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

11408 diff mbox series

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.

Signed-off-by: Sebastian Fricke <sebastian.fricke@posteo.net>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 35 +++++++++++++++++++++---
 1 file changed, 31 insertions(+), 4 deletions(-)

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

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

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.

> >> Signed-off-by: Sebastian Fricke <sebastian.fricke@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();
> >>   
> >

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

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

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

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

Hello Dafna, Laurent and Heiko,

thank you all for your reviews, I have looked further into the problem
and would like to share my thoughts with you.

Comments below...

On 15.03.2021 17:52, Dafna Hirschfeld wrote:
>
>
>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 :-)

Dafna advised me to try and set the MAX and MIN size simply to
4416x3312, so I did and here are the resulting images.

I captured one video with these settings:
`LIBCAMERA_LOG_LEVELS=0 cam --camera=1 --capture=50 --file=/tmp/out_video -s height=600,width=800,pixelformat=NV12,role=video`

libcamera then sets the camera pipeline like this:
```
[0:35:57.915030227] [6669] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 2112x1568-SBGGR10_1X10
[0:35:57.915069602] [6669] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 2112x1568-SBGGR10_1X10
[0:35:57.915125018] [6669] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
[0:35:57.915152143] [6669] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
[0:35:57.915183643] [6669] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
[0:35:57.915234684] [6669] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
[0:35:57.915262101] [6669] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 800x600-YUYV8_2X8
[0:35:57.915284850] [6669] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 800x600-YUYV8_1_5X8
```

And another video with these settings:
`cam --camera=1 --capture=50 --file=/tmp/out_video -s height=2160,width=3840,pixelformat=NV12,role=video`

libcamera sets the camera pipeline like this:
```
[0:52:25.445115742] [8997] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 4224x3136-SBGGR10_1X10
[0:52:25.445158617] [8997] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 4224x3136-SBGGR10_1X10
[0:52:25.445214616] [8997] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 4224x3136-SBGGR10_1X10 crop (0x0)/4224x3136
[0:52:25.445242616] [8997] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
[0:52:25.445274991] [8997] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
[0:52:25.445325449] [8997] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
[0:52:25.445351116] [8997] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 3840x2160-YUYV8_2X8
[0:52:25.445372990] [8997] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 3840x2160-YUYV8_1_5X8
```

Here are the results of both captures (I have taken roughly the same
frame number):
https://imgur.com/a/EFtEmB9

The image quality of the higher resolution image is obviously better and
it is also quite a lot brighter. I was not able to detect any defect
pixels in the image so far.
But I noticed the following:
My sensor has a Black Level Calibration (BLC) register (@ 0x5001),
usually, BLC is enabled. If I turn it off and try to capture with a
sensor resolution of 4224x3136, then the pipeline can still be
created, validated and the buffers are queued, but it seems like the sensor
doesn't start anymore.  When I use the lower resolution mode of the sensor
(2112x1568), while deactivating BLC everything works just fine.
So, something happens when BLC is off when the resolution is greater
than 4032x3024. (Sadly no part of the camera pipeline actually throws an
error)

Here is the debug log from cam:
```
[0:11:23.311130186] [4131] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 4224x3136-SBGGR10_1X10
[0:11:23.311269310] [4131] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 4224x3136-SBGGR10_1X10
[0:11:23.311451309] [4131] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 4224x3136-SBGGR10_1X10 crop (0x0)/4224x3136
[0:11:23.311571766] [4131] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
[0:11:23.311728682] [4131] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
[0:11:23.311918555] [4131] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
[0:11:23.312134679] [4131] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 1920x1920-YUYV8_2X8
[0:11:23.312276428] [4131] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 1920x1920-YUYV8_1_5X8
[0:11:23.313918792] [4131]  INFO IPARkISP1 rkisp1.cpp:120 Exposure: 4-3324 Gain: 16-248
[0:11:23.314731370] [4131] DEBUG DelayedControls delayed_controls.cpp:178 Queuing Analogue Gain to 16 at index 1
[0:11:23.314913660] [4131] DEBUG DelayedControls delayed_controls.cpp:178 Queuing Exposure to 4 at index 1
[0:11:23.336376391] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 4 buffers requested.
[0:11:23.337981130] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 0 buffers requested.
[0:11:23.338467627] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0
[0:11:23.339042498] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0
[0:11:23.339212247] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0
[0:11:23.339364496] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0
[0:11:23.339509453] [4130] DEBUG Camera camera.cpp:1029 Starting capture
[0:11:23.340191657] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video3[15:out]: 4 buffers requested.
[0:11:23.341328566] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video2[14:cap]: 4 buffers requested.
[0:11:23.344065548] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 4 buffers requested.
[0:11:23.344281088] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1351 /dev/video0[17:cap]: Prepared to import 4 buffers
Capture 5 frames
[0:11:23.346221533] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 0
[0:11:23.346598364] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 0
[0:11:23.346800779] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 0
[0:11:23.347141443] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 1
[0:11:23.347319651] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 1
[0:11:23.347473941] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 1
[0:11:23.347759189] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 2
[0:11:23.347921938] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 2
[0:11:23.348139520] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 2
[0:11:23.404159310] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 3
[0:11:23.404329934] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 3
[0:11:23.404401100] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 3
[0:11:23.404646390] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1509 /dev/video3[15:out]: Dequeuing buffer 0
^CExiting
```

Conclusion:
At first glance, I thought it might be fine to just increase the
resolution limits, but after seeing that this only works when the sensor
performs black level calibration, I think that it would be unwise to
perform this change.

I am now working on the initial idea to crop the sink pad of the ISP
down to 4032x3024, if and only if the input resolution is greater than
4032x3024 and smaller or equal to 4416x3312 (because I don't want to
create a crop to 4032x3024 when the input resolution is for example
2112x1568). That crop is automatically propagated to the source pad of
the ISP within `rkisp1_isp_set_sink_crop` and within
`rkisp1_isp_set_src_fmt` we use the crop resolution as format
resolution. I will post that patch soon to the mailing list.

>>
>>>>>>>Signed-off-by: Sebastian Fricke <sebastian.fricke@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.

I would love to work on that right after this patch and the connected
kernel patch :).

>>>>>>
>>>>>>>+		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 ?

Yes, that is a good idea, and actually when I think about it the most
logical location. I hope that my use case will be handled by the kernel
patch, but I will still post this patch in case a camera sensor comes
along with a resolution greater than 4416x3312. I will test the
libcamera patch without the kernel patch to make sure that it works. But
it would be great if someone could test the patch in combination with
the kernel patch, with an appropriate sensor. (I would also be open to
any recommendations as I don't know if such a sensor exists for my
platform).

>>>>>>
>>>>>>
>>>>>>>+	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.

I could add that in another patch, but I believe that if I don't require
that for the rkisp1, I would first wait for an actual use case to
appear.

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

Greetings,
Sebastian

Hello Laurent, Dafna and Helen,

I have investigated this issue and have written some thoughts below.

On 01.03.2021 08:48, Dafna Hirschfeld wrote:
>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?

I have looked into the code and I noticed that the cropping ability of
the ISP sink pad is more like a dummy functionality. You are able to
set a crop that is smaller than the format for the sink pad, but there
is a chain of events that make this condition invalid. When
`rkisp1_isp_set_sink_fmt` is called it automatically calls
`rkisp1_isp_set_sink_crop` with the last used crop for the pad
(0,0,800,600) by default. Within `set_sink_crop` the crop is aligned to
the `sink_fmt` so that it is within the bounds of the new format. Then
it goes on to call `rkisp1_isp_set_src_crop` with the last used crop on
the source pad, it maps the `src_crop` into the `sink_crop`. And finally
`rkisp1_isp_set_src_fmt` is called, which sets the width and height of
the format with the width and height of the crop.
We validate the links with `v4l2_subdev_link_validate_default`, which
checks if the formats of the pads are equal.

Therefore I can state the following:
- the sink pad crop is bounded by the sink pad format
- the source pad crop is bounded by the sink pad crop
- the source pad format is bounded by the source pad format
Meaning: when the sink pad format != sink pad crop, then the sink pad format
and source pad format can never be equal. And therefore can never be
validated.

The rockchip documentation of the ISP1 driver (http://opensource.rock-chips.com/wiki_Rockchip-isp1#V4l2_view),
mentions the following statement for the sink pad of the ISP:
"the size should be equal/less than sensor input size."
and for the source pad of the ISP:
"The size should be equal/less than sink pad size."

This means from the ISP's point of view, the following pad configuration
should probably work:
```
- entity 1: rkisp1_isp (4 pads, 5 links)
     ...
	pad0: Sink
		[fmt:SBGGR10_1X10/4224x3136 field:none
		 crop.bounds:(0,0)/4224x3136
		 crop:(96,72)/4032x2992]
		<- "ov13850 1-0010":0 [ENABLED]
     ...
	pad2: Source
		[fmt:YUYV8_2X8/4032x2992 field:none
		 crop.bounds:(0,0)/4032x2992
		 crop:(0,0)/4032x2992]
		-> "rkisp1_resizer_mainpath":0 [ENABLED]
		-> "rkisp1_resizer_selfpath":0 []
     ...
- entity 6: rkisp1_resizer_mainpath (2 pads, 2 links)
     ...
	pad0: Sink
		[fmt:YUYV8_2X8/4032x2992 field:none
		 crop.bounds:(0,0)/4032x2992
		 crop:(0,0)/4032x2992]
		<- "rkisp1_isp":2 [ENABLED]
	pad1: Source
		[fmt:YUYV8_2X8/1920x1920 field:none]
		-> "rkisp1_mainpath":0 [ENABLED,IMMUTABLE]
- entity 28: ov13850 1-0010 (1 pad, 1 link)
     ...
	pad0: Source
		[fmt:SBGGR10_1X10/4224x3136@20000/150000 field:none
		 crop.bounds:(16,8)/4224x3136
		 crop:(16,8)/4224x3136]
		-> "rkisp1_isp":0 [ENABLED]
```

But the problem is that this not how the link validation currently works,
when the format sizes are not equal `v4l2_subdev_link_validate_default`,
will definitely fail.

Now, I have 3 ideas in mind for how to continue with this issue.
1. We could modify the callback to validate media links, that would
explicitly handle ISP sink and source differently, and check if the crops
align when the sink format size is greater than 4032x3024. I think that
this option is quite ugly and too specific.
2. We could add a new function similar to `v4l2_subdev_link_validate_default`,
that performs the same checks but additionally also checks if the crop
of a sink pad is equal to the format of a source pad when the format
size validation failed. I am not too sure about this either.
3. We don't touch this part of the code and I fix the issue only in
libcamera and the 4224x3136 mode of my camera cannot be used :(.

I am not too happy with any of the solutions, that I have proposed, I
would love to hear your thoughts and ideas.

Greetings,
Sebastian
>
>>
>>>Signed-off-by: Sebastian Fricke <sebastian.fricke@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();
>>

On 19.03.21 08:19, Sebastian Fricke wrote:
> Hello Laurent, Dafna and Helen,
> 
> I have investigated this issue and have written some thoughts below.
> 
> On 01.03.2021 08:48, Dafna Hirschfeld wrote:
>> 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?
> 
> I have looked into the code and I noticed that the cropping ability of
> the ISP sink pad is more like a dummy functionality. You are able to
> set a crop that is smaller than the format for the sink pad, but there
> is a chain of events that make this condition invalid. When
> `rkisp1_isp_set_sink_fmt` is called it automatically calls
> `rkisp1_isp_set_sink_crop` with the last used crop for the pad
> (0,0,800,600) by default. Within `set_sink_crop` the crop is aligned to
> the `sink_fmt` so that it is within the bounds of the new format. Then
> it goes on to call `rkisp1_isp_set_src_crop` with the last used crop on
> the source pad, it maps the `src_crop` into the `sink_crop`. And finally
> `rkisp1_isp_set_src_fmt` is called, which sets the width and height of
> the format with the width and height of the crop.
> We validate the links with `v4l2_subdev_link_validate_default`, which
> checks if the formats of the pads are equal.
> 
> Therefore I can state the following:
> - the sink pad crop is bounded by the sink pad format
> - the source pad crop is bounded by the sink pad crop
> - the source pad format is bounded by the source pad format
> Meaning: when the sink pad format != sink pad crop, then the sink pad format
> and source pad format can never be equal. And therefore can never be
> validated.

Hi, note that the function v4l2_subdev_link_validate_default compares the pads of a link, that is, the two pads belong
to different entities in both side of the link. So inside the isp entity it is valid to have "sink_fmt > sink_crop > src_fmt > src_crop"
since the src_* and sink_* values belong to different pads of the same entity and not to pads of the same link.

> 
> The rockchip documentation of the ISP1 driver (http://opensource.rock-chips.com/wiki_Rockchip-isp1#V4l2_view),
> mentions the following statement for the sink pad of the ISP:
> "the size should be equal/less than sensor input size."
> and for the source pad of the ISP:
> "The size should be equal/less than sink pad size."
> 
> This means from the ISP's point of view, the following pad configuration
> should probably work:
> ```
> - entity 1: rkisp1_isp (4 pads, 5 links)
>      ...
>      pad0: Sink
>          [fmt:SBGGR10_1X10/4224x3136 field:none
>           crop.bounds:(0,0)/4224x3136
>           crop:(96,72)/4032x2992]
>          <- "ov13850 1-0010":0 [ENABLED]
>      ...
>      pad2: Source
>          [fmt:YUYV8_2X8/4032x2992 field:none
>           crop.bounds:(0,0)/4032x2992
>           crop:(0,0)/4032x2992]
>          -> "rkisp1_resizer_mainpath":0 [ENABLED]
>          -> "rkisp1_resizer_selfpath":0 []
>      ...
> - entity 6: rkisp1_resizer_mainpath (2 pads, 2 links)
>      ...
>      pad0: Sink
>          [fmt:YUYV8_2X8/4032x2992 field:none
>           crop.bounds:(0,0)/4032x2992
>           crop:(0,0)/4032x2992]
>          <- "rkisp1_isp":2 [ENABLED]
>      pad1: Source
>          [fmt:YUYV8_2X8/1920x1920 field:none]
>          -> "rkisp1_mainpath":0 [ENABLED,IMMUTABLE]
> - entity 28: ov13850 1-0010 (1 pad, 1 link)
>      ...
>      pad0: Source
>          [fmt:SBGGR10_1X10/4224x3136@20000/150000 field:none
>           crop.bounds:(16,8)/4224x3136
>           crop:(16,8)/4224x3136]
>          -> "rkisp1_isp":0 [ENABLED]

This configuration seems valid to me.
Do you get EPIPE when streaming with this configuration?

Thanks,
Dafna

> ```
> 
> But the problem is that this not how the link validation currently works,
> when the format sizes are not equal `v4l2_subdev_link_validate_default`,
> will definitely fail.
> 
> Now, I have 3 ideas in mind for how to continue with this issue.
> 1. We could modify the callback to validate media links, that would
> explicitly handle ISP sink and source differently, and check if the crops
> align when the sink format size is greater than 4032x3024. I think that
> this option is quite ugly and too specific.
> 2. We could add a new function similar to `v4l2_subdev_link_validate_default`,
> that performs the same checks but additionally also checks if the crop
> of a sink pad is equal to the format of a source pad when the format
> size validation failed. I am not too sure about this either.
> 3. We don't touch this part of the code and I fix the issue only in
> libcamera and the 4224x3136 mode of my camera cannot be used :(.
> 
> I am not too happy with any of the solutions, that I have proposed, I
> would love to hear your thoughts and ideas.
> 
> Greetings,
> Sebastian
>>
>>>
>>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@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();
>>>

Hi Sebastian and Dafna,

Sorry for the late reply.

On Wed, Mar 17, 2021 at 08:07:16AM +0100, Sebastian Fricke wrote:
> Hello Dafna, Laurent and Heiko,
> 
> thank you all for your reviews, I have looked further into the problem
> and would like to share my thoughts with you.
> 
> Comments below...
> 
> On 15.03.2021 17:52, Dafna Hirschfeld wrote:
> > On 12.03.21 22:31, Laurent Pinchart wrote:
> >> 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?

That's right.

> > So userspace should adjust the cropping according to the sensor.

It depends a bit on the sensor. Most sensors will not by default send
the optical black lines, so the ISP just doesn't receive them, and
there's nothing to crop. We can usually instruct the sensor to send
those lines (V4L2 is missing an API to do so properly), in which case
they can be send as a fixed number of lines before and/or after the
image that we would then need to instruct the ISP to crop (or to handle
in a special way, depending on the ISP). For CSI-2 sensors, the optical
black lines can be tagged with a different data type, and the CSI-2
receiver can then (if it supports this feature) capture them in a
separate buffer. There are thus lots of options, depending on the
hardware capabilities.

In this specific case, I don't think the sensor sends us any optical
black lines (at least not with the main image data type).

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

As I understand it, the ISP can't process images larger than 4032x3024
(due to limitations of the processing blocks inside the ISP). It however
can receive large frames at its input and crop them before any further
processing, so a sensor that would send a size larger than 4032x3024
could be supported.

This is based on the information I have so far, I haven't studied the
TRM in depth, so details may not be correct, especially the exact limits
on the resolution (information from different documents seems to
disagree, based on this e-mail discussion).

> >>> 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 :-)
> 
> Dafna advised me to try and set the MAX and MIN size simply to
> 4416x3312, so I did and here are the resulting images.
> 
> I captured one video with these settings:
> `LIBCAMERA_LOG_LEVELS=0 cam --camera=1 --capture=50 --file=/tmp/out_video -s height=600,width=800,pixelformat=NV12,role=video`
> 
> libcamera then sets the camera pipeline like this:
> ```
> [0:35:57.915030227] [6669] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 2112x1568-SBGGR10_1X10
> [0:35:57.915069602] [6669] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 2112x1568-SBGGR10_1X10
> [0:35:57.915125018] [6669] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 2112x1568-SBGGR10_1X10 crop (0x0)/2112x1568
> [0:35:57.915152143] [6669] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [0:35:57.915183643] [6669] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [0:35:57.915234684] [6669] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 2112x1568-YUYV8_2X8 crop (0x0)/2112x1568
> [0:35:57.915262101] [6669] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 800x600-YUYV8_2X8
> [0:35:57.915284850] [6669] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 800x600-YUYV8_1_5X8
> ```
> 
> And another video with these settings:
> `cam --camera=1 --capture=50 --file=/tmp/out_video -s height=2160,width=3840,pixelformat=NV12,role=video`
> 
> libcamera sets the camera pipeline like this:
> ```
> [0:52:25.445115742] [8997] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 4224x3136-SBGGR10_1X10
> [0:52:25.445158617] [8997] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 4224x3136-SBGGR10_1X10
> [0:52:25.445214616] [8997] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 4224x3136-SBGGR10_1X10 crop (0x0)/4224x3136
> [0:52:25.445242616] [8997] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
> [0:52:25.445274991] [8997] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
> [0:52:25.445325449] [8997] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
> [0:52:25.445351116] [8997] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 3840x2160-YUYV8_2X8
> [0:52:25.445372990] [8997] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 3840x2160-YUYV8_1_5X8
> ```
> 
> Here are the results of both captures (I have taken roughly the same
> frame number):
> https://imgur.com/a/EFtEmB9
> 
> The image quality of the higher resolution image is obviously better and
> it is also quite a lot brighter. I was not able to detect any defect
> pixels in the image so far.
> But I noticed the following:
> My sensor has a Black Level Calibration (BLC) register (@ 0x5001),
> usually, BLC is enabled. If I turn it off and try to capture with a
> sensor resolution of 4224x3136, then the pipeline can still be
> created, validated and the buffers are queued, but it seems like the sensor
> doesn't start anymore.  When I use the lower resolution mode of the sensor
> (2112x1568), while deactivating BLC everything works just fine.
> So, something happens when BLC is off when the resolution is greater
> than 4032x3024. (Sadly no part of the camera pipeline actually throws an
> error)

That sounds very fishy. BLC is the process of subtracting the black
level from all pixels. It shouldn't affect anything that the rkisp1
would care about. It may be an issue internal to the sensor.

This being said, maybe there's a side effect of the sensor sending the
black lines out and thus making the image size larger when BLC is
disabled (unlikely, but OmniVision...). This could affect the ISP. You
could try to play with cropping on the sensor side to reduce the height
a little bit and see if it fixes things.

> Here is the debug log from cam:
> ```
> [0:11:23.311130186] [4131] DEBUG RkISP1 rkisp1.cpp:579 Configuring sensor with 4224x3136-SBGGR10_1X10
> [0:11:23.311269310] [4131] DEBUG RkISP1 rkisp1.cpp:585 Sensor configured with 4224x3136-SBGGR10_1X10
> [0:11:23.311451309] [4131] DEBUG RkISP1 rkisp1.cpp:596 ISP input pad configured with 4224x3136-SBGGR10_1X10 crop (0x0)/4224x3136
> [0:11:23.311571766] [4131] DEBUG RkISP1 rkisp1.cpp:602 Configuring ISP output pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
> [0:11:23.311728682] [4131] DEBUG RkISP1 rkisp1.cpp:614 ISP output pad configured with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
> [0:11:23.311918555] [4131] DEBUG RkISP1 rkisp1_path.cpp:119 Configured main resizer input pad with 4224x3136-YUYV8_2X8 crop (0x0)/4224x3136
> [0:11:23.312134679] [4131] DEBUG RkISP1 rkisp1_path.cpp:125 Configuring main resizer output pad with 1920x1920-YUYV8_2X8
> [0:11:23.312276428] [4131] DEBUG RkISP1 rkisp1_path.cpp:143 Configured main resizer output pad with 1920x1920-YUYV8_1_5X8
> [0:11:23.313918792] [4131]  INFO IPARkISP1 rkisp1.cpp:120 Exposure: 4-3324 Gain: 16-248
> [0:11:23.314731370] [4131] DEBUG DelayedControls delayed_controls.cpp:178 Queuing Analogue Gain to 16 at index 1
> [0:11:23.314913660] [4131] DEBUG DelayedControls delayed_controls.cpp:178 Queuing Exposure to 4 at index 1
> [0:11:23.336376391] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 4 buffers requested.
> [0:11:23.337981130] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 0 buffers requested.
> [0:11:23.338467627] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0
> [0:11:23.339042498] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0
> [0:11:23.339212247] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0
> [0:11:23.339364496] [4130] DEBUG Request request.cpp:92 Created request - cookie: 0
> [0:11:23.339509453] [4130] DEBUG Camera camera.cpp:1029 Starting capture
> [0:11:23.340191657] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video3[15:out]: 4 buffers requested.
> [0:11:23.341328566] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video2[14:cap]: 4 buffers requested.
> [0:11:23.344065548] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1139 /dev/video0[17:cap]: 4 buffers requested.
> [0:11:23.344281088] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1351 /dev/video0[17:cap]: Prepared to import 4 buffers
> Capture 5 frames
> [0:11:23.346221533] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 0
> [0:11:23.346598364] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 0
> [0:11:23.346800779] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 0
> [0:11:23.347141443] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 1
> [0:11:23.347319651] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 1
> [0:11:23.347473941] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 1
> [0:11:23.347759189] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 2
> [0:11:23.347921938] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 2
> [0:11:23.348139520] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 2
> [0:11:23.404159310] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video3[15:out]: Queueing buffer 3
> [0:11:23.404329934] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video2[14:cap]: Queueing buffer 3
> [0:11:23.404401100] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1440 /dev/video0[17:cap]: Queueing buffer 3
> [0:11:23.404646390] [4131] DEBUG V4L2 v4l2_videodevice.cpp:1509 /dev/video3[15:out]: Dequeuing buffer 0
> ^CExiting
> ```
> 
> Conclusion:
> At first glance, I thought it might be fine to just increase the
> resolution limits, but after seeing that this only works when the sensor
> performs black level calibration, I think that it would be unwise to
> perform this change.

Can't we always enable BLC on the sensor ?

> I am now working on the initial idea to crop the sink pad of the ISP
> down to 4032x3024, if and only if the input resolution is greater than
> 4032x3024 and smaller or equal to 4416x3312 (because I don't want to
> create a crop to 4032x3024 when the input resolution is for example
> 2112x1568). That crop is automatically propagated to the source pad of
> the ISP within `rkisp1_isp_set_sink_crop` and within
> `rkisp1_isp_set_src_fmt` we use the crop resolution as format
> resolution. I will post that patch soon to the mailing list.
> 
> >>>>>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@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.
> 
> I would love to work on that right after this patch and the connected
> kernel patch :).
> 
> >>>>>> 
> >>>>>>> +		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 ?
> 
> Yes, that is a good idea, and actually when I think about it the most
> logical location. I hope that my use case will be handled by the kernel
> patch, but I will still post this patch in case a camera sensor comes
> along with a resolution greater than 4416x3312. I will test the
> libcamera patch without the kernel patch to make sure that it works. But
> it would be great if someone could test the patch in combination with
> the kernel patch, with an appropriate sensor. (I would also be open to
> any recommendations as I don't know if such a sensor exists for my
> platform).
> 
> >>>>>>> +	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.
> 
> I could add that in another patch, but I believe that if I don't require
> that for the rkisp1, I would first wait for an actual use case to
> appear.
> 
> >>>>> 
> >>>>>>> @@ -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();

Hi Dafna and Sebastian,

On Tue, Mar 23, 2021 at 12:56:11PM +0100, Dafna Hirschfeld wrote:
> On 19.03.21 08:19, Sebastian Fricke wrote:
> > On 01.03.2021 08:48, 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 have looked into the code and I noticed that the cropping ability of
> > the ISP sink pad is more like a dummy functionality. You are able to
> > set a crop that is smaller than the format for the sink pad, but there
> > is a chain of events that make this condition invalid. When
> > `rkisp1_isp_set_sink_fmt` is called it automatically calls
> > `rkisp1_isp_set_sink_crop` with the last used crop for the pad
> > (0,0,800,600) by default. Within `set_sink_crop` the crop is aligned to
> > the `sink_fmt` so that it is within the bounds of the new format. Then
> > it goes on to call `rkisp1_isp_set_src_crop` with the last used crop on
> > the source pad, it maps the `src_crop` into the `sink_crop`. And finally
> > `rkisp1_isp_set_src_fmt` is called, which sets the width and height of
> > the format with the width and height of the crop.
> > We validate the links with `v4l2_subdev_link_validate_default`, which
> > checks if the formats of the pads are equal.
> > 
> > Therefore I can state the following:
> > - the sink pad crop is bounded by the sink pad format
> > - the source pad crop is bounded by the sink pad crop
> > - the source pad format is bounded by the source pad format
> > Meaning: when the sink pad format != sink pad crop, then the sink pad format
> > and source pad format can never be equal. And therefore can never be
> > validated.
> 
> Hi, note that the function v4l2_subdev_link_validate_default compares
> the pads of a link, that is, the two pads belong to different entities
> in both side of the link. So inside the isp entity it is valid to have
> "sink_fmt > sink_crop > src_fmt > src_crop" since the src_* and sink_*
> values belong to different pads of the same entity and not to pads of
> the same link.

It's actually more than valid, it's fairly normal, given that cropping
can only produce a smaller (or equal) image :-)

> > The rockchip documentation of the ISP1 driver (http://opensource.rock-chips.com/wiki_Rockchip-isp1#V4l2_view),
> > mentions the following statement for the sink pad of the ISP:
> > "the size should be equal/less than sensor input size."
> > and for the source pad of the ISP:
> > "The size should be equal/less than sink pad size."

Note that there are two things to consider: the hardware capabilities,
and the driver implementation. The latter could be incorrect in some
areas (which is the topic of this whole e-mail thread), so you can
challenge any driver implementation decision if they don't match the
hardware capabilities.

> > This means from the ISP's point of view, the following pad configuration
> > should probably work:
> > ```
> > - entity 1: rkisp1_isp (4 pads, 5 links)
> >      ...
> >      pad0: Sink
> >          [fmt:SBGGR10_1X10/4224x3136 field:none
> >           crop.bounds:(0,0)/4224x3136
> >           crop:(96,72)/4032x2992]
> >          <- "ov13850 1-0010":0 [ENABLED]
> >      ...
> >      pad2: Source
> >          [fmt:YUYV8_2X8/4032x2992 field:none
> >           crop.bounds:(0,0)/4032x2992
> >           crop:(0,0)/4032x2992]
> >          -> "rkisp1_resizer_mainpath":0 [ENABLED]
> >          -> "rkisp1_resizer_selfpath":0 []
> >      ...
> > - entity 6: rkisp1_resizer_mainpath (2 pads, 2 links)
> >      ...
> >      pad0: Sink
> >          [fmt:YUYV8_2X8/4032x2992 field:none
> >           crop.bounds:(0,0)/4032x2992
> >           crop:(0,0)/4032x2992]
> >          <- "rkisp1_isp":2 [ENABLED]
> >      pad1: Source
> >          [fmt:YUYV8_2X8/1920x1920 field:none]
> >          -> "rkisp1_mainpath":0 [ENABLED,IMMUTABLE]
> > - entity 28: ov13850 1-0010 (1 pad, 1 link)
> >      ...
> >      pad0: Source
> >          [fmt:SBGGR10_1X10/4224x3136@20000/150000 field:none
> >           crop.bounds:(16,8)/4224x3136
> >           crop:(16,8)/4224x3136]
> >          -> "rkisp1_isp":0 [ENABLED]
> 
> This configuration seems valid to me.
> Do you get EPIPE when streaming with this configuration?

The configuration seems valid to me too.

> > ```
> > 
> > But the problem is that this not how the link validation currently works,
> > when the format sizes are not equal `v4l2_subdev_link_validate_default`,
> > will definitely fail.

I think failures need to be investigated, because, as Dafna explained,
link validation will compare formats on the two end of each link, not
formats on different pads of the same subdev. Validation of the
configuration of a given subdev (for instance the formats and crop
rectangles on the rkisp1_isp subdev) is something that is handled by the
subdev drivers themselves, at .set_format() and .set_selection() time,
not when starting the stream.

You can enable the debugging messages in
v4l2_subdev_link_validate_default() to get more information about what
fails exactly (and possibly add more debugging messages in other places
if the existing ones don't provide enough information to debug the
issue).

> > Now, I have 3 ideas in mind for how to continue with this issue.
> > 1. We could modify the callback to validate media links, that would
> > explicitly handle ISP sink and source differently, and check if the crops
> > align when the sink format size is greater than 4032x3024. I think that
> > this option is quite ugly and too specific.
> > 2. We could add a new function similar to `v4l2_subdev_link_validate_default`,
> > that performs the same checks but additionally also checks if the crop
> > of a sink pad is equal to the format of a source pad when the format
> > size validation failed. I am not too sure about this either.
> > 3. We don't touch this part of the code and I fix the issue only in
> > libcamera and the 4224x3136 mode of my camera cannot be used :(.
> > 
> > I am not too happy with any of the solutions, that I have proposed, I
> > would love to hear your thoughts and ideas.
> >
> >>>> Signed-off-by: Sebastian Fricke <sebastian.fricke@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();
> >>>