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

[Dan Scally]

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.

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