[PATCH 2/3] drm/i915: Properly close VM
Thomas Hellström
thomas.hellstrom at linux.intel.com
Tue Feb 8 10:37:34 UTC 2022
From: Niranjana Vishwanathapura <niranjana.vishwanathapura at intel.com>
VMs are not properly getting closed as vm->open ref count is
never reaching zero.
Properly handle vm->open ref counting and fix any issues
resulting from properly closing the VM.
Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura at intel.com>
Co-developed-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 37 ++++----
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 6 ++
.../gpu/drm/i915/gem/selftests/mock_context.c | 4 +-
drivers/gpu/drm/i915/gt/intel_gtt.c | 23 +++--
drivers/gpu/drm/i915/gt/intel_gtt.h | 5 ++
drivers/gpu/drm/i915/gt/selftest_execlists.c | 86 +++++++++----------
drivers/gpu/drm/i915/i915_gem.c | 6 +-
drivers/gpu/drm/i915/i915_vma.c | 25 ++++--
drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 2 +-
9 files changed, 113 insertions(+), 81 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index ebbac2ea0833..9d960a5ee225 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -196,7 +196,7 @@ static void proto_context_close(struct drm_i915_private *i915,
if (pc->pxp_wakeref)
intel_runtime_pm_put(&i915->runtime_pm, pc->pxp_wakeref);
if (pc->vm)
- i915_vm_put(pc->vm);
+ i915_vm_close(pc->vm);
if (pc->user_engines) {
for (i = 0; i < pc->num_user_engines; i++)
kfree(pc->user_engines[i].siblings);
@@ -364,8 +364,9 @@ static int set_proto_ctx_vm(struct drm_i915_file_private *fpriv,
return -ENOENT;
if (pc->vm)
- i915_vm_put(pc->vm);
- pc->vm = vm;
+ i915_vm_close(pc->vm);
+ pc->vm = i915_vm_open(vm);
+ i915_vm_put(vm);
return 0;
}
@@ -1453,6 +1454,13 @@ static void context_close(struct i915_gem_context *ctx)
set_closed_name(ctx);
+ /*
+ * The LUT uses the VMA as a backpointer to unref the object,
+ * so we need to clear the LUT before we close all the VMA (inside
+ * the ppgtt).
+ */
+ lut_close(ctx);
+
vm = ctx->vm;
if (vm) {
/* i915_vm_close drops the final reference, which is a bit too
@@ -1466,13 +1474,6 @@ static void context_close(struct i915_gem_context *ctx)
ctx->file_priv = ERR_PTR(-EBADF);
- /*
- * The LUT uses the VMA as a backpointer to unref the object,
- * so we need to clear the LUT before we close all the VMA (inside
- * the ppgtt).
- */
- lut_close(ctx);
-
spin_lock(&ctx->i915->gem.contexts.lock);
list_del(&ctx->link);
spin_unlock(&ctx->i915->gem.contexts.lock);
@@ -1558,7 +1559,7 @@ i915_gem_create_context(struct drm_i915_private *i915,
INIT_LIST_HEAD(&ctx->stale.engines);
if (pc->vm) {
- vm = i915_vm_get(pc->vm);
+ vm = i915_vm_open(pc->vm);
} else if (HAS_FULL_PPGTT(i915)) {
struct i915_ppgtt *ppgtt;
@@ -1571,12 +1572,8 @@ i915_gem_create_context(struct drm_i915_private *i915,
}
vm = &ppgtt->vm;
}
- if (vm) {
- ctx->vm = i915_vm_open(vm);
-
- /* i915_vm_open() takes a reference */
- i915_vm_put(vm);
- }
+ if (vm)
+ ctx->vm = vm;
mutex_init(&ctx->engines_mutex);
if (pc->num_user_engines >= 0) {
@@ -1725,7 +1722,7 @@ void i915_gem_context_close(struct drm_file *file)
xa_destroy(&file_priv->context_xa);
xa_for_each(&file_priv->vm_xa, idx, vm)
- i915_vm_put(vm);
+ i915_vm_close(vm);
xa_destroy(&file_priv->vm_xa);
}
@@ -1767,7 +1764,7 @@ int i915_gem_vm_create_ioctl(struct drm_device *dev, void *data,
return 0;
err_put:
- i915_vm_put(&ppgtt->vm);
+ i915_vm_close(&ppgtt->vm);
return err;
}
@@ -1788,7 +1785,7 @@ int i915_gem_vm_destroy_ioctl(struct drm_device *dev, void *data,
if (!vm)
return -ENOENT;
- i915_vm_put(vm);
+ i915_vm_close(vm);
return 0;
}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 498b458fd784..abcf6bad82c7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2701,6 +2701,11 @@ eb_select_engine(struct i915_execbuffer *eb)
if (err)
goto err;
+ if (!i915_vm_tryopen(ce->vm)) {
+ err = -ENOENT;
+ goto err;
+ }
+
eb->context = ce;
eb->gt = ce->engine->gt;
@@ -2724,6 +2729,7 @@ eb_put_engine(struct i915_execbuffer *eb)
{
struct intel_context *child;
+ i915_vm_close(eb->context->vm);
intel_gt_pm_put(eb->gt);
for_each_child(eb->context, child)
intel_context_put(child);
diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
index c0a8ef368044..e657e56b49c3 100644
--- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
@@ -158,8 +158,8 @@ kernel_context(struct drm_i915_private *i915,
if (vm) {
if (pc->vm)
- i915_vm_put(pc->vm);
- pc->vm = i915_vm_get(vm);
+ i915_vm_close(pc->vm);
+ pc->vm = i915_vm_open(vm);
}
ctx = i915_gem_create_context(i915, pc);
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
index 341231c07635..c08c057399af 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -96,14 +96,11 @@ int map_pt_dma_locked(struct i915_address_space *vm, struct drm_i915_gem_object
return 0;
}
-void __i915_vm_close(struct i915_address_space *vm)
+static void clear_vm_list(struct list_head *list)
{
struct i915_vma *vma, *vn;
- if (!atomic_dec_and_mutex_lock(&vm->open, &vm->mutex))
- return;
-
- list_for_each_entry_safe(vma, vn, &vm->bound_list, vm_link) {
+ list_for_each_entry_safe(vma, vn, list, vm_link) {
struct drm_i915_gem_object *obj = vma->obj;
if (!kref_get_unless_zero(&obj->base.refcount)) {
@@ -114,6 +111,7 @@ void __i915_vm_close(struct i915_address_space *vm)
*/
atomic_and(~I915_VMA_PIN_MASK, &vma->flags);
WARN_ON(__i915_vma_unbind(vma));
+ list_del_init(&vma->vm_link);
continue;
}
@@ -121,8 +119,20 @@ void __i915_vm_close(struct i915_address_space *vm)
i915_vma_destroy_locked(vma);
i915_gem_object_put(obj);
}
- GEM_BUG_ON(!list_empty(&vm->bound_list));
+}
+void __i915_vm_close(struct i915_address_space *vm)
+{
+ if (!atomic_dec_and_mutex_lock(&vm->open, &vm->mutex))
+ return;
+
+ clear_vm_list(&vm->bound_list);
+ clear_vm_list(&vm->unbound_list);
+
+ /* Check for must-fix unanticipated side-effects */
+ GEM_BUG_ON(!list_empty(&vm->bound_list));
+ GEM_BUG_ON(!list_empty(&vm->unbound_list));
+
mutex_unlock(&vm->mutex);
}
@@ -232,6 +242,7 @@ void i915_address_space_init(struct i915_address_space *vm, int subclass)
vm->mm.head_node.color = I915_COLOR_UNEVICTABLE;
INIT_LIST_HEAD(&vm->bound_list);
+ INIT_LIST_HEAD(&vm->unbound_list);
}
void *__px_vaddr(struct drm_i915_gem_object *p)
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
index 8073438b67c8..ff566f6ee958 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -249,6 +249,11 @@ struct i915_address_space {
*/
struct list_head bound_list;
+ /**
+ * List of vmas not yet bound or evicted.
+ */
+ struct list_head unbound_list;
+
/* Global GTT */
bool is_ggtt:1;
diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c b/drivers/gpu/drm/i915/gt/selftest_execlists.c
index e10da897e07a..401f71973238 100644
--- a/drivers/gpu/drm/i915/gt/selftest_execlists.c
+++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c
@@ -1735,15 +1735,9 @@ static int live_preempt(void *arg)
enum intel_engine_id id;
int err = -ENOMEM;
- if (igt_spinner_init(&spin_hi, gt))
- return -ENOMEM;
-
- if (igt_spinner_init(&spin_lo, gt))
- goto err_spin_hi;
-
ctx_hi = kernel_context(gt->i915, NULL);
if (!ctx_hi)
- goto err_spin_lo;
+ return -ENOMEM;
ctx_hi->sched.priority = I915_CONTEXT_MAX_USER_PRIORITY;
ctx_lo = kernel_context(gt->i915, NULL);
@@ -1751,6 +1745,12 @@ static int live_preempt(void *arg)
goto err_ctx_hi;
ctx_lo->sched.priority = I915_CONTEXT_MIN_USER_PRIORITY;
+ if (igt_spinner_init(&spin_hi, gt))
+ goto err_ctx_lo;
+
+ if (igt_spinner_init(&spin_lo, gt))
+ goto err_spin_hi;
+
for_each_engine(engine, gt, id) {
struct igt_live_test t;
struct i915_request *rq;
@@ -1760,14 +1760,14 @@ static int live_preempt(void *arg)
if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) {
err = -EIO;
- goto err_ctx_lo;
+ goto err_spin_lo;
}
rq = spinner_create_request(&spin_lo, ctx_lo, engine,
MI_ARB_CHECK);
if (IS_ERR(rq)) {
err = PTR_ERR(rq);
- goto err_ctx_lo;
+ goto err_spin_lo;
}
i915_request_add(rq);
@@ -1776,7 +1776,7 @@ static int live_preempt(void *arg)
GEM_TRACE_DUMP();
intel_gt_set_wedged(gt);
err = -EIO;
- goto err_ctx_lo;
+ goto err_spin_lo;
}
rq = spinner_create_request(&spin_hi, ctx_hi, engine,
@@ -1784,7 +1784,7 @@ static int live_preempt(void *arg)
if (IS_ERR(rq)) {
igt_spinner_end(&spin_lo);
err = PTR_ERR(rq);
- goto err_ctx_lo;
+ goto err_spin_lo;
}
i915_request_add(rq);
@@ -1793,7 +1793,7 @@ static int live_preempt(void *arg)
GEM_TRACE_DUMP();
intel_gt_set_wedged(gt);
err = -EIO;
- goto err_ctx_lo;
+ goto err_spin_lo;
}
igt_spinner_end(&spin_hi);
@@ -1801,19 +1801,19 @@ static int live_preempt(void *arg)
if (igt_live_test_end(&t)) {
err = -EIO;
- goto err_ctx_lo;
+ goto err_spin_lo;
}
}
err = 0;
-err_ctx_lo:
- kernel_context_close(ctx_lo);
-err_ctx_hi:
- kernel_context_close(ctx_hi);
err_spin_lo:
igt_spinner_fini(&spin_lo);
err_spin_hi:
igt_spinner_fini(&spin_hi);
+err_ctx_lo:
+ kernel_context_close(ctx_lo);
+err_ctx_hi:
+ kernel_context_close(ctx_hi);
return err;
}
@@ -1827,20 +1827,20 @@ static int live_late_preempt(void *arg)
enum intel_engine_id id;
int err = -ENOMEM;
- if (igt_spinner_init(&spin_hi, gt))
- return -ENOMEM;
-
- if (igt_spinner_init(&spin_lo, gt))
- goto err_spin_hi;
-
ctx_hi = kernel_context(gt->i915, NULL);
if (!ctx_hi)
- goto err_spin_lo;
+ return -ENOMEM;
ctx_lo = kernel_context(gt->i915, NULL);
if (!ctx_lo)
goto err_ctx_hi;
+ if (igt_spinner_init(&spin_hi, gt))
+ goto err_ctx_lo;
+
+ if (igt_spinner_init(&spin_lo, gt))
+ goto err_spin_hi;
+
/* Make sure ctx_lo stays before ctx_hi until we trigger preemption. */
ctx_lo->sched.priority = 1;
@@ -1853,14 +1853,14 @@ static int live_late_preempt(void *arg)
if (igt_live_test_begin(&t, gt->i915, __func__, engine->name)) {
err = -EIO;
- goto err_ctx_lo;
+ goto err_spin_lo;
}
rq = spinner_create_request(&spin_lo, ctx_lo, engine,
MI_ARB_CHECK);
if (IS_ERR(rq)) {
err = PTR_ERR(rq);
- goto err_ctx_lo;
+ goto err_spin_lo;
}
i915_request_add(rq);
@@ -1874,7 +1874,7 @@ static int live_late_preempt(void *arg)
if (IS_ERR(rq)) {
igt_spinner_end(&spin_lo);
err = PTR_ERR(rq);
- goto err_ctx_lo;
+ goto err_spin_lo;
}
i915_request_add(rq);
@@ -1897,19 +1897,19 @@ static int live_late_preempt(void *arg)
if (igt_live_test_end(&t)) {
err = -EIO;
- goto err_ctx_lo;
+ goto err_spin_lo;
}
}
err = 0;
-err_ctx_lo:
- kernel_context_close(ctx_lo);
-err_ctx_hi:
- kernel_context_close(ctx_hi);
err_spin_lo:
igt_spinner_fini(&spin_lo);
err_spin_hi:
igt_spinner_fini(&spin_hi);
+err_ctx_lo:
+ kernel_context_close(ctx_lo);
+err_ctx_hi:
+ kernel_context_close(ctx_hi);
return err;
err_wedged:
@@ -1917,7 +1917,7 @@ static int live_late_preempt(void *arg)
igt_spinner_end(&spin_lo);
intel_gt_set_wedged(gt);
err = -EIO;
- goto err_ctx_lo;
+ goto err_spin_lo;
}
struct preempt_client {
@@ -3381,12 +3381,9 @@ static int live_preempt_timeout(void *arg)
if (!intel_has_reset_engine(gt))
return 0;
- if (igt_spinner_init(&spin_lo, gt))
- return -ENOMEM;
-
ctx_hi = kernel_context(gt->i915, NULL);
if (!ctx_hi)
- goto err_spin_lo;
+ return -ENOMEM;
ctx_hi->sched.priority = I915_CONTEXT_MAX_USER_PRIORITY;
ctx_lo = kernel_context(gt->i915, NULL);
@@ -3394,6 +3391,9 @@ static int live_preempt_timeout(void *arg)
goto err_ctx_hi;
ctx_lo->sched.priority = I915_CONTEXT_MIN_USER_PRIORITY;
+ if (igt_spinner_init(&spin_lo, gt))
+ goto err_ctx_lo;
+
for_each_engine(engine, gt, id) {
unsigned long saved_timeout;
struct i915_request *rq;
@@ -3405,21 +3405,21 @@ static int live_preempt_timeout(void *arg)
MI_NOOP); /* preemption disabled */
if (IS_ERR(rq)) {
err = PTR_ERR(rq);
- goto err_ctx_lo;
+ goto err_spin_lo;
}
i915_request_add(rq);
if (!igt_wait_for_spinner(&spin_lo, rq)) {
intel_gt_set_wedged(gt);
err = -EIO;
- goto err_ctx_lo;
+ goto err_spin_lo;
}
rq = igt_request_alloc(ctx_hi, engine);
if (IS_ERR(rq)) {
igt_spinner_end(&spin_lo);
err = PTR_ERR(rq);
- goto err_ctx_lo;
+ goto err_spin_lo;
}
/* Flush the previous CS ack before changing timeouts */
@@ -3439,7 +3439,7 @@ static int live_preempt_timeout(void *arg)
intel_gt_set_wedged(gt);
i915_request_put(rq);
err = -ETIME;
- goto err_ctx_lo;
+ goto err_spin_lo;
}
igt_spinner_end(&spin_lo);
@@ -3447,12 +3447,12 @@ static int live_preempt_timeout(void *arg)
}
err = 0;
+err_spin_lo:
+ igt_spinner_fini(&spin_lo);
err_ctx_lo:
kernel_context_close(ctx_lo);
err_ctx_hi:
kernel_context_close(ctx_hi);
-err_spin_lo:
- igt_spinner_fini(&spin_lo);
return err;
}
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e3a2c2a0e156..81df379a2a50 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -139,8 +139,6 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj,
while (!ret && (vma = list_first_entry_or_null(&obj->vma.list,
struct i915_vma,
obj_link))) {
- struct i915_address_space *vm = vma->vm;
-
list_move_tail(&vma->obj_link, &still_in_list);
if (!i915_vma_is_bound(vma, I915_VMA_BIND_MASK))
continue;
@@ -151,7 +149,7 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj,
}
ret = -EAGAIN;
- if (!i915_vm_tryopen(vm))
+ if (!i915_vm_tryopen(vma->vm))
break;
/* Prevent vma being freed by i915_vma_parked as we unbind */
@@ -183,7 +181,7 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj,
__i915_vma_put(vma);
}
- i915_vm_close(vm);
+ i915_vm_close(vma->vm);
spin_lock(&obj->vma.lock);
}
list_splice_init(&still_in_list, &obj->vma.list);
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index ad23a4eaba4f..8f1f47078bc8 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -113,6 +113,7 @@ vma_create(struct drm_i915_gem_object *obj,
struct i915_vma *pos = ERR_PTR(-E2BIG);
struct i915_vma *vma;
struct rb_node *rb, **p;
+ int err;
/* The aliasing_ppgtt should never be used directly! */
GEM_BUG_ON(vm == &vm->gt->ggtt->alias->vm);
@@ -122,7 +123,6 @@ vma_create(struct drm_i915_gem_object *obj,
return ERR_PTR(-ENOMEM);
kref_init(&vma->ref);
- vma->vm = i915_vm_get(vm);
vma->ops = &vm->vma_ops;
vma->obj = obj;
vma->size = obj->base.size;
@@ -138,6 +138,8 @@ vma_create(struct drm_i915_gem_object *obj,
}
INIT_LIST_HEAD(&vma->closed_link);
+ INIT_LIST_HEAD(&vma->obj_link);
+ RB_CLEAR_NODE(&vma->obj_node);
if (view && view->type != I915_GGTT_VIEW_NORMAL) {
vma->ggtt_view = *view;
@@ -163,8 +165,16 @@ vma_create(struct drm_i915_gem_object *obj,
GEM_BUG_ON(!IS_ALIGNED(vma->size, I915_GTT_PAGE_SIZE));
- spin_lock(&obj->vma.lock);
+ err = mutex_lock_interruptible(&vm->mutex);
+ if (err) {
+ pos = ERR_PTR(err);
+ goto err_vma;
+ }
+
+ vma->vm = i915_vm_get(vm);
+ list_add_tail(&vma->vm_link, &vm->unbound_list);
+ spin_lock(&obj->vma.lock);
if (i915_is_ggtt(vm)) {
if (unlikely(overflows_type(vma->size, u32)))
goto err_unlock;
@@ -222,13 +232,16 @@ vma_create(struct drm_i915_gem_object *obj,
list_add_tail(&vma->obj_link, &obj->vma.list);
spin_unlock(&obj->vma.lock);
+ mutex_unlock(&vm->mutex);
return vma;
err_unlock:
spin_unlock(&obj->vma.lock);
-err_vma:
+ list_del_init(&vma->vm_link);
+ mutex_unlock(&vm->mutex);
i915_vm_put(vm);
+err_vma:
i915_vma_free(vma);
return pos;
}
@@ -829,7 +842,7 @@ i915_vma_insert(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
GEM_BUG_ON(!drm_mm_node_allocated(&vma->node));
GEM_BUG_ON(!i915_gem_valid_gtt_space(vma, color));
- list_add_tail(&vma->vm_link, &vma->vm->bound_list);
+ list_move_tail(&vma->vm_link, &vma->vm->bound_list);
return 0;
}
@@ -845,7 +858,7 @@ i915_vma_detach(struct i915_vma *vma)
* vma, we can drop its hold on the backing storage and allow
* it to be reaped by the shrinker.
*/
- list_del(&vma->vm_link);
+ list_move_tail(&vma->vm_link, &vma->vm->unbound_list);
}
static bool try_qad_pin(struct i915_vma *vma, unsigned int flags)
@@ -1673,6 +1686,7 @@ void i915_vma_destroy_locked(struct i915_vma *vma)
lockdep_assert_held(&vma->vm->mutex);
force_unbind(vma);
+ list_del_init(&vma->vm_link);
release_references(vma);
}
@@ -1680,6 +1694,7 @@ void i915_vma_destroy(struct i915_vma *vma)
{
mutex_lock(&vma->vm->mutex);
force_unbind(vma);
+ list_del_init(&vma->vm_link);
mutex_unlock(&vma->vm->mutex);
release_references(vma);
}
diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
index fba1c8be1649..fb402789ef8d 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem_gtt.c
@@ -1283,7 +1283,7 @@ static void track_vma_bind(struct i915_vma *vma)
vma->resource->bi.pages = vma->pages;
mutex_lock(&vma->vm->mutex);
- list_add_tail(&vma->vm_link, &vma->vm->bound_list);
+ list_move_tail(&vma->vm_link, &vma->vm->bound_list);
mutex_unlock(&vma->vm->mutex);
}
--
2.34.1
More information about the Intel-gfx-trybot
mailing list