[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>
> 
> I also been after this one previously and managed to bring
> up a testcase where we create a mapping to an object and
> then create context, do execbuf with mapped object as relocatable,
> and destroy context. That would also end up keeping ppgtts
> alive until client closes.
> 
> Would it be worthwhile to include such test or do you think
> gem_ctx_create at files covers enough?

There's no linkage between the mmap and context, so it doesn't strike me
as novel. gem_exec_reloc (and friends) include passing mmap(obj) as the
execbuf, objects and/or relocations, so that should be covered; and
gem_ctx_create covers using lots of temporary and long lived contexts.

We put previous bugs in gem_ppgtt, e.g. the vma-leak, but part of the
challenge there is that to stabilise the counters you need an idle
system.

However, bug discovery is serendipitous and minor differences between
similar tests (even doing the same setup in a different order) can be
all the difference between finding a bug and not. So don't let me stop
you...

You can use the igt_kcov tools to tell if a test case is similar to any
existing one.
-Chris