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 <sys/queue.h> 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 <sys/stat.h>
 #include <sys/kthread.h>
 #include <sys/stdint.h>
+#include <sys/mutex.h>
 
 #include <uvm/uvm_extern.h>
 
@@ -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);
 }