[Intel-gfx] [PATCH v1] drm/i915: Fix a false alert of memory leak when free LRC

[Yu Dai]

On 11/20/2015 12:31 AM, Daniel Vetter wrote:
> On Thu, Nov 19, 2015 at 04:10:26PM -0800, yu.dai at intel.com wrote:
> > From: Alex Dai <yu.dai at intel.com>
> >
> > 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 at intel.com>
>
> 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
> >
>