Re: [PATCH] drm/i915: Nuke struct_mutex from context_setparam

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 11/09/2018 14:22, Chris Wilson wrote:
> Userspace should be free to race against itself and shoot itself in
> the foot if it so desires to adjust a parameter at the same time as
> submitting a batch to that context. As such, the struct_mutex in context
> setparam is only being used to serialise userspace against itself and
> not for any protection of internal structs and so is superfluous.
>
> v2: Separate user_flags from internal flags to reduce chance of
> interference; and use locked bit ops for user updates.
Okay, email criss-cross!

> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c    | 20 +++++--------
>   drivers/gpu/drm/i915/i915_gem_context.h    | 33 +++++++++++++---------
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
>   3 files changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 747b8170a15a..f772593b99ab 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -862,7 +862,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 = test_bit(UCONTEXT_NO_ZEROMAP, &ctx->user_flags);
>   		break;
>   	case I915_CONTEXT_PARAM_GTT_SIZE:
>   		if (ctx->ppgtt)
> @@ -896,27 +896,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)
> +			set_bit(UCONTEXT_NO_ZEROMAP, &ctx->user_flags);
> +		else
> +			clear_bit(UCONTEXT_NO_ZEROMAP, &ctx->user_flags);
>   		break;
>   	case I915_CONTEXT_PARAM_NO_ERROR_CAPTURE:
>   		if (args->size)
> @@ -960,9 +956,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 e09673ca731d..08165f6a0a84 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -116,16 +116,21 @@ struct i915_gem_context {
>   	 */
>   	struct rcu_head rcu;
>   
> +	/**
> +	 * @user_flags: small set of booleans controlled by the user
> +	 */
> +	unsigned long user_flags;
> +#define UCONTEXT_NO_ZEROMAP		0
> +#define UCONTEXT_NO_ERROR_CAPTURE	1
> +#define UCONTEXT_BANNABLE		2
> +
>   	/**
>   	 * @flags: small set of booleans
>   	 */
>   	unsigned long flags;
> -#define CONTEXT_NO_ZEROMAP		BIT(0)
> -#define CONTEXT_NO_ERROR_CAPTURE	1
> -#define CONTEXT_CLOSED			2
> -#define CONTEXT_BANNABLE		3
> -#define CONTEXT_BANNED			4
> -#define CONTEXT_FORCE_SINGLE_SUBMISSION	5
> +#define CONTEXT_BANNED			0
> +#define CONTEXT_CLOSED			1
> +#define CONTEXT_FORCE_SINGLE_SUBMISSION	2
>   
>   	/**
>   	 * @hw_id: - unique identifier for the context
> @@ -209,37 +214,37 @@ static inline bool i915_gem_context_is_closed(const struct i915_gem_context *ctx
>   static inline void i915_gem_context_set_closed(struct i915_gem_context *ctx)
>   {
>   	GEM_BUG_ON(i915_gem_context_is_closed(ctx));
> -	__set_bit(CONTEXT_CLOSED, &ctx->flags);
> +	set_bit(CONTEXT_CLOSED, &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);
> +	return test_bit(UCONTEXT_NO_ERROR_CAPTURE, &ctx->user_flags);
>   }
>   
>   static inline void i915_gem_context_set_no_error_capture(struct i915_gem_context *ctx)
>   {
> -	__set_bit(CONTEXT_NO_ERROR_CAPTURE, &ctx->flags);
> +	set_bit(UCONTEXT_NO_ERROR_CAPTURE, &ctx->user_flags);
>   }
>   
>   static inline void i915_gem_context_clear_no_error_capture(struct i915_gem_context *ctx)
>   {
> -	__clear_bit(CONTEXT_NO_ERROR_CAPTURE, &ctx->flags);
> +	clear_bit(UCONTEXT_NO_ERROR_CAPTURE, &ctx->user_flags);
>   }
>   
>   static inline bool i915_gem_context_is_bannable(const struct i915_gem_context *ctx)
>   {
> -	return test_bit(CONTEXT_BANNABLE, &ctx->flags);
> +	return test_bit(UCONTEXT_BANNABLE, &ctx->user_flags);
>   }
>   
>   static inline void i915_gem_context_set_bannable(struct i915_gem_context *ctx)
>   {
> -	__set_bit(CONTEXT_BANNABLE, &ctx->flags);
> +	set_bit(UCONTEXT_BANNABLE, &ctx->user_flags);
>   }
>   
>   static inline void i915_gem_context_clear_bannable(struct i915_gem_context *ctx)
>   {
> -	__clear_bit(CONTEXT_BANNABLE, &ctx->flags);
> +	clear_bit(UCONTEXT_BANNABLE, &ctx->user_flags);
>   }
>   
>   static inline bool i915_gem_context_is_banned(const struct i915_gem_context *ctx)
> @@ -249,7 +254,7 @@ static inline bool i915_gem_context_is_banned(const struct i915_gem_context *ctx
>   
>   static inline void i915_gem_context_set_banned(struct i915_gem_context *ctx)
>   {
> -	__set_bit(CONTEXT_BANNED, &ctx->flags);
> +	set_bit(CONTEXT_BANNED, &ctx->flags);
>   }
>   
>   static inline bool i915_gem_context_force_single_submission(const struct i915_gem_context *ctx)
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 22b4cb775576..aaa8a26aae86 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -743,7 +743,7 @@ static int eb_select_context(struct i915_execbuffer *eb)
>   	}
>   
>   	eb->context_flags = 0;
> -	if (ctx->flags & CONTEXT_NO_ZEROMAP)
> +	if (test_bit(UCONTEXT_NO_ZEROMAP, &ctx->user_flags))
>   		eb->context_flags |= __EXEC_OBJECT_NEEDS_BIAS;
>   
>   	return 0;

LGTM.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

Regards,

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