[PATCH v6 0/4] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 + other meta fixes

[PATCH v6 0/4] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5 + other meta fixes

[Ricardo Ribalda]

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 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 (4):
      media: uvcvideo: Do not mark valid metadata as invalid
      media: Documentation: Add note about UVCH length field
      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_metadata.c               | 116 +++++++++++++++++++--
 drivers/media/usb/uvc/uvc_video.c                  |  12 +--
 drivers/media/usb/uvc/uvcvideo.h                   |   1 +
 drivers/media/v4l2-core/v4l2-ioctl.c               |   1 +
 include/linux/usb/uvc.h                            |   3 +
 include/uapi/linux/videodev2.h                     |   1 +
 10 files changed, 150 insertions(+), 13 deletions(-)
---
base-commit: 5e1ff2314797bf53636468a97719a8222deca9ae
change-id: 20250403-uvc-meta-e556773d12ae

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

[PATCH v6 1/4] media: uvcvideo: Do not mark valid metadata as invalid

[Ricardo Ribalda]

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 e3567aeb0007c1f0a766f331e4e744359e95a863..b113297dac61f1b2eecd72c36ea61ef2c1e7d28a 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1433,12 +1433,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;
 
@@ -1459,6 +1453,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.rc0.604.gd4ff7b7c86-goog

[PATCH v6 2/4] media: Documentation: Add note about UVCH length field

[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.rc0.604.gd4ff7b7c86-goog

[PATCH v6 3/4] media: uvcvideo: Introduce V4L2_META_FMT_UVC_MSXU_1_5

[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               | 44 +++++++++++++++++++---
 drivers/media/usb/uvc/uvcvideo.h                   |  1 +
 drivers/media/v4l2-core/v4l2-ioctl.c               |  1 +
 include/uapi/linux/videodev2.h                     |  1 +
 7 files changed, 66 insertions(+), 6 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 137122d3dc4e46194eba1c4e035e24b203ed46c5..939def0992c259655c3ae1cc8a757ce53e9c650e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25215,6 +25215,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 82de7781f5b6b70c5ba16bcba9e0741231231904..df3f259271c675feb590c4534dad95b3b786f082 100644
--- a/drivers/media/usb/uvc/uvc_metadata.c
+++ b/drivers/media/usb/uvc/uvc_metadata.c
@@ -63,15 +63,22 @@ static int uvc_meta_v4l2_try_format(struct file *file, void *fh,
 	struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
 	struct uvc_device *dev = stream->dev;
 	struct v4l2_meta_format *fmt = &format->fmt.meta;
-	u32 fmeta = fmt->dataformat;
+	u32 fmeta;
+
+	if (fmt->dataformat == dev->info->meta_format)
+		fmeta = dev->info->meta_format;
+	else if (fmt->dataformat == V4L2_META_FMT_UVC_MSXU_1_5 &&
+		 dev->quirks & UVC_QUIRK_MSXU_META)
+		fmeta = V4L2_META_FMT_UVC_MSXU_1_5;
+	else
+		fmeta = V4L2_META_FMT_UVC;
 
 	if (format->type != vfh->vdev->queue->type)
 		return -EINVAL;
 
 	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;
@@ -106,6 +113,27 @@ static int uvc_meta_v4l2_set_format(struct file *file, void *fh,
 	return ret;
 }
 
+static u32 uvc_meta_idx_to_fmeta(struct uvc_device *dev, u32 index)
+{
+	switch (index) {
+	case 0:
+		return V4L2_META_FMT_UVC;
+	case 1:
+		if (dev->info->meta_format)
+			return dev->info->meta_format;
+		if (dev->quirks & UVC_QUIRK_MSXU_META)
+			return V4L2_META_FMT_UVC_MSXU_1_5;
+		return 0;
+	case 2:
+		if (dev->info->meta_format &&
+		    dev->quirks & UVC_QUIRK_MSXU_META)
+			return V4L2_META_FMT_UVC_MSXU_1_5;
+		return 0;
+	}
+
+	return 0;
+}
+
 static int uvc_meta_v4l2_enum_formats(struct file *file, void *fh,
 				      struct v4l2_fmtdesc *fdesc)
 {
@@ -113,16 +141,20 @@ static int uvc_meta_v4l2_enum_formats(struct file *file, void *fh,
 	struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
 	struct uvc_device *dev = stream->dev;
 	u32 index = fdesc->index;
+	u32 fmeta;
+
+	if (fdesc->type != vfh->vdev->queue->type)
+		return -EINVAL;
 
-	if (fdesc->type != vfh->vdev->queue->type ||
-	    index > 1U || (index && !dev->info->meta_format))
+	fmeta = uvc_meta_idx_to_fmeta(dev, fdesc->index);
+	if (!fmeta)
 		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->pixelformat = fmeta;
 
 	return 0;
 }
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index b9f8eb62ba1d82ea7788cf6c10cc838a429dbc9e..2b6d9ddc18e751bef4d295439c460b741d1a1a2f 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.rc0.604.gd4ff7b7c86-goog

[PATCH v6 4/4] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META

[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 | 72 ++++++++++++++++++++++++++++++++++++
 include/linux/usb/uvc.h              |  3 ++
 2 files changed, 75 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
index df3f259271c675feb590c4534dad95b3b786f082..cd58427578ff413591b60abe0a210b90802dddc7 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>
@@ -188,11 +189,82 @@ static const struct v4l2_file_operations uvc_meta_fops = {
 	.mmap = vb2_fop_mmap,
 };
 
+static const u8 uvc_msxu_guid[16] = UVC_GUID_MSXU_1_5;
+
+static struct uvc_entity *uvc_meta_find_msxu(struct uvc_device *dev)
+{
+	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;
 	struct video_device *vdev = &stream->meta.vdev;
 	struct uvc_video_queue *queue = &stream->meta.queue;
+	int ret;
+
+	ret = uvc_meta_detect_msxu(dev);
+	if (ret)
+		return ret;
 
 	stream->meta.format = V4L2_META_FMT_UVC;
 
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.rc0.604.gd4ff7b7c86-goog

Re: [PATCH v6 4/4] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META

[Hans de Goede]

Hi Ricardo,

On 4-Jun-25 14:16, Ricardo Ribalda wrote:
> 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 | 72 ++++++++++++++++++++++++++++++++++++
>  include/linux/usb/uvc.h              |  3 ++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
> index df3f259271c675feb590c4534dad95b3b786f082..cd58427578ff413591b60abe0a210b90802dddc7 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>
> @@ -188,11 +189,82 @@ static const struct v4l2_file_operations uvc_meta_fops = {
>  	.mmap = vb2_fop_mmap,
>  };
>  
> +static const u8 uvc_msxu_guid[16] = UVC_GUID_MSXU_1_5;
> +
> +static struct uvc_entity *uvc_meta_find_msxu(struct uvc_device *dev)
> +{
> +	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;

Since we set the ctrl to enable MSXU fmt metadata here, this means that
cameras which also support V4L2_META_FMT_D4XX will be switched to MSXU
metadata mode at probe() time.

So even if cameras exist which support both metadata formats, since we
switch to MSXU at probe() time, disabling V4L2_META_FMT_D4XX support,
the uvcvideo driver will only support 1 metadata fmt per camera.
Which is fine supporting more then 1 metadata fmt is not worth
the trouble IMHO.

This means that Laurent's remark on [PATCH v5 4/4]:

"I would prefer if you could instead add a metadata format field in the
uvc_device structure (I'd put it right after the info field, and while
at it you could move the quirks field to that section too). The metadata
format would be initialized from dev->info (when available) or set to
the UVC format, and overridden when the MSXU is detected."

is still relevant, which will also make patch 3/4 cleaner.

The idea is to (in patch 3/4):

1. Introduce a dev->meta_format which gets initialized from dev->info->meta_format
2. Keep the quirk and if the quirk is set override dev->meta_format to
   V4L2_META_FMT_UVC_MSXU_1_5 thus still allowing testing for MSXU metadata on
   cameras which lack the MSXU_CONTROL_METADATA control.

Doing things this way avoids the need for the complexity added to
uvc_meta_v4l2_try_format() / uvc_meta_v4l2_set_format() /
uvc_meta_v4l2_enum_format(). Instead the only changes necessary there now will
be replacing dev->info->meta_format with dev->meta_format.

Regards,

Hans





> +
> +	return 0;
> +}
> +
>  int uvc_meta_register(struct uvc_streaming *stream)
>  {
>  	struct uvc_device *dev = stream->dev;
>  	struct video_device *vdev = &stream->meta.vdev;
>  	struct uvc_video_queue *queue = &stream->meta.queue;
> +	int ret;
> +
> +	ret = uvc_meta_detect_msxu(dev);
> +	if (ret)
> +		return ret;
>  
>  	stream->meta.format = V4L2_META_FMT_UVC;
>  
> 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, \
> 

Re: [PATCH v6 4/4] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META

[Ricardo Ribalda]

Hi Hans

On Mon, 16 Jun 2025 at 16:38, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi Ricardo,
>
> On 4-Jun-25 14:16, Ricardo Ribalda wrote:
> > 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 | 72 ++++++++++++++++++++++++++++++++++++
> >  include/linux/usb/uvc.h              |  3 ++
> >  2 files changed, 75 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
> > index df3f259271c675feb590c4534dad95b3b786f082..cd58427578ff413591b60abe0a210b90802dddc7 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>
> > @@ -188,11 +189,82 @@ static const struct v4l2_file_operations uvc_meta_fops = {
> >       .mmap = vb2_fop_mmap,
> >  };
> >
> > +static const u8 uvc_msxu_guid[16] = UVC_GUID_MSXU_1_5;
> > +
> > +static struct uvc_entity *uvc_meta_find_msxu(struct uvc_device *dev)
> > +{
> > +     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;
>
> Since we set the ctrl to enable MSXU fmt metadata here, this means that
> cameras which also support V4L2_META_FMT_D4XX will be switched to MSXU
> metadata mode at probe() time.

Not sure that I completely follow you. D4XX cameras will not be
switched to MSXU, they will support MSXU and D4XX with the current
patchset.

>
> So even if cameras exist which support both metadata formats, since we
> switch to MSXU at probe() time, disabling V4L2_META_FMT_D4XX support,
> the uvcvideo driver will only support 1 metadata fmt per camera.
> Which is fine supporting more then 1 metadata fmt is not worth
> the trouble IMHO.

If we only support one metadata, we have two options for D4XX cameras:

A) Switch to MSXU: apps that expect D4XX will not work. I think this
will mean breaking uAPI.
B) Keep D4XX and ignore MSXU: apps that work with MSXU will not work
with D4XX cameras. I do not love this but it will not affect my
usecase.


If you are ok with B) I can start the implementation. But I still
believe that the current option is more generic and the extra
complexity is not too excessive.


>
> This means that Laurent's remark on [PATCH v5 4/4]:
>
> "I would prefer if you could instead add a metadata format field in the
> uvc_device structure (I'd put it right after the info field, and while
> at it you could move the quirks field to that section too). The metadata
> format would be initialized from dev->info (when available) or set to
> the UVC format, and overridden when the MSXU is detected."
>
> is still relevant, which will also make patch 3/4 cleaner.
>
> The idea is to (in patch 3/4):
>
> 1. Introduce a dev->meta_format which gets initialized from dev->info->meta_format
> 2. Keep the quirk and if the quirk is set override dev->meta_format to
>    V4L2_META_FMT_UVC_MSXU_1_5 thus still allowing testing for MSXU metadata on
>    cameras which lack the MSXU_CONTROL_METADATA control.
>
> Doing things this way avoids the need for the complexity added to
> uvc_meta_v4l2_try_format() / uvc_meta_v4l2_set_format() /
> uvc_meta_v4l2_enum_format(). Instead the only changes necessary there now will
> be replacing dev->info->meta_format with dev->meta_format.
>
> Regards,
>
> Hans
>
>
>
>
>
> > +
> > +     return 0;
> > +}
> > +
> >  int uvc_meta_register(struct uvc_streaming *stream)
> >  {
> >       struct uvc_device *dev = stream->dev;
> >       struct video_device *vdev = &stream->meta.vdev;
> >       struct uvc_video_queue *queue = &stream->meta.queue;
> > +     int ret;
> > +
> > +     ret = uvc_meta_detect_msxu(dev);
> > +     if (ret)
> > +             return ret;
> >
> >       stream->meta.format = V4L2_META_FMT_UVC;
> >
> > 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, \
> >
>


-- 
Ricardo Ribalda

Re: [PATCH v6 4/4] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META

[Hans de Goede]

Hi Ricardo,

On 16-Jun-25 5:04 PM, Ricardo Ribalda wrote:
> Hi Hans
> 
> On Mon, 16 Jun 2025 at 16:38, Hans de Goede <hansg@kernel.org> wrote:
>>
>> Hi Ricardo,
>>
>> On 4-Jun-25 14:16, Ricardo Ribalda wrote:
>>> 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 | 72 ++++++++++++++++++++++++++++++++++++
>>>  include/linux/usb/uvc.h              |  3 ++
>>>  2 files changed, 75 insertions(+)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
>>> index df3f259271c675feb590c4534dad95b3b786f082..cd58427578ff413591b60abe0a210b90802dddc7 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>
>>> @@ -188,11 +189,82 @@ static const struct v4l2_file_operations uvc_meta_fops = {
>>>       .mmap = vb2_fop_mmap,
>>>  };
>>>
>>> +static const u8 uvc_msxu_guid[16] = UVC_GUID_MSXU_1_5;
>>> +
>>> +static struct uvc_entity *uvc_meta_find_msxu(struct uvc_device *dev)
>>> +{
>>> +     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;
>>
>> Since we set the ctrl to enable MSXU fmt metadata here, this means that
>> cameras which also support V4L2_META_FMT_D4XX will be switched to MSXU
>> metadata mode at probe() time.
> 
> Not sure that I completely follow you. D4XX cameras will not be
> switched to MSXU, they will support MSXU and D4XX with the current
> patchset.

Is MSXU an extension on top of D4XX ? If not then we need to tell
the camera which metadata we want in uvc_meta_v4l2_set_format()

Currently your patch 4/4 does:

+	ret = uvc_query_ctrl(dev, UVC_SET_CUR, entity->id, dev->intfnum,
+			     MSXU_CONTROL_METADATA, data, sizeof(*data));

in uvc_meta_detect_msxu() which runs at probe time.

So patch 4/4 breaks V4L2_META_FMT_D4XX support as it switched the
camera to MSXU metadata mode (I'm assuming the 2 metadata formats
are different and that MSXU metadata is not just a superset of D4xx).

This is why I suggest supporting only one metadata format. If we
want to support both on cameras which support both and can switch
formats with the msxu control, then this patch needs to modify
uvc_meta_v4l2_set_format() to do something like this:

+	ret = uvc_query_ctrl(dev, UVC_SET_CUR, entity->id, dev->intfnum,
+			     MSXU_CONTROL_METADATA, data, sizeof(*data));

When switching formats, that or only support 1 metadata fmt.

I hope this explains my thinking here, if not keep asking questions ...

Regards,

Hans




> 
>>
>> So even if cameras exist which support both metadata formats, since we
>> switch to MSXU at probe() time, disabling V4L2_META_FMT_D4XX support,
>> the uvcvideo driver will only support 1 metadata fmt per camera.
>> Which is fine supporting more then 1 metadata fmt is not worth
>> the trouble IMHO.
> 
> If we only support one metadata, we have two options for D4XX cameras:
> 
> A) Switch to MSXU: apps that expect D4XX will not work. I think this
> will mean breaking uAPI.
> B) Keep D4XX and ignore MSXU: apps that work with MSXU will not work
> with D4XX cameras. I do not love this but it will not affect my
> usecase.
> 
> 
> If you are ok with B) I can start the implementation. But I still
> believe that the current option is more generic and the extra
> complexity is not too excessive.
> 
> 
>>
>> This means that Laurent's remark on [PATCH v5 4/4]:
>>
>> "I would prefer if you could instead add a metadata format field in the
>> uvc_device structure (I'd put it right after the info field, and while
>> at it you could move the quirks field to that section too). The metadata
>> format would be initialized from dev->info (when available) or set to
>> the UVC format, and overridden when the MSXU is detected."
>>
>> is still relevant, which will also make patch 3/4 cleaner.
>>
>> The idea is to (in patch 3/4):
>>
>> 1. Introduce a dev->meta_format which gets initialized from dev->info->meta_format
>> 2. Keep the quirk and if the quirk is set override dev->meta_format to
>>    V4L2_META_FMT_UVC_MSXU_1_5 thus still allowing testing for MSXU metadata on
>>    cameras which lack the MSXU_CONTROL_METADATA control.
>>
>> Doing things this way avoids the need for the complexity added to
>> uvc_meta_v4l2_try_format() / uvc_meta_v4l2_set_format() /
>> uvc_meta_v4l2_enum_format(). Instead the only changes necessary there now will
>> be replacing dev->info->meta_format with dev->meta_format.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>
>>> +
>>> +     return 0;
>>> +}
>>> +
>>>  int uvc_meta_register(struct uvc_streaming *stream)
>>>  {
>>>       struct uvc_device *dev = stream->dev;
>>>       struct video_device *vdev = &stream->meta.vdev;
>>>       struct uvc_video_queue *queue = &stream->meta.queue;
>>> +     int ret;
>>> +
>>> +     ret = uvc_meta_detect_msxu(dev);
>>> +     if (ret)
>>> +             return ret;
>>>
>>>       stream->meta.format = V4L2_META_FMT_UVC;
>>>
>>> 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, \
>>>
>>
> 
> 

Re: [PATCH v6 4/4] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META

[Ricardo Ribalda]

Hi Hans

On Mon, 16 Jun 2025 at 22:47, Hans de Goede <hansg@kernel.org> wrote:
>
> Hi Ricardo,
>
> On 16-Jun-25 5:04 PM, Ricardo Ribalda wrote:
> > Hi Hans
> >
> > On Mon, 16 Jun 2025 at 16:38, Hans de Goede <hansg@kernel.org> wrote:
> >>
> >> Hi Ricardo,
> >>
> >> On 4-Jun-25 14:16, Ricardo Ribalda wrote:
> >>> 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 | 72 ++++++++++++++++++++++++++++++++++++
> >>>  include/linux/usb/uvc.h              |  3 ++
> >>>  2 files changed, 75 insertions(+)
> >>>
> >>> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
> >>> index df3f259271c675feb590c4534dad95b3b786f082..cd58427578ff413591b60abe0a210b90802dddc7 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>
> >>> @@ -188,11 +189,82 @@ static const struct v4l2_file_operations uvc_meta_fops = {
> >>>       .mmap = vb2_fop_mmap,
> >>>  };
> >>>
> >>> +static const u8 uvc_msxu_guid[16] = UVC_GUID_MSXU_1_5;
> >>> +
> >>> +static struct uvc_entity *uvc_meta_find_msxu(struct uvc_device *dev)
> >>> +{
> >>> +     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;
> >>
> >> Since we set the ctrl to enable MSXU fmt metadata here, this means that
> >> cameras which also support V4L2_META_FMT_D4XX will be switched to MSXU
> >> metadata mode at probe() time.
> >
> > Not sure that I completely follow you. D4XX cameras will not be
> > switched to MSXU, they will support MSXU and D4XX with the current
> > patchset.
>
> Is MSXU an extension on top of D4XX ? If not then we need to tell
> the camera which metadata we want in uvc_meta_v4l2_set_format()

I think I know where the miss-comunication happened :)

MSXU is indeed a superset of D4xx. D4xx metadata is formatted in MSXU.

If an app fetches D4XX and MSXU from an Intel D4xx device, they are identical.

Or in other words, if D4XX devices have MSXU_CONTROL_METADATA, they
are probably today initialized with a value != 0.


Thanks!
>
> Currently your patch 4/4 does:
>
> +       ret = uvc_query_ctrl(dev, UVC_SET_CUR, entity->id, dev->intfnum,
> +                            MSXU_CONTROL_METADATA, data, sizeof(*data));
>
> in uvc_meta_detect_msxu() which runs at probe time.
>
> So patch 4/4 breaks V4L2_META_FMT_D4XX support as it switched the
> camera to MSXU metadata mode (I'm assuming the 2 metadata formats
> are different and that MSXU metadata is not just a superset of D4xx).
>
> This is why I suggest supporting only one metadata format. If we
> want to support both on cameras which support both and can switch
> formats with the msxu control, then this patch needs to modify
> uvc_meta_v4l2_set_format() to do something like this:
>
> +       ret = uvc_query_ctrl(dev, UVC_SET_CUR, entity->id, dev->intfnum,
> +                            MSXU_CONTROL_METADATA, data, sizeof(*data));
>
> When switching formats, that or only support 1 metadata fmt.
>
> I hope this explains my thinking here, if not keep asking questions ...
>
> Regards,
>
> Hans
>
>
>
>
> >
> >>
> >> So even if cameras exist which support both metadata formats, since we
> >> switch to MSXU at probe() time, disabling V4L2_META_FMT_D4XX support,
> >> the uvcvideo driver will only support 1 metadata fmt per camera.
> >> Which is fine supporting more then 1 metadata fmt is not worth
> >> the trouble IMHO.
> >
> > If we only support one metadata, we have two options for D4XX cameras:
> >
> > A) Switch to MSXU: apps that expect D4XX will not work. I think this
> > will mean breaking uAPI.
> > B) Keep D4XX and ignore MSXU: apps that work with MSXU will not work
> > with D4XX cameras. I do not love this but it will not affect my
> > usecase.
> >
> >
> > If you are ok with B) I can start the implementation. But I still
> > believe that the current option is more generic and the extra
> > complexity is not too excessive.
> >
> >
> >>
> >> This means that Laurent's remark on [PATCH v5 4/4]:
> >>
> >> "I would prefer if you could instead add a metadata format field in the
> >> uvc_device structure (I'd put it right after the info field, and while
> >> at it you could move the quirks field to that section too). The metadata
> >> format would be initialized from dev->info (when available) or set to
> >> the UVC format, and overridden when the MSXU is detected."
> >>
> >> is still relevant, which will also make patch 3/4 cleaner.
> >>
> >> The idea is to (in patch 3/4):
> >>
> >> 1. Introduce a dev->meta_format which gets initialized from dev->info->meta_format
> >> 2. Keep the quirk and if the quirk is set override dev->meta_format to
> >>    V4L2_META_FMT_UVC_MSXU_1_5 thus still allowing testing for MSXU metadata on
> >>    cameras which lack the MSXU_CONTROL_METADATA control.
> >>
> >> Doing things this way avoids the need for the complexity added to
> >> uvc_meta_v4l2_try_format() / uvc_meta_v4l2_set_format() /
> >> uvc_meta_v4l2_enum_format(). Instead the only changes necessary there now will
> >> be replacing dev->info->meta_format with dev->meta_format.
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >>
> >>
> >>
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>>  int uvc_meta_register(struct uvc_streaming *stream)
> >>>  {
> >>>       struct uvc_device *dev = stream->dev;
> >>>       struct video_device *vdev = &stream->meta.vdev;
> >>>       struct uvc_video_queue *queue = &stream->meta.queue;
> >>> +     int ret;
> >>> +
> >>> +     ret = uvc_meta_detect_msxu(dev);
> >>> +     if (ret)
> >>> +             return ret;
> >>>
> >>>       stream->meta.format = V4L2_META_FMT_UVC;
> >>>
> >>> 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, \
> >>>
> >>
> >
> >
>


--
Ricardo Ribalda

Re: [PATCH v6 4/4] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META

[Hans de Goede]

Hi,

On 16-Jun-25 11:02 PM, Ricardo Ribalda wrote:
> Hi Hans
> 
> On Mon, 16 Jun 2025 at 22:47, Hans de Goede <hansg@kernel.org> wrote:
>>
>> Hi Ricardo,
>>
>> On 16-Jun-25 5:04 PM, Ricardo Ribalda wrote:
>>> Hi Hans
>>>
>>> On Mon, 16 Jun 2025 at 16:38, Hans de Goede <hansg@kernel.org> wrote:
>>>>
>>>> Hi Ricardo,
>>>>
>>>> On 4-Jun-25 14:16, Ricardo Ribalda wrote:
>>>>> 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 | 72 ++++++++++++++++++++++++++++++++++++
>>>>>  include/linux/usb/uvc.h              |  3 ++
>>>>>  2 files changed, 75 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
>>>>> index df3f259271c675feb590c4534dad95b3b786f082..cd58427578ff413591b60abe0a210b90802dddc7 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>
>>>>> @@ -188,11 +189,82 @@ static const struct v4l2_file_operations uvc_meta_fops = {
>>>>>       .mmap = vb2_fop_mmap,
>>>>>  };
>>>>>
>>>>> +static const u8 uvc_msxu_guid[16] = UVC_GUID_MSXU_1_5;
>>>>> +
>>>>> +static struct uvc_entity *uvc_meta_find_msxu(struct uvc_device *dev)
>>>>> +{
>>>>> +     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;
>>>>
>>>> Since we set the ctrl to enable MSXU fmt metadata here, this means that
>>>> cameras which also support V4L2_META_FMT_D4XX will be switched to MSXU
>>>> metadata mode at probe() time.
>>>
>>> Not sure that I completely follow you. D4XX cameras will not be
>>> switched to MSXU, they will support MSXU and D4XX with the current
>>> patchset.
>>
>> Is MSXU an extension on top of D4XX ? If not then we need to tell
>> the camera which metadata we want in uvc_meta_v4l2_set_format()
> 
> I think I know where the miss-comunication happened :)
> 
> MSXU is indeed a superset of D4xx. D4xx metadata is formatted in MSXU.
> 
> If an app fetches D4XX and MSXU from an Intel D4xx device, they are identical.
> 
> Or in other words, if D4XX devices have MSXU_CONTROL_METADATA, they
> are probably today initialized with a value != 0.

Ok I see. But I still think the way this is handled in patch 3/4
is a bit "clunky". I think we should maybe add a 0 terminated
meta_format array to struct uvc_device and populate that during
probe with (again maybe) a uvc_device_add_metadata_fmt() helper?

Having such an array should make uvc_meta_v4l2_try_format() /
uvc_meta_v4l2_set_format() / uvc_meta_v4l2_enum_format() much
cleaner. And maybe we can even use a single implementation
for try and set?

Can you give this approach a try ?

Regards,

Hans