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

12588 diff mbox series

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
write mechanism used in DelayedControls.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 37 ++++++++++++++-----
 1 file changed, 27 insertions(+), 10 deletions(-)

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

With the minor typos fixed:

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

> +}
> +
>  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
>  {
>         RPi::Stream *stream = nullptr;
> --
> 2.25.1
>

Hi David,

Thank you for your feedback.

On Tue, 15 Jun 2021 at 14:33, David Plowman <david.plowman@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@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@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@raspberrypi.com>
>
> Thanks!
> David
>
> > +}
> > +
> >  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)
> >  {
> >         RPi::Stream *stream = nullptr;
> > --
> > 2.25.1
> >
>