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

[Tomasz Stanislawski <t.stanislaws@samsung.com>]

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