From patchwork Mon Nov 16 16:49:10 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Plowman X-Patchwork-Id: 10431 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 4F079BE082 for ; Mon, 16 Nov 2020 16:49:29 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 10D80632D4; Mon, 16 Nov 2020 17:49:29 +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="is8Te22X"; dkim-atps=neutral Received: from mail-wr1-x433.google.com (mail-wr1-x433.google.com [IPv6:2a00:1450:4864:20::433]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id DF2C3632C9 for ; Mon, 16 Nov 2020 17:49:26 +0100 (CET) Received: by mail-wr1-x433.google.com with SMTP id p8so19427387wrx.5 for ; Mon, 16 Nov 2020 08:49:26 -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=fnx644/4e1aclgbmGPuB8wpXNNlNgk4rgNHPuNiIVwo=; b=is8Te22X41nTPWaQpy0bAQKCkBbatmToD+4Z+ORZ6yLkpcJ0sAOnL+idNU7cuSjIWE my1vYGYdBZ8helWF088826vEZXtFuUieTlpowcPnXEd3ZcyK6MeuciO5LidIch93JE1y 6khLEnsSQHDS1liB58YTD4O4PY/f/HbW5qR+/FmCzZ73AYkTLOu4CW7SVzqRaCJXIjNm MR8sgclWe3bvkwoK1d83KLHMBqo3NlEvjBmiZBW7Rk2G0PMSQwBCUtmJJBYUyUn8z11o DY1Y9JN8f1HDF4WJIqLorSKC5y3tZ3WRJDYotxogQtCpAZt0tX1Jx8vX1WynRba3WRQi 2jjA== 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=fnx644/4e1aclgbmGPuB8wpXNNlNgk4rgNHPuNiIVwo=; b=niecWTKt2izCRtNtxvemMxIbHvrei0gzkFWSyVERK8j5fkSjzUS1jHN3/132cNNJkg lhn4nEI/9sBUPxRUPKx8D01PHuyESEChbBkntrFOZnuO1L1j8n2hFcsXc4NxsUCxfSs+ RVg/+2cz92B7cP9HvzXMUqYt3F0X04FIComKMaQP4yA98BOILPFthAop4Cmt02uuF0nZ gJn2/l8PFeucAJXeHpR1bOJ6eSR6u7/2n2XOBmj/CJm8KZRUuON+jhOUJPnreyr68vah qnb1JsFpZLIp4Jfj5/fAQO9dIwfXxM91U+/VPA3vtTQdzbjU2nfsMCpgoiZ5BhG1zMXz ZRGQ== X-Gm-Message-State: AOAM533iWy5dij6CbqOmpC/Ve5f1OYjrqt5mwGbQUotaGCOL/7QrViBk zmX0zgjiFvlF/+vVMEWM/NTXue03liCZNA== X-Google-Smtp-Source: ABdhPJwAGHDfxAlPViJZus04GoHVbCy2opSke3vwWf4/PUWVOeJb2pSeSLwBXpRcoludENWDxd9EsQ== X-Received: by 2002:adf:f906:: with SMTP id b6mr20211716wrr.244.1605545366052; Mon, 16 Nov 2020 08:49:26 -0800 (PST) Received: from pi4-davidp.lan (plowpeople3.plus.com. [80.229.223.72]) by smtp.gmail.com with ESMTPSA id q16sm23716973wrn.13.2020.11.16.08.49.24 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 16 Nov 2020 08:49:24 -0800 (PST) From: David Plowman To: libcamera-devel@lists.libcamera.org Date: Mon, 16 Nov 2020 16:49:10 +0000 Message-Id: <20201116164918.2055-3-david.plowman@raspberrypi.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20201116164918.2055-1-david.plowman@raspberrypi.com> References: <20201116164918.2055-1-david.plowman@raspberrypi.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH 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 | 99 ++++++++-------------- src/ipa/raspberrypi/controller/rpi/agc.hpp | 5 +- 2 files changed, 37 insertions(+), 67 deletions(-) diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp index 8079345b..c4379b99 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,46 @@ 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; + //printf("%s\n", status_.locked ? "+++++++++" : "-"); + image_metadata->Set("agc.status", status_); } } @@ -335,55 +320,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 +615,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_;