[PATCH 26/30] drm/i915: Enable rcu-only context lookups
Chris Wilson
chris at chris-wilson.co.uk
Mon May 29 16:10:26 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 | 77 ++++++++++++++----------------
drivers/gpu/drm/i915/i915_gem_context.h | 5 ++
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 21 ++++----
4 files changed, 64 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bb61cbd33e3f..baf0516ab6b2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3500,15 +3500,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 2ad78bb50137..198b0064a3d3 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -187,7 +187,7 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
list_del(&ctx->link);
ida_simple_remove(&ctx->i915->contexts.hw_ida, ctx->hw_id);
- kfree(ctx);
+ kfree_rcu(ctx, rcu);
}
static void contexts_free(struct drm_i915_private *i915)
@@ -1021,20 +1021,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;
}
@@ -1044,17 +1043,11 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
struct drm_i915_file_private *file_priv = file->driver_priv;
struct drm_i915_gem_context_param *args = data;
struct i915_gem_context *ctx;
- int ret;
-
- ret = i915_mutex_lock_interruptible(dev);
- if (ret)
- return ret;
+ int ret = 0;
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) {
@@ -1085,8 +1078,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;
}
@@ -1098,15 +1091,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:
@@ -1164,6 +1155,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;
}
@@ -1181,27 +1174,31 @@ 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_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index 61146f4aa168..04320f80f9f4 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -100,6 +100,11 @@ struct i915_gem_context {
struct kref ref;
/**
+ * @rcu: rcu_head for deferred freeing.
+ */
+ struct rcu_head rcu;
+
+ /**
* @flags: small set of booleans
*/
unsigned long flags;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index b30725a8ef46..a0d0ea87a72d 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -666,16 +666,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;
@@ -2128,7 +2129,6 @@ i915_gem_do_execbuffer(struct drm_device *dev,
if (DBG_FORCE_RELOC || !(args->flags & I915_EXEC_NO_RELOC))
args->flags |= __EXEC_HAS_RELOC;
eb.exec = exec;
- eb.ctx = NULL;
eb.vma = (struct i915_vma **)(exec + args->buffer_count + 1);
eb.flags = (unsigned int *)(eb.vma + args->buffer_count + 1);
eb.invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
@@ -2185,6 +2185,10 @@ i915_gem_do_execbuffer(struct drm_device *dev,
if (eb_create(&eb))
return -ENOMEM;
+ err = eb_select_context(&eb);
+ if (unlikely(err))
+ goto err_destroy;
+
/*
* Take a local wakeref for preparing to dispatch the execbuf as
* we expect to access the hardware fairly frequently in the
@@ -2193,14 +2197,11 @@ i915_gem_do_execbuffer(struct drm_device *dev,
* 100ms.
*/
intel_runtime_pm_get(eb.i915);
+
err = i915_mutex_lock_interruptible(dev);
if (err)
goto err_rpm;
- err = eb_select_context(&eb);
- if (unlikely(err))
- goto err_unlock;
-
err = eb_relocate(&eb);
if (err)
/*
@@ -2336,11 +2337,11 @@ i915_gem_do_execbuffer(struct drm_device *dev,
err_vma:
if (eb.exec)
eb_release_vmas(&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-trybot
mailing list