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 23:42:04 +0200

On Tue, Aug 12, 2025 at 11:17:43PM +0200, Kirill A. Korinsky wrote:

> On Tue, 12 Aug 2025 22:48:32 +0200,
> Marcus Glocker <marcus@nazgul.ch> wrote:
> > 
> > On Tue, Aug 12, 2025 at 06:30:50PM +0200, Kirill A. Korinsky wrote:
> > 
> > > From antoher hand clean it up, to avoid garbage from some devices seems not
> > > that bad idea.
> > 
> > Well, fine for me.  Just wanted to understand if there is a specific
> > reason for it.  But could you please tweak it a bit:
> > 
> > > > > +	/* Different UVC version defines different length of a probe */
> > 
> > A bit more clear comment would be nice, so we still understand this
> > when reading it in two years.  Something like:
> > 
> > /*
> >  * Different UVC version defines different length of a probe;
> >  * Make sure that the unused porition of probe_data remains zero.
> >  */
> > 
> > > > > +	bzero(probe_data + actlen,
> > > > > +	    sizeof(struct usb_video_probe_commit ) - actlen);
> > 
> > Can you please remove the space here?           ^
> 
> 
> Thanks for tweaks.
> 
> Like this?

Yes, thanks.  I made a type in my comment addition, see in-line.

With that fixed ok mglocker@

 
> 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	12 Aug 2025 21:15:53 -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,21 @@ 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,
> +	    USBD_SHORT_XFER_OK, &actlen, USBD_DEFAULT_TIMEOUT);
> +	if (error != USBD_NORMAL_COMPLETION) {
>  		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;
> +	 * Make sure that the unused porition of probe_data remains zero.

portion				     ^

> +	 */
> +	bzero(probe_data + actlen,
> +	    sizeof(struct usb_video_probe_commit) - actlen);
> +
>  	if (sc->sc_quirk &&
>  	    sc->sc_quirk->flags & UVIDEO_FLAG_FORMAT_INDEX_IN_BMHINT &&
>  	    UGETW(pc->bmHint) > 255) {
> @@ -2065,7 +2074,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 +2091,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;
>  
>  /*
>