<div dir="ltr"><div dir="ltr">Hi Kieran,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 9 Jul 2021 at 14:42, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@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>
On 09/07/2021 10:56, 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>
<br>
Hrm ... ctrls is initialised from the unicam dev, but I can't see where<br>
controls gets initialised...<br>
<br>
Is that an issue?<br>
<br>
Hrm - it looks like it might be constructed with entityControls, which<br>
is both unicam and ISP controls, so I guess it gets there from those.<br>
<br>
<br>
<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,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>
Does controls need to have V4L2_CID_VBLANK removed from the control list<br>
to stop it being set twice? (once above, and once below?<br></blockquote><div><br></div><div>This is a bit of a tricky one. Ideally, I want to remove V4L2_CID_VBLANK, but we don't have</div><div>a ControlList::Erase() type method. I could create a whole new ControlList by iterating over</div><div>controls and adding all but V4L2_CID_VBLANK. This seems a bit inefficient.</div><div><br></div><div>Instead, I am relying on the v4l2 framework to not pass on the control to the driver if the value</div><div>has not changed from the last set value, which is what happens here. Do you think that's a bit</div><div>ugly, and perhaps I should not rely on that?</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>
With that done, or if it's not actually needed:<br>
<br>
Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
<br>
<br>
> + }<br>
> +<br>
> + unicam_[Unicam::Image].dev()->setControls(&controls);<br>
> +}<br>
> +<br>
> void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)<br>
> {<br>
> RPi::Stream *stream = nullptr;<br>
> <br>
</blockquote></div></div>