From patchwork Mon Nov 23 07:37:56 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Plowman X-Patchwork-Id: 10461 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id CB33ABE08A for ; Mon, 23 Nov 2020 07:38:34 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 3D26F63344; Mon, 23 Nov 2020 08:38:34 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="iXiHAFTC"; dkim-atps=neutral Received: from mail-wr1-x436.google.com (mail-wr1-x436.google.com [IPv6:2a00:1450:4864:20::436]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 55B5760332 for ; Mon, 23 Nov 2020 08:38:32 +0100 (CET) Received: by mail-wr1-x436.google.com with SMTP id b6so17594950wrt.4 for ; Sun, 22 Nov 2020 23:38:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=ZHnJxWPMbxEB7oCKged3AtYHg1uDAslixQCdBCXVkrU=; b=iXiHAFTCy8uEyPgsrLqsVMEcoB4tF5aNhQREWnXrJFdxV+LbkyT8kmJGUBJJaAQhwy VuiMaetnmTlHfPfBx0kRg/CYGA7A4W8XIn+S/7RkXgmigQH1AAUMjWuWyZ1xRigS47q5 hbqLH3hSYbDcdM9cEHChv2o3l1LyMhpBhlYMjYjhXui4ScFF8czqPIKErqMaMcVoB5ZP H2K3SVGvZh/pBtP0L8FJyyt2hoMQI/JuafoA1xjA0i/v8q9Y5p6bjlXVHk2bpzq0jMZH Pru7ySUeT9ykHAGJHRqfKcgCEH5kGbldA/DUa7aydGgl8rNB5eYt/sSgXmM/7NuskHsZ iiEQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=ZHnJxWPMbxEB7oCKged3AtYHg1uDAslixQCdBCXVkrU=; b=Ry98rNv1wpxYXHiZXuQf2k9Ha7KD1WjCVAxfYcXs41IsIpXDOr8qSHYnAKJh+DY/gG jFKP4g5KHBgm5JGxxK44mGWa9L3DXp+zwX4TD2mdk/MeDiObD7G5UnKEMCUSIZEi0GXv BF2KJqqPDappuRLOjHbAvuazB8P3nLffnsjUDv3fmZtqgpWZ3Uwc4GeZ9SRh1TIbsxvP +Rrlq1RU+JYc4NLUZ3jdunR34RkiRIC/m8SCyKVKL+/4JnpXrYzYf/XOu2I+sx7GSkgD CIvlzKeCJiYDoY4wJZj9WzTY4lL2Y5JJiTfDCKkgUkKmPsN8uIlCBEV9e7KfdEzu5t0i 25/A== X-Gm-Message-State: AOAM533N/TULYAzoADg3SyQHR7ovxL5CkndUmF9GMHUJn/EJ+TG2AXPh 1ntGD4NEy3cOc1z9tHZv9UcMS2474WHo8dmf X-Google-Smtp-Source: ABdhPJzAUFAkk4ZqE+eza0Iz6gx3shiBrAzh/r/GR7FLS2HvvF7uf+U4FPUFSydfJzeSJlFt2lObXw== X-Received: by 2002:a05:6000:108:: with SMTP id o8mr28194536wrx.337.1606117111649; Sun, 22 Nov 2020 23:38:31 -0800 (PST) Received: from pi4-davidp.lan (plowpeople3.plus.com. [80.229.223.72]) by smtp.gmail.com with ESMTPSA id h15sm17841822wrw.15.2020.11.22.23.38.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 22 Nov 2020 23:38:31 -0800 (PST) From: David Plowman To: libcamera-devel@lists.libcamera.org Date: Mon, 23 Nov 2020 07:37:56 +0000 Message-Id: <20201123073804.3125-3-david.plowman@raspberrypi.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20201123073804.3125-1-david.plowman@raspberrypi.com> References: <20201123073804.3125-1-david.plowman@raspberrypi.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH v2 02/10] libcamera: ipa: raspberrypi: agc: Remove unnecessary locking X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" On the libcamera/VC4 platform the AGC Prepare/Process methods, and any changes to the AGC settings, run synchronously - so a number of mutexes and copies are unnecessary and can be removed. Signed-off-by: David Plowman Reviewed-by: Naushir Patuck --- src/ipa/raspberrypi/controller/rpi/agc.cpp | 98 ++++++++-------------- src/ipa/raspberrypi/controller/rpi/agc.hpp | 5 +- 2 files changed, 36 insertions(+), 67 deletions(-) diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp index 8079345b..0a67f456 100644 --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp @@ -160,7 +160,6 @@ Agc::Agc(Controller *controller) status_.total_exposure_value = 0.0; status_.target_exposure_value = 0.0; status_.locked = false; - output_status_ = status_; } char const *Agc::Name() const @@ -185,43 +184,36 @@ void Agc::Read(boost::property_tree::ptree const ¶ms) void Agc::SetEv(double ev) { - std::unique_lock lock(settings_mutex_); ev_ = ev; } void Agc::SetFlickerPeriod(double flicker_period) { - std::unique_lock lock(settings_mutex_); flicker_period_ = flicker_period; } void Agc::SetFixedShutter(double fixed_shutter) { - std::unique_lock lock(settings_mutex_); fixed_shutter_ = fixed_shutter; } void Agc::SetFixedAnalogueGain(double fixed_analogue_gain) { - std::unique_lock lock(settings_mutex_); fixed_analogue_gain_ = fixed_analogue_gain; } void Agc::SetMeteringMode(std::string const &metering_mode_name) { - std::unique_lock lock(settings_mutex_); metering_mode_name_ = metering_mode_name; } void Agc::SetExposureMode(std::string const &exposure_mode_name) { - std::unique_lock lock(settings_mutex_); exposure_mode_name_ = exposure_mode_name; } void Agc::SetConstraintMode(std::string const &constraint_mode_name) { - std::unique_lock lock(settings_mutex_); constraint_mode_name_ = constraint_mode_name; } @@ -240,14 +232,9 @@ void Agc::SwitchMode([[maybe_unused]] CameraMode const &camera_mode, void Agc::Prepare(Metadata *image_metadata) { - AgcStatus status; - { - std::unique_lock lock(output_mutex_); - status = output_status_; - } int lock_count = lock_count_; lock_count_ = 0; - status.digital_gain = 1.0; + status_.digital_gain = 1.0; if (status_.total_exposure_value) { // Process has run, so we have meaningful values. DeviceStatus device_status; @@ -255,48 +242,45 @@ void Agc::Prepare(Metadata *image_metadata) double actual_exposure = device_status.shutter_speed * device_status.analogue_gain; if (actual_exposure) { - status.digital_gain = + status_.digital_gain = status_.total_exposure_value / actual_exposure; LOG(RPiAgc, Debug) << "Want total exposure " << status_.total_exposure_value; // Never ask for a gain < 1.0, and also impose // some upper limit. Make it customisable? - status.digital_gain = std::max( + status_.digital_gain = std::max( 1.0, - std::min(status.digital_gain, 4.0)); + std::min(status_.digital_gain, 4.0)); LOG(RPiAgc, Debug) << "Actual exposure " << actual_exposure; - LOG(RPiAgc, Debug) << "Use digital_gain " << status.digital_gain; - LOG(RPiAgc, Debug) << "Effective exposure " << actual_exposure * status.digital_gain; + LOG(RPiAgc, Debug) << "Use digital_gain " << status_.digital_gain; + LOG(RPiAgc, Debug) << "Effective exposure " << actual_exposure * status_.digital_gain; // Decide whether AEC/AGC has converged. // Insist AGC is steady for MAX_LOCK_COUNT // frames before we say we are "locked". // (The hard-coded constants may need to // become customisable.) - if (status.target_exposure_value) { + if (status_.target_exposure_value) { #define MAX_LOCK_COUNT 3 - double err = 0.10 * status.target_exposure_value + 200; + double err = 0.10 * status_.target_exposure_value + 200; if (actual_exposure < - status.target_exposure_value + err - && actual_exposure > - status.target_exposure_value - err) + status_.target_exposure_value + err && + actual_exposure > + status_.target_exposure_value - err) lock_count_ = std::min(lock_count + 1, - MAX_LOCK_COUNT); + MAX_LOCK_COUNT); else if (actual_exposure < - status.target_exposure_value - + 1.5 * err && + status_.target_exposure_value + 1.5 * err && actual_exposure > - status.target_exposure_value - - 1.5 * err) + status_.target_exposure_value - 1.5 * err) lock_count_ = lock_count; LOG(RPiAgc, Debug) << "Lock count: " << lock_count_; } } } else LOG(RPiAgc, Debug) << Name() << ": no device metadata"; - status.locked = lock_count_ >= MAX_LOCK_COUNT; - //printf("%s\n", status.locked ? "+++++++++" : "-"); - image_metadata->Set("agc.status", status); + status_.locked = lock_count_ >= MAX_LOCK_COUNT; + image_metadata->Set("agc.status", status_); } } @@ -335,55 +319,47 @@ static void copy_string(std::string const &s, char *d, size_t size) void Agc::housekeepConfig() { // First fetch all the up-to-date settings, so no one else has to do it. - std::string new_exposure_mode_name, new_constraint_mode_name, - new_metering_mode_name; - { - std::unique_lock lock(settings_mutex_); - new_metering_mode_name = metering_mode_name_; - new_exposure_mode_name = exposure_mode_name_; - new_constraint_mode_name = constraint_mode_name_; - status_.ev = ev_; - status_.fixed_shutter = fixed_shutter_; - status_.fixed_analogue_gain = fixed_analogue_gain_; - status_.flicker_period = flicker_period_; - } + status_.ev = ev_; + status_.fixed_shutter = fixed_shutter_; + status_.fixed_analogue_gain = fixed_analogue_gain_; + status_.flicker_period = flicker_period_; LOG(RPiAgc, Debug) << "ev " << status_.ev << " fixed_shutter " << status_.fixed_shutter << " fixed_analogue_gain " << status_.fixed_analogue_gain; // Make sure the "mode" pointers point to the up-to-date things, if // they've changed. - if (strcmp(new_metering_mode_name.c_str(), status_.metering_mode)) { - auto it = config_.metering_modes.find(new_metering_mode_name); + if (strcmp(metering_mode_name_.c_str(), status_.metering_mode)) { + auto it = config_.metering_modes.find(metering_mode_name_); if (it == config_.metering_modes.end()) throw std::runtime_error("Agc: no metering mode " + - new_metering_mode_name); + metering_mode_name_); metering_mode_ = &it->second; - copy_string(new_metering_mode_name, status_.metering_mode, + copy_string(metering_mode_name_, status_.metering_mode, sizeof(status_.metering_mode)); } - if (strcmp(new_exposure_mode_name.c_str(), status_.exposure_mode)) { - auto it = config_.exposure_modes.find(new_exposure_mode_name); + if (strcmp(exposure_mode_name_.c_str(), status_.exposure_mode)) { + auto it = config_.exposure_modes.find(exposure_mode_name_); if (it == config_.exposure_modes.end()) throw std::runtime_error("Agc: no exposure profile " + - new_exposure_mode_name); + exposure_mode_name_); exposure_mode_ = &it->second; - copy_string(new_exposure_mode_name, status_.exposure_mode, + copy_string(exposure_mode_name_, status_.exposure_mode, sizeof(status_.exposure_mode)); } - if (strcmp(new_constraint_mode_name.c_str(), status_.constraint_mode)) { + if (strcmp(constraint_mode_name_.c_str(), status_.constraint_mode)) { auto it = - config_.constraint_modes.find(new_constraint_mode_name); + config_.constraint_modes.find(constraint_mode_name_); if (it == config_.constraint_modes.end()) throw std::runtime_error("Agc: no constraint list " + - new_constraint_mode_name); + constraint_mode_name_); constraint_mode_ = &it->second; - copy_string(new_constraint_mode_name, status_.constraint_mode, + copy_string(constraint_mode_name_, status_.constraint_mode, sizeof(status_.constraint_mode)); } LOG(RPiAgc, Debug) << "exposure_mode " - << new_exposure_mode_name << " constraint_mode " - << new_constraint_mode_name << " metering_mode " - << new_metering_mode_name; + << exposure_mode_name_ << " constraint_mode " + << constraint_mode_name_ << " metering_mode " + << metering_mode_name_; } void Agc::fetchCurrentExposure(Metadata *image_metadata) @@ -638,10 +614,6 @@ void Agc::writeAndFinish(Metadata *image_metadata, bool desaturate) status_.target_exposure_value = desaturate ? 0 : target_.total_exposure_no_dg; status_.shutter_time = filtered_.shutter; status_.analogue_gain = filtered_.analogue_gain; - { - std::unique_lock lock(output_mutex_); - output_status_ = status_; - } // Write to metadata as well, in case anyone wants to update the camera // immediately. image_metadata->Set("agc.status", status_); diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp index ba7ae092..5a02df4e 100644 --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp @@ -106,12 +106,9 @@ private: ExposureValues current_; // values for the current frame ExposureValues target_; // calculate the values we want here ExposureValues filtered_; // these values are filtered towards target - AgcStatus status_; // to "latch" settings so they can't change - AgcStatus output_status_; // the status we will write out - std::mutex output_mutex_; + AgcStatus status_; int lock_count_; // Below here the "settings" that applications can change. - std::mutex settings_mutex_; std::string metering_mode_name_; std::string exposure_mode_name_; std::string constraint_mode_name_;