[libcamera-devel,1/7] ipa: raspberrypi: AWB: Remove unnecessary locking for AWB settings
diff mbox series

Message ID 20210204093457.6879-2-david.plowman@raspberrypi.com
State Accepted
Headers show
Series
  • Raspberry Pi IPA maintenance
Related show

Commit Message

David Plowman Feb. 4, 2021, 9:34 a.m. UTC 
AWB settings get updated synchronously with the main thread, so the
settings_mutex_ and associated locking can be removed.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/rpi/awb.cpp | 18 +++++-------------
 src/ipa/raspberrypi/controller/rpi/awb.hpp |  4 +---
 2 files changed, 6 insertions(+), 16 deletions(-)

Comments

Laurent Pinchart Feb. 7, 2021, 2 p.m. UTC | #1 
Hi David,

Thank you for the patch.

On Thu, Feb 04, 2021 at 09:34:51AM +0000, David Plowman wrote:
> AWB settings get updated synchronously with the main thread, so the
> settings_mutex_ and associated locking can be removed.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/rpi/awb.cpp | 18 +++++-------------
>  src/ipa/raspberrypi/controller/rpi/awb.hpp |  4 +---
>  2 files changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> index 62337b13..dabab726 100644
> --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> @@ -185,13 +185,11 @@ unsigned int Awb::GetConvergenceFrames() const
>  
>  void Awb::SetMode(std::string const &mode_name)
>  {
> -	std::unique_lock<std::mutex> lock(settings_mutex_);
>  	mode_name_ = mode_name;
>  }
>  
>  void Awb::SetManualGains(double manual_r, double manual_b)
>  {
> -	std::unique_lock<std::mutex> lock(settings_mutex_);
>  	// If any of these are 0.0, we swich back to auto.
>  	manual_r_ = manual_r;
>  	manual_b_ = manual_b;

This patch looks good overall, but I think we have a race condition here
as manual_r_ and manual_b_ are accessed in the AWB thread, without any
lock. It's not a new issue introduced by this patch, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

but it should be fixed.

> @@ -229,14 +227,13 @@ void Awb::fetchAsyncResults()
>  	sync_results_ = async_results_;
>  }
>  
> -void Awb::restartAsync(StatisticsPtr &stats, std::string const &mode_name,
> -		       double lux)
> +void Awb::restartAsync(StatisticsPtr &stats, double lux)
>  {
>  	LOG(RPiAwb, Debug) << "Starting AWB calculation";
>  	// this makes a new reference which belongs to the asynchronous thread
>  	statistics_ = stats;
>  	// store the mode as it could technically change
> -	auto m = config_.modes.find(mode_name);
> +	auto m = config_.modes.find(mode_name_);
>  	mode_ = m != config_.modes.end()
>  			? &m->second
>  			: (mode_ == nullptr ? config_.default_mode : mode_);
> @@ -244,8 +241,8 @@ void Awb::restartAsync(StatisticsPtr &stats, std::string const &mode_name,
>  	frame_phase_ = 0;
>  	async_start_ = true;
>  	async_started_ = true;
> -	size_t len = mode_name.copy(async_results_.mode,
> -				    sizeof(async_results_.mode) - 1);
> +	size_t len = mode_name_.copy(async_results_.mode,
> +				     sizeof(async_results_.mode) - 1);
>  	async_results_.mode[len] = '\0';
>  	async_signal_.notify_one();
>  }
> @@ -294,11 +291,6 @@ void Awb::Process(StatisticsPtr &stats, Metadata *image_metadata)
>  	if (frame_phase_ >= (int)config_.frame_period ||
>  	    frame_count2_ < (int)config_.startup_frames) {
>  		// Update any settings and any image metadata that we need.
> -		std::string mode_name;
> -		{
> -			std::unique_lock<std::mutex> lock(settings_mutex_);
> -			mode_name = mode_name_;
> -		}
>  		struct LuxStatus lux_status = {};
>  		lux_status.lux = 400; // in case no metadata
>  		if (image_metadata->Get("lux.status", lux_status) != 0)
> @@ -307,7 +299,7 @@ void Awb::Process(StatisticsPtr &stats, Metadata *image_metadata)
>  
>  		std::unique_lock<std::mutex> lock(mutex_);
>  		if (async_started_ == false)
> -			restartAsync(stats, mode_name, lux_status.lux);
> +			restartAsync(stats, lux_status.lux);
>  	}
>  }
>  
> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.hpp b/src/ipa/raspberrypi/controller/rpi/awb.hpp
> index 83b81498..1b39ab4b 100644
> --- a/src/ipa/raspberrypi/controller/rpi/awb.hpp
> +++ b/src/ipa/raspberrypi/controller/rpi/awb.hpp
> @@ -134,11 +134,9 @@ private:
>  	AwbStatus sync_results_;
>  	AwbStatus prev_sync_results_;
>  	std::string mode_name_;
> -	std::mutex settings_mutex_;
>  	// The following are for the asynchronous thread to use, though the main
>  	// thread can set/reset them if the async thread is known to be idle:
> -	void restartAsync(StatisticsPtr &stats, std::string const &mode_name,
> -			  double lux);
> +	void restartAsync(StatisticsPtr &stats, double lux);
>  	// copy out the results from the async thread so that it can be restarted
>  	void fetchAsyncResults();
>  	StatisticsPtr statistics_;
David Plowman Feb. 7, 2021, 6:04 p.m. UTC | #2 
Hi Laurent

Thanks for the review and for applying the patches!

On Sun, 7 Feb 2021 at 14:01, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Thu, Feb 04, 2021 at 09:34:51AM +0000, David Plowman wrote:
> > AWB settings get updated synchronously with the main thread, so the
> > settings_mutex_ and associated locking can be removed.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/controller/rpi/awb.cpp | 18 +++++-------------
> >  src/ipa/raspberrypi/controller/rpi/awb.hpp |  4 +---
> >  2 files changed, 6 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > index 62337b13..dabab726 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> > @@ -185,13 +185,11 @@ unsigned int Awb::GetConvergenceFrames() const
> >
> >  void Awb::SetMode(std::string const &mode_name)
> >  {
> > -     std::unique_lock<std::mutex> lock(settings_mutex_);
> >       mode_name_ = mode_name;
> >  }
> >
> >  void Awb::SetManualGains(double manual_r, double manual_b)
> >  {
> > -     std::unique_lock<std::mutex> lock(settings_mutex_);
> >       // If any of these are 0.0, we swich back to auto.
> >       manual_r_ = manual_r;
> >       manual_b_ = manual_b;
>
> This patch looks good overall, but I think we have a race condition here
> as manual_r_ and manual_b_ are accessed in the AWB thread, without any
> lock. It's not a new issue introduced by this patch, so

Indeed, I wonder why we'd never paid attention to that. I'll submit a
patch for that shortly as I have some more (slightly more significant)
maintenance patches still in the works!

Thanks again and best regards
David

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> but it should be fixed.
>
> > @@ -229,14 +227,13 @@ void Awb::fetchAsyncResults()
> >       sync_results_ = async_results_;
> >  }
> >
> > -void Awb::restartAsync(StatisticsPtr &stats, std::string const &mode_name,
> > -                    double lux)
> > +void Awb::restartAsync(StatisticsPtr &stats, double lux)
> >  {
> >       LOG(RPiAwb, Debug) << "Starting AWB calculation";
> >       // this makes a new reference which belongs to the asynchronous thread
> >       statistics_ = stats;
> >       // store the mode as it could technically change
> > -     auto m = config_.modes.find(mode_name);
> > +     auto m = config_.modes.find(mode_name_);
> >       mode_ = m != config_.modes.end()
> >                       ? &m->second
> >                       : (mode_ == nullptr ? config_.default_mode : mode_);
> > @@ -244,8 +241,8 @@ void Awb::restartAsync(StatisticsPtr &stats, std::string const &mode_name,
> >       frame_phase_ = 0;
> >       async_start_ = true;
> >       async_started_ = true;
> > -     size_t len = mode_name.copy(async_results_.mode,
> > -                                 sizeof(async_results_.mode) - 1);
> > +     size_t len = mode_name_.copy(async_results_.mode,
> > +                                  sizeof(async_results_.mode) - 1);
> >       async_results_.mode[len] = '\0';
> >       async_signal_.notify_one();
> >  }
> > @@ -294,11 +291,6 @@ void Awb::Process(StatisticsPtr &stats, Metadata *image_metadata)
> >       if (frame_phase_ >= (int)config_.frame_period ||
> >           frame_count2_ < (int)config_.startup_frames) {
> >               // Update any settings and any image metadata that we need.
> > -             std::string mode_name;
> > -             {
> > -                     std::unique_lock<std::mutex> lock(settings_mutex_);
> > -                     mode_name = mode_name_;
> > -             }
> >               struct LuxStatus lux_status = {};
> >               lux_status.lux = 400; // in case no metadata
> >               if (image_metadata->Get("lux.status", lux_status) != 0)
> > @@ -307,7 +299,7 @@ void Awb::Process(StatisticsPtr &stats, Metadata *image_metadata)
> >
> >               std::unique_lock<std::mutex> lock(mutex_);
> >               if (async_started_ == false)
> > -                     restartAsync(stats, mode_name, lux_status.lux);
> > +                     restartAsync(stats, lux_status.lux);
> >       }
> >  }
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/awb.hpp b/src/ipa/raspberrypi/controller/rpi/awb.hpp
> > index 83b81498..1b39ab4b 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/awb.hpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/awb.hpp
> > @@ -134,11 +134,9 @@ private:
> >       AwbStatus sync_results_;
> >       AwbStatus prev_sync_results_;
> >       std::string mode_name_;
> > -     std::mutex settings_mutex_;
> >       // The following are for the asynchronous thread to use, though the main
> >       // thread can set/reset them if the async thread is known to be idle:
> > -     void restartAsync(StatisticsPtr &stats, std::string const &mode_name,
> > -                       double lux);
> > +     void restartAsync(StatisticsPtr &stats, double lux);
> >       // copy out the results from the async thread so that it can be restarted
> >       void fetchAsyncResults();
> >       StatisticsPtr statistics_;
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp
index 62337b13..dabab726 100644
--- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
+++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
@@ -185,13 +185,11 @@  unsigned int Awb::GetConvergenceFrames() const
 
 void Awb::SetMode(std::string const &mode_name)
 {
-	std::unique_lock<std::mutex> lock(settings_mutex_);
 	mode_name_ = mode_name;
 }
 
 void Awb::SetManualGains(double manual_r, double manual_b)
 {
-	std::unique_lock<std::mutex> lock(settings_mutex_);
 	// If any of these are 0.0, we swich back to auto.
 	manual_r_ = manual_r;
 	manual_b_ = manual_b;
@@ -229,14 +227,13 @@  void Awb::fetchAsyncResults()
 	sync_results_ = async_results_;
 }
 
-void Awb::restartAsync(StatisticsPtr &stats, std::string const &mode_name,
-		       double lux)
+void Awb::restartAsync(StatisticsPtr &stats, double lux)
 {
 	LOG(RPiAwb, Debug) << "Starting AWB calculation";
 	// this makes a new reference which belongs to the asynchronous thread
 	statistics_ = stats;
 	// store the mode as it could technically change
-	auto m = config_.modes.find(mode_name);
+	auto m = config_.modes.find(mode_name_);
 	mode_ = m != config_.modes.end()
 			? &m->second
 			: (mode_ == nullptr ? config_.default_mode : mode_);
@@ -244,8 +241,8 @@  void Awb::restartAsync(StatisticsPtr &stats, std::string const &mode_name,
 	frame_phase_ = 0;
 	async_start_ = true;
 	async_started_ = true;
-	size_t len = mode_name.copy(async_results_.mode,
-				    sizeof(async_results_.mode) - 1);
+	size_t len = mode_name_.copy(async_results_.mode,
+				     sizeof(async_results_.mode) - 1);
 	async_results_.mode[len] = '\0';
 	async_signal_.notify_one();
 }
@@ -294,11 +291,6 @@  void Awb::Process(StatisticsPtr &stats, Metadata *image_metadata)
 	if (frame_phase_ >= (int)config_.frame_period ||
 	    frame_count2_ < (int)config_.startup_frames) {
 		// Update any settings and any image metadata that we need.
-		std::string mode_name;
-		{
-			std::unique_lock<std::mutex> lock(settings_mutex_);
-			mode_name = mode_name_;
-		}
 		struct LuxStatus lux_status = {};
 		lux_status.lux = 400; // in case no metadata
 		if (image_metadata->Get("lux.status", lux_status) != 0)
@@ -307,7 +299,7 @@  void Awb::Process(StatisticsPtr &stats, Metadata *image_metadata)
 
 		std::unique_lock<std::mutex> lock(mutex_);
 		if (async_started_ == false)
-			restartAsync(stats, mode_name, lux_status.lux);
+			restartAsync(stats, lux_status.lux);
 	}
 }
 
diff --git a/src/ipa/raspberrypi/controller/rpi/awb.hpp b/src/ipa/raspberrypi/controller/rpi/awb.hpp
index 83b81498..1b39ab4b 100644
--- a/src/ipa/raspberrypi/controller/rpi/awb.hpp
+++ b/src/ipa/raspberrypi/controller/rpi/awb.hpp
@@ -134,11 +134,9 @@  private:
 	AwbStatus sync_results_;
 	AwbStatus prev_sync_results_;
 	std::string mode_name_;
-	std::mutex settings_mutex_;
 	// The following are for the asynchronous thread to use, though the main
 	// thread can set/reset them if the async thread is known to be idle:
-	void restartAsync(StatisticsPtr &stats, std::string const &mode_name,
-			  double lux);
+	void restartAsync(StatisticsPtr &stats, double lux);
 	// copy out the results from the async thread so that it can be restarted
 	void fetchAsyncResults();
 	StatisticsPtr statistics_;