[libcamera-devel] [PATCH 2/7] ipa: raspberrypi: AWB: Improve locking.

[Laurent Pinchart]

Hi David,

Thank you for the patch.

s/.$// on the subject line.

On Thu, Feb 04, 2021 at 09:34:52AM +0000, David Plowman wrote:
> Fix a couple of places where notify_one() was called with the lock
> held. Also restartAsync doesn't need the lock for its entire duration.
> 
> This change exactly matches commit db552b where we do the same for
> ALSC (the asynchronous thread arrangement there is identical).

We usually mention commits with a 12 characters of their ID, as 6
characters is quite collision-prone. It's a rule that we should probably
document somewhere (it originates from the kernel). It's also customary
to quote the commit subject. This would thus become

This change exactly matches commit db552b0b925a ("libcamera: ipa:
raspberrypi: ALSC: Improve locking in a few places") where we do the
same for ALSC (the asynchronous thread arrangement there is identical).

> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>

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

> ---
>  src/ipa/raspberrypi/controller/rpi/awb.cpp | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> index dabab726..791b5108 100644
> --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp
> @@ -136,8 +136,8 @@ Awb::~Awb()
>  	{
>  		std::lock_guard<std::mutex> lock(mutex_);
>  		async_abort_ = true;
> -		async_signal_.notify_one();
>  	}
> +	async_signal_.notify_one();
>  	async_thread_.join();
>  }
>  
> @@ -239,11 +239,14 @@ void Awb::restartAsync(StatisticsPtr &stats, double lux)
>  			: (mode_ == nullptr ? config_.default_mode : mode_);
>  	lux_ = lux;
>  	frame_phase_ = 0;
> -	async_start_ = true;
>  	async_started_ = true;
>  	size_t len = mode_name_.copy(async_results_.mode,
>  				     sizeof(async_results_.mode) - 1);
>  	async_results_.mode[len] = '\0';
> +	{
> +		std::lock_guard<std::mutex> lock(mutex_);
> +		async_start_ = true;
> +	}
>  	async_signal_.notify_one();
>  }
>  
> @@ -297,7 +300,6 @@ void Awb::Process(StatisticsPtr &stats, Metadata *image_metadata)
>  			LOG(RPiAwb, Debug) << "No lux metadata found";
>  		LOG(RPiAwb, Debug) << "Awb lux value is " << lux_status.lux;
>  
> -		std::unique_lock<std::mutex> lock(mutex_);
>  		if (async_started_ == false)
>  			restartAsync(stats, lux_status.lux);
>  	}
> @@ -319,8 +321,8 @@ void Awb::asyncFunc()
>  		{
>  			std::lock_guard<std::mutex> lock(mutex_);
>  			async_finished_ = true;
> -			sync_signal_.notify_one();
>  		}
> +		sync_signal_.notify_one();
>  	}
>  }
>  

-- 
Regards,

Laurent Pinchart