All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
@ 2019-11-15 23:55 Niklas Söderlund
  2019-11-15 23:55 ` [PATCH v2 1/6] v4l2-dev/ioctl: Add default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT} Niklas Söderlund
                   ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: Niklas Söderlund @ 2019-11-15 23:55 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/6 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 3/6 adds the core parts of this change by adding a new video
device capability flag V4L2_CAP_IO_MC which if set provides helper
implementations for the enum inputs and outputs ioctrls freeing the 
video device driver from the need to implement them.

Patch 2/6, 3/6, 5/6 and 6/6 converts the R-Car VIN and Intel IPU3 
drivers to use the new default handlers  and 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 (6):
  v4l2-dev/ioctl: Add default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT}
  rcar-vin: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler
  staging/intel-ipu3: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler
  v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
  rcar-vin: Make use of V4L2_CAP_IO_MC
  staging/intel-ipu3: Make use of V4L2_CAP_IO_MC

 .../media/uapi/v4l/vidioc-querycap.rst        |  3 +
 .../media/videodev2.h.rst.exceptions          |  1 +
 drivers/media/platform/rcar-vin/rcar-v4l2.c   | 32 +-------
 drivers/media/v4l2-core/v4l2-dev.c            | 18 +++--
 drivers/media/v4l2-core/v4l2-ioctl.c          | 80 ++++++++++++++++++-
 drivers/staging/media/ipu3/ipu3-v4l2.c        | 60 +-------------
 include/uapi/linux/videodev2.h                |  2 +
 7 files changed, 97 insertions(+), 99 deletions(-)

-- 
2.24.0


^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH v2 1/6] v4l2-dev/ioctl: Add default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT}
  2019-11-15 23:55 [PATCH v2 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
@ 2019-11-15 23:55 ` Niklas Söderlund
  2019-11-16  9:19   ` Hans Verkuil
  2019-11-15 23:55 ` [PATCH v2 2/6] rcar-vin: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler Niklas Söderlund
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 9+ messages in thread
From: Niklas Söderlund @ 2019-11-15 23:55 UTC (permalink / raw)
  To: Helen Koike, Hans Verkuil, Sakari Ailus, Laurent Pinchart, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

It is very common to have just one input/output, and many drivers
implements the same trivial handlers. Add default handlers for
VIDIOC_{G,S}_{INPUT,OUTPUT} that assumes a single input/output.

Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/v4l2-core/v4l2-dev.c   |  8 ++---
 drivers/media/v4l2-core/v4l2-ioctl.c | 44 +++++++++++++++++++++++++---
 2 files changed, 44 insertions(+), 8 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index da42d172714adcb5..4293df8d664f70b3 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -726,8 +726,8 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		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);
@@ -736,8 +736,8 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		}
 		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);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 4e700583659bac8c..4a461de28677c5a8 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1085,6 +1085,37 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
 	return ret;
 }
 
+static int v4l2_ioctl_g_single_input(struct file *file, void *priv, unsigned int *i)
+{
+	*i = 0;
+	return 0;
+}
+#define v4l2_ioctl_g_single_output v4l2_ioctl_g_single_input
+
+static int v4l2_ioctl_s_single_input(struct file *file, void *priv, unsigned int i)
+{
+	return i ? -EINVAL : 0;
+}
+#define v4l2_ioctl_s_single_output v4l2_ioctl_s_single_input
+
+static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
+		       struct file *file, void *fh, void *arg)
+{
+	if (!ops->vidioc_g_input)
+		return v4l2_ioctl_g_single_input(file, fh, arg);
+
+	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)
+		return v4l2_ioctl_g_single_output(file, fh, arg);
+
+	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 +1125,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 v4l2_ioctl_s_single_input(file, fh, *(unsigned int *)arg);
+
 	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 v4l2_ioctl_s_single_output(file, fh, *(unsigned int *)arg);
+
 	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
 }
 
@@ -2678,10 +2716,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 +2766,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),
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH v2 2/6] rcar-vin: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler
  2019-11-15 23:55 [PATCH v2 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
  2019-11-15 23:55 ` [PATCH v2 1/6] v4l2-dev/ioctl: Add default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT} Niklas Söderlund
@ 2019-11-15 23:55 ` Niklas Söderlund
  2019-11-15 23:55 ` [PATCH v2 3/6] staging/intel-ipu3: " Niklas Söderlund
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Niklas Söderlund @ 2019-11-15 23:55 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.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH v2 3/6] staging/intel-ipu3: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler
  2019-11-15 23:55 [PATCH v2 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
  2019-11-15 23:55 ` [PATCH v2 1/6] v4l2-dev/ioctl: Add default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT} Niklas Söderlund
  2019-11-15 23:55 ` [PATCH v2 2/6] rcar-vin: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler Niklas Söderlund
@ 2019-11-15 23:55 ` Niklas Söderlund
  2019-11-15 23:55 ` [PATCH v2 4/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Niklas Söderlund @ 2019-11-15 23:55 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 3c7ad1eed4343a2a..5f8ad5996aa37db4 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -856,18 +856,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)
 {
@@ -879,20 +867,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 = {
@@ -966,12 +940,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.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH v2 4/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
  2019-11-15 23:55 [PATCH v2 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
                   ` (2 preceding siblings ...)
  2019-11-15 23:55 ` [PATCH v2 3/6] staging/intel-ipu3: " Niklas Söderlund
@ 2019-11-15 23:55 ` Niklas Söderlund
  2019-11-16  9:49   ` Hans Verkuil
  2019-11-15 23:55 ` [PATCH v2 5/6] rcar-vin: Make use of V4L2_CAP_IO_MC Niklas Söderlund
  2019-11-15 23:55 ` [PATCH v2 6/6] staging/intel-ipu3: " Niklas Söderlund
  5 siblings, 1 reply; 9+ messages in thread
From: Niklas Söderlund @ 2019-11-15 23:55 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.

Signed-off-by: Helen Koike <helen.koike@collabora.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 .../media/uapi/v4l/vidioc-querycap.rst        |  3 ++
 .../media/videodev2.h.rst.exceptions          |  1 +
 drivers/media/v4l2-core/v4l2-dev.c            | 10 ++++--
 drivers/media/v4l2-core/v4l2-ioctl.c          | 36 +++++++++++++++++++
 include/uapi/linux/videodev2.h                |  2 ++
 5 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
index 5f9930195d624c73..8b621ecb906afe96 100644
--- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
+++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
@@ -264,6 +264,9 @@ specification the ioctl returns an ``EINVAL`` error code.
     * - ``V4L2_CAP_TOUCH``
       - 0x10000000
       - This is a touch device.
+    * - ``V4L2_CAP_IO_MC``
+      - 0x20000000
+      - The inputs and/or outputs of this device are 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 4293df8d664f70b3..f0f5fcaa618c7be8 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -725,7 +725,6 @@ 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_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);
@@ -733,14 +732,21 @@ static void determine_valid_ioctls(struct video_device *vdev)
 			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 (vdev->device_caps & V4L2_CAP_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_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 (vdev->device_caps & V4L2_CAP_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 4a461de28677c5a8..a1c048c8fe6eff2f 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1085,6 +1085,36 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
 	return ret;
 }
 
+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 > 0)
+		return -EINVAL;
+
+	memset(i, 0, sizeof(*i));
+	strlcpy(i->name, vfd->name, sizeof(i->name));
+	i->type = V4L2_INPUT_TYPE_CAMERA;
+
+	return 0;
+}
+
+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 > 0)
+		return -EINVAL;
+
+	memset(o, 0, sizeof(*o));
+	strlcpy(o->name, vfd->name, sizeof(o->name));
+	o->type = V4L2_OUTPUT_TYPE_ANALOG;
+
+	return 0;
+}
+
 static int v4l2_ioctl_g_single_input(struct file *file, void *priv, unsigned int *i)
 {
 	*i = 0;
@@ -1181,6 +1211,9 @@ 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);
 }
 
@@ -1199,6 +1232,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);
 }
 
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 04481c717fee75c4..c97b55bc19eb892a 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.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH v2 5/6] rcar-vin: Make use of V4L2_CAP_IO_MC
  2019-11-15 23:55 [PATCH v2 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
                   ` (3 preceding siblings ...)
  2019-11-15 23:55 ` [PATCH v2 4/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
@ 2019-11-15 23:55 ` Niklas Söderlund
  2019-11-15 23:55 ` [PATCH v2 6/6] staging/intel-ipu3: " Niklas Söderlund
  5 siblings, 0 replies; 9+ messages in thread
From: Niklas Söderlund @ 2019-11-15 23:55 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.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH v2 6/6] staging/intel-ipu3: Make use of V4L2_CAP_IO_MC
  2019-11-15 23:55 [PATCH v2 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
                   ` (4 preceding siblings ...)
  2019-11-15 23:55 ` [PATCH v2 5/6] rcar-vin: Make use of V4L2_CAP_IO_MC Niklas Söderlund
@ 2019-11-15 23:55 ` Niklas Söderlund
  5 siblings, 0 replies; 9+ messages in thread
From: Niklas Söderlund @ 2019-11-15 23:55 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 5f8ad5996aa37db4..bae0f7212e640ee2 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -845,28 +845,6 @@ static int imgu_vidioc_g_meta_fmt(struct file *file, void *fh,
 	return 0;
 }
 
-static int imgu_vidioc_enum_input(struct file *file, void *fh,
-				  struct v4l2_input *input)
-{
-	if (input->index > 0)
-		return -EINVAL;
-	strscpy(input->name, "camera", sizeof(input->name));
-	input->type = V4L2_INPUT_TYPE_CAMERA;
-
-	return 0;
-}
-
-static int imgu_vidioc_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 = {
@@ -939,10 +917,6 @@ static const struct v4l2_ioctl_ops imgu_v4l2_ioctl_ops = {
 	.vidioc_s_fmt_vid_out_mplane = imgu_vidioc_s_fmt,
 	.vidioc_try_fmt_vid_out_mplane = imgu_vidioc_try_fmt,
 
-	.vidioc_enum_output = imgu_vidioc_enum_output,
-
-	.vidioc_enum_input = imgu_vidioc_enum_input,
-
 	/* buffer queue management */
 	.vidioc_reqbufs = vb2_ioctl_reqbufs,
 	.vidioc_create_bufs = vb2_ioctl_create_bufs,
@@ -1032,7 +1006,7 @@ static void imgu_node_to_v4l2(u32 node, struct video_device *vdev,
 
 	switch (node) {
 	case IMGU_NODE_IN:
-		cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE;
+		cap = V4L2_CAP_VIDEO_OUTPUT_MPLANE | V4L2_CAP_IO_MC;
 		f->type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
 		vdev->ioctl_ops = &imgu_v4l2_ioctl_ops;
 		break;
@@ -1051,7 +1025,7 @@ static void imgu_node_to_v4l2(u32 node, struct video_device *vdev,
 		imgu_css_meta_fmt_set(&f->fmt.meta);
 		break;
 	default:
-		cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE;
+		cap = V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_IO_MC;
 		f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
 		vdev->ioctl_ops = &imgu_v4l2_ioctl_ops;
 	}
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/6] v4l2-dev/ioctl: Add default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT}
  2019-11-15 23:55 ` [PATCH v2 1/6] v4l2-dev/ioctl: Add default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT} Niklas Söderlund
@ 2019-11-16  9:19   ` Hans Verkuil
  0 siblings, 0 replies; 9+ messages in thread
From: Hans Verkuil @ 2019-11-16  9:19 UTC (permalink / raw)
  To: Niklas Söderlund, Helen Koike, Sakari Ailus,
	Laurent Pinchart, linux-media
  Cc: linux-renesas-soc

On 11/16/19 12:55 AM, Niklas Söderlund wrote:
> It is very common to have just one input/output, and many drivers
> implements the same trivial handlers. Add default handlers for
> VIDIOC_{G,S}_{INPUT,OUTPUT} that assumes a single input/output.
> 
> Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c   |  8 ++---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 44 +++++++++++++++++++++++++---
>  2 files changed, 44 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index da42d172714adcb5..4293df8d664f70b3 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -726,8 +726,8 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  		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);

This isn't right. VIDIOC_G/S_INPUT now depends on the presence of vidioc_enum_input
only. So this becomes:

			SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_enum_input);
			SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_enum_input);

Same elsewhere. Note that there are two places where G/S_INPUT is set: it's also
valid for touch devices.

>  			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);
> @@ -736,8 +736,8 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  		}
>  		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);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 4e700583659bac8c..4a461de28677c5a8 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1085,6 +1085,37 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
>  	return ret;
>  }
>  
> +static int v4l2_ioctl_g_single_input(struct file *file, void *priv, unsigned int *i)
> +{
> +	*i = 0;
> +	return 0;
> +}
> +#define v4l2_ioctl_g_single_output v4l2_ioctl_g_single_input
> +
> +static int v4l2_ioctl_s_single_input(struct file *file, void *priv, unsigned int i)
> +{
> +	return i ? -EINVAL : 0;
> +}
> +#define v4l2_ioctl_s_single_output v4l2_ioctl_s_single_input

There is no point to add these static functions and defines, just incorporate this
code directly in the following functions. These are just one or two liners, after all.
And then there is no need for the defines either (they are a bit ugly).

> +
> +static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
> +		       struct file *file, void *fh, void *arg)
> +{
> +	if (!ops->vidioc_g_input)
> +		return v4l2_ioctl_g_single_input(file, fh, arg);
> +
> +	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)
> +		return v4l2_ioctl_g_single_output(file, fh, arg);
> +
> +	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 +1125,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 v4l2_ioctl_s_single_input(file, fh, *(unsigned int *)arg);
> +
>  	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 v4l2_ioctl_s_single_output(file, fh, *(unsigned int *)arg);
> +
>  	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
>  }
>  
> @@ -2678,10 +2716,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 +2766,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),
> 

Regards,

	Hans

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH v2 4/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC
  2019-11-15 23:55 ` [PATCH v2 4/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
@ 2019-11-16  9:49   ` Hans Verkuil
  0 siblings, 0 replies; 9+ messages in thread
From: Hans Verkuil @ 2019-11-16  9:49 UTC (permalink / raw)
  To: Niklas Söderlund, Helen Koike, Sakari Ailus,
	Laurent Pinchart, linux-media
  Cc: linux-renesas-soc

On 11/16/19 12:55 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.
> 
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  .../media/uapi/v4l/vidioc-querycap.rst        |  3 ++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  drivers/media/v4l2-core/v4l2-dev.c            | 10 ++++--
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 36 +++++++++++++++++++
>  include/uapi/linux/videodev2.h                |  2 ++
>  5 files changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-querycap.rst b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> index 5f9930195d624c73..8b621ecb906afe96 100644
> --- a/Documentation/media/uapi/v4l/vidioc-querycap.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-querycap.rst
> @@ -264,6 +264,9 @@ specification the ioctl returns an ``EINVAL`` error code.
>      * - ``V4L2_CAP_TOUCH``
>        - 0x10000000
>        - This is a touch device.
> +    * - ``V4L2_CAP_IO_MC``
> +      - 0x20000000
> +      - The inputs and/or outputs of this device are controlled by the Media Controller see :ref:`media_controller`.

'see' should start a new sentence. So: ... Controller. See ...

I also think this needs a more detailed description. At minimum this should explain that there is only one
input and/or output as seen from userspace and it depends on the topology from/to which actual video source the
video device captures/outputs its video.

And be very precise when you are talking about inputs/outputs: are those the inputs/outputs as seen from
userspace, or the inputs/outputs to the video I/O entity?

>      * - ``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 4293df8d664f70b3..f0f5fcaa618c7be8 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -725,7 +725,6 @@ 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_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);
> @@ -733,14 +732,21 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  			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 (vdev->device_caps & V4L2_CAP_IO_MC)

Add an is_io_mc bool at the top and use that.

> +				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
> +			else
> +				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);

Ah, now I see why you did things the way you did in patch 1.

This is confusing in this series: for once I think it is better if patches 1 and 4 are
combined in a single patch.

In any case, this means that the G/S_IN/OUTPUT ioctls should only be valid if:

- CAP_IO_MC is set
- or the vidioc_g/s_in/output op is set by the driver
- or the vidioc_enum_in/output op is set by the driver

I've been wondering: now that we know what the device caps are we can do a much
better job of validating if the driver supports the right ioctls.

We've been relying on v4l2-compliance for that, but part of the v4l2-compliance
validation can be done here instead, issuing a WARN_ON if the driver forgets to
implement something that it should have implemented.

Something for a future patch, though.

>  		}
>  		if (is_tx) {
> -			SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_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 (vdev->device_caps & V4L2_CAP_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 4a461de28677c5a8..a1c048c8fe6eff2f 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1085,6 +1085,36 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
>  	return ret;
>  }
>  
> +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 > 0)

Just say: if (i->index)

> +		return -EINVAL;
> +
> +	memset(i, 0, sizeof(*i));

Not needed, everything after index is already zeroed.

> +	strlcpy(i->name, vfd->name, sizeof(i->name));

strscpy

> +	i->type = V4L2_INPUT_TYPE_CAMERA;
> +
> +	return 0;
> +}
> +
> +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 > 0)
> +		return -EINVAL;
> +
> +	memset(o, 0, sizeof(*o));
> +	strlcpy(o->name, vfd->name, sizeof(o->name));

Same comments as for v4l2_ioctl_enum_input_mc.

> +	o->type = V4L2_OUTPUT_TYPE_ANALOG;
> +
> +	return 0;
> +}
> +
>  static int v4l2_ioctl_g_single_input(struct file *file, void *priv, unsigned int *i)
>  {
>  	*i = 0;
> @@ -1181,6 +1211,9 @@ 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 think you can incorporate the code of v4l2_ioctl_enum_input_mc
here instead of creating a separate function. It's a matter of taste, though.

> +
>  	return ops->vidioc_enum_input(file, fh, p);
>  }
>  
> @@ -1199,6 +1232,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);

If you DO keep separate functions, then it is better to move v4l2_ioctl_enum_output_mc
to just before this function, so the two are close together.

> +
>  	return ops->vidioc_enum_output(file, fh, p);
>  }
>  
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 04481c717fee75c4..c97b55bc19eb892a 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -487,6 +487,8 @@ struct v4l2_capability {
>  
>  #define V4L2_CAP_TOUCH                  0x10000000  /* Is a touch device */
>  
> +#define V4L2_CAP_IO_MC			0x20000000  /* Is input/output controlled by the media controller */
> +
>  #define V4L2_CAP_DEVICE_CAPS            0x80000000  /* sets device capabilities field */
>  
>  /*
> 

Regards,

	Hans

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-11-16  9:49 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-15 23:55 [PATCH v2 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
2019-11-15 23:55 ` [PATCH v2 1/6] v4l2-dev/ioctl: Add default handlers for VIDIOC_{G,S}_{INPUT,OUTPUT} Niklas Söderlund
2019-11-16  9:19   ` Hans Verkuil
2019-11-15 23:55 ` [PATCH v2 2/6] rcar-vin: Use default VIDIOC_{G,S}_{INPUT,OUTPUT} handler Niklas Söderlund
2019-11-15 23:55 ` [PATCH v2 3/6] staging/intel-ipu3: " Niklas Söderlund
2019-11-15 23:55 ` [PATCH v2 4/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC Niklas Söderlund
2019-11-16  9:49   ` Hans Verkuil
2019-11-15 23:55 ` [PATCH v2 5/6] rcar-vin: Make use of V4L2_CAP_IO_MC Niklas Söderlund
2019-11-15 23:55 ` [PATCH v2 6/6] staging/intel-ipu3: " Niklas Söderlund

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.