linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 + other meta fixes
@ 2025-06-17 14:42 Ricardo Ribalda
  2025-06-17 14:42 ` [PATCH v7 1/5] media: uvcvideo: Do not mark valid metadata as invalid Ricardo Ribalda
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Ricardo Ribalda @ 2025-06-17 14:42 UTC (permalink / raw)
  To: Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
	Greg Kroah-Hartman, Hans de Goede, Hans de Goede
  Cc: linux-media, linux-kernel, linux-usb, Ricardo Ribalda, stable

This series introduces a new metadata format for UVC cameras and adds a
couple of improvements to the UVC metadata handling.

The new metadata format can be enabled in runtime with quirks.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
Changes in v7:
- Add patch: Introduce dev->meta_formats
- Link to v6: https://lore.kernel.org/r/20250604-uvc-meta-v6-0-7141d48c322c@chromium.org

Changes in v6 (Thanks Laurent):
- Fix typo in metafmt-uvc.rst
- Improve metafmt-uvc-msxu-1-5.rst
- uvc_meta_v4l2_try_format() block MSXU format unless the quirk is
  active
- Refactor uvc_enable_msxu
- Document uvc_meta_detect_msxu
- Rebase series
- Add R-b
- Link to v5: https://lore.kernel.org/r/20250404-uvc-meta-v5-0-f79974fc2d20@chromium.org

Changes in v5:
- Fix codestyle and kerneldoc warnings reported by media-ci
- Link to v4: https://lore.kernel.org/r/20250403-uvc-meta-v4-0-877aa6475975@chromium.org

Changes in v4:
- Rename format to V4L2_META_FMT_UVC_MSXU_1_5 (Thanks Mauro)
- Flag the new format with a quirk.
- Autodetect MSXU devices.
- Link to v3: https://lore.kernel.org/linux-media/20250313-uvc-metadata-v3-0-c467af869c60@chromium.org/

Changes in v3:
- Fix doc syntax errors.
- Link to v2: https://lore.kernel.org/r/20250306-uvc-metadata-v2-0-7e939857cad5@chromium.org

Changes in v2:
- Add metadata invalid fix
- Move doc note to a separate patch
- Introduce V4L2_META_FMT_UVC_CUSTOM (thanks HdG!).
- Link to v1: https://lore.kernel.org/r/20250226-uvc-metadata-v1-1-6cd6fe5ec2cb@chromium.org

---
Ricardo Ribalda (5):
      media: uvcvideo: Do not mark valid metadata as invalid
      media: Documentation: Add note about UVCH length field
      media: uvcvideo: Introduce dev->meta_formats
      media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5
      media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META

 .../userspace-api/media/v4l/meta-formats.rst       |   1 +
 .../media/v4l/metafmt-uvc-msxu-1-5.rst             |  23 ++++
 .../userspace-api/media/v4l/metafmt-uvc.rst        |   4 +-
 MAINTAINERS                                        |   1 +
 drivers/media/usb/uvc/uvc_driver.c                 |   7 ++
 drivers/media/usb/uvc/uvc_metadata.c               | 133 +++++++++++++++++++--
 drivers/media/usb/uvc/uvc_video.c                  |  12 +-
 drivers/media/usb/uvc/uvcvideo.h                   |   3 +
 drivers/media/v4l2-core/v4l2-ioctl.c               |   1 +
 include/linux/usb/uvc.h                            |   3 +
 include/uapi/linux/videodev2.h                     |   1 +
 11 files changed, 175 insertions(+), 14 deletions(-)
---
base-commit: c3021d6a80ff05034dfee494115ec71f1954e311
change-id: 20250403-uvc-meta-e556773d12ae

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>


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

* [PATCH v7 1/5] media: uvcvideo: Do not mark valid metadata as invalid
  2025-06-17 14:42 [PATCH v7 0/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 + other meta fixes Ricardo Ribalda
@ 2025-06-17 14:42 ` Ricardo Ribalda
  2025-06-17 14:42 ` [PATCH v7 2/5] media: Documentation: Add note about UVCH length field Ricardo Ribalda
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Ricardo Ribalda @ 2025-06-17 14:42 UTC (permalink / raw)
  To: Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
	Greg Kroah-Hartman, Hans de Goede, Hans de Goede
  Cc: linux-media, linux-kernel, linux-usb, Ricardo Ribalda, stable

Currently, the driver performs a length check of the metadata buffer
before the actual metadata size is known and before the metadata is
decided to be copied. This results in valid metadata buffers being
incorrectly marked as invalid.

Move the length check to occur after the metadata size is determined and
is decided to be copied.

Cc: stable@vger.kernel.org
Fixes: 088ead255245 ("media: uvcvideo: Add a metadata device node")
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_video.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 11769a1832d2ba9b3f9a50bcb10b0c4cdff71f09..2e377e7b9e81599aca19b800a171cc16a09c1e8a 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1442,12 +1442,6 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream,
 	if (!meta_buf || length == 2)
 		return;
 
-	if (meta_buf->length - meta_buf->bytesused <
-	    length + sizeof(meta->ns) + sizeof(meta->sof)) {
-		meta_buf->error = 1;
-		return;
-	}
-
 	has_pts = mem[1] & UVC_STREAM_PTS;
 	has_scr = mem[1] & UVC_STREAM_SCR;
 
@@ -1468,6 +1462,12 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream,
 				  !memcmp(scr, stream->clock.last_scr, 6)))
 		return;
 
+	if (meta_buf->length - meta_buf->bytesused <
+	    length + sizeof(meta->ns) + sizeof(meta->sof)) {
+		meta_buf->error = 1;
+		return;
+	}
+
 	meta = (struct uvc_meta_buf *)((u8 *)meta_buf->mem + meta_buf->bytesused);
 	local_irq_save(flags);
 	time = uvc_video_get_time();

-- 
2.50.0.rc2.692.g299adb8693-goog


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

* [PATCH v7 2/5] media: Documentation: Add note about UVCH length field
  2025-06-17 14:42 [PATCH v7 0/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 + other meta fixes Ricardo Ribalda
  2025-06-17 14:42 ` [PATCH v7 1/5] media: uvcvideo: Do not mark valid metadata as invalid Ricardo Ribalda
@ 2025-06-17 14:42 ` Ricardo Ribalda
  2025-06-17 14:42 ` [PATCH v7 3/5] media: uvcvideo: Introduce dev->meta_formats Ricardo Ribalda
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Ricardo Ribalda @ 2025-06-17 14:42 UTC (permalink / raw)
  To: Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
	Greg Kroah-Hartman, Hans de Goede, Hans de Goede
  Cc: linux-media, linux-kernel, linux-usb, Ricardo Ribalda

The documentation currently describes the UVC length field as the "length
of the rest of the block", which can be misleading. The driver limits the
data copied to a maximum of 12 bytes.

This change adds a clarifying sentence to the documentation to make this
restriction explicit.

Reviewed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 Documentation/userspace-api/media/v4l/metafmt-uvc.rst | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/media/v4l/metafmt-uvc.rst b/Documentation/userspace-api/media/v4l/metafmt-uvc.rst
index 784346d14bbdbf28348262084d5b0646d30bd1da..4c05e9e54683a2bf844ddf26f99d0d9713ef05de 100644
--- a/Documentation/userspace-api/media/v4l/metafmt-uvc.rst
+++ b/Documentation/userspace-api/media/v4l/metafmt-uvc.rst
@@ -44,7 +44,9 @@ Each individual block contains the following fields:
         them
     * - :cspan:`1` *The rest is an exact copy of the UVC payload header:*
     * - __u8 length;
-      - length of the rest of the block, including this field
+      - length of the rest of the block, including this field. Please note that
+        regardless of this value, for V4L2_META_FMT_UVC the kernel will never
+        copy more than 2-12 bytes.
     * - __u8 flags;
       - Flags, indicating presence of other standard UVC fields
     * - __u8 buf[];

-- 
2.50.0.rc2.692.g299adb8693-goog


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

* [PATCH v7 3/5] media: uvcvideo: Introduce dev->meta_formats
  2025-06-17 14:42 [PATCH v7 0/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 + other meta fixes Ricardo Ribalda
  2025-06-17 14:42 ` [PATCH v7 1/5] media: uvcvideo: Do not mark valid metadata as invalid Ricardo Ribalda
  2025-06-17 14:42 ` [PATCH v7 2/5] media: Documentation: Add note about UVCH length field Ricardo Ribalda
@ 2025-06-17 14:42 ` Ricardo Ribalda
  2025-07-07 11:35   ` Hans de Goede
  2025-06-17 14:42 ` [PATCH v7 4/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 Ricardo Ribalda
  2025-06-17 14:42 ` [PATCH v7 5/5] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META Ricardo Ribalda
  4 siblings, 1 reply; 10+ messages in thread
From: Ricardo Ribalda @ 2025-06-17 14:42 UTC (permalink / raw)
  To: Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
	Greg Kroah-Hartman, Hans de Goede, Hans de Goede
  Cc: linux-media, linux-kernel, linux-usb, Ricardo Ribalda

Right now, there driver supports devices with one or two metadata
formats. Prepare it to support more than two metadata formats.

This is achieved with the introduction of a new field `meta_formats`,
that contains the array of metadata formats supported by the device, in
the order expected by userspace.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c   |  7 ++++++
 drivers/media/usb/uvc/uvc_metadata.c | 46 ++++++++++++++++++++++++++++++------
 drivers/media/usb/uvc/uvcvideo.h     |  2 ++
 3 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 62eb45435d8bec5c955720ecb46fb8936871e6cc..9de5abb43e19d9e876cddc5d7124592953db89ac 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -2315,6 +2315,13 @@ static int uvc_probe(struct usb_interface *intf,
 		goto error;
 	}
 
+	ret = uvc_meta_init(dev);
+	if (ret < 0) {
+		dev_err(&dev->udev->dev,
+			"Error initializing the metadata formats (%d)\n", ret);
+		goto error;
+	}
+
 	if (dev->quirks & UVC_QUIRK_NO_RESET_RESUME)
 		udev->quirks &= ~USB_QUIRK_RESET_RESUME;
 
diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
index 82de7781f5b6b70c5ba16bcba9e0741231231904..bc84e849174397f41d1e20bf890a876eeb5a9c67 100644
--- a/drivers/media/usb/uvc/uvc_metadata.c
+++ b/drivers/media/usb/uvc/uvc_metadata.c
@@ -64,14 +64,20 @@ static int uvc_meta_v4l2_try_format(struct file *file, void *fh,
 	struct uvc_device *dev = stream->dev;
 	struct v4l2_meta_format *fmt = &format->fmt.meta;
 	u32 fmeta = fmt->dataformat;
+	u32 i;
 
 	if (format->type != vfh->vdev->queue->type)
 		return -EINVAL;
 
+	for (i = 0; (fmeta != dev->meta_formats[i]) && dev->meta_formats[i];
+	     i++)
+		;
+	if (!dev->meta_formats[i])
+		fmeta = V4L2_META_FMT_UVC;
+
 	memset(fmt, 0, sizeof(*fmt));
 
-	fmt->dataformat = fmeta == dev->info->meta_format
-			? fmeta : V4L2_META_FMT_UVC;
+	fmt->dataformat = fmeta;
 	fmt->buffersize = UVC_METADATA_BUF_SIZE;
 
 	return 0;
@@ -112,17 +118,21 @@ static int uvc_meta_v4l2_enum_formats(struct file *file, void *fh,
 	struct v4l2_fh *vfh = file->private_data;
 	struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
 	struct uvc_device *dev = stream->dev;
-	u32 index = fdesc->index;
+	u32 i;
+
+	if (fdesc->type != vfh->vdev->queue->type)
+		return -EINVAL;
 
-	if (fdesc->type != vfh->vdev->queue->type ||
-	    index > 1U || (index && !dev->info->meta_format))
+	for (i = 0; (i < fdesc->index) && dev->meta_formats[i]; i++)
+		;
+	if (!dev->meta_formats[i])
 		return -EINVAL;
 
 	memset(fdesc, 0, sizeof(*fdesc));
 
 	fdesc->type = vfh->vdev->queue->type;
-	fdesc->index = index;
-	fdesc->pixelformat = index ? dev->info->meta_format : V4L2_META_FMT_UVC;
+	fdesc->index = i;
+	fdesc->pixelformat = dev->meta_formats[i];
 
 	return 0;
 }
@@ -174,3 +184,25 @@ int uvc_meta_register(struct uvc_streaming *stream)
 					 V4L2_BUF_TYPE_META_CAPTURE,
 					 &uvc_meta_fops, &uvc_meta_ioctl_ops);
 }
+
+int uvc_meta_init(struct uvc_device *dev)
+{
+	static const u32 uvch_only[] = {V4L2_META_FMT_UVC, 0};
+	static const u32 d4xx_format[] = {V4L2_META_FMT_UVC, V4L2_META_FMT_D4XX,
+					  0};
+
+	switch (dev->info->meta_format) {
+	case V4L2_META_FMT_D4XX:
+		dev->meta_formats = d4xx_format;
+		break;
+	case 0:
+		dev->meta_formats = uvch_only;
+		break;
+	default:
+		dev_err(&dev->udev->dev, "Unknown metadata format 0x%x\n",
+			dev->info->meta_format);
+		return -EINVAL;
+	}
+
+	return 0;
+}
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 11d6e3c2ebdfbabd7bbe5722f88ff85f406d9bb6..502f1d5608637cd28ce6f01aee31c4f5df160081 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -581,6 +581,7 @@ struct uvc_device {
 	char name[32];
 
 	const struct uvc_device_info *info;
+	const u32 *meta_formats; /* Zero-ended list of meta formats */
 
 	atomic_t nmappings;
 
@@ -751,6 +752,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 void uvc_video_clock_update(struct uvc_streaming *stream,
 			    struct vb2_v4l2_buffer *vbuf,
 			    struct uvc_buffer *buf);
+int uvc_meta_init(struct uvc_device *dev);
 int uvc_meta_register(struct uvc_streaming *stream);
 
 int uvc_register_video_device(struct uvc_device *dev,

-- 
2.50.0.rc2.692.g299adb8693-goog


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

* [PATCH v7 4/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5
  2025-06-17 14:42 [PATCH v7 0/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 + other meta fixes Ricardo Ribalda
                   ` (2 preceding siblings ...)
  2025-06-17 14:42 ` [PATCH v7 3/5] media: uvcvideo: Introduce dev->meta_formats Ricardo Ribalda
@ 2025-06-17 14:42 ` Ricardo Ribalda
  2025-07-07 11:34   ` Hans de Goede
  2025-06-17 14:42 ` [PATCH v7 5/5] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META Ricardo Ribalda
  4 siblings, 1 reply; 10+ messages in thread
From: Ricardo Ribalda @ 2025-06-17 14:42 UTC (permalink / raw)
  To: Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
	Greg Kroah-Hartman, Hans de Goede, Hans de Goede
  Cc: linux-media, linux-kernel, linux-usb, Ricardo Ribalda

The UVC driver provides two metadata types V4L2_META_FMT_UVC, and
V4L2_META_FMT_D4XX. The only difference between the two of them is that
V4L2_META_FMT_UVC only copies PTS, SCR, size and flags, and
V4L2_META_FMT_D4XX copies the whole metadata section.

Now we only enable V4L2_META_FMT_D4XX for the Intel D4xx family of
devices, but it is useful to have the whole metadata payload for any
device where vendors include other metadata, such as the one described by
Microsoft:
https://learn.microsoft.com/en-us/windows-hardware/drivers/stream/mf-capture-metadata

This patch introduces a new format V4L2_META_FMT_UVC_MSXU_1_5, that is
identical to V4L2_META_FMT_D4XX.

Let the user enable this format with a quirk for now. This way they can
test if their devices provide useful metadata without rebuilding the
kernel. They can later contribute patches to auto-quirk their devices.
We will also work in methods to auto-detect devices compatible with this
new metadata format.

Suggested-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 .../userspace-api/media/v4l/meta-formats.rst       |  1 +
 .../media/v4l/metafmt-uvc-msxu-1-5.rst             | 23 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 drivers/media/usb/uvc/uvc_metadata.c               | 20 +++++++++++++++++--
 drivers/media/usb/uvc/uvcvideo.h                   |  1 +
 drivers/media/v4l2-core/v4l2-ioctl.c               |  1 +
 include/uapi/linux/videodev2.h                     |  1 +
 7 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/Documentation/userspace-api/media/v4l/meta-formats.rst b/Documentation/userspace-api/media/v4l/meta-formats.rst
index bb6876cfc271e1a0543eee4209d6251e1a6a73cc..0de80328c36bf148051a19abe9e5241234ddfe5c 100644
--- a/Documentation/userspace-api/media/v4l/meta-formats.rst
+++ b/Documentation/userspace-api/media/v4l/meta-formats.rst
@@ -20,6 +20,7 @@ These formats are used for the :ref:`metadata` interface only.
     metafmt-pisp-fe
     metafmt-rkisp1
     metafmt-uvc
+    metafmt-uvc-msxu-1-5
     metafmt-vivid
     metafmt-vsp1-hgo
     metafmt-vsp1-hgt
diff --git a/Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst b/Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst
new file mode 100644
index 0000000000000000000000000000000000000000..dd1c3076df243d770a13e7f6d07c3296a269e16a
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst
@@ -0,0 +1,23 @@
+.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
+
+.. _v4l2-meta-fmt-uvc-msxu-1-5:
+
+***********************************
+V4L2_META_FMT_UVC_MSXU_1_5 ('UVCM')
+***********************************
+
+Microsoft(R)'s UVC Payload Metadata.
+
+
+Description
+===========
+
+V4L2_META_FMT_UVC_MSXU_1_5 buffers follow the metadata buffer layout of
+V4L2_META_FMT_UVC with the only difference that it includes all the UVC
+metadata in the `buffer[]` field, not just the first 2-12 bytes.
+
+The metadata format follows the specification from Microsoft(R) [1].
+
+.. _1:
+
+[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extensions-1-5
diff --git a/MAINTAINERS b/MAINTAINERS
index e8f3dc93a56921924f57e7d5a03ea2fa182a4448..87101630e528297c57b22ffc2fe553e3864d25cc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25816,6 +25816,7 @@ S:	Maintained
 W:	http://www.ideasonboard.org/uvc/
 T:	git git://linuxtv.org/media.git
 F:	Documentation/userspace-api/media/drivers/uvcvideo.rst
+F:	Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst
 F:	Documentation/userspace-api/media/v4l/metafmt-uvc.rst
 F:	drivers/media/common/uvc.c
 F:	drivers/media/usb/uvc/
diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
index bc84e849174397f41d1e20bf890a876eeb5a9c67..b09f81d907d64f7d7a3b0dc52de319879b7e68be 100644
--- a/drivers/media/usb/uvc/uvc_metadata.c
+++ b/drivers/media/usb/uvc/uvc_metadata.c
@@ -190,13 +190,29 @@ int uvc_meta_init(struct uvc_device *dev)
 	static const u32 uvch_only[] = {V4L2_META_FMT_UVC, 0};
 	static const u32 d4xx_format[] = {V4L2_META_FMT_UVC, V4L2_META_FMT_D4XX,
 					  0};
+	static const u32 all_formats[] = {V4L2_META_FMT_UVC, V4L2_META_FMT_D4XX,
+					  V4L2_META_FMT_UVC_MSXU_1_5, 0};
+	static const u32 ms_format[] = {V4L2_META_FMT_UVC,
+					V4L2_META_FMT_UVC_MSXU_1_5, 0};
+	bool support_msxu;
+
+	support_msxu = dev->quirks & UVC_QUIRK_MSXU_META;
 
 	switch (dev->info->meta_format) {
+	case V4L2_META_FMT_UVC_MSXU_1_5:
+		dev->meta_formats = ms_format;
+		break;
 	case V4L2_META_FMT_D4XX:
-		dev->meta_formats = d4xx_format;
+		if (support_msxu)
+			dev->meta_formats = all_formats;
+		else
+			dev->meta_formats = d4xx_format;
 		break;
 	case 0:
-		dev->meta_formats = uvch_only;
+		if (support_msxu)
+			dev->meta_formats = ms_format;
+		else
+			dev->meta_formats = uvch_only;
 		break;
 	default:
 		dev_err(&dev->udev->dev, "Unknown metadata format 0x%x\n",
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 502f1d5608637cd28ce6f01aee31c4f5df160081..3578ce72fb6a1153ae79c244ec10093e8efdd739 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -77,6 +77,7 @@
 #define UVC_QUIRK_DISABLE_AUTOSUSPEND	0x00008000
 #define UVC_QUIRK_INVALID_DEVICE_SOF	0x00010000
 #define UVC_QUIRK_MJPEG_NO_EOF		0x00020000
+#define UVC_QUIRK_MSXU_META		0x00040000
 
 /* Format flags */
 #define UVC_FMT_FLAG_COMPRESSED		0x00000001
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 650dc1956f73d2f1943b56c42140c7b8d757259f..ba508f7fb577021497009ab23a7be5add23fd08c 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1459,6 +1459,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
 	case V4L2_META_FMT_VSP1_HGO:	descr = "R-Car VSP1 1-D Histogram"; break;
 	case V4L2_META_FMT_VSP1_HGT:	descr = "R-Car VSP1 2-D Histogram"; break;
 	case V4L2_META_FMT_UVC:		descr = "UVC Payload Header Metadata"; break;
+	case V4L2_META_FMT_UVC_MSXU_1_5:	descr = "UVC MSXU Metadata"; break;
 	case V4L2_META_FMT_D4XX:	descr = "Intel D4xx UVC Metadata"; break;
 	case V4L2_META_FMT_VIVID:       descr = "Vivid Metadata"; break;
 	case V4L2_META_FMT_RK_ISP1_PARAMS:	descr = "Rockchip ISP1 3A Parameters"; break;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 9e3b366d5fc79d8a04c6f0752858fc23363db65c..75f2096b2d4fed5e0235ea4732d35044ff77a98b 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -861,6 +861,7 @@ struct v4l2_pix_format {
 #define V4L2_META_FMT_VSP1_HGT    v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */
 #define V4L2_META_FMT_UVC         v4l2_fourcc('U', 'V', 'C', 'H') /* UVC Payload Header metadata */
 #define V4L2_META_FMT_D4XX        v4l2_fourcc('D', '4', 'X', 'X') /* D4XX Payload Header metadata */
+#define V4L2_META_FMT_UVC_MSXU_1_5  v4l2_fourcc('U', 'V', 'C', 'M') /* UVC MSXU metadata */
 #define V4L2_META_FMT_VIVID	  v4l2_fourcc('V', 'I', 'V', 'D') /* Vivid Metadata */
 
 /* Vendor specific - used for RK_ISP1 camera sub-system */

-- 
2.50.0.rc2.692.g299adb8693-goog


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

* [PATCH v7 5/5] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META
  2025-06-17 14:42 [PATCH v7 0/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 + other meta fixes Ricardo Ribalda
                   ` (3 preceding siblings ...)
  2025-06-17 14:42 ` [PATCH v7 4/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 Ricardo Ribalda
@ 2025-06-17 14:42 ` Ricardo Ribalda
  4 siblings, 0 replies; 10+ messages in thread
From: Ricardo Ribalda @ 2025-06-17 14:42 UTC (permalink / raw)
  To: Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
	Greg Kroah-Hartman, Hans de Goede, Hans de Goede
  Cc: linux-media, linux-kernel, linux-usb, Ricardo Ribalda

If the camera supports the MSXU_CONTROL_METADATA control, auto set the
MSXU_META quirk.

Reviewed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_metadata.c | 71 ++++++++++++++++++++++++++++++++++++
 include/linux/usb/uvc.h              |  3 ++
 2 files changed, 74 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
index b09f81d907d64f7d7a3b0dc52de319879b7e68be..5c3e0f8c5b720a2b962ae82470b836f870dc64b6 100644
--- a/drivers/media/usb/uvc/uvc_metadata.c
+++ b/drivers/media/usb/uvc/uvc_metadata.c
@@ -10,6 +10,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/usb.h>
+#include <linux/usb/uvc.h>
 #include <linux/videodev2.h>
 
 #include <media/v4l2-ioctl.h>
@@ -166,6 +167,71 @@ static const struct v4l2_file_operations uvc_meta_fops = {
 	.mmap = vb2_fop_mmap,
 };
 
+static struct uvc_entity *uvc_meta_find_msxu(struct uvc_device *dev)
+{
+	static const u8 uvc_msxu_guid[16] = UVC_GUID_MSXU_1_5;
+	struct uvc_entity *entity;
+
+	list_for_each_entry(entity, &dev->entities, list) {
+		if (!memcmp(entity->guid, uvc_msxu_guid, sizeof(entity->guid)))
+			return entity;
+	}
+
+	return NULL;
+}
+
+#define MSXU_CONTROL_METADATA 0x9
+static int uvc_meta_detect_msxu(struct uvc_device *dev)
+{
+	u32 *data __free(kfree) = NULL;
+	struct uvc_entity *entity;
+	int ret;
+
+	entity = uvc_meta_find_msxu(dev);
+	if (!entity)
+		return 0;
+
+	/*
+	 * USB requires buffers aligned in a special way, simplest way is to
+	 * make sure that query_ctrl will work is to kmalloc() them.
+	 */
+	data = kmalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	/* Check if the metadata is already enabled. */
+	ret = uvc_query_ctrl(dev, UVC_GET_CUR, entity->id, dev->intfnum,
+			     MSXU_CONTROL_METADATA, data, sizeof(*data));
+	if (ret)
+		return 0;
+
+	if (*data) {
+		dev->quirks |= UVC_QUIRK_MSXU_META;
+		return 0;
+	}
+
+	/*
+	 * We have seen devices that require 1 to enable the metadata, others
+	 * requiring a value != 1 and others requiring a value >1. Luckily for
+	 * us, the value from GET_MAX seems to work all the time.
+	 */
+	ret = uvc_query_ctrl(dev, UVC_GET_MAX, entity->id, dev->intfnum,
+			     MSXU_CONTROL_METADATA, data, sizeof(*data));
+	if (ret || !*data)
+		return 0;
+
+	/*
+	 * If we can set MSXU_CONTROL_METADATA, the device will report
+	 * metadata.
+	 */
+	ret = uvc_query_ctrl(dev, UVC_SET_CUR, entity->id, dev->intfnum,
+			     MSXU_CONTROL_METADATA, data, sizeof(*data));
+	if (!ret)
+		dev->quirks |= UVC_QUIRK_MSXU_META;
+
+	return 0;
+}
+
 int uvc_meta_register(struct uvc_streaming *stream)
 {
 	struct uvc_device *dev = stream->dev;
@@ -195,6 +261,11 @@ int uvc_meta_init(struct uvc_device *dev)
 	static const u32 ms_format[] = {V4L2_META_FMT_UVC,
 					V4L2_META_FMT_UVC_MSXU_1_5, 0};
 	bool support_msxu;
+	int ret;
+
+	ret = uvc_meta_detect_msxu(dev);
+	if (ret)
+		return ret;
 
 	support_msxu = dev->quirks & UVC_QUIRK_MSXU_META;
 
diff --git a/include/linux/usb/uvc.h b/include/linux/usb/uvc.h
index bce95153e5a65613a710d7316fc17cf5462b5bce..ee19e9f915b8370c333c426dc1ee4202c7b75c5b 100644
--- a/include/linux/usb/uvc.h
+++ b/include/linux/usb/uvc.h
@@ -29,6 +29,9 @@
 #define UVC_GUID_EXT_GPIO_CONTROLLER \
 	{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, \
 	 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x03}
+#define UVC_GUID_MSXU_1_5 \
+	{0xdc, 0x95, 0x3f, 0x0f, 0x32, 0x26, 0x4e, 0x4c, \
+	 0x92, 0xc9, 0xa0, 0x47, 0x82, 0xf4, 0x3b, 0xc8}
 
 #define UVC_GUID_FORMAT_MJPEG \
 	{ 'M',  'J',  'P',  'G', 0x00, 0x00, 0x10, 0x00, \

-- 
2.50.0.rc2.692.g299adb8693-goog


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

* Re: [PATCH v7 4/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5
  2025-06-17 14:42 ` [PATCH v7 4/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 Ricardo Ribalda
@ 2025-07-07 11:34   ` Hans de Goede
  2025-07-07 11:44     ` Ricardo Ribalda
  0 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2025-07-07 11:34 UTC (permalink / raw)
  To: Ricardo Ribalda, Laurent Pinchart, Mauro Carvalho Chehab,
	Guennadi Liakhovetski, Greg Kroah-Hartman
  Cc: linux-media, linux-kernel, linux-usb

Hi Ricardo,

Thank you for the new version of this series.

On 17-Jun-25 16:42, Ricardo Ribalda wrote:
> The UVC driver provides two metadata types V4L2_META_FMT_UVC, and
> V4L2_META_FMT_D4XX. The only difference between the two of them is that
> V4L2_META_FMT_UVC only copies PTS, SCR, size and flags, and
> V4L2_META_FMT_D4XX copies the whole metadata section.
> 
> Now we only enable V4L2_META_FMT_D4XX for the Intel D4xx family of
> devices, but it is useful to have the whole metadata payload for any
> device where vendors include other metadata, such as the one described by
> Microsoft:
> https://learn.microsoft.com/en-us/windows-hardware/drivers/stream/mf-capture-metadata
> 
> This patch introduces a new format V4L2_META_FMT_UVC_MSXU_1_5, that is
> identical to V4L2_META_FMT_D4XX.
> 
> Let the user enable this format with a quirk for now. This way they can
> test if their devices provide useful metadata without rebuilding the
> kernel. They can later contribute patches to auto-quirk their devices.
> We will also work in methods to auto-detect devices compatible with this
> new metadata format.
> 
> Suggested-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  .../userspace-api/media/v4l/meta-formats.rst       |  1 +
>  .../media/v4l/metafmt-uvc-msxu-1-5.rst             | 23 ++++++++++++++++++++++
>  MAINTAINERS                                        |  1 +
>  drivers/media/usb/uvc/uvc_metadata.c               | 20 +++++++++++++++++--
>  drivers/media/usb/uvc/uvcvideo.h                   |  1 +
>  drivers/media/v4l2-core/v4l2-ioctl.c               |  1 +
>  include/uapi/linux/videodev2.h                     |  1 +
>  7 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/meta-formats.rst b/Documentation/userspace-api/media/v4l/meta-formats.rst
> index bb6876cfc271e1a0543eee4209d6251e1a6a73cc..0de80328c36bf148051a19abe9e5241234ddfe5c 100644
> --- a/Documentation/userspace-api/media/v4l/meta-formats.rst
> +++ b/Documentation/userspace-api/media/v4l/meta-formats.rst
> @@ -20,6 +20,7 @@ These formats are used for the :ref:`metadata` interface only.
>      metafmt-pisp-fe
>      metafmt-rkisp1
>      metafmt-uvc
> +    metafmt-uvc-msxu-1-5
>      metafmt-vivid
>      metafmt-vsp1-hgo
>      metafmt-vsp1-hgt
> diff --git a/Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst b/Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst
> new file mode 100644
> index 0000000000000000000000000000000000000000..dd1c3076df243d770a13e7f6d07c3296a269e16a
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst
> @@ -0,0 +1,23 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +
> +.. _v4l2-meta-fmt-uvc-msxu-1-5:
> +
> +***********************************
> +V4L2_META_FMT_UVC_MSXU_1_5 ('UVCM')
> +***********************************
> +
> +Microsoft(R)'s UVC Payload Metadata.
> +
> +
> +Description
> +===========
> +
> +V4L2_META_FMT_UVC_MSXU_1_5 buffers follow the metadata buffer layout of
> +V4L2_META_FMT_UVC with the only difference that it includes all the UVC
> +metadata in the `buffer[]` field, not just the first 2-12 bytes.
> +
> +The metadata format follows the specification from Microsoft(R) [1].
> +
> +.. _1:
> +
> +[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extensions-1-5
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e8f3dc93a56921924f57e7d5a03ea2fa182a4448..87101630e528297c57b22ffc2fe553e3864d25cc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25816,6 +25816,7 @@ S:	Maintained
>  W:	http://www.ideasonboard.org/uvc/
>  T:	git git://linuxtv.org/media.git
>  F:	Documentation/userspace-api/media/drivers/uvcvideo.rst
> +F:	Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst
>  F:	Documentation/userspace-api/media/v4l/metafmt-uvc.rst
>  F:	drivers/media/common/uvc.c
>  F:	drivers/media/usb/uvc/
> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
> index bc84e849174397f41d1e20bf890a876eeb5a9c67..b09f81d907d64f7d7a3b0dc52de319879b7e68be 100644
> --- a/drivers/media/usb/uvc/uvc_metadata.c
> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> @@ -190,13 +190,29 @@ int uvc_meta_init(struct uvc_device *dev)
>  	static const u32 uvch_only[] = {V4L2_META_FMT_UVC, 0};
>  	static const u32 d4xx_format[] = {V4L2_META_FMT_UVC, V4L2_META_FMT_D4XX,
>  					  0};
> +	static const u32 all_formats[] = {V4L2_META_FMT_UVC, V4L2_META_FMT_D4XX,
> +					  V4L2_META_FMT_UVC_MSXU_1_5, 0};
> +	static const u32 ms_format[] = {V4L2_META_FMT_UVC,
> +					V4L2_META_FMT_UVC_MSXU_1_5, 0};

Hmm, this does not look great, I guess we are not expecting any
new metadata formats soon but just needing the 4 arrays here and
then ... (continued below).


> +	bool support_msxu;
> +
> +	support_msxu = dev->quirks & UVC_QUIRK_MSXU_META;
>  
>  	switch (dev->info->meta_format) {
> +	case V4L2_META_FMT_UVC_MSXU_1_5:
> +		dev->meta_formats = ms_format;
> +		break;
>  	case V4L2_META_FMT_D4XX:
> -		dev->meta_formats = d4xx_format;
> +		if (support_msxu)
> +			dev->meta_formats = all_formats;
> +		else
> +			dev->meta_formats = d4xx_format;
>  		break;
>  	case 0:
> -		dev->meta_formats = uvch_only;
> +		if (support_msxu)
> +			dev->meta_formats = ms_format;
> +		else
> +			dev->meta_formats = uvch_only;

Also having these if else's here both don't look nice /
this does not feel clean.

My suggestion would be to instead do the following:

1. Add a #define UVC_MAX_META_DATA_FORMATS 3 to uvcvideo.h
2. In the struct uvc_device definition change meta_formats to:

	u32 meta_formats[UVC_MAX_META_DATA_FORMATS + 1];

3. Change uvc_meta_init() to:

void uvc_meta_init(struct uvc_device *dev)
{
	unsigned int i = 0;

	dev->meta_formats[i++] = V4L2_META_FMT_UVC;

	if (dev->info->meta_format)
		dev->meta_formats[i++] = dev->info->meta_format;

	if (dev->quirks & UVC_QUIRK_MSXU_META)
		dev->meta_formats[i++] = V4L2_META_FMT_UVC_MSXU_1_5;

	/* IMPORTANT for new meta-formats update UVC_MAX_META_DATA_FORMATS */

	dev->meta_formats[i++] = 0;
}

Note uvc_meta_init() now also is void, so no more need to error check it.

The only downside I can see is that if we ever actually start setting
dev->info->meta_format = V4L2_META_FMT_UVC_MSXU_1_5 and a user manually
enables the quirk we get V4L2_META_FMT_UVC_MSXU_1_5 listed twice, but
that should not cause any issues and normally that will never happen.

IMHO this is better, then the switch-case + if-else code.

Stating the obvious: some / most of these changes should be done in patch 3/5
already.

Regards,

Hans




>  		break;
>  	default:
>  		dev_err(&dev->udev->dev, "Unknown metadata format 0x%x\n",
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 502f1d5608637cd28ce6f01aee31c4f5df160081..3578ce72fb6a1153ae79c244ec10093e8efdd739 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -77,6 +77,7 @@
>  #define UVC_QUIRK_DISABLE_AUTOSUSPEND	0x00008000
>  #define UVC_QUIRK_INVALID_DEVICE_SOF	0x00010000
>  #define UVC_QUIRK_MJPEG_NO_EOF		0x00020000
> +#define UVC_QUIRK_MSXU_META		0x00040000
>  
>  /* Format flags */
>  #define UVC_FMT_FLAG_COMPRESSED		0x00000001
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 650dc1956f73d2f1943b56c42140c7b8d757259f..ba508f7fb577021497009ab23a7be5add23fd08c 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1459,6 +1459,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>  	case V4L2_META_FMT_VSP1_HGO:	descr = "R-Car VSP1 1-D Histogram"; break;
>  	case V4L2_META_FMT_VSP1_HGT:	descr = "R-Car VSP1 2-D Histogram"; break;
>  	case V4L2_META_FMT_UVC:		descr = "UVC Payload Header Metadata"; break;
> +	case V4L2_META_FMT_UVC_MSXU_1_5:	descr = "UVC MSXU Metadata"; break;
>  	case V4L2_META_FMT_D4XX:	descr = "Intel D4xx UVC Metadata"; break;
>  	case V4L2_META_FMT_VIVID:       descr = "Vivid Metadata"; break;
>  	case V4L2_META_FMT_RK_ISP1_PARAMS:	descr = "Rockchip ISP1 3A Parameters"; break;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 9e3b366d5fc79d8a04c6f0752858fc23363db65c..75f2096b2d4fed5e0235ea4732d35044ff77a98b 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -861,6 +861,7 @@ struct v4l2_pix_format {
>  #define V4L2_META_FMT_VSP1_HGT    v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */
>  #define V4L2_META_FMT_UVC         v4l2_fourcc('U', 'V', 'C', 'H') /* UVC Payload Header metadata */
>  #define V4L2_META_FMT_D4XX        v4l2_fourcc('D', '4', 'X', 'X') /* D4XX Payload Header metadata */
> +#define V4L2_META_FMT_UVC_MSXU_1_5  v4l2_fourcc('U', 'V', 'C', 'M') /* UVC MSXU metadata */
>  #define V4L2_META_FMT_VIVID	  v4l2_fourcc('V', 'I', 'V', 'D') /* Vivid Metadata */
>  
>  /* Vendor specific - used for RK_ISP1 camera sub-system */
> 


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

* Re: [PATCH v7 3/5] media: uvcvideo: Introduce dev->meta_formats
  2025-06-17 14:42 ` [PATCH v7 3/5] media: uvcvideo: Introduce dev->meta_formats Ricardo Ribalda
@ 2025-07-07 11:35   ` Hans de Goede
  0 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2025-07-07 11:35 UTC (permalink / raw)
  To: Ricardo Ribalda, Laurent Pinchart, Mauro Carvalho Chehab,
	Guennadi Liakhovetski, Greg Kroah-Hartman
  Cc: linux-media, linux-kernel, linux-usb

Hi Ricardo,

On 17-Jun-25 16:42, Ricardo Ribalda wrote:
> Right now, there driver supports devices with one or two metadata
> formats. Prepare it to support more than two metadata formats.
> 
> This is achieved with the introduction of a new field `meta_formats`,
> that contains the array of metadata formats supported by the device, in
> the order expected by userspace.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Thank you for doing this. See my review remarks on patch 4/5 .

Regards,

Hans


> ---
>  drivers/media/usb/uvc/uvc_driver.c   |  7 ++++++
>  drivers/media/usb/uvc/uvc_metadata.c | 46 ++++++++++++++++++++++++++++++------
>  drivers/media/usb/uvc/uvcvideo.h     |  2 ++
>  3 files changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 62eb45435d8bec5c955720ecb46fb8936871e6cc..9de5abb43e19d9e876cddc5d7124592953db89ac 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -2315,6 +2315,13 @@ static int uvc_probe(struct usb_interface *intf,
>  		goto error;
>  	}
>  
> +	ret = uvc_meta_init(dev);
> +	if (ret < 0) {
> +		dev_err(&dev->udev->dev,
> +			"Error initializing the metadata formats (%d)\n", ret);
> +		goto error;
> +	}
> +
>  	if (dev->quirks & UVC_QUIRK_NO_RESET_RESUME)
>  		udev->quirks &= ~USB_QUIRK_RESET_RESUME;
>  
> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
> index 82de7781f5b6b70c5ba16bcba9e0741231231904..bc84e849174397f41d1e20bf890a876eeb5a9c67 100644
> --- a/drivers/media/usb/uvc/uvc_metadata.c
> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> @@ -64,14 +64,20 @@ static int uvc_meta_v4l2_try_format(struct file *file, void *fh,
>  	struct uvc_device *dev = stream->dev;
>  	struct v4l2_meta_format *fmt = &format->fmt.meta;
>  	u32 fmeta = fmt->dataformat;
> +	u32 i;
>  
>  	if (format->type != vfh->vdev->queue->type)
>  		return -EINVAL;
>  
> +	for (i = 0; (fmeta != dev->meta_formats[i]) && dev->meta_formats[i];
> +	     i++)
> +		;
> +	if (!dev->meta_formats[i])
> +		fmeta = V4L2_META_FMT_UVC;
> +
>  	memset(fmt, 0, sizeof(*fmt));
>  
> -	fmt->dataformat = fmeta == dev->info->meta_format
> -			? fmeta : V4L2_META_FMT_UVC;
> +	fmt->dataformat = fmeta;
>  	fmt->buffersize = UVC_METADATA_BUF_SIZE;
>  
>  	return 0;
> @@ -112,17 +118,21 @@ static int uvc_meta_v4l2_enum_formats(struct file *file, void *fh,
>  	struct v4l2_fh *vfh = file->private_data;
>  	struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
>  	struct uvc_device *dev = stream->dev;
> -	u32 index = fdesc->index;
> +	u32 i;
> +
> +	if (fdesc->type != vfh->vdev->queue->type)
> +		return -EINVAL;
>  
> -	if (fdesc->type != vfh->vdev->queue->type ||
> -	    index > 1U || (index && !dev->info->meta_format))
> +	for (i = 0; (i < fdesc->index) && dev->meta_formats[i]; i++)
> +		;
> +	if (!dev->meta_formats[i])
>  		return -EINVAL;
>  
>  	memset(fdesc, 0, sizeof(*fdesc));
>  
>  	fdesc->type = vfh->vdev->queue->type;
> -	fdesc->index = index;
> -	fdesc->pixelformat = index ? dev->info->meta_format : V4L2_META_FMT_UVC;
> +	fdesc->index = i;
> +	fdesc->pixelformat = dev->meta_formats[i];
>  
>  	return 0;
>  }
> @@ -174,3 +184,25 @@ int uvc_meta_register(struct uvc_streaming *stream)
>  					 V4L2_BUF_TYPE_META_CAPTURE,
>  					 &uvc_meta_fops, &uvc_meta_ioctl_ops);
>  }
> +
> +int uvc_meta_init(struct uvc_device *dev)
> +{
> +	static const u32 uvch_only[] = {V4L2_META_FMT_UVC, 0};
> +	static const u32 d4xx_format[] = {V4L2_META_FMT_UVC, V4L2_META_FMT_D4XX,
> +					  0};
> +
> +	switch (dev->info->meta_format) {
> +	case V4L2_META_FMT_D4XX:
> +		dev->meta_formats = d4xx_format;
> +		break;
> +	case 0:
> +		dev->meta_formats = uvch_only;
> +		break;
> +	default:
> +		dev_err(&dev->udev->dev, "Unknown metadata format 0x%x\n",
> +			dev->info->meta_format);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 11d6e3c2ebdfbabd7bbe5722f88ff85f406d9bb6..502f1d5608637cd28ce6f01aee31c4f5df160081 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -581,6 +581,7 @@ struct uvc_device {
>  	char name[32];
>  
>  	const struct uvc_device_info *info;
> +	const u32 *meta_formats; /* Zero-ended list of meta formats */
>  
>  	atomic_t nmappings;
>  
> @@ -751,6 +752,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
>  void uvc_video_clock_update(struct uvc_streaming *stream,
>  			    struct vb2_v4l2_buffer *vbuf,
>  			    struct uvc_buffer *buf);
> +int uvc_meta_init(struct uvc_device *dev);
>  int uvc_meta_register(struct uvc_streaming *stream);
>  
>  int uvc_register_video_device(struct uvc_device *dev,
> 


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

* Re: [PATCH v7 4/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5
  2025-07-07 11:34   ` Hans de Goede
@ 2025-07-07 11:44     ` Ricardo Ribalda
  2025-07-07 11:52       ` Hans de Goede
  0 siblings, 1 reply; 10+ messages in thread
From: Ricardo Ribalda @ 2025-07-07 11:44 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
	Greg Kroah-Hartman, linux-media, linux-kernel, linux-usb

On Mon, 7 Jul 2025 at 13:34, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi Ricardo,
>
> Thank you for the new version of this series.
>
> On 17-Jun-25 16:42, Ricardo Ribalda wrote:
> > The UVC driver provides two metadata types V4L2_META_FMT_UVC, and
> > V4L2_META_FMT_D4XX. The only difference between the two of them is that
> > V4L2_META_FMT_UVC only copies PTS, SCR, size and flags, and
> > V4L2_META_FMT_D4XX copies the whole metadata section.
> >
> > Now we only enable V4L2_META_FMT_D4XX for the Intel D4xx family of
> > devices, but it is useful to have the whole metadata payload for any
> > device where vendors include other metadata, such as the one described by
> > Microsoft:
> > https://learn.microsoft.com/en-us/windows-hardware/drivers/stream/mf-capture-metadata
> >
> > This patch introduces a new format V4L2_META_FMT_UVC_MSXU_1_5, that is
> > identical to V4L2_META_FMT_D4XX.
> >
> > Let the user enable this format with a quirk for now. This way they can
> > test if their devices provide useful metadata without rebuilding the
> > kernel. They can later contribute patches to auto-quirk their devices.
> > We will also work in methods to auto-detect devices compatible with this
> > new metadata format.
> >
> > Suggested-by: Hans de Goede <hdegoede@redhat.com>
> > Reviewed-by: Hans de Goede <hansg@kernel.org>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  .../userspace-api/media/v4l/meta-formats.rst       |  1 +
> >  .../media/v4l/metafmt-uvc-msxu-1-5.rst             | 23 ++++++++++++++++++++++
> >  MAINTAINERS                                        |  1 +
> >  drivers/media/usb/uvc/uvc_metadata.c               | 20 +++++++++++++++++--
> >  drivers/media/usb/uvc/uvcvideo.h                   |  1 +
> >  drivers/media/v4l2-core/v4l2-ioctl.c               |  1 +
> >  include/uapi/linux/videodev2.h                     |  1 +
> >  7 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/meta-formats.rst b/Documentation/userspace-api/media/v4l/meta-formats.rst
> > index bb6876cfc271e1a0543eee4209d6251e1a6a73cc..0de80328c36bf148051a19abe9e5241234ddfe5c 100644
> > --- a/Documentation/userspace-api/media/v4l/meta-formats.rst
> > +++ b/Documentation/userspace-api/media/v4l/meta-formats.rst
> > @@ -20,6 +20,7 @@ These formats are used for the :ref:`metadata` interface only.
> >      metafmt-pisp-fe
> >      metafmt-rkisp1
> >      metafmt-uvc
> > +    metafmt-uvc-msxu-1-5
> >      metafmt-vivid
> >      metafmt-vsp1-hgo
> >      metafmt-vsp1-hgt
> > diff --git a/Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst b/Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..dd1c3076df243d770a13e7f6d07c3296a269e16a
> > --- /dev/null
> > +++ b/Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst
> > @@ -0,0 +1,23 @@
> > +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> > +
> > +.. _v4l2-meta-fmt-uvc-msxu-1-5:
> > +
> > +***********************************
> > +V4L2_META_FMT_UVC_MSXU_1_5 ('UVCM')
> > +***********************************
> > +
> > +Microsoft(R)'s UVC Payload Metadata.
> > +
> > +
> > +Description
> > +===========
> > +
> > +V4L2_META_FMT_UVC_MSXU_1_5 buffers follow the metadata buffer layout of
> > +V4L2_META_FMT_UVC with the only difference that it includes all the UVC
> > +metadata in the `buffer[]` field, not just the first 2-12 bytes.
> > +
> > +The metadata format follows the specification from Microsoft(R) [1].
> > +
> > +.. _1:
> > +
> > +[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extensions-1-5
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e8f3dc93a56921924f57e7d5a03ea2fa182a4448..87101630e528297c57b22ffc2fe553e3864d25cc 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -25816,6 +25816,7 @@ S:    Maintained
> >  W:   http://www.ideasonboard.org/uvc/
> >  T:   git git://linuxtv.org/media.git
> >  F:   Documentation/userspace-api/media/drivers/uvcvideo.rst
> > +F:   Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst
> >  F:   Documentation/userspace-api/media/v4l/metafmt-uvc.rst
> >  F:   drivers/media/common/uvc.c
> >  F:   drivers/media/usb/uvc/
> > diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
> > index bc84e849174397f41d1e20bf890a876eeb5a9c67..b09f81d907d64f7d7a3b0dc52de319879b7e68be 100644
> > --- a/drivers/media/usb/uvc/uvc_metadata.c
> > +++ b/drivers/media/usb/uvc/uvc_metadata.c
> > @@ -190,13 +190,29 @@ int uvc_meta_init(struct uvc_device *dev)
> >       static const u32 uvch_only[] = {V4L2_META_FMT_UVC, 0};
> >       static const u32 d4xx_format[] = {V4L2_META_FMT_UVC, V4L2_META_FMT_D4XX,
> >                                         0};
> > +     static const u32 all_formats[] = {V4L2_META_FMT_UVC, V4L2_META_FMT_D4XX,
> > +                                       V4L2_META_FMT_UVC_MSXU_1_5, 0};
> > +     static const u32 ms_format[] = {V4L2_META_FMT_UVC,
> > +                                     V4L2_META_FMT_UVC_MSXU_1_5, 0};
>
> Hmm, this does not look great, I guess we are not expecting any
> new metadata formats soon but just needing the 4 arrays here and
> then ... (continued below).

Yeah, this looks better :)


Will implement it as you describe. Just a couple of comments.



>
>
> > +     bool support_msxu;
> > +
> > +     support_msxu = dev->quirks & UVC_QUIRK_MSXU_META;
> >
> >       switch (dev->info->meta_format) {
> > +     case V4L2_META_FMT_UVC_MSXU_1_5:
> > +             dev->meta_formats = ms_format;
> > +             break;
> >       case V4L2_META_FMT_D4XX:
> > -             dev->meta_formats = d4xx_format;
> > +             if (support_msxu)
> > +                     dev->meta_formats = all_formats;
> > +             else
> > +                     dev->meta_formats = d4xx_format;
> >               break;
> >       case 0:
> > -             dev->meta_formats = uvch_only;
> > +             if (support_msxu)
> > +                     dev->meta_formats = ms_format;
> > +             else
> > +                     dev->meta_formats = uvch_only;
>
> Also having these if else's here both don't look nice /
> this does not feel clean.
>
> My suggestion would be to instead do the following:
>
> 1. Add a #define UVC_MAX_META_DATA_FORMATS 3 to uvcvideo.h
> 2. In the struct uvc_device definition change meta_formats to:
>
>         u32 meta_formats[UVC_MAX_META_DATA_FORMATS + 1];
>
> 3. Change uvc_meta_init() to:
>
> void uvc_meta_init(struct uvc_device *dev)
> {
>         unsigned int i = 0;
>
>         dev->meta_formats[i++] = V4L2_META_FMT_UVC;
>
>         if (dev->info->meta_format)
>                 dev->meta_formats[i++] = dev->info->meta_format;
>
>         if (dev->quirks & UVC_QUIRK_MSXU_META)

         if (dev->quirks & UVC_QUIRK_MSXU_META) &&
dev->meta_formats[i-1] != V4L2_META_FMT_UVC_MSXU_1_5)
>                 dev->meta_formats[i++] = V4L2_META_FMT_UVC_MSXU_1_5;
>
>         /* IMPORTANT for new meta-formats update UVC_MAX_META_DATA_FORMATS */

Do we really need this comment? Even if we add more formats the total
number of formats supported will never be more than 3.

FMT_UVC, device_specific, msxu


>
>         dev->meta_formats[i++] = 0;
> }
>
> Note uvc_meta_init() now also is void, so no more need to error check it.
>
> The only downside I can see is that if we ever actually start setting
> dev->info->meta_format = V4L2_META_FMT_UVC_MSXU_1_5 and a user manually
> enables the quirk we get V4L2_META_FMT_UVC_MSXU_1_5 listed twice, but
> that should not cause any issues and normally that will never happen.
>
> IMHO this is better, then the switch-case + if-else code.
>
> Stating the obvious: some / most of these changes should be done in patch 3/5
> already.
>
> Regards,
>
> Hans
>
>
>
>
> >               break;
> >       default:
> >               dev_err(&dev->udev->dev, "Unknown metadata format 0x%x\n",
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 502f1d5608637cd28ce6f01aee31c4f5df160081..3578ce72fb6a1153ae79c244ec10093e8efdd739 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -77,6 +77,7 @@
> >  #define UVC_QUIRK_DISABLE_AUTOSUSPEND        0x00008000
> >  #define UVC_QUIRK_INVALID_DEVICE_SOF 0x00010000
> >  #define UVC_QUIRK_MJPEG_NO_EOF               0x00020000
> > +#define UVC_QUIRK_MSXU_META          0x00040000
> >
> >  /* Format flags */
> >  #define UVC_FMT_FLAG_COMPRESSED              0x00000001
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index 650dc1956f73d2f1943b56c42140c7b8d757259f..ba508f7fb577021497009ab23a7be5add23fd08c 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1459,6 +1459,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> >       case V4L2_META_FMT_VSP1_HGO:    descr = "R-Car VSP1 1-D Histogram"; break;
> >       case V4L2_META_FMT_VSP1_HGT:    descr = "R-Car VSP1 2-D Histogram"; break;
> >       case V4L2_META_FMT_UVC:         descr = "UVC Payload Header Metadata"; break;
> > +     case V4L2_META_FMT_UVC_MSXU_1_5:        descr = "UVC MSXU Metadata"; break;
> >       case V4L2_META_FMT_D4XX:        descr = "Intel D4xx UVC Metadata"; break;
> >       case V4L2_META_FMT_VIVID:       descr = "Vivid Metadata"; break;
> >       case V4L2_META_FMT_RK_ISP1_PARAMS:      descr = "Rockchip ISP1 3A Parameters"; break;
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 9e3b366d5fc79d8a04c6f0752858fc23363db65c..75f2096b2d4fed5e0235ea4732d35044ff77a98b 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -861,6 +861,7 @@ struct v4l2_pix_format {
> >  #define V4L2_META_FMT_VSP1_HGT    v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */
> >  #define V4L2_META_FMT_UVC         v4l2_fourcc('U', 'V', 'C', 'H') /* UVC Payload Header metadata */
> >  #define V4L2_META_FMT_D4XX        v4l2_fourcc('D', '4', 'X', 'X') /* D4XX Payload Header metadata */
> > +#define V4L2_META_FMT_UVC_MSXU_1_5  v4l2_fourcc('U', 'V', 'C', 'M') /* UVC MSXU metadata */
> >  #define V4L2_META_FMT_VIVID    v4l2_fourcc('V', 'I', 'V', 'D') /* Vivid Metadata */
> >
> >  /* Vendor specific - used for RK_ISP1 camera sub-system */
> >
>


-- 
Ricardo Ribalda

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

* Re: [PATCH v7 4/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5
  2025-07-07 11:44     ` Ricardo Ribalda
@ 2025-07-07 11:52       ` Hans de Goede
  0 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2025-07-07 11:52 UTC (permalink / raw)
  To: Ricardo Ribalda
  Cc: Laurent Pinchart, Mauro Carvalho Chehab, Guennadi Liakhovetski,
	Greg Kroah-Hartman, linux-media, linux-kernel, linux-usb

Hi,

On 7-Jul-25 13:44, Ricardo Ribalda wrote:
> On Mon, 7 Jul 2025 at 13:34, Hans de Goede <hansg@kernel.org> wrote:
>>
>> Hi Ricardo,
>>
>> Thank you for the new version of this series.
>>
>> On 17-Jun-25 16:42, Ricardo Ribalda wrote:
>>> The UVC driver provides two metadata types V4L2_META_FMT_UVC, and
>>> V4L2_META_FMT_D4XX. The only difference between the two of them is that
>>> V4L2_META_FMT_UVC only copies PTS, SCR, size and flags, and
>>> V4L2_META_FMT_D4XX copies the whole metadata section.
>>>
>>> Now we only enable V4L2_META_FMT_D4XX for the Intel D4xx family of
>>> devices, but it is useful to have the whole metadata payload for any
>>> device where vendors include other metadata, such as the one described by
>>> Microsoft:
>>> https://learn.microsoft.com/en-us/windows-hardware/drivers/stream/mf-capture-metadata
>>>
>>> This patch introduces a new format V4L2_META_FMT_UVC_MSXU_1_5, that is
>>> identical to V4L2_META_FMT_D4XX.
>>>
>>> Let the user enable this format with a quirk for now. This way they can
>>> test if their devices provide useful metadata without rebuilding the
>>> kernel. They can later contribute patches to auto-quirk their devices.
>>> We will also work in methods to auto-detect devices compatible with this
>>> new metadata format.
>>>
>>> Suggested-by: Hans de Goede <hdegoede@redhat.com>
>>> Reviewed-by: Hans de Goede <hansg@kernel.org>
>>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>>> ---
>>>  .../userspace-api/media/v4l/meta-formats.rst       |  1 +
>>>  .../media/v4l/metafmt-uvc-msxu-1-5.rst             | 23 ++++++++++++++++++++++
>>>  MAINTAINERS                                        |  1 +
>>>  drivers/media/usb/uvc/uvc_metadata.c               | 20 +++++++++++++++++--
>>>  drivers/media/usb/uvc/uvcvideo.h                   |  1 +
>>>  drivers/media/v4l2-core/v4l2-ioctl.c               |  1 +
>>>  include/uapi/linux/videodev2.h                     |  1 +
>>>  7 files changed, 46 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/meta-formats.rst b/Documentation/userspace-api/media/v4l/meta-formats.rst
>>> index bb6876cfc271e1a0543eee4209d6251e1a6a73cc..0de80328c36bf148051a19abe9e5241234ddfe5c 100644
>>> --- a/Documentation/userspace-api/media/v4l/meta-formats.rst
>>> +++ b/Documentation/userspace-api/media/v4l/meta-formats.rst
>>> @@ -20,6 +20,7 @@ These formats are used for the :ref:`metadata` interface only.
>>>      metafmt-pisp-fe
>>>      metafmt-rkisp1
>>>      metafmt-uvc
>>> +    metafmt-uvc-msxu-1-5
>>>      metafmt-vivid
>>>      metafmt-vsp1-hgo
>>>      metafmt-vsp1-hgt
>>> diff --git a/Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst b/Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..dd1c3076df243d770a13e7f6d07c3296a269e16a
>>> --- /dev/null
>>> +++ b/Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst
>>> @@ -0,0 +1,23 @@
>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>> +
>>> +.. _v4l2-meta-fmt-uvc-msxu-1-5:
>>> +
>>> +***********************************
>>> +V4L2_META_FMT_UVC_MSXU_1_5 ('UVCM')
>>> +***********************************
>>> +
>>> +Microsoft(R)'s UVC Payload Metadata.
>>> +
>>> +
>>> +Description
>>> +===========
>>> +
>>> +V4L2_META_FMT_UVC_MSXU_1_5 buffers follow the metadata buffer layout of
>>> +V4L2_META_FMT_UVC with the only difference that it includes all the UVC
>>> +metadata in the `buffer[]` field, not just the first 2-12 bytes.
>>> +
>>> +The metadata format follows the specification from Microsoft(R) [1].
>>> +
>>> +.. _1:
>>> +
>>> +[1] https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-extensions-1-5
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index e8f3dc93a56921924f57e7d5a03ea2fa182a4448..87101630e528297c57b22ffc2fe553e3864d25cc 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -25816,6 +25816,7 @@ S:    Maintained
>>>  W:   http://www.ideasonboard.org/uvc/
>>>  T:   git git://linuxtv.org/media.git
>>>  F:   Documentation/userspace-api/media/drivers/uvcvideo.rst
>>> +F:   Documentation/userspace-api/media/v4l/metafmt-uvc-msxu-1-5.rst
>>>  F:   Documentation/userspace-api/media/v4l/metafmt-uvc.rst
>>>  F:   drivers/media/common/uvc.c
>>>  F:   drivers/media/usb/uvc/
>>> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
>>> index bc84e849174397f41d1e20bf890a876eeb5a9c67..b09f81d907d64f7d7a3b0dc52de319879b7e68be 100644
>>> --- a/drivers/media/usb/uvc/uvc_metadata.c
>>> +++ b/drivers/media/usb/uvc/uvc_metadata.c
>>> @@ -190,13 +190,29 @@ int uvc_meta_init(struct uvc_device *dev)
>>>       static const u32 uvch_only[] = {V4L2_META_FMT_UVC, 0};
>>>       static const u32 d4xx_format[] = {V4L2_META_FMT_UVC, V4L2_META_FMT_D4XX,
>>>                                         0};
>>> +     static const u32 all_formats[] = {V4L2_META_FMT_UVC, V4L2_META_FMT_D4XX,
>>> +                                       V4L2_META_FMT_UVC_MSXU_1_5, 0};
>>> +     static const u32 ms_format[] = {V4L2_META_FMT_UVC,
>>> +                                     V4L2_META_FMT_UVC_MSXU_1_5, 0};
>>
>> Hmm, this does not look great, I guess we are not expecting any
>> new metadata formats soon but just needing the 4 arrays here and
>> then ... (continued below).
> 
> Yeah, this looks better :)
> 
> 
> Will implement it as you describe. Just a couple of comments.
> 
> 
> 
>>
>>
>>> +     bool support_msxu;
>>> +
>>> +     support_msxu = dev->quirks & UVC_QUIRK_MSXU_META;
>>>
>>>       switch (dev->info->meta_format) {
>>> +     case V4L2_META_FMT_UVC_MSXU_1_5:
>>> +             dev->meta_formats = ms_format;
>>> +             break;
>>>       case V4L2_META_FMT_D4XX:
>>> -             dev->meta_formats = d4xx_format;
>>> +             if (support_msxu)
>>> +                     dev->meta_formats = all_formats;
>>> +             else
>>> +                     dev->meta_formats = d4xx_format;
>>>               break;
>>>       case 0:
>>> -             dev->meta_formats = uvch_only;
>>> +             if (support_msxu)
>>> +                     dev->meta_formats = ms_format;
>>> +             else
>>> +                     dev->meta_formats = uvch_only;
>>
>> Also having these if else's here both don't look nice /
>> this does not feel clean.
>>
>> My suggestion would be to instead do the following:
>>
>> 1. Add a #define UVC_MAX_META_DATA_FORMATS 3 to uvcvideo.h
>> 2. In the struct uvc_device definition change meta_formats to:
>>
>>         u32 meta_formats[UVC_MAX_META_DATA_FORMATS + 1];
>>
>> 3. Change uvc_meta_init() to:
>>
>> void uvc_meta_init(struct uvc_device *dev)
>> {
>>         unsigned int i = 0;
>>
>>         dev->meta_formats[i++] = V4L2_META_FMT_UVC;
>>
>>         if (dev->info->meta_format)
>>                 dev->meta_formats[i++] = dev->info->meta_format;
>>
>>         if (dev->quirks & UVC_QUIRK_MSXU_META)
> 
>          if (dev->quirks & UVC_QUIRK_MSXU_META) &&
> dev->meta_formats[i-1] != V4L2_META_FMT_UVC_MSXU_1_5)
>>                 dev->meta_formats[i++] = V4L2_META_FMT_UVC_MSXU_1_5;
>>
>>         /* IMPORTANT for new meta-formats update UVC_MAX_META_DATA_FORMATS */
> 
> Do we really need this comment? Even if we add more formats the total
> number of formats supported will never be more than 3.
> 
> FMT_UVC, device_specific, msxu

The comment is at the place where one would add a new if in case
for some reason we do add another path to add a meta-format in
that case if all 3 (with the new if) if conditions evaluate to
true then we would overrun the array.

Unfortunately there is no way to do e.g. a static_assert()
for that, which is why I still believe at least having
the comment would be good.

That and/or add a:

	WARN_ON(i > UVC_MAX_META_DATA_FORMATS);

Before adding the final 0 terminator. I actually had
the WARN_ON in there first, but a comment seemed to make
more sense...

Regards,

Hans





>>         dev->meta_formats[i++] = 0;
>> }
>>
>> Note uvc_meta_init() now also is void, so no more need to error check it.
>>
>> The only downside I can see is that if we ever actually start setting
>> dev->info->meta_format = V4L2_META_FMT_UVC_MSXU_1_5 and a user manually
>> enables the quirk we get V4L2_META_FMT_UVC_MSXU_1_5 listed twice, but
>> that should not cause any issues and normally that will never happen.
>>
>> IMHO this is better, then the switch-case + if-else code.
>>
>> Stating the obvious: some / most of these changes should be done in patch 3/5
>> already.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>>               break;
>>>       default:
>>>               dev_err(&dev->udev->dev, "Unknown metadata format 0x%x\n",
>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
>>> index 502f1d5608637cd28ce6f01aee31c4f5df160081..3578ce72fb6a1153ae79c244ec10093e8efdd739 100644
>>> --- a/drivers/media/usb/uvc/uvcvideo.h
>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
>>> @@ -77,6 +77,7 @@
>>>  #define UVC_QUIRK_DISABLE_AUTOSUSPEND        0x00008000
>>>  #define UVC_QUIRK_INVALID_DEVICE_SOF 0x00010000
>>>  #define UVC_QUIRK_MJPEG_NO_EOF               0x00020000
>>> +#define UVC_QUIRK_MSXU_META          0x00040000
>>>
>>>  /* Format flags */
>>>  #define UVC_FMT_FLAG_COMPRESSED              0x00000001
>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> index 650dc1956f73d2f1943b56c42140c7b8d757259f..ba508f7fb577021497009ab23a7be5add23fd08c 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
>>> @@ -1459,6 +1459,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
>>>       case V4L2_META_FMT_VSP1_HGO:    descr = "R-Car VSP1 1-D Histogram"; break;
>>>       case V4L2_META_FMT_VSP1_HGT:    descr = "R-Car VSP1 2-D Histogram"; break;
>>>       case V4L2_META_FMT_UVC:         descr = "UVC Payload Header Metadata"; break;
>>> +     case V4L2_META_FMT_UVC_MSXU_1_5:        descr = "UVC MSXU Metadata"; break;
>>>       case V4L2_META_FMT_D4XX:        descr = "Intel D4xx UVC Metadata"; break;
>>>       case V4L2_META_FMT_VIVID:       descr = "Vivid Metadata"; break;
>>>       case V4L2_META_FMT_RK_ISP1_PARAMS:      descr = "Rockchip ISP1 3A Parameters"; break;
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 9e3b366d5fc79d8a04c6f0752858fc23363db65c..75f2096b2d4fed5e0235ea4732d35044ff77a98b 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -861,6 +861,7 @@ struct v4l2_pix_format {
>>>  #define V4L2_META_FMT_VSP1_HGT    v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */
>>>  #define V4L2_META_FMT_UVC         v4l2_fourcc('U', 'V', 'C', 'H') /* UVC Payload Header metadata */
>>>  #define V4L2_META_FMT_D4XX        v4l2_fourcc('D', '4', 'X', 'X') /* D4XX Payload Header metadata */
>>> +#define V4L2_META_FMT_UVC_MSXU_1_5  v4l2_fourcc('U', 'V', 'C', 'M') /* UVC MSXU metadata */
>>>  #define V4L2_META_FMT_VIVID    v4l2_fourcc('V', 'I', 'V', 'D') /* Vivid Metadata */
>>>
>>>  /* Vendor specific - used for RK_ISP1 camera sub-system */
>>>
>>
> 
> 


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

end of thread, other threads:[~2025-07-07 11:52 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-17 14:42 [PATCH v7 0/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 + other meta fixes Ricardo Ribalda
2025-06-17 14:42 ` [PATCH v7 1/5] media: uvcvideo: Do not mark valid metadata as invalid Ricardo Ribalda
2025-06-17 14:42 ` [PATCH v7 2/5] media: Documentation: Add note about UVCH length field Ricardo Ribalda
2025-06-17 14:42 ` [PATCH v7 3/5] media: uvcvideo: Introduce dev->meta_formats Ricardo Ribalda
2025-07-07 11:35   ` Hans de Goede
2025-06-17 14:42 ` [PATCH v7 4/5] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 Ricardo Ribalda
2025-07-07 11:34   ` Hans de Goede
2025-07-07 11:44     ` Ricardo Ribalda
2025-07-07 11:52       ` Hans de Goede
2025-06-17 14:42 ` [PATCH v7 5/5] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META Ricardo Ribalda

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