[PATCH v4 0/3] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[PATCH v4 0/3] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Niklas Söderlund]

Hi,

This series aims to reduce the amount of boiler plate code in video
device drivers who's inputs and/or outputs are controlled by the Media
Controller instead of the V4L2 API.

Patch 1/3 adds a new video device capability flag V4L2_CAP_IO_MC which
if set provides helper implementations for the enum inputs and outputs
ioctrls freeing the video device driver from the need to implement them.

Patch 2/3 and 3/3 converts the R-Car VIN and Intel IPU3 drivers to use 
the new default handlers and capability flag and delete the now  
redundant boiler plate code. I'm sure more video device drivers
can make use of this new flag but as I can only test on these two
platforms I have limited my changes to those.

Niklas Söderlund (3):
  v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
  rcar-vin: Make use of V4L2_CAP_IO_MC
  staging/intel-ipu3: Make use of V4L2_CAP_IO_MC

 .../media/uapi/v4l/vidioc-querycap.rst        |  6 ++
 .../media/videodev2.h.rst.exceptions          |  1 +
 drivers/media/platform/rcar-vin/rcar-v4l2.c   | 17 +-----
 drivers/media/v4l2-core/v4l2-dev.c            | 25 ++++++--
 drivers/media/v4l2-core/v4l2-ioctl.c          | 57 ++++++++++++++++--
 drivers/staging/media/ipu3/ipu3-v4l2.c        | 60 +------------------
 include/uapi/linux/videodev2.h                |  2 +
 7 files changed, 84 insertions(+), 84 deletions(-)

-- 
2.25.1

[PATCH v4 1/3] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Add a video device capability flag to indicate that its inputs and/or
outputs are controlled by the Media Controller instead of the V4L2 API.
When this flag is set, ioctl for enum inputs and outputs are
automatically enabled and programmed to call a helper function.

Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Helen Koike <helen.koike@collabora.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v3
- Update documentation for V4L2_CAP_IO_MC
- Only enable VIDIOC_{G,S}_{INPUT,OUTPUT} if V4L2_CAP_IO_MC is set
  (instead of unconditionally)
- Merge v4l2_ioctl_enum_input_mc() into v4l_enuminput()
- Merge v4l2_ioctl_enum_output_mc() into v4l_enumoutput()

* Changes since v2
- Merged the two patches touching V4L2 core stuff to a single patch.
- Updated documentation for V4L2_CAP_IO_MC
- Added is_io_mc bool in determine_valid_ioctls()
- Folded or moved code closer to where it's used
- Remove unneeded memset()
- Use strscpy() instead of strlcpy()
---
 .../media/uapi/v4l/vidioc-querycap.rst        |  6 ++
 .../media/videodev2.h.rst.exceptions          |  1 +
 drivers/media/v4l2-core/v4l2-dev.c            | 25 ++++++--
 drivers/media/v4l2-core/v4l2-ioctl.c          | 57 +++++++++++++++++--
 include/uapi/linux/videodev2.h                |  2 +
 5 files changed, 81 insertions(+), 10 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
index 5f9930195d624c73..497a6aa2cbebad71 100644
--- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
+++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
@@ -264,6 +264,12 @@ specification the ioctl returns an ``EINVAL`` error code.
     * - ``V4L2_CAP_TOUCH``
       - 0x10000000
       - This is a touch device.
+    * - ``V4L2_CAP_IO_MC``
+      - 0x20000000
+      - There is only one input and/or output seen from userspace. The whole
+        video topology configuration, including which I/O entity is routed to
+        the input/output, is configured by userspace via the Media Controller.
+        See :ref:`media_controller`.
     * - ``V4L2_CAP_DEVICE_CAPS``
       - 0x80000000
       - The driver fills the ``device_caps`` field. This capability can
diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
index cb6ccf91776e6b56..a625fb90e3a989a7 100644
--- a/Documentation/media/videodev2.h.rst.exceptions
+++ b/Documentation/media/videodev2.h.rst.exceptions
@@ -176,6 +176,7 @@ replace define V4L2_CAP_STREAMING device-capabilities
 replace define V4L2_CAP_META_OUTPUT device-capabilities
 replace define V4L2_CAP_DEVICE_CAPS device-capabilities
 replace define V4L2_CAP_TOUCH device-capabilities
+replace define V4L2_CAP_IO_MC device-capabilities
 
 # V4L2 pix flags
 replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 97b6a3af13614639..a593ea0598b551b4 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -552,6 +552,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		       (vdev->device_caps & meta_caps);
 	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
 	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
+	bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
 
 	bitmap_zero(valid_ioctls, BASE_VIDIOC_PRIVATE);
 
@@ -725,9 +726,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		SET_VALID_IOCTL(ops, VIDIOC_G_STD, vidioc_g_std);
 		if (is_rx) {
 			SET_VALID_IOCTL(ops, VIDIOC_QUERYSTD, vidioc_querystd);
-			SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
-			SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
-			SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
+			if (is_io_mc) {
+				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
+				set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
+				set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
+			} else {
+				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
+				SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
+				SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
+			}
 			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDIO, vidioc_enumaudio);
 			SET_VALID_IOCTL(ops, VIDIOC_G_AUDIO, vidioc_g_audio);
 			SET_VALID_IOCTL(ops, VIDIOC_S_AUDIO, vidioc_s_audio);
@@ -735,9 +742,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
 			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
 		}
 		if (is_tx) {
-			SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
-			SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
-			SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
+			if (is_io_mc) {
+				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
+				set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
+				set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
+			} else {
+				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
+				SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
+				SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
+			}
 			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
 			SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
 			SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index fbcc7a20eedf553a..58e9e728f0a7aa4b 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1085,6 +1085,32 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
 	return ret;
 }
 
+static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
+		       struct file *file, void *fh, void *arg)
+{
+	struct video_device *vfd = video_devdata(file);
+
+	if (vfd->device_caps & V4L2_CAP_IO_MC) {
+		*(unsigned int *)arg = 0;
+		return 0;
+	}
+
+	return ops->vidioc_g_input(file, fh, arg);
+}
+
+static int v4l_g_output(const struct v4l2_ioctl_ops *ops,
+		       struct file *file, void *fh, void *arg)
+{
+	struct video_device *vfd = video_devdata(file);
+
+	if (vfd->device_caps & V4L2_CAP_IO_MC) {
+		*(unsigned int *)arg = 0;
+		return 0;
+	}
+
+	return ops->vidioc_g_output(file, fh, arg);
+}
+
 static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
@@ -1094,12 +1120,21 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
 	ret = v4l_enable_media_source(vfd);
 	if (ret)
 		return ret;
+
+	if (vfd->device_caps & V4L2_CAP_IO_MC)
+		return  *(unsigned int *)arg ? -EINVAL : 0;
+
 	return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
 }
 
 static int v4l_s_output(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
+	struct video_device *vfd = video_devdata(file);
+
+	if (vfd->device_caps & V4L2_CAP_IO_MC)
+		return  *(unsigned int *)arg ? -EINVAL : 0;
+
 	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
 }
 
@@ -1143,6 +1178,14 @@ static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
 	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
 		p->capabilities |= V4L2_IN_CAP_STD;
 
+	if (vfd->device_caps & V4L2_CAP_IO_MC) {
+		if (p->index)
+			return -EINVAL;
+		strscpy(p->name, vfd->name, sizeof(p->name));
+		p->type = V4L2_INPUT_TYPE_CAMERA;
+		return 0;
+	}
+
 	return ops->vidioc_enum_input(file, fh, p);
 }
 
@@ -1161,6 +1204,14 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
 	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
 		p->capabilities |= V4L2_OUT_CAP_STD;
 
+	if (vfd->device_caps & V4L2_CAP_IO_MC) {
+		if (p->index)
+			return -EINVAL;
+		strscpy(p->name, vfd->name, sizeof(p->name));
+		p->type = V4L2_OUTPUT_TYPE_ANALOG;
+		return 0;
+	}
+
 	return ops->vidioc_enum_output(file, fh, p);
 }
 
@@ -2678,10 +2729,8 @@ DEFINE_V4L_STUB_FUNC(expbuf)
 DEFINE_V4L_STUB_FUNC(g_std)
 DEFINE_V4L_STUB_FUNC(g_audio)
 DEFINE_V4L_STUB_FUNC(s_audio)
-DEFINE_V4L_STUB_FUNC(g_input)
 DEFINE_V4L_STUB_FUNC(g_edid)
 DEFINE_V4L_STUB_FUNC(s_edid)
-DEFINE_V4L_STUB_FUNC(g_output)
 DEFINE_V4L_STUB_FUNC(g_audout)
 DEFINE_V4L_STUB_FUNC(s_audout)
 DEFINE_V4L_STUB_FUNC(g_jpegcomp)
@@ -2730,11 +2779,11 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO(VIDIOC_S_AUDIO, v4l_stub_s_audio, v4l_print_audio, INFO_FL_PRIO),
 	IOCTL_INFO(VIDIOC_QUERYCTRL, v4l_queryctrl, v4l_print_queryctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_queryctrl, id)),
 	IOCTL_INFO(VIDIOC_QUERYMENU, v4l_querymenu, v4l_print_querymenu, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_querymenu, index)),
-	IOCTL_INFO(VIDIOC_G_INPUT, v4l_stub_g_input, v4l_print_u32, 0),
+	IOCTL_INFO(VIDIOC_G_INPUT, v4l_g_input, v4l_print_u32, 0),
 	IOCTL_INFO(VIDIOC_S_INPUT, v4l_s_input, v4l_print_u32, INFO_FL_PRIO),
 	IOCTL_INFO(VIDIOC_G_EDID, v4l_stub_g_edid, v4l_print_edid, INFO_FL_ALWAYS_COPY),
 	IOCTL_INFO(VIDIOC_S_EDID, v4l_stub_s_edid, v4l_print_edid, INFO_FL_PRIO | INFO_FL_ALWAYS_COPY),
-	IOCTL_INFO(VIDIOC_G_OUTPUT, v4l_stub_g_output, v4l_print_u32, 0),
+	IOCTL_INFO(VIDIOC_G_OUTPUT, v4l_g_output, v4l_print_u32, 0),
 	IOCTL_INFO(VIDIOC_S_OUTPUT, v4l_s_output, v4l_print_u32, INFO_FL_PRIO),
 	IOCTL_INFO(VIDIOC_ENUMOUTPUT, v4l_enumoutput, v4l_print_enumoutput, INFO_FL_CLEAR(v4l2_output, index)),
 	IOCTL_INFO(VIDIOC_G_AUDOUT, v4l_stub_g_audout, v4l_print_audioout, 0),
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 5f9357dcb0602126..c793263a37052856 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -487,6 +487,8 @@ struct v4l2_capability {
 
 #define V4L2_CAP_TOUCH                  0x10000000  /* Is a touch device */
 
+#define V4L2_CAP_IO_MC			0x20000000  /* Is input/output controlled by the media controller */
+
 #define V4L2_CAP_DEVICE_CAPS            0x80000000  /* sets device capabilities field */
 
 /*
-- 
2.25.1

[PATCH v4 2/3] rcar-vin: Make use of V4L2_CAP_IO_MC

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Set the V4L2_CAP_IO_MC capability flag and remove the driver specific
vidioc_enum_input, vidioc_g_input and vidioc_s_input callbacks for the
media controller enabled part of the driver.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 5151a3cd8a6e6754..96ddd36619167fd5 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -767,18 +767,6 @@ static int rvin_mc_s_fmt_vid_cap(struct file *file, void *priv,
 	return 0;
 }
 
-static int rvin_mc_enum_input(struct file *file, void *priv,
-			      struct v4l2_input *i)
-{
-	if (i->index != 0)
-		return -EINVAL;
-
-	i->type = V4L2_INPUT_TYPE_CAMERA;
-	strscpy(i->name, "Camera", sizeof(i->name));
-
-	return 0;
-}
-
 static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
 	.vidioc_querycap		= rvin_querycap,
 	.vidioc_try_fmt_vid_cap		= rvin_mc_try_fmt_vid_cap,
@@ -786,10 +774,6 @@ static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
 	.vidioc_s_fmt_vid_cap		= rvin_mc_s_fmt_vid_cap,
 	.vidioc_enum_fmt_vid_cap	= rvin_enum_fmt_vid_cap,
 
-	.vidioc_enum_input		= rvin_mc_enum_input,
-	.vidioc_g_input			= rvin_g_input,
-	.vidioc_s_input			= rvin_s_input,
-
 	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
 	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
 	.vidioc_querybuf		= vb2_ioctl_querybuf,
@@ -961,6 +945,7 @@ int rvin_v4l2_register(struct rvin_dev *vin)
 	vin->format.colorspace = RVIN_DEFAULT_COLORSPACE;
 
 	if (vin->info->use_mc) {
+		vdev->device_caps |= V4L2_CAP_IO_MC;
 		vdev->ioctl_ops = &rvin_mc_ioctl_ops;
 	} else {
 		vdev->ioctl_ops = &rvin_ioctl_ops;
-- 
2.25.1

[PATCH v4 3/3] staging/intel-ipu3: Make use of V4L2_CAP_IO_MC

[Niklas Söderlund]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Set the V4L2_CAP_IO_MC capability flag and remove the driver specific
vidioc_enum_{input,output}, vidioc_g_{input,output} and
vidioc_s_{input,output} callbacks.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/staging/media/ipu3/ipu3-v4l2.c | 60 +-------------------------
 1 file changed, 2 insertions(+), 58 deletions(-)

diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
index 09c8ede1457cad96..2c42be3d995621e3 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -845,54 +845,6 @@ static int imgu_vidioc_g_meta_fmt(struct file *file, void *fh,
 	return 0;
 }
 
-static int imgu_vidioc_enum_input(struct file *file, void *fh,
-				  struct v4l2_input *input)
-{
-	if (input->index > 0)
-		return -EINVAL;
-	strscpy(input->name, "camera", sizeof(input->name));
-	input->type = V4L2_INPUT_TYPE_CAMERA;
-
-	return 0;
-}
-
-static int imgu_vidioc_g_input(struct file *file, void *fh, unsigned int *input)
-{
-	*input = 0;
-
-	return 0;
-}
-
-static int imgu_vidioc_s_input(struct file *file, void *fh, unsigned int input)
-{
-	return input == 0 ? 0 : -EINVAL;
-}
-
-static int imgu_vidioc_enum_output(struct file *file, void *fh,
-				   struct v4l2_output *output)
-{
-	if (output->index > 0)
-		return -EINVAL;
-	strscpy(output->name, "camera", sizeof(output->name));
-	output->type = V4L2_INPUT_TYPE_CAMERA;
-
-	return 0;
-}
-
-static int imgu_vidioc_g_output(struct file *file, void *fh,
-				unsigned int *output)
-{
-	*output = 0;
-
-	return 0;
-}
-
-static int imgu_vidioc_s_output(struct file *file, void *fh,
-				unsigned int output)
-{
-	return output == 0 ? 0 : -EINVAL;
-}
-
 /******************** function pointers ********************/
 
 static struct v4l2_subdev_internal_ops imgu_subdev_internal_ops = {
@@ -965,14 +917,6 @@ static const struct v4l2_ioctl_ops imgu_v4l2_ioctl_ops = {
 	.vidioc_s_fmt_vid_out_mplane = imgu_vidioc_s_fmt,
 	.vidioc_try_fmt_vid_out_mplane = imgu_vidioc_try_fmt,
 
-	.vidioc_enum_output = imgu_vidioc_enum_output,
-	.vidioc_g_output = imgu_vidioc_g_output,
-	.vidioc_s_output = imgu_vidioc_s_output,
-
-	.vidioc_enum_input = imgu_vidioc_enum_input,
-	.vidioc_g_input = imgu_vidioc_g_input,
-	.vidioc_s_input = imgu_vidioc_s_input,
-
 	/* buffer queue management */
 	.vidioc_reqbufs = vb2_ioctl_reqbufs,
 	.vidioc_create_bufs = vb2_ioctl_create_bufs,
@@ -1062,7 +1006,7 @@ static void imgu_node_to_v4l2(u32 node, struct video_device *vdev,
 
 	switch (node) {
 	case IMGU_NODE_IN:
-		cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE;
+		cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE | V4L2_CAP_IO_MC;
 		f->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
 		vdev->ioctl_ops = &imgu_v4l2_ioctl_ops;
 		break;
@@ -1081,7 +1025,7 @@ static void imgu_node_to_v4l2(u32 node, struct video_device *vdev,
 		imgu_css_meta_fmt_set(&f->fmt.meta);
 		break;
 	default:
-		cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE;
+		cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_IO_MC;
 		f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
 		vdev->ioctl_ops = &imgu_v4l2_ioctl_ops;
 	}
-- 
2.25.1

Re: [PATCH v4 0/3] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Hans Verkuil]

On 3/6/20 5:39 PM, Niklas Söderlund wrote:
> Hi,
> 
> This series aims to reduce the amount of boiler plate code in video
> device drivers who's inputs and/or outputs are controlled by the Media
> Controller instead of the V4L2 API.
> 
> Patch 1/3 adds a new video device capability flag V4L2_CAP_IO_MC which
> if set provides helper implementations for the enum inputs and outputs
> ioctrls freeing the video device driver from the need to implement them.
> 
> Patch 2/3 and 3/3 converts the R-Car VIN and Intel IPU3 drivers to use 
> the new default handlers and capability flag and delete the now  
> redundant boiler plate code. I'm sure more video device drivers
> can make use of this new flag but as I can only test on these two
> platforms I have limited my changes to those.

Can you add a patch that sets this for vimc as well? That's easy to test,
and helps adding compliance tests for this to v4l2-compliance.

Regards,

	Hans

> 
> Niklas Söderlund (3):
>   v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
>   rcar-vin: Make use of V4L2_CAP_IO_MC
>   staging/intel-ipu3: Make use of V4L2_CAP_IO_MC
> 
>  .../media/uapi/v4l/vidioc-querycap.rst        |  6 ++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  drivers/media/platform/rcar-vin/rcar-v4l2.c   | 17 +-----
>  drivers/media/v4l2-core/v4l2-dev.c            | 25 ++++++--
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 57 ++++++++++++++++--
>  drivers/staging/media/ipu3/ipu3-v4l2.c        | 60 +------------------
>  include/uapi/linux/videodev2.h                |  2 +
>  7 files changed, 84 insertions(+), 84 deletions(-)
> 

Re: [PATCH v4 1/3] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Sakari Ailus]

Hi Niklas,

Thanks for the update!

On Fri, Mar 06, 2020 at 05:39:33PM +0100, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Add a video device capability flag to indicate that its inputs and/or
> outputs are controlled by the Media Controller instead of the V4L2 API.
> When this flag is set, ioctl for enum inputs and outputs are
> automatically enabled and programmed to call a helper function.
> 
> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v3
> - Update documentation for V4L2_CAP_IO_MC
> - Only enable VIDIOC_{G,S}_{INPUT,OUTPUT} if V4L2_CAP_IO_MC is set
>   (instead of unconditionally)
> - Merge v4l2_ioctl_enum_input_mc() into v4l_enuminput()
> - Merge v4l2_ioctl_enum_output_mc() into v4l_enumoutput()
> 
> * Changes since v2
> - Merged the two patches touching V4L2 core stuff to a single patch.
> - Updated documentation for V4L2_CAP_IO_MC
> - Added is_io_mc bool in determine_valid_ioctls()
> - Folded or moved code closer to where it's used
> - Remove unneeded memset()
> - Use strscpy() instead of strlcpy()
> ---
>  .../media/uapi/v4l/vidioc-querycap.rst        |  6 ++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  drivers/media/v4l2-core/v4l2-dev.c            | 25 ++++++--
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 57 +++++++++++++++++--
>  include/uapi/linux/videodev2.h                |  2 +
>  5 files changed, 81 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> index 5f9930195d624c73..497a6aa2cbebad71 100644
> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> @@ -264,6 +264,12 @@ specification the ioctl returns an ``EINVAL`` error code.
>      * - ``V4L2_CAP_TOUCH``
>        - 0x10000000
>        - This is a touch device.
> +    * - ``V4L2_CAP_IO_MC``
> +      - 0x20000000
> +      - There is only one input and/or output seen from userspace. The whole
> +        video topology configuration, including which I/O entity is routed to
> +        the input/output, is configured by userspace via the Media Controller.
> +        See :ref:`media_controller`.
>      * - ``V4L2_CAP_DEVICE_CAPS``
>        - 0x80000000
>        - The driver fills the ``device_caps`` field. This capability can
> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> index cb6ccf91776e6b56..a625fb90e3a989a7 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -176,6 +176,7 @@ replace define V4L2_CAP_STREAMING device-capabilities
>  replace define V4L2_CAP_META_OUTPUT device-capabilities
>  replace define V4L2_CAP_DEVICE_CAPS device-capabilities
>  replace define V4L2_CAP_TOUCH device-capabilities
> +replace define V4L2_CAP_IO_MC device-capabilities
>  
>  # V4L2 pix flags
>  replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 97b6a3af13614639..a593ea0598b551b4 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -552,6 +552,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  		       (vdev->device_caps & meta_caps);
>  	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
>  	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
> +	bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
>  
>  	bitmap_zero(valid_ioctls, BASE_VIDIOC_PRIVATE);
>  
> @@ -725,9 +726,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  		SET_VALID_IOCTL(ops, VIDIOC_G_STD, vidioc_g_std);
>  		if (is_rx) {
>  			SET_VALID_IOCTL(ops, VIDIOC_QUERYSTD, vidioc_querystd);
> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> -			SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> -			SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> +			if (is_io_mc) {
> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
> +				set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
> +				set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
> +			} else {
> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> +				SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> +				SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> +			}
>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDIO, vidioc_enumaudio);
>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDIO, vidioc_g_audio);
>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDIO, vidioc_s_audio);
> @@ -735,9 +742,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
>  		}
>  		if (is_tx) {
> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
> -			SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
> -			SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
> +			if (is_io_mc) {
> +				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
> +				set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
> +				set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
> +			} else {
> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
> +				SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
> +				SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
> +			}
>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index fbcc7a20eedf553a..58e9e728f0a7aa4b 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1085,6 +1085,32 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
>  	return ret;
>  }
>  
> +static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
> +		       struct file *file, void *fh, void *arg)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +
> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> +		*(unsigned int *)arg = 0;
> +		return 0;
> +	}
> +
> +	return ops->vidioc_g_input(file, fh, arg);
> +}
> +
> +static int v4l_g_output(const struct v4l2_ioctl_ops *ops,
> +		       struct file *file, void *fh, void *arg)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +
> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> +		*(unsigned int *)arg = 0;
> +		return 0;
> +	}
> +
> +	return ops->vidioc_g_output(file, fh, arg);
> +}
> +
>  static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -1094,12 +1120,21 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>  	ret = v4l_enable_media_source(vfd);
>  	if (ret)
>  		return ret;
> +
> +	if (vfd->device_caps & V4L2_CAP_IO_MC)
> +		return  *(unsigned int *)arg ? -EINVAL : 0;
> +
>  	return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
>  }
>  
>  static int v4l_s_output(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> +	struct video_device *vfd = video_devdata(file);
> +
> +	if (vfd->device_caps & V4L2_CAP_IO_MC)
> +		return  *(unsigned int *)arg ? -EINVAL : 0;

The type is int, not unsigned int. The same on the rest above.

> +
>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>  }
>  
> @@ -1143,6 +1178,14 @@ static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
>  		p->capabilities |= V4L2_IN_CAP_STD;
>  
> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> +		if (p->index)
> +			return -EINVAL;
> +		strscpy(p->name, vfd->name, sizeof(p->name));
> +		p->type = V4L2_INPUT_TYPE_CAMERA;
> +		return 0;
> +	}
> +
>  	return ops->vidioc_enum_input(file, fh, p);
>  }
>  
> @@ -1161,6 +1204,14 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
>  		p->capabilities |= V4L2_OUT_CAP_STD;
>  
> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> +		if (p->index)
> +			return -EINVAL;
> +		strscpy(p->name, vfd->name, sizeof(p->name));
> +		p->type = V4L2_OUTPUT_TYPE_ANALOG;

How about adding a new INPUT and OUTPUT types just for IO_MC device cap?

This isn't necessarily a camera nor analogue output...

With these,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> +		return 0;
> +	}
> +
>  	return ops->vidioc_enum_output(file, fh, p);
>  }
>  
> @@ -2678,10 +2729,8 @@ DEFINE_V4L_STUB_FUNC(expbuf)
>  DEFINE_V4L_STUB_FUNC(g_std)
>  DEFINE_V4L_STUB_FUNC(g_audio)
>  DEFINE_V4L_STUB_FUNC(s_audio)
> -DEFINE_V4L_STUB_FUNC(g_input)
>  DEFINE_V4L_STUB_FUNC(g_edid)
>  DEFINE_V4L_STUB_FUNC(s_edid)
> -DEFINE_V4L_STUB_FUNC(g_output)
>  DEFINE_V4L_STUB_FUNC(g_audout)
>  DEFINE_V4L_STUB_FUNC(s_audout)
>  DEFINE_V4L_STUB_FUNC(g_jpegcomp)
> @@ -2730,11 +2779,11 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
>  	IOCTL_INFO(VIDIOC_S_AUDIO, v4l_stub_s_audio, v4l_print_audio, INFO_FL_PRIO),
>  	IOCTL_INFO(VIDIOC_QUERYCTRL, v4l_queryctrl, v4l_print_queryctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_queryctrl, id)),
>  	IOCTL_INFO(VIDIOC_QUERYMENU, v4l_querymenu, v4l_print_querymenu, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_querymenu, index)),
> -	IOCTL_INFO(VIDIOC_G_INPUT, v4l_stub_g_input, v4l_print_u32, 0),
> +	IOCTL_INFO(VIDIOC_G_INPUT, v4l_g_input, v4l_print_u32, 0),
>  	IOCTL_INFO(VIDIOC_S_INPUT, v4l_s_input, v4l_print_u32, INFO_FL_PRIO),
>  	IOCTL_INFO(VIDIOC_G_EDID, v4l_stub_g_edid, v4l_print_edid, INFO_FL_ALWAYS_COPY),
>  	IOCTL_INFO(VIDIOC_S_EDID, v4l_stub_s_edid, v4l_print_edid, INFO_FL_PRIO | INFO_FL_ALWAYS_COPY),
> -	IOCTL_INFO(VIDIOC_G_OUTPUT, v4l_stub_g_output, v4l_print_u32, 0),
> +	IOCTL_INFO(VIDIOC_G_OUTPUT, v4l_g_output, v4l_print_u32, 0),
>  	IOCTL_INFO(VIDIOC_S_OUTPUT, v4l_s_output, v4l_print_u32, INFO_FL_PRIO),
>  	IOCTL_INFO(VIDIOC_ENUMOUTPUT, v4l_enumoutput, v4l_print_enumoutput, INFO_FL_CLEAR(v4l2_output, index)),
>  	IOCTL_INFO(VIDIOC_G_AUDOUT, v4l_stub_g_audout, v4l_print_audioout, 0),
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 5f9357dcb0602126..c793263a37052856 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -487,6 +487,8 @@ struct v4l2_capability {
>  
>  #define V4L2_CAP_TOUCH                  0x10000000  /* Is a touch device */
>  
> +#define V4L2_CAP_IO_MC			0x20000000  /* Is input/output controlled by the media controller */
> +
>  #define V4L2_CAP_DEVICE_CAPS            0x80000000  /* sets device capabilities field */
>  
>  /*

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 1/3] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Sakari Ailus]

Hi Niklas,

On Fri, Mar 06, 2020 at 05:39:33PM +0100, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Add a video device capability flag to indicate that its inputs and/or
> outputs are controlled by the Media Controller instead of the V4L2 API.
> When this flag is set, ioctl for enum inputs and outputs are
> automatically enabled and programmed to call a helper function.
> 
> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v3
> - Update documentation for V4L2_CAP_IO_MC
> - Only enable VIDIOC_{G,S}_{INPUT,OUTPUT} if V4L2_CAP_IO_MC is set
>   (instead of unconditionally)
> - Merge v4l2_ioctl_enum_input_mc() into v4l_enuminput()
> - Merge v4l2_ioctl_enum_output_mc() into v4l_enumoutput()
> 
> * Changes since v2
> - Merged the two patches touching V4L2 core stuff to a single patch.
> - Updated documentation for V4L2_CAP_IO_MC
> - Added is_io_mc bool in determine_valid_ioctls()
> - Folded or moved code closer to where it's used
> - Remove unneeded memset()
> - Use strscpy() instead of strlcpy()
> ---
>  .../media/uapi/v4l/vidioc-querycap.rst        |  6 ++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  drivers/media/v4l2-core/v4l2-dev.c            | 25 ++++++--
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 57 +++++++++++++++++--
>  include/uapi/linux/videodev2.h                |  2 +
>  5 files changed, 81 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> index 5f9930195d624c73..497a6aa2cbebad71 100644
> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> @@ -264,6 +264,12 @@ specification the ioctl returns an ``EINVAL`` error code.
>      * - ``V4L2_CAP_TOUCH``
>        - 0x10000000
>        - This is a touch device.
> +    * - ``V4L2_CAP_IO_MC``
> +      - 0x20000000
> +      - There is only one input and/or output seen from userspace. The whole
> +        video topology configuration, including which I/O entity is routed to
> +        the input/output, is configured by userspace via the Media Controller.
> +        See :ref:`media_controller`.
>      * - ``V4L2_CAP_DEVICE_CAPS``
>        - 0x80000000
>        - The driver fills the ``device_caps`` field. This capability can
> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> index cb6ccf91776e6b56..a625fb90e3a989a7 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -176,6 +176,7 @@ replace define V4L2_CAP_STREAMING device-capabilities
>  replace define V4L2_CAP_META_OUTPUT device-capabilities
>  replace define V4L2_CAP_DEVICE_CAPS device-capabilities
>  replace define V4L2_CAP_TOUCH device-capabilities
> +replace define V4L2_CAP_IO_MC device-capabilities
>  
>  # V4L2 pix flags
>  replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 97b6a3af13614639..a593ea0598b551b4 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -552,6 +552,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  		       (vdev->device_caps & meta_caps);
>  	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
>  	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
> +	bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
>  
>  	bitmap_zero(valid_ioctls, BASE_VIDIOC_PRIVATE);
>  
> @@ -725,9 +726,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  		SET_VALID_IOCTL(ops, VIDIOC_G_STD, vidioc_g_std);
>  		if (is_rx) {
>  			SET_VALID_IOCTL(ops, VIDIOC_QUERYSTD, vidioc_querystd);
> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> -			SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> -			SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> +			if (is_io_mc) {

One more comment.

I'd make this dependent on the device node type, i.e. devices with
meta_caps set, the IOCTLs won't be supported.

So the check would become:

			if (is_io_mc && !is_meta)

> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
> +				set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
> +				set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
> +			} else {
> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> +				SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> +				SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> +			}
>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDIO, vidioc_enumaudio);
>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDIO, vidioc_g_audio);
>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDIO, vidioc_s_audio);

-- 
Sakari Ailus

Re: [PATCH v4 1/3] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Hans Verkuil]

On 3/9/20 2:47 PM, Sakari Ailus wrote:
> Hi Niklas,
> 
> Thanks for the update!
> 
> On Fri, Mar 06, 2020 at 05:39:33PM +0100, Niklas Söderlund wrote:
>> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>
>> Add a video device capability flag to indicate that its inputs and/or
>> outputs are controlled by the Media Controller instead of the V4L2 API.
>> When this flag is set, ioctl for enum inputs and outputs are
>> automatically enabled and programmed to call a helper function.
>>
>> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>> ---
>> * Changes since v3
>> - Update documentation for V4L2_CAP_IO_MC
>> - Only enable VIDIOC_{G,S}_{INPUT,OUTPUT} if V4L2_CAP_IO_MC is set
>>   (instead of unconditionally)
>> - Merge v4l2_ioctl_enum_input_mc() into v4l_enuminput()
>> - Merge v4l2_ioctl_enum_output_mc() into v4l_enumoutput()
>>
>> * Changes since v2
>> - Merged the two patches touching V4L2 core stuff to a single patch.
>> - Updated documentation for V4L2_CAP_IO_MC
>> - Added is_io_mc bool in determine_valid_ioctls()
>> - Folded or moved code closer to where it's used
>> - Remove unneeded memset()
>> - Use strscpy() instead of strlcpy()
>> ---
>>  .../media/uapi/v4l/vidioc-querycap.rst        |  6 ++
>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>  drivers/media/v4l2-core/v4l2-dev.c            | 25 ++++++--
>>  drivers/media/v4l2-core/v4l2-ioctl.c          | 57 +++++++++++++++++--
>>  include/uapi/linux/videodev2.h                |  2 +
>>  5 files changed, 81 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> index 5f9930195d624c73..497a6aa2cbebad71 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> @@ -264,6 +264,12 @@ specification the ioctl returns an ``EINVAL`` error code.
>>      * - ``V4L2_CAP_TOUCH``
>>        - 0x10000000
>>        - This is a touch device.
>> +    * - ``V4L2_CAP_IO_MC``
>> +      - 0x20000000
>> +      - There is only one input and/or output seen from userspace. The whole
>> +        video topology configuration, including which I/O entity is routed to
>> +        the input/output, is configured by userspace via the Media Controller.
>> +        See :ref:`media_controller`.
>>      * - ``V4L2_CAP_DEVICE_CAPS``
>>        - 0x80000000
>>        - The driver fills the ``device_caps`` field. This capability can
>> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
>> index cb6ccf91776e6b56..a625fb90e3a989a7 100644
>> --- a/Documentation/media/videodev2.h.rst.exceptions
>> +++ b/Documentation/media/videodev2.h.rst.exceptions
>> @@ -176,6 +176,7 @@ replace define V4L2_CAP_STREAMING device-capabilities
>>  replace define V4L2_CAP_META_OUTPUT device-capabilities
>>  replace define V4L2_CAP_DEVICE_CAPS device-capabilities
>>  replace define V4L2_CAP_TOUCH device-capabilities
>> +replace define V4L2_CAP_IO_MC device-capabilities
>>  
>>  # V4L2 pix flags
>>  replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index 97b6a3af13614639..a593ea0598b551b4 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -552,6 +552,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>>  		       (vdev->device_caps & meta_caps);
>>  	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
>>  	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
>> +	bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
>>  
>>  	bitmap_zero(valid_ioctls, BASE_VIDIOC_PRIVATE);
>>  
>> @@ -725,9 +726,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
>>  		SET_VALID_IOCTL(ops, VIDIOC_G_STD, vidioc_g_std);
>>  		if (is_rx) {
>>  			SET_VALID_IOCTL(ops, VIDIOC_QUERYSTD, vidioc_querystd);
>> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
>> -			SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
>> -			SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
>> +			if (is_io_mc) {
>> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
>> +				set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
>> +				set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
>> +			} else {
>> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
>> +				SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
>> +				SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
>> +			}
>>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDIO, vidioc_enumaudio);
>>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDIO, vidioc_g_audio);
>>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDIO, vidioc_s_audio);
>> @@ -735,9 +742,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
>>  			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
>>  		}
>>  		if (is_tx) {
>> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
>> -			SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
>> -			SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
>> +			if (is_io_mc) {
>> +				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
>> +				set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
>> +				set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
>> +			} else {
>> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
>> +				SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
>> +				SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
>> +			}
>>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
>>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
>>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index fbcc7a20eedf553a..58e9e728f0a7aa4b 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -1085,6 +1085,32 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
>>  	return ret;
>>  }
>>  
>> +static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
>> +		       struct file *file, void *fh, void *arg)
>> +{
>> +	struct video_device *vfd = video_devdata(file);
>> +
>> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
>> +		*(unsigned int *)arg = 0;
>> +		return 0;
>> +	}
>> +
>> +	return ops->vidioc_g_input(file, fh, arg);
>> +}
>> +
>> +static int v4l_g_output(const struct v4l2_ioctl_ops *ops,
>> +		       struct file *file, void *fh, void *arg)
>> +{
>> +	struct video_device *vfd = video_devdata(file);
>> +
>> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
>> +		*(unsigned int *)arg = 0;
>> +		return 0;
>> +	}
>> +
>> +	return ops->vidioc_g_output(file, fh, arg);
>> +}
>> +
>>  static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>>  				struct file *file, void *fh, void *arg)
>>  {
>> @@ -1094,12 +1120,21 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>>  	ret = v4l_enable_media_source(vfd);
>>  	if (ret)
>>  		return ret;
>> +
>> +	if (vfd->device_caps & V4L2_CAP_IO_MC)
>> +		return  *(unsigned int *)arg ? -EINVAL : 0;
>> +
>>  	return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
>>  }
>>  
>>  static int v4l_s_output(const struct v4l2_ioctl_ops *ops,
>>  				struct file *file, void *fh, void *arg)
>>  {
>> +	struct video_device *vfd = video_devdata(file);
>> +
>> +	if (vfd->device_caps & V4L2_CAP_IO_MC)
>> +		return  *(unsigned int *)arg ? -EINVAL : 0;
> 
> The type is int, not unsigned int. The same on the rest above.
> 
>> +
>>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>>  }
>>  
>> @@ -1143,6 +1178,14 @@ static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
>>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
>>  		p->capabilities |= V4L2_IN_CAP_STD;
>>  
>> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
>> +		if (p->index)
>> +			return -EINVAL;
>> +		strscpy(p->name, vfd->name, sizeof(p->name));
>> +		p->type = V4L2_INPUT_TYPE_CAMERA;
>> +		return 0;
>> +	}
>> +
>>  	return ops->vidioc_enum_input(file, fh, p);
>>  }
>>  
>> @@ -1161,6 +1204,14 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
>>  		p->capabilities |= V4L2_OUT_CAP_STD;
>>  
>> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
>> +		if (p->index)
>> +			return -EINVAL;
>> +		strscpy(p->name, vfd->name, sizeof(p->name));
>> +		p->type = V4L2_OUTPUT_TYPE_ANALOG;
> 
> How about adding a new INPUT and OUTPUT types just for IO_MC device cap?

V4L2_OUTPUT_TYPE_ANALOG is an old term that is in practice used for anything video,
except for TV modulators. I'm all in favor of changing this to:

#define V4L2_OUTPUT_TYPE_VIDEO 2
#ifndef __KERNEL__
#define V4L2_OUTPUT_TYPE_ANALOG V4L2_OUTPUT_TYPE_VIDEO
#endif

in videodev2.h, if others agree with that. But that can be done in a separate
follow-up patch since this needs to be adapted throughout the kernel & documentation.

The same is true for V4L2_INPUT_TYPE_CAMERA: in practice it's used for any video,
except TV tuners.

Regards,

	Hans

> 
> This isn't necessarily a camera nor analogue output...
> 
> With these,
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
>> +		return 0;
>> +	}
>> +
>>  	return ops->vidioc_enum_output(file, fh, p);
>>  }
>>  
>> @@ -2678,10 +2729,8 @@ DEFINE_V4L_STUB_FUNC(expbuf)
>>  DEFINE_V4L_STUB_FUNC(g_std)
>>  DEFINE_V4L_STUB_FUNC(g_audio)
>>  DEFINE_V4L_STUB_FUNC(s_audio)
>> -DEFINE_V4L_STUB_FUNC(g_input)
>>  DEFINE_V4L_STUB_FUNC(g_edid)
>>  DEFINE_V4L_STUB_FUNC(s_edid)
>> -DEFINE_V4L_STUB_FUNC(g_output)
>>  DEFINE_V4L_STUB_FUNC(g_audout)
>>  DEFINE_V4L_STUB_FUNC(s_audout)
>>  DEFINE_V4L_STUB_FUNC(g_jpegcomp)
>> @@ -2730,11 +2779,11 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
>>  	IOCTL_INFO(VIDIOC_S_AUDIO, v4l_stub_s_audio, v4l_print_audio, INFO_FL_PRIO),
>>  	IOCTL_INFO(VIDIOC_QUERYCTRL, v4l_queryctrl, v4l_print_queryctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_queryctrl, id)),
>>  	IOCTL_INFO(VIDIOC_QUERYMENU, v4l_querymenu, v4l_print_querymenu, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_querymenu, index)),
>> -	IOCTL_INFO(VIDIOC_G_INPUT, v4l_stub_g_input, v4l_print_u32, 0),
>> +	IOCTL_INFO(VIDIOC_G_INPUT, v4l_g_input, v4l_print_u32, 0),
>>  	IOCTL_INFO(VIDIOC_S_INPUT, v4l_s_input, v4l_print_u32, INFO_FL_PRIO),
>>  	IOCTL_INFO(VIDIOC_G_EDID, v4l_stub_g_edid, v4l_print_edid, INFO_FL_ALWAYS_COPY),
>>  	IOCTL_INFO(VIDIOC_S_EDID, v4l_stub_s_edid, v4l_print_edid, INFO_FL_PRIO | INFO_FL_ALWAYS_COPY),
>> -	IOCTL_INFO(VIDIOC_G_OUTPUT, v4l_stub_g_output, v4l_print_u32, 0),
>> +	IOCTL_INFO(VIDIOC_G_OUTPUT, v4l_g_output, v4l_print_u32, 0),
>>  	IOCTL_INFO(VIDIOC_S_OUTPUT, v4l_s_output, v4l_print_u32, INFO_FL_PRIO),
>>  	IOCTL_INFO(VIDIOC_ENUMOUTPUT, v4l_enumoutput, v4l_print_enumoutput, INFO_FL_CLEAR(v4l2_output, index)),
>>  	IOCTL_INFO(VIDIOC_G_AUDOUT, v4l_stub_g_audout, v4l_print_audioout, 0),
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 5f9357dcb0602126..c793263a37052856 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -487,6 +487,8 @@ struct v4l2_capability {
>>  
>>  #define V4L2_CAP_TOUCH                  0x10000000  /* Is a touch device */
>>  
>> +#define V4L2_CAP_IO_MC			0x20000000  /* Is input/output controlled by the media controller */
>> +
>>  #define V4L2_CAP_DEVICE_CAPS            0x80000000  /* sets device capabilities field */
>>  
>>  /*
> 

Re: [PATCH v4 3/3] staging/intel-ipu3: Make use of V4L2_CAP_IO_MC

[Sakari Ailus]

Hi Niklas,

On Fri, Mar 06, 2020 at 05:39:35PM +0100, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Set the V4L2_CAP_IO_MC capability flag and remove the driver specific
> vidioc_enum_{input,output}, vidioc_g_{input,output} and
> vidioc_s_{input,output} callbacks.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/staging/media/ipu3/ipu3-v4l2.c | 60 +-------------------------
>  1 file changed, 2 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
> index 09c8ede1457cad96..2c42be3d995621e3 100644
> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> @@ -845,54 +845,6 @@ static int imgu_vidioc_g_meta_fmt(struct file *file, void *fh,
>  	return 0;
>  }
>  
> -static int imgu_vidioc_enum_input(struct file *file, void *fh,
> -				  struct v4l2_input *input)
> -{
> -	if (input->index > 0)
> -		return -EINVAL;
> -	strscpy(input->name, "camera", sizeof(input->name));
> -	input->type = V4L2_INPUT_TYPE_CAMERA;
> -
> -	return 0;
> -}
> -
> -static int imgu_vidioc_g_input(struct file *file, void *fh, unsigned int *input)
> -{
> -	*input = 0;
> -
> -	return 0;
> -}
> -
> -static int imgu_vidioc_s_input(struct file *file, void *fh, unsigned int input)
> -{
> -	return input == 0 ? 0 : -EINVAL;
> -}
> -
> -static int imgu_vidioc_enum_output(struct file *file, void *fh,
> -				   struct v4l2_output *output)
> -{
> -	if (output->index > 0)
> -		return -EINVAL;
> -	strscpy(output->name, "camera", sizeof(output->name));
> -	output->type = V4L2_INPUT_TYPE_CAMERA;
> -
> -	return 0;
> -}
> -
> -static int imgu_vidioc_g_output(struct file *file, void *fh,
> -				unsigned int *output)
> -{
> -	*output = 0;
> -
> -	return 0;
> -}
> -
> -static int imgu_vidioc_s_output(struct file *file, void *fh,
> -				unsigned int output)
> -{
> -	return output == 0 ? 0 : -EINVAL;
> -}
> -
>  /******************** function pointers ********************/
>  
>  static struct v4l2_subdev_internal_ops imgu_subdev_internal_ops = {
> @@ -965,14 +917,6 @@ static const struct v4l2_ioctl_ops imgu_v4l2_ioctl_ops = {
>  	.vidioc_s_fmt_vid_out_mplane = imgu_vidioc_s_fmt,
>  	.vidioc_try_fmt_vid_out_mplane = imgu_vidioc_try_fmt,
>  
> -	.vidioc_enum_output = imgu_vidioc_enum_output,
> -	.vidioc_g_output = imgu_vidioc_g_output,
> -	.vidioc_s_output = imgu_vidioc_s_output,
> -
> -	.vidioc_enum_input = imgu_vidioc_enum_input,
> -	.vidioc_g_input = imgu_vidioc_g_input,
> -	.vidioc_s_input = imgu_vidioc_s_input,
> -
>  	/* buffer queue management */
>  	.vidioc_reqbufs = vb2_ioctl_reqbufs,
>  	.vidioc_create_bufs = vb2_ioctl_create_bufs,
> @@ -1062,7 +1006,7 @@ static void imgu_node_to_v4l2(u32 node, struct video_device *vdev,
>  
>  	switch (node) {
>  	case IMGU_NODE_IN:
> -		cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE;
> +		cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE | V4L2_CAP_IO_MC;
>  		f->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
>  		vdev->ioctl_ops = &imgu_v4l2_ioctl_ops;
>  		break;
> @@ -1081,7 +1025,7 @@ static void imgu_node_to_v4l2(u32 node, struct video_device *vdev,
>  		imgu_css_meta_fmt_set(&f->fmt.meta);
>  		break;
>  	default:
> -		cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE;
> +		cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_IO_MC;

So here, you'd add V4L2_CAP_IO_MC to all kinds of nodes.

With that,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

>  		f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
>  		vdev->ioctl_ops = &imgu_v4l2_ioctl_ops;
>  	}

-- 
Sakari Ailus

Re: [PATCH v4 1/3] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Sakari Ailus]

Hi Hans,

On Mon, Mar 09, 2020 at 02:53:15PM +0100, Hans Verkuil wrote:
> On 3/9/20 2:47 PM, Sakari Ailus wrote:
> > Hi Niklas,
> > 
> > Thanks for the update!
> > 
> > On Fri, Mar 06, 2020 at 05:39:33PM +0100, Niklas Söderlund wrote:
> >> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>
> >> Add a video device capability flag to indicate that its inputs and/or
> >> outputs are controlled by the Media Controller instead of the V4L2 API.
> >> When this flag is set, ioctl for enum inputs and outputs are
> >> automatically enabled and programmed to call a helper function.
> >>
> >> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> >> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >> ---
> >> * Changes since v3
> >> - Update documentation for V4L2_CAP_IO_MC
> >> - Only enable VIDIOC_{G,S}_{INPUT,OUTPUT} if V4L2_CAP_IO_MC is set
> >>   (instead of unconditionally)
> >> - Merge v4l2_ioctl_enum_input_mc() into v4l_enuminput()
> >> - Merge v4l2_ioctl_enum_output_mc() into v4l_enumoutput()
> >>
> >> * Changes since v2
> >> - Merged the two patches touching V4L2 core stuff to a single patch.
> >> - Updated documentation for V4L2_CAP_IO_MC
> >> - Added is_io_mc bool in determine_valid_ioctls()
> >> - Folded or moved code closer to where it's used
> >> - Remove unneeded memset()
> >> - Use strscpy() instead of strlcpy()
> >> ---
> >>  .../media/uapi/v4l/vidioc-querycap.rst        |  6 ++
> >>  .../media/videodev2.h.rst.exceptions          |  1 +
> >>  drivers/media/v4l2-core/v4l2-dev.c            | 25 ++++++--
> >>  drivers/media/v4l2-core/v4l2-ioctl.c          | 57 +++++++++++++++++--
> >>  include/uapi/linux/videodev2.h                |  2 +
> >>  5 files changed, 81 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> >> index 5f9930195d624c73..497a6aa2cbebad71 100644
> >> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
> >> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> >> @@ -264,6 +264,12 @@ specification the ioctl returns an ``EINVAL`` error code.
> >>      * - ``V4L2_CAP_TOUCH``
> >>        - 0x10000000
> >>        - This is a touch device.
> >> +    * - ``V4L2_CAP_IO_MC``
> >> +      - 0x20000000
> >> +      - There is only one input and/or output seen from userspace. The whole
> >> +        video topology configuration, including which I/O entity is routed to
> >> +        the input/output, is configured by userspace via the Media Controller.
> >> +        See :ref:`media_controller`.
> >>      * - ``V4L2_CAP_DEVICE_CAPS``
> >>        - 0x80000000
> >>        - The driver fills the ``device_caps`` field. This capability can
> >> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> >> index cb6ccf91776e6b56..a625fb90e3a989a7 100644
> >> --- a/Documentation/media/videodev2.h.rst.exceptions
> >> +++ b/Documentation/media/videodev2.h.rst.exceptions
> >> @@ -176,6 +176,7 @@ replace define V4L2_CAP_STREAMING device-capabilities
> >>  replace define V4L2_CAP_META_OUTPUT device-capabilities
> >>  replace define V4L2_CAP_DEVICE_CAPS device-capabilities
> >>  replace define V4L2_CAP_TOUCH device-capabilities
> >> +replace define V4L2_CAP_IO_MC device-capabilities
> >>  
> >>  # V4L2 pix flags
> >>  replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
> >> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> >> index 97b6a3af13614639..a593ea0598b551b4 100644
> >> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >> @@ -552,6 +552,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >>  		       (vdev->device_caps & meta_caps);
> >>  	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
> >>  	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
> >> +	bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
> >>  
> >>  	bitmap_zero(valid_ioctls, BASE_VIDIOC_PRIVATE);
> >>  
> >> @@ -725,9 +726,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >>  		SET_VALID_IOCTL(ops, VIDIOC_G_STD, vidioc_g_std);
> >>  		if (is_rx) {
> >>  			SET_VALID_IOCTL(ops, VIDIOC_QUERYSTD, vidioc_querystd);
> >> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> >> -			SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> >> -			SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> >> +			if (is_io_mc) {
> >> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
> >> +				set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
> >> +				set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
> >> +			} else {
> >> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> >> +				SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> >> +				SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> >> +			}
> >>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDIO, vidioc_enumaudio);
> >>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDIO, vidioc_g_audio);
> >>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDIO, vidioc_s_audio);
> >> @@ -735,9 +742,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >>  			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
> >>  		}
> >>  		if (is_tx) {
> >> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
> >> -			SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
> >> -			SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
> >> +			if (is_io_mc) {
> >> +				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
> >> +				set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
> >> +				set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
> >> +			} else {
> >> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
> >> +				SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
> >> +				SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
> >> +			}
> >>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
> >>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
> >>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
> >> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> >> index fbcc7a20eedf553a..58e9e728f0a7aa4b 100644
> >> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >> @@ -1085,6 +1085,32 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
> >>  	return ret;
> >>  }
> >>  
> >> +static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
> >> +		       struct file *file, void *fh, void *arg)
> >> +{
> >> +	struct video_device *vfd = video_devdata(file);
> >> +
> >> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> >> +		*(unsigned int *)arg = 0;
> >> +		return 0;
> >> +	}
> >> +
> >> +	return ops->vidioc_g_input(file, fh, arg);
> >> +}
> >> +
> >> +static int v4l_g_output(const struct v4l2_ioctl_ops *ops,
> >> +		       struct file *file, void *fh, void *arg)
> >> +{
> >> +	struct video_device *vfd = video_devdata(file);
> >> +
> >> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> >> +		*(unsigned int *)arg = 0;
> >> +		return 0;
> >> +	}
> >> +
> >> +	return ops->vidioc_g_output(file, fh, arg);
> >> +}
> >> +
> >>  static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
> >>  				struct file *file, void *fh, void *arg)
> >>  {
> >> @@ -1094,12 +1120,21 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
> >>  	ret = v4l_enable_media_source(vfd);
> >>  	if (ret)
> >>  		return ret;
> >> +
> >> +	if (vfd->device_caps & V4L2_CAP_IO_MC)
> >> +		return  *(unsigned int *)arg ? -EINVAL : 0;
> >> +
> >>  	return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
> >>  }
> >>  
> >>  static int v4l_s_output(const struct v4l2_ioctl_ops *ops,
> >>  				struct file *file, void *fh, void *arg)
> >>  {
> >> +	struct video_device *vfd = video_devdata(file);
> >> +
> >> +	if (vfd->device_caps & V4L2_CAP_IO_MC)
> >> +		return  *(unsigned int *)arg ? -EINVAL : 0;
> > 
> > The type is int, not unsigned int. The same on the rest above.
> > 
> >> +
> >>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
> >>  }
> >>  
> >> @@ -1143,6 +1178,14 @@ static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
> >>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
> >>  		p->capabilities |= V4L2_IN_CAP_STD;
> >>  
> >> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> >> +		if (p->index)
> >> +			return -EINVAL;
> >> +		strscpy(p->name, vfd->name, sizeof(p->name));
> >> +		p->type = V4L2_INPUT_TYPE_CAMERA;
> >> +		return 0;
> >> +	}
> >> +
> >>  	return ops->vidioc_enum_input(file, fh, p);
> >>  }
> >>  
> >> @@ -1161,6 +1204,14 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
> >>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
> >>  		p->capabilities |= V4L2_OUT_CAP_STD;
> >>  
> >> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> >> +		if (p->index)
> >> +			return -EINVAL;
> >> +		strscpy(p->name, vfd->name, sizeof(p->name));
> >> +		p->type = V4L2_OUTPUT_TYPE_ANALOG;
> > 
> > How about adding a new INPUT and OUTPUT types just for IO_MC device cap?
> 
> V4L2_OUTPUT_TYPE_ANALOG is an old term that is in practice used for anything video,
> except for TV modulators. I'm all in favor of changing this to:
> 
> #define V4L2_OUTPUT_TYPE_VIDEO 2
> #ifndef __KERNEL__
> #define V4L2_OUTPUT_TYPE_ANALOG V4L2_OUTPUT_TYPE_VIDEO
> #endif
> 
> in videodev2.h, if others agree with that. But that can be done in a separate
> follow-up patch since this needs to be adapted throughout the kernel & documentation.
> 
> The same is true for V4L2_INPUT_TYPE_CAMERA: in practice it's used for any video,
> except TV tuners.

My point was rather that here, we don't know what kind of device really is
the source (or the sink) of the data path. Having a designated input and
output type for that would be entirely appropriate.

Although if the user gets a misleading value using an interface that
(s)he's not supposed to use to begin with, I guess I'd fine with that as
well.

-- 
Regards,

Sakari Ailus

Re: [PATCH v4 1/3] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Hans Verkuil]

On 3/9/20 2:56 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Mar 09, 2020 at 02:53:15PM +0100, Hans Verkuil wrote:
>> On 3/9/20 2:47 PM, Sakari Ailus wrote:
>>> Hi Niklas,
>>>
>>> Thanks for the update!
>>>
>>> On Fri, Mar 06, 2020 at 05:39:33PM +0100, Niklas Söderlund wrote:
>>>> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>>
>>>> Add a video device capability flag to indicate that its inputs and/or
>>>> outputs are controlled by the Media Controller instead of the V4L2 API.
>>>> When this flag is set, ioctl for enum inputs and outputs are
>>>> automatically enabled and programmed to call a helper function.
>>>>
>>>> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>> ---
>>>> * Changes since v3
>>>> - Update documentation for V4L2_CAP_IO_MC
>>>> - Only enable VIDIOC_{G,S}_{INPUT,OUTPUT} if V4L2_CAP_IO_MC is set
>>>>   (instead of unconditionally)
>>>> - Merge v4l2_ioctl_enum_input_mc() into v4l_enuminput()
>>>> - Merge v4l2_ioctl_enum_output_mc() into v4l_enumoutput()
>>>>
>>>> * Changes since v2
>>>> - Merged the two patches touching V4L2 core stuff to a single patch.
>>>> - Updated documentation for V4L2_CAP_IO_MC
>>>> - Added is_io_mc bool in determine_valid_ioctls()
>>>> - Folded or moved code closer to where it's used
>>>> - Remove unneeded memset()
>>>> - Use strscpy() instead of strlcpy()
>>>> ---
>>>>  .../media/uapi/v4l/vidioc-querycap.rst        |  6 ++
>>>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>>>  drivers/media/v4l2-core/v4l2-dev.c            | 25 ++++++--
>>>>  drivers/media/v4l2-core/v4l2-ioctl.c          | 57 +++++++++++++++++--
>>>>  include/uapi/linux/videodev2.h                |  2 +
>>>>  5 files changed, 81 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
>>>> index 5f9930195d624c73..497a6aa2cbebad71 100644
>>>> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
>>>> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
>>>> @@ -264,6 +264,12 @@ specification the ioctl returns an ``EINVAL`` error code.
>>>>      * - ``V4L2_CAP_TOUCH``
>>>>        - 0x10000000
>>>>        - This is a touch device.
>>>> +    * - ``V4L2_CAP_IO_MC``
>>>> +      - 0x20000000
>>>> +      - There is only one input and/or output seen from userspace. The whole
>>>> +        video topology configuration, including which I/O entity is routed to
>>>> +        the input/output, is configured by userspace via the Media Controller.
>>>> +        See :ref:`media_controller`.
>>>>      * - ``V4L2_CAP_DEVICE_CAPS``
>>>>        - 0x80000000
>>>>        - The driver fills the ``device_caps`` field. This capability can
>>>> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
>>>> index cb6ccf91776e6b56..a625fb90e3a989a7 100644
>>>> --- a/Documentation/media/videodev2.h.rst.exceptions
>>>> +++ b/Documentation/media/videodev2.h.rst.exceptions
>>>> @@ -176,6 +176,7 @@ replace define V4L2_CAP_STREAMING device-capabilities
>>>>  replace define V4L2_CAP_META_OUTPUT device-capabilities
>>>>  replace define V4L2_CAP_DEVICE_CAPS device-capabilities
>>>>  replace define V4L2_CAP_TOUCH device-capabilities
>>>> +replace define V4L2_CAP_IO_MC device-capabilities
>>>>  
>>>>  # V4L2 pix flags
>>>>  replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
>>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>>> index 97b6a3af13614639..a593ea0598b551b4 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>>> @@ -552,6 +552,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>>>>  		       (vdev->device_caps & meta_caps);
>>>>  	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
>>>>  	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
>>>> +	bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
>>>>  
>>>>  	bitmap_zero(valid_ioctls, BASE_VIDIOC_PRIVATE);
>>>>  
>>>> @@ -725,9 +726,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
>>>>  		SET_VALID_IOCTL(ops, VIDIOC_G_STD, vidioc_g_std);
>>>>  		if (is_rx) {
>>>>  			SET_VALID_IOCTL(ops, VIDIOC_QUERYSTD, vidioc_querystd);
>>>> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
>>>> -			SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
>>>> -			SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
>>>> +			if (is_io_mc) {
>>>> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
>>>> +				set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
>>>> +				set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
>>>> +			} else {
>>>> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
>>>> +				SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
>>>> +				SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
>>>> +			}
>>>>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDIO, vidioc_enumaudio);
>>>>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDIO, vidioc_g_audio);
>>>>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDIO, vidioc_s_audio);
>>>> @@ -735,9 +742,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
>>>>  			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
>>>>  		}
>>>>  		if (is_tx) {
>>>> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
>>>> -			SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
>>>> -			SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
>>>> +			if (is_io_mc) {
>>>> +				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
>>>> +				set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
>>>> +				set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
>>>> +			} else {
>>>> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
>>>> +				SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
>>>> +				SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
>>>> +			}
>>>>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
>>>>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
>>>>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>> index fbcc7a20eedf553a..58e9e728f0a7aa4b 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>>> @@ -1085,6 +1085,32 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
>>>> +		       struct file *file, void *fh, void *arg)
>>>> +{
>>>> +	struct video_device *vfd = video_devdata(file);
>>>> +
>>>> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
>>>> +		*(unsigned int *)arg = 0;
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	return ops->vidioc_g_input(file, fh, arg);
>>>> +}
>>>> +
>>>> +static int v4l_g_output(const struct v4l2_ioctl_ops *ops,
>>>> +		       struct file *file, void *fh, void *arg)
>>>> +{
>>>> +	struct video_device *vfd = video_devdata(file);
>>>> +
>>>> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
>>>> +		*(unsigned int *)arg = 0;
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	return ops->vidioc_g_output(file, fh, arg);
>>>> +}
>>>> +
>>>>  static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>>>>  				struct file *file, void *fh, void *arg)
>>>>  {
>>>> @@ -1094,12 +1120,21 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>>>>  	ret = v4l_enable_media_source(vfd);
>>>>  	if (ret)
>>>>  		return ret;
>>>> +
>>>> +	if (vfd->device_caps & V4L2_CAP_IO_MC)
>>>> +		return  *(unsigned int *)arg ? -EINVAL : 0;
>>>> +
>>>>  	return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
>>>>  }
>>>>  
>>>>  static int v4l_s_output(const struct v4l2_ioctl_ops *ops,
>>>>  				struct file *file, void *fh, void *arg)
>>>>  {
>>>> +	struct video_device *vfd = video_devdata(file);
>>>> +
>>>> +	if (vfd->device_caps & V4L2_CAP_IO_MC)
>>>> +		return  *(unsigned int *)arg ? -EINVAL : 0;
>>>
>>> The type is int, not unsigned int. The same on the rest above.
>>>
>>>> +
>>>>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>>>>  }
>>>>  
>>>> @@ -1143,6 +1178,14 @@ static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
>>>>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
>>>>  		p->capabilities |= V4L2_IN_CAP_STD;
>>>>  
>>>> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
>>>> +		if (p->index)
>>>> +			return -EINVAL;
>>>> +		strscpy(p->name, vfd->name, sizeof(p->name));
>>>> +		p->type = V4L2_INPUT_TYPE_CAMERA;
>>>> +		return 0;
>>>> +	}
>>>> +
>>>>  	return ops->vidioc_enum_input(file, fh, p);
>>>>  }
>>>>  
>>>> @@ -1161,6 +1204,14 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>>>>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
>>>>  		p->capabilities |= V4L2_OUT_CAP_STD;
>>>>  
>>>> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
>>>> +		if (p->index)
>>>> +			return -EINVAL;
>>>> +		strscpy(p->name, vfd->name, sizeof(p->name));
>>>> +		p->type = V4L2_OUTPUT_TYPE_ANALOG;
>>>
>>> How about adding a new INPUT and OUTPUT types just for IO_MC device cap?
>>
>> V4L2_OUTPUT_TYPE_ANALOG is an old term that is in practice used for anything video,
>> except for TV modulators. I'm all in favor of changing this to:
>>
>> #define V4L2_OUTPUT_TYPE_VIDEO 2
>> #ifndef __KERNEL__
>> #define V4L2_OUTPUT_TYPE_ANALOG V4L2_OUTPUT_TYPE_VIDEO
>> #endif
>>
>> in videodev2.h, if others agree with that. But that can be done in a separate
>> follow-up patch since this needs to be adapted throughout the kernel & documentation.
>>
>> The same is true for V4L2_INPUT_TYPE_CAMERA: in practice it's used for any video,
>> except TV tuners.
> 
> My point was rather that here, we don't know what kind of device really is
> the source (or the sink) of the data path. Having a designated input and
> output type for that would be entirely appropriate.

But that's exactly what V4L2_OUTPUT_TYPE_ANALOG/V4L2_INPUT_TYPE_CAMERA means:
it's some kind of video, but we don't know what.

That's why these names are so misleading: they suggest a particular type,
but in reality they don't.

So V4L2_IN/OUTPUT_TYPE_VIDEO is what you want for MC inputs/outputs, but that's
just an alias for V4L2_INPUT_TYPE_CAMERA/V4L2_OUTPUT_TYPE_ANALOG.

Regards,

	Hans

> 
> Although if the user gets a misleading value using an interface that
> (s)he's not supposed to use to begin with, I guess I'd fine with that as
> well.
> 

Re: [PATCH v4 1/3] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Sakari Ailus]

On Mon, Mar 09, 2020 at 03:10:36PM +0100, Hans Verkuil wrote:
> On 3/9/20 2:56 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Mar 09, 2020 at 02:53:15PM +0100, Hans Verkuil wrote:
> >> On 3/9/20 2:47 PM, Sakari Ailus wrote:
> >>> Hi Niklas,
> >>>
> >>> Thanks for the update!
> >>>
> >>> On Fri, Mar 06, 2020 at 05:39:33PM +0100, Niklas Söderlund wrote:
> >>>> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>>>
> >>>> Add a video device capability flag to indicate that its inputs and/or
> >>>> outputs are controlled by the Media Controller instead of the V4L2 API.
> >>>> When this flag is set, ioctl for enum inputs and outputs are
> >>>> automatically enabled and programmed to call a helper function.
> >>>>
> >>>> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>>> ---
> >>>> * Changes since v3
> >>>> - Update documentation for V4L2_CAP_IO_MC
> >>>> - Only enable VIDIOC_{G,S}_{INPUT,OUTPUT} if V4L2_CAP_IO_MC is set
> >>>>   (instead of unconditionally)
> >>>> - Merge v4l2_ioctl_enum_input_mc() into v4l_enuminput()
> >>>> - Merge v4l2_ioctl_enum_output_mc() into v4l_enumoutput()
> >>>>
> >>>> * Changes since v2
> >>>> - Merged the two patches touching V4L2 core stuff to a single patch.
> >>>> - Updated documentation for V4L2_CAP_IO_MC
> >>>> - Added is_io_mc bool in determine_valid_ioctls()
> >>>> - Folded or moved code closer to where it's used
> >>>> - Remove unneeded memset()
> >>>> - Use strscpy() instead of strlcpy()
> >>>> ---
> >>>>  .../media/uapi/v4l/vidioc-querycap.rst        |  6 ++
> >>>>  .../media/videodev2.h.rst.exceptions          |  1 +
> >>>>  drivers/media/v4l2-core/v4l2-dev.c            | 25 ++++++--
> >>>>  drivers/media/v4l2-core/v4l2-ioctl.c          | 57 +++++++++++++++++--
> >>>>  include/uapi/linux/videodev2.h                |  2 +
> >>>>  5 files changed, 81 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> >>>> index 5f9930195d624c73..497a6aa2cbebad71 100644
> >>>> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
> >>>> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> >>>> @@ -264,6 +264,12 @@ specification the ioctl returns an ``EINVAL`` error code.
> >>>>      * - ``V4L2_CAP_TOUCH``
> >>>>        - 0x10000000
> >>>>        - This is a touch device.
> >>>> +    * - ``V4L2_CAP_IO_MC``
> >>>> +      - 0x20000000
> >>>> +      - There is only one input and/or output seen from userspace. The whole
> >>>> +        video topology configuration, including which I/O entity is routed to
> >>>> +        the input/output, is configured by userspace via the Media Controller.
> >>>> +        See :ref:`media_controller`.
> >>>>      * - ``V4L2_CAP_DEVICE_CAPS``
> >>>>        - 0x80000000
> >>>>        - The driver fills the ``device_caps`` field. This capability can
> >>>> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> >>>> index cb6ccf91776e6b56..a625fb90e3a989a7 100644
> >>>> --- a/Documentation/media/videodev2.h.rst.exceptions
> >>>> +++ b/Documentation/media/videodev2.h.rst.exceptions
> >>>> @@ -176,6 +176,7 @@ replace define V4L2_CAP_STREAMING device-capabilities
> >>>>  replace define V4L2_CAP_META_OUTPUT device-capabilities
> >>>>  replace define V4L2_CAP_DEVICE_CAPS device-capabilities
> >>>>  replace define V4L2_CAP_TOUCH device-capabilities
> >>>> +replace define V4L2_CAP_IO_MC device-capabilities
> >>>>  
> >>>>  # V4L2 pix flags
> >>>>  replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
> >>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> >>>> index 97b6a3af13614639..a593ea0598b551b4 100644
> >>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
> >>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> >>>> @@ -552,6 +552,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >>>>  		       (vdev->device_caps & meta_caps);
> >>>>  	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
> >>>>  	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
> >>>> +	bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
> >>>>  
> >>>>  	bitmap_zero(valid_ioctls, BASE_VIDIOC_PRIVATE);
> >>>>  
> >>>> @@ -725,9 +726,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >>>>  		SET_VALID_IOCTL(ops, VIDIOC_G_STD, vidioc_g_std);
> >>>>  		if (is_rx) {
> >>>>  			SET_VALID_IOCTL(ops, VIDIOC_QUERYSTD, vidioc_querystd);
> >>>> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> >>>> -			SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> >>>> -			SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> >>>> +			if (is_io_mc) {
> >>>> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
> >>>> +				set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
> >>>> +				set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
> >>>> +			} else {
> >>>> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> >>>> +				SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> >>>> +				SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> >>>> +			}
> >>>>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDIO, vidioc_enumaudio);
> >>>>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDIO, vidioc_g_audio);
> >>>>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDIO, vidioc_s_audio);
> >>>> @@ -735,9 +742,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >>>>  			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
> >>>>  		}
> >>>>  		if (is_tx) {
> >>>> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
> >>>> -			SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
> >>>> -			SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
> >>>> +			if (is_io_mc) {
> >>>> +				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
> >>>> +				set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
> >>>> +				set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
> >>>> +			} else {
> >>>> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
> >>>> +				SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
> >>>> +				SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
> >>>> +			}
> >>>>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
> >>>>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
> >>>>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
> >>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>> index fbcc7a20eedf553a..58e9e728f0a7aa4b 100644
> >>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> >>>> @@ -1085,6 +1085,32 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
> >>>>  	return ret;
> >>>>  }
> >>>>  
> >>>> +static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
> >>>> +		       struct file *file, void *fh, void *arg)
> >>>> +{
> >>>> +	struct video_device *vfd = video_devdata(file);
> >>>> +
> >>>> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> >>>> +		*(unsigned int *)arg = 0;
> >>>> +		return 0;
> >>>> +	}
> >>>> +
> >>>> +	return ops->vidioc_g_input(file, fh, arg);
> >>>> +}
> >>>> +
> >>>> +static int v4l_g_output(const struct v4l2_ioctl_ops *ops,
> >>>> +		       struct file *file, void *fh, void *arg)
> >>>> +{
> >>>> +	struct video_device *vfd = video_devdata(file);
> >>>> +
> >>>> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> >>>> +		*(unsigned int *)arg = 0;
> >>>> +		return 0;
> >>>> +	}
> >>>> +
> >>>> +	return ops->vidioc_g_output(file, fh, arg);
> >>>> +}
> >>>> +
> >>>>  static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
> >>>>  				struct file *file, void *fh, void *arg)
> >>>>  {
> >>>> @@ -1094,12 +1120,21 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
> >>>>  	ret = v4l_enable_media_source(vfd);
> >>>>  	if (ret)
> >>>>  		return ret;
> >>>> +
> >>>> +	if (vfd->device_caps & V4L2_CAP_IO_MC)
> >>>> +		return  *(unsigned int *)arg ? -EINVAL : 0;
> >>>> +
> >>>>  	return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
> >>>>  }
> >>>>  
> >>>>  static int v4l_s_output(const struct v4l2_ioctl_ops *ops,
> >>>>  				struct file *file, void *fh, void *arg)
> >>>>  {
> >>>> +	struct video_device *vfd = video_devdata(file);
> >>>> +
> >>>> +	if (vfd->device_caps & V4L2_CAP_IO_MC)
> >>>> +		return  *(unsigned int *)arg ? -EINVAL : 0;
> >>>
> >>> The type is int, not unsigned int. The same on the rest above.
> >>>
> >>>> +
> >>>>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
> >>>>  }
> >>>>  
> >>>> @@ -1143,6 +1178,14 @@ static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
> >>>>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
> >>>>  		p->capabilities |= V4L2_IN_CAP_STD;
> >>>>  
> >>>> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> >>>> +		if (p->index)
> >>>> +			return -EINVAL;
> >>>> +		strscpy(p->name, vfd->name, sizeof(p->name));
> >>>> +		p->type = V4L2_INPUT_TYPE_CAMERA;
> >>>> +		return 0;
> >>>> +	}
> >>>> +
> >>>>  	return ops->vidioc_enum_input(file, fh, p);
> >>>>  }
> >>>>  
> >>>> @@ -1161,6 +1204,14 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
> >>>>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
> >>>>  		p->capabilities |= V4L2_OUT_CAP_STD;
> >>>>  
> >>>> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> >>>> +		if (p->index)
> >>>> +			return -EINVAL;
> >>>> +		strscpy(p->name, vfd->name, sizeof(p->name));
> >>>> +		p->type = V4L2_OUTPUT_TYPE_ANALOG;
> >>>
> >>> How about adding a new INPUT and OUTPUT types just for IO_MC device cap?
> >>
> >> V4L2_OUTPUT_TYPE_ANALOG is an old term that is in practice used for anything video,
> >> except for TV modulators. I'm all in favor of changing this to:
> >>
> >> #define V4L2_OUTPUT_TYPE_VIDEO 2
> >> #ifndef __KERNEL__
> >> #define V4L2_OUTPUT_TYPE_ANALOG V4L2_OUTPUT_TYPE_VIDEO
> >> #endif
> >>
> >> in videodev2.h, if others agree with that. But that can be done in a separate
> >> follow-up patch since this needs to be adapted throughout the kernel & documentation.
> >>
> >> The same is true for V4L2_INPUT_TYPE_CAMERA: in practice it's used for any video,
> >> except TV tuners.
> > 
> > My point was rather that here, we don't know what kind of device really is
> > the source (or the sink) of the data path. Having a designated input and
> > output type for that would be entirely appropriate.
> 
> But that's exactly what V4L2_OUTPUT_TYPE_ANALOG/V4L2_INPUT_TYPE_CAMERA means:
> it's some kind of video, but we don't know what.

The documentation says only that:

	Any non-modulator video output, for example Composite Video,
	S-Video, HDMI. The naming as ``_TYPE_ANALOG`` is historical,
	today we would have called it ``_TYPE_VIDEO``.

> 
> That's why these names are so misleading: they suggest a particular type,
> but in reality they don't.
> 
> So V4L2_IN/OUTPUT_TYPE_VIDEO is what you want for MC inputs/outputs, but that's
> just an alias for V4L2_INPUT_TYPE_CAMERA/V4L2_OUTPUT_TYPE_ANALOG.

Fine for me then.

-- 
Regards,

Sakari Ailus

Re: [PATCH v4 1/3] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Mar 06, 2020 at 05:39:33PM +0100, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Add a video device capability flag to indicate that its inputs and/or
> outputs are controlled by the Media Controller instead of the V4L2 API.
> When this flag is set, ioctl for enum inputs and outputs are
> automatically enabled and programmed to call a helper function.
> 
> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v3
> - Update documentation for V4L2_CAP_IO_MC
> - Only enable VIDIOC_{G,S}_{INPUT,OUTPUT} if V4L2_CAP_IO_MC is set
>   (instead of unconditionally)
> - Merge v4l2_ioctl_enum_input_mc() into v4l_enuminput()
> - Merge v4l2_ioctl_enum_output_mc() into v4l_enumoutput()
> 
> * Changes since v2
> - Merged the two patches touching V4L2 core stuff to a single patch.
> - Updated documentation for V4L2_CAP_IO_MC
> - Added is_io_mc bool in determine_valid_ioctls()
> - Folded or moved code closer to where it's used
> - Remove unneeded memset()
> - Use strscpy() instead of strlcpy()
> ---
>  .../media/uapi/v4l/vidioc-querycap.rst        |  6 ++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  drivers/media/v4l2-core/v4l2-dev.c            | 25 ++++++--
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 57 +++++++++++++++++--
>  include/uapi/linux/videodev2.h                |  2 +
>  5 files changed, 81 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> index 5f9930195d624c73..497a6aa2cbebad71 100644
> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> @@ -264,6 +264,12 @@ specification the ioctl returns an ``EINVAL`` error code.
>      * - ``V4L2_CAP_TOUCH``
>        - 0x10000000
>        - This is a touch device.
> +    * - ``V4L2_CAP_IO_MC``
> +      - 0x20000000
> +      - There is only one input and/or output seen from userspace. The whole
> +        video topology configuration, including which I/O entity is routed to
> +        the input/output, is configured by userspace via the Media Controller.

I would have disabled those ioctls instead, as they're completely
useless. Now that there's a V4L2_CAP_IO_MC flag, who will call those ?

I however don't care enough to block this, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +        See :ref:`media_controller`.
>      * - ``V4L2_CAP_DEVICE_CAPS``
>        - 0x80000000
>        - The driver fills the ``device_caps`` field. This capability can
> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> index cb6ccf91776e6b56..a625fb90e3a989a7 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -176,6 +176,7 @@ replace define V4L2_CAP_STREAMING device-capabilities
>  replace define V4L2_CAP_META_OUTPUT device-capabilities
>  replace define V4L2_CAP_DEVICE_CAPS device-capabilities
>  replace define V4L2_CAP_TOUCH device-capabilities
> +replace define V4L2_CAP_IO_MC device-capabilities
>  
>  # V4L2 pix flags
>  replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 97b6a3af13614639..a593ea0598b551b4 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -552,6 +552,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  		       (vdev->device_caps & meta_caps);
>  	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
>  	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
> +	bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
>  
>  	bitmap_zero(valid_ioctls, BASE_VIDIOC_PRIVATE);
>  
> @@ -725,9 +726,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  		SET_VALID_IOCTL(ops, VIDIOC_G_STD, vidioc_g_std);
>  		if (is_rx) {
>  			SET_VALID_IOCTL(ops, VIDIOC_QUERYSTD, vidioc_querystd);
> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> -			SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> -			SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> +			if (is_io_mc) {
> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
> +				set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
> +				set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
> +			} else {
> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> +				SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> +				SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> +			}
>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDIO, vidioc_enumaudio);
>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDIO, vidioc_g_audio);
>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDIO, vidioc_s_audio);
> @@ -735,9 +742,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
>  		}
>  		if (is_tx) {
> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
> -			SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
> -			SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
> +			if (is_io_mc) {
> +				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
> +				set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
> +				set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
> +			} else {
> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
> +				SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
> +				SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
> +			}
>  			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
>  			SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
>  			SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index fbcc7a20eedf553a..58e9e728f0a7aa4b 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1085,6 +1085,32 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
>  	return ret;
>  }
>  
> +static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
> +		       struct file *file, void *fh, void *arg)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +
> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> +		*(unsigned int *)arg = 0;
> +		return 0;
> +	}
> +
> +	return ops->vidioc_g_input(file, fh, arg);
> +}
> +
> +static int v4l_g_output(const struct v4l2_ioctl_ops *ops,
> +		       struct file *file, void *fh, void *arg)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +
> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> +		*(unsigned int *)arg = 0;
> +		return 0;
> +	}
> +
> +	return ops->vidioc_g_output(file, fh, arg);
> +}
> +
>  static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -1094,12 +1120,21 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>  	ret = v4l_enable_media_source(vfd);
>  	if (ret)
>  		return ret;
> +
> +	if (vfd->device_caps & V4L2_CAP_IO_MC)
> +		return  *(unsigned int *)arg ? -EINVAL : 0;
> +
>  	return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
>  }
>  
>  static int v4l_s_output(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> +	struct video_device *vfd = video_devdata(file);
> +
> +	if (vfd->device_caps & V4L2_CAP_IO_MC)
> +		return  *(unsigned int *)arg ? -EINVAL : 0;
> +
>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>  }
>  
> @@ -1143,6 +1178,14 @@ static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
>  		p->capabilities |= V4L2_IN_CAP_STD;
>  
> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> +		if (p->index)
> +			return -EINVAL;
> +		strscpy(p->name, vfd->name, sizeof(p->name));
> +		p->type = V4L2_INPUT_TYPE_CAMERA;
> +		return 0;
> +	}
> +
>  	return ops->vidioc_enum_input(file, fh, p);
>  }
>  
> @@ -1161,6 +1204,14 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>  	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
>  		p->capabilities |= V4L2_OUT_CAP_STD;
>  
> +	if (vfd->device_caps & V4L2_CAP_IO_MC) {
> +		if (p->index)
> +			return -EINVAL;
> +		strscpy(p->name, vfd->name, sizeof(p->name));
> +		p->type = V4L2_OUTPUT_TYPE_ANALOG;
> +		return 0;
> +	}
> +
>  	return ops->vidioc_enum_output(file, fh, p);
>  }
>  
> @@ -2678,10 +2729,8 @@ DEFINE_V4L_STUB_FUNC(expbuf)
>  DEFINE_V4L_STUB_FUNC(g_std)
>  DEFINE_V4L_STUB_FUNC(g_audio)
>  DEFINE_V4L_STUB_FUNC(s_audio)
> -DEFINE_V4L_STUB_FUNC(g_input)
>  DEFINE_V4L_STUB_FUNC(g_edid)
>  DEFINE_V4L_STUB_FUNC(s_edid)
> -DEFINE_V4L_STUB_FUNC(g_output)
>  DEFINE_V4L_STUB_FUNC(g_audout)
>  DEFINE_V4L_STUB_FUNC(s_audout)
>  DEFINE_V4L_STUB_FUNC(g_jpegcomp)
> @@ -2730,11 +2779,11 @@ static const struct v4l2_ioctl_info v4l2_ioctls[] = {
>  	IOCTL_INFO(VIDIOC_S_AUDIO, v4l_stub_s_audio, v4l_print_audio, INFO_FL_PRIO),
>  	IOCTL_INFO(VIDIOC_QUERYCTRL, v4l_queryctrl, v4l_print_queryctrl, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_queryctrl, id)),
>  	IOCTL_INFO(VIDIOC_QUERYMENU, v4l_querymenu, v4l_print_querymenu, INFO_FL_CTRL | INFO_FL_CLEAR(v4l2_querymenu, index)),
> -	IOCTL_INFO(VIDIOC_G_INPUT, v4l_stub_g_input, v4l_print_u32, 0),
> +	IOCTL_INFO(VIDIOC_G_INPUT, v4l_g_input, v4l_print_u32, 0),
>  	IOCTL_INFO(VIDIOC_S_INPUT, v4l_s_input, v4l_print_u32, INFO_FL_PRIO),
>  	IOCTL_INFO(VIDIOC_G_EDID, v4l_stub_g_edid, v4l_print_edid, INFO_FL_ALWAYS_COPY),
>  	IOCTL_INFO(VIDIOC_S_EDID, v4l_stub_s_edid, v4l_print_edid, INFO_FL_PRIO | INFO_FL_ALWAYS_COPY),
> -	IOCTL_INFO(VIDIOC_G_OUTPUT, v4l_stub_g_output, v4l_print_u32, 0),
> +	IOCTL_INFO(VIDIOC_G_OUTPUT, v4l_g_output, v4l_print_u32, 0),
>  	IOCTL_INFO(VIDIOC_S_OUTPUT, v4l_s_output, v4l_print_u32, INFO_FL_PRIO),
>  	IOCTL_INFO(VIDIOC_ENUMOUTPUT, v4l_enumoutput, v4l_print_enumoutput, INFO_FL_CLEAR(v4l2_output, index)),
>  	IOCTL_INFO(VIDIOC_G_AUDOUT, v4l_stub_g_audout, v4l_print_audioout, 0),
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 5f9357dcb0602126..c793263a37052856 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -487,6 +487,8 @@ struct v4l2_capability {
>  
>  #define V4L2_CAP_TOUCH                  0x10000000  /* Is a touch device */
>  
> +#define V4L2_CAP_IO_MC			0x20000000  /* Is input/output controlled by the media controller */
> +
>  #define V4L2_CAP_DEVICE_CAPS            0x80000000  /* sets device capabilities field */
>  
>  /*

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/3] rcar-vin: Make use of V4L2_CAP_IO_MC

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Mar 06, 2020 at 05:39:34PM +0100, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Set the V4L2_CAP_IO_MC capability flag and remove the driver specific
> vidioc_enum_input, vidioc_g_input and vidioc_s_input callbacks for the
> media controller enabled part of the driver.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 5151a3cd8a6e6754..96ddd36619167fd5 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -767,18 +767,6 @@ static int rvin_mc_s_fmt_vid_cap(struct file *file, void *priv,
>  	return 0;
>  }
>  
> -static int rvin_mc_enum_input(struct file *file, void *priv,
> -			      struct v4l2_input *i)
> -{
> -	if (i->index != 0)
> -		return -EINVAL;
> -
> -	i->type = V4L2_INPUT_TYPE_CAMERA;
> -	strscpy(i->name, "Camera", sizeof(i->name));
> -
> -	return 0;
> -}
> -
>  static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
>  	.vidioc_querycap		= rvin_querycap,
>  	.vidioc_try_fmt_vid_cap		= rvin_mc_try_fmt_vid_cap,
> @@ -786,10 +774,6 @@ static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
>  	.vidioc_s_fmt_vid_cap		= rvin_mc_s_fmt_vid_cap,
>  	.vidioc_enum_fmt_vid_cap	= rvin_enum_fmt_vid_cap,
>  
> -	.vidioc_enum_input		= rvin_mc_enum_input,
> -	.vidioc_g_input			= rvin_g_input,
> -	.vidioc_s_input			= rvin_s_input,
> -
>  	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
>  	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
>  	.vidioc_querybuf		= vb2_ioctl_querybuf,
> @@ -961,6 +945,7 @@ int rvin_v4l2_register(struct rvin_dev *vin)
>  	vin->format.colorspace = RVIN_DEFAULT_COLORSPACE;
>  
>  	if (vin->info->use_mc) {
> +		vdev->device_caps |= V4L2_CAP_IO_MC;
>  		vdev->ioctl_ops = &rvin_mc_ioctl_ops;
>  	} else {
>  		vdev->ioctl_ops = &rvin_ioctl_ops;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/3] staging/intel-ipu3: Make use of V4L2_CAP_IO_MC

[Laurent Pinchart]

Hello,

On Mon, Mar 09, 2020 at 03:53:18PM +0200, Sakari Ailus wrote:
> On Fri, Mar 06, 2020 at 05:39:35PM +0100, Niklas Söderlund wrote:
> > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > Set the V4L2_CAP_IO_MC capability flag and remove the driver specific
> > vidioc_enum_{input,output}, vidioc_g_{input,output} and
> > vidioc_s_{input,output} callbacks.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/staging/media/ipu3/ipu3-v4l2.c | 60 +-------------------------
> >  1 file changed, 2 insertions(+), 58 deletions(-)
> > 
> > diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
> > index 09c8ede1457cad96..2c42be3d995621e3 100644
> > --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> > +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> > @@ -845,54 +845,6 @@ static int imgu_vidioc_g_meta_fmt(struct file *file, void *fh,
> >  	return 0;
> >  }
> >  
> > -static int imgu_vidioc_enum_input(struct file *file, void *fh,
> > -				  struct v4l2_input *input)
> > -{
> > -	if (input->index > 0)
> > -		return -EINVAL;
> > -	strscpy(input->name, "camera", sizeof(input->name));
> > -	input->type = V4L2_INPUT_TYPE_CAMERA;
> > -
> > -	return 0;
> > -}
> > -
> > -static int imgu_vidioc_g_input(struct file *file, void *fh, unsigned int *input)
> > -{
> > -	*input = 0;
> > -
> > -	return 0;
> > -}
> > -
> > -static int imgu_vidioc_s_input(struct file *file, void *fh, unsigned int input)
> > -{
> > -	return input == 0 ? 0 : -EINVAL;
> > -}
> > -
> > -static int imgu_vidioc_enum_output(struct file *file, void *fh,
> > -				   struct v4l2_output *output)
> > -{
> > -	if (output->index > 0)
> > -		return -EINVAL;
> > -	strscpy(output->name, "camera", sizeof(output->name));
> > -	output->type = V4L2_INPUT_TYPE_CAMERA;
> > -
> > -	return 0;
> > -}
> > -
> > -static int imgu_vidioc_g_output(struct file *file, void *fh,
> > -				unsigned int *output)
> > -{
> > -	*output = 0;
> > -
> > -	return 0;
> > -}
> > -
> > -static int imgu_vidioc_s_output(struct file *file, void *fh,
> > -				unsigned int output)
> > -{
> > -	return output == 0 ? 0 : -EINVAL;
> > -}
> > -
> >  /******************** function pointers ********************/
> >  
> >  static struct v4l2_subdev_internal_ops imgu_subdev_internal_ops = {
> > @@ -965,14 +917,6 @@ static const struct v4l2_ioctl_ops imgu_v4l2_ioctl_ops = {
> >  	.vidioc_s_fmt_vid_out_mplane = imgu_vidioc_s_fmt,
> >  	.vidioc_try_fmt_vid_out_mplane = imgu_vidioc_try_fmt,
> >  
> > -	.vidioc_enum_output = imgu_vidioc_enum_output,
> > -	.vidioc_g_output = imgu_vidioc_g_output,
> > -	.vidioc_s_output = imgu_vidioc_s_output,
> > -
> > -	.vidioc_enum_input = imgu_vidioc_enum_input,
> > -	.vidioc_g_input = imgu_vidioc_g_input,
> > -	.vidioc_s_input = imgu_vidioc_s_input,
> > -
> >  	/* buffer queue management */
> >  	.vidioc_reqbufs = vb2_ioctl_reqbufs,
> >  	.vidioc_create_bufs = vb2_ioctl_create_bufs,
> > @@ -1062,7 +1006,7 @@ static void imgu_node_to_v4l2(u32 node, struct video_device *vdev,
> >  
> >  	switch (node) {
> >  	case IMGU_NODE_IN:
> > -		cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE;
> > +		cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE | V4L2_CAP_IO_MC;
> >  		f->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> >  		vdev->ioctl_ops = &imgu_v4l2_ioctl_ops;
> >  		break;
> > @@ -1081,7 +1025,7 @@ static void imgu_node_to_v4l2(u32 node, struct video_device *vdev,
> >  		imgu_css_meta_fmt_set(&f->fmt.meta);
> >  		break;
> >  	default:
> > -		cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE;
> > +		cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_IO_MC;
> 
> So here, you'd add V4L2_CAP_IO_MC to all kinds of nodes.
> 
> With that,
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

I like that better too. With that change,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >  		f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> >  		vdev->ioctl_ops = &imgu_v4l2_ioctl_ops;
> >  	}

-- 
Regards,

Laurent Pinchart