From: Marcus Glocker Subject: Re: sys/uvideo: avoid using queue.h To: "Kirill A. Korinsky" Cc: tech@openbsd.org Date: Mon, 17 Mar 2025 20:07:56 +0100 On Mon, Mar 17, 2025 at 12:13:53AM GMT, Kirill A. Korinsky wrote: > On Sun, 16 Mar 2025 23:07:18 +0100, > Marcus Glocker wrote: > > > > Well, I'm not sure if we require a new queuing method in queue.h. > > Probably there are other people who can judge better on this. > > > > I think that we should keep that discussion for some time in the future. > > > In any case, the current issue of mostly re-using the first buffer isn't > > a SIMPLEQ issue as such. The queue.h functions should also provide > > FIFO. The problem is that we're currently using a pretty dumb loop to > > find a free buffer in uvideo(4). It always starts from the top of the > > buffer list, and looks for the first buffer which is V4L2_BUF_FLAG_QUEUED > > flagged. Obviously, if the first buffer gets processed and re-queued > > fast enough, which is usually the case, we will just select the first > > buffer again, and so mostly use only one buffer. This problem could be > > also fixed by using a global index for the frame buffer, and make sure > > that it always iterates over the entire buffer. > > > > And the worry you had about the thread safeness of SIMPLEQ can also be > > solved by using mutxes. > > > > Following a diff which demonstrates this, and works for me what I could > > test so far. > > > > Though, I guess your method comes with a better performance. I had no > > time to compare the performance between the two diffs yet, but I think > > it would be interesting to know. > > > > Also, just to help on a decision making, I would like to understand > > whether your diff is solving another issue which mine isn't, despite > > the eventual performance improvements? > > > > 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? 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 18:55:18 -0000 @@ -24,6 +24,7 @@ #include #include #include +#include #include @@ -68,6 +69,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; @@ -104,6 +106,7 @@ struct uvideo_softc { void (*sc_decode_stream_header) (struct uvideo_softc *, uint8_t *, int); + struct mutex sc_queue_mtx; }; int uvideo_open(void *, int, int *, uint8_t *, void (*)(void *), @@ -172,6 +175,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, @@ -546,6 +550,8 @@ uvideo_attach(struct device *parent, str struct usbd_desc_iter iter; int i; + mtx_init(&sc->sc_queue_mtx, IPL_USB); + sc->sc_udev = uaa->device; /* Find the first unclaimed video interface. */ @@ -1949,6 +1955,8 @@ uvideo_vs_free_frame(struct uvideo_softc { struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer; + mtx_enter(&sc->sc_queue_mtx); + if (fb->buf != NULL) { free(fb->buf, M_USBDEV, fb->buf_size); fb->buf = NULL; @@ -1964,6 +1972,8 @@ uvideo_vs_free_frame(struct uvideo_softc SIMPLEQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames); sc->sc_mmap_count = 0; + + mtx_leave(&sc->sc_queue_mtx); } usbd_status @@ -2498,22 +2508,46 @@ 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) { int i; + mtx_enter(&sc->sc_queue_mtx); + if (sc->sc_mmap_count == 0 || sc->sc_mmap_buffer == NULL) 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__); + mtx_leave(&sc->sc_queue_mtx); return; } @@ -2541,6 +2575,8 @@ uvideo_mmap_queue(struct uvideo_softc *s DPRINTF(2, "%s: %s: frame queued on index %d\n", DEVNAME(sc), __func__, i); + mtx_leave(&sc->sc_queue_mtx); + wakeup(sc); /* @@ -3553,6 +3589,8 @@ uvideo_reqbufs(void *v, struct v4l2_requ 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; @@ -3613,11 +3651,14 @@ uvideo_dqbuf(void *v, struct v4l2_buffer dqb->memory != V4L2_MEMORY_MMAP) return (EINVAL); + mtx_enter(&sc->sc_queue_mtx); if (SIMPLEQ_EMPTY(&sc->sc_mmap_q)) { + mtx_leave(&sc->sc_queue_mtx); /* 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); + mtx_enter(&sc->sc_queue_mtx); } mmap = SIMPLEQ_FIRST(&sc->sc_mmap_q); @@ -3632,6 +3673,8 @@ uvideo_dqbuf(void *v, struct v4l2_buffer 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); + + mtx_leave(&sc->sc_queue_mtx); return (0); }