Download raw body.
sys/uvideo: avoid one bcopy when reads into mmaped memory
On Sat, 01 Mar 2025 09:38:03 +0100,
Marcus Glocker <marcus@nazgul.ch> wrote:
>
> Though, I would prefer if we could do the selection of the buffer in
> the isoc case before we enter the isoc frame loop, instead of calling
> the buffer selection function every time in the stream header function.
>
Indeed, it make sence and should decrease wasting resources when frame will
be droped anyway.
> Also, I would leave the V4L2_BUF_FLAG_QUEUED flagging for when we
> insert the new frame in to our queue.
>
Linux kernel has such description for this flag:
Internally drivers maintain two buffer queues, an incoming and
outgoing queue. When this flag is set, the buffer is currently on
the incoming queue. It automatically moves to the outgoing queue
after the buffer has been filled (capture devices) or displayed
(output devices). Drivers set or clear this flag when the
VIDIOC_QUERYBUF ioctl is called. After (successful) calling the
VIDIOC_QBUF ioctl it is always set and after VIDIOC_DQBUF always
cleared
it looks for me as a kind of mutex which is used to mark a buffer as busy
for incomming data, and to prevent it from be used for something else.
From another hand uvideo_qbuf never updates flags on user land's v4l2_buf.
So, from application point of view it doesn't matter when we update flags.
Probably we need cleanit up, but it should be done as dedicated step.
> Here is a slightly adapted version of your diff, which considers the
> above points, as an alternative.
>
> I can't test the bulk case here currently ...
>
I OK kirill@ with your version.
I also tested on a bulk device Jabra PanaCast 20, no new regression.
>
> Index: sys/dev/usb/uvideo.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
> diff -u -p -u -p -r1.246 uvideo.c
> --- sys/dev/usb/uvideo.c 26 Feb 2025 21:03:52 -0000 1.246
> +++ sys/dev/usb/uvideo.c 1 Mar 2025 08:26:11 -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;
> q_mmap sc_mmap_q;
> @@ -103,7 +104,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 *),
> @@ -168,10 +169,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);
> -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);
> @@ -2196,6 +2198,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) {
> @@ -2222,7 +2225,14 @@ 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) {
> + 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);
> }
> usbd_ref_decr(sc->sc_udev);
>
> @@ -2276,7 +2286,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__);
>
> @@ -2291,6 +2301,12 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
> if (len == 0)
> goto skip;
>
> + if (sc->sc_mmap_flag) {
> + if ((buf = uvideo_mmap_getbuf(sc)) == NULL)
> + goto skip;
> + } else
> + buf = sc->sc_frame_buffer.buf;
> +
> for (i = 0; i < sc->sc_nframes; i++) {
> frame = ixfer->buf + (i * sc->sc_vs_cur->psize);
> frame_size = ixfer->size[i];
> @@ -2299,7 +2315,7 @@ uvideo_vs_cb(struct usbd_xfer *xfer, voi
> /* frame is empty */
> continue;
>
> - sc->sc_decode_stream_header(sc, frame, frame_size);
> + sc->sc_decode_stream_header(sc, frame, frame_size, buf);
> }
>
> skip: /* setup new transfer */
> @@ -2308,7 +2324,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;
> @@ -2371,7 +2387,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;
> }
>
> @@ -2394,7 +2410,7 @@ uvideo_vs_decode_stream_header(struct uv
> #endif
> 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__);
> @@ -2427,7 +2443,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;
> @@ -2448,7 +2464,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);
> @@ -2458,14 +2474,14 @@ 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;
> }
> }
> }
>
> -void
> -uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len, int err)
> +uint8_t *
> +uvideo_mmap_getbuf(struct uvideo_softc *sc)
> {
> int i;
>
> @@ -2480,32 +2496,39 @@ 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;
> + return NULL;
> }
>
> + sc->sc_mmap_cur = &sc->sc_mmap[i];
> +
> + return sc->sc_mmap_cur->buf;
> +}
> +
> +void
> +uvideo_mmap_queue(struct uvideo_softc *sc, int len, int err)
> +{
> /* copy frame to mmap buffer and report length */
> - bcopy(buf, sc->sc_mmap[i].buf, len);
> - sc->sc_mmap[i].v4l2_buf.bytesused = len;
> + 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);
>
>
--
wbr, Kirill
sys/uvideo: avoid one bcopy when reads into mmaped memory