[PATCH 01/15] drm/i915: vma/ppgtt lifetime rules

* [PATCH 01/15] drm/i915: vma/ppgtt lifetime rules
@ 2014-08-06 13:04 Daniel Vetter
  2014-08-06 13:04 ` [PATCH 02/15] drm/i915: Some cleanups for the ppgtt lifetime handling Daniel Vetter
                   ` (13 more replies)
  0 siblings, 14 replies; 32+ messages in thread
From: Daniel Vetter @ 2014-08-06 13:04 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

From: Michel Thierry <michel.thierry@intel.com>

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.

Note that with this we will extend the lifetime of ppgtts which
contain shared objects. But all the non-shared objects will get
removed as soon as they drop of the active list and for the shared
ones the shrinker can eventually reap them. Since we currently can't
evict ppgtt pagetables either I don't think that temporary leak is
important.

Signed-off-by: Michel Thierry <michel.thierry@intel.com>
[danvet: Add note about potential ppgtt leak with this approach.]
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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 7c25345ce3e5..5a18680011da 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2510,7 +2510,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 dcd8d7b42552..25a32b9c9b4b 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);
+
 	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 3b99390e467a..ae706cba05ae 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 1411613f2174..90c3d0fae3f1 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.3

^ permalink raw reply related	[flat|nested] 32+ messages in thread