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

[Dafna Hirschfeld]

On 12.03.21 22:31, Laurent Pinchart wrote:
> Hi Dafna,
> 
> On Tue, Mar 02, 2021 at 09:16:04AM +0100, Dafna Hirschfeld wrote:
>> On 02.03.21 03:39, Laurent Pinchart wrote:
>>> 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?
> 
> In this context, I believe that black level refers to black level
> correction (BLC in short), which is the process of subtracting a fixed
> value from all pixels to account for leakage currents and other
> parasitic effects in the sensor that makes fully black pixels have a
> non-zero value. Sensors typically have a set of lines before the image
> that is physically covered, and those lines can then be transmitted over
> CSI-2. The average black level will then be computed on the SoC side
> (either using the CPU, or in the ISP if it supports doing so), and
> configured in the ISP to subtract it from all pixels. The black lines
> are cropped out of what the ISP processes further down the pipeline.

The number of black/invalid lines depends on the sensor right?
So userspace should adjust the cropping according to the sensor.
If so then why would we want to always clamp to 4032x3024 ?
Or should it just be the initial value and userspace can then increase
the crop size?

> 
>> 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
> 
> Sounds like it's worth a try at least :-)
> 
>>>>>> 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();
>>>>>>     
>