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