On Sat, 08 Mar 2025 21:31:06 +0100,
Marcus Glocker <marcus@nazgul.ch> wrote:
> 
> I would like to cleanup the queuing in small steps.  Hence, not
> removing SIMPLEQ yet.  Would this diff fix your issue when receiving
> multiple frames in one transfer?  It looks my devices don't do that
> very often, so difficult for me to test.
>

I just tested your diff. It's again frozes the system completley, and with
added some print I was able to prove that it is again, inside the queue.

It iterates here:

	while (!SIMPLEQ_EMPTY(&sc->sc_mmap_q))
		SIMPLEQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames);

and by my printf here I see that the cause that sc_mmap_q contains an
element which points to itself as the next one.

I really think that the root cause that ffmpeg / ffplay uses 4-6 buffers
when streams from this device, and we have insert and remove from parallel
threads to the same queue.

Thuis, if I enable debug, I can reproduce it not each time, and if I
increase uvideo_debug, I can't reproduce it at all.

All of this smells like issue from non thread-safe code.

> 
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -u -p -r1.250 uvideo.c
> --- sys/dev/usb/uvideo.c	8 Mar 2025 08:27:32 -0000	1.250
> +++ sys/dev/usb/uvideo.c	8 Mar 2025 20:19:59 -0000
> @@ -102,7 +102,7 @@ struct uvideo_softc {
>  	void					 (*sc_uplayer_intr)(void *);
>  
>  	const struct uvideo_devs		*sc_quirk;
> -	void					(*sc_decode_stream_header)
> +	int					(*sc_decode_stream_header)
>  						    (struct uvideo_softc *,
>  						    uint8_t *, int, uint8_t *);
>  };
> @@ -169,9 +169,9 @@ void		uvideo_vs_start_isoc_ixfer(struct 
>  		    struct uvideo_isoc_xfer *);
>  void		uvideo_vs_cb(struct usbd_xfer *, void *,
>  		    usbd_status);
> -void		uvideo_vs_decode_stream_header(struct uvideo_softc *,
> +int		uvideo_vs_decode_stream_header(struct uvideo_softc *,
>  		    uint8_t *, int, uint8_t *); 
> -void		uvideo_vs_decode_stream_header_isight(struct uvideo_softc *,
> +int		uvideo_vs_decode_stream_header_isight(struct uvideo_softc *,
>  		    uint8_t *, int, uint8_t *);
>  uint8_t		*uvideo_mmap_getbuf(struct uvideo_softc *);
>  void		uvideo_mmap_queue(struct uvideo_softc *, int, int);
> @@ -2346,24 +2346,27 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
>  			/* frame is empty */
>  			continue;
>  
> -		sc->sc_decode_stream_header(sc, frame, frame_size, buf);
> +		if (sc->sc_decode_stream_header(sc, frame, frame_size, buf)) {
> +			if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
> +				break;
> +		}
>  	}
>  
>  skip:	/* setup new transfer */
>  	uvideo_vs_start_isoc_ixfer(sc, ixfer);
>  }
>  
> -void
> +int
>  uvideo_vs_decode_stream_header(struct uvideo_softc *sc, uint8_t *frame,
>      int frame_size, uint8_t *buf)
>  {
>  	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
>  	struct usb_video_stream_header *sh;
> -	int sample_len;
> +	int sample_len, r = 0;
>  
>  	if (frame_size < UVIDEO_SH_MIN_LEN)
>  		/* frame too small to contain a valid stream header */
> -		return;
> +		return r;
>  
>  	sh = (struct usb_video_stream_header *)frame;
>  
> @@ -2371,7 +2374,7 @@ uvideo_vs_decode_stream_header(struct uv
>  
>  	if (sh->bLength > frame_size || sh->bLength < UVIDEO_SH_MIN_LEN)
>  		/* invalid header size */
> -		return;
> +		return r;
>  
>  	DPRINTF(2, "%s: frame_size = %d\n", DEVNAME(sc), frame_size);
>  
> @@ -2442,6 +2445,7 @@ uvideo_vs_decode_stream_header(struct uv
>  		if (sc->sc_mmap_flag) {
>  			/* mmap */
>  			uvideo_mmap_queue(sc, fb->offset, fb->error);
> +			r = 1;
>  		} else if (fb->error) {
>  			DPRINTF(1, "%s: %s: error frame, skipped!\n",
>  			    DEVNAME(sc), __func__);
> @@ -2454,6 +2458,8 @@ uvideo_vs_decode_stream_header(struct uv
>  		fb->fid = 0;
>  		fb->error = 0;
>  	}
> +
> +	return r;
>  }
>  
>  /*
> @@ -2472,12 +2478,12 @@ uvideo_vs_decode_stream_header(struct uv
>   * Sometimes the stream header is prefixed by a unknown byte.  Therefore
>   * we check for the magic value on two offsets.
>   */
> -void
> +int
>  uvideo_vs_decode_stream_header_isight(struct uvideo_softc *sc, uint8_t *frame,
>      int frame_size, uint8_t *buf)
>  {
>  	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
> -	int sample_len, header = 0;
> +	int sample_len, header = 0, r = 0;
>  	uint8_t magic[] = {
>  	    0x11, 0x22, 0x33, 0x44,
>  	    0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xfa, 0xce };
> @@ -2489,13 +2495,14 @@ uvideo_vs_decode_stream_header_isight(st
>  
>  	if (header && fb->fid == 0) {
>  		fb->fid = 1;
> -		return;
> +		return r;
>  	}
>  
>  	if (header) {
>  		if (sc->sc_mmap_flag) {
>  			/* mmap */
>  			uvideo_mmap_queue(sc, fb->offset, 0);
> +			r = 1;
>  		} else {
>  			/* read */
>  			uvideo_read(sc, fb->buf, fb->offset);
> @@ -2509,6 +2516,8 @@ uvideo_vs_decode_stream_header_isight(st
>  			fb->offset += sample_len;
>  		}
>  	}
> +
> +	return r;
>  }
>  
>  uint8_t *
> @@ -2521,7 +2530,8 @@ uvideo_mmap_getbuf(struct uvideo_softc *
>  
>  	/* find a buffer which is ready for queueing */
>  	for (i = 0; i < sc->sc_mmap_count; i++) {
> -		if (sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_QUEUED)
> +		if ((sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_QUEUED) &&
> +		    (sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_DONE) == 0)
>  			break;
>  	}
>  	if (i == sc->sc_mmap_count) {

-- 
wbr, Kirill