On Thu, Mar 06, 2025 at 09:36:46AM GMT, Kirill A. Korinsky wrote:

> tech@,
> 
> macroses from queue.h doesn't thread safe and wraping it by mutex on
> uvideo use case seems isn't so good idea.
> 
> It has a hardcoded number of used buffers as 8 and already has a code
> which iterates against them to find the buffer which is ready for
> queueing in uvideo_mmap_getbuf.
> 
> Here I'd like to replace SIMPLEQ_* by similar logic.
> 
> It fixes two bugs:
> 
> 1. If isochronous webcam sends two frames in one transfer,
> SIMPLEQ_INSERT_TAIL adds the same element twice and creates infinity
> loop inside sc_mmap_q which will be iterating in uvideo_vs_free_frame.
> 
> 2. Prevents from calling SIMPLEQ_INSERT_TAIL and SIMPLEQ_REMOVE_HEAD for
> the same element in the queue which may lead to unexpected state.
> 
> This diff was suceffuly tested on:
>  - Elgato Facecam Pro (isochronous)
>  - Jabra PanaCast (bulk)
> 
> Ok?

I will need a bit more time to do more investigation for our current
queuing logic, since after looking initially at it, I'm not happy with
the current state.  The main thing which bothers me is that with the
current code, and also with your diff below, we only use one buffer
under "normal pressure".  At least that's what I'm seeing with my
devices here.

I'm not sure if that is the right thing to do, or when an application
asks for example to use 4 buffers, if we shouldn't always iterate
through them.
 
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.249 uvideo.c
> --- sys/dev/usb/uvideo.c	4 Mar 2025 22:59:01 -0000	1.249
> +++ sys/dev/usb/uvideo.c	5 Mar 2025 22:40:46 -0000
> @@ -69,7 +69,6 @@ struct uvideo_softc {
>  	struct uvideo_mmap			*sc_mmap_cur;
>  	uint8_t					*sc_mmap_buffer;
>  	size_t					 sc_mmap_buffer_size;
> -	q_mmap					 sc_mmap_q;
>  	int					 sc_mmap_count;
>  	int					 sc_mmap_flag;
>  
> @@ -671,8 +670,6 @@ uvideo_attach_hook(struct device *self)
>  	if (error != USBD_NORMAL_COMPLETION)
>  		return;
>  
> -	/* init mmap queue */
> -	SIMPLEQ_INIT(&sc->sc_mmap_q);
>  	sc->sc_mmap_count = 0;
>  
>  	DPRINTF(1, "uvideo_attach: doing video_attach_mi\n");
> @@ -1962,9 +1959,6 @@ uvideo_vs_free_frame(struct uvideo_softc
>  		sc->sc_mmap_buffer_size = 0;
>  	}
>  
> -	while (!SIMPLEQ_EMPTY(&sc->sc_mmap_q))
> -		SIMPLEQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames);
> -
>  	sc->sc_mmap_count = 0;
>  }
>  
> @@ -2556,7 +2551,6 @@ uvideo_mmap_queue(struct uvideo_softc *s
>  	/* queue it */
>  	sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_DONE;
>  	sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
> -	SIMPLEQ_INSERT_TAIL(&sc->sc_mmap_q, sc->sc_mmap_cur, q_frames);
>  	DPRINTF(2, "%s: %s: frame queued\n", DEVNAME(sc), __func__);
>  
>  	wakeup(sc);
> @@ -3624,32 +3618,34 @@ int
>  uvideo_dqbuf(void *v, struct v4l2_buffer *dqb)
>  {
>  	struct uvideo_softc *sc = v;
> -	struct uvideo_mmap *mmap;
> -	int error;
> +	int error, i;
>  
>  	if (dqb->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
>  	    dqb->memory != V4L2_MEMORY_MMAP)
>  		return (EINVAL);
>  
> -	if (SIMPLEQ_EMPTY(&sc->sc_mmap_q)) {
> +again:
> +	/* find a buffer which is done */
> +	for (i = 0; i < sc->sc_mmap_count; i++) {
> +		if (sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_DONE)
> +			break;
> +	}
> +
> +	if (i == sc->sc_mmap_count) {
>  		/* mmap queue is empty, block until first frame is queued */
>  		error = tsleep_nsec(sc, 0, "vid_mmap", SEC_TO_NSEC(10));
>  		if (error)
>  			return (EINVAL);
> +		goto again;
>  	}
>  
> -	mmap = SIMPLEQ_FIRST(&sc->sc_mmap_q);
> -	if (mmap == NULL)
> -		panic("uvideo_dqbuf: NULL pointer!");
> -
> -	bcopy(&mmap->v4l2_buf, dqb, sizeof(struct v4l2_buffer));
> +	bcopy(&sc->sc_mmap[i].v4l2_buf, dqb, sizeof(struct v4l2_buffer));
>  
> -	mmap->v4l2_buf.flags &= ~V4L2_BUF_FLAG_DONE;
> -	mmap->v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
> +	sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_DONE;
> +	sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
>  
>  	DPRINTF(2, "%s: %s: frame dequeued from index %d\n",
> -	    DEVNAME(sc), __func__, mmap->v4l2_buf.index);
> -	SIMPLEQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames);
> +	    DEVNAME(sc), __func__, sc->sc_mmap[i].v4l2_buf.index);
>  
>  	return (0);
>  }
>