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

[Jean-Michel Hautbois]

Hi Laurent,

On 08/06/2021 00:55, Laurent Pinchart wrote:
> 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>

Sure !
I don't have any regression, and the durations calculated are:
- 32726 for min
- 33331 for def

The def one is more accurate, which is expected as this is set by the
driver based on the current timings.
I did not expect regressions with the current IPA AGC because VBLANK is
not set at all.
So:
Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois 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;
>