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@xxxxxxxxx wrote:
> > From: Armin Reese <armin.c.reese@xxxxxxxxx>
> >
> > Signed-off-by: Armin Reese <armin.c.reese@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxx>
>
> --
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
