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

[Tvrtko Ursulin]

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.

> Also, checking for the view type is some strange layering. Why that?

!ppgtt to skip potential other views I thought, no?

Regards,

Tvrtko