[libcamera-devel,v2,03/10] ipa: ipu3: Introduce modular grid algorithm

13331 diff mbox series

Implement a new modular framework for algorithms with a common context
structure that is passed to each algorithm through a common API.

The initial algorithm is chosen to configure the Bayer Down Scaler grid
which is moved from the IPAIPU3 class.

This patch:
- adds a configure() function to the Algorithm interface
- creates a new Grid class implementing the computation at configure
  call
- removes all the local references from IPAIPU3
- implements the list of pointers and the loop at configure call on each
  algorithm (right now, only Grid is in the list)
- removes the imguCssAwbDefaults structure as it is now configured
  properly

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/algorithm.h | 11 ++++
 src/ipa/ipu3/algorithms/grid.cpp    | 83 +++++++++++++++++++++++++++++
 src/ipa/ipu3/algorithms/grid.h      | 29 ++++++++++
 src/ipa/ipu3/algorithms/meson.build |  1 +
 src/ipa/ipu3/ipa_context.h          |  8 +++
 src/ipa/ipu3/ipu3.cpp               | 75 ++++++--------------------
 src/ipa/ipu3/ipu3_awb.cpp           | 27 ++--------
 7 files changed, 154 insertions(+), 80 deletions(-)
 create mode 100644 src/ipa/ipu3/algorithms/grid.cpp
 create mode 100644 src/ipa/ipu3/algorithms/grid.h

Hi JM,


On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
> Implement a new modular framework for algorithms with a common context
> structure that is passed to each algorithm through a common API.
> 
> The initial algorithm is chosen to configure the Bayer Down Scaler grid
> which is moved from the IPAIPU3 class.
> 
> This patch:
> - adds a configure() function to the Algorithm interface
> - creates a new Grid class implementing the computation at configure
>   call
> - removes all the local references from IPAIPU3
> - implements the list of pointers and the loop at configure call on each
>   algorithm (right now, only Grid is in the list)
> - removes the imguCssAwbDefaults structure as it is now configured
>   properly
> 

A busy patch, but I think in this instance it is reasonable to do all of
that in a single patch.



> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/algorithm.h | 11 ++++
>  src/ipa/ipu3/algorithms/grid.cpp    | 83 +++++++++++++++++++++++++++++
>  src/ipa/ipu3/algorithms/grid.h      | 29 ++++++++++
>  src/ipa/ipu3/algorithms/meson.build |  1 +
>  src/ipa/ipu3/ipa_context.h          |  8 +++
>  src/ipa/ipu3/ipu3.cpp               | 75 ++++++--------------------
>  src/ipa/ipu3/ipu3_awb.cpp           | 27 ++--------
>  7 files changed, 154 insertions(+), 80 deletions(-)
>  create mode 100644 src/ipa/ipu3/algorithms/grid.cpp
>  create mode 100644 src/ipa/ipu3/algorithms/grid.h
> 
> diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> index 072f01c4..c1b37276 100644
> --- a/src/ipa/ipu3/algorithms/algorithm.h
> +++ b/src/ipa/ipu3/algorithms/algorithm.h
> @@ -7,6 +7,12 @@
>  #ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
>  #define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
>  
> +#include <iostream>
> +
> +#include <libcamera/ipa/ipu3_ipa_interface.h>
> +
> +#include "ipa_context.h"
> +
>  namespace libcamera {
>  
>  namespace ipa::ipu3 {
> @@ -15,6 +21,11 @@ class Algorithm
>  {
>  public:
>  	virtual ~Algorithm() {}
> +
> +	virtual int configure([[maybe_unused]] IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo)
> +	{
> +		return 0;
> +	}
>  };
>  
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp
> new file mode 100644
> index 00000000..3578f41b
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/grid.cpp
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * grid.cpp - IPU3 grid configuration
> + */
> +
> +#include "grid.h"
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3::algorithms {
> +
> +LOG_DEFINE_CATEGORY(IPU3Grid)
> +
> +/* Maximum number of cells on a row */
> +static constexpr uint32_t kMaxCellWidthPerSet = 160;
> +/* Maximum number of cells on a column */
> +static constexpr uint32_t kMaxCellHeightPerSet = 56;
> +
> +/**
> + * This function calculates a grid for the AWB algorithm in the IPU3 firmware.
> + * Its input is the BDS output size calculated in the ImgU.
> + * It is limited for now to the simplest method: find the lesser error
> + * with the width/height and respective log2 width/height of the cells.
> + *
> + * \todo The frame is divided into cells which can be 8x8 => 128x128.
> + * As a smaller cell improves the algorithm precision, adapting the
> + * x_start and y_start parameters of the grid would provoke a loss of
> + * some pixels but would also result in more accurate algorithms.
> + */
> +int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> +{
> +	uint32_t minError = std::numeric_limits<uint32_t>::max();
> +	Size best;
> +	Size bestLog2;
> +	ipu3_uapi_grid_config &bdsGrid = context.configuration.grid.bdsGrid;
> +
> +	context.configuration.grid.bdsOutputSize = configInfo.bdsOutputSize;
> +	Size &bdsOutputSize = context.configuration.grid.bdsOutputSize;
> +
> +	bdsGrid.x_start = 0;
> +	bdsGrid.y_start = 0;
> +
> +	for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
> +		uint32_t width = std::min(kMaxCellWidthPerSet,
> +					  bdsOutputSize.width >> widthShift);
> +		width = width << widthShift;
> +		for (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {
> +			int32_t height = std::min(kMaxCellHeightPerSet,
> +						  bdsOutputSize.height >> heightShift);
> +			height = height << heightShift;
> +			uint32_t error = std::abs(static_cast<int>(width - bdsOutputSize.width))
> +				       + std::abs(static_cast<int>(height - bdsOutputSize.height));
> +
> +			if (error > minError)
> +				continue;
> +
> +			minError = error;
> +			best.width = width;
> +			best.height = height;
> +			bestLog2.width = widthShift;
> +			bestLog2.height = heightShift;
> +		}
> +	}
> +
> +	bdsGrid.width = best.width >> bestLog2.width;
> +	bdsGrid.block_width_log2 = bestLog2.width;
> +	bdsGrid.height = best.height >> bestLog2.height;
> +	bdsGrid.block_height_log2 = bestLog2.height;
> +
> +	LOG(IPU3Grid, Debug) << "Best grid found is: ("
> +			     << (int)bdsGrid.width << " << " << (int)bdsGrid.block_width_log2 << ") x ("
> +			     << (int)bdsGrid.height << " << " << (int)bdsGrid.block_height_log2 << ")";
> +
> +	return 0;
> +}
> +
> +} /* namespace ipa::ipu3::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h
> new file mode 100644
> index 00000000..b4a51b42
> --- /dev/null
> +++ b/src/ipa/ipu3/algorithms/grid.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google
> + *
> + * grid.h - IPU3 grid configuration
> + */
> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_GRID_H__
> +#define __LIBCAMERA_IPU3_ALGORITHMS_GRID_H__
> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3::algorithms {
> +
> +class Grid : public Algorithm
> +{
> +public:
> +	~Grid() = default;
> +
> +	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> +};
> +
> +} /* namespace ipa::ipu3::algorithms */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__ */
> +
> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> index 67148333..3fb3ce56 100644
> --- a/src/ipa/ipu3/algorithms/meson.build
> +++ b/src/ipa/ipu3/algorithms/meson.build
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  ipu3_ipa_algorithms = files([
> +        'grid.cpp',
>  ])
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 0a197a41..90b2f2c2 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -11,12 +11,20 @@
>  
>  #include <linux/intel-ipu3.h>
>  
> +#include <libcamera/geometry.h>
> +
>  namespace libcamera {
>  
>  namespace ipa::ipu3 {
>  
>  /* Fixed configuration of the IPA */
>  struct IPAConfiguration {
> +	struct Grid {
> +		/* Bayer Down Scaler grid plane config used by the kernel */
> +		ipu3_uapi_grid_config bdsGrid;

I always find structures with line comments above better with a blank
line in-between, otherwise they 'merge together' in my eyes. But that's
just personal preference, so not an issue.


> +		/* BDS output size configured by the pipeline handler */
> +		Size bdsOutputSize;
> +	} grid;
>  };
>  
>  /*
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index c34fa460..ef7fec86 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -29,13 +29,12 @@
>  
>  #include "libcamera/internal/mapped_framebuffer.h"
>  
> +#include "algorithms/algorithm.h"
> +#include "algorithms/grid.h"
>  #include "ipu3_agc.h"
>  #include "ipu3_awb.h"
>  #include "libipa/camera_sensor_helper.h"
>  
> -static constexpr uint32_t kMaxCellWidthPerSet = 160;
> -static constexpr uint32_t kMaxCellHeightPerSet = 56;
> -
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPAIPU3)
> @@ -91,10 +90,12 @@ private:
>  	/* Interface to the Camera Helper */
>  	std::unique_ptr<CameraSensorHelper> camHelper_;
>  
> +	/* Maintain the algorithms used by the IPA */
> +	std::list<std::unique_ptr<ipa::ipu3::Algorithm>> algorithms_;
> +
>  	/* Local parameter storage */
> +	struct IPAContext context_;
>  	struct ipu3_uapi_params params_;
> -
> -	struct ipu3_uapi_grid_config bdsGrid_;
>  };
>  
>  /**
> @@ -164,6 +165,8 @@ int IPAIPU3::init(const IPASettings &settings,
>  							       frameDurations[2]);
>  
>  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);

I think a new line here is deserved though, Adding algorithms to the
list, is unrelated to initialising the controls - so they should be
separated with a blank line.


Two trivial blank lines is the most I can say ... so I think that
satisfies a:

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


> +	/* Construct our Algorithms */
> +	algorithms_.emplace_back(new algorithms::Grid());
>  
>  	return 0;
>  }
> @@ -175,56 +178,6 @@ int IPAIPU3::start()
>  	return 0;
>  }
>  
> -/**
> - * This function calculates a grid for the AWB algorithm in the IPU3 firmware.
> - * Its input is the BDS output size calculated in the ImgU.
> - * It is limited for now to the simplest method: find the lesser error
> - * with the width/height and respective log2 width/height of the cells.
> - *
> - * \todo The frame is divided into cells which can be 8x8 => 128x128.
> - * As a smaller cell improves the algorithm precision, adapting the
> - * x_start and y_start parameters of the grid would provoke a loss of
> - * some pixels but would also result in more accurate algorithms.
> - */
> -void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
> -{
> -	uint32_t minError = std::numeric_limits<uint32_t>::max();
> -	Size best;
> -	Size bestLog2;
> -	bdsGrid_ = {};
> -
> -	for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
> -		uint32_t width = std::min(kMaxCellWidthPerSet,
> -					  bdsOutputSize.width >> widthShift);
> -		width = width << widthShift;
> -		for (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {
> -			int32_t height = std::min(kMaxCellHeightPerSet,
> -						  bdsOutputSize.height >> heightShift);
> -			height = height << heightShift;
> -			uint32_t error  = std::abs(static_cast<int>(width - bdsOutputSize.width))
> -							+ std::abs(static_cast<int>(height - bdsOutputSize.height));
> -
> -			if (error > minError)
> -				continue;
> -
> -			minError = error;
> -			best.width = width;
> -			best.height = height;
> -			bestLog2.width = widthShift;
> -			bestLog2.height = heightShift;
> -		}
> -	}
> -
> -	bdsGrid_.width = best.width >> bestLog2.width;
> -	bdsGrid_.block_width_log2 = bestLog2.width;
> -	bdsGrid_.height = best.height >> bestLog2.height;
> -	bdsGrid_.block_height_log2 = bestLog2.height;
> -
> -	LOG(IPAIPU3, Debug) << "Best grid found is: ("
> -			    << (int)bdsGrid_.width << " << " << (int)bdsGrid_.block_width_log2 << ") x ("
> -			    << (int)bdsGrid_.height << " << " << (int)bdsGrid_.block_height_log2 << ")";
> -}
> -
>  int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  {
>  	if (configInfo.entityControls.empty()) {
> @@ -264,15 +217,21 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  
>  	defVBlank_ = itVBlank->second.def().get<int32_t>();
>  
> +	/* Clean context and IPU3 parameters at configuration */
>  	params_ = {};
> +	context_ = {};
>  
> -	calculateBdsGrid(configInfo.bdsOutputSize);
> +	for (auto const &algo : algorithms_) {
> +		int ret = algo->configure(context_, configInfo);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	awbAlgo_ = std::make_unique<IPU3Awb>();
> -	awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
> +	awbAlgo_->initialise(params_, context_.configuration.grid.bdsOutputSize, context_.configuration.grid.bdsGrid);
>  
>  	agcAlgo_ = std::make_unique<IPU3Agc>();
> -	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
> +	agcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_);
>  
>  	return 0;
>  }
> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> index 4bb321b3..4ee5ee6f 100644
> --- a/src/ipa/ipu3/ipu3_awb.cpp
> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> @@ -107,25 +107,6 @@ static const struct ipu3_uapi_bnr_static_config imguCssBnrDefaults = {
>  	.opt_center_sqr = { 419904, 133956 },
>  };
>  
> -/* Default settings for Auto White Balance replicated from the Kernel*/
> -static const struct ipu3_uapi_awb_config_s imguCssAwbDefaults = {
> -	.rgbs_thr_gr = 8191,
> -	.rgbs_thr_r = 8191,
> -	.rgbs_thr_gb = 8191,
> -	.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
> -	.grid = {
> -		.width = 160,
> -		.height = 36,
> -		.block_width_log2 = 3,
> -		.block_height_log2 = 4,
> -		.height_per_slice = 1, /* Overridden by kernel. */
> -		.x_start = 0,
> -		.y_start = 0,
> -		.x_end = 0,
> -		.y_end = 0,
> -	},
> -};
> -
>  /* Default color correction matrix defined as an identity matrix */
>  static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
>  	8191, 0, 0, 0,
> @@ -174,10 +155,12 @@ IPU3Awb::~IPU3Awb()
>  void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)
>  {
>  	params.use.acc_awb = 1;
> -	params.acc_param.awb.config = imguCssAwbDefaults;
> -
> +	params.acc_param.awb.config.rgbs_thr_gr = 8191;
> +	params.acc_param.awb.config.rgbs_thr_r = 8191;
> +	params.acc_param.awb.config.rgbs_thr_gb = 8191;
> +	params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;
> +	params.acc_param.awb.config.grid = bdsGrid;
>  	awbGrid_ = bdsGrid;
> -	params.acc_param.awb.config.grid = awbGrid_;
>  
>  	params.use.acc_bnr = 1;
>  	params.acc_param.bnr = imguCssBnrDefaults;
>

Hi Jean-Michel,

Thank you for the patch.

On Fri, Aug 13, 2021 at 09:40:09AM +0100, Kieran Bingham wrote:
> On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
> > Implement a new modular framework for algorithms with a common context
> > structure that is passed to each algorithm through a common API.
> > 
> > The initial algorithm is chosen to configure the Bayer Down Scaler grid
> > which is moved from the IPAIPU3 class.
> > 
> > This patch:
> > - adds a configure() function to the Algorithm interface
> > - creates a new Grid class implementing the computation at configure
> >   call
> > - removes all the local references from IPAIPU3
> > - implements the list of pointers and the loop at configure call on each
> >   algorithm (right now, only Grid is in the list)
> > - removes the imguCssAwbDefaults structure as it is now configured
> >   properly
> 
> A busy patch, but I think in this instance it is reasonable to do all of
> that in a single patch.

I'm sure Niklas would have disagreed ;-) I tend to like splitting things
up too, possibly not as much as he does though.

> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/algorithm.h | 11 ++++
> >  src/ipa/ipu3/algorithms/grid.cpp    | 83 +++++++++++++++++++++++++++++
> >  src/ipa/ipu3/algorithms/grid.h      | 29 ++++++++++
> >  src/ipa/ipu3/algorithms/meson.build |  1 +
> >  src/ipa/ipu3/ipa_context.h          |  8 +++
> >  src/ipa/ipu3/ipu3.cpp               | 75 ++++++--------------------
> >  src/ipa/ipu3/ipu3_awb.cpp           | 27 ++--------
> >  7 files changed, 154 insertions(+), 80 deletions(-)
> >  create mode 100644 src/ipa/ipu3/algorithms/grid.cpp
> >  create mode 100644 src/ipa/ipu3/algorithms/grid.h
> > 
> > diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> > index 072f01c4..c1b37276 100644
> > --- a/src/ipa/ipu3/algorithms/algorithm.h
> > +++ b/src/ipa/ipu3/algorithms/algorithm.h
> > @@ -7,6 +7,12 @@
> >  #ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
> >  #define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
> >  
> > +#include <iostream>
> > +
> > +#include <libcamera/ipa/ipu3_ipa_interface.h>
> > +
> > +#include "ipa_context.h"
> > +
> >  namespace libcamera {
> >  
> >  namespace ipa::ipu3 {
> > @@ -15,6 +21,11 @@ class Algorithm
> >  {
> >  public:
> >  	virtual ~Algorithm() {}
> > +
> > +	virtual int configure([[maybe_unused]] IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo)

Line wrap.

> > +	{
> > +		return 0;
> > +	}
> >  };
> >  
> >  } /* namespace ipa::ipu3 */
> > diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp
> > new file mode 100644
> > index 00000000..3578f41b
> > --- /dev/null
> > +++ b/src/ipa/ipu3/algorithms/grid.cpp
> > @@ -0,0 +1,83 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Ideas On Board
> > + *
> > + * grid.cpp - IPU3 grid configuration
> > + */
> > +
> > +#include "grid.h"
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::ipu3::algorithms {
> > +
> > +LOG_DEFINE_CATEGORY(IPU3Grid)
> > +
> > +/* Maximum number of cells on a row */
> > +static constexpr uint32_t kMaxCellWidthPerSet = 160;
> > +/* Maximum number of cells on a column */
> > +static constexpr uint32_t kMaxCellHeightPerSet = 56;
> > +
> > +/**
> > + * This function calculates a grid for the AWB algorithm in the IPU3 firmware.
> > + * Its input is the BDS output size calculated in the ImgU.
> > + * It is limited for now to the simplest method: find the lesser error
> > + * with the width/height and respective log2 width/height of the cells.
> > + *
> > + * \todo The frame is divided into cells which can be 8x8 => 128x128.
> > + * As a smaller cell improves the algorithm precision, adapting the
> > + * x_start and y_start parameters of the grid would provoke a loss of
> > + * some pixels but would also result in more accurate algorithms.
> > + */
> > +int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)

I don't think this qualifies as an "algorithm" I'm afraid. I'd keep it
in the IPU3IPA class.

> > +{
> > +	uint32_t minError = std::numeric_limits<uint32_t>::max();
> > +	Size best;
> > +	Size bestLog2;
> > +	ipu3_uapi_grid_config &bdsGrid = context.configuration.grid.bdsGrid;
> > +
> > +	context.configuration.grid.bdsOutputSize = configInfo.bdsOutputSize;
> > +	Size &bdsOutputSize = context.configuration.grid.bdsOutputSize;
> > +
> > +	bdsGrid.x_start = 0;
> > +	bdsGrid.y_start = 0;
> > +
> > +	for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
> > +		uint32_t width = std::min(kMaxCellWidthPerSet,
> > +					  bdsOutputSize.width >> widthShift);
> > +		width = width << widthShift;
> > +		for (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {
> > +			int32_t height = std::min(kMaxCellHeightPerSet,
> > +						  bdsOutputSize.height >> heightShift);
> > +			height = height << heightShift;
> > +			uint32_t error = std::abs(static_cast<int>(width - bdsOutputSize.width))
> > +				       + std::abs(static_cast<int>(height - bdsOutputSize.height));
> > +
> > +			if (error > minError)
> > +				continue;
> > +
> > +			minError = error;
> > +			best.width = width;
> > +			best.height = height;
> > +			bestLog2.width = widthShift;
> > +			bestLog2.height = heightShift;
> > +		}
> > +	}
> > +
> > +	bdsGrid.width = best.width >> bestLog2.width;
> > +	bdsGrid.block_width_log2 = bestLog2.width;
> > +	bdsGrid.height = best.height >> bestLog2.height;
> > +	bdsGrid.block_height_log2 = bestLog2.height;
> > +
> > +	LOG(IPU3Grid, Debug) << "Best grid found is: ("
> > +			     << (int)bdsGrid.width << " << " << (int)bdsGrid.block_width_log2 << ") x ("
> > +			     << (int)bdsGrid.height << " << " << (int)bdsGrid.block_height_log2 << ")";
> > +
> > +	return 0;
> > +}
> > +
> > +} /* namespace ipa::ipu3::algorithms */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h
> > new file mode 100644
> > index 00000000..b4a51b42
> > --- /dev/null
> > +++ b/src/ipa/ipu3/algorithms/grid.h
> > @@ -0,0 +1,29 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google
> > + *
> > + * grid.h - IPU3 grid configuration
> > + */
> > +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_GRID_H__
> > +#define __LIBCAMERA_IPU3_ALGORITHMS_GRID_H__
> > +
> > +#include "algorithm.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa::ipu3::algorithms {
> > +
> > +class Grid : public Algorithm
> > +{
> > +public:
> > +	~Grid() = default;
> > +
> > +	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> > +};
> > +
> > +} /* namespace ipa::ipu3::algorithms */
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__ */
> > +
> > diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> > index 67148333..3fb3ce56 100644
> > --- a/src/ipa/ipu3/algorithms/meson.build
> > +++ b/src/ipa/ipu3/algorithms/meson.build
> > @@ -1,4 +1,5 @@
> >  # SPDX-License-Identifier: CC0-1.0
> >  
> >  ipu3_ipa_algorithms = files([
> > +        'grid.cpp',
> >  ])
> > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > index 0a197a41..90b2f2c2 100644
> > --- a/src/ipa/ipu3/ipa_context.h
> > +++ b/src/ipa/ipu3/ipa_context.h
> > @@ -11,12 +11,20 @@
> >  
> >  #include <linux/intel-ipu3.h>
> >  
> > +#include <libcamera/geometry.h>
> > +
> >  namespace libcamera {
> >  
> >  namespace ipa::ipu3 {
> >  
> >  /* Fixed configuration of the IPA */
> >  struct IPAConfiguration {
> > +	struct Grid {
> > +		/* Bayer Down Scaler grid plane config used by the kernel */
> > +		ipu3_uapi_grid_config bdsGrid;
> 
> I always find structures with line comments above better with a blank
> line in-between, otherwise they 'merge together' in my eyes. But that's
> just personal preference, so not an issue.

As this is meant to be a reference implementation, documentation is
fairly important, and I'd prefer if we used Doxygen formatting. It thus
brings the question of whether we want to keep it here or move it to a
.cpp file.

> > +		/* BDS output size configured by the pipeline handler */
> > +		Size bdsOutputSize;
> > +	} grid;
> >  };
> >  
> >  /*
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index c34fa460..ef7fec86 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -29,13 +29,12 @@
> >  
> >  #include "libcamera/internal/mapped_framebuffer.h"
> >  
> > +#include "algorithms/algorithm.h"
> > +#include "algorithms/grid.h"
> >  #include "ipu3_agc.h"
> >  #include "ipu3_awb.h"
> >  #include "libipa/camera_sensor_helper.h"
> >  
> > -static constexpr uint32_t kMaxCellWidthPerSet = 160;
> > -static constexpr uint32_t kMaxCellHeightPerSet = 56;
> > -
> >  namespace libcamera {
> >  
> >  LOG_DEFINE_CATEGORY(IPAIPU3)
> > @@ -91,10 +90,12 @@ private:
> >  	/* Interface to the Camera Helper */
> >  	std::unique_ptr<CameraSensorHelper> camHelper_;
> >  
> > +	/* Maintain the algorithms used by the IPA */
> > +	std::list<std::unique_ptr<ipa::ipu3::Algorithm>> algorithms_;
> > +
> >  	/* Local parameter storage */
> > +	struct IPAContext context_;
> >  	struct ipu3_uapi_params params_;
> > -
> > -	struct ipu3_uapi_grid_config bdsGrid_;
> >  };
> >  
> >  /**
> > @@ -164,6 +165,8 @@ int IPAIPU3::init(const IPASettings &settings,
> >  							       frameDurations[2]);
> >  
> >  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> 
> I think a new line here is deserved though, Adding algorithms to the
> list, is unrelated to initialising the controls - so they should be
> separated with a blank line.
> 
> Two trivial blank lines is the most I can say ... so I think that
> satisfies a:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > +	/* Construct our Algorithms */
> > +	algorithms_.emplace_back(new algorithms::Grid());
> >  
> >  	return 0;
> >  }
> > @@ -175,56 +178,6 @@ int IPAIPU3::start()
> >  	return 0;
> >  }
> >  
> > -/**
> > - * This function calculates a grid for the AWB algorithm in the IPU3 firmware.
> > - * Its input is the BDS output size calculated in the ImgU.
> > - * It is limited for now to the simplest method: find the lesser error
> > - * with the width/height and respective log2 width/height of the cells.
> > - *
> > - * \todo The frame is divided into cells which can be 8x8 => 128x128.
> > - * As a smaller cell improves the algorithm precision, adapting the
> > - * x_start and y_start parameters of the grid would provoke a loss of
> > - * some pixels but would also result in more accurate algorithms.
> > - */
> > -void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
> > -{
> > -	uint32_t minError = std::numeric_limits<uint32_t>::max();
> > -	Size best;
> > -	Size bestLog2;
> > -	bdsGrid_ = {};
> > -
> > -	for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
> > -		uint32_t width = std::min(kMaxCellWidthPerSet,
> > -					  bdsOutputSize.width >> widthShift);
> > -		width = width << widthShift;
> > -		for (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {
> > -			int32_t height = std::min(kMaxCellHeightPerSet,
> > -						  bdsOutputSize.height >> heightShift);
> > -			height = height << heightShift;
> > -			uint32_t error  = std::abs(static_cast<int>(width - bdsOutputSize.width))
> > -							+ std::abs(static_cast<int>(height - bdsOutputSize.height));
> > -
> > -			if (error > minError)
> > -				continue;
> > -
> > -			minError = error;
> > -			best.width = width;
> > -			best.height = height;
> > -			bestLog2.width = widthShift;
> > -			bestLog2.height = heightShift;
> > -		}
> > -	}
> > -
> > -	bdsGrid_.width = best.width >> bestLog2.width;
> > -	bdsGrid_.block_width_log2 = bestLog2.width;
> > -	bdsGrid_.height = best.height >> bestLog2.height;
> > -	bdsGrid_.block_height_log2 = bestLog2.height;
> > -
> > -	LOG(IPAIPU3, Debug) << "Best grid found is: ("
> > -			    << (int)bdsGrid_.width << " << " << (int)bdsGrid_.block_width_log2 << ") x ("
> > -			    << (int)bdsGrid_.height << " << " << (int)bdsGrid_.block_height_log2 << ")";
> > -}
> > -
> >  int IPAIPU3::configure(const IPAConfigInfo &configInfo)
> >  {
> >  	if (configInfo.entityControls.empty()) {
> > @@ -264,15 +217,21 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
> >  
> >  	defVBlank_ = itVBlank->second.def().get<int32_t>();
> >  
> > +	/* Clean context and IPU3 parameters at configuration */
> >  	params_ = {};
> > +	context_ = {};
> >  
> > -	calculateBdsGrid(configInfo.bdsOutputSize);
> > +	for (auto const &algo : algorithms_) {
> > +		int ret = algo->configure(context_, configInfo);
> > +		if (ret)
> > +			return ret;
> > +	}
> >  
> >  	awbAlgo_ = std::make_unique<IPU3Awb>();
> > -	awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
> > +	awbAlgo_->initialise(params_, context_.configuration.grid.bdsOutputSize, context_.configuration.grid.bdsGrid);
> >  
> >  	agcAlgo_ = std::make_unique<IPU3Agc>();
> > -	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
> > +	agcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_);
> >  
> >  	return 0;
> >  }
> > diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> > index 4bb321b3..4ee5ee6f 100644
> > --- a/src/ipa/ipu3/ipu3_awb.cpp
> > +++ b/src/ipa/ipu3/ipu3_awb.cpp
> > @@ -107,25 +107,6 @@ static const struct ipu3_uapi_bnr_static_config imguCssBnrDefaults = {
> >  	.opt_center_sqr = { 419904, 133956 },
> >  };
> >  
> > -/* Default settings for Auto White Balance replicated from the Kernel*/
> > -static const struct ipu3_uapi_awb_config_s imguCssAwbDefaults = {
> > -	.rgbs_thr_gr = 8191,
> > -	.rgbs_thr_r = 8191,
> > -	.rgbs_thr_gb = 8191,
> > -	.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
> > -	.grid = {
> > -		.width = 160,
> > -		.height = 36,
> > -		.block_width_log2 = 3,
> > -		.block_height_log2 = 4,
> > -		.height_per_slice = 1, /* Overridden by kernel. */
> > -		.x_start = 0,
> > -		.y_start = 0,
> > -		.x_end = 0,
> > -		.y_end = 0,
> > -	},
> > -};
> > -
> >  /* Default color correction matrix defined as an identity matrix */
> >  static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
> >  	8191, 0, 0, 0,
> > @@ -174,10 +155,12 @@ IPU3Awb::~IPU3Awb()
> >  void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)
> >  {
> >  	params.use.acc_awb = 1;
> > -	params.acc_param.awb.config = imguCssAwbDefaults;
> > -
> > +	params.acc_param.awb.config.rgbs_thr_gr = 8191;
> > +	params.acc_param.awb.config.rgbs_thr_r = 8191;
> > +	params.acc_param.awb.config.rgbs_thr_gb = 8191;
> > +	params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;

Line wrap here too.

> > +	params.acc_param.awb.config.grid = bdsGrid;
> >  	awbGrid_ = bdsGrid;
> > -	params.acc_param.awb.config.grid = awbGrid_;
> >  
> >  	params.use.acc_bnr = 1;
> >  	params.acc_param.bnr = imguCssBnrDefaults;
> >

Hello JM,

Thanks for your work.

On 2021-08-15 20:58:27 +0300, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Fri, Aug 13, 2021 at 09:40:09AM +0100, Kieran Bingham wrote:
> > On 12/08/2021 17:52, Jean-Michel Hautbois wrote:
> > > Implement a new modular framework for algorithms with a common context
> > > structure that is passed to each algorithm through a common API.
> > > 
> > > The initial algorithm is chosen to configure the Bayer Down Scaler grid
> > > which is moved from the IPAIPU3 class.
> > > 
> > > This patch:
> > > - adds a configure() function to the Algorithm interface
> > > - creates a new Grid class implementing the computation at configure
> > >   call
> > > - removes all the local references from IPAIPU3
> > > - implements the list of pointers and the loop at configure call on each
> > >   algorithm (right now, only Grid is in the list)
> > > - removes the imguCssAwbDefaults structure as it is now configured
> > >   properly
> > 
> > A busy patch, but I think in this instance it is reasonable to do all of
> > that in a single patch.
> 
> I'm sure Niklas would have disagreed ;-) I tend to like splitting things
> up too, possibly not as much as he does though.

So this was the burning sensation I had all day :-)

Maybe the addition of the Grid class could be broken out to a separate 
patch? Over all I like the idea of the algorithms helpers.

> 
> > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > > ---
> > >  src/ipa/ipu3/algorithms/algorithm.h | 11 ++++
> > >  src/ipa/ipu3/algorithms/grid.cpp    | 83 +++++++++++++++++++++++++++++
> > >  src/ipa/ipu3/algorithms/grid.h      | 29 ++++++++++
> > >  src/ipa/ipu3/algorithms/meson.build |  1 +
> > >  src/ipa/ipu3/ipa_context.h          |  8 +++
> > >  src/ipa/ipu3/ipu3.cpp               | 75 ++++++--------------------
> > >  src/ipa/ipu3/ipu3_awb.cpp           | 27 ++--------
> > >  7 files changed, 154 insertions(+), 80 deletions(-)
> > >  create mode 100644 src/ipa/ipu3/algorithms/grid.cpp
> > >  create mode 100644 src/ipa/ipu3/algorithms/grid.h
> > > 
> > > diff --git a/src/ipa/ipu3/algorithms/algorithm.h b/src/ipa/ipu3/algorithms/algorithm.h
> > > index 072f01c4..c1b37276 100644
> > > --- a/src/ipa/ipu3/algorithms/algorithm.h
> > > +++ b/src/ipa/ipu3/algorithms/algorithm.h
> > > @@ -7,6 +7,12 @@
> > >  #ifndef __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
> > >  #define __LIBCAMERA_IPA_IPU3_ALGORITHM_H__
> > >  
> > > +#include <iostream>
> > > +
> > > +#include <libcamera/ipa/ipu3_ipa_interface.h>
> > > +
> > > +#include "ipa_context.h"
> > > +
> > >  namespace libcamera {
> > >  
> > >  namespace ipa::ipu3 {
> > > @@ -15,6 +21,11 @@ class Algorithm
> > >  {
> > >  public:
> > >  	virtual ~Algorithm() {}
> > > +
> > > +	virtual int configure([[maybe_unused]] IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo)
> 
> Line wrap.
> 
> > > +	{
> > > +		return 0;
> > > +	}
> > >  };
> > >  
> > >  } /* namespace ipa::ipu3 */
> > > diff --git a/src/ipa/ipu3/algorithms/grid.cpp b/src/ipa/ipu3/algorithms/grid.cpp
> > > new file mode 100644
> > > index 00000000..3578f41b
> > > --- /dev/null
> > > +++ b/src/ipa/ipu3/algorithms/grid.cpp
> > > @@ -0,0 +1,83 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Ideas On Board
> > > + *
> > > + * grid.cpp - IPU3 grid configuration
> > > + */
> > > +
> > > +#include "grid.h"
> > > +
> > > +#include <libcamera/base/log.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa::ipu3::algorithms {
> > > +
> > > +LOG_DEFINE_CATEGORY(IPU3Grid)
> > > +
> > > +/* Maximum number of cells on a row */
> > > +static constexpr uint32_t kMaxCellWidthPerSet = 160;
> > > +/* Maximum number of cells on a column */
> > > +static constexpr uint32_t kMaxCellHeightPerSet = 56;
> > > +
> > > +/**
> > > + * This function calculates a grid for the AWB algorithm in the IPU3 firmware.
> > > + * Its input is the BDS output size calculated in the ImgU.
> > > + * It is limited for now to the simplest method: find the lesser error
> > > + * with the width/height and respective log2 width/height of the cells.
> > > + *
> > > + * \todo The frame is divided into cells which can be 8x8 => 128x128.
> > > + * As a smaller cell improves the algorithm precision, adapting the
> > > + * x_start and y_start parameters of the grid would provoke a loss of
> > > + * some pixels but would also result in more accurate algorithms.
> > > + */
> > > +int Grid::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> 
> I don't think this qualifies as an "algorithm" I'm afraid. I'd keep it
> in the IPU3IPA class.
> 
> > > +{
> > > +	uint32_t minError = std::numeric_limits<uint32_t>::max();
> > > +	Size best;
> > > +	Size bestLog2;
> > > +	ipu3_uapi_grid_config &bdsGrid = context.configuration.grid.bdsGrid;
> > > +
> > > +	context.configuration.grid.bdsOutputSize = configInfo.bdsOutputSize;
> > > +	Size &bdsOutputSize = context.configuration.grid.bdsOutputSize;
> > > +
> > > +	bdsGrid.x_start = 0;
> > > +	bdsGrid.y_start = 0;
> > > +
> > > +	for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
> > > +		uint32_t width = std::min(kMaxCellWidthPerSet,
> > > +					  bdsOutputSize.width >> widthShift);
> > > +		width = width << widthShift;
> > > +		for (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {
> > > +			int32_t height = std::min(kMaxCellHeightPerSet,
> > > +						  bdsOutputSize.height >> heightShift);
> > > +			height = height << heightShift;
> > > +			uint32_t error = std::abs(static_cast<int>(width - bdsOutputSize.width))
> > > +				       + std::abs(static_cast<int>(height - bdsOutputSize.height));
> > > +
> > > +			if (error > minError)
> > > +				continue;
> > > +
> > > +			minError = error;
> > > +			best.width = width;
> > > +			best.height = height;
> > > +			bestLog2.width = widthShift;
> > > +			bestLog2.height = heightShift;
> > > +		}
> > > +	}
> > > +
> > > +	bdsGrid.width = best.width >> bestLog2.width;
> > > +	bdsGrid.block_width_log2 = bestLog2.width;
> > > +	bdsGrid.height = best.height >> bestLog2.height;
> > > +	bdsGrid.block_height_log2 = bestLog2.height;
> > > +
> > > +	LOG(IPU3Grid, Debug) << "Best grid found is: ("
> > > +			     << (int)bdsGrid.width << " << " << (int)bdsGrid.block_width_log2 << ") x ("
> > > +			     << (int)bdsGrid.height << " << " << (int)bdsGrid.block_height_log2 << ")";
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +} /* namespace ipa::ipu3::algorithms */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/ipu3/algorithms/grid.h b/src/ipa/ipu3/algorithms/grid.h
> > > new file mode 100644
> > > index 00000000..b4a51b42
> > > --- /dev/null
> > > +++ b/src/ipa/ipu3/algorithms/grid.h
> > > @@ -0,0 +1,29 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2021, Google
> > > + *
> > > + * grid.h - IPU3 grid configuration
> > > + */
> > > +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_GRID_H__
> > > +#define __LIBCAMERA_IPU3_ALGORITHMS_GRID_H__
> > > +
> > > +#include "algorithm.h"
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa::ipu3::algorithms {
> > > +
> > > +class Grid : public Algorithm
> > > +{
> > > +public:
> > > +	~Grid() = default;
> > > +
> > > +	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> > > +};
> > > +
> > > +} /* namespace ipa::ipu3::algorithms */
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_CONTRAST_H__ */
> > > +
> > > diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> > > index 67148333..3fb3ce56 100644
> > > --- a/src/ipa/ipu3/algorithms/meson.build
> > > +++ b/src/ipa/ipu3/algorithms/meson.build
> > > @@ -1,4 +1,5 @@
> > >  # SPDX-License-Identifier: CC0-1.0
> > >  
> > >  ipu3_ipa_algorithms = files([
> > > +        'grid.cpp',
> > >  ])
> > > diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> > > index 0a197a41..90b2f2c2 100644
> > > --- a/src/ipa/ipu3/ipa_context.h
> > > +++ b/src/ipa/ipu3/ipa_context.h
> > > @@ -11,12 +11,20 @@
> > >  
> > >  #include <linux/intel-ipu3.h>
> > >  
> > > +#include <libcamera/geometry.h>
> > > +
> > >  namespace libcamera {
> > >  
> > >  namespace ipa::ipu3 {
> > >  
> > >  /* Fixed configuration of the IPA */
> > >  struct IPAConfiguration {
> > > +	struct Grid {
> > > +		/* Bayer Down Scaler grid plane config used by the kernel */
> > > +		ipu3_uapi_grid_config bdsGrid;
> > 
> > I always find structures with line comments above better with a blank
> > line in-between, otherwise they 'merge together' in my eyes. But that's
> > just personal preference, so not an issue.
> 
> As this is meant to be a reference implementation, documentation is
> fairly important, and I'd prefer if we used Doxygen formatting. It thus
> brings the question of whether we want to keep it here or move it to a
> .cpp file.
> 
> > > +		/* BDS output size configured by the pipeline handler */
> > > +		Size bdsOutputSize;
> > > +	} grid;
> > >  };
> > >  
> > >  /*
> > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > > index c34fa460..ef7fec86 100644
> > > --- a/src/ipa/ipu3/ipu3.cpp
> > > +++ b/src/ipa/ipu3/ipu3.cpp
> > > @@ -29,13 +29,12 @@
> > >  
> > >  #include "libcamera/internal/mapped_framebuffer.h"
> > >  
> > > +#include "algorithms/algorithm.h"
> > > +#include "algorithms/grid.h"
> > >  #include "ipu3_agc.h"
> > >  #include "ipu3_awb.h"
> > >  #include "libipa/camera_sensor_helper.h"
> > >  
> > > -static constexpr uint32_t kMaxCellWidthPerSet = 160;
> > > -static constexpr uint32_t kMaxCellHeightPerSet = 56;
> > > -
> > >  namespace libcamera {
> > >  
> > >  LOG_DEFINE_CATEGORY(IPAIPU3)
> > > @@ -91,10 +90,12 @@ private:
> > >  	/* Interface to the Camera Helper */
> > >  	std::unique_ptr<CameraSensorHelper> camHelper_;
> > >  
> > > +	/* Maintain the algorithms used by the IPA */
> > > +	std::list<std::unique_ptr<ipa::ipu3::Algorithm>> algorithms_;
> > > +
> > >  	/* Local parameter storage */
> > > +	struct IPAContext context_;
> > >  	struct ipu3_uapi_params params_;
> > > -
> > > -	struct ipu3_uapi_grid_config bdsGrid_;
> > >  };
> > >  
> > >  /**
> > > @@ -164,6 +165,8 @@ int IPAIPU3::init(const IPASettings &settings,
> > >  							       frameDurations[2]);
> > >  
> > >  	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> > 
> > I think a new line here is deserved though, Adding algorithms to the
> > list, is unrelated to initialising the controls - so they should be
> > separated with a blank line.
> > 
> > Two trivial blank lines is the most I can say ... so I think that
> > satisfies a:
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > > +	/* Construct our Algorithms */
> > > +	algorithms_.emplace_back(new algorithms::Grid());
> > >  
> > >  	return 0;
> > >  }
> > > @@ -175,56 +178,6 @@ int IPAIPU3::start()
> > >  	return 0;
> > >  }
> > >  
> > > -/**
> > > - * This function calculates a grid for the AWB algorithm in the IPU3 firmware.
> > > - * Its input is the BDS output size calculated in the ImgU.
> > > - * It is limited for now to the simplest method: find the lesser error
> > > - * with the width/height and respective log2 width/height of the cells.
> > > - *
> > > - * \todo The frame is divided into cells which can be 8x8 => 128x128.
> > > - * As a smaller cell improves the algorithm precision, adapting the
> > > - * x_start and y_start parameters of the grid would provoke a loss of
> > > - * some pixels but would also result in more accurate algorithms.
> > > - */
> > > -void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
> > > -{
> > > -	uint32_t minError = std::numeric_limits<uint32_t>::max();
> > > -	Size best;
> > > -	Size bestLog2;
> > > -	bdsGrid_ = {};
> > > -
> > > -	for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
> > > -		uint32_t width = std::min(kMaxCellWidthPerSet,
> > > -					  bdsOutputSize.width >> widthShift);
> > > -		width = width << widthShift;
> > > -		for (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {
> > > -			int32_t height = std::min(kMaxCellHeightPerSet,
> > > -						  bdsOutputSize.height >> heightShift);
> > > -			height = height << heightShift;
> > > -			uint32_t error  = std::abs(static_cast<int>(width - bdsOutputSize.width))
> > > -							+ std::abs(static_cast<int>(height - bdsOutputSize.height));
> > > -
> > > -			if (error > minError)
> > > -				continue;
> > > -
> > > -			minError = error;
> > > -			best.width = width;
> > > -			best.height = height;
> > > -			bestLog2.width = widthShift;
> > > -			bestLog2.height = heightShift;
> > > -		}
> > > -	}
> > > -
> > > -	bdsGrid_.width = best.width >> bestLog2.width;
> > > -	bdsGrid_.block_width_log2 = bestLog2.width;
> > > -	bdsGrid_.height = best.height >> bestLog2.height;
> > > -	bdsGrid_.block_height_log2 = bestLog2.height;
> > > -
> > > -	LOG(IPAIPU3, Debug) << "Best grid found is: ("
> > > -			    << (int)bdsGrid_.width << " << " << (int)bdsGrid_.block_width_log2 << ") x ("
> > > -			    << (int)bdsGrid_.height << " << " << (int)bdsGrid_.block_height_log2 << ")";
> > > -}
> > > -
> > >  int IPAIPU3::configure(const IPAConfigInfo &configInfo)
> > >  {
> > >  	if (configInfo.entityControls.empty()) {
> > > @@ -264,15 +217,21 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
> > >  
> > >  	defVBlank_ = itVBlank->second.def().get<int32_t>();
> > >  
> > > +	/* Clean context and IPU3 parameters at configuration */
> > >  	params_ = {};
> > > +	context_ = {};
> > >  
> > > -	calculateBdsGrid(configInfo.bdsOutputSize);
> > > +	for (auto const &algo : algorithms_) {
> > > +		int ret = algo->configure(context_, configInfo);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > >  
> > >  	awbAlgo_ = std::make_unique<IPU3Awb>();
> > > -	awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
> > > +	awbAlgo_->initialise(params_, context_.configuration.grid.bdsOutputSize, context_.configuration.grid.bdsGrid);
> > >  
> > >  	agcAlgo_ = std::make_unique<IPU3Agc>();
> > > -	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
> > > +	agcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_);
> > >  
> > >  	return 0;
> > >  }
> > > diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> > > index 4bb321b3..4ee5ee6f 100644
> > > --- a/src/ipa/ipu3/ipu3_awb.cpp
> > > +++ b/src/ipa/ipu3/ipu3_awb.cpp
> > > @@ -107,25 +107,6 @@ static const struct ipu3_uapi_bnr_static_config imguCssBnrDefaults = {
> > >  	.opt_center_sqr = { 419904, 133956 },
> > >  };
> > >  
> > > -/* Default settings for Auto White Balance replicated from the Kernel*/
> > > -static const struct ipu3_uapi_awb_config_s imguCssAwbDefaults = {
> > > -	.rgbs_thr_gr = 8191,
> > > -	.rgbs_thr_r = 8191,
> > > -	.rgbs_thr_gb = 8191,
> > > -	.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT,
> > > -	.grid = {
> > > -		.width = 160,
> > > -		.height = 36,
> > > -		.block_width_log2 = 3,
> > > -		.block_height_log2 = 4,
> > > -		.height_per_slice = 1, /* Overridden by kernel. */
> > > -		.x_start = 0,
> > > -		.y_start = 0,
> > > -		.x_end = 0,
> > > -		.y_end = 0,
> > > -	},
> > > -};
> > > -
> > >  /* Default color correction matrix defined as an identity matrix */
> > >  static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
> > >  	8191, 0, 0, 0,
> > > @@ -174,10 +155,12 @@ IPU3Awb::~IPU3Awb()
> > >  void IPU3Awb::initialise(ipu3_uapi_params &params, const Size &bdsOutputSize, struct ipu3_uapi_grid_config &bdsGrid)
> > >  {
> > >  	params.use.acc_awb = 1;
> > > -	params.acc_param.awb.config = imguCssAwbDefaults;
> > > -
> > > +	params.acc_param.awb.config.rgbs_thr_gr = 8191;
> > > +	params.acc_param.awb.config.rgbs_thr_r = 8191;
> > > +	params.acc_param.awb.config.rgbs_thr_gb = 8191;
> > > +	params.acc_param.awb.config.rgbs_thr_b = 8191 | IPU3_UAPI_AWB_RGBS_THR_B_EN | IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT;
> 
> Line wrap here too.
> 
> > > +	params.acc_param.awb.config.grid = bdsGrid;
> > >  	awbGrid_ = bdsGrid;
> > > -	params.acc_param.awb.config.grid = awbGrid_;
> > >  
> > >  	params.use.acc_bnr = 1;
> > >  	params.acc_param.bnr = imguCssBnrDefaults;
> > > 
> 
> -- 
> Regards,
> 
> Laurent Pinchart