From: Marcus Glocker Subject: Re: sys/uvideo: avoid using queue.h To: "Kirill A. Korinsky" Cc: tech@openbsd.org Date: Mon, 17 Mar 2025 21:58:50 +0100 On Mon, Mar 17, 2025 at 08:07:56PM GMT, Marcus Glocker wrote: > > Well, I've just tried your diff and it doesn't work. Idea was to perform a > > stress test of your and my diff against uvideo.c,v 1.252 but the system had > > crashed before ffmpeg gets any frame from the device. OCRed stacktrace: > > > > panic: attempt to access user address 0x20 from EL1 > > Stopped at panic+0x140: cmp w21, #0x0 > > TID PID UID PRFLAGS PC PFLAGS CPU COMMAND > > 236976 22771 1000 0x3 0x4000000 1 ffmpeg > > db_enter() at panic+0x13c > > panic() at kdata_abort+0x180 > > do_el0_sync() at handle_el1h_sync+0x68 > > handle_el1h_sync() at uvideo_vs_cb+0xc0 > > uvideo_vs_cb() at usb_transfer_complete+0x220 > > usb_transfer_complete() at xhci_event_dequeue+0x10c > > xhci_event_dequeue() at xhci_softintr+0x34 > > https://www.openbsd.org/ddb.html describes the minimum info required in bug > > reports. Insufficient info makes it difficult to find and fix bugs. > > ddb{0}> > > > > The test was to use Elgato webcam to stream 4K 60 fps stream to null for 100 > > seconds as ffmpeg command: > > > > ffmpeg -input_format mjpeg -video_size 3840x2160 -framerate 60 -i /dev/video0 -t 100 -f null - > > > > Anyway, the same test works well on the same machine with the same device > > with and without my diff. > > Ah crap, I had a left over in that diff from the first version, which > most probably caused this panic. Is this better by any chance? OK. I think we're getting closer. I did some more analysis of this "detach cam during streaming and system freezes" issue and came to the following conclusion: - It happens with the -current code. - It happens with your "own queue implementation" diff. - It happens with my improved frame buffer utilization and mutex diff. - It happens with my improved frame buffer utilization without mutex diff (new, attached). The only diff which helps to fix the detach-freeze issue is this diff from you: [1]. For me this means: - SIMPLEQ isn't the issue. - Missing mutex locking isn't the issue. If the following diff works for your devices, I would propose to only get this in, to fix the frame buffer utilization in uvideo. Then as a next step we review [1], and hopefully can bring that in next to fix the detach-freeze issue. [1] https://marc.info/?l=openbsd-tech&m=174180759929893&w=2 Index: sys/dev/usb/uvideo.c =================================================================== RCS file: /cvs/src/sys/dev/usb/uvideo.c,v diff -u -p -u -p -r1.252 uvideo.c --- sys/dev/usb/uvideo.c 12 Mar 2025 14:08:51 -0000 1.252 +++ sys/dev/usb/uvideo.c 17 Mar 2025 20:35:40 -0000 @@ -68,6 +68,7 @@ struct uvideo_softc { struct uvideo_mmap sc_mmap[UVIDEO_MAX_BUFFERS]; uint8_t *sc_mmap_buffer; size_t sc_mmap_buffer_size; + int sc_mmap_buffer_idx; q_mmap sc_mmap_q; int sc_mmap_count; int sc_mmap_flag; @@ -172,6 +173,7 @@ void uvideo_vs_decode_stream_header(str uint8_t *, int); void uvideo_vs_decode_stream_header_isight(struct uvideo_softc *, uint8_t *, int); +int uvideo_get_buf(struct uvideo_softc *); void uvideo_mmap_queue(struct uvideo_softc *, uint8_t *, int, int); void uvideo_read(struct uvideo_softc *, uint8_t *, int); usbd_status uvideo_usb_control(struct uvideo_softc *, uint8_t, uint8_t, @@ -2498,6 +2500,30 @@ uvideo_vs_decode_stream_header_isight(st } } +int +uvideo_get_buf(struct uvideo_softc *sc) +{ + int i, idx = sc->sc_mmap_buffer_idx; + + /* find a buffer which is ready for queueing */ + for (i = 0; i < sc->sc_mmap_count; i++) { + if (sc->sc_mmap[sc->sc_mmap_buffer_idx].v4l2_buf.flags & + V4L2_BUF_FLAG_QUEUED) { + idx = sc->sc_mmap_buffer_idx; + if (++sc->sc_mmap_buffer_idx == sc->sc_mmap_count) + sc->sc_mmap_buffer_idx = 0; + break; + } + if (++sc->sc_mmap_buffer_idx == sc->sc_mmap_count) + sc->sc_mmap_buffer_idx = 0; + } + + if (i == sc->sc_mmap_count) + return -1; + + return idx; +} + void uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err) { @@ -2507,11 +2533,8 @@ uvideo_mmap_queue(struct uvideo_softc *s panic("%s: mmap buffers not allocated", __func__); /* 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) - break; - } - if (i == sc->sc_mmap_count) { + i = uvideo_get_buf(sc); + if (i == -1) { DPRINTF(1, "%s: %s: mmap queue is full!\n", DEVNAME(sc), __func__); return; @@ -3552,6 +3575,8 @@ uvideo_reqbufs(void *v, struct v4l2_requ sc->sc_mmap[i].v4l2_buf.m.offset, sc->sc_mmap[i].v4l2_buf.length); } + + sc->sc_mmap_buffer_idx = 0; /* tell how many buffers we have really allocated */ rb->count = sc->sc_mmap_count;