diff mbox

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

Message ID 4FD0BA9D.6010704@gmail.com
State New
Headers show

Commit Message

Subash Patel June 7, 2012, 2:28 p.m. UTC 
Hello Tomasz,

On 06/07/2012 06:06 AM, Laurent Pinchart wrote:
> Hi Tomasz,
>
> On Wednesday 06 June 2012 13:56:42 Tomasz Stanislawski wrote:
>> On 06/06/2012 10:06 AM, Laurent Pinchart wrote:
>>> 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
>
> [snip]
>
>>>> +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.
>
> That's totally understood :-) I'm fine with keeping the hack for now until the
> dma_get_sgtable() gets in a usable/mergeable state, please just mention it in
> the code with something like
>
> /* HACK: This is a temporary workaround until the dma_get_sgtable() function
> becomes available. */
>
>> 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).
>
The above function call (because of follow_pfn) doesn't succeed for all 
the allocated pages. Hence I created a patch(attached) which is based on 
[1] series. One can apply it for using your present patch-set in the 
meantime.

Regards,
Subash
[1] http://www.spinics.net/lists/kernel/msg1343092.html

Comments

Tomasz Stanislawski June 8, 2012, 2:31 p.m. UTC | #1 
Hi Laurent and Subash,

I confirm the issue found by Subash. The function
vb2_dc_kaddr_to_pages does fail for some occasions.
The failures are rather strange like 'got 95 of
150 pages'. It took me some time to find the reason
of the problem.

I found that dma_alloc_coherent for iommu an ARM does
use ioremap_page_range to map a buffer to the kernel
space. The mapping is done by updating the page-table.

The problem is that any process has a different first-level
page-table. The ioremap_page_range updates only the table
for init process. The PT present in current->mm shares
a majority of entries of 1st-level PT at kernel range
(above 0xc0000000) but *not all*. That is why
vb2_dc_kaddr_to_pages worked for small buffers and
occasionally failed for larger buffers.

I found two ways to fix this problem.
a) use &init_mm instead of current->mm while
   creating an artificial vma
b) access the dma memory by calling
   *((volatile int *)kaddr) = 0;
   before calling follow_pfn
   This way a fault is generated and the PT is
   updated by copying entries from init_mm.

What do you think about presented solutions?

Regards,
Tomasz Stanislawski



On 06/07/2012 04:28 PM, Subash Patel wrote:
> Hello Tomasz,
> 
> On 06/07/2012 06:06 AM, Laurent Pinchart wrote:
>> Hi Tomasz,
>>
>> On Wednesday 06 June 2012 13:56:42 Tomasz Stanislawski wrote:
>>> On 06/06/2012 10:06 AM, Laurent Pinchart wrote:
>>>> 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
>>
>> [snip]
>>
>>>>> +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.
>>
>> That's totally understood :-) I'm fine with keeping the hack for now until the
>> dma_get_sgtable() gets in a usable/mergeable state, please just mention it in
>> the code with something like
>>
>> /* HACK: This is a temporary workaround until the dma_get_sgtable() function
>> becomes available. */
>>
>>> 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).
>>
> The above function call (because of follow_pfn) doesn't succeed for all the allocated pages. Hence I created a patch(attached)
> which is based on [1] series. One can apply it for using your present patch-set in the meantime.
> 
> Regards,
> Subash
> [1] http://www.spinics.net/lists/kernel/msg1343092.html
Laurent Pinchart June 11, 2012, 6:28 a.m. UTC | #2 
Hi Tomasz,

On Friday 08 June 2012 16:31:31 Tomasz Stanislawski wrote:
> Hi Laurent and Subash,
> 
> I confirm the issue found by Subash. The function vb2_dc_kaddr_to_pages does
> fail for some occasions. The failures are rather strange like 'got 95 of
> 150 pages'. It took me some time to find the reason of the problem.
> 
> I found that dma_alloc_coherent for iommu an ARM does use ioremap_page_range
> to map a buffer to the kernel space. The mapping is done by updating the
> page-table.
> 
> The problem is that any process has a different first-level page-table. The
> ioremap_page_range updates only the table for init process. The PT present
> in current->mm shares a majority of entries of 1st-level PT at kernel range
> (above 0xc0000000) but *not all*. That is why vb2_dc_kaddr_to_pages worked
> for small buffers and occasionally failed for larger buffers.
> 
> I found two ways to fix this problem.
> a) use &init_mm instead of current->mm while creating an artificial vma
> b) access the dma memory by calling
>    *((volatile int *)kaddr) = 0;
>    before calling follow_pfn
>    This way a fault is generated and the PT is
>    updated by copying entries from init_mm.
> 
> What do you think about presented solutions?

Just to be sure, this is a hack until dma_get_sgtable is available, and it 
won't make it to mainline, right ?  In that case using init_mm seem easier.
Subash Patel June 11, 2012, 10:38 p.m. UTC | #3 
Hi Laurent, Tomasz,

On 06/10/2012 11:28 PM, Laurent Pinchart wrote:
> Hi Tomasz,
>
> On Friday 08 June 2012 16:31:31 Tomasz Stanislawski wrote:
>> Hi Laurent and Subash,
>>
>> I confirm the issue found by Subash. The function vb2_dc_kaddr_to_pages does
>> fail for some occasions. The failures are rather strange like 'got 95 of
>> 150 pages'. It took me some time to find the reason of the problem.
>>
>> I found that dma_alloc_coherent for iommu an ARM does use ioremap_page_range
>> to map a buffer to the kernel space. The mapping is done by updating the
>> page-table.
>>
>> The problem is that any process has a different first-level page-table. The
>> ioremap_page_range updates only the table for init process. The PT present
>> in current->mm shares a majority of entries of 1st-level PT at kernel range
>> (above 0xc0000000) but *not all*. That is why vb2_dc_kaddr_to_pages worked
>> for small buffers and occasionally failed for larger buffers.
>>
>> I found two ways to fix this problem.
>> a) use&init_mm instead of current->mm while creating an artificial vma
>> b) access the dma memory by calling
>>     *((volatile int *)kaddr) = 0;
>>     before calling follow_pfn
>>     This way a fault is generated and the PT is
>>     updated by copying entries from init_mm.
>>
>> What do you think about presented solutions?
>
> Just to be sure, this is a hack until dma_get_sgtable is available, and it
> won't make it to mainline, right ?  In that case using init_mm seem easier.
Although I agree adding a hack for timebeing, why not use the 
dma_get_sgtable() RFC itself to solve this in clean way? The hacks 
anyways cannot go into mainline when vb2 patches get merged.

Regards,
Subash
>
diff mbox

Patch

From f9b2eace2eef0038a1830e5e6dd55f3bb6017e1a Mon Sep 17 00:00:00 2001
From: Subash Patel <subash.ramaswamy@linaro.org>
Date: Thu, 7 Jun 2012 19:49:10 +0530
Subject: [PATCH] v4l2: vb2: use dma_get_sgtable

This is patch to remove vb2_dc_kaddr_to_pages() function with dma_get_sgtable()
in the patch set posted by Tomasz. It was observed that the former function
fails to get the pages, as follow_pfn() can fail for remapped kernel va provided
by the dma-mapping sub-system.

As Tomasz mentioned, the later call was temporary patch until dma-mapping author
finalizes the implementation of dma_get_sgtable(). One can use this patch to use
this vb2 patch-set for his/her work in the meantime.

Signed-off-by: Subash Patel <subash.ramaswamy@linaro.org>
---
 drivers/media/video/videobuf2-dma-contig.c |   53 ++-------------------------
 1 files changed, 4 insertions(+), 49 deletions(-)

diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index e8da7f1..1b9023a 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -143,32 +143,11 @@  static void vb2_dc_put(void *buf_priv)
 	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) {
-		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)
@@ -183,35 +162,14 @@  static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
 	WARN_ON((unsigned long)buf->vaddr & ~PAGE_MASK);
 	WARN_ON(buf->dma_addr & ~PAGE_MASK);
 
-	n_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+	ret = dma_get_sgtable(dev, &buf->sgt_base, buf->vaddr,
+						buf->dma_addr, size, NULL);
 
-	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;
-	}
-
-	ret = sg_alloc_table_from_pages(&buf->sgt_base,
-		pages, n_pages, 0, size, GFP_KERNEL);
-	if (ret) {
-		dev_err(dev, "failed to prepare sg table\n");
-		goto fail_pages;
+		dev_err(dev, "failed to get the SGT for the allocated pages\n");
+		goto fail_dma;
 	}
 
-	/* pages are no longer needed */
-	kfree(pages);
-
 	buf->dev = dev;
 	buf->size = size;
 
@@ -223,9 +181,6 @@  static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
 
 	return buf;
 
-fail_pages:
-	kfree(pages);
-
 fail_dma:
 	dma_free_coherent(dev, size, buf->vaddr, buf->dma_addr);
 
-- 
1.7.5.4