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

[Laurent Pinchart]

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.

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

-- 
Regards,

Laurent Pinchart