[libcamera-devel] [PATCH v4 1/8] libcamera: delayed_controls: Add helper for controls that applies with a delay
Naushir Patuck
naush at raspberrypi.com
Fri Jan 8 16:31:33 CET 2021
Hi Niklas,
On Fri, 8 Jan 2021 at 15:21, Niklas Söderlund <niklas.soderlund at ragnatech.se>
wrote:
> Hi Naushir,
>
> Thanks for your comments.
>
> On 2020-12-15 13:53:12 +0000, Naushir Patuck wrote:
> > Hi Niklas,
> >
> > Thank you for your patch.
> >
> > On Tue, 15 Dec 2020 at 00:48, Niklas Söderlund <
> > niklas.soderlund at ragnatech.se> wrote:
> >
> > > Some sensor controls take effect with a delay as the sensor needs time
> > > to adjust, for example exposure. Add an optional helper DelayedControls
> > > to help pipelines deal with such controls.
> > >
> > > The idea is to provide a queue of controls towards the V4L2 device and
> > > apply individual controls with the specified delay with the aim to get
> > > predictable and retrievable control values for any given frame. To do
> > > this the queue of controls needs to be at least as deep as the control
> > > with the largest delay.
> > >
> > > The DelayedControls needs to be informed of every start of exposure.
> > > This can be emulated but the helper is designed to be used with this
> > > event being provide by the kernel thru V4L2 events.
> > >
> > > This helper is based on StaggeredCtrl from the Raspberry Pi pipeline
> > > handler but expands on its API. This helpers aims to replace the
> > > Raspberry Pi implementations and mimics it behavior perfectly.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > > * Changes since v2
> > > - Drop optional argument to reset().
> > > - Update commit message.
> > > - Remove usage of Mutex.
> > > - Rename frameStart() to applyControls)_.
> > > - Rename ControlInfo to Into.
> > > - Rename ControlArray to ControlRingBuffer.
> > > - Drop ControlsDelays and ControlsValues.
> > > - Sort headers.
> > > - Rename iterators.
> > > - Simplify queueCount_ handeling in reset().
> > > - Add more warnings.
> > > - Update documentation.
> > >
> > > * Changes since v2
> > > - Improve error logic in queue() as suggested by Jean-Michel Hautbois.
> > > - s/fistSequence_/firstSequence_/
> > >
> > > * Changes since v1
> > > - Correct copyright to reflect work is derived from Raspberry Pi
> > > pipeline handler. This was always the intention and was wrong in v1.
> > > - Rewrite large parts of the documentation.
> > > - Join two loops to one in DelayedControls::DelayedControls()
> > > ---
> > > include/libcamera/internal/delayed_controls.h | 82 ++++++
> > > src/libcamera/delayed_controls.cpp | 252 ++++++++++++++++++
> > > src/libcamera/meson.build | 1 +
> > > 3 files changed, 335 insertions(+)
> > > create mode 100644 include/libcamera/internal/delayed_controls.h
> > > create mode 100644 src/libcamera/delayed_controls.cpp
> > >
> > > diff --git a/include/libcamera/internal/delayed_controls.h
> > > b/include/libcamera/internal/delayed_controls.h
> > > new file mode 100644
> > > index 0000000000000000..1292b484ec9f53e9
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/delayed_controls.h
> > > @@ -0,0 +1,82 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> > > + *
> > > + * delayed_controls.h - Helper to deal with controls that are applied
> > > with a delay
> > > + */
> > > +#ifndef __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__
> > > +#define __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__
> > > +
> > > +#include <stdint.h>
> > > +#include <unordered_map>
> > > +
> > > +#include <libcamera/controls.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class V4L2Device;
> > > +
> > > +class DelayedControls
> > > +{
> > > +public:
> > > + DelayedControls(V4L2Device *device,
> > > + const std::unordered_map<uint32_t, unsigned
> int>
> > > &delays);
> > > +
> > > + void reset();
> > > +
> > > + bool push(const ControlList &controls);
> > > + ControlList get(uint32_t sequence);
> > > +
> > > + void applyControls(uint32_t sequence);
> > > +
> > > +private:
> > > + class Info
> > > + {
> > > + public:
> > > + Info()
> > > + : updated(false)
> > > + {
> > > + }
> > > +
> > > + Info(const ControlValue &v)
> > > + : value(v), updated(true)
> > > + {
> > > + }
> > > +
> > > + ControlValue value;
> > > + bool updated;
> > > + };
> > > +
> > > + /* \todo: Make the listSize configurable at instance creation
> > > time. */
> > > + static constexpr int listSize = 16;
> > > + class ControlRingBuffer : public std::array<Info, listSize>
> > > + {
> > > + public:
> > > + Info &operator[](unsigned int index)
> > > + {
> > > + return std::array<Info,
> > > listSize>::operator[](index % listSize);
> > > + }
> > > +
> > > + const Info &operator[](unsigned int index) const
> > > + {
> > > + return std::array<Info,
> > > listSize>::operator[](index % listSize);
> > > + }
> > > + };
> > > +
> > > + bool queue(const ControlList &controls);
> > > +
> > > + V4L2Device *device_;
> > > + std::unordered_map<const ControlId *, unsigned int> delays_;
> > > + unsigned int maxDelay_;
> > > +
> > > + bool running_;
> > > + uint32_t firstSequence_;
> > > +
> > > + uint32_t queueCount_;
> > > + uint32_t writeCount_;
> > > + std::unordered_map<const ControlId *, ControlRingBuffer>
> values_;
> > > +};
> > >
> >
> > StaggeredCtrl has a mutex to protect access to state. I wasn't entirely
> > sure that was needed. Presumably the SoF event calling applyControls()
> and
> > all other public method access occur in the same thread context, so no
> > protection is needed? Do you foresee this ever changing?
>
> As Laurent already replied, the mutex is not needed in out current
> design.
>
> >
> > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */
> > > diff --git a/src/libcamera/delayed_controls.cpp
> > > b/src/libcamera/delayed_controls.cpp
> > > new file mode 100644
> > > index 0000000000000000..db2e51f8c93c4755
> > > --- /dev/null
> > > +++ b/src/libcamera/delayed_controls.cpp
> > > @@ -0,0 +1,252 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> > > + *
> > > + * delayed_controls.h - Helper to deal with controls that are applied
> > > with a delay
> > > + */
> > > +
> > > +#include "libcamera/internal/delayed_controls.h"
> > > +
> > > +#include <libcamera/controls.h>
> > > +
> > > +#include "libcamera/internal/log.h"
> > > +#include "libcamera/internal/v4l2_device.h"
> > > +
> > > +/**
> > > + * \file delayed_controls.h
> > > + * \brief Helper to deal with controls that are applied with a delay
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(DelayedControls)
> > > +
> > > +/**
> > > + * \class DelayedControls
> > > + * \brief Helper to deal with controls that take effect with a delay
> > > + *
> > > + * Some sensor controls take effect with a delay as the sensor needs
> time
> > > to
> > > + * adjust, for example exposure and focus. This is a helper class to
> deal
> > > with
> > > + * such controls and the intended users are pipeline handlers.
> > > + *
> > > + * The idea is to extend the concept of the buffer depth of a
> pipeline the
> > > + * application needs to maintain to also cover controls. Just as with
> > > buffer
> > > + * depth if the application keeps the number of requests queued above
> the
> > > + * control depth the controls are guaranteed to take effect for the
> > > correct
> > > + * request. The control depth is determined by the control with the
> > > greatest
> > > + * delay.
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct a DelayedControls instance
> > > + * \param[in] device The V4L2 device the controls have to be applied
> to
> > > + * \param[in] delays Map of the numerical V4L2 control ids to their
> > > associated
> > > + * delays (in frames)
> > > + *
> > > + * Only controls specified in \a delays are handled. If it's desired
> to
> > > mix
> > > + * delayed controls and controls that take effect immediately the
> > > immediate
> > > + * controls must be listed in the \a delays map with a delay value of
> 0.
> > > + */
> > > +DelayedControls::DelayedControls(V4L2Device *device,
> > > + const std::unordered_map<uint32_t,
> > > unsigned int> &delays)
> > > + : device_(device), maxDelay_(0)
> > > +{
> > > + const ControlInfoMap &controls = device_->controls();
> > > +
> > > + /*
> > > + * Create a map of control ids to delays for controls exposed
> by
> > > the
> > > + * device.
> > > + */
> > > + for (auto const &delay : delays) {
> > > + auto it = controls.find(delay.first);
> > > + if (it == controls.end()) {
> > > + LOG(DelayedControls, Error)
> > > + << "Delay request for control id "
> > > + << utils::hex(delay.first)
> > > + << " but control is not exposed by
> device "
> > > + << device_->deviceNode();
> > > + continue;
> > > + }
> > > +
> > > + const ControlId *id = it->first;
> > > +
> > > + delays_[id] = delay.second;
> > > +
> > > + LOG(DelayedControls, Debug)
> > > + << "Set a delay of " << delays_[id]
> > > + << " for " << id->name();
> > > +
> > > + maxDelay_ = std::max(maxDelay_, delays_[id]);
> > > + }
> > > +
> > > + reset();
> > > +}
> > > +
> > > +/**
> > > + * \brief Reset state machine
> > > + *
> > > + * Resets the state machine to a starting position based on control
> values
> > > + * retrieved from the device.
> > > + */
> > > +void DelayedControls::reset()
> > > +{
> > > + running_ = false;
> > > + firstSequence_ = 0;
> > > + queueCount_ = 1;
> > > + writeCount_ = 0;
> > > +
> > > + /* Retrieve control as reported by the device. */
> > > + std::vector<uint32_t> ids;
> > > + for (auto const &delay : delays_)
> > > + ids.push_back(delay.first->id());
> > > +
> > > + ControlList controls = device_->getControls(ids);
> > > +
> > > + /* Seed the control queue with the controls reported by the
> > > device. */
> > > + values_.clear();
> > > + for (const auto &ctrl : controls) {
> > > + const ControlId *id =
> > > device_->controls().idmap().at(ctrl.first);
> > > + values_[id][0] = Info(ctrl.second);
> > > + }
> > >
> >
> > I think this may have been mentioned in one of the earlier rounds - this
> > may not be correct as all the controls available to the device may not
> be
> > registered with DelayedControls. In those cases, the values_ map lookup
> > would fail. You would need to validate that id is an available key in
> the
> > map.
>
> Is this not already done? The 'controls' that are iterated in this loop
> is constructed from the IDs in delays_ which only contains the controls
> with an associated delay.
>
Yes, I see that now. Sorry I missed that in my earlier read of the patch.
With the minor fixup to merge push() and queue(),
Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
>
> >
> >
> > > +}
> > > +
> > > +/**
> > > + * \brief Push a set of controls on the queue
> > > + * \param[in] controls List of controls to add to the device queue
> > > + *
> > > + * Push a set of controls to the control queue. This increases the
> > > control queue
> > > + * depth by one.
> > > + *
> > > + * \returns true if \a controls are accepted, or false otherwise
> > > + */
> > > +bool DelayedControls::push(const ControlList &controls)
> > > +{
> > > + return queue(controls);
> > > +}
> > >
> >
> > Apologies, I may have missed something obvious, but why do we have the
> > push() and queue() methods when one calls the other?
>
> Good call! This is a leftover from when there was a mutex that needed to
> be locked when called from the public API but not when the functionality
> was used internally where the mutex was already locked. Will fold these
> two together.
>
> >
> >
> > > +
> > > +bool DelayedControls::queue(const ControlList &controls)
> > > +{
> > > + /* Copy state from previous frame. */
> > > + for (auto &ctrl : values_) {
> > > + Info &info = ctrl.second[queueCount_];
> > > + info.value = values_[ctrl.first][queueCount_ -
> 1].value;
> > > + info.updated = false;
> > > + }
> > > +
> > > + /* Update with new controls. */
> > > + const ControlIdMap &idmap = device_->controls().idmap();
> > > + for (const auto &control : controls) {
> > > + const auto &it = idmap.find(control.first);
> > > + if (it == idmap.end()) {
> > > + LOG(DelayedControls, Warning)
> > > + << "Unknown control " << control.first;
> > > + return false;
> > > + }
> > > +
> > > + const ControlId *id = it->second;
> > > +
> > > + if (delays_.find(id) == delays_.end())
> > > + return false;
> > > +
> > > + Info &info = values_[id][queueCount_];
> > > +
> > > + info.value = control.second;
> > > + info.updated = true;
> > > +
> > > + LOG(DelayedControls, Debug)
> > > + << "Queuing " << id->name()
> > > + << " to " << info.value.toString()
> > > + << " at index " << queueCount_;
> > > + }
> > > +
> > > + queueCount_++;
> > > +
> > > + return true;
> > > +}
> > > +
> > > +/**
> > > + * \brief Read back controls in effect at a sequence number
> > > + * \param[in] sequence The sequence number to get controls for
> > > + *
> > > + * Read back what controls where in effect at a specific sequence
> number.
> > > The
> > > + * history is a ring buffer of 16 entries where new and old values
> > > coexist. It's
> > > + * the callers responsibility to not read too old sequence numbers
> that
> > > have been
> > > + * pushed out of the history.
> > > + *
> > > + * Historic values are evicted by pushing new values onto the queue
> using
> > > + * push(). The max history from the current sequence number that
> yields
> > > valid
> > > + * values are thus 16 minus number of controls pushed.
> > > + *
> > > + * \return The controls at \a sequence number
> > > + */
> > > +ControlList DelayedControls::get(uint32_t sequence)
> > > +{
> > > + uint32_t adjustedSeq = sequence - firstSequence_ + 1;
> > > + unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);
> > > +
> > > + ControlList out(device_->controls());
> > > + for (const auto &ctrl : values_) {
> > > + const ControlId *id = ctrl.first;
> > > + const Info &info = ctrl.second[index];
> > > +
> > > + out.set(id->id(), info.value);
> > > +
> > > + LOG(DelayedControls, Debug)
> > > + << "Reading " << id->name()
> > > + << " to " << info.value.toString()
> > > + << " at index " << index;
> > > + }
> > > +
> > > + return out;
> > > +}
> > > +
> > > +/**
> > > + * \brief Inform DelayedControls of the start of a new frame
> > > + * \param[in] sequence Sequence number of the frame that started
> > > + *
> > > + * Inform the state machine that a new frame has started and of its
> > > sequence
> > > + * number. Any user of these helpers is responsible to inform the
> helper
> > > about
> > > + * the start of any frame.This can be connected with ease to the
> start of
> > > a
> > > + * exposure (SOE) V4L2 event.
> > > + */
> > > +void DelayedControls::applyControls(uint32_t sequence)
> > > +{
> > > + LOG(DelayedControls, Debug) << "frame " << sequence << "
> started";
> > > +
> > > + if (!running_) {
> > > + firstSequence_ = sequence;
> > > + running_ = true;
> > > + }
> > > +
> > > + /*
> > > + * Create control list peaking ahead in the value queue to
> ensure
> > > + * values are set in time to satisfy the sensor delay.
> > > + */
> > > + ControlList out(device_->controls());
> > > + for (const auto &ctrl : values_) {
> > > + const ControlId *id = ctrl.first;
> > > + unsigned int delayDiff = maxDelay_ - delays_[id];
> > > + unsigned int index = std::max<int>(0, writeCount_ -
> > > delayDiff);
> > > + const Info &info = ctrl.second[index];
> > > +
> > > + if (info.updated) {
> > > + out.set(id->id(), info.value);
> > > + LOG(DelayedControls, Debug)
> > > + << "Setting " << id->name()
> > > + << " to " << info.value.toString()
> > > + << " at index " << index;
> > > + }
> > > + }
> > > +
> > > + writeCount_++;
> > > +
> > > + while (writeCount_ >= queueCount_) {
> > > + LOG(DelayedControls, Debug)
> > > + << "Queue is empty, auto queue no-op.";
> > > + queue({});
> > > + }
> > > +
> > > + device_->setControls(&out);
> > >
> >
> > As mentioned previously, this will need to separate out controls that
> must
> > be written immediately (e.g. VBLANK) instead of batched together, but
> that
> > can be added at a later stage.
>
> I agree this is just the first step and I'm sure we need to evolve and
> rework parts of DelayedControls as we gain more capabilities.
>
> >
> > Regards,
> > Naush
> >
> >
> >
> > > +}
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index 387d5d88ecae11ad..5a4bf0d7ba4fd231 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -12,6 +12,7 @@ libcamera_sources = files([
> > > 'controls.cpp',
> > > 'control_serializer.cpp',
> > > 'control_validator.cpp',
> > > + 'delayed_controls.cpp',
> > > 'device_enumerator.cpp',
> > > 'device_enumerator_sysfs.cpp',
> > > 'event_dispatcher.cpp',
> > > --
> > > 2.29.2
> > >
> > >
>
> --
> Regards,
> Niklas Söderlund
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210108/d348e3b4/attachment-0001.htm>
More information about the libcamera-devel
mailing list