<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your review feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 2 Jul 2021 at 01:04, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</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">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Thu, Jul 01, 2021 at 12:34:40PM +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>
> ---<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 37 ++++++++++++++-----<br>
> 1 file changed, 27 insertions(+), 10 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 082eb1ee1c23..53a30cff1864 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>
Strictly speaking, this could have stayed the same, but it doesn't hurt.<br></blockquote><div><br></div><div>I thought it might be better to have a single function doing all setControls calls to the device for</div><div>maintainability.</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>
> <br>
> + if (!controls.empty())<br>
> + setSensorControls(controls);<br>
> +<br>
> return 0;<br>
> }<br>
> <br>
> @@ -1379,6 +1378,24 @@ 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>
> + 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>Thank you for your<br>
> + unicam_[Unicam::Image].dev()->setControls(&controls);<br>
<br>
VBLANK will be set twice, could it be an issue ? The value will be the<br>
same each time, but that can generate additional I2C writes, which isn't<br>
great.<br></blockquote><div><br></div><div>Yes, I did have to stop and think about what to do here. Ideally, I would want to</div><div>remove VBLANK from controls, and call setControls(&controls) at the end.</div><div>However, no mechanism exists to remove elements from the ControlList that</div><div>I know, is that correct?</div><div><br></div><div>The alternative would be to construct a new ControlList that has all the elements</div><div>from controls, except VBLANK and use that in the last call to setControls. That</div><div>seems quite inefficient when run per frame.</div><div><br></div><div>The proposed way above does set VBLANK twice, but I was relying on the fact</div><div>that the V4L2 framework would realise that the control value is same as the</div><div>current value, and would not pass the control down to the driver, and avoid the</div><div>subsequent second I2C write. At least, that is what I thought would happen,</div><div>but admittedly I have not actually checked to confirm.</div><div><br></div><div>Regards,</div><div>Naush</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>
> +}<br>
> +<br>
> void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
> {<br>
> RPi::Stream *stream = nullptr;<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>