[PATCH] drm/i915: Remove struct_mutex locking from i915_gem_context_setparam

* [PATCH] drm/i915: Remove struct_mutex locking from i915_gem_context_setparam
@ 2017-10-22 12:18 Chris Wilson
  2017-10-22 12:38 ` ✗ Fi.CI.BAT: failure for " Patchwork
  2017-10-22 13:35 ` ✗ Fi.CI.IGT: warning " Patchwork
  0 siblings, 2 replies; 3+ messages in thread
From: Chris Wilson @ 2017-10-22 12:18 UTC (permalink / raw)
  To: intel-gfx

Currently all setparam operations on the context can be done unlocked;
races with concurrent operations using those params are an issue for
userspace (concurrently trying to change parameters and execution with
them is undefined as to which parameters should be used). Later we may
need to change state that requires the struct_mutex, but for now we are
just changing values local to the context.

The impact is tiny since setparam is an infrequent runtime operation;
the context is expected to be configured just after creation and then
rarely changed. Just within igt where we do try many operations in
parallel while poking the context do we observe any effect.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c    | 20 +++++++-------------
 drivers/gpu/drm/i915/i915_gem_context.h    | 17 ++++++++++++++++-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 5bf96a258509..92b08a06318c 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -1054,7 +1054,7 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
 		ret = -EINVAL;
 		break;
 	case I915_CONTEXT_PARAM_NO_ZEROMAP:
-		args->value = ctx->flags & CONTEXT_NO_ZEROMAP;
+		args->value = i915_gem_context_no_zeromap(ctx);
 		break;
 	case I915_CONTEXT_PARAM_GTT_SIZE:
 		if (ctx->ppgtt)
@@ -1088,27 +1088,23 @@ int i915_gem_context_setparam_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;
+	int ret = 0;
 
 	ctx = i915_gem_context_lookup(file_priv, args->ctx_id);
 	if (!ctx)
 		return -ENOENT;
 
-	ret = i915_mutex_lock_interruptible(dev);
-	if (ret)
-		goto out;
-
 	switch (args->param) {
 	case I915_CONTEXT_PARAM_BAN_PERIOD:
 		ret = -EINVAL;
 		break;
 	case I915_CONTEXT_PARAM_NO_ZEROMAP:
-		if (args->size) {
+		if (args->size)
 			ret = -EINVAL;
-		} else {
-			ctx->flags &= ~CONTEXT_NO_ZEROMAP;
-			ctx->flags |= args->value ? CONTEXT_NO_ZEROMAP : 0;
-		}
+		else if (args->value)
+			i915_gem_context_set_no_zeromap(ctx);
+		else
+			i915_gem_context_clear_no_zeromap(ctx);
 		break;
 	case I915_CONTEXT_PARAM_NO_ERROR_CAPTURE:
 		if (args->size)
@@ -1152,9 +1148,7 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
 		ret = -EINVAL;
 		break;
 	}
-	mutex_unlock(&dev->struct_mutex);
 
-out:
 	i915_gem_context_put(ctx);
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index 44688e22a5c2..100ba700c565 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -109,7 +109,7 @@ struct i915_gem_context {
 	 * @flags: small set of booleans
 	 */
 	unsigned long flags;
-#define CONTEXT_NO_ZEROMAP		BIT(0)
+#define CONTEXT_NO_ZEROMAP		0
 #define CONTEXT_NO_ERROR_CAPTURE	1
 #define CONTEXT_CLOSED			2
 #define CONTEXT_BANNABLE		3
@@ -205,6 +205,21 @@ static inline void i915_gem_context_set_closed(struct i915_gem_context *ctx)
 	__set_bit(CONTEXT_CLOSED, &ctx->flags);
 }
 
+static inline bool i915_gem_context_no_zeromap(const struct i915_gem_context *ctx)
+{
+	return test_bit(CONTEXT_NO_ZEROMAP, &ctx->flags);
+}
+
+static inline void i915_gem_context_set_no_zeromap(struct i915_gem_context *ctx)
+{
+	__set_bit(CONTEXT_NO_ZEROMAP, &ctx->flags);
+}
+
+static inline void i915_gem_context_clear_no_zeromap(struct i915_gem_context *ctx)
+{
+	__clear_bit(CONTEXT_NO_ZEROMAP, &ctx->flags);
+}
+
 static inline bool i915_gem_context_no_error_capture(const struct i915_gem_context *ctx)
 {
 	return test_bit(CONTEXT_NO_ERROR_CAPTURE, &ctx->flags);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 3d7190764f10..a111a2f9d688 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -671,7 +671,7 @@ static int eb_select_context(struct i915_execbuffer *eb)
 	eb->vm = ctx->ppgtt ? &ctx->ppgtt->base : &eb->i915->ggtt.base;
 
 	eb->context_flags = 0;
-	if (ctx->flags & CONTEXT_NO_ZEROMAP)
+	if (i915_gem_context_no_zeromap(ctx))
 		eb->context_flags |= __EXEC_OBJECT_NEEDS_BIAS;
 
 	return 0;
-- 
2.15.0.rc1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 3+ messages in thread