[REVIEWv2 PATCH 00/15] vb2: fixes, balancing callbacks (PART 1)

[REVIEWv2 PATCH 00/15] vb2: fixes, balancing callbacks (PART 1)

[Hans Verkuil]

My previous patch series had a bug in patch 12/14 and I found a new bug as
well that had to be fixed.

Changes since REVIEWv1:

- patch 12/14 has been replaced with a new patch that really fixes the problem
- a new patch 14 was added to fix a streamoff problem.

This patch series is the second REVIEW series as opposed to an RFC
series. It follows RFCv4:

http://www.spinics.net/lists/linux-media/msg73039.html

This is part 1 of vb2 changes. Part 2 has already been posted.

Ignore patches 1-3: the first is already merged in 3.14, and 2 and 3
are pending to be merged in 3.14. But you need them for some follow-up
patches.

Patches 04-09 and 15 are unchanged from RFCv4.

Changelog since RFCv4:

- Dropped RFCv4 patch 08/11 as it was wrong. Instead, I added patch
  10/14. This fixes a bug in __vb2_queue_free() and improved the code
  that caused my misunderstanding with RFCv4 patch 08/11.
- In patch 11/14 I dropped the check against the minimum number of
  required buffers in create_bufs. Instead, check for this in streamon.
- Added patch 12: "vb2: properly clean up PREPARED and QUEUED buffers"
  This fixes one corner case that still produced 'Unbalanced' results.
- Added patch 13: "vb2: replace BUG by WARN_ON"
  Just a small change to be more gentle if the driver tries something
  it shouldn't.

This patch series fixes a series of bugs in vb2. Recently I have been
converting the saa7134 driver to vb2 and as part of that work I discovered
that the op calls were not properly balanced, which caused saa7134 to
fail.

Based on that I did more debugging and I found lots of different issues
with vb2 when it comes to balancing ops. This patch series fixes them.
Many thanks to Pawel Osciak for a good brainstorm session.

Patch 4 adds debugging code to check for unbalanced calls. I used this
when testing since without this it is very hard to verify correctness.
It is currently turned on when CONFIG_VIDEO_ADV_DEBUG is set, but perhaps
this should be placed under a vb2 specific debug option?

The next patch changes the buf_finish return type to void. It must not
fail, and you can't do anything useful if it does anyway.

Patch 6 just improves some comments.

Patches 7 and 8 fix several unbalanced ops.

Patch 9 just renames queue_count to owned_by_drv_count. The old name
suggests the number of buffers in the queue_list, but that's not what
it is. Since the next patch will actually add a real count for the
number of buffers in the queue_list I decided to just rename this,
thus freeing up the name 'queue_count'.

Patch 10 fixes a __vb2_queue_free() bug and makes __reqbufs and
__create_bufs code a bit more understandable.

Patch 11 fixes a bug in the handling of start_streaming: before that op
is called any prequeued buffers are handed over to the driver. But if
start_streaming returns an error, then those buffers are not reclaimed.
Since start_streaming failed, q->streaming is 0, so stop_streaming isn't
called either.

There are two reasons for an error in start_streaming: either a real
error when setting up the DMA engine occurred or there were not enough
buffers queued for the DMA engine to start (start_streaming returns
-ENOBUFS in that case). It makes sense to require that drivers return
queued buffers back to vb2 in case of an error, but not if they also 
have to do that in case of insufficient buffers. So this patch replaces
the -ENOBUFS mechanism by a vb2_queue field that tells vb2 what the
minimum number of buffers is.

Now if start_streaming returns an error the vb2 core will check if there
are still buffers owned by the driver and if so produce a warning and
reclaim those buffers. The same is done when the vb2_queue is freed.

This ensures that the prepare/finish memops are correctly called and
the state of all the buffers is consistent.

Patch 12 fixes another corner case introducing unbalanced ops.

Patch 13 replaces a BUG_ON by WARN_ON.

Patch 14 fixes a problem with STREAMOFF that doesn't do what you expect
if STREAMON hasn't been called but buffers have been prepared or queued.

Patch 15 fixes vivi for this start_streaming issue. Note that there are
many drivers that do not clean up properly in case of an error during
start_streaming.

I have been testing these vb2 changes with vivi (vmalloc based), a patched
saa7134 (dma-contig based) and an out-of-tree PCI driver (dma-sg based),
with the mmap/userptr/dmabuf and read/write streaming modes.

Regards,

        Hans

[REVIEWv2 PATCH 01/15] vb2: Check if there are buffers before streamon

[Hans Verkuil]

From: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>

This patch adds a test preventing streamon() if there is no buffer
ready.

Without this patch, a user could call streamon() before
preparing any buffer. This leads to a situation where if he calls
close() before calling streamoff() the device is kept streaming.

Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 5a5fb7f..a127925 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1776,6 +1776,11 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 		return 0;
 	}
 
+	if (!q->num_buffers) {
+		dprintk(1, "streamon: no buffers have been allocated\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * If any buffers were queued before streamon,
 	 * we can now pass them to driver for processing.
-- 
1.9.0

[REVIEWv2 PATCH 02/15] vb2: fix read/write regression

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Commit 88e268702bfba78448abd20a31129458707383aa ("vb2: Improve file I/O
emulation to handle buffers in any order") broke read/write support if
the size of the buffer being read/written is less than the size of the
image.

When the commit was tested originally I used qv4l2, which calls read()
with exactly the size of the image. But if you try 'cat /dev/video0'
then it will fail and typically hang after reading two buffers.

This patch fixes the behavior by adding a new cur_index field that
contains the index of the field currently being filled/read, or it
is num_buffers in which case a new buffer needs to be dequeued.

The old index field has been renamed to initial_index in order to be
a bit more descriptive.

This has been tested with both read and write.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Andy Walls <awalls@md.metrocast.net>
---
 drivers/media/v4l2-core/videobuf2-core.c | 46 +++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index a127925..f1a2857c 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2251,6 +2251,22 @@ struct vb2_fileio_buf {
 /**
  * struct vb2_fileio_data - queue context used by file io emulator
  *
+ * @cur_index:	the index of the buffer currently being read from or
+ *		written to. If equal to q->num_buffers then a new buffer
+ *		must be dequeued.
+ * @initial_index: in the read() case all buffers are queued up immediately
+ *		in __vb2_init_fileio() and __vb2_perform_fileio() just cycles
+ *		buffers. However, in the write() case no buffers are initially
+ *		queued, instead whenever a buffer is full it is queued up by
+ *		__vb2_perform_fileio(). Only once all available buffers have
+ *		been queued up will __vb2_perform_fileio() start to dequeue
+ *		buffers. This means that initially __vb2_perform_fileio()
+ *		needs to know what buffer index to use when it is queuing up
+ *		the buffers for the first time. That initial index is stored
+ *		in this field. Once it is equal to q->num_buffers all
+ *		available buffers have been queued and __vb2_perform_fileio()
+ *		should start the normal dequeue/queue cycle.
+ *
  * vb2 provides a compatibility layer and emulator of file io (read and
  * write) calls on top of streaming API. For proper operation it required
  * this structure to save the driver state between each call of the read
@@ -2260,7 +2276,8 @@ struct vb2_fileio_data {
 	struct v4l2_requestbuffers req;
 	struct v4l2_buffer b;
 	struct vb2_fileio_buf bufs[VIDEO_MAX_FRAME];
-	unsigned int index;
+	unsigned int cur_index;
+	unsigned int initial_index;
 	unsigned int q_count;
 	unsigned int dq_count;
 	unsigned int flags;
@@ -2360,7 +2377,12 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 				goto err_reqbufs;
 			fileio->bufs[i].queued = 1;
 		}
-		fileio->index = q->num_buffers;
+		/*
+		 * All buffers have been queued, so mark that by setting
+		 * initial_index to q->num_buffers
+		 */
+		fileio->initial_index = q->num_buffers;
+		fileio->cur_index = q->num_buffers;
 	}
 
 	/*
@@ -2439,7 +2461,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 	/*
 	 * Check if we need to dequeue the buffer.
 	 */
-	index = fileio->index;
+	index = fileio->cur_index;
 	if (index >= q->num_buffers) {
 		/*
 		 * Call vb2_dqbuf to get buffer back.
@@ -2453,7 +2475,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 			return ret;
 		fileio->dq_count += 1;
 
-		index = fileio->b.index;
+		fileio->cur_index = index = fileio->b.index;
 		buf = &fileio->bufs[index];
 
 		/*
@@ -2529,8 +2551,20 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		buf->queued = 1;
 		buf->size = vb2_plane_size(q->bufs[index], 0);
 		fileio->q_count += 1;
-		if (fileio->index < q->num_buffers)
-			fileio->index++;
+		/*
+		 * If we are queuing up buffers for the first time, then
+		 * increase initial_index by one.
+		 */
+		if (fileio->initial_index < q->num_buffers)
+			fileio->initial_index++;
+		/*
+		 * The next buffer to use is either a buffer that's going to be
+		 * queued for the first time (initial_index < q->num_buffers)
+		 * or it is equal to q->num_buffers, meaning that the next
+		 * time we need to dequeue a buffer since we've now queued up
+		 * all the 'first time' buffers.
+		 */
+		fileio->cur_index = fileio->initial_index;
 	}
 
 	/*
-- 
1.9.0

[REVIEWv2 PATCH 03/15] vb2: fix PREPARE_BUF regression

[Hans Verkuil]

Fix an incorrect test in vb2_internal_qbuf() where only DEQUEUED buffers
are allowed. But PREPARED buffers are also OK.

Introduced by commit 4138111a27859dcc56a5592c804dd16bb12a23d1
("vb2: simplify qbuf/prepare_buf by removing callback").

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index f1a2857c..909f367 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1420,11 +1420,6 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 		return ret;
 
 	vb = q->bufs[b->index];
-	if (vb->state != VB2_BUF_STATE_DEQUEUED) {
-		dprintk(1, "%s(): invalid buffer state %d\n", __func__,
-			vb->state);
-		return -EINVAL;
-	}
 
 	switch (vb->state) {
 	case VB2_BUF_STATE_DEQUEUED:
@@ -1438,7 +1433,8 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 		dprintk(1, "qbuf: buffer still being prepared\n");
 		return -EINVAL;
 	default:
-		dprintk(1, "qbuf: buffer already in use\n");
+		dprintk(1, "%s(): invalid buffer state %d\n", __func__,
+			vb->state);
 		return -EINVAL;
 	}
 
-- 
1.9.0

[REVIEWv2 PATCH 04/15] vb2: add debugging code to check for unbalanced ops

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

When a vb2_queue is freed check if all the mem_ops and queue ops were balanced.
So the number of calls to e.g. buf_finish has to match the number of calls to
buf_prepare, etc.

This code is only enabled if CONFIG_VIDEO_ADV_DEBUG is set.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Pawel Osciak <pawel@osciak.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 233 ++++++++++++++++++++++++-------
 include/media/videobuf2-core.h           |  43 ++++++
 2 files changed, 226 insertions(+), 50 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 909f367..8f1578b 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -33,12 +33,63 @@ module_param(debug, int, 0644);
 			printk(KERN_DEBUG "vb2: " fmt, ## arg);		\
 	} while (0)
 
-#define call_memop(q, op, args...)					\
-	(((q)->mem_ops->op) ?						\
-		((q)->mem_ops->op(args)) : 0)
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+
+/*
+ * If advanced debugging is on, then count how often each op is called,
+ * which can either be per-buffer or per-queue.
+ *
+ * If the op failed then the 'fail_' variant is called to decrease the
+ * counter. That makes it easy to check that the 'init' and 'cleanup'
+ * (and variations thereof) stay balanced.
+ */
+
+#define call_memop(vb, op, args...)					\
+({									\
+	struct vb2_queue *_q = (vb)->vb2_queue;				\
+	dprintk(2, "call_memop(%p, %d, %s)%s\n",			\
+		_q, (vb)->v4l2_buf.index, #op,				\
+		_q->mem_ops->op ? "" : " (nop)");			\
+	(vb)->cnt_mem_ ## op++;						\
+	_q->mem_ops->op ? _q->mem_ops->op(args) : 0;			\
+})
+#define fail_memop(vb, op) ((vb)->cnt_mem_ ## op--)
+
+#define call_qop(q, op, args...)					\
+({									\
+	dprintk(2, "call_qop(%p, %s)%s\n", q, #op,			\
+		(q)->ops->op ? "" : " (nop)");				\
+	(q)->cnt_ ## op++;						\
+	(q)->ops->op ? (q)->ops->op(args) : 0;				\
+})
+#define fail_qop(q, op) ((q)->cnt_ ## op--)
+
+#define call_vb_qop(vb, op, args...)					\
+({									\
+	struct vb2_queue *_q = (vb)->vb2_queue;				\
+	dprintk(2, "call_vb_qop(%p, %d, %s)%s\n",			\
+		_q, (vb)->v4l2_buf.index, #op,				\
+		_q->ops->op ? "" : " (nop)");				\
+	(vb)->cnt_ ## op++;						\
+	_q->ops->op ? _q->ops->op(args) : 0;				\
+})
+#define fail_vb_qop(vb, op) ((vb)->cnt_ ## op--)
+
+#else
+
+#define call_memop(vb, op, args...)					\
+	((vb)->vb2_queue->mem_ops->op ? (vb)->vb2_queue->mem_ops->op(args) : 0)
+#define fail_memop(vb, op)
 
 #define call_qop(q, op, args...)					\
-	(((q)->ops->op) ? ((q)->ops->op(args)) : 0)
+	((q)->ops->op ? (q)->ops->op(args) : 0)
+#define fail_qop(q, op)
+
+#define call_vb_qop(vb, op, args...)					\
+	((vb)->vb2_queue->ops->op ? (vb)->vb2_queue->ops->op(args) : 0)
+#define fail_vb_qop(vb, op)
+
+#endif
 
 #define V4L2_BUFFER_MASK_FLAGS	(V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_QUEUED | \
 				 V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR | \
@@ -61,7 +112,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
 	for (plane = 0; plane < vb->num_planes; ++plane) {
 		unsigned long size = PAGE_ALIGN(q->plane_sizes[plane]);
 
-		mem_priv = call_memop(q, alloc, q->alloc_ctx[plane],
+		mem_priv = call_memop(vb, alloc, q->alloc_ctx[plane],
 				      size, q->gfp_flags);
 		if (IS_ERR_OR_NULL(mem_priv))
 			goto free;
@@ -73,9 +124,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
 
 	return 0;
 free:
+	fail_memop(vb, alloc);
 	/* Free already allocated memory if one of the allocations failed */
 	for (; plane > 0; --plane) {
-		call_memop(q, put, vb->planes[plane - 1].mem_priv);
+		call_memop(vb, put, vb->planes[plane - 1].mem_priv);
 		vb->planes[plane - 1].mem_priv = NULL;
 	}
 
@@ -87,11 +139,10 @@ free:
  */
 static void __vb2_buf_mem_free(struct vb2_buffer *vb)
 {
-	struct vb2_queue *q = vb->vb2_queue;
 	unsigned int plane;
 
 	for (plane = 0; plane < vb->num_planes; ++plane) {
-		call_memop(q, put, vb->planes[plane].mem_priv);
+		call_memop(vb, put, vb->planes[plane].mem_priv);
 		vb->planes[plane].mem_priv = NULL;
 		dprintk(3, "Freed plane %d of buffer %d\n", plane,
 			vb->v4l2_buf.index);
@@ -104,12 +155,11 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb)
  */
 static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
 {
-	struct vb2_queue *q = vb->vb2_queue;
 	unsigned int plane;
 
 	for (plane = 0; plane < vb->num_planes; ++plane) {
 		if (vb->planes[plane].mem_priv)
-			call_memop(q, put_userptr, vb->planes[plane].mem_priv);
+			call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
 		vb->planes[plane].mem_priv = NULL;
 	}
 }
@@ -118,15 +168,15 @@ static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
  * __vb2_plane_dmabuf_put() - release memory associated with
  * a DMABUF shared plane
  */
-static void __vb2_plane_dmabuf_put(struct vb2_queue *q, struct vb2_plane *p)
+static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p)
 {
 	if (!p->mem_priv)
 		return;
 
 	if (p->dbuf_mapped)
-		call_memop(q, unmap_dmabuf, p->mem_priv);
+		call_memop(vb, unmap_dmabuf, p->mem_priv);
 
-	call_memop(q, detach_dmabuf, p->mem_priv);
+	call_memop(vb, detach_dmabuf, p->mem_priv);
 	dma_buf_put(p->dbuf);
 	memset(p, 0, sizeof(*p));
 }
@@ -137,11 +187,10 @@ static void __vb2_plane_dmabuf_put(struct vb2_queue *q, struct vb2_plane *p)
  */
 static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb)
 {
-	struct vb2_queue *q = vb->vb2_queue;
 	unsigned int plane;
 
 	for (plane = 0; plane < vb->num_planes; ++plane)
-		__vb2_plane_dmabuf_put(q, &vb->planes[plane]);
+		__vb2_plane_dmabuf_put(vb, &vb->planes[plane]);
 }
 
 /**
@@ -246,10 +295,11 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
 			 * callback, if given. An error in initialization
 			 * results in queue setup failure.
 			 */
-			ret = call_qop(q, buf_init, vb);
+			ret = call_vb_qop(vb, buf_init, vb);
 			if (ret) {
 				dprintk(1, "Buffer %d %p initialization"
 					" failed\n", buffer, vb);
+				fail_vb_qop(vb, buf_init);
 				__vb2_buf_mem_free(vb);
 				kfree(vb);
 				break;
@@ -321,18 +371,77 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 	}
 
 	/* Call driver-provided cleanup function for each buffer, if provided */
-	if (q->ops->buf_cleanup) {
-		for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
-		     ++buffer) {
-			if (NULL == q->bufs[buffer])
-				continue;
-			q->ops->buf_cleanup(q->bufs[buffer]);
-		}
+	for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
+	     ++buffer) {
+		if (q->bufs[buffer])
+			call_vb_qop(q->bufs[buffer], buf_cleanup, q->bufs[buffer]);
 	}
 
 	/* Release video buffer memory */
 	__vb2_free_mem(q, buffers);
 
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	/*
+	 * Check that all the calls were balances during the life-time of this
+	 * queue. If not (or if the debug level is 1 or up), then dump the
+	 * counters to the kernel log.
+	 */
+	if (q->num_buffers) {
+		bool unbalanced = q->cnt_start_streaming != q->cnt_stop_streaming ||
+				  q->cnt_wait_prepare != q->cnt_wait_finish;
+
+		if (unbalanced || debug) {
+			pr_info("vb2: counters for queue %p:%s\n", q,
+				unbalanced ? " UNBALANCED!" : "");
+			pr_info("vb2:     setup: %u start_streaming: %u stop_streaming: %u\n",
+				q->cnt_queue_setup, q->cnt_start_streaming,
+				q->cnt_stop_streaming);
+			pr_info("vb2:     wait_prepare: %u wait_finish: %u\n",
+				q->cnt_wait_prepare, q->cnt_wait_finish);
+		}
+		q->cnt_queue_setup = 0;
+		q->cnt_wait_prepare = 0;
+		q->cnt_wait_finish = 0;
+		q->cnt_start_streaming = 0;
+		q->cnt_stop_streaming = 0;
+	}
+	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
+		struct vb2_buffer *vb = q->bufs[buffer];
+		bool unbalanced = vb->cnt_mem_alloc != vb->cnt_mem_put ||
+				  vb->cnt_mem_prepare != vb->cnt_mem_finish ||
+				  vb->cnt_mem_get_userptr != vb->cnt_mem_put_userptr ||
+				  vb->cnt_mem_attach_dmabuf != vb->cnt_mem_detach_dmabuf ||
+				  vb->cnt_mem_map_dmabuf != vb->cnt_mem_unmap_dmabuf ||
+				  vb->cnt_buf_queue != vb->cnt_buf_done ||
+				  vb->cnt_buf_prepare != vb->cnt_buf_finish ||
+				  vb->cnt_buf_init != vb->cnt_buf_cleanup;
+
+		if (unbalanced || debug) {
+			pr_info("vb2:   counters for queue %p, buffer %d:%s\n",
+				q, buffer, unbalanced ? " UNBALANCED!" : "");
+			pr_info("vb2:     buf_init: %u buf_cleanup: %u buf_prepare: %u buf_finish: %u\n",
+				vb->cnt_buf_init, vb->cnt_buf_cleanup,
+				vb->cnt_buf_prepare, vb->cnt_buf_finish);
+			pr_info("vb2:     buf_queue: %u buf_done: %u\n",
+				vb->cnt_buf_queue, vb->cnt_buf_done);
+			pr_info("vb2:     alloc: %u put: %u prepare: %u finish: %u mmap: %u\n",
+				vb->cnt_mem_alloc, vb->cnt_mem_put,
+				vb->cnt_mem_prepare, vb->cnt_mem_finish,
+				vb->cnt_mem_mmap);
+			pr_info("vb2:     get_userptr: %u put_userptr: %u\n",
+				vb->cnt_mem_get_userptr, vb->cnt_mem_put_userptr);
+			pr_info("vb2:     attach_dmabuf: %u detach_dmabuf: %u map_dmabuf: %u unmap_dmabuf: %u\n",
+				vb->cnt_mem_attach_dmabuf, vb->cnt_mem_detach_dmabuf,
+				vb->cnt_mem_map_dmabuf, vb->cnt_mem_unmap_dmabuf);
+			pr_info("vb2:     get_dmabuf: %u num_users: %u vaddr: %u cookie: %u\n",
+				vb->cnt_mem_get_dmabuf,
+				vb->cnt_mem_num_users,
+				vb->cnt_mem_vaddr,
+				vb->cnt_mem_cookie);
+		}
+	}
+#endif
+
 	/* Free videobuf buffers */
 	for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
 	     ++buffer) {
@@ -424,7 +533,7 @@ static bool __buffer_in_use(struct vb2_queue *q, struct vb2_buffer *vb)
 		 * case anyway. If num_users() returns more than 1,
 		 * we are not the only user of the plane's memory.
 		 */
-		if (mem_priv && call_memop(q, num_users, mem_priv) > 1)
+		if (mem_priv && call_memop(vb, num_users, mem_priv) > 1)
 			return true;
 	}
 	return false;
@@ -703,8 +812,10 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 	 */
 	ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes,
 		       q->plane_sizes, q->alloc_ctx);
-	if (ret)
+	if (ret) {
+		fail_qop(q, queue_setup);
 		return ret;
+	}
 
 	/* Finally, allocate buffers and video memory */
 	ret = __vb2_queue_alloc(q, req->memory, num_buffers, num_planes);
@@ -723,6 +834,8 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 
 		ret = call_qop(q, queue_setup, q, NULL, &num_buffers,
 			       &num_planes, q->plane_sizes, q->alloc_ctx);
+		if (ret)
+			fail_qop(q, queue_setup);
 
 		if (!ret && allocated_buffers < num_buffers)
 			ret = -ENOMEM;
@@ -803,8 +916,10 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create
 	 */
 	ret = call_qop(q, queue_setup, q, &create->format, &num_buffers,
 		       &num_planes, q->plane_sizes, q->alloc_ctx);
-	if (ret)
+	if (ret) {
+		fail_qop(q, queue_setup);
 		return ret;
+	}
 
 	/* Finally, allocate buffers and video memory */
 	ret = __vb2_queue_alloc(q, create->memory, num_buffers,
@@ -828,6 +943,8 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create
 		 */
 		ret = call_qop(q, queue_setup, q, &create->format, &num_buffers,
 			       &num_planes, q->plane_sizes, q->alloc_ctx);
+		if (ret)
+			fail_qop(q, queue_setup);
 
 		if (!ret && allocated_buffers < num_buffers)
 			ret = -ENOMEM;
@@ -882,12 +999,10 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
  */
 void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no)
 {
-	struct vb2_queue *q = vb->vb2_queue;
-
 	if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
 		return NULL;
 
-	return call_memop(q, vaddr, vb->planes[plane_no].mem_priv);
+	return call_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
 
 }
 EXPORT_SYMBOL_GPL(vb2_plane_vaddr);
@@ -905,12 +1020,10 @@ EXPORT_SYMBOL_GPL(vb2_plane_vaddr);
  */
 void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int plane_no)
 {
-	struct vb2_queue *q = vb->vb2_queue;
-
 	if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
 		return NULL;
 
-	return call_memop(q, cookie, vb->planes[plane_no].mem_priv);
+	return call_memop(vb, cookie, vb->planes[plane_no].mem_priv);
 }
 EXPORT_SYMBOL_GPL(vb2_plane_cookie);
 
@@ -938,12 +1051,19 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 	if (state != VB2_BUF_STATE_DONE && state != VB2_BUF_STATE_ERROR)
 		return;
 
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	/*
+	 * Although this is not a callback, it still does have to balance
+	 * with the buf_queue op. So update this counter manually.
+	 */
+	vb->cnt_buf_done++;
+#endif
 	dprintk(4, "Done processing on buffer %d, state: %d\n",
 			vb->v4l2_buf.index, state);
 
 	/* sync buffers */
 	for (plane = 0; plane < vb->num_planes; ++plane)
-		call_memop(q, finish, vb->planes[plane].mem_priv);
+		call_memop(vb, finish, vb->planes[plane].mem_priv);
 
 	/* Add the buffer to the done buffers list */
 	spin_lock_irqsave(&q->done_lock, flags);
@@ -1067,19 +1187,20 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 
 		/* Release previously acquired memory if present */
 		if (vb->planes[plane].mem_priv)
-			call_memop(q, put_userptr, vb->planes[plane].mem_priv);
+			call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
 
 		vb->planes[plane].mem_priv = NULL;
 		vb->v4l2_planes[plane].m.userptr = 0;
 		vb->v4l2_planes[plane].length = 0;
 
 		/* Acquire each plane's memory */
-		mem_priv = call_memop(q, get_userptr, q->alloc_ctx[plane],
+		mem_priv = call_memop(vb, get_userptr, q->alloc_ctx[plane],
 				      planes[plane].m.userptr,
 				      planes[plane].length, write);
 		if (IS_ERR_OR_NULL(mem_priv)) {
 			dprintk(1, "qbuf: failed acquiring userspace "
 						"memory for plane %d\n", plane);
+			fail_memop(vb, get_userptr);
 			ret = mem_priv ? PTR_ERR(mem_priv) : -EINVAL;
 			goto err;
 		}
@@ -1090,9 +1211,10 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 	 * Call driver-specific initialization on the newly acquired buffer,
 	 * if provided.
 	 */
-	ret = call_qop(q, buf_init, vb);
+	ret = call_vb_qop(vb, buf_init, vb);
 	if (ret) {
 		dprintk(1, "qbuf: buffer initialization failed\n");
+		fail_vb_qop(vb, buf_init);
 		goto err;
 	}
 
@@ -1108,7 +1230,7 @@ err:
 	/* In case of errors, release planes that were already acquired */
 	for (plane = 0; plane < vb->num_planes; ++plane) {
 		if (vb->planes[plane].mem_priv)
-			call_memop(q, put_userptr, vb->planes[plane].mem_priv);
+			call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
 		vb->planes[plane].mem_priv = NULL;
 		vb->v4l2_planes[plane].m.userptr = 0;
 		vb->v4l2_planes[plane].length = 0;
@@ -1173,14 +1295,15 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		dprintk(1, "qbuf: buffer for plane %d changed\n", plane);
 
 		/* Release previously acquired memory if present */
-		__vb2_plane_dmabuf_put(q, &vb->planes[plane]);
+		__vb2_plane_dmabuf_put(vb, &vb->planes[plane]);
 		memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane));
 
 		/* Acquire each plane's memory */
-		mem_priv = call_memop(q, attach_dmabuf, q->alloc_ctx[plane],
+		mem_priv = call_memop(vb, attach_dmabuf, q->alloc_ctx[plane],
 			dbuf, planes[plane].length, write);
 		if (IS_ERR(mem_priv)) {
 			dprintk(1, "qbuf: failed to attach dmabuf\n");
+			fail_memop(vb, attach_dmabuf);
 			ret = PTR_ERR(mem_priv);
 			dma_buf_put(dbuf);
 			goto err;
@@ -1195,10 +1318,11 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 	 * the buffer(s)..
 	 */
 	for (plane = 0; plane < vb->num_planes; ++plane) {
-		ret = call_memop(q, map_dmabuf, vb->planes[plane].mem_priv);
+		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
 		if (ret) {
 			dprintk(1, "qbuf: failed to map dmabuf for plane %d\n",
 				plane);
+			fail_memop(vb, map_dmabuf);
 			goto err;
 		}
 		vb->planes[plane].dbuf_mapped = 1;
@@ -1208,9 +1332,10 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 	 * Call driver-specific initialization on the newly acquired buffer,
 	 * if provided.
 	 */
-	ret = call_qop(q, buf_init, vb);
+	ret = call_vb_qop(vb, buf_init, vb);
 	if (ret) {
 		dprintk(1, "qbuf: buffer initialization failed\n");
+		fail_vb_qop(vb, buf_init);
 		goto err;
 	}
 
@@ -1242,9 +1367,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
 
 	/* sync buffers */
 	for (plane = 0; plane < vb->num_planes; ++plane)
-		call_memop(q, prepare, vb->planes[plane].mem_priv);
+		call_memop(vb, prepare, vb->planes[plane].mem_priv);
 
-	q->ops->buf_queue(vb);
+	call_vb_qop(vb, buf_queue, vb);
 }
 
 static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
@@ -1295,8 +1420,11 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		ret = -EINVAL;
 	}
 
-	if (!ret)
-		ret = call_qop(q, buf_prepare, vb);
+	if (!ret) {
+		ret = call_vb_qop(vb, buf_prepare, vb);
+		if (ret)
+			fail_vb_qop(vb, buf_prepare);
+	}
 	if (ret)
 		dprintk(1, "qbuf: buffer preparation failed: %d\n", ret);
 	vb->state = ret ? VB2_BUF_STATE_DEQUEUED : VB2_BUF_STATE_PREPARED;
@@ -1393,6 +1521,8 @@ static int vb2_start_streaming(struct vb2_queue *q)
 
 	/* Tell the driver to start streaming */
 	ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
+	if (ret)
+		fail_qop(q, start_streaming);
 
 	/*
 	 * If there are not enough buffers queued to start streaming, then
@@ -1635,7 +1765,7 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
 		for (i = 0; i < vb->num_planes; ++i) {
 			if (!vb->planes[i].dbuf_mapped)
 				continue;
-			call_memop(q, unmap_dmabuf, vb->planes[i].mem_priv);
+			call_memop(vb, unmap_dmabuf, vb->planes[i].mem_priv);
 			vb->planes[i].dbuf_mapped = 0;
 		}
 }
@@ -1653,7 +1783,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
 	if (ret < 0)
 		return ret;
 
-	ret = call_qop(q, buf_finish, vb);
+	ret = call_vb_qop(vb, buf_finish, vb);
 	if (ret) {
 		dprintk(1, "dqbuf: buffer finish failed\n");
 		return ret;
@@ -1946,10 +2076,11 @@ int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb)
 
 	vb_plane = &vb->planes[eb->plane];
 
-	dbuf = call_memop(q, get_dmabuf, vb_plane->mem_priv, eb->flags & O_ACCMODE);
+	dbuf = call_memop(vb, get_dmabuf, vb_plane->mem_priv, eb->flags & O_ACCMODE);
 	if (IS_ERR_OR_NULL(dbuf)) {
 		dprintk(1, "Failed to export buffer %d, plane %d\n",
 			eb->index, eb->plane);
+		fail_memop(vb, get_dmabuf);
 		return -EINVAL;
 	}
 
@@ -2041,9 +2172,11 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
 		return -EINVAL;
 	}
 
-	ret = call_memop(q, mmap, vb->planes[plane].mem_priv, vma);
-	if (ret)
+	ret = call_memop(vb, mmap, vb->planes[plane].mem_priv, vma);
+	if (ret) {
+		fail_memop(vb, mmap);
 		return ret;
+	}
 
 	dprintk(3, "Buffer %d, plane %d successfully mapped\n", buffer, plane);
 	return 0;
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index bef53ce..f04eb28 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -203,6 +203,37 @@ struct vb2_buffer {
 	struct list_head	done_entry;
 
 	struct vb2_plane	planes[VIDEO_MAX_PLANES];
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	/*
+	 * Counters for how often these buffer-related ops are
+	 * called. Used to check for unbalanced ops.
+	 */
+	u32		cnt_mem_alloc;
+	u32		cnt_mem_put;
+	u32		cnt_mem_get_dmabuf;
+	u32		cnt_mem_get_userptr;
+	u32		cnt_mem_put_userptr;
+	u32		cnt_mem_prepare;
+	u32		cnt_mem_finish;
+	u32		cnt_mem_attach_dmabuf;
+	u32		cnt_mem_detach_dmabuf;
+	u32		cnt_mem_map_dmabuf;
+	u32		cnt_mem_unmap_dmabuf;
+	u32		cnt_mem_vaddr;
+	u32		cnt_mem_cookie;
+	u32		cnt_mem_num_users;
+	u32		cnt_mem_mmap;
+
+	u32		cnt_buf_init;
+	u32		cnt_buf_prepare;
+	u32		cnt_buf_finish;
+	u32		cnt_buf_cleanup;
+	u32		cnt_buf_queue;
+
+	/* This counts the number of calls to vb2_buffer_done() */
+	u32		cnt_buf_done;
+#endif
 };
 
 /**
@@ -364,6 +395,18 @@ struct vb2_queue {
 	unsigned int			retry_start_streaming:1;
 
 	struct vb2_fileio_data		*fileio;
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+	/*
+	 * Counters for how often these queue-related ops are
+	 * called. Used to check for unbalanced ops.
+	 */
+	u32				cnt_queue_setup;
+	u32				cnt_wait_prepare;
+	u32				cnt_wait_finish;
+	u32				cnt_start_streaming;
+	u32				cnt_stop_streaming;
+#endif
 };
 
 void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no);
-- 
1.9.0

[REVIEWv2 PATCH 05/15] vb2: change result code of buf_finish to void

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The buf_finish op should always work, so change the return type to void.
Update the few drivers that use it. Note that buf_finish can be called
both when the DMA is streaming and when it isn't (during queue_cancel).
Update drivers to check that where appropriate.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Pawel Osciak <pawel@osciak.com>
Reviewed-by: Pawel Osciak <pawel@osciak.com>
---
 drivers/media/parport/bw-qcam.c                 |  6 ++++--
 drivers/media/pci/sta2x11/sta2x11_vip.c         |  7 +++----
 drivers/media/platform/marvell-ccic/mcam-core.c |  3 +--
 drivers/media/usb/pwc/pwc-if.c                  | 16 +++++++++-------
 drivers/media/usb/uvc/uvc_queue.c               |  6 +++---
 drivers/media/v4l2-core/videobuf2-core.c        |  6 +-----
 drivers/staging/media/go7007/go7007-v4l2.c      |  3 +--
 include/media/videobuf2-core.h                  |  2 +-
 8 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/media/parport/bw-qcam.c b/drivers/media/parport/bw-qcam.c
index d12bd33..8d69bfb 100644
--- a/drivers/media/parport/bw-qcam.c
+++ b/drivers/media/parport/bw-qcam.c
@@ -667,13 +667,16 @@ static void buffer_queue(struct vb2_buffer *vb)
 	vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
 }
 
-static int buffer_finish(struct vb2_buffer *vb)
+static void buffer_finish(struct vb2_buffer *vb)
 {
 	struct qcam *qcam = vb2_get_drv_priv(vb->vb2_queue);
 	void *vbuf = vb2_plane_vaddr(vb, 0);
 	int size = vb->vb2_queue->plane_sizes[0];
 	int len;
 
+	if (!vb2_is_streaming(vb->vb2_queue))
+		return;
+
 	mutex_lock(&qcam->lock);
 	parport_claim_or_block(qcam->pdev);
 
@@ -691,7 +694,6 @@ static int buffer_finish(struct vb2_buffer *vb)
 	if (len != size)
 		vb->state = VB2_BUF_STATE_ERROR;
 	vb2_set_plane_payload(vb, 0, len);
-	return 0;
 }
 
 static struct vb2_ops qcam_video_qops = {
diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c b/drivers/media/pci/sta2x11/sta2x11_vip.c
index e5cfb6c..bb11443 100644
--- a/drivers/media/pci/sta2x11/sta2x11_vip.c
+++ b/drivers/media/pci/sta2x11/sta2x11_vip.c
@@ -327,7 +327,7 @@ static void buffer_queue(struct vb2_buffer *vb)
 	}
 	spin_unlock(&vip->lock);
 }
-static int buffer_finish(struct vb2_buffer *vb)
+static void buffer_finish(struct vb2_buffer *vb)
 {
 	struct sta2x11_vip *vip = vb2_get_drv_priv(vb->vb2_queue);
 	struct vip_buffer *vip_buf = to_vip_buffer(vb);
@@ -337,9 +337,8 @@ static int buffer_finish(struct vb2_buffer *vb)
 	list_del_init(&vip_buf->list);
 	spin_unlock(&vip->lock);
 
-	vip_active_buf_next(vip);
-
-	return 0;
+	if (vb2_is_streaming(vb->vb2_queue))
+		vip_active_buf_next(vip);
 }
 
 static int start_streaming(struct vb2_queue *vq, unsigned int count)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 32fab30..8b34c48 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -1238,7 +1238,7 @@ static int mcam_vb_sg_buf_prepare(struct vb2_buffer *vb)
 	return 0;
 }
 
-static int mcam_vb_sg_buf_finish(struct vb2_buffer *vb)
+static void mcam_vb_sg_buf_finish(struct vb2_buffer *vb)
 {
 	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
 	struct sg_table *sg_table = vb2_dma_sg_plane_desc(vb, 0);
@@ -1246,7 +1246,6 @@ static int mcam_vb_sg_buf_finish(struct vb2_buffer *vb)
 	if (sg_table)
 		dma_unmap_sg(cam->dev, sg_table->sgl,
 				sg_table->nents, DMA_FROM_DEVICE);
-	return 0;
 }
 
 static void mcam_vb_sg_buf_cleanup(struct vb2_buffer *vb)
diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
index abf365a..b9c9f10 100644
--- a/drivers/media/usb/pwc/pwc-if.c
+++ b/drivers/media/usb/pwc/pwc-if.c
@@ -614,17 +614,19 @@ static int buffer_prepare(struct vb2_buffer *vb)
 	return 0;
 }
 
-static int buffer_finish(struct vb2_buffer *vb)
+static void buffer_finish(struct vb2_buffer *vb)
 {
 	struct pwc_device *pdev = vb2_get_drv_priv(vb->vb2_queue);
 	struct pwc_frame_buf *buf = container_of(vb, struct pwc_frame_buf, vb);
 
-	/*
-	 * Application has called dqbuf and is getting back a buffer we've
-	 * filled, take the pwc data we've stored in buf->data and decompress
-	 * it into a usable format, storing the result in the vb2_buffer
-	 */
-	return pwc_decompress(pdev, buf);
+	if (vb->state == VB2_BUF_STATE_DONE) {
+		/*
+		 * Application has called dqbuf and is getting back a buffer we've
+		 * filled, take the pwc data we've stored in buf->data and decompress
+		 * it into a usable format, storing the result in the vb2_buffer
+		 */
+		pwc_decompress(pdev, buf);
+	}
 }
 
 static void buffer_cleanup(struct vb2_buffer *vb)
diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index cd962be..db5984e 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -104,15 +104,15 @@ static void uvc_buffer_queue(struct vb2_buffer *vb)
 	spin_unlock_irqrestore(&queue->irqlock, flags);
 }
 
-static int uvc_buffer_finish(struct vb2_buffer *vb)
+static void uvc_buffer_finish(struct vb2_buffer *vb)
 {
 	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
 	struct uvc_streaming *stream =
 			container_of(queue, struct uvc_streaming, queue);
 	struct uvc_buffer *buf = container_of(vb, struct uvc_buffer, buf);
 
-	uvc_video_clock_update(stream, &vb->v4l2_buf, buf);
-	return 0;
+	if (vb2_is_streaming(vb->vb2_queue))
+		uvc_video_clock_update(stream, &vb->v4l2_buf, buf);
 }
 
 static void uvc_wait_prepare(struct vb2_queue *vq)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 8f1578b..59bfd85 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1783,11 +1783,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
 	if (ret < 0)
 		return ret;
 
-	ret = call_vb_qop(vb, buf_finish, vb);
-	if (ret) {
-		dprintk(1, "dqbuf: buffer finish failed\n");
-		return ret;
-	}
+	call_vb_qop(vb, buf_finish, vb);
 
 	switch (vb->state) {
 	case VB2_BUF_STATE_DONE:
diff --git a/drivers/staging/media/go7007/go7007-v4l2.c b/drivers/staging/media/go7007/go7007-v4l2.c
index edc52e2..3a01576 100644
--- a/drivers/staging/media/go7007/go7007-v4l2.c
+++ b/drivers/staging/media/go7007/go7007-v4l2.c
@@ -470,7 +470,7 @@ static int go7007_buf_prepare(struct vb2_buffer *vb)
 	return 0;
 }
 
-static int go7007_buf_finish(struct vb2_buffer *vb)
+static void go7007_buf_finish(struct vb2_buffer *vb)
 {
 	struct vb2_queue *vq = vb->vb2_queue;
 	struct go7007 *go = vb2_get_drv_priv(vq);
@@ -483,7 +483,6 @@ static int go7007_buf_finish(struct vb2_buffer *vb)
 			V4L2_BUF_FLAG_PFRAME);
 	buf->flags |= frame_type_flag;
 	buf->field = V4L2_FIELD_NONE;
-	return 0;
 }
 
 static int go7007_start_streaming(struct vb2_queue *q, unsigned int count)
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index f04eb28..f443ce0 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -311,7 +311,7 @@ struct vb2_ops {
 
 	int (*buf_init)(struct vb2_buffer *vb);
 	int (*buf_prepare)(struct vb2_buffer *vb);
-	int (*buf_finish)(struct vb2_buffer *vb);
+	void (*buf_finish)(struct vb2_buffer *vb);
 	void (*buf_cleanup)(struct vb2_buffer *vb);
 
 	int (*start_streaming)(struct vb2_queue *q, unsigned int count);
-- 
1.9.0

[REVIEWv2 PATCH 06/15] vb2: add note that buf_finish can be called with !vb2_is_streaming()

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Drivers need to be aware that buf_finish can be called when there is no
streaming going on, so make a note of that.

Also add a bunch of missing periods at the end of sentences.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Pawel Osciak <pawel@osciak.com>
---
 include/media/videobuf2-core.h | 44 ++++++++++++++++++++++--------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index f443ce0..82b7f0f 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -34,49 +34,49 @@ struct vb2_fileio_data;
  *		usually will result in the allocator freeing the buffer (if
  *		no other users of this buffer are present); the buf_priv
  *		argument is the allocator private per-buffer structure
- *		previously returned from the alloc callback
+ *		previously returned from the alloc callback.
  * @get_userptr: acquire userspace memory for a hardware operation; used for
  *		 USERPTR memory types; vaddr is the address passed to the
  *		 videobuf layer when queuing a video buffer of USERPTR type;
  *		 should return an allocator private per-buffer structure
  *		 associated with the buffer on success, NULL on failure;
  *		 the returned private structure will then be passed as buf_priv
- *		 argument to other ops in this structure
+ *		 argument to other ops in this structure.
  * @put_userptr: inform the allocator that a USERPTR buffer will no longer
- *		 be used
+ *		 be used.
  * @attach_dmabuf: attach a shared struct dma_buf for a hardware operation;
  *		   used for DMABUF memory types; alloc_ctx is the alloc context
  *		   dbuf is the shared dma_buf; returns NULL on failure;
  *		   allocator private per-buffer structure on success;
- *		   this needs to be used for further accesses to the buffer
+ *		   this needs to be used for further accesses to the buffer.
  * @detach_dmabuf: inform the exporter of the buffer that the current DMABUF
  *		   buffer is no longer used; the buf_priv argument is the
  *		   allocator private per-buffer structure previously returned
- *		   from the attach_dmabuf callback
+ *		   from the attach_dmabuf callback.
  * @map_dmabuf: request for access to the dmabuf from allocator; the allocator
  *		of dmabuf is informed that this driver is going to use the
- *		dmabuf
+ *		dmabuf.
  * @unmap_dmabuf: releases access control to the dmabuf - allocator is notified
- *		  that this driver is done using the dmabuf for now
+ *		  that this driver is done using the dmabuf for now.
  * @prepare:	called every time the buffer is passed from userspace to the
- *		driver, useful for cache synchronisation, optional
+ *		driver, useful for cache synchronisation, optional.
  * @finish:	called every time the buffer is passed back from the driver
- *		to the userspace, also optional
+ *		to the userspace, also optional.
  * @vaddr:	return a kernel virtual address to a given memory buffer
  *		associated with the passed private structure or NULL if no
- *		such mapping exists
+ *		such mapping exists.
  * @cookie:	return allocator specific cookie for a given memory buffer
  *		associated with the passed private structure or NULL if not
- *		available
+ *		available.
  * @num_users:	return the current number of users of a memory buffer;
  *		return 1 if the videobuf layer (or actually the driver using
- *		it) is the only user
+ *		it) is the only user.
  * @mmap:	setup a userspace mapping for a given memory buffer under
- *		the provided virtual memory region
+ *		the provided virtual memory region.
  *
  * Required ops for USERPTR types: get_userptr, put_userptr.
  * Required ops for MMAP types: alloc, put, num_users, mmap.
- * Required ops for read/write access types: alloc, put, num_users, vaddr
+ * Required ops for read/write access types: alloc, put, num_users, vaddr.
  * Required ops for DMABUF types: attach_dmabuf, detach_dmabuf, map_dmabuf,
  *				  unmap_dmabuf.
  */
@@ -258,27 +258,29 @@ struct vb2_buffer {
  * @wait_prepare:	release any locks taken while calling vb2 functions;
  *			it is called before an ioctl needs to wait for a new
  *			buffer to arrive; required to avoid a deadlock in
- *			blocking access type
+ *			blocking access type.
  * @wait_finish:	reacquire all locks released in the previous callback;
  *			required to continue operation after sleeping while
- *			waiting for a new buffer to arrive
+ *			waiting for a new buffer to arrive.
  * @buf_init:		called once after allocating a buffer (in MMAP case)
  *			or after acquiring a new USERPTR buffer; drivers may
  *			perform additional buffer-related initialization;
  *			initialization failure (return != 0) will prevent
- *			queue setup from completing successfully; optional
+ *			queue setup from completing successfully; optional.
  * @buf_prepare:	called every time the buffer is queued from userspace
  *			and from the VIDIOC_PREPARE_BUF ioctl; drivers may
  *			perform any initialization required before each hardware
  *			operation in this callback; drivers that support
  *			VIDIOC_CREATE_BUFS must also validate the buffer size;
  *			if an error is returned, the buffer will not be queued
- *			in driver; optional
+ *			in driver; optional.
  * @buf_finish:		called before every dequeue of the buffer back to
  *			userspace; drivers may perform any operations required
- *			before userspace accesses the buffer; optional
+ *			before userspace accesses the buffer; optional. Note:
+ *			this op can be called as well when vb2_is_streaming()
+ *			returns false!
  * @buf_cleanup:	called once before the buffer is freed; drivers may
- *			perform any additional cleanup; optional
+ *			perform any additional cleanup; optional.
  * @start_streaming:	called once to enter 'streaming' state; the driver may
  *			receive buffers with @buf_queue callback before
  *			@start_streaming is called; the driver gets the number
@@ -299,7 +301,7 @@ struct vb2_buffer {
  *			the buffer back by calling vb2_buffer_done() function;
  *			it is allways called after calling STREAMON ioctl;
  *			might be called before start_streaming callback if user
- *			pre-queued buffers before calling STREAMON
+ *			pre-queued buffers before calling STREAMON.
  */
 struct vb2_ops {
 	int (*queue_setup)(struct vb2_queue *q, const struct v4l2_format *fmt,
-- 
1.9.0

[REVIEWv2 PATCH 07/15] vb2: call buf_finish from __dqbuf

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This ensures that it is also called from queue_cancel, which also calls
__dqbuf(). Without this change any time queue_cancel is called while
streaming the buf_finish op will not be called and any driver cleanup
will not happen.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Pawel Osciak <pawel@osciak.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 59bfd85..b5142e5 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1758,6 +1758,8 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
 	if (vb->state == VB2_BUF_STATE_DEQUEUED)
 		return;
 
+	call_vb_qop(vb, buf_finish, vb);
+
 	vb->state = VB2_BUF_STATE_DEQUEUED;
 
 	/* unmap DMABUF buffer */
@@ -1783,8 +1785,6 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
 	if (ret < 0)
 		return ret;
 
-	call_vb_qop(vb, buf_finish, vb);
-
 	switch (vb->state) {
 	case VB2_BUF_STATE_DONE:
 		dprintk(3, "dqbuf: Returning done buffer\n");
-- 
1.9.0

[REVIEWv2 PATCH 08/15] vb2: fix buf_init/buf_cleanup call sequences

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Ensure that these ops are properly balanced.

There are two scenarios:

1) for MMAP buf_init is called when the buffers are created and buf_cleanup
   must be called when the queue is finally freed. This scenario was always
   working.

2) for USERPTR and DMABUF it is more complicated. When a buffer is queued
   the code checks if all planes of this buffer have been acquired before.
   If that's the case, then only buf_prepare has to be called. Otherwise
   buf_cleanup needs to be called if the buffer was acquired before, then,
   once all changed planes have been (re)acquired, buf_init has to be
   called followed by buf_prepare. Should buf_prepare fail, then buf_cleanup
   must be called on the newly acquired planes to release them in.

Finally, in __vb2_queue_free we have to check if the buffer was actually
acquired before calling buf_cleanup. While that it always true for MMAP
mode, it is not necessarily true for the other modes. E.g. if you just
call REQBUFS and close the file handle, then buffers were never queued and
so no buf_init was ever called.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 100 +++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 33 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index b5142e5..eefcff7 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -373,8 +373,10 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 	/* Call driver-provided cleanup function for each buffer, if provided */
 	for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
 	     ++buffer) {
-		if (q->bufs[buffer])
-			call_vb_qop(q->bufs[buffer], buf_cleanup, q->bufs[buffer]);
+		struct vb2_buffer *vb = q->bufs[buffer];
+
+		if (vb && vb->planes[0].mem_priv)
+			call_vb_qop(vb, buf_cleanup, vb);
 	}
 
 	/* Release video buffer memory */
@@ -1161,6 +1163,7 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 	unsigned int plane;
 	int ret;
 	int write = !V4L2_TYPE_IS_OUTPUT(q->type);
+	bool reacquired = vb->planes[0].mem_priv == NULL;
 
 	/* Copy relevant information provided by the userspace */
 	__fill_vb2_buffer(vb, b, planes);
@@ -1186,12 +1189,16 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		}
 
 		/* Release previously acquired memory if present */
-		if (vb->planes[plane].mem_priv)
+		if (vb->planes[plane].mem_priv) {
+			if (!reacquired) {
+				reacquired = true;
+				call_vb_qop(vb, buf_cleanup, vb);
+			}
 			call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
+		}
 
 		vb->planes[plane].mem_priv = NULL;
-		vb->v4l2_planes[plane].m.userptr = 0;
-		vb->v4l2_planes[plane].length = 0;
+		memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane));
 
 		/* Acquire each plane's memory */
 		mem_priv = call_memop(vb, get_userptr, q->alloc_ctx[plane],
@@ -1208,23 +1215,34 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 	}
 
 	/*
-	 * Call driver-specific initialization on the newly acquired buffer,
-	 * if provided.
-	 */
-	ret = call_vb_qop(vb, buf_init, vb);
-	if (ret) {
-		dprintk(1, "qbuf: buffer initialization failed\n");
-		fail_vb_qop(vb, buf_init);
-		goto err;
-	}
-
-	/*
 	 * Now that everything is in order, copy relevant information
 	 * provided by userspace.
 	 */
 	for (plane = 0; plane < vb->num_planes; ++plane)
 		vb->v4l2_planes[plane] = planes[plane];
 
+	if (reacquired) {
+		/*
+		 * One or more planes changed, so we must call buf_init to do
+		 * the driver-specific initialization on the newly acquired
+		 * buffer, if provided.
+		 */
+		ret = call_vb_qop(vb, buf_init, vb);
+		if (ret) {
+			dprintk(1, "qbuf: buffer initialization failed\n");
+			fail_vb_qop(vb, buf_init);
+			goto err;
+		}
+	}
+
+	ret = call_vb_qop(vb, buf_prepare, vb);
+	if (ret) {
+		dprintk(1, "qbuf: buffer preparation failed\n");
+		fail_vb_qop(vb, buf_prepare);
+		call_vb_qop(vb, buf_cleanup, vb);
+		goto err;
+	}
+
 	return 0;
 err:
 	/* In case of errors, release planes that were already acquired */
@@ -1244,8 +1262,13 @@ err:
  */
 static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 {
+	int ret;
+
 	__fill_vb2_buffer(vb, b, vb->v4l2_planes);
-	return 0;
+	ret = call_vb_qop(vb, buf_prepare, vb);
+	if (ret)
+		fail_vb_qop(vb, buf_prepare);
+	return ret;
 }
 
 /**
@@ -1259,6 +1282,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 	unsigned int plane;
 	int ret;
 	int write = !V4L2_TYPE_IS_OUTPUT(q->type);
+	bool reacquired = vb->planes[0].mem_priv == NULL;
 
 	/* Copy relevant information provided by the userspace */
 	__fill_vb2_buffer(vb, b, planes);
@@ -1294,6 +1318,11 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 
 		dprintk(1, "qbuf: buffer for plane %d changed\n", plane);
 
+		if (!reacquired) {
+			reacquired = true;
+			call_vb_qop(vb, buf_cleanup, vb);
+		}
+
 		/* Release previously acquired memory if present */
 		__vb2_plane_dmabuf_put(vb, &vb->planes[plane]);
 		memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane));
@@ -1329,23 +1358,33 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 	}
 
 	/*
-	 * Call driver-specific initialization on the newly acquired buffer,
-	 * if provided.
-	 */
-	ret = call_vb_qop(vb, buf_init, vb);
-	if (ret) {
-		dprintk(1, "qbuf: buffer initialization failed\n");
-		fail_vb_qop(vb, buf_init);
-		goto err;
-	}
-
-	/*
 	 * Now that everything is in order, copy relevant information
 	 * provided by userspace.
 	 */
 	for (plane = 0; plane < vb->num_planes; ++plane)
 		vb->v4l2_planes[plane] = planes[plane];
 
+	if (reacquired) {
+		/*
+		 * Call driver-specific initialization on the newly acquired buffer,
+		 * if provided.
+		 */
+		ret = call_vb_qop(vb, buf_init, vb);
+		if (ret) {
+			dprintk(1, "qbuf: buffer initialization failed\n");
+			fail_vb_qop(vb, buf_init);
+			goto err;
+		}
+	}
+
+	ret = call_vb_qop(vb, buf_prepare, vb);
+	if (ret) {
+		dprintk(1, "qbuf: buffer preparation failed\n");
+		fail_vb_qop(vb, buf_prepare);
+		call_vb_qop(vb, buf_cleanup, vb);
+		goto err;
+	}
+
 	return 0;
 err:
 	/* In case of errors, release planes that were already acquired */
@@ -1420,11 +1459,6 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		ret = -EINVAL;
 	}
 
-	if (!ret) {
-		ret = call_vb_qop(vb, buf_prepare, vb);
-		if (ret)
-			fail_vb_qop(vb, buf_prepare);
-	}
 	if (ret)
 		dprintk(1, "qbuf: buffer preparation failed: %d\n", ret);
 	vb->state = ret ? VB2_BUF_STATE_DEQUEUED : VB2_BUF_STATE_PREPARED;
-- 
1.9.0

[REVIEWv2 PATCH 09/15] vb2: rename queued_count to owned_by_drv_count

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

'queued_count' is a bit vague since it is not clear to which queue it
refers to: the vb2 internal list of buffers or the driver-owned list
of buffers.

Rename to make it explicit.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Pawel Osciak <pawel@osciak.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 10 +++++-----
 include/media/videobuf2-core.h           |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index eefcff7..2a7815c 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1071,7 +1071,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 	spin_lock_irqsave(&q->done_lock, flags);
 	vb->state = state;
 	list_add_tail(&vb->done_entry, &q->done_list);
-	atomic_dec(&q->queued_count);
+	atomic_dec(&q->owned_by_drv_count);
 	spin_unlock_irqrestore(&q->done_lock, flags);
 
 	/* Inform any processes that may be waiting for buffers */
@@ -1402,7 +1402,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
 	unsigned int plane;
 
 	vb->state = VB2_BUF_STATE_ACTIVE;
-	atomic_inc(&q->queued_count);
+	atomic_inc(&q->owned_by_drv_count);
 
 	/* sync buffers */
 	for (plane = 0; plane < vb->num_planes; ++plane)
@@ -1554,7 +1554,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
 	int ret;
 
 	/* Tell the driver to start streaming */
-	ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
+	ret = call_qop(q, start_streaming, q, atomic_read(&q->owned_by_drv_count));
 	if (ret)
 		fail_qop(q, start_streaming);
 
@@ -1775,7 +1775,7 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
 	}
 
 	if (!q->retry_start_streaming)
-		wait_event(q->done_wq, !atomic_read(&q->queued_count));
+		wait_event(q->done_wq, !atomic_read(&q->owned_by_drv_count));
 	return 0;
 }
 EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
@@ -1907,7 +1907,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 	 * has not already dequeued before initiating cancel.
 	 */
 	INIT_LIST_HEAD(&q->done_list);
-	atomic_set(&q->queued_count, 0);
+	atomic_set(&q->owned_by_drv_count, 0);
 	wake_up_all(&q->done_wq);
 
 	/*
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 82b7f0f..adaffed 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -353,7 +353,7 @@ struct v4l2_fh;
  * @bufs:	videobuf buffer structures
  * @num_buffers: number of allocated/used buffers
  * @queued_list: list of buffers currently queued from userspace
- * @queued_count: number of buffers owned by the driver
+ * @owned_by_drv_count: number of buffers owned by the driver
  * @done_list:	list of buffers ready to be dequeued to userspace
  * @done_lock:	lock to protect done_list list
  * @done_wq:	waitqueue for processes waiting for buffers ready to be dequeued
@@ -385,7 +385,7 @@ struct vb2_queue {
 
 	struct list_head		queued_list;
 
-	atomic_t			queued_count;
+	atomic_t			owned_by_drv_count;
 	struct list_head		done_list;
 	spinlock_t			done_lock;
 	wait_queue_head_t		done_wq;
-- 
1.9.0

[REVIEWv2 PATCH 10/15] vb2: don't init the list if there are still buffers

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

__vb2_queue_free() would init the queued_list at all times, even if
q->num_buffers > 0. This should only happen if num_buffers == 0.

This situation can happen if a CREATE_BUFFERS call couldn't allocate
enough buffers and had to free those it did manage to allocate before
returning an error.

While we're at it: __vb2_queue_alloc() returns the number of buffers
allocated, not an error code. So stick the result in allocated_buffers
instead of ret as that's very confusing.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 2a7815c..90374c0 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -452,9 +452,10 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 	}
 
 	q->num_buffers -= buffers;
-	if (!q->num_buffers)
+	if (!q->num_buffers) {
 		q->memory = 0;
-	INIT_LIST_HEAD(&q->queued_list);
+		INIT_LIST_HEAD(&q->queued_list);
+	}
 	return 0;
 }
 
@@ -820,14 +821,12 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 	}
 
 	/* Finally, allocate buffers and video memory */
-	ret = __vb2_queue_alloc(q, req->memory, num_buffers, num_planes);
-	if (ret == 0) {
+	allocated_buffers = __vb2_queue_alloc(q, req->memory, num_buffers, num_planes);
+	if (allocated_buffers == 0) {
 		dprintk(1, "Memory allocation failed\n");
 		return -ENOMEM;
 	}
 
-	allocated_buffers = ret;
-
 	/*
 	 * Check if driver can handle the allocated number of buffers.
 	 */
@@ -851,6 +850,10 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 	q->num_buffers = allocated_buffers;
 
 	if (ret < 0) {
+		/*
+		 * Note: __vb2_queue_free() will subtract 'allocated_buffers'
+		 * from q->num_buffers.
+		 */
 		__vb2_queue_free(q, allocated_buffers);
 		return ret;
 	}
@@ -924,20 +927,18 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create
 	}
 
 	/* Finally, allocate buffers and video memory */
-	ret = __vb2_queue_alloc(q, create->memory, num_buffers,
+	allocated_buffers = __vb2_queue_alloc(q, create->memory, num_buffers,
 				num_planes);
-	if (ret == 0) {
+	if (allocated_buffers == 0) {
 		dprintk(1, "Memory allocation failed\n");
 		return -ENOMEM;
 	}
 
-	allocated_buffers = ret;
-
 	/*
 	 * Check if driver can handle the so far allocated number of buffers.
 	 */
-	if (ret < num_buffers) {
-		num_buffers = ret;
+	if (allocated_buffers < num_buffers) {
+		num_buffers = allocated_buffers;
 
 		/*
 		 * q->num_buffers contains the total number of buffers, that the
@@ -960,6 +961,10 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create
 	q->num_buffers += allocated_buffers;
 
 	if (ret < 0) {
+		/*
+		 * Note: __vb2_queue_free() will subtract 'allocated_buffers'
+		 * from q->num_buffers.
+		 */
 		__vb2_queue_free(q, allocated_buffers);
 		return -ENOMEM;
 	}
-- 
1.9.0

[REVIEWv2 PATCH 11/15] vb2: only call start_streaming if sufficient buffers are queued

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

In commit 02f142ecd24aaf891324ffba8527284c1731b561 support was added to
start_streaming to return -ENOBUFS if insufficient buffers were queued
for the DMA engine to start. The vb2 core would attempt calling
start_streaming again if another buffer would be queued up.

Later analysis uncovered problems with the queue management if start_streaming
would return an error: the buffers are enqueued to the driver before the
start_streaming op is called, so after an error they are never returned to
the vb2 core. The solution for this is to let the driver return them to
the vb2 core in case of an error while starting the DMA engine. However,
in the case of -ENOBUFS that would be weird: it is not a real error, it
just says that more buffers are needed. Requiring start_streaming to give
them back only to have them requeued again the next time the application
calls QBUF is inefficient.

This patch changes this mechanism: it adds a 'min_buffers_needed' field
to vb2_queue that drivers can set with the minimum number of buffers
required to start the DMA engine. The start_streaming op is only called
if enough buffers are queued. The -ENOBUFS handling has been dropped in
favor of this new method.

Drivers are expected to return buffers back to vb2 core with state QUEUED
if start_streaming would return an error. The vb2 core checks for this
and produces a warning if that didn't happen and it will forcefully
reclaim such buffers to ensure that the internal vb2 core state remains
consistent and all buffer-related resources have been correctly freed
and all op calls have been balanced.

__reqbufs() has been updated to check that at least min_buffers_needed
buffers could be allocated. If fewer buffers were allocated then __reqbufs
will free what was allocated and return -ENOMEM. Based on a suggestion from
Pawel Osciak.

__create_bufs() doesn't do that check, since the use of __create_bufs
assumes some advance scenario where the user might want more control.
Instead streamon will check if enough buffers were allocated to prevent
streaming with fewer than the minimum required number of buffers.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/davinci/vpbe_display.c   |   6 +-
 drivers/media/platform/davinci/vpif_capture.c   |   7 +-
 drivers/media/platform/davinci/vpif_display.c   |   7 +-
 drivers/media/platform/s5p-tv/mixer_video.c     |   6 +-
 drivers/media/v4l2-core/videobuf2-core.c        | 146 ++++++++++++++++--------
 drivers/staging/media/davinci_vpfe/vpfe_video.c |   3 +-
 include/media/videobuf2-core.h                  |  14 ++-
 7 files changed, 116 insertions(+), 73 deletions(-)

diff --git a/drivers/media/platform/davinci/vpbe_display.c b/drivers/media/platform/davinci/vpbe_display.c
index b02aba4..9231e48 100644
--- a/drivers/media/platform/davinci/vpbe_display.c
+++ b/drivers/media/platform/davinci/vpbe_display.c
@@ -344,11 +344,6 @@ static int vpbe_start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct vpbe_device *vpbe_dev = fh->disp_dev->vpbe_dev;
 	int ret;
 
-	/* If buffer queue is empty, return error */
-	if (list_empty(&layer->dma_queue)) {
-		v4l2_err(&vpbe_dev->v4l2_dev, "buffer queue is empty\n");
-		return -ENOBUFS;
-	}
 	/* Get the next frame from the buffer queue */
 	layer->next_frm = layer->cur_frm = list_entry(layer->dma_queue.next,
 				struct vpbe_disp_buffer, list);
@@ -1416,6 +1411,7 @@ static int vpbe_display_reqbufs(struct file *file, void *priv,
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct vpbe_disp_buffer);
 	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+	q->min_buffers_needed = 1;
 
 	ret = vb2_queue_init(q);
 	if (ret) {
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 735ec47..9731ad4 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -272,13 +272,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
 	unsigned long flags;
 	int ret;
 
-	/* If buffer queue is empty, return error */
 	spin_lock_irqsave(&common->irqlock, flags);
-	if (list_empty(&common->dma_queue)) {
-		spin_unlock_irqrestore(&common->irqlock, flags);
-		vpif_dbg(1, debug, "buffer queue is empty\n");
-		return -ENOBUFS;
-	}
 
 	/* Get the next frame from the buffer queue */
 	common->cur_frm = common->next_frm = list_entry(common->dma_queue.next,
@@ -1024,6 +1018,7 @@ static int vpif_reqbufs(struct file *file, void *priv,
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct vpif_cap_buffer);
 	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+	q->min_buffers_needed = 1;
 
 	ret = vb2_queue_init(q);
 	if (ret) {
diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
index 9d115cd..af660f5 100644
--- a/drivers/media/platform/davinci/vpif_display.c
+++ b/drivers/media/platform/davinci/vpif_display.c
@@ -234,13 +234,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
 	unsigned long flags;
 	int ret;
 
-	/* If buffer queue is empty, return error */
 	spin_lock_irqsave(&common->irqlock, flags);
-	if (list_empty(&common->dma_queue)) {
-		spin_unlock_irqrestore(&common->irqlock, flags);
-		vpif_err("buffer queue is empty\n");
-		return -ENOBUFS;
-	}
 
 	/* Get the next frame from the buffer queue */
 	common->next_frm = common->cur_frm =
@@ -984,6 +978,7 @@ static int vpif_reqbufs(struct file *file, void *priv,
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct vpif_disp_buffer);
 	q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+	q->min_buffers_needed = 1;
 
 	ret = vb2_queue_init(q);
 	if (ret) {
diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
index c5059ba..a1ce55f 100644
--- a/drivers/media/platform/s5p-tv/mixer_video.c
+++ b/drivers/media/platform/s5p-tv/mixer_video.c
@@ -946,11 +946,6 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	mxr_dbg(mdev, "%s\n", __func__);
 
-	if (count == 0) {
-		mxr_dbg(mdev, "no output buffers queued\n");
-		return -ENOBUFS;
-	}
-
 	/* block any changes in output configuration */
 	mxr_output_get(mdev);
 
@@ -1124,6 +1119,7 @@ struct mxr_layer *mxr_base_layer_create(struct mxr_device *mdev,
 		.drv_priv = layer,
 		.buf_struct_size = sizeof(struct mxr_buffer),
 		.ops = &mxr_video_qops,
+		.min_buffers_needed = 1,
 		.mem_ops = &vb2_dma_contig_memops,
 	};
 
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 90374c0..f156475 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -805,6 +805,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 	 * Make sure the requested values and current defaults are sane.
 	 */
 	num_buffers = min_t(unsigned int, req->count, VIDEO_MAX_FRAME);
+	num_buffers = max_t(unsigned int, req->count, q->min_buffers_needed);
 	memset(q->plane_sizes, 0, sizeof(q->plane_sizes));
 	memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
 	q->memory = req->memory;
@@ -828,9 +829,16 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 	}
 
 	/*
+	 * There is no point in continuing if we can't allocate the minimum
+	 * number of buffers needed by this vb2_queue.
+	 */
+	if (allocated_buffers < q->min_buffers_needed)
+		ret = -ENOMEM;
+
+	/*
 	 * Check if driver can handle the allocated number of buffers.
 	 */
-	if (allocated_buffers < num_buffers) {
+	if (!ret && allocated_buffers < num_buffers) {
 		num_buffers = allocated_buffers;
 
 		ret = call_qop(q, queue_setup, q, NULL, &num_buffers,
@@ -1038,13 +1046,20 @@ EXPORT_SYMBOL_GPL(vb2_plane_cookie);
  * vb2_buffer_done() - inform videobuf that an operation on a buffer is finished
  * @vb:		vb2_buffer returned from the driver
  * @state:	either VB2_BUF_STATE_DONE if the operation finished successfully
- *		or VB2_BUF_STATE_ERROR if the operation finished with an error
+ *		or VB2_BUF_STATE_ERROR if the operation finished with an error.
+ *		If start_streaming fails then it should return buffers with state
+ *		VB2_BUF_STATE_QUEUED to put them back into the queue.
  *
  * This function should be called by the driver after a hardware operation on
  * a buffer is finished and the buffer may be returned to userspace. The driver
  * cannot use this buffer anymore until it is queued back to it by videobuf
  * by the means of buf_queue callback. Only buffers previously queued to the
  * driver by buf_queue can be passed to this function.
+ *
+ * While streaming a buffer can only be returned in state DONE or ERROR.
+ * The start_streaming op can also return them in case the DMA engine cannot
+ * be started for some reason. In that case the buffers should be returned with
+ * state QUEUED.
  */
 void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 {
@@ -1052,11 +1067,17 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 	unsigned long flags;
 	unsigned int plane;
 
-	if (vb->state != VB2_BUF_STATE_ACTIVE)
+	if (WARN_ON(vb->state != VB2_BUF_STATE_ACTIVE))
 		return;
 
-	if (state != VB2_BUF_STATE_DONE && state != VB2_BUF_STATE_ERROR)
-		return;
+	if (!q->start_streaming_called) {
+		if (WARN_ON(state != VB2_BUF_STATE_QUEUED))
+			state = VB2_BUF_STATE_QUEUED;
+	} else if (!WARN_ON(!q->start_streaming_called)) {
+		if (WARN_ON(state != VB2_BUF_STATE_DONE &&
+			    state != VB2_BUF_STATE_ERROR))
+			state = VB2_BUF_STATE_ERROR;
+	}
 
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	/*
@@ -1075,10 +1096,14 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 	/* Add the buffer to the done buffers list */
 	spin_lock_irqsave(&q->done_lock, flags);
 	vb->state = state;
-	list_add_tail(&vb->done_entry, &q->done_list);
+	if (state != VB2_BUF_STATE_QUEUED)
+		list_add_tail(&vb->done_entry, &q->done_list);
 	atomic_dec(&q->owned_by_drv_count);
 	spin_unlock_irqrestore(&q->done_lock, flags);
 
+	if (state == VB2_BUF_STATE_QUEUED)
+		return;
+
 	/* Inform any processes that may be waiting for buffers */
 	wake_up(&q->done_wq);
 }
@@ -1549,34 +1574,49 @@ EXPORT_SYMBOL_GPL(vb2_prepare_buf);
  * vb2_start_streaming() - Attempt to start streaming.
  * @q:		videobuf2 queue
  *
- * If there are not enough buffers, then retry_start_streaming is set to
- * 1 and 0 is returned. The next time a buffer is queued and
- * retry_start_streaming is 1, this function will be called again to
- * retry starting the DMA engine.
+ * Attempt to start streaming. When this function is called there must be
+ * at least q->min_buffers_needed buffers queued up (i.e. the minimum
+ * number of buffers required for the DMA engine to function). If the
+ * @start_streaming op fails it is supposed to return all the driver-owned
+ * buffers back to vb2 in state QUEUED. Check if that happened and if
+ * not warn and reclaim them forcefully.
  */
 static int vb2_start_streaming(struct vb2_queue *q)
 {
+	struct vb2_buffer *vb;
 	int ret;
 
-	/* Tell the driver to start streaming */
-	ret = call_qop(q, start_streaming, q, atomic_read(&q->owned_by_drv_count));
-	if (ret)
-		fail_qop(q, start_streaming);
-
 	/*
-	 * If there are not enough buffers queued to start streaming, then
-	 * the start_streaming operation will return -ENOBUFS and you have to
-	 * retry when the next buffer is queued.
+	 * If any buffers were queued before streamon,
+	 * we can now pass them to driver for processing.
 	 */
-	if (ret == -ENOBUFS) {
-		dprintk(1, "qbuf: not enough buffers, retry when more buffers are queued.\n");
-		q->retry_start_streaming = 1;
+	list_for_each_entry(vb, &q->queued_list, queued_entry)
+		__enqueue_in_driver(vb);
+
+	/* Tell the driver to start streaming */
+	ret = call_qop(q, start_streaming, q,
+		       atomic_read(&q->owned_by_drv_count));
+	q->start_streaming_called = ret == 0;
+	if (!ret)
 		return 0;
+
+	fail_qop(q, start_streaming);
+	dprintk(1, "qbuf: driver refused to start streaming\n");
+	if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
+		unsigned i;
+
+		/*
+		 * Forcefully reclaim buffers if the driver did not
+		 * correctly return them to vb2.
+		 */
+		for (i = 0; i < q->num_buffers; ++i) {
+			vb = q->bufs[i];
+			if (vb->state == VB2_BUF_STATE_ACTIVE)
+				vb2_buffer_done(vb, VB2_BUF_STATE_QUEUED);
+		}
+		/* Must be zero now */
+		WARN_ON(atomic_read(&q->owned_by_drv_count));
 	}
-	if (ret)
-		dprintk(1, "qbuf: driver refused to start streaming\n");
-	else
-		q->retry_start_streaming = 0;
 	return ret;
 }
 
@@ -1612,19 +1652,27 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 	 * dequeued in dqbuf.
 	 */
 	list_add_tail(&vb->queued_entry, &q->queued_list);
+	q->queued_count++;
 	vb->state = VB2_BUF_STATE_QUEUED;
 
 	/*
 	 * If already streaming, give the buffer to driver for processing.
 	 * If not, the buffer will be given to driver on next streamon.
 	 */
-	if (q->streaming)
+	if (q->start_streaming_called)
 		__enqueue_in_driver(vb);
 
 	/* Fill buffer information for the userspace */
 	__fill_v4l2_buffer(vb, b);
 
-	if (q->retry_start_streaming) {
+	/*
+	 * If streamon has been called, and we haven't yet called
+	 * start_streaming() since not enough buffers were queued, and
+	 * we now have reached the minimum number of queued buffers,
+	 * then we can finally call start_streaming().
+	 */
+	if (q->streaming && !q->start_streaming_called &&
+	    q->queued_count >= q->min_buffers_needed) {
 		ret = vb2_start_streaming(q);
 		if (ret)
 			return ret;
@@ -1779,7 +1827,7 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
 		return -EINVAL;
 	}
 
-	if (!q->retry_start_streaming)
+	if (q->start_streaming_called)
 		wait_event(q->done_wq, !atomic_read(&q->owned_by_drv_count));
 	return 0;
 }
@@ -1840,6 +1888,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
 	__fill_v4l2_buffer(vb, b);
 	/* Remove from videobuf queue */
 	list_del(&vb->queued_entry);
+	q->queued_count--;
 	/* go back to dequeued state */
 	__vb2_dqbuf(vb);
 
@@ -1890,18 +1939,23 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 {
 	unsigned int i;
 
-	if (q->retry_start_streaming) {
-		q->retry_start_streaming = 0;
-		q->streaming = 0;
-	}
-
 	/*
 	 * Tell driver to stop all transactions and release all queued
 	 * buffers.
 	 */
-	if (q->streaming)
+	if (q->start_streaming_called)
 		call_qop(q, stop_streaming, q);
 	q->streaming = 0;
+	q->start_streaming_called = 0;
+	q->queued_count = 0;
+
+	if (WARN_ON(atomic_read(&q->owned_by_drv_count))) {
+		for (i = 0; i < q->num_buffers; ++i)
+			if (q->bufs[i]->state == VB2_BUF_STATE_ACTIVE)
+				vb2_buffer_done(q->bufs[i], VB2_BUF_STATE_ERROR);
+		/* Must be zero now */
+		WARN_ON(atomic_read(&q->owned_by_drv_count));
+	}
 
 	/*
 	 * Remove all buffers from videobuf's list...
@@ -1924,7 +1978,6 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 
 static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 {
-	struct vb2_buffer *vb;
 	int ret;
 
 	if (type != q->type) {
@@ -1941,19 +1994,22 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 		dprintk(1, "streamon: no buffers have been allocated\n");
 		return -EINVAL;
 	}
+	if (q->num_buffers < q->min_buffers_needed) {
+		dprintk(1, "streamon: need at least %u allocated buffers\n",
+				q->min_buffers_needed);
+		return -EINVAL;
+	}
 
 	/*
-	 * If any buffers were queued before streamon,
-	 * we can now pass them to driver for processing.
+	 * Tell driver to start streaming provided sufficient buffers
+	 * are available.
 	 */
-	list_for_each_entry(vb, &q->queued_list, queued_entry)
-		__enqueue_in_driver(vb);
-
-	/* Tell driver to start streaming. */
-	ret = vb2_start_streaming(q);
-	if (ret) {
-		__vb2_queue_cancel(q);
-		return ret;
+	if (q->queued_count >= q->min_buffers_needed) {
+		ret = vb2_start_streaming(q);
+		if (ret) {
+			__vb2_queue_cancel(q);
+			return ret;
+		}
 	}
 
 	q->streaming = 1;
diff --git a/drivers/staging/media/davinci_vpfe/vpfe_video.c b/drivers/staging/media/davinci_vpfe/vpfe_video.c
index 1f3b0f9..8c101cb 100644
--- a/drivers/staging/media/davinci_vpfe/vpfe_video.c
+++ b/drivers/staging/media/davinci_vpfe/vpfe_video.c
@@ -1201,8 +1201,6 @@ static int vpfe_start_streaming(struct vb2_queue *vq, unsigned int count)
 	unsigned long addr;
 	int ret;
 
-	if (count == 0)
-		return -ENOBUFS;
 	ret = mutex_lock_interruptible(&video->lock);
 	if (ret)
 		goto streamoff;
@@ -1327,6 +1325,7 @@ static int vpfe_reqbufs(struct file *file, void *priv,
 	q->type = req_buf->type;
 	q->io_modes = VB2_MMAP | VB2_USERPTR;
 	q->drv_priv = fh;
+	q->min_buffers_needed = 1;
 	q->ops = &video_qops;
 	q->mem_ops = &vb2_dma_contig_memops;
 	q->buf_struct_size = sizeof(struct vpfe_cap_buffer);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index adaffed..2ada71b 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -348,20 +348,24 @@ struct v4l2_fh;
  * @gfp_flags:	additional gfp flags used when allocating the buffers.
  *		Typically this is 0, but it may be e.g. GFP_DMA or __GFP_DMA32
  *		to force the buffer allocation to a specific memory zone.
+ * @min_buffers_needed: the minimum number of buffers needed before
+ *		start_streaming() can be called. Used when a DMA engine
+ *		cannot be started unless at least this number of buffers
+ *		have been queued into the driver.
  *
  * @memory:	current memory type used
  * @bufs:	videobuf buffer structures
  * @num_buffers: number of allocated/used buffers
  * @queued_list: list of buffers currently queued from userspace
+ * @queued_count: number of buffers queued and ready for streaming.
  * @owned_by_drv_count: number of buffers owned by the driver
  * @done_list:	list of buffers ready to be dequeued to userspace
  * @done_lock:	lock to protect done_list list
  * @done_wq:	waitqueue for processes waiting for buffers ready to be dequeued
  * @alloc_ctx:	memory type/allocator-specific contexts for each plane
  * @streaming:	current streaming state
- * @retry_start_streaming: start_streaming() was called, but there were not enough
- *		buffers queued. If set, then retry calling start_streaming when
- *		queuing a new buffer.
+ * @start_streaming_called: start_streaming() was called successfully and we
+ *		started streaming.
  * @fileio:	file io emulator internal data, used only if emulator is active
  */
 struct vb2_queue {
@@ -377,6 +381,7 @@ struct vb2_queue {
 	unsigned int			buf_struct_size;
 	u32				timestamp_type;
 	gfp_t				gfp_flags;
+	u32				min_buffers_needed;
 
 /* private: internal use only */
 	enum v4l2_memory		memory;
@@ -384,6 +389,7 @@ struct vb2_queue {
 	unsigned int			num_buffers;
 
 	struct list_head		queued_list;
+	unsigned int			queued_count;
 
 	atomic_t			owned_by_drv_count;
 	struct list_head		done_list;
@@ -394,7 +400,7 @@ struct vb2_queue {
 	unsigned int			plane_sizes[VIDEO_MAX_PLANES];
 
 	unsigned int			streaming:1;
-	unsigned int			retry_start_streaming:1;
+	unsigned int			start_streaming_called:1;
 
 	struct vb2_fileio_data		*fileio;
 
-- 
1.9.0

[REVIEWv2 PATCH 12/15] vb2: properly clean up PREPARED and QUEUED buffers

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

In __vb2_dqbuf we need to call the finish memop for any pending
buffers in the PREPARED or QUEUED state. This can happen if PREPARE_BUF
or QBUF was called without ever calling STREAMON.

In that case, when either REQBUFS(0) is called or the filehandle is
closed __vb2_queue_cancel needs to handle such buffers correctly.

For the same reason a call to __vb2_queue_cancel was added in __reqbufs
so that these buffers are cleaned up there as well.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index f156475..3815f9c 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -96,6 +96,8 @@ module_param(debug, int, 0644);
 				 V4L2_BUF_FLAG_PREPARED | \
 				 V4L2_BUF_FLAG_TIMESTAMP_MASK)
 
+static void __vb2_queue_cancel(struct vb2_queue *q);
+
 /**
  * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
  */
@@ -789,6 +791,12 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 			return -EBUSY;
 		}
 
+		/*
+		 * Call queue_cancel to clean up any buffers in the PREPARED or
+		 * QUEUED state which is possible if buffers were prepared or
+		 * queued without ever calling STREAMON.
+		 */
+		__vb2_queue_cancel(q);
 		ret = __vb2_queue_free(q, q->num_buffers);
 		if (ret)
 			return ret;
@@ -1839,12 +1847,23 @@ EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
 static void __vb2_dqbuf(struct vb2_buffer *vb)
 {
 	struct vb2_queue *q = vb->vb2_queue;
+	unsigned int plane;
 	unsigned int i;
 
 	/* nothing to do if the buffer is already dequeued */
 	if (vb->state == VB2_BUF_STATE_DEQUEUED)
 		return;
 
+	/*
+	 * If it is prepared or queued the we 'finish' the buffer.
+	 * This can happen if buffers were prepared or queued without STREAMON
+	 * being called, and we are called by __vb2_queue_cancel.
+	 */
+	if (vb->state == VB2_BUF_STATE_PREPARED ||
+	    vb->state == VB2_BUF_STATE_QUEUED)
+		for (plane = 0; plane < vb->num_planes; ++plane)
+			call_memop(vb, finish, vb->planes[plane].mem_priv);
+
 	call_vb_qop(vb, buf_finish, vb);
 
 	vb->state = VB2_BUF_STATE_DEQUEUED;
-- 
1.9.0

[REVIEWv2 PATCH 13/15] vb2: replace BUG by WARN_ON

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

No need to oops for this, WARN_ON is good enough.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 3815f9c..68f3def 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2536,9 +2536,9 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 	/*
 	 * Sanity check
 	 */
-	if ((read && !(q->io_modes & VB2_READ)) ||
-	   (!read && !(q->io_modes & VB2_WRITE)))
-		BUG();
+	if (WARN_ON((read && !(q->io_modes & VB2_READ)) ||
+		    (!read && !(q->io_modes & VB2_WRITE))))
+		return -EINVAL;
 
 	/*
 	 * Check if device supports mapping buffers to kernel virtual space.
-- 
1.9.0

[REVIEWv2 PATCH 14/15] vb2: fix streamoff handling if streamon wasn't called.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

If you request buffers, then queue buffers and then call STREAMOFF
those buffers are not returned to their dequeued state because streamoff
will just return if q->streaming was 0.

This means that afterwards you can never QBUF that same buffer again unless
you do STREAMON, REQBUFS or close the filehandle first.

It is clear that if you do STREAMOFF even if no STREAMON was called before,
you still want to have all buffers returned to their proper dequeued state.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 68f3def..4af6457 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2067,14 +2067,14 @@ static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
 		return -EINVAL;
 	}
 
-	if (!q->streaming) {
-		dprintk(3, "streamoff successful: not streaming\n");
-		return 0;
-	}
-
 	/*
 	 * Cancel will pause streaming and remove all buffers from the driver
 	 * and videobuf, effectively returning control over them to userspace.
+	 *
+	 * Note that we do this even if q->streaming == 0: if you prepare or
+	 * queue buffers, and then call streamoff without ever having called
+	 * streamon, you would still expect those buffers to be returned to
+	 * their normal dequeued state.
 	 */
 	__vb2_queue_cancel(q);
 
-- 
1.9.0

[REVIEWv2 PATCH 15/15] vivi: correctly cleanup after a start_streaming failure

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

If start_streaming fails then any queued buffers must be given back
to the vb2 core.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/vivi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index e9cd96e..643937b 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -889,10 +889,20 @@ static void buffer_queue(struct vb2_buffer *vb)
 static int start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct vivi_dev *dev = vb2_get_drv_priv(vq);
+	int err;
 
 	dprintk(dev, 1, "%s\n", __func__);
 	dev->seq_count = 0;
-	return vivi_start_generating(dev);
+	err = vivi_start_generating(dev);
+	if (err) {
+		struct vivi_buffer *buf, *tmp;
+
+		list_for_each_entry_safe(buf, tmp, &dev->vidq.active, list) {
+			list_del(&buf->list);
+			vb2_buffer_done(&buf->vb, VB2_BUF_STATE_QUEUED);
+		}
+	}
+	return err;
 }
 
 /* abort streaming and wait for last buffer */
-- 
1.9.0

Re: [REVIEWv2 PATCH 03/15] vb2: fix PREPARE_BUF regression

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tuesday 25 February 2014 13:52:43 Hans Verkuil wrote:
> Fix an incorrect test in vb2_internal_qbuf() where only DEQUEUED buffers
> are allowed. But PREPARED buffers are also OK.
> 
> Introduced by commit 4138111a27859dcc56a5592c804dd16bb12a23d1
> ("vb2: simplify qbuf/prepare_buf by removing callback").
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

I wonder how I've managed to ack the commit that broke this without noticing 
the problem :-/ Sorry about that.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index f1a2857c..909f367 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1420,11 +1420,6 @@ static int vb2_internal_qbuf(struct vb2_queue *q,
> struct v4l2_buffer *b) return ret;
> 
>  	vb = q->bufs[b->index];
> -	if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> -		dprintk(1, "%s(): invalid buffer state %d\n", __func__,
> -			vb->state);
> -		return -EINVAL;
> -	}
> 
>  	switch (vb->state) {
>  	case VB2_BUF_STATE_DEQUEUED:
> @@ -1438,7 +1433,8 @@ static int vb2_internal_qbuf(struct vb2_queue *q,
> struct v4l2_buffer *b) dprintk(1, "qbuf: buffer still being prepared\n");
>  		return -EINVAL;
>  	default:
> -		dprintk(1, "qbuf: buffer already in use\n");
> +		dprintk(1, "%s(): invalid buffer state %d\n", __func__,
> +			vb->state);
>  		return -EINVAL;
>  	}

-- 
Regards,

Laurent Pinchart

Re: [REVIEWv2 PATCH 04/15] vb2: add debugging code to check for unbalanced ops

[Laurent Pinchart]

Hi Hans,

Thank you for the patch. This is a pretty useful debugging tool. Please see 
below for two small comments.

On Tuesday 25 February 2014 13:52:44 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> When a vb2_queue is freed check if all the mem_ops and queue ops were
> balanced. So the number of calls to e.g. buf_finish has to match the number
> of calls to buf_prepare, etc.
> 
> This code is only enabled if CONFIG_VIDEO_ADV_DEBUG is set.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Pawel Osciak <pawel@osciak.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 233 +++++++++++++++++++++-------
>  include/media/videobuf2-core.h           |  43 ++++++
>  2 files changed, 226 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 909f367..8f1578b 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -33,12 +33,63 @@ module_param(debug, int, 0644);
>  			printk(KERN_DEBUG "vb2: " fmt, ## arg);		\
>  	} while (0)
> 
> -#define call_memop(q, op, args...)					\
> -	(((q)->mem_ops->op) ?						\
> -		((q)->mem_ops->op(args)) : 0)
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +
> +/*
> + * If advanced debugging is on, then count how often each op is called,
> + * which can either be per-buffer or per-queue.
> + *
> + * If the op failed then the 'fail_' variant is called to decrease the
> + * counter. That makes it easy to check that the 'init' and 'cleanup'
> + * (and variations thereof) stay balanced.

It would have been nice to avoid the fail_ variants by not increasing the 
counter when the operation fails, but I suppose that's not possible given that 
what return value indicates an error is operation-specific.

> + */
> +
> +#define call_memop(vb, op, args...)					\
> +({									\
> +	struct vb2_queue *_q = (vb)->vb2_queue;				\
> +	dprintk(2, "call_memop(%p, %d, %s)%s\n",			\
> +		_q, (vb)->v4l2_buf.index, #op,				\
> +		_q->mem_ops->op ? "" : " (nop)");			\
> +	(vb)->cnt_mem_ ## op++;						\
> +	_q->mem_ops->op ? _q->mem_ops->op(args) : 0;			\
> +})
> +#define fail_memop(vb, op) ((vb)->cnt_mem_ ## op--)
> +
> +#define call_qop(q, op, args...)					\
> +({									\
> +	dprintk(2, "call_qop(%p, %s)%s\n", q, #op,			\
> +		(q)->ops->op ? "" : " (nop)");				\
> +	(q)->cnt_ ## op++;						\
> +	(q)->ops->op ? (q)->ops->op(args) : 0;				\
> +})
> +#define fail_qop(q, op) ((q)->cnt_ ## op--)
> +
> +#define call_vb_qop(vb, op, args...)					\
> +({									\
> +	struct vb2_queue *_q = (vb)->vb2_queue;				\
> +	dprintk(2, "call_vb_qop(%p, %d, %s)%s\n",			\
> +		_q, (vb)->v4l2_buf.index, #op,				\
> +		_q->ops->op ? "" : " (nop)");				\
> +	(vb)->cnt_ ## op++;						\
> +	_q->ops->op ? _q->ops->op(args) : 0;				\
> +})
> +#define fail_vb_qop(vb, op) ((vb)->cnt_ ## op--)
> +
> +#else
> +
> +#define call_memop(vb, op, args...)					\
> +	((vb)->vb2_queue->mem_ops->op ? (vb)->vb2_queue->mem_ops->op(args) : 0)
> +#define fail_memop(vb, op)
> 
>  #define call_qop(q, op, args...)					\
> -	(((q)->ops->op) ? ((q)->ops->op(args)) : 0)
> +	((q)->ops->op ? (q)->ops->op(args) : 0)
> +#define fail_qop(q, op)
> +
> +#define call_vb_qop(vb, op, args...)					\
> +	((vb)->vb2_queue->ops->op ? (vb)->vb2_queue->ops->op(args) : 0)
> +#define fail_vb_qop(vb, op)
> +
> +#endif
> 
>  #define V4L2_BUFFER_MASK_FLAGS	(V4L2_BUF_FLAG_MAPPED | 
V4L2_BUF_FLAG_QUEUED
> | \ V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR | \
> @@ -61,7 +112,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>  		unsigned long size = PAGE_ALIGN(q->plane_sizes[plane]);
> 
> -		mem_priv = call_memop(q, alloc, q->alloc_ctx[plane],
> +		mem_priv = call_memop(vb, alloc, q->alloc_ctx[plane],
>  				      size, q->gfp_flags);
>  		if (IS_ERR_OR_NULL(mem_priv))
>  			goto free;
> @@ -73,9 +124,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> 
>  	return 0;
>  free:
> +	fail_memop(vb, alloc);
>  	/* Free already allocated memory if one of the allocations failed */
>  	for (; plane > 0; --plane) {
> -		call_memop(q, put, vb->planes[plane - 1].mem_priv);
> +		call_memop(vb, put, vb->planes[plane - 1].mem_priv);
>  		vb->planes[plane - 1].mem_priv = NULL;
>  	}
> 
> @@ -87,11 +139,10 @@ free:
>   */
>  static void __vb2_buf_mem_free(struct vb2_buffer *vb)
>  {
> -	struct vb2_queue *q = vb->vb2_queue;
>  	unsigned int plane;
> 
>  	for (plane = 0; plane < vb->num_planes; ++plane) {
> -		call_memop(q, put, vb->planes[plane].mem_priv);
> +		call_memop(vb, put, vb->planes[plane].mem_priv);
>  		vb->planes[plane].mem_priv = NULL;
>  		dprintk(3, "Freed plane %d of buffer %d\n", plane,
>  			vb->v4l2_buf.index);
> @@ -104,12 +155,11 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb)
>   */
>  static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
>  {
> -	struct vb2_queue *q = vb->vb2_queue;
>  	unsigned int plane;
> 
>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>  		if (vb->planes[plane].mem_priv)
> -			call_memop(q, put_userptr, vb->planes[plane].mem_priv);
> +			call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
>  		vb->planes[plane].mem_priv = NULL;
>  	}
>  }
> @@ -118,15 +168,15 @@ static void __vb2_buf_userptr_put(struct vb2_buffer
> *vb) * __vb2_plane_dmabuf_put() - release memory associated with
>   * a DMABUF shared plane
>   */
> -static void __vb2_plane_dmabuf_put(struct vb2_queue *q, struct vb2_plane
> *p) +static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct
> vb2_plane *p) {
>  	if (!p->mem_priv)
>  		return;
> 
>  	if (p->dbuf_mapped)
> -		call_memop(q, unmap_dmabuf, p->mem_priv);
> +		call_memop(vb, unmap_dmabuf, p->mem_priv);
> 
> -	call_memop(q, detach_dmabuf, p->mem_priv);
> +	call_memop(vb, detach_dmabuf, p->mem_priv);
>  	dma_buf_put(p->dbuf);
>  	memset(p, 0, sizeof(*p));
>  }
> @@ -137,11 +187,10 @@ static void __vb2_plane_dmabuf_put(struct vb2_queue
> *q, struct vb2_plane *p) */
>  static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb)
>  {
> -	struct vb2_queue *q = vb->vb2_queue;
>  	unsigned int plane;
> 
>  	for (plane = 0; plane < vb->num_planes; ++plane)
> -		__vb2_plane_dmabuf_put(q, &vb->planes[plane]);
> +		__vb2_plane_dmabuf_put(vb, &vb->planes[plane]);
>  }
> 
>  /**
> @@ -246,10 +295,11 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum
> v4l2_memory memory, * callback, if given. An error in initialization
>  			 * results in queue setup failure.
>  			 */
> -			ret = call_qop(q, buf_init, vb);
> +			ret = call_vb_qop(vb, buf_init, vb);
>  			if (ret) {
>  				dprintk(1, "Buffer %d %p initialization"
>  					" failed\n", buffer, vb);
> +				fail_vb_qop(vb, buf_init);
>  				__vb2_buf_mem_free(vb);
>  				kfree(vb);
>  				break;
> @@ -321,18 +371,77 @@ static int __vb2_queue_free(struct vb2_queue *q,
> unsigned int buffers) }
> 
>  	/* Call driver-provided cleanup function for each buffer, if provided */
> -	if (q->ops->buf_cleanup) {
> -		for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> -		     ++buffer) {
> -			if (NULL == q->bufs[buffer])
> -				continue;
> -			q->ops->buf_cleanup(q->bufs[buffer]);
> -		}
> +	for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> +	     ++buffer) {
> +		if (q->bufs[buffer])
> +			call_vb_qop(q->bufs[buffer], buf_cleanup, q->bufs[buffer]);
>  	}
> 
>  	/* Release video buffer memory */
>  	__vb2_free_mem(q, buffers);
> 

I would have put the code below in a separate function, but that's up to you.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +	/*
> +	 * Check that all the calls were balances during the life-time of this
> +	 * queue. If not (or if the debug level is 1 or up), then dump the
> +	 * counters to the kernel log.
> +	 */
> +	if (q->num_buffers) {
> +		bool unbalanced = q->cnt_start_streaming != q->cnt_stop_streaming ||
> +				  q->cnt_wait_prepare != q->cnt_wait_finish;
> +
> +		if (unbalanced || debug) {
> +			pr_info("vb2: counters for queue %p:%s\n", q,
> +				unbalanced ? " UNBALANCED!" : "");
> +			pr_info("vb2:     setup: %u start_streaming: %u stop_streaming: 
%u\n",
> +				q->cnt_queue_setup, q->cnt_start_streaming,
> +				q->cnt_stop_streaming);
> +			pr_info("vb2:     wait_prepare: %u wait_finish: %u\n",
> +				q->cnt_wait_prepare, q->cnt_wait_finish);
> +		}
> +		q->cnt_queue_setup = 0;
> +		q->cnt_wait_prepare = 0;
> +		q->cnt_wait_finish = 0;
> +		q->cnt_start_streaming = 0;
> +		q->cnt_stop_streaming = 0;
> +	}
> +	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> +		struct vb2_buffer *vb = q->bufs[buffer];
> +		bool unbalanced = vb->cnt_mem_alloc != vb->cnt_mem_put ||
> +				  vb->cnt_mem_prepare != vb->cnt_mem_finish ||
> +				  vb->cnt_mem_get_userptr != vb->cnt_mem_put_userptr ||
> +				  vb->cnt_mem_attach_dmabuf != vb->cnt_mem_detach_dmabuf ||
> +				  vb->cnt_mem_map_dmabuf != vb->cnt_mem_unmap_dmabuf ||
> +				  vb->cnt_buf_queue != vb->cnt_buf_done ||
> +				  vb->cnt_buf_prepare != vb->cnt_buf_finish ||
> +				  vb->cnt_buf_init != vb->cnt_buf_cleanup;
> +
> +		if (unbalanced || debug) {
> +			pr_info("vb2:   counters for queue %p, buffer %d:%s\n",
> +				q, buffer, unbalanced ? " UNBALANCED!" : "");
> +			pr_info("vb2:     buf_init: %u buf_cleanup: %u buf_prepare: %u
> buf_finish: %u\n", +				vb->cnt_buf_init, vb->cnt_buf_cleanup,
> +				vb->cnt_buf_prepare, vb->cnt_buf_finish);
> +			pr_info("vb2:     buf_queue: %u buf_done: %u\n",
> +				vb->cnt_buf_queue, vb->cnt_buf_done);
> +			pr_info("vb2:     alloc: %u put: %u prepare: %u finish: %u mmap: 
%u\n",
> +				vb->cnt_mem_alloc, vb->cnt_mem_put,
> +				vb->cnt_mem_prepare, vb->cnt_mem_finish,
> +				vb->cnt_mem_mmap);
> +			pr_info("vb2:     get_userptr: %u put_userptr: %u\n",
> +				vb->cnt_mem_get_userptr, vb->cnt_mem_put_userptr);
> +			pr_info("vb2:     attach_dmabuf: %u detach_dmabuf: %u map_dmabuf: 
%u
> unmap_dmabuf: %u\n", +				vb->cnt_mem_attach_dmabuf,
> vb->cnt_mem_detach_dmabuf,
> +				vb->cnt_mem_map_dmabuf, vb->cnt_mem_unmap_dmabuf);
> +			pr_info("vb2:     get_dmabuf: %u num_users: %u vaddr: %u cookie: 
%u\n",
> +				vb->cnt_mem_get_dmabuf,
> +				vb->cnt_mem_num_users,
> +				vb->cnt_mem_vaddr,
> +				vb->cnt_mem_cookie);
> +		}
> +	}
> +#endif
> +
>  	/* Free videobuf buffers */
>  	for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
>  	     ++buffer) {
> @@ -424,7 +533,7 @@ static bool __buffer_in_use(struct vb2_queue *q, struct
> vb2_buffer *vb) * case anyway. If num_users() returns more than 1,
>  		 * we are not the only user of the plane's memory.
>  		 */
> -		if (mem_priv && call_memop(q, num_users, mem_priv) > 1)
> +		if (mem_priv && call_memop(vb, num_users, mem_priv) > 1)
>  			return true;
>  	}
>  	return false;
> @@ -703,8 +812,10 @@ static int __reqbufs(struct vb2_queue *q, struct
> v4l2_requestbuffers *req) */
>  	ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes,
>  		       q->plane_sizes, q->alloc_ctx);
> -	if (ret)
> +	if (ret) {
> +		fail_qop(q, queue_setup);
>  		return ret;
> +	}
> 
>  	/* Finally, allocate buffers and video memory */
>  	ret = __vb2_queue_alloc(q, req->memory, num_buffers, num_planes);
> @@ -723,6 +834,8 @@ static int __reqbufs(struct vb2_queue *q, struct
> v4l2_requestbuffers *req)
> 
>  		ret = call_qop(q, queue_setup, q, NULL, &num_buffers,
>  			       &num_planes, q->plane_sizes, q->alloc_ctx);
> +		if (ret)
> +			fail_qop(q, queue_setup);
> 
>  		if (!ret && allocated_buffers < num_buffers)
>  			ret = -ENOMEM;
> @@ -803,8 +916,10 @@ static int __create_bufs(struct vb2_queue *q, struct
> v4l2_create_buffers *create */
>  	ret = call_qop(q, queue_setup, q, &create->format, &num_buffers,
>  		       &num_planes, q->plane_sizes, q->alloc_ctx);
> -	if (ret)
> +	if (ret) {
> +		fail_qop(q, queue_setup);
>  		return ret;
> +	}
> 
>  	/* Finally, allocate buffers and video memory */
>  	ret = __vb2_queue_alloc(q, create->memory, num_buffers,
> @@ -828,6 +943,8 @@ static int __create_bufs(struct vb2_queue *q, struct
> v4l2_create_buffers *create */
>  		ret = call_qop(q, queue_setup, q, &create->format, &num_buffers,
>  			       &num_planes, q->plane_sizes, q->alloc_ctx);
> +		if (ret)
> +			fail_qop(q, queue_setup);
> 
>  		if (!ret && allocated_buffers < num_buffers)
>  			ret = -ENOMEM;
> @@ -882,12 +999,10 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
>   */
>  void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no)
>  {
> -	struct vb2_queue *q = vb->vb2_queue;
> -
>  	if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
>  		return NULL;
> 
> -	return call_memop(q, vaddr, vb->planes[plane_no].mem_priv);
> +	return call_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
> 
>  }
>  EXPORT_SYMBOL_GPL(vb2_plane_vaddr);
> @@ -905,12 +1020,10 @@ EXPORT_SYMBOL_GPL(vb2_plane_vaddr);
>   */
>  void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int plane_no)
>  {
> -	struct vb2_queue *q = vb->vb2_queue;
> -
>  	if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
>  		return NULL;
> 
> -	return call_memop(q, cookie, vb->planes[plane_no].mem_priv);
> +	return call_memop(vb, cookie, vb->planes[plane_no].mem_priv);
>  }
>  EXPORT_SYMBOL_GPL(vb2_plane_cookie);
> 
> @@ -938,12 +1051,19 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum
> vb2_buffer_state state) if (state != VB2_BUF_STATE_DONE && state !=
> VB2_BUF_STATE_ERROR) return;
> 
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +	/*
> +	 * Although this is not a callback, it still does have to balance
> +	 * with the buf_queue op. So update this counter manually.
> +	 */
> +	vb->cnt_buf_done++;
> +#endif
>  	dprintk(4, "Done processing on buffer %d, state: %d\n",
>  			vb->v4l2_buf.index, state);
> 
>  	/* sync buffers */
>  	for (plane = 0; plane < vb->num_planes; ++plane)
> -		call_memop(q, finish, vb->planes[plane].mem_priv);
> +		call_memop(vb, finish, vb->planes[plane].mem_priv);
> 
>  	/* Add the buffer to the done buffers list */
>  	spin_lock_irqsave(&q->done_lock, flags);
> @@ -1067,19 +1187,20 @@ static int __qbuf_userptr(struct vb2_buffer *vb,
> const struct v4l2_buffer *b)
> 
>  		/* Release previously acquired memory if present */
>  		if (vb->planes[plane].mem_priv)
> -			call_memop(q, put_userptr, vb->planes[plane].mem_priv);
> +			call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
> 
>  		vb->planes[plane].mem_priv = NULL;
>  		vb->v4l2_planes[plane].m.userptr = 0;
>  		vb->v4l2_planes[plane].length = 0;
> 
>  		/* Acquire each plane's memory */
> -		mem_priv = call_memop(q, get_userptr, q->alloc_ctx[plane],
> +		mem_priv = call_memop(vb, get_userptr, q->alloc_ctx[plane],
>  				      planes[plane].m.userptr,
>  				      planes[plane].length, write);
>  		if (IS_ERR_OR_NULL(mem_priv)) {
>  			dprintk(1, "qbuf: failed acquiring userspace "
>  						"memory for plane %d\n", plane);
> +			fail_memop(vb, get_userptr);
>  			ret = mem_priv ? PTR_ERR(mem_priv) : -EINVAL;
>  			goto err;
>  		}
> @@ -1090,9 +1211,10 @@ static int __qbuf_userptr(struct vb2_buffer *vb,
> const struct v4l2_buffer *b) * Call driver-specific initialization on the
> newly acquired buffer, * if provided.
>  	 */
> -	ret = call_qop(q, buf_init, vb);
> +	ret = call_vb_qop(vb, buf_init, vb);
>  	if (ret) {
>  		dprintk(1, "qbuf: buffer initialization failed\n");
> +		fail_vb_qop(vb, buf_init);
>  		goto err;
>  	}
> 
> @@ -1108,7 +1230,7 @@ err:
>  	/* In case of errors, release planes that were already acquired */
>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>  		if (vb->planes[plane].mem_priv)
> -			call_memop(q, put_userptr, vb->planes[plane].mem_priv);
> +			call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
>  		vb->planes[plane].mem_priv = NULL;
>  		vb->v4l2_planes[plane].m.userptr = 0;
>  		vb->v4l2_planes[plane].length = 0;
> @@ -1173,14 +1295,15 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb,
> const struct v4l2_buffer *b) dprintk(1, "qbuf: buffer for plane %d
> changed\n", plane);
> 
>  		/* Release previously acquired memory if present */
> -		__vb2_plane_dmabuf_put(q, &vb->planes[plane]);
> +		__vb2_plane_dmabuf_put(vb, &vb->planes[plane]);
>  		memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane));
> 
>  		/* Acquire each plane's memory */
> -		mem_priv = call_memop(q, attach_dmabuf, q->alloc_ctx[plane],
> +		mem_priv = call_memop(vb, attach_dmabuf, q->alloc_ctx[plane],
>  			dbuf, planes[plane].length, write);
>  		if (IS_ERR(mem_priv)) {
>  			dprintk(1, "qbuf: failed to attach dmabuf\n");
> +			fail_memop(vb, attach_dmabuf);
>  			ret = PTR_ERR(mem_priv);
>  			dma_buf_put(dbuf);
>  			goto err;
> @@ -1195,10 +1318,11 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb,
> const struct v4l2_buffer *b) * the buffer(s)..
>  	 */
>  	for (plane = 0; plane < vb->num_planes; ++plane) {
> -		ret = call_memop(q, map_dmabuf, vb->planes[plane].mem_priv);
> +		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
>  		if (ret) {
>  			dprintk(1, "qbuf: failed to map dmabuf for plane %d\n",
>  				plane);
> +			fail_memop(vb, map_dmabuf);
>  			goto err;
>  		}
>  		vb->planes[plane].dbuf_mapped = 1;
> @@ -1208,9 +1332,10 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const
> struct v4l2_buffer *b) * Call driver-specific initialization on the newly
> acquired buffer, * if provided.
>  	 */
> -	ret = call_qop(q, buf_init, vb);
> +	ret = call_vb_qop(vb, buf_init, vb);
>  	if (ret) {
>  		dprintk(1, "qbuf: buffer initialization failed\n");
> +		fail_vb_qop(vb, buf_init);
>  		goto err;
>  	}
> 
> @@ -1242,9 +1367,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> 
>  	/* sync buffers */
>  	for (plane = 0; plane < vb->num_planes; ++plane)
> -		call_memop(q, prepare, vb->planes[plane].mem_priv);
> +		call_memop(vb, prepare, vb->planes[plane].mem_priv);
> 
> -	q->ops->buf_queue(vb);
> +	call_vb_qop(vb, buf_queue, vb);
>  }
> 
>  static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer
> *b) @@ -1295,8 +1420,11 @@ static int __buf_prepare(struct vb2_buffer *vb,
> const struct v4l2_buffer *b) ret = -EINVAL;
>  	}
> 
> -	if (!ret)
> -		ret = call_qop(q, buf_prepare, vb);
> +	if (!ret) {
> +		ret = call_vb_qop(vb, buf_prepare, vb);
> +		if (ret)
> +			fail_vb_qop(vb, buf_prepare);
> +	}
>  	if (ret)
>  		dprintk(1, "qbuf: buffer preparation failed: %d\n", ret);
>  	vb->state = ret ? VB2_BUF_STATE_DEQUEUED : VB2_BUF_STATE_PREPARED;
> @@ -1393,6 +1521,8 @@ static int vb2_start_streaming(struct vb2_queue *q)
> 
>  	/* Tell the driver to start streaming */
>  	ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
> +	if (ret)
> +		fail_qop(q, start_streaming);
> 
>  	/*
>  	 * If there are not enough buffers queued to start streaming, then
> @@ -1635,7 +1765,7 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
>  		for (i = 0; i < vb->num_planes; ++i) {
>  			if (!vb->planes[i].dbuf_mapped)
>  				continue;
> -			call_memop(q, unmap_dmabuf, vb->planes[i].mem_priv);
> +			call_memop(vb, unmap_dmabuf, vb->planes[i].mem_priv);
>  			vb->planes[i].dbuf_mapped = 0;
>  		}
>  }
> @@ -1653,7 +1783,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q,
> struct v4l2_buffer *b, bool n if (ret < 0)
>  		return ret;
> 
> -	ret = call_qop(q, buf_finish, vb);
> +	ret = call_vb_qop(vb, buf_finish, vb);
>  	if (ret) {
>  		dprintk(1, "dqbuf: buffer finish failed\n");
>  		return ret;
> @@ -1946,10 +2076,11 @@ int vb2_expbuf(struct vb2_queue *q, struct
> v4l2_exportbuffer *eb)
> 
>  	vb_plane = &vb->planes[eb->plane];
> 
> -	dbuf = call_memop(q, get_dmabuf, vb_plane->mem_priv, eb->flags &
> O_ACCMODE); +	dbuf = call_memop(vb, get_dmabuf, vb_plane->mem_priv,
> eb->flags & O_ACCMODE); if (IS_ERR_OR_NULL(dbuf)) {
>  		dprintk(1, "Failed to export buffer %d, plane %d\n",
>  			eb->index, eb->plane);
> +		fail_memop(vb, get_dmabuf);
>  		return -EINVAL;
>  	}
> 
> @@ -2041,9 +2172,11 @@ int vb2_mmap(struct vb2_queue *q, struct
> vm_area_struct *vma) return -EINVAL;
>  	}
> 
> -	ret = call_memop(q, mmap, vb->planes[plane].mem_priv, vma);
> -	if (ret)
> +	ret = call_memop(vb, mmap, vb->planes[plane].mem_priv, vma);
> +	if (ret) {
> +		fail_memop(vb, mmap);
>  		return ret;
> +	}
> 
>  	dprintk(3, "Buffer %d, plane %d successfully mapped\n", buffer, plane);
>  	return 0;
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index bef53ce..f04eb28 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -203,6 +203,37 @@ struct vb2_buffer {
>  	struct list_head	done_entry;
> 
>  	struct vb2_plane	planes[VIDEO_MAX_PLANES];
> +
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +	/*
> +	 * Counters for how often these buffer-related ops are
> +	 * called. Used to check for unbalanced ops.
> +	 */
> +	u32		cnt_mem_alloc;
> +	u32		cnt_mem_put;
> +	u32		cnt_mem_get_dmabuf;
> +	u32		cnt_mem_get_userptr;
> +	u32		cnt_mem_put_userptr;
> +	u32		cnt_mem_prepare;
> +	u32		cnt_mem_finish;
> +	u32		cnt_mem_attach_dmabuf;
> +	u32		cnt_mem_detach_dmabuf;
> +	u32		cnt_mem_map_dmabuf;
> +	u32		cnt_mem_unmap_dmabuf;
> +	u32		cnt_mem_vaddr;
> +	u32		cnt_mem_cookie;
> +	u32		cnt_mem_num_users;
> +	u32		cnt_mem_mmap;
> +
> +	u32		cnt_buf_init;
> +	u32		cnt_buf_prepare;
> +	u32		cnt_buf_finish;
> +	u32		cnt_buf_cleanup;
> +	u32		cnt_buf_queue;
> +
> +	/* This counts the number of calls to vb2_buffer_done() */
> +	u32		cnt_buf_done;
> +#endif
>  };
> 
>  /**
> @@ -364,6 +395,18 @@ struct vb2_queue {
>  	unsigned int			retry_start_streaming:1;
> 
>  	struct vb2_fileio_data		*fileio;
> +
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +	/*
> +	 * Counters for how often these queue-related ops are
> +	 * called. Used to check for unbalanced ops.
> +	 */
> +	u32				cnt_queue_setup;
> +	u32				cnt_wait_prepare;
> +	u32				cnt_wait_finish;
> +	u32				cnt_start_streaming;
> +	u32				cnt_stop_streaming;
> +#endif
>  };
> 
>  void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no);

-- 
Regards,

Laurent Pinchart

Re: [REVIEWv2 PATCH 04/15] vb2: add debugging code to check for unbalanced ops

[Hans Verkuil]

On 02/27/14 12:15, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch. This is a pretty useful debugging tool. Please see 
> below for two small comments.
> 
> On Tuesday 25 February 2014 13:52:44 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> When a vb2_queue is freed check if all the mem_ops and queue ops were
>> balanced. So the number of calls to e.g. buf_finish has to match the number
>> of calls to buf_prepare, etc.
>>
>> This code is only enabled if CONFIG_VIDEO_ADV_DEBUG is set.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Acked-by: Pawel Osciak <pawel@osciak.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 233 +++++++++++++++++++++-------
>>  include/media/videobuf2-core.h           |  43 ++++++
>>  2 files changed, 226 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> b/drivers/media/v4l2-core/videobuf2-core.c index 909f367..8f1578b 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -33,12 +33,63 @@ module_param(debug, int, 0644);
>>  			printk(KERN_DEBUG "vb2: " fmt, ## arg);		\
>>  	} while (0)
>>
>> -#define call_memop(q, op, args...)					\
>> -	(((q)->mem_ops->op) ?						\
>> -		((q)->mem_ops->op(args)) : 0)
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> +
>> +/*
>> + * If advanced debugging is on, then count how often each op is called,
>> + * which can either be per-buffer or per-queue.
>> + *
>> + * If the op failed then the 'fail_' variant is called to decrease the
>> + * counter. That makes it easy to check that the 'init' and 'cleanup'
>> + * (and variations thereof) stay balanced.
> 
> It would have been nice to avoid the fail_ variants by not increasing the 
> counter when the operation fails, but I suppose that's not possible given that 
> what return value indicates an error is operation-specific.

Yeah, I tried to come up with the least messy solution and this was it.

> 
>> + */
>> +
>> +#define call_memop(vb, op, args...)					\
>> +({									\
>> +	struct vb2_queue *_q = (vb)->vb2_queue;				\
>> +	dprintk(2, "call_memop(%p, %d, %s)%s\n",			\
>> +		_q, (vb)->v4l2_buf.index, #op,				\
>> +		_q->mem_ops->op ? "" : " (nop)");			\
>> +	(vb)->cnt_mem_ ## op++;						\
>> +	_q->mem_ops->op ? _q->mem_ops->op(args) : 0;			\
>> +})
>> +#define fail_memop(vb, op) ((vb)->cnt_mem_ ## op--)
>> +
>> +#define call_qop(q, op, args...)					\
>> +({									\
>> +	dprintk(2, "call_qop(%p, %s)%s\n", q, #op,			\
>> +		(q)->ops->op ? "" : " (nop)");				\
>> +	(q)->cnt_ ## op++;						\
>> +	(q)->ops->op ? (q)->ops->op(args) : 0;				\
>> +})
>> +#define fail_qop(q, op) ((q)->cnt_ ## op--)
>> +
>> +#define call_vb_qop(vb, op, args...)					\
>> +({									\
>> +	struct vb2_queue *_q = (vb)->vb2_queue;				\
>> +	dprintk(2, "call_vb_qop(%p, %d, %s)%s\n",			\
>> +		_q, (vb)->v4l2_buf.index, #op,				\
>> +		_q->ops->op ? "" : " (nop)");				\
>> +	(vb)->cnt_ ## op++;						\
>> +	_q->ops->op ? _q->ops->op(args) : 0;				\
>> +})
>> +#define fail_vb_qop(vb, op) ((vb)->cnt_ ## op--)
>> +
>> +#else
>> +
>> +#define call_memop(vb, op, args...)					\
>> +	((vb)->vb2_queue->mem_ops->op ? (vb)->vb2_queue->mem_ops->op(args) : 0)
>> +#define fail_memop(vb, op)
>>
>>  #define call_qop(q, op, args...)					\
>> -	(((q)->ops->op) ? ((q)->ops->op(args)) : 0)
>> +	((q)->ops->op ? (q)->ops->op(args) : 0)
>> +#define fail_qop(q, op)
>> +
>> +#define call_vb_qop(vb, op, args...)					\
>> +	((vb)->vb2_queue->ops->op ? (vb)->vb2_queue->ops->op(args) : 0)
>> +#define fail_vb_qop(vb, op)
>> +
>> +#endif
>>
>>  #define V4L2_BUFFER_MASK_FLAGS	(V4L2_BUF_FLAG_MAPPED | 
> V4L2_BUF_FLAG_QUEUED
>> | \ V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR | \
>> @@ -61,7 +112,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>>  		unsigned long size = PAGE_ALIGN(q->plane_sizes[plane]);
>>
>> -		mem_priv = call_memop(q, alloc, q->alloc_ctx[plane],
>> +		mem_priv = call_memop(vb, alloc, q->alloc_ctx[plane],
>>  				      size, q->gfp_flags);
>>  		if (IS_ERR_OR_NULL(mem_priv))
>>  			goto free;
>> @@ -73,9 +124,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
>>
>>  	return 0;
>>  free:
>> +	fail_memop(vb, alloc);
>>  	/* Free already allocated memory if one of the allocations failed */
>>  	for (; plane > 0; --plane) {
>> -		call_memop(q, put, vb->planes[plane - 1].mem_priv);
>> +		call_memop(vb, put, vb->planes[plane - 1].mem_priv);
>>  		vb->planes[plane - 1].mem_priv = NULL;
>>  	}
>>
>> @@ -87,11 +139,10 @@ free:
>>   */
>>  static void __vb2_buf_mem_free(struct vb2_buffer *vb)
>>  {
>> -	struct vb2_queue *q = vb->vb2_queue;
>>  	unsigned int plane;
>>
>>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>> -		call_memop(q, put, vb->planes[plane].mem_priv);
>> +		call_memop(vb, put, vb->planes[plane].mem_priv);
>>  		vb->planes[plane].mem_priv = NULL;
>>  		dprintk(3, "Freed plane %d of buffer %d\n", plane,
>>  			vb->v4l2_buf.index);
>> @@ -104,12 +155,11 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb)
>>   */
>>  static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
>>  {
>> -	struct vb2_queue *q = vb->vb2_queue;
>>  	unsigned int plane;
>>
>>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>>  		if (vb->planes[plane].mem_priv)
>> -			call_memop(q, put_userptr, vb->planes[plane].mem_priv);
>> +			call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
>>  		vb->planes[plane].mem_priv = NULL;
>>  	}
>>  }
>> @@ -118,15 +168,15 @@ static void __vb2_buf_userptr_put(struct vb2_buffer
>> *vb) * __vb2_plane_dmabuf_put() - release memory associated with
>>   * a DMABUF shared plane
>>   */
>> -static void __vb2_plane_dmabuf_put(struct vb2_queue *q, struct vb2_plane
>> *p) +static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct
>> vb2_plane *p) {
>>  	if (!p->mem_priv)
>>  		return;
>>
>>  	if (p->dbuf_mapped)
>> -		call_memop(q, unmap_dmabuf, p->mem_priv);
>> +		call_memop(vb, unmap_dmabuf, p->mem_priv);
>>
>> -	call_memop(q, detach_dmabuf, p->mem_priv);
>> +	call_memop(vb, detach_dmabuf, p->mem_priv);
>>  	dma_buf_put(p->dbuf);
>>  	memset(p, 0, sizeof(*p));
>>  }
>> @@ -137,11 +187,10 @@ static void __vb2_plane_dmabuf_put(struct vb2_queue
>> *q, struct vb2_plane *p) */
>>  static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb)
>>  {
>> -	struct vb2_queue *q = vb->vb2_queue;
>>  	unsigned int plane;
>>
>>  	for (plane = 0; plane < vb->num_planes; ++plane)
>> -		__vb2_plane_dmabuf_put(q, &vb->planes[plane]);
>> +		__vb2_plane_dmabuf_put(vb, &vb->planes[plane]);
>>  }
>>
>>  /**
>> @@ -246,10 +295,11 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum
>> v4l2_memory memory, * callback, if given. An error in initialization
>>  			 * results in queue setup failure.
>>  			 */
>> -			ret = call_qop(q, buf_init, vb);
>> +			ret = call_vb_qop(vb, buf_init, vb);
>>  			if (ret) {
>>  				dprintk(1, "Buffer %d %p initialization"
>>  					" failed\n", buffer, vb);
>> +				fail_vb_qop(vb, buf_init);
>>  				__vb2_buf_mem_free(vb);
>>  				kfree(vb);
>>  				break;
>> @@ -321,18 +371,77 @@ static int __vb2_queue_free(struct vb2_queue *q,
>> unsigned int buffers) }
>>
>>  	/* Call driver-provided cleanup function for each buffer, if provided */
>> -	if (q->ops->buf_cleanup) {
>> -		for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
>> -		     ++buffer) {
>> -			if (NULL == q->bufs[buffer])
>> -				continue;
>> -			q->ops->buf_cleanup(q->bufs[buffer]);
>> -		}
>> +	for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
>> +	     ++buffer) {
>> +		if (q->bufs[buffer])
>> +			call_vb_qop(q->bufs[buffer], buf_cleanup, q->bufs[buffer]);
>>  	}
>>
>>  	/* Release video buffer memory */
>>  	__vb2_free_mem(q, buffers);
>>
> 
> I would have put the code below in a separate function, but that's up to you.

Yeah, it was a borderline situation. I prefer to keep it in, but if it needs
to be extended in the future then it should probably be split up.

Regards,

	Hans

> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> +	/*
>> +	 * Check that all the calls were balances during the life-time of this
>> +	 * queue. If not (or if the debug level is 1 or up), then dump the
>> +	 * counters to the kernel log.
>> +	 */
>> +	if (q->num_buffers) {
>> +		bool unbalanced = q->cnt_start_streaming != q->cnt_stop_streaming ||
>> +				  q->cnt_wait_prepare != q->cnt_wait_finish;
>> +
>> +		if (unbalanced || debug) {
>> +			pr_info("vb2: counters for queue %p:%s\n", q,
>> +				unbalanced ? " UNBALANCED!" : "");
>> +			pr_info("vb2:     setup: %u start_streaming: %u stop_streaming: 
> %u\n",
>> +				q->cnt_queue_setup, q->cnt_start_streaming,
>> +				q->cnt_stop_streaming);
>> +			pr_info("vb2:     wait_prepare: %u wait_finish: %u\n",
>> +				q->cnt_wait_prepare, q->cnt_wait_finish);
>> +		}
>> +		q->cnt_queue_setup = 0;
>> +		q->cnt_wait_prepare = 0;
>> +		q->cnt_wait_finish = 0;
>> +		q->cnt_start_streaming = 0;
>> +		q->cnt_stop_streaming = 0;
>> +	}
>> +	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
>> +		struct vb2_buffer *vb = q->bufs[buffer];
>> +		bool unbalanced = vb->cnt_mem_alloc != vb->cnt_mem_put ||
>> +				  vb->cnt_mem_prepare != vb->cnt_mem_finish ||
>> +				  vb->cnt_mem_get_userptr != vb->cnt_mem_put_userptr ||
>> +				  vb->cnt_mem_attach_dmabuf != vb->cnt_mem_detach_dmabuf ||
>> +				  vb->cnt_mem_map_dmabuf != vb->cnt_mem_unmap_dmabuf ||
>> +				  vb->cnt_buf_queue != vb->cnt_buf_done ||
>> +				  vb->cnt_buf_prepare != vb->cnt_buf_finish ||
>> +				  vb->cnt_buf_init != vb->cnt_buf_cleanup;
>> +
>> +		if (unbalanced || debug) {
>> +			pr_info("vb2:   counters for queue %p, buffer %d:%s\n",
>> +				q, buffer, unbalanced ? " UNBALANCED!" : "");
>> +			pr_info("vb2:     buf_init: %u buf_cleanup: %u buf_prepare: %u
>> buf_finish: %u\n", +				vb->cnt_buf_init, vb->cnt_buf_cleanup,
>> +				vb->cnt_buf_prepare, vb->cnt_buf_finish);
>> +			pr_info("vb2:     buf_queue: %u buf_done: %u\n",
>> +				vb->cnt_buf_queue, vb->cnt_buf_done);
>> +			pr_info("vb2:     alloc: %u put: %u prepare: %u finish: %u mmap: 
> %u\n",
>> +				vb->cnt_mem_alloc, vb->cnt_mem_put,
>> +				vb->cnt_mem_prepare, vb->cnt_mem_finish,
>> +				vb->cnt_mem_mmap);
>> +			pr_info("vb2:     get_userptr: %u put_userptr: %u\n",
>> +				vb->cnt_mem_get_userptr, vb->cnt_mem_put_userptr);
>> +			pr_info("vb2:     attach_dmabuf: %u detach_dmabuf: %u map_dmabuf: 
> %u
>> unmap_dmabuf: %u\n", +				vb->cnt_mem_attach_dmabuf,
>> vb->cnt_mem_detach_dmabuf,
>> +				vb->cnt_mem_map_dmabuf, vb->cnt_mem_unmap_dmabuf);
>> +			pr_info("vb2:     get_dmabuf: %u num_users: %u vaddr: %u cookie: 
> %u\n",
>> +				vb->cnt_mem_get_dmabuf,
>> +				vb->cnt_mem_num_users,
>> +				vb->cnt_mem_vaddr,
>> +				vb->cnt_mem_cookie);
>> +		}
>> +	}
>> +#endif
>> +
>>  	/* Free videobuf buffers */
>>  	for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
>>  	     ++buffer) {
>> @@ -424,7 +533,7 @@ static bool __buffer_in_use(struct vb2_queue *q, struct
>> vb2_buffer *vb) * case anyway. If num_users() returns more than 1,
>>  		 * we are not the only user of the plane's memory.
>>  		 */
>> -		if (mem_priv && call_memop(q, num_users, mem_priv) > 1)
>> +		if (mem_priv && call_memop(vb, num_users, mem_priv) > 1)
>>  			return true;
>>  	}
>>  	return false;
>> @@ -703,8 +812,10 @@ static int __reqbufs(struct vb2_queue *q, struct
>> v4l2_requestbuffers *req) */
>>  	ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes,
>>  		       q->plane_sizes, q->alloc_ctx);
>> -	if (ret)
>> +	if (ret) {
>> +		fail_qop(q, queue_setup);
>>  		return ret;
>> +	}
>>
>>  	/* Finally, allocate buffers and video memory */
>>  	ret = __vb2_queue_alloc(q, req->memory, num_buffers, num_planes);
>> @@ -723,6 +834,8 @@ static int __reqbufs(struct vb2_queue *q, struct
>> v4l2_requestbuffers *req)
>>
>>  		ret = call_qop(q, queue_setup, q, NULL, &num_buffers,
>>  			       &num_planes, q->plane_sizes, q->alloc_ctx);
>> +		if (ret)
>> +			fail_qop(q, queue_setup);
>>
>>  		if (!ret && allocated_buffers < num_buffers)
>>  			ret = -ENOMEM;
>> @@ -803,8 +916,10 @@ static int __create_bufs(struct vb2_queue *q, struct
>> v4l2_create_buffers *create */
>>  	ret = call_qop(q, queue_setup, q, &create->format, &num_buffers,
>>  		       &num_planes, q->plane_sizes, q->alloc_ctx);
>> -	if (ret)
>> +	if (ret) {
>> +		fail_qop(q, queue_setup);
>>  		return ret;
>> +	}
>>
>>  	/* Finally, allocate buffers and video memory */
>>  	ret = __vb2_queue_alloc(q, create->memory, num_buffers,
>> @@ -828,6 +943,8 @@ static int __create_bufs(struct vb2_queue *q, struct
>> v4l2_create_buffers *create */
>>  		ret = call_qop(q, queue_setup, q, &create->format, &num_buffers,
>>  			       &num_planes, q->plane_sizes, q->alloc_ctx);
>> +		if (ret)
>> +			fail_qop(q, queue_setup);
>>
>>  		if (!ret && allocated_buffers < num_buffers)
>>  			ret = -ENOMEM;
>> @@ -882,12 +999,10 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
>>   */
>>  void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no)
>>  {
>> -	struct vb2_queue *q = vb->vb2_queue;
>> -
>>  	if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
>>  		return NULL;
>>
>> -	return call_memop(q, vaddr, vb->planes[plane_no].mem_priv);
>> +	return call_memop(vb, vaddr, vb->planes[plane_no].mem_priv);
>>
>>  }
>>  EXPORT_SYMBOL_GPL(vb2_plane_vaddr);
>> @@ -905,12 +1020,10 @@ EXPORT_SYMBOL_GPL(vb2_plane_vaddr);
>>   */
>>  void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int plane_no)
>>  {
>> -	struct vb2_queue *q = vb->vb2_queue;
>> -
>>  	if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
>>  		return NULL;
>>
>> -	return call_memop(q, cookie, vb->planes[plane_no].mem_priv);
>> +	return call_memop(vb, cookie, vb->planes[plane_no].mem_priv);
>>  }
>>  EXPORT_SYMBOL_GPL(vb2_plane_cookie);
>>
>> @@ -938,12 +1051,19 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum
>> vb2_buffer_state state) if (state != VB2_BUF_STATE_DONE && state !=
>> VB2_BUF_STATE_ERROR) return;
>>
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> +	/*
>> +	 * Although this is not a callback, it still does have to balance
>> +	 * with the buf_queue op. So update this counter manually.
>> +	 */
>> +	vb->cnt_buf_done++;
>> +#endif
>>  	dprintk(4, "Done processing on buffer %d, state: %d\n",
>>  			vb->v4l2_buf.index, state);
>>
>>  	/* sync buffers */
>>  	for (plane = 0; plane < vb->num_planes; ++plane)
>> -		call_memop(q, finish, vb->planes[plane].mem_priv);
>> +		call_memop(vb, finish, vb->planes[plane].mem_priv);
>>
>>  	/* Add the buffer to the done buffers list */
>>  	spin_lock_irqsave(&q->done_lock, flags);
>> @@ -1067,19 +1187,20 @@ static int __qbuf_userptr(struct vb2_buffer *vb,
>> const struct v4l2_buffer *b)
>>
>>  		/* Release previously acquired memory if present */
>>  		if (vb->planes[plane].mem_priv)
>> -			call_memop(q, put_userptr, vb->planes[plane].mem_priv);
>> +			call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
>>
>>  		vb->planes[plane].mem_priv = NULL;
>>  		vb->v4l2_planes[plane].m.userptr = 0;
>>  		vb->v4l2_planes[plane].length = 0;
>>
>>  		/* Acquire each plane's memory */
>> -		mem_priv = call_memop(q, get_userptr, q->alloc_ctx[plane],
>> +		mem_priv = call_memop(vb, get_userptr, q->alloc_ctx[plane],
>>  				      planes[plane].m.userptr,
>>  				      planes[plane].length, write);
>>  		if (IS_ERR_OR_NULL(mem_priv)) {
>>  			dprintk(1, "qbuf: failed acquiring userspace "
>>  						"memory for plane %d\n", plane);
>> +			fail_memop(vb, get_userptr);
>>  			ret = mem_priv ? PTR_ERR(mem_priv) : -EINVAL;
>>  			goto err;
>>  		}
>> @@ -1090,9 +1211,10 @@ static int __qbuf_userptr(struct vb2_buffer *vb,
>> const struct v4l2_buffer *b) * Call driver-specific initialization on the
>> newly acquired buffer, * if provided.
>>  	 */
>> -	ret = call_qop(q, buf_init, vb);
>> +	ret = call_vb_qop(vb, buf_init, vb);
>>  	if (ret) {
>>  		dprintk(1, "qbuf: buffer initialization failed\n");
>> +		fail_vb_qop(vb, buf_init);
>>  		goto err;
>>  	}
>>
>> @@ -1108,7 +1230,7 @@ err:
>>  	/* In case of errors, release planes that were already acquired */
>>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>>  		if (vb->planes[plane].mem_priv)
>> -			call_memop(q, put_userptr, vb->planes[plane].mem_priv);
>> +			call_memop(vb, put_userptr, vb->planes[plane].mem_priv);
>>  		vb->planes[plane].mem_priv = NULL;
>>  		vb->v4l2_planes[plane].m.userptr = 0;
>>  		vb->v4l2_planes[plane].length = 0;
>> @@ -1173,14 +1295,15 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb,
>> const struct v4l2_buffer *b) dprintk(1, "qbuf: buffer for plane %d
>> changed\n", plane);
>>
>>  		/* Release previously acquired memory if present */
>> -		__vb2_plane_dmabuf_put(q, &vb->planes[plane]);
>> +		__vb2_plane_dmabuf_put(vb, &vb->planes[plane]);
>>  		memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane));
>>
>>  		/* Acquire each plane's memory */
>> -		mem_priv = call_memop(q, attach_dmabuf, q->alloc_ctx[plane],
>> +		mem_priv = call_memop(vb, attach_dmabuf, q->alloc_ctx[plane],
>>  			dbuf, planes[plane].length, write);
>>  		if (IS_ERR(mem_priv)) {
>>  			dprintk(1, "qbuf: failed to attach dmabuf\n");
>> +			fail_memop(vb, attach_dmabuf);
>>  			ret = PTR_ERR(mem_priv);
>>  			dma_buf_put(dbuf);
>>  			goto err;
>> @@ -1195,10 +1318,11 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb,
>> const struct v4l2_buffer *b) * the buffer(s)..
>>  	 */
>>  	for (plane = 0; plane < vb->num_planes; ++plane) {
>> -		ret = call_memop(q, map_dmabuf, vb->planes[plane].mem_priv);
>> +		ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
>>  		if (ret) {
>>  			dprintk(1, "qbuf: failed to map dmabuf for plane %d\n",
>>  				plane);
>> +			fail_memop(vb, map_dmabuf);
>>  			goto err;
>>  		}
>>  		vb->planes[plane].dbuf_mapped = 1;
>> @@ -1208,9 +1332,10 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const
>> struct v4l2_buffer *b) * Call driver-specific initialization on the newly
>> acquired buffer, * if provided.
>>  	 */
>> -	ret = call_qop(q, buf_init, vb);
>> +	ret = call_vb_qop(vb, buf_init, vb);
>>  	if (ret) {
>>  		dprintk(1, "qbuf: buffer initialization failed\n");
>> +		fail_vb_qop(vb, buf_init);
>>  		goto err;
>>  	}
>>
>> @@ -1242,9 +1367,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
>>
>>  	/* sync buffers */
>>  	for (plane = 0; plane < vb->num_planes; ++plane)
>> -		call_memop(q, prepare, vb->planes[plane].mem_priv);
>> +		call_memop(vb, prepare, vb->planes[plane].mem_priv);
>>
>> -	q->ops->buf_queue(vb);
>> +	call_vb_qop(vb, buf_queue, vb);
>>  }
>>
>>  static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer
>> *b) @@ -1295,8 +1420,11 @@ static int __buf_prepare(struct vb2_buffer *vb,
>> const struct v4l2_buffer *b) ret = -EINVAL;
>>  	}
>>
>> -	if (!ret)
>> -		ret = call_qop(q, buf_prepare, vb);
>> +	if (!ret) {
>> +		ret = call_vb_qop(vb, buf_prepare, vb);
>> +		if (ret)
>> +			fail_vb_qop(vb, buf_prepare);
>> +	}
>>  	if (ret)
>>  		dprintk(1, "qbuf: buffer preparation failed: %d\n", ret);
>>  	vb->state = ret ? VB2_BUF_STATE_DEQUEUED : VB2_BUF_STATE_PREPARED;
>> @@ -1393,6 +1521,8 @@ static int vb2_start_streaming(struct vb2_queue *q)
>>
>>  	/* Tell the driver to start streaming */
>>  	ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
>> +	if (ret)
>> +		fail_qop(q, start_streaming);
>>
>>  	/*
>>  	 * If there are not enough buffers queued to start streaming, then
>> @@ -1635,7 +1765,7 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
>>  		for (i = 0; i < vb->num_planes; ++i) {
>>  			if (!vb->planes[i].dbuf_mapped)
>>  				continue;
>> -			call_memop(q, unmap_dmabuf, vb->planes[i].mem_priv);
>> +			call_memop(vb, unmap_dmabuf, vb->planes[i].mem_priv);
>>  			vb->planes[i].dbuf_mapped = 0;
>>  		}
>>  }
>> @@ -1653,7 +1783,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q,
>> struct v4l2_buffer *b, bool n if (ret < 0)
>>  		return ret;
>>
>> -	ret = call_qop(q, buf_finish, vb);
>> +	ret = call_vb_qop(vb, buf_finish, vb);
>>  	if (ret) {
>>  		dprintk(1, "dqbuf: buffer finish failed\n");
>>  		return ret;
>> @@ -1946,10 +2076,11 @@ int vb2_expbuf(struct vb2_queue *q, struct
>> v4l2_exportbuffer *eb)
>>
>>  	vb_plane = &vb->planes[eb->plane];
>>
>> -	dbuf = call_memop(q, get_dmabuf, vb_plane->mem_priv, eb->flags &
>> O_ACCMODE); +	dbuf = call_memop(vb, get_dmabuf, vb_plane->mem_priv,
>> eb->flags & O_ACCMODE); if (IS_ERR_OR_NULL(dbuf)) {
>>  		dprintk(1, "Failed to export buffer %d, plane %d\n",
>>  			eb->index, eb->plane);
>> +		fail_memop(vb, get_dmabuf);
>>  		return -EINVAL;
>>  	}
>>
>> @@ -2041,9 +2172,11 @@ int vb2_mmap(struct vb2_queue *q, struct
>> vm_area_struct *vma) return -EINVAL;
>>  	}
>>
>> -	ret = call_memop(q, mmap, vb->planes[plane].mem_priv, vma);
>> -	if (ret)
>> +	ret = call_memop(vb, mmap, vb->planes[plane].mem_priv, vma);
>> +	if (ret) {
>> +		fail_memop(vb, mmap);
>>  		return ret;
>> +	}
>>
>>  	dprintk(3, "Buffer %d, plane %d successfully mapped\n", buffer, plane);
>>  	return 0;
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index bef53ce..f04eb28 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -203,6 +203,37 @@ struct vb2_buffer {
>>  	struct list_head	done_entry;
>>
>>  	struct vb2_plane	planes[VIDEO_MAX_PLANES];
>> +
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> +	/*
>> +	 * Counters for how often these buffer-related ops are
>> +	 * called. Used to check for unbalanced ops.
>> +	 */
>> +	u32		cnt_mem_alloc;
>> +	u32		cnt_mem_put;
>> +	u32		cnt_mem_get_dmabuf;
>> +	u32		cnt_mem_get_userptr;
>> +	u32		cnt_mem_put_userptr;
>> +	u32		cnt_mem_prepare;
>> +	u32		cnt_mem_finish;
>> +	u32		cnt_mem_attach_dmabuf;
>> +	u32		cnt_mem_detach_dmabuf;
>> +	u32		cnt_mem_map_dmabuf;
>> +	u32		cnt_mem_unmap_dmabuf;
>> +	u32		cnt_mem_vaddr;
>> +	u32		cnt_mem_cookie;
>> +	u32		cnt_mem_num_users;
>> +	u32		cnt_mem_mmap;
>> +
>> +	u32		cnt_buf_init;
>> +	u32		cnt_buf_prepare;
>> +	u32		cnt_buf_finish;
>> +	u32		cnt_buf_cleanup;
>> +	u32		cnt_buf_queue;
>> +
>> +	/* This counts the number of calls to vb2_buffer_done() */
>> +	u32		cnt_buf_done;
>> +#endif
>>  };
>>
>>  /**
>> @@ -364,6 +395,18 @@ struct vb2_queue {
>>  	unsigned int			retry_start_streaming:1;
>>
>>  	struct vb2_fileio_data		*fileio;
>> +
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> +	/*
>> +	 * Counters for how often these queue-related ops are
>> +	 * called. Used to check for unbalanced ops.
>> +	 */
>> +	u32				cnt_queue_setup;
>> +	u32				cnt_wait_prepare;
>> +	u32				cnt_wait_finish;
>> +	u32				cnt_start_streaming;
>> +	u32				cnt_stop_streaming;
>> +#endif
>>  };
>>
>>  void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no);
> 

Re: [REVIEWv2 PATCH 05/15] vb2: change result code of buf_finish to void

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tuesday 25 February 2014 13:52:45 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The buf_finish op should always work, so change the return type to void.
> Update the few drivers that use it. Note that buf_finish can be called
> both when the DMA is streaming and when it isn't (during queue_cancel).

I'm not sure what branch this series is based on, but in the latest linuxtv 
master branch buf_finish is only called from vb2_internal_dqbuf(), itself only 
called from vb2_dqbuf() and __vb2_perform_fileio().

I would split the patch in two, one patch to change the buf_finish behaviour 
inside the drivers and update the buf_finish documentation to explain when it 
can be called in more details, and another one to change its return value to 
void.

> Update drivers to check that where appropriate.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Pawel Osciak <pawel@osciak.com>
> Reviewed-by: Pawel Osciak <pawel@osciak.com>
> ---
>  drivers/media/parport/bw-qcam.c                 |  6 ++++--
>  drivers/media/pci/sta2x11/sta2x11_vip.c         |  7 +++----
>  drivers/media/platform/marvell-ccic/mcam-core.c |  3 +--
>  drivers/media/usb/pwc/pwc-if.c                  | 16 +++++++++-------
>  drivers/media/usb/uvc/uvc_queue.c               |  6 +++---
>  drivers/media/v4l2-core/videobuf2-core.c        |  6 +-----
>  drivers/staging/media/go7007/go7007-v4l2.c      |  3 +--
>  include/media/videobuf2-core.h                  |  2 +-
>  8 files changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/parport/bw-qcam.c
> b/drivers/media/parport/bw-qcam.c index d12bd33..8d69bfb 100644
> --- a/drivers/media/parport/bw-qcam.c
> +++ b/drivers/media/parport/bw-qcam.c
> @@ -667,13 +667,16 @@ static void buffer_queue(struct vb2_buffer *vb)
>  	vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
>  }
> 
> -static int buffer_finish(struct vb2_buffer *vb)
> +static void buffer_finish(struct vb2_buffer *vb)
>  {
>  	struct qcam *qcam = vb2_get_drv_priv(vb->vb2_queue);
>  	void *vbuf = vb2_plane_vaddr(vb, 0);
>  	int size = vb->vb2_queue->plane_sizes[0];
>  	int len;
> 
> +	if (!vb2_is_streaming(vb->vb2_queue))
> +		return;
> +
>  	mutex_lock(&qcam->lock);
>  	parport_claim_or_block(qcam->pdev);
> 
> @@ -691,7 +694,6 @@ static int buffer_finish(struct vb2_buffer *vb)
>  	if (len != size)
>  		vb->state = VB2_BUF_STATE_ERROR;
>  	vb2_set_plane_payload(vb, 0, len);
> -	return 0;
>  }
> 
>  static struct vb2_ops qcam_video_qops = {
> diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c
> b/drivers/media/pci/sta2x11/sta2x11_vip.c index e5cfb6c..bb11443 100644
> --- a/drivers/media/pci/sta2x11/sta2x11_vip.c
> +++ b/drivers/media/pci/sta2x11/sta2x11_vip.c
> @@ -327,7 +327,7 @@ static void buffer_queue(struct vb2_buffer *vb)
>  	}
>  	spin_unlock(&vip->lock);
>  }
> -static int buffer_finish(struct vb2_buffer *vb)
> +static void buffer_finish(struct vb2_buffer *vb)
>  {
>  	struct sta2x11_vip *vip = vb2_get_drv_priv(vb->vb2_queue);
>  	struct vip_buffer *vip_buf = to_vip_buffer(vb);
> @@ -337,9 +337,8 @@ static int buffer_finish(struct vb2_buffer *vb)
>  	list_del_init(&vip_buf->list);
>  	spin_unlock(&vip->lock);
> 
> -	vip_active_buf_next(vip);
> -
> -	return 0;
> +	if (vb2_is_streaming(vb->vb2_queue))
> +		vip_active_buf_next(vip);
>  }
> 
>  static int start_streaming(struct vb2_queue *vq, unsigned int count)
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
> b/drivers/media/platform/marvell-ccic/mcam-core.c index 32fab30..8b34c48
> 100644
> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
> @@ -1238,7 +1238,7 @@ static int mcam_vb_sg_buf_prepare(struct vb2_buffer
> *vb) return 0;
>  }
> 
> -static int mcam_vb_sg_buf_finish(struct vb2_buffer *vb)
> +static void mcam_vb_sg_buf_finish(struct vb2_buffer *vb)
>  {
>  	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
>  	struct sg_table *sg_table = vb2_dma_sg_plane_desc(vb, 0);
> @@ -1246,7 +1246,6 @@ static int mcam_vb_sg_buf_finish(struct vb2_buffer
> *vb) if (sg_table)
>  		dma_unmap_sg(cam->dev, sg_table->sgl,
>  				sg_table->nents, DMA_FROM_DEVICE);
> -	return 0;
>  }
> 
>  static void mcam_vb_sg_buf_cleanup(struct vb2_buffer *vb)
> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> index abf365a..b9c9f10 100644
> --- a/drivers/media/usb/pwc/pwc-if.c
> +++ b/drivers/media/usb/pwc/pwc-if.c
> @@ -614,17 +614,19 @@ static int buffer_prepare(struct vb2_buffer *vb)
>  	return 0;
>  }
> 
> -static int buffer_finish(struct vb2_buffer *vb)
> +static void buffer_finish(struct vb2_buffer *vb)
>  {
>  	struct pwc_device *pdev = vb2_get_drv_priv(vb->vb2_queue);
>  	struct pwc_frame_buf *buf = container_of(vb, struct pwc_frame_buf, vb);
> 
> -	/*
> -	 * Application has called dqbuf and is getting back a buffer we've
> -	 * filled, take the pwc data we've stored in buf->data and decompress
> -	 * it into a usable format, storing the result in the vb2_buffer
> -	 */
> -	return pwc_decompress(pdev, buf);
> +	if (vb->state == VB2_BUF_STATE_DONE) {
> +		/*
> +		 * Application has called dqbuf and is getting back a buffer we've
> +		 * filled, take the pwc data we've stored in buf->data and decompress
> +		 * it into a usable format, storing the result in the vb2_buffer
> +		 */
> +		pwc_decompress(pdev, buf);
> +	}
>  }
> 
>  static void buffer_cleanup(struct vb2_buffer *vb)
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c index cd962be..db5984e 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -104,15 +104,15 @@ static void uvc_buffer_queue(struct vb2_buffer *vb)
>  	spin_unlock_irqrestore(&queue->irqlock, flags);
>  }
> 
> -static int uvc_buffer_finish(struct vb2_buffer *vb)
> +static void uvc_buffer_finish(struct vb2_buffer *vb)
>  {
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
>  	struct uvc_streaming *stream =
>  			container_of(queue, struct uvc_streaming, queue);
>  	struct uvc_buffer *buf = container_of(vb, struct uvc_buffer, buf);
> 
> -	uvc_video_clock_update(stream, &vb->v4l2_buf, buf);
> -	return 0;
> +	if (vb2_is_streaming(vb->vb2_queue))
> +		uvc_video_clock_update(stream, &vb->v4l2_buf, buf);
>  }
> 
>  static void uvc_wait_prepare(struct vb2_queue *vq)
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 8f1578b..59bfd85 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1783,11 +1783,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q,
> struct v4l2_buffer *b, bool n if (ret < 0)
>  		return ret;
> 
> -	ret = call_vb_qop(vb, buf_finish, vb);
> -	if (ret) {
> -		dprintk(1, "dqbuf: buffer finish failed\n");
> -		return ret;
> -	}
> +	call_vb_qop(vb, buf_finish, vb);
> 
>  	switch (vb->state) {
>  	case VB2_BUF_STATE_DONE:
> diff --git a/drivers/staging/media/go7007/go7007-v4l2.c
> b/drivers/staging/media/go7007/go7007-v4l2.c index edc52e2..3a01576 100644
> --- a/drivers/staging/media/go7007/go7007-v4l2.c
> +++ b/drivers/staging/media/go7007/go7007-v4l2.c
> @@ -470,7 +470,7 @@ static int go7007_buf_prepare(struct vb2_buffer *vb)
>  	return 0;
>  }
> 
> -static int go7007_buf_finish(struct vb2_buffer *vb)
> +static void go7007_buf_finish(struct vb2_buffer *vb)
>  {
>  	struct vb2_queue *vq = vb->vb2_queue;
>  	struct go7007 *go = vb2_get_drv_priv(vq);
> @@ -483,7 +483,6 @@ static int go7007_buf_finish(struct vb2_buffer *vb)
>  			V4L2_BUF_FLAG_PFRAME);
>  	buf->flags |= frame_type_flag;
>  	buf->field = V4L2_FIELD_NONE;
> -	return 0;
>  }
> 
>  static int go7007_start_streaming(struct vb2_queue *q, unsigned int count)
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index f04eb28..f443ce0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -311,7 +311,7 @@ struct vb2_ops {
> 
>  	int (*buf_init)(struct vb2_buffer *vb);
>  	int (*buf_prepare)(struct vb2_buffer *vb);
> -	int (*buf_finish)(struct vb2_buffer *vb);
> +	void (*buf_finish)(struct vb2_buffer *vb);
>  	void (*buf_cleanup)(struct vb2_buffer *vb);
> 
>  	int (*start_streaming)(struct vb2_queue *q, unsigned int count);

-- 
Regards,

Laurent Pinchart

Re: [REVIEWv2 PATCH 05/15] vb2: change result code of buf_finish to void

[Laurent Pinchart]

On Thursday 27 February 2014 12:23:58 Laurent Pinchart wrote:
> On Tuesday 25 February 2014 13:52:45 Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > The buf_finish op should always work, so change the return type to void.
> > Update the few drivers that use it. Note that buf_finish can be called
> > both when the DMA is streaming and when it isn't (during queue_cancel).
> 
> I'm not sure what branch this series is based on, but in the latest linuxtv
> master branch buf_finish is only called from vb2_internal_dqbuf(), itself
> only called from vb2_dqbuf() and __vb2_perform_fileio().

And I should have read the next patches before commenting on this :-)

> I would split the patch in two, one patch to change the buf_finish behaviour
> inside the drivers and update the buf_finish documentation to explain when
> it can be called in more details, and another one to change its return
> value to void.

Let's reorder the patches if you don't mind. I would still split this as 
described in 5.1 and 5.2. I would then move 5.2 before 5.1, and either squash 
6 with 5.1 or move 6 before 5.1.

> > Update drivers to check that where appropriate.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > Acked-by: Pawel Osciak <pawel@osciak.com>
> > Reviewed-by: Pawel Osciak <pawel@osciak.com>
> > ---
> > 
> >  drivers/media/parport/bw-qcam.c                 |  6 ++++--
> >  drivers/media/pci/sta2x11/sta2x11_vip.c         |  7 +++----
> >  drivers/media/platform/marvell-ccic/mcam-core.c |  3 +--
> >  drivers/media/usb/pwc/pwc-if.c                  | 16 +++++++++-------
> >  drivers/media/usb/uvc/uvc_queue.c               |  6 +++---
> >  drivers/media/v4l2-core/videobuf2-core.c        |  6 +-----
> >  drivers/staging/media/go7007/go7007-v4l2.c      |  3 +--
> >  include/media/videobuf2-core.h                  |  2 +-
> >  8 files changed, 23 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/media/parport/bw-qcam.c
> > b/drivers/media/parport/bw-qcam.c index d12bd33..8d69bfb 100644
> > --- a/drivers/media/parport/bw-qcam.c
> > +++ b/drivers/media/parport/bw-qcam.c
> > @@ -667,13 +667,16 @@ static void buffer_queue(struct vb2_buffer *vb)
> > 
> >  	vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> >  
> >  }
> > 
> > -static int buffer_finish(struct vb2_buffer *vb)
> > +static void buffer_finish(struct vb2_buffer *vb)
> > 
> >  {
> >  
> >  	struct qcam *qcam = vb2_get_drv_priv(vb->vb2_queue);
> >  	void *vbuf = vb2_plane_vaddr(vb, 0);
> >  	int size = vb->vb2_queue->plane_sizes[0];
> >  	int len;
> > 
> > +	if (!vb2_is_streaming(vb->vb2_queue))
> > +		return;
> > +
> > 
> >  	mutex_lock(&qcam->lock);
> >  	parport_claim_or_block(qcam->pdev);
> > 
> > @@ -691,7 +694,6 @@ static int buffer_finish(struct vb2_buffer *vb)
> > 
> >  	if (len != size)
> >  	
> >  		vb->state = VB2_BUF_STATE_ERROR;
> >  	
> >  	vb2_set_plane_payload(vb, 0, len);
> > 
> > -	return 0;
> > 
> >  }
> >  
> >  static struct vb2_ops qcam_video_qops = {
> > 
> > diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c
> > b/drivers/media/pci/sta2x11/sta2x11_vip.c index e5cfb6c..bb11443 100644
> > --- a/drivers/media/pci/sta2x11/sta2x11_vip.c
> > +++ b/drivers/media/pci/sta2x11/sta2x11_vip.c
> > @@ -327,7 +327,7 @@ static void buffer_queue(struct vb2_buffer *vb)
> > 
> >  	}
> >  	spin_unlock(&vip->lock);
> >  
> >  }
> > 
> > -static int buffer_finish(struct vb2_buffer *vb)
> > +static void buffer_finish(struct vb2_buffer *vb)
> > 
> >  {
> >  
> >  	struct sta2x11_vip *vip = vb2_get_drv_priv(vb->vb2_queue);
> >  	struct vip_buffer *vip_buf = to_vip_buffer(vb);
> > 
> > @@ -337,9 +337,8 @@ static int buffer_finish(struct vb2_buffer *vb)
> > 
> >  	list_del_init(&vip_buf->list);
> >  	spin_unlock(&vip->lock);
> > 
> > -	vip_active_buf_next(vip);
> > -
> > -	return 0;
> > +	if (vb2_is_streaming(vb->vb2_queue))
> > +		vip_active_buf_next(vip);
> > 
> >  }
> >  
> >  static int start_streaming(struct vb2_queue *vq, unsigned int count)
> > 
> > diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
> > b/drivers/media/platform/marvell-ccic/mcam-core.c index 32fab30..8b34c48
> > 100644
> > --- a/drivers/media/platform/marvell-ccic/mcam-core.c
> > +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
> > @@ -1238,7 +1238,7 @@ static int mcam_vb_sg_buf_prepare(struct vb2_buffer
> > *vb) return 0;
> > 
> >  }
> > 
> > -static int mcam_vb_sg_buf_finish(struct vb2_buffer *vb)
> > +static void mcam_vb_sg_buf_finish(struct vb2_buffer *vb)
> > 
> >  {
> >  
> >  	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
> >  	struct sg_table *sg_table = vb2_dma_sg_plane_desc(vb, 0);
> > 
> > @@ -1246,7 +1246,6 @@ static int mcam_vb_sg_buf_finish(struct vb2_buffer
> > *vb) if (sg_table)
> > 
> >  		dma_unmap_sg(cam->dev, sg_table->sgl,
> >  		
> >  				sg_table->nents, DMA_FROM_DEVICE);
> > 
> > -	return 0;
> > 
> >  }
> >  
> >  static void mcam_vb_sg_buf_cleanup(struct vb2_buffer *vb)
> > 
> > diff --git a/drivers/media/usb/pwc/pwc-if.c
> > b/drivers/media/usb/pwc/pwc-if.c index abf365a..b9c9f10 100644
> > --- a/drivers/media/usb/pwc/pwc-if.c
> > +++ b/drivers/media/usb/pwc/pwc-if.c
> > @@ -614,17 +614,19 @@ static int buffer_prepare(struct vb2_buffer *vb)
> > 
> >  	return 0;
> >  
> >  }
> > 
> > -static int buffer_finish(struct vb2_buffer *vb)
> > +static void buffer_finish(struct vb2_buffer *vb)
> > 
> >  {
> >  
> >  	struct pwc_device *pdev = vb2_get_drv_priv(vb->vb2_queue);
> >  	struct pwc_frame_buf *buf = container_of(vb, struct pwc_frame_buf, 
vb);
> > 
> > -	/*
> > -	 * Application has called dqbuf and is getting back a buffer we've
> > -	 * filled, take the pwc data we've stored in buf->data and decompress
> > -	 * it into a usable format, storing the result in the vb2_buffer
> > -	 */
> > -	return pwc_decompress(pdev, buf);
> > +	if (vb->state == VB2_BUF_STATE_DONE) {
> > +		/*
> > +		 * Application has called dqbuf and is getting back a buffer 
we've
> > +		 * filled, take the pwc data we've stored in buf->data and 
decompress
> > +		 * it into a usable format, storing the result in the vb2_buffer
> > +		 */
> > +		pwc_decompress(pdev, buf);
> > +	}
> > 
> >  }
> >  
> >  static void buffer_cleanup(struct vb2_buffer *vb)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_queue.c
> > b/drivers/media/usb/uvc/uvc_queue.c index cd962be..db5984e 100644
> > --- a/drivers/media/usb/uvc/uvc_queue.c
> > +++ b/drivers/media/usb/uvc/uvc_queue.c
> > @@ -104,15 +104,15 @@ static void uvc_buffer_queue(struct vb2_buffer *vb)
> > 
> >  	spin_unlock_irqrestore(&queue->irqlock, flags);
> >  
> >  }
> > 
> > -static int uvc_buffer_finish(struct vb2_buffer *vb)
> > +static void uvc_buffer_finish(struct vb2_buffer *vb)
> > 
> >  {
> >  
> >  	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
> >  	struct uvc_streaming *stream =
> >  	
> >  			container_of(queue, struct uvc_streaming, queue);
> >  	
> >  	struct uvc_buffer *buf = container_of(vb, struct uvc_buffer, buf);
> > 
> > -	uvc_video_clock_update(stream, &vb->v4l2_buf, buf);
> > -	return 0;
> > +	if (vb2_is_streaming(vb->vb2_queue))
> > +		uvc_video_clock_update(stream, &vb->v4l2_buf, buf);
> > 
> >  }
> >  
> >  static void uvc_wait_prepare(struct vb2_queue *vq)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> > b/drivers/media/v4l2-core/videobuf2-core.c index 8f1578b..59bfd85 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1783,11 +1783,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q,
> > struct v4l2_buffer *b, bool n if (ret < 0)
> > 
> >  		return ret;
> > 
> > -	ret = call_vb_qop(vb, buf_finish, vb);
> > -	if (ret) {
> > -		dprintk(1, "dqbuf: buffer finish failed\n");
> > -		return ret;
> > -	}
> > +	call_vb_qop(vb, buf_finish, vb);
> > 
> >  	switch (vb->state) {
> > 
> >  	case VB2_BUF_STATE_DONE:
> > diff --git a/drivers/staging/media/go7007/go7007-v4l2.c
> > b/drivers/staging/media/go7007/go7007-v4l2.c index edc52e2..3a01576 100644
> > --- a/drivers/staging/media/go7007/go7007-v4l2.c
> > +++ b/drivers/staging/media/go7007/go7007-v4l2.c
> > @@ -470,7 +470,7 @@ static int go7007_buf_prepare(struct vb2_buffer *vb)
> > 
> >  	return 0;
> >  
> >  }
> > 
> > -static int go7007_buf_finish(struct vb2_buffer *vb)
> > +static void go7007_buf_finish(struct vb2_buffer *vb)
> > 
> >  {
> >  
> >  	struct vb2_queue *vq = vb->vb2_queue;
> >  	struct go7007 *go = vb2_get_drv_priv(vq);
> > 
> > @@ -483,7 +483,6 @@ static int go7007_buf_finish(struct vb2_buffer *vb)
> > 
> >  			V4L2_BUF_FLAG_PFRAME);
> >  	
> >  	buf->flags |= frame_type_flag;
> >  	buf->field = V4L2_FIELD_NONE;
> > 
> > -	return 0;
> > 
> >  }
> >  
> >  static int go7007_start_streaming(struct vb2_queue *q, unsigned int
> >  count)
> > 
> > diff --git a/include/media/videobuf2-core.h
> > b/include/media/videobuf2-core.h index f04eb28..f443ce0 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -311,7 +311,7 @@ struct vb2_ops {
> > 
> >  	int (*buf_init)(struct vb2_buffer *vb);
> >  	int (*buf_prepare)(struct vb2_buffer *vb);
> > 
> > -	int (*buf_finish)(struct vb2_buffer *vb);
> > +	void (*buf_finish)(struct vb2_buffer *vb);
> > 
> >  	void (*buf_cleanup)(struct vb2_buffer *vb);
> >  	
> >  	int (*start_streaming)(struct vb2_queue *q, unsigned int count);

-- 
Regards,

Laurent Pinchart

Re: [REVIEWv2 PATCH 05/15] vb2: change result code of buf_finish to void

[Laurent Pinchart]

And one more separate comment.

On Tuesday 25 February 2014 13:52:45 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The buf_finish op should always work, so change the return type to void.
> Update the few drivers that use it. Note that buf_finish can be called
> both when the DMA is streaming and when it isn't (during queue_cancel).
> Update drivers to check that where appropriate.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Pawel Osciak <pawel@osciak.com>
> Reviewed-by: Pawel Osciak <pawel@osciak.com>
> ---
>  drivers/media/parport/bw-qcam.c                 |  6 ++++--
>  drivers/media/pci/sta2x11/sta2x11_vip.c         |  7 +++----
>  drivers/media/platform/marvell-ccic/mcam-core.c |  3 +--
>  drivers/media/usb/pwc/pwc-if.c                  | 16 +++++++++-------
>  drivers/media/usb/uvc/uvc_queue.c               |  6 +++---
>  drivers/media/v4l2-core/videobuf2-core.c        |  6 +-----
>  drivers/staging/media/go7007/go7007-v4l2.c      |  3 +--
>  include/media/videobuf2-core.h                  |  2 +-
>  8 files changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/parport/bw-qcam.c
> b/drivers/media/parport/bw-qcam.c index d12bd33..8d69bfb 100644
> --- a/drivers/media/parport/bw-qcam.c
> +++ b/drivers/media/parport/bw-qcam.c
> @@ -667,13 +667,16 @@ static void buffer_queue(struct vb2_buffer *vb)
>  	vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
>  }
> 
> -static int buffer_finish(struct vb2_buffer *vb)
> +static void buffer_finish(struct vb2_buffer *vb)
>  {
>  	struct qcam *qcam = vb2_get_drv_priv(vb->vb2_queue);
>  	void *vbuf = vb2_plane_vaddr(vb, 0);
>  	int size = vb->vb2_queue->plane_sizes[0];
>  	int len;
> 
> +	if (!vb2_is_streaming(vb->vb2_queue))
> +		return;
> +
>  	mutex_lock(&qcam->lock);
>  	parport_claim_or_block(qcam->pdev);
> 
> @@ -691,7 +694,6 @@ static int buffer_finish(struct vb2_buffer *vb)
>  	if (len != size)
>  		vb->state = VB2_BUF_STATE_ERROR;
>  	vb2_set_plane_payload(vb, 0, len);
> -	return 0;
>  }
> 
>  static struct vb2_ops qcam_video_qops = {
> diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c
> b/drivers/media/pci/sta2x11/sta2x11_vip.c index e5cfb6c..bb11443 100644
> --- a/drivers/media/pci/sta2x11/sta2x11_vip.c
> +++ b/drivers/media/pci/sta2x11/sta2x11_vip.c
> @@ -327,7 +327,7 @@ static void buffer_queue(struct vb2_buffer *vb)
>  	}
>  	spin_unlock(&vip->lock);
>  }
> -static int buffer_finish(struct vb2_buffer *vb)
> +static void buffer_finish(struct vb2_buffer *vb)
>  {
>  	struct sta2x11_vip *vip = vb2_get_drv_priv(vb->vb2_queue);
>  	struct vip_buffer *vip_buf = to_vip_buffer(vb);
> @@ -337,9 +337,8 @@ static int buffer_finish(struct vb2_buffer *vb)
>  	list_del_init(&vip_buf->list);
>  	spin_unlock(&vip->lock);
> 
> -	vip_active_buf_next(vip);
> -
> -	return 0;
> +	if (vb2_is_streaming(vb->vb2_queue))
> +		vip_active_buf_next(vip);
>  }
> 
>  static int start_streaming(struct vb2_queue *vq, unsigned int count)
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
> b/drivers/media/platform/marvell-ccic/mcam-core.c index 32fab30..8b34c48
> 100644
> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
> @@ -1238,7 +1238,7 @@ static int mcam_vb_sg_buf_prepare(struct vb2_buffer
> *vb) return 0;
>  }
> 
> -static int mcam_vb_sg_buf_finish(struct vb2_buffer *vb)
> +static void mcam_vb_sg_buf_finish(struct vb2_buffer *vb)
>  {
>  	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
>  	struct sg_table *sg_table = vb2_dma_sg_plane_desc(vb, 0);
> @@ -1246,7 +1246,6 @@ static int mcam_vb_sg_buf_finish(struct vb2_buffer
> *vb) if (sg_table)
>  		dma_unmap_sg(cam->dev, sg_table->sgl,
>  				sg_table->nents, DMA_FROM_DEVICE);
> -	return 0;
>  }
> 
>  static void mcam_vb_sg_buf_cleanup(struct vb2_buffer *vb)
> diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
> index abf365a..b9c9f10 100644
> --- a/drivers/media/usb/pwc/pwc-if.c
> +++ b/drivers/media/usb/pwc/pwc-if.c
> @@ -614,17 +614,19 @@ static int buffer_prepare(struct vb2_buffer *vb)
>  	return 0;
>  }
> 
> -static int buffer_finish(struct vb2_buffer *vb)
> +static void buffer_finish(struct vb2_buffer *vb)
>  {
>  	struct pwc_device *pdev = vb2_get_drv_priv(vb->vb2_queue);
>  	struct pwc_frame_buf *buf = container_of(vb, struct pwc_frame_buf, vb);
> 
> -	/*
> -	 * Application has called dqbuf and is getting back a buffer we've
> -	 * filled, take the pwc data we've stored in buf->data and decompress
> -	 * it into a usable format, storing the result in the vb2_buffer
> -	 */
> -	return pwc_decompress(pdev, buf);
> +	if (vb->state == VB2_BUF_STATE_DONE) {
> +		/*
> +		 * Application has called dqbuf and is getting back a buffer we've
> +		 * filled, take the pwc data we've stored in buf->data and decompress
> +		 * it into a usable format, storing the result in the vb2_buffer
> +		 */

You should reflow the text if you want to avoid 80-char checkpatch.pl 
warnings.

> +		pwc_decompress(pdev, buf);
> +	}
>  }
> 
>  static void buffer_cleanup(struct vb2_buffer *vb)
> diff --git a/drivers/media/usb/uvc/uvc_queue.c
> b/drivers/media/usb/uvc/uvc_queue.c index cd962be..db5984e 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -104,15 +104,15 @@ static void uvc_buffer_queue(struct vb2_buffer *vb)
>  	spin_unlock_irqrestore(&queue->irqlock, flags);
>  }
> 
> -static int uvc_buffer_finish(struct vb2_buffer *vb)
> +static void uvc_buffer_finish(struct vb2_buffer *vb)
>  {
>  	struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
>  	struct uvc_streaming *stream =
>  			container_of(queue, struct uvc_streaming, queue);
>  	struct uvc_buffer *buf = container_of(vb, struct uvc_buffer, buf);
> 
> -	uvc_video_clock_update(stream, &vb->v4l2_buf, buf);
> -	return 0;
> +	if (vb2_is_streaming(vb->vb2_queue))
> +		uvc_video_clock_update(stream, &vb->v4l2_buf, buf);
>  }
> 
>  static void uvc_wait_prepare(struct vb2_queue *vq)
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 8f1578b..59bfd85 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1783,11 +1783,7 @@ static int vb2_internal_dqbuf(struct vb2_queue *q,
> struct v4l2_buffer *b, bool n if (ret < 0)
>  		return ret;
> 
> -	ret = call_vb_qop(vb, buf_finish, vb);
> -	if (ret) {
> -		dprintk(1, "dqbuf: buffer finish failed\n");
> -		return ret;
> -	}
> +	call_vb_qop(vb, buf_finish, vb);
> 
>  	switch (vb->state) {
>  	case VB2_BUF_STATE_DONE:
> diff --git a/drivers/staging/media/go7007/go7007-v4l2.c
> b/drivers/staging/media/go7007/go7007-v4l2.c index edc52e2..3a01576 100644
> --- a/drivers/staging/media/go7007/go7007-v4l2.c
> +++ b/drivers/staging/media/go7007/go7007-v4l2.c
> @@ -470,7 +470,7 @@ static int go7007_buf_prepare(struct vb2_buffer *vb)
>  	return 0;
>  }
> 
> -static int go7007_buf_finish(struct vb2_buffer *vb)
> +static void go7007_buf_finish(struct vb2_buffer *vb)
>  {
>  	struct vb2_queue *vq = vb->vb2_queue;
>  	struct go7007 *go = vb2_get_drv_priv(vq);
> @@ -483,7 +483,6 @@ static int go7007_buf_finish(struct vb2_buffer *vb)
>  			V4L2_BUF_FLAG_PFRAME);
>  	buf->flags |= frame_type_flag;
>  	buf->field = V4L2_FIELD_NONE;
> -	return 0;
>  }
> 
>  static int go7007_start_streaming(struct vb2_queue *q, unsigned int count)
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index f04eb28..f443ce0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -311,7 +311,7 @@ struct vb2_ops {
> 
>  	int (*buf_init)(struct vb2_buffer *vb);
>  	int (*buf_prepare)(struct vb2_buffer *vb);
> -	int (*buf_finish)(struct vb2_buffer *vb);
> +	void (*buf_finish)(struct vb2_buffer *vb);
>  	void (*buf_cleanup)(struct vb2_buffer *vb);
> 
>  	int (*start_streaming)(struct vb2_queue *q, unsigned int count);

-- 
Regards,

Laurent Pinchart

Re: [REVIEWv2 PATCH 06/15] vb2: add note that buf_finish can be called with !vb2_is_streaming()

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tuesday 25 February 2014 13:52:46 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Drivers need to be aware that buf_finish can be called when there is no
> streaming going on, so make a note of that.
> 
> Also add a bunch of missing periods at the end of sentences.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Pawel Osciak <pawel@osciak.com>
> ---
>  include/media/videobuf2-core.h | 44 ++++++++++++++++++++-------------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index f443ce0..82b7f0f 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h

[snip]

>   * @buf_finish:		called before every dequeue of the buffer back to
>   *			userspace; drivers may perform any operations required
> - *			before userspace accesses the buffer; optional
> + *			before userspace accesses the buffer; optional. Note:
> + *			this op can be called as well when vb2_is_streaming()
> + *			returns false!

Based on patch 05/15 several drivers assumed that buf_finish is only be called 
when the buffer is to be dequeued to userspace, and performed operations such 
as decompressing the image (yuck...), updating buffer fields such as the 
timestamp, ... If I understand the problem correctly, those operations are 
just a waste of CPU cycles if the buffer will not be returned to userspace, 
hence the driver changes in patch 05/15.

I would document that explicitly here to tell driver developers that 
buf_finish will be called for every buffer that has been queued, and that 
operations related to updating the buffer for userspace can be skipped if the 
queue isn't streaming.

-- 
Regards,

Laurent Pinchart

Re: [REVIEWv2 PATCH 07/15] vb2: call buf_finish from __dqbuf

[Laurent Pinchart]

Hi Hans,

On Tuesday 25 February 2014 13:52:47 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This ensures that it is also called from queue_cancel, which also calls
> __dqbuf(). Without this change any time queue_cancel is called while
> streaming the buf_finish op will not be called and any driver cleanup
> will not happen.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Pawel Osciak <pawel@osciak.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 59bfd85..b5142e5 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1758,6 +1758,8 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
>  	if (vb->state == VB2_BUF_STATE_DEQUEUED)
>  		return;
> 
> +	call_vb_qop(vb, buf_finish, vb);
> +
>  	vb->state = VB2_BUF_STATE_DEQUEUED;
> 
>  	/* unmap DMABUF buffer */
> @@ -1783,8 +1785,6 @@ static int vb2_internal_dqbuf(struct vb2_queue *q,
> struct v4l2_buffer *b, bool n if (ret < 0)
>  		return ret;
> 
> -	call_vb_qop(vb, buf_finish, vb);
> -
>  	switch (vb->state) {
>  	case VB2_BUF_STATE_DONE:
>  		dprintk(3, "dqbuf: Returning done buffer\n");

This will cause an issue with the uvcvideo driver (and possibly others) if I'm 
not mistaken. To give a bit more context, we currently have the following code 
in vb2_internal_dqbuf.

        ret = call_qop(q, buf_finish, vb);
        if (ret) {
                dprintk(1, "dqbuf: buffer finish failed\n");
                return ret;
        }

        switch (vb->state) {
        case VB2_BUF_STATE_DONE:
                dprintk(3, "dqbuf: Returning done buffer\n");
                break;
        case VB2_BUF_STATE_ERROR:
                dprintk(3, "dqbuf: Returning done buffer with errors\n");
                break;
        default:
                dprintk(1, "dqbuf: Invalid buffer state\n");
                return -EINVAL;
        }

        /* Fill buffer information for the userspace */
        __fill_v4l2_buffer(vb, b);
        /* Remove from videobuf queue */
        list_del(&vb->queued_entry);
        /* go back to dequeued state */
        __vb2_dqbuf(vb);

You're thus effectively moving the buf_finish call from before 
__fill_v4l2_buffer() to after it. As the buf_finish operation in uvcvideo 
fills the vb2 timestamp, the timestamp copied to userspace will be wrong.

Other drivers may fill other vb2 fields that need to be copied to userspace as 
well. You should also double check that no driver modifies the vb2 state in 
the buf_finish operation. Expanding the buf_finish documentation to tell what 
drivers are allowed to do could be nice.

-- 
Regards,

Laurent Pinchart

Re: [REVIEWv2 PATCH 09/15] vb2: rename queued_count to owned_by_drv_count

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tuesday 25 February 2014 13:52:49 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> 'queued_count' is a bit vague since it is not clear to which queue it
> refers to: the vb2 internal list of buffers or the driver-owned list
> of buffers.
> 
> Rename to make it explicit.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Acked-by: Pawel Osciak <pawel@osciak.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 10 +++++-----
>  include/media/videobuf2-core.h           |  4 ++--
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index eefcff7..2a7815c 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1071,7 +1071,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum
> vb2_buffer_state state) spin_lock_irqsave(&q->done_lock, flags);
>  	vb->state = state;
>  	list_add_tail(&vb->done_entry, &q->done_list);
> -	atomic_dec(&q->queued_count);
> +	atomic_dec(&q->owned_by_drv_count);
>  	spin_unlock_irqrestore(&q->done_lock, flags);
> 
>  	/* Inform any processes that may be waiting for buffers */
> @@ -1402,7 +1402,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> unsigned int plane;
> 
>  	vb->state = VB2_BUF_STATE_ACTIVE;
> -	atomic_inc(&q->queued_count);
> +	atomic_inc(&q->owned_by_drv_count);
> 
>  	/* sync buffers */
>  	for (plane = 0; plane < vb->num_planes; ++plane)
> @@ -1554,7 +1554,7 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	int ret;
> 
>  	/* Tell the driver to start streaming */
> -	ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
> +	ret = call_qop(q, start_streaming, q,
> atomic_read(&q->owned_by_drv_count)); if (ret)
>  		fail_qop(q, start_streaming);
> 
> @@ -1775,7 +1775,7 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
>  	}
> 
>  	if (!q->retry_start_streaming)
> -		wait_event(q->done_wq, !atomic_read(&q->queued_count));
> +		wait_event(q->done_wq, !atomic_read(&q->owned_by_drv_count));
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
> @@ -1907,7 +1907,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>  	 * has not already dequeued before initiating cancel.
>  	 */
>  	INIT_LIST_HEAD(&q->done_list);
> -	atomic_set(&q->queued_count, 0);
> +	atomic_set(&q->owned_by_drv_count, 0);
>  	wake_up_all(&q->done_wq);
> 
>  	/*
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 82b7f0f..adaffed 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -353,7 +353,7 @@ struct v4l2_fh;
>   * @bufs:	videobuf buffer structures
>   * @num_buffers: number of allocated/used buffers
>   * @queued_list: list of buffers currently queued from userspace
> - * @queued_count: number of buffers owned by the driver
> + * @owned_by_drv_count: number of buffers owned by the driver
>   * @done_list:	list of buffers ready to be dequeued to userspace
>   * @done_lock:	lock to protect done_list list
>   * @done_wq:	waitqueue for processes waiting for buffers ready to be
> dequeued @@ -385,7 +385,7 @@ struct vb2_queue {
> 
>  	struct list_head		queued_list;
> 
> -	atomic_t			queued_count;
> +	atomic_t			owned_by_drv_count;
>  	struct list_head		done_list;
>  	spinlock_t			done_lock;
>  	wait_queue_head_t		done_wq;

-- 
Regards,

Laurent Pinchart

Re: [REVIEWv2 PATCH 10/15] vb2: don't init the list if there are still buffers

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Tuesday 25 February 2014 13:52:50 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> __vb2_queue_free() would init the queued_list at all times, even if
> q->num_buffers > 0. This should only happen if num_buffers == 0.
> 
> This situation can happen if a CREATE_BUFFERS call couldn't allocate
> enough buffers and had to free those it did manage to allocate before
> returning an error.
> 
> While we're at it: __vb2_queue_alloc() returns the number of buffers
> allocated, not an error code. So stick the result in allocated_buffers
> instead of ret as that's very confusing.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 29 +++++++++++++++++------------
> 1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 2a7815c..90374c0 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -452,9 +452,10 @@ static int __vb2_queue_free(struct vb2_queue *q,
> unsigned int buffers) }
> 
>  	q->num_buffers -= buffers;
> -	if (!q->num_buffers)
> +	if (!q->num_buffers) {
>  		q->memory = 0;
> -	INIT_LIST_HEAD(&q->queued_list);
> +		INIT_LIST_HEAD(&q->queued_list);
> +	}
>  	return 0;
>  }
> 
> @@ -820,14 +821,12 @@ static int __reqbufs(struct vb2_queue *q, struct
> v4l2_requestbuffers *req) }
> 
>  	/* Finally, allocate buffers and video memory */
> -	ret = __vb2_queue_alloc(q, req->memory, num_buffers, num_planes);
> -	if (ret == 0) {
> +	allocated_buffers = __vb2_queue_alloc(q, req->memory, num_buffers,
> num_planes); +	if (allocated_buffers == 0) {
>  		dprintk(1, "Memory allocation failed\n");
>  		return -ENOMEM;
>  	}
> 
> -	allocated_buffers = ret;
> -
>  	/*
>  	 * Check if driver can handle the allocated number of buffers.
>  	 */
> @@ -851,6 +850,10 @@ static int __reqbufs(struct vb2_queue *q, struct
> v4l2_requestbuffers *req) q->num_buffers = allocated_buffers;
> 
>  	if (ret < 0) {
> +		/*
> +		 * Note: __vb2_queue_free() will subtract 'allocated_buffers'
> +		 * from q->num_buffers.
> +		 */
>  		__vb2_queue_free(q, allocated_buffers);
>  		return ret;
>  	}
> @@ -924,20 +927,18 @@ static int __create_bufs(struct vb2_queue *q, struct
> v4l2_create_buffers *create }
> 
>  	/* Finally, allocate buffers and video memory */
> -	ret = __vb2_queue_alloc(q, create->memory, num_buffers,
> +	allocated_buffers = __vb2_queue_alloc(q, create->memory, num_buffers,
>  				num_planes);
> -	if (ret == 0) {
> +	if (allocated_buffers == 0) {
>  		dprintk(1, "Memory allocation failed\n");
>  		return -ENOMEM;
>  	}
> 
> -	allocated_buffers = ret;
> -
>  	/*
>  	 * Check if driver can handle the so far allocated number of buffers.
>  	 */
> -	if (ret < num_buffers) {
> -		num_buffers = ret;
> +	if (allocated_buffers < num_buffers) {
> +		num_buffers = allocated_buffers;
> 
>  		/*
>  		 * q->num_buffers contains the total number of buffers, that the
> @@ -960,6 +961,10 @@ static int __create_bufs(struct vb2_queue *q, struct
> v4l2_create_buffers *create q->num_buffers += allocated_buffers;
> 
>  	if (ret < 0) {
> +		/*
> +		 * Note: __vb2_queue_free() will subtract 'allocated_buffers'
> +		 * from q->num_buffers.
> +		 */
>  		__vb2_queue_free(q, allocated_buffers);
>  		return -ENOMEM;
>  	}

-- 
Regards,

Laurent Pinchart

Re: [REVIEWv2 PATCH 07/15] vb2: call buf_finish from __dqbuf

[Hans Verkuil]

On 02/27/14 12:51, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Tuesday 25 February 2014 13:52:47 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> This ensures that it is also called from queue_cancel, which also calls
>> __dqbuf(). Without this change any time queue_cancel is called while
>> streaming the buf_finish op will not be called and any driver cleanup
>> will not happen.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Acked-by: Pawel Osciak <pawel@osciak.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> b/drivers/media/v4l2-core/videobuf2-core.c index 59bfd85..b5142e5 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -1758,6 +1758,8 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
>>  	if (vb->state == VB2_BUF_STATE_DEQUEUED)
>>  		return;
>>
>> +	call_vb_qop(vb, buf_finish, vb);
>> +
>>  	vb->state = VB2_BUF_STATE_DEQUEUED;
>>
>>  	/* unmap DMABUF buffer */
>> @@ -1783,8 +1785,6 @@ static int vb2_internal_dqbuf(struct vb2_queue *q,
>> struct v4l2_buffer *b, bool n if (ret < 0)
>>  		return ret;
>>
>> -	call_vb_qop(vb, buf_finish, vb);
>> -
>>  	switch (vb->state) {
>>  	case VB2_BUF_STATE_DONE:
>>  		dprintk(3, "dqbuf: Returning done buffer\n");
> 
> This will cause an issue with the uvcvideo driver (and possibly others) if I'm 
> not mistaken. To give a bit more context, we currently have the following code 
> in vb2_internal_dqbuf.
> 
>         ret = call_qop(q, buf_finish, vb);
>         if (ret) {
>                 dprintk(1, "dqbuf: buffer finish failed\n");
>                 return ret;
>         }
> 
>         switch (vb->state) {
>         case VB2_BUF_STATE_DONE:
>                 dprintk(3, "dqbuf: Returning done buffer\n");
>                 break;
>         case VB2_BUF_STATE_ERROR:
>                 dprintk(3, "dqbuf: Returning done buffer with errors\n");
>                 break;
>         default:
>                 dprintk(1, "dqbuf: Invalid buffer state\n");
>                 return -EINVAL;
>         }
> 
>         /* Fill buffer information for the userspace */
>         __fill_v4l2_buffer(vb, b);
>         /* Remove from videobuf queue */
>         list_del(&vb->queued_entry);
>         /* go back to dequeued state */
>         __vb2_dqbuf(vb);
> 
> You're thus effectively moving the buf_finish call from before 
> __fill_v4l2_buffer() to after it. As the buf_finish operation in uvcvideo 
> fills the vb2 timestamp, the timestamp copied to userspace will be wrong.

Ouch. Good catch. The solution is to move the __vb2_dqbuf() call to before
the __fill_v4l2_buffer call. But I should see if I can add some test for
this to vivi/v4l2-compliance as well since that should have been caught.

> Other drivers may fill other vb2 fields that need to be copied to userspace as 
> well. You should also double check that no driver modifies the vb2 state in 
> the buf_finish operation. Expanding the buf_finish documentation to tell what 
> drivers are allowed to do could be nice.

Good point.

Regards,

	Hans