From: Marcus Glocker Subject: Re: sys/uvideo: avoid using queue.h To: "Kirill A. Korinsky" Cc: tech@openbsd.org Date: Sun, 16 Mar 2025 23:07:18 +0100 On Sun, Mar 16, 2025 at 11:10:22AM GMT, Kirill A. Korinsky wrote: > > I'm not familiar with this specific queuing algorithm, but it seems to > > work :-) > > > > Yeah, and it is quite obvious, just a two counters. > > > I gave it a spin, and I can see that FIFO is working, and that it always > > iterates through all the buffers, also with lower frame rates. This is > > in line with the behavior of the Linux driver, and is also an improvement > > compared to the current SIMPLEQ implementation, which is only consuming > > buffer zero, mostly. > > > > I will need to do some more testing. > > > > Thanks! I, locally, test it as well with only > https://marc.info/?l=openbsd-tech&m=174180759929893&w=2 > > Everything quite stable on my usecases. > > > Some initial in-line comments. > > > > > +#define UVIDEO_MMAP_QUEUE_SIZE (UVIDEO_MAX_BUFFERS + 1) > > > + > > > +struct uvideo_mmap_queue_item { > > > + unsigned long qi_seq; > > > + int qi_idx; > > > +}; > > > + > > > +struct uvideo_mmap_queue { > > > + struct uvideo_mmap_queue_item q_items[UVIDEO_MMAP_QUEUE_SIZE]; > > > > Line break, maybe remove the additional space indentations for these new > > structures. > > > > Or I may follow style of uvideo_softc and call it as q_item. > > At least fold(1) thinks it's ok. > > > > > #include should be removed as well. > > Do you think it is worth to move FIFO queue into sys/queue.h? 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. 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? 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 16 Mar 2025 21:19:36 -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) + 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); }