On 11/20/2015 12:31 AM, Daniel Vetter wrote:
On Thu, Nov 19, 2015 at 04:10:26PM -0800, yu.dai@xxxxxxxxx wrote:
> From: Alex Dai <yu.dai@xxxxxxxxx>
>
> There is a memory leak warning message from i915_gem_context_clean
> when GuC submission is enabled. The reason is that when LRC is
> released, its ppgtt could be still referenced. The assumption that
> all VMAs are unbound during release of LRC is not true.
>
> v1: Move the code inside i915_gem_context_clean() to where ppgtt is
> released because it is not cleaning context anyway but ppgtt.
>
> Signed-off-by: Alex Dai <yu.dai@xxxxxxxxx>
retire__read drops the ctx (and hence ppgtt) reference too early,
resulting in us hitting the WARNING. See the giant thread with Tvrtko,
Chris and me:
http://www.spinics.net/lists/intel-gfx/msg78918.html
Would be great if someone could test the diff I posted in there.
I have to recall my patch because of calling vma_unbind inside
ppgtt_release is not right.
> ---
> drivers/gpu/drm/i915/i915_gem_context.c | 24 ------------------------
> drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++++
> 2 files changed, 12 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 204dc7c..cc5c8e6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -133,23 +133,6 @@ static int get_context_size(struct drm_device *dev)
> return ret;
> }
>
> -static void i915_gem_context_clean(struct intel_context *ctx)
> -{
> - struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
> - struct i915_vma *vma, *next;
> -
> - if (!ppgtt)
> - return;
> -
> - WARN_ON(!list_empty(&ppgtt->base.active_list));
> -
> - list_for_each_entry_safe(vma, next, &ppgtt->base.inactive_list,
> - mm_list) {
> - if (WARN_ON(__i915_vma_unbind_no_wait(vma)))
> - break;
> - }
> -}
> -
> void i915_gem_context_free(struct kref *ctx_ref)
> {
> struct intel_context *ctx = container_of(ctx_ref, typeof(*ctx), ref);
> @@ -159,13 +142,6 @@ void i915_gem_context_free(struct kref *ctx_ref)
> if (i915.enable_execlists)
> intel_lr_context_free(ctx);
>
> - /*
> - * 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);
>
> if (ctx->legacy_hw_ctx.rcs_state)
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 016739e..d36943c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2214,6 +2214,7 @@ void i915_ppgtt_release(struct kref *kref)
> {
> struct i915_hw_ppgtt *ppgtt =
> container_of(kref, struct i915_hw_ppgtt, ref);
> + struct i915_vma *vma, *next;
>
> trace_i915_ppgtt_release(&ppgtt->base);
>
> @@ -2221,6 +2222,17 @@ void i915_ppgtt_release(struct kref *kref)
> WARN_ON(!list_empty(&ppgtt->base.active_list));
> WARN_ON(!list_empty(&ppgtt->base.inactive_list));
>
> + /*
> + * This ppgtt 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.
> + */
> + list_for_each_entry_safe(vma, next, &ppgtt->base.inactive_list,
> + mm_list) {
> + if (WARN_ON(__i915_vma_unbind_no_wait(vma)))
> + break;
> + }
> +
> list_del(&ppgtt->base.global_link);
> drm_mm_takedown(&ppgtt->base.mm);
>
> --
> 2.5.0
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
