[v8.1,05/12] ipa: raspberry: Port to the new AEGC controls

22579 diff mbox series

From: Jacopo Mondi <jacopo@jmondi.org>

The newly introduced controls to drive the AEGC algorithm allow
to control the computation of the exposure time and analogue gain
separately.

The RPi AEGC implementation already computes the shutter and gain
values separately but does not expose separate functions to control
them.

Augment the AgcAlgorithm interface to allow pausing/resuming the shutter
and gain automatic computations separately and plumb them to the newly
introduced controls.

Add safety checks to ignore ExposureTime and AnalogueGain values if the
algorithms are not paused, and report the correct AeState value by
checking if both algorithms have been paused or if they have converged.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

---
Changes in v8.1:
- fix the racy check in handling the auto/manual controls and the manual
  control value control

Changes in v8:
- add auto as default value for ExposureMode and AnalogueGainMode
- add todo for control processing order

No change in v7

No change in v6

No change in v5

Changes in v4:
- s/shutter/exposure/

Changes in v3:
- recovered from 2-year-old bitrot
---
 src/ipa/rpi/common/ipa_base.cpp            | 106 +++++++++++++++++----
 src/ipa/rpi/controller/agc_algorithm.h     |   8 +-
 src/ipa/rpi/controller/rpi/agc.cpp         |  52 ++++++++--
 src/ipa/rpi/controller/rpi/agc.h           |   8 +-
 src/ipa/rpi/controller/rpi/agc_channel.cpp |  24 ++++-
 src/ipa/rpi/controller/rpi/agc_channel.h   |   8 +-
 6 files changed, 171 insertions(+), 35 deletions(-)

Hi Paul,

On Tue, 14 Jan 2025 at 21:36, Paul Elder <paul.elder@ideasonboard.com> wrote:
>
> From: Jacopo Mondi <jacopo@jmondi.org>
>
> The newly introduced controls to drive the AEGC algorithm allow
> to control the computation of the exposure time and analogue gain
> separately.
>
> The RPi AEGC implementation already computes the shutter and gain
> values separately but does not expose separate functions to control
> them.
>
> Augment the AgcAlgorithm interface to allow pausing/resuming the shutter
> and gain automatic computations separately and plumb them to the newly
> introduced controls.
>
> Add safety checks to ignore ExposureTime and AnalogueGain values if the
> algorithms are not paused, and report the correct AeState value by
> checking if both algorithms have been paused or if they have converged.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
>

Thanks for the update, this looks much better!

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

Before merging, I would like an ack by @David Plowman if he's fine with this.

> ---
> Changes in v8.1:
> - fix the racy check in handling the auto/manual controls and the manual
>   control value control
>
> Changes in v8:
> - add auto as default value for ExposureMode and AnalogueGainMode
> - add todo for control processing order
>
> No change in v7
>
> No change in v6
>
> No change in v5
>
> Changes in v4:
> - s/shutter/exposure/
>
> Changes in v3:
> - recovered from 2-year-old bitrot
> ---
>  src/ipa/rpi/common/ipa_base.cpp            | 106 +++++++++++++++++----
>  src/ipa/rpi/controller/agc_algorithm.h     |   8 +-
>  src/ipa/rpi/controller/rpi/agc.cpp         |  52 ++++++++--
>  src/ipa/rpi/controller/rpi/agc.h           |   8 +-
>  src/ipa/rpi/controller/rpi/agc_channel.cpp |  24 ++++-
>  src/ipa/rpi/controller/rpi/agc_channel.h   |   8 +-
>  6 files changed, 171 insertions(+), 35 deletions(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 6ff1e22b1..78206bb4a 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -55,8 +55,15 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
>
>  /* List of controls handled by the Raspberry Pi IPA */
>  const ControlInfoMap::Map ipaControls{
> -       { &controls::AeEnable, ControlInfo(false, true) },
> +       { &controls::ExposureTimeMode,
> +         ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> +                     static_cast<int32_t>(controls::ExposureTimeModeManual),
> +                     static_cast<int32_t>(controls::ExposureTimeModeAuto)) },
>         { &controls::ExposureTime, ControlInfo(0, 66666) },
> +       { &controls::AnalogueGainMode,
> +         ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> +                     static_cast<int32_t>(controls::AnalogueGainModeManual),
> +                     static_cast<int32_t>(controls::AnalogueGainModeAuto)) },
>         { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
>         { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
>         { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> @@ -749,6 +756,46 @@ void IpaBase::applyControls(const ControlList &controls)
>                         af->setMode(mode->second);
>         }
>
> +       /*
> +        * Because some AE controls are mode-specific, handle the AE-related
> +        * mode changes first.
> +        */
> +       if (controls.contains(controls::EXPOSURE_TIME_MODE)) {
> +               RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +                       controller_.getAlgorithm("agc"));
> +               if (!agc) {
> +                       LOG(IPARPI, Warning)
> +                               << "Could not set EXPOSURE_TIME_MODE - no AGC algorithm";
> +                       break;
> +               }
> +
> +               if (ctrl.second.get<int32_t>() == controls::ExposureTimeModeManual)
> +                       agc->disableAutoExposure();
> +               else
> +                       agc->enableAutoExposure();
> +
> +               libcameraMetadata_.set(controls::ExposureTimeMode,
> +                                      ctrl.second.get<int32_t>());
> +       }
> +
> +       if (controls.contains(controls::ANALOGUE_GAIN_MODE)) {
> +               RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +                       controller_.getAlgorithm("agc"));
> +               if (!agc) {
> +                       LOG(IPARPI, Warning)
> +                               << "Could not set ANALOGUE_GAIN_MODE - no AGC algorithm";
> +                       break;
> +               }
> +
> +               if (ctrl.second.get<int32_t>() == controls::AnalogueGainModeManual)
> +                       agc->disableAutoGain();
> +               else
> +                       agc->enableAutoGain();
> +
> +               libcameraMetadata_.set(controls::AnalogueGainMode,
> +                                      ctrl.second.get<int32_t>());
> +       }
> +
>         /* Iterate over controls */
>         for (auto const &ctrl : controls) {
>                 LOG(IPARPI, Debug) << "Request ctrl: "
> @@ -756,23 +803,8 @@ void IpaBase::applyControls(const ControlList &controls)
>                                    << " = " << ctrl.second.toString();
>
>                 switch (ctrl.first) {
> -               case controls::AE_ENABLE: {
> -                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> -                               controller_.getAlgorithm("agc"));
> -                       if (!agc) {
> -                               LOG(IPARPI, Warning)
> -                                       << "Could not set AE_ENABLE - no AGC algorithm";
> -                               break;
> -                       }
> -
> -                       if (ctrl.second.get<bool>() == false)
> -                               agc->disableAuto();
> -                       else
> -                               agc->enableAuto();
> -
> -                       libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>());
> -                       break;
> -               }
> +               case controls::EXPOSURE_TIME_MODE:
> +                       break; /* We already handled this one above */
>
>                 case controls::EXPOSURE_TIME: {
>                         RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> @@ -783,6 +815,13 @@ void IpaBase::applyControls(const ControlList &controls)
>                                 break;
>                         }
>
> +                       /*
> +                        * Ignore manual exposure time when the auto exposure
> +                        * algorithm is running.
> +                        */
> +                       if (agc->autoExposureEnabled())
> +                               break;
> +
>                         /* The control provides units of microseconds. */
>                         agc->setFixedExposureTime(0, ctrl.second.get<int32_t>() * 1.0us);
>
> @@ -790,6 +829,9 @@ void IpaBase::applyControls(const ControlList &controls)
>                         break;
>                 }
>
> +               case controls::ANALOGUE_GAIN_MODE:
> +                       break; /* We already handled this one above */
> +
>                 case controls::ANALOGUE_GAIN: {
>                         RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>                                 controller_.getAlgorithm("agc"));
> @@ -799,6 +841,13 @@ void IpaBase::applyControls(const ControlList &controls)
>                                 break;
>                         }
>
> +                       /*
> +                        * Ignore manual analogue gain value when the auto gain
> +                        * algorithm is running.
> +                        */
> +                       if (agc->autoGainEnabled())
> +                               break;
> +
>                         agc->setFixedAnalogueGain(0, ctrl.second.get<float>());
>
>                         libcameraMetadata_.set(controls::AnalogueGain,
> @@ -855,6 +904,13 @@ void IpaBase::applyControls(const ControlList &controls)
>                                 break;
>                         }
>
> +                       /*
> +                        * Ignore AE_EXPOSURE_MODE if the shutter or the gain
> +                        * are in auto mode.
> +                        */
> +                       if (agc->autoExposureEnabled() || agc->autoGainEnabled())
> +                               break;
> +
>                         int32_t idx = ctrl.second.get<int32_t>();
>                         if (ExposureModeTable.count(idx)) {
>                                 agc->setExposureMode(ExposureModeTable.at(idx));
> @@ -1334,9 +1390,19 @@ void IpaBase::reportMetadata(unsigned int ipaContext)
>         }
>
>         AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status");
> -       if (agcPrepareStatus) {
> -               libcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);
> +       if (agcPrepareStatus)
>                 libcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);
> +
> +       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> +               controller_.getAlgorithm("agc"));
> +       if (agc) {
> +               if (!agc->autoExposureEnabled() && !agc->autoGainEnabled())
> +                       libcameraMetadata_.set(controls::AeState, controls::AeStateIdle);
> +               else if (agcPrepareStatus)
> +                       libcameraMetadata_.set(controls::AeState,
> +                                              agcPrepareStatus->locked
> +                                                      ? controls::AeStateConverged
> +                                                      : controls::AeStateSearching);
>         }
>
>         LuxStatus *luxStatus = rpiMetadata.getLocked<LuxStatus>("lux.status");
> diff --git a/src/ipa/rpi/controller/agc_algorithm.h b/src/ipa/rpi/controller/agc_algorithm.h
> index c97828577..fdaa10e6c 100644
> --- a/src/ipa/rpi/controller/agc_algorithm.h
> +++ b/src/ipa/rpi/controller/agc_algorithm.h
> @@ -30,8 +30,12 @@ public:
>         virtual void setMeteringMode(std::string const &meteringModeName) = 0;
>         virtual void setExposureMode(std::string const &exposureModeName) = 0;
>         virtual void setConstraintMode(std::string const &contraintModeName) = 0;
> -       virtual void enableAuto() = 0;
> -       virtual void disableAuto() = 0;
> +       virtual void enableAutoExposure() = 0;
> +       virtual void disableAutoExposure() = 0;
> +       virtual bool autoExposureEnabled() const = 0;
> +       virtual void enableAutoGain() = 0;
> +       virtual void disableAutoGain() = 0;
> +       virtual bool autoGainEnabled() const = 0;
>         virtual void setActiveChannels(const std::vector<unsigned int> &activeChannels) = 0;
>  };
>
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index c48fdf156..02bfdb4a5 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -74,22 +74,62 @@ int Agc::checkChannel(unsigned int channelIndex) const
>         return 0;
>  }
>
> -void Agc::disableAuto()
> +void Agc::disableAutoExposure()
>  {
> -       LOG(RPiAgc, Debug) << "disableAuto";
> +       LOG(RPiAgc, Debug) << "disableAutoExposure";
>
>         /* All channels are enabled/disabled together. */
>         for (auto &data : channelData_)
> -               data.channel.disableAuto();
> +               data.channel.disableAutoExposure();
>  }
>
> -void Agc::enableAuto()
> +void Agc::enableAutoExposure()
>  {
> -       LOG(RPiAgc, Debug) << "enableAuto";
> +       LOG(RPiAgc, Debug) << "enableAutoExposure";
>
>         /* All channels are enabled/disabled together. */
>         for (auto &data : channelData_)
> -               data.channel.enableAuto();
> +               data.channel.enableAutoExposure();
> +}
> +
> +bool Agc::autoExposureEnabled() const
> +{
> +       LOG(RPiAgc, Debug) << "autoExposureEnabled";
> +
> +       /*
> +        * We always have at least one channel, and since all channels are
> +        * enabled and disabled together we can simply check the first one.
> +        */
> +       return channelData_[0].channel.autoExposureEnabled();
> +}
> +
> +void Agc::disableAutoGain()
> +{
> +       LOG(RPiAgc, Debug) << "disableAutoGain";
> +
> +       /* All channels are enabled/disabled together. */
> +       for (auto &data : channelData_)
> +               data.channel.disableAutoGain();
> +}
> +
> +void Agc::enableAutoGain()
> +{
> +       LOG(RPiAgc, Debug) << "enableAutoGain";
> +
> +       /* All channels are enabled/disabled together. */
> +       for (auto &data : channelData_)
> +               data.channel.enableAutoGain();
> +}
> +
> +bool Agc::autoGainEnabled() const
> +{
> +       LOG(RPiAgc, Debug) << "autoGainEnabled";
> +
> +       /*
> +        * We always have at least one channel, and since all channels are
> +        * enabled and disabled together we can simply check the first one.
> +        */
> +       return channelData_[0].channel.autoGainEnabled();
>  }
>
>  unsigned int Agc::getConvergenceFrames() const
> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h
> index 3aca000bb..c3a940bf6 100644
> --- a/src/ipa/rpi/controller/rpi/agc.h
> +++ b/src/ipa/rpi/controller/rpi/agc.h
> @@ -40,8 +40,12 @@ public:
>         void setMeteringMode(std::string const &meteringModeName) override;
>         void setExposureMode(std::string const &exposureModeName) override;
>         void setConstraintMode(std::string const &contraintModeName) override;
> -       void enableAuto() override;
> -       void disableAuto() override;
> +       void enableAutoExposure() override;
> +       void disableAutoExposure() override;
> +       bool autoExposureEnabled() const override;
> +       void enableAutoGain() override;
> +       void disableAutoGain() override;
> +       bool autoGainEnabled() const override;
>         void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
>         void prepare(Metadata *imageMetadata) override;
>         void process(StatisticsPtr &stats, Metadata *imageMetadata) override;
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> index 79c459735..e79184b7a 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> @@ -319,18 +319,36 @@ int AgcChannel::read(const libcamera::YamlObject &params,
>         return 0;
>  }
>
> -void AgcChannel::disableAuto()
> +void AgcChannel::disableAutoExposure()
>  {
>         fixedExposureTime_ = status_.exposureTime;
> -       fixedAnalogueGain_ = status_.analogueGain;
>  }
>
> -void AgcChannel::enableAuto()
> +void AgcChannel::enableAutoExposure()
>  {
>         fixedExposureTime_ = 0s;
> +}
> +
> +bool AgcChannel::autoExposureEnabled() const
> +{
> +       return fixedExposureTime_ == 0s;
> +}
> +
> +void AgcChannel::disableAutoGain()
> +{
> +       fixedAnalogueGain_ = status_.analogueGain;
> +}
> +
> +void AgcChannel::enableAutoGain()
> +{
>         fixedAnalogueGain_ = 0;
>  }
>
> +bool AgcChannel::autoGainEnabled() const
> +{
> +       return fixedAnalogueGain_ == 0;
> +}
> +
>  unsigned int AgcChannel::getConvergenceFrames() const
>  {
>         /*
> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h
> index 734e5efd3..fa697e6fa 100644
> --- a/src/ipa/rpi/controller/rpi/agc_channel.h
> +++ b/src/ipa/rpi/controller/rpi/agc_channel.h
> @@ -96,8 +96,12 @@ public:
>         void setMeteringMode(std::string const &meteringModeName);
>         void setExposureMode(std::string const &exposureModeName);
>         void setConstraintMode(std::string const &contraintModeName);
> -       void enableAuto();
> -       void disableAuto();
> +       void enableAutoExposure();
> +       void disableAutoExposure();
> +       bool autoExposureEnabled() const;
> +       void enableAutoGain();
> +       void disableAutoGain();
> +       bool autoGainEnabled() const;
>         void switchMode(CameraMode const &cameraMode, Metadata *metadata);
>         void prepare(Metadata *imageMetadata);
>         void process(StatisticsPtr &stats, DeviceStatus const &deviceStatus, Metadata *imageMetadata,
> --
> 2.39.2
>

Hi everyone

Thanks for all the work and posting a revised version.

On Wed, 15 Jan 2025 at 08:56, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi Paul,
>
> On Tue, 14 Jan 2025 at 21:36, Paul Elder <paul.elder@ideasonboard.com> wrote:
> >
> > From: Jacopo Mondi <jacopo@jmondi.org>
> >
> > The newly introduced controls to drive the AEGC algorithm allow
> > to control the computation of the exposure time and analogue gain
> > separately.
> >
> > The RPi AEGC implementation already computes the shutter and gain
> > values separately but does not expose separate functions to control
> > them.
> >
> > Augment the AgcAlgorithm interface to allow pausing/resuming the shutter
> > and gain automatic computations separately and plumb them to the newly
> > introduced controls.
> >
> > Add safety checks to ignore ExposureTime and AnalogueGain values if the
> > algorithms are not paused, and report the correct AeState value by
> > checking if both algorithms have been paused or if they have converged.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> >
>
> Thanks for the update, this looks much better!
>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>
> Before merging, I would like an ack by @David Plowman if he's fine with this.

As we discussed the other week, I think this file is now in a state
where it needs a bit of work. The plan will be to add explicit
"exposure time mode" and "analogue gain mode" fields, indicating
whether each is manual or auto. Manual exposure/gain values can then
be "remembered" until they take effect (which is how control values
work more usually), and the careful ordering of control applications
will no longer matter.

Until that happens I wouldn't recommend anyone to use this code, as
the external API it presents, while nominally working, is in an "in
between" state. However, I think this is fine given that we don't
distribute it to our users, and I will get round to making these
updates at some point, hopefully quite soon! As such:

Acked-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

>
> > ---
> > Changes in v8.1:
> > - fix the racy check in handling the auto/manual controls and the manual
> >   control value control
> >
> > Changes in v8:
> > - add auto as default value for ExposureMode and AnalogueGainMode
> > - add todo for control processing order
> >
> > No change in v7
> >
> > No change in v6
> >
> > No change in v5
> >
> > Changes in v4:
> > - s/shutter/exposure/
> >
> > Changes in v3:
> > - recovered from 2-year-old bitrot
> > ---
> >  src/ipa/rpi/common/ipa_base.cpp            | 106 +++++++++++++++++----
> >  src/ipa/rpi/controller/agc_algorithm.h     |   8 +-
> >  src/ipa/rpi/controller/rpi/agc.cpp         |  52 ++++++++--
> >  src/ipa/rpi/controller/rpi/agc.h           |   8 +-
> >  src/ipa/rpi/controller/rpi/agc_channel.cpp |  24 ++++-
> >  src/ipa/rpi/controller/rpi/agc_channel.h   |   8 +-
> >  6 files changed, 171 insertions(+), 35 deletions(-)
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index 6ff1e22b1..78206bb4a 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -55,8 +55,15 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
> >
> >  /* List of controls handled by the Raspberry Pi IPA */
> >  const ControlInfoMap::Map ipaControls{
> > -       { &controls::AeEnable, ControlInfo(false, true) },
> > +       { &controls::ExposureTimeMode,
> > +         ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> > +                     static_cast<int32_t>(controls::ExposureTimeModeManual),
> > +                     static_cast<int32_t>(controls::ExposureTimeModeAuto)) },
> >         { &controls::ExposureTime, ControlInfo(0, 66666) },
> > +       { &controls::AnalogueGainMode,
> > +         ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> > +                     static_cast<int32_t>(controls::AnalogueGainModeManual),
> > +                     static_cast<int32_t>(controls::AnalogueGainModeAuto)) },
> >         { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },
> >         { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> >         { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> > @@ -749,6 +756,46 @@ void IpaBase::applyControls(const ControlList &controls)
> >                         af->setMode(mode->second);
> >         }
> >
> > +       /*
> > +        * Because some AE controls are mode-specific, handle the AE-related
> > +        * mode changes first.
> > +        */
> > +       if (controls.contains(controls::EXPOSURE_TIME_MODE)) {
> > +               RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > +                       controller_.getAlgorithm("agc"));
> > +               if (!agc) {
> > +                       LOG(IPARPI, Warning)
> > +                               << "Could not set EXPOSURE_TIME_MODE - no AGC algorithm";
> > +                       break;
> > +               }
> > +
> > +               if (ctrl.second.get<int32_t>() == controls::ExposureTimeModeManual)
> > +                       agc->disableAutoExposure();
> > +               else
> > +                       agc->enableAutoExposure();
> > +
> > +               libcameraMetadata_.set(controls::ExposureTimeMode,
> > +                                      ctrl.second.get<int32_t>());
> > +       }
> > +
> > +       if (controls.contains(controls::ANALOGUE_GAIN_MODE)) {
> > +               RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > +                       controller_.getAlgorithm("agc"));
> > +               if (!agc) {
> > +                       LOG(IPARPI, Warning)
> > +                               << "Could not set ANALOGUE_GAIN_MODE - no AGC algorithm";
> > +                       break;
> > +               }
> > +
> > +               if (ctrl.second.get<int32_t>() == controls::AnalogueGainModeManual)
> > +                       agc->disableAutoGain();
> > +               else
> > +                       agc->enableAutoGain();
> > +
> > +               libcameraMetadata_.set(controls::AnalogueGainMode,
> > +                                      ctrl.second.get<int32_t>());
> > +       }
> > +
> >         /* Iterate over controls */
> >         for (auto const &ctrl : controls) {
> >                 LOG(IPARPI, Debug) << "Request ctrl: "
> > @@ -756,23 +803,8 @@ void IpaBase::applyControls(const ControlList &controls)
> >                                    << " = " << ctrl.second.toString();
> >
> >                 switch (ctrl.first) {
> > -               case controls::AE_ENABLE: {
> > -                       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > -                               controller_.getAlgorithm("agc"));
> > -                       if (!agc) {
> > -                               LOG(IPARPI, Warning)
> > -                                       << "Could not set AE_ENABLE - no AGC algorithm";
> > -                               break;
> > -                       }
> > -
> > -                       if (ctrl.second.get<bool>() == false)
> > -                               agc->disableAuto();
> > -                       else
> > -                               agc->enableAuto();
> > -
> > -                       libcameraMetadata_.set(controls::AeEnable, ctrl.second.get<bool>());
> > -                       break;
> > -               }
> > +               case controls::EXPOSURE_TIME_MODE:
> > +                       break; /* We already handled this one above */
> >
> >                 case controls::EXPOSURE_TIME: {
> >                         RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > @@ -783,6 +815,13 @@ void IpaBase::applyControls(const ControlList &controls)
> >                                 break;
> >                         }
> >
> > +                       /*
> > +                        * Ignore manual exposure time when the auto exposure
> > +                        * algorithm is running.
> > +                        */
> > +                       if (agc->autoExposureEnabled())
> > +                               break;
> > +
> >                         /* The control provides units of microseconds. */
> >                         agc->setFixedExposureTime(0, ctrl.second.get<int32_t>() * 1.0us);
> >
> > @@ -790,6 +829,9 @@ void IpaBase::applyControls(const ControlList &controls)
> >                         break;
> >                 }
> >
> > +               case controls::ANALOGUE_GAIN_MODE:
> > +                       break; /* We already handled this one above */
> > +
> >                 case controls::ANALOGUE_GAIN: {
> >                         RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> >                                 controller_.getAlgorithm("agc"));
> > @@ -799,6 +841,13 @@ void IpaBase::applyControls(const ControlList &controls)
> >                                 break;
> >                         }
> >
> > +                       /*
> > +                        * Ignore manual analogue gain value when the auto gain
> > +                        * algorithm is running.
> > +                        */
> > +                       if (agc->autoGainEnabled())
> > +                               break;
> > +
> >                         agc->setFixedAnalogueGain(0, ctrl.second.get<float>());
> >
> >                         libcameraMetadata_.set(controls::AnalogueGain,
> > @@ -855,6 +904,13 @@ void IpaBase::applyControls(const ControlList &controls)
> >                                 break;
> >                         }
> >
> > +                       /*
> > +                        * Ignore AE_EXPOSURE_MODE if the shutter or the gain
> > +                        * are in auto mode.
> > +                        */
> > +                       if (agc->autoExposureEnabled() || agc->autoGainEnabled())
> > +                               break;
> > +
> >                         int32_t idx = ctrl.second.get<int32_t>();
> >                         if (ExposureModeTable.count(idx)) {
> >                                 agc->setExposureMode(ExposureModeTable.at(idx));
> > @@ -1334,9 +1390,19 @@ void IpaBase::reportMetadata(unsigned int ipaContext)
> >         }
> >
> >         AgcPrepareStatus *agcPrepareStatus = rpiMetadata.getLocked<AgcPrepareStatus>("agc.prepare_status");
> > -       if (agcPrepareStatus) {
> > -               libcameraMetadata_.set(controls::AeLocked, agcPrepareStatus->locked);
> > +       if (agcPrepareStatus)
> >                 libcameraMetadata_.set(controls::DigitalGain, agcPrepareStatus->digitalGain);
> > +
> > +       RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
> > +               controller_.getAlgorithm("agc"));
> > +       if (agc) {
> > +               if (!agc->autoExposureEnabled() && !agc->autoGainEnabled())
> > +                       libcameraMetadata_.set(controls::AeState, controls::AeStateIdle);
> > +               else if (agcPrepareStatus)
> > +                       libcameraMetadata_.set(controls::AeState,
> > +                                              agcPrepareStatus->locked
> > +                                                      ? controls::AeStateConverged
> > +                                                      : controls::AeStateSearching);
> >         }
> >
> >         LuxStatus *luxStatus = rpiMetadata.getLocked<LuxStatus>("lux.status");
> > diff --git a/src/ipa/rpi/controller/agc_algorithm.h b/src/ipa/rpi/controller/agc_algorithm.h
> > index c97828577..fdaa10e6c 100644
> > --- a/src/ipa/rpi/controller/agc_algorithm.h
> > +++ b/src/ipa/rpi/controller/agc_algorithm.h
> > @@ -30,8 +30,12 @@ public:
> >         virtual void setMeteringMode(std::string const &meteringModeName) = 0;
> >         virtual void setExposureMode(std::string const &exposureModeName) = 0;
> >         virtual void setConstraintMode(std::string const &contraintModeName) = 0;
> > -       virtual void enableAuto() = 0;
> > -       virtual void disableAuto() = 0;
> > +       virtual void enableAutoExposure() = 0;
> > +       virtual void disableAutoExposure() = 0;
> > +       virtual bool autoExposureEnabled() const = 0;
> > +       virtual void enableAutoGain() = 0;
> > +       virtual void disableAutoGain() = 0;
> > +       virtual bool autoGainEnabled() const = 0;
> >         virtual void setActiveChannels(const std::vector<unsigned int> &activeChannels) = 0;
> >  };
> >
> > diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> > index c48fdf156..02bfdb4a5 100644
> > --- a/src/ipa/rpi/controller/rpi/agc.cpp
> > +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> > @@ -74,22 +74,62 @@ int Agc::checkChannel(unsigned int channelIndex) const
> >         return 0;
> >  }
> >
> > -void Agc::disableAuto()
> > +void Agc::disableAutoExposure()
> >  {
> > -       LOG(RPiAgc, Debug) << "disableAuto";
> > +       LOG(RPiAgc, Debug) << "disableAutoExposure";
> >
> >         /* All channels are enabled/disabled together. */
> >         for (auto &data : channelData_)
> > -               data.channel.disableAuto();
> > +               data.channel.disableAutoExposure();
> >  }
> >
> > -void Agc::enableAuto()
> > +void Agc::enableAutoExposure()
> >  {
> > -       LOG(RPiAgc, Debug) << "enableAuto";
> > +       LOG(RPiAgc, Debug) << "enableAutoExposure";
> >
> >         /* All channels are enabled/disabled together. */
> >         for (auto &data : channelData_)
> > -               data.channel.enableAuto();
> > +               data.channel.enableAutoExposure();
> > +}
> > +
> > +bool Agc::autoExposureEnabled() const
> > +{
> > +       LOG(RPiAgc, Debug) << "autoExposureEnabled";
> > +
> > +       /*
> > +        * We always have at least one channel, and since all channels are
> > +        * enabled and disabled together we can simply check the first one.
> > +        */
> > +       return channelData_[0].channel.autoExposureEnabled();
> > +}
> > +
> > +void Agc::disableAutoGain()
> > +{
> > +       LOG(RPiAgc, Debug) << "disableAutoGain";
> > +
> > +       /* All channels are enabled/disabled together. */
> > +       for (auto &data : channelData_)
> > +               data.channel.disableAutoGain();
> > +}
> > +
> > +void Agc::enableAutoGain()
> > +{
> > +       LOG(RPiAgc, Debug) << "enableAutoGain";
> > +
> > +       /* All channels are enabled/disabled together. */
> > +       for (auto &data : channelData_)
> > +               data.channel.enableAutoGain();
> > +}
> > +
> > +bool Agc::autoGainEnabled() const
> > +{
> > +       LOG(RPiAgc, Debug) << "autoGainEnabled";
> > +
> > +       /*
> > +        * We always have at least one channel, and since all channels are
> > +        * enabled and disabled together we can simply check the first one.
> > +        */
> > +       return channelData_[0].channel.autoGainEnabled();
> >  }
> >
> >  unsigned int Agc::getConvergenceFrames() const
> > diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h
> > index 3aca000bb..c3a940bf6 100644
> > --- a/src/ipa/rpi/controller/rpi/agc.h
> > +++ b/src/ipa/rpi/controller/rpi/agc.h
> > @@ -40,8 +40,12 @@ public:
> >         void setMeteringMode(std::string const &meteringModeName) override;
> >         void setExposureMode(std::string const &exposureModeName) override;
> >         void setConstraintMode(std::string const &contraintModeName) override;
> > -       void enableAuto() override;
> > -       void disableAuto() override;
> > +       void enableAutoExposure() override;
> > +       void disableAutoExposure() override;
> > +       bool autoExposureEnabled() const override;
> > +       void enableAutoGain() override;
> > +       void disableAutoGain() override;
> > +       bool autoGainEnabled() const override;
> >         void switchMode(CameraMode const &cameraMode, Metadata *metadata) override;
> >         void prepare(Metadata *imageMetadata) override;
> >         void process(StatisticsPtr &stats, Metadata *imageMetadata) override;
> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > index 79c459735..e79184b7a 100644
> > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp
> > @@ -319,18 +319,36 @@ int AgcChannel::read(const libcamera::YamlObject &params,
> >         return 0;
> >  }
> >
> > -void AgcChannel::disableAuto()
> > +void AgcChannel::disableAutoExposure()
> >  {
> >         fixedExposureTime_ = status_.exposureTime;
> > -       fixedAnalogueGain_ = status_.analogueGain;
> >  }
> >
> > -void AgcChannel::enableAuto()
> > +void AgcChannel::enableAutoExposure()
> >  {
> >         fixedExposureTime_ = 0s;
> > +}
> > +
> > +bool AgcChannel::autoExposureEnabled() const
> > +{
> > +       return fixedExposureTime_ == 0s;
> > +}
> > +
> > +void AgcChannel::disableAutoGain()
> > +{
> > +       fixedAnalogueGain_ = status_.analogueGain;
> > +}
> > +
> > +void AgcChannel::enableAutoGain()
> > +{
> >         fixedAnalogueGain_ = 0;
> >  }
> >
> > +bool AgcChannel::autoGainEnabled() const
> > +{
> > +       return fixedAnalogueGain_ == 0;
> > +}
> > +
> >  unsigned int AgcChannel::getConvergenceFrames() const
> >  {
> >         /*
> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h
> > index 734e5efd3..fa697e6fa 100644
> > --- a/src/ipa/rpi/controller/rpi/agc_channel.h
> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.h
> > @@ -96,8 +96,12 @@ public:
> >         void setMeteringMode(std::string const &meteringModeName);
> >         void setExposureMode(std::string const &exposureModeName);
> >         void setConstraintMode(std::string const &contraintModeName);
> > -       void enableAuto();
> > -       void disableAuto();
> > +       void enableAutoExposure();
> > +       void disableAutoExposure();
> > +       bool autoExposureEnabled() const;
> > +       void enableAutoGain();
> > +       void disableAutoGain();
> > +       bool autoGainEnabled() const;
> >         void switchMode(CameraMode const &cameraMode, Metadata *metadata);
> >         void prepare(Metadata *imageMetadata);
> >         void process(StatisticsPtr &stats, DeviceStatus const &deviceStatus, Metadata *imageMetadata,
> > --
> > 2.39.2
> >