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

[Dafna Hirschfeld]

On 02.03.21 03:39, Laurent Pinchart wrote:
> Hi Dafna,
> 
> On Mon, Mar 01, 2021 at 08:48:19AM +0100, Dafna Hirschfeld wrote:
>> On 28.02.21 16:49, Laurent Pinchart wrote:
>>> 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?
> 
> I think so. If the ISP supports a larger size than 4032x3024 before
> cropping, it should accept that on its sink pad, with the sink crop
> rectangle being adjusted to never be larger than 4032x3024 (for instance
> when userspace sets the format on the sink pad, the crop rectangle could
> be automatically set to the same size, clamped to 4032x3024 and
> centered). Userspace can then adjust the crop rectangle further if
> necessary.

In rkisp1-isp.c, there is diagram of the cropping regions.
It says that the 4032x3024 limit is 'for black level'.
Does that means that some sensors might send frames with black pixels in
the edges that need to be cropped?

The TRM says "Maximum input resolution of 4416x3312 pixels"
The datasheet then shows that the default values are 4096x3072 for both the
input resolution (ISP_ACQ) and for the corp in the sink (ISP_OUT).

So from the docs at least it sound possible to do as you suggested,
limit the input to 4416x3312 and then always set a crop with maximum
value 4032x3024

Thanks,
Dafna




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