[RFCv1 PATCH 0/6] vb2 & multiplanar fixes/changes

[RFCv1 PATCH 0/6] vb2 & multiplanar fixes/changes

[Hans Verkuil]

This RFC PATCH series is related to this email I sent:

http://www.mail-archive.com/linux-media@vger.kernel.org/msg52193.html

I've decided to fix the plane verification problem. This is done in the
first three patches. The fourth patch fills in the length field as I
proposed in the mail above.

The fifth fixes a bug for PREPARE_BUF in the multiplanar case that I came
across by accident.

The last patch updates the DocBook and clarifies a number of things.

Comments are welcome, in particular with regards to the fourth patch.

	Hans

[RFCv1 PATCH 1/6] videobuf2-core: move num_planes from vb2_buffer to vb2_queue.

[Hans Verkuil]

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

It's a queue-global value, so keep it there rather than with the
buffer struct.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 4da3df6..febc23b 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -54,7 +54,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
 	int plane;
 
 	/* Allocate memory for all planes in this buffer */
-	for (plane = 0; plane < vb->num_planes; ++plane) {
+	for (plane = 0; plane < q->num_planes; ++plane) {
 		mem_priv = call_memop(q, alloc, q->alloc_ctx[plane],
 				      q->plane_sizes[plane]);
 		if (IS_ERR_OR_NULL(mem_priv))
@@ -84,7 +84,7 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb)
 	struct vb2_queue *q = vb->vb2_queue;
 	unsigned int plane;
 
-	for (plane = 0; plane < vb->num_planes; ++plane) {
+	for (plane = 0; plane < q->num_planes; ++plane) {
 		call_memop(q, put, vb->planes[plane].mem_priv);
 		vb->planes[plane].mem_priv = NULL;
 		dprintk(3, "Freed plane %d of buffer %d\n", plane,
@@ -101,7 +101,7 @@ static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
 	struct vb2_queue *q = vb->vb2_queue;
 	unsigned int plane;
 
-	for (plane = 0; plane < vb->num_planes; ++plane) {
+	for (plane = 0; plane < q->num_planes; ++plane) {
 		if (vb->planes[plane].mem_priv)
 			call_memop(q, put_userptr, vb->planes[plane].mem_priv);
 		vb->planes[plane].mem_priv = NULL;
@@ -121,7 +121,7 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n)
 	if (q->num_buffers) {
 		struct v4l2_plane *p;
 		vb = q->bufs[q->num_buffers - 1];
-		p = &vb->v4l2_planes[vb->num_planes - 1];
+		p = &vb->v4l2_planes[q->num_planes - 1];
 		off = PAGE_ALIGN(p->m.mem_offset + p->length);
 	} else {
 		off = 0;
@@ -132,7 +132,7 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n)
 		if (!vb)
 			continue;
 
-		for (plane = 0; plane < vb->num_planes; ++plane) {
+		for (plane = 0; plane < q->num_planes; ++plane) {
 			vb->v4l2_planes[plane].length = q->plane_sizes[plane];
 			vb->v4l2_planes[plane].m.mem_offset = off;
 
@@ -173,7 +173,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
 
 		vb->state = VB2_BUF_STATE_DEQUEUED;
 		vb->vb2_queue = q;
-		vb->num_planes = num_planes;
+		q->num_planes = num_planes;
 		vb->v4l2_buf.index = q->num_buffers + buffer;
 		vb->v4l2_buf.type = q->type;
 		vb->v4l2_buf.memory = memory;
@@ -276,6 +276,8 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
  */
 static int __verify_planes_array(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 {
+	struct vb2_queue *q = vb->vb2_queue;
+
 	/* Is memory for copying plane information present? */
 	if (NULL == b->m.planes) {
 		dprintk(1, "Multi-planar buffer passed but "
@@ -283,9 +285,9 @@ static int __verify_planes_array(struct vb2_buffer *vb, const struct v4l2_buffer
 		return -EINVAL;
 	}
 
-	if (b->length < vb->num_planes || b->length > VIDEO_MAX_PLANES) {
+	if (b->length < q->num_planes || b->length > VIDEO_MAX_PLANES) {
 		dprintk(1, "Incorrect planes array length, "
-			   "expected %d, got %d\n", vb->num_planes, b->length);
+			   "expected %d, got %d\n", q->num_planes, b->length);
 		return -EINVAL;
 	}
 
@@ -299,7 +301,7 @@ static int __verify_planes_array(struct vb2_buffer *vb, const struct v4l2_buffer
 static bool __buffer_in_use(struct vb2_queue *q, struct vb2_buffer *vb)
 {
 	unsigned int plane;
-	for (plane = 0; plane < vb->num_planes; ++plane) {
+	for (plane = 0; plane < q->num_planes; ++plane) {
 		void *mem_priv = vb->planes[plane].mem_priv;
 		/*
 		 * If num_users() has not been provided, call_memop
@@ -744,7 +746,7 @@ void *vb2_plane_vaddr(struct vb2_buffer *vb, unsigned int plane_no)
 {
 	struct vb2_queue *q = vb->vb2_queue;
 
-	if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
+	if (plane_no > q->num_planes || !vb->planes[plane_no].mem_priv)
 		return NULL;
 
 	return call_memop(q, vaddr, vb->planes[plane_no].mem_priv);
@@ -767,7 +769,7 @@ void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int plane_no)
 {
 	struct vb2_queue *q = vb->vb2_queue;
 
-	if (plane_no > vb->num_planes || !vb->planes[plane_no].mem_priv)
+	if (plane_no > q->num_planes || !vb->planes[plane_no].mem_priv)
 		return NULL;
 
 	return call_memop(q, cookie, vb->planes[plane_no].mem_priv);
@@ -823,6 +825,8 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b,
 	int ret;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
+		struct vb2_queue *q = vb->vb2_queue;
+
 		/*
 		 * Verify that the userspace gave us a valid array for
 		 * plane information.
@@ -837,7 +841,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b,
 			 * Will have to go up to b->length when API starts
 			 * accepting variable number of planes.
 			 */
-			for (plane = 0; plane < vb->num_planes; ++plane) {
+			for (plane = 0; plane < q->num_planes; ++plane) {
 				v4l2_planes[plane].bytesused =
 					b->m.planes[plane].bytesused;
 				v4l2_planes[plane].data_offset =
@@ -846,7 +850,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b,
 		}
 
 		if (b->memory == V4L2_MEMORY_USERPTR) {
-			for (plane = 0; plane < vb->num_planes; ++plane) {
+			for (plane = 0; plane < q->num_planes; ++plane) {
 				v4l2_planes[plane].m.userptr =
 					b->m.planes[plane].m.userptr;
 				v4l2_planes[plane].length =
@@ -893,7 +897,7 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 	if (ret)
 		return ret;
 
-	for (plane = 0; plane < vb->num_planes; ++plane) {
+	for (plane = 0; plane < q->num_planes; ++plane) {
 		/* Skip the plane if already verified */
 		if (vb->v4l2_planes[plane].m.userptr &&
 		    vb->v4l2_planes[plane].m.userptr == planes[plane].m.userptr
@@ -944,13 +948,13 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b)
 	 * Now that everything is in order, copy relevant information
 	 * provided by userspace.
 	 */
-	for (plane = 0; plane < vb->num_planes; ++plane)
+	for (plane = 0; plane < q->num_planes; ++plane)
 		vb->v4l2_planes[plane] = planes[plane];
 
 	return 0;
 err:
 	/* In case of errors, release planes that were already acquired */
-	for (plane = 0; plane < vb->num_planes; ++plane) {
+	for (plane = 0; plane < q->num_planes; ++plane) {
 		if (vb->planes[plane].mem_priv)
 			call_memop(q, put_userptr, vb->planes[plane].mem_priv);
 		vb->planes[plane].mem_priv = NULL;
@@ -1528,7 +1532,7 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
 	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
 		vb = q->bufs[buffer];
 
-		for (plane = 0; plane < vb->num_planes; ++plane) {
+		for (plane = 0; plane < q->num_planes; ++plane) {
 			if (vb->v4l2_planes[plane].m.mem_offset == off) {
 				*_buffer = buffer;
 				*_plane = plane;
@@ -1866,7 +1870,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 	 * Check if plane_count is correct
 	 * (multiplane buffers are not supported).
 	 */
-	if (q->bufs[0]->num_planes != 1) {
+	if (q->num_planes != 1) {
 		ret = -EBUSY;
 		goto err_reqbufs;
 	}
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 8dd9b6c..45f6e1c 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -140,8 +140,6 @@ struct vb2_queue;
  *			instead of v4l2_buf for filling bytesused - drivers
  *			should use the vb2_set_plane_payload() function for that
  * @vb2_queue:		the queue to which this driver belongs
- * @num_planes:		number of planes in the buffer
- *			on an internal driver queue
  * @state:		current buffer state; do not change
  * @queued_entry:	entry on the queued buffers list, which holds all
  *			buffers queued from userspace
@@ -155,8 +153,6 @@ struct vb2_buffer {
 
 	struct vb2_queue	*vb2_queue;
 
-	unsigned int		num_planes;
-
 /* Private: internal use only */
 	enum vb2_buffer_state	state;
 
@@ -271,6 +267,7 @@ struct v4l2_fh;
  * @memory:	current memory type used
  * @bufs:	videobuf buffer structures
  * @num_buffers: number of allocated/used buffers
+ * @num_planes:	number of planes in the buffers
  * @queued_list: list of buffers currently queued from userspace
  * @queued_count: number of buffers owned by the driver
  * @done_list:	list of buffers ready to be dequeued to userspace
@@ -296,6 +293,7 @@ struct vb2_queue {
 	enum v4l2_memory		memory;
 	struct vb2_buffer		*bufs[VIDEO_MAX_FRAME];
 	unsigned int			num_buffers;
+	unsigned int			num_planes;
 
 	struct list_head		queued_list;
 
@@ -386,7 +384,7 @@ static inline void *vb2_get_drv_priv(struct vb2_queue *q)
 static inline void vb2_set_plane_payload(struct vb2_buffer *vb,
 				 unsigned int plane_no, unsigned long size)
 {
-	if (plane_no < vb->num_planes)
+	if (plane_no < vb->vb2_queue->num_planes)
 		vb->v4l2_planes[plane_no].bytesused = size;
 }
 
@@ -399,7 +397,7 @@ static inline void vb2_set_plane_payload(struct vb2_buffer *vb,
 static inline unsigned long vb2_get_plane_payload(struct vb2_buffer *vb,
 				 unsigned int plane_no)
 {
-	if (plane_no < vb->num_planes)
+	if (plane_no < vb->vb2_queue->num_planes)
 		return vb->v4l2_planes[plane_no].bytesused;
 	return 0;
 }
@@ -412,7 +410,7 @@ static inline unsigned long vb2_get_plane_payload(struct vb2_buffer *vb,
 static inline unsigned long
 vb2_plane_size(struct vb2_buffer *vb, unsigned int plane_no)
 {
-	if (plane_no < vb->num_planes)
+	if (plane_no < vb->vb2_queue->num_planes)
 		return vb->v4l2_planes[plane_no].length;
 	return 0;
 }
-- 
1.7.10.4

[RFCv1 PATCH 2/6] videobuf2-core: use vb2_queue in __verify_planes_array

[Hans Verkuil]

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

Since num_planes has been moved to vb2_queue, the __verify_planes_array()
function can now switch to a vb2_queue argument as well.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index febc23b..2e26e58 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -274,10 +274,8 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
  * __verify_planes_array() - verify that the planes array passed in struct
  * v4l2_buffer from userspace can be safely used
  */
-static int __verify_planes_array(struct vb2_buffer *vb, const struct v4l2_buffer *b)
+static int __verify_planes_array(struct vb2_queue *q, const struct v4l2_buffer *b)
 {
-	struct vb2_queue *q = vb->vb2_queue;
-
 	/* Is memory for copying plane information present? */
 	if (NULL == b->m.planes) {
 		dprintk(1, "Multi-planar buffer passed but "
@@ -344,7 +342,7 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 	b->reserved = vb->v4l2_buf.reserved;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
-		ret = __verify_planes_array(vb, b);
+		ret = __verify_planes_array(q, b);
 		if (ret)
 			return ret;
 
@@ -831,7 +829,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b,
 		 * Verify that the userspace gave us a valid array for
 		 * plane information.
 		 */
-		ret = __verify_planes_array(vb, b);
+		ret = __verify_planes_array(q, b);
 		if (ret)
 			return ret;
 
-- 
1.7.10.4

[RFCv1 PATCH 3/6] videobuf2-core: move plane verification out of __fill_v4l2_buffer.

[Hans Verkuil]

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

The plane verification should be done before actually queuing or
dequeuing buffers, so move it out of __fill_v4l2_buffer and call it
as a separate step.

The also makes it possible to change the return type of __fill_v4l2_buffer
to void.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 2e26e58..929cc99 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -276,6 +276,9 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
  */
 static int __verify_planes_array(struct vb2_queue *q, const struct v4l2_buffer *b)
 {
+	if (!V4L2_TYPE_IS_MULTIPLANAR(b->type))
+		return 0;
+
 	/* Is memory for copying plane information present? */
 	if (NULL == b->m.planes) {
 		dprintk(1, "Multi-planar buffer passed but "
@@ -331,10 +334,9 @@ static bool __buffers_in_use(struct vb2_queue *q)
  * __fill_v4l2_buffer() - fill in a struct v4l2_buffer with information to be
  * returned to userspace
  */
-static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
+static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 {
 	struct vb2_queue *q = vb->vb2_queue;
-	int ret;
 
 	/* Copy back data such as timestamp, flags, etc. */
 	memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
@@ -342,10 +344,6 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 	b->reserved = vb->v4l2_buf.reserved;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
-		ret = __verify_planes_array(q, b);
-		if (ret)
-			return ret;
-
 		/*
 		 * Fill in plane-related data if userspace provided an array
 		 * for it. The memory and size is verified above.
@@ -391,8 +389,6 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 
 	if (__buffer_in_use(q, vb))
 		b->flags |= V4L2_BUF_FLAG_MAPPED;
-
-	return 0;
 }
 
 /**
@@ -411,6 +407,7 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
 {
 	struct vb2_buffer *vb;
+	int ret;
 
 	if (b->type != q->type) {
 		dprintk(1, "querybuf: wrong buffer type\n");
@@ -422,8 +419,10 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
 		return -EINVAL;
 	}
 	vb = q->bufs[b->index];
-
-	return __fill_v4l2_buffer(vb, b);
+	ret = __verify_planes_array(q, b);
+	if (!ret)
+		__fill_v4l2_buffer(vb, b);
+	return ret;
 }
 EXPORT_SYMBOL(vb2_querybuf);
 
@@ -1061,8 +1060,8 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
 		dprintk(1, "%s(): invalid buffer state %d\n", __func__, vb->state);
 		return -EINVAL;
 	}
-
-	ret = __buf_prepare(vb, b);
+	ret = __verify_planes_array(q, b);
+	ret = ret ? ret : __buf_prepare(vb, b);
 	if (ret < 0)
 		return ret;
 
@@ -1149,6 +1148,9 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 		ret = -EINVAL;
 		goto unlock;
 	}
+	ret = __verify_planes_array(q, b);
+	if (ret)
+		return ret;
 
 	switch (vb->state) {
 	case VB2_BUF_STATE_DEQUEUED:
@@ -1337,6 +1339,9 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
 		dprintk(1, "dqbuf: invalid buffer type\n");
 		return -EINVAL;
 	}
+	ret = __verify_planes_array(q, b);
+	if (ret)
+		return ret;
 
 	ret = __vb2_get_done_vb(q, &vb, nonblocking);
 	if (ret < 0) {
-- 
1.7.10.4

[RFCv1 PATCH 4/6] videobuf2-core: fill in length field for multiplanar buffers.

[Hans Verkuil]

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

length should be set to num_planes in __fill_v4l2_buffer(). That way the
caller knows how many planes there are in the buffer.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 929cc99..bbfe022 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -348,6 +348,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 		 * Fill in plane-related data if userspace provided an array
 		 * for it. The memory and size is verified above.
 		 */
+		b->length = q->num_planes;
 		memcpy(b->m.planes, vb->v4l2_planes,
 			b->length * sizeof(struct v4l2_plane));
 	} else {
-- 
1.7.10.4

[RFCv1 PATCH 5/6] v4l2-ioctl,c: handle PREPARE_BUF like QUERYBUF.

[Hans Verkuil]

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

The core code for PREPARE_BUF didn't take the multiplanar case into account,
which might cause page faults. Handle PREPARE_BUF just like QUERYBUF and
QBUF/DQBUF.

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

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 932d9bf..bbbda4e 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2173,6 +2173,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
 	int ret = 0;
 
 	switch (cmd) {
+	case VIDIOC_PREPARE_BUF:
 	case VIDIOC_QUERYBUF:
 	case VIDIOC_QBUF:
 	case VIDIOC_DQBUF: {
-- 
1.7.10.4

[RFCv1 PATCH 6/6] DocBook: various updates w.r.t. v4l2_buffer and multiplanar.

[Hans Verkuil]

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

Clarify the behavior of v4l2_buffer in the multiplanar case,
including fixing a false statement: you can't set m.planes to NULL
when calling DQBUF.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/DocBook/media/v4l/io.xml              |    6 ++++--
 Documentation/DocBook/media/v4l/vidioc-qbuf.xml     |    3 +--
 Documentation/DocBook/media/v4l/vidioc-querybuf.xml |   11 +++++++----
 3 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
index 1885cc0..c6d39fe 100644
--- a/Documentation/DocBook/media/v4l/io.xml
+++ b/Documentation/DocBook/media/v4l/io.xml
@@ -677,8 +677,10 @@ memory, set by the application. See <xref linkend="userp" /> for details.
 	    <entry><structfield>length</structfield></entry>
 	    <entry></entry>
 	    <entry>Size of the buffer (not the payload) in bytes for the
-	    single-planar API. For the multi-planar API should contain the
-	    number of elements in the <structfield>planes</structfield> array.
+	    single-planar API. For the multi-planar API the application sets
+	    this to the number of elements in the <structfield>planes</structfield>
+	    array. The driver will fill in the actual number of valid elements in
+	    that array.
 	    </entry>
 	  </row>
 	  <row>
diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
index 6a821a6..2d37abe 100644
--- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
@@ -121,8 +121,7 @@ remaining fields or returns an error code. The driver may also set
 field. It indicates a non-critical (recoverable) streaming error. In such case
 the application may continue as normal, but should be aware that data in the
 dequeued buffer might be corrupted. When using the multi-planar API, the
-planes array does not have to be passed; the <structfield>m.planes</structfield>
-member must be set to NULL in that case.</para>
+planes array must be passed in as well.</para>
 
     <para>By default <constant>VIDIOC_DQBUF</constant> blocks when no
 buffer is in the outgoing queue. When the
diff --git a/Documentation/DocBook/media/v4l/vidioc-querybuf.xml b/Documentation/DocBook/media/v4l/vidioc-querybuf.xml
index 6e414d7..a597155 100644
--- a/Documentation/DocBook/media/v4l/vidioc-querybuf.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-querybuf.xml
@@ -48,8 +48,8 @@
   <refsect1>
     <title>Description</title>
 
-    <para>This ioctl is part of the <link linkend="mmap">memory
-mapping</link> I/O method. It can be used to query the status of a
+    <para>This ioctl is part of the <link linkend="mmap">streaming
+</link> I/O method. It can be used to query the status of a
 buffer at any time after buffers have been allocated with the
 &VIDIOC-REQBUFS; ioctl.</para>
 
@@ -71,6 +71,7 @@ the structure.</para>
 
     <para>In the <structfield>flags</structfield> field the
 <constant>V4L2_BUF_FLAG_MAPPED</constant>,
+<constant>V4L2_BUF_FLAG_PREPARED</constant>,
 <constant>V4L2_BUF_FLAG_QUEUED</constant> and
 <constant>V4L2_BUF_FLAG_DONE</constant> flags will be valid. The
 <structfield>memory</structfield> field will be set to the current
@@ -79,8 +80,10 @@ contains the offset of the buffer from the start of the device memory,
 the <structfield>length</structfield> field its size. For the multi-planar API,
 fields <structfield>m.mem_offset</structfield> and
 <structfield>length</structfield> in the <structfield>m.planes</structfield>
-array elements will be used instead. The driver may or may not set the remaining
-fields and flags, they are meaningless in this context.</para>
+array elements will be used instead and the <structfield>length</structfield>
+field of &v4l2-buffer; is set to the number of filled-in array elements.
+The driver may or may not set the remaining fields and flags, they are
+meaningless in this context.</para>
 
     <para>The <structname>v4l2_buffer</structname> structure is
     specified in <xref linkend="buffer" />.</para>
-- 
1.7.10.4

Re: [RFCv1 PATCH 1/6] videobuf2-core: move num_planes from vb2_buffer to vb2_queue.

[Sylwester Nawrocki]

Hi Hans,

On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> It's a queue-global value, so keep it there rather than with the
> buffer struct.

I would prefer not doing this. It makes the path to variable
number of per buffer planes more difficult.


Regards,
Sylwester

Re: [RFCv1 PATCH 1/6] videobuf2-core: move num_planes from vb2_buffer to vb2_queue.

[Hans Verkuil]

On Wed September 19 2012 17:18:11 Sylwester Nawrocki wrote:
> Hi Hans,
> 
> On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > It's a queue-global value, so keep it there rather than with the
> > buffer struct.
> 
> I would prefer not doing this. It makes the path to variable
> number of per buffer planes more difficult.

You can't have a variable number of planes per buffer. You can decide not to
fill certain planes (e.g. set bytesused to 0 or something), but that's a
different thing.

So applications will always need to set up q->num_planes elements of the array.
And in the MMAP case all planes need to be mmap()ed. You can't have one buffer
that's setup with only 2 planes while all others are setup with 3 planes.

Regards,

	Hans

Re: [RFCv1 PATCH 1/6] videobuf2-core: move num_planes from vb2_buffer to vb2_queue.

[Sylwester Nawrocki]

On 09/19/2012 05:28 PM, Hans Verkuil wrote:
> On Wed September 19 2012 17:18:11 Sylwester Nawrocki wrote:
>> Hi Hans,
>>
>> On 09/19/2012 04:37 PM, Hans Verkuil wrote:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> It's a queue-global value, so keep it there rather than with the
>>> buffer struct.
>>
>> I would prefer not doing this. It makes the path to variable
>> number of per buffer planes more difficult.
> 
> You can't have a variable number of planes per buffer. You can decide not to
> fill certain planes (e.g. set bytesused to 0 or something), but that's a
> different thing.
> 
> So applications will always need to set up q->num_planes elements of the array.
> And in the MMAP case all planes need to be mmap()ed. You can't have one buffer
> that's setup with only 2 planes while all others are setup with 3 planes.

You're right, all planes would need to be prepared anyway. Reporting that
some planes are unused by setting bytesused is probably going to be enough.
And your subsequent patches depend on this one. FWIW,

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

--

Regards,
Sylwester

Re: [RFCv1 PATCH 2/6] videobuf2-core: use vb2_queue in __verify_planes_array

[Sylwester Nawrocki]

On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Since num_planes has been moved to vb2_queue, the __verify_planes_array()
> function can now switch to a vb2_queue argument as well.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

Re: [RFCv1 PATCH 3/6] videobuf2-core: move plane verification out of __fill_v4l2_buffer.

[Sylwester Nawrocki]

On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The plane verification should be done before actually queuing or
> dequeuing buffers, so move it out of __fill_v4l2_buffer and call it
> as a separate step.
> 
> The also makes it possible to change the return type of __fill_v4l2_buffer
> to void.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

There are just two small comment below...

> ---
>  drivers/media/v4l2-core/videobuf2-core.c |   29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 2e26e58..929cc99 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -276,6 +276,9 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>   */
>  static int __verify_planes_array(struct vb2_queue *q, const struct v4l2_buffer *b)
>  {
> +	if (!V4L2_TYPE_IS_MULTIPLANAR(b->type))
> +		return 0;
> +
>  	/* Is memory for copying plane information present? */
>  	if (NULL == b->m.planes) {
>  		dprintk(1, "Multi-planar buffer passed but "
> @@ -331,10 +334,9 @@ static bool __buffers_in_use(struct vb2_queue *q)
>   * __fill_v4l2_buffer() - fill in a struct v4l2_buffer with information to be
>   * returned to userspace
>   */
> -static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> +static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  {
>  	struct vb2_queue *q = vb->vb2_queue;
> -	int ret;
>  
>  	/* Copy back data such as timestamp, flags, etc. */
>  	memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
> @@ -342,10 +344,6 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  	b->reserved = vb->v4l2_buf.reserved;
>  
>  	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
> -		ret = __verify_planes_array(q, b);
> -		if (ret)
> -			return ret;
> -
>  		/*
>  		 * Fill in plane-related data if userspace provided an array
>  		 * for it. The memory and size is verified above.

This comment should be updated, since __verify_planes_array() is now removed.

> @@ -391,8 +389,6 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  
>  	if (__buffer_in_use(q, vb))
>  		b->flags |= V4L2_BUF_FLAG_MAPPED;
> -
> -	return 0;
>  }
>  
>  /**
> @@ -411,6 +407,7 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  {
>  	struct vb2_buffer *vb;
> +	int ret;
>  
>  	if (b->type != q->type) {
>  		dprintk(1, "querybuf: wrong buffer type\n");
> @@ -422,8 +419,10 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  		return -EINVAL;
>  	}
>  	vb = q->bufs[b->index];
> -
> -	return __fill_v4l2_buffer(vb, b);
> +	ret = __verify_planes_array(q, b);
> +	if (!ret)
> +		__fill_v4l2_buffer(vb, b);
> +	return ret;
>  }
>  EXPORT_SYMBOL(vb2_querybuf);
>  
> @@ -1061,8 +1060,8 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
>  		dprintk(1, "%s(): invalid buffer state %d\n", __func__, vb->state);
>  		return -EINVAL;
>  	}
> -
> -	ret = __buf_prepare(vb, b);
> +	ret = __verify_planes_array(q, b);
> +	ret = ret ? ret : __buf_prepare(vb, b);

Could we just make it:

	ret = __verify_planes_array(q, b);
  	if (ret < 0)
  		return ret;

	ret = __buf_prepare(vb, b);
  	if (ret < 0)
  		return ret;

?
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1149,6 +1148,9 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  		ret = -EINVAL;
>  		goto unlock;
>  	}
> +	ret = __verify_planes_array(q, b);
> +	if (ret)
> +		return ret;
>  
>  	switch (vb->state) {
>  	case VB2_BUF_STATE_DEQUEUED:
> @@ -1337,6 +1339,9 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
>  		dprintk(1, "dqbuf: invalid buffer type\n");
>  		return -EINVAL;
>  	}
> +	ret = __verify_planes_array(q, b);
> +	if (ret)
> +		return ret;
>  
>  	ret = __vb2_get_done_vb(q, &vb, nonblocking);
>  	if (ret < 0) {

--

Regards,
Sylwester

Re: [RFCv1 PATCH 3/6] videobuf2-core: move plane verification out of __fill_v4l2_buffer.

[Hans Verkuil]

On Wed September 19 2012 18:55:25 Sylwester Nawrocki wrote:
> On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > The plane verification should be done before actually queuing or
> > dequeuing buffers, so move it out of __fill_v4l2_buffer and call it
> > as a separate step.
> > 
> > The also makes it possible to change the return type of __fill_v4l2_buffer
> > to void.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Tested-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> 
> There are just two small comment below...
> 
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c |   29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index 2e26e58..929cc99 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -276,6 +276,9 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> >   */
> >  static int __verify_planes_array(struct vb2_queue *q, const struct v4l2_buffer *b)
> >  {
> > +	if (!V4L2_TYPE_IS_MULTIPLANAR(b->type))
> > +		return 0;
> > +
> >  	/* Is memory for copying plane information present? */
> >  	if (NULL == b->m.planes) {
> >  		dprintk(1, "Multi-planar buffer passed but "
> > @@ -331,10 +334,9 @@ static bool __buffers_in_use(struct vb2_queue *q)
> >   * __fill_v4l2_buffer() - fill in a struct v4l2_buffer with information to be
> >   * returned to userspace
> >   */
> > -static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> > +static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >  {
> >  	struct vb2_queue *q = vb->vb2_queue;
> > -	int ret;
> >  
> >  	/* Copy back data such as timestamp, flags, etc. */
> >  	memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m));
> > @@ -342,10 +344,6 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >  	b->reserved = vb->v4l2_buf.reserved;
> >  
> >  	if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) {
> > -		ret = __verify_planes_array(q, b);
> > -		if (ret)
> > -			return ret;
> > -
> >  		/*
> >  		 * Fill in plane-related data if userspace provided an array
> >  		 * for it. The memory and size is verified above.
> 
> This comment should be updated, since __verify_planes_array() is now removed.

Will do.

> > @@ -391,8 +389,6 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >  
> >  	if (__buffer_in_use(q, vb))
> >  		b->flags |= V4L2_BUF_FLAG_MAPPED;
> > -
> > -	return 0;
> >  }
> >  
> >  /**
> > @@ -411,6 +407,7 @@ static int __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  {
> >  	struct vb2_buffer *vb;
> > +	int ret;
> >  
> >  	if (b->type != q->type) {
> >  		dprintk(1, "querybuf: wrong buffer type\n");
> > @@ -422,8 +419,10 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  		return -EINVAL;
> >  	}
> >  	vb = q->bufs[b->index];
> > -
> > -	return __fill_v4l2_buffer(vb, b);
> > +	ret = __verify_planes_array(q, b);
> > +	if (!ret)
> > +		__fill_v4l2_buffer(vb, b);
> > +	return ret;
> >  }
> >  EXPORT_SYMBOL(vb2_querybuf);
> >  
> > @@ -1061,8 +1060,8 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  		dprintk(1, "%s(): invalid buffer state %d\n", __func__, vb->state);
> >  		return -EINVAL;
> >  	}
> > -
> > -	ret = __buf_prepare(vb, b);
> > +	ret = __verify_planes_array(q, b);
> > +	ret = ret ? ret : __buf_prepare(vb, b);
> 
> Could we just make it:
> 
> 	ret = __verify_planes_array(q, b);
>   	if (ret < 0)
>   		return ret;
> 
> 	ret = __buf_prepare(vb, b);
>   	if (ret < 0)
>   		return ret;
> 
> ?

OK.

Regards,

	Hans

> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -1149,6 +1148,9 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> >  		ret = -EINVAL;
> >  		goto unlock;
> >  	}
> > +	ret = __verify_planes_array(q, b);
> > +	if (ret)
> > +		return ret;
> >  
> >  	switch (vb->state) {
> >  	case VB2_BUF_STATE_DEQUEUED:
> > @@ -1337,6 +1339,9 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking)
> >  		dprintk(1, "dqbuf: invalid buffer type\n");
> >  		return -EINVAL;
> >  	}
> > +	ret = __verify_planes_array(q, b);
> > +	if (ret)
> > +		return ret;
> >  
> >  	ret = __vb2_get_done_vb(q, &vb, nonblocking);
> >  	if (ret < 0) {
> 
> --
> 
> Regards,
> Sylwester
> 

Re: [RFCv1 PATCH 4/6] videobuf2-core: fill in length field for multiplanar buffers.

[Hans Verkuil]

On Wed September 19 2012 16:37:38 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> length should be set to num_planes in __fill_v4l2_buffer(). That way the
> caller knows how many planes there are in the buffer.

Can someone review this? I'd like to know whether it will cause problems
for existing applications to set the length back to the actual number of
planes, and whether it makes sense at all to do so. I believe it does, but
I don't know if anyone is using the current behavior.

Note that the documentation currently doesn't specify what will happen with
length.

Since the only drivers implementing multiplanar support are Samsung drivers,
I assume that Samsung will know best whether this change might cause problems.

Regards,

	Hans

> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 929cc99..bbfe022 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -348,6 +348,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  		 * Fill in plane-related data if userspace provided an array
>  		 * for it. The memory and size is verified above.
>  		 */
> +		b->length = q->num_planes;
>  		memcpy(b->m.planes, vb->v4l2_planes,
>  			b->length * sizeof(struct v4l2_plane));
>  	} else {
> 

Re: [RFCv1 PATCH 4/6] videobuf2-core: fill in length field for multiplanar buffers.

[Sylwester Nawrocki]

Hi Hans,

On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> length should be set to num_planes in __fill_v4l2_buffer(). That way the
> caller knows how many planes there are in the buffer.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

I think this would break VIDIOC_CREATE_BUFS. We need per buffer num_planes.
Consider a use case where device is streaming with 2-planar pixel format
and we invoke VIDIOC_CREATE_BUFS with single-planar format. On a single 
queue there will be buffers with different number of planes. The number of 
planes information must be attached to a buffer, otherwise VIDIOC_QUERYBUF 
won't work.

> ---
>  drivers/media/v4l2-core/videobuf2-core.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 929cc99..bbfe022 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -348,6 +348,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>  		 * Fill in plane-related data if userspace provided an array
>  		 * for it. The memory and size is verified above.
>  		 */
> +		b->length = q->num_planes;
>  		memcpy(b->m.planes, vb->v4l2_planes,
>  			b->length * sizeof(struct v4l2_plane));
>  	} else {

Regards,
Sylwester

Re: [RFCv1 PATCH 4/6] videobuf2-core: fill in length field for multiplanar buffers.

[Hans Verkuil]

On Fri September 21 2012 18:13:20 Sylwester Nawrocki wrote:
> Hi Hans,
> 
> On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > length should be set to num_planes in __fill_v4l2_buffer(). That way the
> > caller knows how many planes there are in the buffer.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> I think this would break VIDIOC_CREATE_BUFS. We need per buffer num_planes.
> Consider a use case where device is streaming with 2-planar pixel format
> and we invoke VIDIOC_CREATE_BUFS with single-planar format. On a single 
> queue there will be buffers with different number of planes. The number of 
> planes information must be attached to a buffer, otherwise VIDIOC_QUERYBUF 
> won't work.

That's a very good point and one I need to meditate on.

However, your comment applies to patch 1/6, not to this one.
This patch is about whether or not the length field of v4l2_buffer should
be filled in with the actual number of planes used by that buffer or not.

Regards,

	Hans

> 
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index 929cc99..bbfe022 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -348,6 +348,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
> >  		 * Fill in plane-related data if userspace provided an array
> >  		 * for it. The memory and size is verified above.
> >  		 */
> > +		b->length = q->num_planes;
> >  		memcpy(b->m.planes, vb->v4l2_planes,
> >  			b->length * sizeof(struct v4l2_plane));
> >  	} else {
> 
> Regards,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFCv1 PATCH 6/6] DocBook: various updates w.r.t. v4l2_buffer and multiplanar.

[Sylwester Nawrocki]

On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Clarify the behavior of v4l2_buffer in the multiplanar case,
> including fixing a false statement: you can't set m.planes to NULL
> when calling DQBUF.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

I'm not sure what was main argument for not requiring applications to 
pass the planes array in to VIDIOC_DQBUF. Maybe, to avoid copying the 
planes array so performance is improved when applications don't need 
all information from struct v4l2_plane after each DQBUF. So I'm not 
sure about DQBUF. But improving querybuf sounds like a reasonable thing 
to do. So far num_planes have had to be passed in by applications, as 
returned from S_FMT/G_FMT. There seems to be no way to verify how many
planes are valid based on VIDIOC_QUERYBUF output.

Anyway, both changes shouldn't be a big deal for existing applications. 
They should just make sure the pointer to the planes array is passed, 
which most probably already do. I'm OK with this change, it shouldn't 
be a big issue for applications using s5p drivers. 

> ---
>  Documentation/DocBook/media/v4l/io.xml              |    6 ++++--
>  Documentation/DocBook/media/v4l/vidioc-qbuf.xml     |    3 +--
>  Documentation/DocBook/media/v4l/vidioc-querybuf.xml |   11 +++++++----
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/io.xml b/Documentation/DocBook/media/v4l/io.xml
> index 1885cc0..c6d39fe 100644
> --- a/Documentation/DocBook/media/v4l/io.xml
> +++ b/Documentation/DocBook/media/v4l/io.xml
> @@ -677,8 +677,10 @@ memory, set by the application. See <xref linkend="userp" /> for details.
>  	    <entry><structfield>length</structfield></entry>
>  	    <entry></entry>
>  	    <entry>Size of the buffer (not the payload) in bytes for the
> -	    single-planar API. For the multi-planar API should contain the
> -	    number of elements in the <structfield>planes</structfield> array.
> +	    single-planar API. For the multi-planar API the application sets
> +	    this to the number of elements in the <structfield>planes</structfield>
> +	    array. The driver will fill in the actual number of valid elements in
> +	    that array.
>  	    </entry>
>  	  </row>
>  	  <row>
> diff --git a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> index 6a821a6..2d37abe 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-qbuf.xml
> @@ -121,8 +121,7 @@ remaining fields or returns an error code. The driver may also set
>  field. It indicates a non-critical (recoverable) streaming error. In such case
>  the application may continue as normal, but should be aware that data in the
>  dequeued buffer might be corrupted. When using the multi-planar API, the
> -planes array does not have to be passed; the <structfield>m.planes</structfield>
> -member must be set to NULL in that case.</para>
> +planes array must be passed in as well.</para>
>  
>      <para>By default <constant>VIDIOC_DQBUF</constant> blocks when no
>  buffer is in the outgoing queue. When the
> diff --git a/Documentation/DocBook/media/v4l/vidioc-querybuf.xml b/Documentation/DocBook/media/v4l/vidioc-querybuf.xml
> index 6e414d7..a597155 100644
> --- a/Documentation/DocBook/media/v4l/vidioc-querybuf.xml
> +++ b/Documentation/DocBook/media/v4l/vidioc-querybuf.xml
> @@ -48,8 +48,8 @@
>    <refsect1>
>      <title>Description</title>
>  
> -    <para>This ioctl is part of the <link linkend="mmap">memory
> -mapping</link> I/O method. It can be used to query the status of a
> +    <para>This ioctl is part of the <link linkend="mmap">streaming
> +</link> I/O method. It can be used to query the status of a
>  buffer at any time after buffers have been allocated with the
>  &VIDIOC-REQBUFS; ioctl.</para>
>  
> @@ -71,6 +71,7 @@ the structure.</para>
>  
>      <para>In the <structfield>flags</structfield> field the
>  <constant>V4L2_BUF_FLAG_MAPPED</constant>,
> +<constant>V4L2_BUF_FLAG_PREPARED</constant>,
>  <constant>V4L2_BUF_FLAG_QUEUED</constant> and
>  <constant>V4L2_BUF_FLAG_DONE</constant> flags will be valid. The
>  <structfield>memory</structfield> field will be set to the current
> @@ -79,8 +80,10 @@ contains the offset of the buffer from the start of the device memory,
>  the <structfield>length</structfield> field its size. For the multi-planar API,
>  fields <structfield>m.mem_offset</structfield> and
>  <structfield>length</structfield> in the <structfield>m.planes</structfield>
> -array elements will be used instead. The driver may or may not set the remaining
> -fields and flags, they are meaningless in this context.</para>
> +array elements will be used instead and the <structfield>length</structfield>
> +field of &v4l2-buffer; is set to the number of filled-in array elements.
> +The driver may or may not set the remaining fields and flags, they are
> +meaningless in this context.</para>
>  
>      <para>The <structname>v4l2_buffer</structname> structure is
>      specified in <xref linkend="buffer" />.</para>

Regards,
Sylwester

Re: [RFCv1 PATCH 4/6] videobuf2-core: fill in length field for multiplanar buffers.

[Sylwester Nawrocki]

On 09/21/2012 06:23 PM, Hans Verkuil wrote:
> On Fri September 21 2012 18:13:20 Sylwester Nawrocki wrote:
>> Hi Hans,
>>
>> On 09/19/2012 04:37 PM, Hans Verkuil wrote:
>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>
>>> length should be set to num_planes in __fill_v4l2_buffer(). That way the
>>> caller knows how many planes there are in the buffer.
>>>
>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> I think this would break VIDIOC_CREATE_BUFS. We need per buffer num_planes.
>> Consider a use case where device is streaming with 2-planar pixel format
>> and we invoke VIDIOC_CREATE_BUFS with single-planar format. On a single 
>> queue there will be buffers with different number of planes. The number of 
>> planes information must be attached to a buffer, otherwise VIDIOC_QUERYBUF 
>> won't work.
> 
> That's a very good point and one I need to meditate on.
> 
> However, your comment applies to patch 1/6, not to this one.
> This patch is about whether or not the length field of v4l2_buffer should
> be filled in with the actual number of planes used by that buffer or not.

Yes, right. Sorry, I was editing response to multiple patches from this
series and have mixed things a bit. I agree that it is logical and expected
to update struct v4l2_buffer for user space.

I have spent some time on this series, and even prepared a patch for s5p-mfc,
as it relies on num_planes being in struct vb2_buffer. But then a realized
there could be buffers with distinct number of planes an a single queue.

Regards,
Sylwester

Re: [RFCv1 PATCH 4/6] videobuf2-core: fill in length field for multiplanar buffers.

[Hans Verkuil]

On Fri September 21 2012 18:47:54 Sylwester Nawrocki wrote:
> On 09/21/2012 06:23 PM, Hans Verkuil wrote:
> > On Fri September 21 2012 18:13:20 Sylwester Nawrocki wrote:
> >> Hi Hans,
> >>
> >> On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> >>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>
> >>> length should be set to num_planes in __fill_v4l2_buffer(). That way the
> >>> caller knows how many planes there are in the buffer.
> >>>
> >>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> I think this would break VIDIOC_CREATE_BUFS. We need per buffer num_planes.
> >> Consider a use case where device is streaming with 2-planar pixel format
> >> and we invoke VIDIOC_CREATE_BUFS with single-planar format. On a single 
> >> queue there will be buffers with different number of planes. The number of 
> >> planes information must be attached to a buffer, otherwise VIDIOC_QUERYBUF 
> >> won't work.
> > 
> > That's a very good point and one I need to meditate on.
> > 
> > However, your comment applies to patch 1/6, not to this one.
> > This patch is about whether or not the length field of v4l2_buffer should
> > be filled in with the actual number of planes used by that buffer or not.
> 
> Yes, right. Sorry, I was editing response to multiple patches from this
> series and have mixed things a bit. I agree that it is logical and expected
> to update struct v4l2_buffer for user space.

OK, great. That's was actually the main reason for working on this as this
unexpected behavior bit me when writing mplane streaming support for v4l2-ctl.

> I have spent some time on this series, and even prepared a patch for s5p-mfc,
> as it relies on num_planes being in struct vb2_buffer. But then a realized
> there could be buffers with distinct number of planes an a single queue.

I'll get back to you about this, probably on Monday. I need to think about the
implications of this.

Regards,

	Hans

Re: [RFCv1 PATCH 4/6] videobuf2-core: fill in length field for multiplanar buffers.

[Hans Verkuil]

On Fri September 21 2012 18:54:12 Hans Verkuil wrote:
> On Fri September 21 2012 18:47:54 Sylwester Nawrocki wrote:
> > On 09/21/2012 06:23 PM, Hans Verkuil wrote:
> > > On Fri September 21 2012 18:13:20 Sylwester Nawrocki wrote:
> > >> Hi Hans,
> > >>
> > >> On 09/19/2012 04:37 PM, Hans Verkuil wrote:
> > >>> From: Hans Verkuil <hans.verkuil@cisco.com>
> > >>>
> > >>> length should be set to num_planes in __fill_v4l2_buffer(). That way the
> > >>> caller knows how many planes there are in the buffer.
> > >>>
> > >>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > >>
> > >> I think this would break VIDIOC_CREATE_BUFS. We need per buffer num_planes.
> > >> Consider a use case where device is streaming with 2-planar pixel format
> > >> and we invoke VIDIOC_CREATE_BUFS with single-planar format. On a single 
> > >> queue there will be buffers with different number of planes. The number of 
> > >> planes information must be attached to a buffer, otherwise VIDIOC_QUERYBUF 
> > >> won't work.
> > > 
> > > That's a very good point and one I need to meditate on.
> > > 
> > > However, your comment applies to patch 1/6, not to this one.
> > > This patch is about whether or not the length field of v4l2_buffer should
> > > be filled in with the actual number of planes used by that buffer or not.
> > 
> > Yes, right. Sorry, I was editing response to multiple patches from this
> > series and have mixed things a bit. I agree that it is logical and expected
> > to update struct v4l2_buffer for user space.
> 
> OK, great. That's was actually the main reason for working on this as this
> unexpected behavior bit me when writing mplane streaming support for v4l2-ctl.
> 
> > I have spent some time on this series, and even prepared a patch for s5p-mfc,
> > as it relies on num_planes being in struct vb2_buffer. But then a realized
> > there could be buffers with distinct number of planes an a single queue.
> 
> I'll get back to you about this, probably on Monday. I need to think about the
> implications of this.

You are absolutely right about that. It makes my patch a bit more complex since
you have to be careful in VIDIOC_DQBUF not to dequeue unless the provided
v4l2_buffer struct has enough room to store the plane information.

Regards,

	Hans