From: Marcus Glocker Subject: Re: sys/uvideo: don't skip frame immediately when buffer unavailable (regression from v1.256) To: "Kirill A. Korinsky" Cc: tech@openbsd.org, Ian Darwin Date: Sun, 6 Apr 2025 11:22:15 +0200 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; >