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

20048 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 :-)

Cheers,
Stefan

 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

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

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

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


> +		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',
>   ])

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@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',
> >   ])