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

[Naushir Patuck]

Hi Kieran,

On Fri, 9 Jul 2021 at 14:42, Kieran Bingham <kieran.bingham at ideasonboard.com>
wrote:

> Hi Naush,
>
> On 09/07/2021 10:56, 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>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 37 ++++++++++++++-----
> >  1 file changed, 27 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 082eb1ee1c23..53a30cff1864 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());
>
> Hrm ... ctrls is initialised from the unicam dev, but I can't see where
> controls gets initialised...
>
> Is that an issue?
>
> Hrm - it looks like it might be constructed with entityControls, which
> is both unicam and ISP controls, so I guess it gets there from those.
>
>
>
> > -             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,24 @@ 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.
> > +      */
> > +     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);
>
> Does controls need to have V4L2_CID_VBLANK removed from the control list
> to stop it being set twice? (once above, and once below?
>

This is a bit of a tricky one.  Ideally, I want to remove V4L2_CID_VBLANK,
but we don't have
a ControlList::Erase() type method.  I could create a whole new ControlList
by iterating over
controls and adding all but V4L2_CID_VBLANK.  This seems a bit inefficient.

Instead, I am relying on the v4l2 framework to not pass on the control to
the driver if the value
has not changed from the last set value, which is what happens here.  Do
you think that's a bit
ugly, and perhaps I should not rely on that?

Regards,
Naush


>
> With that done, or if it's not actually needed:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
> > +     }
> > +
> > +     unicam_[Unicam::Image].dev()->setControls(&controls);
> > +}
> > +
> >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >  {
> >       RPi::Stream *stream = nullptr;
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210709/dc4ad215/attachment-0001.htm>