On Tue, May 17, 2016 at 05:20:14PM -0700, patrick keshishian wrote: > 4/4: I don't believe V4L2_BUF_FLAG_QUEUED and V4L2_BUF_FLAG_DONE > flags are handled correctly in our uvideo driver.
Agreed. > According to linuxtv.org Buffers Chap 3. Input/Output Table 3.4 > Buffer Flags: > > https://www.linuxtv.org/downloads/legacy/video4linux/API/V4L2_API/spec-single/v4l2.html#buffer-flags > > V4L2_BUF_FLAG_QUEUED > [The buffer] automatically moves to the outgoing queue > after the buffer has been filled or displayed. Drivers > set or clear this flag when the _QUERYBUF ioctl is > called. > After (successful) calling the _QBUF ioctl it is always > set and after _DQBUF alwyas cleared > > V4L2_BUF_FLAG_DONE > When this flag is set, the buffer is currently on the > outgoing queue, ready to be dequeued from the driver. > Drivers set or clear this flag when the _QUERYBUF ioctl > is called. > After calling the _QBUF or _DQBUF it is always cleared. > Of course a buffer cannot be on both queues at the same > time, the _QUEUED and _DONE flag are mutually exclusive. > They can be both cleared however, then the buffer is in > "dequeued" state, in the application domain so to say. > > This patch I'm a bit lukewarm on. > > It gets rid of sc_mmap_cur used mainly in uvideo_mmap_queue() > looking to find a buffer which can be "queued". > > Since the _QUEUE flag is never cleared in this driver, is it > potentially reusing a buffer which the client/consumer may not > yet be done with? > > Also, I couldn't convince myself that the while()-loop searching > for this buffer, once the _QUEUE flag clearing is fixed, would > not end up in a situation where it would equal to sc_mmap_count > and never get past returning ENOMEM. > > I think (but am not sure) the intent of sc_mmap_cur is to give > each buffer a fair chance at getting used. Where, w/o it, and > a fast client/consumer, it is possible that only the first few > buffers would get all the attention. > > Or maybe I'm reading all this incorrectly. > > Thoughts? I think by fixing the flags it's right to get rid of sc_mmap_cur and query the buffer queue just based on the flags. Indeed this will lead to less buffers beeing used. I did some tests and up to a resolution of 800x600 just one buffer got used. With 1600x1200 I could see 4 buffers beeing used. But this is fine IMO. When the client is fast enough to fetch the buffers it's ok when the driver doesn't need to build up a long queue. Also it gives us more transparency about under which conditions the driver queue gets utilized. I made some inline comments and attached an updated diff based on the comments. > Index: uvideo.c > =================================================================== > RCS file: /cvs/obsd/src/sys/dev/usb/uvideo.c,v > retrieving revision 1.185 > diff -u -p -u -p -r1.185 uvideo.c > --- uvideo.c 17 May 2016 08:27:17 -0000 1.185 > +++ uvideo.c 17 May 2016 23:08:32 -0000 > @@ -78,7 +78,6 @@ struct uvideo_softc { > size_t sc_mmap_buffer_size; > q_mmap sc_mmap_q; > int sc_mmap_count; > - int sc_mmap_cur; > int sc_mmap_flag; > > struct vnode *sc_vp; > @@ -590,7 +589,6 @@ uvideo_attach_hook(struct device *self) > > /* init mmap queue */ > SIMPLEQ_INIT(&sc->sc_mmap_q); > - sc->sc_mmap_cur = -1; > sc->sc_mmap_count = 0; > > DPRINTF(1, "uvideo_attach: doing video_attach_mi\n"); > @@ -1695,7 +1693,6 @@ uvideo_vs_free_frame(struct uvideo_softc > while (!SIMPLEQ_EMPTY(&sc->sc_mmap_q)) > SIMPLEQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames); > > - sc->sc_mmap_cur = -1; > sc->sc_mmap_count = 0; > } > > @@ -2208,44 +2205,45 @@ uvideo_vs_decode_stream_header_isight(st > } > > int > +uvideo_find_queued(struct uvideo_softc *sc) > +{ > + int i; > + for (i = 0; i < sc->sc_mmap_count; ++i) { > + if (sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_DONE) > + continue; > + if (sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_QUEUED) > + break; > + } > + return (i == sc->sc_mmap_count) ? -1 : i; > +} I don't think we need to introduce a new function to query a free buffer and can leave it in uvideo_mmap_queue() instead. Also I think it's not required to additionally check for V4L2_BUF_FLAG_DONE. Either V4L2_BUF_FLAG_QUEUED is set or not. > +int > uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len) > { > - if (sc->sc_mmap_cur < 0 || sc->sc_mmap_count == 0 || > - sc->sc_mmap_buffer == NULL) > + 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 */ > - while (sc->sc_mmap_cur < sc->sc_mmap_count) { > - if (sc->sc_mmap[sc->sc_mmap_cur].v4l2_buf.flags & > - V4L2_BUF_FLAG_QUEUED) > - break; > - /* not ready for queueing, try next */ > - sc->sc_mmap_cur++; > - } > - if (sc->sc_mmap_cur == sc->sc_mmap_count) { > - DPRINTF(1, "%s: %s: mmap queue is full!", > - DEVNAME(sc), __func__); > + if (-1 == (i = uvideo_find_queued(sc))) > return ENOMEM; > - } > > /* copy frame to mmap buffer and report length */ > - bcopy(buf, sc->sc_mmap[sc->sc_mmap_cur].buf, len); > - sc->sc_mmap[sc->sc_mmap_cur].v4l2_buf.bytesused = len; > + bcopy(buf, sc->sc_mmap[i].buf, len); > + sc->sc_mmap[i].v4l2_buf.bytesused = len; > > /* timestamp it */ > - getmicrotime(&sc->sc_mmap[sc->sc_mmap_cur].v4l2_buf.timestamp); > + getmicrotime(&sc->sc_mmap[i].v4l2_buf.timestamp); > + > + /* appropriately set/clear flags */ > + sc->sc_mmap[i].v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED; > + sc->sc_mmap[i].v4l2_buf.flags |= V4L2_BUF_FLAG_DONE; I would move the flags setting just below the "/* queue it */" comment. > /* queue it */ > - SIMPLEQ_INSERT_TAIL(&sc->sc_mmap_q, &sc->sc_mmap[sc->sc_mmap_cur], > - q_frames); > + 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__, sc->sc_mmap_cur); > - > - /* point to next mmap buffer */ > - sc->sc_mmap_cur++; > - if (sc->sc_mmap_cur == sc->sc_mmap_count) > - /* we reached the end of the mmap buffer, start over */ > - sc->sc_mmap_cur = 0; > + DEVNAME(sc), __func__, i); > > wakeup(sc); > > @@ -3214,9 +3212,6 @@ uvideo_reqbufs(void *v, struct v4l2_requ > /* tell how many buffers we have really allocated */ > rb->count = sc->sc_mmap_count; > > - /* start with the first buffer */ > - sc->sc_mmap_cur = 0; > - > return (0); > } > > @@ -3286,7 +3281,8 @@ uvideo_dqbuf(void *v, struct v4l2_buffer > > bcopy(&mmap->v4l2_buf, dqb, sizeof(struct v4l2_buffer)); > > - mmap->v4l2_buf.flags |= V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_DONE; > + mmap->v4l2_buf.flags &= ~(V4L2_BUF_FLAG_DONE|V4L2_BUF_FLAG_QUEUED); > + mmap->v4l2_buf.flags |= V4L2_BUF_FLAG_MAPPED; I think we also can remove the V4L2_BUF_FLAG_MAPPED flag from uvideo_dqbuf() and uvideo_qbuf(). As your first diff has fixed it, it already gets set in uvideo_reqbufs() and it's not required to reset it again. Also for a better readability I think one line per flag is in favour. > DPRINTF(2, "%s: %s: frame dequeued from index %d\n", > DEVNAME(sc), __func__, mmap->v4l2_buf.index); Index: uvideo.c =================================================================== RCS file: /cvs/src/sys/dev/usb/uvideo.c,v retrieving revision 1.188 diff -u -p -u -p -r1.188 uvideo.c --- uvideo.c 28 May 2016 06:26:49 -0000 1.188 +++ uvideo.c 31 May 2016 20:14:11 -0000 @@ -78,7 +78,6 @@ struct uvideo_softc { size_t sc_mmap_buffer_size; q_mmap sc_mmap_q; int sc_mmap_count; - int sc_mmap_cur; int sc_mmap_flag; struct vnode *sc_vp; @@ -590,7 +589,6 @@ uvideo_attach_hook(struct device *self) /* init mmap queue */ SIMPLEQ_INIT(&sc->sc_mmap_q); - sc->sc_mmap_cur = -1; sc->sc_mmap_count = 0; DPRINTF(1, "uvideo_attach: doing video_attach_mi\n"); @@ -1693,7 +1691,6 @@ uvideo_vs_free_frame(struct uvideo_softc while (!SIMPLEQ_EMPTY(&sc->sc_mmap_q)) SIMPLEQ_REMOVE_HEAD(&sc->sc_mmap_q, q_frames); - sc->sc_mmap_cur = -1; sc->sc_mmap_count = 0; } @@ -2206,42 +2203,35 @@ uvideo_vs_decode_stream_header_isight(st int uvideo_mmap_queue(struct uvideo_softc *sc, uint8_t *buf, int len) { - if (sc->sc_mmap_cur < 0 || sc->sc_mmap_count == 0 || - sc->sc_mmap_buffer == NULL) + 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 */ - while (sc->sc_mmap_cur < sc->sc_mmap_count) { - if (sc->sc_mmap[sc->sc_mmap_cur].v4l2_buf.flags & - V4L2_BUF_FLAG_QUEUED) + for (i = 0; i < sc->sc_mmap_count; i++) { + if (sc->sc_mmap[i].v4l2_buf.flags & V4L2_BUF_FLAG_QUEUED) break; - /* not ready for queueing, try next */ - sc->sc_mmap_cur++; } - if (sc->sc_mmap_cur == sc->sc_mmap_count) { + if (i == sc->sc_mmap_count) { DPRINTF(1, "%s: %s: mmap queue is full!", DEVNAME(sc), __func__); return ENOMEM; } /* copy frame to mmap buffer and report length */ - bcopy(buf, sc->sc_mmap[sc->sc_mmap_cur].buf, len); - sc->sc_mmap[sc->sc_mmap_cur].v4l2_buf.bytesused = len; + bcopy(buf, sc->sc_mmap[i].buf, len); + sc->sc_mmap[i].v4l2_buf.bytesused = len; /* timestamp it */ - getmicrotime(&sc->sc_mmap[sc->sc_mmap_cur].v4l2_buf.timestamp); + getmicrotime(&sc->sc_mmap[i].v4l2_buf.timestamp); /* queue it */ - SIMPLEQ_INSERT_TAIL(&sc->sc_mmap_q, &sc->sc_mmap[sc->sc_mmap_cur], - q_frames); + 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__, sc->sc_mmap_cur); - - /* point to next mmap buffer */ - sc->sc_mmap_cur++; - if (sc->sc_mmap_cur == sc->sc_mmap_count) - /* we reached the end of the mmap buffer, start over */ - sc->sc_mmap_cur = 0; + DEVNAME(sc), __func__, i); wakeup(sc); @@ -3210,9 +3200,6 @@ uvideo_reqbufs(void *v, struct v4l2_requ /* tell how many buffers we have really allocated */ rb->count = sc->sc_mmap_count; - /* start with the first buffer */ - sc->sc_mmap_cur = 0; - return (0); } @@ -3249,7 +3236,6 @@ uvideo_qbuf(void *v, struct v4l2_buffer return (EINVAL); sc->sc_mmap[qb->index].v4l2_buf.flags &= ~V4L2_BUF_FLAG_DONE; - sc->sc_mmap[qb->index].v4l2_buf.flags |= V4L2_BUF_FLAG_MAPPED; sc->sc_mmap[qb->index].v4l2_buf.flags |= V4L2_BUF_FLAG_QUEUED; DPRINTF(2, "%s: %s: buffer on index %d ready for queueing\n", @@ -3282,7 +3268,8 @@ uvideo_dqbuf(void *v, struct v4l2_buffer bcopy(&mmap->v4l2_buf, dqb, sizeof(struct v4l2_buffer)); - mmap->v4l2_buf.flags |= V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_DONE; + mmap->v4l2_buf.flags &= ~V4L2_BUF_FLAG_DONE; + mmap->v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED; DPRINTF(2, "%s: %s: frame dequeued from index %d\n", DEVNAME(sc), __func__, mmap->v4l2_buf.index);
