[3/3] rkisp1: Honor the FrameDurationLimits control

21623 diff mbox series

From: Paul Elder <paul.elder@ideasonboard.com>

Add support to rkisp1 for controlling the framerate via the
FrameDurationLimits control.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp        | 52 ++++++++++++++++++------
 src/ipa/rkisp1/ipa_context.cpp           | 16 +++++++-
 src/ipa/rkisp1/ipa_context.h             |  4 ++
 src/ipa/rkisp1/rkisp1.cpp                |  2 +
 src/libcamera/pipeline/rkisp1/rkisp1.cpp |  7 ++++
 5 files changed, 68 insertions(+), 13 deletions(-)

Hi Kieran, Paul

Thank you for the patch.

On 14/10/24 9:17 pm, Kieran Bingham wrote:
> From: Paul Elder <paul.elder@ideasonboard.com>
>
> Add support to rkisp1 for controlling the framerate via the
> FrameDurationLimits control.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   src/ipa/rkisp1/algorithms/agc.cpp        | 52 ++++++++++++++++++------
>   src/ipa/rkisp1/ipa_context.cpp           | 16 +++++++-
>   src/ipa/rkisp1/ipa_context.h             |  4 ++
>   src/ipa/rkisp1/rkisp1.cpp                |  2 +
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp |  7 ++++
>   5 files changed, 68 insertions(+), 13 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index e23ab120b3e2..088ecf42c480 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>   
>   	/* Limit the frame duration to match current initialisation */
>   	ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];
> +	context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());
>   	context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());
>   
>   	/*
> @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,
>   
>   	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
>   	if (frameDurationLimits) {
> -		utils::Duration maxFrameDuration =
> -			std::chrono::milliseconds((*frameDurationLimits).back());
> -		agc.maxFrameDuration = maxFrameDuration;
> +		/* Limit the control value to the limits in ControlInfo */
> +		ControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];
> +		int64_t minFrameDuration =
> +			std::clamp((*frameDurationLimits).front(),
> +				   limits.min().get<int64_t>(),
> +				   limits.max().get<int64_t>());
> +		int64_t maxFrameDuration =
> +			std::clamp((*frameDurationLimits).back(),
> +				   limits.min().get<int64_t>(),
> +				   limits.max().get<int64_t>());
> +
> +		agc.minFrameDuration = std::chrono::microseconds(minFrameDuration);
> +		agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);
>   	}
> +	frameContext.agc.minFrameDuration = agc.minFrameDuration;
>   	frameContext.agc.maxFrameDuration = agc.maxFrameDuration;
>   }
>   
> @@ -330,15 +342,8 @@ 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::FrameDuration, frameContext.agc.frameDuration.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
> -			+ context.configuration.sensor.defVBlank;
> -	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);
> @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>   		return;
>   	}
>   
> +	IPACameraSensorInfo &sensorInfo = context.sensorInfo;
>   	utils::Duration lineDuration = context.configuration.sensor.lineDuration;
>   
>   	/*
> @@ -418,11 +424,19 @@ 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 };
>   
> +	/*
> +	 * Limit the allowed shutter speeds for the exposure helper based on
> +	 * the frame duration limits.
> +	 */
> +	utils::Duration minShutterSpeed =
> +		std::clamp(frameContext.agc.minFrameDuration,
> +			   context.configuration.sensor.minShutterSpeed,
> +			   context.configuration.sensor.maxShutterSpeed);
>   	utils::Duration maxShutterSpeed =
>   		std::clamp(frameContext.agc.maxFrameDuration,
>   			   context.configuration.sensor.minShutterSpeed,
>   			   context.configuration.sensor.maxShutterSpeed);
> -	setLimits(context.configuration.sensor.minShutterSpeed,
> +	setLimits(minShutterSpeed,
>   		  maxShutterSpeed,
>   		  context.configuration.sensor.minAnalogueGain,
>   		  context.configuration.sensor.maxAnalogueGain);
> @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>   	activeState.agc.automatic.exposure = shutterTime / lineDuration;
>   	activeState.agc.automatic.gain = aGain;
>   
> +	/*
> +	 * Determine what our FrameDuration must be and adapt VBLANK to suit
> +	 * this if we need to expand the shutter time calculated to fill the
> +	 * remaining time so that we do not run faster than the minimum frame
> +	 * duration (maximum requested frame rate) when we have short exposures.
> +	 */

Feels like this should be broken down to atleast two sentences.

    +	/*
    +	 * Determine what our FrameDuration must be and adapt VBLANK to suit
    +	 * it. If we need to expand the shutter time, calculate VBLANK to fill the
    +	 * remaining time so that we do not run faster than the minimum frame
    +	 * duration (maximum requested frame rate) when we have short exposures.
    +	 */


Other than this,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> +	utils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,
> +						 shutterTime);
> +
> +	frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;
> +
> +	/* Update accounting for line length quantization. */
> +	frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;
> +
>   	fillMetadata(context, frameContext, metadata);
>   	expMeans_ = {};
>   }
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 14d0c02a2b32..c5eb0d64ec29 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {
>    * \var IPAActiveState::agc.meteringMode
>    * \brief Metering mode as set by the AeMeteringMode control
>    *
> + * \var IPAActiveState::agc.minFrameDuration
> + * \brief Minimum frame duration as set by the FrameDurationLimits control
> + *
>    * \var IPAActiveState::agc.maxFrameDuration
>    * \brief Maximum frame duration as set by the FrameDurationLimits control
>    */
> @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {
>    * \brief Automatic Gain Control parameters for this frame
>    *
>    * The exposure and gain are provided by the AGC algorithm, and are to be
> - * applied to the sensor in order to take effect for this frame.
> + * applied to the sensor in order to take effect for this frame. Additionally
> + * the vertical blanking period is determined to maintain a consistent frame
> + * rate matched to the FrameDurationLimits as set by the user.
>    *
>    * \var IPAFrameContext::agc.exposure
>    * \brief Exposure time expressed as a number of lines computed by the algorithm
> @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {
>    *
>    * The gain should be adapted to the sensor specific gain code before applying.
>    *
> + * \var IPAFrameContext::agc.vblank
> + * \brief Vertical blanking parameter computed by the algorithm
> + *
>    * \var IPAFrameContext::agc.autoEnabled
>    * \brief Manual/automatic AGC state as set by the AeEnable control
>    *
> @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {
>    * \var IPAFrameContext::agc.meteringMode
>    * \brief Metering mode as set by the AeMeteringMode control
>    *
> + * \var IPAFrameContext::agc.minFrameDuration
> + * \brief Minimum frame duration as set by the FrameDurationLimits control
> + *
>    * \var IPAFrameContext::agc.maxFrameDuration
>    * \brief Maximum frame duration as set by the FrameDurationLimits control
>    *
> + * \var IPAFrameContext::agc.frameDuration
> + * \brief The actual FrameDuration used by the algorithm for the frame
> + *
>    * \var IPAFrameContext::agc.updateMetering
>    * \brief Indicate if new ISP AGC metering parameters need to be applied
>    */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index e274d9b01e1c..60c4d647f1ef 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -79,6 +79,7 @@ struct IPAActiveState {
>   		controls::AeConstraintModeEnum constraintMode;
>   		controls::AeExposureModeEnum exposureMode;
>   		controls::AeMeteringModeEnum meteringMode;
> +		utils::Duration minFrameDuration;
>   		utils::Duration maxFrameDuration;
>   	} agc;
>   
> @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {
>   	struct {
>   		uint32_t exposure;
>   		double gain;
> +		uint32_t vblank;
>   		bool autoEnabled;
>   		controls::AeConstraintModeEnum constraintMode;
>   		controls::AeExposureModeEnum exposureMode;
>   		controls::AeMeteringModeEnum meteringMode;
> +		utils::Duration minFrameDuration;
>   		utils::Duration maxFrameDuration;
> +		utils::Duration frameDuration;
>   		bool updateMetering;
>   	} agc;
>   
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 47777ece783f..17d56fb4e901 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)
>   	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>   	uint32_t exposure = frameContext.agc.exposure;
>   	uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
> +	uint32_t vblank = frameContext.agc.vblank;
>   
>   	ControlList ctrls(sensorControls_);
>   	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
>   	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> +	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
>   
>   	setSensorControls.emit(frame, ctrls);
>   }
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 42961c120036..1ec12185bb78 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)
>   void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
>   					 const ControlList &sensorControls)
>   {
> +	if (frame == 0) {
> +		ControlList controls = sensorControls;
> +		sensor_->setControls(&controls);
> +		return;
> +	}
> +
>   	delayedCtrls_->push(sensorControls);
>   }
>   
> @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>   	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
>   		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
>   		{ V4L2_CID_EXPOSURE, { 2, false } },
> +		{ V4L2_CID_VBLANK, { 1, false } },
>   	};
>   
>   	data->delayedCtrls_ =

Hi Kieran

On Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:
> From: Paul Elder <paul.elder@ideasonboard.com>
>
> Add support to rkisp1 for controlling the framerate via the
> FrameDurationLimits control.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp        | 52 ++++++++++++++++++------
>  src/ipa/rkisp1/ipa_context.cpp           | 16 +++++++-
>  src/ipa/rkisp1/ipa_context.h             |  4 ++
>  src/ipa/rkisp1/rkisp1.cpp                |  2 +
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  7 ++++
>  5 files changed, 68 insertions(+), 13 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index e23ab120b3e2..088ecf42c480 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
>
>  	/* Limit the frame duration to match current initialisation */
>  	ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];
> +	context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());
>  	context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());
>
>  	/*
> @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,
>
>  	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
>  	if (frameDurationLimits) {
> -		utils::Duration maxFrameDuration =
> -			std::chrono::milliseconds((*frameDurationLimits).back());
> -		agc.maxFrameDuration = maxFrameDuration;
> +		/* Limit the control value to the limits in ControlInfo */
> +		ControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];
> +		int64_t minFrameDuration =
> +			std::clamp((*frameDurationLimits).front(),
> +				   limits.min().get<int64_t>(),
> +				   limits.max().get<int64_t>());
> +		int64_t maxFrameDuration =
> +			std::clamp((*frameDurationLimits).back(),
> +				   limits.min().get<int64_t>(),
> +				   limits.max().get<int64_t>());
> +

We operate on the assumption the control is well-formed, right ? iow
that frameDurationLimits[0] < frameDurationLimits[1] ?

> +		agc.minFrameDuration = std::chrono::microseconds(minFrameDuration);
> +		agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);
>  	}
> +	frameContext.agc.minFrameDuration = agc.minFrameDuration;
>  	frameContext.agc.maxFrameDuration = agc.maxFrameDuration;
>  }
>
> @@ -330,15 +342,8 @@ 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::FrameDuration, frameContext.agc.frameDuration.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
> -			+ context.configuration.sensor.defVBlank;
> -	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);
> @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  		return;
>  	}
>
> +	IPACameraSensorInfo &sensorInfo = context.sensorInfo;
>  	utils::Duration lineDuration = context.configuration.sensor.lineDuration;
>
>  	/*
> @@ -418,11 +424,19 @@ 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 };
>
> +	/*
> +	 * Limit the allowed shutter speeds for the exposure helper based on
> +	 * the frame duration limits.
> +	 */
> +	utils::Duration minShutterSpeed =
> +		std::clamp(frameContext.agc.minFrameDuration,
> +			   context.configuration.sensor.minShutterSpeed,
> +			   context.configuration.sensor.maxShutterSpeed);
>  	utils::Duration maxShutterSpeed =
>  		std::clamp(frameContext.agc.maxFrameDuration,
>  			   context.configuration.sensor.minShutterSpeed,
>  			   context.configuration.sensor.maxShutterSpeed);
> -	setLimits(context.configuration.sensor.minShutterSpeed,
> +	setLimits(minShutterSpeed,
>  		  maxShutterSpeed,
>  		  context.configuration.sensor.minAnalogueGain,
>  		  context.configuration.sensor.maxAnalogueGain);
> @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  	activeState.agc.automatic.exposure = shutterTime / lineDuration;
>  	activeState.agc.automatic.gain = aGain;
>
> +	/*
> +	 * Determine what our FrameDuration must be and adapt VBLANK to suit
> +	 * this if we need to expand the shutter time calculated to fill the
> +	 * remaining time so that we do not run faster than the minimum frame
> +	 * duration (maximum requested frame rate) when we have short exposures.
> +	 */
> +	utils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,
> +						 shutterTime);
> +
> +	frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;
> +
> +	/* Update accounting for line length quantization. */
> +	frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;
> +
>  	fillMetadata(context, frameContext, metadata);
>  	expMeans_ = {};
>  }
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 14d0c02a2b32..c5eb0d64ec29 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {
>   * \var IPAActiveState::agc.meteringMode
>   * \brief Metering mode as set by the AeMeteringMode control
>   *
> + * \var IPAActiveState::agc.minFrameDuration
> + * \brief Minimum frame duration as set by the FrameDurationLimits control
> + *
>   * \var IPAActiveState::agc.maxFrameDuration
>   * \brief Maximum frame duration as set by the FrameDurationLimits control
>   */
> @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {
>   * \brief Automatic Gain Control parameters for this frame
>   *
>   * The exposure and gain are provided by the AGC algorithm, and are to be
> - * applied to the sensor in order to take effect for this frame.
> + * applied to the sensor in order to take effect for this frame. Additionally
> + * the vertical blanking period is determined to maintain a consistent frame
> + * rate matched to the FrameDurationLimits as set by the user.
>   *
>   * \var IPAFrameContext::agc.exposure
>   * \brief Exposure time expressed as a number of lines computed by the algorithm
> @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {
>   *
>   * The gain should be adapted to the sensor specific gain code before applying.
>   *
> + * \var IPAFrameContext::agc.vblank
> + * \brief Vertical blanking parameter computed by the algorithm
> + *
>   * \var IPAFrameContext::agc.autoEnabled
>   * \brief Manual/automatic AGC state as set by the AeEnable control
>   *
> @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {
>   * \var IPAFrameContext::agc.meteringMode
>   * \brief Metering mode as set by the AeMeteringMode control
>   *
> + * \var IPAFrameContext::agc.minFrameDuration
> + * \brief Minimum frame duration as set by the FrameDurationLimits control
> + *
>   * \var IPAFrameContext::agc.maxFrameDuration
>   * \brief Maximum frame duration as set by the FrameDurationLimits control
>   *
> + * \var IPAFrameContext::agc.frameDuration
> + * \brief The actual FrameDuration used by the algorithm for the frame
> + *
>   * \var IPAFrameContext::agc.updateMetering
>   * \brief Indicate if new ISP AGC metering parameters need to be applied
>   */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index e274d9b01e1c..60c4d647f1ef 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -79,6 +79,7 @@ struct IPAActiveState {
>  		controls::AeConstraintModeEnum constraintMode;
>  		controls::AeExposureModeEnum exposureMode;
>  		controls::AeMeteringModeEnum meteringMode;
> +		utils::Duration minFrameDuration;
>  		utils::Duration maxFrameDuration;
>  	} agc;
>
> @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {
>  	struct {
>  		uint32_t exposure;
>  		double gain;
> +		uint32_t vblank;
>  		bool autoEnabled;
>  		controls::AeConstraintModeEnum constraintMode;
>  		controls::AeExposureModeEnum exposureMode;
>  		controls::AeMeteringModeEnum meteringMode;
> +		utils::Duration minFrameDuration;
>  		utils::Duration maxFrameDuration;
> +		utils::Duration frameDuration;
>  		bool updateMetering;
>  	} agc;
>
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 47777ece783f..17d56fb4e901 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)
>  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>  	uint32_t exposure = frameContext.agc.exposure;
>  	uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
> +	uint32_t vblank = frameContext.agc.vblank;
>
>  	ControlList ctrls(sensorControls_);
>  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
>  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> +	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
>
>  	setSensorControls.emit(frame, ctrls);
>  }
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 42961c120036..1ec12185bb78 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)
>  void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
>  					 const ControlList &sensorControls)
>  {
> +	if (frame == 0) {
> +		ControlList controls = sensorControls;
> +		sensor_->setControls(&controls);
> +		return;
> +	}
> +

I might have missed what is this for :)

>  	delayedCtrls_->push(sensorControls);
>  }
>
> @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
>  		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
>  		{ V4L2_CID_EXPOSURE, { 2, false } },
> +		{ V4L2_CID_VBLANK, { 1, false } },
>  	};
>
>  	data->delayedCtrls_ =
> --
> 2.34.1
>

On Thu, Oct 24, 2024 at 05:53:01PM +0200, Jacopo Mondi wrote:
> Hi Kieran
> 
> On Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:
> > From: Paul Elder <paul.elder@ideasonboard.com>
> >
> > Add support to rkisp1 for controlling the framerate via the
> > FrameDurationLimits control.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/agc.cpp        | 52 ++++++++++++++++++------
> >  src/ipa/rkisp1/ipa_context.cpp           | 16 +++++++-
> >  src/ipa/rkisp1/ipa_context.h             |  4 ++
> >  src/ipa/rkisp1/rkisp1.cpp                |  2 +
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  7 ++++
> >  5 files changed, 68 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index e23ab120b3e2..088ecf42c480 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> >
> >  	/* Limit the frame duration to match current initialisation */
> >  	ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];
> > +	context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());
> >  	context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());
> >
> >  	/*
> > @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,
> >
> >  	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> >  	if (frameDurationLimits) {
> > -		utils::Duration maxFrameDuration =
> > -			std::chrono::milliseconds((*frameDurationLimits).back());
> > -		agc.maxFrameDuration = maxFrameDuration;
> > +		/* Limit the control value to the limits in ControlInfo */
> > +		ControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];
> > +		int64_t minFrameDuration =
> > +			std::clamp((*frameDurationLimits).front(),
> > +				   limits.min().get<int64_t>(),
> > +				   limits.max().get<int64_t>());
> > +		int64_t maxFrameDuration =
> > +			std::clamp((*frameDurationLimits).back(),
> > +				   limits.min().get<int64_t>(),
> > +				   limits.max().get<int64_t>());
> > +
> 
> We operate on the assumption the control is well-formed, right ? iow
> that frameDurationLimits[0] < frameDurationLimits[1] ?

Yeah.

> 
> > +		agc.minFrameDuration = std::chrono::microseconds(minFrameDuration);
> > +		agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);
> >  	}
> > +	frameContext.agc.minFrameDuration = agc.minFrameDuration;
> >  	frameContext.agc.maxFrameDuration = agc.maxFrameDuration;
> >  }
> >
> > @@ -330,15 +342,8 @@ 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::FrameDuration, frameContext.agc.frameDuration.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
> > -			+ context.configuration.sensor.defVBlank;
> > -	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);
> > @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >  		return;
> >  	}
> >
> > +	IPACameraSensorInfo &sensorInfo = context.sensorInfo;
> >  	utils::Duration lineDuration = context.configuration.sensor.lineDuration;
> >
> >  	/*
> > @@ -418,11 +424,19 @@ 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 };
> >
> > +	/*
> > +	 * Limit the allowed shutter speeds for the exposure helper based on
> > +	 * the frame duration limits.
> > +	 */
> > +	utils::Duration minShutterSpeed =
> > +		std::clamp(frameContext.agc.minFrameDuration,
> > +			   context.configuration.sensor.minShutterSpeed,
> > +			   context.configuration.sensor.maxShutterSpeed);
> >  	utils::Duration maxShutterSpeed =
> >  		std::clamp(frameContext.agc.maxFrameDuration,
> >  			   context.configuration.sensor.minShutterSpeed,
> >  			   context.configuration.sensor.maxShutterSpeed);
> > -	setLimits(context.configuration.sensor.minShutterSpeed,
> > +	setLimits(minShutterSpeed,
> >  		  maxShutterSpeed,
> >  		  context.configuration.sensor.minAnalogueGain,
> >  		  context.configuration.sensor.maxAnalogueGain);
> > @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >  	activeState.agc.automatic.exposure = shutterTime / lineDuration;
> >  	activeState.agc.automatic.gain = aGain;
> >
> > +	/*
> > +	 * Determine what our FrameDuration must be and adapt VBLANK to suit
> > +	 * this if we need to expand the shutter time calculated to fill the
> > +	 * remaining time so that we do not run faster than the minimum frame
> > +	 * duration (maximum requested frame rate) when we have short exposures.
> > +	 */
> > +	utils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,
> > +						 shutterTime);
> > +
> > +	frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;
> > +
> > +	/* Update accounting for line length quantization. */
> > +	frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;
> > +
> >  	fillMetadata(context, frameContext, metadata);
> >  	expMeans_ = {};
> >  }
> > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > index 14d0c02a2b32..c5eb0d64ec29 100644
> > --- a/src/ipa/rkisp1/ipa_context.cpp
> > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {
> >   * \var IPAActiveState::agc.meteringMode
> >   * \brief Metering mode as set by the AeMeteringMode control
> >   *
> > + * \var IPAActiveState::agc.minFrameDuration
> > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > + *
> >   * \var IPAActiveState::agc.maxFrameDuration
> >   * \brief Maximum frame duration as set by the FrameDurationLimits control
> >   */
> > @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {
> >   * \brief Automatic Gain Control parameters for this frame
> >   *
> >   * The exposure and gain are provided by the AGC algorithm, and are to be
> > - * applied to the sensor in order to take effect for this frame.
> > + * applied to the sensor in order to take effect for this frame. Additionally
> > + * the vertical blanking period is determined to maintain a consistent frame
> > + * rate matched to the FrameDurationLimits as set by the user.
> >   *
> >   * \var IPAFrameContext::agc.exposure
> >   * \brief Exposure time expressed as a number of lines computed by the algorithm
> > @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {
> >   *
> >   * The gain should be adapted to the sensor specific gain code before applying.
> >   *
> > + * \var IPAFrameContext::agc.vblank
> > + * \brief Vertical blanking parameter computed by the algorithm
> > + *
> >   * \var IPAFrameContext::agc.autoEnabled
> >   * \brief Manual/automatic AGC state as set by the AeEnable control
> >   *
> > @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {
> >   * \var IPAFrameContext::agc.meteringMode
> >   * \brief Metering mode as set by the AeMeteringMode control
> >   *
> > + * \var IPAFrameContext::agc.minFrameDuration
> > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > + *
> >   * \var IPAFrameContext::agc.maxFrameDuration
> >   * \brief Maximum frame duration as set by the FrameDurationLimits control
> >   *
> > + * \var IPAFrameContext::agc.frameDuration
> > + * \brief The actual FrameDuration used by the algorithm for the frame
> > + *
> >   * \var IPAFrameContext::agc.updateMetering
> >   * \brief Indicate if new ISP AGC metering parameters need to be applied
> >   */
> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > index e274d9b01e1c..60c4d647f1ef 100644
> > --- a/src/ipa/rkisp1/ipa_context.h
> > +++ b/src/ipa/rkisp1/ipa_context.h
> > @@ -79,6 +79,7 @@ struct IPAActiveState {
> >  		controls::AeConstraintModeEnum constraintMode;
> >  		controls::AeExposureModeEnum exposureMode;
> >  		controls::AeMeteringModeEnum meteringMode;
> > +		utils::Duration minFrameDuration;
> >  		utils::Duration maxFrameDuration;
> >  	} agc;
> >
> > @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {
> >  	struct {
> >  		uint32_t exposure;
> >  		double gain;
> > +		uint32_t vblank;
> >  		bool autoEnabled;
> >  		controls::AeConstraintModeEnum constraintMode;
> >  		controls::AeExposureModeEnum exposureMode;
> >  		controls::AeMeteringModeEnum meteringMode;
> > +		utils::Duration minFrameDuration;
> >  		utils::Duration maxFrameDuration;
> > +		utils::Duration frameDuration;
> >  		bool updateMetering;
> >  	} agc;
> >
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 47777ece783f..17d56fb4e901 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)
> >  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> >  	uint32_t exposure = frameContext.agc.exposure;
> >  	uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
> > +	uint32_t vblank = frameContext.agc.vblank;
> >
> >  	ControlList ctrls(sensorControls_);
> >  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> >  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> > +	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
> >
> >  	setSensorControls.emit(frame, ctrls);
> >  }
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 42961c120036..1ec12185bb78 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)
> >  void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
> >  					 const ControlList &sensorControls)
> >  {
> > +	if (frame == 0) {
> > +		ControlList controls = sensorControls;
> > +		sensor_->setControls(&controls);
> > +		return;
> > +	}
> > +
> 
> I might have missed what is this for :)

iirc I needed this because the vlbank wasn't being applied...
until the next time the control was set.


Paul

> 
> >  	delayedCtrls_->push(sensorControls);
> >  }
> >
> > @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> >  	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> >  		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> >  		{ V4L2_CID_EXPOSURE, { 2, false } },
> > +		{ V4L2_CID_VBLANK, { 1, false } },
> >  	};
> >
> >  	data->delayedCtrls_ =
> > --
> > 2.34.1
> >

Hi Paul

On Mon, Nov 04, 2024 at 08:25:02PM +0900, Paul Elder wrote:
> On Thu, Oct 24, 2024 at 05:53:01PM +0200, Jacopo Mondi wrote:
> > Hi Kieran
> >
> > On Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:
> > > From: Paul Elder <paul.elder@ideasonboard.com>
> > >
> > > Add support to rkisp1 for controlling the framerate via the
> > > FrameDurationLimits control.
> > >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/algorithms/agc.cpp        | 52 ++++++++++++++++++------
> > >  src/ipa/rkisp1/ipa_context.cpp           | 16 +++++++-
> > >  src/ipa/rkisp1/ipa_context.h             |  4 ++
> > >  src/ipa/rkisp1/rkisp1.cpp                |  2 +
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  7 ++++
> > >  5 files changed, 68 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index e23ab120b3e2..088ecf42c480 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > >
> > >  	/* Limit the frame duration to match current initialisation */
> > >  	ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];
> > > +	context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());
> > >  	context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());
> > >
> > >  	/*
> > > @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,
> > >
> > >  	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> > >  	if (frameDurationLimits) {
> > > -		utils::Duration maxFrameDuration =
> > > -			std::chrono::milliseconds((*frameDurationLimits).back());
> > > -		agc.maxFrameDuration = maxFrameDuration;
> > > +		/* Limit the control value to the limits in ControlInfo */
> > > +		ControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];
> > > +		int64_t minFrameDuration =
> > > +			std::clamp((*frameDurationLimits).front(),
> > > +				   limits.min().get<int64_t>(),
> > > +				   limits.max().get<int64_t>());
> > > +		int64_t maxFrameDuration =
> > > +			std::clamp((*frameDurationLimits).back(),
> > > +				   limits.min().get<int64_t>(),
> > > +				   limits.max().get<int64_t>());
> > > +
> >
> > We operate on the assumption the control is well-formed, right ? iow
> > that frameDurationLimits[0] < frameDurationLimits[1] ?
>
> Yeah.
>
> >
> > > +		agc.minFrameDuration = std::chrono::microseconds(minFrameDuration);
> > > +		agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);
> > >  	}
> > > +	frameContext.agc.minFrameDuration = agc.minFrameDuration;
> > >  	frameContext.agc.maxFrameDuration = agc.maxFrameDuration;
> > >  }
> > >
> > > @@ -330,15 +342,8 @@ 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::FrameDuration, frameContext.agc.frameDuration.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
> > > -			+ context.configuration.sensor.defVBlank;
> > > -	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);
> > > @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >  		return;
> > >  	}
> > >
> > > +	IPACameraSensorInfo &sensorInfo = context.sensorInfo;
> > >  	utils::Duration lineDuration = context.configuration.sensor.lineDuration;
> > >
> > >  	/*
> > > @@ -418,11 +424,19 @@ 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 };
> > >
> > > +	/*
> > > +	 * Limit the allowed shutter speeds for the exposure helper based on
> > > +	 * the frame duration limits.
> > > +	 */
> > > +	utils::Duration minShutterSpeed =
> > > +		std::clamp(frameContext.agc.minFrameDuration,
> > > +			   context.configuration.sensor.minShutterSpeed,
> > > +			   context.configuration.sensor.maxShutterSpeed);
> > >  	utils::Duration maxShutterSpeed =
> > >  		std::clamp(frameContext.agc.maxFrameDuration,
> > >  			   context.configuration.sensor.minShutterSpeed,
> > >  			   context.configuration.sensor.maxShutterSpeed);
> > > -	setLimits(context.configuration.sensor.minShutterSpeed,
> > > +	setLimits(minShutterSpeed,
> > >  		  maxShutterSpeed,
> > >  		  context.configuration.sensor.minAnalogueGain,
> > >  		  context.configuration.sensor.maxAnalogueGain);
> > > @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >  	activeState.agc.automatic.exposure = shutterTime / lineDuration;
> > >  	activeState.agc.automatic.gain = aGain;
> > >
> > > +	/*
> > > +	 * Determine what our FrameDuration must be and adapt VBLANK to suit
> > > +	 * this if we need to expand the shutter time calculated to fill the
> > > +	 * remaining time so that we do not run faster than the minimum frame
> > > +	 * duration (maximum requested frame rate) when we have short exposures.
> > > +	 */
> > > +	utils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,
> > > +						 shutterTime);
> > > +
> > > +	frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;
> > > +
> > > +	/* Update accounting for line length quantization. */
> > > +	frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;
> > > +
> > >  	fillMetadata(context, frameContext, metadata);
> > >  	expMeans_ = {};
> > >  }
> > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > > index 14d0c02a2b32..c5eb0d64ec29 100644
> > > --- a/src/ipa/rkisp1/ipa_context.cpp
> > > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > > @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {
> > >   * \var IPAActiveState::agc.meteringMode
> > >   * \brief Metering mode as set by the AeMeteringMode control
> > >   *
> > > + * \var IPAActiveState::agc.minFrameDuration
> > > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > > + *
> > >   * \var IPAActiveState::agc.maxFrameDuration
> > >   * \brief Maximum frame duration as set by the FrameDurationLimits control
> > >   */
> > > @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {
> > >   * \brief Automatic Gain Control parameters for this frame
> > >   *
> > >   * The exposure and gain are provided by the AGC algorithm, and are to be
> > > - * applied to the sensor in order to take effect for this frame.
> > > + * applied to the sensor in order to take effect for this frame. Additionally
> > > + * the vertical blanking period is determined to maintain a consistent frame
> > > + * rate matched to the FrameDurationLimits as set by the user.
> > >   *
> > >   * \var IPAFrameContext::agc.exposure
> > >   * \brief Exposure time expressed as a number of lines computed by the algorithm
> > > @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {
> > >   *
> > >   * The gain should be adapted to the sensor specific gain code before applying.
> > >   *
> > > + * \var IPAFrameContext::agc.vblank
> > > + * \brief Vertical blanking parameter computed by the algorithm
> > > + *
> > >   * \var IPAFrameContext::agc.autoEnabled
> > >   * \brief Manual/automatic AGC state as set by the AeEnable control
> > >   *
> > > @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {
> > >   * \var IPAFrameContext::agc.meteringMode
> > >   * \brief Metering mode as set by the AeMeteringMode control
> > >   *
> > > + * \var IPAFrameContext::agc.minFrameDuration
> > > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > > + *
> > >   * \var IPAFrameContext::agc.maxFrameDuration
> > >   * \brief Maximum frame duration as set by the FrameDurationLimits control
> > >   *
> > > + * \var IPAFrameContext::agc.frameDuration
> > > + * \brief The actual FrameDuration used by the algorithm for the frame
> > > + *
> > >   * \var IPAFrameContext::agc.updateMetering
> > >   * \brief Indicate if new ISP AGC metering parameters need to be applied
> > >   */
> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > index e274d9b01e1c..60c4d647f1ef 100644
> > > --- a/src/ipa/rkisp1/ipa_context.h
> > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > @@ -79,6 +79,7 @@ struct IPAActiveState {
> > >  		controls::AeConstraintModeEnum constraintMode;
> > >  		controls::AeExposureModeEnum exposureMode;
> > >  		controls::AeMeteringModeEnum meteringMode;
> > > +		utils::Duration minFrameDuration;
> > >  		utils::Duration maxFrameDuration;
> > >  	} agc;
> > >
> > > @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {
> > >  	struct {
> > >  		uint32_t exposure;
> > >  		double gain;
> > > +		uint32_t vblank;
> > >  		bool autoEnabled;
> > >  		controls::AeConstraintModeEnum constraintMode;
> > >  		controls::AeExposureModeEnum exposureMode;
> > >  		controls::AeMeteringModeEnum meteringMode;
> > > +		utils::Duration minFrameDuration;
> > >  		utils::Duration maxFrameDuration;
> > > +		utils::Duration frameDuration;
> > >  		bool updateMetering;
> > >  	} agc;
> > >
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 47777ece783f..17d56fb4e901 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)
> > >  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > >  	uint32_t exposure = frameContext.agc.exposure;
> > >  	uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
> > > +	uint32_t vblank = frameContext.agc.vblank;
> > >
> > >  	ControlList ctrls(sensorControls_);
> > >  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> > >  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> > > +	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
> > >
> > >  	setSensorControls.emit(frame, ctrls);
> > >  }
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 42961c120036..1ec12185bb78 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)
> > >  void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
> > >  					 const ControlList &sensorControls)
> > >  {
> > > +	if (frame == 0) {
> > > +		ControlList controls = sensorControls;
> > > +		sensor_->setControls(&controls);
> > > +		return;
> > > +	}
> > > +
> >
> > I might have missed what is this for :)
>
> iirc I needed this because the vlbank wasn't being applied...
> until the next time the control was set.
>

I see. It's then an issue that is worth fixing but it's not related to
this patch ?

How does this happen ? Do we push the control list to delayed controls
-after- we get the frameStart event notification ? Are controls for
the first frame lost ?


>
> Paul
>
> >
> > >  	delayedCtrls_->push(sensorControls);
> > >  }
> > >
> > > @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > >  	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> > >  		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> > >  		{ V4L2_CID_EXPOSURE, { 2, false } },
> > > +		{ V4L2_CID_VBLANK, { 1, false } },
> > >  	};
> > >
> > >  	data->delayedCtrls_ =
> > > --
> > > 2.34.1
> > >

On Mon, Nov 04, 2024 at 02:35:06PM +0100, Jacopo Mondi wrote:
> Hi Paul
> 
> On Mon, Nov 04, 2024 at 08:25:02PM +0900, Paul Elder wrote:
> > On Thu, Oct 24, 2024 at 05:53:01PM +0200, Jacopo Mondi wrote:
> > > Hi Kieran
> > >
> > > On Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:
> > > > From: Paul Elder <paul.elder@ideasonboard.com>
> > > >
> > > > Add support to rkisp1 for controlling the framerate via the
> > > > FrameDurationLimits control.
> > > >
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > ---
> > > >  src/ipa/rkisp1/algorithms/agc.cpp        | 52 ++++++++++++++++++------
> > > >  src/ipa/rkisp1/ipa_context.cpp           | 16 +++++++-
> > > >  src/ipa/rkisp1/ipa_context.h             |  4 ++
> > > >  src/ipa/rkisp1/rkisp1.cpp                |  2 +
> > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  7 ++++
> > > >  5 files changed, 68 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > index e23ab120b3e2..088ecf42c480 100644
> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > > >
> > > >  	/* Limit the frame duration to match current initialisation */
> > > >  	ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];
> > > > +	context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());
> > > >  	context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());
> > > >
> > > >  	/*
> > > > @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,
> > > >
> > > >  	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> > > >  	if (frameDurationLimits) {
> > > > -		utils::Duration maxFrameDuration =
> > > > -			std::chrono::milliseconds((*frameDurationLimits).back());
> > > > -		agc.maxFrameDuration = maxFrameDuration;
> > > > +		/* Limit the control value to the limits in ControlInfo */
> > > > +		ControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];
> > > > +		int64_t minFrameDuration =
> > > > +			std::clamp((*frameDurationLimits).front(),
> > > > +				   limits.min().get<int64_t>(),
> > > > +				   limits.max().get<int64_t>());
> > > > +		int64_t maxFrameDuration =
> > > > +			std::clamp((*frameDurationLimits).back(),
> > > > +				   limits.min().get<int64_t>(),
> > > > +				   limits.max().get<int64_t>());
> > > > +
> > >
> > > We operate on the assumption the control is well-formed, right ? iow
> > > that frameDurationLimits[0] < frameDurationLimits[1] ?
> >
> > Yeah.
> >
> > >
> > > > +		agc.minFrameDuration = std::chrono::microseconds(minFrameDuration);
> > > > +		agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);
> > > >  	}
> > > > +	frameContext.agc.minFrameDuration = agc.minFrameDuration;
> > > >  	frameContext.agc.maxFrameDuration = agc.maxFrameDuration;
> > > >  }
> > > >
> > > > @@ -330,15 +342,8 @@ 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::FrameDuration, frameContext.agc.frameDuration.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
> > > > -			+ context.configuration.sensor.defVBlank;
> > > > -	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);
> > > > @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > >  		return;
> > > >  	}
> > > >
> > > > +	IPACameraSensorInfo &sensorInfo = context.sensorInfo;
> > > >  	utils::Duration lineDuration = context.configuration.sensor.lineDuration;
> > > >
> > > >  	/*
> > > > @@ -418,11 +424,19 @@ 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 };
> > > >
> > > > +	/*
> > > > +	 * Limit the allowed shutter speeds for the exposure helper based on
> > > > +	 * the frame duration limits.
> > > > +	 */
> > > > +	utils::Duration minShutterSpeed =
> > > > +		std::clamp(frameContext.agc.minFrameDuration,
> > > > +			   context.configuration.sensor.minShutterSpeed,
> > > > +			   context.configuration.sensor.maxShutterSpeed);
> > > >  	utils::Duration maxShutterSpeed =
> > > >  		std::clamp(frameContext.agc.maxFrameDuration,
> > > >  			   context.configuration.sensor.minShutterSpeed,
> > > >  			   context.configuration.sensor.maxShutterSpeed);
> > > > -	setLimits(context.configuration.sensor.minShutterSpeed,
> > > > +	setLimits(minShutterSpeed,
> > > >  		  maxShutterSpeed,
> > > >  		  context.configuration.sensor.minAnalogueGain,
> > > >  		  context.configuration.sensor.maxAnalogueGain);
> > > > @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > >  	activeState.agc.automatic.exposure = shutterTime / lineDuration;
> > > >  	activeState.agc.automatic.gain = aGain;
> > > >
> > > > +	/*
> > > > +	 * Determine what our FrameDuration must be and adapt VBLANK to suit
> > > > +	 * this if we need to expand the shutter time calculated to fill the
> > > > +	 * remaining time so that we do not run faster than the minimum frame
> > > > +	 * duration (maximum requested frame rate) when we have short exposures.
> > > > +	 */
> > > > +	utils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,
> > > > +						 shutterTime);
> > > > +
> > > > +	frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;
> > > > +
> > > > +	/* Update accounting for line length quantization. */
> > > > +	frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;
> > > > +
> > > >  	fillMetadata(context, frameContext, metadata);
> > > >  	expMeans_ = {};
> > > >  }
> > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > > > index 14d0c02a2b32..c5eb0d64ec29 100644
> > > > --- a/src/ipa/rkisp1/ipa_context.cpp
> > > > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > > > @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {
> > > >   * \var IPAActiveState::agc.meteringMode
> > > >   * \brief Metering mode as set by the AeMeteringMode control
> > > >   *
> > > > + * \var IPAActiveState::agc.minFrameDuration
> > > > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > > > + *
> > > >   * \var IPAActiveState::agc.maxFrameDuration
> > > >   * \brief Maximum frame duration as set by the FrameDurationLimits control
> > > >   */
> > > > @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {
> > > >   * \brief Automatic Gain Control parameters for this frame
> > > >   *
> > > >   * The exposure and gain are provided by the AGC algorithm, and are to be
> > > > - * applied to the sensor in order to take effect for this frame.
> > > > + * applied to the sensor in order to take effect for this frame. Additionally
> > > > + * the vertical blanking period is determined to maintain a consistent frame
> > > > + * rate matched to the FrameDurationLimits as set by the user.
> > > >   *
> > > >   * \var IPAFrameContext::agc.exposure
> > > >   * \brief Exposure time expressed as a number of lines computed by the algorithm
> > > > @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {
> > > >   *
> > > >   * The gain should be adapted to the sensor specific gain code before applying.
> > > >   *
> > > > + * \var IPAFrameContext::agc.vblank
> > > > + * \brief Vertical blanking parameter computed by the algorithm
> > > > + *
> > > >   * \var IPAFrameContext::agc.autoEnabled
> > > >   * \brief Manual/automatic AGC state as set by the AeEnable control
> > > >   *
> > > > @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {
> > > >   * \var IPAFrameContext::agc.meteringMode
> > > >   * \brief Metering mode as set by the AeMeteringMode control
> > > >   *
> > > > + * \var IPAFrameContext::agc.minFrameDuration
> > > > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > > > + *
> > > >   * \var IPAFrameContext::agc.maxFrameDuration
> > > >   * \brief Maximum frame duration as set by the FrameDurationLimits control
> > > >   *
> > > > + * \var IPAFrameContext::agc.frameDuration
> > > > + * \brief The actual FrameDuration used by the algorithm for the frame
> > > > + *
> > > >   * \var IPAFrameContext::agc.updateMetering
> > > >   * \brief Indicate if new ISP AGC metering parameters need to be applied
> > > >   */
> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > index e274d9b01e1c..60c4d647f1ef 100644
> > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > @@ -79,6 +79,7 @@ struct IPAActiveState {
> > > >  		controls::AeConstraintModeEnum constraintMode;
> > > >  		controls::AeExposureModeEnum exposureMode;
> > > >  		controls::AeMeteringModeEnum meteringMode;
> > > > +		utils::Duration minFrameDuration;
> > > >  		utils::Duration maxFrameDuration;
> > > >  	} agc;
> > > >
> > > > @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {
> > > >  	struct {
> > > >  		uint32_t exposure;
> > > >  		double gain;
> > > > +		uint32_t vblank;
> > > >  		bool autoEnabled;
> > > >  		controls::AeConstraintModeEnum constraintMode;
> > > >  		controls::AeExposureModeEnum exposureMode;
> > > >  		controls::AeMeteringModeEnum meteringMode;
> > > > +		utils::Duration minFrameDuration;
> > > >  		utils::Duration maxFrameDuration;
> > > > +		utils::Duration frameDuration;
> > > >  		bool updateMetering;
> > > >  	} agc;
> > > >
> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > index 47777ece783f..17d56fb4e901 100644
> > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)
> > > >  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > >  	uint32_t exposure = frameContext.agc.exposure;
> > > >  	uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
> > > > +	uint32_t vblank = frameContext.agc.vblank;
> > > >
> > > >  	ControlList ctrls(sensorControls_);
> > > >  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> > > >  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> > > > +	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
> > > >
> > > >  	setSensorControls.emit(frame, ctrls);
> > > >  }
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index 42961c120036..1ec12185bb78 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)
> > > >  void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
> > > >  					 const ControlList &sensorControls)
> > > >  {
> > > > +	if (frame == 0) {
> > > > +		ControlList controls = sensorControls;
> > > > +		sensor_->setControls(&controls);
> > > > +		return;
> > > > +	}
> > > > +
> > >
> > > I might have missed what is this for :)
> >
> > iirc I needed this because the vlbank wasn't being applied...
> > until the next time the control was set.
> >
> 
> I see. It's then an issue that is worth fixing but it's not related to
> this patch ?
> 
> How does this happen ? Do we push the control list to delayed controls
> -after- we get the frameStart event notification ? Are controls for
> the first frame lost ?

I'm pretty sure it's that the vblank isn't ever set until process(), so
until a stats buffer comes in. Before that, at start() time, it's just
set to the initial default value... which is zero and therefore out of
bounds and does nothing.


Paul

> >
> > >
> > > >  	delayedCtrls_->push(sensorControls);
> > > >  }
> > > >
> > > > @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > > >  	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> > > >  		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> > > >  		{ V4L2_CID_EXPOSURE, { 2, false } },
> > > > +		{ V4L2_CID_VBLANK, { 1, false } },
> > > >  	};
> > > >
> > > >  	data->delayedCtrls_ =
> > > > --
> > > > 2.34.1
> > > >

Hi Paul

On Mon, Nov 18, 2024 at 11:51:21PM +0900, Paul Elder wrote:
> On Mon, Nov 04, 2024 at 02:35:06PM +0100, Jacopo Mondi wrote:
> > Hi Paul
> >
> > On Mon, Nov 04, 2024 at 08:25:02PM +0900, Paul Elder wrote:
> > > On Thu, Oct 24, 2024 at 05:53:01PM +0200, Jacopo Mondi wrote:
> > > > Hi Kieran
> > > >
> > > > On Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:
> > > > > From: Paul Elder <paul.elder@ideasonboard.com>
> > > > >
> > > > > Add support to rkisp1 for controlling the framerate via the
> > > > > FrameDurationLimits control.
> > > > >
> > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > ---
> > > > >  src/ipa/rkisp1/algorithms/agc.cpp        | 52 ++++++++++++++++++------
> > > > >  src/ipa/rkisp1/ipa_context.cpp           | 16 +++++++-
> > > > >  src/ipa/rkisp1/ipa_context.h             |  4 ++
> > > > >  src/ipa/rkisp1/rkisp1.cpp                |  2 +
> > > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  7 ++++
> > > > >  5 files changed, 68 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > > index e23ab120b3e2..088ecf42c480 100644
> > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > > @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > > > >
> > > > >  	/* Limit the frame duration to match current initialisation */
> > > > >  	ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];
> > > > > +	context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());
> > > > >  	context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());
> > > > >
> > > > >  	/*
> > > > > @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,
> > > > >
> > > > >  	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> > > > >  	if (frameDurationLimits) {
> > > > > -		utils::Duration maxFrameDuration =
> > > > > -			std::chrono::milliseconds((*frameDurationLimits).back());
> > > > > -		agc.maxFrameDuration = maxFrameDuration;
> > > > > +		/* Limit the control value to the limits in ControlInfo */
> > > > > +		ControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];
> > > > > +		int64_t minFrameDuration =
> > > > > +			std::clamp((*frameDurationLimits).front(),
> > > > > +				   limits.min().get<int64_t>(),
> > > > > +				   limits.max().get<int64_t>());
> > > > > +		int64_t maxFrameDuration =
> > > > > +			std::clamp((*frameDurationLimits).back(),
> > > > > +				   limits.min().get<int64_t>(),
> > > > > +				   limits.max().get<int64_t>());
> > > > > +
> > > >
> > > > We operate on the assumption the control is well-formed, right ? iow
> > > > that frameDurationLimits[0] < frameDurationLimits[1] ?
> > >
> > > Yeah.
> > >
> > > >
> > > > > +		agc.minFrameDuration = std::chrono::microseconds(minFrameDuration);
> > > > > +		agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);
> > > > >  	}
> > > > > +	frameContext.agc.minFrameDuration = agc.minFrameDuration;
> > > > >  	frameContext.agc.maxFrameDuration = agc.maxFrameDuration;
> > > > >  }
> > > > >
> > > > > @@ -330,15 +342,8 @@ 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::FrameDuration, frameContext.agc.frameDuration.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
> > > > > -			+ context.configuration.sensor.defVBlank;
> > > > > -	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);
> > > > > @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > > >  		return;
> > > > >  	}
> > > > >
> > > > > +	IPACameraSensorInfo &sensorInfo = context.sensorInfo;
> > > > >  	utils::Duration lineDuration = context.configuration.sensor.lineDuration;
> > > > >
> > > > >  	/*
> > > > > @@ -418,11 +424,19 @@ 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 };
> > > > >
> > > > > +	/*
> > > > > +	 * Limit the allowed shutter speeds for the exposure helper based on
> > > > > +	 * the frame duration limits.
> > > > > +	 */
> > > > > +	utils::Duration minShutterSpeed =
> > > > > +		std::clamp(frameContext.agc.minFrameDuration,
> > > > > +			   context.configuration.sensor.minShutterSpeed,
> > > > > +			   context.configuration.sensor.maxShutterSpeed);
> > > > >  	utils::Duration maxShutterSpeed =
> > > > >  		std::clamp(frameContext.agc.maxFrameDuration,
> > > > >  			   context.configuration.sensor.minShutterSpeed,
> > > > >  			   context.configuration.sensor.maxShutterSpeed);
> > > > > -	setLimits(context.configuration.sensor.minShutterSpeed,
> > > > > +	setLimits(minShutterSpeed,
> > > > >  		  maxShutterSpeed,
> > > > >  		  context.configuration.sensor.minAnalogueGain,
> > > > >  		  context.configuration.sensor.maxAnalogueGain);
> > > > > @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > > >  	activeState.agc.automatic.exposure = shutterTime / lineDuration;
> > > > >  	activeState.agc.automatic.gain = aGain;
> > > > >
> > > > > +	/*
> > > > > +	 * Determine what our FrameDuration must be and adapt VBLANK to suit
> > > > > +	 * this if we need to expand the shutter time calculated to fill the
> > > > > +	 * remaining time so that we do not run faster than the minimum frame
> > > > > +	 * duration (maximum requested frame rate) when we have short exposures.
> > > > > +	 */
> > > > > +	utils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,
> > > > > +						 shutterTime);
> > > > > +
> > > > > +	frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;
> > > > > +
> > > > > +	/* Update accounting for line length quantization. */
> > > > > +	frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;
> > > > > +
> > > > >  	fillMetadata(context, frameContext, metadata);
> > > > >  	expMeans_ = {};
> > > > >  }
> > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > > > > index 14d0c02a2b32..c5eb0d64ec29 100644
> > > > > --- a/src/ipa/rkisp1/ipa_context.cpp
> > > > > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > > > > @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {
> > > > >   * \var IPAActiveState::agc.meteringMode
> > > > >   * \brief Metering mode as set by the AeMeteringMode control
> > > > >   *
> > > > > + * \var IPAActiveState::agc.minFrameDuration
> > > > > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > > > > + *
> > > > >   * \var IPAActiveState::agc.maxFrameDuration
> > > > >   * \brief Maximum frame duration as set by the FrameDurationLimits control
> > > > >   */
> > > > > @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {
> > > > >   * \brief Automatic Gain Control parameters for this frame
> > > > >   *
> > > > >   * The exposure and gain are provided by the AGC algorithm, and are to be
> > > > > - * applied to the sensor in order to take effect for this frame.
> > > > > + * applied to the sensor in order to take effect for this frame. Additionally
> > > > > + * the vertical blanking period is determined to maintain a consistent frame
> > > > > + * rate matched to the FrameDurationLimits as set by the user.
> > > > >   *
> > > > >   * \var IPAFrameContext::agc.exposure
> > > > >   * \brief Exposure time expressed as a number of lines computed by the algorithm
> > > > > @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {
> > > > >   *
> > > > >   * The gain should be adapted to the sensor specific gain code before applying.
> > > > >   *
> > > > > + * \var IPAFrameContext::agc.vblank
> > > > > + * \brief Vertical blanking parameter computed by the algorithm
> > > > > + *
> > > > >   * \var IPAFrameContext::agc.autoEnabled
> > > > >   * \brief Manual/automatic AGC state as set by the AeEnable control
> > > > >   *
> > > > > @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {
> > > > >   * \var IPAFrameContext::agc.meteringMode
> > > > >   * \brief Metering mode as set by the AeMeteringMode control
> > > > >   *
> > > > > + * \var IPAFrameContext::agc.minFrameDuration
> > > > > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > > > > + *
> > > > >   * \var IPAFrameContext::agc.maxFrameDuration
> > > > >   * \brief Maximum frame duration as set by the FrameDurationLimits control
> > > > >   *
> > > > > + * \var IPAFrameContext::agc.frameDuration
> > > > > + * \brief The actual FrameDuration used by the algorithm for the frame
> > > > > + *
> > > > >   * \var IPAFrameContext::agc.updateMetering
> > > > >   * \brief Indicate if new ISP AGC metering parameters need to be applied
> > > > >   */
> > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > > index e274d9b01e1c..60c4d647f1ef 100644
> > > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > > @@ -79,6 +79,7 @@ struct IPAActiveState {
> > > > >  		controls::AeConstraintModeEnum constraintMode;
> > > > >  		controls::AeExposureModeEnum exposureMode;
> > > > >  		controls::AeMeteringModeEnum meteringMode;
> > > > > +		utils::Duration minFrameDuration;
> > > > >  		utils::Duration maxFrameDuration;
> > > > >  	} agc;
> > > > >
> > > > > @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {
> > > > >  	struct {
> > > > >  		uint32_t exposure;
> > > > >  		double gain;
> > > > > +		uint32_t vblank;
> > > > >  		bool autoEnabled;
> > > > >  		controls::AeConstraintModeEnum constraintMode;
> > > > >  		controls::AeExposureModeEnum exposureMode;
> > > > >  		controls::AeMeteringModeEnum meteringMode;
> > > > > +		utils::Duration minFrameDuration;
> > > > >  		utils::Duration maxFrameDuration;
> > > > > +		utils::Duration frameDuration;
> > > > >  		bool updateMetering;
> > > > >  	} agc;
> > > > >
> > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > index 47777ece783f..17d56fb4e901 100644
> > > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)
> > > > >  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > > >  	uint32_t exposure = frameContext.agc.exposure;
> > > > >  	uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
> > > > > +	uint32_t vblank = frameContext.agc.vblank;
> > > > >
> > > > >  	ControlList ctrls(sensorControls_);
> > > > >  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> > > > >  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> > > > > +	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
> > > > >
> > > > >  	setSensorControls.emit(frame, ctrls);
> > > > >  }
> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > index 42961c120036..1ec12185bb78 100644
> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)
> > > > >  void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
> > > > >  					 const ControlList &sensorControls)
> > > > >  {
> > > > > +	if (frame == 0) {
> > > > > +		ControlList controls = sensorControls;
> > > > > +		sensor_->setControls(&controls);
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > >
> > > > I might have missed what is this for :)
> > >
> > > iirc I needed this because the vlbank wasn't being applied...
> > > until the next time the control was set.
> > >
> >
> > I see. It's then an issue that is worth fixing but it's not related to
> > this patch ?
> >
> > How does this happen ? Do we push the control list to delayed controls
> > -after- we get the frameStart event notification ? Are controls for
> > the first frame lost ?
>
> I'm pretty sure it's that the vblank isn't ever set until process(), so

mmm, maybe I'm confused.

IPARkISP1::start() calls setControls(0) which emits setSensorControls.

So we get here just after start() from

void IPARkISP1::setControls(unsigned int frame)
{
	uint32_t vblank = frameContext.agc.vblank;
	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
	setSensorControls.emit(frame, ctrls);
}

So either
1) frameContext.agc.vblank is not initialized to the right value
   - what's "right" ? The value set at Camera::start ? This is
     currently ignored by

        int PipelineHandlerRkISP1::start(Camera *camera,
                                         [[maybe_unused]] const ControlList *controls)

2) vblank has the right value, but DelayedControls has already
   received the frame start event for frame#0 so the control is lost ?

Could you help clarify this, and if this change is related to this
series or it can be fixed separately ?

Thanks

> until a stats buffer comes in. Before that, at start() time, it's just
> set to the initial default value... which is zero and therefore out of
> bounds and does nothing.
>
>
> Paul
>
> > >
> > > >
> > > > >  	delayedCtrls_->push(sensorControls);
> > > > >  }
> > > > >
> > > > > @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > > > >  	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> > > > >  		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> > > > >  		{ V4L2_CID_EXPOSURE, { 2, false } },
> > > > > +		{ V4L2_CID_VBLANK, { 1, false } },
> > > > >  	};
> > > > >
> > > > >  	data->delayedCtrls_ =
> > > > > --
> > > > > 2.34.1
> > > > >

On Mon, Nov 18, 2024 at 04:30:23PM +0100, Jacopo Mondi wrote:
> Hi Paul
> 
> On Mon, Nov 18, 2024 at 11:51:21PM +0900, Paul Elder wrote:
> > On Mon, Nov 04, 2024 at 02:35:06PM +0100, Jacopo Mondi wrote:
> > > Hi Paul
> > >
> > > On Mon, Nov 04, 2024 at 08:25:02PM +0900, Paul Elder wrote:
> > > > On Thu, Oct 24, 2024 at 05:53:01PM +0200, Jacopo Mondi wrote:
> > > > > Hi Kieran
> > > > >
> > > > > On Mon, Oct 14, 2024 at 04:47:47PM +0100, Kieran Bingham wrote:
> > > > > > From: Paul Elder <paul.elder@ideasonboard.com>
> > > > > >
> > > > > > Add support to rkisp1 for controlling the framerate via the
> > > > > > FrameDurationLimits control.
> > > > > >
> > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > > ---
> > > > > >  src/ipa/rkisp1/algorithms/agc.cpp        | 52 ++++++++++++++++++------
> > > > > >  src/ipa/rkisp1/ipa_context.cpp           | 16 +++++++-
> > > > > >  src/ipa/rkisp1/ipa_context.h             |  4 ++
> > > > > >  src/ipa/rkisp1/rkisp1.cpp                |  2 +
> > > > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  7 ++++
> > > > > >  5 files changed, 68 insertions(+), 13 deletions(-)
> > > > > >
> > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > > > index e23ab120b3e2..088ecf42c480 100644
> > > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > > > @@ -180,6 +180,7 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo)
> > > > > >
> > > > > >  	/* Limit the frame duration to match current initialisation */
> > > > > >  	ControlInfo &frameDurationLimits = context.ctrlMap[&controls::FrameDurationLimits];
> > > > > > +	context.activeState.agc.minFrameDuration = std::chrono::microseconds(frameDurationLimits.min().get<int64_t>());
> > > > > >  	context.activeState.agc.maxFrameDuration = std::chrono::microseconds(frameDurationLimits.max().get<int64_t>());
> > > > > >
> > > > > >  	/*
> > > > > > @@ -267,10 +268,21 @@ void Agc::queueRequest(IPAContext &context,
> > > > > >
> > > > > >  	const auto &frameDurationLimits = controls.get(controls::FrameDurationLimits);
> > > > > >  	if (frameDurationLimits) {
> > > > > > -		utils::Duration maxFrameDuration =
> > > > > > -			std::chrono::milliseconds((*frameDurationLimits).back());
> > > > > > -		agc.maxFrameDuration = maxFrameDuration;
> > > > > > +		/* Limit the control value to the limits in ControlInfo */
> > > > > > +		ControlInfo &limits = context.ctrlMap[&controls::FrameDurationLimits];
> > > > > > +		int64_t minFrameDuration =
> > > > > > +			std::clamp((*frameDurationLimits).front(),
> > > > > > +				   limits.min().get<int64_t>(),
> > > > > > +				   limits.max().get<int64_t>());
> > > > > > +		int64_t maxFrameDuration =
> > > > > > +			std::clamp((*frameDurationLimits).back(),
> > > > > > +				   limits.min().get<int64_t>(),
> > > > > > +				   limits.max().get<int64_t>());
> > > > > > +
> > > > >
> > > > > We operate on the assumption the control is well-formed, right ? iow
> > > > > that frameDurationLimits[0] < frameDurationLimits[1] ?
> > > >
> > > > Yeah.
> > > >
> > > > >
> > > > > > +		agc.minFrameDuration = std::chrono::microseconds(minFrameDuration);
> > > > > > +		agc.maxFrameDuration = std::chrono::microseconds(maxFrameDuration);
> > > > > >  	}
> > > > > > +	frameContext.agc.minFrameDuration = agc.minFrameDuration;
> > > > > >  	frameContext.agc.maxFrameDuration = agc.maxFrameDuration;
> > > > > >  }
> > > > > >
> > > > > > @@ -330,15 +342,8 @@ 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::FrameDuration, frameContext.agc.frameDuration.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
> > > > > > -			+ context.configuration.sensor.defVBlank;
> > > > > > -	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);
> > > > > > @@ -400,6 +405,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > > > >  		return;
> > > > > >  	}
> > > > > >
> > > > > > +	IPACameraSensorInfo &sensorInfo = context.sensorInfo;
> > > > > >  	utils::Duration lineDuration = context.configuration.sensor.lineDuration;
> > > > > >
> > > > > >  	/*
> > > > > > @@ -418,11 +424,19 @@ 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 };
> > > > > >
> > > > > > +	/*
> > > > > > +	 * Limit the allowed shutter speeds for the exposure helper based on
> > > > > > +	 * the frame duration limits.
> > > > > > +	 */
> > > > > > +	utils::Duration minShutterSpeed =
> > > > > > +		std::clamp(frameContext.agc.minFrameDuration,
> > > > > > +			   context.configuration.sensor.minShutterSpeed,
> > > > > > +			   context.configuration.sensor.maxShutterSpeed);
> > > > > >  	utils::Duration maxShutterSpeed =
> > > > > >  		std::clamp(frameContext.agc.maxFrameDuration,
> > > > > >  			   context.configuration.sensor.minShutterSpeed,
> > > > > >  			   context.configuration.sensor.maxShutterSpeed);
> > > > > > -	setLimits(context.configuration.sensor.minShutterSpeed,
> > > > > > +	setLimits(minShutterSpeed,
> > > > > >  		  maxShutterSpeed,
> > > > > >  		  context.configuration.sensor.minAnalogueGain,
> > > > > >  		  context.configuration.sensor.maxAnalogueGain);
> > > > > > @@ -451,6 +465,20 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > > > >  	activeState.agc.automatic.exposure = shutterTime / lineDuration;
> > > > > >  	activeState.agc.automatic.gain = aGain;
> > > > > >
> > > > > > +	/*
> > > > > > +	 * Determine what our FrameDuration must be and adapt VBLANK to suit
> > > > > > +	 * this if we need to expand the shutter time calculated to fill the
> > > > > > +	 * remaining time so that we do not run faster than the minimum frame
> > > > > > +	 * duration (maximum requested frame rate) when we have short exposures.
> > > > > > +	 */
> > > > > > +	utils::Duration frameDuration = std::max(frameContext.agc.minFrameDuration,
> > > > > > +						 shutterTime);
> > > > > > +
> > > > > > +	frameContext.agc.vblank = (frameDuration / lineDuration) - sensorInfo.outputSize.height;
> > > > > > +
> > > > > > +	/* Update accounting for line length quantization. */
> > > > > > +	frameContext.agc.frameDuration = (sensorInfo.outputSize.height + frameContext.agc.vblank) * lineDuration;
> > > > > > +
> > > > > >  	fillMetadata(context, frameContext, metadata);
> > > > > >  	expMeans_ = {};
> > > > > >  }
> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> > > > > > index 14d0c02a2b32..c5eb0d64ec29 100644
> > > > > > --- a/src/ipa/rkisp1/ipa_context.cpp
> > > > > > +++ b/src/ipa/rkisp1/ipa_context.cpp
> > > > > > @@ -177,6 +177,9 @@ namespace libcamera::ipa::rkisp1 {
> > > > > >   * \var IPAActiveState::agc.meteringMode
> > > > > >   * \brief Metering mode as set by the AeMeteringMode control
> > > > > >   *
> > > > > > + * \var IPAActiveState::agc.minFrameDuration
> > > > > > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > > > > > + *
> > > > > >   * \var IPAActiveState::agc.maxFrameDuration
> > > > > >   * \brief Maximum frame duration as set by the FrameDurationLimits control
> > > > > >   */
> > > > > > @@ -297,7 +300,9 @@ namespace libcamera::ipa::rkisp1 {
> > > > > >   * \brief Automatic Gain Control parameters for this frame
> > > > > >   *
> > > > > >   * The exposure and gain are provided by the AGC algorithm, and are to be
> > > > > > - * applied to the sensor in order to take effect for this frame.
> > > > > > + * applied to the sensor in order to take effect for this frame. Additionally
> > > > > > + * the vertical blanking period is determined to maintain a consistent frame
> > > > > > + * rate matched to the FrameDurationLimits as set by the user.
> > > > > >   *
> > > > > >   * \var IPAFrameContext::agc.exposure
> > > > > >   * \brief Exposure time expressed as a number of lines computed by the algorithm
> > > > > > @@ -307,6 +312,9 @@ namespace libcamera::ipa::rkisp1 {
> > > > > >   *
> > > > > >   * The gain should be adapted to the sensor specific gain code before applying.
> > > > > >   *
> > > > > > + * \var IPAFrameContext::agc.vblank
> > > > > > + * \brief Vertical blanking parameter computed by the algorithm
> > > > > > + *
> > > > > >   * \var IPAFrameContext::agc.autoEnabled
> > > > > >   * \brief Manual/automatic AGC state as set by the AeEnable control
> > > > > >   *
> > > > > > @@ -319,9 +327,15 @@ namespace libcamera::ipa::rkisp1 {
> > > > > >   * \var IPAFrameContext::agc.meteringMode
> > > > > >   * \brief Metering mode as set by the AeMeteringMode control
> > > > > >   *
> > > > > > + * \var IPAFrameContext::agc.minFrameDuration
> > > > > > + * \brief Minimum frame duration as set by the FrameDurationLimits control
> > > > > > + *
> > > > > >   * \var IPAFrameContext::agc.maxFrameDuration
> > > > > >   * \brief Maximum frame duration as set by the FrameDurationLimits control
> > > > > >   *
> > > > > > + * \var IPAFrameContext::agc.frameDuration
> > > > > > + * \brief The actual FrameDuration used by the algorithm for the frame
> > > > > > + *
> > > > > >   * \var IPAFrameContext::agc.updateMetering
> > > > > >   * \brief Indicate if new ISP AGC metering parameters need to be applied
> > > > > >   */
> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > > > index e274d9b01e1c..60c4d647f1ef 100644
> > > > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > > > @@ -79,6 +79,7 @@ struct IPAActiveState {
> > > > > >  		controls::AeConstraintModeEnum constraintMode;
> > > > > >  		controls::AeExposureModeEnum exposureMode;
> > > > > >  		controls::AeMeteringModeEnum meteringMode;
> > > > > > +		utils::Duration minFrameDuration;
> > > > > >  		utils::Duration maxFrameDuration;
> > > > > >  	} agc;
> > > > > >
> > > > > > @@ -128,11 +129,14 @@ struct IPAFrameContext : public FrameContext {
> > > > > >  	struct {
> > > > > >  		uint32_t exposure;
> > > > > >  		double gain;
> > > > > > +		uint32_t vblank;
> > > > > >  		bool autoEnabled;
> > > > > >  		controls::AeConstraintModeEnum constraintMode;
> > > > > >  		controls::AeExposureModeEnum exposureMode;
> > > > > >  		controls::AeMeteringModeEnum meteringMode;
> > > > > > +		utils::Duration minFrameDuration;
> > > > > >  		utils::Duration maxFrameDuration;
> > > > > > +		utils::Duration frameDuration;
> > > > > >  		bool updateMetering;
> > > > > >  	} agc;
> > > > > >
> > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > index 47777ece783f..17d56fb4e901 100644
> > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > @@ -449,10 +449,12 @@ void IPARkISP1::setControls(unsigned int frame)
> > > > > >  	IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> > > > > >  	uint32_t exposure = frameContext.agc.exposure;
> > > > > >  	uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain);
> > > > > > +	uint32_t vblank = frameContext.agc.vblank;
> > > > > >
> > > > > >  	ControlList ctrls(sensorControls_);
> > > > > >  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> > > > > >  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> > > > > > +	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
> > > > > >
> > > > > >  	setSensorControls.emit(frame, ctrls);
> > > > > >  }
> > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > index 42961c120036..1ec12185bb78 100644
> > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > @@ -394,6 +394,12 @@ void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)
> > > > > >  void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
> > > > > >  					 const ControlList &sensorControls)
> > > > > >  {
> > > > > > +	if (frame == 0) {
> > > > > > +		ControlList controls = sensorControls;
> > > > > > +		sensor_->setControls(&controls);
> > > > > > +		return;
> > > > > > +	}
> > > > > > +
> > > > >
> > > > > I might have missed what is this for :)
> > > >
> > > > iirc I needed this because the vlbank wasn't being applied...
> > > > until the next time the control was set.
> > > >
> > >
> > > I see. It's then an issue that is worth fixing but it's not related to
> > > this patch ?
> > >
> > > How does this happen ? Do we push the control list to delayed controls
> > > -after- we get the frameStart event notification ? Are controls for
> > > the first frame lost ?
> >
> > I'm pretty sure it's that the vblank isn't ever set until process(), so
> 
> mmm, maybe I'm confused.
> 
> IPARkISP1::start() calls setControls(0) which emits setSensorControls.
> 
> So we get here just after start() from
> 
> void IPARkISP1::setControls(unsigned int frame)
> {
> 	uint32_t vblank = frameContext.agc.vblank;
> 	ctrls.set(V4L2_CID_VBLANK, static_cast<int32_t>(vblank));
> 	setSensorControls.emit(frame, ctrls);
> }
> 
> So either
> 1) frameContext.agc.vblank is not initialized to the right value

Yes, this is the case atm.

>    - what's "right" ? The value set at Camera::start ? This is
>      currently ignored by

Actually, it should probably be initialized at configure() time, when the
FrameDurationLimits ControlInfo limits are computed, and defaulted to
that value. That way setControls() should work as expected.

> 
>         int PipelineHandlerRkISP1::start(Camera *camera,
>                                          [[maybe_unused]] const ControlList *controls)

And then in addition, these should be forwarded in I think.

> 
> 2) vblank has the right value, but DelayedControls has already
>    received the frame start event for frame#0 so the control is lost ?
> 
> Could you help clarify this, and if this change is related to this
> series or it can be fixed separately ?

It should indeed probably be fixed in this patch, because as you pointed
out that hunk isn't very nice/correct.


Paul

> 
> Thanks
> 
> > until a stats buffer comes in. Before that, at start() time, it's just
> > set to the initial default value... which is zero and therefore out of
> > bounds and does nothing.
> >
> >
> > Paul
> >
> > > >
> > > > >
> > > > > >  	delayedCtrls_->push(sensorControls);
> > > > > >  }
> > > > > >
> > > > > > @@ -1138,6 +1144,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > > > > >  	std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> > > > > >  		{ V4L2_CID_ANALOGUE_GAIN, { 1, false } },
> > > > > >  		{ V4L2_CID_EXPOSURE, { 2, false } },
> > > > > > +		{ V4L2_CID_VBLANK, { 1, false } },
> > > > > >  	};
> > > > > >
> > > > > >  	data->delayedCtrls_ =
> > > > > > --
> > > > > > 2.34.1
> > > > > >