[v2,04/10] ipa: libipa: agc: Make Agc::configure() set limits

24842 diff mbox series

The AgcMeanLuminance algorithm interface has a 'configure()' and
'setLimits()' API. configure() doesn't actually do much if not
initialzing a few variables, and the Mali and IPU3 IPAs do not
even call it. Configuring the AGC requires calling setLimits() at
configure() time and at run time.

In order to prepare to differentiate between configure-time settings
of the Agc and run-time handling of dynamic parameters such as the
frame duration limits, make the configure() operation initialize
the Agc limits.

Update all IPAs Agc implementation deriving from AgcMeanLuminance.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/ipa/ipu3/algorithms/agc.cpp         | 14 +++++++---
 src/ipa/libipa/agc_mean_luminance.cpp   | 48 ++++++++++++++++++++++++++++++---
 src/ipa/libipa/agc_mean_luminance.h     | 11 +++++++-
 src/ipa/libipa/exposure_mode_helper.cpp | 20 ++++++++++----
 src/ipa/libipa/exposure_mode_helper.h   |  4 ++-
 src/ipa/mali-c55/algorithms/agc.cpp     | 16 ++++++-----
 src/ipa/rkisp1/algorithms/agc.cpp       | 15 ++++++-----
 7 files changed, 101 insertions(+), 27 deletions(-)

Hi Jacopo,

Thank you for the patch.

Quoting Jacopo Mondi (2025-10-28 10:31:50)
> The AgcMeanLuminance algorithm interface has a 'configure()' and
> 'setLimits()' API. configure() doesn't actually do much if not
> initialzing a few variables, and the Mali and IPU3 IPAs do not
> even call it. Configuring the AGC requires calling setLimits() at
> configure() time and at run time.

Oh I just realized, that my internal policy is to keep the libipa
interface backwards compatible so there is no need to update to update
other pipeline handlers (That's why the others didn't call configure()).
Maybe I should revisit this policy... but then any change get's even
harder to test...

> 
> In order to prepare to differentiate between configure-time settings
> of the Agc and run-time handling of dynamic parameters such as the
> frame duration limits, make the configure() operation initialize
> the Agc limits.

Yes, that makes sense.

> 
> Update all IPAs Agc implementation deriving from AgcMeanLuminance.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc.cpp         | 14 +++++++---
>  src/ipa/libipa/agc_mean_luminance.cpp   | 48 ++++++++++++++++++++++++++++++---
>  src/ipa/libipa/agc_mean_luminance.h     | 11 +++++++-
>  src/ipa/libipa/exposure_mode_helper.cpp | 20 ++++++++++----
>  src/ipa/libipa/exposure_mode_helper.h   |  4 ++-
>  src/ipa/mali-c55/algorithms/agc.cpp     | 16 ++++++-----
>  src/ipa/rkisp1/algorithms/agc.cpp       | 15 ++++++-----
>  7 files changed, 101 insertions(+), 27 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index b0d89541da8550bd50472779da7fa8e33b96e2f0..e303d724ab165bac03ac6bb9d3891a76049cff47 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -115,9 +115,17 @@ int Agc::configure(IPAContext &context,
>         context.activeState.agc.constraintMode = constraintModes().begin()->first;
>         context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
>  
> -       /* \todo Run this again when FrameDurationLimits is passed in */
> -       setLimits(minExposureTime_, maxExposureTime_, minAnalogueGain_,
> -                 maxAnalogueGain_, {});
> +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> +       sensorConfig.minExposureTime = minExposureTime_;
> +       sensorConfig.maxExposureTime = maxExposureTime_;
> +       sensorConfig.minAnalogueGain = minAnalogueGain_;
> +       sensorConfig.maxAnalogueGain = maxAnalogueGain_;
> +
> +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> +
> +       /* \todo Update AGC limits when FrameDurationLimits is passed in */
> +
>         resetFrameCount();
>  
>         return 0;
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 64f36bd75dd213671b5a2e6810245096ed001f21..9886864fc42514040eadb4c0b424796df1f1b6e1 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -95,6 +95,42 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
>   * \brief The luminance target for the constraint
>   */
>  
> +/**
> + * \struct AgcMeanLuminance::AgcSensorConfiguration
> + * \brief The sensor configuration parameters
> + *
> + * This structure collects the sensor configuration parameters which need
> + * to be provided to the AGC algorithm at configure() time.
> + *
> + * Each time configure() is called the sensor configuration need to be updated
> + * with new parameters.

Reading the series I (again) realized how hard it is to follow the
dependencies/constraints in the system.

I wonder if this should be moved to the sensor helper? In the end it is
not a configuration of the AGC but a property of the sensor. Aside from
a hardcoded 60ms in IPU3 I don't see that we impose additional AGC
specific limits. I think Kieran had similar thoughts.

If we leave it in here, we could call it AgcRegulationLimits, then
lineDuration doesn't fit the scheme... so maybe just AgcConfiguration?

But I think fetching this info from the sensor helper and configuring
that one would be best.

> + */
> +
> +/**
> + * \var AgcMeanLuminance::AgcSensorConfiguration::lineDuration
> + * \brief The line duration in microseconds
> + */
> +
> +/**
> + * \var AgcMeanLuminance::AgcSensorConfiguration::minExposureTime
> + * \brief The sensor minimum exposure time in microseconds
> + */
> +
> +/**
> + * \var AgcMeanLuminance::AgcSensorConfiguration::maxExposureTime
> + * \brief The sensor maximum exposure time in microseconds


As a first time IPA implementer I would fill this with the maximum of
V4L2_CID_EXPOSURE converted to microseconds. Which is clearly the wrong
thing to do. Still I wonder if this should be made extra clear but I
have difficulties finding the right words.

Maybe "maximum achievable exposure time" - doesn't actually convey more
information. "max exposure time the hardware can do" might make it
easier to switch away from the exposure control. I don't know.

> + */
> +
> +/**
> + * \var AgcMeanLuminance::AgcSensorConfiguration::minAnalogueGain
> + * \brief The sensor minimum analogue gain absolute value
> + */
> +
> +/**
> + * \var AgcMeanLuminance::AgcSensorConfiguration::maxAnalogueGain
> + * \brief The sensor maximum analogue gain absolute value
> + */
> +
>  /**
>   * \class AgcMeanLuminance
>   * \brief A mean-based auto-exposure algorithm
> @@ -314,17 +350,21 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)
>  
>  /**
>   * \brief Configure the exposure mode helpers
> - * \param[in] lineDuration The sensor line length
> + * \param[in] config The sensor configuration
>   * \param[in] sensorHelper The sensor helper
>   *
> - * This function configures the exposure mode helpers so they can correctly
> + * This function configures the exposure mode helpers by providing them the
> + * sensor configuration parameters and the sensor helper, so they can correctly
>   * take quantization effects into account.
>   */
> -void AgcMeanLuminance::configure(utils::Duration lineDuration,
> +void AgcMeanLuminance::configure(const AgcSensorConfiguration &config,
>                                  const CameraSensorHelper *sensorHelper)
>  {
>         for (auto &[id, helper] : exposureModeHelpers_)
> -               helper->configure(lineDuration, sensorHelper);
> +               helper->configure(config.lineDuration,
> +                                 config.minExposureTime, config.maxExposureTime,
> +                                 config.minAnalogueGain, config.maxAnalogueGain,
> +                                 sensorHelper);

Tying the information to the sensor helper would make the sensor helper
mandatory, but would simplify this.

>  }
>  
>  /**
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index d7ec548e3e585e28fd03b956c8de7eaac5dc6e99..5ab75a3ac6c550a655f4bb7c8c30419ad8249183 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -42,7 +42,16 @@ public:
>                 double yTarget;
>         };
>  
> -       void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);
> +       struct AgcSensorConfiguration {
> +               utils::Duration lineDuration;
> +               utils::Duration minExposureTime;
> +               utils::Duration maxExposureTime;
> +               double minAnalogueGain;
> +               double maxAnalogueGain;
> +       };
> +
> +       void configure(const AgcSensorConfiguration &config,
> +                      const CameraSensorHelper *sensorHelper);
>         int parseTuningData(const YamlObject &tuningData);
>  
>         void setExposureCompensation(double gain)
> diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> index 29e316d9d09181cd65cb08f26babb1a680bee62a..557e14230ba4e0f120653bc09ea23a29a54cb4c8 100644
> --- a/src/ipa/libipa/exposure_mode_helper.cpp
> +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> @@ -82,22 +82,32 @@ ExposureModeHelper::ExposureModeHelper(const Span<std::pair<utils::Duration, dou
>  /**
>   * \brief Configure sensor details
>   * \param[in] lineDuration The current line length of the sensor
> + * \param[in] minExposureTime The minimum exposure time supported
> + * \param[in] maxExposureTime The maximum exposure time supported
> + * \param[in] minGain The minimum analogue gain supported
> + * \param[in] maxGain The maximum analogue gain supported
>   * \param[in] sensorHelper The sensor helper
>   *
> - * This function sets the line length and sensor helper. These are used in
> - * splitExposure() to take the quantization of the exposure and gain into
> - * account.
> + * This function initializes the exposure helper settings using the sensor
> + * configuration parameters.
>   *
> - * When this has not been called, it is assumed that exposure is in micro second
> - * granularity and gain has no quantization at all.
> + * The sensor parameters' min and max limits are used in splitExposure() to take
> + * the quantization of the exposure and gain into account.

This sentence now changed it's meaning as the min/max limits can't be
used to take quantization into account... but I agree that without the
quantization context in mind, this sentence came out of the blue.

>   *
>   * ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the caller
>   * has to ensure that sensorHelper is valid until the next call to configure().
>   */
>  void ExposureModeHelper::configure(utils::Duration lineDuration,
> +                                  utils::Duration minExposureTime,
> +                                  utils::Duration maxExposureTime,
> +                                  double minGain, double maxGain,
>                                    const CameraSensorHelper *sensorHelper)
>  {
>         lineDuration_ = lineDuration;
> +       minExposureTime_ = minExposureTime;
> +       maxExposureTime_ = maxExposureTime;
> +       minGain_ = minGain;
> +       maxGain_ = maxGain;

This now sets the same members as setLimits() so, setLimits() can't
internally clamp to the hardware limits (it never did). When we move the
hardware limits into the sensor helper we'd additionally gain that
ability.

Best regards,
Stefan

>         sensorHelper_ = sensorHelper;
>  }
>  
> diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
> index 968192ddc5af768ae0de58aca6c7230c7b3bd507..bb0a692c99b3128a2ee3f15304f125e073721536 100644
> --- a/src/ipa/libipa/exposure_mode_helper.h
> +++ b/src/ipa/libipa/exposure_mode_helper.h
> @@ -26,7 +26,9 @@ public:
>         ExposureModeHelper(const Span<std::pair<utils::Duration, double>> stages);
>         ~ExposureModeHelper() = default;
>  
> -       void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);
> +       void configure(utils::Duration lineLength, utils::Duration minExposureTime,
> +                      utils::Duration maxExposureTime, double minGain, double maxGain,
> +                      const CameraSensorHelper *sensorHelper);
>         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
>                        double minGain, double maxGain);
>  
> diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
> index f60fddac3f04fd6f09dc782e929ff1593758c29b..ccf72cc9e8c33b1a516322c2445efa1684b7f751 100644
> --- a/src/ipa/mali-c55/algorithms/agc.cpp
> +++ b/src/ipa/mali-c55/algorithms/agc.cpp
> @@ -169,12 +169,16 @@ int Agc::configure(IPAContext &context,
>         context.activeState.agc.constraintMode = constraintModes().begin()->first;
>         context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
>  
> -       /* \todo Run this again when FrameDurationLimits is passed in */
> -       setLimits(context.configuration.agc.minShutterSpeed,
> -                 context.configuration.agc.maxShutterSpeed,
> -                 context.configuration.agc.minAnalogueGain,
> -                 context.configuration.agc.maxAnalogueGain,
> -                 {});
> +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> +       sensorConfig.minExposureTime = context.configuration.agc.minShutterSpeed;
> +       sensorConfig.maxExposureTime = context.configuration.agc.maxShutterSpeed;
> +       sensorConfig.minAnalogueGain = context.configuration.agc.minAnalogueGain;
> +       sensorConfig.maxAnalogueGain = context.configuration.agc.maxAnalogueGain;
> +
> +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> +
> +       /* \todo Update AGC limits when FrameDurationLimits is passed in */
>  
>         resetFrameCount();
>  
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..8c07e6a6eac091d949ad71ee967c893897313cc8 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -200,13 +200,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>         context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;
>         context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;
>  
> -       AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,
> -                                   context.camHelper.get());
> -
> -       setLimits(context.configuration.sensor.minExposureTime,
> -                 context.configuration.sensor.maxExposureTime,
> -                 context.configuration.sensor.minAnalogueGain,
> -                 context.configuration.sensor.maxAnalogueGain, {});
> +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> +       sensorConfig.minExposureTime = context.configuration.sensor.minExposureTime;
> +       sensorConfig.maxExposureTime = context.configuration.sensor.maxExposureTime;
> +       sensorConfig.minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> +       sensorConfig.maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;
> +
> +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
>  
>         context.activeState.agc.automatic.yTarget = effectiveYTarget();
>  
> 
> -- 
> 2.51.0
>

Quoting Stefan Klug (2025-11-05 07:43:02)
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> Quoting Jacopo Mondi (2025-10-28 10:31:50)
> > The AgcMeanLuminance algorithm interface has a 'configure()' and
> > 'setLimits()' API. configure() doesn't actually do much if not
> > initialzing a few variables, and the Mali and IPU3 IPAs do not
> > even call it. Configuring the AGC requires calling setLimits() at
> > configure() time and at run time.
> 
> Oh I just realized, that my internal policy is to keep the libipa
> interface backwards compatible so there is no need to update to update
> other pipeline handlers (That's why the others didn't call configure()).
> Maybe I should revisit this policy... but then any change get's even
> harder to test...
> 
> > 
> > In order to prepare to differentiate between configure-time settings
> > of the Agc and run-time handling of dynamic parameters such as the
> > frame duration limits, make the configure() operation initialize
> > the Agc limits.
> 
> Yes, that makes sense.
> 
> > 
> > Update all IPAs Agc implementation deriving from AgcMeanLuminance.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/agc.cpp         | 14 +++++++---
> >  src/ipa/libipa/agc_mean_luminance.cpp   | 48 ++++++++++++++++++++++++++++++---
> >  src/ipa/libipa/agc_mean_luminance.h     | 11 +++++++-
> >  src/ipa/libipa/exposure_mode_helper.cpp | 20 ++++++++++----
> >  src/ipa/libipa/exposure_mode_helper.h   |  4 ++-
> >  src/ipa/mali-c55/algorithms/agc.cpp     | 16 ++++++-----
> >  src/ipa/rkisp1/algorithms/agc.cpp       | 15 ++++++-----
> >  7 files changed, 101 insertions(+), 27 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index b0d89541da8550bd50472779da7fa8e33b96e2f0..e303d724ab165bac03ac6bb9d3891a76049cff47 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -115,9 +115,17 @@ int Agc::configure(IPAContext &context,
> >         context.activeState.agc.constraintMode = constraintModes().begin()->first;
> >         context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> >  
> > -       /* \todo Run this again when FrameDurationLimits is passed in */
> > -       setLimits(minExposureTime_, maxExposureTime_, minAnalogueGain_,
> > -                 maxAnalogueGain_, {});
> > +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> > +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> > +       sensorConfig.minExposureTime = minExposureTime_;
> > +       sensorConfig.maxExposureTime = maxExposureTime_;
> > +       sensorConfig.minAnalogueGain = minAnalogueGain_;
> > +       sensorConfig.maxAnalogueGain = maxAnalogueGain_;
> > +
> > +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> > +
> > +       /* \todo Update AGC limits when FrameDurationLimits is passed in */
> > +
> >         resetFrameCount();
> >  
> >         return 0;
> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > index 64f36bd75dd213671b5a2e6810245096ed001f21..9886864fc42514040eadb4c0b424796df1f1b6e1 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > @@ -95,6 +95,42 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> >   * \brief The luminance target for the constraint
> >   */
> >  
> > +/**
> > + * \struct AgcMeanLuminance::AgcSensorConfiguration
> > + * \brief The sensor configuration parameters
> > + *
> > + * This structure collects the sensor configuration parameters which need
> > + * to be provided to the AGC algorithm at configure() time.
> > + *
> > + * Each time configure() is called the sensor configuration need to be updated
> > + * with new parameters.
> 
> Reading the series I (again) realized how hard it is to follow the
> dependencies/constraints in the system.
> 
> I wonder if this should be moved to the sensor helper? In the end it is
> not a configuration of the AGC but a property of the sensor. Aside from
> a hardcoded 60ms in IPU3 I don't see that we impose additional AGC
> specific limits. I think Kieran had similar thoughts.
> 
> If we leave it in here, we could call it AgcRegulationLimits, then
> lineDuration doesn't fit the scheme... so maybe just AgcConfiguration?
> 
> But I think fetching this info from the sensor helper and configuring
> that one would be best.


Yes, I think all tasks related to the CameraSensor should be in the
CameraSensorHelper - *not* replicated across algorithms and IPAs.
Especially in this topic where we're talking about how should we
configure and manage the limits of the CameraSensor - that should
definitely all be common in my view.

 ipa: Move libipa camera configuration to helpers 
 - https://git.uk.ideasonboard.com/kbingham/libcamera/commit/5c711d2a896d5a892ef61f263d623a440b58b33d

I think CameraSensorHelper should manage the v4l2 conversions, and
provide an interface that an Agc can ask "Hey what limits can I use"

Then the *common* CameraSensorHelper will report that based on any
manual control parsing.

I'd also like to introduce a new wrapper around min/max for this topic
because they're so intrinsically tied together and when I worked through
this I was really bothered with always having to move/manage them as a
pair :-)

 ipa: libipa: Provide a Bounds type to pass min/max 
 - https://git.uk.ideasonboard.com/kbingham/libcamera/commit/9e85cd913213e16ea20c614003ce421ab323b9ac



Those commits are part of my slow progressing soft-libipa branch:
 https://git.uk.ideasonboard.com/kbingham/libcamera/pulls/18

where I'm working towards making it easier to integrate AgcMeanLuminance
in new IPA modules - and factor out all the repeated or common code that
keeps getting duplicated.

--
Kieran


> 
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::lineDuration
> > + * \brief The line duration in microseconds
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::minExposureTime
> > + * \brief The sensor minimum exposure time in microseconds
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::maxExposureTime
> > + * \brief The sensor maximum exposure time in microseconds
> 
> 
> As a first time IPA implementer I would fill this with the maximum of
> V4L2_CID_EXPOSURE converted to microseconds. Which is clearly the wrong
> thing to do. Still I wonder if this should be made extra clear but I
> have difficulties finding the right words.
> 
> Maybe "maximum achievable exposure time" - doesn't actually convey more
> information. "max exposure time the hardware can do" might make it
> easier to switch away from the exposure control. I don't know.
> 
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::minAnalogueGain
> > + * \brief The sensor minimum analogue gain absolute value
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::maxAnalogueGain
> > + * \brief The sensor maximum analogue gain absolute value
> > + */
> > +
> >  /**
> >   * \class AgcMeanLuminance
> >   * \brief A mean-based auto-exposure algorithm
> > @@ -314,17 +350,21 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)
> >  
> >  /**
> >   * \brief Configure the exposure mode helpers
> > - * \param[in] lineDuration The sensor line length
> > + * \param[in] config The sensor configuration
> >   * \param[in] sensorHelper The sensor helper
> >   *
> > - * This function configures the exposure mode helpers so they can correctly
> > + * This function configures the exposure mode helpers by providing them the
> > + * sensor configuration parameters and the sensor helper, so they can correctly
> >   * take quantization effects into account.
> >   */
> > -void AgcMeanLuminance::configure(utils::Duration lineDuration,
> > +void AgcMeanLuminance::configure(const AgcSensorConfiguration &config,
> >                                  const CameraSensorHelper *sensorHelper)
> >  {
> >         for (auto &[id, helper] : exposureModeHelpers_)
> > -               helper->configure(lineDuration, sensorHelper);
> > +               helper->configure(config.lineDuration,
> > +                                 config.minExposureTime, config.maxExposureTime,
> > +                                 config.minAnalogueGain, config.maxAnalogueGain,
> > +                                 sensorHelper);
> 
> Tying the information to the sensor helper would make the sensor helper
> mandatory, but would simplify this.
> 
> >  }
> >  
> >  /**
> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > index d7ec548e3e585e28fd03b956c8de7eaac5dc6e99..5ab75a3ac6c550a655f4bb7c8c30419ad8249183 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.h
> > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > @@ -42,7 +42,16 @@ public:
> >                 double yTarget;
> >         };
> >  
> > -       void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);
> > +       struct AgcSensorConfiguration {
> > +               utils::Duration lineDuration;
> > +               utils::Duration minExposureTime;
> > +               utils::Duration maxExposureTime;
> > +               double minAnalogueGain;
> > +               double maxAnalogueGain;
> > +       };
> > +
> > +       void configure(const AgcSensorConfiguration &config,
> > +                      const CameraSensorHelper *sensorHelper);
> >         int parseTuningData(const YamlObject &tuningData);
> >  
> >         void setExposureCompensation(double gain)
> > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> > index 29e316d9d09181cd65cb08f26babb1a680bee62a..557e14230ba4e0f120653bc09ea23a29a54cb4c8 100644
> > --- a/src/ipa/libipa/exposure_mode_helper.cpp
> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> > @@ -82,22 +82,32 @@ ExposureModeHelper::ExposureModeHelper(const Span<std::pair<utils::Duration, dou
> >  /**
> >   * \brief Configure sensor details
> >   * \param[in] lineDuration The current line length of the sensor
> > + * \param[in] minExposureTime The minimum exposure time supported
> > + * \param[in] maxExposureTime The maximum exposure time supported
> > + * \param[in] minGain The minimum analogue gain supported
> > + * \param[in] maxGain The maximum analogue gain supported
> >   * \param[in] sensorHelper The sensor helper
> >   *
> > - * This function sets the line length and sensor helper. These are used in
> > - * splitExposure() to take the quantization of the exposure and gain into
> > - * account.
> > + * This function initializes the exposure helper settings using the sensor
> > + * configuration parameters.
> >   *
> > - * When this has not been called, it is assumed that exposure is in micro second
> > - * granularity and gain has no quantization at all.
> > + * The sensor parameters' min and max limits are used in splitExposure() to take
> > + * the quantization of the exposure and gain into account.
> 
> This sentence now changed it's meaning as the min/max limits can't be
> used to take quantization into account... but I agree that without the
> quantization context in mind, this sentence came out of the blue.


Do we need to keep a quantized number of lines associated with its'
utils::Duration? if so I could further abstract Quantised to support
Duration types as well as floats.



> 
> >   *
> >   * ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the caller
> >   * has to ensure that sensorHelper is valid until the next call to configure().
> >   */
> >  void ExposureModeHelper::configure(utils::Duration lineDuration,
> > +                                  utils::Duration minExposureTime,
> > +                                  utils::Duration maxExposureTime,

This is why I'd like to introduce 

https://git.uk.ideasonboard.com/kbingham/libcamera/commit/9e85cd913213e16ea20c614003ce421ab323b9ac

> > +                                  double minGain, double maxGain,


So this is instead:

	configure(utils::Duration lineDuration,
		  Bounds<utils::Duration> exposureLimits,
	          Bounds<double> gainLimits)

Is the only reason we pass lineDuration around because the Agc has to do
quantization based on it ?

I think that's probably something that should also get deferred into the
CameraSensorHelper so that all control of the sensor calculations are
common.


> >                                    const CameraSensorHelper *sensorHelper)
> >  {
> >         lineDuration_ = lineDuration;
> > +       minExposureTime_ = minExposureTime;
> > +       maxExposureTime_ = maxExposureTime;
> > +       minGain_ = minGain;
> > +       maxGain_ = maxGain;
> 
> This now sets the same members as setLimits() so, setLimits() can't
> internally clamp to the hardware limits (it never did). When we move the
> hardware limits into the sensor helper we'd additionally gain that
> ability.
> 
> Best regards,
> Stefan
> 
> >         sensorHelper_ = sensorHelper;
> >  }
> >  
> > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
> > index 968192ddc5af768ae0de58aca6c7230c7b3bd507..bb0a692c99b3128a2ee3f15304f125e073721536 100644
> > --- a/src/ipa/libipa/exposure_mode_helper.h
> > +++ b/src/ipa/libipa/exposure_mode_helper.h
> > @@ -26,7 +26,9 @@ public:
> >         ExposureModeHelper(const Span<std::pair<utils::Duration, double>> stages);
> >         ~ExposureModeHelper() = default;
> >  
> > -       void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);
> > +       void configure(utils::Duration lineLength, utils::Duration minExposureTime,
> > +                      utils::Duration maxExposureTime, double minGain, double maxGain,
> > +                      const CameraSensorHelper *sensorHelper);
> >         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> >                        double minGain, double maxGain);
> >  
> > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
> > index f60fddac3f04fd6f09dc782e929ff1593758c29b..ccf72cc9e8c33b1a516322c2445efa1684b7f751 100644
> > --- a/src/ipa/mali-c55/algorithms/agc.cpp
> > +++ b/src/ipa/mali-c55/algorithms/agc.cpp
> > @@ -169,12 +169,16 @@ int Agc::configure(IPAContext &context,
> >         context.activeState.agc.constraintMode = constraintModes().begin()->first;
> >         context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> >  
> > -       /* \todo Run this again when FrameDurationLimits is passed in */
> > -       setLimits(context.configuration.agc.minShutterSpeed,
> > -                 context.configuration.agc.maxShutterSpeed,
> > -                 context.configuration.agc.minAnalogueGain,
> > -                 context.configuration.agc.maxAnalogueGain,
> > -                 {});
> > +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> > +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> > +       sensorConfig.minExposureTime = context.configuration.agc.minShutterSpeed;
> > +       sensorConfig.maxExposureTime = context.configuration.agc.maxShutterSpeed;
> > +       sensorConfig.minAnalogueGain = context.configuration.agc.minAnalogueGain;
> > +       sensorConfig.maxAnalogueGain = context.configuration.agc.maxAnalogueGain;
> > +
> > +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> > +
> > +       /* \todo Update AGC limits when FrameDurationLimits is passed in */
> >  
> >         resetFrameCount();
> >  
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..8c07e6a6eac091d949ad71ee967c893897313cc8 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -200,13 +200,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >         context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;
> >         context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;
> >  
> > -       AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,
> > -                                   context.camHelper.get());
> > -
> > -       setLimits(context.configuration.sensor.minExposureTime,
> > -                 context.configuration.sensor.maxExposureTime,
> > -                 context.configuration.sensor.minAnalogueGain,
> > -                 context.configuration.sensor.maxAnalogueGain, {});
> > +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> > +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> > +       sensorConfig.minExposureTime = context.configuration.sensor.minExposureTime;
> > +       sensorConfig.maxExposureTime = context.configuration.sensor.maxExposureTime;
> > +       sensorConfig.minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> > +       sensorConfig.maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;
> > +
> > +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> >  
> >         context.activeState.agc.automatic.yTarget = effectiveYTarget();
> >  
> > 
> > -- 
> > 2.51.0
> >

Quoting Stefan Klug (2025-11-05 07:43:02)
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> Quoting Jacopo Mondi (2025-10-28 10:31:50)
> > The AgcMeanLuminance algorithm interface has a 'configure()' and
> > 'setLimits()' API. configure() doesn't actually do much if not
> > initialzing a few variables, and the Mali and IPU3 IPAs do not
> > even call it. Configuring the AGC requires calling setLimits() at
> > configure() time and at run time.
> 
> Oh I just realized, that my internal policy is to keep the libipa
> interface backwards compatible so there is no need to update to update
> other pipeline handlers (That's why the others didn't call configure()).
> Maybe I should revisit this policy... but then any change get's even
> harder to test...
> 
> > 
> > In order to prepare to differentiate between configure-time settings
> > of the Agc and run-time handling of dynamic parameters such as the
> > frame duration limits, make the configure() operation initialize
> > the Agc limits.
> 
> Yes, that makes sense.
> 
> > 
> > Update all IPAs Agc implementation deriving from AgcMeanLuminance.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/agc.cpp         | 14 +++++++---
> >  src/ipa/libipa/agc_mean_luminance.cpp   | 48 ++++++++++++++++++++++++++++++---
> >  src/ipa/libipa/agc_mean_luminance.h     | 11 +++++++-
> >  src/ipa/libipa/exposure_mode_helper.cpp | 20 ++++++++++----
> >  src/ipa/libipa/exposure_mode_helper.h   |  4 ++-
> >  src/ipa/mali-c55/algorithms/agc.cpp     | 16 ++++++-----
> >  src/ipa/rkisp1/algorithms/agc.cpp       | 15 ++++++-----
> >  7 files changed, 101 insertions(+), 27 deletions(-)
> > 
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index b0d89541da8550bd50472779da7fa8e33b96e2f0..e303d724ab165bac03ac6bb9d3891a76049cff47 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -115,9 +115,17 @@ int Agc::configure(IPAContext &context,
> >         context.activeState.agc.constraintMode = constraintModes().begin()->first;
> >         context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> >  
> > -       /* \todo Run this again when FrameDurationLimits is passed in */
> > -       setLimits(minExposureTime_, maxExposureTime_, minAnalogueGain_,
> > -                 maxAnalogueGain_, {});
> > +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> > +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> > +       sensorConfig.minExposureTime = minExposureTime_;
> > +       sensorConfig.maxExposureTime = maxExposureTime_;
> > +       sensorConfig.minAnalogueGain = minAnalogueGain_;
> > +       sensorConfig.maxAnalogueGain = maxAnalogueGain_;
> > +
> > +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> > +
> > +       /* \todo Update AGC limits when FrameDurationLimits is passed in */
> > +
> >         resetFrameCount();
> >  
> >         return 0;
> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > index 64f36bd75dd213671b5a2e6810245096ed001f21..9886864fc42514040eadb4c0b424796df1f1b6e1 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > @@ -95,6 +95,42 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> >   * \brief The luminance target for the constraint
> >   */
> >  
> > +/**
> > + * \struct AgcMeanLuminance::AgcSensorConfiguration
> > + * \brief The sensor configuration parameters
> > + *
> > + * This structure collects the sensor configuration parameters which need
> > + * to be provided to the AGC algorithm at configure() time.
> > + *
> > + * Each time configure() is called the sensor configuration need to be updated
> > + * with new parameters.

This looks like what I was working towards in 

 AgcMeanLuminance tidy up 
 https://git.uk.ideasonboard.com/kbingham/libcamera/commit/100cec322815863f9a5168d361a8118e7287e7a9

I think we should have AgcMeanLuminance able to declare what goes into
the IPA Context with a struct for each of the
  Configuration <const data after configure>
  ActiveState <dynamic updating state>
  FrameContext <per frame settings>

Then users of the AgcMeanLuminance should declare *these* instead of all
the current places where we have many different instantiations of the
same data.

--
Kieran


> 
> Reading the series I (again) realized how hard it is to follow the
> dependencies/constraints in the system.
> 
> I wonder if this should be moved to the sensor helper? In the end it is
> not a configuration of the AGC but a property of the sensor. Aside from
> a hardcoded 60ms in IPU3 I don't see that we impose additional AGC
> specific limits. I think Kieran had similar thoughts.
> 
> If we leave it in here, we could call it AgcRegulationLimits, then
> lineDuration doesn't fit the scheme... so maybe just AgcConfiguration?
> 
> But I think fetching this info from the sensor helper and configuring
> that one would be best.
> 
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::lineDuration
> > + * \brief The line duration in microseconds
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::minExposureTime
> > + * \brief The sensor minimum exposure time in microseconds
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::maxExposureTime
> > + * \brief The sensor maximum exposure time in microseconds
> 
> 
> As a first time IPA implementer I would fill this with the maximum of
> V4L2_CID_EXPOSURE converted to microseconds. Which is clearly the wrong
> thing to do. Still I wonder if this should be made extra clear but I
> have difficulties finding the right words.
> 
> Maybe "maximum achievable exposure time" - doesn't actually convey more
> information. "max exposure time the hardware can do" might make it
> easier to switch away from the exposure control. I don't know.
> 
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::minAnalogueGain
> > + * \brief The sensor minimum analogue gain absolute value
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::maxAnalogueGain
> > + * \brief The sensor maximum analogue gain absolute value
> > + */
> > +
> >  /**
> >   * \class AgcMeanLuminance
> >   * \brief A mean-based auto-exposure algorithm
> > @@ -314,17 +350,21 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)
> >  
> >  /**
> >   * \brief Configure the exposure mode helpers
> > - * \param[in] lineDuration The sensor line length
> > + * \param[in] config The sensor configuration
> >   * \param[in] sensorHelper The sensor helper
> >   *
> > - * This function configures the exposure mode helpers so they can correctly
> > + * This function configures the exposure mode helpers by providing them the
> > + * sensor configuration parameters and the sensor helper, so they can correctly
> >   * take quantization effects into account.
> >   */
> > -void AgcMeanLuminance::configure(utils::Duration lineDuration,
> > +void AgcMeanLuminance::configure(const AgcSensorConfiguration &config,
> >                                  const CameraSensorHelper *sensorHelper)
> >  {
> >         for (auto &[id, helper] : exposureModeHelpers_)
> > -               helper->configure(lineDuration, sensorHelper);
> > +               helper->configure(config.lineDuration,
> > +                                 config.minExposureTime, config.maxExposureTime,
> > +                                 config.minAnalogueGain, config.maxAnalogueGain,
> > +                                 sensorHelper);
> 
> Tying the information to the sensor helper would make the sensor helper
> mandatory, but would simplify this.
> 
> >  }
> >  
> >  /**
> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > index d7ec548e3e585e28fd03b956c8de7eaac5dc6e99..5ab75a3ac6c550a655f4bb7c8c30419ad8249183 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.h
> > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > @@ -42,7 +42,16 @@ public:
> >                 double yTarget;
> >         };
> >  
> > -       void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);
> > +       struct AgcSensorConfiguration {
> > +               utils::Duration lineDuration;
> > +               utils::Duration minExposureTime;
> > +               utils::Duration maxExposureTime;
> > +               double minAnalogueGain;
> > +               double maxAnalogueGain;
> > +       };
> > +
> > +       void configure(const AgcSensorConfiguration &config,
> > +                      const CameraSensorHelper *sensorHelper);
> >         int parseTuningData(const YamlObject &tuningData);
> >  
> >         void setExposureCompensation(double gain)
> > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> > index 29e316d9d09181cd65cb08f26babb1a680bee62a..557e14230ba4e0f120653bc09ea23a29a54cb4c8 100644
> > --- a/src/ipa/libipa/exposure_mode_helper.cpp
> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> > @@ -82,22 +82,32 @@ ExposureModeHelper::ExposureModeHelper(const Span<std::pair<utils::Duration, dou
> >  /**
> >   * \brief Configure sensor details
> >   * \param[in] lineDuration The current line length of the sensor
> > + * \param[in] minExposureTime The minimum exposure time supported
> > + * \param[in] maxExposureTime The maximum exposure time supported
> > + * \param[in] minGain The minimum analogue gain supported
> > + * \param[in] maxGain The maximum analogue gain supported
> >   * \param[in] sensorHelper The sensor helper
> >   *
> > - * This function sets the line length and sensor helper. These are used in
> > - * splitExposure() to take the quantization of the exposure and gain into
> > - * account.
> > + * This function initializes the exposure helper settings using the sensor
> > + * configuration parameters.
> >   *
> > - * When this has not been called, it is assumed that exposure is in micro second
> > - * granularity and gain has no quantization at all.
> > + * The sensor parameters' min and max limits are used in splitExposure() to take
> > + * the quantization of the exposure and gain into account.
> 
> This sentence now changed it's meaning as the min/max limits can't be
> used to take quantization into account... but I agree that without the
> quantization context in mind, this sentence came out of the blue.
> 
> >   *
> >   * ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the caller
> >   * has to ensure that sensorHelper is valid until the next call to configure().
> >   */
> >  void ExposureModeHelper::configure(utils::Duration lineDuration,
> > +                                  utils::Duration minExposureTime,
> > +                                  utils::Duration maxExposureTime,
> > +                                  double minGain, double maxGain,
> >                                    const CameraSensorHelper *sensorHelper)
> >  {
> >         lineDuration_ = lineDuration;
> > +       minExposureTime_ = minExposureTime;
> > +       maxExposureTime_ = maxExposureTime;
> > +       minGain_ = minGain;
> > +       maxGain_ = maxGain;
> 
> This now sets the same members as setLimits() so, setLimits() can't
> internally clamp to the hardware limits (it never did). When we move the
> hardware limits into the sensor helper we'd additionally gain that
> ability.
> 
> Best regards,
> Stefan
> 
> >         sensorHelper_ = sensorHelper;
> >  }
> >  
> > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
> > index 968192ddc5af768ae0de58aca6c7230c7b3bd507..bb0a692c99b3128a2ee3f15304f125e073721536 100644
> > --- a/src/ipa/libipa/exposure_mode_helper.h
> > +++ b/src/ipa/libipa/exposure_mode_helper.h
> > @@ -26,7 +26,9 @@ public:
> >         ExposureModeHelper(const Span<std::pair<utils::Duration, double>> stages);
> >         ~ExposureModeHelper() = default;
> >  
> > -       void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);
> > +       void configure(utils::Duration lineLength, utils::Duration minExposureTime,
> > +                      utils::Duration maxExposureTime, double minGain, double maxGain,
> > +                      const CameraSensorHelper *sensorHelper);
> >         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> >                        double minGain, double maxGain);
> >  
> > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
> > index f60fddac3f04fd6f09dc782e929ff1593758c29b..ccf72cc9e8c33b1a516322c2445efa1684b7f751 100644
> > --- a/src/ipa/mali-c55/algorithms/agc.cpp
> > +++ b/src/ipa/mali-c55/algorithms/agc.cpp
> > @@ -169,12 +169,16 @@ int Agc::configure(IPAContext &context,
> >         context.activeState.agc.constraintMode = constraintModes().begin()->first;
> >         context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> >  
> > -       /* \todo Run this again when FrameDurationLimits is passed in */
> > -       setLimits(context.configuration.agc.minShutterSpeed,
> > -                 context.configuration.agc.maxShutterSpeed,
> > -                 context.configuration.agc.minAnalogueGain,
> > -                 context.configuration.agc.maxAnalogueGain,
> > -                 {});
> > +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> > +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> > +       sensorConfig.minExposureTime = context.configuration.agc.minShutterSpeed;
> > +       sensorConfig.maxExposureTime = context.configuration.agc.maxShutterSpeed;
> > +       sensorConfig.minAnalogueGain = context.configuration.agc.minAnalogueGain;
> > +       sensorConfig.maxAnalogueGain = context.configuration.agc.maxAnalogueGain;
> > +
> > +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> > +
> > +       /* \todo Update AGC limits when FrameDurationLimits is passed in */
> >  
> >         resetFrameCount();
> >  
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..8c07e6a6eac091d949ad71ee967c893897313cc8 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -200,13 +200,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >         context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;
> >         context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;
> >  
> > -       AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,
> > -                                   context.camHelper.get());
> > -
> > -       setLimits(context.configuration.sensor.minExposureTime,
> > -                 context.configuration.sensor.maxExposureTime,
> > -                 context.configuration.sensor.minAnalogueGain,
> > -                 context.configuration.sensor.maxAnalogueGain, {});
> > +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> > +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> > +       sensorConfig.minExposureTime = context.configuration.sensor.minExposureTime;
> > +       sensorConfig.maxExposureTime = context.configuration.sensor.maxExposureTime;
> > +       sensorConfig.minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> > +       sensorConfig.maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;
> > +
> > +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> >  
> >         context.activeState.agc.automatic.yTarget = effectiveYTarget();
> >  
> > 
> > -- 
> > 2.51.0
> >

Hi Stefan

On Wed, Nov 05, 2025 at 08:43:02AM +0100, Stefan Klug wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> Quoting Jacopo Mondi (2025-10-28 10:31:50)
> > The AgcMeanLuminance algorithm interface has a 'configure()' and
> > 'setLimits()' API. configure() doesn't actually do much if not
> > initialzing a few variables, and the Mali and IPU3 IPAs do not
> > even call it. Configuring the AGC requires calling setLimits() at
> > configure() time and at run time.
>
> Oh I just realized, that my internal policy is to keep the libipa
> interface backwards compatible so there is no need to update to update
> other pipeline handlers (That's why the others didn't call configure()).
> Maybe I should revisit this policy... but then any change get's even
> harder to test...
>

gnn, I would do the exact opposite :)

Internal API much like the Linux kernel API are unstable by definition
and the only way to ensure we can advance them is to modify them in
all places where they are in use.

The alternative is to create a sub-optimal API in order to keep it
backward compatible. This quickly becomes a nightmare to maintain.

You're right, it will require to test changes on all users, at the
minimum compile time changes (already done by the CI) but also
functionally.

I feel we'll soon need maintainers for each platform we support in
libipa responsible for keeping the platform working as expected.

> >
> > In order to prepare to differentiate between configure-time settings
> > of the Agc and run-time handling of dynamic parameters such as the
> > frame duration limits, make the configure() operation initialize
> > the Agc limits.
>
> Yes, that makes sense.
>
> >
> > Update all IPAs Agc implementation deriving from AgcMeanLuminance.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/algorithms/agc.cpp         | 14 +++++++---
> >  src/ipa/libipa/agc_mean_luminance.cpp   | 48 ++++++++++++++++++++++++++++++---
> >  src/ipa/libipa/agc_mean_luminance.h     | 11 +++++++-
> >  src/ipa/libipa/exposure_mode_helper.cpp | 20 ++++++++++----
> >  src/ipa/libipa/exposure_mode_helper.h   |  4 ++-
> >  src/ipa/mali-c55/algorithms/agc.cpp     | 16 ++++++-----
> >  src/ipa/rkisp1/algorithms/agc.cpp       | 15 ++++++-----
> >  7 files changed, 101 insertions(+), 27 deletions(-)
> >
> > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > index b0d89541da8550bd50472779da7fa8e33b96e2f0..e303d724ab165bac03ac6bb9d3891a76049cff47 100644
> > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > @@ -115,9 +115,17 @@ int Agc::configure(IPAContext &context,
> >         context.activeState.agc.constraintMode = constraintModes().begin()->first;
> >         context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> >
> > -       /* \todo Run this again when FrameDurationLimits is passed in */
> > -       setLimits(minExposureTime_, maxExposureTime_, minAnalogueGain_,
> > -                 maxAnalogueGain_, {});
> > +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> > +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> > +       sensorConfig.minExposureTime = minExposureTime_;
> > +       sensorConfig.maxExposureTime = maxExposureTime_;
> > +       sensorConfig.minAnalogueGain = minAnalogueGain_;
> > +       sensorConfig.maxAnalogueGain = maxAnalogueGain_;
> > +
> > +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> > +
> > +       /* \todo Update AGC limits when FrameDurationLimits is passed in */
> > +
> >         resetFrameCount();
> >
> >         return 0;
> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > index 64f36bd75dd213671b5a2e6810245096ed001f21..9886864fc42514040eadb4c0b424796df1f1b6e1 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > @@ -95,6 +95,42 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> >   * \brief The luminance target for the constraint
> >   */
> >
> > +/**
> > + * \struct AgcMeanLuminance::AgcSensorConfiguration
> > + * \brief The sensor configuration parameters
> > + *
> > + * This structure collects the sensor configuration parameters which need
> > + * to be provided to the AGC algorithm at configure() time.
> > + *
> > + * Each time configure() is called the sensor configuration need to be updated
> > + * with new parameters.
>
> Reading the series I (again) realized how hard it is to follow the
> dependencies/constraints in the system.
>
> I wonder if this should be moved to the sensor helper? In the end it is
> not a configuration of the AGC but a property of the sensor. Aside from
> a hardcoded 60ms in IPU3 I don't see that we impose additional AGC
> specific limits. I think Kieran had similar thoughts.
>
> If we leave it in here, we could call it AgcRegulationLimits, then
> lineDuration doesn't fit the scheme... so maybe just AgcConfiguration?
>
> But I think fetching this info from the sensor helper and configuring
> that one would be best.
>

I'll reply to Kieran's email as well, but yes, this is where we want
to go and when Kieran showed me his branch I tried to design this so
that it can be easily replaceble by a centralized sensor helper.

> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::lineDuration
> > + * \brief The line duration in microseconds
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::minExposureTime
> > + * \brief The sensor minimum exposure time in microseconds
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::maxExposureTime
> > + * \brief The sensor maximum exposure time in microseconds
>
>
> As a first time IPA implementer I would fill this with the maximum of
> V4L2_CID_EXPOSURE converted to microseconds. Which is clearly the wrong
> thing to do. Still I wonder if this should be made extra clear but I
> have difficulties finding the right words.
>
> Maybe "maximum achievable exposure time" - doesn't actually convey more
> information. "max exposure time the hardware can do" might make it
> easier to switch away from the exposure control. I don't know.
>

This field will likely go in next version.

If the max exposure is always "max duration - margin" then it doesn't
make sense to pass it in.

> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::minAnalogueGain
> > + * \brief The sensor minimum analogue gain absolute value
> > + */
> > +
> > +/**
> > + * \var AgcMeanLuminance::AgcSensorConfiguration::maxAnalogueGain
> > + * \brief The sensor maximum analogue gain absolute value
> > + */
> > +
> >  /**
> >   * \class AgcMeanLuminance
> >   * \brief A mean-based auto-exposure algorithm
> > @@ -314,17 +350,21 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)
> >
> >  /**
> >   * \brief Configure the exposure mode helpers
> > - * \param[in] lineDuration The sensor line length
> > + * \param[in] config The sensor configuration
> >   * \param[in] sensorHelper The sensor helper
> >   *
> > - * This function configures the exposure mode helpers so they can correctly
> > + * This function configures the exposure mode helpers by providing them the
> > + * sensor configuration parameters and the sensor helper, so they can correctly
> >   * take quantization effects into account.
> >   */
> > -void AgcMeanLuminance::configure(utils::Duration lineDuration,
> > +void AgcMeanLuminance::configure(const AgcSensorConfiguration &config,
> >                                  const CameraSensorHelper *sensorHelper)
> >  {
> >         for (auto &[id, helper] : exposureModeHelpers_)
> > -               helper->configure(lineDuration, sensorHelper);
> > +               helper->configure(config.lineDuration,
> > +                                 config.minExposureTime, config.maxExposureTime,
> > +                                 config.minAnalogueGain, config.maxAnalogueGain,
> > +                                 sensorHelper);
>
> Tying the information to the sensor helper would make the sensor helper
> mandatory, but would simplify this.
>

I think we should make sensor helpers mandatory, yes.

I agree we should get those information from the unified sensor
helper, but I don't think we should delay fixing this bug (which is
not really a bug, it's a design deficiency) by requiring to introduce a
unified sensor helper first.

What I'm trying to do here is to fix this in a way that will make it
"easy" to get the same information that the IPA now feed to the AGC
from a sensor helper.

> >  }
> >
> >  /**
> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > index d7ec548e3e585e28fd03b956c8de7eaac5dc6e99..5ab75a3ac6c550a655f4bb7c8c30419ad8249183 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.h
> > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > @@ -42,7 +42,16 @@ public:
> >                 double yTarget;
> >         };
> >
> > -       void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);
> > +       struct AgcSensorConfiguration {
> > +               utils::Duration lineDuration;
> > +               utils::Duration minExposureTime;
> > +               utils::Duration maxExposureTime;
> > +               double minAnalogueGain;
> > +               double maxAnalogueGain;
> > +       };
> > +
> > +       void configure(const AgcSensorConfiguration &config,
> > +                      const CameraSensorHelper *sensorHelper);
> >         int parseTuningData(const YamlObject &tuningData);
> >
> >         void setExposureCompensation(double gain)
> > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> > index 29e316d9d09181cd65cb08f26babb1a680bee62a..557e14230ba4e0f120653bc09ea23a29a54cb4c8 100644
> > --- a/src/ipa/libipa/exposure_mode_helper.cpp
> > +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> > @@ -82,22 +82,32 @@ ExposureModeHelper::ExposureModeHelper(const Span<std::pair<utils::Duration, dou
> >  /**
> >   * \brief Configure sensor details
> >   * \param[in] lineDuration The current line length of the sensor
> > + * \param[in] minExposureTime The minimum exposure time supported
> > + * \param[in] maxExposureTime The maximum exposure time supported
> > + * \param[in] minGain The minimum analogue gain supported
> > + * \param[in] maxGain The maximum analogue gain supported
> >   * \param[in] sensorHelper The sensor helper
> >   *
> > - * This function sets the line length and sensor helper. These are used in
> > - * splitExposure() to take the quantization of the exposure and gain into
> > - * account.
> > + * This function initializes the exposure helper settings using the sensor
> > + * configuration parameters.
> >   *
> > - * When this has not been called, it is assumed that exposure is in micro second
> > - * granularity and gain has no quantization at all.
> > + * The sensor parameters' min and max limits are used in splitExposure() to take
> > + * the quantization of the exposure and gain into account.
>
> This sentence now changed it's meaning as the min/max limits can't be
> used to take quantization into account... but I agree that without the
> quantization context in mind, this sentence came out of the blue.
>

How should I re-phrase it ?

> >   *
> >   * ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the caller
> >   * has to ensure that sensorHelper is valid until the next call to configure().
> >   */
> >  void ExposureModeHelper::configure(utils::Duration lineDuration,
> > +                                  utils::Duration minExposureTime,
> > +                                  utils::Duration maxExposureTime,
> > +                                  double minGain, double maxGain,
> >                                    const CameraSensorHelper *sensorHelper)
> >  {
> >         lineDuration_ = lineDuration;
> > +       minExposureTime_ = minExposureTime;
> > +       maxExposureTime_ = maxExposureTime;
> > +       minGain_ = minGain;
> > +       maxGain_ = maxGain;
>
> This now sets the same members as setLimits() so, setLimits() can't
> internally clamp to the hardware limits (it never did). When we move the
> hardware limits into the sensor helper we'd additionally gain that
> ability.

In the next patches configure() and setLimits() will use the same
internal routine, and we can centralize hw constraints handling there.

Thanks
  j
>
> Best regards,
> Stefan
>
> >         sensorHelper_ = sensorHelper;
> >  }
> >
> > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
> > index 968192ddc5af768ae0de58aca6c7230c7b3bd507..bb0a692c99b3128a2ee3f15304f125e073721536 100644
> > --- a/src/ipa/libipa/exposure_mode_helper.h
> > +++ b/src/ipa/libipa/exposure_mode_helper.h
> > @@ -26,7 +26,9 @@ public:
> >         ExposureModeHelper(const Span<std::pair<utils::Duration, double>> stages);
> >         ~ExposureModeHelper() = default;
> >
> > -       void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);
> > +       void configure(utils::Duration lineLength, utils::Duration minExposureTime,
> > +                      utils::Duration maxExposureTime, double minGain, double maxGain,
> > +                      const CameraSensorHelper *sensorHelper);
> >         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> >                        double minGain, double maxGain);
> >
> > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
> > index f60fddac3f04fd6f09dc782e929ff1593758c29b..ccf72cc9e8c33b1a516322c2445efa1684b7f751 100644
> > --- a/src/ipa/mali-c55/algorithms/agc.cpp
> > +++ b/src/ipa/mali-c55/algorithms/agc.cpp
> > @@ -169,12 +169,16 @@ int Agc::configure(IPAContext &context,
> >         context.activeState.agc.constraintMode = constraintModes().begin()->first;
> >         context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> >
> > -       /* \todo Run this again when FrameDurationLimits is passed in */
> > -       setLimits(context.configuration.agc.minShutterSpeed,
> > -                 context.configuration.agc.maxShutterSpeed,
> > -                 context.configuration.agc.minAnalogueGain,
> > -                 context.configuration.agc.maxAnalogueGain,
> > -                 {});
> > +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> > +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> > +       sensorConfig.minExposureTime = context.configuration.agc.minShutterSpeed;
> > +       sensorConfig.maxExposureTime = context.configuration.agc.maxShutterSpeed;
> > +       sensorConfig.minAnalogueGain = context.configuration.agc.minAnalogueGain;
> > +       sensorConfig.maxAnalogueGain = context.configuration.agc.maxAnalogueGain;
> > +
> > +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> > +
> > +       /* \todo Update AGC limits when FrameDurationLimits is passed in */
> >
> >         resetFrameCount();
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..8c07e6a6eac091d949ad71ee967c893897313cc8 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -200,13 +200,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >         context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;
> >         context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;
> >
> > -       AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,
> > -                                   context.camHelper.get());
> > -
> > -       setLimits(context.configuration.sensor.minExposureTime,
> > -                 context.configuration.sensor.maxExposureTime,
> > -                 context.configuration.sensor.minAnalogueGain,
> > -                 context.configuration.sensor.maxAnalogueGain, {});
> > +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> > +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> > +       sensorConfig.minExposureTime = context.configuration.sensor.minExposureTime;
> > +       sensorConfig.maxExposureTime = context.configuration.sensor.maxExposureTime;
> > +       sensorConfig.minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> > +       sensorConfig.maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;
> > +
> > +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> >
> >         context.activeState.agc.automatic.yTarget = effectiveYTarget();
> >
> >
> > --
> > 2.51.0
> >

Hi Kieran

On Wed, Nov 05, 2025 at 08:57:25AM +0000, Kieran Bingham wrote:
> Quoting Stefan Klug (2025-11-05 07:43:02)
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > Quoting Jacopo Mondi (2025-10-28 10:31:50)
> > > The AgcMeanLuminance algorithm interface has a 'configure()' and
> > > 'setLimits()' API. configure() doesn't actually do much if not
> > > initialzing a few variables, and the Mali and IPU3 IPAs do not
> > > even call it. Configuring the AGC requires calling setLimits() at
> > > configure() time and at run time.
> >
> > Oh I just realized, that my internal policy is to keep the libipa
> > interface backwards compatible so there is no need to update to update
> > other pipeline handlers (That's why the others didn't call configure()).
> > Maybe I should revisit this policy... but then any change get's even
> > harder to test...
> >
> > >
> > > In order to prepare to differentiate between configure-time settings
> > > of the Agc and run-time handling of dynamic parameters such as the
> > > frame duration limits, make the configure() operation initialize
> > > the Agc limits.
> >
> > Yes, that makes sense.
> >
> > >
> > > Update all IPAs Agc implementation deriving from AgcMeanLuminance.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  src/ipa/ipu3/algorithms/agc.cpp         | 14 +++++++---
> > >  src/ipa/libipa/agc_mean_luminance.cpp   | 48 ++++++++++++++++++++++++++++++---
> > >  src/ipa/libipa/agc_mean_luminance.h     | 11 +++++++-
> > >  src/ipa/libipa/exposure_mode_helper.cpp | 20 ++++++++++----
> > >  src/ipa/libipa/exposure_mode_helper.h   |  4 ++-
> > >  src/ipa/mali-c55/algorithms/agc.cpp     | 16 ++++++-----
> > >  src/ipa/rkisp1/algorithms/agc.cpp       | 15 ++++++-----
> > >  7 files changed, 101 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> > > index b0d89541da8550bd50472779da7fa8e33b96e2f0..e303d724ab165bac03ac6bb9d3891a76049cff47 100644
> > > --- a/src/ipa/ipu3/algorithms/agc.cpp
> > > +++ b/src/ipa/ipu3/algorithms/agc.cpp
> > > @@ -115,9 +115,17 @@ int Agc::configure(IPAContext &context,
> > >         context.activeState.agc.constraintMode = constraintModes().begin()->first;
> > >         context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> > >
> > > -       /* \todo Run this again when FrameDurationLimits is passed in */
> > > -       setLimits(minExposureTime_, maxExposureTime_, minAnalogueGain_,
> > > -                 maxAnalogueGain_, {});
> > > +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> > > +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> > > +       sensorConfig.minExposureTime = minExposureTime_;
> > > +       sensorConfig.maxExposureTime = maxExposureTime_;
> > > +       sensorConfig.minAnalogueGain = minAnalogueGain_;
> > > +       sensorConfig.maxAnalogueGain = maxAnalogueGain_;
> > > +
> > > +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> > > +
> > > +       /* \todo Update AGC limits when FrameDurationLimits is passed in */
> > > +
> > >         resetFrameCount();
> > >
> > >         return 0;
> > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > > index 64f36bd75dd213671b5a2e6810245096ed001f21..9886864fc42514040eadb4c0b424796df1f1b6e1 100644
> > > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > > @@ -95,6 +95,42 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> > >   * \brief The luminance target for the constraint
> > >   */
> > >
> > > +/**
> > > + * \struct AgcMeanLuminance::AgcSensorConfiguration
> > > + * \brief The sensor configuration parameters
> > > + *
> > > + * This structure collects the sensor configuration parameters which need
> > > + * to be provided to the AGC algorithm at configure() time.
> > > + *
> > > + * Each time configure() is called the sensor configuration need to be updated
> > > + * with new parameters.
> >
> > Reading the series I (again) realized how hard it is to follow the
> > dependencies/constraints in the system.
> >
> > I wonder if this should be moved to the sensor helper? In the end it is
> > not a configuration of the AGC but a property of the sensor. Aside from
> > a hardcoded 60ms in IPU3 I don't see that we impose additional AGC
> > specific limits. I think Kieran had similar thoughts.
> >
> > If we leave it in here, we could call it AgcRegulationLimits, then
> > lineDuration doesn't fit the scheme... so maybe just AgcConfiguration?
> >
> > But I think fetching this info from the sensor helper and configuring
> > that one would be best.
>
>
> Yes, I think all tasks related to the CameraSensor should be in the
> CameraSensorHelper - *not* replicated across algorithms and IPAs.
> Especially in this topic where we're talking about how should we
> configure and manage the limits of the CameraSensor - that should
> definitely all be common in my view.
>
>  ipa: Move libipa camera configuration to helpers
>  - https://git.uk.ideasonboard.com/kbingham/libcamera/commit/5c711d2a896d5a892ef61f263d623a440b58b33d
>
> I think CameraSensorHelper should manage the v4l2 conversions, and
> provide an interface that an Agc can ask "Hey what limits can I use"
>
> Then the *common* CameraSensorHelper will report that based on any
> manual control parsing.

I agree.

Once you showed me your branch I tried to design this in a way that
will make it easier for the AgcMeanLuminance and ExposureModeHelper
classes to get the same information that are currently fed by the IPA
module from a CameraSensorHelper instead.

As said in reply to Stefan please let me know if something goes in a
direction that will make it harder to get where we want to go, but I
don't this we should delay fixing the issue I'm trying to fix until we
don't have the CameraSensorHelper unification in place.

>
> I'd also like to introduce a new wrapper around min/max for this topic
> because they're so intrinsically tied together and when I worked through
> this I was really bothered with always having to move/manage them as a
> pair :-)
>
>  ipa: libipa: Provide a Bounds type to pass min/max
>  - https://git.uk.ideasonboard.com/kbingham/libcamera/commit/9e85cd913213e16ea20c614003ce421ab323b9ac
>
>
>
> Those commits are part of my slow progressing soft-libipa branch:
>  https://git.uk.ideasonboard.com/kbingham/libcamera/pulls/18
>
> where I'm working towards making it easier to integrate AgcMeanLuminance
> in new IPA modules - and factor out all the repeated or common code that
> keeps getting duplicated.

I hope this series helps getting there

>
> --
> Kieran
>
>
> >
> > > + */
> > > +
> > > +/**
> > > + * \var AgcMeanLuminance::AgcSensorConfiguration::lineDuration
> > > + * \brief The line duration in microseconds
> > > + */
> > > +
> > > +/**
> > > + * \var AgcMeanLuminance::AgcSensorConfiguration::minExposureTime
> > > + * \brief The sensor minimum exposure time in microseconds
> > > + */
> > > +
> > > +/**
> > > + * \var AgcMeanLuminance::AgcSensorConfiguration::maxExposureTime
> > > + * \brief The sensor maximum exposure time in microseconds
> >
> >
> > As a first time IPA implementer I would fill this with the maximum of
> > V4L2_CID_EXPOSURE converted to microseconds. Which is clearly the wrong
> > thing to do. Still I wonder if this should be made extra clear but I
> > have difficulties finding the right words.
> >
> > Maybe "maximum achievable exposure time" - doesn't actually convey more
> > information. "max exposure time the hardware can do" might make it
> > easier to switch away from the exposure control. I don't know.
> >
> > > + */
> > > +
> > > +/**
> > > + * \var AgcMeanLuminance::AgcSensorConfiguration::minAnalogueGain
> > > + * \brief The sensor minimum analogue gain absolute value
> > > + */
> > > +
> > > +/**
> > > + * \var AgcMeanLuminance::AgcSensorConfiguration::maxAnalogueGain
> > > + * \brief The sensor maximum analogue gain absolute value
> > > + */
> > > +
> > >  /**
> > >   * \class AgcMeanLuminance
> > >   * \brief A mean-based auto-exposure algorithm
> > > @@ -314,17 +350,21 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)
> > >
> > >  /**
> > >   * \brief Configure the exposure mode helpers
> > > - * \param[in] lineDuration The sensor line length
> > > + * \param[in] config The sensor configuration
> > >   * \param[in] sensorHelper The sensor helper
> > >   *
> > > - * This function configures the exposure mode helpers so they can correctly
> > > + * This function configures the exposure mode helpers by providing them the
> > > + * sensor configuration parameters and the sensor helper, so they can correctly
> > >   * take quantization effects into account.
> > >   */
> > > -void AgcMeanLuminance::configure(utils::Duration lineDuration,
> > > +void AgcMeanLuminance::configure(const AgcSensorConfiguration &config,
> > >                                  const CameraSensorHelper *sensorHelper)
> > >  {
> > >         for (auto &[id, helper] : exposureModeHelpers_)
> > > -               helper->configure(lineDuration, sensorHelper);
> > > +               helper->configure(config.lineDuration,
> > > +                                 config.minExposureTime, config.maxExposureTime,
> > > +                                 config.minAnalogueGain, config.maxAnalogueGain,
> > > +                                 sensorHelper);
> >
> > Tying the information to the sensor helper would make the sensor helper
> > mandatory, but would simplify this.
> >
> > >  }
> > >
> > >  /**
> > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > > index d7ec548e3e585e28fd03b956c8de7eaac5dc6e99..5ab75a3ac6c550a655f4bb7c8c30419ad8249183 100644
> > > --- a/src/ipa/libipa/agc_mean_luminance.h
> > > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > > @@ -42,7 +42,16 @@ public:
> > >                 double yTarget;
> > >         };
> > >
> > > -       void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);
> > > +       struct AgcSensorConfiguration {
> > > +               utils::Duration lineDuration;
> > > +               utils::Duration minExposureTime;
> > > +               utils::Duration maxExposureTime;
> > > +               double minAnalogueGain;
> > > +               double maxAnalogueGain;
> > > +       };
> > > +
> > > +       void configure(const AgcSensorConfiguration &config,
> > > +                      const CameraSensorHelper *sensorHelper);
> > >         int parseTuningData(const YamlObject &tuningData);
> > >
> > >         void setExposureCompensation(double gain)
> > > diff --git a/src/ipa/libipa/exposure_mode_helper.cpp b/src/ipa/libipa/exposure_mode_helper.cpp
> > > index 29e316d9d09181cd65cb08f26babb1a680bee62a..557e14230ba4e0f120653bc09ea23a29a54cb4c8 100644
> > > --- a/src/ipa/libipa/exposure_mode_helper.cpp
> > > +++ b/src/ipa/libipa/exposure_mode_helper.cpp
> > > @@ -82,22 +82,32 @@ ExposureModeHelper::ExposureModeHelper(const Span<std::pair<utils::Duration, dou
> > >  /**
> > >   * \brief Configure sensor details
> > >   * \param[in] lineDuration The current line length of the sensor
> > > + * \param[in] minExposureTime The minimum exposure time supported
> > > + * \param[in] maxExposureTime The maximum exposure time supported
> > > + * \param[in] minGain The minimum analogue gain supported
> > > + * \param[in] maxGain The maximum analogue gain supported
> > >   * \param[in] sensorHelper The sensor helper
> > >   *
> > > - * This function sets the line length and sensor helper. These are used in
> > > - * splitExposure() to take the quantization of the exposure and gain into
> > > - * account.
> > > + * This function initializes the exposure helper settings using the sensor
> > > + * configuration parameters.
> > >   *
> > > - * When this has not been called, it is assumed that exposure is in micro second
> > > - * granularity and gain has no quantization at all.
> > > + * The sensor parameters' min and max limits are used in splitExposure() to take
> > > + * the quantization of the exposure and gain into account.
> >
> > This sentence now changed it's meaning as the min/max limits can't be
> > used to take quantization into account... but I agree that without the
> > quantization context in mind, this sentence came out of the blue.
>
>
> Do we need to keep a quantized number of lines associated with its'
> utils::Duration? if so I could further abstract Quantised to support
> Duration types as well as floats.
>
>
>
> >
> > >   *
> > >   * ExposureModeHelper keeps a pointer to the CameraSensorHelper, so the caller
> > >   * has to ensure that sensorHelper is valid until the next call to configure().
> > >   */
> > >  void ExposureModeHelper::configure(utils::Duration lineDuration,
> > > +                                  utils::Duration minExposureTime,
> > > +                                  utils::Duration maxExposureTime,
>
> This is why I'd like to introduce
>
> https://git.uk.ideasonboard.com/kbingham/libcamera/commit/9e85cd913213e16ea20c614003ce421ab323b9ac
>
> > > +                                  double minGain, double maxGain,
>
>
> So this is instead:
>
> 	configure(utils::Duration lineDuration,
> 		  Bounds<utils::Duration> exposureLimits,
> 	          Bounds<double> gainLimits)
>
> Is the only reason we pass lineDuration around because the Agc has to do
> quantization based on it ?
>
> I think that's probably something that should also get deferred into the
> CameraSensorHelper so that all control of the sensor calculations are
> common.
>
>
> > >                                    const CameraSensorHelper *sensorHelper)
> > >  {
> > >         lineDuration_ = lineDuration;
> > > +       minExposureTime_ = minExposureTime;
> > > +       maxExposureTime_ = maxExposureTime;
> > > +       minGain_ = minGain;
> > > +       maxGain_ = maxGain;
> >
> > This now sets the same members as setLimits() so, setLimits() can't
> > internally clamp to the hardware limits (it never did). When we move the
> > hardware limits into the sensor helper we'd additionally gain that
> > ability.
> >
> > Best regards,
> > Stefan
> >
> > >         sensorHelper_ = sensorHelper;
> > >  }
> > >
> > > diff --git a/src/ipa/libipa/exposure_mode_helper.h b/src/ipa/libipa/exposure_mode_helper.h
> > > index 968192ddc5af768ae0de58aca6c7230c7b3bd507..bb0a692c99b3128a2ee3f15304f125e073721536 100644
> > > --- a/src/ipa/libipa/exposure_mode_helper.h
> > > +++ b/src/ipa/libipa/exposure_mode_helper.h
> > > @@ -26,7 +26,9 @@ public:
> > >         ExposureModeHelper(const Span<std::pair<utils::Duration, double>> stages);
> > >         ~ExposureModeHelper() = default;
> > >
> > > -       void configure(utils::Duration lineLength, const CameraSensorHelper *sensorHelper);
> > > +       void configure(utils::Duration lineLength, utils::Duration minExposureTime,
> > > +                      utils::Duration maxExposureTime, double minGain, double maxGain,
> > > +                      const CameraSensorHelper *sensorHelper);
> > >         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> > >                        double minGain, double maxGain);
> > >
> > > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
> > > index f60fddac3f04fd6f09dc782e929ff1593758c29b..ccf72cc9e8c33b1a516322c2445efa1684b7f751 100644
> > > --- a/src/ipa/mali-c55/algorithms/agc.cpp
> > > +++ b/src/ipa/mali-c55/algorithms/agc.cpp
> > > @@ -169,12 +169,16 @@ int Agc::configure(IPAContext &context,
> > >         context.activeState.agc.constraintMode = constraintModes().begin()->first;
> > >         context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> > >
> > > -       /* \todo Run this again when FrameDurationLimits is passed in */
> > > -       setLimits(context.configuration.agc.minShutterSpeed,
> > > -                 context.configuration.agc.maxShutterSpeed,
> > > -                 context.configuration.agc.minAnalogueGain,
> > > -                 context.configuration.agc.maxAnalogueGain,
> > > -                 {});
> > > +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> > > +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> > > +       sensorConfig.minExposureTime = context.configuration.agc.minShutterSpeed;
> > > +       sensorConfig.maxExposureTime = context.configuration.agc.maxShutterSpeed;
> > > +       sensorConfig.minAnalogueGain = context.configuration.agc.minAnalogueGain;
> > > +       sensorConfig.maxAnalogueGain = context.configuration.agc.maxAnalogueGain;
> > > +
> > > +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> > > +
> > > +       /* \todo Update AGC limits when FrameDurationLimits is passed in */
> > >
> > >         resetFrameCount();
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index f5a3c917cb6909f6ef918e5ee8e46cf97ba55010..8c07e6a6eac091d949ad71ee967c893897313cc8 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -200,13 +200,14 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > >         context.configuration.agc.measureWindow.h_size = configInfo.outputSize.width;
> > >         context.configuration.agc.measureWindow.v_size = configInfo.outputSize.height;
> > >
> > > -       AgcMeanLuminance::configure(context.configuration.sensor.lineDuration,
> > > -                                   context.camHelper.get());
> > > -
> > > -       setLimits(context.configuration.sensor.minExposureTime,
> > > -                 context.configuration.sensor.maxExposureTime,
> > > -                 context.configuration.sensor.minAnalogueGain,
> > > -                 context.configuration.sensor.maxAnalogueGain, {});
> > > +       AgcMeanLuminance::AgcSensorConfiguration sensorConfig;
> > > +       sensorConfig.lineDuration = context.configuration.sensor.lineDuration;
> > > +       sensorConfig.minExposureTime = context.configuration.sensor.minExposureTime;
> > > +       sensorConfig.maxExposureTime = context.configuration.sensor.maxExposureTime;
> > > +       sensorConfig.minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> > > +       sensorConfig.maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;
> > > +
> > > +       AgcMeanLuminance::configure(sensorConfig, context.camHelper.get());
> > >
> > >         context.activeState.agc.automatic.yTarget = effectiveYTarget();
> > >
> > >
> > > --
> > > 2.51.0
> > >