On Sat, Mar 08, 2025 at 09:21:00AM GMT, Marcus Glocker wrote:

> On Thu, Mar 06, 2025 at 09:36:46AM GMT, Kirill A. Korinsky wrote:
> 
> > tech@,
> > 
> > macroses from queue.h doesn't thread safe and wraping it by mutex on
> > uvideo use case seems isn't so good idea.
> > 
> > It has a hardcoded number of used buffers as 8 and already has a code
> > which iterates against them to find the buffer which is ready for
> > queueing in uvideo_mmap_getbuf.
> > 
> > Here I'd like to replace SIMPLEQ_* by similar logic.
> > 
> > It fixes two bugs:
> > 
> > 1. If isochronous webcam sends two frames in one transfer,
> > SIMPLEQ_INSERT_TAIL adds the same element twice and creates infinity
> > loop inside sc_mmap_q which will be iterating in uvideo_vs_free_frame.
> > 
> > 2. Prevents from calling SIMPLEQ_INSERT_TAIL and SIMPLEQ_REMOVE_HEAD for
> > the same element in the queue which may lead to unexpected state.
> > 
> > This diff was suceffuly tested on:
> >  - Elgato Facecam Pro (isochronous)
> >  - Jabra PanaCast (bulk)
> > 
> > Ok?
> 
> I will need a bit more time to do more investigation for our current
> queuing logic, since after looking initially at it, I'm not happy with
> the current state.  The main thing which bothers me is that with the
> current code, and also with your diff below, we only use one buffer
> under "normal pressure".  At least that's what I'm seeing with my
> devices here.
> 
> I'm not sure if that is the right thing to do, or when an application
> asks for example to use 4 buffers, if we shouldn't always iterate
> through them.

I would like to cleanup the queuing in small steps.  Hence, not
removing SIMPLEQ yet.  Would this diff fix your issue when receiving
multiple frames in one transfer?  It looks my devices don't do that
very often, so difficult for me to test.


Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
diff -u -p -u -p -r1.250 uvideo.c
--- sys/dev/usb/uvideo.c	8 Mar 2025 08:27:32 -0000	1.250
+++ sys/dev/usb/uvideo.c	8 Mar 2025 20:19:59 -0000
@@ -102,7 +102,7 @@ struct uvideo_softc {
 	void					 (*sc_uplayer_intr)(void *);
 
 	const struct uvideo_devs		*sc_quirk;
-	void					(*sc_decode_stream_header)
+	int					(*sc_decode_stream_header)
 						    (struct uvideo_softc *,
 						    uint8_t *, int, uint8_t *);
 };
@@ -169,9 +169,9 @@ void		uvideo_vs_start_isoc_ixfer(struct 
 		    struct uvideo_isoc_xfer *);
 void		uvideo_vs_cb(struct usbd_xfer *, void *,
 		    usbd_status);
-void		uvideo_vs_decode_stream_header(struct uvideo_softc *,
+int		uvideo_vs_decode_stream_header(struct uvideo_softc *,
 		    uint8_t *, int, uint8_t *); 
-void		uvideo_vs_decode_stream_header_isight(struct uvideo_softc *,
+int		uvideo_vs_decode_stream_header_isight(struct uvideo_softc *,
 		    uint8_t *, int, uint8_t *);
 uint8_t		*uvideo_mmap_getbuf(struct uvideo_softc *);
 void		uvideo_mmap_queue(struct uvideo_softc *, int, int);
@@ -2346,24 +2346,27 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
 			/* frame is empty */
 			continue;
 
-		sc->sc_decode_stream_header(sc, frame, frame_size, buf);
+		if (sc->sc_decode_stream_header(sc, frame, frame_size, buf)) {
+			if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
+				break;
+		}
 	}
 
 skip:	/* setup new transfer */
 	uvideo_vs_start_isoc_ixfer(sc, ixfer);
 }
 
-void
+int
 uvideo_vs_decode_stream_header(struct uvideo_softc *sc, uint8_t *frame,
     int frame_size, uint8_t *buf)
 {
 	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
 	struct usb_video_stream_header *sh;
-	int sample_len;
+	int sample_len, r = 0;
 
 	if (frame_size < UVIDEO_SH_MIN_LEN)
 		/* frame too small to contain a valid stream header */
-		return;
+		return r;
 
 	sh = (struct usb_video_stream_header *)frame;
 
@@ -2371,7 +2374,7 @@ uvideo_vs_decode_stream_header(struct uv
 
 	if (sh->bLength > frame_size || sh->bLength < UVIDEO_SH_MIN_LEN)
 		/* invalid header size */
-		return;
+		return r;
 
 	DPRINTF(2, "%s: frame_size = %d\n", DEVNAME(sc), frame_size);
 
@@ -2442,6 +2445,7 @@ uvideo_vs_decode_stream_header(struct uv
 		if (sc->sc_mmap_flag) {
 			/* mmap */
 			uvideo_mmap_queue(sc, fb->offset, fb->error);
+			r = 1;
 		} else if (fb->error) {
 			DPRINTF(1, "%s: %s: error frame, skipped!\n",
 			    DEVNAME(sc), __func__);
@@ -2454,6 +2458,8 @@ uvideo_vs_decode_stream_header(struct uv
 		fb->fid = 0;
 		fb->error = 0;
 	}
+
+	return r;
 }
 
 /*
@@ -2472,12 +2478,12 @@ uvideo_vs_decode_stream_header(struct uv
  * Sometimes the stream header is prefixed by a unknown byte.  Therefore
  * we check for the magic value on two offsets.
  */
-void
+int
 uvideo_vs_decode_stream_header_isight(struct uvideo_softc *sc, uint8_t *frame,
     int frame_size, uint8_t *buf)
 {
 	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
-	int sample_len, header = 0;
+	int sample_len, header = 0, r = 0;
 	uint8_t magic[] = {
 	    0x11, 0x22, 0x33, 0x44,
 	    0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xfa, 0xce };
@@ -2489,13 +2495,14 @@ uvideo_vs_decode_stream_header_isight(st
 
 	if (header && fb->fid == 0) {
 		fb->fid = 1;
-		return;
+		return r;
 	}
 
 	if (header) {
 		if (sc->sc_mmap_flag) {
 			/* mmap */
 			uvideo_mmap_queue(sc, fb->offset, 0);
+			r = 1;
 		} else {
 			/* read */
 			uvideo_read(sc, fb->buf, fb->offset);
@@ -2509,6 +2516,8 @@ uvideo_vs_decode_stream_header_isight(st
 			fb->offset += sample_len;
 		}
 	}
+
+	return r;
 }
 
 uint8_t *
@@ -2521,7 +2530,8 @@ uvideo_mmap_getbuf(struct uvideo_softc *
 
 	/* 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)
+		if ((sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_QUEUED) &&
+		    (sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_DONE) == 0)
 			break;
 	}
 	if (i == sc->sc_mmap_count) {