[libcamera-devel,v4,4/7] libcamera: delayed_controls: Remove spurious no-op queued controls
diff mbox series

Message ID 20210304081728.1058394-5-naush@raspberrypi.com
State Accepted
Headers show
Series
  • DelayedControls updates and fixes
Related show

Commit Message

Naushir Patuck March 4, 2021, 8:17 a.m. UTC 
In DelayedControls::applyControls(), the controls queue would be
extended via a no-op control push to fill the intermittent slots with
unchanged control values. This is needed so that we read back unchanged
control values correctly on every frame.

However, there was one additional no-op performed on every frame that is
not required, meaning that any controls queued by the pipeline handler
would have their write delayed by one further frame. The original
StaggeredCtrl did not do this, as it only had one index to manage,
whereas DelayedControls uses two.

Remove this last no-op push so that the pipeline_handler provided
controls would be written on the very next frame if possible. As a
consequence, we must mark the control update as completed within the
DelayedControls::applyControls() loop, otherwise it might get reused
when cycling through the ring buffer.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reported-by: David Plowman <david.plowman@raspberrypi.com>
Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay")
Tested-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/libcamera/delayed_controls.cpp | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Jean-Michel Hautbois March 9, 2021, 9:56 a.m. UTC | #1 
On 04/03/2021 09:17, Naushir Patuck wrote:
> In DelayedControls::applyControls(), the controls queue would be
> extended via a no-op control push to fill the intermittent slots with
> unchanged control values. This is needed so that we read back unchanged
> control values correctly on every frame.
> 
> However, there was one additional no-op performed on every frame that is
> not required, meaning that any controls queued by the pipeline handler
> would have their write delayed by one further frame. The original
> StaggeredCtrl did not do this, as it only had one index to manage,
> whereas DelayedControls uses two.
> 
> Remove this last no-op push so that the pipeline_handler provided
> controls would be written on the very next frame if possible. As a
> consequence, we must mark the control update as completed within the
> DelayedControls::applyControls() loop, otherwise it might get reused
> when cycling through the ring buffer.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reported-by: David Plowman <david.plowman@raspberrypi.com>
> Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay")
> 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>
> ---
>  src/libcamera/delayed_controls.cpp | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index f6d761d1cc41..5a05741c0285 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -226,11 +226,11 @@ void DelayedControls::applyControls(uint32_t sequence)
>  	 * values are set in time to satisfy the sensor delay.
>  	 */
>  	ControlList out(device_->controls());
> -	for (const auto &ctrl : values_) {
> +	for (auto &ctrl : values_) {
>  		const ControlId *id = ctrl.first;
>  		unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;
>  		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
> -		const Info &info = ctrl.second[index];
> +		Info &info = ctrl.second[index];
>  
>  		if (info.updated) {
>  			if (controlParams_[id].priorityWrite) {
> @@ -253,12 +253,15 @@ void DelayedControls::applyControls(uint32_t sequence)
>  				<< "Setting " << id->name()
>  				<< " to " << info.toString()
>  				<< " at index " << index;
> +
> +			/* Done with this update, so mark as completed. */
> +			info.updated = false;
>  		}
>  	}
>  
>  	writeCount_++;
>  
> -	while (writeCount_ >= queueCount_) {
> +	while (writeCount_ > queueCount_) {
>  		LOG(DelayedControls, Debug)
>  			<< "Queue is empty, auto queue no-op.";
>  		push({});
>
Kieran Bingham March 12, 2021, 1:30 p.m. UTC | #2 
On 04/03/2021 08:17, Naushir Patuck wrote:
> In DelayedControls::applyControls(), the controls queue would be
> extended via a no-op control push to fill the intermittent slots with
> unchanged control values. This is needed so that we read back unchanged
> control values correctly on every frame.
> 
> However, there was one additional no-op performed on every frame that is
> not required, meaning that any controls queued by the pipeline handler
> would have their write delayed by one further frame. The original
> StaggeredCtrl did not do this, as it only had one index to manage,
> whereas DelayedControls uses two.
> 
> Remove this last no-op push so that the pipeline_handler provided
> controls would be written on the very next frame if possible. As a
> consequence, we must mark the control update as completed within the
> DelayedControls::applyControls() loop, otherwise it might get reused
> when cycling through the ring buffer.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reported-by: David Plowman <david.plowman@raspberrypi.com>
> Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay")
> Tested-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

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

> ---
>  src/libcamera/delayed_controls.cpp | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index f6d761d1cc41..5a05741c0285 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -226,11 +226,11 @@ void DelayedControls::applyControls(uint32_t sequence)
>  	 * values are set in time to satisfy the sensor delay.
>  	 */
>  	ControlList out(device_->controls());
> -	for (const auto &ctrl : values_) {
> +	for (auto &ctrl : values_) {
>  		const ControlId *id = ctrl.first;
>  		unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;
>  		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
> -		const Info &info = ctrl.second[index];
> +		Info &info = ctrl.second[index];
>  
>  		if (info.updated) {
>  			if (controlParams_[id].priorityWrite) {
> @@ -253,12 +253,15 @@ void DelayedControls::applyControls(uint32_t sequence)
>  				<< "Setting " << id->name()
>  				<< " to " << info.toString()
>  				<< " at index " << index;
> +
> +			/* Done with this update, so mark as completed. */
> +			info.updated = false;
>  		}
>  	}
>  
>  	writeCount_++;
>  
> -	while (writeCount_ >= queueCount_) {
> +	while (writeCount_ > queueCount_) {
>  		LOG(DelayedControls, Debug)
>  			<< "Queue is empty, auto queue no-op.";
>  		push({});
>

Patch
diff mbox series 

diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
index f6d761d1cc41..5a05741c0285 100644
--- a/src/libcamera/delayed_controls.cpp
+++ b/src/libcamera/delayed_controls.cpp
@@ -226,11 +226,11 @@  void DelayedControls::applyControls(uint32_t sequence)
 	 * values are set in time to satisfy the sensor delay.
 	 */
 	ControlList out(device_->controls());
-	for (const auto &ctrl : values_) {
+	for (auto &ctrl : values_) {
 		const ControlId *id = ctrl.first;
 		unsigned int delayDiff = maxDelay_ - controlParams_[id].delay;
 		unsigned int index = std::max<int>(0, writeCount_ - delayDiff);
-		const Info &info = ctrl.second[index];
+		Info &info = ctrl.second[index];
 
 		if (info.updated) {
 			if (controlParams_[id].priorityWrite) {
@@ -253,12 +253,15 @@  void DelayedControls::applyControls(uint32_t sequence)
 				<< "Setting " << id->name()
 				<< " to " << info.toString()
 				<< " at index " << index;
+
+			/* Done with this update, so mark as completed. */
+			info.updated = false;
 		}
 	}
 
 	writeCount_++;
 
-	while (writeCount_ >= queueCount_) {
+	while (writeCount_ > queueCount_) {
 		LOG(DelayedControls, Debug)
 			<< "Queue is empty, auto queue no-op.";
 		push({});