[PATCH 00/18] dev->struct_mutex crusade

[PATCH 00/18] dev->struct_mutex crusade

[Daniel Vetter]

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(-)

-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/18] drm/gem: rip out drm vma accounting for gem mmaps

[Daniel Vetter]

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);
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/18] drm/gem: rip out drm vma accounting for gem mmaps

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/18] drm/gem: rip out drm vma accounting for gem mmaps

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/18] drm/gem: rip out drm vma accounting for gem mmaps

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 588 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/18] drm/cma-helper: Fix locking in drm_fb_cma_debugfs_show

[Daniel Vetter]

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",
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/18] drm/cma-helper: Fix locking in drm_fb_cma_debugfs_show

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 739 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/18] drm/gem: Be more friendly with locking checks

[Daniel Vetter]

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);
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/18] drm/gem: Be more friendly with locking checks

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 304 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/18] drm/gem: Be more friendly with locking checks

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/18] drm/ast: Don't grab dev->struct_mutex for in mmap offset ioctl

[Daniel Vetter]

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;
 
 }
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/18] drm/ast: Don't grab dev->struct_mutex for in mmap offset ioctl

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 554 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/18] drm/bochs: Don't grab dev->struct_mutex for in mmap offset ioctl

[Daniel Vetter]

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;
 }
 
 /* ---------------------------------------------------------------------- */
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/18] drm/bochs: Don't grab dev->struct_mutex for in mmap offset ioctl

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 556 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/18] drm/mga200g: Don't grab dev->struct_mutex for in mmap offset ioctl

[Daniel Vetter]

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;
 }
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/18] drm/mga200g: Don't grab dev->struct_mutex for in mmap offset ioctl

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 562 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/18] drm/mga200g: Hold a proper reference for cursor_set

[Daniel Vetter]

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;
 }
 
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/18] drm/mga200g: Hold a proper reference for cursor_set

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 594 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/18] drm/cirrus: Don't grab dev->struct_mutex for in mmap offset ioctl

[Daniel Vetter]

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,
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/18] drm/cirrus: Don't grab dev->struct_mutex for in mmap offset ioctl

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 559 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/18] drm/cma-helper: Don't grab dev->struct_mutex for in mmap offset ioctl

[Daniel Vetter]

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;
 }
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/18] drm/cma-helper: Don't grab dev->struct_mutex for in mmap offset ioctl

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 647 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/18] drm/rockchip: Don't grab dev->struct_mutex for in mmap offset ioctl

[Daniel Vetter]

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;
 }
 
 /*
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/18] drm/rockchip: Don't grab dev->struct_mutex for in mmap offset ioctl

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 842 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/18] drm/armada: Don't grab dev->struct_mutex for in mmap offset ioctl

[Daniel Vetter]

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;
 }
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/18] drm/armada: Don't grab dev->struct_mutex for in mmap offset ioctl

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 690 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/18] drm/armada: Don't grab dev->struct_mutex for in mmap offset ioctl

[Russell King - ARM Linux]

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>

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 12/18] drm/nouveau: Don't take dev->struct_mutex in fbcon init

[Daniel Vetter]

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);
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 12/18] drm/nouveau: Don't take dev->struct_mutex in fbcon init

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 785 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 13/18] drm/nouveau: Don't take dev->struct_mutex in ttm_fini

[Daniel Vetter]

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);
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/18] drm/nouveau: Don't take dev->struct_mutex in ttm_fini

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 436 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 14/18] drm/qxl: Don't take dev->struct_mutex in bo_force_delete

[Daniel Vetter]

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);
 	}
 }
 
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/18] drm/qxl: Don't take dev->struct_mutex in bo_force_delete

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 466 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 15/18] drm/radeon: Don't take dev->struct_mutex in bo_force_delete

[Daniel Vetter]

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);
 	}
 }
 
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 15/18] drm/radeon: Don't take dev->struct_mutex in bo_force_delete

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 549 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 16/18] drm/radeon: Don't take dev->struct_mutex in pm functions

[Daniel Vetter]

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)
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 16/18] drm/radeon: Don't take dev->struct_mutex in pm functions

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 844 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 17/18] drm/amdgpu: Don't take dev->struct_mutex in bo_force_delete

[Daniel Vetter]

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);
 	}
 }
 
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 17/18] drm/amdgpu: Don't take dev->struct_mutex in bo_force_delete

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 354 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 18/18] drm/amdgpu: don't grab dev->struct_mutex in pm functions

[Daniel Vetter]

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)
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 18/18] drm/amdgpu: don't grab dev->struct_mutex in pm functions

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6765
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                 -1              287/287              286/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 18/18] drm/amdgpu: don't grab dev->struct_mutex in pm functions

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 680 bytes --]

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>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/tegra: don't take dev->struct_mutex in mmap offset ioctl

[Daniel Vetter]

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;
 }
 
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/tegra: Use drm_gem_object_reference_unlocked

[Daniel Vetter]

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;
 }
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/tegra: Use drm_gem_object_reference_unlocked

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6807
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  315/315              315/315
IVB                                  342/342              342/342
BYT                 -2              285/285              283/285
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
*BYT  igt@gem_partial_pwrite_pread@reads-uncached      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/tegra: don't take dev->struct_mutex in mmap offset ioctl

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1583 bytes --]

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);
>  		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;
>  }
>  
> -- 
> 2.1.4
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/tegra: don't take dev->struct_mutex in mmap offset ioctl

[Daniel Vetter]

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);
> >  		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;
> >  }
> >  
> > -- 
> > 2.1.4
> > 



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/tegra: don't take dev->struct_mutex in mmap offset ioctl

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1169 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/18] dev->struct_mutex crusade

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1590 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/18] dev->struct_mutex crusade

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/18] dev->struct_mutex crusade

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2361 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/18] dev->struct_mutex crusade

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx