[libcamera-devel] [PATCH 1/7] ipa: raspberrypi: AWB: Remove unnecessary locking for AWB settings
David Plowman
david.plowman at raspberrypi.com
Sun Feb 7 19:04:11 CET 2021
Hi Laurent
Thanks for the review and for applying the patches!
On Sun, 7 Feb 2021 at 14:01, Laurent Pinchart
<laurent.pinchart at 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 at 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 at 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
More information about the libcamera-devel
mailing list