[libcamera-devel] [PATCH 2/2] ipa: ipu3: Bump frame duration value

[Umang Jain]

Hi Laurent,

On 6/7/21 5:35 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> On Thu, Jun 03, 2021 at 08:41:25AM +0530, Umang Jain wrote:
>> On 6/2/21 11:40 PM, Laurent Pinchart wrote:
>>> On Wed, Jun 02, 2021 at 03:57:37PM +0530, Umang Jain wrote:
>>>> On 6/2/21 6:38 AM, Laurent Pinchart wrote:
>>>>> On Tue, Jun 01, 2021 at 04:54:56PM +0530, Umang Jain wrote:
>>>>>> We report a hard-coded frame duration value for CTS. However, when the
>>>>>> CTS is tested on nautilus, which has a 'imx258' sensor, the frame
>>>>>> duration was found to be out-of-range. Since different sensors have
>>>>>> different frame durations range-limits, we need to bump up our value
>>>>>> to match the lower end of the 'imx258' frame duration range-limit
>>>>>> (rounded up to the nearest micro-second).
>>>>>>
>>>>>> This allows the following CTS test to pass on nautilus platform:
>>>>>>     - android.hardware.camera2.cts.CaptureRequestTest#testNoiseReductionModeControl
>>>>>>
>>>>>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>>>>>> ---
>>>>>>     src/ipa/ipu3/ipu3.cpp | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
>>>>>> index 700a5660..e51b0401 100644
>>>>>> --- a/src/ipa/ipu3/ipu3.cpp
>>>>>> +++ b/src/ipa/ipu3/ipu3.cpp
>>>>>> @@ -271,7 +271,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>>>>>>     
>>>>>>     	/* \todo Populate this with real values */
>>>>>>     	ctrls.set(controls::FrameDuration,
>>>>>> -		  static_cast<int64_t>(33334));
>>>>>> +		  static_cast<int64_t>(34483));
>>>>> I think it's time to fix this properly by calculating the actual frame
>>>>> duration. It should hopefully not be more complicated than
>>>>>
>>>>> 	duration = (width + hblank) * (height + vblank) / pixel_rate;
>>>> We need to get vblank, for each frame's exposure I think. We need to
>>>> plumb this into IPAIPU3 and JM seem to have the base helper class
>>>> in-progress. For now, I have slightly made an improvement by using the
>>>> minVBlank_ value instead. It doesn't makes CTS on nautilus pass, but
>>>> anyway is an improvement over hard-coded value. I'll check with Paul, to
>>>> see if it doesn't regress CTS on soraka. Let me know you thoughts!
>>> V4L2_CID_VBLANK is set by userspace. It can be updated by drivers when
>>> the format and/or selection rectangles are modified, but will not
>>> otherwise be changed automatically at runtime. The IPA should compute
>>> the desired vblank value and pass it to the pipeline handler to
>>> configure the sensor. I don't think that's related to the work that JM
>>> is doing.
>> Ok, I will check in with JM for that,
>>
>> Meanwhile, I saw and JM also mentioned that, for getting the vblank - we
>> need 'frameIntegrationDiff' (referencing CamHelper::GetVBlanking from
>> RPi IPA). Looking into it, it's seems to a value tied to a particular
>> sensor, so that's something. Not sure if we are going to put in some
>> specific sensor-related parameters into IPU3 IPA.
> We will, Jean-Michel is working on that.
>
> The frame integration diff value will be used by the IPA to compute the
> desired exposure time and/or vertical blanking, but you won't need to
> care about it to compute the frame duration. Only the vblank value is
> needed for that.


Yes, the vblank value is needed and currently I am using the minimum 
vblank value
queried from sensor controls in the " ipa: ipu3: Calculate frame 
duration from minimum VBLANK value"
patch. It also contains the \todo stating that it can be improved on, by 
using vblank from each frame's exposure.

Can you please take a look at that series too,  "IPAIPU3 drive-by 
improvements"


>
>> CC-ing JM, Would like your thoughts on the above discussion.
>>
>>>>>>     
>>>>>>     	IPU3Action op;
>>>>>>     	op.op = ActionMetadataReady;