[libcamera-devel,v5,1/8] libcamera: delayed_controls: Add helper for controls that apply with a delay

10875 diff mbox series

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 through 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@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
---
* Changes since v4
- Fold queue() into push() as the mutex was dropped in v3 having a
  public accessors that takes the mutex is no longer needed.
- Add delayed_controls.h to meson.build.
- Update patch subject and commit message.
- Have class Info inherit from ControlValue.
- Add todo to reevaluate if maps should be indexed on ControlID or
  unsigned int.
- Update documentation.

* Changes since v3
- 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 |  81 ++++++
 include/libcamera/internal/meson.build        |   1 +
 src/libcamera/delayed_controls.cpp            | 247 ++++++++++++++++++
 src/libcamera/meson.build                     |   1 +
 4 files changed, 330 insertions(+)
 create mode 100644 include/libcamera/internal/delayed_controls.h
 create mode 100644 src/libcamera/delayed_controls.cpp

Hi Niklas,

On 16/01/2021 11:47, Niklas Söderlund 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 through 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@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> ---
> * Changes since v4
> - Fold queue() into push() as the mutex was dropped in v3 having a
>   public accessors that takes the mutex is no longer needed.
> - Add delayed_controls.h to meson.build.
> - Update patch subject and commit message.
> - Have class Info inherit from ControlValue.
> - Add todo to reevaluate if maps should be indexed on ControlID or
>   unsigned int.
> - Update documentation.
> 
> * Changes since v3
> - 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 |  81 ++++++
>  include/libcamera/internal/meson.build        |   1 +
>  src/libcamera/delayed_controls.cpp            | 247 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  4 files changed, 330 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..dc447a882514182f
> --- /dev/null
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> + *
> + * delayed_controls.h - Helper to deal with controls that take effect 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 ControlValue
> +	{
> +	public:
> +		Info()
> +			: updated(false)
> +		{
> +		}
> +
> +		Info(const ControlValue &v)
> +			: ControlValue(v), updated(true)
> +		{
> +		}
> +
> +		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);
> +		}
> +	};
> +
> +	V4L2Device *device_;
> +	/* \todo Evaluate if we should index on ControlId * or unsigned int */
> +	std::unordered_map<const ControlId *, unsigned int> delays_;
> +	unsigned int maxDelay_;
> +
> +	bool running_;
> +	uint32_t firstSequence_;
> +
> +	uint32_t queueCount_;
> +	uint32_t writeCount_;
> +	/* \todo Evaluate if we should index on ControlId * or unsigned int */
> +	std::unordered_map<const ControlId *, ControlRingBuffer> values_;

Oh, so we have a ring buffer for every control. That sort of surprises
me a little.

Would a ring buffer of ControlLists make sense? Or are they more
difficult to instantiate? (That wouldn't surprise me ;D)


> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 1b1bdc77951235a5..e67a359fb8656f71 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -17,6 +17,7 @@ libcamera_internal_headers = files([
>      'camera_sensor.h',
>      'control_serializer.h',
>      'control_validator.h',
> +    'delayed_controls.h',
>      'device_enumerator.h',
>      'device_enumerator_sysfs.h',
>      'device_enumerator_udev.h',
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> new file mode 100644
> index 0000000000000000..f24db43d7f1e357d
> --- /dev/null
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -0,0 +1,247 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> + *
> + * delayed_controls.h - Helper to deal with controls that take effect 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 take effect 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 analog gain. 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);
> +	}
> +}
> +
> +/**
> + * \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)
> +{
> +	/* Copy state from previous frame. */
> +	for (auto &ctrl : values_) {
> +		Info &info = ctrl.second[queueCount_];
> +		info = values_[ctrl.first][queueCount_ - 1];
> +		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 = control.second;
> +		info.updated = true;
> +
> +		LOG(DelayedControls, Debug)
> +			<< "Queuing " << id->name()
> +			<< " to " << info.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_) {

I think I was a bit surprised to see that we have a ring per control, so
I now see that this function is taking all the controls in parallel ring
buffers of a specific sequence and generating a list from them.

That makes me think even more that the ControlList could be the item on
the RingBuffer in the first place - but the list would have to be reset
when 'removed' from the Ring I expect.

Perhaps that's as 'simple' as a std::move() operation though.



> +		const ControlId *id = ctrl.first;
> +		const Info &info = ctrl.second[index];
> +
> +		out.set(id->id(), info);
> +
> +		LOG(DelayedControls, Debug)
> +			<< "Reading " << id->name()
> +			<< " to " << info.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

s/frame.This/frame. This/

> + * exposure (SOE) V4L2 event.

This doesn't feel very clear to me.

This function 'writes' controls to the device, and

> + */
> +void DelayedControls::applyControls(uint32_t sequence)
> +{

If this is occurring as a callback from the SOE v4L2 event, should there
be locking in here to protect variables such as writeCount_ or queueCount_ ?


> +	LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> +
> +	if (!running_) {
> +		firstSequence_ = sequence;
> +		running_ = true;
> +	}
> +
> +	/*
> +	 * Create control list peaking ahead in the value queue to ensure

s/peaking/peeking/ ?


> +	 * 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];

Aha - actually - seeing that each control has different delays probably
changes my mind about storing a full ControlList in the ring buffer....



> +		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
> +		const Info &info = ctrl.second[index];
> +
> +		if (info.updated) {
> +			out.set(id->id(), info);
> +			LOG(DelayedControls, Debug)
> +				<< "Setting " << id->name()
> +				<< " to " << info.toString()
> +				<< " at index " << index;
> +		}
> +	}
> +
> +	writeCount_++;
> +
> +	while (writeCount_ >= queueCount_) {
> +		LOG(DelayedControls, Debug)
> +			<< "Queue is empty, auto queue no-op.";
> +		push({});
> +	}
> +
> +	device_->setControls(&out);
> +}
> +
> +} /* 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',
>

Hi Kieran,

Thanks for your feedback.

On 2021-01-27 17:39:11 +0000, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 16/01/2021 11:47, Niklas Söderlund 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 through 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@ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> > * Changes since v4
> > - Fold queue() into push() as the mutex was dropped in v3 having a
> >   public accessors that takes the mutex is no longer needed.
> > - Add delayed_controls.h to meson.build.
> > - Update patch subject and commit message.
> > - Have class Info inherit from ControlValue.
> > - Add todo to reevaluate if maps should be indexed on ControlID or
> >   unsigned int.
> > - Update documentation.
> > 
> > * Changes since v3
> > - 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 |  81 ++++++
> >  include/libcamera/internal/meson.build        |   1 +
> >  src/libcamera/delayed_controls.cpp            | 247 ++++++++++++++++++
> >  src/libcamera/meson.build                     |   1 +
> >  4 files changed, 330 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..dc447a882514182f
> > --- /dev/null
> > +++ b/include/libcamera/internal/delayed_controls.h
> > @@ -0,0 +1,81 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> > + *
> > + * delayed_controls.h - Helper to deal with controls that take effect 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 ControlValue
> > +	{
> > +	public:
> > +		Info()
> > +			: updated(false)
> > +		{
> > +		}
> > +
> > +		Info(const ControlValue &v)
> > +			: ControlValue(v), updated(true)
> > +		{
> > +		}
> > +
> > +		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);
> > +		}
> > +	};
> > +
> > +	V4L2Device *device_;
> > +	/* \todo Evaluate if we should index on ControlId * or unsigned int */
> > +	std::unordered_map<const ControlId *, unsigned int> delays_;
> > +	unsigned int maxDelay_;
> > +
> > +	bool running_;
> > +	uint32_t firstSequence_;
> > +
> > +	uint32_t queueCount_;
> > +	uint32_t writeCount_;
> > +	/* \todo Evaluate if we should index on ControlId * or unsigned int */
> > +	std::unordered_map<const ControlId *, ControlRingBuffer> values_;
> 
> Oh, so we have a ring buffer for every control. That sort of surprises
> me a little.
> 
> Would a ring buffer of ControlLists make sense? Or are they more
> difficult to instantiate? (That wouldn't surprise me ;D)
> 

I think you answer this yourself at the end ;-)

> 
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_INTERNAL_DELAYED_CONTROLS_H__ */
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 1b1bdc77951235a5..e67a359fb8656f71 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -17,6 +17,7 @@ libcamera_internal_headers = files([
> >      'camera_sensor.h',
> >      'control_serializer.h',
> >      'control_validator.h',
> > +    'delayed_controls.h',
> >      'device_enumerator.h',
> >      'device_enumerator_sysfs.h',
> >      'device_enumerator_udev.h',
> > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> > new file mode 100644
> > index 0000000000000000..f24db43d7f1e357d
> > --- /dev/null
> > +++ b/src/libcamera/delayed_controls.cpp
> > @@ -0,0 +1,247 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> > + *
> > + * delayed_controls.h - Helper to deal with controls that take effect 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 take effect 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 analog gain. 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);
> > +	}
> > +}
> > +
> > +/**
> > + * \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)
> > +{
> > +	/* Copy state from previous frame. */
> > +	for (auto &ctrl : values_) {
> > +		Info &info = ctrl.second[queueCount_];
> > +		info = values_[ctrl.first][queueCount_ - 1];
> > +		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 = control.second;
> > +		info.updated = true;
> > +
> > +		LOG(DelayedControls, Debug)
> > +			<< "Queuing " << id->name()
> > +			<< " to " << info.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_) {
> 
> I think I was a bit surprised to see that we have a ring per control, so
> I now see that this function is taking all the controls in parallel ring
> buffers of a specific sequence and generating a list from them.
> 
> That makes me think even more that the ControlList could be the item on
> the RingBuffer in the first place - but the list would have to be reset
> when 'removed' from the Ring I expect.
> 
> Perhaps that's as 'simple' as a std::move() operation though.
> 
> 
> 
> > +		const ControlId *id = ctrl.first;
> > +		const Info &info = ctrl.second[index];
> > +
> > +		out.set(id->id(), info);
> > +
> > +		LOG(DelayedControls, Debug)
> > +			<< "Reading " << id->name()
> > +			<< " to " << info.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
> 
> s/frame.This/frame. This/
> 
> > + * exposure (SOE) V4L2 event.
> 
> This doesn't feel very clear to me.
> 
> This function 'writes' controls to the device, and

and ?

> 
> > + */
> > +void DelayedControls::applyControls(uint32_t sequence)
> > +{
> 
> If this is occurring as a callback from the SOE v4L2 event, should there
> be locking in here to protect variables such as writeCount_ or queueCount_ ?

There where locking for this in earlier versions but after review it was 
judged not needed, at least not yet.

> 
> 
> > +	LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> > +
> > +	if (!running_) {
> > +		firstSequence_ = sequence;
> > +		running_ = true;
> > +	}
> > +
> > +	/*
> > +	 * Create control list peaking ahead in the value queue to ensure
> 
> s/peaking/peeking/ ?

Thanks.

> 
> 
> > +	 * 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];
> 
> Aha - actually - seeing that each control has different delays probably
> changes my mind about storing a full ControlList in the ring buffer....

I'm sure we will rework this design a few times as we learn more.

> 
> 
> 
> > +		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
> > +		const Info &info = ctrl.second[index];
> > +
> > +		if (info.updated) {
> > +			out.set(id->id(), info);
> > +			LOG(DelayedControls, Debug)
> > +				<< "Setting " << id->name()
> > +				<< " to " << info.toString()
> > +				<< " at index " << index;
> > +		}
> > +	}
> > +
> > +	writeCount_++;
> > +
> > +	while (writeCount_ >= queueCount_) {
> > +		LOG(DelayedControls, Debug)
> > +			<< "Queue is empty, auto queue no-op.";
> > +		push({});
> > +	}
> > +
> > +	device_->setControls(&out);
> > +}
> > +
> > +} /* 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',
> > 
> 
> -- 
> Regards
> --
> Kieran