Commit 9490edb5 authored by Armin Reese's avatar Armin Reese Committed by Daniel Vetter

drm/i915: Do not unmap object unless no other VMAs reference it

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 trying to backport full PPGTT:

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.
Signed-off-by: default avatarArmin Reese <armin.c.reese@intel.com>
[danvet: Add the excellent commit message provided by Ben.]
Reviewed-by: default avatarBen Widawsky <ben@bwidawsk.net>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
parent c17c654d
...@@ -2927,8 +2927,6 @@ int i915_vma_unbind(struct i915_vma *vma) ...@@ -2927,8 +2927,6 @@ int i915_vma_unbind(struct i915_vma *vma)
vma->unbind_vma(vma); vma->unbind_vma(vma);
i915_gem_gtt_finish_object(obj);
list_del_init(&vma->mm_list); list_del_init(&vma->mm_list);
/* Avoid an unnecessary call to unbind on rebind. */ /* Avoid an unnecessary call to unbind on rebind. */
if (i915_is_ggtt(vma->vm)) if (i915_is_ggtt(vma->vm))
...@@ -2939,8 +2937,10 @@ int i915_vma_unbind(struct i915_vma *vma) ...@@ -2939,8 +2937,10 @@ int i915_vma_unbind(struct i915_vma *vma)
/* Since the unbound list is global, only move to that list if /* Since the unbound list is global, only move to that list if
* no more VMAs exist. */ * 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); list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
}
/* And finally now the object is completely decoupled from this vma, /* 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 * we can drop its hold on the backing storage and allow it to be
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment