[libcamera-devel] [PATCH v2 2/6] libcamera: ipu3: Register FrameDurations control

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jan 26, 2021 at 06:30:04PM +0100, Jacopo Mondi wrote:
> Register the FrameDurations control in the IPU3 pipeline handler
> computed using the vertical blanking limits and the current
> configuration pixel rate as parameters.
> 
> The FrameDurations control limits should be updated everytime a new
> configuration is applied to the sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 35 +++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index db0d6b91be70..fe5694f9893a 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -754,6 +754,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  		return ret;
>  
>  	ControlInfoMap::Map controls = IPU3Controls;
> +	const ControlInfoMap &sensorControls = sensor->controls();
>  
>  	/*
>  	 * Compute exposure time limits.
> @@ -766,7 +767,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  	 */
>  	double lineDuration = sensorInfo.lineLength
>  			    / (sensorInfo.pixelRate / 1e6);
> -	const ControlInfoMap &sensorControls = sensor->controls();
>  	const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second;
>  	int32_t minExposure = v4l2Exposure.min().get<int32_t>() * lineDuration;
>  	int32_t maxExposure = v4l2Exposure.max().get<int32_t>() * lineDuration;
> @@ -781,6 +781,39 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  	controls[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,
>  							defExposure);
>  
> +	/*
> +	 * Compute the frame duration limits.
> +	 *
> +	 * \todo The frame duration limits depend on the sensor configuration.
> +	 * Initialize the control using the frame sizes and pixel rate of the
> +	 * current configuration.

This is the perfect way to make sure it will break in ways that are
horrible to debug, as the value we report will depend on the
configuration applied to the subdev the previous time the camera was
used. Could we use a fixed size instead, maybe sensor->resolution() ?
We'll still need to make the limits dynamic (and figure out how to
handle this on the Android side), so a todo comment will still be
needed, but at least the behaviour will be less random.

> +	 *
> +	 * The frame length is computed assuming a fixed line length combined
> +	 * with the vertical frame sizes.
> +	 */
> +	const ControlInfo &v4l2HBlank = sensorControls.find(V4L2_CID_HBLANK)->second;
> +	uint32_t hblank = v4l2HBlank.def().get<int32_t>();
> +	uint32_t lineLength = sensorInfo.outputSize.width + hblank;
> +
> +	const ControlInfo &v4l2VBlank = sensorControls.find(V4L2_CID_VBLANK)->second;
> +	std::array<uint32_t, 3> frameHeights{
> +		(v4l2VBlank.min().get<int32_t>() + sensorInfo.outputSize.height),
> +		(v4l2VBlank.max().get<int32_t>() + sensorInfo.outputSize.height),
> +		(v4l2VBlank.def().get<int32_t>() + sensorInfo.outputSize.height),

You can drop the outer parentheses.

> +	};
> +
> +	std::vector<int64_t> frameDurations;
> +	frameDurations.reserve(3);
> +	std::transform(frameHeights.begin(), frameHeights.end(),
> +		       std::back_inserter(frameDurations),
> +		       [&](int32_t frameHeight) {
> +			       int64_t frameSize = lineLength * frameHeight;

This can be unsigned.

> +			       return frameSize / (sensorInfo.pixelRate / 1000000U);
> +		       });

Maybe the following would be more readable ?

	std::array<int64_t, 3> frameDurations;
	for (unsigned int i = 0; i < frameHeights.size(); ++i) {
		uint64_t frameSize = lineLength * frameHeights[i];
		frameDurations[i] = frameSize / (sensorInfo.pixelRate / 1000000U)
	}

> +	controls[&controls::FrameDurations] = ControlInfo(frameDurations[0],
> +							  frameDurations[1],
> +							  frameDurations[2]);
> +
>  	/*
>  	 * Compute the scaler crop limits.
>  	 *

-- 
Regards,

Laurent Pinchart