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

[Chris Wilson]

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);
> +	}

Also, please no. This is how you make synmark angry. If there aren't
enough igt tests to demonstrate that this can hurt badly, I need to add
some more.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre