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

[Umang Jain]

Hi Laurent,

On 6/2/21 11:40 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> 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.

CC-ing JM, Would like your thoughts on the above discussion.


>
>>>>    
>>>>    	IPU3Action op;
>>>>    	op.op = ActionMetadataReady;