From: Marcus Glocker Subject: Re: sys/uvideo: add quriks for Elgato devices To: "Kirill A. Korinsky" Cc: OpenBSD tech , Laurence Tratt Date: Wed, 5 Feb 2025 21:03:52 +0100 On Tue, Feb 04, 2025 at 09:46:25AM GMT, Kirill A. Korinsky wrote: > tech@, > > I'd like to "backport" from Linux one more qurik which introduced support of > Elgato devices. Such devices had a bug which reports bFormatIndex in wrong > place. Almost the same trick is used by Linux: > https://github.com/torvalds/linux/blob/v6.13/drivers/media/usb/uvc/uvc_video.c#L144-L166 > > It was tested by Laurence (cc'ed) who has access to such device. > > Ok? Comment inline. > Index: sys/dev/usb/usbdevs > =================================================================== > RCS file: /home/cvs/src/sys/dev/usb/usbdevs,v > diff -u -p -r1.773 usbdevs > --- sys/dev/usb/usbdevs 14 Dec 2024 10:02:47 -0000 1.773 > +++ sys/dev/usb/usbdevs 20 Jan 2025 00:09:43 -0000 > @@ -692,6 +692,7 @@ vendor DATAAPEX 0xdaae DataApex > vendor EVOLUTION 0xdeee Evolution Robotics > vendor EMPIA 0xeb1a eMPIA Technology > vendor HP2 0xf003 Hewlett Packard > +vendor ELGATO 0x0fd9 Elgato > > /* > * List of known products. Grouped by vendor. > @@ -1783,6 +1784,9 @@ product EICON DIVA852 0x4905 Diva 852 I > /* EIZO products */ > product EIZO HUB 0x0000 hub > product EIZO MONITOR 0x0001 monitor > + > +/* Elgato products */ > +product ELGATO HD60 0x006a Game Capture HD60 S+ > > /* Elan Microelectronics products */ > product ELAN BARCODE 0x0001 Barcode Scanner > Index: sys/dev/usb/uvideo.c > =================================================================== > RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v > diff -u -p -r1.236 uvideo.c > --- sys/dev/usb/uvideo.c 16 Jan 2025 22:58:19 -0000 1.236 > +++ sys/dev/usb/uvideo.c 31 Jan 2025 00:33:54 -0000 > @@ -294,10 +294,11 @@ const struct video_hw_if uvideo_hw_if = > * Devices which either fail to declare themselves as UICLASS_VIDEO, > * or which need firmware uploads or other quirk handling later on. > */ > -#define UVIDEO_FLAG_ISIGHT_STREAM_HEADER 0x1 > -#define UVIDEO_FLAG_REATTACH 0x2 > -#define UVIDEO_FLAG_VENDOR_CLASS 0x4 > -#define UVIDEO_FLAG_NOATTACH 0x8 > +#define UVIDEO_FLAG_ISIGHT_STREAM_HEADER 0x01 > +#define UVIDEO_FLAG_REATTACH 0x02 > +#define UVIDEO_FLAG_VENDOR_CLASS 0x04 > +#define UVIDEO_FLAG_NOATTACH 0x08 > +#define UVIDEO_FLAG_FORMAT_INDEX_IN_BMHINT 0x10 > const struct uvideo_devs { > struct usb_devno uv_dev; > char *ucode_name; > @@ -379,6 +380,12 @@ const struct uvideo_devs { > NULL, > UVIDEO_FLAG_NOATTACH > }, > + { /* Has incorrect control response */ > + { USB_VENDOR_ELGATO, USB_PRODUCT_ELGATO_HD60 }, > + NULL, > + NULL, > + UVIDEO_FLAG_FORMAT_INDEX_IN_BMHINT > + }, > }; > #define uvideo_lookup(v, p) \ > ((const struct uvideo_devs *)usb_lookup(uvideo_devs, v, p)) > @@ -1725,6 +1732,13 @@ uvideo_vs_get_probe(struct uvideo_softc > DEVNAME(sc), usbd_errstr(error)); > return (USBD_INVAL); > } > + > + if (sc->sc_quirk && > + sc->sc_quirk->flags & UVIDEO_FLAG_FORMAT_INDEX_IN_BMHINT) { According to the Linux drivers comment, the issue seems to exist with the latest Elgato Cam Link 4K firmware, which indicates that the issue might not exist with older firmware's. Therefore, they also check if the second byte of bmHint contains a value, since with a correct bmHint field this byte should be always zero, according the UVC specs (reserved bits). Should we do the same? if (sc->sc_quirk && sc->sc_quirk->flags & UVIDEO_FLAG_FORMAT_INDEX_IN_BMHINT && UGETW(pc->bmHint) > 255) I think a comment inside of the quirks 'if' branch would be also helpful to understand what's going on. Something like /* * The Elgato Cam Link 4K returns an invalid bmHint response, which * contains the bFormatIndex in the second byte. But it should be * always zero since it's a reserved bit field. */ maybe. > + pc->bFormatIndex = UGETW(pc->bmHint) >> 8; > + USETW(pc->bmHint, 1); > + } > + > DPRINTF(1, "%s: GET probe request successfully\n", DEVNAME(sc)); > > DPRINTF(1, "bmHint=0x%02x\n", UGETW(pc->bmHint)); > > -- > wbr, Kirill