[{"id":35596,"web_url":"https://patchwork.libcamera.org/comment/35596/","msgid":"<175633432565.694985.2542454723943054602@ping.linuxembedded.co.uk>","date":"2025-08-27T22:38:45","subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Niklas,\n\nQuoting Niklas Söderlund (2025-08-27 23:20:32)\n> The digital gain is already calculated in the AGC algorithm but the\n> value is not consumed by the AWB algorithm, which is where the gain\n> stage is located. Add the needed plumbing to carry the value between the\n> two algorithms.\n> \n> This is needed to get good images from sensors such as the IMX219 where\n> large sensor frames needs a small VBLANK value. This results in images\n\nDo you really mean vblank here? Or is it just that you're hitting a\nlimitation on the exposure time or frame duration limit ? (and thus end\nup with a small vblank...?)\n\n> that are so under exposed that even the sensors analog gain can't\n> compensate. The digital gain stage of the ISP helps with this.\n\nThis is interesting - but I think would soon be superceeded by the\ncapabilities we can use in the companding block or such with recent\ndevelopment from Stefan in his WDR series ...\n\nSee [0] and in particular [1] ....\n[0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382\n[1] https://patchwork.libcamera.org/patch/24120/\n\nUsing AWB to apply a gain here can be problematic because we can\nsaturate a single channel and I think the quantisation of the gain here\nis quite coarse?\n\nBut perhaps we'll end up with potentially multiple places we 'could'\napply the digital gain ...\n\nKieran\n\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++--\n> src/ipa/rkisp1/algorithms/awb.cpp | 4 +++-\n> src/ipa/rkisp1/ipa_context.h | 3 +++\n> 3 files changed, 16 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 35440b67e999..17aaa259244b 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> {\n> /* Configure the default exposure and gain. */\n> context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain;\n> + context.activeState.agc.automatic.ispGain = 1.0;\n> context.activeState.agc.automatic.exposure =\n> 10ms / context.configuration.sensor.lineDuration;\n> context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> + context.activeState.agc.manual.ispGain = 1.0;\n> context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> context.activeState.agc.autoExposureEnabled = !context.configuration.raw;\n> context.activeState.agc.autoGainEnabled = !context.configuration.raw;\n> @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context,\n> \n> if (!frameContext.agc.autoExposureEnabled)\n> frameContext.agc.exposure = agc.manual.exposure;\n> - if (!frameContext.agc.autoGainEnabled)\n> + if (!frameContext.agc.autoGainEnabled) {\n> frameContext.agc.gain = agc.manual.gain;\n> + frameContext.agc.ispGain = agc.manual.ispGain;\n> + }\n> \n> const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> if (meteringMode) {\n> @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> {\n> uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;\n> double activeAutoGain = context.activeState.agc.automatic.gain;\n> + double activeAutoIspGain = context.activeState.agc.automatic.ispGain;\n> \n> /* Populate exposure and gain in auto mode */\n> if (frameContext.agc.autoExposureEnabled)\n> frameContext.agc.exposure = activeAutoExposure;\n> - if (frameContext.agc.autoGainEnabled)\n> + if (frameContext.agc.autoGainEnabled) {\n> frameContext.agc.gain = activeAutoGain;\n> + frameContext.agc.ispGain = activeAutoIspGain;\n> + }\n> \n> /*\n> * Populate manual exposure and gain from the active auto values when\n> @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> /* Update the estimated exposure and gain. */\n> activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n> activeState.agc.automatic.gain = aGain;\n> + activeState.agc.automatic.ispGain = dGain;\n> \n> /*\n> * Expand the target frame duration so that we do not run faster than\n> diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> index 399fb51be414..569cac4a3466 100644\n> --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,\n> */\n> if (frameContext.awb.autoEnabled) {\n> const auto &awb = context.activeState.awb;\n> - frameContext.awb.gains = awb.automatic.gains;\n> + const auto &agc = context.activeState.agc;\n> +\n> + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain;\n> frameContext.awb.temperatureK = awb.automatic.temperatureK;\n> }\n> \n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 7ccc7b501aff..7180b0b7dff2 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -73,10 +73,12 @@ struct IPAActiveState {\n> struct {\n> uint32_t exposure;\n> double gain;\n> + double ispGain;\n> } manual;\n> struct {\n> uint32_t exposure;\n> double gain;\n> + double ispGain;\n> } automatic;\n> \n> bool autoExposureEnabled;\n> @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext {\n> struct {\n> uint32_t exposure;\n> double gain;\n> + double ispGain;\n> double exposureValue;\n> uint32_t vblank;\n> bool autoExposureEnabled;\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 CBE23BD87C\n\tfor ;\n\tWed, 27 Aug 2025 22:38:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AB2BD692EF;\n\tThu, 28 Aug 2025 00:38:50 +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 B932C613B9\n\tfor ;\n\tThu, 28 Aug 2025 00:38:48 +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 6648D15D6;\n\tThu, 28 Aug 2025 00:37:44 +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=\"ET06PwwU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1756334264;\n\tbh=XugeTlDq8uyrQqwWm2H3+iL+b0UcqurF1MvNQ/qMO88=;\n\th=In-Reply-To:References:Subject:From:To:Cc:Date:From;\n\tb=ET06PwwU4v9kUIZitxTy3et7bqJD16KVd2/GG8JHOQGeb1LvLl45c+R4lMK+2rCPU\n\t18AAnWd66WKNhIXcpWV4vuNz7lYDyhjsAUS5t3d7Mv9pqN9XfPHcueN+t921+5oGF8\n\tO0QvgV7B1aXem5WzLJeC7bNimpIsLXH7F/wzIKj0=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250827222032.644435-1-niklas.soderlund+renesas@ragnatech.se>","References":"<20250827222032.644435-1-niklas.soderlund+renesas@ragnatech.se>","Subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","From":"Kieran Bingham ","To":"Niklas =?utf-8?q?S=C3=B6derlund?=\n\t, Paul Elder\n\t, libcamera-devel@lists.libcamera.org","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?=\n\t, Stefan Klug\n\t, ","Date":"Wed, 27 Aug 2025 23:38:45 +0100","Message-ID":"<175633432565.694985.2542454723943054602@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":35597,"web_url":"https://patchwork.libcamera.org/comment/35597/","msgid":"<20250827225856.GA2901748@ragnatech.se>","date":"2025-08-27T22:58:56","subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","submitter":{"id":230,"url":"https://patchwork.libcamera.org/api/people/230/","name":"Niklas Söderlund","email":"niklas.soderlund+renesas@ragnatech.se"},"content":"Hi Kieran,\n\nOn 2025-08-27 23:38:45 +0100, Kieran Bingham wrote:\n> Hi Niklas,\n> \n> Quoting Niklas Söderlund (2025-08-27 23:20:32)\n> > The digital gain is already calculated in the AGC algorithm but the\n> > value is not consumed by the AWB algorithm, which is where the gain\n> > stage is located. Add the needed plumbing to carry the value between the\n> > two algorithms.\n> > \n> > This is needed to get good images from sensors such as the IMX219 where\n> > large sensor frames needs a small VBLANK value. This results in images\n> \n> Do you really mean vblank here? Or is it just that you're hitting a\n> limitation on the exposure time or frame duration limit ? (and thus end\n> up with a small vblank...?)\n\nYes, they are all intertwined in my head ;-) The default FrameLimits \nrenders a VBLANK setting, this VBLANK setting limits the range of the \nEXPOSURE. Even if the IPA max the EXPOSURE and ANALOG_GAIN the image is \nvery dark. From a .ko modules point of view the driving factor is the \nVBLANK setting.\n\n> \n> > that are so under exposed that even the sensors analog gain can't\n> > compensate. The digital gain stage of the ISP helps with this.\n> \n> This is interesting - but I think would soon be superceeded by the\n> capabilities we can use in the companding block or such with recent\n> development from Stefan in his WDR series ...\n\nI can't tell if it will, or not.\n\n> \n> See [0] and in particular [1] ....\n> [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382\n> [1] https://patchwork.libcamera.org/patch/24120/\n> \n> Using AWB to apply a gain here can be problematic because we can\n> saturate a single channel\n\nI can set the gain on each channel separately, but I only get one value \nfrom libipa. Maybe I'm misunderstanding you, or maybe this is why it \nwill be superseded by Stefan's work?\n\n> and I think the quantisation of the gain here\n> is quite coarse?\n\nThe digital gain I setting used by this patch comes from libipa at the \nsame time as the exposure and analog gain settings. It's just that the \nrksip1 IPA have never used the digital gain setting returned.\n\n> \n> But perhaps we'll end up with potentially multiple places we 'could'\n> apply the digital gain ...\n\nI'm using a RPi Module 2 so I compered how the RPi IPA handles this, and \nit applies DIGITAL_GAIN on the sensor. I have tried this and it works, \nbut I gather using the sensor for DIGITAL_GAIN is not the best idea \nthese days. And I could not find any other block in the ISP where this \ncould be done.\n\n> \n> Kieran\n> \n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++--\n> > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++-\n> > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > 3 files changed, 16 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 35440b67e999..17aaa259244b 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > {\n> > /* Configure the default exposure and gain. */\n> > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain;\n> > + context.activeState.agc.automatic.ispGain = 1.0;\n> > context.activeState.agc.automatic.exposure =\n> > 10ms / context.configuration.sensor.lineDuration;\n> > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> > + context.activeState.agc.manual.ispGain = 1.0;\n> > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> > context.activeState.agc.autoExposureEnabled = !context.configuration.raw;\n> > context.activeState.agc.autoGainEnabled = !context.configuration.raw;\n> > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context,\n> > \n> > if (!frameContext.agc.autoExposureEnabled)\n> > frameContext.agc.exposure = agc.manual.exposure;\n> > - if (!frameContext.agc.autoGainEnabled)\n> > + if (!frameContext.agc.autoGainEnabled) {\n> > frameContext.agc.gain = agc.manual.gain;\n> > + frameContext.agc.ispGain = agc.manual.ispGain;\n> > + }\n> > \n> > const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> > if (meteringMode) {\n> > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > {\n> > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;\n> > double activeAutoGain = context.activeState.agc.automatic.gain;\n> > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain;\n> > \n> > /* Populate exposure and gain in auto mode */\n> > if (frameContext.agc.autoExposureEnabled)\n> > frameContext.agc.exposure = activeAutoExposure;\n> > - if (frameContext.agc.autoGainEnabled)\n> > + if (frameContext.agc.autoGainEnabled) {\n> > frameContext.agc.gain = activeAutoGain;\n> > + frameContext.agc.ispGain = activeAutoIspGain;\n> > + }\n> > \n> > /*\n> > * Populate manual exposure and gain from the active auto values when\n> > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > /* Update the estimated exposure and gain. */\n> > activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n> > activeState.agc.automatic.gain = aGain;\n> > + activeState.agc.automatic.ispGain = dGain;\n> > \n> > /*\n> > * Expand the target frame duration so that we do not run faster than\n> > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > index 399fb51be414..569cac4a3466 100644\n> > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,\n> > */\n> > if (frameContext.awb.autoEnabled) {\n> > const auto &awb = context.activeState.awb;\n> > - frameContext.awb.gains = awb.automatic.gains;\n> > + const auto &agc = context.activeState.agc;\n> > +\n> > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain;\n> > frameContext.awb.temperatureK = awb.automatic.temperatureK;\n> > }\n> > \n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 7ccc7b501aff..7180b0b7dff2 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -73,10 +73,12 @@ struct IPAActiveState {\n> > struct {\n> > uint32_t exposure;\n> > double gain;\n> > + double ispGain;\n> > } manual;\n> > struct {\n> > uint32_t exposure;\n> > double gain;\n> > + double ispGain;\n> > } automatic;\n> > \n> > bool autoExposureEnabled;\n> > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext {\n> > struct {\n> > uint32_t exposure;\n> > double gain;\n> > + double ispGain;\n> > double exposureValue;\n> > uint32_t vblank;\n> > bool autoExposureEnabled;\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 09B28BEFBE\n\tfor ;\n\tWed, 27 Aug 2025 22:59:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2D7D692EF;\n\tThu, 28 Aug 2025 00:59:02 +0200 (CEST)","from fhigh-b1-smtp.messagingengine.com\n\t(fhigh-b1-smtp.messagingengine.com [202.12.124.152])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3242F613AF\n\tfor ;\n\tThu, 28 Aug 2025 00:59:01 +0200 (CEST)","from phl-compute-06.internal (phl-compute-06.internal\n\t[10.202.2.46])\n\tby mailfhigh.stl.internal (Postfix) with ESMTP id DB4E27A01A3;\n\tWed, 27 Aug 2025 18:58:59 -0400 (EDT)","from phl-mailfrontend-02 ([10.202.2.163])\n\tby phl-compute-06.internal (MEProxy); Wed, 27 Aug 2025 18:59:00 -0400","by mail.messagingengine.com (Postfix) with ESMTPA; Wed,\n\t27 Aug 2025 18:58:59 -0400 (EDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ragnatech.se header.i=@ragnatech.se\n\theader.b=\"nZohlhSv\"; dkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"gpIafaWI\"; \n\tdkim-atps=neutral","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=ragnatech.se; h=\n\tcc:cc:content-transfer-encoding:content-type:content-type:date\n\t:date:from:from:in-reply-to:in-reply-to:message-id:mime-version\n\t:references:reply-to:subject:subject:to:to; s=fm3; t=1756335539;\n\tx=1756421939; bh=Byl8Ut9I0pCyWZsZngSAP5/ncrYLLdHA1WfFV90MT4I=; b=\n\tnZohlhSvW3x+1hsp3piTD6Uz+a5tT098pr+ACUKnwMVRx3n29Hzwcokk6qeF+ejb\n\thDLQhcci7Tl1eMlh74naTn0x2k9p5+lU7rnKYhq5x0k0WbVCn8LFasnlYr+Qhyes\n\tyS5Wll68WfIH+4KumZXPqJawpz0CX9FC4UglvvAv8xnNcSrUbFq1Iklbu7mCXfTY\n\tam7Dj4JcmQPUZpZh+SnLiOK8NnCIAPK8gDw9FWvZ2zs3ms35VnThO/0evZft3Ivl\n\tMlRui5n+9nYyw92qsAJSYa9yCqofThBrWkOMF6ETNg357rUDVG6LNC1sX5P2eKS7\n\tAy9qumvrfYsXHlwhF4I43Q==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:cc:content-transfer-encoding\n\t:content-type:content-type:date:date:feedback-id:feedback-id\n\t:from:from:in-reply-to:in-reply-to:message-id:mime-version\n\t:references:reply-to:subject:subject:to:to:x-me-proxy\n\t:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1756335539; x=\n\t1756421939; bh=Byl8Ut9I0pCyWZsZngSAP5/ncrYLLdHA1WfFV90MT4I=; b=g\n\tpIafaWI6SCfxlPRAsSAsaRrBxc0QxJ9M1Rlh++wLz1Adx7c7wZBwubns2Li8UfAz\n\tJTVJ4lfOMAmqiyZ4Zd4v0uWQLp8phy951l2l7tGtw4JjHbZ29qX2d70Infm5peD+\n\teFRYutcxeeqDG6M3pCEtwvGW/yBSGYqwmwOu5F5/WD3jT/YodolubzqTmebSXZgH\n\tO0cVHgJaFGLrBzRiVzEN5JDWp3AAlgTlb6/oJgmTkUO0XSS4mCzDvRZOqGO7LUR/\n\tyhcaejzV3j0/u+cTxM6Dk6XFFo15mI8KFf01cxGeInPSFSTLjoJ+W8zVsvvj7u8f\n\tlqi5eSxEkimih8mrCg4sQ=="],"X-ME-Sender":"\n\t","X-ME-Received":"","X-ME-Proxy-Cause":"gggruggvucftvghtrhhoucdtuddrgeeffedrtdefgddujeelgedtucetufdoteggodetrf\n\tdotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu\n\trghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujf\n\tgurhepfffhvfevuffkfhggtggugfgjsehtkeertddttdejnecuhfhrohhmpefpihhklhgr\n\tshcuufpnuggvrhhluhhnugcuoehnihhklhgrshdrshhouggvrhhluhhnugdorhgvnhgvsh\n\tgrshesrhgrghhnrghtvggthhdrshgvqeenucggtffrrghtthgvrhhnpeduteffgfduleej\n\theeuudduteetffeivddtkefghfeftdfgieevueetjefhfeehteenucffohhmrghinheplh\n\thisggtrghmvghrrgdrohhrghenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhep\n\tmhgrihhlfhhrohhmpehnihhklhgrshdrshhouggvrhhluhhnugdorhgvnhgvshgrshesrh\n\tgrghhnrghtvggthhdrshgvpdhnsggprhgtphhtthhopeegpdhmohguvgepshhmthhpohhu\n\tthdprhgtphhtthhopehkihgvrhgrnhdrsghinhhghhgrmhesihguvggrshhonhgsohgrrh\n\tgurdgtohhmpdhrtghpthhtohepphgruhhlrdgvlhguvghrsehiuggvrghsohhnsghorghr\n\tugdrtghomhdprhgtphhtthhopehlihgstggrmhgvrhgrqdguvghvvghlsehlihhsthhsrd\n\thlihgstggrmhgvrhgrrdhorhhgpdhrtghpthhtohepshhtvghfrghnrdhklhhughesihgu\n\tvggrshhonhgsohgrrhgurdgtohhm","X-ME-Proxy":"\n\t\n\t\n\t\n\t","Feedback-ID":"i80c9496c:Fastmail","Date":"Thu, 28 Aug 2025 00:58:56 +0200","From":"Niklas =?utf-8?q?S=C3=B6derlund?=\n\t","To":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","Subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","Message-ID":"<20250827225856.GA2901748@ragnatech.se>","References":"<20250827222032.644435-1-niklas.soderlund+renesas@ragnatech.se>\n\t<175633432565.694985.2542454723943054602@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<175633432565.694985.2542454723943054602@ping.linuxembedded.co.uk>","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":35598,"web_url":"https://patchwork.libcamera.org/comment/35598/","msgid":"<175633702034.694985.11867669616423405565@ping.linuxembedded.co.uk>","date":"2025-08-27T23:23:40","subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Niklas Söderlund (2025-08-27 23:58:56)\n> Hi Kieran,\n> \n> On 2025-08-27 23:38:45 +0100, Kieran Bingham wrote:\n> > Hi Niklas,\n> > \n> > Quoting Niklas Söderlund (2025-08-27 23:20:32)\n> > > The digital gain is already calculated in the AGC algorithm but the\n> > > value is not consumed by the AWB algorithm, which is where the gain\n> > > stage is located. Add the needed plumbing to carry the value between the\n> > > two algorithms.\n> > > \n> > > This is needed to get good images from sensors such as the IMX219 where\n> > > large sensor frames needs a small VBLANK value. This results in images\n> > \n> > Do you really mean vblank here? Or is it just that you're hitting a\n> > limitation on the exposure time or frame duration limit ? (and thus end\n> > up with a small vblank...?)\n> \n> Yes, they are all intertwined in my head ;-) The default FrameLimits \n> renders a VBLANK setting, this VBLANK setting limits the range of the \n> EXPOSURE. Even if the IPA max the EXPOSURE and ANALOG_GAIN the image is \n> very dark. From a .ko modules point of view the driving factor is the \n> VBLANK setting.\n> \n> > \n> > > that are so under exposed that even the sensors analog gain can't\n> > > compensate. The digital gain stage of the ISP helps with this.\n> > \n> > This is interesting - but I think would soon be superceeded by the\n> > capabilities we can use in the companding block or such with recent\n> > development from Stefan in his WDR series ...\n> \n> I can't tell if it will, or not.\n> \n> > \n> > See [0] and in particular [1] ....\n> > [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382\n> > [1] https://patchwork.libcamera.org/patch/24120/\n> > \n> > Using AWB to apply a gain here can be problematic because we can\n> > saturate a single channel\n> \n> I can set the gain on each channel separately, but I only get one value \n> from libipa. Maybe I'm misunderstanding you, or maybe this is why it \n> will be superseded by Stefan's work?\n> \n> > and I think the quantisation of the gain here\n> > is quite coarse?\n> \n> The digital gain I setting used by this patch comes from libipa at the \n> same time as the exposure and analog gain settings. It's just that the \n> rksip1 IPA have never used the digital gain setting returned.\n\nI mean the available values that can be applied through the AWB gains...\n\n> > But perhaps we'll end up with potentially multiple places we 'could'\n> > apply the digital gain ...\n> \n> I'm using a RPi Module 2 so I compered how the RPi IPA handles this, and \n> it applies DIGITAL_GAIN on the sensor. I have tried this and it works, \n> but I gather using the sensor for DIGITAL_GAIN is not the best idea \n\nWe haven't used digital gain on sensors yet - but perhaps that is indeed\nanother source that could be utilised.\n\n> these days. And I could not find any other block in the ISP where this \n> could be done.\n\nThe companding/compress block implemented by the WDR series has a\n'fine-grained/floating point' range gain that can be applied. I've\nalready been talking with Stefan to use this to similarly apply digital\ngain - so I'm just waiting for his series to land ;-)\n\n---\nKieran\n\n> \n> > \n> > Kieran\n> > \n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++--\n> > > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++-\n> > > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > > 3 files changed, 16 insertions(+), 3 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 35440b67e999..17aaa259244b 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > {\n> > > /* Configure the default exposure and gain. */\n> > > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain;\n> > > + context.activeState.agc.automatic.ispGain = 1.0;\n> > > context.activeState.agc.automatic.exposure =\n> > > 10ms / context.configuration.sensor.lineDuration;\n> > > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> > > + context.activeState.agc.manual.ispGain = 1.0;\n> > > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> > > context.activeState.agc.autoExposureEnabled = !context.configuration.raw;\n> > > context.activeState.agc.autoGainEnabled = !context.configuration.raw;\n> > > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context,\n> > > \n> > > if (!frameContext.agc.autoExposureEnabled)\n> > > frameContext.agc.exposure = agc.manual.exposure;\n> > > - if (!frameContext.agc.autoGainEnabled)\n> > > + if (!frameContext.agc.autoGainEnabled) {\n> > > frameContext.agc.gain = agc.manual.gain;\n> > > + frameContext.agc.ispGain = agc.manual.ispGain;\n> > > + }\n> > > \n> > > const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> > > if (meteringMode) {\n> > > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > {\n> > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;\n> > > double activeAutoGain = context.activeState.agc.automatic.gain;\n> > > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain;\n> > > \n> > > /* Populate exposure and gain in auto mode */\n> > > if (frameContext.agc.autoExposureEnabled)\n> > > frameContext.agc.exposure = activeAutoExposure;\n> > > - if (frameContext.agc.autoGainEnabled)\n> > > + if (frameContext.agc.autoGainEnabled) {\n> > > frameContext.agc.gain = activeAutoGain;\n> > > + frameContext.agc.ispGain = activeAutoIspGain;\n> > > + }\n> > > \n> > > /*\n> > > * Populate manual exposure and gain from the active auto values when\n> > > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > /* Update the estimated exposure and gain. */\n> > > activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n> > > activeState.agc.automatic.gain = aGain;\n> > > + activeState.agc.automatic.ispGain = dGain;\n> > > \n> > > /*\n> > > * Expand the target frame duration so that we do not run faster than\n> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > index 399fb51be414..569cac4a3466 100644\n> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,\n> > > */\n> > > if (frameContext.awb.autoEnabled) {\n> > > const auto &awb = context.activeState.awb;\n> > > - frameContext.awb.gains = awb.automatic.gains;\n> > > + const auto &agc = context.activeState.agc;\n> > > +\n> > > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain;\n> > > frameContext.awb.temperatureK = awb.automatic.temperatureK;\n> > > }\n> > > \n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index 7ccc7b501aff..7180b0b7dff2 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -73,10 +73,12 @@ struct IPAActiveState {\n> > > struct {\n> > > uint32_t exposure;\n> > > double gain;\n> > > + double ispGain;\n> > > } manual;\n> > > struct {\n> > > uint32_t exposure;\n> > > double gain;\n> > > + double ispGain;\n> > > } automatic;\n> > > \n> > > bool autoExposureEnabled;\n> > > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext {\n> > > struct {\n> > > uint32_t exposure;\n> > > double gain;\n> > > + double ispGain;\n> > > double exposureValue;\n> > > uint32_t vblank;\n> > > bool autoExposureEnabled;\n> > > -- \n> > > 2.51.0\n> > >\n> \n> -- \n> Kind Regards,\n> Niklas Söderlund","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 C3D5EBD87C\n\tfor ;\n\tWed, 27 Aug 2025 23:23:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8113C692EF;\n\tThu, 28 Aug 2025 01:23:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6488A613AF\n\tfor ;\n\tThu, 28 Aug 2025 01:23:45 +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 B900D19F6;\n\tThu, 28 Aug 2025 01:22:40 +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=\"qWw7/s7+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1756336960;\n\tbh=YywCOd9rP+gIBjxZGYdfpNTbmbXGfahsUATbMw+dAwY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=qWw7/s7+aTGD1+Vpb8uDYxf69fiy4nkIdjAB0uPJydvqMjsozKwPmSJmUFpM1QHki\n\t3x99QbaN4XgQ1ptE99reFPOTjeEmf+a2YYD9+iUilUR6X7KR51RKhTNq3FH3LDe490\n\tzeEvuT2AX9uupSXCwHXlVQMAVIG5FS+dOHOlSW3E=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250827225856.GA2901748@ragnatech.se>","References":"<20250827222032.644435-1-niklas.soderlund+renesas@ragnatech.se>\n\t<175633432565.694985.2542454723943054602@ping.linuxembedded.co.uk>\n\t<20250827225856.GA2901748@ragnatech.se>","Subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","From":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Date":"Thu, 28 Aug 2025 00:23:40 +0100","Message-ID":"<175633702034.694985.11867669616423405565@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":35602,"web_url":"https://patchwork.libcamera.org/comment/35602/","msgid":"","date":"2025-08-28T12:12:32","subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Niklas and Kieran,\n\nOn Wed, 27 Aug 2025 at 23:59, Niklas Söderlund\n wrote:\n>\n> Hi Kieran,\n>\n> On 2025-08-27 23:38:45 +0100, Kieran Bingham wrote:\n> > Hi Niklas,\n> >\n> > Quoting Niklas Söderlund (2025-08-27 23:20:32)\n> > > The digital gain is already calculated in the AGC algorithm but the\n> > > value is not consumed by the AWB algorithm, which is where the gain\n> > > stage is located. Add the needed plumbing to carry the value between the\n> > > two algorithms.\n> > >\n> > > This is needed to get good images from sensors such as the IMX219 where\n> > > large sensor frames needs a small VBLANK value. This results in images\n> >\n> > Do you really mean vblank here? Or is it just that you're hitting a\n> > limitation on the exposure time or frame duration limit ? (and thus end\n> > up with a small vblank...?)\n>\n> Yes, they are all intertwined in my head ;-) The default FrameLimits\n> renders a VBLANK setting, this VBLANK setting limits the range of the\n> EXPOSURE. Even if the IPA max the EXPOSURE and ANALOG_GAIN the image is\n> very dark. From a .ko modules point of view the driving factor is the\n> VBLANK setting.\n>\n> >\n> > > that are so under exposed that even the sensors analog gain can't\n> > > compensate. The digital gain stage of the ISP helps with this.\n> >\n> > This is interesting - but I think would soon be superceeded by the\n> > capabilities we can use in the companding block or such with recent\n> > development from Stefan in his WDR series ...\n>\n> I can't tell if it will, or not.\n>\n> >\n> > See [0] and in particular [1] ....\n> > [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382\n> > [1] https://patchwork.libcamera.org/patch/24120/\n> >\n> > Using AWB to apply a gain here can be problematic because we can\n> > saturate a single channel\n>\n> I can set the gain on each channel separately, but I only get one value\n> from libipa. Maybe I'm misunderstanding you, or maybe this is why it\n> will be superseded by Stefan's work?\n>\n> > and I think the quantisation of the gain here\n> > is quite coarse?\n>\n> The digital gain I setting used by this patch comes from libipa at the\n> same time as the exposure and analog gain settings. It's just that the\n> rksip1 IPA have never used the digital gain setting returned.\n>\n> >\n> > But perhaps we'll end up with potentially multiple places we 'could'\n> > apply the digital gain ...\n>\n> I'm using a RPi Module 2 so I compered how the RPi IPA handles this, and\n> it applies DIGITAL_GAIN on the sensor. I have tried this and it works,\n> but I gather using the sensor for DIGITAL_GAIN is not the best idea\n> these days. And I could not find any other block in the ISP where this\n> could be done.\n\nJust a quick correction, we don't apply digital gain on the sensor.\nInstead, we only apply digital gain (if needed) in the ISP pipeline.\n\nThat said, we don't have any issues with setting up a bright image\nwith a shutter speed/analogue gain combination on the IMX219. With\ndigital gain, we only aim to correct the line length and/or gain code\nquantization effects, so you would typically only add a few percent\ngain. I suspect you might be encountering another problem in the IPA\nperhaps that is limiting your shutter speed, likely based on VBLANK as\nyou mentioned earlier.\n\nNaush\n\n\n>\n> >\n> > Kieran\n> >\n> > >\n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++--\n> > > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++-\n> > > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > > 3 files changed, 16 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index 35440b67e999..17aaa259244b 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > {\n> > > /* Configure the default exposure and gain. */\n> > > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain;\n> > > + context.activeState.agc.automatic.ispGain = 1.0;\n> > > context.activeState.agc.automatic.exposure =\n> > > 10ms / context.configuration.sensor.lineDuration;\n> > > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> > > + context.activeState.agc.manual.ispGain = 1.0;\n> > > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> > > context.activeState.agc.autoExposureEnabled = !context.configuration.raw;\n> > > context.activeState.agc.autoGainEnabled = !context.configuration.raw;\n> > > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context,\n> > >\n> > > if (!frameContext.agc.autoExposureEnabled)\n> > > frameContext.agc.exposure = agc.manual.exposure;\n> > > - if (!frameContext.agc.autoGainEnabled)\n> > > + if (!frameContext.agc.autoGainEnabled) {\n> > > frameContext.agc.gain = agc.manual.gain;\n> > > + frameContext.agc.ispGain = agc.manual.ispGain;\n> > > + }\n> > >\n> > > const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> > > if (meteringMode) {\n> > > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > {\n> > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;\n> > > double activeAutoGain = context.activeState.agc.automatic.gain;\n> > > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain;\n> > >\n> > > /* Populate exposure and gain in auto mode */\n> > > if (frameContext.agc.autoExposureEnabled)\n> > > frameContext.agc.exposure = activeAutoExposure;\n> > > - if (frameContext.agc.autoGainEnabled)\n> > > + if (frameContext.agc.autoGainEnabled) {\n> > > frameContext.agc.gain = activeAutoGain;\n> > > + frameContext.agc.ispGain = activeAutoIspGain;\n> > > + }\n> > >\n> > > /*\n> > > * Populate manual exposure and gain from the active auto values when\n> > > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > /* Update the estimated exposure and gain. */\n> > > activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n> > > activeState.agc.automatic.gain = aGain;\n> > > + activeState.agc.automatic.ispGain = dGain;\n> > >\n> > > /*\n> > > * Expand the target frame duration so that we do not run faster than\n> > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > index 399fb51be414..569cac4a3466 100644\n> > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,\n> > > */\n> > > if (frameContext.awb.autoEnabled) {\n> > > const auto &awb = context.activeState.awb;\n> > > - frameContext.awb.gains = awb.automatic.gains;\n> > > + const auto &agc = context.activeState.agc;\n> > > +\n> > > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain;\n> > > frameContext.awb.temperatureK = awb.automatic.temperatureK;\n> > > }\n> > >\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index 7ccc7b501aff..7180b0b7dff2 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -73,10 +73,12 @@ struct IPAActiveState {\n> > > struct {\n> > > uint32_t exposure;\n> > > double gain;\n> > > + double ispGain;\n> > > } manual;\n> > > struct {\n> > > uint32_t exposure;\n> > > double gain;\n> > > + double ispGain;\n> > > } automatic;\n> > >\n> > > bool autoExposureEnabled;\n> > > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext {\n> > > struct {\n> > > uint32_t exposure;\n> > > double gain;\n> > > + double ispGain;\n> > > double exposureValue;\n> > > uint32_t vblank;\n> > > bool autoExposureEnabled;\n> > > --\n> > > 2.51.0\n> > >\n>\n> --\n> Kind Regards,\n> Niklas Söderlund","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 E0A8FBEFBE\n\tfor ;\n\tThu, 28 Aug 2025 12:13:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33782692F3;\n\tThu, 28 Aug 2025 14:13:10 +0200 (CEST)","from mail-vk1-xa32.google.com (mail-vk1-xa32.google.com\n\t[IPv6:2607:f8b0:4864:20::a32])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65AD0613B8\n\tfor ;\n\tThu, 28 Aug 2025 14:13:08 +0200 (CEST)","by mail-vk1-xa32.google.com with SMTP id\n\t71dfb90a1353d-53b2ae1bd57so41875e0c.0\n\tfor ;\n\tThu, 28 Aug 2025 05:13:08 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Xe61sao7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1756383187; x=1756987987;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=L0bXdmnn+qx4JB1sIQ2PMoXe+1n7Ff2slraqwu2lzL8=;\n\tb=Xe61sao7vVKdlyBFTHiUHAEQXJKpH9m2sXAm7ls/5GuO6IoMqVgNGiaTfhbJTL+XzM\n\teVzjB03qC8cewNGc/5RPFhsX5yldcF5Vq7w1h50klB1sQrxe63EtSCav+YPFpJe2Dn3r\n\tfNrcnbMn4WRsvHOAafyinAn67cW6Kdz4kWl/i0JOgAH0g89pmNZdtkECtJuVLc+LpvgI\n\tMMor1PbHlWxDSqs8D+PcFGWe7j0x3e9CaFz2zm4xuSiWI4IwGD4YEwB7XoHy4uNzrYky\n\t1n8E1ej2VXMxhB7FrKOdAN80zuD5UYn89cxvpbMBTfRecSQB+MBKCeaFVcX0JIcvrwiT\n\t8HqQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1756383187; x=1756987987;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=L0bXdmnn+qx4JB1sIQ2PMoXe+1n7Ff2slraqwu2lzL8=;\n\tb=rAonhlh6UiNYRxKX/NMfYbucUByG1bgjgca6x5OyW7nhyrZuwIBc9NZeqnDwIYG6ot\n\tuuv5pO5g1s34r01UOiXzO96Pia7Pja44Cc32j1NW9HAoBLLpd8WYEjruRh0LzwuvE8hx\n\tSJWCPYQIxHma6AFTMug/pHAQrYuVY7hLLRURdm3l0nV8kx/HqnwwmBLY237IWyGJo6Hp\n\tzBxFypT5aLgpI12scUGhu3qXTCDOC09Q6zHuFUpwyU1n5rwxNkLMd6f0lULADAg4Cc4q\n\tvcEwyD6nkPC2GYNjOVIBmZehEmlI0xssP2mtsGBwOV83pzgGxxm0UC93htkL5GRFeWYb\n\teMlg==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXrq4F/FP3XIR3yJ4qKQiprDDpFdSq7LMYIC9Slwv6wZiVyZLB4hVMfh7gs4cI50RoXg1/ZKXogKoqd0cP0VFY=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YzqoKUY17HC+snvklDekLLyHzoCDNDaUONgSVnh60RAfTBTHknL\n\tVJ7ZJmOKSCwab4WEl6Yj4YUv3HTCs42bhNixocQdmJwh5fd886UwDaOM9ydh869sxojgq3SZm27\n\tljg0fYBGZUhEk3BCRu49193pSG1/UkY5ssI3lBAHEww==","X-Gm-Gg":"ASbGnctWiVOrYdv9lMqGC3nF0SzmZR0qIrXHkdpdfzpw5AJHebqCIhkDxkunuiTuhR4\n\tFCBuWDO7RgzhRi53iOUgsrXefOnjpaMvmm/sKrVXZzjtZOJJhx3WtktEzsc4Q9HlIoNstjda8/d\n\taqFK78LBlmi3lVJKW3uuATU2e2EE1aJjy9S7/YRy4LR4EvwHH8UyPMQJ9B8zArbca/leGq+ZuIU\n\tiXzgRU3SYKw0Uqy2fIZnb41AJUZs07GTFYN9Mo=","X-Google-Smtp-Source":"AGHT+IGj3iLdPsNbBM0FSc2D13EDhTGTcIru5u8LHm3yn64BzJJ+3elPPSuPmoU+GIDYyPD99I3X4kepkN0/0bP0Kd0=","X-Received":"by 2002:a05:6122:6b8:b0:53f:f004:6b8c with SMTP id\n\t71dfb90a1353d-53ff004c2d7mr1627723e0c.3.1756383186953;\n\tThu, 28 Aug 2025 05:13:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20250827222032.644435-1-niklas.soderlund+renesas@ragnatech.se>\n\t<175633432565.694985.2542454723943054602@ping.linuxembedded.co.uk>\n\t<20250827225856.GA2901748@ragnatech.se>","In-Reply-To":"<20250827225856.GA2901748@ragnatech.se>","From":"Naushir Patuck ","Date":"Thu, 28 Aug 2025 13:12:32 +0100","X-Gm-Features":"Ac12FXztsslp-IjSAWlbx3oYNQOru51DxHaIONuA5mUazPj-88Dif5QnijRUDus","Message-ID":"","Subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Cc":"Kieran Bingham , \n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org, \n\tStefan Klug ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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":35604,"web_url":"https://patchwork.libcamera.org/comment/35604/","msgid":"<20250828135207.GB2901748@ragnatech.se>","date":"2025-08-28T13:52:07","subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","submitter":{"id":230,"url":"https://patchwork.libcamera.org/api/people/230/","name":"Niklas Söderlund","email":"niklas.soderlund+renesas@ragnatech.se"},"content":"Hi Naushir,\n\nThanks for the clarification.\n\nOn 2025-08-28 13:12:32 +0100, Naushir Patuck wrote:\n> Hi Niklas and Kieran,\n> \n> On Wed, 27 Aug 2025 at 23:59, Niklas Söderlund\n> wrote:\n> >\n> > Hi Kieran,\n> >\n> > On 2025-08-27 23:38:45 +0100, Kieran Bingham wrote:\n> > > Hi Niklas,\n> > >\n> > > Quoting Niklas Söderlund (2025-08-27 23:20:32)\n> > > > The digital gain is already calculated in the AGC algorithm but the\n> > > > value is not consumed by the AWB algorithm, which is where the gain\n> > > > stage is located. Add the needed plumbing to carry the value between the\n> > > > two algorithms.\n> > > >\n> > > > This is needed to get good images from sensors such as the IMX219 where\n> > > > large sensor frames needs a small VBLANK value. This results in images\n> > >\n> > > Do you really mean vblank here? Or is it just that you're hitting a\n> > > limitation on the exposure time or frame duration limit ? (and thus end\n> > > up with a small vblank...?)\n> >\n> > Yes, they are all intertwined in my head ;-) The default FrameLimits\n> > renders a VBLANK setting, this VBLANK setting limits the range of the\n> > EXPOSURE. Even if the IPA max the EXPOSURE and ANALOG_GAIN the image is\n> > very dark. From a .ko modules point of view the driving factor is the\n> > VBLANK setting.\n> >\n> > >\n> > > > that are so under exposed that even the sensors analog gain can't\n> > > > compensate. The digital gain stage of the ISP helps with this.\n> > >\n> > > This is interesting - but I think would soon be superceeded by the\n> > > capabilities we can use in the companding block or such with recent\n> > > development from Stefan in his WDR series ...\n> >\n> > I can't tell if it will, or not.\n> >\n> > >\n> > > See [0] and in particular [1] ....\n> > > [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382\n> > > [1] https://patchwork.libcamera.org/patch/24120/\n> > >\n> > > Using AWB to apply a gain here can be problematic because we can\n> > > saturate a single channel\n> >\n> > I can set the gain on each channel separately, but I only get one value\n> > from libipa. Maybe I'm misunderstanding you, or maybe this is why it\n> > will be superseded by Stefan's work?\n> >\n> > > and I think the quantisation of the gain here\n> > > is quite coarse?\n> >\n> > The digital gain I setting used by this patch comes from libipa at the\n> > same time as the exposure and analog gain settings. It's just that the\n> > rksip1 IPA have never used the digital gain setting returned.\n> >\n> > >\n> > > But perhaps we'll end up with potentially multiple places we 'could'\n> > > apply the digital gain ...\n> >\n> > I'm using a RPi Module 2 so I compered how the RPi IPA handles this, and\n> > it applies DIGITAL_GAIN on the sensor. I have tried this and it works,\n> > but I gather using the sensor for DIGITAL_GAIN is not the best idea\n> > these days. And I could not find any other block in the ISP where this\n> > could be done.\n> \n> Just a quick correction, we don't apply digital gain on the sensor.\n> Instead, we only apply digital gain (if needed) in the ISP pipeline.\n\nI was just investigating my issue and found that the RPI/VC4 IPA is the \nonly IPA that sets the V4L2_CID_DIGITAL_GAIN. I might have \nmisunderstood how that is used.\n\n> \n> That said, we don't have any issues with setting up a bright image\n> with a shutter speed/analogue gain combination on the IMX219. With\n> digital gain, we only aim to correct the line length and/or gain code\n> quantization effects, so you would typically only add a few percent\n> gain. I suspect you might be encountering another problem in the IPA\n> perhaps that is limiting your shutter speed, likely based on VBLANK as\n> you mentioned earlier.\n\nI think so too. After digging more in this I don't think using digital \ngain the way I do in this patch is a good way forward.\n\n> \n> Naush\n> \n> \n> >\n> > >\n> > > Kieran\n> > >\n> > > >\n> > > > Signed-off-by: Niklas Söderlund \n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++--\n> > > > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++-\n> > > > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > > > 3 files changed, 16 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > index 35440b67e999..17aaa259244b 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > > {\n> > > > /* Configure the default exposure and gain. */\n> > > > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain;\n> > > > + context.activeState.agc.automatic.ispGain = 1.0;\n> > > > context.activeState.agc.automatic.exposure =\n> > > > 10ms / context.configuration.sensor.lineDuration;\n> > > > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> > > > + context.activeState.agc.manual.ispGain = 1.0;\n> > > > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> > > > context.activeState.agc.autoExposureEnabled = !context.configuration.raw;\n> > > > context.activeState.agc.autoGainEnabled = !context.configuration.raw;\n> > > > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context,\n> > > >\n> > > > if (!frameContext.agc.autoExposureEnabled)\n> > > > frameContext.agc.exposure = agc.manual.exposure;\n> > > > - if (!frameContext.agc.autoGainEnabled)\n> > > > + if (!frameContext.agc.autoGainEnabled) {\n> > > > frameContext.agc.gain = agc.manual.gain;\n> > > > + frameContext.agc.ispGain = agc.manual.ispGain;\n> > > > + }\n> > > >\n> > > > const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> > > > if (meteringMode) {\n> > > > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > > {\n> > > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;\n> > > > double activeAutoGain = context.activeState.agc.automatic.gain;\n> > > > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain;\n> > > >\n> > > > /* Populate exposure and gain in auto mode */\n> > > > if (frameContext.agc.autoExposureEnabled)\n> > > > frameContext.agc.exposure = activeAutoExposure;\n> > > > - if (frameContext.agc.autoGainEnabled)\n> > > > + if (frameContext.agc.autoGainEnabled) {\n> > > > frameContext.agc.gain = activeAutoGain;\n> > > > + frameContext.agc.ispGain = activeAutoIspGain;\n> > > > + }\n> > > >\n> > > > /*\n> > > > * Populate manual exposure and gain from the active auto values when\n> > > > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > /* Update the estimated exposure and gain. */\n> > > > activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n> > > > activeState.agc.automatic.gain = aGain;\n> > > > + activeState.agc.automatic.ispGain = dGain;\n> > > >\n> > > > /*\n> > > > * Expand the target frame duration so that we do not run faster than\n> > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > index 399fb51be414..569cac4a3466 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,\n> > > > */\n> > > > if (frameContext.awb.autoEnabled) {\n> > > > const auto &awb = context.activeState.awb;\n> > > > - frameContext.awb.gains = awb.automatic.gains;\n> > > > + const auto &agc = context.activeState.agc;\n> > > > +\n> > > > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain;\n> > > > frameContext.awb.temperatureK = awb.automatic.temperatureK;\n> > > > }\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index 7ccc7b501aff..7180b0b7dff2 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -73,10 +73,12 @@ struct IPAActiveState {\n> > > > struct {\n> > > > uint32_t exposure;\n> > > > double gain;\n> > > > + double ispGain;\n> > > > } manual;\n> > > > struct {\n> > > > uint32_t exposure;\n> > > > double gain;\n> > > > + double ispGain;\n> > > > } automatic;\n> > > >\n> > > > bool autoExposureEnabled;\n> > > > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext {\n> > > > struct {\n> > > > uint32_t exposure;\n> > > > double gain;\n> > > > + double ispGain;\n> > > > double exposureValue;\n> > > > uint32_t vblank;\n> > > > bool autoExposureEnabled;\n> > > > --\n> > > > 2.51.0\n> > > >\n> >\n> > --\n> > Kind Regards,\n> > Niklas Söderlund","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 2722DBD87C\n\tfor ;\n\tThu, 28 Aug 2025 13:52:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B581692F3;\n\tThu, 28 Aug 2025 15:52:15 +0200 (CEST)","from fout-b7-smtp.messagingengine.com\n\t(fout-b7-smtp.messagingengine.com [202.12.124.150])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 58373613B8\n\tfor ;\n\tThu, 28 Aug 2025 15:52:12 +0200 (CEST)","from phl-compute-03.internal (phl-compute-03.internal\n\t[10.202.2.43])\n\tby mailfout.stl.internal (Postfix) with ESMTP id D82431D001EA;\n\tThu, 28 Aug 2025 09:52:10 -0400 (EDT)","from phl-mailfrontend-02 ([10.202.2.163])\n\tby phl-compute-03.internal (MEProxy); Thu, 28 Aug 2025 09:52:11 -0400","by mail.messagingengine.com (Postfix) with ESMTPA; Thu,\n\t28 Aug 2025 09:52:09 -0400 (EDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ragnatech.se header.i=@ragnatech.se\n\theader.b=\"OG6zGaWC\"; dkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"cB8rYqY6\"; \n\tdkim-atps=neutral","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=ragnatech.se; h=\n\tcc:cc:content-transfer-encoding:content-type:content-type:date\n\t:date:from:from:in-reply-to:in-reply-to:message-id:mime-version\n\t:references:reply-to:subject:subject:to:to; s=fm3; t=1756389130;\n\tx=1756475530; bh=V9DbLcMO+72ugupZvcT3MC8/PIqdkbW3k4rIixHf1rY=; b=\n\tOG6zGaWCScJgsK/G4oFC7Z2mG5vzKCGUKFHdsys6g88wRhkBx9u3vnTiM/IYpAmz\n\tPfkm/uGM5P5EwtNfKx4d0ENhNFznQflNbUa1taV74CiPxJU4QICcM0DWL6MxIYpa\n\tRDvegC70hUZqNH8+fkgcX6L9B52uxNTiYX0yvxeJ/3u7Ma77aWZNg0JkbwWCiwLZ\n\tTLRKG83tVKYkGgJid5RBVfS5K681b3gi7WFhnfl14shIDS1VjHypAPmWHpxMH5uL\n\tF9CiVjrxX7B9czRuU2OtXgcKED2LeYtRvjcEITcq/ulMOfUBmNImEL2jcUGNwC1e\n\tblikB0lFMWN5BPIfa0WGUQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:cc:content-transfer-encoding\n\t:content-type:content-type:date:date:feedback-id:feedback-id\n\t:from:from:in-reply-to:in-reply-to:message-id:mime-version\n\t:references:reply-to:subject:subject:to:to:x-me-proxy\n\t:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1756389130; x=\n\t1756475530; bh=V9DbLcMO+72ugupZvcT3MC8/PIqdkbW3k4rIixHf1rY=; b=c\n\tB8rYqY6owg8s2mZuLIja6MUf7ydMOn36KkhsOoTtXCkuGxJXNNGhATiBA+GDrKIL\n\tFNJcHsv2T6SqUE6ZvU4m+hTKwaYAZP9+7S1LZmb1vCnbT45vRiN6N/bnwdFy4+ia\n\tEFob3q8/oTBjN9+cL/m0oVWB5/cJSkC3dktg5p79aQtyazZaQIDXxUqW43LD/kwJ\n\tK+Br36el7M4LvNFqQxuWkXA4lX69aZLCm15g7LD3Cm7QBGHTfrxGUvF14UTRMkwy\n\t4hV41d87UKucE8eL1J3LZ5hSjwy+YRJD5KNFnsyHI8da/cXIITh4FOr1VZe8eHTN\n\txdpvwMKB4RXcPPAu4wEcA=="],"X-ME-Sender":"\n\t","X-ME-Received":"","X-ME-Proxy-Cause":"gggruggvucftvghtrhhoucdtuddrgeeffedrtdefgddukeduvddtucetufdoteggodetrf\n\tdotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu\n\trghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujf\n\tgurhepfffhvfevuffkfhggtggugfgjsehtkeertddttdejnecuhfhrohhmpefpihhklhgr\n\tshcuufpnuggvrhhluhhnugcuoehnihhklhgrshdrshhouggvrhhluhhnugdorhgvnhgvsh\n\tgrshesrhgrghhnrghtvggthhdrshgvqeenucggtffrrghtthgvrhhnpeduteffgfduleej\n\theeuudduteetffeivddtkefghfeftdfgieevueetjefhfeehteenucffohhmrghinheplh\n\thisggtrghmvghrrgdrohhrghenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhep\n\tmhgrihhlfhhrohhmpehnihhklhgrshdrshhouggvrhhluhhnugdorhgvnhgvshgrshesrh\n\tgrghhnrghtvggthhdrshgvpdhnsggprhgtphhtthhopeehpdhmohguvgepshhmthhpohhu\n\tthdprhgtphhtthhopehnrghushhhsehrrghsphgsvghrrhihphhirdgtohhmpdhrtghpth\n\thtohepkhhivghrrghnrdgsihhnghhhrghmsehiuggvrghsohhnsghorghrugdrtghomhdp\n\trhgtphhtthhopehprghulhdrvghluggvrhesihguvggrshhonhgsohgrrhgurdgtohhmpd\n\thrtghpthhtoheplhhisggtrghmvghrrgdquggvvhgvlheslhhishhtshdrlhhisggtrghm\n\tvghrrgdrohhrghdprhgtphhtthhopehsthgvfhgrnhdrkhhluhhgsehiuggvrghsohhnsg\n\thorghrugdrtghomh","X-ME-Proxy":"\n\t\n\t\n\t\n\t","Feedback-ID":"i80c9496c:Fastmail","Date":"Thu, 28 Aug 2025 15:52:07 +0200","From":"Niklas =?utf-8?q?S=C3=B6derlund?=\n\t","To":"Naushir Patuck ","Cc":"Kieran Bingham ,\n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","Subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","Message-ID":"<20250828135207.GB2901748@ragnatech.se>","References":"<20250827222032.644435-1-niklas.soderlund+renesas@ragnatech.se>\n\t<175633432565.694985.2542454723943054602@ping.linuxembedded.co.uk>\n\t<20250827225856.GA2901748@ragnatech.se>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":35605,"web_url":"https://patchwork.libcamera.org/comment/35605/","msgid":"<20250828135515.GC2901748@ragnatech.se>","date":"2025-08-28T13:55:15","subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","submitter":{"id":230,"url":"https://patchwork.libcamera.org/api/people/230/","name":"Niklas Söderlund","email":"niklas.soderlund+renesas@ragnatech.se"},"content":"Hi Kieran,\n\nOn 2025-08-28 00:23:40 +0100, Kieran Bingham wrote:\n> Quoting Niklas Söderlund (2025-08-27 23:58:56)\n> > Hi Kieran,\n> > \n> > On 2025-08-27 23:38:45 +0100, Kieran Bingham wrote:\n> > > Hi Niklas,\n> > > \n> > > Quoting Niklas Söderlund (2025-08-27 23:20:32)\n> > > > The digital gain is already calculated in the AGC algorithm but the\n> > > > value is not consumed by the AWB algorithm, which is where the gain\n> > > > stage is located. Add the needed plumbing to carry the value between the\n> > > > two algorithms.\n> > > > \n> > > > This is needed to get good images from sensors such as the IMX219 where\n> > > > large sensor frames needs a small VBLANK value. This results in images\n> > > \n> > > Do you really mean vblank here? Or is it just that you're hitting a\n> > > limitation on the exposure time or frame duration limit ? (and thus end\n> > > up with a small vblank...?)\n> > \n> > Yes, they are all intertwined in my head ;-) The default FrameLimits \n> > renders a VBLANK setting, this VBLANK setting limits the range of the \n> > EXPOSURE. Even if the IPA max the EXPOSURE and ANALOG_GAIN the image is \n> > very dark. From a .ko modules point of view the driving factor is the \n> > VBLANK setting.\n> > \n> > > \n> > > > that are so under exposed that even the sensors analog gain can't\n> > > > compensate. The digital gain stage of the ISP helps with this.\n> > > \n> > > This is interesting - but I think would soon be superceeded by the\n> > > capabilities we can use in the companding block or such with recent\n> > > development from Stefan in his WDR series ...\n> > \n> > I can't tell if it will, or not.\n> > \n> > > \n> > > See [0] and in particular [1] ....\n> > > [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382\n> > > [1] https://patchwork.libcamera.org/patch/24120/\n> > > \n> > > Using AWB to apply a gain here can be problematic because we can\n> > > saturate a single channel\n> > \n> > I can set the gain on each channel separately, but I only get one value \n> > from libipa. Maybe I'm misunderstanding you, or maybe this is why it \n> > will be superseded by Stefan's work?\n> > \n> > > and I think the quantisation of the gain here\n> > > is quite coarse?\n> > \n> > The digital gain I setting used by this patch comes from libipa at the \n> > same time as the exposure and analog gain settings. It's just that the \n> > rksip1 IPA have never used the digital gain setting returned.\n> \n> I mean the available values that can be applied through the AWB gains...\n> \n> > > But perhaps we'll end up with potentially multiple places we 'could'\n> > > apply the digital gain ...\n> > \n> > I'm using a RPi Module 2 so I compered how the RPi IPA handles this, and \n> > it applies DIGITAL_GAIN on the sensor. I have tried this and it works, \n> > but I gather using the sensor for DIGITAL_GAIN is not the best idea \n> \n> We haven't used digital gain on sensors yet - but perhaps that is indeed\n> another source that could be utilised.\n\nIt feels a bit dangerous to do so, not all sensors expose digital gain \ncontrols.\n\n> \n> > these days. And I could not find any other block in the ISP where this \n> > could be done.\n> \n> The companding/compress block implemented by the WDR series has a\n> 'fine-grained/floating point' range gain that can be applied. I've\n> already been talking with Stefan to use this to similarly apply digital\n> gain - so I'm just waiting for his series to land ;-)\n\nNot all rkisp1 variations have this block, the one in R-Car don't so it \nwould not help my use-case. That being said I dug some more in this and \nI think I found out what I can do differently without touching the \nrkisp1 IPA.\n\nI propose this patch be rejected.\n\n> \n> ---\n> Kieran\n> \n> > \n> > > \n> > > Kieran\n> > > \n> > > > \n> > > > Signed-off-by: Niklas Söderlund \n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++--\n> > > > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++-\n> > > > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > > > 3 files changed, 16 insertions(+), 3 deletions(-)\n> > > > \n> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > index 35440b67e999..17aaa259244b 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > > {\n> > > > /* Configure the default exposure and gain. */\n> > > > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain;\n> > > > + context.activeState.agc.automatic.ispGain = 1.0;\n> > > > context.activeState.agc.automatic.exposure =\n> > > > 10ms / context.configuration.sensor.lineDuration;\n> > > > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> > > > + context.activeState.agc.manual.ispGain = 1.0;\n> > > > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> > > > context.activeState.agc.autoExposureEnabled = !context.configuration.raw;\n> > > > context.activeState.agc.autoGainEnabled = !context.configuration.raw;\n> > > > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context,\n> > > > \n> > > > if (!frameContext.agc.autoExposureEnabled)\n> > > > frameContext.agc.exposure = agc.manual.exposure;\n> > > > - if (!frameContext.agc.autoGainEnabled)\n> > > > + if (!frameContext.agc.autoGainEnabled) {\n> > > > frameContext.agc.gain = agc.manual.gain;\n> > > > + frameContext.agc.ispGain = agc.manual.ispGain;\n> > > > + }\n> > > > \n> > > > const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> > > > if (meteringMode) {\n> > > > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > > {\n> > > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;\n> > > > double activeAutoGain = context.activeState.agc.automatic.gain;\n> > > > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain;\n> > > > \n> > > > /* Populate exposure and gain in auto mode */\n> > > > if (frameContext.agc.autoExposureEnabled)\n> > > > frameContext.agc.exposure = activeAutoExposure;\n> > > > - if (frameContext.agc.autoGainEnabled)\n> > > > + if (frameContext.agc.autoGainEnabled) {\n> > > > frameContext.agc.gain = activeAutoGain;\n> > > > + frameContext.agc.ispGain = activeAutoIspGain;\n> > > > + }\n> > > > \n> > > > /*\n> > > > * Populate manual exposure and gain from the active auto values when\n> > > > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > /* Update the estimated exposure and gain. */\n> > > > activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n> > > > activeState.agc.automatic.gain = aGain;\n> > > > + activeState.agc.automatic.ispGain = dGain;\n> > > > \n> > > > /*\n> > > > * Expand the target frame duration so that we do not run faster than\n> > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > index 399fb51be414..569cac4a3466 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,\n> > > > */\n> > > > if (frameContext.awb.autoEnabled) {\n> > > > const auto &awb = context.activeState.awb;\n> > > > - frameContext.awb.gains = awb.automatic.gains;\n> > > > + const auto &agc = context.activeState.agc;\n> > > > +\n> > > > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain;\n> > > > frameContext.awb.temperatureK = awb.automatic.temperatureK;\n> > > > }\n> > > > \n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index 7ccc7b501aff..7180b0b7dff2 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -73,10 +73,12 @@ struct IPAActiveState {\n> > > > struct {\n> > > > uint32_t exposure;\n> > > > double gain;\n> > > > + double ispGain;\n> > > > } manual;\n> > > > struct {\n> > > > uint32_t exposure;\n> > > > double gain;\n> > > > + double ispGain;\n> > > > } automatic;\n> > > > \n> > > > bool autoExposureEnabled;\n> > > > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext {\n> > > > struct {\n> > > > uint32_t exposure;\n> > > > double gain;\n> > > > + double ispGain;\n> > > > double exposureValue;\n> > > > uint32_t vblank;\n> > > > bool autoExposureEnabled;\n> > > > -- \n> > > > 2.51.0\n> > > >\n> > \n> > -- \n> > Kind Regards,\n> > Niklas Söderlund","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 537D3BD87C\n\tfor ;\n\tThu, 28 Aug 2025 13:55:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3B4F2692E1;\n\tThu, 28 Aug 2025 15:55:21 +0200 (CEST)","from fhigh-b2-smtp.messagingengine.com\n\t(fhigh-b2-smtp.messagingengine.com [202.12.124.153])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70125692E1\n\tfor ;\n\tThu, 28 Aug 2025 15:55:18 +0200 (CEST)","from phl-compute-12.internal (phl-compute-12.internal\n\t[10.202.2.52])\n\tby mailfhigh.stl.internal (Postfix) with ESMTP id 5016F7A0264;\n\tThu, 28 Aug 2025 09:55:17 -0400 (EDT)","from phl-mailfrontend-02 ([10.202.2.163])\n\tby phl-compute-12.internal (MEProxy); Thu, 28 Aug 2025 09:55:17 -0400","by mail.messagingengine.com (Postfix) with ESMTPA; Thu,\n\t28 Aug 2025 09:55:16 -0400 (EDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ragnatech.se header.i=@ragnatech.se\n\theader.b=\"gX2yf3i5\"; dkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"l7eWgr27\"; \n\tdkim-atps=neutral","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=ragnatech.se; h=\n\tcc:cc:content-transfer-encoding:content-type:content-type:date\n\t:date:from:from:in-reply-to:in-reply-to:message-id:mime-version\n\t:references:reply-to:subject:subject:to:to; s=fm3; t=1756389317;\n\tx=1756475717; bh=DPouHtvi34QbPtxh8Uvnt7kJubxd+1IUbmgeUnupwIE=; b=\n\tgX2yf3i5nbV/GzQxng6vx3DNBC50CHBitNnp3RCYIQsp96o4ZBiuOUxf+59FqW7q\n\tTL+xLbpRwYIWRs9aDbNPlw42O5P3tywhPuS45J6gnBbSlb6/X0el0q64zIB3hcMI\n\tUnSbaFrWdvVTaOgqltgg61LqVJMJyZ7uymBmAMfjoqQQpAu4V2b+NtdGyBM6VP/r\n\tOgoJ1rg3DLA+dpTzE9wMv+fDzSPwudO5bsVoyT4YddCN7DNcz9Ln8/uxBlYFRsoG\n\tC05zj9xZ8gocuyMMQYiSTZwGg+Pae9TZnK2egP3tg2r8JduZSrRP0R/a8WKnzheh\n\tO32gF91ZaOitgGaY2/Atgg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:cc:content-transfer-encoding\n\t:content-type:content-type:date:date:feedback-id:feedback-id\n\t:from:from:in-reply-to:in-reply-to:message-id:mime-version\n\t:references:reply-to:subject:subject:to:to:x-me-proxy\n\t:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; t=1756389317; x=\n\t1756475717; bh=DPouHtvi34QbPtxh8Uvnt7kJubxd+1IUbmgeUnupwIE=; b=l\n\t7eWgr27h4DywvTy4gyxypiz22UwMSgMNHJyNRKd9KEyJeQ3CSpF3ION7nvXPTidk\n\tJm1Ot1MuYlSjK5Oo4vRq8Oym49+ArRgMvppkfk/uUFmcMVEH1W0r+op7Xrpi7sac\n\tGmwtITs4h1wnJAUewPaUl0pywsERvEbhYhm4smojw+3DssASkPHlJom7f9o1R7Hp\n\tSw4BaPxoqbBxEgITqpvzNBMgexKO0Soikkn3wcgsEC+7VkgRsSF8wbcUJJpXnv03\n\t9ltZXgLgB5/cv5kpx/bu67tCeaohltXOc1Fn6kyIMYA5yV4XeNE+vw+quA1B9RAf\n\tUCnLBSqZBNf04UQkFQZ7Q=="],"X-ME-Sender":"\n\t","X-ME-Received":"","X-ME-Proxy-Cause":"gggruggvucftvghtrhhoucdtuddrgeeffedrtdefgddukeduvdduucetufdoteggodetrf\n\tdotffvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfurfetoffkrfgpnffqhgenuceu\n\trghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujf\n\tgurhepfffhvfevuffkfhggtggugfgjsehtkeertddttdejnecuhfhrohhmpefpihhklhgr\n\tshcuufpnuggvrhhluhhnugcuoehnihhklhgrshdrshhouggvrhhluhhnugdorhgvnhgvsh\n\tgrshesrhgrghhnrghtvggthhdrshgvqeenucggtffrrghtthgvrhhnpeduteffgfduleej\n\theeuudduteetffeivddtkefghfeftdfgieevueetjefhfeehteenucffohhmrghinheplh\n\thisggtrghmvghrrgdrohhrghenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhep\n\tmhgrihhlfhhrohhmpehnihhklhgrshdrshhouggvrhhluhhnugdorhgvnhgvshgrshesrh\n\tgrghhnrghtvggthhdrshgvpdhnsggprhgtphhtthhopeegpdhmohguvgepshhmthhpohhu\n\tthdprhgtphhtthhopehkihgvrhgrnhdrsghinhhghhgrmhesihguvggrshhonhgsohgrrh\n\tgurdgtohhmpdhrtghpthhtohepphgruhhlrdgvlhguvghrsehiuggvrghsohhnsghorghr\n\tugdrtghomhdprhgtphhtthhopehlihgstggrmhgvrhgrqdguvghvvghlsehlihhsthhsrd\n\thlihgstggrmhgvrhgrrdhorhhgpdhrtghpthhtohepshhtvghfrghnrdhklhhughesihgu\n\tvggrshhonhgsohgrrhgurdgtohhm","X-ME-Proxy":"\n\t\n\t\n\t\n\t","Feedback-ID":"i80c9496c:Fastmail","Date":"Thu, 28 Aug 2025 15:55:15 +0200","From":"Niklas =?utf-8?q?S=C3=B6derlund?=\n\t","To":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","Subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","Message-ID":"<20250828135515.GC2901748@ragnatech.se>","References":"<20250827222032.644435-1-niklas.soderlund+renesas@ragnatech.se>\n\t<175633432565.694985.2542454723943054602@ping.linuxembedded.co.uk>\n\t<20250827225856.GA2901748@ragnatech.se>\n\t<175633702034.694985.11867669616423405565@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<175633702034.694985.11867669616423405565@ping.linuxembedded.co.uk>","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":35606,"web_url":"https://patchwork.libcamera.org/comment/35606/","msgid":"","date":"2025-08-28T13:55:37","subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Niklas,\n\nOn Thu, 28 Aug 2025 at 14:52, Niklas Söderlund\n wrote:\n>\n> Hi Naushir,\n>\n> Thanks for the clarification.\n>\n> On 2025-08-28 13:12:32 +0100, Naushir Patuck wrote:\n> > Hi Niklas and Kieran,\n> >\n> > On Wed, 27 Aug 2025 at 23:59, Niklas Söderlund\n> > wrote:\n> > >\n> > > Hi Kieran,\n> > >\n> > > On 2025-08-27 23:38:45 +0100, Kieran Bingham wrote:\n> > > > Hi Niklas,\n> > > >\n> > > > Quoting Niklas Söderlund (2025-08-27 23:20:32)\n> > > > > The digital gain is already calculated in the AGC algorithm but the\n> > > > > value is not consumed by the AWB algorithm, which is where the gain\n> > > > > stage is located. Add the needed plumbing to carry the value between the\n> > > > > two algorithms.\n> > > > >\n> > > > > This is needed to get good images from sensors such as the IMX219 where\n> > > > > large sensor frames needs a small VBLANK value. This results in images\n> > > >\n> > > > Do you really mean vblank here? Or is it just that you're hitting a\n> > > > limitation on the exposure time or frame duration limit ? (and thus end\n> > > > up with a small vblank...?)\n> > >\n> > > Yes, they are all intertwined in my head ;-) The default FrameLimits\n> > > renders a VBLANK setting, this VBLANK setting limits the range of the\n> > > EXPOSURE. Even if the IPA max the EXPOSURE and ANALOG_GAIN the image is\n> > > very dark. From a .ko modules point of view the driving factor is the\n> > > VBLANK setting.\n> > >\n> > > >\n> > > > > that are so under exposed that even the sensors analog gain can't\n> > > > > compensate. The digital gain stage of the ISP helps with this.\n> > > >\n> > > > This is interesting - but I think would soon be superceeded by the\n> > > > capabilities we can use in the companding block or such with recent\n> > > > development from Stefan in his WDR series ...\n> > >\n> > > I can't tell if it will, or not.\n> > >\n> > > >\n> > > > See [0] and in particular [1] ....\n> > > > [0] https://patchwork.libcamera.org/project/libcamera/list/?series=5382\n> > > > [1] https://patchwork.libcamera.org/patch/24120/\n> > > >\n> > > > Using AWB to apply a gain here can be problematic because we can\n> > > > saturate a single channel\n> > >\n> > > I can set the gain on each channel separately, but I only get one value\n> > > from libipa. Maybe I'm misunderstanding you, or maybe this is why it\n> > > will be superseded by Stefan's work?\n> > >\n> > > > and I think the quantisation of the gain here\n> > > > is quite coarse?\n> > >\n> > > The digital gain I setting used by this patch comes from libipa at the\n> > > same time as the exposure and analog gain settings. It's just that the\n> > > rksip1 IPA have never used the digital gain setting returned.\n> > >\n> > > >\n> > > > But perhaps we'll end up with potentially multiple places we 'could'\n> > > > apply the digital gain ...\n> > >\n> > > I'm using a RPi Module 2 so I compered how the RPi IPA handles this, and\n> > > it applies DIGITAL_GAIN on the sensor. I have tried this and it works,\n> > > but I gather using the sensor for DIGITAL_GAIN is not the best idea\n> > > these days. And I could not find any other block in the ISP where this\n> > > could be done.\n> >\n> > Just a quick correction, we don't apply digital gain on the sensor.\n> > Instead, we only apply digital gain (if needed) in the ISP pipeline.\n>\n> I was just investigating my issue and found that the RPI/VC4 IPA is the\n> only IPA that sets the V4L2_CID_DIGITAL_GAIN. I might have\n> misunderstood how that is used.\n\nAh, I understand now. Use of V4L2_CID_DIGITAL_GAIN in ipa/vc4.cpp is\nindeed used to set up the digital gain, but is directed to the ISP\ndriver, not the sensor subdev. This is probably where the confusion\ncomes from.\n\nRegards,\nNaush\n\n>\n> >\n> > That said, we don't have any issues with setting up a bright image\n> > with a shutter speed/analogue gain combination on the IMX219. With\n> > digital gain, we only aim to correct the line length and/or gain code\n> > quantization effects, so you would typically only add a few percent\n> > gain. I suspect you might be encountering another problem in the IPA\n> > perhaps that is limiting your shutter speed, likely based on VBLANK as\n> > you mentioned earlier.\n>\n> I think so too. After digging more in this I don't think using digital\n> gain the way I do in this patch is a good way forward.\n>\n> >\n> > Naush\n> >\n> >\n> > >\n> > > >\n> > > > Kieran\n> > > >\n> > > > >\n> > > > > Signed-off-by: Niklas Söderlund \n> > > > > ---\n> > > > > src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++++++--\n> > > > > src/ipa/rkisp1/algorithms/awb.cpp | 4 +++-\n> > > > > src/ipa/rkisp1/ipa_context.h | 3 +++\n> > > > > 3 files changed, 16 insertions(+), 3 deletions(-)\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > index 35440b67e999..17aaa259244b 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > > > @@ -174,9 +174,11 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > > > {\n> > > > > /* Configure the default exposure and gain. */\n> > > > > context.activeState.agc.automatic.gain = context.configuration.sensor.minAnalogueGain;\n> > > > > + context.activeState.agc.automatic.ispGain = 1.0;\n> > > > > context.activeState.agc.automatic.exposure =\n> > > > > 10ms / context.configuration.sensor.lineDuration;\n> > > > > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> > > > > + context.activeState.agc.manual.ispGain = 1.0;\n> > > > > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> > > > > context.activeState.agc.autoExposureEnabled = !context.configuration.raw;\n> > > > > context.activeState.agc.autoGainEnabled = !context.configuration.raw;\n> > > > > @@ -280,8 +282,10 @@ void Agc::queueRequest(IPAContext &context,\n> > > > >\n> > > > > if (!frameContext.agc.autoExposureEnabled)\n> > > > > frameContext.agc.exposure = agc.manual.exposure;\n> > > > > - if (!frameContext.agc.autoGainEnabled)\n> > > > > + if (!frameContext.agc.autoGainEnabled) {\n> > > > > frameContext.agc.gain = agc.manual.gain;\n> > > > > + frameContext.agc.ispGain = agc.manual.ispGain;\n> > > > > + }\n> > > > >\n> > > > > const auto &meteringMode = controls.get(controls::AeMeteringMode);\n> > > > > if (meteringMode) {\n> > > > > @@ -336,12 +340,15 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > > > > {\n> > > > > uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;\n> > > > > double activeAutoGain = context.activeState.agc.automatic.gain;\n> > > > > + double activeAutoIspGain = context.activeState.agc.automatic.ispGain;\n> > > > >\n> > > > > /* Populate exposure and gain in auto mode */\n> > > > > if (frameContext.agc.autoExposureEnabled)\n> > > > > frameContext.agc.exposure = activeAutoExposure;\n> > > > > - if (frameContext.agc.autoGainEnabled)\n> > > > > + if (frameContext.agc.autoGainEnabled) {\n> > > > > frameContext.agc.gain = activeAutoGain;\n> > > > > + frameContext.agc.ispGain = activeAutoIspGain;\n> > > > > + }\n> > > > >\n> > > > > /*\n> > > > > * Populate manual exposure and gain from the active auto values when\n> > > > > @@ -581,6 +588,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > /* Update the estimated exposure and gain. */\n> > > > > activeState.agc.automatic.exposure = newExposureTime / lineDuration;\n> > > > > activeState.agc.automatic.gain = aGain;\n> > > > > + activeState.agc.automatic.ispGain = dGain;\n> > > > >\n> > > > > /*\n> > > > > * Expand the target frame duration so that we do not run faster than\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > > index 399fb51be414..569cac4a3466 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > > +++ b/src/ipa/rkisp1/algorithms/awb.cpp\n> > > > > @@ -218,7 +218,9 @@ void Awb::prepare(IPAContext &context, const uint32_t frame,\n> > > > > */\n> > > > > if (frameContext.awb.autoEnabled) {\n> > > > > const auto &awb = context.activeState.awb;\n> > > > > - frameContext.awb.gains = awb.automatic.gains;\n> > > > > + const auto &agc = context.activeState.agc;\n> > > > > +\n> > > > > + frameContext.awb.gains = awb.automatic.gains * agc.automatic.ispGain;\n> > > > > frameContext.awb.temperatureK = awb.automatic.temperatureK;\n> > > > > }\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > index 7ccc7b501aff..7180b0b7dff2 100644\n> > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > @@ -73,10 +73,12 @@ struct IPAActiveState {\n> > > > > struct {\n> > > > > uint32_t exposure;\n> > > > > double gain;\n> > > > > + double ispGain;\n> > > > > } manual;\n> > > > > struct {\n> > > > > uint32_t exposure;\n> > > > > double gain;\n> > > > > + double ispGain;\n> > > > > } automatic;\n> > > > >\n> > > > > bool autoExposureEnabled;\n> > > > > @@ -130,6 +132,7 @@ struct IPAFrameContext : public FrameContext {\n> > > > > struct {\n> > > > > uint32_t exposure;\n> > > > > double gain;\n> > > > > + double ispGain;\n> > > > > double exposureValue;\n> > > > > uint32_t vblank;\n> > > > > bool autoExposureEnabled;\n> > > > > --\n> > > > > 2.51.0\n> > > > >\n> > >\n> > > --\n> > > Kind Regards,\n> > > Niklas Söderlund\n>\n> --\n> Kind Regards,\n> Niklas Söderlund","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 870B0BD87C\n\tfor ;\n\tThu, 28 Aug 2025 13:56:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 25937692FB;\n\tThu, 28 Aug 2025 15:56:16 +0200 (CEST)","from mail-ua1-x92b.google.com (mail-ua1-x92b.google.com\n\t[IPv6:2607:f8b0:4864:20::92b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA46F692E1\n\tfor ;\n\tThu, 28 Aug 2025 15:56:13 +0200 (CEST)","by mail-ua1-x92b.google.com with SMTP id\n\ta1e0cc1a2514c-89436f9dea6so1933241.3\n\tfor ;\n\tThu, 28 Aug 2025 06:56:13 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"LtyKpwHA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1756389373; x=1756994173;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=6/ytUkGlULdJSE/MB/B34WzAib0dAKMeUO2d5NV8W28=;\n\tb=LtyKpwHAtFhH9VQ4YHUH8o+UoK6Ib7vKs7n+bxpKzmGw08Hz8YEznazvg7CppGhrW1\n\tuDT5KF2xlo4E0FLCmzB86zVUYzV+IuS43sb/Tt2GaXln9Gq+gXRL3lY/iCLhIRWI9ZRT\n\tlYLtRdexLTxJZy4ggbv+k+ySRgHqe+a4tsxYqFJpUQ5Kx0MjjTBiZ85BgznAjPCYSuiC\n\t8FBgS3AAEwwcvPGso6PsraZ10v3hlDyFvnU3D7hc2YgodgBjuCoQ3i4OoB/cVpgiNo88\n\tbmuoguad4Fk6RToIuZewJuL0aSebwspsMC6gg3tmELhARj+IPD9mYY59pMmVOql7PWrc\n\toIGg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1756389373; x=1756994173;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=6/ytUkGlULdJSE/MB/B34WzAib0dAKMeUO2d5NV8W28=;\n\tb=IIgmxbTS3ubB/MgCunm7nwwTIylvWS1nawGl8kN6kjeFXHEkH+f9sAARdInjjpRTsy\n\t1xYwAtOEcM1hcbuttAIIDBxF8l+EfxlQ7az79H0JqcPFYftcg9wUSPBdalD4sF0Ja4Kg\n\tfvsMH27USGa+jgwj8ujsA7ORARhidrfIhgcSZ/g/tGz6AEVOQDO1CrcqxmODVIG4iXcc\n\tM8dJX6bXotnGqiFhZs91On/V/adhCZQpG8cEKEmovTq/Mzg65K6LhU5NAFZqygx7K016\n\tfrrYiucU5SUK4DZ9MZh3MMG4m+5ivBWu5iw2REJpFMJqr/ONQNbkfGSNgeSVInJKwS4r\n\tPI2Q==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCXoJiSNo8w1UnqcilGi674xEygNHGKwZGjZC7s7ajZ/fVb0L3MLjVtnSbWOzlUGnV9hIFO68YohTaK6fpfCbi8=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yy1AIiqMr7m0fs0G2dvm2KvA11IXbBiG5njYrsYX0SFkiOgfZsk\n\tNPFrbLuH565OTmJ8lMH5tBwQir/upD8oneVMIqi2pNCnIi/7av3VcfPlVsP0DWyxX5yeOpWJdUK\n\tUZEPl03pGsxkZay+cDvWPcRN1jQyn/KuSUgFJUZg/GUnjFNNhhE4i9eE=","X-Gm-Gg":"ASbGncsZgvD1asZONEdNHhGkpGpOiXPCUjxL+wYVRmxCIe/biT45OPofGAPpBXLS53B\n\tBp8e4ZKkmT55F4QoJ8B+tdZapBp4Gdx2uKhgrOJW0lL0UKpr+PSRj9fI0/bp2u2MipQ7yAoT2NI\n\t64jo4srPIkNkX1RaSnKCe0r6ciAYx9WYP7cjaqLN31Fg796R1AVThwQ366uzLIDls/QxpEJL0Om\n\tJzmzNFWhdTLIxuH3fplkj11T73sjAyLFsXsQnhXKmYd37hJ2w==","X-Google-Smtp-Source":"AGHT+IGiXA3x0pUm5UGY98JfW6RYDG0Zv9pq0ASgK16a1vyHhU3MLahsmwSAR9lwupudPFWqERuY2ouNFNurE61aVQ0=","X-Received":"by 2002:a05:6102:2c08:b0:4f4:aff8:18d with SMTP id\n\tada2fe7eead31-51d0f501196mr2845301137.7.1756389372510;\n\tThu, 28 Aug 2025 06:56:12 -0700 (PDT)","MIME-Version":"1.0","References":"<20250827222032.644435-1-niklas.soderlund+renesas@ragnatech.se>\n\t<175633432565.694985.2542454723943054602@ping.linuxembedded.co.uk>\n\t<20250827225856.GA2901748@ragnatech.se>\n\t\n\t<20250828135207.GB2901748@ragnatech.se>","In-Reply-To":"<20250828135207.GB2901748@ragnatech.se>","From":"Naushir Patuck ","Date":"Thu, 28 Aug 2025 14:55:37 +0100","X-Gm-Features":"Ac12FXx60dhX3mpnKCukzLWXa2yL01jQOxb0gqWB9IzwDresiYBJCPnNO1WILew","Message-ID":"","Subject":"Re: [PATCH] ipa: rkisp1: agc: awb: Apply digital gain using ISP","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Cc":"Kieran Bingham , \n\tPaul Elder ,\n\tlibcamera-devel@lists.libcamera.org, \n\tStefan Klug ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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\" "}}]