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