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

[Laurent Pinchart]

Hi Jacopo,

On Fri, May 01, 2020 at 09:43:02PM +0200, Jacopo Mondi wrote:
> On Fri, May 01, 2020 at 07:43:56PM +0300, Laurent Pinchart wrote:
> > 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 ?

It's a good point. I think we'll need version checks in the future, but
I'm fine delaying the two base patches until they're needed, and
addressed this in the V4L2Subdevice class. We would then need to review
the users though, as what I really want to avoid is an error going
unnoticed without any message whatsoever.

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