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-googHi 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, \
>
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
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, \
>>>
>>
>
>
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
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
© 2016 - 2025 Red Hat, Inc.