[{"id":13733,"web_url":"https://patchwork.libcamera.org/comment/13733/","msgid":"","date":"2020-11-17T10:32:57","subject":"Re: [libcamera-devel] [PATCH 02/10] libcamera: ipa: raspberrypi:\n\tagc: Remove unnecessary locking","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\n\nOn Mon, 16 Nov 2020 at 16:49, David Plowman \nwrote:\n\n> On the libcamera/VC4 platform the AGC Prepare/Process methods, and any\n> changes to the AGC settings, run synchronously - so a number of\n> mutexes and copies are unnecessary and can be removed.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 99 ++++++++--------------\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 5 +-\n> 2 files changed, 37 insertions(+), 67 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 8079345b..c4379b99 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -160,7 +160,6 @@ Agc::Agc(Controller *controller)\n> status_.total_exposure_value = 0.0;\n> status_.target_exposure_value = 0.0;\n> status_.locked = false;\n> - output_status_ = status_;\n> }\n>\n> char const *Agc::Name() const\n> @@ -185,43 +184,36 @@ void Agc::Read(boost::property_tree::ptree const\n> ¶ms)\n>\n> void Agc::SetEv(double ev)\n> {\n> - std::unique_lock lock(settings_mutex_);\n> ev_ = ev;\n> }\n>\n> void Agc::SetFlickerPeriod(double flicker_period)\n> {\n> - std::unique_lock lock(settings_mutex_);\n> flicker_period_ = flicker_period;\n> }\n>\n> void Agc::SetFixedShutter(double fixed_shutter)\n> {\n> - std::unique_lock lock(settings_mutex_);\n> fixed_shutter_ = fixed_shutter;\n> }\n>\n> void Agc::SetFixedAnalogueGain(double fixed_analogue_gain)\n> {\n> - std::unique_lock lock(settings_mutex_);\n> fixed_analogue_gain_ = fixed_analogue_gain;\n> }\n>\n> void Agc::SetMeteringMode(std::string const &metering_mode_name)\n> {\n> - std::unique_lock lock(settings_mutex_);\n> metering_mode_name_ = metering_mode_name;\n> }\n>\n> void Agc::SetExposureMode(std::string const &exposure_mode_name)\n> {\n> - std::unique_lock lock(settings_mutex_);\n> exposure_mode_name_ = exposure_mode_name;\n> }\n>\n> void Agc::SetConstraintMode(std::string const &constraint_mode_name)\n> {\n> - std::unique_lock lock(settings_mutex_);\n> constraint_mode_name_ = constraint_mode_name;\n> }\n>\n> @@ -240,14 +232,9 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode\n> const &camera_mode,\n>\n> void Agc::Prepare(Metadata *image_metadata)\n> {\n> - AgcStatus status;\n> - {\n> - std::unique_lock lock(output_mutex_);\n> - status = output_status_;\n> - }\n> int lock_count = lock_count_;\n> lock_count_ = 0;\n> - status.digital_gain = 1.0;\n> + status_.digital_gain = 1.0;\n> if (status_.total_exposure_value) {\n> // Process has run, so we have meaningful values.\n> DeviceStatus device_status;\n> @@ -255,48 +242,46 @@ void Agc::Prepare(Metadata *image_metadata)\n> double actual_exposure =\n> device_status.shutter_speed *\n>\n> device_status.analogue_gain;\n> if (actual_exposure) {\n> - status.digital_gain =\n> + status_.digital_gain =\n> status_.total_exposure_value /\n> actual_exposure;\n> LOG(RPiAgc, Debug) << \"Want total exposure\n> \" << status_.total_exposure_value;\n> // Never ask for a gain < 1.0, and also\n> impose\n> // some upper limit. Make it customisable?\n> - status.digital_gain = std::max(\n> + status_.digital_gain = std::max(\n> 1.0,\n> - std::min(status.digital_gain,\n> 4.0));\n> + std::min(status_.digital_gain,\n> 4.0));\n> LOG(RPiAgc, Debug) << \"Actual exposure \"\n> << actual_exposure;\n> - LOG(RPiAgc, Debug) << \"Use digital_gain \"\n> << status.digital_gain;\n> - LOG(RPiAgc, Debug) << \"Effective exposure\n> \" << actual_exposure * status.digital_gain;\n> + LOG(RPiAgc, Debug) << \"Use digital_gain \"\n> << status_.digital_gain;\n> + LOG(RPiAgc, Debug) << \"Effective exposure\n> \" << actual_exposure * status_.digital_gain;\n> // Decide whether AEC/AGC has converged.\n> // Insist AGC is steady for MAX_LOCK_COUNT\n> // frames before we say we are \"locked\".\n> // (The hard-coded constants may need to\n> // become customisable.)\n> - if (status.target_exposure_value) {\n> + if (status_.target_exposure_value) {\n> #define MAX_LOCK_COUNT 3\n> - double err = 0.10 *\n> status.target_exposure_value + 200;\n> + double err = 0.10 *\n> status_.target_exposure_value + 200;\n> if (actual_exposure <\n> - status.target_exposure_value +\n> err\n> - && actual_exposure >\n> - status.target_exposure_value -\n> err)\n> +\n> status_.target_exposure_value + err &&\n> + actual_exposure >\n> +\n> status_.target_exposure_value - err)\n> lock_count_ =\n>\n> std::min(lock_count + 1,\n> -\n> MAX_LOCK_COUNT);\n> +\n> MAX_LOCK_COUNT);\n> else if (actual_exposure <\n> -\n> status.target_exposure_value\n> - + 1.5 * err &&\n> +\n> status_.target_exposure_value + 1.5 * err &&\n> actual_exposure >\n> -\n> status.target_exposure_value\n> - - 1.5 * err)\n> +\n> status_.target_exposure_value - 1.5 * err)\n> lock_count_ = lock_count;\n> LOG(RPiAgc, Debug) << \"Lock count:\n> \" << lock_count_;\n> }\n> }\n> } else\n> LOG(RPiAgc, Debug) << Name() << \": no device\n> metadata\";\n> - status.locked = lock_count_ >= MAX_LOCK_COUNT;\n> - //printf(\"%s\\n\", status.locked ? \"+++++++++\" : \"-\");\n> - image_metadata->Set(\"agc.status\", status);\n> + status_.locked = lock_count_ >= MAX_LOCK_COUNT;\n> + //printf(\"%s\\n\", status_.locked ? \"+++++++++\" : \"-\");\n>\n\nIs it worth getting rid of this commented out printf, or perhaps changing\nto a trace log point?\n\n\n> + image_metadata->Set(\"agc.status\", status_);\n> }\n> }\n>\n> @@ -335,55 +320,47 @@ static void copy_string(std::string const &s, char\n> *d, size_t size)\n> void Agc::housekeepConfig()\n> {\n> // First fetch all the up-to-date settings, so no one else has to\n> do it.\n> - std::string new_exposure_mode_name, new_constraint_mode_name,\n> - new_metering_mode_name;\n> - {\n> - std::unique_lock lock(settings_mutex_);\n> - new_metering_mode_name = metering_mode_name_;\n> - new_exposure_mode_name = exposure_mode_name_;\n> - new_constraint_mode_name = constraint_mode_name_;\n> - status_.ev = ev_;\n> - status_.fixed_shutter = fixed_shutter_;\n> - status_.fixed_analogue_gain = fixed_analogue_gain_;\n> - status_.flicker_period = flicker_period_;\n> - }\n> + status_.ev = ev_;\n> + status_.fixed_shutter = fixed_shutter_;\n> + status_.fixed_analogue_gain = fixed_analogue_gain_;\n> + status_.flicker_period = flicker_period_;\n> LOG(RPiAgc, Debug) << \"ev \" << status_.ev << \" fixed_shutter \"\n> << status_.fixed_shutter << \"\n> fixed_analogue_gain \"\n> << status_.fixed_analogue_gain;\n> // Make sure the \"mode\" pointers point to the up-to-date things, if\n> // they've changed.\n> - if (strcmp(new_metering_mode_name.c_str(), status_.metering_mode))\n> {\n> - auto it =\n> config_.metering_modes.find(new_metering_mode_name);\n> + if (strcmp(metering_mode_name_.c_str(), status_.metering_mode)) {\n> + auto it = config_.metering_modes.find(metering_mode_name_);\n> if (it == config_.metering_modes.end())\n> throw std::runtime_error(\"Agc: no metering mode \" +\n> - new_metering_mode_name);\n> + metering_mode_name_);\n> metering_mode_ = &it->second;\n> - copy_string(new_metering_mode_name, status_.metering_mode,\n> + copy_string(metering_mode_name_, status_.metering_mode,\n> sizeof(status_.metering_mode));\n> }\n> - if (strcmp(new_exposure_mode_name.c_str(), status_.exposure_mode))\n> {\n> - auto it =\n> config_.exposure_modes.find(new_exposure_mode_name);\n> + if (strcmp(exposure_mode_name_.c_str(), status_.exposure_mode)) {\n> + auto it = config_.exposure_modes.find(exposure_mode_name_);\n> if (it == config_.exposure_modes.end())\n> throw std::runtime_error(\"Agc: no exposure profile\n> \" +\n> - new_exposure_mode_name);\n> + exposure_mode_name_);\n> exposure_mode_ = &it->second;\n> - copy_string(new_exposure_mode_name, status_.exposure_mode,\n> + copy_string(exposure_mode_name_, status_.exposure_mode,\n> sizeof(status_.exposure_mode));\n> }\n> - if (strcmp(new_constraint_mode_name.c_str(),\n> status_.constraint_mode)) {\n> + if (strcmp(constraint_mode_name_.c_str(),\n> status_.constraint_mode)) {\n> auto it =\n> -\n> config_.constraint_modes.find(new_constraint_mode_name);\n> +\n> config_.constraint_modes.find(constraint_mode_name_);\n> if (it == config_.constraint_modes.end())\n> throw std::runtime_error(\"Agc: no constraint list\n> \" +\n> - new_constraint_mode_name);\n> + constraint_mode_name_);\n> constraint_mode_ = &it->second;\n> - copy_string(new_constraint_mode_name,\n> status_.constraint_mode,\n> + copy_string(constraint_mode_name_, status_.constraint_mode,\n> sizeof(status_.constraint_mode));\n> }\n> LOG(RPiAgc, Debug) << \"exposure_mode \"\n> - << new_exposure_mode_name << \" constraint_mode \"\n> - << new_constraint_mode_name << \" metering_mode \"\n> - << new_metering_mode_name;\n> + << exposure_mode_name_ << \" constraint_mode \"\n> + << constraint_mode_name_ << \" metering_mode \"\n> + << metering_mode_name_;\n> }\n>\n> void Agc::fetchCurrentExposure(Metadata *image_metadata)\n> @@ -638,10 +615,6 @@ void Agc::writeAndFinish(Metadata *image_metadata,\n> bool desaturate)\n> status_.target_exposure_value = desaturate ? 0 :\n> target_.total_exposure_no_dg;\n> status_.shutter_time = filtered_.shutter;\n> status_.analogue_gain = filtered_.analogue_gain;\n> - {\n> - std::unique_lock lock(output_mutex_);\n> - output_status_ = status_;\n> - }\n> // Write to metadata as well, in case anyone wants to update the\n> camera\n> // immediately.\n> image_metadata->Set(\"agc.status\", status_);\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index ba7ae092..5a02df4e 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> @@ -106,12 +106,9 @@ private:\n> ExposureValues current_; // values for the current frame\n> ExposureValues target_; // calculate the values we want here\n> ExposureValues filtered_; // these values are filtered towards\n> target\n> - AgcStatus status_; // to \"latch\" settings so they can't\n> change\n> - AgcStatus output_status_; // the status we will write out\n> - std::mutex output_mutex_;\n> + AgcStatus status_;\n> int lock_count_;\n> // Below here the \"settings\" that applications can change.\n> - std::mutex settings_mutex_;\n> std::string metering_mode_name_;\n> std::string exposure_mode_name_;\n> std::string constraint_mode_name_;\n> --\n> 2.20.1\n>\n\nApart from the minor nit,\n\nReviewed-by: Naushir Patuck \n\n\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel\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 4A688BE081\n\tfor ;\n\tTue, 17 Nov 2020 10:33:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC2DE63323;\n\tTue, 17 Nov 2020 11:33:15 +0100 (CET)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6D8863282\n\tfor ;\n\tTue, 17 Nov 2020 11:33:14 +0100 (CET)","by mail-lf1-x12e.google.com with SMTP id u19so23162365lfr.7\n\tfor ;\n\tTue, 17 Nov 2020 02:33:14 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"BtObkGWp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=zM28SmMX+TfRt0syu8iQ4ZjhMxpPMyqqsdK0qIiWnLw=;\n\tb=BtObkGWpApQabKe2eBncUUvq1ORVTejPg9Nj8CQd5nwY9tmvbEBIBtFQv0xHwE3IJy\n\t74rSht/F5g/BxegOnbuF3zHHWdxUIVSn3XdCs78/315/G+f3QdSKRNBBUPbwKRyagfLX\n\tOPTzk7DSyqcUMiNYhNsy2S71m8tnAjGFsY8kMMETTKcvU8bGZIgbya+XPcD/ejmB98T3\n\tNfkbEiW1vu7oSSdwi9zx3f0TmYX5mEITl0bTcaLKaIG+gqnRgAnWxnaa57aWzFV/EzF2\n\thuLNotatFS5yphd5KUf3AeIpH+tI/E76Q//NkrMQLuIX64TXqow6esJRSg0/xp/at1Vp\n\tHjwg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=zM28SmMX+TfRt0syu8iQ4ZjhMxpPMyqqsdK0qIiWnLw=;\n\tb=Ly/JvDv5k3XEyer6QQXga/qfNNZIa1gjPxEuGMIAE26J1awb1qXGAbhDXjQlQ3rquE\n\tuZpKHa/lCeyd9RUponAtE/S91iVKlN+rSp8Je3MYylYi0FZ7XXCpAJ32AwOrbwqud6il\n\t1Lc6kArzwTBUEntSOwFG5Zk9Pv7evrz+Wg9qfuz6MGi/n2lkfucUAAkTw8kFWdDosjQZ\n\tApJ5g8TTVR74FWU2LoUHE8bX/2F1hGNXCdVVr9ZN5EmkLjiSY/hh/EmyBb1i5ypvqSAS\n\tj5TYDY2/tYyEJDOFmfJkvzVkKqEdPipytKkP/gOz8Nw17spAvo6E3aWb4ehlf8Duup9A\n\t7joQ==","X-Gm-Message-State":"AOAM533LfrN3dXhESVJezykXCoDDD58sNm50SV9dK/SCwVJDoU3e9iME\n\tzrh8RVIrvC/5s7wcEB3qcjqj/j5pORtIU8DF7SR0lw==","X-Google-Smtp-Source":"ABdhPJxckvSaQD1yv+3pWMKOA5ICo9jb2pZb+j+4tdf9eTJTgS66o/2xi19mB2Npn5HFY4z90ymrD+pRiGUDjOe2k4Q=","X-Received":"by 2002:a19:b46:: with SMTP id 67mr1334376lfl.488.1605609194163; \n\tTue, 17 Nov 2020 02:33:14 -0800 (PST)","MIME-Version":"1.0","References":"<20201116164918.2055-1-david.plowman@raspberrypi.com>\n\t<20201116164918.2055-3-david.plowman@raspberrypi.com>","In-Reply-To":"<20201116164918.2055-3-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Tue, 17 Nov 2020 10:32:57 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH 02/10] libcamera: ipa: raspberrypi:\n\tagc: Remove unnecessary locking","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":"multipart/mixed;\n\tboundary=\"===============0571125128446156821==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]