[{"id":35956,"web_url":"https://patchwork.libcamera.org/comment/35956/","msgid":"<85zfakmgkc.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-09-24T07:54:11","subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Hans,\n\nthank you for the patch.\n\nHans de Goede writes:\n\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 then its default\n\ns/then/than/\n\n> ctrl value and after that lower the exposure instead of lowering\n> 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 very low gain which is no good.\n\nDo I understand it correctly that the gain should never be set below the\ndefault value and bright images can then be fully handled by reducing exposure?\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> then the default gain, the gain should be lowered first.\n\ns/then/than/\n\n> Signed-off-by: Hans de Goede \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 c46bb0ebe..94961f9fe 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.againDef) {\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 a471b80ae..ba525a881 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, againDef, 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 e70439ee5..f0764ef46 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.againDef = camHelper_->gain(againDef);\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.againDef = 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 A46C9BDB1C\n\tfor ;\n\tWed, 24 Sep 2025 07:54:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 09DCE6B5F9;\n\tWed, 24 Sep 2025 09:54:29 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 169216B5C2\n\tfor ;\n\tWed, 24 Sep 2025 09:54:17 +0200 (CEST)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-258-VYqCThbgOs-jdm1KEoI0hg-1; Wed, 24 Sep 2025 03:54:15 -0400","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-3ecdf7b5c46so2738628f8f.2\n\tfor ;\n\tWed, 24 Sep 2025 00:54:14 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb ([85.93.96.130])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-46e2a998321sm19712985e9.1.2025.09.24.00.54.12\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 24 Sep 2025 00:54:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"ikJRXw5V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1758700456;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=4ZJBWIOa1ubBm2vverMoK41kWR3rt5EJ16HllapohCw=;\n\tb=ikJRXw5V5s2FpNPwVM1TyXev9CR4koq6HZS6qy+NgkhEcay0YxIjFONPiykkRbZawoWa6t\n\t6OFRgc4oGH/icyrbAcML7DllqodJfIdCXdK8OtUm5JH+GbiNI5VbNzFKIBQ4NYWlMP5/1r\n\tAKfAvbP5G8RQSG4OYgFiZGOSuft0OXw=","X-MC-Unique":"VYqCThbgOs-jdm1KEoI0hg-1","X-Mimecast-MFC-AGG-ID":"VYqCThbgOs-jdm1KEoI0hg_1758700454","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1758700453; x=1759305253;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=4ZJBWIOa1ubBm2vverMoK41kWR3rt5EJ16HllapohCw=;\n\tb=qPhV7F7NPkXCIz+2SPBCZjpKpY96ci097wLED7rMXnbw3b9AwvP7xQ7dvlKz0NcieM\n\tceOUutWovHjOj8gBKTtravHvxC0xsXs26yIwFsPpD6T1VLHQLFoXJaCjGyTzrXl+G9CH\n\tWCuUJ1nQlWexHGjaOChlFtHWGt8FvqpCrKgLho4i7tPvamM11/oWGcrsxeDa2Qtv5Rjw\n\tcJT552uJgFiSRKy4ZvcN1MhVq3MzgYVs3P77Wwn0MDQPYKncqHPD5KXPSDVXftzTXwOs\n\te6mPXkeOVh7IW/cZF2pNdbpD/FicL3/WQVN/oY7tQPXOVxQDKzkg1Gv/Tvhuv6uxoJFQ\n\tAxdQ==","X-Gm-Message-State":"AOJu0YxYSdhUCYSPCIqVILzsltoQxzsVHguQ0TsetcCr2TxNTgmffGik\n\tGIpALnMypDotGc3vZkMD7BcJhZO46mYjkEOIAdXnSHgDfRP9aPTvLUl6NP8f8PTSek5KCWw0ir7\n\tVMsi3/L+PAg51qzjfahpyVLmyMhA7jCqvYNlpxhVCF0ZffaZqOhnHQMb5Vc64MIjMdtodBJ6Ajo\n\thx06BuSdagZtOCEUGwBz1NTXoGkxviTGrs8VvtnPgpe8lRjkzZdCamWirnC3w=","X-Gm-Gg":"ASbGncvX+MxkjgFWxG0YR1kVUwr26eeET0ly3hhe70slapcqf70pCgLYQczN4Ld981C\n\t6vC4oTO2xbHn+P5qhgxImsRrzjCW4jXoNqhKiX5BMwfZrhM2FpGGvuLIdHjWPCqYD02j9UA66I7\n\t2ann1b1iN8HYLge/Xm3hEdhhJOvfnojBmmUguwMB1KgO7RBZVQEdxKxYThWosHuQ5p6ULJqU8UR\n\thArTBhM6Jx8iF8W6bOEp/UD6pffskVMXY+N+8zUfw6YnQbNlgsBfrA6k9e0gR5cm2tPDw8REgaF\n\tcWw5pMbLk7AV2Aya1gI+qUR6AsPcJl1tE4Ib+tj+t+auicfUwAmcveCDVBGvWSQ3jA==","X-Received":["by 2002:a05:6000:26c5:b0:3fe:d6df:c679 with SMTP id\n\tffacd0b85a97d-405cc70f8d4mr4211031f8f.55.1758700453387; \n\tWed, 24 Sep 2025 00:54:13 -0700 (PDT)","by 2002:a05:6000:26c5:b0:3fe:d6df:c679 with SMTP id\n\tffacd0b85a97d-405cc70f8d4mr4211010f8f.55.1758700452885; \n\tWed, 24 Sep 2025 00:54:12 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHx3kfp72hmL1VaoVHtuumfL8h6UieSUhnQ5W4Ap3UmTzawLvhMhtiTHU672nwkhJAwx2JnRw==","From":"Milan Zamazal ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","In-Reply-To":"<20250923190657.21453-3-hansg@kernel.org> (Hans de Goede's\n\tmessage of \"Tue, 23 Sep 2025 21:06:54 +0200\")","References":"<20250923190657.21453-1-hansg@kernel.org>\n\t<20250923190657.21453-3-hansg@kernel.org>","Date":"Wed, 24 Sep 2025 09:54:11 +0200","Message-ID":"<85zfakmgkc.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"6Y9R5bZeRjd1JgD1heaiesSTBGlWkVtzivqnpzGiOmA_1758700454","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":35967,"web_url":"https://patchwork.libcamera.org/comment/35967/","msgid":"<175879462815.88644.10985798763928439670@isaac-ThinkPad-T16-Gen-2>","date":"2025-09-25T10:03:48","subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","submitter":{"id":215,"url":"https://patchwork.libcamera.org/api/people/215/","name":"Isaac Scott","email":"isaac.scott@ideasonboard.com"},"content":"Hi Hans,\n\nThank you for the patch!\n\nQuoting Hans de Goede (2025-09-23 20:06:54)\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 then its default\n> ctrl value and after that lower the exposure instead of lowering\n> the gain.\n\nI may be wrong, but what if the default gain is something like 2.0,\nwouldn't that mean the light levels received at maximum exposure would\nbe multiplied by 2?\n\nWhile I agree that using the absolute minimum is probably a bad idea (as\neven if it goes < 1, we're still artificially modifying the data), I\nthink we maybe should be able to reduce the analogue gain to 1.0 at a\nminimum before touching exposure, to ensure we're making the most of the\nexposure we have.\n\nBest wishes,\nIsaac\n\n> \n> Otherwise there might be a heavily overexposed image (e.g. all white)\n> which then is made less white by a very low gain which is no good.\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> then the default gain, the gain should be lowered first.\n> \n> Signed-off-by: Hans de Goede \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 c46bb0ebe..94961f9fe 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.againDef) {\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 a471b80ae..ba525a881 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, againDef, 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 e70439ee5..f0764ef46 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.againDef = camHelper_->gain(againDef);\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.againDef = 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 E4DA6BDB1C\n\tfor ;\n\tThu, 25 Sep 2025 10:03:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B264B6B5F3;\n\tThu, 25 Sep 2025 12:03:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B1DF969318\n\tfor ;\n\tThu, 25 Sep 2025 12:03:51 +0200 (CEST)","from thinkpad.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 EF300E1F;\n\tThu, 25 Sep 2025 12:02:26 +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=\"P1dpTLER\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758794547;\n\tbh=C/FfD/OwdlETBbmVL9sYmYbVUgmInjhuJgfZsogAHDk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=P1dpTLERU5/4L4kWE0RZezfi367GLmrNPj3+ZRIa5BGWNaDmhyrdL/FwXNn2oyRWk\n\t97DqUI1Xs24x9KrnC8Ksg6jQb+oqsv1W3S1wZjGOndeexruTzRyekVtbI2EM3+sXXs\n\tRN3PPFUtbCxj5wvwsy/aCA8hC3cW6YSXzwv4gQug=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250923190657.21453-3-hansg@kernel.org>","References":"<20250923190657.21453-1-hansg@kernel.org>\n\t<20250923190657.21453-3-hansg@kernel.org>","Subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","From":"Isaac Scott ","Cc":"Hans de Goede ","To":"Hans de Goede , libcamera-devel@lists.libcamera.org","Date":"Thu, 25 Sep 2025 11:03:48 +0100","Message-ID":"<175879462815.88644.10985798763928439670@isaac-ThinkPad-T16-Gen-2>","User-Agent":"alot/0.10","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":35976,"web_url":"https://patchwork.libcamera.org/comment/35976/","msgid":"","date":"2025-09-25T12:05:38","subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","submitter":{"id":239,"url":"https://patchwork.libcamera.org/api/people/239/","name":"Hans de Goede","email":"hansg@kernel.org"},"content":"Hi Milan,\n\nOn 24-Sep-25 09:54, Milan Zamazal wrote:\n> Hi Hans,\n> \n> thank you for the patch.\n> \n> Hans de Goede writes:\n> \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 then its default\n> \n> s/then/than/\n\nAck.\n\n>> ctrl value and after that lower the exposure instead of lowering\n>> 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 very low gain which is no good.\n> \n> Do I understand it correctly that the gain should never be set below the\n> default value and bright images can then be fully handled by reducing exposure?\n\nYes basically the pipeline for a single pixel looks like this:\n\nlight-sensor -> gain -> adc\n\nWhere the light-sensor gives of an analog signal between\nblack-level and max.\n\nNow lets say initially there is a lens-cap covering the sensor, so AGC\nboosts exposure and gain to their max values and the room is brightly lit\nwith daylight, so after removing the cap the image brightness is way too\nhigh.\n\nNow with the max exposure pretty much all pixels will be outputting\ntheir max analog signal. So changing the gain to < 1.0 (if supported)\nwill result in the frames going from fully white to light gray\nto 50% gray at which point the overall brightness measures as ok,\nbut we don't have a proper picture.\n\nWhere as we lower the gain from max-gain to default gain which typically\nis ~1.0 at which point the resulting picture is still full white\n(heavily overexposed) and then start lowering the exposure will result\nin a proper picture.\n\nActually thinking more about this, sensors where the min-gain advertised\nby the sensor-driver is less then 1.0 is probably why we have the weird\ncode to come up with some alternative min-value when the v4l2-ctrl reports\n0 as min value. E.g. many ov sensors have a linear gain where ctrl-value / 128\nis the gain value. So setting the ctrl to 128 results in a gain of 1.0 .\nMost of these report a min-value of 128, as well as a default value of\n128. But I'm pretty sure the hw will accept 0 just fine too and then\nset a gain to 0.0 (not really useful), or accept 64 for a gain of 0.5 .\n\nI think the magic min-gain == 0 handling is based on sensor drivers\nwhich do allow control of the full gain range (this was inherited from\nAndrey's work). So if we go with this change then we can also remove\nthat weird magic handling of min-gain == 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>> then the default gain, the gain should be lowered first.\n> \n> s/then/than/\n\nAck.\n\nRegards,\n\nHans\n\n\n\n> \n>> Signed-off-by: Hans de Goede \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 c46bb0ebe..94961f9fe 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.againDef) {\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 a471b80ae..ba525a881 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, againDef, 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 e70439ee5..f0764ef46 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.againDef = camHelper_->gain(againDef);\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.againDef = againDef;\n>> \t\tif (againMin) {\n>> \t\t\tcontext_.configuration.agc.againMin = againMin;\n>> \t\t} else {\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 0DC8FBDB1C\n\tfor ;\n\tThu, 25 Sep 2025 12:05:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 255146B5F3;\n\tThu, 25 Sep 2025 14:05:47 +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 4E3B869318\n\tfor ;\n\tThu, 25 Sep 2025 14:05:43 +0200 (CEST)","from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58])\n\tby sea.source.kernel.org (Postfix) with ESMTP id 7C47744D71;\n\tThu, 25 Sep 2025 12:05:41 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id AE2F4C4CEF0;\n\tThu, 25 Sep 2025 12:05:40 +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=\"EJgPE04c\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1758801941;\n\tbh=14ZuIAhIhONcLh0wi70L1ujalXSkKvZt8iOP5IofzEo=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=EJgPE04cM/WF7RLD549OVtNW88vfRP49pyz+WzpJ+fdc7/bLQRDmOEq0/wsnzV33j\n\thf+1Z2p5O8fkTNEzu50MBtU52aqZSHqikl/S9X7g6jwoRJReCKnq/0X0qIV/ZGWgSU\n\t2rgu5KUAgBCTSDMAWz3cIUDSlD2J8aED78w5gQQNnjlBw59VmR+C1FBJDeBB+QOR1Z\n\t/66lgwARGb1zwBRUbhg3tYht6K0KbHg/YNi/qnnwzSn+90DcEhIO9yu/xsZIqvamHR\n\tZNv0peNy609N4rm7lTzA6H9lsN6Txuc5qdtv+eEM7EuOW8IMNlMJ1IFDisKmvNFNgk\n\t4p5ZuYNmQirrQ==","Message-ID":"","Date":"Thu, 25 Sep 2025 14:05:38 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250923190657.21453-1-hansg@kernel.org>\n\t<20250923190657.21453-3-hansg@kernel.org>\n\t<85zfakmgkc.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Content-Language":"en-US, nl","From":"Hans de Goede ","In-Reply-To":"<85zfakmgkc.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","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\" "}},{"id":35977,"web_url":"https://patchwork.libcamera.org/comment/35977/","msgid":"<4debb843-56f9-4919-9938-e3762e015d44@kernel.org>","date":"2025-09-25T12:08:50","subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","submitter":{"id":239,"url":"https://patchwork.libcamera.org/api/people/239/","name":"Hans de Goede","email":"hansg@kernel.org"},"content":"Hi Isaac,\n\nOn 25-Sep-25 12:03, Isaac Scott wrote:\n> Hi Hans,\n> \n> Thank you for the patch!\n> \n> Quoting Hans de Goede (2025-09-23 20:06:54)\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 then its default\n>> ctrl value and after that lower the exposure instead of lowering\n>> the gain.\n> \n> I may be wrong, but what if the default gain is something like 2.0,\n> wouldn't that mean the light levels received at maximum exposure would\n> be multiplied by 2?\n> \n> While I agree that using the absolute minimum is probably a bad idea (as\n> even if it goes < 1, we're still artificially modifying the data), I\n> think we maybe should be able to reduce the analogue gain to 1.0 at a\n> minimum before touching exposure, to ensure we're making the most of the\n> exposure we have.\n\nI agree 1.0 is the value we want to use. But if we don't have\nthe sensor listed in the sensor-helper then we don't now what\nvalue corresponds to a gain of 1.0 and then using the default gain\nis a better option then using the min-gain, also see my longer reply\nto Milan.\n\nI do think that using 1.0 when we do have the helper is best. So I'll\nchange that for the next version.\n\nRegards,\n\nHans\n\n\n\n\n>> Otherwise there might be a heavily overexposed image (e.g. all white)\n>> which then is made less white by a very low gain which is no good.\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>> then the default gain, the gain should be lowered first.\n>>\n>> Signed-off-by: Hans de Goede \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 c46bb0ebe..94961f9fe 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.againDef) {\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 a471b80ae..ba525a881 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, againDef, 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 e70439ee5..f0764ef46 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.againDef = camHelper_->gain(againDef);\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.againDef = 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 20030BDB1C\n\tfor ;\n\tThu, 25 Sep 2025 12:08:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0832F6B5F3;\n\tThu, 25 Sep 2025 14:08:57 +0200 (CEST)","from sea.source.kernel.org (sea.source.kernel.org [172.234.252.31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6AEF769318\n\tfor ;\n\tThu, 25 Sep 2025 14:08:54 +0200 (CEST)","from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58])\n\tby sea.source.kernel.org (Postfix) with ESMTP id 3320043265;\n\tThu, 25 Sep 2025 12:08:53 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 618BEC4CEF0;\n\tThu, 25 Sep 2025 12:08:52 +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=\"bFwQYY17\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1758802133;\n\tbh=Msj485dI6U6I2UPKnAdKcUvnTx9Ffjixd1soUnV1Etc=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=bFwQYY17+KubBNQ8ZEqXgzCV861wVDgsNF1SzJuvIMCnJe2tAKADjhQv66DL9UMOl\n\tv/jrDxZm/S+GIxoNMXRLphY55ZBDvJuSPf0lT8/jd/RGtCTgyrqUneGBWu/lF0Zx3T\n\tPZIfbNvXIhxb2nNcbZKvrggF7Wtdl+74zyEFrdWVyUihhwByloxbFBe3jG2ku8eaLS\n\t4F4UHAhk7CZVbzPp0Kg1Ges+Si+YIPtJewPIqMApiahehTo0LfMZnDtHw2mNOuOpNd\n\t+mgiIdu8iYUZkecqm51E0+zGf7DNdyrekfEWHpwY5QOAE9yXXf3YMrsAl15zEPt9hO\n\t3r5B3LGa+shlg==","Message-ID":"<4debb843-56f9-4919-9938-e3762e015d44@kernel.org>","Date":"Thu, 25 Sep 2025 14:08:50 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","To":"Isaac Scott ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250923190657.21453-1-hansg@kernel.org>\n\t<20250923190657.21453-3-hansg@kernel.org>\n\t<175879462815.88644.10985798763928439670@isaac-ThinkPad-T16-Gen-2>","Content-Language":"en-US, nl","From":"Hans de Goede ","In-Reply-To":"<175879462815.88644.10985798763928439670@isaac-ThinkPad-T16-Gen-2>","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\" "}},{"id":35978,"web_url":"https://patchwork.libcamera.org/comment/35978/","msgid":"<175880225105.1246375.13761609933755112697@ping.linuxembedded.co.uk>","date":"2025-09-25T12:10:51","subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Isaac Scott (2025-09-25 11:03:48)\n> Hi Hans,\n> \n> Thank you for the patch!\n> \n> Quoting Hans de Goede (2025-09-23 20:06:54)\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 then its default\n> > ctrl value and after that lower the exposure instead of lowering\n> > the gain.\n\nHave you encountered sensors where againMin != default ?\n\nWhat does the sensor driver convey in these instances? (What would min\nbe and what would default be?)\n\n> I may be wrong, but what if the default gain is something like 2.0,\n> wouldn't that mean the light levels received at maximum exposure would\n> be multiplied by 2?\n\nIndeed, I don't think we should ever go below 1.0x gain. But I think the\nissue here is that without a gain model - we don't know what a 1.0x gain\nis - so I anticipate we're 'hoping' that the default is always a gain\ncode that corresponds to 1.0x.\n\nWe probably don't have anything else to convey that here so I think this\nis actually correct given the context here.\n\n\n> While I agree that using the absolute minimum is probably a bad idea (as\n> even if it goes < 1, we're still artificially modifying the data), I\n> think we maybe should be able to reduce the analogue gain to 1.0 at a\n> minimum before touching exposure, to ensure we're making the most of the\n> exposure we have.\n\nI think it's likely rare to have a sensor with an analogue gain less\nthan 1.0. I don't think I've seen one - has anyone else?\n\n\n> \n> Best wishes,\n> Isaac\n> \n> > \n> > Otherwise there might be a heavily overexposed image (e.g. all white)\n> > which then is made less white by a very low gain which is no good.\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> > then the default gain, the gain should be lowered first.\n\nPresumably the gain is only /increased/ after the exposure reaches it's\nmaximum ?\n\nBut I suspect this is fine too.\n\n\n\n> > \n> > Signed-off-by: Hans de Goede \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 c46bb0ebe..94961f9fe 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.againDef) {\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 a471b80ae..ba525a881 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, againDef, 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 e70439ee5..f0764ef46 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.againDef = camHelper_->gain(againDef);\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.againDef = 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 0D02CC328C\n\tfor ;\n\tThu, 25 Sep 2025 12:10:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 093D369318;\n\tThu, 25 Sep 2025 14:10:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 512A269318\n\tfor ;\n\tThu, 25 Sep 2025 14:10:53 +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 AD0251419;\n\tThu, 25 Sep 2025 14:09:28 +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=\"rnMQSNRj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758802168;\n\tbh=/F+vIwudk3Q356lwpJNPtV3uLm15uPfdvC/q08w7cRs=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=rnMQSNRju7wn8Q4GVzmQWN9rlJxJLtaG8hWX2dhPVyHD16jF8NJjesUgn41e3TKMp\n\t3dkb7bh3bA9OBWA5eW3tQuVmrH8+DwsPkF0a4ug78YNv+zqFX9Vyp/3UDf4Ocogvvj\n\t1pf/qSnNBZByC9+BpGNahpo16AwZpc+ROnDj4fYY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<175879462815.88644.10985798763928439670@isaac-ThinkPad-T16-Gen-2>","References":"<20250923190657.21453-1-hansg@kernel.org>\n\t<20250923190657.21453-3-hansg@kernel.org>\n\t<175879462815.88644.10985798763928439670@isaac-ThinkPad-T16-Gen-2>","Subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","From":"Kieran Bingham ","Cc":"Hans de Goede ","To":"Hans de Goede ,\n\tIsaac Scott ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 25 Sep 2025 13:10:51 +0100","Message-ID":"<175880225105.1246375.13761609933755112697@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":35981,"web_url":"https://patchwork.libcamera.org/comment/35981/","msgid":"","date":"2025-09-25T12:37:20","subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","submitter":{"id":239,"url":"https://patchwork.libcamera.org/api/people/239/","name":"Hans de Goede","email":"hansg@kernel.org"},"content":"Hi,\n\nOn 25-Sep-25 14:10, Kieran Bingham wrote:\n> Quoting Isaac Scott (2025-09-25 11:03:48)\n>> Hi Hans,\n>>\n>> Thank you for the patch!\n>>\n>> Quoting Hans de Goede (2025-09-23 20:06:54)\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 then its default\n>>> ctrl value and after that lower the exposure instead of lowering\n>>> the gain.\n> \n> Have you encountered sensors where againMin != default ?\n\nYes for example for the ov2680 the V4L2_CID_ANALOGUE_GAIN range is\n0 - 1023 and the default gain is 250.\n\n> What does the sensor driver convey in these instances? (What would min\n> be and what would default be?)\n\nminimum gain is the minimum value the register accepts without behaving\nunexpectedly. Default gain should correspond to 1.0 .\n\n> \n>> I may be wrong, but what if the default gain is something like 2.0,\n>> wouldn't that mean the light levels received at maximum exposure would\n>> be multiplied by 2?\n> \n> Indeed, I don't think we should ever go below 1.0x gain. But I think the\n> issue here is that without a gain model - we don't know what a 1.0x gain\n> is - so I anticipate we're 'hoping' that the default is always a gain\n> code that corresponds to 1.0x.\n\nRight that is the idea, that default-gain == 1.0 Note many sensor drivers\nindeed use min-gain == def-gain in which case this patch is a no-op.\n\n> We probably don't have anything else to convey that here so I think this\n> is actually correct given the context here.\n\nRight.\n\n>> While I agree that using the absolute minimum is probably a bad idea (as\n>> even if it goes < 1, we're still artificially modifying the data), I\n>> think we maybe should be able to reduce the analogue gain to 1.0 at a\n>> minimum before touching exposure, to ensure we're making the most of the\n>> exposure we have.\n> \n> I think it's likely rare to have a sensor with an analogue gain less\n> than 1.0. I don't think I've seen one - has anyone else?\n\nI think (need to check) 0 on the ov2680 is actually 0.\n\nAnother example is the ov2740 which according to the sensor-helper code in\nlibcamera has a gain formula of:\n\ngain = gain-ctrl-reg / 128\n\nhave a gain-ctrl range exposed on V4L2_CID_ANALOGUE_GAIN of 128 - 1983,\nwhich gets written directly to the register. According to the formula above\nthis means 1.0 - 15.5 . But I wonder if the register won't accept lower\nvalues and then do a gain below 1.0. The 1.0 / 128 min is currently enforced\nby the driver. But that does not mean the hw won't accept lower values.\n\nLet me give this a test-run on an ov08x40 and get back to you.\n\n>>> Otherwise there might be a heavily overexposed image (e.g. all white)\n>>> which then is made less white by a very low gain which is no good.\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>>> then the default gain, the gain should be lowered first.\n> \n> Presumably the gain is only /increased/ after the exposure reaches it's\n> maximum ?\n\nWhen done by AGC yes, but libcamera atm inherits whatever control values\nwere set at libcamera startup which may use a gain > 1.0 while not being\nat exposure max.\n\nRegards,\n\nHans\n\n\n\n>>>\n>>> Signed-off-by: Hans de Goede \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 c46bb0ebe..94961f9fe 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.againDef) {\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 a471b80ae..ba525a881 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, againDef, 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 e70439ee5..f0764ef46 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.againDef = camHelper_->gain(againDef);\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.againDef = 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 8404BBDB1C\n\tfor ;\n\tThu, 25 Sep 2025 12:37:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 593B06B5F3;\n\tThu, 25 Sep 2025 14:37:28 +0200 (CEST)","from sea.source.kernel.org (sea.source.kernel.org [172.234.252.31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E002369318\n\tfor ;\n\tThu, 25 Sep 2025 14:37:25 +0200 (CEST)","from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58])\n\tby sea.source.kernel.org (Postfix) with ESMTP id 9EB5D44306;\n\tThu, 25 Sep 2025 12:37:24 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 7EC5EC4CEF0;\n\tThu, 25 Sep 2025 12:37:23 +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=\"QyP5pvKw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1758803844;\n\tbh=YI2GSwG4YpFZZiaxb04aR1LZoQfqtT2fKqDlT0G3Pto=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=QyP5pvKwnFygYqeCq6Q4yEkk2j3/jb1T44IddPD0gKO64Bxg/GidZJDqpum2T9qYI\n\tGDfoJ1chNw/9MlyicESIIxFFOUj83JBxvbs8SfdrRFlaJPWPVOcp0ktb/477ADx1CF\n\tUfQdzaEw8tUt6HtIY1/E1nFhnzvySLhnW2WrBkwFv5OsipjNyj1HNO6DU7x5A0/eGv\n\tcLNPxVqGxjyGLPdZLj7gvMcBUPcDvPGk8BjNWqhasRDJcyvXTrqL960+ok1nXl116t\n\tE1D53KyAdL8PxONRfOad2XvHPUIvERVWYAeBXyQhVnrAEyVRtXA125f7tZTZCRLzjz\n\teI6KfhLn9Z4lg==","Message-ID":"","Date":"Thu, 25 Sep 2025 14:37:20 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","To":"Kieran Bingham ,\n\tIsaac Scott ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250923190657.21453-1-hansg@kernel.org>\n\t<20250923190657.21453-3-hansg@kernel.org>\n\t<175879462815.88644.10985798763928439670@isaac-ThinkPad-T16-Gen-2>\n\t<175880225105.1246375.13761609933755112697@ping.linuxembedded.co.uk>","Content-Language":"en-US, nl","From":"Hans de Goede ","In-Reply-To":"<175880225105.1246375.13761609933755112697@ping.linuxembedded.co.uk>","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\" "}},{"id":35983,"web_url":"https://patchwork.libcamera.org/comment/35983/","msgid":"<2f0f4078-049a-48d3-8d09-67bf82a58133@kernel.org>","date":"2025-09-25T13:37:42","subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","submitter":{"id":239,"url":"https://patchwork.libcamera.org/api/people/239/","name":"Hans de Goede","email":"hansg@kernel.org"},"content":"Hi,\n\nOn 25-Sep-25 14:37, Hans de Goede wrote:\n> Hi,\n> \n> On 25-Sep-25 14:10, Kieran Bingham wrote:\n>> Quoting Isaac Scott (2025-09-25 11:03:48)\n>>> Hi Hans,\n>>>\n>>> Thank you for the patch!\n>>>\n>>> Quoting Hans de Goede (2025-09-23 20:06:54)\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 then its default\n>>>> ctrl value and after that lower the exposure instead of lowering\n>>>> the gain.\n>>\n>> Have you encountered sensors where againMin != default ?\n> \n> Yes for example for the ov2680 the V4L2_CID_ANALOGUE_GAIN range is\n> 0 - 1023 and the default gain is 250.\n> \n>> What does the sensor driver convey in these instances? (What would min\n>> be and what would default be?)\n> \n> minimum gain is the minimum value the register accepts without behaving\n> unexpectedly. Default gain should correspond to 1.0 .\n> \n>>\n>>> I may be wrong, but what if the default gain is something like 2.0,\n>>> wouldn't that mean the light levels received at maximum exposure would\n>>> be multiplied by 2?\n>>\n>> Indeed, I don't think we should ever go below 1.0x gain. But I think the\n>> issue here is that without a gain model - we don't know what a 1.0x gain\n>> is - so I anticipate we're 'hoping' that the default is always a gain\n>> code that corresponds to 1.0x.\n> \n> Right that is the idea, that default-gain == 1.0 Note many sensor drivers\n> indeed use min-gain == def-gain in which case this patch is a no-op.\n> \n>> We probably don't have anything else to convey that here so I think this\n>> is actually correct given the context here.\n> \n> Right.\n> \n>>> While I agree that using the absolute minimum is probably a bad idea (as\n>>> even if it goes < 1, we're still artificially modifying the data), I\n>>> think we maybe should be able to reduce the analogue gain to 1.0 at a\n>>> minimum before touching exposure, to ensure we're making the most of the\n>>> exposure we have.\n>>\n>> I think it's likely rare to have a sensor with an analogue gain less\n>> than 1.0. I don't think I've seen one - has anyone else?\n> \n> I think (need to check) 0 on the ov2680 is actually 0.\n> \n> Another example is the ov2740 which according to the sensor-helper code in\n> libcamera has a gain formula of:\n> \n> gain = gain-ctrl-reg / 128\n> \n> have a gain-ctrl range exposed on V4L2_CID_ANALOGUE_GAIN of 128 - 1983,\n> which gets written directly to the register. According to the formula above\n> this means 1.0 - 15.5 . But I wonder if the register won't accept lower\n> values and then do a gain below 1.0. The 1.0 / 128 min is currently enforced\n> by the driver. But that does not mean the hw won't accept lower values.\n> \n> Let me give this a test-run on an ov08x40 and get back to you.\n\nSo I modified the ov08x40 driver to allow values less then 128 and\ngave those values a try. On the ov08x40 sensor gain values below 128 result\nin the gain going up (so above 1.0) rather then down.\n\nRegards,\n\nHans\n\n\n\n\n>>>> Signed-off-by: Hans de Goede \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 c46bb0ebe..94961f9fe 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.againDef) {\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 a471b80ae..ba525a881 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, againDef, 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 e70439ee5..f0764ef46 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.againDef = camHelper_->gain(againDef);\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.againDef = againDef;\n>>>> if (againMin) {\n>>>> context_.configuration.agc.againMin = againMin;\n>>>> } else {\n>>>> -- \n>>>> 2.51.0\n>>>>\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 0F02BC328C\n\tfor ;\n\tThu, 25 Sep 2025 13:37:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAD296B5F3;\n\tThu, 25 Sep 2025 15:37:49 +0200 (CEST)","from sea.source.kernel.org (sea.source.kernel.org [172.234.252.31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7C35969318\n\tfor ;\n\tThu, 25 Sep 2025 15:37:47 +0200 (CEST)","from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58])\n\tby sea.source.kernel.org (Postfix) with ESMTP id B636244759;\n\tThu, 25 Sep 2025 13:37:45 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id B2937C4CEF0;\n\tThu, 25 Sep 2025 13:37:44 +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=\"j1rdkhZz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1758807465;\n\tbh=N0ozd5JhFPoaMEHQ9DfG6BmbPUNtXLI90GVwChH7gLY=;\n\th=Date:Subject:From:To:References:In-Reply-To:From;\n\tb=j1rdkhZz4mrpVC8tlbuc4pv/kJLmVpe+imJqUdVLqCA5PMRWPklGfI+ObhS2Hp+52\n\tRG8KfktLIDV/M4X6gYeYCQFzU2krWSt6Kx0ijvEGN/cNa+o0mTWKVLlucHADNsKlLF\n\tiDlbqPI1kOYNzt4dYdyCPUob2U9FRNQaqNbeKSUWItocmBnB7+BDpZusnlHQz6vK1+\n\t48QsU/RQG3mEq3drWqE5ir1xcQkDP81zXrHTDAVbYEzTGM4aiBbnEw8ToEXDJsTACe\n\tzj71gOjX4Pmu4KmIhmwSAjKens3XNN03zQA01rPCwkcrXPDiKbbw4WInsOu9lGeMIp\n\t02+TiRgP/VxRA==","Message-ID":"<2f0f4078-049a-48d3-8d09-67bf82a58133@kernel.org>","Date":"Thu, 25 Sep 2025 15:37:42 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","From":"Hans de Goede ","To":"Kieran Bingham ,\n\tIsaac Scott ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250923190657.21453-1-hansg@kernel.org>\n\t<20250923190657.21453-3-hansg@kernel.org>\n\t<175879462815.88644.10985798763928439670@isaac-ThinkPad-T16-Gen-2>\n\t<175880225105.1246375.13761609933755112697@ping.linuxembedded.co.uk>\n\t","Content-Language":"en-US, nl","In-Reply-To":"","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\" "}},{"id":35984,"web_url":"https://patchwork.libcamera.org/comment/35984/","msgid":"<85tt0qy7hc.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-09-25T13:41:51","subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hans de Goede writes:\n\n> Hi Milan,\n>\n> On 24-Sep-25 09:54, Milan Zamazal wrote:\n>> Hi Hans,\n>> \n>> thank you for the patch.\n>> \n>> Hans de Goede writes:\n>> \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 then its default\n>> \n>> s/then/than/\n>\n> Ack.\n>\n>>> ctrl value and after that lower the exposure instead of lowering\n>>> 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 very low gain which is no good.\n>> \n>> Do I understand it correctly that the gain should never be set below the\n>> default value and bright images can then be fully handled by reducing exposure?\n>\n> Yes basically the pipeline for a single pixel looks like this:\n>\n> light-sensor -> gain -> adc\n>\n> Where the light-sensor gives of an analog signal between\n> black-level and max.\n>\n> Now lets say initially there is a lens-cap covering the sensor, so AGC\n> boosts exposure and gain to their max values and the room is brightly lit\n> with daylight, so after removing the cap the image brightness is way too\n> high.\n>\n> Now with the max exposure pretty much all pixels will be outputting\n> their max analog signal. So changing the gain to < 1.0 (if supported)\n> will result in the frames going from fully white to light gray\n> to 50% gray at which point the overall brightness measures as ok,\n> but we don't have a proper picture.\n\nI assume this applies also to situations where exposure is at the\nminimum value but the image is so bright that a sensor with the default\ngain is still saturated, right?\n\nReviewed-by: Milan Zamazal \n\n> Where as we lower the gain from max-gain to default gain which typically\n> is ~1.0 at which point the resulting picture is still full white\n> (heavily overexposed) and then start lowering the exposure will result\n> in a proper picture.\n>\n> Actually thinking more about this, sensors where the min-gain advertised\n> by the sensor-driver is less then 1.0 is probably why we have the weird\n> code to come up with some alternative min-value when the v4l2-ctrl reports\n> 0 as min value. E.g. many ov sensors have a linear gain where ctrl-value / 128\n> is the gain value. So setting the ctrl to 128 results in a gain of 1.0 .\n> Most of these report a min-value of 128, as well as a default value of\n> 128. But I'm pretty sure the hw will accept 0 just fine too and then\n> set a gain to 0.0 (not really useful), or accept 64 for a gain of 0.5 .\n>\n> I think the magic min-gain == 0 handling is based on sensor drivers\n> which do allow control of the full gain range (this was inherited from\n> Andrey's work). So if we go with this change then we can also remove\n> that weird magic handling of min-gain == 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>>> then the default gain, the gain should be lowered first.\n>> \n>> s/then/than/\n>\n> Ack.\n>\n> Regards,\n>\n> Hans\n>\n>\n>\n>> \n>>> Signed-off-by: Hans de Goede \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 c46bb0ebe..94961f9fe 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.againDef) {\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 a471b80ae..ba525a881 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, againDef, 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 e70439ee5..f0764ef46 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.againDef = camHelper_->gain(againDef);\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.againDef = againDef;\n>>> \t\tif (againMin) {\n>>> \t\t\tcontext_.configuration.agc.againMin = againMin;\n>>> \t\t} else {\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 006C3BDB1C\n\tfor ;\n\tThu, 25 Sep 2025 13:42:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE6AC6B5AA;\n\tThu, 25 Sep 2025 15:42:00 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 85EEA69318\n\tfor ;\n\tThu, 25 Sep 2025 15:41:59 +0200 (CEST)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-592-2oMkJszKO2m0Cnu9kOITOg-1; Thu, 25 Sep 2025 09:41:55 -0400","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-45df7e734e0so4938275e9.0\n\tfor ;\n\tThu, 25 Sep 2025 06:41:55 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-46e33be3268sm34835835e9.11.2025.09.25.06.41.52\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 25 Sep 2025 06:41:52 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"QKEbMFWY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1758807718;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=KrgE4L3XVFyA5js57cwx7tuaGJD0UUmOnABYqAawK9M=;\n\tb=QKEbMFWY5ABheaCmJ4MN1L6CeIjlrKad/zcvAseniIatdJmjruJNmD6K54wqtrJHyQ4YMF\n\t8Yx6AMCN23BDtsXuE6ED0bBi+uMZXQgBVMJyS4fTy7N4E6reeLf3+rn+X0DF6SF88lywTb\n\tOSZ2+BlsqTtWjaIvTEVoIMfcluolIbg=","X-MC-Unique":"2oMkJszKO2m0Cnu9kOITOg-1","X-Mimecast-MFC-AGG-ID":"2oMkJszKO2m0Cnu9kOITOg_1758807714","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1758807714; x=1759412514;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=KrgE4L3XVFyA5js57cwx7tuaGJD0UUmOnABYqAawK9M=;\n\tb=hwUDJ4uwyXTfZSJNLvuEq/1mhDYUpFjNFjazHNu2k3mJwn2mZmg7mfX4vCd+JSiTEn\n\t9CUAx1cOrB/AcYKQnojh4erK7DfR7tHvZ+ox7WXtxbqZ6ww2HjnJwQ1jL4TqtwHu6HD5\n\tTZhbbH6RvGBSN08/cGScU1J8kpD97nRO3JsYH8HMHcgj4O+G7+fgssPfo07JIJWVaMQN\n\tfSRvTOdhmVRN4G9y9uHDIm9dlWBEG+Dr5A1+ioFze2WEaXnitfqiXU83oK8qQac7Qwvq\n\tuWtTP6vX6iNOl6WNYZ2iVe1lLDI98Es+p1BxmTiIoSgw5OvrWK5Ngyor5YitTpHhVdzO\n\tphQA==","X-Gm-Message-State":"AOJu0YxsRnLHM8AvSLomqMeDA1Z7zxkorG/VE0qMvtN0J4YzyL34g4SV\n\tFDXw7HcaLjj0gkvezU15YQtwtHGEQh+FnukqskL58TFriN1T3fwstKFpIhqM/b2ZKkRtX2Ngp2a\n\thx1OZ4+ZyW2RR+zy39EcyfUlXF0qSgFq5bC4zIJ/p7dEBXwz0d8FjbPDshJjZcN3wsafbxz7Jvb\n\t+QnjgXSmaoWQbHhJd/O+HLBhxuYti4O6hW9EdemuBLlElfP4hpLDAUOVE5S90=","X-Gm-Gg":"ASbGncsUyBQH5retqyswTXJNz8sD+gjs9VMW2ZZZR00xm+D8Yx0iha11xmJifg5E4GZ\n\tDGAmn4VYjezRDdHExchz7W3QrWaaRz+LWrlD0fyQnMNOd5XXXzLnVNs8YBrxxRDvPe2YaS6AwmI\n\tUIkE4iQ3hqk+oml466SVgz3zKdl+qIUvPKqzbGDi1QB++QwL0aJD+UojxtVG0tG0tABV+iSkSvN\n\t+D3E6ERpPMnFxdAxkJH/kcq1/1+XTSE64/DjRUsXaExZo1JTenfCZJOz1e/MhXl7KhV4zGcZY18\n\t9FSXbQyydTCRWH8B/AK3k7KHMXWuXEeJAyJ5+mLv4ucE13JnsyMJR3KG72uqhXJnZCLo7OlYptq\n\tsbKgpKvTwlslwPFizHA==","X-Received":["by 2002:a05:600c:3b10:b0:45d:f83b:96aa with SMTP id\n\t5b1f17b1804b1-46e3299a5aamr35969595e9.7.1758807713984; \n\tThu, 25 Sep 2025 06:41:53 -0700 (PDT)","by 2002:a05:600c:3b10:b0:45d:f83b:96aa with SMTP id\n\t5b1f17b1804b1-46e3299a5aamr35969305e9.7.1758807713272; \n\tThu, 25 Sep 2025 06:41:53 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGZiVl6bxEWuD+NPvqMyCS6NRaadlosNOAMy6U8ng/ZZGy2ON3biX/zntbb5jVAULzuVgY1Yw==","From":"Milan Zamazal ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","In-Reply-To":" (Hans de\n\tGoede's message of \"Thu, 25 Sep 2025 14:05:38 +0200\")","References":"<20250923190657.21453-1-hansg@kernel.org>\n\t<20250923190657.21453-3-hansg@kernel.org>\n\t<85zfakmgkc.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t","Date":"Thu, 25 Sep 2025 15:41:51 +0200","Message-ID":"<85tt0qy7hc.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"wEQiTGB3OUMF_Jr4TkIHzN7AE3ok0uLuhWXm9cDjHzE_1758807714","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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":35985,"web_url":"https://patchwork.libcamera.org/comment/35985/","msgid":"<175880874950.1246375.11267888285882107519@ping.linuxembedded.co.uk>","date":"2025-09-25T13:59:09","subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","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-25 14:37:42)\n> Hi,\n> \n> On 25-Sep-25 14:37, Hans de Goede wrote:\n> > Hi,\n> > \n> > On 25-Sep-25 14:10, Kieran Bingham wrote:\n> >> Quoting Isaac Scott (2025-09-25 11:03:48)\n> >>> Hi Hans,\n> >>>\n> >>> Thank you for the patch!\n> >>>\n> >>> Quoting Hans de Goede (2025-09-23 20:06:54)\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 then its default\n> >>>> ctrl value and after that lower the exposure instead of lowering\n> >>>> the gain.\n> >>\n> >> Have you encountered sensors where againMin != default ?\n> > \n> > Yes for example for the ov2680 the V4L2_CID_ANALOGUE_GAIN range is\n> > 0 - 1023 and the default gain is 250.\n> > \n> >> What does the sensor driver convey in these instances? (What would min\n> >> be and what would default be?)\n> > \n> > minimum gain is the minimum value the register accepts without behaving\n> > unexpectedly. Default gain should correspond to 1.0 .\n> > \n> >>\n> >>> I may be wrong, but what if the default gain is something like 2.0,\n> >>> wouldn't that mean the light levels received at maximum exposure would\n> >>> be multiplied by 2?\n> >>\n> >> Indeed, I don't think we should ever go below 1.0x gain. But I think the\n> >> issue here is that without a gain model - we don't know what a 1.0x gain\n> >> is - so I anticipate we're 'hoping' that the default is always a gain\n> >> code that corresponds to 1.0x.\n> > \n> > Right that is the idea, that default-gain == 1.0 Note many sensor drivers\n> > indeed use min-gain == def-gain in which case this patch is a no-op.\n> > \n> >> We probably don't have anything else to convey that here so I think this\n> >> is actually correct given the context here.\n> > \n> > Right.\n> > \n> >>> While I agree that using the absolute minimum is probably a bad idea (as\n> >>> even if it goes < 1, we're still artificially modifying the data), I\n> >>> think we maybe should be able to reduce the analogue gain to 1.0 at a\n> >>> minimum before touching exposure, to ensure we're making the most of the\n> >>> exposure we have.\n> >>\n> >> I think it's likely rare to have a sensor with an analogue gain less\n> >> than 1.0. I don't think I've seen one - has anyone else?\n> > \n> > I think (need to check) 0 on the ov2680 is actually 0.\n> > \n> > Another example is the ov2740 which according to the sensor-helper code in\n> > libcamera has a gain formula of:\n> > \n> > gain = gain-ctrl-reg / 128\n> > \n> > have a gain-ctrl range exposed on V4L2_CID_ANALOGUE_GAIN of 128 - 1983,\n> > which gets written directly to the register. According to the formula above\n> > this means 1.0 - 15.5 . But I wonder if the register won't accept lower\n> > values and then do a gain below 1.0. The 1.0 / 128 min is currently enforced\n> > by the driver. But that does not mean the hw won't accept lower values.\n> > \n> > Let me give this a test-run on an ov08x40 and get back to you.\n> \n> So I modified the ov08x40 driver to allow values less then 128 and\n> gave those values a try. On the ov08x40 sensor gain values below 128 result\n> in the gain going up (so above 1.0) rather then down.\n\nHaha - Ok well at least it's the kernels job to protect us from that\npart!\n\n--\nKieran\n\n> \n> Regards,\n> \n> Hans\n> \n> \n> \n> \n> >>>> Signed-off-by: Hans de Goede \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 c46bb0ebe..94961f9fe 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.againDef) {\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 a471b80ae..ba525a881 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, againDef, 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 e70439ee5..f0764ef46 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.againDef = camHelper_->gain(againDef);\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.againDef = againDef;\n> >>>> if (againMin) {\n> >>>> context_.configuration.agc.againMin = againMin;\n> >>>> } else {\n> >>>> -- \n> >>>> 2.51.0\n> >>>>\n> > \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 4FBA6BDB1C\n\tfor ;\n\tThu, 25 Sep 2025 13:59:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A31AF6B5F3;\n\tThu, 25 Sep 2025 15:59:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 21D2E69318\n\tfor ;\n\tThu, 25 Sep 2025 15:59:13 +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 2712C55;\n\tThu, 25 Sep 2025 15:57:48 +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=\"I2pyIG5P\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758808668;\n\tbh=C+388rjHxV1gnji0jMaqcvuIWvUsvj+BECKyPpJhm4o=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=I2pyIG5PHdx1HavcZeNTbRL/gafNIspav1OOtltX5U/bfeo4glanhdS2zLJxLhiZ4\n\twYZP/iQHxqNJn1VdM7c+SxcZhRyrgMicd+sbgohjoPjZREVUF3y8aS3EHUT4aujrxB\n\tgKpzEURQ+/+jp5KPbNkqVbEaQhoJSg+T3Gd2+hT8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<2f0f4078-049a-48d3-8d09-67bf82a58133@kernel.org>","References":"<20250923190657.21453-1-hansg@kernel.org>\n\t<20250923190657.21453-3-hansg@kernel.org>\n\t<175879462815.88644.10985798763928439670@isaac-ThinkPad-T16-Gen-2>\n\t<175880225105.1246375.13761609933755112697@ping.linuxembedded.co.uk>\n\t\n\t<2f0f4078-049a-48d3-8d09-67bf82a58133@kernel.org>","Subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","From":"Kieran Bingham ","To":"Hans de Goede ,\n\tIsaac Scott ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 25 Sep 2025 14:59:09 +0100","Message-ID":"<175880874950.1246375.11267888285882107519@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":35986,"web_url":"https://patchwork.libcamera.org/comment/35986/","msgid":"<90d9a0ab-2df2-494a-808d-c7690ab74f70@kernel.org>","date":"2025-09-25T14:00:45","subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","submitter":{"id":239,"url":"https://patchwork.libcamera.org/api/people/239/","name":"Hans de Goede","email":"hansg@kernel.org"},"content":"Hi Milan,\n\nOn 25-Sep-25 15:41, Milan Zamazal wrote:\n> Hans de Goede writes:\n> \n>> Hi Milan,\n>>\n>> On 24-Sep-25 09:54, Milan Zamazal wrote:\n>>> Hi Hans,\n>>>\n>>> thank you for the patch.\n>>>\n>>> Hans de Goede writes:\n>>>\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 then its default\n>>>\n>>> s/then/than/\n>>\n>> Ack.\n>>\n>>>> ctrl value and after that lower the exposure instead of lowering\n>>>> 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 very low gain which is no good.\n>>>\n>>> Do I understand it correctly that the gain should never be set below the\n>>> default value and bright images can then be fully handled by reducing exposure?\n>>\n>> Yes basically the pipeline for a single pixel looks like this:\n>>\n>> light-sensor -> gain -> adc\n>>\n>> Where the light-sensor gives of an analog signal between\n>> black-level and max.\n>>\n>> Now lets say initially there is a lens-cap covering the sensor, so AGC\n>> boosts exposure and gain to their max values and the room is brightly lit\n>> with daylight, so after removing the cap the image brightness is way too\n>> high.\n>>\n>> Now with the max exposure pretty much all pixels will be outputting\n>> their max analog signal. So changing the gain to < 1.0 (if supported)\n>> will result in the frames going from fully white to light gray\n>> to 50% gray at which point the overall brightness measures as ok,\n>> but we don't have a proper picture.\n> \n> I assume this applies also to situations where exposure is at the\n> minimum value but the image is so bright that a sensor with the default\n> gain is still saturated, right?\n\nThat should never happen. For the lens shading correction I've\npointed the camera directly at a bright LED flood light and\nthen I can get the image to not be saturated with an exposure\nof ~250 / 4000, or in other words 1/16th of a frame time.\n\nSo with an expected minimum exposure range of 0-511 where we\ncan lower the exposure time to 1/512th of a frame which should\nbe small enough for even an extremely bright image.\n\nRegards,\n\nHans\n\n\n\n\n\n> \n>> Where as we lower the gain from max-gain to default gain which typically\n>> is ~1.0 at which point the resulting picture is still full white\n>> (heavily overexposed) and then start lowering the exposure will result\n>> in a proper picture.\n>>\n>> Actually thinking more about this, sensors where the min-gain advertised\n>> by the sensor-driver is less then 1.0 is probably why we have the weird\n>> code to come up with some alternative min-value when the v4l2-ctrl reports\n>> 0 as min value. E.g. many ov sensors have a linear gain where ctrl-value / 128\n>> is the gain value. So setting the ctrl to 128 results in a gain of 1.0 .\n>> Most of these report a min-value of 128, as well as a default value of\n>> 128. But I'm pretty sure the hw will accept 0 just fine too and then\n>> set a gain to 0.0 (not really useful), or accept 64 for a gain of 0.5 .\n>>\n>> I think the magic min-gain == 0 handling is based on sensor drivers\n>> which do allow control of the full gain range (this was inherited from\n>> Andrey's work). So if we go with this change then we can also remove\n>> that weird magic handling of min-gain == 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>>>> then the default gain, the gain should be lowered first.\n>>>\n>>> s/then/than/\n>>\n>> Ack.\n>>\n>> Regards,\n>>\n>> Hans\n>>\n>>\n>>\n>>>\n>>>> Signed-off-by: Hans de Goede \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 c46bb0ebe..94961f9fe 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.againDef) {\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 a471b80ae..ba525a881 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, againDef, 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 e70439ee5..f0764ef46 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.againDef = camHelper_->gain(againDef);\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.againDef = againDef;\n>>>> \t\tif (againMin) {\n>>>> \t\t\tcontext_.configuration.agc.againMin = againMin;\n>>>> \t\t} else {\n>>>\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 E2B59BDB1C\n\tfor ;\n\tThu, 25 Sep 2025 14:00:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 677266B5F3;\n\tThu, 25 Sep 2025 16:00:52 +0200 (CEST)","from tor.source.kernel.org (tor.source.kernel.org\n\t[IPv6:2600:3c04:e001:324:0:1991:8:25])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E5E269318\n\tfor ;\n\tThu, 25 Sep 2025 16:00:50 +0200 (CEST)","from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58])\n\tby tor.source.kernel.org (Postfix) with ESMTP id D3BAF605B1;\n\tThu, 25 Sep 2025 14:00:48 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id EA567C4CEF0;\n\tThu, 25 Sep 2025 14:00:47 +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=\"q/U9o91T\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1758808848;\n\tbh=ZhzH1jc75zBeQjqhAOE7tonn0KgyUmFP6OBKtp6yBBw=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=q/U9o91TZTO+pAe16854xQCV4HMyGlelmY7mRUDmC3ayAEzon26r3veNV/iwdxO1j\n\tkTgoClNPuiIYgvtfqGMrnmOMHniONQ4+IkW7oH3uR3Y0NuPjJ3AM4aNLO7uMVxuZjZ\n\tqrsrRa98/myzcZ+3setgD6vghQcc+F+6jqEW/jhfvJxi5fR6W8CL+GVP955UONwdsR\n\tI7wu6b8B8xZ4udzITrs5l7isdDZndKK0JSk7kKMzfMbF9ERuF+6Ndl3HiDi89auaDC\n\tCtql1dRAUTE2EVmZGfSQULJUpPVsR4EIJvSs8daZYDjPYmDyOVJtW9IU4wlgPKRoh6\n\t/FdbX6g0a+4Pg==","Message-ID":"<90d9a0ab-2df2-494a-808d-c7690ab74f70@kernel.org>","Date":"Thu, 25 Sep 2025 16:00:45 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/5] ipa: software_isp: AGC: do not lower gain below\n\tdefault gain value","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250923190657.21453-1-hansg@kernel.org>\n\t<20250923190657.21453-3-hansg@kernel.org>\n\t<85zfakmgkc.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t\n\t<85tt0qy7hc.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Content-Language":"en-US, nl","From":"Hans de Goede ","In-Reply-To":"<85tt0qy7hc.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","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\" "}}]