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

[Sebastian Fricke]

On 19.11.2020 15:58, Jacopo Mondi wrote:
>Hi Sebastian,

Hey Jacopo,

>
>On Thu, Nov 19, 2020 at 02:13:31PM +0100, Sebastian Fricke wrote:
>> 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
>>
>> Change the order of setting the formats, in order to first check if the
>> requested resolution exceeds the maximum and search for the next smaller
>> available sensor resolution if that is the case.
>> Fail if no viable sensor format was located.
>>
>> 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.linux at gmail.com>
>> ---
>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 36 ++++++++++++++++++++++--
>>  1 file changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index 1b1922a..621e9bf 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -671,6 +671,9 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>>  	RkISP1CameraData *data = cameraData(camera);
>>  	CameraSensor *sensor = data->sensor_;
>>  	int ret;
>> +	Size original_format_size = Size(0, 0);
>> +	Size max_size = Size(0, 0);
>> +
>>
>>  	ret = initLinks(camera, sensor, *config);
>>  	if (ret)
>> @@ -682,17 +685,44 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>>  	 */
>>  	V4L2SubdeviceFormat format = config->sensorFormat();
>>  	LOG(RkISP1, Debug) << "Configuring sensor with " << format.toString();
>> +	// format is changed in setFormat, keep the resolution for comparison
>
>Just a style note skimming through the patch.
>
>libcamera enforces a common code style
>http://libcamera.org/coding-style.html#coding-style-guidelines

I think you refer to the '//' comment, should I use a '/* */' comment
block for this comment? I read through both the style guidelines and
kernel guidelines and I couldn't spot a sentence where it states that
'//' comments are not allowed. Or do you not like that explain why I do
it?

>
>and provides a style checker tool in utils/checkstyle.py
>
>Make sure to use it before submitting patches.

That is odd, here is the result of the run I did before submitting the
patch:

```
basti at basti:~/Kernel/libcamera$ ./utils/checkstyle.py
-----------------------------------------------------------------------------------------
4cfb814796f2f27bfa4057b2a69618b01eb1de93 pipeline: rkisp1: Fix sensor-ISP format mismatch
-----------------------------------------------------------------------------------------
No style issue detected
```

Do you get a different result?
And should we change the the style checker in order to detect this
mistake?


>
>Thanks
>  j

Thanks for taking a look!
Sebastian

>
>> +	original_format_size = format.size;
>>
>> -	ret = sensor->setFormat(&format);
>> +	ret = isp_->setFormat(0, &format);
>>  	if (ret < 0)
>>  		return ret;
>> +	LOG(RkISP1, Debug) << "ISP configured with " << format.toString();
>> +
>> +	if (original_format_size > format.size) {
>> +		LOG(RkISP1, Info) << "Configured resolution is greater than "
>> +				     "the maximum resolution for the ISP, "
>> +				     "trying to re-configure to a smaller "
>> +				     "valid sensor format";
>> +		for (const Size &size : sensor->sizes()) {
>> +			if (size <= format.size && size > max_size)
>> +				max_size = size;
>> +		}
>> +		if (max_size == Size(0, 0)) {
>> +			LOG(RkISP1, Error) << "No available sensor resolution"
>> +					      "that is smaller or equal to "
>> +					   << format.toString();
>> +			return -1;
>> +		}
>> +		format = sensor->getFormat(sensor->mbusCodes(), max_size);
>>
>> -	LOG(RkISP1, Debug) << "Sensor configured with " << format.toString();
>> +		ret = isp_->setFormat(0, &format);
>> +		if (ret < 0)
>> +			return ret;
>> +		LOG(RkISP1, Debug) << "ISP re-configured with "
>> +				   << format.toString();
>> +	}
>>
>> -	ret = isp_->setFormat(0, &format);
>> +	ret = sensor->setFormat(&format);
>>  	if (ret < 0)
>>  		return ret;
>>
>> +	LOG(RkISP1, Debug) << "Sensor configured with " << format.toString();
>> +
>>  	Rectangle rect(0, 0, format.size);
>>  	ret = isp_->setSelection(0, V4L2_SEL_TGT_CROP, &rect);
>>  	if (ret < 0)
>> --
>> 2.20.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel