[libcamera-devel,v5,3/4] ipa: ipu3: Add support for IPU3 AWB algorithm

11960 diff mbox series

The IPA will locally modify the parameters before they are passed down
to the ImgU. Use a local parameter object to give a reference to those
algorithms.

Inherit from the Algorithm class to implement basic AWB functions.

The configure() call will set exposure and gain to their minimum value,
so while AGC is not there, the frames will be dark.

Once AWB is done, a color temperature is estimated and a default CCM matrix
will be used (yet to be tuned).
Implement a basic "grey-world" AWB algorithm just for demonstration purpose.

The BDS output size is passed by the pipeline handler to the IPA.
The best grid is then calculated to maximize the number of pixels taken
into account in each cells.

As commented in the source code, it can be improved, as it has (at least)
one limitation: if a cell is big (say 128 pixels wide) and indicated as
saturated, it won't be taken into account at all.
Maybe is it possible to have a smaller one, at the cost of a few pixels
to lose, in which case we can center the grid using the x_start and
y_start parameters.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp     |  86 +++++++++-
 src/ipa/ipu3/ipu3_awb.cpp | 351 ++++++++++++++++++++++++++++++++++++++
 src/ipa/ipu3/ipu3_awb.h   |  91 ++++++++++
 src/ipa/ipu3/meson.build  |   7 +-
 4 files changed, 526 insertions(+), 9 deletions(-)
 create mode 100644 src/ipa/ipu3/ipu3_awb.cpp
 create mode 100644 src/ipa/ipu3/ipu3_awb.h

Hi JM,

On 16/04/2021 08:49, Jean-Michel Hautbois wrote:
> The IPA will locally modify the parameters before they are passed down
> to the ImgU. Use a local parameter object to give a reference to those
> algorithms.
> 
> Inherit from the Algorithm class to implement basic AWB functions.
> 
> The configure() call will set exposure and gain to their minimum value,
> so while AGC is not there, the frames will be dark.
> 
> Once AWB is done, a color temperature is estimated and a default CCM matrix
> will be used (yet to be tuned).
> Implement a basic "grey-world" AWB algorithm just for demonstration purpose.
> 
> The BDS output size is passed by the pipeline handler to the IPA.
> The best grid is then calculated to maximize the number of pixels taken
> into account in each cells.
> 
> As commented in the source code, it can be improved, as it has (at least)
> one limitation: if a cell is big (say 128 pixels wide) and indicated as
> saturated, it won't be taken into account at all.
> Maybe is it possible to have a smaller one, at the cost of a few pixels
> to lose, in which case we can center the grid using the x_start and
> y_start parameters.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp     |  86 +++++++++-
>  src/ipa/ipu3/ipu3_awb.cpp | 351 ++++++++++++++++++++++++++++++++++++++
>  src/ipa/ipu3/ipu3_awb.h   |  91 ++++++++++
>  src/ipa/ipu3/meson.build  |   7 +-
>  4 files changed, 526 insertions(+), 9 deletions(-)
>  create mode 100644 src/ipa/ipu3/ipu3_awb.cpp
>  create mode 100644 src/ipa/ipu3/ipu3_awb.h
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 34a907f2..ab052a8a 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -21,6 +21,11 @@
>  #include "libcamera/internal/buffer.h"
>  #include "libcamera/internal/log.h"
>  
> +#include "ipu3_awb.h"
> +
> +static constexpr uint32_t kMaxCellWidthPerSet = 160;
> +static constexpr uint32_t kMaxCellHeightPerSet = 80;
> +
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPAIPU3)
> @@ -49,6 +54,7 @@ private:
>  			     const ipu3_uapi_stats_3a *stats);
>  
>  	void setControls(unsigned int frame);
> +	void calculateBdsGrid(const Size &bdsOutputSize);
>  
>  	std::map<unsigned int, MappedFrameBuffer> buffers_;
>  
> @@ -61,6 +67,14 @@ private:
>  	uint32_t gain_;
>  	uint32_t minGain_;
>  	uint32_t maxGain_;
> +
> +	/* Interface to the AWB algorithm */
> +	std::unique_ptr<ipa::IPU3Awb> awbAlgo_;
> +
> +	/* Local parameter storage */
> +	struct ipu3_uapi_params params_;
> +
> +	struct ipu3_uapi_grid_config bdsGrid_;
>  };
>  
>  int IPAIPU3::start()
> @@ -70,8 +84,58 @@ int IPAIPU3::start()
>  	return 0;
>  }
>  
> +/**
> + * This method calculates a grid for the AWB algorithm in the IPU3 firmware.
> + * Its input is the BDS output size calculated in the ImgU.
> + * It is limited for now to the simplest method: find the lesser error
> + * with the width/height and respective log2 width/height of the cells.
> + *
> + * \todo The frame is divided into cells which can be 8x8 => 128x128.
> + * As a smaller cell improves the algorithm precision, adapting the
> + * x_start and y_start parameters of the grid would provoke a loss of
> + * some pixels but would also result in more accurate algorithms.
> + */
> +void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
> +{
> +	uint32_t minError = std::numeric_limits<uint32_t>::max();
> +	Size best;
> +	Size bestLog2;
> +	bdsGrid_ = {};
> +
> +	for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
> +		uint32_t width = std::min(kMaxCellWidthPerSet,
> +					  bdsOutputSize.width >> widthShift);
> +		width = width << widthShift;
> +		for (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {
> +			int32_t height = std::min(kMaxCellHeightPerSet,
> +						  bdsOutputSize.height >> heightShift);
> +			height = height << heightShift;
> +			uint32_t error  = std::abs(static_cast<int>(width - bdsOutputSize.width))
> +							+ std::abs(static_cast<int>(height - bdsOutputSize.height));
> +
> +			if (error > minError)
> +				continue;
> +
> +			minError = error;
> +			best.width = width;
> +			best.height = height;
> +			bestLog2.width = widthShift;
> +			bestLog2.height = heightShift;
> +		}
> +	}
> +
> +	bdsGrid_.width = best.width >> bestLog2.width;
> +	bdsGrid_.block_width_log2 = bestLog2.width;
> +	bdsGrid_.height = best.height >> bestLog2.height;
> +	bdsGrid_.block_height_log2 = bestLog2.height;
> +
> +	LOG(IPAIPU3, Debug) << "Best grid found is: ("
> +			    << (int)bdsGrid_.width << " << " << (int)bdsGrid_.block_width_log2 << ") x ("
> +			    << (int)bdsGrid_.height << " <<" << (int)bdsGrid_.block_height_log2 << ")";

Somehow there's still an odd spacing around those << ;-)

> +}
> +
>  void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
> -			[[maybe_unused]] const Size &bdsOutputSize)
> +			const Size &bdsOutputSize)
>  {
>  	if (entityControls.empty())
>  		return;
> @@ -92,11 +156,18 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls
>  
>  	minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
>  	maxExposure_ = itExp->second.max().get<int32_t>();
> -	exposure_ = maxExposure_;
> +	exposure_ = minExposure_;
>  
>  	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>  	maxGain_ = itGain->second.max().get<int32_t>();
> -	gain_ = maxGain_;
> +	gain_ = minGain_;
> +
> +	params_ = {};
> +
> +	calculateBdsGrid(bdsOutputSize);
> +
> +	awbAlgo_ = std::make_unique<ipa::IPU3Awb>();
> +	awbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);
>  }
>  
>  void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
> @@ -168,10 +239,10 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>  
>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  {
> -	/* Prepare parameters buffer. */
> -	memset(params, 0, sizeof(*params));
> +	/* Pass a default gamma of 1.0 (default linear correction) */
> +	awbAlgo_->updateWbParameters(params_, 1.0);
>  
> -	/* \todo Fill in parameters buffer. */
> +	*params = params_;
>  
>  	ipa::ipu3::IPU3Action op;
>  	op.op = ipa::ipu3::ActionParamFilled;
> @@ -184,8 +255,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  {
>  	ControlList ctrls(controls::controls);
>  
> -	/* \todo React to statistics and update internal state machine. */
> -	/* \todo Add meta-data information to ctrls. */
> +	awbAlgo_->calculateWBGains(stats);
>  
>  	ipa::ipu3::IPU3Action op;
>  	op.op = ipa::ipu3::ActionMetadataReady;
> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> new file mode 100644
> index 00000000..b4920e4b
> --- /dev/null
> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> @@ -0,0 +1,351 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * ipu3_awb.cpp - AWB control algorithm
> + */
> +#include "ipu3_awb.h"
> +
> +#include <cmath>
> +#include <numeric>
> +#include <unordered_map>
> +
> +#include "libcamera/internal/log.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +LOG_DEFINE_CATEGORY(IPU3Awb)
> +
> +/**
> + * \struct IspStatsRegion
> + * \brief RGB statistics for a given region
> + *
> + * The IspStatsRegion structure is intended to abstract the ISP specific
> + * statistics and use an agnostic algorithm to compute AWB.
> + *
> + * \var IspStatsRegion::counted
> + * \brief Number of pixels used to calculate the sums
> + *
> + * \var IspStatsRegion::notcounted

should this be notCounted ? (or uncounted to be a single word ?)

> + * \brief Remaining number of pixels in the region
> + *
> + * \var IspStatsRegion::rSum
> + * \brief Sum of the red values in the region
> + *
> + * \var IspStatsRegion::gSum
> + * \brief Sum of the green values in the region
> + *
> + * \var IspStatsRegion::bSum
> + * \brief Sum of the blue values in the region
> + */
> +
> +/**
> + * \struct AwbStatus
> + * \brief AWB parameters calculated
> + *
> + * The AwbStatus structure is intended to store the AWB
> + * parameters calculated by the algorithm
> + *
> + * \var AwbStatus::temperatureK
> + * \brief Color temperature calculated
> + *
> + * \var AwbStatus::redGain
> + * \brief Gain calculated for the red channel
> + *
> + * \var AwbStatus::greenGain
> + * \brief Gain calculated for the green channel
> + *
> + * \var AwbStatus::blueGain
> + * \brief Gain calculated for the blue channel
> + */
> +
> +/**
> + * \struct Ipu3AwbCell
> + * \brief Memory layout for each cell in AWB metadata
> + *
> + * The Ipu3AwbCell structure is used to get individual values
> + * such as red average or saturation ratio in a particular cell.
> + *
> + * \var Ipu3AwbCell::greenRedAvg
> + * \brief Green average for red lines in the cell
> + *
> + * \var Ipu3AwbCell::redAvg
> + * \brief Red average in the cell
> + *
> + * \var Ipu3AwbCell::blueAvg
> + * \brief blue average in the cell
> + *
> + * \var Ipu3AwbCell::greenBlueAvg
> + * \brief Green average for blue lines
> + *
> + * \var Ipu3AwbCell::satRatio
> + * \brief Saturation ratio in the cell
> + *
> + * \var Ipu3AwbCell::padding
> + * \brief array of unused bytes for padding
> + */
> +
> +/* Default settings for Bayer noise reduction replicated from the Kernel */
> +static const struct ipu3_uapi_bnr_static_config imguCssBnrDefaults = {
> +	.wb_gains = { 16, 16, 16, 16 },
> +	.wb_gains_thr = { 255, 255, 255, 255 },
> +	.thr_coeffs = { 1700, 0, 31, 31, 0, 16 },
> +	.thr_ctrl_shd = { 26, 26, 26, 26 },
> +	.opt_center{ -648, 0, -366, 0 },
> +	.lut = {
> +		{ 17, 23, 28, 32, 36, 39, 42, 45,
> +		  48, 51, 53, 55, 58, 60, 62, 64,
> +		  66, 68, 70, 72, 73, 75, 77, 78,
> +		  80, 82, 83, 85, 86, 88, 89, 90 } },
> +	.bp_ctrl = { 20, 0, 1, 40, 0, 6, 0, 6, 0 },
> +	.dn_detect_ctrl{ 9, 3, 4, 0, 8, 0, 1, 1, 1, 1, 0 },
> +	.column_size = 1296,
> +	.opt_center_sqr = { 419904, 133956 },
> +};
> +
> +/* Default settings for Auto White Balance replicated from the Kernel*/
> +static const struct ipu3_uapi_awb_config_s imguCssAwbDefaults = {
> +	.rgbs_thr_gr = 8191,
> +	.rgbs_thr_r = 8191,
> +	.rgbs_thr_gb = 8191,
> +	.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
> +	.grid = {
> +		.width = 160,
> +		.height = 36,
> +		.block_width_log2 = 3,
> +		.block_height_log2 = 4,
> +		.height_per_slice = 1, /* Overridden by kernel. */
> +		.x_start = 0,
> +		.y_start = 0,
> +		.x_end = 0,
> +		.y_end = 0,
> +	},
> +};
> +
> +/* Default color correction matrix defined as an identity matrix */
> +static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
> +	8191, 0, 0, 0,
> +	0, 8191, 0, 0,
> +	0, 0, 8191, 0
> +};
> +
> +IPU3Awb::IPU3Awb()
> +	: Algorithm()
> +{
> +	asyncResults_.blueGain = 1.0;
> +	asyncResults_.greenGain = 1.0;
> +	asyncResults_.redGain = 1.0;
> +	asyncResults_.temperatureK = 4500;
> +}
> +
> +IPU3Awb::~IPU3Awb()
> +{
> +}
> +
> +void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)
> +{
> +	params.use.acc_awb = 1;
> +	params.acc_param.awb.config = imguCssAwbDefaults;
> +
> +	awbGrid_ = bdsGrid;
> +	params.acc_param.awb.config.grid = awbGrid_;
> +
> +	params.use.acc_bnr = 1;
> +	params.acc_param.bnr = imguCssBnrDefaults;
> +	/**
> +	 * Optical center is colum (resp row) start - X (resp Y) center.

s/colum/column/

What is resp?


> +	 * For the moment use BDS as a first approximation, but it should
> +	 * be calculated based on Shading (SHD) parameters.
> +	 */
> +	params.acc_param.bnr.column_size = bdsOutputSize.width;
> +	params.acc_param.bnr.opt_center.x_reset = awbGrid_.x_start - (bdsOutputSize.width / 2);
> +	params.acc_param.bnr.opt_center.y_reset = awbGrid_.y_start - (bdsOutputSize.height / 2);
> +	params.acc_param.bnr.opt_center_sqr.x_sqr_reset = params.acc_param.bnr.opt_center.x_reset
> +							* params.acc_param.bnr.opt_center.x_reset;
> +	params.acc_param.bnr.opt_center_sqr.y_sqr_reset = params.acc_param.bnr.opt_center.y_reset
> +							* params.acc_param.bnr.opt_center.y_reset;
> +
> +	params.use.acc_ccm = 1;
> +	params.acc_param.ccm = imguCssCcmDefault;
> +
> +	params.use.acc_gamma = 1;
> +	params.acc_param.gamma.gc_ctrl.enable = 1;
> +
> +	zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
> +}
> +
> +/**
> + * The function estimates the correlated color temperature using
> + * from RGB color space input.
> + * In physics and color science, the Planckian locus or black body locus is
> + * the path or locus that the color of an incandescent black body would take
> + * in a particular chromaticity space as the blackbody temperature changes.
> + *
> + * If a narrow range of color temperatures is considered (those encapsulating
> + * daylight being the most practical case) one can approximate the Planckian
> + * locus in order to calculate the CCT in terms of chromaticity coordinates.
> + *
> + * More detailed information can be found in:
> + * https://en.wikipedia.org/wiki/Color_temperature#Approximation
> + */
> +uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)
> +{
> +	/* Convert the RGB values to CIE tristimulus values (XYZ) */
> +	double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);
> +	double Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);
> +	double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);
> +
> +	/* Calculate the normalized chromaticity values */
> +	double x = X / (X + Y + Z);
> +	double y = Y / (X + Y + Z);
> +
> +	/* Calculate CCT */
> +	double n = (x - 0.3320) / (0.1858 - y);
> +	return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;
> +}
> +
> +/* Generate a RGB vector with the average values for each region */

s/a/an/


> +void IPU3Awb::generateZones(std::vector<RGB> &zones)
> +{
> +	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
> +		RGB zone;
> +		double counted = awbStats_[i].counted;
> +		if (counted >= 16) {
> +			zone.G = awbStats_[i].gSum / counted;
> +			if (zone.G >= 32) {

What is the selection criteria or meaning behind 16 and 32 here?

Perhaps we should define them as documented constexpr's at the top of
the function?


> +				zone.R = awbStats_[i].rSum / counted;
> +				zone.B = awbStats_[i].bSum / counted;
> +				zones.push_back(zone);
> +			}
> +		}
> +	}
> +}
> +
> +/* Translate the IPU3 statistics into the default statistics region array */
> +void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
> +{
> +	uint32_t regionWidth = round(awbGrid_.width / static_cast<double>(kAwbStatsSizeX));
> +	uint32_t regionHeight = round(awbGrid_.height / static_cast<double>(kAwbStatsSizeY));
> +
> +	/**

only /* needed here.

/** is only for doxygen comments


> +	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
> +	 * (awbGrid_.width x awbGrid_.height).
> +	 */
> +	for (unsigned int j = 0; j < kAwbStatsSizeY * regionHeight; j++) {
> +		for (unsigned int i = 0; i < kAwbStatsSizeX * regionWidth; i++) {
> +			uint32_t cellPosition = j * awbGrid_.width + i;
> +			uint32_t cellX = (cellPosition / regionWidth) % kAwbStatsSizeX;
> +			uint32_t cellY = ((cellPosition / awbGrid_.width) / regionHeight) % kAwbStatsSizeY;
> +
> +			uint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
> +			cellPosition *= 8;
> +
> +			/* Cast the initial IPU3 structure to simplify the reading */
> +			Ipu3AwbCell *currentCell = reinterpret_cast<Ipu3AwbCell *>(const_cast<uint8_t *>(&stats->awb_raw_buffer.meta_data[cellPosition]));
> +			if (currentCell->satRatio == 0) {
> +				/* The cell is not saturated, use the current cell */
> +				awbStats_[awbRegionPosition].counted++;
> +				uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
> +				awbStats_[awbRegionPosition].gSum += greenValue / 2;
> +				awbStats_[awbRegionPosition].rSum += currentCell->redAvg;
> +				awbStats_[awbRegionPosition].bSum += currentCell->blueAvg;
> +			}
> +		}
> +	}
> +}
> +
> +void IPU3Awb::clearAwbStats()
> +{
> +	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
> +		awbStats_[i].bSum = 0;
> +		awbStats_[i].rSum = 0;
> +		awbStats_[i].gSum = 0;
> +		awbStats_[i].counted = 0;
> +		awbStats_[i].notcounted = 0;
> +	}
> +}
> +
> +void IPU3Awb::awbGrey()

awbGreyWorld?

> +{
> +	LOG(IPU3Awb, Debug) << "Grey world AWB";
> +	/**

/*

> +	 * Make a separate list of the derivatives for each of red and blue, so
> +	 * that we can sort them to exclude the extreme gains.  We could

s/  / /
(double space to single before We)

> +	 * consider some variations, such as normalising all the zones first, or
> +	 * doing an L2 average etc.
> +	 */
> +	std::vector<RGB> &redDerivative(zones_);

Is the & intentional here?

I'm a little confused as to what that's doing. Creating a local reference?

> +	std::vector<RGB> blueDerivative(redDerivative);

Is this equivalent to blueDerivative(zones_) ? or is there a specific
reason to initiliase it to the redDerivative?

I don't expect there's much difference between either, but initialising
both from zones_ would look more obvious that they are initialised from
the same dataset.


> +	std::sort(redDerivative.begin(), redDerivative.end(),
> +		  [](RGB const &a, RGB const &b) {
> +			  return a.G * b.R < b.G * a.R;
> +		  });
> +	std::sort(blueDerivative.begin(), blueDerivative.end(),
> +		  [](RGB const &a, RGB const &b) {
> +			  return a.G * b.B < b.G * a.B;
> +		  });
> +
> +	/* Average the middle half of the values. */
> +	int discard = redDerivative.size() / 4;
> +	RGB sumRed(0, 0, 0), sumBlue(0, 0, 0);

Hrm ... Perhaps those are better on two lines. ... Not sure, it doesn't
hurt but I haven't seen us use single line multiple constructs much I
don't think.


> +	for (auto ri = redDerivative.begin() + discard,
> +		  bi = blueDerivative.begin() + discard;
> +	     ri != redDerivative.end() - discard; ri++, bi++)
> +		sumRed += *ri, sumBlue += *bi;
> +
> +	double redGain = sumRed.G / (sumRed.R + 1),
> +	       blueGain = sumBlue.G / (sumBlue.B + 1);
> +
> +	/* Color temperature is not relevant in Gray world but still useful to estimate it :-) */

Gray or Grey?


> +	asyncResults_.temperatureK = estimateCCT(sumRed.R, sumRed.G, sumBlue.B);
> +	asyncResults_.redGain = redGain;
> +	asyncResults_.greenGain = 1.0;
> +	asyncResults_.blueGain = blueGain;

Are these results really calculated asynchronously already?
Or is that a future improvement?


> +}
> +
> +void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
> +{
> +	ASSERT(stats->stats_3a_status.awb_en);
> +	zones_.clear();
> +	clearAwbStats();
> +	generateAwbStats(stats);
> +	generateZones(zones_);
> +	LOG(IPU3Awb, Debug) << "Valid zones: " << zones_.size();
> +	if (zones_.size() > 10)
> +		awbGrey();
> +
> +	LOG(IPU3Awb, Debug) << "Gain found for red: " << asyncResults_.redGain
> +			    << " and for blue: " << asyncResults_.blueGain;

Should the debug print be in the context scope of the if (zones_... ) ?

It won't change otherwise will it?


> +}
> +
> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
> +{
> +	/**

/*

> +	 * Green gains should not be touched and considered 1.
> +	 * Default is 16, so do not change it at all.
> +	 * 4096 is the value for a gain of 1.0
> +	 */
> +	params.acc_param.bnr.wb_gains.gr = 16;
> +	params.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;
> +	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;
> +
> +	/* 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 */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> new file mode 100644
> index 00000000..0994d18d
> --- /dev/null
> +++ b/src/ipa/ipu3/ipu3_awb.h
> @@ -0,0 +1,91 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * ipu3_awb.h - IPU3 AWB control algorithm
> + */
> +#ifndef __LIBCAMERA_IPU3_AWB_H__
> +#define __LIBCAMERA_IPU3_AWB_H__
> +
> +#include <vector>
> +
> +#include <linux/intel-ipu3.h>
> +
> +#include <libcamera/geometry.h>
> +
> +#include "libipa/algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +/* Region size for the statistics generation algorithm */
> +static constexpr uint32_t kAwbStatsSizeX = 16;
> +static constexpr uint32_t kAwbStatsSizeY = 12;
> +
> +class IPU3Awb : public Algorithm
> +{
> +public:
> +	IPU3Awb();
> +	~IPU3Awb();
> +
> +	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);
> +
> +	struct Ipu3AwbCell {
> +		unsigned char greenRedAvg;
> +		unsigned char redAvg;
> +		unsigned char blueAvg;
> +		unsigned char greenBlueAvg;
> +		unsigned char satRatio;
> +		unsigned char padding[3];
> +	} __attribute__((packed));
> +
> +	/* \todo Make these three structs available to all the ISPs ? */

Interesting, we might indeed want some colour helpers.
But this can be later.

> +	struct RGB {
> +		RGB(double _R = 0, double _G = 0, double _B = 0)
> +			: R(_R), G(_G), B(_B)
> +		{
> +		}
> +		double R, G, B;
> +		RGB &operator+=(RGB const &other)
> +		{
> +			R += other.R, G += other.G, B += other.B;
> +			return *this;
> +		}
> +	};
> +
> +	struct IspStatsRegion {
> +		unsigned int counted;
> +		unsigned int notcounted;

mentioned above, this might be 'uncounted'

All my comments here seem fairly trivial.

with those:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> +		unsigned long long rSum;
> +		unsigned long long gSum;
> +		unsigned long long bSum;
> +	};
> +
> +	struct AwbStatus {
> +		double temperatureK;
> +		double redGain;
> +		double greenGain;
> +		double blueGain;
> +	};
> +
> +private:
> +	void generateZones(std::vector<RGB> &zones);
> +	void generateAwbStats(const ipu3_uapi_stats_3a *stats);
> +	void clearAwbStats();
> +	void awbGrey();
> +	uint32_t estimateCCT(double red, double green, double blue);
> +
> +	struct ipu3_uapi_grid_config awbGrid_;
> +
> +	std::vector<RGB> zones_;
> +	IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
> +	AwbStatus asyncResults_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera*/
> +#endif /* __LIBCAMERA_IPU3_AWB_H__ */
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index a241f617..1040698e 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -2,8 +2,13 @@
>  
>  ipa_name = 'ipa_ipu3'
>  
> +ipu3_ipa_sources = files([
> +    'ipu3.cpp',
> +    'ipu3_awb.cpp',
> +])
> +
>  mod = shared_module(ipa_name,
> -                    ['ipu3.cpp', libcamera_generated_ipa_headers],
> +                    [ipu3_ipa_sources, libcamera_generated_ipa_headers],
>                      name_prefix : '',
>                      include_directories : [ipa_includes, libipa_includes],
>                      dependencies : libcamera_dep,
>

Hi Kieran,

On 19/04/2021 13:01, Kieran Bingham wrote:
> Hi JM,
> 
> On 16/04/2021 08:49, Jean-Michel Hautbois wrote:
>> The IPA will locally modify the parameters before they are passed down
>> to the ImgU. Use a local parameter object to give a reference to those
>> algorithms.
>>
>> Inherit from the Algorithm class to implement basic AWB functions.
>>
>> The configure() call will set exposure and gain to their minimum value,
>> so while AGC is not there, the frames will be dark.
>>
>> Once AWB is done, a color temperature is estimated and a default CCM matrix
>> will be used (yet to be tuned).
>> Implement a basic "grey-world" AWB algorithm just for demonstration purpose.
>>
>> The BDS output size is passed by the pipeline handler to the IPA.
>> The best grid is then calculated to maximize the number of pixels taken
>> into account in each cells.
>>
>> As commented in the source code, it can be improved, as it has (at least)
>> one limitation: if a cell is big (say 128 pixels wide) and indicated as
>> saturated, it won't be taken into account at all.
>> Maybe is it possible to have a smaller one, at the cost of a few pixels
>> to lose, in which case we can center the grid using the x_start and
>> y_start parameters.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> ---
>>  src/ipa/ipu3/ipu3.cpp     |  86 +++++++++-
>>  src/ipa/ipu3/ipu3_awb.cpp | 351 ++++++++++++++++++++++++++++++++++++++
>>  src/ipa/ipu3/ipu3_awb.h   |  91 ++++++++++
>>  src/ipa/ipu3/meson.build  |   7 +-
>>  4 files changed, 526 insertions(+), 9 deletions(-)
>>  create mode 100644 src/ipa/ipu3/ipu3_awb.cpp
>>  create mode 100644 src/ipa/ipu3/ipu3_awb.h
>>
>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>> index 34a907f2..ab052a8a 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -21,6 +21,11 @@
>>  #include "libcamera/internal/buffer.h"
>>  #include "libcamera/internal/log.h"
>>  
>> +#include "ipu3_awb.h"
>> +
>> +static constexpr uint32_t kMaxCellWidthPerSet = 160;
>> +static constexpr uint32_t kMaxCellHeightPerSet = 80;
>> +
>>  namespace libcamera {
>>  
>>  LOG_DEFINE_CATEGORY(IPAIPU3)
>> @@ -49,6 +54,7 @@ private:
>>  			     const ipu3_uapi_stats_3a *stats);
>>  
>>  	void setControls(unsigned int frame);
>> +	void calculateBdsGrid(const Size &bdsOutputSize);
>>  
>>  	std::map<unsigned int, MappedFrameBuffer> buffers_;
>>  
>> @@ -61,6 +67,14 @@ private:
>>  	uint32_t gain_;
>>  	uint32_t minGain_;
>>  	uint32_t maxGain_;
>> +
>> +	/* Interface to the AWB algorithm */
>> +	std::unique_ptr<ipa::IPU3Awb> awbAlgo_;
>> +
>> +	/* Local parameter storage */
>> +	struct ipu3_uapi_params params_;
>> +
>> +	struct ipu3_uapi_grid_config bdsGrid_;
>>  };
>>  
>>  int IPAIPU3::start()
>> @@ -70,8 +84,58 @@ int IPAIPU3::start()
>>  	return 0;
>>  }
>>  
>> +/**
>> + * This method calculates a grid for the AWB algorithm in the IPU3 firmware.
>> + * Its input is the BDS output size calculated in the ImgU.
>> + * It is limited for now to the simplest method: find the lesser error
>> + * with the width/height and respective log2 width/height of the cells.
>> + *
>> + * \todo The frame is divided into cells which can be 8x8 => 128x128.
>> + * As a smaller cell improves the algorithm precision, adapting the
>> + * x_start and y_start parameters of the grid would provoke a loss of
>> + * some pixels but would also result in more accurate algorithms.
>> + */
>> +void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
>> +{
>> +	uint32_t minError = std::numeric_limits<uint32_t>::max();
>> +	Size best;
>> +	Size bestLog2;
>> +	bdsGrid_ = {};
>> +
>> +	for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
>> +		uint32_t width = std::min(kMaxCellWidthPerSet,
>> +					  bdsOutputSize.width >> widthShift);
>> +		width = width << widthShift;
>> +		for (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {
>> +			int32_t height = std::min(kMaxCellHeightPerSet,
>> +						  bdsOutputSize.height >> heightShift);
>> +			height = height << heightShift;
>> +			uint32_t error  = std::abs(static_cast<int>(width - bdsOutputSize.width))
>> +							+ std::abs(static_cast<int>(height - bdsOutputSize.height));
>> +
>> +			if (error > minError)
>> +				continue;
>> +
>> +			minError = error;
>> +			best.width = width;
>> +			best.height = height;
>> +			bestLog2.width = widthShift;
>> +			bestLog2.height = heightShift;
>> +		}
>> +	}
>> +
>> +	bdsGrid_.width = best.width >> bestLog2.width;
>> +	bdsGrid_.block_width_log2 = bestLog2.width;
>> +	bdsGrid_.height = best.height >> bestLog2.height;
>> +	bdsGrid_.block_height_log2 = bestLog2.height;
>> +
>> +	LOG(IPAIPU3, Debug) << "Best grid found is: ("
>> +			    << (int)bdsGrid_.width << " << " << (int)bdsGrid_.block_width_log2 << ") x ("
>> +			    << (int)bdsGrid_.height << " <<" << (int)bdsGrid_.block_height_log2 << ")";
> 
> Somehow there's still an odd spacing around those << ;-)
> 
>> +}
>> +
>>  void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls,
>> -			[[maybe_unused]] const Size &bdsOutputSize)
>> +			const Size &bdsOutputSize)
>>  {
>>  	if (entityControls.empty())
>>  		return;
>> @@ -92,11 +156,18 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls
>>  
>>  	minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
>>  	maxExposure_ = itExp->second.max().get<int32_t>();
>> -	exposure_ = maxExposure_;
>> +	exposure_ = minExposure_;
>>  
>>  	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>>  	maxGain_ = itGain->second.max().get<int32_t>();
>> -	gain_ = maxGain_;
>> +	gain_ = minGain_;
>> +
>> +	params_ = {};
>> +
>> +	calculateBdsGrid(bdsOutputSize);
>> +
>> +	awbAlgo_ = std::make_unique<ipa::IPU3Awb>();
>> +	awbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);
>>  }
>>  
>>  void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
>> @@ -168,10 +239,10 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>>  
>>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>>  {
>> -	/* Prepare parameters buffer. */
>> -	memset(params, 0, sizeof(*params));
>> +	/* Pass a default gamma of 1.0 (default linear correction) */
>> +	awbAlgo_->updateWbParameters(params_, 1.0);
>>  
>> -	/* \todo Fill in parameters buffer. */
>> +	*params = params_;
>>  
>>  	ipa::ipu3::IPU3Action op;
>>  	op.op = ipa::ipu3::ActionParamFilled;
>> @@ -184,8 +255,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>  {
>>  	ControlList ctrls(controls::controls);
>>  
>> -	/* \todo React to statistics and update internal state machine. */
>> -	/* \todo Add meta-data information to ctrls. */
>> +	awbAlgo_->calculateWBGains(stats);
>>  
>>  	ipa::ipu3::IPU3Action op;
>>  	op.op = ipa::ipu3::ActionMetadataReady;
>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
>> new file mode 100644
>> index 00000000..b4920e4b
>> --- /dev/null
>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
>> @@ -0,0 +1,351 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Ideas On Board
>> + *
>> + * ipu3_awb.cpp - AWB control algorithm
>> + */
>> +#include "ipu3_awb.h"
>> +
>> +#include <cmath>
>> +#include <numeric>
>> +#include <unordered_map>
>> +
>> +#include "libcamera/internal/log.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa {
>> +
>> +LOG_DEFINE_CATEGORY(IPU3Awb)
>> +
>> +/**
>> + * \struct IspStatsRegion
>> + * \brief RGB statistics for a given region
>> + *
>> + * The IspStatsRegion structure is intended to abstract the ISP specific
>> + * statistics and use an agnostic algorithm to compute AWB.
>> + *
>> + * \var IspStatsRegion::counted
>> + * \brief Number of pixels used to calculate the sums
>> + *
>> + * \var IspStatsRegion::notcounted
> 
> should this be notCounted ? (or uncounted to be a single word ?)

OK, I voted for uncounted ;-).

>> + * \brief Remaining number of pixels in the region
>> + *
>> + * \var IspStatsRegion::rSum
>> + * \brief Sum of the red values in the region
>> + *
>> + * \var IspStatsRegion::gSum
>> + * \brief Sum of the green values in the region
>> + *
>> + * \var IspStatsRegion::bSum
>> + * \brief Sum of the blue values in the region
>> + */
>> +
>> +/**
>> + * \struct AwbStatus
>> + * \brief AWB parameters calculated
>> + *
>> + * The AwbStatus structure is intended to store the AWB
>> + * parameters calculated by the algorithm
>> + *
>> + * \var AwbStatus::temperatureK
>> + * \brief Color temperature calculated
>> + *
>> + * \var AwbStatus::redGain
>> + * \brief Gain calculated for the red channel
>> + *
>> + * \var AwbStatus::greenGain
>> + * \brief Gain calculated for the green channel
>> + *
>> + * \var AwbStatus::blueGain
>> + * \brief Gain calculated for the blue channel
>> + */
>> +
>> +/**
>> + * \struct Ipu3AwbCell
>> + * \brief Memory layout for each cell in AWB metadata
>> + *
>> + * The Ipu3AwbCell structure is used to get individual values
>> + * such as red average or saturation ratio in a particular cell.
>> + *
>> + * \var Ipu3AwbCell::greenRedAvg
>> + * \brief Green average for red lines in the cell
>> + *
>> + * \var Ipu3AwbCell::redAvg
>> + * \brief Red average in the cell
>> + *
>> + * \var Ipu3AwbCell::blueAvg
>> + * \brief blue average in the cell
>> + *
>> + * \var Ipu3AwbCell::greenBlueAvg
>> + * \brief Green average for blue lines
>> + *
>> + * \var Ipu3AwbCell::satRatio
>> + * \brief Saturation ratio in the cell
>> + *
>> + * \var Ipu3AwbCell::padding
>> + * \brief array of unused bytes for padding
>> + */
>> +
>> +/* Default settings for Bayer noise reduction replicated from the Kernel */
>> +static const struct ipu3_uapi_bnr_static_config imguCssBnrDefaults = {
>> +	.wb_gains = { 16, 16, 16, 16 },
>> +	.wb_gains_thr = { 255, 255, 255, 255 },
>> +	.thr_coeffs = { 1700, 0, 31, 31, 0, 16 },
>> +	.thr_ctrl_shd = { 26, 26, 26, 26 },
>> +	.opt_center{ -648, 0, -366, 0 },
>> +	.lut = {
>> +		{ 17, 23, 28, 32, 36, 39, 42, 45,
>> +		  48, 51, 53, 55, 58, 60, 62, 64,
>> +		  66, 68, 70, 72, 73, 75, 77, 78,
>> +		  80, 82, 83, 85, 86, 88, 89, 90 } },
>> +	.bp_ctrl = { 20, 0, 1, 40, 0, 6, 0, 6, 0 },
>> +	.dn_detect_ctrl{ 9, 3, 4, 0, 8, 0, 1, 1, 1, 1, 0 },
>> +	.column_size = 1296,
>> +	.opt_center_sqr = { 419904, 133956 },
>> +};
>> +
>> +/* Default settings for Auto White Balance replicated from the Kernel*/
>> +static const struct ipu3_uapi_awb_config_s imguCssAwbDefaults = {
>> +	.rgbs_thr_gr = 8191,
>> +	.rgbs_thr_r = 8191,
>> +	.rgbs_thr_gb = 8191,
>> +	.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
>> +	.grid = {
>> +		.width = 160,
>> +		.height = 36,
>> +		.block_width_log2 = 3,
>> +		.block_height_log2 = 4,
>> +		.height_per_slice = 1, /* Overridden by kernel. */
>> +		.x_start = 0,
>> +		.y_start = 0,
>> +		.x_end = 0,
>> +		.y_end = 0,
>> +	},
>> +};
>> +
>> +/* Default color correction matrix defined as an identity matrix */
>> +static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
>> +	8191, 0, 0, 0,
>> +	0, 8191, 0, 0,
>> +	0, 0, 8191, 0
>> +};
>> +
>> +IPU3Awb::IPU3Awb()
>> +	: Algorithm()
>> +{
>> +	asyncResults_.blueGain = 1.0;
>> +	asyncResults_.greenGain = 1.0;
>> +	asyncResults_.redGain = 1.0;
>> +	asyncResults_.temperatureK = 4500;
>> +}
>> +
>> +IPU3Awb::~IPU3Awb()
>> +{
>> +}
>> +
>> +void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)
>> +{
>> +	params.use.acc_awb = 1;
>> +	params.acc_param.awb.config = imguCssAwbDefaults;
>> +
>> +	awbGrid_ = bdsGrid;
>> +	params.acc_param.awb.config.grid = awbGrid_;
>> +
>> +	params.use.acc_bnr = 1;
>> +	params.acc_param.bnr = imguCssBnrDefaults;
>> +	/**
>> +	 * Optical center is colum (resp row) start - X (resp Y) center.
> 
> s/colum/column/
> 
> What is resp?

respectively ?

> 
>> +	 * For the moment use BDS as a first approximation, but it should
>> +	 * be calculated based on Shading (SHD) parameters.
>> +	 */
>> +	params.acc_param.bnr.column_size = bdsOutputSize.width;
>> +	params.acc_param.bnr.opt_center.x_reset = awbGrid_.x_start - (bdsOutputSize.width / 2);
>> +	params.acc_param.bnr.opt_center.y_reset = awbGrid_.y_start - (bdsOutputSize.height / 2);
>> +	params.acc_param.bnr.opt_center_sqr.x_sqr_reset = params.acc_param.bnr.opt_center.x_reset
>> +							* params.acc_param.bnr.opt_center.x_reset;
>> +	params.acc_param.bnr.opt_center_sqr.y_sqr_reset = params.acc_param.bnr.opt_center.y_reset
>> +							* params.acc_param.bnr.opt_center.y_reset;
>> +
>> +	params.use.acc_ccm = 1;
>> +	params.acc_param.ccm = imguCssCcmDefault;
>> +
>> +	params.use.acc_gamma = 1;
>> +	params.acc_param.gamma.gc_ctrl.enable = 1;
>> +
>> +	zones_.reserve(kAwbStatsSizeX * kAwbStatsSizeY);
>> +}
>> +
>> +/**
>> + * The function estimates the correlated color temperature using
>> + * from RGB color space input.
>> + * In physics and color science, the Planckian locus or black body locus is
>> + * the path or locus that the color of an incandescent black body would take
>> + * in a particular chromaticity space as the blackbody temperature changes.
>> + *
>> + * If a narrow range of color temperatures is considered (those encapsulating
>> + * daylight being the most practical case) one can approximate the Planckian
>> + * locus in order to calculate the CCT in terms of chromaticity coordinates.
>> + *
>> + * More detailed information can be found in:
>> + * https://en.wikipedia.org/wiki/Color_temperature#Approximation
>> + */
>> +uint32_t IPU3Awb::estimateCCT(double red, double green, double blue)
>> +{
>> +	/* Convert the RGB values to CIE tristimulus values (XYZ) */
>> +	double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);
>> +	double Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);
>> +	double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);
>> +
>> +	/* Calculate the normalized chromaticity values */
>> +	double x = X / (X + Y + Z);
>> +	double y = Y / (X + Y + Z);
>> +
>> +	/* Calculate CCT */
>> +	double n = (x - 0.3320) / (0.1858 - y);
>> +	return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;
>> +}
>> +
>> +/* Generate a RGB vector with the average values for each region */
> 
> s/a/an/
> 
> 
>> +void IPU3Awb::generateZones(std::vector<RGB> &zones)
>> +{
>> +	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
>> +		RGB zone;
>> +		double counted = awbStats_[i].counted;
>> +		if (counted >= 16) {
>> +			zone.G = awbStats_[i].gSum / counted;
>> +			if (zone.G >= 32) {
> 
> What is the selection criteria or meaning behind 16 and 32 here?
> 
> Perhaps we should define them as documented constexpr's at the top of
> the function?

Indeed the values are taken from RPi code, but it should be calculated
based on the original grid size.

> 
>> +				zone.R = awbStats_[i].rSum / counted;
>> +				zone.B = awbStats_[i].bSum / counted;
>> +				zones.push_back(zone);
>> +			}
>> +		}
>> +	}
>> +}
>> +
>> +/* Translate the IPU3 statistics into the default statistics region array */
>> +void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>> +{
>> +	uint32_t regionWidth = round(awbGrid_.width / static_cast<double>(kAwbStatsSizeX));
>> +	uint32_t regionHeight = round(awbGrid_.height / static_cast<double>(kAwbStatsSizeY));
>> +
>> +	/**
> 
> only /* needed here.
> 
> /** is only for doxygen comments
> 
> 
>> +	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
>> +	 * (awbGrid_.width x awbGrid_.height).
>> +	 */
>> +	for (unsigned int j = 0; j < kAwbStatsSizeY * regionHeight; j++) {
>> +		for (unsigned int i = 0; i < kAwbStatsSizeX * regionWidth; i++) {
>> +			uint32_t cellPosition = j * awbGrid_.width + i;
>> +			uint32_t cellX = (cellPosition / regionWidth) % kAwbStatsSizeX;
>> +			uint32_t cellY = ((cellPosition / awbGrid_.width) / regionHeight) % kAwbStatsSizeY;
>> +
>> +			uint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
>> +			cellPosition *= 8;
>> +
>> +			/* Cast the initial IPU3 structure to simplify the reading */
>> +			Ipu3AwbCell *currentCell = reinterpret_cast<Ipu3AwbCell *>(const_cast<uint8_t *>(&stats->awb_raw_buffer.meta_data[cellPosition]));
>> +			if (currentCell->satRatio == 0) {
>> +				/* The cell is not saturated, use the current cell */
>> +				awbStats_[awbRegionPosition].counted++;
>> +				uint32_t greenValue = currentCell->greenRedAvg + currentCell->greenBlueAvg;
>> +				awbStats_[awbRegionPosition].gSum += greenValue / 2;
>> +				awbStats_[awbRegionPosition].rSum += currentCell->redAvg;
>> +				awbStats_[awbRegionPosition].bSum += currentCell->blueAvg;
>> +			}
>> +		}
>> +	}
>> +}
>> +
>> +void IPU3Awb::clearAwbStats()
>> +{
>> +	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
>> +		awbStats_[i].bSum = 0;
>> +		awbStats_[i].rSum = 0;
>> +		awbStats_[i].gSum = 0;
>> +		awbStats_[i].counted = 0;
>> +		awbStats_[i].notcounted = 0;
>> +	}
>> +}
>> +
>> +void IPU3Awb::awbGrey()
> 
> awbGreyWorld?
> 
>> +{
>> +	LOG(IPU3Awb, Debug) << "Grey world AWB";
>> +	/**
> 
> /*
> 
>> +	 * Make a separate list of the derivatives for each of red and blue, so
>> +	 * that we can sort them to exclude the extreme gains.  We could
> 
> s/  / /
> (double space to single before We)
> 
>> +	 * consider some variations, such as normalising all the zones first, or
>> +	 * doing an L2 average etc.
>> +	 */
>> +	std::vector<RGB> &redDerivative(zones_);
> 
> Is the & intentional here?
> 
> I'm a little confused as to what that's doing. Creating a local reference?
> 
>> +	std::vector<RGB> blueDerivative(redDerivative);
> 
> Is this equivalent to blueDerivative(zones_) ? or is there a specific
> reason to initiliase it to the redDerivative?
> 
> I don't expect there's much difference between either, but initialising
> both from zones_ would look more obvious that they are initialised from
> the same dataset.

Again, this is from RPi and I think it is optimizing an allocation (one
new/delete less) per call at lease.
You have a reference for the red values, and blue is using the reference
too.
I don't think there is a difference in the implementation by doing:
std::vector<RGB> redDerivative(zones_);
std::vector<RGB> blueDerivative(zones_);

But it may be lighter.

> 
>> +	std::sort(redDerivative.begin(), redDerivative.end(),
>> +		  [](RGB const &a, RGB const &b) {
>> +			  return a.G * b.R < b.G * a.R;
>> +		  });
>> +	std::sort(blueDerivative.begin(), blueDerivative.end(),
>> +		  [](RGB const &a, RGB const &b) {
>> +			  return a.G * b.B < b.G * a.B;
>> +		  });
>> +
>> +	/* Average the middle half of the values. */
>> +	int discard = redDerivative.size() / 4;
>> +	RGB sumRed(0, 0, 0), sumBlue(0, 0, 0);
> 
> Hrm ... Perhaps those are better on two lines. ... Not sure, it doesn't
> hurt but I haven't seen us use single line multiple constructs much I
> don't think.
> 
> 
>> +	for (auto ri = redDerivative.begin() + discard,
>> +		  bi = blueDerivative.begin() + discard;
>> +	     ri != redDerivative.end() - discard; ri++, bi++)
>> +		sumRed += *ri, sumBlue += *bi;
>> +
>> +	double redGain = sumRed.G / (sumRed.R + 1),
>> +	       blueGain = sumBlue.G / (sumBlue.B + 1);
>> +
>> +	/* Color temperature is not relevant in Gray world but still useful to estimate it :-) */
> 
> Gray or Grey?
> 
> 
>> +	asyncResults_.temperatureK = estimateCCT(sumRed.R, sumRed.G, sumBlue.B);
>> +	asyncResults_.redGain = redGain;
>> +	asyncResults_.greenGain = 1.0;
>> +	asyncResults_.blueGain = blueGain;
> 
> Are these results really calculated asynchronously already?
> Or is that a future improvement?

Good catch, they are calculated in the same thread, so not really async
no...

> 
>> +}
>> +
>> +void IPU3Awb::calculateWBGains(const ipu3_uapi_stats_3a *stats)
>> +{
>> +	ASSERT(stats->stats_3a_status.awb_en);
>> +	zones_.clear();
>> +	clearAwbStats();
>> +	generateAwbStats(stats);
>> +	generateZones(zones_);
>> +	LOG(IPU3Awb, Debug) << "Valid zones: " << zones_.size();
>> +	if (zones_.size() > 10)
>> +		awbGrey();
>> +
>> +	LOG(IPU3Awb, Debug) << "Gain found for red: " << asyncResults_.redGain
>> +			    << " and for blue: " << asyncResults_.blueGain;
> 
> Should the debug print be in the context scope of the if (zones_... ) ?
> 
> It won't change otherwise will it?
> 
> 
>> +}
>> +
>> +void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
>> +{
>> +	/**
> 
> /*
> 
>> +	 * Green gains should not be touched and considered 1.
>> +	 * Default is 16, so do not change it at all.
>> +	 * 4096 is the value for a gain of 1.0
>> +	 */
>> +	params.acc_param.bnr.wb_gains.gr = 16;
>> +	params.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;
>> +	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;
>> +
>> +	/* 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 */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
>> new file mode 100644
>> index 00000000..0994d18d
>> --- /dev/null
>> +++ b/src/ipa/ipu3/ipu3_awb.h
>> @@ -0,0 +1,91 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Ideas On Board
>> + *
>> + * ipu3_awb.h - IPU3 AWB control algorithm
>> + */
>> +#ifndef __LIBCAMERA_IPU3_AWB_H__
>> +#define __LIBCAMERA_IPU3_AWB_H__
>> +
>> +#include <vector>
>> +
>> +#include <linux/intel-ipu3.h>
>> +
>> +#include <libcamera/geometry.h>
>> +
>> +#include "libipa/algorithm.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa {
>> +
>> +/* Region size for the statistics generation algorithm */
>> +static constexpr uint32_t kAwbStatsSizeX = 16;
>> +static constexpr uint32_t kAwbStatsSizeY = 12;
>> +
>> +class IPU3Awb : public Algorithm
>> +{
>> +public:
>> +	IPU3Awb();
>> +	~IPU3Awb();
>> +
>> +	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);
>> +
>> +	struct Ipu3AwbCell {
>> +		unsigned char greenRedAvg;
>> +		unsigned char redAvg;
>> +		unsigned char blueAvg;
>> +		unsigned char greenBlueAvg;
>> +		unsigned char satRatio;
>> +		unsigned char padding[3];
>> +	} __attribute__((packed));
>> +
>> +	/* \todo Make these three structs available to all the ISPs ? */
> 
> Interesting, we might indeed want some colour helpers.
> But this can be later.
> 
>> +	struct RGB {
>> +		RGB(double _R = 0, double _G = 0, double _B = 0)
>> +			: R(_R), G(_G), B(_B)
>> +		{
>> +		}
>> +		double R, G, B;
>> +		RGB &operator+=(RGB const &other)
>> +		{
>> +			R += other.R, G += other.G, B += other.B;
>> +			return *this;
>> +		}
>> +	};
>> +
>> +	struct IspStatsRegion {
>> +		unsigned int counted;
>> +		unsigned int notcounted;
> 
> mentioned above, this might be 'uncounted'
> 
> All my comments here seem fairly trivial.
> 
> with those:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Thanks !

> 
>> +		unsigned long long rSum;
>> +		unsigned long long gSum;
>> +		unsigned long long bSum;
>> +	};
>> +
>> +	struct AwbStatus {
>> +		double temperatureK;
>> +		double redGain;
>> +		double greenGain;
>> +		double blueGain;
>> +	};
>> +
>> +private:
>> +	void generateZones(std::vector<RGB> &zones);
>> +	void generateAwbStats(const ipu3_uapi_stats_3a *stats);
>> +	void clearAwbStats();
>> +	void awbGrey();
>> +	uint32_t estimateCCT(double red, double green, double blue);
>> +
>> +	struct ipu3_uapi_grid_config awbGrid_;
>> +
>> +	std::vector<RGB> zones_;
>> +	IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
>> +	AwbStatus asyncResults_;
>> +};
>> +
>> +} /* namespace ipa */
>> +
>> +} /* namespace libcamera*/
>> +#endif /* __LIBCAMERA_IPU3_AWB_H__ */
>> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
>> index a241f617..1040698e 100644
>> --- a/src/ipa/ipu3/meson.build
>> +++ b/src/ipa/ipu3/meson.build
>> @@ -2,8 +2,13 @@
>>  
>>  ipa_name = 'ipa_ipu3'
>>  
>> +ipu3_ipa_sources = files([
>> +    'ipu3.cpp',
>> +    'ipu3_awb.cpp',
>> +])
>> +
>>  mod = shared_module(ipa_name,
>> -                    ['ipu3.cpp', libcamera_generated_ipa_headers],
>> +                    [ipu3_ipa_sources, libcamera_generated_ipa_headers],
>>                      name_prefix : '',
>>                      include_directories : [ipa_includes, libipa_includes],
>>                      dependencies : libcamera_dep,
>>
>