On Wed, Apr 16, 2025 at 05:18:58PM GMT, Kirill A. Korinsky wrote:

> On Sun, 06 Apr 2025 21:44:24 +0200,
> Kirill A. Korinsky <kirill@korins.ky> wrote:
> > 
> > On Sun, 06 Apr 2025 15:54:03 +0200,
> > Kirill A. Korinsky <kirill@korins.ky> wrote:
> > > 
> > > On Sun, 06 Apr 2025 11:22:15 +0200,
> > > Marcus Glocker <marcus@nazgul.ch> wrote:
> > > >
> > > > On Sun, Apr 06, 2025 at 10:44:36AM GMT, Kirill A. Korinsky wrote:
> > > >
> > > > > > As already discussed a bit on ICB:
> > > > > >
> > > > > > I don't exactly understand ian@ issue, and I also didn't see a bug
> > > > > > report on bugs@ for it which would explain it well.  I can half way
> > > > > > imagine the issue based on your explanation.
> > > > > >
> > > > > > I also don't understand how your diff would fix what I did understand
> > > > > > from your issue explanation.
> > > > > >
> > > > > > In any case, since I'm also using motion;  I don't see glitches with
> > > > > > my setup using motion with uvideo.c,1.256.  And if I apply your diff,
> > > > > > *then* I'm seeing glitches, almost every 5-8 seconds.
> > > > > >
> > > > > > So based on the current information, it's pretty hard to discuss
> > > > > > about a possible solution.  Maybe ian@ could provide a detailed bug
> > > > > > report on bugs@, showing a dmesg, 'ubsdevs -v' output, and his
> > > > > > motion.conf.  Based on that, by any chance we could try to reproduce
> > > > > > the issue locally, and then discuss about a possible solution.
> > > > > >
> > > > >
> > > > > Well, I do not understand your issue but I can reproduce ian@ one on one of
> > > > > my webcam, and only one. So, I will prepare two debug outputs later today.
> > > > >
> > > > > But we're short in the time, this changes needed only to support new devices
> > > > > with high FPS... So, I suggest to revert it for release.
> > > > >
> > > > > Ok?
> > > >
> > > > Fine for me to revert the bcopy() performance diff for the release.
> > > >
> > > > ok mglocker@
> > > >
> > > 
> > > Thanks, it is reverted.
> > > 
> > > Meanwhile I had played with different screen size and frame rate and had
> > > found that some settings which triggers on my affected device some glitches
> > > with any tested version of uvideo when I had enabled UVIDEO_DEBUG and
> > > increases uvideo_debug to 2. As a base line to proof that it doesn't recent
> > > changes I've used 1.221.
> > > 
> > > If I touches framerate or screen size, it behaves differently. Threshold is
> > > 10 FPS, and if I consume more than 10 FPS I haven't got major glitches. With
> > > 15 FPS or more I haven't got any kind of glitches on tested version.
> > > 
> > > So, I have:
> > > 
> > >  - uvideo.c,v 1.221 and 1.257 some micro glitches time to time when I had
> > >    enabled debug, and major glitches when I also increased uvideo_debug.
> > > 
> > >  - uvideo.c,v 1.256 major gliteches, screenshot:
> > >    https://kirill.korins.ky/pub/uvideo-motion-regression/screenshot-2025-04-06_13-19-56.png
> > > 
> > >  - uvideo.c,v 1.256 + diff from a thread better, but still see some glitches
> > >    with enabled debug.
> > > 
> > > BTW I had keep samples of debug log and differences with motion.conf here:
> > > https://kirill.korins.ky/pub/uvideo-motion-regression/
> > > 
> > > Anyway, base on all that I said, I don't think that bcopy diff is a root
> > > cause of the issue. I think that that we already have the issue, but bcopy
> > > diff uncovers it on some device and settings as side effect, which allows us
> > > to notice it.
> > > 
> > 
> > Here a bit improved version of remove bcopy again. On my tests I haven't see
> > any differences with 1.257 with or without increased debug. I mean that
> > without debug it hasn't got any glitches, with it has, but amount matches
> > that I see on 1.221 and 1.257.
> > 
> > May I ask you to try it?
> >
> 
> The tree is unlocked, and I really would like to put it back.
> 
> But before Ian, Markus, may I ask you to try this diff on your hardware?
> 
> I don't see any differences with and without it on my devices.

The glitches when using motion with my cams are gone.

But when reading the diff, I'm currently just not happy how we try to
solve a specific application behavior in the driver.  It feels like a
hack, misusing buffers which had another purpose, and it makes the
code hard to understand IMO.

I'm sorry, but I need to think over this more further to get a better
understanding.
 
> 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	6 Apr 2025 19:33:21 -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;
> @@ -101,7 +102,7 @@ struct uvideo_softc {
>  	const struct uvideo_devs		*sc_quirk;
>  	void					(*sc_decode_stream_header)
>  						    (struct uvideo_softc *,
> -						    uint8_t *, int);
> +						    uint8_t *, int, uint8_t *);
>  };
>  
>  int		uvideo_open(void *, int, int *, uint8_t *, void (*)(void *),
> @@ -167,11 +168,11 @@ void		uvideo_vs_start_isoc_ixfer(struct 
>  void		uvideo_vs_cb(struct usbd_xfer *, void *,
>  		    usbd_status);
>  void		uvideo_vs_decode_stream_header(struct uvideo_softc *,
> -		    uint8_t *, int);
> +		    uint8_t *, int, uint8_t *);
>  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 *, int, uint8_t *);
> +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);
> @@ -2219,6 +2220,7 @@ uvideo_vs_start_bulk_thread(void *arg)
>  	struct uvideo_softc *sc = arg;
>  	usbd_status error;
>  	int size;
> +	uint8_t *buf;
>  
>  	usbd_ref_incr(sc->sc_udev);
>  	while (sc->sc_vs_cur->bulk_running) {
> @@ -2245,7 +2247,13 @@ uvideo_vs_start_bulk_thread(void *arg)
>  
>  		DPRINTF(2, "%s: *** buffer len = %d\n", DEVNAME(sc), size);
>  
> -		sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size);
> +		if (sc->sc_mmap_flag)
> +			buf = uvideo_mmap_getbuf(sc);
> +		else
> +			buf = sc->sc_frame_buffer.buf;
> +
> +		sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size,
> +		    buf);
>  	}
>  	usbd_ref_decr(sc->sc_udev);
>  
> @@ -2296,7 +2304,7 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
>  	struct uvideo_isoc_xfer *ixfer = priv;
>  	struct uvideo_softc *sc = ixfer->sc;
>  	int len, i, frame_size;
> -	uint8_t *frame;
> +	uint8_t *frame, *buf;
>  
>  	DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__);
>  
> @@ -2319,7 +2327,12 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
>  			/* frame is empty */
>  			continue;
>  
> -		sc->sc_decode_stream_header(sc, frame, frame_size);
> +		if (sc->sc_mmap_flag)
> +			buf = uvideo_mmap_getbuf(sc);
> +		else
> +			buf = sc->sc_frame_buffer.buf;
> +
> +		sc->sc_decode_stream_header(sc, frame, frame_size, buf);
>  	}
>  
>  skip:	/* setup new transfer */
> @@ -2328,7 +2341,7 @@ skip:	/* setup new transfer */
>  
>  void
>  uvideo_vs_decode_stream_header(struct uvideo_softc *sc, uint8_t *frame,
> -    int frame_size)
> +    int frame_size, uint8_t *buf)
>  {
>  	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
>  	struct usb_video_stream_header *sh;
> @@ -2391,7 +2404,7 @@ uvideo_vs_decode_stream_header(struct uv
>  		fb->error = 1;
>  	}
>  	if (sample_len > 0) {
> -		bcopy(frame + sh->bLength, fb->buf + fb->offset, sample_len);
> +		bcopy(frame + sh->bLength, buf + fb->offset, sample_len);
>  		fb->offset += sample_len;
>  	}
>  
> @@ -2409,7 +2422,7 @@ uvideo_vs_decode_stream_header(struct uv
>  
>  		if (sc->sc_mmap_flag) {
>  			/* mmap */
> -			uvideo_mmap_queue(sc, fb->buf, fb->offset, fb->error);
> +			uvideo_mmap_queue(sc, fb->offset, fb->error);
>  		} else if (fb->error) {
>  			DPRINTF(1, "%s: %s: error frame, skipped!\n",
>  			    DEVNAME(sc), __func__);
> @@ -2442,7 +2455,7 @@ uvideo_vs_decode_stream_header(struct uv
>   */
>  void
>  uvideo_vs_decode_stream_header_isight(struct uvideo_softc *sc, uint8_t *frame,
> -    int frame_size)
> +    int frame_size, uint8_t *buf)
>  {
>  	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
>  	int sample_len, header = 0;
> @@ -2463,7 +2476,7 @@ uvideo_vs_decode_stream_header_isight(st
>  	if (header) {
>  		if (sc->sc_mmap_flag) {
>  			/* mmap */
> -			uvideo_mmap_queue(sc, fb->buf, fb->offset, 0);
> +			uvideo_mmap_queue(sc, fb->offset, 0);
>  		} else {
>  			/* read */
>  			uvideo_read(sc, fb->buf, fb->offset);
> @@ -2473,17 +2486,30 @@ uvideo_vs_decode_stream_header_isight(st
>  		/* save sample */
>  		sample_len = frame_size;
>  		if ((fb->offset + sample_len) <= fb->buf_size) {
> -			bcopy(frame, fb->buf + fb->offset, sample_len);
> +			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_frame_buffer.offset > 0)
> +		return sc->sc_frame_buffer.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 +2523,54 @@ 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 sc->sc_frame_buffer.buf;
> +	}
>  
> -	return idx;
> +	sc->sc_mmap_cur = &sc->sc_mmap[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;
> +	uint8_t *buf;
>  
> -	if (sc->sc_mmap_count == 0 || sc->sc_mmap_buffer == NULL)
> -		panic("%s: mmap buffers not allocated", __func__);
> +	if (sc->sc_mmap_cur == NULL) {
> +		sc->sc_frame_buffer.offset = 0;
> +		buf = uvideo_mmap_getbuf(sc);
>  
> -	/* 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 (buf == sc->sc_frame_buffer.buf)
> +			return;
> +
> +		bcopy(sc->sc_frame_buffer.buf, buf, len);
>  	}
>  
> -	/* 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 +3536,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;
>