[{"id":32174,"web_url":"https://patchwork.libcamera.org/comment/32174/","msgid":"<7vxzvp2vjmmjilosnbclkls6kctrxmb4hn2ckbh4oy3btmicuq@fmtx7oymmnda>","date":"2024-11-14T15:20:40","subject":"Re: [PATCH v3 6/8] ipa: rkisp1: Port to the new AEGC controls","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch. \n\nOn Wed, Nov 13, 2024 at 10:12:54PM +0900, Paul Elder wrote:\n> The newly introduced controls to drive the AEGC algorithm allow\n> controlling the computation of the exposure time and analogue gain\n> separately.\n> \n> Augument the RkISP1 AEGC implementation to handle the exposure and gain\n> controls separately using the new controls.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> New in v3\n> \n> Back 2 years ago in v2 RkISP1 didn't yet support AeEnable properly yet\n> so the AeEnable control was simply removed.\n> ---\n> src/ipa/rkisp1/algorithms/agc.cpp | 68 ++++++++++++++++++++++++-------\n> src/ipa/rkisp1/ipa_context.cpp | 14 +++++--\n> src/ipa/rkisp1/ipa_context.h | 6 ++-\n> 3 files changed, 67 insertions(+), 21 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 301b7ec26508..0ad206621516 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -148,7 +148,12 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> \tif (ret)\n> \t\treturn ret;\n> \n> -\tcontext.ctrlMap[&controls::AeEnable] = ControlInfo(false, true);\n> +\tcontext.ctrlMap[&controls::ExposureTimeMode] =\n> +\t\tControlInfo(static_cast(controls::ExposureTimeModeAuto),\n> +\t\t\t static_cast(controls::ExposureTimeModeManual));\n> +\tcontext.ctrlMap[&controls::AnalogueGainMode] =\n> +\t\tControlInfo(static_cast(controls::AnalogueGainModeAuto),\n> +\t\t\t static_cast(controls::AnalogueGainModeManual));\n\nI believe these should also have default values.\n\n> \tcontext.ctrlMap.merge(controls());\n> \n> \treturn 0;\n> @@ -169,7 +174,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> \t\t10ms / context.configuration.sensor.lineDuration;\n> \tcontext.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> \tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> -\tcontext.activeState.agc.autoEnabled = !context.configuration.raw;\n> +\tcontext.activeState.agc.autoExposureEnabled = !context.configuration.raw;\n> +\tcontext.activeState.agc.autoGainEnabled = !context.configuration.raw;\n> \n\nAhh this actually sets the defaults internally, but the defaults above\nare the ones seen by applications like camshark.\n\n> \tcontext.activeState.agc.constraintMode =\n> \t\tstatic_cast(constraintModes().begin()->first);\n> @@ -215,18 +221,42 @@ void Agc::queueRequest(IPAContext &context,\n> \tauto &agc = context.activeState.agc;\n> \n> \tif (!context.configuration.raw) {\n> -\t\tconst auto &agcEnable = controls.get(controls::AeEnable);\n> -\t\tif (agcEnable && *agcEnable != agc.autoEnabled) {\n> -\t\t\tagc.autoEnabled = *agcEnable;\n> +\t\tconst auto &aeEnable = controls.get(controls::ExposureTimeMode);\n> +\t\tif (aeEnable &&\n> +\t\t (*aeEnable == controls::ExposureTimeModeAuto) != agc.autoExposureEnabled) {\n> +\t\t\tagc.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);\n> \n> \t\t\tLOG(RkISP1Agc, Debug)\n> -\t\t\t\t<< (agc.autoEnabled ? \"Enabling\" : \"Disabling\")\n> -\t\t\t\t<< \" AGC\";\n> +\t\t\t\t<< (agc.autoExposureEnabled ? \"Enabling\" : \"Disabling\")\n> +\t\t\t\t<< \" AGC (exposure)\";\n> +\n> +\t\t\t/*\n> +\t\t\t * If we go from auto -> manual with no manual control\n> +\t\t\t * set, use the last computed value\n> +\t\t\t */\n> +\t\t\tif (!agc.autoExposureEnabled && !controls.get(controls::ExposureTime))\n> +\t\t\t\tframeContext.agc.exposure = agc.automatic.exposure;\n\nDoes this work? It sets the exposure time of the future frame (that is\ncurrently beeing queued) to the current value from active state. But\nthat value will change depending on how many frames are queued already.\nI guess we need to set a flag and fetch the exposure time from\nactiveState when this frame gets processed.\n\n> +\t\t}\n> +\n> +\t\tconst auto &agEnable = controls.get(controls::AnalogueGainMode);\n> +\t\tif (agEnable &&\n> +\t\t (*agEnable == controls::AnalogueGainModeAuto) != agc.autoGainEnabled) {\n> +\t\t\tagc.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);\n> +\n> +\t\t\tLOG(RkISP1Agc, Debug)\n> +\t\t\t\t<< (agc.autoGainEnabled ? \"Enabling\" : \"Disabling\")\n> +\t\t\t\t<< \" AGC (gain)\";\n> +\t\t\t/*\n> +\t\t\t * If we go from auto -> manual with no manual control\n> +\t\t\t * set, use the last computed value\n> +\t\t\t */\n> +\t\t\tif (!agc.autoGainEnabled && !controls.get(controls::AnalogueGain))\n> +\t\t\t\tframeContext.agc.gain = agc.automatic.gain;\n\nAs above.\n\n> \t\t}\n> \t}\n> \n> \tconst auto &exposure = controls.get(controls::ExposureTime);\n> -\tif (exposure && !agc.autoEnabled) {\n> +\tif (exposure && !agc.autoExposureEnabled) {\n> \t\tagc.manual.exposure = *exposure * 1.0us\n> \t\t\t\t / context.configuration.sensor.lineDuration;\n> \n> @@ -235,18 +265,19 @@ void Agc::queueRequest(IPAContext &context,\n> \t}\n> \n> \tconst auto &gain = controls.get(controls::AnalogueGain);\n> -\tif (gain && !agc.autoEnabled) {\n> +\tif (gain && !agc.autoGainEnabled) {\n> \t\tagc.manual.gain = *gain;\n> \n> \t\tLOG(RkISP1Agc, Debug) << \"Set gain to \" << agc.manual.gain;\n> \t}\n> \n> -\tframeContext.agc.autoEnabled = agc.autoEnabled;\n> +\tframeContext.agc.autoExposureEnabled = agc.autoExposureEnabled;\n> +\tframeContext.agc.autoGainEnabled = agc.autoGainEnabled;\n> \n> -\tif (!frameContext.agc.autoEnabled) {\n> +\tif (!frameContext.agc.autoExposureEnabled)\n> \t\tframeContext.agc.exposure = agc.manual.exposure;\n> +\tif (!frameContext.agc.autoGainEnabled)\n> \t\tframeContext.agc.gain = agc.manual.gain;\n> -\t}\n> \n> \tconst auto &meteringMode = controls.get(controls::AeMeteringMode);\n> \tif (meteringMode) {\n> @@ -283,10 +314,10 @@ void Agc::queueRequest(IPAContext &context,\n> void Agc::prepare(IPAContext &context, const uint32_t frame,\n> \t\t IPAFrameContext &frameContext, RkISP1Params *params)\n> {\n> -\tif (frameContext.agc.autoEnabled) {\n> +\tif (frameContext.agc.autoExposureEnabled)\n> \t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> +\tif (frameContext.agc.autoGainEnabled)\n> \t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> -\t}\n> \n> \tif (frame > 0 && !frameContext.agc.updateMetering)\n> \t\treturn;\n> @@ -333,7 +364,14 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> \t\t\t\t * frameContext.sensor.exposure;\n> \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> -\tmetadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> +\tmetadata.set(controls::ExposureTimeMode,\n> +\t\t frameContext.agc.autoExposureEnabled\n> +\t\t ? controls::ExposureTimeModeAuto\n> +\t\t : controls::ExposureTimeModeManual);\n> +\tmetadata.set(controls::AnalogueGainMode,\n> +\t\t frameContext.agc.autoGainEnabled\n> +\t\t ? controls::AnalogueGainModeAuto\n> +\t\t : controls::AnalogueGainModeManual);\n> \n> \t/* \\todo Use VBlank value calculated from each frame exposure. */\n> \tuint32_t vTotal = context.configuration.sensor.size.height\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 14d0c02a2b32..a8e8e1c0d06e 100644\n> --- a/src/ipa/rkisp1/ipa_context.cpp\n> +++ b/src/ipa/rkisp1/ipa_context.cpp\n> @@ -165,8 +165,11 @@ namespace libcamera::ipa::rkisp1 {\n> * \\var IPAActiveState::agc.automatic.gain\n> * \\brief Automatic analogue gain multiplier\n> *\n> - * \\var IPAActiveState::agc.autoEnabled\n> - * \\brief Manual/automatic AGC state as set by the AeEnable control\n> + * \\var IPAActiveState::agc.autoExposureEnabled\n> + * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n> + *\n> + * \\var IPAActiveState::agc.autoGainEnabled\n> + * \\brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control\n> *\n> * \\var IPAActiveState::agc.constraintMode\n> * \\brief Constraint mode as set by the AeConstraintMode control\n> @@ -307,8 +310,11 @@ namespace libcamera::ipa::rkisp1 {\n> *\n> * The gain should be adapted to the sensor specific gain code before applying.\n> *\n> - * \\var IPAFrameContext::agc.autoEnabled\n> - * \\brief Manual/automatic AGC state as set by the AeEnable control\n> + * \\var IPAFrameContext::agc.autoExposureEnabled\n> + * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n> + *\n> + * \\var IPAFrameContext::agc.autoGainEnabled\n> + * \\brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control\n> *\n> * \\var IPAFrameContext::agc.constraintMode\n> * \\brief Constraint mode as set by the AeConstraintMode control\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index e274d9b01e1c..7f819d31015e 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -75,7 +75,8 @@ struct IPAActiveState {\n> \t\t\tdouble gain;\n> \t\t} automatic;\n> \n> -\t\tbool autoEnabled;\n> +\t\tbool autoExposureEnabled;\n> +\t\tbool autoGainEnabled;\n> \t\tcontrols::AeConstraintModeEnum constraintMode;\n> \t\tcontrols::AeExposureModeEnum exposureMode;\n> \t\tcontrols::AeMeteringModeEnum meteringMode;\n> @@ -128,7 +129,8 @@ struct IPAFrameContext : public FrameContext {\n> \tstruct {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> -\t\tbool autoEnabled;\n> +\t\tbool autoExposureEnabled;\n> +\t\tbool autoGainEnabled;\n> \t\tcontrols::AeConstraintModeEnum constraintMode;\n> \t\tcontrols::AeExposureModeEnum exposureMode;\n> \t\tcontrols::AeMeteringModeEnum meteringMode;\n> -- \n> 2.39.2\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 530F5C0F1B\n\tfor ;\n\tThu, 14 Nov 2024 15:20:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 385AA6584C;\n\tThu, 14 Nov 2024 16:20:49 +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 1DF8C657E0\n\tfor ;\n\tThu, 14 Nov 2024 16:20:44 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:cd64:b5ef:1d95:ef1c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E6A5E291;\n\tThu, 14 Nov 2024 16:20:29 +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=\"nm+MamnZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731597630;\n\tbh=SOWw68IgfNLWrc06SMBjnzJTeU4vkCvU/lWlNd7VTs0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nm+MamnZhSfYFQMewQHVMUV4E/qrdJwNS8IREdQz4mfp1Hpd7QAhcX6ffE8Ze5NqJ\n\tUHnq4rfuEA8MGjQmGHJ+bBIs/hvnDtQux/0Y3SGUcC5DTaqREBSvJ/Ed44MB8/2OB0\n\t87LlBTHDhsMogckGwOwKbYGyzlBHBvYZRtomIac4=","Date":"Thu, 14 Nov 2024 16:20:40 +0100","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, laurent.pinchart@ideasonboard.com, \n\tjacopo.mondi@ideasonboard.com, naush@raspberrypi.com,\n\tdavid.plowman@raspberrypi.com","Subject":"Re: [PATCH v3 6/8] ipa: rkisp1: Port to the new AEGC controls","Message-ID":"<7vxzvp2vjmmjilosnbclkls6kctrxmb4hn2ckbh4oy3btmicuq@fmtx7oymmnda>","References":"<20241113131256.3170817-1-paul.elder@ideasonboard.com>\n\t<20241113131256.3170817-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241113131256.3170817-7-paul.elder@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":32321,"web_url":"https://patchwork.libcamera.org/comment/32321/","msgid":"<20241121032741.GD12409@pendragon.ideasonboard.com>","date":"2024-11-21T03:27:41","subject":"Re: [PATCH v3 6/8] ipa: rkisp1: Port to the new AEGC controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Nov 14, 2024 at 04:20:40PM +0100, Stefan Klug wrote:\n> Hi Paul,\n> \n> Thank you for the patch. \n> \n> On Wed, Nov 13, 2024 at 10:12:54PM +0900, Paul Elder wrote:\n> > The newly introduced controls to drive the AEGC algorithm allow\n> > controlling the computation of the exposure time and analogue gain\n> > separately.\n> > \n> > Augument the RkISP1 AEGC implementation to handle the exposure and gain\n> > controls separately using the new controls.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > New in v3\n> > \n> > Back 2 years ago in v2 RkISP1 didn't yet support AeEnable properly yet\n> > so the AeEnable control was simply removed.\n> > ---\n> > src/ipa/rkisp1/algorithms/agc.cpp | 68 ++++++++++++++++++++++++-------\n> > src/ipa/rkisp1/ipa_context.cpp | 14 +++++--\n> > src/ipa/rkisp1/ipa_context.h | 6 ++-\n> > 3 files changed, 67 insertions(+), 21 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 301b7ec26508..0ad206621516 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -148,7 +148,12 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)\n> > \tif (ret)\n> > \t\treturn ret;\n> > \n> > -\tcontext.ctrlMap[&controls::AeEnable] = ControlInfo(false, true);\n> > +\tcontext.ctrlMap[&controls::ExposureTimeMode] =\n> > +\t\tControlInfo(static_cast(controls::ExposureTimeModeAuto),\n> > +\t\t\t static_cast(controls::ExposureTimeModeManual));\n> > +\tcontext.ctrlMap[&controls::AnalogueGainMode] =\n> > +\t\tControlInfo(static_cast(controls::AnalogueGainModeAuto),\n> > +\t\t\t static_cast(controls::AnalogueGainModeManual));\n> \n> I believe these should also have default values.\n\nThey should, and I think they should default to auto. Can we mandate in\nthe controls documentation that a camera that supports auto mode must\ndefault to auto ?\n\n> > \tcontext.ctrlMap.merge(controls());\n> > \n> > \treturn 0;\n> > @@ -169,7 +174,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > \t\t10ms / context.configuration.sensor.lineDuration;\n> > \tcontext.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;\n> > \tcontext.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;\n> > -\tcontext.activeState.agc.autoEnabled = !context.configuration.raw;\n> > +\tcontext.activeState.agc.autoExposureEnabled = !context.configuration.raw;\n> > +\tcontext.activeState.agc.autoGainEnabled = !context.configuration.raw;\n> > \n> \n> Ahh this actually sets the defaults internally, but the defaults above\n> are the ones seen by applications like camshark.\n> \n> > \tcontext.activeState.agc.constraintMode =\n> > \t\tstatic_cast(constraintModes().begin()->first);\n> > @@ -215,18 +221,42 @@ void Agc::queueRequest(IPAContext &context,\n> > \tauto &agc = context.activeState.agc;\n> > \n> > \tif (!context.configuration.raw) {\n> > -\t\tconst auto &agcEnable = controls.get(controls::AeEnable);\n> > -\t\tif (agcEnable && *agcEnable != agc.autoEnabled) {\n> > -\t\t\tagc.autoEnabled = *agcEnable;\n> > +\t\tconst auto &aeEnable = controls.get(controls::ExposureTimeMode);\n> > +\t\tif (aeEnable &&\n> > +\t\t (*aeEnable == controls::ExposureTimeModeAuto) != agc.autoExposureEnabled) {\n> > +\t\t\tagc.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);\n> > \n> > \t\t\tLOG(RkISP1Agc, Debug)\n> > -\t\t\t\t<< (agc.autoEnabled ? \"Enabling\" : \"Disabling\")\n> > -\t\t\t\t<< \" AGC\";\n> > +\t\t\t\t<< (agc.autoExposureEnabled ? \"Enabling\" : \"Disabling\")\n> > +\t\t\t\t<< \" AGC (exposure)\";\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * If we go from auto -> manual with no manual control\n> > +\t\t\t * set, use the last computed value\n> > +\t\t\t */\n> > +\t\t\tif (!agc.autoExposureEnabled && !controls.get(controls::ExposureTime))\n> > +\t\t\t\tframeContext.agc.exposure = agc.automatic.exposure;\n> \n> Does this work? It sets the exposure time of the future frame (that is\n> currently beeing queued) to the current value from active state. But\n> that value will change depending on how many frames are queued already.\n> I guess we need to set a flag and fetch the exposure time from\n> activeState when this frame gets processed.\n> \n> > +\t\t}\n> > +\n> > +\t\tconst auto &agEnable = controls.get(controls::AnalogueGainMode);\n> > +\t\tif (agEnable &&\n> > +\t\t (*agEnable == controls::AnalogueGainModeAuto) != agc.autoGainEnabled) {\n> > +\t\t\tagc.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);\n> > +\n> > +\t\t\tLOG(RkISP1Agc, Debug)\n> > +\t\t\t\t<< (agc.autoGainEnabled ? \"Enabling\" : \"Disabling\")\n> > +\t\t\t\t<< \" AGC (gain)\";\n> > +\t\t\t/*\n> > +\t\t\t * If we go from auto -> manual with no manual control\n> > +\t\t\t * set, use the last computed value\n> > +\t\t\t */\n> > +\t\t\tif (!agc.autoGainEnabled && !controls.get(controls::AnalogueGain))\n> > +\t\t\t\tframeContext.agc.gain = agc.automatic.gain;\n> \n> As above.\n> \n> > \t\t}\n> > \t}\n> > \n> > \tconst auto &exposure = controls.get(controls::ExposureTime);\n> > -\tif (exposure && !agc.autoEnabled) {\n> > +\tif (exposure && !agc.autoExposureEnabled) {\n> > \t\tagc.manual.exposure = *exposure * 1.0us\n> > \t\t\t\t / context.configuration.sensor.lineDuration;\n> > \n> > @@ -235,18 +265,19 @@ void Agc::queueRequest(IPAContext &context,\n> > \t}\n> > \n> > \tconst auto &gain = controls.get(controls::AnalogueGain);\n> > -\tif (gain && !agc.autoEnabled) {\n> > +\tif (gain && !agc.autoGainEnabled) {\n> > \t\tagc.manual.gain = *gain;\n> > \n> > \t\tLOG(RkISP1Agc, Debug) << \"Set gain to \" << agc.manual.gain;\n> > \t}\n> > \n> > -\tframeContext.agc.autoEnabled = agc.autoEnabled;\n> > +\tframeContext.agc.autoExposureEnabled = agc.autoExposureEnabled;\n> > +\tframeContext.agc.autoGainEnabled = agc.autoGainEnabled;\n> > \n> > -\tif (!frameContext.agc.autoEnabled) {\n> > +\tif (!frameContext.agc.autoExposureEnabled)\n> > \t\tframeContext.agc.exposure = agc.manual.exposure;\n> > +\tif (!frameContext.agc.autoGainEnabled)\n> > \t\tframeContext.agc.gain = agc.manual.gain;\n> > -\t}\n> > \n> > \tconst auto &meteringMode = controls.get(controls::AeMeteringMode);\n> > \tif (meteringMode) {\n> > @@ -283,10 +314,10 @@ void Agc::queueRequest(IPAContext &context,\n> > void Agc::prepare(IPAContext &context, const uint32_t frame,\n> > \t\t IPAFrameContext &frameContext, RkISP1Params *params)\n> > {\n> > -\tif (frameContext.agc.autoEnabled) {\n> > +\tif (frameContext.agc.autoExposureEnabled)\n> > \t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> > +\tif (frameContext.agc.autoGainEnabled)\n> > \t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> > -\t}\n> > \n> > \tif (frame > 0 && !frameContext.agc.updateMetering)\n> > \t\treturn;\n> > @@ -333,7 +364,14 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,\n> > \t\t\t\t * frameContext.sensor.exposure;\n> > \tmetadata.set(controls::AnalogueGain, frameContext.sensor.gain);\n> > \tmetadata.set(controls::ExposureTime, exposureTime.get());\n> > -\tmetadata.set(controls::AeEnable, frameContext.agc.autoEnabled);\n> > +\tmetadata.set(controls::ExposureTimeMode,\n> > +\t\t frameContext.agc.autoExposureEnabled\n> > +\t\t ? controls::ExposureTimeModeAuto\n> > +\t\t : controls::ExposureTimeModeManual);\n> > +\tmetadata.set(controls::AnalogueGainMode,\n> > +\t\t frameContext.agc.autoGainEnabled\n> > +\t\t ? controls::AnalogueGainModeAuto\n> > +\t\t : controls::AnalogueGainModeManual);\n> > \n> > \t/* \\todo Use VBlank value calculated from each frame exposure. */\n> > \tuint32_t vTotal = context.configuration.sensor.size.height\n> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> > index 14d0c02a2b32..a8e8e1c0d06e 100644\n> > --- a/src/ipa/rkisp1/ipa_context.cpp\n> > +++ b/src/ipa/rkisp1/ipa_context.cpp\n> > @@ -165,8 +165,11 @@ namespace libcamera::ipa::rkisp1 {\n> > * \\var IPAActiveState::agc.automatic.gain\n> > * \\brief Automatic analogue gain multiplier\n> > *\n> > - * \\var IPAActiveState::agc.autoEnabled\n> > - * \\brief Manual/automatic AGC state as set by the AeEnable control\n> > + * \\var IPAActiveState::agc.autoExposureEnabled\n> > + * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n> > + *\n> > + * \\var IPAActiveState::agc.autoGainEnabled\n> > + * \\brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control\n> > *\n> > * \\var IPAActiveState::agc.constraintMode\n> > * \\brief Constraint mode as set by the AeConstraintMode control\n> > @@ -307,8 +310,11 @@ namespace libcamera::ipa::rkisp1 {\n> > *\n> > * The gain should be adapted to the sensor specific gain code before applying.\n> > *\n> > - * \\var IPAFrameContext::agc.autoEnabled\n> > - * \\brief Manual/automatic AGC state as set by the AeEnable control\n> > + * \\var IPAFrameContext::agc.autoExposureEnabled\n> > + * \\brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control\n> > + *\n> > + * \\var IPAFrameContext::agc.autoGainEnabled\n> > + * \\brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control\n> > *\n> > * \\var IPAFrameContext::agc.constraintMode\n> > * \\brief Constraint mode as set by the AeConstraintMode control\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index e274d9b01e1c..7f819d31015e 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -75,7 +75,8 @@ struct IPAActiveState {\n> > \t\t\tdouble gain;\n> > \t\t} automatic;\n> > \n> > -\t\tbool autoEnabled;\n> > +\t\tbool autoExposureEnabled;\n> > +\t\tbool autoGainEnabled;\n> > \t\tcontrols::AeConstraintModeEnum constraintMode;\n> > \t\tcontrols::AeExposureModeEnum exposureMode;\n> > \t\tcontrols::AeMeteringModeEnum meteringMode;\n> > @@ -128,7 +129,8 @@ struct IPAFrameContext : public FrameContext {\n> > \tstruct {\n> > \t\tuint32_t exposure;\n> > \t\tdouble gain;\n> > -\t\tbool autoEnabled;\n> > +\t\tbool autoExposureEnabled;\n> > +\t\tbool autoGainEnabled;\n> > \t\tcontrols::AeConstraintModeEnum constraintMode;\n> > \t\tcontrols::AeExposureModeEnum exposureMode;\n> > \t\tcontrols::AeMeteringModeEnum meteringMode;","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 EDE9CC32F5\n\tfor ;\n\tThu, 21 Nov 2024 03:27:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0998465F9F;\n\tThu, 21 Nov 2024 04:27:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07AA265F54\n\tfor ;\n\tThu, 21 Nov 2024 04:27:51 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 411FB219;\n\tThu, 21 Nov 2024 04:27:32 +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=\"kvV8RJqB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732159652;\n\tbh=aKkf7k43Kl6yoF8mXP1yk6+qwSacEx3eQQOuLYFv+x8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kvV8RJqBV/osH1t3Pd32ZQjF4QWtSfIH5fCtV/q9a44fZy3qmlQxApwTIJQefqkeS\n\t7Q3qeIYx08+fqdusTvodhZDYQRIJqwglzMi1+qWPxJzoojkvGtC7GOYlNIpNXk0Tlj\n\tAq2WlMxVsyuOlA+m/G/EeAZfJ8DOZe7k3ylHvyaM=","Date":"Thu, 21 Nov 2024 05:27:41 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org, jacopo.mondi@ideasonboard.com,\n\tnaush@raspberrypi.com, david.plowman@raspberrypi.com","Subject":"Re: [PATCH v3 6/8] ipa: rkisp1: Port to the new AEGC controls","Message-ID":"<20241121032741.GD12409@pendragon.ideasonboard.com>","References":"<20241113131256.3170817-1-paul.elder@ideasonboard.com>\n\t<20241113131256.3170817-7-paul.elder@ideasonboard.com>\n\t<7vxzvp2vjmmjilosnbclkls6kctrxmb4hn2ckbh4oy3btmicuq@fmtx7oymmnda>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<7vxzvp2vjmmjilosnbclkls6kctrxmb4hn2ckbh4oy3btmicuq@fmtx7oymmnda>","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\" "}}]