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

[Laurent Pinchart]

Hi Umang,

On Mon, Jun 07, 2021 at 06:59:57PM +0530, Umang Jain wrote:
> On 6/7/21 5:35 AM, Laurent Pinchart wrote:
> > 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.

Any chance we could address this \todo now already ? I fear it may be a
cause for regressions.

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

Next on my list :-)

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

-- 
Regards,

Laurent Pinchart