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

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

[armin.c.reese]

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
-- 
1.9.1

[PATCH] drm/i915: Optimize the i915_gem_gtt_finish_object function

[armin.c.reese]

From: Armin Reese <armin.c.reese@intel.com>

Signed-off-by: Armin Reese <armin.c.reese@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index afd4eef..7e2190e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1624,18 +1624,17 @@ static void ggtt_unbind_vma(struct i915_vma *vma)
 
 void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
 {
-	struct drm_device *dev = obj->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	bool interruptible;
-
-	interruptible = do_idling(dev_priv);
+	if (!obj->has_dma_mapping) {
+		struct drm_device *dev = obj->base.dev;
+		struct drm_i915_private *dev_priv = dev->dev_private;
+		bool interruptible = do_idling(dev_priv);
 
-	if (!obj->has_dma_mapping)
 		dma_unmap_sg(&dev->pdev->dev,
 			     obj->pages->sgl, obj->pages->nents,
 			     PCI_DMA_BIDIRECTIONAL);
 
-	undo_idling(dev_priv, interruptible);
+		undo_idling(dev_priv, interruptible);
+	}
 }
 
 static void i915_gtt_color_adjust(struct drm_mm_node *node,
-- 
1.9.1

Re: [PATCH] drm/i915: Optimize the i915_gem_gtt_finish_object function

[Ben Widawsky]

On Fri, Jul 11, 2014 at 10:20:08AM -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_gtt.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index afd4eef..7e2190e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1624,18 +1624,17 @@ static void ggtt_unbind_vma(struct i915_vma *vma)
>  
>  void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
>  {
> -	struct drm_device *dev = obj->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	bool interruptible;
> -
> -	interruptible = do_idling(dev_priv);
> +	if (!obj->has_dma_mapping) {
> +		struct drm_device *dev = obj->base.dev;
> +		struct drm_i915_private *dev_priv = dev->dev_private;
> +		bool interruptible = do_idling(dev_priv);
>  
> -	if (!obj->has_dma_mapping)
>  		dma_unmap_sg(&dev->pdev->dev,
>  			     obj->pages->sgl, obj->pages->nents,
>  			     PCI_DMA_BIDIRECTIONAL);
>  
> -	undo_idling(dev_priv, interruptible);
> +		undo_idling(dev_priv, interruptible);
> +	}
>  }
>  
>  static void i915_gtt_color_adjust(struct drm_mm_node *node,

Note that this doesn't do much on platforms you care about.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Optimize the i915_gem_gtt_finish_object function

[Daniel Vetter]

On Fri, Jul 18, 2014 at 1:16 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Fri, Jul 11, 2014 at 10:20:08AM -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_gtt.c | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index afd4eef..7e2190e 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -1624,18 +1624,17 @@ static void ggtt_unbind_vma(struct i915_vma *vma)
>>
>>  void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj)
>>  {
>> -     struct drm_device *dev = obj->base.dev;
>> -     struct drm_i915_private *dev_priv = dev->dev_private;
>> -     bool interruptible;
>> -
>> -     interruptible = do_idling(dev_priv);
>> +     if (!obj->has_dma_mapping) {
>> +             struct drm_device *dev = obj->base.dev;
>> +             struct drm_i915_private *dev_priv = dev->dev_private;
>> +             bool interruptible = do_idling(dev_priv);
>>
>> -     if (!obj->has_dma_mapping)
>>               dma_unmap_sg(&dev->pdev->dev,
>>                            obj->pages->sgl, obj->pages->nents,
>>                            PCI_DMA_BIDIRECTIONAL);
>>
>> -     undo_idling(dev_priv, interruptible);
>> +             undo_idling(dev_priv, interruptible);
>> +     }
>>  }
>>
>>  static void i915_gtt_color_adjust(struct drm_mm_node *node,
>
> Note that this doesn't do much on platforms you care about.

To elaborate: This w/a is _only_ used on ironlake when the iommu is
enabled. Also the performance impact of it is so disastrous that any
kind of optimization is pointless.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [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@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>

-- 
Ben Widawsky, Intel Open Source Technology Center

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

[Ben Widawsky]

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.

> 
-- 
Ben Widawsky, Intel Open Source Technology Center

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

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[armin.c.reese]

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
-- 
1.9.1

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

[Chris Wilson]

On Fri, Jul 11, 2014 at 10:15:35AM -0700, armin.c.reese@intel.com wrote:
> From: Armin Reese <armin.c.reese@intel.com>

If you can a description of how to trigger the bug and the user impact
would be great. And Daniel is about to ask for a test case in 3
seconds...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Reese, Armin C]

No bugs have been reported so far ...
Daniel caught this by inspection of the Full PPGTT patch set and wanted me to fix it.
- Armin

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Friday, July 11, 2014 10:21 AM
To: Reese, Armin C
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Do not unmap object unless no other VMAs reference it

On Fri, Jul 11, 2014 at 10:15:35AM -0700, armin.c.reese@intel.com wrote:
> From: Armin Reese <armin.c.reese@intel.com>

If you can a description of how to trigger the bug and the user impact would be great. And Daniel is about to ask for a test case in 3 seconds...
-Chris

--
Chris Wilson, Intel Open Source Technology Centre