[libcamera-devel] [PATCH v3 1/5] libcamera: ipu3: Initialize controls using sensor resolution

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Feb 22, 2021 at 11:52:18AM +0100, Jacopo Mondi wrote:
> The controls' limits initialized by the IPU3 pipeline handler depend
> on the sensor configuration. In order to compute controls using a known
> state apply to the sensor a configuration equal to its own resolution.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index b8a655ce0b68..f867b5913b27 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -805,10 +805,21 @@ bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)
>   */
>  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  {
> +	/*
> +	 * \todo The here intialized controls depend on sensor configuration.

s/intialized/initialized/

"here initialized" sounds weird to me, would "The controls initialized
herein depend on sensor configuration" be better ?

But more than that, the todo should describe what needs to be done, not
just the current state, otherwise we'll wonder in a few months from now
what was meant.

The rest of the patch looks good, so it's really a minor comment.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	 *
> +	 * Initialize the sensor using its resolution and compute the control
> +	 * limits.
> +	 */
>  	CameraSensor *sensor = data->cio2_.sensor();
> -	CameraSensorInfo sensorInfo{};
> +	V4L2SubdeviceFormat sensorFormat = {};
> +	sensorFormat.size = sensor->resolution();
> +	int ret = sensor->setFormat(&sensorFormat);
> +	if (ret)
> +		return ret;
>  
> -	int ret = sensor->sensorInfo(&sensorInfo);
> +	CameraSensorInfo sensorInfo{};
> +	ret = sensor->sensorInfo(&sensorInfo);
>  	if (ret)
>  		return ret;
>  
> @@ -851,17 +862,6 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
>  	 * sensor's full frame as ImgU input).
>  	 */
>  
> -	/* Re-fetch the sensor info updated to use the largest resolution. */
> -	V4L2SubdeviceFormat sensorFormat = {};
> -	sensorFormat.size = sensor->resolution();
> -	ret = sensor->setFormat(&sensorFormat);
> -	if (ret)
> -		return ret;
> -
> -	ret = sensor->sensorInfo(&sensorInfo);
> -	if (ret)
> -		return ret;
> -
>  	/*
>  	 * The maximum scaler crop rectangle is the analogue crop used to
>  	 * produce the maximum frame size.

-- 
Regards,

Laurent Pinchart