[libcamera-devel] [RFC PATCH 1/5] ipa: ipu3: Introduce a Context structure

[Kieran Bingham]

Hi JM,

On 09/08/2021 10:20, Jean-Michel Hautbois wrote:
> Passing parameters, statistics and all specific data each algorithm shares.

We're introducing something new, so we should describe the need first.

"""
An increasing amount of data and information needs to be shared between
the components that build up to implement image processing algorithms.

Create a context structure which will allow us to work towards calling
algorithms in a modular way, and sharing information between the modules.
"""


> As the ipu3_uapi_params are part of the IPA context referenced by the
> algorithms, move the structure into the IPAContext directly and adapt existing
> algorithm implementations.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipa_context.h | 30 ++++++++++++++++++++++++++++++
>  src/ipa/ipu3/ipu3.cpp      | 16 +++++++++-------
>  2 files changed, 39 insertions(+), 7 deletions(-)
>  create mode 100644 src/ipa/ipu3/ipa_context.h
> 
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> new file mode 100644
> index 00000000..5d717be5
> --- /dev/null
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * ipu3_ipa_context.h - IPU3 IPA Context
> + *
> + * Context information shared between the algorithms
> + */
> +#ifndef __LIBCAMERA_IPU3_IPA_CONTEXT_H__
> +#define __LIBCAMERA_IPU3_IPA_CONTEXT_H__
> +
> +#include <linux/intel-ipu3.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::ipu3 {
> +
> +struct IPAContext {
> +	/* Input statistics from the previous frame */
> +	const ipu3_uapi_stats_3a *stats;

It looks like we don't use this yet in this patch.

Given that we also question in patch 2/5 if this should be used in here,
I think it should be introduced there too.


Other than that,

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

> +
> +	/* Output Parameters which will be written to the hardware */
> +	ipu3_uapi_params params;
> +};
> +
> +} /* namespace ipa::ipu3 */
> +
> +} /* namespace libcamera*/
> +
> +#endif /* __LIBCAMERA_IPU3_IPA_CONTEXT_H__ */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 71698d36..a714af85 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -22,6 +22,8 @@
>  
>  #include "libcamera/internal/framebuffer.h"
>  
> +#include "ipa_context.h"
> +
>  #include "ipu3_agc.h"
>  #include "ipu3_awb.h"
>  #include "libipa/camera_sensor_helper.h"
> @@ -81,9 +83,8 @@ private:
>  	std::unique_ptr<CameraSensorHelper> camHelper_;
>  
>  	/* Local parameter storage */
> -	struct ipu3_uapi_params params_;
> -
>  	struct ipu3_uapi_grid_config bdsGrid_;
> +	struct IPAContext context_;
>  };
>  
>  int IPAIPU3::init(const IPASettings &settings)
> @@ -94,6 +95,9 @@ int IPAIPU3::init(const IPASettings &settings)
>  		return -ENODEV;
>  	}
>  
> +	/* Reset all the hardware settings */
> +	context_.params = {};
> +
>  	return 0;
>  }
>  
> @@ -193,12 +197,10 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  
>  	defVBlank_ = itVBlank->second.def().get<int32_t>();
>  
> -	params_ = {};
> -
>  	calculateBdsGrid(configInfo.bdsOutputSize);
>  
>  	awbAlgo_ = std::make_unique<IPU3Awb>();
> -	awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
> +	awbAlgo_->initialise(context_.params, configInfo.bdsOutputSize, bdsGrid_);
>  
>  	agcAlgo_ = std::make_unique<IPU3Agc>();
>  	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
> @@ -276,9 +278,9 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>  void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
>  {
>  	if (agcAlgo_->updateControls())
> -		awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
> +		awbAlgo_->updateWbParameters(context_.params, agcAlgo_->gamma());
>  
> -	*params = params_;
> +	*params = context_.params;
>  
>  	IPU3Action op;
>  	op.op = ActionParamFilled;
>