[Intel-gfx] [PATCH 3/3] drm/i915: Enable rcu-only context lookups
Chris Wilson
chris at chris-wilson.co.uk
Wed Mar 29 18:28:21 UTC 2017
Whilst the contents of the context is still protected by the big
struct_mutex, this is not much of an improvement. It is just one tiny
step towards reducing our BKL.
Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_drv.h | 16 +++++--
drivers/gpu/drm/i915/i915_gem_context.c | 74 +++++++++++++++---------------
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 21 +++++----
3 files changed, 58 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index aa5b7c8969ab..0d68966673bd 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3505,15 +3505,21 @@ void i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj,
struct sg_table *pages);
static inline struct i915_gem_context *
+__i915_gem_context_lookup_rcu(struct drm_i915_file_private *file_priv, u32 id)
+{
+ return idr_find(&file_priv->context_idr, id);
+}
+
+static inline struct i915_gem_context *
i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id)
{
struct i915_gem_context *ctx;
- lockdep_assert_held(&file_priv->dev_priv->drm.struct_mutex);
-
- ctx = idr_find(&file_priv->context_idr, id);
- if (!ctx)
- return ERR_PTR(-ENOENT);
+ rcu_read_lock();
+ ctx = __i915_gem_context_lookup_rcu(file_priv, id);
+ if (ctx && !kref_get_unless_zero(&ctx->ref))
+ ctx = NULL;
+ rcu_read_unlock();
return ctx;
}
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 3df050ee6ce4..d53efa107c03 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -225,6 +225,8 @@ static void free_contexts(struct work_struct *work)
while ((freed = llist_del_all(&dev_priv->contexts.free_list))) {
struct i915_gem_context *ctx, *cn;
+ synchronize_rcu();
+
mutex_lock(&dev_priv->drm.struct_mutex);
llist_for_each_entry_safe(ctx, cn, freed, free_link)
i915_gem_context_free(ctx);
@@ -1112,20 +1114,19 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
if (args->ctx_id == DEFAULT_CONTEXT_HANDLE)
return -ENOENT;
- ret = i915_mutex_lock_interruptible(dev);
- if (ret)
- return ret;
-
ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
- if (IS_ERR(ctx)) {
- mutex_unlock(&dev->struct_mutex);
- return PTR_ERR(ctx);
- }
+ if (!ctx)
+ return -ENOENT;
+
+ ret = mutex_lock_interruptible(&dev->struct_mutex);
+ if (ret)
+ goto out;
__destroy_hw_context(ctx, file_priv);
mutex_unlock(&dev->struct_mutex);
- DRM_DEBUG("HW context %d destroyed\n", args->ctx_id);
+out:
+ i915_gem_context_put(ctx);
return 0;
}
@@ -1137,15 +1138,9 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
struct i915_gem_context *ctx;
int ret;
- ret = i915_mutex_lock_interruptible(dev);
- if (ret)
- return ret;
-
ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
- if (IS_ERR(ctx)) {
- mutex_unlock(&dev->struct_mutex);
- return PTR_ERR(ctx);
- }
+ if (!ctx)
+ return -ENOENT;
args->size = 0;
switch (args->param) {
@@ -1176,8 +1171,8 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
ret = -EINVAL;
break;
}
- mutex_unlock(&dev->struct_mutex);
+ i915_gem_context_put(ctx);
return ret;
}
@@ -1189,15 +1184,13 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
struct i915_gem_context *ctx;
int ret;
+ ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
+ if (!ctx)
+ return -ENOENT;
+
ret = i915_mutex_lock_interruptible(dev);
if (ret)
- return ret;
-
- ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
- if (IS_ERR(ctx)) {
- mutex_unlock(&dev->struct_mutex);
- return PTR_ERR(ctx);
- }
+ goto out;
switch (args->param) {
case I915_CONTEXT_PARAM_BAN_PERIOD:
@@ -1252,6 +1245,8 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
}
mutex_unlock(&dev->struct_mutex);
+out:
+ i915_gem_context_put(ctx);
return ret;
}
@@ -1269,27 +1264,30 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
if (args->ctx_id == DEFAULT_CONTEXT_HANDLE && !capable(CAP_SYS_ADMIN))
return -EPERM;
- ret = i915_mutex_lock_interruptible(dev);
- if (ret)
- return ret;
+ ret = -ENOENT;
+ rcu_read_lock();
+ ctx = __i915_gem_context_lookup_rcu(file->driver_priv, args->ctx_id);
+ if (!ctx)
+ goto out;
- ctx = i915_gem_context_lookup(file->driver_priv, args->ctx_id);
- if (IS_ERR(ctx)) {
- mutex_unlock(&dev->struct_mutex);
- return PTR_ERR(ctx);
- }
+ /* We opt for unserialised reads here. This may result in tearing
+ * in the extremely unlikely event of a GPU hang on this context
+ * as we are querying them. If we need that extra layer of protection,
+ * we should wrap the hangstats with a seqlock.
+ */
if (capable(CAP_SYS_ADMIN))
args->reset_count = i915_reset_count(&dev_priv->gpu_error);
else
args->reset_count = 0;
- args->batch_active = ctx->guilty_count;
- args->batch_pending = ctx->active_count;
-
- mutex_unlock(&dev->struct_mutex);
+ args->batch_active = READ_ONCE(ctx->guilty_count);
+ args->batch_pending = READ_ONCE(ctx->active_count);
- return 0;
+ ret = 0;
+out:
+ rcu_read_unlock();
+ return ret;
}
#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 017e27b7c300..26d4b450b7f8 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -597,16 +597,17 @@ static int eb_select_context(struct i915_execbuffer *eb)
struct i915_gem_context *ctx;
ctx = i915_gem_context_lookup(eb->file->driver_priv, eb->args->rsvd1);
- if (unlikely(IS_ERR(ctx)))
- return PTR_ERR(ctx);
+ if (unlikely(!ctx))
+ return -ENOENT;
if (unlikely(i915_gem_context_is_banned(ctx))) {
DRM_DEBUG("Context %u tried to submit while banned\n",
ctx->user_handle);
+ i915_gem_context_put(ctx);
return -EIO;
}
- eb->ctx = i915_gem_context_get(ctx);
+ eb->ctx = ctx;
eb->vm = ctx->ppgtt ? &ctx->ppgtt->base : &eb->i915->ggtt.base;
eb->context_flags = 0;
@@ -2040,7 +2041,6 @@ i915_gem_do_execbuffer(struct drm_device *dev,
if ((args->flags & I915_EXEC_NO_RELOC) == 0)
args->flags |= __EXEC_HAS_RELOC;
eb.exec = exec;
- eb.ctx = NULL;
eb.invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
if (USES_FULL_PPGTT(eb.i915))
eb.invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
@@ -2095,6 +2095,10 @@ i915_gem_do_execbuffer(struct drm_device *dev,
if (eb_create(&eb))
return -ENOMEM;
+ ret = eb_select_context(&eb);
+ if (unlikely(ret))
+ goto err_destroy;
+
/* Take a local wakeref for preparing to dispatch the execbuf as
* we expect to access the hardware fairly frequently in the
* process. Upon first dispatch, we acquire another prolonged
@@ -2102,14 +2106,11 @@ i915_gem_do_execbuffer(struct drm_device *dev,
* 100ms.
*/
intel_runtime_pm_get(eb.i915);
+
ret = i915_mutex_lock_interruptible(dev);
if (ret)
goto err_rpm;
- ret = eb_select_context(&eb);
- if (unlikely(ret))
- goto err_unlock;
-
ret = eb_lookup_vmas(&eb);
if (likely(!ret && args->flags & __EXEC_HAS_RELOC))
ret = eb_relocate(&eb);
@@ -2242,11 +2243,11 @@ i915_gem_do_execbuffer(struct drm_device *dev,
i915_vma_unpin(eb.batch);
err_vma:
eb_release_vma(&eb);
- i915_gem_context_put(eb.ctx);
-err_unlock:
mutex_unlock(&dev->struct_mutex);
err_rpm:
intel_runtime_pm_put(eb.i915);
+ i915_gem_context_put(eb.ctx);
+err_destroy:
eb_destroy(&eb);
if (out_fence_fd != -1)
put_unused_fd(out_fence_fd);
--
2.11.0
More information about the Intel-gfx
mailing list