[v5,6/8] ipa: rkisp1: Port to the new AEGC controls
diff mbox series

Message ID 20241216043954.3506855-7-paul.elder@ideasonboard.com
State New
Headers show
Series
  • AEGC controls
Related show

Commit Message

Paul Elder Dec. 16, 2024, 4:39 a.m. UTC 
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@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(-)

Comments

Laurent Pinchart Dec. 16, 2024, 3:04 p.m. UTC | #1 
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@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@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 {
Paul Elder Dec. 17, 2024, 4:27 a.m. UTC | #2 
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@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@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 {
> 
> --

Patch
diff mbox series 

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);
+	}
 
 	/*
 	 * 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 {