| Message ID | 1430997295-8336-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 07, 2015 at 12:14:55PM +0100, Chris Wilson wrote: > Reading a single value from the object, the locking only provides futile > protection against userspace races. The locking is useless so remove it. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_gem.c | 17 ++++------------- > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 2b2b74dbb446..d071d0af2a6c 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3983,17 +3983,10 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data, > { > struct drm_i915_gem_caching *args = data; > struct drm_i915_gem_object *obj; > - int ret; > - > - ret = i915_mutex_lock_interruptible(dev); > - if (ret) > - return ret; > > obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); > - if (&obj->base == NULL) { > - ret = -ENOENT; > - goto unlock; > - } > + if (&obj->base == NULL) > + return -ENOENT; > > switch (obj->cache_level) { Wrap this in ACCESS_ONCE, just for documentation purpose? Can do while applying if you ack. -Daniel > case I915_CACHE_LLC: > @@ -4010,10 +4003,8 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data, > break; > } > > - drm_gem_object_unreference(&obj->base); > -unlock: > - mutex_unlock(&dev->struct_mutex); > - return ret; > + drm_gem_object_unreference_unlocked(&obj->base); > + return 0; > } > > int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data, > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, May 07, 2015 at 03:22:41PM +0200, Daniel Vetter wrote: > On Thu, May 07, 2015 at 12:14:55PM +0100, Chris Wilson wrote: > > Reading a single value from the object, the locking only provides futile > > protection against userspace races. The locking is useless so remove it. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/i915_gem.c | 17 ++++------------- > > 1 file changed, 4 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 2b2b74dbb446..d071d0af2a6c 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -3983,17 +3983,10 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data, > > { > > struct drm_i915_gem_caching *args = data; > > struct drm_i915_gem_object *obj; > > - int ret; > > - > > - ret = i915_mutex_lock_interruptible(dev); > > - if (ret) > > - return ret; > > > > obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); > > - if (&obj->base == NULL) { > > - ret = -ENOENT; > > - goto unlock; > > - } > > + if (&obj->base == NULL) > > + return -ENOENT; > > > > switch (obj->cache_level) { > > Wrap this in ACCESS_ONCE, just for documentation purpose? Can do while > applying if you ack. I was going to but I thought obj->cache_level was another bitfield... It is :| -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > Reading a single value from the object, the locking only provides futile > protection against userspace races. The locking is useless so remove it. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 17 ++++------------- > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 2b2b74dbb446..d071d0af2a6c 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3983,17 +3983,10 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data, > { > struct drm_i915_gem_caching *args = data; > struct drm_i915_gem_object *obj; > - int ret; > - > - ret = i915_mutex_lock_interruptible(dev); > - if (ret) > - return ret; > > obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); > - if (&obj->base == NULL) { > - ret = -ENOENT; > - goto unlock; > - } > + if (&obj->base == NULL) > + return -ENOENT; > > switch (obj->cache_level) { > case I915_CACHE_LLC: > @@ -4010,10 +4003,8 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data, > break; > } > > - drm_gem_object_unreference(&obj->base); > -unlock: > - mutex_unlock(&dev->struct_mutex); > - return ret; > + drm_gem_object_unreference_unlocked(&obj->base); > + return 0; > } > > int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data, > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6343
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV -1 199/199 198/199
ILK 302/302 302/302
SNB 316/316 316/316
IVB 342/342 342/342
BYT 286/286 286/286
BDW 321/321 321/321
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*PNV igt@gem_userptr_blits@forked-sync-swapping-interruptible PASS(1) INIT(1)
Note: You need to pay more attention to line start with '*'
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 2b2b74dbb446..d071d0af2a6c 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3983,17 +3983,10 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data, { struct drm_i915_gem_caching *args = data; struct drm_i915_gem_object *obj; - int ret; - - ret = i915_mutex_lock_interruptible(dev); - if (ret) - return ret; obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle)); - if (&obj->base == NULL) { - ret = -ENOENT; - goto unlock; - } + if (&obj->base == NULL) + return -ENOENT; switch (obj->cache_level) { case I915_CACHE_LLC: @@ -4010,10 +4003,8 @@ int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data, break; } - drm_gem_object_unreference(&obj->base); -unlock: - mutex_unlock(&dev->struct_mutex); - return ret; + drm_gem_object_unreference_unlocked(&obj->base); + return 0; } int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
Reading a single value from the object, the locking only provides futile protection against userspace races. The locking is useless so remove it. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-)