[libcamera-devel,v3,4/9] ipa: ipu3: Introduce modular algorithm

13391 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.

This patch:
- removes all the local references from IPAIPU3 and uses IPAContext
- implements the list of pointers and the loop at configure call on each
  algorithm
- loops in fillParams on each prepare() call on the algorithm list
- loops in prepareStats on each process() call on the algorithm list

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/ipa_context.h | 27 +++--------
 src/ipa/ipu3/ipu3.cpp      | 94 +++++++++++++++++++++++++++++++++-----
 2 files changed, 89 insertions(+), 32 deletions(-)

Hi Jean-Michel,

Thank you for the patch.

On Wed, Aug 18, 2021 at 05:53:58PM +0200, 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.
> 
> This patch:
> - removes all the local references from IPAIPU3 and uses IPAContext
> - implements the list of pointers and the loop at configure call on each
>   algorithm
> - loops in fillParams on each prepare() call on the algorithm list
> - loops in prepareStats on each process() call on the algorithm list
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipa_context.h | 27 +++--------
>  src/ipa/ipu3/ipu3.cpp      | 94 +++++++++++++++++++++++++++++++++-----
>  2 files changed, 89 insertions(+), 32 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index 76534f7f..46291d9e 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -10,37 +10,22 @@
>  
>  #include <linux/intel-ipu3.h>
>  
> +#include <libcamera/geometry.h>
> +
>  namespace libcamera {
>  
>  namespace ipa::ipu3 {
>  
> -/**
> - * Fixed configuration of the IPA
> - *
> - * This structure contains data set at the configure state of the IPA.
> - * It can't be modified while streaming.
> - */

Could you please move the documentation to ipu3.cpp in 2/9 already ? It
will make review easier.

>  struct IPAConfiguration {
> +	struct Grid {
> +		ipu3_uapi_grid_config bdsGrid;
> +		Size bdsOutputSize;
> +	} grid;
>  };
>  
> -/**
> - * Context of a frame for each algorithms
> - *
> - * This may be stored in a way that is associated with a given request
> - * lifetime, though for now a single instance is used.
> - * It contains informations updated while streaming by the algorithms.
> - */
>  struct IPAFrameContext {
>  };
>  
> -/**
> - * Context information shared between the algorithms
> - *
> - * The global context structure contains:
> - * - a configuration structure set while the IPA is configured, and not
> - * modified when streaming
> - * - a frameContext structure updated by the algorithms while streaming
> - */
>  struct IPAContext {
>  	IPAConfiguration configuration;
>  	IPAFrameContext frameContext;
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index c34fa460..dc6f0735 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -29,6 +29,7 @@
>  
>  #include "libcamera/internal/mapped_framebuffer.h"
>  
> +#include "algorithms/algorithm.h"
>  #include "ipu3_agc.h"
>  #include "ipu3_awb.h"
>  #include "libipa/camera_sensor_helper.h"
> @@ -36,6 +37,56 @@
>  static constexpr uint32_t kMaxCellWidthPerSet = 160;
>  static constexpr uint32_t kMaxCellHeightPerSet = 56;
>  
> +/**
> + * \file ipa_context.h
> + * \brief Context information shared between the algorithms
> + */
> +
> +/**
> + * \struct IPAConfiguration
> + * \brief Fixed configuration of the IPA
> + *
> + * This structure contains data set at the configure state of the IPA.
> + * It can't be modified while streaming.
> + */
> +
> +/**
> + * \struct IPAConfiguration::Grid

This should instead document "\var IPAConfiguration::grid". We never use
the IPAConfiguration::Grid structure, but we use the
IPAConfiguration::grid field. That's why I recommended dropping the
structure name, to avoid double documentation.

> + * \brief Grid configuration of the IPA
> + *
> + * This structure contains all the parameters needed to use the statistics.

Doesn't seem to match the structure.

> + *

Extra blank line.

> + */
> +
> +/**
> + * \var IPAConfiguration::Grid::bdsGrid
> + * \brief Bayer Down Scaler grid plane config used by the kernel
> + */
> +
> +/**
> + * \var IPAConfiguration::Grid::bdsOutputSize
> + * \brief BDS output size configured by the pipeline handler
> + */
> +
> +/**
> + * \struct IPAFrameContext
> + * \brief Context of a frame for each algorithms
> + *
> + * This may be stored in a way that is associated with a given request
> + * lifetime, though for now a single instance is used.
> + * It contains informations updated while streaming by the algorithms.
> + */
> +
> +/**
> + * \struct IPAContext
> + * \brief Context information shared between the algorithms
> + *
> + * The global context structure contains:
> + * - a configuration structure set while the IPA is configured, and not
> + * modified when streaming
> + * - a frameContext structure updated by the algorithms while streaming
> + */
> +
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPAIPU3)
> @@ -91,10 +142,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_;
>  };
>  
>  /**
> @@ -191,7 +244,13 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
>  	uint32_t minError = std::numeric_limits<uint32_t>::max();
>  	Size best;
>  	Size bestLog2;
> -	bdsGrid_ = {};
> +	ipu3_uapi_grid_config &bdsGrid = context_.configuration.grid.bdsGrid;
> +
> +	/* Set the BDS output size in the IPAConfiguration structure */
> +	context_.configuration.grid.bdsOutputSize = bdsOutputSize;
> +
> +	bdsGrid.x_start = 0;
> +	bdsGrid.y_start = 0;

I'd reorder the code to avoid mixing the bdsGrid and bdsOutputSize
configuration. Those two lines, as well as the local bdsGrid variable,
would then move below a the beginning of the next hunk.

>  
>  	for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
>  		uint32_t width = std::min(kMaxCellWidthPerSet,
> @@ -215,14 +274,14 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
>  		}
>  	}
>  
> -	bdsGrid_.width = best.width >> bestLog2.width;
> -	bdsGrid_.block_width_log2 = bestLog2.width;
> -	bdsGrid_.height = best.height >> bestLog2.height;
> -	bdsGrid_.block_height_log2 = bestLog2.height;
> +	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)bdsGrid.width << " << " << (int)bdsGrid.block_width_log2 << ") x ("
> +			    << (int)bdsGrid.height << " << " << (int)bdsGrid.block_height_log2 << ")";
>  }
>  
>  int IPAIPU3::configure(const IPAConfigInfo &configInfo)
> @@ -264,15 +323,22 @@ 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);

Blank line.

> +	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);

Line wrap.

>  
>  	agcAlgo_ = std::make_unique<IPU3Agc>();
> -	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
> +	agcAlgo_->initialise(context_.configuration.grid.bdsGrid, sensorInfo_);
>  
>  	return 0;
>  }
> @@ -346,6 +412,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>  
>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  {
> +	for (auto const &algo : algorithms_)
> +		algo->prepare(context_, params_);
> +
>  	if (agcAlgo_->updateControls())
>  		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
>  
> @@ -363,6 +432,9 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  {
>  	ControlList ctrls(controls::controls);
>  
> +	for (auto const &algo : algorithms_)
> +		algo->process(context_, stats);
> +
>  	double gain = camHelper_->gain(gain_);
>  	agcAlgo_->process(stats, exposure_, gain);
>  	gain_ = camHelper_->gainCode(gain);