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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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>

And pushed. Hopefully this simplifies handling struct_mutex recursion!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux