[libcamera-devel] [PATCH v4 2/5] libcamera: camera_sensor: Add frame length limits to CameraSensorInfo

[Naushir Patuck]

Hi Laurent,

Thank you for your review feedback.

On Thu, 4 Feb 2021 at 20:07, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Fri, Jan 29, 2021 at 11:16:13AM +0000, Naushir Patuck wrote:
> > Sensor frame length is made up of active and inactive (blanking) lines.
> > The minimum and maximum frame length values may be used by pipeline
> > handlers to limit frame durations based on the sensor mode capabilities.
> >
> > Store the minimum and maximum allowable frame length values (in lines)
> > in the CameraSensorInfo structure. These values are computed in
> > CameraSensor::sensorInfo() by querying the sensor subdevice
> > V4L2_CID_VBLANK control limits. This in turn means that V4L2_CID_VBLANK
> > is now a mandatory subdevice control.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  3 ++
> >  src/libcamera/camera_sensor.cpp            | 43 ++++++++++++++++++++--
> >  test/ipa/ipa_wrappers_test.cpp             |  2 +
> >  3 files changed, 44 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h
> b/include/libcamera/internal/camera_sensor.h
> > index fed36bf26e47..5d8c9b1a3121 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -33,6 +33,9 @@ struct CameraSensorInfo {
> >
> >       uint64_t pixelRate;
> >       uint32_t lineLength;
> > +
> > +     uint32_t minFrameLength;
> > +     uint32_t maxFrameLength;
> >  };
> >
> >  class CameraSensor : protected Loggable
> > diff --git a/src/libcamera/camera_sensor.cpp
> b/src/libcamera/camera_sensor.cpp
> > index ab315bdc468c..f60d0cc9c6fa 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -113,6 +113,36 @@ LOG_DEFINE_CATEGORY(CameraSensor)
> >   * The total line length in pixel clock periods, including blanking.
> >   */
> >
> > +/**
> > + * \var CameraSensorInfo::minFrameLength
> > + * \brief The minimum allowable frame length in units of lines
> > + *
> > + * The sensor frame length comprises of active output lines and
> blanking lines
>
> s/comprises of/comprises/ ? If you can let me know which form is
> correct, I can fix this when applying.
>

I would have said "comprises of" sounds more appropriate here.  If it's ok
with you, I'd leave as-is.

Regards,
Naush


> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > + * in a frame. The minimum frame length value dictates the minimum
> allowable
> > + * frame duration of the sensor mode.
> > + *
> > + * To obtain the minimum frame duration:
> > + *
> > + * \verbatim
> > +     frameDuration(s) = minFrameLength(lines) * lineLength(pixels) /
> pixelRate(pixels per second)
> > +   \endverbatim
> > + */
> > +
> > +/**
> > + * \var CameraSensorInfo::maxFrameLength
> > + * \brief The maximum allowable frame length in units of lines
> > + *
> > + * The sensor frame length comprises of active output lines and
> blanking lines
> > + * in a frame. The maximum frame length value dictates the maximum
> allowable
> > + * frame duration of the sensor mode.
> > + *
> > + * To obtain the maximum frame duration:
> > + *
> > + * \verbatim
> > +     frameDuration(s) = maxFrameLength(lines) * lineLength(pixels) /
> pixelRate(pixels per second)
> > +   \endverbatim
> > + */
> > +
> >  /**
> >   * \class CameraSensor
> >   * \brief A camera sensor based on V4L2 subdevices
> > @@ -699,12 +729,13 @@ int CameraSensor::sensorInfo(CameraSensorInfo
> *info) const
> >       info->outputSize = format.size;
> >
> >       /*
> > -      * Retrieve the pixel rate and the line length through V4L2
> controls.
> > -      * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK
> controls is
> > -      * mandatory.
> > +      * Retrieve the pixel rate, line length and minimum/maximum frame
> > +      * duration through V4L2 controls. Support for the
> V4L2_CID_PIXEL_RATE,
> > +      * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.
> >        */
> >       ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
> > -                                                V4L2_CID_HBLANK });
> > +                                                V4L2_CID_HBLANK,
> > +                                                V4L2_CID_VBLANK });
> >       if (ctrls.empty()) {
> >               LOG(CameraSensor, Error)
> >                       << "Failed to retrieve camera info controls";
> > @@ -715,6 +746,10 @@ int CameraSensor::sensorInfo(CameraSensorInfo
> *info) const
> >       info->lineLength = info->outputSize.width + hblank;
> >       info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>();
> >
> > +     const ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK);
> > +     info->minFrameLength = info->outputSize.height +
> vblank.min().get<int32_t>();
> > +     info->maxFrameLength = info->outputSize.height +
> vblank.max().get<int32_t>();
> > +
> >       return 0;
> >  }
> >
> > diff --git a/test/ipa/ipa_wrappers_test.cpp
> b/test/ipa/ipa_wrappers_test.cpp
> > index 47533d105d03..eb6d783e8489 100644
> > --- a/test/ipa/ipa_wrappers_test.cpp
> > +++ b/test/ipa/ipa_wrappers_test.cpp
> > @@ -313,6 +313,8 @@ protected:
> >                       .outputSize = { 2560, 1940 },
> >                       .pixelRate = 96000000,
> >                       .lineLength = 2918,
> > +                     .minFrameLength = 1940,
> > +                     .maxFrameLength = 2880
> >               };
> >               std::map<unsigned int, IPAStream> config{
> >                       { 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } },
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210204/45303747/attachment.htm>