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

[Tvrtko Ursulin]

On 12/07/16 17:42, Chris Wilson wrote:
> 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...

Or object sharing. But yeah, I agree that it feels OK to leave that for 
later if it ever surfaces.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko