<div dir="ltr"><div dir="ltr">Hi Jacopo,<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, 29 Jan 2021 at 10:08, 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">Hi Naush,<br>
<br>
On Thu, Jan 28, 2021 at 09:10:50AM +0000, Naushir Patuck wrote:<br>
> If an exposure time change adjusts the vblanking limits, and we write<br>
> both VBLANK and EXPOSURE controls as a set, the latter may fail if the<br>
> value is outside of the limits calculated by the old VBLANK value. This<br>
> is a limitation in V4L2 and cannot be fixed by writing VBLANK before<br>
> EXPOSURE.<br>
><br>
> The workaround here is to have the StaggeredCtrl mark the<br>
> VBLANK control as "priority write", which then write VBLANK separately<br>
> from (and ahead of) any other controls. This way, the sensor driver will<br>
> update the EXPOSURE control with new limits before the new values is<br>
> presented, and will thus be seen as valid.<br>
><br>
> In addition to this, the following changes have also been made to<br>
> the module:<br>
><br>
> - The initializer list passed into init() now uses a structure type<br>
> instead of a std::pair.<br>
> - Use unsigned int to store control delays to avoid unnecessary casts.<br>
><br>
> StaggeredCtrl is due to be deprecated and replaced by DelayedCtrl, so<br>
> this change serves more a working proof-of-concept on the workaround,<br>
> and not much care has been taken to provide a nice new API for applying<br>
> this "priority write" flag to the control. A similar workaround must be<br>
> available to DelayedCtrl eventually.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> src/ipa/raspberrypi/raspberrypi.cpp | 5 ++-<br>
> .../pipeline/raspberrypi/raspberrypi.cpp | 12 ++++--<br>
> .../pipeline/raspberrypi/staggered_ctrl.cpp | 41 +++++++++++++------<br>
> .../pipeline/raspberrypi/staggered_ctrl.h | 17 ++++++--<br>
> 4 files changed, 54 insertions(+), 21 deletions(-)<br>
><br>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> index 8c0e699184f6..2ad7b7dabb3e 100644<br>
> --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> @@ -1038,8 +1038,9 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)<br>
><br>
> /*<br>
> * Due to the behavior of V4L2, the current value of VBLANK could clip the<br>
> - * exposure time without us knowing. The next time though this function should<br>
> - * clip exposure correctly.<br>
> + * exposure time without us knowing. We get around this by ensuring the<br>
> + * staggered write component submits VBLANK separately from, and before the<br>
> + * EXPOSURE control.<br>
> */<br>
> ctrls.set(V4L2_CID_VBLANK, vblanking);<br>
> ctrls.set(V4L2_CID_EXPOSURE, exposureLines);<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 524cc960dd37..2118f2e72486 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1229,12 +1229,18 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)<br>
> /*<br>
> * Setup our staggered control writer with the sensor default<br>
> * gain and exposure delays.<br>
> + *<br>
> + * VBLANK must be flagged as "priority write" to allow it to<br>
> + * be set ahead of (and separate from) all other controls that<br>
> + * are batched together. This is needed so that any update to the<br>
> + * EXPOSURE control will be validated based on the new VBLANK<br>
> + * control value.<br>
> */<br>
> if (!staggeredCtrl_) {<br>
> staggeredCtrl_.init(unicam_[Unicam::Image].dev(),<br>
> - { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },<br>
> - { V4L2_CID_EXPOSURE, result.data[resultIdx++] },<br>
> - { V4L2_CID_VBLANK, result.data[resultIdx++] } });<br>
> + { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++], false },<br>
> + { V4L2_CID_EXPOSURE, result.data[resultIdx++], false },<br>
> + { V4L2_CID_VBLANK, result.data[resultIdx++], true } });<br>
> sensorMetadata_ = result.data[resultIdx++];<br>
> }<br>
> }<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp<br>
> index 62605c0fceee..498cd65b4cb6 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp<br>
> @@ -22,21 +22,23 @@ LOG_DEFINE_CATEGORY(RPI_S_W)<br>
> namespace RPi {<br>
><br>
> void StaggeredCtrl::init(V4L2VideoDevice *dev,<br>
> - std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList)<br>
> + std::initializer_list<CtrlInitParams> ctrlList)<br>
> {<br>
> std::lock_guard<std::mutex> lock(lock_);<br>
><br>
> dev_ = dev;<br>
> - delay_ = delayList;<br>
> + ctrlParams_.clear();<br>
> ctrl_.clear();<br>
><br>
> - /* Find the largest delay across all controls. */<br>
> maxDelay_ = 0;<br>
> - for (auto const &p : delay_) {<br>
> + for (auto const &c : ctrlList) {<br>
> LOG(RPI_S_W, Info) << "Init ctrl "<br>
> - << utils::hex(p.first) << " with delay "<br>
> - << static_cast<int>(p.second);<br>
> - maxDelay_ = std::max(maxDelay_, p.second);<br>
> + << utils::hex(<a href="http://c.id" rel="noreferrer" target="_blank">c.id</a>) << " with delay "<br>
> + << static_cast<int>(c.delay);<br>
> +<br>
> + ctrlParams_[<a href="http://c.id" rel="noreferrer" target="_blank">c.id</a>] = { c.delay, c.priorityWrite };<br>
> + /* Find the largest delay across all controls. */<br>
> + maxDelay_ = std::max(maxDelay_, c.delay);<br>
> }<br>
><br>
> init_ = true;<br>
> @@ -67,7 +69,7 @@ bool StaggeredCtrl::set(uint32_t ctrl, int32_t value)<br>
> std::lock_guard<std::mutex> lock(lock_);<br>
><br>
> /* Can we find this ctrl as one that is registered? */<br>
> - if (delay_.find(ctrl) == delay_.end())<br>
> + if (ctrlParams_.find(ctrl) == ctrlParams_.end())<br>
> return false;<br>
><br>
> ctrl_[ctrl][setCount_].value = value;<br>
> @@ -82,7 +84,7 @@ bool StaggeredCtrl::set(std::initializer_list<std::pair<const uint32_t, int32_t><br>
><br>
> for (auto const &p : ctrlList) {<br>
> /* Can we find this ctrl? */<br>
> - if (delay_.find(p.first) == delay_.end())<br>
> + if (ctrlParams_.find(p.first) == ctrlParams_.end())<br>
> return false;<br>
><br>
> ctrl_[p.first][setCount_] = CtrlInfo(p.second);<br>
> @@ -97,7 +99,7 @@ bool StaggeredCtrl::set(const ControlList &controls)<br>
><br>
> for (auto const &p : controls) {<br>
> /* Can we find this ctrl? */<br>
> - if (delay_.find(p.first) == delay_.end())<br>
> + if (ctrlParams_.find(p.first) == ctrlParams_.end())<br>
> return false;<br>
><br>
> ctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>());<br>
> @@ -117,12 +119,25 @@ int StaggeredCtrl::write()<br>
> ControlList controls(dev_->controls());<br>
><br>
> for (auto &p : ctrl_) {<br>
> - int delayDiff = maxDelay_ - delay_[p.first];<br>
> + int delayDiff = maxDelay_ - ctrlParams_[p.first].delay;<br>
> int index = std::max<int>(0, setCount_ - delayDiff);<br>
><br>
> if (p.second[index].updated) {<br>
> - /* We need to write this value out. */<br>
> - controls.set(p.first, p.second[index].value);<br>
> + if (ctrlParams_[p.first].priorityWrite) {<br>
> + /*<br>
> + * This control must be written now, it could<br>
> + * affect validity of the other controls.<br>
> + */<br>
> + ControlList immediate(dev_->controls());<br>
> + immediate.set(p.first, p.second[index].value);<br>
> + dev_->setControls(&immediate);<br>
> + } else {<br>
> + /*<br>
> + * Batch up the list of controls and write them<br>
> + * at the end of the function.<br>
> + */<br>
> + controls.set(p.first, p.second[index].value);<br>
> + }<br>
> p.second[index].updated = false;<br>
<br>
Won't nextFrame() set updated to false ?<br>
Anyway, this was here already and it's really minor. Feel free to<br>
leave it as it is.<br></blockquote><div><br></div><div>Slightly different - the above line is clearing the updated flag for the current control index. In nextFrame(), we are clearing the update fag for the next-control-to-be-written index. I do get your point that you only need one of these statements though :-)</div><div>I'll leave it as it is, on account of this being deprecated soon, and it is slightly unrelated to this patch subject.</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>
The patch looks *unfortunately* reasonable. And I say unfortunately as<br>
you're really working around a limitation of the kernel API,<br></blockquote><div><br></div><div>Indeed. It does fix the problem, but in a bit of an ugly way :-(</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>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
<br>
Thanks<br>
j<br>
<br>
> LOG(RPI_S_W, Debug) << "Writing ctrl "<br>
> << utils::hex(p.first) << " to "<br>
> diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h<br>
> index 382fa31a6853..637629c0d9a8 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h<br>
> +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h<br>
> @@ -23,6 +23,12 @@ namespace RPi {<br>
> class StaggeredCtrl<br>
> {<br>
> public:<br>
> + struct CtrlInitParams {<br>
> + unsigned int id;<br>
> + unsigned int delay;<br>
> + bool priorityWrite;<br>
> + };<br>
> +<br>
> StaggeredCtrl()<br>
> : init_(false), setCount_(0), getCount_(0), maxDelay_(0)<br>
> {<br>
> @@ -34,7 +40,7 @@ public:<br>
> }<br>
><br>
> void init(V4L2VideoDevice *dev,<br>
> - std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList);<br>
> + std::initializer_list<CtrlInitParams> ctrlList);<br>
> void reset();<br>
><br>
> void get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t offset = 0);<br>
> @@ -79,12 +85,17 @@ private:<br>
> }<br>
> };<br>
><br>
> + struct CtrlParams {<br>
> + unsigned int delay;<br>
> + bool priorityWrite;<br>
> + };<br>
> +<br>
> bool init_;<br>
> uint32_t setCount_;<br>
> uint32_t getCount_;<br>
> - uint8_t maxDelay_;<br>
> + unsigned int maxDelay_;<br>
> V4L2VideoDevice *dev_;<br>
> - std::unordered_map<uint32_t, uint8_t> delay_;<br>
> + std::unordered_map<uint32_t, CtrlParams> ctrlParams_;<br>
> std::unordered_map<uint32_t, CircularArray> ctrl_;<br>
> std::mutex lock_;<br>
> };<br>
> --<br>
> 2.25.1<br>
><br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>