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

[Dafna Hirschfeld]

Hi

On 28.02.21 16:49, Laurent Pinchart wrote:
> 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.

The isp does support cropping on the sink pad.
But currently the function v4l2_subdev_link_validate_default compares
the dimensions defined in v4l2_subdev_format which are not the crop
dimensions but the ones set by set_fmt. Is that wrong?

> 
>> 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,

I wonder if it won't be an easier solution to add another optional parameter
to the getFormat method of the sensor that limits the size that the sensor
should return. This might benefit other pipline-handlers as well where
a sensor is connected to an entity that has a maximal size it can accept.
In rkisp1 all the above code could be replaced by just adding
Size(4032,3024) as another parameter to getFormat.

Thanks,
Dafna

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