Re: [RFC] drm/ttm: dma: Fixes for 32-bit and 64-bit ARM

[Alexandre Courbot <acourbot@nvidia.com>]

On 12/08/2014 04:14 PM, Alexandre Courbot wrote:
> On 11/12/2014 09:39 PM, Thierry Reding wrote:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> dma_alloc_coherent() returns a kernel virtual address that is part of
>> the linear range. Passing such an address to virt_to_page() is illegal
>> on non-coherent architectures. This causes the kernel to oops on 64-bit
>> ARM because the struct page * obtained from virt_to_page() points to
>> unmapped memory.
>>
>> This commit fixes this by using phys_to_page() since we get a physical
>> address from dma_alloc_coherent(). Note that this is not a proper fix
>> because if an IOMMU is set up to translate addresses for the GPU this
>> address will be an I/O virtual address rather than a physical one. The
>> proper fix probably involves not getting a pointer to the struct page
>> in the first place, but that would be a much more intrusive change, if
>> at all possible.
>>
>> Until that time, this temporary fix will allow TTM to work on 32-bit
>> and 64-bit ARM as well, provided that no IOMMU translations are enabled
>> for the GPU.
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> ---
>> Arnd, I realize that this isn't a proper fix according to what we
>> discussed on
>> IRC yesterday, but I can't see a way to remove access to the pages
>> array that
>> would be as simple as this. I've marked this as RFC in the hope that
>> it will
>> trigger some discussion that will lead to a proper solution.
>>
>>   drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> index c96db433f8af..d7993985752c 100644
>> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
>> @@ -343,7 +343,11 @@ static struct dma_page
>> *__ttm_dma_alloc_page(struct dma_pool *pool)
>>                          &d_page->dma,
>>                          pool->gfp_flags);
>>       if (d_page->vaddr)
>> +#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
>> +        d_page->p = phys_to_page(d_page->dma);
>> +#else
>>           d_page->p = virt_to_page(d_page->vaddr);
>> +#endif
>
> Since I am messing with the IOMMU I just happened to have hit the issue
> you are mentioning. Wouldn't the following work:
>
> -       if (d_page->vaddr)
> -#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
> -               d_page->p = phys_to_page(d_page->dma);
> -#else
> -               d_page->p = virt_to_page(d_page->vaddr);
> -#endif
> -       else {
> +       if (d_page->vaddr) {
> +               if (is_vmalloc_addr(d_page->vaddr)) {
> +                       d_page->p = vmalloc_to_page(d_page->vaddr);
> +               } else {
> +                       d_page->p = virt_to_page(d_page->vaddr);
> +               }
> +       } else {
>
> A remapped page will end up in the vmalloc range of the address space,
> and in this case we can use vmalloc_to_page() to get the right page.
> Pages outside of this range are part of the linear mapping and can be
> resolved using virt_to_page().
>
> Jetson seems to be mostly happy with this, although I sometimes get the
> following trace:
>
> [   13.174763] kernel BUG at ../mm/slab.c:2593!
> [   13.174767] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
> [   13.174790] Modules linked in: nouveau_platform(O+) nouveau(O)
> cfbfillrect cfbimgblt cfbcopyarea ttm
> ...
> [   13.175234] [<c00de238>] (cache_alloc_refill) from [<c00de528>]
> (__kmalloc+0x100/0x13c)
> [   13.175247] [<c00de528>] (__kmalloc) from [<c001d564>]
> (arm_iommu_alloc_attrs+0x94/0x3a8)
> [   13.175269] [<c001d564>] (arm_iommu_alloc_attrs) from [<bf008f4c>]
> (ttm_dma_populate+0x498/0x76c [ttm])
> [   13.175294] [<bf008f4c>] (ttm_dma_populate [ttm]) from [<bf000bb8>]
> (ttm_tt_bind+0x38/0x68 [ttm])
> [   13.175315] [<bf000bb8>] (ttm_tt_bind [ttm]) from [<bf00298c>]
> (ttm_bo_handle_move_mem+0x408/0x47c [ttm])
> [   13.175337] [<bf00298c>] (ttm_bo_handle_move_mem [ttm]) from
> [<bf003758>] (ttm_bo_validate+0x220/0x22c [ttm])
> [   13.175359] [<bf003758>] (ttm_bo_validate [ttm]) from [<bf003984>]
> (ttm_bo_init+0x220/0x338 [ttm])
> [   13.175480] [<bf003984>] (ttm_bo_init [ttm]) from [<bf0c70a0>]
> (nouveau_bo_new+0x1c0/0x294 [nouveau])
> [   13.175688] [<bf0c70a0>] (nouveau_bo_new [nouveau]) from [<bf0ce88c>]
> (nv84_fence_create+0x1cc/0x240 [nouveau])
> [   13.175891] [<bf0ce88c>] (nv84_fence_create [nouveau]) from
> [<bf0cec90>] (nvc0_fence_create+0xc/0x24 [nouveau])
> [   13.176094] [<bf0cec90>] (nvc0_fence_create [nouveau]) from
> [<bf0c1480>] (nouveau_accel_init+0xec/0x450 [nouveau])
>
> I suspect this is related to this change, but it might also be the
> side-effect of another bug in my code.

Ok, I finally understand where this trace comes from.

slab.c:2593:
BUG_ON(flags & GFP_SLAB_BUG_MASK);

gfp.h:
#define GFP_SLAB_BUG_MASK (__GFP_DMA32|__GFP_HIGHMEM|~__GFP_BITS_MASK)

and ttm_dma_populate() calls ttm_dma_pool_init() with gfp_flags == 
GFP_HIGHUSER, which includes __GFP_HIGHMEM. Somehow these flags are 
propagated through arm_iommu_alloc_attrs() up to the slab allocator, 
which meets this BUG_ON() when it needs to grow its cache.

Note that ttm_dma_pool_init() can also be called with GFP_DMA32, which 
will trigger the same error.

So although I am still not sure how this should be fixed, it seems like 
this has nothing to do with the change I am proposing.

Temporary workaround:

--- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
+++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
@@ -882,9 +882,9 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, 
struct device *dev)

         type = ttm_to_type(ttm->page_flags, ttm->caching_state);
         if (ttm->page_flags & TTM_PAGE_FLAG_DMA32)
-               gfp_flags = GFP_USER | GFP_DMA32;
+               gfp_flags = GFP_USER;
         else
-               gfp_flags = GFP_HIGHUSER;
+               gfp_flags = GFP_USER;
         if (ttm->page_flags & TTM_PAGE_FLAG_ZERO_ALLOC)
                 gfp_flags |= __GFP_ZERO;

... which obviously doesn't look right.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel