| Message ID | 20180911132206.23032-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Nuke struct_mutex from context_setparam | expand |
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@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> Regards, Tvrtko
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@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@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@intel.com> And pushed. Hopefully this simplifies handling struct_mutex recursion! -Chris
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;
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. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@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(-)