[Intel-gfx] [PATCH] drm/i915: Move VMAs to inactive as request are retired

[Tvrtko Ursulin]

On 26/11/15 10:01, Daniel Vetter wrote:
> On Wed, Nov 25, 2015 at 10:16:37AM +0000, Tvrtko Ursulin wrote:
>>
>> On 24/11/15 17:47, Daniel Vetter wrote:
>>> On Mon, Nov 23, 2015 at 03:12:35PM +0000, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> Current code moves _any_ VMA to the inactive list only when
>>>> _all_ rendering on an object (so from any context or VM) has
>>>> been completed.
>>>>
>>>> This creates an un-natural situation where the context (and
>>>> VM) destructors can run with VMAs still on the respective
>>>> active list.
>>>>
>>>> Change here is to move VMAs to the inactive list as the
>>>> requests are getting retired.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92638
>>>> Testcase: igt/gem_request_retire/retire-vma-not-inactive
>>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>>> ---
>>>>   drivers/gpu/drm/i915/i915_gem.c | 17 ++++++++++++++++-
>>>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>> index cd7e102720f4..47a743246d2c 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -2413,17 +2413,32 @@ static void
>>>>   i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
>>>>   {
>>>>   	struct i915_vma *vma;
>>>> +	struct i915_address_space *vm;
>>>>
>>>>   	RQ_BUG_ON(obj->last_read_req[ring] == NULL);
>>>>   	RQ_BUG_ON(!(obj->active & (1 << ring)));
>>>>
>>>>   	list_del_init(&obj->ring_list[ring]);
>>>> -	i915_gem_request_assign(&obj->last_read_req[ring], NULL);
>>>>
>>>>   	if (obj->last_write_req && obj->last_write_req->ring->id == ring)
>>>>   		i915_gem_object_retire__write(obj);
>>>>
>>>>   	obj->active &= ~(1 << ring);
>>>> +
>>>> +	if (obj->last_read_req[ring]->ctx->ppgtt)
>>>> +		vm = &obj->last_read_req[ring]->ctx->ppgtt->base;
>>>> +	else
>>>> +		vm = &obj->last_read_req[ring]->i915->gtt.base;
>>>> +
>>>> +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>>>> +		if (vma->vm == vm &&
>>>> +		    vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL &&
>>>> +		    !list_empty(&vma->mm_list))
>>>> +			list_move_tail(&vma->mm_list, &vma->vm->inactive_list);
>>>> +	}
>>>
>>> This is only a partial solution since with schedulers and semaphores and a
>>> few depencies on a given object, but in different vm you can still end up
>>> with an object that is idle in a vm, but slipped through here.
>>
>> Could you describe the exact scenario you had in mind? I won't pretend it
>> this is simple code and I have it all figured out.
>
> Well tbh I don't fully understand what exactly your code will do in all
> corner-cases since there's a lot of ifs and special cases. But
> fundamentally the problem is that an object can be active in a given vm
> and there's no request pointing at the corresponding context in either
> last_read_req or last_write_req. It works like this:
>
> - ctx A uses obj 1
> - ctx B uses obj 1 on the same engine
>
> Your code above will miss to retire obj 1 on ctx 1's vm, so if you then
> destroy ctx A you'll hit the nice warn above (presuming ctx B keeps obj 1
> busy for all that time). So not even scheduler needed.

Ah yes... I was even close to figuring this one out but got too confused 
and decided I am imagining things...

> Fundamentally if you really want to accurately keep track of vma instaed
> of obj activeness, then you need to have per-vma tracking of all of it,
> i.e. all the last_*_req stuff.
>
> Without that we essentially only keep track of an lru on each vm, and the
> active/inactive split is totally bogus (well not quite, but since it
> reflects obj->active there's not really any value, but it does create tons
> of confusion).
>
> To unconfuse this we'd need to have proper vma active tracking (not sure
> whether that's worth it) or just merge the per-vm active/inactive lists
> since they're really just one big list.

... so all in all best to drop this for now. Since it is a half 
solution, plus Chris says Synmark, if I read it right, likes to use an 
object from a zillion of contexts simultaneously so would be hurt by 
traversing the obj->vma_list here.

>>> Also, checking for the view type is some strange layering. Why that?
>>
>> !ppgtt to skip potential other views I thought, no?
>
> The vma lrus should be irrespective of ggtt vs. ppgtt or exactly what kind
> of view it is. If we exclude special case everywhere the might pop up but
> should the code will become unreadable (and the abstraction useless). What
> do you fear could blow up without this check?

Nothing really, since other view types will never end up on the active 
list. Just a never ending dilemma of whether to be generic or optimal. :)

Regards,

Tvrtko