4/4: I don't believe V4L2_BUF_FLAG_QUEUED and V4L2_BUF_FLAG_DONE
flags are handled correctly in our uvideo driver.
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?
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;
+}
+
+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;
/* 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;
DPRINTF(2, "%s: %s: frame dequeued from index %d\n",
DEVNAME(sc), __func__, mmap->v4l2_buf.index);
