[Intel-gfx] [PATCH] drm/i915: Always move bo onto global unbound list after final unbind

[Chris Wilson]

At first glance, it seems reasonable that a currently unbound bo during
i915_vma_unbind() could only arrive there by an interrupted execbuffer
that never successfully bound. However, inserting a BUG, viz

diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c
index b52c38d63914..8e52ac771a52 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2858,6 +2858,7 @@ int i915_vma_unbind(struct i915_vma *vma)

        if (!drm_mm_node_allocated(&vma->node)) {
                i915_gem_vma_destroy(vma);
+               BUG_ON(!list_empty(&obj->vma_list));
                return 0;
        }

quickly disproves that notion. So as it is possible then to have an
unallocated bo on the global bound list, during the final unbind we must
always check to see if we need to move the bo back to the global unbound
list.

We are left with two choices to do so. Either we continue to extend the
special case early return for an unallocated vma->node, or we almagamate
the normal exit for the two paths. The latter does unfortunately
slightly break the neat setup/teardown symmetry established by

commit 70903c3ba8fa5ad391d1519c60666a389e4be597
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date:   Wed Dec 4 09:59:09 2013 +0000

    drm/i915: Fix ordering of unbind vs unpin pages

but on the other hand it reduces the complexity of the code.

Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Cc: Ben Widawsky <ben at bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b52c38d63914..4dbd2affdac1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2856,10 +2856,8 @@ int i915_vma_unbind(struct i915_vma *vma)
 	if (list_empty(&vma->vma_link))
 		return 0;
 
-	if (!drm_mm_node_allocated(&vma->node)) {
-		i915_gem_vma_destroy(vma);
-		return 0;
-	}
+	if (!drm_mm_node_allocated(&vma->node))
+		goto out;
 
 	if (vma->pin_count)
 		return -EBUSY;
@@ -2892,12 +2890,6 @@ int i915_vma_unbind(struct i915_vma *vma)
 		obj->map_and_fenceable = false;
 
 	drm_mm_remove_node(&vma->node);
-	i915_gem_vma_destroy(vma);
-
-	/* Since the unbound list is global, only move to that list if
-	 * no more VMAs exist. */
-	if (list_empty(&obj->vma_list))
-		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
@@ -2905,6 +2897,14 @@ int i915_vma_unbind(struct i915_vma *vma)
 	 */
 	i915_gem_object_unpin_pages(obj);
 
+out:
+	i915_gem_vma_destroy(vma);
+
+	/* Since the unbound list is global, only move to that list if
+	 * no more VMAs exist. */
+	if (list_empty(&obj->vma_list))
+		list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
+
 	return 0;
 }
 
-- 
1.8.5.2