[{"id":36693,"web_url":"https://patchwork.libcamera.org/comment/36693/","msgid":"<176232858261.231750.6488373674243924560@localhost>","date":"2025-11-05T07:43:02","subject":"Re: [PATCH v2 04/10] ipa: libipa: agc: Make Agc::configure() set\n\tlimits","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nQuoting Jacopo Mondi (2025-10-28 10:31:50)\n> The AgcMeanLuminance algorithm interface has a 'configure()' and\n> 'setLimits()' API. configure() doesn't actually do much if not\n> initialzing a few variables, and the Mali and IPU3 IPAs do not\n> even call it. Configuring the AGC requires calling setLimits() at\n> configure() time and at run time.\n\nOh I just realized, that my internal policy is to keep the libipa\ninterface backwards compatible so there is no need to update to update\nother pipeline handlers (That's why the others didn't call configure()).\nMaybe I should revisit this policy... but then any change get's even\nharder to test...\n\n> \n> In order to prepare to differentiate between configure-time settings\n> of the Agc and run-time handling of dynamic parameters such as the\n> frame duration limits, make the configure() operation initialize\n> the Agc limits.\n\nYes, that makes sense.\n\n> \n> Update all IPAs Agc implementation deriving from AgcMeanLuminance.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/ipa/ipu3/algorithms/agc.cpp | 14 +++++++---\n> src/ipa/libipa/agc_mean_luminance.cpp | 48 ++++++++++++++++++++++++++++++---\n> src/ipa/libipa/agc_mean_luminance.h | 11 +++++++-\n> src/ipa/libipa/exposure_mode_helper.cpp | 20 ++++++++++----\n> src/ipa/libipa/exposure_mode_helper.h | 4 ++-\n> src/ipa/mali-c55/algorithms/agc.cpp | 16 ++++++-----\n> src/ipa/rkisp1/algorithms/agc.cpp | 15 ++++++-----\n> 7 files changed, 101 insertions(+), 27 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index b0d89541da8550bd50472779da7fa8e33b96e2f0..e303d724ab165bac03ac6bb9d3891a76049cff47 100644\n> --- a/src/ipa/ipu3/algorithms/agc.cpp\n> +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> @@ -115,9 +115,17 @@ int Agc::configure(IPAContext &context,\n> context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> \n> - /* \\todo Run this again when FrameDurationLimits is passed in */\n> - setLimits(minExposureTime_, maxExposureTime_, minAnalogueGain_,\n> - maxAnalogueGain_, {});\n> + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> + sensorConfig.minExposureTime = minExposureTime_;\n> + sensorConfig.maxExposureTime = maxExposureTime_;\n> + sensorConfig.minAnalogueGain = minAnalogueGain_;\n> + sensorConfig.maxAnalogueGain = maxAnalogueGain_;\n> +\n> + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> +\n> + /* \\todo Update AGC limits when FrameDurationLimits is passed in */\n> +\n> resetFrameCount();\n> \n> return 0;\n> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> index 64f36bd75dd213671b5a2e6810245096ed001f21..9886864fc42514040eadb4c0b424796df1f1b6e1 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -95,6 +95,42 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> * \\brief The luminance target for the constraint\n> */\n> \n> +/**\n> + * \\struct AgcMeanLuminance::AgcSensorConfiguration\n> + * \\brief The sensor configuration parameters\n> + *\n> + * This structure collects the sensor configuration parameters which need\n> + * to be provided to the AGC algorithm at configure() time.\n> + *\n> + * Each time configure() is called the sensor configuration need to be updated\n> + * with new parameters.\n\nReading the series I (again) realized how hard it is to follow the\ndependencies/constraints in the system.\n\nI wonder if this should be moved to the sensor helper? In the end it is\nnot a configuration of the AGC but a property of the sensor. Aside from\na hardcoded 60ms in IPU3 I don't see that we impose additional AGC\nspecific limits. I think Kieran had similar thoughts.\n\nIf we leave it in here, we could call it AgcRegulationLimits, then\nlineDuration doesn't fit the scheme... so maybe just AgcConfiguration?\n\nBut I think fetching this info from the sensor helper and configuring\nthat one would be best.\n\n> + */\n> +\n> +/**\n> + * \\var AgcMeanLuminance::AgcSensorConfiguration::lineDuration\n> + * \\brief The line duration in microseconds\n> + */\n> +\n> +/**\n> + * \\var AgcMeanLuminance::AgcSensorConfiguration::minExposureTime\n> + * \\brief The sensor minimum exposure time in microseconds\n> + */\n> +\n> +/**\n> + * \\var AgcMeanLuminance::AgcSensorConfiguration::maxExposureTime\n> + * \\brief The sensor maximum exposure time in microseconds\n\n\nAs a first time IPA implementer I would fill this with the maximum of\nV4L2_CID_EXPOSURE converted to microseconds. Which is clearly the wrong\nthing to do. Still I wonder if this should be made extra clear but I\nhave difficulties finding the right words.\n\nMaybe \"maximum achievable exposure time\" - doesn't actually convey more\ninformation. \"max exposure time the hardware can do\" might make it\neasier to switch away from the exposure control. I don't know.\n\n> + */\n> +\n> +/**\n> + * \\var AgcMeanLuminance::AgcSensorConfiguration::minAnalogueGain\n> + * \\brief The sensor minimum analogue gain absolute value\n> + */\n> +\n> +/**\n> + * \\var AgcMeanLuminance::AgcSensorConfiguration::maxAnalogueGain\n> + * \\brief The sensor maximum analogue gain absolute value\n> + */\n> +\n> /**\n> * \\class AgcMeanLuminance\n> * \\brief A mean-based auto-exposure algorithm\n> @@ -314,17 +350,21 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)\n> \n> /**\n> * \\brief Configure the exposure mode helpers\n> - * \\param[in] lineDuration The sensor line length\n> + * \\param[in] config The sensor configuration\n> * \\param[in] sensorHelper The sensor helper\n> *\n> - * This function configures the exposure mode helpers so they can correctly\n> + * This function configures the exposure mode helpers by providing them the\n> + * sensor configuration parameters and the sensor helper, so they can correctly\n> * take quantization effects into account.\n> */\n> -void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> +void AgcMeanLuminance::configure(const AgcSensorConfiguration &config,\n> const CameraSensorHelper *sensorHelper)\n> {\n> for (auto &[id, helper] : exposureModeHelpers_)\n> - helper->configure(lineDuration, sensorHelper);\n> + helper->configure(config.lineDuration,\n> + config.minExposureTime, config.maxExposureTime,\n> + config.minAnalogueGain, config.maxAnalogueGain,\n> + sensorHelper);\n\nTying the information to the sensor helper would make the sensor helper\nmandatory, but would simplify this.\n\n> }\n> \n> /**\n> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> index d7ec548e3e585e28fd03b956c8de7eaac5dc6e99..5ab75a3ac6c550a655f4bb7c8c30419ad8249183 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.h\n> +++ b/src/ipa/libipa/agc_mean_luminance.h\n> @@ -42,7 +42,16 @@ public:\n> double yTarget;\n> };\n> \n> - void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);\n> + struct AgcSensorConfiguration {\n> + utils::Duration lineDuration;\n> + utils::Duration minExposureTime;\n> + utils::Duration maxExposureTime;\n> + double minAnalogueGain;\n> + double maxAnalogueGain;\n> + };\n> +\n> + void configure(const AgcSensorConfiguration &config,\n> + const CameraSensorHelper *sensorHelper);\n> int parseTuningData(const YamlObject &tuningData);\n> \n> void setExposureCompensation(double gain)\n> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> index 29e316d9d09181cd65cb08f26babb1a680bee62a..557e14230ba4e0f120653bc09ea23a29a54cb4c8 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> @@ -82,22 +82,32 @@ ExposureModeHelper::ExposureModeHelper(const Span /**\n> * \\brief Configure sensor details\n> * \\param[in] lineDuration The current line length of the sensor\n> + * \\param[in] minExposureTime The minimum exposure time supported\n> + * \\param[in] maxExposureTime The maximum exposure time supported\n> + * \\param[in] minGain The minimum analogue gain supported\n> + * \\param[in] maxGain The maximum analogue gain supported\n> * \\param[in] sensorHelper The sensor helper\n> *\n> - * This function sets the line length and sensor helper. These are used in\n> - * splitExposure() to take the quantization of the exposure and gain into\n> - * account.\n> + * This function initializes the exposure helper settings using the sensor\n> + * configuration parameters.\n> *\n> - * When this has not been called, it is assumed that exposure is in micro second\n> - * granularity and gain has no quantization at all.\n> + * The sensor parameters' min and max limits are used in splitExposure() to take\n> + * the quantization of the exposure and gain into account.\n\nThis sentence now changed it's meaning as the min/max limits can't be\nused to take quantization into account... but I agree that without the\nquantization context in mind, this sentence came out of the blue.\n\n> *\n> * ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the caller\n> * has to ensure that sensorHelper is valid until the next call to configure().\n> */\n> void ExposureModeHelper::configure(utils::Duration lineDuration,\n> + utils::Duration minExposureTime,\n> + utils::Duration maxExposureTime,\n> + double minGain, double maxGain,\n> const CameraSensorHelper *sensorHelper)\n> {\n> lineDuration_ = lineDuration;\n> + minExposureTime_ = minExposureTime;\n> + maxExposureTime_ = maxExposureTime;\n> + minGain_ = minGain;\n> + maxGain_ = maxGain;\n\nThis now sets the same members as setLimits() so, setLimits() can't\ninternally clamp to the hardware limits (it never did). When we move the\nhardware limits into the sensor helper we'd additionally gain that\nability.\n\nBest regards,\nStefan\n\n> sensorHelper_ = sensorHelper;\n> }\n> \n> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> index 968192ddc5af768ae0de58aca6c7230c7b3bd507..bb0a692c99b3128a2ee3f15304f125e073721536 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.h\n> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> @@ -26,7 +26,9 @@ public:\n> ExposureModeHelper(const Span> stages);\n> ~ExposureModeHelper() = default;\n> \n> - void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);\n> + void configure(utils::Duration lineLength, utils::Duration minExposureTime,\n> + utils::Duration maxExposureTime, double minGain, double maxGain,\n> + const CameraSensorHelper *sensorHelper);\n> void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> double minGain, double maxGain);\n> \n> diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp\n> index f60fddac3f04fd6f09dc782e929ff1593758c29b..ccf72cc9e8c33b1a516322c2445efa1684b7f751 100644\n> --- a/src/ipa/mali-c55/algorithms/agc.cpp\n> +++ b/src/ipa/mali-c55/algorithms/agc.cpp\n> @@ -169,12 +169,16 @@ int Agc::configure(IPAContext &context,\n> context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> \n> - /* \\todo Run this again when FrameDurationLimits is passed in */\n> - setLimits(context.configuration.agc.minShutterSpeed,\n> - context.configuration.agc.maxShutterSpeed,\n> - context.configuration.agc.minAnalogueGain,\n> - context.configuration.agc.maxAnalogueGain,\n> - {});\n> + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> + sensorConfig.minExposureTime = context.configuration.agc.minShutterSpeed;\n> + sensorConfig.maxExposureTime = context.configuration.agc.maxShutterSpeed;\n> + sensorConfig.minAnalogueGain = context.configuration.agc.minAnalogueGain;\n> + sensorConfig.maxAnalogueGain = context.configuration.agc.maxAnalogueGain;\n> +\n> + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> +\n> + /* \\todo Update AGC limits when FrameDurationLimits is passed in */\n> \n> resetFrameCount();\n> \n> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..8c07e6a6eac091d949ad71ee967c893897313cc8 100644\n> --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> @@ -200,13 +200,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;\n> context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;\n> \n> - AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,\n> - context.camHelper.get());\n> -\n> - setLimits(context.configuration.sensor.minExposureTime,\n> - context.configuration.sensor.maxExposureTime,\n> - context.configuration.sensor.minAnalogueGain,\n> - context.configuration.sensor.maxAnalogueGain, {});\n> + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> + sensorConfig.minExposureTime = context.configuration.sensor.minExposureTime;\n> + sensorConfig.maxExposureTime = context.configuration.sensor.maxExposureTime;\n> + sensorConfig.minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> + sensorConfig.maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;\n> +\n> + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> \n> context.activeState.agc.automatic.yTarget = effectiveYTarget();\n> \n> \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 49D75BDE4C\n\tfor ;\n\tWed, 5 Nov 2025 07:43:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D28260A80;\n\tWed, 5 Nov 2025 08:43:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6589A606A0\n\tfor ;\n\tWed, 5 Nov 2025 08:43:05 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:3793:9dc2:3dec:ebc3])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 2335E82E;\n\tWed, 5 Nov 2025 08:41:11 +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=\"pjuNwx4C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762328471;\n\tbh=zYocjEO+FnS22IBqKRUMl1nc03HVBO6F9qXqPuUYnsw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=pjuNwx4C7P3zhdbhWioBHKROlVV2HUYLibtBPZIJ20G7okFHkoof2SnPW0c/St9Z1\n\tkgTleJaPWD35OEtPfyRdQFdeYXTqlGUVsCnsm2td1QrSq57gLJjGl0cSqwKpJ2Y/qO\n\t93CwEGjLVLYMtRaQ+l+cHoe3e1iXW6EPse0mduwQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251028-exposure-limits-v2-4-a8b5a318323e@ideasonboard.com>","References":"<20251028-exposure-limits-v2-0-a8b5a318323e@ideasonboard.com>\n\t<20251028-exposure-limits-v2-4-a8b5a318323e@ideasonboard.com>","Subject":"Re: [PATCH v2 04/10] ipa: libipa: agc: Make Agc::configure() set\n\tlimits","From":"Stefan Klug ","Cc":"Jacopo Mondi ","To":"Jacopo Mondi , Niklas =?utf-8?b?U8O2?=\n\t=?utf-8?q?derlund?= , Robert Mader\n\t, libcamera-devel@lists.libcamera.org","Date":"Wed, 05 Nov 2025 08:43:02 +0100","Message-ID":"<176232858261.231750.6488373674243924560@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36695,"web_url":"https://patchwork.libcamera.org/comment/36695/","msgid":"<176233304563.3742839.8261569944215394507@ping.linuxembedded.co.uk>","date":"2025-11-05T08:57:25","subject":"Re: [PATCH v2 04/10] ipa: libipa: agc: Make Agc::configure() set\n\tlimits","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-11-05 07:43:02)\n> Hi Jacopo,\n> \n> Thank you for the patch.\n> \n> Quoting Jacopo Mondi (2025-10-28 10:31:50)\n> > The AgcMeanLuminance algorithm interface has a 'configure()' and\n> > 'setLimits()' API. configure() doesn't actually do much if not\n> > initialzing a few variables, and the Mali and IPU3 IPAs do not\n> > even call it. Configuring the AGC requires calling setLimits() at\n> > configure() time and at run time.\n> \n> Oh I just realized, that my internal policy is to keep the libipa\n> interface backwards compatible so there is no need to update to update\n> other pipeline handlers (That's why the others didn't call configure()).\n> Maybe I should revisit this policy... but then any change get's even\n> harder to test...\n> \n> > \n> > In order to prepare to differentiate between configure-time settings\n> > of the Agc and run-time handling of dynamic parameters such as the\n> > frame duration limits, make the configure() operation initialize\n> > the Agc limits.\n> \n> Yes, that makes sense.\n> \n> > \n> > Update all IPAs Agc implementation deriving from AgcMeanLuminance.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/ipu3/algorithms/agc.cpp | 14 +++++++---\n> > src/ipa/libipa/agc_mean_luminance.cpp | 48 ++++++++++++++++++++++++++++++---\n> > src/ipa/libipa/agc_mean_luminance.h | 11 +++++++-\n> > src/ipa/libipa/exposure_mode_helper.cpp | 20 ++++++++++----\n> > src/ipa/libipa/exposure_mode_helper.h | 4 ++-\n> > src/ipa/mali-c55/algorithms/agc.cpp | 16 ++++++-----\n> > src/ipa/rkisp1/algorithms/agc.cpp | 15 ++++++-----\n> > 7 files changed, 101 insertions(+), 27 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > index b0d89541da8550bd50472779da7fa8e33b96e2f0..e303d724ab165bac03ac6bb9d3891a76049cff47 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > @@ -115,9 +115,17 @@ int Agc::configure(IPAContext &context,\n> > context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> > \n> > - /* \\todo Run this again when FrameDurationLimits is passed in */\n> > - setLimits(minExposureTime_, maxExposureTime_, minAnalogueGain_,\n> > - maxAnalogueGain_, {});\n> > + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> > + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> > + sensorConfig.minExposureTime = minExposureTime_;\n> > + sensorConfig.maxExposureTime = maxExposureTime_;\n> > + sensorConfig.minAnalogueGain = minAnalogueGain_;\n> > + sensorConfig.maxAnalogueGain = maxAnalogueGain_;\n> > +\n> > + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> > +\n> > + /* \\todo Update AGC limits when FrameDurationLimits is passed in */\n> > +\n> > resetFrameCount();\n> > \n> > return 0;\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > index 64f36bd75dd213671b5a2e6810245096ed001f21..9886864fc42514040eadb4c0b424796df1f1b6e1 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > @@ -95,6 +95,42 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > * \\brief The luminance target for the constraint\n> > */\n> > \n> > +/**\n> > + * \\struct AgcMeanLuminance::AgcSensorConfiguration\n> > + * \\brief The sensor configuration parameters\n> > + *\n> > + * This structure collects the sensor configuration parameters which need\n> > + * to be provided to the AGC algorithm at configure() time.\n> > + *\n> > + * Each time configure() is called the sensor configuration need to be updated\n> > + * with new parameters.\n> \n> Reading the series I (again) realized how hard it is to follow the\n> dependencies/constraints in the system.\n> \n> I wonder if this should be moved to the sensor helper? In the end it is\n> not a configuration of the AGC but a property of the sensor. Aside from\n> a hardcoded 60ms in IPU3 I don't see that we impose additional AGC\n> specific limits. I think Kieran had similar thoughts.\n> \n> If we leave it in here, we could call it AgcRegulationLimits, then\n> lineDuration doesn't fit the scheme... so maybe just AgcConfiguration?\n> \n> But I think fetching this info from the sensor helper and configuring\n> that one would be best.\n\n\nYes, I think all tasks related to the CameraSensor should be in the\nCameraSensorHelper - *not* replicated across algorithms and IPAs.\nEspecially in this topic where we're talking about how should we\nconfigure and manage the limits of the CameraSensor - that should\ndefinitely all be common in my view.\n\n ipa: Move libipa camera configuration to helpers \n - https://git.uk.ideasonboard.com/kbingham/libcamera/commit/5c711d2a896d5a892ef61f263d623a440b58b33d\n\nI think CameraSensorHelper should manage the v4l2 conversions, and\nprovide an interface that an Agc can ask \"Hey what limits can I use\"\n\nThen the *common* CameraSensorHelper will report that based on any\nmanual control parsing.\n\nI'd also like to introduce a new wrapper around min/max for this topic\nbecause they're so intrinsically tied together and when I worked through\nthis I was really bothered with always having to move/manage them as a\npair :-)\n\n ipa: libipa: Provide a Bounds type to pass min/max \n - https://git.uk.ideasonboard.com/kbingham/libcamera/commit/9e85cd913213e16ea20c614003ce421ab323b9ac\n\n\n\nThose commits are part of my slow progressing soft-libipa branch:\n https://git.uk.ideasonboard.com/kbingham/libcamera/pulls/18\n\nwhere I'm working towards making it easier to integrate AgcMeanLuminance\nin new IPA modules - and factor out all the repeated or common code that\nkeeps getting duplicated.\n\n--\nKieran\n\n\n> \n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::lineDuration\n> > + * \\brief The line duration in microseconds\n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::minExposureTime\n> > + * \\brief The sensor minimum exposure time in microseconds\n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::maxExposureTime\n> > + * \\brief The sensor maximum exposure time in microseconds\n> \n> \n> As a first time IPA implementer I would fill this with the maximum of\n> V4L2_CID_EXPOSURE converted to microseconds. Which is clearly the wrong\n> thing to do. Still I wonder if this should be made extra clear but I\n> have difficulties finding the right words.\n> \n> Maybe \"maximum achievable exposure time\" - doesn't actually convey more\n> information. \"max exposure time the hardware can do\" might make it\n> easier to switch away from the exposure control. I don't know.\n> \n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::minAnalogueGain\n> > + * \\brief The sensor minimum analogue gain absolute value\n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::maxAnalogueGain\n> > + * \\brief The sensor maximum analogue gain absolute value\n> > + */\n> > +\n> > /**\n> > * \\class AgcMeanLuminance\n> > * \\brief A mean-based auto-exposure algorithm\n> > @@ -314,17 +350,21 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)\n> > \n> > /**\n> > * \\brief Configure the exposure mode helpers\n> > - * \\param[in] lineDuration The sensor line length\n> > + * \\param[in] config The sensor configuration\n> > * \\param[in] sensorHelper The sensor helper\n> > *\n> > - * This function configures the exposure mode helpers so they can correctly\n> > + * This function configures the exposure mode helpers by providing them the\n> > + * sensor configuration parameters and the sensor helper, so they can correctly\n> > * take quantization effects into account.\n> > */\n> > -void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> > +void AgcMeanLuminance::configure(const AgcSensorConfiguration &config,\n> > const CameraSensorHelper *sensorHelper)\n> > {\n> > for (auto &[id, helper] : exposureModeHelpers_)\n> > - helper->configure(lineDuration, sensorHelper);\n> > + helper->configure(config.lineDuration,\n> > + config.minExposureTime, config.maxExposureTime,\n> > + config.minAnalogueGain, config.maxAnalogueGain,\n> > + sensorHelper);\n> \n> Tying the information to the sensor helper would make the sensor helper\n> mandatory, but would simplify this.\n> \n> > }\n> > \n> > /**\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > index d7ec548e3e585e28fd03b956c8de7eaac5dc6e99..5ab75a3ac6c550a655f4bb7c8c30419ad8249183 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > @@ -42,7 +42,16 @@ public:\n> > double yTarget;\n> > };\n> > \n> > - void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);\n> > + struct AgcSensorConfiguration {\n> > + utils::Duration lineDuration;\n> > + utils::Duration minExposureTime;\n> > + utils::Duration maxExposureTime;\n> > + double minAnalogueGain;\n> > + double maxAnalogueGain;\n> > + };\n> > +\n> > + void configure(const AgcSensorConfiguration &config,\n> > + const CameraSensorHelper *sensorHelper);\n> > int parseTuningData(const YamlObject &tuningData);\n> > \n> > void setExposureCompensation(double gain)\n> > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> > index 29e316d9d09181cd65cb08f26babb1a680bee62a..557e14230ba4e0f120653bc09ea23a29a54cb4c8 100644\n> > --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> > @@ -82,22 +82,32 @@ ExposureModeHelper::ExposureModeHelper(const Span > /**\n> > * \\brief Configure sensor details\n> > * \\param[in] lineDuration The current line length of the sensor\n> > + * \\param[in] minExposureTime The minimum exposure time supported\n> > + * \\param[in] maxExposureTime The maximum exposure time supported\n> > + * \\param[in] minGain The minimum analogue gain supported\n> > + * \\param[in] maxGain The maximum analogue gain supported\n> > * \\param[in] sensorHelper The sensor helper\n> > *\n> > - * This function sets the line length and sensor helper. These are used in\n> > - * splitExposure() to take the quantization of the exposure and gain into\n> > - * account.\n> > + * This function initializes the exposure helper settings using the sensor\n> > + * configuration parameters.\n> > *\n> > - * When this has not been called, it is assumed that exposure is in micro second\n> > - * granularity and gain has no quantization at all.\n> > + * The sensor parameters' min and max limits are used in splitExposure() to take\n> > + * the quantization of the exposure and gain into account.\n> \n> This sentence now changed it's meaning as the min/max limits can't be\n> used to take quantization into account... but I agree that without the\n> quantization context in mind, this sentence came out of the blue.\n\n\nDo we need to keep a quantized number of lines associated with its'\nutils::Duration? if so I could further abstract Quantised to support\nDuration types as well as floats.\n\n\n\n> \n> > *\n> > * ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the caller\n> > * has to ensure that sensorHelper is valid until the next call to configure().\n> > */\n> > void ExposureModeHelper::configure(utils::Duration lineDuration,\n> > + utils::Duration minExposureTime,\n> > + utils::Duration maxExposureTime,\n\nThis is why I'd like to introduce \n\nhttps://git.uk.ideasonboard.com/kbingham/libcamera/commit/9e85cd913213e16ea20c614003ce421ab323b9ac\n\n> > + double minGain, double maxGain,\n\n\nSo this is instead:\n\n\tconfigure(utils::Duration lineDuration,\n\t\t Bounds exposureLimits,\n\t Bounds gainLimits)\n\nIs the only reason we pass lineDuration around because the Agc has to do\nquantization based on it ?\n\nI think that's probably something that should also get deferred into the\nCameraSensorHelper so that all control of the sensor calculations are\ncommon.\n\n\n> > const CameraSensorHelper *sensorHelper)\n> > {\n> > lineDuration_ = lineDuration;\n> > + minExposureTime_ = minExposureTime;\n> > + maxExposureTime_ = maxExposureTime;\n> > + minGain_ = minGain;\n> > + maxGain_ = maxGain;\n> \n> This now sets the same members as setLimits() so, setLimits() can't\n> internally clamp to the hardware limits (it never did). When we move the\n> hardware limits into the sensor helper we'd additionally gain that\n> ability.\n> \n> Best regards,\n> Stefan\n> \n> > sensorHelper_ = sensorHelper;\n> > }\n> > \n> > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> > index 968192ddc5af768ae0de58aca6c7230c7b3bd507..bb0a692c99b3128a2ee3f15304f125e073721536 100644\n> > --- a/src/ipa/libipa/exposure_mode_helper.h\n> > +++ b/src/ipa/libipa/exposure_mode_helper.h\n> > @@ -26,7 +26,9 @@ public:\n> > ExposureModeHelper(const Span> stages);\n> > ~ExposureModeHelper() = default;\n> > \n> > - void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);\n> > + void configure(utils::Duration lineLength, utils::Duration minExposureTime,\n> > + utils::Duration maxExposureTime, double minGain, double maxGain,\n> > + const CameraSensorHelper *sensorHelper);\n> > void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> > double minGain, double maxGain);\n> > \n> > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp\n> > index f60fddac3f04fd6f09dc782e929ff1593758c29b..ccf72cc9e8c33b1a516322c2445efa1684b7f751 100644\n> > --- a/src/ipa/mali-c55/algorithms/agc.cpp\n> > +++ b/src/ipa/mali-c55/algorithms/agc.cpp\n> > @@ -169,12 +169,16 @@ int Agc::configure(IPAContext &context,\n> > context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> > \n> > - /* \\todo Run this again when FrameDurationLimits is passed in */\n> > - setLimits(context.configuration.agc.minShutterSpeed,\n> > - context.configuration.agc.maxShutterSpeed,\n> > - context.configuration.agc.minAnalogueGain,\n> > - context.configuration.agc.maxAnalogueGain,\n> > - {});\n> > + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> > + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> > + sensorConfig.minExposureTime = context.configuration.agc.minShutterSpeed;\n> > + sensorConfig.maxExposureTime = context.configuration.agc.maxShutterSpeed;\n> > + sensorConfig.minAnalogueGain = context.configuration.agc.minAnalogueGain;\n> > + sensorConfig.maxAnalogueGain = context.configuration.agc.maxAnalogueGain;\n> > +\n> > + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> > +\n> > + /* \\todo Update AGC limits when FrameDurationLimits is passed in */\n> > \n> > resetFrameCount();\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..8c07e6a6eac091d949ad71ee967c893897313cc8 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -200,13 +200,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;\n> > context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;\n> > \n> > - AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,\n> > - context.camHelper.get());\n> > -\n> > - setLimits(context.configuration.sensor.minExposureTime,\n> > - context.configuration.sensor.maxExposureTime,\n> > - context.configuration.sensor.minAnalogueGain,\n> > - context.configuration.sensor.maxAnalogueGain, {});\n> > + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> > + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> > + sensorConfig.minExposureTime = context.configuration.sensor.minExposureTime;\n> > + sensorConfig.maxExposureTime = context.configuration.sensor.maxExposureTime;\n> > + sensorConfig.minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> > + sensorConfig.maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;\n> > +\n> > + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> > \n> > context.activeState.agc.automatic.yTarget = effectiveYTarget();\n> > \n> > \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 C0689BDE4C\n\tfor ;\n\tWed, 5 Nov 2025 08:57:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0904B60A7B;\n\tWed, 5 Nov 2025 09:57:31 +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 0AD25609D8\n\tfor ;\n\tWed, 5 Nov 2025 09:57:29 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 867DBC9;\n\tWed, 5 Nov 2025 09:55:34 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"sV444WNE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762332934;\n\tbh=GL6V4yJw5xz9IOjQ7csZFrcZ4sKpiIR+Q17uNbwdL4g=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=sV444WNEbf0alh2JActFnOuEtXBnhYlOqhQWeYdJUJVG0DyVuuK0kmpB2izKsOvJL\n\tlPee9Q8+i2en8aWRTP21Goo776fS7l1Q1Z4mMvTpeMKH8T51rWElMjyXTwYcDrK091\n\tLPkX0OwCBvDon8ZLhA8IvE7AddDG18YZygiNmvmw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176232858261.231750.6488373674243924560@localhost>","References":"<20251028-exposure-limits-v2-0-a8b5a318323e@ideasonboard.com>\n\t<20251028-exposure-limits-v2-4-a8b5a318323e@ideasonboard.com>\n\t<176232858261.231750.6488373674243924560@localhost>","Subject":"Re: [PATCH v2 04/10] ipa: libipa: agc: Make Agc::configure() set\n\tlimits","From":"Kieran Bingham ","Cc":"Jacopo Mondi ","To":"Jacopo Mondi , Niklas =?utf-8?b?U8O2?=\n\t=?utf-8?q?derlund?= , Robert Mader\n\t, \n\tStefan Klug , \n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 05 Nov 2025 08:57:25 +0000","Message-ID":"<176233304563.3742839.8261569944215394507@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":36696,"web_url":"https://patchwork.libcamera.org/comment/36696/","msgid":"<176233338573.3742839.15394843069742329294@ping.linuxembedded.co.uk>","date":"2025-11-05T09:03:05","subject":"Re: [PATCH v2 04/10] ipa: libipa: agc: Make Agc::configure() set\n\tlimits","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-11-05 07:43:02)\n> Hi Jacopo,\n> \n> Thank you for the patch.\n> \n> Quoting Jacopo Mondi (2025-10-28 10:31:50)\n> > The AgcMeanLuminance algorithm interface has a 'configure()' and\n> > 'setLimits()' API. configure() doesn't actually do much if not\n> > initialzing a few variables, and the Mali and IPU3 IPAs do not\n> > even call it. Configuring the AGC requires calling setLimits() at\n> > configure() time and at run time.\n> \n> Oh I just realized, that my internal policy is to keep the libipa\n> interface backwards compatible so there is no need to update to update\n> other pipeline handlers (That's why the others didn't call configure()).\n> Maybe I should revisit this policy... but then any change get's even\n> harder to test...\n> \n> > \n> > In order to prepare to differentiate between configure-time settings\n> > of the Agc and run-time handling of dynamic parameters such as the\n> > frame duration limits, make the configure() operation initialize\n> > the Agc limits.\n> \n> Yes, that makes sense.\n> \n> > \n> > Update all IPAs Agc implementation deriving from AgcMeanLuminance.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/ipu3/algorithms/agc.cpp | 14 +++++++---\n> > src/ipa/libipa/agc_mean_luminance.cpp | 48 ++++++++++++++++++++++++++++++---\n> > src/ipa/libipa/agc_mean_luminance.h | 11 +++++++-\n> > src/ipa/libipa/exposure_mode_helper.cpp | 20 ++++++++++----\n> > src/ipa/libipa/exposure_mode_helper.h | 4 ++-\n> > src/ipa/mali-c55/algorithms/agc.cpp | 16 ++++++-----\n> > src/ipa/rkisp1/algorithms/agc.cpp | 15 ++++++-----\n> > 7 files changed, 101 insertions(+), 27 deletions(-)\n> > \n> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > index b0d89541da8550bd50472779da7fa8e33b96e2f0..e303d724ab165bac03ac6bb9d3891a76049cff47 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > @@ -115,9 +115,17 @@ int Agc::configure(IPAContext &context,\n> > context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> > \n> > - /* \\todo Run this again when FrameDurationLimits is passed in */\n> > - setLimits(minExposureTime_, maxExposureTime_, minAnalogueGain_,\n> > - maxAnalogueGain_, {});\n> > + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> > + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> > + sensorConfig.minExposureTime = minExposureTime_;\n> > + sensorConfig.maxExposureTime = maxExposureTime_;\n> > + sensorConfig.minAnalogueGain = minAnalogueGain_;\n> > + sensorConfig.maxAnalogueGain = maxAnalogueGain_;\n> > +\n> > + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> > +\n> > + /* \\todo Update AGC limits when FrameDurationLimits is passed in */\n> > +\n> > resetFrameCount();\n> > \n> > return 0;\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > index 64f36bd75dd213671b5a2e6810245096ed001f21..9886864fc42514040eadb4c0b424796df1f1b6e1 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > @@ -95,6 +95,42 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > * \\brief The luminance target for the constraint\n> > */\n> > \n> > +/**\n> > + * \\struct AgcMeanLuminance::AgcSensorConfiguration\n> > + * \\brief The sensor configuration parameters\n> > + *\n> > + * This structure collects the sensor configuration parameters which need\n> > + * to be provided to the AGC algorithm at configure() time.\n> > + *\n> > + * Each time configure() is called the sensor configuration need to be updated\n> > + * with new parameters.\n\nThis looks like what I was working towards in \n\n AgcMeanLuminance tidy up \n https://git.uk.ideasonboard.com/kbingham/libcamera/commit/100cec322815863f9a5168d361a8118e7287e7a9\n\nI think we should have AgcMeanLuminance able to declare what goes into\nthe IPA Context with a struct for each of the\n Configuration \n ActiveState \n FrameContext \n\nThen users of the AgcMeanLuminance should declare *these* instead of all\nthe current places where we have many different instantiations of the\nsame data.\n\n--\nKieran\n\n\n> \n> Reading the series I (again) realized how hard it is to follow the\n> dependencies/constraints in the system.\n> \n> I wonder if this should be moved to the sensor helper? In the end it is\n> not a configuration of the AGC but a property of the sensor. Aside from\n> a hardcoded 60ms in IPU3 I don't see that we impose additional AGC\n> specific limits. I think Kieran had similar thoughts.\n> \n> If we leave it in here, we could call it AgcRegulationLimits, then\n> lineDuration doesn't fit the scheme... so maybe just AgcConfiguration?\n> \n> But I think fetching this info from the sensor helper and configuring\n> that one would be best.\n> \n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::lineDuration\n> > + * \\brief The line duration in microseconds\n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::minExposureTime\n> > + * \\brief The sensor minimum exposure time in microseconds\n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::maxExposureTime\n> > + * \\brief The sensor maximum exposure time in microseconds\n> \n> \n> As a first time IPA implementer I would fill this with the maximum of\n> V4L2_CID_EXPOSURE converted to microseconds. Which is clearly the wrong\n> thing to do. Still I wonder if this should be made extra clear but I\n> have difficulties finding the right words.\n> \n> Maybe \"maximum achievable exposure time\" - doesn't actually convey more\n> information. \"max exposure time the hardware can do\" might make it\n> easier to switch away from the exposure control. I don't know.\n> \n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::minAnalogueGain\n> > + * \\brief The sensor minimum analogue gain absolute value\n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::maxAnalogueGain\n> > + * \\brief The sensor maximum analogue gain absolute value\n> > + */\n> > +\n> > /**\n> > * \\class AgcMeanLuminance\n> > * \\brief A mean-based auto-exposure algorithm\n> > @@ -314,17 +350,21 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)\n> > \n> > /**\n> > * \\brief Configure the exposure mode helpers\n> > - * \\param[in] lineDuration The sensor line length\n> > + * \\param[in] config The sensor configuration\n> > * \\param[in] sensorHelper The sensor helper\n> > *\n> > - * This function configures the exposure mode helpers so they can correctly\n> > + * This function configures the exposure mode helpers by providing them the\n> > + * sensor configuration parameters and the sensor helper, so they can correctly\n> > * take quantization effects into account.\n> > */\n> > -void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> > +void AgcMeanLuminance::configure(const AgcSensorConfiguration &config,\n> > const CameraSensorHelper *sensorHelper)\n> > {\n> > for (auto &[id, helper] : exposureModeHelpers_)\n> > - helper->configure(lineDuration, sensorHelper);\n> > + helper->configure(config.lineDuration,\n> > + config.minExposureTime, config.maxExposureTime,\n> > + config.minAnalogueGain, config.maxAnalogueGain,\n> > + sensorHelper);\n> \n> Tying the information to the sensor helper would make the sensor helper\n> mandatory, but would simplify this.\n> \n> > }\n> > \n> > /**\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > index d7ec548e3e585e28fd03b956c8de7eaac5dc6e99..5ab75a3ac6c550a655f4bb7c8c30419ad8249183 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > @@ -42,7 +42,16 @@ public:\n> > double yTarget;\n> > };\n> > \n> > - void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);\n> > + struct AgcSensorConfiguration {\n> > + utils::Duration lineDuration;\n> > + utils::Duration minExposureTime;\n> > + utils::Duration maxExposureTime;\n> > + double minAnalogueGain;\n> > + double maxAnalogueGain;\n> > + };\n> > +\n> > + void configure(const AgcSensorConfiguration &config,\n> > + const CameraSensorHelper *sensorHelper);\n> > int parseTuningData(const YamlObject &tuningData);\n> > \n> > void setExposureCompensation(double gain)\n> > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> > index 29e316d9d09181cd65cb08f26babb1a680bee62a..557e14230ba4e0f120653bc09ea23a29a54cb4c8 100644\n> > --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> > @@ -82,22 +82,32 @@ ExposureModeHelper::ExposureModeHelper(const Span > /**\n> > * \\brief Configure sensor details\n> > * \\param[in] lineDuration The current line length of the sensor\n> > + * \\param[in] minExposureTime The minimum exposure time supported\n> > + * \\param[in] maxExposureTime The maximum exposure time supported\n> > + * \\param[in] minGain The minimum analogue gain supported\n> > + * \\param[in] maxGain The maximum analogue gain supported\n> > * \\param[in] sensorHelper The sensor helper\n> > *\n> > - * This function sets the line length and sensor helper. These are used in\n> > - * splitExposure() to take the quantization of the exposure and gain into\n> > - * account.\n> > + * This function initializes the exposure helper settings using the sensor\n> > + * configuration parameters.\n> > *\n> > - * When this has not been called, it is assumed that exposure is in micro second\n> > - * granularity and gain has no quantization at all.\n> > + * The sensor parameters' min and max limits are used in splitExposure() to take\n> > + * the quantization of the exposure and gain into account.\n> \n> This sentence now changed it's meaning as the min/max limits can't be\n> used to take quantization into account... but I agree that without the\n> quantization context in mind, this sentence came out of the blue.\n> \n> > *\n> > * ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the caller\n> > * has to ensure that sensorHelper is valid until the next call to configure().\n> > */\n> > void ExposureModeHelper::configure(utils::Duration lineDuration,\n> > + utils::Duration minExposureTime,\n> > + utils::Duration maxExposureTime,\n> > + double minGain, double maxGain,\n> > const CameraSensorHelper *sensorHelper)\n> > {\n> > lineDuration_ = lineDuration;\n> > + minExposureTime_ = minExposureTime;\n> > + maxExposureTime_ = maxExposureTime;\n> > + minGain_ = minGain;\n> > + maxGain_ = maxGain;\n> \n> This now sets the same members as setLimits() so, setLimits() can't\n> internally clamp to the hardware limits (it never did). When we move the\n> hardware limits into the sensor helper we'd additionally gain that\n> ability.\n> \n> Best regards,\n> Stefan\n> \n> > sensorHelper_ = sensorHelper;\n> > }\n> > \n> > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> > index 968192ddc5af768ae0de58aca6c7230c7b3bd507..bb0a692c99b3128a2ee3f15304f125e073721536 100644\n> > --- a/src/ipa/libipa/exposure_mode_helper.h\n> > +++ b/src/ipa/libipa/exposure_mode_helper.h\n> > @@ -26,7 +26,9 @@ public:\n> > ExposureModeHelper(const Span> stages);\n> > ~ExposureModeHelper() = default;\n> > \n> > - void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);\n> > + void configure(utils::Duration lineLength, utils::Duration minExposureTime,\n> > + utils::Duration maxExposureTime, double minGain, double maxGain,\n> > + const CameraSensorHelper *sensorHelper);\n> > void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> > double minGain, double maxGain);\n> > \n> > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp\n> > index f60fddac3f04fd6f09dc782e929ff1593758c29b..ccf72cc9e8c33b1a516322c2445efa1684b7f751 100644\n> > --- a/src/ipa/mali-c55/algorithms/agc.cpp\n> > +++ b/src/ipa/mali-c55/algorithms/agc.cpp\n> > @@ -169,12 +169,16 @@ int Agc::configure(IPAContext &context,\n> > context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> > \n> > - /* \\todo Run this again when FrameDurationLimits is passed in */\n> > - setLimits(context.configuration.agc.minShutterSpeed,\n> > - context.configuration.agc.maxShutterSpeed,\n> > - context.configuration.agc.minAnalogueGain,\n> > - context.configuration.agc.maxAnalogueGain,\n> > - {});\n> > + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> > + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> > + sensorConfig.minExposureTime = context.configuration.agc.minShutterSpeed;\n> > + sensorConfig.maxExposureTime = context.configuration.agc.maxShutterSpeed;\n> > + sensorConfig.minAnalogueGain = context.configuration.agc.minAnalogueGain;\n> > + sensorConfig.maxAnalogueGain = context.configuration.agc.maxAnalogueGain;\n> > +\n> > + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> > +\n> > + /* \\todo Update AGC limits when FrameDurationLimits is passed in */\n> > \n> > resetFrameCount();\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..8c07e6a6eac091d949ad71ee967c893897313cc8 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -200,13 +200,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;\n> > context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;\n> > \n> > - AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,\n> > - context.camHelper.get());\n> > -\n> > - setLimits(context.configuration.sensor.minExposureTime,\n> > - context.configuration.sensor.maxExposureTime,\n> > - context.configuration.sensor.minAnalogueGain,\n> > - context.configuration.sensor.maxAnalogueGain, {});\n> > + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> > + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> > + sensorConfig.minExposureTime = context.configuration.sensor.minExposureTime;\n> > + sensorConfig.maxExposureTime = context.configuration.sensor.maxExposureTime;\n> > + sensorConfig.minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> > + sensorConfig.maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;\n> > +\n> > + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> > \n> > context.activeState.agc.automatic.yTarget = effectiveYTarget();\n> > \n> > \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 0C494C3241\n\tfor ;\n\tWed, 5 Nov 2025 09:03:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D80D60A80;\n\tWed, 5 Nov 2025 10:03:11 +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 7ADC0608CF\n\tfor ;\n\tWed, 5 Nov 2025 10:03:09 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3C004C6F;\n\tWed, 5 Nov 2025 10:01:15 +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=\"UY++10JY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762333275;\n\tbh=jd5X+pu2nY9OVkeVxENNLkb57u92yUGZ1dDjXKSD1Ns=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=UY++10JYu60K5be+DkPq6n1ryhdHpxqJ7GbBS0jlLQyepYeK/LyLRzGJeBNTASRCi\n\tu+geJOcxO3cQerdoiLdL7PLrMKYwGj2EA3WKOwgDExnEm/mjx8ZQVnTAWst1w1yG0q\n\to0aO2Y9tZcWbaRLhFicFTZhmnQxFA6OOw0dXunzU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176232858261.231750.6488373674243924560@localhost>","References":"<20251028-exposure-limits-v2-0-a8b5a318323e@ideasonboard.com>\n\t<20251028-exposure-limits-v2-4-a8b5a318323e@ideasonboard.com>\n\t<176232858261.231750.6488373674243924560@localhost>","Subject":"Re: [PATCH v2 04/10] ipa: libipa: agc: Make Agc::configure() set\n\tlimits","From":"Kieran Bingham ","Cc":"Jacopo Mondi ","To":"Jacopo Mondi , Niklas =?utf-8?b?U8O2?=\n\t=?utf-8?q?derlund?= , Robert Mader\n\t, \n\tStefan Klug , \n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 05 Nov 2025 09:03:05 +0000","Message-ID":"<176233338573.3742839.15394843069742329294@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":36721,"web_url":"https://patchwork.libcamera.org/comment/36721/","msgid":"<2xvhwb3m2pwvfd523ympt4tarxb5pp3l7n3pz5drarspadvlxb@sp4yedbizjnv>","date":"2025-11-06T08:05:05","subject":"Re: [PATCH v2 04/10] ipa: libipa: agc: Make Agc::configure() set\n\tlimits","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Wed, Nov 05, 2025 at 08:43:02AM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> Quoting Jacopo Mondi (2025-10-28 10:31:50)\n> > The AgcMeanLuminance algorithm interface has a 'configure()' and\n> > 'setLimits()' API. configure() doesn't actually do much if not\n> > initialzing a few variables, and the Mali and IPU3 IPAs do not\n> > even call it. Configuring the AGC requires calling setLimits() at\n> > configure() time and at run time.\n>\n> Oh I just realized, that my internal policy is to keep the libipa\n> interface backwards compatible so there is no need to update to update\n> other pipeline handlers (That's why the others didn't call configure()).\n> Maybe I should revisit this policy... but then any change get's even\n> harder to test...\n>\n\ngnn, I would do the exact opposite :)\n\nInternal API much like the Linux kernel API are unstable by definition\nand the only way to ensure we can advance them is to modify them in\nall places where they are in use.\n\nThe alternative is to create a sub-optimal API in order to keep it\nbackward compatible. This quickly becomes a nightmare to maintain.\n\nYou're right, it will require to test changes on all users, at the\nminimum compile time changes (already done by the CI) but also\nfunctionally.\n\nI feel we'll soon need maintainers for each platform we support in\nlibipa responsible for keeping the platform working as expected.\n\n> >\n> > In order to prepare to differentiate between configure-time settings\n> > of the Agc and run-time handling of dynamic parameters such as the\n> > frame duration limits, make the configure() operation initialize\n> > the Agc limits.\n>\n> Yes, that makes sense.\n>\n> >\n> > Update all IPAs Agc implementation deriving from AgcMeanLuminance.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/ipa/ipu3/algorithms/agc.cpp | 14 +++++++---\n> > src/ipa/libipa/agc_mean_luminance.cpp | 48 ++++++++++++++++++++++++++++++---\n> > src/ipa/libipa/agc_mean_luminance.h | 11 +++++++-\n> > src/ipa/libipa/exposure_mode_helper.cpp | 20 ++++++++++----\n> > src/ipa/libipa/exposure_mode_helper.h | 4 ++-\n> > src/ipa/mali-c55/algorithms/agc.cpp | 16 ++++++-----\n> > src/ipa/rkisp1/algorithms/agc.cpp | 15 ++++++-----\n> > 7 files changed, 101 insertions(+), 27 deletions(-)\n> >\n> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > index b0d89541da8550bd50472779da7fa8e33b96e2f0..e303d724ab165bac03ac6bb9d3891a76049cff47 100644\n> > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > @@ -115,9 +115,17 @@ int Agc::configure(IPAContext &context,\n> > context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> >\n> > - /* \\todo Run this again when FrameDurationLimits is passed in */\n> > - setLimits(minExposureTime_, maxExposureTime_, minAnalogueGain_,\n> > - maxAnalogueGain_, {});\n> > + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> > + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> > + sensorConfig.minExposureTime = minExposureTime_;\n> > + sensorConfig.maxExposureTime = maxExposureTime_;\n> > + sensorConfig.minAnalogueGain = minAnalogueGain_;\n> > + sensorConfig.maxAnalogueGain = maxAnalogueGain_;\n> > +\n> > + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> > +\n> > + /* \\todo Update AGC limits when FrameDurationLimits is passed in */\n> > +\n> > resetFrameCount();\n> >\n> > return 0;\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > index 64f36bd75dd213671b5a2e6810245096ed001f21..9886864fc42514040eadb4c0b424796df1f1b6e1 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > @@ -95,6 +95,42 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > * \\brief The luminance target for the constraint\n> > */\n> >\n> > +/**\n> > + * \\struct AgcMeanLuminance::AgcSensorConfiguration\n> > + * \\brief The sensor configuration parameters\n> > + *\n> > + * This structure collects the sensor configuration parameters which need\n> > + * to be provided to the AGC algorithm at configure() time.\n> > + *\n> > + * Each time configure() is called the sensor configuration need to be updated\n> > + * with new parameters.\n>\n> Reading the series I (again) realized how hard it is to follow the\n> dependencies/constraints in the system.\n>\n> I wonder if this should be moved to the sensor helper? In the end it is\n> not a configuration of the AGC but a property of the sensor. Aside from\n> a hardcoded 60ms in IPU3 I don't see that we impose additional AGC\n> specific limits. I think Kieran had similar thoughts.\n>\n> If we leave it in here, we could call it AgcRegulationLimits, then\n> lineDuration doesn't fit the scheme... so maybe just AgcConfiguration?\n>\n> But I think fetching this info from the sensor helper and configuring\n> that one would be best.\n>\n\nI'll reply to Kieran's email as well, but yes, this is where we want\nto go and when Kieran showed me his branch I tried to design this so\nthat it can be easily replaceble by a centralized sensor helper.\n\n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::lineDuration\n> > + * \\brief The line duration in microseconds\n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::minExposureTime\n> > + * \\brief The sensor minimum exposure time in microseconds\n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::maxExposureTime\n> > + * \\brief The sensor maximum exposure time in microseconds\n>\n>\n> As a first time IPA implementer I would fill this with the maximum of\n> V4L2_CID_EXPOSURE converted to microseconds. Which is clearly the wrong\n> thing to do. Still I wonder if this should be made extra clear but I\n> have difficulties finding the right words.\n>\n> Maybe \"maximum achievable exposure time\" - doesn't actually convey more\n> information. \"max exposure time the hardware can do\" might make it\n> easier to switch away from the exposure control. I don't know.\n>\n\nThis field will likely go in next version.\n\nIf the max exposure is always \"max duration - margin\" then it doesn't\nmake sense to pass it in.\n\n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::minAnalogueGain\n> > + * \\brief The sensor minimum analogue gain absolute value\n> > + */\n> > +\n> > +/**\n> > + * \\var AgcMeanLuminance::AgcSensorConfiguration::maxAnalogueGain\n> > + * \\brief The sensor maximum analogue gain absolute value\n> > + */\n> > +\n> > /**\n> > * \\class AgcMeanLuminance\n> > * \\brief A mean-based auto-exposure algorithm\n> > @@ -314,17 +350,21 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)\n> >\n> > /**\n> > * \\brief Configure the exposure mode helpers\n> > - * \\param[in] lineDuration The sensor line length\n> > + * \\param[in] config The sensor configuration\n> > * \\param[in] sensorHelper The sensor helper\n> > *\n> > - * This function configures the exposure mode helpers so they can correctly\n> > + * This function configures the exposure mode helpers by providing them the\n> > + * sensor configuration parameters and the sensor helper, so they can correctly\n> > * take quantization effects into account.\n> > */\n> > -void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> > +void AgcMeanLuminance::configure(const AgcSensorConfiguration &config,\n> > const CameraSensorHelper *sensorHelper)\n> > {\n> > for (auto &[id, helper] : exposureModeHelpers_)\n> > - helper->configure(lineDuration, sensorHelper);\n> > + helper->configure(config.lineDuration,\n> > + config.minExposureTime, config.maxExposureTime,\n> > + config.minAnalogueGain, config.maxAnalogueGain,\n> > + sensorHelper);\n>\n> Tying the information to the sensor helper would make the sensor helper\n> mandatory, but would simplify this.\n>\n\nI think we should make sensor helpers mandatory, yes.\n\nI agree we should get those information from the unified sensor\nhelper, but I don't think we should delay fixing this bug (which is\nnot really a bug, it's a design deficiency) by requiring to introduce a\nunified sensor helper first.\n\nWhat I'm trying to do here is to fix this in a way that will make it\n\"easy\" to get the same information that the IPA now feed to the AGC\nfrom a sensor helper.\n\n> > }\n> >\n> > /**\n> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > index d7ec548e3e585e28fd03b956c8de7eaac5dc6e99..5ab75a3ac6c550a655f4bb7c8c30419ad8249183 100644\n> > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > @@ -42,7 +42,16 @@ public:\n> > double yTarget;\n> > };\n> >\n> > - void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);\n> > + struct AgcSensorConfiguration {\n> > + utils::Duration lineDuration;\n> > + utils::Duration minExposureTime;\n> > + utils::Duration maxExposureTime;\n> > + double minAnalogueGain;\n> > + double maxAnalogueGain;\n> > + };\n> > +\n> > + void configure(const AgcSensorConfiguration &config,\n> > + const CameraSensorHelper *sensorHelper);\n> > int parseTuningData(const YamlObject &tuningData);\n> >\n> > void setExposureCompensation(double gain)\n> > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> > index 29e316d9d09181cd65cb08f26babb1a680bee62a..557e14230ba4e0f120653bc09ea23a29a54cb4c8 100644\n> > --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> > @@ -82,22 +82,32 @@ ExposureModeHelper::ExposureModeHelper(const Span > /**\n> > * \\brief Configure sensor details\n> > * \\param[in] lineDuration The current line length of the sensor\n> > + * \\param[in] minExposureTime The minimum exposure time supported\n> > + * \\param[in] maxExposureTime The maximum exposure time supported\n> > + * \\param[in] minGain The minimum analogue gain supported\n> > + * \\param[in] maxGain The maximum analogue gain supported\n> > * \\param[in] sensorHelper The sensor helper\n> > *\n> > - * This function sets the line length and sensor helper. These are used in\n> > - * splitExposure() to take the quantization of the exposure and gain into\n> > - * account.\n> > + * This function initializes the exposure helper settings using the sensor\n> > + * configuration parameters.\n> > *\n> > - * When this has not been called, it is assumed that exposure is in micro second\n> > - * granularity and gain has no quantization at all.\n> > + * The sensor parameters' min and max limits are used in splitExposure() to take\n> > + * the quantization of the exposure and gain into account.\n>\n> This sentence now changed it's meaning as the min/max limits can't be\n> used to take quantization into account... but I agree that without the\n> quantization context in mind, this sentence came out of the blue.\n>\n\nHow should I re-phrase it ?\n\n> > *\n> > * ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the caller\n> > * has to ensure that sensorHelper is valid until the next call to configure().\n> > */\n> > void ExposureModeHelper::configure(utils::Duration lineDuration,\n> > + utils::Duration minExposureTime,\n> > + utils::Duration maxExposureTime,\n> > + double minGain, double maxGain,\n> > const CameraSensorHelper *sensorHelper)\n> > {\n> > lineDuration_ = lineDuration;\n> > + minExposureTime_ = minExposureTime;\n> > + maxExposureTime_ = maxExposureTime;\n> > + minGain_ = minGain;\n> > + maxGain_ = maxGain;\n>\n> This now sets the same members as setLimits() so, setLimits() can't\n> internally clamp to the hardware limits (it never did). When we move the\n> hardware limits into the sensor helper we'd additionally gain that\n> ability.\n\nIn the next patches configure() and setLimits() will use the same\ninternal routine, and we can centralize hw constraints handling there.\n\nThanks\n j\n>\n> Best regards,\n> Stefan\n>\n> > sensorHelper_ = sensorHelper;\n> > }\n> >\n> > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> > index 968192ddc5af768ae0de58aca6c7230c7b3bd507..bb0a692c99b3128a2ee3f15304f125e073721536 100644\n> > --- a/src/ipa/libipa/exposure_mode_helper.h\n> > +++ b/src/ipa/libipa/exposure_mode_helper.h\n> > @@ -26,7 +26,9 @@ public:\n> > ExposureModeHelper(const Span> stages);\n> > ~ExposureModeHelper() = default;\n> >\n> > - void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);\n> > + void configure(utils::Duration lineLength, utils::Duration minExposureTime,\n> > + utils::Duration maxExposureTime, double minGain, double maxGain,\n> > + const CameraSensorHelper *sensorHelper);\n> > void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> > double minGain, double maxGain);\n> >\n> > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp\n> > index f60fddac3f04fd6f09dc782e929ff1593758c29b..ccf72cc9e8c33b1a516322c2445efa1684b7f751 100644\n> > --- a/src/ipa/mali-c55/algorithms/agc.cpp\n> > +++ b/src/ipa/mali-c55/algorithms/agc.cpp\n> > @@ -169,12 +169,16 @@ int Agc::configure(IPAContext &context,\n> > context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> >\n> > - /* \\todo Run this again when FrameDurationLimits is passed in */\n> > - setLimits(context.configuration.agc.minShutterSpeed,\n> > - context.configuration.agc.maxShutterSpeed,\n> > - context.configuration.agc.minAnalogueGain,\n> > - context.configuration.agc.maxAnalogueGain,\n> > - {});\n> > + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> > + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> > + sensorConfig.minExposureTime = context.configuration.agc.minShutterSpeed;\n> > + sensorConfig.maxExposureTime = context.configuration.agc.maxShutterSpeed;\n> > + sensorConfig.minAnalogueGain = context.configuration.agc.minAnalogueGain;\n> > + sensorConfig.maxAnalogueGain = context.configuration.agc.maxAnalogueGain;\n> > +\n> > + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> > +\n> > + /* \\todo Update AGC limits when FrameDurationLimits is passed in */\n> >\n> > resetFrameCount();\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..8c07e6a6eac091d949ad71ee967c893897313cc8 100644\n> > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > @@ -200,13 +200,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;\n> > context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;\n> >\n> > - AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,\n> > - context.camHelper.get());\n> > -\n> > - setLimits(context.configuration.sensor.minExposureTime,\n> > - context.configuration.sensor.maxExposureTime,\n> > - context.configuration.sensor.minAnalogueGain,\n> > - context.configuration.sensor.maxAnalogueGain, {});\n> > + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> > + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> > + sensorConfig.minExposureTime = context.configuration.sensor.minExposureTime;\n> > + sensorConfig.maxExposureTime = context.configuration.sensor.maxExposureTime;\n> > + sensorConfig.minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> > + sensorConfig.maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;\n> > +\n> > + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> >\n> > context.activeState.agc.automatic.yTarget = effectiveYTarget();\n> >\n> >\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 13D4EC3241\n\tfor ;\n\tThu, 6 Nov 2025 08:05:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3DE2860A8B;\n\tThu, 6 Nov 2025 09:05:10 +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 645AB6096B\n\tfor ;\n\tThu, 6 Nov 2025 09:05:08 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CFC8922F;\n\tThu, 6 Nov 2025 09:03:12 +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=\"SKwVVRf+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762416192;\n\tbh=IbWejAoOACpogyHnTewAwAPn8jMwyBSJjGrxiBkb038=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SKwVVRf+YzNZKPoAFXn6NXa4ytjAmSUsOnHRoBBjWLYVcpBwc2DTLtARzWfix+wJx\n\tjEtzcvZZfqklXrSsaqK0ZMpsdP34LNJvoi5V+9gG1IhvbDTwdfe/p7tktepaAOHEY5\n\tx10+6WFRHKXMl84OFze45cWRwReaNeMyAxymjKMg=","Date":"Thu, 6 Nov 2025 09:05:05 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , Niklas =?utf-8?b?U8O2?=\n\t=?utf-8?q?derlund?= , Robert Mader\n\t, libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 04/10] ipa: libipa: agc: Make Agc::configure() set\n\tlimits","Message-ID":"<2xvhwb3m2pwvfd523ympt4tarxb5pp3l7n3pz5drarspadvlxb@sp4yedbizjnv>","References":"<20251028-exposure-limits-v2-0-a8b5a318323e@ideasonboard.com>\n\t<20251028-exposure-limits-v2-4-a8b5a318323e@ideasonboard.com>\n\t<176232858261.231750.6488373674243924560@localhost>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176232858261.231750.6488373674243924560@localhost>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36722,"web_url":"https://patchwork.libcamera.org/comment/36722/","msgid":"","date":"2025-11-06T08:20:09","subject":"Re: [PATCH v2 04/10] ipa: libipa: agc: Make Agc::configure() set\n\tlimits","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Wed, Nov 05, 2025 at 08:57:25AM +0000, Kieran Bingham wrote:\n> Quoting Stefan Klug (2025-11-05 07:43:02)\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > Quoting Jacopo Mondi (2025-10-28 10:31:50)\n> > > The AgcMeanLuminance algorithm interface has a 'configure()' and\n> > > 'setLimits()' API. configure() doesn't actually do much if not\n> > > initialzing a few variables, and the Mali and IPU3 IPAs do not\n> > > even call it. Configuring the AGC requires calling setLimits() at\n> > > configure() time and at run time.\n> >\n> > Oh I just realized, that my internal policy is to keep the libipa\n> > interface backwards compatible so there is no need to update to update\n> > other pipeline handlers (That's why the others didn't call configure()).\n> > Maybe I should revisit this policy... but then any change get's even\n> > harder to test...\n> >\n> > >\n> > > In order to prepare to differentiate between configure-time settings\n> > > of the Agc and run-time handling of dynamic parameters such as the\n> > > frame duration limits, make the configure() operation initialize\n> > > the Agc limits.\n> >\n> > Yes, that makes sense.\n> >\n> > >\n> > > Update all IPAs Agc implementation deriving from AgcMeanLuminance.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/ipa/ipu3/algorithms/agc.cpp | 14 +++++++---\n> > > src/ipa/libipa/agc_mean_luminance.cpp | 48 ++++++++++++++++++++++++++++++---\n> > > src/ipa/libipa/agc_mean_luminance.h | 11 +++++++-\n> > > src/ipa/libipa/exposure_mode_helper.cpp | 20 ++++++++++----\n> > > src/ipa/libipa/exposure_mode_helper.h | 4 ++-\n> > > src/ipa/mali-c55/algorithms/agc.cpp | 16 ++++++-----\n> > > src/ipa/rkisp1/algorithms/agc.cpp | 15 ++++++-----\n> > > 7 files changed, 101 insertions(+), 27 deletions(-)\n> > >\n> > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> > > index b0d89541da8550bd50472779da7fa8e33b96e2f0..e303d724ab165bac03ac6bb9d3891a76049cff47 100644\n> > > --- a/src/ipa/ipu3/algorithms/agc.cpp\n> > > +++ b/src/ipa/ipu3/algorithms/agc.cpp\n> > > @@ -115,9 +115,17 @@ int Agc::configure(IPAContext &context,\n> > > context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > > context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> > >\n> > > - /* \\todo Run this again when FrameDurationLimits is passed in */\n> > > - setLimits(minExposureTime_, maxExposureTime_, minAnalogueGain_,\n> > > - maxAnalogueGain_, {});\n> > > + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> > > + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> > > + sensorConfig.minExposureTime = minExposureTime_;\n> > > + sensorConfig.maxExposureTime = maxExposureTime_;\n> > > + sensorConfig.minAnalogueGain = minAnalogueGain_;\n> > > + sensorConfig.maxAnalogueGain = maxAnalogueGain_;\n> > > +\n> > > + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> > > +\n> > > + /* \\todo Update AGC limits when FrameDurationLimits is passed in */\n> > > +\n> > > resetFrameCount();\n> > >\n> > > return 0;\n> > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > index 64f36bd75dd213671b5a2e6810245096ed001f21..9886864fc42514040eadb4c0b424796df1f1b6e1 100644\n> > > --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> > > @@ -95,6 +95,42 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n> > > * \\brief The luminance target for the constraint\n> > > */\n> > >\n> > > +/**\n> > > + * \\struct AgcMeanLuminance::AgcSensorConfiguration\n> > > + * \\brief The sensor configuration parameters\n> > > + *\n> > > + * This structure collects the sensor configuration parameters which need\n> > > + * to be provided to the AGC algorithm at configure() time.\n> > > + *\n> > > + * Each time configure() is called the sensor configuration need to be updated\n> > > + * with new parameters.\n> >\n> > Reading the series I (again) realized how hard it is to follow the\n> > dependencies/constraints in the system.\n> >\n> > I wonder if this should be moved to the sensor helper? In the end it is\n> > not a configuration of the AGC but a property of the sensor. Aside from\n> > a hardcoded 60ms in IPU3 I don't see that we impose additional AGC\n> > specific limits. I think Kieran had similar thoughts.\n> >\n> > If we leave it in here, we could call it AgcRegulationLimits, then\n> > lineDuration doesn't fit the scheme... so maybe just AgcConfiguration?\n> >\n> > But I think fetching this info from the sensor helper and configuring\n> > that one would be best.\n>\n>\n> Yes, I think all tasks related to the CameraSensor should be in the\n> CameraSensorHelper - *not* replicated across algorithms and IPAs.\n> Especially in this topic where we're talking about how should we\n> configure and manage the limits of the CameraSensor - that should\n> definitely all be common in my view.\n>\n> ipa: Move libipa camera configuration to helpers\n> - https://git.uk.ideasonboard.com/kbingham/libcamera/commit/5c711d2a896d5a892ef61f263d623a440b58b33d\n>\n> I think CameraSensorHelper should manage the v4l2 conversions, and\n> provide an interface that an Agc can ask \"Hey what limits can I use\"\n>\n> Then the *common* CameraSensorHelper will report that based on any\n> manual control parsing.\n\nI agree.\n\nOnce you showed me your branch I tried to design this in a way that\nwill make it easier for the AgcMeanLuminance and ExposureModeHelper\nclasses to get the same information that are currently fed by the IPA\nmodule from a CameraSensorHelper instead.\n\nAs said in reply to Stefan please let me know if something goes in a\ndirection that will make it harder to get where we want to go, but I\ndon't this we should delay fixing the issue I'm trying to fix until we\ndon't have the CameraSensorHelper unification in place.\n\n>\n> I'd also like to introduce a new wrapper around min/max for this topic\n> because they're so intrinsically tied together and when I worked through\n> this I was really bothered with always having to move/manage them as a\n> pair :-)\n>\n> ipa: libipa: Provide a Bounds type to pass min/max\n> - https://git.uk.ideasonboard.com/kbingham/libcamera/commit/9e85cd913213e16ea20c614003ce421ab323b9ac\n>\n>\n>\n> Those commits are part of my slow progressing soft-libipa branch:\n> https://git.uk.ideasonboard.com/kbingham/libcamera/pulls/18\n>\n> where I'm working towards making it easier to integrate AgcMeanLuminance\n> in new IPA modules - and factor out all the repeated or common code that\n> keeps getting duplicated.\n\nI hope this series helps getting there\n\n>\n> --\n> Kieran\n>\n>\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var AgcMeanLuminance::AgcSensorConfiguration::lineDuration\n> > > + * \\brief The line duration in microseconds\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var AgcMeanLuminance::AgcSensorConfiguration::minExposureTime\n> > > + * \\brief The sensor minimum exposure time in microseconds\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var AgcMeanLuminance::AgcSensorConfiguration::maxExposureTime\n> > > + * \\brief The sensor maximum exposure time in microseconds\n> >\n> >\n> > As a first time IPA implementer I would fill this with the maximum of\n> > V4L2_CID_EXPOSURE converted to microseconds. Which is clearly the wrong\n> > thing to do. Still I wonder if this should be made extra clear but I\n> > have difficulties finding the right words.\n> >\n> > Maybe \"maximum achievable exposure time\" - doesn't actually convey more\n> > information. \"max exposure time the hardware can do\" might make it\n> > easier to switch away from the exposure control. I don't know.\n> >\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var AgcMeanLuminance::AgcSensorConfiguration::minAnalogueGain\n> > > + * \\brief The sensor minimum analogue gain absolute value\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var AgcMeanLuminance::AgcSensorConfiguration::maxAnalogueGain\n> > > + * \\brief The sensor maximum analogue gain absolute value\n> > > + */\n> > > +\n> > > /**\n> > > * \\class AgcMeanLuminance\n> > > * \\brief A mean-based auto-exposure algorithm\n> > > @@ -314,17 +350,21 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)\n> > >\n> > > /**\n> > > * \\brief Configure the exposure mode helpers\n> > > - * \\param[in] lineDuration The sensor line length\n> > > + * \\param[in] config The sensor configuration\n> > > * \\param[in] sensorHelper The sensor helper\n> > > *\n> > > - * This function configures the exposure mode helpers so they can correctly\n> > > + * This function configures the exposure mode helpers by providing them the\n> > > + * sensor configuration parameters and the sensor helper, so they can correctly\n> > > * take quantization effects into account.\n> > > */\n> > > -void AgcMeanLuminance::configure(utils::Duration lineDuration,\n> > > +void AgcMeanLuminance::configure(const AgcSensorConfiguration &config,\n> > > const CameraSensorHelper *sensorHelper)\n> > > {\n> > > for (auto &[id, helper] : exposureModeHelpers_)\n> > > - helper->configure(lineDuration, sensorHelper);\n> > > + helper->configure(config.lineDuration,\n> > > + config.minExposureTime, config.maxExposureTime,\n> > > + config.minAnalogueGain, config.maxAnalogueGain,\n> > > + sensorHelper);\n> >\n> > Tying the information to the sensor helper would make the sensor helper\n> > mandatory, but would simplify this.\n> >\n> > > }\n> > >\n> > > /**\n> > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h\n> > > index d7ec548e3e585e28fd03b956c8de7eaac5dc6e99..5ab75a3ac6c550a655f4bb7c8c30419ad8249183 100644\n> > > --- a/src/ipa/libipa/agc_mean_luminance.h\n> > > +++ b/src/ipa/libipa/agc_mean_luminance.h\n> > > @@ -42,7 +42,16 @@ public:\n> > > double yTarget;\n> > > };\n> > >\n> > > - void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);\n> > > + struct AgcSensorConfiguration {\n> > > + utils::Duration lineDuration;\n> > > + utils::Duration minExposureTime;\n> > > + utils::Duration maxExposureTime;\n> > > + double minAnalogueGain;\n> > > + double maxAnalogueGain;\n> > > + };\n> > > +\n> > > + void configure(const AgcSensorConfiguration &config,\n> > > + const CameraSensorHelper *sensorHelper);\n> > > int parseTuningData(const YamlObject &tuningData);\n> > >\n> > > void setExposureCompensation(double gain)\n> > > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp\n> > > index 29e316d9d09181cd65cb08f26babb1a680bee62a..557e14230ba4e0f120653bc09ea23a29a54cb4c8 100644\n> > > --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> > > +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> > > @@ -82,22 +82,32 @@ ExposureModeHelper::ExposureModeHelper(const Span > > /**\n> > > * \\brief Configure sensor details\n> > > * \\param[in] lineDuration The current line length of the sensor\n> > > + * \\param[in] minExposureTime The minimum exposure time supported\n> > > + * \\param[in] maxExposureTime The maximum exposure time supported\n> > > + * \\param[in] minGain The minimum analogue gain supported\n> > > + * \\param[in] maxGain The maximum analogue gain supported\n> > > * \\param[in] sensorHelper The sensor helper\n> > > *\n> > > - * This function sets the line length and sensor helper. These are used in\n> > > - * splitExposure() to take the quantization of the exposure and gain into\n> > > - * account.\n> > > + * This function initializes the exposure helper settings using the sensor\n> > > + * configuration parameters.\n> > > *\n> > > - * When this has not been called, it is assumed that exposure is in micro second\n> > > - * granularity and gain has no quantization at all.\n> > > + * The sensor parameters' min and max limits are used in splitExposure() to take\n> > > + * the quantization of the exposure and gain into account.\n> >\n> > This sentence now changed it's meaning as the min/max limits can't be\n> > used to take quantization into account... but I agree that without the\n> > quantization context in mind, this sentence came out of the blue.\n>\n>\n> Do we need to keep a quantized number of lines associated with its'\n> utils::Duration? if so I could further abstract Quantised to support\n> Duration types as well as floats.\n>\n>\n>\n> >\n> > > *\n> > > * ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the caller\n> > > * has to ensure that sensorHelper is valid until the next call to configure().\n> > > */\n> > > void ExposureModeHelper::configure(utils::Duration lineDuration,\n> > > + utils::Duration minExposureTime,\n> > > + utils::Duration maxExposureTime,\n>\n> This is why I'd like to introduce\n>\n> https://git.uk.ideasonboard.com/kbingham/libcamera/commit/9e85cd913213e16ea20c614003ce421ab323b9ac\n>\n> > > + double minGain, double maxGain,\n>\n>\n> So this is instead:\n>\n> \tconfigure(utils::Duration lineDuration,\n> \t\t Bounds exposureLimits,\n> \t Bounds gainLimits)\n>\n> Is the only reason we pass lineDuration around because the Agc has to do\n> quantization based on it ?\n>\n> I think that's probably something that should also get deferred into the\n> CameraSensorHelper so that all control of the sensor calculations are\n> common.\n>\n>\n> > > const CameraSensorHelper *sensorHelper)\n> > > {\n> > > lineDuration_ = lineDuration;\n> > > + minExposureTime_ = minExposureTime;\n> > > + maxExposureTime_ = maxExposureTime;\n> > > + minGain_ = minGain;\n> > > + maxGain_ = maxGain;\n> >\n> > This now sets the same members as setLimits() so, setLimits() can't\n> > internally clamp to the hardware limits (it never did). When we move the\n> > hardware limits into the sensor helper we'd additionally gain that\n> > ability.\n> >\n> > Best regards,\n> > Stefan\n> >\n> > > sensorHelper_ = sensorHelper;\n> > > }\n> > >\n> > > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> > > index 968192ddc5af768ae0de58aca6c7230c7b3bd507..bb0a692c99b3128a2ee3f15304f125e073721536 100644\n> > > --- a/src/ipa/libipa/exposure_mode_helper.h\n> > > +++ b/src/ipa/libipa/exposure_mode_helper.h\n> > > @@ -26,7 +26,9 @@ public:\n> > > ExposureModeHelper(const Span> stages);\n> > > ~ExposureModeHelper() = default;\n> > >\n> > > - void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);\n> > > + void configure(utils::Duration lineLength, utils::Duration minExposureTime,\n> > > + utils::Duration maxExposureTime, double minGain, double maxGain,\n> > > + const CameraSensorHelper *sensorHelper);\n> > > void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,\n> > > double minGain, double maxGain);\n> > >\n> > > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp\n> > > index f60fddac3f04fd6f09dc782e929ff1593758c29b..ccf72cc9e8c33b1a516322c2445efa1684b7f751 100644\n> > > --- a/src/ipa/mali-c55/algorithms/agc.cpp\n> > > +++ b/src/ipa/mali-c55/algorithms/agc.cpp\n> > > @@ -169,12 +169,16 @@ int Agc::configure(IPAContext &context,\n> > > context.activeState.agc.constraintMode = constraintModes().begin()->first;\n> > > context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;\n> > >\n> > > - /* \\todo Run this again when FrameDurationLimits is passed in */\n> > > - setLimits(context.configuration.agc.minShutterSpeed,\n> > > - context.configuration.agc.maxShutterSpeed,\n> > > - context.configuration.agc.minAnalogueGain,\n> > > - context.configuration.agc.maxAnalogueGain,\n> > > - {});\n> > > + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> > > + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> > > + sensorConfig.minExposureTime = context.configuration.agc.minShutterSpeed;\n> > > + sensorConfig.maxExposureTime = context.configuration.agc.maxShutterSpeed;\n> > > + sensorConfig.minAnalogueGain = context.configuration.agc.minAnalogueGain;\n> > > + sensorConfig.maxAnalogueGain = context.configuration.agc.maxAnalogueGain;\n> > > +\n> > > + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> > > +\n> > > + /* \\todo Update AGC limits when FrameDurationLimits is passed in */\n> > >\n> > > resetFrameCount();\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..8c07e6a6eac091d949ad71ee967c893897313cc8 100644\n> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp\n> > > @@ -200,13 +200,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)\n> > > context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;\n> > > context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;\n> > >\n> > > - AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,\n> > > - context.camHelper.get());\n> > > -\n> > > - setLimits(context.configuration.sensor.minExposureTime,\n> > > - context.configuration.sensor.maxExposureTime,\n> > > - context.configuration.sensor.minAnalogueGain,\n> > > - context.configuration.sensor.maxAnalogueGain, {});\n> > > + AgcMeanLuminance::AgcSensorConfiguration sensorConfig;\n> > > + sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> > > + sensorConfig.minExposureTime = context.configuration.sensor.minExposureTime;\n> > > + sensorConfig.maxExposureTime = context.configuration.sensor.maxExposureTime;\n> > > + sensorConfig.minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> > > + sensorConfig.maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;\n> > > +\n> > > + AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());\n> > >\n> > > context.activeState.agc.automatic.yTarget = effectiveYTarget();\n> > >\n> > >\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 BBD19BDE4C\n\tfor ;\n\tThu, 6 Nov 2025 08:20:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EDD9A609D8;\n\tThu, 6 Nov 2025 09:20:14 +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 0C3936096B\n\tfor ;\n\tThu, 6 Nov 2025 09:20:13 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 085E96DC;\n\tThu, 6 Nov 2025 09:18:17 +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=\"S1FttuIB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762417098;\n\tbh=lHiiuO5TvzbIA5HhHbfkuh+POV6dTLZQkPq/vhLahDI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=S1FttuIBhcUNE8BOf6gYy8wRH6s1joJdC99bkD7L9mWNopaeaAJDitbAWO5WJRsEQ\n\t5XPGQSRQDMWuVGG0MkaFNpbxmjXfo5/bHNOOsS9vU9Wfv8SzDhcA14j2FYHS1n5KeJ\n\tDeSWHT5+RwtdskWYbyZHdAX5/dRr/5FuR1CbkU9w=","Date":"Thu, 6 Nov 2025 09:20:09 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"Jacopo Mondi , Niklas =?utf-8?b?U8O2?=\n\t=?utf-8?q?derlund?= , Robert Mader\n\t, \n\tStefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 04/10] ipa: libipa: agc: Make Agc::configure() set\n\tlimits","Message-ID":"","References":"<20251028-exposure-limits-v2-0-a8b5a318323e@ideasonboard.com>\n\t<20251028-exposure-limits-v2-4-a8b5a318323e@ideasonboard.com>\n\t<176232858261.231750.6488373674243924560@localhost>\n\t<176233304563.3742839.8261569944215394507@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176233304563.3742839.8261569944215394507@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\" "}}]