[libcamera-devel] [PATCH v2 2/4] pipeline: raspberrypi: Use priority write for vblank when writing sensor ctrls

[Naushir Patuck]

Hi Laurent,

Thank you for your review feedback.

On Fri, 2 Jul 2021 at 01:04, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Thu, Jul 01, 2021 at 12:34:40PM +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>
> > ---
> >  .../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());
> > -             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)));
> >       }
>
> Strictly speaking, this could have stayed the same, but it doesn't hurt.
>

I thought it might be better to have a single function doing all
setControls calls to the device for
maintainability.


>
> >
> > +     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);
> > +     }
> > +
> Thank you for your
> > +     unicam_[Unicam::Image].dev()->setControls(&controls);
>
> VBLANK will be set twice, could it be an issue ? The value will be the
> same each time, but that can generate additional I2C writes, which isn't
> great.
>

Yes, I did have to stop and think about what to do here.  Ideally, I would
want to
remove VBLANK from controls, and call setControls(&controls) at the end.
However, no mechanism exists to remove elements from the ControlList that
I know, is that correct?

The alternative would be to construct a new ControlList that has all the
elements
from controls, except VBLANK and use that in the last call to setControls.
That
seems quite inefficient when run per frame.

The proposed way above does set VBLANK twice, but I was relying on the fact
that the V4L2 framework would realise that the control value is same as the
current value, and would not pass the control down to the driver, and avoid
the
subsequent second I2C write.  At least, that is what I thought would happen,
but admittedly I have not actually checked to confirm.

Regards,
Naush


>
> > +}
> > +
> >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >  {
> >       RPi::Stream *stream = nullptr;
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210702/03fa6dd1/attachment-0001.htm>