[libcamera-devel] [PATCH 16/16] ipa: ipu3: Cap frame duration to 30 FPS

[Umang Jain]

Hi Jacopo,

I have a few questions regarding the todos

On 8/27/21 5:37 PM, Jacopo Mondi wrote:
> Limit the IPU3 frame rate to 30 FPS.
>
> The reason to do is to bring the IPU3 IPA in par with the Intel
> HAL implementation on IPU3 platform, where 30FPS is the frame rate used
> to perform quality tuning in the closed-source IPA module and has been
> validated as the most efficient rate for the power/performace budget.
>
> Compute the vertical blanking to maintain such frame rate and configure
> the sensor with that.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>   src/ipa/ipu3/ipu3.cpp | 37 ++++++++++++++++++++++++++++++++-----
>   1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index fc5f69ed5ddc..0e5d5e479e20 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -183,7 +183,7 @@ private:
>   	IPACameraSensorInfo sensorInfo_;
>   
>   	/* Camera sensor controls. */
> -	uint32_t defVBlank_;
> +	uint32_t vBlank_;
>   	uint32_t exposure_;
>   	uint32_t minExposure_;
>   	uint32_t maxExposure_;
> @@ -257,10 +257,39 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
>   		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U);
>   	}
>   
> +	/*
> +	 * Cap minimum frame duration to 30FPS.
> +	 *
> +	 * 30 FPS has been validated in the closed source Intel 3A module as the
> +	 * most opportune frame rate for quality tuning, and power
> +	 * vs performances budget on Intel IPU3.
> +	 *
> +	 * Reduce the minimum achievable frame rate to 30 FPS and compute the
> +	 * vertical blanking to maintain that rate.
> +	 */
> +	int64_t *minFrameDuration = &frameDurations[0];
> +	if (*minFrameDuration < 1e6 / 30.0)
> +		*minFrameDuration = 1e6 / 30.0;
> +
>   	controls[&controls::FrameDurationLimits] = ControlInfo(frameDurations[0],
>   							       frameDurations[1],
>   							       frameDurations[2]);
>   
> +	/*
> +	 * Adjust the vertical blanking to obtain the desired frame duration.
> +	 *
> +	 * Assume a fixed line length as horizontal blanking is seldom
> +	 * controllable.
> +	 *
> +	 * \todo Support making this overridable by the application through
> +	 * controls::FrameDuration.


In general, are applications poised to alter frame duration ? I think 
applications are poised to request manual exposure control, correct me 
if I am wrong! Hence, I don't understand the concept of overridable 
controls::FrameDuration as such.

> +	 *
> +	 * \todo Clamp exposure to frame duration.


If a manual exposure is requested, will the clamping be in effect? In my 
head, the broader concept is - manual exposure -> set exposure (which 
requires new vblank), calculate new vblank -> set new vblank (which is 
to (-) \todo  of in patch, see below).

> +	 */
> +	vBlank_ = *minFrameDuration * (sensorInfo.pixelRate / 1000000U);
> +	vBlank_ /= lineLength;
> +	vBlank_ -= sensorInfo.outputSize.height;
> +
>   	*ipaControls = ControlInfoMap(std::move(controls), controls::controls);
>   }
>   
> @@ -399,8 +428,6 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>   	maxGain_ = itGain->second.max().get<int32_t>();
>   	gain_ = minGain_;
>   
> -	defVBlank_ = itVBlank->second.def().get<int32_t>();
> -
>   	/* Clean context at configuration */
>   	context_ = {};
>   
> @@ -511,8 +538,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>   
>   	setControls(frame);
>   
> -	/* \todo Use VBlank value calculated from each frame exposure. */


I think what I am trying to ask is, this patch deletes one \todo, and 
introduces two other \todos. The descriptions  of new \todos are 
confusing to me or maybe they need some general discussion.

This previous todo, seems to comply with Intel HAL i.e. The Vblank is 
set from exposure's results. (See SyncManager::applySensorParams and  
SensorHwOp::setFrameDuration). These use exposure's results derived from 
IPA.


> -	int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /
> +	int64_t frameDuration = sensorInfo_.lineLength * (vBlank_ + sensorInfo_.outputSize.height) /
>   				(sensorInfo_.pixelRate / 1e6);
>   	ctrls.set(controls::FrameDuration, frameDuration);
>   
> @@ -534,6 +560,7 @@ void IPAIPU3::setControls(unsigned int frame)
>   	ControlList ctrls(ctrls_);
>   	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_));
>   	op.controls = ctrls;
>   
>   	queueFrameAction.emit(frame, op);