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

Joonas Lahtinen
2016-07-27T10:18:16Z

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