Hi all,
I wanted to take another look at struct_mutex usage in modern (gem) drivers and noticed that for a fair lot we're very to be completely struct_mutex free.
This pile here is the the simple part, which mostly just removes code and mutex_lock/unlock calls. All the patches here are independent and can be merged in any order whatsoever. My plan is to send out a pull request for all those not picked up by driver maintainers in 2-3 weeks or so, assuming no one complains.
Of course review & comments still very much welcome.
The more tricky 2nd part of this (and that one's not yet done) is to rework the gem mmap handler to use the same kref_get_unless_zero trick as ttm. With that there's no core requirement to hold struct_mutex over the final unref, which means we can make that one lockless. I plan to add a gem_object_free_unlocked for all the drivers which don't have any need for this lock.
Also there's a few more drivers which can be made struct_mutex free easily, I'll propably stitch together poc patches for those.
Cheers, Daniel
Daniel Vetter (18): drm/gem: rip out drm vma accounting for gem mmaps drm/cma-helper: Fix locking in drm_fb_cma_debugfs_show drm/gem: Be more friendly with locking checks drm/ast: Don't grab dev->struct_mutex for in mmap offset ioctl drm/bochs: Don't grab dev->struct_mutex for in mmap offset ioctl drm/mga200g: Don't grab dev->struct_mutex for in mmap offset ioctl drm/mga200g: Hold a proper reference for cursor_set drm/cirrus: Don't grab dev->struct_mutex for in mmap offset ioctl drm/cma-helper: Don't grab dev->struct_mutex for in mmap offset ioctl drm/rockchip: Don't grab dev->struct_mutex for in mmap offset ioctl drm/armada: Don't grab dev->struct_mutex for in mmap offset ioctl drm/nouveau: Don't take dev->struct_mutex in fbcon init drm/nouveau: Don't take dev->struct_mutex in ttm_fini drm/qxl: Don't take dev->struct_mutex in bo_force_delete drm/radeon: Don't take dev->struct_mutex in bo_force_delete drm/radeon: Don't take dev->struct_mutex in pm functions drm/amdgpu: Don't take dev->struct_mutex in bo_force_delete drm/amdgpu: don't grab dev->struct_mutex in pm functions
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 +--- drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 -- drivers/gpu/drm/armada/armada_gem.c | 11 ++++------- drivers/gpu/drm/ast/ast_main.c | 16 +++++----------- drivers/gpu/drm/bochs/bochs_mm.c | 16 ++++------------ drivers/gpu/drm/cirrus/cirrus_main.c | 15 ++++----------- drivers/gpu/drm/drm_fb_cma_helper.c | 16 ++-------------- drivers/gpu/drm/drm_gem.c | 13 ++----------- drivers/gpu/drm/drm_gem_cma_helper.c | 9 +-------- drivers/gpu/drm/mgag200/mgag200_cursor.c | 22 ++++++++++------------ drivers/gpu/drm/mgag200/mgag200_main.c | 16 ++++------------ drivers/gpu/drm/nouveau/nouveau_fbcon.c | 11 +++-------- drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 -- drivers/gpu/drm/qxl/qxl_object.c | 4 +--- drivers/gpu/drm/radeon/radeon_object.c | 4 +--- drivers/gpu/drm/radeon/radeon_pm.c | 5 ----- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 13 ++++--------- 17 files changed, 46 insertions(+), 133 deletions(-)
Doesn't really add anything which can't be figured out through proc files. And more clearly separates the new gem mmap handling code from the old drm maps mmap handling code, which is surely a good thing.
Cc: Martin Peres martin.peres@free.fr Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_gem.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 16a164770713..27a4228b4343 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -778,22 +778,14 @@ void drm_gem_vm_open(struct vm_area_struct *vma) struct drm_gem_object *obj = vma->vm_private_data;
drm_gem_object_reference(obj); - - mutex_lock(&obj->dev->struct_mutex); - drm_vm_open_locked(obj->dev, vma); - mutex_unlock(&obj->dev->struct_mutex); } EXPORT_SYMBOL(drm_gem_vm_open);
void drm_gem_vm_close(struct vm_area_struct *vma) { struct drm_gem_object *obj = vma->vm_private_data; - struct drm_device *dev = obj->dev;
- mutex_lock(&dev->struct_mutex); - drm_vm_close_locked(obj->dev, vma); - drm_gem_object_unreference(obj); - mutex_unlock(&dev->struct_mutex); + drm_gem_object_unreference_unlocked(obj); } EXPORT_SYMBOL(drm_gem_vm_close);
@@ -850,7 +842,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, */ drm_gem_object_reference(obj);
- drm_vm_open_locked(dev, vma); return 0; } EXPORT_SYMBOL(drm_gem_mmap_obj);
On Thu, Jul 09, 2015 at 11:32:33PM +0200, Daniel Vetter wrote:
Doesn't really add anything which can't be figured out through proc files. And more clearly separates the new gem mmap handling code from the old drm maps mmap handling code, which is surely a good thing.
Cc: Martin Peres martin.peres@free.fr Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
Long have I wanted vmalist dead. -Chris
On Sat, Jul 11, 2015 at 10:11:43PM +0100, Chris Wilson wrote:
On Thu, Jul 09, 2015 at 11:32:33PM +0200, Daniel Vetter wrote:
Doesn't really add anything which can't be figured out through proc files. And more clearly separates the new gem mmap handling code from the old drm maps mmap handling code, which is surely a good thing.
Cc: Martin Peres martin.peres@free.fr Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
Applied to topic/drm-misc, thanks for the review. -Daniel
On Thu, Jul 09, 2015 at 11:32:33PM +0200, Daniel Vetter wrote:
Doesn't really add anything which can't be figured out through proc files. And more clearly separates the new gem mmap handling code from the old drm maps mmap handling code, which is surely a good thing.
Cc: Martin Peres martin.peres@free.fr Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_gem.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
Doesn't this mean that the "vma" debugfs file will now be empty for GEM drivers?
Thierry
This function takes two locks, both of them the wrong ones. This wasn't an oversight from my fb locking rework since both patches landed in parallel. We really only need fb_lock when walking that list, since everything we can reach from that is refcounted properly already.
Cc: Rob Clark robdclark@gmail.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_fb_cma_helper.c | 16 ++-------------- drivers/gpu/drm/drm_gem_cma_helper.c | 2 -- 2 files changed, 2 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 5c1aca443e54..46c4e6ee1cb1 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -209,23 +209,11 @@ int drm_fb_cma_debugfs_show(struct seq_file *m, void *arg) struct drm_info_node *node = (struct drm_info_node *) m->private; struct drm_device *dev = node->minor->dev; struct drm_framebuffer *fb; - int ret; - - ret = mutex_lock_interruptible(&dev->mode_config.mutex); - if (ret) - return ret; - - ret = mutex_lock_interruptible(&dev->struct_mutex); - if (ret) { - mutex_unlock(&dev->mode_config.mutex); - return ret; - }
+ mutex_lock(&dev->mode_config.fb_lock); list_for_each_entry(fb, &dev->mode_config.fb_list, head) drm_fb_cma_describe(fb, m); - - mutex_unlock(&dev->struct_mutex); - mutex_unlock(&dev->mode_config.mutex); + mutex_unlock(&dev->mode_config.fb_lock);
return 0; } diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index bd75f303da63..ca2ed7e7b595 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -384,8 +384,6 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct drm_device *dev = obj->dev; uint64_t off;
- WARN_ON(!mutex_is_locked(&dev->struct_mutex)); - off = drm_vma_node_start(&obj->vma_node);
seq_printf(m, "%2d (%2d) %08llx %pad %p %zu",
On Thu, Jul 09, 2015 at 11:32:34PM +0200, Daniel Vetter wrote:
This function takes two locks, both of them the wrong ones. This wasn't an oversight from my fb locking rework since both patches landed in parallel. We really only need fb_lock when walking that list, since everything we can reach from that is refcounted properly already.
Cc: Rob Clark robdclark@gmail.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_fb_cma_helper.c | 16 ++-------------- drivers/gpu/drm/drm_gem_cma_helper.c | 2 -- 2 files changed, 2 insertions(+), 16 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
BUG_ON kills the driver, WARN_ON is much friendly. And usually nothing bad happens when the locking is lightly busted.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 27a4228b4343..3c2d4abd71c5 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -766,7 +766,7 @@ drm_gem_object_free(struct kref *kref) struct drm_gem_object *obj = (struct drm_gem_object *) kref; struct drm_device *dev = obj->dev;
- BUG_ON(!mutex_is_locked(&dev->struct_mutex)); + WARN_ON(!mutex_is_locked(&dev->struct_mutex));
if (dev->driver->gem_free_object != NULL) dev->driver->gem_free_object(obj);
On Thu, Jul 09, 2015 at 11:32:35PM +0200, Daniel Vetter wrote:
BUG_ON kills the driver, WARN_ON is much friendly. And usually nothing bad happens when the locking is lightly busted.
s/much friendly/much friendlier/, s/lightly/slightly/?
Otherwise:
Reviewed-by: Thierry Reding treding@nvidia.com
On Mon, Aug 10, 2015 at 12:42:30PM +0200, Thierry Reding wrote:
On Thu, Jul 09, 2015 at 11:32:35PM +0200, Daniel Vetter wrote:
BUG_ON kills the driver, WARN_ON is much friendly. And usually nothing bad happens when the locking is lightly busted.
s/much friendly/much friendlier/, s/lightly/slightly/?
Yeah, changed.
Otherwise:
Reviewed-by: Thierry Reding treding@nvidia.com
Thanks for your review, I've applied the patches to topic/drm-misc except armada (needs to be split), one nouveau patch (superseeded meanwhile by Archit's work), radeon/amdgpu (I'll ping Alex) and the two tegra ones.
Thanks, Daniel
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/ast/ast_main.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c index 035dacc93382..838217f8ce7d 100644 --- a/drivers/gpu/drm/ast/ast_main.c +++ b/drivers/gpu/drm/ast/ast_main.c @@ -571,24 +571,18 @@ ast_dumb_mmap_offset(struct drm_file *file, uint64_t *offset) { struct drm_gem_object *obj; - int ret; struct ast_bo *bo;
- mutex_lock(&dev->struct_mutex); obj = drm_gem_object_lookup(dev, file, handle); - if (obj == NULL) { - ret = -ENOENT; - goto out_unlock; - } + if (obj == NULL) + return -ENOENT;
bo = gem_to_ast_bo(obj); *offset = ast_bo_mmap_offset(bo);
- drm_gem_object_unreference(obj); - ret = 0; -out_unlock: - mutex_unlock(&dev->struct_mutex); - return ret; + drm_gem_object_unreference_unlocked(obj); + + return 0;
}
On Thu, Jul 09, 2015 at 11:32:36PM +0200, Daniel Vetter wrote:
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/ast/ast_main.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/bochs/bochs_mm.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bochs/bochs_mm.c b/drivers/gpu/drm/bochs/bochs_mm.c index 66286ff518d4..f69e6bf9bb0e 100644 --- a/drivers/gpu/drm/bochs/bochs_mm.c +++ b/drivers/gpu/drm/bochs/bochs_mm.c @@ -454,25 +454,17 @@ int bochs_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, uint32_t handle, uint64_t *offset) { struct drm_gem_object *obj; - int ret; struct bochs_bo *bo;
- mutex_lock(&dev->struct_mutex); obj = drm_gem_object_lookup(dev, file, handle); - if (obj == NULL) { - ret = -ENOENT; - goto out_unlock; - } + if (obj == NULL) + return -ENOENT;
bo = gem_to_bochs_bo(obj); *offset = bochs_bo_mmap_offset(bo);
- drm_gem_object_unreference(obj); - ret = 0; -out_unlock: - mutex_unlock(&dev->struct_mutex); - return ret; - + drm_gem_object_unreference_unlocked(obj); + return 0; }
/* ---------------------------------------------------------------------- */
On Thu, Jul 09, 2015 at 11:32:37PM +0200, Daniel Vetter wrote:
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/bochs/bochs_mm.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/mgag200/mgag200_main.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index f6b283b8375e..c99c2cb28939 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -345,23 +345,15 @@ mgag200_dumb_mmap_offset(struct drm_file *file, uint64_t *offset) { struct drm_gem_object *obj; - int ret; struct mgag200_bo *bo;
- mutex_lock(&dev->struct_mutex); obj = drm_gem_object_lookup(dev, file, handle); - if (obj == NULL) { - ret = -ENOENT; - goto out_unlock; - } + if (obj == NULL) + return -ENOENT;
bo = gem_to_mga_bo(obj); *offset = mgag200_bo_mmap_offset(bo);
- drm_gem_object_unreference(obj); - ret = 0; -out_unlock: - mutex_unlock(&dev->struct_mutex); - return ret; - + drm_gem_object_unreference_unlocked(obj); + return 0; }
On Thu, Jul 09, 2015 at 11:32:38PM +0200, Daniel Vetter wrote:
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/mgag200/mgag200_main.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
Looking up an obj, immediate dropping the acquired reference and then continuing to use it isn't how this is supposed to work. Fix this by holding a reference for the entire function.
While at it stop grabbing dev->struct_mutex, it doesn't protect anything here.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/mgag200/mgag200_cursor.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c index 9f9780b7ddf0..4f2068fe5d88 100644 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c @@ -70,18 +70,22 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc, BUG_ON(pixels_2 != pixels_current && pixels_2 != pixels_prev); BUG_ON(pixels_current == pixels_prev);
+ obj = drm_gem_object_lookup(dev, file_priv, handle); + if (!obj) + return -ENOENT; + ret = mgag200_bo_reserve(pixels_1, true); if (ret) { WREG8(MGA_CURPOSXL, 0); WREG8(MGA_CURPOSXH, 0); - return ret; + goto out_unref; } ret = mgag200_bo_reserve(pixels_2, true); if (ret) { WREG8(MGA_CURPOSXL, 0); WREG8(MGA_CURPOSXH, 0); mgag200_bo_unreserve(pixels_1); - return ret; + goto out_unreserve1; }
if (!handle) { @@ -106,16 +110,6 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc, } }
- mutex_lock(&dev->struct_mutex); - obj = drm_gem_object_lookup(dev, file_priv, handle); - if (!obj) { - mutex_unlock(&dev->struct_mutex); - ret = -ENOENT; - goto out1; - } - drm_gem_object_unreference(obj); - mutex_unlock(&dev->struct_mutex); - bo = gem_to_mga_bo(obj); ret = mgag200_bo_reserve(bo, true); if (ret) { @@ -252,7 +246,11 @@ int mga_crtc_cursor_set(struct drm_crtc *crtc, if (ret) mga_hide_cursor(mdev); mgag200_bo_unreserve(pixels_1); +out_unreserve1: mgag200_bo_unreserve(pixels_2); +out_unref: + drm_gem_object_unreference_unlocked(obj); + return ret; }
On Thu, Jul 09, 2015 at 11:32:39PM +0200, Daniel Vetter wrote:
Looking up an obj, immediate dropping the acquired reference and then continuing to use it isn't how this is supposed to work. Fix this by holding a reference for the entire function.
While at it stop grabbing dev->struct_mutex, it doesn't protect anything here.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/mgag200/mgag200_cursor.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/cirrus/cirrus_main.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c index e4b976658087..055fd86ba717 100644 --- a/drivers/gpu/drm/cirrus/cirrus_main.c +++ b/drivers/gpu/drm/cirrus/cirrus_main.c @@ -293,25 +293,18 @@ cirrus_dumb_mmap_offset(struct drm_file *file, uint64_t *offset) { struct drm_gem_object *obj; - int ret; struct cirrus_bo *bo;
- mutex_lock(&dev->struct_mutex); obj = drm_gem_object_lookup(dev, file, handle); - if (obj == NULL) { - ret = -ENOENT; - goto out_unlock; - } + if (obj == NULL) + return -ENOENT;
bo = gem_to_cirrus_bo(obj); *offset = cirrus_bo_mmap_offset(bo);
- drm_gem_object_unreference(obj); - ret = 0; -out_unlock: - mutex_unlock(&dev->struct_mutex); - return ret; + drm_gem_object_unreference_unlocked(obj);
+ return 0; }
bool cirrus_check_framebuffer(struct cirrus_device *cdev, int width, int height,
On Thu, Jul 09, 2015 at 11:32:40PM +0200, Daniel Vetter wrote:
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/cirrus/cirrus_main.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Rob Clark robdclark@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/drm_gem_cma_helper.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index ca2ed7e7b595..cd55e4e9a891 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -289,20 +289,15 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv, { struct drm_gem_object *gem_obj;
- mutex_lock(&drm->struct_mutex); - gem_obj = drm_gem_object_lookup(drm, file_priv, handle); if (!gem_obj) { dev_err(drm->dev, "failed to lookup GEM object\n"); - mutex_unlock(&drm->struct_mutex); return -EINVAL; }
*offset = drm_vma_node_offset_addr(&gem_obj->vma_node);
- drm_gem_object_unreference(gem_obj); - - mutex_unlock(&drm->struct_mutex); + drm_gem_object_unreference_unlocked(gem_obj);
return 0; }
On Thu, Jul 09, 2015 at 11:32:41PM +0200, Daniel Vetter wrote:
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Rob Clark robdclark@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_gem_cma_helper.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
Aside: I stumbled over the mmap handler which directly does a dma_mmap_attrs. But totally fails to grab a reference on the underlying object and hence looks like it happily just leaks the ptes since there's no guarantee the mmap isn't still around when gem_free_object is called. Which the kerneldoc of dma_mmap_attrs explicitly forbids.
Cc: Mark Yao mark.yao@rock-chips.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c index eba5f8a52fbd..ca7b6ebe1145 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c @@ -198,15 +198,11 @@ int rockchip_gem_dumb_map_offset(struct drm_file *file_priv, uint64_t *offset) { struct drm_gem_object *obj; - int ret; - - mutex_lock(&dev->struct_mutex);
obj = drm_gem_object_lookup(dev, file_priv, handle); if (!obj) { DRM_ERROR("failed to lookup gem object.\n"); - ret = -EINVAL; - goto unlock; + return -EINVAL; }
ret = drm_gem_create_mmap_offset(obj); @@ -217,10 +213,9 @@ int rockchip_gem_dumb_map_offset(struct drm_file *file_priv, DRM_DEBUG_KMS("offset = 0x%llx\n", *offset);
out: - drm_gem_object_unreference(obj); -unlock: - mutex_unlock(&dev->struct_mutex); - return ret; + drm_gem_object_unreference_unlocked(obj); + + return 0; }
/*
On Thu, Jul 09, 2015 at 11:32:42PM +0200, Daniel Vetter wrote:
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
Aside: I stumbled over the mmap handler which directly does a dma_mmap_attrs. But totally fails to grab a reference on the underlying object and hence looks like it happily just leaks the ptes since there's no guarantee the mmap isn't still around when gem_free_object is called. Which the kerneldoc of dma_mmap_attrs explicitly forbids.
Same is true for Exynos, which seems to be the source for copy/paste here.
Anyway, for this change:
Reviewed-by: Thierry Reding treding@nvidia.com
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
While at it also fix a leak when this ioctl is called on an imported buffer.
Cc: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/armada/armada_gem.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 580e10acaa3a..95df74227ec0 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -273,18 +273,16 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, struct armada_gem_object *obj; int ret = 0;
- mutex_lock(&dev->struct_mutex); obj = armada_gem_object_lookup(dev, file, handle); if (!obj) { DRM_ERROR("failed to lookup gem object\n"); - ret = -EINVAL; - goto err_unlock; + return -EINVAL; }
/* Don't allow imported objects to be mapped */ if (obj->obj.import_attach) { ret = -EINVAL; - goto err_unlock; + goto err_unref; }
ret = drm_gem_create_mmap_offset(&obj->obj); @@ -293,9 +291,8 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, DRM_DEBUG_DRIVER("handle %#x offset %llx\n", handle, *offset); }
- drm_gem_object_unreference(&obj->obj); - err_unlock: - mutex_unlock(&dev->struct_mutex); +err_unref: + drm_gem_object_unreference_unlocked(&obj->obj);
return ret; }
On Thu, Jul 09, 2015 at 11:32:43PM +0200, Daniel Vetter wrote:
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
While at it also fix a leak when this ioctl is called on an imported buffer.
Cc: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/armada/armada_gem.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
On Thu, Jul 09, 2015 at 11:32:43PM +0200, Daniel Vetter wrote:
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
While at it also fix a leak when this ioctl is called on an imported buffer.
Good catch, thanks Daniel. I'd prefer these to be two different commits though, so that the leak can be easily backported to stable kernels if needed.
I suspect this should go through the same tree that David's work is, otherwise we end up with random dependencies between trees. With the bug fix separated out:
Acked-by: Russell King rmk+kernel@arm.linux.org.uk
It doesn't protect anything at all. fbdev helper state is all protected by modeset locks, and nouveau bo state is taken care of by ttm. There's also nothing else grabbing struct_mutex that might need to coordinate with this code. Also this is driver load code, no one can get at the device yet anyway so locking is fairly futile. There's also no drm_gem_object_unreference that would now suddenly need the _unlocked variant.
Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c index 6751553abe4a..89691ee48277 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c @@ -363,12 +363,10 @@ nouveau_fbcon_create(struct drm_fb_helper *helper, } }
- mutex_lock(&dev->struct_mutex); - info = framebuffer_alloc(0, &pdev->dev); if (!info) { ret = -ENOMEM; - goto out_unlock; + goto out_unmap; } info->skip_vt_switch = 1;
@@ -376,7 +374,7 @@ nouveau_fbcon_create(struct drm_fb_helper *helper, if (ret) { ret = -ENOMEM; framebuffer_release(info); - goto out_unlock; + goto out_unmap; }
info->par = fbcon; @@ -411,8 +409,6 @@ nouveau_fbcon_create(struct drm_fb_helper *helper,
/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
- mutex_unlock(&dev->struct_mutex); - if (chan) nouveau_fbcon_accel_init(dev); nouveau_fbcon_zfill(dev, fbcon); @@ -425,8 +421,7 @@ nouveau_fbcon_create(struct drm_fb_helper *helper, vga_switcheroo_client_fb_set(dev->pdev, info); return 0;
-out_unlock: - mutex_unlock(&dev->struct_mutex); +out_unmap: if (chan) nouveau_bo_vma_del(nvbo, &fbcon->nouveau_fb.vma); nouveau_bo_unmap(nvbo);
On Thu, Jul 09, 2015 at 11:32:44PM +0200, Daniel Vetter wrote:
It doesn't protect anything at all. fbdev helper state is all protected by modeset locks, and nouveau bo state is taken care of by ttm. There's also nothing else grabbing struct_mutex that might need to coordinate with this code. Also this is driver load code, no one can get at the device yet anyway so locking is fairly futile. There's also no drm_gem_object_unreference that would now suddenly need the _unlocked variant.
Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/nouveau/nouveau_fbcon.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
This is only called in driver load/unload paths, no need to grab any locks at all. Also, ttm takes care of itself anyway.
Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c index 18f449715788..1f8ec0e2156c 100644 --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c @@ -424,10 +424,8 @@ nouveau_ttm_init(struct nouveau_drm *drm) void nouveau_ttm_fini(struct nouveau_drm *drm) { - mutex_lock(&drm->dev->struct_mutex); ttm_bo_clean_mm(&drm->ttm.bdev, TTM_PL_VRAM); ttm_bo_clean_mm(&drm->ttm.bdev, TTM_PL_TT); - mutex_unlock(&drm->dev->struct_mutex);
ttm_bo_device_release(&drm->ttm.bdev);
On Thu, Jul 09, 2015 at 11:32:45PM +0200, Daniel Vetter wrote:
This is only called in driver load/unload paths, no need to grab any locks at all. Also, ttm takes care of itself anyway.
Cc: Ben Skeggs bskeggs@redhat.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/nouveau/nouveau_ttm.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
It really doesn't protect anything which doesn't have other locks already. It also doesn't seem to be wired up into the driver unload code fwiw, but that's a different issue.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/qxl/qxl_object.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c index 6d6f33de48f4..b28370e014c6 100644 --- a/drivers/gpu/drm/qxl/qxl_object.c +++ b/drivers/gpu/drm/qxl/qxl_object.c @@ -272,7 +272,6 @@ void qxl_bo_force_delete(struct qxl_device *qdev) return; dev_err(qdev->dev, "Userspace still has active objects !\n"); list_for_each_entry_safe(bo, n, &qdev->gem.objects, list) { - mutex_lock(&qdev->ddev->struct_mutex); dev_err(qdev->dev, "%p %p %lu %lu force free\n", &bo->gem_base, bo, (unsigned long)bo->gem_base.size, *((unsigned long *)&bo->gem_base.refcount)); @@ -280,8 +279,7 @@ void qxl_bo_force_delete(struct qxl_device *qdev) list_del_init(&bo->list); mutex_unlock(&qdev->gem.mutex); /* this should unref the ttm bo */ - drm_gem_object_unreference(&bo->gem_base); - mutex_unlock(&qdev->ddev->struct_mutex); + drm_gem_object_unreference_unlocked(&bo->gem_base); } }
On Thu, Jul 09, 2015 at 11:32:46PM +0200, Daniel Vetter wrote:
It really doesn't protect anything which doesn't have other locks already. It also doesn't seem to be wired up into the driver unload code fwiw, but that's a different issue.
Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/qxl/qxl_object.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
It really doesn't protect anything which doesn't have other locks already. Also this is run from driver unload code so not much need for locks anyway.
Cc: Alex Deucher alexander.deucher@amd.com Cc: "Christian König" christian.koenig@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/radeon/radeon_object.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 318165d4855c..3aefcb25d953 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -420,7 +420,6 @@ void radeon_bo_force_delete(struct radeon_device *rdev) } dev_err(rdev->dev, "Userspace still has active objects !\n"); list_for_each_entry_safe(bo, n, &rdev->gem.objects, list) { - mutex_lock(&rdev->ddev->struct_mutex); dev_err(rdev->dev, "%p %p %lu %lu force free\n", &bo->gem_base, bo, (unsigned long)bo->gem_base.size, *((unsigned long *)&bo->gem_base.refcount)); @@ -428,8 +427,7 @@ void radeon_bo_force_delete(struct radeon_device *rdev) list_del_init(&bo->list); mutex_unlock(&bo->rdev->gem.mutex); /* this should unref the ttm bo */ - drm_gem_object_unreference(&bo->gem_base); - mutex_unlock(&rdev->ddev->struct_mutex); + drm_gem_object_unreference_unlocked(&bo->gem_base); } }
On Thu, Jul 09, 2015 at 11:32:47PM +0200, Daniel Vetter wrote:
It really doesn't protect anything which doesn't have other locks already. Also this is run from driver unload code so not much need for locks anyway.
Cc: Alex Deucher alexander.deucher@amd.com Cc: "Christian König" christian.koenig@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/radeon/radeon_object.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
We already grab 2 device-global locks (write-sema rdev->pm.mclk_lock and rdev->ring_lock), adding another global mutex won't serialize this code more. And since there's really nothing interesting that gets protected in radeon by dev->struct mutex (we only have the global z buffer owners and it's still serializing gem bo destruction in the drm core - which is irrelevant since radeon uses ttm anyway internally) this doesn't add protection. Remove it.
Cc: Alex Deucher alexander.deucher@amd.com Cc: "Christian König" christian.koenig@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/radeon/radeon_pm.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c index c1ba83a8dd8c..05751f3f8444 100644 --- a/drivers/gpu/drm/radeon/radeon_pm.c +++ b/drivers/gpu/drm/radeon/radeon_pm.c @@ -253,7 +253,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev) (rdev->pm.requested_power_state_index == rdev->pm.current_power_state_index)) return;
- mutex_lock(&rdev->ddev->struct_mutex); down_write(&rdev->pm.mclk_lock); mutex_lock(&rdev->ring_lock);
@@ -268,7 +267,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev) /* needs a GPU reset dont reset here */ mutex_unlock(&rdev->ring_lock); up_write(&rdev->pm.mclk_lock); - mutex_unlock(&rdev->ddev->struct_mutex); return; } } @@ -304,7 +302,6 @@ static void radeon_pm_set_clocks(struct radeon_device *rdev)
mutex_unlock(&rdev->ring_lock); up_write(&rdev->pm.mclk_lock); - mutex_unlock(&rdev->ddev->struct_mutex); }
static void radeon_pm_print_states(struct radeon_device *rdev) @@ -1062,7 +1059,6 @@ force: radeon_dpm_print_power_state(rdev, rdev->pm.dpm.requested_ps); }
- mutex_lock(&rdev->ddev->struct_mutex); down_write(&rdev->pm.mclk_lock); mutex_lock(&rdev->ring_lock);
@@ -1113,7 +1109,6 @@ force: done: mutex_unlock(&rdev->ring_lock); up_write(&rdev->pm.mclk_lock); - mutex_unlock(&rdev->ddev->struct_mutex); }
void radeon_dpm_enable_uvd(struct radeon_device *rdev, bool enable)
On Thu, Jul 09, 2015 at 11:32:48PM +0200, Daniel Vetter wrote:
We already grab 2 device-global locks (write-sema rdev->pm.mclk_lock and rdev->ring_lock), adding another global mutex won't serialize this code more. And since there's really nothing interesting that gets protected in radeon by dev->struct mutex (we only have the global z buffer owners and it's still serializing gem bo destruction in the drm core - which is irrelevant since radeon uses ttm anyway internally) this doesn't add protection. Remove it.
Cc: Alex Deucher alexander.deucher@amd.com Cc: "Christian König" christian.koenig@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/radeon/radeon_pm.c | 5 ----- 1 file changed, 5 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
It really doesn't protect anything which doesn't have other locks already. Also this is run from driver unload code so not much need for locks anyway.
Same changes as for readone really.
Cc: Alex Deucher alexander.deucher@amd.com Cc: "Christian König" christian.koenig@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index 8da64245b31b..97422bf3b7d5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -478,7 +478,6 @@ void amdgpu_bo_force_delete(struct amdgpu_device *adev) } dev_err(adev->dev, "Userspace still has active objects !\n"); list_for_each_entry_safe(bo, n, &adev->gem.objects, list) { - mutex_lock(&adev->ddev->struct_mutex); dev_err(adev->dev, "%p %p %lu %lu force free\n", &bo->gem_base, bo, (unsigned long)bo->gem_base.size, *((unsigned long *)&bo->gem_base.refcount)); @@ -486,8 +485,7 @@ void amdgpu_bo_force_delete(struct amdgpu_device *adev) list_del_init(&bo->list); mutex_unlock(&bo->adev->gem.mutex); /* this should unref the ttm bo */ - drm_gem_object_unreference(&bo->gem_base); - mutex_unlock(&adev->ddev->struct_mutex); + drm_gem_object_unreference_unlocked(&bo->gem_base); } }
On Thu, Jul 09, 2015 at 11:32:49PM +0200, Daniel Vetter wrote:
It really doesn't protect anything which doesn't have other locks already. Also this is run from driver unload code so not much need for locks anyway.
Same changes as for readone really.
s/radeone/radeon/
Otherwise:
Reviewed-by: Thierry Reding treding@nvidia.com
Similar to radeon, except that amdgpu doesn't even use struct_mutex to protect anything like the shared z buffer (sane gpu architecture, yay!). And the code already grabs the globa adev->ring_lock, so this code can't race with itself. Which makes struct_mutex completely redundnant. Remove it.
Cc: Alex Deucher alexander.deucher@amd.com Cc: "Christian König" christian.koenig@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c index ed13baa7c976..535d90254673 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c @@ -580,7 +580,6 @@ force: amdgpu_dpm_print_power_state(adev, adev->pm.dpm.requested_ps); }
- mutex_lock(&adev->ddev->struct_mutex); mutex_lock(&adev->ring_lock);
/* update whether vce is active */ @@ -628,7 +627,6 @@ force:
done: mutex_unlock(&adev->ring_lock); - mutex_unlock(&adev->ddev->struct_mutex); }
void amdgpu_dpm_enable_uvd(struct amdgpu_device *adev, bool enable)
On Thu, Jul 09, 2015 at 11:32:50PM +0200, Daniel Vetter wrote:
Similar to radeon, except that amdgpu doesn't even use struct_mutex to protect anything like the shared z buffer (sane gpu architecture, yay!). And the code already grabs the globa adev->ring_lock, so this code can't race with itself. Which makes struct_mutex completely redundnant. Remove it.
Cc: Alex Deucher alexander.deucher@amd.com Cc: "Christian König" christian.koenig@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 2 -- 1 file changed, 2 deletions(-)
Reviewed-by: Thierry Reding treding@nvidia.com
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
While at it also fix a leak when this ioctl is called on an imported buffer.
Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/tegra/gem.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 01e16e146bfe..827838e64d6e 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -408,12 +408,9 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm, struct drm_gem_object *gem; struct tegra_bo *bo;
- mutex_lock(&drm->struct_mutex); - gem = drm_gem_object_lookup(drm, file, handle); if (!gem) { dev_err(drm->dev, "failed to lookup GEM object\n"); - mutex_unlock(&drm->struct_mutex); return -EINVAL; }
@@ -423,8 +420,6 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
drm_gem_object_unreference(gem);
- mutex_unlock(&drm->struct_mutex); - return 0; }
This only grabs the mutex when really needed, but still has a might-acquire lockdep check to make sure that's always possible. With this patch tegra is officially struct_mutex free, yay!
Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/tegra/drm.c | 4 +--- drivers/gpu/drm/tegra/gem.c | 8 ++------ 2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index 56b606178204..c6276aebfec3 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -276,9 +276,7 @@ host1x_bo_lookup(struct drm_device *drm, struct drm_file *file, u32 handle) if (!gem) return NULL;
- mutex_lock(&drm->struct_mutex); - drm_gem_object_unreference(gem); - mutex_unlock(&drm->struct_mutex); + drm_gem_object_unreference_unlocked(gem);
bo = to_tegra_bo(gem); return &bo->base; diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 827838e64d6e..a946259495e1 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -30,9 +30,7 @@ static void tegra_bo_put(struct host1x_bo *bo) struct tegra_bo *obj = host1x_to_tegra_bo(bo); struct drm_device *drm = obj->gem.dev;
- mutex_lock(&drm->struct_mutex); - drm_gem_object_unreference(&obj->gem); - mutex_unlock(&drm->struct_mutex); + drm_gem_object_unreference_unlocked(&obj->gem); }
static dma_addr_t tegra_bo_pin(struct host1x_bo *bo, struct sg_table **sgt) @@ -74,9 +72,7 @@ static struct host1x_bo *tegra_bo_get(struct host1x_bo *bo) struct tegra_bo *obj = host1x_to_tegra_bo(bo); struct drm_device *drm = obj->gem.dev;
- mutex_lock(&drm->struct_mutex); - drm_gem_object_reference(&obj->gem); - mutex_unlock(&drm->struct_mutex); + drm_gem_object_reference_unlocked(&obj->gem);
return bo; }
On Wed, Jul 15, 2015 at 03:38:51PM +0200, Daniel Vetter wrote:
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
While at it also fix a leak when this ioctl is called on an imported buffer.
I don't see where the leak's fixed, but other than that this looks good to me. Shall I pick this up into the drm/tegra tree?
Thierry
Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/tegra/gem.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 01e16e146bfe..827838e64d6e 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -408,12 +408,9 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm, struct drm_gem_object *gem; struct tegra_bo *bo;
- mutex_lock(&drm->struct_mutex);
- gem = drm_gem_object_lookup(drm, file, handle); if (!gem) { dev_err(drm->dev, "failed to lookup GEM object\n");
mutex_unlock(&drm->struct_mutex);@@ -423,8 +420,6 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
drm_gem_object_unreference(gem);
- mutex_unlock(&drm->struct_mutex);
- return 0;
}
-- 2.1.4
On Mon, Aug 10, 2015 at 12:30:21PM +0200, Thierry Reding wrote:
On Wed, Jul 15, 2015 at 03:38:51PM +0200, Daniel Vetter wrote:
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
While at it also fix a leak when this ioctl is called on an imported buffer.
I don't see where the leak's fixed, but other than that this looks good to me. Shall I pick this up into the drm/tegra tree?
Copypaste in the commit message from armada, doesn't apply to tegra. Do you also plan to pick up "drm/tegra: Use drm_gem_object_reference_unlocked" directly?
And thanks for all the review. -Daniel
Thierry
Cc: Thierry Reding thierry.reding@gmail.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/tegra/gem.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 01e16e146bfe..827838e64d6e 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -408,12 +408,9 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm, struct drm_gem_object *gem; struct tegra_bo *bo;
- mutex_lock(&drm->struct_mutex);
- gem = drm_gem_object_lookup(drm, file, handle); if (!gem) { dev_err(drm->dev, "failed to lookup GEM object\n");
mutex_unlock(&drm->struct_mutex);@@ -423,8 +420,6 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
drm_gem_object_unreference(gem);
- mutex_unlock(&drm->struct_mutex);
- return 0;
}
-- 2.1.4
On Mon, Aug 10, 2015 at 01:31:42PM +0200, Daniel Vetter wrote:
On Mon, Aug 10, 2015 at 12:30:21PM +0200, Thierry Reding wrote:
On Wed, Jul 15, 2015 at 03:38:51PM +0200, Daniel Vetter wrote:
Since David Herrmann's mmap vma manager rework we don't need to grab dev->struct_mutex any more to prevent races when looking up the mmap offset. Drop it and instead don't forget to use the unref_unlocked variant (since the drm core still cares).
While at it also fix a leak when this ioctl is called on an imported buffer.
I don't see where the leak's fixed, but other than that this looks good to me. Shall I pick this up into the drm/tegra tree?
Copypaste in the commit message from armada, doesn't apply to tegra. Do you also plan to pick up "drm/tegra: Use drm_gem_object_reference_unlocked" directly?
I don't mind much either way. I don't think they'll conflict with anything, but since you already said that they're all independent, I don't see a reason why I shouldn't pull them into drm/tegra. If you prefer to keep them together, that's fine with me too.
Thanks, Thierry
On Thu, Jul 09, 2015 at 11:32:32PM +0200, Daniel Vetter wrote:
Hi all,
I wanted to take another look at struct_mutex usage in modern (gem) drivers and noticed that for a fair lot we're very to be completely struct_mutex free.
This pile here is the the simple part, which mostly just removes code and mutex_lock/unlock calls. All the patches here are independent and can be merged in any order whatsoever. My plan is to send out a pull request for all those not picked up by driver maintainers in 2-3 weeks or so, assuming no one complains.
Of course review & comments still very much welcome.
The more tricky 2nd part of this (and that one's not yet done) is to rework the gem mmap handler to use the same kref_get_unless_zero trick as ttm. With that there's no core requirement to hold struct_mutex over the final unref, which means we can make that one lockless. I plan to add a gem_object_free_unlocked for all the drivers which don't have any need for this lock.
Also there's a few more drivers which can be made struct_mutex free easily, I'll propably stitch together poc patches for those.
There's a concurrency bug in Tegra DRM currently because we don't lock accesses to drm_mm (I guess this demonstrates how badly we need better testing...) and it seems like this is typically protected by the very same struct_mutex that you're on a crusade to remove. If your goal is to get rid of it for good, should we simply add a separate lock just for the drm_mm? We don't have another one that would fit.
Thierry
On Mon, Aug 10, 2015 at 01:35:58PM +0200, Thierry Reding wrote:
On Thu, Jul 09, 2015 at 11:32:32PM +0200, Daniel Vetter wrote:
Hi all,
I wanted to take another look at struct_mutex usage in modern (gem) drivers and noticed that for a fair lot we're very to be completely struct_mutex free.
This pile here is the the simple part, which mostly just removes code and mutex_lock/unlock calls. All the patches here are independent and can be merged in any order whatsoever. My plan is to send out a pull request for all those not picked up by driver maintainers in 2-3 weeks or so, assuming no one complains.
Of course review & comments still very much welcome.
The more tricky 2nd part of this (and that one's not yet done) is to rework the gem mmap handler to use the same kref_get_unless_zero trick as ttm. With that there's no core requirement to hold struct_mutex over the final unref, which means we can make that one lockless. I plan to add a gem_object_free_unlocked for all the drivers which don't have any need for this lock.
Also there's a few more drivers which can be made struct_mutex free easily, I'll propably stitch together poc patches for those.
There's a concurrency bug in Tegra DRM currently because we don't lock accesses to drm_mm (I guess this demonstrates how badly we need better testing...) and it seems like this is typically protected by the very same struct_mutex that you're on a crusade to remove. If your goal is to get rid of it for good, should we simply add a separate lock just for the drm_mm? We don't have another one that would fit.
Actually that is one of the first targets for more fine-grained locking. I would not add a new lock to drm_mm as at least for i915, we want to use a similar per-vm lock (of which the drm_mm is just one part). -Chris
On Mon, Aug 10, 2015 at 12:53:23PM +0100, Chris Wilson wrote:
On Mon, Aug 10, 2015 at 01:35:58PM +0200, Thierry Reding wrote:
On Thu, Jul 09, 2015 at 11:32:32PM +0200, Daniel Vetter wrote:
Hi all,
I wanted to take another look at struct_mutex usage in modern (gem) drivers and noticed that for a fair lot we're very to be completely struct_mutex free.
This pile here is the the simple part, which mostly just removes code and mutex_lock/unlock calls. All the patches here are independent and can be merged in any order whatsoever. My plan is to send out a pull request for all those not picked up by driver maintainers in 2-3 weeks or so, assuming no one complains.
Of course review & comments still very much welcome.
The more tricky 2nd part of this (and that one's not yet done) is to rework the gem mmap handler to use the same kref_get_unless_zero trick as ttm. With that there's no core requirement to hold struct_mutex over the final unref, which means we can make that one lockless. I plan to add a gem_object_free_unlocked for all the drivers which don't have any need for this lock.
Also there's a few more drivers which can be made struct_mutex free easily, I'll propably stitch together poc patches for those.
There's a concurrency bug in Tegra DRM currently because we don't lock accesses to drm_mm (I guess this demonstrates how badly we need better testing...) and it seems like this is typically protected by the very same struct_mutex that you're on a crusade to remove. If your goal is to get rid of it for good, should we simply add a separate lock just for the drm_mm? We don't have another one that would fit.
Actually that is one of the first targets for more fine-grained locking. I would not add a new lock to drm_mm as at least for i915, we want to use a similar per-vm lock (of which the drm_mm is just one part).
Sorry if I was being unclear. I wasn't suggesting adding the lock to struct drm_mm, but rather add a driver-specific one specifically to serialize accesses to the drm_mm. I agree that it's better to do this in a driver-specific way because other structures may need to be protected by the same lock.
Thierry
On Mon, Aug 10, 2015 at 01:35:58PM +0200, Thierry Reding wrote:
On Thu, Jul 09, 2015 at 11:32:32PM +0200, Daniel Vetter wrote:
Hi all,
I wanted to take another look at struct_mutex usage in modern (gem) drivers and noticed that for a fair lot we're very to be completely struct_mutex free.
This pile here is the the simple part, which mostly just removes code and mutex_lock/unlock calls. All the patches here are independent and can be merged in any order whatsoever. My plan is to send out a pull request for all those not picked up by driver maintainers in 2-3 weeks or so, assuming no one complains.
Of course review & comments still very much welcome.
The more tricky 2nd part of this (and that one's not yet done) is to rework the gem mmap handler to use the same kref_get_unless_zero trick as ttm. With that there's no core requirement to hold struct_mutex over the final unref, which means we can make that one lockless. I plan to add a gem_object_free_unlocked for all the drivers which don't have any need for this lock.
Also there's a few more drivers which can be made struct_mutex free easily, I'll propably stitch together poc patches for those.
There's a concurrency bug in Tegra DRM currently because we don't lock accesses to drm_mm (I guess this demonstrates how badly we need better testing...) and it seems like this is typically protected by the very same struct_mutex that you're on a crusade to remove. If your goal is to get rid of it for good, should we simply add a separate lock just for the drm_mm? We don't have another one that would fit.
Yes, please just add your own driver-private lock. The next struct_mutex crusade series will actually do exactly that, at least for simple cases like armada. With that changes most drivers don't care about struct_mutex any more in their ->gem_free_object hook and I plan to add a new ->gem_free_object_unlocked variant for all the drivers which don't have any residual usage of struct_mutex. -Daniel
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Daniel Vetter -
Russell King - ARM Linux -
Thierry Reding