[RFC 00/13] Support for dmabuf exporting for videobuf2

[RFC 00/13] Support for dmabuf exporting for videobuf2

[Tomasz Stanislawski]

Hello everyone,
The patches adds support for DMABUF [1] exporting to V4L2 stack.  It was
updated from [7] after Laurent Pinchart's review.  The previous patchset was
split into two parts.  The support for DMABUF importing was posted in [2]. The
exporter part is dependant on DMA mapping redesign [3] which is not merged into
the mainline. Therefore it is posted as a separate patchset.

This patchset is rebased on 3.4-rc1 plus the following patchsets:

- support for DMABUF importing in V4L2 [2]
- DMA mapping redesign [3]
- support for dma_get_pages extension [4]
- support for vmap extension to dmabuf framework by Dave Airlie [5][6] 

[1] https://lkml.org/lkml/2011/12/26/29
[2] http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/46586
[3] http://thread.gmane.org/gmane.linux.kernel.cross-arch/12819
[4] http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/43793/focus=43803
[5] http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/46713
[6] http://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-dmabuf2&id=c481a5451744fe3c4c950a446be10d3212d633d8
[7] http://thread.gmane.org/gmane.comp.video.dri.devel/66213

Marek Szyprowski (1):
  v4l: vb2-dma-contig: let mmap method to use dma_mmap_coherent call

Tomasz Stanislawski (12):
  v4l: add buffer exporting via dmabuf
  v4l: vb2: add buffer exporting via dmabuf
  v4l: vb2-dma-contig: add setup of sglist for MMAP buffers
  v4l: vb2-dma-contig: add support for DMABUF exporting
  v4l: vb2-dma-contig: add vmap/kmap for dmabuf exporting
  v4l: vb2-dma-contig: change map/unmap behaviour for importers
  v4l: vb2-dma-contig: change map/unmap behaviour for exporters
  v4l: s5p-tv: mixer: support for dmabuf importing
  v4l: s5p-tv: mixer: support for dmabuf exporting
  v4l: fimc: support for dmabuf importing
  v4l: fimc: support for dmabuf exporting
  v4l: vivi: support for dmabuf exporting

 drivers/media/video/Kconfig                 |    1 +
 drivers/media/video/s5p-fimc/fimc-capture.c |   11 ++-
 drivers/media/video/s5p-tv/Kconfig          |    1 +
 drivers/media/video/s5p-tv/mixer_video.c    |   12 ++-
 drivers/media/video/v4l2-compat-ioctl32.c   |    1 +
 drivers/media/video/v4l2-ioctl.c            |    7 +
 drivers/media/video/videobuf2-core.c        |   66 ++++++++
 drivers/media/video/videobuf2-dma-contig.c  |  224 +++++++++++++++++++++++++-
 drivers/media/video/vivi.c                  |    9 +
 include/linux/videodev2.h                   |   23 +++
 include/media/v4l2-ioctl.h                  |    2 +
 include/media/videobuf2-core.h              |    2 +
 12 files changed, 348 insertions(+), 11 deletions(-)

-- 
1.7.5.4

[RFC 01/13] v4l: add buffer exporting via dmabuf

[Tomasz Stanislawski]

This patch adds extension to V4L2 api. It allow to export a mmap buffer as file
descriptor. New ioctl VIDIOC_EXPBUF is added. It takes a buffer offset used by
mmap and return a file descriptor on success.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/v4l2-compat-ioctl32.c |    1 +
 drivers/media/video/v4l2-ioctl.c          |    7 +++++++
 include/linux/videodev2.h                 |   23 +++++++++++++++++++++++
 include/media/v4l2-ioctl.h                |    2 ++
 4 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
index 2829d25..f32b291 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -954,6 +954,7 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
 	case VIDIOC_S_FBUF32:
 	case VIDIOC_OVERLAY32:
 	case VIDIOC_QBUF32:
+	case VIDIOC_EXPBUF:
 	case VIDIOC_DQBUF32:
 	case VIDIOC_STREAMON32:
 	case VIDIOC_STREAMOFF32:
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 5b2ec1f..3ca9d68 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -207,6 +207,7 @@ static const char *v4l2_ioctls[] = {
 	[_IOC_NR(VIDIOC_S_FBUF)]           = "VIDIOC_S_FBUF",
 	[_IOC_NR(VIDIOC_OVERLAY)]          = "VIDIOC_OVERLAY",
 	[_IOC_NR(VIDIOC_QBUF)]             = "VIDIOC_QBUF",
+	[_IOC_NR(VIDIOC_EXPBUF)]           = "VIDIOC_EXPBUF",
 	[_IOC_NR(VIDIOC_DQBUF)]            = "VIDIOC_DQBUF",
 	[_IOC_NR(VIDIOC_STREAMON)]         = "VIDIOC_STREAMON",
 	[_IOC_NR(VIDIOC_STREAMOFF)]        = "VIDIOC_STREAMOFF",
@@ -938,6 +939,12 @@ static long __video_do_ioctl(struct file *file,
 			dbgbuf(cmd, vfd, p);
 		break;
 	}
+	case VIDIOC_EXPBUF:
+	{
+		if (ops->vidioc_expbuf)
+			ret = ops->vidioc_expbuf(file, fh, arg);
+		break;
+	}
 	case VIDIOC_DQBUF:
 	{
 		struct v4l2_buffer *p = arg;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 38259bf..627e235 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -680,6 +680,28 @@ struct v4l2_buffer {
 #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE	0x0800
 #define V4L2_BUF_FLAG_NO_CACHE_CLEAN		0x1000
 
+/**
+ * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
+ *
+ * @fd:		file descriptor associated with DMABUF (set by driver)
+ * @mem_offset:	a "cookie" that is passed to mmap() as offset
+ * @flags:	flags for newly created file, currently only O_CLOEXEC is
+ *		supported, refer to manual of open syscall for more details
+ *
+ * Contains data used for exporting a video buffer as DMABUF file
+ * descriptor. Uses the same 'cookie' as mmap() syscall. All reserved fields
+ * must be set to zero. The field reserved0 is expected to become a structure
+ * 'type' allowing an alternative layout of the structure content. Therefore
+ * this field should not be used for any other extensions.
+ */
+struct v4l2_exportbuffer {
+	__u32		fd;
+	__u32		reserved0;
+	__u32		mem_offset;
+	__u32		flags;
+	__u32		reserved[12];
+};
+
 /*
  *	O V E R L A Y   P R E V I E W
  */
@@ -2327,6 +2349,7 @@ struct v4l2_create_buffers {
 #define VIDIOC_S_FBUF		 _IOW('V', 11, struct v4l2_framebuffer)
 #define VIDIOC_OVERLAY		 _IOW('V', 14, int)
 #define VIDIOC_QBUF		_IOWR('V', 15, struct v4l2_buffer)
+#define VIDIOC_EXPBUF		_IOWR('V', 16, struct v4l2_exportbuffer)
 #define VIDIOC_DQBUF		_IOWR('V', 17, struct v4l2_buffer)
 #define VIDIOC_STREAMON		 _IOW('V', 18, int)
 #define VIDIOC_STREAMOFF	 _IOW('V', 19, int)
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index 3cb939c..c161ec3 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -119,6 +119,8 @@ struct v4l2_ioctl_ops {
 	int (*vidioc_reqbufs) (struct file *file, void *fh, struct v4l2_requestbuffers *b);
 	int (*vidioc_querybuf)(struct file *file, void *fh, struct v4l2_buffer *b);
 	int (*vidioc_qbuf)    (struct file *file, void *fh, struct v4l2_buffer *b);
+	int (*vidioc_expbuf)  (struct file *file, void *fh,
+				struct v4l2_exportbuffer *e);
 	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);
-- 
1.7.5.4

[RFC 02/13] v4l: vb2: add buffer exporting via dmabuf

[Tomasz Stanislawski]

This patch adds extension to videobuf2-core. It allow to export a mmap buffer
as a file descriptor.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/videobuf2-core.c |   66 ++++++++++++++++++++++++++++++++++
 include/media/videobuf2-core.h       |    2 +
 2 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
index b37feea..ff902aa 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -1710,6 +1710,72 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
 }
 
 /**
+ * vb2_expbuf() - Export a buffer as a file descriptor
+ * @q:		videobuf2 queue
+ * @b:		export buffer structure passed from userspace to vidioc_expbuf
+ *		handler in driver
+ *
+ * The return values from this function are intended to be directly returned
+ * from vidioc_expbuf handler in driver.
+ */
+int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb)
+{
+	struct vb2_buffer *vb = NULL;
+	struct vb2_plane *vb_plane;
+	unsigned int buffer, plane;
+	int ret;
+	struct dma_buf *dbuf;
+
+	if (q->memory != V4L2_MEMORY_MMAP) {
+		dprintk(1, "Queue is not currently set up for mmap\n");
+		return -EINVAL;
+	}
+
+	if (!q->mem_ops->get_dmabuf) {
+		dprintk(1, "Queue does not support DMA buffer exporting\n");
+		return -EINVAL;
+	}
+
+	if (eb->flags & ~O_CLOEXEC) {
+		dprintk(1, "Queue supports only O_CLOEXEC flag\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Find the plane corresponding to the offset passed by userspace.
+	 */
+	ret = __find_plane_by_offset(q, eb->mem_offset, &buffer, &plane);
+	if (ret) {
+		dprintk(1, "invalid offset %u\n", eb->mem_offset);
+		return ret;
+	}
+
+	vb = q->bufs[buffer];
+	vb_plane = &vb->planes[plane];
+
+	dbuf = call_memop(q, get_dmabuf, vb_plane->mem_priv);
+	if (IS_ERR_OR_NULL(dbuf)) {
+		dprintk(1, "Failed to export buffer %d, plane %d\n",
+			buffer, plane);
+		return -EINVAL;
+	}
+
+	ret = dma_buf_fd(dbuf, eb->flags);
+	if (ret < 0) {
+		dprintk(3, "buffer %d, plane %d failed to export (%d)\n",
+			buffer, plane, ret);
+		return ret;
+	}
+
+	dprintk(3, "buffer %d, plane %d exported as %d descriptor\n",
+		buffer, plane, ret);
+	eb->fd = ret;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vb2_expbuf);
+
+/**
  * vb2_mmap() - map video buffers into application address space
  * @q:		videobuf2 queue
  * @vma:	vma passed to the mmap file operation handler in the driver
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 244165a..3bd4225 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -81,6 +81,7 @@ struct vb2_fileio_data;
 struct vb2_mem_ops {
 	void		*(*alloc)(void *alloc_ctx, unsigned long size);
 	void		(*put)(void *buf_priv);
+	struct dma_buf *(*get_dmabuf)(void *buf_priv);
 
 	void		*(*get_userptr)(void *alloc_ctx, unsigned long vaddr,
 					unsigned long size, int write);
@@ -354,6 +355,7 @@ int vb2_queue_init(struct vb2_queue *q);
 void vb2_queue_release(struct vb2_queue *q);
 
 int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b);
+int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb);
 int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking);
 
 int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type);
-- 
1.7.5.4

[RFC 03/13] v4l: vb2-dma-contig: let mmap method to use dma_mmap_coherent call

[Tomasz Stanislawski]

From: Marek Szyprowski <m.szyprowski@samsung.com>

Let mmap method to use dma_mmap_coherent call.  This patch depends on DMA
mapping redesign patches because the usage of dma_mmap_coherent breaks
dma-contig allocator for architectures other than ARM and AVR.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/media/video/videobuf2-dma-contig.c |   28 ++++++++++++++++++++++++++--
 1 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index 6329483..f4df9e2 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -224,14 +224,38 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
 static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
 {
 	struct vb2_dc_buf *buf = buf_priv;
+	int ret;
 
 	if (!buf) {
 		printk(KERN_ERR "No buffer to map\n");
 		return -EINVAL;
 	}
 
-	return vb2_mmap_pfn_range(vma, buf->dma_addr, buf->size,
-				  &vb2_common_vm_ops, &buf->handler);
+	/*
+	 * dma_mmap_* uses vm_pgoff as in-buffer offset, but we want to
+	 * map whole buffer
+	 */
+	vma->vm_pgoff = 0;
+
+	ret = dma_mmap_coherent(buf->dev, vma, buf->vaddr,
+		buf->dma_addr, buf->size);
+
+	if (ret) {
+		printk(KERN_ERR "Remapping memory failed, error: %d\n", ret);
+		return ret;
+	}
+
+	vma->vm_flags		|= VM_DONTEXPAND | VM_RESERVED;
+	vma->vm_private_data	= &buf->handler;
+	vma->vm_ops		= &vb2_common_vm_ops;
+
+	vma->vm_ops->open(vma);
+
+	printk(KERN_DEBUG "%s: mapped dma addr 0x%08lx at 0x%08lx, size %ld\n",
+		__func__, (unsigned long)buf->dma_addr, vma->vm_start,
+		buf->size);
+
+	return 0;
 }
 
 /*********************************************/
-- 
1.7.5.4

[RFC 04/13] v4l: vb2-dma-contig: add setup of sglist for MMAP buffers

[Tomasz Stanislawski]

This patch adds the setup of sglist list for MMAP buffers.
It is needed for buffer exporting via DMABUF mechanism.

This patch depends on dma_get_pages extension to DMA api.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/videobuf2-dma-contig.c |   51 ++++++++++++++++++++++++++-
 1 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index f4df9e2..0cdcd2b 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -31,6 +31,7 @@ struct vb2_dc_buf {
 	/* MMAP related */
 	struct vb2_vmarea_handler	handler;
 	atomic_t			refcount;
+	struct sg_table			*sgt_base;
 
 	/* USERPTR related */
 	struct vm_area_struct		*vma;
@@ -189,6 +190,7 @@ static void vb2_dc_put(void *buf_priv)
 	if (!atomic_dec_and_test(&buf->refcount))
 		return;
 
+	vb2_dc_release_sgtable(buf->sgt_base);
 	dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
 	kfree(buf);
 }
@@ -197,6 +199,9 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
 {
 	struct device *dev = alloc_ctx;
 	struct vb2_dc_buf *buf;
+	int ret = -ENOMEM;
+	int n_pages;
+	struct page **pages = NULL;
 
 	buf = kzalloc(sizeof *buf, GFP_KERNEL);
 	if (!buf)
@@ -205,10 +210,41 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
 	buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, GFP_KERNEL);
 	if (!buf->vaddr) {
 		dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
-		kfree(buf);
-		return ERR_PTR(-ENOMEM);
+		goto fail_buf;
+	}
+
+	WARN_ON((unsigned long)buf->vaddr & ~PAGE_MASK);
+	WARN_ON(buf->dma_addr & ~PAGE_MASK);
+
+	n_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+
+	pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL);
+	if (!pages) {
+		dev_err(dev, "failed to alloc page table\n");
+		goto fail_dma;
+	}
+
+	ret = dma_get_pages(dev, buf->vaddr, buf->dma_addr, pages, n_pages);
+	if (ret < 0) {
+		dev_err(dev, "failed to get buffer pages from DMA API\n");
+		goto fail_pages;
+	}
+	if (ret != n_pages) {
+		ret = -EFAULT;
+		dev_err(dev, "failed to get all pages from DMA API\n");
+		goto fail_pages;
 	}
 
+	buf->sgt_base = vb2_dc_pages_to_sgt(pages, n_pages, 0, 0);
+	if (IS_ERR(buf->sgt_base)) {
+		ret = PTR_ERR(buf->sgt_base);
+		dev_err(dev, "failed to prepare sg table\n");
+		goto fail_pages;
+	}
+
+	/* pages are no longer needed */
+	kfree(pages);
+
 	buf->dev = dev;
 	buf->size = size;
 
@@ -219,6 +255,17 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
 	atomic_inc(&buf->refcount);
 
 	return buf;
+
+fail_pages:
+	kfree(pages);
+
+fail_dma:
+	dma_free_coherent(dev, size, buf->vaddr, buf->dma_addr);
+
+fail_buf:
+	kfree(buf);
+
+	return ERR_PTR(ret);
 }
 
 static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
-- 
1.7.5.4

[RFC 05/13] v4l: vb2-dma-contig: add support for DMABUF exporting

[Tomasz Stanislawski]

This patch adds support for exporting a dma-contig buffer using
DMABUF interface.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/videobuf2-dma-contig.c |  128 ++++++++++++++++++++++++++++
 1 files changed, 128 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index 0cdcd2b..e1ad47e 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -31,6 +31,7 @@ struct vb2_dc_buf {
 	/* MMAP related */
 	struct vb2_vmarea_handler	handler;
 	atomic_t			refcount;
+	struct dma_buf			*dma_buf;
 	struct sg_table			*sgt_base;
 
 	/* USERPTR related */
@@ -190,6 +191,8 @@ static void vb2_dc_put(void *buf_priv)
 	if (!atomic_dec_and_test(&buf->refcount))
 		return;
 
+	if (buf->dma_buf)
+		dma_buf_put(buf->dma_buf);
 	vb2_dc_release_sgtable(buf->sgt_base);
 	dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
 	kfree(buf);
@@ -306,6 +309,130 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
 }
 
 /*********************************************/
+/*         DMABUF ops for exporters          */
+/*********************************************/
+
+struct vb2_dc_attachment {
+	struct sg_table sgt;
+	enum dma_data_direction dir;
+};
+
+static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev,
+	struct dma_buf_attachment *dbuf_attach)
+{
+	/* nothing to be done */
+	return 0;
+}
+
+static void vb2_dc_dmabuf_ops_detach(struct dma_buf *dbuf,
+	struct dma_buf_attachment *db_attach)
+{
+	struct vb2_dc_attachment *attach = db_attach->priv;
+	struct sg_table *sgt;
+
+	if (!attach)
+		return;
+
+	sgt = &attach->sgt;
+
+	dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->nents, attach->dir);
+	sg_free_table(sgt);
+	kfree(attach);
+	db_attach->priv = NULL;
+}
+
+static struct sg_table *vb2_dc_dmabuf_ops_map(
+	struct dma_buf_attachment *db_attach, enum dma_data_direction dir)
+{
+	struct dma_buf *dbuf = db_attach->dmabuf;
+	struct vb2_dc_buf *buf = dbuf->priv;
+	struct vb2_dc_attachment *attach = db_attach->priv;
+	struct sg_table *sgt;
+	struct scatterlist *rd, *wr;
+	int i, ret;
+
+	/* return previously mapped sg table */
+	if (attach)
+		return &attach->sgt;
+
+	attach = kzalloc(sizeof *attach, GFP_KERNEL);
+	if (!attach)
+		return ERR_PTR(-ENOMEM);
+
+	sgt = &attach->sgt;
+	attach->dir = dir;
+
+	/* copying the buf->base_sgt to attachment */
+	ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL);
+	if (ret) {
+		kfree(attach);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	rd = buf->sgt_base->sgl;
+	wr = sgt->sgl;
+	for (i = 0; i < sgt->orig_nents; ++i) {
+		sg_set_page(wr, sg_page(rd), rd->length, rd->offset);
+		rd = sg_next(rd);
+		wr = sg_next(wr);
+	}
+
+	/* mapping new sglist to the client */
+	ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dir);
+	if (ret <= 0) {
+		printk(KERN_ERR "failed to map scatterlist\n");
+		sg_free_table(sgt);
+		kfree(attach);
+		return ERR_PTR(-EIO);
+	}
+
+	db_attach->priv = attach;
+
+	return sgt;
+}
+
+static void vb2_dc_dmabuf_ops_unmap(struct dma_buf_attachment *db_attach,
+	struct sg_table *sgt, enum dma_data_direction dir)
+{
+	/* nothing to be done here */
+}
+
+static void vb2_dc_dmabuf_ops_release(struct dma_buf *dbuf)
+{
+	/* drop reference obtained in vb2_dc_get_dmabuf */
+	vb2_dc_put(dbuf->priv);
+}
+
+static struct dma_buf_ops vb2_dc_dmabuf_ops = {
+	.attach = vb2_dc_dmabuf_ops_attach,
+	.detach = vb2_dc_dmabuf_ops_detach,
+	.map_dma_buf = vb2_dc_dmabuf_ops_map,
+	.unmap_dma_buf = vb2_dc_dmabuf_ops_unmap,
+	.release = vb2_dc_dmabuf_ops_release,
+};
+
+static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv)
+{
+	struct vb2_dc_buf *buf = buf_priv;
+	struct dma_buf *dbuf;
+
+	if (buf->dma_buf)
+		return buf->dma_buf;
+
+	/* dmabuf keeps reference to vb2 buffer */
+	atomic_inc(&buf->refcount);
+	dbuf = dma_buf_export(buf, &vb2_dc_dmabuf_ops, buf->size, 0);
+	if (IS_ERR(dbuf)) {
+		atomic_dec(&buf->refcount);
+		return NULL;
+	}
+
+	buf->dma_buf = dbuf;
+
+	return dbuf;
+}
+
+/*********************************************/
 /*       callbacks for USERPTR buffers       */
 /*********************************************/
 
@@ -615,6 +742,7 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf,
 const struct vb2_mem_ops vb2_dma_contig_memops = {
 	.alloc		= vb2_dc_alloc,
 	.put		= vb2_dc_put,
+	.get_dmabuf	= vb2_dc_get_dmabuf,
 	.cookie		= vb2_dc_cookie,
 	.vaddr		= vb2_dc_vaddr,
 	.mmap		= vb2_dc_mmap,
-- 
1.7.5.4

[RFC 06/13] v4l: vb2-dma-contig: add vmap/kmap for dmabuf exporting

[Tomasz Stanislawski]

This patch adds support for vmap and kmap callbacks
for DMABUF exporter.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/videobuf2-dma-contig.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index e1ad47e..537926b 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -403,11 +403,28 @@ static void vb2_dc_dmabuf_ops_release(struct dma_buf *dbuf)
 	vb2_dc_put(dbuf->priv);
 }
 
+static void *vb2_dc_dmabuf_ops_kmap(struct dma_buf *dbuf, unsigned long pgnum)
+{
+	struct vb2_dc_buf *buf = dbuf->priv;
+
+	return buf->vaddr + pgnum * PAGE_SIZE;
+}
+
+static void *vb2_dc_dmabuf_ops_vmap(struct dma_buf *dbuf)
+{
+	struct vb2_dc_buf *buf = dbuf->priv;
+
+	return buf->vaddr;
+}
+
 static struct dma_buf_ops vb2_dc_dmabuf_ops = {
 	.attach = vb2_dc_dmabuf_ops_attach,
 	.detach = vb2_dc_dmabuf_ops_detach,
 	.map_dma_buf = vb2_dc_dmabuf_ops_map,
 	.unmap_dma_buf = vb2_dc_dmabuf_ops_unmap,
+	.kmap = vb2_dc_dmabuf_ops_kmap,
+	.kmap_atomic = vb2_dc_dmabuf_ops_kmap,
+	.vmap = vb2_dc_dmabuf_ops_vmap,
 	.release = vb2_dc_dmabuf_ops_release,
 };
 
-- 
1.7.5.4

[RFC 07/13] v4l: vb2-dma-contig: change map/unmap behaviour for importers

[Tomasz Stanislawski]

The DMABUF documentation says that the map_dma_buf callback should return
scatterlist that is mapped into a caller's address space. In practice, almost
none of existing implementations of DMABUF exporter does it.  This patch breaks
the DMABUF specification in order to allow exchange DMABUF buffers between
other APIs like DRM.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/videobuf2-dma-contig.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index 537926b..7f4a58a 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -652,7 +652,7 @@ static int vb2_dc_map_dmabuf(void *mem_priv)
 	struct vb2_dc_buf *buf = mem_priv;
 	struct sg_table *sgt;
 	unsigned long contig_size;
-	int ret = 0;
+	int ret = -EFAULT;
 
 	if (WARN_ON(!buf->db_attach)) {
 		printk(KERN_ERR "trying to pin a non attached buffer\n");
@@ -671,12 +671,20 @@ static int vb2_dc_map_dmabuf(void *mem_priv)
 		return -EINVAL;
 	}
 
+	/* mapping new sglist to the client */
+	sgt->nents = dma_map_sg(buf->dev, sgt->sgl, sgt->orig_nents,
+		buf->dma_dir);
+	if (sgt->nents <= 0) {
+		printk(KERN_ERR "failed to map scatterlist\n");
+		goto fail_map_attachment;
+	}
+
 	/* checking if dmabuf is big enough to store contiguous chunk */
 	contig_size = vb2_dc_get_contiguous_size(sgt);
 	if (contig_size < buf->size) {
-		printk(KERN_ERR "contiguous chunk of dmabuf is too small\n");
-		ret = -EFAULT;
-		goto fail_map;
+		printk(KERN_ERR "contiguous chunk is too small %lu/%lu b\n",
+			contig_size, buf->size);
+		goto fail_map_sg;
 	}
 
 	buf->dma_addr = sg_dma_address(sgt->sgl);
@@ -684,7 +692,10 @@ static int vb2_dc_map_dmabuf(void *mem_priv)
 
 	return 0;
 
-fail_map:
+fail_map_sg:
+	dma_unmap_sg(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
+
+fail_map_attachment:
 	dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
 
 	return ret;
@@ -705,6 +716,7 @@ static void vb2_dc_unmap_dmabuf(void *mem_priv)
 		return;
 	}
 
+	dma_unmap_sg(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
 	dma_buf_unmap_attachment(buf->db_attach, sgt, buf->dma_dir);
 
 	buf->dma_addr = 0;
-- 
1.7.5.4

[RFC 08/13] v4l: vb2-dma-contig: change map/unmap behaviour for exporters

[Tomasz Stanislawski]

The DMABUF documentation says that the map_dma_buf callback should return
scatterlist that is mapped into a caller's address space. In practice, almost
none of existing implementations of DMABUF exporter does it.  This patch breaks
the DMABUF specification in order to allow exchange DMABUF buffers between
other APIs like DRM.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/videobuf2-dma-contig.c |   42 ++++++---------------------
 1 files changed, 10 insertions(+), 32 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index 7f4a58a..a6676bd 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -312,11 +312,6 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)
 /*         DMABUF ops for exporters          */
 /*********************************************/
 
-struct vb2_dc_attachment {
-	struct sg_table sgt;
-	enum dma_data_direction dir;
-};
-
 static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev,
 	struct dma_buf_attachment *dbuf_attach)
 {
@@ -327,17 +322,13 @@ static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev,
 static void vb2_dc_dmabuf_ops_detach(struct dma_buf *dbuf,
 	struct dma_buf_attachment *db_attach)
 {
-	struct vb2_dc_attachment *attach = db_attach->priv;
-	struct sg_table *sgt;
+	struct sg_table *sgt = db_attach->priv;
 
-	if (!attach)
+	if (!sgt)
 		return;
 
-	sgt = &attach->sgt;
-
-	dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->nents, attach->dir);
 	sg_free_table(sgt);
-	kfree(attach);
+	kfree(sgt);
 	db_attach->priv = NULL;
 }
 
@@ -346,26 +337,22 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
 {
 	struct dma_buf *dbuf = db_attach->dmabuf;
 	struct vb2_dc_buf *buf = dbuf->priv;
-	struct vb2_dc_attachment *attach = db_attach->priv;
-	struct sg_table *sgt;
+	struct sg_table *sgt = db_attach->priv;
 	struct scatterlist *rd, *wr;
 	int i, ret;
 
 	/* return previously mapped sg table */
-	if (attach)
-		return &attach->sgt;
+	if (sgt)
+		return sgt;
 
-	attach = kzalloc(sizeof *attach, GFP_KERNEL);
-	if (!attach)
+	sgt = kzalloc(sizeof *sgt, GFP_KERNEL);
+	if (!sgt)
 		return ERR_PTR(-ENOMEM);
 
-	sgt = &attach->sgt;
-	attach->dir = dir;
-
 	/* copying the buf->base_sgt to attachment */
 	ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL);
 	if (ret) {
-		kfree(attach);
+		kfree(sgt);
 		return ERR_PTR(-ENOMEM);
 	}
 
@@ -377,16 +364,7 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(
 		wr = sg_next(wr);
 	}
 
-	/* mapping new sglist to the client */
-	ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dir);
-	if (ret <= 0) {
-		printk(KERN_ERR "failed to map scatterlist\n");
-		sg_free_table(sgt);
-		kfree(attach);
-		return ERR_PTR(-EIO);
-	}
-
-	db_attach->priv = attach;
+	db_attach->priv = sgt;
 
 	return sgt;
 }
-- 
1.7.5.4

[RFC 09/13] v4l: s5p-tv: mixer: support for dmabuf importing

[Tomasz Stanislawski]

This patch enhances s5p-tv with support for DMABUF importing via
V4L2_MEMORY_DMABUF memory type.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/s5p-tv/Kconfig       |    1 +
 drivers/media/video/s5p-tv/mixer_video.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/s5p-tv/Kconfig b/drivers/media/video/s5p-tv/Kconfig
index f248b28..2e80126 100644
--- a/drivers/media/video/s5p-tv/Kconfig
+++ b/drivers/media/video/s5p-tv/Kconfig
@@ -10,6 +10,7 @@ config VIDEO_SAMSUNG_S5P_TV
 	bool "Samsung TV driver for S5P platform (experimental)"
 	depends on PLAT_S5P && PM_RUNTIME
 	depends on EXPERIMENTAL
+	select DMA_SHARED_BUFFER
 	default n
 	---help---
 	  Say Y here to enable selecting the TV output devices for
diff --git a/drivers/media/video/s5p-tv/mixer_video.c b/drivers/media/video/s5p-tv/mixer_video.c
index f7ca5cc..6b45d93 100644
--- a/drivers/media/video/s5p-tv/mixer_video.c
+++ b/drivers/media/video/s5p-tv/mixer_video.c
@@ -1074,7 +1074,7 @@ struct mxr_layer *mxr_base_layer_create(struct mxr_device *mdev,
 
 	layer->vb_queue = (struct vb2_queue) {
 		.type = V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE,
-		.io_modes = VB2_MMAP | VB2_USERPTR,
+		.io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF,
 		.drv_priv = layer,
 		.buf_struct_size = sizeof(struct mxr_buffer),
 		.ops = &mxr_video_qops,
-- 
1.7.5.4

[RFC 10/13] v4l: s5p-tv: mixer: support for dmabuf exporting

[Tomasz Stanislawski]

This patch enhances s5p-tv with support for DMABUF exporting via
VIDIOC_EXPBUF ioctl.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/s5p-tv/mixer_video.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/s5p-tv/mixer_video.c b/drivers/media/video/s5p-tv/mixer_video.c
index 6b45d93..f08edbf 100644
--- a/drivers/media/video/s5p-tv/mixer_video.c
+++ b/drivers/media/video/s5p-tv/mixer_video.c
@@ -697,6 +697,15 @@ static int mxr_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p)
 	return vb2_dqbuf(&layer->vb_queue, p, file->f_flags & O_NONBLOCK);
 }
 
+static int mxr_expbuf(struct file *file, void *priv,
+	struct v4l2_exportbuffer *eb)
+{
+	struct mxr_layer *layer = video_drvdata(file);
+
+	mxr_dbg(layer->mdev, "%s:%d\n", __func__, __LINE__);
+	return vb2_expbuf(&layer->vb_queue, eb);
+}
+
 static int mxr_streamon(struct file *file, void *priv, enum v4l2_buf_type i)
 {
 	struct mxr_layer *layer = video_drvdata(file);
@@ -724,6 +733,7 @@ static const struct v4l2_ioctl_ops mxr_ioctl_ops = {
 	.vidioc_querybuf = mxr_querybuf,
 	.vidioc_qbuf = mxr_qbuf,
 	.vidioc_dqbuf = mxr_dqbuf,
+	.vidioc_expbuf = mxr_expbuf,
 	/* Streaming control */
 	.vidioc_streamon = mxr_streamon,
 	.vidioc_streamoff = mxr_streamoff,
-- 
1.7.5.4

[RFC 11/13] v4l: fimc: support for dmabuf importing

[Tomasz Stanislawski]

This patch enhances s5p-fimc with support for DMABUF importing via
V4L2_MEMORY_DMABUF memory type.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/Kconfig                 |    1 +
 drivers/media/video/s5p-fimc/fimc-capture.c |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index d6294d6..2c3250a 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -1134,6 +1134,7 @@ config  VIDEO_SAMSUNG_S5P_FIMC
 		VIDEO_V4L2_SUBDEV_API && EXPERIMENTAL
 	select VIDEOBUF2_DMA_CONTIG
 	select V4L2_MEM2MEM_DEV
+	select DMA_SHARED_BUFFER
 	---help---
 	  This is a v4l2 driver for Samsung S5P and EXYNOS4 camera
 	  host interface and video postprocessor.
diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c
index b06efd2..38fb39e 100644
--- a/drivers/media/video/s5p-fimc/fimc-capture.c
+++ b/drivers/media/video/s5p-fimc/fimc-capture.c
@@ -1530,7 +1530,7 @@ int fimc_register_capture_device(struct fimc_dev *fimc,
 	q = &fimc->vid_cap.vbq;
 	memset(q, 0, sizeof(*q));
 	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
-	q->io_modes = VB2_MMAP | VB2_USERPTR;
+	q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF;
 	q->drv_priv = fimc->vid_cap.ctx;
 	q->ops = &fimc_capture_qops;
 	q->mem_ops = &vb2_dma_contig_memops;
-- 
1.7.5.4

[RFC 12/13] v4l: fimc: support for dmabuf exporting

[Tomasz Stanislawski]

This patch enhances s5p-fimc with support for DMABUF exporting via
VIDIOC_EXPBUF ioctl.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-capture.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c
index 38fb39e..78a4db9 100644
--- a/drivers/media/video/s5p-fimc/fimc-capture.c
+++ b/drivers/media/video/s5p-fimc/fimc-capture.c
@@ -1011,6 +1011,14 @@ static int fimc_cap_qbuf(struct file *file, void *priv,
 	return vb2_qbuf(&fimc->vid_cap.vbq, buf);
 }
 
+static int fimc_cap_expbuf(struct file *file, void *priv,
+			  struct v4l2_exportbuffer *eb)
+{
+	struct fimc_dev *fimc = video_drvdata(file);
+
+	return vb2_expbuf(&fimc->vid_cap.vbq, eb);
+}
+
 static int fimc_cap_dqbuf(struct file *file, void *priv,
 			   struct v4l2_buffer *buf)
 {
@@ -1146,6 +1154,7 @@ static const struct v4l2_ioctl_ops fimc_capture_ioctl_ops = {
 
 	.vidioc_qbuf			= fimc_cap_qbuf,
 	.vidioc_dqbuf			= fimc_cap_dqbuf,
+	.vidioc_expbuf			= fimc_cap_expbuf,
 
 	.vidioc_prepare_buf		= fimc_cap_prepare_buf,
 	.vidioc_create_bufs		= fimc_cap_create_bufs,
-- 
1.7.5.4

[RFC 13/13] v4l: vivi: support for dmabuf exporting

[Tomasz Stanislawski]

This patch enhances vivi driver with a support for DMABUF exporting via
VIDIOC_EXPBUF ioctl.

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/vivi.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index 489d685..e34aa70 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -947,6 +947,14 @@ static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *p)
 	return vb2_dqbuf(&dev->vb_vidq, p, file->f_flags & O_NONBLOCK);
 }
 
+static int vidioc_expbuf(struct file *file, void *priv,
+	struct v4l2_exportbuffer *eb)
+{
+	struct vivi_dev *dev = video_drvdata(file);
+	return vb2_expbuf(&dev->vb_vidq, eb);
+}
+
+
 static int vidioc_streamon(struct file *file, void *priv, enum v4l2_buf_type i)
 {
 	struct vivi_dev *dev = video_drvdata(file);
@@ -1185,6 +1193,7 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
 	.vidioc_querybuf      = vidioc_querybuf,
 	.vidioc_qbuf          = vidioc_qbuf,
 	.vidioc_dqbuf         = vidioc_dqbuf,
+	.vidioc_expbuf        = vidioc_expbuf,
 	.vidioc_s_std         = vidioc_s_std,
 	.vidioc_enum_input    = vidioc_enum_input,
 	.vidioc_g_input       = vidioc_g_input,
-- 
1.7.5.4

Re: [RFC 01/13] v4l: add buffer exporting via dmabuf

[Laurent Pinchart]

Hi Tomasz,

Thanks for the patch.

On Tuesday 10 April 2012 15:10:35 Tomasz Stanislawski wrote:
> This patch adds extension to V4L2 api. It allow to export a mmap buffer as
> file descriptor. New ioctl VIDIOC_EXPBUF is added. It takes a buffer offset
> used by mmap and return a file descriptor on success.
> 
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

This mostly looks good to me, except for the lack of documentation of course 
:-)

> ---
>  drivers/media/video/v4l2-compat-ioctl32.c |    1 +
>  drivers/media/video/v4l2-ioctl.c          |    7 +++++++
>  include/linux/videodev2.h                 |   23 +++++++++++++++++++++++
>  include/media/v4l2-ioctl.h                |    2 ++
>  4 files changed, 33 insertions(+), 0 deletions(-)

[snip]

> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 38259bf..627e235 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -680,6 +680,28 @@ struct v4l2_buffer {
>  #define V4L2_BUF_FLAG_NO_CACHE_INVALIDATE	0x0800
>  #define V4L2_BUF_FLAG_NO_CACHE_CLEAN		0x1000
> 
> +/**
> + * struct v4l2_exportbuffer - export of video buffer as DMABUF file
> descriptor
> + *
> + * @fd:		file descriptor associated with DMABUF (set by driver)
> + * @mem_offset:	a "cookie" that is passed to mmap() as offset

I wouldn't mention mmap() here, as that's unrelated to the DMABUF exporter 
API. Maybe something like

"buffer memory offset as returned by VIDIOC_QUERYBUF in struct 
v4l2_buffer::m.offset (for single-plane formats) or v4l2_plane::m.offset (for 
multi-planar formats)"

> + * @flags:	flags for newly created file, currently only O_CLOEXEC is
> + *		supported, refer to manual of open syscall for more details
> + *
> + * Contains data used for exporting a video buffer as DMABUF file
> + *descriptor. Uses the same 'cookie' as mmap() syscall.

The buffer is identified by a 'cookie' returned by VIDIOC_QUERYBUF (identical 
to the cookie used to mmap() the buffer to userspace).

> All reserved fields
> +*must be set to zero. The field reserved0 is expected to become a structure
> + *'type' allowing an alternative layout of the structure content. Therefore
> + * this field should not be used for any other extensions.
> + */
> +struct v4l2_exportbuffer {
> +	__u32		fd;
> +	__u32		reserved0;
> +	__u32		mem_offset;
> +	__u32		flags;
> +	__u32		reserved[12];
> +};

-- 
Regards,

Laurent Pinchart

Re: [RFC 03/13] v4l: vb2-dma-contig: let mmap method to use dma_mmap_coherent call

[Laurent Pinchart]

Hi Tomasz,

Thanks for the patch.

On Tuesday 10 April 2012 15:10:37 Tomasz Stanislawski wrote:
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> Let mmap method to use dma_mmap_coherent call.  This patch depends on DMA
> mapping redesign patches because the usage of dma_mmap_coherent breaks
> dma-contig allocator for architectures other than ARM and AVR.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/media/video/videobuf2-dma-contig.c |   28 +++++++++++++++++++++++--
>  1 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-dma-contig.c
> b/drivers/media/video/videobuf2-dma-contig.c index 6329483..f4df9e2 100644
> --- a/drivers/media/video/videobuf2-dma-contig.c
> +++ b/drivers/media/video/videobuf2-dma-contig.c
> @@ -224,14 +224,38 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned
> long size) static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct
> *vma) {
>  	struct vb2_dc_buf *buf = buf_priv;
> +	int ret;
> 
>  	if (!buf) {
>  		printk(KERN_ERR "No buffer to map\n");
>  		return -EINVAL;
>  	}
> 
> -	return vb2_mmap_pfn_range(vma, buf->dma_addr, buf->size,
> -				  &vb2_common_vm_ops, &buf->handler);

This was the only vb2_mmap_pfn_range() if I'm not mistaken. Should the 
function be removed ?

> +	/*
> +	 * dma_mmap_* uses vm_pgoff as in-buffer offset, but we want to
> +	 * map whole buffer
> +	 */
> +	vma->vm_pgoff = 0;

Is it safe to set vma->vm_pgoff to 0 here behind memory core's back ?

> +	ret = dma_mmap_coherent(buf->dev, vma, buf->vaddr,
> +		buf->dma_addr, buf->size);
> +
> +	if (ret) {
> +		printk(KERN_ERR "Remapping memory failed, error: %d\n", ret);
> +		return ret;
> +	}
> +
> +	vma->vm_flags		|= VM_DONTEXPAND | VM_RESERVED;
> +	vma->vm_private_data	= &buf->handler;
> +	vma->vm_ops		= &vb2_common_vm_ops;
> +
> +	vma->vm_ops->open(vma);
> +
> +	printk(KERN_DEBUG "%s: mapped dma addr 0x%08lx at 0x%08lx, size %ld\n",
> +		__func__, (unsigned long)buf->dma_addr, vma->vm_start,
> +		buf->size);
> +
> +	return 0;
>  }
> 
>  /*********************************************/
-- 
Regards,

Laurent Pinchart

Re: [RFC 02/13] v4l: vb2: add buffer exporting via dmabuf

[Laurent Pinchart]

Hi Tomasz,

Thanks for the patch.

On Tuesday 10 April 2012 15:10:36 Tomasz Stanislawski wrote:
> This patch adds extension to videobuf2-core. It allow to export a mmap
> buffer as a file descriptor.
> 
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/video/videobuf2-core.c |   66 +++++++++++++++++++++++++++++++
>  include/media/videobuf2-core.h       |    2 +
>  2 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-core.c
> b/drivers/media/video/videobuf2-core.c index b37feea..ff902aa 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -1710,6 +1710,72 @@ static int __find_plane_by_offset(struct vb2_queue
> *q, unsigned long off, }
> 
>  /**
> + * vb2_expbuf() - Export a buffer as a file descriptor
> + * @q:		videobuf2 queue
> + * @b:		export buffer structure passed from userspace to vidioc_expbuf
> + *		handler in driver

This should be @eb.

> + *
> + *The return values from this function are intended to be directly returned
> + * from vidioc_expbuf handler in driver.
> + */
> +int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb)
> +{
> +	struct vb2_buffer *vb = NULL;
> +	struct vb2_plane *vb_plane;
> +	unsigned int buffer, plane;
> +	int ret;
> +	struct dma_buf *dbuf;
> +
> +	if (q->memory != V4L2_MEMORY_MMAP) {
> +		dprintk(1, "Queue is not currently set up for mmap\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!q->mem_ops->get_dmabuf) {
> +		dprintk(1, "Queue does not support DMA buffer exporting\n");
> +		return -EINVAL;
> +	}
> +
> +	if (eb->flags & ~O_CLOEXEC) {
> +		dprintk(1, "Queue supports only O_CLOEXEC flag\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Find the plane corresponding to the offset passed by userspace.
> +	 */
> +	ret = __find_plane_by_offset(q, eb->mem_offset, &buffer, &plane);
> +	if (ret) {
> +		dprintk(1, "invalid offset %u\n", eb->mem_offset);
> +		return ret;
> +	}
> +
> +	vb = q->bufs[buffer];
> +	vb_plane = &vb->planes[plane];
> +
> +	dbuf = call_memop(q, get_dmabuf, vb_plane->mem_priv);
> +	if (IS_ERR_OR_NULL(dbuf)) {
> +		dprintk(1, "Failed to export buffer %d, plane %d\n",
> +			buffer, plane);
> +		return -EINVAL;
> +	}
> +
> +	ret = dma_buf_fd(dbuf, eb->flags);
> +	if (ret < 0) {
> +		dprintk(3, "buffer %d, plane %d failed to export (%d)\n",
> +			buffer, plane, ret);
> +		return ret;
> +	}
> +
> +	dprintk(3, "buffer %d, plane %d exported as %d descriptor\n",
> +		buffer, plane, ret);
> +	eb->fd = ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vb2_expbuf);
> +
> +/**
>   * vb2_mmap() - map video buffers into application address space
>   * @q:		videobuf2 queue
>   * @vma:	vma passed to the mmap file operation handler in the driver
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 244165a..3bd4225 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -81,6 +81,7 @@ struct vb2_fileio_data;
>  struct vb2_mem_ops {
>  	void		*(*alloc)(void *alloc_ctx, unsigned long size);
>  	void		(*put)(void *buf_priv);
> +	struct dma_buf *(*get_dmabuf)(void *buf_priv);
> 
>  	void		*(*get_userptr)(void *alloc_ctx, unsigned long vaddr,
>  					unsigned long size, int write);
> @@ -354,6 +355,7 @@ int vb2_queue_init(struct vb2_queue *q);
>  void vb2_queue_release(struct vb2_queue *q);
> 
>  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b);
> +int vb2_expbuf(struct vb2_queue *q, struct v4l2_exportbuffer *eb);
>  int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool
> nonblocking);
> 
>  int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type);

-- 
Regards,

Laurent Pinchart

Re: [RFC 04/13] v4l: vb2-dma-contig: add setup of sglist for MMAP buffers

[Laurent Pinchart]

Hi Tomasz,

Thanks for the patch.

On Tuesday 10 April 2012 15:10:38 Tomasz Stanislawski wrote:
> This patch adds the setup of sglist list for MMAP buffers.
> It is needed for buffer exporting via DMABUF mechanism.
> 
> This patch depends on dma_get_pages extension to DMA api.
> 
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/video/videobuf2-dma-contig.c |   51 ++++++++++++++++++++++++-
>  1 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-dma-contig.c
> b/drivers/media/video/videobuf2-dma-contig.c index f4df9e2..0cdcd2b 100644
> --- a/drivers/media/video/videobuf2-dma-contig.c
> +++ b/drivers/media/video/videobuf2-dma-contig.c

[snip]

> @@ -197,6 +199,9 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long
> size) {
>  	struct device *dev = alloc_ctx;
>  	struct vb2_dc_buf *buf;
> +	int ret = -ENOMEM;
> +	int n_pages;
> +	struct page **pages = NULL;
> 
>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>  	if (!buf)
> @@ -205,10 +210,41 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned
> long size) buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr,
> GFP_KERNEL); if (!buf->vaddr) {
>  		dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
> -		kfree(buf);
> -		return ERR_PTR(-ENOMEM);
> +		goto fail_buf;
> +	}
> +
> +	WARN_ON((unsigned long)buf->vaddr & ~PAGE_MASK);
> +	WARN_ON(buf->dma_addr & ~PAGE_MASK);
> +
> +	n_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> +
> +	pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL);
> +	if (!pages) {
> +		dev_err(dev, "failed to alloc page table\n");
> +		goto fail_dma;
> +	}
> +
> +	ret = dma_get_pages(dev, buf->vaddr, buf->dma_addr, pages, n_pages);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get buffer pages from DMA API\n");
> +		goto fail_pages;
> +	}
> +	if (ret != n_pages) {
> +		ret = -EFAULT;
> +		dev_err(dev, "failed to get all pages from DMA API\n");
> +		goto fail_pages;
>  	}
> 
> +	buf->sgt_base = vb2_dc_pages_to_sgt(pages, n_pages, 0, 0);
> +	if (IS_ERR(buf->sgt_base)) {
> +		ret = PTR_ERR(buf->sgt_base);
> +		dev_err(dev, "failed to prepare sg table\n");
> +		goto fail_pages;
> +	}

I still (at least partially) share Daniel's opinion regarding dma_get_pages(), 
As I stated before, I think what we need here would be either

-  a DMA API call that maps the memory to the importer device instead of
dma_get_pages() + vb2_dc_pages_to_sgt(). The call would take a DMA memory
"cookie" (see the "Minutes from V4L2 update call" mail thread) and a pointer
to the importer device.

- a DMA API call to retrieve a scatter list suitable to be passed to
dma_map_sg(). This would be similar to dma_get_pages() +
vb2_dc_pages_to_sgt().

(And we still have to figure out whether the mapping call should be in the 
exporter or importer, which might have an influence here).

> +
> +	/* pages are no longer needed */
> +	kfree(pages);
> +
>  	buf->dev = dev;
>  	buf->size = size;
> 
> @@ -219,6 +255,17 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned
> long size) atomic_inc(&buf->refcount);
> 
>  	return buf;
> +
> +fail_pages:
> +	kfree(pages);

As kfree(NULL) is legal, you can remove the fail_pages label and move the 
kfree() call just before dma_free_coherent().

> +
> +fail_dma:
> +	dma_free_coherent(dev, size, buf->vaddr, buf->dma_addr);
> +
> +fail_buf:
> +	kfree(buf);
> +
> +	return ERR_PTR(ret);
>  }
> 
>  static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)

-- 
Regards,

Laurent Pinchart

Re: [RFC 04/13] v4l: vb2-dma-contig: add setup of sglist for MMAP buffers

[Laurent Pinchart]

Hi Tomasz,

Thanks for the patch.

On Tuesday 10 April 2012 15:10:38 Tomasz Stanislawski wrote:
> This patch adds the setup of sglist list for MMAP buffers.
> It is needed for buffer exporting via DMABUF mechanism.
> 
> This patch depends on dma_get_pages extension to DMA api.
> 
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/video/videobuf2-dma-contig.c |   51 ++++++++++++++++++++++++-
>  1 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-dma-contig.c
> b/drivers/media/video/videobuf2-dma-contig.c index f4df9e2..0cdcd2b 100644
> --- a/drivers/media/video/videobuf2-dma-contig.c
> +++ b/drivers/media/video/videobuf2-dma-contig.c

[snip]

> @@ -197,6 +199,9 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long
> size) {
>  	struct device *dev = alloc_ctx;
>  	struct vb2_dc_buf *buf;
> +	int ret = -ENOMEM;
> +	int n_pages;
> +	struct page **pages = NULL;
> 
>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>  	if (!buf)
> @@ -205,10 +210,41 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned
> long size) buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr,
> GFP_KERNEL); if (!buf->vaddr) {
>  		dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
> -		kfree(buf);
> -		return ERR_PTR(-ENOMEM);
> +		goto fail_buf;
> +	}
> +
> +	WARN_ON((unsigned long)buf->vaddr & ~PAGE_MASK);
> +	WARN_ON(buf->dma_addr & ~PAGE_MASK);
> +
> +	n_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> +
> +	pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL);
> +	if (!pages) {
> +		dev_err(dev, "failed to alloc page table\n");
> +		goto fail_dma;
> +	}
> +
> +	ret = dma_get_pages(dev, buf->vaddr, buf->dma_addr, pages, n_pages);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get buffer pages from DMA API\n");
> +		goto fail_pages;
> +	}
> +	if (ret != n_pages) {
> +		ret = -EFAULT;
> +		dev_err(dev, "failed to get all pages from DMA API\n");
> +		goto fail_pages;
>  	}
> 
> +	buf->sgt_base = vb2_dc_pages_to_sgt(pages, n_pages, 0, 0);
> +	if (IS_ERR(buf->sgt_base)) {
> +		ret = PTR_ERR(buf->sgt_base);
> +		dev_err(dev, "failed to prepare sg table\n");
> +		goto fail_pages;
> +	}

I still (at least partially) share Daniel's opinion regarding dma_get_pages(), 
As I stated before, I think what we need here would be either

-  a DMA API call that maps the memory to the importer device instead of
dma_get_pages() + vb2_dc_pages_to_sgt(). The call would take a DMA memory
"cookie" (see the "Minutes from V4L2 update call" mail thread) and a pointer
to the importer device.

- a DMA API call to retrieve a scatter list suitable to be passed to
dma_map_sg(). This would be similar to dma_get_pages() +
vb2_dc_pages_to_sgt().

(And we still have to figure out whether the mapping call should be in the 
exporter or importer, which might have an influence here).

> +
> +	/* pages are no longer needed */
> +	kfree(pages);
> +
>  	buf->dev = dev;
>  	buf->size = size;
> 
> @@ -219,6 +255,17 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned
> long size) atomic_inc(&buf->refcount);
> 
>  	return buf;
> +
> +fail_pages:
> +	kfree(pages);

As kfree(NULL) is legal, you can remove the fail_pages label and move the 
kfree() call just before dma_free_coherent().

> +
> +fail_dma:
> +	dma_free_coherent(dev, size, buf->vaddr, buf->dma_addr);
> +
> +fail_buf:
> +	kfree(buf);
> +
> +	return ERR_PTR(ret);
>  }
> 
>  static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma)

-- 
Regards,

Laurent Pinchart

Re: [RFC 05/13] v4l: vb2-dma-contig: add support for DMABUF exporting

[Laurent Pinchart]

Hi Tomasz,

Thanks for the patch.

On Tuesday 10 April 2012 15:10:39 Tomasz Stanislawski wrote:
> This patch adds support for exporting a dma-contig buffer using
> DMABUF interface.
> 
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/video/videobuf2-dma-contig.c |  128 +++++++++++++++++++++++++
>  1 files changed, 128 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf2-dma-contig.c
> b/drivers/media/video/videobuf2-dma-contig.c index 0cdcd2b..e1ad47e 100644
> --- a/drivers/media/video/videobuf2-dma-contig.c
> +++ b/drivers/media/video/videobuf2-dma-contig.c
> @@ -31,6 +31,7 @@ struct vb2_dc_buf {
>  	/* MMAP related */
>  	struct vb2_vmarea_handler	handler;
>  	atomic_t			refcount;
> +	struct dma_buf			*dma_buf;
>  	struct sg_table			*sgt_base;
> 
>  	/* USERPTR related */
> @@ -190,6 +191,8 @@ static void vb2_dc_put(void *buf_priv)
>  	if (!atomic_dec_and_test(&buf->refcount))
>  		return;
> 
> +	if (buf->dma_buf)
> +		dma_buf_put(buf->dma_buf);
>  	vb2_dc_release_sgtable(buf->sgt_base);
>  	dma_free_coherent(buf->dev, buf->size, buf->vaddr, buf->dma_addr);
>  	kfree(buf);
> @@ -306,6 +309,130 @@ static int vb2_dc_mmap(void *buf_priv, struct
> vm_area_struct *vma) }
> 
>  /*********************************************/
> +/*         DMABUF ops for exporters          */
> +/*********************************************/
> +
> +struct vb2_dc_attachment {
> +	struct sg_table sgt;
> +	enum dma_data_direction dir;
> +};
> +
> +static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device
> *dev,
> +	struct dma_buf_attachment *dbuf_attach)
> +{
> +	/* nothing to be done */
> +	return 0;
> +}
> +
> +static void vb2_dc_dmabuf_ops_detach(struct dma_buf *dbuf,
> +	struct dma_buf_attachment *db_attach)
> +{
> +	struct vb2_dc_attachment *attach = db_attach->priv;
> +	struct sg_table *sgt;
> +
> +	if (!attach)
> +		return;
> +
> +	sgt = &attach->sgt;
> +
> +	dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->nents, attach->dir);
> +	sg_free_table(sgt);
> +	kfree(attach);
> +	db_attach->priv = NULL;
> +}
> +
> +static struct sg_table *vb2_dc_dmabuf_ops_map(
> +	struct dma_buf_attachment *db_attach, enum dma_data_direction dir)
> +{
> +	struct dma_buf *dbuf = db_attach->dmabuf;
> +	struct vb2_dc_buf *buf = dbuf->priv;
> +	struct vb2_dc_attachment *attach = db_attach->priv;
> +	struct sg_table *sgt;
> +	struct scatterlist *rd, *wr;
> +	int i, ret;

You can make i an unsigned int :-)

> +
> +	/* return previously mapped sg table */
> +	if (attach)
> +		return &attach->sgt;

This effectively keeps the mapping around as long as the attachment exists. We 
don't try to swap out buffers in V4L2 as is done in DRM at the moment, so it 
might not be too much of an issue, but the behaviour of the implementation 
will change if we later decide to map/unmap the buffers in the map/unmap 
handlers. Do you think that could be a problem ?

> +
> +	attach = kzalloc(sizeof *attach, GFP_KERNEL);
> +	if (!attach)
> +		return ERR_PTR(-ENOMEM);

Why don't you allocate the vb2_dc_attachment here instead of 
vb2_dc_dmabuf_ops_attach() ?

> +	sgt = &attach->sgt;
> +	attach->dir = dir;
> +
> +	/* copying the buf->base_sgt to attachment */

I would add an explanation regarding why you need to copy the SG list. 
Something like.

"Copy the buf->base_sgt scatter list to the attachment, as we can't map the 
same scatter list to multiple devices at the same time."

> +	ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL);
> +	if (ret) {
> +		kfree(attach);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	rd = buf->sgt_base->sgl;
> +	wr = sgt->sgl;
> +	for (i = 0; i < sgt->orig_nents; ++i) {
> +		sg_set_page(wr, sg_page(rd), rd->length, rd->offset);
> +		rd = sg_next(rd);
> +		wr = sg_next(wr);
> +	}
>
> +	/* mapping new sglist to the client */
> +	ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dir);
> +	if (ret <= 0) {
> +		printk(KERN_ERR "failed to map scatterlist\n");
> +		sg_free_table(sgt);
> +		kfree(attach);
> +		return ERR_PTR(-EIO);
> +	}
> +
> +	db_attach->priv = attach;
> +
> +	return sgt;
> +}
> +
> +static void vb2_dc_dmabuf_ops_unmap(struct dma_buf_attachment *db_attach,
> +	struct sg_table *sgt, enum dma_data_direction dir)
> +{
> +	/* nothing to be done here */
> +}
> +
> +static void vb2_dc_dmabuf_ops_release(struct dma_buf *dbuf)
> +{
> +	/* drop reference obtained in vb2_dc_get_dmabuf */
> +	vb2_dc_put(dbuf->priv);

Shouldn't you set vb2_dc_buf::dma_buf to NULL here ? Otherwise the next 
vb2_dc_get_dmabuf() call will return a DMABUF object that has been freed.

> +}
> +
> +static struct dma_buf_ops vb2_dc_dmabuf_ops = {
> +	.attach = vb2_dc_dmabuf_ops_attach,
> +	.detach = vb2_dc_dmabuf_ops_detach,
> +	.map_dma_buf = vb2_dc_dmabuf_ops_map,
> +	.unmap_dma_buf = vb2_dc_dmabuf_ops_unmap,
> +	.release = vb2_dc_dmabuf_ops_release,
> +};
> +
> +static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv)
> +{
> +	struct vb2_dc_buf *buf = buf_priv;
> +	struct dma_buf *dbuf;
> +
> +	if (buf->dma_buf)
> +		return buf->dma_buf;

Can't there be a race condition here if the user closes the DMABUF file handle 
before vb2 core calls dma_buf_fd() ?

> +	/* dmabuf keeps reference to vb2 buffer */
> +	atomic_inc(&buf->refcount);
> +	dbuf = dma_buf_export(buf, &vb2_dc_dmabuf_ops, buf->size, 0);
> +	if (IS_ERR(dbuf)) {
> +		atomic_dec(&buf->refcount);
> +		return NULL;
> +	}
> +
> +	buf->dma_buf = dbuf;
> +
> +	return dbuf;
> +}
> +
> +/*********************************************/
>  /*       callbacks for USERPTR buffers       */
>  /*********************************************/
> 
> @@ -615,6 +742,7 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx,
> struct dma_buf *dbuf, const struct vb2_mem_ops vb2_dma_contig_memops = {
>  	.alloc		= vb2_dc_alloc,
>  	.put		= vb2_dc_put,
> +	.get_dmabuf	= vb2_dc_get_dmabuf,
>  	.cookie		= vb2_dc_cookie,
>  	.vaddr		= vb2_dc_vaddr,
>  	.mmap		= vb2_dc_mmap,
-- 
Regards,

Laurent Pinchart

Re: [RFC 05/13] v4l: vb2-dma-contig: add support for DMABUF exporting

[Tomasz Stanislawski]

Hi Laurent,
Thank you for your review.
Please refer to the comments below.

On 04/17/2012 04:08 PM, Laurent Pinchart wrote:
> Hi Tomasz,
> 
> Thanks for the patch.
> 
> On Tuesday 10 April 2012 15:10:39 Tomasz Stanislawski wrote:
>> This patch adds support for exporting a dma-contig buffer using
>> DMABUF interface.
>>
>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---

[snip]

>> +static struct sg_table *vb2_dc_dmabuf_ops_map(
>> +	struct dma_buf_attachment *db_attach, enum dma_data_direction dir)
>> +{
>> +	struct dma_buf *dbuf = db_attach->dmabuf;
>> +	struct vb2_dc_buf *buf = dbuf->priv;
>> +	struct vb2_dc_attachment *attach = db_attach->priv;
>> +	struct sg_table *sgt;
>> +	struct scatterlist *rd, *wr;
>> +	int i, ret;
> 
> You can make i an unsigned int :-)
> 

Right.. splitting declaration may be also a good idea :)

>> +
>> +	/* return previously mapped sg table */
>> +	if (attach)
>> +		return &attach->sgt;
> 
> This effectively keeps the mapping around as long as the attachment exists. We 
> don't try to swap out buffers in V4L2 as is done in DRM at the moment, so it 
> might not be too much of an issue, but the behaviour of the implementation 
> will change if we later decide to map/unmap the buffers in the map/unmap 
> handlers. Do you think that could be a problem ?

I don't that it is a problem. If an importer calls dma_map_sg then
caching sgt on an exporter side reduces a cost of an allocating
and an initialization of sgt.

> 
>> +
>> +	attach = kzalloc(sizeof *attach, GFP_KERNEL);
>> +	if (!attach)
>> +		return ERR_PTR(-ENOMEM);
> 
> Why don't you allocate the vb2_dc_attachment here instead of 
> vb2_dc_dmabuf_ops_attach() ?
> 

Good point.
The attachment could be allocated at vb2_dc_attachment but all its
fields would be uninitialized. I mean an empty sgt and an undefined
dma direction. I decided to allocate the attachment in vb2_dc_dmabuf_ops_map
because only than all information needed to create a valid attachment
object are available.

The other solution might be the allocation at vb2_dc_attachment. The field dir
would be set to DMA_NONE. If this filed is equal to DMA_NONE at
vb2_dc_dmabuf_ops_map then sgt is allocated and mapped and direction field is
updated. If value is not DMA_NONE then the sgt is reused.

Do you think that it is a good idea?

>> +	sgt = &attach->sgt;
>> +	attach->dir = dir;
>> +
>> +	/* copying the buf->base_sgt to attachment */
> 
> I would add an explanation regarding why you need to copy the SG list. 
> Something like.
> 
> "Copy the buf->base_sgt scatter list to the attachment, as we can't map the 
> same scatter list to multiple devices at the same time."
> 

ok

>> +	ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL);
>> +	if (ret) {
>> +		kfree(attach);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	rd = buf->sgt_base->sgl;
>> +	wr = sgt->sgl;
>> +	for (i = 0; i < sgt->orig_nents; ++i) {
>> +		sg_set_page(wr, sg_page(rd), rd->length, rd->offset);
>> +		rd = sg_next(rd);
>> +		wr = sg_next(wr);
>> +	}
>>
>> +	/* mapping new sglist to the client */
>> +	ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dir);
>> +	if (ret <= 0) {
>> +		printk(KERN_ERR "failed to map scatterlist\n");
>> +		sg_free_table(sgt);
>> +		kfree(attach);
>> +		return ERR_PTR(-EIO);
>> +	}
>> +
>> +	db_attach->priv = attach;
>> +
>> +	return sgt;
>> +}
>> +
>> +static void vb2_dc_dmabuf_ops_unmap(struct dma_buf_attachment *db_attach,
>> +	struct sg_table *sgt, enum dma_data_direction dir)
>> +{
>> +	/* nothing to be done here */
>> +}
>> +
>> +static void vb2_dc_dmabuf_ops_release(struct dma_buf *dbuf)
>> +{
>> +	/* drop reference obtained in vb2_dc_get_dmabuf */
>> +	vb2_dc_put(dbuf->priv);
> 
> Shouldn't you set vb2_dc_buf::dma_buf to NULL here ? Otherwise the next 
> vb2_dc_get_dmabuf() call will return a DMABUF object that has been freed.
> 

No.

The buffer object is destroyed at vb2_dc_put when reference count drops to 0.
It happens could happen after only REQBUF(count=0) or on last close().
The DMABUF object is created only for MMAP buffers. The DMABUF object is based
only on results of dma_alloc_coherent and dma_get_pages (or its future equivalent).
Therefore the DMABUF object is valid as long as the buffer is valid.

Notice that dmabuf object could be created in vb2_dc_alloc. I moved
it to vb2_dc_get_dmabuf to avoid a creation of an object that
may not be used.

>> +}
>> +
>> +static struct dma_buf_ops vb2_dc_dmabuf_ops = {
>> +	.attach = vb2_dc_dmabuf_ops_attach,
>> +	.detach = vb2_dc_dmabuf_ops_detach,
>> +	.map_dma_buf = vb2_dc_dmabuf_ops_map,
>> +	.unmap_dma_buf = vb2_dc_dmabuf_ops_unmap,
>> +	.release = vb2_dc_dmabuf_ops_release,
>> +};
>> +
>> +static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv)
>> +{
>> +	struct vb2_dc_buf *buf = buf_priv;
>> +	struct dma_buf *dbuf;
>> +
>> +	if (buf->dma_buf)
>> +		return buf->dma_buf;
> 
> Can't there be a race condition here if the user closes the DMABUF file handle 
> before vb2 core calls dma_buf_fd() ?

The user cannot access the file until it is associated with a file
descriptor. How can the user close it? Could you give me a more
detailed description of this potential race condition?

> 
>> +	/* dmabuf keeps reference to vb2 buffer */
>> +	atomic_inc(&buf->refcount);
>> +	dbuf = dma_buf_export(buf, &vb2_dc_dmabuf_ops, buf->size, 0);
>> +	if (IS_ERR(dbuf)) {
>> +		atomic_dec(&buf->refcount);
>> +		return NULL;
>> +	}
>> +
>> +	buf->dma_buf = dbuf;
>> +
>> +	return dbuf;
>> +}
>> +
>> +/*********************************************/
>>  /*       callbacks for USERPTR buffers       */
>>  /*********************************************/
>>
>> @@ -615,6 +742,7 @@ static void *vb2_dc_attach_dmabuf(void *alloc_ctx,
>> struct dma_buf *dbuf, const struct vb2_mem_ops vb2_dma_contig_memops = {
>>  	.alloc		= vb2_dc_alloc,
>>  	.put		= vb2_dc_put,
>> +	.get_dmabuf	= vb2_dc_get_dmabuf,
>>  	.cookie		= vb2_dc_cookie,
>>  	.vaddr		= vb2_dc_vaddr,
>>  	.mmap		= vb2_dc_mmap,

Regards,
Tomasz Stanislawski

Re: [RFC 05/13] v4l: vb2-dma-contig: add support for DMABUF exporting

[Laurent Pinchart]

Hi Tomasz,

Sorry for the late reply, this one slipped through the cracks.

On Thursday 19 April 2012 12:42:12 Tomasz Stanislawski wrote:
> On 04/17/2012 04:08 PM, Laurent Pinchart wrote:
> > On Tuesday 10 April 2012 15:10:39 Tomasz Stanislawski wrote:
> >> This patch adds support for exporting a dma-contig buffer using
> >> DMABUF interface.
> >> 
> >> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> ---
> 
> [snip]
> 
> >> +static struct sg_table *vb2_dc_dmabuf_ops_map(
> >> +	struct dma_buf_attachment *db_attach, enum dma_data_direction dir)
> >> +{
> >> +	struct dma_buf *dbuf = db_attach->dmabuf;
> >> +	struct vb2_dc_buf *buf = dbuf->priv;
> >> +	struct vb2_dc_attachment *attach = db_attach->priv;
> >> +	struct sg_table *sgt;
> >> +	struct scatterlist *rd, *wr;
> >> +	int i, ret;
> > 
> > You can make i an unsigned int :-)
> 
> Right.. splitting declaration may be also a good idea :)
> 
> >> +
> >> +	/* return previously mapped sg table */
> >> +	if (attach)
> >> +		return &attach->sgt;
> > 
> > This effectively keeps the mapping around as long as the attachment
> > exists. We don't try to swap out buffers in V4L2 as is done in DRM at the
> > moment, so it might not be too much of an issue, but the behaviour of the
> > implementation will change if we later decide to map/unmap the buffers in
> > the map/unmap handlers. Do you think that could be a problem ?
> 
> I don't that it is a problem. If an importer calls dma_map_sg then caching
> sgt on an exporter side reduces a cost of an allocating and an
> initialization of sgt.
> 
> >> +
> >> +	attach = kzalloc(sizeof *attach, GFP_KERNEL);
> >> +	if (!attach)
> >> +		return ERR_PTR(-ENOMEM);
> > 
> > Why don't you allocate the vb2_dc_attachment here instead of
> > vb2_dc_dmabuf_ops_attach() ?
> 
> Good point.
> The attachment could be allocated at vb2_dc_attachment but all its
> fields would be uninitialized. I mean an empty sgt and an undefined
> dma direction. I decided to allocate the attachment in vb2_dc_dmabuf_ops_map
> because only than all information needed to create a valid attachment
> object are available.
> 
> The other solution might be the allocation at vb2_dc_attachment. The field
> dir would be set to DMA_NONE. If this filed is equal to DMA_NONE at
> vb2_dc_dmabuf_ops_map then sgt is allocated and mapped and direction field
> is updated. If value is not DMA_NONE then the sgt is reused.
> 
> Do you think that it is a good idea?

I think I would prefer that. It sounds more logical to allocate the attachment 
in the attach operation handler.

> >> +	sgt = &attach->sgt;
> >> +	attach->dir = dir;
> >> +
> >> +	/* copying the buf->base_sgt to attachment */
> > 
> > I would add an explanation regarding why you need to copy the SG list.
> > Something like.
> > 
> > "Copy the buf->base_sgt scatter list to the attachment, as we can't map
> > the same scatter list to multiple devices at the same time."
> 
> ok
> 
> >> +	ret = sg_alloc_table(sgt, buf->sgt_base->orig_nents, GFP_KERNEL);
> >> +	if (ret) {
> >> +		kfree(attach);
> >> +		return ERR_PTR(-ENOMEM);
> >> +	}
> >> +
> >> +	rd = buf->sgt_base->sgl;
> >> +	wr = sgt->sgl;
> >> +	for (i = 0; i < sgt->orig_nents; ++i) {
> >> +		sg_set_page(wr, sg_page(rd), rd->length, rd->offset);
> >> +		rd = sg_next(rd);
> >> +		wr = sg_next(wr);
> >> +	}
> >> 
> >> +	/* mapping new sglist to the client */
> >> +	ret = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents, dir);
> >> +	if (ret <= 0) {
> >> +		printk(KERN_ERR "failed to map scatterlist\n");
> >> +		sg_free_table(sgt);
> >> +		kfree(attach);
> >> +		return ERR_PTR(-EIO);
> >> +	}
> >> +
> >> +	db_attach->priv = attach;
> >> +
> >> +	return sgt;
> >> +}
> >> +
> >> +static void vb2_dc_dmabuf_ops_unmap(struct dma_buf_attachment
> >> *db_attach,
> >> +	struct sg_table *sgt, enum dma_data_direction dir)
> >> +{
> >> +	/* nothing to be done here */
> >> +}
> >> +
> >> +static void vb2_dc_dmabuf_ops_release(struct dma_buf *dbuf)
> >> +{
> >> +	/* drop reference obtained in vb2_dc_get_dmabuf */
> >> +	vb2_dc_put(dbuf->priv);
> > 
> > Shouldn't you set vb2_dc_buf::dma_buf to NULL here ? Otherwise the next
> > vb2_dc_get_dmabuf() call will return a DMABUF object that has been freed.
> 
> No.
> 
> The buffer object is destroyed at vb2_dc_put when reference count drops to
> 0. It happens could happen after only REQBUF(count=0) or on last close().
> The DMABUF object is created only for MMAP buffers. The DMABUF object is
> based only on results of dma_alloc_coherent and dma_get_pages (or its future
> equivalent). Therefore the DMABUF object is valid as long as the buffer is
> valid.

OK.

> Notice that dmabuf object could be created in vb2_dc_alloc. I moved it to
> vb2_dc_get_dmabuf to avoid a creation of an object that may not be used.
> 
> >> +}
> >> +
> >> +static struct dma_buf_ops vb2_dc_dmabuf_ops = {
> >> +	.attach = vb2_dc_dmabuf_ops_attach,
> >> +	.detach = vb2_dc_dmabuf_ops_detach,
> >> +	.map_dma_buf = vb2_dc_dmabuf_ops_map,
> >> +	.unmap_dma_buf = vb2_dc_dmabuf_ops_unmap,
> >> +	.release = vb2_dc_dmabuf_ops_release,
> >> +};
> >> +
> >> +static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv)
> >> +{
> >> +	struct vb2_dc_buf *buf = buf_priv;
> >> +	struct dma_buf *dbuf;
> >> +
> >> +	if (buf->dma_buf)
> >> +		return buf->dma_buf;
> > 
> > Can't there be a race condition here if the user closes the DMABUF file
> > handle before vb2 core calls dma_buf_fd() ?
> 
> The user cannot access the file until it is associated with a file
> descriptor. How can the user close it? Could you give me a more detailed
> description of this potential race condition?

Let's assume the V4L2 buffer has already been exported once. buf->dma_buf is 
set to a non-NULL value, and the application has an open file handle for the 
buffer. The application then tries to export the buffer a second time. 
vb2_dc_get_dmabuf() gets called, checks buf->dma_buf and returns it as it's 
non-NULL. Right after that, before the vb2 core calls dma_buf_fd() on the 
struct dma_buf, the application closes the file handle to the exported buffer. 
The struct dma_buf object gets freed, as the reference count drops to 0. The 
vb2 core will then try to call dma_buf_fd() on a dma_buf object that has been 
freed.

> >> +	/* dmabuf keeps reference to vb2 buffer */
> >> +	atomic_inc(&buf->refcount);
> >> +	dbuf = dma_buf_export(buf, &vb2_dc_dmabuf_ops, buf->size, 0);
> >> +	if (IS_ERR(dbuf)) {
> >> +		atomic_dec(&buf->refcount);
> >> +		return NULL;
> >> +	}
> >> +
> >> +	buf->dma_buf = dbuf;
> >> +
> >> +	return dbuf;
> >> +}

-- 
Regards,

Laurent Pinchart

Re: [RFC 05/13] v4l: vb2-dma-contig: add support for DMABUF exporting

[Tomasz Stanislawski]

Hi Laurent,

Sorry for the late reply.
Thank you very much for noticing the issue.

>>>> +static struct dma_buf *vb2_dc_get_dmabuf(void *buf_priv)
>>>> +{
>>>> +	struct vb2_dc_buf *buf = buf_priv;
>>>> +	struct dma_buf *dbuf;
>>>> +
>>>> +	if (buf->dma_buf)
>>>> +		return buf->dma_buf;
>>>
>>> Can't there be a race condition here if the user closes the DMABUF file
>>> handle before vb2 core calls dma_buf_fd() ?
>>
>> The user cannot access the file until it is associated with a file
>> descriptor. How can the user close it? Could you give me a more detailed
>> description of this potential race condition?
> 
> Let's assume the V4L2 buffer has already been exported once. buf->dma_buf is 
> set to a non-NULL value, and the application has an open file handle for the 
> buffer. The application then tries to export the buffer a second time. 
> vb2_dc_get_dmabuf() gets called, checks buf->dma_buf and returns it as it's 
> non-NULL. Right after that, before the vb2 core calls dma_buf_fd() on the 
> struct dma_buf, the application closes the file handle to the exported buffer. 
> The struct dma_buf object gets freed, as the reference count drops to 0.

I am not sure if reference counter drops to 0 in this case. Notice that after
first dma_buf_export the dma_buf's refcnt is 1, after first dma_buf_fd it goes
to 2. After closing a file handle the refcnt drops back to 1 so the file
(and dma_buf) is not released. Therefore IMO there no dangling pointer issue.

However it looks that there is a circular reference between vb2_dc_buf and dma_buf.
vb2_dc_buf::dma_buf is pointing to dma_buf with reference taken at dma_buf_export.
On the other hand the dma_buf->priv is pointing to vb2_dc_buf with reference
taken at atomic_inc(&buf->refcount) in vb2_dc_get_dmabuf.

The circular reference leads into resource leakage.
The problem could be fixed by dropping caching of dma_buf pointer.
The new dma_buf would be created and exported at every export event.
The dma_buf_put would be called in vb2_expbuf just after
successful dma_buf_fd.

Do you have any ideas how I could deal with resource leakage/dangling problems
without creating a new dma_buf instance at every export event?

Regards,
Tomasz Stanislawski


> vb2 core will then try to call dma_buf_fd() on a dma_buf object that has been 
> freed.
> 
>>>> +	/* dmabuf keeps reference to vb2 buffer */
>>>> +	atomic_inc(&buf->refcount);
>>>> +	dbuf = dma_buf_export(buf, &vb2_dc_dmabuf_ops, buf->size, 0);
>>>> +	if (IS_ERR(dbuf)) {
>>>> +		atomic_dec(&buf->refcount);
>>>> +		return NULL;
>>>> +	}
>>>> +
>>>> +	buf->dma_buf = dbuf;
>>>> +
>>>> +	return dbuf;
>>>> +}
>