| Message ID | 20210204093457.6879-3-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@raspberrypi.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@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(); > } > } >
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(); } }
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). Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/raspberrypi/controller/rpi/awb.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)