[libcamera-devel,4/4] ipa: ipu3: Calculate frame duration from minimum VBLANK value
diff mbox series

Message ID 20210602102326.106549-5-umang.jain@ideasonboard.com
State Superseded
Headers show
Series
  • IPAIPU3 drive-by improvements
Related show

Commit Message

Umang Jain June 2, 2021, 10:23 a.m. UTC 
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
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@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(-)

Comments

Laurent Pinchart June 7, 2021, 10:55 p.m. UTC | #1 
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@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@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;
Jean-Michel Hautbois June 8, 2021, 5:25 a.m. UTC | #2 
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@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@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@ideasonboard.com>
Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@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;
>
Umang Jain June 8, 2021, 6:09 a.m. UTC | #3 
Hi JM, Laurent,

On 6/8/21 10:55 AM, Jean-Michel Hautbois wrote:
> 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@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@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.
Interesting, on nautilus (imx258) the min and default VBLANK values 
turns out to be the same. I'll change it to use the default one, as per 
your feedback here.
> So:
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Thanks for Testing!
>
>>> +
>>>   	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;

Patch
diff mbox series 

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>();
+
 	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;