| 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: <CANiDSCtXmXYkv3b_62iegTFOxBVrGUv9+mbioxvQvPsadwBpqg@mail.gmail.com>
Date: Mon, 16 Jun 2025 17:04:47 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Hans de Goede <hansg@...nel.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 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.
>
> 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
Powered by blists - more mailing lists