[Intel-gfx] [PATCH] drm/i915: Nuke struct_mutex from context_setparam
Chris Wilson
chris at chris-wilson.co.uk
Tue Sep 11 19:49:51 UTC 2018
Quoting Tvrtko Ursulin (2018-09-11 14:34:03)
>
> 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 at chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > ---
> > 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 at intel.com>
And pushed. Hopefully this simplifies handling struct_mutex recursion!
-Chris
More information about the Intel-gfx
mailing list