[{"id":36009,"web_url":"https://patchwork.libcamera.org/comment/36009/","msgid":"<175899987586.416692.6848288558061127577@ping.linuxembedded.co.uk>","date":"2025-09-27T19:04:35","subject":"Re: [PATCH v3 2/6] ipa: software_isp: AGC: Do not lower gain below\n\t1.0","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2025-09-27 19:00:00)\n> At the moment when the overall image brightness is considered too high\n> the AGC code will lower the gain all the way down to againMin before\n> considering lowering the exposure.\n> \n> What should happen instead is lower the gain no lower than 1.0 and after\n> that lower the exposure instead of lowering the gain.\n> \n> Otherwise there might be a heavily overexposed image (e.g. all white)\n> which then is made less white by a gain < 1.0 which is no good.\n> \n> When there is no sensor-helper, assume the driver reported default-gain\n> value is close to a gain of 1.0 .\n> \n> While at it also remove the weird limitation to only lower the gain\n> when exposure is set to the maximum. As long as the gain is higher\n> than the default gain, the gain should be lowered first.\n> \n> Reviewed-by: Milan Zamazal \n> Tested-by: Milan Zamazal \n> Tested-by: Kieran Bingham \n\nReviewed-by: Kieran Bingham \n\n> Signed-off-by: Hans de Goede \n> ---\n> Changes in v2:\n> - Use a gain of 1.0 as low-limit instead of always using the default-gain,\n> falling back to the default if there is no sensor-helper for the sensor.\n> ---\n> src/ipa/simple/algorithms/agc.cpp | 3 +--\n> src/ipa/simple/ipa_context.h | 2 +-\n> src/ipa/simple/soft_simple.cpp | 3 +++\n> 3 files changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp\n> index c46bb0eb..1fc8d7f4 100644\n> --- a/src/ipa/simple/algorithms/agc.cpp\n> +++ b/src/ipa/simple/algorithms/agc.cpp\n> @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou\n> }\n> \n> if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {\n> - if (exposure == context.configuration.agc.exposureMax &&\n> - again > context.configuration.agc.againMin) {\n> + if (again > context.configuration.agc.again10) {\n> next = again * kExpNumeratorDown / kExpDenominator;\n> if (again - next < context.configuration.agc.againMinStep)\n> again -= context.configuration.agc.againMinStep;\n> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index a471b80a..468fccab 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -28,7 +28,7 @@ struct IPASessionConfiguration {\n> float gamma;\n> struct {\n> int32_t exposureMin, exposureMax;\n> - double againMin, againMax, againMinStep;\n> + double againMin, againMax, again10, againMinStep;\n> utils::Duration lineDuration;\n> } agc;\n> struct {\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index e70439ee..b147aca2 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n> \n> int32_t againMin = gainInfo.min().get();\n> int32_t againMax = gainInfo.max().get();\n> + int32_t againDef = gainInfo.def().get();\n> \n> if (camHelper_) {\n> context_.configuration.agc.againMin = camHelper_->gain(againMin);\n> context_.configuration.agc.againMax = camHelper_->gain(againMax);\n> + context_.configuration.agc.again10 = camHelper_->gain(1.0);\n> context_.configuration.agc.againMinStep =\n> (context_.configuration.agc.againMax -\n> context_.configuration.agc.againMin) /\n> @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n> * other) we limit the range of the gain values used.\n> */\n> context_.configuration.agc.againMax = againMax;\n> + context_.configuration.agc.again10 = againDef;\n> if (againMin) {\n> context_.configuration.agc.againMin = againMin;\n> } else {\n> -- \n> 2.51.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 54AEABDB1C\n\tfor ;\n\tSat, 27 Sep 2025 19:04:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 091736B5F3;\n\tSat, 27 Sep 2025 21:04:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E11236B5A2\n\tfor ;\n\tSat, 27 Sep 2025 21:04:38 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A189183D;\n\tSat, 27 Sep 2025 21:03:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gPD0+rQO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758999792;\n\tbh=MH1SDxQsR8V2m4Na49Dc3j7MNoqBdEBGbs3W/+GnUbY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=gPD0+rQOwDR6pgWZk5jhBAa3ed5t8q/uqtLVUtpGKl1WJAP1+eW4i8LwhVVIh3NN2\n\tVK4pb3p2QeeLQ+Y7ia5u2Dh+mjg3J9xMYEt3M4O97bzh7snGpcLBpqymJzbEMwgJxb\n\twFIstriiMoJ4A0p+lkAbnMX2bf7Pb6sBfVGLVzO8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250927180004.84620-3-hansg@kernel.org>","References":"<20250927180004.84620-1-hansg@kernel.org>\n\t<20250927180004.84620-3-hansg@kernel.org>","Subject":"Re: [PATCH v3 2/6] ipa: software_isp: AGC: Do not lower gain below\n\t1.0","From":"Kieran Bingham ","Cc":"Hans de Goede , Milan Zamazal ","To":"Hans de Goede , libcamera-devel@lists.libcamera.org","Date":"Sat, 27 Sep 2025 20:04:35 +0100","Message-ID":"<175899987586.416692.6848288558061127577@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36012,"web_url":"https://patchwork.libcamera.org/comment/36012/","msgid":"","date":"2025-09-28T09:27:52","subject":"Re: [PATCH v3 2/6] ipa: software_isp: AGC: Do not lower gain below\n\t1.0","submitter":{"id":239,"url":"https://patchwork.libcamera.org/api/people/239/","name":"Hans de Goede","email":"hansg@kernel.org"},"content":"Hi,\n\nOn 27-Sep-25 8:00 PM, Hans de Goede wrote:\n> At the moment when the overall image brightness is considered too high\n> the AGC code will lower the gain all the way down to againMin before\n> considering lowering the exposure.\n> \n> What should happen instead is lower the gain no lower than 1.0 and after\n> that lower the exposure instead of lowering the gain.\n> \n> Otherwise there might be a heavily overexposed image (e.g. all white)\n> which then is made less white by a gain < 1.0 which is no good.\n> \n> When there is no sensor-helper, assume the driver reported default-gain\n> value is close to a gain of 1.0 .\n> \n> While at it also remove the weird limitation to only lower the gain\n> when exposure is set to the maximum. As long as the gain is higher\n> than the default gain, the gain should be lowered first.\n> \n> Reviewed-by: Milan Zamazal \n> Tested-by: Milan Zamazal \n> Tested-by: Kieran Bingham \n> Signed-off-by: Hans de Goede \n\nThis patch is unchanged from v2 and I forgot to add Isaac's\nreviewed-by from v2:\n\nReviewed-by: Isaac Scott \n\nRegards,\n\nHans\n\n\n\n> ---\n> Changes in v2:\n> - Use a gain of 1.0 as low-limit instead of always using the default-gain,\n> falling back to the default if there is no sensor-helper for the sensor.\n> ---\n> src/ipa/simple/algorithms/agc.cpp | 3 +--\n> src/ipa/simple/ipa_context.h | 2 +-\n> src/ipa/simple/soft_simple.cpp | 3 +++\n> 3 files changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp\n> index c46bb0eb..1fc8d7f4 100644\n> --- a/src/ipa/simple/algorithms/agc.cpp\n> +++ b/src/ipa/simple/algorithms/agc.cpp\n> @@ -71,8 +71,7 @@ void Agc::updateExposure(IPAContext &context, IPAFrameContext &frameContext, dou\n> \t}\n> \n> \tif (exposureMSV > kExposureOptimal + kExposureSatisfactory) {\n> -\t\tif (exposure == context.configuration.agc.exposureMax &&\n> -\t\t again > context.configuration.agc.againMin) {\n> +\t\tif (again > context.configuration.agc.again10) {\n> \t\t\tnext = again * kExpNumeratorDown / kExpDenominator;\n> \t\t\tif (again - next < context.configuration.agc.againMinStep)\n> \t\t\t\tagain -= context.configuration.agc.againMinStep;\n> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index a471b80a..468fccab 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -28,7 +28,7 @@ struct IPASessionConfiguration {\n> \tfloat gamma;\n> \tstruct {\n> \t\tint32_t exposureMin, exposureMax;\n> -\t\tdouble againMin, againMax, againMinStep;\n> +\t\tdouble againMin, againMax, again10, againMinStep;\n> \t\tutils::Duration lineDuration;\n> \t} agc;\n> \tstruct {\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index e70439ee..b147aca2 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -216,10 +216,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n> \n> \tint32_t againMin = gainInfo.min().get();\n> \tint32_t againMax = gainInfo.max().get();\n> +\tint32_t againDef = gainInfo.def().get();\n> \n> \tif (camHelper_) {\n> \t\tcontext_.configuration.agc.againMin = camHelper_->gain(againMin);\n> \t\tcontext_.configuration.agc.againMax = camHelper_->gain(againMax);\n> +\t\tcontext_.configuration.agc.again10 = camHelper_->gain(1.0);\n> \t\tcontext_.configuration.agc.againMinStep =\n> \t\t\t(context_.configuration.agc.againMax -\n> \t\t\t context_.configuration.agc.againMin) /\n> @@ -246,6 +248,7 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)\n> \t\t * other) we limit the range of the gain values used.\n> \t\t */\n> \t\tcontext_.configuration.agc.againMax = againMax;\n> +\t\tcontext_.configuration.agc.again10 = againDef;\n> \t\tif (againMin) {\n> \t\t\tcontext_.configuration.agc.againMin = againMin;\n> \t\t} else {","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5B467C324C\n\tfor ;\n\tSun, 28 Sep 2025 09:28:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 00F9D6B5F3;\n\tSun, 28 Sep 2025 11:27:58 +0200 (CEST)","from sea.source.kernel.org (sea.source.kernel.org\n\t[IPv6:2600:3c0a:e001:78e:0:1991:8:25])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 963C662C35\n\tfor ;\n\tSun, 28 Sep 2025 11:27:56 +0200 (CEST)","from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58])\n\tby sea.source.kernel.org (Postfix) with ESMTP id 00E5F40275;\n\tSun, 28 Sep 2025 09:27:55 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 27CABC4CEF0;\n\tSun, 28 Sep 2025 09:27:53 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=kernel.org header.i=@kernel.org\n\theader.b=\"Pd2qmCk+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1759051674;\n\tbh=GJK16h6bki+xZmUAnBhCxRbmhG4wbpKkHRJtDUBBAaQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Pd2qmCk+7nCzHQslamOv5Q7076YK4DUHNWaxOXahYsuVkSjVWK3S0jI6nGdg3yozu\n\thSj2ZbuVB+wSpk5gKPBumf3MCYcHCzX0aQOT/LGdSxNlbN0DdfrNSMr07PO6PJIjtn\n\t4/mEejPg4vRLAl6LnqETk956OZnov9+UKKR33be0476P+syGDywh02Kbd9owGcmVOM\n\tgtucV1hzFdTzbmdCzF6u5e01WEW78v1hEPYIc3LYQXqP9jU+uxgJf8KQvNlvjKIh2e\n\t562ZEQ5kpGIZCwe9lt4LVLWUh0rJ0xeKGWBF1+eEKanco7vELWhQuIQMkXpU4+qo5Y\n\tdETYLo2jj2nsQ==","Message-ID":"","Date":"Sun, 28 Sep 2025 11:27:52 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 2/6] ipa: software_isp: AGC: Do not lower gain below\n\t1.0","To":"libcamera-devel@lists.libcamera.org","Cc":"Milan Zamazal ,\n\tKieran Bingham ","References":"<20250927180004.84620-1-hansg@kernel.org>\n\t<20250927180004.84620-3-hansg@kernel.org>","From":"Hans de Goede ","Content-Language":"en-US, nl","In-Reply-To":"<20250927180004.84620-3-hansg@kernel.org>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]