[libcamera-devel] [PATCH] libcamera: vimc: Adjust crop rectangle

[Jacopo Mondi]

Hi Laurent,

On Fri, May 01, 2020 at 07:43:56PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, May 01, 2020 at 07:19:10PM +0300, Laurent Pinchart wrote:
> > On Fri, May 01, 2020 at 05:16:51PM +0200, Jacopo Mondi wrote:
> > > Since the Linux kernel commit:
> > > 69e39d40587b ("media: vimc: Implement get/set selection in sink")
> > > the crop rectangle on the VIMC scaler sink pad needs to be
> > > reset to match the requested format to avoid hitting a pipeline format
> > > mis-configurations when the applied format is larger than the one
> >
> > s/mis-configurations/misconfiguration/
> >
> > > set in a previous capture session.
> > >
> > > As the V4L2 specification is not clear on what the correct behaviour
> > > is, if the crop rectangle should be reset automatically or not on a
> > > set_fmt operation, momentary fix the pipeline handler to please the
> > > driver and manually reset the crop rectangle.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > ---
> > >
> > > Tested on v5.6, please test on older kernel releases.
> >
> > I'll test it now.
>
> Tested-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com> # on v5.4.28
>
> I however get
>
> [5:10:58.023539356] [23523] ERROR V4L2 v4l2_subdevice.cpp:301 'Scaler': Unable to set rectangle 0 on pad 0: Inappropriate ioctl for device
>
> which I understand is expected as the driver doesn't support that ioctl
> yet.
>
> If you think it's worth it, you may want to squash

I'm debated... this is a pipeline specific fix, while we'll have the
same issue once we'll try to get crop rectangles to collect properties
from sensor.. In that case, as long as selection API support is not
mandatory, we will receive the same error message, while a less
worrying one might be more appropriate...

I was thinking of addressing this in the video device and subdevice
classes, and reduce the message level severity to INFO if -ENOTTY is
returned.

This would silence this error in VIMC as well, without requiring a
pipeline and version specific workaround..

What do you think ?

>
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 0b8a0a0c6890..0742b37c377a 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -12,6 +12,7 @@
>  #include <tuple>
>
>  #include <linux/media-bus-format.h>
> +#include <linux/version.h>
>
>  #include <ipa/ipa_interface.h>
>  #include <ipa/ipa_module_info.h>
> @@ -226,15 +227,17 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
>  	if (ret)
>  		return ret;
>
> -	Rectangle crop = {
> -		.x = 0,
> -		.y = 0,
> -		.width = subformat.size.width,
> -		.height = subformat.size.height,
> -	};
> -	ret = data->scaler_->setSelection(0, V4L2_SEL_TGT_CROP, &crop);
> -	if (ret && ret != -ENOTTY)
> -		return ret;
> +	if (data->media_->version() >= KERNEL_VERSION(5, 6, 0)) {
> +		Rectangle crop = {
> +			.x = 0,
> +			.y = 0,
> +			.width = subformat.size.width,
> +			.height = subformat.size.height,
> +		};
> +		ret = data->scaler_->setSelection(0, V4L2_SEL_TGT_CROP, &crop);
> +		if (ret)
> +			return ret;
> +	}
>
>  	subformat.size = cfg.size;
>  	ret = data->scaler_->setFormat(1, &subformat);
>
>
> with this patch, and base is on top of
>
> [PATCH 1/2] libcamera: media_device: Expose media device API version number
> [PATCH 2/2] libcamera: pipeline: vimc: Store media device pointer in camera data
>
> > > ---
> > >  src/libcamera/pipeline/vimc/vimc.cpp | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > index ccfd7f86d158..339d1cf653fb 100644
> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > @@ -224,6 +224,16 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> > >  	if (ret)
> > >  		return ret;
> > >
> > > +	Rectangle crop = {
> > > +		.x = 0,
> > > +		.y = 0,
> > > +		.width = subformat.size.width,
> > > +		.height = subformat.size.height,
> > > +	};
> > > +	ret = data->scaler_->setSelection(0, V4L2_SEL_TGT_CROP, &crop);
> > > +	if (ret && ret != -ENOTTY)
> > > +		return ret;
> > > +
> > >  	subformat.size = cfg.size;
> > >  	ret = data->scaler_->setFormat(1, &subformat);
> > >  	if (ret)
>
> --
> Regards,
>
> Laurent Pinchart