[libcamera-devel] [PATCH v2 4/4] pipeline: raspberrypi: Add notion of immediate write to StaggeredCtrl
Naushir Patuck
naush at raspberrypi.com
Wed Jan 27 10:17:43 CET 2021
Hi Laurent,
Thank you for your review feedback. Sending again, as I forgot to
reply-all.
On Wed, 27 Jan 2021 at 00:30, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush,
>
> Thank you for the patch.
>
> On Sun, Jan 24, 2021 at 02:05:06PM +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 "immediate 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.
> >
> > 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 immediate 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 | 11 ++++--
> > .../pipeline/raspberrypi/staggered_ctrl.cpp | 39 ++++++++++++++-----
> > .../pipeline/raspberrypi/staggered_ctrl.h | 3 +-
> > 4 files changed, 43 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 75c9e404dcc1..fefca32ce187 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -1040,8 +1040,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..1485999ad2a0 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1229,12 +1229,17 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> > /*
> > * Setup our staggered control writer with the sensor
> default
> > * gain and exposure delays.
> > + *
> > + * VBLANK must be flagged as "immediate write" to allow it
> to
> > + * be set immediately instead of being batched with all
> other
> > + * controls. 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..07f8c95d4f2c 100644
> > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
> > @@ -22,21 +22,29 @@ 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<std::tuple<uint32_t,
> uint8_t, bool>> delayList)
>
> It could make sense to define a struct type to group the three
> parameters.
>
Agree, I will fix this. I was going for the easiest/quickest possible
thing here, as this will all be deprecated shortly.
>
> > {
> > std::lock_guard<std::mutex> lock(lock_);
> >
> > dev_ = dev;
> > - delay_ = delayList;
> > + delay_.clear();
> > ctrl_.clear();
> >
> > - /* Find the largest delay across all controls. */
> > maxDelay_ = 0;
> > - for (auto const &p : delay_) {
> > + for (auto const &c : delayList) {
> > + uint32_t id = std::get<0>(c);
> > + uint8_t delay = std::get<1>(c);
> > + bool immediateWrite = std::get<2>(c);
> > +
> > + delay_[id] = delay;
> > + immediateWrite_[id] = immediateWrite;
>
> "immediate write" isn't a great name, as the control isn't written
> immediately, it's still delayed.
>
How about "priority controls" to signify they must be written ahead of the
batch?
>
> > +
> > 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(id) << " with delay "
> > + << static_cast<int>(delay);
> > +
> > + /* Find the largest delay across all controls. */
> > + maxDelay_ = std::max(maxDelay_, delay);
> > }
> >
> > init_ = true;
> > @@ -121,8 +129,21 @@ int StaggeredCtrl::write()
> > 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 (immediateWrite_[p.first]) {
> > + /*
> > + * 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;
> > 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..7c920c3a13c7 100644
> > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h
> > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h
> > @@ -34,7 +34,7 @@ public:
> > }
> >
> > void init(V4L2VideoDevice *dev,
> > - std::initializer_list<std::pair<const uint32_t,
> uint8_t>> delayList);
> > + std::initializer_list<std::tuple<uint32_t, uint8_t,
> bool>> delayList);
> > void reset();
> >
> > void get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t
> offset = 0);
> > @@ -85,6 +85,7 @@ private:
> > uint8_t maxDelay_;
> > V4L2VideoDevice *dev_;
> > std::unordered_map<uint32_t, uint8_t> delay_;
> > + std::unordered_map<uint32_t, bool> immediateWrite_;
>
> Should we store both the delay and the immediate write flag in a single
> map ?
>
If we use a structure to group things in the constructor, this can change
to a structure as well. We can then store both in a single map.
>
> Otherwise this looks fine. We could possibly solve these issues as part
> of the DelayedControls work. I'll let Niklas discuss his preference.
>
Yes, as I mentioned in the cover letter, DelayedControls does not
necessarily need this functionality on day 1. This change is more of an
outline as to how we can get around the problem.
Regards,
Naush
>
> > std::unordered_map<uint32_t, CircularArray> ctrl_;
> > std::mutex lock_;
> > };
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210127/db94ccde/attachment.htm>
More information about the libcamera-devel
mailing list