[Intel-gfx] [PATCH 09/11] drm/i915: Release vma when the handle is closed

[Chris Wilson]

On Thu, Dec 17, 2015 at 01:46:58PM +0000, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 14/12/15 11:36, Chris Wilson wrote:
> >In order to prevent a leak of the vma on shared objects, we need to
> >hook into the object_close callback to destroy the vma on the object for
> >this file. However, if we destroyed that vma immediately we may cause
> >unexpected application stalls as we try to unbind a busy vma - hence we
> >defer the unbind to when we retire the vma.
> >
> >Testcase: igt/gem_ppggtt/flink-and-close-vma-leak
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> >Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com
> >---
> >  drivers/gpu/drm/i915/i915_drv.c     |  1 +
> >  drivers/gpu/drm/i915/i915_drv.h     |  1 +
> >  drivers/gpu/drm/i915/i915_gem.c     | 41 ++++++++++++++++++++++---------------
> >  drivers/gpu/drm/i915/i915_gem_gtt.c |  2 ++
> >  drivers/gpu/drm/i915/i915_gem_gtt.h |  1 +
> >  5 files changed, 30 insertions(+), 16 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >index e7eef5fd6918..70979339d58a 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.c
> >+++ b/drivers/gpu/drm/i915/i915_drv.c
> >@@ -1656,6 +1656,7 @@ static struct drm_driver driver = {
> >  	.debugfs_init = i915_debugfs_init,
> >  	.debugfs_cleanup = i915_debugfs_cleanup,
> >  #endif
> >+	.gem_close_object = i915_gem_close_object,
> >  	.gem_free_object = i915_gem_free_object,
> >  	.gem_vm_ops = &i915_gem_vm_ops,
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index eb775eb1c693..696469a06715 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -2686,6 +2686,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
> >  						  size_t size);
> >  struct drm_i915_gem_object *i915_gem_object_create_from_data(
> >  		struct drm_device *dev, const void *data, size_t size);
> >+void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file);
> >  void i915_gem_free_object(struct drm_gem_object *obj);
> >  void i915_gem_vma_destroy(struct i915_vma *vma);
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 1d21c5b79215..7c13c27a6470 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2367,6 +2367,30 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
> >  	return 0;
> >  }
> >
> >+static void i915_vma_close(struct i915_vma *vma)
> >+{
> >+	RQ_BUG_ON(vma->closed);
> 
> Same complaint as in the previous patch, cannot use RQ_BUG_ON. Maybe
> need GEM_BUG_ON then, don't know.

Hopefully Joonas will jump in to the rescue. GEM_BUG_ON() works for me.
 
> >+	vma->closed = true;
> 
> Hmmm, vma->detached? Because VMA is not really closed. And
> i915_vma_detach - it would symbolise then that VMA has been detached
> from the object and is lingering only on the VM lists.

Perhaps. I chose _close() simply because that it the user action that
initiated all the actitive (either GEM_CLOSE or GEM_CONTEXT_CLOSE, or
the implicit close from close(fd)/task_exit).

detach feels a little too undefined, close at least implies termination
to me.

Of course on the vfs side, close() is handled by fput/delayed_fput!

Maybe:

gem_object_close -> i915_vma_release
context_close -> i915_ppgtt_release -> i915_vma_release

though release is already used by kref tracking (i915_ppgtt_release).

I'm not keen on using i915_vma_get/i915_vma_put, precisely because we
have managed to avoid using kref vma so far (and so we are not doing
typical reference tracking).

gem_object_close -> i915_vma_detach
context_close -> i915_ppgtt_detach -> i915_vma_detach

Still liking the consistency of close.

gem_object_close -> i915_vma_close
context_close -> i915_ppgtt_close -> i915_vma_close

Could be worse, but also there may easily be a better naming pattern.

> >@@ -3792,20 +3813,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> >  	trace_i915_gem_object_destroy(obj);
> >
> >  	list_for_each_entry_safe(vma, next, &obj->vma_list, obj_link) {
> >-		int ret;
> >-
> >  		vma->pin_count = 0;
> >-		ret = i915_vma_unbind(vma);
> >-		if (WARN_ON(ret == -ERESTARTSYS)) {
> >-			bool was_interruptible;
> >-
> >-			was_interruptible = dev_priv->mm.interruptible;
> >-			dev_priv->mm.interruptible = false;
> >-
> >-			WARN_ON(i915_vma_unbind(vma));
> >-
> >-			dev_priv->mm.interruptible = was_interruptible;
> >-		}
> >+		i915_vma_close(vma);
> 
> In what circumstances can there be any VMAs still left unclosed at
> this point? I thought i915_gem_close_object would had closed them
> all.

vma belonging to GGTT are not owned by any one file but shared, so we
still expect them here as well.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre