[PATCH v5 6/8] ipa: rkisp1: Port to the new AEGC controls
Paul Elder
paul.elder at ideasonboard.com
Tue Dec 17 05:27:57 CET 2024
On Mon, Dec 16, 2024 at 05:04:50PM +0200, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> On Mon, Dec 16, 2024 at 01:39:52PM +0900, Paul Elder wrote:
> > The newly introduced controls to drive the AEGC algorithm allow
> > controlling the computation of the exposure time and analogue gain
> > separately.
> >
> > Augument the RkISP1 AEGC implementation to handle the exposure and gain
> > controls separately using the new controls.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > Changes in v5:
> > - fix mixed manual-gain ae modes
> > - improve documentation for mode change flags in the ipa context
> >
> > Changes in v4:
> > - make auto the default value in the control infos
> > - implement the expected behavior when transitioning between manual and
> > auto modes
> >
> > New in v3
> >
> > Back 2 years ago in v2 RkISP1 didn't yet support AeEnable properly yet
> > so the AeEnable control was simply removed.
> > ---
> > src/ipa/rkisp1/algorithms/agc.cpp | 122 +++++++++++++++++++++++++-----
> > src/ipa/rkisp1/ipa_context.cpp | 24 +++++-
> > src/ipa/rkisp1/ipa_context.h | 8 +-
> > 3 files changed, 128 insertions(+), 26 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 40e5a8f481b2..6dfb4c8adc41 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -148,7 +148,14 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
> > if (ret)
> > return ret;
> >
> > - context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true);
> > + context.ctrlMap[&controls::ExposureTimeMode] =
> > + ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> > + static_cast<int32_t>(controls::ExposureTimeModeManual),
> > + static_cast<int32_t>(controls::ExposureTimeModeAuto));
> > + context.ctrlMap[&controls::AnalogueGainMode] =
> > + ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> > + static_cast<int32_t>(controls::AnalogueGainModeManual),
> > + static_cast<int32_t>(controls::AnalogueGainModeAuto));
> > context.ctrlMap.merge(controls());
> >
> > return 0;
> > @@ -169,7 +176,8 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > 10ms / context.configuration.sensor.lineDuration;
> > context.activeState.agc.manual.gain = context.activeState.agc.automatic.gain;
> > context.activeState.agc.manual.exposure = context.activeState.agc.automatic.exposure;
> > - context.activeState.agc.autoEnabled = !context.configuration.raw;
> > + context.activeState.agc.autoExposureEnabled = !context.configuration.raw;
> > + context.activeState.agc.autoGainEnabled = !context.configuration.raw;
> >
> > context.activeState.agc.constraintMode =
> > static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first);
> > @@ -215,18 +223,47 @@ void Agc::queueRequest(IPAContext &context,
> > auto &agc = context.activeState.agc;
> >
> > if (!context.configuration.raw) {
> > - const auto &agcEnable = controls.get(controls::AeEnable);
> > - if (agcEnable && *agcEnable != agc.autoEnabled) {
> > - agc.autoEnabled = *agcEnable;
> > + const auto &aeEnable = controls.get(controls::ExposureTimeMode);
> > + if (aeEnable &&
> > + (*aeEnable == controls::ExposureTimeModeAuto) != agc.autoExposureEnabled) {
> > + agc.autoExposureEnabled = (*aeEnable == controls::ExposureTimeModeAuto);
> >
> > LOG(RkISP1Agc, Debug)
> > - << (agc.autoEnabled ? "Enabling" : "Disabling")
> > - << " AGC";
> > + << (agc.autoExposureEnabled ? "Enabling" : "Disabling")
> > + << " AGC (exposure)";
> > +
> > + /*
> > + * If we go from auto -> manual with no manual control
> > + * set, use the last computed value, which we don't
> > + * know until prepare() so save this information.
> > + *
> > + * \todo Check the previous frame at prepare() time
> > + * instead of saving a flag here
> > + */
> > + if (!agc.autoExposureEnabled && !controls.get(controls::ExposureTime))
> > + frameContext.agc.autoExposureModeChange = true;
> > + }
> > +
> > + const auto &agEnable = controls.get(controls::AnalogueGainMode);
> > + if (agEnable &&
> > + (*agEnable == controls::AnalogueGainModeAuto) != agc.autoGainEnabled) {
> > + agc.autoGainEnabled = (*agEnable == controls::AnalogueGainModeAuto);
> > +
> > + LOG(RkISP1Agc, Debug)
> > + << (agc.autoGainEnabled ? "Enabling" : "Disabling")
> > + << " AGC (gain)";
> > + /*
> > + * If we go from auto -> manual with no manual control
> > + * set, use the last computed value, which we don't
> > + * know until prepare() so save this information.
> > + */
> > + if (!agc.autoGainEnabled && !controls.get(controls::AnalogueGain))
> > + frameContext.agc.autoGainModeChange = true;
> > }
> > }
> >
> > const auto &exposure = controls.get(controls::ExposureTime);
> > - if (exposure && !agc.autoEnabled) {
> > + if (exposure && !agc.autoExposureEnabled) {
> > agc.manual.exposure = *exposure * 1.0us
> > / context.configuration.sensor.lineDuration;
> >
> > @@ -235,18 +272,19 @@ void Agc::queueRequest(IPAContext &context,
> > }
> >
> > const auto &gain = controls.get(controls::AnalogueGain);
> > - if (gain && !agc.autoEnabled) {
> > + if (gain && !agc.autoGainEnabled) {
> > agc.manual.gain = *gain;
> >
> > LOG(RkISP1Agc, Debug) << "Set gain to " << agc.manual.gain;
> > }
> >
> > - frameContext.agc.autoEnabled = agc.autoEnabled;
> > + frameContext.agc.autoExposureEnabled = agc.autoExposureEnabled;
> > + frameContext.agc.autoGainEnabled = agc.autoGainEnabled;
> >
> > - if (!frameContext.agc.autoEnabled) {
> > + if (!frameContext.agc.autoExposureEnabled)
> > frameContext.agc.exposure = agc.manual.exposure;
> > + if (!frameContext.agc.autoGainEnabled)
> > frameContext.agc.gain = agc.manual.gain;
> > - }
> >
> > const auto &meteringMode = controls.get(controls::AeMeteringMode);
> > if (meteringMode) {
> > @@ -283,9 +321,26 @@ void Agc::queueRequest(IPAContext &context,
> > void Agc::prepare(IPAContext &context, const uint32_t frame,
> > IPAFrameContext &frameContext, RkISP1Params *params)
> > {
> > - if (frameContext.agc.autoEnabled) {
> > - frameContext.agc.exposure = context.activeState.agc.automatic.exposure;
> > - frameContext.agc.gain = context.activeState.agc.automatic.gain;
> > + uint32_t activeAutoExposure = context.activeState.agc.automatic.exposure;
> > + double activeAutoGain = context.activeState.agc.automatic.gain;
> > +
> > + /* Populate exposure and gain in auto mode */
> > + if (frameContext.agc.autoExposureEnabled)
> > + frameContext.agc.exposure = activeAutoExposure;
> > + if (frameContext.agc.autoGainEnabled)
> > + frameContext.agc.gain = activeAutoGain;
> > +
> > + /*
> > + * Populate manual exposure and gain from the active auto values when
> > + * transitioning from auto to manual
> > + */
> > + if (!frameContext.agc.autoExposureEnabled && frameContext.agc.autoExposureModeChange) {
> > + context.activeState.agc.manual.exposure = activeAutoExposure;
> > + frameContext.agc.exposure = activeAutoExposure;
> > + }
> > + if (!frameContext.agc.autoGainEnabled && frameContext.agc.autoGainModeChange) {
> > + context.activeState.agc.manual.gain = activeAutoGain;
> > + frameContext.agc.gain = activeAutoGain;
> > }
> >
> > if (frame > 0 && !frameContext.agc.updateMetering)
> > @@ -333,7 +388,14 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > * frameContext.sensor.exposure;
> > metadata.set(controls::AnalogueGain, frameContext.sensor.gain);
> > metadata.set(controls::ExposureTime, exposureTime.get<std::micro>());
> > - metadata.set(controls::AeEnable, frameContext.agc.autoEnabled);
> > + metadata.set(controls::ExposureTimeMode,
> > + frameContext.agc.autoExposureEnabled
> > + ? controls::ExposureTimeModeAuto
> > + : controls::ExposureTimeModeManual);
> > + metadata.set(controls::AnalogueGainMode,
> > + frameContext.agc.autoGainEnabled
> > + ? controls::AnalogueGainModeAuto
> > + : controls::AnalogueGainModeManual);
> >
> > /* \todo Use VBlank value calculated from each frame exposure. */
> > uint32_t vTotal = context.configuration.sensor.size.height
> > @@ -428,10 +490,30 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > std::clamp(frameContext.agc.maxFrameDuration,
> > context.configuration.sensor.minExposureTime,
> > context.configuration.sensor.maxExposureTime);
> > - setLimits(context.configuration.sensor.minExposureTime,
> > - maxExposureTime,
> > - context.configuration.sensor.minAnalogueGain,
> > - context.configuration.sensor.maxAnalogueGain);
> > + utils::Duration manualExposureTime =
> > + context.configuration.sensor.lineDuration * frameContext.agc.exposure;
> > +
> > + if (frameContext.agc.autoExposureEnabled &&
> > + !frameContext.agc.autoGainEnabled) {
> > + setLimits(context.configuration.sensor.minExposureTime,
> > + maxExposureTime,
> > + frameContext.agc.gain,
> > + frameContext.agc.gain);
> > + } else if (!frameContext.agc.autoExposureEnabled &&
> > + frameContext.agc.autoGainEnabled) {
> > + setLimits(manualExposureTime, manualExposureTime,
> > + context.configuration.sensor.minAnalogueGain,
> > + context.configuration.sensor.maxAnalogueGain);
> > + } else if (!frameContext.agc.autoExposureEnabled &&
> > + !frameContext.agc.autoGainEnabled) {
> > + setLimits(manualExposureTime, manualExposureTime,
> > + frameContext.agc.gain, frameContext.agc.gain);
> > + } else {
> > + setLimits(context.configuration.sensor.minExposureTime,
> > + maxExposureTime,
> > + context.configuration.sensor.minAnalogueGain,
> > + context.configuration.sensor.maxAnalogueGain);
> > + }
>
> That looks complicated. How about the following ?
>
> /*
> * Set the AGC limits using the fixed exposure time and/or gain in
> * manual mode, or the sensor limits in auto mode.
> */
> utils::Duration minExposureTime;
> utils::Duration maxExposureTime;
> double minAnalogueGain;
> double maxAnalogueGain;
>
> if (frameContext.agc.autoExposureEnabled) {
> minExposureTime = context.configuration.sensor.minExposureTime;
> maxExposureTime = std::clamp(frameContext.agc.maxFrameDuration,
> context.configuration.sensor.minExposureTime,
> context.configuration.sensor.maxExposureTime);
> } else {
> minExposureTime = context.configuration.sensor.lineDuration
> * frameContext.agc.exposure;
> maxExposureTime = minExposureTime;
> }
>
> if (frameContext.agc.autoGainEnabled) {
> minAnalogueGain = context.configuration.sensor.minAnalogueGain;
> maxAnalogueGain = context.configuration.sensor.maxAnalogueGain;
> } else {
> minAnalogueGain = frameContext.agc.gain;
> maxAnalogueGain = frameContext.agc.gain;
> }
>
> setLimits(minExposureTime, maxExposureTime, minAnalogueGain, maxAnalogueGain);
>
> With that,
Ah yeah that is a lot nicer.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> I'm not confident making this change when pushing the patches, would you
> be able to test this first ? No need to post a v6 for this, once you get
> an ack from David or Naush for patch 5/8, you can push the series.
It works! Thanks!
Paul
>
> >
> > /*
> > * The Agc algorithm needs to know the effective exposure value that was
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 80b99df8eaf8..261c0472a4fc 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -165,8 +165,11 @@ namespace libcamera::ipa::rkisp1 {
> > * \var IPAActiveState::agc.automatic.gain
> > * \brief Automatic analogue gain multiplier
> > *
> > - * \var IPAActiveState::agc.autoEnabled
> > - * \brief Manual/automatic AGC state as set by the AeEnable control
> > + * \var IPAActiveState::agc.autoExposureEnabled
> > + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
> > + *
> > + * \var IPAActiveState::agc.autoGainEnabled
> > + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
> > *
> > * \var IPAActiveState::agc.constraintMode
> > * \brief Constraint mode as set by the AeConstraintMode control
> > @@ -289,8 +292,11 @@ namespace libcamera::ipa::rkisp1 {
> > *
> > * The gain should be adapted to the sensor specific gain code before applying.
> > *
> > - * \var IPAFrameContext::agc.autoEnabled
> > - * \brief Manual/automatic AGC state as set by the AeEnable control
> > + * \var IPAFrameContext::agc.autoExposureEnabled
> > + * \brief Manual/automatic AGC state (exposure) as set by the ExposureTimeMode control
> > + *
> > + * \var IPAFrameContext::agc.autoGainEnabled
> > + * \brief Manual/automatic AGC state (gain) as set by the AnalogueGainMode control
> > *
> > * \var IPAFrameContext::agc.constraintMode
> > * \brief Constraint mode as set by the AeConstraintMode control
> > @@ -306,6 +312,16 @@ namespace libcamera::ipa::rkisp1 {
> > *
> > * \var IPAFrameContext::agc.updateMetering
> > * \brief Indicate if new ISP AGC metering parameters need to be applied
> > + *
> > + * \var IPAFrameContext::agc.autoExposureModeChange
> > + * \brief Indicate if autoExposureEnabled has changed from true in the previous
> > + * frame to false in the current frame, and no manual exposure value has been
> > + * supplied in the current frame.
> > + *
> > + * \var IPAFrameContext::agc.autoGainModeChange
> > + * \brief Indicate if autoGainEnabled has changed from true in the previous
> > + * frame to false in the current frame, and no manual gain value has been
> > + * supplied in the current frame.
> > */
> >
> > /**
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index deb8c196f1b8..bc3d5a24be3a 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -79,7 +79,8 @@ struct IPAActiveState {
> > double gain;
> > } automatic;
> >
> > - bool autoEnabled;
> > + bool autoExposureEnabled;
> > + bool autoGainEnabled;
> > controls::AeConstraintModeEnum constraintMode;
> > controls::AeExposureModeEnum exposureMode;
> > controls::AeMeteringModeEnum meteringMode;
> > @@ -124,12 +125,15 @@ struct IPAFrameContext : public FrameContext {
> > struct {
> > uint32_t exposure;
> > double gain;
> > - bool autoEnabled;
> > + bool autoExposureEnabled;
> > + bool autoGainEnabled;
> > controls::AeConstraintModeEnum constraintMode;
> > controls::AeExposureModeEnum exposureMode;
> > controls::AeMeteringModeEnum meteringMode;
> > utils::Duration maxFrameDuration;
> > bool updateMetering;
> > + bool autoExposureModeChange;
> > + bool autoGainModeChange;
> > } agc;
> >
> > struct {
>
> --
More information about the libcamera-devel
mailing list