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

[Alexandre Courbot]

On Mon, Dec 8, 2014 at 4:14 PM, Alexandre Courbot <acourbot at nvidia.com> wrote:
> On 11/12/2014 09:39 PM, Thierry Reding wrote:
>>
>> From: Thierry Reding <treding at 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 at 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().

Thierry, have you had a chance to try this? If not, do you want to to
try to push this patch? It seems to solve the issue AFAICT, but needs
more testing.