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

[Hans de Goede <hansg@kernel.org>]

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