| Message ID | 20180911125022.2555-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Nuke struct_mutex from context_setparam | expand |
Quoting Chris Wilson (2018-09-11 13:50:22) > 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. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> To reduce the amount of queasiness here, we should split off the ctx->flags and ctx->user_flags. -Chris
On 11/09/2018 14:14, Chris Wilson wrote: > Quoting Chris Wilson (2018-09-11 13:50:22) >> 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. >> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > To reduce the amount of queasiness here, we should split off the > ctx->flags and ctx->user_flags. I think all that would be needed for a start is to handle PARAM_NO_ZEROMAP with the same set/clear_bit approach as the rest. With that it would all be atomic for all theoretical scenarios. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 747b8170a15a..10fd742b591d 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -896,16 +896,12 @@ 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; @@ -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; }
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. 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 | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)