[libcamera-devel,v2,05/10] ipa: ipu3: Introduce a modular contrast algorithm

13333 diff mbox series

Introduce a new algorithm to manage the contrast handling of the IPU3.

The initial algorithm is chosen to configure the gamma contrast curve
which moves the implementation out of AWB for simplicity. As it is
initialised with a default gamma value of 1.1, there is no need to use
the default table at init time anymore.

This demonstrates the way to use process() call when the EventStatReady
comes in. The function calculates the LUT in the context of a frame, and
when prepare() is called, the parameters are filled with the updated
values.

AGC needs to be modified to take the new process interface into account.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/algorithm.h  |  2 ++
 src/ipa/ipu3/algorithms/contrast.cpp | 51 ++++++++++++++++++++++++++++
 src/ipa/ipu3/algorithms/contrast.h   | 33 ++++++++++++++++++
 src/ipa/ipu3/algorithms/meson.build  |  3 +-
 src/ipa/ipu3/ipa_context.h           |  8 +++++
 src/ipa/ipu3/ipu3.cpp                | 12 +++++--
 src/ipa/ipu3/ipu3_agc.cpp            |  9 +++--
 src/ipa/ipu3/ipu3_agc.h              |  5 +--
 src/ipa/ipu3/ipu3_awb.cpp            | 41 ++--------------------
 src/ipa/ipu3/ipu3_awb.h              |  2 +-
 10 files changed, 114 insertions(+), 52 deletions(-)
 create mode 100644 src/ipa/ipu3/algorithms/contrast.cpp
 create mode 100644 src/ipa/ipu3/algorithms/contrast.h

On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
> Introduce a new algorithm to manage the contrast handling of the IPU3.
> 
> The initial algorithm is chosen to configure the gamma contrast curve

I don't think this is the 'initial algorithm' any more ...


> which moves the implementation out of AWB for simplicity. As it is
> initialised with a default gamma value of 1.1, there is no need to use
> the default table at init time anymore.

s/at init time/at initialisation/
> 
> This demonstrates the way to use process() call when the EventStatReady
> comes in. The function calculates the LUT in the context of a frame, and
> when prepare() is called, the parameters are filled with the updated
> values.
> 
> AGC needs to be modified to take the new process interface into account.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/algorithm.h  |  2 ++
>  src/ipa/ipu3/algorithms/contrast.cpp | 51 ++++++++++++++++++++++++++++
>  src/ipa/ipu3/algorithms/contrast.h   | 33 ++++++++++++++++++
>  src/ipa/ipu3/algorithms/meson.build  |  3 +-
>  src/ipa/ipu3/ipa_context.h           |  8 +++++
>  src/ipa/ipu3/ipu3.cpp                | 12 +++++--
>  src/ipa/ipu3/ipu3_agc.cpp            |  9 +++--
>  src/ipa/ipu3/ipu3_agc.h              |  5 +--
>  src/ipa/ipu3/ipu3_awb.cpp            | 41 ++--------------------
>  src/ipa/ipu3/ipu3_awb.h              |  2 +-
>  10 files changed, 114 insertions(+), 52 deletions(-)
>  create mode 100644 src/ipa/ipu3/algorithms/contrast.cpp
>  create mode 100644 src/ipa/ipu3/algorithms/contrast.h
> 
> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> index b571f55a..13e5f2d4 100644
> --- a/src/ipa/ipu3/algorithms/algorithm.h
> +++ b/src/ipa/ipu3/algorithms/algorithm.h
> @@ -28,6 +28,8 @@ public:
>  	}
>  
>  	virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params &params) {}
> +
> +	virtual void process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats) {}
>  };
>  
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/algorithms/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp
> new file mode 100644
> index 00000000..bc00490f
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/contrast.cpp
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board

Keep this consistent with contrast.h please.


> + *
> + * constrast.cpp - IPU3 Contrast and Gamma control

s/constrast/contrast/

> + */
> +
> +#include "contrast.h"
> +
> +#include <cmath>
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3::algorithms {
> +
> +LOG_DEFINE_CATEGORY(IPU3Contrast)
> +
> +Contrast::Contrast()
> +	: gamma_(1.0)
> +{
> +}
> +
> +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params &params)
> +{
> +	params.use.acc_gamma = 1;
> +	params.acc_param.gamma.gc_ctrl.enable = 1;

The ordering is almost irrelevant to the code, but for some reason, I
think you should set the enable and use flags /after/ filling in the data.

Technically it doesn't matter, but conceptually, they should only be
enabled, after they are filled in correctly.


> +
> +	for (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++)
> +		params.acc_param.gamma.gc_lut.lut[i] = context.frameContext.contrast.lut[i];

We have two tables of a fixed number of entries.
I wonder if a memcpy() would be more efficient than a loop, but perhaps
it doesn't make a difference ?

Would the compiler do the right thing if you said:

  params.acc_param.gamma.gc_lut.lut = context.frameContext.contrast.lut;

In fact, I see that
  params.acc_param.gamma.gc_lut.lut stores unsigned shorts,
while
  context.frameContext.contrast.lut stores uint16_t

Maybe we should store a
	struct ipu3_uapi_gamma_corr_lut

in context.frameContext.contrast instead, Yes they're all the same size
types - but we have access to the original type of the array, so why not
use it to keep them identical..



> +}
> +
> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> +{
> +	/* Limit the gamma effect for now */

Can this be explained any more? Why do we set this here and not
initialise to 1.1 in the initialisation list?

Perhaps a
   \todo expose gamma control setting through the libcamera control API

should be added here too to make it clear that this is hardcoded, when
it should somehow be a parameter or control into the IPA or such.


> +	gamma_ = 1.1;
> +
> +	/* Plot the gamma curve into the look up table */
> +	for (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++) {
> +		double j = i / 255.0;
> +		double gamma = std::pow(j, 1.0 / gamma_);
> +
> +		/* The maximum value 255 is represented on 13 bits in the IPU3 */
> +		context.frameContext.contrast.lut[i] = gamma * 8191;

8191 still looks like a magic number, even with the comment.
I wonder if we should define this somewhere, perhaps as


/* The maximum value 255 is represented on 13 bits in the IPU3 */
#define LUT_INTEGER_SCALE(g) (g * ((1 << 13) - 1))

	context.frameContext.contrast.lut[i] = LUT_INTEGER_SCALE(gamma);


LUT_INTEGER_SCALE may be a completely inappropriate name ... I'm guessing.


Also - even having said that - this patch is moving existing code - so
this shouldn't be changed here - but it might be a distinct patch of
it's own to come later.




> +	}
> +}
> +
> +} /* namespace ipa::ipu3::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/ipu3/algorithms/contrast.h b/src/ipa/ipu3/algorithms/contrast.h
> new file mode 100644
> index 00000000..50cb626f
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/contrast.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google
> + *
> + * constrast.h - IPU3 Contrast and Gamma control
> + */
> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__
> +#define __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__
> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3::algorithms {
> +
> +class Contrast : public Algorithm
> +{
> +public:
> +	Contrast();
> +	~Contrast() = default;
> +
> +	void prepare(IPAContext &context, ipu3_uapi_params &params) override;
> +	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> +
> +private:
> +	double gamma_;
> +};
> +
> +} /* namespace ipa::ipu3::algorithms */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__ */
> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> index 3fb3ce56..69a59096 100644
> --- a/src/ipa/ipu3/algorithms/meson.build
> +++ b/src/ipa/ipu3/algorithms/meson.build
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  ipu3_ipa_algorithms = files([
> -        'grid.cpp',

Please fix this alignment in the patch that adds it.



> +    'contrast.cpp',
> +    'grid.cpp',
>  ])
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 90b2f2c2..e995c663 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -33,6 +33,14 @@ struct IPAConfiguration {
>   * lifetime, though for now a single instance is used.
>   */
>  struct IPAFrameContext {
> +	struct Agc {
> +		uint32_t exposure;
> +		double gain;
> +	} agc;
> +
> +	struct Contrast {
> +		uint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];

As highilghted earlier, I think this should be:
		struct ipu3_uapi_gamma_corr_lut lut


> +	} contrast;
>  };
>  
>  /* Global context of the IPA */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 394a7a45..e09adfc3 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -30,6 +30,7 @@
>  #include "libcamera/internal/mapped_framebuffer.h"
>  
>  #include "algorithms/algorithm.h"
> +#include "algorithms/contrast.h"
>  #include "algorithms/grid.h"
>  #include "ipu3_agc.h"
>  #include "ipu3_awb.h"
> @@ -167,6 +168,7 @@ int IPAIPU3::init(const IPASettings &settings,
>  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
>  	/* Construct our Algorithms */
>  	algorithms_.emplace_back(new algorithms::Grid());
> +	algorithms_.emplace_back(new algorithms::Contrast());
>  
>  	return 0;
>  }
> @@ -309,7 +311,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  		algo->prepare(context_, params_);
>  
>  	if (agcAlgo_->updateControls())
> -		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
> +		awbAlgo_->updateWbParameters(params_);
>  
>  	*params = params_;
>  
> @@ -324,9 +326,15 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>  {
>  	ControlList ctrls(controls::controls);

New line here to separate local variable declarations from code segments.

> +	for (auto const &algo : algorithms_)
> +		algo->process(context_, stats);
>  
>  	double gain = camHelper_->gain(gain_);
> -	agcAlgo_->process(stats, exposure_, gain);
> +	context_.frameContext.agc.exposure = exposure_;
> +	context_.frameContext.agc.gain = gain;

the IPU3 layer sould not be filling in agc specific structures. That's a
layering violation to me.

The algorithms (in this instance, the agc?) own those structures, so
they should be manipulating them.

It's also showing where the algorithms need to know historical data.
In this instance, presumably they need to know what gain or expsosure
was set at the time that this buffer ran.

This shows where we will need to keep historical versions of the
FrameContext, and perhaps track them along with the frame counter.

For now, while we get things moving to be more modular, I think we could
keep this as it is - but it should be commented with a todo


> +	agcAlgo_->process(context_, stats);
> +	exposure_ = context_.frameContext.agc.exposure;
> +	gain = context_.frameContext.agc.gain;
>  	gain_ = camHelper_->gainCode(gain);
>  
>  	awbAlgo_->calculateWBGains(stats);
> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> index 408eb849..c6782ff4 100644
> --- a/src/ipa/ipu3/ipu3_agc.cpp
> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> @@ -52,7 +52,7 @@ static constexpr uint8_t kCellSize = 8;
>  
>  IPU3Agc::IPU3Agc()
>  	: frameCount_(0), lastFrame_(0), converged_(false),
> -	  updateControls_(false), iqMean_(0.0), gamma_(1.0),
> +	  updateControls_(false), iqMean_(0.0),
>  	  lineDuration_(0s), maxExposureTime_(0s),
>  	  prevExposure_(0s), prevExposureNoDg_(0s),
>  	  currentExposure_(0s), currentExposureNoDg_(0s)
> @@ -104,9 +104,6 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>  		}
>  	}
>  
> -	/* Limit the gamma effect for now */
> -	gamma_ = 1.1;
> -
>  	/* Estimate the quantile mean of the top 2% of the histogram */
>  	iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>  }
> @@ -193,8 +190,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  	lastFrame_ = frameCount_;
>  }
>  
> -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain)
> +void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>  {
> +	uint32_t &exposure = context.frameContext.agc.exposure;
> +	double &gain = context.frameContext.agc.gain;
>  	processBrightness(stats);
>  	lockExposureGain(exposure, gain);
>  	frameCount_++;
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> index f00b98d6..8b32e52f 100644
> --- a/src/ipa/ipu3/ipu3_agc.h
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -30,11 +30,9 @@ public:
>  	~IPU3Agc() = default;
>  
>  	void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);
> -	void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);
> +	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>  	bool converged() { return converged_; }
>  	bool updateControls() { return updateControls_; }
> -	/* \todo Use a metadata exchange between IPAs */
> -	double gamma() { return gamma_; }
>  
>  private:
>  	void processBrightness(const ipu3_uapi_stats_3a *stats);
> @@ -50,7 +48,6 @@ private:
>  	bool updateControls_;
>  
>  	double iqMean_;
> -	double gamma_;
>  
>  	Duration lineDuration_;
>  	Duration maxExposureTime_;
> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> index 911760b3..0f3bcdc9 100644
> --- a/src/ipa/ipu3/ipu3_awb.cpp
> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> @@ -114,31 +114,6 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
>  	0, 0, 8191, 0
>  };
>  
> -/* Default settings for Gamma correction */
> -const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {
> -	63, 79, 95, 111, 127, 143, 159, 175, 191, 207, 223, 239, 255, 271, 287,
> -	303, 319, 335, 351, 367, 383, 399, 415, 431, 447, 463, 479, 495, 511,
> -	527, 543, 559, 575, 591, 607, 623, 639, 655, 671, 687, 703, 719, 735,
> -	751, 767, 783, 799, 815, 831, 847, 863, 879, 895, 911, 927, 943, 959,
> -	975, 991, 1007, 1023, 1039, 1055, 1071, 1087, 1103, 1119, 1135, 1151,
> -	1167, 1183, 1199, 1215, 1231, 1247, 1263, 1279, 1295, 1311, 1327, 1343,
> -	1359, 1375, 1391, 1407, 1423, 1439, 1455, 1471, 1487, 1503, 1519, 1535,
> -	1551, 1567, 1583, 1599, 1615, 1631, 1647, 1663, 1679, 1695, 1711, 1727,
> -	1743, 1759, 1775, 1791, 1807, 1823, 1839, 1855, 1871, 1887, 1903, 1919,
> -	1935, 1951, 1967, 1983, 1999, 2015, 2031, 2047, 2063, 2079, 2095, 2111,
> -	2143, 2175, 2207, 2239, 2271, 2303, 2335, 2367, 2399, 2431, 2463, 2495,
> -	2527, 2559, 2591, 2623, 2655, 2687, 2719, 2751, 2783, 2815, 2847, 2879,
> -	2911, 2943, 2975, 3007, 3039, 3071, 3103, 3135, 3167, 3199, 3231, 3263,
> -	3295, 3327, 3359, 3391, 3423, 3455, 3487, 3519, 3551, 3583, 3615, 3647,
> -	3679, 3711, 3743, 3775, 3807, 3839, 3871, 3903, 3935, 3967, 3999, 4031,
> -	4063, 4095, 4127, 4159, 4223, 4287, 4351, 4415, 4479, 4543, 4607, 4671,
> -	4735, 4799, 4863, 4927, 4991, 5055, 5119, 5183, 5247, 5311, 5375, 5439,
> -	5503, 5567, 5631, 5695, 5759, 5823, 5887, 5951, 6015, 6079, 6143, 6207,
> -	6271, 6335, 6399, 6463, 6527, 6591, 6655, 6719, 6783, 6847, 6911, 6975,
> -	7039, 7103, 7167, 7231, 7295, 7359, 7423, 7487, 7551, 7615, 7679, 7743,
> -	7807, 7871, 7935, 7999, 8063, 8127, 8191
> -} };
> -
>  IPU3Awb::IPU3Awb()
>  	: Algorithm()
>  {
> @@ -179,10 +154,6 @@ void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, st
>  	params.use.acc_ccm = 1;
>  	params.acc_param.ccm = imguCssCcmDefault;
>  
> -	params.use.acc_gamma = 1;
> -	params.acc_param.gamma.gc_lut = imguCssGammaLut;
> -	params.acc_param.gamma.gc_ctrl.enable = 1;
> -
>  	zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
>  }
>  
> @@ -332,7 +303,7 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
>  	}
>  }
>  
> -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)
>  {
>  	/*
>  	 * Green gains should not be touched and considered 1.
> @@ -344,18 +315,10 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
>  	params.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;
>  	params.acc_param.bnr.wb_gains.gb = 16;
>  
> -	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK
> -			    << " and gamma calculated: " << agcGamma;
> +	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
>  
>  	/* The CCM matrix may change when color temperature will be used */
>  	params.acc_param.ccm = imguCssCcmDefault;
> -
> -	for (uint32_t i = 0; i < 256; i++) {
> -		double j = i / 255.0;
> -		double gamma = std::pow(j, 1.0 / agcGamma);
> -		/* The maximum value 255 is represented on 13 bits in the IPU3 */
> -		params.acc_param.gamma.gc_lut.lut[i] = gamma * 8191;
> -	}
>  }
>  
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> index ea2d4320..eeb2920b 100644
> --- a/src/ipa/ipu3/ipu3_awb.h
> +++ b/src/ipa/ipu3/ipu3_awb.h
> @@ -31,7 +31,7 @@ public:
>  
>  	void initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
>  	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
> -	void updateWbParameters(ipu3_uapi_params &params, double agcGamma);
> +	void updateWbParameters(ipu3_uapi_params &params);
>  
>  	struct Ipu3AwbCell {
>  		unsigned char greenRedAvg;
>

Hi Jean-Michel,

On Fri, Aug 13, 2021 at 10:31:17AM +0100, Kieran Bingham wrote:
> On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
> > Introduce a new algorithm to manage the contrast handling of the IPU3.
> > 
> > The initial algorithm is chosen to configure the gamma contrast curve
> 
> I don't think this is the 'initial algorithm' any more ...

It could in v3 unless there's a disagreement regarding my comments on
03/10 though.

> > which moves the implementation out of AWB for simplicity. As it is
> > initialised with a default gamma value of 1.1, there is no need to use
> > the default table at init time anymore.
> 
> s/at init time/at initialisation/
>
> > This demonstrates the way to use process() call when the EventStatReady
> > comes in. The function calculates the LUT in the context of a frame, and
> > when prepare() is called, the parameters are filled with the updated
> > values.
> > 
> > AGC needs to be modified to take the new process interface into account.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/algorithm.h  |  2 ++
> >  src/ipa/ipu3/algorithms/contrast.cpp | 51 ++++++++++++++++++++++++++++
> >  src/ipa/ipu3/algorithms/contrast.h   | 33 ++++++++++++++++++
> >  src/ipa/ipu3/algorithms/meson.build  |  3 +-
> >  src/ipa/ipu3/ipa_context.h           |  8 +++++
> >  src/ipa/ipu3/ipu3.cpp                | 12 +++++--
> >  src/ipa/ipu3/ipu3_agc.cpp            |  9 +++--
> >  src/ipa/ipu3/ipu3_agc.h              |  5 +--
> >  src/ipa/ipu3/ipu3_awb.cpp            | 41 ++--------------------
> >  src/ipa/ipu3/ipu3_awb.h              |  2 +-
> >  10 files changed, 114 insertions(+), 52 deletions(-)
> >  create mode 100644 src/ipa/ipu3/algorithms/contrast.cpp
> >  create mode 100644 src/ipa/ipu3/algorithms/contrast.h
> > 
> > diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> > index b571f55a..13e5f2d4 100644
> > --- a/src/ipa/ipu3/algorithms/algorithm.h
> > +++ b/src/ipa/ipu3/algorithms/algorithm.h
> > @@ -28,6 +28,8 @@ public:
> >  	}
> >  
> >  	virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params &params) {}
> > +
> > +	virtual void process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats) {}

Line wrap here too.

> >  };
> >  
> >  } /* namespace ipa::ipu3 */
> > diff --git a/src/ipa/ipu3/algorithms/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp
> > new file mode 100644
> > index 00000000..bc00490f
> > --- /dev/null
> > +++ b/src/ipa/ipu3/algorithms/contrast.cpp
> > @@ -0,0 +1,51 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Ideas On Board
> 
> Keep this consistent with contrast.h please.
> 
> > + *
> > + * constrast.cpp - IPU3 Contrast and Gamma control
> 
> s/constrast/contrast/
> 
> > + */
> > +
> > +#include "contrast.h"
> > +
> > +#include <cmath>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::ipu3::algorithms {
> > +
> > +LOG_DEFINE_CATEGORY(IPU3Contrast)

This isn't used, so you could drop it.

> > +
> > +Contrast::Contrast()
> > +	: gamma_(1.0)
> > +{
> > +}
> > +
> > +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params &params)
> > +{
> > +	params.use.acc_gamma = 1;
> > +	params.acc_param.gamma.gc_ctrl.enable = 1;
> 
> The ordering is almost irrelevant to the code, but for some reason, I
> think you should set the enable and use flags /after/ filling in the data.
> 
> Technically it doesn't matter, but conceptually, they should only be
> enabled, after they are filled in correctly.

I wonder if Kieran has his kernel developer hat on, thinking this will
be written to registers :-)

> > +
> > +	for (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++)
> > +		params.acc_param.gamma.gc_lut.lut[i] = context.frameContext.contrast.lut[i];
> 
> We have two tables of a fixed number of entries.
> I wonder if a memcpy() would be more efficient than a loop, but perhaps
> it doesn't make a difference ?

It shouldn't be less efficient at least.

> Would the compiler do the right thing if you said:
> 
>   params.acc_param.gamma.gc_lut.lut = context.frameContext.contrast.lut;

You can assign structs, but not arrays.

> In fact, I see that
>   params.acc_param.gamma.gc_lut.lut stores unsigned shorts,
> while
>   context.frameContext.contrast.lut stores uint16_t
> 
> Maybe we should store a
> 	struct ipu3_uapi_gamma_corr_lut
> 
> in context.frameContext.contrast instead, Yes they're all the same size
> types - but we have access to the original type of the array, so why not
> use it to keep them identical..
> 
> > +}
> > +
> > +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> > +{
> > +	/* Limit the gamma effect for now */
> 
> Can this be explained any more? Why do we set this here and not
> initialise to 1.1 in the initialisation list?
> 
> Perhaps a
>    \todo expose gamma control setting through the libcamera control API
> 
> should be added here too to make it clear that this is hardcoded, when
> it should somehow be a parameter or control into the IPA or such.
> 
> > +	gamma_ = 1.1;
> > +
> > +	/* Plot the gamma curve into the look up table */
> > +	for (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++) {
> > +		double j = i / 255.0;
> > +		double gamma = std::pow(j, 1.0 / gamma_);
> > +
> > +		/* The maximum value 255 is represented on 13 bits in the IPU3 */
> > +		context.frameContext.contrast.lut[i] = gamma * 8191;
> 
> 8191 still looks like a magic number, even with the comment.

So does 255.0, which I assume is IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1.
Maybe

		double j = static_cast<double>(i)
			 / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);

> I wonder if we should define this somewhere, perhaps as
> 
> /* The maximum value 255 is represented on 13 bits in the IPU3 */

I'm not sure to understand the 255 here. The gamma table contains values
in the [0.0, 1.0] range and maps them to [0, 8191]. 8191 thus
corresponds to 1.0, there's no 255 anywhere there.

(By the way, I only assume that 8191 is 1.0, I wonder if it could be
8192 that is mapped to 1.0, in which case the highest possible value
would be 0.9998779296875)

> #define LUT_INTEGER_SCALE(g) (g * ((1 << 13) - 1))
> 
> 	context.frameContext.contrast.lut[i] = LUT_INTEGER_SCALE(gamma);
> 
> LUT_INTEGER_SCALE may be a completely inappropriate name ... I'm guessing.

Do we need to round to the closest integer instead of the lower one, or
does this make little difference ?

> Also - even having said that - this patch is moving existing code - so
> this shouldn't be changed here - but it might be a distinct patch of
> it's own to come later.

A fixed point helper template class, with template parameters for the
number of integer bits and the number of fractional bits:

template<unsigned int, unsigned int>
class FixedPoint
{
	...
};

with conversion functions, in a fashion similar to
std::chrono::duration. Something for later.

> > +	}
> > +}
> > +
> > +} /* namespace ipa::ipu3::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/ipu3/algorithms/contrast.h b/src/ipa/ipu3/algorithms/contrast.h
> > new file mode 100644
> > index 00000000..50cb626f
> > --- /dev/null
> > +++ b/src/ipa/ipu3/algorithms/contrast.h
> > @@ -0,0 +1,33 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google
> > + *
> > + * constrast.h - IPU3 Contrast and Gamma control
> > + */
> > +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__
> > +#define __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__
> > +
> > +#include "algorithm.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::ipu3::algorithms {
> > +
> > +class Contrast : public Algorithm
> > +{
> > +public:
> > +	Contrast();
> > +	~Contrast() = default;

I don't think this is needed, and if it is, let's not make it an inline
function, you can define it in contrast.cpp as

Contrast::~Contrast() = default;

> > +
> > +	void prepare(IPAContext &context, ipu3_uapi_params &params) override;
> > +	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> > +
> > +private:
> > +	double gamma_;

I wonder if the gamma value should be stored in the context structure. A
bit more design documentation could answer this question :-)

Could you also please explain (just as a reply to this e-mail, not in
comments) why the gamma value was provided by the AGC algorithm, and the
gamma LUT filled by the AWB algorithm ? How does gamma relate, in
genetal, to AGC and/or AWB ?

Also, this doesn't handle contrast at the moment. Do you plan to expand
the algorithm ? I also wonder if it shouldn't be called tonemapping
instead of contrast.

> > +};
> > +
> > +} /* namespace ipa::ipu3::algorithms */
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__ */
> > diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> > index 3fb3ce56..69a59096 100644
> > --- a/src/ipa/ipu3/algorithms/meson.build
> > +++ b/src/ipa/ipu3/algorithms/meson.build
> > @@ -1,5 +1,6 @@
> >  # SPDX-License-Identifier: CC0-1.0
> >  
> >  ipu3_ipa_algorithms = files([
> > -        'grid.cpp',
> 
> Please fix this alignment in the patch that adds it.
> 
> > +    'contrast.cpp',
> > +    'grid.cpp',
> >  ])
> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > index 90b2f2c2..e995c663 100644
> > --- a/src/ipa/ipu3/ipa_context.h
> > +++ b/src/ipa/ipu3/ipa_context.h
> > @@ -33,6 +33,14 @@ struct IPAConfiguration {
> >   * lifetime, though for now a single instance is used.
> >   */
> >  struct IPAFrameContext {
> > +	struct Agc {
> > +		uint32_t exposure;
> > +		double gain;
> > +	} agc;
> > +
> > +	struct Contrast {
> > +		uint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];
> 
> As highilghted earlier, I think this should be:
> 		struct ipu3_uapi_gamma_corr_lut lut
> 
> > +	} contrast;
> >  };
> >  
> >  /* Global context of the IPA */
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 394a7a45..e09adfc3 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -30,6 +30,7 @@
> >  #include "libcamera/internal/mapped_framebuffer.h"
> >  
> >  #include "algorithms/algorithm.h"
> > +#include "algorithms/contrast.h"
> >  #include "algorithms/grid.h"
> >  #include "ipu3_agc.h"
> >  #include "ipu3_awb.h"
> > @@ -167,6 +168,7 @@ int IPAIPU3::init(const IPASettings &settings,
> >  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> >  	/* Construct our Algorithms */
> >  	algorithms_.emplace_back(new algorithms::Grid());
> > +	algorithms_.emplace_back(new algorithms::Contrast());
> >  
> >  	return 0;
> >  }
> > @@ -309,7 +311,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> >  		algo->prepare(context_, params_);
> >  
> >  	if (agcAlgo_->updateControls())
> > -		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
> > +		awbAlgo_->updateWbParameters(params_);
> >  
> >  	*params = params_;
> >  
> > @@ -324,9 +326,15 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> >  			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> >  {
> >  	ControlList ctrls(controls::controls);
> 
> New line here to separate local variable declarations from code segments.
> 
> > +	for (auto const &algo : algorithms_)
> > +		algo->process(context_, stats);
> >  
> >  	double gain = camHelper_->gain(gain_);
> > -	agcAlgo_->process(stats, exposure_, gain);
> > +	context_.frameContext.agc.exposure = exposure_;
> > +	context_.frameContext.agc.gain = gain;
> 
> the IPU3 layer sould not be filling in agc specific structures. That's a
> layering violation to me.
> 
> The algorithms (in this instance, the agc?) own those structures, so
> they should be manipulating them.

I think I agree, but how would you pass the exposure and gain parameters
to the algorithm in that case ?

> It's also showing where the algorithms need to know historical data.
> In this instance, presumably they need to know what gain or expsosure
> was set at the time that this buffer ran.
> 
> This shows where we will need to keep historical versions of the
> FrameContext, and perhaps track them along with the frame counter.
> 
> For now, while we get things moving to be more modular, I think we could
> keep this as it is - but it should be commented with a todo
> 
> > +	agcAlgo_->process(context_, stats);
> > +	exposure_ = context_.frameContext.agc.exposure;
> > +	gain = context_.frameContext.agc.gain;
> >  	gain_ = camHelper_->gainCode(gain);
> >  
> >  	awbAlgo_->calculateWBGains(stats);
> > diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> > index 408eb849..c6782ff4 100644
> > --- a/src/ipa/ipu3/ipu3_agc.cpp
> > +++ b/src/ipa/ipu3/ipu3_agc.cpp
> > @@ -52,7 +52,7 @@ static constexpr uint8_t kCellSize = 8;
> >  
> >  IPU3Agc::IPU3Agc()
> >  	: frameCount_(0), lastFrame_(0), converged_(false),
> > -	  updateControls_(false), iqMean_(0.0), gamma_(1.0),
> > +	  updateControls_(false), iqMean_(0.0),
> >  	  lineDuration_(0s), maxExposureTime_(0s),
> >  	  prevExposure_(0s), prevExposureNoDg_(0s),
> >  	  currentExposure_(0s), currentExposureNoDg_(0s)
> > @@ -104,9 +104,6 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> >  		}
> >  	}
> >  
> > -	/* Limit the gamma effect for now */
> > -	gamma_ = 1.1;
> > -
> >  	/* Estimate the quantile mean of the top 2% of the histogram */
> >  	iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
> >  }
> > @@ -193,8 +190,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
> >  	lastFrame_ = frameCount_;
> >  }
> >  
> > -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain)
> > +void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> >  {
> > +	uint32_t &exposure = context.frameContext.agc.exposure;
> > +	double &gain = context.frameContext.agc.gain;
> >  	processBrightness(stats);
> >  	lockExposureGain(exposure, gain);
> >  	frameCount_++;
> > diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> > index f00b98d6..8b32e52f 100644
> > --- a/src/ipa/ipu3/ipu3_agc.h
> > +++ b/src/ipa/ipu3/ipu3_agc.h
> > @@ -30,11 +30,9 @@ public:
> >  	~IPU3Agc() = default;
> >  
> >  	void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);
> > -	void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);
> > +	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> >  	bool converged() { return converged_; }
> >  	bool updateControls() { return updateControls_; }
> > -	/* \todo Use a metadata exchange between IPAs */
> > -	double gamma() { return gamma_; }
> >  
> >  private:
> >  	void processBrightness(const ipu3_uapi_stats_3a *stats);
> > @@ -50,7 +48,6 @@ private:
> >  	bool updateControls_;
> >  
> >  	double iqMean_;
> > -	double gamma_;
> >  
> >  	Duration lineDuration_;
> >  	Duration maxExposureTime_;
> > diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> > index 911760b3..0f3bcdc9 100644
> > --- a/src/ipa/ipu3/ipu3_awb.cpp
> > +++ b/src/ipa/ipu3/ipu3_awb.cpp
> > @@ -114,31 +114,6 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
> >  	0, 0, 8191, 0
> >  };
> >  
> > -/* Default settings for Gamma correction */
> > -const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {
> > -	63, 79, 95, 111, 127, 143, 159, 175, 191, 207, 223, 239, 255, 271, 287,
> > -	303, 319, 335, 351, 367, 383, 399, 415, 431, 447, 463, 479, 495, 511,
> > -	527, 543, 559, 575, 591, 607, 623, 639, 655, 671, 687, 703, 719, 735,
> > -	751, 767, 783, 799, 815, 831, 847, 863, 879, 895, 911, 927, 943, 959,
> > -	975, 991, 1007, 1023, 1039, 1055, 1071, 1087, 1103, 1119, 1135, 1151,
> > -	1167, 1183, 1199, 1215, 1231, 1247, 1263, 1279, 1295, 1311, 1327, 1343,
> > -	1359, 1375, 1391, 1407, 1423, 1439, 1455, 1471, 1487, 1503, 1519, 1535,
> > -	1551, 1567, 1583, 1599, 1615, 1631, 1647, 1663, 1679, 1695, 1711, 1727,
> > -	1743, 1759, 1775, 1791, 1807, 1823, 1839, 1855, 1871, 1887, 1903, 1919,
> > -	1935, 1951, 1967, 1983, 1999, 2015, 2031, 2047, 2063, 2079, 2095, 2111,
> > -	2143, 2175, 2207, 2239, 2271, 2303, 2335, 2367, 2399, 2431, 2463, 2495,
> > -	2527, 2559, 2591, 2623, 2655, 2687, 2719, 2751, 2783, 2815, 2847, 2879,
> > -	2911, 2943, 2975, 3007, 3039, 3071, 3103, 3135, 3167, 3199, 3231, 3263,
> > -	3295, 3327, 3359, 3391, 3423, 3455, 3487, 3519, 3551, 3583, 3615, 3647,
> > -	3679, 3711, 3743, 3775, 3807, 3839, 3871, 3903, 3935, 3967, 3999, 4031,
> > -	4063, 4095, 4127, 4159, 4223, 4287, 4351, 4415, 4479, 4543, 4607, 4671,
> > -	4735, 4799, 4863, 4927, 4991, 5055, 5119, 5183, 5247, 5311, 5375, 5439,
> > -	5503, 5567, 5631, 5695, 5759, 5823, 5887, 5951, 6015, 6079, 6143, 6207,
> > -	6271, 6335, 6399, 6463, 6527, 6591, 6655, 6719, 6783, 6847, 6911, 6975,
> > -	7039, 7103, 7167, 7231, 7295, 7359, 7423, 7487, 7551, 7615, 7679, 7743,
> > -	7807, 7871, 7935, 7999, 8063, 8127, 8191
> > -} };
> > -
> >  IPU3Awb::IPU3Awb()
> >  	: Algorithm()
> >  {
> > @@ -179,10 +154,6 @@ void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, st
> >  	params.use.acc_ccm = 1;
> >  	params.acc_param.ccm = imguCssCcmDefault;
> >  
> > -	params.use.acc_gamma = 1;
> > -	params.acc_param.gamma.gc_lut = imguCssGammaLut;
> > -	params.acc_param.gamma.gc_ctrl.enable = 1;
> > -
> >  	zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
> >  }
> >  
> > @@ -332,7 +303,7 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> >  	}
> >  }
> >  
> > -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
> > +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)
> >  {
> >  	/*
> >  	 * Green gains should not be touched and considered 1.
> > @@ -344,18 +315,10 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
> >  	params.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;
> >  	params.acc_param.bnr.wb_gains.gb = 16;
> >  
> > -	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK
> > -			    << " and gamma calculated: " << agcGamma;
> > +	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
> >  
> >  	/* The CCM matrix may change when color temperature will be used */
> >  	params.acc_param.ccm = imguCssCcmDefault;
> > -
> > -	for (uint32_t i = 0; i < 256; i++) {
> > -		double j = i / 255.0;
> > -		double gamma = std::pow(j, 1.0 / agcGamma);
> > -		/* The maximum value 255 is represented on 13 bits in the IPU3 */
> > -		params.acc_param.gamma.gc_lut.lut[i] = gamma * 8191;
> > -	}
> >  }
> >  
> >  } /* namespace ipa::ipu3 */
> > diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> > index ea2d4320..eeb2920b 100644
> > --- a/src/ipa/ipu3/ipu3_awb.h
> > +++ b/src/ipa/ipu3/ipu3_awb.h
> > @@ -31,7 +31,7 @@ public:
> >  
> >  	void initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
> >  	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
> > -	void updateWbParameters(ipu3_uapi_params &params, double agcGamma);
> > +	void updateWbParameters(ipu3_uapi_params &params);
> >  
> >  	struct Ipu3AwbCell {
> >  		unsigned char greenRedAvg;

Hi Laurent,

On 16/08/2021 00:53, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> On Fri, Aug 13, 2021 at 10:31:17AM +0100, Kieran Bingham wrote:
>> On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
>>> Introduce a new algorithm to manage the contrast handling of the IPU3.
>>>
>>> The initial algorithm is chosen to configure the gamma contrast curve
>>
>> I don't think this is the 'initial algorithm' any more ...
> 
> It could in v3 unless there's a disagreement regarding my comments on
> 03/10 though.
> 
>>> which moves the implementation out of AWB for simplicity. As it is
>>> initialised with a default gamma value of 1.1, there is no need to use
>>> the default table at init time anymore.
>>
>> s/at init time/at initialisation/
>>
>>> This demonstrates the way to use process() call when the EventStatReady
>>> comes in. The function calculates the LUT in the context of a frame, and
>>> when prepare() is called, the parameters are filled with the updated
>>> values.
>>>
>>> AGC needs to be modified to take the new process interface into account.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>>> ---
>>>  src/ipa/ipu3/algorithms/algorithm.h  |  2 ++
>>>  src/ipa/ipu3/algorithms/contrast.cpp | 51 ++++++++++++++++++++++++++++
>>>  src/ipa/ipu3/algorithms/contrast.h   | 33 ++++++++++++++++++
>>>  src/ipa/ipu3/algorithms/meson.build  |  3 +-
>>>  src/ipa/ipu3/ipa_context.h           |  8 +++++
>>>  src/ipa/ipu3/ipu3.cpp                | 12 +++++--
>>>  src/ipa/ipu3/ipu3_agc.cpp            |  9 +++--
>>>  src/ipa/ipu3/ipu3_agc.h              |  5 +--
>>>  src/ipa/ipu3/ipu3_awb.cpp            | 41 ++--------------------
>>>  src/ipa/ipu3/ipu3_awb.h              |  2 +-
>>>  10 files changed, 114 insertions(+), 52 deletions(-)
>>>  create mode 100644 src/ipa/ipu3/algorithms/contrast.cpp
>>>  create mode 100644 src/ipa/ipu3/algorithms/contrast.h
>>>
>>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
>>> index b571f55a..13e5f2d4 100644
>>> --- a/src/ipa/ipu3/algorithms/algorithm.h
>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h
>>> @@ -28,6 +28,8 @@ public:
>>>  	}
>>>  
>>>  	virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params &params) {}
>>> +
>>> +	virtual void process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats) {}
> 
> Line wrap here too.
> 
>>>  };
>>>  
>>>  } /* namespace ipa::ipu3 */
>>> diff --git a/src/ipa/ipu3/algorithms/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp
>>> new file mode 100644
>>> index 00000000..bc00490f
>>> --- /dev/null
>>> +++ b/src/ipa/ipu3/algorithms/contrast.cpp
>>> @@ -0,0 +1,51 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2021, Ideas On Board
>>
>> Keep this consistent with contrast.h please.
>>
>>> + *
>>> + * constrast.cpp - IPU3 Contrast and Gamma control
>>
>> s/constrast/contrast/
>>
>>> + */
>>> +
>>> +#include "contrast.h"
>>> +
>>> +#include <cmath>
>>> +
>>> +#include <libcamera/base/log.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +namespace ipa::ipu3::algorithms {
>>> +
>>> +LOG_DEFINE_CATEGORY(IPU3Contrast)
> 
> This isn't used, so you could drop it.
> 
>>> +
>>> +Contrast::Contrast()
>>> +	: gamma_(1.0)
>>> +{
>>> +}
>>> +
>>> +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params &params)
>>> +{
>>> +	params.use.acc_gamma = 1;
>>> +	params.acc_param.gamma.gc_ctrl.enable = 1;
>>
>> The ordering is almost irrelevant to the code, but for some reason, I
>> think you should set the enable and use flags /after/ filling in the data.
>>
>> Technically it doesn't matter, but conceptually, they should only be
>> enabled, after they are filled in correctly.
> 
> I wonder if Kieran has his kernel developer hat on, thinking this will
> be written to registers :-)
> 
>>> +
>>> +	for (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++)
>>> +		params.acc_param.gamma.gc_lut.lut[i] = context.frameContext.contrast.lut[i];
>>
>> We have two tables of a fixed number of entries.
>> I wonder if a memcpy() would be more efficient than a loop, but perhaps
>> it doesn't make a difference ?
> 
> It shouldn't be less efficient at least.
> 
>> Would the compiler do the right thing if you said:
>>
>>   params.acc_param.gamma.gc_lut.lut = context.frameContext.contrast.lut;
> 
> You can assign structs, but not arrays.

Yes, memcpy is probably a better option ;-).

>> In fact, I see that
>>   params.acc_param.gamma.gc_lut.lut stores unsigned shorts,
>> while
>>   context.frameContext.contrast.lut stores uint16_t
>>
>> Maybe we should store a
>> 	struct ipu3_uapi_gamma_corr_lut
>>
>> in context.frameContext.contrast instead, Yes they're all the same size
>> types - but we have access to the original type of the array, so why not
>> use it to keep them identical..
>>
>>> +}
>>> +
>>> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>>> +{
>>> +	/* Limit the gamma effect for now */
>>
>> Can this be explained any more? Why do we set this here and not
>> initialise to 1.1 in the initialisation list?
>>
>> Perhaps a
>>    \todo expose gamma control setting through the libcamera control API
>>
>> should be added here too to make it clear that this is hardcoded, when
>> it should somehow be a parameter or control into the IPA or such.
>>
>>> +	gamma_ = 1.1;
>>> +
>>> +	/* Plot the gamma curve into the look up table */
>>> +	for (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++) {
>>> +		double j = i / 255.0;
>>> +		double gamma = std::pow(j, 1.0 / gamma_);
>>> +
>>> +		/* The maximum value 255 is represented on 13 bits in the IPU3 */
>>> +		context.frameContext.contrast.lut[i] = gamma * 8191;
>>
>> 8191 still looks like a magic number, even with the comment.
> 
> So does 255.0, which I assume is IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1.
> Maybe
> 
> 		double j = static_cast<double>(i)
> 			 / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);
> 
>> I wonder if we should define this somewhere, perhaps as
>>
>> /* The maximum value 255 is represented on 13 bits in the IPU3 */
> 
> I'm not sure to understand the 255 here. The gamma table contains values
> in the [0.0, 1.0] range and maps them to [0, 8191]. 8191 thus
> corresponds to 1.0, there's no 255 anywhere there.
> 
> (By the way, I only assume that 8191 is 1.0, I wonder if it could be
> 8192 that is mapped to 1.0, in which case the highest possible value
> would be 0.9998779296875)
> 

Well, the maximum is 8191 (we have 8192 values). Each input pixel is
between 0 and 255 and this is mapped to a new value between 0 and 8191.


>> #define LUT_INTEGER_SCALE(g) (g * ((1 << 13) - 1))
>>
>> 	context.frameContext.contrast.lut[i] = LUT_INTEGER_SCALE(gamma);
>>
>> LUT_INTEGER_SCALE may be a completely inappropriate name ... I'm guessing.
> 
> Do we need to round to the closest integer instead of the lower one, or
> does this make little difference ?
> 
>> Also - even having said that - this patch is moving existing code - so
>> this shouldn't be changed here - but it might be a distinct patch of
>> it's own to come later.
> 
> A fixed point helper template class, with template parameters for the
> number of integer bits and the number of fractional bits:
> 
> template<unsigned int, unsigned int>
> class FixedPoint
> {
> 	...
> };
> 
> with conversion functions, in a fashion similar to
> std::chrono::duration. Something for later.
> 
>>> +	}
>>> +}
>>> +
>>> +} /* namespace ipa::ipu3::algorithms */
>>> +
>>> +} /* namespace libcamera */
>>> diff --git a/src/ipa/ipu3/algorithms/contrast.h b/src/ipa/ipu3/algorithms/contrast.h
>>> new file mode 100644
>>> index 00000000..50cb626f
>>> --- /dev/null
>>> +++ b/src/ipa/ipu3/algorithms/contrast.h
>>> @@ -0,0 +1,33 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2021, Google
>>> + *
>>> + * constrast.h - IPU3 Contrast and Gamma control
>>> + */
>>> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__
>>> +#define __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__
>>> +
>>> +#include "algorithm.h"
>>> +
>>> +namespace libcamera {
>>> +
>>> +namespace ipa::ipu3::algorithms {
>>> +
>>> +class Contrast : public Algorithm
>>> +{
>>> +public:
>>> +	Contrast();
>>> +	~Contrast() = default;
> 
> I don't think this is needed, and if it is, let's not make it an inline
> function, you can define it in contrast.cpp as
> 
> Contrast::~Contrast() = default;
> 
>>> +
>>> +	void prepare(IPAContext &context, ipu3_uapi_params &params) override;
>>> +	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>>> +
>>> +private:
>>> +	double gamma_;
> 
> I wonder if the gamma value should be stored in the context structure. A
> bit more design documentation could answer this question :-)
> 
> Could you also please explain (just as a reply to this e-mail, not in
> comments) why the gamma value was provided by the AGC algorithm, and the
> gamma LUT filled by the AWB algorithm ? How does gamma relate, in
> genetal, to AGC and/or AWB ?

The original AGC algorithm comes partially from a paper "A New Auto
Exposure and Auto White-Balance Algorithm to Detect High Dynamic Range
Conditions Using CMOS Technology", Quoc Kien Vuong, Se-Hwan Yun, and
Suki Kim

The idea was to be able to distinguish when we are in a back lighting
condition, in particular. On this original algorithm, adding a
calculated gamma value to compensate the over/under exposure was used.

This has been abandonned but beeing able to change the default gamma
correction table is interesting.
Now, the gamma correction should not be controlled by AGC or AWB, but on
its own, as well as the ipu3_uapi_isp_lin_vmem_params structure, which
really controls the contrast (as it is here to linearize the non linear
sensor).

> Also, this doesn't handle contrast at the moment. Do you plan to expand
> the algorithm ? I also wonder if it shouldn't be called tonemapping
> instead of contrast.
> 
>>> +};
>>> +
>>> +} /* namespace ipa::ipu3::algorithms */
>>> +
>>> +} /* namespace libcamera */
>>> +
>>> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__ */
>>> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
>>> index 3fb3ce56..69a59096 100644
>>> --- a/src/ipa/ipu3/algorithms/meson.build
>>> +++ b/src/ipa/ipu3/algorithms/meson.build
>>> @@ -1,5 +1,6 @@
>>>  # SPDX-License-Identifier: CC0-1.0
>>>  
>>>  ipu3_ipa_algorithms = files([
>>> -        'grid.cpp',
>>
>> Please fix this alignment in the patch that adds it.
>>
>>> +    'contrast.cpp',
>>> +    'grid.cpp',
>>>  ])
>>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>>> index 90b2f2c2..e995c663 100644
>>> --- a/src/ipa/ipu3/ipa_context.h
>>> +++ b/src/ipa/ipu3/ipa_context.h
>>> @@ -33,6 +33,14 @@ struct IPAConfiguration {
>>>   * lifetime, though for now a single instance is used.
>>>   */
>>>  struct IPAFrameContext {
>>> +	struct Agc {
>>> +		uint32_t exposure;
>>> +		double gain;
>>> +	} agc;
>>> +
>>> +	struct Contrast {
>>> +		uint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];
>>
>> As highilghted earlier, I think this should be:
>> 		struct ipu3_uapi_gamma_corr_lut lut
>>
>>> +	} contrast;
>>>  };
>>>  
>>>  /* Global context of the IPA */
>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>>> index 394a7a45..e09adfc3 100644
>>> --- a/src/ipa/ipu3/ipu3.cpp
>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>> @@ -30,6 +30,7 @@
>>>  #include "libcamera/internal/mapped_framebuffer.h"
>>>  
>>>  #include "algorithms/algorithm.h"
>>> +#include "algorithms/contrast.h"
>>>  #include "algorithms/grid.h"
>>>  #include "ipu3_agc.h"
>>>  #include "ipu3_awb.h"
>>> @@ -167,6 +168,7 @@ int IPAIPU3::init(const IPASettings &settings,
>>>  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
>>>  	/* Construct our Algorithms */
>>>  	algorithms_.emplace_back(new algorithms::Grid());
>>> +	algorithms_.emplace_back(new algorithms::Contrast());
>>>  
>>>  	return 0;
>>>  }
>>> @@ -309,7 +311,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>>>  		algo->prepare(context_, params_);
>>>  
>>>  	if (agcAlgo_->updateControls())
>>> -		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
>>> +		awbAlgo_->updateWbParameters(params_);
>>>  
>>>  	*params = params_;
>>>  
>>> @@ -324,9 +326,15 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>>  			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>>>  {
>>>  	ControlList ctrls(controls::controls);
>>
>> New line here to separate local variable declarations from code segments.
>>
>>> +	for (auto const &algo : algorithms_)
>>> +		algo->process(context_, stats);
>>>  
>>>  	double gain = camHelper_->gain(gain_);
>>> -	agcAlgo_->process(stats, exposure_, gain);
>>> +	context_.frameContext.agc.exposure = exposure_;
>>> +	context_.frameContext.agc.gain = gain;
>>
>> the IPU3 layer sould not be filling in agc specific structures. That's a
>> layering violation to me.
>>
>> The algorithms (in this instance, the agc?) own those structures, so
>> they should be manipulating them.
> 
> I think I agree, but how would you pass the exposure and gain parameters
> to the algorithm in that case ?

Kieran, I agree with Laurent on this :-)

>> It's also showing where the algorithms need to know historical data.
>> In this instance, presumably they need to know what gain or expsosure
>> was set at the time that this buffer ran.
>>
>> This shows where we will need to keep historical versions of the
>> FrameContext, and perhaps track them along with the frame counter.
>>
>> For now, while we get things moving to be more modular, I think we could
>> keep this as it is - but it should be commented with a todo
>>
>>> +	agcAlgo_->process(context_, stats);
>>> +	exposure_ = context_.frameContext.agc.exposure;
>>> +	gain = context_.frameContext.agc.gain;
>>>  	gain_ = camHelper_->gainCode(gain);
>>>  
>>>  	awbAlgo_->calculateWBGains(stats);
>>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
>>> index 408eb849..c6782ff4 100644
>>> --- a/src/ipa/ipu3/ipu3_agc.cpp
>>> +++ b/src/ipa/ipu3/ipu3_agc.cpp
>>> @@ -52,7 +52,7 @@ static constexpr uint8_t kCellSize = 8;
>>>  
>>>  IPU3Agc::IPU3Agc()
>>>  	: frameCount_(0), lastFrame_(0), converged_(false),
>>> -	  updateControls_(false), iqMean_(0.0), gamma_(1.0),
>>> +	  updateControls_(false), iqMean_(0.0),
>>>  	  lineDuration_(0s), maxExposureTime_(0s),
>>>  	  prevExposure_(0s), prevExposureNoDg_(0s),
>>>  	  currentExposure_(0s), currentExposureNoDg_(0s)
>>> @@ -104,9 +104,6 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>>>  		}
>>>  	}
>>>  
>>> -	/* Limit the gamma effect for now */
>>> -	gamma_ = 1.1;
>>> -
>>>  	/* Estimate the quantile mean of the top 2% of the histogram */
>>>  	iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>>>  }
>>> @@ -193,8 +190,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>>>  	lastFrame_ = frameCount_;
>>>  }
>>>  
>>> -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain)
>>> +void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>>>  {
>>> +	uint32_t &exposure = context.frameContext.agc.exposure;
>>> +	double &gain = context.frameContext.agc.gain;
>>>  	processBrightness(stats);
>>>  	lockExposureGain(exposure, gain);
>>>  	frameCount_++;
>>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
>>> index f00b98d6..8b32e52f 100644
>>> --- a/src/ipa/ipu3/ipu3_agc.h
>>> +++ b/src/ipa/ipu3/ipu3_agc.h
>>> @@ -30,11 +30,9 @@ public:
>>>  	~IPU3Agc() = default;
>>>  
>>>  	void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);
>>> -	void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);
>>> +	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>>>  	bool converged() { return converged_; }
>>>  	bool updateControls() { return updateControls_; }
>>> -	/* \todo Use a metadata exchange between IPAs */
>>> -	double gamma() { return gamma_; }
>>>  
>>>  private:
>>>  	void processBrightness(const ipu3_uapi_stats_3a *stats);
>>> @@ -50,7 +48,6 @@ private:
>>>  	bool updateControls_;
>>>  
>>>  	double iqMean_;
>>> -	double gamma_;
>>>  
>>>  	Duration lineDuration_;
>>>  	Duration maxExposureTime_;
>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
>>> index 911760b3..0f3bcdc9 100644
>>> --- a/src/ipa/ipu3/ipu3_awb.cpp
>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
>>> @@ -114,31 +114,6 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
>>>  	0, 0, 8191, 0
>>>  };
>>>  
>>> -/* Default settings for Gamma correction */
>>> -const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {
>>> -	63, 79, 95, 111, 127, 143, 159, 175, 191, 207, 223, 239, 255, 271, 287,
>>> -	303, 319, 335, 351, 367, 383, 399, 415, 431, 447, 463, 479, 495, 511,
>>> -	527, 543, 559, 575, 591, 607, 623, 639, 655, 671, 687, 703, 719, 735,
>>> -	751, 767, 783, 799, 815, 831, 847, 863, 879, 895, 911, 927, 943, 959,
>>> -	975, 991, 1007, 1023, 1039, 1055, 1071, 1087, 1103, 1119, 1135, 1151,
>>> -	1167, 1183, 1199, 1215, 1231, 1247, 1263, 1279, 1295, 1311, 1327, 1343,
>>> -	1359, 1375, 1391, 1407, 1423, 1439, 1455, 1471, 1487, 1503, 1519, 1535,
>>> -	1551, 1567, 1583, 1599, 1615, 1631, 1647, 1663, 1679, 1695, 1711, 1727,
>>> -	1743, 1759, 1775, 1791, 1807, 1823, 1839, 1855, 1871, 1887, 1903, 1919,
>>> -	1935, 1951, 1967, 1983, 1999, 2015, 2031, 2047, 2063, 2079, 2095, 2111,
>>> -	2143, 2175, 2207, 2239, 2271, 2303, 2335, 2367, 2399, 2431, 2463, 2495,
>>> -	2527, 2559, 2591, 2623, 2655, 2687, 2719, 2751, 2783, 2815, 2847, 2879,
>>> -	2911, 2943, 2975, 3007, 3039, 3071, 3103, 3135, 3167, 3199, 3231, 3263,
>>> -	3295, 3327, 3359, 3391, 3423, 3455, 3487, 3519, 3551, 3583, 3615, 3647,
>>> -	3679, 3711, 3743, 3775, 3807, 3839, 3871, 3903, 3935, 3967, 3999, 4031,
>>> -	4063, 4095, 4127, 4159, 4223, 4287, 4351, 4415, 4479, 4543, 4607, 4671,
>>> -	4735, 4799, 4863, 4927, 4991, 5055, 5119, 5183, 5247, 5311, 5375, 5439,
>>> -	5503, 5567, 5631, 5695, 5759, 5823, 5887, 5951, 6015, 6079, 6143, 6207,
>>> -	6271, 6335, 6399, 6463, 6527, 6591, 6655, 6719, 6783, 6847, 6911, 6975,
>>> -	7039, 7103, 7167, 7231, 7295, 7359, 7423, 7487, 7551, 7615, 7679, 7743,
>>> -	7807, 7871, 7935, 7999, 8063, 8127, 8191
>>> -} };
>>> -
>>>  IPU3Awb::IPU3Awb()
>>>  	: Algorithm()
>>>  {
>>> @@ -179,10 +154,6 @@ void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, st
>>>  	params.use.acc_ccm = 1;
>>>  	params.acc_param.ccm = imguCssCcmDefault;
>>>  
>>> -	params.use.acc_gamma = 1;
>>> -	params.acc_param.gamma.gc_lut = imguCssGammaLut;
>>> -	params.acc_param.gamma.gc_ctrl.enable = 1;
>>> -
>>>  	zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
>>>  }
>>>  
>>> @@ -332,7 +303,7 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
>>>  	}
>>>  }
>>>  
>>> -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
>>> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)
>>>  {
>>>  	/*
>>>  	 * Green gains should not be touched and considered 1.
>>> @@ -344,18 +315,10 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
>>>  	params.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;
>>>  	params.acc_param.bnr.wb_gains.gb = 16;
>>>  
>>> -	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK
>>> -			    << " and gamma calculated: " << agcGamma;
>>> +	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
>>>  
>>>  	/* The CCM matrix may change when color temperature will be used */
>>>  	params.acc_param.ccm = imguCssCcmDefault;
>>> -
>>> -	for (uint32_t i = 0; i < 256; i++) {
>>> -		double j = i / 255.0;
>>> -		double gamma = std::pow(j, 1.0 / agcGamma);
>>> -		/* The maximum value 255 is represented on 13 bits in the IPU3 */
>>> -		params.acc_param.gamma.gc_lut.lut[i] = gamma * 8191;
>>> -	}
>>>  }
>>>  
>>>  } /* namespace ipa::ipu3 */
>>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
>>> index ea2d4320..eeb2920b 100644
>>> --- a/src/ipa/ipu3/ipu3_awb.h
>>> +++ b/src/ipa/ipu3/ipu3_awb.h
>>> @@ -31,7 +31,7 @@ public:
>>>  
>>>  	void initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
>>>  	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
>>> -	void updateWbParameters(ipu3_uapi_params &params, double agcGamma);
>>> +	void updateWbParameters(ipu3_uapi_params &params);
>>>  
>>>  	struct Ipu3AwbCell {
>>>  		unsigned char greenRedAvg;
>

On 16/08/2021 08:47, Jean-Michel Hautbois wrote:
> Hi Laurent,
> 
> On 16/08/2021 00:53, Laurent Pinchart wrote:
>> Hi Jean-Michel,
>>
>> On Fri, Aug 13, 2021 at 10:31:17AM +0100, Kieran Bingham wrote:
>>> On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
>>>> Introduce a new algorithm to manage the contrast handling of the IPU3.
>>>>
>>>> The initial algorithm is chosen to configure the gamma contrast curve
>>>
>>> I don't think this is the 'initial algorithm' any more ...
>>
>> It could in v3 unless there's a disagreement regarding my comments on
>> 03/10 though.
>>
>>>> which moves the implementation out of AWB for simplicity. As it is
>>>> initialised with a default gamma value of 1.1, there is no need to use
>>>> the default table at init time anymore.
>>>
>>> s/at init time/at initialisation/
>>>
>>>> This demonstrates the way to use process() call when the EventStatReady
>>>> comes in. The function calculates the LUT in the context of a frame, and
>>>> when prepare() is called, the parameters are filled with the updated
>>>> values.
>>>>
>>>> AGC needs to be modified to take the new process interface into account.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>>>> ---
>>>>  src/ipa/ipu3/algorithms/algorithm.h  |  2 ++
>>>>  src/ipa/ipu3/algorithms/contrast.cpp | 51 ++++++++++++++++++++++++++++
>>>>  src/ipa/ipu3/algorithms/contrast.h   | 33 ++++++++++++++++++
>>>>  src/ipa/ipu3/algorithms/meson.build  |  3 +-
>>>>  src/ipa/ipu3/ipa_context.h           |  8 +++++
>>>>  src/ipa/ipu3/ipu3.cpp                | 12 +++++--
>>>>  src/ipa/ipu3/ipu3_agc.cpp            |  9 +++--
>>>>  src/ipa/ipu3/ipu3_agc.h              |  5 +--
>>>>  src/ipa/ipu3/ipu3_awb.cpp            | 41 ++--------------------
>>>>  src/ipa/ipu3/ipu3_awb.h              |  2 +-
>>>>  10 files changed, 114 insertions(+), 52 deletions(-)
>>>>  create mode 100644 src/ipa/ipu3/algorithms/contrast.cpp
>>>>  create mode 100644 src/ipa/ipu3/algorithms/contrast.h
>>>>
>>>> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
>>>> index b571f55a..13e5f2d4 100644
>>>> --- a/src/ipa/ipu3/algorithms/algorithm.h
>>>> +++ b/src/ipa/ipu3/algorithms/algorithm.h
>>>> @@ -28,6 +28,8 @@ public:
>>>>  	}
>>>>  
>>>>  	virtual void prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] ipu3_uapi_params &params) {}
>>>> +
>>>> +	virtual void process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats) {}
>>
>> Line wrap here too.
>>
>>>>  };
>>>>  
>>>>  } /* namespace ipa::ipu3 */
>>>> diff --git a/src/ipa/ipu3/algorithms/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp
>>>> new file mode 100644
>>>> index 00000000..bc00490f
>>>> --- /dev/null
>>>> +++ b/src/ipa/ipu3/algorithms/contrast.cpp
>>>> @@ -0,0 +1,51 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2021, Ideas On Board
>>>
>>> Keep this consistent with contrast.h please.
>>>
>>>> + *
>>>> + * constrast.cpp - IPU3 Contrast and Gamma control
>>>
>>> s/constrast/contrast/
>>>
>>>> + */
>>>> +
>>>> +#include "contrast.h"
>>>> +
>>>> +#include <cmath>
>>>> +
>>>> +#include <libcamera/base/log.h>
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +namespace ipa::ipu3::algorithms {
>>>> +
>>>> +LOG_DEFINE_CATEGORY(IPU3Contrast)
>>
>> This isn't used, so you could drop it.
>>
>>>> +
>>>> +Contrast::Contrast()
>>>> +	: gamma_(1.0)
>>>> +{
>>>> +}
>>>> +
>>>> +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params &params)
>>>> +{
>>>> +	params.use.acc_gamma = 1;
>>>> +	params.acc_param.gamma.gc_ctrl.enable = 1;
>>>
>>> The ordering is almost irrelevant to the code, but for some reason, I
>>> think you should set the enable and use flags /after/ filling in the data.
>>>
>>> Technically it doesn't matter, but conceptually, they should only be
>>> enabled, after they are filled in correctly.
>>
>> I wonder if Kieran has his kernel developer hat on, thinking this will
>> be written to registers :-)

Of course ;-)

But perhaps it makes more sense to me to say something is 'valid' after
it is configured. Like I said - it doesn't matter to the code or the
compiler here, so there's no requirement from me, it was just a comment.


>>
>>>> +
>>>> +	for (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++)
>>>> +		params.acc_param.gamma.gc_lut.lut[i] = context.frameContext.contrast.lut[i];
>>>
>>> We have two tables of a fixed number of entries.
>>> I wonder if a memcpy() would be more efficient than a loop, but perhaps
>>> it doesn't make a difference ?
>>
>> It shouldn't be less efficient at least.
>>
>>> Would the compiler do the right thing if you said:
>>>
>>>   params.acc_param.gamma.gc_lut.lut = context.frameContext.contrast.lut;
>>
>> You can assign structs, but not arrays.
> 
> Yes, memcpy is probably a better option ;-).
> 
>>> In fact, I see that
>>>   params.acc_param.gamma.gc_lut.lut stores unsigned shorts,
>>> while
>>>   context.frameContext.contrast.lut stores uint16_t
>>>
>>> Maybe we should store a
>>> 	struct ipu3_uapi_gamma_corr_lut
>>>
>>> in context.frameContext.contrast instead, Yes they're all the same size
>>> types - but we have access to the original type of the array, so why not
>>> use it to keep them identical..
>>>
>>>> +}
>>>> +
>>>> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>>>> +{
>>>> +	/* Limit the gamma effect for now */
>>>
>>> Can this be explained any more? Why do we set this here and not
>>> initialise to 1.1 in the initialisation list?
>>>
>>> Perhaps a
>>>    \todo expose gamma control setting through the libcamera control API
>>>
>>> should be added here too to make it clear that this is hardcoded, when
>>> it should somehow be a parameter or control into the IPA or such.
>>>
>>>> +	gamma_ = 1.1;
>>>> +
>>>> +	/* Plot the gamma curve into the look up table */
>>>> +	for (uint32_t i = 0; i < IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES; i++) {
>>>> +		double j = i / 255.0;
>>>> +		double gamma = std::pow(j, 1.0 / gamma_);
>>>> +
>>>> +		/* The maximum value 255 is represented on 13 bits in the IPU3 */
>>>> +		context.frameContext.contrast.lut[i] = gamma * 8191;
>>>
>>> 8191 still looks like a magic number, even with the comment.
>>
>> So does 255.0, which I assume is IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1.
>> Maybe
>>
>> 		double j = static_cast<double>(i)
>> 			 / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);
>>
>>> I wonder if we should define this somewhere, perhaps as
>>>
>>> /* The maximum value 255 is represented on 13 bits in the IPU3 */
>>
>> I'm not sure to understand the 255 here. The gamma table contains values
>> in the [0.0, 1.0] range and maps them to [0, 8191]. 8191 thus
>> corresponds to 1.0, there's no 255 anywhere there.
>>
>> (By the way, I only assume that 8191 is 1.0, I wonder if it could be
>> 8192 that is mapped to 1.0, in which case the highest possible value
>> would be 0.9998779296875)
>>
> 
> Well, the maximum is 8191 (we have 8192 values). Each input pixel is
> between 0 and 255 and this is mapped to a new value between 0 and 8191.
> 
> 
>>> #define LUT_INTEGER_SCALE(g) (g * ((1 << 13) - 1))
>>>
>>> 	context.frameContext.contrast.lut[i] = LUT_INTEGER_SCALE(gamma);
>>>
>>> LUT_INTEGER_SCALE may be a completely inappropriate name ... I'm guessing.
>>
>> Do we need to round to the closest integer instead of the lower one, or
>> does this make little difference ?
>>
>>> Also - even having said that - this patch is moving existing code - so
>>> this shouldn't be changed here - but it might be a distinct patch of
>>> it's own to come later.
>>
>> A fixed point helper template class, with template parameters for the
>> number of integer bits and the number of fractional bits:
>>
>> template<unsigned int, unsigned int>
>> class FixedPoint
>> {
>> 	...
>> };
>>
>> with conversion functions, in a fashion similar to
>> std::chrono::duration. Something for later.
>>
>>>> +	}
>>>> +}
>>>> +
>>>> +} /* namespace ipa::ipu3::algorithms */
>>>> +
>>>> +} /* namespace libcamera */
>>>> diff --git a/src/ipa/ipu3/algorithms/contrast.h b/src/ipa/ipu3/algorithms/contrast.h
>>>> new file mode 100644
>>>> index 00000000..50cb626f
>>>> --- /dev/null
>>>> +++ b/src/ipa/ipu3/algorithms/contrast.h
>>>> @@ -0,0 +1,33 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2021, Google
>>>> + *
>>>> + * constrast.h - IPU3 Contrast and Gamma control
>>>> + */
>>>> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__
>>>> +#define __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__
>>>> +
>>>> +#include "algorithm.h"
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +namespace ipa::ipu3::algorithms {
>>>> +
>>>> +class Contrast : public Algorithm
>>>> +{
>>>> +public:
>>>> +	Contrast();
>>>> +	~Contrast() = default;
>>
>> I don't think this is needed, and if it is, let's not make it an inline
>> function, you can define it in contrast.cpp as
>>
>> Contrast::~Contrast() = default;
>>
>>>> +
>>>> +	void prepare(IPAContext &context, ipu3_uapi_params &params) override;
>>>> +	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>>>> +
>>>> +private:
>>>> +	double gamma_;
>>
>> I wonder if the gamma value should be stored in the context structure. A
>> bit more design documentation could answer this question :-)
>>
>> Could you also please explain (just as a reply to this e-mail, not in
>> comments) why the gamma value was provided by the AGC algorithm, and the
>> gamma LUT filled by the AWB algorithm ? How does gamma relate, in
>> genetal, to AGC and/or AWB ?
> 
> The original AGC algorithm comes partially from a paper "A New Auto
> Exposure and Auto White-Balance Algorithm to Detect High Dynamic Range
> Conditions Using CMOS Technology", Quoc Kien Vuong, Se-Hwan Yun, and
> Suki Kim
> 
> The idea was to be able to distinguish when we are in a back lighting
> condition, in particular. On this original algorithm, adding a
> calculated gamma value to compensate the over/under exposure was used.
> 
> This has been abandonned but beeing able to change the default gamma
> correction table is interesting.
> Now, the gamma correction should not be controlled by AGC or AWB, but on
> its own, as well as the ipu3_uapi_isp_lin_vmem_params structure, which
> really controls the contrast (as it is here to linearize the non linear
> sensor).
> 
>> Also, this doesn't handle contrast at the moment. Do you plan to expand
>> the algorithm ? I also wonder if it shouldn't be called tonemapping
>> instead of contrast.
>>
>>>> +};
>>>> +
>>>> +} /* namespace ipa::ipu3::algorithms */
>>>> +
>>>> +} /* namespace libcamera */
>>>> +
>>>> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__ */
>>>> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
>>>> index 3fb3ce56..69a59096 100644
>>>> --- a/src/ipa/ipu3/algorithms/meson.build
>>>> +++ b/src/ipa/ipu3/algorithms/meson.build
>>>> @@ -1,5 +1,6 @@
>>>>  # SPDX-License-Identifier: CC0-1.0
>>>>  
>>>>  ipu3_ipa_algorithms = files([
>>>> -        'grid.cpp',
>>>
>>> Please fix this alignment in the patch that adds it.
>>>
>>>> +    'contrast.cpp',
>>>> +    'grid.cpp',
>>>>  ])
>>>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>>>> index 90b2f2c2..e995c663 100644
>>>> --- a/src/ipa/ipu3/ipa_context.h
>>>> +++ b/src/ipa/ipu3/ipa_context.h
>>>> @@ -33,6 +33,14 @@ struct IPAConfiguration {
>>>>   * lifetime, though for now a single instance is used.
>>>>   */
>>>>  struct IPAFrameContext {
>>>> +	struct Agc {
>>>> +		uint32_t exposure;
>>>> +		double gain;
>>>> +	} agc;
>>>> +
>>>> +	struct Contrast {
>>>> +		uint16_t lut[IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES];
>>>
>>> As highilghted earlier, I think this should be:
>>> 		struct ipu3_uapi_gamma_corr_lut lut
>>>
>>>> +	} contrast;
>>>>  };
>>>>  
>>>>  /* Global context of the IPA */
>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>>>> index 394a7a45..e09adfc3 100644
>>>> --- a/src/ipa/ipu3/ipu3.cpp
>>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>>> @@ -30,6 +30,7 @@
>>>>  #include "libcamera/internal/mapped_framebuffer.h"
>>>>  
>>>>  #include "algorithms/algorithm.h"
>>>> +#include "algorithms/contrast.h"
>>>>  #include "algorithms/grid.h"
>>>>  #include "ipu3_agc.h"
>>>>  #include "ipu3_awb.h"
>>>> @@ -167,6 +168,7 @@ int IPAIPU3::init(const IPASettings &settings,
>>>>  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
>>>>  	/* Construct our Algorithms */
>>>>  	algorithms_.emplace_back(new algorithms::Grid());
>>>> +	algorithms_.emplace_back(new algorithms::Contrast());
>>>>  
>>>>  	return 0;
>>>>  }
>>>> @@ -309,7 +311,7 @@ void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>>>>  		algo->prepare(context_, params_);
>>>>  
>>>>  	if (agcAlgo_->updateControls())
>>>> -		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
>>>> +		awbAlgo_->updateWbParameters(params_);
>>>>  
>>>>  	*params = params_;
>>>>  
>>>> @@ -324,9 +326,15 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>>>  			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>>>>  {
>>>>  	ControlList ctrls(controls::controls);
>>>
>>> New line here to separate local variable declarations from code segments.
>>>
>>>> +	for (auto const &algo : algorithms_)
>>>> +		algo->process(context_, stats);
>>>>  
>>>>  	double gain = camHelper_->gain(gain_);
>>>> -	agcAlgo_->process(stats, exposure_, gain);
>>>> +	context_.frameContext.agc.exposure = exposure_;
>>>> +	context_.frameContext.agc.gain = gain;
>>>
>>> the IPU3 layer sould not be filling in agc specific structures. That's a
>>> layering violation to me.
>>>
>>> The algorithms (in this instance, the agc?) own those structures, so
>>> they should be manipulating them.
>>
>> I think I agree, but how would you pass the exposure and gain parameters
>> to the algorithm in that case ?
> 
> Kieran, I agree with Laurent on this :-)


As I said below, I think we should keep this as is for now, I'm not
saying that we need to fix this for this patch - But it should be marked
with a todo as it's not 'right'.

We could ..
As an initial idea which should be disregarded other than to highlight
we can solve this issue later, and this is not thoughtout or designed or
considered:


 - Have a hook 'processControls()' which passing an incoming control
   list into algorithms as a point to say "The app is requesting these,
   are any for you" - where an algorithm could then read and handle
   controls it cares for.


My point was, simply from my perspective so far _frameContext.agc ...
does not belong to IPU3.


Dealing with incoming controls is an entire topic that needs to be
thought out (soon), such as "Is this a manually set override, so we
really must be setting a gain of X.

But ... and I'll stress this again - The aim of /this/ series is to move
the framework to be more modular such that it can be further developed.
We shouldn't try to tackle the entire redesign in this series.




>>> It's also showing where the algorithms need to know historical data.
>>> In this instance, presumably they need to know what gain or expsosure
>>> was set at the time that this buffer ran.
>>>
>>> This shows where we will need to keep historical versions of the
>>> FrameContext, and perhaps track them along with the frame counter.
>>>
>>> For now, while we get things moving to be more modular, I think we could
>>> keep this as it is - but it should be commented with a todo>>>
>>>> +	agcAlgo_->process(context_, stats);
>>>> +	exposure_ = context_.frameContext.agc.exposure;
>>>> +	gain = context_.frameContext.agc.gain;
>>>>  	gain_ = camHelper_->gainCode(gain);
>>>>  
>>>>  	awbAlgo_->calculateWBGains(stats);
>>>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
>>>> index 408eb849..c6782ff4 100644
>>>> --- a/src/ipa/ipu3/ipu3_agc.cpp
>>>> +++ b/src/ipa/ipu3/ipu3_agc.cpp
>>>> @@ -52,7 +52,7 @@ static constexpr uint8_t kCellSize = 8;
>>>>  
>>>>  IPU3Agc::IPU3Agc()
>>>>  	: frameCount_(0), lastFrame_(0), converged_(false),
>>>> -	  updateControls_(false), iqMean_(0.0), gamma_(1.0),
>>>> +	  updateControls_(false), iqMean_(0.0),
>>>>  	  lineDuration_(0s), maxExposureTime_(0s),
>>>>  	  prevExposure_(0s), prevExposureNoDg_(0s),
>>>>  	  currentExposure_(0s), currentExposureNoDg_(0s)
>>>> @@ -104,9 +104,6 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>>>>  		}
>>>>  	}
>>>>  
>>>> -	/* Limit the gamma effect for now */
>>>> -	gamma_ = 1.1;
>>>> -
>>>>  	/* Estimate the quantile mean of the top 2% of the histogram */
>>>>  	iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>>>>  }
>>>> @@ -193,8 +190,10 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>>>>  	lastFrame_ = frameCount_;
>>>>  }
>>>>  
>>>> -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain)
>>>> +void IPU3Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>>>>  {
>>>> +	uint32_t &exposure = context.frameContext.agc.exposure;
>>>> +	double &gain = context.frameContext.agc.gain;
>>>>  	processBrightness(stats);
>>>>  	lockExposureGain(exposure, gain);
>>>>  	frameCount_++;
>>>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
>>>> index f00b98d6..8b32e52f 100644
>>>> --- a/src/ipa/ipu3/ipu3_agc.h
>>>> +++ b/src/ipa/ipu3/ipu3_agc.h
>>>> @@ -30,11 +30,9 @@ public:
>>>>  	~IPU3Agc() = default;
>>>>  
>>>>  	void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);
>>>> -	void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);
>>>> +	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>>>>  	bool converged() { return converged_; }
>>>>  	bool updateControls() { return updateControls_; }
>>>> -	/* \todo Use a metadata exchange between IPAs */
>>>> -	double gamma() { return gamma_; }
>>>>  
>>>>  private:
>>>>  	void processBrightness(const ipu3_uapi_stats_3a *stats);
>>>> @@ -50,7 +48,6 @@ private:
>>>>  	bool updateControls_;
>>>>  
>>>>  	double iqMean_;
>>>> -	double gamma_;
>>>>  
>>>>  	Duration lineDuration_;
>>>>  	Duration maxExposureTime_;
>>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
>>>> index 911760b3..0f3bcdc9 100644
>>>> --- a/src/ipa/ipu3/ipu3_awb.cpp
>>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
>>>> @@ -114,31 +114,6 @@ static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
>>>>  	0, 0, 8191, 0
>>>>  };
>>>>  
>>>> -/* Default settings for Gamma correction */
>>>> -const struct ipu3_uapi_gamma_corr_lut imguCssGammaLut = { {
>>>> -	63, 79, 95, 111, 127, 143, 159, 175, 191, 207, 223, 239, 255, 271, 287,
>>>> -	303, 319, 335, 351, 367, 383, 399, 415, 431, 447, 463, 479, 495, 511,
>>>> -	527, 543, 559, 575, 591, 607, 623, 639, 655, 671, 687, 703, 719, 735,
>>>> -	751, 767, 783, 799, 815, 831, 847, 863, 879, 895, 911, 927, 943, 959,
>>>> -	975, 991, 1007, 1023, 1039, 1055, 1071, 1087, 1103, 1119, 1135, 1151,
>>>> -	1167, 1183, 1199, 1215, 1231, 1247, 1263, 1279, 1295, 1311, 1327, 1343,
>>>> -	1359, 1375, 1391, 1407, 1423, 1439, 1455, 1471, 1487, 1503, 1519, 1535,
>>>> -	1551, 1567, 1583, 1599, 1615, 1631, 1647, 1663, 1679, 1695, 1711, 1727,
>>>> -	1743, 1759, 1775, 1791, 1807, 1823, 1839, 1855, 1871, 1887, 1903, 1919,
>>>> -	1935, 1951, 1967, 1983, 1999, 2015, 2031, 2047, 2063, 2079, 2095, 2111,
>>>> -	2143, 2175, 2207, 2239, 2271, 2303, 2335, 2367, 2399, 2431, 2463, 2495,
>>>> -	2527, 2559, 2591, 2623, 2655, 2687, 2719, 2751, 2783, 2815, 2847, 2879,
>>>> -	2911, 2943, 2975, 3007, 3039, 3071, 3103, 3135, 3167, 3199, 3231, 3263,
>>>> -	3295, 3327, 3359, 3391, 3423, 3455, 3487, 3519, 3551, 3583, 3615, 3647,
>>>> -	3679, 3711, 3743, 3775, 3807, 3839, 3871, 3903, 3935, 3967, 3999, 4031,
>>>> -	4063, 4095, 4127, 4159, 4223, 4287, 4351, 4415, 4479, 4543, 4607, 4671,
>>>> -	4735, 4799, 4863, 4927, 4991, 5055, 5119, 5183, 5247, 5311, 5375, 5439,
>>>> -	5503, 5567, 5631, 5695, 5759, 5823, 5887, 5951, 6015, 6079, 6143, 6207,
>>>> -	6271, 6335, 6399, 6463, 6527, 6591, 6655, 6719, 6783, 6847, 6911, 6975,
>>>> -	7039, 7103, 7167, 7231, 7295, 7359, 7423, 7487, 7551, 7615, 7679, 7743,
>>>> -	7807, 7871, 7935, 7999, 8063, 8127, 8191
>>>> -} };
>>>> -
>>>>  IPU3Awb::IPU3Awb()
>>>>  	: Algorithm()
>>>>  {
>>>> @@ -179,10 +154,6 @@ void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, st
>>>>  	params.use.acc_ccm = 1;
>>>>  	params.acc_param.ccm = imguCssCcmDefault;
>>>>  
>>>> -	params.use.acc_gamma = 1;
>>>> -	params.acc_param.gamma.gc_lut = imguCssGammaLut;
>>>> -	params.acc_param.gamma.gc_ctrl.enable = 1;
>>>> -
>>>>  	zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
>>>>  }
>>>>  
>>>> @@ -332,7 +303,7 @@ void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
>>>>  	}
>>>>  }
>>>>  
>>>> -void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
>>>> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params)
>>>>  {
>>>>  	/*
>>>>  	 * Green gains should not be touched and considered 1.
>>>> @@ -344,18 +315,10 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
>>>>  	params.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;
>>>>  	params.acc_param.bnr.wb_gains.gb = 16;
>>>>  
>>>> -	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK
>>>> -			    << " and gamma calculated: " << agcGamma;
>>>> +	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK;
>>>>  
>>>>  	/* The CCM matrix may change when color temperature will be used */
>>>>  	params.acc_param.ccm = imguCssCcmDefault;
>>>> -
>>>> -	for (uint32_t i = 0; i < 256; i++) {
>>>> -		double j = i / 255.0;
>>>> -		double gamma = std::pow(j, 1.0 / agcGamma);
>>>> -		/* The maximum value 255 is represented on 13 bits in the IPU3 */
>>>> -		params.acc_param.gamma.gc_lut.lut[i] = gamma * 8191;
>>>> -	}
>>>>  }
>>>>  
>>>>  } /* namespace ipa::ipu3 */
>>>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
>>>> index ea2d4320..eeb2920b 100644
>>>> --- a/src/ipa/ipu3/ipu3_awb.h
>>>> +++ b/src/ipa/ipu3/ipu3_awb.h
>>>> @@ -31,7 +31,7 @@ public:
>>>>  
>>>>  	void initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid);
>>>>  	void calculateWBGains(const ipu3_uapi_stats_3a *stats);
>>>> -	void updateWbParameters(ipu3_uapi_params &params, double agcGamma);
>>>> +	void updateWbParameters(ipu3_uapi_params &params);
>>>>  
>>>>  	struct Ipu3AwbCell {
>>>>  		unsigned char greenRedAvg;
>>