Hi Laurent, Thank your for your comments.
On 06/06/2012 10:06 AM, Laurent Pinchart wrote:
Hi Tomasz,
Thanks for the patch.
On Wednesday 23 May 2012 15:07:27 Tomasz Stanislawski wrote:
This patch adds the setup of sglist list for MMAP buffers. It is needed for buffer exporting via DMABUF mechanism.
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 | 70 ++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c index 52b4f59..ae656be 100644 --- a/drivers/media/video/videobuf2-dma-contig.c +++ b/drivers/media/video/videobuf2-dma-contig.c @@ -32,6 +32,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,14 +190,37 @@ 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);
}
+static int vb2_dc_kaddr_to_pages(unsigned long kaddr,
- struct page **pages, unsigned int n_pages)
+{
- unsigned int i;
- unsigned long pfn;
- struct vm_area_struct vma = {
.vm_flags = VM_IO | VM_PFNMAP,.vm_mm = current->mm,- };
- for (i = 0; i < n_pages; ++i, kaddr += PAGE_SIZE) {
The follow_pfn() kerneldoc mentions that it looks up a PFN for a user address. The only users I've found in the kernel sources pass a user address. Is it legal to use it for kernel addresses ?
It is not completely legal :). As I understand the mm code, the follow_pfn works only for IO/PFN mappings. This is the typical case (every case?) of mappings created by dma_alloc_coherent.
In order to make this function work for a kernel pointer, one has to create an artificial VMA that has IO/PFN bits on.
This solution is a hack-around for dma_get_pages (aka dma_get_sgtable). This way the dependency on dma_get_pages was broken giving a small hope of merging vb2 exporting.
Marek prepared a patchset 'ARM: DMA-mapping: new extensions for buffer sharing' that adds dma buffers with no kernel mappings and dma_get_sgtable function.
However this patchset is still in a RFC state.
I have prepared a patch that removes vb2_dc_kaddr_to_pages and substitutes it with dma_get_pages. It will become a part of vb2-exporter patches just after dma_get_sgtable is merged (or at least acked by major maintainers).
if (follow_pfn(&vma, kaddr, &pfn))break;pages[i] = pfn_to_page(pfn);- }
- return i;
+}
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 +229,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 = vb2_dc_kaddr_to_pages((unsigned long)buf->vaddr, 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, size);
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 +274,17 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size) atomic_inc(&buf->refcount);
return buf;
+fail_pages:
- kfree(pages);
+fail_dma:
You can merge the fail_pages and fail_dma labels, as kfree(NULL) is valid.
Yes, I can. But there are two reasons for not doing that: - first: calling a dummy kfree introduces a negligible but non-zero overhead - second: the fail-path becomes more difficult to understand
Regards, Tomasz Stanislawski
- 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)