From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sys/uvideo: support pre UVC 1.5 devices
To: "Kirill A. Korinsky" <kirill@korins.ky>
Cc: tech@openbsd.org
Date: Tue, 12 Aug 2025 13:22:46 +0200

On Fri, Aug 08, 2025 at 07:34:29PM +0200, Kirill A. Korinsky wrote:

> tech@,
> 
> my call for testing USB cameras leads to a bug report that old devices
> may not work.
> 
> Why? Because UVC has a few different definition of probe structure.
> 
> Basic probe contains 26 bytes, and extended one 34 bytes. And UVC 1.5
> extends it to 48 bytes.
> 
> When old device is attached, usbd_do_request() returns USBD_SHORT_XFER,
> and we discard this device.
> 
> Instead I track actual len, reset not used memory and forward it to a
> consumer, because it may know how to work with it.
> 
> This diff doesn't introduce any regression on my devices.
> 
> It was reported and tested by Daniel Konnoff on his Quanta ACER HD.
> 
> Ok?

See feedback in-line.
 
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.260 uvideo.c
> --- sys/dev/usb/uvideo.c	3 Aug 2025 20:00:11 -0000	1.260
> +++ sys/dev/usb/uvideo.c	8 Aug 2025 12:41:35 -0000
> @@ -2032,6 +2032,7 @@ uvideo_vs_get_probe(struct uvideo_softc 
>  	usb_device_request_t req;
>  	usbd_status error;
>  	uint16_t tmp;
> +	int actlen;
>  	struct usb_video_probe_commit *pc;
>  
>  	req.bmRequestType = UVIDEO_GET_IF;
> @@ -2044,13 +2045,18 @@ uvideo_vs_get_probe(struct uvideo_softc 
>  
>  	pc = (struct usb_video_probe_commit *)probe_data;
>  
> -	error = usbd_do_request(sc->sc_udev, &req, probe_data);
> -	if (error) {
> +	error = usbd_do_request_flags(sc->sc_udev, &req, probe_data, 0,
> +	    &actlen, USBD_DEFAULT_TIMEOUT);

I'm OK if we accept short transfers here, but have you tried to use
USBD_SHORT_XFER_OK as an usbd_do_request_flags() flag?  Then it shouldn't
be required to make a specific error check on it.

> +	if (error != USBD_NORMAL_COMPLETION && error != USBD_SHORT_XFER) {
>  		printf("%s: could not GET probe request: %s\n",
>  		    DEVNAME(sc), usbd_errstr(error));
>  		return (USBD_INVAL);
>  	}
>  
> +	/* Different UVC version defines different length of a probe */
> +	bzero(probe_data + actlen,
> +	    sizeof(struct usb_video_probe_commit ) - actlen);
> +

Why is it required to bzero probe_data here?  There is one use of
uvideo_vs_get_probe() currently, and that always bzero's probe_data
before:

        /* get probe */
        bzero(probe_data, sizeof(probe_data));
        error = uvideo_vs_get_probe(sc, probe_data, GET_CUR);

On a short transfer I wouldn't expect the rest of probe_data to contain
garbage, but I have no device here to test that.

>  	if (sc->sc_quirk &&
>  	    sc->sc_quirk->flags & UVIDEO_FLAG_FORMAT_INDEX_IN_BMHINT &&
>  	    UGETW(pc->bmHint) > 255) {
> @@ -2065,7 +2071,8 @@ uvideo_vs_get_probe(struct uvideo_softc 
>  		USETW(pc->bmHint, 1);
>  	}
>  
> -	DPRINTF(1, "%s: GET probe request successfully\n", DEVNAME(sc));
> +	DPRINTF(1, "%s: GET probe request successfully, length: %d\n",
> +	    DEVNAME(sc), actlen);
>  
>  	DPRINTF(1, "bmHint=0x%02x\n", UGETW(pc->bmHint));
>  	DPRINTF(1, "bFormatIndex=0x%02x\n", pc->bFormatIndex);
> @@ -2081,6 +2088,26 @@ uvideo_vs_get_probe(struct uvideo_softc 
>  	    UGETDW(pc->dwMaxVideoFrameSize));
>  	DPRINTF(1, "dwMaxPayloadTransferSize=%d (bytes)\n",
>  	    UGETDW(pc->dwMaxPayloadTransferSize));
> +	DPRINTF(1, "dwClockFrequency=%d (Hz)\n",
> +	    UGETDW(pc->dwClockFrequency));
> +	DPRINTF(1, "bmFramingInfo=0x%02x\n", pc->bmFramingInfo);
> +	DPRINTF(1, "bPreferedVersion=%d\n", pc->bPreferedVersion);
> +	DPRINTF(1, "bMinVersion=%d\n", pc->bMinVersion);
> +	DPRINTF(1, "bMaxVersion=%d\n", pc->bMaxVersion);
> +	DPRINTF(1, "bUsage=%d\n", pc->bUsage);
> +	DPRINTF(1, "bBitDepthLuma=%d\n", pc->bBitDepthLuma);
> +	DPRINTF(1, "bmSettings=0x%02x\n", pc->bmSettings);
> +	DPRINTF(1, "bMaxNumberOfRefFramesPlus1=%d\n",
> +	    pc->bMaxNumberOfRefFramesPlus1);
> +	DPRINTF(1, "bmRateControlModes=%d\n",
> +	    UGETW(pc->bmRateControlModes));
> +#ifdef UVIDEO_DEBUG
> +	if (1 <= uvideo_debug) {
> +		printf("bmLayoutPerStream=0x");
> +		uvideo_hexdump(pc->bmLayoutPerStream,
> +		    sizeof(pc->bmLayoutPerStream), 1);
> +	}
> +#endif
>  
>  	return (USBD_NORMAL_COMPLETION);
>  }
> Index: sys/dev/usb/uvideo.h
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.h,v
> diff -u -p -r1.67 uvideo.h
> --- sys/dev/usb/uvideo.h	3 Aug 2025 20:00:11 -0000	1.67
> +++ sys/dev/usb/uvideo.h	8 Aug 2025 12:18:43 -0000
> @@ -275,7 +275,7 @@ struct usb_video_color_matching_descr {
>  	uByte	bMatrixCoefficients;
>  } __packed;
>  
> -/* Table 4-47: Video Probe and Commit Controls */
> +/* Table 4-75: Video Probe and Commit Controls */
>  struct usb_video_probe_commit {
>  	uWord	bmHint;
>  	uByte	bFormatIndex;
> @@ -293,6 +293,12 @@ struct usb_video_probe_commit {
>  	uByte	bPreferedVersion;
>  	uByte	bMinVersion;
>  	uByte	bMaxVersion;
> +	uByte	bUsage;
> +	uByte	bBitDepthLuma;
> +	uByte	bmSettings;
> +	uByte	bMaxNumberOfRefFramesPlus1;
> +	uWord	bmRateControlModes;
> +	uByte	bmLayoutPerStream[8];
>  } __packed;
>  
>  /*