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