[libcamera-devel,v2,08/18] libcamera: software_isp: Add SwStatsCpu class
diff mbox series

Message ID 20240113142218.28063-9-hdegoede@redhat.com
State Superseded
Headers show
Series
  • [libcamera-devel,v2,01/18] libcamera: pipeline: simple: fix size adjustment in validate()
Related show

Commit Message

Hans de Goede Jan. 13, 2024, 2:22 p.m. UTC 
Add a CPU based SwStats implementation for SoftwareISP / SoftIPA use.

Co-authored-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Co-authored-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Pavel Machek <pavel@ucw.cz>
Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
Co-authored-by: Marttico <g.martti@gmail.com>
Signed-off-by: Marttico <g.martti@gmail.com>
Co-authored-by: Toon Langendam <t.langendam@gmail.com>
Signed-off-by: Toon Langendam <t.langendam@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
Tested-by: Pavel Machek <pavel@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

Comments

Kieran Bingham Jan. 13, 2024, 11:02 p.m. UTC | #1 
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@linaro.org>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Co-authored-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Co-authored-by: Marttico <g.martti@gmail.com>
> Signed-off-by: Marttico <g.martti@gmail.com>
> Co-authored-by: Toon Langendam <t.langendam@gmail.com>
> Signed-off-by: Toon Langendam <t.langendam@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@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@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@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
>
Hans de Goede Jan. 14, 2024, 2:14 p.m. UTC | #2 
Hi,

On 1/14/24 00:02, Kieran Bingham wrote:
> 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@linaro.org>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> Co-authored-by: Pavel Machek <pavel@ucw.cz>
>> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>> Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
>> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
>> Co-authored-by: Marttico <g.martti@gmail.com>
>> Signed-off-by: Marttico <g.martti@gmail.com>
>> Co-authored-by: Toon Langendam <t.langendam@gmail.com>
>> Signed-off-by: Toon Langendam <t.langendam@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
>> Tested-by: Pavel Machek <pavel@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@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@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.

The LINARO still being there is my bad. Orginally the simple
implementation of the swisp was called the linaro implementation and
there was linaro everywhere, this was either dropped or replaced with
SIMPLE/CPU and I missed these couple a macros. I'll fix this for
the next version.

Regards,

Hans




> 
> 
>> +       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
>>
>
Pavel Machek Jan. 14, 2024, 4:52 p.m. UTC | #3 
Hi!

> Add a CPU based SwStats implementation for SoftwareISP / SoftIPA use.
> 

> +/* 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;

I believe these two are unused?

								Pavel
Andrei Konovalov Jan. 14, 2024, 5:26 p.m. UTC | #4 
Hi All,

On 14.01.2024 19:52, Pavel Machek wrote:
> Hi!
> 
>> Add a CPU based SwStats implementation for SoftwareISP / SoftIPA use.
>>
> 
>> +/* 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;
> 
> I believe these two are unused?

Right. These are left-overs from the initial implementation of the AE/AGC algorithm.

I am sure I've noticed that earlier, but completely forgot to report that sigh...


Thanks,
Andrey


> 								Pavel
Laurent Pinchart Jan. 23, 2024, 5:16 p.m. UTC | #5 
Hi Hans,

Thank you for the patch.

On Sat, Jan 13, 2024 at 03:22:08PM +0100, Hans de Goede wrote:
> Add a CPU based SwStats implementation for SoftwareISP / SoftIPA use.
> 
> Co-authored-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Co-authored-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Co-authored-by: Marttico <g.martti@gmail.com>
> Signed-off-by: Marttico <g.martti@gmail.com>
> Co-authored-by: Toon Langendam <t.langendam@gmail.com>
> Signed-off-by: Toon Langendam <t.langendam@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@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@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@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 constexpr unsigned int kRedYMul = 77; /* 0.30 * 256 */

I would even suggest

static constexpr unsigned int kRedYMul = std::lround(0.30 * 256);

but lround is constexpr starting in C++23 only :-(

> +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) \
> +	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;

As these macros are only used in a single location, wouldn't the code be
more readable if you just inlined them ?

> +
> +static inline __attribute__((always_inline)) void
> +statsBayer10P(const int width, const uint8_t *src0, const uint8_t *src1, bool bggr, SwIspStats &stats_)

[[gnu::always_inline]] would be the C++ way.

[[gnu::always_inline]]
static 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;

widthInBytes

Same where applicable.

> +
> +	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_);
> +}

These two functions are identical except for the bggr argument they pass
to statsBayer10P(). Can't we store the bayer order in a member variable
and just use statsBayer10P() directly ?

> +
> +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);

If you always emit this signal with 0, you don't have to give it an
argument.

> +}
> +
> +int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
> +{
> +	BayerFormat bayerFormat =
> +		BayerFormat::fromPixelFormat(inputCfg.pixelFormat);
> +
> +	startFrame_ = (SwStats::statsVoidFn)&SwStatsCpu::resetStats;
> +	finishFrame_ = (SwStats::statsVoidFn)&SwStatsCpu::finishStats;

Use virtual functions directly.

> +
> +	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)

Sounds like an error.

> +		<< "Unsupported input format " << inputCfg.pixelFormat.toString();
> +	return -EINVAL;
> +}
> +
> +} /* namespace libcamera */
Milan Zamazal Jan. 23, 2024, 8:04 p.m. UTC | #6 
Hans de Goede <hdegoede@redhat.com> writes:

> Add a CPU based SwStats implementation for SoftwareISP / SoftIPA use.
>
> Co-authored-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Co-authored-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> Co-authored-by: Marttico <g.martti@gmail.com>
> Signed-off-by: Marttico <g.martti@gmail.com>
> Co-authored-by: Toon Langendam <t.langendam@gmail.com>
> Signed-off-by: Toon Langendam <t.langendam@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> Tested-by: Pavel Machek <pavel@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@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@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 */

The last two lines comments don't match the given values.
They should be 0.587 * 256 and 0.114 * 256 to equal.

> +#define SWISP_LINARO_START_LINE_STATS(pixel_t) \
> +	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))]++;

As the size of y_histogram (16) is used in multiple places, it should be a named constant.

> +#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)
> +	}

Does this loop process only every other pixel?  If yes, probably nothing wrong
with it for stats, but it deserves a comment.

> +	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;

This is difficult to understand: why src[1], src[2] and not src[0], src[1]?

> +	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);

Another use of the y_histogram size constant.

> +}
> +
> +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 */

Why?  Worth an explanation in the comment?

> +		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 */
Laurent Pinchart Jan. 23, 2024, 9:55 p.m. UTC | #7 
On Tue, Jan 23, 2024 at 09:04:18PM +0100, Milan Zamazal wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
> 
> > Add a CPU based SwStats implementation for SoftwareISP / SoftIPA use.
> >
> > Co-authored-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > Co-authored-by: Pavel Machek <pavel@ucw.cz>
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > Co-authored-by: Dennis Bonke <admin@dennisbonke.com>
> > Signed-off-by: Dennis Bonke <admin@dennisbonke.com>
> > Co-authored-by: Marttico <g.martti@gmail.com>
> > Signed-off-by: Marttico <g.martti@gmail.com>
> > Co-authored-by: Toon Langendam <t.langendam@gmail.com>
> > Signed-off-by: Toon Langendam <t.langendam@gmail.com>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s
> > Tested-by: Pavel Machek <pavel@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@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@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 */
> 
> The last two lines comments don't match the given values.
> They should be 0.587 * 256 and 0.114 * 256 to equal.
> 
> > +#define SWISP_LINARO_START_LINE_STATS(pixel_t) \
> > +	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))]++;
> 
> As the size of y_histogram (16) is used in multiple places, it should be a named constant.
> 
> > +#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)
> > +	}
> 
> Does this loop process only every other pixel?  If yes, probably nothing wrong
> with it for stats, but it deserves a comment.
> 
> > +	SWISP_LINARO_FINISH_LINE_STATS()
> > +}
> > +
> > +void SwStatsCpu::statsBGGR10PLine0(const uint8_t *src[])

This should take a std::array<const uint8_t *, 3>.

> > +{
> > +	const uint8_t *src0 = src[1] + window_.x * 5 / 4;
> > +	const uint8_t *src1 = src[2] + window_.x * 5 / 4;
> 
> This is difficult to understand: why src[1], src[2] and not src[0], src[1]?
> 
> > +	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);
> 
> Another use of the y_histogram size constant.

You could make the histogram a std::array<unsigned int, 16>.

> > +}
> > +
> > +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 */
> 
> Why?  Worth an explanation in the comment?
> 
> > +		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 */
>
Hans de Goede Feb. 8, 2024, 5:18 p.m. UTC | #8 
Hi Laurent,

Thank you for the review.

On 1/23/24 18:16, Laurent Pinchart wrote:

<snip>

>> +#define SWISP_LINARO_START_LINE_STATS(pixel_t) \
>> +	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;
> 
> As these macros are only used in a single location, wouldn't the code be
> more readable if you just inlined them ?

These get used to avoid copy and pasting the above
over and over when added 8, 10 and 12bpp unpacked
bayer data format in a later patch. This initial patch
only adds 10bpp packed support. Since that is what
both the Qualcomm and IPU6 initial test-cases used.

> [[gnu::always_inline]]
> static 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_);
>> +}
> 
> These two functions are identical except for the bggr argument they pass
> to statsBayer10P(). Can't we store the bayer order in a member variable
> and just use statsBayer10P() directly ?

At first these were 2 different functions with the loop from
statsBayer10P() copies in both functions and then either
the if or the else part of the if (bggr) check in statsBayer10P()
inside the loop.

Pavel suggested adding statsBayer10P() with the parameter
(and always inline it) for some code-reduction.

By having bggr be a const value passed in these 2 wrappers
we allow the compiler to optimize away the if (bggr).

If we replace the if (bggr) where bggr is a const value passed by
the wrappers, with some check based on data stored in the object
we add an extra if *per pixel* and modern pipelined CPUs suffer
greatly from this.

Note when Pavel made this change he also dropped the macros
because back then we still had only 10bpp packed support so
this change reduced the users of the macros to only a single
caller as you already pointed out.

With the macros (which we need to avoid duplication when adding
more formats) the loop is not that big.

I just tried moving the loop back inside statsBGGR10PLine0 /
statsGBRG10PLine0 with the if (bggr) removed and that actually
does not change the line count.

This also make the code more readable and more like the unpacked
bayer fmt support added later (and this also gets rid of the always
inline), so I will go with just removing the statsBayer10P() helper
for v3.

>> +
>> +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);
> 
> If you always emit this signal with 0, you don't have to give it an
> argument.

The Signal<> template requires a dummy argument in this case,
I just tried:

--- a/src/libcamera/software_isp/swstats.h
+++ b/src/libcamera/software_isp/swstats.h
@@ -213,7 +213,7 @@ public:
         *
         * The int parameter isn't actually used.
         */
-	Signal<int> statsReady;
+	Signal<void> statsReady;
 };
 
 } /* namespace libcamera */

And this leads to:

In file included from ../src/libcamera/software_isp/swstats.h:17,
                 from ../src/libcamera/software_isp/swstats.cpp:12:
../include/libcamera/base/signal.h: In instantiation of ‘class libcamera::Signal<void>’:
../src/libcamera/software_isp/swstats.h:216:15:   required from here
../include/libcamera/base/signal.h:49:14: error: invalid parameter type ‘void’
   49 |         void connect(T *obj, R (T::*func)(Args...),
      |              ^~~~~~~
../include/libcamera/base/signal.h:49:14: error: in declaration ‘void libcamera::Signal<Args>::connect(T*, R (T::*)(Args ...), libcamera::ConnectionType)’
../include/libcamera/base/signal.h:60:14: error: invalid parameter type ‘void’
   60 |         void connect(T *obj, R (T::*func)(Args...))
      |              ^~~~~~~
../include/libcamera/base/signal.h:60:14: error: in declaration ‘void libcamera::Signal<Args>::connect(T*, R (T::*)(Args ...))’
../include/libcamera/base/signal.h:93:14: error: invalid parameter type ‘void’
   93 |         void connect(R (*func)(Args...))
      |              ^~~~~~~
../include/libcamera/base/signal.h:93:14: error: in declaration ‘void libcamera::Signal<Args>::connect(R (*)(Args ...))’
../include/libcamera/base/signal.h:114:14: error: invalid parameter type ‘void’
  114 |         void disconnect(T *obj, R (T::*func)(Args...))
      |              ^~~~~~~~~~

and then much more errors ...

<snip>

>> +	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)
> 
> Sounds like an error.
> 
>> +		<< "Unsupported input format " << inputCfg.pixelFormat.toString();
>> +	return -EINVAL;

This may get hit when the simplepipeline is enumerating and a CSI receiver
has an unsupported format. E.g. the IPU6 supports both 10bpp packed / unpacked
and when testing with only the initial patches adding only unpacked support this
gets hit.

Regards,

Hans
Hans de Goede Feb. 12, 2024, 3:48 p.m. UTC | #9 
Hi,

On 1/23/24 21:04, Milan Zamazal wrote:
> Hans de Goede <hdegoede@redhat.com> writes:

<snip>

>> +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 */
> 
> The last two lines comments don't match the given values.
> They should be 0.587 * 256 and 0.114 * 256 to equal.

I've fixed the comment to use the usual 0.299, 0.587, 0, 114 values
now, thanks.

>> +#define SWISP_LINARO_START_LINE_STATS(pixel_t) \
>> +	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))]++;
> 
> As the size of y_histogram (16) is used in multiple places, it should be a named constant.

Ack, fixed.

>> +#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)
>> +	}
> 
> Does this loop process only every other pixel?  If yes, probably nothing wrong
> with it for stats, but it deserves a comment.

Yes it processes only every other pixel I've added a comment for this.

> 
>> +	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;
> 
> This is difficult to understand: why src[1], src[2] and not src[0], src[1]?

I have added the following comment to the statsProcessFn:

        /**
         * \brief Called when there is data to get statistics from.
         * \param[in] src The input data
         *
         * These functions take an array of (patternSize_.height + 1) src
         * pointers each pointing to a line in the source image. The middle
         * element of the array will point to the actual line being processed.
         * Earlier element(s) will point to the previous line(s) and later
         * element(s) to the next line(s).
         *
         * See the documentation of DebayerCpu::debayerFn for more details.
         */
        typedef void (SwStatsCpu::*statsProcessFn)(const uint8_t *src[]);

And the following comment to DebayerCpu::debayerFn:

	/**
	 * \brief Called to debayer 1 line of Bayer input data to output format
	 * \param[out] dst Pointer to the start of the output line to write
	 * \param[in] src The input data
	 *
	 * Input data is an array of (patternSize_.height + 1) src
	 * pointers each pointing to a line in the Bayer source. The middle
	 * element of the array will point to the actual line being processed.
	 * Earlier element(s) will point to the previous line(s) and later
	 * element(s) to the next line(s).
	 *
	 * These functions take an array of src pointers, rather then
	 * a single src pointer + a stride for the source, so that when the src
	 * is slow uncached memory it can be copied to faster memory before
	 * debayering. Debayering a standard 2x2 Bayer pattern requires access
	 * to the previous and next src lines for interpolating the missing
	 * colors. To allow copying the src lines only once 3 buffers each
	 * holding a single line are used, re-using the oldest buffer for
	 * the next line and the pointers are swizzled so that:
	 * src[0] = previous-line, src[1] = currrent-line, src[2] = next-line.
	 * This way the 3 pointers passed to the debayer functions form
	 * a sliding window over the src avoiding the need to copy each
	 * line more then once.
	 *
	 * Similarly for bayer patterns which repeat every 4 lines, 5 src
	 * pointers are passed holding: src[0] = 2-lines-up, src[1] = 1-line-up
	 * src[2] = current-line, src[3] = 1-line-down, src[4] = 2-lines-down.
	 */
	typedef void (DebayerCpu::*debayerFn)(uint8_t *dst, const uint8_t *src[]);

Regards,

Hans
Hans de Goede Feb. 12, 2024, 3:51 p.m. UTC | #10 
Hi,

On 1/23/24 22:55, Laurent Pinchart wrote:
> On Tue, Jan 23, 2024 at 09:04:18PM +0100, Milan Zamazal wrote:
>> Hans de Goede <hdegoede@redhat.com> writes:

<sni>

>>> +void SwStatsCpu::statsBGGR10PLine0(const uint8_t *src[])
> 
> This should take a std::array<const uint8_t *, 3>.

That won't work since the same function pointer is also used
for Bayer patterns which repeat every 4 lines and in that
case 5 src line pointers are passed.

>>> +void SwStatsCpu::resetStats(void)
>>> +{
>>> +	stats_.sumR_ = 0;
>>> +	stats_.sumB_ = 0;
>>> +	stats_.sumG_ = 0;
>>> +	std::fill_n(stats_.y_histogram, 16, 0);
>>
>> Another use of the y_histogram size constant.
> 
> You could make the histogram a std::array<unsigned int, 16>.

Ack, fixed.

Regards,

Hans

Patch
diff mbox series 

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@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@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) \
+	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 */