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

[Naushir Patuck]

Hi David,

Thank you for your feedback.

On Tue, 15 Jun 2021 at 14:33, David Plowman <david.plowman at raspberrypi.com>
wrote:

> Hi Naush
>
> Thanks for this patch.
>
> Coincidentally, one for my forthcoming patches (related to
> V4L2_CID_NOTIFY_GAIN_RED/BLUE) added a setSensorControls method too,
> so this will in fact be helpful.
>
> On Mon, 14 Jun 2021 at 11:00, Naushir Patuck <naush at raspberrypi.com>
> wrote:
> >
> > When directly writing controls to the sensor device, ensure that VBLANK
> is
> > written ahead of and before the EXPSOURE control. This is the same
> priority
>
> s/EXPSOURE/EXPOSURE/
>
> > write mechanism used in DelayedControls.
> >
> > Signed-off-by: Naushir Patuck <naush 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 887f8d0f7404..d180fc059613 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -153,6 +153,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);
> > @@ -827,7 +828,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;
> > @@ -1293,22 +1294,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;
> >  }
> >
> > @@ -1378,6 +1377,24 @@ void RPiCameraData::setDelayedControls(const
> ControlList &controls)
> >         handleState();
> >  }
> >
> > +void RPiCameraData::setSensorControls(ControlList &controls)
> > +{
> > +       /*
> > +        * We need to ensure that if both VBLANK and EXPSURE are
> present, the
>
> s/EXPSURE/EXPOSURE/
>
> > +        * 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);
> > +       }
> > +
> > +       unicam_[Unicam::Image].dev()->setControls(&controls);
>
> This might be setting the VBLANK for a second time, but presumably
> it's easier to let that happen than to do anything about it?
>

Yes, it will do the set twice.  However, v4l2 will stop it from getting to
the sensor
as the value will be identical.  I though it easier to do that than to
construct another
ControlList and not include VBLANK from that new list.  Perhaps we could
consider
a ControlList::remove() method for cases like this...?

Regards,
Naush



>
> With the minor typos fixed:
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks!
> David
>
> > +}
> > +
> >  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/20210615/ab725b2f/attachment.htm>