On 16/03/25(Sun) 23:07, Marcus Glocker wrote:
> 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.

We definitively don't need a new queueing method for the moment.

> 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.

What is the problem?

It's not clear to me what it is and using mutexes won't help as the code is
already running under KERNEL_LOCK().  The SIMPLEQ is already working as
a FIFO since descriptors are added at the tail and removed from the
head.

> And the worry you had about the thread safeness of SIMPLEQ can also be
> solved by using mutxes.

I fail to see how this will be different knowing that the KERNEL_LOCK()
is held..  Please do not add locks without the corresponding
annotations that explains which data structure they are protecting.
r
> 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);
>  }
>