<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>