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