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

[Laurent Pinchart]

Hi Sebastian,

Thank you for the patch.

On Sat, Feb 27, 2021 at 07:01:26PM +0100, Sebastian Fricke wrote:
> 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
> 
> 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.

It would have been nice if the ISP had an input crop rectangle :-S It
would have allowed us to crop the input image a little bit to 4032x2992
(keeping the same aspect ratio) or 4032x3024 (4:3). Just
double-checking, is there indeed no way to crop at the input, or could
it be that it's not implemented in the driver ? I can't find the
4032x3024 limit in the documentation I have access to.

> 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);

Related to 1/2, note that you don't necessarily need to store the ISP
pointer in RkISP1CameraData. You can access the pipeline handler here:

	PipelineHandlerRkISP1 *pipe =
		static_cast<PipelineHandlerRkISP1 *>(data_->pipe_);
	V4L2Subdevice::Formats ispFormats = pipe->isp_->formats(0);

> +	if (ispFormats.empty()) {
> +		LOG(RkISP1, Error) << "Unable to fetch ISP formats.";
> +		return Invalid;
> +	}

Missing blank line.

> +	/*
> +	 * 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;

This makes me think we need better comparison functions for the Size
class. Maybe a Size::contains() function ? It doesn't have to be part of
this series.

> +		maxSensorSize = std::max(maxSensorSize, size);
> +	}

Missing blank line here too.

Could we move all the code above to
PipelineHandlerRkISP1::createCamera() and store maxSensorSize in
RkISP1CameraData, to avoid doing the calculation every time validate()
is called ?


> +	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();
>  

-- 
Regards,

Laurent Pinchart