From: Marcus Glocker Subject: Re: sys/uvideo: avoid using queue.h To: "Kirill A. Korinsky" Cc: tech@openbsd.org Date: Tue, 18 Mar 2025 14:11:37 +0100 On Tue, Mar 18, 2025 at 12:50:38AM GMT, Kirill A. Korinsky wrote: > > 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 > > > > Both your diff doesn't crash system, so, I was able to make a performance > testing of all 3 diffs. ffmpeg can read with ~35 fps from all three of them > with 3840x2160 and 60 fps when I stream as 1920x1080. THe bottel neck is > ffmpeg because it had hit almost 100% CPU on large stream. Great! > Regarding freezing on detach. I was able to reproduce it with your latest > diff where you improved buffer utilization (like on snapshot), and with my > diff and your mutex diff the system crashes with OCRed stacktrace: > > panic: attempt to access user address 0x62d98 from ELI > Stopped at panic+0x114f cmp w21, #0x0 > TID PID UID FLAGS PFLAGS CPU COMMAND > *1076599 3134 35 0x12 0 0 xorg > 338189 39674 0 0x10000 0 3 hotplugd > > db_enter() at panic+0x1c > panic() at kdata_abort+0x180 > do_el0_sync() at handle_el1h_sync+0x68 > handle_el1h_sync() at uvideo_vs_decode_stream_header+0x9c > uvideo_vs_decode_stream_header() at uvideo_vs_cb+0x94 > uvideo_vs_cb() at usb_transfer_complete+0x220 > usb_transfer_complete() at xhci_event_dequeue+0x10c > > Ok, next. Adding only my diff with device_lookup changes the crash to: > > panic: uvm_fault failed: fffff80002fd48 esr 96000004 far dead07fdeadbf37 > Stopped at panic+0x140: cmp w21, #0x0 > TID PID UID FLAGS PFLAGS CPU COMMAND > 304169 16111 35 0x12 0 7 xorg > > db_enter() at panic+0x13c > panic() at kdata_abort+0x170 > kdata_abort() at handle_el1h_sync+0x68 > handle_el1h_sync() at usbd_free_xfer+0x58 > usbd_free_xfer() at uvideo_vs_free_isoc+0x48 > uvideo_vs_free_isoc() at uvideo_close+0xe0 > uvideo_close() at videoclose+0x5c > > but it only happens if ffplay doesn't quit on detach and consumes a lot of > wired errors (really a lot), and I forced to kill it. > > Yeah, non of diffs help with that, and I think that all consumer should be > stop before device is detached. In any case, a user-land application still shouldn't crash the kernel. I'm still puzzled where exactly and why this happens. I think we should try to understand and tackle this specific crash next. > Anyway, I'd like to point that I like your diff, and I think it should go > because it improves behaviour and make it compatible with Linux. > > I also would like to point that I had discovered the freeze after nuke of > the second bcopy was commited, and I recall that I can't reproduce it easy > if I had enabled debug. I think that this issue may turn back when we > removed bcopy again, but so far I agree that SIMPLEQ is the best option. I'm willing to take OKs to get this peace in ;-) Following the same diff for reference. 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;