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

[Umang Jain]

Hi JM,

On 8/9/21 2:50 PM, Jean-Michel Hautbois wrote:
> Passing parameters, statistics and all specific data each algorithm shares.
I would re-phrase this sentence to sound more complete.
> 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;

Is there used somewhere in the series? If yes, I suggest moving this to 
the patch where this is used, as I can't see in this patch.

With that addressed,

Reviewed-by: Umang Jain <umang.jain 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;