| Message ID | 20200501151651.864358-1-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2020-05-01 17:16:51 +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 > 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@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > > Tested on v5.6, please test on older kernel releases. > > --- > 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) > -- > 2.26.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. 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@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > > Tested on v5.6, please test on older kernel releases. I'll test it now. > > --- > 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)
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@jmondi.org> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > > Tested on v5.6, please test on older kernel releases. > > I'll test it now. Tested-by: Laurent Pinchart <laurent.pinchart@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)
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@jmondi.org> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > > > > Tested on v5.6, please test on older kernel releases. > > > > I'll test it now. > > Tested-by: Laurent Pinchart <laurent.pinchart@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
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@jmondi.org> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > --- > > > > > > > > Tested on v5.6, please test on older kernel releases. > > > > > > I'll test it now. > > > > Tested-by: Laurent Pinchart <laurent.pinchart@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)
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)
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 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@jmondi.org> --- Tested on v5.6, please test on older kernel releases. --- src/libcamera/pipeline/vimc/vimc.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) -- 2.26.1