[{"id":38124,"web_url":"https://patchwork.libcamera.org/comment/38124/","msgid":"<177039187051.607498.18216993346490602000@neptunite.rasen.tech>","date":"2026-02-06T15:31:10","subject":"Re: [PATCH v3 10/19] ipa: libipa: agc: Make Agc::configure() set\n\tlimits","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2025-11-14 23:17:05)\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> 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 and provide an AgcMeanLuminance::SensorConfiguration\n> type for IPAs to be populated with the sensor's default values.\n> \n> The new type mimics the ExposureModeHelper::SensorConfiguration on\n> introduced in the previous patches and both will be removed once all the\n> information there contained will be made available from the\n> CameraSensorHelper.\n> \n> Update all IPAs Agc implementation deriving from AgcMeanLuminance to\n> populate a AgcMeanLuminance::SensorConfiguration and use it in\n> configure().\n> \n> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> ---\n>  src/ipa/ipu3/algorithms/agc.cpp         | 14 +++++++--\n>  src/ipa/libipa/agc_mean_luminance.cpp   | 56 ++++++++++++++++++++++++++++++---\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   |  3 +-\n>  src/ipa/mali-c55/algorithms/agc.cpp     | 16 ++++++----\n>  src/ipa/rkisp1/algorithms/agc.cpp       | 15 ++++-----\n>  7 files changed, 105 insertions(+), 30 deletions(-)\n> \n> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp\n> index 4574f3a1a9cd3f40b1b1402238809ee1a777946d..5c72806dede5f55459bde69ab8cfaebc495c7560 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::SensorConfiguration 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..512e153791f5b98da01efad6675192a5358e7698 100644\n> --- a/src/ipa/libipa/agc_mean_luminance.cpp\n> +++ b/src/ipa/libipa/agc_mean_luminance.cpp\n> @@ -95,6 +95,35 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;\n>   * \\brief The luminance target for the constraint\n>   */\n>  \n> +/**\n> + * \\struct AgcMeanLuminance::SensorConfiguration\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> + * \\todo Remove it once all the information are available from the\n> + * CameraSensorHelper.\n> + *\n> + * \\var AgcMeanLuminance::SensorConfiguration::lineDuration\n> + * \\brief The line duration in microseconds\n> + *\n> + * \\var AgcMeanLuminance::SensorConfiguration::minExposureTime\n> + * \\brief The sensor minimum exposure time in microseconds\n> + *\n> + * \\var AgcMeanLuminance::SensorConfiguration::maxExposureTime\n> + * \\brief The sensor maximum exposure time in microseconds\n> + *\n> + * \\var AgcMeanLuminance::SensorConfiguration::minAnalogueGain\n> + * \\brief The sensor minimum analogue gain absolute value\n> + *\n> + * \\var AgcMeanLuminance::SensorConfiguration::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 +343,34 @@ 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 SensorConfiguration &config,\n>                                  const CameraSensorHelper *sensorHelper)\n>  {\n> -       for (auto &[id, helper] : exposureModeHelpers_)\n> -               helper->configure(lineDuration, sensorHelper);\n> +       for (auto &[id, helper] : exposureModeHelpers_) {\n> +               /*\n> +                * Translate from the SensorConfiguration to the\n> +                * ExposureModeHelper::SensorConfiguration.\n> +                *\n> +                * These are just place holders before all the information are\n> +                * available from the sensor helper.\n> +                */\n> +\n> +               ExposureModeHelper::SensorConfiguration sensorConfig;\n> +               sensorConfig.lineDuration_ = config.lineDuration;\n> +               sensorConfig.minExposureTime_ = config.minExposureTime;\n> +               sensorConfig.maxExposureTime_ = config.maxExposureTime;\n> +               sensorConfig.minGain_ = config.minAnalogueGain;\n> +               sensorConfig.maxGain_ = config.maxAnalogueGain;\n> +\n> +               helper->configure(sensorConfig, sensorHelper);\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 e5f164c3186b6b99cb0df5dd8dccf9026c22af20..42ead74b0cdc197bc2b27aee16918e2b42ea3d08 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 SensorConfiguration {\n> +               utils::Duration lineDuration;\n> +               utils::Duration minExposureTime;\n> +               utils::Duration maxExposureTime;\n> +               double minAnalogueGain;\n> +               double maxAnalogueGain;\n> +       };\n> +\n> +       void configure(const SensorConfiguration &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 c3ed1601939bd28435bbbc540d9b8c9d92b81912..f771b10a28eead2976c0000cf099ba5cfbe78e0f 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.cpp\n> +++ b/src/ipa/libipa/exposure_mode_helper.cpp\n> @@ -111,24 +111,30 @@ ExposureModeHelper::ExposureModeHelper(const Span<std::pair<utils::Duration, dou\n>  \n>  /**\n>   * \\brief Configure sensor details\n> - * \\param[in] lineDuration The current line length of the sensor\n> + * \\param[in] sensorConfig The sensor configuration\n>   * \\param[in] sensorHelper The sensor helper\n>   *\n> - * This function sets the line length and sensor helper. These are used in\n> + * This function initializes the exposure helper settings using the sensor\n> + * configuration parameters.\n> + *\n> + * The sensor parameters' are used to initialize the min and max limits used in\n\ns/'//\n\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n\n>   * splitExposure() to take the quantization of the exposure and gain into\n>   * account.\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> - *\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> +void ExposureModeHelper::configure(const SensorConfiguration &sensorConfig,\n>                                    const CameraSensorHelper *sensorHelper)\n>  {\n> -       sensor_.lineDuration_ = lineDuration;\n> +       sensor_ = sensorConfig;\n>         sensorHelper_ = sensorHelper;\n> +\n> +       /* Initialize run-time limits with sensor's default. */\n> +       minExposureTime_ = sensor_.minExposureTime_;\n> +       maxExposureTime_ = sensor_.maxExposureTime_;\n> +       minGain_ = sensor_.minGain_;\n> +       maxGain_ = sensor_.maxGain_;\n>  }\n>  \n>  /**\n> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h\n> index 4971cfbf5b2be9ef0e3e95a64b815902833e93a4..e41c58767eee65dd27946336beb2bc182dd4ab58 100644\n> --- a/src/ipa/libipa/exposure_mode_helper.h\n> +++ b/src/ipa/libipa/exposure_mode_helper.h\n> @@ -38,7 +38,8 @@ public:\n>         ExposureModeHelper(const Span<std::pair<utils::Duration, double>> stages);\n>         ~ExposureModeHelper() = default;\n>  \n> -       void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);\n> +       void configure(const SensorConfiguration &sensorConfig,\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 4fa00769d201d906be357809f5af02c71201a4f1..d6a1ff5aaca136c387feb8c948053fc83bb664ee 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.sensor.minShutterSpeed,\n> -                 context.configuration.sensor.maxShutterSpeed,\n> -                 context.configuration.sensor.minAnalogueGain,\n> -                 context.configuration.sensor.maxAnalogueGain,\n> -                 {});\n> +       AgcMeanLuminance::SensorConfiguration sensorConfig;\n> +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;\n> +       sensorConfig.minExposureTime = context.configuration.sensor.minShutterSpeed;\n> +       sensorConfig.maxExposureTime = context.configuration.sensor.maxShutterSpeed;\n> +       sensorConfig.minAnalogueGain = context.configuration.sensor.minAnalogueGain;\n> +       sensorConfig.maxAnalogueGain = context.configuration.sensor.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 aa1a90daf3ca7d0041c56000c12fc4d1ab5700eb..b0c8966eea63901640bbe16af2a5d8a303c63ece 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::SensorConfiguration 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.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 9D793C32EF\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  6 Feb 2026 15:31:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 30BA8620B2;\n\tFri,  6 Feb 2026 16:31:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 962266209B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  6 Feb 2026 16:31:16 +0100 (CET)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:5247:7b72:2b7:10da])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 896CC2E0;\n\tFri,  6 Feb 2026 16:30:32 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"F/9FLKIq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1770391833;\n\tbh=PGMM0uXd2upoE6YQqBZpp9kH5EZjLPAKrhiNyA6K9Nw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=F/9FLKIqvjr4rhJBS/L27wxfsXDPrrkM+fxrJ2e8mgBMXadBYsUJFuEIciZCH7IsF\n\t5oubQzyxCt6lRplzPr8Ixrlmc+E6KOgxFGsWVbHvlmu5bZ1zD6KF++xrSac5Ftiu8W\n\t0QE7Mw4MeuQqTySY7wJEMmr4vvzEYEsqjjQBvAjE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251114-exposure-limits-v3-10-b7c07feba026@ideasonboard.com>","References":"<20251114-exposure-limits-v3-0-b7c07feba026@ideasonboard.com>\n\t<20251114-exposure-limits-v3-10-b7c07feba026@ideasonboard.com>","Subject":"Re: [PATCH v3 10/19] ipa: libipa: agc: Make Agc::configure() set\n\tlimits","From":"Paul Elder <paul.elder@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>, Niklas =?utf-8?b?U8O2?=\n\t=?utf-8?q?derlund?= <niklas.soderlund@ragnatech.se>, Robert Mader\n\t<robert.mader@collabora.com>, libcamera-devel@lists.libcamera.org","Date":"Sat, 07 Feb 2026 00:31:10 +0900","Message-ID":"<177039187051.607498.18216993346490602000@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]