[PATCH v2 1/4] ipa: rkisp1: Add gamma algorithm

[Jacopo Mondi]

Hi Stefan

On Wed, May 22, 2024 at 11:40:15PM GMT, Kieran Bingham wrote:
> Quoting Stefan Klug (2024-05-22 15:54:35)
> > Add a gamma algorithm for the rkisp1. It defaults to a gamma of 2.2 which
> > closely resembles sRGB.  No seperate sRGB mode was implemented because the pwl
> > that models the gamma curve is so coarse that there is basically no difference
> > between srgb and gamma=2.2
> >
> > The gamma algorithm is not enabled by default. This will be done in future
> > tuning file updates.
> >
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >
> > Note:
> >
> > This patch breaks the naming conventions. It is implemented inside goc.h/cpp
> > because the hardware block and params inside the rkisp1 are called "goc". The
> > class itself is called Gamma, and the algorithm is registered with the name
> > "Gamma". The idea was that similar functionalities should be named the same
> > inside the tuning files (even among multipe isps).  It still feels a bit
> > awkward. So thoughts are welcome :-)
> >
> >
> > v1 -> v2:
> > - fixed some style issues
> > - fail in case of a V12 isp
> >
> >  src/ipa/rkisp1/algorithms/goc.cpp     | 100 ++++++++++++++++++++++++++
> >  src/ipa/rkisp1/algorithms/goc.h       |  32 +++++++++
> >  src/ipa/rkisp1/algorithms/meson.build |   1 +
> >  3 files changed, 133 insertions(+)
> >  create mode 100644 src/ipa/rkisp1/algorithms/goc.cpp
> >  create mode 100644 src/ipa/rkisp1/algorithms/goc.h
> >
> > diff --git a/src/ipa/rkisp1/algorithms/goc.cpp b/src/ipa/rkisp1/algorithms/goc.cpp
> > new file mode 100644
> > index 00000000..6f313820
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/goc.cpp
> > @@ -0,0 +1,100 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * RkISP1 Gamma out control
> > + */
> > +#include "goc.h"
> > +
> > +#include <cmath>
> > +
> > +#include <libcamera/base/log.h>
> > +#include <libcamera/base/utils.h>
> > +
> > +#include "libcamera/internal/yaml_parser.h"
> > +
> > +#include "linux/rkisp1-config.h"
> > +
> > +/**
> > + * \file goc.h
> > + */
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1::algorithms {
> > +
> > +/**
> > + * \class Gamma
> > + * \brief RkISP1 Gamma out control
> > + *
> > + * This algorithm implements the gamma out curve for the RkISP1.
> > + * It defaults to a gamma value of 2.2
> > + * As gamma is internally represented as a piecewise linear function with only
> > + * 16 knots, the difference between gamma=2.2 and sRGB gamma is minimal.
> > + * Therefore sRGB gamma was not implemented as special case.
> > + *
> > + * Useful links:
> > + * https://www.cambridgeincolour.com/tutorials/gamma-correction.htm
> > + * https://en.wikipedia.org/wiki/SRGB
> > + */
> > +
> > +LOG_DEFINE_CATEGORY(RkISP1Gamma)
> > +
> > +Gamma::Gamma()
> > +{
> > +}
>
> I see this and think why not = default ... but I see it gets updated
> later ...
>
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::init
> > + */
> > +int Gamma::init([[maybe_unused]] IPAContext &context,
> > +               [[maybe_unused]] const YamlObject &tuningData)
> > +{
> > +       if (context.hw->numGammaOutSamples !=
> > +           RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10) {
> > +               LOG(RkISP1Gamma, Error)
> > +                       << "Gamma is not implemented for RkISP1 V12";
> > +               return -EINVAL;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::prepare
> > + */
> > +void Gamma::prepare([[maybe_unused]] IPAContext &context,
> > +                   const uint32_t frame,
> > +                   [[maybe_unused]] IPAFrameContext &frameContext,
> > +                   rkisp1_params_cfg *params)
> > +{
> > +       /* The logarithmic segments as specified in the reference.
>
> 	/*
> 	 * nit: multiline comments have an empty first and last
> 	 * line for the leading '/*' and trailing '*/'
> 	 */
>
> > +        * Plus an additional 0 to make the loop easier
> > +        */
> > +       int segments[] = { 64, 64, 64, 64, 128, 128, 128, 128, 256, 256, 256,

Any reason for using int instead of unsigned ?

Also, this has to have a fixed size which corresponds to
RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10. std::array<> is safer in
this case

> > +                          512, 512, 512, 512, 512, 0 };

This gamma curve segmentation only applies if
ISP_GAMMA_OUT_MODE[0] = 0, which is the default. Should this be
configured or do we assume POR value for ISP registers ?

> > +       auto gamma_y = params->others.goc_config.gamma_y;
> > +
> > +       ASSERT(context.hw->numGammaOutSamples ==
> > +              RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10);
> > +
> > +       if (frame > 0)
> > +               return;
> > +
> > +       int x = 0;

Also this is an unsigned

> > +       for (unsigned i = 0; i < context.hw->numGammaOutSamples; i++) {

And if you utils::enumerate(segments) you make sure you can't get out
of bounds

> > +               gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;

This curve uses the same 2.2 gamma and the same segments as the
default programmed gamma curve in the POR register values ?

> > +               x += segments[i];
> > +       }
> > +
> > +       params->others.goc_config.mode = RKISP1_CIF_ISP_GOC_MODE_LOGARITHMIC;
> > +       params->module_en_update |= RKISP1_CIF_ISP_MODULE_GOC;
> > +       params->module_ens |= RKISP1_CIF_ISP_MODULE_GOC;
> > +       params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_GOC;
> > +}
> > +
> > +REGISTER_IPA_ALGORITHM(Gamma, "Gamma")
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/goc.h b/src/ipa/rkisp1/algorithms/goc.h
> > new file mode 100644
> > index 00000000..fe7caba3
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/goc.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas On Board
> > + *
> > + * RkISP1 Gamma out control
> > + */
> > +
> > +#pragma once
> > +
> > +#include "algorithm.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::rkisp1::algorithms {
> > +
> > +class Gamma : public Algorithm
> > +{
> > +public:
> > +       Gamma();
> > +       ~Gamma() = default;
> > +
> > +       int init(IPAContext &context, const YamlObject &tuningData) override;
> > +       void prepare(IPAContext &context, const uint32_t frame,
> > +                    IPAFrameContext &frameContext,
> > +                    rkisp1_params_cfg *params) override;
> > +
> > +private:
> > +       float gamma_ = 2.2;

this might be a static constexpr

To be honest I would make the segments a static constexpr std::array<>
as they come from the HW (and we can have different ones depending on
the HW revision maybe ?)

Thanks
  j

> > +};
> > +
> > +} /* namespace ipa::rkisp1::algorithms */
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> > index 93a48329..6ee71a9b 100644
> > --- a/src/ipa/rkisp1/algorithms/meson.build
> > +++ b/src/ipa/rkisp1/algorithms/meson.build
> > @@ -8,6 +8,7 @@ rkisp1_ipa_algorithms = files([
> >      'dpcc.cpp',
> >      'dpf.cpp',
> >      'filter.cpp',
> > +    'goc.cpp',
> >      'gsl.cpp',
> >      'lsc.cpp',
> >  ])
> > --
> > 2.40.1
> >