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

[Naushir Patuck]

Hi Laurent and Umang,

On Wed, 2 Jun 2021 at 19:10, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> 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.
>

Sorry for jumping in on this topic, but for RPi IPA, it is possible for
VBLANK to
change at runtime when we are running in variable framerate mode.  This
decision is down to the AGC and what exposure it decides to use.

This is also where we run into the problem with V4L2 control limits not
allowing
us to update VBLANK and EXPOSURE at the same time without using
priority writes.

Naush


> > >>
> > >>    IPU3Action op;
> > >>    op.op = ActionMetadataReady;
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210603/900197ba/attachment.htm>