<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 9 Jul 2021 at 15:16, 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 15:08, Naushir Patuck wrote:<br>
> Hi Kieran,<br>
> <br>
> On Fri, 9 Jul 2021 at 14:42, Kieran Bingham<br>
> <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a><br>
> <mailto:<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>>> wrote:<br>
> <br>
> Hi Naush,<br>
> <br>
> On 09/07/2021 10:56, Naushir Patuck wrote:<br>
> > When directly writing controls to the sensor device, ensure that<br>
> VBLANK is<br>
> > written ahead of and before the EXPOSURE control. This is the same<br>
> 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>
> <mailto:<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>
> <mailto:<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>>><br>
> > ---<br>
> > .../pipeline/raspberrypi/raspberrypi.cpp | 37<br>
> ++++++++++++++-----<br>
> > 1 file changed, 27 insertions(+), 10 deletions(-)<br>
> ><br>
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> 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,<br>
> const ControlList *controls)<br>
> > <br>
> > /* Apply any gain/exposure settings that the IPA may have<br>
> passed back. */<br>
> > if (!startConfig.controls.empty())<br>
> > - <br>
> data->unicam_[Unicam::Image].dev()->setControls(&startConfig.controls);<br>
> > + data->setSensorControls(startConfig.controls);<br>
> > <br>
> > /* Configure the number of dropped frames required on<br>
> startup. */<br>
> > data->dropFrameCount_ = startConfig.dropFrameCount;<br>
> > @@ -1294,22 +1295,20 @@ int RPiCameraData::configureIPA(const<br>
> 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<br>
> 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>
> > - <br>
> static_cast<int32_t>(!!(rpiConfig->combinedTransform_ &<br>
> Transform::HFlip)));<br>
> > - ctrls.set(V4L2_CID_VFLIP,<br>
> > - <br>
> static_cast<int32_t>(!!(rpiConfig->combinedTransform_ &<br>
> Transform::VFlip)));<br>
> > - unicam_[Unicam::Image].dev()->setControls(&ctrls);<br>
> > + controls.set(V4L2_CID_HFLIP,<br>
> > + <br>
> static_cast<int32_t>(!!(rpiConfig->combinedTransform_ &<br>
> Transform::HFlip)));<br>
> > + controls.set(V4L2_CID_VFLIP,<br>
> > + <br>
> static_cast<int32_t>(!!(rpiConfig->combinedTransform_ &<br>
> Transform::VFlip)));<br>
> > }<br>
> > <br>
> > + if (!controls.empty())<br>
> > + setSensorControls(controls);<br>
> > +<br>
> > return 0;<br>
> > }<br>
> > <br>
> > @@ -1379,6 +1378,24 @@ void<br>
> 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<br>
> present, the<br>
> > + * former must be written ahead of, and separately from<br>
> EXPOSURE to avoid<br>
> > + * V4L2 rejecting the latter. This is identical to what<br>
> DelayedControls<br>
> > + * does with the priority write flag.<br>
> > + */<br>
> > + if (controls.contains(V4L2_CID_EXPOSURE) &&<br>
> controls.contains(V4L2_CID_VBLANK)) {<br>
> > + ControlList vblank_ctrl;<br>
> > +<br>
> > + vblank_ctrl.set(V4L2_CID_VBLANK,<br>
> 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>
> <br>
> <br>
> This is a bit of a tricky one. Ideally, I want to remove<br>
> V4L2_CID_VBLANK, but we don't have<br>
> a ControlList::Erase() type method. I could create a whole new<br>
> ControlList by iterating over<br>
> controls and adding all but V4L2_CID_VBLANK. This seems a bit inefficient.<br>
> <br>
> Instead, I am relying on the v4l2 framework to not pass on the control<br>
> to the driver if the value<br>
> has not changed from the last set value, which is what happens here. Do<br>
> you think that's a bit<br>
> ugly, and perhaps I should not rely on that?<br>
<br>
Oh I see ...<br>
<br>
Indeed, I don't think we should copy the lists just to erase a value...<br>
<br>
So I suspect we either need to implement an erase function on a control<br>
list control - or ... (perhaps the simpler short term option) put an<br>
explicit comment here stating that it relies on the behaviour you've<br>
described so it's not implicit ...<br></blockquote><div><br></div><div>I'll add the comment and post an updated series shortly!</div><div><br></div><div> <br></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>
> Regards,<br>
> Naush<br>
> <br>
> <br>
> <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>
> <mailto:<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>
> <br>
</blockquote></div></div>