[PATCH/RFC 0/4] V4L: new ioctl()s to support multi-sized video-buffers

[PATCH/RFC 0/4] V4L: new ioctl()s to support multi-sized video-buffers

[Guennadi Liakhovetski]

Hi all

As discussed at the last V4L2 meeting in Warsaw, one of the prerequisites 
to support fast switching between different image formats is an ability to 
preallocate buffers of different sizes and handle them over to the driver 
in advance. This avoids the need to allocate buffers at the time of 
switching. This patch series is a first implementation of these ioctl()s, 
implemented for the sh_mobile_ceu_camera soc-camera host driver. Tested on 
an sh7722 migor SuperH platform. Yes, I know, documentation is missing 
yet;-)

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

[PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

A possibility to preallocate and initialise buffers of different sizes
in V4L2 is required for an efficient implementation of asnapshot mode.
This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
structures.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
 drivers/media/video/v4l2-ioctl.c          |   43 +++++++++++++++++++++++++++++
 include/linux/videodev2.h                 |   24 ++++++++++++++++
 include/media/v4l2-ioctl.h                |    3 ++
 4 files changed, 73 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
index 7c26947..d71b289 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
 	case VIDIOC_DQEVENT:
 	case VIDIOC_SUBSCRIBE_EVENT:
 	case VIDIOC_UNSUBSCRIBE_EVENT:
+	case VIDIOC_CREATE_BUFS:
+	case VIDIOC_DESTROY_BUFS:
+	case VIDIOC_SUBMIT_BUF:
 		ret = do_video_ioctl(file, cmd, arg);
 		break;
 
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index a01ed39..b80a211 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
 	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
 	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
 	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
+	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
+	[_IOC_NR(VIDIOC_DESTROY_BUFS)]     = "VIDIOC_DESTROY_BUFS",
+	[_IOC_NR(VIDIOC_SUBMIT_BUF)]       = "VIDIOC_SUBMIT_BUF",
 };
 #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
 
@@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
 		dbgarg(cmd, "type=0x%8.8x", sub->type);
 		break;
 	}
+	case VIDIOC_CREATE_BUFS:
+	{
+		struct v4l2_create_buffers *create = arg;
+
+		if (!ops->vidioc_create_bufs)
+			break;
+		ret = check_fmt(ops, create->format.type);
+		if (ret)
+			break;
+
+		if (create->size)
+			CLEAR_AFTER_FIELD(create, count);
+
+		ret = ops->vidioc_create_bufs(file, fh, create);
+
+		dbgarg(cmd, "count=%d\n", create->count);
+		break;
+	}
+	case VIDIOC_DESTROY_BUFS:
+	{
+		struct v4l2_buffer_span *span = arg;
+
+		if (!ops->vidioc_destroy_bufs)
+			break;
+
+		ret = ops->vidioc_destroy_bufs(file, fh, span);
+
+		dbgarg(cmd, "count=%d", span->count);
+		break;
+	}
+	case VIDIOC_SUBMIT_BUF:
+	{
+		unsigned int *i = arg;
+
+		if (!ops->vidioc_submit_buf)
+			break;
+		ret = ops->vidioc_submit_buf(file, fh, *i);
+		dbgarg(cmd, "index=%d", *i);
+		break;
+	}
 	default:
 	{
 		bool valid_prio = true;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index aa6c393..b6ef46e 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
 	__u32 revision;    /* chip revision, chip specific */
 } __attribute__ ((packed));
 
+/* VIDIOC_DESTROY_BUFS */
+struct v4l2_buffer_span {
+	__u32			index;	/* output: buffers index...index + count - 1 have been created */
+	__u32			count;
+	__u32			reserved[2];
+};
+
+/* struct v4l2_createbuffers::flags */
+#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
+
+/* VIDIOC_CREATE_BUFS */
+struct v4l2_create_buffers {
+	__u32			index;		/* output: buffers index...index + count - 1 have been created */
+	__u32			count;
+	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
+	enum v4l2_memory        memory;
+	__u32			size;		/* Explicit size, e.g., for compressed streams */
+	struct v4l2_format	format;		/* "type" is used always, the rest if size == 0 */
+};
+
 /*
  *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
  *
@@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
 #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct v4l2_event_subscription)
 #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
 
+#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
+#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
+#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
+
 /* Reminder: when adding new ioctls please add support for them to
    drivers/media/video/v4l2-compat-ioctl32.c as well! */
 
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index dd9f1e7..00962c6 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -122,6 +122,9 @@ struct v4l2_ioctl_ops {
 	int (*vidioc_qbuf)    (struct file *file, void *fh, struct v4l2_buffer *b);
 	int (*vidioc_dqbuf)   (struct file *file, void *fh, struct v4l2_buffer *b);
 
+	int (*vidioc_create_bufs) (struct file *file, void *fh, struct v4l2_create_buffers *b);
+	int (*vidioc_destroy_bufs)(struct file *file, void *fh, struct v4l2_buffer_span *b);
+	int (*vidioc_submit_buf)  (struct file *file, void *fh, unsigned int i);
 
 	int (*vidioc_overlay) (struct file *file, void *fh, unsigned int i);
 	int (*vidioc_g_fbuf)   (struct file *file, void *fh,
-- 
1.7.2.5

[PATCH/RFC 2/4] V4L: add videobuf2 helper functions to support multi-size video-buffers

[Guennadi Liakhovetski]

This patch extends the videobuf2 framework with new helper functions and
modifies existing ones to support multi-size video-buffers.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/video/videobuf2-core.c |  184 ++++++++++++++++++++++++++++++----
 include/media/videobuf2-core.h       |   15 +++
 2 files changed, 177 insertions(+), 22 deletions(-)

diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
index 71734a4..12cf4b1 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -41,7 +41,7 @@ module_param(debug, int, 0644);
  * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
  */
 static int __vb2_buf_mem_alloc(struct vb2_buffer *vb,
-				unsigned long *plane_sizes)
+				const unsigned long *plane_sizes)
 {
 	struct vb2_queue *q = vb->vb2_queue;
 	void *mem_priv;
@@ -107,13 +107,13 @@ static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
  * __setup_offsets() - setup unique offsets ("cookies") for every plane in
  * every buffer on the queue
  */
-static void __setup_offsets(struct vb2_queue *q)
+static void __setup_offsets(struct vb2_queue *q, unsigned int n)
 {
 	unsigned int buffer, plane;
 	struct vb2_buffer *vb;
 	unsigned long off = 0;
 
-	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
+	for (buffer = q->num_buffers - n; buffer < q->num_buffers; ++buffer) {
 		vb = q->bufs[buffer];
 		if (!vb)
 			continue;
@@ -139,7 +139,7 @@ static void __setup_offsets(struct vb2_queue *q)
  */
 static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
 			     unsigned int num_buffers, unsigned int num_planes,
-			     unsigned long plane_sizes[])
+			     const unsigned long plane_sizes[])
 {
 	unsigned int buffer;
 	struct vb2_buffer *vb;
@@ -160,7 +160,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;
-		vb->v4l2_buf.index = buffer;
+		vb->v4l2_buf.index = q->num_buffers + buffer;
 		vb->v4l2_buf.type = q->type;
 		vb->v4l2_buf.memory = memory;
 
@@ -188,12 +188,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
 			}
 		}
 
-		q->bufs[buffer] = vb;
+		q->bufs[q->num_buffers + buffer] = vb;
 	}
 
-	q->num_buffers = buffer;
+	q->num_buffers += buffer;
 
-	__setup_offsets(q);
+	__setup_offsets(q, buffer);
 
 	dprintk(1, "Allocated %d buffers, %d plane(s) each\n",
 			q->num_buffers, num_planes);
@@ -204,12 +204,13 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
 /**
  * __vb2_free_mem() - release all video buffer memory for a given queue
  */
-static void __vb2_free_mem(struct vb2_queue *q)
+static void __vb2_free_mem(struct vb2_queue *q, struct v4l2_buffer_span *span)
 {
 	unsigned int buffer;
 	struct vb2_buffer *vb;
 
-	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
+	for (buffer = span->index; buffer < span->index + span->count;
+	     ++buffer) {
 		vb = q->bufs[buffer];
 		if (!vb)
 			continue;
@@ -222,18 +223,17 @@ static void __vb2_free_mem(struct vb2_queue *q)
 	}
 }
 
-/**
- * __vb2_queue_free() - free the queue - video memory and related information
- * and 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)
+int vb2_destroy_bufs(struct vb2_queue *q, struct v4l2_buffer_span *span)
 {
-	unsigned int buffer;
+	int buffer;
+
+	if (span->index + span->count > q->num_buffers)
+		return -EINVAL;
 
 	/* Call driver-provided cleanup function for each buffer, if provided */
 	if (q->ops->buf_cleanup) {
-		for (buffer = 0; buffer < q->num_buffers; ++buffer) {
+		for (buffer = span->index;
+		     buffer < span->index + span->count; ++buffer) {
 			if (NULL == q->bufs[buffer])
 				continue;
 			q->ops->buf_cleanup(q->bufs[buffer]);
@@ -241,16 +241,36 @@ static void __vb2_queue_free(struct vb2_queue *q)
 	}
 
 	/* Release video buffer memory */
-	__vb2_free_mem(q);
+	__vb2_free_mem(q, span);
 
 	/* Free videobuf buffers */
-	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
+	for (buffer = span->index;
+	     buffer < span->index + span->count; ++buffer) {
 		kfree(q->bufs[buffer]);
 		q->bufs[buffer] = NULL;
 	}
 
-	q->num_buffers = 0;
-	q->memory = 0;
+	q->num_buffers -= span->count;
+	if (!q->num_buffers)
+		q->memory = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vb2_destroy_bufs);
+
+/**
+ * __vb2_queue_free() - free the queue - video memory and related information
+ * and 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)
+{
+	struct v4l2_buffer_span span = {
+		.index = 0,
+		.count = q->num_buffers,
+	};
+
+	vb2_destroy_bufs(q, &span);
 }
 
 /**
@@ -576,6 +596,126 @@ free_mem:
 }
 EXPORT_SYMBOL_GPL(vb2_reqbufs);
 
+int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
+{
+	unsigned int num_planes, num_buffers = 0;
+	unsigned long plane_sizes[VIDEO_MAX_PLANES] = {create->size};
+	int ret = 0;
+
+	if (q->fileio) {
+		dprintk(1, "%s(): file io in progress\n", __func__);
+		return -EBUSY;
+	}
+
+	if (create->memory != V4L2_MEMORY_MMAP
+			&& create->memory != V4L2_MEMORY_USERPTR) {
+		dprintk(1, "%s(): unsupported memory type\n", __func__);
+		return -EINVAL;
+	}
+
+	if (create->format.type != q->type) {
+		dprintk(1, "%s(): requested type is incorrect\n", __func__);
+		return -EINVAL;
+	}
+
+	/*
+	 * Make sure all the required memory ops for given memory type
+	 * are available.
+	 */
+	if (create->memory == V4L2_MEMORY_MMAP && __verify_mmap_ops(q)) {
+		dprintk(1, "%s(): MMAP for current setup unsupported\n", __func__);
+		return -EINVAL;
+	}
+
+	if (create->memory == V4L2_MEMORY_USERPTR && __verify_userptr_ops(q)) {
+		dprintk(1, "%s(): USERPTR for current setup unsupported\n", __func__);
+		return -EINVAL;
+	}
+
+	if (q->num_buffers == VIDEO_MAX_FRAME) {
+		dprintk(1, "%s(): maximum number of buffers already allocated\n",
+			__func__);
+		return -ENOBUFS;
+	}
+
+	create->index = q->num_buffers + 1;
+
+	/*
+	 * Make sure the requested values and current defaults are sane.
+	 */
+	create->count = min_t(unsigned int, create->count, VIDEO_MAX_FRAME - q->num_buffers);
+
+	if (!q->num_buffers)
+		memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
+
+	/*
+	 * Ask the driver how many buffers and planes per buffer it requires.
+	 * Driver also sets the size and allocator context for each plane.
+	 */
+	ret = call_qop(q, queue_add, q, create, &num_buffers, &num_planes,
+		       plane_sizes, q->alloc_ctx);
+	if (ret)
+		return ret;
+
+	/* Finally, allocate buffers and video memory */
+	ret = __vb2_queue_alloc(q, create->memory, num_buffers, num_planes,
+				plane_sizes);
+	if (ret < 0) {
+		dprintk(1, "Memory allocation failed with error: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Check if driver can handle the so far allocated number of buffers.
+	 */
+	if (ret < num_buffers) {
+		unsigned int orig_num_buffers;
+
+		create->count = ret;
+		orig_num_buffers = ret;
+
+		/*
+		 * num_buffers still contains the number of buffers, that the
+		 * queue driver has allocated
+		 */
+		ret = call_qop(q, queue_add, q, create, &num_buffers,
+			       &num_planes, plane_sizes, q->alloc_ctx);
+		if (ret)
+			goto free_mem;
+
+		if (orig_num_buffers < num_buffers) {
+			ret = -ENOMEM;
+			goto free_mem;
+		}
+
+		/*
+		 * Ok, driver accepted smaller number of buffers.
+		 */
+		ret = num_buffers;
+	}
+
+	/*
+	 * Return the number of successfully allocated buffers
+	 * to the userspace.
+	 */
+	create->count = ret;
+
+	q->memory = create->memory;
+
+	return 0;
+
+free_mem:
+	__vb2_queue_free(q);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(vb2_create_bufs);
+
+int vb2_submit_buf(struct vb2_queue *q, unsigned int idx)
+{
+	return call_qop(q, buf_submit, q->bufs[idx]);
+}
+EXPORT_SYMBOL_GPL(vb2_submit_buf);
+
 /**
  * vb2_plane_vaddr() - Return a kernel virtual address of a given plane
  * @vb:		vb2_buffer to which the plane in question belongs to
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index f87472a..88076fc 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -177,6 +177,10 @@ struct vb2_buffer {
  *			plane should be set in the sizes[] array and optional
  *			per-plane allocator specific context in alloc_ctxs[]
  *			array
+ * @queue_add:		like above, but called from VIDIOC_CREATE_BUFS, but if
+ *			there are already buffers on the queue, it won't replace
+ *			them, but add new ones, possibly with a different format
+ *			and plane sizes
  * @wait_prepare:	release any locks taken while calling vb2 functions;
  *			it is called before an ioctl needs to wait for a new
  *			buffer to arrive; required to avoid a deadlock in
@@ -194,6 +198,8 @@ struct vb2_buffer {
  *			each hardware operation in this callback;
  *			if an error is returned, the buffer will not be queued
  *			in driver; optional
+ * @buf_submit:		called primarily to invalidate buffer caches for faster
+ *			consequent queuing; optional
  * @buf_finish:		called before every dequeue of the buffer back to
  *			userspace; drivers may perform any operations required
  *			before userspace accesses the buffer; optional
@@ -213,11 +219,16 @@ struct vb2_ops {
 	int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers,
 			   unsigned int *num_planes, unsigned long sizes[],
 			   void *alloc_ctxs[]);
+	int (*queue_add)(struct vb2_queue *q,
+			 struct v4l2_create_buffers *create,
+			 unsigned int *num_buffers, unsigned int *num_planes,
+			 unsigned long sizes[], void *alloc_ctxs[]);
 
 	void (*wait_prepare)(struct vb2_queue *q);
 	void (*wait_finish)(struct vb2_queue *q);
 
 	int (*buf_init)(struct vb2_buffer *vb);
+	int (*buf_submit)(struct vb2_buffer *vb);
 	int (*buf_prepare)(struct vb2_buffer *vb);
 	int (*buf_finish)(struct vb2_buffer *vb);
 	void (*buf_cleanup)(struct vb2_buffer *vb);
@@ -291,6 +302,10 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q);
 int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
 int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req);
 
+int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
+int vb2_destroy_bufs(struct vb2_queue *q, struct v4l2_buffer_span *span);
+int vb2_submit_buf(struct vb2_queue *q, unsigned int idx);
+
 int vb2_queue_init(struct vb2_queue *q);
 
 void vb2_queue_release(struct vb2_queue *q);
-- 
1.7.2.5

[PATCH/RFC 3/4] V4L: soc-camera: add support for new multi-size video-buffer ioctl()s

[Guennadi Liakhovetski]

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/video/soc_camera.c |   46 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 11f0f1e..6a41e89 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -286,6 +286,45 @@ static int soc_camera_dqbuf(struct file *file, void *priv,
 		return vb2_dqbuf(&icd->vb2_vidq, p, file->f_flags & O_NONBLOCK);
 }
 
+static int soc_camera_create_bufs(struct file *file, void *priv,
+			    struct v4l2_create_buffers *create)
+{
+	struct soc_camera_device *icd = file->private_data;
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+
+	/* videobuf2 only */
+	if (ici->ops->init_videobuf)
+		return -EINVAL;
+	else
+		return vb2_create_bufs(&icd->vb2_vidq, create);
+}
+
+static int soc_camera_destroy_bufs(struct file *file, void *priv,
+			    struct v4l2_buffer_span *span)
+{
+	struct soc_camera_device *icd = file->private_data;
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+
+	/* videobuf2 only */
+	if (ici->ops->init_videobuf)
+		return -EINVAL;
+	else
+		return vb2_destroy_bufs(&icd->vb2_vidq, span);
+}
+
+static int soc_camera_submit_buf(struct file *file, void *priv,
+			    unsigned int idx)
+{
+	struct soc_camera_device *icd = file->private_data;
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+
+	/* videobuf2 only */
+	if (ici->ops->init_videobuf)
+		return -EINVAL;
+	else
+		return vb2_submit_buf(&icd->vb2_vidq, idx);
+}
+
 /* Always entered with .video_lock held */
 static int soc_camera_init_user_formats(struct soc_camera_device *icd)
 {
@@ -1420,19 +1459,22 @@ static void soc_camera_device_unregister(struct soc_camera_device *icd)
 
 static const struct v4l2_ioctl_ops soc_camera_ioctl_ops = {
 	.vidioc_querycap	 = soc_camera_querycap,
+	.vidioc_try_fmt_vid_cap  = soc_camera_try_fmt_vid_cap,
 	.vidioc_g_fmt_vid_cap    = soc_camera_g_fmt_vid_cap,
-	.vidioc_enum_fmt_vid_cap = soc_camera_enum_fmt_vid_cap,
 	.vidioc_s_fmt_vid_cap    = soc_camera_s_fmt_vid_cap,
+	.vidioc_enum_fmt_vid_cap = soc_camera_enum_fmt_vid_cap,
 	.vidioc_enum_input	 = soc_camera_enum_input,
 	.vidioc_g_input		 = soc_camera_g_input,
 	.vidioc_s_input		 = soc_camera_s_input,
 	.vidioc_s_std		 = soc_camera_s_std,
 	.vidioc_enum_framesizes  = soc_camera_enum_fsizes,
 	.vidioc_reqbufs		 = soc_camera_reqbufs,
-	.vidioc_try_fmt_vid_cap  = soc_camera_try_fmt_vid_cap,
 	.vidioc_querybuf	 = soc_camera_querybuf,
 	.vidioc_qbuf		 = soc_camera_qbuf,
 	.vidioc_dqbuf		 = soc_camera_dqbuf,
+	.vidioc_create_bufs	 = soc_camera_create_bufs,
+	.vidioc_destroy_bufs	 = soc_camera_destroy_bufs,
+	.vidioc_submit_buf	 = soc_camera_submit_buf,
 	.vidioc_streamon	 = soc_camera_streamon,
 	.vidioc_streamoff	 = soc_camera_streamoff,
 	.vidioc_queryctrl	 = soc_camera_queryctrl,
-- 
1.7.2.5

[PATCH/RFC 4/4] V4L: sh_mobile_ceu_camera: support multi-size video-buffers

[Guennadi Liakhovetski]

With this patch it is possible to prequeue buffers of different sizes
in the driver and switch between them by just stopping streaming,
setting a new format, queuing the suitable buffers and re-starting the
streaming escaping the need to allocate buffers on this time-critical
path.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/video/sh_mobile_ceu_camera.c |  104 ++++++++++++++++++++++++----
 1 files changed, 91 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/sh_mobile_ceu_camera.c b/drivers/media/video/sh_mobile_ceu_camera.c
index d1446ad..3245fff 100644
--- a/drivers/media/video/sh_mobile_ceu_camera.c
+++ b/drivers/media/video/sh_mobile_ceu_camera.c
@@ -100,6 +100,7 @@ struct sh_mobile_ceu_dev {
 	unsigned int irq;
 	void __iomem *base;
 	unsigned long video_limit;
+	unsigned long buf_total;
 
 	spinlock_t lock;		/* Protects video buffer lists */
 	struct list_head capture;
@@ -215,38 +216,110 @@ static int sh_mobile_ceu_soft_reset(struct sh_mobile_ceu_dev *pcdev)
 /*
  *  Videobuf operations
  */
-static int sh_mobile_ceu_videobuf_setup(struct vb2_queue *vq,
+
+/*
+ * .queue_add() can be called in two situations:
+ * (1)	to add a new buffer set. In this case create->count is the number of
+ *	buffers to be added, *count == 0. We have to return the number of
+ *	added buffers in *count.
+ * (2)	to try to adjust the number of buffers down. In this case create->count
+ *	is the (smaller) number of buffers, that the caller wants to have, and
+ *	*count is the number of buffers, that we actually allocated in step (1)
+ *	above. If the smaller create->count is still sufficient for us, we have
+ *	to adjust our internal configuration and return *count = create->count.
+ */
+static int sh_mobile_ceu_videobuf_add(struct vb2_queue *vq,
+			struct v4l2_create_buffers *create,
 			unsigned int *count, unsigned int *num_planes,
 			unsigned long sizes[], void *alloc_ctxs[])
 {
 	struct soc_camera_device *icd = container_of(vq, struct soc_camera_device, vb2_vidq);
 	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	struct sh_mobile_ceu_dev *pcdev = ici->priv;
-	int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
-						icd->current_fmt->host_fmt);
+	const struct soc_camera_format_xlate *xlate = soc_camera_xlate_by_fourcc(icd,
+						create->format.fmt.pix.pixelformat);
+	const struct soc_mbus_pixelfmt *fmt;
+	int bytes_per_line;
+	ssize_t size;
+
+	if (!xlate)
+		return -ENOENT;
 
+	fmt = xlate->host_fmt;
+
+	/* fmt must be != NULL */
+	bytes_per_line = soc_mbus_bytes_per_line(create->format.fmt.pix.width, fmt);
 	if (bytes_per_line < 0)
 		return bytes_per_line;
 
+	if (create->count < 2)
+		create->count = 2;
+
 	*num_planes = 1;
 
-	pcdev->sequence = 0;
-	sizes[0] = bytes_per_line * icd->user_height;
+	if (!pcdev->buf_total)
+		pcdev->sequence = 0;
+	/* Ignore possible user-provided size, we cannot use it */
+	sizes[0] = bytes_per_line * create->format.fmt.pix.height;
 	alloc_ctxs[0] = pcdev->alloc_ctx;
 
-	if (!*count)
-		*count = 2;
+	size = PAGE_ALIGN(sizes[0]) * (create->count - *count);
+
+	if (pcdev->video_limit &&
+	    pcdev->buf_total + size > pcdev->video_limit) {
+		/* This can only be entered in case (1) in the above comment */
+		unsigned int cnt = (pcdev->video_limit - pcdev->buf_total) /
+			PAGE_ALIGN(sizes[0]);
+
+		/*
+		 * Normally *count would be 0 here, but add it anyway in case
+		 * someone decides to call this function to increase the number
+		 * of buffers from != 0
+		 */
+		if (cnt + *count < 2)
+			return -ENOBUFS;
 
-	if (pcdev->video_limit) {
-		if (PAGE_ALIGN(sizes[0]) * *count > pcdev->video_limit)
-			*count = pcdev->video_limit / PAGE_ALIGN(sizes[0]);
+		size = PAGE_ALIGN(sizes[0]) * cnt;
+		*count += cnt;
+	} else {
+		*count = create->count;
 	}
 
-	dev_dbg(icd->dev.parent, "count=%d, size=%lu\n", *count, sizes[0]);
+	pcdev->buf_total += size;
+
+	dev_dbg(icd->dev.parent, "count=%d, size=%lu, fmt=0x%x\n",
+		*count, sizes[0], fmt->fourcc);
 
 	return 0;
 }
 
+static int sh_mobile_ceu_videobuf_setup(struct vb2_queue *vq,
+			unsigned int *count, unsigned int *num_planes,
+			unsigned long sizes[], void *alloc_ctxs[])
+{
+	struct soc_camera_device *icd = container_of(vq, struct soc_camera_device, vb2_vidq);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	struct sh_mobile_ceu_dev *pcdev = ici->priv;
+	struct v4l2_create_buffers create = {
+		.count = *count,
+		.format.fmt.pix = {
+			.width = icd->user_width,
+			.height = icd->user_height,
+			.pixelformat = icd->current_fmt->host_fmt->fourcc,
+		},
+	};
+
+	if (vq->num_buffers)
+		/* The core failed to allocate the required number of buffers */
+		*count = pcdev->buf_total / PAGE_ALIGN(sizes[0]);
+	else
+		*count = 0;
+
+	/* Normal allocation */
+	return sh_mobile_ceu_videobuf_add(vq, &create, count, num_planes, sizes,
+					  alloc_ctxs);
+}
+
 #define CEU_CETCR_MAGIC 0x0317f313 /* acknowledge magical interrupt sources */
 #define CEU_CETCR_IGRW (1 << 4) /* prohibited register access interrupt bit */
 #define CEU_CEIER_CPEIE (1 << 0) /* one-frame capture end interrupt */
@@ -371,8 +444,8 @@ static int sh_mobile_ceu_videobuf_prepare(struct vb2_buffer *vb)
 	size = icd->user_height * bytes_per_line;
 
 	if (vb2_plane_size(vb, 0) < size) {
-		dev_err(icd->dev.parent, "Buffer too small (%lu < %lu)\n",
-			vb2_plane_size(vb, 0), size);
+		dev_err(icd->dev.parent, "Buffer #%d too small (%lu < %lu)\n",
+			vb->v4l2_buf.index, vb2_plane_size(vb, 0), size);
 		return -ENOBUFS;
 	}
 
@@ -424,6 +497,8 @@ static void sh_mobile_ceu_videobuf_release(struct vb2_buffer *vb)
 	/* Doesn't hurt also if the list is empty */
 	list_del_init(&buf->queue);
 
+	pcdev->buf_total -= vb2_plane_size(vb, 0);
+
 	spin_unlock_irq(&pcdev->lock);
 }
 
@@ -455,6 +530,7 @@ static int sh_mobile_ceu_stop_streaming(struct vb2_queue *q)
 
 static struct vb2_ops sh_mobile_ceu_videobuf_ops = {
 	.queue_setup	= sh_mobile_ceu_videobuf_setup,
+	.queue_add	= sh_mobile_ceu_videobuf_add,
 	.buf_prepare	= sh_mobile_ceu_videobuf_prepare,
 	.buf_queue	= sh_mobile_ceu_videobuf_queue,
 	.buf_cleanup	= sh_mobile_ceu_videobuf_release,
@@ -515,6 +591,8 @@ static int sh_mobile_ceu_add_device(struct soc_camera_device *icd)
 
 	pm_runtime_get_sync(ici->v4l2_dev.dev);
 
+	pcdev->buf_total = 0;
+
 	ret = sh_mobile_ceu_soft_reset(pcdev);
 	if (!ret)
 		pcdev->icd = icd;
-- 
1.7.2.5

[PATCH/RFC 2/4 v2] V4L: add videobuf2 helper functions to support multi-size video-buffers

[Guennadi Liakhovetski]

This patch extends the videobuf2 framework with new helper functions and
modifies existing ones to support multi-size video-buffers.

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

v2: fix offset calculation in __setup_offsets()

 drivers/media/video/videobuf2-core.c |  195 ++++++++++++++++++++++++++++++----
 include/media/videobuf2-core.h       |   15 +++
 2 files changed, 187 insertions(+), 23 deletions(-)

diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
index 71734a4..20e1572 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -41,7 +41,7 @@ module_param(debug, int, 0644);
  * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
  */
 static int __vb2_buf_mem_alloc(struct vb2_buffer *vb,
-				unsigned long *plane_sizes)
+				const unsigned long *plane_sizes)
 {
 	struct vb2_queue *q = vb->vb2_queue;
 	void *mem_priv;
@@ -107,13 +107,22 @@ static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
  * __setup_offsets() - setup unique offsets ("cookies") for every plane in
  * every buffer on the queue
  */
-static void __setup_offsets(struct vb2_queue *q)
+static void __setup_offsets(struct vb2_queue *q, unsigned int n)
 {
 	unsigned int buffer, plane;
 	struct vb2_buffer *vb;
-	unsigned long off = 0;
+	unsigned long off;
 
-	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
+	if (q->num_buffers) {
+		struct v4l2_plane *p;
+		vb = q->bufs[q->num_buffers - 1];
+		p = &vb->v4l2_planes[vb->num_planes - 1];
+		off = PAGE_ALIGN(p->m.mem_offset + p->length);
+	} else {
+		off = 0;
+	}
+
+	for (buffer = q->num_buffers - n; buffer < q->num_buffers; ++buffer) {
 		vb = q->bufs[buffer];
 		if (!vb)
 			continue;
@@ -139,7 +148,7 @@ static void __setup_offsets(struct vb2_queue *q)
  */
 static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
 			     unsigned int num_buffers, unsigned int num_planes,
-			     unsigned long plane_sizes[])
+			     const unsigned long plane_sizes[])
 {
 	unsigned int buffer;
 	struct vb2_buffer *vb;
@@ -160,7 +169,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;
-		vb->v4l2_buf.index = buffer;
+		vb->v4l2_buf.index = q->num_buffers + buffer;
 		vb->v4l2_buf.type = q->type;
 		vb->v4l2_buf.memory = memory;
 
@@ -188,12 +197,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
 			}
 		}
 
-		q->bufs[buffer] = vb;
+		q->bufs[q->num_buffers + buffer] = vb;
 	}
 
-	q->num_buffers = buffer;
+	q->num_buffers += buffer;
 
-	__setup_offsets(q);
+	__setup_offsets(q, buffer);
 
 	dprintk(1, "Allocated %d buffers, %d plane(s) each\n",
 			q->num_buffers, num_planes);
@@ -204,12 +213,13 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
 /**
  * __vb2_free_mem() - release all video buffer memory for a given queue
  */
-static void __vb2_free_mem(struct vb2_queue *q)
+static void __vb2_free_mem(struct vb2_queue *q, struct v4l2_buffer_span *span)
 {
 	unsigned int buffer;
 	struct vb2_buffer *vb;
 
-	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
+	for (buffer = span->index; buffer < span->index + span->count;
+	     ++buffer) {
 		vb = q->bufs[buffer];
 		if (!vb)
 			continue;
@@ -222,18 +232,17 @@ static void __vb2_free_mem(struct vb2_queue *q)
 	}
 }
 
-/**
- * __vb2_queue_free() - free the queue - video memory and related information
- * and 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)
+int vb2_destroy_bufs(struct vb2_queue *q, struct v4l2_buffer_span *span)
 {
-	unsigned int buffer;
+	int buffer;
+
+	if (span->index + span->count > q->num_buffers)
+		return -EINVAL;
 
 	/* Call driver-provided cleanup function for each buffer, if provided */
 	if (q->ops->buf_cleanup) {
-		for (buffer = 0; buffer < q->num_buffers; ++buffer) {
+		for (buffer = span->index;
+		     buffer < span->index + span->count; ++buffer) {
 			if (NULL == q->bufs[buffer])
 				continue;
 			q->ops->buf_cleanup(q->bufs[buffer]);
@@ -241,16 +250,36 @@ static void __vb2_queue_free(struct vb2_queue *q)
 	}
 
 	/* Release video buffer memory */
-	__vb2_free_mem(q);
+	__vb2_free_mem(q, span);
 
 	/* Free videobuf buffers */
-	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
+	for (buffer = span->index;
+	     buffer < span->index + span->count; ++buffer) {
 		kfree(q->bufs[buffer]);
 		q->bufs[buffer] = NULL;
 	}
 
-	q->num_buffers = 0;
-	q->memory = 0;
+	q->num_buffers -= span->count;
+	if (!q->num_buffers)
+		q->memory = 0;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vb2_destroy_bufs);
+
+/**
+ * __vb2_queue_free() - free the queue - video memory and related information
+ * and 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)
+{
+	struct v4l2_buffer_span span = {
+		.index = 0,
+		.count = q->num_buffers,
+	};
+
+	vb2_destroy_bufs(q, &span);
 }
 
 /**
@@ -576,6 +605,126 @@ free_mem:
 }
 EXPORT_SYMBOL_GPL(vb2_reqbufs);
 
+int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
+{
+	unsigned int num_planes, num_buffers = 0;
+	unsigned long plane_sizes[VIDEO_MAX_PLANES] = {create->size};
+	int ret = 0;
+
+	if (q->fileio) {
+		dprintk(1, "%s(): file io in progress\n", __func__);
+		return -EBUSY;
+	}
+
+	if (create->memory != V4L2_MEMORY_MMAP
+			&& create->memory != V4L2_MEMORY_USERPTR) {
+		dprintk(1, "%s(): unsupported memory type\n", __func__);
+		return -EINVAL;
+	}
+
+	if (create->format.type != q->type) {
+		dprintk(1, "%s(): requested type is incorrect\n", __func__);
+		return -EINVAL;
+	}
+
+	/*
+	 * Make sure all the required memory ops for given memory type
+	 * are available.
+	 */
+	if (create->memory == V4L2_MEMORY_MMAP && __verify_mmap_ops(q)) {
+		dprintk(1, "%s(): MMAP for current setup unsupported\n", __func__);
+		return -EINVAL;
+	}
+
+	if (create->memory == V4L2_MEMORY_USERPTR && __verify_userptr_ops(q)) {
+		dprintk(1, "%s(): USERPTR for current setup unsupported\n", __func__);
+		return -EINVAL;
+	}
+
+	if (q->num_buffers == VIDEO_MAX_FRAME) {
+		dprintk(1, "%s(): maximum number of buffers already allocated\n",
+			__func__);
+		return -ENOBUFS;
+	}
+
+	create->index = q->num_buffers + 1;
+
+	/*
+	 * Make sure the requested values and current defaults are sane.
+	 */
+	create->count = min_t(unsigned int, create->count, VIDEO_MAX_FRAME - q->num_buffers);
+
+	if (!q->num_buffers)
+		memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
+
+	/*
+	 * Ask the driver how many buffers and planes per buffer it requires.
+	 * Driver also sets the size and allocator context for each plane.
+	 */
+	ret = call_qop(q, queue_add, q, create, &num_buffers, &num_planes,
+		       plane_sizes, q->alloc_ctx);
+	if (ret)
+		return ret;
+
+	/* Finally, allocate buffers and video memory */
+	ret = __vb2_queue_alloc(q, create->memory, num_buffers, num_planes,
+				plane_sizes);
+	if (ret < 0) {
+		dprintk(1, "Memory allocation failed with error: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Check if driver can handle the so far allocated number of buffers.
+	 */
+	if (ret < num_buffers) {
+		unsigned int orig_num_buffers;
+
+		create->count = ret;
+		orig_num_buffers = ret;
+
+		/*
+		 * num_buffers still contains the number of buffers, that the
+		 * queue driver has allocated
+		 */
+		ret = call_qop(q, queue_add, q, create, &num_buffers,
+			       &num_planes, plane_sizes, q->alloc_ctx);
+		if (ret)
+			goto free_mem;
+
+		if (orig_num_buffers < num_buffers) {
+			ret = -ENOMEM;
+			goto free_mem;
+		}
+
+		/*
+		 * Ok, driver accepted smaller number of buffers.
+		 */
+		ret = num_buffers;
+	}
+
+	/*
+	 * Return the number of successfully allocated buffers
+	 * to the userspace.
+	 */
+	create->count = ret;
+
+	q->memory = create->memory;
+
+	return 0;
+
+free_mem:
+	__vb2_queue_free(q);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(vb2_create_bufs);
+
+int vb2_submit_buf(struct vb2_queue *q, unsigned int idx)
+{
+	return call_qop(q, buf_submit, q->bufs[idx]);
+}
+EXPORT_SYMBOL_GPL(vb2_submit_buf);
+
 /**
  * vb2_plane_vaddr() - Return a kernel virtual address of a given plane
  * @vb:		vb2_buffer to which the plane in question belongs to
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index f87472a..88076fc 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -177,6 +177,10 @@ struct vb2_buffer {
  *			plane should be set in the sizes[] array and optional
  *			per-plane allocator specific context in alloc_ctxs[]
  *			array
+ * @queue_add:		like above, but called from VIDIOC_CREATE_BUFS, but if
+ *			there are already buffers on the queue, it won't replace
+ *			them, but add new ones, possibly with a different format
+ *			and plane sizes
  * @wait_prepare:	release any locks taken while calling vb2 functions;
  *			it is called before an ioctl needs to wait for a new
  *			buffer to arrive; required to avoid a deadlock in
@@ -194,6 +198,8 @@ struct vb2_buffer {
  *			each hardware operation in this callback;
  *			if an error is returned, the buffer will not be queued
  *			in driver; optional
+ * @buf_submit:		called primarily to invalidate buffer caches for faster
+ *			consequent queuing; optional
  * @buf_finish:		called before every dequeue of the buffer back to
  *			userspace; drivers may perform any operations required
  *			before userspace accesses the buffer; optional
@@ -213,11 +219,16 @@ struct vb2_ops {
 	int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers,
 			   unsigned int *num_planes, unsigned long sizes[],
 			   void *alloc_ctxs[]);
+	int (*queue_add)(struct vb2_queue *q,
+			 struct v4l2_create_buffers *create,
+			 unsigned int *num_buffers, unsigned int *num_planes,
+			 unsigned long sizes[], void *alloc_ctxs[]);
 
 	void (*wait_prepare)(struct vb2_queue *q);
 	void (*wait_finish)(struct vb2_queue *q);
 
 	int (*buf_init)(struct vb2_buffer *vb);
+	int (*buf_submit)(struct vb2_buffer *vb);
 	int (*buf_prepare)(struct vb2_buffer *vb);
 	int (*buf_finish)(struct vb2_buffer *vb);
 	void (*buf_cleanup)(struct vb2_buffer *vb);
@@ -291,6 +302,10 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q);
 int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
 int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req);
 
+int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
+int vb2_destroy_bufs(struct vb2_queue *q, struct v4l2_buffer_span *span);
+int vb2_submit_buf(struct vb2_queue *q, unsigned int idx);
+
 int vb2_queue_init(struct vb2_queue *q);
 
 void vb2_queue_release(struct vb2_queue *q);
-- 
1.7.2.5

Re: [PATCH/RFC 0/4] V4L: new ioctl()s to support multi-sized video-buffers

[Pawel Osciak]

Hi Guennadi,

On Fri, Apr 1, 2011 at 01:12, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> Hi all
>
> As discussed at the last V4L2 meeting in Warsaw, one of the prerequisites
> to support fast switching between different image formats is an ability to
> preallocate buffers of different sizes and handle them over to the driver
> in advance. This avoids the need to allocate buffers at the time of
> switching. This patch series is a first implementation of these ioctl()s,
> implemented for the sh_mobile_ceu_camera soc-camera host driver. Tested on
> an sh7722 migor SuperH platform. Yes, I know, documentation is missing
> yet;-)
>

I will have to wait for documentation before doing a full review, it's
hard to comment without it. Also, please mention how the new ioctls
influence the state machine. Some questions and doubts I'm having:
- Can you call CREATE more than once, before/after REQBUFS, for all
streaming states? What about reading/writing?
- Can driver decline CREATE if it is not supported? What if the format
is not supported?
- If we fail allocating in CREATE, should the whole queue be freed (as
it is done in your patch I believe)?
- I'm assuming REQBUFS(0) is to free buffers allocated with CREATE too?
- Are we allowing DESTROY to free arbitrary span of buffers (i.e.
those created with REQBUFS as well)?
- Are "holes" in buffer indexes allowed? I don't like the ability to
free an arbitrary span of buffers in the queue, it complicates checks
in many places and I don't think is worth it...
- I understand SUBMIT is optional?
- Could you give an example of how this could be used in an application?

-- 
Best regards,
Pawel Osciak

Re: [PATCH/RFC 2/4 v2] V4L: add videobuf2 helper functions to support multi-size video-buffers

[Pawel Osciak]

Hi Gueannadi,
A preliminary quick scan over the patch while waiting for documentation.

On Fri, Apr 1, 2011 at 07:06, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> This patch extends the videobuf2 framework with new helper functions and
> modifies existing ones to support multi-size video-buffers.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> v2: fix offset calculation in __setup_offsets()
>
>  drivers/media/video/videobuf2-core.c |  195 ++++++++++++++++++++++++++++++----
>  include/media/videobuf2-core.h       |   15 +++
>  2 files changed, 187 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
> index 71734a4..20e1572 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -41,7 +41,7 @@ module_param(debug, int, 0644);
>  * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
>  */
>  static int __vb2_buf_mem_alloc(struct vb2_buffer *vb,
> -                               unsigned long *plane_sizes)
> +                               const unsigned long *plane_sizes)
>  {
>        struct vb2_queue *q = vb->vb2_queue;
>        void *mem_priv;
> @@ -107,13 +107,22 @@ static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
>  * __setup_offsets() - setup unique offsets ("cookies") for every plane in
>  * every buffer on the queue
>  */
> -static void __setup_offsets(struct vb2_queue *q)
> +static void __setup_offsets(struct vb2_queue *q, unsigned int n)
>  {
>        unsigned int buffer, plane;
>        struct vb2_buffer *vb;
> -       unsigned long off = 0;
> +       unsigned long off;
>
> -       for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> +       if (q->num_buffers) {
> +               struct v4l2_plane *p;
> +               vb = q->bufs[q->num_buffers - 1];
> +               p = &vb->v4l2_planes[vb->num_planes - 1];
> +               off = PAGE_ALIGN(p->m.mem_offset + p->length);
> +       } else {
> +               off = 0;
> +       }
> +
> +       for (buffer = q->num_buffers - n; buffer < q->num_buffers; ++buffer) {
>                vb = q->bufs[buffer];
>                if (!vb)
>                        continue;
> @@ -139,7 +148,7 @@ static void __setup_offsets(struct vb2_queue *q)
>  */
>  static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
>                             unsigned int num_buffers, unsigned int num_planes,
> -                            unsigned long plane_sizes[])
> +                            const unsigned long plane_sizes[])
>  {
>        unsigned int buffer;
>        struct vb2_buffer *vb;
> @@ -160,7 +169,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;
> -               vb->v4l2_buf.index = buffer;
> +               vb->v4l2_buf.index = q->num_buffers + buffer;
>                vb->v4l2_buf.type = q->type;
>                vb->v4l2_buf.memory = memory;
>
> @@ -188,12 +197,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
>                        }
>                }
>
> -               q->bufs[buffer] = vb;
> +               q->bufs[q->num_buffers + buffer] = vb;
>        }
>
> -       q->num_buffers = buffer;
> +       q->num_buffers += buffer;
>
> -       __setup_offsets(q);
> +       __setup_offsets(q, buffer);
>
>        dprintk(1, "Allocated %d buffers, %d plane(s) each\n",
>                        q->num_buffers, num_planes);
> @@ -204,12 +213,13 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
>  /**
>  * __vb2_free_mem() - release all video buffer memory for a given queue
>  */
> -static void __vb2_free_mem(struct vb2_queue *q)
> +static void __vb2_free_mem(struct vb2_queue *q, struct v4l2_buffer_span *span)
>  {
>        unsigned int buffer;
>        struct vb2_buffer *vb;
>
> -       for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> +       for (buffer = span->index; buffer < span->index + span->count;
> +            ++buffer) {
>                vb = q->bufs[buffer];
>                if (!vb)
>                        continue;
> @@ -222,18 +232,17 @@ static void __vb2_free_mem(struct vb2_queue *q)
>        }
>  }
>
> -/**
> - * __vb2_queue_free() - free the queue - video memory and related information
> - * and 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)
> +int vb2_destroy_bufs(struct vb2_queue *q, struct v4l2_buffer_span *span)
>  {
> -       unsigned int buffer;
> +       int buffer;
> +
> +       if (span->index + span->count > q->num_buffers)
> +               return -EINVAL;
>

Maybe it'd be better to free what we can here?

>        /* Call driver-provided cleanup function for each buffer, if provided */
>        if (q->ops->buf_cleanup) {
> -               for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> +               for (buffer = span->index;
> +                    buffer < span->index + span->count; ++buffer) {
>                        if (NULL == q->bufs[buffer])
>                                continue;
>                        q->ops->buf_cleanup(q->bufs[buffer]);
> @@ -241,16 +250,36 @@ static void __vb2_queue_free(struct vb2_queue *q)
>        }
>
>        /* Release video buffer memory */
> -       __vb2_free_mem(q);
> +       __vb2_free_mem(q, span);
>
>        /* Free videobuf buffers */
> -       for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> +       for (buffer = span->index;
> +            buffer < span->index + span->count; ++buffer) {
>                kfree(q->bufs[buffer]);
>                q->bufs[buffer] = NULL;
>        }
>
> -       q->num_buffers = 0;
> -       q->memory = 0;
> +       q->num_buffers -= span->count;
> +       if (!q->num_buffers)
> +               q->memory = 0;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(vb2_destroy_bufs);
> +

This messes up q->num_buffers, which is used in other places as the
upper bound for indexes and assumes <0; q->num_buffers> is contiguous.
Examples include querybufs, buffers_in_use. In general, I find it
unnecessary complicated and dangerous to allow introducing "holes" in
buffer indexes.

> +/**
> + * __vb2_queue_free() - free the queue - video memory and related information
> + * and 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)
> +{
> +       struct v4l2_buffer_span span = {
> +               .index = 0,
> +               .count = q->num_buffers,
> +       };
> +
> +       vb2_destroy_bufs(q, &span);
>  }
>
>  /**
> @@ -576,6 +605,126 @@ free_mem:
>  }
>  EXPORT_SYMBOL_GPL(vb2_reqbufs);
>
> +int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
> +{
> +       unsigned int num_planes, num_buffers = 0;
> +       unsigned long plane_sizes[VIDEO_MAX_PLANES] = {create->size};
> +       int ret = 0;
> +
> +       if (q->fileio) {
> +               dprintk(1, "%s(): file io in progress\n", __func__);
> +               return -EBUSY;
> +       }
> +
> +       if (create->memory != V4L2_MEMORY_MMAP
> +                       && create->memory != V4L2_MEMORY_USERPTR) {
> +               dprintk(1, "%s(): unsupported memory type\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       if (create->format.type != q->type) {
> +               dprintk(1, "%s(): requested type is incorrect\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * Make sure all the required memory ops for given memory type
> +        * are available.
> +        */
> +       if (create->memory == V4L2_MEMORY_MMAP && __verify_mmap_ops(q)) {
> +               dprintk(1, "%s(): MMAP for current setup unsupported\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       if (create->memory == V4L2_MEMORY_USERPTR && __verify_userptr_ops(q)) {
> +               dprintk(1, "%s(): USERPTR for current setup unsupported\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       if (q->num_buffers == VIDEO_MAX_FRAME) {
> +               dprintk(1, "%s(): maximum number of buffers already allocated\n",
> +                       __func__);
> +               return -ENOBUFS;
> +       }
> +
> +       create->index = q->num_buffers + 1;
> +

Is this gap in indexes deliberate?

> +       /*
> +        * Make sure the requested values and current defaults are sane.
> +        */
> +       create->count = min_t(unsigned int, create->count, VIDEO_MAX_FRAME - q->num_buffers);
> +
> +       if (!q->num_buffers)
> +               memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
> +
> +       /*
> +        * Ask the driver how many buffers and planes per buffer it requires.
> +        * Driver also sets the size and allocator context for each plane.
> +        */
> +       ret = call_qop(q, queue_add, q, create, &num_buffers, &num_planes,
> +                      plane_sizes, q->alloc_ctx);
> +       if (ret)
> +               return ret;
> +
> +       /* Finally, allocate buffers and video memory */
> +       ret = __vb2_queue_alloc(q, create->memory, num_buffers, num_planes,
> +                               plane_sizes);
> +       if (ret < 0) {
> +               dprintk(1, "Memory allocation failed with error: %d\n", ret);
> +               return ret;
> +       }
> +
> +       /*
> +        * Check if driver can handle the so far allocated number of buffers.
> +        */
> +       if (ret < num_buffers) {
> +               unsigned int orig_num_buffers;
> +
> +               create->count = ret;
> +               orig_num_buffers = ret;
> +
> +               /*
> +                * num_buffers still contains the number of buffers, that the
> +                * queue driver has allocated
> +                */
> +               ret = call_qop(q, queue_add, q, create, &num_buffers,
> +                              &num_planes, plane_sizes, q->alloc_ctx);
> +               if (ret)
> +                       goto free_mem;
> +
> +               if (orig_num_buffers < num_buffers) {
> +                       ret = -ENOMEM;
> +                       goto free_mem;
> +               }
> +
> +               /*
> +                * Ok, driver accepted smaller number of buffers.
> +                */
> +               ret = num_buffers;
> +       }
> +
> +       /*
> +        * Return the number of successfully allocated buffers
> +        * to the userspace.
> +        */
> +       create->count = ret;
> +
> +       q->memory = create->memory;
> +
> +       return 0;
> +
> +free_mem:
> +       __vb2_queue_free(q);

Are we freeing the whole queue on a failed CREATE? Shouldn't we only
be freeing the buffers that CREATE requested?

> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(vb2_create_bufs);
> +
> +int vb2_submit_buf(struct vb2_queue *q, unsigned int idx)
> +{
> +       return call_qop(q, buf_submit, q->bufs[idx]);
> +}
> +EXPORT_SYMBOL_GPL(vb2_submit_buf);
> +
>  /**
>  * vb2_plane_vaddr() - Return a kernel virtual address of a given plane
>  * @vb:                vb2_buffer to which the plane in question belongs to
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index f87472a..88076fc 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h

I'll be adding more detailed documentation of vb2_ops soon as well.
Some comments in spirit of that change:

> @@ -177,6 +177,10 @@ struct vb2_buffer {
>  *                     plane should be set in the sizes[] array and optional
>  *                     per-plane allocator specific context in alloc_ctxs[]
>  *                     array
> + * @queue_add:         like above, but called from VIDIOC_CREATE_BUFS, but if
> + *                     there are already buffers on the queue, it won't replace
> + *                     them, but add new ones, possibly with a different format
> + *                     and plane sizes

Needs some info how the driver can react and what to return.

>  * @wait_prepare:      release any locks taken while calling vb2 functions;
>  *                     it is called before an ioctl needs to wait for a new
>  *                     buffer to arrive; required to avoid a deadlock in
> @@ -194,6 +198,8 @@ struct vb2_buffer {
>  *                     each hardware operation in this callback;
>  *                     if an error is returned, the buffer will not be queued
>  *                     in driver; optional
> + * @buf_submit:                called primarily to invalidate buffer caches for faster
> + *                     consequent queuing; optional

Ditto. Also, I think it needs a more detailed explanation...

>  * @buf_finish:                called before every dequeue of the buffer back to
>  *                     userspace; drivers may perform any operations required
>  *                     before userspace accesses the buffer; optional
> @@ -213,11 +219,16 @@ struct vb2_ops {
>        int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers,
>                           unsigned int *num_planes, unsigned long sizes[],
>                           void *alloc_ctxs[]);
> +       int (*queue_add)(struct vb2_queue *q,
> +                        struct v4l2_create_buffers *create,
> +                        unsigned int *num_buffers, unsigned int *num_planes,
> +                        unsigned long sizes[], void *alloc_ctxs[]);
>
>        void (*wait_prepare)(struct vb2_queue *q);
>        void (*wait_finish)(struct vb2_queue *q);
>
>        int (*buf_init)(struct vb2_buffer *vb);
> +       int (*buf_submit)(struct vb2_buffer *vb);
>        int (*buf_prepare)(struct vb2_buffer *vb);
>        int (*buf_finish)(struct vb2_buffer *vb);
>        void (*buf_cleanup)(struct vb2_buffer *vb);
> @@ -291,6 +302,10 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q);
>  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
>  int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req);
>
> +int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
> +int vb2_destroy_bufs(struct vb2_queue *q, struct v4l2_buffer_span *span);
> +int vb2_submit_buf(struct vb2_queue *q, unsigned int idx);
> +
>  int vb2_queue_init(struct vb2_queue *q);
>
>  void vb2_queue_release(struct vb2_queue *q);
> --
> 1.7.2.5
>
> --
> 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
>

-- 
Best regards,
Pawel Osciak

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Hans Verkuil]

On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote:
> A possibility to preallocate and initialise buffers of different sizes
> in V4L2 is required for an efficient implementation of asnapshot mode.
> This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
> VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
> structures.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
>  drivers/media/video/v4l2-ioctl.c          |   43 +++++++++++++++++++++++++++++
>  include/linux/videodev2.h                 |   24 ++++++++++++++++
>  include/media/v4l2-ioctl.h                |    3 ++
>  4 files changed, 73 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
> index 7c26947..d71b289 100644
> --- a/drivers/media/video/v4l2-compat-ioctl32.c
> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> @@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
>  	case VIDIOC_DQEVENT:
>  	case VIDIOC_SUBSCRIBE_EVENT:
>  	case VIDIOC_UNSUBSCRIBE_EVENT:
> +	case VIDIOC_CREATE_BUFS:
> +	case VIDIOC_DESTROY_BUFS:
> +	case VIDIOC_SUBMIT_BUF:
>  		ret = do_video_ioctl(file, cmd, arg);
>  		break;
>  
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index a01ed39..b80a211 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
>  	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
>  	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
>  	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> +	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
> +	[_IOC_NR(VIDIOC_DESTROY_BUFS)]     = "VIDIOC_DESTROY_BUFS",
> +	[_IOC_NR(VIDIOC_SUBMIT_BUF)]       = "VIDIOC_SUBMIT_BUF",
>  };
>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>  
> @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
>  		dbgarg(cmd, "type=0x%8.8x", sub->type);
>  		break;
>  	}
> +	case VIDIOC_CREATE_BUFS:
> +	{
> +		struct v4l2_create_buffers *create = arg;
> +
> +		if (!ops->vidioc_create_bufs)
> +			break;
> +		ret = check_fmt(ops, create->format.type);
> +		if (ret)
> +			break;
> +
> +		if (create->size)
> +			CLEAR_AFTER_FIELD(create, count);
> +
> +		ret = ops->vidioc_create_bufs(file, fh, create);
> +
> +		dbgarg(cmd, "count=%d\n", create->count);
> +		break;
> +	}
> +	case VIDIOC_DESTROY_BUFS:
> +	{
> +		struct v4l2_buffer_span *span = arg;
> +
> +		if (!ops->vidioc_destroy_bufs)
> +			break;
> +
> +		ret = ops->vidioc_destroy_bufs(file, fh, span);
> +
> +		dbgarg(cmd, "count=%d", span->count);
> +		break;
> +	}
> +	case VIDIOC_SUBMIT_BUF:
> +	{
> +		unsigned int *i = arg;
> +
> +		if (!ops->vidioc_submit_buf)
> +			break;
> +		ret = ops->vidioc_submit_buf(file, fh, *i);
> +		dbgarg(cmd, "index=%d", *i);
> +		break;
> +	}
>  	default:
>  	{
>  		bool valid_prio = true;
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index aa6c393..b6ef46e 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
>  	__u32 revision;    /* chip revision, chip specific */
>  } __attribute__ ((packed));
>  
> +/* VIDIOC_DESTROY_BUFS */
> +struct v4l2_buffer_span {
> +	__u32			index;	/* output: buffers index...index + count - 1 have been created */
> +	__u32			count;
> +	__u32			reserved[2];
> +};
> +
> +/* struct v4l2_createbuffers::flags */
> +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)

We also need a FLAG_NO_CACHE_FLUSH. This is for output devices.

> +
> +/* VIDIOC_CREATE_BUFS */
> +struct v4l2_create_buffers {
> +	__u32			index;		/* output: buffers index...index + count - 1 have been created */
> +	__u32			count;
> +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> +	enum v4l2_memory        memory;
> +	__u32			size;		/* Explicit size, e.g., for compressed streams */

Hmm, shouldn't this be an array of size VIDEO_MAX_PLANES?

> +	struct v4l2_format	format;		/* "type" is used always, the rest if size == 0 */

Needs some reserved fields as well.

> +};
> +
>  /*
>   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
>   *
> @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
>  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct v4l2_event_subscription)
>  #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
>  
> +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)

I don't really like the name. I think I'd go for _PRE_QBUF or _PREP_BUF or
something like that.

BTW, I agree with other reviewers that DESTROY_BUFS shouldn't leave any holes.
And if CREATE_BUFS has an error, then it shouldn't destroy all buffers, but only
those that create_bufs managed to allocate before the error occurred.

Regards,

	Hans

> +
>  /* Reminder: when adding new ioctls please add support for them to
>     drivers/media/video/v4l2-compat-ioctl32.c as well! */

Don't forget this! VIDIOC_CREATE_BUFS will need to be handled in compat-ioctl32.

Regards,

	Hans

>  
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index dd9f1e7..00962c6 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -122,6 +122,9 @@ struct v4l2_ioctl_ops {
>  	int (*vidioc_qbuf)    (struct file *file, void *fh, struct v4l2_buffer *b);
>  	int (*vidioc_dqbuf)   (struct file *file, void *fh, struct v4l2_buffer *b);
>  
> +	int (*vidioc_create_bufs) (struct file *file, void *fh, struct v4l2_create_buffers *b);
> +	int (*vidioc_destroy_bufs)(struct file *file, void *fh, struct v4l2_buffer_span *b);
> +	int (*vidioc_submit_buf)  (struct file *file, void *fh, unsigned int i);
>  
>  	int (*vidioc_overlay) (struct file *file, void *fh, unsigned int i);
>  	int (*vidioc_g_fbuf)   (struct file *file, void *fh,
> 

Re: [PATCH/RFC 0/4] V4L: new ioctl()s to support multi-sized video-buffers

[Guennadi Liakhovetski]

On Sun, 3 Apr 2011, Pawel Osciak wrote:

> Hi Guennadi,
> 
> On Fri, Apr 1, 2011 at 01:12, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > Hi all
> >
> > As discussed at the last V4L2 meeting in Warsaw, one of the prerequisites
> > to support fast switching between different image formats is an ability to
> > preallocate buffers of different sizes and handle them over to the driver
> > in advance. This avoids the need to allocate buffers at the time of
> > switching. This patch series is a first implementation of these ioctl()s,
> > implemented for the sh_mobile_ceu_camera soc-camera host driver. Tested on
> > an sh7722 migor SuperH platform. Yes, I know, documentation is missing
> > yet;-)
> >
> 
> I will have to wait for documentation before doing a full review, it's
> hard to comment without it. Also, please mention how the new ioctls
> influence the state machine.

Ok, I wanted to wait with the documentation until we have the API settled, 
because modifying the code is easier, than modifying the documentation:-) 
But right, I'll try to put something together.

> Some questions and doubts I'm having:
> - Can you call CREATE more than once, before/after REQBUFS, for all
> streaming states? What about reading/writing?

The idea was to use CREATE/DESTROY _instead_ of REQBUFS. And yes, one of 
the purposes of CREATE is to be able to call it multiple times with 
different parameters. The new API should provide at least all the 
functionality, that REQBUFS provides, i.e., you should be able to use it 
with MMAP and USERPTR memory.

> - Can driver decline CREATE if it is not supported? What if the format
> is not supported?

Sure, if .vidioc_create_bufs() is not implemented by the driver, the 
ioctl() will just error out. Of course you're allowed to do any checks you 
see fit in your driver, like unsupported formats and return an error in 
case of a problem.

> - If we fail allocating in CREATE, should the whole queue be freed (as
> it is done in your patch I believe)?

No, that's a bug, thanks for spotting!

> - I'm assuming REQBUFS(0) is to free buffers allocated with CREATE too?

Currently it is possible to mix CREATE/DESTROY and REQBUFS. Not sure if 
this is good, maybe we have to allow the use of only one API. I'd probably 
prefer the latter, but I'm open for suggestions here.

> - Are we allowing DESTROY to free arbitrary span of buffers (i.e.
> those created with REQBUFS as well)?

Again, we can decide, whether we want to support mixing of these APIs or 
not.

> - Are "holes" in buffer indexes allowed? I don't like the ability to
> free an arbitrary span of buffers in the queue, it complicates checks
> in many places and I don't think is worth it...

That's how this ioctl() has been proposed at the Warsaw meeting.

> - I understand SUBMIT is optional?

yes.

> - Could you give an example of how this could be used in an application?

Ok, for my testing I modified the capture.c example from v4l2, I'll try to 
clean up a patch for it and post it.

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

Hi Hans

Thanks for the review

On Mon, 4 Apr 2011, Hans Verkuil wrote:

> On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote:
> > A possibility to preallocate and initialise buffers of different sizes
> > in V4L2 is required for an efficient implementation of asnapshot mode.
> > This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
> > VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
> > structures.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
> >  drivers/media/video/v4l2-ioctl.c          |   43 +++++++++++++++++++++++++++++
> >  include/linux/videodev2.h                 |   24 ++++++++++++++++
> >  include/media/v4l2-ioctl.h                |    3 ++
> >  4 files changed, 73 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
> > index 7c26947..d71b289 100644
> > --- a/drivers/media/video/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> > @@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
> >  	case VIDIOC_DQEVENT:
> >  	case VIDIOC_SUBSCRIBE_EVENT:
> >  	case VIDIOC_UNSUBSCRIBE_EVENT:
> > +	case VIDIOC_CREATE_BUFS:
> > +	case VIDIOC_DESTROY_BUFS:
> > +	case VIDIOC_SUBMIT_BUF:
> >  		ret = do_video_ioctl(file, cmd, arg);
> >  		break;
> >  
> > diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> > index a01ed39..b80a211 100644
> > --- a/drivers/media/video/v4l2-ioctl.c
> > +++ b/drivers/media/video/v4l2-ioctl.c
> > @@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
> >  	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
> >  	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
> >  	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> > +	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
> > +	[_IOC_NR(VIDIOC_DESTROY_BUFS)]     = "VIDIOC_DESTROY_BUFS",
> > +	[_IOC_NR(VIDIOC_SUBMIT_BUF)]       = "VIDIOC_SUBMIT_BUF",
> >  };
> >  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> >  
> > @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
> >  		dbgarg(cmd, "type=0x%8.8x", sub->type);
> >  		break;
> >  	}
> > +	case VIDIOC_CREATE_BUFS:
> > +	{
> > +		struct v4l2_create_buffers *create = arg;
> > +
> > +		if (!ops->vidioc_create_bufs)
> > +			break;
> > +		ret = check_fmt(ops, create->format.type);
> > +		if (ret)
> > +			break;
> > +
> > +		if (create->size)
> > +			CLEAR_AFTER_FIELD(create, count);
> > +
> > +		ret = ops->vidioc_create_bufs(file, fh, create);
> > +
> > +		dbgarg(cmd, "count=%d\n", create->count);
> > +		break;
> > +	}
> > +	case VIDIOC_DESTROY_BUFS:
> > +	{
> > +		struct v4l2_buffer_span *span = arg;
> > +
> > +		if (!ops->vidioc_destroy_bufs)
> > +			break;
> > +
> > +		ret = ops->vidioc_destroy_bufs(file, fh, span);
> > +
> > +		dbgarg(cmd, "count=%d", span->count);
> > +		break;
> > +	}
> > +	case VIDIOC_SUBMIT_BUF:
> > +	{
> > +		unsigned int *i = arg;
> > +
> > +		if (!ops->vidioc_submit_buf)
> > +			break;
> > +		ret = ops->vidioc_submit_buf(file, fh, *i);
> > +		dbgarg(cmd, "index=%d", *i);
> > +		break;
> > +	}
> >  	default:
> >  	{
> >  		bool valid_prio = true;
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index aa6c393..b6ef46e 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
> >  	__u32 revision;    /* chip revision, chip specific */
> >  } __attribute__ ((packed));
> >  
> > +/* VIDIOC_DESTROY_BUFS */
> > +struct v4l2_buffer_span {
> > +	__u32			index;	/* output: buffers index...index + count - 1 have been created */
> > +	__u32			count;
> > +	__u32			reserved[2];
> > +};
> > +
> > +/* struct v4l2_createbuffers::flags */
> > +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
> 
> We also need a FLAG_NO_CACHE_FLUSH. This is for output devices.

Ok.

> > +
> > +/* VIDIOC_CREATE_BUFS */
> > +struct v4l2_create_buffers {
> > +	__u32			index;		/* output: buffers index...index + count - 1 have been created */
> > +	__u32			count;
> > +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> > +	enum v4l2_memory        memory;
> > +	__u32			size;		/* Explicit size, e.g., for compressed streams */
> 
> Hmm, shouldn't this be an array of size VIDEO_MAX_PLANES?

Not sure. As the comment says, this is mainly for bitstream formats. For 
any pixel-based format you really should just fill in the format below. If 
you allow the user to specify planes, then you also need to know 
alignment, contiguity, padding, etc.

> > +	struct v4l2_format	format;		/* "type" is used always, the rest if size == 0 */
> 
> Needs some reserved fields as well.

v4l2_format is a union with 200 bytes, is this not enough?

> > +};
> > +
> >  /*
> >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> >   *
> > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct v4l2_event_subscription)
> >  #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
> >  
> > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> 
> I don't really like the name. I think I'd go for _PRE_QBUF or _PREP_BUF or
> something like that.

I didn't want to use any form of prepare, because we already have a 
.buf_prepare() method, and IMHO it would be confusing. Pre-queue I didn't 
like very much either, for the same reasons, why we rejected it at the 
meeting, which was, I think, that it's not really doing anything with the 
queue, right? What this ioctl() does is it passes buffer ownership from 
the user-space to the kernel, right? Any good name for that? In fact 
"submit" doesn't sound too bad to me:-)

> BTW, I agree with other reviewers that DESTROY_BUFS shouldn't leave any holes.

That was our decision at the meeting - I can well remember that, I was 
surprised about that too, so, I think, I even asked to clarify this point. 
But we can change it of course. Shall we return -EBUSY if the user is 
trying to free buffers in the middle?

> And if CREATE_BUFS has an error, then it shouldn't destroy all buffers, but only
> those that create_bufs managed to allocate before the error occurred.

Sure, that's a bug.

> Regards,
> 
> 	Hans
> 
> > +
> >  /* Reminder: when adding new ioctls please add support for them to
> >     drivers/media/video/v4l2-compat-ioctl32.c as well! */
> 
> Don't forget this! VIDIOC_CREATE_BUFS will need to be handled in compat-ioctl32.

I didn't:

> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++

Or is that not enough? Is any special handling required? AFAICS, REQBUFS 
is not treated specially either.

Thanks
Guennadi

> Regards,
> 
> 	Hans
> 
> >  
> > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> > index dd9f1e7..00962c6 100644
> > --- a/include/media/v4l2-ioctl.h
> > +++ b/include/media/v4l2-ioctl.h
> > @@ -122,6 +122,9 @@ struct v4l2_ioctl_ops {
> >  	int (*vidioc_qbuf)    (struct file *file, void *fh, struct v4l2_buffer *b);
> >  	int (*vidioc_dqbuf)   (struct file *file, void *fh, struct v4l2_buffer *b);
> >  
> > +	int (*vidioc_create_bufs) (struct file *file, void *fh, struct v4l2_create_buffers *b);
> > +	int (*vidioc_destroy_bufs)(struct file *file, void *fh, struct v4l2_buffer_span *b);
> > +	int (*vidioc_submit_buf)  (struct file *file, void *fh, unsigned int i);
> >  
> >  	int (*vidioc_overlay) (struct file *file, void *fh, unsigned int i);
> >  	int (*vidioc_g_fbuf)   (struct file *file, void *fh,
> > 
> 

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

Re: [PATCH/RFC 2/4 v2] V4L: add videobuf2 helper functions to support multi-size video-buffers

[Guennadi Liakhovetski]

Hi Pawel

On Sun, 3 Apr 2011, Pawel Osciak wrote:

> Hi Gueannadi,
> A preliminary quick scan over the patch while waiting for documentation.
> 
> On Fri, Apr 1, 2011 at 07:06, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> > This patch extends the videobuf2 framework with new helper functions and
> > modifies existing ones to support multi-size video-buffers.
> >
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >
> > v2: fix offset calculation in __setup_offsets()
> >
> >  drivers/media/video/videobuf2-core.c |  195 ++++++++++++++++++++++++++++++----
> >  include/media/videobuf2-core.h       |   15 +++
> >  2 files changed, 187 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
> > index 71734a4..20e1572 100644
> > --- a/drivers/media/video/videobuf2-core.c
> > +++ b/drivers/media/video/videobuf2-core.c
> > @@ -41,7 +41,7 @@ module_param(debug, int, 0644);
> >  * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
> >  */
> >  static int __vb2_buf_mem_alloc(struct vb2_buffer *vb,
> > -                               unsigned long *plane_sizes)
> > +                               const unsigned long *plane_sizes)
> >  {
> >        struct vb2_queue *q = vb->vb2_queue;
> >        void *mem_priv;
> > @@ -107,13 +107,22 @@ static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
> >  * __setup_offsets() - setup unique offsets ("cookies") for every plane in
> >  * every buffer on the queue
> >  */
> > -static void __setup_offsets(struct vb2_queue *q)
> > +static void __setup_offsets(struct vb2_queue *q, unsigned int n)
> >  {
> >        unsigned int buffer, plane;
> >        struct vb2_buffer *vb;
> > -       unsigned long off = 0;
> > +       unsigned long off;
> >
> > -       for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> > +       if (q->num_buffers) {
> > +               struct v4l2_plane *p;
> > +               vb = q->bufs[q->num_buffers - 1];
> > +               p = &vb->v4l2_planes[vb->num_planes - 1];
> > +               off = PAGE_ALIGN(p->m.mem_offset + p->length);
> > +       } else {
> > +               off = 0;
> > +       }
> > +
> > +       for (buffer = q->num_buffers - n; buffer < q->num_buffers; ++buffer) {
> >                vb = q->bufs[buffer];
> >                if (!vb)
> >                        continue;
> > @@ -139,7 +148,7 @@ static void __setup_offsets(struct vb2_queue *q)
> >  */
> >  static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
> >                             unsigned int num_buffers, unsigned int num_planes,
> > -                            unsigned long plane_sizes[])
> > +                            const unsigned long plane_sizes[])
> >  {
> >        unsigned int buffer;
> >        struct vb2_buffer *vb;
> > @@ -160,7 +169,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;
> > -               vb->v4l2_buf.index = buffer;
> > +               vb->v4l2_buf.index = q->num_buffers + buffer;
> >                vb->v4l2_buf.type = q->type;
> >                vb->v4l2_buf.memory = memory;
> >
> > @@ -188,12 +197,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
> >                        }
> >                }
> >
> > -               q->bufs[buffer] = vb;
> > +               q->bufs[q->num_buffers + buffer] = vb;
> >        }
> >
> > -       q->num_buffers = buffer;
> > +       q->num_buffers += buffer;
> >
> > -       __setup_offsets(q);
> > +       __setup_offsets(q, buffer);
> >
> >        dprintk(1, "Allocated %d buffers, %d plane(s) each\n",
> >                        q->num_buffers, num_planes);
> > @@ -204,12 +213,13 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
> >  /**
> >  * __vb2_free_mem() - release all video buffer memory for a given queue
> >  */
> > -static void __vb2_free_mem(struct vb2_queue *q)
> > +static void __vb2_free_mem(struct vb2_queue *q, struct v4l2_buffer_span *span)
> >  {
> >        unsigned int buffer;
> >        struct vb2_buffer *vb;
> >
> > -       for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> > +       for (buffer = span->index; buffer < span->index + span->count;
> > +            ++buffer) {
> >                vb = q->bufs[buffer];
> >                if (!vb)
> >                        continue;
> > @@ -222,18 +232,17 @@ static void __vb2_free_mem(struct vb2_queue *q)
> >        }
> >  }
> >
> > -/**
> > - * __vb2_queue_free() - free the queue - video memory and related information
> > - * and 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)
> > +int vb2_destroy_bufs(struct vb2_queue *q, struct v4l2_buffer_span *span)
> >  {
> > -       unsigned int buffer;
> > +       int buffer;
> > +
> > +       if (span->index + span->count > q->num_buffers)
> > +               return -EINVAL;
> >
> 
> Maybe it'd be better to free what we can here?

Don't think so - it's a bogus request...

> >        /* Call driver-provided cleanup function for each buffer, if provided */
> >        if (q->ops->buf_cleanup) {
> > -               for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> > +               for (buffer = span->index;
> > +                    buffer < span->index + span->count; ++buffer) {
> >                        if (NULL == q->bufs[buffer])
> >                                continue;
> >                        q->ops->buf_cleanup(q->bufs[buffer]);
> > @@ -241,16 +250,36 @@ static void __vb2_queue_free(struct vb2_queue *q)
> >        }
> >
> >        /* Release video buffer memory */
> > -       __vb2_free_mem(q);
> > +       __vb2_free_mem(q, span);
> >
> >        /* Free videobuf buffers */
> > -       for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> > +       for (buffer = span->index;
> > +            buffer < span->index + span->count; ++buffer) {
> >                kfree(q->bufs[buffer]);
> >                q->bufs[buffer] = NULL;
> >        }
> >
> > -       q->num_buffers = 0;
> > -       q->memory = 0;
> > +       q->num_buffers -= span->count;
> > +       if (!q->num_buffers)
> > +               q->memory = 0;
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(vb2_destroy_bufs);
> > +
> 
> This messes up q->num_buffers, which is used in other places as the
> upper bound for indexes and assumes <0; q->num_buffers> is contiguous.
> Examples include querybufs, buffers_in_use. In general, I find it
> unnecessary complicated and dangerous to allow introducing "holes" in
> buffer indexes.

Yes, there are issues... I think, we can just return -EBUSY on such 
fragmenting DESTROY calls. Anything left over will be freed on last 
close() anyway. Let's decide how we want to handle these and I'll prepare 
a v2, probably with documentation this time:-)

> > +/**
> > + * __vb2_queue_free() - free the queue - video memory and related information
> > + * and 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)
> > +{
> > +       struct v4l2_buffer_span span = {
> > +               .index = 0,
> > +               .count = q->num_buffers,
> > +       };
> > +
> > +       vb2_destroy_bufs(q, &span);
> >  }
> >
> >  /**
> > @@ -576,6 +605,126 @@ free_mem:
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_reqbufs);
> >
> > +int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
> > +{
> > +       unsigned int num_planes, num_buffers = 0;
> > +       unsigned long plane_sizes[VIDEO_MAX_PLANES] = {create->size};
> > +       int ret = 0;
> > +
> > +       if (q->fileio) {
> > +               dprintk(1, "%s(): file io in progress\n", __func__);
> > +               return -EBUSY;
> > +       }
> > +
> > +       if (create->memory != V4L2_MEMORY_MMAP
> > +                       && create->memory != V4L2_MEMORY_USERPTR) {
> > +               dprintk(1, "%s(): unsupported memory type\n", __func__);
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (create->format.type != q->type) {
> > +               dprintk(1, "%s(): requested type is incorrect\n", __func__);
> > +               return -EINVAL;
> > +       }
> > +
> > +       /*
> > +        * Make sure all the required memory ops for given memory type
> > +        * are available.
> > +        */
> > +       if (create->memory == V4L2_MEMORY_MMAP && __verify_mmap_ops(q)) {
> > +               dprintk(1, "%s(): MMAP for current setup unsupported\n", __func__);
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (create->memory == V4L2_MEMORY_USERPTR && __verify_userptr_ops(q)) {
> > +               dprintk(1, "%s(): USERPTR for current setup unsupported\n", __func__);
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (q->num_buffers == VIDEO_MAX_FRAME) {
> > +               dprintk(1, "%s(): maximum number of buffers already allocated\n",
> > +                       __func__);
> > +               return -ENOBUFS;
> > +       }
> > +
> > +       create->index = q->num_buffers + 1;
> > +
> 
> Is this gap in indexes deliberate?

It was deliberate, yes, but it seems wrong to me now too... I'll re-think 
and probably remove it.

> > +       /*
> > +        * Make sure the requested values and current defaults are sane.
> > +        */
> > +       create->count = min_t(unsigned int, create->count, VIDEO_MAX_FRAME - q->num_buffers);
> > +
> > +       if (!q->num_buffers)
> > +               memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
> > +
> > +       /*
> > +        * Ask the driver how many buffers and planes per buffer it requires.
> > +        * Driver also sets the size and allocator context for each plane.
> > +        */
> > +       ret = call_qop(q, queue_add, q, create, &num_buffers, &num_planes,
> > +                      plane_sizes, q->alloc_ctx);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Finally, allocate buffers and video memory */
> > +       ret = __vb2_queue_alloc(q, create->memory, num_buffers, num_planes,
> > +                               plane_sizes);
> > +       if (ret < 0) {
> > +               dprintk(1, "Memory allocation failed with error: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       /*
> > +        * Check if driver can handle the so far allocated number of buffers.
> > +        */
> > +       if (ret < num_buffers) {
> > +               unsigned int orig_num_buffers;
> > +
> > +               create->count = ret;
> > +               orig_num_buffers = ret;
> > +
> > +               /*
> > +                * num_buffers still contains the number of buffers, that the
> > +                * queue driver has allocated
> > +                */
> > +               ret = call_qop(q, queue_add, q, create, &num_buffers,
> > +                              &num_planes, plane_sizes, q->alloc_ctx);
> > +               if (ret)
> > +                       goto free_mem;
> > +
> > +               if (orig_num_buffers < num_buffers) {
> > +                       ret = -ENOMEM;
> > +                       goto free_mem;
> > +               }
> > +
> > +               /*
> > +                * Ok, driver accepted smaller number of buffers.
> > +                */
> > +               ret = num_buffers;
> > +       }
> > +
> > +       /*
> > +        * Return the number of successfully allocated buffers
> > +        * to the userspace.
> > +        */
> > +       create->count = ret;
> > +
> > +       q->memory = create->memory;
> > +
> > +       return 0;
> > +
> > +free_mem:
> > +       __vb2_queue_free(q);
> 
> Are we freeing the whole queue on a failed CREATE? Shouldn't we only
> be freeing the buffers that CREATE requested?

Right, thanks for catching.

> > +       return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(vb2_create_bufs);
> > +
> > +int vb2_submit_buf(struct vb2_queue *q, unsigned int idx)
> > +{
> > +       return call_qop(q, buf_submit, q->bufs[idx]);
> > +}
> > +EXPORT_SYMBOL_GPL(vb2_submit_buf);
> > +
> >  /**
> >  * vb2_plane_vaddr() - Return a kernel virtual address of a given plane
> >  * @vb:                vb2_buffer to which the plane in question belongs to
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index f87472a..88076fc 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> 
> I'll be adding more detailed documentation of vb2_ops soon as well.
> Some comments in spirit of that change:
> 
> > @@ -177,6 +177,10 @@ struct vb2_buffer {
> >  *                     plane should be set in the sizes[] array and optional
> >  *                     per-plane allocator specific context in alloc_ctxs[]
> >  *                     array
> > + * @queue_add:         like above, but called from VIDIOC_CREATE_BUFS, but if
> > + *                     there are already buffers on the queue, it won't replace
> > + *                     them, but add new ones, possibly with a different format
> > + *                     and plane sizes
> 
> Needs some info how the driver can react and what to return.
> 
> >  * @wait_prepare:      release any locks taken while calling vb2 functions;
> >  *                     it is called before an ioctl needs to wait for a new
> >  *                     buffer to arrive; required to avoid a deadlock in
> > @@ -194,6 +198,8 @@ struct vb2_buffer {
> >  *                     each hardware operation in this callback;
> >  *                     if an error is returned, the buffer will not be queued
> >  *                     in driver; optional
> > + * @buf_submit:                called primarily to invalidate buffer caches for faster
> > + *                     consequent queuing; optional
> 
> Ditto. Also, I think it needs a more detailed explanation...

Ok, will have a look at these.

> 
> >  * @buf_finish:                called before every dequeue of the buffer back to
> >  *                     userspace; drivers may perform any operations required
> >  *                     before userspace accesses the buffer; optional
> > @@ -213,11 +219,16 @@ struct vb2_ops {
> >        int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers,
> >                           unsigned int *num_planes, unsigned long sizes[],
> >                           void *alloc_ctxs[]);
> > +       int (*queue_add)(struct vb2_queue *q,
> > +                        struct v4l2_create_buffers *create,
> > +                        unsigned int *num_buffers, unsigned int *num_planes,
> > +                        unsigned long sizes[], void *alloc_ctxs[]);
> >
> >        void (*wait_prepare)(struct vb2_queue *q);
> >        void (*wait_finish)(struct vb2_queue *q);
> >
> >        int (*buf_init)(struct vb2_buffer *vb);
> > +       int (*buf_submit)(struct vb2_buffer *vb);
> >        int (*buf_prepare)(struct vb2_buffer *vb);
> >        int (*buf_finish)(struct vb2_buffer *vb);
> >        void (*buf_cleanup)(struct vb2_buffer *vb);
> > @@ -291,6 +302,10 @@ int vb2_wait_for_all_buffers(struct vb2_queue *q);
> >  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
> >  int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req);
> >
> > +int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create);
> > +int vb2_destroy_bufs(struct vb2_queue *q, struct v4l2_buffer_span *span);
> > +int vb2_submit_buf(struct vb2_queue *q, unsigned int idx);
> > +
> >  int vb2_queue_init(struct vb2_queue *q);
> >
> >  void vb2_queue_release(struct vb2_queue *q);
> > --
> > 1.7.2.5
> >
> > --
> > 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
> >
> 
> -- 
> Best regards,
> Pawel Osciak

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Hans Verkuil]

> Hi Hans
>
> Thanks for the review
>
> On Mon, 4 Apr 2011, Hans Verkuil wrote:
>
>> On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote:
>> > A possibility to preallocate and initialise buffers of different sizes
>> > in V4L2 is required for an efficient implementation of asnapshot mode.
>> > This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
>> > VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
>> > structures.
>> >
>> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> > ---
>> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
>> >  drivers/media/video/v4l2-ioctl.c          |   43
>> +++++++++++++++++++++++++++++
>> >  include/linux/videodev2.h                 |   24 ++++++++++++++++
>> >  include/media/v4l2-ioctl.h                |    3 ++
>> >  4 files changed, 73 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
>> b/drivers/media/video/v4l2-compat-ioctl32.c
>> > index 7c26947..d71b289 100644
>> > --- a/drivers/media/video/v4l2-compat-ioctl32.c
>> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c
>> > @@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file,
>> unsigned int cmd, unsigned long arg)
>> >  	case VIDIOC_DQEVENT:
>> >  	case VIDIOC_SUBSCRIBE_EVENT:
>> >  	case VIDIOC_UNSUBSCRIBE_EVENT:
>> > +	case VIDIOC_CREATE_BUFS:
>> > +	case VIDIOC_DESTROY_BUFS:
>> > +	case VIDIOC_SUBMIT_BUF:
>> >  		ret = do_video_ioctl(file, cmd, arg);
>> >  		break;
>> >
>> > diff --git a/drivers/media/video/v4l2-ioctl.c
>> b/drivers/media/video/v4l2-ioctl.c
>> > index a01ed39..b80a211 100644
>> > --- a/drivers/media/video/v4l2-ioctl.c
>> > +++ b/drivers/media/video/v4l2-ioctl.c
>> > @@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
>> >  	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
>> >  	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
>> >  	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
>> > +	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
>> > +	[_IOC_NR(VIDIOC_DESTROY_BUFS)]     = "VIDIOC_DESTROY_BUFS",
>> > +	[_IOC_NR(VIDIOC_SUBMIT_BUF)]       = "VIDIOC_SUBMIT_BUF",
>> >  };
>> >  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>> >
>> > @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
>> >  		dbgarg(cmd, "type=0x%8.8x", sub->type);
>> >  		break;
>> >  	}
>> > +	case VIDIOC_CREATE_BUFS:
>> > +	{
>> > +		struct v4l2_create_buffers *create = arg;
>> > +
>> > +		if (!ops->vidioc_create_bufs)
>> > +			break;
>> > +		ret = check_fmt(ops, create->format.type);
>> > +		if (ret)
>> > +			break;
>> > +
>> > +		if (create->size)
>> > +			CLEAR_AFTER_FIELD(create, count);
>> > +
>> > +		ret = ops->vidioc_create_bufs(file, fh, create);
>> > +
>> > +		dbgarg(cmd, "count=%d\n", create->count);
>> > +		break;
>> > +	}
>> > +	case VIDIOC_DESTROY_BUFS:
>> > +	{
>> > +		struct v4l2_buffer_span *span = arg;
>> > +
>> > +		if (!ops->vidioc_destroy_bufs)
>> > +			break;
>> > +
>> > +		ret = ops->vidioc_destroy_bufs(file, fh, span);
>> > +
>> > +		dbgarg(cmd, "count=%d", span->count);
>> > +		break;
>> > +	}
>> > +	case VIDIOC_SUBMIT_BUF:
>> > +	{
>> > +		unsigned int *i = arg;
>> > +
>> > +		if (!ops->vidioc_submit_buf)
>> > +			break;
>> > +		ret = ops->vidioc_submit_buf(file, fh, *i);
>> > +		dbgarg(cmd, "index=%d", *i);
>> > +		break;
>> > +	}
>> >  	default:
>> >  	{
>> >  		bool valid_prio = true;
>> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>> > index aa6c393..b6ef46e 100644
>> > --- a/include/linux/videodev2.h
>> > +++ b/include/linux/videodev2.h
>> > @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
>> >  	__u32 revision;    /* chip revision, chip specific */
>> >  } __attribute__ ((packed));
>> >
>> > +/* VIDIOC_DESTROY_BUFS */
>> > +struct v4l2_buffer_span {
>> > +	__u32			index;	/* output: buffers index...index + count - 1 have
>> been created */
>> > +	__u32			count;
>> > +	__u32			reserved[2];
>> > +};
>> > +
>> > +/* struct v4l2_createbuffers::flags */
>> > +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
>>
>> We also need a FLAG_NO_CACHE_FLUSH. This is for output devices.
>
> Ok.
>
>> > +
>> > +/* VIDIOC_CREATE_BUFS */
>> > +struct v4l2_create_buffers {
>> > +	__u32			index;		/* output: buffers index...index + count - 1 have
>> been created */
>> > +	__u32			count;
>> > +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
>> > +	enum v4l2_memory        memory;
>> > +	__u32			size;		/* Explicit size, e.g., for compressed streams */
>>
>> Hmm, shouldn't this be an array of size VIDEO_MAX_PLANES?
>
> Not sure. As the comment says, this is mainly for bitstream formats. For
> any pixel-based format you really should just fill in the format below. If
> you allow the user to specify planes, then you also need to know
> alignment, contiguity, padding, etc.
>
>> > +	struct v4l2_format	format;		/* "type" is used always, the rest if
>> size == 0 */
>>
>> Needs some reserved fields as well.
>
> v4l2_format is a union with 200 bytes, is this not enough?

You can't add new fields to the v4l2_pix_format struct in this union,
because that would mess up the G/S_FBUF ioctls. Really a V4L2 design flaw.

So it's better to add some extra reserveds at the top level.

>
>> > +};
>> > +
>> >  /*
>> >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
>> >   *
>> > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
>> >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
>> v4l2_event_subscription)
>> >  #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct
>> v4l2_event_subscription)
>> >
>> > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
>> > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
>> > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
>>
>> I don't really like the name. I think I'd go for _PRE_QBUF or _PREP_BUF
>> or
>> something like that.
>
> I didn't want to use any form of prepare, because we already have a
> .buf_prepare() method, and IMHO it would be confusing. Pre-queue I didn't
> like very much either, for the same reasons, why we rejected it at the
> meeting, which was, I think, that it's not really doing anything with the
> queue, right? What this ioctl() does is it passes buffer ownership from
> the user-space to the kernel, right? Any good name for that? In fact
> "submit" doesn't sound too bad to me:-)

Well, naming isn't my strongest point. But isn't buf_prepare exactly the
op that SUBMIT is supposed to call (besides pinning the buffers in
memory)? That's the whole point of this ioctl. Anyway, I'd have to hear
what others think.

>> BTW, I agree with other reviewers that DESTROY_BUFS shouldn't leave any
>> holes.
>
> That was our decision at the meeting - I can well remember that, I was
> surprised about that too, so, I think, I even asked to clarify this point.
> But we can change it of course. Shall we return -EBUSY if the user is
> trying to free buffers in the middle?

I'm having second thoughts about this. If you have lots of buffers of
different sizes, then it might make sense to destroy buffers in the
middle.

But we should perhaps just disallow it for now. We can always be more
lenient later if necessary.

>> And if CREATE_BUFS has an error, then it shouldn't destroy all buffers,
>> but only
>> those that create_bufs managed to allocate before the error occurred.
>
> Sure, that's a bug.
>
>> Regards,
>>
>> 	Hans
>>
>> > +
>> >  /* Reminder: when adding new ioctls please add support for them to
>> >     drivers/media/video/v4l2-compat-ioctl32.c as well! */
>>
>> Don't forget this! VIDIOC_CREATE_BUFS will need to be handled in
>> compat-ioctl32.
>
> I didn't:
>
>> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
>
> Or is that not enough? Is any special handling required? AFAICS, REQBUFS
> is not treated specially either.

No, you need special handling for the 'enum memory' and the struct
v4l2_format. These have different sizes depending on the architecture.

BTW, REQBUFS and CREATE/DESTROY_BUFS should definitely co-exist. REQBUFS
is compulsory, while CREATE/DESTROY are optional.

Regards,

      Hans

>
> Thanks
> Guennadi
>
>> Regards,
>>
>> 	Hans
>>
>> >
>> > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
>> > index dd9f1e7..00962c6 100644
>> > --- a/include/media/v4l2-ioctl.h
>> > +++ b/include/media/v4l2-ioctl.h
>> > @@ -122,6 +122,9 @@ struct v4l2_ioctl_ops {
>> >  	int (*vidioc_qbuf)    (struct file *file, void *fh, struct
>> v4l2_buffer *b);
>> >  	int (*vidioc_dqbuf)   (struct file *file, void *fh, struct
>> v4l2_buffer *b);
>> >
>> > +	int (*vidioc_create_bufs) (struct file *file, void *fh, struct
>> v4l2_create_buffers *b);
>> > +	int (*vidioc_destroy_bufs)(struct file *file, void *fh, struct
>> v4l2_buffer_span *b);
>> > +	int (*vidioc_submit_buf)  (struct file *file, void *fh, unsigned int
>> i);
>> >
>> >  	int (*vidioc_overlay) (struct file *file, void *fh, unsigned int i);
>> >  	int (*vidioc_g_fbuf)   (struct file *file, void *fh,
>> >
>>
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

On Mon, 4 Apr 2011, Hans Verkuil wrote:

[snip]

> >> > +/* VIDIOC_CREATE_BUFS */
> >> > +struct v4l2_create_buffers {
> >> > +	__u32			index;		/* output: buffers index...index + count - 1 have
> >> been created */
> >> > +	__u32			count;
> >> > +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> >> > +	enum v4l2_memory        memory;
> >> > +	__u32			size;		/* Explicit size, e.g., for compressed streams */
> >>
> >> Hmm, shouldn't this be an array of size VIDEO_MAX_PLANES?
> >
> > Not sure. As the comment says, this is mainly for bitstream formats. For
> > any pixel-based format you really should just fill in the format below. If
> > you allow the user to specify planes, then you also need to know
> > alignment, contiguity, padding, etc.
> >
> >> > +	struct v4l2_format	format;		/* "type" is used always, the rest if
> >> size == 0 */
> >>
> >> Needs some reserved fields as well.
> >
> > v4l2_format is a union with 200 bytes, is this not enough?
> 
> You can't add new fields to the v4l2_pix_format struct in this union,
> because that would mess up the G/S_FBUF ioctls. Really a V4L2 design flaw.
> 
> So it's better to add some extra reserveds at the top level.

Can do that, sure, but just for understanding: as long as we're sure the 
other union members don't exceed ("a bit fewer than") 200 bytes, why 
cannot we change 200 to 196 in the union and add a reserver u32 to all 
affected ioctl()s - if / when needed?

> >> > +};
> >> > +
> >> >  /*
> >> >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> >> >   *
> >> > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> >> >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> >> v4l2_event_subscription)
> >> >  #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct
> >> v4l2_event_subscription)
> >> >
> >> > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> >> > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> >> > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> >>
> >> I don't really like the name. I think I'd go for _PRE_QBUF or _PREP_BUF
> >> or
> >> something like that.
> >
> > I didn't want to use any form of prepare, because we already have a
> > .buf_prepare() method, and IMHO it would be confusing. Pre-queue I didn't
> > like very much either, for the same reasons, why we rejected it at the
> > meeting, which was, I think, that it's not really doing anything with the
> > queue, right? What this ioctl() does is it passes buffer ownership from
> > the user-space to the kernel, right? Any good name for that? In fact
> > "submit" doesn't sound too bad to me:-)
> 
> Well, naming isn't my strongest point. But isn't buf_prepare exactly the
> op that SUBMIT is supposed to call (besides pinning the buffers in
> memory)? That's the whole point of this ioctl. Anyway, I'd have to hear
> what others think.

I've been thinking about that too. Currently .buf_prepare() is called on 
each QBUF, so, it performs a different task - a dynamic preparation, 
necessary during a running capture / playback. Whereas the new ioctl() 
should only perform a one-off operation - passing the ownership on the 
buffer. Seems pretty different to me.

> >> BTW, I agree with other reviewers that DESTROY_BUFS shouldn't leave any
> >> holes.
> >
> > That was our decision at the meeting - I can well remember that, I was
> > surprised about that too, so, I think, I even asked to clarify this point.
> > But we can change it of course. Shall we return -EBUSY if the user is
> > trying to free buffers in the middle?
> 
> I'm having second thoughts about this. If you have lots of buffers of
> different sizes, then it might make sense to destroy buffers in the
> middle.
> 
> But we should perhaps just disallow it for now. We can always be more
> lenient later if necessary.

That would be the easiest for now. Do we all agree?

> >> > +
> >> >  /* Reminder: when adding new ioctls please add support for them to
> >> >     drivers/media/video/v4l2-compat-ioctl32.c as well! */
> >>
> >> Don't forget this! VIDIOC_CREATE_BUFS will need to be handled in
> >> compat-ioctl32.
> >
> > I didn't:
> >
> >> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
> >
> > Or is that not enough? Is any special handling required? AFAICS, REQBUFS
> > is not treated specially either.
> 
> No, you need special handling for the 'enum memory' and the struct
> v4l2_format. These have different sizes depending on the architecture.

Hm, I'll have a look at them then...

> BTW, REQBUFS and CREATE/DESTROY_BUFS should definitely co-exist. REQBUFS
> is compulsory, while CREATE/DESTROY are optional.

Sure, drivers _must_ implement REQBUFS and _may_ implement CREATE/DESTROY. 
The question is - should we allow mixing of them? E.g., an app first 
calling REQBUFS, then CREATE to add more buffers, and eventually 
REQBUFS(0) to destroy them all?...

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Laurent Pinchart]

Hi Guennadi,

On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
> A possibility to preallocate and initialise buffers of different sizes
> in V4L2 is required for an efficient implementation of asnapshot mode.
> This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
> VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
> structures.

[snip]

> diff --git a/drivers/media/video/v4l2-ioctl.c
> b/drivers/media/video/v4l2-ioctl.c index a01ed39..b80a211 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c

[snip]

> @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
>  		dbgarg(cmd, "type=0x%8.8x", sub->type);
>  		break;
>  	}
> +	case VIDIOC_CREATE_BUFS:
> +	{
> +		struct v4l2_create_buffers *create = arg;
> +
> +		if (!ops->vidioc_create_bufs)
> +			break;
> +		ret = check_fmt(ops, create->format.type);
> +		if (ret)
> +			break;
> +
> +		if (create->size)
> +			CLEAR_AFTER_FIELD(create, count);

Why only when create->size is > 0 ?

> +		ret = ops->vidioc_create_bufs(file, fh, create);
> +
> +		dbgarg(cmd, "count=%d\n", create->count);
> +		break;
> +	}
> +	case VIDIOC_DESTROY_BUFS:
> +	{
> +		struct v4l2_buffer_span *span = arg;
> +
> +		if (!ops->vidioc_destroy_bufs)
> +			break;
> +
> +		ret = ops->vidioc_destroy_bufs(file, fh, span);
> +
> +		dbgarg(cmd, "count=%d", span->count);
> +		break;
> +	}
> +	case VIDIOC_SUBMIT_BUF:
> +	{
> +		unsigned int *i = arg;
> +
> +		if (!ops->vidioc_submit_buf)
> +			break;
> +		ret = ops->vidioc_submit_buf(file, fh, *i);
> +		dbgarg(cmd, "index=%d", *i);
> +		break;
> +	}
>  	default:
>  	{
>  		bool valid_prio = true;
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index aa6c393..b6ef46e 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
>  	__u32 revision;    /* chip revision, chip specific */
>  } __attribute__ ((packed));
> 
> +/* VIDIOC_DESTROY_BUFS */
> +struct v4l2_buffer_span {
> +	__u32			index;	/* output: buffers index...index + count - 1 have been
> created */ +	__u32			count;
> +	__u32			reserved[2];
> +};
> +
> +/* struct v4l2_createbuffers::flags */
> +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)

Shouldn't cache management be handled at submit/qbuf time instead of being a 
buffer property ?

> +/* VIDIOC_CREATE_BUFS */
> +struct v4l2_create_buffers {
> +	__u32			index;		/* output: buffers index...index + count - 1 have been
> created */ +	__u32			count;
> +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> +	enum v4l2_memory        memory;
> +	__u32			size;		/* Explicit size, e.g., for compressed streams */
> +	struct v4l2_format	format;		/* "type" is used always, the rest if size 
==
> 0 */ +};

You need reserved fields here.

> +
>  /*
>   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
>   *
> @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
>  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
> struct v4l2_event_subscription)
> 
> +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)

Just throwing an idea in here, what about using the same structure for both 
ioctls ? Or even a single ioctl for both create and destroy, like we do with 
REQBUFS ?

> +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> +
>  /* Reminder: when adding new ioctls please add support for them to
>     drivers/media/video/v4l2-compat-ioctl32.c as well! */
> 

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Laurent Pinchart]

On Monday 04 April 2011 10:06:47 Hans Verkuil wrote:
> > On Mon, 4 Apr 2011, Hans Verkuil wrote:
> >> On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote:

[snip]

> BTW, REQBUFS and CREATE/DESTROY_BUFS should definitely co-exist. REQBUFS
> is compulsory, while CREATE/DESTROY are optional.

Drivers must support REQBUFS and should support CREATE/DESTROY, but I think 
applications should not be allowed to mix calls.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 0/4] V4L: new ioctl()s to support multi-sized video-buffers

[Laurent Pinchart]

Hi Guennadi,

On Monday 04 April 2011 09:15:55 Guennadi Liakhovetski wrote:
> On Sun, 3 Apr 2011, Pawel Osciak wrote:
> > On Fri, Apr 1, 2011 at 01:12, Guennadi Liakhovetski wrote:
> > > Hi all
> > > 
> > > As discussed at the last V4L2 meeting in Warsaw, one of the
> > > prerequisites to support fast switching between different image
> > > formats is an ability to preallocate buffers of different sizes and
> > > handle them over to the driver in advance. This avoids the need to
> > > allocate buffers at the time of switching. This patch series is a
> > > first implementation of these ioctl()s, implemented for the
> > > sh_mobile_ceu_camera soc-camera host driver. Tested on an sh7722 migor
> > > SuperH platform. Yes, I know, documentation is missing yet;-)
> > 
> > I will have to wait for documentation before doing a full review, it's
> > hard to comment without it. Also, please mention how the new ioctls
> > influence the state machine.
> 
> Ok, I wanted to wait with the documentation until we have the API settled,
> because modifying the code is easier, than modifying the documentation:-)
> But right, I'll try to put something together.
> 
> > Some questions and doubts I'm having:
> > - Can you call CREATE more than once, before/after REQBUFS, for all
> > streaming states? What about reading/writing?
> 
> The idea was to use CREATE/DESTROY _instead_ of REQBUFS. And yes, one of
> the purposes of CREATE is to be able to call it multiple times with
> different parameters. The new API should provide at least all the
> functionality, that REQBUFS provides, i.e., you should be able to use it
> with MMAP and USERPTR memory.
> 
> > - Can driver decline CREATE if it is not supported? What if the format
> > is not supported?
> 
> Sure, if .vidioc_create_bufs() is not implemented by the driver, the
> ioctl() will just error out. Of course you're allowed to do any checks you
> see fit in your driver, like unsupported formats and return an error in
> case of a problem.
> 
> > - If we fail allocating in CREATE, should the whole queue be freed (as
> > it is done in your patch I believe)?
> 
> No, that's a bug, thanks for spotting!
> 
> > - I'm assuming REQBUFS(0) is to free buffers allocated with CREATE too?
> 
> Currently it is possible to mix CREATE/DESTROY and REQBUFS. Not sure if
> this is good, maybe we have to allow the use of only one API. I'd probably
> prefer the latter, but I'm open for suggestions here.
> 
> > - Are we allowing DESTROY to free arbitrary span of buffers (i.e.
> > those created with REQBUFS as well)?
> 
> Again, we can decide, whether we want to support mixing of these APIs or
> not.
> 
> > - Are "holes" in buffer indexes allowed? I don't like the ability to
> > free an arbitrary span of buffers in the queue, it complicates checks
> > in many places and I don't think is worth it...
> 
> That's how this ioctl() has been proposed at the Warsaw meeting.

If my memory is correct, we agreed that buffers created with a single CREATE 
call had to be freed all at once by DESTROY. This won't prevent holes though, 
as applications could call CREATE three times and then free buffers allocated 
by the second call.

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Laurent Pinchart]

On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
> A possibility to preallocate and initialise buffers of different sizes
> in V4L2 is required for an efficient implementation of asnapshot mode.
> This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
> VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
> structures.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
>  drivers/media/video/v4l2-ioctl.c          |   43
> +++++++++++++++++++++++++++++ include/linux/videodev2.h                 | 
>  24 ++++++++++++++++ include/media/v4l2-ioctl.h                |    3 ++
>  4 files changed, 73 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
> b/drivers/media/video/v4l2-compat-ioctl32.c index 7c26947..d71b289 100644
> --- a/drivers/media/video/v4l2-compat-ioctl32.c
> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> @@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned
> int cmd, unsigned long arg) case VIDIOC_DQEVENT:
>  	case VIDIOC_SUBSCRIBE_EVENT:
>  	case VIDIOC_UNSUBSCRIBE_EVENT:
> +	case VIDIOC_CREATE_BUFS:
> +	case VIDIOC_DESTROY_BUFS:
> +	case VIDIOC_SUBMIT_BUF:
>  		ret = do_video_ioctl(file, cmd, arg);
>  		break;
> 
> diff --git a/drivers/media/video/v4l2-ioctl.c
> b/drivers/media/video/v4l2-ioctl.c index a01ed39..b80a211 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
>  	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
>  	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
>  	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> +	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
> +	[_IOC_NR(VIDIOC_DESTROY_BUFS)]     = "VIDIOC_DESTROY_BUFS",
> +	[_IOC_NR(VIDIOC_SUBMIT_BUF)]       = "VIDIOC_SUBMIT_BUF",
>  };
>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> 
> @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
>  		dbgarg(cmd, "type=0x%8.8x", sub->type);
>  		break;
>  	}
> +	case VIDIOC_CREATE_BUFS:
> +	{
> +		struct v4l2_create_buffers *create = arg;
> +
> +		if (!ops->vidioc_create_bufs)
> +			break;
> +		ret = check_fmt(ops, create->format.type);
> +		if (ret)
> +			break;
> +
> +		if (create->size)
> +			CLEAR_AFTER_FIELD(create, count);
> +
> +		ret = ops->vidioc_create_bufs(file, fh, create);
> +
> +		dbgarg(cmd, "count=%d\n", create->count);
> +		break;
> +	}
> +	case VIDIOC_DESTROY_BUFS:
> +	{
> +		struct v4l2_buffer_span *span = arg;
> +
> +		if (!ops->vidioc_destroy_bufs)
> +			break;
> +
> +		ret = ops->vidioc_destroy_bufs(file, fh, span);
> +
> +		dbgarg(cmd, "count=%d", span->count);
> +		break;
> +	}
> +	case VIDIOC_SUBMIT_BUF:
> +	{
> +		unsigned int *i = arg;
> +
> +		if (!ops->vidioc_submit_buf)
> +			break;
> +		ret = ops->vidioc_submit_buf(file, fh, *i);
> +		dbgarg(cmd, "index=%d", *i);
> +		break;
> +	}
>  	default:
>  	{
>  		bool valid_prio = true;
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index aa6c393..b6ef46e 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
>  	__u32 revision;    /* chip revision, chip specific */
>  } __attribute__ ((packed));
> 
> +/* VIDIOC_DESTROY_BUFS */
> +struct v4l2_buffer_span {
> +	__u32			index;	/* output: buffers index...index + count - 1 have been
> created */ +	__u32			count;
> +	__u32			reserved[2];
> +};
> +
> +/* struct v4l2_createbuffers::flags */
> +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
> +
> +/* VIDIOC_CREATE_BUFS */
> +struct v4l2_create_buffers {
> +	__u32			index;		/* output: buffers index...index + count - 1 have been
> created */ +	__u32			count;
> +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> +	enum v4l2_memory        memory;
> +	__u32			size;		/* Explicit size, e.g., for compressed streams */
> +	struct v4l2_format	format;		/* "type" is used always, the rest if size 
==
> 0 */ +};
> +
>  /*
>   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
>   *
> @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
>  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
> struct v4l2_event_subscription)
> 
> +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> +

In case we later need to pass other information (such as flags) to 
VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.

>  /* Reminder: when adding new ioctls please add support for them to
>     drivers/media/video/v4l2-compat-ioctl32.c as well! */
> 
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index dd9f1e7..00962c6 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -122,6 +122,9 @@ struct v4l2_ioctl_ops {
>  	int (*vidioc_qbuf)    (struct file *file, void *fh, struct v4l2_buffer
> *b); int (*vidioc_dqbuf)   (struct file *file, void *fh, struct
> v4l2_buffer *b);
> 
> +	int (*vidioc_create_bufs) (struct file *file, void *fh, struct
> v4l2_create_buffers *b); +	int (*vidioc_destroy_bufs)(struct file *file,
> void *fh, struct v4l2_buffer_span *b); +	int (*vidioc_submit_buf)  (struct
> file *file, void *fh, unsigned int i);
> 
>  	int (*vidioc_overlay) (struct file *file, void *fh, unsigned int i);
>  	int (*vidioc_g_fbuf)   (struct file *file, void *fh,

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Hans Verkuil]

On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
> On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
> > A possibility to preallocate and initialise buffers of different sizes
> > in V4L2 is required for an efficient implementation of asnapshot mode.
> > This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
> > VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
> > structures.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
> >  drivers/media/video/v4l2-ioctl.c          |   43
> > +++++++++++++++++++++++++++++ include/linux/videodev2.h                 | 
> >  24 ++++++++++++++++ include/media/v4l2-ioctl.h                |    3 ++
> >  4 files changed, 73 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
> > b/drivers/media/video/v4l2-compat-ioctl32.c index 7c26947..d71b289 100644
> > --- a/drivers/media/video/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> > @@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned
> > int cmd, unsigned long arg) case VIDIOC_DQEVENT:
> >  	case VIDIOC_SUBSCRIBE_EVENT:
> >  	case VIDIOC_UNSUBSCRIBE_EVENT:
> > +	case VIDIOC_CREATE_BUFS:
> > +	case VIDIOC_DESTROY_BUFS:
> > +	case VIDIOC_SUBMIT_BUF:
> >  		ret = do_video_ioctl(file, cmd, arg);
> >  		break;
> > 
> > diff --git a/drivers/media/video/v4l2-ioctl.c
> > b/drivers/media/video/v4l2-ioctl.c index a01ed39..b80a211 100644
> > --- a/drivers/media/video/v4l2-ioctl.c
> > +++ b/drivers/media/video/v4l2-ioctl.c
> > @@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
> >  	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
> >  	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
> >  	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> > +	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
> > +	[_IOC_NR(VIDIOC_DESTROY_BUFS)]     = "VIDIOC_DESTROY_BUFS",
> > +	[_IOC_NR(VIDIOC_SUBMIT_BUF)]       = "VIDIOC_SUBMIT_BUF",
> >  };
> >  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> > 
> > @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
> >  		dbgarg(cmd, "type=0x%8.8x", sub->type);
> >  		break;
> >  	}
> > +	case VIDIOC_CREATE_BUFS:
> > +	{
> > +		struct v4l2_create_buffers *create = arg;
> > +
> > +		if (!ops->vidioc_create_bufs)
> > +			break;
> > +		ret = check_fmt(ops, create->format.type);
> > +		if (ret)
> > +			break;
> > +
> > +		if (create->size)
> > +			CLEAR_AFTER_FIELD(create, count);
> > +
> > +		ret = ops->vidioc_create_bufs(file, fh, create);
> > +
> > +		dbgarg(cmd, "count=%d\n", create->count);
> > +		break;
> > +	}
> > +	case VIDIOC_DESTROY_BUFS:
> > +	{
> > +		struct v4l2_buffer_span *span = arg;
> > +
> > +		if (!ops->vidioc_destroy_bufs)
> > +			break;
> > +
> > +		ret = ops->vidioc_destroy_bufs(file, fh, span);
> > +
> > +		dbgarg(cmd, "count=%d", span->count);
> > +		break;
> > +	}
> > +	case VIDIOC_SUBMIT_BUF:
> > +	{
> > +		unsigned int *i = arg;
> > +
> > +		if (!ops->vidioc_submit_buf)
> > +			break;
> > +		ret = ops->vidioc_submit_buf(file, fh, *i);
> > +		dbgarg(cmd, "index=%d", *i);
> > +		break;
> > +	}
> >  	default:
> >  	{
> >  		bool valid_prio = true;
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index aa6c393..b6ef46e 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
> >  	__u32 revision;    /* chip revision, chip specific */
> >  } __attribute__ ((packed));
> > 
> > +/* VIDIOC_DESTROY_BUFS */
> > +struct v4l2_buffer_span {
> > +	__u32			index;	/* output: buffers index...index + count 
- 1 have been
> > created */ +	__u32			count;
> > +	__u32			reserved[2];
> > +};
> > +
> > +/* struct v4l2_createbuffers::flags */
> > +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
> > +
> > +/* VIDIOC_CREATE_BUFS */
> > +struct v4l2_create_buffers {
> > +	__u32			index;		/* output: buffers index...index + 
count - 1 have been
> > created */ +	__u32			count;
> > +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> > +	enum v4l2_memory        memory;
> > +	__u32			size;		/* Explicit size, e.g., for 
compressed streams */
> > +	struct v4l2_format	format;		/* "type" is used always, the rest if 
size 
> ==
> > 0 */ +};
> > +
> >  /*
> >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> >   *
> > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
> > struct v4l2_event_subscription)
> > 
> > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> > +
> 
> In case we later need to pass other information (such as flags) to 
> VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.

I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF do.

Regards,

	Hans

> 
> >  /* Reminder: when adding new ioctls please add support for them to
> >     drivers/media/video/v4l2-compat-ioctl32.c as well! */
> > 
> > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> > index dd9f1e7..00962c6 100644
> > --- a/include/media/v4l2-ioctl.h
> > +++ b/include/media/v4l2-ioctl.h
> > @@ -122,6 +122,9 @@ struct v4l2_ioctl_ops {
> >  	int (*vidioc_qbuf)    (struct file *file, void *fh, struct 
v4l2_buffer
> > *b); int (*vidioc_dqbuf)   (struct file *file, void *fh, struct
> > v4l2_buffer *b);
> > 
> > +	int (*vidioc_create_bufs) (struct file *file, void *fh, struct
> > v4l2_create_buffers *b); +	int (*vidioc_destroy_bufs)(struct file *file,
> > void *fh, struct v4l2_buffer_span *b); +	int (*vidioc_submit_buf)  
(struct
> > file *file, void *fh, unsigned int i);
> > 
> >  	int (*vidioc_overlay) (struct file *file, void *fh, unsigned int i);
> >  	int (*vidioc_g_fbuf)   (struct file *file, void *fh,
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> --
> 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: [PATCH/RFC 2/4 v2] V4L: add videobuf2 helper functions to support multi-size video-buffers

[Laurent Pinchart]

Hi Guennadi,

On Friday 01 April 2011 16:06:42 Guennadi Liakhovetski wrote:
> This patch extends the videobuf2 framework with new helper functions and
> modifies existing ones to support multi-size video-buffers.

[snip]

> diff --git a/include/media/videobuf2-core.h
> b/include/media/videobuf2-core.h index f87472a..88076fc 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -177,6 +177,10 @@ struct vb2_buffer {
>   *			plane should be set in the sizes[] array and optional
>   *			per-plane allocator specific context in alloc_ctxs[]
>   *			array
> + * @queue_add:		like above, but called from VIDIOC_CREATE_BUFS, but if
> + *			there are already buffers on the queue, it won't replace
> + *			them, but add new ones, possibly with a different format
> + *			and plane sizes

I don't think drivers will need to perform different operations in queue_setup 
and queue_add. You could merge the two operations.

>   * @wait_prepare:	release any locks taken while calling vb2 functions;
>   *			it is called before an ioctl needs to wait for a new
>   *			buffer to arrive; required to avoid a deadlock in
> @@ -194,6 +198,8 @@ struct vb2_buffer {
>   *			each hardware operation in this callback;
>   *			if an error is returned, the buffer will not be queued
>   *			in driver; optional
> + * @buf_submit:		called primarily to invalidate buffer caches for faster
> + *			consequent queuing; optional

What's the difference between buf_submit and buf_prepare ?

>   * @buf_finish:		called before every dequeue of the buffer back to
>   *			userspace; drivers may perform any operations required
>   *			before userspace accesses the buffer; optional

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

Hi Laurent

On Tue, 5 Apr 2011, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
> > A possibility to preallocate and initialise buffers of different sizes
> > in V4L2 is required for an efficient implementation of asnapshot mode.
> > This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
> > VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
> > structures.
> 
> [snip]
> 
> > diff --git a/drivers/media/video/v4l2-ioctl.c
> > b/drivers/media/video/v4l2-ioctl.c index a01ed39..b80a211 100644
> > --- a/drivers/media/video/v4l2-ioctl.c
> > +++ b/drivers/media/video/v4l2-ioctl.c
> 
> [snip]
> 
> > @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
> >  		dbgarg(cmd, "type=0x%8.8x", sub->type);
> >  		break;
> >  	}
> > +	case VIDIOC_CREATE_BUFS:
> > +	{
> > +		struct v4l2_create_buffers *create = arg;
> > +
> > +		if (!ops->vidioc_create_bufs)
> > +			break;
> > +		ret = check_fmt(ops, create->format.type);
> > +		if (ret)
> > +			break;
> > +
> > +		if (create->size)
> > +			CLEAR_AFTER_FIELD(create, count);
> 
> Why only when create->size is > 0 ?

Because otherwise create->format contains the frame format, that will be 
used for plane-size calculations.

> > +		ret = ops->vidioc_create_bufs(file, fh, create);
> > +
> > +		dbgarg(cmd, "count=%d\n", create->count);
> > +		break;
> > +	}
> > +	case VIDIOC_DESTROY_BUFS:
> > +	{
> > +		struct v4l2_buffer_span *span = arg;
> > +
> > +		if (!ops->vidioc_destroy_bufs)
> > +			break;
> > +
> > +		ret = ops->vidioc_destroy_bufs(file, fh, span);
> > +
> > +		dbgarg(cmd, "count=%d", span->count);
> > +		break;
> > +	}
> > +	case VIDIOC_SUBMIT_BUF:
> > +	{
> > +		unsigned int *i = arg;
> > +
> > +		if (!ops->vidioc_submit_buf)
> > +			break;
> > +		ret = ops->vidioc_submit_buf(file, fh, *i);
> > +		dbgarg(cmd, "index=%d", *i);
> > +		break;
> > +	}
> >  	default:
> >  	{
> >  		bool valid_prio = true;
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index aa6c393..b6ef46e 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
> >  	__u32 revision;    /* chip revision, chip specific */
> >  } __attribute__ ((packed));
> > 
> > +/* VIDIOC_DESTROY_BUFS */
> > +struct v4l2_buffer_span {
> > +	__u32			index;	/* output: buffers index...index + count - 1 have been
> > created */ +	__u32			count;
> > +	__u32			reserved[2];
> > +};
> > +
> > +/* struct v4l2_createbuffers::flags */
> > +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
> 
> Shouldn't cache management be handled at submit/qbuf time instead of being a 
> buffer property ?

hmm, I'd prefer fixing it at create. Or do you want to be able to create 
buffers and then submit / queue them with different flags?...

> > +/* VIDIOC_CREATE_BUFS */
> > +struct v4l2_create_buffers {
> > +	__u32			index;		/* output: buffers index...index + count - 1 have been
> > created */ +	__u32			count;
> > +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> > +	enum v4l2_memory        memory;
> > +	__u32			size;		/* Explicit size, e.g., for compressed streams */
> > +	struct v4l2_format	format;		/* "type" is used always, the rest if size 
> ==
> > 0 */ +};
> 
> You need reserved fields here.

Yes, already discussed with Hans, will add.

> 
> > +
> >  /*
> >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> >   *
> > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
> > struct v4l2_event_subscription)
> > 
> > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> 
> Just throwing an idea in here, what about using the same structure for both 
> ioctls ? Or even a single ioctl for both create and destroy, like we do with 
> REQBUFS ?

Personally, tbh, I don't like either of them. The first one seems an 
overkill - you don't need all those fields for destroy. The second one is 
a particular case of the first one, plus it adds confusion by re-using the 
ioctl:-) Where with REQBUFS we could just set count = 0 to say - release 
all buffers, with this one we need index and count, so, we'd need one more 
flag to distinguish between create / destroy...

> > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> > +
> >  /* Reminder: when adding new ioctls please add support for them to
> >     drivers/media/video/v4l2-compat-ioctl32.c as well! */
> > 
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

On Tue, 5 Apr 2011, Laurent Pinchart wrote:

> On Monday 04 April 2011 10:06:47 Hans Verkuil wrote:
> > > On Mon, 4 Apr 2011, Hans Verkuil wrote:
> > >> On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote:
> 
> [snip]
> 
> > BTW, REQBUFS and CREATE/DESTROY_BUFS should definitely co-exist. REQBUFS
> > is compulsory, while CREATE/DESTROY are optional.
> 
> Drivers must support REQBUFS and should support CREATE/DESTROY, but I think 
> applications should not be allowed to mix calls.

So, you want to track it down in the kernel which mode is being used?... 
Hans?

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Hans Verkuil]

On Tuesday, April 05, 2011 14:02:17 Laurent Pinchart wrote:
> On Monday 04 April 2011 10:06:47 Hans Verkuil wrote:
> > > On Mon, 4 Apr 2011, Hans Verkuil wrote:
> > >> On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote:
> 
> [snip]
> 
> > BTW, REQBUFS and CREATE/DESTROY_BUFS should definitely co-exist. REQBUFS
> > is compulsory, while CREATE/DESTROY are optional.
> 
> Drivers must support REQBUFS and should support CREATE/DESTROY, but I think 
> applications should not be allowed to mix calls.

Why not? The videobuf2-core.c implementation shouldn't care about that, so
I don't see why userspace should care.

Regards,

	Hans

> 
> -- 
> Regards,
> 
> Laurent Pinchart
> --
> 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: [PATCH/RFC 2/4 v2] V4L: add videobuf2 helper functions to support multi-size video-buffers

[Laurent Pinchart]

On Monday 04 April 2011 09:55:17 Guennadi Liakhovetski wrote:
> On Sun, 3 Apr 2011, Pawel Osciak wrote:
> > On Fri, Apr 1, 2011 at 07:06, Guennadi Liakhovetski wrote:
> > > This patch extends the videobuf2 framework with new helper functions
> > > and modifies existing ones to support multi-size video-buffers.

[snip]

> > > diff --git a/drivers/media/video/videobuf2-core.c
> > > b/drivers/media/video/videobuf2-core.c index 71734a4..20e1572 100644
> > > --- a/drivers/media/video/videobuf2-core.c
> > > +++ b/drivers/media/video/videobuf2-core.c

[snip]

> > > @@ -241,16 +250,36 @@ static void __vb2_queue_free(struct vb2_queue *q)
> > >        }
> > > 
> > >        /* Release video buffer memory */
> > > -       __vb2_free_mem(q);
> > > +       __vb2_free_mem(q, span);
> > > 
> > >        /* Free videobuf buffers */
> > > -       for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> > > +       for (buffer = span->index;
> > > +            buffer < span->index + span->count; ++buffer) {
> > >                kfree(q->bufs[buffer]);
> > >                q->bufs[buffer] = NULL;
> > >        }
> > > 
> > > -       q->num_buffers = 0;
> > > -       q->memory = 0;
> > > +       q->num_buffers -= span->count;
> > > +       if (!q->num_buffers)
> > > +               q->memory = 0;
> > > +
> > > +       return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(vb2_destroy_bufs);
> > > +
> > 
> > This messes up q->num_buffers, which is used in other places as the
> > upper bound for indexes and assumes <0; q->num_buffers> is contiguous.
> > Examples include querybufs, buffers_in_use. In general, I find it
> > unnecessary complicated and dangerous to allow introducing "holes" in
> > buffer indexes.
> 
> Yes, there are issues... I think, we can just return -EBUSY on such
> fragmenting DESTROY calls. Anything left over will be freed on last
> close() anyway. Let's decide how we want to handle these and I'll prepare
> a v2, probably with documentation this time:-)

I also feel a bit uneasy about introducing holes in buffer indexes. Do we have 
use cases for that right now ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 0/4] V4L: new ioctl()s to support multi-sized video-buffers

[Guennadi Liakhovetski]

On Tue, 5 Apr 2011, Laurent Pinchart wrote:

[snip]

> > > - Are "holes" in buffer indexes allowed? I don't like the ability to
> > > free an arbitrary span of buffers in the queue, it complicates checks
> > > in many places and I don't think is worth it...
> > 
> > That's how this ioctl() has been proposed at the Warsaw meeting.
> 
> If my memory is correct, we agreed that buffers created with a single CREATE 
> call had to be freed all at once by DESTROY. This won't prevent holes though, 
> as applications could call CREATE three times and then free buffers allocated 
> by the second call.

Yes, I think, you're right. Currently I don't track those creation sets... 
Do we really want that? What does it give us?

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Laurent Pinchart]

On Tuesday 05 April 2011 14:34:57 Hans Verkuil wrote:
> On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
> > On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:

[snip]

> > > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> > > 
> > >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> > > 
> > > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
> > > struct v4l2_event_subscription)
> > > 
> > > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> > > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> > > +
> > 
> > In case we later need to pass other information (such as flags) to
> > VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
> 
> I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF
> do.

v4l2_buffer has a single reserved field (we could reclaim the input field, 
it's not used by any in-tree driver). Shouldn't we a bit more future-proof ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

On Tue, 5 Apr 2011, Hans Verkuil wrote:

> On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
> > On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:

[snip]

> > >  /*
> > >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> > >   *
> > > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> > >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> > > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
> > > struct v4l2_event_subscription)
> > > 
> > > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> > > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> > > +
> > 
> > In case we later need to pass other information (such as flags) to 
> > VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
> 
> I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF do.

Why??? You do not need all that extra information. Well, we could, of 
course, make a struct with reserved fields... but it seems nice to me to 
have the clarity here - this ioctl() _only_ gives buffer ownership to the 
kernel. No more configuration...

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Laurent Pinchart]

On Tuesday 05 April 2011 14:40:16 Hans Verkuil wrote:
> On Tuesday, April 05, 2011 14:02:17 Laurent Pinchart wrote:
> > On Monday 04 April 2011 10:06:47 Hans Verkuil wrote:
> > > > On Mon, 4 Apr 2011, Hans Verkuil wrote:
> > > >> On Friday, April 01, 2011 10:13:02 Guennadi Liakhovetski wrote:
> > [snip]
> > 
> > > BTW, REQBUFS and CREATE/DESTROY_BUFS should definitely co-exist.
> > > REQBUFS is compulsory, while CREATE/DESTROY are optional.
> > 
> > Drivers must support REQBUFS and should support CREATE/DESTROY, but I
> > think applications should not be allowed to mix calls.
> 
> Why not? The videobuf2-core.c implementation shouldn't care about that, so
> I don't see why userspace should care.

Because it makes the API semantics much more complex to define. We would have 
to precisely define interactions between REQBUFS and CREATE/DESTROY. That will 
be error-prone, for very little benefits (if any at all). If an application is 
aware of the CREATE/DESTROY ioctls and wants to use them, why would it need to 
use REQBUFS ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Laurent Pinchart]

Hi Guennadi,

On Tuesday 05 April 2011 14:39:19 Guennadi Liakhovetski wrote:
> On Tue, 5 Apr 2011, Laurent Pinchart wrote:
> > On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
> > > A possibility to preallocate and initialise buffers of different sizes
> > > in V4L2 is required for an efficient implementation of asnapshot mode.
> > > This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
> > > VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
> > > structures.

[snip]

> > > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > > index aa6c393..b6ef46e 100644
> > > --- a/include/linux/videodev2.h
> > > +++ b/include/linux/videodev2.h
> > > @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {

[snip]

> > > +/* struct v4l2_createbuffers::flags */
> > > +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
> > 
> > Shouldn't cache management be handled at submit/qbuf time instead of
> > being a buffer property ?
> 
> hmm, I'd prefer fixing it at create. Or do you want to be able to create
> buffers and then submit / queue them with different flags?...

That's the idea, yes. I'm not sure yet how useful that would be though.

[snip]

> > > +
> > > 
> > >  /*
> > >  
> > >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> > >   *
> > > 
> > > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> > > 
> > >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> > > 
> > > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
> > > struct v4l2_event_subscription)
> > > 
> > > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> > 
> > Just throwing an idea in here, what about using the same structure for
> > both ioctls ? Or even a single ioctl for both create and destroy, like
> > we do with REQBUFS ?
> 
> Personally, tbh, I don't like either of them. The first one seems an
> overkill - you don't need all those fields for destroy. The second one is
> a particular case of the first one, plus it adds confusion by re-using the
> ioctl:-) Where with REQBUFS we could just set count = 0 to say - release
> all buffers, with this one we need index and count, so, we'd need one more
> flag to distinguish between create / destroy...

OK, idea dismissed :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Laurent Pinchart]

On Tuesday 05 April 2011 14:52:20 Guennadi Liakhovetski wrote:
> On Tue, 5 Apr 2011, Hans Verkuil wrote:
> > On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
> > > On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
> [snip]
> 
> > > >  /*
> > > >  
> > > >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> > > >   *
> > > > 
> > > > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> > > > 
> > > >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> > > > 
> > > > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V',
> > > > 91, struct v4l2_event_subscription)
> > > > 
> > > > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct
> > > > v4l2_create_buffers) +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93,
> > > > struct v4l2_buffer_span) +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94,
> > > > int)
> > > > +
> > > 
> > > In case we later need to pass other information (such as flags) to
> > > VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
> > 
> > I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF
> > do.
> 
> Why??? You do not need all that extra information. Well, we could, of
> course, make a struct with reserved fields... but it seems nice to me to
> have the clarity here - this ioctl() _only_ gives buffer ownership to the
> kernel. No more configuration...

Right now you're correct. In the future we might need extra fields, so we need 
a structure with reserved fields.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 2/4 v2] V4L: add videobuf2 helper functions to support multi-size video-buffers

[Guennadi Liakhovetski]

On Tue, 5 Apr 2011, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Friday 01 April 2011 16:06:42 Guennadi Liakhovetski wrote:
> > This patch extends the videobuf2 framework with new helper functions and
> > modifies existing ones to support multi-size video-buffers.
> 
> [snip]
> 
> > diff --git a/include/media/videobuf2-core.h
> > b/include/media/videobuf2-core.h index f87472a..88076fc 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -177,6 +177,10 @@ struct vb2_buffer {
> >   *			plane should be set in the sizes[] array and optional
> >   *			per-plane allocator specific context in alloc_ctxs[]
> >   *			array
> > + * @queue_add:		like above, but called from VIDIOC_CREATE_BUFS, but if
> > + *			there are already buffers on the queue, it won't replace
> > + *			them, but add new ones, possibly with a different format
> > + *			and plane sizes
> 
> I don't think drivers will need to perform different operations in queue_setup 
> and queue_add. You could merge the two operations.

In principle yes. That's also what I ended up doing internally in the 
sh-mobile driver. Would have to change existing drivers then...

> >   * @wait_prepare:	release any locks taken while calling vb2 functions;
> >   *			it is called before an ioctl needs to wait for a new
> >   *			buffer to arrive; required to avoid a deadlock in
> > @@ -194,6 +198,8 @@ struct vb2_buffer {
> >   *			each hardware operation in this callback;
> >   *			if an error is returned, the buffer will not be queued
> >   *			in driver; optional
> > + * @buf_submit:		called primarily to invalidate buffer caches for faster
> > + *			consequent queuing; optional
> 
> What's the difference between buf_submit and buf_prepare ?

My confusion;) Clarified this with Hans too already.

> >   * @buf_finish:		called before every dequeue of the buffer back to
> >   *			userspace; drivers may perform any operations required
> >   *			before userspace accesses the buffer; optional
> 
> -- 
> Regards,
> 
> Laurent Pinchart

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi Guennadi,

Hi all,

> On Tuesday 05 April 2011 14:39:19 Guennadi Liakhovetski wrote:
>> On Tue, 5 Apr 2011, Laurent Pinchart wrote:
>>> On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
>>>> A possibility to preallocate and initialise buffers of different sizes
>>>> in V4L2 is required for an efficient implementation of asnapshot mode.
>>>> This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
>>>> VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
>>>> structures.
> 
> [snip]
> 
>>>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>>>> index aa6c393..b6ef46e 100644
>>>> --- a/include/linux/videodev2.h
>>>> +++ b/include/linux/videodev2.h
>>>> @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
> 
> [snip]
> 
>>>> +/* struct v4l2_createbuffers::flags */
>>>> +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
>>>
>>> Shouldn't cache management be handled at submit/qbuf time instead of
>>> being a buffer property ?
>>
>> hmm, I'd prefer fixing it at create. Or do you want to be able to create
>> buffers and then submit / queue them with different flags?...
> 
> That's the idea, yes. I'm not sure yet how useful that would be though.

I agree that it'd be nice to support this. It depends on where the data
is being used.

For example, you could have an algorithm on the host side which does
process the image data but only uses every second frame, with no other
processing done on the host CPU. In this case the cache would be flushed
needlessly for the frames that are not touched by the CPU.

I admit that this is fine optimisation but I don't think that should be
ruled out either.

The default cache behaviour would still be to flush not to break
existing applications, so I don't think this would be a significant
burden for applications.

Cheers,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

On Tue, 5 Apr 2011, Hans Verkuil wrote:

> On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
> > On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:

[snip]

> > >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> > >   *
> > > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> > >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> > > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
> > > struct v4l2_event_subscription)
> > > 
> > > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> > > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> > > +
> > 
> > In case we later need to pass other information (such as flags) to 
> > VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
> 
> I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF do.

As I said, I didn't like this very much, because it involves redundant 
data, but if we want to call .buf_prepare() from it, then we need 
v4l2_buffer...

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Hans Verkuil]

On Wednesday, April 06, 2011 18:19:18 Guennadi Liakhovetski wrote:
> On Tue, 5 Apr 2011, Hans Verkuil wrote:
> 
> > On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
> > > On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
> 
> [snip]
> 
> > > >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> > > >   *
> > > > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> > > >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> > > > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
> > > > struct v4l2_event_subscription)
> > > > 
> > > > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > > > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> > > > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> > > > +
> > > 
> > > In case we later need to pass other information (such as flags) to 
> > > VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
> > 
> > I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF do.
> 
> As I said, I didn't like this very much, because it involves redundant 
> data, but if we want to call .buf_prepare() from it, then we need 
> v4l2_buffer...

I don't see a problem with this. Applications already *have* the v4l2_buffer
after all. It's not as if they have to fill that structure just for this call.

Furthermore, you need all that data anyway because you need to do the same
checks that vb2_qbuf does.

Regarding DESTROY_BUFS: perhaps we should just skip this for now and wait for
the first use-case. That way we don't need to care about holes. I don't like
artificial restrictions like 'no holes'. If someone has a good use-case for
selectively destroying buffers, then we need to look at this again.

Regards,

	Hans

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

On Thu, 7 Apr 2011, Hans Verkuil wrote:

> On Wednesday, April 06, 2011 18:19:18 Guennadi Liakhovetski wrote:
> > On Tue, 5 Apr 2011, Hans Verkuil wrote:
> > 
> > > On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
> > > > On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
> > 
> > [snip]
> > 
> > > > >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> > > > >   *
> > > > > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> > > > >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> > > > > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
> > > > > struct v4l2_event_subscription)
> > > > > 
> > > > > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > > > > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> > > > > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> > > > > +
> > > > 
> > > > In case we later need to pass other information (such as flags) to 
> > > > VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
> > > 
> > > I would just pass struct v4l2_buffer to this ioctl, just like QBUF/DQBUF do.
> > 
> > As I said, I didn't like this very much, because it involves redundant 
> > data, but if we want to call .buf_prepare() from it, then we need 
> > v4l2_buffer...
> 
> I don't see a problem with this. Applications already *have* the v4l2_buffer
> after all. It's not as if they have to fill that structure just for this call.
> 
> Furthermore, you need all that data anyway because you need to do the same
> checks that vb2_qbuf does.
> 
> Regarding DESTROY_BUFS: perhaps we should just skip this for now and wait for
> the first use-case. That way we don't need to care about holes. I don't like
> artificial restrictions like 'no holes'. If someone has a good use-case for
> selectively destroying buffers, then we need to look at this again.

Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) / 
close()?

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Hans Verkuil]

> On Thu, 7 Apr 2011, Hans Verkuil wrote:
>
>> On Wednesday, April 06, 2011 18:19:18 Guennadi Liakhovetski wrote:
>> > On Tue, 5 Apr 2011, Hans Verkuil wrote:
>> >
>> > > On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
>> > > > On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
>> >
>> > [snip]
>> >
>> > > > >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
>> > > > >   *
>> > > > > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
>> > > > >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
>> > > > > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT
>> _IOW('V', 91,
>> > > > > struct v4l2_event_subscription)
>> > > > >
>> > > > > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct
>> v4l2_create_buffers)
>> > > > > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct
>> v4l2_buffer_span)
>> > > > > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
>> > > > > +
>> > > >
>> > > > In case we later need to pass other information (such as flags) to
>> > > > VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
>> > >
>> > > I would just pass struct v4l2_buffer to this ioctl, just like
>> QBUF/DQBUF do.
>> >
>> > As I said, I didn't like this very much, because it involves redundant
>> > data, but if we want to call .buf_prepare() from it, then we need
>> > v4l2_buffer...
>>
>> I don't see a problem with this. Applications already *have* the
>> v4l2_buffer
>> after all. It's not as if they have to fill that structure just for this
>> call.
>>
>> Furthermore, you need all that data anyway because you need to do the
>> same
>> checks that vb2_qbuf does.
>>
>> Regarding DESTROY_BUFS: perhaps we should just skip this for now and
>> wait for
>> the first use-case. That way we don't need to care about holes. I don't
>> like
>> artificial restrictions like 'no holes'. If someone has a good use-case
>> for
>> selectively destroying buffers, then we need to look at this again.
>
> Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) /
> close()?

Yes.

       Hans


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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

On Thu, 7 Apr 2011, Hans Verkuil wrote:

> > On Thu, 7 Apr 2011, Hans Verkuil wrote:
> >
> >> On Wednesday, April 06, 2011 18:19:18 Guennadi Liakhovetski wrote:
> >> > On Tue, 5 Apr 2011, Hans Verkuil wrote:
> >> >
> >> > > On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
> >> > > > On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
> >> >
> >> > [snip]
> >> >
> >> > > > >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> >> > > > >   *
> >> > > > > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> >> > > > >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> >> > > > > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT
> >> _IOW('V', 91,
> >> > > > > struct v4l2_event_subscription)
> >> > > > >
> >> > > > > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct
> >> v4l2_create_buffers)
> >> > > > > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct
> >> v4l2_buffer_span)
> >> > > > > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> >> > > > > +
> >> > > >
> >> > > > In case we later need to pass other information (such as flags) to
> >> > > > VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
> >> > >
> >> > > I would just pass struct v4l2_buffer to this ioctl, just like
> >> QBUF/DQBUF do.
> >> >
> >> > As I said, I didn't like this very much, because it involves redundant
> >> > data, but if we want to call .buf_prepare() from it, then we need
> >> > v4l2_buffer...
> >>
> >> I don't see a problem with this. Applications already *have* the
> >> v4l2_buffer
> >> after all. It's not as if they have to fill that structure just for this
> >> call.
> >>
> >> Furthermore, you need all that data anyway because you need to do the
> >> same
> >> checks that vb2_qbuf does.
> >>
> >> Regarding DESTROY_BUFS: perhaps we should just skip this for now and
> >> wait for
> >> the first use-case. That way we don't need to care about holes. I don't
> >> like
> >> artificial restrictions like 'no holes'. If someone has a good use-case
> >> for
> >> selectively destroying buffers, then we need to look at this again.
> >
> > Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) /
> > close()?
> 
> Yes.

Ok, how about this: I remove the ioctl definition and struct 
v4l2_ioctl_ops callback for it, but I keep the span struct and the vb2 
implementation - in case we need it later? The vb2_destroy_bufs() will be 
used to implement freeing the queue as a particular case of freeing some 
buffers.

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Hans Verkuil]

On Thursday, April 07, 2011 10:53:32 Guennadi Liakhovetski wrote:
> On Thu, 7 Apr 2011, Hans Verkuil wrote:
> 
> > > On Thu, 7 Apr 2011, Hans Verkuil wrote:
> > >
> > >> On Wednesday, April 06, 2011 18:19:18 Guennadi Liakhovetski wrote:
> > >> > On Tue, 5 Apr 2011, Hans Verkuil wrote:
> > >> >
> > >> > > On Tuesday, April 05, 2011 14:21:03 Laurent Pinchart wrote:
> > >> > > > On Friday 01 April 2011 10:13:02 Guennadi Liakhovetski wrote:
> > >> >
> > >> > [snip]
> > >> >
> > >> > > > >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> > >> > > > >   *
> > >> > > > > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> > >> > > > >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> > >> > > > > v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT
> > >> _IOW('V', 91,
> > >> > > > > struct v4l2_event_subscription)
> > >> > > > >
> > >> > > > > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct
> > >> v4l2_create_buffers)
> > >> > > > > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct
> > >> v4l2_buffer_span)
> > >> > > > > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> > >> > > > > +
> > >> > > >
> > >> > > > In case we later need to pass other information (such as flags) 
to
> > >> > > > VIDIOC_SUBMIT_BUF, you should use a structure instead of an int.
> > >> > >
> > >> > > I would just pass struct v4l2_buffer to this ioctl, just like
> > >> QBUF/DQBUF do.
> > >> >
> > >> > As I said, I didn't like this very much, because it involves 
redundant
> > >> > data, but if we want to call .buf_prepare() from it, then we need
> > >> > v4l2_buffer...
> > >>
> > >> I don't see a problem with this. Applications already *have* the
> > >> v4l2_buffer
> > >> after all. It's not as if they have to fill that structure just for 
this
> > >> call.
> > >>
> > >> Furthermore, you need all that data anyway because you need to do the
> > >> same
> > >> checks that vb2_qbuf does.
> > >>
> > >> Regarding DESTROY_BUFS: perhaps we should just skip this for now and
> > >> wait for
> > >> the first use-case. That way we don't need to care about holes. I don't
> > >> like
> > >> artificial restrictions like 'no holes'. If someone has a good use-case
> > >> for
> > >> selectively destroying buffers, then we need to look at this again.
> > >
> > > Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) /
> > > close()?
> > 
> > Yes.
> 
> Ok, how about this: I remove the ioctl definition and struct 
> v4l2_ioctl_ops callback for it, but I keep the span struct and the vb2 
> implementation - in case we need it later? The vb2_destroy_bufs() will be 
> used to implement freeing the queue as a particular case of freeing some 
> buffers.

I wouldn't do that. Just keep the code clean and the patch as small as 
possible. Having unused/unnecessary code is bad practice.

Regards,

        Hans

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Laurent Pinchart]

Hi Hans,

On Thursday 07 April 2011 09:50:13 Hans Verkuil wrote:
> > On Thu, 7 Apr 2011, Hans Verkuil wrote:

[snip]

> >> Regarding DESTROY_BUFS: perhaps we should just skip this for now and wait
> >> for the first use-case. That way we don't need to care about holes. I
> >> don't like artificial restrictions like 'no holes'. If someone has a good
> >> use-case for selectively destroying buffers, then we need to look at this
> >> again.
> > 
> > Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) /
> > close()?
> 
> Yes.

I don't really like that as it would mix CREATE and REQBUFS calls. 
Applications should either use the old API (REQBUFS) or the new one, but not 
mix both.

The fact that freeing arbitrary spans of buffers gives us uneasy feelings 
might be a sign that the CREATE/DESTROY API is not mature enough. I'd rather 
try to solve the issue now instead of postponing it for later and discover 
that our CREATE API should have been different.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Hans Verkuil]

> Hi Hans,
>
> On Thursday 07 April 2011 09:50:13 Hans Verkuil wrote:
>> > On Thu, 7 Apr 2011, Hans Verkuil wrote:
>
> [snip]
>
>> >> Regarding DESTROY_BUFS: perhaps we should just skip this for now and
>> wait
>> >> for the first use-case. That way we don't need to care about holes. I
>> >> don't like artificial restrictions like 'no holes'. If someone has a
>> good
>> >> use-case for selectively destroying buffers, then we need to look at
>> this
>> >> again.
>> >
>> > Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) /
>> > close()?
>>
>> Yes.
>
> I don't really like that as it would mix CREATE and REQBUFS calls.
> Applications should either use the old API (REQBUFS) or the new one, but
> not
> mix both.

That's a completely unnecessary limitation. And from the point of view of
vb2 it shouldn't even matter.

> The fact that freeing arbitrary spans of buffers gives us uneasy feelings
> might be a sign that the CREATE/DESTROY API is not mature enough. I'd
> rather
> try to solve the issue now instead of postponing it for later and discover
> that our CREATE API should have been different.

What gives me an uneasy feeling is prohibiting freeing arbitrary spans of
buffers. I rather choose not to implement the DESTROY ioctl instead of
implementing a limited version of it, also because we do not have proper
use cases yet. But I have no problems with the CREATE/DESTROY API as such.

Regards,

       Hans

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Sakari Ailus]

Hi Guennadi,

Thanks for the RFC! I have a few comments below.

Guennadi Liakhovetski wrote:
> A possibility to preallocate and initialise buffers of different sizes
> in V4L2 is required for an efficient implementation of asnapshot mode.
> This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
> VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
> structures.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
>  drivers/media/video/v4l2-ioctl.c          |   43 +++++++++++++++++++++++++++++
>  include/linux/videodev2.h                 |   24 ++++++++++++++++
>  include/media/v4l2-ioctl.h                |    3 ++
>  4 files changed, 73 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
> index 7c26947..d71b289 100644
> --- a/drivers/media/video/v4l2-compat-ioctl32.c
> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> @@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
>  	case VIDIOC_DQEVENT:
>  	case VIDIOC_SUBSCRIBE_EVENT:
>  	case VIDIOC_UNSUBSCRIBE_EVENT:
> +	case VIDIOC_CREATE_BUFS:
> +	case VIDIOC_DESTROY_BUFS:
> +	case VIDIOC_SUBMIT_BUF:
>  		ret = do_video_ioctl(file, cmd, arg);
>  		break;
>  
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index a01ed39..b80a211 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
>  	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
>  	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
>  	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> +	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
> +	[_IOC_NR(VIDIOC_DESTROY_BUFS)]     = "VIDIOC_DESTROY_BUFS",
> +	[_IOC_NR(VIDIOC_SUBMIT_BUF)]       = "VIDIOC_SUBMIT_BUF",
>  };
>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>  
> @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
>  		dbgarg(cmd, "type=0x%8.8x", sub->type);
>  		break;
>  	}
> +	case VIDIOC_CREATE_BUFS:
> +	{
> +		struct v4l2_create_buffers *create = arg;
> +
> +		if (!ops->vidioc_create_bufs)
> +			break;
> +		ret = check_fmt(ops, create->format.type);
> +		if (ret)
> +			break;
> +
> +		if (create->size)
> +			CLEAR_AFTER_FIELD(create, count);
> +
> +		ret = ops->vidioc_create_bufs(file, fh, create);
> +
> +		dbgarg(cmd, "count=%d\n", create->count);
> +		break;
> +	}
> +	case VIDIOC_DESTROY_BUFS:
> +	{
> +		struct v4l2_buffer_span *span = arg;
> +
> +		if (!ops->vidioc_destroy_bufs)
> +			break;
> +
> +		ret = ops->vidioc_destroy_bufs(file, fh, span);
> +
> +		dbgarg(cmd, "count=%d", span->count);
> +		break;
> +	}
> +	case VIDIOC_SUBMIT_BUF:
> +	{
> +		unsigned int *i = arg;
> +
> +		if (!ops->vidioc_submit_buf)
> +			break;
> +		ret = ops->vidioc_submit_buf(file, fh, *i);
> +		dbgarg(cmd, "index=%d", *i);
> +		break;
> +	}
>  	default:
>  	{
>  		bool valid_prio = true;
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index aa6c393..b6ef46e 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
>  	__u32 revision;    /* chip revision, chip specific */
>  } __attribute__ ((packed));
>  
> +/* VIDIOC_DESTROY_BUFS */
> +struct v4l2_buffer_span {
> +	__u32			index;	/* output: buffers index...index + count - 1 have been created */
> +	__u32			count;
> +	__u32			reserved[2];
> +};
> +
> +/* struct v4l2_createbuffers::flags */
> +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
> +
> +/* VIDIOC_CREATE_BUFS */
> +struct v4l2_create_buffers {
> +	__u32			index;		/* output: buffers index...index + count - 1 have been created */
> +	__u32			count;
> +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> +	enum v4l2_memory        memory;
> +	__u32			size;		/* Explicit size, e.g., for compressed streams */
> +	struct v4l2_format	format;		/* "type" is used always, the rest if size == 0 */
> +};

This assumes that the buffer indices that are returned by these ioctls
will be incremented beyond V4L2_MAX_FRAME. Don't you think this is an issue?

Proper handling of this still requires that once the count reaches
UINT_MAX, you do check that the buffer indices actually are available.
This might not be easier than keeping the range as contiguous as
possible and returning a set of buffer indices to the user. This would
change how the user needs to handle the buffer ids, so essentially
VIDIOC_{CREATE,DESTROY}_BUFS would operate on a array of buffer indices
rather than a single index.

I'm not fully convinced we absolutely must be able to operate on
multiple buffers at once either; most operations always work on single
buffers anyway.

Just my 0,05 euros --- as we have no smaller coins in here. :-)

>  /*
>   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
>   *
> @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
>  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct v4l2_event_subscription)
>  #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
>  
> +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> +
>  /* Reminder: when adding new ioctls please add support for them to
>     drivers/media/video/v4l2-compat-ioctl32.c as well! */
>  
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index dd9f1e7..00962c6 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -122,6 +122,9 @@ struct v4l2_ioctl_ops {
>  	int (*vidioc_qbuf)    (struct file *file, void *fh, struct v4l2_buffer *b);
>  	int (*vidioc_dqbuf)   (struct file *file, void *fh, struct v4l2_buffer *b);
>  
> +	int (*vidioc_create_bufs) (struct file *file, void *fh, struct v4l2_create_buffers *b);
> +	int (*vidioc_destroy_bufs)(struct file *file, void *fh, struct v4l2_buffer_span *b);
> +	int (*vidioc_submit_buf)  (struct file *file, void *fh, unsigned int i);
>  
>  	int (*vidioc_overlay) (struct file *file, void *fh, unsigned int i);
>  	int (*vidioc_g_fbuf)   (struct file *file, void *fh,

Cheers,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Sakari Ailus]

Hi Hans,

Hans Verkuil wrote:
>> Hi Hans,
>>
>> On Thursday 07 April 2011 09:50:13 Hans Verkuil wrote:
>>>> On Thu, 7 Apr 2011, Hans Verkuil wrote:
>>
>> [snip]
>>
>>>>> Regarding DESTROY_BUFS: perhaps we should just skip this for now and
>>> wait
>>>>> for the first use-case. That way we don't need to care about holes. I
>>>>> don't like artificial restrictions like 'no holes'. If someone has a
>>> good
>>>>> use-case for selectively destroying buffers, then we need to look at
>>> this
>>>>> again.
>>>>
>>>> Sorry, skip what? skip the ioctl completely and rely on REQBUFS(0) /
>>>> close()?
>>>
>>> Yes.
>>
>> I don't really like that as it would mix CREATE and REQBUFS calls.
>> Applications should either use the old API (REQBUFS) or the new one, but
>> not
>> mix both.
> 
> That's a completely unnecessary limitation. And from the point of view of
> vb2 it shouldn't even matter.

If calls to {CREATE,DESTROY}_BUF and REQBUFS are allowed to be mixed, it
would be nice to define the API so that one could implement REQBUFS
using CREATE_BUFS and DESTROY_BUFS. Then, drivers would not need to
implement REQBUFS separately which would still be used by majority of
applications. And Videobuf2 wouldn't need to implement REQBUFS at all.

Would this require more than to require buffer indices starting from
zero and be contiguous when there are no existing allocations?

The spec says under VIDIOC_QBUF that "Valid index numbers range from
zero to the number of buffers allocated with VIDIOC_REQBUFS (struct
v4l2_requestbuffers count) minus one."

>> The fact that freeing arbitrary spans of buffers gives us uneasy feelings
>> might be a sign that the CREATE/DESTROY API is not mature enough. I'd
>> rather
>> try to solve the issue now instead of postponing it for later and discover
>> that our CREATE API should have been different.
> 
> What gives me an uneasy feeling is prohibiting freeing arbitrary spans of
> buffers. I rather choose not to implement the DESTROY ioctl instead of
> implementing a limited version of it, also because we do not have proper
> use cases yet. But I have no problems with the CREATE/DESTROY API as such.

What would you think about using an array of index numbers rather than a
range for both? One must manage index number allocation even when using
a range and it might not be easier than to allocate all buffers from a
relatively small range (e.g. index numbers from 0 to 63), whose
implementation could be a bit field.

Cheers,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

I've found some more time to get back to this. Let me try to recap, what 
has been discussed. I've looked through all replies again (thanks to 
all!), so, I'll present a summary. Any mistakes and misinterpretations are 
mine;) If I misunderstand someone or forget anything - please, shout!

On Fri, 1 Apr 2011, Guennadi Liakhovetski wrote:

> A possibility to preallocate and initialise buffers of different sizes
> in V4L2 is required for an efficient implementation of asnapshot mode.
> This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
> VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
> structures.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
>  drivers/media/video/v4l2-ioctl.c          |   43 +++++++++++++++++++++++++++++
>  include/linux/videodev2.h                 |   24 ++++++++++++++++
>  include/media/v4l2-ioctl.h                |    3 ++
>  4 files changed, 73 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
> index 7c26947..d71b289 100644
> --- a/drivers/media/video/v4l2-compat-ioctl32.c
> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> @@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
>  	case VIDIOC_DQEVENT:
>  	case VIDIOC_SUBSCRIBE_EVENT:
>  	case VIDIOC_UNSUBSCRIBE_EVENT:
> +	case VIDIOC_CREATE_BUFS:
> +	case VIDIOC_DESTROY_BUFS:
> +	case VIDIOC_SUBMIT_BUF:
>  		ret = do_video_ioctl(file, cmd, arg);
>  		break;
>  
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index a01ed39..b80a211 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
>  	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
>  	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
>  	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> +	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
> +	[_IOC_NR(VIDIOC_DESTROY_BUFS)]     = "VIDIOC_DESTROY_BUFS",
> +	[_IOC_NR(VIDIOC_SUBMIT_BUF)]       = "VIDIOC_SUBMIT_BUF",
>  };
>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>  
> @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
>  		dbgarg(cmd, "type=0x%8.8x", sub->type);
>  		break;
>  	}
> +	case VIDIOC_CREATE_BUFS:
> +	{
> +		struct v4l2_create_buffers *create = arg;
> +
> +		if (!ops->vidioc_create_bufs)
> +			break;
> +		ret = check_fmt(ops, create->format.type);
> +		if (ret)
> +			break;
> +
> +		if (create->size)
> +			CLEAR_AFTER_FIELD(create, count);
> +
> +		ret = ops->vidioc_create_bufs(file, fh, create);
> +
> +		dbgarg(cmd, "count=%d\n", create->count);
> +		break;
> +	}
> +	case VIDIOC_DESTROY_BUFS:
> +	{
> +		struct v4l2_buffer_span *span = arg;
> +
> +		if (!ops->vidioc_destroy_bufs)
> +			break;
> +
> +		ret = ops->vidioc_destroy_bufs(file, fh, span);
> +
> +		dbgarg(cmd, "count=%d", span->count);
> +		break;
> +	}
> +	case VIDIOC_SUBMIT_BUF:
> +	{
> +		unsigned int *i = arg;
> +
> +		if (!ops->vidioc_submit_buf)
> +			break;
> +		ret = ops->vidioc_submit_buf(file, fh, *i);
> +		dbgarg(cmd, "index=%d", *i);
> +		break;
> +	}
>  	default:
>  	{
>  		bool valid_prio = true;
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index aa6c393..b6ef46e 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
>  	__u32 revision;    /* chip revision, chip specific */
>  } __attribute__ ((packed));
>  
> +/* VIDIOC_DESTROY_BUFS */
> +struct v4l2_buffer_span {
> +	__u32			index;	/* output: buffers index...index + count - 1 have been created */
> +	__u32			count;
> +	__u32			reserved[2];
> +};
> +
> +/* struct v4l2_createbuffers::flags */
> +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)

1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.

2. Both these flags should not be passed with CREATE, but with SUBMIT 
(which will be renamed to PREPARE or something similar). It should be 
possible to prepare the same buffer with different cacheing attributes 
during a running operation. Shall these flags be added to values, taken by 
struct v4l2_buffer::flags, since that is the struct, that will be used as 
the argument for the new version of the SUBMIT ioctl()?

> +
> +/* VIDIOC_CREATE_BUFS */
> +struct v4l2_create_buffers {
> +	__u32			index;		/* output: buffers index...index + count - 1 have been created */
> +	__u32			count;
> +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> +	enum v4l2_memory        memory;
> +	__u32			size;		/* Explicit size, e.g., for compressed streams */
> +	struct v4l2_format	format;		/* "type" is used always, the rest if size == 0 */
> +};

1. Care must be taken to keep index <= V4L2_MAX_FRAME

2. A reserved field is needed.

> +
>  /*
>   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
>   *
> @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
>  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct v4l2_event_subscription)
>  #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
>  
> +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)

This has become the hottest point for discussion.

1. VIDIOC_CREATE_BUFS: should the REQBUFS and CREATE/DESTROY APIs be 
allowed to be mixed? REQBUFS is compulsory, CREATE/DESTROY will be 
optional. But shall applications be allowed to mix them? No consensus has 
been riched. This will also depend on whether DESTROY will be implemented 
at all (see below).

2. VIDIOC_DESTROY_BUFS: has been discussed a lot

(a) shall it be allowed to create holes in indices? agreement was: not at 
this stage, but in the future this might be needed.

(b) ioctl() argument: shall it take a span or an array of indices? I don't 
think arrays make any sense here: on CREATE you always get contiguous 
index sequences, and you are only allowed to DESTROY the same index sets. 

(c) shall it be implemented at all, now that we don't know, how to handle 
holes, or shall we just continue using REQBUFS(0) or close() to release 
all buffers at once? Not implementing DESTROY now has the disadvantage, 
that if you allocate 2 buffer sets of sizes A (small) and B (big), and 
then don't need B any more, but instead need C != B (big), you cannot do 
this. But this is just one of hypothetical use-cases. No consensus 
reached.

3. VIDIOC_SUBMIT_BUF:

(a) shall be renamed to something with prepare or pre-queue in the name 
and call the .buf_prepare() videobuf2 method. This hasn't raised any 
objections, has been implemented in v2 (has not been posted yet). Name 
will be changed to VIDIOC_PREPARE_BUF

(b) Proposed to use struct v4l2_buffer as the argument. Applications 
anyway need those structs for other ioctl()s and the information is needed 
for .buf_prepare(). This is done in v2.

4. It has been proposed to create wrappers to allow drivers to only 
implement CREATE/DESTROY and have those wrappers also provide REQBUFS, 
using them.

> +
>  /* Reminder: when adding new ioctls please add support for them to
>     drivers/media/video/v4l2-compat-ioctl32.c as well! */

1. 'enum memory' and struct v4l2_format need special handling. Fixed in 
v2, untested.

>  
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index dd9f1e7..00962c6 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -122,6 +122,9 @@ struct v4l2_ioctl_ops {
>  	int (*vidioc_qbuf)    (struct file *file, void *fh, struct v4l2_buffer *b);
>  	int (*vidioc_dqbuf)   (struct file *file, void *fh, struct v4l2_buffer *b);
>  
> +	int (*vidioc_create_bufs) (struct file *file, void *fh, struct v4l2_create_buffers *b);
> +	int (*vidioc_destroy_bufs)(struct file *file, void *fh, struct v4l2_buffer_span *b);
> +	int (*vidioc_submit_buf)  (struct file *file, void *fh, unsigned int i);
>  
>  	int (*vidioc_overlay) (struct file *file, void *fh, unsigned int i);
>  	int (*vidioc_g_fbuf)   (struct file *file, void *fh,
> -- 
> 1.7.2.5

Personally, I think, one of viable solutions for now would be to implement

> +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> +#define VIDIOC_PREPARE_BUF	 _IOW('V', 93, struct v4l2_buffer)

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Hans Verkuil]

On Friday, May 13, 2011 09:45:51 Guennadi Liakhovetski wrote:
> I've found some more time to get back to this. Let me try to recap, what 
> has been discussed. I've looked through all replies again (thanks to 
> all!), so, I'll present a summary. Any mistakes and misinterpretations are 
> mine;) If I misunderstand someone or forget anything - please, shout!
> 
> On Fri, 1 Apr 2011, Guennadi Liakhovetski wrote:
> 
> > A possibility to preallocate and initialise buffers of different sizes
> > in V4L2 is required for an efficient implementation of asnapshot mode.
> > This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
> > VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
> > structures.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
> >  drivers/media/video/v4l2-ioctl.c          |   43 +++++++++++++++++++++++++++++
> >  include/linux/videodev2.h                 |   24 ++++++++++++++++
> >  include/media/v4l2-ioctl.h                |    3 ++
> >  4 files changed, 73 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
> > index 7c26947..d71b289 100644
> > --- a/drivers/media/video/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> > @@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
> >  	case VIDIOC_DQEVENT:
> >  	case VIDIOC_SUBSCRIBE_EVENT:
> >  	case VIDIOC_UNSUBSCRIBE_EVENT:
> > +	case VIDIOC_CREATE_BUFS:
> > +	case VIDIOC_DESTROY_BUFS:
> > +	case VIDIOC_SUBMIT_BUF:
> >  		ret = do_video_ioctl(file, cmd, arg);
> >  		break;
> >  
> > diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> > index a01ed39..b80a211 100644
> > --- a/drivers/media/video/v4l2-ioctl.c
> > +++ b/drivers/media/video/v4l2-ioctl.c
> > @@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
> >  	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
> >  	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
> >  	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> > +	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
> > +	[_IOC_NR(VIDIOC_DESTROY_BUFS)]     = "VIDIOC_DESTROY_BUFS",
> > +	[_IOC_NR(VIDIOC_SUBMIT_BUF)]       = "VIDIOC_SUBMIT_BUF",
> >  };
> >  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> >  
> > @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
> >  		dbgarg(cmd, "type=0x%8.8x", sub->type);
> >  		break;
> >  	}
> > +	case VIDIOC_CREATE_BUFS:
> > +	{
> > +		struct v4l2_create_buffers *create = arg;
> > +
> > +		if (!ops->vidioc_create_bufs)
> > +			break;
> > +		ret = check_fmt(ops, create->format.type);
> > +		if (ret)
> > +			break;
> > +
> > +		if (create->size)
> > +			CLEAR_AFTER_FIELD(create, count);
> > +
> > +		ret = ops->vidioc_create_bufs(file, fh, create);
> > +
> > +		dbgarg(cmd, "count=%d\n", create->count);
> > +		break;
> > +	}
> > +	case VIDIOC_DESTROY_BUFS:
> > +	{
> > +		struct v4l2_buffer_span *span = arg;
> > +
> > +		if (!ops->vidioc_destroy_bufs)
> > +			break;
> > +
> > +		ret = ops->vidioc_destroy_bufs(file, fh, span);
> > +
> > +		dbgarg(cmd, "count=%d", span->count);
> > +		break;
> > +	}
> > +	case VIDIOC_SUBMIT_BUF:
> > +	{
> > +		unsigned int *i = arg;
> > +
> > +		if (!ops->vidioc_submit_buf)
> > +			break;
> > +		ret = ops->vidioc_submit_buf(file, fh, *i);
> > +		dbgarg(cmd, "index=%d", *i);
> > +		break;
> > +	}
> >  	default:
> >  	{
> >  		bool valid_prio = true;
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index aa6c393..b6ef46e 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
> >  	__u32 revision;    /* chip revision, chip specific */
> >  } __attribute__ ((packed));
> >  
> > +/* VIDIOC_DESTROY_BUFS */
> > +struct v4l2_buffer_span {
> > +	__u32			index;	/* output: buffers index...index + count - 1 have been created */
> > +	__u32			count;
> > +	__u32			reserved[2];
> > +};
> > +
> > +/* struct v4l2_createbuffers::flags */
> > +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
> 
> 1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.
> 
> 2. Both these flags should not be passed with CREATE, but with SUBMIT 
> (which will be renamed to PREPARE or something similar). It should be 
> possible to prepare the same buffer with different cacheing attributes 
> during a running operation. Shall these flags be added to values, taken by 
> struct v4l2_buffer::flags, since that is the struct, that will be used as 
> the argument for the new version of the SUBMIT ioctl()?

Yes. You may want to give these flags to QBUF as well, so they belong in
v4l2_buffer.

> 
> > +
> > +/* VIDIOC_CREATE_BUFS */
> > +struct v4l2_create_buffers {
> > +	__u32			index;		/* output: buffers index...index + count - 1 have been created */
> > +	__u32			count;
> > +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> > +	enum v4l2_memory        memory;
> > +	__u32			size;		/* Explicit size, e.g., for compressed streams */
> > +	struct v4l2_format	format;		/* "type" is used always, the rest if size == 0 */
> > +};
> 
> 1. Care must be taken to keep index <= V4L2_MAX_FRAME
> 
> 2. A reserved field is needed.
> 
> > +
> >  /*
> >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> >   *
> > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct v4l2_event_subscription)
> >  #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
> >  
> > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> 
> This has become the hottest point for discussion.
> 
> 1. VIDIOC_CREATE_BUFS: should the REQBUFS and CREATE/DESTROY APIs be 
> allowed to be mixed? REQBUFS is compulsory, CREATE/DESTROY will be 
> optional. But shall applications be allowed to mix them? No consensus has 
> been riched. This will also depend on whether DESTROY will be implemented 
> at all (see below).

As I mentioned before I see no technical reason whatsoever why REQBUFS and
CREATE/DESTROY_BUFS can't live in perfect harmony. Requiring apps to use one
or the other only will lead to massive confusion (I made that mistake once with
the extended controls API, which I've corrected later).

> 2. VIDIOC_DESTROY_BUFS: has been discussed a lot
> 
> (a) shall it be allowed to create holes in indices? agreement was: not at 
> this stage, but in the future this might be needed.
> 
> (b) ioctl() argument: shall it take a span or an array of indices? I don't 
> think arrays make any sense here: on CREATE you always get contiguous 
> index sequences, and you are only allowed to DESTROY the same index sets. 
> 
> (c) shall it be implemented at all, now that we don't know, how to handle 
> holes, or shall we just continue using REQBUFS(0) or close() to release 
> all buffers at once? Not implementing DESTROY now has the disadvantage, 
> that if you allocate 2 buffer sets of sizes A (small) and B (big), and 
> then don't need B any more, but instead need C != B (big), you cannot do 
> this. But this is just one of hypothetical use-cases. No consensus 
> reached.

I go for (c). Note that one fall-out from the whole GPU/V4L buffer sharing
discussion might be that we need it after all. But at least then we have a
specific use-case.

> 3. VIDIOC_SUBMIT_BUF:
> 
> (a) shall be renamed to something with prepare or pre-queue in the name 
> and call the .buf_prepare() videobuf2 method. This hasn't raised any 
> objections, has been implemented in v2 (has not been posted yet). Name 
> will be changed to VIDIOC_PREPARE_BUF
> 
> (b) Proposed to use struct v4l2_buffer as the argument. Applications 
> anyway need those structs for other ioctl()s and the information is needed 
> for .buf_prepare(). This is done in v2.

Great! Looking forward to v2.
 
> 4. It has been proposed to create wrappers to allow drivers to only 
> implement CREATE/DESTROY and have those wrappers also provide REQBUFS, 
> using them.
> 
> > +
> >  /* Reminder: when adding new ioctls please add support for them to
> >     drivers/media/video/v4l2-compat-ioctl32.c as well! */
> 
> 1. 'enum memory' and struct v4l2_format need special handling. Fixed in 
> v2, untested.
> 
> >  
> > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> > index dd9f1e7..00962c6 100644
> > --- a/include/media/v4l2-ioctl.h
> > +++ b/include/media/v4l2-ioctl.h
> > @@ -122,6 +122,9 @@ struct v4l2_ioctl_ops {
> >  	int (*vidioc_qbuf)    (struct file *file, void *fh, struct v4l2_buffer *b);
> >  	int (*vidioc_dqbuf)   (struct file *file, void *fh, struct v4l2_buffer *b);
> >  
> > +	int (*vidioc_create_bufs) (struct file *file, void *fh, struct v4l2_create_buffers *b);
> > +	int (*vidioc_destroy_bufs)(struct file *file, void *fh, struct v4l2_buffer_span *b);
> > +	int (*vidioc_submit_buf)  (struct file *file, void *fh, unsigned int i);
> >  
> >  	int (*vidioc_overlay) (struct file *file, void *fh, unsigned int i);
> >  	int (*vidioc_g_fbuf)   (struct file *file, void *fh,
> 
> Personally, I think, one of viable solutions for now would be to implement
> 
> > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > +#define VIDIOC_PREPARE_BUF	 _IOW('V', 93, struct v4l2_buffer)

Agreed.

Regards,

	Hans

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Sakari Ailus]

Hi Guennadi,

Thanks for the patch!

Guennadi Liakhovetski wrote:
> I've found some more time to get back to this. Let me try to recap, what 
> has been discussed. I've looked through all replies again (thanks to 
> all!), so, I'll present a summary. Any mistakes and misinterpretations are 
> mine;) If I misunderstand someone or forget anything - please, shout!
> 
> On Fri, 1 Apr 2011, Guennadi Liakhovetski wrote:
> 
>> A possibility to preallocate and initialise buffers of different sizes
>> in V4L2 is required for an efficient implementation of asnapshot mode.
>> This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
>> VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
>> structures.
>>
>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>> ---
>>  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
>>  drivers/media/video/v4l2-ioctl.c          |   43 +++++++++++++++++++++++++++++
>>  include/linux/videodev2.h                 |   24 ++++++++++++++++
>>  include/media/v4l2-ioctl.h                |    3 ++
>>  4 files changed, 73 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
>> index 7c26947..d71b289 100644
>> --- a/drivers/media/video/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
>> @@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
>>  	case VIDIOC_DQEVENT:
>>  	case VIDIOC_SUBSCRIBE_EVENT:
>>  	case VIDIOC_UNSUBSCRIBE_EVENT:
>> +	case VIDIOC_CREATE_BUFS:
>> +	case VIDIOC_DESTROY_BUFS:
>> +	case VIDIOC_SUBMIT_BUF:
>>  		ret = do_video_ioctl(file, cmd, arg);
>>  		break;
>>  
>> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
>> index a01ed39..b80a211 100644
>> --- a/drivers/media/video/v4l2-ioctl.c
>> +++ b/drivers/media/video/v4l2-ioctl.c
>> @@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
>>  	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
>>  	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
>>  	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
>> +	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
>> +	[_IOC_NR(VIDIOC_DESTROY_BUFS)]     = "VIDIOC_DESTROY_BUFS",
>> +	[_IOC_NR(VIDIOC_SUBMIT_BUF)]       = "VIDIOC_SUBMIT_BUF",
>>  };
>>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>>  
>> @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
>>  		dbgarg(cmd, "type=0x%8.8x", sub->type);
>>  		break;
>>  	}
>> +	case VIDIOC_CREATE_BUFS:
>> +	{
>> +		struct v4l2_create_buffers *create = arg;
>> +
>> +		if (!ops->vidioc_create_bufs)
>> +			break;
>> +		ret = check_fmt(ops, create->format.type);
>> +		if (ret)
>> +			break;
>> +
>> +		if (create->size)
>> +			CLEAR_AFTER_FIELD(create, count);
>> +
>> +		ret = ops->vidioc_create_bufs(file, fh, create);
>> +
>> +		dbgarg(cmd, "count=%d\n", create->count);
>> +		break;
>> +	}
>> +	case VIDIOC_DESTROY_BUFS:
>> +	{
>> +		struct v4l2_buffer_span *span = arg;
>> +
>> +		if (!ops->vidioc_destroy_bufs)
>> +			break;
>> +
>> +		ret = ops->vidioc_destroy_bufs(file, fh, span);
>> +
>> +		dbgarg(cmd, "count=%d", span->count);
>> +		break;
>> +	}
>> +	case VIDIOC_SUBMIT_BUF:
>> +	{
>> +		unsigned int *i = arg;
>> +
>> +		if (!ops->vidioc_submit_buf)
>> +			break;
>> +		ret = ops->vidioc_submit_buf(file, fh, *i);
>> +		dbgarg(cmd, "index=%d", *i);
>> +		break;
>> +	}
>>  	default:
>>  	{
>>  		bool valid_prio = true;
>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>> index aa6c393..b6ef46e 100644
>> --- a/include/linux/videodev2.h
>> +++ b/include/linux/videodev2.h
>> @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
>>  	__u32 revision;    /* chip revision, chip specific */
>>  } __attribute__ ((packed));
>>  
>> +/* VIDIOC_DESTROY_BUFS */
>> +struct v4l2_buffer_span {
>> +	__u32			index;	/* output: buffers index...index + count - 1 have been created */
>> +	__u32			count;
>> +	__u32			reserved[2];
>> +};
>> +
>> +/* struct v4l2_createbuffers::flags */
>> +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
> 
> 1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.

Should this be called FLAG_NO_CACHE_CLEAN?

Invalidate == Make contents of the cache invalid

Clean == Make sure no dirty stuff resides in the cache

Flush == invalidate + clean

It occurred to me to wonder if two flags are needed for this, but I
think the answer is yes, since there can be memory-to-memory devices
which are both OUTPUT and CAPTURE.

> 2. Both these flags should not be passed with CREATE, but with SUBMIT 
> (which will be renamed to PREPARE or something similar). It should be 
> possible to prepare the same buffer with different cacheing attributes 
> during a running operation. Shall these flags be added to values, taken by 
> struct v4l2_buffer::flags, since that is the struct, that will be used as 
> the argument for the new version of the SUBMIT ioctl()?
> 
>> +
>> +/* VIDIOC_CREATE_BUFS */
>> +struct v4l2_create_buffers {
>> +	__u32			index;		/* output: buffers index...index + count - 1 have been created */
>> +	__u32			count;
>> +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
>> +	enum v4l2_memory        memory;
>> +	__u32			size;		/* Explicit size, e.g., for compressed streams */
>> +	struct v4l2_format	format;		/* "type" is used always, the rest if size == 0 */
>> +};
> 
> 1. Care must be taken to keep index <= V4L2_MAX_FRAME

This will make allocating new ranges of buffers impossible if the
existing buffer indices are scattered within the range.

What about making it possible to pass an array of buffer indices to the
user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
perfect, but it would avoid the problem of requiring continuous ranges
of buffer ids.

struct v4l2_create_buffers {
	__u32			*index;
	__u32			count;
	__u32			flags;
	enum v4l2_memory        memory;
	__u32			size;
	struct v4l2_format	format;
};

Index would be a pointer to an array of buffer indices and its length
would be count.

[clip]

Regards,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

Hi Sakari

On Mon, 16 May 2011, Sakari Ailus wrote:

> Hi Guennadi,
> 
> Thanks for the patch!
> 
> Guennadi Liakhovetski wrote:
> > I've found some more time to get back to this. Let me try to recap, what 
> > has been discussed. I've looked through all replies again (thanks to 
> > all!), so, I'll present a summary. Any mistakes and misinterpretations are 
> > mine;) If I misunderstand someone or forget anything - please, shout!
> > 
> > On Fri, 1 Apr 2011, Guennadi Liakhovetski wrote:
> > 
> >> A possibility to preallocate and initialise buffers of different sizes
> >> in V4L2 is required for an efficient implementation of asnapshot mode.
> >> This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
> >> VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
> >> structures.
> >>
> >> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >> ---
> >>  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
> >>  drivers/media/video/v4l2-ioctl.c          |   43 +++++++++++++++++++++++++++++
> >>  include/linux/videodev2.h                 |   24 ++++++++++++++++
> >>  include/media/v4l2-ioctl.h                |    3 ++
> >>  4 files changed, 73 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
> >> index 7c26947..d71b289 100644
> >> --- a/drivers/media/video/v4l2-compat-ioctl32.c
> >> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> >> @@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
> >>  	case VIDIOC_DQEVENT:
> >>  	case VIDIOC_SUBSCRIBE_EVENT:
> >>  	case VIDIOC_UNSUBSCRIBE_EVENT:
> >> +	case VIDIOC_CREATE_BUFS:
> >> +	case VIDIOC_DESTROY_BUFS:
> >> +	case VIDIOC_SUBMIT_BUF:
> >>  		ret = do_video_ioctl(file, cmd, arg);
> >>  		break;
> >>  
> >> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> >> index a01ed39..b80a211 100644
> >> --- a/drivers/media/video/v4l2-ioctl.c
> >> +++ b/drivers/media/video/v4l2-ioctl.c
> >> @@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
> >>  	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
> >>  	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
> >>  	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> >> +	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
> >> +	[_IOC_NR(VIDIOC_DESTROY_BUFS)]     = "VIDIOC_DESTROY_BUFS",
> >> +	[_IOC_NR(VIDIOC_SUBMIT_BUF)]       = "VIDIOC_SUBMIT_BUF",
> >>  };
> >>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> >>  
> >> @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
> >>  		dbgarg(cmd, "type=0x%8.8x", sub->type);
> >>  		break;
> >>  	}
> >> +	case VIDIOC_CREATE_BUFS:
> >> +	{
> >> +		struct v4l2_create_buffers *create = arg;
> >> +
> >> +		if (!ops->vidioc_create_bufs)
> >> +			break;
> >> +		ret = check_fmt(ops, create->format.type);
> >> +		if (ret)
> >> +			break;
> >> +
> >> +		if (create->size)
> >> +			CLEAR_AFTER_FIELD(create, count);
> >> +
> >> +		ret = ops->vidioc_create_bufs(file, fh, create);
> >> +
> >> +		dbgarg(cmd, "count=%d\n", create->count);
> >> +		break;
> >> +	}
> >> +	case VIDIOC_DESTROY_BUFS:
> >> +	{
> >> +		struct v4l2_buffer_span *span = arg;
> >> +
> >> +		if (!ops->vidioc_destroy_bufs)
> >> +			break;
> >> +
> >> +		ret = ops->vidioc_destroy_bufs(file, fh, span);
> >> +
> >> +		dbgarg(cmd, "count=%d", span->count);
> >> +		break;
> >> +	}
> >> +	case VIDIOC_SUBMIT_BUF:
> >> +	{
> >> +		unsigned int *i = arg;
> >> +
> >> +		if (!ops->vidioc_submit_buf)
> >> +			break;
> >> +		ret = ops->vidioc_submit_buf(file, fh, *i);
> >> +		dbgarg(cmd, "index=%d", *i);
> >> +		break;
> >> +	}
> >>  	default:
> >>  	{
> >>  		bool valid_prio = true;
> >> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> >> index aa6c393..b6ef46e 100644
> >> --- a/include/linux/videodev2.h
> >> +++ b/include/linux/videodev2.h
> >> @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
> >>  	__u32 revision;    /* chip revision, chip specific */
> >>  } __attribute__ ((packed));
> >>  
> >> +/* VIDIOC_DESTROY_BUFS */
> >> +struct v4l2_buffer_span {
> >> +	__u32			index;	/* output: buffers index...index + count - 1 have been created */
> >> +	__u32			count;
> >> +	__u32			reserved[2];
> >> +};
> >> +
> >> +/* struct v4l2_createbuffers::flags */
> >> +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
> > 
> > 1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.
> 
> Should this be called FLAG_NO_CACHE_CLEAN?
> 
> Invalidate == Make contents of the cache invalid
> 
> Clean == Make sure no dirty stuff resides in the cache

and mark it clean?...

> Flush == invalidate + clean

Maybe you meant "first clean, then invalidate?"

In principle, I think, yes, CLEAN would define it more strictly.

> It occurred to me to wonder if two flags are needed for this, but I
> think the answer is yes, since there can be memory-to-memory devices
> which are both OUTPUT and CAPTURE.
> 
> > 2. Both these flags should not be passed with CREATE, but with SUBMIT 
> > (which will be renamed to PREPARE or something similar). It should be 
> > possible to prepare the same buffer with different cacheing attributes 
> > during a running operation. Shall these flags be added to values, taken by 
> > struct v4l2_buffer::flags, since that is the struct, that will be used as 
> > the argument for the new version of the SUBMIT ioctl()?
> > 
> >> +
> >> +/* VIDIOC_CREATE_BUFS */
> >> +struct v4l2_create_buffers {
> >> +	__u32			index;		/* output: buffers index...index + count - 1 have been created */
> >> +	__u32			count;
> >> +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> >> +	enum v4l2_memory        memory;
> >> +	__u32			size;		/* Explicit size, e.g., for compressed streams */
> >> +	struct v4l2_format	format;		/* "type" is used always, the rest if size == 0 */
> >> +};
> > 
> > 1. Care must be taken to keep index <= V4L2_MAX_FRAME
> 
> This will make allocating new ranges of buffers impossible if the
> existing buffer indices are scattered within the range.
> 
> What about making it possible to pass an array of buffer indices to the
> user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
> perfect, but it would avoid the problem of requiring continuous ranges
> of buffer ids.
> 
> struct v4l2_create_buffers {
> 	__u32			*index;
> 	__u32			count;
> 	__u32			flags;
> 	enum v4l2_memory        memory;
> 	__u32			size;
> 	struct v4l2_format	format;
> };
> 
> Index would be a pointer to an array of buffer indices and its length
> would be count.

I don't understand this. We do _not_ want to allow holes in indices. For 
now we decide to not implement DESTROY at all. In this case indices just 
increment contiguously.

The next stage is to implement DESTROY, but only in strict reverse order - 
without holes and in the same ranges, as buffers have been CREATEd before. 
So, I really don't understand why we need arrays, sorry.

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Sakari Ailus]

Guennadi Liakhovetski wrote:
> Hi Sakari

Hi Guennadi,

[clip]

>>>>  		bool valid_prio = true;
>>>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>>>> index aa6c393..b6ef46e 100644
>>>> --- a/include/linux/videodev2.h
>>>> +++ b/include/linux/videodev2.h
>>>> @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
>>>>  	__u32 revision;    /* chip revision, chip specific */
>>>>  } __attribute__ ((packed));
>>>>  
>>>> +/* VIDIOC_DESTROY_BUFS */
>>>> +struct v4l2_buffer_span {
>>>> +	__u32			index;	/* output: buffers index...index + count - 1 have been created */
>>>> +	__u32			count;
>>>> +	__u32			reserved[2];
>>>> +};
>>>> +
>>>> +/* struct v4l2_createbuffers::flags */
>>>> +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
>>>
>>> 1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.
>>
>> Should this be called FLAG_NO_CACHE_CLEAN?
>>
>> Invalidate == Make contents of the cache invalid
>>
>> Clean == Make sure no dirty stuff resides in the cache
> 
> and mark it clean?...
> 
>> Flush == invalidate + clean
> 
> Maybe you meant "first clean, then invalidate?"
> 
> In principle, I think, yes, CLEAN would define it more strictly.

Yes; I'd prefer that. :-)

>> It occurred to me to wonder if two flags are needed for this, but I
>> think the answer is yes, since there can be memory-to-memory devices
>> which are both OUTPUT and CAPTURE.
>>
>>> 2. Both these flags should not be passed with CREATE, but with SUBMIT 
>>> (which will be renamed to PREPARE or something similar). It should be 
>>> possible to prepare the same buffer with different cacheing attributes 
>>> during a running operation. Shall these flags be added to values, taken by 
>>> struct v4l2_buffer::flags, since that is the struct, that will be used as 
>>> the argument for the new version of the SUBMIT ioctl()?
>>>
>>>> +
>>>> +/* VIDIOC_CREATE_BUFS */
>>>> +struct v4l2_create_buffers {
>>>> +	__u32			index;		/* output: buffers index...index + count - 1 have been created */
>>>> +	__u32			count;
>>>> +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
>>>> +	enum v4l2_memory        memory;
>>>> +	__u32			size;		/* Explicit size, e.g., for compressed streams */
>>>> +	struct v4l2_format	format;		/* "type" is used always, the rest if size == 0 */
>>>> +};
>>>
>>> 1. Care must be taken to keep index <= V4L2_MAX_FRAME
>>
>> This will make allocating new ranges of buffers impossible if the
>> existing buffer indices are scattered within the range.
>>
>> What about making it possible to pass an array of buffer indices to the
>> user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
>> perfect, but it would avoid the problem of requiring continuous ranges
>> of buffer ids.
>>
>> struct v4l2_create_buffers {
>> 	__u32			*index;
>> 	__u32			count;
>> 	__u32			flags;
>> 	enum v4l2_memory        memory;
>> 	__u32			size;
>> 	struct v4l2_format	format;
>> };
>>
>> Index would be a pointer to an array of buffer indices and its length
>> would be count.
> 
> I don't understand this. We do _not_ want to allow holes in indices. For 
> now we decide to not implement DESTROY at all. In this case indices just 
> increment contiguously.
> 
> The next stage is to implement DESTROY, but only in strict reverse order - 
> without holes and in the same ranges, as buffers have been CREATEd before. 
> So, I really don't understand why we need arrays, sorry.

Well, now that we're defining a second interface to make new buffer
objects, I just thought it should be made as future-proof as we can. But
even with single index, it's always possible to issue the ioctl more
than once and achieve the same result as if there was an array of indices.

What would be the reason to disallow creating holes to index range? I
don't see much reason from application or implementation point of view,
as we're even being limited to such low numbers.

Speaking of which; perhaps I'm bringing this up rather late, but should
we define the API to allow larger numbers than VIDEO_MAX_FRAME? 32 isn't
all that much after all --- this might become a limiting factor later on
when there are devices with huge amounts of memory.

Allowing CREATE_BUF to do that right now would be possible since
applications using it are new users and can be expected to be using it
properly. :-)

Regards,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Laurent Pinchart]

On Friday 13 May 2011 09:45:51 Guennadi Liakhovetski wrote:
> I've found some more time to get back to this. Let me try to recap, what
> has been discussed. I've looked through all replies again (thanks to
> all!), so, I'll present a summary. Any mistakes and misinterpretations are
> mine;) If I misunderstand someone or forget anything - please, shout!
> 
> On Fri, 1 Apr 2011, Guennadi Liakhovetski wrote:
> > A possibility to preallocate and initialise buffers of different sizes
> > in V4L2 is required for an efficient implementation of asnapshot mode.
> > This patch adds three new ioctl()s: VIDIOC_CREATE_BUFS,
> > VIDIOC_DESTROY_BUFS, and VIDIOC_SUBMIT_BUF and defines respective data
> > structures.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 ++
> >  drivers/media/video/v4l2-ioctl.c          |   43
> >  +++++++++++++++++++++++++++++ include/linux/videodev2.h                
> >  |   24 ++++++++++++++++ include/media/v4l2-ioctl.h                |   
> >  3 ++
> >  4 files changed, 73 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
> > b/drivers/media/video/v4l2-compat-ioctl32.c index 7c26947..d71b289
> > 100644
> > --- a/drivers/media/video/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> > @@ -922,6 +922,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned
> > int cmd, unsigned long arg)
> > 
> >  	case VIDIOC_DQEVENT:
> >  	case VIDIOC_SUBSCRIBE_EVENT:
> > 
> >  	case VIDIOC_UNSUBSCRIBE_EVENT:
> > +	case VIDIOC_CREATE_BUFS:
> > +	case VIDIOC_DESTROY_BUFS:
> > 
> > +	case VIDIOC_SUBMIT_BUF:
> >  		ret = do_video_ioctl(file, cmd, arg);
> >  		break;
> > 
> > diff --git a/drivers/media/video/v4l2-ioctl.c
> > b/drivers/media/video/v4l2-ioctl.c index a01ed39..b80a211 100644
> > --- a/drivers/media/video/v4l2-ioctl.c
> > +++ b/drivers/media/video/v4l2-ioctl.c
> > @@ -259,6 +259,9 @@ static const char *v4l2_ioctls[] = {
> > 
> >  	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
> >  	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
> >  	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> > 
> > +	[_IOC_NR(VIDIOC_CREATE_BUFS)]      = "VIDIOC_CREATE_BUFS",
> > +	[_IOC_NR(VIDIOC_DESTROY_BUFS)]     = "VIDIOC_DESTROY_BUFS",
> > +	[_IOC_NR(VIDIOC_SUBMIT_BUF)]       = "VIDIOC_SUBMIT_BUF",
> > 
> >  };
> >  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> > 
> > @@ -2184,6 +2187,46 @@ static long __video_do_ioctl(struct file *file,
> > 
> >  		dbgarg(cmd, "type=0x%8.8x", sub->type);
> >  		break;
> >  	
> >  	}
> > 
> > +	case VIDIOC_CREATE_BUFS:
> > +	{
> > +		struct v4l2_create_buffers *create = arg;
> > +
> > +		if (!ops->vidioc_create_bufs)
> > +			break;
> > +		ret = check_fmt(ops, create->format.type);
> > +		if (ret)
> > +			break;
> > +
> > +		if (create->size)
> > +			CLEAR_AFTER_FIELD(create, count);
> > +
> > +		ret = ops->vidioc_create_bufs(file, fh, create);
> > +
> > +		dbgarg(cmd, "count=%d\n", create->count);
> > +		break;
> > +	}
> > +	case VIDIOC_DESTROY_BUFS:
> > +	{
> > +		struct v4l2_buffer_span *span = arg;
> > +
> > +		if (!ops->vidioc_destroy_bufs)
> > +			break;
> > +
> > +		ret = ops->vidioc_destroy_bufs(file, fh, span);
> > +
> > +		dbgarg(cmd, "count=%d", span->count);
> > +		break;
> > +	}
> > +	case VIDIOC_SUBMIT_BUF:
> > +	{
> > +		unsigned int *i = arg;
> > +
> > +		if (!ops->vidioc_submit_buf)
> > +			break;
> > +		ret = ops->vidioc_submit_buf(file, fh, *i);
> > +		dbgarg(cmd, "index=%d", *i);
> > +		break;
> > +	}
> > 
> >  	default:
> >  	{
> >  	
> >  		bool valid_prio = true;
> > 
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index aa6c393..b6ef46e 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
> > 
> >  	__u32 revision;    /* chip revision, chip specific */
> >  
> >  } __attribute__ ((packed));
> > 
> > +/* VIDIOC_DESTROY_BUFS */
> > +struct v4l2_buffer_span {
> > +	__u32			index;	/* output: buffers index...index + count - 1 have been
> > created */ +	__u32			count;
> > +	__u32			reserved[2];
> > +};
> > +
> > +/* struct v4l2_createbuffers::flags */
> > +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
> 
> 1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.

Shouldn't it be NO_CACHE_CLEAN ?

> 2. Both these flags should not be passed with CREATE, but with SUBMIT
> (which will be renamed to PREPARE or something similar). It should be
> possible to prepare the same buffer with different cacheing attributes
> during a running operation. Shall these flags be added to values, taken by
> struct v4l2_buffer::flags, since that is the struct, that will be used as
> the argument for the new version of the SUBMIT ioctl()?

Do you have a use case for per-submit cache handling ?

> > +
> > +/* VIDIOC_CREATE_BUFS */
> > +struct v4l2_create_buffers {
> > +	__u32			index;		/* output: buffers index...index + count - 1 have 
been
> > created */ +	__u32			count;
> > +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> > +	enum v4l2_memory        memory;
> > +	__u32			size;		/* Explicit size, e.g., for compressed streams */
> > +	struct v4l2_format	format;		/* "type" is used always, the rest if 
size
> > == 0 */ +};
> 
> 1. Care must be taken to keep index <= V4L2_MAX_FRAME

Does that requirement still make sense ?

> 2. A reserved field is needed.
> 
> > +
> > 
> >  /*
> >  
> >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> >   *
> > 
> > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> > 
> >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> >  v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
> >  struct v4l2_event_subscription)
> > 
> > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> 
> This has become the hottest point for discussion.
> 
> 1. VIDIOC_CREATE_BUFS: should the REQBUFS and CREATE/DESTROY APIs be
> allowed to be mixed? REQBUFS is compulsory, CREATE/DESTROY will be
> optional. But shall applications be allowed to mix them? No consensus has
> been riched. This will also depend on whether DESTROY will be implemented
> at all (see below).

Would mixing them help in any known use case ? The API and implementation 
would be cleaner if we didn't allow mixing them.

> 2. VIDIOC_DESTROY_BUFS: has been discussed a lot
> 
> (a) shall it be allowed to create holes in indices? agreement was: not at
> this stage, but in the future this might be needed.
> 
> (b) ioctl() argument: shall it take a span or an array of indices? I don't
> think arrays make any sense here: on CREATE you always get contiguous
> index sequences, and you are only allowed to DESTROY the same index sets.
> 
> (c) shall it be implemented at all, now that we don't know, how to handle
> holes, or shall we just continue using REQBUFS(0) or close() to release
> all buffers at once? Not implementing DESTROY now has the disadvantage,
> that if you allocate 2 buffer sets of sizes A (small) and B (big), and
> then don't need B any more, but instead need C != B (big), you cannot do
> this. But this is just one of hypothetical use-cases. No consensus
> reached.

We could go with (c) as a first step, but it might be temporary only. I feel a 
bit uneasy implementing an API that we know will be temporary.

> 3. VIDIOC_SUBMIT_BUF:
> 
> (a) shall be renamed to something with prepare or pre-queue in the name
> and call the .buf_prepare() videobuf2 method. This hasn't raised any
> objections, has been implemented in v2 (has not been posted yet). Name
> will be changed to VIDIOC_PREPARE_BUF
> 
> (b) Proposed to use struct v4l2_buffer as the argument. Applications
> anyway need those structs for other ioctl()s and the information is needed
> for .buf_prepare(). This is done in v2.
> 
> 4. It has been proposed to create wrappers to allow drivers to only
> implement CREATE/DESTROY and have those wrappers also provide REQBUFS,
> using them.
> 
> > +
> > 
> >  /* Reminder: when adding new ioctls please add support for them to
> >  
> >     drivers/media/video/v4l2-compat-ioctl32.c as well! */
> 
> 1. 'enum memory' and struct v4l2_format need special handling. Fixed in
> v2, untested.
> 
> > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> > index dd9f1e7..00962c6 100644
> > --- a/include/media/v4l2-ioctl.h
> > +++ b/include/media/v4l2-ioctl.h
> > @@ -122,6 +122,9 @@ struct v4l2_ioctl_ops {
> > 
> >  	int (*vidioc_qbuf)    (struct file *file, void *fh, struct v4l2_buffer
> >  	*b); int (*vidioc_dqbuf)   (struct file *file, void *fh, struct
> >  	v4l2_buffer *b);
> > 
> > +	int (*vidioc_create_bufs) (struct file *file, void *fh, struct
> > v4l2_create_buffers *b); +	int (*vidioc_destroy_bufs)(struct file *file,
> > void *fh, struct v4l2_buffer_span *b); +	int (*vidioc_submit_buf) 
> > (struct file *file, void *fh, unsigned int i);
> > 
> >  	int (*vidioc_overlay) (struct file *file, void *fh, unsigned int i);
> >  	int (*vidioc_g_fbuf)   (struct file *file, void *fh,
> 
> Personally, I think, one of viable solutions for now would be to implement
> 
> > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > +#define VIDIOC_PREPARE_BUF	 _IOW('V', 93, struct v4l2_buffer)

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Laurent Pinchart]

On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
> Guennadi Liakhovetski wrote:
> > Hi Sakari
> 
> Hi Guennadi,
> 
> [clip]
> 
> >>>>  		bool valid_prio = true;
> >>>> 
> >>>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> >>>> index aa6c393..b6ef46e 100644
> >>>> --- a/include/linux/videodev2.h
> >>>> +++ b/include/linux/videodev2.h
> >>>> @@ -1847,6 +1847,26 @@ struct v4l2_dbg_chip_ident {
> >>>> 
> >>>>  	__u32 revision;    /* chip revision, chip specific */
> >>>>  
> >>>>  } __attribute__ ((packed));
> >>>> 
> >>>> +/* VIDIOC_DESTROY_BUFS */
> >>>> +struct v4l2_buffer_span {
> >>>> +	__u32			index;	/* output: buffers index...index + count - 1 have
> >>>> been created */ +	__u32			count;
> >>>> +	__u32			reserved[2];
> >>>> +};
> >>>> +
> >>>> +/* struct v4l2_createbuffers::flags */
> >>>> +#define V4L2_BUFFER_FLAG_NO_CACHE_INVALIDATE	(1 << 0)
> >>> 
> >>> 1. An additional flag FLAG_NO_CACHE_FLUSH is needed for output devices.
> >> 
> >> Should this be called FLAG_NO_CACHE_CLEAN?
> >> 
> >> Invalidate == Make contents of the cache invalid
> >> 
> >> Clean == Make sure no dirty stuff resides in the cache
> > 
> > and mark it clean?...
> > 
> >> Flush == invalidate + clean
> > 
> > Maybe you meant "first clean, then invalidate?"
> > 
> > In principle, I think, yes, CLEAN would define it more strictly.
> 
> Yes; I'd prefer that. :-)
> 
> >> It occurred to me to wonder if two flags are needed for this, but I
> >> think the answer is yes, since there can be memory-to-memory devices
> >> which are both OUTPUT and CAPTURE.
> >> 
> >>> 2. Both these flags should not be passed with CREATE, but with SUBMIT
> >>> (which will be renamed to PREPARE or something similar). It should be
> >>> possible to prepare the same buffer with different cacheing attributes
> >>> during a running operation. Shall these flags be added to values, taken
> >>> by struct v4l2_buffer::flags, since that is the struct, that will be
> >>> used as the argument for the new version of the SUBMIT ioctl()?
> >>> 
> >>>> +
> >>>> +/* VIDIOC_CREATE_BUFS */
> >>>> +struct v4l2_create_buffers {
> >>>> +	__u32			index;		/* output: buffers index...index + count - 1 
have
> >>>> been created */ +	__u32			count;
> >>>> +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> >>>> +	enum v4l2_memory        memory;
> >>>> +	__u32			size;		/* Explicit size, e.g., for compressed streams 
*/
> >>>> +	struct v4l2_format	format;		/* "type" is used always, the rest 
if
> >>>> size == 0 */ +};
> >>> 
> >>> 1. Care must be taken to keep index <= V4L2_MAX_FRAME
> >> 
> >> This will make allocating new ranges of buffers impossible if the
> >> existing buffer indices are scattered within the range.
> >> 
> >> What about making it possible to pass an array of buffer indices to the
> >> user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
> >> perfect, but it would avoid the problem of requiring continuous ranges
> >> of buffer ids.
> >> 
> >> struct v4l2_create_buffers {
> >> 
> >> 	__u32			*index;
> >> 	__u32			count;
> >> 	__u32			flags;
> >> 	enum v4l2_memory        memory;
> >> 	__u32			size;
> >> 	struct v4l2_format	format;
> >> 
> >> };
> >> 
> >> Index would be a pointer to an array of buffer indices and its length
> >> would be count.
> > 
> > I don't understand this. We do _not_ want to allow holes in indices. For
> > now we decide to not implement DESTROY at all. In this case indices just
> > increment contiguously.
> > 
> > The next stage is to implement DESTROY, but only in strict reverse order
> > - without holes and in the same ranges, as buffers have been CREATEd
> > before. So, I really don't understand why we need arrays, sorry.
> 
> Well, now that we're defining a second interface to make new buffer
> objects, I just thought it should be made as future-proof as we can.

I second that. I don't like rushing new APIs to find out we need something 
else after 6 months.

> But even with single index, it's always possible to issue the ioctl more
> than once and achieve the same result as if there was an array of indices.
> 
> What would be the reason to disallow creating holes to index range? I
> don't see much reason from application or implementation point of view,
> as we're even being limited to such low numbers.
> 
> Speaking of which; perhaps I'm bringing this up rather late, but should
> we define the API to allow larger numbers than VIDEO_MAX_FRAME? 32 isn't
> all that much after all --- this might become a limiting factor later on
> when there are devices with huge amounts of memory.
> 
> Allowing CREATE_BUF to do that right now would be possible since
> applications using it are new users and can be expected to be using it
> properly. :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

On Wed, 18 May 2011, Laurent Pinchart wrote:

> On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
> > Guennadi Liakhovetski wrote:

[snip]

> > >> What about making it possible to pass an array of buffer indices to the
> > >> user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
> > >> perfect, but it would avoid the problem of requiring continuous ranges
> > >> of buffer ids.
> > >> 
> > >> struct v4l2_create_buffers {
> > >> 
> > >> 	__u32			*index;
> > >> 	__u32			count;
> > >> 	__u32			flags;
> > >> 	enum v4l2_memory        memory;
> > >> 	__u32			size;
> > >> 	struct v4l2_format	format;
> > >> 
> > >> };
> > >> 
> > >> Index would be a pointer to an array of buffer indices and its length
> > >> would be count.
> > > 
> > > I don't understand this. We do _not_ want to allow holes in indices. For
> > > now we decide to not implement DESTROY at all. In this case indices just
> > > increment contiguously.
> > > 
> > > The next stage is to implement DESTROY, but only in strict reverse order
> > > - without holes and in the same ranges, as buffers have been CREATEd
> > > before. So, I really don't understand why we need arrays, sorry.
> > 
> > Well, now that we're defining a second interface to make new buffer
> > objects, I just thought it should be made as future-proof as we can.
> 
> I second that. I don't like rushing new APIs to find out we need something 
> else after 6 months.

Ok, so, we pass an array from user-space with CREATE of size count. The 
kernel fills it with as many buffers entries as it has allocated. But 
currently drivers are also allowed to allocate more buffers, than the 
user-space has requested. What do we do in such a case?

Thanks
Guennadi

> > But even with single index, it's always possible to issue the ioctl more
> > than once and achieve the same result as if there was an array of indices.
> > 
> > What would be the reason to disallow creating holes to index range? I
> > don't see much reason from application or implementation point of view,
> > as we're even being limited to such low numbers.
> > 
> > Speaking of which; perhaps I'm bringing this up rather late, but should
> > we define the API to allow larger numbers than VIDEO_MAX_FRAME? 32 isn't
> > all that much after all --- this might become a limiting factor later on
> > when there are devices with huge amounts of memory.
> > 
> > Allowing CREATE_BUF to do that right now would be possible since
> > applications using it are new users and can be expected to be using it
> > properly. :-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

On Wed, 18 May 2011, Laurent Pinchart wrote:

> On Friday 13 May 2011 09:45:51 Guennadi Liakhovetski wrote:

[snip]

> > 2. Both these flags should not be passed with CREATE, but with SUBMIT
> > (which will be renamed to PREPARE or something similar). It should be
> > possible to prepare the same buffer with different cacheing attributes
> > during a running operation. Shall these flags be added to values, taken by
> > struct v4l2_buffer::flags, since that is the struct, that will be used as
> > the argument for the new version of the SUBMIT ioctl()?
> 
> Do you have a use case for per-submit cache handling ?

This was Sakari's idea, I think, ask him;) But I think, he suggested a 
case, when not all buffers have to be processed in the user-space. In 
principle, I can imagine such a case too. E.g., if most of the buffers you 
pass directly to output / further processing, and only some of them you 
analyse in software, perhaps, for some WB, exposure, etc.

> > > +
> > > +/* VIDIOC_CREATE_BUFS */
> > > +struct v4l2_create_buffers {
> > > +	__u32			index;		/* output: buffers index...index + count - 1 have 
> been
> > > created */ +	__u32			count;
> > > +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
> > > +	enum v4l2_memory        memory;
> > > +	__u32			size;		/* Explicit size, e.g., for compressed streams */
> > > +	struct v4l2_format	format;		/* "type" is used always, the rest if 
> size
> > > == 0 */ +};
> > 
> > 1. Care must be taken to keep index <= V4L2_MAX_FRAME
> 
> Does that requirement still make sense ?

Don't know, again, not my idea. videobuf2-core still uses it. At one 
location it seems to be pretty arbitrary, at another it is the size of an 
array in struct vb2_fileio_data, but maybe we could allocate that one 
dynamically too. So, do I understand it right, that this would affect our 
case, if we would CREATE our buffers and then the user would decide to do 
read() / write() on them?

> > 2. A reserved field is needed.
> > 
> > > +
> > > 
> > >  /*
> > >  
> > >   *	I O C T L   C O D E S   F O R   V I D E O   D E V I C E S
> > >   *
> > > 
> > > @@ -1937,6 +1957,10 @@ struct v4l2_dbg_chip_ident {
> > > 
> > >  #define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 90, struct
> > >  v4l2_event_subscription) #define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91,
> > >  struct v4l2_event_subscription)
> > > 
> > > +#define VIDIOC_CREATE_BUFS	_IOWR('V', 92, struct v4l2_create_buffers)
> > > +#define VIDIOC_DESTROY_BUFS	_IOWR('V', 93, struct v4l2_buffer_span)
> > > +#define VIDIOC_SUBMIT_BUF	 _IOW('V', 94, int)
> > 
> > This has become the hottest point for discussion.
> > 
> > 1. VIDIOC_CREATE_BUFS: should the REQBUFS and CREATE/DESTROY APIs be
> > allowed to be mixed? REQBUFS is compulsory, CREATE/DESTROY will be
> > optional. But shall applications be allowed to mix them? No consensus has
> > been riched. This will also depend on whether DESTROY will be implemented
> > at all (see below).
> 
> Would mixing them help in any known use case ? The API and implementation 
> would be cleaner if we didn't allow mixing them.

It would help us avoid designing non-mature APIs and still have the 
functionality, we need;)

> > 2. VIDIOC_DESTROY_BUFS: has been discussed a lot
> > 
> > (a) shall it be allowed to create holes in indices? agreement was: not at
> > this stage, but in the future this might be needed.
> > 
> > (b) ioctl() argument: shall it take a span or an array of indices? I don't
> > think arrays make any sense here: on CREATE you always get contiguous
> > index sequences, and you are only allowed to DESTROY the same index sets.
> > 
> > (c) shall it be implemented at all, now that we don't know, how to handle
> > holes, or shall we just continue using REQBUFS(0) or close() to release
> > all buffers at once? Not implementing DESTROY now has the disadvantage,
> > that if you allocate 2 buffer sets of sizes A (small) and B (big), and
> > then don't need B any more, but instead need C != B (big), you cannot do
> > this. But this is just one of hypothetical use-cases. No consensus
> > reached.
> 
> We could go with (c) as a first step, but it might be temporary only. I feel a 
> bit uneasy implementing an API that we know will be temporary.

It shouldn't be temporary. CREATE and PREPARE should stay. It's just 
DESTROY that we're not certain about yet.

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Sakari Ailus]

Hi,

Guennadi Liakhovetski wrote:
> On Wed, 18 May 2011, Laurent Pinchart wrote:
> 
>> On Friday 13 May 2011 09:45:51 Guennadi Liakhovetski wrote:
> 
> [snip]
> 
>>> 2. Both these flags should not be passed with CREATE, but with SUBMIT
>>> (which will be renamed to PREPARE or something similar). It should be
>>> possible to prepare the same buffer with different cacheing attributes
>>> during a running operation. Shall these flags be added to values, taken by
>>> struct v4l2_buffer::flags, since that is the struct, that will be used as
>>> the argument for the new version of the SUBMIT ioctl()?
>>
>> Do you have a use case for per-submit cache handling ?
> 
> This was Sakari's idea, I think, ask him;) But I think, he suggested a 
> case, when not all buffers have to be processed in the user-space. In 
> principle, I can imagine such a case too. E.g., if most of the buffers you 
> pass directly to output / further processing, and only some of them you 
> analyse in software, perhaps, for some WB, exposure, etc.

Yes; I think I mentioned this. It's possible that some rather
CPU-intensive processing is only done on the CPU on every n:th image,
for example. In this case flushing the cache on images that won't be
touched by the CPU is not necessary.

>>>> +
>>>> +/* VIDIOC_CREATE_BUFS */
>>>> +struct v4l2_create_buffers {
>>>> +	__u32			index;		/* output: buffers index...index + count - 1 have 
>> been
>>>> created */ +	__u32			count;
>>>> +	__u32			flags;		/* V4L2_BUFFER_FLAG_* */
>>>> +	enum v4l2_memory        memory;
>>>> +	__u32			size;		/* Explicit size, e.g., for compressed streams */
>>>> +	struct v4l2_format	format;		/* "type" is used always, the rest if 
>> size
>>>> == 0 */ +};
>>>
>>> 1. Care must be taken to keep index <= V4L2_MAX_FRAME
>>
>> Does that requirement still make sense ?
> 
> Don't know, again, not my idea. videobuf2-core still uses it. At one 
> location it seems to be pretty arbitrary, at another it is the size of an 
> array in struct vb2_fileio_data, but maybe we could allocate that one 
> dynamically too. So, do I understand it right, that this would affect our 
> case, if we would CREATE our buffers and then the user would decide to do 
> read() / write() on them?

My issue with this number, as I gave it a little more thought, is that
it is actually quite low. The devices will have more memory in the
future and 32 might become a real limitation. I think it would be wise
to define the API so that the number of simultaneous buffers allocated
on a device is not limited by this number.

Kind regards,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Sakari Ailus]

Hi Guennadi and Laurent,

Guennadi Liakhovetski wrote:
> On Wed, 18 May 2011, Laurent Pinchart wrote:
> 
>> On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
>>> Guennadi Liakhovetski wrote:
> 
> [snip]
> 
>>>>> What about making it possible to pass an array of buffer indices to the
>>>>> user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
>>>>> perfect, but it would avoid the problem of requiring continuous ranges
>>>>> of buffer ids.
>>>>>
>>>>> struct v4l2_create_buffers {
>>>>>
>>>>> 	__u32			*index;
>>>>> 	__u32			count;
>>>>> 	__u32			flags;
>>>>> 	enum v4l2_memory        memory;
>>>>> 	__u32			size;
>>>>> 	struct v4l2_format	format;
>>>>>
>>>>> };
>>>>>
>>>>> Index would be a pointer to an array of buffer indices and its length
>>>>> would be count.
>>>>
>>>> I don't understand this. We do _not_ want to allow holes in indices. For
>>>> now we decide to not implement DESTROY at all. In this case indices just
>>>> increment contiguously.
>>>>
>>>> The next stage is to implement DESTROY, but only in strict reverse order
>>>> - without holes and in the same ranges, as buffers have been CREATEd
>>>> before. So, I really don't understand why we need arrays, sorry.
>>>
>>> Well, now that we're defining a second interface to make new buffer
>>> objects, I just thought it should be made as future-proof as we can.
>>
>> I second that. I don't like rushing new APIs to find out we need something 
>> else after 6 months.
> 
> Ok, so, we pass an array from user-space with CREATE of size count. The 
> kernel fills it with as many buffers entries as it has allocated. But 
> currently drivers are also allowed to allocate more buffers, than the 
> user-space has requested. What do we do in such a case?

That's a good point.

But even if there was no array, shouldn't the user be allowed to create
the buffers using a number of separate CREATE_BUF calls? The result
would be still the same n buffers as with a single call allocating the n
buffers at once.

Also, consider the (hopefully!) forthcoming DMA buffer management API
patches. It looks like that those buffers will be referred to by file
handles. To associate several DMA buffer objects to V4L2 buffers at
once, there would have to be an array of those objects.

<URL:http://www.spinics.net/lists/linux-media/msg32448.html>

(See the links, too!)

Thus, I would think that CREATE_BUF can be used to create buffers but
not to enforce how many of them are required by a device on a single
CREATE_BUF call.

I don't have a good answer for the stated problem, but these ones
crossed my mind:

- Have a new ioctl to tell the minimum number of buffers to make
streaming possible.

- Add a field for the minimum number of buffers to CREATE_BUF.

- Use the old REQBUFS to tell the number. It didn't do much other work
in the past either, right?

Regards,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

On Tue, 17 May 2011, Sakari Ailus wrote:

> Guennadi Liakhovetski wrote:

[snip]

> > I don't understand this. We do _not_ want to allow holes in indices. For 
> > now we decide to not implement DESTROY at all. In this case indices just 
> > increment contiguously.
> > 
> > The next stage is to implement DESTROY, but only in strict reverse order - 
> > without holes and in the same ranges, as buffers have been CREATEd before. 
> > So, I really don't understand why we need arrays, sorry.
> 
> Well, now that we're defining a second interface to make new buffer
> objects, I just thought it should be made as future-proof as we can. But
> even with single index, it's always possible to issue the ioctl more
> than once and achieve the same result as if there was an array of indices.
> 
> What would be the reason to disallow creating holes to index range? I
> don't see much reason from application or implementation point of view,
> as we're even being limited to such low numbers.

I think, there are a few locations in V4L2, that assume, that for N number 
of buffers currently allocated, their indices are 0...N-1. Just look for 
loops like

	for (buffer = 0; buffer < q->num_buffers; ++buffer) {

in videobuf2-core.c.

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Sakari Ailus]

Guennadi Liakhovetski wrote:
> On Tue, 17 May 2011, Sakari Ailus wrote:
> 
>> Guennadi Liakhovetski wrote:
> 
> [snip]
> 
>>> I don't understand this. We do _not_ want to allow holes in indices. For 
>>> now we decide to not implement DESTROY at all. In this case indices just 
>>> increment contiguously.
>>>
>>> The next stage is to implement DESTROY, but only in strict reverse order - 
>>> without holes and in the same ranges, as buffers have been CREATEd before. 
>>> So, I really don't understand why we need arrays, sorry.
>>
>> Well, now that we're defining a second interface to make new buffer
>> objects, I just thought it should be made as future-proof as we can. But
>> even with single index, it's always possible to issue the ioctl more
>> than once and achieve the same result as if there was an array of indices.
>>
>> What would be the reason to disallow creating holes to index range? I
>> don't see much reason from application or implementation point of view,
>> as we're even being limited to such low numbers.
> 
> I think, there are a few locations in V4L2, that assume, that for N number 
> of buffers currently allocated, their indices are 0...N-1. Just look for 
> loops like
> 
> 	for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> 
> in videobuf2-core.c.

This code is in implementation of videobuf2, it's not the spec. We're
designing a new interface here and its behaviour musn't be restrained by
the current codebase. The videobuf2 must be changed to support the new
ioctls in any case; those functions must be fixed as the support for
CREATE_BUF and other new IOCTLs is added to videobuf2.

The above loop also likely assumes that the index of the first video
buffer to be allocated is zero; this would mean that no more than one
allocation of n buffers could be made, defeating the purpose of the new
interface.

Regards,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

On Wed, 18 May 2011, Sakari Ailus wrote:

> Hi Guennadi and Laurent,
> 
> Guennadi Liakhovetski wrote:
> > On Wed, 18 May 2011, Laurent Pinchart wrote:
> > 
> >> On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
> >>> Guennadi Liakhovetski wrote:
> > 
> > [snip]
> > 
> >>>>> What about making it possible to pass an array of buffer indices to the
> >>>>> user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
> >>>>> perfect, but it would avoid the problem of requiring continuous ranges
> >>>>> of buffer ids.
> >>>>>
> >>>>> struct v4l2_create_buffers {
> >>>>>
> >>>>> 	__u32			*index;
> >>>>> 	__u32			count;
> >>>>> 	__u32			flags;
> >>>>> 	enum v4l2_memory        memory;
> >>>>> 	__u32			size;
> >>>>> 	struct v4l2_format	format;
> >>>>>
> >>>>> };
> >>>>>
> >>>>> Index would be a pointer to an array of buffer indices and its length
> >>>>> would be count.
> >>>>
> >>>> I don't understand this. We do _not_ want to allow holes in indices. For
> >>>> now we decide to not implement DESTROY at all. In this case indices just
> >>>> increment contiguously.
> >>>>
> >>>> The next stage is to implement DESTROY, but only in strict reverse order
> >>>> - without holes and in the same ranges, as buffers have been CREATEd
> >>>> before. So, I really don't understand why we need arrays, sorry.
> >>>
> >>> Well, now that we're defining a second interface to make new buffer
> >>> objects, I just thought it should be made as future-proof as we can.
> >>
> >> I second that. I don't like rushing new APIs to find out we need something 
> >> else after 6 months.
> > 
> > Ok, so, we pass an array from user-space with CREATE of size count. The 
> > kernel fills it with as many buffers entries as it has allocated. But 
> > currently drivers are also allowed to allocate more buffers, than the 
> > user-space has requested. What do we do in such a case?
> 
> That's a good point.
> 
> But even if there was no array, shouldn't the user be allowed to create
> the buffers using a number of separate CREATE_BUF calls? The result
> would be still the same n buffers as with a single call allocating the n
> buffers at once.
> 
> Also, consider the (hopefully!) forthcoming DMA buffer management API
> patches. It looks like that those buffers will be referred to by file
> handles. To associate several DMA buffer objects to V4L2 buffers at
> once, there would have to be an array of those objects.
> 
> <URL:http://www.spinics.net/lists/linux-media/msg32448.html>

So, does this mean now, that we have to wait for those APIs to become 
solid before or even implemented we proceed with this one?

Thanks
Guennadi

> 
> (See the links, too!)
> 
> Thus, I would think that CREATE_BUF can be used to create buffers but
> not to enforce how many of them are required by a device on a single
> CREATE_BUF call.
> 
> I don't have a good answer for the stated problem, but these ones
> crossed my mind:
> 
> - Have a new ioctl to tell the minimum number of buffers to make
> streaming possible.
> 
> - Add a field for the minimum number of buffers to CREATE_BUF.
> 
> - Use the old REQBUFS to tell the number. It didn't do much other work
> in the past either, right?
> 
> Regards,
> 
> -- 
> Sakari Ailus
> sakari.ailus@maxwell.research.nokia.com
> 

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Sakari Ailus]

Hi Guennadi,

On Mon, Jun 06, 2011 at 03:10:54PM +0200, Guennadi Liakhovetski wrote:
> On Wed, 18 May 2011, Sakari Ailus wrote:
> 
> > Hi Guennadi and Laurent,
> > 
> > Guennadi Liakhovetski wrote:
> > > On Wed, 18 May 2011, Laurent Pinchart wrote:
> > > 
> > >> On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
> > >>> Guennadi Liakhovetski wrote:
> > > 
> > > [snip]
> > > 
> > >>>>> What about making it possible to pass an array of buffer indices to the
> > >>>>> user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
> > >>>>> perfect, but it would avoid the problem of requiring continuous ranges
> > >>>>> of buffer ids.
> > >>>>>
> > >>>>> struct v4l2_create_buffers {
> > >>>>>
> > >>>>> 	__u32			*index;
> > >>>>> 	__u32			count;
> > >>>>> 	__u32			flags;
> > >>>>> 	enum v4l2_memory        memory;
> > >>>>> 	__u32			size;
> > >>>>> 	struct v4l2_format	format;
> > >>>>>
> > >>>>> };
> > >>>>>
> > >>>>> Index would be a pointer to an array of buffer indices and its length
> > >>>>> would be count.
> > >>>>
> > >>>> I don't understand this. We do _not_ want to allow holes in indices. For
> > >>>> now we decide to not implement DESTROY at all. In this case indices just
> > >>>> increment contiguously.
> > >>>>
> > >>>> The next stage is to implement DESTROY, but only in strict reverse order
> > >>>> - without holes and in the same ranges, as buffers have been CREATEd
> > >>>> before. So, I really don't understand why we need arrays, sorry.
> > >>>
> > >>> Well, now that we're defining a second interface to make new buffer
> > >>> objects, I just thought it should be made as future-proof as we can.
> > >>
> > >> I second that. I don't like rushing new APIs to find out we need something 
> > >> else after 6 months.
> > > 
> > > Ok, so, we pass an array from user-space with CREATE of size count. The 
> > > kernel fills it with as many buffers entries as it has allocated. But 
> > > currently drivers are also allowed to allocate more buffers, than the 
> > > user-space has requested. What do we do in such a case?
> > 
> > That's a good point.
> > 
> > But even if there was no array, shouldn't the user be allowed to create
> > the buffers using a number of separate CREATE_BUF calls? The result
> > would be still the same n buffers as with a single call allocating the n
> > buffers at once.
> > 
> > Also, consider the (hopefully!) forthcoming DMA buffer management API
> > patches. It looks like that those buffers will be referred to by file
> > handles. To associate several DMA buffer objects to V4L2 buffers at
> > once, there would have to be an array of those objects.
> > 
> > <URL:http://www.spinics.net/lists/linux-media/msg32448.html>
> 
> So, does this mean now, that we have to wait for those APIs to become 
> solid before or even implemented we proceed with this one?

No. But I think we should take into account the foreseeable future. Any
which form the buffer id passing mechanism will take, it will very likely
involve referring to individual memory buffers the ids of which are not
contiguous ranges in a general case. In short, my point is that CREATE_BUF
should allow associating generic buffer ids to V4L2 buffers.

If the hardware requires more than one buffer to operate, STREAMON could
return ERANGE in a case there ane not enough queued, for example.

Regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Guennadi Liakhovetski]

On Mon, 6 Jun 2011, Sakari Ailus wrote:

> Hi Guennadi,
> 
> On Mon, Jun 06, 2011 at 03:10:54PM +0200, Guennadi Liakhovetski wrote:
> > On Wed, 18 May 2011, Sakari Ailus wrote:
> > 
> > > Hi Guennadi and Laurent,
> > > 
> > > Guennadi Liakhovetski wrote:
> > > > On Wed, 18 May 2011, Laurent Pinchart wrote:
> > > > 
> > > >> On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
> > > >>> Guennadi Liakhovetski wrote:
> > > > 
> > > > [snip]
> > > > 
> > > >>>>> What about making it possible to pass an array of buffer indices to the
> > > >>>>> user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
> > > >>>>> perfect, but it would avoid the problem of requiring continuous ranges
> > > >>>>> of buffer ids.
> > > >>>>>
> > > >>>>> struct v4l2_create_buffers {
> > > >>>>>
> > > >>>>> 	__u32			*index;
> > > >>>>> 	__u32			count;
> > > >>>>> 	__u32			flags;
> > > >>>>> 	enum v4l2_memory        memory;
> > > >>>>> 	__u32			size;
> > > >>>>> 	struct v4l2_format	format;
> > > >>>>>
> > > >>>>> };
> > > >>>>>
> > > >>>>> Index would be a pointer to an array of buffer indices and its length
> > > >>>>> would be count.
> > > >>>>
> > > >>>> I don't understand this. We do _not_ want to allow holes in indices. For
> > > >>>> now we decide to not implement DESTROY at all. In this case indices just
> > > >>>> increment contiguously.
> > > >>>>
> > > >>>> The next stage is to implement DESTROY, but only in strict reverse order
> > > >>>> - without holes and in the same ranges, as buffers have been CREATEd
> > > >>>> before. So, I really don't understand why we need arrays, sorry.
> > > >>>
> > > >>> Well, now that we're defining a second interface to make new buffer
> > > >>> objects, I just thought it should be made as future-proof as we can.
> > > >>
> > > >> I second that. I don't like rushing new APIs to find out we need something 
> > > >> else after 6 months.
> > > > 
> > > > Ok, so, we pass an array from user-space with CREATE of size count. The 
> > > > kernel fills it with as many buffers entries as it has allocated. But 
> > > > currently drivers are also allowed to allocate more buffers, than the 
> > > > user-space has requested. What do we do in such a case?
> > > 
> > > That's a good point.
> > > 
> > > But even if there was no array, shouldn't the user be allowed to create
> > > the buffers using a number of separate CREATE_BUF calls? The result
> > > would be still the same n buffers as with a single call allocating the n
> > > buffers at once.
> > > 
> > > Also, consider the (hopefully!) forthcoming DMA buffer management API
> > > patches. It looks like that those buffers will be referred to by file
> > > handles. To associate several DMA buffer objects to V4L2 buffers at
> > > once, there would have to be an array of those objects.
> > > 
> > > <URL:http://www.spinics.net/lists/linux-media/msg32448.html>
> > 
> > So, does this mean now, that we have to wait for those APIs to become 
> > solid before or even implemented we proceed with this one?
> 
> No. But I think we should take into account the foreseeable future. Any
> which form the buffer id passing mechanism will take, it will very likely
> involve referring to individual memory buffers the ids of which are not
> contiguous ranges in a general case. In short, my point is that CREATE_BUF
> should allow associating generic buffer ids to V4L2 buffers.

Ok, so, first point is: yes, it can well happen, that video buffers will 
use some further buffer allocator as a back-end, that each videobuf will 
possibly reference more than one of those memory buffers, and that those 
memory buffers will have arbitrary IDs. AFAIC, this so far doesn't affect 
our design of the CREATE ioctl(), right? As you say, both indices are 
unrelated.

I can imagine, that in the future, when we implement DESTROY, videobuf 
indices can become sparse. Say, if then we have indices 3 to 5 allocated, 
and the user requests 4 buffers. We can either use indices 0-2 and 6, or 
6-9. Personally, I would go with the latter option. Maybe we'll have to 
increase or completely eliminate the VIDEO_MAX_FRAME. But otherwise I 
think, this would be the best way to handle this. Which means, our CREATE 
ioctl() with contiguous indics should be fine. As for DESTROY, the idea 
was to only allow destroying same sets of buffers, that have been 
previously allocated with one CREATE, i.e., it will also only take 
contiguous index sets. Am I still missing anything?

> If the hardware requires more than one buffer to operate, STREAMON could
> return ERANGE in a case there ane not enough queued, for example.

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

Re: [PATCH/RFC 1/4] V4L: add three new ioctl()s for multi-size videobuffer management

[Sakari Ailus]

On Tue, Jun 07, 2011 at 02:14:06PM +0200, Guennadi Liakhovetski wrote:
> On Mon, 6 Jun 2011, Sakari Ailus wrote:
> 
> > Hi Guennadi,
> > 
> > On Mon, Jun 06, 2011 at 03:10:54PM +0200, Guennadi Liakhovetski wrote:
> > > On Wed, 18 May 2011, Sakari Ailus wrote:
> > > 
> > > > Hi Guennadi and Laurent,
> > > > 
> > > > Guennadi Liakhovetski wrote:
> > > > > On Wed, 18 May 2011, Laurent Pinchart wrote:
> > > > > 
> > > > >> On Tuesday 17 May 2011 07:52:28 Sakari Ailus wrote:
> > > > >>> Guennadi Liakhovetski wrote:
> > > > > 
> > > > > [snip]
> > > > > 
> > > > >>>>> What about making it possible to pass an array of buffer indices to the
> > > > >>>>> user, just like VIDIOC_S_EXT_CTRLS does? I'm not sure if this would be
> > > > >>>>> perfect, but it would avoid the problem of requiring continuous ranges
> > > > >>>>> of buffer ids.
> > > > >>>>>
> > > > >>>>> struct v4l2_create_buffers {
> > > > >>>>>
> > > > >>>>> 	__u32			*index;
> > > > >>>>> 	__u32			count;
> > > > >>>>> 	__u32			flags;
> > > > >>>>> 	enum v4l2_memory        memory;
> > > > >>>>> 	__u32			size;
> > > > >>>>> 	struct v4l2_format	format;
> > > > >>>>>
> > > > >>>>> };
> > > > >>>>>
> > > > >>>>> Index would be a pointer to an array of buffer indices and its length
> > > > >>>>> would be count.
> > > > >>>>
> > > > >>>> I don't understand this. We do _not_ want to allow holes in indices. For
> > > > >>>> now we decide to not implement DESTROY at all. In this case indices just
> > > > >>>> increment contiguously.
> > > > >>>>
> > > > >>>> The next stage is to implement DESTROY, but only in strict reverse order
> > > > >>>> - without holes and in the same ranges, as buffers have been CREATEd
> > > > >>>> before. So, I really don't understand why we need arrays, sorry.
> > > > >>>
> > > > >>> Well, now that we're defining a second interface to make new buffer
> > > > >>> objects, I just thought it should be made as future-proof as we can.
> > > > >>
> > > > >> I second that. I don't like rushing new APIs to find out we need something 
> > > > >> else after 6 months.
> > > > > 
> > > > > Ok, so, we pass an array from user-space with CREATE of size count. The 
> > > > > kernel fills it with as many buffers entries as it has allocated. But 
> > > > > currently drivers are also allowed to allocate more buffers, than the 
> > > > > user-space has requested. What do we do in such a case?
> > > > 
> > > > That's a good point.
> > > > 
> > > > But even if there was no array, shouldn't the user be allowed to create
> > > > the buffers using a number of separate CREATE_BUF calls? The result
> > > > would be still the same n buffers as with a single call allocating the n
> > > > buffers at once.
> > > > 
> > > > Also, consider the (hopefully!) forthcoming DMA buffer management API
> > > > patches. It looks like that those buffers will be referred to by file
> > > > handles. To associate several DMA buffer objects to V4L2 buffers at
> > > > once, there would have to be an array of those objects.
> > > > 
> > > > <URL:http://www.spinics.net/lists/linux-media/msg32448.html>
> > > 
> > > So, does this mean now, that we have to wait for those APIs to become 
> > > solid before or even implemented we proceed with this one?
> > 
> > No. But I think we should take into account the foreseeable future. Any
> > which form the buffer id passing mechanism will take, it will very likely
> > involve referring to individual memory buffers the ids of which are not
> > contiguous ranges in a general case. In short, my point is that CREATE_BUF
> > should allow associating generic buffer ids to V4L2 buffers.
> 
> Ok, so, first point is: yes, it can well happen, that video buffers will 
> use some further buffer allocator as a back-end, that each videobuf will 
> possibly reference more than one of those memory buffers, and that those 
> memory buffers will have arbitrary IDs. AFAIC, this so far doesn't affect 
> our design of the CREATE ioctl(), right? As you say, both indices are 
> unrelated.

Both indices are unrelated. However, consider a case where the user wants to
allocate two video buffers with three planes on each. This would quite
possibly involve six different generic buffer object ids. How would buffers
like this be created using the current proposal, assuming the hardware
requires at least two of them?

> I can imagine, that in the future, when we implement DESTROY, videobuf 
> indices can become sparse. Say, if then we have indices 3 to 5 allocated, 
> and the user requests 4 buffers. We can either use indices 0-2 and 6, or 
> 6-9. Personally, I would go with the latter option. Maybe we'll have to 
> increase or completely eliminate the VIDEO_MAX_FRAME. But otherwise I 

I think we should specify indices returned by CREATE_BUF so that they are
not limited by VIDEO_MAX_FRAME. The actual implementation may still be that
they're limited to, say, 128, as it's likely easier that way. This makes it
extensible for the future use --- think of very high-speed cameras, for
example, where the number of required buffers may well be large.

> think, this would be the best way to handle this. Which means, our CREATE 
> ioctl() with contiguous indics should be fine. As for DESTROY, the idea 
> was to only allow destroying same sets of buffers, that have been 
> previously allocated with one CREATE, i.e., it will also only take 
> contiguous index sets. Am I still missing anything?
> 
> > If the hardware requires more than one buffer to operate, STREAMON could
> > return ERANGE in a case there ane not enough queued, for example.

What do you think of this?

When and how would be the userptr given to the CREATE_BUFS/SUBMIT_BUFS? It
has to be known at SUBMIT_BUF time to actually pin the buffer to memory. The
same would apply to generic DMA buffers in the future.

I don't think that artificial bindings should be created between independent
V4L2 buffers. I don't fiercely object this however, if others agree on it,
but there are limitations. The usability of the feature is limited to
USERPTR and MMAP memory types. Generic buffer objects may not make use of it
since each V4L2 buffer would have to be associated with one or more generic
buffer references. For the same reason, the minimum number of buffers may
not be enforced by CREATE_BUFS.

-- 
Sakari Ailus
sakari.ailus@iki.fi