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

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

[Hans Verkuil]

This patch series does some cleanups in the qbuf/prepare_buf handling
(the first three patches). The fourth 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 patch adds a fix based on a patch from Andy that removes the
file I/O emulation assumption that buffers are dequeued in the same
order that they were enqueued.

With regards to patch 7/8 I would appreciate some Acks. See patch 5
how ENODATA is used to implement retrying start_streaming in vb2.

Regards,

        Hans

New in v4:

- Reorder the patches
- Drop the thread/DVB support: postpone until at least one driver will
  need this.

New in v3:

- Added a comment to the thread_start function making it explicit that
  it is for use with videobuf2-dvb only.
- Added patch 10/10 to address yet another race condition.

New in v2:

- Added a PREPARING state in patch 1 to prevent a race condition that Laurent
  mentioned (two QBUF calls with the same index number at the same time).
- Changed some minor issues in patch 4 that Laurent mentioned in his review.
- Added the reworked version of Andy's original patch to remove the order
  assumption in the file I/O emulation.

[RFCv4 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. The only way I can see that a race can happen is when two
threads queue the same buffer. The solution for that it to introduce
a PREPARING state.

Moving it down offers opportunities to simplify the code.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 82 ++++++++++++++------------------
 include/media/videobuf2-core.h           |  2 +
 2 files changed, 38 insertions(+), 46 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 57ba131..634dc95 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -462,6 +462,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 	case VB2_BUF_STATE_PREPARED:
 		b->flags |= V4L2_BUF_FLAG_PREPARED;
 		break;
+	case VB2_BUF_STATE_PREPARING:
 	case VB2_BUF_STATE_DEQUEUED:
 		/* nothing */
 		break;
@@ -1207,6 +1208,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);
@@ -1216,12 +1218,31 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		return ret;
 	}
 
+	vb->state = VB2_BUF_STATE_PREPARING;
 	switch (q->memory) {
 	case V4L2_MEMORY_MMAP:
 		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);
@@ -1235,8 +1256,7 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 		ret = call_qop(q, buf_prepare, vb);
 	if (ret)
 		dprintk(1, "qbuf: buffer preparation failed: %d\n", ret);
-	else
-		vb->state = VB2_BUF_STATE_PREPARED;
+	vb->state = ret ? VB2_BUF_STATE_DEQUEUED : VB2_BUF_STATE_PREPARED;
 
 	return ret;
 }
@@ -1247,80 +1267,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;
 }
 
@@ -1369,6 +1356,9 @@ static int __vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b,
 			return ret;
 	case VB2_BUF_STATE_PREPARED:
 		break;
+	case VB2_BUF_STATE_PREPARING:
+		dprintk(1, "qbuf: buffer still being prepared\n");
+		return -EINVAL;
 	default:
 		dprintk(1, "qbuf: buffer already in use\n");
 		return -EINVAL;
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 941055e..67972f6 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -142,6 +142,7 @@ enum vb2_fileio_flags {
 /**
  * enum vb2_buffer_state - current video buffer state
  * @VB2_BUF_STATE_DEQUEUED:	buffer under userspace control
+ * @VB2_BUF_STATE_PREPARING:	buffer is being prepared in videobuf
  * @VB2_BUF_STATE_PREPARED:	buffer prepared in videobuf and by the driver
  * @VB2_BUF_STATE_QUEUED:	buffer queued in videobuf, but not in driver
  * @VB2_BUF_STATE_ACTIVE:	buffer queued in driver and possibly used
@@ -154,6 +155,7 @@ enum vb2_fileio_flags {
  */
 enum vb2_buffer_state {
 	VB2_BUF_STATE_DEQUEUED,
+	VB2_BUF_STATE_PREPARING,
 	VB2_BUF_STATE_PREPARED,
 	VB2_BUF_STATE_QUEUED,
 	VB2_BUF_STATE_ACTIVE,
-- 
1.8.4.3

[RFCv4 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>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 634dc95..1754d3f 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1262,14 +1262,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;
@@ -1285,8 +1279,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;
@@ -1297,30 +1290,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);
 }
 
 /**
@@ -1340,20 +1310,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;
 	case VB2_BUF_STATE_PREPARING:
@@ -1378,29 +1396,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

[RFCv4 PATCH 3/8] vb2: fix race condition between REQBUFS and QBUF/PREPARE_BUF.

[Hans Verkuil]

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

When preparing a buffer the queue lock is released for a short while
if the memory mode is USERPTR (see __buf_prepare for the details), which
would allow a race with a REQBUFS which can free the buffers. Removing the
buffers from underneath __buf_prepare is obviously a bad idea, so we
check if any of the buffers is in the state PREPARING, and if so we
just return -EAGAIN.

If this happens, then the application does something really strange. The
REQBUFS call can be retried safely, since this situation is transient.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 1754d3f..4236ed9 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -279,10 +279,28 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers)
  * related information, if no buffers are left return the queue to an
  * uninitialized state. Might be called even if the queue has already been freed.
  */
-static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
+static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 {
 	unsigned int buffer;
 
+	/*
+	 * Sanity check: when preparing a buffer the queue lock is released for
+	 * a short while (see __buf_prepare for the details), which would allow
+	 * a race with a reqbufs which can call this function. Removing the
+	 * buffers from underneath __buf_prepare is obviously a bad idea, so we
+	 * check if any of the buffers is in the state PREPARING, and if so we
+	 * just return -EAGAIN.
+	 */
+	for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
+	     ++buffer) {
+		if (q->bufs[buffer] == NULL)
+			continue;
+		if (q->bufs[buffer]->state == VB2_BUF_STATE_PREPARING) {
+			dprintk(1, "reqbufs: preparing buffers, cannot free\n");
+			return -EAGAIN;
+		}
+	}
+
 	/* Call driver-provided cleanup function for each buffer, if provided */
 	if (q->ops->buf_cleanup) {
 		for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
@@ -307,6 +325,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 	if (!q->num_buffers)
 		q->memory = 0;
 	INIT_LIST_HEAD(&q->queued_list);
+	return 0;
 }
 
 /**
@@ -639,7 +658,9 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 			return -EBUSY;
 		}
 
-		__vb2_queue_free(q, q->num_buffers);
+		ret = __vb2_queue_free(q, q->num_buffers);
+		if (ret)
+			return ret;
 
 		/*
 		 * In case of REQBUFS(0) return immediately without calling
-- 
1.8.4.3

[RFCv4 PATCH 4/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 4236ed9..f0b3683 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1285,11 +1285,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;
@@ -1331,9 +1326,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;
 
@@ -1355,24 +1356,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;
@@ -1423,6 +1407,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);
 
 /**
@@ -1571,37 +1582,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;
@@ -1640,6 +1625,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);
 
 /**
@@ -1679,29 +1694,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;
@@ -1734,31 +1731,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;
@@ -1778,6 +1776,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);
 
 /**
@@ -2300,13 +2322,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);
@@ -2349,12 +2366,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];
 
@@ -2371,10 +2382,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;
 
 		/*
@@ -2404,8 +2415,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;
 	}
 
 	/*
@@ -2425,10 +2435,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);
 		}
 
@@ -2440,10 +2446,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
@@ -2462,9 +2468,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;
 		}
 	}
 
@@ -2473,11 +2479,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

[RFCv4 PATCH 5/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>
Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 68 ++++++++++++++++++++++++++------
 include/media/videobuf2-core.h           | 15 +++++--
 2 files changed, 66 insertions(+), 17 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index f0b3683..00a3f98 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1356,6 +1356,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
+ * 1 and 0 is returned. The next time a buffer is queued and
+ * retry_start_streaming is 1, this function will be called again to
+ * retry starting the DMA engine.
+ */
+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 = 1;
+		return 0;
+	}
+	if (ret)
+		dprintk(1, "qbuf: driver refused to start streaming\n");
+	else
+		q->retry_start_streaming = 0;
+	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");
@@ -1404,6 +1437,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;
 }
@@ -1553,7 +1592,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);
@@ -1667,6 +1707,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 {
 	unsigned int i;
 
+	if (q->retry_start_streaming) {
+		q->retry_start_streaming = 0;
+		q->streaming = 0;
+	}
+
 	/*
 	 * Tell driver to stop all transactions and release all queued
 	 * buffers.
@@ -1716,12 +1761,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;
 	}
@@ -2291,15 +2333,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 67972f6..d2a823e 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -252,10 +252,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()
@@ -323,6 +326,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 {
@@ -355,6 +361,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

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

[Guennadi Liakhovetski]

Hi Hans

On Mon, 9 Dec 2013, 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.

Wouldn't ENOBUFS be a better error code?

Thanks
Guennadi

> 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>
> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 68 ++++++++++++++++++++++++++------
>  include/media/videobuf2-core.h           | 15 +++++--
>  2 files changed, 66 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index f0b3683..00a3f98 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1356,6 +1356,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
> + * 1 and 0 is returned. The next time a buffer is queued and
> + * retry_start_streaming is 1, this function will be called again to
> + * retry starting the DMA engine.
> + */
> +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 = 1;
> +		return 0;
> +	}
> +	if (ret)
> +		dprintk(1, "qbuf: driver refused to start streaming\n");
> +	else
> +		q->retry_start_streaming = 0;
> +	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");
> @@ -1404,6 +1437,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;
>  }
> @@ -1553,7 +1592,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);
> @@ -1667,6 +1707,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>  {
>  	unsigned int i;
>  
> +	if (q->retry_start_streaming) {
> +		q->retry_start_streaming = 0;
> +		q->streaming = 0;
> +	}
> +
>  	/*
>  	 * Tell driver to stop all transactions and release all queued
>  	 * buffers.
> @@ -1716,12 +1761,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;
>  	}
> @@ -2291,15 +2333,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 67972f6..d2a823e 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -252,10 +252,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()
> @@ -323,6 +326,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 {
> @@ -355,6 +361,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
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

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

[Hans Verkuil]

On 12/10/2013 07:38 AM, Guennadi Liakhovetski wrote:
> Hi Hans
> 
> On Mon, 9 Dec 2013, 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.
> 
> Wouldn't ENOBUFS be a better error code?

Actually, it would. I wasn't aware that error code existed. Good idea, I'll change it.

But with that change made, can you ack patch 7/8?

Regards,

	Hans

> 
> Thanks
> Guennadi
> 
>> 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>
>> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 68 ++++++++++++++++++++++++++------
>>  include/media/videobuf2-core.h           | 15 +++++--
>>  2 files changed, 66 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index f0b3683..00a3f98 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -1356,6 +1356,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
>> + * 1 and 0 is returned. The next time a buffer is queued and
>> + * retry_start_streaming is 1, this function will be called again to
>> + * retry starting the DMA engine.
>> + */
>> +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 = 1;
>> +		return 0;
>> +	}
>> +	if (ret)
>> +		dprintk(1, "qbuf: driver refused to start streaming\n");
>> +	else
>> +		q->retry_start_streaming = 0;
>> +	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");
>> @@ -1404,6 +1437,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;
>>  }
>> @@ -1553,7 +1592,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);
>> @@ -1667,6 +1707,11 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>>  {
>>  	unsigned int i;
>>  
>> +	if (q->retry_start_streaming) {
>> +		q->retry_start_streaming = 0;
>> +		q->streaming = 0;
>> +	}
>> +
>>  	/*
>>  	 * Tell driver to stop all transactions and release all queued
>>  	 * buffers.
>> @@ -1716,12 +1761,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;
>>  	}
>> @@ -2291,15 +2333,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 67972f6..d2a823e 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -252,10 +252,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()
>> @@ -323,6 +326,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 {
>> @@ -355,6 +361,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
>>
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> 

[RFCv4 PATCH 6/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 00a3f98..a0b931e 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2423,7 +2423,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)
@@ -2505,15 +2504,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

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

[Hans Verkuil]

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

This works together with the retry_start_streaming mechanism to allow userspace
to start streaming even if not all required buffers have been queued.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com>
Cc: Tomasz Stanislawski <t.stanislaws@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Kamil Debski <k.debski@samsung.com>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 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 81b97db..220ec31 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

Re: [RFCv4 PATCH 7/8] vb2: return ENODATA in start_streaming in case of too few buffers.

[Guennadi Liakhovetski]

On Mon, 9 Dec 2013, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This works together with the retry_start_streaming mechanism to allow userspace
> to start streaming even if not all required buffers have been queued.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Cc: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Kamil Debski <k.debski@samsung.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---

[snip]

>  drivers/media/platform/soc_camera/mx2_camera.c  | 2 +-

Provided ENOBUFS is used instead of ENODATA:

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFCv4 PATCH 7/8] vb2: return ENODATA in start_streaming in case of too few buffers.

[Hans Verkuil]

As Guennadi mentioned in his review, ENODATA will be replaced by ENOBUFS, which is
more appropriate.

Prabhakar, Kamil, Tomasz, are you OK with this patch provided s/ENODATA/ENOBUFS/ ?

Regards,

	Hans

On 12/09/2013 02:43 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This works together with the retry_start_streaming mechanism to allow userspace
> to start streaming even if not all required buffers have been queued.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Cc: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Kamil Debski <k.debski@samsung.com>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  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 81b97db..220ec31 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;
> 

Re: [RFCv4 PATCH 7/8] vb2: return ENODATA in start_streaming in case of too few buffers.

[Prabhakar Lad]

Hi Hans,

On Tue, Dec 10, 2013 at 1:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> As Guennadi mentioned in his review, ENODATA will be replaced by ENOBUFS, which is
> more appropriate.
>
> Prabhakar, Kamil, Tomasz, are you OK with this patch provided s/ENODATA/ENOBUFS/ ?
>
+1 for ENOBUFS.

Regards,
--Prabhakar Lad

> Regards,
>
>         Hans
>
> On 12/09/2013 02:43 PM, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> This works together with the retry_start_streaming mechanism to allow userspace
>> to start streaming even if not all required buffers have been queued.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>> Cc: Tomasz Stanislawski <t.stanislaws@samsung.com>
>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Kamil Debski <k.debski@samsung.com>
>> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> ---
>>  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 81b97db..220ec31 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;
>>
>

Re: [RFCv4 PATCH 7/8] vb2: return ENODATA in start_streaming in case of too few buffers.

[Prabhakar Lad]

On Tue, Dec 10, 2013 at 3:26 PM, Prabhakar Lad
<prabhakar.csengg@gmail.com> wrote:
> Hi Hans,
>
> On Tue, Dec 10, 2013 at 1:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> As Guennadi mentioned in his review, ENODATA will be replaced by ENOBUFS, which is
>> more appropriate.
>>
>> Prabhakar, Kamil, Tomasz, are you OK with this patch provided s/ENODATA/ENOBUFS/ ?
>>
> +1 for ENOBUFS.
>
Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>

Regrads,
--Prabhakar Lad

RE: [RFCv4 PATCH 7/8] vb2: return ENODATA in start_streaming in case of too few buffers.

[Kamil Debski]

Hi,

> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> Sent: Tuesday, December 10, 2013 8:52 AM
> 
> As Guennadi mentioned in his review, ENODATA will be replaced by
> ENOBUFS, which is more appropriate.
> 
> Prabhakar, Kamil, Tomasz, are you OK with this patch provided
> s/ENODATA/ENOBUFS/ ?

The patch looks good. However, shouldn't the documentation be changed too?

Now it says: [1]
"(...) Accordingly the output hardware is disabled, no video signal is
produced until VIDIOC_STREAMON has been called. The ioctl will succeed
only when at least one output buffer is in the incoming queue. (...)"

If I understand correctly, now the ioctl will succeed with no buffers
queued. Apart from the above you have my ack.

Acked-by: Kamil Debski <k.debski@samsung.com>

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

[1] - http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-streamon.html

> 
> On 12/09/2013 02:43 PM, Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > This works together with the retry_start_streaming mechanism to allow
> > userspace to start streaming even if not all required buffers have
> been queued.
> >
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> > Cc: Tomasz Stanislawski <t.stanislaws@samsung.com>
> > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Kamil Debski <k.debski@samsung.com>
> > Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >  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 81b97db..220ec31 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;
> >

Re: [RFCv4 PATCH 7/8] vb2: return ENODATA in start_streaming in case of too few buffers.

[Hans Verkuil]

Hi Kamil,

On 12/11/13 11:27, Kamil Debski wrote:
> Hi,
> 
>> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
>> Sent: Tuesday, December 10, 2013 8:52 AM
>>
>> As Guennadi mentioned in his review, ENODATA will be replaced by
>> ENOBUFS, which is more appropriate.
>>
>> Prabhakar, Kamil, Tomasz, are you OK with this patch provided
>> s/ENODATA/ENOBUFS/ ?
> 
> The patch looks good. However, shouldn't the documentation be changed too?
> 
> Now it says: [1]
> "(...) Accordingly the output hardware is disabled, no video signal is
> produced until VIDIOC_STREAMON has been called. The ioctl will succeed
> only when at least one output buffer is in the incoming queue. (...)"
> 
> If I understand correctly, now the ioctl will succeed with no buffers
> queued.

That's true *only* for drivers using vb2. As long as not all drivers are
converted (which is a *very* long-term project) I don't think i can change
the documentation.

Regards,

	Hans

> Apart from the above you have my ack.
> 
> Acked-by: Kamil Debski <k.debski@samsung.com>
> 
> Best wishes,
> 

[RFCv4 PATCH 8/8] vb2: Improve file I/O emulation to handle buffers in any order

[Hans Verkuil]

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

videobuf2 file I/O emulation assumed that buffers dequeued from the
driver would return in the order they were enqueued in the driver.

Improve the file I/O emulator's book-keeping to remove this assumption.

Also set the buf->size properly if a write() dequeues a buffer and the
VB2_FILEIO_WRITE_IMMEDIATELY flag is set.

Based on an initial patch by Andy Walls.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index a0b931e..af89721 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -2333,6 +2333,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 				goto err_reqbufs;
 			fileio->bufs[i].queued = 1;
 		}
+		fileio->index = q->num_buffers;
 	}
 
 	/*
@@ -2408,15 +2409,11 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 	}
 	fileio = q->fileio;
 
-	index = fileio->index;
-	buf = &fileio->bufs[index];
-
 	/*
 	 * Check if we need to dequeue the buffer.
 	 */
-	if (buf->queued) {
-		struct vb2_buffer *vb;
-
+	index = fileio->index;
+	if (index >= q->num_buffers) {
 		/*
 		 * Call vb2_dqbuf to get buffer back.
 		 */
@@ -2429,12 +2426,18 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 			return ret;
 		fileio->dq_count += 1;
 
+		index = fileio->b.index;
+		buf = &fileio->bufs[index];
+
 		/*
 		 * Get number of bytes filled by the driver
 		 */
-		vb = q->bufs[index];
-		buf->size = vb2_get_plane_payload(vb, 0);
+		buf->pos = 0;
 		buf->queued = 0;
+		buf->size = read ? vb2_get_plane_payload(q->bufs[index], 0)
+				 : vb2_plane_size(q->bufs[index], 0);
+	} else {
+		buf = &fileio->bufs[index];
 	}
 
 	/*
@@ -2497,13 +2500,10 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 		 */
 		buf->pos = 0;
 		buf->queued = 1;
-		buf->size = q->bufs[0]->v4l2_planes[0].length;
+		buf->size = vb2_plane_size(q->bufs[index], 0);
 		fileio->q_count += 1;
-
-		/*
-		 * Switch to the next buffer
-		 */
-		fileio->index = (index + 1) % q->num_buffers;
+		if (fileio->index < q->num_buffers)
+			fileio->index++;
 	}
 
 	/*
-- 
1.8.4.3