Re: [PATCH 55/55] Revert "drm/i915: Clean up associated VMAs on context destruction"

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ma, 2016-07-25 at 18:32 +0100, Chris Wilson wrote:
> This reverts commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae.
> 
> The patch was only a stop-gap measure that fixed half the problem - the
> leak of the fbcon when restarting X. A complete solution required
> releasing the VMA when the object itself was closed rather than rely on
> file/process exit. The previous patches add the VMA tracking necessary
> to do close them along with the object, context or file, and so the time
> has come to remove the partial fix.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

With the improvements in tracking, makes sense.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  5 -----
>  drivers/gpu/drm/i915/i915_gem.c         | 14 ++------------
>  drivers/gpu/drm/i915/i915_gem_context.c | 22 ----------------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c     |  2 +-
>  4 files changed, 3 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c6c64003504f..40033ca30e55 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3044,11 +3044,6 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  		  u32 flags);
>  void __i915_vma_set_map_and_fenceable(struct i915_vma *vma);
>  int __must_check i915_vma_unbind(struct i915_vma *vma);
> -/*
> - * BEWARE: Do not use the function below unless you can _absolutely_
> - * _guarantee_ VMA in question is _not in use_ anywhere.
> - */
> -int __must_check __i915_vma_unbind_no_wait(struct i915_vma *vma);
>  void i915_vma_close(struct i915_vma *vma);
>  void i915_vma_destroy(struct i915_vma *vma);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0d9a80b41101..e3278f4e1ad2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2810,7 +2810,7 @@ static void __i915_vma_iounmap(struct i915_vma *vma)
>  	vma->iomap = NULL;
>  }
>  
> -static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
> +int i915_vma_unbind(struct i915_vma *vma)
>  {
>  	struct drm_i915_gem_object *obj = vma->obj;
>  	unsigned long active;
> @@ -2820,7 +2820,7 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
>  	 * have side-effects such as unpinning or even unbinding this vma.
>  	 */
>  	active = vma->active;
> -	if (active && wait) {
> +	if (active) {
>  		int idx;
>  
>  		/* When a closed VMA is retired, it is unbound - eek.
> @@ -2902,16 +2902,6 @@ destroy:
>  	return 0;
>  }
>  
> -int i915_vma_unbind(struct i915_vma *vma)
> -{
> -	return __i915_vma_unbind(vma, true);
> -}
> -
> -int __i915_vma_unbind_no_wait(struct i915_vma *vma)
> -{
> -	return __i915_vma_unbind(vma, false);
> -}
> -
>  int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_engine_cs *engine;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 1ba6c0bb856a..e9da8aaaa41d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -134,21 +134,6 @@ static int get_context_size(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -static void i915_gem_context_clean(struct i915_gem_context *ctx)
> -{
> -	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
> -	struct i915_vma *vma, *next;
> -
> -	if (!ppgtt)
> -		return;
> -
> -	list_for_each_entry_safe(vma, next, &ppgtt->base.inactive_list,
> -				 vm_link) {
> -		if (WARN_ON(__i915_vma_unbind_no_wait(vma)))
> -			break;
> -	}
> -}
> -
>  void i915_gem_context_free(struct kref *ctx_ref)
>  {
>  	struct i915_gem_context *ctx = container_of(ctx_ref, typeof(*ctx), ref);
> @@ -158,13 +143,6 @@ void i915_gem_context_free(struct kref *ctx_ref)
>  	trace_i915_context_free(ctx);
>  	GEM_BUG_ON(!ctx->closed);
>  
> -	/*
> -	 * This context is going away and we need to remove all VMAs still
> -	 * around. This is to handle imported shared objects for which
> -	 * destructor did not run when their handles were closed.
> -	 */
> -	i915_gem_context_clean(ctx);
> -
>  	i915_ppgtt_put(ctx->ppgtt);
>  
>  	for (i = 0; i < I915_NUM_ENGINES; i++) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index d2130da3de9d..e19a5fd5f15f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3365,7 +3365,7 @@ void i915_vma_close(struct i915_vma *vma)
>  
>  	list_del_init(&vma->obj_link);
>  	if (!i915_vma_is_active(vma) && !vma->pin_count)
> -		WARN_ON(__i915_vma_unbind_no_wait(vma));
> +		WARN_ON(i915_vma_unbind(vma));
>  }
>  
>  static struct i915_vma *
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx