On Sat, Apr 19, 2025 at 04:16:51PM GMT, Kirill A. Korinsky wrote:

> On Sat, 19 Apr 2025 14:41:49 +0200,
> Marcus Glocker <marcus@nazgul.ch> wrote:
> >
> > I think your logic by only returning a buffer for the first sample is
> > basically fine.  But for somebody who isn't in to the topic, reading
> > the code, it's hard to understand what we're doing here.  Would it make
> > sense to take a more holistic approach, by introducing a buffer flag
> > which explicitly shows that we had a queue full situation, and therefore
> > we need to skip the frame?  Instead of doing the necessary checks
> > inside of uvideo_mmap_queue() and uvideo_mmap_getbuf(), we do it
> > directly in uvideo_vs_decode_stream_header() by checking that flag.
> >
> > I think it makes it more obvious.  Though, since I can't test it here,
> > I'm not sure if it works, or if I'm missing something.
> >
> 
> Indeed, avoid access from uvideo_mmap_getbuf() into sc->sc_frame_buffer
> looks correct and make sense.
>
> Here a bit improved version where:
> - reset new flag in uvideo_vs_alloc_frame()

For consistency we probably should also initialize fb->error = 0.

> - fixed a typo at uvideo_mmap_getbuf(sc;
> - use the flag inside uvideo_vs_decode_stream_header_isight() that allows to
>   prevent a crash on overflow of the mmap queue
> - revert "nice" panic in uvideo_mmap_queue()
> 
> I had tested it on my devices and I can't reproduce Ian's issue anymore, and
> it works stable on all of them.

Good, I think we're getting there.

> Ian, may I ask you to try this one on your setup as well?

If this diff still fixes Ian's setup -> ok mglocker@
 
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.257 uvideo.c
> --- sys/dev/usb/uvideo.c	6 Apr 2025 09:46:30 -0000	1.257
> +++ sys/dev/usb/uvideo.c	19 Apr 2025 13:49:28 -0000
> @@ -66,6 +66,7 @@ struct uvideo_softc {
>  	struct uvideo_frame_buffer		 sc_frame_buffer;
> 
>  	struct uvideo_mmap			 sc_mmap[UVIDEO_MAX_BUFFERS];
> +	struct uvideo_mmap			*sc_mmap_cur;
>  	uint8_t					*sc_mmap_buffer;
>  	size_t					 sc_mmap_buffer_size;
>  	int					 sc_mmap_buffer_idx;
> @@ -170,8 +171,8 @@ 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);
> +uint8_t *	uvideo_mmap_getbuf(struct uvideo_softc *);
> +void		uvideo_mmap_queue(struct uvideo_softc *, int, int);
>  void		uvideo_read(struct uvideo_softc *, uint8_t *, int);
>  usbd_status	uvideo_usb_control(struct uvideo_softc *, uint8_t, uint8_t,
>  		    uint16_t, uint8_t *, size_t);
> @@ -1932,6 +1933,7 @@ uvideo_vs_alloc_frame(struct uvideo_soft
>  	fb->sample = 0;
>  	fb->fid = 0;
>  	fb->offset = 0;
> +	fb->mmap_q_full = 0;
>  	fb->fmt_flags = sc->sc_fmtgrp_cur->frame_cur->bDescriptorSubtype ==
>  	    UDESCSUB_VS_FRAME_UNCOMPRESSED ? 0 : V4L2_FMT_FLAG_COMPRESSED;
> 
> @@ -2333,6 +2335,7 @@ uvideo_vs_decode_stream_header(struct uv
>  	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
>  	struct usb_video_stream_header *sh;
>  	int sample_len;
> +	uint8_t *buf;
> 
>  	if (frame_size < UVIDEO_SH_MIN_LEN)
>  		/* frame too small to contain a valid stream header */
> @@ -2364,6 +2367,7 @@ uvideo_vs_decode_stream_header(struct uv
>  		fb->fid = sh->bFlags & UVIDEO_SH_FLAG_FID;
>  		fb->offset = 0;
>  		fb->error = 0;
> +		fb->mmap_q_full = 0;
>  	} else {
>  		/* continues sample for a frame, check consistency */
>  		if (fb->fid != (sh->bFlags & UVIDEO_SH_FLAG_FID)) {
> @@ -2373,6 +2377,7 @@ uvideo_vs_decode_stream_header(struct uv
>  			fb->fid = sh->bFlags & UVIDEO_SH_FLAG_FID;
>  			fb->offset = 0;
>  			fb->error = 0;
> +			fb->mmap_q_full = 0;
>  		}
>  	}
> 
> @@ -2382,6 +2387,15 @@ uvideo_vs_decode_stream_header(struct uv
>  		fb->error = 1;
>  	}
> 
> +	if (sc->sc_mmap_flag) {
> +		if (!fb->mmap_q_full) {
> +			buf = uvideo_mmap_getbuf(sc);
> +			if (buf == NULL)
> +				fb->mmap_q_full = 1;
> +		}
> +	} else
> +		buf = sc->sc_frame_buffer.buf;
> +
>  	/* save sample */
>  	sample_len = frame_size - sh->bLength;
>  	if (sample_len > fb->buf_size - fb->offset) {
> @@ -2390,8 +2404,8 @@ uvideo_vs_decode_stream_header(struct uv
>  		sample_len = fb->buf_size - fb->offset;
>  		fb->error = 1;
>  	}
> -	if (sample_len > 0) {
> -		bcopy(frame + sh->bLength, fb->buf + fb->offset, sample_len);
> +	if (!fb->mmap_q_full && sample_len > 0) {
> +		bcopy(frame + sh->bLength, buf + fb->offset, sample_len);
>  		fb->offset += sample_len;
>  	}
> 
> @@ -2409,7 +2423,8 @@ uvideo_vs_decode_stream_header(struct uv
> 
>  		if (sc->sc_mmap_flag) {
>  			/* mmap */
> -			uvideo_mmap_queue(sc, fb->buf, fb->offset, fb->error);
> +			if (!fb->mmap_q_full)
> +				uvideo_mmap_queue(sc, fb->offset, fb->error);
>  		} else if (fb->error) {
>  			DPRINTF(1, "%s: %s: error frame, skipped!\n",
>  			    DEVNAME(sc), __func__);
> @@ -2421,6 +2436,7 @@ uvideo_vs_decode_stream_header(struct uv
>  		fb->sample = 0;
>  		fb->fid = 0;
>  		fb->error = 0;
> +		fb->mmap_q_full = 0;
>  	}
>  }
> 
> @@ -2446,6 +2462,7 @@ uvideo_vs_decode_stream_header_isight(st
>  {
>  	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
>  	int sample_len, header = 0;
> +	uint8_t *buf;
>  	uint8_t magic[] = {
>  	    0x11, 0x22, 0x33, 0x44,
>  	    0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xfa, 0xce };
> @@ -2463,27 +2480,49 @@ uvideo_vs_decode_stream_header_isight(st
>  	if (header) {
>  		if (sc->sc_mmap_flag) {
>  			/* mmap */
> -			uvideo_mmap_queue(sc, fb->buf, fb->offset, 0);
> +			if (!fb->mmap_q_full)
> +				uvideo_mmap_queue(sc, fb->offset, 0);
>  		} else {
>  			/* read */
>  			uvideo_read(sc, fb->buf, fb->offset);
>  		}
>  		fb->offset = 0;
> +		fb->mmap_q_full = 0;
>  	} else {
> +		if (sc->sc_mmap_flag) {
> +			if (!fb->mmap_q_full) {
> +				buf = uvideo_mmap_getbuf(sc);
> +				if (buf == NULL)
> +					fb->mmap_q_full = 1;
> +			}
> +		} else
> +			buf = sc->sc_frame_buffer.buf;
> +
>  		/* save sample */
>  		sample_len = frame_size;
> -		if ((fb->offset + sample_len) <= fb->buf_size) {
> -			bcopy(frame, fb->buf + fb->offset, sample_len);
> +		if (!fb->mmap_q_full &&
> +		    (fb->offset + sample_len) <= fb->buf_size) {
> +			bcopy(frame, buf + fb->offset, sample_len);
>  			fb->offset += sample_len;
>  		}
>  	}
>  }
> 
> -int
> -uvideo_get_buf(struct uvideo_softc *sc)
> +uint8_t *
> +uvideo_mmap_getbuf(struct uvideo_softc *sc)
>  {
>  	int i, idx = sc->sc_mmap_buffer_idx;
> 
> +	/*
> +	 * Section 2.4.3.2 explicitly allows multiple frames per one
> +	 * transfer and multiple transfers per one frame.
> +	 */
> +	if (sc->sc_mmap_cur != NULL)
> +		return sc->sc_mmap_cur->buf;
> +
> +	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[sc->sc_mmap_buffer_idx].v4l2_buf.flags &
> @@ -2497,51 +2536,45 @@ uvideo_get_buf(struct uvideo_softc *sc)
>  			sc->sc_mmap_buffer_idx = 0;
>  	}
> 
> -	if (i == sc->sc_mmap_count)
> -		return -1;
> +	if (i == sc->sc_mmap_count) {
> +		DPRINTF(1, "%s: %s: mmap queue is full!\n",
> +		    DEVNAME(sc), __func__);
> +		return NULL;
> +	}
> +
> +	sc->sc_mmap_cur = &sc->sc_mmap[idx];
> 
> -	return idx;
> +	return sc->sc_mmap_cur->buf;
>  }
> 
>  void
> -uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err)
> +uvideo_mmap_queue(struct uvideo_softc *sc, int len, int err)
>  {
> -	int i;
> -
> -	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 */
> -	i = uvideo_get_buf(sc);
> -	if (i == -1) {
> -		DPRINTF(1, "%s: %s: mmap queue is full!\n",
> -		    DEVNAME(sc), __func__);
> -		return;
> -	}
> +	if (sc->sc_mmap_cur == NULL)
> +		panic("uvideo_mmap_queue: NULL pointer!");
> 
> -	/* copy frame to mmap buffer and report length */
> -	bcopy(buf, sc->sc_mmap[i].buf, len);
> -	sc->sc_mmap[i].v4l2_buf.bytesused = len;
> +	/* report frame length */
> +	sc->sc_mmap_cur->v4l2_buf.bytesused = len;
> 
>  	/* timestamp it */
> -	getmicrouptime(&sc->sc_mmap[i].v4l2_buf.timestamp);
> -	sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_TIMESTAMP_MASK;
> -	sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> -	sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> -	sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_TSTAMP_SRC_EOF;
> -	sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_TIMECODE;
> +	getmicrouptime(&sc->sc_mmap_cur->v4l2_buf.timestamp);
> +	sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TIMESTAMP_MASK;
> +	sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> +	sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
> +	sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_TSTAMP_SRC_EOF;
> +	sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TIMECODE;
> 
>  	/* forward error bit */
> -	sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_ERROR;
> +	sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_ERROR;
>  	if (err)
> -		sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_ERROR;
> +		sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_ERROR;
> 
>  	/* queue it */
> -	sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_DONE;
> -	sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
> -	SIMPLEQ_INSERT_TAIL(&sc->sc_mmap_q, &sc->sc_mmap[i], q_frames);
> -	DPRINTF(2, "%s: %s: frame queued on index %d\n",
> -	    DEVNAME(sc), __func__, i);
> +	sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_DONE;
> +	sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
> +	SIMPLEQ_INSERT_TAIL(&sc->sc_mmap_q, sc->sc_mmap_cur, q_frames);
> +	sc->sc_mmap_cur = NULL;
> +	DPRINTF(2, "%s: %s: frame queued\n", DEVNAME(sc), __func__);
> 
>  	wakeup(sc);
> 
> @@ -3507,6 +3540,7 @@ uvideo_reqbufs(void *v, struct v4l2_requ
>  	}
> 
>  	sc->sc_mmap_buffer_idx = 0;
> +	sc->sc_mmap_cur = NULL;
> 
>  	/* tell how many buffers we have really allocated */
>  	rb->count = sc->sc_mmap_count;
> Index: sys/dev/usb/uvideo.h
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.h,v
> diff -u -p -r1.64 uvideo.h
> --- sys/dev/usb/uvideo.h	26 Feb 2025 20:50:46 -0000	1.64
> +++ sys/dev/usb/uvideo.h	19 Apr 2025 13:42:20 -0000
> @@ -540,6 +540,7 @@ struct uvideo_frame_buffer {
>  	int		 sample;
>  	uint8_t		 fid;
>  	uint8_t		 error;
> +	uint8_t		 mmap_q_full;
>  	int		 offset;
>  	int		 buf_size;
>  	uint8_t		*buf;
>