From: Kirill A. Korinsky Subject: Re: sys/uvideo: avoid using queue.h To: Marcus Glocker Cc: tech@openbsd.org Date: Sun, 09 Mar 2025 00:26:08 +0100 On Sat, 08 Mar 2025 21:31:06 +0100, Marcus Glocker 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