[RFCv1 PATCH 0/9] vb2: fixes, balancing callbacks

[RFCv1 PATCH 0/9] vb2: fixes, balancing callbacks

[Hans Verkuil]

Hi all,

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,
but I do need some additional feedback.

Patch 1 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.

Note that I really would like to do the same for stop_streaming. The
stop_streaming error is currently ignored, and there are some drivers
that can actually return an error (waiting for an interruptible mutex
for example). Opinions are welcome.

Patch 3 just improves some comments.

Patches 4 and 5 fix several unbalanced ops.

Patch 6 fixes a regression (must go to 3.14!).

Patch 7 is were things get interesting. There is 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.

This patch adds a reinit_streaming op to tell the driver to reinitialize
its internal buffer lists. This is called both after stop_streaming or
after a failed start_streaming call. In addition when the queue is freed
the vb2 core will call vb2_buffer_done for any buffers still in the
active state. This last action should be done regardless IMHO. I actually
found some drivers that do not call vb2_buffer_done in stop_streaming :-(
thus preventing the vb2 core from calling the finish() memop.

The question is if the reinit_streaming op is a good idea or not.

Alternatives are: requiring drivers to cleanup if start_streaming fails
(hard to check) or a buf_dequeue call that requires the driver to just
delete that buffer from the queue. I tried the last option, but it didn't
really work out: you expect that buf_queue and buf_dequeue would balance
but the only way to do that would be to call buf_dequeue from vb2_buffer_done,
which is not a great idea since that is called from interrupt context.

A queue-level reinit_streaming() op worked better in that regard.

Some drivers already do the cleanup if start_streaming fails, so that
is an option. However, it is hard to enforce in drivers. A reinit_streaming
op would make this explicit.

Patch 8 shows how a reinit_streaming looks in vivi.

The final patch attempts to fix another issue: I noticed that in the few
drivers that implement VIDIOC_CREATE_BUFS the v4l2_format is just used as-is,
i.e. no checks are done whether the contents of the struct makes sense or not.

This patch calls TRY_FMT (and WARNs if no TRY_FMT is defined) to check if the
format is valid/consistent. Again, I'm not certain whether this should be
applied or not.

I have been testing these vb2 changes with vivi (vmalloc based) and an
out-of-tree PCI driver (dma-sg based), with the mmap/userptr and read/write
streaming modes. I hope to test this also for a dma-contig based driver.
I have no way of testing with dmabuf, though. Does anyone know of a simple
way to obtain dmabufs from somewhere so they can be passed to a v4l driver?
It would be great to add a --stream-dmabuf option for v4l2-ctl.

I have to admit that I was a bit disappointed by all these vb2 issues...

Regards,

	Hans

[RFCv1 PATCH 1/9] 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>
---
 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 5a5fb7f..07b58bd 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
@@ -1639,7 +1769,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;
 		}
 }
@@ -1657,7 +1787,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;
@@ -1945,10 +2075,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;
 	}
 
@@ -2040,9 +2171,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.8.5.2

[RFCv1 PATCH 2/9] 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>
---
 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/staging/media/go7007/go7007-v4l2.c      |  3 +--
 include/media/videobuf2-core.h                  |  2 +-
 7 files changed, 22 insertions(+), 21 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/staging/media/go7007/go7007-v4l2.c b/drivers/staging/media/go7007/go7007-v4l2.c
index 50eb69a..1eea4eb 100644
--- a/drivers/staging/media/go7007/go7007-v4l2.c
+++ b/drivers/staging/media/go7007/go7007-v4l2.c
@@ -471,7 +471,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);
@@ -484,7 +484,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.8.5.2

[RFCv1 PATCH 3/9] 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>
---
 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.8.5.2

[RFCv1 PATCH 4/9] 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>
---
 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 07b58bd..3756378 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1762,6 +1762,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 */
@@ -1787,12 +1789,6 @@ 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;
-	}
-
 	switch (vb->state) {
 	case VB2_BUF_STATE_DONE:
 		dprintk(3, "dqbuf: Returning done buffer\n");
-- 
1.8.5.2

[RFCv1 PATCH 5/9] 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 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_clean 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 again followed by buf_prepare. Should buf_prepare fail, then
   buf_cleanup must be called again because all planes will be released
   in case of an error.

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 filehandle, then buffers were ever 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 3756378..7766bf5 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.8.5.2

[RFCv1 PATCH 6/9] 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 call 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 buf_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.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 7766bf5..a3b4b4c 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2418,6 +2418,7 @@ struct vb2_fileio_data {
 	struct v4l2_requestbuffers req;
 	struct v4l2_buffer b;
 	struct vb2_fileio_buf bufs[VIDEO_MAX_FRAME];
+	unsigned int buf_index;
 	unsigned int index;
 	unsigned int q_count;
 	unsigned int dq_count;
@@ -2519,6 +2520,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 			fileio->bufs[i].queued = 1;
 		}
 		fileio->index = q->num_buffers;
+		fileio->buf_index = q->num_buffers;
 	}
 
 	/*
@@ -2597,7 +2599,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->buf_index;
 	if (index >= q->num_buffers) {
 		/*
 		 * Call vb2_dqbuf to get buffer back.
@@ -2611,7 +2613,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->buf_index = index = fileio->b.index;
 		buf = &fileio->bufs[index];
 
 		/*
@@ -2689,6 +2691,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		fileio->q_count += 1;
 		if (fileio->index < q->num_buffers)
 			fileio->index++;
+		fileio->buf_index = fileio->index;
 	}
 
 	/*
-- 
1.8.5.2

[RFCv1 PATCH 7/9] vb2: add reinit_streaming op.

[Hans Verkuil]

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

This new op is called after stop_streaming() or after a failed call to
start_streaming(). The driver needs to dequeue any pending active buffers
it got from the buf_queue() callback.

The reason this op was added is that stop_streaming() traditionally dequeued
any pending active buffers after stopping the DMA engine. However,
stop_streaming() is never called if start_streaming() fails, even though any
prequeued buffers have been passed on to the driver. In that case those
pending active buffers may still be in the driver's active buffer list,
which can cause all sorts of problems if they are not removed.

By splitting stop_streaming into stop_streaming (i.e. stop the DMA engine)
and reinit_streaming (i.e. reinitialize the buffer lists) this problem is
solved. After calling reinit_streaming() the vb2 core will also call
vb2_buffer_done() for any remaining active buffers.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 13 +++++++++++--
 include/media/videobuf2-core.h           |  9 +++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index a3b4b4c..3030ef6 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -395,9 +395,9 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 		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",
+			pr_info("vb2:     setup: %u start_streaming: %u stop_streaming: %u reinit_streaming: %u\n",
 				q->cnt_queue_setup, q->cnt_start_streaming,
-				q->cnt_stop_streaming);
+				q->cnt_stop_streaming, q->cnt_reinit_streaming);
 			pr_info("vb2:     wait_prepare: %u wait_finish: %u\n",
 				q->cnt_wait_prepare, q->cnt_wait_finish);
 		}
@@ -406,6 +406,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 		q->cnt_wait_finish = 0;
 		q->cnt_start_streaming = 0;
 		q->cnt_stop_streaming = 0;
+		q->cnt_reinit_streaming = 0;
 	}
 	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
 		struct vb2_buffer *vb = q->bufs[buffer];
@@ -1900,7 +1901,15 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 	 */
 	if (q->streaming)
 		call_qop(q, stop_streaming, q);
+
 	q->streaming = 0;
+	if (atomic_read(&q->queued_count)) {
+		call_qop(q, reinit_streaming, q);
+
+		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);
+	}
 
 	/*
 	 * Remove all buffers from videobuf's list...
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 82b7f0f..b40dfbc 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -294,8 +294,11 @@ struct vb2_buffer {
  *			buffer is queued.
  * @stop_streaming:	called when 'streaming' state must be disabled; driver
  *			should stop any DMA transactions or wait until they
- *			finish and give back all buffers it got from buf_queue()
- *			callback; may use vb2_wait_for_all_buffers() function
+ *			finish; may use vb2_wait_for_all_buffers() function.
+ * @reinit_streaming:	called after stop_streaming() or after a failed call to
+ *			start_streaming(). The driver needs to dequeue any
+ *			pending active buffers it got from the buf_queue()
+ *			callback.
  * @buf_queue:		passes buffer vb to the driver; driver may start
  *			hardware operation on this buffer; driver should give
  *			the buffer back by calling vb2_buffer_done() function;
@@ -318,6 +321,7 @@ struct vb2_ops {
 
 	int (*start_streaming)(struct vb2_queue *q, unsigned int count);
 	int (*stop_streaming)(struct vb2_queue *q);
+	void (*reinit_streaming)(struct vb2_queue *q);
 
 	void (*buf_queue)(struct vb2_buffer *vb);
 };
@@ -408,6 +412,7 @@ struct vb2_queue {
 	u32				cnt_wait_finish;
 	u32				cnt_start_streaming;
 	u32				cnt_stop_streaming;
+	u32				cnt_reinit_streaming;
 #endif
 };
 
-- 
1.8.5.2

Re: [RFCv1 PATCH 7/9] vb2: add reinit_streaming op.

[Hans Verkuil]

On 01/30/2014 03:51 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This new op is called after stop_streaming() or after a failed call to
> start_streaming(). The driver needs to dequeue any pending active buffers
> it got from the buf_queue() callback.
> 
> The reason this op was added is that stop_streaming() traditionally dequeued
> any pending active buffers after stopping the DMA engine. However,
> stop_streaming() is never called if start_streaming() fails, even though any
> prequeued buffers have been passed on to the driver. In that case those
> pending active buffers may still be in the driver's active buffer list,
> which can cause all sorts of problems if they are not removed.
> 
> By splitting stop_streaming into stop_streaming (i.e. stop the DMA engine)
> and reinit_streaming (i.e. reinitialize the buffer lists) this problem is
> solved. After calling reinit_streaming() the vb2 core will also call
> vb2_buffer_done() for any remaining active buffers.

No need to review patches 7+8: this is going to change. I had a useful discussion
with Pawel regarding this and we came up with a better solution.

Regards,

	Hans

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 13 +++++++++++--
>  include/media/videobuf2-core.h           |  9 +++++++--
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index a3b4b4c..3030ef6 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -395,9 +395,9 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>  		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",
> +			pr_info("vb2:     setup: %u start_streaming: %u stop_streaming: %u reinit_streaming: %u\n",
>  				q->cnt_queue_setup, q->cnt_start_streaming,
> -				q->cnt_stop_streaming);
> +				q->cnt_stop_streaming, q->cnt_reinit_streaming);
>  			pr_info("vb2:     wait_prepare: %u wait_finish: %u\n",
>  				q->cnt_wait_prepare, q->cnt_wait_finish);
>  		}
> @@ -406,6 +406,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>  		q->cnt_wait_finish = 0;
>  		q->cnt_start_streaming = 0;
>  		q->cnt_stop_streaming = 0;
> +		q->cnt_reinit_streaming = 0;
>  	}
>  	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
>  		struct vb2_buffer *vb = q->bufs[buffer];
> @@ -1900,7 +1901,15 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>  	 */
>  	if (q->streaming)
>  		call_qop(q, stop_streaming, q);
> +
>  	q->streaming = 0;
> +	if (atomic_read(&q->queued_count)) {
> +		call_qop(q, reinit_streaming, q);
> +
> +		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);
> +	}
>  
>  	/*
>  	 * Remove all buffers from videobuf's list...
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 82b7f0f..b40dfbc 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -294,8 +294,11 @@ struct vb2_buffer {
>   *			buffer is queued.
>   * @stop_streaming:	called when 'streaming' state must be disabled; driver
>   *			should stop any DMA transactions or wait until they
> - *			finish and give back all buffers it got from buf_queue()
> - *			callback; may use vb2_wait_for_all_buffers() function
> + *			finish; may use vb2_wait_for_all_buffers() function.
> + * @reinit_streaming:	called after stop_streaming() or after a failed call to
> + *			start_streaming(). The driver needs to dequeue any
> + *			pending active buffers it got from the buf_queue()
> + *			callback.
>   * @buf_queue:		passes buffer vb to the driver; driver may start
>   *			hardware operation on this buffer; driver should give
>   *			the buffer back by calling vb2_buffer_done() function;
> @@ -318,6 +321,7 @@ struct vb2_ops {
>  
>  	int (*start_streaming)(struct vb2_queue *q, unsigned int count);
>  	int (*stop_streaming)(struct vb2_queue *q);
> +	void (*reinit_streaming)(struct vb2_queue *q);
>  
>  	void (*buf_queue)(struct vb2_buffer *vb);
>  };
> @@ -408,6 +412,7 @@ struct vb2_queue {
>  	u32				cnt_wait_finish;
>  	u32				cnt_start_streaming;
>  	u32				cnt_stop_streaming;
> +	u32				cnt_reinit_streaming;
>  #endif
>  };
>  
> 

[RFCv1 PATCH 8/9] vivi: add support for reinit_streaming

[Hans Verkuil]

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

This ensures that the driver's buffer list is always initialized, also
if start_streaming returns an error.

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

diff --git a/drivers/media/platform/vivi.c b/drivers/media/platform/vivi.c
index 2d4e73b..c9bf8d3 100644
--- a/drivers/media/platform/vivi.c
+++ b/drivers/media/platform/vivi.c
@@ -793,20 +793,6 @@ static void vivi_stop_generating(struct vivi_dev *dev)
 		kthread_stop(dma_q->kthread);
 		dma_q->kthread = NULL;
 	}
-
-	/*
-	 * Typical driver might need to wait here until dma engine stops.
-	 * In this case we can abort imiedetly, so it's just a noop.
-	 */
-
-	/* Release all active buffers */
-	while (!list_empty(&dma_q->active)) {
-		struct vivi_buffer *buf;
-		buf = list_entry(dma_q->active.next, struct vivi_buffer, list);
-		list_del(&buf->list);
-		vb2_buffer_done(&buf->vb, VB2_BUF_STATE_ERROR);
-		dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
-	}
 }
 /* ------------------------------------------------------------------
 	Videobuf operations
@@ -914,6 +900,13 @@ static int stop_streaming(struct vb2_queue *vq)
 	return 0;
 }
 
+static void reinit_streaming(struct vb2_queue *vq)
+{
+	struct vivi_dev *dev = vb2_get_drv_priv(vq);
+
+	INIT_LIST_HEAD(&dev->vidq.active);
+}
+
 static void vivi_lock(struct vb2_queue *vq)
 {
 	struct vivi_dev *dev = vb2_get_drv_priv(vq);
@@ -933,6 +926,7 @@ static const struct vb2_ops vivi_video_qops = {
 	.buf_queue		= buffer_queue,
 	.start_streaming	= start_streaming,
 	.stop_streaming		= stop_streaming,
+	.reinit_streaming	= reinit_streaming,
 	.wait_prepare		= vivi_unlock,
 	.wait_finish		= vivi_lock,
 };
-- 
1.8.5.2

[RFCv1 PATCH 9/9] v4l2-ioctl: check CREATE_BUFS format via TRY_FMT.

[Hans Verkuil]

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

The format passed to VIDIOC_CREATE_BUFS is completely unchecked at
the moment. So pass it to VIDIOC_TRY_FMT first.

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

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 707aef7..7b9d59e 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1443,9 +1443,15 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
 static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
+	struct video_device *vfd = video_devdata(file);
 	struct v4l2_create_buffers *create = arg;
 	int ret = check_fmt(file, create->format.type);
 
+	if (ret)
+		return ret;
+
+	if (!WARN_ON(!is_valid_ioctl(vfd, VIDIOC_TRY_FMT)))
+		ret = v4l_try_fmt(ops, file, fh, &create->format);
 	return ret ? ret : ops->vidioc_create_bufs(file, fh, create);
 }
 
-- 
1.8.5.2

Re: [RFCv1 PATCH 9/9] v4l2-ioctl: check CREATE_BUFS format via TRY_FMT.

[Hans Verkuil]

On 01/30/2014 03:51 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The format passed to VIDIOC_CREATE_BUFS is completely unchecked at
> the moment. So pass it to VIDIOC_TRY_FMT first.

Don't bother reviewing this. I'm going to change this anyway.

Regards,

	Hans

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 707aef7..7b9d59e 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1443,9 +1443,15 @@ static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
>  static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
>  				struct file *file, void *fh, void *arg)
>  {
> +	struct video_device *vfd = video_devdata(file);
>  	struct v4l2_create_buffers *create = arg;
>  	int ret = check_fmt(file, create->format.type);
>  
> +	if (ret)
> +		return ret;
> +
> +	if (!WARN_ON(!is_valid_ioctl(vfd, VIDIOC_TRY_FMT)))
> +		ret = v4l_try_fmt(ops, file, fh, &create->format);
>  	return ret ? ret : ops->vidioc_create_bufs(file, fh, create);
>  }
>  
>