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

[heiko at sntech.de]

Am Samstag, 27. Februar 2021, 19:01:26 CET schrieb Sebastian Fricke:
> This patch fixes a mismatch of image formats during the pipeline
> creation of the RkISP1. The mismatch happens because the current code
> does not check if the configured format exceeds the maximum viable
> resolution of the ISP.
> 
> 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

hmm, somehow these sizes look interesting, but this patch triggered me
into looking this up :-)

I.e. in the vendor-kernel Rockchip defines [0] a number of different max-sizes
and none seem to match these 4032.

#define CIF_ISP_INPUT_W_MAX		4416
#define CIF_ISP_INPUT_H_MAX		3312
#define CIF_ISP_INPUT_W_MAX_V12	3264
#define CIF_ISP_INPUT_H_MAX_V12	2448
#define CIF_ISP_INPUT_W_MAX_V13	1920
#define CIF_ISP_INPUT_H_MAX_V13	1080

Funnily enough my (old) rk3399 datasheet specifies 3264x2448 as max
ISP input size, but I guess the vendor-code will be "more" correct.

At least I understand now that I need to also handle different sizes when
updating my V12 patches for the kernel-side.


Heiko

[0] https://github.com/rockchip-linux/kernel/commit/40ce742d635eb8f821009b20f09668f4a1261e47

> Enumerate the frame-sizes of the ISP entity and compare the maximum with
> the configured resolution.
> 
> 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 at posteo.net>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 50eaa6a4..56a406c1 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -474,10 +474,37 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  		return Invalid;
>  	}
>  
> -	/* Select the sensor format. */
> -	Size maxSize;
> +	/* Get the ISP resolution limits */
> +	V4L2Subdevice::Formats ispFormats = data_->isp_->formats(0);
> +	if (ispFormats.empty()) {
> +		LOG(RkISP1, Error) << "Unable to fetch ISP formats.";
> +		return Invalid;
> +	}
> +	/*
> +	 * The maximum resolution is identical for all media bus codes on
> +	 * the RkISP1 isp entity. Therefore take the first available resolution.
> +	 */
> +	Size ispMaximum = ispFormats.begin()->second[0].max;
> +
> +	/*
> +	 * Select the sensor format, use either the best fit to the configured
> +	 * format or a specific sensor format, when getFormat would choose a
> +	 * resolution that surpasses the ISP maximum.
> +	 */
> +	Size maxSensorSize;
> +	for (const Size &size : sensor->sizes()) {
> +		if (size.width > ispMaximum.width ||
> +		    size.height > ispMaximum.height)
> +			continue;
> +		maxSensorSize = std::max(maxSensorSize, size);
> +	}
> +	Size maxConfigSize;
>  	for (const StreamConfiguration &cfg : config_)
> -		maxSize = std::max(maxSize, cfg.size);
> +		maxConfigSize = std::max(maxConfigSize, cfg.size);
> +
> +	if (maxConfigSize.height <= maxSensorSize.height &&
> +	    maxConfigSize.width <= maxSensorSize.width)
> +		maxSensorSize = maxConfigSize;
>  
>  	sensorFormat_ = sensor->getFormat({ MEDIA_BUS_FMT_SBGGR12_1X12,
>  					    MEDIA_BUS_FMT_SGBRG12_1X12,
> @@ -491,7 +518,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  					    MEDIA_BUS_FMT_SGBRG8_1X8,
>  					    MEDIA_BUS_FMT_SGRBG8_1X8,
>  					    MEDIA_BUS_FMT_SRGGB8_1X8 },
> -					  maxSize);
> +					  maxSensorSize);
>  	if (sensorFormat_.size.isNull())
>  		sensorFormat_.size = sensor->resolution();
>  
>