From patchwork Tue Sep 30 15:04:23 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 24534 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 D0ABAC328C for ; Tue, 30 Sep 2025 15:04:40 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 9C8A06B5FF; Tue, 30 Sep 2025 17:04:39 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="FjXSMHRw"; dkim-atps=neutral Received: from tor.source.kernel.org (tor.source.kernel.org [IPv6:2600:3c04:e001:324:0:1991:8:25]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 9496269367 for ; Tue, 30 Sep 2025 17:04:35 +0200 (CEST) Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id CCE0162392; Tue, 30 Sep 2025 15:04:34 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0A002C116B1; Tue, 30 Sep 2025 15:04:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1759244674; bh=ai8KVMNvlLzP8vSPJW7zKzjEWtbw5jwS5ofzUqqDeWs=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=FjXSMHRwIlwo43PbMyvhtdAN3zgfUXSSQYq+i+Tn6Ne29gebaWmIO74NQrRQdRY9O WLYP/z3CBb13Ox3q8kv6jLzkpt2OjH2nfbfesW+DWbdyDHIZYXuR05treV0p5vu3j6 jE+XR7oZNRSYUEK02EDvsGjdV5OpKlKGjkS5UyOBm6TVTCq/jLxkxKo9xldjuWF9sE 5tX8qruI/FBPJeD+F2nbgJjQVLt/a+nBdcTnmfFcREWUZuxwiq3Mqtu9CBn5xnPMvv ONP0EN0+fLetjEfrk5tAwLU/wXhtUfuNmqBWnyACXmtcqlFjSGiqLMRKKWDrY94W11 0mlw66wUcEdkw== From: Hans de Goede To: libcamera-devel@lists.libcamera.org Cc: Hans de Goede , Milan Zamazal , Isaac Scott , Kieran Bingham Subject: [PATCH v4 1/6] ipa: software_isp: Fix context_.configuration.agc.againMin init Date: Tue, 30 Sep 2025 17:04:23 +0200 Message-ID: <20250930150428.11101-2-hansg@kernel.org> X-Mailer: git-send-email 2.51.0 In-Reply-To: <20250930150428.11101-1-hansg@kernel.org> References: <20250930150428.11101-1-hansg@kernel.org> 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" Currently context_.configuration.agc.againMin is not initialized when the control reports a non 0 minimum gain value. So far only the againMin == 0 case was handled and context_.configuration.agc.againMin was left unset otherwise. Reviewed-by: Milan Zamazal Reviewed-by: Isaac Scott Tested-by: Milan Zamazal Tested-by: Kieran Bingham Signed-off-by: Hans de Goede --- src/ipa/simple/soft_simple.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index c94c4cd55..e70439ee5 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -246,7 +246,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) * other) we limit the range of the gain values used. */ context_.configuration.agc.againMax = againMax; - if (!againMin) { + if (againMin) { + context_.configuration.agc.againMin = againMin; + } else { LOG(IPASoft, Warning) << "Minimum gain is zero, that can't be linear"; context_.configuration.agc.againMin = From patchwork Tue Sep 30 15:04:24 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 24535 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 75F68C328C for ; Tue, 30 Sep 2025 15:04:43 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 28C2E6B5F8; Tue, 30 Sep 2025 17:04:41 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="uQ1NRTJB"; dkim-atps=neutral Received: from tor.source.kernel.org (tor.source.kernel.org [IPv6:2600:3c04:e001:324:0:1991:8:25]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 85FA36936E for ; Tue, 30 Sep 2025 17:04:37 +0200 (CEST) Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id B647562392; Tue, 30 Sep 2025 15:04:36 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 13532C4CEF0; Tue, 30 Sep 2025 15:04:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1759244676; bh=byFQsN32b7e8y5Kk37vOrxjURkNYLKfUJWAyd2JThNc=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=uQ1NRTJBe3+vjJm8oix/i8Cmi1dcD1W0bIUpgqdisBnNPp8oqDHDebv/1mSkSKyDq vKhTd+3/LW4Y3NSy6F7hUOpMq3VnBhe0YZkG61McXT029STM6MtH6RYvDZC7dnZb5A nZHetLfn5/JBLIvkHP0cnNzB0qGey5zEYddf+ZDwP8O+fnO/Nx6T0XrmfR4hRSJ1+E u+YRitAVKa5rzZ129IE/pqtkoiEICUo32ocnApO1CRBEa2MPXutVKVR83XN2rpsr6v dNkLXKQ76ohwhD4h9cfYOicPD6W54ORRPjRY8lGmLM5JqDD38i8/P/tgR26HOepJn6 VxhCT9P0dl/tw== From: Hans de Goede To: libcamera-devel@lists.libcamera.org Cc: Hans de Goede , Milan Zamazal , Kieran Bingham , Isaac Scott Subject: [PATCH v4 2/6] ipa: software_isp: AGC: Do not lower gain below 1.0 Date: Tue, 30 Sep 2025 17:04:24 +0200 Message-ID: <20250930150428.11101-3-hansg@kernel.org> X-Mailer: git-send-email 2.51.0 In-Reply-To: <20250930150428.11101-1-hansg@kernel.org> References: <20250930150428.11101-1-hansg@kernel.org> 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" At the moment when the overall image brightness is considered too high the AGC code will lower the gain all the way down to againMin before considering lowering the exposure. What should happen instead is lower the gain no lower than 1.0 and after that lower the exposure instead of lowering the gain. Otherwise there might be a heavily overexposed image (e.g. all white) which then is made less white by a gain < 1.0 which is no good. When there is no sensor-helper, assume the driver reported default-gain value is close to a gain of 1.0 . While at it also remove the weird limitation to only lower the gain when exposure is set to the maximum. As long as the gain is higher than the default gain, the gain should be lowered first. Reviewed-by: Milan Zamazal Tested-by: Milan Zamazal Tested-by: Kieran Bingham Reviewed-by: Kieran Bingham Reviewed-by: Isaac Scott Signed-off-by: Hans de Goede --- Changes in v2: - Use a gain of 1.0 as low-limit instead of always using the default-gain, falling back to the default if there is no sensor-helper for the sensor. --- src/ipa/simple/algorithms/agc.cpp | 3 +-- src/ipa/simple/ipa_context.h | 2 +- src/ipa/simple/soft_simple.cpp | 3 +++ 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp index c46bb0ebe..1fc8d7f42 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou } if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { - if (exposure == context.configuration.agc.exposureMax && - again > context.configuration.agc.againMin) { + if (again > context.configuration.agc.again10) { next = again * kExpNumeratorDown / kExpDenominator; if (again - next < context.configuration.agc.againMinStep) again -= context.configuration.agc.againMinStep; diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h index a471b80ae..468fccabb 100644 --- a/src/ipa/simple/ipa_context.h +++ b/src/ipa/simple/ipa_context.h @@ -28,7 +28,7 @@ struct IPASessionConfiguration { float gamma; struct { int32_t exposureMin, exposureMax; - double againMin, againMax, againMinStep; + double againMin, againMax, again10, againMinStep; utils::Duration lineDuration; } agc; struct { diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index e70439ee5..b147aca2e 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) int32_t againMin = gainInfo.min().get(); int32_t againMax = gainInfo.max().get(); + int32_t againDef = gainInfo.def().get(); if (camHelper_) { context_.configuration.agc.againMin = camHelper_->gain(againMin); context_.configuration.agc.againMax = camHelper_->gain(againMax); + context_.configuration.agc.again10 = camHelper_->gain(1.0); context_.configuration.agc.againMinStep = (context_.configuration.agc.againMax - context_.configuration.agc.againMin) / @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) * other) we limit the range of the gain values used. */ context_.configuration.agc.againMax = againMax; + context_.configuration.agc.again10 = againDef; if (againMin) { context_.configuration.agc.againMin = againMin; } else { From patchwork Tue Sep 30 15:04:25 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 24536 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 5661AC328C for ; Tue, 30 Sep 2025 15:04:45 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 457626B614; Tue, 30 Sep 2025 17:04:44 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="GJgZZivJ"; dkim-atps=neutral Received: from tor.source.kernel.org (tor.source.kernel.org [172.105.4.254]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B8C46B5F9 for ; Tue, 30 Sep 2025 17:04:39 +0200 (CEST) Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id 67C7D62892; Tue, 30 Sep 2025 15:04:38 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id D9F00C4CEF0; Tue, 30 Sep 2025 15:04:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1759244678; bh=Ytqc3j+RlSAjQpfzFAAH1QPOZnlUEEjm1YxgWtn6q3A=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=GJgZZivJi5oP2uJZaO4sw+LwwM+MFmCUHWPfgcbpndLzKlf4puHxaaKkfOnc2vmA7 kVrN+Qb9XaCbOSAlya+4J4eDPMCy9TSB7i7k3UWimVobx5mkfTprW150nleiuH28Ua X1lnpyJy88xrhjs2NlZaOG8bKoBY4MDUxDepntwgsIQ6klN3vuCOf8+2+dozmMVEy2 4NdukgxlN2RNVjCEexGdLzer11987ttXnTn2jVLR+CV4Xxv6DfEOyPqv7ivysgOcbT ZAJl9F4kTnL7TTZy5cbD2WFW7t1SKklQbXAiue20IJDcNTtMQJjn5xjfKNfw17Ewgy YILNQiqLwigKg== From: Hans de Goede To: libcamera-devel@lists.libcamera.org Cc: Hans de Goede , Milan Zamazal , Kieran Bingham Subject: [PATCH v4 3/6] ipa: software_isp: AGC: Raise exposure or gain not both at the same time Date: Tue, 30 Sep 2025 17:04:25 +0200 Message-ID: <20250930150428.11101-4-hansg@kernel.org> X-Mailer: git-send-email 2.51.0 In-Reply-To: <20250930150428.11101-1-hansg@kernel.org> References: <20250930150428.11101-1-hansg@kernel.org> 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" Raise either exposure or gain; not both at the same time. Before this change when the last exposure raise is done, hitting exposure-max, gain would be increased at the same time. Reviewed-by: Milan Zamazal Tested-by: Milan Zamazal Tested-by: Kieran Bingham Signed-off-by: Hans de Goede --- src/ipa/simple/algorithms/agc.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp index 1fc8d7f42..f7f734514 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -56,12 +56,13 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou double &again = frameContext.sensor.gain; if (exposureMSV < kExposureOptimal - kExposureSatisfactory) { - next = exposure * kExpNumeratorUp / kExpDenominator; - if (next - exposure < 1) - exposure += 1; - else - exposure = next; - if (exposure >= context.configuration.agc.exposureMax) { + if (exposure < context.configuration.agc.exposureMax) { + next = exposure * kExpNumeratorUp / kExpDenominator; + if (next - exposure < 1) + exposure += 1; + else + exposure = next; + } else { next = again * kExpNumeratorUp / kExpDenominator; if (next - again < context.configuration.agc.againMinStep) again += context.configuration.agc.againMinStep; From patchwork Tue Sep 30 15:04:26 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 24537 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 8A755C328C for ; Tue, 30 Sep 2025 15:04:47 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 29E206B621; Tue, 30 Sep 2025 17:04:47 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="Uqh+aJVk"; dkim-atps=neutral Received: from sea.source.kernel.org (sea.source.kernel.org [IPv6:2600:3c0a:e001:78e:0:1991:8:25]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id DDAEF6B5FB for ; Tue, 30 Sep 2025 17:04:41 +0200 (CEST) Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sea.source.kernel.org (Postfix) with ESMTP id 1548B43D82; Tue, 30 Sep 2025 15:04:40 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9C6A1C116C6; Tue, 30 Sep 2025 15:04:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1759244679; bh=EJWoEApCSoUNGJ78tbGabNYw6JtT8v2fsDNctWXc/2Y=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Uqh+aJVkjTQT234V5BOXLTYh0texLpotbyjja0rt3pRCeJbcw0vb1ZhbjRJr/Zpm4 8sLADo4f3z4vX5J1ux62hOwBn2kZvA3krGmixGBViep0UmkGwvBMmEOhF41sycO1sM F3uJ+spfKBmfsEL2I1v6KwSTZZ4QBaFmgUtxbn2CMb0egXPfnQzgINhOhjezE5TKBr b3vNxIrrfcBUmmeWMB8NHUdLHUx/YTVBBxW8HaE2glpAAfU0wU72XXS+w23zuTpzyn Rbt+qAD1X8c+W4G7NlUOzW/xVBI1lVZRyQUpMx/f1nYKNW8uw1/i/3SXNlywlfC3DH M7pn3ew7h5Wkg== From: Hans de Goede To: libcamera-devel@lists.libcamera.org Cc: Hans de Goede , Milan Zamazal , Isaac Scott , Kieran Bingham Subject: [PATCH v4 4/6] ipa: software_isp: AGC: Only use integers for exposure calculations Date: Tue, 30 Sep 2025 17:04:26 +0200 Message-ID: <20250930150428.11101-5-hansg@kernel.org> X-Mailer: git-send-email 2.51.0 In-Reply-To: <20250930150428.11101-1-hansg@kernel.org> References: <20250930150428.11101-1-hansg@kernel.org> 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" Exposure is an integer, instead of re-using the "double next" used for again calculations, doing intermediate calculations with double precision, use a local next variable of an integer type. Reviewed-by: Milan Zamazal Reviewed-by: Isaac Scott Tested-by: Milan Zamazal Tested-by: Kieran Bingham Signed-off-by: Hans de Goede --- src/ipa/simple/algorithms/agc.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp index f7f734514..3471cc881 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -51,19 +51,18 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1; static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1; - double next; int32_t &exposure = frameContext.sensor.exposure; double &again = frameContext.sensor.gain; if (exposureMSV < kExposureOptimal - kExposureSatisfactory) { if (exposure < context.configuration.agc.exposureMax) { - next = exposure * kExpNumeratorUp / kExpDenominator; + int32_t next = exposure * kExpNumeratorUp / kExpDenominator; if (next - exposure < 1) exposure += 1; else exposure = next; } else { - next = again * kExpNumeratorUp / kExpDenominator; + double next = again * kExpNumeratorUp / kExpDenominator; if (next - again < context.configuration.agc.againMinStep) again += context.configuration.agc.againMinStep; else @@ -73,13 +72,13 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou if (exposureMSV > kExposureOptimal + kExposureSatisfactory) { if (again > context.configuration.agc.again10) { - next = again * kExpNumeratorDown / kExpDenominator; + double next = again * kExpNumeratorDown / kExpDenominator; if (again - next < context.configuration.agc.againMinStep) again -= context.configuration.agc.againMinStep; else again = next; } else { - next = exposure * kExpNumeratorDown / kExpDenominator; + int32_t next = exposure * kExpNumeratorDown / kExpDenominator; if (exposure - next < 1) exposure -= 1; else From patchwork Tue Sep 30 15:04:27 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 24538 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 F3B35C328C for ; Tue, 30 Sep 2025 15:04:48 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 618176B619; Tue, 30 Sep 2025 17:04:48 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="fKCte6Oc"; dkim-atps=neutral Received: from sea.source.kernel.org (sea.source.kernel.org [IPv6:2600:3c0a:e001:78e:0:1991:8:25]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 320376B601 for ; Tue, 30 Sep 2025 17:04:43 +0200 (CEST) Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sea.source.kernel.org (Postfix) with ESMTP id BE9F243CBF; Tue, 30 Sep 2025 15:04:41 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7AFBDC4CEF0; Tue, 30 Sep 2025 15:04:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1759244681; bh=FeUG4GKlhVLhUviw9aOdli4GM+DFNBO80MuynjHWCZQ=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=fKCte6OcQxYmCLSX8su0BkqRcNdNk1fsXF66EChSN+/A093o439c4/mKdxvWkvp1k LR8A+tmZhSPFeEg8SMv/8Lr3I8B1ASZb8tHWnWzDE6/6EkjTjccENVL3eigg683DZw /2anyFJpc4BsbA6XnFicwJlEVc+Cgt5YP4+PNSRZtf1jtUdS46JVCyEiQppZQ9iJNn 5DIcAkS5Bs7zJ4ox7cZStEJWhAREElNoXweuTaC3xbCmAcWDxZDun/vBeTKyn2zETo V7C0epkLNeSRdSN3ZjUGNn6ZH6LoSO4A7BQKCxCZC+Zk1OCpntLKIKOLpMdPnQpIbw rTAYMQwo5Y/FQ== From: Hans de Goede To: libcamera-devel@lists.libcamera.org Cc: Hans de Goede , Kieran Bingham , Milan Zamazal Subject: [PATCH v4 5/6] libcamera: software_isp: Add valid flag to struct SwIspStats Date: Tue, 30 Sep 2025 17:04:27 +0200 Message-ID: <20250930150428.11101-6-hansg@kernel.org> X-Mailer: git-send-email 2.51.0 In-Reply-To: <20250930150428.11101-1-hansg@kernel.org> References: <20250930150428.11101-1-hansg@kernel.org> 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" Generating statistics for every single frame is not really necessary. However a roundtrip through ipa_->processStats() still need to be done every frame, even if there are no stats to make the IPA generate metadata for every frame. Add a valid flag to the statistics struct to let the IPA know when there are no statistics for the frame being processed and modify the IPA to only generate metadata for frames without valid statistics. This is a preparation patch for skipping statistics generation for some frames. Reviewed-by: Kieran Bingham Reviewed-by: Milan Zamazal Tested-by: Milan Zamazal Tested-by: Kieran Bingham Signed-off-by: Hans de Goede --- Changes in v4: - Add an empty line after the agc substruct declaration - Init active-state agc gain and exposue from sensor values in case updateExposure() does not run for the first frame Changes in v3: - Drop if (!stats_->valid) early exit from IPASoftSimple::processStats() - Save last AGC calculated new gain and exposure values and re-use these for frames without statistics, this avoids delayedCtrl history underruns - Improve SwIspStats.valid documentation Changes in v2: - This is a new patch in v2 of this series --- .../internal/software_isp/swisp_stats.h | 5 ++++ src/ipa/simple/algorithms/agc.cpp | 24 ++++++++++++++++++- src/ipa/simple/algorithms/awb.cpp | 3 +++ src/ipa/simple/algorithms/blc.cpp | 3 +++ src/ipa/simple/ipa_context.h | 5 ++++ src/libcamera/software_isp/swstats_cpu.cpp | 1 + 6 files changed, 40 insertions(+), 1 deletion(-) diff --git a/include/libcamera/internal/software_isp/swisp_stats.h b/include/libcamera/internal/software_isp/swisp_stats.h index ae11f112e..3c9860185 100644 --- a/include/libcamera/internal/software_isp/swisp_stats.h +++ b/include/libcamera/internal/software_isp/swisp_stats.h @@ -20,6 +20,11 @@ namespace libcamera { * wrap around. */ struct SwIspStats { + /** + * \brief True if the statistics buffer contains valid data, false if + * no statistics were generated for this frame + */ + bool valid; /** * \brief Holds the sum of all sampled red pixels */ diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp index 3471cc881..e0447cbd6 100644 --- a/src/ipa/simple/algorithms/agc.cpp +++ b/src/ipa/simple/algorithms/agc.cpp @@ -91,13 +91,16 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou again = std::clamp(again, context.configuration.agc.againMin, context.configuration.agc.againMax); + context.activeState.agc.exposure = exposure; + context.activeState.agc.again = again; + LOG(IPASoftExposure, Debug) << "exposureMSV " << exposureMSV << " exp " << exposure << " again " << again; } void Agc::process(IPAContext &context, - [[maybe_unused]] const uint32_t frame, + const uint32_t frame, IPAFrameContext &frameContext, const SwIspStats *stats, ControlList &metadata) @@ -107,6 +110,25 @@ void Agc::process(IPAContext &context, metadata.set(controls::ExposureTime, exposureTime.get()); metadata.set(controls::AnalogueGain, frameContext.sensor.gain); + if (frame == 0) { + /* + * Init active-state from sensor values in case updateExposure() + * does not run for the first frame. + */ + context.activeState.agc.exposure = frameContext.sensor.exposure; + context.activeState.agc.again = frameContext.sensor.gain; + } + + if (!stats->valid) { + /* + * Use the new exposure and gain values calculated the last time + * there were valid stats. + */ + frameContext.sensor.exposure = context.activeState.agc.exposure; + frameContext.sensor.gain = context.activeState.agc.again; + return; + } + /* * Calculate Mean Sample Value (MSV) according to formula from: * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp index cf567e894..ddd0b7914 100644 --- a/src/ipa/simple/algorithms/awb.cpp +++ b/src/ipa/simple/algorithms/awb.cpp @@ -61,6 +61,9 @@ void Awb::process(IPAContext &context, }; metadata.set(controls::ColourGains, mdGains); + if (!stats->valid) + return; + /* * Black level must be subtracted to get the correct AWB ratios, they * would be off if they were computed from the whole brightness range diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp index 8c1e9ed08..616da0ee7 100644 --- a/src/ipa/simple/algorithms/blc.cpp +++ b/src/ipa/simple/algorithms/blc.cpp @@ -60,6 +60,9 @@ void BlackLevel::process(IPAContext &context, }; metadata.set(controls::SensorBlackLevels, blackLevels); + if (!stats->valid) + return; + if (context.configuration.black.level.has_value()) return; diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h index 468fccabb..c3081e306 100644 --- a/src/ipa/simple/ipa_context.h +++ b/src/ipa/simple/ipa_context.h @@ -37,6 +37,11 @@ struct IPASessionConfiguration { }; struct IPAActiveState { + struct { + int32_t exposure; + double again; + } agc; + struct { uint8_t level; int32_t lastExposure; diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp index 4b77b3600..eb416dfdc 100644 --- a/src/libcamera/software_isp/swstats_cpu.cpp +++ b/src/libcamera/software_isp/swstats_cpu.cpp @@ -318,6 +318,7 @@ void SwStatsCpu::startFrame(void) */ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) { + stats_.valid = true; *sharedStats_ = stats_; statsReady.emit(frame, bufferId); } From patchwork Tue Sep 30 15:04:28 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hans de Goede X-Patchwork-Id: 24539 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 9B2A5C328C for ; Tue, 30 Sep 2025 15:04:50 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id C00166B608; Tue, 30 Sep 2025 17:04:49 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="f2/2ryl2"; dkim-atps=neutral Received: from tor.source.kernel.org (tor.source.kernel.org [172.105.4.254]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id 6AAEC6B615 for ; Tue, 30 Sep 2025 17:04:44 +0200 (CEST) Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id 97B2562890; Tue, 30 Sep 2025 15:04:43 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 23544C4CEF0; Tue, 30 Sep 2025 15:04:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1759244683; bh=RnbEW+JJJj2WQrZC2bqN7fhdjO5jAqhOiCDeEX+v1u4=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=f2/2ryl2CgEXHlXHkdnAqdS3R3eA4aT09Wzyodb8A7qSFrMJDWLP4LXLpJjn/F4Y6 xKWZLt9Ef6PlufwpZm1ai0fwPP8MuSGrDg4uFu6C5pROXhY3ZX0eCsySBsgyCi0gqm IlDOy07gKqpjNtXZFLA5WhzB2kQZBn+TYN007DJcrDZYu7LccKDWrI0btvkLbYC3l4 TxwvxsjV9eV1SKm8WMD1gRgYEe2uf2h4IE29iBK/E1+hdO0nNMUcuMrAIhKg+wPBC3 ZerL2nN4Vf5wuLKpszdYkEHzecjtTBMlSFwrceT7OOV8c1bDUYft9SW0lYM4b5XgYh S8k9TW69btqtw== From: Hans de Goede To: libcamera-devel@lists.libcamera.org Cc: Hans de Goede , Kieran Bingham , Milan Zamazal Subject: [PATCH v4 6/6] libcamera: software_isp: Run sw-statistics once every 4th frame Date: Tue, 30 Sep 2025 17:04:28 +0200 Message-ID: <20250930150428.11101-7-hansg@kernel.org> X-Mailer: git-send-email 2.51.0 In-Reply-To: <20250930150428.11101-1-hansg@kernel.org> References: <20250930150428.11101-1-hansg@kernel.org> 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" Run sw-statistics once every 4th frame, instead of every frame. There are 2 reasons for this: 1. There really is no need to have statistics for every frame and only doing this every 4th frame helps save some CPU time. 2. The generic nature of the simple pipeline-handler, so no information about possible CSI receiver frame-delays. In combination with the software ISP often being used with sensors without sensor info in the sensor-helper code, so no reliable control-delay information means that the software ISP is prone to AGC oscillation. Skipping statistics gathering also means skipping running the AGC algorithm slowing it down, avoiding this oscillation. Note ideally the AGC oscillation problem would be fixed by adding sensor metadata support all through the stack so that the exact gain and exposure used for a specific frame are reliably provided by the sensor metadata. Reviewed-by: Kieran Bingham Tested-by: Milan Zamazal Tested-by: Kieran Bingham Signed-off-by: Hans de Goede Reviewed-by: Milan Zamazal --- Changes in v4: - Document why to skip 3 frames / why once every 4 frames - Pass frame number to SwStatsCpu::startFrame() SwStatsCpu::processLine?() and move all skipping handling to inside the SwStatsCpu class --- src/libcamera/software_isp/debayer_cpu.cpp | 18 +++++++++--------- src/libcamera/software_isp/debayer_cpu.h | 4 ++-- src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++---- src/libcamera/software_isp/swstats_cpu.h | 20 +++++++++++++++++--- 4 files changed, 43 insertions(+), 18 deletions(-) diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index 2dc85e5e0..d5fd0ae73 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -655,7 +655,7 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[]) lineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1); } -void DebayerCpu::process2(const uint8_t *src, uint8_t *dst) +void DebayerCpu::process2(uint32_t frame, const uint8_t *src, uint8_t *dst) { unsigned int yEnd = window_.y + window_.height; /* Holds [0] previous- [1] current- [2] next-line */ @@ -681,7 +681,7 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst) for (unsigned int y = window_.y; y < yEnd; y += 2) { shiftLinePointers(linePointers, src); memcpyNextLine(linePointers); - stats_->processLine0(y, linePointers); + stats_->processLine0(frame, y, linePointers); (this->*debayer0_)(dst, linePointers); src += inputConfig_.stride; dst += outputConfig_.stride; @@ -696,7 +696,7 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst) if (window_.y == 0) { shiftLinePointers(linePointers, src); memcpyNextLine(linePointers); - stats_->processLine0(yEnd, linePointers); + stats_->processLine0(frame, yEnd, linePointers); (this->*debayer0_)(dst, linePointers); src += inputConfig_.stride; dst += outputConfig_.stride; @@ -710,7 +710,7 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst) } } -void DebayerCpu::process4(const uint8_t *src, uint8_t *dst) +void DebayerCpu::process4(uint32_t frame, const uint8_t *src, uint8_t *dst) { const unsigned int yEnd = window_.y + window_.height; /* @@ -733,7 +733,7 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst) for (unsigned int y = window_.y; y < yEnd; y += 4) { shiftLinePointers(linePointers, src); memcpyNextLine(linePointers); - stats_->processLine0(y, linePointers); + stats_->processLine0(frame, y, linePointers); (this->*debayer0_)(dst, linePointers); src += inputConfig_.stride; dst += outputConfig_.stride; @@ -746,7 +746,7 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst) shiftLinePointers(linePointers, src); memcpyNextLine(linePointers); - stats_->processLine2(y, linePointers); + stats_->processLine2(frame, y, linePointers); (this->*debayer2_)(dst, linePointers); src += inputConfig_.stride; dst += outputConfig_.stride; @@ -821,12 +821,12 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output return; } - stats_->startFrame(); + stats_->startFrame(frame); if (inputConfig_.patternSize.height == 2) - process2(in.planes()[0].data(), out.planes()[0].data()); + process2(frame, in.planes()[0].data(), out.planes()[0].data()); else - process4(in.planes()[0].data(), out.planes()[0].data()); + process4(frame, in.planes()[0].data(), out.planes()[0].data()); metadata.planes()[0].bytesused = out.planes()[0].size(); diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h index 9d343e464..03e0d7843 100644 --- a/src/libcamera/software_isp/debayer_cpu.h +++ b/src/libcamera/software_isp/debayer_cpu.h @@ -133,8 +133,8 @@ private: void setupInputMemcpy(const uint8_t *linePointers[]); void shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src); void memcpyNextLine(const uint8_t *linePointers[]); - void process2(const uint8_t *src, uint8_t *dst); - void process4(const uint8_t *src, uint8_t *dst); + void process2(uint32_t frame, const uint8_t *src, uint8_t *dst); + void process4(uint32_t frame, const uint8_t *src, uint8_t *dst); /* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */ static constexpr unsigned int kMaxLineBuffers = 5; diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp index eb416dfdc..634ebfc3c 100644 --- a/src/libcamera/software_isp/swstats_cpu.cpp +++ b/src/libcamera/software_isp/swstats_cpu.cpp @@ -62,8 +62,9 @@ namespace libcamera { */ /** - * \fn void SwStatsCpu::processLine0(unsigned int y, const uint8_t *src[]) + * \fn void SwStatsCpu::processLine0(uint32_t frame, unsigned int y, const uint8_t *src[]) * \brief Process line 0 + * \param[in] frame The frame number * \param[in] y The y coordinate. * \param[in] src The input data. * @@ -74,8 +75,9 @@ namespace libcamera { */ /** - * \fn void SwStatsCpu::processLine2(unsigned int y, const uint8_t *src[]) + * \fn void SwStatsCpu::processLine2(uint32_t frame, unsigned int y, const uint8_t *src[]) * \brief Process line 2 and 3 + * \param[in] frame The frame number * \param[in] y The y coordinate. * \param[in] src The input data. * @@ -89,6 +91,11 @@ namespace libcamera { * \brief Signals that the statistics are ready */ +/** + * \var SwStatsCpu::kStatPerNumFrames + * \brief Run stats once every kStatPerNumFrames frames + */ + /** * \typedef SwStatsCpu::statsProcessFn * \brief Called when there is data to get statistics from @@ -295,11 +302,15 @@ void SwStatsCpu::statsGBRG10PLine0(const uint8_t *src[]) /** * \brief Reset state to start statistics gathering for a new frame + * \param[in] frame The frame number * * This may only be called after a successful setWindow() call. */ -void SwStatsCpu::startFrame(void) +void SwStatsCpu::startFrame(uint32_t frame) { + if (frame % kStatPerNumFrames) + return; + if (window_.width == 0) LOG(SwStatsCpu, Error) << "Calling startFrame() without setWindow()"; @@ -318,7 +329,7 @@ void SwStatsCpu::startFrame(void) */ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId) { - stats_.valid = true; + stats_.valid = frame % kStatPerNumFrames == 0; *sharedStats_ = stats_; statsReady.emit(frame, bufferId); } diff --git a/src/libcamera/software_isp/swstats_cpu.h b/src/libcamera/software_isp/swstats_cpu.h index 26a2f462e..fae575f85 100644 --- a/src/libcamera/software_isp/swstats_cpu.h +++ b/src/libcamera/software_isp/swstats_cpu.h @@ -32,6 +32,14 @@ public: SwStatsCpu(); ~SwStatsCpu() = default; + /* + * The combination of pipeline + sensor delays means that + * exposure changes can take up to 3 frames to get applied, + * Run stats once every 4 frames to ensure any previous + * exposure changes have been applied. + */ + static constexpr uint32_t kStatPerNumFrames = 4; + bool isValid() const { return sharedStats_.fd().isValid(); } const SharedFD &getStatsFD() { return sharedStats_.fd(); } @@ -40,11 +48,14 @@ public: int configure(const StreamConfiguration &inputCfg); void setWindow(const Rectangle &window); - void startFrame(); + void startFrame(uint32_t frame); void finishFrame(uint32_t frame, uint32_t bufferId); - void processLine0(unsigned int y, const uint8_t *src[]) + void processLine0(uint32_t frame, unsigned int y, const uint8_t *src[]) { + if (frame % kStatPerNumFrames) + return; + if ((y & ySkipMask_) || y < static_cast(window_.y) || y >= (window_.y + window_.height)) return; @@ -52,8 +63,11 @@ public: (this->*stats0_)(src); } - void processLine2(unsigned int y, const uint8_t *src[]) + void processLine2(uint32_t frame, unsigned int y, const uint8_t *src[]) { + if (frame % kStatPerNumFrames) + return; + if ((y & ySkipMask_) || y < static_cast(window_.y) || y >= (window_.y + window_.height)) return;