[libcamera-devel,v3,5/9] ipa: ipu3: Introduce a modular contrast algorithm
diff mbox series

Message ID 20210818155403.268694-6-jeanmichel.hautbois@ideasonboard.com
State Superseded
Headers show
Series
  • IPU3: Introduce modularity for algorithms
Related show

Commit Message

Jean-Michel Hautbois Aug. 18, 2021, 3:53 p.m. UTC 
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 initialisation 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 is 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>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/contrast.cpp | 53 ++++++++++++++++++++++++++++
 src/ipa/ipu3/algorithms/contrast.h   | 32 +++++++++++++++++
 src/ipa/ipu3/algorithms/meson.build  |  1 +
 src/ipa/ipu3/ipa_context.h           |  8 +++++
 src/ipa/ipu3/ipu3.cpp                | 15 +++++---
 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 +-
 9 files changed, 113 insertions(+), 53 deletions(-)
 create mode 100644 src/ipa/ipu3/algorithms/contrast.cpp
 create mode 100644 src/ipa/ipu3/algorithms/contrast.h

Comments

Laurent Pinchart Aug. 18, 2021, 9:41 p.m. UTC | #1 
Hi Jean-Michel,

Thank you for the patch.

On Wed, Aug 18, 2021 at 05:53:59PM +0200, 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
> 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 initialisation 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 is 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>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/contrast.cpp | 53 ++++++++++++++++++++++++++++
>  src/ipa/ipu3/algorithms/contrast.h   | 32 +++++++++++++++++
>  src/ipa/ipu3/algorithms/meson.build  |  1 +
>  src/ipa/ipu3/ipa_context.h           |  8 +++++
>  src/ipa/ipu3/ipu3.cpp                | 15 +++++---
>  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 +-
>  9 files changed, 113 insertions(+), 53 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/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp
> new file mode 100644
> index 00000000..eb92d3c7
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/contrast.cpp
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google inc.
> + *
> + * contrast.cpp - IPU3 Contrast and Gamma control
> + */
> +
> +#include "contrast.h"
> +
> +#include <cmath>
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3::algorithms {
> +
> +

One extra blank line.

> +Contrast::Contrast()
> +	: gamma_(1.0)
> +{
> +}
> +
> +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params &params)
> +{
> +	/* Copy the calculated LUT into the parameters buffer */

s/buffer/buffer./

> +	memcpy(params.acc_param.gamma.gc_lut.lut,

Missing header.

> +	       context.frameContext.contrast.gammaCorrection.lut,
> +	       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES * sizeof(params.acc_param.gamma.gc_lut.lut[0]));

Line wrap:

	       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *
	       sizeof(params.acc_param.gamma.gc_lut.lut[0]));

> +
> +	/* Enable the custom gamma table */

s/table/table./

I won't repeat this comment everywhere, you can apply it through the
series.

> +	params.use.acc_gamma = 1;
> +	params.acc_param.gamma.gc_ctrl.enable = 1;
> +}
> +
> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)

Line wrap.

I'll stop repeating this comment too :-)

> +{
> +	/* Limit the gamma effect for now */

What do you mean by "limit" ? I understand it as

	/*
	 * Hardcode gamma to 1.1 as a default for now.
	 *
	 * \todo Expose gamma control setting through the libcamera control API
	 */

> +	/* \todo expose gamma control setting through the libcamera control API */
> +	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 = static_cast<double>(i)
> +			 / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);
> +		double gamma = std::pow(j, 1.0 / gamma_);
> +
> +		/* The maximum value 255 is represented on 13 bits in the IPU3 */

I think "The output value is expressed on 13 bits." would be clearer.

> +		context.frameContext.contrast.gammaCorrection.lut[i] = gamma * 8191;
> +	}

How about using std::size to avoid overflowing the array by mistake ?

	struct ipu3_uapi_gamma_corr_lut &lut =
		context.frameContext.contrast.gammaCorrection;

	for (uint32_t i = 0; i < std::size(lut.lut); i++) {
		double j = static_cast<double>(i) / (std::size(lut.lut) - 1);
		double gamma = std::pow(j, 1.0 / gamma_);

		/* The output value is expressed on 13 bits. */
		lut.lut[i] = gamma * 8191;
	}

Up to you.

> +}
> +
> +} /* 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..6cd6c5db
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/contrast.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google inc.
> + *
> + * contrast.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

I wonder of the algorithm shouldn't be called ToneMapping as it does
more than just contrast.

> +{
> +public:
> +	Contrast();
> +	~Contrast() = default;

You can drop the destructor, it's not needed.

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

Missing blank line.

> +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 dc538b79..e3ff3b78 100644
> --- a/src/ipa/ipu3/algorithms/meson.build
> +++ b/src/ipa/ipu3/algorithms/meson.build
> @@ -2,4 +2,5 @@
>  
>  ipu3_ipa_algorithms = files([
>      'algorithm.cpp',
> +    'contrast.cpp',
>  ])
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 46291d9e..5964ba6d 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -24,6 +24,14 @@ struct IPAConfiguration {
>  };
>  
>  struct IPAFrameContext {
> +	struct Agc {
> +		uint32_t exposure;
> +		double gain;
> +	} agc;

I think this belongs to patch 7/9, along with the changes to
ipu3_agc.cpp and ipu3_agc.h.

> +
> +	struct Contrast {
> +		struct ipu3_uapi_gamma_corr_lut gammaCorrection;
> +	} contrast;

Missing documentation :-)

>  };
>  
>  struct IPAContext {
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index dc6f0735..ee0dd9fe 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 "ipu3_agc.h"
>  #include "ipu3_awb.h"
>  #include "libipa/camera_sensor_helper.h"
> @@ -218,6 +219,9 @@ int IPAIPU3::init(const IPASettings &settings,
>  
>  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
>  
> +	/* Construct our Algorithms */
> +	algorithms_.emplace_back(new algorithms::Contrast());

Or

	algorithms_.push_back(std::make_unique<algorithms::Contrast>());

? It won't make much practical difference, but may better show we're
dealing with unique pointers.

> +
>  	return 0;
>  }
>  
> @@ -416,7 +420,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_;
>  
> @@ -431,13 +435,16 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>  {
>  	ControlList ctrls(controls::controls);
> +	/* \todo These fields should not be written by the IPAIPU3 layer */
> +	context_.frameContext.agc.gain = camHelper_->gain(gain_);
> +	context_.frameContext.agc.exposure = exposure_;
>  
>  	for (auto const &algo : algorithms_)
>  		algo->process(context_, stats);
>  
> -	double gain = camHelper_->gain(gain_);
> -	agcAlgo_->process(stats, exposure_, gain);
> -	gain_ = camHelper_->gainCode(gain);
> +	agcAlgo_->process(context_, stats);
> +	exposure_ = context_.frameContext.agc.exposure;
> +	gain_ = camHelper_->gainCode(context_.frameContext.agc.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..f8290abd 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);
>  	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 4bb321b3..c2d9e0c1 100644
> --- a/src/ipa/ipu3/ipu3_awb.cpp
> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> @@ -133,31 +133,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()
>  {
> @@ -197,10 +172,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);
>  }
>  
> @@ -350,7 +321,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.
> @@ -362,18 +333,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;
>
Jean-Michel Hautbois Aug. 19, 2021, 8:11 a.m. UTC | #2 
Hi Laurent,

On 18/08/2021 23:41, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Wed, Aug 18, 2021 at 05:53:59PM +0200, 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
>> 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 initialisation 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 is 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>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/ipa/ipu3/algorithms/contrast.cpp | 53 ++++++++++++++++++++++++++++
>>  src/ipa/ipu3/algorithms/contrast.h   | 32 +++++++++++++++++
>>  src/ipa/ipu3/algorithms/meson.build  |  1 +
>>  src/ipa/ipu3/ipa_context.h           |  8 +++++
>>  src/ipa/ipu3/ipu3.cpp                | 15 +++++---
>>  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 +-
>>  9 files changed, 113 insertions(+), 53 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/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp
>> new file mode 100644
>> index 00000000..eb92d3c7
>> --- /dev/null
>> +++ b/src/ipa/ipu3/algorithms/contrast.cpp
>> @@ -0,0 +1,53 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Google inc.
>> + *
>> + * contrast.cpp - IPU3 Contrast and Gamma control
>> + */
>> +
>> +#include "contrast.h"
>> +
>> +#include <cmath>
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::ipu3::algorithms {
>> +
>> +
> 
> One extra blank line.
> 
>> +Contrast::Contrast()
>> +	: gamma_(1.0)
>> +{
>> +}
>> +
>> +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params &params)
>> +{
>> +	/* Copy the calculated LUT into the parameters buffer */
> 
> s/buffer/buffer./
> 
>> +	memcpy(params.acc_param.gamma.gc_lut.lut,
> 
> Missing header.
> 
>> +	       context.frameContext.contrast.gammaCorrection.lut,
>> +	       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES * sizeof(params.acc_param.gamma.gc_lut.lut[0]));
> 
> Line wrap:
> 
> 	       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *
> 	       sizeof(params.acc_param.gamma.gc_lut.lut[0]));
> 
>> +
>> +	/* Enable the custom gamma table */
> 
> s/table/table./
> 
> I won't repeat this comment everywhere, you can apply it through the
> series.
> 
>> +	params.use.acc_gamma = 1;
>> +	params.acc_param.gamma.gc_ctrl.enable = 1;
>> +}
>> +
>> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> 
> Line wrap.
> 
> I'll stop repeating this comment too :-)
> 
>> +{
>> +	/* Limit the gamma effect for now */
> 
> What do you mean by "limit" ? I understand it as
> 
> 	/*
> 	 * Hardcode gamma to 1.1 as a default for now.
> 	 *
> 	 * \todo Expose gamma control setting through the libcamera control API
> 	 */
> 
>> +	/* \todo expose gamma control setting through the libcamera control API */
>> +	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 = static_cast<double>(i)
>> +			 / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);
>> +		double gamma = std::pow(j, 1.0 / gamma_);
>> +
>> +		/* The maximum value 255 is represented on 13 bits in the IPU3 */
> 
> I think "The output value is expressed on 13 bits." would be clearer.
> 
>> +		context.frameContext.contrast.gammaCorrection.lut[i] = gamma * 8191;
>> +	}
> 
> How about using std::size to avoid overflowing the array by mistake ?
> 
> 	struct ipu3_uapi_gamma_corr_lut &lut =
> 		context.frameContext.contrast.gammaCorrection;
> 
> 	for (uint32_t i = 0; i < std::size(lut.lut); i++) {
> 		double j = static_cast<double>(i) / (std::size(lut.lut) - 1);
> 		double gamma = std::pow(j, 1.0 / gamma_);
> 
> 		/* The output value is expressed on 13 bits. */
> 		lut.lut[i] = gamma * 8191;
> 	}
> 
> Up to you.
> 
>> +}
>> +
>> +} /* 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..6cd6c5db
>> --- /dev/null
>> +++ b/src/ipa/ipu3/algorithms/contrast.h
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Google inc.
>> + *
>> + * contrast.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
> 
> I wonder of the algorithm shouldn't be called ToneMapping as it does
> more than just contrast.
> 
>> +{
>> +public:
>> +	Contrast();
>> +	~Contrast() = default;
> 
> You can drop the destructor, it's not needed.
> 
>> +
>> +	void prepare(IPAContext &context, ipu3_uapi_params &params) override;
>> +	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> 
> Missing blank line.
> 
>> +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 dc538b79..e3ff3b78 100644
>> --- a/src/ipa/ipu3/algorithms/meson.build
>> +++ b/src/ipa/ipu3/algorithms/meson.build
>> @@ -2,4 +2,5 @@
>>  
>>  ipu3_ipa_algorithms = files([
>>      'algorithm.cpp',
>> +    'contrast.cpp',
>>  ])
>> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
>> index 46291d9e..5964ba6d 100644
>> --- a/src/ipa/ipu3/ipa_context.h
>> +++ b/src/ipa/ipu3/ipa_context.h
>> @@ -24,6 +24,14 @@ struct IPAConfiguration {
>>  };
>>  
>>  struct IPAFrameContext {
>> +	struct Agc {
>> +		uint32_t exposure;
>> +		double gain;
>> +	} agc;
> 
> I think this belongs to patch 7/9, along with the changes to
> ipu3_agc.cpp and ipu3_agc.h.
> 
>> +
>> +	struct Contrast {
>> +		struct ipu3_uapi_gamma_corr_lut gammaCorrection;
>> +	} contrast;
> 
> Missing documentation :-)
> 

Should I add the doc in tone_mapping.cpp (yes, I changed the name :-))
or in ipu3.cpp as the rest of the structure ?

>>  };
>>  
>>  struct IPAContext {
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index dc6f0735..ee0dd9fe 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 "ipu3_agc.h"
>>  #include "ipu3_awb.h"
>>  #include "libipa/camera_sensor_helper.h"
>> @@ -218,6 +219,9 @@ int IPAIPU3::init(const IPASettings &settings,
>>  
>>  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
>>  
>> +	/* Construct our Algorithms */
>> +	algorithms_.emplace_back(new algorithms::Contrast());
> 
> Or
> 
> 	algorithms_.push_back(std::make_unique<algorithms::Contrast>());
> 
> ? It won't make much practical difference, but may better show we're
> dealing with unique pointers.
> 

OK for me !

>> +
>>  	return 0;
>>  }
>>  
>> @@ -416,7 +420,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_;
>>  
>> @@ -431,13 +435,16 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>  			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
>>  {
>>  	ControlList ctrls(controls::controls);
>> +	/* \todo These fields should not be written by the IPAIPU3 layer */
>> +	context_.frameContext.agc.gain = camHelper_->gain(gain_);
>> +	context_.frameContext.agc.exposure = exposure_;
>>  
>>  	for (auto const &algo : algorithms_)
>>  		algo->process(context_, stats);
>>  
>> -	double gain = camHelper_->gain(gain_);
>> -	agcAlgo_->process(stats, exposure_, gain);
>> -	gain_ = camHelper_->gainCode(gain);
>> +	agcAlgo_->process(context_, stats);
>> +	exposure_ = context_.frameContext.agc.exposure;
>> +	gain_ = camHelper_->gainCode(context_.frameContext.agc.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..f8290abd 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);
>>  	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 4bb321b3..c2d9e0c1 100644
>> --- a/src/ipa/ipu3/ipu3_awb.cpp
>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
>> @@ -133,31 +133,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()
>>  {
>> @@ -197,10 +172,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);
>>  }
>>  
>> @@ -350,7 +321,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.
>> @@ -362,18 +333,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;
>>
>
Laurent Pinchart Aug. 19, 2021, 9:53 a.m. UTC | #3 
Hi Jean-Michel,

On Thu, Aug 19, 2021 at 10:11:11AM +0200, Jean-Michel Hautbois wrote:
> On 18/08/2021 23:41, Laurent Pinchart wrote:
> > On Wed, Aug 18, 2021 at 05:53:59PM +0200, 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
> >> 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 initialisation 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 is 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>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  src/ipa/ipu3/algorithms/contrast.cpp | 53 ++++++++++++++++++++++++++++
> >>  src/ipa/ipu3/algorithms/contrast.h   | 32 +++++++++++++++++
> >>  src/ipa/ipu3/algorithms/meson.build  |  1 +
> >>  src/ipa/ipu3/ipa_context.h           |  8 +++++
> >>  src/ipa/ipu3/ipu3.cpp                | 15 +++++---
> >>  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 +-
> >>  9 files changed, 113 insertions(+), 53 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/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp
> >> new file mode 100644
> >> index 00000000..eb92d3c7
> >> --- /dev/null
> >> +++ b/src/ipa/ipu3/algorithms/contrast.cpp
> >> @@ -0,0 +1,53 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Google inc.
> >> + *
> >> + * contrast.cpp - IPU3 Contrast and Gamma control
> >> + */
> >> +
> >> +#include "contrast.h"
> >> +
> >> +#include <cmath>
> >> +
> >> +namespace libcamera {
> >> +
> >> +namespace ipa::ipu3::algorithms {
> >> +
> >> +
> > 
> > One extra blank line.
> > 
> >> +Contrast::Contrast()
> >> +	: gamma_(1.0)
> >> +{
> >> +}
> >> +
> >> +void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params &params)
> >> +{
> >> +	/* Copy the calculated LUT into the parameters buffer */
> > 
> > s/buffer/buffer./
> > 
> >> +	memcpy(params.acc_param.gamma.gc_lut.lut,
> > 
> > Missing header.
> > 
> >> +	       context.frameContext.contrast.gammaCorrection.lut,
> >> +	       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES * sizeof(params.acc_param.gamma.gc_lut.lut[0]));
> > 
> > Line wrap:
> > 
> > 	       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES *
> > 	       sizeof(params.acc_param.gamma.gc_lut.lut[0]));
> > 
> >> +
> >> +	/* Enable the custom gamma table */
> > 
> > s/table/table./
> > 
> > I won't repeat this comment everywhere, you can apply it through the
> > series.
> > 
> >> +	params.use.acc_gamma = 1;
> >> +	params.acc_param.gamma.gc_ctrl.enable = 1;
> >> +}
> >> +
> >> +void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> > 
> > Line wrap.
> > 
> > I'll stop repeating this comment too :-)
> > 
> >> +{
> >> +	/* Limit the gamma effect for now */
> > 
> > What do you mean by "limit" ? I understand it as
> > 
> > 	/*
> > 	 * Hardcode gamma to 1.1 as a default for now.
> > 	 *
> > 	 * \todo Expose gamma control setting through the libcamera control API
> > 	 */
> > 
> >> +	/* \todo expose gamma control setting through the libcamera control API */
> >> +	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 = static_cast<double>(i)
> >> +			 / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);
> >> +		double gamma = std::pow(j, 1.0 / gamma_);
> >> +
> >> +		/* The maximum value 255 is represented on 13 bits in the IPU3 */
> > 
> > I think "The output value is expressed on 13 bits." would be clearer.
> > 
> >> +		context.frameContext.contrast.gammaCorrection.lut[i] = gamma * 8191;
> >> +	}
> > 
> > How about using std::size to avoid overflowing the array by mistake ?
> > 
> > 	struct ipu3_uapi_gamma_corr_lut &lut =
> > 		context.frameContext.contrast.gammaCorrection;
> > 
> > 	for (uint32_t i = 0; i < std::size(lut.lut); i++) {
> > 		double j = static_cast<double>(i) / (std::size(lut.lut) - 1);
> > 		double gamma = std::pow(j, 1.0 / gamma_);
> > 
> > 		/* The output value is expressed on 13 bits. */
> > 		lut.lut[i] = gamma * 8191;
> > 	}
> > 
> > Up to you.
> > 
> >> +}
> >> +
> >> +} /* 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..6cd6c5db
> >> --- /dev/null
> >> +++ b/src/ipa/ipu3/algorithms/contrast.h
> >> @@ -0,0 +1,32 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Google inc.
> >> + *
> >> + * contrast.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
> > 
> > I wonder of the algorithm shouldn't be called ToneMapping as it does
> > more than just contrast.
> > 
> >> +{
> >> +public:
> >> +	Contrast();
> >> +	~Contrast() = default;
> > 
> > You can drop the destructor, it's not needed.
> > 
> >> +
> >> +	void prepare(IPAContext &context, ipu3_uapi_params &params) override;
> >> +	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> > 
> > Missing blank line.
> > 
> >> +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 dc538b79..e3ff3b78 100644
> >> --- a/src/ipa/ipu3/algorithms/meson.build
> >> +++ b/src/ipa/ipu3/algorithms/meson.build
> >> @@ -2,4 +2,5 @@
> >>  
> >>  ipu3_ipa_algorithms = files([
> >>      'algorithm.cpp',
> >> +    'contrast.cpp',
> >>  ])
> >> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> >> index 46291d9e..5964ba6d 100644
> >> --- a/src/ipa/ipu3/ipa_context.h
> >> +++ b/src/ipa/ipu3/ipa_context.h
> >> @@ -24,6 +24,14 @@ struct IPAConfiguration {
> >>  };
> >>  
> >>  struct IPAFrameContext {
> >> +	struct Agc {
> >> +		uint32_t exposure;
> >> +		double gain;
> >> +	} agc;
> > 
> > I think this belongs to patch 7/9, along with the changes to
> > ipu3_agc.cpp and ipu3_agc.h.
> > 
> >> +
> >> +	struct Contrast {
> >> +		struct ipu3_uapi_gamma_corr_lut gammaCorrection;
> >> +	} contrast;
> > 
> > Missing documentation :-)
> 
> Should I add the doc in tone_mapping.cpp (yes, I changed the name :-))
> or in ipu3.cpp as the rest of the structure ?

The structure is documented in ipu3.cpp, so that's where the field
should be documented. If desired we could move the Contrast structure
definition to tone_mapping.cpp, and the documentation of the structure
and its gammaCorrection field should then move to tone_mapping.cpp, but
the contract field of IPAFrameContext should go with the definition of
IPAFrameContext itself.

> >>  };
> >>  
> >>  struct IPAContext {
> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> >> index dc6f0735..ee0dd9fe 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 "ipu3_agc.h"
> >>  #include "ipu3_awb.h"
> >>  #include "libipa/camera_sensor_helper.h"
> >> @@ -218,6 +219,9 @@ int IPAIPU3::init(const IPASettings &settings,
> >>  
> >>  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> >>  
> >> +	/* Construct our Algorithms */
> >> +	algorithms_.emplace_back(new algorithms::Contrast());
> > 
> > Or
> > 
> > 	algorithms_.push_back(std::make_unique<algorithms::Contrast>());
> > 
> > ? It won't make much practical difference, but may better show we're
> > dealing with unique pointers.
> 
> OK for me !
> 
> >> +
> >>  	return 0;
> >>  }
> >>  
> >> @@ -416,7 +420,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_;
> >>  
> >> @@ -431,13 +435,16 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> >>  			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
> >>  {
> >>  	ControlList ctrls(controls::controls);
> >> +	/* \todo These fields should not be written by the IPAIPU3 layer */
> >> +	context_.frameContext.agc.gain = camHelper_->gain(gain_);
> >> +	context_.frameContext.agc.exposure = exposure_;
> >>  
> >>  	for (auto const &algo : algorithms_)
> >>  		algo->process(context_, stats);
> >>  
> >> -	double gain = camHelper_->gain(gain_);
> >> -	agcAlgo_->process(stats, exposure_, gain);
> >> -	gain_ = camHelper_->gainCode(gain);
> >> +	agcAlgo_->process(context_, stats);
> >> +	exposure_ = context_.frameContext.agc.exposure;
> >> +	gain_ = camHelper_->gainCode(context_.frameContext.agc.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..f8290abd 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);
> >>  	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 4bb321b3..c2d9e0c1 100644
> >> --- a/src/ipa/ipu3/ipu3_awb.cpp
> >> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> >> @@ -133,31 +133,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()
> >>  {
> >> @@ -197,10 +172,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);
> >>  }
> >>  
> >> @@ -350,7 +321,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.
> >> @@ -362,18 +333,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;
> >>

Patch
diff mbox series 

diff --git a/src/ipa/ipu3/algorithms/contrast.cpp b/src/ipa/ipu3/algorithms/contrast.cpp
new file mode 100644
index 00000000..eb92d3c7
--- /dev/null
+++ b/src/ipa/ipu3/algorithms/contrast.cpp
@@ -0,0 +1,53 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2021, Google inc.
+ *
+ * contrast.cpp - IPU3 Contrast and Gamma control
+ */
+
+#include "contrast.h"
+
+#include <cmath>
+
+namespace libcamera {
+
+namespace ipa::ipu3::algorithms {
+
+
+Contrast::Contrast()
+	: gamma_(1.0)
+{
+}
+
+void Contrast::prepare([[maybe_unused]] IPAContext &context, ipu3_uapi_params &params)
+{
+	/* Copy the calculated LUT into the parameters buffer */
+	memcpy(params.acc_param.gamma.gc_lut.lut,
+	       context.frameContext.contrast.gammaCorrection.lut,
+	       IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES * sizeof(params.acc_param.gamma.gc_lut.lut[0]));
+
+	/* Enable the custom gamma table */
+	params.use.acc_gamma = 1;
+	params.acc_param.gamma.gc_ctrl.enable = 1;
+}
+
+void Contrast::process([[maybe_unused]] IPAContext &context, [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
+{
+	/* Limit the gamma effect for now */
+	/* \todo expose gamma control setting through the libcamera control API */
+	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 = static_cast<double>(i)
+			 / (IPU3_UAPI_GAMMA_CORR_LUT_ENTRIES - 1);
+		double gamma = std::pow(j, 1.0 / gamma_);
+
+		/* The maximum value 255 is represented on 13 bits in the IPU3 */
+		context.frameContext.contrast.gammaCorrection.lut[i] = gamma * 8191;
+	}
+}
+
+} /* 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..6cd6c5db
--- /dev/null
+++ b/src/ipa/ipu3/algorithms/contrast.h
@@ -0,0 +1,32 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2021, Google inc.
+ *
+ * contrast.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 dc538b79..e3ff3b78 100644
--- a/src/ipa/ipu3/algorithms/meson.build
+++ b/src/ipa/ipu3/algorithms/meson.build
@@ -2,4 +2,5 @@ 
 
 ipu3_ipa_algorithms = files([
     'algorithm.cpp',
+    'contrast.cpp',
 ])
diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
index 46291d9e..5964ba6d 100644
--- a/src/ipa/ipu3/ipa_context.h
+++ b/src/ipa/ipu3/ipa_context.h
@@ -24,6 +24,14 @@  struct IPAConfiguration {
 };
 
 struct IPAFrameContext {
+	struct Agc {
+		uint32_t exposure;
+		double gain;
+	} agc;
+
+	struct Contrast {
+		struct ipu3_uapi_gamma_corr_lut gammaCorrection;
+	} contrast;
 };
 
 struct IPAContext {
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
index dc6f0735..ee0dd9fe 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 "ipu3_agc.h"
 #include "ipu3_awb.h"
 #include "libipa/camera_sensor_helper.h"
@@ -218,6 +219,9 @@  int IPAIPU3::init(const IPASettings &settings,
 
 	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
 
+	/* Construct our Algorithms */
+	algorithms_.emplace_back(new algorithms::Contrast());
+
 	return 0;
 }
 
@@ -416,7 +420,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_;
 
@@ -431,13 +435,16 @@  void IPAIPU3::parseStatistics(unsigned int frame,
 			      [[maybe_unused]] const ipu3_uapi_stats_3a *stats)
 {
 	ControlList ctrls(controls::controls);
+	/* \todo These fields should not be written by the IPAIPU3 layer */
+	context_.frameContext.agc.gain = camHelper_->gain(gain_);
+	context_.frameContext.agc.exposure = exposure_;
 
 	for (auto const &algo : algorithms_)
 		algo->process(context_, stats);
 
-	double gain = camHelper_->gain(gain_);
-	agcAlgo_->process(stats, exposure_, gain);
-	gain_ = camHelper_->gainCode(gain);
+	agcAlgo_->process(context_, stats);
+	exposure_ = context_.frameContext.agc.exposure;
+	gain_ = camHelper_->gainCode(context_.frameContext.agc.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..f8290abd 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);
 	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 4bb321b3..c2d9e0c1 100644
--- a/src/ipa/ipu3/ipu3_awb.cpp
+++ b/src/ipa/ipu3/ipu3_awb.cpp
@@ -133,31 +133,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()
 {
@@ -197,10 +172,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);
 }
 
@@ -350,7 +321,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.
@@ -362,18 +333,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;