Message-ID: <CANiDSCvtqe8MKpb=O-=Mv28dK+g=REi7kpdr-eyAD-mLLpzQJw@mail.gmail.com>
Date: Wed, 22 Oct 2025 14:29:15 +0200
From: Ricardo Ribalda <ribalda@...omium.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Hans de Goede <hansg@...nel.org>, Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: uvcvideo: Create a specific id namespace for
 output entities

On Wed, 22 Oct 2025 at 14:09, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> On Wed, Oct 22, 2025 at 11:55:16AM +0000, Ricardo Ribalda wrote:
> > Nothing can be connected from an output entity. Which means that no
>
> s/output entity/output terminal. Same below.
>
> Did you mean s/from an/to an/ ?
>
> > other entity can reference an output entity as baSourceId.
> >
>
> Some output terminals have controls, so we need to preserve their ID.
> That's why my proposal only set the UVC_TERM_OUTPUT bit for the
> *streaming* output terminals, not for all output terminals.
>
> > Use this fact to move all the output entities to a different namespace
> > id.
> >
> > The output entities are usually named after the dev_name() of the usb
> > device, so there should not be any uAPI change from this change.
> >
> > Although with this change we can handle some id collisions
> > automagically, change the logic of uvc_alloc_new_entity() to keep
> > showing a warning when a camera has invalid descriptors. Hopefully this
> > message will help vendors fix their invalid descriptors.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> > ---
> > Hi, this patch fixes support for some devices with invalid USB
> > descriptor.
> >
> > It is orthogonal to:
> > https://lore.kernel.org/linux-media/20251021184213.GC19043@pendragon.ideasonboard.com/T/#t
> >
> > Some devices will be fixed by the other patch, other devices will be
> > fixed by this. In my opinion is worth to land both patches.
> >
> > Tested with GRANDSTREAM GUV3100 in a 6.6 kernel.
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index fb6afb8e84f00961f86fd8f840fba48d706d7a9a..40f8ae0df89e104992f5d55af3d3539dea3d146e 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -165,10 +165,14 @@ static struct uvc_entity *uvc_entity_by_reference(struct uvc_device *dev,
> >       return NULL;
> >  }
> >
> > +#define ENTITY_HARDWARE_ID(id) ((id) & ~UVC_TERM_OUTPUT)
>
> This needs a UVC_ prefix, and should probably go to uvcvideo.h. You can
> also & 0xff, as the UVC descriptors store IDs in 8-bit fields.
>
> > +
> >  static struct uvc_streaming *uvc_stream_by_id(struct uvc_device *dev, int id)
> >  {
> >       struct uvc_streaming *stream;
> >
> > +     id = ENTITY_HARDWARE_ID(id);
> > +
> >       list_for_each_entry(stream, &dev->streams, list) {
> >               if (stream->header.bTerminalLink == id)
> >                       return stream;
> > @@ -810,10 +814,12 @@ static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> >       }
> >
> >       /* Per UVC 1.1+ spec 3.7.2, the ID is unique. */
> > -     if (uvc_entity_by_id(dev, id)) {
> > -             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n", id);
> > +     if (uvc_entity_by_id(dev, ENTITY_HARDWARE_ID(id)))
> > +             dev_err(&dev->intf->dev, "Found multiple Units with ID %u\n",
> > +                     ENTITY_HARDWARE_ID(id));
>
> It's not an error anymore if there's no collision of the full 16-bit ID,
> right ? Should it be demoted to a dev_warn() ?

if it is OK with you I'd rather keep the dev_err(). If an ISP
manufacturer tests their camera in Linux I want them to really notice
that there is an error.
Besides that, I have implemented all your proposed changes.

I cannot test it until tomorrow in real hardware. But the changes are
trivial, let me know if I shall send the v2 right now or wait til it
is tested.

Regards

>
> > +
> > +     if (uvc_entity_by_id(dev, id))
> >               id = UVC_INVALID_ENTITY_ID;
> > -     }
> >
> >       extra_size = roundup(extra_size, sizeof(*entity->pads));
> >       if (num_pads)
> > @@ -969,6 +975,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> >       struct usb_host_interface *alts = dev->intf->cur_altsetting;
> >       unsigned int i, n, p, len;
> >       const char *type_name;
> > +     unsigned int id;
> >       u16 type;
> >
> >       switch (buffer[2]) {
> > @@ -1107,8 +1114,16 @@ static int uvc_parse_standard_control(struct uvc_device *dev,
> >                       return 0;
> >               }
> >
> > +             /*
> > +              * Nothing can be connected from an output terminal. To avoid
> > +              * entity-id's collisions in devices with invalid USB
> > +              * descriptors, move the output terminal id to its own
> > +              * namespace.
> > +              */
> > +             id = buffer[3] | UVC_TERM_OUTPUT;
> > +
> >               term = uvc_alloc_new_entity(dev, type | UVC_TERM_OUTPUT,
> > -                                         buffer[3], 1, 0);
> > +                                         id, 1, 0);
> >               if (IS_ERR(term))
> >                       return PTR_ERR(term);
> >
> >
> > ---
> > base-commit: ea299a2164262ff787c9d33f46049acccd120672
> > change-id: 20251022-uvc-grandstream-laurent-3f9abb8a0d5b
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Powered by blists - more mailing lists