[PATCH 79/81] drm/i915: Release vma when the handle is closed
Chris Wilson
chris at chris-wilson.co.uk
Sat Apr 16 18:11:36 UTC 2016
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.
v2: Keep vma allocated until closed. This is useful for a later
optimisation, but it is required now in order to handle potential
recursion of i915_vma_unbind() by retiring itself.
v3: Comments are important.
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 | 4 +-
drivers/gpu/drm/i915/i915_gem.c | 104 +++++++++++++++++++---------------
drivers/gpu/drm/i915/i915_gem_evict.c | 7 +--
drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++
drivers/gpu/drm/i915/i915_gem_gtt.h | 1 +
6 files changed, 89 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f1e30c460c23..c999caad1e3a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1666,6 +1666,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 21643a97d524..92583992f8bc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2778,8 +2778,8 @@ 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);
/* Flags used by pin/bind&friends. */
#define PIN_MAPPABLE (1<<0)
@@ -2812,6 +2812,8 @@ int __must_check i915_vma_unbind(struct i915_vma *vma);
* _guarantee_ VMA in question is _not in use_ anywhere.
*/
int __must_check __i915_vma_unbind_no_wait(struct i915_vma *vma);
+void i915_vma_close(struct i915_vma *vma);
+void i915_vma_destroy(struct i915_vma *vma);
int i915_gem_object_unbind(struct drm_i915_gem_object *obj);
int i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3a578141d0e6..ee498b15f868 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2385,6 +2385,20 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
}
}
+void i915_gem_close_object(struct drm_gem_object *gem,
+ struct drm_file *file)
+{
+ struct drm_i915_gem_object *obj = to_intel_bo(gem);
+ struct drm_i915_file_private *fpriv = file->driver_priv;
+ struct i915_vma *vma, *vn;
+
+ mutex_lock(&obj->base.dev->struct_mutex);
+ list_for_each_entry_safe(vma, vn, &obj->vma_list, obj_link)
+ if (vma->vm->file == fpriv)
+ i915_vma_close(vma);
+ mutex_unlock(&obj->base.dev->struct_mutex);
+}
+
/**
* i915_gem_wait_ioctl - implements DRM_IOCTL_I915_GEM_WAIT
* @DRM_IOCTL_ARGS: standard ioctl arguments
@@ -2577,28 +2591,41 @@ static void i915_gem_object_finish_gtt(struct drm_i915_gem_object *obj)
static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
{
struct drm_i915_gem_object *obj = vma->obj;
- int ret;
+ int ret, i;
- if (list_empty(&vma->obj_link))
- return 0;
+ /* First wait upon any activity as retiring the request may
+ * have side-effects such as unpinning or even unbinding this vma.
+ */
+ if (i915_vma_is_active(vma) && wait) {
+ /* When a closed VMA is retired, it is unbound - eek.
+ * In order to prevent it from being recursively closed,
+ * take a pin on the vma so that the second unbind is
+ * aborted.
+ */
+ vma->pin_count++;
- if (!drm_mm_node_allocated(&vma->node)) {
- i915_gem_vma_destroy(vma);
- return 0;
+ for (i = 0; i < ARRAY_SIZE(vma->last_read); i++) {
+ ret = i915_wait_request(vma->last_read[i].request);
+ if (ret)
+ break;
+ }
+
+ vma->pin_count--;
+ if (ret)
+ return ret;
+
+ GEM_BUG_ON(i915_vma_is_active(vma));
}
if (vma->pin_count)
return -EBUSY;
+ if (!drm_mm_node_allocated(&vma->node))
+ goto destroy;
+
GEM_BUG_ON(obj->bind_count == 0);
GEM_BUG_ON(obj->pages == NULL);
- if (wait) {
- ret = i915_gem_object_wait_rendering(obj, false);
- if (ret)
- return ret;
- }
-
if (vma->is_ggtt && vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {
i915_gem_object_finish_gtt(obj);
@@ -2625,7 +2652,6 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
}
drm_mm_remove_node(&vma->node);
- i915_gem_vma_destroy(vma);
/* Since the unbound list is global, only move to that list if
* no more VMAs exist. */
@@ -2639,6 +2665,10 @@ static int __i915_vma_unbind(struct i915_vma *vma, bool wait)
*/
i915_gem_object_unpin_pages(obj);
+destroy:
+ if (unlikely(vma->closed))
+ i915_vma_destroy(vma);
+
return 0;
}
@@ -2818,7 +2848,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
if (offset & (alignment - 1) || offset + size > end) {
ret = -EINVAL;
- goto err_free_vma;
+ goto err_vma;
}
vma->node.start = offset;
vma->node.size = size;
@@ -2830,7 +2860,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
ret = drm_mm_reserve_node(&vm->mm, &vma->node);
}
if (ret)
- goto err_free_vma;
+ goto err_vma;
} else {
if (flags & PIN_HIGH) {
search_flag = DRM_MM_SEARCH_BELOW;
@@ -2855,7 +2885,7 @@ search_free:
if (ret == 0)
goto search_free;
- goto err_free_vma;
+ goto err_vma;
}
}
if (WARN_ON(!i915_gem_valid_gtt_space(vma, obj->cache_level))) {
@@ -2876,8 +2906,7 @@ search_free:
err_remove_node:
drm_mm_remove_node(&vma->node);
-err_free_vma:
- i915_gem_vma_destroy(vma);
+err_vma:
vma = ERR_PTR(ret);
err_unpin:
i915_gem_object_unpin_pages(obj);
@@ -3825,21 +3854,18 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
trace_i915_gem_object_destroy(obj);
+ /* All file-owned VMA should have been released by this point through
+ * i915_gem_close_object(), or earlier by i915_gem_context_close().
+ * However, the object may also be bound into the global GTT (e.g.
+ * older GPUs without per-process support, or for direct access through
+ * the GTT either for the user or for scanout). Those VMA still need to
+ * unbound now.
+ */
list_for_each_entry_safe(vma, next, &obj->vma_list, obj_link) {
- int ret;
-
+ GEM_BUG_ON(!vma->is_ggtt);
+ GEM_BUG_ON(i915_vma_is_active(vma));
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);
}
GEM_BUG_ON(obj->bind_count);
@@ -3906,22 +3932,6 @@ struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
return NULL;
}
-void i915_gem_vma_destroy(struct i915_vma *vma)
-{
- WARN_ON(vma->node.allocated);
-
- /* Keep the vma as a placeholder in the execbuffer reservation lists */
- if (!list_empty(&vma->exec_list))
- return;
-
- if (!vma->is_ggtt)
- i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm));
-
- list_del(&vma->obj_link);
-
- kmem_cache_free(to_i915(vma->obj)->vmas, vma);
-}
-
static void
i915_gem_stop_engines(struct drm_device *dev)
{
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index ea1f8d1bd228..3f19e729b6d1 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -174,8 +174,8 @@ found:
struct i915_vma,
exec_list);
if (drm_mm_scan_remove_block(&vma->node)) {
+ vma->pin_count++;
list_move(&vma->exec_list, &eviction_list);
- drm_gem_object_reference(&vma->obj->base);
continue;
}
list_del_init(&vma->exec_list);
@@ -183,17 +183,14 @@ found:
/* Unbinding will emit any required flushes */
while (!list_empty(&eviction_list)) {
- struct drm_gem_object *obj;
vma = list_first_entry(&eviction_list,
struct i915_vma,
exec_list);
- obj = &vma->obj->base;
list_del_init(&vma->exec_list);
+ vma->pin_count--;
if (ret == 0)
ret = i915_vma_unbind(vma);
-
- drm_gem_object_unreference(obj);
}
return ret;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2fd66e2f4c1f..6453c13ae343 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3293,6 +3293,31 @@ i915_vma_retire(struct i915_gem_active *active,
return;
list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
+ if (unlikely(vma->closed && !vma->pin_count))
+ WARN_ON(i915_vma_unbind(vma));
+}
+
+void i915_vma_destroy(struct i915_vma *vma)
+{
+ GEM_BUG_ON(vma->node.allocated);
+ GEM_BUG_ON(i915_vma_is_active(vma));
+ GEM_BUG_ON(!vma->closed);
+
+ list_del(&vma->vm_link);
+ if (!vma->is_ggtt)
+ i915_ppgtt_put(i915_vm_to_ppgtt(vma->vm));
+
+ kmem_cache_free(to_i915(vma->obj)->vmas, vma);
+}
+
+void i915_vma_close(struct i915_vma *vma)
+{
+ GEM_BUG_ON(vma->closed);
+ vma->closed = true;
+
+ list_del_init(&vma->obj_link);
+ if (!i915_vma_is_active(vma) && !vma->pin_count)
+ WARN_ON(i915_vma_unbind(vma));
}
static struct i915_vma *
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 5cf11a9acf9f..8dbc28b2061e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -186,6 +186,7 @@ struct i915_vma {
unsigned int bound : 4;
unsigned int active : I915_NUM_ENGINES;
bool is_ggtt : 1;
+ bool closed : 1;
/**
* Support different GGTT views into the same object.
--
2.8.0.rc3
More information about the Intel-gfx-trybot
mailing list