[Intel-gfx] [PATCH] drm/i915: Unwind partial VMA rebinding after failure in set-cache-level

[Chris Wilson]

On Fri, Oct 09, 2015 at 01:11:47PM +0100, Tvrtko Ursulin wrote:
> 
> On 09/10/15 12:51, Chris Wilson wrote:
> >If the impossible happens and we fail to rebind a VMA in the middle of
> >rebinding all VMA for an object we currently bail out and leave the
> >object in an inconsistent state. Attempt to unwind the incomplete update
> >by reverting all updated VMA back to the original cache-level, and WARN
> >if that fails.
> 
> Hey a BUG_ON would have been more your style! ;)
> 
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> >  drivers/gpu/drm/i915/i915_gem.c | 35 ++++++++++++++++++++++++++---------
> >  1 file changed, 26 insertions(+), 9 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 1e67484fd5dc..24ba47a22260 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -3664,6 +3664,12 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  	struct i915_vma *vma, *next;
> >  	int ret = 0;
> >
> >+	/* We manipulate all the PTEs in the various GTT associated with
> >+	 * this object which requires that the caller takes the
> >+	 * struct_mutex on our behalf.
> >+	 */
> >+	lockdep_assert_held(&dev->struct_mutex);
> >+
> >  	if (obj->cache_level == cache_level)
> >  		goto out;
> >
> >@@ -3697,17 +3703,18 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  				return ret;
> >  		}
> >
> >-		list_for_each_entry(vma, &obj->vma_list, vma_link)
> >-			if (drm_mm_node_allocated(&vma->node)) {
> >-				ret = i915_vma_bind(vma, cache_level,
> >-						    PIN_UPDATE);
> >-				if (ret)
> >-					return ret;
> >-			}
> >+		list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >+			if (!drm_mm_node_allocated(&vma->node))
> >+				continue;
> >+
> >+			ret = i915_vma_bind(vma, cache_level, PIN_UPDATE);
> >+			if (ret)
> >+				goto unwind;
> >+
> >+			vma->node.color = cache_level;
> >+		}
> >  	}
> >
> >-	list_for_each_entry(vma, &obj->vma_list, vma_link)
> >-		vma->node.color = cache_level;
> >  	obj->cache_level = cache_level;
> >
> >  out:
> >@@ -3719,6 +3726,16 @@ out:
> >  	}
> >
> >  	return 0;
> >+
> >+unwind:
> >+	list_for_each_entry_continue_reverse(vma, &obj->vma_list, vma_link) {
> 
> I did not know of this one! But it confuses me (emphasis mine):
> 
> """
> list_for_each_entry_continue_reverse - iterate backwards _from the
> given point_
> """
> 
> or
> 
> """
>  * Start to iterate over list of given type backwards, continuing _after
>  * the current position_.
> """
> 
> Code is "for (pos = list_prev_entry(pos, member); " though, so I
> think you'll miss rebinding the one it failed on.

Correct, but we didn't change the one we failed upon. If i915_vma_bind()
itself doesn't unwind, we have another bug to fix! :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre