[PATCH v5 6/8] ipa: rkisp1: Port to the new AEGC controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Dec 16 16:04:50 CET 2024
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,
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.
>
> /*
> * 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 {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list