From: Marcus Glocker <marcus@nazgul.ch>
Subject: Re: sys/uvideo: allow multiple frames per transfer
To: "Kirill A. Korinsky" <kirill@korins.ky>
Cc: tech@openbsd.org
Date: Tue, 4 Mar 2025 23:19:29 +0100

On Tue, Mar 04, 2025 at 07:42:12PM GMT, Kirill A. Korinsky wrote:

> tech@,
> 
> Section 2.4.3.2 of UVC Specification explicitly allows multiple frames
> per one transfer and multiple transfers per one frame.
> 
> Ok?

I don't think the diff is right.  An isoc transfer (one sample) can
contain multiple frames (up to 40), but the buffer which
uvideo_mmap_getbuf() allocates is for one sample, not for one frame.

Hence we allocate a buffer for one sample, and fill that up with the
1-40 frames which a transfer can contain.  Once the sample is complete,
we queue it up through uvideo_mmap_queue() and release the buffer for
new transfers.

So I think the current logic is right.
 
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.248 uvideo.c
> --- sys/dev/usb/uvideo.c	1 Mar 2025 14:44:09 -0000	1.248
> +++ sys/dev/usb/uvideo.c	4 Mar 2025 18:39:09 -0000
> @@ -2331,12 +2331,6 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
>  	if (len == 0)
>  		goto skip;
> 
> -	if (sc->sc_mmap_flag) {
> -		if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
> -			goto skip;
> -	} else
> -		buf = sc->sc_frame_buffer.buf;
> -
>  	for (i = 0; i < sc->sc_nframes; i++) {
>  		frame = ixfer->buf + (i * sc->sc_vs_cur->psize);
>  		frame_size = ixfer->size[i];
> @@ -2345,6 +2339,12 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
>  			/* frame is empty */
>  			continue;
> 
> +		if (sc->sc_mmap_flag) {
> +			if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
> +				goto skip;
> +		} else
> +			buf = sc->sc_frame_buffer.buf;
> +
>  		sc->sc_decode_stream_header(sc, frame, frame_size, buf);
>  	}
> 
> @@ -2514,6 +2514,13 @@ uint8_t *
>  uvideo_mmap_getbuf(struct uvideo_softc *sc)
>  {
>  	int i;
> +
> +	/*
> +	 * Section 2.4.3.2 explicitly allows multiple frames per one
> +	 * transfer and multiple transfers per one frame.
> +	 */
> +	if (sc->sc_mmap_cur != NULL)
> +		return sc->sc_mmap_cur->buf;
> 
>  	if (sc->sc_mmap_count == 0 || sc->sc_mmap_buffer == NULL)
>  		panic("%s: mmap buffers not allocated", __func__);
>