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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@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.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx