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

[Laurent Pinchart]

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

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