Hi Tomasz,

Thanks for the patch.

On Tuesday 13 March 2012 11:16:59 Tomasz Stanislawski wrote:

This patch contains fixes to DMABUF support in vb2-core.

• fixes number of arguments of call_memop macro
• fixes setup of plane length
• fixes handling of error pointers

Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com

Tested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

I suppose this will be squashed with Sumit's "[RFCv1 2/4] v4l:vb2: add support for shared buffer (dma_buf)" patch.

---

drivers/media/video/videobuf2-core.c | 24 +++++++++++------------- include/media/videobuf2-core.h | 6 +++--- 2 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c index 951cb56..e7df560 100644 --- a/drivers/media/video/videobuf2-core.c +++ b/drivers/media/video/videobuf2-core.c @@ -118,7 +118,7 @@ static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb) void *mem_priv = vb->planes[plane].mem_priv;

if (mem_priv) {

• 	call_memop(q, plane, detach_dmabuf, mem_priv);
  

• 	call_memop(q, detach_dmabuf, mem_priv);
  dma_buf_put(vb->planes[plane].dbuf);
  vb->planes[plane].dbuf = NULL;
  vb->planes[plane].mem_priv = NULL;
  

@@ -905,6 +905,8 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b, } if (b->memory == V4L2_MEMORY_DMABUF) { for (plane = 0; plane < vb->num_planes; ++plane) {

• 		v4l2_planes[plane].bytesused =
  

• 			b->m.planes[plane].bytesused;
  	v4l2_planes[plane].m.fd = b->m.planes[plane].m.fd;
  }
  

  }

@@ -1052,17 +1054,13 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) if (IS_ERR_OR_NULL(dbuf)) { dprintk(1, "qbuf: invalid dmabuf fd for " "plane %d\n", plane);

• 	ret = PTR_ERR(dbuf);
  

• 	ret = -EINVAL;
  goto err;
  

  }

• /* this doesn't get filled in until __fill_vb2_buffer(),
  

•  * since it isn't known until after dma_buf_get()..
  

•  */
  

• planes[plane].length = dbuf->size;
  

• /* Skip the plane if already verified */ if (dbuf == vb->planes[plane].dbuf) {

• 	planes[plane].length = dbuf->size;
  dma_buf_put(dbuf);
  continue;
  

  }

@@ -1072,7 +1070,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b)

/* Release previously acquired memory if present */
if (vb->planes[plane].mem_priv) {

• 	call_memop(q, plane, detach_dmabuf,
  

• 	call_memop(q, detach_dmabuf,
  	vb->planes[plane].mem_priv);
  dma_buf_put(vb->planes[plane].dbuf);
  

  }

@@ -1080,8 +1078,8 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) vb->planes[plane].mem_priv = NULL;

/* Acquire each plane's memory */

• mem_priv = q->mem_ops->attach_dmabuf(
  

• 		q->alloc_ctx[plane], dbuf);
  

• mem_priv = call_memop(q, attach_dmabuf, q->alloc_ctx[plane],
  

• 	dbuf, q->plane_sizes[plane], write);
  

  if (IS_ERR(mem_priv)) { dprintk(1, "qbuf: failed acquiring dmabuf " "memory for plane %d\n", plane);

@@ -1089,6 +1087,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) goto err; }

• planes[plane].length = dbuf->size;
  

  vb->planes[plane].dbuf = dbuf; vb->planes[plane].mem_priv = mem_priv; }

@@ -1098,8 +1097,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) * the buffer(s).. */ for (plane = 0; plane < vb->num_planes; ++plane) {

• ret = q->mem_ops->map_dmabuf(
  

• 		vb->planes[plane].mem_priv, write);
  

• ret = call_memop(q, map_dmabuf, vb->planes[plane].mem_priv);
  

  if (ret) { dprintk(1, "qbuf: failed mapping dmabuf " "memory for plane %d\n", plane);

@@ -1527,7 +1525,7 @@ int vb2_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking) */ if (q->memory == V4L2_MEMORY_DMABUF) for (plane = 0; plane < vb->num_planes; ++plane)

• 	call_memop(q, plane, unmap_dmabuf,
  

• 	call_memop(q, unmap_dmabuf,
  	vb->planes[plane].mem_priv);
  
  

  switch (vb->state) {

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index d8b8171..412c6a4 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -88,10 +88,10 @@ struct vb2_mem_ops { * in the vb2 core, and vb2_mem_ops really just need to get/put the * sglist (and make sure that the sglist fits it's needs..) */

• void *(*attach_dmabuf)(void *alloc_ctx,

• 			  struct dma_buf *dbuf);
  

• void *(*attach_dmabuf)(void *alloc_ctx, struct dma_buf *dbuf,

• 		unsigned long size, int write);
  

  void (*detach_dmabuf)(void *buf_priv);

• int (*map_dmabuf)(void *buf_priv, int write);

• int (*map_dmabuf)(void *buf_priv); void (*unmap_dmabuf)(void *buf_priv);

  void *(*vaddr)(void *buf_priv);

Group functions by buffer type.

Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/media/video/videobuf2-dma-contig.c | 94 ++++++++++++++++----------- 1 files changed, 56 insertions(+), 38 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index 5207eb1..6bc4651 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -24,14 +24,58 @@ struct vb2_dc_conf { struct vb2_dc_buf { struct vb2_dc_conf *conf; void *vaddr; - dma_addr_t dma_addr; unsigned long size; - struct vm_area_struct *vma; - atomic_t refcount; + dma_addr_t dma_addr; + + /* MMAP related */ struct vb2_vmarea_handler handler; + atomic_t refcount; + + /* USERPTR related */ + struct vm_area_struct *vma; };

-static void vb2_dc_put(void *buf_priv); +/*********************************************/ +/* callbacks for all buffers */ +/*********************************************/ + +static void *vb2_dc_cookie(void *buf_priv) +{ + struct vb2_dc_buf *buf = buf_priv; + + return &buf->dma_addr; +} + +static void *vb2_dc_vaddr(void *buf_priv) +{ + struct vb2_dc_buf *buf = buf_priv; + if (!buf) + return 0; + + return buf->vaddr; +} + +static unsigned int vb2_dc_num_users(void *buf_priv) +{ + struct vb2_dc_buf *buf = buf_priv; + + return atomic_read(&buf->refcount); +} + +/*********************************************/ +/* callbacks for MMAP buffers */ +/*********************************************/ + +static void vb2_dc_put(void *buf_priv) +{ + struct vb2_dc_buf *buf = buf_priv; + + if (atomic_dec_and_test(&buf->refcount)) { + dma_free_coherent(buf->conf->dev, buf->size, buf->vaddr, + buf->dma_addr); + kfree(buf); + } +}

static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) { @@ -63,40 +107,6 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) return buf; }

-static void vb2_dc_put(void *buf_priv) -{ - struct vb2_dc_buf *buf = buf_priv; - - if (atomic_dec_and_test(&buf->refcount)) { - dma_free_coherent(buf->conf->dev, buf->size, buf->vaddr, - buf->dma_addr); - kfree(buf); - } -} - -static void *vb2_dc_cookie(void *buf_priv) -{ - struct vb2_dc_buf *buf = buf_priv; - - return &buf->dma_addr; -} - -static void *vb2_dc_vaddr(void *buf_priv) -{ - struct vb2_dc_buf *buf = buf_priv; - if (!buf) - return 0; - - return buf->vaddr; -} - -static unsigned int vb2_dc_num_users(void *buf_priv) -{ - struct vb2_dc_buf *buf = buf_priv; - - return atomic_read(&buf->refcount); -} - static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma) { struct vb2_dc_buf *buf = buf_priv; @@ -110,6 +120,10 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma) &vb2_common_vm_ops, &buf->handler); }

+/*********************************************/ +/* callbacks for USERPTR buffers */ +/*********************************************/ + static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr, unsigned long size, int write) { @@ -148,6 +162,10 @@ static void vb2_dc_put_userptr(void *mem_priv) kfree(buf); }

+/*********************************************/ +/* DMA CONTIG exported functions */ +/*********************************************/ + const struct vb2_mem_ops vb2_dma_contig_memops = { .alloc = vb2_dc_alloc, .put = vb2_dc_put,

From: Tomasz Stanislawski t.stanislaws@samsung.com

This patch combines updates and fixes to dma-contig allocator. Moreover the allocator code was refactored. The most important changes are: - functions were reordered - move compression of scatterlist to separete function - add support for multichunk but contiguous scatterlists - simplified implementation of vb2-dma-contig context structure - let mmap method to use dma_mmap_writecombine - add support for scatterlist in userptr mode

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com [mmap method] Signed-off-by: Andrzej Pietrasiewicz andrzej.p@samsung.com [scatterlist in userptr mode] Signed-off-by: Kamil Debski k.debski@samsung.com [bugfixing] Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com [core refactoring, helper functions] Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- drivers/media/video/videobuf2-dma-contig.c | 400 +++++++++++++++++++++++++--- 1 files changed, 365 insertions(+), 35 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index c898e6f..9965465 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -10,9 +10,12 @@ * the Free Software Foundation. */

+#include <linux/dma-buf.h> +#include <linux/dma-mapping.h> #include <linux/module.h> +#include <linux/scatterlist.h> +#include <linux/sched.h> #include <linux/slab.h> -#include <linux/dma-mapping.h>

#include <media/videobuf2-core.h> #include <media/videobuf2-memops.h> @@ -22,16 +25,115 @@ struct vb2_dc_buf { void *vaddr; unsigned long size; dma_addr_t dma_addr; + struct sg_table *dma_sgt; + enum dma_data_direction dma_dir;

/* MMAP related */ struct vb2_vmarea_handler handler; atomic_t refcount; + struct sg_table *sgt_base;

/* USERPTR related */ struct vm_area_struct *vma; };

/*********************************************/ +/* scatterlist table functions */ +/*********************************************/ + +static struct sg_table *vb2_dc_pages_to_sgt(struct page **pages, + unsigned long n_pages, size_t offset, size_t offset2) +{ + struct sg_table *sgt; + int i, j; /* loop counters */ + int cur_page, chunks; + int ret; + struct scatterlist *s; + + sgt = kzalloc(sizeof *sgt, GFP_KERNEL); + if (!sgt) + return ERR_PTR(-ENOMEM); + + /* compute number of chunks */ + chunks = 1; + for (i = 1; i < n_pages; ++i) + if (pages[i] != pages[i - 1] + 1) + ++chunks; + + ret = sg_alloc_table(sgt, chunks, GFP_KERNEL); + if (ret) { + kfree(sgt); + return ERR_PTR(-ENOMEM); + } + + /* merging chunks and putting them into the scatterlist */ + cur_page = 0; + for_each_sg(sgt->sgl, s, sgt->orig_nents, i) { + size_t size = PAGE_SIZE; + + for (j = cur_page + 1; j < n_pages; ++j) { + if (pages[j] != pages[j - 1] + 1) + break; + size += PAGE_SIZE; + } + + /* cut offset if chunk starts at the first page */ + if (cur_page == 0) + size -= offset; + /* cut offset2 if chunk ends at the last page */ + if (j == n_pages) + size -= offset2; + + sg_set_page(s, pages[cur_page], size, offset); + offset = 0; + cur_page = j; + } + + return sgt; +} + +static void vb2_dc_release_sgtable(struct sg_table *sgt) +{ + sg_free_table(sgt); + kfree(sgt); +} + +static void vb2_dc_put_sgtable(struct sg_table *sgt, int dirty) +{ + struct scatterlist *s; + int i, j; + + for_each_sg(sgt->sgl, s, sgt->nents, i) { + struct page *page = sg_page(s); + int n_pages = PAGE_ALIGN(s->offset + s->length) >> PAGE_SHIFT; + + for (j = 0; j < n_pages; ++j, ++page) { + if (dirty) + set_page_dirty_lock(page); + put_page(page); + } + } + + vb2_dc_release_sgtable(sgt); +} + +static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt) +{ + struct scatterlist *s; + dma_addr_t expected = sg_dma_address(sgt->sgl); + int i; + unsigned long size = 0; + + for_each_sg(sgt->sgl, s, sgt->nents, i) { + if (sg_dma_address(s) != expected) + break; + expected = sg_dma_address(s) + sg_dma_len(s); + size += sg_dma_len(s); + } + return size; +} + +/*********************************************/ /* callbacks for all buffers */ /*********************************************/

@@ -45,8 +147,6 @@ static void *vb2_dc_cookie(void *buf_priv) static void *vb2_dc_vaddr(void *buf_priv) { struct vb2_dc_buf *buf = buf_priv; - if (!buf) - return 0;

return buf->vaddr; } @@ -58,6 +158,28 @@ static unsigned int vb2_dc_num_users(void *buf_priv) return atomic_read(&buf->refcount); }

+static void vb2_dc_prepare(void *buf_priv) +{ + struct vb2_dc_buf *buf = buf_priv; + struct sg_table *sgt = buf->dma_sgt; + + if (!sgt) + return; + + dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); +} + +static void vb2_dc_finish(void *buf_priv) +{ + struct vb2_dc_buf *buf = buf_priv; + struct sg_table *sgt = buf->dma_sgt; + + if (!sgt) + return; + + dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); +} + /*********************************************/ /* callbacks for MMAP buffers */ /*********************************************/ @@ -66,31 +188,70 @@ static void vb2_dc_put(void *buf_priv) { struct vb2_dc_buf *buf = buf_priv;

- if (atomic_dec_and_test(&buf->refcount)) { - dma_free_coherent(buf->dev, buf->size, buf->vaddr, - buf->dma_addr); - kfree(buf); - } + 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); }

static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) { struct device *dev = alloc_ctx; struct vb2_dc_buf *buf; + int ret; + int n_pages; + struct page **pages = NULL;

buf = kzalloc(sizeof *buf, GFP_KERNEL); if (!buf) return ERR_PTR(-ENOMEM);

- buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, GFP_KERNEL); + buf->dev = dev; + buf->size = size; + buf->vaddr = dma_alloc_coherent(buf->dev, buf->size, &buf->dma_addr, + GFP_KERNEL); + + ret = -ENOMEM; if (!buf->vaddr) { - dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size); - kfree(buf); - return ERR_PTR(-ENOMEM); + dev_err(dev, "dma_alloc_coherent of size %ld failed\n", + size); + goto fail_buf; }

- buf->dev = dev; - buf->size = size; + 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) { + printk(KERN_ERR "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) { + printk(KERN_ERR "failed to get buffer pages from DMA API\n"); + goto fail_pages; + } + if (ret != n_pages) { + ret = -EFAULT; + printk(KERN_ERR "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); + printk(KERN_ERR "failed to prepare sg table\n"); + goto fail_pages; + } + + /* pages are no longer needed */ + kfree(pages);

buf->handler.refcount = &buf->refcount; buf->handler.put = vb2_dc_put; @@ -99,59 +260,226 @@ 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(buf->dev, buf->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) { struct vb2_dc_buf *buf = buf_priv; + int ret; + + /* + * dma_mmap_* uses vm_pgoff as in-buffer offset, but we want to + * map whole buffer + */ + vma->vm_pgoff = 0; + + ret = dma_mmap_writecombine(buf->dev, vma, buf->vaddr, + buf->dma_addr, buf->size);

- if (!buf) { - printk(KERN_ERR "No buffer to map\n"); - return -EINVAL; + if (ret) { + printk(KERN_ERR "Remapping memory failed, error: %d\n", ret); + return ret; }

- return vb2_mmap_pfn_range(vma, buf->dma_addr, buf->size, - &vb2_common_vm_ops, &buf->handler); + 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; }

/*********************************************/ /* callbacks for USERPTR buffers */ /*********************************************/

+static inline int vma_is_io(struct vm_area_struct *vma) +{ + return !!(vma->vm_flags & (VM_IO | VM_PFNMAP)); +} + +static int vb2_dc_get_pages(unsigned long start, struct page **pages, + int n_pages, struct vm_area_struct **copy_vma, int write) +{ + struct vm_area_struct *vma; + int n = 0; /* number of get pages */ + int ret = -EFAULT; + + /* entering critical section for mm access */ + down_read(&current->mm->mmap_sem); + + vma = find_vma(current->mm, start); + if (!vma) { + printk(KERN_ERR "no vma for address %lu\n", start); + goto cleanup; + } + + if (vma_is_io(vma)) { + unsigned long pfn; + + if (vma->vm_end - start < n_pages * PAGE_SIZE) { + printk(KERN_ERR "vma is too small\n"); + goto cleanup; + } + + for (n = 0; n < n_pages; ++n, start += PAGE_SIZE) { + ret = follow_pfn(vma, start, &pfn); + if (ret) { + printk(KERN_ERR "no page for address %lu\n", + start); + goto cleanup; + } + pages[n] = pfn_to_page(pfn); + get_page(pages[n]); + } + } else { + n = get_user_pages(current, current->mm, start & PAGE_MASK, + n_pages, write, 1, pages, NULL); + if (n != n_pages) { + printk(KERN_ERR "got only %d of %d user pages\n", + n, n_pages); + goto cleanup; + } + } + + *copy_vma = vb2_get_vma(vma); + if (!*copy_vma) { + printk(KERN_ERR "failed to copy vma\n"); + ret = -ENOMEM; + goto cleanup; + } + + /* leaving critical section for mm access */ + up_read(&current->mm->mmap_sem); + + return 0; + +cleanup: + up_read(&current->mm->mmap_sem); + + /* putting user pages if used, can be done wothout the lock */ + while (n) + put_page(pages[--n]); + + return ret; +} + static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr, - unsigned long size, int write) + unsigned long size, int write) { struct vb2_dc_buf *buf; - struct vm_area_struct *vma; - dma_addr_t dma_addr = 0; - int ret; + unsigned long start, end, offset, offset2; + struct page **pages; + int n_pages; + int ret = 0; + struct sg_table *sgt; + unsigned long contig_size;

buf = kzalloc(sizeof *buf, GFP_KERNEL); if (!buf) return ERR_PTR(-ENOMEM);

- ret = vb2_get_contig_userptr(vaddr, size, &vma, &dma_addr); + buf->dev = alloc_ctx; + buf->dma_dir = write ? DMA_FROM_DEVICE : DMA_TO_DEVICE; + + start = (unsigned long)vaddr & PAGE_MASK; + offset = (unsigned long)vaddr & ~PAGE_MASK; + end = PAGE_ALIGN((unsigned long)vaddr + size); + offset2 = end - (unsigned long)vaddr - size; + n_pages = (end - start) >> PAGE_SHIFT; + + pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL); + if (!pages) { + ret = -ENOMEM; + printk(KERN_ERR "failed to allocate pages table\n"); + goto fail_buf; + } + + /* extract page list from userspace mapping */ + ret = vb2_dc_get_pages(start, pages, n_pages, &buf->vma, write); if (ret) { - printk(KERN_ERR "Failed acquiring VMA for vaddr 0x%08lx\n", - vaddr); - kfree(buf); - return ERR_PTR(ret); + printk(KERN_ERR "failed to get user pages\n"); + goto fail_pages; + } + + sgt = vb2_dc_pages_to_sgt(pages, n_pages, offset, offset2); + if (!sgt) { + printk(KERN_ERR "failed to create scatterlist table\n"); + ret = -ENOMEM; + goto fail_get_pages; }

+ /* pages are no longer needed */ + kfree(pages); + pages = NULL; + + 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"); + ret = -EIO; + goto fail_sgt; + } + + contig_size = vb2_dc_get_contiguous_size(sgt); + if (contig_size < size) { + printk(KERN_ERR "contiguous mapping is too small %lu/%lu\n", + contig_size, size); + ret = -EFAULT; + goto fail_map_sg; + } + + buf->dma_addr = sg_dma_address(sgt->sgl); buf->size = size; - buf->dma_addr = dma_addr; - buf->vma = vma; + buf->dma_sgt = sgt; + + atomic_inc(&buf->refcount);

return buf; + +fail_map_sg: + dma_unmap_sg(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir); + +fail_sgt: + vb2_dc_put_sgtable(sgt, 0); + +fail_get_pages: + while (pages && n_pages) + put_page(pages[--n_pages]); + vb2_put_vma(buf->vma); + +fail_pages: + kfree(pages); /* kfree is NULL-proof */ + +fail_buf: + kfree(buf); + + return ERR_PTR(ret); }

-static void vb2_dc_put_userptr(void *mem_priv) +static void vb2_dc_put_userptr(void *buf_priv) { - struct vb2_dc_buf *buf = mem_priv; - - if (!buf) - return; + struct vb2_dc_buf *buf = buf_priv; + struct sg_table *sgt = buf->dma_sgt;

+ dma_unmap_sg(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir); + vb2_dc_put_sgtable(sgt, !vma_is_io(buf->vma)); vb2_put_vma(buf->vma); kfree(buf); } @@ -168,6 +496,8 @@ const struct vb2_mem_ops vb2_dma_contig_memops = { .mmap = vb2_dc_mmap, .get_userptr = vb2_dc_get_userptr, .put_userptr = vb2_dc_put_userptr, + .prepare = vb2_dc_prepare, + .finish = vb2_dc_finish, .num_users = vb2_dc_num_users, }; EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);

Hi Laurent, Thank you very much for your comments and question. They were very useful. Please refer to the comments below.

On 03/22/2012 11:50 AM, Laurent Pinchart wrote:

Hi Tomasz,

Thanks for the patch.

On Thursday 22 March 2012 11:02:23 Laurent Pinchart wrote:

...

From: Tomasz Stanislawski t.stanislaws@samsung.com

This patch combines updates and fixes to dma-contig allocator. Moreover the allocator code was refactored. The most important changes are:

• functions were reordered
• move compression of scatterlist to separete function
• add support for multichunk but contiguous scatterlists
• simplified implementation of vb2-dma-contig context structure
• let mmap method to use dma_mmap_writecombine
• add support for scatterlist in userptr mode

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com [mmap method] Signed-off-by: Andrzej Pietrasiewicz andrzej.p@samsung.com [scatterlist in userptr mode] Signed-off-by: Kamil Debski k.debski@samsung.com [bugfixing] Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com [core refactoring, helper functions] Signed-off-by: Kyungmin Park kyungmin.park@samsung.com

---

drivers/media/video/videobuf2-dma-contig.c | 400 +++++++++++++++++++++++++--- 1 files changed, 365 insertions(+), 35 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index c898e6f..9965465 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -10,9 +10,12 @@

• the Free Software Foundation.

*/

+#include <linux/dma-buf.h> +#include <linux/dma-mapping.h> #include <linux/module.h> +#include <linux/scatterlist.h> +#include <linux/sched.h> #include <linux/slab.h> -#include <linux/dma-mapping.h>

#include <media/videobuf2-core.h> #include <media/videobuf2-memops.h> @@ -22,16 +25,115 @@ struct vb2_dc_buf { void *vaddr; unsigned long size; dma_addr_t dma_addr;

• struct sg_table *dma_sgt;

• enum dma_data_direction dma_dir;

  /* MMAP related */ struct vb2_vmarea_handler handler; atomic_t refcount;

• struct sg_table *sgt_base;

  /* USERPTR related */ struct vm_area_struct *vma;

};

/*********************************************/ +/* scatterlist table functions */ +/*********************************************/

+static struct sg_table *vb2_dc_pages_to_sgt(struct page **pages,

• unsigned long n_pages, size_t offset, size_t offset2)

+{

• struct sg_table *sgt;
• int i, j; /* loop counters */
• int cur_page, chunks;
• int ret;
• struct scatterlist *s;
• sgt = kzalloc(sizeof *sgt, GFP_KERNEL);
• if (!sgt)

• return ERR_PTR(-ENOMEM);
  

• /* compute number of chunks */
• chunks = 1;
• for (i = 1; i < n_pages; ++i)

• if (pages[i] != pages[i - 1] + 1)
  

• 	++chunks;
  

• ret = sg_alloc_table(sgt, chunks, GFP_KERNEL);
• if (ret) {

• kfree(sgt);
  

• return ERR_PTR(-ENOMEM);
  

• }
• /* merging chunks and putting them into the scatterlist */
• cur_page = 0;
• for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {

• size_t size = PAGE_SIZE;
  

• for (j = cur_page + 1; j < n_pages; ++j) {
  

• 	if (pages[j] != pages[j - 1] + 1)
  

• 		break;
  

• 	size += PAGE_SIZE;
  

• }
  

• /* cut offset if chunk starts at the first page */
  

• if (cur_page == 0)
  

• 	size -= offset;
  

• /* cut offset2 if chunk ends at the last page */
  

• if (j == n_pages)
  

• 	size -= offset2;
  

• sg_set_page(s, pages[cur_page], size, offset);
  

• offset = 0;
  

• cur_page = j;
  

• }
• return sgt;

+}

+static void vb2_dc_release_sgtable(struct sg_table *sgt) +{

• sg_free_table(sgt);
• kfree(sgt);

+}

+static void vb2_dc_put_sgtable(struct sg_table *sgt, int dirty) +{

• struct scatterlist *s;
• int i, j;
• for_each_sg(sgt->sgl, s, sgt->nents, i) {

• struct page *page = sg_page(s);
  

• int n_pages = PAGE_ALIGN(s->offset + s->length) >> PAGE_SHIFT;
  

• for (j = 0; j < n_pages; ++j, ++page) {
  

• 	if (dirty)
  

• 		set_page_dirty_lock(page);
  

• 	put_page(page);
  

• }
  

• }
• vb2_dc_release_sgtable(sgt);

+}

+static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt) +{

• struct scatterlist *s;
• dma_addr_t expected = sg_dma_address(sgt->sgl);
• int i;
• unsigned long size = 0;
• for_each_sg(sgt->sgl, s, sgt->nents, i) {

• if (sg_dma_address(s) != expected)
  

• 	break;
  

• expected = sg_dma_address(s) + sg_dma_len(s);
  

• size += sg_dma_len(s);
  

• }
• return size;

+}

+/*********************************************/ /* callbacks for all buffers */ /*********************************************/

@@ -45,8 +147,6 @@ static void *vb2_dc_cookie(void *buf_priv) static void *vb2_dc_vaddr(void *buf_priv) { struct vb2_dc_buf *buf = buf_priv;

• if (!buf)

• return 0;
  
  

  return buf->vaddr;

} @@ -58,6 +158,28 @@ static unsigned int vb2_dc_num_users(void *buf_priv) return atomic_read(&buf->refcount); }

+static void vb2_dc_prepare(void *buf_priv) +{

• struct vb2_dc_buf *buf = buf_priv;
• struct sg_table *sgt = buf->dma_sgt;
• if (!sgt)

• return;
  

• dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);

+}

+static void vb2_dc_finish(void *buf_priv) +{

• struct vb2_dc_buf *buf = buf_priv;
• struct sg_table *sgt = buf->dma_sgt;
• if (!sgt)

• return;
  

• dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);

+}

/*********************************************/ /* callbacks for MMAP buffers */ /*********************************************/ @@ -66,31 +188,70 @@ static void vb2_dc_put(void *buf_priv) { struct vb2_dc_buf *buf = buf_priv;

• if (atomic_dec_and_test(&buf->refcount)) {

• dma_free_coherent(buf->dev, buf->size, buf->vaddr,
  

• 		  buf->dma_addr);
  

• kfree(buf);
  

• }

• 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);

}

static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) { struct device *dev = alloc_ctx; struct vb2_dc_buf *buf;

• int ret;

• int n_pages;http://165.213.219.115/cgi-bin/gitweb.cgi?p=mirror/linux-3.0-midas%3Ba=short...

• struct page **pages = NULL;

  buf = kzalloc(sizeof *buf, GFP_KERNEL); if (!buf) return ERR_PTR(-ENOMEM);

• buf->vaddr = dma_alloc_coherent(dev, size, &buf->dma_addr, GFP_KERNEL);

• buf->dev = dev;
• buf->size = size;
• buf->vaddr = dma_alloc_coherent(buf->dev, buf->size, &buf->dma_addr,

• GFP_KERNEL);
  

• ret = -ENOMEM; if (!buf->vaddr) {

• dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
  

• kfree(buf);
  

• return ERR_PTR(-ENOMEM);
  

• dev_err(dev, "dma_alloc_coherent of size %ld failed\n",
  

• 	size);
  

• goto fail_buf;
  

  }

• buf->dev = dev;
• buf->size = size;

• 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) {

• printk(KERN_ERR "failed to alloc page table\n");
  

• goto fail_dma;
  

• }
• ret = dma_get_pages(dev, buf->vaddr, buf->dma_addr, pages, n_pages);

As the only purpose of this is to retrieve a list of pages that will be used to create a single-entry sgt, wouldn't it be possible to shortcut the code and get the physical address of the buffer directly ?

The physical address should not be used since they are meaningless in a context of different devices. It seams that only the list of pages is more-or-less portable between different drivers.

The physical address is already present in buf->dma_addr, but it is only valid if the device has no MMU. Notice that vb2-dma-contig possess no knowledge if MMU is present for a given device.

The sg list is not going to be single-entry if the device is provided with its own MMU.

...

• if (ret < 0) {

• printk(KERN_ERR "failed to get buffer pages from DMA API\n");
  

• goto fail_pages;
  

• }
• if (ret != n_pages) {

• ret = -EFAULT;
  

• printk(KERN_ERR "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);
  

• printk(KERN_ERR "failed to prepare sg table\n");
  

• goto fail_pages;
  

• }

buf->sgt_base isn't used in this patch. I would move the buf->sgt_base creation code to the patch that uses it then, or to its own patch just before the patch that uses it.

Good point. The sgt_base is used by exporter only. Thanks for noticing it.

...

• /* pages are no longer needed */

• kfree(pages);

  buf->handler.refcount = &buf->refcount; buf->handler.put = vb2_dc_put;

@@ -99,59 +260,226 @@ 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(buf->dev, buf->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) { struct vb2_dc_buf *buf = buf_priv;

• int ret;
• /*

• * dma_mmap_* uses vm_pgoff as in-buffer offset, but we want to
  

• * map whole buffer
  

• */
  

• vma->vm_pgoff = 0;
• ret = dma_mmap_writecombine(buf->dev, vma, buf->vaddr,

• buf->dma_addr, buf->size);
  
  

• if (!buf) {

• printk(KERN_ERR "No buffer to map\n");
  

• return -EINVAL;
  

• if (ret) {

• printk(KERN_ERR "Remapping memory failed, error: %d\n", ret);
  

• return ret;
  

  }

• return vb2_mmap_pfn_range(vma, buf->dma_addr, buf->size,

• 		  &vb2_common_vm_ops, &buf->handler);
  

• 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;

}

/*********************************************/ /* callbacks for USERPTR buffers */ /*********************************************/

+static inline int vma_is_io(struct vm_area_struct *vma) +{

• return !!(vma->vm_flags & (VM_IO | VM_PFNMAP));

Isn't VM_PFNMAP enough ? Wouldn't it be possible (at least in theory) to get a discontinuous physical range with VM_IO ?

Frankly, I found that that in get_user_pages flags are checked against (VM_IO | VM_PFNMAP). Probably for noMMU (not no IOMMU) case it is possible to get vma with VM_IO on and VM_PFNMAP off, isn't it?

The problem is that this framework should work in both cases so this check was added just in case :).

...

+}

+static int vb2_dc_get_pages(unsigned long start, struct page **pages,

• int n_pages, struct vm_area_struct **copy_vma, int write)

+{

• struct vm_area_struct *vma;
• int n = 0; /* number of get pages */
• int ret = -EFAULT;
• /* entering critical section for mm access */
• down_read(&current->mm->mmap_sem);

This will generate AB-BA deadlock warnings if lockdep is enabled. This function is called with the queue lock held, and the mmap() handler which takes the queue lock is called with current->mm->mmap_sem held.

This is a known issue with videobuf2, not specific to this patch. The warning is usually a false positive (which we still need to fix, as it worries users), but can become a real issue if an MMAP queue and a USERPTR queue are created by a driver with the same queue lock.

Good point. Do you know any good solution to this problem?

...

• vma = find_vma(current->mm, start);
• if (!vma) {

• printk(KERN_ERR "no vma for address %lu\n", start);
  

• goto cleanup;
  

• }
• if (vma_is_io(vma)) {

• unsigned long pfn;
  

• if (vma->vm_end - start < n_pages * PAGE_SIZE) {
  

• 	printk(KERN_ERR "vma is too small\n");
  

• 	goto cleanup;
  

• }
  

• for (n = 0; n < n_pages; ++n, start += PAGE_SIZE) {
  

• 	ret = follow_pfn(vma, start, &pfn);
  

• 	if (ret) {
  

• 		printk(KERN_ERR "no page for address %lu\n",
  

• 			start);
  

• 		goto cleanup;
  

• 	}
  

• 	pages[n] = pfn_to_page(pfn);
  

• 	get_page(pages[n]);
  

This worries me. When the VM_PFNMAP flag is set, the memory pages are not backed by a struct page. Creating a struct page pointer out of it can be an acceptable hack (for instance to store a page in an scatterlist with sg_set_page() and then retrieve its physical address with sg_phys()), but you should not expect the struct page to be valid for anything else. Calling get_page() on it will likely crash.

You are completetly right. This is the corner case where list of pages is not a portable way of describing the memory. Maybe pfn_valid should be used to check validity of the page (pfn) before getting it?

...

• }
  

• } else {

• n = get_user_pages(current, current->mm, start & PAGE_MASK,
  

• 	n_pages, write, 1, pages, NULL);
  

• if (n != n_pages) {
  

• 	printk(KERN_ERR "got only %d of %d user pages\n",
  

• 		n, n_pages);
  

• 	goto cleanup;
  

• }
  

• }
• *copy_vma = vb2_get_vma(vma);
• if (!*copy_vma) {

• printk(KERN_ERR "failed to copy vma\n");
  

• ret = -ENOMEM;
  

• goto cleanup;
  

• }

Do we really need to make a copy of the VMA ? The only reason why we store a pointer to it is to check the flags in vb2_dc_put_userptr(). We could store the flags instead and avoid vb2_get_dma()/vb2_put_dma() calls altogether.

I remember that there was a very good reason of copying this vma structure. You caught me on 'cargo-cult' programming. I will do some reverse engineering and try to answer it soon.

...

• /* leaving critical section for mm access */
• up_read(&current->mm->mmap_sem);
• return 0;

+cleanup:

• up_read(&current->mm->mmap_sem);
• /* putting user pages if used, can be done wothout the lock */
• while (n)

• put_page(pages[--n]);
  

• return ret;

+}

static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,

• 			unsigned long size, int write)
  

• unsigned long size, int write)

{ struct vb2_dc_buf *buf;

• struct vm_area_struct *vma;
• dma_addr_t dma_addr = 0;
• int ret;

• unsigned long start, end, offset, offset2;

• struct page **pages;

• int n_pages;

• int ret = 0;

• struct sg_table *sgt;

• unsigned long contig_size;

  buf = kzalloc(sizeof *buf, GFP_KERNEL); if (!buf) return ERR_PTR(-ENOMEM);

• ret = vb2_get_contig_userptr(vaddr, size, &vma, &dma_addr);

• buf->dev = alloc_ctx;
• buf->dma_dir = write ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
• start = (unsigned long)vaddr & PAGE_MASK;
• offset = (unsigned long)vaddr & ~PAGE_MASK;
• end = PAGE_ALIGN((unsigned long)vaddr + size);
• offset2 = end - (unsigned long)vaddr - size;
• n_pages = (end - start) >> PAGE_SHIFT;
• pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL);
• if (!pages) {

• ret = -ENOMEM;
  

• printk(KERN_ERR "failed to allocate pages table\n");
  

• goto fail_buf;
  

• }
• /* extract page list from userspace mapping */
• ret = vb2_dc_get_pages(start, pages, n_pages, &buf->vma, write); if (ret) {

• printk(KERN_ERR "Failed acquiring VMA for vaddr 0x%08lx\n",
  

• 		vaddr);
  

• kfree(buf);
  

• return ERR_PTR(ret);
  

• printk(KERN_ERR "failed to get user pages\n");
  

• goto fail_pages;
  

• }
• sgt = vb2_dc_pages_to_sgt(pages, n_pages, offset, offset2);
• if (!sgt) {

• printk(KERN_ERR "failed to create scatterlist table\n");
  

• ret = -ENOMEM;
  

• goto fail_get_pages;
  

  }

This looks overly complex to me. You create a multi-chunk sgt out of the user pointer address and map it completely, and then check if it starts with a big enough contiguous chunk.

Notice that vb2_dc_pages_to_sgt does compress contiguous ranges of pfns (pages). So if the memory is contiguous, then sigle chunk sglist is produced. The memory used to store pages list is just temporary. It is freed after the sglist is created.

Why don't you create an sgt with a single continuous chunk then ? In the VM_PFNMAP case you could check whether the area is contiguous when you follow the PFNs, stop at the first discontinuity, and create an sgt with a single element right there. You would then need to call vb2_dc_pages_to_sgt() in the normal case only, and stop at the first discontinuity as well.

Discontinuity of pfns is not a problem if a device has own IOMMU. It is not known for vb2-dma-contig if mapping this multi-chunk sglist will succeed until calling and checking a result of dma_map_sg.

Why bothering if both VM_PFNMAP and non-VM_PFNMAP are handled in the same way after list of pages is obtained? Trating them the same way allows to reuse code and simplify the program flow.

The DMA framework does not provide any way to force single chunk mapping in sg. If the device is capable of mapping discontinous pages into a single chunk the DMA framework will probably do merge the pages. The documentation encourages to merge the list but it is not obligatory.

The reason is that if 'struct scatterlist' contains no dma_length field, what is controlled by CONFIG_NEED_SG_DMA_LENGTH macro, then field length is used instead. Chunk marging cannot be done in such a case. This is the reason why I look for the longest contiguous block.

...

• /* pages are no longer needed */
• kfree(pages);
• pages = NULL;
• 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");
  

• ret = -EIO;
  

• goto fail_sgt;
  

• }
• contig_size = vb2_dc_get_contiguous_size(sgt);
• if (contig_size < size) {

• printk(KERN_ERR "contiguous mapping is too small %lu/%lu\n",
  

• 	contig_size, size);
  

• ret = -EFAULT;
  

• goto fail_map_sg;
  

• }
• buf->dma_addr = sg_dma_address(sgt->sgl); buf->size = size;

• buf->dma_addr = dma_addr;
• buf->vma = vma;

• buf->dma_sgt = sgt;

• atomic_inc(&buf->refcount);

  return buf;

+fail_map_sg:

• dma_unmap_sg(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);

I think this will break in the VM_PFNMAP case on non-coherent architectures. arm_dma_unmap_page() will call __dma_page_dev_to_cpu() in that case, which can dereference struct page. As explain above, the struct page isn't valid with VM_PFNMAP. I haven't check the dma_map_sg() and dma_sync_sg_*() calls, but changes are they might break as well.

It will crash as long it is true that there is no struct page behind given pfn. In practice, I found that VM_PFNMAP means that one cannot assume that there is a 'struct page' behind PFNs of a given mapping. Thoses struct pages really exists for all our drivers. Anyway, I agree that using those pages is a hack.

It could be avoided if vb2_dc_get_pages returned a list of PFNs. Anyway, those PFNs have to be transformed to pages to create an sglist. Those pointers might be accessed somewhere deep inside dma_map_sg internals.

The quite good solution would be dropping support for VM_PFNMAP mappings since they cannot be handled reliably.

...

+fail_sgt:

• vb2_dc_put_sgtable(sgt, 0);

+fail_get_pages:

• while (pages && n_pages)

• put_page(pages[--n_pages]);
  

• vb2_put_vma(buf->vma);

+fail_pages:

• kfree(pages); /* kfree is NULL-proof */

+fail_buf:

• kfree(buf);
• return ERR_PTR(ret);

}

-static void vb2_dc_put_userptr(void *mem_priv) +static void vb2_dc_put_userptr(void *buf_priv) {

• struct vb2_dc_buf *buf = mem_priv;
• if (!buf)

• return;
  

• struct vb2_dc_buf *buf = buf_priv;

• struct sg_table *sgt = buf->dma_sgt;

• dma_unmap_sg(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);

• vb2_dc_put_sgtable(sgt, !vma_is_io(buf->vma)); vb2_put_vma(buf->vma); kfree(buf);

} @@ -168,6 +496,8 @@ const struct vb2_mem_ops vb2_dma_contig_memops = { .mmap = vb2_dc_mmap, .get_userptr = vb2_dc_get_userptr, .put_userptr = vb2_dc_put_userptr,

• .prepare = vb2_dc_prepare,
• .finish = vb2_dc_finish, .num_users = vb2_dc_num_users,

}; EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);

Regards, Tomasz Stanislawski

Hello Thomasz, Marek,

I found an issue with below patch. Please read below.

On 03/13/2012 03:47 PM, Tomasz Stanislawski wrote:

This patch combines updates and fixes to dma-contig allocator. Moreover the allocator code was refactored. The most important changes are:

• functions were reordered
• move compression of scatterlist to separete function
• add support for multichunk but contiguous scatterlists
• simplified implementation of vb2-dma-contig context structure
• let mmap method to use dma_mmap_writecombine
• add support for scatterlist in userptr mode

Signed-off-by: Marek Szyprowskim.szyprowski@samsung.com [mmap method] Signed-off-by: Andrzej Pietrasiewiczandrzej.p@samsung.com [scatterlist in userptr mode] Signed-off-by: Kamil Debskik.debski@samsung.com [bugfixing] Signed-off-by: Tomasz Stanislawskit.stanislaws@samsung.com [core refactoring, helper functions] Signed-off-by: Kyungmin Parkkyungmin.park@samsung.com

---

drivers/media/video/videobuf2-dma-contig.c | 495 +++++++++++++++++++++++----- 1 files changed, 414 insertions(+), 81 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index f17ad98..c1dc043 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -10,173 +10,506 @@

• the Free Software Foundation.

*/

+#include<linux/dma-buf.h> +#include<linux/dma-mapping.h> #include<linux/module.h> +#include<linux/scatterlist.h> +#include<linux/sched.h> #include<linux/slab.h> -#include<linux/dma-mapping.h>

#include<media/videobuf2-core.h> #include<media/videobuf2-memops.h>

-struct vb2_dc_conf {

• struct device *dev;

-};

• struct vb2_dc_buf {
• struct vb2_dc_conf *conf;

• struct device *dev; void *vaddr;

• dma_addr_t dma_addr; unsigned long size;
• struct vm_area_struct *vma;
• atomic_t refcount;

• dma_addr_t dma_addr;
• struct sg_table *dma_sgt;
• enum dma_data_direction dma_dir;
• /* MMAP related */ struct vb2_vmarea_handler handler;
• atomic_t refcount;
• struct sg_table *sgt_base;
• /* USERPTR related */
• struct vm_area_struct *vma; };

-static void vb2_dma_contig_put(void *buf_priv); +/*********************************************/ +/* scatterlist table functions */ +/*********************************************/

-static void *vb2_dma_contig_alloc(void *alloc_ctx, unsigned long size) +static struct sg_table *vb2_dc_pages_to_sgt(struct page **pages,

• unsigned long n_pages, size_t offset, size_t offset2) {

• struct vb2_dc_conf *conf = alloc_ctx;
• struct vb2_dc_buf *buf;

• struct sg_table *sgt;
• int i, j; /* loop counters */
• int cur_page, chunks;
• int ret;
• struct scatterlist *s;

• buf = kzalloc(sizeof *buf, GFP_KERNEL);
• if (!buf)

• sgt = kzalloc(sizeof *sgt, GFP_KERNEL);
• if (!sgt) return ERR_PTR(-ENOMEM);

• buf->vaddr = dma_alloc_coherent(conf->dev, size,&buf->dma_addr,

• 			GFP_KERNEL);
  

• if (!buf->vaddr) {

• dev_err(conf->dev, "dma_alloc_coherent of size %ld failed\n",
  

• 	size);
  

• kfree(buf);
  

• /* compute number of chunks */
• chunks = 1;
• for (i = 1; i< n_pages; ++i)

• if (pages[i] != pages[i - 1] + 1)
  

• 	++chunks;
  

• ret = sg_alloc_table(sgt, chunks, GFP_KERNEL);
• if (ret) {

• kfree(sgt);
  

  return ERR_PTR(-ENOMEM); }

• buf->conf = conf;
• buf->size = size;
• buf->handler.refcount =&buf->refcount;
• buf->handler.put = vb2_dma_contig_put;
• buf->handler.arg = buf;

• /* merging chunks and putting them into the scatterlist */
• cur_page = 0;
• for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {

• size_t size = PAGE_SIZE;
  

• for (j = cur_page + 1; j<  n_pages; ++j) {
  

• 	if (pages[j] != pages[j - 1] + 1)
  

• 		break;
  

• 	size += PAGE_SIZE;
  

• }
  

• /* cut offset if chunk starts at the first page */
  

• if (cur_page == 0)
  

• 	size -= offset;
  

• /* cut offset2 if chunk ends at the last page */
  

• if (j == n_pages)
  

• 	size -= offset2;
  

• sg_set_page(s, pages[cur_page], size, offset);
  

• offset = 0;
  

• cur_page = j;
  

• }

• atomic_inc(&buf->refcount);

• return sgt;

+}

• return buf;

+static void vb2_dc_release_sgtable(struct sg_table *sgt) +{

• sg_free_table(sgt);
• kfree(sgt); }

-static void vb2_dma_contig_put(void *buf_priv) +static void vb2_dc_put_sgtable(struct sg_table *sgt, int dirty) {

• struct vb2_dc_buf *buf = buf_priv;

• struct scatterlist *s;
• int i, j;
• for_each_sg(sgt->sgl, s, sgt->nents, i) {

• struct page *page = sg_page(s);
  

• int n_pages = PAGE_ALIGN(s->offset + s->length)>>  PAGE_SHIFT;
  

• for (j = 0; j<  n_pages; ++j, ++page) {
  

• 	if (dirty)
  

• 		set_page_dirty_lock(page);
  

• 	put_page(page);
  

• }
  

• }
• vb2_dc_release_sgtable(sgt);

+}

• if (atomic_dec_and_test(&buf->refcount)) {

• dma_free_coherent(buf->conf->dev, buf->size, buf->vaddr,
  

• 		  buf->dma_addr);
  

• kfree(buf);
  

+static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt) +{

• struct scatterlist *s;
• dma_addr_t expected = sg_dma_address(sgt->sgl);
• int i;
• unsigned long size = 0;
• for_each_sg(sgt->sgl, s, sgt->nents, i) {

• if (sg_dma_address(s) != expected)
  

• 	break;
  

• expected = sg_dma_address(s) + sg_dma_len(s);
  

• size += sg_dma_len(s);
  

  }
• return size; }

-static void *vb2_dma_contig_cookie(void *buf_priv) +/*********************************************/ +/* callbacks for all buffers */ +/*********************************************/

+static void *vb2_dc_cookie(void *buf_priv) { struct vb2_dc_buf *buf = buf_priv;

return&buf->dma_addr; }

-static void *vb2_dma_contig_vaddr(void *buf_priv) +static void *vb2_dc_vaddr(void *buf_priv) { struct vb2_dc_buf *buf = buf_priv;

• if (!buf)

• return 0;
  
  

  return buf->vaddr; }

-static unsigned int vb2_dma_contig_num_users(void *buf_priv) +static unsigned int vb2_dc_num_users(void *buf_priv) { struct vb2_dc_buf *buf = buf_priv;

return atomic_read(&buf->refcount); }

-static int vb2_dma_contig_mmap(void *buf_priv, struct vm_area_struct *vma) +static void vb2_dc_prepare(void *buf_priv) { struct vb2_dc_buf *buf = buf_priv;

• struct sg_table *sgt = buf->dma_sgt;

• if (!buf) {

• printk(KERN_ERR "No buffer to map\n");
  

• return -EINVAL;
  

• }

• if (!sgt)

• return;
  

• dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);

+}

+static void vb2_dc_finish(void *buf_priv) +{

• struct vb2_dc_buf *buf = buf_priv;
• struct sg_table *sgt = buf->dma_sgt;
• if (!sgt)

• return;
  

• dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);

+}

+/*********************************************/ +/* callbacks for MMAP buffers */ +/*********************************************/

+static void vb2_dc_put(void *buf_priv) +{

• struct vb2_dc_buf *buf = buf_priv;

• return vb2_mmap_pfn_range(vma, buf->dma_addr, buf->size,

• 		&vb2_common_vm_ops,&buf->handler);
  

• 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); }

-static void *vb2_dma_contig_get_userptr(void *alloc_ctx, unsigned long vaddr,

• 			unsigned long size, int write)
  

+static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) {

• struct device *dev = alloc_ctx; struct vb2_dc_buf *buf;

• struct vm_area_struct *vma;
• dma_addr_t dma_addr = 0; int ret;

• int n_pages;

• struct page **pages = NULL;

  buf = kzalloc(sizeof *buf, GFP_KERNEL); if (!buf) return ERR_PTR(-ENOMEM);

• ret = vb2_get_contig_userptr(vaddr, size,&vma,&dma_addr);
• if (ret) {

• printk(KERN_ERR "Failed acquiring VMA for vaddr 0x%08lx\n",
  

• 		vaddr);
  

• kfree(buf);
  

• return ERR_PTR(ret);
  

• buf->dev = dev;
• buf->size = size;
• buf->vaddr = dma_alloc_coherent(buf->dev, buf->size,&buf->dma_addr,

• GFP_KERNEL);
  

Memory allocation can happen from device coherent or alloc_pages(). Pls ref to arm_dma_alloc() in dma mapping v7 patchset. This info will be elaborated more below.

• ret = -ENOMEM;
• if (!buf->vaddr) {

• dev_err(dev, "dma_alloc_coherent of size %ld failed\n",
  

• 	size);
  

• goto fail_buf;
  

  }

• buf->size = size;
• buf->dma_addr = dma_addr;
• buf->vma = vma;

• 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) {

• printk(KERN_ERR "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) {

• printk(KERN_ERR "failed to get buffer pages from DMA API\n");
  

• goto fail_pages;
  

• }

• if (ret != n_pages) {

• ret = -EFAULT;
  

• printk(KERN_ERR "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);
  

• printk(KERN_ERR "failed to prepare sg table\n");
  

• goto fail_pages;
  

• }

• /* pages are no longer needed */

• kfree(pages);

• buf->handler.refcount =&buf->refcount;

• buf->handler.put = vb2_dc_put;

• buf->handler.arg = buf;

• atomic_inc(&buf->refcount);

  return buf;

+fail_pages:

• kfree(pages);

+fail_dma:

• dma_free_coherent(buf->dev, buf->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) +{

• struct vb2_dc_buf *buf = buf_priv;
• int ret;
• /*

• * dma_mmap_* uses vm_pgoff as in-buffer offset, but we want to
  

• * map whole buffer
  

• */
  

• vma->vm_pgoff = 0;
• ret = dma_mmap_writecombine(buf->dev, vma, buf->vaddr,

• buf->dma_addr, buf->size);
  

For devices, which do not have a coherent/reserved pool, then the allocation in function __dma_alloc() happens, and the memory will be remapped by calling __dma_alloc_remap(). In the above function, arm_vmregion_find() will be called, and it succeeds to get the map and it will be mapped to user.

This has issues with devices with coherent memory. If we have any device which has declared coherent memory, then the allocation happens from per-device coherent area. Eg: MFC which declares: s5p_mfc_reserve_mem(). In that case, dma_mmap_writecombine() fails, and hence vb2_dc_mmap() fails as well.

I will reply the same in the dma mapping thread as well, as I dont want to combine both these RFC's together.

• 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; }

-static void vb2_dma_contig_put_userptr(void *mem_priv) +/*********************************************/ +/* callbacks for USERPTR buffers */ +/*********************************************/

+static inline int vma_is_io(struct vm_area_struct *vma) {

• struct vb2_dc_buf *buf = mem_priv;

• return !!(vma->vm_flags& (VM_IO | VM_PFNMAP));

+}

+static int vb2_dc_get_pages(unsigned long start, struct page **pages,

• int n_pages, struct vm_area_struct **copy_vma, int write)

+{

• struct vm_area_struct *vma;
• int n = 0; /* number of get pages */
• int ret = -EFAULT;
• /* entering critical section for mm access */
• down_read(&current->mm->mmap_sem);
• vma = find_vma(current->mm, start);
• if (!vma) {

• printk(KERN_ERR "no vma for address %lu\n", start);
  

• goto cleanup;
  

• }
• if (vma_is_io(vma)) {

• unsigned long pfn;
  

• if (vma->vm_end - start<  n_pages * PAGE_SIZE) {
  

• 	printk(KERN_ERR "vma is too small\n");
  

• 	goto cleanup;
  

• }
  

• for (n = 0; n<  n_pages; ++n, start += PAGE_SIZE) {
  

• 	ret = follow_pfn(vma, start,&pfn);
  

• 	if (ret) {
  

• 		printk(KERN_ERR "no page for address %lu\n",
  

• 			start);
  

• 		goto cleanup;
  

• 	}
  

• 	pages[n] = pfn_to_page(pfn);
  

• 	get_page(pages[n]);
  

• }
  

• } else {

• n = get_user_pages(current, current->mm, start&  PAGE_MASK,
  

• 	n_pages, write, 1, pages, NULL);
  

• if (n != n_pages) {
  

• 	printk(KERN_ERR "got only %d of %d user pages\n",
  

• 		n, n_pages);
  

• 	goto cleanup;
  

• }
  

• }
• *copy_vma = vb2_get_vma(vma);
• if (!*copy_vma) {

• printk(KERN_ERR "failed to copy vma\n");
  

• ret = -ENOMEM;
  

• goto cleanup;
  

• }
• /* leaving critical section for mm access */
• up_read(&current->mm->mmap_sem);
• return 0;

+cleanup:

• up_read(&current->mm->mmap_sem);
• /* putting user pages if used, can be done wothout the lock */
• while (n)

• put_page(pages[--n]);
  

• return ret;

+}

+static void vb2_dc_put_userptr(void *buf_priv) +{

• struct vb2_dc_buf *buf = buf_priv;
• struct sg_table *sgt = buf->dma_sgt;
• dma_unmap_sg(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
• vb2_dc_put_sgtable(sgt, !vma_is_io(buf->vma));
• vb2_put_vma(buf->vma);
• kfree(buf);

+}

+static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,

• unsigned long size, int write)

+{

• struct vb2_dc_buf *buf;
• unsigned long start, end, offset, offset2;
• struct page **pages;
• int n_pages;
• int ret = 0;
• struct sg_table *sgt;
• unsigned long contig_size;
• buf = kzalloc(sizeof *buf, GFP_KERNEL); if (!buf)

• return;
  

• return ERR_PTR(-ENOMEM);
  

• buf->dev = alloc_ctx;
• buf->dma_dir = write ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
• start = (unsigned long)vaddr& PAGE_MASK;
• offset = (unsigned long)vaddr& ~PAGE_MASK;
• end = PAGE_ALIGN((unsigned long)vaddr + size);
• offset2 = end - (unsigned long)vaddr - size;
• n_pages = (end - start)>> PAGE_SHIFT;
• pages = kmalloc(n_pages * sizeof pages[0], GFP_KERNEL);
• if (!pages) {

• ret = -ENOMEM;
  

• printk(KERN_ERR "failed to allocate pages table\n");
  

• goto fail_buf;
  

• }
• /* extract page list from userspace mapping */
• ret = vb2_dc_get_pages(start, pages, n_pages,&buf->vma, write);
• if (ret) {

• printk(KERN_ERR "failed to get user pages\n");
  

• goto fail_pages;
  

• }
• sgt = vb2_dc_pages_to_sgt(pages, n_pages, offset, offset2);
• if (!sgt) {

• printk(KERN_ERR "failed to create scatterlist table\n");
  

• ret = -ENOMEM;
  

• goto fail_get_pages;
  

• }
• /* pages are no longer needed */
• kfree(pages);
• pages = NULL;
• 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");
  

• ret = -EIO;
  

• goto fail_sgt;
  

• }
• contig_size = vb2_dc_get_contiguous_size(sgt);
• if (contig_size< size) {

• printk(KERN_ERR "contiguous mapping is too small %lu/%lu\n",
  

• 	contig_size, size);
  

• ret = -EFAULT;
  

• goto fail_map_sg;
  

• }
• buf->dma_addr = sg_dma_address(sgt->sgl);
• buf->size = size;
• buf->dma_sgt = sgt;
• atomic_inc(&buf->refcount);
• return buf;

+fail_map_sg:

• dma_unmap_sg(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);

+fail_sgt:

• vb2_dc_put_sgtable(sgt, 0);

+fail_get_pages:

• while (pages&& n_pages)

• put_page(pages[--n_pages]);
  

  vb2_put_vma(buf->vma);

+fail_pages:

• kfree(pages); /* kfree is NULL-proof */

+fail_buf: kfree(buf);

• return ERR_PTR(ret); }

+/*********************************************/ +/* DMA CONTIG exported functions */ +/*********************************************/

• const struct vb2_mem_ops vb2_dma_contig_memops = {

• .alloc = vb2_dma_contig_alloc,
• .put = vb2_dma_contig_put,
• .cookie = vb2_dma_contig_cookie,
• .vaddr = vb2_dma_contig_vaddr,
• .mmap = vb2_dma_contig_mmap,
• .get_userptr = vb2_dma_contig_get_userptr,
• .put_userptr = vb2_dma_contig_put_userptr,
• .num_users = vb2_dma_contig_num_users,

• .alloc = vb2_dc_alloc,

• .put = vb2_dc_put,

• .cookie = vb2_dc_cookie,

• .vaddr = vb2_dc_vaddr,

• .mmap = vb2_dc_mmap,

• .get_userptr = vb2_dc_get_userptr,

• .put_userptr = vb2_dc_put_userptr,

• .prepare = vb2_dc_prepare,

• .finish = vb2_dc_finish,

• .num_users = vb2_dc_num_users, }; EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);

  void *vb2_dma_contig_init_ctx(struct device *dev) {

• struct vb2_dc_conf *conf;
• conf = kzalloc(sizeof *conf, GFP_KERNEL);
• if (!conf)

• return ERR_PTR(-ENOMEM);
  

• conf->dev = dev;
• return conf;

• return dev; } EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx);

  void vb2_dma_contig_cleanup_ctx(void *alloc_ctx) {

• kfree(alloc_ctx); } EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx);

Regards, Subash

Hi Tomasz,

On Tuesday 13 March 2012 11:17:01 Tomasz Stanislawski wrote:

From: Sumit Semwal sumit.semwal@ti.com

This patch makes changes for adding dma-contig as a dma_buf user. It provides function implementations for the {attach, detach, map, unmap}_dmabuf() mem_ops of DMABUF memory type.

Signed-off-by: Sumit Semwal sumit.semwal@ti.com Signed-off-by: Sumit Semwal sumit.semwal@linaro.org [author of the original patch] Signed-off-by: Tomasz Stanislawski t.stanislaws@samsung.com [integration with refactored dma-contig allocator]

---

drivers/media/video/videobuf2-dma-contig.c | 116 ++++++++++++++++++++++++++++ 1 files changed, 116 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index c1dc043..746dd5f 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -35,6 +35,9 @@ struct vb2_dc_buf {

/* USERPTR related */ struct vm_area_struct *vma;

• /* DMABUF related */
• struct dma_buf_attachment *db_attach;

};

/*********************************************/ @@ -485,6 +488,115 @@ fail_buf: }

/*********************************************/ +/* callbacks for DMABUF buffers */ +/*********************************************/

+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;
• if (WARN_ON(!buf->db_attach)) {

• printk(KERN_ERR "trying to pin a non attached buffer\n");
  

• return -EINVAL;
  

• }
• if (WARN_ON(buf->dma_sgt)) {

• printk(KERN_ERR "dmabuf buffer is already pinned\n");
  

• return 0;
  

• }
• /* get the associated scatterlist for this buffer */
• sgt = dma_buf_map_attachment(buf->db_attach, buf->dma_dir);
• if (IS_ERR_OR_NULL(sgt)) {

• printk(KERN_ERR "Error getting dmabuf scatterlist\n");
  

• return -EINVAL;
  

• }

I've checked why dma_map_sg() was missing from here, and then remembered that the exporter is still responsible for mapping the buffer to the importer's device address space :-) I'll raise that topic separately.

• /* 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;
  

• }
• buf->dma_addr = sg_dma_address(sgt->sgl);
• buf->dma_sgt = sgt;
• return 0;

+fail_map:

• dma_buf_unmap_attachment(buf->db_attach, sgt);
• return ret;

+}

+static void vb2_dc_unmap_dmabuf(void *mem_priv) +{

• struct vb2_dc_buf *buf = mem_priv;
• struct sg_table *sgt = buf->dma_sgt;
• if (WARN_ON(!buf->db_attach)) {

• printk(KERN_ERR "trying to unpin a not attached buffer\n");
  

• return;
  

• }
• if (WARN_ON(!sgt)) {

• printk(KERN_ERR "dmabuf buffer is already unpinned\n");
  

• return;
  

• }
• dma_buf_unmap_attachment(buf->db_attach, sgt);
• buf->dma_addr = 0;
• buf->dma_sgt = NULL;

+}

+static void vb2_dc_detach_dmabuf(void *mem_priv) +{

• struct vb2_dc_buf *buf = mem_priv;
• if (buf->dma_addr)

• vb2_dc_unmap_dmabuf(buf);
  

Can detach() be called with the buffer still mapped() ? Wouldn't that be a bug in the caller ?

• /* detach this attachment */
• dma_buf_detach(buf->db_attach->dmabuf, buf->db_attach);
• kfree(buf);

+}

There's nothing allocator-specific in the attach/detach operations. Shouldn't they be moved to the vb2 core ?

+static void *vb2_dc_attach_dmabuf(void *alloc_ctx, struct dma_buf *dbuf,

• unsigned long size, int write)

+{

• struct vb2_dc_buf *buf;
• struct dma_buf_attachment *dba;
• if (dbuf->size < size)

• return ERR_PTR(-EFAULT);
  

• buf = kzalloc(sizeof *buf, GFP_KERNEL);
• if (!buf)

• return ERR_PTR(-ENOMEM);
  

• buf->dev = alloc_ctx;
• /* create attachment for the dmabuf with the user device */
• dba = dma_buf_attach(dbuf, buf->dev);
• if (IS_ERR(dba)) {

• printk(KERN_ERR "failed to attach dmabuf\n");
  

• kfree(buf);
  

• return dba;
  

• }
• buf->dma_dir = write ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
• buf->size = size;
• buf->db_attach = dba;
• return buf;

+}

+/*********************************************/ /* DMA CONTIG exported functions */ /*********************************************/

@@ -498,6 +610,10 @@ const struct vb2_mem_ops vb2_dma_contig_memops = { .put_userptr = vb2_dc_put_userptr, .prepare = vb2_dc_prepare, .finish = vb2_dc_finish,

• .map_dmabuf = vb2_dc_map_dmabuf,
• .unmap_dmabuf = vb2_dc_unmap_dmabuf,
• .attach_dmabuf = vb2_dc_attach_dmabuf,
• .detach_dmabuf = vb2_dc_detach_dmabuf, .num_users = vb2_dc_num_users,

}; EXPORT_SYMBOL_GPL(vb2_dma_contig_memops);

On Tue, Mar 13, 2012 at 11:17:02AM +0100, 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

Just a quick comment: You need to add a flags parameter to at least support O_CLOSEXEC semantics on the returned fd. Otherwise library writes will plainly hate you and demand a new version with this support added. -Daniel

---

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

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c index e6f67aa..fd157cb 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 74cab51..a125016 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,16 @@ static long __video_do_ioctl(struct file *file, dbgbuf(cmd, vfd, p); break; }

• case VIDIOC_EXPBUF:
• {

• struct v4l2_exportbuffer *p = arg;
  

• if (!ops->vidioc_expbuf)
  

• 	break;
  

• ret = ops->vidioc_expbuf(file, fh, p);
  

• break;
  

• } case VIDIOC_DQBUF: { struct v4l2_buffer *p = arg;

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index bb6844e..e71c787 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -680,6 +680,25 @@ 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: for non-multiplanar buffers with memory == V4L2_MEMORY_MMAP;

• • offset from the start of the device memory for this plane,
    

• • (or a "cookie" that should be passed to mmap() as offset)
    

• • 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.
• */

+struct v4l2_exportbuffer {

• __u32 fd;
• __u32 reserved0;
• __u32 mem_offset;
• __u32 reserved[13];

+};

/*

• O V E R L A Y P R E V I E W

*/ @@ -2303,6 +2322,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 4df031a..d8716c6f 100644 --- a/include/media/v4l2-ioctl.h +++ b/include/media/v4l2-ioctl.h @@ -120,6 +120,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

Hi Thomasz,

On 03/22/2012 04:46 PM, Laurent Pinchart wrote:

Hi Tomasz,

Thanks for the patch.

On Tuesday 13 March 2012 11:17:02 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.

I know code is more fun to write than documentation, but Documentation/DocBook/media/v4l will be sad if this patch is merged as-is ;-)

...

Signed-off-by: Tomasz Stanislawskit.stanislaws@samsung.com Signed-off-by: Kyungmin Parkkyungmin.park@samsung.com

---

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

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c index e6f67aa..fd157cb 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 74cab51..a125016 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,16 @@ static long __video_do_ioctl(struct file *file, dbgbuf(cmd, vfd, p); break; }

• case VIDIOC_EXPBUF:
• {

• struct v4l2_exportbuffer *p = arg;
  

• if (!ops->vidioc_expbuf)
  

• 	break;
  

• ret = ops->vidioc_expbuf(file, fh, p);
  

You can pass arg to ops->vidioc_expbuf() directly, there's no need to create a struct v4l2_exportbuffer *p variable.

...

• break;
  

• } case VIDIOC_DQBUF: { struct v4l2_buffer *p = arg;

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index bb6844e..e71c787 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -680,6 +680,25 @@ 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: for non-multiplanar buffers with memory ==

V4L2_MEMORY_MMAP;

I don't think we will ever support exporting anything else than V4L2_MEMORY_MMAP buffers. What will happen for multiplanar buffers ?

...

• • offset from the start of the device memory for this plane,
    

• • (or a "cookie" that should be passed to mmap() as offset)
    

Shouldn't the mem_offset field always be set to the mmap cookie value ? I'm a bit confused by the "or" part, it seems to have been copied from the v4l2_buffer documentation directly. We should clarify that.

...

• • 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.
• */

+struct v4l2_exportbuffer {

• __u32 fd;
• __u32 reserved0;

Why is there a reserved field here ?

+1 to Laurent. Any particular need for reserved0 and reserved[13] below? I think one void user pointer is sufficient even for future need.

...

• __u32 mem_offset;
• __u32 reserved[13];

+};

Also, what is the reason for returning the fd through this structure? To keep it aligned with other v4l2 calls? I liked(or now hate making change in the app) how it was being returned through the ioctl in your last patch.

...

/*

• O V E R L A Y P R E V I E W

*/ @@ -2303,6 +2322,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 4df031a..d8716c6f 100644 --- a/include/media/v4l2-ioctl.h +++ b/include/media/v4l2-ioctl.h @@ -120,6 +120,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);

Regards, Subash