diff mbox series

[RESEND,v2,2/8] media: uvc: Allow quirking by entity guid

Message ID 20220920-resend-hwtimestamp-v2-2-0d7978a817cc@chromium.org
State New
Headers
Series uvcvideo: Fixes for hw timestamping |

Commit Message

Ricardo Ribalda Dec. 2, 2022, 5:02 p.m. UTC 
  When an IP is shared by multiple devices its erratas will be shared by
all of them. Instead of creating a long list of device quirks, or
waiting for the users to report errors in their hardware lets add a
routine to add quirks based on the entity guid.

Tested-by: HungNien Chen <hn.chen@sunplusit.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/usb/uvc/uvc_driver.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Laurent Pinchart Dec. 30, 2022, 1:40 p.m. UTC | #1 
Hi Ricardo,

Thank you for the patch.


On Fri, Dec 02, 2022 at 06:02:42PM +0100, Ricardo Ribalda wrote:
> When an IP is shared by multiple devices its erratas will be shared by
> all of them. Instead of creating a long list of device quirks, or
> waiting for the users to report errors in their hardware lets add a
> routine to add quirks based on the entity guid.

I'm not thrilled by this. An entity is not an "IP". Quirks are needed to
handle issues with particular firmware versions on particular devices.
The same entity GUID can be used by different devices running different
firmware versions, some that would require a quirk and some that
wouldn't.

> Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 9c05776f11d1..c63ecfd4617d 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1493,6 +1493,28 @@ static int uvc_parse_control(struct uvc_device *dev)
>  	return 0;
>  }
>  
> +static const struct uvc_entity_quirk {
> +	u8 guid[16];
> +	u32 quirks;
> +} uvc_entity_quirk[] = {
> +};
> +
> +static void uvc_entity_quirks(struct uvc_device *dev)
> +{
> +	struct uvc_entity *entity;
> +	int i;

unsigned int

> +
> +	list_for_each_entry(entity, &dev->entities, list) {
> +		for (i = 0; i < ARRAY_SIZE(uvc_entity_quirk); i++) {
> +			if (memcmp(entity->guid, uvc_entity_quirk[i].guid,
> +				   sizeof(entity->guid)) == 0) {
> +				dev->quirks |= uvc_entity_quirk[i].quirks;
> +				break;
> +			}
> +		}
> +	}
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Privacy GPIO
>   */
> @@ -2452,6 +2474,9 @@ static int uvc_probe(struct usb_interface *intf,
>  		goto error;
>  	}
>  
> +	/* Apply entity based quirks */
> +	uvc_entity_quirks(dev);
> +
>  	dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
>  		 dev->uvc_version >> 8, dev->uvc_version & 0xff,
>  		 udev->product ? udev->product : "<unnamed>",
>
Ricardo Ribalda Jan. 3, 2023, 3:40 p.m. UTC | #2 
Hi Laurent

On Fri, 30 Dec 2022 at 14:40, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Ricardo,
>
> Thank you for the patch.
>
>
> On Fri, Dec 02, 2022 at 06:02:42PM +0100, Ricardo Ribalda wrote:
> > When an IP is shared by multiple devices its erratas will be shared by
> > all of them. Instead of creating a long list of device quirks, or
> > waiting for the users to report errors in their hardware lets add a
> > routine to add quirks based on the entity guid.
>
> I'm not thrilled by this. An entity is not an "IP". Quirks are needed to
> handle issues with particular firmware versions on particular devices.
> The same entity GUID can be used by different devices running different
> firmware versions, some that would require a quirk and some that
> wouldn't.

Unfortunately there are ISPs that do not support firmware upgrading
that have an error on their firmware (or in this particular case a
different interpretation of the standard). Those ISPs are mounted in
different boards with a VID:PID that is chosen by the module
manufacturer.
In those cases we cannot get a list of the devices that are broken, we
could only get a sublist that we will keep updating indefinitely as
users keep reporting bugs (if they do so).

We are lucky enough that SunplusIT has been very active and provided
us a way to detect what hardware requires quirking. In those
situations where the vendor is on board and there is no upgrade
mechanism I think that this is a good compromise.

>
> > Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 9c05776f11d1..c63ecfd4617d 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -1493,6 +1493,28 @@ static int uvc_parse_control(struct uvc_device *dev)
> >       return 0;
> >  }
> >
> > +static const struct uvc_entity_quirk {
> > +     u8 guid[16];
> > +     u32 quirks;
> > +} uvc_entity_quirk[] = {
> > +};
> > +
> > +static void uvc_entity_quirks(struct uvc_device *dev)
> > +{
> > +     struct uvc_entity *entity;
> > +     int i;
>
> unsigned int
>
> > +
> > +     list_for_each_entry(entity, &dev->entities, list) {
> > +             for (i = 0; i < ARRAY_SIZE(uvc_entity_quirk); i++) {
> > +                     if (memcmp(entity->guid, uvc_entity_quirk[i].guid,
> > +                                sizeof(entity->guid)) == 0) {
> > +                             dev->quirks |= uvc_entity_quirk[i].quirks;
> > +                             break;
> > +                     }
> > +             }
> > +     }
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Privacy GPIO
> >   */
> > @@ -2452,6 +2474,9 @@ static int uvc_probe(struct usb_interface *intf,
> >               goto error;
> >       }
> >
> > +     /* Apply entity based quirks */
> > +     uvc_entity_quirks(dev);
> > +
> >       dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
> >                dev->uvc_version >> 8, dev->uvc_version & 0xff,
> >                udev->product ? udev->product : "<unnamed>",
> >
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Jan. 3, 2023, 9:16 p.m. UTC | #3 
Hi Ricardo,

On Tue, Jan 03, 2023 at 04:40:48PM +0100, Ricardo Ribalda wrote:
> On Fri, 30 Dec 2022 at 14:40, Laurent Pinchart wrote:
> > On Fri, Dec 02, 2022 at 06:02:42PM +0100, Ricardo Ribalda wrote:
> > > When an IP is shared by multiple devices its erratas will be shared by
> > > all of them. Instead of creating a long list of device quirks, or
> > > waiting for the users to report errors in their hardware lets add a
> > > routine to add quirks based on the entity guid.
> >
> > I'm not thrilled by this. An entity is not an "IP". Quirks are needed to
> > handle issues with particular firmware versions on particular devices.
> > The same entity GUID can be used by different devices running different
> > firmware versions, some that would require a quirk and some that
> > wouldn't.
> 
> Unfortunately there are ISPs that do not support firmware upgrading
> that have an error on their firmware (or in this particular case a
> different interpretation of the standard).

I recall we've discussed that. I'll stick to the non politically correct
interpretation of the issue, and call it a bug :-)

> Those ISPs are mounted in
> different boards with a VID:PID that is chosen by the module
> manufacturer.
> In those cases we cannot get a list of the devices that are broken, we
> could only get a sublist that we will keep updating indefinitely as
> users keep reporting bugs (if they do so).
> 
> We are lucky enough that SunplusIT has been very active and provided
> us a way to detect what hardware requires quirking.

Is there a guarantee that none of the newer firmware versions that do
not exhibit this bug will *not* use the same XU GUID ?

> In those
> situations where the vendor is on board and there is no upgrade
> mechanism I think that this is a good compromise.

What I'm interested in is how to prevent this kind of issues in the
future. HungNien, would you be interested in engaging with us in order
to test future ISP firmwares against Linux ? I do appreciate that you
have supported Ricardo with handling this issue. We could work on a
publicly available UVC compliance test suite. You can reply about this
privately if you would rather not discuss this topic on public mailing
lists.

> > > Tested-by: HungNien Chen <hn.chen@sunplusit.com>
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/usb/uvc/uvc_driver.c | 25 +++++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > > index 9c05776f11d1..c63ecfd4617d 100644
> > > --- a/drivers/media/usb/uvc/uvc_driver.c
> > > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > > @@ -1493,6 +1493,28 @@ static int uvc_parse_control(struct uvc_device *dev)
> > >       return 0;
> > >  }
> > >
> > > +static const struct uvc_entity_quirk {
> > > +     u8 guid[16];
> > > +     u32 quirks;
> > > +} uvc_entity_quirk[] = {
> > > +};
> > > +
> > > +static void uvc_entity_quirks(struct uvc_device *dev)
> > > +{
> > > +     struct uvc_entity *entity;
> > > +     int i;
> >
> > unsigned int
> >
> > > +
> > > +     list_for_each_entry(entity, &dev->entities, list) {
> > > +             for (i = 0; i < ARRAY_SIZE(uvc_entity_quirk); i++) {
> > > +                     if (memcmp(entity->guid, uvc_entity_quirk[i].guid,
> > > +                                sizeof(entity->guid)) == 0) {
> > > +                             dev->quirks |= uvc_entity_quirk[i].quirks;
> > > +                             break;
> > > +                     }
> > > +             }
> > > +     }
> > > +}
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * Privacy GPIO
> > >   */
> > > @@ -2452,6 +2474,9 @@ static int uvc_probe(struct usb_interface *intf,
> > >               goto error;
> > >       }
> > >
> > > +     /* Apply entity based quirks */
> > > +     uvc_entity_quirks(dev);
> > > +
> > >       dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
> > >                dev->uvc_version >> 8, dev->uvc_version & 0xff,
> > >                udev->product ? udev->product : "<unnamed>",
diff mbox series

Patch

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 9c05776f11d1..c63ecfd4617d 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -1493,6 +1493,28 @@  static int uvc_parse_control(struct uvc_device *dev)
 	return 0;
 }
 
+static const struct uvc_entity_quirk {
+	u8 guid[16];
+	u32 quirks;
+} uvc_entity_quirk[] = {
+};
+
+static void uvc_entity_quirks(struct uvc_device *dev)
+{
+	struct uvc_entity *entity;
+	int i;
+
+	list_for_each_entry(entity, &dev->entities, list) {
+		for (i = 0; i < ARRAY_SIZE(uvc_entity_quirk); i++) {
+			if (memcmp(entity->guid, uvc_entity_quirk[i].guid,
+				   sizeof(entity->guid)) == 0) {
+				dev->quirks |= uvc_entity_quirk[i].quirks;
+				break;
+			}
+		}
+	}
+}
+
 /* -----------------------------------------------------------------------------
  * Privacy GPIO
  */
@@ -2452,6 +2474,9 @@  static int uvc_probe(struct usb_interface *intf,
 		goto error;
 	}
 
+	/* Apply entity based quirks */
+	uvc_entity_quirks(dev);
+
 	dev_info(&dev->udev->dev, "Found UVC %u.%02x device %s (%04x:%04x)\n",
 		 dev->uvc_version >> 8, dev->uvc_version & 0xff,
 		 udev->product ? udev->product : "<unnamed>",