linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
@ 2020-01-12 23:24 Niklas Söderlund
  2020-01-12 23:24 ` [PATCH v3 1/5] " Niklas Söderlund
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Niklas Söderlund @ 2020-01-12 23:24 UTC (permalink / raw)
  To: Helen Koike, Hans Verkuil, Sakari Ailus, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

Hi,

First of this series is based on work done by Helen in 2017 [1], I have
synced with her that it's OK for me to takeover the work.

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/5 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.  

It also adds a default handler for VIDIOC_{G,S}_{INPUT,OUTPUT} as
suggested by Hans in v1. This allows video drivers (disregarding of the
V4L2_CAP_IO_MC flag) to remove callbacks for these operations if they
only provide a single input/output.

Patch 2/5, 3/5, 4/5 and 5/5 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.

1. https://patchwork.linuxtv.org/patch/41857/

Niklas Söderlund (5):
  v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
  rcar-vin: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler
  staging/intel-ipu3: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler
  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        |  5 ++
 .../media/videodev2.h.rst.exceptions          |  1 +
 drivers/media/platform/rcar-vin/rcar-v4l2.c   | 32 +--------
 drivers/media/v4l2-core/v4l2-dev.c            | 19 +++--
 drivers/media/v4l2-core/v4l2-ioctl.c          | 69 +++++++++++++++++--
 drivers/staging/media/ipu3/ipu3-v4l2.c        | 60 +---------------
 include/uapi/linux/videodev2.h                |  2 +
 7 files changed, 89 insertions(+), 99 deletions(-)

-- 
2.24.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3 1/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
  2020-01-12 23:24 [PATCH v3 0/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
@ 2020-01-12 23:24 ` Niklas Söderlund
  2020-01-13 12:09   ` Sakari Ailus
                     ` (2 more replies)
  2020-01-12 23:24 ` [PATCH v3 2/5] rcar-vin: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler Niklas Söderlund
                   ` (3 subsequent siblings)
  4 siblings, 3 replies; 12+ messages in thread
From: Niklas Söderlund @ 2020-01-12 23:24 UTC (permalink / raw)
  To: Helen Koike, Hans Verkuil, Sakari Ailus, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

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.

As a bonus fallback default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT} are
provided which can be used by drivers who only provide one input/output.
Such drivers need just not declare vidioc_{g,s}_{input,output} in its
struct v4l2_ioctl_ops and v4l2 will fallback to the single input/output
defaults.

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 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        |  5 ++
 .../media/videodev2.h.rst.exceptions          |  1 +
 drivers/media/v4l2-core/v4l2-dev.c            | 19 +++--
 drivers/media/v4l2-core/v4l2-ioctl.c          | 69 +++++++++++++++++--
 include/uapi/linux/videodev2.h                |  2 +
 5 files changed, 86 insertions(+), 10 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
index 5f9930195d624c73..da17e0e3c6f9a571 100644
--- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
+++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
@@ -264,6 +264,11 @@ 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. Which I/O
+        entity is routed to the input/output is controlled by 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 da42d172714adcb5..3c1a1200aa1fa9f3 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,22 +726,28 @@ 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);
+			set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
+			set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
 			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);
 			SET_VALID_IOCTL(ops, VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings);
 			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
+			if (is_io_mc)
+				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
+			else
+				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
 		}
 		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);
+			set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
+			set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
 			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);
+			if (is_io_mc)
+				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
+			else
+				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
 		}
 		if (ops->vidioc_g_parm || ops->vidioc_g_std)
 			set_bit(_IOC_NR(VIDIOC_G_PARM), valid_ioctls);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index aaf83e25427288fd..e8e15deeb8f36a9d 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1085,6 +1085,28 @@ 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)
+{
+	if (!ops->vidioc_g_input) {
+		*(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)
+{
+	if (!ops->vidioc_g_output) {
+		*(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 +1116,19 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
 	ret = v4l_enable_media_source(vfd);
 	if (ret)
 		return ret;
+
+	if (!ops->vidioc_s_input)
+		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)
 {
+	if (!ops->vidioc_s_output)
+		return  *(unsigned int *)arg ? -EINVAL : 0;
+
 	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
 }
 
@@ -1128,6 +1157,20 @@ static int v4l_s_priority(const struct v4l2_ioctl_ops *ops,
 	return v4l2_prio_change(vfd->prio, &vfh->prio, *p);
 }
 
+static int v4l2_ioctl_enum_input_mc(struct file *file, void *priv,
+				    struct v4l2_input *i)
+{
+	struct video_device *vfd = video_devdata(file);
+
+	if (i->index)
+		return -EINVAL;
+
+	strscpy(i->name, vfd->name, sizeof(i->name));
+	i->type = V4L2_INPUT_TYPE_CAMERA;
+
+	return 0;
+}
+
 static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
@@ -1143,9 +1186,26 @@ 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)
+		return v4l2_ioctl_enum_input_mc(file, fh, p);
+
 	return ops->vidioc_enum_input(file, fh, p);
 }
 
+static int v4l2_ioctl_enum_output_mc(struct file *file, void *priv,
+				     struct v4l2_output *o)
+{
+	struct video_device *vfd = video_devdata(file);
+
+	if (o->index)
+		return -EINVAL;
+
+	strscpy(o->name, vfd->name, sizeof(o->name));
+	o->type = V4L2_OUTPUT_TYPE_ANALOG;
+
+	return 0;
+}
+
 static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
@@ -1161,6 +1221,9 @@ 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)
+		return v4l2_ioctl_enum_output_mc(file, fh, p);
+
 	return ops->vidioc_enum_output(file, fh, p);
 }
 
@@ -2678,10 +2741,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 +2791,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.24.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 2/5] rcar-vin: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler
  2020-01-12 23:24 [PATCH v3 0/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
  2020-01-12 23:24 ` [PATCH v3 1/5] " Niklas Söderlund
@ 2020-01-12 23:24 ` Niklas Söderlund
  2020-01-12 23:24 ` [PATCH v3 3/5] staging/intel-ipu3: " Niklas Söderlund
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 12+ messages in thread
From: Niklas Söderlund @ 2020-01-12 23:24 UTC (permalink / raw)
  To: Helen Koike, Hans Verkuil, Sakari Ailus, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

The driver only has a single input, use the default handler.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 9e2e63ffcc47acad..6c6465e61657b390 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -489,19 +489,6 @@ static int rvin_enum_input(struct file *file, void *priv,
 	return 0;
 }
 
-static int rvin_g_input(struct file *file, void *priv, unsigned int *i)
-{
-	*i = 0;
-	return 0;
-}
-
-static int rvin_s_input(struct file *file, void *priv, unsigned int i)
-{
-	if (i > 0)
-		return -EINVAL;
-	return 0;
-}
-
 static int rvin_querystd(struct file *file, void *priv, v4l2_std_id *a)
 {
 	struct rvin_dev *vin = video_drvdata(file);
@@ -667,8 +654,6 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
 	.vidioc_g_pixelaspect		= rvin_g_pixelaspect,
 
 	.vidioc_enum_input		= rvin_enum_input,
-	.vidioc_g_input			= rvin_g_input,
-	.vidioc_s_input			= rvin_s_input,
 
 	.vidioc_dv_timings_cap		= rvin_dv_timings_cap,
 	.vidioc_enum_dv_timings		= rvin_enum_dv_timings,
@@ -771,8 +756,6 @@ static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
 	.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,
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 3/5] staging/intel-ipu3: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler
  2020-01-12 23:24 [PATCH v3 0/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
  2020-01-12 23:24 ` [PATCH v3 1/5] " Niklas Söderlund
  2020-01-12 23:24 ` [PATCH v3 2/5] rcar-vin: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler Niklas Söderlund
@ 2020-01-12 23:24 ` Niklas Söderlund
  2020-01-12 23:24 ` [PATCH v3 4/5] rcar-vin: Make use of V4L2_CAP_IO_MC Niklas Söderlund
  2020-01-12 23:24 ` [PATCH v3 5/5] staging/intel-ipu3: " Niklas Söderlund
  4 siblings, 0 replies; 12+ messages in thread
From: Niklas Söderlund @ 2020-01-12 23:24 UTC (permalink / raw)
  To: Helen Koike, Hans Verkuil, Sakari Ailus, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

The driver only has a single input and output, use the default handler.

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

diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
index 45de77baf08034c9..2f6b5bee39898715 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -854,18 +854,6 @@ static int imgu_vidioc_enum_input(struct file *file, void *fh,
 	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)
 {
@@ -877,20 +865,6 @@ static int imgu_vidioc_enum_output(struct file *file, void *fh,
 	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 = {
@@ -958,12 +932,8 @@ static const struct v4l2_ioctl_ops imgu_v4l2_ioctl_ops = {
 	.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,
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 4/5] rcar-vin: Make use of V4L2_CAP_IO_MC
  2020-01-12 23:24 [PATCH v3 0/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
                   ` (2 preceding siblings ...)
  2020-01-12 23:24 ` [PATCH v3 3/5] staging/intel-ipu3: " Niklas Söderlund
@ 2020-01-12 23:24 ` Niklas Söderlund
  2020-01-12 23:24 ` [PATCH v3 5/5] staging/intel-ipu3: " Niklas Söderlund
  4 siblings, 0 replies; 12+ messages in thread
From: Niklas Söderlund @ 2020-01-12 23:24 UTC (permalink / raw)
  To: Helen Koike, Hans Verkuil, Sakari Ailus, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

Set the V4L2_CAP_IO_MC capability flag and remove the driver specific
enum input callbacks for the media controller enabled mode of the
driver.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 6c6465e61657b390..83f757123ef8f468 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -736,18 +736,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,
@@ -755,8 +743,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_reqbufs			= vb2_ioctl_reqbufs,
 	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
 	.vidioc_querybuf		= vb2_ioctl_querybuf,
@@ -928,6 +914,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.24.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v3 5/5] staging/intel-ipu3: Make use of V4L2_CAP_IO_MC
  2020-01-12 23:24 [PATCH v3 0/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
                   ` (3 preceding siblings ...)
  2020-01-12 23:24 ` [PATCH v3 4/5] rcar-vin: Make use of V4L2_CAP_IO_MC Niklas Söderlund
@ 2020-01-12 23:24 ` Niklas Söderlund
  4 siblings, 0 replies; 12+ messages in thread
From: Niklas Söderlund @ 2020-01-12 23:24 UTC (permalink / raw)
  To: Helen Koike, Hans Verkuil, Sakari Ailus, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

Set the V4L2_CAP_IO_MC capability flag and remove the driver specific
enum input and output callbacks.

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

diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
index 2f6b5bee39898715..e2f48f4c61245309 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -843,28 +843,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_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;
-}
-
 /******************** function pointers ********************/
 
 static struct v4l2_subdev_internal_ops imgu_subdev_internal_ops = {
@@ -931,10 +909,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_enum_input = imgu_vidioc_enum_input,
-
 	/* buffer queue management */
 	.vidioc_reqbufs = vb2_ioctl_reqbufs,
 	.vidioc_create_bufs = vb2_ioctl_create_bufs,
@@ -986,7 +960,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;
@@ -1005,7 +979,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.24.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
  2020-01-12 23:24 ` [PATCH v3 1/5] " Niklas Söderlund
@ 2020-01-13 12:09   ` Sakari Ailus
  2020-02-14 13:14     ` Hans Verkuil
  2020-01-22 21:50   ` Helen Koike
  2020-02-14 13:29   ` Hans Verkuil
  2 siblings, 1 reply; 12+ messages in thread
From: Sakari Ailus @ 2020-01-13 12:09 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Helen Koike, Hans Verkuil, Laurent Pinchart, linux-media,
	linux-renesas-soc

Hejssan!

Tack för lappan!

On Mon, Jan 13, 2020 at 12:24:54AM +0100, Niklas Söderlund wrote:
> 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.
> 
> As a bonus fallback default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT} are
> provided which can be used by drivers who only provide one input/output.
> Such drivers need just not declare vidioc_{g,s}_{input,output} in its
> struct v4l2_ioctl_ops and v4l2 will fallback to the single input/output
> defaults.
> 
> 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 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        |  5 ++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  drivers/media/v4l2-core/v4l2-dev.c            | 19 +++--
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 69 +++++++++++++++++--
>  include/uapi/linux/videodev2.h                |  2 +
>  5 files changed, 86 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> index 5f9930195d624c73..da17e0e3c6f9a571 100644
> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> @@ -264,6 +264,11 @@ 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. Which I/O
> +        entity is routed to the input/output is controlled by the Media
> +        Controller. See :ref:`media_controller`.

I feel we should think of the MC flag in a bit wider context of device
profiles. They've been suggested but I don't think anyone has implemented
any code to support them.

Valid formats, for instance, also are determined by the format
configuration on the MC pipeline in this case.

I wonder if the MC input type (as below) would address this bit, without
going to the device profiles yet?

>      * - ``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 da42d172714adcb5..3c1a1200aa1fa9f3 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,22 +726,28 @@ 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);
> +			set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
> +			set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
>  			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);
>  			SET_VALID_IOCTL(ops, VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings);
>  			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
> +			if (is_io_mc)
> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
> +			else
> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
>  		}
>  		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);
> +			set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
> +			set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
>  			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);
> +			if (is_io_mc)
> +				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
> +			else
> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
>  		}
>  		if (ops->vidioc_g_parm || ops->vidioc_g_std)
>  			set_bit(_IOC_NR(VIDIOC_G_PARM), valid_ioctls);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index aaf83e25427288fd..e8e15deeb8f36a9d 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1085,6 +1085,28 @@ 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)
> +{
> +	if (!ops->vidioc_g_input) {
> +		*(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)
> +{
> +	if (!ops->vidioc_g_output) {
> +		*(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 +1116,19 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>  	ret = v4l_enable_media_source(vfd);
>  	if (ret)
>  		return ret;
> +
> +	if (!ops->vidioc_s_input)
> +		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)
>  {
> +	if (!ops->vidioc_s_output)
> +		return  *(unsigned int *)arg ? -EINVAL : 0;
> +
>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>  }
>  
> @@ -1128,6 +1157,20 @@ static int v4l_s_priority(const struct v4l2_ioctl_ops *ops,
>  	return v4l2_prio_change(vfd->prio, &vfh->prio, *p);
>  }
>  
> +static int v4l2_ioctl_enum_input_mc(struct file *file, void *priv,
> +				    struct v4l2_input *i)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +
> +	if (i->index)
> +		return -EINVAL;
> +
> +	strscpy(i->name, vfd->name, sizeof(i->name));
> +	i->type = V4L2_INPUT_TYPE_CAMERA;

Doesn't this deserve its own input type? Say, V4L2_INPUT_TYPE_MC, for
instance?

Most MC-enabled drivers are related to cameras but you can attach e.g. a TV
tuner to at least some of them. The API is really not about cameras as such
either.

> +
> +	return 0;
> +}
> +
>  static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -1143,9 +1186,26 @@ 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)
> +		return v4l2_ioctl_enum_input_mc(file, fh, p);
> +
>  	return ops->vidioc_enum_input(file, fh, p);
>  }
>  
> +static int v4l2_ioctl_enum_output_mc(struct file *file, void *priv,
> +				     struct v4l2_output *o)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +
> +	if (o->index)
> +		return -EINVAL;
> +
> +	strscpy(o->name, vfd->name, sizeof(o->name));
> +	o->type = V4L2_OUTPUT_TYPE_ANALOG;
> +
> +	return 0;
> +}
> +
>  static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -1161,6 +1221,9 @@ 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)
> +		return v4l2_ioctl_enum_output_mc(file, fh, p);
> +
>  	return ops->vidioc_enum_output(file, fh, p);
>  }
>  
> @@ -2678,10 +2741,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 +2791,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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
  2020-01-12 23:24 ` [PATCH v3 1/5] " Niklas Söderlund
  2020-01-13 12:09   ` Sakari Ailus
@ 2020-01-22 21:50   ` Helen Koike
  2020-02-14 13:25     ` Hans Verkuil
  2020-02-14 13:29   ` Hans Verkuil
  2 siblings, 1 reply; 12+ messages in thread
From: Helen Koike @ 2020-01-22 21:50 UTC (permalink / raw)
  To: Niklas Söderlund, Hans Verkuil, Sakari Ailus,
	Laurent Pinchart, linux-media
  Cc: linux-renesas-soc

Hi Niklas,

On 1/12/20 9:24 PM, Niklas Söderlund wrote:
> 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.
> 
> As a bonus fallback default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT} are
> provided which can be used by drivers who only provide one input/output.
> Such drivers need just not declare vidioc_{g,s}_{input,output} in its
> struct v4l2_ioctl_ops and v4l2 will fallback to the single input/output
> defaults.
> 
> 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 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        |  5 ++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  drivers/media/v4l2-core/v4l2-dev.c            | 19 +++--
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 69 +++++++++++++++++--
>  include/uapi/linux/videodev2.h                |  2 +
>  5 files changed, 86 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> index 5f9930195d624c73..da17e0e3c6f9a571 100644
> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> @@ -264,6 +264,11 @@ 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. Which I/O
> +        entity is routed to the input/output is controlled by 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 da42d172714adcb5..3c1a1200aa1fa9f3 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,22 +726,28 @@ 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);
> +			set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
> +			set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);

Maybe I'm reading this wrong, but from what I understand, in case where
(!is_io_mc && !ops->vidioc_enum_input) is true, shouldn't {G,S}_INPUT be invalid?
Same for output.

Also, if this is a MC device, the default with your patch is to enumerate one input and one output right?
I'm just wondering if vimc should be updated too, as it doesn't implement these ioctls.
(or other drivers who doesn't implement them).

Maybe I understood this wrong (very likely), but does it make sense to enumerate
output in a Capture node? Or to enumerate input in an Output node?

Regards,
Helen

>  			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);
>  			SET_VALID_IOCTL(ops, VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings);
>  			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
> +			if (is_io_mc)
> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
> +			else
> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
>  		}
>  		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);
> +			set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
> +			set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
>  			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);
> +			if (is_io_mc)
> +				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
> +			else
> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
>  		}
>  		if (ops->vidioc_g_parm || ops->vidioc_g_std)
>  			set_bit(_IOC_NR(VIDIOC_G_PARM), valid_ioctls);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index aaf83e25427288fd..e8e15deeb8f36a9d 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1085,6 +1085,28 @@ 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)
> +{
> +	if (!ops->vidioc_g_input) {
> +		*(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)
> +{
> +	if (!ops->vidioc_g_output) {
> +		*(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 +1116,19 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>  	ret = v4l_enable_media_source(vfd);
>  	if (ret)
>  		return ret;
> +
> +	if (!ops->vidioc_s_input)
> +		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)
>  {
> +	if (!ops->vidioc_s_output)
> +		return  *(unsigned int *)arg ? -EINVAL : 0;
> +
>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>  }
>  
> @@ -1128,6 +1157,20 @@ static int v4l_s_priority(const struct v4l2_ioctl_ops *ops,
>  	return v4l2_prio_change(vfd->prio, &vfh->prio, *p);
>  }
>  
> +static int v4l2_ioctl_enum_input_mc(struct file *file, void *priv,
> +				    struct v4l2_input *i)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +
> +	if (i->index)
> +		return -EINVAL;
> +
> +	strscpy(i->name, vfd->name, sizeof(i->name));
> +	i->type = V4L2_INPUT_TYPE_CAMERA;
> +
> +	return 0;
> +}
> +
>  static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -1143,9 +1186,26 @@ 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)
> +		return v4l2_ioctl_enum_input_mc(file, fh, p);
> +
>  	return ops->vidioc_enum_input(file, fh, p);
>  }
>  
> +static int v4l2_ioctl_enum_output_mc(struct file *file, void *priv,
> +				     struct v4l2_output *o)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +
> +	if (o->index)
> +		return -EINVAL;
> +
> +	strscpy(o->name, vfd->name, sizeof(o->name));
> +	o->type = V4L2_OUTPUT_TYPE_ANALOG;
> +
> +	return 0;
> +}
> +
>  static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -1161,6 +1221,9 @@ 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)
> +		return v4l2_ioctl_enum_output_mc(file, fh, p);
> +
>  	return ops->vidioc_enum_output(file, fh, p);
>  }
>  
> @@ -2678,10 +2741,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 +2791,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 */
>  
>  /*
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
  2020-01-13 12:09   ` Sakari Ailus
@ 2020-02-14 13:14     ` Hans Verkuil
  0 siblings, 0 replies; 12+ messages in thread
From: Hans Verkuil @ 2020-02-14 13:14 UTC (permalink / raw)
  To: Sakari Ailus, Niklas Söderlund
  Cc: Helen Koike, Laurent Pinchart, linux-media, linux-renesas-soc

On 1/13/20 1:09 PM, Sakari Ailus wrote:
> Hejssan!
> 
> Tack för lappan!
> 
> On Mon, Jan 13, 2020 at 12:24:54AM +0100, Niklas Söderlund wrote:
>> 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.
>>
>> As a bonus fallback default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT} are
>> provided which can be used by drivers who only provide one input/output.
>> Such drivers need just not declare vidioc_{g,s}_{input,output} in its
>> struct v4l2_ioctl_ops and v4l2 will fallback to the single input/output
>> defaults.
>>
>> 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 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        |  5 ++
>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>  drivers/media/v4l2-core/v4l2-dev.c            | 19 +++--
>>  drivers/media/v4l2-core/v4l2-ioctl.c          | 69 +++++++++++++++++--
>>  include/uapi/linux/videodev2.h                |  2 +
>>  5 files changed, 86 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> index 5f9930195d624c73..da17e0e3c6f9a571 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> @@ -264,6 +264,11 @@ 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. Which I/O
>> +        entity is routed to the input/output is controlled by the Media
>> +        Controller. See :ref:`media_controller`.
> 
> I feel we should think of the MC flag in a bit wider context of device
> profiles. They've been suggested but I don't think anyone has implemented
> any code to support them.

There is implicit code: i.e. v4l2-dev.c does ever more checking of which ioctls
should be disabled based on the device capabilities, and v4l2-compliance is also
checking that. But nothing of that is really written down.

> Valid formats, for instance, also are determined by the format
> configuration on the MC pipeline in this case.

How about this:

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

That makes it more explicit that 1) it is userspace that configures this, and
2) that there is a lot more that is configured besides the routing.

> 
> I wonder if the MC input type (as below) would address this bit, without
> going to the device profiles yet?
> 
>>      * - ``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 da42d172714adcb5..3c1a1200aa1fa9f3 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,22 +726,28 @@ 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);
>> +			set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
>> +			set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
>>  			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);
>>  			SET_VALID_IOCTL(ops, VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings);
>>  			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
>> +			if (is_io_mc)
>> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
>> +			else
>> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
>>  		}
>>  		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);
>> +			set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
>> +			set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
>>  			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);
>> +			if (is_io_mc)
>> +				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
>> +			else
>> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
>>  		}
>>  		if (ops->vidioc_g_parm || ops->vidioc_g_std)
>>  			set_bit(_IOC_NR(VIDIOC_G_PARM), valid_ioctls);
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index aaf83e25427288fd..e8e15deeb8f36a9d 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -1085,6 +1085,28 @@ 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)
>> +{
>> +	if (!ops->vidioc_g_input) {
>> +		*(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)
>> +{
>> +	if (!ops->vidioc_g_output) {
>> +		*(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 +1116,19 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>>  	ret = v4l_enable_media_source(vfd);
>>  	if (ret)
>>  		return ret;
>> +
>> +	if (!ops->vidioc_s_input)
>> +		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)
>>  {
>> +	if (!ops->vidioc_s_output)
>> +		return  *(unsigned int *)arg ? -EINVAL : 0;
>> +
>>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>>  }
>>  
>> @@ -1128,6 +1157,20 @@ static int v4l_s_priority(const struct v4l2_ioctl_ops *ops,
>>  	return v4l2_prio_change(vfd->prio, &vfh->prio, *p);
>>  }
>>  
>> +static int v4l2_ioctl_enum_input_mc(struct file *file, void *priv,
>> +				    struct v4l2_input *i)
>> +{
>> +	struct video_device *vfd = video_devdata(file);
>> +
>> +	if (i->index)
>> +		return -EINVAL;
>> +
>> +	strscpy(i->name, vfd->name, sizeof(i->name));
>> +	i->type = V4L2_INPUT_TYPE_CAMERA;
> 
> Doesn't this deserve its own input type? Say, V4L2_INPUT_TYPE_MC, for
> instance?

I think that will break the ABI. It's always been TYPE_CAMERA.

> Most MC-enabled drivers are related to cameras but you can attach e.g. a TV
> tuner to at least some of them. The API is really not about cameras as such
> either.

It's really an historical accident. I think I proposed at some time to add an
alias:

#define V4L2_INPUT_TYPE_VIDEO V4L2_INPUT_TYPE_CAMERA

Since 'Camera' really is used as a 'Video' input which may come from various
sources (sensors, video receivers, etc.).

Regards,

	Hans

> 
>> +
>> +	return 0;
>> +}
>> +
>>  static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
>>  				struct file *file, void *fh, void *arg)
>>  {
>> @@ -1143,9 +1186,26 @@ 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)
>> +		return v4l2_ioctl_enum_input_mc(file, fh, p);
>> +
>>  	return ops->vidioc_enum_input(file, fh, p);
>>  }
>>  
>> +static int v4l2_ioctl_enum_output_mc(struct file *file, void *priv,
>> +				     struct v4l2_output *o)
>> +{
>> +	struct video_device *vfd = video_devdata(file);
>> +
>> +	if (o->index)
>> +		return -EINVAL;
>> +
>> +	strscpy(o->name, vfd->name, sizeof(o->name));
>> +	o->type = V4L2_OUTPUT_TYPE_ANALOG;
>> +
>> +	return 0;
>> +}
>> +
>>  static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>>  				struct file *file, void *fh, void *arg)
>>  {
>> @@ -1161,6 +1221,9 @@ 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)
>> +		return v4l2_ioctl_enum_output_mc(file, fh, p);
>> +
>>  	return ops->vidioc_enum_output(file, fh, p);
>>  }
>>  
>> @@ -2678,10 +2741,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 +2791,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 */
>>  
>>  /*
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
  2020-01-22 21:50   ` Helen Koike
@ 2020-02-14 13:25     ` Hans Verkuil
  2020-02-14 14:06       ` Helen Koike
  0 siblings, 1 reply; 12+ messages in thread
From: Hans Verkuil @ 2020-02-14 13:25 UTC (permalink / raw)
  To: Helen Koike, Niklas Söderlund, Sakari Ailus,
	Laurent Pinchart, linux-media
  Cc: linux-renesas-soc

On 1/22/20 10:50 PM, Helen Koike wrote:
> Hi Niklas,
> 
> On 1/12/20 9:24 PM, Niklas Söderlund wrote:
>> 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.
>>
>> As a bonus fallback default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT} are
>> provided which can be used by drivers who only provide one input/output.
>> Such drivers need just not declare vidioc_{g,s}_{input,output} in its
>> struct v4l2_ioctl_ops and v4l2 will fallback to the single input/output
>> defaults.
>>
>> 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 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        |  5 ++
>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>  drivers/media/v4l2-core/v4l2-dev.c            | 19 +++--
>>  drivers/media/v4l2-core/v4l2-ioctl.c          | 69 +++++++++++++++++--
>>  include/uapi/linux/videodev2.h                |  2 +
>>  5 files changed, 86 insertions(+), 10 deletions(-)
>>
>> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> index 5f9930195d624c73..da17e0e3c6f9a571 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
>> @@ -264,6 +264,11 @@ 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. Which I/O
>> +        entity is routed to the input/output is controlled by 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 da42d172714adcb5..3c1a1200aa1fa9f3 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,22 +726,28 @@ 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);
>> +			set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
>> +			set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
> 
> Maybe I'm reading this wrong, but from what I understand, in case where
> (!is_io_mc && !ops->vidioc_enum_input) is true, shouldn't {G,S}_INPUT be invalid?
> Same for output.

Yup, that's right.

Niklas, why not just do:

			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);
			}

and ditto for output.

> 
> Also, if this is a MC device, the default with your patch is to enumerate one input and one output right?
> I'm just wondering if vimc should be updated too, as it doesn't implement these ioctls.
> (or other drivers who doesn't implement them).

Yes, vimc should certainly be updated.

> 
> Maybe I understood this wrong (very likely), but does it make sense to enumerate
> output in a Capture node? Or to enumerate input in an Output node?

No, that doesn't make sense. But why do you ask? That doesn't happen here, or am I
missing something?

Regards,

	Hans

> 
> Regards,
> Helen
> 
>>  			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);
>>  			SET_VALID_IOCTL(ops, VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings);
>>  			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
>> +			if (is_io_mc)
>> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
>> +			else
>> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
>>  		}
>>  		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);
>> +			set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
>> +			set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
>>  			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);
>> +			if (is_io_mc)
>> +				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
>> +			else
>> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
>>  		}
>>  		if (ops->vidioc_g_parm || ops->vidioc_g_std)
>>  			set_bit(_IOC_NR(VIDIOC_G_PARM), valid_ioctls);
>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>> index aaf83e25427288fd..e8e15deeb8f36a9d 100644
>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>> @@ -1085,6 +1085,28 @@ 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)
>> +{
>> +	if (!ops->vidioc_g_input) {
>> +		*(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)
>> +{
>> +	if (!ops->vidioc_g_output) {
>> +		*(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 +1116,19 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>>  	ret = v4l_enable_media_source(vfd);
>>  	if (ret)
>>  		return ret;
>> +
>> +	if (!ops->vidioc_s_input)
>> +		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)
>>  {
>> +	if (!ops->vidioc_s_output)
>> +		return  *(unsigned int *)arg ? -EINVAL : 0;
>> +
>>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>>  }
>>  
>> @@ -1128,6 +1157,20 @@ static int v4l_s_priority(const struct v4l2_ioctl_ops *ops,
>>  	return v4l2_prio_change(vfd->prio, &vfh->prio, *p);
>>  }
>>  
>> +static int v4l2_ioctl_enum_input_mc(struct file *file, void *priv,
>> +				    struct v4l2_input *i)
>> +{
>> +	struct video_device *vfd = video_devdata(file);
>> +
>> +	if (i->index)
>> +		return -EINVAL;
>> +
>> +	strscpy(i->name, vfd->name, sizeof(i->name));
>> +	i->type = V4L2_INPUT_TYPE_CAMERA;
>> +
>> +	return 0;
>> +}
>> +
>>  static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
>>  				struct file *file, void *fh, void *arg)
>>  {
>> @@ -1143,9 +1186,26 @@ 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)
>> +		return v4l2_ioctl_enum_input_mc(file, fh, p);
>> +
>>  	return ops->vidioc_enum_input(file, fh, p);
>>  }
>>  
>> +static int v4l2_ioctl_enum_output_mc(struct file *file, void *priv,
>> +				     struct v4l2_output *o)
>> +{
>> +	struct video_device *vfd = video_devdata(file);
>> +
>> +	if (o->index)
>> +		return -EINVAL;
>> +
>> +	strscpy(o->name, vfd->name, sizeof(o->name));
>> +	o->type = V4L2_OUTPUT_TYPE_ANALOG;
>> +
>> +	return 0;
>> +}
>> +
>>  static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>>  				struct file *file, void *fh, void *arg)
>>  {
>> @@ -1161,6 +1221,9 @@ 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)
>> +		return v4l2_ioctl_enum_output_mc(file, fh, p);
>> +
>>  	return ops->vidioc_enum_output(file, fh, p);
>>  }
>>  
>> @@ -2678,10 +2741,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 +2791,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 */
>>  
>>  /*
>>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
  2020-01-12 23:24 ` [PATCH v3 1/5] " Niklas Söderlund
  2020-01-13 12:09   ` Sakari Ailus
  2020-01-22 21:50   ` Helen Koike
@ 2020-02-14 13:29   ` Hans Verkuil
  2 siblings, 0 replies; 12+ messages in thread
From: Hans Verkuil @ 2020-02-14 13:29 UTC (permalink / raw)
  To: Niklas Söderlund, Helen Koike, Sakari Ailus,
	Laurent Pinchart, linux-media
  Cc: linux-renesas-soc

On 1/13/20 12:24 AM, Niklas Söderlund wrote:
> 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.
> 
> As a bonus fallback default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT} are
> provided which can be used by drivers who only provide one input/output.
> Such drivers need just not declare vidioc_{g,s}_{input,output} in its
> struct v4l2_ioctl_ops and v4l2 will fallback to the single input/output
> defaults.
> 
> 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 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        |  5 ++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  drivers/media/v4l2-core/v4l2-dev.c            | 19 +++--
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 69 +++++++++++++++++--
>  include/uapi/linux/videodev2.h                |  2 +
>  5 files changed, 86 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> index 5f9930195d624c73..da17e0e3c6f9a571 100644
> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> @@ -264,6 +264,11 @@ 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. Which I/O
> +        entity is routed to the input/output is controlled by 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 da42d172714adcb5..3c1a1200aa1fa9f3 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,22 +726,28 @@ 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);
> +			set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
> +			set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
>  			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);
>  			SET_VALID_IOCTL(ops, VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings);
>  			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
> +			if (is_io_mc)
> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
> +			else
> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
>  		}
>  		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);
> +			set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
> +			set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
>  			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);
> +			if (is_io_mc)
> +				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
> +			else
> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
>  		}
>  		if (ops->vidioc_g_parm || ops->vidioc_g_std)
>  			set_bit(_IOC_NR(VIDIOC_G_PARM), valid_ioctls);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index aaf83e25427288fd..e8e15deeb8f36a9d 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1085,6 +1085,28 @@ 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)
> +{
> +	if (!ops->vidioc_g_input) {
> +		*(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)
> +{
> +	if (!ops->vidioc_g_output) {
> +		*(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 +1116,19 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>  	ret = v4l_enable_media_source(vfd);
>  	if (ret)
>  		return ret;
> +
> +	if (!ops->vidioc_s_input)
> +		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)
>  {
> +	if (!ops->vidioc_s_output)
> +		return  *(unsigned int *)arg ? -EINVAL : 0;
> +
>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>  }
>  
> @@ -1128,6 +1157,20 @@ static int v4l_s_priority(const struct v4l2_ioctl_ops *ops,
>  	return v4l2_prio_change(vfd->prio, &vfh->prio, *p);
>  }
>  
> +static int v4l2_ioctl_enum_input_mc(struct file *file, void *priv,
> +				    struct v4l2_input *i)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +
> +	if (i->index)
> +		return -EINVAL;
> +
> +	strscpy(i->name, vfd->name, sizeof(i->name));
> +	i->type = V4L2_INPUT_TYPE_CAMERA;
> +
> +	return 0;
> +}
> +
>  static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -1143,9 +1186,26 @@ 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)
> +		return v4l2_ioctl_enum_input_mc(file, fh, p);

I really think v4l2_ioctl_enum_input_mc() should be merged here:

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

and ditto for v4l_enumoutput. I think it is easier to understand and more
compact code.

Regards,

	Hans

> +
>  	return ops->vidioc_enum_input(file, fh, p);
>  }
>  
> +static int v4l2_ioctl_enum_output_mc(struct file *file, void *priv,
> +				     struct v4l2_output *o)
> +{
> +	struct video_device *vfd = video_devdata(file);
> +
> +	if (o->index)
> +		return -EINVAL;
> +
> +	strscpy(o->name, vfd->name, sizeof(o->name));
> +	o->type = V4L2_OUTPUT_TYPE_ANALOG;
> +
> +	return 0;
> +}
> +
>  static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> @@ -1161,6 +1221,9 @@ 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)
> +		return v4l2_ioctl_enum_output_mc(file, fh, p);
> +
>  	return ops->vidioc_enum_output(file, fh, p);
>  }
>  
> @@ -2678,10 +2741,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 +2791,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 */
>  
>  /*
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3 1/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
  2020-02-14 13:25     ` Hans Verkuil
@ 2020-02-14 14:06       ` Helen Koike
  0 siblings, 0 replies; 12+ messages in thread
From: Helen Koike @ 2020-02-14 14:06 UTC (permalink / raw)
  To: Hans Verkuil, Niklas Söderlund, Sakari Ailus,
	Laurent Pinchart, linux-media
  Cc: linux-renesas-soc



On 2/14/20 11:25 AM, Hans Verkuil wrote:
> On 1/22/20 10:50 PM, Helen Koike wrote:
>> Hi Niklas,
>>
>> On 1/12/20 9:24 PM, Niklas Söderlund wrote:
>>> 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.
>>>
>>> As a bonus fallback default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT} are
>>> provided which can be used by drivers who only provide one input/output.
>>> Such drivers need just not declare vidioc_{g,s}_{input,output} in its
>>> struct v4l2_ioctl_ops and v4l2 will fallback to the single input/output
>>> defaults.
>>>
>>> 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 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        |  5 ++
>>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>>  drivers/media/v4l2-core/v4l2-dev.c            | 19 +++--
>>>  drivers/media/v4l2-core/v4l2-ioctl.c          | 69 +++++++++++++++++--
>>>  include/uapi/linux/videodev2.h                |  2 +
>>>  5 files changed, 86 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
>>> index 5f9930195d624c73..da17e0e3c6f9a571 100644
>>> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
>>> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
>>> @@ -264,6 +264,11 @@ 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. Which I/O
>>> +        entity is routed to the input/output is controlled by 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 da42d172714adcb5..3c1a1200aa1fa9f3 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,22 +726,28 @@ 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);
>>> +			set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
>>> +			set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
>>
>> Maybe I'm reading this wrong, but from what I understand, in case where
>> (!is_io_mc && !ops->vidioc_enum_input) is true, shouldn't {G,S}_INPUT be invalid?
>> Same for output.
> 
> Yup, that's right.
> 
> Niklas, why not just do:
> 
> 			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);
> 			}
> 
> and ditto for output.
> 
>>
>> Also, if this is a MC device, the default with your patch is to enumerate one input and one output right?
>> I'm just wondering if vimc should be updated too, as it doesn't implement these ioctls.
>> (or other drivers who doesn't implement them).
> 
> Yes, vimc should certainly be updated.
> 
>>
>> Maybe I understood this wrong (very likely), but does it make sense to enumerate
>> output in a Capture node? Or to enumerate input in an Output node?
> 
> No, that doesn't make sense. But why do you ask? That doesn't happen here, or am I
> missing something?

Please ignore, I misread something.

Regards,
Helen

> 
> Regards,
> 
> 	Hans
> 
>>
>> Regards,
>> Helen
>>
>>>  			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);
>>>  			SET_VALID_IOCTL(ops, VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings);
>>>  			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
>>> +			if (is_io_mc)
>>> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
>>> +			else
>>> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
>>>  		}
>>>  		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);
>>> +			set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
>>> +			set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
>>>  			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);
>>> +			if (is_io_mc)
>>> +				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
>>> +			else
>>> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
>>>  		}
>>>  		if (ops->vidioc_g_parm || ops->vidioc_g_std)
>>>  			set_bit(_IOC_NR(VIDIOC_G_PARM), valid_ioctls);
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index aaf83e25427288fd..e8e15deeb8f36a9d 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -1085,6 +1085,28 @@ 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)
>>> +{
>>> +	if (!ops->vidioc_g_input) {
>>> +		*(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)
>>> +{
>>> +	if (!ops->vidioc_g_output) {
>>> +		*(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 +1116,19 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
>>>  	ret = v4l_enable_media_source(vfd);
>>>  	if (ret)
>>>  		return ret;
>>> +
>>> +	if (!ops->vidioc_s_input)
>>> +		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)
>>>  {
>>> +	if (!ops->vidioc_s_output)
>>> +		return  *(unsigned int *)arg ? -EINVAL : 0;
>>> +
>>>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>>>  }
>>>  
>>> @@ -1128,6 +1157,20 @@ static int v4l_s_priority(const struct v4l2_ioctl_ops *ops,
>>>  	return v4l2_prio_change(vfd->prio, &vfh->prio, *p);
>>>  }
>>>  
>>> +static int v4l2_ioctl_enum_input_mc(struct file *file, void *priv,
>>> +				    struct v4l2_input *i)
>>> +{
>>> +	struct video_device *vfd = video_devdata(file);
>>> +
>>> +	if (i->index)
>>> +		return -EINVAL;
>>> +
>>> +	strscpy(i->name, vfd->name, sizeof(i->name));
>>> +	i->type = V4L2_INPUT_TYPE_CAMERA;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
>>>  				struct file *file, void *fh, void *arg)
>>>  {
>>> @@ -1143,9 +1186,26 @@ 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)
>>> +		return v4l2_ioctl_enum_input_mc(file, fh, p);
>>> +
>>>  	return ops->vidioc_enum_input(file, fh, p);
>>>  }
>>>  
>>> +static int v4l2_ioctl_enum_output_mc(struct file *file, void *priv,
>>> +				     struct v4l2_output *o)
>>> +{
>>> +	struct video_device *vfd = video_devdata(file);
>>> +
>>> +	if (o->index)
>>> +		return -EINVAL;
>>> +
>>> +	strscpy(o->name, vfd->name, sizeof(o->name));
>>> +	o->type = V4L2_OUTPUT_TYPE_ANALOG;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
>>>  				struct file *file, void *fh, void *arg)
>>>  {
>>> @@ -1161,6 +1221,9 @@ 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)
>>> +		return v4l2_ioctl_enum_output_mc(file, fh, p);
>>> +
>>>  	return ops->vidioc_enum_output(file, fh, p);
>>>  }
>>>  
>>> @@ -2678,10 +2741,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 +2791,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 */
>>>  
>>>  /*
>>>
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-02-14 14:06 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-12 23:24 [PATCH v3 0/5] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
2020-01-12 23:24 ` [PATCH v3 1/5] " Niklas Söderlund
2020-01-13 12:09   ` Sakari Ailus
2020-02-14 13:14     ` Hans Verkuil
2020-01-22 21:50   ` Helen Koike
2020-02-14 13:25     ` Hans Verkuil
2020-02-14 14:06       ` Helen Koike
2020-02-14 13:29   ` Hans Verkuil
2020-01-12 23:24 ` [PATCH v3 2/5] rcar-vin: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler Niklas Söderlund
2020-01-12 23:24 ` [PATCH v3 3/5] staging/intel-ipu3: " Niklas Söderlund
2020-01-12 23:24 ` [PATCH v3 4/5] rcar-vin: Make use of V4L2_CAP_IO_MC Niklas Söderlund
2020-01-12 23:24 ` [PATCH v3 5/5] staging/intel-ipu3: " Niklas Söderlund

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).