[PATCH 1/3] ipa: rkisp1: Add gamma algorithm

[Stefan Klug]

Hi Dan,

thanks for the review.

On Mon, May 20, 2024 at 11:42:52AM +0100, Dan Scally wrote:
> Hi Stefan - thanks for the patch
> 
> On 16/05/2024 13:41, Stefan Klug wrote:
> > 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 :-)
> > 
> > Cheers,
> > Stefan
> 
> 
> Is it worth a libipa implementation? Handling the gamma control and building
> the PWL array from the input gamma value should be basically the same across
> most of the IPAs, though the logarithmic mode here perhaps makes that not
> worthwhile.

Yes, that could be a solution. I'll give it a try.

> 
> > 
> >   src/ipa/rkisp1/algorithms/goc.cpp     | 89 +++++++++++++++++++++++++++
> >   src/ipa/rkisp1/algorithms/goc.h       | 32 ++++++++++
> >   src/ipa/rkisp1/algorithms/meson.build |  1 +
> >   3 files changed, 122 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..1598d730
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/goc.cpp
> > @@ -0,0 +1,89 @@
> > +/* 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 gsl.h
> > + */
> s/gsl/goc?
> > +
> > +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()
> > +{
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::init
> > + */
> > +int Gamma::init([[maybe_unused]] IPAContext &context,
> > +		[[maybe_unused]] const YamlObject &tuningData)
> > +{
> > +	return 0;
> > +}
> 
> 
> You don't need to provide the function if it does nothing I think?

Oh yes, sure.

> 
> > +
> > +/**
> > + * \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.
> > +	   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;
> > +
> > +	if (frame > 0)
> > +		return;
> > +
> > +	int x = 0;
> > +	for (int i = 0; i < RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10; i++) {
> 
> 
> Does this need to be done differently for V12? What happens if we half-fill the LUT for V12 revisions?

Yes, it would need to be done differently. I don't have a V12 based
device, so I'm not able to tests it. I could do a dry implementation and
issue a warning, or error out in case of the V12. I'll have a look.

Cheers,
Stefan

> 
> 
> > +		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..3c83655b
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/algorithms/goc.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021-2022, Ideas On Board
> > + *
> > + * gsl.h - RkISP1 Gamma Sensor Linearization control
> > + */
> s/gsl/goc
> > +
> > +#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',
> >   ])