[libcamera-devel] [PATCH 3/4] ipa: ipu3: Copy IPACameraSensorInfo for future usage

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Wed, Jun 02, 2021 at 03:53:25PM +0530, Umang Jain wrote:
> IPACameraSensorInfo members will be needed at various places in the
> IPAIPU3 class, in subsequent commits. Hence, it seems trivial to copy
> this structure for wider availability throughout the class.
> 
> This commit does not introduce any functional changes.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 2496b0a0..97ddb863 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -63,6 +63,8 @@ private:
>  
>  	ControlInfoMap ctrls_;
>  
> +	IPACameraSensorInfo sensorInfo_;

This is OK for now, but in the not too distant future I think we should
store fields individually as we should probably convert them. For
instance, the line duration should be converted to a time unit when we
receive it, and then used as such, instead of converting between pixels
and time in multiple places.

Jean-Michel, how about moving IPU3Agc to handling exposure time in time
units, and doing the conversion in ipu3.cpp ? Is this something you have
considered, or maybe you're already planning it ?

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

> +
>  	/* Camera sensor controls. */
>  	uint32_t exposure_;
>  	uint32_t minExposure_;
> @@ -144,6 +146,8 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  	if (configInfo.entityControls.empty())
>  		return;
>  
> +	sensorInfo_ = configInfo.sensorInfo;
> +
>  	ctrls_ = configInfo.entityControls.at(0);
>  
>  	const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE);
> @@ -174,7 +178,7 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  	awbAlgo_->initialise(params_, configInfo.bdsOutputSize, bdsGrid_);
>  
>  	agcAlgo_ = std::make_unique<IPU3Agc>();
> -	agcAlgo_->initialise(bdsGrid_, configInfo.sensorInfo);
> +	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
>  }
>  
>  void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)

-- 
Regards,

Laurent Pinchart