Download raw body.
sys/uvideo: treat frame as done when mmap queue is overflowed
sys/uvideo: treat frame as done when mmap queue is overflowed
sys/uvideo: treat frame as done when mmap queue is overflowed
On Wed, 26 Feb 2025 21:34:25 +0100,
Marcus Glocker <marcus@nazgul.ch> wrote:
>
> On Tue, Feb 25, 2025 at 11:19:58PM GMT, Kirill A. Korinsky wrote:
>
> > On Tue, 25 Feb 2025 22:07:30 +0100,
> > Marcus Glocker <marcus@nazgul.ch> wrote:
> > >
> > > I've noticed that the for loop in uvideo_vs_cb() is checking
> > > sc->sc_decode_stream_header() for an USBD_CANCELLED return code, but
> > > sc->sc_decode_stream_header() is never returning that. Probably we
> > > should better check for 'error != USBD_NORMAL_COMPLETION' to break
> > > the for loop, and then always reset the frame buffers sample, fid, and
> > > error flags to zero in sc->sc_decode_stream_header() return error case.
> > >
> > > I can come up with a diff for that later.
> > >
> >
> > Not sure that it is a good idea.
> >
> > A for loop exists only for isochronous endpoint, and if one of parallel
> > readed frame is contained something very wired, other one may contain
> > usefull data.
>
> Probably I was too long away from this topic, and for a minute I was
> thinking that a broken frame would lead to a broken image, but probably
> brain fart.
>
> Anyway, at the moment nothing is checking the values which
> sc_decode_stream_header returns, and so we could convert it to a void
> function to remove some confusion. Same for uvideo_mmap_queue.
>
Reads and compiles OK kirill@
>
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -u -p -r1.244 uvideo.c
> --- sys/dev/usb/uvideo.c 25 Feb 2025 22:13:44 -0000 1.244
> +++ sys/dev/usb/uvideo.c 26 Feb 2025 20:27:59 -0000
> @@ -101,7 +101,7 @@ struct uvideo_softc {
> void (*sc_uplayer_intr)(void *);
>
> const struct uvideo_devs *sc_quirk;
> - usbd_status (*sc_decode_stream_header)
> + void (*sc_decode_stream_header)
> (struct uvideo_softc *,
> uint8_t *, int);
> };
> @@ -165,11 +165,11 @@ void uvideo_vs_start_isoc_ixfer(struct
> struct uvideo_isoc_xfer *);
> void uvideo_vs_cb(struct usbd_xfer *, void *,
> usbd_status);
> -usbd_status uvideo_vs_decode_stream_header(struct uvideo_softc *,
> +void uvideo_vs_decode_stream_header(struct uvideo_softc *,
> uint8_t *, int);
> -usbd_status uvideo_vs_decode_stream_header_isight(struct uvideo_softc *,
> +void uvideo_vs_decode_stream_header_isight(struct uvideo_softc *,
> uint8_t *, int);
> -int uvideo_mmap_queue(struct uvideo_softc *, uint8_t *, int, int);
> +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);
> @@ -2149,8 +2149,7 @@ uvideo_vs_start_bulk_thread(void *arg)
>
> DPRINTF(2, "%s: *** buffer len = %d\n", DEVNAME(sc), size);
>
> - (void)sc->sc_decode_stream_header(sc,
> - sc->sc_vs_cur->bxfer.buf, size);
> + sc->sc_decode_stream_header(sc, sc->sc_vs_cur->bxfer.buf, size);
> }
> usbd_ref_decr(sc->sc_udev);
>
> @@ -2205,7 +2204,6 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
> struct uvideo_softc *sc = ixfer->sc;
> int len, i, frame_size;
> uint8_t *frame;
> - usbd_status error;
>
> DPRINTF(2, "%s: %s\n", DEVNAME(sc), __func__);
>
> @@ -2228,27 +2226,24 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
> /* frame is empty */
> continue;
>
> - error = sc->sc_decode_stream_header(sc, frame, frame_size);
> - if (error == USBD_CANCELLED)
> - break;
> + sc->sc_decode_stream_header(sc, frame, frame_size);
> }
>
> skip: /* setup new transfer */
> uvideo_vs_start_isoc_ixfer(sc, ixfer);
> }
>
> -usbd_status
> +void
> uvideo_vs_decode_stream_header(struct uvideo_softc *sc, uint8_t *frame,
> int frame_size)
> {
> struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
> struct usb_video_stream_header *sh;
> int sample_len;
> - usbd_status ret = USBD_NORMAL_COMPLETION;
>
> if (frame_size < UVIDEO_SH_MIN_LEN)
> /* frame too small to contain a valid stream header */
> - return (USBD_INVAL);
> + return;
>
> sh = (struct usb_video_stream_header *)frame;
>
> @@ -2256,7 +2251,7 @@ uvideo_vs_decode_stream_header(struct uv
>
> if (sh->bLength > frame_size || sh->bLength < UVIDEO_SH_MIN_LEN)
> /* invalid header size */
> - return (USBD_INVAL);
> + return;
>
> DPRINTF(2, "%s: frame_size = %d\n", DEVNAME(sc), frame_size);
>
> @@ -2326,9 +2321,7 @@ uvideo_vs_decode_stream_header(struct uv
> #endif
> if (sc->sc_mmap_flag) {
> /* mmap */
> - if (uvideo_mmap_queue(sc, fb->buf, fb->offset,
> - fb->error))
> - ret = USBD_NOMEM;
> + 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__);
> @@ -2341,8 +2334,6 @@ uvideo_vs_decode_stream_header(struct uv
> fb->fid = 0;
> fb->error = 0;
> }
> -
> - return (ret);
> }
>
> /*
> @@ -2361,13 +2352,12 @@ uvideo_vs_decode_stream_header(struct uv
> * Sometimes the stream header is prefixed by a unknown byte. Therefore
> * we check for the magic value on two offsets.
> */
> -usbd_status
> +void
> uvideo_vs_decode_stream_header_isight(struct uvideo_softc *sc, uint8_t *frame,
> int frame_size)
> {
> struct uvideo_frame_buffer *fb = &sc->sc_frame_buffer;
> int sample_len, header = 0;
> - usbd_status ret = USBD_NORMAL_COMPLETION;
> uint8_t magic[] = {
> 0x11, 0x22, 0x33, 0x44,
> 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xfa, 0xce };
> @@ -2379,14 +2369,13 @@ uvideo_vs_decode_stream_header_isight(st
>
> if (header && fb->fid == 0) {
> fb->fid = 1;
> - return (USBD_NORMAL_COMPLETION);
> + return;
> }
>
> if (header) {
> if (sc->sc_mmap_flag) {
> /* mmap */
> - if (uvideo_mmap_queue(sc, fb->buf, fb->offset, 0))
> - ret = USBD_NOMEM;
> + uvideo_mmap_queue(sc, fb->buf, fb->offset, 0);
> } else {
> /* read */
> uvideo_read(sc, fb->buf, fb->offset);
> @@ -2400,11 +2389,9 @@ uvideo_vs_decode_stream_header_isight(st
> fb->offset += sample_len;
> }
> }
> -
> - return (ret);
> }
>
> -int
> +void
> uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err)
> {
> int i;
> @@ -2420,7 +2407,7 @@ uvideo_mmap_queue(struct uvideo_softc *s
> if (i == sc->sc_mmap_count) {
> DPRINTF(1, "%s: %s: mmap queue is full!\n",
> DEVNAME(sc), __func__);
> - return ENOMEM;
> + return;
> }
>
> /* copy frame to mmap buffer and report length */
> @@ -2454,8 +2441,6 @@ uvideo_mmap_queue(struct uvideo_softc *s
> * ready to dequeue.
> */
> sc->sc_uplayer_intr(sc->sc_uplayer_arg);
> -
> - return 0;
> }
>
> void
--
wbr, Kirill
sys/uvideo: treat frame as done when mmap queue is overflowed
sys/uvideo: treat frame as done when mmap queue is overflowed
sys/uvideo: treat frame as done when mmap queue is overflowed