[Intel-gfx] [PATCH] drm/i915: Do not unmap object unless no other VMAs reference it

[Ben Widawsky]

On Fri, Jul 11, 2014 at 10:20:07AM -0700, armin.c.reese at intel.com wrote:
> From: Armin Reese <armin.c.reese at intel.com>
> 
> Signed-off-by: Armin Reese <armin.c.reese at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 87d0aac..676e5f4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2922,8 +2922,6 @@ int i915_vma_unbind(struct i915_vma *vma)
>  
>  	vma->unbind_vma(vma);
>  
> -	i915_gem_gtt_finish_object(obj);
> -
>  	list_del_init(&vma->mm_list);
>  	/* Avoid an unnecessary call to unbind on rebind. */
>  	if (i915_is_ggtt(vma->vm))
> @@ -2934,8 +2932,10 @@ int i915_vma_unbind(struct i915_vma *vma)
>  
>  	/* Since the unbound list is global, only move to that list if
>  	 * no more VMAs exist. */
> -	if (list_empty(&obj->vma_list))
> +	if (list_empty(&obj->vma_list)) {
> +		i915_gem_gtt_finish_object(obj);
>  		list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
> +	}
>  
>  	/* And finally now the object is completely decoupled from this vma,
>  	 * we can drop its hold on the backing storage and allow it to be



This patch is a fix, and therefore needs a commit message to explain the
bug that you're trying to fix. I will help.
"
When using an IOMMU, GEM objects are mapped by their DMA address as the
physical address is unknown. This depends on the underlying IOMMU
driver to map and unmap the physical pages properly as defined in
intel_iommu.c.

The current code will tell the IOMMU to unmap the GEM BO's pages on the
destruction of the first VMA that "maps" that BO. This is clearly wrong
as there may be other VMAs "mapping" that BO (using flink). The scanout
is one such example.

The patch fixes this issue by only unmapping the DMA maps when there are
no more VMAs mapping that object. This is equivalent to when an object
is considered unbound as can be seen by the code. On the first VMA that
again because bound, we will remap.

An alternate solution would be to move the dma mapping to object
creation and destrubtion. I am not sure if this is considered an
unfriendly thing to do.

Some notes to backporters:
The bug can never be hit without enabling the IOMMU. The existing code
will also do the right thing when the object is shared via dmabuf. The
failure should be demonstrable with flink. In cases when not using
intel_iommu_strict it is likely (likely, as defined by: off the top of
my head) on current workloads to *not* hit this bug since we often
teardown all VMAs for an object shared across multiple VMs.  We also
finish access to that object before the first dma_unmapping.
intel_iommu_strict with flinked buffers is likely to hit this issue.
"

As a side note:
I was expecting this to be fixed as part of Daniels "lifetime tracking"
plans. I had imagined that would have a handler run when refcount hits 0
that can do the dma_unmap. Therefore I assume everything in that
list_empty() condition is a temporary hack.


This patch is:
Reviewed-by: Ben Widawsky <ben at bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center