[libcamera-devel] [PATCH v2 08/18] libcamera: software_isp: Add SwStatsCpu class
Kieran Bingham
kieran.bingham at ideasonboard.com
Sun Jan 14 00:02:18 CET 2024
Quoting Hans de Goede via libcamera-devel (2024-01-13 14:22:08)
> Add a CPU based SwStats implementation for SoftwareISP / SoftIPA use.
>
> Co-authored-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> Co-authored-by: Pavel Machek <pavel at ucw.cz>
> Signed-off-by: Pavel Machek <pavel at ucw.cz>
> Co-authored-by: Dennis Bonke <admin at dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin at dennisbonke.com>
> Co-authored-by: Marttico <g.martti at gmail.com>
> Signed-off-by: Marttico <g.martti at gmail.com>
> Co-authored-by: Toon Langendam <t.langendam at gmail.com>
> Signed-off-by: Toon Langendam <t.langendam at gmail.com>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue at linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel at ucw.cz>
> ---
> .../internal/software_isp/meson.build | 1 +
> .../internal/software_isp/swstats_cpu.h | 44 +++++
> src/libcamera/software_isp/meson.build | 1 +
> src/libcamera/software_isp/swstats_cpu.cpp | 164 ++++++++++++++++++
> 4 files changed, 210 insertions(+)
> create mode 100644 include/libcamera/internal/software_isp/swstats_cpu.h
> create mode 100644 src/libcamera/software_isp/swstats_cpu.cpp
>
> diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build
> index 1c43acc4..1d9e4018 100644
> --- a/include/libcamera/internal/software_isp/meson.build
> +++ b/include/libcamera/internal/software_isp/meson.build
> @@ -3,4 +3,5 @@
> libcamera_internal_headers += files([
> 'swisp_stats.h',
> 'swstats.h',
> + 'swstats_cpu.h',
> ])
> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h
> new file mode 100644
> index 00000000..8bb86e98
> --- /dev/null
> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + * Copyright (C) 2023, Red Hat Inc.
> + *
> + * Authors:
> + * Hans de Goede <hdegoede at redhat.com>
> + *
> + * swstats_cpu.h - CPU based software statistics implementation
> + */
> +
> +#pragma once
> +
> +#include "libcamera/internal/shared_mem_object.h"
> +#include "libcamera/internal/software_isp/swisp_stats.h"
> +#include "libcamera/internal/software_isp/swstats.h"
> +
> +namespace libcamera {
> +
> +/**
> + * \class SwStatsCpu
> + * \brief Implementation for the Software statistics on the CPU.
> + */
> +class SwStatsCpu : public SwStats
> +{
> +public:
> + SwStatsCpu();
> + ~SwStatsCpu() { }
> +
> + bool isValid() const { return sharedStats_.fd().isValid(); }
> + const SharedFD &getStatsFD() { return sharedStats_.fd(); }
> + int configure(const StreamConfiguration &inputCfg);
> +private:
> + void statsBGGR10PLine0(const uint8_t *src[]);
> + void statsGBRG10PLine0(const uint8_t *src[]);
> + void resetStats(void);
> + void finishStats(void);
> +
> + SharedMemObject<SwIspStats> sharedStats_;
> + SwIspStats stats_;
> + bool swap_lines_;
> +};
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build
> index 9359075d..d31c6217 100644
> --- a/src/libcamera/software_isp/meson.build
> +++ b/src/libcamera/software_isp/meson.build
> @@ -2,4 +2,5 @@
>
> libcamera_sources += files([
> 'swstats.cpp',
> + 'swstats_cpu.cpp',
> ])
> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
> new file mode 100644
> index 00000000..59453d07
> --- /dev/null
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -0,0 +1,164 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Linaro Ltd
> + * Copyright (C) 2023, Red Hat Inc.
> + *
> + * Authors:
> + * Hans de Goede <hdegoede at redhat.com>
> + *
> + * swstats_cpu.cpp - CPU based software statistics implementation
> + */
> +
> +#include "libcamera/internal/software_isp/swstats_cpu.h"
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/stream.h>
> +
> +#include "libcamera/internal/bayer_format.h"
> +
> +namespace libcamera {
> +
> +SwStatsCpu::SwStatsCpu()
> + : SwStats()
> +{
> + sharedStats_ = SharedMemObject<SwIspStats>("softIsp_stats");
> + if (!sharedStats_.fd().isValid())
> + LOG(SwStats, Error)
> + << "Failed to create shared memory for statistics";
> +}
> +
> +/* for brightness values in the 0 to 255 range: */
> +static const unsigned int BRIGHT_LVL = 200U << 8;
> +static const unsigned int TOO_BRIGHT_LVL = 240U << 8;
> +
> +static const unsigned int RED_Y_MUL = 77; /* 0.30 * 256 */
> +static const unsigned int GREEN_Y_MUL = 150; /* 0.59 * 256 */
> +static const unsigned int BLUE_Y_MUL = 29; /* 0.11 * 256 */
> +
> +#define SWISP_LINARO_START_LINE_STATS(pixel_t) \
I get that Linaro may have written parts of this... but why is LINARO
scattered everywhere? Does this code only work on Linaro hardware for
linaro pixels?
Do we need equivalent definitions for
SWISP_{CANONICAL,REDHAT,FERRARI}_START_LINE_STATS?
I don't want to take 'credit' away from anyone doing this work, but I
don't like the idea of vendoring a generic software implementation here.
The macros are more specific to this SwISP implementation than to
Linaro.
If you need to put a name in, we could use _FERRARI_ to make the code
run faster (for cost premium of course) :D
Given that's the extend of my complaint on this patch, aside from that
I'd hope we can see this go in fairly easily otherwise.
> + pixel_t r, g, g2, b; \
> + unsigned int y_val; \
> + \
> + unsigned int sumR = 0; \
> + unsigned int sumG = 0; \
> + unsigned int sumB = 0;
> +
> +#define SWISP_LINARO_ACCUMULATE_LINE_STATS(div) \
> + sumR += r; \
> + sumG += g; \
> + sumB += b; \
> + \
> + y_val = r * RED_Y_MUL; \
> + y_val += g * GREEN_Y_MUL; \
> + y_val += b * BLUE_Y_MUL; \
> + stats_.y_histogram[y_val / (256 * 16 * (div))]++;
> +
> +#define SWISP_LINARO_FINISH_LINE_STATS() \
> + stats_.sumR_ += sumR; \
> + stats_.sumG_ += sumG; \
> + stats_.sumB_ += sumB;
> +
> +static inline __attribute__((always_inline)) void
> +statsBayer10P(const int width, const uint8_t *src0, const uint8_t *src1, bool bggr, SwIspStats &stats_)
> +{
> + const int width_in_bytes = width * 5 / 4;
> +
> + SWISP_LINARO_START_LINE_STATS(uint8_t)
> +
> + for (int x = 0; x < width_in_bytes; x += 5) {
> + if (bggr) {
> + /* BGGR */
> + b = src0[x];
> + g = src0[x + 1];
> + g2 = src1[x];
> + r = src1[x + 1];
> + } else {
> + /* GBRG */
> + g = src0[x];
> + b = src0[x + 1];
> + r = src1[x];
> + g2 = src1[x + 1];
> + }
> + g = (g + g2) / 2;
> +
> + SWISP_LINARO_ACCUMULATE_LINE_STATS(1)
> + }
> +
> + SWISP_LINARO_FINISH_LINE_STATS()
> +}
> +
> +void SwStatsCpu::statsBGGR10PLine0(const uint8_t *src[])
> +{
> + const uint8_t *src0 = src[1] + window_.x * 5 / 4;
> + const uint8_t *src1 = src[2] + window_.x * 5 / 4;
> +
> + if (swap_lines_)
> + std::swap(src0, src1);
> +
> + statsBayer10P(window_.width, src0, src1, true, stats_);
> +}
> +
> +void SwStatsCpu::statsGBRG10PLine0(const uint8_t *src[])
> +{
> + const uint8_t *src0 = src[1] + window_.x * 5 / 4;
> + const uint8_t *src1 = src[2] + window_.x * 5 / 4;
> +
> + if (swap_lines_)
> + std::swap(src0, src1);
> +
> + statsBayer10P(window_.width, src0, src1, false, stats_);
> +}
> +
> +void SwStatsCpu::resetStats(void)
> +{
> + stats_.sumR_ = 0;
> + stats_.sumB_ = 0;
> + stats_.sumG_ = 0;
> + std::fill_n(stats_.y_histogram, 16, 0);
> +}
> +
> +void SwStatsCpu::finishStats(void)
> +{
> + *sharedStats_ = stats_;
> + statsReady.emit(0);
> +}
> +
> +int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
> +{
> + BayerFormat bayerFormat =
> + BayerFormat::fromPixelFormat(inputCfg.pixelFormat);
> +
> + startFrame_ = (SwStats::statsVoidFn)&SwStatsCpu::resetStats;
> + finishFrame_ = (SwStats::statsVoidFn)&SwStatsCpu::finishStats;
> +
> + if (bayerFormat.bitDepth == 10 &&
> + bayerFormat.packing == BayerFormat::Packing::CSI2) {
> + bpp_ = 10;
> + patternSize_.height = 2;
> + patternSize_.width = 4; /* 5 bytes per *4* pixels */
> + y_skip_mask_ = 0x02; /* Skip every 3th and 4th line */
> + x_shift_ = 0;
> +
> + switch (bayerFormat.order) {
> + case BayerFormat::BGGR:
> + case BayerFormat::GRBG:
> + stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsBGGR10PLine0;
> + swap_lines_ = bayerFormat.order == BayerFormat::GRBG;
> + return 0;
> + case BayerFormat::GBRG:
> + case BayerFormat::RGGB:
> + stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsGBRG10PLine0;
> + swap_lines_ = bayerFormat.order == BayerFormat::RGGB;
> + return 0;
> + default:
> + break;
> + }
> + }
> +
> + LOG(SwStats, Info)
> + << "Unsupported input format " << inputCfg.pixelFormat.toString();
> + return -EINVAL;
> +}
> +
> +} /* namespace libcamera */
> --
> 2.43.0
>
More information about the libcamera-devel
mailing list