[PATCH] drm/i915: Do not leak VMAs (and PPGTT VMs) of imported flinked objects

* [PATCH] drm/i915: Do not leak VMAs (and PPGTT VMs) of imported flinked objects
@ 2015-04-20 12:14 Tvrtko Ursulin
  2015-04-20 12:36 ` Chris Wilson
  2015-04-20 16:57 ` shuang.he
  0 siblings, 2 replies; 7+ messages in thread
From: Tvrtko Ursulin @ 2015-04-20 12:14 UTC (permalink / raw)
  To: Intel-gfx

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

If a client instantiates a VMA against an imported object (flink) this VMA
will not get unbound when the object is closed.

This happens because the exporter holds a reference count on the object and
will also keep a reference to the PPGTT VM.

In real life this happens with xorg-driver-intel and fbcon takeover. Latter
is copied from via the flink name and when Xorg process exists one VMA
remains dangling with a now unreachable VM.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Testcase: igt/gem_ppgtt/flink-vs-ctx-vm-leak
---
 drivers/gpu/drm/i915/i915_drv.c |  1 +
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/i915_gem.c | 63 ++++++++++++++++++++++++++++++++---------
 3 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f9754c3..16a0b34 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1630,6 +1630,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 6a2528c..e82790b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2635,6 +2635,8 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 						  size_t size);
 void i915_init_vm(struct drm_i915_private *dev_priv,
 		  struct i915_address_space *vm);
+void i915_gem_close_object(struct drm_gem_object *gem_obj,
+			   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 f7b8766..a720154 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4523,6 +4523,55 @@ static bool discard_backing_storage(struct drm_i915_gem_object *obj)
 	return atomic_long_read(&obj->base.filp->f_count) == 1;
 }
 
+static void i915_gem_unbind_vma(struct drm_i915_private *dev_priv,
+				struct i915_vma *vma)
+{
+	if (WARN_ON(i915_vma_unbind(vma) == -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;
+	}
+}
+
+void i915_gem_close_object(struct drm_gem_object *gem_obj,
+			   struct drm_file *file)
+{
+	struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
+	struct drm_device *dev = obj->base.dev;
+	struct drm_i915_file_private *file_priv = file->driver_priv;
+	struct i915_vma *vma, *next;
+	struct i915_hw_ppgtt *ppgtt;
+
+	mutex_lock(&dev->struct_mutex);
+
+	/*
+	 * Release all VMAs associated with this client's PPGTT.
+	 *
+	 * This is to avoid potentially unreachable VMAs since contexts can have
+	 * shorter lifetime than objects. Meaning if a client has a reference to
+	 * an object (flink) and an instantiated VMA, when it exists neither VMA
+	 * will be unbound (since object free won't run), nor the PPGTT VM
+	 * freed (since VMA holds a reference to it).
+	 */
+	list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
+		if (i915_is_ggtt(vma->vm))
+			continue;
+
+		ppgtt = (struct i915_hw_ppgtt *)vma->vm;
+		if (ppgtt->file_priv != file_priv)
+			continue;
+
+		i915_gem_unbind_vma(dev->dev_private, vma);
+	}
+
+	mutex_unlock(&dev->struct_mutex);
+}
+
 void i915_gem_free_object(struct drm_gem_object *gem_obj)
 {
 	struct drm_i915_gem_object *obj = to_intel_bo(gem_obj);
@@ -4535,20 +4584,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, vma_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_gem_unbind_vma(dev_priv, vma);
 	}
 
 	/* Stolen objects don't hold a ref, but do hold pin count. Fix that up
-- 
2.3.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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