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

[David Plowman]

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?

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
>