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@xxxxxxxxxxxxxxxxxx>
> >---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
