[{"id":36884,"web_url":"https://patchwork.libcamera.org/comment/36884/","msgid":"<176346971332.880260.4987283514199265302@isaac-ThinkPad-T16-Gen-2>","date":"2025-11-18T12:41:53","subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","submitter":{"id":215,"url":"https://patchwork.libcamera.org/api/people/215/","name":"Isaac Scott","email":"isaac.scott@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch!\n\nReviewed-by: Isaac Scott \n\nQuoting Kieran Bingham (2025-11-14 00:54:22)\n> The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8\n> format, a range of 0.0 to (very nearly) 32.0.\n> \n> Convert usage to the new UQ5_8 FixedPoint Quantised type which will\n> support the conversion, clamping and quantisation so that the metadata\n> and debug prints can now report the effective gain applied instead of\n> the potentially inaccurate float.\n> \n> As the UQ5_8 type already clamps values, remove the explicit clamping.\n> This removes the clamping to a minimum of 1.0 gain, so we rely on\n> calculateNewEv to provide a valid gain.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/mali-c55/algorithms/agc.cpp | 20 ++++++++++----------\n> src/ipa/mali-c55/ipa_context.h | 6 +++---\n> 2 files changed, 13 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp\n> index f60fddac3f04..3f10b237f581 100644\n> --- a/src/ipa/mali-c55/algorithms/agc.cpp\n> +++ b/src/ipa/mali-c55/algorithms/agc.cpp\n> @@ -38,8 +38,8 @@ static constexpr unsigned int kNumHistogramBins = 256;\n> * format, a range of 0.0 to (very nearly) 32.0. We clamp from 1.0 to the actual\n> * max value which is 8191 * 2^-8.\n> */\n> -static constexpr double kMinDigitalGain = 1.0;\n> -static constexpr double kMaxDigitalGain = 31.99609375;\n> +static constexpr float kMinDigitalGain = 1.0;\n> +static constexpr float kMaxDigitalGain = UQ5_8::TraitsType::max;\n> \n> uint32_t AgcStatistics::decodeBinValue(uint16_t binVal)\n> {\n> @@ -236,7 +236,7 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame,\n> agc.manual.ispGain = *digitalGain;\n> \n> LOG(MaliC55Agc, Debug)\n> - << \"Digital gain set to \" << agc.manual.ispGain\n> + << \"Digital gain set to \" << agc.manual.ispGain.value()\n> << \" on request sequence \" << frame;\n> }\n> }\n> @@ -245,7 +245,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex\n> mali_c55_params_block block)\n> {\n> IPAActiveState &activeState = context.activeState;\n> - double gain;\n> + UQ5_8 gain;\n> \n> if (activeState.agc.autoEnabled)\n> gain = activeState.agc.automatic.ispGain;\n> @@ -256,7 +256,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex\n> block.header->flags = MALI_C55_PARAM_BLOCK_FL_NONE;\n> block.header->size = sizeof(struct mali_c55_params_digital_gain);\n> \n> - block.digital_gain->gain = floatingToFixedPoint<5, 8, uint16_t, double>(gain);\n> + block.digital_gain->gain = gain.quantized();\n> frameContext.agc.ispGain = gain;\n> \n> return block.header->size;\n> @@ -376,7 +376,7 @@ void Agc::process(IPAContext &context,\n> */\n> uint32_t exposure = frameContext.agc.exposure;\n> double analogueGain = frameContext.agc.sensorGain;\n> - double digitalGain = frameContext.agc.ispGain;\n> + double digitalGain = frameContext.agc.ispGain.value();\n> double totalGain = analogueGain * digitalGain;\n> utils::Duration currentShutter = exposure * configuration.sensor.lineDuration;\n> utils::Duration effectiveExposureValue = currentShutter * totalGain;\n> @@ -388,19 +388,19 @@ void Agc::process(IPAContext &context,\n> activeState.agc.exposureMode, statistics_.yHist,\n> effectiveExposureValue);\n> \n> - dGain = std::clamp(dGain, kMinDigitalGain, kMaxDigitalGain);\n> + UQ5_8 dGainQ = static_cast(dGain);\n> \n> LOG(MaliC55Agc, Debug)\n> << \"Divided up shutter, analogue gain and digital gain are \"\n> - << shutterTime << \", \" << aGain << \" and \" << dGain;\n> + << shutterTime << \", \" << aGain << \" and \" << dGainQ.value();\n> \n> activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n> activeState.agc.automatic.sensorGain = aGain;\n> - activeState.agc.automatic.ispGain = dGain;\n> + activeState.agc.automatic.ispGain = dGainQ;\n> \n> metadata.set(controls::ExposureTime, currentShutter.get());\n> metadata.set(controls::AnalogueGain, frameContext.agc.sensorGain);\n> - metadata.set(controls::DigitalGain, frameContext.agc.ispGain);\n> + metadata.set(controls::DigitalGain, frameContext.agc.ispGain.value());\n> metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK);\n> }\n> \n> diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h\n> index 86d060a731cb..2db3fdf9e5cf 100644\n> --- a/src/ipa/mali-c55/ipa_context.h\n> +++ b/src/ipa/mali-c55/ipa_context.h\n> @@ -41,12 +41,12 @@ struct IPAActiveState {\n> struct {\n> uint32_t exposure;\n> double sensorGain;\n> - double ispGain;\n> + UQ5_8 ispGain;\n> } automatic;\n> struct {\n> uint32_t exposure;\n> double sensorGain;\n> - double ispGain;\n> + UQ5_8 ispGain;\n> } manual;\n> bool autoEnabled;\n> uint32_t constraintMode;\n> @@ -64,7 +64,7 @@ struct IPAFrameContext : public FrameContext {\n> struct {\n> uint32_t exposure;\n> double sensorGain;\n> - double ispGain;\n> + UQ5_8 ispGain;\n> } agc;\n> \n> struct {\n> -- \n> 2.51.1\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 38420C0F1B\n\tfor ;\n\tTue, 18 Nov 2025 12:41:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DBB4E60AAC;\n\tTue, 18 Nov 2025 13:41:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A13C60AA0\n\tfor ;\n\tTue, 18 Nov 2025 13:41:56 +0100 (CET)","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 C9769D52;\n\tTue, 18 Nov 2025 13:39:52 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tPPhyB71\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763469592;\n\tbh=Z6EUH35Auf0M2UMiafF305J8zwnpZxWmYcmBKcYNw2s=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=tPPhyB71IxDLVhF6KeU1MLNhSWLn8q4KkgOyuGyo9QTIhXBJMhSbkX9DotGWH/Suu\n\tAJaDadwL66b1oFXhxhHaudIQSGvY7rB6Jh1QYiHH1Lv4SUrHLxbzK1c+BJ8Kn81SIp\n\tjbZKXXSu+TMDbGunSUnBrtAcy2g5JkWWD2+GMiMA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251114005428.90024-19-kieran.bingham@ideasonboard.com>","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-19-kieran.bingham@ideasonboard.com>","Subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","From":"Isaac Scott ","Cc":"Kieran Bingham ","To":"Kieran Bingham ,\n\tlibcamera devel ","Date":"Tue, 18 Nov 2025 12:41:53 +0000","Message-ID":"<176346971332.880260.4987283514199265302@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":36900,"web_url":"https://patchwork.libcamera.org/comment/36900/","msgid":"<20251119032631.GA10488@pendragon.ideasonboard.com>","date":"2025-11-19T04:30:07","subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, Nov 14, 2025 at 12:54:22AM +0000, Kieran Bingham wrote:\n> The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8\n> format, a range of 0.0 to (very nearly) 32.0.\n> \n> Convert usage to the new UQ5_8 FixedPoint Quantised type which will\n> support the conversion, clamping and quantisation so that the metadata\n> and debug prints can now report the effective gain applied instead of\n> the potentially inaccurate float.\n> \n> As the UQ5_8 type already clamps values, remove the explicit clamping.\n> This removes the clamping to a minimum of 1.0 gain, so we rely on\n> calculateNewEv to provide a valid gain.\n\nIs that guaranteed ? The function documentation doesn't indicate it, and\nit seems that the constraint is not enforced at least in the fixed\nexposure time and gain code path.\n\n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/mali-c55/algorithms/agc.cpp | 20 ++++++++++----------\n> src/ipa/mali-c55/ipa_context.h | 6 +++---\n> 2 files changed, 13 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp\n> index f60fddac3f04..3f10b237f581 100644\n> --- a/src/ipa/mali-c55/algorithms/agc.cpp\n> +++ b/src/ipa/mali-c55/algorithms/agc.cpp\n> @@ -38,8 +38,8 @@ static constexpr unsigned int kNumHistogramBins = 256;\n> * format, a range of 0.0 to (very nearly) 32.0. We clamp from 1.0 to the actual\n> * max value which is 8191 * 2^-8.\n> */\n> -static constexpr double kMinDigitalGain = 1.0;\n> -static constexpr double kMaxDigitalGain = 31.99609375;\n> +static constexpr float kMinDigitalGain = 1.0;\n> +static constexpr float kMaxDigitalGain = UQ5_8::TraitsType::max;\n> \n> uint32_t AgcStatistics::decodeBinValue(uint16_t binVal)\n> {\n> @@ -236,7 +236,7 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame,\n> \t\tagc.manual.ispGain = *digitalGain;\n> \n> \t\tLOG(MaliC55Agc, Debug)\n> -\t\t\t<< \"Digital gain set to \" << agc.manual.ispGain\n> +\t\t\t<< \"Digital gain set to \" << agc.manual.ispGain.value()\n> \t\t\t<< \" on request sequence \" << frame;\n> \t}\n> }\n> @@ -245,7 +245,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex\n> \t\t\t mali_c55_params_block block)\n> {\n> \tIPAActiveState &activeState = context.activeState;\n> -\tdouble gain;\n> +\tUQ5_8 gain;\n> \n> \tif (activeState.agc.autoEnabled)\n> \t\tgain = activeState.agc.automatic.ispGain;\n> @@ -256,7 +256,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex\n> \tblock.header->flags = MALI_C55_PARAM_BLOCK_FL_NONE;\n> \tblock.header->size = sizeof(struct mali_c55_params_digital_gain);\n> \n> -\tblock.digital_gain->gain = floatingToFixedPoint<5, 8, uint16_t, double>(gain);\n> +\tblock.digital_gain->gain = gain.quantized();\n> \tframeContext.agc.ispGain = gain;\n> \n> \treturn block.header->size;\n> @@ -376,7 +376,7 @@ void Agc::process(IPAContext &context,\n> \t */\n> \tuint32_t exposure = frameContext.agc.exposure;\n> \tdouble analogueGain = frameContext.agc.sensorGain;\n> -\tdouble digitalGain = frameContext.agc.ispGain;\n> +\tdouble digitalGain = frameContext.agc.ispGain.value();\n> \tdouble totalGain = analogueGain * digitalGain;\n> \tutils::Duration currentShutter = exposure * configuration.sensor.lineDuration;\n> \tutils::Duration effectiveExposureValue = currentShutter * totalGain;\n> @@ -388,19 +388,19 @@ void Agc::process(IPAContext &context,\n> \t\t\t activeState.agc.exposureMode, statistics_.yHist,\n> \t\t\t effectiveExposureValue);\n> \n> -\tdGain = std::clamp(dGain, kMinDigitalGain, kMaxDigitalGain);\n> +\tUQ5_8 dGainQ = static_cast(dGain);\n\nYou don't name quantizzed variables with a 'Q' suffix elsewhere, I\nwouldn't do it here either.\n\n> \n> \tLOG(MaliC55Agc, Debug)\n> \t\t<< \"Divided up shutter, analogue gain and digital gain are \"\n> -\t\t<< shutterTime << \", \" << aGain << \" and \" << dGain;\n> +\t\t<< shutterTime << \", \" << aGain << \" and \" << dGainQ.value();\n\nLooking forward to operator<<() :-)\n\n> \n> \tactiveState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n> \tactiveState.agc.automatic.sensorGain = aGain;\n> -\tactiveState.agc.automatic.ispGain = dGain;\n> +\tactiveState.agc.automatic.ispGain = dGainQ;\n> \n> \tmetadata.set(controls::ExposureTime, currentShutter.get());\n> \tmetadata.set(controls::AnalogueGain, frameContext.agc.sensorGain);\n> -\tmetadata.set(controls::DigitalGain, frameContext.agc.ispGain);\n> +\tmetadata.set(controls::DigitalGain, frameContext.agc.ispGain.value());\n> \tmetadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK);\n> }\n> \n> diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h\n> index 86d060a731cb..2db3fdf9e5cf 100644\n> --- a/src/ipa/mali-c55/ipa_context.h\n> +++ b/src/ipa/mali-c55/ipa_context.h\n> @@ -41,12 +41,12 @@ struct IPAActiveState {\n> \t\tstruct {\n> \t\t\tuint32_t exposure;\n> \t\t\tdouble sensorGain;\n> -\t\t\tdouble ispGain;\n> +\t\t\tUQ5_8 ispGain;\n> \t\t} automatic;\n> \t\tstruct {\n> \t\t\tuint32_t exposure;\n> \t\t\tdouble sensorGain;\n> -\t\t\tdouble ispGain;\n> +\t\t\tUQ5_8 ispGain;\n> \t\t} manual;\n> \t\tbool autoEnabled;\n> \t\tuint32_t constraintMode;\n> @@ -64,7 +64,7 @@ struct IPAFrameContext : public FrameContext {\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble sensorGain;\n> -\t\tdouble ispGain;\n> +\t\tUQ5_8 ispGain;\n> \t} agc;\n> \n> \tstruct {\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 E9DE7C0F1B\n\tfor ;\n\tWed, 19 Nov 2025 04:30:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9879F60A8B;\n\tWed, 19 Nov 2025 05:30:43 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F0AAD609D8\n\tfor ;\n\tWed, 19 Nov 2025 05:30:41 +0100 (CET)","from pendragon.ideasonboard.com (unknown [205.220.129.225])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 21ACE14B0; \n\tWed, 19 Nov 2025 05:28:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"a7xQbOkB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763526517;\n\tbh=00/Ma7Oq0aWTdCbeNpUM66h8jy6UnOCGx8vCyRi29H0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=a7xQbOkBX2S+rkqBC41rLWUC/0HIQShJS7lGfAJncwSwHP2BZWnK9eXao58R4enzX\n\tgWkvvxYHrxVI80mOVw9f6Q1muvSVYcimsloRmYwcYRPfS0epiWpsydZpBMkQ1JB8qE\n\tI44Hhot+4ofV7ezRfLIbryGdjlT06WHP2DP5bMcE=","Date":"Wed, 19 Nov 2025 13:30:07 +0900","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera devel ","Subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","Message-ID":"<20251119032631.GA10488@pendragon.ideasonboard.com>","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-19-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251114005428.90024-19-kieran.bingham@ideasonboard.com>","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":36921,"web_url":"https://patchwork.libcamera.org/comment/36921/","msgid":"<176356746267.1040004.17126839856803374021@ping.linuxembedded.co.uk>","date":"2025-11-19T15:51:02","subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2025-11-19 04:30:07)\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Fri, Nov 14, 2025 at 12:54:22AM +0000, Kieran Bingham wrote:\n> > The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8\n> > format, a range of 0.0 to (very nearly) 32.0.\n> > \n> > Convert usage to the new UQ5_8 FixedPoint Quantised type which will\n> > support the conversion, clamping and quantisation so that the metadata\n> > and debug prints can now report the effective gain applied instead of\n> > the potentially inaccurate float.\n> > \n> > As the UQ5_8 type already clamps values, remove the explicit clamping.\n> > This removes the clamping to a minimum of 1.0 gain, so we rely on\n> > calculateNewEv to provide a valid gain.\n> \n> Is that guaranteed ? The function documentation doesn't indicate it, and\n> it seems that the constraint is not enforced at least in the fixed\n> exposure time and gain code path.\n\nGuaranteed, no\nExpected, yes.\n\nThat's why I've stated it here.\n\nI haven't dwelled on this because if calculateNewEv() asks for a digital\ngain less than 1 ... I could imagine that it might really want that.\n\nOr if it asks for it - it could/should anticipate that it might get\napplied.\n\n\nStefan, would you expect/desire/anticipate digital gains less than 1.0 ?\nor should we restrict them either here or in the calculateNewEv() call?\n\n\n\n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/ipa/mali-c55/algorithms/agc.cpp | 20 ++++++++++----------\n> > src/ipa/mali-c55/ipa_context.h | 6 +++---\n> > 2 files changed, 13 insertions(+), 13 deletions(-)\n> > \n> > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp\n> > index f60fddac3f04..3f10b237f581 100644\n> > --- a/src/ipa/mali-c55/algorithms/agc.cpp\n> > +++ b/src/ipa/mali-c55/algorithms/agc.cpp\n> > @@ -38,8 +38,8 @@ static constexpr unsigned int kNumHistogramBins = 256;\n> > * format, a range of 0.0 to (very nearly) 32.0. We clamp from 1.0 to the actual\n> > * max value which is 8191 * 2^-8.\n> > */\n> > -static constexpr double kMinDigitalGain = 1.0;\n> > -static constexpr double kMaxDigitalGain = 31.99609375;\n> > +static constexpr float kMinDigitalGain = 1.0;\n> > +static constexpr float kMaxDigitalGain = UQ5_8::TraitsType::max;\n> > \n> > uint32_t AgcStatistics::decodeBinValue(uint16_t binVal)\n> > {\n> > @@ -236,7 +236,7 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame,\n> > agc.manual.ispGain = *digitalGain;\n> > \n> > LOG(MaliC55Agc, Debug)\n> > - << \"Digital gain set to \" << agc.manual.ispGain\n> > + << \"Digital gain set to \" << agc.manual.ispGain.value()\n> > << \" on request sequence \" << frame;\n> > }\n> > }\n> > @@ -245,7 +245,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex\n> > mali_c55_params_block block)\n> > {\n> > IPAActiveState &activeState = context.activeState;\n> > - double gain;\n> > + UQ5_8 gain;\n> > \n> > if (activeState.agc.autoEnabled)\n> > gain = activeState.agc.automatic.ispGain;\n> > @@ -256,7 +256,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex\n> > block.header->flags = MALI_C55_PARAM_BLOCK_FL_NONE;\n> > block.header->size = sizeof(struct mali_c55_params_digital_gain);\n> > \n> > - block.digital_gain->gain = floatingToFixedPoint<5, 8, uint16_t, double>(gain);\n> > + block.digital_gain->gain = gain.quantized();\n> > frameContext.agc.ispGain = gain;\n> > \n> > return block.header->size;\n> > @@ -376,7 +376,7 @@ void Agc::process(IPAContext &context,\n> > */\n> > uint32_t exposure = frameContext.agc.exposure;\n> > double analogueGain = frameContext.agc.sensorGain;\n> > - double digitalGain = frameContext.agc.ispGain;\n> > + double digitalGain = frameContext.agc.ispGain.value();\n> > double totalGain = analogueGain * digitalGain;\n> > utils::Duration currentShutter = exposure * configuration.sensor.lineDuration;\n> > utils::Duration effectiveExposureValue = currentShutter * totalGain;\n> > @@ -388,19 +388,19 @@ void Agc::process(IPAContext &context,\n> > activeState.agc.exposureMode, statistics_.yHist,\n> > effectiveExposureValue);\n> > \n> > - dGain = std::clamp(dGain, kMinDigitalGain, kMaxDigitalGain);\n> > + UQ5_8 dGainQ = static_cast(dGain);\n> \n> You don't name quantizzed variables with a 'Q' suffix elsewhere, I\n> wouldn't do it here either.\n\nWhat do you recommend?\n\nI can't call it dGain - that's already the floating point digital gain\nfrom calculateNewEv().\n\nquantizedDigitalGain gets a bit long ;-)\nqDGain?\n\nor digitalGain along side dGain ? I think we need to convey that /this/\nvariable is the quantized one.\n\n\n> \n> > \n> > LOG(MaliC55Agc, Debug)\n> > << \"Divided up shutter, analogue gain and digital gain are \"\n> > - << shutterTime << \", \" << aGain << \" and \" << dGain;\n> > + << shutterTime << \", \" << aGain << \" and \" << dGainQ.value();\n> \n> Looking forward to operator<<() :-)\n> \n> > \n> > activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n> > activeState.agc.automatic.sensorGain = aGain;\n> > - activeState.agc.automatic.ispGain = dGain;\n> > + activeState.agc.automatic.ispGain = dGainQ;\n> > \n> > metadata.set(controls::ExposureTime, currentShutter.get());\n> > metadata.set(controls::AnalogueGain, frameContext.agc.sensorGain);\n> > - metadata.set(controls::DigitalGain, frameContext.agc.ispGain);\n> > + metadata.set(controls::DigitalGain, frameContext.agc.ispGain.value());\n> > metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK);\n> > }\n> > \n> > diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h\n> > index 86d060a731cb..2db3fdf9e5cf 100644\n> > --- a/src/ipa/mali-c55/ipa_context.h\n> > +++ b/src/ipa/mali-c55/ipa_context.h\n> > @@ -41,12 +41,12 @@ struct IPAActiveState {\n> > struct {\n> > uint32_t exposure;\n> > double sensorGain;\n> > - double ispGain;\n> > + UQ5_8 ispGain;\n> > } automatic;\n> > struct {\n> > uint32_t exposure;\n> > double sensorGain;\n> > - double ispGain;\n> > + UQ5_8 ispGain;\n> > } manual;\n> > bool autoEnabled;\n> > uint32_t constraintMode;\n> > @@ -64,7 +64,7 @@ struct IPAFrameContext : public FrameContext {\n> > struct {\n> > uint32_t exposure;\n> > double sensorGain;\n> > - double ispGain;\n> > + UQ5_8 ispGain;\n> > } agc;\n> > \n> > struct {\n> > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 E9029C0F1B\n\tfor ;\n\tWed, 19 Nov 2025 15:51:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DD24E60A8B;\n\tWed, 19 Nov 2025 16:51:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1792C606A0\n\tfor ;\n\tWed, 19 Nov 2025 16:51:06 +0100 (CET)","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 166D4E7C;\n\tWed, 19 Nov 2025 16:49:01 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"f83lu04R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763567341;\n\tbh=YQMC/JFvwTAIp1VJ8MlYM43uMD6aj8v7xivyWWfuKIk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=f83lu04RnO9TAaLaoPDWl8hQqt6Rd3wgF1DEEFblA+7SSEUgUOmYCT/TMdyDFCf36\n\tgsSdbBn77OG34DXgDxA/U1RiJyrP2HEFVH8rJf1z0uY+/9NvKwedYCSgdtSP9YXD//\n\tucyB9DXRp5M22eejjl0DTYd2RJEJylAg7ZLgef80=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251119032631.GA10488@pendragon.ideasonboard.com>","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-19-kieran.bingham@ideasonboard.com>\n\t<20251119032631.GA10488@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","From":"Kieran Bingham ","Cc":"libcamera devel ","To":"Laurent Pinchart ,\n\tStefan Klug ","Date":"Wed, 19 Nov 2025 15:51:02 +0000","Message-ID":"<176356746267.1040004.17126839856803374021@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":36922,"web_url":"https://patchwork.libcamera.org/comment/36922/","msgid":"<176356857558.2176992.14943700306884964289@localhost>","date":"2025-11-19T16:09:35","subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Kieran,\n\nQuoting Kieran Bingham (2025-11-19 16:51:02)\n> Quoting Laurent Pinchart (2025-11-19 04:30:07)\n> > Hi Kieran,\n> > \n> > Thank you for the patch.\n> > \n> > On Fri, Nov 14, 2025 at 12:54:22AM +0000, Kieran Bingham wrote:\n> > > The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8\n> > > format, a range of 0.0 to (very nearly) 32.0.\n> > > \n> > > Convert usage to the new UQ5_8 FixedPoint Quantised type which will\n> > > support the conversion, clamping and quantisation so that the metadata\n> > > and debug prints can now report the effective gain applied instead of\n> > > the potentially inaccurate float.\n> > > \n> > > As the UQ5_8 type already clamps values, remove the explicit clamping.\n> > > This removes the clamping to a minimum of 1.0 gain, so we rely on\n> > > calculateNewEv to provide a valid gain.\n> > \n> > Is that guaranteed ? The function documentation doesn't indicate it, and\n> > it seems that the constraint is not enforced at least in the fixed\n> > exposure time and gain code path.\n> \n> Guaranteed, no\n> Expected, yes.\n> \n> That's why I've stated it here.\n> \n> I haven't dwelled on this because if calculateNewEv() asks for a digital\n> gain less than 1 ... I could imagine that it might really want that.\n> \n> Or if it asks for it - it could/should anticipate that it might get\n> applied.\n> \n> \n> Stefan, would you expect/desire/anticipate digital gains less than 1.0 ?\n> or should we restrict them either here or in the calculateNewEv() call?\n\nThat is hard to tell. I can't really come up with a reasonable use case.\nThe issue would arise if the scene is extremely bright and the minimum\nexposure time is too high to create a properly exposed image. I've never\nseen this in practice. If we then apply a digital gain < 1 all the\nsaturated pixels will end up grey, which is also most likely not what we\nwant. So my tendency would be to clamp at 1.0.\n\nNow regarding calculateNewEv()/splitExposure(): If you see that as a\nbuilding block to build all kinds of IPAs, I would expect it to split\nthe exposure within the available constraints and potentially return a\ndigital gain < 1. The IPA would then be responsible to clamp to 1. If we\ntreat it as the \"libcamera IPA base class\" we can move the clamping in\nthere as we currently don't see a reasonable use case for a digital gain\n< 1.0. Does that make sense?\n\nBest regards,\nStefan\n\n> \n> \n> \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > src/ipa/mali-c55/algorithms/agc.cpp | 20 ++++++++++----------\n> > > src/ipa/mali-c55/ipa_context.h | 6 +++---\n> > > 2 files changed, 13 insertions(+), 13 deletions(-)\n> > > \n> > > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp\n> > > index f60fddac3f04..3f10b237f581 100644\n> > > --- a/src/ipa/mali-c55/algorithms/agc.cpp\n> > > +++ b/src/ipa/mali-c55/algorithms/agc.cpp\n> > > @@ -38,8 +38,8 @@ static constexpr unsigned int kNumHistogramBins = 256;\n> > > * format, a range of 0.0 to (very nearly) 32.0. We clamp from 1.0 to the actual\n> > > * max value which is 8191 * 2^-8.\n> > > */\n> > > -static constexpr double kMinDigitalGain = 1.0;\n> > > -static constexpr double kMaxDigitalGain = 31.99609375;\n> > > +static constexpr float kMinDigitalGain = 1.0;\n> > > +static constexpr float kMaxDigitalGain = UQ5_8::TraitsType::max;\n> > > \n> > > uint32_t AgcStatistics::decodeBinValue(uint16_t binVal)\n> > > {\n> > > @@ -236,7 +236,7 @@ void Agc::queueRequest(IPAContext &context, const uint32_t frame,\n> > > agc.manual.ispGain = *digitalGain;\n> > > \n> > > LOG(MaliC55Agc, Debug)\n> > > - << \"Digital gain set to \" << agc.manual.ispGain\n> > > + << \"Digital gain set to \" << agc.manual.ispGain.value()\n> > > << \" on request sequence \" << frame;\n> > > }\n> > > }\n> > > @@ -245,7 +245,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex\n> > > mali_c55_params_block block)\n> > > {\n> > > IPAActiveState &activeState = context.activeState;\n> > > - double gain;\n> > > + UQ5_8 gain;\n> > > \n> > > if (activeState.agc.autoEnabled)\n> > > gain = activeState.agc.automatic.ispGain;\n> > > @@ -256,7 +256,7 @@ size_t Agc::fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContex\n> > > block.header->flags = MALI_C55_PARAM_BLOCK_FL_NONE;\n> > > block.header->size = sizeof(struct mali_c55_params_digital_gain);\n> > > \n> > > - block.digital_gain->gain = floatingToFixedPoint<5, 8, uint16_t, double>(gain);\n> > > + block.digital_gain->gain = gain.quantized();\n> > > frameContext.agc.ispGain = gain;\n> > > \n> > > return block.header->size;\n> > > @@ -376,7 +376,7 @@ void Agc::process(IPAContext &context,\n> > > */\n> > > uint32_t exposure = frameContext.agc.exposure;\n> > > double analogueGain = frameContext.agc.sensorGain;\n> > > - double digitalGain = frameContext.agc.ispGain;\n> > > + double digitalGain = frameContext.agc.ispGain.value();\n> > > double totalGain = analogueGain * digitalGain;\n> > > utils::Duration currentShutter = exposure * configuration.sensor.lineDuration;\n> > > utils::Duration effectiveExposureValue = currentShutter * totalGain;\n> > > @@ -388,19 +388,19 @@ void Agc::process(IPAContext &context,\n> > > activeState.agc.exposureMode, statistics_.yHist,\n> > > effectiveExposureValue);\n> > > \n> > > - dGain = std::clamp(dGain, kMinDigitalGain, kMaxDigitalGain);\n> > > + UQ5_8 dGainQ = static_cast(dGain);\n> > \n> > You don't name quantizzed variables with a 'Q' suffix elsewhere, I\n> > wouldn't do it here either.\n> \n> What do you recommend?\n> \n> I can't call it dGain - that's already the floating point digital gain\n> from calculateNewEv().\n> \n> quantizedDigitalGain gets a bit long ;-)\n> qDGain?\n> \n> or digitalGain along side dGain ? I think we need to convey that /this/\n> variable is the quantized one.\n> \n> \n> > \n> > > \n> > > LOG(MaliC55Agc, Debug)\n> > > << \"Divided up shutter, analogue gain and digital gain are \"\n> > > - << shutterTime << \", \" << aGain << \" and \" << dGain;\n> > > + << shutterTime << \", \" << aGain << \" and \" << dGainQ.value();\n> > \n> > Looking forward to operator<<() :-)\n> > \n> > > \n> > > activeState.agc.automatic.exposure = shutterTime / configuration.sensor.lineDuration;\n> > > activeState.agc.automatic.sensorGain = aGain;\n> > > - activeState.agc.automatic.ispGain = dGain;\n> > > + activeState.agc.automatic.ispGain = dGainQ;\n> > > \n> > > metadata.set(controls::ExposureTime, currentShutter.get());\n> > > metadata.set(controls::AnalogueGain, frameContext.agc.sensorGain);\n> > > - metadata.set(controls::DigitalGain, frameContext.agc.ispGain);\n> > > + metadata.set(controls::DigitalGain, frameContext.agc.ispGain.value());\n> > > metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK);\n> > > }\n> > > \n> > > diff --git a/src/ipa/mali-c55/ipa_context.h b/src/ipa/mali-c55/ipa_context.h\n> > > index 86d060a731cb..2db3fdf9e5cf 100644\n> > > --- a/src/ipa/mali-c55/ipa_context.h\n> > > +++ b/src/ipa/mali-c55/ipa_context.h\n> > > @@ -41,12 +41,12 @@ struct IPAActiveState {\n> > > struct {\n> > > uint32_t exposure;\n> > > double sensorGain;\n> > > - double ispGain;\n> > > + UQ5_8 ispGain;\n> > > } automatic;\n> > > struct {\n> > > uint32_t exposure;\n> > > double sensorGain;\n> > > - double ispGain;\n> > > + UQ5_8 ispGain;\n> > > } manual;\n> > > bool autoEnabled;\n> > > uint32_t constraintMode;\n> > > @@ -64,7 +64,7 @@ struct IPAFrameContext : public FrameContext {\n> > > struct {\n> > > uint32_t exposure;\n> > > double sensorGain;\n> > > - double ispGain;\n> > > + UQ5_8 ispGain;\n> > > } agc;\n> > > \n> > > struct {\n> > > \n> > \n> > -- \n> > Regards,\n> > \n> > Laurent Pinchart","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 DF539BD80A\n\tfor ;\n\tWed, 19 Nov 2025 16:09:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2C074609D8;\n\tWed, 19 Nov 2025 17:09:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5FCE2606A0\n\tfor ;\n\tWed, 19 Nov 2025 17:09:39 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:5419:7bb:83a3:3d7a])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 350BBDD9;\n\tWed, 19 Nov 2025 17:07:34 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dLUis4mY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763568454;\n\tbh=nPrVtwwrxhjhcHMJnjJ+HXUJrULtJzeiQm5jTHUIly4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=dLUis4mYFSwhHzj0qsTExE36JjOX3LTo8gIfvi9gUdQ08jKrgen4GoQ+XpKZYqs8X\n\tbxDY+q5YwwBYdDVxlP+cGUNJsBlMc4Gh2unzD0BrqwIHKbB9CE5F5IsNsuRIXCg+4/\n\tfq6dhMAgpvlqN5OHJ9/oUzi2Ty3FdlNhewkAMd6s=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176356746267.1040004.17126839856803374021@ping.linuxembedded.co.uk>","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-19-kieran.bingham@ideasonboard.com>\n\t<20251119032631.GA10488@pendragon.ideasonboard.com>\n\t<176356746267.1040004.17126839856803374021@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","From":"Stefan Klug ","Cc":"libcamera devel ","To":"Kieran Bingham ,\n\tLaurent Pinchart ","Date":"Wed, 19 Nov 2025 17:09:35 +0100","Message-ID":"<176356857558.2176992.14943700306884964289@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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":36923,"web_url":"https://patchwork.libcamera.org/comment/36923/","msgid":"<176356994620.1127434.2014255271888333154@ping.linuxembedded.co.uk>","date":"2025-11-19T16:32:26","subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-11-19 16:09:35)\n> Hi Kieran,\n> \n> Quoting Kieran Bingham (2025-11-19 16:51:02)\n> > Quoting Laurent Pinchart (2025-11-19 04:30:07)\n> > > Hi Kieran,\n> > > \n> > > Thank you for the patch.\n> > > \n> > > On Fri, Nov 14, 2025 at 12:54:22AM +0000, Kieran Bingham wrote:\n> > > > The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8\n> > > > format, a range of 0.0 to (very nearly) 32.0.\n> > > > \n> > > > Convert usage to the new UQ5_8 FixedPoint Quantised type which will\n> > > > support the conversion, clamping and quantisation so that the metadata\n> > > > and debug prints can now report the effective gain applied instead of\n> > > > the potentially inaccurate float.\n> > > > \n> > > > As the UQ5_8 type already clamps values, remove the explicit clamping.\n> > > > This removes the clamping to a minimum of 1.0 gain, so we rely on\n> > > > calculateNewEv to provide a valid gain.\n> > > \n> > > Is that guaranteed ? The function documentation doesn't indicate it, and\n> > > it seems that the constraint is not enforced at least in the fixed\n> > > exposure time and gain code path.\n> > \n> > Guaranteed, no\n> > Expected, yes.\n> > \n> > That's why I've stated it here.\n> > \n> > I haven't dwelled on this because if calculateNewEv() asks for a digital\n> > gain less than 1 ... I could imagine that it might really want that.\n> > \n> > Or if it asks for it - it could/should anticipate that it might get\n> > applied.\n> > \n> > \n> > Stefan, would you expect/desire/anticipate digital gains less than 1.0 ?\n> > or should we restrict them either here or in the calculateNewEv() call?\n> \n> That is hard to tell. I can't really come up with a reasonable use case.\n> The issue would arise if the scene is extremely bright and the minimum\n> exposure time is too high to create a properly exposed image. I've never\n> seen this in practice. If we then apply a digital gain < 1 all the\n> saturated pixels will end up grey, which is also most likely not what we\n> want. So my tendency would be to clamp at 1.0.\n> \n> Now regarding calculateNewEv()/splitExposure(): If you see that as a\n> building block to build all kinds of IPAs, I would expect it to split\n> the exposure within the available constraints and potentially return a\n> digital gain < 1. The IPA would then be responsible to clamp to 1. If we\n> treat it as the \"libcamera IPA base class\" we can move the clamping in\n> there as we currently don't see a reasonable use case for a digital gain\n> < 1.0. Does that make sense?\n\nIf a digital gain of < 1.0 doesn't make sense, then I don't think every\nIPA who uses calculateNewEv//splitExposure should do the same clamping,\nand we should make that helper guarantee it will always return gains >\n1.0.\n\n--\nKieran\n\n> Best regards,\n> Stefan","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 6759CC0F1B\n\tfor ;\n\tWed, 19 Nov 2025 16:32:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5CE6960A80;\n\tWed, 19 Nov 2025 17:32:30 +0100 (CET)","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 15365606A0\n\tfor ;\n\tWed, 19 Nov 2025 17:32:29 +0100 (CET)","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 5C1EDDD9;\n\tWed, 19 Nov 2025 17:30:24 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hEKysLaI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763569824;\n\tbh=GFicyqTA/JB9JWStMWkf1t2SWWRb+xWbF76aiJX7ipM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=hEKysLaIxodWPtkCPwGbIlE3e8klm0U2wWi4+q/vtAZ9DDGgZjV7IAS83K+2tSFnW\n\tbwoha0+0tBzBiKmUnDzbTZtEEJG5C2Wsk+kFflsudFaMEOZ2MD/c2FBD6Fq5f9d903\n\tF4JgUGB4vEO3p0plK46yjwLy5hOX1KGIwzc12Img=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176356857558.2176992.14943700306884964289@localhost>","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-19-kieran.bingham@ideasonboard.com>\n\t<20251119032631.GA10488@pendragon.ideasonboard.com>\n\t<176356746267.1040004.17126839856803374021@ping.linuxembedded.co.uk>\n\t<176356857558.2176992.14943700306884964289@localhost>","Subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","From":"Kieran Bingham ","Cc":"libcamera devel ","To":"Laurent Pinchart ,\n\tStefan Klug ","Date":"Wed, 19 Nov 2025 16:32:26 +0000","Message-ID":"<176356994620.1127434.2014255271888333154@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":36924,"web_url":"https://patchwork.libcamera.org/comment/36924/","msgid":"<176357012699.2176992.5672655895542472938@localhost>","date":"2025-11-19T16:35:26","subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Quoting Kieran Bingham (2025-11-19 17:32:26)\n> Quoting Stefan Klug (2025-11-19 16:09:35)\n> > Hi Kieran,\n> > \n> > Quoting Kieran Bingham (2025-11-19 16:51:02)\n> > > Quoting Laurent Pinchart (2025-11-19 04:30:07)\n> > > > Hi Kieran,\n> > > > \n> > > > Thank you for the patch.\n> > > > \n> > > > On Fri, Nov 14, 2025 at 12:54:22AM +0000, Kieran Bingham wrote:\n> > > > > The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8\n> > > > > format, a range of 0.0 to (very nearly) 32.0.\n> > > > > \n> > > > > Convert usage to the new UQ5_8 FixedPoint Quantised type which will\n> > > > > support the conversion, clamping and quantisation so that the metadata\n> > > > > and debug prints can now report the effective gain applied instead of\n> > > > > the potentially inaccurate float.\n> > > > > \n> > > > > As the UQ5_8 type already clamps values, remove the explicit clamping.\n> > > > > This removes the clamping to a minimum of 1.0 gain, so we rely on\n> > > > > calculateNewEv to provide a valid gain.\n> > > > \n> > > > Is that guaranteed ? The function documentation doesn't indicate it, and\n> > > > it seems that the constraint is not enforced at least in the fixed\n> > > > exposure time and gain code path.\n> > > \n> > > Guaranteed, no\n> > > Expected, yes.\n> > > \n> > > That's why I've stated it here.\n> > > \n> > > I haven't dwelled on this because if calculateNewEv() asks for a digital\n> > > gain less than 1 ... I could imagine that it might really want that.\n> > > \n> > > Or if it asks for it - it could/should anticipate that it might get\n> > > applied.\n> > > \n> > > \n> > > Stefan, would you expect/desire/anticipate digital gains less than 1.0 ?\n> > > or should we restrict them either here or in the calculateNewEv() call?\n> > \n> > That is hard to tell. I can't really come up with a reasonable use case.\n> > The issue would arise if the scene is extremely bright and the minimum\n> > exposure time is too high to create a properly exposed image. I've never\n> > seen this in practice. If we then apply a digital gain < 1 all the\n> > saturated pixels will end up grey, which is also most likely not what we\n> > want. So my tendency would be to clamp at 1.0.\n> > \n> > Now regarding calculateNewEv()/splitExposure(): If you see that as a\n> > building block to build all kinds of IPAs, I would expect it to split\n> > the exposure within the available constraints and potentially return a\n> > digital gain < 1. The IPA would then be responsible to clamp to 1. If we\n> > treat it as the \"libcamera IPA base class\" we can move the clamping in\n> > there as we currently don't see a reasonable use case for a digital gain\n> > < 1.0. Does that make sense?\n> \n> If a digital gain of < 1.0 doesn't make sense, then I don't think every\n> IPA who uses calculateNewEv//splitExposure should do the same clamping,\n> and we should make that helper guarantee it will always return gains >\n> 1.0.\n\nWell, just because I can't come up with a use case right now doesn't\nmean it doesn't make sense :-). But I'm fine with clamping in there...\n\nCheers,\nStefan\n\n> \n> --\n> Kieran\n> \n> > Best regards,\n> > Stefan","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 F0EB3BD80A\n\tfor ;\n\tWed, 19 Nov 2025 16:35:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53C39609E0;\n\tWed, 19 Nov 2025 17:35:32 +0100 (CET)","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 EC454609D8\n\tfor ;\n\tWed, 19 Nov 2025 17:35:30 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:5419:7bb:83a3:3d7a])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id DF05BDD9;\n\tWed, 19 Nov 2025 17:33:25 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oQRPehJG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763570006;\n\tbh=TXGok2np7lof057ksfMdVyQ9EfmkwmljP+ht/Zu2FCY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=oQRPehJGa/YmbSzOnNJ3zD/ghc/myOjv1LoWTbMUYqtgCDXT+k6joKufbC1+ZGsYf\n\tD+NQEHZURMfn1WuvM9Nq4D8juoYN5jvqYGKNxYdoDNkP40NVyTGirOYAjZxIr92BAs\n\tT3rRlkvZR+SPISE3/Hg52r+ZrapOw3c+k3x31SjI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176356994620.1127434.2014255271888333154@ping.linuxembedded.co.uk>","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-19-kieran.bingham@ideasonboard.com>\n\t<20251119032631.GA10488@pendragon.ideasonboard.com>\n\t<176356746267.1040004.17126839856803374021@ping.linuxembedded.co.uk>\n\t<176356857558.2176992.14943700306884964289@localhost>\n\t<176356994620.1127434.2014255271888333154@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","From":"Stefan Klug ","Cc":"libcamera devel ","To":"Kieran Bingham ,\n\tLaurent Pinchart ","Date":"Wed, 19 Nov 2025 17:35:26 +0100","Message-ID":"<176357012699.2176992.5672655895542472938@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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":36929,"web_url":"https://patchwork.libcamera.org/comment/36929/","msgid":"<20251120021636.GH10711@pendragon.ideasonboard.com>","date":"2025-11-20T02:16:36","subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Nov 19, 2025 at 05:35:26PM +0100, Stefan Klug wrote:\n> Quoting Kieran Bingham (2025-11-19 17:32:26)\n> > Quoting Stefan Klug (2025-11-19 16:09:35)\n> > > Quoting Kieran Bingham (2025-11-19 16:51:02)\n> > > > Quoting Laurent Pinchart (2025-11-19 04:30:07)\n> > > > > On Fri, Nov 14, 2025 at 12:54:22AM +0000, Kieran Bingham wrote:\n> > > > > > The Mali-C55 ISP has a digital gain block which allows setting gain in Q5.8\n> > > > > > format, a range of 0.0 to (very nearly) 32.0.\n> > > > > > \n> > > > > > Convert usage to the new UQ5_8 FixedPoint Quantised type which will\n> > > > > > support the conversion, clamping and quantisation so that the metadata\n> > > > > > and debug prints can now report the effective gain applied instead of\n> > > > > > the potentially inaccurate float.\n> > > > > > \n> > > > > > As the UQ5_8 type already clamps values, remove the explicit clamping.\n> > > > > > This removes the clamping to a minimum of 1.0 gain, so we rely on\n> > > > > > calculateNewEv to provide a valid gain.\n> > > > > \n> > > > > Is that guaranteed ? The function documentation doesn't indicate it, and\n> > > > > it seems that the constraint is not enforced at least in the fixed\n> > > > > exposure time and gain code path.\n> > > > \n> > > > Guaranteed, no\n> > > > Expected, yes.\n\nIf it's expected but not guaranteed then this patch may introduce a\nregression.\n\n> > > > That's why I've stated it here.\n> > > > \n> > > > I haven't dwelled on this because if calculateNewEv() asks for a digital\n> > > > gain less than 1 ... I could imagine that it might really want that.\n> > > > \n> > > > Or if it asks for it - it could/should anticipate that it might get\n> > > > applied.\n> > > > \n> > > > \n> > > > Stefan, would you expect/desire/anticipate digital gains less than 1.0 ?\n> > > > or should we restrict them either here or in the calculateNewEv() call?\n> > > \n> > > That is hard to tell. I can't really come up with a reasonable use case.\n> > > The issue would arise if the scene is extremely bright and the minimum\n> > > exposure time is too high to create a properly exposed image. I've never\n> > > seen this in practice. If we then apply a digital gain < 1 all the\n> > > saturated pixels will end up grey, which is also most likely not what we\n> > > want. So my tendency would be to clamp at 1.0.\n\nMost hardware won't allow applying a digital gain smaller than 1, and\neven if it was possible, it would be quite useless due to saturation.\n\n> > > Now regarding calculateNewEv()/splitExposure(): If you see that as a\n> > > building block to build all kinds of IPAs, I would expect it to split\n> > > the exposure within the available constraints and potentially return a\n> > > digital gain < 1. The IPA would then be responsible to clamp to 1. If we\n> > > treat it as the \"libcamera IPA base class\" we can move the clamping in\n> > > there as we currently don't see a reasonable use case for a digital gain\n> > > < 1.0. Does that make sense?\n> > \n> > If a digital gain of < 1.0 doesn't make sense, then I don't think every\n> > IPA who uses calculateNewEv//splitExposure should do the same clamping,\n> > and we should make that helper guarantee it will always return gains >\n> > 1.0.\n> \n> Well, just because I can't come up with a use case right now doesn't\n> mean it doesn't make sense :-). But I'm fine with clamping in there...\n\nI agree with Kieran that duplicating clamping is not ideal. This being\nsaid, we do duplicate code between AGC implementations already, and I\nthink Jacopo is working on fixing that by factoring out code to a common\nhelper in libipa. We can clamp in calculateNewEv(), or somewhere else,\ndepending on how we define calculateNewEv(). As long as things are well\ndesigned (as in giving calculateNewEv() a clear and meaningful purpose)\nand clearly documented, I don't mind much.","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 772D6C0F1B\n\tfor ;\n\tThu, 20 Nov 2025 02:17:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5EF960A80;\n\tThu, 20 Nov 2025 03:17:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 524F4606D5\n\tfor ;\n\tThu, 20 Nov 2025 03:16:59 +0100 (CET)","from pendragon.ideasonboard.com (fs276ed015.tkyc509.ap.nuro.jp\n\t[39.110.208.21])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 748E0B5;\n\tThu, 20 Nov 2025 03:14:53 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"iZU0P60i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763604894;\n\tbh=TCKn8C7EQJF96bBidplDviJpItUXYkZa/sdrYu+Wlrw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iZU0P60iF80yfjDmYtgG0+0DIMKkqbpUmT4Pwt0YH6Mnz7isufQ+d/XLxAdws3CFm\n\tXuRoUEPpfY7HIKZCB0vDmwl1Z4xcalShN/V8va9zH1pnhVvOBTzisL2R8iD5WnUY6l\n\tcReiUCunMq0AJtHMdTv/ZWItnc1uoB4wpbkTZfIg=","Date":"Thu, 20 Nov 2025 11:16:36 +0900","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"Kieran Bingham ,\n\tlibcamera devel ","Subject":"Re: [PATCH v4 18/21] ipa: mali-c55: agc: Quantise the ISP Digital\n\tGain","Message-ID":"<20251120021636.GH10711@pendragon.ideasonboard.com>","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-19-kieran.bingham@ideasonboard.com>\n\t<20251119032631.GA10488@pendragon.ideasonboard.com>\n\t<176356746267.1040004.17126839856803374021@ping.linuxembedded.co.uk>\n\t<176356857558.2176992.14943700306884964289@localhost>\n\t<176356994620.1127434.2014255271888333154@ping.linuxembedded.co.uk>\n\t<176357012699.2176992.5672655895542472938@localhost>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176357012699.2176992.5672655895542472938@localhost>","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\" "}}]