[Intel-gfx] [PATCH 47/64] drm/i915: Be more careful when unbinding vma

[Chris Wilson]

On Tue, Jul 12, 2016 at 04:04:57PM +0100, Tvrtko Ursulin wrote:
> >@@ -3463,11 +3483,18 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  			return -EBUSY;
> >  		}
> >
> >-		if (!i915_gem_valid_gtt_space(vma, cache_level)) {
> >-			ret = i915_vma_unbind(vma);
> >-			if (ret)
> >-				return ret;
> >-		}
> >+		if (i915_gem_valid_gtt_space(vma, cache_level))
> >+			continue;
> >+
> >+		ret = i915_vma_unbind(vma);
> >+		if (ret)
> >+			return ret;
> >+
> >+		/* As unbinding may affect other elements in the
> >+		 * obj->vma_list (due to side-effects from retiring
> >+		 * an active vma), play safe and restart the iterator.
> >+		 */
> >+		goto restart;
> >  	}
> 
> Does not look efficient for long lists but I don't see a solution
> right now. Any chance of this O(N^2) iteration hurting us in the
> real world?

Defintely not pretty, but couldn't think of a clean & safe alternative,
so went with simple for a change.

Fortunately, it is more or less a one-time conversion, operating on a
shared buffer between normally 2 clients. However, they will full-ppgtt
or we would not have multple vma, and so not affected.

Single client multiple vma case would be partial vma. That could be a
much larger list... But on the afffected machines, such vma would
already be in the correct cache regime and so not need rebinding.

I don't forsee (or have seen) anybody spinning here, so I am quite happy
to punt the problem until someone complains. At least I think I can
write a test case to have lots of partial vma, but not necessarily able
to trigger the restart. That would require careful contrl of
fragmentation with the ggtt... But mixing gtt faults of a bunch of small
objects and different partials of a large should be enough...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre