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

10401 diff mbox series

Some sensor controls take effect with a delay as the sensor needs time
to adjust, for example exposure. Add a 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@ragnatech.se>
---
* 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 |  87 ++++++
 src/libcamera/delayed_controls.cpp            | 260 ++++++++++++++++++
 src/libcamera/meson.build                     |   1 +
 3 files changed, 348 insertions(+)
 create mode 100644 include/libcamera/internal/delayed_controls.h
 create mode 100644 src/libcamera/delayed_controls.cpp

Hi Niklas,

On Tue, Nov 10, 2020 at 01:27:03AM +0100, Niklas Söderlund wrote:
> Some sensor controls take effect with a delay as the sensor needs time
> to adjust, for example exposure. Add a 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@ragnatech.se>

As we discussed, some comments on v1 where missed.

Should I wait for a v3 to review this part ?

Thanks
  j

> ---
> * 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 |  87 ++++++
>  src/libcamera/delayed_controls.cpp            | 260 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  3 files changed, 348 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..52bfba876e00e081
> --- /dev/null
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -0,0 +1,87 @@
> +/* 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 <mutex>
> +#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(ControlList *controls = nullptr);
> +
> +	bool push(const ControlList &controls);
> +	ControlList get(uint32_t sequence);
> +
> +	void frameStart(uint32_t sequence);
> +
> +private:
> +	class ControlInfo
> +	{
> +	public:
> +		ControlInfo()
> +			: updated(false)
> +		{
> +		}
> +
> +		ControlInfo(const ControlValue &v)
> +			: value(v), updated(true)
> +		{
> +		}
> +
> +		ControlValue value;
> +		bool updated;
> +	};
> +
> +	static constexpr int listSize = 16;
> +	class ControlArray : public std::array<ControlInfo, listSize>
> +	{
> +	public:
> +		ControlInfo &operator[](unsigned int index)
> +		{
> +			return std::array<ControlInfo, listSize>::operator[](index % listSize);
> +		}
> +
> +		const ControlInfo &operator[](unsigned int index) const
> +		{
> +			return std::array<ControlInfo, listSize>::operator[](index % listSize);
> +		}
> +	};
> +
> +	using ControlsDelays = std::unordered_map<const ControlId *, unsigned int>;
> +	using ControlsValues = std::unordered_map<const ControlId *, ControlArray>;
> +
> +	bool queue(const ControlList &controls);
> +
> +	std::mutex lock_;
> +
> +	V4L2Device *device_;
> +	ControlsDelays delays_;
> +	unsigned int maxDelay_;
> +
> +	bool running_;
> +	uint32_t fistSequence_;
> +
> +	uint32_t queueCount_;
> +	uint32_t writeCount_;
> +	ControlsValues ctrls_;
> +};
> +
> +} /* 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..f1c5f890957ec040
> --- /dev/null
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -0,0 +1,260 @@
> +/* 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/internal/v4l2_device.h"
> +
> +#include <libcamera/controls.h>
> +
> +#include "libcamera/internal/log.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 an optional 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 need 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 grates
> + * delay.
> + */
> +
> +/**
> + * \brief Construct a DelayedControls
> + * \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 itControl = controls.find(delay.first);
> +		if (itControl == 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 = itControl->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 and controls
> + * \param[in] controls Optional list of controls to apply to the device
> + *
> + * Resets the state machine to a starting position based on control values
> + * retrieved from the device. Controls may optionally be set before they are
> + * read back using \a controls.
> + */
> +void DelayedControls::reset(ControlList *controls)
> +{
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	running_ = false;
> +	fistSequence_ = 0;
> +	queueCount_ = 0;
> +	writeCount_ = 0;
> +
> +	/* Set the controls on the device if requested. */
> +	if (controls)
> +		device_->setControls(controls);
> +
> +	/* Retrieve current control values reported by the device. */
> +	std::vector<uint32_t> ids;
> +	for (auto const &delay : delays_)
> +		ids.push_back(delay.first->id());
> +
> +	ControlList devCtrls = device_->getControls(ids);
> +
> +	/* Seed the control queue with the controls reported by the device. */
> +	ctrls_.clear();
> +	for (const auto &ctrl : devCtrls) {
> +		const ControlId *id = devCtrls.infoMap()->idmap().at(ctrl.first);
> +		ctrls_[id][queueCount_] = ControlInfo(ctrl.second);
> +	}
> +
> +	queueCount_++;
> +}
> +
> +/**
> + * \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)
> +{
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	return queue(controls);
> +}
> +
> +bool DelayedControls::queue(const ControlList &controls)
> +{
> +	/* Copy state from previous frame. */
> +	for (auto &ctrl : ctrls_) {
> +		ControlInfo &info = ctrl.second[queueCount_];
> +		info.value = ctrls_[ctrl.first][queueCount_ - 1].value;
> +		info.updated = false;
> +	}
> +
> +	/* Update with new controls. */
> +	for (const auto &control : controls) {
> +		const ControlId *id = device_->controls().idmap().at(control.first);
> +
> +		if (delays_.find(id) == delays_.end())
> +			return false;
> +
> +		ControlInfo &info = ctrls_[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 to 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)
> +{
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	uint32_t adjustedSeq = sequence - fistSequence_ + 1;
> +	unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);
> +
> +	ControlList out(device_->controls());
> +	for (const auto &ctrl : ctrls_) {
> +		const ControlId *id = ctrl.first;
> +		const ControlInfo &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::frameStart(uint32_t sequence)
> +{
> +	LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> +
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	if (!running_) {
> +		fistSequence_ = 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 : ctrls_) {
> +		const ControlId *id = ctrl.first;
> +		unsigned int delayDiff = maxDelay_ - delays_[id];
> +		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
> +		const ControlInfo &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);
> +}
> +
> +} /* 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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,
   one small additional note

On Tue, Nov 10, 2020 at 01:27:03AM +0100, Niklas Söderlund wrote:
> Some sensor controls take effect with a delay as the sensor needs time
> to adjust, for example exposure. Add a 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@ragnatech.se>
> ---
> * 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 |  87 ++++++
>  src/libcamera/delayed_controls.cpp            | 260 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  3 files changed, 348 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..52bfba876e00e081
> --- /dev/null
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -0,0 +1,87 @@
> +/* 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 <mutex>
> +#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(ControlList *controls = nullptr);
> +
> +	bool push(const ControlList &controls);
> +	ControlList get(uint32_t sequence);
> +
> +	void frameStart(uint32_t sequence);
> +
> +private:
> +	class ControlInfo

We have a library wide ControlInfo class.
Should we find a non-conflicting name ?

> +	{
> +	public:
> +		ControlInfo()
> +			: updated(false)
> +		{
> +		}
> +
> +		ControlInfo(const ControlValue &v)
> +			: value(v), updated(true)
> +		{
> +		}
> +
> +		ControlValue value;
> +		bool updated;
> +	};
> +
> +	static constexpr int listSize = 16;
> +	class ControlArray : public std::array<ControlInfo, listSize>
> +	{
> +	public:
> +		ControlInfo &operator[](unsigned int index)
> +		{
> +			return std::array<ControlInfo, listSize>::operator[](index % listSize);
> +		}
> +
> +		const ControlInfo &operator[](unsigned int index) const
> +		{
> +			return std::array<ControlInfo, listSize>::operator[](index % listSize);
> +		}
> +	};
> +
> +	using ControlsDelays = std::unordered_map<const ControlId *, unsigned int>;
> +	using ControlsValues = std::unordered_map<const ControlId *, ControlArray>;
> +
> +	bool queue(const ControlList &controls);
> +
> +	std::mutex lock_;
> +
> +	V4L2Device *device_;
> +	ControlsDelays delays_;
> +	unsigned int maxDelay_;
> +
> +	bool running_;
> +	uint32_t fistSequence_;
> +
> +	uint32_t queueCount_;
> +	uint32_t writeCount_;
> +	ControlsValues ctrls_;
> +};
> +
> +} /* 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..f1c5f890957ec040
> --- /dev/null
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -0,0 +1,260 @@
> +/* 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/internal/v4l2_device.h"
> +
> +#include <libcamera/controls.h>
> +
> +#include "libcamera/internal/log.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 an optional 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 need 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 grates
> + * delay.
> + */
> +
> +/**
> + * \brief Construct a DelayedControls
> + * \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 itControl = controls.find(delay.first);
> +		if (itControl == 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 = itControl->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 and controls
> + * \param[in] controls Optional list of controls to apply to the device
> + *
> + * Resets the state machine to a starting position based on control values
> + * retrieved from the device. Controls may optionally be set before they are
> + * read back using \a controls.
> + */
> +void DelayedControls::reset(ControlList *controls)
> +{
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	running_ = false;
> +	fistSequence_ = 0;
> +	queueCount_ = 0;
> +	writeCount_ = 0;
> +
> +	/* Set the controls on the device if requested. */
> +	if (controls)
> +		device_->setControls(controls);
> +
> +	/* Retrieve current control values reported by the device. */
> +	std::vector<uint32_t> ids;
> +	for (auto const &delay : delays_)
> +		ids.push_back(delay.first->id());
> +
> +	ControlList devCtrls = device_->getControls(ids);
> +
> +	/* Seed the control queue with the controls reported by the device. */
> +	ctrls_.clear();
> +	for (const auto &ctrl : devCtrls) {
> +		const ControlId *id = devCtrls.infoMap()->idmap().at(ctrl.first);
> +		ctrls_[id][queueCount_] = ControlInfo(ctrl.second);
> +	}
> +
> +	queueCount_++;
> +}
> +
> +/**
> + * \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)
> +{
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	return queue(controls);
> +}
> +
> +bool DelayedControls::queue(const ControlList &controls)
> +{
> +	/* Copy state from previous frame. */
> +	for (auto &ctrl : ctrls_) {
> +		ControlInfo &info = ctrl.second[queueCount_];
> +		info.value = ctrls_[ctrl.first][queueCount_ - 1].value;
> +		info.updated = false;
> +	}
> +
> +	/* Update with new controls. */
> +	for (const auto &control : controls) {
> +		const ControlId *id = device_->controls().idmap().at(control.first);
> +
> +		if (delays_.find(id) == delays_.end())
> +			return false;
> +
> +		ControlInfo &info = ctrls_[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 to 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)
> +{
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	uint32_t adjustedSeq = sequence - fistSequence_ + 1;
> +	unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);
> +
> +	ControlList out(device_->controls());
> +	for (const auto &ctrl : ctrls_) {
> +		const ControlId *id = ctrl.first;
> +		const ControlInfo &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::frameStart(uint32_t sequence)
> +{
> +	LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> +
> +	std::lock_guard<std::mutex> lock(lock_);
> +
> +	if (!running_) {
> +		fistSequence_ = 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 : ctrls_) {
> +		const ControlId *id = ctrl.first;
> +		unsigned int delayDiff = maxDelay_ - delays_[id];
> +		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
> +		const ControlInfo &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);
> +}
> +
> +} /* 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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Tue, 10 Nov 2020 at 00:27, Niklas Söderlund <
niklas.soderlund@ragnatech.se> wrote:

> Some sensor controls take effect with a delay as the sensor needs time
> to adjust, for example exposure. Add a 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@ragnatech.se>
> ---
> * 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 |  87 ++++++
>  src/libcamera/delayed_controls.cpp            | 260 ++++++++++++++++++
>  src/libcamera/meson.build                     |   1 +
>  3 files changed, 348 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..52bfba876e00e081
> --- /dev/null
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -0,0 +1,87 @@
> +/* 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 <mutex>
> +#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(ControlList *controls = nullptr);
> +
> +       bool push(const ControlList &controls);
> +       ControlList get(uint32_t sequence);
> +
> +       void frameStart(uint32_t sequence);
> +
> +private:
> +       class ControlInfo
> +       {
> +       public:
> +               ControlInfo()
> +                       : updated(false)
> +               {
> +               }
> +
> +               ControlInfo(const ControlValue &v)
> +                       : value(v), updated(true)
> +               {
> +               }
> +
> +               ControlValue value;
> +               bool updated;
> +       };
> +
> +       static constexpr int listSize = 16;
> +       class ControlArray : public std::array<ControlInfo, listSize>
> +       {
> +       public:
> +               ControlInfo &operator[](unsigned int index)
> +               {
> +                       return std::array<ControlInfo,
> listSize>::operator[](index % listSize);
> +               }
> +
> +               const ControlInfo &operator[](unsigned int index) const
> +               {
> +                       return std::array<ControlInfo,
> listSize>::operator[](index % listSize);
> +               }
> +       };
> +
> +       using ControlsDelays = std::unordered_map<const ControlId *,
> unsigned int>;
> +       using ControlsValues = std::unordered_map<const ControlId *,
> ControlArray>;
> +
> +       bool queue(const ControlList &controls);
> +
> +       std::mutex lock_;
> +
> +       V4L2Device *device_;
> +       ControlsDelays delays_;
> +       unsigned int maxDelay_;
> +
> +       bool running_;
> +       uint32_t fistSequence_;
> +
> +       uint32_t queueCount_;
> +       uint32_t writeCount_;
> +       ControlsValues ctrls_;
> +};
> +
> +} /* 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..f1c5f890957ec040
> --- /dev/null
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -0,0 +1,260 @@
> +/* 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/internal/v4l2_device.h"
> +
> +#include <libcamera/controls.h>
> +
> +#include "libcamera/internal/log.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 an optional 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 need 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 grates
> + * delay.
> + */
> +
> +/**
> + * \brief Construct a DelayedControls
> + * \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 itControl = controls.find(delay.first);
> +               if (itControl == 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 = itControl->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 and controls
> + * \param[in] controls Optional list of controls to apply to the device
> + *
> + * Resets the state machine to a starting position based on control values
> + * retrieved from the device. Controls may optionally be set before they
> are
> + * read back using \a controls.
> + */
> +void DelayedControls::reset(ControlList *controls)
> +{
> +       std::lock_guard<std::mutex> lock(lock_);
> +
> +       running_ = false;
> +       fistSequence_ = 0;
> +       queueCount_ = 0;
> +       writeCount_ = 0;
> +
> +       /* Set the controls on the device if requested. */
> +       if (controls)
> +               device_->setControls(controls);
> +
> +       /* Retrieve current control values reported by the device. */
> +       std::vector<uint32_t> ids;
> +       for (auto const &delay : delays_)
> +               ids.push_back(delay.first->id());
> +
> +       ControlList devCtrls = device_->getControls(ids);
> +
> +       /* Seed the control queue with the controls reported by the
> device. */
> +       ctrls_.clear();
> +       for (const auto &ctrl : devCtrls) {
> +               const ControlId *id =
> devCtrls.infoMap()->idmap().at(ctrl.first);
> +               ctrls_[id][queueCount_] = ControlInfo(ctrl.second);
> +       }
> +
> +       queueCount_++;
> +}
> +
> +/**
> + * \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)
> +{
> +       std::lock_guard<std::mutex> lock(lock_);
> +
> +       return queue(controls);
> +}
> +
> +bool DelayedControls::queue(const ControlList &controls)
> +{
> +       /* Copy state from previous frame. */
> +       for (auto &ctrl : ctrls_) {
> +               ControlInfo &info = ctrl.second[queueCount_];
> +               info.value = ctrls_[ctrl.first][queueCount_ - 1].value;
> +               info.updated = false;
> +       }
> +
> +       /* Update with new controls. */
> +       for (const auto &control : controls) {
> +               const ControlId *id =
> device_->controls().idmap().at(control.first);
> +
> +               if (delays_.find(id) == delays_.end())
> +                       return false;
> +
> +               ControlInfo &info = ctrls_[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 to 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)
> +{
> +       std::lock_guard<std::mutex> lock(lock_);
> +
> +       uint32_t adjustedSeq = sequence - fistSequence_ + 1;
> +       unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);
> +
> +       ControlList out(device_->controls());
> +       for (const auto &ctrl : ctrls_) {
> +               const ControlId *id = ctrl.first;
> +               const ControlInfo &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::frameStart(uint32_t sequence)
> +{
> +       LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> +
> +       std::lock_guard<std::mutex> lock(lock_);
> +
> +       if (!running_) {
> +               fistSequence_ = 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 : ctrls_) {
> +               const ControlId *id = ctrl.first;
> +               unsigned int delayDiff = maxDelay_ - delays_[id];
> +               unsigned int index = std::max<int>(0, writeCount_ -
> delayDiff);
> +               const ControlInfo &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);
> +}
>

There is one thing I've been meaning to do in the StaggeredWriter for
framerate / exposure control that will be superseded with this work. I
wonder if you could add that functionality here?
The issue is that some controls may need to be set before others.  For
example, VBLANK must be set separately before EXPOSURE.  If this does not
happen, the EXPOSURE value may be artificially constrained by the old
VBLANK limits, and the device driver may erroneously reject the control.

To work around this, I was imagining that we have an "immediate update"
flag associated with a control.  If that flag is set,
DelayedControls::frameStart() will set that control straight away, i.e.
without passing a list of controls to the device.  All other controls
without that flag set will work as above, i.e. get added to the
ControlList::out map and set at the end of the function.  With this
implementation, we can guarantee VBLANK is set separately and before
EXPOSURE, hence the new EXPOSURE control will be correctly validated by the
limits of the new VBLANK.

Hope that makes sense?

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
>
>

Hi Naushir,

Thanks for your feedback.

On 2020-11-25 17:54:53 +0000, Naushir Patuck wrote:
> Hi Niklas,
> 
> On Tue, 10 Nov 2020 at 00:27, Niklas Söderlund <
> niklas.soderlund@ragnatech.se> wrote:
> 
> > Some sensor controls take effect with a delay as the sensor needs time
> > to adjust, for example exposure. Add a 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@ragnatech.se>
> > ---
> > * 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 |  87 ++++++
> >  src/libcamera/delayed_controls.cpp            | 260 ++++++++++++++++++
> >  src/libcamera/meson.build                     |   1 +
> >  3 files changed, 348 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..52bfba876e00e081
> > --- /dev/null
> > +++ b/include/libcamera/internal/delayed_controls.h
> > @@ -0,0 +1,87 @@
> > +/* 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 <mutex>
> > +#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(ControlList *controls = nullptr);
> > +
> > +       bool push(const ControlList &controls);
> > +       ControlList get(uint32_t sequence);
> > +
> > +       void frameStart(uint32_t sequence);
> > +
> > +private:
> > +       class ControlInfo
> > +       {
> > +       public:
> > +               ControlInfo()
> > +                       : updated(false)
> > +               {
> > +               }
> > +
> > +               ControlInfo(const ControlValue &v)
> > +                       : value(v), updated(true)
> > +               {
> > +               }
> > +
> > +               ControlValue value;
> > +               bool updated;
> > +       };
> > +
> > +       static constexpr int listSize = 16;
> > +       class ControlArray : public std::array<ControlInfo, listSize>
> > +       {
> > +       public:
> > +               ControlInfo &operator[](unsigned int index)
> > +               {
> > +                       return std::array<ControlInfo,
> > listSize>::operator[](index % listSize);
> > +               }
> > +
> > +               const ControlInfo &operator[](unsigned int index) const
> > +               {
> > +                       return std::array<ControlInfo,
> > listSize>::operator[](index % listSize);
> > +               }
> > +       };
> > +
> > +       using ControlsDelays = std::unordered_map<const ControlId *,
> > unsigned int>;
> > +       using ControlsValues = std::unordered_map<const ControlId *,
> > ControlArray>;
> > +
> > +       bool queue(const ControlList &controls);
> > +
> > +       std::mutex lock_;
> > +
> > +       V4L2Device *device_;
> > +       ControlsDelays delays_;
> > +       unsigned int maxDelay_;
> > +
> > +       bool running_;
> > +       uint32_t fistSequence_;
> > +
> > +       uint32_t queueCount_;
> > +       uint32_t writeCount_;
> > +       ControlsValues ctrls_;
> > +};
> > +
> > +} /* 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..f1c5f890957ec040
> > --- /dev/null
> > +++ b/src/libcamera/delayed_controls.cpp
> > @@ -0,0 +1,260 @@
> > +/* 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/internal/v4l2_device.h"
> > +
> > +#include <libcamera/controls.h>
> > +
> > +#include "libcamera/internal/log.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 an optional 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 need 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 grates
> > + * delay.
> > + */
> > +
> > +/**
> > + * \brief Construct a DelayedControls
> > + * \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 itControl = controls.find(delay.first);
> > +               if (itControl == 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 = itControl->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 and controls
> > + * \param[in] controls Optional list of controls to apply to the device
> > + *
> > + * Resets the state machine to a starting position based on control values
> > + * retrieved from the device. Controls may optionally be set before they
> > are
> > + * read back using \a controls.
> > + */
> > +void DelayedControls::reset(ControlList *controls)
> > +{
> > +       std::lock_guard<std::mutex> lock(lock_);
> > +
> > +       running_ = false;
> > +       fistSequence_ = 0;
> > +       queueCount_ = 0;
> > +       writeCount_ = 0;
> > +
> > +       /* Set the controls on the device if requested. */
> > +       if (controls)
> > +               device_->setControls(controls);
> > +
> > +       /* Retrieve current control values reported by the device. */
> > +       std::vector<uint32_t> ids;
> > +       for (auto const &delay : delays_)
> > +               ids.push_back(delay.first->id());
> > +
> > +       ControlList devCtrls = device_->getControls(ids);
> > +
> > +       /* Seed the control queue with the controls reported by the
> > device. */
> > +       ctrls_.clear();
> > +       for (const auto &ctrl : devCtrls) {
> > +               const ControlId *id =
> > devCtrls.infoMap()->idmap().at(ctrl.first);
> > +               ctrls_[id][queueCount_] = ControlInfo(ctrl.second);
> > +       }
> > +
> > +       queueCount_++;
> > +}
> > +
> > +/**
> > + * \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)
> > +{
> > +       std::lock_guard<std::mutex> lock(lock_);
> > +
> > +       return queue(controls);
> > +}
> > +
> > +bool DelayedControls::queue(const ControlList &controls)
> > +{
> > +       /* Copy state from previous frame. */
> > +       for (auto &ctrl : ctrls_) {
> > +               ControlInfo &info = ctrl.second[queueCount_];
> > +               info.value = ctrls_[ctrl.first][queueCount_ - 1].value;
> > +               info.updated = false;
> > +       }
> > +
> > +       /* Update with new controls. */
> > +       for (const auto &control : controls) {
> > +               const ControlId *id =
> > device_->controls().idmap().at(control.first);
> > +
> > +               if (delays_.find(id) == delays_.end())
> > +                       return false;
> > +
> > +               ControlInfo &info = ctrls_[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 to 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)
> > +{
> > +       std::lock_guard<std::mutex> lock(lock_);
> > +
> > +       uint32_t adjustedSeq = sequence - fistSequence_ + 1;
> > +       unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);
> > +
> > +       ControlList out(device_->controls());
> > +       for (const auto &ctrl : ctrls_) {
> > +               const ControlId *id = ctrl.first;
> > +               const ControlInfo &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::frameStart(uint32_t sequence)
> > +{
> > +       LOG(DelayedControls, Debug) << "frame " << sequence << " started";
> > +
> > +       std::lock_guard<std::mutex> lock(lock_);
> > +
> > +       if (!running_) {
> > +               fistSequence_ = 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 : ctrls_) {
> > +               const ControlId *id = ctrl.first;
> > +               unsigned int delayDiff = maxDelay_ - delays_[id];
> > +               unsigned int index = std::max<int>(0, writeCount_ -
> > delayDiff);
> > +               const ControlInfo &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);
> > +}
> >
> 
> There is one thing I've been meaning to do in the StaggeredWriter for
> framerate / exposure control that will be superseded with this work. I
> wonder if you could add that functionality here?
> The issue is that some controls may need to be set before others.  For
> example, VBLANK must be set separately before EXPOSURE.  If this does not
> happen, the EXPOSURE value may be artificially constrained by the old
> VBLANK limits, and the device driver may erroneously reject the control.
> 
> To work around this, I was imagining that we have an "immediate update"
> flag associated with a control.  If that flag is set,
> DelayedControls::frameStart() will set that control straight away, i.e.
> without passing a list of controls to the device.  All other controls
> without that flag set will work as above, i.e. get added to the
> ControlList::out map and set at the end of the function.  With this
> implementation, we can guarantee VBLANK is set separately and before
> EXPOSURE, hence the new EXPOSURE control will be correctly validated by the
> limits of the new VBLANK.
> 
> Hope that makes sense?

I think the idea make sens but I think it should be added as part of a 
series that makes use of if so it can be evaluated in a working context.  
For now I would like to focus on getting this series merged as it's been 
floating around the ML for far too long IMHO and then we can build new 
features on top.

> 
> 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
> >
> >

Hi Niklas,



On Wed, 25 Nov 2020, 7:58 pm Niklas Söderlund, <
niklas.soderlund@ragnatech.se> wrote:

> Hi Naushir,
>
> Thanks for your feedback.
>
> On 2020-11-25 17:54:53 +0000, Naushir Patuck wrote:
> > Hi Niklas,
> >
> > On Tue, 10 Nov 2020 at 00:27, Niklas Söderlund <
> > niklas.soderlund@ragnatech.se> wrote:
> >
> > > Some sensor controls take effect with a delay as the sensor needs time
> > > to adjust, for example exposure. Add a 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@ragnatech.se>
> > > ---
> > > * 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 |  87 ++++++
> > >  src/libcamera/delayed_controls.cpp            | 260 ++++++++++++++++++
> > >  src/libcamera/meson.build                     |   1 +
> > >  3 files changed, 348 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..52bfba876e00e081
> > > --- /dev/null
> > > +++ b/include/libcamera/internal/delayed_controls.h
> > > @@ -0,0 +1,87 @@
> > > +/* 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 <mutex>
> > > +#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(ControlList *controls = nullptr);
> > > +
> > > +       bool push(const ControlList &controls);
> > > +       ControlList get(uint32_t sequence);
> > > +
> > > +       void frameStart(uint32_t sequence);
> > > +
> > > +private:
> > > +       class ControlInfo
> > > +       {
> > > +       public:
> > > +               ControlInfo()
> > > +                       : updated(false)
> > > +               {
> > > +               }
> > > +
> > > +               ControlInfo(const ControlValue &v)
> > > +                       : value(v), updated(true)
> > > +               {
> > > +               }
> > > +
> > > +               ControlValue value;
> > > +               bool updated;
> > > +       };
> > > +
> > > +       static constexpr int listSize = 16;
> > > +       class ControlArray : public std::array<ControlInfo, listSize>
> > > +       {
> > > +       public:
> > > +               ControlInfo &operator[](unsigned int index)
> > > +               {
> > > +                       return std::array<ControlInfo,
> > > listSize>::operator[](index % listSize);
> > > +               }
> > > +
> > > +               const ControlInfo &operator[](unsigned int index) const
> > > +               {
> > > +                       return std::array<ControlInfo,
> > > listSize>::operator[](index % listSize);
> > > +               }
> > > +       };
> > > +
> > > +       using ControlsDelays = std::unordered_map<const ControlId *,
> > > unsigned int>;
> > > +       using ControlsValues = std::unordered_map<const ControlId *,
> > > ControlArray>;
> > > +
> > > +       bool queue(const ControlList &controls);
> > > +
> > > +       std::mutex lock_;
> > > +
> > > +       V4L2Device *device_;
> > > +       ControlsDelays delays_;
> > > +       unsigned int maxDelay_;
> > > +
> > > +       bool running_;
> > > +       uint32_t fistSequence_;
> > > +
> > > +       uint32_t queueCount_;
> > > +       uint32_t writeCount_;
> > > +       ControlsValues ctrls_;
> > > +};
> > > +
> > > +} /* 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..f1c5f890957ec040
> > > --- /dev/null
> > > +++ b/src/libcamera/delayed_controls.cpp
> > > @@ -0,0 +1,260 @@
> > > +/* 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/internal/v4l2_device.h"
> > > +
> > > +#include <libcamera/controls.h>
> > > +
> > > +#include "libcamera/internal/log.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 an optional 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 need 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
> grates
> > > + * delay.
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct a DelayedControls
> > > + * \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 itControl = controls.find(delay.first);
> > > +               if (itControl == 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 = itControl->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 and controls
> > > + * \param[in] controls Optional list of controls to apply to the
> device
> > > + *
> > > + * Resets the state machine to a starting position based on control
> values
> > > + * retrieved from the device. Controls may optionally be set before
> they
> > > are
> > > + * read back using \a controls.
> > > + */
> > > +void DelayedControls::reset(ControlList *controls)
> > > +{
> > > +       std::lock_guard<std::mutex> lock(lock_);
> > > +
> > > +       running_ = false;
> > > +       fistSequence_ = 0;
> > > +       queueCount_ = 0;
> > > +       writeCount_ = 0;
> > > +
> > > +       /* Set the controls on the device if requested. */
> > > +       if (controls)
> > > +               device_->setControls(controls);
> > > +
> > > +       /* Retrieve current control values reported by the device. */
> > > +       std::vector<uint32_t> ids;
> > > +       for (auto const &delay : delays_)
> > > +               ids.push_back(delay.first->id());
> > > +
> > > +       ControlList devCtrls = device_->getControls(ids);
> > > +
> > > +       /* Seed the control queue with the controls reported by the
> > > device. */
> > > +       ctrls_.clear();
> > > +       for (const auto &ctrl : devCtrls) {
> > > +               const ControlId *id =
> > > devCtrls.infoMap()->idmap().at(ctrl.first);
> > > +               ctrls_[id][queueCount_] = ControlInfo(ctrl.second);
> > > +       }
> > > +
> > > +       queueCount_++;
> > > +}
> > > +
> > > +/**
> > > + * \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)
> > > +{
> > > +       std::lock_guard<std::mutex> lock(lock_);
> > > +
> > > +       return queue(controls);
> > > +}
> > > +
> > > +bool DelayedControls::queue(const ControlList &controls)
> > > +{
> > > +       /* Copy state from previous frame. */
> > > +       for (auto &ctrl : ctrls_) {
> > > +               ControlInfo &info = ctrl.second[queueCount_];
> > > +               info.value = ctrls_[ctrl.first][queueCount_ - 1].value;
> > > +               info.updated = false;
> > > +       }
> > > +
> > > +       /* Update with new controls. */
> > > +       for (const auto &control : controls) {
> > > +               const ControlId *id =
> > > device_->controls().idmap().at(control.first);
> > > +
> > > +               if (delays_.find(id) == delays_.end())
> > > +                       return false;
> > > +
> > > +               ControlInfo &info = ctrls_[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 to 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)
> > > +{
> > > +       std::lock_guard<std::mutex> lock(lock_);
> > > +
> > > +       uint32_t adjustedSeq = sequence - fistSequence_ + 1;
> > > +       unsigned int index = std::max<int>(0, adjustedSeq - maxDelay_);
> > > +
> > > +       ControlList out(device_->controls());
> > > +       for (const auto &ctrl : ctrls_) {
> > > +               const ControlId *id = ctrl.first;
> > > +               const ControlInfo &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::frameStart(uint32_t sequence)
> > > +{
> > > +       LOG(DelayedControls, Debug) << "frame " << sequence << "
> started";
> > > +
> > > +       std::lock_guard<std::mutex> lock(lock_);
> > > +
> > > +       if (!running_) {
> > > +               fistSequence_ = 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 : ctrls_) {
> > > +               const ControlId *id = ctrl.first;
> > > +               unsigned int delayDiff = maxDelay_ - delays_[id];
> > > +               unsigned int index = std::max<int>(0, writeCount_ -
> > > delayDiff);
> > > +               const ControlInfo &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);
> > > +}
> > >
> >
> > There is one thing I've been meaning to do in the StaggeredWriter for
> > framerate / exposure control that will be superseded with this work. I
> > wonder if you could add that functionality here?
> > The issue is that some controls may need to be set before others.  For
> > example, VBLANK must be set separately before EXPOSURE.  If this does not
> > happen, the EXPOSURE value may be artificially constrained by the old
> > VBLANK limits, and the device driver may erroneously reject the control.
> >
> > To work around this, I was imagining that we have an "immediate update"
> > flag associated with a control.  If that flag is set,
> > DelayedControls::frameStart() will set that control straight away, i.e.
> > without passing a list of controls to the device.  All other controls
> > without that flag set will work as above, i.e. get added to the
> > ControlList::out map and set at the end of the function.  With this
> > implementation, we can guarantee VBLANK is set separately and before
> > EXPOSURE, hence the new EXPOSURE control will be correctly validated by
> the
> > limits of the new VBLANK.
> >
> > Hope that makes sense?
>
> I think the idea make sens but I think it should be added as part of a
> series that makes use of if so it can be evaluated in a working context.
> For now I would like to focus on getting this series merged as it's been
> floating around the ML for far too long IMHO and then we can build new
> features on top.
>

Sure that makes sense. I might include it as part of my fps control
changes.  Otherwise, apart from adding the flushing mechanism to match the
reset behaviour of staggered write, I have no other objections to this work.

Regards,
Naush

>
> > 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
>