Re: [Intel-gfx] [PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma
Quoting Mika Kuoppala (2018-08-16 13:14:37)
> Chris Wilson writes:
>
> > Quoting Mika Kuoppala (2018-08-16 12:42:08)
> >> Chris Wilson 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
> >> > Testcase: igt/gem_ctx_create/files
> >> > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
> >> > Signed-off-by: Chris Wilson
> >> > Cc: Tvrtko Ursulin
> >> > Cc: Paulo Zanoni
> >> > ---
> >> > 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
>
> Also
>
> Tested-by: Mika Kuoppala
Applied the bug fix, thank you for the review.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma
Quoting Mika Kuoppala (2018-08-16 13:14:37)
> Chris Wilson writes:
>
> > Quoting Mika Kuoppala (2018-08-16 12:42:08)
> >> Chris Wilson 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
> >> > Testcase: igt/gem_ctx_create/files
> >> > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
> >> > Signed-off-by: Chris Wilson
> >> > Cc: Tvrtko Ursulin
> >> > Cc: Paulo Zanoni
> >> > ---
> >> > 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
>
> Also
>
> Tested-by: Mika Kuoppala
>
> 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@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma
Chris Wilson writes:
> Quoting Mika Kuoppala (2018-08-16 12:42:08)
>> Chris Wilson 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
>> > Testcase: igt/gem_ctx_create/files
>> > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
>> > Signed-off-by: Chris Wilson
>> > Cc: Tvrtko Ursulin
>> > Cc: Paulo Zanoni
>> > ---
>> > 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
Also
Tested-by: Mika Kuoppala
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@files covers enough?
-Mika
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma
Quoting Mika Kuoppala (2018-08-16 12:42:08)
> Chris Wilson 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
> > Testcase: igt/gem_ctx_create/files
> > Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
> > Signed-off-by: Chris Wilson
> > Cc: Tvrtko Ursulin
> > Cc: Paulo Zanoni
> > ---
> > 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.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma
Chris Wilson 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?
> 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
> Testcase: igt/gem_ctx_create/files
> Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
> Signed-off-by: Chris Wilson
> Cc: Tvrtko Ursulin
> Cc: Paulo Zanoni
> ---
> 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.
-Mika
> list_add_tail(&vma->obj_link, &obj->vma_list);
> }
>
> @@ -810,9 +809,6 @@ static void __i915_vma_destroy(struct i915_vma *vma)
> if (vma->obj)
> rb_erase(&vma->obj_node, &vma->obj->vma_tree);
>
> - if (!i915_vma_is_ggtt(vma))
> - i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm));
> -
> rbtree_postorder_for_each_entry_safe(iter, n, &vma->active, node) {
> GEM_BUG_ON(i915_gem_active_isset(&iter->base));
> kfree(iter);
> --
> 2.18.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: Stop holding a ref to the ppgtt from each vma
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.
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
Testcase: igt/gem_ctx_create/files
Fixes: 3365e2268b6b ("drm/i915: Lazily unbind vma on close")
Signed-off-by: Chris Wilson
Cc: Tvrtko Ursulin
Cc: Paulo Zanoni
---
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));
list_add_tail(&vma->obj_link, &obj->vma_list);
}
@@ -810,9 +809,6 @@ static void __i915_vma_destroy(struct i915_vma *vma)
if (vma->obj)
rb_erase(&vma->obj_node, &vma->obj->vma_tree);
- if (!i915_vma_is_ggtt(vma))
- i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm));
-
rbtree_postorder_for_each_entry_safe(iter, n, &vma->active, node) {
GEM_BUG_ON(i915_gem_active_isset(&iter->base));
kfree(iter);
--
2.18.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
