[Intel-gfx] [PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma

[Chris Wilson]

Quoting Mika Kuoppala (2018-08-16 13:14:37)
> Chris Wilson <chris at chris-wilson.co.uk> writes:
> 
> > Quoting Mika Kuoppala (2018-08-16 12:42:08)
> >> Chris Wilson <chris at chris-wilson.co.uk> writes:
> >> 
> >> > The context owns both the ppgtt and the vma within it, and our activity
> >> > tracking on the context ensures that we do not release active ppgtt. As
> >> > the context fulfils our obligations for active memory tracking, we can
> >> > relinquish the reference from the vma.
> >> >
> >> 
> >> The part of owning the vma within it escapes me. The vma is tied
> >> to the object. Is it about that active objects, with their vmas
> >> hold the refs to the context they are executing on?
> >
> > The vma belongs to the (object, context). If the context is closed (or
> > on close(/dev/dri/card0)), so is the ppgtt. Closing the object
> > (gem_close(fd, obj)) closes the vma that match the contexts for the fd,
> > and if it was the last handle, then frees the object.
> >  
> >> > This fixes a silly transient refleak from closed vma being kept alive
> >> > until the entire system was idle, keeping all vm alive as well.
> >> >
> >> > Reported-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >> > Testcase: igt/gem_ctx_create/files
> >> > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
> >> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_vma.c | 4 ----
> >> >  1 file changed, 4 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> >> > index 274fd2a7bcb6..31efc971a3a8 100644
> >> > --- a/drivers/gpu/drm/i915/i915_vma.c
> >> > +++ b/drivers/gpu/drm/i915/i915_vma.c
> >> > @@ -199,7 +199,6 @@ vma_create(struct drm_i915_gem_object *obj,
> >> >               vma->flags |= I915_VMA_GGTT;
> >> >               list_add(&vma->obj_link, &obj->vma_list);
> >> >       } else {
> >> > -             i915_ppgtt_get(i915_vm_to_ppgtt(vm));
> >> 
> >> It seems this is the sole user of i915_ppgtt_get so you can
> >> remove that too.
> >
> > Such shortsightedness; it will reappear shortly. See the patches to
> > share vm between contexts.
> 
> Ok let it float in there for a while.
> 
> Reviewed-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> 
> Also
> 
> Tested-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>

Applied the bug fix, thank you for the review.
-Chris