<div dir="ltr"><div dir="ltr">Hi David,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 15 Jun 2021 at 14:33, David Plowman <<a href="mailto:david.plowman@raspberrypi.com">david.plowman@raspberrypi.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>
Thanks for this patch.<br>
<br>
Coincidentally, one for my forthcoming patches (related to<br>
V4L2_CID_NOTIFY_GAIN_RED/BLUE) added a setSensorControls method too,<br>
so this will in fact be helpful.<br>
<br>
On Mon, 14 Jun 2021 at 11:00, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>> wrote:<br>
><br>
> When directly writing controls to the sensor device, ensure that VBLANK is<br>
> written ahead of and before the EXPSOURE control. This is the same priority<br>
<br>
s/EXPSOURE/EXPOSURE/<br>
<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>
> ---<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 887f8d0f7404..d180fc059613 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -153,6 +153,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>
> @@ -827,7 +828,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>
> @@ -1293,22 +1294,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>
> @@ -1378,6 +1377,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 EXPSURE are present, the<br>
<br>
s/EXPSURE/EXPOSURE/<br>
<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>
> + unicam_[Unicam::Image].dev()->setControls(&controls);<br>
<br>
This might be setting the VBLANK for a second time, but presumably<br>
it's easier to let that happen than to do anything about it?<br></blockquote><div><br></div><div>Yes, it will do the set twice. However, v4l2 will stop it from getting to the sensor</div><div>as the value will be identical. I though it easier to do that than to construct another</div><div>ControlList and not include VBLANK from that new list. Perhaps we could consider</div><div>a ControlList::remove() method for cases like this...?</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>
With the minor typos fixed:<br>
<br>
Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
<br>
Thanks!<br>
David<br>
<br>
> +}<br>
> +<br>
> void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
> {<br>
> RPi::Stream *stream = nullptr;<br>
> --<br>
> 2.25.1<br>
><br>
</blockquote></div></div>