[PATCH v8 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[PATCH v8 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Niklas Söderlund]

Hi,

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

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

Patch 2/6 fix initialization of reserved fields in the cx18 and ivtv 
drivers which becomes a problem in 3/6 where Laurent adds mbus filters to
VIDIOC_ENUM_FMT.

Patch 4/6, 5/6 and 6/6 converts the R-Car VIN, Intel IPU3 and VIMC
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.

A separate patch to v4l-utils have been posted as [1] to add a
test for this feature in v4l2-compliance.

This version have been rebased to latest media-tree to account for the 
large shuffles of files. It has also replaced patch 2/6 with a different 
version that addresses both cx18 and ivtv instead of only ivtv.

1. [PATCH 0/2] v4l2-compliance: add tests for V4L2_CAP_IO_MC

Laurent Pinchart (2):
  media: pci: Fill v4l2_fmtdesc with designated initializers
  media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

Niklas Söderlund (4):
  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
  vimc: Make use of V4L2_CAP_IO_MC

 .../media/v4l/vidioc-enum-fmt.rst             | 16 ++++-
 .../media/v4l/vidioc-querycap.rst             |  6 ++
 .../media/videodev2.h.rst.exceptions          |  1 +
 drivers/media/pci/cx18/cx18-ioctl.c           | 22 ++++--
 drivers/media/pci/ivtv/ivtv-ioctl.c           | 26 +++----
 drivers/media/platform/rcar-vin/rcar-v4l2.c   | 40 ++++++-----
 .../media/test-drivers/vimc/vimc-capture.c    | 14 +++-
 drivers/media/v4l2-core/v4l2-dev.c            | 25 +++++--
 drivers/media/v4l2-core/v4l2-ioctl.c          | 70 +++++++++++++++++--
 drivers/staging/media/ipu3/ipu3-v4l2.c        | 64 ++---------------
 include/uapi/linux/videodev2.h                |  5 +-
 11 files changed, 180 insertions(+), 109 deletions(-)

-- 
2.26.0

[PATCH v8 1/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

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

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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
--
* Changes since v7
- Dropped !is_meta check in determine_valid_ioctls(), dropped tag from
  Sakari as it was added when adding the !is_meta check
---
 .../media/v4l/vidioc-querycap.rst             |  6 ++
 .../media/videodev2.h.rst.exceptions          |  1 +
 drivers/media/v4l2-core/v4l2-dev.c            | 25 ++++++--
 drivers/media/v4l2-core/v4l2-ioctl.c          | 57 +++++++++++++++++--
 include/uapi/linux/videodev2.h                |  2 +
 5 files changed, 81 insertions(+), 10 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/vidioc-querycap.rst b/Documentation/userspace-api/media/v4l/vidioc-querycap.rst
index 28e1f766128c41f3..666ac4d420519999 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-querycap.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-querycap.rst
@@ -264,6 +264,12 @@ specification the ioctl returns an ``EINVAL`` error code.
     * - ``V4L2_CAP_TOUCH``
       - 0x10000000
       - This is a touch device.
+    * - ``V4L2_CAP_IO_MC``
+      - 0x20000000
+      - There is only one input and/or output seen from userspace. The whole
+        video topology configuration, including which I/O entity is routed to
+        the input/output, is configured by userspace via the Media Controller.
+        See :ref:`media_controller`.
     * - ``V4L2_CAP_DEVICE_CAPS``
       - 0x80000000
       - The driver fills the ``device_caps`` field. This capability can
diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
index cb6ccf91776e6b56..a625fb90e3a989a7 100644
--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
@@ -176,6 +176,7 @@ replace define V4L2_CAP_STREAMING device-capabilities
 replace define V4L2_CAP_META_OUTPUT device-capabilities
 replace define V4L2_CAP_DEVICE_CAPS device-capabilities
 replace define V4L2_CAP_TOUCH device-capabilities
+replace define V4L2_CAP_IO_MC device-capabilities
 
 # V4L2 pix flags
 replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 97b6a3af13614639..a593ea0598b551b4 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -552,6 +552,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		       (vdev->device_caps & meta_caps);
 	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
 	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
+	bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
 
 	bitmap_zero(valid_ioctls, BASE_VIDIOC_PRIVATE);
 
@@ -725,9 +726,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		SET_VALID_IOCTL(ops, VIDIOC_G_STD, vidioc_g_std);
 		if (is_rx) {
 			SET_VALID_IOCTL(ops, VIDIOC_QUERYSTD, vidioc_querystd);
-			SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
-			SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
-			SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
+			if (is_io_mc) {
+				set_bit(_IOC_NR(VIDIOC_ENUMINPUT), valid_ioctls);
+				set_bit(_IOC_NR(VIDIOC_G_INPUT), valid_ioctls);
+				set_bit(_IOC_NR(VIDIOC_S_INPUT), valid_ioctls);
+			} else {
+				SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
+				SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
+				SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
+			}
 			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDIO, vidioc_enumaudio);
 			SET_VALID_IOCTL(ops, VIDIOC_G_AUDIO, vidioc_g_audio);
 			SET_VALID_IOCTL(ops, VIDIOC_S_AUDIO, vidioc_s_audio);
@@ -735,9 +742,15 @@ static void determine_valid_ioctls(struct video_device *vdev)
 			SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
 		}
 		if (is_tx) {
-			SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
-			SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
-			SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
+			if (is_io_mc) {
+				set_bit(_IOC_NR(VIDIOC_ENUMOUTPUT), valid_ioctls);
+				set_bit(_IOC_NR(VIDIOC_G_OUTPUT), valid_ioctls);
+				set_bit(_IOC_NR(VIDIOC_S_OUTPUT), valid_ioctls);
+			} else {
+				SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
+				SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
+				SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
+			}
 			SET_VALID_IOCTL(ops, VIDIOC_ENUMAUDOUT, vidioc_enumaudout);
 			SET_VALID_IOCTL(ops, VIDIOC_G_AUDOUT, vidioc_g_audout);
 			SET_VALID_IOCTL(ops, VIDIOC_S_AUDOUT, vidioc_s_audout);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index b2ef8e60ea7da19d..afd1f427df557f71 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1085,6 +1085,32 @@ static int v4l_querycap(const struct v4l2_ioctl_ops *ops,
 	return ret;
 }
 
+static int v4l_g_input(const struct v4l2_ioctl_ops *ops,
+		       struct file *file, void *fh, void *arg)
+{
+	struct video_device *vfd = video_devdata(file);
+
+	if (vfd->device_caps & V4L2_CAP_IO_MC) {
+		*(int *)arg = 0;
+		return 0;
+	}
+
+	return ops->vidioc_g_input(file, fh, arg);
+}
+
+static int v4l_g_output(const struct v4l2_ioctl_ops *ops,
+		       struct file *file, void *fh, void *arg)
+{
+	struct video_device *vfd = video_devdata(file);
+
+	if (vfd->device_caps & V4L2_CAP_IO_MC) {
+		*(int *)arg = 0;
+		return 0;
+	}
+
+	return ops->vidioc_g_output(file, fh, arg);
+}
+
 static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
@@ -1094,12 +1120,21 @@ static int v4l_s_input(const struct v4l2_ioctl_ops *ops,
 	ret = v4l_enable_media_source(vfd);
 	if (ret)
 		return ret;
+
+	if (vfd->device_caps & V4L2_CAP_IO_MC)
+		return  *(int *)arg ? -EINVAL : 0;
+
 	return ops->vidioc_s_input(file, fh, *(unsigned int *)arg);
 }
 
 static int v4l_s_output(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
+	struct video_device *vfd = video_devdata(file);
+
+	if (vfd->device_caps & V4L2_CAP_IO_MC)
+		return  *(int *)arg ? -EINVAL : 0;
+
 	return ops->vidioc_s_output(file, fh, *(unsigned int *)arg);
 }
 
@@ -1143,6 +1178,14 @@ static int v4l_enuminput(const struct v4l2_ioctl_ops *ops,
 	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
 		p->capabilities |= V4L2_IN_CAP_STD;
 
+	if (vfd->device_caps & V4L2_CAP_IO_MC) {
+		if (p->index)
+			return -EINVAL;
+		strscpy(p->name, vfd->name, sizeof(p->name));
+		p->type = V4L2_INPUT_TYPE_CAMERA;
+		return 0;
+	}
+
 	return ops->vidioc_enum_input(file, fh, p);
 }
 
@@ -1161,6 +1204,14 @@ static int v4l_enumoutput(const struct v4l2_ioctl_ops *ops,
 	if (is_valid_ioctl(vfd, VIDIOC_S_STD))
 		p->capabilities |= V4L2_OUT_CAP_STD;
 
+	if (vfd->device_caps & V4L2_CAP_IO_MC) {
+		if (p->index)
+			return -EINVAL;
+		strscpy(p->name, vfd->name, sizeof(p->name));
+		p->type = V4L2_OUTPUT_TYPE_ANALOG;
+		return 0;
+	}
+
 	return ops->vidioc_enum_output(file, fh, p);
 }
 
@@ -2683,10 +2734,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)
@@ -2735,11 +2784,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 9817b7e2c968fd04..b18f3f7cde31c2e4 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.26.0

[PATCH v8 2/6] media: pci: Fill v4l2_fmtdesc with designated initializers

[Niklas Söderlund]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Replace initialization of static const v4l2_fmtdesc instances that
specify every struct member with designated initializers. This allows
not zeroing the reserved fields explicitly, and will avoid a need to
patch these drivers every time a reserved field is repurposed.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/pci/cx18/cx18-ioctl.c | 22 ++++++++++++++++------
 drivers/media/pci/ivtv/ivtv-ioctl.c | 26 ++++++++++++++------------
 2 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/drivers/media/pci/cx18/cx18-ioctl.c b/drivers/media/pci/cx18/cx18-ioctl.c
index fa57e12f2ac8f0d9..4864def20676ecef 100644
--- a/drivers/media/pci/cx18/cx18-ioctl.c
+++ b/drivers/media/pci/cx18/cx18-ioctl.c
@@ -466,14 +466,24 @@ static int cx18_enum_fmt_vid_cap(struct file *file, void *fh,
 					struct v4l2_fmtdesc *fmt)
 {
 	static const struct v4l2_fmtdesc formats[] = {
-		{ 0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
-		  "HM12 (YUV 4:1:1)", V4L2_PIX_FMT_HM12, { 0, 0, 0, 0 }
+		{
+			.index = 0,
+			.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+			.description = "HM12 (YUV 4:1:1)",
+			.pixelformat = V4L2_PIX_FMT_HM12,
 		},
-		{ 1, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
-		  "MPEG", V4L2_PIX_FMT_MPEG, { 0, 0, 0, 0 }
+		{
+			.index = 1,
+			.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+			.flags = V4L2_FMT_FLAG_COMPRESSED,
+			.description = "MPEG",
+			.pixelformat = V4L2_PIX_FMT_MPEG,
 		},
-		{ 2, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
-		  "UYVY 4:2:2", V4L2_PIX_FMT_UYVY, { 0, 0, 0, 0 }
+		{
+			.index = 2,
+			.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+			.description = "UYVY 4:2:2",
+			.pixelformat = V4L2_PIX_FMT_UYVY,
 		},
 	};
 
diff --git a/drivers/media/pci/ivtv/ivtv-ioctl.c b/drivers/media/pci/ivtv/ivtv-ioctl.c
index 137853944e4619cb..35dccb31174c1e82 100644
--- a/drivers/media/pci/ivtv/ivtv-ioctl.c
+++ b/drivers/media/pci/ivtv/ivtv-ioctl.c
@@ -920,14 +920,15 @@ static int ivtv_g_selection(struct file *file, void *fh,
 static int ivtv_enum_fmt_vid_cap(struct file *file, void *fh, struct v4l2_fmtdesc *fmt)
 {
 	static const struct v4l2_fmtdesc hm12 = {
-		0, V4L2_BUF_TYPE_VIDEO_CAPTURE, 0,
-		"HM12 (YUV 4:2:0)", V4L2_PIX_FMT_HM12,
-		{ 0, 0, 0, 0 }
+		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+		.description = "HM12 (YUV 4:2:0)",
+		.pixelformat = V4L2_PIX_FMT_HM12,
 	};
 	static const struct v4l2_fmtdesc mpeg = {
-		0, V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FMT_FLAG_COMPRESSED,
-		"MPEG", V4L2_PIX_FMT_MPEG,
-		{ 0, 0, 0, 0 }
+		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
+		.flags = V4L2_FMT_FLAG_COMPRESSED,
+		.description = "MPEG",
+		.pixelformat = V4L2_PIX_FMT_MPEG,
 	};
 	struct ivtv *itv = fh2id(fh)->itv;
 	struct ivtv_stream *s = &itv->streams[fh2id(fh)->type];
@@ -946,14 +947,15 @@ static int ivtv_enum_fmt_vid_cap(struct file *file, void *fh, struct v4l2_fmtdes
 static int ivtv_enum_fmt_vid_out(struct file *file, void *fh, struct v4l2_fmtdesc *fmt)
 {
 	static const struct v4l2_fmtdesc hm12 = {
-		0, V4L2_BUF_TYPE_VIDEO_OUTPUT, 0,
-		"HM12 (YUV 4:2:0)", V4L2_PIX_FMT_HM12,
-		{ 0, 0, 0, 0 }
+		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT,
+		.description = "HM12 (YUV 4:2:0)",
+		.pixelformat = V4L2_PIX_FMT_HM12,
 	};
 	static const struct v4l2_fmtdesc mpeg = {
-		0, V4L2_BUF_TYPE_VIDEO_OUTPUT, V4L2_FMT_FLAG_COMPRESSED,
-		"MPEG", V4L2_PIX_FMT_MPEG,
-		{ 0, 0, 0, 0 }
+		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT,
+		.flags = V4L2_FMT_FLAG_COMPRESSED,
+		.description = "MPEG",
+		.pixelformat = V4L2_PIX_FMT_MPEG,
 	};
 	struct ivtv *itv = fh2id(fh)->itv;
 	struct ivtv_stream *s = &itv->streams[fh2id(fh)->type];
-- 
2.26.0

[PATCH v8 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Niklas Söderlund]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

The VIDIOC_ENUM_FMT ioctl enumerates all formats supported by a video
node. For MC-centric devices, its behaviour has always been ill-defined,
with drivers implementing one of the following behaviours:

- No support for VIDIOC_ENUM_FMT at all
- Enumerating all formats supported by the video node, regardless of the
  configuration of the pipeline
- Enumerating formats supported by the video node for the active
  configuration of the connected subdevice

The first behaviour is obviously useless for applications. The second
behaviour provides the most information, but doesn't offer a way to find
what formats are compatible with a given pipeline configuration. The
third behaviour fixes that, but with the drawback that applications
can't enumerate all supported formats anymore, and have to modify the
active configuration of the pipeline to enumerate formats.

The situation is messy as none of the implemented behaviours are ideal,
and userspace can't predict what will happen as the behaviour is
driver-specific.

To fix this, let's extend the VIDIOC_ENUM_FMT with a missing capability:
enumerating pixel formats for a given media bus code. The media bus code
is passed through the v4l2_fmtdesc structure in a new mbus_code field
(repurposed from the reserved fields). With this capability in place,
applications can enumerate pixel formats for a given media bus code
without modifying the active configuration of the device.

The current behaviour of the ioctl is preserved when the new mbus_code
field is set to 0, ensuring compatibility with existing userspace. The
API extension is documented as mandatory for MC-centric devices (as
advertised through the V4L2_CAP_IO_MC capability), allowing applications
and compliance tools to easily determine the availability of the
VIDIOC_ENUM_FMT extension.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v7
- Update documentation.
---
 .../userspace-api/media/v4l/vidioc-enum-fmt.rst  | 16 +++++++++++++---
 drivers/media/v4l2-core/v4l2-ioctl.c             | 13 +++++++++++--
 include/uapi/linux/videodev2.h                   |  3 ++-
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
index 7e3142e11d77d9c0..8dc8a73c320dda98 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
@@ -48,10 +48,20 @@ one until ``EINVAL`` is returned. If applicable, drivers shall return
 formats in preference order, where preferred formats are returned before
 (that is, with lower ``index`` value) less-preferred formats.
 
-.. note::
+If the driver doesn't advertise the ``V4L2_CAP_IO_MC``
+:ref:`device-capabilities <capability>`, applications shall initialize the
+``mbus_code`` field to zero. Drivers shall enumerate all image formats. The
+enumerated formats may depend on the active input or output of the device.
 
-   After switching input or output the list of enumerated image
-   formats may be different.
+If the driver advertises the ``V4L2_CAP_IO_MC`` :ref:`device-capabilities
+<capability>`, applications may initialize the ``mbus_code`` to a valid
+:ref:`v4l2_mbus_pixelcode <media bus format code>`. If the ``mbus_code` field
+is not zero, drivers shall restrict enumeration to only the image formats that
+can produce (for video output devices) or be produced from (for video capture
+devices) that media bus code. Regardless of the value of the ``mbus_code``
+field, the enumerated image formats shall not depend on the active
+configuration of the video device or device pipeline. Enumeration shall
+otherwise operate as previously described.
 
 
 .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index afd1f427df557f71..3e7b99fa415222c6 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -264,12 +264,13 @@ static void v4l_print_fmtdesc(const void *arg, bool write_only)
 {
 	const struct v4l2_fmtdesc *p = arg;
 
-	pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c, description='%.*s'\n",
+	pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c, mbus_code=0x%04x, description='%.*s'\n",
 		p->index, prt_names(p->type, v4l2_type_names),
 		p->flags, (p->pixelformat & 0xff),
 		(p->pixelformat >>  8) & 0xff,
 		(p->pixelformat >> 16) & 0xff,
 		(p->pixelformat >> 24) & 0xff,
+		p->mbus_code,
 		(int)sizeof(p->description), p->description);
 }
 
@@ -1472,12 +1473,20 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
 	struct video_device *vdev = video_devdata(file);
 	struct v4l2_fmtdesc *p = arg;
 	int ret = check_fmt(file, p->type);
+	u32 mbus_code;
 	u32 cap_mask;
 
 	if (ret)
 		return ret;
 	ret = -EINVAL;
 
+	if (p->mbus_code && !(vdev->device_caps & V4L2_CAP_IO_MC))
+		return -EINVAL;
+
+	mbus_code = p->mbus_code;
+	CLEAR_AFTER_FIELD(p, type);
+	p->mbus_code = mbus_code;
+
 	switch (p->type) {
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
@@ -2757,7 +2766,7 @@ DEFINE_V4L_STUB_FUNC(dv_timings_cap)
 
 static const struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO(VIDIOC_QUERYCAP, v4l_querycap, v4l_print_querycap, 0),
-	IOCTL_INFO(VIDIOC_ENUM_FMT, v4l_enum_fmt, v4l_print_fmtdesc, INFO_FL_CLEAR(v4l2_fmtdesc, type)),
+	IOCTL_INFO(VIDIOC_ENUM_FMT, v4l_enum_fmt, v4l_print_fmtdesc, 0),
 	IOCTL_INFO(VIDIOC_G_FMT, v4l_g_fmt, v4l_print_format, 0),
 	IOCTL_INFO(VIDIOC_S_FMT, v4l_s_fmt, v4l_print_format, INFO_FL_PRIO),
 	IOCTL_INFO(VIDIOC_REQBUFS, v4l_reqbufs, v4l_print_requestbuffers, INFO_FL_PRIO | INFO_FL_QUEUE),
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index b18f3f7cde31c2e4..c3a1cf1c507f5506 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -784,7 +784,8 @@ struct v4l2_fmtdesc {
 	__u32               flags;
 	__u8		    description[32];   /* Description string */
 	__u32		    pixelformat;       /* Format fourcc      */
-	__u32		    reserved[4];
+	__u32		    mbus_code;		/* Media bus code    */
+	__u32		    reserved[3];
 };
 
 #define V4L2_FMT_FLAG_COMPRESSED		0x0001
-- 
2.26.0

[PATCH v8 4/6] rcar-vin: Make use of V4L2_CAP_IO_MC

[Niklas Söderlund]

Set the V4L2_CAP_IO_MC capability flag and remove the driver specific
vidioc_enum_input, vidioc_g_input and vidioc_s_input callbacks for the
media controller enabled part of the driver. Also add support mbus_code
filtering for format enumeration.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
* Changes since v6
- Implement mbus_code filtering for format enumeration
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 40 ++++++++++++---------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 5151a3cd8a6e6754..f421e25848756c97 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -343,6 +343,29 @@ static int rvin_enum_fmt_vid_cap(struct file *file, void *priv,
 	unsigned int i;
 	int matched;
 
+	/*
+	 * If mbus_code is set only enumerate supported pixel formats for that
+	 * bus code. Converting from YCbCr to RGB and RGB to YCbCr is possible
+	 * with VIN, so all supported YCbCr and RGB media bus codes can produce
+	 * all of the related pixel formats. If mbus_code is not set enumerate
+	 * all possible pixelformats.
+	 *
+	 * TODO: Once raw capture formats are added to the driver this needs
+	 * to be extended so raw media bus codes only result in raw pixel
+	 * formats.
+	 */
+	switch (f->mbus_code) {
+	case 0:
+	case MEDIA_BUS_FMT_YUYV8_1X16:
+	case MEDIA_BUS_FMT_UYVY8_1X16:
+	case MEDIA_BUS_FMT_UYVY8_2X8:
+	case MEDIA_BUS_FMT_UYVY10_2X10:
+	case MEDIA_BUS_FMT_RGB888_1X24:
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	matched = -1;
 	for (i = 0; i < ARRAY_SIZE(rvin_formats); i++) {
 		if (rvin_format_from_pixel(vin, rvin_formats[i].fourcc))
@@ -767,18 +790,6 @@ static int rvin_mc_s_fmt_vid_cap(struct file *file, void *priv,
 	return 0;
 }
 
-static int rvin_mc_enum_input(struct file *file, void *priv,
-			      struct v4l2_input *i)
-{
-	if (i->index != 0)
-		return -EINVAL;
-
-	i->type = V4L2_INPUT_TYPE_CAMERA;
-	strscpy(i->name, "Camera", sizeof(i->name));
-
-	return 0;
-}
-
 static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
 	.vidioc_querycap		= rvin_querycap,
 	.vidioc_try_fmt_vid_cap		= rvin_mc_try_fmt_vid_cap,
@@ -786,10 +797,6 @@ static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
 	.vidioc_s_fmt_vid_cap		= rvin_mc_s_fmt_vid_cap,
 	.vidioc_enum_fmt_vid_cap	= rvin_enum_fmt_vid_cap,
 
-	.vidioc_enum_input		= rvin_mc_enum_input,
-	.vidioc_g_input			= rvin_g_input,
-	.vidioc_s_input			= rvin_s_input,
-
 	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
 	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
 	.vidioc_querybuf		= vb2_ioctl_querybuf,
@@ -961,6 +968,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.26.0

[PATCH v8 5/6] staging/intel-ipu3: Make use of V4L2_CAP_IO_MC

[Niklas Söderlund]

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

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v5:

- Implement mbus_code filtering for format enumeration
---
 drivers/staging/media/ipu3/ipu3-v4l2.c | 64 +++-----------------------
 1 file changed, 7 insertions(+), 57 deletions(-)

diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c b/drivers/staging/media/ipu3/ipu3-v4l2.c
index afbb4636e714d5b5..4dc8d9165f634325 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -605,6 +605,9 @@ static int enum_fmts(struct v4l2_fmtdesc *f, u32 type)
 {
 	unsigned int i, j;
 
+	if (f->mbus_code != 0 && f->mbus_code != MEDIA_BUS_FMT_FIXED)
+		return -EINVAL;
+
 	for (i = j = 0; i < ARRAY_SIZE(formats); ++i) {
 		if (formats[i].type == type) {
 			if (j == f->index)
@@ -834,6 +837,9 @@ static int imgu_meta_enum_format(struct file *file, void *fh,
 	if (fmt->index > 0 || fmt->type != node->vbq.type)
 		return -EINVAL;
 
+	if (fmt->mbus_code != 0 && fmt->mbus_code != MEDIA_BUS_FMT_FIXED)
+		return -EINVAL;
+
 	strscpy(fmt->description, meta_fmts[i].name, sizeof(fmt->description));
 	fmt->pixelformat = meta_fmts[i].fourcc;
 
@@ -853,54 +859,6 @@ static int imgu_vidioc_g_meta_fmt(struct file *file, void *fh,
 	return 0;
 }
 
-static int imgu_vidioc_enum_input(struct file *file, void *fh,
-				  struct v4l2_input *input)
-{
-	if (input->index > 0)
-		return -EINVAL;
-	strscpy(input->name, "camera", sizeof(input->name));
-	input->type = V4L2_INPUT_TYPE_CAMERA;
-
-	return 0;
-}
-
-static int imgu_vidioc_g_input(struct file *file, void *fh, unsigned int *input)
-{
-	*input = 0;
-
-	return 0;
-}
-
-static int imgu_vidioc_s_input(struct file *file, void *fh, unsigned int input)
-{
-	return input == 0 ? 0 : -EINVAL;
-}
-
-static int imgu_vidioc_enum_output(struct file *file, void *fh,
-				   struct v4l2_output *output)
-{
-	if (output->index > 0)
-		return -EINVAL;
-	strscpy(output->name, "camera", sizeof(output->name));
-	output->type = V4L2_INPUT_TYPE_CAMERA;
-
-	return 0;
-}
-
-static int imgu_vidioc_g_output(struct file *file, void *fh,
-				unsigned int *output)
-{
-	*output = 0;
-
-	return 0;
-}
-
-static int imgu_vidioc_s_output(struct file *file, void *fh,
-				unsigned int output)
-{
-	return output == 0 ? 0 : -EINVAL;
-}
-
 /******************** function pointers ********************/
 
 static struct v4l2_subdev_internal_ops imgu_subdev_internal_ops = {
@@ -973,14 +931,6 @@ static const struct v4l2_ioctl_ops imgu_v4l2_ioctl_ops = {
 	.vidioc_s_fmt_vid_out_mplane = imgu_vidioc_s_fmt,
 	.vidioc_try_fmt_vid_out_mplane = imgu_vidioc_try_fmt,
 
-	.vidioc_enum_output = imgu_vidioc_enum_output,
-	.vidioc_g_output = imgu_vidioc_g_output,
-	.vidioc_s_output = imgu_vidioc_s_output,
-
-	.vidioc_enum_input = imgu_vidioc_enum_input,
-	.vidioc_g_input = imgu_vidioc_g_input,
-	.vidioc_s_input = imgu_vidioc_s_input,
-
 	/* buffer queue management */
 	.vidioc_reqbufs = vb2_ioctl_reqbufs,
 	.vidioc_create_bufs = vb2_ioctl_create_bufs,
@@ -1094,7 +1044,7 @@ static void imgu_node_to_v4l2(u32 node, struct video_device *vdev,
 		vdev->ioctl_ops = &imgu_v4l2_ioctl_ops;
 	}
 
-	vdev->device_caps = V4L2_CAP_STREAMING | cap;
+	vdev->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_IO_MC | cap;
 }
 
 static int imgu_v4l2_subdev_register(struct imgu_device *imgu,
-- 
2.26.0

[PATCH v8 6/6] vimc: Make use of V4L2_CAP_IO_MC

[Niklas Söderlund]

Set the V4L2_CAP_IO_MC capability flag to report this vimc
inputs/outputs are controlled by the media graph.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
* Changes since v7:
- Add check that index is not greater then 0 in
  vimc_cap_enum_fmt_vid_cap().

* Changes since v5:
- Wrap line longer than 80 characters
- Implement mbus_code filtering for format enumeration
---
 drivers/media/test-drivers/vimc/vimc-capture.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
index 20c2f5e281bc44ed..c63496b17b9a5370 100644
--- a/drivers/media/test-drivers/vimc/vimc-capture.c
+++ b/drivers/media/test-drivers/vimc/vimc-capture.c
@@ -149,7 +149,16 @@ static int vimc_cap_s_fmt_vid_cap(struct file *file, void *priv,
 static int vimc_cap_enum_fmt_vid_cap(struct file *file, void *priv,
 				     struct v4l2_fmtdesc *f)
 {
-	const struct vimc_pix_map *vpix = vimc_pix_map_by_index(f->index);
+	const struct vimc_pix_map *vpix;
+
+	if (f->mbus_code) {
+		if (f->index > 0)
+			return -EINVAL;
+
+		vpix = vimc_pix_map_by_code(f->mbus_code);
+	} else {
+		vpix = vimc_pix_map_by_index(f->index);
+	}
 
 	if (!vpix)
 		return -EINVAL;
@@ -450,7 +459,8 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
 
 	/* Initialize the video_device struct */
 	vdev = &vcap->vdev;
-	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
+	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING
+			  | V4L2_CAP_IO_MC;
 	vdev->entity.ops = &vimc_cap_mops;
 	vdev->release = video_device_release_empty;
 	vdev->fops = &vimc_cap_fops;
-- 
2.26.0

Re: [PATCH v8 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Laurent Pinchart]

Hi Niklas,

(With a question for Hans below)

Thank you for the patches.

On Tue, Apr 21, 2020 at 03:57:37PM +0200, Niklas Söderlund wrote:
> Hi,
> 
> This series aims to reduce the amount of boiler plate code in video
> device drivers who's inputs and/or outputs are controlled by the Media
> Controller instead of the V4L2 API.
> 
> Patch 1/6 adds a new video device capability flag V4L2_CAP_IO_MC which
> if set provides helper implementations for the enum inputs and outputs
> ioctls freeing the video device driver from the need to implement them.
> 
> Patch 2/6 fix initialization of reserved fields in the cx18 and ivtv 
> drivers which becomes a problem in 3/6 where Laurent adds mbus filters to
> VIDIOC_ENUM_FMT.
> 
> Patch 4/6, 5/6 and 6/6 converts the R-Car VIN, Intel IPU3 and VIMC
> 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.
> 
> A separate patch to v4l-utils have been posted as [1] to add a
> test for this feature in v4l2-compliance.
> 
> This version have been rebased to latest media-tree to account for the 
> large shuffles of files. It has also replaced patch 2/6 with a different 
> version that addresses both cx18 and ivtv instead of only ivtv.

It looks like everything has been reviewed. Hans, do you see any
remaining blocker, or will you take it in your tree and send a pull
request ?

> 1. [PATCH 0/2] v4l2-compliance: add tests for V4L2_CAP_IO_MC
> 
> Laurent Pinchart (2):
>   media: pci: Fill v4l2_fmtdesc with designated initializers
>   media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices
> 
> Niklas Söderlund (4):
>   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
>   vimc: Make use of V4L2_CAP_IO_MC
> 
>  .../media/v4l/vidioc-enum-fmt.rst             | 16 ++++-
>  .../media/v4l/vidioc-querycap.rst             |  6 ++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  drivers/media/pci/cx18/cx18-ioctl.c           | 22 ++++--
>  drivers/media/pci/ivtv/ivtv-ioctl.c           | 26 +++----
>  drivers/media/platform/rcar-vin/rcar-v4l2.c   | 40 ++++++-----
>  .../media/test-drivers/vimc/vimc-capture.c    | 14 +++-
>  drivers/media/v4l2-core/v4l2-dev.c            | 25 +++++--
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 70 +++++++++++++++++--
>  drivers/staging/media/ipu3/ipu3-v4l2.c        | 64 ++---------------
>  include/uapi/linux/videodev2.h                |  5 +-
>  11 files changed, 180 insertions(+), 109 deletions(-)
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Sakari Ailus]

On Tue, Apr 21, 2020 at 03:57:40PM +0200, Niklas Söderlund wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> The VIDIOC_ENUM_FMT ioctl enumerates all formats supported by a video
> node. For MC-centric devices, its behaviour has always been ill-defined,
> with drivers implementing one of the following behaviours:
> 
> - No support for VIDIOC_ENUM_FMT at all
> - Enumerating all formats supported by the video node, regardless of the
>   configuration of the pipeline
> - Enumerating formats supported by the video node for the active
>   configuration of the connected subdevice
> 
> The first behaviour is obviously useless for applications. The second
> behaviour provides the most information, but doesn't offer a way to find
> what formats are compatible with a given pipeline configuration. The
> third behaviour fixes that, but with the drawback that applications
> can't enumerate all supported formats anymore, and have to modify the
> active configuration of the pipeline to enumerate formats.
> 
> The situation is messy as none of the implemented behaviours are ideal,
> and userspace can't predict what will happen as the behaviour is
> driver-specific.
> 
> To fix this, let's extend the VIDIOC_ENUM_FMT with a missing capability:
> enumerating pixel formats for a given media bus code. The media bus code
> is passed through the v4l2_fmtdesc structure in a new mbus_code field
> (repurposed from the reserved fields). With this capability in place,
> applications can enumerate pixel formats for a given media bus code
> without modifying the active configuration of the device.
> 
> The current behaviour of the ioctl is preserved when the new mbus_code
> field is set to 0, ensuring compatibility with existing userspace. The
> API extension is documented as mandatory for MC-centric devices (as
> advertised through the V4L2_CAP_IO_MC capability), allowing applications
> and compliance tools to easily determine the availability of the
> VIDIOC_ENUM_FMT extension.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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

-- 
Sakari Ailus

Re: [PATCH v8 0/6] v4l2-dev/ioctl: Add V4L2_CAP_IO_MC

[Hans Verkuil]

On 22/04/2020 19:22, Laurent Pinchart wrote:
> Hi Niklas,
> 
> (With a question for Hans below)
> 
> Thank you for the patches.
> 
> On Tue, Apr 21, 2020 at 03:57:37PM +0200, Niklas Söderlund wrote:
>> Hi,
>>
>> This series aims to reduce the amount of boiler plate code in video
>> device drivers who's inputs and/or outputs are controlled by the Media
>> Controller instead of the V4L2 API.
>>
>> Patch 1/6 adds a new video device capability flag V4L2_CAP_IO_MC which
>> if set provides helper implementations for the enum inputs and outputs
>> ioctls freeing the video device driver from the need to implement them.
>>
>> Patch 2/6 fix initialization of reserved fields in the cx18 and ivtv 
>> drivers which becomes a problem in 3/6 where Laurent adds mbus filters to
>> VIDIOC_ENUM_FMT.
>>
>> Patch 4/6, 5/6 and 6/6 converts the R-Car VIN, Intel IPU3 and VIMC
>> 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.
>>
>> A separate patch to v4l-utils have been posted as [1] to add a
>> test for this feature in v4l2-compliance.
>>
>> This version have been rebased to latest media-tree to account for the 
>> large shuffles of files. It has also replaced patch 2/6 with a different 
>> version that addresses both cx18 and ivtv instead of only ivtv.
> 
> It looks like everything has been reviewed. Hans, do you see any
> remaining blocker, or will you take it in your tree and send a pull
> request ?

Hmm, I'm getting v4l2-compliance errors when testing with vivid et al.
Looking into that now.

Regards,

	Hans

> 
>> 1. [PATCH 0/2] v4l2-compliance: add tests for V4L2_CAP_IO_MC
>>
>> Laurent Pinchart (2):
>>   media: pci: Fill v4l2_fmtdesc with designated initializers
>>   media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices
>>
>> Niklas Söderlund (4):
>>   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
>>   vimc: Make use of V4L2_CAP_IO_MC
>>
>>  .../media/v4l/vidioc-enum-fmt.rst             | 16 ++++-
>>  .../media/v4l/vidioc-querycap.rst             |  6 ++
>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>  drivers/media/pci/cx18/cx18-ioctl.c           | 22 ++++--
>>  drivers/media/pci/ivtv/ivtv-ioctl.c           | 26 +++----
>>  drivers/media/platform/rcar-vin/rcar-v4l2.c   | 40 ++++++-----
>>  .../media/test-drivers/vimc/vimc-capture.c    | 14 +++-
>>  drivers/media/v4l2-core/v4l2-dev.c            | 25 +++++--
>>  drivers/media/v4l2-core/v4l2-ioctl.c          | 70 +++++++++++++++++--
>>  drivers/staging/media/ipu3/ipu3-v4l2.c        | 64 ++---------------
>>  include/uapi/linux/videodev2.h                |  5 +-
>>  11 files changed, 180 insertions(+), 109 deletions(-)
>>
> 

Re: [PATCH v8 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Hans Verkuil]

On 21/04/2020 15:57, Niklas Söderlund wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> The VIDIOC_ENUM_FMT ioctl enumerates all formats supported by a video
> node. For MC-centric devices, its behaviour has always been ill-defined,
> with drivers implementing one of the following behaviours:
> 
> - No support for VIDIOC_ENUM_FMT at all
> - Enumerating all formats supported by the video node, regardless of the
>   configuration of the pipeline
> - Enumerating formats supported by the video node for the active
>   configuration of the connected subdevice
> 
> The first behaviour is obviously useless for applications. The second
> behaviour provides the most information, but doesn't offer a way to find
> what formats are compatible with a given pipeline configuration. The
> third behaviour fixes that, but with the drawback that applications
> can't enumerate all supported formats anymore, and have to modify the
> active configuration of the pipeline to enumerate formats.
> 
> The situation is messy as none of the implemented behaviours are ideal,
> and userspace can't predict what will happen as the behaviour is
> driver-specific.
> 
> To fix this, let's extend the VIDIOC_ENUM_FMT with a missing capability:
> enumerating pixel formats for a given media bus code. The media bus code
> is passed through the v4l2_fmtdesc structure in a new mbus_code field
> (repurposed from the reserved fields). With this capability in place,
> applications can enumerate pixel formats for a given media bus code
> without modifying the active configuration of the device.
> 
> The current behaviour of the ioctl is preserved when the new mbus_code
> field is set to 0, ensuring compatibility with existing userspace. The
> API extension is documented as mandatory for MC-centric devices (as
> advertised through the V4L2_CAP_IO_MC capability), allowing applications
> and compliance tools to easily determine the availability of the
> VIDIOC_ENUM_FMT extension.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v7
> - Update documentation.
> ---
>  .../userspace-api/media/v4l/vidioc-enum-fmt.rst  | 16 +++++++++++++---
>  drivers/media/v4l2-core/v4l2-ioctl.c             | 13 +++++++++++--
>  include/uapi/linux/videodev2.h                   |  3 ++-
>  3 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> index 7e3142e11d77d9c0..8dc8a73c320dda98 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst
> @@ -48,10 +48,20 @@ one until ``EINVAL`` is returned. If applicable, drivers shall return
>  formats in preference order, where preferred formats are returned before
>  (that is, with lower ``index`` value) less-preferred formats.
>  
> -.. note::
> +If the driver doesn't advertise the ``V4L2_CAP_IO_MC``
> +:ref:`device-capabilities <capability>`, applications shall initialize the

That's :ref:`capability <device-capabilities>`

> +``mbus_code`` field to zero. Drivers shall enumerate all image formats. The
> +enumerated formats may depend on the active input or output of the device.
>  
> -   After switching input or output the list of enumerated image
> -   formats may be different.
> +If the driver advertises the ``V4L2_CAP_IO_MC`` :ref:`device-capabilities
> +<capability>`, applications may initialize the ``mbus_code`` to a valid

Ditto.

> +:ref:`v4l2_mbus_pixelcode <media bus format code>`. If the ``mbus_code` field

:ref:`media bus format code <v4l2-mbus-pixelcode>`

``mbus_code` -> ``mbus_code``

> +is not zero, drivers shall restrict enumeration to only the image formats that
> +can produce (for video output devices) or be produced from (for video capture
> +devices) that media bus code. Regardless of the value of the ``mbus_code``
> +field, the enumerated image formats shall not depend on the active
> +configuration of the video device or device pipeline. Enumeration shall
> +otherwise operate as previously described.

The new field isn't documented, vidioc-enum-fmt.rst still shows 'reserved[4]' in
the struct v4l2_fmtdesc description.

>  
>  
>  .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index afd1f427df557f71..3e7b99fa415222c6 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -264,12 +264,13 @@ static void v4l_print_fmtdesc(const void *arg, bool write_only)
>  {
>  	const struct v4l2_fmtdesc *p = arg;
>  
> -	pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c, description='%.*s'\n",
> +	pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c, mbus_code=0x%04x, description='%.*s'\n",
>  		p->index, prt_names(p->type, v4l2_type_names),
>  		p->flags, (p->pixelformat & 0xff),
>  		(p->pixelformat >>  8) & 0xff,
>  		(p->pixelformat >> 16) & 0xff,
>  		(p->pixelformat >> 24) & 0xff,
> +		p->mbus_code,
>  		(int)sizeof(p->description), p->description);
>  }
>  
> @@ -1472,12 +1473,20 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
>  	struct video_device *vdev = video_devdata(file);
>  	struct v4l2_fmtdesc *p = arg;
>  	int ret = check_fmt(file, p->type);
> +	u32 mbus_code;
>  	u32 cap_mask;
>  
>  	if (ret)
>  		return ret;
>  	ret = -EINVAL;
>  
> +	if (p->mbus_code && !(vdev->device_caps & V4L2_CAP_IO_MC))
> +		return -EINVAL;

I suggest changing this to:

	if (!(vdev->device_caps & V4L2_CAP_IO_MC))
		p->mbus_code = 0;

This is much more robust for existing userspace since they may not have
properly zeroed the reserved array. Also, mbus_code only makes sense in
combination with CAP_IO_MC, so zeroing it if CAP_IO_MC isn't set makes sense.

The documentation probably needs to be updated to reflect this change as well.

I'll post a patch for v4l2-compliance as well.

Regards,

	Hans

> +
> +	mbus_code = p->mbus_code;
> +	CLEAR_AFTER_FIELD(p, type);
> +	p->mbus_code = mbus_code;
> +
>  	switch (p->type) {
>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
>  	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> @@ -2757,7 +2766,7 @@ DEFINE_V4L_STUB_FUNC(dv_timings_cap)
>  
>  static const struct v4l2_ioctl_info v4l2_ioctls[] = {
>  	IOCTL_INFO(VIDIOC_QUERYCAP, v4l_querycap, v4l_print_querycap, 0),
> -	IOCTL_INFO(VIDIOC_ENUM_FMT, v4l_enum_fmt, v4l_print_fmtdesc, INFO_FL_CLEAR(v4l2_fmtdesc, type)),
> +	IOCTL_INFO(VIDIOC_ENUM_FMT, v4l_enum_fmt, v4l_print_fmtdesc, 0),
>  	IOCTL_INFO(VIDIOC_G_FMT, v4l_g_fmt, v4l_print_format, 0),
>  	IOCTL_INFO(VIDIOC_S_FMT, v4l_s_fmt, v4l_print_format, INFO_FL_PRIO),
>  	IOCTL_INFO(VIDIOC_REQBUFS, v4l_reqbufs, v4l_print_requestbuffers, INFO_FL_PRIO | INFO_FL_QUEUE),
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index b18f3f7cde31c2e4..c3a1cf1c507f5506 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -784,7 +784,8 @@ struct v4l2_fmtdesc {
>  	__u32               flags;
>  	__u8		    description[32];   /* Description string */
>  	__u32		    pixelformat;       /* Format fourcc      */
> -	__u32		    reserved[4];
> +	__u32		    mbus_code;		/* Media bus code    */
> +	__u32		    reserved[3];
>  };
>  
>  #define V4L2_FMT_FLAG_COMPRESSED		0x0001
> 

[PATCH v8.1 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Laurent Pinchart]

The VIDIOC_ENUM_FMT ioctl enumerates all formats supported by a video
node. For MC-centric devices, its behaviour has always been ill-defined,
with drivers implementing one of the following behaviours:

- No support for VIDIOC_ENUM_FMT at all
- Enumerating all formats supported by the video node, regardless of the
  configuration of the pipeline
- Enumerating formats supported by the video node for the active
  configuration of the connected subdevice

The first behaviour is obviously useless for applications. The second
behaviour provides the most information, but doesn't offer a way to find
what formats are compatible with a given pipeline configuration. The
third behaviour fixes that, but with the drawback that applications
can't enumerate all supported formats anymore, and have to modify the
active configuration of the pipeline to enumerate formats.

The situation is messy as none of the implemented behaviours are ideal,
and userspace can't predict what will happen as the behaviour is
driver-specific.

To fix this, let's extend the VIDIOC_ENUM_FMT with a missing capability:
enumerating pixel formats for a given media bus code. The media bus code
is passed through the v4l2_fmtdesc structure in a new mbus_code field
(repurposed from the reserved fields). With this capability in place,
applications can enumerate pixel formats for a given media bus code
without modifying the active configuration of the device.

The current behaviour of the ioctl is preserved when the new mbus_code
field is set to 0, ensuring compatibility with existing userspace. The
API extension is documented as mandatory for MC-centric devices (as
advertised through the V4L2_CAP_IO_MC capability), allowing applications
and compliance tools to easily determine the availability of the
VIDIOC_ENUM_FMT extension.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
Changes since v8:

- Zero the mbus_code field if V4L2_CAP_IO_MC isn't set
- Document the new mbus_code field
- Fix ref tags in documentation
---
 .../media/uapi/v4l/vidioc-enum-fmt.rst        | 25 ++++++++++++++++---
 drivers/media/v4l2-core/v4l2-ioctl.c          | 13 ++++++++--
 include/uapi/linux/videodev2.h                |  3 ++-
 3 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
index 8ca6ab701e4a..a987debc7654 100644
--- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
+++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
@@ -48,10 +48,21 @@ one until ``EINVAL`` is returned. If applicable, drivers shall return
 formats in preference order, where preferred formats are returned before
 (that is, with lower ``index`` value) less-preferred formats.
 
-.. note::
+If the driver doesn't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
+<device-capabilities>`, applications shall initialize the ``mbus_code`` field
+to zero and drivers shall ignore the value of the field.  Drivers shall
+enumerate all image formats. The enumerated formats may depend on the active
+input or output of the device.
 
-   After switching input or output the list of enumerated image
-   formats may be different.
+If the driver advertises the ``V4L2_CAP_IO_MC`` :ref:`capability
+<device-capabilities>`, applications may initialize the ``mbus_code`` field to
+a valid :ref:`media bus format code <v4l2-mbus-pixelcode>`. If the
+``mbus_code`` field is not zero, drivers shall restrict enumeration to only the
+image formats that can produce (for video output devices) or be produced from
+(for video capture devices) that media bus code.  Regardless of the value of
+the ``mbus_code`` field, the enumerated image formats shall not depend on the
+active configuration of the video device or device pipeline. Enumeration shall
+otherwise operate as previously described.
 
 
 .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
@@ -106,7 +117,13 @@ formats in preference order, where preferred formats are returned before
 	   These codes are not the same as those used
 	   in the Windows world.
     * - __u32
-      - ``reserved``\ [4]
+      - ``mbus_code``
+      - Media bus code restricting the enumerated formats, set by the
+        application. Only applicable to drivers that advertise the
+        ``V4L2_CAP_IO_MC`` :ref:`capability <device-capabilities>`, shall be 0
+        otherwise.
+    * - __u32
+      - ``reserved``\ [3]
       - Reserved for future extensions. Drivers must set the array to
 	zero.
 
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 3545a8adf844..0550f20d7177 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -264,12 +264,13 @@ static void v4l_print_fmtdesc(const void *arg, bool write_only)
 {
 	const struct v4l2_fmtdesc *p = arg;
 
-	pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c, description='%.*s'\n",
+	pr_cont("index=%u, type=%s, flags=0x%x, pixelformat=%c%c%c%c, mbus_code=0x%04x, description='%.*s'\n",
 		p->index, prt_names(p->type, v4l2_type_names),
 		p->flags, (p->pixelformat & 0xff),
 		(p->pixelformat >>  8) & 0xff,
 		(p->pixelformat >> 16) & 0xff,
 		(p->pixelformat >> 24) & 0xff,
+		p->mbus_code,
 		(int)sizeof(p->description), p->description);
 }
 
@@ -1467,12 +1468,20 @@ static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
 	struct video_device *vdev = video_devdata(file);
 	struct v4l2_fmtdesc *p = arg;
 	int ret = check_fmt(file, p->type);
+	u32 mbus_code;
 	u32 cap_mask;
 
 	if (ret)
 		return ret;
 	ret = -EINVAL;
 
+	if (!(vdev->device_caps & V4L2_CAP_IO_MC))
+		p->mbus_code = 0;
+
+	mbus_code = p->mbus_code;
+	CLEAR_AFTER_FIELD(p, type);
+	p->mbus_code = mbus_code;
+
 	switch (p->type) {
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
@@ -2752,7 +2761,7 @@ DEFINE_V4L_STUB_FUNC(dv_timings_cap)
 
 static const struct v4l2_ioctl_info v4l2_ioctls[] = {
 	IOCTL_INFO(VIDIOC_QUERYCAP, v4l_querycap, v4l_print_querycap, 0),
-	IOCTL_INFO(VIDIOC_ENUM_FMT, v4l_enum_fmt, v4l_print_fmtdesc, INFO_FL_CLEAR(v4l2_fmtdesc, type)),
+	IOCTL_INFO(VIDIOC_ENUM_FMT, v4l_enum_fmt, v4l_print_fmtdesc, 0),
 	IOCTL_INFO(VIDIOC_G_FMT, v4l_g_fmt, v4l_print_format, 0),
 	IOCTL_INFO(VIDIOC_S_FMT, v4l_s_fmt, v4l_print_format, INFO_FL_PRIO),
 	IOCTL_INFO(VIDIOC_REQBUFS, v4l_reqbufs, v4l_print_requestbuffers, INFO_FL_PRIO | INFO_FL_QUEUE),
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index c793263a3705..0ece960844a5 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -779,7 +779,8 @@ struct v4l2_fmtdesc {
 	__u32               flags;
 	__u8		    description[32];   /* Description string */
 	__u32		    pixelformat;       /* Format fourcc      */
-	__u32		    reserved[4];
+	__u32		    mbus_code;		/* Media bus code    */
+	__u32		    reserved[3];
 };
 
 #define V4L2_FMT_FLAG_COMPRESSED		0x0001
-- 
Regards,

Laurent Pinchart

Re: [PATCH v8.1 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Mauro Carvalho Chehab]

Em Fri, 24 Apr 2020 16:43:31 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> The VIDIOC_ENUM_FMT ioctl enumerates all formats supported by a video
> node. For MC-centric devices, its behaviour has always been ill-defined,
> with drivers implementing one of the following behaviours:
> 

...

The patch itself is OK. However, there's a change you did at the
documentation that it is unrelated. 

See below.

> diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> index 8ca6ab701e4a..a987debc7654 100644
> --- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> @@ -48,10 +48,21 @@ one until ``EINVAL`` is returned. If applicable, drivers shall return
>  formats in preference order, where preferred formats are returned before
>  (that is, with lower ``index`` value) less-preferred formats.
>  
> -.. note::
> -   After switching input or output the list of enumerated image
> -   formats may be different.

Why are you dropping this note?

This basically reverts this changeset:

  commit 93828d6438081649e81b8681df9bf6ad5d691650
  Author: Hans Verkuil <hans.verkuil@cisco.com>
  Date:   Mon Sep 3 09:37:18 2012 -0300

    [media] DocBook: make the G/S/TRY_FMT specification more strict
    
    - S/TRY_FMT should always succeed, unless an invalid type field is passed in.
    - TRY_FMT should give the same result as S_FMT, all other things being equal.
    - ENUMFMT may return different formats for different inputs or outputs.
    This was decided during the 2012 Media Workshop.
    
    Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
    Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
    Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
    Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

As far as I remember, from our 2012 discussions, some drivers may change 
the enumerated image formats when some ioctls like VIDIOC_S_INPUT and
VIDIOC_S_OUTPUT ioctls are used. I also vaguely remember that 90 and 270
degrees rotation would equally affect it.

Perhaps, the removal was just some mistake. If so, please re-submit
another patch without it.

Otherwise, if are there any good reasons for such change, and it won't
cause any API regressions, please place it on a separate patch, clearly
that.

... Or, maybe you felt that it won't make sense for MC-centric devices.
On such case, please improve the note stating it on a way that it would
be understandable on both MC-centric and bridge-centrid drivers. 

Thanks,
Mauro

Re: [PATCH v8.1 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Laurent Pinchart]

Hi Mauro,

On Wed, Apr 29, 2020 at 06:27:39PM +0200, Mauro Carvalho Chehab wrote:
> Em Fri, 24 Apr 2020 16:43:31 +0300 Laurent Pinchart escreveu:
> 
> > The VIDIOC_ENUM_FMT ioctl enumerates all formats supported by a video
> > node. For MC-centric devices, its behaviour has always been ill-defined,
> > with drivers implementing one of the following behaviours:
> > 
> 
> ...
> 
> The patch itself is OK. However, there's a change you did at the
> documentation that it is unrelated. 
> 
> See below.
> 
> > diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > index 8ca6ab701e4a..a987debc7654 100644
> > --- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > +++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > @@ -48,10 +48,21 @@ one until ``EINVAL`` is returned. If applicable, drivers shall return
> >  formats in preference order, where preferred formats are returned before
> >  (that is, with lower ``index`` value) less-preferred formats.
> >  
> > -.. note::
> > -   After switching input or output the list of enumerated image
> > -   formats may be different.
> 
> Why are you dropping this note?
> 
> This basically reverts this changeset:
> 
>   commit 93828d6438081649e81b8681df9bf6ad5d691650
>   Author: Hans Verkuil <hans.verkuil@cisco.com>
>   Date:   Mon Sep 3 09:37:18 2012 -0300
> 
>     [media] DocBook: make the G/S/TRY_FMT specification more strict
>     
>     - S/TRY_FMT should always succeed, unless an invalid type field is passed in.
>     - TRY_FMT should give the same result as S_FMT, all other things being equal.
>     - ENUMFMT may return different formats for different inputs or outputs.
>     This was decided during the 2012 Media Workshop.
>     
>     Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>     Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>     Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> 
> As far as I remember, from our 2012 discussions, some drivers may change 
> the enumerated image formats when some ioctls like VIDIOC_S_INPUT and
> VIDIOC_S_OUTPUT ioctls are used. I also vaguely remember that 90 and 270
> degrees rotation would equally affect it.
> 
> Perhaps, the removal was just some mistake. If so, please re-submit
> another patch without it.
> 
> Otherwise, if are there any good reasons for such change, and it won't
> cause any API regressions, please place it on a separate patch, clearly
> that.
> 
> ... Or, maybe you felt that it won't make sense for MC-centric devices.
> On such case, please improve the note stating it on a way that it would
> be understandable on both MC-centric and bridge-centrid drivers.

I'm not dropping the requirement, I'm rewriting it :-) The patch indeed
removes the above, but adds the following

----
If the driver doesn't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
<device-capabilities>`, applications shall initialize the ``mbus_code`` field
to zero and drivers shall ignore the value of the field.  Drivers shall
enumerate all image formats. The enumerated formats may depend on the active
input or output of the device.

If the driver advertises the ``V4L2_CAP_IO_MC`` :ref:`capability
<device-capabilities>`, applications may initialize the ``mbus_code`` field to
a valid :ref:`media bus format code <v4l2-mbus-pixelcode>`. If the
``mbus_code`` field is not zero, drivers shall restrict enumeration to only the
image formats that can produce (for video output devices) or be produced from
(for video capture devices) that media bus code.  Regardless of the value of
the ``mbus_code`` field, the enumerated image formats shall not depend on the
active configuration of the video device or device pipeline. Enumeration shall
otherwise operate as previously described.
----

Note the last sentence for the !V4L2_CAP_IO_MC case:

"The enumerated formats may depend on the active input or output of the
device."

We can extend it with

"The enumerated formats may depend on the active input or output of the
device, switching the input or output may thus produce different lists
of enumerated formats."

I think it's a bit overkill as it's saying the same thing twice, but if
you prefer that, I'm fine with it.

For the V4L2_CAP_IO_MC case there's no .s_input() or .s_output(), so the
note isn't applicable.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8.1 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Mauro Carvalho Chehab]

Em Wed, 29 Apr 2020 19:38:49 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Wed, Apr 29, 2020 at 06:27:39PM +0200, Mauro Carvalho Chehab wrote:
> > Em Fri, 24 Apr 2020 16:43:31 +0300 Laurent Pinchart escreveu:
> >   
> > > The VIDIOC_ENUM_FMT ioctl enumerates all formats supported by a video
> > > node. For MC-centric devices, its behaviour has always been ill-defined,
> > > with drivers implementing one of the following behaviours:
> > >   
> > 
> > ...
> > 
> > The patch itself is OK. However, there's a change you did at the
> > documentation that it is unrelated. 
> > 
> > See below.
> >   
> > > diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > > index 8ca6ab701e4a..a987debc7654 100644
> > > --- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > > +++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > > @@ -48,10 +48,21 @@ one until ``EINVAL`` is returned. If applicable, drivers shall return
> > >  formats in preference order, where preferred formats are returned before
> > >  (that is, with lower ``index`` value) less-preferred formats.
> > >  
> > > -.. note::
> > > -   After switching input or output the list of enumerated image
> > > -   formats may be different.  
> > 
> > Why are you dropping this note?
> > 
> > This basically reverts this changeset:
> > 
> >   commit 93828d6438081649e81b8681df9bf6ad5d691650
> >   Author: Hans Verkuil <hans.verkuil@cisco.com>
> >   Date:   Mon Sep 3 09:37:18 2012 -0300
> > 
> >     [media] DocBook: make the G/S/TRY_FMT specification more strict
> >     
> >     - S/TRY_FMT should always succeed, unless an invalid type field is passed in.
> >     - TRY_FMT should give the same result as S_FMT, all other things being equal.
> >     - ENUMFMT may return different formats for different inputs or outputs.
> >     This was decided during the 2012 Media Workshop.
> >     
> >     Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >     Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >     Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
> >     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > 
> > As far as I remember, from our 2012 discussions, some drivers may change 
> > the enumerated image formats when some ioctls like VIDIOC_S_INPUT and
> > VIDIOC_S_OUTPUT ioctls are used. I also vaguely remember that 90 and 270
> > degrees rotation would equally affect it.
> > 
> > Perhaps, the removal was just some mistake. If so, please re-submit
> > another patch without it.
> > 
> > Otherwise, if are there any good reasons for such change, and it won't
> > cause any API regressions, please place it on a separate patch, clearly
> > that.
> > 
> > ... Or, maybe you felt that it won't make sense for MC-centric devices.
> > On such case, please improve the note stating it on a way that it would
> > be understandable on both MC-centric and bridge-centrid drivers.  
> 
> I'm not dropping the requirement, I'm rewriting it :-) The patch indeed
> removes the above, but adds the following
> 
> ----
> If the driver doesn't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
> <device-capabilities>`, applications shall initialize the ``mbus_code`` field
> to zero and drivers shall ignore the value of the field.  Drivers shall
> enumerate all image formats. The enumerated formats may depend on the active
> input or output of the device.
> 
> If the driver advertises the ``V4L2_CAP_IO_MC`` :ref:`capability
> <device-capabilities>`, applications may initialize the ``mbus_code`` field to
> a valid :ref:`media bus format code <v4l2-mbus-pixelcode>`. If the
> ``mbus_code`` field is not zero, drivers shall restrict enumeration to only the
> image formats that can produce (for video output devices) or be produced from
> (for video capture devices) that media bus code.  Regardless of the value of
> the ``mbus_code`` field, the enumerated image formats shall not depend on the
> active configuration of the video device or device pipeline. Enumeration shall
> otherwise operate as previously described.

Hmm... it took me re-reading this text 4 or 5 times, in order to understand
that you're actually meaning bridge-centric devices here :-)

Also, you placed two independent and unrelated information at the same
paragraph.

IMHO, you should let it more clear, like, for example adding a numerated
list, like:

1. Bridge-centric devices

   As such devices don't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
   <device-capabilities>`, applications shall initialize the ``mbus_code`` field
   to zero and drivers shall ignore the value of the field.  

   Drivers shall enumerate all image formats. The enumerated formats may depend
   on the active input or output of the device.

2. MC-centric devices

  As such devices advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
  <device-capabilities>`, applications may initialize the ``mbus_code`` field to
  a valid :ref:`media bus format code <v4l2-mbus-pixelcode>`. 

  If the ``mbus_code`` field is not zero, drivers shall restrict enumeration to 
  only the image formats that can produce (for video output devices) or be produced 
  from (for video capture devices) that media bus code.  

  Regardless of the value of the ``mbus_code`` field, the enumerated image formats
  shall not depend on the active configuration of the video device or device
  pipeline. Enumeration shall otherwise operate as previously described.

- 

On a side note: can a MC-centric device fill ``mbus_code`` field with zero?
The second paragraph seems to contradict the first one with mandates that
``mbus_code`` should be a valid format.

> ----
> 
> Note the last sentence for the !V4L2_CAP_IO_MC case:
> 
> "The enumerated formats may depend on the active input or output of the
> device."
> 
> We can extend it with
> 
> "The enumerated formats may depend on the active input or output of the
> device, switching the input or output may thus produce different lists
> of enumerated formats."

That sounds better, but "may" seems to weak for my taste. So, I would 
also add:

  Applications should (re)call VIDIOC_ENUM_FMT after changing the ative
  input or output of the device.

> 
> I think it's a bit overkill as it's saying the same thing twice, but if
> you prefer that, I'm fine with it.
> 
> For the V4L2_CAP_IO_MC case there's no .s_input() or .s_output(), so the
> note isn't applicable.

Ok.

Thanks,
Mauro

Re: [PATCH v8.1 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Laurent Pinchart]

Hi Mauro,

On Wed, Apr 29, 2020 at 08:01:40PM +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 29 Apr 2020 19:38:49 +0300 Laurent Pinchart escreveu:
> > On Wed, Apr 29, 2020 at 06:27:39PM +0200, Mauro Carvalho Chehab wrote:
> > > Em Fri, 24 Apr 2020 16:43:31 +0300 Laurent Pinchart escreveu:
> > >   
> > > > The VIDIOC_ENUM_FMT ioctl enumerates all formats supported by a video
> > > > node. For MC-centric devices, its behaviour has always been ill-defined,
> > > > with drivers implementing one of the following behaviours:
> > > 
> > > ...
> > > 
> > > The patch itself is OK. However, there's a change you did at the
> > > documentation that it is unrelated. 
> > > 
> > > See below.
> > >   
> > > > diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > > > index 8ca6ab701e4a..a987debc7654 100644
> > > > --- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > > > +++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > > > @@ -48,10 +48,21 @@ one until ``EINVAL`` is returned. If applicable, drivers shall return
> > > >  formats in preference order, where preferred formats are returned before
> > > >  (that is, with lower ``index`` value) less-preferred formats.
> > > >  
> > > > -.. note::
> > > > -   After switching input or output the list of enumerated image
> > > > -   formats may be different.  
> > > 
> > > Why are you dropping this note?
> > > 
> > > This basically reverts this changeset:
> > > 
> > >   commit 93828d6438081649e81b8681df9bf6ad5d691650
> > >   Author: Hans Verkuil <hans.verkuil@cisco.com>
> > >   Date:   Mon Sep 3 09:37:18 2012 -0300
> > > 
> > >     [media] DocBook: make the G/S/TRY_FMT specification more strict
> > >     
> > >     - S/TRY_FMT should always succeed, unless an invalid type field is passed in.
> > >     - TRY_FMT should give the same result as S_FMT, all other things being equal.
> > >     - ENUMFMT may return different formats for different inputs or outputs.
> > >     This was decided during the 2012 Media Workshop.
> > >     
> > >     Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > >     Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > >     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >     Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
> > >     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > > 
> > > As far as I remember, from our 2012 discussions, some drivers may change 
> > > the enumerated image formats when some ioctls like VIDIOC_S_INPUT and
> > > VIDIOC_S_OUTPUT ioctls are used. I also vaguely remember that 90 and 270
> > > degrees rotation would equally affect it.
> > > 
> > > Perhaps, the removal was just some mistake. If so, please re-submit
> > > another patch without it.
> > > 
> > > Otherwise, if are there any good reasons for such change, and it won't
> > > cause any API regressions, please place it on a separate patch, clearly
> > > that.
> > > 
> > > ... Or, maybe you felt that it won't make sense for MC-centric devices.
> > > On such case, please improve the note stating it on a way that it would
> > > be understandable on both MC-centric and bridge-centrid drivers.  
> > 
> > I'm not dropping the requirement, I'm rewriting it :-) The patch indeed
> > removes the above, but adds the following
> > 
> > ----
> > If the driver doesn't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
> > <device-capabilities>`, applications shall initialize the ``mbus_code`` field
> > to zero and drivers shall ignore the value of the field.  Drivers shall
> > enumerate all image formats. The enumerated formats may depend on the active
> > input or output of the device.
> > 
> > If the driver advertises the ``V4L2_CAP_IO_MC`` :ref:`capability
> > <device-capabilities>`, applications may initialize the ``mbus_code`` field to
> > a valid :ref:`media bus format code <v4l2-mbus-pixelcode>`. If the
> > ``mbus_code`` field is not zero, drivers shall restrict enumeration to only the
> > image formats that can produce (for video output devices) or be produced from
> > (for video capture devices) that media bus code.  Regardless of the value of
> > the ``mbus_code`` field, the enumerated image formats shall not depend on the
> > active configuration of the video device or device pipeline. Enumeration shall
> > otherwise operate as previously described.
> 
> Hmm... it took me re-reading this text 4 or 5 times, in order to understand
> that you're actually meaning bridge-centric devices here :-)
> 
> Also, you placed two independent and unrelated information at the same
> paragraph.
> 
> IMHO, you should let it more clear, like, for example adding a numerated
> list, like:
> 
> 1. Bridge-centric devices
> 
>    As such devices don't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
>    <device-capabilities>`, applications shall initialize the ``mbus_code`` field
>    to zero and drivers shall ignore the value of the field.

I could settle for

   These devices don't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
   <device-capabilities>`. Applications shall initialize the ``mbus_code`` field
   to zero and drivers shall ignore the value of the field.

and similarly below. It bothers me though, as "bridge" isn't formally
defined anywhere in the userspace API documentation. It's more formal to
explain the behaviour of individual ioctls based solely on the
V4L2_CAP_IO_MC flag, and gather all the explanation of what
bridge-centric vs. MC-centric means in a single place, an introductory
section, instead of spreading it through documentation of individual
ioctls. V4L2 has more UAPI documentation than most other kernel APIs,
but there are lots of places where details are not very clear.
Standardizing ioctl documentation on the use of common vocabulary
("may", "shall", ...) and using clearly defined concepts (e.g.
V4L2_CAP_IO_MC) instead of losely defined ones (e.g. Bridge-centric
devices) that are explained in non-normative sections would increase
clarity. I thus prefer the wording in v8.1 of this patch, or possibly
with the small extension I've proposed in my previous e-mail.

Hans, Sakari, what do you think ?

> 
>    Drivers shall enumerate all image formats. The enumerated formats may depend
>    on the active input or output of the device.
> 
> 2. MC-centric devices
> 
>   As such devices advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
>   <device-capabilities>`, applications may initialize the ``mbus_code`` field to
>   a valid :ref:`media bus format code <v4l2-mbus-pixelcode>`. 
> 
>   If the ``mbus_code`` field is not zero, drivers shall restrict enumeration to 
>   only the image formats that can produce (for video output devices) or be produced 
>   from (for video capture devices) that media bus code.  
> 
>   Regardless of the value of the ``mbus_code`` field, the enumerated image formats
>   shall not depend on the active configuration of the video device or device
>   pipeline. Enumeration shall otherwise operate as previously described.
> 
> - 
> 
> On a side note: can a MC-centric device fill ``mbus_code`` field with zero?

I assume you mean application here ? mbus_code is set by applications.

> The second paragraph seems to contradict the first one with mandates that
> ``mbus_code`` should be a valid format.

The first paragraph says that applications "may" set the field. The
second paragraph explains what happens if they do.

> > ----
> > 
> > Note the last sentence for the !V4L2_CAP_IO_MC case:
> > 
> > "The enumerated formats may depend on the active input or output of the
> > device."
> > 
> > We can extend it with
> > 
> > "The enumerated formats may depend on the active input or output of the
> > device, switching the input or output may thus produce different lists
> > of enumerated formats."
> 
> That sounds better, but "may" seems to weak for my taste. So, I would 
> also add:
> 
>   Applications should (re)call VIDIOC_ENUM_FMT after changing the ative
>   input or output of the device.

That only applies if they need to enumerate formats in the first place.
I'd prefer avoiding making this more complex. "may" is standard
vocabulary in specifications to indicate a permitted behaviour. How
applications react to that is based on their needs, and I don't think
here is the right place to try and imagine what those needs are.

> > I think it's a bit overkill as it's saying the same thing twice, but if
> > you prefer that, I'm fine with it.
> > 
> > For the V4L2_CAP_IO_MC case there's no .s_input() or .s_output(), so the
> > note isn't applicable.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8.1 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Mauro Carvalho Chehab]

Em Wed, 29 Apr 2020 21:50:33 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Wed, Apr 29, 2020 at 08:01:40PM +0200, Mauro Carvalho Chehab wrote:
> > Em Wed, 29 Apr 2020 19:38:49 +0300 Laurent Pinchart escreveu:  
> > > On Wed, Apr 29, 2020 at 06:27:39PM +0200, Mauro Carvalho Chehab wrote:  
> > > > Em Fri, 24 Apr 2020 16:43:31 +0300 Laurent Pinchart escreveu:
> > > >     
> > > > > The VIDIOC_ENUM_FMT ioctl enumerates all formats supported by a video
> > > > > node. For MC-centric devices, its behaviour has always been ill-defined,
> > > > > with drivers implementing one of the following behaviours:  
> > > > 
> > > > ...
> > > > 
> > > > The patch itself is OK. However, there's a change you did at the
> > > > documentation that it is unrelated. 
> > > > 
> > > > See below.
> > > >     
> > > > > diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > > > > index 8ca6ab701e4a..a987debc7654 100644
> > > > > --- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > > > > +++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > > > > @@ -48,10 +48,21 @@ one until ``EINVAL`` is returned. If applicable, drivers shall return
> > > > >  formats in preference order, where preferred formats are returned before
> > > > >  (that is, with lower ``index`` value) less-preferred formats.
> > > > >  
> > > > > -.. note::
> > > > > -   After switching input or output the list of enumerated image
> > > > > -   formats may be different.    
> > > > 
> > > > Why are you dropping this note?
> > > > 
> > > > This basically reverts this changeset:
> > > > 
> > > >   commit 93828d6438081649e81b8681df9bf6ad5d691650
> > > >   Author: Hans Verkuil <hans.verkuil@cisco.com>
> > > >   Date:   Mon Sep 3 09:37:18 2012 -0300
> > > > 
> > > >     [media] DocBook: make the G/S/TRY_FMT specification more strict
> > > >     
> > > >     - S/TRY_FMT should always succeed, unless an invalid type field is passed in.
> > > >     - TRY_FMT should give the same result as S_FMT, all other things being equal.
> > > >     - ENUMFMT may return different formats for different inputs or outputs.
> > > >     This was decided during the 2012 Media Workshop.
> > > >     
> > > >     Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > >     Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > > >     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >     Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > >     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > > > 
> > > > As far as I remember, from our 2012 discussions, some drivers may change 
> > > > the enumerated image formats when some ioctls like VIDIOC_S_INPUT and
> > > > VIDIOC_S_OUTPUT ioctls are used. I also vaguely remember that 90 and 270
> > > > degrees rotation would equally affect it.
> > > > 
> > > > Perhaps, the removal was just some mistake. If so, please re-submit
> > > > another patch without it.
> > > > 
> > > > Otherwise, if are there any good reasons for such change, and it won't
> > > > cause any API regressions, please place it on a separate patch, clearly
> > > > that.
> > > > 
> > > > ... Or, maybe you felt that it won't make sense for MC-centric devices.
> > > > On such case, please improve the note stating it on a way that it would
> > > > be understandable on both MC-centric and bridge-centrid drivers.    
> > > 
> > > I'm not dropping the requirement, I'm rewriting it :-) The patch indeed
> > > removes the above, but adds the following
> > > 
> > > ----
> > > If the driver doesn't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
> > > <device-capabilities>`, applications shall initialize the ``mbus_code`` field
> > > to zero and drivers shall ignore the value of the field.  Drivers shall
> > > enumerate all image formats. The enumerated formats may depend on the active
> > > input or output of the device.
> > > 
> > > If the driver advertises the ``V4L2_CAP_IO_MC`` :ref:`capability
> > > <device-capabilities>`, applications may initialize the ``mbus_code`` field to
> > > a valid :ref:`media bus format code <v4l2-mbus-pixelcode>`. If the
> > > ``mbus_code`` field is not zero, drivers shall restrict enumeration to only the
> > > image formats that can produce (for video output devices) or be produced from
> > > (for video capture devices) that media bus code.  Regardless of the value of
> > > the ``mbus_code`` field, the enumerated image formats shall not depend on the
> > > active configuration of the video device or device pipeline. Enumeration shall
> > > otherwise operate as previously described.  
> > 
> > Hmm... it took me re-reading this text 4 or 5 times, in order to understand
> > that you're actually meaning bridge-centric devices here :-)
> > 
> > Also, you placed two independent and unrelated information at the same
> > paragraph.
> > 
> > IMHO, you should let it more clear, like, for example adding a numerated
> > list, like:
> > 
> > 1. Bridge-centric devices
> > 
> >    As such devices don't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
> >    <device-capabilities>`, applications shall initialize the ``mbus_code`` field
> >    to zero and drivers shall ignore the value of the field.  
> 
> I could settle for
> 
>    These devices don't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
>    <device-capabilities>`. Applications shall initialize the ``mbus_code`` field
>    to zero and drivers shall ignore the value of the field.
> 
> and similarly below. 

Works for me.

> It bothers me though, as "bridge" isn't formally
> defined anywhere in the userspace API documentation.

Actually, I submitted some patches some time ago related to documenting
it. They are the "v8, x/7" at patchwork:

	https://patchwork.linuxtv.org/project/linux-media/list/?series=&submitter=&state=&q=&archive=&delegate=1

> It's more formal to
> explain the behaviour of individual ioctls based solely on the
> V4L2_CAP_IO_MC flag, and gather all the explanation of what
> bridge-centric vs. MC-centric means in a single place, an introductory
> section, instead of spreading it through documentation of individual
> ioctls. 

This patch in particular (from the above series) does that:

	https://patchwork.linuxtv.org/patch/44904/

-

That's said, that series is outdated. IMO, we should do another spin 
on that, in the light of the V4L2_CAP_IO_MC changes (making the 
introductory chapter point to it).

I'll seek for some time and re-submit such patchset.

> V4L2 has more UAPI documentation than most other kernel APIs,
> but there are lots of places where details are not very clear.
> Standardizing ioctl documentation on the use of common vocabulary
> ("may", "shall", ...) and using clearly defined concepts (e.g.
> V4L2_CAP_IO_MC) instead of losely defined ones (e.g. Bridge-centric
> devices) that are explained in non-normative sections would increase
> clarity. I thus prefer the wording in v8.1 of this patch, or possibly
> with the small extension I've proposed in my previous e-mail.

See, my proposal is to have both ;-)

The list have the "less formal" definition (MC-centric x bridge-centric),
with is how media developers usually refer to those (as you did at
the subject of this patch series), while the contents preserve a more 
technical definition, based on V4L2_CAP_IO_MC field.

> 
> Hans, Sakari, what do you think ?
> 
> > 
> >    Drivers shall enumerate all image formats. The enumerated formats may depend
> >    on the active input or output of the device.
> > 
> > 2. MC-centric devices
> > 
> >   As such devices advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
> >   <device-capabilities>`, applications may initialize the ``mbus_code`` field to
> >   a valid :ref:`media bus format code <v4l2-mbus-pixelcode>`. 
> > 
> >   If the ``mbus_code`` field is not zero, drivers shall restrict enumeration to 
> >   only the image formats that can produce (for video output devices) or be produced 
> >   from (for video capture devices) that media bus code.  
> > 
> >   Regardless of the value of the ``mbus_code`` field, the enumerated image formats
> >   shall not depend on the active configuration of the video device or device
> >   pipeline. Enumeration shall otherwise operate as previously described.
> > 
> > - 
> > 
> > On a side note: can a MC-centric device fill ``mbus_code`` field with zero?  
> 
> I assume you mean application here ? mbus_code is set by applications.
> 
> > The second paragraph seems to contradict the first one with mandates that
> > ``mbus_code`` should be a valid format.  
> 
> The first paragraph says that applications "may" set the field. The
> second paragraph explains what happens if they do.

Ok. I would then write it as:

 2. MC-centric devices
 
   These devices advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
   <device-capabilities>`.

   Applications shall initialize the ``mbus_code`` field to zero or to
   a valid :ref:`media bus format code <v4l2-mbus-pixelcode>`. 
 
   If the ``mbus_code`` field is zero, the driver shall return all
   media formats supported by the driver. Otherwise, drivers shall
   restrict enumeration to only the image formats that can produce 
   (for video output devices) or be produced from (for video capture
   devices) that media bus code.

   If the ``mbus_code`` is not valid or if it is not supported, the
   driver shall return -EINVAL.
 
   Regardless of the value of the ``mbus_code`` field, the enumerated image
   formats shall not depend on the active configuration of the video device
   or device pipeline. Enumeration shall otherwise operate as previously
   described.


> 
> > > ----
> > > 
> > > Note the last sentence for the !V4L2_CAP_IO_MC case:
> > > 
> > > "The enumerated formats may depend on the active input or output of the
> > > device."
> > > 
> > > We can extend it with
> > > 
> > > "The enumerated formats may depend on the active input or output of the
> > > device, switching the input or output may thus produce different lists
> > > of enumerated formats."  
> > 
> > That sounds better, but "may" seems to weak for my taste. So, I would 
> > also add:
> > 
> >   Applications should (re)call VIDIOC_ENUM_FMT after changing the ative
> >   input or output of the device.  
> 
> That only applies if they need to enumerate formats in the first place.
> I'd prefer avoiding making this more complex. "may" is standard
> vocabulary in specifications to indicate a permitted behaviour. How
> applications react to that is based on their needs, and I don't think
> here is the right place to try and imagine what those needs are.

Works for me.

Thanks,
Mauro

Re: [PATCH v8.1 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Sakari Ailus]

Hi Laurent, Mauro,

On Wed, Apr 29, 2020 at 09:50:33PM +0300, Laurent Pinchart wrote:
> Hi Mauro,
> 
> On Wed, Apr 29, 2020 at 08:01:40PM +0200, Mauro Carvalho Chehab wrote:
> > Em Wed, 29 Apr 2020 19:38:49 +0300 Laurent Pinchart escreveu:
> > > On Wed, Apr 29, 2020 at 06:27:39PM +0200, Mauro Carvalho Chehab wrote:
> > > > Em Fri, 24 Apr 2020 16:43:31 +0300 Laurent Pinchart escreveu:
> > > >   
> > > > > The VIDIOC_ENUM_FMT ioctl enumerates all formats supported by a video
> > > > > node. For MC-centric devices, its behaviour has always been ill-defined,
> > > > > with drivers implementing one of the following behaviours:
> > > > 
> > > > ...
> > > > 
> > > > The patch itself is OK. However, there's a change you did at the
> > > > documentation that it is unrelated. 
> > > > 
> > > > See below.
> > > >   
> > > > > diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > > > > index 8ca6ab701e4a..a987debc7654 100644
> > > > > --- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > > > > +++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
> > > > > @@ -48,10 +48,21 @@ one until ``EINVAL`` is returned. If applicable, drivers shall return
> > > > >  formats in preference order, where preferred formats are returned before
> > > > >  (that is, with lower ``index`` value) less-preferred formats.
> > > > >  
> > > > > -.. note::
> > > > > -   After switching input or output the list of enumerated image
> > > > > -   formats may be different.  
> > > > 
> > > > Why are you dropping this note?
> > > > 
> > > > This basically reverts this changeset:
> > > > 
> > > >   commit 93828d6438081649e81b8681df9bf6ad5d691650
> > > >   Author: Hans Verkuil <hans.verkuil@cisco.com>
> > > >   Date:   Mon Sep 3 09:37:18 2012 -0300
> > > > 
> > > >     [media] DocBook: make the G/S/TRY_FMT specification more strict
> > > >     
> > > >     - S/TRY_FMT should always succeed, unless an invalid type field is passed in.
> > > >     - TRY_FMT should give the same result as S_FMT, all other things being equal.
> > > >     - ENUMFMT may return different formats for different inputs or outputs.
> > > >     This was decided during the 2012 Media Workshop.
> > > >     
> > > >     Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > >     Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > > >     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >     Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
> > > >     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > > > 
> > > > As far as I remember, from our 2012 discussions, some drivers may change 
> > > > the enumerated image formats when some ioctls like VIDIOC_S_INPUT and
> > > > VIDIOC_S_OUTPUT ioctls are used. I also vaguely remember that 90 and 270
> > > > degrees rotation would equally affect it.
> > > > 
> > > > Perhaps, the removal was just some mistake. If so, please re-submit
> > > > another patch without it.
> > > > 
> > > > Otherwise, if are there any good reasons for such change, and it won't
> > > > cause any API regressions, please place it on a separate patch, clearly
> > > > that.
> > > > 
> > > > ... Or, maybe you felt that it won't make sense for MC-centric devices.
> > > > On such case, please improve the note stating it on a way that it would
> > > > be understandable on both MC-centric and bridge-centrid drivers.  
> > > 
> > > I'm not dropping the requirement, I'm rewriting it :-) The patch indeed
> > > removes the above, but adds the following
> > > 
> > > ----
> > > If the driver doesn't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
> > > <device-capabilities>`, applications shall initialize the ``mbus_code`` field
> > > to zero and drivers shall ignore the value of the field.  Drivers shall
> > > enumerate all image formats. The enumerated formats may depend on the active
> > > input or output of the device.
> > > 
> > > If the driver advertises the ``V4L2_CAP_IO_MC`` :ref:`capability
> > > <device-capabilities>`, applications may initialize the ``mbus_code`` field to
> > > a valid :ref:`media bus format code <v4l2-mbus-pixelcode>`. If the
> > > ``mbus_code`` field is not zero, drivers shall restrict enumeration to only the
> > > image formats that can produce (for video output devices) or be produced from
> > > (for video capture devices) that media bus code.  Regardless of the value of
> > > the ``mbus_code`` field, the enumerated image formats shall not depend on the
> > > active configuration of the video device or device pipeline. Enumeration shall
> > > otherwise operate as previously described.
> > 
> > Hmm... it took me re-reading this text 4 or 5 times, in order to understand
> > that you're actually meaning bridge-centric devices here :-)
> > 
> > Also, you placed two independent and unrelated information at the same
> > paragraph.
> > 
> > IMHO, you should let it more clear, like, for example adding a numerated
> > list, like:
> > 
> > 1. Bridge-centric devices
> > 
> >    As such devices don't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
> >    <device-capabilities>`, applications shall initialize the ``mbus_code`` field
> >    to zero and drivers shall ignore the value of the field.
> 
> I could settle for
> 
>    These devices don't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
>    <device-capabilities>`. Applications shall initialize the ``mbus_code`` field
>    to zero and drivers shall ignore the value of the field.
> 
> and similarly below. It bothers me though, as "bridge" isn't formally
> defined anywhere in the userspace API documentation. It's more formal to
> explain the behaviour of individual ioctls based solely on the
> V4L2_CAP_IO_MC flag, and gather all the explanation of what
> bridge-centric vs. MC-centric means in a single place, an introductory

How about "video node centric"? That's what I recall has been used
previously to differentiate regular V4L2 uAPI drivers from MC-centric
drivers. Bridge refers to hardware whereas MC-centric is software, just as
video node centric would be.

> section, instead of spreading it through documentation of individual
> ioctls. V4L2 has more UAPI documentation than most other kernel APIs,
> but there are lots of places where details are not very clear.
> Standardizing ioctl documentation on the use of common vocabulary
> ("may", "shall", ...) and using clearly defined concepts (e.g.
> V4L2_CAP_IO_MC) instead of losely defined ones (e.g. Bridge-centric
> devices) that are explained in non-normative sections would increase
> clarity. I thus prefer the wording in v8.1 of this patch, or possibly
> with the small extension I've proposed in my previous e-mail.
> 
> Hans, Sakari, what do you think ?

My preference is with v8.1 wording, with bridge-centric replaced by video
node centric. This is because it differentiates between the flag what
actually defines device behaviour. That's what applications see, they don't
necessarily know what kind of devices they're working with when they open
the device node.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v8.1 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Hans Verkuil]

On 29/04/2020 22:08, Sakari Ailus wrote:
> Hi Laurent, Mauro,
> 
> On Wed, Apr 29, 2020 at 09:50:33PM +0300, Laurent Pinchart wrote:
>> Hi Mauro,
>>
>> On Wed, Apr 29, 2020 at 08:01:40PM +0200, Mauro Carvalho Chehab wrote:
>>> Em Wed, 29 Apr 2020 19:38:49 +0300 Laurent Pinchart escreveu:
>>>> On Wed, Apr 29, 2020 at 06:27:39PM +0200, Mauro Carvalho Chehab wrote:
>>>>> Em Fri, 24 Apr 2020 16:43:31 +0300 Laurent Pinchart escreveu:
>>>>>   
>>>>>> The VIDIOC_ENUM_FMT ioctl enumerates all formats supported by a video
>>>>>> node. For MC-centric devices, its behaviour has always been ill-defined,
>>>>>> with drivers implementing one of the following behaviours:
>>>>>
>>>>> ...
>>>>>
>>>>> The patch itself is OK. However, there's a change you did at the
>>>>> documentation that it is unrelated. 
>>>>>
>>>>> See below.
>>>>>   
>>>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
>>>>>> index 8ca6ab701e4a..a987debc7654 100644
>>>>>> --- a/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
>>>>>> +++ b/Documentation/media/uapi/v4l/vidioc-enum-fmt.rst
>>>>>> @@ -48,10 +48,21 @@ one until ``EINVAL`` is returned. If applicable, drivers shall return
>>>>>>  formats in preference order, where preferred formats are returned before
>>>>>>  (that is, with lower ``index`` value) less-preferred formats.
>>>>>>  
>>>>>> -.. note::
>>>>>> -   After switching input or output the list of enumerated image
>>>>>> -   formats may be different.  
>>>>>
>>>>> Why are you dropping this note?
>>>>>
>>>>> This basically reverts this changeset:
>>>>>
>>>>>   commit 93828d6438081649e81b8681df9bf6ad5d691650
>>>>>   Author: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>   Date:   Mon Sep 3 09:37:18 2012 -0300
>>>>>
>>>>>     [media] DocBook: make the G/S/TRY_FMT specification more strict
>>>>>     
>>>>>     - S/TRY_FMT should always succeed, unless an invalid type field is passed in.
>>>>>     - TRY_FMT should give the same result as S_FMT, all other things being equal.
>>>>>     - ENUMFMT may return different formats for different inputs or outputs.
>>>>>     This was decided during the 2012 Media Workshop.
>>>>>     
>>>>>     Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>     Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>>>     Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>>     Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
>>>>>     Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>>>>
>>>>> As far as I remember, from our 2012 discussions, some drivers may change 
>>>>> the enumerated image formats when some ioctls like VIDIOC_S_INPUT and
>>>>> VIDIOC_S_OUTPUT ioctls are used. I also vaguely remember that 90 and 270
>>>>> degrees rotation would equally affect it.
>>>>>
>>>>> Perhaps, the removal was just some mistake. If so, please re-submit
>>>>> another patch without it.
>>>>>
>>>>> Otherwise, if are there any good reasons for such change, and it won't
>>>>> cause any API regressions, please place it on a separate patch, clearly
>>>>> that.
>>>>>
>>>>> ... Or, maybe you felt that it won't make sense for MC-centric devices.
>>>>> On such case, please improve the note stating it on a way that it would
>>>>> be understandable on both MC-centric and bridge-centrid drivers.  
>>>>
>>>> I'm not dropping the requirement, I'm rewriting it :-) The patch indeed
>>>> removes the above, but adds the following
>>>>
>>>> ----
>>>> If the driver doesn't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
>>>> <device-capabilities>`, applications shall initialize the ``mbus_code`` field
>>>> to zero and drivers shall ignore the value of the field.  Drivers shall
>>>> enumerate all image formats. The enumerated formats may depend on the active
>>>> input or output of the device.
>>>>
>>>> If the driver advertises the ``V4L2_CAP_IO_MC`` :ref:`capability
>>>> <device-capabilities>`, applications may initialize the ``mbus_code`` field to
>>>> a valid :ref:`media bus format code <v4l2-mbus-pixelcode>`. If the
>>>> ``mbus_code`` field is not zero, drivers shall restrict enumeration to only the
>>>> image formats that can produce (for video output devices) or be produced from
>>>> (for video capture devices) that media bus code.  Regardless of the value of
>>>> the ``mbus_code`` field, the enumerated image formats shall not depend on the
>>>> active configuration of the video device or device pipeline. Enumeration shall
>>>> otherwise operate as previously described.
>>>
>>> Hmm... it took me re-reading this text 4 or 5 times, in order to understand
>>> that you're actually meaning bridge-centric devices here :-)
>>>
>>> Also, you placed two independent and unrelated information at the same
>>> paragraph.
>>>
>>> IMHO, you should let it more clear, like, for example adding a numerated
>>> list, like:
>>>
>>> 1. Bridge-centric devices
>>>
>>>    As such devices don't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
>>>    <device-capabilities>`, applications shall initialize the ``mbus_code`` field
>>>    to zero and drivers shall ignore the value of the field.
>>
>> I could settle for
>>
>>    These devices don't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
>>    <device-capabilities>`. Applications shall initialize the ``mbus_code`` field
>>    to zero and drivers shall ignore the value of the field.
>>
>> and similarly below. It bothers me though, as "bridge" isn't formally
>> defined anywhere in the userspace API documentation. It's more formal to
>> explain the behaviour of individual ioctls based solely on the
>> V4L2_CAP_IO_MC flag, and gather all the explanation of what
>> bridge-centric vs. MC-centric means in a single place, an introductory
> 
> How about "video node centric"? That's what I recall has been used
> previously to differentiate regular V4L2 uAPI drivers from MC-centric
> drivers. Bridge refers to hardware whereas MC-centric is software, just as
> video node centric would be.

I like that. Video node centric vs MC-centric. Although I think we had
this discussion before. We never really managed to come up with
satisfying names for this.

> 
>> section, instead of spreading it through documentation of individual
>> ioctls. V4L2 has more UAPI documentation than most other kernel APIs,
>> but there are lots of places where details are not very clear.
>> Standardizing ioctl documentation on the use of common vocabulary
>> ("may", "shall", ...) and using clearly defined concepts (e.g.
>> V4L2_CAP_IO_MC) instead of losely defined ones (e.g. Bridge-centric
>> devices) that are explained in non-normative sections would increase
>> clarity. I thus prefer the wording in v8.1 of this patch, or possibly
>> with the small extension I've proposed in my previous e-mail.
>>
>> Hans, Sakari, what do you think ?
> 
> My preference is with v8.1 wording, with bridge-centric replaced by video
> node centric. This is because it differentiates between the flag what
> actually defines device behaviour. That's what applications see, they don't
> necessarily know what kind of devices they're working with when they open
> the device node.
> 

You can easily say: 'If the driver doesn't advertise the ``V4L2_CAP_IO_MC``
capability (also known as a 'video node centric' driver)'. That way you
associate the absence of the capability with the type of driver.

I would really like to keep the old text of the note, so replace:

"The enumerated formats may depend on the active input or output of the device."

with:

"After switching to another input or output the list of enumerated formats
 may be different."

I know it says the same, but the original text clearly indicates that it is
the act of switching inputs/outputs that can cause a change of formats.

Just referring to the "active" input/output leaves it to the reader to infer
that the list can change when you select a new active input/output. It's better
(less work for the reader) to be explicit about that.

Regards,

	Hans

Re: [PATCH v8.1 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Mauro Carvalho Chehab]

Em Wed, 29 Apr 2020 23:08:08 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> > I could settle for
> > 
> >    These devices don't advertise the ``V4L2_CAP_IO_MC`` :ref:`capability
> >    <device-capabilities>`. Applications shall initialize the ``mbus_code`` field
> >    to zero and drivers shall ignore the value of the field.
> > 
> > and similarly below. It bothers me though, as "bridge" isn't formally
> > defined anywhere in the userspace API documentation. It's more formal to
> > explain the behaviour of individual ioctls based solely on the
> > V4L2_CAP_IO_MC flag, and gather all the explanation of what
> > bridge-centric vs. MC-centric means in a single place, an introductory  
> 
> How about "video node centric"? 

Works for me.

Thanks,
Mauro

Re: [PATCH v8.1 3/6] media: v4l2: Extend VIDIOC_ENUM_FMT to support MC-centric devices

[Mauro Carvalho Chehab]

Em Tue, 5 May 2020 16:27:25 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> "The enumerated formats may depend on the active input or output of the device."
> 
> with:
> 
> "After switching to another input or output the list of enumerated formats
>  may be different."
>

This change sounds OK to me as well. It should be clear that this applies
to video node centric devices, though.

Thanks,
Mauro