[libcamera-devel] [PATCH v1 7/8] ipa: rkisp1: agc: Introduce prepare call

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Fri, Nov 19, 2021 at 12:16:53PM +0100, Jean-Michel Hautbois wrote:
> When a new parameter buffer needs to be queued, we need to specify which
> algorithm is activated or not in the ISP. Add a simple prepare function
> in AGC for that, which may later evolve to take the exposure locking
> into account. For that function to be called, we also need to add the
> loop on the algorithms in IPARkISP1::queueRequest.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp |  7 +++++++
>  src/ipa/rkisp1/algorithms/agc.h   |  1 +
>  src/ipa/rkisp1/rkisp1.cpp         | 12 +++---------
>  3 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 3d4a17ff..2b7c6fda 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -244,6 +244,13 @@ void Agc::process(IPAContext &context, const rkisp1_stat_buffer *stats)
>  	frameCount_++;
>  }
>  
> +void Agc::prepare([[maybe_unused]] IPAContext &context,
> +		  rkisp1_params_cfg *params)
> +{
> +	params->module_ens |= RKISP1_CIF_ISP_MODULE_AEC;
> +	params->module_en_update |= RKISP1_CIF_ISP_MODULE_AEC;

There would be a tiny performance improvement if you set the
RKISP1_CIF_ISP_MODULE_AEC flag in module_en_update only when the AEC
configuration changed. Currently, that would be on the first frame only,
as we don't support disabling AE anymore after this patch. It's so tiny
that it doesn't matter much at the moment, but once we implement support
for manual exposure, it would be nice to improve this.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +}
> +
>  } /* namespace ipa::rkisp1::algorithms */
>  
>  } /* namespace libcamera */
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> index fbb8ea98..934a455e 100644
> --- a/src/ipa/rkisp1/algorithms/agc.h
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -28,6 +28,7 @@ public:
>  	~Agc() = default;
>  
>  	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> +	void prepare(IPAContext &context, rkisp1_params_cfg *params) override;
>  	void process(IPAContext &context, const rkisp1_stat_buffer *stats) override;
>  
>  private:
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 1783f91f..b1bfb8b6 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -271,19 +271,13 @@ void IPARkISP1::processEvent(const RkISP1Event &event)
>  }
>  
>  void IPARkISP1::queueRequest(unsigned int frame, rkisp1_params_cfg *params,
> -			     const ControlList &controls)
> +			     [[maybe_unused]] const ControlList &controls)
>  {
>  	/* Prepare parameters buffer. */
>  	memset(params, 0, sizeof(*params));
>  
> -	/* Auto Exposure on/off. */
> -	if (controls.contains(controls::AeEnable)) {
> -		autoExposure_ = controls.get(controls::AeEnable);
> -		if (autoExposure_)
> -			params->module_ens = RKISP1_CIF_ISP_MODULE_AEC;
> -
> -		params->module_en_update = RKISP1_CIF_ISP_MODULE_AEC;
> -	}
> +	for (auto const &algo : algorithms_)
> +		algo->prepare(context_, params);
>  
>  	RkISP1Action op;
>  	op.op = ActionParamFilled;

-- 
Regards,

Laurent Pinchart