[PATCH 3/3] drm/i915: Remove the vma from the drm_mm if binding fails

[PATCH 3/3] drm/i915: Remove the vma from the drm_mm if binding fails

[Chris Wilson]

As we track whether a vma has been inserted into the drm_mm using the
vma->flags, if we fail to bind the vma into the GTT we do not update
those bits and will attempt to reinsert the vma into the drm_mm on
future passes. To prevent that, we want to unwind i915_vma_insert() if
we fail in our attempt to bind.

Fixes: 59bfa1248e22 ("drm/i915: Start passing around i915_vma from execbuffer")
Testcase: igt/drv_selftest/live_gtt
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.9+
---
 drivers/gpu/drm/i915/i915_vma.c | 54 ++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 6e9eade304b8..59dd53b1240a 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -509,6 +509,31 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 	return ret;
 }
 
+static void
+i915_vma_remove(struct i915_vma *vma)
+{
+	struct drm_i915_gem_object *obj = vma->obj;
+
+	GEM_BUG_ON(!drm_mm_node_allocated(&vma->node));
+	GEM_BUG_ON(vma->flags & (I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND));
+
+	drm_mm_remove_node(&vma->node);
+	list_move_tail(&vma->vm_link, &vma->vm->unbound_list);
+
+	/* Since the unbound list is global, only move to that list if
+	 * no more VMAs exist. */
+	if (--obj->bind_count == 0)
+		list_move_tail(&obj->global_link,
+			       &to_i915(obj->base.dev)->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
+	 * reaped by the shrinker.
+	 */
+	i915_gem_object_unpin_pages(obj);
+	GEM_BUG_ON(atomic_read(&obj->mm.pages_pin_count) < obj->bind_count);
+}
+
 int __i915_vma_do_pin(struct i915_vma *vma,
 		      u64 size, u64 alignment, u64 flags)
 {
@@ -521,18 +546,18 @@ int __i915_vma_do_pin(struct i915_vma *vma,
 
 	if (WARN_ON(bound & I915_VMA_PIN_OVERFLOW)) {
 		ret = -EBUSY;
-		goto err;
+		goto err_unpin;
 	}
 
 	if ((bound & I915_VMA_BIND_MASK) == 0) {
 		ret = i915_vma_insert(vma, size, alignment, flags);
 		if (ret)
-			goto err;
+			goto err_unpin;
 	}
 
 	ret = i915_vma_bind(vma, vma->obj->cache_level, flags);
 	if (ret)
-		goto err;
+		goto err_remove;
 
 	if ((bound ^ vma->flags) & I915_VMA_GLOBAL_BIND)
 		__i915_vma_set_map_and_fenceable(vma);
@@ -541,7 +566,12 @@ int __i915_vma_do_pin(struct i915_vma *vma,
 	GEM_BUG_ON(i915_vma_misplaced(vma, size, alignment, flags));
 	return 0;
 
-err:
+err_remove:
+	if ((vma->flags & I915_VMA_BIND_MASK) == 0) {
+		GEM_BUG_ON(vma->pages);
+		i915_vma_remove(vma);
+	}
+err_unpin:
 	__i915_vma_unpin(vma);
 	return ret;
 }
@@ -654,9 +684,6 @@ int i915_vma_unbind(struct i915_vma *vma)
 	}
 	vma->flags &= ~(I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND);
 
-	drm_mm_remove_node(&vma->node);
-	list_move_tail(&vma->vm_link, &vma->vm->unbound_list);
-
 	if (vma->pages != obj->mm.pages) {
 		GEM_BUG_ON(!vma->pages);
 		sg_free_table(vma->pages);
@@ -664,18 +691,7 @@ int i915_vma_unbind(struct i915_vma *vma)
 	}
 	vma->pages = NULL;
 
-	/* Since the unbound list is global, only move to that list if
-	 * no more VMAs exist. */
-	if (--obj->bind_count == 0)
-		list_move_tail(&obj->global_link,
-			       &to_i915(obj->base.dev)->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
-	 * reaped by the shrinker.
-	 */
-	i915_gem_object_unpin_pages(obj);
-	GEM_BUG_ON(atomic_read(&obj->mm.pages_pin_count) < obj->bind_count);
+	i915_vma_remove(vma);
 
 destroy:
 	if (unlikely(i915_vma_is_closed(vma)))
-- 
2.11.0

Re: [PATCH 3/3] drm/i915: Remove the vma from the drm_mm if binding fails

[Joonas Lahtinen]

On la, 2017-02-25 at 23:25 +0000, Chris Wilson wrote:
> As we track whether a vma has been inserted into the drm_mm using the
> vma->flags, if we fail to bind the vma into the GTT we do not update
> those bits and will attempt to reinsert the vma into the drm_mm on
> future passes. To prevent that, we want to unwind i915_vma_insert() if
> we fail in our attempt to bind.
> 
> Fixes: 59bfa1248e22 ("drm/i915: Start passing around i915_vma from execbuffer")
> Testcase: igt/drv_selftest/live_gtt
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.9+

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

One note below.

> @@ -541,7 +566,12 @@ int __i915_vma_do_pin(struct i915_vma *vma,
>  	GEM_BUG_ON(i915_vma_misplaced(vma, size, alignment, flags));
>  	return 0;
>  
> -err:
> +err_remove:
> +	if ((vma->flags & I915_VMA_BIND_MASK) == 0) {

This condition could be more symmetric.

Regards, Joonas

> +		GEM_BUG_ON(vma->pages);
> +		i915_vma_remove(vma);
> +	}
> +err_unpin:
>  	__i915_vma_unpin(vma);
>  	return ret;
>  }

-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH 3/3] drm/i915: Remove the vma from the drm_mm if binding fails

[Chris Wilson]

On Mon, Feb 27, 2017 at 02:02:00PM +0200, Joonas Lahtinen wrote:
> On la, 2017-02-25 at 23:25 +0000, Chris Wilson wrote:
> > As we track whether a vma has been inserted into the drm_mm using the
> > vma->flags, if we fail to bind the vma into the GTT we do not update
> > those bits and will attempt to reinsert the vma into the drm_mm on
> > future passes. To prevent that, we want to unwind i915_vma_insert() if
> > we fail in our attempt to bind.
> > 
> > Fixes: 59bfa1248e22 ("drm/i915: Start passing around i915_vma from execbuffer")
> > Testcase: igt/drv_selftest/live_gtt
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: <stable@vger.kernel.org> # v4.9+
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> One note below.
> 
> > @@ -541,7 +566,12 @@ int __i915_vma_do_pin(struct i915_vma *vma,
> > �	GEM_BUG_ON(i915_vma_misplaced(vma, size, alignment, flags));
> > �	return 0;
> > �
> > -err:
> > +err_remove:
> > +	if ((vma->flags & I915_VMA_BIND_MASK) == 0) {
> 
> This condition could be more symmetric.

Done: if ((bound & I915_VMA_BIND_MASK) == 0) and made bound const to be
clear.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre