[libcamera-devel] [PATCH 1/1] pipeline: rkisp1: Fix sensor-ISP format mismatch

[Helen Koike]

Hi Sebastian,

Thanks for your patch.

On 11/19/20 10:13 AM, Sebastian Fricke wrote:
> Make sure to use a sensor format resolution that is smaller or equal to
> the maximum allowed resolution for the RkISP1. The maximum resolution
> is defined within the `rkisp1-common.h` file as:
> define RKISP1_ISP_MAX_WIDTH		4032
> define RKISP1_ISP_MAX_HEIGHT		3024
> 
> Change the order of setting the formats, in order to first check if the
> requested resolution exceeds the maximum and search for the next smaller
> available sensor resolution if that is the case.
> Fail if no viable sensor format was located.
> 
> This means that some camera sensors can never operate with their maximum
> resolution, for example on my OV13850 camera sensor, there are two
> possible resolutions: 4224x3136 & 2112x1568, the first of those two will
> never be picked as it surpasses the maximum of the ISP.
> 
> Signed-off-by: Sebastian Fricke <sebastian.fricke.linux at gmail.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 36 ++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 1b1922a..621e9bf 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -671,6 +671,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	RkISP1CameraData *data = cameraData(camera);
>  	CameraSensor *sensor = data->sensor_;
>  	int ret;
> +	Size original_format_size = Size(0, 0);

I believe you can declare this when you need it (when assigning format.size),
and you don't need to call the constructor to init to zero.

(at least this is the style I see being used in the project).

> +	Size max_size = Size(0, 0);

You can declare this inside the if block, since you only use this there.

> +
>  
>  	ret = initLinks(camera, sensor, *config);
>  	if (ret)
> @@ -682,17 +685,44 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	 */
>  	V4L2SubdeviceFormat format = config->sensorFormat();
>  	LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString();

This message should be updated, since you are configuring the isp and not the sensor.

> +	// format is changed in setFormat, keep the resolution for comparison
> +	original_format_size = format.size;
>  
> -	ret = sensor->setFormat(&format);
> +	ret = isp_->setFormat(0, &format);
>  	if (ret < 0)
>  		return ret;
> +	LOG(RkISP1, Debug) << "ISP configured with " << format.toString();
> +
> +	if (original_format_size > format.size) {
> +		LOG(RkISP1, Info) << "Configured resolution is greater than "
> +				     "the maximum resolution for the ISP, "
> +				     "trying to re-configure to a smaller "
> +				     "valid sensor format";
> +		for (const Size &size : sensor->sizes()) {
> +			if (size <= format.size && size > max_size)

I'm not sure if comparing Size objects is enough.

According to the docs:

 * Sizes are compared on three criteria, in the following order.
 *
 * - A size with smaller width and smaller height is smaller.
 * - A size with smaller area is smaller.
 * - A size with smaller width is smaller.

If it reaches the 3rd case, we can have a resolution that is smaller in
width but bigger in height that could go over isp limitation leading to
a non-matching configuration.

Regards,
Helen

> +				max_size = size;
> +		}
> +		if (max_size == Size(0, 0)) {
> +			LOG(RkISP1, Error) << "No available sensor resolution"
> +					      "that is smaller or equal to "
> +					   << format.toString();
> +			return -1;
> +		}
> +		format = sensor->getFormat(sensor->mbusCodes(), max_size);
>  
> -	LOG(RkISP1, Debug) << "Sensor configured with " << format.toString();
> +		ret = isp_->setFormat(0, &format);
> +		if (ret < 0)
> +			return ret;
> +		LOG(RkISP1, Debug) << "ISP re-configured with "
> +				   << format.toString();
> +	}
>  
> -	ret = isp_->setFormat(0, &format);
> +	ret = sensor->setFormat(&format);
>  	if (ret < 0)
>  		return ret;
>  
> +	LOG(RkISP1, Debug) << "Sensor configured with " << format.toString();
> +
>  	Rectangle rect(0, 0, format.size);
>  	ret = isp_->setSelection(0, V4L2_SEL_TGT_CROP, &rect);
>  	if (ret < 0)
>