[RFC PATCH 0/8] vb2: various cleanups and improvements

[RFC PATCH 0/8] vb2: various cleanups and improvements

[Hans Verkuil]

This patch series does some cleanups in the qbuf/prepare_buf handling
(the first two patches). The third patch removes the 'fileio = NULL'
hack. That hack no longer works when dealing with asynchronous calls
from a kernel thread so it had to be fixed.

The next three patches implement retrying start_streaming() if there are
not enough buffers queued for the DMA engine to start. I know that there
are more drivers that can be simplified with this feature available in
the core. Those drivers do the retry of start_streaming in the buf_queue
op which frankly defeats the purpose of having a central start_streaming
op. But I leave it to the driver developers to decide whether or not to
cleanup their drivers.

The big advantage is that apps can just call STREAMON first, then start
queuing buffers without having to know the minimum number of buffers that
have to be queued before the DMA engine will kick in. It always annoyed
me that vb2 didn't take care of that for me as it is easy enough to do.

The final two patches add vb2 thread support which is necessary
for both videobuf2-dvb (the vb2 replacement of videobuf-dvb) and for e.g.
alsa drivers where you use the same trick as with dvb.

The thread implementation has been tested with both alsa recording and
playback for an internal driver (sorry, I can't share the source yet).

Andy, your patch hasn't been added to this series yet, but the patch
I mailed you in reply to your patch will apply cleanly on top of this
series.

Regards,

	Hans

[RFC PATCH 1/8] vb2: push the mmap semaphore down to __buf_prepare()

[Hans Verkuil]

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

Rather than taking the mmap semaphore at a relatively high-level function,
push it down to the place where it is really needed.

It was placed in vb2_queue_or_prepare_buf() to prevent racing with other
vb2 calls, however, I see no way that any race can happen.

Moving it down offers opportunities to simplify the code.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 91412d4..d2b2efb 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1205,6 +1205,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
 static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 {
 	struct vb2_queue *q = vb->vb2_queue;
+	struct rw_semaphore *mmap_sem;
 	int ret;
 
 	ret = __verify_length(vb, b);
@@ -1219,7 +1220,25 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		ret = __qbuf_mmap(vb, b);
 		break;
 	case V4L2_MEMORY_USERPTR:
+		/*
+		 * In case of user pointer buffers vb2 allocators need to get direct
+		 * access to userspace pages. This requires getting the mmap semaphore
+		 * for read access in the current process structure. The same semaphore
+		 * is taken before calling mmap operation, while both qbuf/prepare_buf
+		 * and mmap are called by the driver or v4l2 core with the driver's lock
+		 * held. To avoid an AB-BA deadlock (mmap_sem then driver's lock in mmap
+		 * and driver's lock then mmap_sem in qbuf/prepare_buf) the videobuf2
+		 * core releases the driver's lock, takes mmap_sem and then takes the
+		 * driver's lock again.
+		 */
+		mmap_sem = &current->mm->mmap_sem;
+		call_qop(q, wait_prepare, q);
+		down_read(mmap_sem);
+		call_qop(q, wait_finish, q);
+
 		ret = __qbuf_userptr(vb, b);
+
+		up_read(mmap_sem);
 		break;
 	case V4L2_MEMORY_DMABUF:
 		ret = __qbuf_dmabuf(vb, b);
@@ -1245,80 +1264,47 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
 						   struct v4l2_buffer *,
 						   struct vb2_buffer *))
 {
-	struct rw_semaphore *mmap_sem = NULL;
 	struct vb2_buffer *vb;
 	int ret;
 
-	/*
-	 * In case of user pointer buffers vb2 allocators need to get direct
-	 * access to userspace pages. This requires getting the mmap semaphore
-	 * for read access in the current process structure. The same semaphore
-	 * is taken before calling mmap operation, while both qbuf/prepare_buf
-	 * and mmap are called by the driver or v4l2 core with the driver's lock
-	 * held. To avoid an AB-BA deadlock (mmap_sem then driver's lock in mmap
-	 * and driver's lock then mmap_sem in qbuf/prepare_buf) the videobuf2
-	 * core releases the driver's lock, takes mmap_sem and then takes the
-	 * driver's lock again.
-	 *
-	 * To avoid racing with other vb2 calls, which might be called after
-	 * releasing the driver's lock, this operation is performed at the
-	 * beginning of qbuf/prepare_buf processing. This way the queue status
-	 * is consistent after getting the driver's lock back.
-	 */
-	if (q->memory == V4L2_MEMORY_USERPTR) {
-		mmap_sem = &current->mm->mmap_sem;
-		call_qop(q, wait_prepare, q);
-		down_read(mmap_sem);
-		call_qop(q, wait_finish, q);
-	}
-
 	if (q->fileio) {
 		dprintk(1, "%s(): file io in progress\n", opname);
-		ret = -EBUSY;
-		goto unlock;
+		return -EBUSY;
 	}
 
 	if (b->type != q->type) {
 		dprintk(1, "%s(): invalid buffer type\n", opname);
-		ret = -EINVAL;
-		goto unlock;
+		return -EINVAL;
 	}
 
 	if (b->index >= q->num_buffers) {
 		dprintk(1, "%s(): buffer index out of range\n", opname);
-		ret = -EINVAL;
-		goto unlock;
+		return -EINVAL;
 	}
 
 	vb = q->bufs[b->index];
 	if (NULL == vb) {
 		/* Should never happen */
 		dprintk(1, "%s(): buffer is NULL\n", opname);
-		ret = -EINVAL;
-		goto unlock;
+		return -EINVAL;
 	}
 
 	if (b->memory != q->memory) {
 		dprintk(1, "%s(): invalid memory type\n", opname);
-		ret = -EINVAL;
-		goto unlock;
+		return -EINVAL;
 	}
 
 	ret = __verify_planes_array(vb, b);
 	if (ret)
-		goto unlock;
+		return ret;
 
 	ret = handler(q, b, vb);
-	if (ret)
-		goto unlock;
-
-	/* Fill buffer information for the userspace */
-	__fill_v4l2_buffer(vb, b);
+	if (!ret) {
+		/* Fill buffer information for the userspace */
+		__fill_v4l2_buffer(vb, b);
 
-	dprintk(1, "%s() of buffer %d succeeded\n", opname, vb->v4l2_buf.index);
-unlock:
-	if (mmap_sem)
-		up_read(mmap_sem);
+		dprintk(1, "%s() of buffer %d succeeded\n", opname, vb->v4l2_buf.index);
+	}
 	return ret;
 }
 
-- 
1.8.4.3

[RFC PATCH 2/8] vb2: simplify qbuf/prepare_buf by removing callback.

[Hans Verkuil]

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

The callback used to merge the common code of the qbuf/prepare_buf
code can be removed now that the mmap_sem handling is pushed down to
__buf_prepare(). This makes the code more readable.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index d2b2efb..e90a5be 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1259,14 +1259,8 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 }
 
 static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
-				    const char *opname,
-				    int (*handler)(struct vb2_queue *,
-						   struct v4l2_buffer *,
-						   struct vb2_buffer *))
+				    const char *opname)
 {
-	struct vb2_buffer *vb;
-	int ret;
-
 	if (q->fileio) {
 		dprintk(1, "%s(): file io in progress\n", opname);
 		return -EBUSY;
@@ -1282,8 +1276,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
 		return -EINVAL;
 	}
 
-	vb = q->bufs[b->index];
-	if (NULL == vb) {
+	if (q->bufs[b->index] == NULL) {
 		/* Should never happen */
 		dprintk(1, "%s(): buffer is NULL\n", opname);
 		return -EINVAL;
@@ -1294,30 +1287,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
 		return -EINVAL;
 	}
 
-	ret = __verify_planes_array(vb, b);
-	if (ret)
-		return ret;
-
-	ret = handler(q, b, vb);
-	if (!ret) {
-		/* Fill buffer information for the userspace */
-		__fill_v4l2_buffer(vb, b);
-
-		dprintk(1, "%s() of buffer %d succeeded\n", opname, vb->v4l2_buf.index);
-	}
-	return ret;
-}
-
-static int __vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
-			     struct vb2_buffer *vb)
-{
-	if (vb->state != VB2_BUF_STATE_DEQUEUED) {
-		dprintk(1, "%s(): invalid buffer state %d\n", __func__,
-			vb->state);
-		return -EINVAL;
-	}
-
-	return __buf_prepare(vb, b);
+	return __verify_planes_array(q->bufs[b->index], b);
 }
 
 /**
@@ -1337,20 +1307,68 @@ static int __vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
  */
 int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
 {
-	return vb2_queue_or_prepare_buf(q, b, "prepare_buf", __vb2_prepare_buf);
+	int ret = vb2_queue_or_prepare_buf(q, b, "prepare_buf");
+	struct vb2_buffer *vb;
+
+	if (ret)
+		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;
+	}
+
+	ret = __buf_prepare(vb, b);
+	if (!ret) {
+		/* Fill buffer information for the userspace */
+		__fill_v4l2_buffer(vb, b);
+
+		dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb->v4l2_buf.index);
+	}
+	return ret;
 }
 EXPORT_SYMBOL_GPL(vb2_prepare_buf);
 
-static int __vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b,
-		      struct vb2_buffer *vb)
+/**
+ * vb2_qbuf() - Queue a buffer from userspace
+ * @q:		videobuf2 queue
+ * @b:		buffer structure passed from userspace to vidioc_qbuf handler
+ *		in driver
+ *
+ * Should be called from vidioc_qbuf ioctl handler of a driver.
+ * This function:
+ * 1) verifies the passed buffer,
+ * 2) if necessary, calls buf_prepare callback in the driver (if provided), in
+ *    which driver-specific buffer initialization can be performed,
+ * 3) if streaming is on, queues the buffer in driver by the means of buf_queue
+ *    callback for processing.
+ *
+ * The return values from this function are intended to be directly returned
+ * from vidioc_qbuf handler in driver.
+ */
+int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 {
-	int ret;
+	int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
+	struct vb2_buffer *vb;
+
+	if (ret)
+		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:
 		ret = __buf_prepare(vb, b);
 		if (ret)
 			return ret;
+		break;
 	case VB2_BUF_STATE_PREPARED:
 		break;
 	default:
@@ -1372,29 +1390,11 @@ static int __vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b,
 	if (q->streaming)
 		__enqueue_in_driver(vb);
 
-	return 0;
-}
+	/* Fill buffer information for the userspace */
+	__fill_v4l2_buffer(vb, b);
 
-/**
- * vb2_qbuf() - Queue a buffer from userspace
- * @q:		videobuf2 queue
- * @b:		buffer structure passed from userspace to vidioc_qbuf handler
- *		in driver
- *
- * Should be called from vidioc_qbuf ioctl handler of a driver.
- * This function:
- * 1) verifies the passed buffer,
- * 2) if necessary, calls buf_prepare callback in the driver (if provided), in
- *    which driver-specific buffer initialization can be performed,
- * 3) if streaming is on, queues the buffer in driver by the means of buf_queue
- *    callback for processing.
- *
- * The return values from this function are intended to be directly returned
- * from vidioc_qbuf handler in driver.
- */
-int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
-{
-	return vb2_queue_or_prepare_buf(q, b, "qbuf", __vb2_qbuf);
+	dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb->v4l2_buf.index);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(vb2_qbuf);
 
-- 
1.8.4.3

[RFC PATCH 3/8] vb2: remove the 'fileio = NULL' hack.

[Hans Verkuil]

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

The read/write implementation in vb2 reuses existing vb2 functions, but
it sets q->fileio to NULL before calling them in order to skip the
'q->fileio != NULL' check.

This works today due to the synchronous nature of read/write, but it
1) is ugly, and 2) will fail in an asynchronous use-case such as a
thread queuing and dequeuing buffers. This last example will be necessary
in order to implement vb2 DVB support.

This patch removes the hack by splitting up the dqbuf/qbuf/streamon/streamoff
functions into an external and an internal version. The external version
checks q->fileio and then calls the internal version. The read/write
implementation now just uses the internal version, removing the need to
set q->fileio to NULL.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index e90a5be..9ea3ae9 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1261,11 +1261,6 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
 				    const char *opname)
 {
-	if (q->fileio) {
-		dprintk(1, "%s(): file io in progress\n", opname);
-		return -EBUSY;
-	}
-
 	if (b->type != q->type) {
 		dprintk(1, "%s(): invalid buffer type\n", opname);
 		return -EINVAL;
@@ -1307,9 +1302,15 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
  */
 int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
 {
-	int ret = vb2_queue_or_prepare_buf(q, b, "prepare_buf");
 	struct vb2_buffer *vb;
+	int ret;
+
+	if (q->fileio) {
+		dprintk(1, "%s(): file io in progress\n", __func__);
+		return -EBUSY;
+	}
 
+	ret = vb2_queue_or_prepare_buf(q, b, "prepare_buf");
 	if (ret)
 		return ret;
 
@@ -1331,24 +1332,7 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
 }
 EXPORT_SYMBOL_GPL(vb2_prepare_buf);
 
-/**
- * vb2_qbuf() - Queue a buffer from userspace
- * @q:		videobuf2 queue
- * @b:		buffer structure passed from userspace to vidioc_qbuf handler
- *		in driver
- *
- * Should be called from vidioc_qbuf ioctl handler of a driver.
- * This function:
- * 1) verifies the passed buffer,
- * 2) if necessary, calls buf_prepare callback in the driver (if provided), in
- *    which driver-specific buffer initialization can be performed,
- * 3) if streaming is on, queues the buffer in driver by the means of buf_queue
- *    callback for processing.
- *
- * The return values from this function are intended to be directly returned
- * from vidioc_qbuf handler in driver.
- */
-int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
+static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 {
 	int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
 	struct vb2_buffer *vb;
@@ -1396,6 +1380,33 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 	dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb->v4l2_buf.index);
 	return 0;
 }
+
+/**
+ * vb2_qbuf() - Queue a buffer from userspace
+ * @q:		videobuf2 queue
+ * @b:		buffer structure passed from userspace to vidioc_qbuf handler
+ *		in driver
+ *
+ * Should be called from vidioc_qbuf ioctl handler of a driver.
+ * This function:
+ * 1) verifies the passed buffer,
+ * 2) if necessary, calls buf_prepare callback in the driver (if provided), in
+ *    which driver-specific buffer initialization can be performed,
+ * 3) if streaming is on, queues the buffer in driver by the means of buf_queue
+ *    callback for processing.
+ *
+ * The return values from this function are intended to be directly returned
+ * from vidioc_qbuf handler in driver.
+ */
+int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
+{
+	if (q->fileio) {
+		dprintk(1, "%s(): file io in progress\n", __func__);
+		return -EBUSY;
+	}
+
+	return vb2_internal_qbuf(q, b);
+}
 EXPORT_SYMBOL_GPL(vb2_qbuf);
 
 /**
@@ -1544,37 +1555,11 @@ static void __vb2_dqbuf(struct vb2_buffer *vb)
 		}
 }
 
-/**
- * vb2_dqbuf() - Dequeue a buffer to the userspace
- * @q:		videobuf2 queue
- * @b:		buffer structure passed from userspace to vidioc_dqbuf handler
- *		in driver
- * @nonblocking: if true, this call will not sleep waiting for a buffer if no
- *		 buffers ready for dequeuing are present. Normally the driver
- *		 would be passing (file->f_flags & O_NONBLOCK) here
- *
- * Should be called from vidioc_dqbuf ioctl handler of a driver.
- * This function:
- * 1) verifies the passed buffer,
- * 2) calls buf_finish callback in the driver (if provided), in which
- *    driver can perform any additional operations that may be required before
- *    returning the buffer to userspace, such as cache sync,
- * 3) the buffer struct members are filled with relevant information for
- *    the userspace.
- *
- * The return values from this function are intended to be directly returned
- * from vidioc_dqbuf handler in driver.
- */
-int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
+static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
 {
 	struct vb2_buffer *vb = NULL;
 	int ret;
 
-	if (q->fileio) {
-		dprintk(1, "dqbuf: file io in progress\n");
-		return -EBUSY;
-	}
-
 	if (b->type != q->type) {
 		dprintk(1, "dqbuf: invalid buffer type\n");
 		return -EINVAL;
@@ -1613,6 +1598,36 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
 
 	return 0;
 }
+
+/**
+ * vb2_dqbuf() - Dequeue a buffer to the userspace
+ * @q:		videobuf2 queue
+ * @b:		buffer structure passed from userspace to vidioc_dqbuf handler
+ *		in driver
+ * @nonblocking: if true, this call will not sleep waiting for a buffer if no
+ *		 buffers ready for dequeuing are present. Normally the driver
+ *		 would be passing (file->f_flags & O_NONBLOCK) here
+ *
+ * Should be called from vidioc_dqbuf ioctl handler of a driver.
+ * This function:
+ * 1) verifies the passed buffer,
+ * 2) calls buf_finish callback in the driver (if provided), in which
+ *    driver can perform any additional operations that may be required before
+ *    returning the buffer to userspace, such as cache sync,
+ * 3) the buffer struct members are filled with relevant information for
+ *    the userspace.
+ *
+ * The return values from this function are intended to be directly returned
+ * from vidioc_dqbuf handler in driver.
+ */
+int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
+{
+	if (q->fileio) {
+		dprintk(1, "dqbuf: file io in progress\n");
+		return -EBUSY;
+	}
+	return vb2_internal_dqbuf(q, b, nonblocking);
+}
 EXPORT_SYMBOL_GPL(vb2_dqbuf);
 
 /**
@@ -1652,29 +1667,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 		__vb2_dqbuf(q->bufs[i]);
 }
 
-/**
- * vb2_streamon - start streaming
- * @q:		videobuf2 queue
- * @type:	type argument passed from userspace to vidioc_streamon handler
- *
- * Should be called from vidioc_streamon handler of a driver.
- * This function:
- * 1) verifies current state
- * 2) passes any previously queued buffers to the driver and starts streaming
- *
- * The return values from this function are intended to be directly returned
- * from vidioc_streamon handler in the driver.
- */
-int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
+static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 {
 	struct vb2_buffer *vb;
 	int ret;
 
-	if (q->fileio) {
-		dprintk(1, "streamon: file io in progress\n");
-		return -EBUSY;
-	}
-
 	if (type != q->type) {
 		dprintk(1, "streamon: invalid stream type\n");
 		return -EINVAL;
@@ -1707,31 +1704,32 @@ int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 	dprintk(3, "Streamon successful\n");
 	return 0;
 }
-EXPORT_SYMBOL_GPL(vb2_streamon);
-
 
 /**
- * vb2_streamoff - stop streaming
+ * vb2_streamon - start streaming
  * @q:		videobuf2 queue
- * @type:	type argument passed from userspace to vidioc_streamoff handler
+ * @type:	type argument passed from userspace to vidioc_streamon handler
  *
- * Should be called from vidioc_streamoff handler of a driver.
+ * Should be called from vidioc_streamon handler of a driver.
  * This function:
- * 1) verifies current state,
- * 2) stop streaming and dequeues any queued buffers, including those previously
- *    passed to the driver (after waiting for the driver to finish).
+ * 1) verifies current state
+ * 2) passes any previously queued buffers to the driver and starts streaming
  *
- * This call can be used for pausing playback.
  * The return values from this function are intended to be directly returned
- * from vidioc_streamoff handler in the driver
+ * from vidioc_streamon handler in the driver.
  */
-int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
+int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 {
 	if (q->fileio) {
-		dprintk(1, "streamoff: file io in progress\n");
+		dprintk(1, "streamon: file io in progress\n");
 		return -EBUSY;
 	}
+	return vb2_internal_streamon(q, type);
+}
+EXPORT_SYMBOL_GPL(vb2_streamon);
 
+static int vb2_internal_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
+{
 	if (type != q->type) {
 		dprintk(1, "streamoff: invalid stream type\n");
 		return -EINVAL;
@@ -1751,6 +1749,30 @@ int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
 	dprintk(3, "Streamoff successful\n");
 	return 0;
 }
+
+/**
+ * vb2_streamoff - stop streaming
+ * @q:		videobuf2 queue
+ * @type:	type argument passed from userspace to vidioc_streamoff handler
+ *
+ * Should be called from vidioc_streamoff handler of a driver.
+ * This function:
+ * 1) verifies current state,
+ * 2) stop streaming and dequeues any queued buffers, including those previously
+ *    passed to the driver (after waiting for the driver to finish).
+ *
+ * This call can be used for pausing playback.
+ * The return values from this function are intended to be directly returned
+ * from vidioc_streamoff handler in the driver
+ */
+int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type)
+{
+	if (q->fileio) {
+		dprintk(1, "streamoff: file io in progress\n");
+		return -EBUSY;
+	}
+	return vb2_internal_streamoff(q, type);
+}
 EXPORT_SYMBOL_GPL(vb2_streamoff);
 
 /**
@@ -2273,13 +2295,8 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q)
 	struct vb2_fileio_data *fileio = q->fileio;
 
 	if (fileio) {
-		/*
-		 * Hack fileio context to enable direct calls to vb2 ioctl
-		 * interface.
-		 */
+		vb2_internal_streamoff(q, q->type);
 		q->fileio = NULL;
-
-		vb2_streamoff(q, q->type);
 		fileio->req.count = 0;
 		vb2_reqbufs(q, &fileio->req);
 		kfree(fileio);
@@ -2322,12 +2339,6 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 	}
 	fileio = q->fileio;
 
-	/*
-	 * Hack fileio context to enable direct calls to vb2 ioctl interface.
-	 * The pointer will be restored before returning from this function.
-	 */
-	q->fileio = NULL;
-
 	index = fileio->index;
 	buf = &fileio->bufs[index];
 
@@ -2344,10 +2355,10 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		fileio->b.type = q->type;
 		fileio->b.memory = q->memory;
 		fileio->b.index = index;
-		ret = vb2_dqbuf(q, &fileio->b, nonblock);
+		ret = vb2_internal_dqbuf(q, &fileio->b, nonblock);
 		dprintk(5, "file io: vb2_dqbuf result: %d\n", ret);
 		if (ret)
-			goto end;
+			return ret;
 		fileio->dq_count += 1;
 
 		/*
@@ -2377,8 +2388,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		ret = copy_from_user(buf->vaddr + buf->pos, data, count);
 	if (ret) {
 		dprintk(3, "file io: error copying data\n");
-		ret = -EFAULT;
-		goto end;
+		return -EFAULT;
 	}
 
 	/*
@@ -2398,10 +2408,6 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		if (read && (fileio->flags & VB2_FILEIO_READ_ONCE) &&
 		    fileio->dq_count == 1) {
 			dprintk(3, "file io: read limit reached\n");
-			/*
-			 * Restore fileio pointer and release the context.
-			 */
-			q->fileio = fileio;
 			return __vb2_cleanup_fileio(q);
 		}
 
@@ -2413,10 +2419,10 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		fileio->b.memory = q->memory;
 		fileio->b.index = index;
 		fileio->b.bytesused = buf->pos;
-		ret = vb2_qbuf(q, &fileio->b);
+		ret = vb2_internal_qbuf(q, &fileio->b);
 		dprintk(5, "file io: vb2_dbuf result: %d\n", ret);
 		if (ret)
-			goto end;
+			return ret;
 
 		/*
 		 * Buffer has been queued, update the status
@@ -2435,9 +2441,9 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		 * Start streaming if required.
 		 */
 		if (!read && !q->streaming) {
-			ret = vb2_streamon(q, q->type);
+			ret = vb2_internal_streamon(q, q->type);
 			if (ret)
-				goto end;
+				return ret;
 		}
 	}
 
@@ -2446,11 +2452,6 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 	 */
 	if (ret == 0)
 		ret = count;
-end:
-	/*
-	 * Restore the fileio context and block vb2 ioctl interface.
-	 */
-	q->fileio = fileio;
 	return ret;
 }
 
-- 
1.8.4.3

[RFC PATCH 4/8] vb2: retry start_streaming in case of insufficient buffers.

[Hans Verkuil]

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

If start_streaming returns -ENODATA, then it will be retried the next time
a buffer is queued. This means applications no longer need to know how many
buffers need to be queued before STREAMON can be called. This is particularly
useful for output stream I/O.

If a DMA engine needs at least X buffers before it can start streaming, then
for applications to get a buffer out as soon as possible they need to know
the minimum number of buffers to queue before STREAMON can be called. You can't
just try STREAMON after every buffer since on failure STREAMON will dequeue
all your buffers. (Is that a bug or a feature? Frankly, I'm not sure).

This patch simplifies applications substantially: they can just call STREAMON
at the beginning and then start queuing buffers and the DMA engine will
kick in automagically once enough buffers are available.

This also fixes using write() to stream video: the fileio implementation
calls streamon without having any queued buffers, which will fail today for
any driver that requires a minimum number of buffers.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 9ea3ae9..5bb91f7 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1332,6 +1332,39 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
 }
 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
+ * true and 0 is returned. The next time a buffer is queued and
+ * retry_start_streaming is true, this function will be called again to
+ * retry starting the DMA engine.
+ */
+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));
+
+	/*
+	 * If there are not enough buffers queued to start streaming, then
+	 * the start_streaming operation will return -ENODATA and you have to
+	 * retry when the next buffer is queued.
+	 */
+	if (ret == -ENODATA) {
+		dprintk(1, "qbuf: not enough buffers, retry when more buffers are queued.\n");
+		q->retry_start_streaming = true;
+		return 0;
+	}
+	if (ret)
+		dprintk(1, "qbuf: driver refused to start streaming\n");
+	else
+		q->retry_start_streaming = false;
+	return ret;
+}
+
 static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 {
 	int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
@@ -1377,6 +1410,12 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 	/* Fill buffer information for the userspace */
 	__fill_v4l2_buffer(vb, b);
 
+	if (q->retry_start_streaming) {
+		ret = vb2_start_streaming(q);
+		if (ret)
+			return ret;
+	}
+
 	dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb->v4l2_buf.index);
 	return 0;
 }
@@ -1526,7 +1565,8 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
 		return -EINVAL;
 	}
 
-	wait_event(q->done_wq, !atomic_read(&q->queued_count));
+	if (!q->retry_start_streaming)
+		wait_event(q->done_wq, !atomic_read(&q->queued_count));
 	return 0;
 }
 EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
@@ -1640,6 +1680,9 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 {
 	unsigned int i;
 
+	if (q->retry_start_streaming)
+		q->retry_start_streaming = q->streaming = 0;
+
 	/*
 	 * Tell driver to stop all transactions and release all queued
 	 * buffers.
@@ -1689,12 +1732,9 @@ static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
 	list_for_each_entry(vb, &q->queued_list, queued_entry)
 		__enqueue_in_driver(vb);
 
-	/*
-	 * Let driver notice that streaming state has been enabled.
-	 */
-	ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
+	/* Tell driver to start streaming. */
+	ret = vb2_start_streaming(q);
 	if (ret) {
-		dprintk(1, "streamon: driver refused to start streaming\n");
 		__vb2_queue_cancel(q);
 		return ret;
 	}
@@ -2264,15 +2304,15 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 				goto err_reqbufs;
 			fileio->bufs[i].queued = 1;
 		}
-
-		/*
-		 * Start streaming.
-		 */
-		ret = vb2_streamon(q, q->type);
-		if (ret)
-			goto err_reqbufs;
 	}
 
+	/*
+	 * Start streaming.
+	 */
+	ret = vb2_streamon(q, q->type);
+	if (ret)
+		goto err_reqbufs;
+
 	q->fileio = fileio;
 
 	return ret;
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index bd8218b..2d88897 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -250,10 +250,13 @@ struct vb2_buffer {
  *			receive buffers with @buf_queue callback before
  *			@start_streaming is called; the driver gets the number
  *			of already queued buffers in count parameter; driver
- *			can return an error if hardware fails or not enough
- *			buffers has been queued, in such case all buffers that
- *			have been already given by the @buf_queue callback are
- *			invalidated.
+ *			can return an error if hardware fails, in that case all
+ *			buffers that have been already given by the @buf_queue
+ *			callback are invalidated.
+ *			If there were not enough queued buffers to start
+ *			streaming, then this callback returns -ENODATA, and the
+ *			vb2 core will retry calling @start_streaming when a new
+ *			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()
@@ -321,6 +324,9 @@ struct v4l2_fh;
  * @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.
  * @fileio:	file io emulator internal data, used only if emulator is active
  */
 struct vb2_queue {
@@ -353,6 +359,7 @@ struct vb2_queue {
 	unsigned int			plane_sizes[VIDEO_MAX_PLANES];
 
 	unsigned int			streaming:1;
+	unsigned int			retry_start_streaming:1;
 
 	struct vb2_fileio_data		*fileio;
 };
-- 
1.8.4.3

[RFC PATCH 5/8] vb2: don't set index, don't start streaming for write()

[Hans Verkuil]

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

Two fixes:

- there is no need to set the index when calling dqbuf: dqbuf will
  overwrite it.
- __vb2_init_fileio already starts streaming for write(), so there is
  no need to do it again in __vb2_perform_fileio. It can never have
  worked anyway: either __vb2_init_fileio succeeds in starting streaming
  or it is never going to happen.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 5bb91f7..7d1498f 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2394,7 +2394,6 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		memset(&fileio->b, 0, sizeof(fileio->b));
 		fileio->b.type = q->type;
 		fileio->b.memory = q->memory;
-		fileio->b.index = index;
 		ret = vb2_internal_dqbuf(q, &fileio->b, nonblock);
 		dprintk(5, "file io: vb2_dqbuf result: %d\n", ret);
 		if (ret)
@@ -2476,15 +2475,6 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		 * Switch to the next buffer
 		 */
 		fileio->index = (index + 1) % q->num_buffers;
-
-		/*
-		 * Start streaming if required.
-		 */
-		if (!read && !q->streaming) {
-			ret = vb2_internal_streamon(q, q->type);
-			if (ret)
-				return ret;
-		}
 	}
 
 	/*
-- 
1.8.4.3

[RFC PATCH 6/8] vb2: return ENODATA in start_streaming in case of too few buffers.

[Hans Verkuil]

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

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/davinci/vpbe_display.c   | 2 +-
 drivers/media/platform/davinci/vpif_capture.c   | 2 +-
 drivers/media/platform/davinci/vpif_display.c   | 2 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    | 2 +-
 drivers/media/platform/s5p-tv/mixer_video.c     | 2 +-
 drivers/media/platform/soc_camera/mx2_camera.c  | 2 +-
 drivers/staging/media/davinci_vpfe/vpfe_video.c | 2 ++
 7 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/davinci/vpbe_display.c b/drivers/media/platform/davinci/vpbe_display.c
index eac472b..53be7fc 100644
--- a/drivers/media/platform/davinci/vpbe_display.c
+++ b/drivers/media/platform/davinci/vpbe_display.c
@@ -347,7 +347,7 @@ static int vpbe_start_streaming(struct vb2_queue *vq, unsigned int count)
 	/* 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 -EINVAL;
+		return -ENODATA;
 	}
 	/* Get the next frame from the buffer queue */
 	layer->next_frm = layer->cur_frm = list_entry(layer->dma_queue.next,
diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index 52ac5e6..4b04a27 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -277,7 +277,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
 	if (list_empty(&common->dma_queue)) {
 		spin_unlock_irqrestore(&common->irqlock, flags);
 		vpif_dbg(1, debug, "buffer queue is empty\n");
-		return -EIO;
+		return -ENODATA;
 	}
 
 	/* Get the next frame from the buffer queue */
diff --git a/drivers/media/platform/davinci/vpif_display.c b/drivers/media/platform/davinci/vpif_display.c
index c31bcf1..c5070dc 100644
--- a/drivers/media/platform/davinci/vpif_display.c
+++ b/drivers/media/platform/davinci/vpif_display.c
@@ -239,7 +239,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, unsigned int count)
 	if (list_empty(&common->dma_queue)) {
 		spin_unlock_irqrestore(&common->irqlock, flags);
 		vpif_err("buffer queue is empty\n");
-		return -EIO;
+		return -ENODATA;
 	}
 
 	/* Get the next frame from the buffer queue */
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 4ff3b6c..3bdfe85 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1863,7 +1863,7 @@ static int s5p_mfc_start_streaming(struct vb2_queue *q, unsigned int count)
 		if (ctx->src_bufs_cnt < ctx->pb_count) {
 			mfc_err("Need minimum %d OUTPUT buffers\n",
 					ctx->pb_count);
-			return -EINVAL;
+			return -ENODATA;
 		}
 	}
 
diff --git a/drivers/media/platform/s5p-tv/mixer_video.c b/drivers/media/platform/s5p-tv/mixer_video.c
index 641b1f0..7d2fe64 100644
--- a/drivers/media/platform/s5p-tv/mixer_video.c
+++ b/drivers/media/platform/s5p-tv/mixer_video.c
@@ -948,7 +948,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	if (count == 0) {
 		mxr_dbg(mdev, "no output buffers queued\n");
-		return -EINVAL;
+		return -ENODATA;
 	}
 
 	/* block any changes in output configuration */
diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c
index 45a0276..587e3d1 100644
--- a/drivers/media/platform/soc_camera/mx2_camera.c
+++ b/drivers/media/platform/soc_camera/mx2_camera.c
@@ -659,7 +659,7 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
 	unsigned long flags;
 
 	if (count < 2)
-		return -EINVAL;
+		return -ENODATA;
 
 	spin_lock_irqsave(&pcdev->lock, flags);
 
diff --git a/drivers/staging/media/davinci_vpfe/vpfe_video.c b/drivers/staging/media/davinci_vpfe/vpfe_video.c
index 24d98a6..a81b0ab 100644
--- a/drivers/staging/media/davinci_vpfe/vpfe_video.c
+++ b/drivers/staging/media/davinci_vpfe/vpfe_video.c
@@ -1201,6 +1201,8 @@ static int vpfe_start_streaming(struct vb2_queue *vq, unsigned int count)
 	unsigned long addr;
 	int ret;
 
+	if (count == 0)
+		return -ENODATA;
 	ret = mutex_lock_interruptible(&video->lock);
 	if (ret)
 		goto streamoff;
-- 
1.8.4.3

[RFC PATCH 7/8] vb2: add thread support

[Hans Verkuil]

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

In order to implement vb2 DVB or ALSA support you need to be able to start
a kernel thread that queues and dequeues buffers, calling a callback
function for every captured/displayed buffer. This patch adds support for
that.

It's based on drivers/media/v4l2-core/videobuf-dvb.c, but with all the DVB
specific stuff stripped out, thus making it much more generic.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 7d1498f..66cd81d 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -6,6 +6,9 @@
  * Author: Pawel Osciak <pawel@osciak.com>
  *	   Marek Szyprowski <m.szyprowski@samsung.com>
  *
+ * The vb2_thread implementation was based on code from videobuf-dvb.c:
+ * 	(c) 2004 Gerd Knorr <kraxel@bytesex.org> [SUSE Labs]
+ *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation.
@@ -18,6 +21,8 @@
 #include <linux/poll.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
+#include <linux/freezer.h>
+#include <linux/kthread.h>
 
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
@@ -2500,6 +2505,135 @@ size_t vb2_write(struct vb2_queue *q, const char __user *data, size_t count,
 }
 EXPORT_SYMBOL_GPL(vb2_write);
 
+struct vb2_threadio_data {
+	struct task_struct *thread;
+	vb2_thread_fnc fnc;
+	void *priv;
+	bool stop;
+};
+
+static int vb2_thread(void *data)
+{
+	struct vb2_queue *q = data;
+	struct vb2_threadio_data *threadio = q->threadio;
+	struct vb2_fileio_data *fileio = q->fileio;
+	int prequeue = 0;
+	int index = 0;
+	int ret = 0;
+
+	if (V4L2_TYPE_IS_OUTPUT(q->type))
+		prequeue = q->num_buffers;
+
+	set_freezable();
+
+	for (;;) {
+		struct vb2_buffer *vb;
+
+		/*
+		 * Call vb2_dqbuf to get buffer back.
+		 */
+		memset(&fileio->b, 0, sizeof(fileio->b));
+		fileio->b.type = q->type;
+		fileio->b.memory = q->memory;
+		if (prequeue) {
+			fileio->b.index = index++;
+			prequeue--;
+		} else {
+			call_qop(q, wait_finish, q);
+			ret = vb2_internal_dqbuf(q, &fileio->b, 0);
+			call_qop(q, wait_prepare, q);
+			dprintk(5, "file io: vb2_dqbuf result: %d\n", ret);
+		}
+		if (threadio->stop)
+			break;
+		if (ret)
+			break;
+		try_to_freeze();
+
+		vb = q->bufs[fileio->b.index];
+		if (!(fileio->b.flags & V4L2_BUF_FLAG_ERROR))
+			ret = threadio->fnc(vb, threadio->priv);
+		if (ret)
+			break;
+		call_qop(q, wait_finish, q);
+		ret = vb2_internal_qbuf(q, &fileio->b);
+		call_qop(q, wait_prepare, q);
+		if (ret)
+			break;
+	}
+
+	/* Hmm, linux becomes *very* unhappy without this ... */
+	while (!kthread_should_stop()) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule();
+	}
+	return 0;
+}
+
+int vb2_thread_start(struct vb2_queue *q, vb2_thread_fnc fnc, void *priv,
+		     const char *thread_name)
+{
+	struct vb2_threadio_data *threadio;
+	int ret = 0;
+
+	if (q->threadio)
+		return -EBUSY;
+	if (vb2_is_busy(q))
+		return -EBUSY;
+	if (WARN_ON(q->fileio))
+		return -EBUSY;
+
+	threadio = kzalloc(sizeof(*threadio), GFP_KERNEL);
+	if (threadio == NULL)
+		return -ENOMEM;
+	threadio->fnc = fnc;
+	threadio->priv = priv;
+
+	ret = __vb2_init_fileio(q, !V4L2_TYPE_IS_OUTPUT(q->type));
+	dprintk(3, "file io: vb2_init_fileio result: %d\n", ret);
+	if (ret)
+		goto nomem;
+	q->threadio = threadio;
+	threadio->thread = kthread_run(vb2_thread, q, "vb2-%s", thread_name);
+	if (IS_ERR(threadio->thread)) {
+		ret = PTR_ERR(threadio->thread);
+		threadio->thread = NULL;
+		goto nothread;
+	}
+	return 0;
+
+nothread:
+	__vb2_cleanup_fileio(q);
+nomem:
+	kfree(threadio);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(vb2_thread_start);
+
+int vb2_thread_stop(struct vb2_queue *q)
+{
+	struct vb2_threadio_data *threadio = q->threadio;
+	struct vb2_fileio_data *fileio = q->fileio;
+	int err;
+
+	if (threadio == NULL)
+		return 0;
+	call_qop(q, wait_finish, q);
+	threadio->stop = true;
+	vb2_internal_streamoff(q, q->type);
+	call_qop(q, wait_prepare, q);
+	q->fileio = NULL;
+	fileio->req.count = 0;
+	vb2_reqbufs(q, &fileio->req);
+	kfree(fileio);
+	err = kthread_stop(threadio->thread);
+	threadio->thread = NULL;
+	kfree(threadio);
+	q->fileio = NULL;
+	q->threadio = NULL;
+	return err;
+}
+EXPORT_SYMBOL_GPL(vb2_thread_stop);
 
 /*
  * The following functions are not part of the vb2 core API, but are helper
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 2d88897..5f6347a 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -20,6 +20,7 @@
 
 struct vb2_alloc_ctx;
 struct vb2_fileio_data;
+struct vb2_threadio_data;
 
 /**
  * struct vb2_mem_ops - memory handling/memory allocator operations
@@ -328,6 +329,7 @@ struct v4l2_fh;
  *		buffers queued. If set, then retry calling start_streaming when
  *		queuing a new buffer.
  * @fileio:	file io emulator internal data, used only if emulator is active
+ * @threadio:	thread io internal data, used only if thread is active
  */
 struct vb2_queue {
 	enum v4l2_buf_type		type;
@@ -362,6 +364,7 @@ struct vb2_queue {
 	unsigned int			retry_start_streaming:1;
 
 	struct vb2_fileio_data		*fileio;
+	struct vb2_threadio_data	*threadio;
 };
 
 void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no);
@@ -400,6 +403,31 @@ size_t vb2_read(struct vb2_queue *q, char __user *data, size_t count,
 		loff_t *ppos, int nonblock);
 size_t vb2_write(struct vb2_queue *q, const char __user *data, size_t count,
 		loff_t *ppos, int nonblock);
+/**
+ * vb2_thread_fnc - callback function for use with vb2_thread
+ *
+ * This is called whenever a buffer is dequeued in the thread.
+ */
+typedef int (*vb2_thread_fnc)(struct vb2_buffer *vb, void *priv);
+
+/**
+ * vb2_thread_start() - start a thread for the given queue.
+ * @q:		videobuf queue
+ * @fnc:	callback function
+ * @priv:	priv pointer passed to the callback function
+ * @thread_name:the name of the thread. This will be prefixed with "vb2-".
+ *
+ * This starts a thread that will queue and dequeue until an error occurs
+ * or @vb2_thread_stop is called.
+ */
+int vb2_thread_start(struct vb2_queue *q, vb2_thread_fnc fnc, void *priv,
+		     const char *thread_name);
+
+/**
+ * vb2_thread_stop() - stop the thread for the given queue.
+ * @q:		videobuf queue
+ */
+int vb2_thread_stop(struct vb2_queue *q);
 
 /**
  * vb2_is_streaming() - return streaming status of the queue
-- 
1.8.4.3

[RFC PATCH 8/8] vb2: Add videobuf2-dvb support.

[Hans Verkuil]

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

With the new vb2_thread_start/stop core code it is very easy to implement
videobuf2-dvb. This should simplify coverting existing videobuf drivers to
vb2.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/Kconfig         |   4 +
 drivers/media/v4l2-core/Makefile        |   1 +
 drivers/media/v4l2-core/videobuf2-dvb.c | 336 ++++++++++++++++++++++++++++++++
 include/media/videobuf2-dvb.h           |  58 ++++++
 4 files changed, 399 insertions(+)
 create mode 100644 drivers/media/v4l2-core/videobuf2-dvb.c
 create mode 100644 include/media/videobuf2-dvb.h

diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index 8c05565..8cc76da 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -84,6 +84,10 @@ config VIDEOBUF2_DMA_SG
 	select VIDEOBUF2_CORE
 	select VIDEOBUF2_MEMOPS
 
+config VIDEOBUF2_DVB
+	tristate
+	select VIDEOBUF2_CORE
+
 config VIDEO_V4L2_INT_DEVICE
 	tristate "V4L2 int device (DEPRECATED)"
 	depends on VIDEO_V4L2
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 1a85eee..c73ac0d 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_VIDEOBUF2_MEMOPS) += videobuf2-memops.o
 obj-$(CONFIG_VIDEOBUF2_VMALLOC) += videobuf2-vmalloc.o
 obj-$(CONFIG_VIDEOBUF2_DMA_CONTIG) += videobuf2-dma-contig.o
 obj-$(CONFIG_VIDEOBUF2_DMA_SG) += videobuf2-dma-sg.o
+obj-$(CONFIG_VIDEOBUF2_DVB) += videobuf2-dvb.o
 
 ccflags-y += -I$(srctree)/drivers/media/dvb-core
 ccflags-y += -I$(srctree)/drivers/media/dvb-frontends
diff --git a/drivers/media/v4l2-core/videobuf2-dvb.c b/drivers/media/v4l2-core/videobuf2-dvb.c
new file mode 100644
index 0000000..d092698
--- /dev/null
+++ b/drivers/media/v4l2-core/videobuf2-dvb.c
@@ -0,0 +1,336 @@
+/*
+ *
+ * some helper function for simple DVB cards which simply DMA the
+ * complete transport stream and let the computer sort everything else
+ * (i.e. we are using the software demux, ...).  Also uses the
+ * video-buf to manage DMA buffers.
+ *
+ * (c) 2004 Gerd Knorr <kraxel@bytesex.org> [SUSE Labs]
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+
+#include <media/videobuf2-dvb.h>
+
+/* ------------------------------------------------------------------ */
+
+MODULE_AUTHOR("Gerd Knorr <kraxel@bytesex.org> [SuSE Labs]");
+MODULE_LICENSE("GPL");
+
+/* ------------------------------------------------------------------ */
+
+static int dvb_fnc(struct vb2_buffer *vb, void *priv)
+{
+	struct vb2_dvb *dvb = priv;
+
+	dvb_dmx_swfilter(&dvb->demux, vb2_plane_vaddr(vb, 0),
+				      vb2_get_plane_payload(vb, 0));
+	return 0;
+}
+
+static int vb2_dvb_start_feed(struct dvb_demux_feed *feed)
+{
+	struct dvb_demux *demux = feed->demux;
+	struct vb2_dvb *dvb = demux->priv;
+	int rc = 0;
+
+	if (!demux->dmx.frontend)
+		return -EINVAL;
+
+	mutex_lock(&dvb->lock);
+	dvb->nfeeds++;
+
+	if (!dvb->dvbq.threadio) {
+		rc = vb2_thread_start(&dvb->dvbq, dvb_fnc, dvb, dvb->name);
+		if (rc)
+			dvb->nfeeds--;
+	}
+	if (!rc)
+		rc = dvb->nfeeds;
+	mutex_unlock(&dvb->lock);
+	return rc;
+}
+
+static int vb2_dvb_stop_feed(struct dvb_demux_feed *feed)
+{
+	struct dvb_demux *demux = feed->demux;
+	struct vb2_dvb *dvb = demux->priv;
+	int err = 0;
+
+	mutex_lock(&dvb->lock);
+	dvb->nfeeds--;
+	if (0 == dvb->nfeeds)
+		err = vb2_thread_stop(&dvb->dvbq);
+	mutex_unlock(&dvb->lock);
+	return err;
+}
+
+static int vb2_dvb_register_adapter(struct vb2_dvb_frontends *fe,
+			  struct module *module,
+			  void *adapter_priv,
+			  struct device *device,
+			  char *adapter_name,
+			  short *adapter_nr,
+			  int mfe_shared)
+{
+	int result;
+
+	mutex_init(&fe->lock);
+
+	/* register adapter */
+	result = dvb_register_adapter(&fe->adapter, adapter_name, module,
+		device, adapter_nr);
+	if (result < 0) {
+		pr_warn("%s: dvb_register_adapter failed (errno = %d)\n",
+		       adapter_name, result);
+	}
+	fe->adapter.priv = adapter_priv;
+	fe->adapter.mfe_shared = mfe_shared;
+
+	return result;
+}
+
+static int vb2_dvb_register_frontend(struct dvb_adapter *adapter,
+	struct vb2_dvb *dvb)
+{
+	int result;
+
+	/* register frontend */
+	result = dvb_register_frontend(adapter, dvb->frontend);
+	if (result < 0) {
+		pr_warn("%s: dvb_register_frontend failed (errno = %d)\n",
+		       dvb->name, result);
+		goto fail_frontend;
+	}
+
+	/* register demux stuff */
+	dvb->demux.dmx.capabilities =
+		DMX_TS_FILTERING | DMX_SECTION_FILTERING |
+		DMX_MEMORY_BASED_FILTERING;
+	dvb->demux.priv       = dvb;
+	dvb->demux.filternum  = 256;
+	dvb->demux.feednum    = 256;
+	dvb->demux.start_feed = vb2_dvb_start_feed;
+	dvb->demux.stop_feed  = vb2_dvb_stop_feed;
+	result = dvb_dmx_init(&dvb->demux);
+	if (result < 0) {
+		pr_warn("%s: dvb_dmx_init failed (errno = %d)\n",
+		       dvb->name, result);
+		goto fail_dmx;
+	}
+
+	dvb->dmxdev.filternum    = 256;
+	dvb->dmxdev.demux        = &dvb->demux.dmx;
+	dvb->dmxdev.capabilities = 0;
+	result = dvb_dmxdev_init(&dvb->dmxdev, adapter);
+
+	if (result < 0) {
+		pr_warn("%s: dvb_dmxdev_init failed (errno = %d)\n",
+		       dvb->name, result);
+		goto fail_dmxdev;
+	}
+
+	dvb->fe_hw.source = DMX_FRONTEND_0;
+	result = dvb->demux.dmx.add_frontend(&dvb->demux.dmx, &dvb->fe_hw);
+	if (result < 0) {
+		pr_warn("%s: add_frontend failed (DMX_FRONTEND_0, errno = %d)\n",
+		       dvb->name, result);
+		goto fail_fe_hw;
+	}
+
+	dvb->fe_mem.source = DMX_MEMORY_FE;
+	result = dvb->demux.dmx.add_frontend(&dvb->demux.dmx, &dvb->fe_mem);
+	if (result < 0) {
+		pr_warn("%s: add_frontend failed (DMX_MEMORY_FE, errno = %d)\n",
+		       dvb->name, result);
+		goto fail_fe_mem;
+	}
+
+	result = dvb->demux.dmx.connect_frontend(&dvb->demux.dmx, &dvb->fe_hw);
+	if (result < 0) {
+		pr_warn("%s: connect_frontend failed (errno = %d)\n",
+		       dvb->name, result);
+		goto fail_fe_conn;
+	}
+
+	/* register network adapter */
+	result = dvb_net_init(adapter, &dvb->net, &dvb->demux.dmx);
+	if (result < 0) {
+		pr_warn("%s: dvb_net_init failed (errno = %d)\n",
+		       dvb->name, result);
+		goto fail_fe_conn;
+	}
+	return 0;
+
+fail_fe_conn:
+	dvb->demux.dmx.remove_frontend(&dvb->demux.dmx, &dvb->fe_mem);
+fail_fe_mem:
+	dvb->demux.dmx.remove_frontend(&dvb->demux.dmx, &dvb->fe_hw);
+fail_fe_hw:
+	dvb_dmxdev_release(&dvb->dmxdev);
+fail_dmxdev:
+	dvb_dmx_release(&dvb->demux);
+fail_dmx:
+	dvb_unregister_frontend(dvb->frontend);
+fail_frontend:
+	dvb_frontend_detach(dvb->frontend);
+	dvb->frontend = NULL;
+
+	return result;
+}
+
+/* ------------------------------------------------------------------ */
+/* Register a single adapter and one or more frontends */
+int vb2_dvb_register_bus(struct vb2_dvb_frontends *f,
+			 struct module *module,
+			 void *adapter_priv,
+			 struct device *device,
+			 short *adapter_nr,
+			 int mfe_shared)
+{
+	struct list_head *list, *q;
+	struct vb2_dvb_frontend *fe;
+	int res;
+
+	fe = vb2_dvb_get_frontend(f, 1);
+	if (!fe) {
+		pr_warn("Unable to register the adapter which has no frontends\n");
+		return -EINVAL;
+	}
+
+	/* Bring up the adapter */
+	res = vb2_dvb_register_adapter(f, module, adapter_priv, device,
+		fe->dvb.name, adapter_nr, mfe_shared);
+	if (res < 0) {
+		pr_warn("vb2_dvb_register_adapter failed (errno = %d)\n", res);
+		return res;
+	}
+
+	/* Attach all of the frontends to the adapter */
+	mutex_lock(&f->lock);
+	list_for_each_safe(list, q, &f->felist) {
+		fe = list_entry(list, struct vb2_dvb_frontend, felist);
+		res = vb2_dvb_register_frontend(&f->adapter, &fe->dvb);
+		if (res < 0) {
+			pr_warn("%s: vb2_dvb_register_frontend failed (errno = %d)\n",
+				fe->dvb.name, res);
+			goto err;
+		}
+	}
+	mutex_unlock(&f->lock);
+	return 0;
+
+err:
+	mutex_unlock(&f->lock);
+	vb2_dvb_unregister_bus(f);
+	return res;
+}
+EXPORT_SYMBOL(vb2_dvb_register_bus);
+
+void vb2_dvb_unregister_bus(struct vb2_dvb_frontends *f)
+{
+	vb2_dvb_dealloc_frontends(f);
+
+	dvb_unregister_adapter(&f->adapter);
+}
+EXPORT_SYMBOL(vb2_dvb_unregister_bus);
+
+struct vb2_dvb_frontend *vb2_dvb_get_frontend(
+	struct vb2_dvb_frontends *f, int id)
+{
+	struct list_head *list, *q;
+	struct vb2_dvb_frontend *fe, *ret = NULL;
+
+	mutex_lock(&f->lock);
+
+	list_for_each_safe(list, q, &f->felist) {
+		fe = list_entry(list, struct vb2_dvb_frontend, felist);
+		if (fe->id == id) {
+			ret = fe;
+			break;
+		}
+	}
+
+	mutex_unlock(&f->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(vb2_dvb_get_frontend);
+
+int vb2_dvb_find_frontend(struct vb2_dvb_frontends *f,
+	struct dvb_frontend *p)
+{
+	struct list_head *list, *q;
+	struct vb2_dvb_frontend *fe = NULL;
+	int ret = 0;
+
+	mutex_lock(&f->lock);
+
+	list_for_each_safe(list, q, &f->felist) {
+		fe = list_entry(list, struct vb2_dvb_frontend, felist);
+		if (fe->dvb.frontend == p) {
+			ret = fe->id;
+			break;
+		}
+	}
+
+	mutex_unlock(&f->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(vb2_dvb_find_frontend);
+
+struct vb2_dvb_frontend *vb2_dvb_alloc_frontend(
+	struct vb2_dvb_frontends *f, int id)
+{
+	struct vb2_dvb_frontend *fe;
+
+	fe = kzalloc(sizeof(struct vb2_dvb_frontend), GFP_KERNEL);
+	if (fe == NULL)
+		return NULL;
+
+	fe->id = id;
+	mutex_init(&fe->dvb.lock);
+
+	mutex_lock(&f->lock);
+	list_add_tail(&fe->felist, &f->felist);
+	mutex_unlock(&f->lock);
+	return fe;
+}
+EXPORT_SYMBOL(vb2_dvb_alloc_frontend);
+
+void vb2_dvb_dealloc_frontends(struct vb2_dvb_frontends *f)
+{
+	struct list_head *list, *q;
+	struct vb2_dvb_frontend *fe;
+
+	mutex_lock(&f->lock);
+	list_for_each_safe(list, q, &f->felist) {
+		fe = list_entry(list, struct vb2_dvb_frontend, felist);
+		if (fe->dvb.net.dvbdev) {
+			dvb_net_release(&fe->dvb.net);
+			fe->dvb.demux.dmx.remove_frontend(&fe->dvb.demux.dmx,
+				&fe->dvb.fe_mem);
+			fe->dvb.demux.dmx.remove_frontend(&fe->dvb.demux.dmx,
+				&fe->dvb.fe_hw);
+			dvb_dmxdev_release(&fe->dvb.dmxdev);
+			dvb_dmx_release(&fe->dvb.demux);
+			dvb_unregister_frontend(fe->dvb.frontend);
+		}
+		if (fe->dvb.frontend)
+			/* always allocated, may have been reset */
+			dvb_frontend_detach(fe->dvb.frontend);
+		list_del(list); /* remove list entry */
+		kfree(fe);	/* free frontend allocation */
+	}
+	mutex_unlock(&f->lock);
+}
+EXPORT_SYMBOL(vb2_dvb_dealloc_frontends);
diff --git a/include/media/videobuf2-dvb.h b/include/media/videobuf2-dvb.h
new file mode 100644
index 0000000..8f61456
--- /dev/null
+++ b/include/media/videobuf2-dvb.h
@@ -0,0 +1,58 @@
+#ifndef _VIDEOBUF2_DVB_H_
+#define	_VIDEOBUF2_DVB_H_
+
+#include <dvbdev.h>
+#include <dmxdev.h>
+#include <dvb_demux.h>
+#include <dvb_net.h>
+#include <dvb_frontend.h>
+#include <media/videobuf2-core.h>
+
+struct vb2_dvb {
+	/* filling that the job of the driver */
+	char			*name;
+	struct dvb_frontend	*frontend;
+	struct vb2_queue	dvbq;
+
+	/* video-buf-dvb state info */
+	struct mutex		lock;
+	int			nfeeds;
+
+	/* vb2_dvb_(un)register manages this */
+	struct dvb_demux	demux;
+	struct dmxdev		dmxdev;
+	struct dmx_frontend	fe_hw;
+	struct dmx_frontend	fe_mem;
+	struct dvb_net		net;
+};
+
+struct vb2_dvb_frontend {
+	struct list_head felist;
+	int id;
+	struct vb2_dvb dvb;
+};
+
+struct vb2_dvb_frontends {
+	struct list_head felist;
+	struct mutex lock;
+	struct dvb_adapter adapter;
+	int active_fe_id; /* Indicates which frontend in the felist is in use */
+	int gate; /* Frontend with gate control 0=!MFE,1=fe0,2=fe1 etc */
+};
+
+int vb2_dvb_register_bus(struct vb2_dvb_frontends *f,
+			 struct module *module,
+			 void *adapter_priv,
+			 struct device *device,
+			 short *adapter_nr,
+			 int mfe_shared);
+
+void vb2_dvb_unregister_bus(struct vb2_dvb_frontends *f);
+
+struct vb2_dvb_frontend *vb2_dvb_alloc_frontend(struct vb2_dvb_frontends *f, int id);
+void vb2_dvb_dealloc_frontends(struct vb2_dvb_frontends *f);
+
+struct vb2_dvb_frontend *vb2_dvb_get_frontend(struct vb2_dvb_frontends *f, int id);
+int vb2_dvb_find_frontend(struct vb2_dvb_frontends *f, struct dvb_frontend *p);
+
+#endif			/* _VIDEOBUF2_DVB_H_ */
-- 
1.8.4.3

Re: [RFC PATCH 1/8] vb2: push the mmap semaphore down to __buf_prepare()

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thursday 21 November 2013 16:21:59 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Rather than taking the mmap semaphore at a relatively high-level function,
> push it down to the place where it is really needed.
> 
> It was placed in vb2_queue_or_prepare_buf() to prevent racing with other
> vb2 calls, however, I see no way that any race can happen.

What about the following scenario ? Both QBUF calls are performed on the same 
buffer.

	CPU 0							CPU 1
	-------------------------------------------------------------------------
	QBUF								QBUF
		locks the queue mutex				waits for the queue mutex
	vb2_qbuf
	vb2_queue_or_prepare_buf
	__vb2_qbuf
		checks vb->state, calls
	__buf_prepare
	call_qop(q, wait_prepare, q);
		unlocks the queue mutex
										locks the queue mutex
									vb2_qbuf
									vb2_queue_or_prepare_buf
									__vb2_qbuf
										checks vb->state, calls
									__buf_prepare
									call_qop(q, wait_prepare, q);
										unlocks the queue mutex

									queue the buffer, set buffer
									 state to queue

	queue the buffer, set buffer
	 state to queue


We would thus end up queueing the buffer twice. The vb->state check needs to 
be performed after the brief release of the queue mutex.

> Moving it down offers opportunities to simplify the code.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 74 +++++++++++-----------------
>  1 file changed, 30 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 91412d4..d2b2efb 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1205,6 +1205,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer
> *b) {
>  	struct vb2_queue *q = vb->vb2_queue;
> +	struct rw_semaphore *mmap_sem;
>  	int ret;
> 
>  	ret = __verify_length(vb, b);
> @@ -1219,7 +1220,25 @@ static int __buf_prepare(struct vb2_buffer *vb, const
> struct v4l2_buffer *b) ret = __qbuf_mmap(vb, b);
>  		break;
>  	case V4L2_MEMORY_USERPTR:
> +		/*
> +		 * In case of user pointer buffers vb2 allocators need to get direct
> +		 * access to userspace pages. This requires getting the mmap 
semaphore
> +		 * for read access in the current process structure. The same 
semaphore
> +		 * is taken before calling mmap operation, while both 
qbuf/prepare_buf
> +		 * and mmap are called by the driver or v4l2 core with the driver's 
lock
> +		 * held. To avoid an AB-BA deadlock (mmap_sem then driver's lock in 
mmap
> +		 * and driver's lock then mmap_sem in qbuf/prepare_buf) the videobuf2
> +		 * core releases the driver's lock, takes mmap_sem and then takes the
> +		 * driver's lock again.
> +		 */
> +		mmap_sem = &current->mm->mmap_sem;
> +		call_qop(q, wait_prepare, q);
> +		down_read(mmap_sem);
> +		call_qop(q, wait_finish, q);
> +
>  		ret = __qbuf_userptr(vb, b);
> +
> +		up_read(mmap_sem);
>  		break;
>  	case V4L2_MEMORY_DMABUF:
>  		ret = __qbuf_dmabuf(vb, b);
> @@ -1245,80 +1264,47 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue
> *q, struct v4l2_buffer *b, struct v4l2_buffer *,
>  						   struct vb2_buffer *))
>  {
> -	struct rw_semaphore *mmap_sem = NULL;
>  	struct vb2_buffer *vb;
>  	int ret;
> 
> -	/*
> -	 * In case of user pointer buffers vb2 allocators need to get direct
> -	 * access to userspace pages. This requires getting the mmap semaphore
> -	 * for read access in the current process structure. The same semaphore
> -	 * is taken before calling mmap operation, while both qbuf/prepare_buf
> -	 * and mmap are called by the driver or v4l2 core with the driver's lock
> -	 * held. To avoid an AB-BA deadlock (mmap_sem then driver's lock in mmap
> -	 * and driver's lock then mmap_sem in qbuf/prepare_buf) the videobuf2
> -	 * core releases the driver's lock, takes mmap_sem and then takes the
> -	 * driver's lock again.
> -	 *
> -	 * To avoid racing with other vb2 calls, which might be called after
> -	 * releasing the driver's lock, this operation is performed at the
> -	 * beginning of qbuf/prepare_buf processing. This way the queue status
> -	 * is consistent after getting the driver's lock back.
> -	 */
> -	if (q->memory == V4L2_MEMORY_USERPTR) {
> -		mmap_sem = &current->mm->mmap_sem;
> -		call_qop(q, wait_prepare, q);
> -		down_read(mmap_sem);
> -		call_qop(q, wait_finish, q);
> -	}
> -
>  	if (q->fileio) {
>  		dprintk(1, "%s(): file io in progress\n", opname);
> -		ret = -EBUSY;
> -		goto unlock;
> +		return -EBUSY;
>  	}
> 
>  	if (b->type != q->type) {
>  		dprintk(1, "%s(): invalid buffer type\n", opname);
> -		ret = -EINVAL;
> -		goto unlock;
> +		return -EINVAL;
>  	}
> 
>  	if (b->index >= q->num_buffers) {
>  		dprintk(1, "%s(): buffer index out of range\n", opname);
> -		ret = -EINVAL;
> -		goto unlock;
> +		return -EINVAL;
>  	}
> 
>  	vb = q->bufs[b->index];
>  	if (NULL == vb) {
>  		/* Should never happen */
>  		dprintk(1, "%s(): buffer is NULL\n", opname);
> -		ret = -EINVAL;
> -		goto unlock;
> +		return -EINVAL;
>  	}
> 
>  	if (b->memory != q->memory) {
>  		dprintk(1, "%s(): invalid memory type\n", opname);
> -		ret = -EINVAL;
> -		goto unlock;
> +		return -EINVAL;
>  	}
> 
>  	ret = __verify_planes_array(vb, b);
>  	if (ret)
> -		goto unlock;
> +		return ret;
> 
>  	ret = handler(q, b, vb);
> -	if (ret)
> -		goto unlock;
> -
> -	/* Fill buffer information for the userspace */
> -	__fill_v4l2_buffer(vb, b);
> +	if (!ret) {
> +		/* Fill buffer information for the userspace */
> +		__fill_v4l2_buffer(vb, b);
> 
> -	dprintk(1, "%s() of buffer %d succeeded\n", opname, vb->v4l2_buf.index);
> -unlock:
> -	if (mmap_sem)
> -		up_read(mmap_sem);
> +		dprintk(1, "%s() of buffer %d succeeded\n", opname, vb-
>v4l2_buf.index);
> +	}
>  	return ret;
>  }
-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 4/8] vb2: retry start_streaming in case of insufficient buffers.

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thursday 21 November 2013 16:22:02 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> If start_streaming returns -ENODATA, then it will be retried the next time
> a buffer is queued. This means applications no longer need to know how many
> buffers need to be queued before STREAMON can be called. This is
> particularly useful for output stream I/O.
> 
> If a DMA engine needs at least X buffers before it can start streaming, then
> for applications to get a buffer out as soon as possible they need to know
> the minimum number of buffers to queue before STREAMON can be called. You
> can't just try STREAMON after every buffer since on failure STREAMON will
> dequeue all your buffers. (Is that a bug or a feature? Frankly, I'm not
> sure).
> 
> This patch simplifies applications substantially: they can just call
> STREAMON at the beginning and then start queuing buffers and the DMA engine
> will kick in automagically once enough buffers are available.
> 
> This also fixes using write() to stream video: the fileio implementation
> calls streamon without having any queued buffers, which will fail today for
> any driver that requires a minimum number of buffers.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 66 ++++++++++++++++++++++-------
>  include/media/videobuf2-core.h           | 15 ++++++--
>  2 files changed, 64 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 9ea3ae9..5bb91f7 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1332,6 +1332,39 @@ int vb2_prepare_buf(struct vb2_queue *q, struct
> v4l2_buffer *b) }
>  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
> + * true and 0 is returned. The next time a buffer is queued and
> + * retry_start_streaming is true, this function will be called again to
> + * retry starting the DMA engine.
> + */
> +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));
> +
> +	/*
> +	 * If there are not enough buffers queued to start streaming, then
> +	 * the start_streaming operation will return -ENODATA and you have to
> +	 * retry when the next buffer is queued.
> +	 */
> +	if (ret == -ENODATA) {
> +		dprintk(1, "qbuf: not enough buffers, retry when more buffers are
> queued.\n");
> +		q->retry_start_streaming = true;
> +		return 0;
> +	}
> +	if (ret)
> +		dprintk(1, "qbuf: driver refused to start streaming\n");
> +	else
> +		q->retry_start_streaming = false;
> +	return ret;
> +}
> +
>  static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  {
>  	int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> @@ -1377,6 +1410,12 @@ static int vb2_internal_qbuf(struct vb2_queue *q,
> struct v4l2_buffer *b) /* Fill buffer information for the userspace */
>  	__fill_v4l2_buffer(vb, b);
> 
> +	if (q->retry_start_streaming) {
> +		ret = vb2_start_streaming(q);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb-
>v4l2_buf.index);
> return 0;
>  }
> @@ -1526,7 +1565,8 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
>  		return -EINVAL;
>  	}
> 
> -	wait_event(q->done_wq, !atomic_read(&q->queued_count));
> +	if (!q->retry_start_streaming)
> +		wait_event(q->done_wq, !atomic_read(&q->queued_count));
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
> @@ -1640,6 +1680,9 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>  {
>  	unsigned int i;
> 
> +	if (q->retry_start_streaming)
> +		q->retry_start_streaming = q->streaming = 0;
> +
>  	/*
>  	 * Tell driver to stop all transactions and release all queued
>  	 * buffers.
> @@ -1689,12 +1732,9 @@ static int vb2_internal_streamon(struct vb2_queue *q,
> enum v4l2_buf_type type) list_for_each_entry(vb, &q->queued_list,
> queued_entry)
>  		__enqueue_in_driver(vb);
> 
> -	/*
> -	 * Let driver notice that streaming state has been enabled.
> -	 */
> -	ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
> +	/* Tell driver to start streaming. */

Wouldn't it be better to reset q->retry_start_streaming to 0 here instead of 
in the several other locations ?

> +	ret = vb2_start_streaming(q);
>  	if (ret) {
> -		dprintk(1, "streamon: driver refused to start streaming\n");
>  		__vb2_queue_cancel(q);
>  		return ret;
>  	}
> @@ -2264,15 +2304,15 @@ static int __vb2_init_fileio(struct vb2_queue *q,
> int read) goto err_reqbufs;
>  			fileio->bufs[i].queued = 1;
>  		}
> -
> -		/*
> -		 * Start streaming.
> -		 */
> -		ret = vb2_streamon(q, q->type);
> -		if (ret)
> -			goto err_reqbufs;
>  	}
> 
> +	/*
> +	 * Start streaming.
> +	 */
> +	ret = vb2_streamon(q, q->type);
> +	if (ret)
> +		goto err_reqbufs;
> +
>  	q->fileio = fileio;
> 
>  	return ret;
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index bd8218b..2d88897 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -250,10 +250,13 @@ struct vb2_buffer {
>   *			receive buffers with @buf_queue callback before
>   *			@start_streaming is called; the driver gets the number
>   *			of already queued buffers in count parameter; driver
> - *			can return an error if hardware fails or not enough
> - *			buffers has been queued, in such case all buffers that
> - *			have been already given by the @buf_queue callback are
> - *			invalidated.
> + *			can return an error if hardware fails, in that case all
> + *			buffers that have been already given by the @buf_queue
> + *			callback are invalidated.
> + *			If there were not enough queued buffers to start
> + *			streaming, then this callback returns -ENODATA, and the
> + *			vb2 core will retry calling @start_streaming when a new
> + *			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()
> @@ -321,6 +324,9 @@ struct v4l2_fh;
>   * @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.
>   * @fileio:	file io emulator internal data, used only if emulator is 
active
> */
>  struct vb2_queue {
> @@ -353,6 +359,7 @@ struct vb2_queue {
>  	unsigned int			plane_sizes[VIDEO_MAX_PLANES];
> 
>  	unsigned int			streaming:1;
> +	unsigned int			retry_start_streaming:1;
> 
>  	struct vb2_fileio_data		*fileio;
>  };
-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 4/8] vb2: retry start_streaming in case of insufficient buffers.

[Hans Verkuil]

On 11/21/2013 08:09 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Thursday 21 November 2013 16:22:02 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> If start_streaming returns -ENODATA, then it will be retried the next time
>> a buffer is queued. This means applications no longer need to know how many
>> buffers need to be queued before STREAMON can be called. This is
>> particularly useful for output stream I/O.
>>
>> If a DMA engine needs at least X buffers before it can start streaming, then
>> for applications to get a buffer out as soon as possible they need to know
>> the minimum number of buffers to queue before STREAMON can be called. You
>> can't just try STREAMON after every buffer since on failure STREAMON will
>> dequeue all your buffers. (Is that a bug or a feature? Frankly, I'm not
>> sure).
>>
>> This patch simplifies applications substantially: they can just call
>> STREAMON at the beginning and then start queuing buffers and the DMA engine
>> will kick in automagically once enough buffers are available.
>>
>> This also fixes using write() to stream video: the fileio implementation
>> calls streamon without having any queued buffers, which will fail today for
>> any driver that requires a minimum number of buffers.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 66 ++++++++++++++++++++++-------
>>  include/media/videobuf2-core.h           | 15 ++++++--
>>  2 files changed, 64 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> b/drivers/media/v4l2-core/videobuf2-core.c index 9ea3ae9..5bb91f7 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -1332,6 +1332,39 @@ int vb2_prepare_buf(struct vb2_queue *q, struct
>> v4l2_buffer *b) }
>>  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
>> + * true and 0 is returned. The next time a buffer is queued and
>> + * retry_start_streaming is true, this function will be called again to
>> + * retry starting the DMA engine.
>> + */
>> +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));
>> +
>> +	/*
>> +	 * If there are not enough buffers queued to start streaming, then
>> +	 * the start_streaming operation will return -ENODATA and you have to
>> +	 * retry when the next buffer is queued.
>> +	 */
>> +	if (ret == -ENODATA) {
>> +		dprintk(1, "qbuf: not enough buffers, retry when more buffers are
>> queued.\n");
>> +		q->retry_start_streaming = true;
>> +		return 0;
>> +	}
>> +	if (ret)
>> +		dprintk(1, "qbuf: driver refused to start streaming\n");
>> +	else
>> +		q->retry_start_streaming = false;
>> +	return ret;
>> +}
>> +
>>  static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>>  {
>>  	int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
>> @@ -1377,6 +1410,12 @@ static int vb2_internal_qbuf(struct vb2_queue *q,
>> struct v4l2_buffer *b) /* Fill buffer information for the userspace */
>>  	__fill_v4l2_buffer(vb, b);
>>
>> +	if (q->retry_start_streaming) {
>> +		ret = vb2_start_streaming(q);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb-
>> v4l2_buf.index);
>> return 0;
>>  }
>> @@ -1526,7 +1565,8 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
>>  		return -EINVAL;
>>  	}
>>
>> -	wait_event(q->done_wq, !atomic_read(&q->queued_count));
>> +	if (!q->retry_start_streaming)
>> +		wait_event(q->done_wq, !atomic_read(&q->queued_count));
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
>> @@ -1640,6 +1680,9 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>>  {
>>  	unsigned int i;
>>
>> +	if (q->retry_start_streaming)
>> +		q->retry_start_streaming = q->streaming = 0;
>> +
>>  	/*
>>  	 * Tell driver to stop all transactions and release all queued
>>  	 * buffers.
>> @@ -1689,12 +1732,9 @@ static int vb2_internal_streamon(struct vb2_queue *q,
>> enum v4l2_buf_type type) list_for_each_entry(vb, &q->queued_list,
>> queued_entry)
>>  		__enqueue_in_driver(vb);
>>
>> -	/*
>> -	 * Let driver notice that streaming state has been enabled.
>> -	 */
>> -	ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
>> +	/* Tell driver to start streaming. */
> 
> Wouldn't it be better to reset q->retry_start_streaming to 0 here instead of 
> in the several other locations ? 

I don't follow. retry_start_streaming is set only in vb2_start_streaming or
in queue_cancel. I'm not sure what vb2_internal_streamon has to do with
this.

Regards,

	Hans

> 
>> +	ret = vb2_start_streaming(q);
>>  	if (ret) {
>> -		dprintk(1, "streamon: driver refused to start streaming\n");
>>  		__vb2_queue_cancel(q);
>>  		return ret;
>>  	}
>> @@ -2264,15 +2304,15 @@ static int __vb2_init_fileio(struct vb2_queue *q,
>> int read) goto err_reqbufs;
>>  			fileio->bufs[i].queued = 1;
>>  		}
>> -
>> -		/*
>> -		 * Start streaming.
>> -		 */
>> -		ret = vb2_streamon(q, q->type);
>> -		if (ret)
>> -			goto err_reqbufs;
>>  	}
>>
>> +	/*
>> +	 * Start streaming.
>> +	 */
>> +	ret = vb2_streamon(q, q->type);
>> +	if (ret)
>> +		goto err_reqbufs;
>> +
>>  	q->fileio = fileio;
>>
>>  	return ret;
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index bd8218b..2d88897 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -250,10 +250,13 @@ struct vb2_buffer {
>>   *			receive buffers with @buf_queue callback before
>>   *			@start_streaming is called; the driver gets the number
>>   *			of already queued buffers in count parameter; driver
>> - *			can return an error if hardware fails or not enough
>> - *			buffers has been queued, in such case all buffers that
>> - *			have been already given by the @buf_queue callback are
>> - *			invalidated.
>> + *			can return an error if hardware fails, in that case all
>> + *			buffers that have been already given by the @buf_queue
>> + *			callback are invalidated.
>> + *			If there were not enough queued buffers to start
>> + *			streaming, then this callback returns -ENODATA, and the
>> + *			vb2 core will retry calling @start_streaming when a new
>> + *			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()
>> @@ -321,6 +324,9 @@ struct v4l2_fh;
>>   * @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.
>>   * @fileio:	file io emulator internal data, used only if emulator is 
> active
>> */
>>  struct vb2_queue {
>> @@ -353,6 +359,7 @@ struct vb2_queue {
>>  	unsigned int			plane_sizes[VIDEO_MAX_PLANES];
>>
>>  	unsigned int			streaming:1;
>> +	unsigned int			retry_start_streaming:1;
>>
>>  	struct vb2_fileio_data		*fileio;
>>  };

Re: [RFC PATCH 1/8] vb2: push the mmap semaphore down to __buf_prepare()

[Hans Verkuil]

On 11/21/2013 08:04 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Thursday 21 November 2013 16:21:59 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Rather than taking the mmap semaphore at a relatively high-level function,
>> push it down to the place where it is really needed.
>>
>> It was placed in vb2_queue_or_prepare_buf() to prevent racing with other
>> vb2 calls, however, I see no way that any race can happen.
> 
> What about the following scenario ? Both QBUF calls are performed on the same 
> buffer.
> 
> 	CPU 0							CPU 1
> 	-------------------------------------------------------------------------
> 	QBUF								QBUF
> 		locks the queue mutex				waits for the queue mutex
> 	vb2_qbuf
> 	vb2_queue_or_prepare_buf
> 	__vb2_qbuf
> 		checks vb->state, calls
> 	__buf_prepare
> 	call_qop(q, wait_prepare, q);
> 		unlocks the queue mutex
> 										locks the queue mutex
> 									vb2_qbuf
> 									vb2_queue_or_prepare_buf
> 									__vb2_qbuf
> 										checks vb->state, calls
> 									__buf_prepare
> 									call_qop(q, wait_prepare, q);
> 										unlocks the queue mutex
> 
> 									queue the buffer, set buffer
> 									 state to queue
> 
> 	queue the buffer, set buffer
> 	 state to queue
> 
> 
> We would thus end up queueing the buffer twice. The vb->state check needs to 
> be performed after the brief release of the queue mutex.

Good point, I hadn't thought about that scenario. However, using mmap_sem to
introduce a large critical section just to protect against state changes is IMHO
not the right approach. Why not introduce a VB2_BUF_STATE_PREPARING state?

That's set at the start of __buf_prepare while the queue mutex is still held, and
which prevents other threads of queuing the same buffer again. If the prepare fails,
then the state is reverted back to DEQUEUED.

__fill_v4l2_buffer() will handle the PREPARING state as if it was the DEQUEUED state.

What do you think?

Regards,

	Hans

Re: [RFC PATCH 1/8] vb2: push the mmap semaphore down to __buf_prepare()

[Laurent Pinchart]

Hi Hans,

On Friday 22 November 2013 10:02:49 Hans Verkuil wrote:
> On 11/21/2013 08:04 PM, Laurent Pinchart wrote:
> > On Thursday 21 November 2013 16:21:59 Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >> 
> >> Rather than taking the mmap semaphore at a relatively high-level
> >> function, push it down to the place where it is really needed.
> >> 
> >> It was placed in vb2_queue_or_prepare_buf() to prevent racing with other
> >> vb2 calls, however, I see no way that any race can happen.
> > 
> > What about the following scenario ? Both QBUF calls are performed on the
> > same buffer.
> > 
> > 	CPU 0							CPU 1
> > 	-------------------------------------------------------------------------
> > 	QBUF								QBUF
> > 		locks the queue mutex				waits for the queue mutex
> > 	vb2_qbuf
> > 	vb2_queue_or_prepare_buf
> > 	__vb2_qbuf
> > 		checks vb->state, calls
> > 	__buf_prepare
> > 	call_qop(q, wait_prepare, q);
> > 		unlocks the queue mutex
> > 		
> > 										locks the queue mutex
> > 									vb2_qbuf
> > 									vb2_queue_or_prepare_buf
> > 									__vb2_qbuf
> > 										checks vb->state, calls
> > 									__buf_prepare
> > 									call_qop(q, wait_prepare, q);
> > 										unlocks the queue mutex
> > 									queue the buffer, set buffer
> > 									 state to queue
> > 	
> > 	queue the buffer, set buffer
> > 	 state to queue
> > 
> > We would thus end up queueing the buffer twice. The vb->state check needs
> > to be performed after the brief release of the queue mutex.
> 
> Good point, I hadn't thought about that scenario. However, using mmap_sem to
> introduce a large critical section just to protect against state changes is
> IMHO not the right approach. Why not introduce a VB2_BUF_STATE_PREPARING
> state?

Note that we use the queue mutex to do so, not mmap_sem. The problem is that 
we can't release the queue mutex in the middle of a critical section without 
risking being preempted by another task. Introducing a new state might be 
possible if it effectively breaks the critical section in two independent 
parts.

> That's set at the start of __buf_prepare while the queue mutex is still
> held, and which prevents other threads of queuing the same buffer again. If
> the prepare fails, then the state is reverted back to DEQUEUED.
> 
> __fill_v4l2_buffer() will handle the PREPARING state as if it was the
> DEQUEUED state.
> 
> What do you think?

I'll have to review that in details given the potential complexity of locking 
issues :-) I'm not opposed to the idea, if it works I believe we should do it.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 4/8] vb2: retry start_streaming in case of insufficient buffers.

[Laurent Pinchart]

Hi Hans,

On Friday 22 November 2013 09:43:23 Hans Verkuil wrote:
> On 11/21/2013 08:09 PM, Laurent Pinchart wrote:
> > On Thursday 21 November 2013 16:22:02 Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >> 
> >> If start_streaming returns -ENODATA, then it will be retried the next
> >> time a buffer is queued. This means applications no longer need to know
> >> how many buffers need to be queued before STREAMON can be called. This is
> >> particularly useful for output stream I/O.
> >> 
> >> If a DMA engine needs at least X buffers before it can start streaming,
> >> then for applications to get a buffer out as soon as possible they need
> >> to know the minimum number of buffers to queue before STREAMON can be
> >> called. You can't just try STREAMON after every buffer since on failure
> >> STREAMON will dequeue all your buffers. (Is that a bug or a feature?
> >> Frankly, I'm not sure).
> >> 
> >> This patch simplifies applications substantially: they can just call
> >> STREAMON at the beginning and then start queuing buffers and the DMA
> >> engine will kick in automagically once enough buffers are available.
> >> 
> >> This also fixes using write() to stream video: the fileio implementation
> >> calls streamon without having any queued buffers, which will fail today
> >> for any driver that requires a minimum number of buffers.
> >> 
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >> 
> >>  drivers/media/v4l2-core/videobuf2-core.c | 66 +++++++++++++++++++-------
> >>  include/media/videobuf2-core.h           | 15 ++++++--
> >>  2 files changed, 64 insertions(+), 17 deletions(-)
> >> 
> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> >> b/drivers/media/v4l2-core/videobuf2-core.c index 9ea3ae9..5bb91f7 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >> @@ -1332,6 +1332,39 @@ int vb2_prepare_buf(struct vb2_queue *q, struct
> >> v4l2_buffer *b) }
> >> 
> >>  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
> >> + * true and 0 is returned. The next time a buffer is queued and
> >> + * retry_start_streaming is true, this function will be called again to
> >> + * retry starting the DMA engine.
> >> + */
> >> +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));
> >> +
> >> +	/*
> >> +	 * If there are not enough buffers queued to start streaming, then
> >> +	 * the start_streaming operation will return -ENODATA and you have to
> >> +	 * retry when the next buffer is queued.
> >> +	 */
> >> +	if (ret == -ENODATA) {
> >> +		dprintk(1, "qbuf: not enough buffers, retry when more buffers are
> >> queued.\n");
> >> +		q->retry_start_streaming = true;
> >> +		return 0;
> >> +	}
> >> +	if (ret)
> >> +		dprintk(1, "qbuf: driver refused to start streaming\n");
> >> +	else
> >> +		q->retry_start_streaming = false;
> >> +	return ret;
> >> +}
> >> +
> >>  static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >>  {
> >>  	int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> >> @@ -1377,6 +1410,12 @@ static int vb2_internal_qbuf(struct vb2_queue *q,
> >> struct v4l2_buffer *b) /* Fill buffer information for the userspace */
> >> 
> >>  	__fill_v4l2_buffer(vb, b);
> >> 
> >> +	if (q->retry_start_streaming) {
> >> +		ret = vb2_start_streaming(q);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >>  	dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb-
> >> v4l2_buf.index);
> >> return 0;
> >>  }
> >> 
> >> @@ -1526,7 +1565,8 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
> >>  		return -EINVAL;
> >>  	}
> >> 
> >> -	wait_event(q->done_wq, !atomic_read(&q->queued_count));
> >> +	if (!q->retry_start_streaming)
> >> +		wait_event(q->done_wq, !atomic_read(&q->queued_count));
> >>  	return 0;
> >>  
> >>  }
> >>  EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
> >> @@ -1640,6 +1680,9 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> >>  {
> >>  	unsigned int i;
> >> 
> >> +	if (q->retry_start_streaming)
> >> +		q->retry_start_streaming = q->streaming = 0;
> >> +
> >>  	/*
> >>  	 * Tell driver to stop all transactions and release all queued
> >>  	 * buffers.
> >> @@ -1689,12 +1732,9 @@ static int vb2_internal_streamon(struct vb2_queue
> >> *q, enum v4l2_buf_type type) list_for_each_entry(vb, &q->queued_list,
> >> queued_entry)
> >> 
> >>  		__enqueue_in_driver(vb);
> >> 
> >> -	/*
> >> -	 * Let driver notice that streaming state has been enabled.
> >> -	 */
> >> -	ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
> >> +	/* Tell driver to start streaming. */
> > 
> > Wouldn't it be better to reset q->retry_start_streaming to 0 here instead
> > of in the several other locations ?
> 
> I don't follow. retry_start_streaming is set only in vb2_start_streaming or
> in queue_cancel. I'm not sure what vb2_internal_streamon has to do with
> this.

My point is that the code would be simpler and less error-prone if we reset 
retry_start_streaming in a single location right before starting the stream 
instead of in multiple locations when stopping the stream. There would be less 
chances to introduce a bug when refactoring code in the future in that case.

> >> +	ret = vb2_start_streaming(q);
> >>  	if (ret) {
> >> -		dprintk(1, "streamon: driver refused to start streaming\n");
> >>  		__vb2_queue_cancel(q);
> >>  		return ret;
> >>  	}
> >> @@ -2264,15 +2304,15 @@ static int __vb2_init_fileio(struct vb2_queue *q,
> >> int read) goto err_reqbufs;
> >> 
> >>  			fileio->bufs[i].queued = 1;
> >>  		}
> >> -
> >> -		/*
> >> -		 * Start streaming.
> >> -		 */
> >> -		ret = vb2_streamon(q, q->type);
> >> -		if (ret)
> >> -			goto err_reqbufs;
> >>  	}
> >> 
> >> +	/*
> >> +	 * Start streaming.
> >> +	 */
> >> +	ret = vb2_streamon(q, q->type);
> >> +	if (ret)
> >> +		goto err_reqbufs;
> >> +
> >>  	q->fileio = fileio;
> >>  	
> >>  	return ret;

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/8] vb2: push the mmap semaphore down to __buf_prepare()

[Hans Verkuil]

On 11/26/2013 04:42 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 22 November 2013 10:02:49 Hans Verkuil wrote:
>> On 11/21/2013 08:04 PM, Laurent Pinchart wrote:
>>> On Thursday 21 November 2013 16:21:59 Hans Verkuil wrote:
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> Rather than taking the mmap semaphore at a relatively high-level
>>>> function, push it down to the place where it is really needed.
>>>>
>>>> It was placed in vb2_queue_or_prepare_buf() to prevent racing with other
>>>> vb2 calls, however, I see no way that any race can happen.
>>>
>>> What about the following scenario ? Both QBUF calls are performed on the
>>> same buffer.
>>>
>>> 	CPU 0							CPU 1
>>> 	-------------------------------------------------------------------------
>>> 	QBUF								QBUF
>>> 		locks the queue mutex				waits for the queue mutex
>>> 	vb2_qbuf
>>> 	vb2_queue_or_prepare_buf
>>> 	__vb2_qbuf
>>> 		checks vb->state, calls
>>> 	__buf_prepare
>>> 	call_qop(q, wait_prepare, q);
>>> 		unlocks the queue mutex
>>> 		
>>> 										locks the queue mutex
>>> 									vb2_qbuf
>>> 									vb2_queue_or_prepare_buf
>>> 									__vb2_qbuf
>>> 										checks vb->state, calls
>>> 									__buf_prepare
>>> 									call_qop(q, wait_prepare, q);
>>> 										unlocks the queue mutex
>>> 									queue the buffer, set buffer
>>> 									 state to queue
>>> 	
>>> 	queue the buffer, set buffer
>>> 	 state to queue
>>>
>>> We would thus end up queueing the buffer twice. The vb->state check needs
>>> to be performed after the brief release of the queue mutex.
>>
>> Good point, I hadn't thought about that scenario. However, using mmap_sem to
>> introduce a large critical section just to protect against state changes is
>> IMHO not the right approach. Why not introduce a VB2_BUF_STATE_PREPARING
>> state?
> 
> Note that we use the queue mutex to do so, not mmap_sem. The problem is that 
> we can't release the queue mutex in the middle of a critical section without 
> risking being preempted by another task. Introducing a new state might be 
> possible if it effectively breaks the critical section in two independent 
> parts.
> 
>> That's set at the start of __buf_prepare while the queue mutex is still
>> held, and which prevents other threads of queuing the same buffer again. If
>> the prepare fails, then the state is reverted back to DEQUEUED.
>>
>> __fill_v4l2_buffer() will handle the PREPARING state as if it was the
>> DEQUEUED state.
>>
>> What do you think?
> 
> I'll have to review that in details given the potential complexity of locking 
> issues :-) I'm not opposed to the idea, if it works I believe we should do it.
> 

Do you want to think about this first, or shall I make a new patch that you can
then review?

Regards,

	Hans

Re: [RFC PATCH 4/8] vb2: retry start_streaming in case of insufficient buffers.

[Hans Verkuil]

On 11/26/2013 04:46 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 22 November 2013 09:43:23 Hans Verkuil wrote:
>> On 11/21/2013 08:09 PM, Laurent Pinchart wrote:
>>> On Thursday 21 November 2013 16:22:02 Hans Verkuil wrote:
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> If start_streaming returns -ENODATA, then it will be retried the next
>>>> time a buffer is queued. This means applications no longer need to know
>>>> how many buffers need to be queued before STREAMON can be called. This is
>>>> particularly useful for output stream I/O.
>>>>
>>>> If a DMA engine needs at least X buffers before it can start streaming,
>>>> then for applications to get a buffer out as soon as possible they need
>>>> to know the minimum number of buffers to queue before STREAMON can be
>>>> called. You can't just try STREAMON after every buffer since on failure
>>>> STREAMON will dequeue all your buffers. (Is that a bug or a feature?
>>>> Frankly, I'm not sure).
>>>>
>>>> This patch simplifies applications substantially: they can just call
>>>> STREAMON at the beginning and then start queuing buffers and the DMA
>>>> engine will kick in automagically once enough buffers are available.
>>>>
>>>> This also fixes using write() to stream video: the fileio implementation
>>>> calls streamon without having any queued buffers, which will fail today
>>>> for any driver that requires a minimum number of buffers.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> ---
>>>>
>>>>  drivers/media/v4l2-core/videobuf2-core.c | 66 +++++++++++++++++++-------
>>>>  include/media/videobuf2-core.h           | 15 ++++++--
>>>>  2 files changed, 64 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>>>> b/drivers/media/v4l2-core/videobuf2-core.c index 9ea3ae9..5bb91f7 100644
>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>>> @@ -1332,6 +1332,39 @@ int vb2_prepare_buf(struct vb2_queue *q, struct
>>>> v4l2_buffer *b) }
>>>>
>>>>  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
>>>> + * true and 0 is returned. The next time a buffer is queued and
>>>> + * retry_start_streaming is true, this function will be called again to
>>>> + * retry starting the DMA engine.
>>>> + */
>>>> +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));
>>>> +
>>>> +	/*
>>>> +	 * If there are not enough buffers queued to start streaming, then
>>>> +	 * the start_streaming operation will return -ENODATA and you have to
>>>> +	 * retry when the next buffer is queued.
>>>> +	 */
>>>> +	if (ret == -ENODATA) {
>>>> +		dprintk(1, "qbuf: not enough buffers, retry when more buffers are
>>>> queued.\n");
>>>> +		q->retry_start_streaming = true;
>>>> +		return 0;
>>>> +	}
>>>> +	if (ret)
>>>> +		dprintk(1, "qbuf: driver refused to start streaming\n");
>>>> +	else
>>>> +		q->retry_start_streaming = false;
>>>> +	return ret;
>>>> +}
>>>> +
>>>>  static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>>>>  {
>>>>  	int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
>>>> @@ -1377,6 +1410,12 @@ static int vb2_internal_qbuf(struct vb2_queue *q,
>>>> struct v4l2_buffer *b) /* Fill buffer information for the userspace */
>>>>
>>>>  	__fill_v4l2_buffer(vb, b);
>>>>
>>>> +	if (q->retry_start_streaming) {
>>>> +		ret = vb2_start_streaming(q);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>>  	dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb-
>>>> v4l2_buf.index);
>>>> return 0;
>>>>  }
>>>>
>>>> @@ -1526,7 +1565,8 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
>>>>  		return -EINVAL;
>>>>  	}
>>>>
>>>> -	wait_event(q->done_wq, !atomic_read(&q->queued_count));
>>>> +	if (!q->retry_start_streaming)
>>>> +		wait_event(q->done_wq, !atomic_read(&q->queued_count));
>>>>  	return 0;
>>>>  
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
>>>> @@ -1640,6 +1680,9 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>>>>  {
>>>>  	unsigned int i;
>>>>
>>>> +	if (q->retry_start_streaming)
>>>> +		q->retry_start_streaming = q->streaming = 0;
>>>> +
>>>>  	/*
>>>>  	 * Tell driver to stop all transactions and release all queued
>>>>  	 * buffers.
>>>> @@ -1689,12 +1732,9 @@ static int vb2_internal_streamon(struct vb2_queue
>>>> *q, enum v4l2_buf_type type) list_for_each_entry(vb, &q->queued_list,
>>>> queued_entry)
>>>>
>>>>  		__enqueue_in_driver(vb);
>>>>
>>>> -	/*
>>>> -	 * Let driver notice that streaming state has been enabled.
>>>> -	 */
>>>> -	ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
>>>> +	/* Tell driver to start streaming. */
>>>
>>> Wouldn't it be better to reset q->retry_start_streaming to 0 here instead
>>> of in the several other locations ?
>>
>> I don't follow. retry_start_streaming is set only in vb2_start_streaming or
>> in queue_cancel. I'm not sure what vb2_internal_streamon has to do with
>> this.
> 
> My point is that the code would be simpler and less error-prone if we reset 
> retry_start_streaming in a single location right before starting the stream 
> instead of in multiple locations when stopping the stream. There would be less 
> chances to introduce a bug when refactoring code in the future in that case.

But that's what happens: it's set when starting the stream (vb2_start_streaming)
and cleared when stopping the stream (__vb2_queue_cancel). I really can't make
it simpler than that.

I still don't see what it is you want to improve here.

Regards,

	Hans

Re: [RFC PATCH 1/8] vb2: push the mmap semaphore down to __buf_prepare()

[Laurent Pinchart]

Hi Hans,

On Wednesday 27 November 2013 08:12:24 Hans Verkuil wrote:
> On 11/26/2013 04:42 PM, Laurent Pinchart wrote:
> > On Friday 22 November 2013 10:02:49 Hans Verkuil wrote:
> >> On 11/21/2013 08:04 PM, Laurent Pinchart wrote:
> >>> On Thursday 21 November 2013 16:21:59 Hans Verkuil wrote:
> >>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> 
> >>>> Rather than taking the mmap semaphore at a relatively high-level
> >>>> function, push it down to the place where it is really needed.
> >>>> 
> >>>> It was placed in vb2_queue_or_prepare_buf() to prevent racing with
> >>>> other vb2 calls, however, I see no way that any race can happen.
> >>> 
> >>> What about the following scenario ? Both QBUF calls are performed on the
> >>> same buffer.
> >>> 
> >>> 	CPU 0							CPU 1
> >>> 	---------------------------------------------------------------------
> >>> 	QBUF								QBUF
> >>> 		locks the queue mutex				waits for the queue mutex
> >>> 	vb2_qbuf
> >>> 	vb2_queue_or_prepare_buf
> >>> 	__vb2_qbuf
> >>> 		checks vb->state, calls
> >>> 	__buf_prepare
> >>> 	call_qop(q, wait_prepare, q);
> >>> 		unlocks the queue mutex
> >>> 										locks the queue mutex
> >>> 									vb2_qbuf
> >>> 									vb2_queue_or_prepare_buf
> >>> 									__vb2_qbuf
> >>> 										checks vb->state, calls
> >>> 									__buf_prepare
> >>> 									call_qop(q, wait_prepare, q);
> >>> 										unlocks the queue mutex
> >>> 									queue the buffer, set buffer
> >>> 									 state to queue
> >>> 	queue the buffer, set buffer
> >>> 	 state to queue
> >>> 
> >>> We would thus end up queueing the buffer twice. The vb->state check
> >>> needs to be performed after the brief release of the queue mutex.
> >> 
> >> Good point, I hadn't thought about that scenario. However, using mmap_sem
> >> to introduce a large critical section just to protect against state
> >> changes is IMHO not the right approach. Why not introduce a
> >> VB2_BUF_STATE_PREPARING state?
> > 
> > Note that we use the queue mutex to do so, not mmap_sem. The problem is
> > that we can't release the queue mutex in the middle of a critical section
> > without risking being preempted by another task. Introducing a new state
> > might be possible if it effectively breaks the critical section in two
> > independent parts.
> > 
> >> That's set at the start of __buf_prepare while the queue mutex is still
> >> held, and which prevents other threads of queuing the same buffer again.
> >> If the prepare fails, then the state is reverted back to DEQUEUED.
> >> 
> >> __fill_v4l2_buffer() will handle the PREPARING state as if it was the
> >> DEQUEUED state.
> >> 
> >> What do you think?
> > 
> > I'll have to review that in details given the potential complexity of
> > locking issues :-) I'm not opposed to the idea, if it works I believe we
> > should do it.
>
> Do you want to think about this first, or shall I make a new patch that you
> can then review?

As the devil is in the details I'd prefer a patch. I would have to write one 
to think about this anyway :-)

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 4/8] vb2: retry start_streaming in case of insufficient buffers.

[Laurent Pinchart]

Hi Hans,

Re-reading the code I can't see my original point anymore :-/ Let's assume I 
was just wrong. I have two additional small comments though, please see below.

On Wednesday 27 November 2013 08:17:15 Hans Verkuil wrote:
> On 11/26/2013 04:46 PM, Laurent Pinchart wrote:
> > On Friday 22 November 2013 09:43:23 Hans Verkuil wrote:
> >> On 11/21/2013 08:09 PM, Laurent Pinchart wrote:
> >>> On Thursday 21 November 2013 16:22:02 Hans Verkuil wrote:
> >>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> 
> >>>> If start_streaming returns -ENODATA, then it will be retried the next
> >>>> time a buffer is queued. This means applications no longer need to know
> >>>> how many buffers need to be queued before STREAMON can be called. This
> >>>> is particularly useful for output stream I/O.
> >>>> 
> >>>> If a DMA engine needs at least X buffers before it can start streaming,
> >>>> then for applications to get a buffer out as soon as possible they need
> >>>> to know the minimum number of buffers to queue before STREAMON can be
> >>>> called. You can't just try STREAMON after every buffer since on failure
> >>>> STREAMON will dequeue all your buffers. (Is that a bug or a feature?
> >>>> Frankly, I'm not sure).
> >>>> 
> >>>> This patch simplifies applications substantially: they can just call
> >>>> STREAMON at the beginning and then start queuing buffers and the DMA
> >>>> engine will kick in automagically once enough buffers are available.
> >>>> 
> >>>> This also fixes using write() to stream video: the fileio
> >>>> implementation calls streamon without having any queued buffers, which
> >>>> will fail today for any driver that requires a minimum number of
> >>>> buffers.
> >>>> 
> >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> ---
> >>>> 
> >>>>  drivers/media/v4l2-core/videobuf2-core.c | 66 +++++++++++++++++-------
> >>>>  include/media/videobuf2-core.h           | 15 ++++++--
> >>>>  2 files changed, 64 insertions(+), 17 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> >>>> b/drivers/media/v4l2-core/videobuf2-core.c index 9ea3ae9..5bb91f7
> >>>> 100644
> >>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
> >>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> >>>> @@ -1332,6 +1332,39 @@ int vb2_prepare_buf(struct vb2_queue *q, struct
> >>>> v4l2_buffer *b) }
> >>>> 
> >>>>  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
> >>>> + * true and 0 is returned. The next time a buffer is queued and
> >>>> + * retry_start_streaming is true, this function will be called again
> >>>> to
> >>>> + * retry starting the DMA engine.
> >>>> + */
> >>>> +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));
> >>>> +
> >>>> +	/*
> >>>> +	 * If there are not enough buffers queued to start streaming, then
> >>>> +	 * the start_streaming operation will return -ENODATA and you have to
> >>>> +	 * retry when the next buffer is queued.
> >>>> +	 */
> >>>> +	if (ret == -ENODATA) {
> >>>> +		dprintk(1, "qbuf: not enough buffers, retry when more buffers are
> >>>> queued.\n");
> >>>> +		q->retry_start_streaming = true;

retry_start_streaming is an unsigned int, should you use 1 and 0 here ?

> >>>> +		return 0;
> >>>> +	}
> >>>> +	if (ret)
> >>>> +		dprintk(1, "qbuf: driver refused to start streaming\n");
> >>>> +	else
> >>>> +		q->retry_start_streaming = false;
> >>>> +	return ret;
> >>>> +}
> >>>> +
> >>>>  static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer
> >>>>  *b)
> >>>>  {
> >>>>  	int ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> >>>> @@ -1377,6 +1410,12 @@ static int vb2_internal_qbuf(struct vb2_queue
> >>>> *q,
> >>>> struct v4l2_buffer *b) /* Fill buffer information for the userspace */
> >>>>  	__fill_v4l2_buffer(vb, b);
> >>>> 
> >>>> +	if (q->retry_start_streaming) {
> >>>> +		ret = vb2_start_streaming(q);
> >>>> +		if (ret)
> >>>> +			return ret;
> >>>> +	}
> >>>> +
> >>>>  	dprintk(1, "%s() of buffer %d succeeded\n", __func__, vb-
> >>>> v4l2_buf.index);
> >>>> return 0;
> >>>>  }
> >>>> 
> >>>> @@ -1526,7 +1565,8 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q)
> >>>>  		return -EINVAL;
> >>>>  	}
> >>>> 
> >>>> -	wait_event(q->done_wq, !atomic_read(&q->queued_count));
> >>>> +	if (!q->retry_start_streaming)
> >>>> +		wait_event(q->done_wq, !atomic_read(&q->queued_count));
> >>>>  	return 0;
> >>>>  }
> >>>>  EXPORT_SYMBOL_GPL(vb2_wait_for_all_buffers);
> >>>> @@ -1640,6 +1680,9 @@ static void __vb2_queue_cancel(struct vb2_queue
> >>>> *q)
> >>>>  {
> >>>>  	unsigned int i;
> >>>> 
> >>>> +	if (q->retry_start_streaming)
> >>>> +		q->retry_start_streaming = q->streaming = 0;

The kernel coding style discourages multiple assignments per line.

> >>>> +
> >>>>  	/*
> >>>>  	 * Tell driver to stop all transactions and release all queued
> >>>>  	 * buffers.
> >>>> @@ -1689,12 +1732,9 @@ static int vb2_internal_streamon(struct
> >>>> vb2_queue *q, enum v4l2_buf_type type)
> >>>>  list_for_each_entry(vb, &q->queued_list, queued_entry)
> >>>>  		__enqueue_in_driver(vb);
> >>>> 
> >>>> -	/*
> >>>> -	 * Let driver notice that streaming state has been enabled.
> >>>> -	 */
> >>>> -	ret = call_qop(q, start_streaming, q, atomic_read(&q->queued_count));
> >>>> +	/* Tell driver to start streaming. */
> >>> 
> >>> Wouldn't it be better to reset q->retry_start_streaming to 0 here
> >>> instead of in the several other locations ?
> >> 
> >> I don't follow. retry_start_streaming is set only in vb2_start_streaming
> >> or in queue_cancel. I'm not sure what vb2_internal_streamon has to do
> >> with this.
> > 
> > My point is that the code would be simpler and less error-prone if we
> > reset retry_start_streaming in a single location right before starting the
> > stream instead of in multiple locations when stopping the stream. There
> > would be less chances to introduce a bug when refactoring code in the
> > future in that case.
>
> But that's what happens: it's set when starting the stream
> (vb2_start_streaming) and cleared when stopping the stream
> (__vb2_queue_cancel). I really can't make it simpler than that.
> 
> I still don't see what it is you want to improve here.

-- 
Regards,

Laurent Pinchart