[libcamera-devel] [PATCH v6 6/8] pipeline: raspberrypi: Use priority write for vblank when writing sensor ctrls

[Naushir Patuck]

Hi Jacopo,

Thank you for your review feedback for all this series.

On Fri, 9 Jul 2021 at 17:23, Jacopo Mondi <jacopo at jmondi.org> wrote:

> On Fri, Jul 09, 2021 at 03:58:23PM +0100, Naushir Patuck wrote:
> > When directly writing controls to the sensor device, ensure that VBLANK
> is
> > written ahead of and before the EXPOSURE control. This is the same
> priority
> > write mechanism used in DelayedControls.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 41 ++++++++++++++-----
> >  1 file changed, 31 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 082eb1ee1c23..e09328ffa0bc 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -154,6 +154,7 @@ public:
> >       void embeddedComplete(uint32_t bufferId);
> >       void setIspControls(const ControlList &controls);
> >       void setDelayedControls(const ControlList &controls);
> > +     void setSensorControls(ControlList &controls);
> >
> >       /* bufferComplete signal handlers. */
> >       void unicamBufferDequeue(FrameBuffer *buffer);
> > @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera, const
> ControlList *controls)
> >
> >       /* Apply any gain/exposure settings that the IPA may have passed
> back. */
> >       if (!startConfig.controls.empty())
> > -
>  data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls);
> > +             data->setSensorControls(startConfig.controls);
> >
> >       /* Configure the number of dropped frames required on startup. */
> >       data->dropFrameCount_ = startConfig.dropFrameCount;
> > @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >               return -EPIPE;
> >       }
> >
> > -     if (!controls.empty())
> > -             unicam_[Unicam::Image].dev()->setControls(&controls);
> > -
> >       /*
> >        * Configure the H/V flip controls based on the combination of
> >        * the sensor and user transform.
> >        */
> >       if (supportsFlips_) {
> > -             ControlList
> ctrls(unicam_[Unicam::Image].dev()->controls());
> > -             ctrls.set(V4L2_CID_HFLIP,
> > -
>  static_cast<int32_t>(!!(rpiConfig->combinedTransform_ &
> Transform::HFlip)));
> > -             ctrls.set(V4L2_CID_VFLIP,
> > -
>  static_cast<int32_t>(!!(rpiConfig->combinedTransform_ &
> Transform::VFlip)));
> > -             unicam_[Unicam::Image].dev()->setControls(&ctrls);
> > +             controls.set(V4L2_CID_HFLIP,
> > +
> static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
> > +             controls.set(V4L2_CID_VFLIP,
> > +
> static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
> >       }
> >
> > +     if (!controls.empty())
> > +             setSensorControls(controls);
> > +
> >       return 0;
> >  }
> >
> > @@ -1379,6 +1378,28 @@ void RPiCameraData::setDelayedControls(const
> ControlList &controls)
> >       handleState();
> >  }
> >
> > +void RPiCameraData::setSensorControls(ControlList &controls)
> > +{
> > +     /*
> > +      * We need to ensure that if both VBLANK and EXPOSURE are present,
> the
> > +      * former must be written ahead of, and separately from EXPOSURE
> to avoid
> > +      * V4L2 rejecting the latter. This is identical to what
> DelayedControls
> > +      * does with the priority write flag.
> > +      *
> > +      * As a consequence of the below logic, VBLANK gets set twice, and
> we
> > +      * rely on the v4l2 framework to not pass the second control set
> to the
> > +      * driver as the actual control value has not changed.
> > +      */
> > +     if (controls.contains(V4L2_CID_EXPOSURE) &&
> controls.contains(V4L2_CID_VBLANK)) {
> > +             ControlList vblank_ctrl;
> > +
> > +             vblank_ctrl.set(V4L2_CID_VBLANK,
> controls.get(V4L2_CID_VBLANK));
> > +             unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);
> > +     }
> > +
> > +     unicam_[Unicam::Image].dev()->setControls(&controls);
> > +}
> > +
>
> Uff, v4l2...
>
> I see another usage of setControls() at match time, but it should not
> involve VBLANK, so it could be kept the way it is
>

Good spot, i'll switch to using RPiCameraData::setSensorControls for that
call
in the next revision.

Regards,
Naush



>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks
>   j
>
> >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >  {
> >       RPi::Stream *stream = nullptr;
> > --
> > 2.25.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210712/e2c0d30d/attachment-0001.htm>