[v3,3/3] ipa: rkisp1: agc: Plumb mode-selection and frame duration controls
diff mbox series

Message ID 20240517080802.3896531-4-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: rkisp1: Improve AGC (plumbing)
Related show

Commit Message

Paul Elder May 17, 2024, 8:08 a.m. UTC 
Plumb controls for setting metering mode, exposure mode, constraint
mode, and frame duration limits. Also report them as available controls,
as well as in metadata.

While at it, add the missing #include for tuple, as a std::tie is used.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v3:
- prevent maxShutterSpeed (for setLimits) from choosing 0 shutter speed
  if one of the inputs std::min() is not set (this fixes an assertion
  failure in the exposure mode helper)

Changes in v2:
- add #include <tuple>
- don't overwrite metering/exposure/constraint mode controls to default
  if no control is supplied, and retain the old value instead (same for
  frame duration limits)
---
 src/ipa/rkisp1/algorithms/agc.cpp     | 55 ++++++++++++++++++++++++---
 src/ipa/rkisp1/algorithms/algorithm.h |  2 +
 src/ipa/rkisp1/ipa_context.h          | 10 ++++-
 src/ipa/rkisp1/rkisp1.cpp             | 10 +++++
 4 files changed, 70 insertions(+), 7 deletions(-)

Comments

Stefan Klug May 17, 2024, 10:47 a.m. UTC | #1 
Hi Paul,

thanks for the patch.

On Fri, May 17, 2024 at 05:08:01PM +0900, Paul Elder wrote:
> Plumb controls for setting metering mode, exposure mode, constraint
> mode, and frame duration limits. Also report them as available controls,
> as well as in metadata.
> 
> While at it, add the missing #include for tuple, as a std::tie is used.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v3:
> - prevent maxShutterSpeed (for setLimits) from choosing 0 shutter speed
>   if one of the inputs std::min() is not set (this fixes an assertion
>   failure in the exposure mode helper)
> 
> Changes in v2:
> - add #include <tuple>
> - don't overwrite metering/exposure/constraint mode controls to default
>   if no control is supplied, and retain the old value instead (same for
>   frame duration limits)
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp     | 55 ++++++++++++++++++++++++---
>  src/ipa/rkisp1/algorithms/algorithm.h |  2 +
>  src/ipa/rkisp1/ipa_context.h          | 10 ++++-
>  src/ipa/rkisp1/rkisp1.cpp             | 10 +++++
>  4 files changed, 70 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 1c9872d02..ce8beae24 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -10,6 +10,8 @@
>  #include <algorithm>
>  #include <chrono>
>  #include <cmath>
> +#include <tuple>
> +#include <vector>
>  
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/utils.h>
> @@ -48,6 +50,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
>  		return -EINVAL;
>  	}
>  
> +	std::vector<ControlValue> availableMeteringModes;
>  	for (const auto &[key, value] : yamlMeteringModes.asDict()) {
>  		if (controls::AeMeteringModeNameValueMap.find(key) ==
>  		    controls::AeMeteringModeNameValueMap.end()) {
> @@ -66,17 +69,23 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
>  				<< "Matched metering matrix mode "
>  				<< key << ", version " << version;
>  
> -			meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;
> +			int32_t control = controls::AeMeteringModeNameValueMap.at(key);
> +			meteringModes_[control] = weights;
> +			availableMeteringModes.push_back(control);
>  		}
>  	}
>  
> -	if (meteringModes_.empty()) {
> +	if (availableMeteringModes.empty()) {
>  		int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at("MeteringMatrix");
>  		std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1);
>  
>  		meteringModes_[meteringModeId] = weights;
> +		availableMeteringModes.push_back(meteringModeId);
>  	}
>  
> +	Algorithm::controls_[&controls::AeMeteringMode] =
> +		ControlInfo(availableMeteringModes);
> +
>  	return 0;
>  }
>  
> @@ -137,6 +146,8 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
>  
>  	context.ctrlMap.merge(controls());
>  
> +	Algorithm::controls_.merge(ControlInfoMap::Map(controls()));
> +
>  	return 0;
>  }
>  
> @@ -159,6 +170,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  
>  	context.activeState.agc.constraintMode = constraintModes().begin()->first;
>  	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> +	context.activeState.agc.meteringMode = meteringModes_.begin()->first;
>  
>  	/*
>  	 * Define the measurement window for AGC as a centered rectangle
> @@ -169,7 +181,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>  	context.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
>  	context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
>  
> -	/* \todo Run this again when FrameDurationLimits is passed in */
>  	setLimits(context.configuration.sensor.minShutterSpeed,
>  		  context.configuration.sensor.maxShutterSpeed,
>  		  context.configuration.sensor.minAnalogueGain,
> @@ -223,6 +234,26 @@ void Agc::queueRequest(IPAContext &context,
>  		frameContext.agc.exposure = agc.manual.exposure;
>  		frameContext.agc.gain = agc.manual.gain;
>  	}
> +
> +	const auto &meteringMode = controls.get(controls::AeMeteringMode);
> +	if (meteringMode)
> +		agc.meteringMode = *meteringMode;
> +	frameContext.agc.meteringMode = agc.meteringMode;
> +
> +	const auto &exposureMode = controls.get(controls::AeExposureMode);
> +	if (exposureMode)
> +		agc.exposureMode = *exposureMode;
> +	frameContext.agc.exposureMode = agc.exposureMode;
> +
> +	const auto &constraintMode = controls.get(controls::AeConstraintMode);
> +	if (constraintMode)
> +		agc.constraintMode = *constraintMode;
> +	frameContext.agc.constraintMode = agc.constraintMode;
> +
> +	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> +	if (frameDurationLimits)
> +		agc.maxShutterSpeed = std::chrono::milliseconds((*frameDurationLimits).back());
> +	frameContext.agc.maxShutterSpeed = agc.maxShutterSpeed;
>  }
>  
>  /**
> @@ -261,8 +292,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
>  		params->meas.hst_config.hist_weight,
>  		context.hw->numHistogramWeights
>  	};
> -	/* \todo Get this from control */
> -	std::vector<uint8_t> &modeWeights = meteringModes_.at(controls::MeteringMatrix);
> +	std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);
>  	std::copy(modeWeights.begin(), modeWeights.end(), weights.begin());
>  
>  	std::stringstream str;
> @@ -289,6 +319,7 @@ 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);
>  
>  	/* \todo Use VBlank value calculated from each frame exposure. */
>  	uint32_t vTotal = context.configuration.sensor.size.height
> @@ -296,6 +327,10 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>  	utils::Duration frameDuration = context.configuration.sensor.lineDuration
>  				      * vTotal;
>  	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> +
> +	metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> +	metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
> +	metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
>  }
>  
>  /**
> @@ -370,6 +405,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  		       [](uint32_t x) { return x >> 4; });
>  	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
>  
> +	utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,
> +						   frameContext.agc.maxShutterSpeed);
> +	if (!maxShutterSpeed)
> +		maxShutterSpeed = std::max(context.configuration.sensor.maxShutterSpeed,
> +					   frameContext.agc.maxShutterSpeed);
> +	setLimits(context.configuration.sensor.minShutterSpeed,
> +		  maxShutterSpeed,
> +		  context.configuration.sensor.minAnalogueGain,
> +		  context.configuration.sensor.maxAnalogueGain);
> +
>  	/*
>  	 * The Agc algorithm needs to know the effective exposure value that was
>  	 * applied to the sensor when the statistics were collected.
> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
> index 9454c9a1f..c3a002b85 100644
> --- a/src/ipa/rkisp1/algorithms/algorithm.h
> +++ b/src/ipa/rkisp1/algorithms/algorithm.h
> @@ -25,6 +25,8 @@ public:
>  
>  	bool disabled_;
>  	bool supportsRaw_;
> +
> +	ControlInfoMap::Map controls_;
>  };
>  
>  } /* namespace ipa::rkisp1 */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 256b75ebc..26c395704 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -68,8 +68,10 @@ struct IPAActiveState {
>  		} automatic;
>  
>  		bool autoEnabled;
> -		uint32_t constraintMode;
> -		uint32_t exposureMode;
> +		int32_t constraintMode;
> +		int32_t exposureMode;
> +		int32_t meteringMode;
> +		utils::Duration maxShutterSpeed;
>  	} agc;
>  
>  	struct {
> @@ -111,6 +113,10 @@ struct IPAFrameContext : public FrameContext {
>  		uint32_t exposure;
>  		double gain;
>  		bool autoEnabled;
> +		int32_t exposureMode;
> +		int32_t constraintMode;
> +		int32_t meteringMode;
> +		utils::Duration maxShutterSpeed;
>  	} agc;
>  
>  	struct {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 963cdbab4..7716c11bb 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -80,6 +80,7 @@ private:
>  	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
>  
>  	ControlInfoMap sensorControls_;
> +	ControlInfoMap::Map algoControls_;
>  
>  	/* Interface to the Camera Helper */
>  	std::unique_ptr<CameraSensorHelper> camHelper_;
> @@ -193,6 +194,14 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>  	if (ret)
>  		return ret;
>  
> +	for (auto const &a : algorithms()) {
> +		Algorithm *algo = static_cast<Algorithm *>(a.get());
> +
> +		/* \todo Avoid merging duplicate controls */
> +		if (!algo->controls_.empty())
> +			algoControls_.merge(ControlInfoMap::Map(algo->controls_));

It doesn't feel right to have controls_ as public member without an
accessor function. But maybe it doesn't matter... and Laurent started it
with the disabled_ mamber :-) I should use this way of adding controls
in Gamma also...

The rest looks good to me.

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Cheers,
Stefan

> +	}
> +
>  	/* Initialize controls. */
>  	updateControls(sensorInfo, sensorControls, ipaControls);
>  
> @@ -377,6 +386,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
>  			       ControlInfoMap *ipaControls)
>  {
>  	ControlInfoMap::Map ctrlMap = rkisp1Controls;
> +	ctrlMap.merge(algoControls_);
>  
>  	/*
>  	 * Compute exposure time limits from the V4L2_CID_EXPOSURE control
> -- 
> 2.39.2
>
Dan Scally May 20, 2024, 12:34 p.m. UTC | #2 
Hi Paul, thanks for the patch

On 17/05/2024 09:08, Paul Elder wrote:
> Plumb controls for setting metering mode, exposure mode, constraint
> mode, and frame duration limits. Also report them as available controls,
> as well as in metadata.
>
> While at it, add the missing #include for tuple, as a std::tie is used.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v3:
> - prevent maxShutterSpeed (for setLimits) from choosing 0 shutter speed
>    if one of the inputs std::min() is not set (this fixes an assertion
>    failure in the exposure mode helper)
>
> Changes in v2:
> - add #include <tuple>
> - don't overwrite metering/exposure/constraint mode controls to default
>    if no control is supplied, and retain the old value instead (same for
>    frame duration limits)
> ---
>   src/ipa/rkisp1/algorithms/agc.cpp     | 55 ++++++++++++++++++++++++---
>   src/ipa/rkisp1/algorithms/algorithm.h |  2 +
>   src/ipa/rkisp1/ipa_context.h          | 10 ++++-
>   src/ipa/rkisp1/rkisp1.cpp             | 10 +++++
>   4 files changed, 70 insertions(+), 7 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 1c9872d02..ce8beae24 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -10,6 +10,8 @@
>   #include <algorithm>
>   #include <chrono>
>   #include <cmath>
> +#include <tuple>
> +#include <vector>
>   
>   #include <libcamera/base/log.h>
>   #include <libcamera/base/utils.h>
> @@ -48,6 +50,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
>   		return -EINVAL;
>   	}
>   
> +	std::vector<ControlValue> availableMeteringModes;
>   	for (const auto &[key, value] : yamlMeteringModes.asDict()) {
>   		if (controls::AeMeteringModeNameValueMap.find(key) ==
>   		    controls::AeMeteringModeNameValueMap.end()) {
> @@ -66,17 +69,23 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
>   				<< "Matched metering matrix mode "
>   				<< key << ", version " << version;
>   
> -			meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;
> +			int32_t control = controls::AeMeteringModeNameValueMap.at(key);
> +			meteringModes_[control] = weights;
> +			availableMeteringModes.push_back(control);
>   		}
>   	}
>   
> -	if (meteringModes_.empty()) {
> +	if (availableMeteringModes.empty()) {
>   		int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at("MeteringMatrix");
>   		std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1);
>   
>   		meteringModes_[meteringModeId] = weights;
> +		availableMeteringModes.push_back(meteringModeId);
>   	}
>   
> +	Algorithm::controls_[&controls::AeMeteringMode] =
> +		ControlInfo(availableMeteringModes);
> +


I was going to say "shouldn't this be controls()[&controls::AeMeteringMode] instead?", but I see 
that the patch adds a new ControlInfoMap to the Algorithm base class. I did a similar thing in the 
central AGC series with a ControlInfoMap in the context that algorithms fill with control data - we 
need to merge those methods and just do one or the other...I don't particularly mind which method we 
settle on, the only difference I can see at the moment is that the context.ctrlMap allows the 
algorithms to all fill the same ControlInfoMap and so avoids the needs to merge them all into 
IPARkISP1::algoControls_ with the loop in IPARkISP1::init()

>   	return 0;
>   }
>   
> @@ -137,6 +146,8 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
>   
>   	context.ctrlMap.merge(controls());
>   
> +	Algorithm::controls_.merge(ControlInfoMap::Map(controls()));
> +
>   	return 0;
>   }
>   
> @@ -159,6 +170,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>   
>   	context.activeState.agc.constraintMode = constraintModes().begin()->first;
>   	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> +	context.activeState.agc.meteringMode = meteringModes_.begin()->first;
>   
>   	/*
>   	 * Define the measurement window for AGC as a centered rectangle
> @@ -169,7 +181,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>   	context.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
>   	context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
>   
> -	/* \todo Run this again when FrameDurationLimits is passed in */
>   	setLimits(context.configuration.sensor.minShutterSpeed,
>   		  context.configuration.sensor.maxShutterSpeed,
>   		  context.configuration.sensor.minAnalogueGain,
> @@ -223,6 +234,26 @@ void Agc::queueRequest(IPAContext &context,
>   		frameContext.agc.exposure = agc.manual.exposure;
>   		frameContext.agc.gain = agc.manual.gain;
>   	}
> +
> +	const auto &meteringMode = controls.get(controls::AeMeteringMode);
> +	if (meteringMode)
> +		agc.meteringMode = *meteringMode;
> +	frameContext.agc.meteringMode = agc.meteringMode;
> +
> +	const auto &exposureMode = controls.get(controls::AeExposureMode);
> +	if (exposureMode)
> +		agc.exposureMode = *exposureMode;
> +	frameContext.agc.exposureMode = agc.exposureMode;
> +
> +	const auto &constraintMode = controls.get(controls::AeConstraintMode);
> +	if (constraintMode)
> +		agc.constraintMode = *constraintMode;
> +	frameContext.agc.constraintMode = agc.constraintMode;
> +
> +	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> +	if (frameDurationLimits)
> +		agc.maxShutterSpeed = std::chrono::milliseconds((*frameDurationLimits).back());
> +	frameContext.agc.maxShutterSpeed = agc.maxShutterSpeed;
>   }
>   
>   /**
> @@ -261,8 +292,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
>   		params->meas.hst_config.hist_weight,
>   		context.hw->numHistogramWeights
>   	};
> -	/* \todo Get this from control */
> -	std::vector<uint8_t> &modeWeights = meteringModes_.at(controls::MeteringMatrix);
> +	std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);
>   	std::copy(modeWeights.begin(), modeWeights.end(), weights.begin());
>   
>   	std::stringstream str;
> @@ -289,6 +319,7 @@ 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);
>   
>   	/* \todo Use VBlank value calculated from each frame exposure. */
>   	uint32_t vTotal = context.configuration.sensor.size.height
> @@ -296,6 +327,10 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
>   	utils::Duration frameDuration = context.configuration.sensor.lineDuration
>   				      * vTotal;
>   	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> +
> +	metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> +	metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
> +	metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
>   }
>   
>   /**
> @@ -370,6 +405,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>   		       [](uint32_t x) { return x >> 4; });
>   	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
>   
> +	utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,
> +						   frameContext.agc.maxShutterSpeed);
> +	if (!maxShutterSpeed)
> +		maxShutterSpeed = std::max(context.configuration.sensor.maxShutterSpeed,
> +					   frameContext.agc.maxShutterSpeed);
> +	setLimits(context.configuration.sensor.minShutterSpeed,
> +		  maxShutterSpeed,
> +		  context.configuration.sensor.minAnalogueGain,
> +		  context.configuration.sensor.maxAnalogueGain);
> +
>   	/*
>   	 * The Agc algorithm needs to know the effective exposure value that was
>   	 * applied to the sensor when the statistics were collected.
> diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
> index 9454c9a1f..c3a002b85 100644
> --- a/src/ipa/rkisp1/algorithms/algorithm.h
> +++ b/src/ipa/rkisp1/algorithms/algorithm.h
> @@ -25,6 +25,8 @@ public:
>   
>   	bool disabled_;
>   	bool supportsRaw_;
> +
> +	ControlInfoMap::Map controls_;
>   };
>   
>   } /* namespace ipa::rkisp1 */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 256b75ebc..26c395704 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -68,8 +68,10 @@ struct IPAActiveState {
>   		} automatic;
>   
>   		bool autoEnabled;
> -		uint32_t constraintMode;
> -		uint32_t exposureMode;
> +		int32_t constraintMode;
> +		int32_t exposureMode;
> +		int32_t meteringMode;
> +		utils::Duration maxShutterSpeed;
>   	} agc;
>   
>   	struct {
> @@ -111,6 +113,10 @@ struct IPAFrameContext : public FrameContext {
>   		uint32_t exposure;
>   		double gain;
>   		bool autoEnabled;
> +		int32_t exposureMode;
> +		int32_t constraintMode;
> +		int32_t meteringMode;
> +		utils::Duration maxShutterSpeed;
>   	} agc;
>   
>   	struct {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 963cdbab4..7716c11bb 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -80,6 +80,7 @@ private:
>   	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
>   
>   	ControlInfoMap sensorControls_;
> +	ControlInfoMap::Map algoControls_;
>   
>   	/* Interface to the Camera Helper */
>   	std::unique_ptr<CameraSensorHelper> camHelper_;
> @@ -193,6 +194,14 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
>   	if (ret)
>   		return ret;
>   
> +	for (auto const &a : algorithms()) {
> +		Algorithm *algo = static_cast<Algorithm *>(a.get());
> +
> +		/* \todo Avoid merging duplicate controls */
> +		if (!algo->controls_.empty())
> +			algoControls_.merge(ControlInfoMap::Map(algo->controls_));
> +	}
> +


If we settle on having a ControlInfoMap in the Algorithm class then I think this loop should move to 
updateControls() and just merge to ctrlMap directly, and algoControls_ can be dropped.

>   	/* Initialize controls. */
>   	updateControls(sensorInfo, sensorControls, ipaControls);
>   
> @@ -377,6 +386,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
>   			       ControlInfoMap *ipaControls)
>   {
>   	ControlInfoMap::Map ctrlMap = rkisp1Controls;
> +	ctrlMap.merge(algoControls_);
>   
>   	/*
>   	 * Compute exposure time limits from the V4L2_CID_EXPOSURE control
Paul Elder May 29, 2024, 7:44 a.m. UTC | #3 
On Mon, May 20, 2024 at 01:34:56PM +0100, Dan Scally wrote:
> Hi Paul, thanks for the patch
> 
> On 17/05/2024 09:08, Paul Elder wrote:
> > Plumb controls for setting metering mode, exposure mode, constraint
> > mode, and frame duration limits. Also report them as available controls,
> > as well as in metadata.
> > 
> > While at it, add the missing #include for tuple, as a std::tie is used.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v3:
> > - prevent maxShutterSpeed (for setLimits) from choosing 0 shutter speed
> >    if one of the inputs std::min() is not set (this fixes an assertion
> >    failure in the exposure mode helper)
> > 
> > Changes in v2:
> > - add #include <tuple>
> > - don't overwrite metering/exposure/constraint mode controls to default
> >    if no control is supplied, and retain the old value instead (same for
> >    frame duration limits)
> > ---
> >   src/ipa/rkisp1/algorithms/agc.cpp     | 55 ++++++++++++++++++++++++---
> >   src/ipa/rkisp1/algorithms/algorithm.h |  2 +
> >   src/ipa/rkisp1/ipa_context.h          | 10 ++++-
> >   src/ipa/rkisp1/rkisp1.cpp             | 10 +++++
> >   4 files changed, 70 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index 1c9872d02..ce8beae24 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -10,6 +10,8 @@
> >   #include <algorithm>
> >   #include <chrono>
> >   #include <cmath>
> > +#include <tuple>
> > +#include <vector>
> >   #include <libcamera/base/log.h>
> >   #include <libcamera/base/utils.h>
> > @@ -48,6 +50,7 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
> >   		return -EINVAL;
> >   	}
> > +	std::vector<ControlValue> availableMeteringModes;
> >   	for (const auto &[key, value] : yamlMeteringModes.asDict()) {
> >   		if (controls::AeMeteringModeNameValueMap.find(key) ==
> >   		    controls::AeMeteringModeNameValueMap.end()) {
> > @@ -66,17 +69,23 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
> >   				<< "Matched metering matrix mode "
> >   				<< key << ", version " << version;
> > -			meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;
> > +			int32_t control = controls::AeMeteringModeNameValueMap.at(key);
> > +			meteringModes_[control] = weights;
> > +			availableMeteringModes.push_back(control);
> >   		}
> >   	}
> > -	if (meteringModes_.empty()) {
> > +	if (availableMeteringModes.empty()) {
> >   		int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at("MeteringMatrix");
> >   		std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1);
> >   		meteringModes_[meteringModeId] = weights;
> > +		availableMeteringModes.push_back(meteringModeId);
> >   	}
> > +	Algorithm::controls_[&controls::AeMeteringMode] =
> > +		ControlInfo(availableMeteringModes);
> > +
> 
> 
> I was going to say "shouldn't this be controls()[&controls::AeMeteringMode]
> instead?", but I see that the patch adds a new ControlInfoMap to the
> Algorithm base class. I did a similar thing in the central AGC series with a
> ControlInfoMap in the context that algorithms fill with control data - we
> need to merge those methods and just do one or the other...I don't
> particularly mind which method we settle on, the only difference I can see
> at the moment is that the context.ctrlMap allows the algorithms to all fill
> the same ControlInfoMap and so avoids the needs to merge them all into
> IPARkISP1::algoControls_ with the loop in IPARkISP1::init()
> 

Yeah... I needed it for rkisp1's algorithms for reporting and merging
the valid controls and values, so to consolidate it with the one that
you added in agc I have them merged in ipa::rkisp1::algorithms::Agc::init():

Algorithm::controls_.merge(ControlInfoMap::Map(controls()));


Paul

> >   	return 0;
> >   }
> > @@ -137,6 +146,8 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
> >   	context.ctrlMap.merge(controls());
> > +	Algorithm::controls_.merge(ControlInfoMap::Map(controls()));
> > +
> >   	return 0;
> >   }
> > @@ -159,6 +170,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >   	context.activeState.agc.constraintMode = constraintModes().begin()->first;
> >   	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
> > +	context.activeState.agc.meteringMode = meteringModes_.begin()->first;
> >   	/*
> >   	 * Define the measurement window for AGC as a centered rectangle
> > @@ -169,7 +181,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >   	context.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
> >   	context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
> > -	/* \todo Run this again when FrameDurationLimits is passed in */
> >   	setLimits(context.configuration.sensor.minShutterSpeed,
> >   		  context.configuration.sensor.maxShutterSpeed,
> >   		  context.configuration.sensor.minAnalogueGain,
> > @@ -223,6 +234,26 @@ void Agc::queueRequest(IPAContext &context,
> >   		frameContext.agc.exposure = agc.manual.exposure;
> >   		frameContext.agc.gain = agc.manual.gain;
> >   	}
> > +
> > +	const auto &meteringMode = controls.get(controls::AeMeteringMode);
> > +	if (meteringMode)
> > +		agc.meteringMode = *meteringMode;
> > +	frameContext.agc.meteringMode = agc.meteringMode;
> > +
> > +	const auto &exposureMode = controls.get(controls::AeExposureMode);
> > +	if (exposureMode)
> > +		agc.exposureMode = *exposureMode;
> > +	frameContext.agc.exposureMode = agc.exposureMode;
> > +
> > +	const auto &constraintMode = controls.get(controls::AeConstraintMode);
> > +	if (constraintMode)
> > +		agc.constraintMode = *constraintMode;
> > +	frameContext.agc.constraintMode = agc.constraintMode;
> > +
> > +	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> > +	if (frameDurationLimits)
> > +		agc.maxShutterSpeed = std::chrono::milliseconds((*frameDurationLimits).back());
> > +	frameContext.agc.maxShutterSpeed = agc.maxShutterSpeed;
> >   }
> >   /**
> > @@ -261,8 +292,7 @@ void Agc::prepare(IPAContext &context, const uint32_t frame,
> >   		params->meas.hst_config.hist_weight,
> >   		context.hw->numHistogramWeights
> >   	};
> > -	/* \todo Get this from control */
> > -	std::vector<uint8_t> &modeWeights = meteringModes_.at(controls::MeteringMatrix);
> > +	std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);
> >   	std::copy(modeWeights.begin(), modeWeights.end(), weights.begin());
> >   	std::stringstream str;
> > @@ -289,6 +319,7 @@ 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);
> >   	/* \todo Use VBlank value calculated from each frame exposure. */
> >   	uint32_t vTotal = context.configuration.sensor.size.height
> > @@ -296,6 +327,10 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> >   	utils::Duration frameDuration = context.configuration.sensor.lineDuration
> >   				      * vTotal;
> >   	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
> > +
> > +	metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
> > +	metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
> > +	metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
> >   }
> >   /**
> > @@ -370,6 +405,16 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >   		       [](uint32_t x) { return x >> 4; });
> >   	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> > +	utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,
> > +						   frameContext.agc.maxShutterSpeed);
> > +	if (!maxShutterSpeed)
> > +		maxShutterSpeed = std::max(context.configuration.sensor.maxShutterSpeed,
> > +					   frameContext.agc.maxShutterSpeed);
> > +	setLimits(context.configuration.sensor.minShutterSpeed,
> > +		  maxShutterSpeed,
> > +		  context.configuration.sensor.minAnalogueGain,
> > +		  context.configuration.sensor.maxAnalogueGain);
> > +
> >   	/*
> >   	 * The Agc algorithm needs to know the effective exposure value that was
> >   	 * applied to the sensor when the statistics were collected.
> > diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
> > index 9454c9a1f..c3a002b85 100644
> > --- a/src/ipa/rkisp1/algorithms/algorithm.h
> > +++ b/src/ipa/rkisp1/algorithms/algorithm.h
> > @@ -25,6 +25,8 @@ public:
> >   	bool disabled_;
> >   	bool supportsRaw_;
> > +
> > +	ControlInfoMap::Map controls_;
> >   };
> >   } /* namespace ipa::rkisp1 */
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index 256b75ebc..26c395704 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -68,8 +68,10 @@ struct IPAActiveState {
> >   		} automatic;
> >   		bool autoEnabled;
> > -		uint32_t constraintMode;
> > -		uint32_t exposureMode;
> > +		int32_t constraintMode;
> > +		int32_t exposureMode;
> > +		int32_t meteringMode;
> > +		utils::Duration maxShutterSpeed;
> >   	} agc;
> >   	struct {
> > @@ -111,6 +113,10 @@ struct IPAFrameContext : public FrameContext {
> >   		uint32_t exposure;
> >   		double gain;
> >   		bool autoEnabled;
> > +		int32_t exposureMode;
> > +		int32_t constraintMode;
> > +		int32_t meteringMode;
> > +		utils::Duration maxShutterSpeed;
> >   	} agc;
> >   	struct {
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 963cdbab4..7716c11bb 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -80,6 +80,7 @@ private:
> >   	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
> >   	ControlInfoMap sensorControls_;
> > +	ControlInfoMap::Map algoControls_;
> >   	/* Interface to the Camera Helper */
> >   	std::unique_ptr<CameraSensorHelper> camHelper_;
> > @@ -193,6 +194,14 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> >   	if (ret)
> >   		return ret;
> > +	for (auto const &a : algorithms()) {
> > +		Algorithm *algo = static_cast<Algorithm *>(a.get());
> > +
> > +		/* \todo Avoid merging duplicate controls */
> > +		if (!algo->controls_.empty())
> > +			algoControls_.merge(ControlInfoMap::Map(algo->controls_));
> > +	}
> > +
> 
> 
> If we settle on having a ControlInfoMap in the Algorithm class then I think
> this loop should move to updateControls() and just merge to ctrlMap
> directly, and algoControls_ can be dropped.
> 
> >   	/* Initialize controls. */
> >   	updateControls(sensorInfo, sensorControls, ipaControls);
> > @@ -377,6 +386,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
> >   			       ControlInfoMap *ipaControls)
> >   {
> >   	ControlInfoMap::Map ctrlMap = rkisp1Controls;
> > +	ctrlMap.merge(algoControls_);
> >   	/*
> >   	 * Compute exposure time limits from the V4L2_CID_EXPOSURE control

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index 1c9872d02..ce8beae24 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -10,6 +10,8 @@ 
 #include <algorithm>
 #include <chrono>
 #include <cmath>
+#include <tuple>
+#include <vector>
 
 #include <libcamera/base/log.h>
 #include <libcamera/base/utils.h>
@@ -48,6 +50,7 @@  int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
 		return -EINVAL;
 	}
 
+	std::vector<ControlValue> availableMeteringModes;
 	for (const auto &[key, value] : yamlMeteringModes.asDict()) {
 		if (controls::AeMeteringModeNameValueMap.find(key) ==
 		    controls::AeMeteringModeNameValueMap.end()) {
@@ -66,17 +69,23 @@  int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData,
 				<< "Matched metering matrix mode "
 				<< key << ", version " << version;
 
-			meteringModes_[controls::AeMeteringModeNameValueMap.at(key)] = weights;
+			int32_t control = controls::AeMeteringModeNameValueMap.at(key);
+			meteringModes_[control] = weights;
+			availableMeteringModes.push_back(control);
 		}
 	}
 
-	if (meteringModes_.empty()) {
+	if (availableMeteringModes.empty()) {
 		int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at("MeteringMatrix");
 		std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1);
 
 		meteringModes_[meteringModeId] = weights;
+		availableMeteringModes.push_back(meteringModeId);
 	}
 
+	Algorithm::controls_[&controls::AeMeteringMode] =
+		ControlInfo(availableMeteringModes);
+
 	return 0;
 }
 
@@ -137,6 +146,8 @@  int Agc::init(IPAContext &context, const YamlObject &tuningData)
 
 	context.ctrlMap.merge(controls());
 
+	Algorithm::controls_.merge(ControlInfoMap::Map(controls()));
+
 	return 0;
 }
 
@@ -159,6 +170,7 @@  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
 
 	context.activeState.agc.constraintMode = constraintModes().begin()->first;
 	context.activeState.agc.exposureMode = exposureModeHelpers().begin()->first;
+	context.activeState.agc.meteringMode = meteringModes_.begin()->first;
 
 	/*
 	 * Define the measurement window for AGC as a centered rectangle
@@ -169,7 +181,6 @@  int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
 	context.configuration.agc.measureWindow.h_size = 3 * configInfo.outputSize.width / 4;
 	context.configuration.agc.measureWindow.v_size = 3 * configInfo.outputSize.height / 4;
 
-	/* \todo Run this again when FrameDurationLimits is passed in */
 	setLimits(context.configuration.sensor.minShutterSpeed,
 		  context.configuration.sensor.maxShutterSpeed,
 		  context.configuration.sensor.minAnalogueGain,
@@ -223,6 +234,26 @@  void Agc::queueRequest(IPAContext &context,
 		frameContext.agc.exposure = agc.manual.exposure;
 		frameContext.agc.gain = agc.manual.gain;
 	}
+
+	const auto &meteringMode = controls.get(controls::AeMeteringMode);
+	if (meteringMode)
+		agc.meteringMode = *meteringMode;
+	frameContext.agc.meteringMode = agc.meteringMode;
+
+	const auto &exposureMode = controls.get(controls::AeExposureMode);
+	if (exposureMode)
+		agc.exposureMode = *exposureMode;
+	frameContext.agc.exposureMode = agc.exposureMode;
+
+	const auto &constraintMode = controls.get(controls::AeConstraintMode);
+	if (constraintMode)
+		agc.constraintMode = *constraintMode;
+	frameContext.agc.constraintMode = agc.constraintMode;
+
+	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
+	if (frameDurationLimits)
+		agc.maxShutterSpeed = std::chrono::milliseconds((*frameDurationLimits).back());
+	frameContext.agc.maxShutterSpeed = agc.maxShutterSpeed;
 }
 
 /**
@@ -261,8 +292,7 @@  void Agc::prepare(IPAContext &context, const uint32_t frame,
 		params->meas.hst_config.hist_weight,
 		context.hw->numHistogramWeights
 	};
-	/* \todo Get this from control */
-	std::vector<uint8_t> &modeWeights = meteringModes_.at(controls::MeteringMatrix);
+	std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);
 	std::copy(modeWeights.begin(), modeWeights.end(), weights.begin());
 
 	std::stringstream str;
@@ -289,6 +319,7 @@  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);
 
 	/* \todo Use VBlank value calculated from each frame exposure. */
 	uint32_t vTotal = context.configuration.sensor.size.height
@@ -296,6 +327,10 @@  void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
 	utils::Duration frameDuration = context.configuration.sensor.lineDuration
 				      * vTotal;
 	metadata.set(controls::FrameDuration, frameDuration.get<std::micro>());
+
+	metadata.set(controls::AeMeteringMode, frameContext.agc.meteringMode);
+	metadata.set(controls::AeExposureMode, frameContext.agc.exposureMode);
+	metadata.set(controls::AeConstraintMode, frameContext.agc.constraintMode);
 }
 
 /**
@@ -370,6 +405,16 @@  void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
 		       [](uint32_t x) { return x >> 4; });
 	expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
 
+	utils::Duration maxShutterSpeed = std::min(context.configuration.sensor.maxShutterSpeed,
+						   frameContext.agc.maxShutterSpeed);
+	if (!maxShutterSpeed)
+		maxShutterSpeed = std::max(context.configuration.sensor.maxShutterSpeed,
+					   frameContext.agc.maxShutterSpeed);
+	setLimits(context.configuration.sensor.minShutterSpeed,
+		  maxShutterSpeed,
+		  context.configuration.sensor.minAnalogueGain,
+		  context.configuration.sensor.maxAnalogueGain);
+
 	/*
 	 * The Agc algorithm needs to know the effective exposure value that was
 	 * applied to the sensor when the statistics were collected.
diff --git a/src/ipa/rkisp1/algorithms/algorithm.h b/src/ipa/rkisp1/algorithms/algorithm.h
index 9454c9a1f..c3a002b85 100644
--- a/src/ipa/rkisp1/algorithms/algorithm.h
+++ b/src/ipa/rkisp1/algorithms/algorithm.h
@@ -25,6 +25,8 @@  public:
 
 	bool disabled_;
 	bool supportsRaw_;
+
+	ControlInfoMap::Map controls_;
 };
 
 } /* namespace ipa::rkisp1 */
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
index 256b75ebc..26c395704 100644
--- a/src/ipa/rkisp1/ipa_context.h
+++ b/src/ipa/rkisp1/ipa_context.h
@@ -68,8 +68,10 @@  struct IPAActiveState {
 		} automatic;
 
 		bool autoEnabled;
-		uint32_t constraintMode;
-		uint32_t exposureMode;
+		int32_t constraintMode;
+		int32_t exposureMode;
+		int32_t meteringMode;
+		utils::Duration maxShutterSpeed;
 	} agc;
 
 	struct {
@@ -111,6 +113,10 @@  struct IPAFrameContext : public FrameContext {
 		uint32_t exposure;
 		double gain;
 		bool autoEnabled;
+		int32_t exposureMode;
+		int32_t constraintMode;
+		int32_t meteringMode;
+		utils::Duration maxShutterSpeed;
 	} agc;
 
 	struct {
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
index 963cdbab4..7716c11bb 100644
--- a/src/ipa/rkisp1/rkisp1.cpp
+++ b/src/ipa/rkisp1/rkisp1.cpp
@@ -80,6 +80,7 @@  private:
 	std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
 
 	ControlInfoMap sensorControls_;
+	ControlInfoMap::Map algoControls_;
 
 	/* Interface to the Camera Helper */
 	std::unique_ptr<CameraSensorHelper> camHelper_;
@@ -193,6 +194,14 @@  int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
 	if (ret)
 		return ret;
 
+	for (auto const &a : algorithms()) {
+		Algorithm *algo = static_cast<Algorithm *>(a.get());
+
+		/* \todo Avoid merging duplicate controls */
+		if (!algo->controls_.empty())
+			algoControls_.merge(ControlInfoMap::Map(algo->controls_));
+	}
+
 	/* Initialize controls. */
 	updateControls(sensorInfo, sensorControls, ipaControls);
 
@@ -377,6 +386,7 @@  void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
 			       ControlInfoMap *ipaControls)
 {
 	ControlInfoMap::Map ctrlMap = rkisp1Controls;
+	ctrlMap.merge(algoControls_);
 
 	/*
 	 * Compute exposure time limits from the V4L2_CID_EXPOSURE control