| Message ID | 1405099208-4085-1-git-send-email-armin.c.reese@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 11, 2014 at 10:20:07AM -0700, armin.c.reese@intel.com wrote: > From: Armin Reese <armin.c.reese@intel.com> > > Signed-off-by: Armin Reese <armin.c.reese@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@bwidawsk.net>
On Thu, Jul 17, 2014 at 04:45:02PM -0700, Ben Widawsky wrote: > On Fri, Jul 11, 2014 at 10:20:07AM -0700, armin.c.reese@intel.com wrote: > > From: Armin Reese <armin.c.reese@intel.com> > > > > Signed-off-by: Armin Reese <armin.c.reese@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. > " Crap. I left out the important part. Note to backporters trying to backport full PPGTT, which is disabled by default on all kernels to date. >
On Thu, Jul 17, 2014 at 04:45:02PM -0700, Ben Widawsky wrote: > On Fri, Jul 11, 2014 at 10:20:07AM -0700, armin.c.reese@intel.com wrote: > > From: Armin Reese <armin.c.reese@intel.com> > > > > Signed-off-by: Armin Reese <armin.c.reese@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. Indeed, and thanks a lot for providing the commit message. I've added "for full ppgtt" as requested and merged the patch to dinq. Thanks, Daniel > " > 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@bwidawsk.net> > > -- > Ben Widawsky, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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