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

20083 diff mbox series

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@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

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@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,
> +                          512, 512, 512, 512, 512, 0 };
> +       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;
> +       for (unsigned i = 0; i < context.hw->numGammaOutSamples; i++) {
> +               gamma_y[i] = std::pow(x / 4096.0, 1.0 / gamma_) * 1023.0;
> +               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;
> +};
> +
> +} /* 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
>

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@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
> >