On Tue, Jul 29, 2014 at 11:08:05AM +0100, Michel Thierry wrote:
> VMAs should take a reference of the address space they use.
>
> Now, when the fd is closed, it will release the ref that the context was
> holding, but it will still be referenced by any vmas that are still
> active.
>
> ppgtt_release() should then only be called when the last thing referencing
> it releases the ref, and it can just call the base cleanup and free the
> ppgtt.
>
> Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 ++
> drivers/gpu/drm/i915/i915_gem.c | 8 ++++++++
> drivers/gpu/drm/i915/i915_gem_context.c | 23 +++--------------------
> drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++
> 4 files changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2acc03f..a879a93 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2495,7 +2495,9 @@ void i915_gem_object_ggtt_unpin(struct drm_i915_gem_object *obj);
>
> /* i915_gem_context.c */
> #define ctx_to_ppgtt(ctx) container_of((ctx)->vm, struct i915_hw_ppgtt, base)
> +#define vm_to_ppgtt(vm) container_of(vm, struct i915_hw_ppgtt, base)
> int __must_check i915_gem_context_init(struct drm_device *dev);
> +void ppgtt_release(struct kref *kref);
> void i915_gem_context_fini(struct drm_device *dev);
> void i915_gem_context_reset(struct drm_device *dev);
> int i915_gem_context_open(struct drm_device *dev, struct drm_file *file);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index dcd8d7b..25a32b9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4499,12 +4499,20 @@ struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
>
> void i915_gem_vma_destroy(struct i915_vma *vma)
> {
> + struct i915_address_space *vm = NULL;
> + struct i915_hw_ppgtt *ppgtt = NULL;
> WARN_ON(vma->node.allocated);
>
> /* Keep the vma as a placeholder in the execbuffer reservation lists */
> if (!list_empty(&vma->exec_list))
> return;
>
> + vm = vma->vm;
> + ppgtt = vm_to_ppgtt(vm);
> +
> + if (ppgtt)
> + kref_put(&ppgtt->ref, ppgtt_release);
Hm, this has the risk that we leave unwanted vmas around for a bit longer.
They will get cleaned up though when the final object references goes
away, so the leak is fairly restricted. And currently we don't have a
shrinker to just whack out vma objects even ...
It's definitely a much neater solution than what I had in mind with moving
vmas to full-blown active tracking like we do objects. So I'm tempted to
go with, but have a bit a lurking feeling that I'm missing something.
Chris?
Cheers, Daniel
> +
> list_del(&vma->vma_link);
>
> kfree(vma);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 5b5af6c..59272f9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -108,30 +108,13 @@ static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
> return;
> }
>
> - /*
> - * Make sure vmas are unbound before we take down the drm_mm
> - *
> - * FIXME: Proper refcounting should take care of this, this shouldn't be
> - * needed at all.
> - */
> - if (!list_empty(&vm->active_list)) {
> - struct i915_vma *vma;
> -
> - list_for_each_entry(vma, &vm->active_list, mm_list)
> - if (WARN_ON(list_empty(&vma->vma_link) ||
> - list_is_singular(&vma->vma_link)))
> - break;
> -
> - i915_gem_evict_vm(&ppgtt->base, true);
> - } else {
> - i915_gem_retire_requests(dev);
> - i915_gem_evict_vm(&ppgtt->base, false);
> - }
> + /* vmas should already be unbound */
> + WARN_ON(!list_empty(&vm->active_list));
>
> ppgtt->base.cleanup(&ppgtt->base);
> }
>
> -static void ppgtt_release(struct kref *kref)
> +void ppgtt_release(struct kref *kref)
> {
> struct i915_hw_ppgtt *ppgtt =
> container_of(kref, struct i915_hw_ppgtt, ref);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 1411613..90c3d0f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2159,10 +2159,15 @@ i915_gem_obj_lookup_or_create_vma(struct drm_i915_gem_object *obj,
> struct i915_address_space *vm)
> {
> struct i915_vma *vma;
> + struct i915_hw_ppgtt *ppgtt = NULL;
>
> vma = i915_gem_obj_to_vma(obj, vm);
> if (!vma)
> vma = __i915_gem_vma_create(obj, vm);
>
> + ppgtt = vm_to_ppgtt(vm);
> + if (ppgtt)
> + kref_get(&ppgtt->ref);
> +
> return vma;
> }
> --
> 1.9.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
