<div dir="ltr"><div dir="ltr">Hi Jacopo,<div><br></div><div>Thank you for your review feedback for all this series.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 9 Jul 2021 at 17:23, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Fri, Jul 09, 2021 at 03:58:23PM +0100, Naushir Patuck wrote:<br>
> When directly writing controls to the sensor device, ensure that VBLANK is<br>
> written ahead of and before the EXPOSURE control. This is the same priority<br>
> write mechanism used in DelayedControls.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> ---<br>
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 41 ++++++++++++++-----<br>
>  1 file changed, 31 insertions(+), 10 deletions(-)<br>
><br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 082eb1ee1c23..e09328ffa0bc 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -154,6 +154,7 @@ public:<br>
>       void embeddedComplete(uint32_t bufferId);<br>
>       void setIspControls(const ControlList &controls);<br>
>       void setDelayedControls(const ControlList &controls);<br>
> +     void setSensorControls(ControlList &controls);<br>
><br>
>       /* bufferComplete signal handlers. */<br>
>       void unicamBufferDequeue(FrameBuffer *buffer);<br>
> @@ -828,7 +829,7 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)<br>
><br>
>       /* Apply any gain/exposure settings that the IPA may have passed back. */<br>
>       if (!startConfig.controls.empty())<br>
> -             data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls);<br>
> +             data->setSensorControls(startConfig.controls);<br>
><br>
>       /* Configure the number of dropped frames required on startup. */<br>
>       data->dropFrameCount_ = startConfig.dropFrameCount;<br>
> @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
>               return -EPIPE;<br>
>       }<br>
><br>
> -     if (!controls.empty())<br>
> -             unicam_[Unicam::Image].dev()->setControls(&controls);<br>
> -<br>
>       /*<br>
>        * Configure the H/V flip controls based on the combination of<br>
>        * the sensor and user transform.<br>
>        */<br>
>       if (supportsFlips_) {<br>
> -             ControlList ctrls(unicam_[Unicam::Image].dev()->controls());<br>
> -             ctrls.set(V4L2_CID_HFLIP,<br>
> -                       static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));<br>
> -             ctrls.set(V4L2_CID_VFLIP,<br>
> -                       static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));<br>
> -             unicam_[Unicam::Image].dev()->setControls(&ctrls);<br>
> +             controls.set(V4L2_CID_HFLIP,<br>
> +                          static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));<br>
> +             controls.set(V4L2_CID_VFLIP,<br>
> +                          static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));<br>
>       }<br>
><br>
> +     if (!controls.empty())<br>
> +             setSensorControls(controls);<br>
> +<br>
>       return 0;<br>
>  }<br>
><br>
> @@ -1379,6 +1378,28 @@ void RPiCameraData::setDelayedControls(const ControlList &controls)<br>
>       handleState();<br>
>  }<br>
><br>
> +void RPiCameraData::setSensorControls(ControlList &controls)<br>
> +{<br>
> +     /*<br>
> +      * We need to ensure that if both VBLANK and EXPOSURE are present, the<br>
> +      * former must be written ahead of, and separately from EXPOSURE to avoid<br>
> +      * V4L2 rejecting the latter. This is identical to what DelayedControls<br>
> +      * does with the priority write flag.<br>
> +      *<br>
> +      * As a consequence of the below logic, VBLANK gets set twice, and we<br>
> +      * rely on the v4l2 framework to not pass the second control set to the<br>
> +      * driver as the actual control value has not changed.<br>
> +      */<br>
> +     if (controls.contains(V4L2_CID_EXPOSURE) && controls.contains(V4L2_CID_VBLANK)) {<br>
> +             ControlList vblank_ctrl;<br>
> +<br>
> +             vblank_ctrl.set(V4L2_CID_VBLANK, controls.get(V4L2_CID_VBLANK));<br>
> +             unicam_[Unicam::Image].dev()->setControls(&vblank_ctrl);<br>
> +     }<br>
> +<br>
> +     unicam_[Unicam::Image].dev()->setControls(&controls);<br>
> +}<br>
> +<br>
<br>
Uff, v4l2...<br>
<br>
I see another usage of setControls() at match time, but it should not<br>
involve VBLANK, so it could be kept the way it is<br></blockquote><div><br></div><div>Good spot, i'll switch to using RPiCameraData::setSensorControls for that call</div><div>in the next revision.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
<br>
Thanks<br>
  j<br>
<br>
>  void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
>  {<br>
>       RPi::Stream *stream = nullptr;<br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>