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

[Jacopo Mondi]

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

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

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