[{"id":32773,"web_url":"https://patchwork.libcamera.org/comment/32773/","msgid":"<20241216150450.GA32392@pendragon.ideasonboard.com>","date":"2024-12-16T15:04:50","subject":"Re: [PATCH v5 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":"Hi Paul,\n\nThank you for the patch.\n\nOn Mon, Dec 16, 2024 at 01:39:52PM +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> 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 | 122 +++++++++++++++++++++++++-----\n> src/ipa/rkisp1/ipa_context.cpp | 24 +++++-\n> src/ipa/rkisp1/ipa_context.h | 8 +-\n> 3 files changed, 128 insertions(+), 26 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index 40e5a8f481b2..6dfb4c8adc41 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> @@ -428,10 +490,30 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\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> +\tutils::Duration manualExposureTime =\n> +\t\tcontext.configuration.sensor.lineDuration * frameContext.agc.exposure;\n> +\n> +\tif (frameContext.agc.autoExposureEnabled &&\n> +\t !frameContext.agc.autoGainEnabled) {\n> +\t\tsetLimits(context.configuration.sensor.minExposureTime,\n> +\t\t\t maxExposureTime,\n> +\t\t\t frameContext.agc.gain,\n> +\t\t\t frameContext.agc.gain);\n> +\t} else if (!frameContext.agc.autoExposureEnabled &&\n> +\t\t frameContext.agc.autoGainEnabled) {\n> +\t\tsetLimits(manualExposureTime, manualExposureTime,\n> +\t\t\t context.configuration.sensor.minAnalogueGain,\n> +\t\t\t context.configuration.sensor.maxAnalogueGain);\n> +\t} else if (!frameContext.agc.autoExposureEnabled &&\n> +\t\t !frameContext.agc.autoGainEnabled) {\n> +\t\tsetLimits(manualExposureTime, manualExposureTime,\n> +\t\t\t frameContext.agc.gain, frameContext.agc.gain);\n> +\t} else {\n> +\t\tsetLimits(context.configuration.sensor.minExposureTime,\n> +\t\t\t maxExposureTime,\n> +\t\t\t context.configuration.sensor.minAnalogueGain,\n> +\t\t\t context.configuration.sensor.maxAnalogueGain);\n> +\t}\n\nThat looks complicated. How about the following ?\n\n /*\n * Set the AGC limits using the fixed exposure time and/or gain in\n * manual mode, or the sensor limits in auto mode.\n */\n utils::Duration minExposureTime;\n utils::Duration maxExposureTime;\n double minAnalogueGain;\n double maxAnalogueGain;\n\n if (frameContext.agc.autoExposureEnabled) {\n minExposureTime = context.configuration.sensor.minExposureTime;\n maxExposureTime = std::clamp(frameContext.agc.maxFrameDuration,\n context.configuration.sensor.minExposureTime,\n context.configuration.sensor.maxExposureTime);\n } else {\n minExposureTime = context.configuration.sensor.lineDuration\n * frameContext.agc.exposure;\n maxExposureTime = minExposureTime;\n }\n\n if (frameContext.agc.autoGainEnabled) {\n minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;\n } else {\n minAnalogueGain = frameContext.agc.gain;\n maxAnalogueGain = frameContext.agc.gain;\n }\n\n setLimits(minExposureTime, maxExposureTime, minAnalogueGain, maxAnalogueGain);\n\nWith that,\n\nReviewed-by: Laurent Pinchart \n\nI'm not confident making this change when pushing the patches, would you\nbe able to test this first ? No need to post a v6 for this, once you get\nan ack from David or Naush for patch 5/8, you can push the series.\n\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 80b99df8eaf8..261c0472a4fc 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 deb8c196f1b8..bc3d5a24be3a 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 {","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 5EDA0C32DA\n\tfor ;\n\tMon, 16 Dec 2024 15:05:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 140DE67F6E;\n\tMon, 16 Dec 2024 16:05:09 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 32E4C67F64\n\tfor ;\n\tMon, 16 Dec 2024 16:05:07 +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 85376675;\n\tMon, 16 Dec 2024 16:04:30 +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=\"vxJEnQGx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734361470;\n\tbh=xFGQWoD7nTeA71Z0uuTz6LiPGsnKH9NPgsAd2L9lX0o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vxJEnQGxZcd7RtWUYSk+nNmuHWnadyqBsOqWwkmhfljPpqpMeVs09ic5KeUg5auZg\n\tH2wtFC0+2H26ZH+rEG6yeRSwUw6cgfOhV828joNqra7MUQPTPRIdmjy/Nv9sOSlunq\n\tr5zBbKxo6jW/KChOEgPjeEzW4ZldzLudP3OXYhWw=","Date":"Mon, 16 Dec 2024 17:04:50 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 6/8] ipa: rkisp1: Port to the new AEGC controls","Message-ID":"<20241216150450.GA32392@pendragon.ideasonboard.com>","References":"<20241216043954.3506855-1-paul.elder@ideasonboard.com>\n\t<20241216043954.3506855-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241216043954.3506855-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":32802,"web_url":"https://patchwork.libcamera.org/comment/32802/","msgid":"","date":"2024-12-17T04:27:57","subject":"Re: [PATCH v5 6/8] ipa: rkisp1: Port to the new AEGC controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Dec 16, 2024 at 05:04:50PM +0200, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Mon, Dec 16, 2024 at 01:39:52PM +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> > 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 | 122 +++++++++++++++++++++++++-----\n> > src/ipa/rkisp1/ipa_context.cpp | 24 +++++-\n> > src/ipa/rkisp1/ipa_context.h | 8 +-\n> > 3 files changed, 128 insertions(+), 26 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index 40e5a8f481b2..6dfb4c8adc41 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> > @@ -428,10 +490,30 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,\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> > +\tutils::Duration manualExposureTime =\n> > +\t\tcontext.configuration.sensor.lineDuration * frameContext.agc.exposure;\n> > +\n> > +\tif (frameContext.agc.autoExposureEnabled &&\n> > +\t !frameContext.agc.autoGainEnabled) {\n> > +\t\tsetLimits(context.configuration.sensor.minExposureTime,\n> > +\t\t\t maxExposureTime,\n> > +\t\t\t frameContext.agc.gain,\n> > +\t\t\t frameContext.agc.gain);\n> > +\t} else if (!frameContext.agc.autoExposureEnabled &&\n> > +\t\t frameContext.agc.autoGainEnabled) {\n> > +\t\tsetLimits(manualExposureTime, manualExposureTime,\n> > +\t\t\t context.configuration.sensor.minAnalogueGain,\n> > +\t\t\t context.configuration.sensor.maxAnalogueGain);\n> > +\t} else if (!frameContext.agc.autoExposureEnabled &&\n> > +\t\t !frameContext.agc.autoGainEnabled) {\n> > +\t\tsetLimits(manualExposureTime, manualExposureTime,\n> > +\t\t\t frameContext.agc.gain, frameContext.agc.gain);\n> > +\t} else {\n> > +\t\tsetLimits(context.configuration.sensor.minExposureTime,\n> > +\t\t\t maxExposureTime,\n> > +\t\t\t context.configuration.sensor.minAnalogueGain,\n> > +\t\t\t context.configuration.sensor.maxAnalogueGain);\n> > +\t}\n> \n> That looks complicated. How about the following ?\n> \n> /*\n> * Set the AGC limits using the fixed exposure time and/or gain in\n> * manual mode, or the sensor limits in auto mode.\n> */\n> utils::Duration minExposureTime;\n> utils::Duration maxExposureTime;\n> double minAnalogueGain;\n> double maxAnalogueGain;\n> \n> if (frameContext.agc.autoExposureEnabled) {\n> minExposureTime = context.configuration.sensor.minExposureTime;\n> maxExposureTime = std::clamp(frameContext.agc.maxFrameDuration,\n> context.configuration.sensor.minExposureTime,\n> context.configuration.sensor.maxExposureTime);\n> } else {\n> minExposureTime = context.configuration.sensor.lineDuration\n> * frameContext.agc.exposure;\n> maxExposureTime = minExposureTime;\n> }\n> \n> if (frameContext.agc.autoGainEnabled) {\n> minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;\n> } else {\n> minAnalogueGain = frameContext.agc.gain;\n> maxAnalogueGain = frameContext.agc.gain;\n> }\n> \n> setLimits(minExposureTime, maxExposureTime, minAnalogueGain, maxAnalogueGain);\n> \n> With that,\n\nAh yeah that is a lot nicer.\n\n> \n> Reviewed-by: Laurent Pinchart \n> \n> I'm not confident making this change when pushing the patches, would you\n> be able to test this first ? No need to post a v6 for this, once you get\n> an ack from David or Naush for patch 5/8, you can push the series.\n\nIt works! Thanks!\n\n\nPaul\n\n> \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 80b99df8eaf8..261c0472a4fc 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 deb8c196f1b8..bc3d5a24be3a 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> --","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 59BB8C32AF\n\tfor ;\n\tTue, 17 Dec 2024 04:28:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D39A67FBA;\n\tTue, 17 Dec 2024 05:28:06 +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 2B15C61899\n\tfor ;\n\tTue, 17 Dec 2024 05:28:04 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:1a78:c005:412c:c675])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F11F59D;\n\tTue, 17 Dec 2024 05:27:25 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"lZxThDYK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734409647;\n\tbh=Tv1VJnNIV2ELBpmKZQkU2V2ZEe6JSHhHYwnp6Cpq2zk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lZxThDYKe5myVjYu9RYuF6YhifUL1y6+8SMejofeSv1OA7Q9J66t4PoaQOoVKMlSs\n\tX3nwOGL5oJBST6oIuSjACAz8OpHWBO1/IX3PS+DlTWvi4eoGuwFkRwmltyg1jbhFGc\n\tMU4DMcNeDniD+Wf0/VAuaQUwGdTJcGr5N8+r9HP4=","Date":"Tue, 17 Dec 2024 13:27:57 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 6/8] ipa: rkisp1: Port to the new AEGC controls","Message-ID":"","References":"<20241216043954.3506855-1-paul.elder@ideasonboard.com>\n\t<20241216043954.3506855-7-paul.elder@ideasonboard.com>\n\t<20241216150450.GA32392@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20241216150450.GA32392@pendragon.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\" "}}]