| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9233836e-c50e-4118-95f6-0be02dc0c45e@kernel.org>
Date: Tue, 17 Jun 2025 11:33:14 +0200
From: Hans de Goede <hansg@...nel.org>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Hans de Goede <hdegoede@...hat.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Guennadi Liakhovetski <guennadi.liakhovetski@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-usb@...r.kernel.org
Subject: Re: [PATCH v6 4/4] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META
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@...nel.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@...nel.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@...nel.org>
>>>>> Signed-off-by: Ricardo Ribalda <ribalda@...omium.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
Powered by blists - more mailing lists