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

[Mika Kuoppala]

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?

-Mika