[libcamera-devel,v4,1/7] libcamera: delayed_controls: Add notion of priority write

11487 diff mbox series

If an exposure time change adjusts the vblanking limits, and we set both
VBLANK and EXPOSURE controls through the VIDIOC_S_EXT_CTRLS ioctl, the
latter may fail if the value is outside of the limits calculated by the
old VBLANK value. This is a limitation in V4L2 and cannot be fixed by
setting VBLANK before EXPOSURE in a single VIDIOC_S_EXT_CTRLS ioctl.

The workaround here is to have the DelayedControls object mark the
VBLANK control as "priority write", which then write VBLANK separately
from (and ahead of) any other controls. This way, the sensor driver will
update the EXPOSURE control with new limits before the new values is
presented, and will thus be seen as valid.

To support this, a new struct DelayedControls::ControlParams is used in
the constructor to provide the control delay value as well as the
priority write flag.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Tested-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 include/libcamera/internal/delayed_controls.h |  9 +++-
 src/libcamera/delayed_controls.cpp            | 54 +++++++++++++------
 src/libcamera/pipeline/ipu3/ipu3.cpp          |  8 +--
 .../pipeline/raspberrypi/raspberrypi.cpp      | 13 +++--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  8 +--
 test/delayed_contols.cpp                      | 20 +++----
 6 files changed, 68 insertions(+), 44 deletions(-)

Hi Naushir,

Thanks for the patch !

On 04/03/2021 09:17, Naushir Patuck wrote:
> If an exposure time change adjusts the vblanking limits, and we set both
> VBLANK and EXPOSURE controls through the VIDIOC_S_EXT_CTRLS ioctl, the
> latter may fail if the value is outside of the limits calculated by the
> old VBLANK value. This is a limitation in V4L2 and cannot be fixed by
> setting VBLANK before EXPOSURE in a single VIDIOC_S_EXT_CTRLS ioctl.
> 
> The workaround here is to have the DelayedControls object mark the
> VBLANK control as "priority write", which then write VBLANK separately
> from (and ahead of) any other controls. This way, the sensor driver will
> update the EXPOSURE control with new limits before the new values is
> presented, and will thus be seen as valid.
> 
> To support this, a new struct DelayedControls::ControlParams is used in
> the constructor to provide the control delay value as well as the
> priority write flag.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Tested-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

> ---
>  include/libcamera/internal/delayed_controls.h |  9 +++-
>  src/libcamera/delayed_controls.cpp            | 54 +++++++++++++------
>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  8 +--
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 13 +++--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  8 +--
>  test/delayed_contols.cpp                      | 20 +++----
>  6 files changed, 68 insertions(+), 44 deletions(-)
> 
> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
> index dc447a882514..564d9f2e2440 100644
> --- a/include/libcamera/internal/delayed_controls.h
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -19,8 +19,13 @@ class V4L2Device;
>  class DelayedControls
>  {
>  public:
> +	struct ControlParams {
> +		unsigned int delay;
> +		bool priorityWrite;
> +	};
> +
>  	DelayedControls(V4L2Device *device,
> -			const std::unordered_map<uint32_t, unsigned int> &delays);
> +			const std::unordered_map<uint32_t, ControlParams> &controlParams);
>  
>  	void reset();
>  
> @@ -64,7 +69,7 @@ private:
>  
>  	V4L2Device *device_;
>  	/* \todo Evaluate if we should index on ControlId * or unsigned int */
> -	std::unordered_map<const ControlId *, unsigned int> delays_;
> +	std::unordered_map<const ControlId *, ControlParams> controlParams_;
>  	unsigned int maxDelay_;
>  
>  	bool running_;
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index ab1d40057c5f..3ed1dfebd035 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -40,15 +40,19 @@ LOG_DEFINE_CATEGORY(DelayedControls)
>  /**
>   * \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)
> + * \param[in] controlParams Map of the numerical V4L2 control ids to their
> + * associated control parameters.
>   *
> - * 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.
> + * The control parameters comprise of delays (in frames) and a priority write
> + * flag. If this flag is set, the relevant control is written separately from,
> + * ahead of the reset of the batched controls.
> + *
> + * Only controls specified in \a controlParams 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 controlParams map with a delay value of 0.
>   */
>  DelayedControls::DelayedControls(V4L2Device *device,
> -				 const std::unordered_map<uint32_t, unsigned int> &delays)
> +				 const std::unordered_map<uint32_t, ControlParams> &controlParams)
>  	: device_(device), maxDelay_(0)
>  {
>  	const ControlInfoMap &controls = device_->controls();
> @@ -57,12 +61,12 @@ DelayedControls::DelayedControls(V4L2Device *device,
>  	 * 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);
> +	for (auto const &param : controlParams) {
> +		auto it = controls.find(param.first);
>  		if (it == controls.end()) {
>  			LOG(DelayedControls, Error)
>  				<< "Delay request for control id "
> -				<< utils::hex(delay.first)
> +				<< utils::hex(param.first)
>  				<< " but control is not exposed by device "
>  				<< device_->deviceNode();
>  			continue;
> @@ -70,13 +74,14 @@ DelayedControls::DelayedControls(V4L2Device *device,
>  
>  		const ControlId *id = it->first;
>  
> -		delays_[id] = delay.second;
> +		controlParams_[id] = param.second;
>  
>  		LOG(DelayedControls, Debug)
> -			<< "Set a delay of " << delays_[id]
> +			<< "Set a delay of " << controlParams_[id].delay
> +			<< " and priority write flag " << controlParams_[id].priorityWrite
>  			<< " for " << id->name();
>  
> -		maxDelay_ = std::max(maxDelay_, delays_[id]);
> +		maxDelay_ = std::max(maxDelay_, controlParams_[id].delay);
>  	}
>  
>  	reset();
> @@ -97,8 +102,8 @@ void DelayedControls::reset()
>  
>  	/* Retrieve control as reported by the device. */
>  	std::vector<uint32_t> ids;
> -	for (auto const &delay : delays_)
> -		ids.push_back(delay.first->id());
> +	for (auto const &param : controlParams_)
> +		ids.push_back(param.first->id());
>  
>  	ControlList controls = device_->getControls(ids);
>  
> @@ -140,7 +145,7 @@ bool DelayedControls::push(const ControlList &controls)
>  
>  		const ControlId *id = it->second;
>  
> -		if (delays_.find(id) == delays_.end())
> +		if (controlParams_.find(id) == controlParams_.end())
>  			return false;
>  
>  		Info &info = values_[id][queueCount_];
> @@ -220,12 +225,27 @@ void DelayedControls::applyControls(uint32_t sequence)
>  	ControlList out(device_->controls());
>  	for (const auto &ctrl : values_) {
>  		const ControlId *id = ctrl.first;
> -		unsigned int delayDiff = maxDelay_ - delays_[id];
> +		unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;
>  		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
>  		const Info &info = ctrl.second[index];
>  
>  		if (info.updated) {
> -			out.set(id->id(), info);
> +			if (controlParams_[id].priorityWrite) {
> +				/*
> +				 * This control must be written now, it could
> +				 * affect validity of the other controls.
> +				 */
> +				ControlList priority(device_->controls());
> +				priority.set(id->id(), info);
> +				device_->setControls(&priority);
> +			} else {
> +				/*
> +				 * Batch up the list of controls and write them
> +				 * at the end of the function.
> +				 */
> +				out.set(id->id(), info);
> +			}
> +
>  			LOG(DelayedControls, Debug)
>  				<< "Setting " << id->name()
>  				<< " to " << info.toString()
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 3e6b88af4188..ac92f066a07e 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -963,14 +963,14 @@ int PipelineHandlerIPU3::registerCameras()
>  		 * a sensor database. For now use generic values taken from
>  		 * the Raspberry Pi and listed as 'generic values'.
>  		 */
> -		std::unordered_map<uint32_t, unsigned int> delays = {
> -			{ V4L2_CID_ANALOGUE_GAIN, 1 },
> -			{ V4L2_CID_EXPOSURE, 2 },
> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> +			{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> +			{ V4L2_CID_EXPOSURE, { 2, false } },
>  		};
>  
>  		data->delayedCtrls_ =
>  			std::make_unique<DelayedControls>(cio2->sensor()->device(),
> -							  delays);
> +							  params);
>  		data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
>  						 &DelayedControls::applyControls);
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index a60415d93705..ba74ace183db 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1244,16 +1244,15 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	if (result.params & ipa::RPi::ConfigSensorParams) {
>  		/*
>  		 * Setup our delayed control writer with the sensor default
> -		 * gain and exposure delays.
> +		 * gain and exposure delays. Mark VBLANK for priority write.
>  		 */
> -		std::unordered_map<uint32_t, unsigned int> delays = {
> -			{ V4L2_CID_ANALOGUE_GAIN, result.sensorConfig.gainDelay },
> -			{ V4L2_CID_EXPOSURE, result.sensorConfig.exposureDelay },
> -			{ V4L2_CID_VBLANK, result.sensorConfig.vblank }
> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> +			{ V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },
> +			{ V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },
> +			{ V4L2_CID_VBLANK, { result.sensorConfig.vblank, true } }
>  		};
>  
> -		delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);
> -
> +		delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), params);
>  		sensorMetadata_ = result.sensorConfig.sensorMetadata;
>  	}
>  
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index a794501a9c8d..17c0f9751cd3 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -938,14 +938,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	 * a sensor database. For now use generic values taken from
>  	 * the Raspberry Pi and listed as generic values.
>  	 */
> -	std::unordered_map<uint32_t, unsigned int> delays = {
> -		{ V4L2_CID_ANALOGUE_GAIN, 1 },
> -		{ V4L2_CID_EXPOSURE, 2 },
> +	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> +		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> +		{ V4L2_CID_EXPOSURE, { 2, false } },
>  	};
>  
>  	data->delayedCtrls_ =
>  		std::make_unique<DelayedControls>(data->sensor_->device(),
> -						  delays);
> +						  params);
>  	isp_->frameStart.connect(data->delayedCtrls_.get(),
>  				 &DelayedControls::applyControls);
>  
> diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
> index 50169b12e566..3855eb18ecd4 100644
> --- a/test/delayed_contols.cpp
> +++ b/test/delayed_contols.cpp
> @@ -72,8 +72,8 @@ protected:
>  
>  	int singleControlNoDelay()
>  	{
> -		std::unordered_map<uint32_t, unsigned int> delays = {
> -			{ V4L2_CID_BRIGHTNESS, 0 },
> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
> +			{ V4L2_CID_BRIGHTNESS, { 0, false } },
>  		};
>  		std::unique_ptr<DelayedControls> delayed =
>  			std::make_unique<DelayedControls>(dev_.get(), delays);
> @@ -109,8 +109,8 @@ protected:
>  
>  	int singleControlWithDelay()
>  	{
> -		std::unordered_map<uint32_t, unsigned int> delays = {
> -			{ V4L2_CID_BRIGHTNESS, 1 },
> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
> +			{ V4L2_CID_BRIGHTNESS, { 1, false } },
>  		};
>  		std::unique_ptr<DelayedControls> delayed =
>  			std::make_unique<DelayedControls>(dev_.get(), delays);
> @@ -150,9 +150,9 @@ protected:
>  
>  	int dualControlsWithDelay(uint32_t startOffset)
>  	{
> -		std::unordered_map<uint32_t, unsigned int> delays = {
> -			{ V4L2_CID_BRIGHTNESS, 1 },
> -			{ V4L2_CID_CONTRAST, 2 },
> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
> +			{ V4L2_CID_BRIGHTNESS, { 1, false } },
> +			{ V4L2_CID_CONTRAST, { 2, false } },
>  		};
>  		std::unique_ptr<DelayedControls> delayed =
>  			std::make_unique<DelayedControls>(dev_.get(), delays);
> @@ -197,9 +197,9 @@ protected:
>  
>  	int dualControlsMultiQueue()
>  	{
> -		std::unordered_map<uint32_t, unsigned int> delays = {
> -			{ V4L2_CID_BRIGHTNESS, 1 },
> -			{ V4L2_CID_CONTRAST, 2 },
> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
> +			{ V4L2_CID_BRIGHTNESS, { 1, false } },
> +			{ V4L2_CID_CONTRAST, { 2, false } }
>  		};
>  		std::unique_ptr<DelayedControls> delayed =
>  			std::make_unique<DelayedControls>(dev_.get(), delays);
>

Hi Naush,

On 09/03/2021 09:54, Jean-Michel Hautbois wrote:
> Hi Naushir,
> 
> Thanks for the patch !
> 
> On 04/03/2021 09:17, Naushir Patuck wrote:
>> If an exposure time change adjusts the vblanking limits, and we set both
>> VBLANK and EXPOSURE controls through the VIDIOC_S_EXT_CTRLS ioctl, the
>> latter may fail if the value is outside of the limits calculated by the
>> old VBLANK value. This is a limitation in V4L2 and cannot be fixed by
>> setting VBLANK before EXPOSURE in a single VIDIOC_S_EXT_CTRLS ioctl.
>>
>> The workaround here is to have the DelayedControls object mark the
>> VBLANK control as "priority write", which then write VBLANK separately
>> from (and ahead of) any other controls. This way, the sensor driver will
>> update the EXPOSURE control with new limits before the new values is
>> presented, and will thus be seen as valid.
>>
>> To support this, a new struct DelayedControls::ControlParams is used in
>> the constructor to provide the control delay value as well as the
>> priority write flag.
>>
>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>> Tested-by: David Plowman <david.plowman@raspberrypi.com>
>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> 
>> ---
>>  include/libcamera/internal/delayed_controls.h |  9 +++-
>>  src/libcamera/delayed_controls.cpp            | 54 +++++++++++++------
>>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  8 +--
>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 13 +++--
>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  8 +--
>>  test/delayed_contols.cpp                      | 20 +++----
>>  6 files changed, 68 insertions(+), 44 deletions(-)
>>
>> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
>> index dc447a882514..564d9f2e2440 100644
>> --- a/include/libcamera/internal/delayed_controls.h
>> +++ b/include/libcamera/internal/delayed_controls.h
>> @@ -19,8 +19,13 @@ class V4L2Device;
>>  class DelayedControls
>>  {
>>  public:
>> +	struct ControlParams {
>> +		unsigned int delay;
>> +		bool priorityWrite;
>> +	};
>> +
>>  	DelayedControls(V4L2Device *device,
>> -			const std::unordered_map<uint32_t, unsigned int> &delays);
>> +			const std::unordered_map<uint32_t, ControlParams> &controlParams);
>>  
>>  	void reset();
>>  
>> @@ -64,7 +69,7 @@ private:
>>  
>>  	V4L2Device *device_;
>>  	/* \todo Evaluate if we should index on ControlId * or unsigned int */
>> -	std::unordered_map<const ControlId *, unsigned int> delays_;
>> +	std::unordered_map<const ControlId *, ControlParams> controlParams_;
>>  	unsigned int maxDelay_;
>>  
>>  	bool running_;
>> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
>> index ab1d40057c5f..3ed1dfebd035 100644
>> --- a/src/libcamera/delayed_controls.cpp
>> +++ b/src/libcamera/delayed_controls.cpp
>> @@ -40,15 +40,19 @@ LOG_DEFINE_CATEGORY(DelayedControls)
>>  /**
>>   * \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)
>> + * \param[in] controlParams Map of the numerical V4L2 control ids to their
>> + * associated control parameters.
>>   *
>> - * 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.
>> + * The control parameters comprise of delays (in frames) and a priority write
>> + * flag. If this flag is set, the relevant control is written separately from,
>> + * ahead of the reset of the batched controls.

minor:

'and ahead of' ?

could be fixed while applying if there's nothing else major:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



>> + *
>> + * Only controls specified in \a controlParams 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 controlParams map with a delay value of 0.
>>   */
>>  DelayedControls::DelayedControls(V4L2Device *device,
>> -				 const std::unordered_map<uint32_t, unsigned int> &delays)
>> +				 const std::unordered_map<uint32_t, ControlParams> &controlParams)
>>  	: device_(device), maxDelay_(0)
>>  {
>>  	const ControlInfoMap &controls = device_->controls();
>> @@ -57,12 +61,12 @@ DelayedControls::DelayedControls(V4L2Device *device,
>>  	 * 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);
>> +	for (auto const &param : controlParams) {
>> +		auto it = controls.find(param.first);
>>  		if (it == controls.end()) {
>>  			LOG(DelayedControls, Error)
>>  				<< "Delay request for control id "
>> -				<< utils::hex(delay.first)
>> +				<< utils::hex(param.first)
>>  				<< " but control is not exposed by device "
>>  				<< device_->deviceNode();
>>  			continue;
>> @@ -70,13 +74,14 @@ DelayedControls::DelayedControls(V4L2Device *device,
>>  
>>  		const ControlId *id = it->first;
>>  
>> -		delays_[id] = delay.second;
>> +		controlParams_[id] = param.second;
>>  
>>  		LOG(DelayedControls, Debug)
>> -			<< "Set a delay of " << delays_[id]
>> +			<< "Set a delay of " << controlParams_[id].delay
>> +			<< " and priority write flag " << controlParams_[id].priorityWrite
>>  			<< " for " << id->name();
>>  
>> -		maxDelay_ = std::max(maxDelay_, delays_[id]);
>> +		maxDelay_ = std::max(maxDelay_, controlParams_[id].delay);
>>  	}
>>  
>>  	reset();
>> @@ -97,8 +102,8 @@ void DelayedControls::reset()
>>  
>>  	/* Retrieve control as reported by the device. */
>>  	std::vector<uint32_t> ids;
>> -	for (auto const &delay : delays_)
>> -		ids.push_back(delay.first->id());
>> +	for (auto const &param : controlParams_)
>> +		ids.push_back(param.first->id());
>>  
>>  	ControlList controls = device_->getControls(ids);
>>  
>> @@ -140,7 +145,7 @@ bool DelayedControls::push(const ControlList &controls)
>>  
>>  		const ControlId *id = it->second;
>>  
>> -		if (delays_.find(id) == delays_.end())
>> +		if (controlParams_.find(id) == controlParams_.end())
>>  			return false;
>>  
>>  		Info &info = values_[id][queueCount_];
>> @@ -220,12 +225,27 @@ void DelayedControls::applyControls(uint32_t sequence)
>>  	ControlList out(device_->controls());
>>  	for (const auto &ctrl : values_) {
>>  		const ControlId *id = ctrl.first;
>> -		unsigned int delayDiff = maxDelay_ - delays_[id];
>> +		unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;
>>  		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
>>  		const Info &info = ctrl.second[index];
>>  
>>  		if (info.updated) {
>> -			out.set(id->id(), info);
>> +			if (controlParams_[id].priorityWrite) {
>> +				/*
>> +				 * This control must be written now, it could
>> +				 * affect validity of the other controls.
>> +				 */
>> +				ControlList priority(device_->controls());
>> +				priority.set(id->id(), info);
>> +				device_->setControls(&priority);
>> +			} else {
>> +				/*
>> +				 * Batch up the list of controls and write them
>> +				 * at the end of the function.
>> +				 */
>> +				out.set(id->id(), info);
>> +			}
>> +
>>  			LOG(DelayedControls, Debug)
>>  				<< "Setting " << id->name()
>>  				<< " to " << info.toString()
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index 3e6b88af4188..ac92f066a07e 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -963,14 +963,14 @@ int PipelineHandlerIPU3::registerCameras()
>>  		 * a sensor database. For now use generic values taken from
>>  		 * the Raspberry Pi and listed as 'generic values'.
>>  		 */
>> -		std::unordered_map<uint32_t, unsigned int> delays = {
>> -			{ V4L2_CID_ANALOGUE_GAIN, 1 },
>> -			{ V4L2_CID_EXPOSURE, 2 },
>> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
>> +			{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
>> +			{ V4L2_CID_EXPOSURE, { 2, false } },
>>  		};
>>  
>>  		data->delayedCtrls_ =
>>  			std::make_unique<DelayedControls>(cio2->sensor()->device(),
>> -							  delays);
>> +							  params);
>>  		data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
>>  						 &DelayedControls::applyControls);
>>  
>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> index a60415d93705..ba74ace183db 100644
>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> @@ -1244,16 +1244,15 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>>  	if (result.params & ipa::RPi::ConfigSensorParams) {
>>  		/*
>>  		 * Setup our delayed control writer with the sensor default
>> -		 * gain and exposure delays.
>> +		 * gain and exposure delays. Mark VBLANK for priority write.
>>  		 */
>> -		std::unordered_map<uint32_t, unsigned int> delays = {
>> -			{ V4L2_CID_ANALOGUE_GAIN, result.sensorConfig.gainDelay },
>> -			{ V4L2_CID_EXPOSURE, result.sensorConfig.exposureDelay },
>> -			{ V4L2_CID_VBLANK, result.sensorConfig.vblank }
>> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
>> +			{ V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },
>> +			{ V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },
>> +			{ V4L2_CID_VBLANK, { result.sensorConfig.vblank, true } }
>>  		};
>>  
>> -		delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);
>> -
>> +		delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), params);
>>  		sensorMetadata_ = result.sensorConfig.sensorMetadata;
>>  	}
>>  
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index a794501a9c8d..17c0f9751cd3 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -938,14 +938,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>>  	 * a sensor database. For now use generic values taken from
>>  	 * the Raspberry Pi and listed as generic values.
>>  	 */
>> -	std::unordered_map<uint32_t, unsigned int> delays = {
>> -		{ V4L2_CID_ANALOGUE_GAIN, 1 },
>> -		{ V4L2_CID_EXPOSURE, 2 },
>> +	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
>> +		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
>> +		{ V4L2_CID_EXPOSURE, { 2, false } },
>>  	};
>>  
>>  	data->delayedCtrls_ =
>>  		std::make_unique<DelayedControls>(data->sensor_->device(),
>> -						  delays);
>> +						  params);
>>  	isp_->frameStart.connect(data->delayedCtrls_.get(),
>>  				 &DelayedControls::applyControls);
>>  
>> diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
>> index 50169b12e566..3855eb18ecd4 100644
>> --- a/test/delayed_contols.cpp
>> +++ b/test/delayed_contols.cpp
>> @@ -72,8 +72,8 @@ protected:
>>  
>>  	int singleControlNoDelay()
>>  	{
>> -		std::unordered_map<uint32_t, unsigned int> delays = {
>> -			{ V4L2_CID_BRIGHTNESS, 0 },
>> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
>> +			{ V4L2_CID_BRIGHTNESS, { 0, false } },
>>  		};
>>  		std::unique_ptr<DelayedControls> delayed =
>>  			std::make_unique<DelayedControls>(dev_.get(), delays);
>> @@ -109,8 +109,8 @@ protected:
>>  
>>  	int singleControlWithDelay()
>>  	{
>> -		std::unordered_map<uint32_t, unsigned int> delays = {
>> -			{ V4L2_CID_BRIGHTNESS, 1 },
>> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
>> +			{ V4L2_CID_BRIGHTNESS, { 1, false } },
>>  		};
>>  		std::unique_ptr<DelayedControls> delayed =
>>  			std::make_unique<DelayedControls>(dev_.get(), delays);
>> @@ -150,9 +150,9 @@ protected:
>>  
>>  	int dualControlsWithDelay(uint32_t startOffset)
>>  	{
>> -		std::unordered_map<uint32_t, unsigned int> delays = {
>> -			{ V4L2_CID_BRIGHTNESS, 1 },
>> -			{ V4L2_CID_CONTRAST, 2 },
>> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
>> +			{ V4L2_CID_BRIGHTNESS, { 1, false } },
>> +			{ V4L2_CID_CONTRAST, { 2, false } },
>>  		};
>>  		std::unique_ptr<DelayedControls> delayed =
>>  			std::make_unique<DelayedControls>(dev_.get(), delays);
>> @@ -197,9 +197,9 @@ protected:
>>  
>>  	int dualControlsMultiQueue()
>>  	{
>> -		std::unordered_map<uint32_t, unsigned int> delays = {
>> -			{ V4L2_CID_BRIGHTNESS, 1 },
>> -			{ V4L2_CID_CONTRAST, 2 },
>> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
>> +			{ V4L2_CID_BRIGHTNESS, { 1, false } },
>> +			{ V4L2_CID_CONTRAST, { 2, false } }
>>  		};
>>  		std::unique_ptr<DelayedControls> delayed =
>>  			std::make_unique<DelayedControls>(dev_.get(), delays);
>>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>

On 12/03/2021 13:25, Kieran Bingham wrote:
> Hi Naush,
> 
> On 09/03/2021 09:54, Jean-Michel Hautbois wrote:
>> Hi Naushir,
>>
>> Thanks for the patch !
>>
>> On 04/03/2021 09:17, Naushir Patuck wrote:
>>> If an exposure time change adjusts the vblanking limits, and we set both
>>> VBLANK and EXPOSURE controls through the VIDIOC_S_EXT_CTRLS ioctl, the
>>> latter may fail if the value is outside of the limits calculated by the
>>> old VBLANK value. This is a limitation in V4L2 and cannot be fixed by
>>> setting VBLANK before EXPOSURE in a single VIDIOC_S_EXT_CTRLS ioctl.
>>>
>>> The workaround here is to have the DelayedControls object mark the
>>> VBLANK control as "priority write", which then write VBLANK separately
>>> from (and ahead of) any other controls. This way, the sensor driver will
>>> update the EXPOSURE control with new limits before the new values is
>>> presented, and will thus be seen as valid.
>>>
>>> To support this, a new struct DelayedControls::ControlParams is used in
>>> the constructor to provide the control delay value as well as the
>>> priority write flag.
>>>
>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>>> Tested-by: David Plowman <david.plowman@raspberrypi.com>
>>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
>> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>>
>>> ---
>>>  include/libcamera/internal/delayed_controls.h |  9 +++-
>>>  src/libcamera/delayed_controls.cpp            | 54 +++++++++++++------
>>>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  8 +--
>>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 13 +++--
>>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  8 +--
>>>  test/delayed_contols.cpp                      | 20 +++----
>>>  6 files changed, 68 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
>>> index dc447a882514..564d9f2e2440 100644
>>> --- a/include/libcamera/internal/delayed_controls.h
>>> +++ b/include/libcamera/internal/delayed_controls.h
>>> @@ -19,8 +19,13 @@ class V4L2Device;
>>>  class DelayedControls
>>>  {
>>>  public:
>>> +	struct ControlParams {
>>> +		unsigned int delay;
>>> +		bool priorityWrite;
>>> +	};
>>> +
>>>  	DelayedControls(V4L2Device *device,
>>> -			const std::unordered_map<uint32_t, unsigned int> &delays);
>>> +			const std::unordered_map<uint32_t, ControlParams> &controlParams);
>>>  
>>>  	void reset();
>>>  
>>> @@ -64,7 +69,7 @@ private:
>>>  
>>>  	V4L2Device *device_;
>>>  	/* \todo Evaluate if we should index on ControlId * or unsigned int */
>>> -	std::unordered_map<const ControlId *, unsigned int> delays_;
>>> +	std::unordered_map<const ControlId *, ControlParams> controlParams_;
>>>  	unsigned int maxDelay_;
>>>  
>>>  	bool running_;
>>> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
>>> index ab1d40057c5f..3ed1dfebd035 100644
>>> --- a/src/libcamera/delayed_controls.cpp
>>> +++ b/src/libcamera/delayed_controls.cpp
>>> @@ -40,15 +40,19 @@ LOG_DEFINE_CATEGORY(DelayedControls)
>>>  /**
>>>   * \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)
>>> + * \param[in] controlParams Map of the numerical V4L2 control ids to their
>>> + * associated control parameters.
>>>   *
>>> - * 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.
>>> + * The control parameters comprise of delays (in frames) and a priority write
>>> + * flag. If this flag is set, the relevant control is written separately from,
>>> + * ahead of the reset of the batched controls.
> 
> minor:
> 
> 'and ahead of' ?
> 
> could be fixed while applying if there's nothing else major:


Could you confirm if this is correct to say 'and ahead of the reset of'
or if it was supposed to be 'and ahead of the rest of' ...


> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 
> 
>>> + *
>>> + * Only controls specified in \a controlParams 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 controlParams map with a delay value of 0.
>>>   */
>>>  DelayedControls::DelayedControls(V4L2Device *device,
>>> -				 const std::unordered_map<uint32_t, unsigned int> &delays)
>>> +				 const std::unordered_map<uint32_t, ControlParams> &controlParams)
>>>  	: device_(device), maxDelay_(0)
>>>  {
>>>  	const ControlInfoMap &controls = device_->controls();
>>> @@ -57,12 +61,12 @@ DelayedControls::DelayedControls(V4L2Device *device,
>>>  	 * 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);
>>> +	for (auto const &param : controlParams) {
>>> +		auto it = controls.find(param.first);
>>>  		if (it == controls.end()) {
>>>  			LOG(DelayedControls, Error)
>>>  				<< "Delay request for control id "
>>> -				<< utils::hex(delay.first)
>>> +				<< utils::hex(param.first)
>>>  				<< " but control is not exposed by device "
>>>  				<< device_->deviceNode();
>>>  			continue;
>>> @@ -70,13 +74,14 @@ DelayedControls::DelayedControls(V4L2Device *device,
>>>  
>>>  		const ControlId *id = it->first;
>>>  
>>> -		delays_[id] = delay.second;
>>> +		controlParams_[id] = param.second;
>>>  
>>>  		LOG(DelayedControls, Debug)
>>> -			<< "Set a delay of " << delays_[id]
>>> +			<< "Set a delay of " << controlParams_[id].delay
>>> +			<< " and priority write flag " << controlParams_[id].priorityWrite
>>>  			<< " for " << id->name();
>>>  
>>> -		maxDelay_ = std::max(maxDelay_, delays_[id]);
>>> +		maxDelay_ = std::max(maxDelay_, controlParams_[id].delay);
>>>  	}
>>>  
>>>  	reset();
>>> @@ -97,8 +102,8 @@ void DelayedControls::reset()
>>>  
>>>  	/* Retrieve control as reported by the device. */
>>>  	std::vector<uint32_t> ids;
>>> -	for (auto const &delay : delays_)
>>> -		ids.push_back(delay.first->id());
>>> +	for (auto const &param : controlParams_)
>>> +		ids.push_back(param.first->id());
>>>  
>>>  	ControlList controls = device_->getControls(ids);
>>>  
>>> @@ -140,7 +145,7 @@ bool DelayedControls::push(const ControlList &controls)
>>>  
>>>  		const ControlId *id = it->second;
>>>  
>>> -		if (delays_.find(id) == delays_.end())
>>> +		if (controlParams_.find(id) == controlParams_.end())
>>>  			return false;
>>>  
>>>  		Info &info = values_[id][queueCount_];
>>> @@ -220,12 +225,27 @@ void DelayedControls::applyControls(uint32_t sequence)
>>>  	ControlList out(device_->controls());
>>>  	for (const auto &ctrl : values_) {
>>>  		const ControlId *id = ctrl.first;
>>> -		unsigned int delayDiff = maxDelay_ - delays_[id];
>>> +		unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;
>>>  		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
>>>  		const Info &info = ctrl.second[index];
>>>  
>>>  		if (info.updated) {
>>> -			out.set(id->id(), info);
>>> +			if (controlParams_[id].priorityWrite) {
>>> +				/*
>>> +				 * This control must be written now, it could
>>> +				 * affect validity of the other controls.
>>> +				 */
>>> +				ControlList priority(device_->controls());
>>> +				priority.set(id->id(), info);
>>> +				device_->setControls(&priority);
>>> +			} else {
>>> +				/*
>>> +				 * Batch up the list of controls and write them
>>> +				 * at the end of the function.
>>> +				 */
>>> +				out.set(id->id(), info);
>>> +			}
>>> +
>>>  			LOG(DelayedControls, Debug)
>>>  				<< "Setting " << id->name()
>>>  				<< " to " << info.toString()
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index 3e6b88af4188..ac92f066a07e 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -963,14 +963,14 @@ int PipelineHandlerIPU3::registerCameras()
>>>  		 * a sensor database. For now use generic values taken from
>>>  		 * the Raspberry Pi and listed as 'generic values'.
>>>  		 */
>>> -		std::unordered_map<uint32_t, unsigned int> delays = {
>>> -			{ V4L2_CID_ANALOGUE_GAIN, 1 },
>>> -			{ V4L2_CID_EXPOSURE, 2 },
>>> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
>>> +			{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
>>> +			{ V4L2_CID_EXPOSURE, { 2, false } },
>>>  		};
>>>  
>>>  		data->delayedCtrls_ =
>>>  			std::make_unique<DelayedControls>(cio2->sensor()->device(),
>>> -							  delays);
>>> +							  params);
>>>  		data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
>>>  						 &DelayedControls::applyControls);
>>>  
>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> index a60415d93705..ba74ace183db 100644
>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>>> @@ -1244,16 +1244,15 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>>>  	if (result.params & ipa::RPi::ConfigSensorParams) {
>>>  		/*
>>>  		 * Setup our delayed control writer with the sensor default
>>> -		 * gain and exposure delays.
>>> +		 * gain and exposure delays. Mark VBLANK for priority write.
>>>  		 */
>>> -		std::unordered_map<uint32_t, unsigned int> delays = {
>>> -			{ V4L2_CID_ANALOGUE_GAIN, result.sensorConfig.gainDelay },
>>> -			{ V4L2_CID_EXPOSURE, result.sensorConfig.exposureDelay },
>>> -			{ V4L2_CID_VBLANK, result.sensorConfig.vblank }
>>> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
>>> +			{ V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },
>>> +			{ V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },
>>> +			{ V4L2_CID_VBLANK, { result.sensorConfig.vblank, true } }

Applying has a merge conflict on master here.

Setting this as
    { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }



>>>  		};
>>>  
>>> -		delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);
>>> -
>>> +		delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), params);
>>>  		sensorMetadata_ = result.sensorConfig.sensorMetadata;
>>>  	}
>>>  
>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> index a794501a9c8d..17c0f9751cd3 100644
>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> @@ -938,14 +938,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>>>  	 * a sensor database. For now use generic values taken from
>>>  	 * the Raspberry Pi and listed as generic values.
>>>  	 */
>>> -	std::unordered_map<uint32_t, unsigned int> delays = {
>>> -		{ V4L2_CID_ANALOGUE_GAIN, 1 },
>>> -		{ V4L2_CID_EXPOSURE, 2 },
>>> +	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
>>> +		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
>>> +		{ V4L2_CID_EXPOSURE, { 2, false } },
>>>  	};
>>>  
>>>  	data->delayedCtrls_ =
>>>  		std::make_unique<DelayedControls>(data->sensor_->device(),
>>> -						  delays);
>>> +						  params);
>>>  	isp_->frameStart.connect(data->delayedCtrls_.get(),
>>>  				 &DelayedControls::applyControls);
>>>  
>>> diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
>>> index 50169b12e566..3855eb18ecd4 100644
>>> --- a/test/delayed_contols.cpp
>>> +++ b/test/delayed_contols.cpp
>>> @@ -72,8 +72,8 @@ protected:
>>>  
>>>  	int singleControlNoDelay()
>>>  	{
>>> -		std::unordered_map<uint32_t, unsigned int> delays = {
>>> -			{ V4L2_CID_BRIGHTNESS, 0 },
>>> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
>>> +			{ V4L2_CID_BRIGHTNESS, { 0, false } },
>>>  		};
>>>  		std::unique_ptr<DelayedControls> delayed =
>>>  			std::make_unique<DelayedControls>(dev_.get(), delays);
>>> @@ -109,8 +109,8 @@ protected:
>>>  
>>>  	int singleControlWithDelay()
>>>  	{
>>> -		std::unordered_map<uint32_t, unsigned int> delays = {
>>> -			{ V4L2_CID_BRIGHTNESS, 1 },
>>> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
>>> +			{ V4L2_CID_BRIGHTNESS, { 1, false } },
>>>  		};
>>>  		std::unique_ptr<DelayedControls> delayed =
>>>  			std::make_unique<DelayedControls>(dev_.get(), delays);
>>> @@ -150,9 +150,9 @@ protected:
>>>  
>>>  	int dualControlsWithDelay(uint32_t startOffset)
>>>  	{
>>> -		std::unordered_map<uint32_t, unsigned int> delays = {
>>> -			{ V4L2_CID_BRIGHTNESS, 1 },
>>> -			{ V4L2_CID_CONTRAST, 2 },
>>> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
>>> +			{ V4L2_CID_BRIGHTNESS, { 1, false } },
>>> +			{ V4L2_CID_CONTRAST, { 2, false } },
>>>  		};
>>>  		std::unique_ptr<DelayedControls> delayed =
>>>  			std::make_unique<DelayedControls>(dev_.get(), delays);
>>> @@ -197,9 +197,9 @@ protected:
>>>  
>>>  	int dualControlsMultiQueue()
>>>  	{
>>> -		std::unordered_map<uint32_t, unsigned int> delays = {
>>> -			{ V4L2_CID_BRIGHTNESS, 1 },
>>> -			{ V4L2_CID_CONTRAST, 2 },
>>> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
>>> +			{ V4L2_CID_BRIGHTNESS, { 1, false } },
>>> +			{ V4L2_CID_CONTRAST, { 2, false } }
>>>  		};
>>>  		std::unique_ptr<DelayedControls> delayed =
>>>  			std::make_unique<DelayedControls>(dev_.get(), delays);
>>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
>

Hi Kieran,

Thank you for your review feedback.

On Fri, 12 Mar 2021 at 14:00, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

>
>
> On 12/03/2021 13:25, Kieran Bingham wrote:
> > Hi Naush,
> >
> > On 09/03/2021 09:54, Jean-Michel Hautbois wrote:
> >> Hi Naushir,
> >>
> >> Thanks for the patch !
> >>
> >> On 04/03/2021 09:17, Naushir Patuck wrote:
> >>> If an exposure time change adjusts the vblanking limits, and we set
> both
> >>> VBLANK and EXPOSURE controls through the VIDIOC_S_EXT_CTRLS ioctl, the
> >>> latter may fail if the value is outside of the limits calculated by the
> >>> old VBLANK value. This is a limitation in V4L2 and cannot be fixed by
> >>> setting VBLANK before EXPOSURE in a single VIDIOC_S_EXT_CTRLS ioctl.
> >>>
> >>> The workaround here is to have the DelayedControls object mark the
> >>> VBLANK control as "priority write", which then write VBLANK separately
> >>> from (and ahead of) any other controls. This way, the sensor driver
> will
> >>> update the EXPOSURE control with new limits before the new values is
> >>> presented, and will thus be seen as valid.
> >>>
> >>> To support this, a new struct DelayedControls::ControlParams is used in
> >>> the constructor to provide the control delay value as well as the
> >>> priority write flag.
> >>>
> >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> >>> Tested-by: David Plowman <david.plowman@raspberrypi.com>
> >>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> >> Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >>
> >>> ---
> >>>  include/libcamera/internal/delayed_controls.h |  9 +++-
> >>>  src/libcamera/delayed_controls.cpp            | 54 +++++++++++++------
> >>>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  8 +--
> >>>  .../pipeline/raspberrypi/raspberrypi.cpp      | 13 +++--
> >>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  8 +--
> >>>  test/delayed_contols.cpp                      | 20 +++----
> >>>  6 files changed, 68 insertions(+), 44 deletions(-)
> >>>
> >>> diff --git a/include/libcamera/internal/delayed_controls.h
> b/include/libcamera/internal/delayed_controls.h
> >>> index dc447a882514..564d9f2e2440 100644
> >>> --- a/include/libcamera/internal/delayed_controls.h
> >>> +++ b/include/libcamera/internal/delayed_controls.h
> >>> @@ -19,8 +19,13 @@ class V4L2Device;
> >>>  class DelayedControls
> >>>  {
> >>>  public:
> >>> +   struct ControlParams {
> >>> +           unsigned int delay;
> >>> +           bool priorityWrite;
> >>> +   };
> >>> +
> >>>     DelayedControls(V4L2Device *device,
> >>> -                   const std::unordered_map<uint32_t, unsigned int>
> &delays);
> >>> +                   const std::unordered_map<uint32_t, ControlParams>
> &controlParams);
> >>>
> >>>     void reset();
> >>>
> >>> @@ -64,7 +69,7 @@ private:
> >>>
> >>>     V4L2Device *device_;
> >>>     /* \todo Evaluate if we should index on ControlId * or unsigned
> int */
> >>> -   std::unordered_map<const ControlId *, unsigned int> delays_;
> >>> +   std::unordered_map<const ControlId *, ControlParams>
> controlParams_;
> >>>     unsigned int maxDelay_;
> >>>
> >>>     bool running_;
> >>> diff --git a/src/libcamera/delayed_controls.cpp
> b/src/libcamera/delayed_controls.cpp
> >>> index ab1d40057c5f..3ed1dfebd035 100644
> >>> --- a/src/libcamera/delayed_controls.cpp
> >>> +++ b/src/libcamera/delayed_controls.cpp
> >>> @@ -40,15 +40,19 @@ LOG_DEFINE_CATEGORY(DelayedControls)
> >>>  /**
> >>>   * \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)
> >>> + * \param[in] controlParams Map of the numerical V4L2 control ids to
> their
> >>> + * associated control parameters.
> >>>   *
> >>> - * 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.
> >>> + * The control parameters comprise of delays (in frames) and a
> priority write
> >>> + * flag. If this flag is set, the relevant control is written
> separately from,
> >>> + * ahead of the reset of the batched controls.
> >
> > minor:
> >
> > 'and ahead of' ?
> >
> > could be fixed while applying if there's nothing else major:
>
>
> Could you confirm if this is correct to say 'and ahead of the reset of'
> or if it was supposed to be 'and ahead of the rest of' ...
>

'separately from, and ahead of the rest of` should be the correct term here
:)

Happy for you to fix while applying if it is convenient for you.

Regards,
Naush



>
>
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> >
> >
> >>> + *
> >>> + * Only controls specified in \a controlParams 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 controlParams map with a delay
> value of 0.
> >>>   */
> >>>  DelayedControls::DelayedControls(V4L2Device *device,
> >>> -                            const std::unordered_map<uint32_t,
> unsigned int> &delays)
> >>> +                            const std::unordered_map<uint32_t,
> ControlParams> &controlParams)
> >>>     : device_(device), maxDelay_(0)
> >>>  {
> >>>     const ControlInfoMap &controls = device_->controls();
> >>> @@ -57,12 +61,12 @@ DelayedControls::DelayedControls(V4L2Device
> *device,
> >>>      * 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);
> >>> +   for (auto const &param : controlParams) {
> >>> +           auto it = controls.find(param.first);
> >>>             if (it == controls.end()) {
> >>>                     LOG(DelayedControls, Error)
> >>>                             << "Delay request for control id "
> >>> -                           << utils::hex(delay.first)
> >>> +                           << utils::hex(param.first)
> >>>                             << " but control is not exposed by device "
> >>>                             << device_->deviceNode();
> >>>                     continue;
> >>> @@ -70,13 +74,14 @@ DelayedControls::DelayedControls(V4L2Device
> *device,
> >>>
> >>>             const ControlId *id = it->first;
> >>>
> >>> -           delays_[id] = delay.second;
> >>> +           controlParams_[id] = param.second;
> >>>
> >>>             LOG(DelayedControls, Debug)
> >>> -                   << "Set a delay of " << delays_[id]
> >>> +                   << "Set a delay of " << controlParams_[id].delay
> >>> +                   << " and priority write flag " <<
> controlParams_[id].priorityWrite
> >>>                     << " for " << id->name();
> >>>
> >>> -           maxDelay_ = std::max(maxDelay_, delays_[id]);
> >>> +           maxDelay_ = std::max(maxDelay_, controlParams_[id].delay);
> >>>     }
> >>>
> >>>     reset();
> >>> @@ -97,8 +102,8 @@ void DelayedControls::reset()
> >>>
> >>>     /* Retrieve control as reported by the device. */
> >>>     std::vector<uint32_t> ids;
> >>> -   for (auto const &delay : delays_)
> >>> -           ids.push_back(delay.first->id());
> >>> +   for (auto const &param : controlParams_)
> >>> +           ids.push_back(param.first->id());
> >>>
> >>>     ControlList controls = device_->getControls(ids);
> >>>
> >>> @@ -140,7 +145,7 @@ bool DelayedControls::push(const ControlList
> &controls)
> >>>
> >>>             const ControlId *id = it->second;
> >>>
> >>> -           if (delays_.find(id) == delays_.end())
> >>> +           if (controlParams_.find(id) == controlParams_.end())
> >>>                     return false;
> >>>
> >>>             Info &info = values_[id][queueCount_];
> >>> @@ -220,12 +225,27 @@ void DelayedControls::applyControls(uint32_t
> sequence)
> >>>     ControlList out(device_->controls());
> >>>     for (const auto &ctrl : values_) {
> >>>             const ControlId *id = ctrl.first;
> >>> -           unsigned int delayDiff = maxDelay_ - delays_[id];
> >>> +           unsigned int delayDiff = maxDelay_ -
> controlParams_[id].delay;
> >>>             unsigned int index = std::max<int>(0, writeCount_ -
> delayDiff);
> >>>             const Info &info = ctrl.second[index];
> >>>
> >>>             if (info.updated) {
> >>> -                   out.set(id->id(), info);
> >>> +                   if (controlParams_[id].priorityWrite) {
> >>> +                           /*
> >>> +                            * This control must be written now, it
> could
> >>> +                            * affect validity of the other controls.
> >>> +                            */
> >>> +                           ControlList priority(device_->controls());
> >>> +                           priority.set(id->id(), info);
> >>> +                           device_->setControls(&priority);
> >>> +                   } else {
> >>> +                           /*
> >>> +                            * Batch up the list of controls and write
> them
> >>> +                            * at the end of the function.
> >>> +                            */
> >>> +                           out.set(id->id(), info);
> >>> +                   }
> >>> +
> >>>                     LOG(DelayedControls, Debug)
> >>>                             << "Setting " << id->name()
> >>>                             << " to " << info.toString()
> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> index 3e6b88af4188..ac92f066a07e 100644
> >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >>> @@ -963,14 +963,14 @@ int PipelineHandlerIPU3::registerCameras()
> >>>              * a sensor database. For now use generic values taken from
> >>>              * the Raspberry Pi and listed as 'generic values'.
> >>>              */
> >>> -           std::unordered_map<uint32_t, unsigned int> delays = {
> >>> -                   { V4L2_CID_ANALOGUE_GAIN, 1 },
> >>> -                   { V4L2_CID_EXPOSURE, 2 },
> >>> +           std::unordered_map<uint32_t,
> DelayedControls::ControlParams> params = {
> >>> +                   { V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> >>> +                   { V4L2_CID_EXPOSURE, { 2, false } },
> >>>             };
> >>>
> >>>             data->delayedCtrls_ =
> >>>
>  std::make_unique<DelayedControls>(cio2->sensor()->device(),
> >>> -                                                     delays);
> >>> +                                                     params);
> >>>             data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
> >>>
> &DelayedControls::applyControls);
> >>>
> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> index a60415d93705..ba74ace183db 100644
> >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> >>> @@ -1244,16 +1244,15 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >>>     if (result.params & ipa::RPi::ConfigSensorParams) {
> >>>             /*
> >>>              * Setup our delayed control writer with the sensor default
> >>> -            * gain and exposure delays.
> >>> +            * gain and exposure delays. Mark VBLANK for priority
> write.
> >>>              */
> >>> -           std::unordered_map<uint32_t, unsigned int> delays = {
> >>> -                   { V4L2_CID_ANALOGUE_GAIN,
> result.sensorConfig.gainDelay },
> >>> -                   { V4L2_CID_EXPOSURE,
> result.sensorConfig.exposureDelay },
> >>> -                   { V4L2_CID_VBLANK, result.sensorConfig.vblank }
> >>> +           std::unordered_map<uint32_t,
> DelayedControls::ControlParams> params = {
> >>> +                   { V4L2_CID_ANALOGUE_GAIN, {
> result.sensorConfig.gainDelay, false } },
> >>> +                   { V4L2_CID_EXPOSURE, {
> result.sensorConfig.exposureDelay, false } },
> >>> +                   { V4L2_CID_VBLANK, { result.sensorConfig.vblank,
> true } }
>
> Applying has a merge conflict on master here.
>
> Setting this as
>     { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }
>
>
>
> >>>             };
> >>>
> >>> -           delayedCtrls_ =
> std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);
> >>> -
> >>> +           delayedCtrls_ =
> std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), params);
> >>>             sensorMetadata_ = result.sensorConfig.sensorMetadata;
> >>>     }
> >>>
> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> index a794501a9c8d..17c0f9751cd3 100644
> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> @@ -938,14 +938,14 @@ int
> PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >>>      * a sensor database. For now use generic values taken from
> >>>      * the Raspberry Pi and listed as generic values.
> >>>      */
> >>> -   std::unordered_map<uint32_t, unsigned int> delays = {
> >>> -           { V4L2_CID_ANALOGUE_GAIN, 1 },
> >>> -           { V4L2_CID_EXPOSURE, 2 },
> >>> +   std::unordered_map<uint32_t, DelayedControls::ControlParams>
> params = {
> >>> +           { V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> >>> +           { V4L2_CID_EXPOSURE, { 2, false } },
> >>>     };
> >>>
> >>>     data->delayedCtrls_ =
> >>>             std::make_unique<DelayedControls>(data->sensor_->device(),
> >>> -                                             delays);
> >>> +                                             params);
> >>>     isp_->frameStart.connect(data->delayedCtrls_.get(),
> >>>                              &DelayedControls::applyControls);
> >>>
> >>> diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
> >>> index 50169b12e566..3855eb18ecd4 100644
> >>> --- a/test/delayed_contols.cpp
> >>> +++ b/test/delayed_contols.cpp
> >>> @@ -72,8 +72,8 @@ protected:
> >>>
> >>>     int singleControlNoDelay()
> >>>     {
> >>> -           std::unordered_map<uint32_t, unsigned int> delays = {
> >>> -                   { V4L2_CID_BRIGHTNESS, 0 },
> >>> +           std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> >>> +                   { V4L2_CID_BRIGHTNESS, { 0, false } },
> >>>             };
> >>>             std::unique_ptr<DelayedControls> delayed =
> >>>                     std::make_unique<DelayedControls>(dev_.get(),
> delays);
> >>> @@ -109,8 +109,8 @@ protected:
> >>>
> >>>     int singleControlWithDelay()
> >>>     {
> >>> -           std::unordered_map<uint32_t, unsigned int> delays = {
> >>> -                   { V4L2_CID_BRIGHTNESS, 1 },
> >>> +           std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> >>> +                   { V4L2_CID_BRIGHTNESS, { 1, false } },
> >>>             };
> >>>             std::unique_ptr<DelayedControls> delayed =
> >>>                     std::make_unique<DelayedControls>(dev_.get(),
> delays);
> >>> @@ -150,9 +150,9 @@ protected:
> >>>
> >>>     int dualControlsWithDelay(uint32_t startOffset)
> >>>     {
> >>> -           std::unordered_map<uint32_t, unsigned int> delays = {
> >>> -                   { V4L2_CID_BRIGHTNESS, 1 },
> >>> -                   { V4L2_CID_CONTRAST, 2 },
> >>> +           std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> >>> +                   { V4L2_CID_BRIGHTNESS, { 1, false } },
> >>> +                   { V4L2_CID_CONTRAST, { 2, false } },
> >>>             };
> >>>             std::unique_ptr<DelayedControls> delayed =
> >>>                     std::make_unique<DelayedControls>(dev_.get(),
> delays);
> >>> @@ -197,9 +197,9 @@ protected:
> >>>
> >>>     int dualControlsMultiQueue()
> >>>     {
> >>> -           std::unordered_map<uint32_t, unsigned int> delays = {
> >>> -                   { V4L2_CID_BRIGHTNESS, 1 },
> >>> -                   { V4L2_CID_CONTRAST, 2 },
> >>> +           std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> >>> +                   { V4L2_CID_BRIGHTNESS, { 1, false } },
> >>> +                   { V4L2_CID_CONTRAST, { 2, false } }
> >>>             };
> >>>             std::unique_ptr<DelayedControls> delayed =
> >>>                     std::make_unique<DelayedControls>(dev_.get(),
> delays);
> >>>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
> >>
> >
>
> --
> Regards
> --
> Kieran
>

On 12/03/2021 14:04, Naushir Patuck wrote:
> Hi Kieran,
> 
> Thank you for your review feedback.
> 
>     b/src/libcamera/delayed_controls.cpp
>     >>> index ab1d40057c5f..3ed1dfebd035 100644
>     >>> --- a/src/libcamera/delayed_controls.cpp
>     >>> +++ b/src/libcamera/delayed_controls.cpp
>     >>> @@ -40,15 +40,19 @@ LOG_DEFINE_CATEGORY(DelayedControls)
>     >>>  /**
>     >>>   * \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)
>     >>> + * \param[in] controlParams Map of the numerical V4L2 control
>     ids to their
>     >>> + * associated control parameters.
>     >>>   *
>     >>> - * 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.
>     >>> + * The control parameters comprise of delays (in frames) and a
>     priority write
>     >>> + * flag. If this flag is set, the relevant control is written
>     separately from,
>     >>> + * ahead of the reset of the batched controls.
>     >
>     > minor:
>     >
>     > 'and ahead of' ?
>     >
>     > could be fixed while applying if there's nothing else major:
> 
> 
>     Could you confirm if this is correct to say 'and ahead of the reset of'
>     or if it was supposed to be 'and ahead of the rest of' ...
> 
> 
> 'separately from, and ahead of the rest of` should be the correct term
> here :)
> 

Aha great.

> Happy for you to fix while applying if it is convenient for you.

Done and pushed.

--
Thanks

Kieran

Hi Naush,

Thank you for the patch.

On Thu, Mar 04, 2021 at 08:17:22AM +0000, Naushir Patuck wrote:
> If an exposure time change adjusts the vblanking limits, and we set both
> VBLANK and EXPOSURE controls through the VIDIOC_S_EXT_CTRLS ioctl, the
> latter may fail if the value is outside of the limits calculated by the
> old VBLANK value. This is a limitation in V4L2 and cannot be fixed by
> setting VBLANK before EXPOSURE in a single VIDIOC_S_EXT_CTRLS ioctl.
> 
> The workaround here is to have the DelayedControls object mark the
> VBLANK control as "priority write", which then write VBLANK separately
> from (and ahead of) any other controls. This way, the sensor driver will
> update the EXPOSURE control with new limits before the new values is
> presented, and will thus be seen as valid.
> 
> To support this, a new struct DelayedControls::ControlParams is used in
> the constructor to provide the control delay value as well as the
> priority write flag.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Tested-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  include/libcamera/internal/delayed_controls.h |  9 +++-
>  src/libcamera/delayed_controls.cpp            | 54 +++++++++++++------
>  src/libcamera/pipeline/ipu3/ipu3.cpp          |  8 +--
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 13 +++--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  8 +--
>  test/delayed_contols.cpp                      | 20 +++----
>  6 files changed, 68 insertions(+), 44 deletions(-)
> 
> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
> index dc447a882514..564d9f2e2440 100644
> --- a/include/libcamera/internal/delayed_controls.h
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -19,8 +19,13 @@ class V4L2Device;
>  class DelayedControls
>  {
>  public:
> +	struct ControlParams {
> +		unsigned int delay;
> +		bool priorityWrite;
> +	};

I've only noticed now that the series has been merged, this structure
isn't documented:

[333/464] Generating doxygen with a custom command
include/libcamera/internal/delayed_controls.h:22: warning: Compound libcamera::DelayedControls::ControlParams is not documented.
include/libcamera/internal/delayed_controls.h:23: warning: Member delay (variable) of struct libcamera::DelayedControls::ControlParams is not documented.
include/libcamera/internal/delayed_controls.h:24: warning: Member priorityWrite (variable) of struct libcamera::DelayedControls::ControlParams is not documented.

Could you send a follow-up patch to address this ?

I'm also curious if I'm the only one compiling the documentation :-)

> +
>  	DelayedControls(V4L2Device *device,
> -			const std::unordered_map<uint32_t, unsigned int> &delays);
> +			const std::unordered_map<uint32_t, ControlParams> &controlParams);
>  
>  	void reset();
>  
> @@ -64,7 +69,7 @@ private:
>  
>  	V4L2Device *device_;
>  	/* \todo Evaluate if we should index on ControlId * or unsigned int */
> -	std::unordered_map<const ControlId *, unsigned int> delays_;
> +	std::unordered_map<const ControlId *, ControlParams> controlParams_;
>  	unsigned int maxDelay_;
>  
>  	bool running_;
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index ab1d40057c5f..3ed1dfebd035 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -40,15 +40,19 @@ LOG_DEFINE_CATEGORY(DelayedControls)
>  /**
>   * \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)
> + * \param[in] controlParams Map of the numerical V4L2 control ids to their
> + * associated control parameters.
>   *
> - * 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.
> + * The control parameters comprise of delays (in frames) and a priority write
> + * flag. If this flag is set, the relevant control is written separately from,
> + * ahead of the reset of the batched controls.
> + *
> + * Only controls specified in \a controlParams 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 controlParams map with a delay value of 0.
>   */
>  DelayedControls::DelayedControls(V4L2Device *device,
> -				 const std::unordered_map<uint32_t, unsigned int> &delays)
> +				 const std::unordered_map<uint32_t, ControlParams> &controlParams)
>  	: device_(device), maxDelay_(0)
>  {
>  	const ControlInfoMap &controls = device_->controls();
> @@ -57,12 +61,12 @@ DelayedControls::DelayedControls(V4L2Device *device,
>  	 * 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);
> +	for (auto const &param : controlParams) {
> +		auto it = controls.find(param.first);
>  		if (it == controls.end()) {
>  			LOG(DelayedControls, Error)
>  				<< "Delay request for control id "
> -				<< utils::hex(delay.first)
> +				<< utils::hex(param.first)
>  				<< " but control is not exposed by device "
>  				<< device_->deviceNode();
>  			continue;
> @@ -70,13 +74,14 @@ DelayedControls::DelayedControls(V4L2Device *device,
>  
>  		const ControlId *id = it->first;
>  
> -		delays_[id] = delay.second;
> +		controlParams_[id] = param.second;
>  
>  		LOG(DelayedControls, Debug)
> -			<< "Set a delay of " << delays_[id]
> +			<< "Set a delay of " << controlParams_[id].delay
> +			<< " and priority write flag " << controlParams_[id].priorityWrite
>  			<< " for " << id->name();
>  
> -		maxDelay_ = std::max(maxDelay_, delays_[id]);
> +		maxDelay_ = std::max(maxDelay_, controlParams_[id].delay);
>  	}
>  
>  	reset();
> @@ -97,8 +102,8 @@ void DelayedControls::reset()
>  
>  	/* Retrieve control as reported by the device. */
>  	std::vector<uint32_t> ids;
> -	for (auto const &delay : delays_)
> -		ids.push_back(delay.first->id());
> +	for (auto const &param : controlParams_)
> +		ids.push_back(param.first->id());
>  
>  	ControlList controls = device_->getControls(ids);
>  
> @@ -140,7 +145,7 @@ bool DelayedControls::push(const ControlList &controls)
>  
>  		const ControlId *id = it->second;
>  
> -		if (delays_.find(id) == delays_.end())
> +		if (controlParams_.find(id) == controlParams_.end())
>  			return false;
>  
>  		Info &info = values_[id][queueCount_];
> @@ -220,12 +225,27 @@ void DelayedControls::applyControls(uint32_t sequence)
>  	ControlList out(device_->controls());
>  	for (const auto &ctrl : values_) {
>  		const ControlId *id = ctrl.first;
> -		unsigned int delayDiff = maxDelay_ - delays_[id];
> +		unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;
>  		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
>  		const Info &info = ctrl.second[index];
>  
>  		if (info.updated) {
> -			out.set(id->id(), info);
> +			if (controlParams_[id].priorityWrite) {
> +				/*
> +				 * This control must be written now, it could
> +				 * affect validity of the other controls.
> +				 */
> +				ControlList priority(device_->controls());
> +				priority.set(id->id(), info);
> +				device_->setControls(&priority);
> +			} else {
> +				/*
> +				 * Batch up the list of controls and write them
> +				 * at the end of the function.
> +				 */
> +				out.set(id->id(), info);
> +			}
> +
>  			LOG(DelayedControls, Debug)
>  				<< "Setting " << id->name()
>  				<< " to " << info.toString()
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 3e6b88af4188..ac92f066a07e 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -963,14 +963,14 @@ int PipelineHandlerIPU3::registerCameras()
>  		 * a sensor database. For now use generic values taken from
>  		 * the Raspberry Pi and listed as 'generic values'.
>  		 */
> -		std::unordered_map<uint32_t, unsigned int> delays = {
> -			{ V4L2_CID_ANALOGUE_GAIN, 1 },
> -			{ V4L2_CID_EXPOSURE, 2 },
> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> +			{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> +			{ V4L2_CID_EXPOSURE, { 2, false } },
>  		};
>  
>  		data->delayedCtrls_ =
>  			std::make_unique<DelayedControls>(cio2->sensor()->device(),
> -							  delays);
> +							  params);
>  		data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
>  						 &DelayedControls::applyControls);
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index a60415d93705..ba74ace183db 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1244,16 +1244,15 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  	if (result.params & ipa::RPi::ConfigSensorParams) {
>  		/*
>  		 * Setup our delayed control writer with the sensor default
> -		 * gain and exposure delays.
> +		 * gain and exposure delays. Mark VBLANK for priority write.
>  		 */
> -		std::unordered_map<uint32_t, unsigned int> delays = {
> -			{ V4L2_CID_ANALOGUE_GAIN, result.sensorConfig.gainDelay },
> -			{ V4L2_CID_EXPOSURE, result.sensorConfig.exposureDelay },
> -			{ V4L2_CID_VBLANK, result.sensorConfig.vblank }
> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> +			{ V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },
> +			{ V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },
> +			{ V4L2_CID_VBLANK, { result.sensorConfig.vblank, true } }
>  		};
>  
> -		delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);
> -
> +		delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), params);
>  		sensorMetadata_ = result.sensorConfig.sensorMetadata;
>  	}
>  
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index a794501a9c8d..17c0f9751cd3 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -938,14 +938,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	 * a sensor database. For now use generic values taken from
>  	 * the Raspberry Pi and listed as generic values.
>  	 */
> -	std::unordered_map<uint32_t, unsigned int> delays = {
> -		{ V4L2_CID_ANALOGUE_GAIN, 1 },
> -		{ V4L2_CID_EXPOSURE, 2 },
> +	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> +		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> +		{ V4L2_CID_EXPOSURE, { 2, false } },
>  	};
>  
>  	data->delayedCtrls_ =
>  		std::make_unique<DelayedControls>(data->sensor_->device(),
> -						  delays);
> +						  params);
>  	isp_->frameStart.connect(data->delayedCtrls_.get(),
>  				 &DelayedControls::applyControls);
>  
> diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
> index 50169b12e566..3855eb18ecd4 100644
> --- a/test/delayed_contols.cpp
> +++ b/test/delayed_contols.cpp
> @@ -72,8 +72,8 @@ protected:
>  
>  	int singleControlNoDelay()
>  	{
> -		std::unordered_map<uint32_t, unsigned int> delays = {
> -			{ V4L2_CID_BRIGHTNESS, 0 },
> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
> +			{ V4L2_CID_BRIGHTNESS, { 0, false } },
>  		};
>  		std::unique_ptr<DelayedControls> delayed =
>  			std::make_unique<DelayedControls>(dev_.get(), delays);
> @@ -109,8 +109,8 @@ protected:
>  
>  	int singleControlWithDelay()
>  	{
> -		std::unordered_map<uint32_t, unsigned int> delays = {
> -			{ V4L2_CID_BRIGHTNESS, 1 },
> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
> +			{ V4L2_CID_BRIGHTNESS, { 1, false } },
>  		};
>  		std::unique_ptr<DelayedControls> delayed =
>  			std::make_unique<DelayedControls>(dev_.get(), delays);
> @@ -150,9 +150,9 @@ protected:
>  
>  	int dualControlsWithDelay(uint32_t startOffset)
>  	{
> -		std::unordered_map<uint32_t, unsigned int> delays = {
> -			{ V4L2_CID_BRIGHTNESS, 1 },
> -			{ V4L2_CID_CONTRAST, 2 },
> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
> +			{ V4L2_CID_BRIGHTNESS, { 1, false } },
> +			{ V4L2_CID_CONTRAST, { 2, false } },
>  		};
>  		std::unique_ptr<DelayedControls> delayed =
>  			std::make_unique<DelayedControls>(dev_.get(), delays);
> @@ -197,9 +197,9 @@ protected:
>  
>  	int dualControlsMultiQueue()
>  	{
> -		std::unordered_map<uint32_t, unsigned int> delays = {
> -			{ V4L2_CID_BRIGHTNESS, 1 },
> -			{ V4L2_CID_CONTRAST, 2 },
> +		std::unordered_map<uint32_t, DelayedControls::ControlParams> delays = {
> +			{ V4L2_CID_BRIGHTNESS, { 1, false } },
> +			{ V4L2_CID_CONTRAST, { 2, false } }
>  		};
>  		std::unique_ptr<DelayedControls> delayed =
>  			std::make_unique<DelayedControls>(dev_.get(), delays);

Hi Laurent,

On Fri, 12 Mar 2021, 8:57 pm Laurent Pinchart, <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Thu, Mar 04, 2021 at 08:17:22AM +0000, Naushir Patuck wrote:
> > If an exposure time change adjusts the vblanking limits, and we set both
> > VBLANK and EXPOSURE controls through the VIDIOC_S_EXT_CTRLS ioctl, the
> > latter may fail if the value is outside of the limits calculated by the
> > old VBLANK value. This is a limitation in V4L2 and cannot be fixed by
> > setting VBLANK before EXPOSURE in a single VIDIOC_S_EXT_CTRLS ioctl.
> >
> > The workaround here is to have the DelayedControls object mark the
> > VBLANK control as "priority write", which then write VBLANK separately
> > from (and ahead of) any other controls. This way, the sensor driver will
> > update the EXPOSURE control with new limits before the new values is
> > presented, and will thus be seen as valid.
> >
> > To support this, a new struct DelayedControls::ControlParams is used in
> > the constructor to provide the control delay value as well as the
> > priority write flag.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Tested-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  include/libcamera/internal/delayed_controls.h |  9 +++-
> >  src/libcamera/delayed_controls.cpp            | 54 +++++++++++++------
> >  src/libcamera/pipeline/ipu3/ipu3.cpp          |  8 +--
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 13 +++--
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  8 +--
> >  test/delayed_contols.cpp                      | 20 +++----
> >  6 files changed, 68 insertions(+), 44 deletions(-)
> >
> > diff --git a/include/libcamera/internal/delayed_controls.h
> b/include/libcamera/internal/delayed_controls.h
> > index dc447a882514..564d9f2e2440 100644
> > --- a/include/libcamera/internal/delayed_controls.h
> > +++ b/include/libcamera/internal/delayed_controls.h
> > @@ -19,8 +19,13 @@ class V4L2Device;
> >  class DelayedControls
> >  {
> >  public:
> > +     struct ControlParams {
> > +             unsigned int delay;
> > +             bool priorityWrite;
> > +     };
>
> I've only noticed now that the series has been merged, this structure
> isn't documented:
>
> [333/464] Generating doxygen with a custom command
> include/libcamera/internal/delayed_controls.h:22: warning: Compound
> libcamera::DelayedControls::ControlParams is not documented.
> include/libcamera/internal/delayed_controls.h:23: warning: Member delay
> (variable) of struct libcamera::DelayedControls::ControlParams is not
> documented.
> include/libcamera/internal/delayed_controls.h:24: warning: Member
> priorityWrite (variable) of struct
> libcamera::DelayedControls::ControlParams is not documented.
>
> Could you send a follow-up patch to address this ?
>

Apologies for missing the doc update. I will post a patch with it soon.


> I'm also curious if I'm the only one compiling the documentation :-)
>

My documentation compilation is likely disabled, but I will switch it back
on now :-)

Regards,
Naush


> > +
> >       DelayedControls(V4L2Device *device,
> > -                     const std::unordered_map<uint32_t, unsigned int>
> &delays);
> > +                     const std::unordered_map<uint32_t, ControlParams>
> &controlParams);
> >
> >       void reset();
> >
> > @@ -64,7 +69,7 @@ private:
> >
> >       V4L2Device *device_;
> >       /* \todo Evaluate if we should index on ControlId * or unsigned
> int */
> > -     std::unordered_map<const ControlId *, unsigned int> delays_;
> > +     std::unordered_map<const ControlId *, ControlParams>
> controlParams_;
> >       unsigned int maxDelay_;
> >
> >       bool running_;
> > diff --git a/src/libcamera/delayed_controls.cpp
> b/src/libcamera/delayed_controls.cpp
> > index ab1d40057c5f..3ed1dfebd035 100644
> > --- a/src/libcamera/delayed_controls.cpp
> > +++ b/src/libcamera/delayed_controls.cpp
> > @@ -40,15 +40,19 @@ LOG_DEFINE_CATEGORY(DelayedControls)
> >  /**
> >   * \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)
> > + * \param[in] controlParams Map of the numerical V4L2 control ids to
> their
> > + * associated control parameters.
> >   *
> > - * 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.
> > + * The control parameters comprise of delays (in frames) and a priority
> write
> > + * flag. If this flag is set, the relevant control is written
> separately from,
> > + * ahead of the reset of the batched controls.
> > + *
> > + * Only controls specified in \a controlParams 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 controlParams map with a delay
> value of 0.
> >   */
> >  DelayedControls::DelayedControls(V4L2Device *device,
> > -                              const std::unordered_map<uint32_t,
> unsigned int> &delays)
> > +                              const std::unordered_map<uint32_t,
> ControlParams> &controlParams)
> >       : device_(device), maxDelay_(0)
> >  {
> >       const ControlInfoMap &controls = device_->controls();
> > @@ -57,12 +61,12 @@ DelayedControls::DelayedControls(V4L2Device *device,
> >        * 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);
> > +     for (auto const &param : controlParams) {
> > +             auto it = controls.find(param.first);
> >               if (it == controls.end()) {
> >                       LOG(DelayedControls, Error)
> >                               << "Delay request for control id "
> > -                             << utils::hex(delay.first)
> > +                             << utils::hex(param.first)
> >                               << " but control is not exposed by device "
> >                               << device_->deviceNode();
> >                       continue;
> > @@ -70,13 +74,14 @@ DelayedControls::DelayedControls(V4L2Device *device,
> >
> >               const ControlId *id = it->first;
> >
> > -             delays_[id] = delay.second;
> > +             controlParams_[id] = param.second;
> >
> >               LOG(DelayedControls, Debug)
> > -                     << "Set a delay of " << delays_[id]
> > +                     << "Set a delay of " << controlParams_[id].delay
> > +                     << " and priority write flag " <<
> controlParams_[id].priorityWrite
> >                       << " for " << id->name();
> >
> > -             maxDelay_ = std::max(maxDelay_, delays_[id]);
> > +             maxDelay_ = std::max(maxDelay_, controlParams_[id].delay);
> >       }
> >
> >       reset();
> > @@ -97,8 +102,8 @@ void DelayedControls::reset()
> >
> >       /* Retrieve control as reported by the device. */
> >       std::vector<uint32_t> ids;
> > -     for (auto const &delay : delays_)
> > -             ids.push_back(delay.first->id());
> > +     for (auto const &param : controlParams_)
> > +             ids.push_back(param.first->id());
> >
> >       ControlList controls = device_->getControls(ids);
> >
> > @@ -140,7 +145,7 @@ bool DelayedControls::push(const ControlList
> &controls)
> >
> >               const ControlId *id = it->second;
> >
> > -             if (delays_.find(id) == delays_.end())
> > +             if (controlParams_.find(id) == controlParams_.end())
> >                       return false;
> >
> >               Info &info = values_[id][queueCount_];
> > @@ -220,12 +225,27 @@ void DelayedControls::applyControls(uint32_t
> sequence)
> >       ControlList out(device_->controls());
> >       for (const auto &ctrl : values_) {
> >               const ControlId *id = ctrl.first;
> > -             unsigned int delayDiff = maxDelay_ - delays_[id];
> > +             unsigned int delayDiff = maxDelay_ -
> controlParams_[id].delay;
> >               unsigned int index = std::max<int>(0, writeCount_ -
> delayDiff);
> >               const Info &info = ctrl.second[index];
> >
> >               if (info.updated) {
> > -                     out.set(id->id(), info);
> > +                     if (controlParams_[id].priorityWrite) {
> > +                             /*
> > +                              * This control must be written now, it
> could
> > +                              * affect validity of the other controls.
> > +                              */
> > +                             ControlList priority(device_->controls());
> > +                             priority.set(id->id(), info);
> > +                             device_->setControls(&priority);
> > +                     } else {
> > +                             /*
> > +                              * Batch up the list of controls and write
> them
> > +                              * at the end of the function.
> > +                              */
> > +                             out.set(id->id(), info);
> > +                     }
> > +
> >                       LOG(DelayedControls, Debug)
> >                               << "Setting " << id->name()
> >                               << " to " << info.toString()
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 3e6b88af4188..ac92f066a07e 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -963,14 +963,14 @@ int PipelineHandlerIPU3::registerCameras()
> >                * a sensor database. For now use generic values taken from
> >                * the Raspberry Pi and listed as 'generic values'.
> >                */
> > -             std::unordered_map<uint32_t, unsigned int> delays = {
> > -                     { V4L2_CID_ANALOGUE_GAIN, 1 },
> > -                     { V4L2_CID_EXPOSURE, 2 },
> > +             std::unordered_map<uint32_t,
> DelayedControls::ControlParams> params = {
> > +                     { V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> > +                     { V4L2_CID_EXPOSURE, { 2, false } },
> >               };
> >
> >               data->delayedCtrls_ =
> >
>  std::make_unique<DelayedControls>(cio2->sensor()->device(),
> > -                                                       delays);
> > +                                                       params);
> >               data->cio2_.frameStart().connect(data->delayedCtrls_.get(),
> >
> &DelayedControls::applyControls);
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index a60415d93705..ba74ace183db 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -1244,16 +1244,15 @@ int RPiCameraData::configureIPA(const
> CameraConfiguration *config)
> >       if (result.params & ipa::RPi::ConfigSensorParams) {
> >               /*
> >                * Setup our delayed control writer with the sensor default
> > -              * gain and exposure delays.
> > +              * gain and exposure delays. Mark VBLANK for priority
> write.
> >                */
> > -             std::unordered_map<uint32_t, unsigned int> delays = {
> > -                     { V4L2_CID_ANALOGUE_GAIN,
> result.sensorConfig.gainDelay },
> > -                     { V4L2_CID_EXPOSURE,
> result.sensorConfig.exposureDelay },
> > -                     { V4L2_CID_VBLANK, result.sensorConfig.vblank }
> > +             std::unordered_map<uint32_t,
> DelayedControls::ControlParams> params = {
> > +                     { V4L2_CID_ANALOGUE_GAIN, {
> result.sensorConfig.gainDelay, false } },
> > +                     { V4L2_CID_EXPOSURE, {
> result.sensorConfig.exposureDelay, false } },
> > +                     { V4L2_CID_VBLANK, { result.sensorConfig.vblank,
> true } }
> >               };
> >
> > -             delayedCtrls_ =
> std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays);
> > -
> > +             delayedCtrls_ =
> std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), params);
> >               sensorMetadata_ = result.sensorConfig.sensorMetadata;
> >       }
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index a794501a9c8d..17c0f9751cd3 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -938,14 +938,14 @@ int
> PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >        * a sensor database. For now use generic values taken from
> >        * the Raspberry Pi and listed as generic values.
> >        */
> > -     std::unordered_map<uint32_t, unsigned int> delays = {
> > -             { V4L2_CID_ANALOGUE_GAIN, 1 },
> > -             { V4L2_CID_EXPOSURE, 2 },
> > +     std::unordered_map<uint32_t, DelayedControls::ControlParams>
> params = {
> > +             { V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> > +             { V4L2_CID_EXPOSURE, { 2, false } },
> >       };
> >
> >       data->delayedCtrls_ =
> >               std::make_unique<DelayedControls>(data->sensor_->device(),
> > -                                               delays);
> > +                                               params);
> >       isp_->frameStart.connect(data->delayedCtrls_.get(),
> >                                &DelayedControls::applyControls);
> >
> > diff --git a/test/delayed_contols.cpp b/test/delayed_contols.cpp
> > index 50169b12e566..3855eb18ecd4 100644
> > --- a/test/delayed_contols.cpp
> > +++ b/test/delayed_contols.cpp
> > @@ -72,8 +72,8 @@ protected:
> >
> >       int singleControlNoDelay()
> >       {
> > -             std::unordered_map<uint32_t, unsigned int> delays = {
> > -                     { V4L2_CID_BRIGHTNESS, 0 },
> > +             std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> > +                     { V4L2_CID_BRIGHTNESS, { 0, false } },
> >               };
> >               std::unique_ptr<DelayedControls> delayed =
> >                       std::make_unique<DelayedControls>(dev_.get(),
> delays);
> > @@ -109,8 +109,8 @@ protected:
> >
> >       int singleControlWithDelay()
> >       {
> > -             std::unordered_map<uint32_t, unsigned int> delays = {
> > -                     { V4L2_CID_BRIGHTNESS, 1 },
> > +             std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> > +                     { V4L2_CID_BRIGHTNESS, { 1, false } },
> >               };
> >               std::unique_ptr<DelayedControls> delayed =
> >                       std::make_unique<DelayedControls>(dev_.get(),
> delays);
> > @@ -150,9 +150,9 @@ protected:
> >
> >       int dualControlsWithDelay(uint32_t startOffset)
> >       {
> > -             std::unordered_map<uint32_t, unsigned int> delays = {
> > -                     { V4L2_CID_BRIGHTNESS, 1 },
> > -                     { V4L2_CID_CONTRAST, 2 },
> > +             std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> > +                     { V4L2_CID_BRIGHTNESS, { 1, false } },
> > +                     { V4L2_CID_CONTRAST, { 2, false } },
> >               };
> >               std::unique_ptr<DelayedControls> delayed =
> >                       std::make_unique<DelayedControls>(dev_.get(),
> delays);
> > @@ -197,9 +197,9 @@ protected:
> >
> >       int dualControlsMultiQueue()
> >       {
> > -             std::unordered_map<uint32_t, unsigned int> delays = {
> > -                     { V4L2_CID_BRIGHTNESS, 1 },
> > -                     { V4L2_CID_CONTRAST, 2 },
> > +             std::unordered_map<uint32_t,
> DelayedControls::ControlParams> delays = {
> > +                     { V4L2_CID_BRIGHTNESS, { 1, false } },
> > +                     { V4L2_CID_CONTRAST, { 2, false } }
> >               };
> >               std::unique_ptr<DelayedControls> delayed =
> >                       std::make_unique<DelayedControls>(dev_.get(),
> delays);
>
> --
> Regards,
>
> Laurent Pinchart
>