[{"id":33035,"web_url":"https://patchwork.libcamera.org/comment/33035/","msgid":"","date":"2025-01-13T10:43:41","subject":"Re: [PATCH v7 06/12] 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 Fri, Jan 10, 2025 at 05:57:31PM -0600, 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> Reviewed-by: Laurent Pinchart \n\nLooks good to me!\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> \n> ---\n> No change in v7\n> \n> No change in v6\n> \n> Changes in v5:\n> - fix mixed manual-gain ae modes\n> - improve documentation for mode change flags in the ipa context\n> \n> Changes in v4:\n> - make auto the default value in the control infos\n> - implement the expected behavior when transitioning between manual and\n> auto modes\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 | 131 ++++++++++++++++++++++++------\n> src/ipa/rkisp1/ipa_context.cpp | 24 +++++-\n> src/ipa/rkisp1/ipa_context.h | 8 +-\n> 3 files changed, 133 insertions(+), 30 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 40e5a8f48..78122a1f0 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -148,7 +148,14 @@ 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> +\t\t\t static_cast(controls::ExposureTimeModeAuto));\n> +\tcontext.ctrlMap[&controls::AnalogueGainMode] =\n> +\t\tControlInfo(static_cast(controls::AnalogueGainModeAuto),\n> +\t\t\t static_cast(controls::AnalogueGainModeManual),\n> +\t\t\t static_cast(controls::AnalogueGainModeAuto));\n> \tcontext.ctrlMap.merge(controls());\n> \n> \treturn 0;\n> @@ -169,7 +176,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> \tcontext.activeState.agc.constraintMode =\n> \t\tstatic_cast(constraintModes().begin()->first);\n> @@ -215,18 +223,47 @@ 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, which we don't\n> +\t\t\t * know until prepare() so save this information.\n> +\t\t\t *\n> +\t\t\t * \\todo Check the previous frame at prepare() time\n> +\t\t\t * instead of saving a flag here\n> +\t\t\t */\n> +\t\t\tif (!agc.autoExposureEnabled && !controls.get(controls::ExposureTime))\n> +\t\t\t\tframeContext.agc.autoExposureModeChange = true;\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, which we don't\n> +\t\t\t * know until prepare() so save this information.\n> +\t\t\t */\n> +\t\t\tif (!agc.autoGainEnabled && !controls.get(controls::AnalogueGain))\n> +\t\t\t\tframeContext.agc.autoGainModeChange = true;\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 +272,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,9 +321,26 @@ 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> -\t\tframeContext.agc.exposure = context.activeState.agc.automatic.exposure;\n> -\t\tframeContext.agc.gain = context.activeState.agc.automatic.gain;\n> +\tuint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;\n> +\tdouble activeAutoGain = context.activeState.agc.automatic.gain;\n> +\n> +\t/* Populate exposure and gain in auto mode */\n> +\tif (frameContext.agc.autoExposureEnabled)\n> +\t\tframeContext.agc.exposure = activeAutoExposure;\n> +\tif (frameContext.agc.autoGainEnabled)\n> +\t\tframeContext.agc.gain = activeAutoGain;\n> +\n> +\t/*\n> +\t * Populate manual exposure and gain from the active auto values when\n> +\t * transitioning from auto to manual\n> +\t */\n> +\tif (!frameContext.agc.autoExposureEnabled && frameContext.agc.autoExposureModeChange) {\n> +\t\tcontext.activeState.agc.manual.exposure = activeAutoExposure;\n> +\t\tframeContext.agc.exposure = activeAutoExposure;\n> +\t}\n> +\tif (!frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange) {\n> +\t\tcontext.activeState.agc.manual.gain = activeAutoGain;\n> +\t\tframeContext.agc.gain = activeAutoGain;\n> \t}\n> \n> \tif (frame > 0 && !frameContext.agc.updateMetering)\n> @@ -333,7 +388,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> @@ -424,14 +486,35 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> \t\t [](uint32_t x) { return x >> 4; });\n> \texpMeans_ = { params->ae.exp_mean, context.hw->numAeCells };\n> \n> -\tutils::Duration maxExposureTime =\n> -\t\tstd::clamp(frameContext.agc.maxFrameDuration,\n> -\t\t\t context.configuration.sensor.minExposureTime,\n> -\t\t\t context.configuration.sensor.maxExposureTime);\n> -\tsetLimits(context.configuration.sensor.minExposureTime,\n> -\t\t maxExposureTime,\n> -\t\t context.configuration.sensor.minAnalogueGain,\n> -\t\t context.configuration.sensor.maxAnalogueGain);\n> +\t/*\n> +\t * Set the AGC limits using the fixed exposure time and/or gain in\n> +\t * manual mode, or the sensor limits in auto mode.\n> +\t */\n> +\tutils::Duration minExposureTime;\n> +\tutils::Duration maxExposureTime;\n> +\tdouble minAnalogueGain;\n> +\tdouble maxAnalogueGain;\n> +\n> +\tif (frameContext.agc.autoExposureEnabled) {\n> +\t\tminExposureTime = context.configuration.sensor.minExposureTime;\n> +\t\tmaxExposureTime = std::clamp(frameContext.agc.maxFrameDuration,\n> +\t\t\t\t\t context.configuration.sensor.minExposureTime,\n> +\t\t\t\t\t context.configuration.sensor.maxExposureTime);\n> +\t} else {\n> +\t\tminExposureTime = context.configuration.sensor.lineDuration\n> +\t\t\t\t* frameContext.agc.exposure;\n> +\t\tmaxExposureTime = minExposureTime;\n> +\t}\n> +\n> +\tif (frameContext.agc.autoGainEnabled) {\n> +\t\tminAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> +\t\tmaxAnalogueGain = context.configuration.sensor.maxAnalogueGain;\n> +\t} else {\n> +\t\tminAnalogueGain = frameContext.agc.gain;\n> +\t\tmaxAnalogueGain = frameContext.agc.gain;\n> +\t}\n> +\n> +\tsetLimits(minExposureTime, maxExposureTime, minAnalogueGain, maxAnalogueGain);\n> \n> \t/*\n> \t * The Agc algorithm needs to know the effective exposure value that was\n> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp\n> index 80b99df8e..261c0472a 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> @@ -289,8 +292,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> @@ -306,6 +312,16 @@ namespace libcamera::ipa::rkisp1 {\n> *\n> * \\var IPAFrameContext::agc.updateMetering\n> * \\brief Indicate if new ISP AGC metering parameters need to be applied\n> + *\n> + * \\var IPAFrameContext::agc.autoExposureModeChange\n> + * \\brief Indicate if autoExposureEnabled has changed from true in the previous\n> + * frame to false in the current frame, and no manual exposure value has been\n> + * supplied in the current frame.\n> + *\n> + * \\var IPAFrameContext::agc.autoGainModeChange\n> + * \\brief Indicate if autoGainEnabled has changed from true in the previous\n> + * frame to false in the current frame, and no manual gain value has been\n> + * supplied in the current frame.\n> */\n> \n> /**\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index b83c18229..5d5b79fa6 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -79,7 +79,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> @@ -124,12 +125,15 @@ 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> \t\tutils::Duration maxFrameDuration;\n> \t\tbool updateMetering;\n> +\t\tbool autoExposureModeChange;\n> +\t\tbool autoGainModeChange;\n> \t} agc;\n> \n> \tstruct {\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 A18D2C32F6\n\tfor ;\n\tMon, 13 Jan 2025 10:43:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D5132684E1;\n\tMon, 13 Jan 2025 11:43:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D7A2607D6\n\tfor ;\n\tMon, 13 Jan 2025 11:43:44 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:3c15:8eea:8742:2e5a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 199F77E9;\n\tMon, 13 Jan 2025 11:42:48 +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=\"uIxayPFe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736764968;\n\tbh=DEQBvDm7MAZUc6GMNfeGzu/wVGVpJKvOLslTh6HeqOk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uIxayPFeY0KsYKFX5SybsSSTXj9bdKWaT9sDtllDw7Q+R3Gm1bEg8hpTJTvwFkhjf\n\tx1UStH0gWISTZMUMM+P4a/xc06hbrIGS4xKJDDsGG2Xacd+2Gn/6A9Cy32XWE6lFzV\n\tl+b0lbP/v77TwYOkwNt3gXO5kXu6MLGaYIjIaeRU=","Date":"Mon, 13 Jan 2025 11:43:41 +0100","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, laurent.pinchart@ideasonboard.com","Subject":"Re: [PATCH v7 06/12] ipa: rkisp1: Port to the new AEGC controls","Message-ID":"","References":"<20250110235737.1524733-1-paul.elder@ideasonboard.com>\n\t<20250110235737.1524733-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250110235737.1524733-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\" "}}]