[PATCH v3 6/8] ipa: rkisp1: Port to the new AEGC controls

Laurent Pinchart laurent.pinchart at ideasonboard.com
Thu Nov 21 04:27:41 CET 2024


On Thu, Nov 14, 2024 at 04:20:40PM +0100, Stefan Klug wrote:
> Hi Paul,
> 
> Thank you for the patch. 
> 
> On Wed, Nov 13, 2024 at 10:12:54PM +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>
> > 
> > ---
> > 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 | 68 ++++++++++++++++++++++++-------
> >  src/ipa/rkisp1/ipa_context.cpp    | 14 +++++--
> >  src/ipa/rkisp1/ipa_context.h      |  6 ++-
> >  3 files changed, 67 insertions(+), 21 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 301b7ec26508..0ad206621516 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -148,7 +148,12 @@ 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));
> > +	context.ctrlMap[&controls::AnalogueGainMode] =
> > +		ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> > +			    static_cast<int32_t>(controls::AnalogueGainModeManual));
> 
> I believe these should also have default values.

They should, and I think they should default to auto. Can we mandate in
the controls documentation that a camera that supports auto mode must
default to auto ?

> >  	context.ctrlMap.merge(controls());
> >  
> >  	return 0;
> > @@ -169,7 +174,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;
> > 
> 
> Ahh this actually sets the defaults internally, but the defaults above
> are the ones seen by applications like camshark.
> 
> >  	context.activeState.agc.constraintMode =
> >  		static_cast<controls::AeConstraintModeEnum>(constraintModes().begin()->first);
> > @@ -215,18 +221,42 @@ 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
> > +			 */
> > +			if (!agc.autoExposureEnabled && !controls.get(controls::ExposureTime))
> > +				frameContext.agc.exposure = agc.automatic.exposure;
> 
> Does this work? It sets the exposure time of the future frame (that is
> currently beeing queued) to the current value from active state. But
> that value will change depending on how many frames are queued already.
> I guess we need to set a flag and fetch the exposure time from
> activeState when this frame gets processed.
> 
> > +		}
> > +
> > +		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
> > +			 */
> > +			if (!agc.autoGainEnabled && !controls.get(controls::AnalogueGain))
> > +				frameContext.agc.gain = agc.automatic.gain;
> 
> As above.
> 
> >  		}
> >  	}
> >  
> >  	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 +265,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,10 +314,10 @@ void Agc::queueRequest(IPAContext &context,
> >  void Agc::prepare(IPAContext &context, const uint32_t frame,
> >  		  IPAFrameContext &frameContext, RkISP1Params *params)
> >  {
> > -	if (frameContext.agc.autoEnabled) {
> > +	if (frameContext.agc.autoExposureEnabled)
> >  		frameContext.agc.exposure = context.activeState.agc.automatic.exposure;
> > +	if (frameContext.agc.autoGainEnabled)
> >  		frameContext.agc.gain = context.activeState.agc.automatic.gain;
> > -	}
> >  
> >  	if (frame > 0 && !frameContext.agc.updateMetering)
> >  		return;
> > @@ -333,7 +364,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
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 14d0c02a2b32..a8e8e1c0d06e 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
> > @@ -307,8 +310,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
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index e274d9b01e1c..7f819d31015e 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -75,7 +75,8 @@ struct IPAActiveState {
> >  			double gain;
> >  		} automatic;
> >  
> > -		bool autoEnabled;
> > +		bool autoExposureEnabled;
> > +		bool autoGainEnabled;
> >  		controls::AeConstraintModeEnum constraintMode;
> >  		controls::AeExposureModeEnum exposureMode;
> >  		controls::AeMeteringModeEnum meteringMode;
> > @@ -128,7 +129,8 @@ 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;

-- 
Regards,

Laurent Pinchart


More information about the libcamera-devel mailing list