[Intel-gfx] [PATCH 33/39] drm/i915: Move object close under its own lock
Chris Wilson
chris at chris-wilson.co.uk
Wed Mar 13 14:43:55 UTC 2019
Use i915_gem_object_lock() to guard the LUT and active reference to
allow us to break free of struct_mutex for handling GEM_CLOSE.
Testcase: igt/gem_close_race
Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 16 +++---
.../gpu/drm/i915/gem/i915_gem_context_types.h | 6 ---
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 22 +++++---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 51 +++++++++++++------
.../gpu/drm/i915/gem/i915_gem_object_types.h | 1 -
.../gpu/drm/i915/gem/selftests/mock_context.c | 1 -
drivers/gpu/drm/i915/i915_drv.h | 4 +-
drivers/gpu/drm/i915/i915_gem.c | 1 +
drivers/gpu/drm/i915/i915_vma.c | 41 ++++++++++-----
drivers/gpu/drm/i915/i915_vma.h | 15 +++---
.../gpu/drm/i915/selftests/mock_gem_device.c | 1 +
11 files changed, 97 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index af96b57f9d1d..7b7aea2d1877 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -112,24 +112,17 @@ void i915_lut_handle_free(struct i915_lut_handle *lut)
static void lut_close(struct i915_gem_context *ctx)
{
- struct i915_lut_handle *lut, *ln;
struct radix_tree_iter iter;
void __rcu **slot;
- list_for_each_entry_safe(lut, ln, &ctx->handles_list, ctx_link) {
- list_del(&lut->obj_link);
- i915_lut_handle_free(lut);
- }
- INIT_LIST_HEAD(&ctx->handles_list);
+ lockdep_assert_held(&ctx->mutex);
rcu_read_lock();
radix_tree_for_each_slot(slot, &ctx->handles_vma, &iter, 0) {
struct i915_vma *vma = rcu_dereference_raw(*slot);
radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
-
vma->open_count--;
- i915_vma_put(vma);
}
rcu_read_unlock();
}
@@ -312,6 +305,8 @@ void i915_gem_context_release(struct kref *ref)
static void context_close(struct i915_gem_context *ctx)
{
+ mutex_lock(&ctx->mutex);
+
i915_gem_context_set_closed(ctx);
/*
@@ -330,6 +325,8 @@ static void context_close(struct i915_gem_context *ctx)
i915_ppgtt_close(ctx->ppgtt);
ctx->file_priv = ERR_PTR(-EBADF);
+
+ mutex_unlock(&ctx->mutex);
i915_gem_context_put(ctx);
}
@@ -380,7 +377,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
spin_lock_init(&ctx->hw_contexts_lock);
INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
- INIT_LIST_HEAD(&ctx->handles_list);
INIT_LIST_HEAD(&ctx->hw_id_link);
/* Default context will never have a file_priv */
@@ -1100,7 +1096,9 @@ static int set_ppgtt(struct i915_gem_context *ctx,
goto unlock;
/* Teardown the existing obj:vma cache, it will have to be rebuilt. */
+ mutex_lock(&ctx->mutex);
lut_close(ctx);
+ mutex_unlock(&ctx->mutex);
old = __set_ppgtt(ctx, ppgtt);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
index 47ed1f65cc06..333eeacef5e3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
@@ -177,12 +177,6 @@ struct i915_gem_context {
* per vm, which may be one per context or shared with the global GTT)
*/
struct radix_tree_root handles_vma;
-
- /** handles_list: reverse list of all the rbtree entries in use for
- * this context, which allows us to free all the allocations on
- * context close.
- */
- struct list_head handles_list;
};
#endif /* __I915_GEM_CONTEXT_TYPES_H__ */
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 900417510fe0..1a9b14361e34 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -803,9 +803,6 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
unsigned int i, batch;
int err;
- if (unlikely(i915_gem_context_is_closed(eb->ctx)))
- return -ENOENT;
-
if (unlikely(i915_gem_context_is_banned(eb->ctx)))
return -EIO;
@@ -814,6 +811,12 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
batch = eb_batch_index(eb);
+ mutex_lock(&eb->ctx->mutex);
+ if (unlikely(i915_gem_context_is_closed(eb->ctx))) {
+ err = -ENOENT;
+ goto err_ctx;
+ }
+
for (i = 0; i < eb->buffer_count; i++) {
u32 handle = eb->exec[i].handle;
struct i915_lut_handle *lut;
@@ -850,10 +853,13 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
/* transfer ref to ctx */
if (!vma->open_count++)
i915_vma_reopen(vma);
- list_add(&lut->obj_link, &obj->lut_list);
- list_add(&lut->ctx_link, &eb->ctx->handles_list);
- lut->ctx = eb->ctx;
+
lut->handle = handle;
+ lut->ctx = i915_gem_context_get(eb->ctx);
+
+ i915_gem_object_lock(obj);
+ list_add(&lut->obj_link, &obj->lut_list);
+ i915_gem_object_unlock(obj);
add_vma:
err = eb_add_vma(eb, i, batch, vma);
@@ -866,6 +872,8 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
eb_vma_misplaced(&eb->exec[i], vma, eb->flags[i]));
}
+ mutex_unlock(&eb->ctx->mutex);
+
eb->args->flags |= __EXEC_VALIDATED;
return eb_reserve(eb);
@@ -873,6 +881,8 @@ static int eb_lookup_vmas(struct i915_execbuffer *eb)
i915_gem_object_put(obj);
err_vma:
eb->vma[i] = NULL;
+err_ctx:
+ mutex_unlock(&eb->ctx->mutex);
return err;
}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index d21a7095092c..7da9dbad55c9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -125,41 +125,60 @@ void i915_gem_object_set_cache_coherency(struct drm_i915_gem_object *obj,
!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_WRITE);
}
+static void lut_handle_release(struct i915_lut_handle *lut)
+{
+ list_del(&lut->obj_link);
+ i915_gem_context_put(lut->ctx);
+ i915_lut_handle_free(lut);
+}
+
void i915_gem_close_object(struct drm_gem_object *gem, struct drm_file *file)
{
- struct drm_i915_private *i915 = to_i915(gem->dev);
struct drm_i915_gem_object *obj = to_intel_bo(gem);
struct drm_i915_file_private *fpriv = file->driver_priv;
struct i915_lut_handle *lut, *ln;
+ LIST_HEAD(close);
- mutex_lock(&i915->drm.struct_mutex);
+ i915_gem_object_lock(obj);
list_for_each_entry_safe(lut, ln, &obj->lut_list, obj_link) {
struct i915_gem_context *ctx = lut->ctx;
- struct i915_vma *vma;
- GEM_BUG_ON(ctx->file_priv == ERR_PTR(-EBADF));
- if (ctx->file_priv != fpriv)
+ /* prune any stale entries */
+ if (i915_gem_context_is_closed(ctx)) {
+ lut_handle_release(lut);
+ i915_gem_object_put(obj);
continue;
+ }
- vma = radix_tree_delete(&ctx->handles_vma, lut->handle);
- GEM_BUG_ON(vma->obj != obj);
+ if (ctx->file_priv == fpriv)
+ list_move(&lut->obj_link, &close);
+ }
- /* We allow the process to have multiple handles to the same
+ list_for_each_entry_safe(lut, ln, &close, obj_link) {
+ struct i915_gem_context *ctx = lut->ctx;
+ struct i915_vma *vma;
+
+ i915_gem_object_unlock(obj);
+ mutex_lock(&ctx->mutex);
+ /*
+ * We allow the process to have multiple handles to the same
* vma, in the same fd namespace, by virtue of flink/open.
*/
- GEM_BUG_ON(!vma->open_count);
- if (!--vma->open_count && !i915_vma_is_ggtt(vma))
- i915_vma_close(vma);
-
- list_del(&lut->obj_link);
- list_del(&lut->ctx_link);
+ vma = radix_tree_delete(&ctx->handles_vma, lut->handle);
+ if (vma) {
+ GEM_BUG_ON(!vma->open_count);
+ if (!--vma->open_count && !i915_vma_is_ggtt(vma))
+ i915_vma_close(vma);
+ }
+ mutex_unlock(&ctx->mutex);
+ i915_gem_object_lock(obj);
- i915_lut_handle_free(lut);
+ lut_handle_release(lut);
i915_gem_object_put(obj);
}
- mutex_unlock(&i915->drm.struct_mutex);
+ i915_gem_object_unlock(obj);
}
static bool discard_backing_storage(struct drm_i915_gem_object *obj)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index 9f61220795a6..1d42b7681944 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -24,7 +24,6 @@ struct drm_i915_gem_object;
*/
struct i915_lut_handle {
struct list_head obj_link;
- struct list_head ctx_link;
struct i915_gem_context *ctx;
u32 handle;
};
diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
index 7f88fd6b3ce8..c9d9969821e0 100644
--- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
@@ -26,7 +26,6 @@ mock_context(struct drm_i915_private *i915,
spin_lock_init(&ctx->hw_contexts_lock);
INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
- INIT_LIST_HEAD(&ctx->handles_list);
INIT_LIST_HEAD(&ctx->hw_id_link);
INIT_LIST_HEAD(&ctx->active_engines);
mutex_init(&ctx->mutex);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2b9d92a1672f..13a0f33e6c39 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1987,9 +1987,11 @@ struct drm_i915_private {
intel_engine_mask_t active_engines;
struct list_head active_rings;
- struct list_head closed_vma;
u32 active_requests;
+ struct list_head closed_vma;
+ spinlock_t closed_lock; /* guards the list of closed_vma */
+
/**
* Is the GPU currently considered idle, or busy executing
* userspace requests? Whilst idle, we allow runtime power
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index de6db95e812a..8b2c9dcba0e0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2086,6 +2086,7 @@ int i915_gem_init_early(struct drm_i915_private *dev_priv)
INIT_LIST_HEAD(&dev_priv->gt.active_rings);
INIT_LIST_HEAD(&dev_priv->gt.closed_vma);
+ spin_lock_init(&dev_priv->gt.closed_lock);
i915_gem_init__mm(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 741ae66eeff8..9e95a572af72 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -142,6 +142,8 @@ vma_create(struct drm_i915_gem_object *obj,
vma->size = obj->base.size;
vma->display_alignment = I915_GTT_MIN_ALIGNMENT;
+ INIT_LIST_HEAD(&vma->closed_link);
+
if (view && view->type != I915_GGTT_VIEW_NORMAL) {
vma->ggtt_view = *view;
if (view->type == I915_GGTT_VIEW_PARTIAL) {
@@ -784,10 +786,9 @@ int __i915_vma_do_pin(struct i915_vma *vma,
void i915_vma_close(struct i915_vma *vma)
{
- lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
+ struct drm_i915_private *i915 = vma->vm->i915;
GEM_BUG_ON(i915_vma_is_closed(vma));
- vma->flags |= I915_VMA_CLOSED;
/*
* We defer actually closing, unbinding and destroying the VMA until
@@ -801,17 +802,26 @@ void i915_vma_close(struct i915_vma *vma)
* causing us to rebind the VMA once more. This ends up being a lot
* of wasted work for the steady state.
*/
- list_add_tail(&vma->closed_link, &vma->vm->i915->gt.closed_vma);
+ spin_lock(&i915->gt.closed_lock);
+ list_add(&vma->closed_link, &i915->gt.closed_vma);
+ spin_unlock(&i915->gt.closed_lock);
}
-void i915_vma_reopen(struct i915_vma *vma)
+static void __i915_vma_remove_closed(struct i915_vma *vma)
{
- lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
+ struct drm_i915_private *i915 = vma->vm->i915;
- if (vma->flags & I915_VMA_CLOSED) {
- vma->flags &= ~I915_VMA_CLOSED;
- list_del(&vma->closed_link);
- }
+ if (!i915_vma_is_closed(vma))
+ return;
+
+ spin_lock(&i915->gt.closed_lock);
+ list_del_init(&vma->closed_link);
+ spin_unlock(&i915->gt.closed_lock);
+}
+
+void i915_vma_reopen(struct i915_vma *vma)
+{
+ __i915_vma_remove_closed(vma);
}
static void __i915_vma_destroy(struct i915_vma *vma)
@@ -845,8 +855,7 @@ void i915_vma_destroy(struct i915_vma *vma)
GEM_BUG_ON(i915_vma_is_pinned(vma));
- if (i915_vma_is_closed(vma))
- list_del(&vma->closed_link);
+ __i915_vma_remove_closed(vma);
WARN_ON(i915_vma_unbind(vma));
GEM_BUG_ON(i915_vma_is_active(vma));
@@ -858,12 +867,16 @@ void i915_vma_parked(struct drm_i915_private *i915)
{
struct i915_vma *vma, *next;
+ spin_lock(&i915->gt.closed_lock);
list_for_each_entry_safe(vma, next, &i915->gt.closed_vma, closed_link) {
- GEM_BUG_ON(!i915_vma_is_closed(vma));
+ list_del_init(&vma->closed_link);
+ spin_unlock(&i915->gt.closed_lock);
+
i915_vma_destroy(vma);
- }
- GEM_BUG_ON(!list_empty(&i915->gt.closed_vma));
+ spin_lock(&i915->gt.closed_lock);
+ }
+ spin_unlock(&i915->gt.closed_lock);
}
static void __i915_vma_iounmap(struct i915_vma *vma)
diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
index 71ac7ee8620a..67bc67241c1b 100644
--- a/drivers/gpu/drm/i915/i915_vma.h
+++ b/drivers/gpu/drm/i915/i915_vma.h
@@ -104,10 +104,9 @@ struct i915_vma {
#define I915_VMA_GGTT BIT(11)
#define I915_VMA_CAN_FENCE BIT(12)
-#define I915_VMA_CLOSED BIT(13)
-#define I915_VMA_USERFAULT_BIT 14
+#define I915_VMA_USERFAULT_BIT 13
#define I915_VMA_USERFAULT BIT(I915_VMA_USERFAULT_BIT)
-#define I915_VMA_GGTT_WRITE BIT(15)
+#define I915_VMA_GGTT_WRITE BIT(14)
struct i915_active active;
struct i915_active_request last_fence;
@@ -190,11 +189,6 @@ static inline bool i915_vma_is_map_and_fenceable(const struct i915_vma *vma)
return vma->flags & I915_VMA_CAN_FENCE;
}
-static inline bool i915_vma_is_closed(const struct i915_vma *vma)
-{
- return vma->flags & I915_VMA_CLOSED;
-}
-
static inline bool i915_vma_set_userfault(struct i915_vma *vma)
{
GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
@@ -211,6 +205,11 @@ static inline bool i915_vma_has_userfault(const struct i915_vma *vma)
return test_bit(I915_VMA_USERFAULT_BIT, &vma->flags);
}
+static inline bool i915_vma_is_closed(const struct i915_vma *vma)
+{
+ return !list_empty(&vma->closed_link);
+}
+
static inline u32 i915_ggtt_offset(const struct i915_vma *vma)
{
GEM_BUG_ON(!i915_vma_is_ggtt(vma));
diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
index 930f9cb1661f..37c9a5706f69 100644
--- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
+++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
@@ -206,6 +206,7 @@ struct drm_i915_private *mock_gem_device(void)
INIT_LIST_HEAD(&i915->gt.active_rings);
INIT_LIST_HEAD(&i915->gt.closed_vma);
+ spin_lock_init(&i915->gt.closed_lock);
mutex_lock(&i915->drm.struct_mutex);
--
2.20.1
More information about the Intel-gfx
mailing list