| 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: <CANiDSCv-U_suiOQjF5iaPzBw_mZ9uxPDNqyr4CAi5WtWGEX0ng@mail.gmail.com>
Date: Mon, 26 May 2025 16:17:25 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Hans de Goede <hdegoede@...hat.com>, Mauro Carvalho Chehab <mchehab@...nel.org>,
Guennadi Liakhovetski <guennadi.liakhovetski@...el.com>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 4/4] media: uvcvideo: Auto-set UVC_QUIRK_MSXU_META
Hi Laurent
On Mon, 26 May 2025 at 16:02, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
> On Fri, Apr 04, 2025 at 06:37:37AM +0000, Ricardo Ribalda wrote:
> > If the camera supports the MSXU_CONTROL_METADATA control, auto set the
> > MSXU_META quirk.
>
> Ah, that's why you introduce a quirk in 3/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.
I assume that there will be plenty of devices that do not have the
MSXU_CONTROL_METADATA control and have metadata.
With a quirk, users can try enabling the metadata without rebuilding
their kernel. They can report it to the mailing list and they or we
improve the driver. The contribution barrier is lower.
Another issue of the dev->info would be that D4XX devices will not
support the MSXU_META format. I'd expect userspace to prefer
supporting one format instead of 2.
>
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > ---
> > drivers/media/usb/uvc/uvc_metadata.c | 54 ++++++++++++++++++++++++++++++++++++
> > include/linux/usb/uvc.h | 3 ++
> > 2 files changed, 57 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_metadata.c b/drivers/media/usb/uvc/uvc_metadata.c
> > index fe2678fc795d7fd5a64e8113199012f34c419176..776d280f34afad515594a873acf075acf0438304 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>
> > @@ -187,11 +188,64 @@ 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;
> > +
> > +#define MSXU_CONTROL_METADATA 0x9
> > +static int uvc_enable_msxu(struct uvc_device *dev)
>
> uvc_meta_detect_msxu()
>
> > +{
> > + u32 *data __free(kfree) = NULL;
> > + struct uvc_entity *entity;
> > +
> > + list_for_each_entry(entity, &dev->entities, list) {
> > + int ret;
> > +
> > + if (memcmp(entity->guid, uvc_msxu_guid, sizeof(entity->guid)))
> > + continue;
> > +
> > + if (!data)
> > + data = kmalloc(sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
>
> A comment here to explain how the control operates would be useful.
> Reading the code I assume that GET_MAX will indicate if the MS metadata
> format is supported by reporting a value different than 0 (is it always
> 1, or can it take other values), and SET_CUR will enable metadata
> generation. I suppose the first GET_CUR call catches the case where it
> has already been enabled, are there also cameras where it can't be
> disabled, and where SET_CUR would fail ?
>
> > + ret = uvc_query_ctrl(dev, UVC_GET_CUR, entity->id,
> > + dev->intfnum, MSXU_CONTROL_METADATA,
> > + data, sizeof(*data));
> > + if (ret)
> > + continue;
>
> Can there be multiple MSXU instances, or can you break here (and below)
> ?
I think it is safe to break. Thanks :)
>
> > +
> > + if (*data) {
> > + dev->quirks |= UVC_QUIRK_MSXU_META;
> > + return 0;
> > + }
> > +
> > + ret = uvc_query_ctrl(dev, UVC_GET_MAX, entity->id,
> > + dev->intfnum, MSXU_CONTROL_METADATA,
> > + data, sizeof(*data));
> > + if (ret || !*data)
> > + continue;
> > +
> > + 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;
> > + }
> > + }
> > +
> > + 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_enable_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.49.0.504.g3bcea36a83-goog
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Ricardo Ribalda
Powered by blists - more mailing lists