Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1603846 > unrolled thread

Re: [PATCH v5 00/39] i.MX Media Driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-18 20:30 +0100
    Re: [PATCH v5 00/39] i.MX Media Driver Steve Longerbeam <steve_longerbeam@mentor.com> - 2017-03-18 21:00 +0100
      Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-18 21:50 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Nicolas Dufresne <nicolas@ndufresne.ca> - 2017-03-19 01:50 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 02:10 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 16:00 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Nicolas Dufresne <nicolas@ndufresne.ca> - 2017-03-19 16:10 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 11:00 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Nicolas Dufresne <nicolas@ndufresne.ca> - 2017-03-19 15:50 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2017-03-19 15:00 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 15:30 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2017-03-19 16:10 +0100
              Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 16:20 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 15:30 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Nicolas Dufresne <nicolas@ndufresne.ca> - 2017-03-19 15:50 +0100
      Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 11:50 +0100
        [PATCH 2/4] media: imx: allow bayer pixel formats to be looked up Russell King <rmk+kernel@armlinux.org.uk> - 2017-03-19 11:50 +0100
          Re: [PATCH 2/4] media: imx: allow bayer pixel formats to be looked up Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 23:20 +0100
        [PATCH 1/4] media: imx-media-csi: fix v4l2-compliance check Russell King <rmk+kernel@armlinux.org.uk> - 2017-03-19 11:50 +0100
          Re: [PATCH 1/4] media: imx-media-csi: fix v4l2-compliance check Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 23:20 +0100
        [PATCH 4/4] media: imx-media-capture: add frame sizes/interval  enumeration Russell King <rmk+kernel@armlinux.org.uk> - 2017-03-19 12:00 +0100
          Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval  enumeration Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 23:30 +0100
            Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval  enumeration Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 23:50 +0100
          Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval         enumeration Philippe De Muyter <phdm@macq.eu> - 2017-03-20 10:00 +0100
            Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval  enumeration Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 10:10 +0100
              Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval         enumeration Philippe De Muyter <phdm@macq.eu> - 2017-03-20 10:30 +0100
                Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval  enumeration Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 11:50 +0100
        [PATCH 3/4] media: imx-csi: add frame size/interval enumeration Russell King <rmk+kernel@armlinux.org.uk> - 2017-03-19 12:10 +0100
          Re: [PATCH 3/4] media: imx-csi: add frame size/interval enumeration Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 23:30 +0100
          Re: [PATCH 3/4] media: imx-csi: add frame size/interval enumeration Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-22 00:50 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 19:00 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 19:10 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-20 14:10 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 14:40 +0100
              Re: [PATCH v5 00/39] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-20 15:00 +0100
                Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 15:20 +0100
                  Re: [PATCH v5 00/39] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-20 17:10 +0100
                    Re: [PATCH v5 00/39] i.MX Media Driver "Niklas Söderlund" <niklas.soderlund@ragnatech.se> - 2017-03-21 11:50 +0100
                      Re: [PATCH v5 00/39] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-21 12:10 +0100
                        Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-21 12:50 +0100
                          Re: [PATCH v5 00/39] i.MX Media Driver Nicolas Dufresne <nicolas@ndufresne.ca> - 2017-03-22 19:20 +0100
      Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 13:20 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-19 20:10 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 21:00 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-03-19 21:00 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-03-20 14:00 +0100
        Re: [PATCH v5 00/39] i.MX Media Driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-03-20 14:30 +0100
          Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 16:50 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-20 17:40 +0100
            Re: [PATCH v5 00/39] i.MX Media Driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-03-20 17:40 +0100
      Re: [PATCH v5 00/39] i.MX Media Driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-03-20 14:20 +0100

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1603954 — [PATCH 4/4] media: imx-media-capture: add frame sizes/interval enumeration

Add support for enumerating frame sizes and frame intervals from the
first subdev via the V4L2 interfaces.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/staging/media/imx/imx-media-capture.c | 62 +++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index cdeb2cd8b1d7..bc99d9310e36 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -82,6 +82,65 @@ static int vidioc_querycap(struct file *file, void *fh,
 	return 0;
 }
 
+static int capture_enum_framesizes(struct file *file, void *fh,
+				   struct v4l2_frmsizeenum *fsize)
+{
+	struct capture_priv *priv = video_drvdata(file);
+	const struct imx_media_pixfmt *cc;
+	struct v4l2_subdev_frame_size_enum fse = {
+		.index = fsize->index,
+		.pad = priv->src_sd_pad,
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+	int ret;
+
+	cc = imx_media_find_format(fsize->pixel_format, CS_SEL_ANY, true);
+	if (!cc)
+		return -EINVAL;
+
+	fse.code = cc->codes[0];
+
+	ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
+	if (ret)
+		return ret;
+
+	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
+	fsize->discrete.width = fse.min_width;
+	fsize->discrete.height = fse.max_height;
+
+	return 0;
+}
+
+static int capture_enum_frameintervals(struct file *file, void *fh,
+				       struct v4l2_frmivalenum *fival)
+{
+	struct capture_priv *priv = video_drvdata(file);
+	const struct imx_media_pixfmt *cc;
+	struct v4l2_subdev_frame_interval_enum fie = {
+		.index = fival->index,
+		.pad = priv->src_sd_pad,
+		.width = fival->width,
+		.height = fival->height,
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+	int ret;
+
+	cc = imx_media_find_format(fival->pixel_format, CS_SEL_ANY, true);
+	if (!cc)
+		return -EINVAL;
+
+	fie.code = cc->codes[0];
+
+	ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval, NULL, &fie);
+	if (ret)
+		return ret;
+
+	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
+	fival->discrete = fie.interval;
+
+	return 0;
+}
+
 static int capture_enum_fmt_vid_cap(struct file *file, void *fh,
 				    struct v4l2_fmtdesc *f)
 {
@@ -270,6 +329,9 @@ static int capture_s_parm(struct file *file, void *fh,
 static const struct v4l2_ioctl_ops capture_ioctl_ops = {
 	.vidioc_querycap	= vidioc_querycap,
 
+	.vidioc_enum_framesizes = capture_enum_framesizes,
+	.vidioc_enum_frameintervals = capture_enum_frameintervals,
+
 	.vidioc_enum_fmt_vid_cap        = capture_enum_fmt_vid_cap,
 	.vidioc_g_fmt_vid_cap           = capture_g_fmt_vid_cap,
 	.vidioc_try_fmt_vid_cap         = capture_try_fmt_vid_cap,
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1604102 — Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval enumeration

On 03/19/2017 03:49 AM, Russell King wrote:
> Add support for enumerating frame sizes and frame intervals from the
> first subdev via the V4L2 interfaces.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>   drivers/staging/media/imx/imx-media-capture.c | 62 +++++++++++++++++++++++++++
>   1 file changed, 62 insertions(+)
>
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index cdeb2cd8b1d7..bc99d9310e36 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -82,6 +82,65 @@ static int vidioc_querycap(struct file *file, void *fh,
>   	return 0;
>   }
>   
> +static int capture_enum_framesizes(struct file *file, void *fh,
> +				   struct v4l2_frmsizeenum *fsize)
> +{
> +	struct capture_priv *priv = video_drvdata(file);
> +	const struct imx_media_pixfmt *cc;
> +	struct v4l2_subdev_frame_size_enum fse = {
> +		.index = fsize->index,
> +		.pad = priv->src_sd_pad,
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +	};
> +	int ret;
> +
> +	cc = imx_media_find_format(fsize->pixel_format, CS_SEL_ANY, true);
> +	if (!cc)
> +		return -EINVAL;
> +
> +	fse.code = cc->codes[0];
> +
> +	ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
> +	if (ret)
> +		return ret;
> +
> +	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> +	fsize->discrete.width = fse.min_width;
> +	fsize->discrete.height = fse.max_height;
> +
> +	return 0;
> +}


The PRP ENC/VF subdevices will return a continuous range of
supported frame sizes at their source pad, so this should be
modified to:

...
     if (fse.min_width == fse.max_width &&
         fse.min_height == fse.max_height) {
         fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
         fsize->discrete.width = fse.min_width;
         fsize->discrete.height = fse.min_height;
     } else {
         fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
         fsize->stepwise.min_width = fse.min_width;
         fsize->stepwise.max_width = fse.max_width;
         fsize->stepwise.min_height = fse.min_height;
         fsize->stepwise.max_height = fse.max_height;
         fsize->stepwise.step_width = 1;
         fsize->stepwise.step_height = 1;
     }
...

Steve


> +
> +static int capture_enum_frameintervals(struct file *file, void *fh,
> +				       struct v4l2_frmivalenum *fival)
> +{
> +	struct capture_priv *priv = video_drvdata(file);
> +	const struct imx_media_pixfmt *cc;
> +	struct v4l2_subdev_frame_interval_enum fie = {
> +		.index = fival->index,
> +		.pad = priv->src_sd_pad,
> +		.width = fival->width,
> +		.height = fival->height,
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +	};
> +	int ret;
> +
> +	cc = imx_media_find_format(fival->pixel_format, CS_SEL_ANY, true);
> +	if (!cc)
> +		return -EINVAL;
> +
> +	fie.code = cc->codes[0];
> +
> +	ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval, NULL, &fie);
> +	if (ret)
> +		return ret;
> +
> +	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> +	fival->discrete = fie.interval;
> +
> +	return 0;
> +}
> +
>   static int capture_enum_fmt_vid_cap(struct file *file, void *fh,
>   				    struct v4l2_fmtdesc *f)
>   {
> @@ -270,6 +329,9 @@ static int capture_s_parm(struct file *file, void *fh,
>   static const struct v4l2_ioctl_ops capture_ioctl_ops = {
>   	.vidioc_querycap	= vidioc_querycap,
>   
> +	.vidioc_enum_framesizes = capture_enum_framesizes,
> +	.vidioc_enum_frameintervals = capture_enum_frameintervals,
> +
>   	.vidioc_enum_fmt_vid_cap        = capture_enum_fmt_vid_cap,
>   	.vidioc_g_fmt_vid_cap           = capture_g_fmt_vid_cap,
>   	.vidioc_try_fmt_vid_cap         = capture_try_fmt_vid_cap,

[toc] | [prev] | [next] | [standalone]

#1604104 — Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval enumeration

On Sun, Mar 19, 2017 at 03:21:37PM -0700, Steve Longerbeam wrote:
> 
> 
> On 03/19/2017 03:49 AM, Russell King wrote:
> >Add support for enumerating frame sizes and frame intervals from the
> >first subdev via the V4L2 interfaces.
> >
> >Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> >---
> >  drivers/staging/media/imx/imx-media-capture.c | 62 +++++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> >
> >diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> >index cdeb2cd8b1d7..bc99d9310e36 100644
> >--- a/drivers/staging/media/imx/imx-media-capture.c
> >+++ b/drivers/staging/media/imx/imx-media-capture.c
> >@@ -82,6 +82,65 @@ static int vidioc_querycap(struct file *file, void *fh,
> >  	return 0;
> >  }
> >+static int capture_enum_framesizes(struct file *file, void *fh,
> >+				   struct v4l2_frmsizeenum *fsize)
> >+{
> >+	struct capture_priv *priv = video_drvdata(file);
> >+	const struct imx_media_pixfmt *cc;
> >+	struct v4l2_subdev_frame_size_enum fse = {
> >+		.index = fsize->index,
> >+		.pad = priv->src_sd_pad,
> >+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >+	};
> >+	int ret;
> >+
> >+	cc = imx_media_find_format(fsize->pixel_format, CS_SEL_ANY, true);
> >+	if (!cc)
> >+		return -EINVAL;
> >+
> >+	fse.code = cc->codes[0];
> >+
> >+	ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
> >+	if (ret)
> >+		return ret;
> >+
> >+	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> >+	fsize->discrete.width = fse.min_width;
> >+	fsize->discrete.height = fse.max_height;
> >+
> >+	return 0;
> >+}
> 
> 
> The PRP ENC/VF subdevices will return a continuous range of
> supported frame sizes at their source pad, so this should be
> modified to:
> 
> ...
>     if (fse.min_width == fse.max_width &&
>         fse.min_height == fse.max_height) {
>         fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
>         fsize->discrete.width = fse.min_width;
>         fsize->discrete.height = fse.min_height;
>     } else {
>         fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
>         fsize->stepwise.min_width = fse.min_width;
>         fsize->stepwise.max_width = fse.max_width;
>         fsize->stepwise.min_height = fse.min_height;
>         fsize->stepwise.max_height = fse.max_height;
>         fsize->stepwise.step_width = 1;
>         fsize->stepwise.step_height = 1;
>     }
> ...

Fine by me - I don't have any experience of those subdevices as they're
unusable for me.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1604252 — Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval enumeration

Hi Russel,

On Sun, Mar 19, 2017 at 10:49:08AM +0000, Russell King wrote:
> Add support for enumerating frame sizes and frame intervals from the
> first subdev via the V4L2 interfaces.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/staging/media/imx/imx-media-capture.c | 62 +++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
...
> +static int capture_enum_frameintervals(struct file *file, void *fh,
> +				       struct v4l2_frmivalenum *fival)
> +{
> +	struct capture_priv *priv = video_drvdata(file);
> +	const struct imx_media_pixfmt *cc;
> +	struct v4l2_subdev_frame_interval_enum fie = {
> +		.index = fival->index,
> +		.pad = priv->src_sd_pad,
> +		.width = fival->width,
> +		.height = fival->height,
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +	};
> +	int ret;
> +
> +	cc = imx_media_find_format(fival->pixel_format, CS_SEL_ANY, true);
> +	if (!cc)
> +		return -EINVAL;
> +
> +	fie.code = cc->codes[0];
> +
> +	ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval, NULL, &fie);
> +	if (ret)
> +		return ret;
> +
> +	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> +	fival->discrete = fie.interval;

For some parallel sensors (mine is a E2V ev76c560) "any" frame interval is possible,
and hence type should be V4L2_FRMIVAL_TYPE_CONTINUOUS.

see also https://www.spinics.net/lists/linux-media/msg98622.html,
https://patchwork.kernel.org/patch/9171201/ and
https://patchwork.kernel.org/patch/9171199/

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

[toc] | [prev] | [next] | [standalone]

#1604262 — Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval enumeration

On Mon, Mar 20, 2017 at 09:55:12AM +0100, Philippe De Muyter wrote:
> Hi Russel,
> 
> On Sun, Mar 19, 2017 at 10:49:08AM +0000, Russell King wrote:
> > Add support for enumerating frame sizes and frame intervals from the
> > first subdev via the V4L2 interfaces.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/staging/media/imx/imx-media-capture.c | 62 +++++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> > 
> ...
> > +static int capture_enum_frameintervals(struct file *file, void *fh,
> > +				       struct v4l2_frmivalenum *fival)
> > +{
> > +	struct capture_priv *priv = video_drvdata(file);
> > +	const struct imx_media_pixfmt *cc;
> > +	struct v4l2_subdev_frame_interval_enum fie = {
> > +		.index = fival->index,
> > +		.pad = priv->src_sd_pad,
> > +		.width = fival->width,
> > +		.height = fival->height,
> > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +	};
> > +	int ret;
> > +
> > +	cc = imx_media_find_format(fival->pixel_format, CS_SEL_ANY, true);
> > +	if (!cc)
> > +		return -EINVAL;
> > +
> > +	fie.code = cc->codes[0];
> > +
> > +	ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval, NULL, &fie);
> > +	if (ret)
> > +		return ret;
> > +
> > +	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> > +	fival->discrete = fie.interval;
> 
> For some parallel sensors (mine is a E2V ev76c560) "any" frame interval is possible,
> and hence type should be V4L2_FRMIVAL_TYPE_CONTINUOUS.

For my sensor, any frame interval is also possible, but that isn't the
point here.

/dev/video* only talks to the CSI source pad, not it's sink pad.  The
sink pad gets configured with the sensor frame rate via the media
controller API.  /dev/video* itself has no control over the sensor
frame rate.

The media controller stuff completely changes the way the established
/dev/video* functionality works - the ability to select arbitary frame
sizes and frame rates supported by the ultimate sensor is gone.  All
that needs to be setup through the media controller pipeline, one
subdev at a time.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1604274 — Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval enumeration

On Mon, Mar 20, 2017 at 09:05:25AM +0000, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 09:55:12AM +0100, Philippe De Muyter wrote:
> > Hi Russel,
> > 
> > On Sun, Mar 19, 2017 at 10:49:08AM +0000, Russell King wrote:
> > > Add support for enumerating frame sizes and frame intervals from the
> > > first subdev via the V4L2 interfaces.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  drivers/staging/media/imx/imx-media-capture.c | 62 +++++++++++++++++++++++++++
> > >  1 file changed, 62 insertions(+)
> > > 
> > ...
> > > +static int capture_enum_frameintervals(struct file *file, void *fh,
> > > +				       struct v4l2_frmivalenum *fival)
> > > +{
> > > +	struct capture_priv *priv = video_drvdata(file);
> > > +	const struct imx_media_pixfmt *cc;
> > > +	struct v4l2_subdev_frame_interval_enum fie = {
> > > +		.index = fival->index,
> > > +		.pad = priv->src_sd_pad,
> > > +		.width = fival->width,
> > > +		.height = fival->height,
> > > +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > > +	};
> > > +	int ret;
> > > +
> > > +	cc = imx_media_find_format(fival->pixel_format, CS_SEL_ANY, true);
> > > +	if (!cc)
> > > +		return -EINVAL;
> > > +
> > > +	fie.code = cc->codes[0];
> > > +
> > > +	ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval, NULL, &fie);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> > > +	fival->discrete = fie.interval;
> > 
> > For some parallel sensors (mine is a E2V ev76c560) "any" frame interval is possible,
> > and hence type should be V4L2_FRMIVAL_TYPE_CONTINUOUS.
> 
> For my sensor, any frame interval is also possible, but that isn't the
> point here.
> 
> /dev/video* only talks to the CSI source pad, not it's sink pad.  The
> sink pad gets configured with the sensor frame rate via the media
> controller API.  /dev/video* itself has no control over the sensor
> frame rate.
> 
> The media controller stuff completely changes the way the established
> /dev/video* functionality works - the ability to select arbitary frame
> sizes and frame rates supported by the ultimate sensor is gone.  All
> that needs to be setup through the media controller pipeline, one
> subdev at a time.
> 
So existing gstreamer applications using /dev/video* to control framerate,
and even gain and exposure won't work anymore :( ?

I had hoped to keep compatibility, with added robustness and functionality.

I seems like I'll stay with my NXP/Freescale old and imperfect kernel.

Best regards

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

[toc] | [prev] | [next] | [standalone]

#1604363 — Re: [PATCH 4/4] media: imx-media-capture: add frame sizes/interval enumeration

On Mon, Mar 20, 2017 at 10:23:30AM +0100, Philippe De Muyter wrote:
> So existing gstreamer applications using /dev/video* to control framerate,
> and even gain and exposure won't work anymore :( ?
> 
> I had hoped to keep compatibility, with added robustness and functionality.
> 
> I seems like I'll stay with my NXP/Freescale old and imperfect kernel.

Thank you for saying this, this supports my views which I've already
stated about what influences which kernel people will use.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1603960 — [PATCH 3/4] media: imx-csi: add frame size/interval enumeration

Add frame size and frame interval enumeration to CSI.

CSI can scale the image independently horizontally and vertically by a
factor of two, which enumerates to four different frame sizes.

CSI can also drop frames, resulting in frame rate reduction, so
enumerate the resulting possible output frame rates.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/staging/media/imx/imx-media-csi.c | 51 +++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 65346e789dd6..d19659d7ddc2 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1073,6 +1073,55 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static int csi_enum_frame_size(struct v4l2_subdev *sd,
+			       struct v4l2_subdev_pad_config *cfg,
+			       struct v4l2_subdev_frame_size_enum *fse)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	struct v4l2_mbus_framefmt *fmt;
+	int ret = 0;
+
+	if (fse->pad >= CSI_NUM_PADS ||
+	    fse->index > (fse->pad == CSI_SINK_PAD ? 0 : 3))
+		return -EINVAL;
+
+	mutex_lock(&priv->lock);
+	fmt = __csi_get_fmt(priv, cfg, fse->pad, fse->which);
+	fse->min_width = fse->max_width = fse->index & 1 ?
+					    fmt->width >> 1 : fmt->width;
+	fse->min_height = fse->max_height = fse->index & 2 ?
+					      fmt->height >> 1 : fmt->height;
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
+static int csi_enum_frame_interval(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_pad_config *cfg,
+				   struct v4l2_subdev_frame_interval_enum *fie)
+{
+	struct csi_priv *priv = v4l2_get_subdevdata(sd);
+	struct v4l2_mbus_framefmt *fmt;
+	int ret = 0;
+
+	if (fie->pad >= CSI_NUM_PADS ||
+	    fie->index >= (fie->pad == CSI_SINK_PAD ? 1 : ARRAY_SIZE(csi_skip)))
+		return -EINVAL;
+
+	mutex_lock(&priv->lock);
+	fmt = __csi_get_fmt(priv, cfg, fie->pad, fie->which);
+	if ((fie->width == fmt->width || fie->width == fmt->width / 2) &&
+	    (fie->height == fmt->height || fie->height == fmt->height / 2)) {
+		fie->interval = priv->frame_interval;
+		csi_apply_skip_interval(&csi_skip[fie->index], &fie->interval);
+	} else {
+		ret = -EINVAL;
+	}
+	mutex_unlock(&priv->lock);
+
+	return ret;
+}
+
 static int csi_get_fmt(struct v4l2_subdev *sd,
 		       struct v4l2_subdev_pad_config *cfg,
 		       struct v4l2_subdev_format *sdformat)
@@ -1473,6 +1522,8 @@ static struct v4l2_subdev_video_ops csi_video_ops = {
 
 static struct v4l2_subdev_pad_ops csi_pad_ops = {
 	.enum_mbus_code = csi_enum_mbus_code,
+	.enum_frame_size = csi_enum_frame_size,
+	.enum_frame_interval = csi_enum_frame_interval,
 	.get_fmt = csi_get_fmt,
 	.set_fmt = csi_set_fmt,
 	.get_selection = csi_get_selection,
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1604101 — Re: [PATCH 3/4] media: imx-csi: add frame size/interval enumeration

On 03/19/2017 03:49 AM, Russell King wrote:
> Add frame size and frame interval enumeration to CSI.
>
> CSI can scale the image independently horizontally and vertically by a
> factor of two, which enumerates to four different frame sizes.
>
> CSI can also drop frames, resulting in frame rate reduction, so
> enumerate the resulting possible output frame rates.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>   drivers/staging/media/imx/imx-media-csi.c | 51 +++++++++++++++++++++++++++++++
>   1 file changed, 51 insertions(+)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 65346e789dd6..d19659d7ddc2 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1073,6 +1073,55 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd,
>   	return ret;
>   }
>   
> +static int csi_enum_frame_size(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_pad_config *cfg,
> +			       struct v4l2_subdev_frame_size_enum *fse)
> +{
> +	struct csi_priv *priv = v4l2_get_subdevdata(sd);
> +	struct v4l2_mbus_framefmt *fmt;
> +	int ret = 0;
> +
> +	if (fse->pad >= CSI_NUM_PADS ||
> +	    fse->index > (fse->pad == CSI_SINK_PAD ? 0 : 3))
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->lock);
> +	fmt = __csi_get_fmt(priv, cfg, fse->pad, fse->which);
> +	fse->min_width = fse->max_width = fse->index & 1 ?
> +					    fmt->width >> 1 : fmt->width;
> +	fse->min_height = fse->max_height = fse->index & 2 ?
> +					      fmt->height >> 1 : fmt->height;
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}

This will only enumerate the currently set/tried format at the sink
pad. But the sink pad supports a range of widths/heights.

I think this function should call csi_try_fmt() at the sink pad.

The other media drivers do the same, they return a supported range at
the sinks rather than only the last set/tried values.

So the sink enumeration should return a continuous range.
The source enumeration should behave the same as you have it above
however, i.e. a discrete set of 4 possibilities since the CSI is limited to
only /2 downsizing in both dimensions.

Steve

> +
> +static int csi_enum_frame_interval(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_pad_config *cfg,
> +				   struct v4l2_subdev_frame_interval_enum *fie)
> +{
> +	struct csi_priv *priv = v4l2_get_subdevdata(sd);
> +	struct v4l2_mbus_framefmt *fmt;
> +	int ret = 0;
> +
> +	if (fie->pad >= CSI_NUM_PADS ||
> +	    fie->index >= (fie->pad == CSI_SINK_PAD ? 1 : ARRAY_SIZE(csi_skip)))
> +		return -EINVAL;
> +
> +	mutex_lock(&priv->lock);
> +	fmt = __csi_get_fmt(priv, cfg, fie->pad, fie->which);
> +	if ((fie->width == fmt->width || fie->width == fmt->width / 2) &&
> +	    (fie->height == fmt->height || fie->height == fmt->height / 2)) {
> +		fie->interval = priv->frame_interval;
> +		csi_apply_skip_interval(&csi_skip[fie->index], &fie->interval);
> +	} else {
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
> +}
> +
>   static int csi_get_fmt(struct v4l2_subdev *sd,
>   		       struct v4l2_subdev_pad_config *cfg,
>   		       struct v4l2_subdev_format *sdformat)
> @@ -1473,6 +1522,8 @@ static struct v4l2_subdev_video_ops csi_video_ops = {
>   
>   static struct v4l2_subdev_pad_ops csi_pad_ops = {
>   	.enum_mbus_code = csi_enum_mbus_code,
> +	.enum_frame_size = csi_enum_frame_size,
> +	.enum_frame_interval = csi_enum_frame_interval,
>   	.get_fmt = csi_get_fmt,
>   	.set_fmt = csi_set_fmt,
>   	.get_selection = csi_get_selection,

[toc] | [prev] | [next] | [standalone]

#1606088 — Re: [PATCH 3/4] media: imx-csi: add frame size/interval enumeration

[Multipart message — attachments visible in raw view] — view raw

Hi Russell,

On 03/19/2017 03:49 AM, Russell King wrote:
> Add frame size and frame interval enumeration to CSI.
>
> CSI can scale the image independently horizontally and vertically by a
> factor of two, which enumerates to four different frame sizes.
>
> CSI can also drop frames, resulting in frame rate reduction, so
> enumerate the resulting possible output frame rates.
>

I applied this patch, modified to return a frame size range on the
sink pad. Also, I believe both frame size and frame interval
enumeration should base their decision making on the crop rectangle
at the sink pad, not on the format at the source pads, due to the
crop/compose re-org patch from Philipp.

The updated patch is attached.

Steve

[toc] | [prev] | [next] | [standalone]

#1604060

On 03/19/2017 03:38 AM, Russell King - ARM Linux wrote:
> On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
>> Right, imx-media-capture.c (the "standard" v4l2 user interface module)
>> is not implementing VIDIOC_ENUM_FRAMESIZES. It should, but it can only
>> return the single frame size that the pipeline has configured (the mbus
>> format of the attached source pad).
> I now have a set of patches that enumerate the frame sizes and intervals
> from the source pad of the first subdev (since you're setting the formats
> etc there from the capture device, it seems sensible to return what it
> can support.)  This means my patch set doesn't add to non-CSI subdevs.
>
>> Can you share your gstreamer pipeline? For now, until
>> VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
>> does not attempt to specify a frame rate. I use the attached
>> script for testing, which works for me.
> Note that I'm not specifying a frame rate on gstreamer - I'm setting
> the pipeline up for 60fps, but gstreamer in its wisdom is unable to
> enumerate the frame sizes, and therefore is unable to enumerate the
> frame intervals (frame intervals depend on frame sizes), so it
> falls back to the "tvnorms" which are basically 25/1 and 30000/1001.
>
> It sees 60fps via G_PARM, and then decides to set 30000/1001 via S_PARM.
> So, we end up with most of the pipeline operating at 60fps, with CSI
> doing frame skipping to reduce the frame rate to 30fps.
>
> gstreamer doesn't complain, doesn't issue any warnings, the only way
> you can spot this is to enable debugging and look through the copious
> debug log, or use -v and check the pad capabilities.
>
> Testing using gstreamer, and only using "does it produce video" is a
> good simple test, but it's just that - it's a simple test.  It doesn't
> tell you that what you're seeing is what you intended to see (such as
> video at the frame rate you expected) without more work.
>
>> Thanks, I've fixed most of v4l2-compliance issues, but this is not
>> done yet. Is that something you can help with?
> What did you do with:
>
> ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, memory=0 /* V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
>                  test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> ioctl(3, VIDIOC_EXPBUF, 0xbef405bc)     = -1 EINVAL (Invalid argument)
>                  fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)
>                  test VIDIOC_EXPBUF: FAIL
>
> To me, this looks like a bug in v4l2-compliance (I'm using 1.10.0).
> I'm not sure what buffer VIDIOC_EXPBUF is expected to export, since
> afaics no buffers have been allocated, so of course it's going to fail.
> Either that, or the v4l2 core vb2 code is non-compliant with v4l2's
> interface requirements.
>
> In any case, it doesn't look like the buffer management is being
> tested at all by v4l2-compliance - we know that gstreamer works, so
> buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
> so I also know that VIDIOC_EXPBUF works there.
>

I wouldn't be surprised if you hit on a bug in v4l2-compliance. I 
stopped with v4l2-compliance
at a different test failure that also didn't make sense to me:

Streaming ioctls:
     test read/write: OK (Not Supported)
         Video Capture:
         Buffer: 0 Sequence: 0 Field: Any Timestamp: 41.664259s
         fail: 
.../v4l-utils-1.6.2/utils/v4l2-compliance/v4l2-test-buffers.cpp(281): 
!(g_flags() & (V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR))
         fail: 
.../v4l-utils-1.6.2/utils/v4l2-compliance/v4l2-test-buffers.cpp(610): 
buf.check(q, last_seq)
         fail: 
.../v4l-utils-1.6.2/utils/v4l2-compliance/v4l2-test-buffers.cpp(883): 
captureBufs(node, q, m2m_q, frame_count, false)
     test MMAP: FAIL
     test USERPTR: OK (Not Supported)
     test DMABUF: Cannot test, specify --expbuf-device

Total: 42, Succeeded: 38, Failed: 4, Warnings: 0


In this case the driver completed and returned only one buffer, and it set
VB2_BUF_STATE_DONE, so these test failures didn't make sense to me. I
was using version 1.6.2 at the time.

Steve

[toc] | [prev] | [next] | [standalone]

#1604064

On Sun, Mar 19, 2017 at 10:54:22AM -0700, Steve Longerbeam wrote:
> 
> 
> On 03/19/2017 03:38 AM, Russell King - ARM Linux wrote:
> >On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
> >>Right, imx-media-capture.c (the "standard" v4l2 user interface module)
> >>is not implementing VIDIOC_ENUM_FRAMESIZES. It should, but it can only
> >>return the single frame size that the pipeline has configured (the mbus
> >>format of the attached source pad).
> >I now have a set of patches that enumerate the frame sizes and intervals
> >from the source pad of the first subdev (since you're setting the formats
> >etc there from the capture device, it seems sensible to return what it
> >can support.)  This means my patch set doesn't add to non-CSI subdevs.
> >
> >>Can you share your gstreamer pipeline? For now, until
> >>VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
> >>does not attempt to specify a frame rate. I use the attached
> >>script for testing, which works for me.
> >Note that I'm not specifying a frame rate on gstreamer - I'm setting
> >the pipeline up for 60fps, but gstreamer in its wisdom is unable to
> >enumerate the frame sizes, and therefore is unable to enumerate the
> >frame intervals (frame intervals depend on frame sizes), so it
> >falls back to the "tvnorms" which are basically 25/1 and 30000/1001.
> >
> >It sees 60fps via G_PARM, and then decides to set 30000/1001 via S_PARM.
> >So, we end up with most of the pipeline operating at 60fps, with CSI
> >doing frame skipping to reduce the frame rate to 30fps.
> >
> >gstreamer doesn't complain, doesn't issue any warnings, the only way
> >you can spot this is to enable debugging and look through the copious
> >debug log, or use -v and check the pad capabilities.
> >
> >Testing using gstreamer, and only using "does it produce video" is a
> >good simple test, but it's just that - it's a simple test.  It doesn't
> >tell you that what you're seeing is what you intended to see (such as
> >video at the frame rate you expected) without more work.
> >
> >>Thanks, I've fixed most of v4l2-compliance issues, but this is not
> >>done yet. Is that something you can help with?
> >What did you do with:
> >
> >ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, memory=0 /* V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
> >                 test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> >ioctl(3, VIDIOC_EXPBUF, 0xbef405bc)     = -1 EINVAL (Invalid argument)
> >                 fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)
> >                 test VIDIOC_EXPBUF: FAIL
> >
> >To me, this looks like a bug in v4l2-compliance (I'm using 1.10.0).
> >I'm not sure what buffer VIDIOC_EXPBUF is expected to export, since
> >afaics no buffers have been allocated, so of course it's going to fail.
> >Either that, or the v4l2 core vb2 code is non-compliant with v4l2's
> >interface requirements.
> >
> >In any case, it doesn't look like the buffer management is being
> >tested at all by v4l2-compliance - we know that gstreamer works, so
> >buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
> >so I also know that VIDIOC_EXPBUF works there.
> >
> 
> I wouldn't be surprised if you hit on a bug in v4l2-compliance. I stopped
> with v4l2-compliance
> at a different test failure that also didn't make sense to me:

It isn't - the problem is that the results are misleading.  The
VIDIOC_REQBUFS depends on the GET_FMT test succeeding, so it knows
which buffer formats are valid.

Since the GET_FMT test fails due to the colorspace issue, it decides
that it can't trust the format, so it ends up with no formats to test.
This causes the "VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF" test to pass,
but then it moves on to testing "VIDIOC_EXPBUF" with no available
buffers, which then fails.

Fixing GET_FMT (which I've done locally) to return proper colorspace
information results in GET_FMT passing, and also solves the EXPBUF
problem too.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1604543

On 03/19/2017 06:54 PM, Steve Longerbeam wrote:
> 
> 
> On 03/19/2017 03:38 AM, Russell King - ARM Linux wrote:
>> On Sat, Mar 18, 2017 at 12:58:27PM -0700, Steve Longerbeam wrote:
>>> Right, imx-media-capture.c (the "standard" v4l2 user interface module)
>>> is not implementing VIDIOC_ENUM_FRAMESIZES. It should, but it can only
>>> return the single frame size that the pipeline has configured (the mbus
>>> format of the attached source pad).
>> I now have a set of patches that enumerate the frame sizes and intervals
>> from the source pad of the first subdev (since you're setting the formats
>> etc there from the capture device, it seems sensible to return what it
>> can support.)  This means my patch set doesn't add to non-CSI subdevs.
>>
>>> Can you share your gstreamer pipeline? For now, until
>>> VIDIOC_ENUM_FRAMESIZES is implemented, try a pipeline that
>>> does not attempt to specify a frame rate. I use the attached
>>> script for testing, which works for me.
>> Note that I'm not specifying a frame rate on gstreamer - I'm setting
>> the pipeline up for 60fps, but gstreamer in its wisdom is unable to
>> enumerate the frame sizes, and therefore is unable to enumerate the
>> frame intervals (frame intervals depend on frame sizes), so it
>> falls back to the "tvnorms" which are basically 25/1 and 30000/1001.
>>
>> It sees 60fps via G_PARM, and then decides to set 30000/1001 via S_PARM.
>> So, we end up with most of the pipeline operating at 60fps, with CSI
>> doing frame skipping to reduce the frame rate to 30fps.
>>
>> gstreamer doesn't complain, doesn't issue any warnings, the only way
>> you can spot this is to enable debugging and look through the copious
>> debug log, or use -v and check the pad capabilities.
>>
>> Testing using gstreamer, and only using "does it produce video" is a
>> good simple test, but it's just that - it's a simple test.  It doesn't
>> tell you that what you're seeing is what you intended to see (such as
>> video at the frame rate you expected) without more work.
>>
>>> Thanks, I've fixed most of v4l2-compliance issues, but this is not
>>> done yet. Is that something you can help with?
>> What did you do with:
>>
>> ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, memory=0 /* V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
>>                  test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>> ioctl(3, VIDIOC_EXPBUF, 0xbef405bc)     = -1 EINVAL (Invalid argument)
>>                  fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)

This is really a knock-on effect from an earlier issue where the compliance test
didn't detect support for MEMORY_MMAP.

>>                  test VIDIOC_EXPBUF: FAIL
>>
>> To me, this looks like a bug in v4l2-compliance (I'm using 1.10.0).

Always build from the master repo. 1.10 is pretty old.

>> I'm not sure what buffer VIDIOC_EXPBUF is expected to export, since
>> afaics no buffers have been allocated, so of course it's going to fail.

It just tests if EXPBUF is supported.

I think I will modify v4l2-compliance to bail out if it doesn't find support
for MEMORY_MMAP. Even though in theory support for this is optional, in practice
all applications expect that it is supported. That should fix this
hard-to-understand error.

>> Either that, or the v4l2 core vb2 code is non-compliant with v4l2's
>> interface requirements.
>>
>> In any case, it doesn't look like the buffer management is being
>> tested at all by v4l2-compliance - we know that gstreamer works, so
>> buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
>> so I also know that VIDIOC_EXPBUF works there.

To test actual streaming you need to provide the -s option.

Note: v4l2-compliance has been developed for 'regular' video devices,
not MC devices. It may or may not work with the -s option.

As I think I mentioned somewhere else, creating a compliance test for
MC devices would help enormously in verifying drivers. I'm not sure if
it is better to create a new test or integrate it in v4l2-compliance.

I'm leaning towards the latter since there is a lot of overlap.

>>
> 
> I wouldn't be surprised if you hit on a bug in v4l2-compliance. I 
> stopped with v4l2-compliance
> at a different test failure that also didn't make sense to me:
> 
> Streaming ioctls:
>      test read/write: OK (Not Supported)
>          Video Capture:
>          Buffer: 0 Sequence: 0 Field: Any Timestamp: 41.664259s
>          fail: 
> .../v4l-utils-1.6.2/utils/v4l2-compliance/v4l2-test-buffers.cpp(281): 
> !(g_flags() & (V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR))
>          fail: 
> .../v4l-utils-1.6.2/utils/v4l2-compliance/v4l2-test-buffers.cpp(610): 
> buf.check(q, last_seq)
>          fail: 
> .../v4l-utils-1.6.2/utils/v4l2-compliance/v4l2-test-buffers.cpp(883): 
> captureBufs(node, q, m2m_q, frame_count, false)
>      test MMAP: FAIL
>      test USERPTR: OK (Not Supported)
>      test DMABUF: Cannot test, specify --expbuf-device
> 
> Total: 42, Succeeded: 38, Failed: 4, Warnings: 0
> 
> 
> In this case the driver completed and returned only one buffer, and it set
> VB2_BUF_STATE_DONE, so these test failures didn't make sense to me. I
> was using version 1.6.2 at the time.

I can't do anything with that. Always use the master branch in the v4l-utils
repo.

Regards,

	Hans

[toc] | [prev] | [next] | [standalone]

#1604573

On Mon, Mar 20, 2017 at 02:01:58PM +0100, Hans Verkuil wrote:
> On 03/19/2017 06:54 PM, Steve Longerbeam wrote:
> > 
> > 
> > On 03/19/2017 03:38 AM, Russell King - ARM Linux wrote:
> >> What did you do with:
> >>
> >> ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, memory=0 /* V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
> >>                  test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> >> ioctl(3, VIDIOC_EXPBUF, 0xbef405bc)     = -1 EINVAL (Invalid argument)
> >>                  fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)
> 
> This is really a knock-on effect from an earlier issue where the compliance test
> didn't detect support for MEMORY_MMAP.

So why does it succeed when I fix the compliance errors with VIDIOC_G_FMT?
With that fixed, I now get:

        Format ioctls:
                test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
                test VIDIOC_G/S_PARM: OK
                test VIDIOC_G_FBUF: OK (Not Supported)
                test VIDIOC_G_FMT: OK
                test VIDIOC_TRY_FMT: OK
                test VIDIOC_S_FMT: OK
                test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
                test Cropping: OK (Not Supported)
                test Composing: OK (Not Supported)
                test Scaling: OK (Not Supported)

        Buffer ioctls:
                test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
                test VIDIOC_EXPBUF: OK

The reason is, if you look at the code, VIDIOC_G_FMT populates a list
of possible buffer formats "node->valid_buftypes".  If the VIDIOC_G_FMT
test fails, then node->valid_buftypes is zero.

This causes testReqBufs() to only check for the all-zeroed VIDIOC_REQBUFS
and declare it conformant, without creating any buffers (it can't, it
doesn't know which formats are supported.)

This causes node->valid_memorytype to be zero.

We then go on to testExpBuf(), and valid_memorytype zero, claiming (falsely)
that MMAP is not supported.  The reality is that it _is_ supported, but
it's just the non-compliant VICIOC_G_FMT call (due to the colorspace
issue) causes the sequence of tests to fail.

> Always build from the master repo. 1.10 is pretty old.

It's what I have - remember, not everyone is happy to constantly replace
their distro packages with random new stuff.

> >> In any case, it doesn't look like the buffer management is being
> >> tested at all by v4l2-compliance - we know that gstreamer works, so
> >> buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
> >> so I also know that VIDIOC_EXPBUF works there.
> 
> To test actual streaming you need to provide the -s option.
> 
> Note: v4l2-compliance has been developed for 'regular' video devices,
> not MC devices. It may or may not work with the -s option.

Right, and it exists to verify that the establised v4l2 API is correctly
implemented.  If the v4l2 API is being offered to user applications,
then it must be conformant, otherwise it's not offering the v4l2 API.
(That's very much a definition statement in itself.)

So, are we really going to say MC devices do not offer the v4l2 API to
userspace, but something that might work?  We've already seen today
one user say that they're not going to use mainline because of the
crud surrounding MC.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1604594

On 03/20/2017 02:29 PM, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 02:01:58PM +0100, Hans Verkuil wrote:
>> On 03/19/2017 06:54 PM, Steve Longerbeam wrote:
>>>
>>>
>>> On 03/19/2017 03:38 AM, Russell King - ARM Linux wrote:
>>>> What did you do with:
>>>>
>>>> ioctl(3, VIDIOC_REQBUFS, {count=0, type=0 /* V4L2_BUF_TYPE_??? */, memory=0 /* V4L2_MEMORY_??? */}) = -1 EINVAL (Invalid argument)
>>>>                  test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>>>> ioctl(3, VIDIOC_EXPBUF, 0xbef405bc)     = -1 EINVAL (Invalid argument)
>>>>                  fail: v4l2-test-buffers.cpp(571): q.has_expbuf(node)
>>
>> This is really a knock-on effect from an earlier issue where the compliance test
>> didn't detect support for MEMORY_MMAP.
> 
> So why does it succeed when I fix the compliance errors with VIDIOC_G_FMT?
> With that fixed, I now get:
> 
>         Format ioctls:
>                 test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
>                 test VIDIOC_G/S_PARM: OK
>                 test VIDIOC_G_FBUF: OK (Not Supported)
>                 test VIDIOC_G_FMT: OK
>                 test VIDIOC_TRY_FMT: OK
>                 test VIDIOC_S_FMT: OK
>                 test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
>                 test Cropping: OK (Not Supported)
>                 test Composing: OK (Not Supported)
>                 test Scaling: OK (Not Supported)
> 
>         Buffer ioctls:
>                 test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>                 test VIDIOC_EXPBUF: OK
> 
> The reason is, if you look at the code, VIDIOC_G_FMT populates a list
> of possible buffer formats "node->valid_buftypes".  If the VIDIOC_G_FMT
> test fails, then node->valid_buftypes is zero.
> 
> This causes testReqBufs() to only check for the all-zeroed VIDIOC_REQBUFS
> and declare it conformant, without creating any buffers (it can't, it
> doesn't know which formats are supported.)
> 
> This causes node->valid_memorytype to be zero.

It should fail on this and return a more understandable error message.

> 
> We then go on to testExpBuf(), and valid_memorytype zero, claiming (falsely)
> that MMAP is not supported.  The reality is that it _is_ supported, but
> it's just the non-compliant VICIOC_G_FMT call (due to the colorspace
> issue) causes the sequence of tests to fail.

Yeah, you're not the first to complain about this. I plan on fixing this this
week.

> 
>> Always build from the master repo. 1.10 is pretty old.
> 
> It's what I have - remember, not everyone is happy to constantly replace
> their distro packages with random new stuff.

This is a compliance test, which is continuously developed in tandem with
new kernel versions. If you are working with an upstream kernel, then you
should also use the corresponding v4l2-compliance test. What's the point
of using an old one?

I will not support driver developers that use an old version of the
compliance test, that's a waste of my time.

> 
>>>> In any case, it doesn't look like the buffer management is being
>>>> tested at all by v4l2-compliance - we know that gstreamer works, so
>>>> buffers _can_ be allocated, and I've also used dmabufs with gstreamer,
>>>> so I also know that VIDIOC_EXPBUF works there.
>>
>> To test actual streaming you need to provide the -s option.
>>
>> Note: v4l2-compliance has been developed for 'regular' video devices,
>> not MC devices. It may or may not work with the -s option.
> 
> Right, and it exists to verify that the establised v4l2 API is correctly
> implemented.  If the v4l2 API is being offered to user applications,
> then it must be conformant, otherwise it's not offering the v4l2 API.
> (That's very much a definition statement in itself.)
> 
> So, are we really going to say MC devices do not offer the v4l2 API to
> userspace, but something that might work?  We've already seen today
> one user say that they're not going to use mainline because of the
> crud surrounding MC.
> 

Actually, my understanding was that he was stuck on the old kernel code.

In the case of v4l2-compliance, I never had the time to make it work with
MC devices. Same for that matter of certain memory to memory devices.

Just like MC devices these too behave differently. They are partially
supported in v4l2-compliance, but not fully.

Why? NO TIME.

Be glad there *is* a v4l2-compliance test at all! It's really, really useful
already, but it took *years* to develop, little bit by little bit. And yes,
I would really like to update it to fully support codecs and MC devices.
And with a bit of luck I hope to get permission from my boss to work on
this (among others) later in the year.

Complaining about this really won't help. We know it's a problem and unless
someone (me perhaps?) manages to get paid to work on this it's unlikely to
change for now.

Regards,

	Hans

[toc] | [prev] | [next] | [standalone]

#1604619

On Mon, Mar 20, 2017 at 02:57:03PM +0100, Hans Verkuil wrote:
> On 03/20/2017 02:29 PM, Russell King - ARM Linux wrote:
> > It's what I have - remember, not everyone is happy to constantly replace
> > their distro packages with random new stuff.
> 
> This is a compliance test, which is continuously developed in tandem with
> new kernel versions. If you are working with an upstream kernel, then you
> should also use the corresponding v4l2-compliance test. What's the point
> of using an old one?
> 
> I will not support driver developers that use an old version of the
> compliance test, that's a waste of my time.

The reason that people may _not_ wish to constantly update v4l-utils
is that it changes the libraries installed on their systems.

So, the solution to that is not to complain about developers not using
the latest version, but instead to de-couple it from the rest of the
package, and provide it as a separate, stand-alone package that doesn't
come with all the extra baggage.

Now, there's two possible answers to that:

1. it depends on the libv4l2 version.  If that's so, then you are
   insisting that people constantly move to the latest libv4l2 because
   of API changes, and those API changes may upset applications they're
   using.  So this isn't really on.

2. it doesn't depend on libv4l2 version, in which case there's no reason
   for it to be packaged with v4l-utils.

The reality is that v4l2-compliance links with libv4l2, so I'm not sure
which it is.  What I am sure of is that I don't want to upgrade libv4l2
on an ad-hoc basis, potentially causing issues with applications.

> >> To test actual streaming you need to provide the -s option.
> >>
> >> Note: v4l2-compliance has been developed for 'regular' video devices,
> >> not MC devices. It may or may not work with the -s option.
> > 
> > Right, and it exists to verify that the establised v4l2 API is correctly
> > implemented.  If the v4l2 API is being offered to user applications,
> > then it must be conformant, otherwise it's not offering the v4l2 API.
> > (That's very much a definition statement in itself.)
> > 
> > So, are we really going to say MC devices do not offer the v4l2 API to
> > userspace, but something that might work?  We've already seen today
> > one user say that they're not going to use mainline because of the
> > crud surrounding MC.
> > 
> 
> Actually, my understanding was that he was stuck on the old kernel code.

Err, sorry, I really don't follow.  Who is "he"?

_I_ was the one who reported the EXPBUF problem.  Your comment makes no
sense.

> In the case of v4l2-compliance, I never had the time to make it work with
> MC devices. Same for that matter of certain memory to memory devices.
> 
> Just like MC devices these too behave differently. They are partially
> supported in v4l2-compliance, but not fully.

It seems you saying that the API provided by /dev/video* for a MC device
breaks the v4l2-compliance tests?

_No one_ has mentioned using v4l2-compliance on the subdevs.

> Complaining about this really won't help. We know it's a problem and unless
> someone (me perhaps?) manages to get paid to work on this it's unlikely to
> change for now.

Like the above comment, your comment makes no sense.  I'm not complaining,
I'm trying to find out the details.

Yes, MC stuff sucks big time right now, the documentation is poor, there's
a lack of understanding on all sides of the issues (which can be seen by
the different opinions that people hold.)  The only way to resolve these
differences is via discussion, and if you're going to start thinking that
everyone is complaining, then there's not going to be any forward progress.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1604722

On 03/20/2017 03:11 PM, Russell King - ARM Linux wrote:
> On Mon, Mar 20, 2017 at 02:57:03PM +0100, Hans Verkuil wrote:
>> On 03/20/2017 02:29 PM, Russell King - ARM Linux wrote:
>>> It's what I have - remember, not everyone is happy to constantly replace
>>> their distro packages with random new stuff.
>>
>> This is a compliance test, which is continuously developed in tandem with
>> new kernel versions. If you are working with an upstream kernel, then you
>> should also use the corresponding v4l2-compliance test. What's the point
>> of using an old one?
>>
>> I will not support driver developers that use an old version of the
>> compliance test, that's a waste of my time.
> 
> The reason that people may _not_ wish to constantly update v4l-utils
> is that it changes the libraries installed on their systems.
> 
> So, the solution to that is not to complain about developers not using
> the latest version, but instead to de-couple it from the rest of the
> package, and provide it as a separate, stand-alone package that doesn't
> come with all the extra baggage.
> 
> Now, there's two possible answers to that:
> 
> 1. it depends on the libv4l2 version.  If that's so, then you are
>    insisting that people constantly move to the latest libv4l2 because
>    of API changes, and those API changes may upset applications they're
>    using.  So this isn't really on.
> 
> 2. it doesn't depend on libv4l2 version, in which case there's no reason
>    for it to be packaged with v4l-utils.

Run configure with --disable-v4l2-compliance-libv4l.

This avoids linking with libv4l and allows you to build it stand-alone.

Perhaps I should invert this option since in most cases you don't want to
run v4l2-compliance with libv4l (it's off by default unless you pass the
-w option to v4l2-compliance).

> 
> The reality is that v4l2-compliance links with libv4l2, so I'm not sure
> which it is.  What I am sure of is that I don't want to upgrade libv4l2
> on an ad-hoc basis, potentially causing issues with applications.
> 
>>>> To test actual streaming you need to provide the -s option.
>>>>
>>>> Note: v4l2-compliance has been developed for 'regular' video devices,
>>>> not MC devices. It may or may not work with the -s option.
>>>
>>> Right, and it exists to verify that the establised v4l2 API is correctly
>>> implemented.  If the v4l2 API is being offered to user applications,
>>> then it must be conformant, otherwise it's not offering the v4l2 API.
>>> (That's very much a definition statement in itself.)
>>>
>>> So, are we really going to say MC devices do not offer the v4l2 API to
>>> userspace, but something that might work?  We've already seen today
>>> one user say that they're not going to use mainline because of the
>>> crud surrounding MC.
>>>
>>
>> Actually, my understanding was that he was stuck on the old kernel code.
> 
> Err, sorry, I really don't follow.  Who is "he"?

"one user say that they're not going to use mainline because of the
crud surrounding MC."

> 
> _I_ was the one who reported the EXPBUF problem.  Your comment makes no
> sense.
> 
>> In the case of v4l2-compliance, I never had the time to make it work with
>> MC devices. Same for that matter of certain memory to memory devices.
>>
>> Just like MC devices these too behave differently. They are partially
>> supported in v4l2-compliance, but not fully.
> 
> It seems you saying that the API provided by /dev/video* for a MC device
> breaks the v4l2-compliance tests?

There may be tests in the compliance suite that do not apply for MC devices
and for which I never check. The compliance suite was never written with MC
devices in mind, and it certainly hasn't been tested much with such devices.

It's only very recent that I even got hardware that has MC support...

From what I can tell from the feedback I got it seems to be OKish, but I
just can't guarantee that the compliance utility is correct for such devices.

In particular I doubt the streaming tests (-s, -f, etc.) will work. The -s
test *might* work if the pipeline is configured correctly before running
v4l2-compliance. I can't imagine that the -f option would work at all since
I would expect pipeline validation errors.

I've been gently pushing Helen Koike to finish her virtual MC driver
(https://patchwork.kernel.org/patch/9312783/) since having a virtual driver
makes writing compliance tests much easier.

> _No one_ has mentioned using v4l2-compliance on the subdevs.
> 
>> Complaining about this really won't help. We know it's a problem and unless
>> someone (me perhaps?) manages to get paid to work on this it's unlikely to
>> change for now.
> 
> Like the above comment, your comment makes no sense.  I'm not complaining,
> I'm trying to find out the details.

Must be me then, it did feel like complaining...

> Yes, MC stuff sucks big time right now, the documentation is poor, there's
> a lack of understanding on all sides of the issues (which can be seen by
> the different opinions that people hold.)  The only way to resolve these
> differences is via discussion, and if you're going to start thinking that
> everyone is complaining, then there's not going to be any forward progress.
> 

Regards,

	Hans

[toc] | [prev] | [next] | [standalone]

#1605500

On 2017-03-20 16:57:54 +0100, Hans Verkuil wrote:
> On 03/20/2017 03:11 PM, Russell King - ARM Linux wrote:
> > On Mon, Mar 20, 2017 at 02:57:03PM +0100, Hans Verkuil wrote:
> >> On 03/20/2017 02:29 PM, Russell King - ARM Linux wrote:
> >>> It's what I have - remember, not everyone is happy to constantly replace
> >>> their distro packages with random new stuff.
> >>
> >> This is a compliance test, which is continuously developed in tandem with
> >> new kernel versions. If you are working with an upstream kernel, then you
> >> should also use the corresponding v4l2-compliance test. What's the point
> >> of using an old one?
> >>
> >> I will not support driver developers that use an old version of the
> >> compliance test, that's a waste of my time.
> > 
> > The reason that people may _not_ wish to constantly update v4l-utils
> > is that it changes the libraries installed on their systems.
> > 
> > So, the solution to that is not to complain about developers not using
> > the latest version, but instead to de-couple it from the rest of the
> > package, and provide it as a separate, stand-alone package that doesn't
> > come with all the extra baggage.
> > 
> > Now, there's two possible answers to that:
> > 
> > 1. it depends on the libv4l2 version.  If that's so, then you are
> >    insisting that people constantly move to the latest libv4l2 because
> >    of API changes, and those API changes may upset applications they're
> >    using.  So this isn't really on.
> > 
> > 2. it doesn't depend on libv4l2 version, in which case there's no reason
> >    for it to be packaged with v4l-utils.
> 
> Run configure with --disable-v4l2-compliance-libv4l.
> 
> This avoids linking with libv4l and allows you to build it stand-alone.
> 
> Perhaps I should invert this option since in most cases you don't want to
> run v4l2-compliance with libv4l (it's off by default unless you pass the
> -w option to v4l2-compliance).
> 
> > 
> > The reality is that v4l2-compliance links with libv4l2, so I'm not sure
> > which it is.  What I am sure of is that I don't want to upgrade libv4l2
> > on an ad-hoc basis, potentially causing issues with applications.
> > 
> >>>> To test actual streaming you need to provide the -s option.
> >>>>
> >>>> Note: v4l2-compliance has been developed for 'regular' video devices,
> >>>> not MC devices. It may or may not work with the -s option.
> >>>
> >>> Right, and it exists to verify that the establised v4l2 API is correctly
> >>> implemented.  If the v4l2 API is being offered to user applications,
> >>> then it must be conformant, otherwise it's not offering the v4l2 API.
> >>> (That's very much a definition statement in itself.)
> >>>
> >>> So, are we really going to say MC devices do not offer the v4l2 API to
> >>> userspace, but something that might work?  We've already seen today
> >>> one user say that they're not going to use mainline because of the
> >>> crud surrounding MC.
> >>>
> >>
> >> Actually, my understanding was that he was stuck on the old kernel code.
> > 
> > Err, sorry, I really don't follow.  Who is "he"?
> 
> "one user say that they're not going to use mainline because of the
> crud surrounding MC."
> 
> > 
> > _I_ was the one who reported the EXPBUF problem.  Your comment makes no
> > sense.
> > 
> >> In the case of v4l2-compliance, I never had the time to make it work with
> >> MC devices. Same for that matter of certain memory to memory devices.
> >>
> >> Just like MC devices these too behave differently. They are partially
> >> supported in v4l2-compliance, but not fully.
> > 
> > It seems you saying that the API provided by /dev/video* for a MC device
> > breaks the v4l2-compliance tests?
> 
> There may be tests in the compliance suite that do not apply for MC devices
> and for which I never check. The compliance suite was never written with MC
> devices in mind, and it certainly hasn't been tested much with such devices.
> 
> It's only very recent that I even got hardware that has MC support...
> 
> From what I can tell from the feedback I got it seems to be OKish, but I
> just can't guarantee that the compliance utility is correct for such devices.
> 
> In particular I doubt the streaming tests (-s, -f, etc.) will work. The -s
> test *might* work if the pipeline is configured correctly before running
> v4l2-compliance. I can't imagine that the -f option would work at all since
> I would expect pipeline validation errors.

I successfully use v4l2-compliance with the -s option to test the 
Renesas R-Car Gen3 driver which uses MC, I first have to setup the 
pipeline using media-ctl. I have had much use of the tool and it have 
helped me catch many errors in the rcar-vin driver both on Gen2 (no MC 
involved) and Gen3. And yes the -f option is only usable on Gen2 where 
MC is not used.

For what it's worth, the first versions of the R-Car Gen3 patches did 
not use MC for anything else then setting up the pipeline, all format 
propagation and communication with subdevice where done using the 
/dev/videoX node and it was rather cumbersome. This was done in part due 
the me not understanding the MC framework. Later versions of the patches 
use MC and depends on the user configuring the pipeline and formats 
before starting the stream and it works better for me.

My ahh moment came when I realised that the pipeline can be different on 
different SoC but still be needed to be handled by the same drivers, I 
for example have two use-cases I look at.

Onboard components on Renesas R-Car Salvator-X. This was the only 
use-case I first considered and the early Gen3 patches handled this 
without MC support.

[ADV7482 (CSI-2 transmitter)] -> [R-Car CSI-2 (CSI-2 receiver)] -> [R-Car VIN (dma engine)]

Later I was presented with a more complex use-case where more devices 
would be involved.

[Sensor (Parallel output)] -> [Parallel to GSML] -> [GSML to CSI-2 (CSI-2 transmitter)] -> [R-Car CSI-2 (CSI-2 receiver)] -> [R-Car VIN (dma engine)]

And I could not see a reasonable way to support both use-cases (or 
others where a different pipeline would be connected to the CSI-2 
receiver present on the SoC itself) without the use of MC. There is no 
good way I can see where I can add the logic to the R-Car VIN driver 
which could cover all the knowledge needed to work with arbitrary 
pipelines. But then again I have lot to still learn about V4L2.

> 
> I've been gently pushing Helen Koike to finish her virtual MC driver
> (https://patchwork.kernel.org/patch/9312783/) since having a virtual driver
> makes writing compliance tests much easier.
> 
> > _No one_ has mentioned using v4l2-compliance on the subdevs.
> > 
> >> Complaining about this really won't help. We know it's a problem and unless
> >> someone (me perhaps?) manages to get paid to work on this it's unlikely to
> >> change for now.
> > 
> > Like the above comment, your comment makes no sense.  I'm not complaining,
> > I'm trying to find out the details.
> 
> Must be me then, it did feel like complaining...
> 
> > Yes, MC stuff sucks big time right now, the documentation is poor, there's
> > a lack of understanding on all sides of the issues (which can be seen by
> > the different opinions that people hold.)  The only way to resolve these
> > differences is via discussion, and if you're going to start thinking that
> > everyone is complaining, then there's not going to be any forward progress.
> > 
> 
> Regards,
> 
> 	Hans

-- 
Regards,
Niklas Söderlund

[toc] | [prev] | [next] | [standalone]

#1605503

On 03/21/17 11:42, Niklas Söderlund wrote:
> On 2017-03-20 16:57:54 +0100, Hans Verkuil wrote:
>> On 03/20/2017 03:11 PM, Russell King - ARM Linux wrote:
>>> On Mon, Mar 20, 2017 at 02:57:03PM +0100, Hans Verkuil wrote:
>>>> On 03/20/2017 02:29 PM, Russell King - ARM Linux wrote:
>>>>> It's what I have - remember, not everyone is happy to constantly replace
>>>>> their distro packages with random new stuff.
>>>>
>>>> This is a compliance test, which is continuously developed in tandem with
>>>> new kernel versions. If you are working with an upstream kernel, then you
>>>> should also use the corresponding v4l2-compliance test. What's the point
>>>> of using an old one?
>>>>
>>>> I will not support driver developers that use an old version of the
>>>> compliance test, that's a waste of my time.
>>>
>>> The reason that people may _not_ wish to constantly update v4l-utils
>>> is that it changes the libraries installed on their systems.
>>>
>>> So, the solution to that is not to complain about developers not using
>>> the latest version, but instead to de-couple it from the rest of the
>>> package, and provide it as a separate, stand-alone package that doesn't
>>> come with all the extra baggage.
>>>
>>> Now, there's two possible answers to that:
>>>
>>> 1. it depends on the libv4l2 version.  If that's so, then you are
>>>    insisting that people constantly move to the latest libv4l2 because
>>>    of API changes, and those API changes may upset applications they're
>>>    using.  So this isn't really on.
>>>
>>> 2. it doesn't depend on libv4l2 version, in which case there's no reason
>>>    for it to be packaged with v4l-utils.
>>
>> Run configure with --disable-v4l2-compliance-libv4l.
>>
>> This avoids linking with libv4l and allows you to build it stand-alone.
>>
>> Perhaps I should invert this option since in most cases you don't want to
>> run v4l2-compliance with libv4l (it's off by default unless you pass the
>> -w option to v4l2-compliance).
>>
>>>
>>> The reality is that v4l2-compliance links with libv4l2, so I'm not sure
>>> which it is.  What I am sure of is that I don't want to upgrade libv4l2
>>> on an ad-hoc basis, potentially causing issues with applications.
>>>
>>>>>> To test actual streaming you need to provide the -s option.
>>>>>>
>>>>>> Note: v4l2-compliance has been developed for 'regular' video devices,
>>>>>> not MC devices. It may or may not work with the -s option.
>>>>>
>>>>> Right, and it exists to verify that the establised v4l2 API is correctly
>>>>> implemented.  If the v4l2 API is being offered to user applications,
>>>>> then it must be conformant, otherwise it's not offering the v4l2 API.
>>>>> (That's very much a definition statement in itself.)
>>>>>
>>>>> So, are we really going to say MC devices do not offer the v4l2 API to
>>>>> userspace, but something that might work?  We've already seen today
>>>>> one user say that they're not going to use mainline because of the
>>>>> crud surrounding MC.
>>>>>
>>>>
>>>> Actually, my understanding was that he was stuck on the old kernel code.
>>>
>>> Err, sorry, I really don't follow.  Who is "he"?
>>
>> "one user say that they're not going to use mainline because of the
>> crud surrounding MC."
>>
>>>
>>> _I_ was the one who reported the EXPBUF problem.  Your comment makes no
>>> sense.
>>>
>>>> In the case of v4l2-compliance, I never had the time to make it work with
>>>> MC devices. Same for that matter of certain memory to memory devices.
>>>>
>>>> Just like MC devices these too behave differently. They are partially
>>>> supported in v4l2-compliance, but not fully.
>>>
>>> It seems you saying that the API provided by /dev/video* for a MC device
>>> breaks the v4l2-compliance tests?
>>
>> There may be tests in the compliance suite that do not apply for MC devices
>> and for which I never check. The compliance suite was never written with MC
>> devices in mind, and it certainly hasn't been tested much with such devices.
>>
>> It's only very recent that I even got hardware that has MC support...
>>
>> From what I can tell from the feedback I got it seems to be OKish, but I
>> just can't guarantee that the compliance utility is correct for such devices.
>>
>> In particular I doubt the streaming tests (-s, -f, etc.) will work. The -s
>> test *might* work if the pipeline is configured correctly before running
>> v4l2-compliance. I can't imagine that the -f option would work at all since
>> I would expect pipeline validation errors.
> 
> I successfully use v4l2-compliance with the -s option to test the 
> Renesas R-Car Gen3 driver which uses MC, I first have to setup the 
> pipeline using media-ctl. I have had much use of the tool and it have 
> helped me catch many errors in the rcar-vin driver both on Gen2 (no MC 
> involved) and Gen3. And yes the -f option is only usable on Gen2 where 
> MC is not used.

Ah, good to hear that -s works with MC. I was not sure about that.
Thanks for the feedback!

Regards,

	Hans

[toc] | [prev] | [next] | [standalone]

#1605532

On Tue, Mar 21, 2017 at 11:59:02AM +0100, Hans Verkuil wrote:
> Ah, good to hear that -s works with MC. I was not sure about that.
> Thanks for the feedback!

Not soo good on iMX6:

$ v4l2-compliance -d /dev/video10 -s --expbuf-device=/dev/video0
...
Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
                fail: v4l2-test-input-output.cpp(420): G_INPUT not supported for a capture device
        test VIDIOC_G/S/ENUMINPUT: FAIL
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 0 Audio Inputs: 0 Tuners: 0
...
        Control ioctls:
                test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
                test VIDIOC_QUERYCTRL: OK
                test VIDIOC_G/S_CTRL: OK
                test VIDIOC_G/S/TRY_EXT_CTRLS: OK
                fail: v4l2-test-controls.cpp(782): subscribe event for control 'User Controls' failed
                test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
...
Streaming ioctls:
        test read/write: OK (Not Supported)
                fail: v4l2-test-buffers.cpp(297): g_field() == V4L2_FIELD_ANY
                fail: v4l2-test-buffers.cpp(703): buf.check(q, last_seq)
                fail: v4l2-test-buffers.cpp(973): captureBufs(node, q, m2m_q, frame_count, false)
        test MMAP: FAIL
        test USERPTR: OK (Not Supported)
                fail: v4l2-test-buffers.cpp(1188): can_stream && ret != EINVAL
        test DMABUF: FAIL

(/dev/video0 being CODA).  CODA itself seems to have failures:

        Format ioctls:
                test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
                warn: v4l2-test-formats.cpp(1187): S_PARM is supported for buftype 2, but not ENUM_FRAMEINTERVALS
                warn: v4l2-test-formats.cpp(1194): S_PARM is supported but doesn't report V4L2_CAP_TIMEPERFRAME
                test VIDIOC_G/S_PARM: OK
                test VIDIOC_G_FBUF: OK (Not Supported)
                test VIDIOC_G_FMT: OK
                test VIDIOC_TRY_FMT: OK
                fail: v4l2-test-formats.cpp(774): fmt_out.g_colorspace() != col
...
Streaming ioctls:
        test read/write: OK (Not Supported)
                fail: v4l2-test-buffers.cpp(956): q.create_bufs(node, 1, &fmt) != EINVAL
        test MMAP: FAIL
        test USERPTR: OK (Not Supported)
        test DMABUF: Cannot test, specify --expbuf-device

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web