Re: [PATCH] drm/i915: Fix ordering of unbind vs unpin pages

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Dec 04, 2013 at 09:59:09AM +0000, Chris Wilson wrote:
> It is useful to assert that if the object is bound, then it must have
> its pages pinned to prevent the shrinker from reaping its backing store.
> This is even more useful with the introduction of real-ppgtt whereupon
> we may have the object bound into several vma, with each instance
> pinning the backing store. This assertion breaks down during unbind
> where we unpinned the backing store before decoupling the vma binding.
> This can be fixed with a trivial reording of the unbind sequence, which
> reinforces the
> 
>    pin pages
>    bind to vma
>    ...
>    unbind from vma
>    unpin pages
> 
> concept.
> 
> v2: Bonus comment
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> Tested-by: Ben Widawsky <ben@bwidawsk.net>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6312d61f5198..9b805f0e8387 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2755,7 +2755,6 @@ int i915_vma_unbind(struct i915_vma *vma)
>  	vma->unbind_vma(vma);
>  
>  	i915_gem_gtt_finish_object(obj);
> -	i915_gem_object_unpin_pages(obj);
>  
>  	list_del(&vma->mm_list);
>  	/* Avoid an unnecessary call to unbind on rebind. */
> @@ -2763,7 +2762,6 @@ int i915_vma_unbind(struct i915_vma *vma)
>  		obj->map_and_fenceable = true;
>  
>  	drm_mm_remove_node(&vma->node);
> -
>  	i915_gem_vma_destroy(vma);
>  
>  	/* Since the unbound list is global, only move to that list if
> @@ -2771,6 +2769,12 @@ int i915_vma_unbind(struct i915_vma *vma)
>  	if (list_empty(&obj->vma_list))
>  		list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
>  
> +	/* And finally now the object is completely decoupled from this vma,
> +	 * we can drop its hold on the backing storage and allow it to be
> +	 * reaped by the shrinker.
> +	 */
> +	i915_gem_object_unpin_pages(obj);
> +
>  	return 0;
>  }
>  
> -- 
> 1.8.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch