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@
 
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /home/cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -r1.256 uvideo.c
> --- sys/dev/usb/uvideo.c	24 Mar 2025 18:56:34 -0000	1.256
> +++ sys/dev/usb/uvideo.c	6 Apr 2025 08:41:40 -0000
> @@ -66,7 +66,6 @@ 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;
> @@ -102,7 +101,7 @@ struct uvideo_softc {
>  	const struct uvideo_devs		*sc_quirk;
>  	void					(*sc_decode_stream_header)
>  						    (struct uvideo_softc *,
> -						    uint8_t *, int, uint8_t *);
> +						    uint8_t *, int);
>  };
>  
>  int		uvideo_open(void *, int, int *, uint8_t *, void (*)(void *),
> @@ -168,11 +167,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 *);
> +		    uint8_t *, int);
>  void		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);
> +		    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,
>  		    uint16_t, uint8_t *, size_t);
> @@ -2220,7 +2219,6 @@ 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) {
> @@ -2247,14 +2245,7 @@ uvideo_vs_start_bulk_thread(void *arg)
>  
>  		DPRINTF(2, "%s: *** buffer len = %d\n", DEVNAME(sc), size);
>  
> -		if (sc->sc_mmap_flag) {
> -			if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
> -				continue;
> -		} else
> -			buf = sc->sc_frame_buffer.buf;
> -
> -		sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size,
> -		    buf);
> +		sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size);
>  	}
>  	usbd_ref_decr(sc->sc_udev);
>  
> @@ -2305,7 +2296,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, *buf;
> +	uint8_t *frame;
>  
>  	DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__);
>  
> @@ -2328,13 +2319,7 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
>  			/* frame is empty */
>  			continue;
>  
> -		if (sc->sc_mmap_flag) {
> -			if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
> -				goto skip;
> -		} else
> -			buf = sc->sc_frame_buffer.buf;
> -
> -		sc->sc_decode_stream_header(sc, frame, frame_size, buf);
> +		sc->sc_decode_stream_header(sc, frame, frame_size);
>  	}
>  
>  skip:	/* setup new transfer */
> @@ -2343,7 +2328,7 @@ skip:	/* setup new transfer */
>  
>  void
>  uvideo_vs_decode_stream_header(struct uvideo_softc *sc, uint8_t *frame,
> -    int frame_size, uint8_t *buf)
> +    int frame_size)
>  {
>  	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
>  	struct usb_video_stream_header *sh;
> @@ -2406,7 +2391,7 @@ uvideo_vs_decode_stream_header(struct uv
>  		fb->error = 1;
>  	}
>  	if (sample_len > 0) {
> -		bcopy(frame + sh->bLength, buf + fb->offset, sample_len);
> +		bcopy(frame + sh->bLength, fb->buf + fb->offset, sample_len);
>  		fb->offset += sample_len;
>  	}
>  
> @@ -2424,7 +2409,7 @@ uvideo_vs_decode_stream_header(struct uv
>  
>  		if (sc->sc_mmap_flag) {
>  			/* mmap */
> -			uvideo_mmap_queue(sc, fb->offset, fb->error);
> +			uvideo_mmap_queue(sc, fb->buf, fb->offset, fb->error);
>  		} else if (fb->error) {
>  			DPRINTF(1, "%s: %s: error frame, skipped!\n",
>  			    DEVNAME(sc), __func__);
> @@ -2457,7 +2442,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, uint8_t *buf)
> +    int frame_size)
>  {
>  	struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
>  	int sample_len, header = 0;
> @@ -2478,7 +2463,7 @@ uvideo_vs_decode_stream_header_isight(st
>  	if (header) {
>  		if (sc->sc_mmap_flag) {
>  			/* mmap */
> -			uvideo_mmap_queue(sc, fb->offset, 0);
> +			uvideo_mmap_queue(sc, fb->buf, fb->offset, 0);
>  		} else {
>  			/* read */
>  			uvideo_read(sc, fb->buf, fb->offset);
> @@ -2488,27 +2473,17 @@ uvideo_vs_decode_stream_header_isight(st
>  		/* save sample */
>  		sample_len = frame_size;
>  		if ((fb->offset + sample_len) <= fb->buf_size) {
> -			bcopy(frame, buf + fb->offset, sample_len);
> +			bcopy(frame, fb->buf + fb->offset, sample_len);
>  			fb->offset += sample_len;
>  		}
>  	}
>  }
>  
> -uint8_t *
> -uvideo_mmap_getbuf(struct uvideo_softc *sc)
> +int
> +uvideo_get_buf(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 &
> @@ -2522,45 +2497,51 @@ uvideo_mmap_getbuf(struct uvideo_softc *
>  			sc->sc_mmap_buffer_idx = 0;
>  	}
>  
> -	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];
> +	if (i == sc->sc_mmap_count)
> +		return -1;
>  
> -	return sc->sc_mmap_cur->buf;
> +	return idx;
>  }
>  
>  void
> -uvideo_mmap_queue(struct uvideo_softc *sc, int len, int err)
> +uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err)
>  {
> -	if (sc->sc_mmap_cur == NULL)
> -		panic("uvideo_mmap_queue: NULL pointer!");
> +	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;
> +	}
>  
> -	/* report frame length */
> -	sc->sc_mmap_cur->v4l2_buf.bytesused = 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;
>  
>  	/* timestamp it */
> -	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;
> +	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;
>  
>  	/* forward error bit */
> -	sc->sc_mmap_cur->v4l2_buf.flags &= ~V4L2_BUF_FLAG_ERROR;
> +	sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_ERROR;
>  	if (err)
> -		sc->sc_mmap_cur->v4l2_buf.flags |= V4L2_BUF_FLAG_ERROR;
> +		sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_ERROR;
>  
>  	/* queue it */
> -	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__);
> +	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);
>  
>  	wakeup(sc);
>  
> @@ -3526,7 +3507,6 @@ 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;
>