From patchwork Tue Jun 17 08:29:49 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Plowman X-Patchwork-Id: 23584 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 9151EBDE6B for ; Tue, 17 Jun 2025 08:30:10 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 4E02E68DD2; Tue, 17 Jun 2025 10:30:05 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="A6HjDcxG"; dkim-atps=neutral Received: from mail-wm1-x333.google.com (mail-wm1-x333.google.com [IPv6:2a00:1450:4864:20::333]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 3C00168DB4 for ; Tue, 17 Jun 2025 10:30:02 +0200 (CEST) Received: by mail-wm1-x333.google.com with SMTP id 5b1f17b1804b1-451e2f0d9c2so4698825e9.1 for ; Tue, 17 Jun 2025 01:30:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; t=1750149001; x=1750753801; darn=lists.libcamera.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=YX87qigBl9t1AU9Ees2mQoY/o3WFqTSoOY5Nua1k+Lo=; b=A6HjDcxGiMOTyyB+SQMImepGUcxk9VE7qQpnqNE7rHpODLlxVyqiBpez0uaPpuvsIg 0f7vBc+1oDjVFIpmMc/7WAT/aPwx39tsSIcWv2agmvzJwbvDizjp2z/ciJbxZWYCyMpU xUEcrxHbcQBHiuZ3s6lU0BvRxUG+stGVSaWQZl4WoJuFky1scChEkf0l71yqyGuYtlcf zx7RSg2LjmoPP8nP5UcD9s2VP8+QX6lx/NF+1XgDH7ri3QY6Zi9hSIVIB5nLm19IeQH6 B+x181of4t2q207mbPPBwhduz5MCZw8NHwa2yR5zE6VrK/4+Pc62f33Y3d2fxqHN/kAl VwPQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750149001; x=1750753801; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=YX87qigBl9t1AU9Ees2mQoY/o3WFqTSoOY5Nua1k+Lo=; b=rS9gcDKhlDv5L1OctinwseWhIVpod2ETzxPc6IGeM0h1h66o/nf4sRhL48Z9OvqmUK gwpqI7RuSGVrwHvWVBMGeD0zea3SawhjsQYyey8+m8+vEPiN8UCGM0M7vitHHA5ufUDG hH/FWcV6bS5TBdY+SHvOCzVIGQo5HzB/XD1NgOi+idcafaKOIBc9v8N6AK3icxqtUKBP 8Hv3rHCd7Hks21WhAQJXwt1L8PmYlRC1z70R1xw48tDNkp1dZz+cXQjmQyvAHdeIuhZJ uxmcTCNXWRntOD+v5oEJ8UAncd5ii8wbQDm/tdeZYj9gUVBmsrxyKt6vCRzSr1HziGOO s1og== X-Gm-Message-State: AOJu0YzLDs2dk7oPybtSZQQq2DHlPExPUQLzYubNvOxfNyMad54Svsgb Vg7u9N3wDBoLWOqF86SXHippyqZfsTdVqeSQXiqR+vXBeY1eqcvkpBZdYbB065hzLvuGzZbSOYI UBRt0 X-Gm-Gg: ASbGnctugxpTNAOoHg8LwUgFjm5jDju81I7gXC7C+JZWBw0nU9zZ1CPKSuSVz7mHaET SR+lJyBewtMi130JCN3DS9kqR6yHX00wcEihwqfOULrJ5gHvnlHMlG38WvtA85Pj97CXdLhUUrz +IkT60Ckmd3OOHWAF3w7NvUWTLLJ94jjWgtUwXvieO23+Zi+dxntYMWWu038wkdyODFGZ0mvx3V itONAF1L0nnpUIDFfaHFflMK3JtGjz8qyK++nVPdeP8pCSd9vxw1Kz+oTko/3wGVyMZXNWW6Q5y Dm+8Fuc37OiUqm9hEIr9QlxcuoXaXrBCXAd+yon493rIOEhvrDcuyfORZYHo2RYnCagc30phtwS n5B4S2GIdSwFov3qzxA== X-Google-Smtp-Source: AGHT+IFl0870O7TU4HjOJPCG6wKq5uqNDWzeJa757q0mtwBdAe3XWCFxZc5UyMSKxzb1CMQrHcbOJQ== X-Received: by 2002:a05:600c:314c:b0:442:f4a3:a2c0 with SMTP id 5b1f17b1804b1-4533c95bc97mr128678745e9.13.1750149001005; Tue, 17 Jun 2025 01:30:01 -0700 (PDT) Received: from raspberrypi.pitowers.org ([2a00:1098:3142:1f:ffc9:aff6:7f7f:893b]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4532e259108sm166062955e9.32.2025.06.17.01.30.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Jun 2025 01:30:00 -0700 (PDT) From: David Plowman To: libcamera-devel@lists.libcamera.org Cc: David Plowman Subject: [PATCH 1/7] ipa: rpi: agc: Change handling of colour gains less than 1 Date: Tue, 17 Jun 2025 09:29:49 +0100 Message-Id: <20250617082956.5699-2-david.plowman@raspberrypi.com> X-Mailer: git-send-email 2.39.5 In-Reply-To: <20250617082956.5699-1-david.plowman@raspberrypi.com> References: <20250617082956.5699-1-david.plowman@raspberrypi.com> MIME-Version: 1.0 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" Previously these were handled in the AGC/AEC exposure update calculations by explicitly driving a higher digital gain to "cancel out" any colour gains that were less than 1. Now we're ignoring this in the AGC and leaving it to the IPA code to normalise all the gains so that the smallest is 1. We don't regard this as a "real" increase because one of the colour channels (just not necessarily the green one) still gets the minimum gain possible. We do, however, update the statistics calculations so that they reflect any such digital gain increase, so that images are driven to the correct level. Signed-off-by: David Plowman --- src/ipa/rpi/controller/rpi/agc_channel.cpp | 41 +++++++--------------- src/ipa/rpi/pisp/pisp.cpp | 26 ++++++++++---- src/ipa/rpi/vc4/vc4.cpp | 26 +++++++++++--- 3 files changed, 54 insertions(+), 39 deletions(-) diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp index a5562760..a87dc194 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp @@ -435,14 +435,9 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, Duration fixedExposureTime = limitExposureTime(fixedExposureTime_); if (fixedExposureTime && fixedAnalogueGain_) { - /* We're going to reset the algorithm here with these fixed values. */ - fetchAwbStatus(metadata); - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); - ASSERT(minColourGain != 0.0); - /* This is the equivalent of computeTargetExposure and applyDigitalGain. */ target_.totalExposureNoDG = fixedExposureTime_ * fixedAnalogueGain_; - target_.totalExposure = target_.totalExposureNoDG / minColourGain; + target_.totalExposure = target_.totalExposureNoDG; /* Equivalent of filterExposure. This resets any "history". */ filtered_ = target_; @@ -462,10 +457,10 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, */ double ratio = lastSensitivity / cameraMode.sensitivity; - target_.totalExposureNoDG *= ratio; target_.totalExposure *= ratio; - filtered_.totalExposureNoDG *= ratio; + target_.totalExposureNoDG = target_.totalExposure; filtered_.totalExposure *= ratio; + filtered_.totalExposureNoDG = filtered_.totalExposure; divideUpExposure(); } else { @@ -716,8 +711,13 @@ static double computeInitialY(StatisticsPtr &stats, AwbStatus const &awb, } /* Factor in the AWB correction if needed. */ - if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) - sum *= RGB{ { awb.gainR, awb.gainR, awb.gainB } }; + if (stats->agcStatsPos == Statistics::AgcStatsPos::PreWb) { + double minColourGain = std::min({ awb.gainR, awb.gainG, awb.gainB, 1.0 }); + minColourGain = std::max(minColourGain, 1.0); + RGB colourGains{ { awb.gainR, awb.gainR, awb.gainB } }; + colourGains /= minColourGain; + sum *= colourGains; + } double ySum = ipa::rec601LuminanceFromRGB(sum); @@ -797,16 +797,8 @@ void AgcChannel::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata, void AgcChannel::computeTargetExposure(double gain) { if (status_.fixedExposureTime && status_.fixedAnalogueGain) { - /* - * When analogue gain and exposure time are both fixed, we need - * to drive the total exposure so that we end up with a digital - * gain of at least 1/minColourGain. Otherwise we'd desaturate - * channels causing white to go cyan or magenta. - */ - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); - ASSERT(minColourGain != 0.0); target_.totalExposure = - status_.fixedExposureTime * status_.fixedAnalogueGain / minColourGain; + status_.fixedExposureTime * status_.fixedAnalogueGain; } else { /* * The statistics reflect the image without digital gain, so the final @@ -867,15 +859,8 @@ bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channel bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound) { - double minColourGain = std::min({ awb_.gainR, awb_.gainG, awb_.gainB, 1.0 }); - ASSERT(minColourGain != 0.0); - double dg = 1.0 / minColourGain; - /* - * I think this pipeline subtracts black level and rescales before we - * get the stats, so no need to worry about it. - */ - LOG(RPiAgc, Debug) << "after AWB, target dg " << dg << " gain " << gain - << " target_Y " << targetY; + double dg = 1.0; + /* * Finally, if we're trying to reduce exposure but the target_Y is * "close" to 1.0, then the gain computed for that constraint will be diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp index bb50a9e0..e1a804f5 100644 --- a/src/ipa/rpi/pisp/pisp.cpp +++ b/src/ipa/rpi/pisp/pisp.cpp @@ -521,10 +521,24 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr pisp_wbg_config wbg; pisp_fe_rgby_config rgby = {}; double dg = agcPrepareStatus ? agcPrepareStatus->digitalGain : 1.0; + double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 }); + /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */ + double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); - wbg.gain_r = clampField(dg * awbStatus->gainR, 14, 10); - wbg.gain_g = clampField(dg * awbStatus->gainG, 14, 10); - wbg.gain_b = clampField(dg * awbStatus->gainB, 14, 10); + /* + * Apply an extra gain of 1 / minColourGain so as not to apply < 1 gains to any + * channels (which would cause saturated pixels to go cyan or magenta). + * Doing this doesn't really apply more gain than necessary, because one of the + * channels is always getting the minimum gain possible. For this reason we also + * don't change the values that we report externally. + */ + double gainR = awbStatus->gainR * extraGain; + double gainG = awbStatus->gainG * extraGain; + double gainB = awbStatus->gainB * extraGain; + + wbg.gain_r = clampField(dg * gainR, 14, 10); + wbg.gain_g = clampField(dg * gainG, 14, 10); + wbg.gain_b = clampField(dg * gainB, 14, 10); /* * The YCbCr conversion block should contain the appropriate YCbCr @@ -535,9 +549,9 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr be_->GetYcbcr(csc); /* The CSC coefficients already have the << 10 scaling applied. */ - rgby.gain_r = clampField(csc.coeffs[0] * awbStatus->gainR, 14); - rgby.gain_g = clampField(csc.coeffs[1] * awbStatus->gainG, 14); - rgby.gain_b = clampField(csc.coeffs[2] * awbStatus->gainB, 14); + rgby.gain_r = clampField(csc.coeffs[0] * gainR, 14); + rgby.gain_g = clampField(csc.coeffs[1] * gainG, 14); + rgby.gain_b = clampField(csc.coeffs[2] * gainB, 14); LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gainR << " B: " << awbStatus->gainB; diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp index ba43e474..8a7a37c8 100644 --- a/src/ipa/rpi/vc4/vc4.cpp +++ b/src/ipa/rpi/vc4/vc4.cpp @@ -63,7 +63,8 @@ private: bool validateIspControls(); void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls); - void applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls); + void applyDG(const struct AgcPrepareStatus *dgStatus, + const struct AwbStatus *awbStatus, ControlList &ctrls); void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls); void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls); void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls); @@ -152,8 +153,7 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, applyCCM(ccmStatus, ctrls); AgcPrepareStatus *dgStatus = rpiMetadata.getLocked("agc.prepare_status"); - if (dgStatus) - applyDG(dgStatus, ctrls); + applyDG(dgStatus, awbStatus, ctrls); AlscStatus *lsStatus = rpiMetadata.getLocked("alsc.status"); if (lsStatus) @@ -329,10 +329,26 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) static_cast(awbStatus->gainB * 1000)); } -void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, ControlList &ctrls) +void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, + const struct AwbStatus *awbStatus, ControlList &ctrls) { + double digitalGain = dgStatus ? dgStatus->digitalGain : 1.0; + + if (awbStatus) { + /* + * We must apply sufficient extra digital gain to stop any of the channel gains being + * less than 1, which would cause saturation artifacts. Note that one of the colour + * channels is still getting the minimum possible gain, so it's not a "real" gain + * increase. + */ + double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 }); + /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */ + double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); + digitalGain *= extraGain; + } + ctrls.set(V4L2_CID_DIGITAL_GAIN, - static_cast(dgStatus->digitalGain * 1000)); + static_cast(digitalGain * 1000)); } void IpaVc4::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls) From patchwork Tue Jun 17 08:29:50 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Plowman X-Patchwork-Id: 23585 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 3D042BDE6B for ; Tue, 17 Jun 2025 08:30:13 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 6B52868DD6; Tue, 17 Jun 2025 10:30:06 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="qMKJDaTq"; 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 C3B6468DCB for ; Tue, 17 Jun 2025 10:30:02 +0200 (CEST) Received: by mail-wr1-x433.google.com with SMTP id ffacd0b85a97d-3a4f72cba73so4399396f8f.1 for ; Tue, 17 Jun 2025 01:30:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; t=1750149002; x=1750753802; darn=lists.libcamera.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=hJMknMs8EuyLaccgLb33jdJSpEDrlFJKRlOxS4uGqYI=; b=qMKJDaTq+SsiIDzpot1Ly9Rbv2ZxSC3XzvIN0nTie+ntr/3pHTy8pdRZRx+RSY6nKx I0V3cMXKj26LeoQaDcYx2FGOfr5uu8BqEvoS9K0q/m01KzPMjKN448yMy3dqvQaBorYR LwVyk7psil/iLlC+6G1r7/CB12Q7PLCT6vxztpD2BqzIQrm+QQqw0feHt1B/CFJhjdi6 jhlrpN5f6dDXy291Pq6jiSQnvmYje/jzb7DB8vDnHKxySSopBiQuNURHHMIMWoayTNjT a7wuQlbLRcoIoGH1VPABtZ/hYnNKZtIG+ZfQhWjGfUzp2MUftVD1lsqM3bpgZGt/pgEd H3pw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750149002; x=1750753802; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=hJMknMs8EuyLaccgLb33jdJSpEDrlFJKRlOxS4uGqYI=; b=oFT1zvjoUrmM4WRAPS/6ILU+o77VG9fJltVJWJCRmoRSQRG06jDzQPF+WKPtaGc0AZ FBAS9HInGlaKHqzaIyp9s/N4HI9Hv5TmVaoTPN6kpEeBL20wlm2CJxINVu4AYhd/bFgx CFdxHmhWG1lFVW50W/s3qRS5RtjZx9O93ZS0SA/lHhG9ZeCoVGT/HPXTG2ExR8MYZM5c YLvQzZGwIA0JQYzoj71k21qFsZk807WfExPtwMJBYT9Tq5X4PZlH4Kje3MXbFFc/v0Gw BVybberlCZ2iAbGVZbb+vcFTYd521txJhqQARU9UBeyxap51RY5jrMkpp7vGx83hqvtJ 7N0A== X-Gm-Message-State: AOJu0Yx+pmnS/4GV0i1yNQjbNbQAhy9Iv83gKjr4vxRZixIbzxjuvp/K zYmvPz+iEG/EwYKpvyl+MxVEHKqfHiWAZ8taG449y9/yduG+kIbTHarHCTv/mcvXcscdoK2VALu aTFMQ X-Gm-Gg: ASbGncvEyOM3pLYaGZQ46s+y4N/tj5RxV4DFMrtWy4lQC5UpBEHq+XpeKEgfXPs+d2j vWA8DLhjQOkf4QxRy8pWGx8oNer3LIWZnxJfUuPU5Hr2K5kBFkjJIxkZ8HjgwC4ydZ+goEqAbtS S9x7wRFSLxo9C+UD5dpn3w+D8l3/Pt0xMRxOmSUhBIaqbUbgb9S9FCYt0afzjSj3CjABGgPy9hC GDuCaWFezyyV/urgxpMG/6IYopgfaY9sWgSqlKzC/ZkAOOlE/15JKuZcqaqwg6+KuzC+IF2Ug8h zCOAVCEDO/9Cwu1GpfUdM0w4/P0KAkpiOCiSJfj9yWMBCywELLPA4WNtOJPh478Ts+odKzCrRHq +MBtWJv4eZ0Gq2hlhqw== X-Google-Smtp-Source: AGHT+IFNGvm8b/42IZxmyDiZDevdm74XMOYEyqoSJv4s4fRaRhIRdoKkAsm1NcZeqXvT15aKY313Vg== X-Received: by 2002:a05:6000:26cb:b0:3a4:f607:a5ad with SMTP id ffacd0b85a97d-3a56d834103mr11170136f8f.19.1750149001942; Tue, 17 Jun 2025 01:30:01 -0700 (PDT) Received: from raspberrypi.pitowers.org ([2a00:1098:3142:1f:ffc9:aff6:7f7f:893b]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4532e259108sm166062955e9.32.2025.06.17.01.30.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Jun 2025 01:30:01 -0700 (PDT) From: David Plowman To: libcamera-devel@lists.libcamera.org Cc: David Plowman Subject: [PATCH 2/7] ipa: rpi: agc: Make the maximum digital gain configurable Date: Tue, 17 Jun 2025 09:29:50 +0100 Message-Id: <20250617082956.5699-3-david.plowman@raspberrypi.com> X-Mailer: git-send-email 2.39.5 In-Reply-To: <20250617082956.5699-1-david.plowman@raspberrypi.com> References: <20250617082956.5699-1-david.plowman@raspberrypi.com> MIME-Version: 1.0 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" The maximum allowed digital gain is hard-coded to 4. Make it a configurable parameter. Signed-off-by: David Plowman --- src/ipa/rpi/controller/rpi/agc_channel.cpp | 5 ++++- src/ipa/rpi/controller/rpi/agc_channel.h | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp index a87dc194..d7fff2fc 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp @@ -260,6 +260,8 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms) desaturate = params["desaturate"].get(1); + maxDigitalGain = params["max_digital_gain"].get(4.0); + return 0; } @@ -508,7 +510,8 @@ void AgcChannel::prepare(Metadata *imageMetadata) * Never ask for a gain < 1.0, and also impose * some upper limit. Make it customisable? */ - prepareStatus.digitalGain = std::max(1.0, std::min(digitalGain, 4.0)); + prepareStatus.digitalGain = std::max(1.0, std::min(digitalGain, + config_.maxDigitalGain)); LOG(RPiAgc, Debug) << "Actual exposure " << actualExposure; LOG(RPiAgc, Debug) << "Use digitalGain " << prepareStatus.digitalGain; LOG(RPiAgc, Debug) << "Effective exposure " diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h index fa697e6f..e3475d86 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.h +++ b/src/ipa/rpi/controller/rpi/agc_channel.h @@ -78,6 +78,7 @@ struct AgcConfig { double defaultAnalogueGain; double stableRegion; bool desaturate; + double maxDigitalGain; }; class AgcChannel From patchwork Tue Jun 17 08:29:51 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Plowman X-Patchwork-Id: 23586 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 335CDBDE6B for ; Tue, 17 Jun 2025 08:30:15 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id BACE968DC1; Tue, 17 Jun 2025 10:30:10 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="Yd2oYbvF"; dkim-atps=neutral Received: from mail-wm1-x32c.google.com (mail-wm1-x32c.google.com [IPv6:2a00:1450:4864:20::32c]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 3897768DCF for ; Tue, 17 Jun 2025 10:30:04 +0200 (CEST) Received: by mail-wm1-x32c.google.com with SMTP id 5b1f17b1804b1-450ccda1a6eso48181355e9.2 for ; Tue, 17 Jun 2025 01:30:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; t=1750149003; x=1750753803; darn=lists.libcamera.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=qOB8nBAG4kp9rTIZ7GZcgT3x3VEbWhGE6/qVbsYOuEk=; b=Yd2oYbvFmcg4dBIQMlrzR2FS5tiYDGbcPSDUZhvL73VQuei6LyU3Aw4g3HsIN0FTDb TPLvUhfB/UL2JeWM5izQ27HcG1GwrpQX5WHZswX/17IUGZxh85ZuNPULmlnlf/QGXy3c HmbRWUmhryswzdtfjU4j03VHwDYKQ1j/avEoE9zTHth1eAGux+hKY4Vi5DTYU20kdxkD y/jv15cCZhZQNuzd1zJZgZUGyy9FW5ch0NEOc6ahZ7N7SxJHPm60dUKFIxsie/vjARSW PdsTReswMnjqNr5X3yJHQbMBlBjRZKxZ/6wSSTVEYszIpyd+eJn3fSJgnerfNcfrJ6zF pJzA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750149003; x=1750753803; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=qOB8nBAG4kp9rTIZ7GZcgT3x3VEbWhGE6/qVbsYOuEk=; b=M/mi0B8Rdm+f7TH8h/hKD6sNgBky7UgNyP6nrX+Uu9ymvg6jHxxusdT1w3XpAtlNiH jPE52S1ZsRmfLR4Y4bufrwP9SnA2TGvP4P+TNtmcUM29Su/DhGx25HSzrsiXT/bx0h4r ExVgLhiM+Dm1fYQu2jxzux07jUNBylMOKyb44C9vBTZyQ7jZJluVtthjGBnM913Q4AO8 iPSk5OIEzp9wCoQm75PYpBf3hsN3R0T2C1BWCCHjM7w6nler/beSZHDSh4sK9nV9/xha ERvUbgaG8P59v11mVI+TBTDmUR4jPDz2LxFhCKY7FzkvVK3Z4HwuQV8px8O0V46hJcbV lonw== X-Gm-Message-State: AOJu0Yyd2OHeuC52el3issbJNOMfK66Q0WX6zgme74xNE+bJYAzCO06u utcnUUogMij90y9GtcUpqV/h+ZSabuhGRD2HJ7VDDRcarezffxZm/fu06KfKl+L+lh/slWJYm4H T93lN X-Gm-Gg: ASbGncs5ahmO2+OYBVlYRmtEv/s6mrZ/ysAn7T21rRJfQFu8DkWATT+qEp4L6P9c6/0 U62cQ0JHfkKn1Vg42DYbOp63GAEIRohXmOeeM/O8BOhNIAFWH3V340DDleuvH2TN4sasf4w/Cmb kdkpBIqBf/vpRdL3MnIdcALY7Le7xtuWjvO01/k95R5L47bXD3vg0o6z1WYp2dIy7HwdOCq+q3p elanuMxbh4wGRc0KTa33lz5n030IfuwTIS0JMJTSQtNORVY9RcsRrOfNGTi2fjc5DcdTh4psYS7 3v0dHATcDaL6djgUOrGUxPBpE/n5SCu72+ScTN4M2nR3RTzsCprXas00zKIzEGBYJAcZiehHyhx L6sgUa9CbWr70khlCZA== X-Google-Smtp-Source: AGHT+IF36OMgQNa4YXaSjfrqmWm4p2UXIoYFc9LsF/dUsx+UU812IziUoZctEEhl1DCTZ4qQkh6LSQ== X-Received: by 2002:a05:600c:3513:b0:453:a88:d509 with SMTP id 5b1f17b1804b1-4533ca7443cmr150359325e9.10.1750149003046; Tue, 17 Jun 2025 01:30:03 -0700 (PDT) Received: from raspberrypi.pitowers.org ([2a00:1098:3142:1f:ffc9:aff6:7f7f:893b]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4532e259108sm166062955e9.32.2025.06.17.01.30.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Jun 2025 01:30:02 -0700 (PDT) From: David Plowman To: libcamera-devel@lists.libcamera.org Cc: David Plowman Subject: [PATCH 3/7] ipa: rpi: agc: Rename "analogue gain" to "gain" where appropriate Date: Tue, 17 Jun 2025 09:29:51 +0100 Message-Id: <20250617082956.5699-4-david.plowman@raspberrypi.com> X-Mailer: git-send-email 2.39.5 In-Reply-To: <20250617082956.5699-1-david.plowman@raspberrypi.com> References: <20250617082956.5699-1-david.plowman@raspberrypi.com> MIME-Version: 1.0 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" Much of the time we use the term "analogue gain" where we really mean the combined analogue and digital gain (because the digital gain will make up whatever the analogue gain can't deliver). This commit replaces the use of "analogue gain" by just "gain" in places where we really mean the combined gain. There are a couple of principle areas: 1. Where we previously talked about the "fixedAnalaogueGain" (including setting it "manually") this is now just the "fixedGain" (because it always encompassed both analogue and digital gain). Along with this, the setfixedExposureTime/Gain functions no longer update the output status directly. Applications should wait in the usual way for AGC/AEC changes to take effect, and this "shortcut" actually doesn't fit well with the gain being the combined gain. 2. The divideUpExposure method is adjusted to be clearer that it's setting the combined gain, and it's prepare() that will discover later what the analogue gain actually delivered. Signed-off-by: David Plowman --- src/ipa/rpi/common/ipa_base.cpp | 2 +- src/ipa/rpi/controller/agc_algorithm.h | 2 +- src/ipa/rpi/controller/agc_status.h | 2 +- src/ipa/rpi/controller/rpi/agc.cpp | 6 +- src/ipa/rpi/controller/rpi/agc.h | 4 +- src/ipa/rpi/controller/rpi/agc_channel.cpp | 77 +++++++++++----------- src/ipa/rpi/controller/rpi/agc_channel.h | 4 +- 7 files changed, 48 insertions(+), 49 deletions(-) diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index 80c17588..05699228 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -873,7 +873,7 @@ void IpaBase::applyControls(const ControlList &controls) if (agc->autoGainEnabled()) break; - agc->setFixedAnalogueGain(0, ctrl.second.get()); + agc->setFixedGain(0, ctrl.second.get()); libcameraMetadata_.set(controls::AnalogueGain, ctrl.second.get()); diff --git a/src/ipa/rpi/controller/agc_algorithm.h b/src/ipa/rpi/controller/agc_algorithm.h index fdaa10e6..9fa2bd20 100644 --- a/src/ipa/rpi/controller/agc_algorithm.h +++ b/src/ipa/rpi/controller/agc_algorithm.h @@ -26,7 +26,7 @@ public: virtual void setFixedExposureTime(unsigned int channel, libcamera::utils::Duration fixedExposureTime) = 0; virtual void setMaxExposureTime(libcamera::utils::Duration maxExposureTime) = 0; - virtual void setFixedAnalogueGain(unsigned int channel, double fixedAnalogueGain) = 0; + virtual void setFixedGain(unsigned int channel, double fixedGain) = 0; virtual void setMeteringMode(std::string const &meteringModeName) = 0; virtual void setExposureMode(std::string const &exposureModeName) = 0; virtual void setConstraintMode(std::string const &contraintModeName) = 0; diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h index 9308b156..d4cedcf4 100644 --- a/src/ipa/rpi/controller/agc_status.h +++ b/src/ipa/rpi/controller/agc_status.h @@ -37,7 +37,7 @@ struct AgcStatus { libcamera::utils::Duration flickerPeriod; int floatingRegionEnable; libcamera::utils::Duration fixedExposureTime; - double fixedAnalogueGain; + double fixedGain; unsigned int channel; HdrStatus hdr; }; diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp index 02bfdb4a..afda2e36 100644 --- a/src/ipa/rpi/controller/rpi/agc.cpp +++ b/src/ipa/rpi/controller/rpi/agc.cpp @@ -184,14 +184,14 @@ void Agc::setFixedExposureTime(unsigned int channelIndex, Duration fixedExposure channelData_[channelIndex].channel.setFixedExposureTime(fixedExposureTime); } -void Agc::setFixedAnalogueGain(unsigned int channelIndex, double fixedAnalogueGain) +void Agc::setFixedGain(unsigned int channelIndex, double fixedGain) { if (checkChannel(channelIndex)) return; - LOG(RPiAgc, Debug) << "setFixedAnalogueGain " << fixedAnalogueGain + LOG(RPiAgc, Debug) << "setFixedGain " << fixedGain << " for channel " << channelIndex; - channelData_[channelIndex].channel.setFixedAnalogueGain(fixedAnalogueGain); + channelData_[channelIndex].channel.setFixedGain(fixedGain); } void Agc::setMeteringMode(std::string const &meteringModeName) diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h index c3a940bf..966630a2 100644 --- a/src/ipa/rpi/controller/rpi/agc.h +++ b/src/ipa/rpi/controller/rpi/agc.h @@ -35,8 +35,8 @@ public: void setMaxExposureTime(libcamera::utils::Duration maxExposureTime) override; void setFixedExposureTime(unsigned int channelIndex, libcamera::utils::Duration fixedExposureTime) override; - void setFixedAnalogueGain(unsigned int channelIndex, - double fixedAnalogueGain) override; + void setFixedGain(unsigned int channelIndex, + double fixedGain) override; void setMeteringMode(std::string const &meteringModeName) override; void setExposureMode(std::string const &exposureModeName) override; void setConstraintMode(std::string const &contraintModeName) override; diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp index d7fff2fc..1028d82e 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp @@ -275,7 +275,7 @@ AgcChannel::AgcChannel() : meteringMode_(nullptr), exposureMode_(nullptr), constraintMode_(nullptr), frameCount_(0), lockCount_(0), lastTargetExposure_(0s), ev_(1.0), flickerPeriod_(0s), - maxExposureTime_(0s), fixedExposureTime_(0s), fixedAnalogueGain_(0.0) + maxExposureTime_(0s), fixedExposureTime_(0s), fixedGain_(0.0) { /* Set AWB default values in case early frames have no updates in metadata. */ awb_.gainR = 1.0; @@ -339,17 +339,17 @@ bool AgcChannel::autoExposureEnabled() const void AgcChannel::disableAutoGain() { - fixedAnalogueGain_ = status_.analogueGain; + fixedGain_ = status_.analogueGain; } void AgcChannel::enableAutoGain() { - fixedAnalogueGain_ = 0; + fixedGain_ = 0; } bool AgcChannel::autoGainEnabled() const { - return fixedAnalogueGain_ == 0; + return fixedGain_ == 0; } unsigned int AgcChannel::getConvergenceFrames() const @@ -358,7 +358,7 @@ unsigned int AgcChannel::getConvergenceFrames() const * If exposure time and gain have been explicitly set, there is no * convergence to happen, so no need to drop any frames - return zero. */ - if (fixedExposureTime_ && fixedAnalogueGain_) + if (fixedExposureTime_ && fixedGain_) return 0; else return config_.convergenceFrames; @@ -398,11 +398,9 @@ void AgcChannel::setFixedExposureTime(Duration fixedExposureTime) status_.exposureTime = limitExposureTime(fixedExposureTime_); } -void AgcChannel::setFixedAnalogueGain(double fixedAnalogueGain) +void AgcChannel::setFixedGain(double fixedGain) { - fixedAnalogueGain_ = fixedAnalogueGain; - /* Set this in case someone calls disableAuto() straight after. */ - status_.analogueGain = limitGain(fixedAnalogueGain); + fixedGain_ = fixedGain; } void AgcChannel::setMeteringMode(std::string const &meteringModeName) @@ -436,9 +434,9 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, mode_ = cameraMode; Duration fixedExposureTime = limitExposureTime(fixedExposureTime_); - if (fixedExposureTime && fixedAnalogueGain_) { + if (fixedExposureTime && fixedGain_) { /* This is the equivalent of computeTargetExposure and applyDigitalGain. */ - target_.totalExposureNoDG = fixedExposureTime_ * fixedAnalogueGain_; + target_.totalExposureNoDG = fixedExposureTime_ * fixedGain_; target_.totalExposure = target_.totalExposureNoDG; /* Equivalent of filterExposure. This resets any "history". */ @@ -446,7 +444,7 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, /* Equivalent of divideUpExposure. */ filtered_.exposureTime = fixedExposureTime; - filtered_.analogueGain = fixedAnalogueGain_; + filtered_.analogueGain = fixedGain_; } else if (status_.totalExposureValue) { /* * On a mode switch, various things could happen: @@ -476,7 +474,7 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, /* Equivalent of divideUpExposure. */ filtered_.exposureTime = fixedExposureTime ? fixedExposureTime : config_.defaultExposureTime; - filtered_.analogueGain = fixedAnalogueGain_ ? fixedAnalogueGain_ : config_.defaultAnalogueGain; + filtered_.analogueGain = fixedGain_ ? fixedGain_ : config_.defaultAnalogueGain; } writeAndFinish(metadata, false); @@ -608,11 +606,11 @@ void AgcChannel::housekeepConfig() /* First fetch all the up-to-date settings, so no one else has to do it. */ status_.ev = ev_; status_.fixedExposureTime = limitExposureTime(fixedExposureTime_); - status_.fixedAnalogueGain = fixedAnalogueGain_; + status_.fixedGain = fixedGain_; status_.flickerPeriod = flickerPeriod_; LOG(RPiAgc, Debug) << "ev " << status_.ev << " fixedExposureTime " - << status_.fixedExposureTime << " fixedAnalogueGain " - << status_.fixedAnalogueGain; + << status_.fixedExposureTime << " fixedGain " + << status_.fixedGain; /* * Make sure the "mode" pointers point to the up-to-date things, if * they've changed. @@ -799,9 +797,9 @@ void AgcChannel::computeGain(StatisticsPtr &statistics, Metadata *imageMetadata, void AgcChannel::computeTargetExposure(double gain) { - if (status_.fixedExposureTime && status_.fixedAnalogueGain) { + if (status_.fixedExposureTime && status_.fixedGain) { target_.totalExposure = - status_.fixedExposureTime * status_.fixedAnalogueGain; + status_.fixedExposureTime * status_.fixedGain; } else { /* * The statistics reflect the image without digital gain, so the final @@ -815,8 +813,8 @@ void AgcChannel::computeTargetExposure(double gain) maxExposureTime = limitExposureTime(maxExposureTime); Duration maxTotalExposure = maxExposureTime * - (status_.fixedAnalogueGain != 0.0 - ? status_.fixedAnalogueGain + (status_.fixedGain != 0.0 + ? status_.fixedGain : exposureMode_->gain.back()); target_.totalExposure = std::min(target_.totalExposure, maxTotalExposure); } @@ -896,7 +894,7 @@ void AgcChannel::filterExposure() * region, because we want to reflect any user exposure/gain updates, * however small. */ - if ((status_.fixedExposureTime && status_.fixedAnalogueGain) || + if ((status_.fixedExposureTime && status_.fixedGain) || frameCount_ <= config_.startupFrames) { speed = 1.0; stableRegion = 0.0; @@ -930,63 +928,64 @@ void AgcChannel::divideUpExposure() */ Duration exposureValue = filtered_.totalExposureNoDG; Duration exposureTime; - double analogueGain; + double gain; exposureTime = status_.fixedExposureTime ? status_.fixedExposureTime : exposureMode_->exposureTime[0]; exposureTime = limitExposureTime(exposureTime); - analogueGain = status_.fixedAnalogueGain != 0.0 ? status_.fixedAnalogueGain - : exposureMode_->gain[0]; - analogueGain = limitGain(analogueGain); - if (exposureTime * analogueGain < exposureValue) { + gain = status_.fixedGain != 0.0 ? status_.fixedGain + : exposureMode_->gain[0]; + gain = limitGain(gain); + if (exposureTime * gain < exposureValue) { for (unsigned int stage = 1; stage < exposureMode_->gain.size(); stage++) { if (!status_.fixedExposureTime) { Duration stageExposureTime = limitExposureTime(exposureMode_->exposureTime[stage]); - if (stageExposureTime * analogueGain >= exposureValue) { - exposureTime = exposureValue / analogueGain; + if (stageExposureTime * gain >= exposureValue) { + exposureTime = exposureValue / gain; break; } exposureTime = stageExposureTime; } - if (status_.fixedAnalogueGain == 0.0) { + if (status_.fixedGain == 0.0) { if (exposureMode_->gain[stage] * exposureTime >= exposureValue) { - analogueGain = exposureValue / exposureTime; + gain = exposureValue / exposureTime; break; } - analogueGain = exposureMode_->gain[stage]; - analogueGain = limitGain(analogueGain); + gain = exposureMode_->gain[stage]; + gain = limitGain(gain); } } } LOG(RPiAgc, Debug) << "Divided up exposure time and gain are " << exposureTime - << " and " << analogueGain; + << " and " << gain; /* * Finally adjust exposure time for flicker avoidance (require both * exposure time and gain not to be fixed). */ - if (!status_.fixedExposureTime && !status_.fixedAnalogueGain && + if (!status_.fixedExposureTime && !status_.fixedGain && status_.flickerPeriod) { int flickerPeriods = exposureTime / status_.flickerPeriod; if (flickerPeriods) { Duration newExposureTime = flickerPeriods * status_.flickerPeriod; - analogueGain *= exposureTime / newExposureTime; + gain *= exposureTime / newExposureTime; /* * We should still not allow the ag to go over the * largest value in the exposure mode. Note that this * may force more of the total exposure into the digital * gain as a side-effect. */ - analogueGain = std::min(analogueGain, exposureMode_->gain.back()); - analogueGain = limitGain(analogueGain); + gain = std::min(gain, exposureMode_->gain.back()); + gain = limitGain(gain); exposureTime = newExposureTime; } LOG(RPiAgc, Debug) << "After flicker avoidance, exposure time " - << exposureTime << " gain " << analogueGain; + << exposureTime << " gain " << gain; } filtered_.exposureTime = exposureTime; - filtered_.analogueGain = analogueGain; + /* We ask for all the gain as analogue gain; prepare() will be told what we got. */ + filtered_.analogueGain = gain; } void AgcChannel::writeAndFinish(Metadata *imageMetadata, bool desaturate) diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h index e3475d86..93229128 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.h +++ b/src/ipa/rpi/controller/rpi/agc_channel.h @@ -93,7 +93,7 @@ public: void setFlickerPeriod(libcamera::utils::Duration flickerPeriod); void setMaxExposureTime(libcamera::utils::Duration maxExposureTime); void setFixedExposureTime(libcamera::utils::Duration fixedExposureTime); - void setFixedAnalogueGain(double fixedAnalogueGain); + void setFixedGain(double fixedGain); void setMeteringMode(std::string const &meteringModeName); void setExposureMode(std::string const &exposureModeName); void setConstraintMode(std::string const &contraintModeName); @@ -153,7 +153,7 @@ private: libcamera::utils::Duration flickerPeriod_; libcamera::utils::Duration maxExposureTime_; libcamera::utils::Duration fixedExposureTime_; - double fixedAnalogueGain_; + double fixedGain_; }; } /* namespace RPiController */ From patchwork Tue Jun 17 08:29:52 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Plowman X-Patchwork-Id: 23587 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 E17DEBDE6B for ; Tue, 17 Jun 2025 08:30:16 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 7C14F68DDC; Tue, 17 Jun 2025 10:30:12 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="CJNbhO5k"; 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 8166968DC1 for ; Tue, 17 Jun 2025 10:30:04 +0200 (CEST) Received: by mail-wr1-x433.google.com with SMTP id ffacd0b85a97d-3a365a6804eso3920855f8f.3 for ; Tue, 17 Jun 2025 01:30:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; t=1750149004; x=1750753804; darn=lists.libcamera.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=zGffA9MI6iLQV7MMBALDScxf8HvhkdQ9UfVj+SjJCKA=; b=CJNbhO5kcS5Uazs80BPPNfBKSdWMYsGLguLqA0T3AMQpw6PI55cUlF7Ob0L/ZuKJaS wCqbMaIXj05kA3m58VSqHwV00AcYkymmYzdHr9PXBC7MnPJlLGyuWS0T2/8XOej0a5X3 Fe6swBYWSSkANHqZdb++/pwBa5K8fEH34b9jLFST/oWIqoZqPaUNO2HxiSWznVCcyqCV oobJJ+aUnBYsqzpn1F6alsJE31JE+s91SKBe9y/L1zF7WM/QA9dVZ65b8gNdkAA+dwXc PFegVx/oyBKxBDnkRiiZxZSlC8riTU+s5RFh8IX+WK3MeqOiW8pyEoFwxig7wB7Okb0l hdGw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750149004; x=1750753804; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=zGffA9MI6iLQV7MMBALDScxf8HvhkdQ9UfVj+SjJCKA=; b=dzPl0dqV1Kn0AaH0RJU3wr4YwAPDFQM/zX/gL4VrOecq+xlXwlxqMQLXv4ZKXjWKes H3/nM+4KlWxHmBfYd+NUpmPmtl+/rjfYmGhZepJ50TJlsdwtPPtfPAY6GuLOIKEP73Fl 7MjcChQilWhtsPO6Xx5Bx0mK97g/o2QFNvcUJd/xEOJowLv29LjGm6fVttYiInBjiSOE ht4bnPJ5F9RjVaVCB+cY7/bdz5jSNCpdIGNlHn9TKkCvDML5tRU2uAhCq3lGPjbhpj6T syhKLyKXHJ/IFMk1HGvbpRBZAWcqmJAH5nr8AANJIdr1jbxufvu14Bk0uuP5sHCxVHcn JUdA== X-Gm-Message-State: AOJu0Yy163CYqGYwAVb1yzFosAW56q5FjoVrZe6bnql6VWjVDj+c5O16 KgNeBf0D2t015xLx/T5L74mT9bO0ZjuKPSqIXr/cb4gcruKssmOhPJk/PFyHDZXWXZSy5jaMYKE iQ4u3 X-Gm-Gg: ASbGncsQ3y0X8gDEkoGqRYTPhVa3rnEDgVtAa9K2JQaHUIOjMuU9Dxl6IQ5zkpD+ey6 +dUDvAQ9YHB/7VQrlm6vFGu0aQYPTEqY6uUgg60Xt/tIihT1nfcsgNbo+eN+YOThaBNmfIL5gbh ug7Lj9L7keTfoSCgfkDenftwy+aShba1hLBVaVIuxE2HgxPWOc9PCWdS1Jb4mXcj2jVOu88c2ad FaP650norbdTC1oqLaEcG2Rizv7ofedw22YfFnVrsn6RGI/CYG/PtGMYCmAG/GGNt2dwAgUFdza CtYsv1rNyPOPCfQd0UIXzECaVW0LXB9Zfh1qZbj00K64F7zK5E03RpBLrwAGGgJQV41KpZnZXfu gwKmaBjN/22ph+GJ2hw== X-Google-Smtp-Source: AGHT+IF5mf3OfIarWoHXUuHBJcmJitTNaoeR9yUHmqRUMJR4A2HRENz/bccDV00mPU3EMlDj6VottA== X-Received: by 2002:a05:6000:2088:b0:3a5:67d5:a400 with SMTP id ffacd0b85a97d-3a5723a3650mr10718931f8f.33.1750149003712; Tue, 17 Jun 2025 01:30:03 -0700 (PDT) Received: from raspberrypi.pitowers.org ([2a00:1098:3142:1f:ffc9:aff6:7f7f:893b]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4532e259108sm166062955e9.32.2025.06.17.01.30.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Jun 2025 01:30:03 -0700 (PDT) From: David Plowman To: libcamera-devel@lists.libcamera.org Cc: David Plowman Subject: [PATCH 4/7] ipa: rpi: Advance the delay context counter even when IPAs don't run Date: Tue, 17 Jun 2025 09:29:52 +0100 Message-Id: <20250617082956.5699-5-david.plowman@raspberrypi.com> X-Mailer: git-send-email 2.39.5 In-Reply-To: <20250617082956.5699-1-david.plowman@raspberrypi.com> References: <20250617082956.5699-1-david.plowman@raspberrypi.com> MIME-Version: 1.0 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" The delay context counter must be advanced even when we decide not to run prepare/process. Otherwise, when we skip them at higher framerates, the current IPA context counter will catch up and overwrite the delay context. It's safe to advance the counter because the metadata is always copied forward a slot when we decide not to run the IPAs. Signed-off-by: David Plowman --- src/ipa/rpi/common/ipa_base.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index 05699228..7af4ea5e 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -491,10 +491,11 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) void IpaBase::processStats(const ProcessParams ¶ms) { unsigned int ipaContext = params.ipaContext % rpiMetadata_.size(); + RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext]; - if (processPending_ && frameCount_ >= mistrustCount_) { - RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext]; + Duration offset(0s); + if (processPending_ && frameCount_ >= mistrustCount_) { auto it = buffers_.find(params.buffers.stats); if (it == buffers_.end()) { LOG(IPARPI, Error) << "Could not find stats buffer!"; @@ -510,7 +511,6 @@ void IpaBase::processStats(const ProcessParams ¶ms) controller_.process(statistics, &rpiMetadata); /* Send any sync algorithm outputs back to the pipeline handler */ - Duration offset(0s); struct SyncStatus syncStatus; if (rpiMetadata.get("sync.status", syncStatus) == 0) { if (minFrameDuration_ != maxFrameDuration_) @@ -522,14 +522,14 @@ void IpaBase::processStats(const ProcessParams ¶ms) if (syncStatus.timerKnown) libcameraMetadata_.set(controls::rpi::SyncTimer, syncStatus.timerValue); } + } - struct AgcStatus agcStatus; - if (rpiMetadata.get("agc.status", agcStatus) == 0) { - ControlList ctrls(sensorCtrls_); - applyAGC(&agcStatus, ctrls, offset); - setDelayedControls.emit(ctrls, ipaContext); - setCameraTimeoutValue(); - } + struct AgcStatus agcStatus; + if (rpiMetadata.get("agc.status", agcStatus) == 0) { + ControlList ctrls(sensorCtrls_); + applyAGC(&agcStatus, ctrls, offset); + setDelayedControls.emit(ctrls, ipaContext); + setCameraTimeoutValue(); } /* From patchwork Tue Jun 17 08:29:53 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Plowman X-Patchwork-Id: 23588 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 69DCABDE6B for ; Tue, 17 Jun 2025 08:30:18 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 7801968DDA; Tue, 17 Jun 2025 10:30:13 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="pMZU0uiu"; dkim-atps=neutral Received: from mail-wm1-x32a.google.com (mail-wm1-x32a.google.com [IPv6:2a00:1450:4864:20::32a]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id BE94E68DD4 for ; Tue, 17 Jun 2025 10:30:05 +0200 (CEST) Received: by mail-wm1-x32a.google.com with SMTP id 5b1f17b1804b1-451d41e1ad1so46129455e9.1 for ; Tue, 17 Jun 2025 01:30:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; t=1750149005; x=1750753805; darn=lists.libcamera.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=aYyKjOsdJLwWOcff0kElCz9r2+aJ89+xtpQUrkR4tfQ=; b=pMZU0uiuP8B4j9aPmVswXOxBxsrcYkGxvpY1kN87+pa2hbi9zT+F5JUulHFJ9bgvns crL3438fB7L8FfOvktxt7ldIxq2NiKNzvgTQUxkoMxjQJZ2F2Vf1IbatUxJfNb6eRhOy //B1T5Ol/NjmHbAuayFnBTwSRcutLPwtwqsY5VfqAwG9EasYEwJ8ZJqE05K0chHcqv3i +xWvYaHiZoL47/eygcyTOcuUShILLdxPMJ0io3a8NIYMu6uwwM3QX/4LYSI+mdRne73Z Y3otyyNcmzEuSdp5eZGDV0VUdiUtDmQlvcNXBVeWrodSQLZxNyS+FLvEcrLl7oIC79In RpZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750149005; x=1750753805; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=aYyKjOsdJLwWOcff0kElCz9r2+aJ89+xtpQUrkR4tfQ=; b=aMwvCRBU6cyS9ln7u/KZdkHXMsVTc5V0fYqdZkgUiydx5V4UA7XlfSGpQVb/vYzF4m BWu5R+ijhTswVJEyghdqSsmJVIU+W51EmCR1Nf+dgjwwzPZc37beJT4L5zRRYzeTmcq7 K+fUY9Aabj0omNCsnkt7Du5+cvykiEefCfoGAhQ4AFARFhK2Y68VtBW36B5rSGnyaXox ijoESv5iIVOKD0yJELU+DLGpFUWG9+vPzTA7WgpfSPif3jKioLcEIVcSrsgpGiR3IAFd aj2kBAELF0SVJ+L2iT0l0f3DwH7GhQiXtqtLoCCNMwsth5tEunZplAfDBcH4iuG0xShc CHoQ== X-Gm-Message-State: AOJu0YyVIC7L2gdSrkPyngCVMzN4oXodEofDrznyshPS7ORqhoOtGqud tF9Hkg1QIPWMih78fbx31NojJTP5auK/w0tERQ6CYUfkubiz2jFM33t/M0oqeihynhibEv/lyqB puA+f X-Gm-Gg: ASbGncsnDxGj/T4uMKHuBwskKOLHH2Azzw81XE86G8dnzP/u60BcTsvFmvfVfFUmPAD 2ge0czcVYsUzD9out5c/RHoSoyCt0EubhZbbQZcKa/r1Mq136bnkHXvqDDM5hqX/01yQLrqVG9f FZOlyFc0YZ/shxi7XEvUSERYt4i1Rjz8YhmNCv9UgCZ23wAYYs6KqjbePWFiiWhw/jRYtz7DcUr 3q1p9p1RPJIEBB2somKtDy8F0ArrBfCKVeC6fJa4Pf60WdZfkt052MHS6TjNOqIQfIYL1Li7z4p CdrlTgm0yYG7ZoHKt2mkKW1hjL7VuMgVckSATnyVkkjnEpUbhZbAgOh2Eny9esGGhY5tQu62Wmw IbRBnB7MJUZhW/LyApetTMXHkhtsJ X-Google-Smtp-Source: AGHT+IGvh/cZvHSWOsx54Fnh2Mk5IyMt4pToui5MLbb+fhDXDraeFy+FqWzwkVBje/pTDpKRkoaGUg== X-Received: by 2002:a05:600c:1c0a:b0:453:b44:eb71 with SMTP id 5b1f17b1804b1-4533caa2114mr110052405e9.19.1750149004779; Tue, 17 Jun 2025 01:30:04 -0700 (PDT) Received: from raspberrypi.pitowers.org ([2a00:1098:3142:1f:ffc9:aff6:7f7f:893b]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4532e259108sm166062955e9.32.2025.06.17.01.30.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Jun 2025 01:30:03 -0700 (PDT) From: David Plowman To: libcamera-devel@lists.libcamera.org Cc: David Plowman Subject: [PATCH 5/7] ipa: rpi: agc: Calculate digital gain in process() Date: Tue, 17 Jun 2025 09:29:53 +0100 Message-Id: <20250617082956.5699-6-david.plowman@raspberrypi.com> X-Mailer: git-send-email 2.39.5 In-Reply-To: <20250617082956.5699-1-david.plowman@raspberrypi.com> References: <20250617082956.5699-1-david.plowman@raspberrypi.com> MIME-Version: 1.0 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" Previously we let prepare() do the work by comparing the desired total exposure against the shutter time and analogue gain. This can cause the image to "wink" at high framerates because we may skip running prepare() to get the new digital gain even when the delayed AGC status (which came out of an earlier call to process()) shows that a change was required. Now we're taking explicit control of the digital gain by calculating it ourselves so that we can output it in the standard AgcStatus object. This means that whenever the delayed AGC status changes, we have the correct digital gain to go with it. Signed-off-by: David Plowman --- src/ipa/rpi/controller/agc_status.h | 1 + src/ipa/rpi/controller/rpi/agc_channel.cpp | 163 +++++++++------------ src/ipa/rpi/controller/rpi/agc_channel.h | 1 + 3 files changed, 75 insertions(+), 90 deletions(-) diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h index d4cedcf4..956d6abf 100644 --- a/src/ipa/rpi/controller/agc_status.h +++ b/src/ipa/rpi/controller/agc_status.h @@ -30,6 +30,7 @@ struct AgcStatus { libcamera::utils::Duration targetExposureValue; /* (unfiltered) target total exposure AGC is aiming for */ libcamera::utils::Duration exposureTime; double analogueGain; + double digitalGain; std::string exposureMode; std::string constraintMode; std::string meteringMode; diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp index 1028d82e..3829534c 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp @@ -266,7 +266,7 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms) } AgcChannel::ExposureValues::ExposureValues() - : exposureTime(0s), analogueGain(0), + : exposureTime(0s), analogueGain(0), digitalGain(0), totalExposure(0s), totalExposureNoDG(0s) { } @@ -434,17 +434,10 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, mode_ = cameraMode; Duration fixedExposureTime = limitExposureTime(fixedExposureTime_); + double fixedGain = limitGain(fixedGain_); if (fixedExposureTime && fixedGain_) { - /* This is the equivalent of computeTargetExposure and applyDigitalGain. */ - target_.totalExposureNoDG = fixedExposureTime_ * fixedGain_; - target_.totalExposure = target_.totalExposureNoDG; - - /* Equivalent of filterExposure. This resets any "history". */ - filtered_ = target_; - - /* Equivalent of divideUpExposure. */ - filtered_.exposureTime = fixedExposureTime; - filtered_.analogueGain = fixedGain_; + filtered_.totalExposureNoDG = fixedExposureTime * fixedGain; + filtered_.totalExposure = filtered_.totalExposureNoDG; } else if (status_.totalExposureValue) { /* * On a mode switch, various things could happen: @@ -457,12 +450,8 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, */ double ratio = lastSensitivity / cameraMode.sensitivity; - target_.totalExposure *= ratio; - target_.totalExposureNoDG = target_.totalExposure; filtered_.totalExposure *= ratio; filtered_.totalExposureNoDG = filtered_.totalExposure; - - divideUpExposure(); } else { /* * We come through here on startup, when at least one of the @@ -472,55 +461,46 @@ void AgcChannel::switchMode(CameraMode const &cameraMode, * weren't set. */ - /* Equivalent of divideUpExposure. */ - filtered_.exposureTime = fixedExposureTime ? fixedExposureTime : config_.defaultExposureTime; - filtered_.analogueGain = fixedGain_ ? fixedGain_ : config_.defaultAnalogueGain; + Duration exposureTime = fixedExposureTime ? fixedExposureTime : config_.defaultExposureTime; + double gain = fixedGain ? fixedGain : config_.defaultAnalogueGain; + filtered_.totalExposure = exposureTime * gain; + filtered_.totalExposureNoDG = filtered_.totalExposure; } + /* Setting target_ to filtered_ removes any history from before the mode switch. */ + target_ = filtered_; + divideUpExposure(); + writeAndFinish(metadata, false); } void AgcChannel::prepare(Metadata *imageMetadata) { - Duration totalExposureValue = status_.totalExposureValue; - AgcStatus delayedStatus; + DeviceStatus deviceStatus; AgcPrepareStatus prepareStatus; - /* Fetch the AWB status now because AWB also sets it in the prepare method. */ - fetchAwbStatus(imageMetadata); - - if (!imageMetadata->get("agc.delayed_status", delayedStatus)) - totalExposureValue = delayedStatus.totalExposureValue; - - prepareStatus.digitalGain = 1.0; prepareStatus.locked = false; + prepareStatus.digitalGain = 1.0; - if (status_.totalExposureValue) { - /* Process has run, so we have meaningful values. */ - DeviceStatus deviceStatus; - if (imageMetadata->get("device.status", deviceStatus) == 0) { - Duration actualExposure = deviceStatus.exposureTime * - deviceStatus.analogueGain; - if (actualExposure) { - double digitalGain = totalExposureValue / actualExposure; - LOG(RPiAgc, Debug) << "Want total exposure " << totalExposureValue; - /* - * Never ask for a gain < 1.0, and also impose - * some upper limit. Make it customisable? - */ - prepareStatus.digitalGain = std::max(1.0, std::min(digitalGain, - config_.maxDigitalGain)); - LOG(RPiAgc, Debug) << "Actual exposure " << actualExposure; - LOG(RPiAgc, Debug) << "Use digitalGain " << prepareStatus.digitalGain; - LOG(RPiAgc, Debug) << "Effective exposure " - << actualExposure * prepareStatus.digitalGain; - /* Decide whether AEC/AGC has converged. */ - prepareStatus.locked = updateLockStatus(deviceStatus); - } - } else - LOG(RPiAgc, Warning) << "AgcChannel: no device metadata"; - imageMetadata->set("agc.prepare_status", prepareStatus); + if (!imageMetadata->get("device.status", deviceStatus)) { + prepareStatus.locked = updateLockStatus(deviceStatus); + + /* + * For now, the IPA code is still expecting the digital gain to come back in + * the prepare_status. To keep things happy, we'll just fill in the value that + * we calculated previously and put in the AgcStatus (which comes back as the + * "delayed" status). Once the rest of the IPA code is updated, we'll be able + * to remove this, and indeed remove the digitalGain from the AgcPrepareStatus. + */ + AgcStatus delayedStatus; + if (!imageMetadata->get("agc.delayed_status", delayedStatus)) + prepareStatus.digitalGain = delayedStatus.digitalGain; + else + /* After a mode switch, this must be correct until new values come through. */ + prepareStatus.digitalGain = status_.digitalGain; } + + imageMetadata->set("agc.prepare_status", prepareStatus); } void AgcChannel::process(StatisticsPtr &stats, DeviceStatus const &deviceStatus, @@ -606,7 +586,7 @@ void AgcChannel::housekeepConfig() /* First fetch all the up-to-date settings, so no one else has to do it. */ status_.ev = ev_; status_.fixedExposureTime = limitExposureTime(fixedExposureTime_); - status_.fixedGain = fixedGain_; + status_.fixedGain = limitGain(fixedGain_); status_.flickerPeriod = flickerPeriod_; LOG(RPiAgc, Debug) << "ev " << status_.ev << " fixedExposureTime " << status_.fixedExposureTime << " fixedGain " @@ -657,6 +637,9 @@ void AgcChannel::fetchCurrentExposure(DeviceStatus const &deviceStatus) current_.analogueGain = deviceStatus.analogueGain; current_.totalExposure = 0s; /* this value is unused */ current_.totalExposureNoDG = current_.exposureTime * current_.analogueGain; + LOG(RPiAgc, Debug) << "Current frame: exposure time " << current_.exposureTime + << " ag " << current_.analogueGain + << " (total " << current_.totalExposureNoDG << ")"; } void AgcChannel::fetchAwbStatus(Metadata *imageMetadata) @@ -808,14 +791,12 @@ void AgcChannel::computeTargetExposure(double gain) target_.totalExposure = current_.totalExposureNoDG * gain; /* The final target exposure is also limited to what the exposure mode allows. */ Duration maxExposureTime = status_.fixedExposureTime - ? status_.fixedExposureTime - : exposureMode_->exposureTime.back(); + ? status_.fixedExposureTime + : exposureMode_->exposureTime.back(); maxExposureTime = limitExposureTime(maxExposureTime); - Duration maxTotalExposure = - maxExposureTime * - (status_.fixedGain != 0.0 - ? status_.fixedGain - : exposureMode_->gain.back()); + double maxGain = status_.fixedGain ? status_.fixedGain : exposureMode_->gain.back(); + maxGain = limitGain(maxGain); + Duration maxTotalExposure = maxExposureTime * maxGain; target_.totalExposure = std::min(target_.totalExposure, maxTotalExposure); } LOG(RPiAgc, Debug) << "Target totalExposure " << target_.totalExposure; @@ -824,8 +805,6 @@ void AgcChannel::computeTargetExposure(double gain) bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channelTotalExposures) { bool channelBound = false; - LOG(RPiAgc, Debug) - << "Total exposure before channel constraints " << filtered_.totalExposure; for (const auto &constraint : config_.channelConstraints) { LOG(RPiAgc, Debug) @@ -860,7 +839,7 @@ bool AgcChannel::applyChannelConstraints(const AgcChannelTotalExposures &channel bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound) { - double dg = 1.0; + filtered_.totalExposureNoDG = filtered_.totalExposure; /* * Finally, if we're trying to reduce exposure but the target_Y is @@ -871,15 +850,14 @@ bool AgcChannel::applyDigitalGain(double gain, double targetY, bool channelBound * quickly (and we then approach the correct value more quickly from * below). */ - bool desaturate = false; - if (config_.desaturate) - desaturate = !channelBound && - targetY > config_.fastReduceThreshold && gain < sqrt(targetY); - if (desaturate) - dg /= config_.fastReduceThreshold; - LOG(RPiAgc, Debug) << "Digital gain " << dg << " desaturate? " << desaturate; - filtered_.totalExposureNoDG = filtered_.totalExposure / dg; - LOG(RPiAgc, Debug) << "Target totalExposureNoDG " << filtered_.totalExposureNoDG; + bool desaturate = config_.desaturate && !channelBound && + targetY > config_.fastReduceThreshold && gain < sqrt(targetY); + + if (desaturate) { + filtered_.totalExposureNoDG *= config_.fastReduceThreshold; + LOG(RPiAgc, Debug) << "Desaturating, exposure no dg " << filtered_.totalExposureNoDG; + } + return desaturate; } @@ -915,8 +893,7 @@ void AgcChannel::filterExposure() filtered_.totalExposure = speed * target_.totalExposure + filtered_.totalExposure * (1.0 - speed); } - LOG(RPiAgc, Debug) << "After filtering, totalExposure " << filtered_.totalExposure - << " no dg " << filtered_.totalExposureNoDG; + LOG(RPiAgc, Debug) << "After filtering, totalExposure " << filtered_.totalExposure; } void AgcChannel::divideUpExposure() @@ -957,9 +934,7 @@ void AgcChannel::divideUpExposure() } } } - LOG(RPiAgc, Debug) - << "Divided up exposure time and gain are " << exposureTime - << " and " << gain; + /* * Finally adjust exposure time for flicker avoidance (require both * exposure time and gain not to be fixed). @@ -970,22 +945,30 @@ void AgcChannel::divideUpExposure() if (flickerPeriods) { Duration newExposureTime = flickerPeriods * status_.flickerPeriod; gain *= exposureTime / newExposureTime; - /* - * We should still not allow the ag to go over the - * largest value in the exposure mode. Note that this - * may force more of the total exposure into the digital - * gain as a side-effect. - */ - gain = std::min(gain, exposureMode_->gain.back()); - gain = limitGain(gain); exposureTime = newExposureTime; } LOG(RPiAgc, Debug) << "After flicker avoidance, exposure time " << exposureTime << " gain " << gain; } + + /* Limit analogue gain to maximum allowed. */ + double analogueGain = std::min(gain, mode_.maxAnalogueGain); + + /* Finally work out the digital gain that we will need. */ + filtered_.totalExposureNoDG = analogueGain * exposureTime; + double digitalGain = filtered_.totalExposure / filtered_.totalExposureNoDG; + /* Limit dg by what is allowed. */ + digitalGain = std::min(digitalGain, config_.maxDigitalGain); + /* Update total exposure, in case the dg went down. */ + filtered_.totalExposure = filtered_.totalExposureNoDG * digitalGain; + filtered_.exposureTime = exposureTime; - /* We ask for all the gain as analogue gain; prepare() will be told what we got. */ - filtered_.analogueGain = gain; + filtered_.analogueGain = analogueGain; + filtered_.digitalGain = digitalGain; + LOG(RPiAgc, Debug) << "DivideUpExposure: total " << filtered_.totalExposure + << " no dg " << filtered_.totalExposureNoDG; + LOG(RPiAgc, Debug) << "DivideUpExposure: exp " << exposureTime + << " ag " << gain << " dg " << digitalGain; } void AgcChannel::writeAndFinish(Metadata *imageMetadata, bool desaturate) @@ -994,6 +977,7 @@ void AgcChannel::writeAndFinish(Metadata *imageMetadata, bool desaturate) status_.targetExposureValue = desaturate ? 0s : target_.totalExposure; status_.exposureTime = filtered_.exposureTime; status_.analogueGain = filtered_.analogueGain; + status_.digitalGain = filtered_.digitalGain; /* * Write to metadata as well, in case anyone wants to update the camera * immediately. @@ -1001,8 +985,6 @@ void AgcChannel::writeAndFinish(Metadata *imageMetadata, bool desaturate) imageMetadata->set("agc.status", status_); LOG(RPiAgc, Debug) << "Output written, total exposure requested is " << filtered_.totalExposure; - LOG(RPiAgc, Debug) << "Camera exposure update: exposure time " << filtered_.exposureTime - << " analogue gain " << filtered_.analogueGain; } Duration AgcChannel::limitExposureTime(Duration exposureTime) @@ -1031,6 +1013,7 @@ double AgcChannel::limitGain(double gain) const if (!gain) return gain; - gain = std::max(gain, mode_.minAnalogueGain); + gain = std::clamp(gain, mode_.minAnalogueGain, + mode_.maxAnalogueGain * config_.maxDigitalGain); return gain; } diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h index 93229128..42d85ec1 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.h +++ b/src/ipa/rpi/controller/rpi/agc_channel.h @@ -135,6 +135,7 @@ private: libcamera::utils::Duration exposureTime; double analogueGain; + double digitalGain; libcamera::utils::Duration totalExposure; libcamera::utils::Duration totalExposureNoDG; /* without digital gain */ }; From patchwork Tue Jun 17 08:29:54 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Plowman X-Patchwork-Id: 23589 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 63987BDE6B for ; Tue, 17 Jun 2025 08:30:19 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id B881C68DD4; Tue, 17 Jun 2025 10:30:15 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="ippmbL5n"; dkim-atps=neutral Received: from mail-wr1-x430.google.com (mail-wr1-x430.google.com [IPv6:2a00:1450:4864:20::430]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id B7B0D68DD8 for ; Tue, 17 Jun 2025 10:30:06 +0200 (CEST) Received: by mail-wr1-x430.google.com with SMTP id ffacd0b85a97d-3a548a73ff2so4976803f8f.0 for ; Tue, 17 Jun 2025 01:30:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; t=1750149006; x=1750753806; darn=lists.libcamera.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=Ka6w8XlXuyTMARzl8Nb760HyjvSz5j6FevThoPwy1Bo=; b=ippmbL5nQsnxFthvtSSkfZtIyCBhYiLex/J2ISEwnDcDFYcS0Kxz3oOim+MzbgZWX2 BuvZRQkDSdPQMu4fuOiDatVFLaWEPRfgbsCIJCCZ3lnutyF9hgwtDxSd02m8gELqoUvh jIoGr5pojQ7RWG66GG0PkX7mkil+TLvwqx27YEJwxgYel88EbEFu7qvYn1vLUZ2i5iZe GGy1rPHWhCc7MC4vNN/QjA3jHYYYZ/yLXwX4KYUY4v7sWEo0VdT1me80cSr4yhb3AT9T iROV8Qpz4aNPvL0pyXg5dwIfCpYPiWjG+2yTklqntO7NtqFV7nf3awUnF4NCW4iaJV4Z OO7A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750149006; x=1750753806; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Ka6w8XlXuyTMARzl8Nb760HyjvSz5j6FevThoPwy1Bo=; b=JLSCTMRnzT9z9AKoZFKs8wHJUTKoOOVkUzsFYzzypX5UOmZqkyVh0MJnl9fJFNdJfo doNSjeHQYCKJFE8lysEXvyck+MAm2hiHJyJtuf+rCjQv1tgpG6gBoL/NwXEykmHFdC1Z WIEQjWUJwm5kPd97G4zILxXcasEV6U3z9HJ1Uv1f48MQSNnhg2SWbv8+XAthGwS8yrdG d9Arfbqef4FlXMDVaoJkWCNQqLrMk2AE5t6sqcghqxirJ/J0rVmgdlr3Ge/WWmypXNZy EfBWjyVTW4FoQOz9ZemSZhSReeAjcZG1UC0Zayd8wDyWDWqyuUJ0sKyjkCLW7XbTO2Ae XHpw== X-Gm-Message-State: AOJu0YxzxRyp910Pp7qUCL5RBVwBXHw9sUuvh9WgTsp9Po5+NbVtXHy9 P9d+VVLoHq9wwryboD588HXOKIYJL9JFRp07RHEAtZuw29Oq6b2mjlSn1ExLgkhRo3rnTph7rzR AzY8V X-Gm-Gg: ASbGnctZKctboSULaUOjYHxPMessMD8v6AQm4fvMpJS2p7fI5QUhCw+JaVvEF6cor+Y r1pa631c4fo3bihgyGTADQ6uZVzwMNEc/YS8dV8cul48N97SdWYfi5SAzvOMtPnK/Z4I7RNwMBK PG39TqgB9zjMbcJIgqfYrCVqUgvk5kWZp9gkae7NSsBcXs6i+QIJiMjtzugbVdGnw20T5Oiaui3 xOCH2zKmwdrdKDwhYvJa2Lj23m1ue6s3yx8czHmwT6pNg40aGfg1kosbafgmrMLN3R9Udia4MbW mZO/RJQgnsDsGhZp/+/jQ9H/MQ8ADAX1g+BPJfz9hxjvHoWPbYM5y2n1ipOt//Uyqfpy44mLDoZ jO4RWq13/cmU7Xj8VQQ== X-Google-Smtp-Source: AGHT+IGYHsSHGmlki6gW/buxyX97m5rntrA5Bo0C08oXWbswIRROrtJ9wXtFnwzTBJObNc+c4iGkCA== X-Received: by 2002:a05:6000:25eb:b0:3a3:65b5:51d7 with SMTP id ffacd0b85a97d-3a572e2df7bmr11058480f8f.26.1750149005514; Tue, 17 Jun 2025 01:30:05 -0700 (PDT) Received: from raspberrypi.pitowers.org ([2a00:1098:3142:1f:ffc9:aff6:7f7f:893b]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4532e259108sm166062955e9.32.2025.06.17.01.30.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Jun 2025 01:30:05 -0700 (PDT) From: David Plowman To: libcamera-devel@lists.libcamera.org Cc: David Plowman Subject: [PATCH 6/7] ipa: rpi: Update digital gain handling in IPA base and derived classes Date: Tue, 17 Jun 2025 09:29:54 +0100 Message-Id: <20250617082956.5699-7-david.plowman@raspberrypi.com> X-Mailer: git-send-email 2.39.5 In-Reply-To: <20250617082956.5699-1-david.plowman@raspberrypi.com> References: <20250617082956.5699-1-david.plowman@raspberrypi.com> MIME-Version: 1.0 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" Here we update the digital gain handling to use the value computed by process() in the AgcStatus, not the version that was previously in the AgcPrepareStatus. Because we apply this digital gain directly with no further modification, we have to update it to reflect any exposure/gain quantisation that happens (in IpaBase::applyAGC). We must also run the new platformPrepareAgc() even when we're skipping platformPrepareIsp(), which has been split out of the previous platformPrepareIsp() implementation. Signed-off-by: David Plowman --- src/ipa/rpi/common/ipa_base.cpp | 50 +++++++++++++++++++-------- src/ipa/rpi/common/ipa_base.h | 6 +++- src/ipa/rpi/pisp/pisp.cpp | 61 +++++++++++++++++++++------------ src/ipa/rpi/vc4/vc4.cpp | 28 +++++++++------ 4 files changed, 98 insertions(+), 47 deletions(-) diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index 7af4ea5e..22aadeba 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -309,20 +309,23 @@ void IpaBase::start(const ControlList &controls, StartResult *result) frameLengths_.clear(); frameLengths_.resize(FrameLengthsQueueSize, 0s); - /* SwitchMode may supply updated exposure/gain values to use. */ - AgcStatus agcStatus; - agcStatus.exposureTime = 0.0s; - agcStatus.analogueGain = 0.0; + /* + * SwitchMode may supply updated exposure/gain values to use. + * agcStatus_ will store these values for us to use until delayed_status values + * start to appear. + */ + agcStatus_.exposureTime = 0.0s; + agcStatus_.analogueGain = 0.0; - metadata.get("agc.status", agcStatus); - if (agcStatus.exposureTime && agcStatus.analogueGain) { + metadata.get("agc.status", agcStatus_); + if (agcStatus_.exposureTime && agcStatus_.analogueGain) { ControlList ctrls(sensorCtrls_); - applyAGC(&agcStatus, ctrls); + applyAGC(&agcStatus_, ctrls); result->controls = std::move(ctrls); setCameraTimeoutValue(); } /* Make a note of this as it tells us the HDR status of the first few frames. */ - hdrStatus_ = agcStatus.hdr; + hdrStatus_ = agcStatus_.hdr; /* * Initialise frame counts, and decide how many frames must be hidden or @@ -476,7 +479,9 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) controller_.prepare(&rpiMetadata); /* Actually prepare the ISP parameters for the frame. */ platformPrepareIsp(params, rpiMetadata); - } + platformPrepareAgc(rpiMetadata); + } else + platformPrepareAgc(rpiMetadata); frameCount_++; @@ -528,6 +533,7 @@ void IpaBase::processStats(const ProcessParams ¶ms) if (rpiMetadata.get("agc.status", agcStatus) == 0) { ControlList ctrls(sensorCtrls_); applyAGC(&agcStatus, ctrls, offset); + rpiMetadata.set("agc.status", agcStatus); setDelayedControls.emit(ctrls, ipaContext); setCameraTimeoutValue(); } @@ -1472,9 +1478,6 @@ void IpaBase::reportMetadata(unsigned int ipaContext) } AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked("agc.prepare_status"); - if (agcPrepareStatus) - libcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain); - RPiController::AgcAlgorithm *agc = dynamic_cast( controller_.getAlgorithm("agc")); if (agc) { @@ -1487,6 +1490,13 @@ void IpaBase::reportMetadata(unsigned int ipaContext) : controls::AeStateSearching); } + const AgcStatus *agcStatus = rpiMetadata.getLocked("agc.delayed_status"); + if (agcStatus) + libcameraMetadata_.set(controls::DigitalGain, agcStatus->digitalGain); + else + libcameraMetadata_.set(controls::DigitalGain, agcStatus_.digitalGain); + /* The HDR metadata reporting will use this agcStatus too. */ + LuxStatus *luxStatus = rpiMetadata.getLocked("lux.status"); if (luxStatus) libcameraMetadata_.set(controls::Lux, luxStatus->lux); @@ -1576,7 +1586,6 @@ void IpaBase::reportMetadata(unsigned int ipaContext) * delayed_status to be available, we use the HDR status that came out of the * switchMode call. */ - const AgcStatus *agcStatus = rpiMetadata.getLocked("agc.delayed_status"); const HdrStatus &hdrStatus = agcStatus ? agcStatus->hdr : hdrStatus_; if (!hdrStatus.mode.empty() && hdrStatus.mode != "Off") { int32_t hdrMode = controls::HdrModeOff; @@ -1686,7 +1695,7 @@ void IpaBase::applyFrameDurations(Duration minFrameDuration, Duration maxFrameDu } } -void IpaBase::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls, Duration frameDurationOffset) +void IpaBase::applyAGC(struct AgcStatus *agcStatus, ControlList &ctrls, Duration frameDurationOffset) { const int32_t minGainCode = helper_->gainCode(mode_.minAnalogueGain); const int32_t maxGainCode = helper_->gainCode(mode_.maxAnalogueGain); @@ -1716,6 +1725,19 @@ void IpaBase::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls, Du ctrls.set(V4L2_CID_EXPOSURE, exposureLines); ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode); + /* + * We must update the digital gain to make up for any quantisation that happens, and + * communicate that back into the metadata so that it will appear as the "delayed" status. + * (Note that "exposure" is already the "actual" exposure.) + */ + double actualGain = helper_->gain(gainCode); + double ratio = agcStatus->analogueGain / actualGain; + ratio *= agcStatus->exposureTime / exposure; + double newDigitalGain = agcStatus->digitalGain * ratio; + agcStatus->digitalGain = newDigitalGain; + agcStatus->analogueGain = actualGain; + agcStatus->exposureTime = exposure; + /* * At present, there is no way of knowing if a control is read-only. * As a workaround, assume that if the minimum and maximum values of diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h index ed378f45..943a3c91 100644 --- a/src/ipa/rpi/common/ipa_base.h +++ b/src/ipa/rpi/common/ipa_base.h @@ -73,6 +73,9 @@ protected: /* Remember the HDR status after a mode switch. */ HdrStatus hdrStatus_; + /* Remember the AGC status after a mode switch. */ + AgcStatus agcStatus_; + /* Whether the stitch block (if available) needs to swap buffers. */ bool stitchSwapBuffers_; @@ -86,6 +89,7 @@ private: virtual void platformPrepareIsp(const PrepareParams ¶ms, RPiController::Metadata &rpiMetadata) = 0; + virtual void platformPrepareAgc(RPiController::Metadata &rpiMetadata) = 0; virtual RPiController::StatisticsPtr platformProcessStats(Span mem) = 0; void setMode(const IPACameraSensorInfo &sensorInfo); @@ -98,7 +102,7 @@ private: void fillSyncParams(const PrepareParams ¶ms, unsigned int ipaContext); void reportMetadata(unsigned int ipaContext); void applyFrameDurations(utils::Duration minFrameDuration, utils::Duration maxFrameDuration); - void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls, + void applyAGC(struct AgcStatus *agcStatus, ControlList &ctrls, utils::Duration frameDurationOffset = utils::Duration(0)); std::map buffers_; diff --git a/src/ipa/rpi/pisp/pisp.cpp b/src/ipa/rpi/pisp/pisp.cpp index e1a804f5..ab70d8f4 100644 --- a/src/ipa/rpi/pisp/pisp.cpp +++ b/src/ipa/rpi/pisp/pisp.cpp @@ -218,13 +218,14 @@ private: void platformPrepareIsp(const PrepareParams ¶ms, RPiController::Metadata &rpiMetadata) override; + void platformPrepareAgc(RPiController::Metadata &rpiMetadata) override; RPiController::StatisticsPtr platformProcessStats(Span mem) override; void handleControls(const ControlList &controls) override; - void applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcStatus, + void applyWBG(const AwbStatus *awbStatus, double digitalGain, pisp_be_global_config &global); - void applyDgOnly(const AgcPrepareStatus *agcPrepareStatus, pisp_be_global_config &global); + void applyDgOnly(double digitalGain, pisp_be_global_config &global); void applyCAC(const CacStatus *cacStatus, pisp_be_global_config &global); void applyContrast(const ContrastStatus *contrastStatus, pisp_be_global_config &global); @@ -341,7 +342,6 @@ void IpaPiSP::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, PISP_BE_RGB_ENABLE_SHARPEN + PISP_BE_RGB_ENABLE_SAT_CONTROL); NoiseStatus *noiseStatus = rpiMetadata.getLocked("noise.status"); - AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked("agc.prepare_status"); { /* All Frontend config goes first, we do not want to hold the FE lock for long! */ @@ -355,14 +355,6 @@ void IpaPiSP::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, if (blackLevelStatus) applyBlackLevel(blackLevelStatus, global); - AwbStatus *awbStatus = rpiMetadata.getLocked("awb.status"); - if (awbStatus && agcPrepareStatus) { - /* Applies digital gain as well. */ - applyWBG(awbStatus, agcPrepareStatus, global); - } else if (agcPrepareStatus) { - /* Mono sensor fallback for digital gain. */ - applyDgOnly(agcPrepareStatus, global); - } } CacStatus *cacStatus = rpiMetadata.getLocked("cac.status"); @@ -443,6 +435,34 @@ void IpaPiSP::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, } } +void IpaPiSP::platformPrepareAgc(RPiController::Metadata &rpiMetadata) +{ + std::scoped_lock l(rpiMetadata); + + AgcStatus *delayedAgcStatus = rpiMetadata.getLocked("agc.delayed_status"); + /* If no delayed status, use the gain from the last mode switch. */ + double digitalGain = delayedAgcStatus ? delayedAgcStatus->digitalGain : agcStatus_.digitalGain; + AwbStatus *awbStatus = rpiMetadata.getLocked("awb.status"); + + pisp_be_global_config global; + be_->GetGlobal(global); + + { + /* All Frontend config goes first, we do not want to hold the FE lock for long! */ + std::scoped_lock lf(*fe_); + + if (awbStatus) { + /* Applies digital gain as well. */ + applyWBG(awbStatus, digitalGain, global); + } else { + /* Mono sensor fallback for digital gain. */ + applyDgOnly(digitalGain, global); + } + } + + be_->SetGlobal(global); +} + RPiController::StatisticsPtr IpaPiSP::platformProcessStats(Span mem) { using namespace RPiController; @@ -515,12 +535,11 @@ void IpaPiSP::handleControls(const ControlList &controls) } } -void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPrepareStatus, +void IpaPiSP::applyWBG(const AwbStatus *awbStatus, double digitalGain, pisp_be_global_config &global) { pisp_wbg_config wbg; pisp_fe_rgby_config rgby = {}; - double dg = agcPrepareStatus ? agcPrepareStatus->digitalGain : 1.0; double minColourGain = std::min({ awbStatus->gainR, awbStatus->gainG, awbStatus->gainB, 1.0 }); /* The 0.1 here doesn't mean much, but just stops arithmetic errors and extreme behaviour. */ double extraGain = 1.0 / std::max({ minColourGain, 0.1 }); @@ -536,9 +555,9 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr double gainG = awbStatus->gainG * extraGain; double gainB = awbStatus->gainB * extraGain; - wbg.gain_r = clampField(dg * gainR, 14, 10); - wbg.gain_g = clampField(dg * gainG, 14, 10); - wbg.gain_b = clampField(dg * gainB, 14, 10); + wbg.gain_r = clampField(digitalGain * gainR, 14, 10); + wbg.gain_g = clampField(digitalGain * gainG, 14, 10); + wbg.gain_b = clampField(digitalGain * gainB, 14, 10); /* * The YCbCr conversion block should contain the appropriate YCbCr @@ -561,15 +580,15 @@ void IpaPiSP::applyWBG(const AwbStatus *awbStatus, const AgcPrepareStatus *agcPr global.bayer_enables |= PISP_BE_BAYER_ENABLE_WBG; } -void IpaPiSP::applyDgOnly(const AgcPrepareStatus *agcPrepareStatus, pisp_be_global_config &global) +void IpaPiSP::applyDgOnly(double digitalGain, pisp_be_global_config &global) { pisp_wbg_config wbg; - wbg.gain_r = clampField(agcPrepareStatus->digitalGain, 14, 10); - wbg.gain_g = clampField(agcPrepareStatus->digitalGain, 14, 10); - wbg.gain_b = clampField(agcPrepareStatus->digitalGain, 14, 10); + wbg.gain_r = clampField(digitalGain, 14, 10); + wbg.gain_g = clampField(digitalGain, 14, 10); + wbg.gain_b = clampField(digitalGain, 14, 10); - LOG(IPARPI, Debug) << "Applying DG (only) : " << agcPrepareStatus->digitalGain; + LOG(IPARPI, Debug) << "Applying DG (only) : " << digitalGain; be_->SetWbg(wbg); global.bayer_enables |= PISP_BE_BAYER_ENABLE_WBG; diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp index 8a7a37c8..b2fec934 100644 --- a/src/ipa/rpi/vc4/vc4.cpp +++ b/src/ipa/rpi/vc4/vc4.cpp @@ -57,14 +57,14 @@ private: int32_t platformConfigure(const ConfigParams ¶ms, ConfigResult *result) override; void platformPrepareIsp(const PrepareParams ¶ms, RPiController::Metadata &rpiMetadata) override; + void platformPrepareAgc([[maybe_unused]] RPiController::Metadata &rpiMetadata) override; RPiController::StatisticsPtr platformProcessStats(Span mem) override; void handleControls(const ControlList &controls) override; bool validateIspControls(); void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls); - void applyDG(const struct AgcPrepareStatus *dgStatus, - const struct AwbStatus *awbStatus, ControlList &ctrls); + void applyDG(double digitalGain, const struct AwbStatus *awbStatus, ControlList &ctrls); void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls); void applyBlackLevel(const struct BlackLevelStatus *blackLevelStatus, ControlList &ctrls); void applyGamma(const struct ContrastStatus *contrastStatus, ControlList &ctrls); @@ -78,6 +78,7 @@ private: /* VC4 ISP controls. */ ControlInfoMap ispCtrls_; + ControlList ctrls_; /* LS table allocation passed in from the pipeline handler. */ SharedFD lsTableHandle_; @@ -107,6 +108,7 @@ int32_t IpaVc4::platformStart([[maybe_unused]] const ControlList &controls, int32_t IpaVc4::platformConfigure(const ConfigParams ¶ms, [[maybe_unused]] ConfigResult *result) { ispCtrls_ = params.ispControls; + ctrls_ = ControlList(ispCtrls_); if (!validateIspControls()) { LOG(IPARPI, Error) << "ISP control validation failed."; return -1; @@ -139,7 +141,7 @@ int32_t IpaVc4::platformConfigure(const ConfigParams ¶ms, [[maybe_unused]] C void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, RPiController::Metadata &rpiMetadata) { - ControlList ctrls(ispCtrls_); + ControlList &ctrls = ctrls_; /* Lock the metadata buffer to avoid constant locks/unlocks. */ std::unique_lock lock(rpiMetadata); @@ -152,9 +154,6 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, if (ccmStatus) applyCCM(ccmStatus, ctrls); - AgcPrepareStatus *dgStatus = rpiMetadata.getLocked("agc.prepare_status"); - applyDG(dgStatus, awbStatus, ctrls); - AlscStatus *lsStatus = rpiMetadata.getLocked("alsc.status"); if (lsStatus) applyLS(lsStatus, ctrls); @@ -190,9 +189,18 @@ void IpaVc4::platformPrepareIsp([[maybe_unused]] const PrepareParams ¶ms, if (!lensctrls.empty()) setLensControls.emit(lensctrls); } +} - if (!ctrls.empty()) - setIspControls.emit(ctrls); +void IpaVc4::platformPrepareAgc(RPiController::Metadata &rpiMetadata) +{ + AgcStatus *delayedAgcStatus = rpiMetadata.getLocked("agc.delayed_status"); + double digitalGain = delayedAgcStatus ? delayedAgcStatus->digitalGain : agcStatus_.digitalGain; + AwbStatus *awbStatus = rpiMetadata.getLocked("awb.status"); + + applyDG(digitalGain, awbStatus, ctrls_); + + setIspControls.emit(ctrls_); + ctrls_ = ControlList(ispCtrls_); } RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span mem) @@ -329,11 +337,9 @@ void IpaVc4::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) static_cast(awbStatus->gainB * 1000)); } -void IpaVc4::applyDG(const struct AgcPrepareStatus *dgStatus, +void IpaVc4::applyDG(double digitalGain, const struct AwbStatus *awbStatus, ControlList &ctrls) { - double digitalGain = dgStatus ? dgStatus->digitalGain : 1.0; - if (awbStatus) { /* * We must apply sufficient extra digital gain to stop any of the channel gains being From patchwork Tue Jun 17 08:29:55 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Plowman X-Patchwork-Id: 23590 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 1B7B4C3240 for ; Tue, 17 Jun 2025 08:30:20 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id C762D68DDF; Tue, 17 Jun 2025 10:30:16 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=raspberrypi.com header.i=@raspberrypi.com header.b="hNA+atFZ"; dkim-atps=neutral Received: from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com [IPv6:2a00:1450:4864:20::42c]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 20DCB68DD9 for ; Tue, 17 Jun 2025 10:30:07 +0200 (CEST) Received: by mail-wr1-x42c.google.com with SMTP id ffacd0b85a97d-3a507e88b0aso5246176f8f.1 for ; Tue, 17 Jun 2025 01:30:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=raspberrypi.com; s=google; t=1750149006; x=1750753806; darn=lists.libcamera.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=RmFQZIIJ4r2zWdK/Ht3tViSq/HztBUjjfdDbZYNWKY8=; b=hNA+atFZo9dWRNLyayg0hRIP+BhyO0XN3fzcRQXcasJNpyKjDwWR66AiirZJP9odma QRK0RugdP1EckW5WLbd+UM93LVm0FbVL2H5nNCxDy0Tq7rYdkmjerD4W2aPacZvwqFsE IEW4PNSEHa3WHcdJi2dDLiZO/00Iliss5/PPBqpQR4xW1xZ15OBLn+YxSAbgm+WdJktx Y8R546PVsyTy3P1FHSen9JLtzvwrlOekERzo85BQ7D2dHAy0L0sm+G+txrbjZ8VAjmsy S+TmniTs6nttFvwcvl2FKBQoIxSNfanBm5o/RMFN9multZ9r3UKIwviTmZu+W3rYAFRu Uuwg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1750149006; x=1750753806; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=RmFQZIIJ4r2zWdK/Ht3tViSq/HztBUjjfdDbZYNWKY8=; b=TkhF2BTjBehCbBCnbfuI9Z6xeVQuBP5GiYpqC5mV6fyy0GIflCBCbcRZ49GwbsWgH6 SXjpbitnexfa0qdNp5koVH2+/N10f+Yo6aYt1dVtMIgTpXtnwPaBVT8lPSM2vufNOIFm gP1KHIbJjgjwtbxZbm6H4PGyujsk88Fp9OH/FP/VeU4BWZHEiZiZ1GUpMNQvp2DABqYy X0eRwvYhQIuw9e3Czvejl4W6tYz4vLJ5loNjtGzSszBJ4QaGCuMN6ohIIECNkEJv2uR0 YaH/zJwTRu3nc2d+Vq3H/Z0Hlrn54ONfhnesjImsmPQuGB7v2LavxHwk6DjNz3xlsPtf wSnQ== X-Gm-Message-State: AOJu0YxqCkqy4h0a640GqTjfqdLLHFAjCM+8Ug5bwr091csF4a8BrcRG 2xFFjRTzoDNyr+oBZVDCgCKM2QpB/7lzZdyHt84cOaDKh797dd8JRij4h6t8YIM65wWFRUTSaRn l3Bac X-Gm-Gg: ASbGncvEkclLpRI+tuMPxdG6VSWWVsB0mIhceasoANj3qzXl20enO9qDjp9YTpltu4K mKwQ1SeCeeAEaAqUMWAzyCPkopjWeMUH/GJLC5ypXm1SbuyGsRpcn/0oz7QYohLSo8kW1/YeUGZ lzHwqIVeVNyeryEWzjcEBiZTm4/1htrZ0H1IGTzxkFj/DmOPDr/bYO+cYK+nvEnZvsQJfJFnveK 4F/wNgZEFF54ERmeKgakS++Hs7hmC4c1kJ9ZMSsF+qCZ6B8bt/JbDfBYI7QqKnfHCTTVj26wvRc aMPeNefHlKaM84Dv2gJXut6h/Fav3jHLAvKbYylNupS4k7TyPmauOhATPN84BhMerh7IT+v7Ig4 yGe+OuV/oj0v0I3KJUr9zs4Km1+L6 X-Google-Smtp-Source: AGHT+IFrO5mBRrM1IUOseMLdFE6KIW3wUVI7DRxTXvrlPXfHi+s0M0q75G51enhu2vF5Qr4d/RvPfg== X-Received: by 2002:a05:6000:250c:b0:399:6dd9:9f40 with SMTP id ffacd0b85a97d-3a5723678b8mr11869437f8f.9.1750149006242; Tue, 17 Jun 2025 01:30:06 -0700 (PDT) Received: from raspberrypi.pitowers.org ([2a00:1098:3142:1f:ffc9:aff6:7f7f:893b]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4532e259108sm166062955e9.32.2025.06.17.01.30.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 17 Jun 2025 01:30:05 -0700 (PDT) From: David Plowman To: libcamera-devel@lists.libcamera.org Cc: David Plowman Subject: [PATCH 7/7] ipa: rpi: agc: Remove digital gain from AgcPrepareStatus Date: Tue, 17 Jun 2025 09:29:55 +0100 Message-Id: <20250617082956.5699-8-david.plowman@raspberrypi.com> X-Mailer: git-send-email 2.39.5 In-Reply-To: <20250617082956.5699-1-david.plowman@raspberrypi.com> References: <20250617082956.5699-1-david.plowman@raspberrypi.com> MIME-Version: 1.0 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" All platforms are now using the digital gain from the AgcStatus, so the AgcPrepareStatus and prepare() methods can be tidied up. Signed-off-by: David Plowman --- src/ipa/rpi/controller/agc_status.h | 1 - src/ipa/rpi/controller/rpi/agc_channel.cpp | 18 +----------------- 2 files changed, 1 insertion(+), 18 deletions(-) diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h index 956d6abf..1ae069f2 100644 --- a/src/ipa/rpi/controller/agc_status.h +++ b/src/ipa/rpi/controller/agc_status.h @@ -44,6 +44,5 @@ struct AgcStatus { }; struct AgcPrepareStatus { - double digitalGain; int locked; }; diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp index 3829534c..acc67b71 100644 --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp @@ -480,26 +480,10 @@ void AgcChannel::prepare(Metadata *imageMetadata) AgcPrepareStatus prepareStatus; prepareStatus.locked = false; - prepareStatus.digitalGain = 1.0; - if (!imageMetadata->get("device.status", deviceStatus)) { + if (!imageMetadata->get("device.status", deviceStatus)) prepareStatus.locked = updateLockStatus(deviceStatus); - /* - * For now, the IPA code is still expecting the digital gain to come back in - * the prepare_status. To keep things happy, we'll just fill in the value that - * we calculated previously and put in the AgcStatus (which comes back as the - * "delayed" status). Once the rest of the IPA code is updated, we'll be able - * to remove this, and indeed remove the digitalGain from the AgcPrepareStatus. - */ - AgcStatus delayedStatus; - if (!imageMetadata->get("agc.delayed_status", delayedStatus)) - prepareStatus.digitalGain = delayedStatus.digitalGain; - else - /* After a mode switch, this must be correct until new values come through. */ - prepareStatus.digitalGain = status_.digitalGain; - } - imageMetadata->set("agc.prepare_status", prepareStatus); }