From: Kirill A. Korinsky Subject: Re: sys/uvideo: don't skip frame immediately when buffer unavailable (regression from v1.256) To: Marcus Glocker , Ian Darwin Cc: tech@openbsd.org Date: Sat, 19 Apr 2025 16:16:51 +0200 On Sat, 19 Apr 2025 14:41:49 +0200, Marcus Glocker 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() - 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. Ian, may I ask you to try this one on your setup as well? 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;