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

[Laurent Pinchart]

Hi Naush,

On Thu, Feb 04, 2021 at 10:15:58PM +0000, Naushir Patuck wrote:
> On Thu, 4 Feb 2021 at 20:07, Laurent Pinchart wrote:
> > 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.

I was looking at https://en.wiktionary.org/wiki/comprise#Usage_notes
when trying to figure out why it sounded weird to me, but I'll trust you
more than a webpage and will keep it as-is :-)

> > 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