[{"id":15036,"web_url":"https://patchwork.libcamera.org/comment/15036/","msgid":"","date":"2021-02-07T14:04:30","subject":"Re: [libcamera-devel] [PATCH 2/7] ipa: raspberrypi: AWB: Improve\n\tlocking.","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\ns/.$// on the subject line.\n\nOn Thu, Feb 04, 2021 at 09:34:52AM +0000, David Plowman wrote:\n> Fix a couple of places where notify_one() was called with the lock\n> held. Also restartAsync doesn't need the lock for its entire duration.\n> \n> This change exactly matches commit db552b where we do the same for\n> ALSC (the asynchronous thread arrangement there is identical).\n\nWe usually mention commits with a 12 characters of their ID, as 6\ncharacters is quite collision-prone. It's a rule that we should probably\ndocument somewhere (it originates from the kernel). It's also customary\nto quote the commit subject. This would thus become\n\nThis change exactly matches commit db552b0b925a (\"libcamera: ipa:\nraspberrypi: ALSC: Improve locking in a few places\") where we do the\nsame for ALSC (the asynchronous thread arrangement there is identical).\n\n> Signed-off-by: David Plowman \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/ipa/raspberrypi/controller/rpi/awb.cpp | 10 ++++++----\n> 1 file changed, 6 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/awb.cpp b/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> index dabab726..791b5108 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/awb.cpp\n> @@ -136,8 +136,8 @@ Awb::~Awb()\n> \t{\n> \t\tstd::lock_guard lock(mutex_);\n> \t\tasync_abort_ = true;\n> -\t\tasync_signal_.notify_one();\n> \t}\n> +\tasync_signal_.notify_one();\n> \tasync_thread_.join();\n> }\n> \n> @@ -239,11 +239,14 @@ void Awb::restartAsync(StatisticsPtr &stats, double lux)\n> \t\t\t: (mode_ == nullptr ? config_.default_mode : mode_);\n> \tlux_ = lux;\n> \tframe_phase_ = 0;\n> -\tasync_start_ = true;\n> \tasync_started_ = true;\n> \tsize_t len = mode_name_.copy(async_results_.mode,\n> \t\t\t\t sizeof(async_results_.mode) - 1);\n> \tasync_results_.mode[len] = '\\0';\n> +\t{\n> +\t\tstd::lock_guard lock(mutex_);\n> +\t\tasync_start_ = true;\n> +\t}\n> \tasync_signal_.notify_one();\n> }\n> \n> @@ -297,7 +300,6 @@ void Awb::Process(StatisticsPtr &stats, Metadata *image_metadata)\n> \t\t\tLOG(RPiAwb, Debug) << \"No lux metadata found\";\n> \t\tLOG(RPiAwb, Debug) << \"Awb lux value is \" << lux_status.lux;\n> \n> -\t\tstd::unique_lock lock(mutex_);\n> \t\tif (async_started_ == false)\n> \t\t\trestartAsync(stats, lux_status.lux);\n> \t}\n> @@ -319,8 +321,8 @@ void Awb::asyncFunc()\n> \t\t{\n> \t\t\tstd::lock_guard lock(mutex_);\n> \t\t\tasync_finished_ = true;\n> -\t\t\tsync_signal_.notify_one();\n> \t\t}\n> +\t\tsync_signal_.notify_one();\n> \t}\n> }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BF379BD162\n\tfor ;\n\tSun, 7 Feb 2021 14:04:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3436E601B3;\n\tSun, 7 Feb 2021 15:04:56 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93EDC601AB\n\tfor ;\n\tSun, 7 Feb 2021 15:04:54 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ECE89F3;\n\tSun, 7 Feb 2021 15:04:53 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ncdM2npg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612706694;\n\tbh=IMHB71xnYSAZz3DFMen3RPKFGHCZEQATvgxk4KaGrgc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ncdM2npgIG1CE8KGCxs+1Z6waLa9nqBfyAaGBFU5YUKlt5EJiPrbpO4OcqfMKc8VG\n\t+Jq0hNPC5SgRCyh9mrm+zXzEZS8gcU3RmLT5iR7E+PND+L2w/PyzK6x7/KtiL+nXaz\n\tE8awGndyHA9pk6SLp0nPzf9p51BFIR5BG1uMYwKE=","Date":"Sun, 7 Feb 2021 16:04:30 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210204093457.6879-1-david.plowman@raspberrypi.com>\n\t<20210204093457.6879-3-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210204093457.6879-3-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 2/7] ipa: raspberrypi: AWB: Improve\n\tlocking.","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]