[libcamera-devel,v4,3/7] libcamera: delayed_controls: Remove unneeded write when starting up
diff mbox series

Message ID 20210304081728.1058394-4-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 
On DelayedControls::reset(), the values retrieved from the sensor device
were added to the queues with the updated flag set to true. This would
cause the helper to write out the value to the device again on the first
DelayedControls::applyControls() call. This is unnecessary, as the
controls written are identical to what is stored in the device driver.

Fix this by explicitly setting the update flag to false in
DelayedControls::reset() when adding the controls to the queue.

Additionally, use the Info() constructor when adding items to the queue
for consistency.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Fixes: 3d4b7b005911 ("libcamera: delayed_controls: Add helper for controls that apply with a delay")
Tested-by: David Plowman <david.plowman@raspberrypi.com>
---
 include/libcamera/internal/delayed_controls.h | 4 ++--
 src/libcamera/delayed_controls.cpp            | 9 ++++++---
 2 files changed, 8 insertions(+), 5 deletions(-)

Comments

Paul Elder March 9, 2021, 9:50 a.m. UTC | #1 
Hi Naush,

On Thu, Mar 04, 2021 at 08:17:24AM +0000, Naushir Patuck wrote:
> On DelayedControls::reset(), the values retrieved from the sensor device
> were added to the queues with the updated flag set to true. This would
> cause the helper to write out the value to the device again on the first
> DelayedControls::applyControls() call. This is unnecessary, as the
> controls written are identical to what is stored in the device driver.
> 
> Fix this by explicitly setting the update flag to false in
> DelayedControls::reset() when adding the controls to the queue.
> 
> Additionally, use the Info() constructor when adding items to the queue
> for consistency.
> 
> Signed-off-by: Naushir Patuck <naush@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>

> ---
>  include/libcamera/internal/delayed_controls.h | 4 ++--
>  src/libcamera/delayed_controls.cpp            | 9 ++++++---
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
> index 564d9f2e2440..2a6a912bde10 100644
> --- a/include/libcamera/internal/delayed_controls.h
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -43,8 +43,8 @@ private:
>  		{
>  		}
>  
> -		Info(const ControlValue &v)
> -			: ControlValue(v), updated(true)
> +		Info(const ControlValue &v, bool updated_ = true)
> +			: ControlValue(v), updated(updated_)
>  		{
>  		}
>  
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index 3ed1dfebd035..f6d761d1cc41 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -111,7 +111,11 @@ void DelayedControls::reset()
>  	values_.clear();
>  	for (const auto &ctrl : controls) {
>  		const ControlId *id = device_->controls().idmap().at(ctrl.first);
> -		values_[id][0] = Info(ctrl.second);
> +		/*
> +		 * Do not mark this control value as updated, it does not need
> +		 * to be written to to device on startup.
> +		 */
> +		values_[id][0] = Info(ctrl.second, false);
>  	}
>  }
>  
> @@ -150,8 +154,7 @@ bool DelayedControls::push(const ControlList &controls)
>  
>  		Info &info = values_[id][queueCount_];
>  
> -		info = control.second;
> -		info.updated = true;
> +		info = Info(control.second);
>  
>  		LOG(DelayedControls, Debug)
>  			<< "Queuing " << id->name()
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jean-Michel Hautbois March 9, 2021, 9:55 a.m. UTC | #2 
On 09/03/2021 10:50, paul.elder@ideasonboard.com wrote:
> Hi Naush,
> 
> On Thu, Mar 04, 2021 at 08:17:24AM +0000, Naushir Patuck wrote:
>> On DelayedControls::reset(), the values retrieved from the sensor device
>> were added to the queues with the updated flag set to true. This would
>> cause the helper to write out the value to the device again on the first
>> DelayedControls::applyControls() call. This is unnecessary, as the
>> controls written are identical to what is stored in the device driver.
>>
>> Fix this by explicitly setting the update flag to false in
>> DelayedControls::reset() when adding the controls to the queue.
>>
>> Additionally, use the Info() constructor when adding items to the queue
>> for consistency.
>>
>> Signed-off-by: Naushir Patuck <naush@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>
> 
>> ---
>>  include/libcamera/internal/delayed_controls.h | 4 ++--
>>  src/libcamera/delayed_controls.cpp            | 9 ++++++---
>>  2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
>> index 564d9f2e2440..2a6a912bde10 100644
>> --- a/include/libcamera/internal/delayed_controls.h
>> +++ b/include/libcamera/internal/delayed_controls.h
>> @@ -43,8 +43,8 @@ private:
>>  		{
>>  		}
>>  
>> -		Info(const ControlValue &v)
>> -			: ControlValue(v), updated(true)
>> +		Info(const ControlValue &v, bool updated_ = true)
>> +			: ControlValue(v), updated(updated_)
>>  		{
>>  		}
>>  
>> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
>> index 3ed1dfebd035..f6d761d1cc41 100644
>> --- a/src/libcamera/delayed_controls.cpp
>> +++ b/src/libcamera/delayed_controls.cpp
>> @@ -111,7 +111,11 @@ void DelayedControls::reset()
>>  	values_.clear();
>>  	for (const auto &ctrl : controls) {
>>  		const ControlId *id = device_->controls().idmap().at(ctrl.first);
>> -		values_[id][0] = Info(ctrl.second);
>> +		/*
>> +		 * Do not mark this control value as updated, it does not need
>> +		 * to be written to to device on startup.
>> +		 */
>> +		values_[id][0] = Info(ctrl.second, false);
>>  	}
>>  }
>>  
>> @@ -150,8 +154,7 @@ bool DelayedControls::push(const ControlList &controls)
>>  
>>  		Info &info = values_[id][queueCount_];
>>  
>> -		info = control.second;
>> -		info.updated = true;
>> +		info = Info(control.second);
>>  
>>  		LOG(DelayedControls, Debug)
>>  			<< "Queuing " << id->name()
>> -- 
>> 2.25.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Kieran Bingham March 12, 2021, 1:27 p.m. UTC | #3 
Hi Naush,


On 04/03/2021 08:17, Naushir Patuck wrote:
> On DelayedControls::reset(), the values retrieved from the sensor device
> were added to the queues with the updated flag set to true. This would
> cause the helper to write out the value to the device again on the first
> DelayedControls::applyControls() call. This is unnecessary, as the
> controls written are identical to what is stored in the device driver.
> 
> Fix this by explicitly setting the update flag to false in
> DelayedControls::reset() when adding the controls to the queue.
> 
> Additionally, use the Info() constructor when adding items to the queue
> for consistency.
> 
> Signed-off-by: Naushir Patuck <naush@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: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  include/libcamera/internal/delayed_controls.h | 4 ++--
>  src/libcamera/delayed_controls.cpp            | 9 ++++++---
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
> index 564d9f2e2440..2a6a912bde10 100644
> --- a/include/libcamera/internal/delayed_controls.h
> +++ b/include/libcamera/internal/delayed_controls.h
> @@ -43,8 +43,8 @@ private:
>  		{
>  		}
>  
> -		Info(const ControlValue &v)
> -			: ControlValue(v), updated(true)
> +		Info(const ControlValue &v, bool updated_ = true)
> +			: ControlValue(v), updated(updated_)
>  		{
>  		}
>  
> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> index 3ed1dfebd035..f6d761d1cc41 100644
> --- a/src/libcamera/delayed_controls.cpp
> +++ b/src/libcamera/delayed_controls.cpp
> @@ -111,7 +111,11 @@ void DelayedControls::reset()
>  	values_.clear();
>  	for (const auto &ctrl : controls) {
>  		const ControlId *id = device_->controls().idmap().at(ctrl.first);
> -		values_[id][0] = Info(ctrl.second);
> +		/*
> +		 * Do not mark this control value as updated, it does not need
> +		 * to be written to to device on startup.
> +		 */
> +		values_[id][0] = Info(ctrl.second, false);
>  	}
>  }
>  
> @@ -150,8 +154,7 @@ bool DelayedControls::push(const ControlList &controls)
>  
>  		Info &info = values_[id][queueCount_];
>  
> -		info = control.second;
> -		info.updated = true;
> +		info = Info(control.second);
>  
>  		LOG(DelayedControls, Debug)
>  			<< "Queuing " << id->name()
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/delayed_controls.h b/include/libcamera/internal/delayed_controls.h
index 564d9f2e2440..2a6a912bde10 100644
--- a/include/libcamera/internal/delayed_controls.h
+++ b/include/libcamera/internal/delayed_controls.h
@@ -43,8 +43,8 @@  private:
 		{
 		}
 
-		Info(const ControlValue &v)
-			: ControlValue(v), updated(true)
+		Info(const ControlValue &v, bool updated_ = true)
+			: ControlValue(v), updated(updated_)
 		{
 		}
 
diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
index 3ed1dfebd035..f6d761d1cc41 100644
--- a/src/libcamera/delayed_controls.cpp
+++ b/src/libcamera/delayed_controls.cpp
@@ -111,7 +111,11 @@  void DelayedControls::reset()
 	values_.clear();
 	for (const auto &ctrl : controls) {
 		const ControlId *id = device_->controls().idmap().at(ctrl.first);
-		values_[id][0] = Info(ctrl.second);
+		/*
+		 * Do not mark this control value as updated, it does not need
+		 * to be written to to device on startup.
+		 */
+		values_[id][0] = Info(ctrl.second, false);
 	}
 }
 
@@ -150,8 +154,7 @@  bool DelayedControls::push(const ControlList &controls)
 
 		Info &info = values_[id][queueCount_];
 
-		info = control.second;
-		info.updated = true;
+		info = Info(control.second);
 
 		LOG(DelayedControls, Debug)
 			<< "Queuing " << id->name()