[libcamera-devel] [PATCH 4/4] ipa: ipu3: Calculate frame duration from minimum VBLANK value

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Wed, Jun 02, 2021 at 03:53:26PM +0530, Umang Jain wrote:
> Frame duration is hard-coded for CTS as per [1]. Ideally, to accurately
> calculate the frame duration, it needs the VBLANK value from every
> frame's exposure. However, this particular bit is yet to be implemented
> in IPAIPU3.
> 
> Meanwhile, we can atleast head in the right direction by not hard
> coding the value, instead using the minimum VBLANK value as reported

s/reported/reported by/

> the sensor. Update the existing \todo, to use the derived VBLANK value
> as and when it's available from each frame exposure.
> 
> [1] 6c5f3fe6ced7 ("ipa: ipu3: Set output frame duration metadata")
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
> 
> For reference, on `nautilus`:
> - minVBlank_ was reported as '104'
> - Calculating frame-duration using minVBlank_ came out to be: 34041
> 
> ---
>  src/ipa/ipu3/ipu3.cpp | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 97ddb863..db4ec684 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -72,6 +72,7 @@ private:
>  	uint32_t gain_;
>  	uint32_t minGain_;
>  	uint32_t maxGain_;
> +	uint32_t minVBlank_;
>  
>  	/* Interface to the AWB algorithm */
>  	std::unique_ptr<IPU3Awb> awbAlgo_;
> @@ -162,6 +163,12 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  		return;
>  	}
>  
> +	const auto itVBlank = ctrls_.find(V4L2_CID_VBLANK);
> +	if (itVBlank == ctrls_.end()) {
> +		LOG(IPAIPU3, Error) << "Can't find VBLANK control";
> +		return;
> +	}
> +
>  	minExposure_ = std::max(itExp->second.min().get<int32_t>(), 1);
>  	maxExposure_ = itExp->second.max().get<int32_t>();
>  	exposure_ = minExposure_;
> @@ -170,6 +177,8 @@ void IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  	maxGain_ = itGain->second.max().get<int32_t>();
>  	gain_ = minGain_;
>  
> +	minVBlank_ = itVBlank->second.min().get<int32_t>();

We should address vblank setting sooner than later, I'm a bit worried
bit possible regressions introduced by this patch. At the very least,
could we use the default vblank value instead of the minimum ? That
would have a better chance of matching what the sensor driver uses.

Jean-Michel, could you test this series ton SGo2 to verify there's no
regression ? Ideally, could you try both the min and def options ?
Assuming no regression,

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


> +
>  	params_ = {};
>  
>  	calculateBdsGrid(configInfo.bdsOutputSize);
> @@ -273,9 +282,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  	if (agcAlgo_->updateControls())
>  		setControls(frame);
>  
> -	/* \todo Populate this with real values */
> -	ctrls.set(controls::FrameDuration,
> -		  static_cast<int64_t>(33334));
> +	/* \todo Use VBlank value calculated from each frame exposure. */
> +	int64_t frameDuration = sensorInfo_.lineLength * (minVBlank_ + sensorInfo_.outputSize.height) /
> +				(sensorInfo_.pixelRate / 1e6);
> +	ctrls.set(controls::FrameDuration, frameDuration);
>  
>  	IPU3Action op;
>  	op.op = ActionMetadataReady;

-- 
Regards,

Laurent Pinchart