[libcamera-devel,RFC,2/2] WIP: ipa: ipu3: Add support for IPU3 AWB algorithm

11347 diff mbox series

Inherit from the AWBAlgorithm and AGCAlgorithm classes to implement
basic functions.
While exposure is not locked, AWB is not calculated and corrected.
Once AWB is done, a color temperature is estimated and default CCM matrices
are used.
Implement a basic "grey-world" AWB algorithm just for demonstration purpose.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp                |  31 ++++-
 src/ipa/ipu3/ipu3_agc.cpp            | 195 +++++++++++++++++++++++++++
 src/ipa/ipu3/ipu3_agc.h              |  96 +++++++++++++
 src/ipa/ipu3/ipu3_awb.cpp            | 182 +++++++++++++++++++++++++
 src/ipa/ipu3/ipu3_awb.h              | 130 ++++++++++++++++++
 src/ipa/ipu3/meson.build             |   8 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp |   1 +
 7 files changed, 638 insertions(+), 5 deletions(-)
 create mode 100644 src/ipa/ipu3/ipu3_agc.cpp
 create mode 100644 src/ipa/ipu3/ipu3_agc.h
 create mode 100644 src/ipa/ipu3/ipu3_awb.cpp
 create mode 100644 src/ipa/ipu3/ipu3_awb.h

Hi Jean-Michel,

Thank you for the patch.

On Fri, Feb 19, 2021 at 06:22:24PM +0100, Jean-Michel Hautbois wrote:
> Inherit from the AWBAlgorithm and AGCAlgorithm classes to implement
> basic functions.
> While exposure is not locked, AWB is not calculated and corrected.
> Once AWB is done, a color temperature is estimated and default CCM matrices
> are used.
> Implement a basic "grey-world" AWB algorithm just for demonstration purpose.

Nice to see an initial implementation ! I'll provide a handful of review
comments below already. Some are related to coding style or similar
issues and should be addressed in v2. Others are related to the IPA
architecture, and could be fixed on top of this series to avoid delaying
it unnecessarily. They should in that case be captured in a \todo
comment.

As for 1/2, I won't repeat the same comments all over the patch, so
please apply them wherever they're applicable.

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp                |  31 ++++-
>  src/ipa/ipu3/ipu3_agc.cpp            | 195 +++++++++++++++++++++++++++
>  src/ipa/ipu3/ipu3_agc.h              |  96 +++++++++++++
>  src/ipa/ipu3/ipu3_awb.cpp            | 182 +++++++++++++++++++++++++
>  src/ipa/ipu3/ipu3_awb.h              | 130 ++++++++++++++++++
>  src/ipa/ipu3/meson.build             |   8 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp |   1 +
>  7 files changed, 638 insertions(+), 5 deletions(-)
>  create mode 100644 src/ipa/ipu3/ipu3_agc.cpp
>  create mode 100644 src/ipa/ipu3/ipu3_agc.h
>  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 b63e58be..c3859f39 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -13,6 +13,7 @@
>  
>  #include <libcamera/buffer.h>
>  #include <libcamera/control_ids.h>
> +#include <libcamera/ipa/ipa_controller.h>
>  #include <libcamera/ipa/ipa_interface.h>
>  #include <libcamera/ipa/ipa_module_info.h>
>  #include <libcamera/ipa/ipu3_ipa_interface.h>
> @@ -21,6 +22,9 @@
>  #include "libcamera/internal/buffer.h"
>  #include "libcamera/internal/log.h"
>  
> +#include "ipu3_agc.h"
> +#include "ipu3_awb.h"
> +
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPAIPU3)
> @@ -28,6 +32,9 @@ LOG_DEFINE_CATEGORY(IPAIPU3)
>  class IPAIPU3 : public ipa::ipu3::IPAIPU3Interface
>  {
>  public:
> +	IPAIPU3()
> +		: controller_() {}

		: controller_()
	{
	}

I may not repeat all the comments from 1/2, but they're generally
applicable to this patch as well.

> +
>  	int init([[maybe_unused]] const IPASettings &settings) override
>  	{
>  		return 0;
> @@ -60,6 +67,11 @@ private:
>  	uint32_t gain_;
>  	uint32_t minGain_;
>  	uint32_t maxGain_;
> +
> +	IPAController controller_;
> +	IPU3Awb *awbAlgo_;
> +	IPU3Agc *agcAlgo_;
> +	ipu3_uapi_params params_;
>  };
>  
>  void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls)
> @@ -83,11 +95,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_ = 123;

A peculiar value :-)

>  
>  	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>  	maxGain_ = itGain->second.max().get<int32_t>();
> -	gain_ = maxGain_;
> +	gain_ = 1;
> +
> +	awbAlgo_ = new IPU3Awb(&controller_);
> +	awbAlgo_->Initialise(params_);
> +	agcAlgo_ = new IPU3Agc(&controller_);
> +
> +	/*\todo not used yet... */
> +	controller_.Initialise();
>  
>  	setControls(0);
>  }
> @@ -162,10 +181,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));
> +	awbAlgo_->updateBNR(params_);
> +	memcpy(params, &params_, sizeof(*params));

	*params = params_;

>  
>  	/* \todo Fill in parameters buffer. */
> -
>  	ipa::ipu3::IPU3Action op;
>  	op.op = ipa::ipu3::ActionParamFilled;
>  
> @@ -179,6 +198,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  
>  	/* \todo React to statistics and update internal state machine. */
>  	/* \todo Add meta-data information to ctrls. */
> +	agcAlgo_->Process(stats, exposure_, gain_);
> +	if (agcAlgo_->Converged())
> +		awbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400), stats);
> +	setControls(frame);
>  
>  	ipa::ipu3::IPU3Action op;
>  	op.op = ipa::ipu3::ActionMetadataReady;
> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> new file mode 100644
> index 00000000..db591e33
> --- /dev/null
> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> @@ -0,0 +1,195 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * agc.cpp - AGC/AEC control algorithm
> + */
> +#include <iostream>
> +#include <numeric>
> +#include <unordered_map>
> +
> +#include "libcamera/internal/log.h"
> +
> +#include "ipu3_agc.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(IPU3Agc)
> +
> +#define NAME "ipu3.agc"
> +
> +IPU3Agc::IPU3Agc(IPAController *controller)
> +	: AgcAlgorithm(controller), frameCount_(0),
> +	  ev_(1.0), flicker_period_(0.0),
> +	  max_shutter_(0), fixed_shutter_(0),
> +	  fixed_analogue_gain_(0.0), algoConverged_(false)
> +{
> +}
> +
> +char const *IPU3Agc::Name() const
> +{
> +	return NAME;
> +}
> +
> +unsigned int IPU3Agc::GetConvergenceFrames() const
> +{
> +	return config_.convergence_frames;
> +}
> +
> +void IPU3Agc::SetEv(double ev)
> +{
> +	ev_ = ev;
> +}
> +
> +void IPU3Agc::SetFlickerPeriod(double flicker_period)
> +{
> +	flicker_period_ = flicker_period;
> +}
> +
> +void IPU3Agc::SetMaxShutter(double max_shutter)
> +{
> +	max_shutter_ = max_shutter;
> +}
> +
> +void IPU3Agc::SetFixedShutter(double fixed_shutter)
> +{
> +	fixed_shutter_ = fixed_shutter;
> +}
> +
> +void IPU3Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
> +{
> +	fixed_analogue_gain_ = fixed_analogue_gain;
> +}
> +
> +void IPU3Agc::SetMeteringMode(std::string const &metering_mode_name)
> +{
> +	metering_mode_name_ = metering_mode_name;
> +}
> +
> +void IPU3Agc::SetExposureMode(std::string const &exposure_mode_name)
> +{
> +	exposure_mode_name_ = exposure_mode_name;
> +}
> +
> +void IPU3Agc::Prepare() {}
> +
> +void IPU3Agc::Process() {}
> +
> +/* \todo This function is taken from numerical recipes and calculates all moments
> + * It needs to be rewritten properly and maybe in a "math" class ? */

/*
 * \todo This function is taken from numerical recipes and calculates all
 * moments. It needs to be rewritten properly and maybe in a "math" class ?
 */

> +void IPU3Agc::moments(std::unordered_map<uint32_t, uint32_t> &data, int n)
> +{
> +	int j;
> +	double ep = 0.0, s, p;
> +	double ave, adev, sdev;
> +	double var, skew, curt;
> +
> +	s = 0.0;
> +	for (j = 1; j <= n; j++)
> +		s += data[j];
> +
> +	ave = s / n;
> +	adev = var = skew = curt = 0.0;
> +
> +	for (j = 1; j <= n; j++) {
> +		adev += s = data[j] - (ave);
> +		ep += s;
> +		var += (p = s * s);
> +		skew += (p *= s);
> +		curt += (p *= s);
> +	}
> +
> +	adev /= n;
> +	var = (var - ep * ep / n) / (n - 1);
> +	sdev = std::sqrt(var);
> +
> +	if (var) {
> +		skew /= n * var * sdev;
> +		curt = curt / (n * var * var) - 3.0;
> +	}
> +	skew_ = skew;
> +}
> +
> +void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> +{
> +	brightnessVec_.clear();
> +
> +	/*\todo Replace constant values with real BDS configuration */
> +	for (uint32_t j = 0; j < 45; j++) {
> +		for (uint32_t i = 0; i < 160 * 45 * 8; i += 8) {
> +			uint8_t Gr = stats->awb_raw_buffer.meta_data[i];
> +			uint8_t R = stats->awb_raw_buffer.meta_data[i + 1];
> +			uint8_t B = stats->awb_raw_buffer.meta_data[i + 2];
> +			uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3];
> +			brightnessVec_.push_back(static_cast<uint32_t>(0.299 * R + 0.587 * (Gr + Gb) / 2 + 0.114 * B));
> +		}
> +	}
> +	std::sort(brightnessVec_.begin(), brightnessVec_.end());
> +
> +	/* \todo create a class to generate histograms ! */
> +	std::unordered_map<uint32_t, uint32_t> hist;
> +	for (uint32_t const &val : brightnessVec_)
> +		hist[val]++;
> +	moments(hist, 256);
> +}
> +
> +void IPU3Agc::lockExposure(uint32_t &exposure, uint32_t &gain)
> +{
> +	/* Algorithm initialization wait for first valid frames */
> +	/* \todo - have a number of frames given by DelayedControls ?
> +	 * - implement a function for IIR */
> +	if (frameCount_ == 4) {
> +		prevExposure_ = exposure;
> +
> +		prevSkew_ = skew_;
> +		/* \tdo use configured values */

s/tdo/todo/

> +		exposure = 800;
> +		gain = 8;
> +		currentExposure_ = exposure;
> +	} else if (frameCount_ == 8) {
> +		currentSkew_ = skew_;
> +		exposure = ((currentExposure_ * prevSkew_) + (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);

Line wrap. No need for the inner parentheses.

> +		nextExposure_ = exposure;
> +		lastFrame_ = frameCount_;
> +	} else if ((frameCount_ >= 12) && (frameCount_ - lastFrame_ >= 4)) {

Could you define constants for magic values, to make the code more
readable ? They can be defined as static constexpr members of the
IPU3Agc class, and should be named with camelCase. While we don't do so
yet in many places, it's customary to start them with a 'k' prefix (e.g.
kInitialFrameSkipCount).

> +		currentSkew_ = skew_;
> +		/* \todo properly calculate a gain */
> +		if (frameCount_ == 12)
> +			gain = ((8 * prevSkew_) + (1 * currentSkew_)) / (prevSkew_ + currentSkew_);
> +
> +		if (currentSkew_ - prevSkew_ > 1) {
> +			/* under exposed */
> +			prevExposure_ = nextExposure_;
> +			exposure = ((currentExposure_ * prevSkew_) + (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
> +			nextExposure_ = exposure;
> +		} else if (currentSkew_ - prevSkew_ < -1) {
> +			/* over exposed */
> +			currentExposure_ = nextExposure_;
> +			exposure = ((currentExposure_ * prevSkew_) + (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
> +			nextExposure_ = exposure;
> +		} else {
> +			/* we have converged */
> +			algoConverged_ = true;
> +		}
> +		lastFrame_ = frameCount_;
> +		prevSkew_ = currentSkew_;
> +	}
> +}
> +
> +void IPU3Agc::Process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain)
> +{
> +	processBrightness(stats);
> +	if (!algoConverged_)
> +		lockExposure(exposure, gain);
> +	else {
> +		/* Are we still well exposed ? */
> +		if ((skew_ < 2) || (skew_ > 4))
> +			algoConverged_ = false;
> +	}
> +	frameCount_++;
> +}
> +
> +bool IPU3Agc::Converged()
> +{
> +	return algoConverged_;
> +}
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> new file mode 100644
> index 00000000..9a69d628
> --- /dev/null
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * ipu3_agc.h - AGC/AEC control algorithm
> + */
> +#ifndef __LIBCAMERA_IPU3_AGC_H__
> +#define __LIBCAMERA_IPU3_AGC_H__
> +

#include <map>

> +#include <mutex>

This doesn't seem to be needed.

> +#include <vector>
> +
> +#include <linux/intel-ipu3.h>
> +
> +#include <libcamera/geometry.h>
> +
> +#include <libcamera/ipa/agc_algorithm.h>
> +#include <libcamera/ipa/ipa_controller.h>
> +
> +#define AGC_STATS_SIZE 15
> +
> +namespace libcamera {
> +
> +struct AgcMeteringMode {
> +	double weights[AGC_STATS_SIZE];
> +	Rectangle metering_region[AGC_STATS_SIZE];
> +};
> +
> +struct AgcExposureMode {
> +	std::vector<double> shutter;
> +	std::vector<double> gain;
> +};
> +
> +struct AgcConfig {
> +	std::map<std::string, AgcMeteringMode> metering_modes;
> +	std::map<std::string, AgcExposureMode> exposure_modes;
> +	double speed;
> +	uint16_t startup_frames;
> +	unsigned int convergence_frames;
> +	std::string default_metering_mode;
> +	std::string default_exposure_mode;
> +	double default_exposure_time;
> +	double default_analogue_gain;
> +};
> +
> +class IPU3Agc : public AgcAlgorithm
> +{
> +public:
> +	IPU3Agc(IPAController *IPAcontroller);

Blank line.

> +	char const *Name() const override;
> +	unsigned int GetConvergenceFrames() const override;
> +	void SetEv(double ev) override;
> +	void SetFlickerPeriod(double flicker_period) override;
> +	void SetMaxShutter(double max_shutter) override; // microseconds
> +	void SetFixedShutter(double fixed_shutter) override; // microseconds
> +	void SetFixedAnalogueGain(double fixed_analogue_gain) override;
> +	void SetMeteringMode(std::string const &metering_mode_name) override;
> +	void SetExposureMode(std::string const &exposure_mode_name) override;
> +	void Prepare() override;
> +	void Process() override;
> +	void Process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain);

The last two arguments should be replaced by a generic data container,
similar to the Metadata class in the RPi IPA (not something to fix now).

> +	bool Converged();
> +
> +private:
> +	void moments(std::unordered_map<uint32_t, uint32_t> &data, int n);
> +	void processBrightness(const ipu3_uapi_stats_3a *stats);
> +	void lockExposure(uint32_t &exposure, uint32_t &gain);
> +
> +	AgcConfig config_;

This is never set, and only one member is read. I'd drop it for now.

> +	std::string metering_mode_name_;
> +	std::string exposure_mode_name_;
> +	uint64_t frameCount_;
> +	uint64_t lastFrame_;
> +
> +	/* Vector of calculated brightness for each cell */
> +	std::vector<uint32_t> brightnessVec_;

We avoid encoding the type in the variable name. Would this be better
named cellsBrightness_ ?

> +	double ev_;

Never used. Same for other members below.

> +	double flicker_period_;
> +	double max_shutter_;
> +	double fixed_shutter_;
> +	double fixed_analogue_gain_;
> +
> +	/* Values for filtering */
> +	uint32_t prevExposure_;
> +	uint32_t currentExposure_;
> +	uint32_t nextExposure_;
> +
> +	double skew_;
> +	double prevSkew_;
> +	double currentSkew_;
> +	bool algoConverged_;

Maybe just converged_ ?

> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPU3_AGC_H__ */
> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> new file mode 100644
> index 00000000..21286e4c
> --- /dev/null
> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> @@ -0,0 +1,182 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * ipu3_awb.cpp - AWB control algorithm
> + */
> +#include <iostream>

I don't think this is needed.

> +#include <numeric>
> +#include <unordered_map>
> +
> +#include "libcamera/internal/log.h"
> +
> +#include "ipu3_awb.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(IPU3Awb)
> +
> +#define NAME "ipu3.awb"
> +
> +IPU3Awb::IPU3Awb(IPAController *controller)
> +	: AwbAlgorithm(controller)
> +{
> +}
> +
> +IPU3Awb::~IPU3Awb()
> +{
> +}
> +
> +char const *IPU3Awb::Name() const
> +{
> +	return NAME;
> +}
> +
> +void IPU3Awb::Initialise() {}

void IPU3Awb::Initialise()
{
}

> +
> +void IPU3Awb::Initialise(ipu3_uapi_params &params)
> +{
> +	memset(&params, 0, sizeof(params));
> +	memset(wbGains_, 8192, sizeof(wbGains_));

memset() initializes the memory byte by byte, this will not store 8192
in each entry of the array.


> +	wbGains_[0] = 8192 * 0.8;
> +	wbGains_[3] = 8192 * 0.8;

I would group the initialization of member variables together, either
before, or after the initialization of params.

> +	params.use.acc_awb = 1;
> +	/*\todo fill the grid calculated based on BDS configuration */
> +	params.acc_param.awb.config = imgu_css_awb_defaults;
> +
> +	params.use.acc_bnr = 1;
> +	params.acc_param.bnr = imgu_css_bnr_defaults;
> +
> +	params.use.acc_ccm = 1;
> +	params.acc_param.ccm = imgu_css_ccm_3800k;
> +
> +	params.use.acc_gamma = 1;
> +	params.acc_param.gamma.gc_ctrl.enable = 1;
> +
> +	uint32_t a = (32 * 245) / (245 - 9);
> +
> +	for (uint32_t i = 0; i < 10; i++)
> +		params.acc_param.gamma.gc_lut.lut[i] = 0;
> +	for (uint32_t i = 10; i < 245; i++)
> +		params.acc_param.gamma.gc_lut.lut[i] = a * i + (0 - a * 9);
> +	for (uint32_t i = 245; i < 255; i++)
> +		params.acc_param.gamma.gc_lut.lut[i] = 32 * 245;
> +
> +	frame_count_ = 0;
> +	algoConverged_ = false;
> +}
> +
> +unsigned int IPU3Awb::GetConvergenceFrames() const
> +{
> +	// If colour gains have been explicitly set, there is no convergence
> +	// to happen, so no need to drop any frames - return zero.
> +	if (manual_r_ && manual_b_)
> +		return 0;
> +	else
> +		return config_.convergence_frames;
> +}
> +
> +void IPU3Awb::SetMode(std::string const &mode_name)
> +{
> +	mode_name_ = mode_name;
> +}
> +
> +void IPU3Awb::SetManualGains(double manual_r, double manual_b)
> +{
> +	// If any of these are 0.0, we swich back to auto.
> +	manual_r_ = manual_r;
> +	manual_b_ = manual_b;
> +}
> +
> +uint32_t IPU3Awb::estimateCCT(uint8_t R, uint8_t G, uint8_t B)
> +{
> +	double X = (-0.14282) * (R) + (1.54924) * (G) + (-0.95641) * (B);
> +	double Y = (-0.32466) * (R) + (1.57837) * (G) + (-0.73191) * (B);
> +	double Z = (-0.68202) * (R) + (0.77073) * (G) + (0.56332) * (B);
> +
> +	double x = X / (X + Y + Z);
> +	double y = Y / (X + Y + Z);
> +
> +	double n = (x - 0.3320) / (0.1858 - y);
> +	return static_cast<uint32_t>(449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33);
> +}
> +
> +void IPU3Awb::calculateWBGains([[maybe_unused]] Rectangle roi, const ipu3_uapi_stats_3a *stats)

Line wrap.

> +{
> +	if (algoConverged_)
> +		return;

So once it converges, you never update the gains anymore, even if the
scene changes ?

> +
> +	std::vector<uint32_t> R_v, Gr_v, Gb_v, B_v;

camelCase. Feel free to also make the name slightly longer to make the
more explicit. redValues ?

> +	Point topleft = roi.topLeft();
> +	uint32_t startY = (topleft.y / 16) * 160 * 8;
> +	uint32_t startX = (topleft.x / 8) * 8;
> +	uint32_t endX = (startX + (roi.size().width / 8)) * 8;
> +
> +	for (uint32_t j = (topleft.y / 16); j < (topleft.y / 16) + (roi.size().height / 16); j++) {
> +		for (uint32_t i = startX + startY; i < endX + startY; i += 8) {
> +			Gr_v.push_back(stats->awb_raw_buffer.meta_data[i]);
> +			R_v.push_back(stats->awb_raw_buffer.meta_data[i + 1]);
> +			B_v.push_back(stats->awb_raw_buffer.meta_data[i + 2]);
> +			Gb_v.push_back(stats->awb_raw_buffer.meta_data[i + 3]);
> +		}
> +	}
> +
> +	std::sort(R_v.begin(), R_v.end());
> +	std::sort(Gr_v.begin(), Gr_v.end());
> +	std::sort(B_v.begin(), B_v.end());
> +	std::sort(Gb_v.begin(), Gb_v.end());
> +
> +	double Grmed = Gr_v[Gr_v.size() / 2];
> +	double Rmed = R_v[R_v.size() / 2];
> +	double Bmed = B_v[B_v.size() / 2];
> +	double Gbmed = Gb_v[Gb_v.size() / 2];
> +
> +	double Rgain = Grmed / Rmed;
> +	double Bgain = Gbmed / Bmed;
> +	LOG(IPU3Awb, Debug) << "max R, Gr, B, Gb: "
> +			    << R_v.back() << ","
> +			    << Gr_v.back() << ","
> +			    << B_v.back() << ","
> +			    << Gb_v.back();
> +	tint_ = ((Rmed / Grmed) + (Bmed / Gbmed)) / 2;
> +
> +	/* \todo Those are corrections when light is really low
> +	 * it should be taken into account by AGC somehow */
> +	if ((Rgain >= 2) && (Bgain < 2)) {
> +		wbGains_[0] = 4096 * tint_;
> +		wbGains_[1] = 8192 * Rgain;
> +		wbGains_[2] = 4096 * Bgain;
> +		wbGains_[3] = 4096 * tint_;
> +	} else if ((Bgain >= 2) && (Rgain < 2)) {
> +		wbGains_[0] = 4096 * tint_;
> +		wbGains_[1] = 4096 * Rgain;
> +		wbGains_[2] = 8192 * Bgain;
> +		wbGains_[3] = 4096 * tint_;
> +	} else {
> +		wbGains_[0] = 8192 * tint_;
> +		wbGains_[1] = 8192 * Rgain;
> +		wbGains_[2] = 8192 * Bgain;
> +		wbGains_[3] = 8192 * tint_;
> +	}
> +
> +	frame_count_++;
> +
> +	cct_ = estimateCCT(Rmed, (Grmed + Gbmed) / 2, Bmed);
> +
> +	algoConverged_ = true;
> +}
> +
> +void IPU3Awb::updateBNR(ipu3_uapi_params &params)

updateBNR() isn't a great name for a function that updates the WB gains,
even if they're part of the Bayer noise reduction block of data.

Should we push the ipu3_uapi_params structure down to individual
algorithms, or would it be better to decouple it, creating custom
structures to hold algorithm results, and fillin ipu3_uapi_params in
IPAIPU3::fillParams() instead ? On one hand moving ipu3_uapi_params out
will make the code more generic, and thus more reusable, but on the
other hand we would still depend on a custom stats format, so it may not
be worth it.

> +{
> +	if (!algoConverged_)
> +		return;
> +
> +	params.acc_param.bnr.wb_gains.gr = wbGains_[0];
> +	params.acc_param.bnr.wb_gains.r = wbGains_[1];
> +	params.acc_param.bnr.wb_gains.b = wbGains_[2];
> +	params.acc_param.bnr.wb_gains.gb = wbGains_[3];
> +	if (cct_ < 5500)
> +		params.acc_param.ccm = imgu_css_ccm_3800k;
> +	else
> +		params.acc_param.ccm = imgu_css_ccm_6000k;
> +}
> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> new file mode 100644
> index 00000000..06389020
> --- /dev/null
> +++ b/src/ipa/ipu3/ipu3_awb.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * awb.h - AWB control algorithm
> + */
> +#ifndef __LIBCAMERA_IPU3_AWB_H__
> +#define __LIBCAMERA_IPU3_AWB_H__
> +
> +#include <linux/intel-ipu3.h>
> +
> +#include <libcamera/geometry.h>
> +
> +#include <libcamera/ipa/awb_algorithm.h>
> +#include <libcamera/ipa/ipa_controller.h>
> +
> +namespace libcamera {
> +
> +const struct ipu3_uapi_bnr_static_config imgu_css_bnr_defaults = {
> +	{ 16, 16, 16, 16 },			/* wb_gains */
> +	{ 255, 255, 255, 255 },			/* wb_gains_thr */
> +	{ 0, 0, 8, 6, 0, 14 },			/* thr_coeffs */
> +	{ 0, 0, 0, 0 },				/* thr_ctrl_shd */
> +	{ -648, 0, -366, 0 },			/* opt_center */
> +	{					/* 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 }
> +	},
> +	{ 4, 0, 1, 8, 0, 8, 0, 8, 0 },		/* bp_ctrl */
> +	{ 8, 4, 4, 0, 8, 0, 1, 1, 1, 1, 0 },	/* dn_detect_ctrl */
> +	1296,
> +	{ 419904, 133956 },
> +};
> +
> +/* settings for Auto White Balance */
> +const struct ipu3_uapi_awb_config_s imgu_css_awb_defaults = {
> +	8191,
> +	8191,
> +	8191,
> +	8191 | /* rgbs_thr_gr/r/gb/b */
> +		IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
> +	.grid = {
> +		.width = 160,
> +		.height = 45,
> +		.block_width_log2 = 3,
> +		.block_height_log2 = 4,
> +		.x_start = 0,
> +		.y_start = 0,
> +	},
> +};
> +
> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_6000k = {
> +	7239, -750, -37, 0,
> +	-215, 9176, -200, 0,
> +	-70, -589, 6810, 0
> +};
> +
> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_4900k = {
> +	7811, -464, -466, 0,
> +	-635, 8762, -533, 0,
> +	-469, -154, 6583, 0
> +};
> +
> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_3800k = {
> +	7379, -526, -296, 0,
> +	-411, 7397, -415, 0,
> +	-224, -564, 7244, 0
> +};

All these should be moved to ipu3_awb.cpp, and be made static. Otherwise
they would get duplicated in every compilation unit in which this header
is included.

> +
> +struct AwbConfig {
> +	// Only repeat the AWB calculation every "this many" frames
> +	uint16_t frame_period;
> +	// number of initial frames for which speed taken as 1.0 (maximum)
> +	uint16_t startup_frames;
> +	unsigned int convergence_frames; // approx number of frames to converge
> +	double speed; // IIR filter speed applied to algorithm results
> +};
> +
> +#if 0
> +typedef struct awb_public_set_item{
> +    unsigned char Gr_avg;
> +    unsigned char R_avg;
> +    unsigned char B_avg;
> +    unsigned char Gb_avg;
> +    unsigned char sat_ratio;
> +    unsigned char padding0; /**< Added the padding so that the public matches that private */
> +    unsigned char padding1; /**< Added the padding so that the public matches that private */
> +    unsigned char padding2; /**< Added the padding so that the public matches that private */
> +} awb_public_set_item_t;
> +#endif

If the compiler doesn't need to see this, I think you can drop it
completely :-)

> +
> +class IPU3Awb : public AwbAlgorithm
> +{
> +public:
> +	IPU3Awb(IPAController *controller = NULL);

This is C++, so nullptr :-) But there's no need for a default value, as
the controller parameter should either be mandatory, or removed
completely.

> +	~IPU3Awb();

Blank line.

> +	virtual char const *Name() const override;

This is never used, you can drop it from the base class. When (and if)
we'll need this, I'd rather pass the name to the base class constructor,
store it there, and implement name() in the base class only, as a
non-virtual function.

> +	virtual void Initialise() override;

No need to repeat the virtual keyword for overridden functions.

> +	void Initialise(ipu3_uapi_params &params);

This isn't very nice, as it's not aligned with the design of the
IPAAlgorithm class, but we can fix it later. I would move the function
down, after the mandatory functions, with calculateWBGains() and
updateBNR().

> +	unsigned int GetConvergenceFrames() const override;
> +	void SetMode(std::string const &name) override;
> +	void SetManualGains(double manual_r, double manual_b) override;

A blank line here would be nice.

> +	void calculateWBGains(Rectangle roi,

const Rectangle &roi

> +			      const ipu3_uapi_stats_3a *stats);
> +	void updateBNR(ipu3_uapi_params &params);
> +
> +private:
> +	uint32_t estimateCCT(uint8_t R, uint8_t G, uint8_t B);

r, g and b.

> +
> +	/* configuration is read-only, and available to both threads */

What threads ? :-)

> +	AwbConfig config_;
> +	std::string mode_name_;

Never used.

> +	/* manual r setting */
> +	double manual_r_;

manualRedGain_ ?

> +	/* manual b setting */
> +	double manual_b_;
> +	/* WB calculated gains */
> +	uint16_t wbGains_[4];
> +	double tint_;
> +	uint32_t cct_;
> +
> +	uint32_t frame_count_;
> +
> +	bool algoConverged_;
> +};
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPU3_AWB_H__ */
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index a241f617..43ad0e0d 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -2,8 +2,14 @@
>  
>  ipa_name = 'ipa_ipu3'
>  
> +ipu3_ipa_sources = files([
> +  'ipu3.cpp',
> +  'ipu3_agc.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,
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index ff980b38..3809c943 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -825,6 +825,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  	 */
>  	double lineDuration = sensorInfo.lineLength
>  			    / (sensorInfo.pixelRate / 1e6);
> +	LOG(IPU3, Error) << "line duration: " << lineDuration;

Is this an error ? :-)

>  	const ControlInfoMap &sensorControls = sensor->controls();
>  	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
>  	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;

Hi,

On 22.02.21 05:56, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Fri, Feb 19, 2021 at 06:22:24PM +0100, Jean-Michel Hautbois wrote:
>> Inherit from the AWBAlgorithm and AGCAlgorithm classes to implement
>> basic functions.
>> While exposure is not locked, AWB is not calculated and corrected.
>> Once AWB is done, a color temperature is estimated and default CCM matrices
>> are used.
>> Implement a basic "grey-world" AWB algorithm just for demonstration purpose.

Are those algorithms specific to IPU3 ?
If not then maybe it can used by rkisp1 as well?
Maybe it worth to think of general API for basic 3a implementations that other IPAs can use?

> 
> Nice to see an initial implementation ! I'll provide a handful of review
> comments below already. Some are related to coding style or similar
> issues and should be addressed in v2. Others are related to the IPA
> architecture, and could be fixed on top of this series to avoid delaying
> it unnecessarily. They should in that case be captured in a \todo
> comment.
> 
> As for 1/2, I won't repeat the same comments all over the patch, so
> please apply them wherever they're applicable.
> 
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> ---
>>   src/ipa/ipu3/ipu3.cpp                |  31 ++++-
>>   src/ipa/ipu3/ipu3_agc.cpp            | 195 +++++++++++++++++++++++++++
>>   src/ipa/ipu3/ipu3_agc.h              |  96 +++++++++++++
>>   src/ipa/ipu3/ipu3_awb.cpp            | 182 +++++++++++++++++++++++++
>>   src/ipa/ipu3/ipu3_awb.h              | 130 ++++++++++++++++++
>>   src/ipa/ipu3/meson.build             |   8 +-
>>   src/libcamera/pipeline/ipu3/ipu3.cpp |   1 +
>>   7 files changed, 638 insertions(+), 5 deletions(-)
>>   create mode 100644 src/ipa/ipu3/ipu3_agc.cpp
>>   create mode 100644 src/ipa/ipu3/ipu3_agc.h
>>   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 b63e58be..c3859f39 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -13,6 +13,7 @@
>>   
>>   #include <libcamera/buffer.h>
>>   #include <libcamera/control_ids.h>
>> +#include <libcamera/ipa/ipa_controller.h>
>>   #include <libcamera/ipa/ipa_interface.h>
>>   #include <libcamera/ipa/ipa_module_info.h>
>>   #include <libcamera/ipa/ipu3_ipa_interface.h>
>> @@ -21,6 +22,9 @@
>>   #include "libcamera/internal/buffer.h"
>>   #include "libcamera/internal/log.h"
>>   
>> +#include "ipu3_agc.h"
>> +#include "ipu3_awb.h"
>> +
>>   namespace libcamera {
>>   
>>   LOG_DEFINE_CATEGORY(IPAIPU3)
>> @@ -28,6 +32,9 @@ LOG_DEFINE_CATEGORY(IPAIPU3)
>>   class IPAIPU3 : public ipa::ipu3::IPAIPU3Interface
>>   {
>>   public:
>> +	IPAIPU3()
>> +		: controller_() {}
> 
> 		: controller_()
> 	{
> 	}
> 
> I may not repeat all the comments from 1/2, but they're generally
> applicable to this patch as well.
> 
>> +
>>   	int init([[maybe_unused]] const IPASettings &settings) override
>>   	{
>>   		return 0;
>> @@ -60,6 +67,11 @@ private:
>>   	uint32_t gain_;
>>   	uint32_t minGain_;
>>   	uint32_t maxGain_;
>> +
>> +	IPAController controller_;
>> +	IPU3Awb *awbAlgo_;
>> +	IPU3Agc *agcAlgo_;
>> +	ipu3_uapi_params params_;
>>   };
>>   
>>   void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls)
>> @@ -83,11 +95,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_ = 123;
> 
> A peculiar value :-)
> 
>>   
>>   	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>>   	maxGain_ = itGain->second.max().get<int32_t>();
>> -	gain_ = maxGain_;
>> +	gain_ = 1;
>> +
>> +	awbAlgo_ = new IPU3Awb(&controller_);
>> +	awbAlgo_->Initialise(params_);
>> +	agcAlgo_ = new IPU3Agc(&controller_);
>> +
>> +	/*\todo not used yet... */
>> +	controller_.Initialise();
>>   
>>   	setControls(0);
>>   }
>> @@ -162,10 +181,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));
>> +	awbAlgo_->updateBNR(params_);
>> +	memcpy(params, &params_, sizeof(*params));
> 
> 	*params = params_;
> 
>>   
>>   	/* \todo Fill in parameters buffer. */
>> -
>>   	ipa::ipu3::IPU3Action op;
>>   	op.op = ipa::ipu3::ActionParamFilled;
>>   
>> @@ -179,6 +198,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>   
>>   	/* \todo React to statistics and update internal state machine. */
>>   	/* \todo Add meta-data information to ctrls. */
>> +	agcAlgo_->Process(stats, exposure_, gain_);
>> +	if (agcAlgo_->Converged())
>> +		awbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400), stats);
>> +	setControls(frame);
>>   
>>   	ipa::ipu3::IPU3Action op;
>>   	op.op = ipa::ipu3::ActionMetadataReady;
>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
>> new file mode 100644
>> index 00000000..db591e33
>> --- /dev/null
>> +++ b/src/ipa/ipu3/ipu3_agc.cpp
>> @@ -0,0 +1,195 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>> + *
>> + * agc.cpp - AGC/AEC control algorithm
>> + */
>> +#include <iostream>
>> +#include <numeric>
>> +#include <unordered_map>
>> +
>> +#include "libcamera/internal/log.h"
>> +
>> +#include "ipu3_agc.h"
>> +
>> +using namespace libcamera;
>> +
>> +LOG_DEFINE_CATEGORY(IPU3Agc)
>> +
>> +#define NAME "ipu3.agc"
>> +
>> +IPU3Agc::IPU3Agc(IPAController *controller)
>> +	: AgcAlgorithm(controller), frameCount_(0),
>> +	  ev_(1.0), flicker_period_(0.0),
>> +	  max_shutter_(0), fixed_shutter_(0),
>> +	  fixed_analogue_gain_(0.0), algoConverged_(false)
>> +{
>> +}
>> +
>> +char const *IPU3Agc::Name() const
>> +{
>> +	return NAME;
>> +}
>> +
>> +unsigned int IPU3Agc::GetConvergenceFrames() const
>> +{
>> +	return config_.convergence_frames;
>> +}
>> +
>> +void IPU3Agc::SetEv(double ev)
>> +{
>> +	ev_ = ev;
>> +}
>> +
>> +void IPU3Agc::SetFlickerPeriod(double flicker_period)
>> +{
>> +	flicker_period_ = flicker_period;
>> +}
>> +
>> +void IPU3Agc::SetMaxShutter(double max_shutter)
>> +{
>> +	max_shutter_ = max_shutter;
>> +}
>> +
>> +void IPU3Agc::SetFixedShutter(double fixed_shutter)
>> +{
>> +	fixed_shutter_ = fixed_shutter;
>> +}
>> +
>> +void IPU3Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
>> +{
>> +	fixed_analogue_gain_ = fixed_analogue_gain;
>> +}
>> +
>> +void IPU3Agc::SetMeteringMode(std::string const &metering_mode_name)
>> +{
>> +	metering_mode_name_ = metering_mode_name;
>> +}
>> +
>> +void IPU3Agc::SetExposureMode(std::string const &exposure_mode_name)
>> +{
>> +	exposure_mode_name_ = exposure_mode_name;
>> +}
>> +
>> +void IPU3Agc::Prepare() {}
>> +
>> +void IPU3Agc::Process() {}
>> +
>> +/* \todo This function is taken from numerical recipes and calculates all moments
>> + * It needs to be rewritten properly and maybe in a "math" class ? */
> 
> /*
>   * \todo This function is taken from numerical recipes and calculates all
>   * moments. It needs to be rewritten properly and maybe in a "math" class ?
>   */
> 
>> +void IPU3Agc::moments(std::unordered_map<uint32_t, uint32_t> &data, int n)

'data' is the hist, so why not call it 'hist' here?

>> +{
>> +	int j;
>> +	double ep = 0.0, s, p;
>> +	double ave, adev, sdev;
>> +	double var, skew, curt;
>> +
>> +	s = 0.0;
>> +	for (j = 1; j <= n; j++)
>> +		s += data[j];

shouldn't that be:
s += data[j] * j

since data[j] is the number of pixels with value j

>> +
>> +	ave = s / n;
>> +	adev = var = skew = curt = 0.0;
>> +
>> +	for (j = 1; j <= n; j++) {
>> +		adev += s = data[j] - (ave);
>> +		ep += s;
>> +		var += (p = s * s);
>> +		skew += (p *= s);
>> +		curt += (p *= s);
>> +	}
>> +
>> +	adev /= n;
>> +	var = (var - ep * ep / n) / (n - 1);
>> +	sdev = std::sqrt(var);
>> +
>> +	if (var) {
>> +		skew /= n * var * sdev;
>> +		curt = curt / (n * var * var) - 3.0;
>> +	}
>> +	skew_ = skew;
>> +}
>> +
>> +void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>> +{
>> +	brightnessVec_.clear();
>> +
>> +	/*\todo Replace constant values with real BDS configuration */
>> +	for (uint32_t j = 0; j < 45; j++) {
>> +		for (uint32_t i = 0; i < 160 * 45 * 8; i += 8) {
>> +			uint8_t Gr = stats->awb_raw_buffer.meta_data[i];
>> +			uint8_t R = stats->awb_raw_buffer.meta_data[i + 1];
>> +			uint8_t B = stats->awb_raw_buffer.meta_data[i + 2];
>> +			uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3];
>> +			brightnessVec_.push_back(static_cast<uint32_t>(0.299 * R + 0.587 * (Gr + Gb) / 2 + 0.114 * B));

You iterate on j in the outer loop but don't use j anywhere for indexing, this is a bit suspicious

>> +		}
>> +	}
>> +	std::sort(brightnessVec_.begin(), brightnessVec_.end());

I don't think the sort is is necessary

>> +
>> +	/* \todo create a class to generate histograms ! */
>> +	std::unordered_map<uint32_t, uint32_t> hist;

you want to have up to 256 indexes so you need to define with uint8_t

std::unordered_map<uint8_t, uint32_t> hist;


>> +	for (uint32_t const &val : brightnessVec_)
>> +		hist[val]++;
>> +	moments(hist, 256);
>> +}
>> +
>> +void IPU3Agc::lockExposure(uint32_t &exposure, uint32_t &gain)
>> +{
>> +	/* Algorithm initialization wait for first valid frames */
>> +	/* \todo - have a number of frames given by DelayedControls ?
>> +	 * - implement a function for IIR */
>> +	if (frameCount_ == 4) {
>> +		prevExposure_ = exposure;
>> +
>> +		prevSkew_ = skew_;
>> +		/* \tdo use configured values */
> 
> s/tdo/todo/
> 
>> +		exposure = 800;
>> +		gain = 8;
>> +		currentExposure_ = exposure;
>> +	} else if (frameCount_ == 8) {
>> +		currentSkew_ = skew_;
>> +		exposure = ((currentExposure_ * prevSkew_) + (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
> 
> Line wrap. No need for the inner parentheses.
> 
>> +		nextExposure_ = exposure;
>> +		lastFrame_ = frameCount_;
>> +	} else if ((frameCount_ >= 12) && (frameCount_ - lastFrame_ >= 4)) {
> 
> Could you define constants for magic values, to make the code more
> readable ? They can be defined as static constexpr members of the
> IPU3Agc class, and should be named with camelCase. While we don't do so
> yet in many places, it's customary to start them with a 'k' prefix (e.g.
> kInitialFrameSkipCount).
> 
>> +		currentSkew_ = skew_;
>> +		/* \todo properly calculate a gain */
>> +		if (frameCount_ == 12)
>> +			gain = ((8 * prevSkew_) + (1 * currentSkew_)) / (prevSkew_ + currentSkew_);
>> +
>> +		if (currentSkew_ - prevSkew_ > 1) {
>> +			/* under exposed */
>> +			prevExposure_ = nextExposure_;
>> +			exposure = ((currentExposure_ * prevSkew_) + (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
>> +			nextExposure_ = exposure;
>> +		} else if (currentSkew_ - prevSkew_ < -1) {
>> +			/* over exposed */
>> +			currentExposure_ = nextExposure_;
>> +			exposure = ((currentExposure_ * prevSkew_) + (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
>> +			nextExposure_ = exposure;
>> +		} else {
>> +			/* we have converged */
>> +			algoConverged_ = true;
>> +		}
>> +		lastFrame_ = frameCount_;
>> +		prevSkew_ = currentSkew_;
>> +	}

Could you explain the general idea behind this algorithm or give a link where it is explained?

Thanks,
Dafna

>> +}
>> +
>> +void IPU3Agc::Process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain)
>> +{
>> +	processBrightness(stats);
>> +	if (!algoConverged_)
>> +		lockExposure(exposure, gain);
>> +	else {
>> +		/* Are we still well exposed ? */
>> +		if ((skew_ < 2) || (skew_ > 4))
>> +			algoConverged_ = false;
>> +	}
>> +	frameCount_++;
>> +}
>> +
>> +bool IPU3Agc::Converged()
>> +{
>> +	return algoConverged_;
>> +}
>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
>> new file mode 100644
>> index 00000000..9a69d628
>> --- /dev/null
>> +++ b/src/ipa/ipu3/ipu3_agc.h
>> @@ -0,0 +1,96 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>> + *
>> + * ipu3_agc.h - AGC/AEC control algorithm
>> + */
>> +#ifndef __LIBCAMERA_IPU3_AGC_H__
>> +#define __LIBCAMERA_IPU3_AGC_H__
>> +
> 
> #include <map>
> 
>> +#include <mutex>
> 
> This doesn't seem to be needed.
> 
>> +#include <vector>
>> +
>> +#include <linux/intel-ipu3.h>
>> +
>> +#include <libcamera/geometry.h>
>> +
>> +#include <libcamera/ipa/agc_algorithm.h>
>> +#include <libcamera/ipa/ipa_controller.h>
>> +
>> +#define AGC_STATS_SIZE 15
>> +
>> +namespace libcamera {
>> +
>> +struct AgcMeteringMode {
>> +	double weights[AGC_STATS_SIZE];
>> +	Rectangle metering_region[AGC_STATS_SIZE];
>> +};
>> +
>> +struct AgcExposureMode {
>> +	std::vector<double> shutter;
>> +	std::vector<double> gain;
>> +};
>> +
>> +struct AgcConfig {
>> +	std::map<std::string, AgcMeteringMode> metering_modes;
>> +	std::map<std::string, AgcExposureMode> exposure_modes;
>> +	double speed;
>> +	uint16_t startup_frames;
>> +	unsigned int convergence_frames;
>> +	std::string default_metering_mode;
>> +	std::string default_exposure_mode;
>> +	double default_exposure_time;
>> +	double default_analogue_gain;
>> +};
>> +
>> +class IPU3Agc : public AgcAlgorithm
>> +{
>> +public:
>> +	IPU3Agc(IPAController *IPAcontroller);
> 
> Blank line.
> 
>> +	char const *Name() const override;
>> +	unsigned int GetConvergenceFrames() const override;
>> +	void SetEv(double ev) override;
>> +	void SetFlickerPeriod(double flicker_period) override;
>> +	void SetMaxShutter(double max_shutter) override; // microseconds
>> +	void SetFixedShutter(double fixed_shutter) override; // microseconds
>> +	void SetFixedAnalogueGain(double fixed_analogue_gain) override;
>> +	void SetMeteringMode(std::string const &metering_mode_name) override;
>> +	void SetExposureMode(std::string const &exposure_mode_name) override;
>> +	void Prepare() override;
>> +	void Process() override;
>> +	void Process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain);
> 
> The last two arguments should be replaced by a generic data container,
> similar to the Metadata class in the RPi IPA (not something to fix now).
> 
>> +	bool Converged();
>> +
>> +private:
>> +	void moments(std::unordered_map<uint32_t, uint32_t> &data, int n);
>> +	void processBrightness(const ipu3_uapi_stats_3a *stats);
>> +	void lockExposure(uint32_t &exposure, uint32_t &gain);
>> +
>> +	AgcConfig config_;
> 
> This is never set, and only one member is read. I'd drop it for now.
> 
>> +	std::string metering_mode_name_;
>> +	std::string exposure_mode_name_;
>> +	uint64_t frameCount_;
>> +	uint64_t lastFrame_;
>> +
>> +	/* Vector of calculated brightness for each cell */
>> +	std::vector<uint32_t> brightnessVec_;
> 
> We avoid encoding the type in the variable name. Would this be better
> named cellsBrightness_ ?
> 
>> +	double ev_;
> 
> Never used. Same for other members below.
> 
>> +	double flicker_period_;
>> +	double max_shutter_;
>> +	double fixed_shutter_;
>> +	double fixed_analogue_gain_;
>> +
>> +	/* Values for filtering */
>> +	uint32_t prevExposure_;
>> +	uint32_t currentExposure_;
>> +	uint32_t nextExposure_;
>> +
>> +	double skew_;
>> +	double prevSkew_;
>> +	double currentSkew_;
>> +	bool algoConverged_;
> 
> Maybe just converged_ ?
> 
>> +};
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_IPU3_AGC_H__ */
>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
>> new file mode 100644
>> index 00000000..21286e4c
>> --- /dev/null
>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
>> @@ -0,0 +1,182 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>> + *
>> + * ipu3_awb.cpp - AWB control algorithm
>> + */
>> +#include <iostream>
> 
> I don't think this is needed.
> 
>> +#include <numeric>
>> +#include <unordered_map>
>> +
>> +#include "libcamera/internal/log.h"
>> +
>> +#include "ipu3_awb.h"
>> +
>> +using namespace libcamera;
>> +
>> +LOG_DEFINE_CATEGORY(IPU3Awb)
>> +
>> +#define NAME "ipu3.awb"
>> +
>> +IPU3Awb::IPU3Awb(IPAController *controller)
>> +	: AwbAlgorithm(controller)
>> +{
>> +}
>> +
>> +IPU3Awb::~IPU3Awb()
>> +{
>> +}
>> +
>> +char const *IPU3Awb::Name() const
>> +{
>> +	return NAME;
>> +}
>> +
>> +void IPU3Awb::Initialise() {}
> 
> void IPU3Awb::Initialise()
> {
> }
> 
>> +
>> +void IPU3Awb::Initialise(ipu3_uapi_params &params)
>> +{
>> +	memset(&params, 0, sizeof(params));
>> +	memset(wbGains_, 8192, sizeof(wbGains_));
> 
> memset() initializes the memory byte by byte, this will not store 8192
> in each entry of the array.
> 
> 
>> +	wbGains_[0] = 8192 * 0.8;
>> +	wbGains_[3] = 8192 * 0.8;
> 
> I would group the initialization of member variables together, either
> before, or after the initialization of params.
> 
>> +	params.use.acc_awb = 1;
>> +	/*\todo fill the grid calculated based on BDS configuration */
>> +	params.acc_param.awb.config = imgu_css_awb_defaults;
>> +
>> +	params.use.acc_bnr = 1;
>> +	params.acc_param.bnr = imgu_css_bnr_defaults;
>> +
>> +	params.use.acc_ccm = 1;
>> +	params.acc_param.ccm = imgu_css_ccm_3800k;
>> +
>> +	params.use.acc_gamma = 1;
>> +	params.acc_param.gamma.gc_ctrl.enable = 1;
>> +
>> +	uint32_t a = (32 * 245) / (245 - 9);
>> +
>> +	for (uint32_t i = 0; i < 10; i++)
>> +		params.acc_param.gamma.gc_lut.lut[i] = 0;
>> +	for (uint32_t i = 10; i < 245; i++)
>> +		params.acc_param.gamma.gc_lut.lut[i] = a * i + (0 - a * 9);
>> +	for (uint32_t i = 245; i < 255; i++)
>> +		params.acc_param.gamma.gc_lut.lut[i] = 32 * 245;
>> +
>> +	frame_count_ = 0;
>> +	algoConverged_ = false;
>> +}
>> +
>> +unsigned int IPU3Awb::GetConvergenceFrames() const
>> +{
>> +	// If colour gains have been explicitly set, there is no convergence
>> +	// to happen, so no need to drop any frames - return zero.
>> +	if (manual_r_ && manual_b_)
>> +		return 0;
>> +	else
>> +		return config_.convergence_frames;
>> +}
>> +
>> +void IPU3Awb::SetMode(std::string const &mode_name)
>> +{
>> +	mode_name_ = mode_name;
>> +}
>> +
>> +void IPU3Awb::SetManualGains(double manual_r, double manual_b)
>> +{
>> +	// If any of these are 0.0, we swich back to auto.
>> +	manual_r_ = manual_r;
>> +	manual_b_ = manual_b;
>> +}
>> +
>> +uint32_t IPU3Awb::estimateCCT(uint8_t R, uint8_t G, uint8_t B)
>> +{
>> +	double X = (-0.14282) * (R) + (1.54924) * (G) + (-0.95641) * (B);
>> +	double Y = (-0.32466) * (R) + (1.57837) * (G) + (-0.73191) * (B);
>> +	double Z = (-0.68202) * (R) + (0.77073) * (G) + (0.56332) * (B);
>> +
>> +	double x = X / (X + Y + Z);
>> +	double y = Y / (X + Y + Z);
>> +
>> +	double n = (x - 0.3320) / (0.1858 - y);
>> +	return static_cast<uint32_t>(449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33);
>> +}
>> +
>> +void IPU3Awb::calculateWBGains([[maybe_unused]] Rectangle roi, const ipu3_uapi_stats_3a *stats)
> 
> Line wrap.
> 
>> +{
>> +	if (algoConverged_)
>> +		return;
> 
> So once it converges, you never update the gains anymore, even if the
> scene changes ?
> 
>> +
>> +	std::vector<uint32_t> R_v, Gr_v, Gb_v, B_v;
> 
> camelCase. Feel free to also make the name slightly longer to make the
> more explicit. redValues ?
> 
>> +	Point topleft = roi.topLeft();
>> +	uint32_t startY = (topleft.y / 16) * 160 * 8;
>> +	uint32_t startX = (topleft.x / 8) * 8;
>> +	uint32_t endX = (startX + (roi.size().width / 8)) * 8;
>> +
>> +	for (uint32_t j = (topleft.y / 16); j < (topleft.y / 16) + (roi.size().height / 16); j++) {
>> +		for (uint32_t i = startX + startY; i < endX + startY; i += 8) {
>> +			Gr_v.push_back(stats->awb_raw_buffer.meta_data[i]);
>> +			R_v.push_back(stats->awb_raw_buffer.meta_data[i + 1]);
>> +			B_v.push_back(stats->awb_raw_buffer.meta_data[i + 2]);
>> +			Gb_v.push_back(stats->awb_raw_buffer.meta_data[i + 3]);
>> +		}
>> +	}
>> +
>> +	std::sort(R_v.begin(), R_v.end());
>> +	std::sort(Gr_v.begin(), Gr_v.end());
>> +	std::sort(B_v.begin(), B_v.end());
>> +	std::sort(Gb_v.begin(), Gb_v.end());
>> +
>> +	double Grmed = Gr_v[Gr_v.size() / 2];
>> +	double Rmed = R_v[R_v.size() / 2];
>> +	double Bmed = B_v[B_v.size() / 2];
>> +	double Gbmed = Gb_v[Gb_v.size() / 2];
>> +
>> +	double Rgain = Grmed / Rmed;
>> +	double Bgain = Gbmed / Bmed;
>> +	LOG(IPU3Awb, Debug) << "max R, Gr, B, Gb: "
>> +			    << R_v.back() << ","
>> +			    << Gr_v.back() << ","
>> +			    << B_v.back() << ","
>> +			    << Gb_v.back();
>> +	tint_ = ((Rmed / Grmed) + (Bmed / Gbmed)) / 2;
>> +
>> +	/* \todo Those are corrections when light is really low
>> +	 * it should be taken into account by AGC somehow */
>> +	if ((Rgain >= 2) && (Bgain < 2)) {
>> +		wbGains_[0] = 4096 * tint_;
>> +		wbGains_[1] = 8192 * Rgain;
>> +		wbGains_[2] = 4096 * Bgain;
>> +		wbGains_[3] = 4096 * tint_;
>> +	} else if ((Bgain >= 2) && (Rgain < 2)) {
>> +		wbGains_[0] = 4096 * tint_;
>> +		wbGains_[1] = 4096 * Rgain;
>> +		wbGains_[2] = 8192 * Bgain;
>> +		wbGains_[3] = 4096 * tint_;
>> +	} else {
>> +		wbGains_[0] = 8192 * tint_;
>> +		wbGains_[1] = 8192 * Rgain;
>> +		wbGains_[2] = 8192 * Bgain;
>> +		wbGains_[3] = 8192 * tint_;
>> +	}
>> +
>> +	frame_count_++;
>> +
>> +	cct_ = estimateCCT(Rmed, (Grmed + Gbmed) / 2, Bmed);
>> +
>> +	algoConverged_ = true;
>> +}
>> +
>> +void IPU3Awb::updateBNR(ipu3_uapi_params &params)
> 
> updateBNR() isn't a great name for a function that updates the WB gains,
> even if they're part of the Bayer noise reduction block of data.
> 
> Should we push the ipu3_uapi_params structure down to individual
> algorithms, or would it be better to decouple it, creating custom
> structures to hold algorithm results, and fillin ipu3_uapi_params in
> IPAIPU3::fillParams() instead ? On one hand moving ipu3_uapi_params out
> will make the code more generic, and thus more reusable, but on the
> other hand we would still depend on a custom stats format, so it may not
> be worth it.
> 
>> +{
>> +	if (!algoConverged_)
>> +		return;
>> +
>> +	params.acc_param.bnr.wb_gains.gr = wbGains_[0];
>> +	params.acc_param.bnr.wb_gains.r = wbGains_[1];
>> +	params.acc_param.bnr.wb_gains.b = wbGains_[2];
>> +	params.acc_param.bnr.wb_gains.gb = wbGains_[3];
>> +	if (cct_ < 5500)
>> +		params.acc_param.ccm = imgu_css_ccm_3800k;
>> +	else
>> +		params.acc_param.ccm = imgu_css_ccm_6000k;
>> +}
>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
>> new file mode 100644
>> index 00000000..06389020
>> --- /dev/null
>> +++ b/src/ipa/ipu3/ipu3_awb.h
>> @@ -0,0 +1,130 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>> + *
>> + * awb.h - AWB control algorithm
>> + */
>> +#ifndef __LIBCAMERA_IPU3_AWB_H__
>> +#define __LIBCAMERA_IPU3_AWB_H__
>> +
>> +#include <linux/intel-ipu3.h>
>> +
>> +#include <libcamera/geometry.h>
>> +
>> +#include <libcamera/ipa/awb_algorithm.h>
>> +#include <libcamera/ipa/ipa_controller.h>
>> +
>> +namespace libcamera {
>> +
>> +const struct ipu3_uapi_bnr_static_config imgu_css_bnr_defaults = {
>> +	{ 16, 16, 16, 16 },			/* wb_gains */
>> +	{ 255, 255, 255, 255 },			/* wb_gains_thr */
>> +	{ 0, 0, 8, 6, 0, 14 },			/* thr_coeffs */
>> +	{ 0, 0, 0, 0 },				/* thr_ctrl_shd */
>> +	{ -648, 0, -366, 0 },			/* opt_center */
>> +	{					/* 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 }
>> +	},
>> +	{ 4, 0, 1, 8, 0, 8, 0, 8, 0 },		/* bp_ctrl */
>> +	{ 8, 4, 4, 0, 8, 0, 1, 1, 1, 1, 0 },	/* dn_detect_ctrl */
>> +	1296,
>> +	{ 419904, 133956 },
>> +};
>> +
>> +/* settings for Auto White Balance */
>> +const struct ipu3_uapi_awb_config_s imgu_css_awb_defaults = {
>> +	8191,
>> +	8191,
>> +	8191,
>> +	8191 | /* rgbs_thr_gr/r/gb/b */
>> +		IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
>> +	.grid = {
>> +		.width = 160,
>> +		.height = 45,
>> +		.block_width_log2 = 3,
>> +		.block_height_log2 = 4,
>> +		.x_start = 0,
>> +		.y_start = 0,
>> +	},
>> +};
>> +
>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_6000k = {
>> +	7239, -750, -37, 0,
>> +	-215, 9176, -200, 0,
>> +	-70, -589, 6810, 0
>> +};
>> +
>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_4900k = {
>> +	7811, -464, -466, 0,
>> +	-635, 8762, -533, 0,
>> +	-469, -154, 6583, 0
>> +};
>> +
>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_3800k = {
>> +	7379, -526, -296, 0,
>> +	-411, 7397, -415, 0,
>> +	-224, -564, 7244, 0
>> +};
> 
> All these should be moved to ipu3_awb.cpp, and be made static. Otherwise
> they would get duplicated in every compilation unit in which this header
> is included.
> 
>> +
>> +struct AwbConfig {
>> +	// Only repeat the AWB calculation every "this many" frames
>> +	uint16_t frame_period;
>> +	// number of initial frames for which speed taken as 1.0 (maximum)
>> +	uint16_t startup_frames;
>> +	unsigned int convergence_frames; // approx number of frames to converge
>> +	double speed; // IIR filter speed applied to algorithm results
>> +};
>> +
>> +#if 0
>> +typedef struct awb_public_set_item{
>> +    unsigned char Gr_avg;
>> +    unsigned char R_avg;
>> +    unsigned char B_avg;
>> +    unsigned char Gb_avg;
>> +    unsigned char sat_ratio;
>> +    unsigned char padding0; /**< Added the padding so that the public matches that private */
>> +    unsigned char padding1; /**< Added the padding so that the public matches that private */
>> +    unsigned char padding2; /**< Added the padding so that the public matches that private */
>> +} awb_public_set_item_t;
>> +#endif
> 
> If the compiler doesn't need to see this, I think you can drop it
> completely :-)
> 
>> +
>> +class IPU3Awb : public AwbAlgorithm
>> +{
>> +public:
>> +	IPU3Awb(IPAController *controller = NULL);
> 
> This is C++, so nullptr :-) But there's no need for a default value, as
> the controller parameter should either be mandatory, or removed
> completely.
> 
>> +	~IPU3Awb();
> 
> Blank line.
> 
>> +	virtual char const *Name() const override;
> 
> This is never used, you can drop it from the base class. When (and if)
> we'll need this, I'd rather pass the name to the base class constructor,
> store it there, and implement name() in the base class only, as a
> non-virtual function.
> 
>> +	virtual void Initialise() override;
> 
> No need to repeat the virtual keyword for overridden functions.
> 
>> +	void Initialise(ipu3_uapi_params &params);
> 
> This isn't very nice, as it's not aligned with the design of the
> IPAAlgorithm class, but we can fix it later. I would move the function
> down, after the mandatory functions, with calculateWBGains() and
> updateBNR().
> 
>> +	unsigned int GetConvergenceFrames() const override;
>> +	void SetMode(std::string const &name) override;
>> +	void SetManualGains(double manual_r, double manual_b) override;
> 
> A blank line here would be nice.
> 
>> +	void calculateWBGains(Rectangle roi,
> 
> const Rectangle &roi
> 
>> +			      const ipu3_uapi_stats_3a *stats);
>> +	void updateBNR(ipu3_uapi_params &params);
>> +
>> +private:
>> +	uint32_t estimateCCT(uint8_t R, uint8_t G, uint8_t B);
> 
> r, g and b.
> 
>> +
>> +	/* configuration is read-only, and available to both threads */
> 
> What threads ? :-)
> 
>> +	AwbConfig config_;
>> +	std::string mode_name_;
> 
> Never used.
> 
>> +	/* manual r setting */
>> +	double manual_r_;
> 
> manualRedGain_ ?
> 
>> +	/* manual b setting */
>> +	double manual_b_;
>> +	/* WB calculated gains */
>> +	uint16_t wbGains_[4];
>> +	double tint_;
>> +	uint32_t cct_;
>> +
>> +	uint32_t frame_count_;
>> +
>> +	bool algoConverged_;
>> +};
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_IPU3_AWB_H__ */
>> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
>> index a241f617..43ad0e0d 100644
>> --- a/src/ipa/ipu3/meson.build
>> +++ b/src/ipa/ipu3/meson.build
>> @@ -2,8 +2,14 @@
>>   
>>   ipa_name = 'ipa_ipu3'
>>   
>> +ipu3_ipa_sources = files([
>> +  'ipu3.cpp',
>> +  'ipu3_agc.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,
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index ff980b38..3809c943 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -825,6 +825,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>>   	 */
>>   	double lineDuration = sensorInfo.lineLength
>>   			    / (sensorInfo.pixelRate / 1e6);
>> +	LOG(IPU3, Error) << "line duration: " << lineDuration;
> 
> Is this an error ? :-)
> 
>>   	const ControlInfoMap &sensorControls = sensor->controls();
>>   	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
>>   	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
>

Hi Dafna,

On 22/02/2021 19:50, Dafna Hirschfeld wrote:
> Hi,
> 
> On 22.02.21 05:56, Laurent Pinchart wrote:
>> Hi Jean-Michel,
>>
>> Thank you for the patch.
>>
>> On Fri, Feb 19, 2021 at 06:22:24PM +0100, Jean-Michel Hautbois wrote:
>>> Inherit from the AWBAlgorithm and AGCAlgorithm classes to implement
>>> basic functions.
>>> While exposure is not locked, AWB is not calculated and corrected.
>>> Once AWB is done, a color temperature is estimated and default CCM
>>> matrices
>>> are used.
>>> Implement a basic "grey-world" AWB algorithm just for demonstration
>>> purpose.
> 
> Are those algorithms specific to IPU3 ?
> If not then maybe it can used by rkisp1 as well?
> Maybe it worth to think of general API for basic 3a implementations that
> other IPAs can use?

Not at all ! And I will send a second patch version which will show how
to implement the image processing algorithm, and indeed it aims to be
platform agnostic (even if the parameters and statistics are always
specific ;-)) !

>>
>> Nice to see an initial implementation ! I'll provide a handful of review
>> comments below already. Some are related to coding style or similar
>> issues and should be addressed in v2. Others are related to the IPA
>> architecture, and could be fixed on top of this series to avoid delaying
>> it unnecessarily. They should in that case be captured in a \todo
>> comment.
>>
>> As for 1/2, I won't repeat the same comments all over the patch, so
>> please apply them wherever they're applicable.
>>
>>> Signed-off-by: Jean-Michel Hautbois
>>> <jeanmichel.hautbois@ideasonboard.com>
>>> ---
>>>   src/ipa/ipu3/ipu3.cpp                |  31 ++++-
>>>   src/ipa/ipu3/ipu3_agc.cpp            | 195 +++++++++++++++++++++++++++
>>>   src/ipa/ipu3/ipu3_agc.h              |  96 +++++++++++++
>>>   src/ipa/ipu3/ipu3_awb.cpp            | 182 +++++++++++++++++++++++++
>>>   src/ipa/ipu3/ipu3_awb.h              | 130 ++++++++++++++++++
>>>   src/ipa/ipu3/meson.build             |   8 +-
>>>   src/libcamera/pipeline/ipu3/ipu3.cpp |   1 +
>>>   7 files changed, 638 insertions(+), 5 deletions(-)
>>>   create mode 100644 src/ipa/ipu3/ipu3_agc.cpp
>>>   create mode 100644 src/ipa/ipu3/ipu3_agc.h
>>>   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 b63e58be..c3859f39 100644
>>> --- a/src/ipa/ipu3/ipu3.cpp
>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>> @@ -13,6 +13,7 @@
>>>     #include <libcamera/buffer.h>
>>>   #include <libcamera/control_ids.h>
>>> +#include <libcamera/ipa/ipa_controller.h>
>>>   #include <libcamera/ipa/ipa_interface.h>
>>>   #include <libcamera/ipa/ipa_module_info.h>
>>>   #include <libcamera/ipa/ipu3_ipa_interface.h>
>>> @@ -21,6 +22,9 @@
>>>   #include "libcamera/internal/buffer.h"
>>>   #include "libcamera/internal/log.h"
>>>   +#include "ipu3_agc.h"
>>> +#include "ipu3_awb.h"
>>> +
>>>   namespace libcamera {
>>>     LOG_DEFINE_CATEGORY(IPAIPU3)
>>> @@ -28,6 +32,9 @@ LOG_DEFINE_CATEGORY(IPAIPU3)
>>>   class IPAIPU3 : public ipa::ipu3::IPAIPU3Interface
>>>   {
>>>   public:
>>> +    IPAIPU3()
>>> +        : controller_() {}
>>
>>         : controller_()
>>     {
>>     }
>>
>> I may not repeat all the comments from 1/2, but they're generally
>> applicable to this patch as well.
>>
>>> +
>>>       int init([[maybe_unused]] const IPASettings &settings) override
>>>       {
>>>           return 0;
>>> @@ -60,6 +67,11 @@ private:
>>>       uint32_t gain_;
>>>       uint32_t minGain_;
>>>       uint32_t maxGain_;
>>> +
>>> +    IPAController controller_;
>>> +    IPU3Awb *awbAlgo_;
>>> +    IPU3Agc *agcAlgo_;
>>> +    ipu3_uapi_params params_;
>>>   };
>>>     void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap>
>>> &entityControls)
>>> @@ -83,11 +95,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_ = 123;
>>
>> A peculiar value :-)
>>
>>>         minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>>>       maxGain_ = itGain->second.max().get<int32_t>();
>>> -    gain_ = maxGain_;
>>> +    gain_ = 1;
>>> +
>>> +    awbAlgo_ = new IPU3Awb(&controller_);
>>> +    awbAlgo_->Initialise(params_);
>>> +    agcAlgo_ = new IPU3Agc(&controller_);
>>> +
>>> +    /*\todo not used yet... */
>>> +    controller_.Initialise();
>>>         setControls(0);
>>>   }
>>> @@ -162,10 +181,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));
>>> +    awbAlgo_->updateBNR(params_);
>>> +    memcpy(params, &params_, sizeof(*params));
>>
>>     *params = params_;
>>
>>>         /* \todo Fill in parameters buffer. */
>>> -
>>>       ipa::ipu3::IPU3Action op;
>>>       op.op = ipa::ipu3::ActionParamFilled;
>>>   @@ -179,6 +198,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>>         /* \todo React to statistics and update internal state
>>> machine. */
>>>       /* \todo Add meta-data information to ctrls. */
>>> +    agcAlgo_->Process(stats, exposure_, gain_);
>>> +    if (agcAlgo_->Converged())
>>> +        awbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400),
>>> stats);
>>> +    setControls(frame);
>>>         ipa::ipu3::IPU3Action op;
>>>       op.op = ipa::ipu3::ActionMetadataReady;
>>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
>>> new file mode 100644
>>> index 00000000..db591e33
>>> --- /dev/null
>>> +++ b/src/ipa/ipu3/ipu3_agc.cpp
>>> @@ -0,0 +1,195 @@
>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>> +/*
>>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>>> + *
>>> + * agc.cpp - AGC/AEC control algorithm
>>> + */
>>> +#include <iostream>
>>> +#include <numeric>
>>> +#include <unordered_map>
>>> +
>>> +#include "libcamera/internal/log.h"
>>> +
>>> +#include "ipu3_agc.h"
>>> +
>>> +using namespace libcamera;
>>> +
>>> +LOG_DEFINE_CATEGORY(IPU3Agc)
>>> +
>>> +#define NAME "ipu3.agc"
>>> +
>>> +IPU3Agc::IPU3Agc(IPAController *controller)
>>> +    : AgcAlgorithm(controller), frameCount_(0),
>>> +      ev_(1.0), flicker_period_(0.0),
>>> +      max_shutter_(0), fixed_shutter_(0),
>>> +      fixed_analogue_gain_(0.0), algoConverged_(false)
>>> +{
>>> +}
>>> +
>>> +char const *IPU3Agc::Name() const
>>> +{
>>> +    return NAME;
>>> +}
>>> +
>>> +unsigned int IPU3Agc::GetConvergenceFrames() const
>>> +{
>>> +    return config_.convergence_frames;
>>> +}
>>> +
>>> +void IPU3Agc::SetEv(double ev)
>>> +{
>>> +    ev_ = ev;
>>> +}
>>> +
>>> +void IPU3Agc::SetFlickerPeriod(double flicker_period)
>>> +{
>>> +    flicker_period_ = flicker_period;
>>> +}
>>> +
>>> +void IPU3Agc::SetMaxShutter(double max_shutter)
>>> +{
>>> +    max_shutter_ = max_shutter;
>>> +}
>>> +
>>> +void IPU3Agc::SetFixedShutter(double fixed_shutter)
>>> +{
>>> +    fixed_shutter_ = fixed_shutter;
>>> +}
>>> +
>>> +void IPU3Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
>>> +{
>>> +    fixed_analogue_gain_ = fixed_analogue_gain;
>>> +}
>>> +
>>> +void IPU3Agc::SetMeteringMode(std::string const &metering_mode_name)
>>> +{
>>> +    metering_mode_name_ = metering_mode_name;
>>> +}
>>> +
>>> +void IPU3Agc::SetExposureMode(std::string const &exposure_mode_name)
>>> +{
>>> +    exposure_mode_name_ = exposure_mode_name;
>>> +}
>>> +
>>> +void IPU3Agc::Prepare() {}
>>> +
>>> +void IPU3Agc::Process() {}
>>> +
>>> +/* \todo This function is taken from numerical recipes and
>>> calculates all moments
>>> + * It needs to be rewritten properly and maybe in a "math" class ? */
>>
>> /*
>>   * \todo This function is taken from numerical recipes and calculates
>> all
>>   * moments. It needs to be rewritten properly and maybe in a "math"
>> class ?
>>   */
>>
>>> +void IPU3Agc::moments(std::unordered_map<uint32_t, uint32_t> &data,
>>> int n)
> 
> 'data' is the hist, so why not call it 'hist' here?

Yes, why not :-) thanks !

>>> +{
>>> +    int j;
>>> +    double ep = 0.0, s, p;
>>> +    double ave, adev, sdev;
>>> +    double var, skew, curt;
>>> +
>>> +    s = 0.0;
>>> +    for (j = 1; j <= n; j++)
>>> +        s += data[j];
> 
> shouldn't that be:
> s += data[j] * j
> 
> since data[j] is the number of pixels with value j

I don't think so, no, but I will have to verify...

>>> +
>>> +    ave = s / n;
>>> +    adev = var = skew = curt = 0.0;
>>> +
>>> +    for (j = 1; j <= n; j++) {
>>> +        adev += s = data[j] - (ave);
>>> +        ep += s;
>>> +        var += (p = s * s);
>>> +        skew += (p *= s);
>>> +        curt += (p *= s);
>>> +    }
>>> +
>>> +    adev /= n;
>>> +    var = (var - ep * ep / n) / (n - 1);
>>> +    sdev = std::sqrt(var);
>>> +
>>> +    if (var) {
>>> +        skew /= n * var * sdev;
>>> +        curt = curt / (n * var * var) - 3.0;
>>> +    }
>>> +    skew_ = skew;
>>> +}
>>> +
>>> +void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>>> +{
>>> +    brightnessVec_.clear();
>>> +
>>> +    /*\todo Replace constant values with real BDS configuration */
>>> +    for (uint32_t j = 0; j < 45; j++) {
>>> +        for (uint32_t i = 0; i < 160 * 45 * 8; i += 8) {
>>> +            uint8_t Gr = stats->awb_raw_buffer.meta_data[i];
>>> +            uint8_t R = stats->awb_raw_buffer.meta_data[i + 1];
>>> +            uint8_t B = stats->awb_raw_buffer.meta_data[i + 2];
>>> +            uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3];
>>> +            brightnessVec_.push_back(static_cast<uint32_t>(0.299 * R
>>> + 0.587 * (Gr + Gb) / 2 + 0.114 * B));
> 
> You iterate on j in the outer loop but don't use j anywhere for
> indexing, this is a bit suspicious

This is just for an easier splitting of rectangles (taken from the AWB
algorithm).

>>> +        }
>>> +    }
>>> +    std::sort(brightnessVec_.begin(), brightnessVec_.end());
> 
> I don't think the sort is is necessary

It is there from an old calculation of median value. Not needed indeed.

>>> +
>>> +    /* \todo create a class to generate histograms ! */
>>> +    std::unordered_map<uint32_t, uint32_t> hist;
> 
> you want to have up to 256 indexes so you need to define with uint8_t
> 
> std::unordered_map<uint8_t, uint32_t> hist;

ack

> 
>>> +    for (uint32_t const &val : brightnessVec_)
>>> +        hist[val]++;
>>> +    moments(hist, 256);
>>> +}
>>> +
>>> +void IPU3Agc::lockExposure(uint32_t &exposure, uint32_t &gain)
>>> +{
>>> +    /* Algorithm initialization wait for first valid frames */
>>> +    /* \todo - have a number of frames given by DelayedControls ?
>>> +     * - implement a function for IIR */
>>> +    if (frameCount_ == 4) {
>>> +        prevExposure_ = exposure;
>>> +
>>> +        prevSkew_ = skew_;
>>> +        /* \tdo use configured values */
>>
>> s/tdo/todo/
>>
>>> +        exposure = 800;
>>> +        gain = 8;
>>> +        currentExposure_ = exposure;
>>> +    } else if (frameCount_ == 8) {
>>> +        currentSkew_ = skew_;
>>> +        exposure = ((currentExposure_ * prevSkew_) + (prevExposure_
>>> * currentSkew_)) / (prevSkew_ + currentSkew_);
>>
>> Line wrap. No need for the inner parentheses.
>>
>>> +        nextExposure_ = exposure;
>>> +        lastFrame_ = frameCount_;
>>> +    } else if ((frameCount_ >= 12) && (frameCount_ - lastFrame_ >=
>>> 4)) {
>>
>> Could you define constants for magic values, to make the code more
>> readable ? They can be defined as static constexpr members of the
>> IPU3Agc class, and should be named with camelCase. While we don't do so
>> yet in many places, it's customary to start them with a 'k' prefix (e.g.
>> kInitialFrameSkipCount).
>>
>>> +        currentSkew_ = skew_;
>>> +        /* \todo properly calculate a gain */
>>> +        if (frameCount_ == 12)
>>> +            gain = ((8 * prevSkew_) + (1 * currentSkew_)) /
>>> (prevSkew_ + currentSkew_);
>>> +
>>> +        if (currentSkew_ - prevSkew_ > 1) {
>>> +            /* under exposed */
>>> +            prevExposure_ = nextExposure_;
>>> +            exposure = ((currentExposure_ * prevSkew_) +
>>> (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
>>> +            nextExposure_ = exposure;
>>> +        } else if (currentSkew_ - prevSkew_ < -1) {
>>> +            /* over exposed */
>>> +            currentExposure_ = nextExposure_;
>>> +            exposure = ((currentExposure_ * prevSkew_) +
>>> (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
>>> +            nextExposure_ = exposure;
>>> +        } else {
>>> +            /* we have converged */
>>> +            algoConverged_ = true;
>>> +        }
>>> +        lastFrame_ = frameCount_;
>>> +        prevSkew_ = currentSkew_;
>>> +    }
> 
> Could you explain the general idea behind this algorithm or give a link
> where it is explained?

Yes, I will, and this algorithm will evolve as I want to use Q3-Q1
inter-quartile measure to optimize auto exposure.

> Thanks,
> Dafna
> 
>>> +}
>>> +
>>> +void IPU3Agc::Process(const ipu3_uapi_stats_3a *stats, uint32_t
>>> &exposure, uint32_t &gain)
>>> +{
>>> +    processBrightness(stats);
>>> +    if (!algoConverged_)
>>> +        lockExposure(exposure, gain);
>>> +    else {
>>> +        /* Are we still well exposed ? */
>>> +        if ((skew_ < 2) || (skew_ > 4))
>>> +            algoConverged_ = false;
>>> +    }
>>> +    frameCount_++;
>>> +}
>>> +
>>> +bool IPU3Agc::Converged()
>>> +{
>>> +    return algoConverged_;
>>> +}
>>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
>>> new file mode 100644
>>> index 00000000..9a69d628
>>> --- /dev/null
>>> +++ b/src/ipa/ipu3/ipu3_agc.h
>>> @@ -0,0 +1,96 @@
>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>> +/*
>>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>>> + *
>>> + * ipu3_agc.h - AGC/AEC control algorithm
>>> + */
>>> +#ifndef __LIBCAMERA_IPU3_AGC_H__
>>> +#define __LIBCAMERA_IPU3_AGC_H__
>>> +
>>
>> #include <map>
>>
>>> +#include <mutex>
>>
>> This doesn't seem to be needed.
>>
>>> +#include <vector>
>>> +
>>> +#include <linux/intel-ipu3.h>
>>> +
>>> +#include <libcamera/geometry.h>
>>> +
>>> +#include <libcamera/ipa/agc_algorithm.h>
>>> +#include <libcamera/ipa/ipa_controller.h>
>>> +
>>> +#define AGC_STATS_SIZE 15
>>> +
>>> +namespace libcamera {
>>> +
>>> +struct AgcMeteringMode {
>>> +    double weights[AGC_STATS_SIZE];
>>> +    Rectangle metering_region[AGC_STATS_SIZE];
>>> +};
>>> +
>>> +struct AgcExposureMode {
>>> +    std::vector<double> shutter;
>>> +    std::vector<double> gain;
>>> +};
>>> +
>>> +struct AgcConfig {
>>> +    std::map<std::string, AgcMeteringMode> metering_modes;
>>> +    std::map<std::string, AgcExposureMode> exposure_modes;
>>> +    double speed;
>>> +    uint16_t startup_frames;
>>> +    unsigned int convergence_frames;
>>> +    std::string default_metering_mode;
>>> +    std::string default_exposure_mode;
>>> +    double default_exposure_time;
>>> +    double default_analogue_gain;
>>> +};
>>> +
>>> +class IPU3Agc : public AgcAlgorithm
>>> +{
>>> +public:
>>> +    IPU3Agc(IPAController *IPAcontroller);
>>
>> Blank line.
>>
>>> +    char const *Name() const override;
>>> +    unsigned int GetConvergenceFrames() const override;
>>> +    void SetEv(double ev) override;
>>> +    void SetFlickerPeriod(double flicker_period) override;
>>> +    void SetMaxShutter(double max_shutter) override; // microseconds
>>> +    void SetFixedShutter(double fixed_shutter) override; //
>>> microseconds
>>> +    void SetFixedAnalogueGain(double fixed_analogue_gain) override;
>>> +    void SetMeteringMode(std::string const &metering_mode_name)
>>> override;
>>> +    void SetExposureMode(std::string const &exposure_mode_name)
>>> override;
>>> +    void Prepare() override;
>>> +    void Process() override;
>>> +    void Process(const ipu3_uapi_stats_3a *stats, uint32_t
>>> &exposure, uint32_t &gain);
>>
>> The last two arguments should be replaced by a generic data container,
>> similar to the Metadata class in the RPi IPA (not something to fix now).
>>
>>> +    bool Converged();
>>> +
>>> +private:
>>> +    void moments(std::unordered_map<uint32_t, uint32_t> &data, int n);
>>> +    void processBrightness(const ipu3_uapi_stats_3a *stats);
>>> +    void lockExposure(uint32_t &exposure, uint32_t &gain);
>>> +
>>> +    AgcConfig config_;
>>
>> This is never set, and only one member is read. I'd drop it for now.
>>
>>> +    std::string metering_mode_name_;
>>> +    std::string exposure_mode_name_;
>>> +    uint64_t frameCount_;
>>> +    uint64_t lastFrame_;
>>> +
>>> +    /* Vector of calculated brightness for each cell */
>>> +    std::vector<uint32_t> brightnessVec_;
>>
>> We avoid encoding the type in the variable name. Would this be better
>> named cellsBrightness_ ?
>>
>>> +    double ev_;
>>
>> Never used. Same for other members below.
>>
>>> +    double flicker_period_;
>>> +    double max_shutter_;
>>> +    double fixed_shutter_;
>>> +    double fixed_analogue_gain_;
>>> +
>>> +    /* Values for filtering */
>>> +    uint32_t prevExposure_;
>>> +    uint32_t currentExposure_;
>>> +    uint32_t nextExposure_;
>>> +
>>> +    double skew_;
>>> +    double prevSkew_;
>>> +    double currentSkew_;
>>> +    bool algoConverged_;
>>
>> Maybe just converged_ ?
>>
>>> +};
>>> +
>>> +} /* namespace libcamera */
>>> +
>>> +#endif /* __LIBCAMERA_IPU3_AGC_H__ */
>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
>>> new file mode 100644
>>> index 00000000..21286e4c
>>> --- /dev/null
>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
>>> @@ -0,0 +1,182 @@
>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>> +/*
>>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>>> + *
>>> + * ipu3_awb.cpp - AWB control algorithm
>>> + */
>>> +#include <iostream>
>>
>> I don't think this is needed.
>>
>>> +#include <numeric>
>>> +#include <unordered_map>
>>> +
>>> +#include "libcamera/internal/log.h"
>>> +
>>> +#include "ipu3_awb.h"
>>> +
>>> +using namespace libcamera;
>>> +
>>> +LOG_DEFINE_CATEGORY(IPU3Awb)
>>> +
>>> +#define NAME "ipu3.awb"
>>> +
>>> +IPU3Awb::IPU3Awb(IPAController *controller)
>>> +    : AwbAlgorithm(controller)
>>> +{
>>> +}
>>> +
>>> +IPU3Awb::~IPU3Awb()
>>> +{
>>> +}
>>> +
>>> +char const *IPU3Awb::Name() const
>>> +{
>>> +    return NAME;
>>> +}
>>> +
>>> +void IPU3Awb::Initialise() {}
>>
>> void IPU3Awb::Initialise()
>> {
>> }
>>
>>> +
>>> +void IPU3Awb::Initialise(ipu3_uapi_params &params)
>>> +{
>>> +    memset(&params, 0, sizeof(params));
>>> +    memset(wbGains_, 8192, sizeof(wbGains_));
>>
>> memset() initializes the memory byte by byte, this will not store 8192
>> in each entry of the array.
>>
>>
>>> +    wbGains_[0] = 8192 * 0.8;
>>> +    wbGains_[3] = 8192 * 0.8;
>>
>> I would group the initialization of member variables together, either
>> before, or after the initialization of params.
>>
>>> +    params.use.acc_awb = 1;
>>> +    /*\todo fill the grid calculated based on BDS configuration */
>>> +    params.acc_param.awb.config = imgu_css_awb_defaults;
>>> +
>>> +    params.use.acc_bnr = 1;
>>> +    params.acc_param.bnr = imgu_css_bnr_defaults;
>>> +
>>> +    params.use.acc_ccm = 1;
>>> +    params.acc_param.ccm = imgu_css_ccm_3800k;
>>> +
>>> +    params.use.acc_gamma = 1;
>>> +    params.acc_param.gamma.gc_ctrl.enable = 1;
>>> +
>>> +    uint32_t a = (32 * 245) / (245 - 9);
>>> +
>>> +    for (uint32_t i = 0; i < 10; i++)
>>> +        params.acc_param.gamma.gc_lut.lut[i] = 0;
>>> +    for (uint32_t i = 10; i < 245; i++)
>>> +        params.acc_param.gamma.gc_lut.lut[i] = a * i + (0 - a * 9);
>>> +    for (uint32_t i = 245; i < 255; i++)
>>> +        params.acc_param.gamma.gc_lut.lut[i] = 32 * 245;
>>> +
>>> +    frame_count_ = 0;
>>> +    algoConverged_ = false;
>>> +}
>>> +
>>> +unsigned int IPU3Awb::GetConvergenceFrames() const
>>> +{
>>> +    // If colour gains have been explicitly set, there is no
>>> convergence
>>> +    // to happen, so no need to drop any frames - return zero.
>>> +    if (manual_r_ && manual_b_)
>>> +        return 0;
>>> +    else
>>> +        return config_.convergence_frames;
>>> +}
>>> +
>>> +void IPU3Awb::SetMode(std::string const &mode_name)
>>> +{
>>> +    mode_name_ = mode_name;
>>> +}
>>> +
>>> +void IPU3Awb::SetManualGains(double manual_r, double manual_b)
>>> +{
>>> +    // If any of these are 0.0, we swich back to auto.
>>> +    manual_r_ = manual_r;
>>> +    manual_b_ = manual_b;
>>> +}
>>> +
>>> +uint32_t IPU3Awb::estimateCCT(uint8_t R, uint8_t G, uint8_t B)
>>> +{
>>> +    double X = (-0.14282) * (R) + (1.54924) * (G) + (-0.95641) * (B);
>>> +    double Y = (-0.32466) * (R) + (1.57837) * (G) + (-0.73191) * (B);
>>> +    double Z = (-0.68202) * (R) + (0.77073) * (G) + (0.56332) * (B);
>>> +
>>> +    double x = X / (X + Y + Z);
>>> +    double y = Y / (X + Y + Z);
>>> +
>>> +    double n = (x - 0.3320) / (0.1858 - y);
>>> +    return static_cast<uint32_t>(449 * n * n * n + 3525 * n * n +
>>> 6823.3 * n + 5520.33);
>>> +}
>>> +
>>> +void IPU3Awb::calculateWBGains([[maybe_unused]] Rectangle roi, const
>>> ipu3_uapi_stats_3a *stats)
>>
>> Line wrap.
>>
>>> +{
>>> +    if (algoConverged_)
>>> +        return;
>>
>> So once it converges, you never update the gains anymore, even if the
>> scene changes ?
>>
>>> +
>>> +    std::vector<uint32_t> R_v, Gr_v, Gb_v, B_v;
>>
>> camelCase. Feel free to also make the name slightly longer to make the
>> more explicit. redValues ?
>>
>>> +    Point topleft = roi.topLeft();
>>> +    uint32_t startY = (topleft.y / 16) * 160 * 8;
>>> +    uint32_t startX = (topleft.x / 8) * 8;
>>> +    uint32_t endX = (startX + (roi.size().width / 8)) * 8;
>>> +
>>> +    for (uint32_t j = (topleft.y / 16); j < (topleft.y / 16) +
>>> (roi.size().height / 16); j++) {
>>> +        for (uint32_t i = startX + startY; i < endX + startY; i += 8) {
>>> +            Gr_v.push_back(stats->awb_raw_buffer.meta_data[i]);
>>> +            R_v.push_back(stats->awb_raw_buffer.meta_data[i + 1]);
>>> +            B_v.push_back(stats->awb_raw_buffer.meta_data[i + 2]);
>>> +            Gb_v.push_back(stats->awb_raw_buffer.meta_data[i + 3]);
>>> +        }
>>> +    }
>>> +
>>> +    std::sort(R_v.begin(), R_v.end());
>>> +    std::sort(Gr_v.begin(), Gr_v.end());
>>> +    std::sort(B_v.begin(), B_v.end());
>>> +    std::sort(Gb_v.begin(), Gb_v.end());
>>> +
>>> +    double Grmed = Gr_v[Gr_v.size() / 2];
>>> +    double Rmed = R_v[R_v.size() / 2];
>>> +    double Bmed = B_v[B_v.size() / 2];
>>> +    double Gbmed = Gb_v[Gb_v.size() / 2];
>>> +
>>> +    double Rgain = Grmed / Rmed;
>>> +    double Bgain = Gbmed / Bmed;
>>> +    LOG(IPU3Awb, Debug) << "max R, Gr, B, Gb: "
>>> +                << R_v.back() << ","
>>> +                << Gr_v.back() << ","
>>> +                << B_v.back() << ","
>>> +                << Gb_v.back();
>>> +    tint_ = ((Rmed / Grmed) + (Bmed / Gbmed)) / 2;
>>> +
>>> +    /* \todo Those are corrections when light is really low
>>> +     * it should be taken into account by AGC somehow */
>>> +    if ((Rgain >= 2) && (Bgain < 2)) {
>>> +        wbGains_[0] = 4096 * tint_;
>>> +        wbGains_[1] = 8192 * Rgain;
>>> +        wbGains_[2] = 4096 * Bgain;
>>> +        wbGains_[3] = 4096 * tint_;
>>> +    } else if ((Bgain >= 2) && (Rgain < 2)) {
>>> +        wbGains_[0] = 4096 * tint_;
>>> +        wbGains_[1] = 4096 * Rgain;
>>> +        wbGains_[2] = 8192 * Bgain;
>>> +        wbGains_[3] = 4096 * tint_;
>>> +    } else {
>>> +        wbGains_[0] = 8192 * tint_;
>>> +        wbGains_[1] = 8192 * Rgain;
>>> +        wbGains_[2] = 8192 * Bgain;
>>> +        wbGains_[3] = 8192 * tint_;
>>> +    }
>>> +
>>> +    frame_count_++;
>>> +
>>> +    cct_ = estimateCCT(Rmed, (Grmed + Gbmed) / 2, Bmed);
>>> +
>>> +    algoConverged_ = true;
>>> +}
>>> +
>>> +void IPU3Awb::updateBNR(ipu3_uapi_params &params)
>>
>> updateBNR() isn't a great name for a function that updates the WB gains,
>> even if they're part of the Bayer noise reduction block of data.
>>
>> Should we push the ipu3_uapi_params structure down to individual
>> algorithms, or would it be better to decouple it, creating custom
>> structures to hold algorithm results, and fillin ipu3_uapi_params in
>> IPAIPU3::fillParams() instead ? On one hand moving ipu3_uapi_params out
>> will make the code more generic, and thus more reusable, but on the
>> other hand we would still depend on a custom stats format, so it may not
>> be worth it.
>>
>>> +{
>>> +    if (!algoConverged_)
>>> +        return;
>>> +
>>> +    params.acc_param.bnr.wb_gains.gr = wbGains_[0];
>>> +    params.acc_param.bnr.wb_gains.r = wbGains_[1];
>>> +    params.acc_param.bnr.wb_gains.b = wbGains_[2];
>>> +    params.acc_param.bnr.wb_gains.gb = wbGains_[3];
>>> +    if (cct_ < 5500)
>>> +        params.acc_param.ccm = imgu_css_ccm_3800k;
>>> +    else
>>> +        params.acc_param.ccm = imgu_css_ccm_6000k;
>>> +}
>>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
>>> new file mode 100644
>>> index 00000000..06389020
>>> --- /dev/null
>>> +++ b/src/ipa/ipu3/ipu3_awb.h
>>> @@ -0,0 +1,130 @@
>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>> +/*
>>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>>> + *
>>> + * awb.h - AWB control algorithm
>>> + */
>>> +#ifndef __LIBCAMERA_IPU3_AWB_H__
>>> +#define __LIBCAMERA_IPU3_AWB_H__
>>> +
>>> +#include <linux/intel-ipu3.h>
>>> +
>>> +#include <libcamera/geometry.h>
>>> +
>>> +#include <libcamera/ipa/awb_algorithm.h>
>>> +#include <libcamera/ipa/ipa_controller.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +const struct ipu3_uapi_bnr_static_config imgu_css_bnr_defaults = {
>>> +    { 16, 16, 16, 16 },            /* wb_gains */
>>> +    { 255, 255, 255, 255 },            /* wb_gains_thr */
>>> +    { 0, 0, 8, 6, 0, 14 },            /* thr_coeffs */
>>> +    { 0, 0, 0, 0 },                /* thr_ctrl_shd */
>>> +    { -648, 0, -366, 0 },            /* opt_center */
>>> +    {                    /* 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 }
>>> +    },
>>> +    { 4, 0, 1, 8, 0, 8, 0, 8, 0 },        /* bp_ctrl */
>>> +    { 8, 4, 4, 0, 8, 0, 1, 1, 1, 1, 0 },    /* dn_detect_ctrl */
>>> +    1296,
>>> +    { 419904, 133956 },
>>> +};
>>> +
>>> +/* settings for Auto White Balance */
>>> +const struct ipu3_uapi_awb_config_s imgu_css_awb_defaults = {
>>> +    8191,
>>> +    8191,
>>> +    8191,
>>> +    8191 | /* rgbs_thr_gr/r/gb/b */
>>> +        IPU3_UAPI_AWB_RGBS_THR_B_EN |
>>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
>>> +    .grid = {
>>> +        .width = 160,
>>> +        .height = 45,
>>> +        .block_width_log2 = 3,
>>> +        .block_height_log2 = 4,
>>> +        .x_start = 0,
>>> +        .y_start = 0,
>>> +    },
>>> +};
>>> +
>>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_6000k = {
>>> +    7239, -750, -37, 0,
>>> +    -215, 9176, -200, 0,
>>> +    -70, -589, 6810, 0
>>> +};
>>> +
>>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_4900k = {
>>> +    7811, -464, -466, 0,
>>> +    -635, 8762, -533, 0,
>>> +    -469, -154, 6583, 0
>>> +};
>>> +
>>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_3800k = {
>>> +    7379, -526, -296, 0,
>>> +    -411, 7397, -415, 0,
>>> +    -224, -564, 7244, 0
>>> +};
>>
>> All these should be moved to ipu3_awb.cpp, and be made static. Otherwise
>> they would get duplicated in every compilation unit in which this header
>> is included.
>>
>>> +
>>> +struct AwbConfig {
>>> +    // Only repeat the AWB calculation every "this many" frames
>>> +    uint16_t frame_period;
>>> +    // number of initial frames for which speed taken as 1.0 (maximum)
>>> +    uint16_t startup_frames;
>>> +    unsigned int convergence_frames; // approx number of frames to
>>> converge
>>> +    double speed; // IIR filter speed applied to algorithm results
>>> +};
>>> +
>>> +#if 0
>>> +typedef struct awb_public_set_item{
>>> +    unsigned char Gr_avg;
>>> +    unsigned char R_avg;
>>> +    unsigned char B_avg;
>>> +    unsigned char Gb_avg;
>>> +    unsigned char sat_ratio;
>>> +    unsigned char padding0; /**< Added the padding so that the
>>> public matches that private */
>>> +    unsigned char padding1; /**< Added the padding so that the
>>> public matches that private */
>>> +    unsigned char padding2; /**< Added the padding so that the
>>> public matches that private */
>>> +} awb_public_set_item_t;
>>> +#endif
>>
>> If the compiler doesn't need to see this, I think you can drop it
>> completely :-)
>>
>>> +
>>> +class IPU3Awb : public AwbAlgorithm
>>> +{
>>> +public:
>>> +    IPU3Awb(IPAController *controller = NULL);
>>
>> This is C++, so nullptr :-) But there's no need for a default value, as
>> the controller parameter should either be mandatory, or removed
>> completely.
>>
>>> +    ~IPU3Awb();
>>
>> Blank line.
>>
>>> +    virtual char const *Name() const override;
>>
>> This is never used, you can drop it from the base class. When (and if)
>> we'll need this, I'd rather pass the name to the base class constructor,
>> store it there, and implement name() in the base class only, as a
>> non-virtual function.
>>
>>> +    virtual void Initialise() override;
>>
>> No need to repeat the virtual keyword for overridden functions.
>>
>>> +    void Initialise(ipu3_uapi_params &params);
>>
>> This isn't very nice, as it's not aligned with the design of the
>> IPAAlgorithm class, but we can fix it later. I would move the function
>> down, after the mandatory functions, with calculateWBGains() and
>> updateBNR().
>>
>>> +    unsigned int GetConvergenceFrames() const override;
>>> +    void SetMode(std::string const &name) override;
>>> +    void SetManualGains(double manual_r, double manual_b) override;
>>
>> A blank line here would be nice.
>>
>>> +    void calculateWBGains(Rectangle roi,
>>
>> const Rectangle &roi
>>
>>> +                  const ipu3_uapi_stats_3a *stats);
>>> +    void updateBNR(ipu3_uapi_params &params);
>>> +
>>> +private:
>>> +    uint32_t estimateCCT(uint8_t R, uint8_t G, uint8_t B);
>>
>> r, g and b.
>>
>>> +
>>> +    /* configuration is read-only, and available to both threads */
>>
>> What threads ? :-)
>>
>>> +    AwbConfig config_;
>>> +    std::string mode_name_;
>>
>> Never used.
>>
>>> +    /* manual r setting */
>>> +    double manual_r_;
>>
>> manualRedGain_ ?
>>
>>> +    /* manual b setting */
>>> +    double manual_b_;
>>> +    /* WB calculated gains */
>>> +    uint16_t wbGains_[4];
>>> +    double tint_;
>>> +    uint32_t cct_;
>>> +
>>> +    uint32_t frame_count_;
>>> +
>>> +    bool algoConverged_;
>>> +};
>>> +} /* namespace libcamera */
>>> +
>>> +#endif /* __LIBCAMERA_IPU3_AWB_H__ */
>>> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
>>> index a241f617..43ad0e0d 100644
>>> --- a/src/ipa/ipu3/meson.build
>>> +++ b/src/ipa/ipu3/meson.build
>>> @@ -2,8 +2,14 @@
>>>     ipa_name = 'ipa_ipu3'
>>>   +ipu3_ipa_sources = files([
>>> +  'ipu3.cpp',
>>> +  'ipu3_agc.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,
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index ff980b38..3809c943 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -825,6 +825,7 @@ int
>>> PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>>>        */
>>>       double lineDuration = sensorInfo.lineLength
>>>                   / (sensorInfo.pixelRate / 1e6);
>>> +    LOG(IPU3, Error) << "line duration: " << lineDuration;
>>
>> Is this an error ? :-)
>>
>>>       const ControlInfoMap &sensorControls = sensor->controls();
>>>       const ControlInfo &v4l2Exposure =
>>> sensorControls.find(V4L2_CID_EXPOSURE)->second;
>>>       int32_t minExposure = v4l2Exposure.min().get<int32_t>() *
>>> lineDuration;
>>

Hi Laurent,

On 22/02/2021 05:56, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Fri, Feb 19, 2021 at 06:22:24PM +0100, Jean-Michel Hautbois wrote:
>> Inherit from the AWBAlgorithm and AGCAlgorithm classes to implement
>> basic functions.
>> While exposure is not locked, AWB is not calculated and corrected.
>> Once AWB is done, a color temperature is estimated and default CCM matrices
>> are used.
>> Implement a basic "grey-world" AWB algorithm just for demonstration purpose.
> 
> Nice to see an initial implementation ! I'll provide a handful of review
> comments below already. Some are related to coding style or similar
> issues and should be addressed in v2. Others are related to the IPA
> architecture, and could be fixed on top of this series to avoid delaying
> it unnecessarily. They should in that case be captured in a \todo
> comment.
> 
> As for 1/2, I won't repeat the same comments all over the patch, so
> please apply them wherever they're applicable.

Thanks, I only have a few questions :-).

>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> ---
>>  src/ipa/ipu3/ipu3.cpp                |  31 ++++-
>>  src/ipa/ipu3/ipu3_agc.cpp            | 195 +++++++++++++++++++++++++++
>>  src/ipa/ipu3/ipu3_agc.h              |  96 +++++++++++++
>>  src/ipa/ipu3/ipu3_awb.cpp            | 182 +++++++++++++++++++++++++
>>  src/ipa/ipu3/ipu3_awb.h              | 130 ++++++++++++++++++
>>  src/ipa/ipu3/meson.build             |   8 +-
>>  src/libcamera/pipeline/ipu3/ipu3.cpp |   1 +
>>  7 files changed, 638 insertions(+), 5 deletions(-)
>>  create mode 100644 src/ipa/ipu3/ipu3_agc.cpp
>>  create mode 100644 src/ipa/ipu3/ipu3_agc.h
>>  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 b63e58be..c3859f39 100644
>> --- a/src/ipa/ipu3/ipu3.cpp
>> +++ b/src/ipa/ipu3/ipu3.cpp
>> @@ -13,6 +13,7 @@
>>  
>>  #include <libcamera/buffer.h>
>>  #include <libcamera/control_ids.h>
>> +#include <libcamera/ipa/ipa_controller.h>
>>  #include <libcamera/ipa/ipa_interface.h>
>>  #include <libcamera/ipa/ipa_module_info.h>
>>  #include <libcamera/ipa/ipu3_ipa_interface.h>
>> @@ -21,6 +22,9 @@
>>  #include "libcamera/internal/buffer.h"
>>  #include "libcamera/internal/log.h"
>>  
>> +#include "ipu3_agc.h"
>> +#include "ipu3_awb.h"
>> +
>>  namespace libcamera {
>>  
>>  LOG_DEFINE_CATEGORY(IPAIPU3)
>> @@ -28,6 +32,9 @@ LOG_DEFINE_CATEGORY(IPAIPU3)
>>  class IPAIPU3 : public ipa::ipu3::IPAIPU3Interface
>>  {
>>  public:
>> +	IPAIPU3()
>> +		: controller_() {}
> 
> 		: controller_()
> 	{
> 	}
> 
> I may not repeat all the comments from 1/2, but they're generally
> applicable to this patch as well.
> 
>> +
>>  	int init([[maybe_unused]] const IPASettings &settings) override
>>  	{
>>  		return 0;
>> @@ -60,6 +67,11 @@ private:
>>  	uint32_t gain_;
>>  	uint32_t minGain_;
>>  	uint32_t maxGain_;
>> +
>> +	IPAController controller_;
>> +	IPU3Awb *awbAlgo_;
>> +	IPU3Agc *agcAlgo_;
>> +	ipu3_uapi_params params_;
>>  };
>>  
>>  void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls)
>> @@ -83,11 +95,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_ = 123;
> 
> A peculiar value :-)
> 
>>  
>>  	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>>  	maxGain_ = itGain->second.max().get<int32_t>();
>> -	gain_ = maxGain_;
>> +	gain_ = 1;
>> +
>> +	awbAlgo_ = new IPU3Awb(&controller_);
>> +	awbAlgo_->Initialise(params_);
>> +	agcAlgo_ = new IPU3Agc(&controller_);
>> +
>> +	/*\todo not used yet... */
>> +	controller_.Initialise();
>>  
>>  	setControls(0);
>>  }
>> @@ -162,10 +181,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));
>> +	awbAlgo_->updateBNR(params_);
>> +	memcpy(params, &params_, sizeof(*params));
> 
> 	*params = params_;
> 
>>  
>>  	/* \todo Fill in parameters buffer. */
>> -
>>  	ipa::ipu3::IPU3Action op;
>>  	op.op = ipa::ipu3::ActionParamFilled;
>>  
>> @@ -179,6 +198,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>  
>>  	/* \todo React to statistics and update internal state machine. */
>>  	/* \todo Add meta-data information to ctrls. */
>> +	agcAlgo_->Process(stats, exposure_, gain_);
>> +	if (agcAlgo_->Converged())
>> +		awbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400), stats);
>> +	setControls(frame);
>>  
>>  	ipa::ipu3::IPU3Action op;
>>  	op.op = ipa::ipu3::ActionMetadataReady;
>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
>> new file mode 100644
>> index 00000000..db591e33
>> --- /dev/null
>> +++ b/src/ipa/ipu3/ipu3_agc.cpp
>> @@ -0,0 +1,195 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>> + *
>> + * agc.cpp - AGC/AEC control algorithm
>> + */
>> +#include <iostream>
>> +#include <numeric>
>> +#include <unordered_map>
>> +
>> +#include "libcamera/internal/log.h"
>> +
>> +#include "ipu3_agc.h"
>> +
>> +using namespace libcamera;
>> +
>> +LOG_DEFINE_CATEGORY(IPU3Agc)
>> +
>> +#define NAME "ipu3.agc"
>> +
>> +IPU3Agc::IPU3Agc(IPAController *controller)
>> +	: AgcAlgorithm(controller), frameCount_(0),
>> +	  ev_(1.0), flicker_period_(0.0),
>> +	  max_shutter_(0), fixed_shutter_(0),
>> +	  fixed_analogue_gain_(0.0), algoConverged_(false)
>> +{
>> +}
>> +
>> +char const *IPU3Agc::Name() const
>> +{
>> +	return NAME;
>> +}
>> +
>> +unsigned int IPU3Agc::GetConvergenceFrames() const
>> +{
>> +	return config_.convergence_frames;
>> +}
>> +
>> +void IPU3Agc::SetEv(double ev)
>> +{
>> +	ev_ = ev;
>> +}
>> +
>> +void IPU3Agc::SetFlickerPeriod(double flicker_period)
>> +{
>> +	flicker_period_ = flicker_period;
>> +}
>> +
>> +void IPU3Agc::SetMaxShutter(double max_shutter)
>> +{
>> +	max_shutter_ = max_shutter;
>> +}
>> +
>> +void IPU3Agc::SetFixedShutter(double fixed_shutter)
>> +{
>> +	fixed_shutter_ = fixed_shutter;
>> +}
>> +
>> +void IPU3Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
>> +{
>> +	fixed_analogue_gain_ = fixed_analogue_gain;
>> +}
>> +
>> +void IPU3Agc::SetMeteringMode(std::string const &metering_mode_name)
>> +{
>> +	metering_mode_name_ = metering_mode_name;
>> +}
>> +
>> +void IPU3Agc::SetExposureMode(std::string const &exposure_mode_name)
>> +{
>> +	exposure_mode_name_ = exposure_mode_name;
>> +}
>> +
>> +void IPU3Agc::Prepare() {}
>> +
>> +void IPU3Agc::Process() {}
>> +
>> +/* \todo This function is taken from numerical recipes and calculates all moments
>> + * It needs to be rewritten properly and maybe in a "math" class ? */
> 
> /*
>  * \todo This function is taken from numerical recipes and calculates all
>  * moments. It needs to be rewritten properly and maybe in a "math" class ?
>  */
> 
>> +void IPU3Agc::moments(std::unordered_map<uint32_t, uint32_t> &data, int n)
>> +{
>> +	int j;
>> +	double ep = 0.0, s, p;
>> +	double ave, adev, sdev;
>> +	double var, skew, curt;
>> +
>> +	s = 0.0;
>> +	for (j = 1; j <= n; j++)
>> +		s += data[j];
>> +
>> +	ave = s / n;
>> +	adev = var = skew = curt = 0.0;
>> +
>> +	for (j = 1; j <= n; j++) {
>> +		adev += s = data[j] - (ave);
>> +		ep += s;
>> +		var += (p = s * s);
>> +		skew += (p *= s);
>> +		curt += (p *= s);
>> +	}
>> +
>> +	adev /= n;
>> +	var = (var - ep * ep / n) / (n - 1);
>> +	sdev = std::sqrt(var);
>> +
>> +	if (var) {
>> +		skew /= n * var * sdev;
>> +		curt = curt / (n * var * var) - 3.0;
>> +	}
>> +	skew_ = skew;
>> +}
>> +
>> +void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>> +{
>> +	brightnessVec_.clear();
>> +
>> +	/*\todo Replace constant values with real BDS configuration */
>> +	for (uint32_t j = 0; j < 45; j++) {
>> +		for (uint32_t i = 0; i < 160 * 45 * 8; i += 8) {
>> +			uint8_t Gr = stats->awb_raw_buffer.meta_data[i];
>> +			uint8_t R = stats->awb_raw_buffer.meta_data[i + 1];
>> +			uint8_t B = stats->awb_raw_buffer.meta_data[i + 2];
>> +			uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3];
>> +			brightnessVec_.push_back(static_cast<uint32_t>(0.299 * R + 0.587 * (Gr + Gb) / 2 + 0.114 * B));
>> +		}
>> +	}
>> +	std::sort(brightnessVec_.begin(), brightnessVec_.end());
>> +
>> +	/* \todo create a class to generate histograms ! */
>> +	std::unordered_map<uint32_t, uint32_t> hist;
>> +	for (uint32_t const &val : brightnessVec_)
>> +		hist[val]++;
>> +	moments(hist, 256);
>> +}
>> +
>> +void IPU3Agc::lockExposure(uint32_t &exposure, uint32_t &gain)
>> +{
>> +	/* Algorithm initialization wait for first valid frames */
>> +	/* \todo - have a number of frames given by DelayedControls ?
>> +	 * - implement a function for IIR */
>> +	if (frameCount_ == 4) {
>> +		prevExposure_ = exposure;
>> +
>> +		prevSkew_ = skew_;
>> +		/* \tdo use configured values */
> 
> s/tdo/todo/
> 
>> +		exposure = 800;
>> +		gain = 8;
>> +		currentExposure_ = exposure;
>> +	} else if (frameCount_ == 8) {
>> +		currentSkew_ = skew_;
>> +		exposure = ((currentExposure_ * prevSkew_) + (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
> 
> Line wrap. No need for the inner parentheses.
> 
>> +		nextExposure_ = exposure;
>> +		lastFrame_ = frameCount_;
>> +	} else if ((frameCount_ >= 12) && (frameCount_ - lastFrame_ >= 4)) {
> 
> Could you define constants for magic values, to make the code more
> readable ? They can be defined as static constexpr members of the
> IPU3Agc class, and should be named with camelCase. While we don't do so
> yet in many places, it's customary to start them with a 'k' prefix (e.g.
> kInitialFrameSkipCount).

OK, I will !

>> +		currentSkew_ = skew_;
>> +		/* \todo properly calculate a gain */
>> +		if (frameCount_ == 12)
>> +			gain = ((8 * prevSkew_) + (1 * currentSkew_)) / (prevSkew_ + currentSkew_);
>> +
>> +		if (currentSkew_ - prevSkew_ > 1) {
>> +			/* under exposed */
>> +			prevExposure_ = nextExposure_;
>> +			exposure = ((currentExposure_ * prevSkew_) + (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
>> +			nextExposure_ = exposure;
>> +		} else if (currentSkew_ - prevSkew_ < -1) {
>> +			/* over exposed */
>> +			currentExposure_ = nextExposure_;
>> +			exposure = ((currentExposure_ * prevSkew_) + (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
>> +			nextExposure_ = exposure;
>> +		} else {
>> +			/* we have converged */
>> +			algoConverged_ = true;
>> +		}
>> +		lastFrame_ = frameCount_;
>> +		prevSkew_ = currentSkew_;
>> +	}
>> +}
>> +
>> +void IPU3Agc::Process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain)
>> +{
>> +	processBrightness(stats);
>> +	if (!algoConverged_)
>> +		lockExposure(exposure, gain);
>> +	else {
>> +		/* Are we still well exposed ? */
>> +		if ((skew_ < 2) || (skew_ > 4))
>> +			algoConverged_ = false;
>> +	}
>> +	frameCount_++;
>> +}
>> +
>> +bool IPU3Agc::Converged()
>> +{
>> +	return algoConverged_;
>> +}
>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
>> new file mode 100644
>> index 00000000..9a69d628
>> --- /dev/null
>> +++ b/src/ipa/ipu3/ipu3_agc.h
>> @@ -0,0 +1,96 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>> + *
>> + * ipu3_agc.h - AGC/AEC control algorithm
>> + */
>> +#ifndef __LIBCAMERA_IPU3_AGC_H__
>> +#define __LIBCAMERA_IPU3_AGC_H__
>> +
> 
> #include <map>
> 
>> +#include <mutex>
> 
> This doesn't seem to be needed.
> 
>> +#include <vector>
>> +
>> +#include <linux/intel-ipu3.h>
>> +
>> +#include <libcamera/geometry.h>
>> +
>> +#include <libcamera/ipa/agc_algorithm.h>
>> +#include <libcamera/ipa/ipa_controller.h>
>> +
>> +#define AGC_STATS_SIZE 15
>> +
>> +namespace libcamera {
>> +
>> +struct AgcMeteringMode {
>> +	double weights[AGC_STATS_SIZE];
>> +	Rectangle metering_region[AGC_STATS_SIZE];
>> +};
>> +
>> +struct AgcExposureMode {
>> +	std::vector<double> shutter;
>> +	std::vector<double> gain;
>> +};
>> +
>> +struct AgcConfig {
>> +	std::map<std::string, AgcMeteringMode> metering_modes;
>> +	std::map<std::string, AgcExposureMode> exposure_modes;
>> +	double speed;
>> +	uint16_t startup_frames;
>> +	unsigned int convergence_frames;
>> +	std::string default_metering_mode;
>> +	std::string default_exposure_mode;
>> +	double default_exposure_time;
>> +	double default_analogue_gain;
>> +};
>> +
>> +class IPU3Agc : public AgcAlgorithm
>> +{
>> +public:
>> +	IPU3Agc(IPAController *IPAcontroller);
> 
> Blank line.
> 
>> +	char const *Name() const override;
>> +	unsigned int GetConvergenceFrames() const override;
>> +	void SetEv(double ev) override;
>> +	void SetFlickerPeriod(double flicker_period) override;
>> +	void SetMaxShutter(double max_shutter) override; // microseconds
>> +	void SetFixedShutter(double fixed_shutter) override; // microseconds
>> +	void SetFixedAnalogueGain(double fixed_analogue_gain) override;
>> +	void SetMeteringMode(std::string const &metering_mode_name) override;
>> +	void SetExposureMode(std::string const &exposure_mode_name) override;
>> +	void Prepare() override;
>> +	void Process() override;
>> +	void Process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain);
> 
> The last two arguments should be replaced by a generic data container,
> similar to the Metadata class in the RPi IPA (not something to fix now).
> 
>> +	bool Converged();
>> +
>> +private:
>> +	void moments(std::unordered_map<uint32_t, uint32_t> &data, int n);
>> +	void processBrightness(const ipu3_uapi_stats_3a *stats);
>> +	void lockExposure(uint32_t &exposure, uint32_t &gain);
>> +
>> +	AgcConfig config_;
> 
> This is never set, and only one member is read. I'd drop it for now.
> 
>> +	std::string metering_mode_name_;
>> +	std::string exposure_mode_name_;
>> +	uint64_t frameCount_;
>> +	uint64_t lastFrame_;
>> +
>> +	/* Vector of calculated brightness for each cell */
>> +	std::vector<uint32_t> brightnessVec_;
> 
> We avoid encoding the type in the variable name. Would this be better
> named cellsBrightness_ ?
> 
>> +	double ev_;
> 
> Never used. Same for other members below.
> 
>> +	double flicker_period_;
>> +	double max_shutter_;
>> +	double fixed_shutter_;
>> +	double fixed_analogue_gain_;
>> +
>> +	/* Values for filtering */
>> +	uint32_t prevExposure_;
>> +	uint32_t currentExposure_;
>> +	uint32_t nextExposure_;
>> +
>> +	double skew_;
>> +	double prevSkew_;
>> +	double currentSkew_;
>> +	bool algoConverged_;
> 
> Maybe just converged_ ?
> 
>> +};
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_IPU3_AGC_H__ */
>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
>> new file mode 100644
>> index 00000000..21286e4c
>> --- /dev/null
>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
>> @@ -0,0 +1,182 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>> + *
>> + * ipu3_awb.cpp - AWB control algorithm
>> + */
>> +#include <iostream>
> 
> I don't think this is needed.
> 
>> +#include <numeric>
>> +#include <unordered_map>
>> +
>> +#include "libcamera/internal/log.h"
>> +
>> +#include "ipu3_awb.h"
>> +
>> +using namespace libcamera;
>> +
>> +LOG_DEFINE_CATEGORY(IPU3Awb)
>> +
>> +#define NAME "ipu3.awb"
>> +
>> +IPU3Awb::IPU3Awb(IPAController *controller)
>> +	: AwbAlgorithm(controller)
>> +{
>> +}
>> +
>> +IPU3Awb::~IPU3Awb()
>> +{
>> +}
>> +
>> +char const *IPU3Awb::Name() const
>> +{
>> +	return NAME;
>> +}
>> +
>> +void IPU3Awb::Initialise() {}
> 
> void IPU3Awb::Initialise()
> {
> }
> 
>> +
>> +void IPU3Awb::Initialise(ipu3_uapi_params &params)
>> +{
>> +	memset(&params, 0, sizeof(params));
>> +	memset(wbGains_, 8192, sizeof(wbGains_));
> 
> memset() initializes the memory byte by byte, this will not store 8192
> in each entry of the array.
> 

Indeed... :-/

>> +	wbGains_[0] = 8192 * 0.8;
>> +	wbGains_[3] = 8192 * 0.8;
> 
> I would group the initialization of member variables together, either
> before, or after the initialization of params.
> 
>> +	params.use.acc_awb = 1;
>> +	/*\todo fill the grid calculated based on BDS configuration */
>> +	params.acc_param.awb.config = imgu_css_awb_defaults;
>> +
>> +	params.use.acc_bnr = 1;
>> +	params.acc_param.bnr = imgu_css_bnr_defaults;
>> +
>> +	params.use.acc_ccm = 1;
>> +	params.acc_param.ccm = imgu_css_ccm_3800k;
>> +
>> +	params.use.acc_gamma = 1;
>> +	params.acc_param.gamma.gc_ctrl.enable = 1;
>> +
>> +	uint32_t a = (32 * 245) / (245 - 9);
>> +
>> +	for (uint32_t i = 0; i < 10; i++)
>> +		params.acc_param.gamma.gc_lut.lut[i] = 0;
>> +	for (uint32_t i = 10; i < 245; i++)
>> +		params.acc_param.gamma.gc_lut.lut[i] = a * i + (0 - a * 9);
>> +	for (uint32_t i = 245; i < 255; i++)
>> +		params.acc_param.gamma.gc_lut.lut[i] = 32 * 245;
>> +
>> +	frame_count_ = 0;
>> +	algoConverged_ = false;
>> +}
>> +
>> +unsigned int IPU3Awb::GetConvergenceFrames() const
>> +{
>> +	// If colour gains have been explicitly set, there is no convergence
>> +	// to happen, so no need to drop any frames - return zero.
>> +	if (manual_r_ && manual_b_)
>> +		return 0;
>> +	else
>> +		return config_.convergence_frames;
>> +}
>> +
>> +void IPU3Awb::SetMode(std::string const &mode_name)
>> +{
>> +	mode_name_ = mode_name;
>> +}
>> +
>> +void IPU3Awb::SetManualGains(double manual_r, double manual_b)
>> +{
>> +	// If any of these are 0.0, we swich back to auto.
>> +	manual_r_ = manual_r;
>> +	manual_b_ = manual_b;
>> +}
>> +
>> +uint32_t IPU3Awb::estimateCCT(uint8_t R, uint8_t G, uint8_t B)
>> +{
>> +	double X = (-0.14282) * (R) + (1.54924) * (G) + (-0.95641) * (B);
>> +	double Y = (-0.32466) * (R) + (1.57837) * (G) + (-0.73191) * (B);
>> +	double Z = (-0.68202) * (R) + (0.77073) * (G) + (0.56332) * (B);
>> +
>> +	double x = X / (X + Y + Z);
>> +	double y = Y / (X + Y + Z);
>> +
>> +	double n = (x - 0.3320) / (0.1858 - y);
>> +	return static_cast<uint32_t>(449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33);
>> +}
>> +
>> +void IPU3Awb::calculateWBGains([[maybe_unused]] Rectangle roi, const ipu3_uapi_stats_3a *stats)
> 
> Line wrap.
> 
>> +{
>> +	if (algoConverged_)
>> +		return;
> 
> So once it converges, you never update the gains anymore, even if the
> scene changes ?
> 

This is an error, I copy/pasted it to ipu3 and it should be called only
when AE has converged. If there is a sufficient scene change, then AE
will diverge and so it will be called again after the new convergence
(does it make sense :-) ?).

>> +
>> +	std::vector<uint32_t> R_v, Gr_v, Gb_v, B_v;
> 
> camelCase. Feel free to also make the name slightly longer to make the
> more explicit. redValues ?
> 
>> +	Point topleft = roi.topLeft();
>> +	uint32_t startY = (topleft.y / 16) * 160 * 8;
>> +	uint32_t startX = (topleft.x / 8) * 8;
>> +	uint32_t endX = (startX + (roi.size().width / 8)) * 8;
>> +
>> +	for (uint32_t j = (topleft.y / 16); j < (topleft.y / 16) + (roi.size().height / 16); j++) {
>> +		for (uint32_t i = startX + startY; i < endX + startY; i += 8) {
>> +			Gr_v.push_back(stats->awb_raw_buffer.meta_data[i]);
>> +			R_v.push_back(stats->awb_raw_buffer.meta_data[i + 1]);
>> +			B_v.push_back(stats->awb_raw_buffer.meta_data[i + 2]);
>> +			Gb_v.push_back(stats->awb_raw_buffer.meta_data[i + 3]);
>> +		}
>> +	}
>> +
>> +	std::sort(R_v.begin(), R_v.end());
>> +	std::sort(Gr_v.begin(), Gr_v.end());
>> +	std::sort(B_v.begin(), B_v.end());
>> +	std::sort(Gb_v.begin(), Gb_v.end());
>> +
>> +	double Grmed = Gr_v[Gr_v.size() / 2];
>> +	double Rmed = R_v[R_v.size() / 2];
>> +	double Bmed = B_v[B_v.size() / 2];
>> +	double Gbmed = Gb_v[Gb_v.size() / 2];
>> +
>> +	double Rgain = Grmed / Rmed;
>> +	double Bgain = Gbmed / Bmed;
>> +	LOG(IPU3Awb, Debug) << "max R, Gr, B, Gb: "
>> +			    << R_v.back() << ","
>> +			    << Gr_v.back() << ","
>> +			    << B_v.back() << ","
>> +			    << Gb_v.back();
>> +	tint_ = ((Rmed / Grmed) + (Bmed / Gbmed)) / 2;
>> +
>> +	/* \todo Those are corrections when light is really low
>> +	 * it should be taken into account by AGC somehow */
>> +	if ((Rgain >= 2) && (Bgain < 2)) {
>> +		wbGains_[0] = 4096 * tint_;
>> +		wbGains_[1] = 8192 * Rgain;
>> +		wbGains_[2] = 4096 * Bgain;
>> +		wbGains_[3] = 4096 * tint_;
>> +	} else if ((Bgain >= 2) && (Rgain < 2)) {
>> +		wbGains_[0] = 4096 * tint_;
>> +		wbGains_[1] = 4096 * Rgain;
>> +		wbGains_[2] = 8192 * Bgain;
>> +		wbGains_[3] = 4096 * tint_;
>> +	} else {
>> +		wbGains_[0] = 8192 * tint_;
>> +		wbGains_[1] = 8192 * Rgain;
>> +		wbGains_[2] = 8192 * Bgain;
>> +		wbGains_[3] = 8192 * tint_;
>> +	}
>> +
>> +	frame_count_++;
>> +
>> +	cct_ = estimateCCT(Rmed, (Grmed + Gbmed) / 2, Bmed);
>> +
>> +	algoConverged_ = true;
>> +}
>> +
>> +void IPU3Awb::updateBNR(ipu3_uapi_params &params)
> 
> updateBNR() isn't a great name for a function that updates the WB gains,
> even if they're part of the Bayer noise reduction block of data.
> 
> Should we push the ipu3_uapi_params structure down to individual
> algorithms, or would it be better to decouple it, creating custom
> structures to hold algorithm results, and fillin ipu3_uapi_params in
> IPAIPU3::fillParams() instead ? On one hand moving ipu3_uapi_params out
> will make the code more generic, and thus more reusable, but on the
> other hand we would still depend on a custom stats format, so it may not
> be worth it.
> 

That is indeed a blocker here.
I will rename the function, but I think we need to let AWB fill the
parameters related to it and not directly filling those in
IPAIPU3::fillParams().

>> +{
>> +	if (!algoConverged_)
>> +		return;
>> +
>> +	params.acc_param.bnr.wb_gains.gr = wbGains_[0];
>> +	params.acc_param.bnr.wb_gains.r = wbGains_[1];
>> +	params.acc_param.bnr.wb_gains.b = wbGains_[2];
>> +	params.acc_param.bnr.wb_gains.gb = wbGains_[3];
>> +	if (cct_ < 5500)
>> +		params.acc_param.ccm = imgu_css_ccm_3800k;
>> +	else
>> +		params.acc_param.ccm = imgu_css_ccm_6000k;
>> +}
>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
>> new file mode 100644
>> index 00000000..06389020
>> --- /dev/null
>> +++ b/src/ipa/ipu3/ipu3_awb.h
>> @@ -0,0 +1,130 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>> + *
>> + * awb.h - AWB control algorithm
>> + */
>> +#ifndef __LIBCAMERA_IPU3_AWB_H__
>> +#define __LIBCAMERA_IPU3_AWB_H__
>> +
>> +#include <linux/intel-ipu3.h>
>> +
>> +#include <libcamera/geometry.h>
>> +
>> +#include <libcamera/ipa/awb_algorithm.h>
>> +#include <libcamera/ipa/ipa_controller.h>
>> +
>> +namespace libcamera {
>> +
>> +const struct ipu3_uapi_bnr_static_config imgu_css_bnr_defaults = {
>> +	{ 16, 16, 16, 16 },			/* wb_gains */
>> +	{ 255, 255, 255, 255 },			/* wb_gains_thr */
>> +	{ 0, 0, 8, 6, 0, 14 },			/* thr_coeffs */
>> +	{ 0, 0, 0, 0 },				/* thr_ctrl_shd */
>> +	{ -648, 0, -366, 0 },			/* opt_center */
>> +	{					/* 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 }
>> +	},
>> +	{ 4, 0, 1, 8, 0, 8, 0, 8, 0 },		/* bp_ctrl */
>> +	{ 8, 4, 4, 0, 8, 0, 1, 1, 1, 1, 0 },	/* dn_detect_ctrl */
>> +	1296,
>> +	{ 419904, 133956 },
>> +};
>> +
>> +/* settings for Auto White Balance */
>> +const struct ipu3_uapi_awb_config_s imgu_css_awb_defaults = {
>> +	8191,
>> +	8191,
>> +	8191,
>> +	8191 | /* rgbs_thr_gr/r/gb/b */
>> +		IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
>> +	.grid = {
>> +		.width = 160,
>> +		.height = 45,
>> +		.block_width_log2 = 3,
>> +		.block_height_log2 = 4,
>> +		.x_start = 0,
>> +		.y_start = 0,
>> +	},
>> +};
>> +
>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_6000k = {
>> +	7239, -750, -37, 0,
>> +	-215, 9176, -200, 0,
>> +	-70, -589, 6810, 0
>> +};
>> +
>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_4900k = {
>> +	7811, -464, -466, 0,
>> +	-635, 8762, -533, 0,
>> +	-469, -154, 6583, 0
>> +};
>> +
>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_3800k = {
>> +	7379, -526, -296, 0,
>> +	-411, 7397, -415, 0,
>> +	-224, -564, 7244, 0
>> +};
> 
> All these should be moved to ipu3_awb.cpp, and be made static. Otherwise
> they would get duplicated in every compilation unit in which this header
> is included.
> 
>> +
>> +struct AwbConfig {
>> +	// Only repeat the AWB calculation every "this many" frames
>> +	uint16_t frame_period;
>> +	// number of initial frames for which speed taken as 1.0 (maximum)
>> +	uint16_t startup_frames;
>> +	unsigned int convergence_frames; // approx number of frames to converge
>> +	double speed; // IIR filter speed applied to algorithm results
>> +};
>> +
>> +#if 0
>> +typedef struct awb_public_set_item{
>> +    unsigned char Gr_avg;
>> +    unsigned char R_avg;
>> +    unsigned char B_avg;
>> +    unsigned char Gb_avg;
>> +    unsigned char sat_ratio;
>> +    unsigned char padding0; /**< Added the padding so that the public matches that private */
>> +    unsigned char padding1; /**< Added the padding so that the public matches that private */
>> +    unsigned char padding2; /**< Added the padding so that the public matches that private */
>> +} awb_public_set_item_t;
>> +#endif
> 
> If the compiler doesn't need to see this, I think you can drop it
> completely :-)
> 

I will remove them now, thanks !
It will be used later as this is how the stats are organized internally.

>> +
>> +class IPU3Awb : public AwbAlgorithm
>> +{
>> +public:
>> +	IPU3Awb(IPAController *controller = NULL);
> 
> This is C++, so nullptr :-) But there's no need for a default value, as
> the controller parameter should either be mandatory, or removed
> completely.
> 
>> +	~IPU3Awb();
> 
> Blank line.
> 
>> +	virtual char const *Name() const override;
> 
> This is never used, you can drop it from the base class. When (and if)
> we'll need this, I'd rather pass the name to the base class constructor,
> store it there, and implement name() in the base class only, as a
> non-virtual function.
> 
>> +	virtual void Initialise() override;
> 
> No need to repeat the virtual keyword for overridden functions.
> 
>> +	void Initialise(ipu3_uapi_params &params);
> 
> This isn't very nice, as it's not aligned with the design of the
> IPAAlgorithm class, but we can fix it later. I would move the function
> down, after the mandatory functions, with calculateWBGains() and
> updateBNR().
> 
>> +	unsigned int GetConvergenceFrames() const override;
>> +	void SetMode(std::string const &name) override;
>> +	void SetManualGains(double manual_r, double manual_b) override;
> 
> A blank line here would be nice.
> 
>> +	void calculateWBGains(Rectangle roi,
> 
> const Rectangle &roi
> 
>> +			      const ipu3_uapi_stats_3a *stats);
>> +	void updateBNR(ipu3_uapi_params &params);
>> +
>> +private:
>> +	uint32_t estimateCCT(uint8_t R, uint8_t G, uint8_t B);
> 
> r, g and b.
> 
>> +
>> +	/* configuration is read-only, and available to both threads */
> 
> What threads ? :-)
> 

Yes, which ones ?? ;-)
Well, as in RPi, there will be threads later, if the algorithms are too
complicated (they probably will be)...

>> +	AwbConfig config_;
>> +	std::string mode_name_;
> 
> Never used.
> 
>> +	/* manual r setting */
>> +	double manual_r_;
> 
> manualRedGain_ ?
> 
>> +	/* manual b setting */
>> +	double manual_b_;
>> +	/* WB calculated gains */
>> +	uint16_t wbGains_[4];
>> +	double tint_;
>> +	uint32_t cct_;
>> +
>> +	uint32_t frame_count_;
>> +
>> +	bool algoConverged_;
>> +};
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_IPU3_AWB_H__ */
>> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
>> index a241f617..43ad0e0d 100644
>> --- a/src/ipa/ipu3/meson.build
>> +++ b/src/ipa/ipu3/meson.build
>> @@ -2,8 +2,14 @@
>>  
>>  ipa_name = 'ipa_ipu3'
>>  
>> +ipu3_ipa_sources = files([
>> +  'ipu3.cpp',
>> +  'ipu3_agc.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,
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index ff980b38..3809c943 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -825,6 +825,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>>  	 */
>>  	double lineDuration = sensorInfo.lineLength
>>  			    / (sensorInfo.pixelRate / 1e6);
>> +	LOG(IPU3, Error) << "line duration: " << lineDuration;
> 
> Is this an error ? :-)
> 
Ahem ->[]
>>  	const ControlInfoMap &sensorControls = sensor->controls();
>>  	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
>>  	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
>

Hi Jean-Michel,

On Tue, Feb 23, 2021 at 03:27:56PM +0100, Jean-Michel Hautbois wrote:
> On 22/02/2021 05:56, Laurent Pinchart wrote:
> > On Fri, Feb 19, 2021 at 06:22:24PM +0100, Jean-Michel Hautbois wrote:
> >> Inherit from the AWBAlgorithm and AGCAlgorithm classes to implement
> >> basic functions.
> >> While exposure is not locked, AWB is not calculated and corrected.
> >> Once AWB is done, a color temperature is estimated and default CCM matrices
> >> are used.
> >> Implement a basic "grey-world" AWB algorithm just for demonstration purpose.
> > 
> > Nice to see an initial implementation ! I'll provide a handful of review
> > comments below already. Some are related to coding style or similar
> > issues and should be addressed in v2. Others are related to the IPA
> > architecture, and could be fixed on top of this series to avoid delaying
> > it unnecessarily. They should in that case be captured in a \todo
> > comment.
> > 
> > As for 1/2, I won't repeat the same comments all over the patch, so
> > please apply them wherever they're applicable.
> 
> Thanks, I only have a few questions :-).
> 
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >> ---
> >>  src/ipa/ipu3/ipu3.cpp                |  31 ++++-
> >>  src/ipa/ipu3/ipu3_agc.cpp            | 195 +++++++++++++++++++++++++++
> >>  src/ipa/ipu3/ipu3_agc.h              |  96 +++++++++++++
> >>  src/ipa/ipu3/ipu3_awb.cpp            | 182 +++++++++++++++++++++++++
> >>  src/ipa/ipu3/ipu3_awb.h              | 130 ++++++++++++++++++
> >>  src/ipa/ipu3/meson.build             |   8 +-
> >>  src/libcamera/pipeline/ipu3/ipu3.cpp |   1 +
> >>  7 files changed, 638 insertions(+), 5 deletions(-)
> >>  create mode 100644 src/ipa/ipu3/ipu3_agc.cpp
> >>  create mode 100644 src/ipa/ipu3/ipu3_agc.h
> >>  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 b63e58be..c3859f39 100644
> >> --- a/src/ipa/ipu3/ipu3.cpp
> >> +++ b/src/ipa/ipu3/ipu3.cpp
> >> @@ -13,6 +13,7 @@
> >>  
> >>  #include <libcamera/buffer.h>
> >>  #include <libcamera/control_ids.h>
> >> +#include <libcamera/ipa/ipa_controller.h>
> >>  #include <libcamera/ipa/ipa_interface.h>
> >>  #include <libcamera/ipa/ipa_module_info.h>
> >>  #include <libcamera/ipa/ipu3_ipa_interface.h>
> >> @@ -21,6 +22,9 @@
> >>  #include "libcamera/internal/buffer.h"
> >>  #include "libcamera/internal/log.h"
> >>  
> >> +#include "ipu3_agc.h"
> >> +#include "ipu3_awb.h"
> >> +
> >>  namespace libcamera {
> >>  
> >>  LOG_DEFINE_CATEGORY(IPAIPU3)
> >> @@ -28,6 +32,9 @@ LOG_DEFINE_CATEGORY(IPAIPU3)
> >>  class IPAIPU3 : public ipa::ipu3::IPAIPU3Interface
> >>  {
> >>  public:
> >> +	IPAIPU3()
> >> +		: controller_() {}
> > 
> > 		: controller_()
> > 	{
> > 	}
> > 
> > I may not repeat all the comments from 1/2, but they're generally
> > applicable to this patch as well.
> > 
> >> +
> >>  	int init([[maybe_unused]] const IPASettings &settings) override
> >>  	{
> >>  		return 0;
> >> @@ -60,6 +67,11 @@ private:
> >>  	uint32_t gain_;
> >>  	uint32_t minGain_;
> >>  	uint32_t maxGain_;
> >> +
> >> +	IPAController controller_;
> >> +	IPU3Awb *awbAlgo_;
> >> +	IPU3Agc *agcAlgo_;
> >> +	ipu3_uapi_params params_;
> >>  };
> >>  
> >>  void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls)
> >> @@ -83,11 +95,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_ = 123;
> > 
> > A peculiar value :-)
> > 
> >>  
> >>  	minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
> >>  	maxGain_ = itGain->second.max().get<int32_t>();
> >> -	gain_ = maxGain_;
> >> +	gain_ = 1;
> >> +
> >> +	awbAlgo_ = new IPU3Awb(&controller_);
> >> +	awbAlgo_->Initialise(params_);
> >> +	agcAlgo_ = new IPU3Agc(&controller_);
> >> +
> >> +	/*\todo not used yet... */
> >> +	controller_.Initialise();
> >>  
> >>  	setControls(0);
> >>  }
> >> @@ -162,10 +181,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));
> >> +	awbAlgo_->updateBNR(params_);
> >> +	memcpy(params, &params_, sizeof(*params));
> > 
> > 	*params = params_;
> > 
> >>  
> >>  	/* \todo Fill in parameters buffer. */
> >> -
> >>  	ipa::ipu3::IPU3Action op;
> >>  	op.op = ipa::ipu3::ActionParamFilled;
> >>  
> >> @@ -179,6 +198,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> >>  
> >>  	/* \todo React to statistics and update internal state machine. */
> >>  	/* \todo Add meta-data information to ctrls. */
> >> +	agcAlgo_->Process(stats, exposure_, gain_);
> >> +	if (agcAlgo_->Converged())
> >> +		awbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400), stats);
> >> +	setControls(frame);
> >>  
> >>  	ipa::ipu3::IPU3Action op;
> >>  	op.op = ipa::ipu3::ActionMetadataReady;
> >> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> >> new file mode 100644
> >> index 00000000..db591e33
> >> --- /dev/null
> >> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> >> @@ -0,0 +1,195 @@
> >> +/* SPDX-License-Identifier: BSD-2-Clause */
> >> +/*
> >> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> >> + *
> >> + * agc.cpp - AGC/AEC control algorithm
> >> + */
> >> +#include <iostream>
> >> +#include <numeric>
> >> +#include <unordered_map>
> >> +
> >> +#include "libcamera/internal/log.h"
> >> +
> >> +#include "ipu3_agc.h"
> >> +
> >> +using namespace libcamera;
> >> +
> >> +LOG_DEFINE_CATEGORY(IPU3Agc)
> >> +
> >> +#define NAME "ipu3.agc"
> >> +
> >> +IPU3Agc::IPU3Agc(IPAController *controller)
> >> +	: AgcAlgorithm(controller), frameCount_(0),
> >> +	  ev_(1.0), flicker_period_(0.0),
> >> +	  max_shutter_(0), fixed_shutter_(0),
> >> +	  fixed_analogue_gain_(0.0), algoConverged_(false)
> >> +{
> >> +}
> >> +
> >> +char const *IPU3Agc::Name() const
> >> +{
> >> +	return NAME;
> >> +}
> >> +
> >> +unsigned int IPU3Agc::GetConvergenceFrames() const
> >> +{
> >> +	return config_.convergence_frames;
> >> +}
> >> +
> >> +void IPU3Agc::SetEv(double ev)
> >> +{
> >> +	ev_ = ev;
> >> +}
> >> +
> >> +void IPU3Agc::SetFlickerPeriod(double flicker_period)
> >> +{
> >> +	flicker_period_ = flicker_period;
> >> +}
> >> +
> >> +void IPU3Agc::SetMaxShutter(double max_shutter)
> >> +{
> >> +	max_shutter_ = max_shutter;
> >> +}
> >> +
> >> +void IPU3Agc::SetFixedShutter(double fixed_shutter)
> >> +{
> >> +	fixed_shutter_ = fixed_shutter;
> >> +}
> >> +
> >> +void IPU3Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
> >> +{
> >> +	fixed_analogue_gain_ = fixed_analogue_gain;
> >> +}
> >> +
> >> +void IPU3Agc::SetMeteringMode(std::string const &metering_mode_name)
> >> +{
> >> +	metering_mode_name_ = metering_mode_name;
> >> +}
> >> +
> >> +void IPU3Agc::SetExposureMode(std::string const &exposure_mode_name)
> >> +{
> >> +	exposure_mode_name_ = exposure_mode_name;
> >> +}
> >> +
> >> +void IPU3Agc::Prepare() {}
> >> +
> >> +void IPU3Agc::Process() {}
> >> +
> >> +/* \todo This function is taken from numerical recipes and calculates all moments
> >> + * It needs to be rewritten properly and maybe in a "math" class ? */
> > 
> > /*
> >  * \todo This function is taken from numerical recipes and calculates all
> >  * moments. It needs to be rewritten properly and maybe in a "math" class ?
> >  */
> > 
> >> +void IPU3Agc::moments(std::unordered_map<uint32_t, uint32_t> &data, int n)
> >> +{
> >> +	int j;
> >> +	double ep = 0.0, s, p;
> >> +	double ave, adev, sdev;
> >> +	double var, skew, curt;
> >> +
> >> +	s = 0.0;
> >> +	for (j = 1; j <= n; j++)
> >> +		s += data[j];
> >> +
> >> +	ave = s / n;
> >> +	adev = var = skew = curt = 0.0;
> >> +
> >> +	for (j = 1; j <= n; j++) {
> >> +		adev += s = data[j] - (ave);
> >> +		ep += s;
> >> +		var += (p = s * s);
> >> +		skew += (p *= s);
> >> +		curt += (p *= s);
> >> +	}
> >> +
> >> +	adev /= n;
> >> +	var = (var - ep * ep / n) / (n - 1);
> >> +	sdev = std::sqrt(var);
> >> +
> >> +	if (var) {
> >> +		skew /= n * var * sdev;
> >> +		curt = curt / (n * var * var) - 3.0;
> >> +	}
> >> +	skew_ = skew;
> >> +}
> >> +
> >> +void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> >> +{
> >> +	brightnessVec_.clear();
> >> +
> >> +	/*\todo Replace constant values with real BDS configuration */
> >> +	for (uint32_t j = 0; j < 45; j++) {
> >> +		for (uint32_t i = 0; i < 160 * 45 * 8; i += 8) {
> >> +			uint8_t Gr = stats->awb_raw_buffer.meta_data[i];
> >> +			uint8_t R = stats->awb_raw_buffer.meta_data[i + 1];
> >> +			uint8_t B = stats->awb_raw_buffer.meta_data[i + 2];
> >> +			uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3];
> >> +			brightnessVec_.push_back(static_cast<uint32_t>(0.299 * R + 0.587 * (Gr + Gb) / 2 + 0.114 * B));
> >> +		}
> >> +	}
> >> +	std::sort(brightnessVec_.begin(), brightnessVec_.end());
> >> +
> >> +	/* \todo create a class to generate histograms ! */
> >> +	std::unordered_map<uint32_t, uint32_t> hist;
> >> +	for (uint32_t const &val : brightnessVec_)
> >> +		hist[val]++;
> >> +	moments(hist, 256);
> >> +}
> >> +
> >> +void IPU3Agc::lockExposure(uint32_t &exposure, uint32_t &gain)
> >> +{
> >> +	/* Algorithm initialization wait for first valid frames */
> >> +	/* \todo - have a number of frames given by DelayedControls ?
> >> +	 * - implement a function for IIR */
> >> +	if (frameCount_ == 4) {
> >> +		prevExposure_ = exposure;
> >> +
> >> +		prevSkew_ = skew_;
> >> +		/* \tdo use configured values */
> > 
> > s/tdo/todo/
> > 
> >> +		exposure = 800;
> >> +		gain = 8;
> >> +		currentExposure_ = exposure;
> >> +	} else if (frameCount_ == 8) {
> >> +		currentSkew_ = skew_;
> >> +		exposure = ((currentExposure_ * prevSkew_) + (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
> > 
> > Line wrap. No need for the inner parentheses.
> > 
> >> +		nextExposure_ = exposure;
> >> +		lastFrame_ = frameCount_;
> >> +	} else if ((frameCount_ >= 12) && (frameCount_ - lastFrame_ >= 4)) {
> > 
> > Could you define constants for magic values, to make the code more
> > readable ? They can be defined as static constexpr members of the
> > IPU3Agc class, and should be named with camelCase. While we don't do so
> > yet in many places, it's customary to start them with a 'k' prefix (e.g.
> > kInitialFrameSkipCount).
> 
> OK, I will !
> 
> >> +		currentSkew_ = skew_;
> >> +		/* \todo properly calculate a gain */
> >> +		if (frameCount_ == 12)
> >> +			gain = ((8 * prevSkew_) + (1 * currentSkew_)) / (prevSkew_ + currentSkew_);
> >> +
> >> +		if (currentSkew_ - prevSkew_ > 1) {
> >> +			/* under exposed */
> >> +			prevExposure_ = nextExposure_;
> >> +			exposure = ((currentExposure_ * prevSkew_) + (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
> >> +			nextExposure_ = exposure;
> >> +		} else if (currentSkew_ - prevSkew_ < -1) {
> >> +			/* over exposed */
> >> +			currentExposure_ = nextExposure_;
> >> +			exposure = ((currentExposure_ * prevSkew_) + (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
> >> +			nextExposure_ = exposure;
> >> +		} else {
> >> +			/* we have converged */
> >> +			algoConverged_ = true;
> >> +		}
> >> +		lastFrame_ = frameCount_;
> >> +		prevSkew_ = currentSkew_;
> >> +	}
> >> +}
> >> +
> >> +void IPU3Agc::Process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain)
> >> +{
> >> +	processBrightness(stats);
> >> +	if (!algoConverged_)
> >> +		lockExposure(exposure, gain);
> >> +	else {
> >> +		/* Are we still well exposed ? */
> >> +		if ((skew_ < 2) || (skew_ > 4))
> >> +			algoConverged_ = false;
> >> +	}
> >> +	frameCount_++;
> >> +}
> >> +
> >> +bool IPU3Agc::Converged()
> >> +{
> >> +	return algoConverged_;
> >> +}
> >> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> >> new file mode 100644
> >> index 00000000..9a69d628
> >> --- /dev/null
> >> +++ b/src/ipa/ipu3/ipu3_agc.h
> >> @@ -0,0 +1,96 @@
> >> +/* SPDX-License-Identifier: BSD-2-Clause */
> >> +/*
> >> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> >> + *
> >> + * ipu3_agc.h - AGC/AEC control algorithm
> >> + */
> >> +#ifndef __LIBCAMERA_IPU3_AGC_H__
> >> +#define __LIBCAMERA_IPU3_AGC_H__
> >> +
> > 
> > #include <map>
> > 
> >> +#include <mutex>
> > 
> > This doesn't seem to be needed.
> > 
> >> +#include <vector>
> >> +
> >> +#include <linux/intel-ipu3.h>
> >> +
> >> +#include <libcamera/geometry.h>
> >> +
> >> +#include <libcamera/ipa/agc_algorithm.h>
> >> +#include <libcamera/ipa/ipa_controller.h>
> >> +
> >> +#define AGC_STATS_SIZE 15
> >> +
> >> +namespace libcamera {
> >> +
> >> +struct AgcMeteringMode {
> >> +	double weights[AGC_STATS_SIZE];
> >> +	Rectangle metering_region[AGC_STATS_SIZE];
> >> +};
> >> +
> >> +struct AgcExposureMode {
> >> +	std::vector<double> shutter;
> >> +	std::vector<double> gain;
> >> +};
> >> +
> >> +struct AgcConfig {
> >> +	std::map<std::string, AgcMeteringMode> metering_modes;
> >> +	std::map<std::string, AgcExposureMode> exposure_modes;
> >> +	double speed;
> >> +	uint16_t startup_frames;
> >> +	unsigned int convergence_frames;
> >> +	std::string default_metering_mode;
> >> +	std::string default_exposure_mode;
> >> +	double default_exposure_time;
> >> +	double default_analogue_gain;
> >> +};
> >> +
> >> +class IPU3Agc : public AgcAlgorithm
> >> +{
> >> +public:
> >> +	IPU3Agc(IPAController *IPAcontroller);
> > 
> > Blank line.
> > 
> >> +	char const *Name() const override;
> >> +	unsigned int GetConvergenceFrames() const override;
> >> +	void SetEv(double ev) override;
> >> +	void SetFlickerPeriod(double flicker_period) override;
> >> +	void SetMaxShutter(double max_shutter) override; // microseconds
> >> +	void SetFixedShutter(double fixed_shutter) override; // microseconds
> >> +	void SetFixedAnalogueGain(double fixed_analogue_gain) override;
> >> +	void SetMeteringMode(std::string const &metering_mode_name) override;
> >> +	void SetExposureMode(std::string const &exposure_mode_name) override;
> >> +	void Prepare() override;
> >> +	void Process() override;
> >> +	void Process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain);
> > 
> > The last two arguments should be replaced by a generic data container,
> > similar to the Metadata class in the RPi IPA (not something to fix now).
> > 
> >> +	bool Converged();
> >> +
> >> +private:
> >> +	void moments(std::unordered_map<uint32_t, uint32_t> &data, int n);
> >> +	void processBrightness(const ipu3_uapi_stats_3a *stats);
> >> +	void lockExposure(uint32_t &exposure, uint32_t &gain);
> >> +
> >> +	AgcConfig config_;
> > 
> > This is never set, and only one member is read. I'd drop it for now.
> > 
> >> +	std::string metering_mode_name_;
> >> +	std::string exposure_mode_name_;
> >> +	uint64_t frameCount_;
> >> +	uint64_t lastFrame_;
> >> +
> >> +	/* Vector of calculated brightness for each cell */
> >> +	std::vector<uint32_t> brightnessVec_;
> > 
> > We avoid encoding the type in the variable name. Would this be better
> > named cellsBrightness_ ?
> > 
> >> +	double ev_;
> > 
> > Never used. Same for other members below.
> > 
> >> +	double flicker_period_;
> >> +	double max_shutter_;
> >> +	double fixed_shutter_;
> >> +	double fixed_analogue_gain_;
> >> +
> >> +	/* Values for filtering */
> >> +	uint32_t prevExposure_;
> >> +	uint32_t currentExposure_;
> >> +	uint32_t nextExposure_;
> >> +
> >> +	double skew_;
> >> +	double prevSkew_;
> >> +	double currentSkew_;
> >> +	bool algoConverged_;
> > 
> > Maybe just converged_ ?
> > 
> >> +};
> >> +
> >> +} /* namespace libcamera */
> >> +
> >> +#endif /* __LIBCAMERA_IPU3_AGC_H__ */
> >> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> >> new file mode 100644
> >> index 00000000..21286e4c
> >> --- /dev/null
> >> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> >> @@ -0,0 +1,182 @@
> >> +/* SPDX-License-Identifier: BSD-2-Clause */
> >> +/*
> >> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> >> + *
> >> + * ipu3_awb.cpp - AWB control algorithm
> >> + */
> >> +#include <iostream>
> > 
> > I don't think this is needed.
> > 
> >> +#include <numeric>
> >> +#include <unordered_map>
> >> +
> >> +#include "libcamera/internal/log.h"
> >> +
> >> +#include "ipu3_awb.h"
> >> +
> >> +using namespace libcamera;
> >> +
> >> +LOG_DEFINE_CATEGORY(IPU3Awb)
> >> +
> >> +#define NAME "ipu3.awb"
> >> +
> >> +IPU3Awb::IPU3Awb(IPAController *controller)
> >> +	: AwbAlgorithm(controller)
> >> +{
> >> +}
> >> +
> >> +IPU3Awb::~IPU3Awb()
> >> +{
> >> +}
> >> +
> >> +char const *IPU3Awb::Name() const
> >> +{
> >> +	return NAME;
> >> +}
> >> +
> >> +void IPU3Awb::Initialise() {}
> > 
> > void IPU3Awb::Initialise()
> > {
> > }
> > 
> >> +
> >> +void IPU3Awb::Initialise(ipu3_uapi_params &params)
> >> +{
> >> +	memset(&params, 0, sizeof(params));
> >> +	memset(wbGains_, 8192, sizeof(wbGains_));
> > 
> > memset() initializes the memory byte by byte, this will not store 8192
> > in each entry of the array.
> > 
> 
> Indeed... :-/
> 
> >> +	wbGains_[0] = 8192 * 0.8;
> >> +	wbGains_[3] = 8192 * 0.8;
> > 
> > I would group the initialization of member variables together, either
> > before, or after the initialization of params.
> > 
> >> +	params.use.acc_awb = 1;
> >> +	/*\todo fill the grid calculated based on BDS configuration */
> >> +	params.acc_param.awb.config = imgu_css_awb_defaults;
> >> +
> >> +	params.use.acc_bnr = 1;
> >> +	params.acc_param.bnr = imgu_css_bnr_defaults;
> >> +
> >> +	params.use.acc_ccm = 1;
> >> +	params.acc_param.ccm = imgu_css_ccm_3800k;
> >> +
> >> +	params.use.acc_gamma = 1;
> >> +	params.acc_param.gamma.gc_ctrl.enable = 1;
> >> +
> >> +	uint32_t a = (32 * 245) / (245 - 9);
> >> +
> >> +	for (uint32_t i = 0; i < 10; i++)
> >> +		params.acc_param.gamma.gc_lut.lut[i] = 0;
> >> +	for (uint32_t i = 10; i < 245; i++)
> >> +		params.acc_param.gamma.gc_lut.lut[i] = a * i + (0 - a * 9);
> >> +	for (uint32_t i = 245; i < 255; i++)
> >> +		params.acc_param.gamma.gc_lut.lut[i] = 32 * 245;
> >> +
> >> +	frame_count_ = 0;
> >> +	algoConverged_ = false;
> >> +}
> >> +
> >> +unsigned int IPU3Awb::GetConvergenceFrames() const
> >> +{
> >> +	// If colour gains have been explicitly set, there is no convergence
> >> +	// to happen, so no need to drop any frames - return zero.
> >> +	if (manual_r_ && manual_b_)
> >> +		return 0;
> >> +	else
> >> +		return config_.convergence_frames;
> >> +}
> >> +
> >> +void IPU3Awb::SetMode(std::string const &mode_name)
> >> +{
> >> +	mode_name_ = mode_name;
> >> +}
> >> +
> >> +void IPU3Awb::SetManualGains(double manual_r, double manual_b)
> >> +{
> >> +	// If any of these are 0.0, we swich back to auto.
> >> +	manual_r_ = manual_r;
> >> +	manual_b_ = manual_b;
> >> +}
> >> +
> >> +uint32_t IPU3Awb::estimateCCT(uint8_t R, uint8_t G, uint8_t B)
> >> +{
> >> +	double X = (-0.14282) * (R) + (1.54924) * (G) + (-0.95641) * (B);
> >> +	double Y = (-0.32466) * (R) + (1.57837) * (G) + (-0.73191) * (B);
> >> +	double Z = (-0.68202) * (R) + (0.77073) * (G) + (0.56332) * (B);
> >> +
> >> +	double x = X / (X + Y + Z);
> >> +	double y = Y / (X + Y + Z);
> >> +
> >> +	double n = (x - 0.3320) / (0.1858 - y);
> >> +	return static_cast<uint32_t>(449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33);
> >> +}
> >> +
> >> +void IPU3Awb::calculateWBGains([[maybe_unused]] Rectangle roi, const ipu3_uapi_stats_3a *stats)
> > 
> > Line wrap.
> > 
> >> +{
> >> +	if (algoConverged_)
> >> +		return;
> > 
> > So once it converges, you never update the gains anymore, even if the
> > scene changes ?
> > 
> 
> This is an error, I copy/pasted it to ipu3 and it should be called only
> when AE has converged. If there is a sufficient scene change, then AE
> will diverge and so it will be called again after the new convergence
> (does it make sense :-) ?).

Do you mean this should be

	if (!AE has converged)
		return;

? It sounds sensible. I'm sure we'll find out we'll need something more
complex, but for now I think it's good enough :-)

> >> +
> >> +	std::vector<uint32_t> R_v, Gr_v, Gb_v, B_v;
> > 
> > camelCase. Feel free to also make the name slightly longer to make the
> > more explicit. redValues ?
> > 
> >> +	Point topleft = roi.topLeft();
> >> +	uint32_t startY = (topleft.y / 16) * 160 * 8;
> >> +	uint32_t startX = (topleft.x / 8) * 8;
> >> +	uint32_t endX = (startX + (roi.size().width / 8)) * 8;
> >> +
> >> +	for (uint32_t j = (topleft.y / 16); j < (topleft.y / 16) + (roi.size().height / 16); j++) {
> >> +		for (uint32_t i = startX + startY; i < endX + startY; i += 8) {
> >> +			Gr_v.push_back(stats->awb_raw_buffer.meta_data[i]);
> >> +			R_v.push_back(stats->awb_raw_buffer.meta_data[i + 1]);
> >> +			B_v.push_back(stats->awb_raw_buffer.meta_data[i + 2]);
> >> +			Gb_v.push_back(stats->awb_raw_buffer.meta_data[i + 3]);
> >> +		}
> >> +	}
> >> +
> >> +	std::sort(R_v.begin(), R_v.end());
> >> +	std::sort(Gr_v.begin(), Gr_v.end());
> >> +	std::sort(B_v.begin(), B_v.end());
> >> +	std::sort(Gb_v.begin(), Gb_v.end());
> >> +
> >> +	double Grmed = Gr_v[Gr_v.size() / 2];
> >> +	double Rmed = R_v[R_v.size() / 2];
> >> +	double Bmed = B_v[B_v.size() / 2];
> >> +	double Gbmed = Gb_v[Gb_v.size() / 2];
> >> +
> >> +	double Rgain = Grmed / Rmed;
> >> +	double Bgain = Gbmed / Bmed;
> >> +	LOG(IPU3Awb, Debug) << "max R, Gr, B, Gb: "
> >> +			    << R_v.back() << ","
> >> +			    << Gr_v.back() << ","
> >> +			    << B_v.back() << ","
> >> +			    << Gb_v.back();
> >> +	tint_ = ((Rmed / Grmed) + (Bmed / Gbmed)) / 2;
> >> +
> >> +	/* \todo Those are corrections when light is really low
> >> +	 * it should be taken into account by AGC somehow */
> >> +	if ((Rgain >= 2) && (Bgain < 2)) {
> >> +		wbGains_[0] = 4096 * tint_;
> >> +		wbGains_[1] = 8192 * Rgain;
> >> +		wbGains_[2] = 4096 * Bgain;
> >> +		wbGains_[3] = 4096 * tint_;
> >> +	} else if ((Bgain >= 2) && (Rgain < 2)) {
> >> +		wbGains_[0] = 4096 * tint_;
> >> +		wbGains_[1] = 4096 * Rgain;
> >> +		wbGains_[2] = 8192 * Bgain;
> >> +		wbGains_[3] = 4096 * tint_;
> >> +	} else {
> >> +		wbGains_[0] = 8192 * tint_;
> >> +		wbGains_[1] = 8192 * Rgain;
> >> +		wbGains_[2] = 8192 * Bgain;
> >> +		wbGains_[3] = 8192 * tint_;
> >> +	}
> >> +
> >> +	frame_count_++;
> >> +
> >> +	cct_ = estimateCCT(Rmed, (Grmed + Gbmed) / 2, Bmed);
> >> +
> >> +	algoConverged_ = true;
> >> +}
> >> +
> >> +void IPU3Awb::updateBNR(ipu3_uapi_params &params)
> > 
> > updateBNR() isn't a great name for a function that updates the WB gains,
> > even if they're part of the Bayer noise reduction block of data.
> > 
> > Should we push the ipu3_uapi_params structure down to individual
> > algorithms, or would it be better to decouple it, creating custom
> > structures to hold algorithm results, and fillin ipu3_uapi_params in
> > IPAIPU3::fillParams() instead ? On one hand moving ipu3_uapi_params out
> > will make the code more generic, and thus more reusable, but on the
> > other hand we would still depend on a custom stats format, so it may not
> > be worth it.
> 
> That is indeed a blocker here.
> I will rename the function, but I think we need to let AWB fill the
> parameters related to it and not directly filling those in
> IPAIPU3::fillParams().
> 
> >> +{
> >> +	if (!algoConverged_)
> >> +		return;
> >> +
> >> +	params.acc_param.bnr.wb_gains.gr = wbGains_[0];
> >> +	params.acc_param.bnr.wb_gains.r = wbGains_[1];
> >> +	params.acc_param.bnr.wb_gains.b = wbGains_[2];
> >> +	params.acc_param.bnr.wb_gains.gb = wbGains_[3];
> >> +	if (cct_ < 5500)
> >> +		params.acc_param.ccm = imgu_css_ccm_3800k;
> >> +	else
> >> +		params.acc_param.ccm = imgu_css_ccm_6000k;
> >> +}
> >> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> >> new file mode 100644
> >> index 00000000..06389020
> >> --- /dev/null
> >> +++ b/src/ipa/ipu3/ipu3_awb.h
> >> @@ -0,0 +1,130 @@
> >> +/* SPDX-License-Identifier: BSD-2-Clause */
> >> +/*
> >> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> >> + *
> >> + * awb.h - AWB control algorithm
> >> + */
> >> +#ifndef __LIBCAMERA_IPU3_AWB_H__
> >> +#define __LIBCAMERA_IPU3_AWB_H__
> >> +
> >> +#include <linux/intel-ipu3.h>
> >> +
> >> +#include <libcamera/geometry.h>
> >> +
> >> +#include <libcamera/ipa/awb_algorithm.h>
> >> +#include <libcamera/ipa/ipa_controller.h>
> >> +
> >> +namespace libcamera {
> >> +
> >> +const struct ipu3_uapi_bnr_static_config imgu_css_bnr_defaults = {
> >> +	{ 16, 16, 16, 16 },			/* wb_gains */
> >> +	{ 255, 255, 255, 255 },			/* wb_gains_thr */
> >> +	{ 0, 0, 8, 6, 0, 14 },			/* thr_coeffs */
> >> +	{ 0, 0, 0, 0 },				/* thr_ctrl_shd */
> >> +	{ -648, 0, -366, 0 },			/* opt_center */
> >> +	{					/* 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 }
> >> +	},
> >> +	{ 4, 0, 1, 8, 0, 8, 0, 8, 0 },		/* bp_ctrl */
> >> +	{ 8, 4, 4, 0, 8, 0, 1, 1, 1, 1, 0 },	/* dn_detect_ctrl */
> >> +	1296,
> >> +	{ 419904, 133956 },
> >> +};
> >> +
> >> +/* settings for Auto White Balance */
> >> +const struct ipu3_uapi_awb_config_s imgu_css_awb_defaults = {
> >> +	8191,
> >> +	8191,
> >> +	8191,
> >> +	8191 | /* rgbs_thr_gr/r/gb/b */
> >> +		IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
> >> +	.grid = {
> >> +		.width = 160,
> >> +		.height = 45,
> >> +		.block_width_log2 = 3,
> >> +		.block_height_log2 = 4,
> >> +		.x_start = 0,
> >> +		.y_start = 0,
> >> +	},
> >> +};
> >> +
> >> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_6000k = {
> >> +	7239, -750, -37, 0,
> >> +	-215, 9176, -200, 0,
> >> +	-70, -589, 6810, 0
> >> +};
> >> +
> >> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_4900k = {
> >> +	7811, -464, -466, 0,
> >> +	-635, 8762, -533, 0,
> >> +	-469, -154, 6583, 0
> >> +};
> >> +
> >> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_3800k = {
> >> +	7379, -526, -296, 0,
> >> +	-411, 7397, -415, 0,
> >> +	-224, -564, 7244, 0
> >> +};
> > 
> > All these should be moved to ipu3_awb.cpp, and be made static. Otherwise
> > they would get duplicated in every compilation unit in which this header
> > is included.
> > 
> >> +
> >> +struct AwbConfig {
> >> +	// Only repeat the AWB calculation every "this many" frames
> >> +	uint16_t frame_period;
> >> +	// number of initial frames for which speed taken as 1.0 (maximum)
> >> +	uint16_t startup_frames;
> >> +	unsigned int convergence_frames; // approx number of frames to converge
> >> +	double speed; // IIR filter speed applied to algorithm results
> >> +};
> >> +
> >> +#if 0
> >> +typedef struct awb_public_set_item{
> >> +    unsigned char Gr_avg;
> >> +    unsigned char R_avg;
> >> +    unsigned char B_avg;
> >> +    unsigned char Gb_avg;
> >> +    unsigned char sat_ratio;
> >> +    unsigned char padding0; /**< Added the padding so that the public matches that private */
> >> +    unsigned char padding1; /**< Added the padding so that the public matches that private */
> >> +    unsigned char padding2; /**< Added the padding so that the public matches that private */
> >> +} awb_public_set_item_t;
> >> +#endif
> > 
> > If the compiler doesn't need to see this, I think you can drop it
> > completely :-)
> 
> I will remove them now, thanks !
> It will be used later as this is how the stats are organized internally.
> 
> >> +
> >> +class IPU3Awb : public AwbAlgorithm
> >> +{
> >> +public:
> >> +	IPU3Awb(IPAController *controller = NULL);
> > 
> > This is C++, so nullptr :-) But there's no need for a default value, as
> > the controller parameter should either be mandatory, or removed
> > completely.
> > 
> >> +	~IPU3Awb();
> > 
> > Blank line.
> > 
> >> +	virtual char const *Name() const override;
> > 
> > This is never used, you can drop it from the base class. When (and if)
> > we'll need this, I'd rather pass the name to the base class constructor,
> > store it there, and implement name() in the base class only, as a
> > non-virtual function.
> > 
> >> +	virtual void Initialise() override;
> > 
> > No need to repeat the virtual keyword for overridden functions.
> > 
> >> +	void Initialise(ipu3_uapi_params &params);
> > 
> > This isn't very nice, as it's not aligned with the design of the
> > IPAAlgorithm class, but we can fix it later. I would move the function
> > down, after the mandatory functions, with calculateWBGains() and
> > updateBNR().
> > 
> >> +	unsigned int GetConvergenceFrames() const override;
> >> +	void SetMode(std::string const &name) override;
> >> +	void SetManualGains(double manual_r, double manual_b) override;
> > 
> > A blank line here would be nice.
> > 
> >> +	void calculateWBGains(Rectangle roi,
> > 
> > const Rectangle &roi
> > 
> >> +			      const ipu3_uapi_stats_3a *stats);
> >> +	void updateBNR(ipu3_uapi_params &params);
> >> +
> >> +private:
> >> +	uint32_t estimateCCT(uint8_t R, uint8_t G, uint8_t B);
> > 
> > r, g and b.
> > 
> >> +
> >> +	/* configuration is read-only, and available to both threads */
> > 
> > What threads ? :-)
> 
> Yes, which ones ?? ;-)
> Well, as in RPi, there will be threads later, if the algorithms are too
> complicated (they probably will be)...
> 
> >> +	AwbConfig config_;
> >> +	std::string mode_name_;
> > 
> > Never used.
> > 
> >> +	/* manual r setting */
> >> +	double manual_r_;
> > 
> > manualRedGain_ ?
> > 
> >> +	/* manual b setting */
> >> +	double manual_b_;
> >> +	/* WB calculated gains */
> >> +	uint16_t wbGains_[4];
> >> +	double tint_;
> >> +	uint32_t cct_;
> >> +
> >> +	uint32_t frame_count_;
> >> +
> >> +	bool algoConverged_;
> >> +};
> >> +} /* namespace libcamera */
> >> +
> >> +#endif /* __LIBCAMERA_IPU3_AWB_H__ */
> >> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> >> index a241f617..43ad0e0d 100644
> >> --- a/src/ipa/ipu3/meson.build
> >> +++ b/src/ipa/ipu3/meson.build
> >> @@ -2,8 +2,14 @@
> >>  
> >>  ipa_name = 'ipa_ipu3'
> >>  
> >> +ipu3_ipa_sources = files([
> >> +  'ipu3.cpp',
> >> +  'ipu3_agc.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,
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index ff980b38..3809c943 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -825,6 +825,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >>  	 */
> >>  	double lineDuration = sensorInfo.lineLength
> >>  			    / (sensorInfo.pixelRate / 1e6);
> >> +	LOG(IPU3, Error) << "line duration: " << lineDuration;
> > 
> > Is this an error ? :-)
>
> Ahem ->[]
>
> >>  	const ControlInfoMap &sensorControls = sensor->controls();
> >>  	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
> >>  	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;

Hi Jean-Michel,

On Mon, Feb 22, 2021 at 09:32:02PM +0100, Jean-Michel Hautbois wrote:
> On 22/02/2021 19:50, Dafna Hirschfeld wrote:
> > On 22.02.21 05:56, Laurent Pinchart wrote:
> >> On Fri, Feb 19, 2021 at 06:22:24PM +0100, Jean-Michel Hautbois wrote:
> >>> Inherit from the AWBAlgorithm and AGCAlgorithm classes to implement
> >>> basic functions.
> >>> While exposure is not locked, AWB is not calculated and corrected.
> >>> Once AWB is done, a color temperature is estimated and default CCM
> >>> matrices
> >>> are used.
> >>> Implement a basic "grey-world" AWB algorithm just for demonstration
> >>> purpose.
> > 
> > Are those algorithms specific to IPU3 ?
> > If not then maybe it can used by rkisp1 as well?
> > Maybe it worth to think of general API for basic 3a implementations that
> > other IPAs can use?
> 
> Not at all ! And I will send a second patch version which will show how
> to implement the image processing algorithm, and indeed it aims to be
> platform agnostic (even if the parameters and statistics are always
> specific ;-)) !
> 
> >> Nice to see an initial implementation ! I'll provide a handful of review
> >> comments below already. Some are related to coding style or similar
> >> issues and should be addressed in v2. Others are related to the IPA
> >> architecture, and could be fixed on top of this series to avoid delaying
> >> it unnecessarily. They should in that case be captured in a \todo
> >> comment.
> >>
> >> As for 1/2, I won't repeat the same comments all over the patch, so
> >> please apply them wherever they're applicable.
> >>
> >>> Signed-off-by: Jean-Michel Hautbois
> >>> <jeanmichel.hautbois@ideasonboard.com>
> >>> ---
> >>>   src/ipa/ipu3/ipu3.cpp                |  31 ++++-
> >>>   src/ipa/ipu3/ipu3_agc.cpp            | 195 +++++++++++++++++++++++++++
> >>>   src/ipa/ipu3/ipu3_agc.h              |  96 +++++++++++++
> >>>   src/ipa/ipu3/ipu3_awb.cpp            | 182 +++++++++++++++++++++++++
> >>>   src/ipa/ipu3/ipu3_awb.h              | 130 ++++++++++++++++++
> >>>   src/ipa/ipu3/meson.build             |   8 +-
> >>>   src/libcamera/pipeline/ipu3/ipu3.cpp |   1 +
> >>>   7 files changed, 638 insertions(+), 5 deletions(-)
> >>>   create mode 100644 src/ipa/ipu3/ipu3_agc.cpp
> >>>   create mode 100644 src/ipa/ipu3/ipu3_agc.h
> >>>   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 b63e58be..c3859f39 100644
> >>> --- a/src/ipa/ipu3/ipu3.cpp
> >>> +++ b/src/ipa/ipu3/ipu3.cpp
> >>> @@ -13,6 +13,7 @@
> >>>     #include <libcamera/buffer.h>
> >>>   #include <libcamera/control_ids.h>
> >>> +#include <libcamera/ipa/ipa_controller.h>
> >>>   #include <libcamera/ipa/ipa_interface.h>
> >>>   #include <libcamera/ipa/ipa_module_info.h>
> >>>   #include <libcamera/ipa/ipu3_ipa_interface.h>
> >>> @@ -21,6 +22,9 @@
> >>>   #include "libcamera/internal/buffer.h"
> >>>   #include "libcamera/internal/log.h"
> >>>   +#include "ipu3_agc.h"
> >>> +#include "ipu3_awb.h"
> >>> +
> >>>   namespace libcamera {
> >>>     LOG_DEFINE_CATEGORY(IPAIPU3)
> >>> @@ -28,6 +32,9 @@ LOG_DEFINE_CATEGORY(IPAIPU3)
> >>>   class IPAIPU3 : public ipa::ipu3::IPAIPU3Interface
> >>>   {
> >>>   public:
> >>> +    IPAIPU3()
> >>> +        : controller_() {}
> >>
> >>         : controller_()
> >>     {
> >>     }
> >>
> >> I may not repeat all the comments from 1/2, but they're generally
> >> applicable to this patch as well.
> >>
> >>> +
> >>>       int init([[maybe_unused]] const IPASettings &settings) override
> >>>       {
> >>>           return 0;
> >>> @@ -60,6 +67,11 @@ private:
> >>>       uint32_t gain_;
> >>>       uint32_t minGain_;
> >>>       uint32_t maxGain_;
> >>> +
> >>> +    IPAController controller_;
> >>> +    IPU3Awb *awbAlgo_;
> >>> +    IPU3Agc *agcAlgo_;
> >>> +    ipu3_uapi_params params_;
> >>>   };
> >>>     void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap>
> >>> &entityControls)
> >>> @@ -83,11 +95,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_ = 123;
> >>
> >> A peculiar value :-)
> >>
> >>>         minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
> >>>       maxGain_ = itGain->second.max().get<int32_t>();
> >>> -    gain_ = maxGain_;
> >>> +    gain_ = 1;
> >>> +
> >>> +    awbAlgo_ = new IPU3Awb(&controller_);
> >>> +    awbAlgo_->Initialise(params_);
> >>> +    agcAlgo_ = new IPU3Agc(&controller_);
> >>> +
> >>> +    /*\todo not used yet... */
> >>> +    controller_.Initialise();
> >>>         setControls(0);
> >>>   }
> >>> @@ -162,10 +181,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));
> >>> +    awbAlgo_->updateBNR(params_);
> >>> +    memcpy(params, &params_, sizeof(*params));
> >>
> >>     *params = params_;
> >>
> >>>         /* \todo Fill in parameters buffer. */
> >>> -
> >>>       ipa::ipu3::IPU3Action op;
> >>>       op.op = ipa::ipu3::ActionParamFilled;
> >>>   @@ -179,6 +198,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> >>>         /* \todo React to statistics and update internal state
> >>> machine. */
> >>>       /* \todo Add meta-data information to ctrls. */
> >>> +    agcAlgo_->Process(stats, exposure_, gain_);
> >>> +    if (agcAlgo_->Converged())
> >>> +        awbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400),
> >>> stats);
> >>> +    setControls(frame);
> >>>         ipa::ipu3::IPU3Action op;
> >>>       op.op = ipa::ipu3::ActionMetadataReady;
> >>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> >>> new file mode 100644
> >>> index 00000000..db591e33
> >>> --- /dev/null
> >>> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> >>> @@ -0,0 +1,195 @@
> >>> +/* SPDX-License-Identifier: BSD-2-Clause */
> >>> +/*
> >>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> >>> + *
> >>> + * agc.cpp - AGC/AEC control algorithm
> >>> + */
> >>> +#include <iostream>
> >>> +#include <numeric>
> >>> +#include <unordered_map>
> >>> +
> >>> +#include "libcamera/internal/log.h"
> >>> +
> >>> +#include "ipu3_agc.h"
> >>> +
> >>> +using namespace libcamera;
> >>> +
> >>> +LOG_DEFINE_CATEGORY(IPU3Agc)
> >>> +
> >>> +#define NAME "ipu3.agc"
> >>> +
> >>> +IPU3Agc::IPU3Agc(IPAController *controller)
> >>> +    : AgcAlgorithm(controller), frameCount_(0),
> >>> +      ev_(1.0), flicker_period_(0.0),
> >>> +      max_shutter_(0), fixed_shutter_(0),
> >>> +      fixed_analogue_gain_(0.0), algoConverged_(false)
> >>> +{
> >>> +}
> >>> +
> >>> +char const *IPU3Agc::Name() const
> >>> +{
> >>> +    return NAME;
> >>> +}
> >>> +
> >>> +unsigned int IPU3Agc::GetConvergenceFrames() const
> >>> +{
> >>> +    return config_.convergence_frames;
> >>> +}
> >>> +
> >>> +void IPU3Agc::SetEv(double ev)
> >>> +{
> >>> +    ev_ = ev;
> >>> +}
> >>> +
> >>> +void IPU3Agc::SetFlickerPeriod(double flicker_period)
> >>> +{
> >>> +    flicker_period_ = flicker_period;
> >>> +}
> >>> +
> >>> +void IPU3Agc::SetMaxShutter(double max_shutter)
> >>> +{
> >>> +    max_shutter_ = max_shutter;
> >>> +}
> >>> +
> >>> +void IPU3Agc::SetFixedShutter(double fixed_shutter)
> >>> +{
> >>> +    fixed_shutter_ = fixed_shutter;
> >>> +}
> >>> +
> >>> +void IPU3Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
> >>> +{
> >>> +    fixed_analogue_gain_ = fixed_analogue_gain;
> >>> +}
> >>> +
> >>> +void IPU3Agc::SetMeteringMode(std::string const &metering_mode_name)
> >>> +{
> >>> +    metering_mode_name_ = metering_mode_name;
> >>> +}
> >>> +
> >>> +void IPU3Agc::SetExposureMode(std::string const &exposure_mode_name)
> >>> +{
> >>> +    exposure_mode_name_ = exposure_mode_name;
> >>> +}
> >>> +
> >>> +void IPU3Agc::Prepare() {}
> >>> +
> >>> +void IPU3Agc::Process() {}
> >>> +
> >>> +/* \todo This function is taken from numerical recipes and
> >>> calculates all moments
> >>> + * It needs to be rewritten properly and maybe in a "math" class ? */
> >>
> >> /*
> >>   * \todo This function is taken from numerical recipes and calculates
> >> all
> >>   * moments. It needs to be rewritten properly and maybe in a "math"
> >> class ?
> >>   */
> >>
> >>> +void IPU3Agc::moments(std::unordered_map<uint32_t, uint32_t> &data,
> >>> int n)
> > 
> > 'data' is the hist, so why not call it 'hist' here?
> 
> Yes, why not :-) thanks !
> 
> >>> +{
> >>> +    int j;
> >>> +    double ep = 0.0, s, p;
> >>> +    double ave, adev, sdev;
> >>> +    double var, skew, curt;
> >>> +
> >>> +    s = 0.0;
> >>> +    for (j = 1; j <= n; j++)
> >>> +        s += data[j];
> > 
> > shouldn't that be:
> > s += data[j] * j
> > 
> > since data[j] is the number of pixels with value j
> 
> I don't think so, no, but I will have to verify...

I would have agreed with Dafna here. If the code is correct, then maybe
more explicit names for the variables, and/or comments, could help
explaining what's going on.

> >>> +
> >>> +    ave = s / n;
> >>> +    adev = var = skew = curt = 0.0;
> >>> +
> >>> +    for (j = 1; j <= n; j++) {
> >>> +        adev += s = data[j] - (ave);
> >>> +        ep += s;
> >>> +        var += (p = s * s);
> >>> +        skew += (p *= s);
> >>> +        curt += (p *= s);
> >>> +    }
> >>> +
> >>> +    adev /= n;
> >>> +    var = (var - ep * ep / n) / (n - 1);
> >>> +    sdev = std::sqrt(var);
> >>> +
> >>> +    if (var) {
> >>> +        skew /= n * var * sdev;
> >>> +        curt = curt / (n * var * var) - 3.0;
> >>> +    }
> >>> +    skew_ = skew;
> >>> +}
> >>> +
> >>> +void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> >>> +{
> >>> +    brightnessVec_.clear();
> >>> +
> >>> +    /*\todo Replace constant values with real BDS configuration */
> >>> +    for (uint32_t j = 0; j < 45; j++) {
> >>> +        for (uint32_t i = 0; i < 160 * 45 * 8; i += 8) {
> >>> +            uint8_t Gr = stats->awb_raw_buffer.meta_data[i];
> >>> +            uint8_t R = stats->awb_raw_buffer.meta_data[i + 1];
> >>> +            uint8_t B = stats->awb_raw_buffer.meta_data[i + 2];
> >>> +            uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3];
> >>> +            brightnessVec_.push_back(static_cast<uint32_t>(0.299 * R
> >>> + 0.587 * (Gr + Gb) / 2 + 0.114 * B));
> > 
> > You iterate on j in the outer loop but don't use j anywhere for
> > indexing, this is a bit suspicious
> 
> This is just for an easier splitting of rectangles (taken from the AWB
> algorithm).

Is the aim really to repeat the same inner loop 45 times ?

> >>> +        }
> >>> +    }
> >>> +    std::sort(brightnessVec_.begin(), brightnessVec_.end());
> > 
> > I don't think the sort is is necessary
> 
> It is there from an old calculation of median value. Not needed indeed.
> 
> >>> +
> >>> +    /* \todo create a class to generate histograms ! */
> >>> +    std::unordered_map<uint32_t, uint32_t> hist;
> > 
> > you want to have up to 256 indexes so you need to define with uint8_t
> > 
> > std::unordered_map<uint8_t, uint32_t> hist;
> 
> ack
> 
> >>> +    for (uint32_t const &val : brightnessVec_)
> >>> +        hist[val]++;
> >>> +    moments(hist, 256);
> >>> +}
> >>> +
> >>> +void IPU3Agc::lockExposure(uint32_t &exposure, uint32_t &gain)
> >>> +{
> >>> +    /* Algorithm initialization wait for first valid frames */
> >>> +    /* \todo - have a number of frames given by DelayedControls ?
> >>> +     * - implement a function for IIR */
> >>> +    if (frameCount_ == 4) {
> >>> +        prevExposure_ = exposure;
> >>> +
> >>> +        prevSkew_ = skew_;
> >>> +        /* \tdo use configured values */
> >>
> >> s/tdo/todo/
> >>
> >>> +        exposure = 800;
> >>> +        gain = 8;
> >>> +        currentExposure_ = exposure;
> >>> +    } else if (frameCount_ == 8) {
> >>> +        currentSkew_ = skew_;
> >>> +        exposure = ((currentExposure_ * prevSkew_) + (prevExposure_
> >>> * currentSkew_)) / (prevSkew_ + currentSkew_);
> >>
> >> Line wrap. No need for the inner parentheses.
> >>
> >>> +        nextExposure_ = exposure;
> >>> +        lastFrame_ = frameCount_;
> >>> +    } else if ((frameCount_ >= 12) && (frameCount_ - lastFrame_ >=
> >>> 4)) {
> >>
> >> Could you define constants for magic values, to make the code more
> >> readable ? They can be defined as static constexpr members of the
> >> IPU3Agc class, and should be named with camelCase. While we don't do so
> >> yet in many places, it's customary to start them with a 'k' prefix (e.g.
> >> kInitialFrameSkipCount).
> >>
> >>> +        currentSkew_ = skew_;
> >>> +        /* \todo properly calculate a gain */
> >>> +        if (frameCount_ == 12)
> >>> +            gain = ((8 * prevSkew_) + (1 * currentSkew_)) /
> >>> (prevSkew_ + currentSkew_);
> >>> +
> >>> +        if (currentSkew_ - prevSkew_ > 1) {
> >>> +            /* under exposed */
> >>> +            prevExposure_ = nextExposure_;
> >>> +            exposure = ((currentExposure_ * prevSkew_) +
> >>> (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
> >>> +            nextExposure_ = exposure;
> >>> +        } else if (currentSkew_ - prevSkew_ < -1) {
> >>> +            /* over exposed */
> >>> +            currentExposure_ = nextExposure_;
> >>> +            exposure = ((currentExposure_ * prevSkew_) +
> >>> (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
> >>> +            nextExposure_ = exposure;
> >>> +        } else {
> >>> +            /* we have converged */
> >>> +            algoConverged_ = true;
> >>> +        }
> >>> +        lastFrame_ = frameCount_;
> >>> +        prevSkew_ = currentSkew_;
> >>> +    }
> > 
> > Could you explain the general idea behind this algorithm or give a link
> > where it is explained?
> 
> Yes, I will, and this algorithm will evolve as I want to use Q3-Q1
> inter-quartile measure to optimize auto exposure.
> 
> >>> +}
> >>> +
> >>> +void IPU3Agc::Process(const ipu3_uapi_stats_3a *stats, uint32_t
> >>> &exposure, uint32_t &gain)
> >>> +{
> >>> +    processBrightness(stats);
> >>> +    if (!algoConverged_)
> >>> +        lockExposure(exposure, gain);
> >>> +    else {
> >>> +        /* Are we still well exposed ? */
> >>> +        if ((skew_ < 2) || (skew_ > 4))
> >>> +            algoConverged_ = false;
> >>> +    }
> >>> +    frameCount_++;
> >>> +}
> >>> +
> >>> +bool IPU3Agc::Converged()
> >>> +{
> >>> +    return algoConverged_;
> >>> +}
> >>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> >>> new file mode 100644
> >>> index 00000000..9a69d628
> >>> --- /dev/null
> >>> +++ b/src/ipa/ipu3/ipu3_agc.h
> >>> @@ -0,0 +1,96 @@
> >>> +/* SPDX-License-Identifier: BSD-2-Clause */
> >>> +/*
> >>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> >>> + *
> >>> + * ipu3_agc.h - AGC/AEC control algorithm
> >>> + */
> >>> +#ifndef __LIBCAMERA_IPU3_AGC_H__
> >>> +#define __LIBCAMERA_IPU3_AGC_H__
> >>> +
> >>
> >> #include <map>
> >>
> >>> +#include <mutex>
> >>
> >> This doesn't seem to be needed.
> >>
> >>> +#include <vector>
> >>> +
> >>> +#include <linux/intel-ipu3.h>
> >>> +
> >>> +#include <libcamera/geometry.h>
> >>> +
> >>> +#include <libcamera/ipa/agc_algorithm.h>
> >>> +#include <libcamera/ipa/ipa_controller.h>
> >>> +
> >>> +#define AGC_STATS_SIZE 15
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +struct AgcMeteringMode {
> >>> +    double weights[AGC_STATS_SIZE];
> >>> +    Rectangle metering_region[AGC_STATS_SIZE];
> >>> +};
> >>> +
> >>> +struct AgcExposureMode {
> >>> +    std::vector<double> shutter;
> >>> +    std::vector<double> gain;
> >>> +};
> >>> +
> >>> +struct AgcConfig {
> >>> +    std::map<std::string, AgcMeteringMode> metering_modes;
> >>> +    std::map<std::string, AgcExposureMode> exposure_modes;
> >>> +    double speed;
> >>> +    uint16_t startup_frames;
> >>> +    unsigned int convergence_frames;
> >>> +    std::string default_metering_mode;
> >>> +    std::string default_exposure_mode;
> >>> +    double default_exposure_time;
> >>> +    double default_analogue_gain;
> >>> +};
> >>> +
> >>> +class IPU3Agc : public AgcAlgorithm
> >>> +{
> >>> +public:
> >>> +    IPU3Agc(IPAController *IPAcontroller);
> >>
> >> Blank line.
> >>
> >>> +    char const *Name() const override;
> >>> +    unsigned int GetConvergenceFrames() const override;
> >>> +    void SetEv(double ev) override;
> >>> +    void SetFlickerPeriod(double flicker_period) override;
> >>> +    void SetMaxShutter(double max_shutter) override; // microseconds
> >>> +    void SetFixedShutter(double fixed_shutter) override; //
> >>> microseconds
> >>> +    void SetFixedAnalogueGain(double fixed_analogue_gain) override;
> >>> +    void SetMeteringMode(std::string const &metering_mode_name)
> >>> override;
> >>> +    void SetExposureMode(std::string const &exposure_mode_name)
> >>> override;
> >>> +    void Prepare() override;
> >>> +    void Process() override;
> >>> +    void Process(const ipu3_uapi_stats_3a *stats, uint32_t
> >>> &exposure, uint32_t &gain);
> >>
> >> The last two arguments should be replaced by a generic data container,
> >> similar to the Metadata class in the RPi IPA (not something to fix now).
> >>
> >>> +    bool Converged();
> >>> +
> >>> +private:
> >>> +    void moments(std::unordered_map<uint32_t, uint32_t> &data, int n);
> >>> +    void processBrightness(const ipu3_uapi_stats_3a *stats);
> >>> +    void lockExposure(uint32_t &exposure, uint32_t &gain);
> >>> +
> >>> +    AgcConfig config_;
> >>
> >> This is never set, and only one member is read. I'd drop it for now.
> >>
> >>> +    std::string metering_mode_name_;
> >>> +    std::string exposure_mode_name_;
> >>> +    uint64_t frameCount_;
> >>> +    uint64_t lastFrame_;
> >>> +
> >>> +    /* Vector of calculated brightness for each cell */
> >>> +    std::vector<uint32_t> brightnessVec_;
> >>
> >> We avoid encoding the type in the variable name. Would this be better
> >> named cellsBrightness_ ?
> >>
> >>> +    double ev_;
> >>
> >> Never used. Same for other members below.
> >>
> >>> +    double flicker_period_;
> >>> +    double max_shutter_;
> >>> +    double fixed_shutter_;
> >>> +    double fixed_analogue_gain_;
> >>> +
> >>> +    /* Values for filtering */
> >>> +    uint32_t prevExposure_;
> >>> +    uint32_t currentExposure_;
> >>> +    uint32_t nextExposure_;
> >>> +
> >>> +    double skew_;
> >>> +    double prevSkew_;
> >>> +    double currentSkew_;
> >>> +    bool algoConverged_;
> >>
> >> Maybe just converged_ ?
> >>
> >>> +};
> >>> +
> >>> +} /* namespace libcamera */
> >>> +
> >>> +#endif /* __LIBCAMERA_IPU3_AGC_H__ */
> >>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> >>> new file mode 100644
> >>> index 00000000..21286e4c
> >>> --- /dev/null
> >>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> >>> @@ -0,0 +1,182 @@
> >>> +/* SPDX-License-Identifier: BSD-2-Clause */
> >>> +/*
> >>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> >>> + *
> >>> + * ipu3_awb.cpp - AWB control algorithm
> >>> + */
> >>> +#include <iostream>
> >>
> >> I don't think this is needed.
> >>
> >>> +#include <numeric>
> >>> +#include <unordered_map>
> >>> +
> >>> +#include "libcamera/internal/log.h"
> >>> +
> >>> +#include "ipu3_awb.h"
> >>> +
> >>> +using namespace libcamera;
> >>> +
> >>> +LOG_DEFINE_CATEGORY(IPU3Awb)
> >>> +
> >>> +#define NAME "ipu3.awb"
> >>> +
> >>> +IPU3Awb::IPU3Awb(IPAController *controller)
> >>> +    : AwbAlgorithm(controller)
> >>> +{
> >>> +}
> >>> +
> >>> +IPU3Awb::~IPU3Awb()
> >>> +{
> >>> +}
> >>> +
> >>> +char const *IPU3Awb::Name() const
> >>> +{
> >>> +    return NAME;
> >>> +}
> >>> +
> >>> +void IPU3Awb::Initialise() {}
> >>
> >> void IPU3Awb::Initialise()
> >> {
> >> }
> >>
> >>> +
> >>> +void IPU3Awb::Initialise(ipu3_uapi_params &params)
> >>> +{
> >>> +    memset(&params, 0, sizeof(params));
> >>> +    memset(wbGains_, 8192, sizeof(wbGains_));
> >>
> >> memset() initializes the memory byte by byte, this will not store 8192
> >> in each entry of the array.
> >>
> >>
> >>> +    wbGains_[0] = 8192 * 0.8;
> >>> +    wbGains_[3] = 8192 * 0.8;
> >>
> >> I would group the initialization of member variables together, either
> >> before, or after the initialization of params.
> >>
> >>> +    params.use.acc_awb = 1;
> >>> +    /*\todo fill the grid calculated based on BDS configuration */
> >>> +    params.acc_param.awb.config = imgu_css_awb_defaults;
> >>> +
> >>> +    params.use.acc_bnr = 1;
> >>> +    params.acc_param.bnr = imgu_css_bnr_defaults;
> >>> +
> >>> +    params.use.acc_ccm = 1;
> >>> +    params.acc_param.ccm = imgu_css_ccm_3800k;
> >>> +
> >>> +    params.use.acc_gamma = 1;
> >>> +    params.acc_param.gamma.gc_ctrl.enable = 1;
> >>> +
> >>> +    uint32_t a = (32 * 245) / (245 - 9);
> >>> +
> >>> +    for (uint32_t i = 0; i < 10; i++)
> >>> +        params.acc_param.gamma.gc_lut.lut[i] = 0;
> >>> +    for (uint32_t i = 10; i < 245; i++)
> >>> +        params.acc_param.gamma.gc_lut.lut[i] = a * i + (0 - a * 9);
> >>> +    for (uint32_t i = 245; i < 255; i++)
> >>> +        params.acc_param.gamma.gc_lut.lut[i] = 32 * 245;
> >>> +
> >>> +    frame_count_ = 0;
> >>> +    algoConverged_ = false;
> >>> +}
> >>> +
> >>> +unsigned int IPU3Awb::GetConvergenceFrames() const
> >>> +{
> >>> +    // If colour gains have been explicitly set, there is no
> >>> convergence
> >>> +    // to happen, so no need to drop any frames - return zero.
> >>> +    if (manual_r_ && manual_b_)
> >>> +        return 0;
> >>> +    else
> >>> +        return config_.convergence_frames;
> >>> +}
> >>> +
> >>> +void IPU3Awb::SetMode(std::string const &mode_name)
> >>> +{
> >>> +    mode_name_ = mode_name;
> >>> +}
> >>> +
> >>> +void IPU3Awb::SetManualGains(double manual_r, double manual_b)
> >>> +{
> >>> +    // If any of these are 0.0, we swich back to auto.
> >>> +    manual_r_ = manual_r;
> >>> +    manual_b_ = manual_b;
> >>> +}
> >>> +
> >>> +uint32_t IPU3Awb::estimateCCT(uint8_t R, uint8_t G, uint8_t B)
> >>> +{
> >>> +    double X = (-0.14282) * (R) + (1.54924) * (G) + (-0.95641) * (B);
> >>> +    double Y = (-0.32466) * (R) + (1.57837) * (G) + (-0.73191) * (B);
> >>> +    double Z = (-0.68202) * (R) + (0.77073) * (G) + (0.56332) * (B);
> >>> +
> >>> +    double x = X / (X + Y + Z);
> >>> +    double y = Y / (X + Y + Z);
> >>> +
> >>> +    double n = (x - 0.3320) / (0.1858 - y);
> >>> +    return static_cast<uint32_t>(449 * n * n * n + 3525 * n * n +
> >>> 6823.3 * n + 5520.33);
> >>> +}
> >>> +
> >>> +void IPU3Awb::calculateWBGains([[maybe_unused]] Rectangle roi, const
> >>> ipu3_uapi_stats_3a *stats)
> >>
> >> Line wrap.
> >>
> >>> +{
> >>> +    if (algoConverged_)
> >>> +        return;
> >>
> >> So once it converges, you never update the gains anymore, even if the
> >> scene changes ?
> >>
> >>> +
> >>> +    std::vector<uint32_t> R_v, Gr_v, Gb_v, B_v;
> >>
> >> camelCase. Feel free to also make the name slightly longer to make the
> >> more explicit. redValues ?
> >>
> >>> +    Point topleft = roi.topLeft();
> >>> +    uint32_t startY = (topleft.y / 16) * 160 * 8;
> >>> +    uint32_t startX = (topleft.x / 8) * 8;
> >>> +    uint32_t endX = (startX + (roi.size().width / 8)) * 8;
> >>> +
> >>> +    for (uint32_t j = (topleft.y / 16); j < (topleft.y / 16) +
> >>> (roi.size().height / 16); j++) {
> >>> +        for (uint32_t i = startX + startY; i < endX + startY; i += 8) {
> >>> +            Gr_v.push_back(stats->awb_raw_buffer.meta_data[i]);
> >>> +            R_v.push_back(stats->awb_raw_buffer.meta_data[i + 1]);
> >>> +            B_v.push_back(stats->awb_raw_buffer.meta_data[i + 2]);
> >>> +            Gb_v.push_back(stats->awb_raw_buffer.meta_data[i + 3]);
> >>> +        }
> >>> +    }
> >>> +
> >>> +    std::sort(R_v.begin(), R_v.end());
> >>> +    std::sort(Gr_v.begin(), Gr_v.end());
> >>> +    std::sort(B_v.begin(), B_v.end());
> >>> +    std::sort(Gb_v.begin(), Gb_v.end());
> >>> +
> >>> +    double Grmed = Gr_v[Gr_v.size() / 2];
> >>> +    double Rmed = R_v[R_v.size() / 2];
> >>> +    double Bmed = B_v[B_v.size() / 2];
> >>> +    double Gbmed = Gb_v[Gb_v.size() / 2];
> >>> +
> >>> +    double Rgain = Grmed / Rmed;
> >>> +    double Bgain = Gbmed / Bmed;
> >>> +    LOG(IPU3Awb, Debug) << "max R, Gr, B, Gb: "
> >>> +                << R_v.back() << ","
> >>> +                << Gr_v.back() << ","
> >>> +                << B_v.back() << ","
> >>> +                << Gb_v.back();
> >>> +    tint_ = ((Rmed / Grmed) + (Bmed / Gbmed)) / 2;
> >>> +
> >>> +    /* \todo Those are corrections when light is really low
> >>> +     * it should be taken into account by AGC somehow */
> >>> +    if ((Rgain >= 2) && (Bgain < 2)) {
> >>> +        wbGains_[0] = 4096 * tint_;
> >>> +        wbGains_[1] = 8192 * Rgain;
> >>> +        wbGains_[2] = 4096 * Bgain;
> >>> +        wbGains_[3] = 4096 * tint_;
> >>> +    } else if ((Bgain >= 2) && (Rgain < 2)) {
> >>> +        wbGains_[0] = 4096 * tint_;
> >>> +        wbGains_[1] = 4096 * Rgain;
> >>> +        wbGains_[2] = 8192 * Bgain;
> >>> +        wbGains_[3] = 4096 * tint_;
> >>> +    } else {
> >>> +        wbGains_[0] = 8192 * tint_;
> >>> +        wbGains_[1] = 8192 * Rgain;
> >>> +        wbGains_[2] = 8192 * Bgain;
> >>> +        wbGains_[3] = 8192 * tint_;
> >>> +    }
> >>> +
> >>> +    frame_count_++;
> >>> +
> >>> +    cct_ = estimateCCT(Rmed, (Grmed + Gbmed) / 2, Bmed);
> >>> +
> >>> +    algoConverged_ = true;
> >>> +}
> >>> +
> >>> +void IPU3Awb::updateBNR(ipu3_uapi_params &params)
> >>
> >> updateBNR() isn't a great name for a function that updates the WB gains,
> >> even if they're part of the Bayer noise reduction block of data.
> >>
> >> Should we push the ipu3_uapi_params structure down to individual
> >> algorithms, or would it be better to decouple it, creating custom
> >> structures to hold algorithm results, and fillin ipu3_uapi_params in
> >> IPAIPU3::fillParams() instead ? On one hand moving ipu3_uapi_params out
> >> will make the code more generic, and thus more reusable, but on the
> >> other hand we would still depend on a custom stats format, so it may not
> >> be worth it.
> >>
> >>> +{
> >>> +    if (!algoConverged_)
> >>> +        return;
> >>> +
> >>> +    params.acc_param.bnr.wb_gains.gr = wbGains_[0];
> >>> +    params.acc_param.bnr.wb_gains.r = wbGains_[1];
> >>> +    params.acc_param.bnr.wb_gains.b = wbGains_[2];
> >>> +    params.acc_param.bnr.wb_gains.gb = wbGains_[3];
> >>> +    if (cct_ < 5500)
> >>> +        params.acc_param.ccm = imgu_css_ccm_3800k;
> >>> +    else
> >>> +        params.acc_param.ccm = imgu_css_ccm_6000k;
> >>> +}
> >>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> >>> new file mode 100644
> >>> index 00000000..06389020
> >>> --- /dev/null
> >>> +++ b/src/ipa/ipu3/ipu3_awb.h
> >>> @@ -0,0 +1,130 @@
> >>> +/* SPDX-License-Identifier: BSD-2-Clause */
> >>> +/*
> >>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> >>> + *
> >>> + * awb.h - AWB control algorithm
> >>> + */
> >>> +#ifndef __LIBCAMERA_IPU3_AWB_H__
> >>> +#define __LIBCAMERA_IPU3_AWB_H__
> >>> +
> >>> +#include <linux/intel-ipu3.h>
> >>> +
> >>> +#include <libcamera/geometry.h>
> >>> +
> >>> +#include <libcamera/ipa/awb_algorithm.h>
> >>> +#include <libcamera/ipa/ipa_controller.h>
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +const struct ipu3_uapi_bnr_static_config imgu_css_bnr_defaults = {
> >>> +    { 16, 16, 16, 16 },            /* wb_gains */
> >>> +    { 255, 255, 255, 255 },            /* wb_gains_thr */
> >>> +    { 0, 0, 8, 6, 0, 14 },            /* thr_coeffs */
> >>> +    { 0, 0, 0, 0 },                /* thr_ctrl_shd */
> >>> +    { -648, 0, -366, 0 },            /* opt_center */
> >>> +    {                    /* 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 }
> >>> +    },
> >>> +    { 4, 0, 1, 8, 0, 8, 0, 8, 0 },        /* bp_ctrl */
> >>> +    { 8, 4, 4, 0, 8, 0, 1, 1, 1, 1, 0 },    /* dn_detect_ctrl */
> >>> +    1296,
> >>> +    { 419904, 133956 },
> >>> +};
> >>> +
> >>> +/* settings for Auto White Balance */
> >>> +const struct ipu3_uapi_awb_config_s imgu_css_awb_defaults = {
> >>> +    8191,
> >>> +    8191,
> >>> +    8191,
> >>> +    8191 | /* rgbs_thr_gr/r/gb/b */
> >>> +        IPU3_UAPI_AWB_RGBS_THR_B_EN |
> >>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
> >>> +    .grid = {
> >>> +        .width = 160,
> >>> +        .height = 45,
> >>> +        .block_width_log2 = 3,
> >>> +        .block_height_log2 = 4,
> >>> +        .x_start = 0,
> >>> +        .y_start = 0,
> >>> +    },
> >>> +};
> >>> +
> >>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_6000k = {
> >>> +    7239, -750, -37, 0,
> >>> +    -215, 9176, -200, 0,
> >>> +    -70, -589, 6810, 0
> >>> +};
> >>> +
> >>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_4900k = {
> >>> +    7811, -464, -466, 0,
> >>> +    -635, 8762, -533, 0,
> >>> +    -469, -154, 6583, 0
> >>> +};
> >>> +
> >>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_3800k = {
> >>> +    7379, -526, -296, 0,
> >>> +    -411, 7397, -415, 0,
> >>> +    -224, -564, 7244, 0
> >>> +};
> >>
> >> All these should be moved to ipu3_awb.cpp, and be made static. Otherwise
> >> they would get duplicated in every compilation unit in which this header
> >> is included.
> >>
> >>> +
> >>> +struct AwbConfig {
> >>> +    // Only repeat the AWB calculation every "this many" frames
> >>> +    uint16_t frame_period;
> >>> +    // number of initial frames for which speed taken as 1.0 (maximum)
> >>> +    uint16_t startup_frames;
> >>> +    unsigned int convergence_frames; // approx number of frames to
> >>> converge
> >>> +    double speed; // IIR filter speed applied to algorithm results
> >>> +};
> >>> +
> >>> +#if 0
> >>> +typedef struct awb_public_set_item{
> >>> +    unsigned char Gr_avg;
> >>> +    unsigned char R_avg;
> >>> +    unsigned char B_avg;
> >>> +    unsigned char Gb_avg;
> >>> +    unsigned char sat_ratio;
> >>> +    unsigned char padding0; /**< Added the padding so that the
> >>> public matches that private */
> >>> +    unsigned char padding1; /**< Added the padding so that the
> >>> public matches that private */
> >>> +    unsigned char padding2; /**< Added the padding so that the
> >>> public matches that private */
> >>> +} awb_public_set_item_t;
> >>> +#endif
> >>
> >> If the compiler doesn't need to see this, I think you can drop it
> >> completely :-)
> >>
> >>> +
> >>> +class IPU3Awb : public AwbAlgorithm
> >>> +{
> >>> +public:
> >>> +    IPU3Awb(IPAController *controller = NULL);
> >>
> >> This is C++, so nullptr :-) But there's no need for a default value, as
> >> the controller parameter should either be mandatory, or removed
> >> completely.
> >>
> >>> +    ~IPU3Awb();
> >>
> >> Blank line.
> >>
> >>> +    virtual char const *Name() const override;
> >>
> >> This is never used, you can drop it from the base class. When (and if)
> >> we'll need this, I'd rather pass the name to the base class constructor,
> >> store it there, and implement name() in the base class only, as a
> >> non-virtual function.
> >>
> >>> +    virtual void Initialise() override;
> >>
> >> No need to repeat the virtual keyword for overridden functions.
> >>
> >>> +    void Initialise(ipu3_uapi_params &params);
> >>
> >> This isn't very nice, as it's not aligned with the design of the
> >> IPAAlgorithm class, but we can fix it later. I would move the function
> >> down, after the mandatory functions, with calculateWBGains() and
> >> updateBNR().
> >>
> >>> +    unsigned int GetConvergenceFrames() const override;
> >>> +    void SetMode(std::string const &name) override;
> >>> +    void SetManualGains(double manual_r, double manual_b) override;
> >>
> >> A blank line here would be nice.
> >>
> >>> +    void calculateWBGains(Rectangle roi,
> >>
> >> const Rectangle &roi
> >>
> >>> +                  const ipu3_uapi_stats_3a *stats);
> >>> +    void updateBNR(ipu3_uapi_params &params);
> >>> +
> >>> +private:
> >>> +    uint32_t estimateCCT(uint8_t R, uint8_t G, uint8_t B);
> >>
> >> r, g and b.
> >>
> >>> +
> >>> +    /* configuration is read-only, and available to both threads */
> >>
> >> What threads ? :-)
> >>
> >>> +    AwbConfig config_;
> >>> +    std::string mode_name_;
> >>
> >> Never used.
> >>
> >>> +    /* manual r setting */
> >>> +    double manual_r_;
> >>
> >> manualRedGain_ ?
> >>
> >>> +    /* manual b setting */
> >>> +    double manual_b_;
> >>> +    /* WB calculated gains */
> >>> +    uint16_t wbGains_[4];
> >>> +    double tint_;
> >>> +    uint32_t cct_;
> >>> +
> >>> +    uint32_t frame_count_;
> >>> +
> >>> +    bool algoConverged_;
> >>> +};
> >>> +} /* namespace libcamera */
> >>> +
> >>> +#endif /* __LIBCAMERA_IPU3_AWB_H__ */
> >>> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> >>> index a241f617..43ad0e0d 100644
> >>> --- a/src/ipa/ipu3/meson.build
> >>> +++ b/src/ipa/ipu3/meson.build
> >>> @@ -2,8 +2,14 @@
> >>>     ipa_name = 'ipa_ipu3'
> >>>   +ipu3_ipa_sources = files([
> >>> +  'ipu3.cpp',
> >>> +  'ipu3_agc.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,
> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> index ff980b38..3809c943 100644
> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> @@ -825,6 +825,7 @@ int
> >>> PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >>>        */
> >>>       double lineDuration = sensorInfo.lineLength
> >>>                   / (sensorInfo.pixelRate / 1e6);
> >>> +    LOG(IPU3, Error) << "line duration: " << lineDuration;
> >>
> >> Is this an error ? :-)
> >>
> >>>       const ControlInfoMap &sensorControls = sensor->controls();
> >>>       const ControlInfo &v4l2Exposure =
> >>> sensorControls.find(V4L2_CID_EXPOSURE)->second;
> >>>       int32_t minExposure = v4l2Exposure.min().get<int32_t>() *
> >>> lineDuration;

Hi Laurent,

On 25/02/2021 02:43, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> On Mon, Feb 22, 2021 at 09:32:02PM +0100, Jean-Michel Hautbois wrote:
>> On 22/02/2021 19:50, Dafna Hirschfeld wrote:
>>> On 22.02.21 05:56, Laurent Pinchart wrote:
>>>> On Fri, Feb 19, 2021 at 06:22:24PM +0100, Jean-Michel Hautbois wrote:
>>>>> Inherit from the AWBAlgorithm and AGCAlgorithm classes to implement
>>>>> basic functions.
>>>>> While exposure is not locked, AWB is not calculated and corrected.
>>>>> Once AWB is done, a color temperature is estimated and default CCM
>>>>> matrices
>>>>> are used.
>>>>> Implement a basic "grey-world" AWB algorithm just for demonstration
>>>>> purpose.
>>>
>>> Are those algorithms specific to IPU3 ?
>>> If not then maybe it can used by rkisp1 as well?
>>> Maybe it worth to think of general API for basic 3a implementations that
>>> other IPAs can use?
>>
>> Not at all ! And I will send a second patch version which will show how
>> to implement the image processing algorithm, and indeed it aims to be
>> platform agnostic (even if the parameters and statistics are always
>> specific ;-)) !
>>
>>>> Nice to see an initial implementation ! I'll provide a handful of review
>>>> comments below already. Some are related to coding style or similar
>>>> issues and should be addressed in v2. Others are related to the IPA
>>>> architecture, and could be fixed on top of this series to avoid delaying
>>>> it unnecessarily. They should in that case be captured in a \todo
>>>> comment.
>>>>
>>>> As for 1/2, I won't repeat the same comments all over the patch, so
>>>> please apply them wherever they're applicable.
>>>>
>>>>> Signed-off-by: Jean-Michel Hautbois
>>>>> <jeanmichel.hautbois@ideasonboard.com>
>>>>> ---
>>>>>   src/ipa/ipu3/ipu3.cpp                |  31 ++++-
>>>>>   src/ipa/ipu3/ipu3_agc.cpp            | 195 +++++++++++++++++++++++++++
>>>>>   src/ipa/ipu3/ipu3_agc.h              |  96 +++++++++++++
>>>>>   src/ipa/ipu3/ipu3_awb.cpp            | 182 +++++++++++++++++++++++++
>>>>>   src/ipa/ipu3/ipu3_awb.h              | 130 ++++++++++++++++++
>>>>>   src/ipa/ipu3/meson.build             |   8 +-
>>>>>   src/libcamera/pipeline/ipu3/ipu3.cpp |   1 +
>>>>>   7 files changed, 638 insertions(+), 5 deletions(-)
>>>>>   create mode 100644 src/ipa/ipu3/ipu3_agc.cpp
>>>>>   create mode 100644 src/ipa/ipu3/ipu3_agc.h
>>>>>   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 b63e58be..c3859f39 100644
>>>>> --- a/src/ipa/ipu3/ipu3.cpp
>>>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>>>> @@ -13,6 +13,7 @@
>>>>>     #include <libcamera/buffer.h>
>>>>>   #include <libcamera/control_ids.h>
>>>>> +#include <libcamera/ipa/ipa_controller.h>
>>>>>   #include <libcamera/ipa/ipa_interface.h>
>>>>>   #include <libcamera/ipa/ipa_module_info.h>
>>>>>   #include <libcamera/ipa/ipu3_ipa_interface.h>
>>>>> @@ -21,6 +22,9 @@
>>>>>   #include "libcamera/internal/buffer.h"
>>>>>   #include "libcamera/internal/log.h"
>>>>>   +#include "ipu3_agc.h"
>>>>> +#include "ipu3_awb.h"
>>>>> +
>>>>>   namespace libcamera {
>>>>>     LOG_DEFINE_CATEGORY(IPAIPU3)
>>>>> @@ -28,6 +32,9 @@ LOG_DEFINE_CATEGORY(IPAIPU3)
>>>>>   class IPAIPU3 : public ipa::ipu3::IPAIPU3Interface
>>>>>   {
>>>>>   public:
>>>>> +    IPAIPU3()
>>>>> +        : controller_() {}
>>>>
>>>>         : controller_()
>>>>     {
>>>>     }
>>>>
>>>> I may not repeat all the comments from 1/2, but they're generally
>>>> applicable to this patch as well.
>>>>
>>>>> +
>>>>>       int init([[maybe_unused]] const IPASettings &settings) override
>>>>>       {
>>>>>           return 0;
>>>>> @@ -60,6 +67,11 @@ private:
>>>>>       uint32_t gain_;
>>>>>       uint32_t minGain_;
>>>>>       uint32_t maxGain_;
>>>>> +
>>>>> +    IPAController controller_;
>>>>> +    IPU3Awb *awbAlgo_;
>>>>> +    IPU3Agc *agcAlgo_;
>>>>> +    ipu3_uapi_params params_;
>>>>>   };
>>>>>     void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap>
>>>>> &entityControls)
>>>>> @@ -83,11 +95,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_ = 123;
>>>>
>>>> A peculiar value :-)
>>>>
>>>>>         minGain_ = std::max(itGain->second.min().get<int32_t>(), 1);
>>>>>       maxGain_ = itGain->second.max().get<int32_t>();
>>>>> -    gain_ = maxGain_;
>>>>> +    gain_ = 1;
>>>>> +
>>>>> +    awbAlgo_ = new IPU3Awb(&controller_);
>>>>> +    awbAlgo_->Initialise(params_);
>>>>> +    agcAlgo_ = new IPU3Agc(&controller_);
>>>>> +
>>>>> +    /*\todo not used yet... */
>>>>> +    controller_.Initialise();
>>>>>         setControls(0);
>>>>>   }
>>>>> @@ -162,10 +181,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));
>>>>> +    awbAlgo_->updateBNR(params_);
>>>>> +    memcpy(params, &params_, sizeof(*params));
>>>>
>>>>     *params = params_;
>>>>
>>>>>         /* \todo Fill in parameters buffer. */
>>>>> -
>>>>>       ipa::ipu3::IPU3Action op;
>>>>>       op.op = ipa::ipu3::ActionParamFilled;
>>>>>   @@ -179,6 +198,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>>>>         /* \todo React to statistics and update internal state
>>>>> machine. */
>>>>>       /* \todo Add meta-data information to ctrls. */
>>>>> +    agcAlgo_->Process(stats, exposure_, gain_);
>>>>> +    if (agcAlgo_->Converged())
>>>>> +        awbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400),
>>>>> stats);
>>>>> +    setControls(frame);
>>>>>         ipa::ipu3::IPU3Action op;
>>>>>       op.op = ipa::ipu3::ActionMetadataReady;
>>>>> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
>>>>> new file mode 100644
>>>>> index 00000000..db591e33
>>>>> --- /dev/null
>>>>> +++ b/src/ipa/ipu3/ipu3_agc.cpp
>>>>> @@ -0,0 +1,195 @@
>>>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>>>> +/*
>>>>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>>>>> + *
>>>>> + * agc.cpp - AGC/AEC control algorithm
>>>>> + */
>>>>> +#include <iostream>
>>>>> +#include <numeric>
>>>>> +#include <unordered_map>
>>>>> +
>>>>> +#include "libcamera/internal/log.h"
>>>>> +
>>>>> +#include "ipu3_agc.h"
>>>>> +
>>>>> +using namespace libcamera;
>>>>> +
>>>>> +LOG_DEFINE_CATEGORY(IPU3Agc)
>>>>> +
>>>>> +#define NAME "ipu3.agc"
>>>>> +
>>>>> +IPU3Agc::IPU3Agc(IPAController *controller)
>>>>> +    : AgcAlgorithm(controller), frameCount_(0),
>>>>> +      ev_(1.0), flicker_period_(0.0),
>>>>> +      max_shutter_(0), fixed_shutter_(0),
>>>>> +      fixed_analogue_gain_(0.0), algoConverged_(false)
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +char const *IPU3Agc::Name() const
>>>>> +{
>>>>> +    return NAME;
>>>>> +}
>>>>> +
>>>>> +unsigned int IPU3Agc::GetConvergenceFrames() const
>>>>> +{
>>>>> +    return config_.convergence_frames;
>>>>> +}
>>>>> +
>>>>> +void IPU3Agc::SetEv(double ev)
>>>>> +{
>>>>> +    ev_ = ev;
>>>>> +}
>>>>> +
>>>>> +void IPU3Agc::SetFlickerPeriod(double flicker_period)
>>>>> +{
>>>>> +    flicker_period_ = flicker_period;
>>>>> +}
>>>>> +
>>>>> +void IPU3Agc::SetMaxShutter(double max_shutter)
>>>>> +{
>>>>> +    max_shutter_ = max_shutter;
>>>>> +}
>>>>> +
>>>>> +void IPU3Agc::SetFixedShutter(double fixed_shutter)
>>>>> +{
>>>>> +    fixed_shutter_ = fixed_shutter;
>>>>> +}
>>>>> +
>>>>> +void IPU3Agc::SetFixedAnalogueGain(double fixed_analogue_gain)
>>>>> +{
>>>>> +    fixed_analogue_gain_ = fixed_analogue_gain;
>>>>> +}
>>>>> +
>>>>> +void IPU3Agc::SetMeteringMode(std::string const &metering_mode_name)
>>>>> +{
>>>>> +    metering_mode_name_ = metering_mode_name;
>>>>> +}
>>>>> +
>>>>> +void IPU3Agc::SetExposureMode(std::string const &exposure_mode_name)
>>>>> +{
>>>>> +    exposure_mode_name_ = exposure_mode_name;
>>>>> +}
>>>>> +
>>>>> +void IPU3Agc::Prepare() {}
>>>>> +
>>>>> +void IPU3Agc::Process() {}
>>>>> +
>>>>> +/* \todo This function is taken from numerical recipes and
>>>>> calculates all moments
>>>>> + * It needs to be rewritten properly and maybe in a "math" class ? */
>>>>
>>>> /*
>>>>   * \todo This function is taken from numerical recipes and calculates
>>>> all
>>>>   * moments. It needs to be rewritten properly and maybe in a "math"
>>>> class ?
>>>>   */
>>>>
>>>>> +void IPU3Agc::moments(std::unordered_map<uint32_t, uint32_t> &data,
>>>>> int n)
>>>
>>> 'data' is the hist, so why not call it 'hist' here?
>>
>> Yes, why not :-) thanks !
>>
>>>>> +{
>>>>> +    int j;
>>>>> +    double ep = 0.0, s, p;
>>>>> +    double ave, adev, sdev;
>>>>> +    double var, skew, curt;
>>>>> +
>>>>> +    s = 0.0;
>>>>> +    for (j = 1; j <= n; j++)
>>>>> +        s += data[j];
>>>
>>> shouldn't that be:
>>> s += data[j] * j
>>>
>>> since data[j] is the number of pixels with value j
>>
>> I don't think so, no, but I will have to verify...
> 
> I would have agreed with Dafna here. If the code is correct, then maybe
> more explicit names for the variables, and/or comments, could help
> explaining what's going on.
> 
>>>>> +
>>>>> +    ave = s / n;
>>>>> +    adev = var = skew = curt = 0.0;
>>>>> +
>>>>> +    for (j = 1; j <= n; j++) {
>>>>> +        adev += s = data[j] - (ave);
>>>>> +        ep += s;
>>>>> +        var += (p = s * s);
>>>>> +        skew += (p *= s);
>>>>> +        curt += (p *= s);
>>>>> +    }
>>>>> +
>>>>> +    adev /= n;
>>>>> +    var = (var - ep * ep / n) / (n - 1);
>>>>> +    sdev = std::sqrt(var);
>>>>> +
>>>>> +    if (var) {
>>>>> +        skew /= n * var * sdev;
>>>>> +        curt = curt / (n * var * var) - 3.0;
>>>>> +    }
>>>>> +    skew_ = skew;
>>>>> +}
>>>>> +
>>>>> +void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>>>>> +{
>>>>> +    brightnessVec_.clear();
>>>>> +
>>>>> +    /*\todo Replace constant values with real BDS configuration */
>>>>> +    for (uint32_t j = 0; j < 45; j++) {
>>>>> +        for (uint32_t i = 0; i < 160 * 45 * 8; i += 8) {
>>>>> +            uint8_t Gr = stats->awb_raw_buffer.meta_data[i];
>>>>> +            uint8_t R = stats->awb_raw_buffer.meta_data[i + 1];
>>>>> +            uint8_t B = stats->awb_raw_buffer.meta_data[i + 2];
>>>>> +            uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3];
>>>>> +            brightnessVec_.push_back(static_cast<uint32_t>(0.299 * R
>>>>> + 0.587 * (Gr + Gb) / 2 + 0.114 * B));
>>>
>>> You iterate on j in the outer loop but don't use j anywhere for
>>> indexing, this is a bit suspicious
>>
>> This is just for an easier splitting of rectangles (taken from the AWB
>> algorithm).
> 
> Is the aim really to repeat the same inner loop 45 times ?

Oh, Dafna pointed it I misread, and now it is obvious...
No of course not !
It is now 45 times faster :-).
Thanks !

>>>>> +        }
>>>>> +    }
>>>>> +    std::sort(brightnessVec_.begin(), brightnessVec_.end());
>>>
>>> I don't think the sort is is necessary
>>
>> It is there from an old calculation of median value. Not needed indeed.
>>
>>>>> +
>>>>> +    /* \todo create a class to generate histograms ! */
>>>>> +    std::unordered_map<uint32_t, uint32_t> hist;
>>>
>>> you want to have up to 256 indexes so you need to define with uint8_t
>>>
>>> std::unordered_map<uint8_t, uint32_t> hist;
>>
>> ack
>>
>>>>> +    for (uint32_t const &val : brightnessVec_)
>>>>> +        hist[val]++;
>>>>> +    moments(hist, 256);
>>>>> +}
>>>>> +
>>>>> +void IPU3Agc::lockExposure(uint32_t &exposure, uint32_t &gain)
>>>>> +{
>>>>> +    /* Algorithm initialization wait for first valid frames */
>>>>> +    /* \todo - have a number of frames given by DelayedControls ?
>>>>> +     * - implement a function for IIR */
>>>>> +    if (frameCount_ == 4) {
>>>>> +        prevExposure_ = exposure;
>>>>> +
>>>>> +        prevSkew_ = skew_;
>>>>> +        /* \tdo use configured values */
>>>>
>>>> s/tdo/todo/
>>>>
>>>>> +        exposure = 800;
>>>>> +        gain = 8;
>>>>> +        currentExposure_ = exposure;
>>>>> +    } else if (frameCount_ == 8) {
>>>>> +        currentSkew_ = skew_;
>>>>> +        exposure = ((currentExposure_ * prevSkew_) + (prevExposure_
>>>>> * currentSkew_)) / (prevSkew_ + currentSkew_);
>>>>
>>>> Line wrap. No need for the inner parentheses.
>>>>
>>>>> +        nextExposure_ = exposure;
>>>>> +        lastFrame_ = frameCount_;
>>>>> +    } else if ((frameCount_ >= 12) && (frameCount_ - lastFrame_ >=
>>>>> 4)) {
>>>>
>>>> Could you define constants for magic values, to make the code more
>>>> readable ? They can be defined as static constexpr members of the
>>>> IPU3Agc class, and should be named with camelCase. While we don't do so
>>>> yet in many places, it's customary to start them with a 'k' prefix (e.g.
>>>> kInitialFrameSkipCount).
>>>>
>>>>> +        currentSkew_ = skew_;
>>>>> +        /* \todo properly calculate a gain */
>>>>> +        if (frameCount_ == 12)
>>>>> +            gain = ((8 * prevSkew_) + (1 * currentSkew_)) /
>>>>> (prevSkew_ + currentSkew_);
>>>>> +
>>>>> +        if (currentSkew_ - prevSkew_ > 1) {
>>>>> +            /* under exposed */
>>>>> +            prevExposure_ = nextExposure_;
>>>>> +            exposure = ((currentExposure_ * prevSkew_) +
>>>>> (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
>>>>> +            nextExposure_ = exposure;
>>>>> +        } else if (currentSkew_ - prevSkew_ < -1) {
>>>>> +            /* over exposed */
>>>>> +            currentExposure_ = nextExposure_;
>>>>> +            exposure = ((currentExposure_ * prevSkew_) +
>>>>> (prevExposure_ * currentSkew_)) / (prevSkew_ + currentSkew_);
>>>>> +            nextExposure_ = exposure;
>>>>> +        } else {
>>>>> +            /* we have converged */
>>>>> +            algoConverged_ = true;
>>>>> +        }
>>>>> +        lastFrame_ = frameCount_;
>>>>> +        prevSkew_ = currentSkew_;
>>>>> +    }
>>>
>>> Could you explain the general idea behind this algorithm or give a link
>>> where it is explained?
>>
>> Yes, I will, and this algorithm will evolve as I want to use Q3-Q1
>> inter-quartile measure to optimize auto exposure.
>>
>>>>> +}
>>>>> +
>>>>> +void IPU3Agc::Process(const ipu3_uapi_stats_3a *stats, uint32_t
>>>>> &exposure, uint32_t &gain)
>>>>> +{
>>>>> +    processBrightness(stats);
>>>>> +    if (!algoConverged_)
>>>>> +        lockExposure(exposure, gain);
>>>>> +    else {
>>>>> +        /* Are we still well exposed ? */
>>>>> +        if ((skew_ < 2) || (skew_ > 4))
>>>>> +            algoConverged_ = false;
>>>>> +    }
>>>>> +    frameCount_++;
>>>>> +}
>>>>> +
>>>>> +bool IPU3Agc::Converged()
>>>>> +{
>>>>> +    return algoConverged_;
>>>>> +}
>>>>> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
>>>>> new file mode 100644
>>>>> index 00000000..9a69d628
>>>>> --- /dev/null
>>>>> +++ b/src/ipa/ipu3/ipu3_agc.h
>>>>> @@ -0,0 +1,96 @@
>>>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>>>> +/*
>>>>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>>>>> + *
>>>>> + * ipu3_agc.h - AGC/AEC control algorithm
>>>>> + */
>>>>> +#ifndef __LIBCAMERA_IPU3_AGC_H__
>>>>> +#define __LIBCAMERA_IPU3_AGC_H__
>>>>> +
>>>>
>>>> #include <map>
>>>>
>>>>> +#include <mutex>
>>>>
>>>> This doesn't seem to be needed.
>>>>
>>>>> +#include <vector>
>>>>> +
>>>>> +#include <linux/intel-ipu3.h>
>>>>> +
>>>>> +#include <libcamera/geometry.h>
>>>>> +
>>>>> +#include <libcamera/ipa/agc_algorithm.h>
>>>>> +#include <libcamera/ipa/ipa_controller.h>
>>>>> +
>>>>> +#define AGC_STATS_SIZE 15
>>>>> +
>>>>> +namespace libcamera {
>>>>> +
>>>>> +struct AgcMeteringMode {
>>>>> +    double weights[AGC_STATS_SIZE];
>>>>> +    Rectangle metering_region[AGC_STATS_SIZE];
>>>>> +};
>>>>> +
>>>>> +struct AgcExposureMode {
>>>>> +    std::vector<double> shutter;
>>>>> +    std::vector<double> gain;
>>>>> +};
>>>>> +
>>>>> +struct AgcConfig {
>>>>> +    std::map<std::string, AgcMeteringMode> metering_modes;
>>>>> +    std::map<std::string, AgcExposureMode> exposure_modes;
>>>>> +    double speed;
>>>>> +    uint16_t startup_frames;
>>>>> +    unsigned int convergence_frames;
>>>>> +    std::string default_metering_mode;
>>>>> +    std::string default_exposure_mode;
>>>>> +    double default_exposure_time;
>>>>> +    double default_analogue_gain;
>>>>> +};
>>>>> +
>>>>> +class IPU3Agc : public AgcAlgorithm
>>>>> +{
>>>>> +public:
>>>>> +    IPU3Agc(IPAController *IPAcontroller);
>>>>
>>>> Blank line.
>>>>
>>>>> +    char const *Name() const override;
>>>>> +    unsigned int GetConvergenceFrames() const override;
>>>>> +    void SetEv(double ev) override;
>>>>> +    void SetFlickerPeriod(double flicker_period) override;
>>>>> +    void SetMaxShutter(double max_shutter) override; // microseconds
>>>>> +    void SetFixedShutter(double fixed_shutter) override; //
>>>>> microseconds
>>>>> +    void SetFixedAnalogueGain(double fixed_analogue_gain) override;
>>>>> +    void SetMeteringMode(std::string const &metering_mode_name)
>>>>> override;
>>>>> +    void SetExposureMode(std::string const &exposure_mode_name)
>>>>> override;
>>>>> +    void Prepare() override;
>>>>> +    void Process() override;
>>>>> +    void Process(const ipu3_uapi_stats_3a *stats, uint32_t
>>>>> &exposure, uint32_t &gain);
>>>>
>>>> The last two arguments should be replaced by a generic data container,
>>>> similar to the Metadata class in the RPi IPA (not something to fix now).
>>>>
>>>>> +    bool Converged();
>>>>> +
>>>>> +private:
>>>>> +    void moments(std::unordered_map<uint32_t, uint32_t> &data, int n);
>>>>> +    void processBrightness(const ipu3_uapi_stats_3a *stats);
>>>>> +    void lockExposure(uint32_t &exposure, uint32_t &gain);
>>>>> +
>>>>> +    AgcConfig config_;
>>>>
>>>> This is never set, and only one member is read. I'd drop it for now.
>>>>
>>>>> +    std::string metering_mode_name_;
>>>>> +    std::string exposure_mode_name_;
>>>>> +    uint64_t frameCount_;
>>>>> +    uint64_t lastFrame_;
>>>>> +
>>>>> +    /* Vector of calculated brightness for each cell */
>>>>> +    std::vector<uint32_t> brightnessVec_;
>>>>
>>>> We avoid encoding the type in the variable name. Would this be better
>>>> named cellsBrightness_ ?
>>>>
>>>>> +    double ev_;
>>>>
>>>> Never used. Same for other members below.
>>>>
>>>>> +    double flicker_period_;
>>>>> +    double max_shutter_;
>>>>> +    double fixed_shutter_;
>>>>> +    double fixed_analogue_gain_;
>>>>> +
>>>>> +    /* Values for filtering */
>>>>> +    uint32_t prevExposure_;
>>>>> +    uint32_t currentExposure_;
>>>>> +    uint32_t nextExposure_;
>>>>> +
>>>>> +    double skew_;
>>>>> +    double prevSkew_;
>>>>> +    double currentSkew_;
>>>>> +    bool algoConverged_;
>>>>
>>>> Maybe just converged_ ?
>>>>
>>>>> +};
>>>>> +
>>>>> +} /* namespace libcamera */
>>>>> +
>>>>> +#endif /* __LIBCAMERA_IPU3_AGC_H__ */
>>>>> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
>>>>> new file mode 100644
>>>>> index 00000000..21286e4c
>>>>> --- /dev/null
>>>>> +++ b/src/ipa/ipu3/ipu3_awb.cpp
>>>>> @@ -0,0 +1,182 @@
>>>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>>>> +/*
>>>>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>>>>> + *
>>>>> + * ipu3_awb.cpp - AWB control algorithm
>>>>> + */
>>>>> +#include <iostream>
>>>>
>>>> I don't think this is needed.
>>>>
>>>>> +#include <numeric>
>>>>> +#include <unordered_map>
>>>>> +
>>>>> +#include "libcamera/internal/log.h"
>>>>> +
>>>>> +#include "ipu3_awb.h"
>>>>> +
>>>>> +using namespace libcamera;
>>>>> +
>>>>> +LOG_DEFINE_CATEGORY(IPU3Awb)
>>>>> +
>>>>> +#define NAME "ipu3.awb"
>>>>> +
>>>>> +IPU3Awb::IPU3Awb(IPAController *controller)
>>>>> +    : AwbAlgorithm(controller)
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +IPU3Awb::~IPU3Awb()
>>>>> +{
>>>>> +}
>>>>> +
>>>>> +char const *IPU3Awb::Name() const
>>>>> +{
>>>>> +    return NAME;
>>>>> +}
>>>>> +
>>>>> +void IPU3Awb::Initialise() {}
>>>>
>>>> void IPU3Awb::Initialise()
>>>> {
>>>> }
>>>>
>>>>> +
>>>>> +void IPU3Awb::Initialise(ipu3_uapi_params &params)
>>>>> +{
>>>>> +    memset(&params, 0, sizeof(params));
>>>>> +    memset(wbGains_, 8192, sizeof(wbGains_));
>>>>
>>>> memset() initializes the memory byte by byte, this will not store 8192
>>>> in each entry of the array.
>>>>
>>>>
>>>>> +    wbGains_[0] = 8192 * 0.8;
>>>>> +    wbGains_[3] = 8192 * 0.8;
>>>>
>>>> I would group the initialization of member variables together, either
>>>> before, or after the initialization of params.
>>>>
>>>>> +    params.use.acc_awb = 1;
>>>>> +    /*\todo fill the grid calculated based on BDS configuration */
>>>>> +    params.acc_param.awb.config = imgu_css_awb_defaults;
>>>>> +
>>>>> +    params.use.acc_bnr = 1;
>>>>> +    params.acc_param.bnr = imgu_css_bnr_defaults;
>>>>> +
>>>>> +    params.use.acc_ccm = 1;
>>>>> +    params.acc_param.ccm = imgu_css_ccm_3800k;
>>>>> +
>>>>> +    params.use.acc_gamma = 1;
>>>>> +    params.acc_param.gamma.gc_ctrl.enable = 1;
>>>>> +
>>>>> +    uint32_t a = (32 * 245) / (245 - 9);
>>>>> +
>>>>> +    for (uint32_t i = 0; i < 10; i++)
>>>>> +        params.acc_param.gamma.gc_lut.lut[i] = 0;
>>>>> +    for (uint32_t i = 10; i < 245; i++)
>>>>> +        params.acc_param.gamma.gc_lut.lut[i] = a * i + (0 - a * 9);
>>>>> +    for (uint32_t i = 245; i < 255; i++)
>>>>> +        params.acc_param.gamma.gc_lut.lut[i] = 32 * 245;
>>>>> +
>>>>> +    frame_count_ = 0;
>>>>> +    algoConverged_ = false;
>>>>> +}
>>>>> +
>>>>> +unsigned int IPU3Awb::GetConvergenceFrames() const
>>>>> +{
>>>>> +    // If colour gains have been explicitly set, there is no
>>>>> convergence
>>>>> +    // to happen, so no need to drop any frames - return zero.
>>>>> +    if (manual_r_ && manual_b_)
>>>>> +        return 0;
>>>>> +    else
>>>>> +        return config_.convergence_frames;
>>>>> +}
>>>>> +
>>>>> +void IPU3Awb::SetMode(std::string const &mode_name)
>>>>> +{
>>>>> +    mode_name_ = mode_name;
>>>>> +}
>>>>> +
>>>>> +void IPU3Awb::SetManualGains(double manual_r, double manual_b)
>>>>> +{
>>>>> +    // If any of these are 0.0, we swich back to auto.
>>>>> +    manual_r_ = manual_r;
>>>>> +    manual_b_ = manual_b;
>>>>> +}
>>>>> +
>>>>> +uint32_t IPU3Awb::estimateCCT(uint8_t R, uint8_t G, uint8_t B)
>>>>> +{
>>>>> +    double X = (-0.14282) * (R) + (1.54924) * (G) + (-0.95641) * (B);
>>>>> +    double Y = (-0.32466) * (R) + (1.57837) * (G) + (-0.73191) * (B);
>>>>> +    double Z = (-0.68202) * (R) + (0.77073) * (G) + (0.56332) * (B);
>>>>> +
>>>>> +    double x = X / (X + Y + Z);
>>>>> +    double y = Y / (X + Y + Z);
>>>>> +
>>>>> +    double n = (x - 0.3320) / (0.1858 - y);
>>>>> +    return static_cast<uint32_t>(449 * n * n * n + 3525 * n * n +
>>>>> 6823.3 * n + 5520.33);
>>>>> +}
>>>>> +
>>>>> +void IPU3Awb::calculateWBGains([[maybe_unused]] Rectangle roi, const
>>>>> ipu3_uapi_stats_3a *stats)
>>>>
>>>> Line wrap.
>>>>
>>>>> +{
>>>>> +    if (algoConverged_)
>>>>> +        return;
>>>>
>>>> So once it converges, you never update the gains anymore, even if the
>>>> scene changes ?
>>>>
>>>>> +
>>>>> +    std::vector<uint32_t> R_v, Gr_v, Gb_v, B_v;
>>>>
>>>> camelCase. Feel free to also make the name slightly longer to make the
>>>> more explicit. redValues ?
>>>>
>>>>> +    Point topleft = roi.topLeft();
>>>>> +    uint32_t startY = (topleft.y / 16) * 160 * 8;
>>>>> +    uint32_t startX = (topleft.x / 8) * 8;
>>>>> +    uint32_t endX = (startX + (roi.size().width / 8)) * 8;
>>>>> +
>>>>> +    for (uint32_t j = (topleft.y / 16); j < (topleft.y / 16) +
>>>>> (roi.size().height / 16); j++) {
>>>>> +        for (uint32_t i = startX + startY; i < endX + startY; i += 8) {
>>>>> +            Gr_v.push_back(stats->awb_raw_buffer.meta_data[i]);
>>>>> +            R_v.push_back(stats->awb_raw_buffer.meta_data[i + 1]);
>>>>> +            B_v.push_back(stats->awb_raw_buffer.meta_data[i + 2]);
>>>>> +            Gb_v.push_back(stats->awb_raw_buffer.meta_data[i + 3]);
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    std::sort(R_v.begin(), R_v.end());
>>>>> +    std::sort(Gr_v.begin(), Gr_v.end());
>>>>> +    std::sort(B_v.begin(), B_v.end());
>>>>> +    std::sort(Gb_v.begin(), Gb_v.end());
>>>>> +
>>>>> +    double Grmed = Gr_v[Gr_v.size() / 2];
>>>>> +    double Rmed = R_v[R_v.size() / 2];
>>>>> +    double Bmed = B_v[B_v.size() / 2];
>>>>> +    double Gbmed = Gb_v[Gb_v.size() / 2];
>>>>> +
>>>>> +    double Rgain = Grmed / Rmed;
>>>>> +    double Bgain = Gbmed / Bmed;
>>>>> +    LOG(IPU3Awb, Debug) << "max R, Gr, B, Gb: "
>>>>> +                << R_v.back() << ","
>>>>> +                << Gr_v.back() << ","
>>>>> +                << B_v.back() << ","
>>>>> +                << Gb_v.back();
>>>>> +    tint_ = ((Rmed / Grmed) + (Bmed / Gbmed)) / 2;
>>>>> +
>>>>> +    /* \todo Those are corrections when light is really low
>>>>> +     * it should be taken into account by AGC somehow */
>>>>> +    if ((Rgain >= 2) && (Bgain < 2)) {
>>>>> +        wbGains_[0] = 4096 * tint_;
>>>>> +        wbGains_[1] = 8192 * Rgain;
>>>>> +        wbGains_[2] = 4096 * Bgain;
>>>>> +        wbGains_[3] = 4096 * tint_;
>>>>> +    } else if ((Bgain >= 2) && (Rgain < 2)) {
>>>>> +        wbGains_[0] = 4096 * tint_;
>>>>> +        wbGains_[1] = 4096 * Rgain;
>>>>> +        wbGains_[2] = 8192 * Bgain;
>>>>> +        wbGains_[3] = 4096 * tint_;
>>>>> +    } else {
>>>>> +        wbGains_[0] = 8192 * tint_;
>>>>> +        wbGains_[1] = 8192 * Rgain;
>>>>> +        wbGains_[2] = 8192 * Bgain;
>>>>> +        wbGains_[3] = 8192 * tint_;
>>>>> +    }
>>>>> +
>>>>> +    frame_count_++;
>>>>> +
>>>>> +    cct_ = estimateCCT(Rmed, (Grmed + Gbmed) / 2, Bmed);
>>>>> +
>>>>> +    algoConverged_ = true;
>>>>> +}
>>>>> +
>>>>> +void IPU3Awb::updateBNR(ipu3_uapi_params &params)
>>>>
>>>> updateBNR() isn't a great name for a function that updates the WB gains,
>>>> even if they're part of the Bayer noise reduction block of data.
>>>>
>>>> Should we push the ipu3_uapi_params structure down to individual
>>>> algorithms, or would it be better to decouple it, creating custom
>>>> structures to hold algorithm results, and fillin ipu3_uapi_params in
>>>> IPAIPU3::fillParams() instead ? On one hand moving ipu3_uapi_params out
>>>> will make the code more generic, and thus more reusable, but on the
>>>> other hand we would still depend on a custom stats format, so it may not
>>>> be worth it.
>>>>
>>>>> +{
>>>>> +    if (!algoConverged_)
>>>>> +        return;
>>>>> +
>>>>> +    params.acc_param.bnr.wb_gains.gr = wbGains_[0];
>>>>> +    params.acc_param.bnr.wb_gains.r = wbGains_[1];
>>>>> +    params.acc_param.bnr.wb_gains.b = wbGains_[2];
>>>>> +    params.acc_param.bnr.wb_gains.gb = wbGains_[3];
>>>>> +    if (cct_ < 5500)
>>>>> +        params.acc_param.ccm = imgu_css_ccm_3800k;
>>>>> +    else
>>>>> +        params.acc_param.ccm = imgu_css_ccm_6000k;
>>>>> +}
>>>>> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
>>>>> new file mode 100644
>>>>> index 00000000..06389020
>>>>> --- /dev/null
>>>>> +++ b/src/ipa/ipu3/ipu3_awb.h
>>>>> @@ -0,0 +1,130 @@
>>>>> +/* SPDX-License-Identifier: BSD-2-Clause */
>>>>> +/*
>>>>> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
>>>>> + *
>>>>> + * awb.h - AWB control algorithm
>>>>> + */
>>>>> +#ifndef __LIBCAMERA_IPU3_AWB_H__
>>>>> +#define __LIBCAMERA_IPU3_AWB_H__
>>>>> +
>>>>> +#include <linux/intel-ipu3.h>
>>>>> +
>>>>> +#include <libcamera/geometry.h>
>>>>> +
>>>>> +#include <libcamera/ipa/awb_algorithm.h>
>>>>> +#include <libcamera/ipa/ipa_controller.h>
>>>>> +
>>>>> +namespace libcamera {
>>>>> +
>>>>> +const struct ipu3_uapi_bnr_static_config imgu_css_bnr_defaults = {
>>>>> +    { 16, 16, 16, 16 },            /* wb_gains */
>>>>> +    { 255, 255, 255, 255 },            /* wb_gains_thr */
>>>>> +    { 0, 0, 8, 6, 0, 14 },            /* thr_coeffs */
>>>>> +    { 0, 0, 0, 0 },                /* thr_ctrl_shd */
>>>>> +    { -648, 0, -366, 0 },            /* opt_center */
>>>>> +    {                    /* 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 }
>>>>> +    },
>>>>> +    { 4, 0, 1, 8, 0, 8, 0, 8, 0 },        /* bp_ctrl */
>>>>> +    { 8, 4, 4, 0, 8, 0, 1, 1, 1, 1, 0 },    /* dn_detect_ctrl */
>>>>> +    1296,
>>>>> +    { 419904, 133956 },
>>>>> +};
>>>>> +
>>>>> +/* settings for Auto White Balance */
>>>>> +const struct ipu3_uapi_awb_config_s imgu_css_awb_defaults = {
>>>>> +    8191,
>>>>> +    8191,
>>>>> +    8191,
>>>>> +    8191 | /* rgbs_thr_gr/r/gb/b */
>>>>> +        IPU3_UAPI_AWB_RGBS_THR_B_EN |
>>>>> IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
>>>>> +    .grid = {
>>>>> +        .width = 160,
>>>>> +        .height = 45,
>>>>> +        .block_width_log2 = 3,
>>>>> +        .block_height_log2 = 4,
>>>>> +        .x_start = 0,
>>>>> +        .y_start = 0,
>>>>> +    },
>>>>> +};
>>>>> +
>>>>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_6000k = {
>>>>> +    7239, -750, -37, 0,
>>>>> +    -215, 9176, -200, 0,
>>>>> +    -70, -589, 6810, 0
>>>>> +};
>>>>> +
>>>>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_4900k = {
>>>>> +    7811, -464, -466, 0,
>>>>> +    -635, 8762, -533, 0,
>>>>> +    -469, -154, 6583, 0
>>>>> +};
>>>>> +
>>>>> +const struct ipu3_uapi_ccm_mat_config imgu_css_ccm_3800k = {
>>>>> +    7379, -526, -296, 0,
>>>>> +    -411, 7397, -415, 0,
>>>>> +    -224, -564, 7244, 0
>>>>> +};
>>>>
>>>> All these should be moved to ipu3_awb.cpp, and be made static. Otherwise
>>>> they would get duplicated in every compilation unit in which this header
>>>> is included.
>>>>
>>>>> +
>>>>> +struct AwbConfig {
>>>>> +    // Only repeat the AWB calculation every "this many" frames
>>>>> +    uint16_t frame_period;
>>>>> +    // number of initial frames for which speed taken as 1.0 (maximum)
>>>>> +    uint16_t startup_frames;
>>>>> +    unsigned int convergence_frames; // approx number of frames to
>>>>> converge
>>>>> +    double speed; // IIR filter speed applied to algorithm results
>>>>> +};
>>>>> +
>>>>> +#if 0
>>>>> +typedef struct awb_public_set_item{
>>>>> +    unsigned char Gr_avg;
>>>>> +    unsigned char R_avg;
>>>>> +    unsigned char B_avg;
>>>>> +    unsigned char Gb_avg;
>>>>> +    unsigned char sat_ratio;
>>>>> +    unsigned char padding0; /**< Added the padding so that the
>>>>> public matches that private */
>>>>> +    unsigned char padding1; /**< Added the padding so that the
>>>>> public matches that private */
>>>>> +    unsigned char padding2; /**< Added the padding so that the
>>>>> public matches that private */
>>>>> +} awb_public_set_item_t;
>>>>> +#endif
>>>>
>>>> If the compiler doesn't need to see this, I think you can drop it
>>>> completely :-)
>>>>
>>>>> +
>>>>> +class IPU3Awb : public AwbAlgorithm
>>>>> +{
>>>>> +public:
>>>>> +    IPU3Awb(IPAController *controller = NULL);
>>>>
>>>> This is C++, so nullptr :-) But there's no need for a default value, as
>>>> the controller parameter should either be mandatory, or removed
>>>> completely.
>>>>
>>>>> +    ~IPU3Awb();
>>>>
>>>> Blank line.
>>>>
>>>>> +    virtual char const *Name() const override;
>>>>
>>>> This is never used, you can drop it from the base class. When (and if)
>>>> we'll need this, I'd rather pass the name to the base class constructor,
>>>> store it there, and implement name() in the base class only, as a
>>>> non-virtual function.
>>>>
>>>>> +    virtual void Initialise() override;
>>>>
>>>> No need to repeat the virtual keyword for overridden functions.
>>>>
>>>>> +    void Initialise(ipu3_uapi_params &params);
>>>>
>>>> This isn't very nice, as it's not aligned with the design of the
>>>> IPAAlgorithm class, but we can fix it later. I would move the function
>>>> down, after the mandatory functions, with calculateWBGains() and
>>>> updateBNR().
>>>>
>>>>> +    unsigned int GetConvergenceFrames() const override;
>>>>> +    void SetMode(std::string const &name) override;
>>>>> +    void SetManualGains(double manual_r, double manual_b) override;
>>>>
>>>> A blank line here would be nice.
>>>>
>>>>> +    void calculateWBGains(Rectangle roi,
>>>>
>>>> const Rectangle &roi
>>>>
>>>>> +                  const ipu3_uapi_stats_3a *stats);
>>>>> +    void updateBNR(ipu3_uapi_params &params);
>>>>> +
>>>>> +private:
>>>>> +    uint32_t estimateCCT(uint8_t R, uint8_t G, uint8_t B);
>>>>
>>>> r, g and b.
>>>>
>>>>> +
>>>>> +    /* configuration is read-only, and available to both threads */
>>>>
>>>> What threads ? :-)
>>>>
>>>>> +    AwbConfig config_;
>>>>> +    std::string mode_name_;
>>>>
>>>> Never used.
>>>>
>>>>> +    /* manual r setting */
>>>>> +    double manual_r_;
>>>>
>>>> manualRedGain_ ?
>>>>
>>>>> +    /* manual b setting */
>>>>> +    double manual_b_;
>>>>> +    /* WB calculated gains */
>>>>> +    uint16_t wbGains_[4];
>>>>> +    double tint_;
>>>>> +    uint32_t cct_;
>>>>> +
>>>>> +    uint32_t frame_count_;
>>>>> +
>>>>> +    bool algoConverged_;
>>>>> +};
>>>>> +} /* namespace libcamera */
>>>>> +
>>>>> +#endif /* __LIBCAMERA_IPU3_AWB_H__ */
>>>>> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
>>>>> index a241f617..43ad0e0d 100644
>>>>> --- a/src/ipa/ipu3/meson.build
>>>>> +++ b/src/ipa/ipu3/meson.build
>>>>> @@ -2,8 +2,14 @@
>>>>>     ipa_name = 'ipa_ipu3'
>>>>>   +ipu3_ipa_sources = files([
>>>>> +  'ipu3.cpp',
>>>>> +  'ipu3_agc.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,
>>>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>> b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>> index ff980b38..3809c943 100644
>>>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>>>> @@ -825,6 +825,7 @@ int
>>>>> PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>>>>>        */
>>>>>       double lineDuration = sensorInfo.lineLength
>>>>>                   / (sensorInfo.pixelRate / 1e6);
>>>>> +    LOG(IPU3, Error) << "line duration: " << lineDuration;
>>>>
>>>> Is this an error ? :-)
>>>>
>>>>>       const ControlInfoMap &sensorControls = sensor->controls();
>>>>>       const ControlInfo &v4l2Exposure =
>>>>> sensorControls.find(V4L2_CID_EXPOSURE)->second;
>>>>>       int32_t minExposure = v4l2Exposure.min().get<int32_t>() *
>>>>> lineDuration;
>