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

[Sebastian Fricke]

On 19.11.2020 16:25, Jacopo Mondi wrote:
>Hi Sebastian,
>
>On Thu, Nov 19, 2020 at 04:11:01PM +0100, Sebastian Fricke wrote:
>> 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?
>
>Well, kernel is C, so it does not disallow C++ commenting explicitely
>in facts :)
>
>But yes, we use /* */
>
>Also we use camelCase and not snake_case for variables
>

Thanks I take that into consideration for v2.

>>
>> >
>> > 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?
>
>I thought checkstyle checks for // and snake_case, I'm sorry...
>

Do we want to add that to the ToDo list?

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