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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Nov 12, 2014 at 01:39:05PM +0100, 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.

Oh! That is not good!
> 
> 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.

What type of caching types are there on ARM? We use the 'struct page'
on the set_pages_to_[wc|uc|wb] but all of those are X86 specfic.

But I think you could by passing the 'struct dma_page' instead
of 'struct page' (and the array uses) around. That should solve
the touching of 'struct page' and we can treat it as an opaque
type.
> 
> 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.

Is there a way to query the 'struct device' to see if the IOMMU translation
is enabled/disabled for said device?

Now your patch looks to get the 'struct page' by doing some  form of
translation. Could you explain to me which type of memory have a 'struct page'
and which ones do not ?

It is OK if you explain this in nauseating details :-)

> 
> 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
>  	else {
>  		kfree(d_page);
>  		d_page = NULL;
> -- 
> 2.1.3
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel