[PATCH 00/29] dev->struct_mutex crusade, once more

[PATCH 00/29] dev->struct_mutex crusade, once more

[Daniel Vetter]

Hi all,

Since Daniel Stone pointed out in the last round that I fumbled one locked vs.
unlocked case I audited all the drivers once more and uprooted a bunch more
offenders. They're mostly in error paths, but in case anyone wants to pick them
up I put them at the beginning of the patch series.

Again review, comments and acks highly welcome. I'd like to get this all in for
4.5 if possible, and will send out a pull request to Dave with the leftovers.

Cheers, Daniel

Daniel Vetter (29):
  drm/armada: Use unlocked gem unreferencing
  drm/nouveau: Use unlocked gem unreferencing
  drm/omapdrm: Use unlocked gem unreferencing
  drm/amdgpu: Use unlocked gem unreferencing
  drm/radeon: Use unlocked gem unreferencing
  drm/qxl: Use unlocked gem unreferencing
  drm/tegra: Use unlocked gem unreferencing
  drm/msm: Use unlocked gem unreferencing
  drm/udl: Use unlocked gem unreferencing
  drm/armada: Plug leak in dumb_map_offset
  drm/armada: Don't grab dev->struct_mutex for in mmap offset ioctl
  drm/armada: Drop struct_mutex from cursor paths
  drm/armada: Use a private mutex to protect priv->linear
  drm/tegra: don't take dev->struct_mutex in mmap offset ioctl
  drm/tegra: Use drm_gem_object_unreference_unlocked
  drm/gma500: Use correct unref in the gem bo create function
  drm/gma500: Drop dev->struct_mutex from modeset code
  drm/gma500: Drop dev->struct_mutex from fbdev init/teardown code
  drm/gma500: Drop dev->struct_mutex from mmap offset function
  drm/gma500: Add driver private mutex for the fault handler
  drm/nouveau: Drop dev->struct_mutex from fbdev init
  drm/exynos: Drop dev->struct_mutex from mmap offset function
  drm/exynos: drop struct_mutex from exynos_gem_map_sgt_with_dma
  drm/exynos: drop struct_mutex from exynos_drm_gem_get_ioctl
  drm/exynos: drop struct_mutex from fbdev setup
  drm/vgem: Simplify dum_map
  drm/vgem: Move get_pages to gem_create
  drm/vgem: Drop dev->struct_mutex
  drm/vma_manage: Drop has_offset

 drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c    |  2 +-
 drivers/gpu/drm/armada/armada_crtc.c      |  8 ++-----
 drivers/gpu/drm/armada/armada_debugfs.c   |  4 ++--
 drivers/gpu/drm/armada/armada_drm.h       |  3 ++-
 drivers/gpu/drm/armada/armada_drv.c       |  1 +
 drivers/gpu/drm/armada/armada_gem.c       | 25 ++++++++++-----------
 drivers/gpu/drm/drm_gem.c                 | 17 +++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 22 +++++++------------
 drivers/gpu/drm/exynos/exynos_drm_gem.c   | 19 +++-------------
 drivers/gpu/drm/gma500/framebuffer.c      | 12 ++---------
 drivers/gpu/drm/gma500/gem.c              | 19 ++++++----------
 drivers/gpu/drm/gma500/gma_display.c      | 13 +++--------
 drivers/gpu/drm/gma500/gtt.c              |  1 +
 drivers/gpu/drm/gma500/psb_drv.h          |  2 ++
 drivers/gpu/drm/i915/i915_gem.c           |  3 ---
 drivers/gpu/drm/msm/msm_fbdev.c           |  2 +-
 drivers/gpu/drm/nouveau/nouveau_display.c |  2 +-
 drivers/gpu/drm/nouveau/nouveau_fbcon.c   |  5 -----
 drivers/gpu/drm/omapdrm/omap_fbdev.c      |  2 +-
 drivers/gpu/drm/qxl/qxl_fb.c              |  4 ++--
 drivers/gpu/drm/radeon/radeon_fb.c        |  2 +-
 drivers/gpu/drm/tegra/drm.c               | 14 ++++++------
 drivers/gpu/drm/tegra/gem.c               | 13 ++---------
 drivers/gpu/drm/udl/udl_fb.c              |  2 +-
 drivers/gpu/drm/udl/udl_gem.c             |  2 +-
 drivers/gpu/drm/vgem/vgem_drv.c           | 36 ++++++++++---------------------
 include/drm/drm_vma_manager.h             | 15 +------------
 27 files changed, 89 insertions(+), 161 deletions(-)

-- 
2.5.1

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

[PATCH 01/29] drm/armada: Use unlocked gem unreferencing

[Daniel Vetter]

For drm_gem_object_unreference callers are required to hold
dev->struct_mutex, which these paths don't. Enforcing this requirement
has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Oct 15 09:36:25 2015 +0200

    drm/gem: Check locking in drm_gem_object_unreference

Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/armada/armada_crtc.c | 2 +-
 drivers/gpu/drm/armada/armada_gem.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
index cebcab560626..7ea35bee7cd5 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -972,7 +972,7 @@ static void armada_drm_crtc_destroy(struct drm_crtc *crtc)
 	struct armada_private *priv = crtc->dev->dev_private;
 
 	if (dcrtc->cursor_obj)
-		drm_gem_object_unreference(&dcrtc->cursor_obj->obj);
+		drm_gem_object_unreference_unlocked(&dcrtc->cursor_obj->obj);
 
 	priv->dcrtc[dcrtc->num] = NULL;
 	drm_crtc_cleanup(&dcrtc->crtc);
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index 60a688ef81c7..aaf88641bfc5 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -352,13 +352,13 @@ int armada_gem_mmap_ioctl(struct drm_device *dev, void *data,
 		return -ENOENT;
 
 	if (!dobj->obj.filp) {
-		drm_gem_object_unreference(&dobj->obj);
+		drm_gem_object_unreference_unlocked(&dobj->obj);
 		return -EINVAL;
 	}
 
 	addr = vm_mmap(dobj->obj.filp, 0, args->size, PROT_READ | PROT_WRITE,
 		       MAP_SHARED, args->offset);
-	drm_gem_object_unreference(&dobj->obj);
+	drm_gem_object_unreference_unlocked(&dobj->obj);
 	if (IS_ERR_VALUE(addr))
 		return addr;
 
-- 
2.5.1

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

[PATCH 02/29] drm/nouveau: Use unlocked gem unreferencing

[Daniel Vetter]

For drm_gem_object_unreference callers are required to hold
dev->struct_mutex, which these paths don't. Enforcing this requirement
has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Oct 15 09:36:25 2015 +0200

    drm/gem: Check locking in drm_gem_object_unreference

Cc: Ben Skeggs <bskeggs@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/nouveau/nouveau_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 025504911b39..62633b110d70 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -295,7 +295,7 @@ nouveau_user_framebuffer_create(struct drm_device *dev,
 err:
 	kfree(nouveau_fb);
 err_unref:
-	drm_gem_object_unreference(gem);
+	drm_gem_object_unreference_unlocked(gem);
 	return ERR_PTR(ret);
 }
 
-- 
2.5.1

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

[PATCH 03/29] drm/omapdrm: Use unlocked gem unreferencing

[Daniel Vetter]

For drm_gem_object_unreference callers are required to hold
dev->struct_mutex, which these paths don't. Enforcing this requirement
has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Oct 15 09:36:25 2015 +0200

    drm/gem: Check locking in drm_gem_object_unreference

Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index b8e4cdec28c3..c59090e8d23f 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -156,7 +156,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 		/* note: if fb creation failed, we can't rely on fb destroy
 		 * to unref the bo:
 		 */
-		drm_gem_object_unreference(fbdev->bo);
+		drm_gem_object_unreference_unlocked(fbdev->bo);
 		ret = PTR_ERR(fb);
 		goto fail;
 	}
-- 
2.5.1

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

[PATCH 04/29] drm/amdgpu: Use unlocked gem unreferencing

[Daniel Vetter]

For drm_gem_object_unreference callers are required to hold
dev->struct_mutex, which these paths don't. Enforcing this requirement
has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Oct 15 09:36:25 2015 +0200

    drm/gem: Check locking in drm_gem_object_unreference

Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
index 6fcbbcc2e99e..cfb6caad2a73 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
@@ -263,7 +263,7 @@ out_unref:
 
 	}
 	if (fb && ret) {
-		drm_gem_object_unreference(gobj);
+		drm_gem_object_unreference_unlocked(gobj);
 		drm_framebuffer_unregister_private(fb);
 		drm_framebuffer_cleanup(fb);
 		kfree(fb);
-- 
2.5.1

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

[PATCH 05/29] drm/radeon: Use unlocked gem unreferencing

[Daniel Vetter]

For drm_gem_object_unreference callers are required to hold
dev->struct_mutex, which these paths don't. Enforcing this requirement
has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Oct 15 09:36:25 2015 +0200

    drm/gem: Check locking in drm_gem_object_unreference

Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/radeon/radeon_fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c
index adc44bbc81a9..d2e628eea53d 100644
--- a/drivers/gpu/drm/radeon/radeon_fb.c
+++ b/drivers/gpu/drm/radeon/radeon_fb.c
@@ -282,7 +282,7 @@ out_unref:
 
 	}
 	if (fb && ret) {
-		drm_gem_object_unreference(gobj);
+		drm_gem_object_unreference_unlocked(gobj);
 		drm_framebuffer_unregister_private(fb);
 		drm_framebuffer_cleanup(fb);
 		kfree(fb);
-- 
2.5.1

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

[PATCH 06/29] drm/qxl: Use unlocked gem unreferencing

[Daniel Vetter]

For drm_gem_object_unreference callers are required to hold
dev->struct_mutex, which these paths don't. Enforcing this requirement
has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Oct 15 09:36:25 2015 +0200

    drm/gem: Check locking in drm_gem_object_unreference

Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/qxl/qxl_fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
index 7136e521e6db..bb7ce07b788b 100644
--- a/drivers/gpu/drm/qxl/qxl_fb.c
+++ b/drivers/gpu/drm/qxl/qxl_fb.c
@@ -443,11 +443,11 @@ out_unref:
 		}
 	}
 	if (fb && ret) {
-		drm_gem_object_unreference(gobj);
+		drm_gem_object_unreference_unlocked(gobj);
 		drm_framebuffer_cleanup(fb);
 		kfree(fb);
 	}
-	drm_gem_object_unreference(gobj);
+	drm_gem_object_unreference_unlocked(gobj);
 	return ret;
 }
 
-- 
2.5.1

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

[PATCH 07/29] drm/tegra: Use unlocked gem unreferencing

[Daniel Vetter]

For drm_gem_object_unreference callers are required to hold
dev->struct_mutex, which these paths don't. Enforcing this requirement
has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Oct 15 09:36:25 2015 +0200

    drm/gem: Check locking in drm_gem_object_unreference

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/tegra/drm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index e0f827790a5e..3479c57151af 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -473,7 +473,7 @@ static int tegra_gem_mmap(struct drm_device *drm, void *data,
 
 	args->offset = drm_vma_node_offset_addr(&bo->gem.vma_node);
 
-	drm_gem_object_unreference(gem);
+	drm_gem_object_unreference_unlocked(gem);
 
 	return 0;
 }
@@ -683,7 +683,7 @@ static int tegra_gem_set_tiling(struct drm_device *drm, void *data,
 	bo->tiling.mode = mode;
 	bo->tiling.value = value;
 
-	drm_gem_object_unreference(gem);
+	drm_gem_object_unreference_unlocked(gem);
 
 	return 0;
 }
@@ -723,7 +723,7 @@ static int tegra_gem_get_tiling(struct drm_device *drm, void *data,
 		break;
 	}
 
-	drm_gem_object_unreference(gem);
+	drm_gem_object_unreference_unlocked(gem);
 
 	return err;
 }
@@ -748,7 +748,7 @@ static int tegra_gem_set_flags(struct drm_device *drm, void *data,
 	if (args->flags & DRM_TEGRA_GEM_BOTTOM_UP)
 		bo->flags |= TEGRA_BO_BOTTOM_UP;
 
-	drm_gem_object_unreference(gem);
+	drm_gem_object_unreference_unlocked(gem);
 
 	return 0;
 }
@@ -770,7 +770,7 @@ static int tegra_gem_get_flags(struct drm_device *drm, void *data,
 	if (bo->flags & TEGRA_BO_BOTTOM_UP)
 		args->flags |= DRM_TEGRA_GEM_BOTTOM_UP;
 
-	drm_gem_object_unreference(gem);
+	drm_gem_object_unreference_unlocked(gem);
 
 	return 0;
 }
-- 
2.5.1

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

[PATCH 08/29] drm/msm: Use unlocked gem unreferencing

[Daniel Vetter]

For drm_gem_object_unreference callers are required to hold
dev->struct_mutex, which these paths don't. Enforcing this requirement
has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Oct 15 09:36:25 2015 +0200

    drm/gem: Check locking in drm_gem_object_unreference

Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/msm/msm_fbdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index 3f6ec077b51d..d95af6eba602 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -121,7 +121,7 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
 		/* note: if fb creation failed, we can't rely on fb destroy
 		 * to unref the bo:
 		 */
-		drm_gem_object_unreference(fbdev->bo);
+		drm_gem_object_unreference_unlocked(fbdev->bo);
 		ret = PTR_ERR(fb);
 		goto fail;
 	}
-- 
2.5.1

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

[PATCH 09/29] drm/udl: Use unlocked gem unreferencing

[Daniel Vetter]

For drm_gem_object_unreference callers are required to hold
dev->struct_mutex, which these paths don't. Enforcing this requirement
has become a bit more strict with

commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Oct 15 09:36:25 2015 +0200

    drm/gem: Check locking in drm_gem_object_unreference

Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/udl/udl_fb.c  | 2 +-
 drivers/gpu/drm/udl/udl_gem.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index 200419d4d43c..18a2acbccb7d 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -538,7 +538,7 @@ static int udlfb_create(struct drm_fb_helper *helper,
 out_destroy_fbi:
 	drm_fb_helper_release_fbi(helper);
 out_gfree:
-	drm_gem_object_unreference(&ufbdev->ufb.obj->base);
+	drm_gem_object_unreference_unlocked(&ufbdev->ufb.obj->base);
 out:
 	return ret;
 }
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
index 2a0a784ab6ee..d7528e0d8442 100644
--- a/drivers/gpu/drm/udl/udl_gem.c
+++ b/drivers/gpu/drm/udl/udl_gem.c
@@ -52,7 +52,7 @@ udl_gem_create(struct drm_file *file,
 		return ret;
 	}
 
-	drm_gem_object_unreference(&obj->base);
+	drm_gem_object_unreference_unlocked(&obj->base);
 	*handle_p = handle;
 	return 0;
 }
-- 
2.5.1

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

[PATCH 10/29] drm/armada: Plug leak in dumb_map_offset

[Daniel Vetter]

We need to drop the gem bo reference if it's an imported one.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index aaf88641bfc5..2a3ef7938f30 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -285,7 +285,7 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
 	/* 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);
@@ -294,6 +294,7 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
 		DRM_DEBUG_DRIVER("handle %#x offset %llx\n", handle, *offset);
 	}
 
+ err_unref:
 	drm_gem_object_unreference(&obj->obj);
  err_unlock:
 	mutex_unlock(&dev->struct_mutex);
-- 
2.5.1

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

[PATCH 11/29] 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).

v2: Split out the leak fix in dump_map_offset into a separate patch as
requested by Russell. Also align labels the same way as before to
stick with local coding style.

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 | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index 2a3ef7938f30..e3a86b96af2a 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -274,12 +274,10 @@ 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 */
@@ -295,9 +293,7 @@ int armada_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
 	}
 
  err_unref:
-	drm_gem_object_unreference(&obj->obj);
- err_unlock:
-	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(&obj->obj);
 
 	return ret;
 }
-- 
2.5.1

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

[PATCH 12/29] drm/armada: Drop struct_mutex from cursor paths

[Daniel Vetter]

The kms state itself is already protected by the modeset locks
acquired by the drm core. The only thing left is gem bo state, and
since the cursor code expects small objects which are statically
mapped at create time and then invariant over the lifetime of the gem
bo there's nothing to protect.

See armada_gem_dumb_create -> armada_gem_linear_back which assigns
obj->addr which is the only thing used by the cursor code.

Only tricky bit is to switch to the _unlocked unreference function.

Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/armada/armada_crtc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
index 7ea35bee7cd5..4bd1ce75a9f7 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -928,11 +928,10 @@ static int armada_drm_crtc_cursor_set(struct drm_crtc *crtc,
 		}
 	}
 
-	mutex_lock(&dev->struct_mutex);
 	if (dcrtc->cursor_obj) {
 		dcrtc->cursor_obj->update = NULL;
 		dcrtc->cursor_obj->update_data = NULL;
-		drm_gem_object_unreference(&dcrtc->cursor_obj->obj);
+		drm_gem_object_unreference_unlocked(&dcrtc->cursor_obj->obj);
 	}
 	dcrtc->cursor_obj = obj;
 	dcrtc->cursor_w = w;
@@ -942,7 +941,6 @@ static int armada_drm_crtc_cursor_set(struct drm_crtc *crtc,
 		obj->update_data = dcrtc;
 		obj->update = cursor_update;
 	}
-	mutex_unlock(&dev->struct_mutex);
 
 	return ret;
 }
@@ -957,11 +955,9 @@ static int armada_drm_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 	if (!dcrtc->variant->has_spu_adv_reg)
 		return -EFAULT;
 
-	mutex_lock(&dev->struct_mutex);
 	dcrtc->cursor_x = x;
 	dcrtc->cursor_y = y;
 	ret = armada_drm_crtc_cursor_update(dcrtc, false);
-	mutex_unlock(&dev->struct_mutex);
 
 	return ret;
 }
-- 
2.5.1

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

[PATCH 13/29] drm/armada: Use a private mutex to protect priv->linear

[Daniel Vetter]

Reusing the Big DRM Lock just leaks, and the few things left that
dev->struct_mutex protected are very well contained - it's just the
linear drm_mm manager.

With this armada is completely struct_mutex free!

Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/armada/armada_debugfs.c |  4 ++--
 drivers/gpu/drm/armada/armada_drm.h     |  3 ++-
 drivers/gpu/drm/armada/armada_drv.c     |  1 +
 drivers/gpu/drm/armada/armada_gem.c     | 10 +++++-----
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c
index 471e45627f1e..d4f7ab0a30d4 100644
--- a/drivers/gpu/drm/armada/armada_debugfs.c
+++ b/drivers/gpu/drm/armada/armada_debugfs.c
@@ -21,9 +21,9 @@ static int armada_debugfs_gem_linear_show(struct seq_file *m, void *data)
 	struct armada_private *priv = dev->dev_private;
 	int ret;
 
-	mutex_lock(&dev->struct_mutex);
+	mutex_lock(&priv->linear_lock);
 	ret = drm_mm_dump_table(m, &priv->linear);
-	mutex_unlock(&dev->struct_mutex);
+	mutex_unlock(&priv->linear_lock);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/armada/armada_drm.h b/drivers/gpu/drm/armada/armada_drm.h
index 4df6f2af2b21..6d2d8be38297 100644
--- a/drivers/gpu/drm/armada/armada_drm.h
+++ b/drivers/gpu/drm/armada/armada_drm.h
@@ -57,7 +57,8 @@ struct armada_private {
 	DECLARE_KFIFO(fb_unref, struct drm_framebuffer *, 8);
 	struct drm_fb_helper	*fbdev;
 	struct armada_crtc	*dcrtc[2];
-	struct drm_mm		linear;
+	struct drm_mm		linear; /* protectec by linear_lock */
+	struct mutex		linear_lock;
 	struct drm_property	*csc_yuv_prop;
 	struct drm_property	*csc_rgb_prop;
 	struct drm_property	*colorkey_prop;
diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 77ab93d60125..3bd7e1cde99e 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -102,6 +102,7 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags)
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.funcs = &armada_drm_mode_config_funcs;
 	drm_mm_init(&priv->linear, mem->start, resource_size(mem));
+	mutex_init(&priv->linear_lock);
 
 	ret = component_bind_all(dev->dev, dev);
 	if (ret)
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index e3a86b96af2a..a43690ab18b9 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -46,7 +46,7 @@ static size_t roundup_gem_size(size_t size)
 	return roundup(size, PAGE_SIZE);
 }
 
-/* dev->struct_mutex is held here */
+/* dev_priv->linear_lock is held here */
 void armada_gem_free_object(struct drm_gem_object *obj)
 {
 	struct armada_gem_object *dobj = drm_to_armada_gem(obj);
@@ -144,10 +144,10 @@ armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj)
 		if (!node)
 			return -ENOSPC;
 
-		mutex_lock(&dev->struct_mutex);
+		mutex_lock(&priv->linear_lock);
 		ret = drm_mm_insert_node(&priv->linear, node, size, align,
 					 DRM_MM_SEARCH_DEFAULT);
-		mutex_unlock(&dev->struct_mutex);
+		mutex_unlock(&priv->linear_lock);
 		if (ret) {
 			kfree(node);
 			return ret;
@@ -158,9 +158,9 @@ armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj)
 		/* Ensure that the memory we're returning is cleared. */
 		ptr = ioremap_wc(obj->linear->start, size);
 		if (!ptr) {
-			mutex_lock(&dev->struct_mutex);
+			mutex_lock(&priv->linear_lock);
 			drm_mm_remove_node(obj->linear);
-			mutex_unlock(&dev->struct_mutex);
+			mutex_unlock(&priv->linear_lock);
 			kfree(obj->linear);
 			obj->linear = NULL;
 			return -ENOMEM;
-- 
2.5.1

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

[PATCH 14/29] 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).

v2: Finally get rid of the copypasta from another commit in this
commit message. And convert to _unlocked like we need to (Patrik).

Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Acked-by: Thierry Reding <thierry.reding@gmail.com>
Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/tegra/gem.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index 01e16e146bfe..fb712316c522 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;
 	}
 
@@ -421,9 +418,7 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
 
 	*offset = drm_vma_node_offset_addr(&bo->gem.vma_node);
 
-	drm_gem_object_unreference(gem);
-
-	mutex_unlock(&drm->struct_mutex);
+	drm_gem_object_unreference_unlocked(gem);
 
 	return 0;
 }
-- 
2.5.1

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

[PATCH 15/29] drm/tegra: Use drm_gem_object_unreference_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!

v2: refernce_unlocked doesn't exist as kbuild spotted.

Cc: Thierry Reding <thierry.reding@gmail.com>
Acked-by: 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 | 6 +-----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 3479c57151af..34d067b677bd 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -277,9 +277,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 fb712316c522..fbec29516bcb 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);
 
 	return bo;
 }
-- 
2.5.1

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

[PATCH 16/29] drm/gma500: Use correct unref in the gem bo create function

[Daniel Vetter]

This is called without dev->struct_mutex held, we need to use the
_unlocked variant.

Never caught in the wild since you'd need an evil userspace which
races a gem_close ioctl call with the in-progress open.

Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Acked-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/gma500/gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index c707fa6fca85..e3bdc8b1c32c 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -130,7 +130,7 @@ int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size,
 		return ret;
 	}
 	/* We have the initial and handle reference but need only one now */
-	drm_gem_object_unreference(&r->gem);
+	drm_gem_object_unreference_unlocked(&r->gem);
 	*handlep = handle;
 	return 0;
 }
-- 
2.5.1

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

[PATCH 17/29] drm/gma500: Drop dev->struct_mutex from modeset code

[Daniel Vetter]

It's either init code or already protected by other means. Note
that psb_gtt_pin/unpin has it's own lock, and that's really the
only piece of driver private state - all the modeset state is
protected with modeset locks already.

The only important bit is to switch all gem_obj_unref calls to the
_unlocked variant.

Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Acked-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/gma500/gma_display.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
index 001b450b27b3..ff17af4cfc64 100644
--- a/drivers/gpu/drm/gma500/gma_display.c
+++ b/drivers/gpu/drm/gma500/gma_display.c
@@ -349,8 +349,6 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 	/* If we didn't get a handle then turn the cursor off */
 	if (!handle) {
 		temp = CURSOR_MODE_DISABLE;
-		mutex_lock(&dev->struct_mutex);
-
 		if (gma_power_begin(dev, false)) {
 			REG_WRITE(control, temp);
 			REG_WRITE(base, 0);
@@ -362,11 +360,9 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 			gt = container_of(gma_crtc->cursor_obj,
 					  struct gtt_range, gem);
 			psb_gtt_unpin(gt);
-			drm_gem_object_unreference(gma_crtc->cursor_obj);
+			drm_gem_object_unreference_unlocked(gma_crtc->cursor_obj);
 			gma_crtc->cursor_obj = NULL;
 		}
-
-		mutex_unlock(&dev->struct_mutex);
 		return 0;
 	}
 
@@ -376,7 +372,6 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 		return -EINVAL;
 	}
 
-	mutex_lock(&dev->struct_mutex);
 	obj = drm_gem_object_lookup(dev, file_priv, handle);
 	if (!obj) {
 		ret = -ENOENT;
@@ -441,17 +436,15 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc,
 	if (gma_crtc->cursor_obj) {
 		gt = container_of(gma_crtc->cursor_obj, struct gtt_range, gem);
 		psb_gtt_unpin(gt);
-		drm_gem_object_unreference(gma_crtc->cursor_obj);
+		drm_gem_object_unreference_unlocked(gma_crtc->cursor_obj);
 	}
 
 	gma_crtc->cursor_obj = obj;
 unlock:
-	mutex_unlock(&dev->struct_mutex);
 	return ret;
 
 unref_cursor:
-	drm_gem_object_unreference(obj);
-	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(obj);
 	return ret;
 }
 
-- 
2.5.1

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

[PATCH 18/29] drm/gma500: Drop dev->struct_mutex from fbdev init/teardown code

[Daniel Vetter]

This is init/teardown code, locking is just to appease locking checks.
And since gem create/free doesn't need this any more there's really no
reason for grabbing dev->struct_mutex.

Again important to switch obj_unref to _unlocked variants.

Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Acked-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/gma500/framebuffer.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index dc0508dca1d4..ee95c03a8c54 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -406,8 +406,6 @@ static int psbfb_create(struct psb_fbdev *fbdev,
 
 	memset(dev_priv->vram_addr + backing->offset, 0, size);
 
-	mutex_lock(&dev->struct_mutex);
-
 	info = drm_fb_helper_alloc_fbi(&fbdev->psb_fb_helper);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
@@ -463,17 +461,15 @@ static int psbfb_create(struct psb_fbdev *fbdev,
 	dev_dbg(dev->dev, "allocated %dx%d fb\n",
 					psbfb->base.width, psbfb->base.height);
 
-	mutex_unlock(&dev->struct_mutex);
 	return 0;
 out_unref:
 	if (backing->stolen)
 		psb_gtt_free_range(dev, backing);
 	else
-		drm_gem_object_unreference(&backing->gem);
+		drm_gem_object_unreference_unlocked(&backing->gem);
 
 	drm_fb_helper_release_fbi(&fbdev->psb_fb_helper);
 out_err1:
-	mutex_unlock(&dev->struct_mutex);
 	psb_gtt_free_range(dev, backing);
 	return ret;
 }
@@ -569,7 +565,7 @@ static int psb_fbdev_destroy(struct drm_device *dev, struct psb_fbdev *fbdev)
 	drm_framebuffer_cleanup(&psbfb->base);
 
 	if (psbfb->gtt)
-		drm_gem_object_unreference(&psbfb->gtt->gem);
+		drm_gem_object_unreference_unlocked(&psbfb->gtt->gem);
 	return 0;
 }
 
@@ -784,12 +780,8 @@ void psb_modeset_cleanup(struct drm_device *dev)
 {
 	struct drm_psb_private *dev_priv = dev->dev_private;
 	if (dev_priv->modeset) {
-		mutex_lock(&dev->struct_mutex);
-
 		drm_kms_helper_poll_fini(dev);
 		psb_fbdev_fini(dev);
 		drm_mode_config_cleanup(dev);
-
-		mutex_unlock(&dev->struct_mutex);
 	}
 }
-- 
2.5.1

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

[PATCH 19/29] drm/gma500: Drop dev->struct_mutex from mmap offset function

[Daniel Vetter]

Simply forgotten about this when I was doing my general cleansing of
simple gem mmap offset functions. There's nothing but core functions
called here, and they all have their own protection already.

Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Acked-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/gma500/gem.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index e3bdc8b1c32c..f0357f525f56 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -62,15 +62,10 @@ int psb_gem_dumb_map_gtt(struct drm_file *file, struct drm_device *dev,
 	int ret = 0;
 	struct drm_gem_object *obj;
 
-	mutex_lock(&dev->struct_mutex);
-
 	/* GEM does all our handle to object mapping */
 	obj = drm_gem_object_lookup(dev, file, handle);
-	if (obj == NULL) {
-		ret = -ENOENT;
-		goto unlock;
-	}
-	/* What validation is needed here ? */
+	if (obj == NULL)
+		return -ENOENT;
 
 	/* Make it mmapable */
 	ret = drm_gem_create_mmap_offset(obj);
@@ -78,9 +73,7 @@ int psb_gem_dumb_map_gtt(struct drm_file *file, struct drm_device *dev,
 		goto out;
 	*offset = drm_vma_node_offset_addr(&obj->vma_node);
 out:
-	drm_gem_object_unreference(obj);
-unlock:
-	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(obj);
 	return ret;
 }
 
-- 
2.5.1

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

[PATCH 20/29] drm/gma500: Add driver private mutex for the fault handler

[Daniel Vetter]

There's currently two places where the gma500 fault handler relies
upon dev->struct_mutex:
- To protect r->mappping
- To make sure vm_insert_pfn isn't called concurrently (in which case
  the 2nd thread would get an error code).

Everything else (specifically psb_gtt_pin) is already protected by
some other locks. Hence just create a new driver-private mmap_mutex
just for this function.

With this gma500 is complete dev->struct_mutex free!

Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Acked-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/gma500/gem.c     | 4 ++--
 drivers/gpu/drm/gma500/gtt.c     | 1 +
 drivers/gpu/drm/gma500/psb_drv.h | 2 ++
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index f0357f525f56..506224b3a0ad 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -182,7 +182,7 @@ int psb_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 
 	/* Make sure we don't parallel update on a fault, nor move or remove
 	   something from beneath our feet */
-	mutex_lock(&dev->struct_mutex);
+	mutex_lock(&dev_priv->mmap_mutex);
 
 	/* For now the mmap pins the object and it stays pinned. As things
 	   stand that will do us no harm */
@@ -208,7 +208,7 @@ int psb_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	ret = vm_insert_pfn(vma, (unsigned long)vmf->virtual_address, pfn);
 
 fail:
-	mutex_unlock(&dev->struct_mutex);
+	mutex_unlock(&dev_priv->mmap_mutex);
 	switch (ret) {
 	case 0:
 	case -ERESTARTSYS:
diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c
index ce015db59dc6..8f69225ce2b4 100644
--- a/drivers/gpu/drm/gma500/gtt.c
+++ b/drivers/gpu/drm/gma500/gtt.c
@@ -425,6 +425,7 @@ int psb_gtt_init(struct drm_device *dev, int resume)
 
 	if (!resume) {
 		mutex_init(&dev_priv->gtt_mutex);
+		mutex_init(&dev_priv->mmap_mutex);
 		psb_gtt_alloc(dev);
 	}
 
diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h
index e21726ecac32..3bd2c726dd61 100644
--- a/drivers/gpu/drm/gma500/psb_drv.h
+++ b/drivers/gpu/drm/gma500/psb_drv.h
@@ -465,6 +465,8 @@ struct drm_psb_private {
 	struct mutex gtt_mutex;
 	struct resource *gtt_mem;	/* Our PCI resource */
 
+	struct mutex mmap_mutex;
+
 	struct psb_mmu_driver *mmu;
 	struct psb_mmu_pd *pf_pd;
 
-- 
2.5.1

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

[PATCH 21/29] drm/nouveau: Drop dev->struct_mutex from fbdev init

[Daniel Vetter]

Doesn't protect anything at all.

With this patch nouveau is completely dev->struct_mutex free!

Cc: Ben Skeggs <bskeggs@redhat.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/nouveau/nouveau_fbcon.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
index 59f27e774acb..3bae706126bd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
@@ -386,8 +386,6 @@ nouveau_fbcon_create(struct drm_fb_helper *helper,
 		}
 	}
 
-	mutex_lock(&dev->struct_mutex);
-
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
 		ret = PTR_ERR(info);
@@ -426,8 +424,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);
@@ -441,7 +437,6 @@ nouveau_fbcon_create(struct drm_fb_helper *helper,
 	return 0;
 
 out_unlock:
-	mutex_unlock(&dev->struct_mutex);
 	if (chan)
 		nouveau_bo_vma_del(nvbo, &fbcon->nouveau_fb.vma);
 	nouveau_bo_unmap(nvbo);
-- 
2.5.1

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

[PATCH 22/29] drm/exynos: Drop dev->struct_mutex from mmap offset function

[Daniel Vetter]

Simply forgotten about this when I was doing my general cleansing of
simple gem mmap offset functions. There's nothing but core functions
called here, and they all have their own protection already.

Aside: DRM_ERROR for userspace controlled input isn't great, but
that's for another patch.

v2: Use _unlocked unreference (Daniel Stone).

Cc: Daniel Stone <daniel@fooishbar.org>
Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 252eb301470c..44e710ab84de 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -448,8 +448,6 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv,
 	struct drm_gem_object *obj;
 	int ret = 0;
 
-	mutex_lock(&dev->struct_mutex);
-
 	/*
 	 * get offset of memory allocated for drm framebuffer.
 	 * - this callback would be called by user application
@@ -459,16 +457,13 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv,
 	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;
 	}
 
 	*offset = drm_vma_node_offset_addr(&obj->vma_node);
 	DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset);
 
-	drm_gem_object_unreference(obj);
-unlock:
-	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(obj);
 	return ret;
 }
 
-- 
2.5.1

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

[PATCH 23/29] drm/exynos: drop struct_mutex from exynos_gem_map_sgt_with_dma

[Daniel Vetter]

The sg table isn't refcounted, there's no corresponding locking for
unmapping and drm_map_sg is ok with being called concurrently.

So drop the locking since it doesn't protect anything.

Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 44e710ab84de..b198fa560106 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -378,16 +378,12 @@ int exynos_gem_map_sgt_with_dma(struct drm_device *drm_dev,
 {
 	int nents;
 
-	mutex_lock(&drm_dev->struct_mutex);
-
 	nents = dma_map_sg(drm_dev->dev, sgt->sgl, sgt->nents, dir);
 	if (!nents) {
 		DRM_ERROR("failed to map sgl with dma.\n");
-		mutex_unlock(&drm_dev->struct_mutex);
 		return nents;
 	}
 
-	mutex_unlock(&drm_dev->struct_mutex);
 	return 0;
 }
 
-- 
2.5.1

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

[PATCH 24/29] drm/exynos: drop struct_mutex from exynos_drm_gem_get_ioctl

[Daniel Vetter]

The only things this protects is reading ->flags and ->size, both of
which are invariant over the lifetime of an exynos gem bo. So no
locking needed at all (besides that, nothing protects the writers
anyway).

Aside: exynos_gem_obj->size is redundant with
exynos_gem_obj->base.size and probably should be removed.

v2: Use _unlocked unreference (Daniel Stone).

Cc: Daniel Stone <daniel@fooishbar.org>
Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index b198fa560106..e735827e0c6d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -352,12 +352,9 @@ int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data,
 	struct drm_exynos_gem_info *args = data;
 	struct drm_gem_object *obj;
 
-	mutex_lock(&dev->struct_mutex);
-
 	obj = drm_gem_object_lookup(dev, file_priv, args->handle);
 	if (!obj) {
 		DRM_ERROR("failed to lookup gem object.\n");
-		mutex_unlock(&dev->struct_mutex);
 		return -EINVAL;
 	}
 
@@ -366,8 +363,7 @@ int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data,
 	args->flags = exynos_gem->flags;
 	args->size = exynos_gem->size;
 
-	drm_gem_object_unreference(obj);
-	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(obj);
 
 	return 0;
 }
-- 
2.5.1

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

[PATCH 25/29] drm/exynos: drop struct_mutex from fbdev setup

[Daniel Vetter]

Doesn't protect anything at all, and probably just here because a long
time ago dev->struct_mutex was required to allocate gem objects.

With this patch exynos is completely struct_mutex free!

Cc: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index f6118baa8e3e..e1f7abaa61df 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -138,8 +138,6 @@ static int exynos_drm_fbdev_create(struct drm_fb_helper *helper,
 	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
 							  sizes->surface_depth);
 
-	mutex_lock(&dev->struct_mutex);
-
 	size = mode_cmd.pitches[0] * mode_cmd.height;
 
 	exynos_gem = exynos_drm_gem_create(dev, EXYNOS_BO_CONTIG, size);
@@ -154,10 +152,8 @@ static int exynos_drm_fbdev_create(struct drm_fb_helper *helper,
 						   size);
 	}
 
-	if (IS_ERR(exynos_gem)) {
-		ret = PTR_ERR(exynos_gem);
-		goto out;
-	}
+	if (IS_ERR(exynos_gem))
+		return PTR_ERR(exynos_gem);
 
 	exynos_fbdev->exynos_gem = exynos_gem;
 
@@ -173,7 +169,6 @@ static int exynos_drm_fbdev_create(struct drm_fb_helper *helper,
 	if (ret < 0)
 		goto err_destroy_framebuffer;
 
-	mutex_unlock(&dev->struct_mutex);
 	return ret;
 
 err_destroy_framebuffer:
@@ -181,13 +176,12 @@ err_destroy_framebuffer:
 err_destroy_gem:
 	exynos_drm_gem_destroy(exynos_gem);
 
-/*
- * if failed, all resources allocated above would be released by
- * drm_mode_config_cleanup() when drm_load() had been called prior
- * to any specific driver such as fimd or hdmi driver.
- */
-out:
-	mutex_unlock(&dev->struct_mutex);
+	/*
+	 * if failed, all resources allocated above would be released by
+	 * drm_mode_config_cleanup() when drm_load() had been called prior
+	 * to any specific driver such as fimd or hdmi driver.
+	 */
+
 	return ret;
 }
 
-- 
2.5.1

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

[PATCH 26/29] drm/vgem: Simplify dum_map

[Daniel Vetter]

The offset manager already checks for existing offsets internally,
while holding suitable locks. We can drop this check.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/vgem/vgem_drv.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 02ae60c395b7..63026d4324ad 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -208,11 +208,9 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev,
 		goto unlock;
 	}
 
-	if (!drm_vma_node_has_offset(&obj->vma_node)) {
-		ret = drm_gem_create_mmap_offset(obj);
-		if (ret)
-			goto unref;
-	}
+	ret = drm_gem_create_mmap_offset(obj);
+	if (ret)
+		goto unref;
 
 	BUG_ON(!obj->filp);
 
-- 
2.5.1

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

[PATCH 27/29] drm/vgem: Move get_pages to gem_create

[Daniel Vetter]

vgem doesn't have a shrinker or anything like that and drops backing
storage only at object_free time. There's no use in trying to be
clever and allocating backing storage delayed, it only causes trouble
by requiring locking.

Instead grab pages when we allocate the object right away.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/vgem/vgem_drv.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 63026d4324ad..1a609347236b 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -154,6 +154,10 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
 	if (err)
 		goto out;
 
+	ret = vgem_gem_get_pages(to_vgem_bo(obj));
+	if (ret)
+		goto out;
+
 	err = drm_gem_handle_create(file, gem_object, handle);
 	if (err)
 		goto handle_out;
@@ -216,16 +220,8 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev,
 
 	obj->filp->private_data = obj;
 
-	ret = vgem_gem_get_pages(to_vgem_bo(obj));
-	if (ret)
-		goto fail_get_pages;
-
 	*offset = drm_vma_node_offset_addr(&obj->vma_node);
 
-	goto unref;
-
-fail_get_pages:
-	drm_gem_free_mmap_offset(obj);
 unref:
 	drm_gem_object_unreference(obj);
 unlock:
-- 
2.5.1

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

[PATCH 28/29] drm/vgem: Drop dev->struct_mutex

[Daniel Vetter]

With the previous two changes it doesn't protect anything any more.

v2: Use _unlocked unreference variant.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/vgem/vgem_drv.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 1a609347236b..807a24d3c0a8 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -103,12 +103,8 @@ static int vgem_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	if (page_offset > num_pages)
 		return VM_FAULT_SIGBUS;
 
-	mutex_lock(&dev->struct_mutex);
-
 	ret = vm_insert_page(vma, (unsigned long)vmf->virtual_address,
 			     obj->pages[page_offset]);
-
-	mutex_unlock(&dev->struct_mutex);
 	switch (ret) {
 	case 0:
 		return VM_FAULT_NOPAGE;
@@ -205,12 +201,9 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev,
 	int ret = 0;
 	struct drm_gem_object *obj;
 
-	mutex_lock(&dev->struct_mutex);
 	obj = drm_gem_object_lookup(dev, file, handle);
-	if (!obj) {
-		ret = -ENOENT;
-		goto unlock;
-	}
+	if (!obj)
+		return -ENOENT;
 
 	ret = drm_gem_create_mmap_offset(obj);
 	if (ret)
@@ -223,9 +216,8 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev,
 	*offset = drm_vma_node_offset_addr(&obj->vma_node);
 
 unref:
-	drm_gem_object_unreference(obj);
-unlock:
-	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(obj);
+
 	return ret;
 }
 
-- 
2.5.1

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

[PATCH 29/29] drm/vma_manage: Drop has_offset

[Daniel Vetter]

It's racy, creating mmap offsets is a slowpath, so better to remove it
to avoid drivers doing broken things.

The only user is i915, and it's ok there because everything (well
almost) is protected by dev->struct_mutex in i915-gem.

While at it add a note in the create_mmap_offset kerneldoc that
drivers must release it again. And then I also noticed that
drm_gem_object_release entirely lacks kerneldoc.

Cc: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_gem.c       | 17 +++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c |  3 ---
 include/drm/drm_vma_manager.h   | 15 +--------------
 3 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 2e10bba4468b..5c5001171dd3 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -392,6 +392,10 @@ EXPORT_SYMBOL(drm_gem_handle_create);
  * @obj: obj in question
  *
  * This routine frees fake offsets allocated by drm_gem_create_mmap_offset().
+ *
+ * Note that drm_gem_object_release() already calls this function, so drivers
+ * don't have to take care of releasing the mmap offset themselves when freeing
+ * the GEM object.
  */
 void
 drm_gem_free_mmap_offset(struct drm_gem_object *obj)
@@ -415,6 +419,9 @@ EXPORT_SYMBOL(drm_gem_free_mmap_offset);
  * This routine allocates and attaches a fake offset for @obj, in cases where
  * the virtual size differs from the physical size (ie. obj->size).  Otherwise
  * just use drm_gem_create_mmap_offset().
+ *
+ * This function is idempotent and handles an already allocated mmap offset
+ * transparently. Drivers do not need to check for this case.
  */
 int
 drm_gem_create_mmap_offset_size(struct drm_gem_object *obj, size_t size)
@@ -436,6 +443,9 @@ EXPORT_SYMBOL(drm_gem_create_mmap_offset_size);
  * structures.
  *
  * This routine allocates and attaches a fake offset for @obj.
+ *
+ * Drivers can call drm_gem_free_mmap_offset() before freeing @obj to release
+ * the fake offset again.
  */
 int drm_gem_create_mmap_offset(struct drm_gem_object *obj)
 {
@@ -754,6 +764,13 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private)
 	idr_destroy(&file_private->object_idr);
 }
 
+/**
+ * drm_gem_object_release - release GEM buffer object resources
+ * @obj: GEM buffer object
+ *
+ * This releases any structures and resources used by @obj and is the invers of
+ * drm_gem_object_init().
+ */
 void
 drm_gem_object_release(struct drm_gem_object *obj)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 33adc8f8ab20..f10a5d57377e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1972,9 +1972,6 @@ static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
 	int ret;
 
-	if (drm_vma_node_has_offset(&obj->base.vma_node))
-		return 0;
-
 	dev_priv->mm.shrinker_no_lock_stealing = true;
 
 	ret = drm_gem_create_mmap_offset(&obj->base);
diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
index 2f63dd5e05eb..06ea8e077ec2 100644
--- a/include/drm/drm_vma_manager.h
+++ b/include/drm/drm_vma_manager.h
@@ -176,19 +176,6 @@ static inline unsigned long drm_vma_node_size(struct drm_vma_offset_node *node)
 }
 
 /**
- * drm_vma_node_has_offset() - Check whether node is added to offset manager
- * @node: Node to be checked
- *
- * RETURNS:
- * true iff the node was previously allocated an offset and added to
- * an vma offset manager.
- */
-static inline bool drm_vma_node_has_offset(struct drm_vma_offset_node *node)
-{
-	return drm_mm_node_allocated(&node->vm_node);
-}
-
-/**
  * drm_vma_node_offset_addr() - Return sanitized offset for user-space mmaps
  * @node: Linked offset node
  *
@@ -220,7 +207,7 @@ static inline __u64 drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
 static inline void drm_vma_node_unmap(struct drm_vma_offset_node *node,
 				      struct address_space *file_mapping)
 {
-	if (drm_vma_node_has_offset(node))
+	if (drm_mm_node_allocated(&node->vm_node))
 		unmap_mapping_range(file_mapping,
 				    drm_vma_node_offset_addr(node),
 				    drm_vma_node_size(node) << PAGE_SHIFT, 1);
-- 
2.5.1

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

[PATCH] v2: Also add a DRIVER_* check like for all other maps functions to really short-circuit the code. And give drm_legacy_rmmap used by the dev unregister code the same treatment.

[Daniel Vetter]

---
 drivers/gpu/drm/drm_bufs.c | 14 ++++++++++----
 include/drm/drm_legacy.h   |  2 +-
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
index de18b8b78a26..5a51633da033 100644
--- a/drivers/gpu/drm/drm_bufs.c
+++ b/drivers/gpu/drm/drm_bufs.c
@@ -542,15 +542,17 @@ int drm_legacy_rmmap_locked(struct drm_device *dev, struct drm_local_map *map)
 }
 EXPORT_SYMBOL(drm_legacy_rmmap_locked);
 
-int drm_legacy_rmmap(struct drm_device *dev, struct drm_local_map *map)
+void drm_legacy_rmmap(struct drm_device *dev, struct drm_local_map *map)
 {
-	int ret;
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
+	    drm_core_check_feature(dev, DRIVER_MODESET))
+		return;
 
 	mutex_lock(&dev->struct_mutex);
-	ret = drm_legacy_rmmap_locked(dev, map);
+	drm_legacy_rmmap_locked(dev, map);
 	mutex_unlock(&dev->struct_mutex);
 
-	return ret;
+	return;
 }
 EXPORT_SYMBOL(drm_legacy_rmmap);
 
@@ -558,6 +560,10 @@ void drm_legacy_master_rmmaps(struct drm_device *dev, struct drm_master *master)
 {
 	struct drm_map_list *r_list, *list_temp;
 
+	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
+	    drm_core_check_feature(dev, DRIVER_MODESET))
+		return;
+
 	mutex_lock(&dev->struct_mutex);
 	list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) {
 		if (r_list->master == master) {
diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h
index 24504b0eafea..a5ef2c7e40f8 100644
--- a/include/drm/drm_legacy.h
+++ b/include/drm/drm_legacy.h
@@ -154,7 +154,7 @@ struct drm_map_list {
 int drm_legacy_addmap(struct drm_device *d, resource_size_t offset,
 		      unsigned int size, enum drm_map_type type,
 		      enum drm_map_flags flags, struct drm_local_map **map_p);
-int drm_legacy_rmmap(struct drm_device *d, struct drm_local_map *map);
+void drm_legacy_rmmap(struct drm_device *d, struct drm_local_map *map);
 int drm_legacy_rmmap_locked(struct drm_device *d, struct drm_local_map *map);
 void drm_legacy_master_rmmaps(struct drm_device *dev,
 			      struct drm_master *master);
-- 
2.5.1

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

Re: [PATCH 04/29] drm/amdgpu: Use unlocked gem unreferencing

[Christian König]

On 23.11.2015 10:32, Daniel Vetter wrote:
> For drm_gem_object_unreference callers are required to hold
> dev->struct_mutex, which these paths don't. Enforcing this requirement
> has become a bit more strict with
>
> commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Oct 15 09:36:25 2015 +0200
>
>      drm/gem: Check locking in drm_gem_object_unreference
>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

For this and the radeon patch Reviewed-by: Christian König 
<christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> index 6fcbbcc2e99e..cfb6caad2a73 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> @@ -263,7 +263,7 @@ out_unref:
>   
>   	}
>   	if (fb && ret) {
> -		drm_gem_object_unreference(gobj);
> +		drm_gem_object_unreference_unlocked(gobj);
>   		drm_framebuffer_unregister_private(fb);
>   		drm_framebuffer_cleanup(fb);
>   		kfree(fb);


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

Re: [PATCH 25/29] drm/exynos: drop struct_mutex from fbdev setup

[Daniel Stone]

Hi,

On 23 November 2015 at 09:32, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Doesn't protect anything at all, and probably just here because a long
> time ago dev->struct_mutex was required to allocate gem objects.
>
> With this patch exynos is completely struct_mutex free!
>
> Cc: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Daniel Stone <daniels@collabora.com>

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

Re: [PATCH 22/29] drm/exynos: Drop dev->struct_mutex from mmap offset function

[Daniel Stone]

Hi,

On 23 November 2015 at 09:32, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Simply forgotten about this when I was doing my general cleansing of
> simple gem mmap offset functions. There's nothing but core functions
> called here, and they all have their own protection already.
>
> Aside: DRM_ERROR for userspace controlled input isn't great, but
> that's for another patch.
>
> v2: Use _unlocked unreference (Daniel Stone).
>
> Cc: Daniel Stone <daniel@fooishbar.org>
> Cc: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Daniel Stone <daniels@collabora.com>

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

Re: [PATCH 24/29] drm/exynos: drop struct_mutex from exynos_drm_gem_get_ioctl

[Daniel Stone]

Hi,

On 23 November 2015 at 09:32, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The only things this protects is reading ->flags and ->size, both of
> which are invariant over the lifetime of an exynos gem bo. So no
> locking needed at all (besides that, nothing protects the writers
> anyway).
>
> Aside: exynos_gem_obj->size is redundant with
> exynos_gem_obj->base.size and probably should be removed.
>
> v2: Use _unlocked unreference (Daniel Stone).
>
> Cc: Daniel Stone <daniel@fooishbar.org>
> Cc: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Daniel Stone <daniels@collabora.com>

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

Re: [PATCH 07/29] drm/tegra: Use unlocked gem unreferencing

[Thierry Reding]

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

On Mon, Nov 23, 2015 at 10:32:40AM +0100, Daniel Vetter wrote:
> For drm_gem_object_unreference callers are required to hold
> dev->struct_mutex, which these paths don't. Enforcing this requirement
> has become a bit more strict with
> 
> commit ef4c6270bf2867e2f8032e9614d1a8cfc6c71663
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Oct 15 09:36:25 2015 +0200
> 
>     drm/gem: Check locking in drm_gem_object_unreference
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/tegra/drm.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Applied, 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 14/29] drm/tegra: don't take dev->struct_mutex in mmap offset ioctl

[Thierry Reding]

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

On Mon, Nov 23, 2015 at 10:32:47AM +0100, 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).
> 
> v2: Finally get rid of the copypasta from another commit in this
> commit message. And convert to _unlocked like we need to (Patrik).
> 
> Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Acked-by: Thierry Reding <thierry.reding@gmail.com>
> Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/tegra/gem.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)

Applied, thanks.

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 15/29] drm/tegra: Use drm_gem_object_unreference_unlocked

[Thierry Reding]

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

On Mon, Nov 23, 2015 at 10:32:48AM +0100, Daniel Vetter wrote:
> 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!
> 
> v2: refernce_unlocked doesn't exist as kbuild spotted.
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Acked-by: 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 | 6 +-----
>  2 files changed, 2 insertions(+), 8 deletions(-)

Applied, thanks.

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 23/29] drm/exynos: drop struct_mutex from exynos_gem_map_sgt_with_dma

[Daniel Stone]

Hi,

On 23 November 2015 at 09:32, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The sg table isn't refcounted, there's no corresponding locking for
> unmapping and drm_map_sg is ok with being called concurrently.
>
> So drop the locking since it doesn't protect anything.
>
> Cc: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Daniel Stone <daniels@collabora.com>

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

Re: [PATCH] v2: Also add a DRIVER_* check like for all other maps functions to really short-circuit the code. And give drm_legacy_rmmap used by the dev unregister code the same treatment.

[Daniel Vetter]

On Mon, Nov 23, 2015 at 12:15:18PM +0200, Jani Nikula wrote:
> 
> Accidental patch?

Yup, dunno how that one ended up in there ...
-Daniel

> 
> On Mon, 23 Nov 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > ---
> >  drivers/gpu/drm/drm_bufs.c | 14 ++++++++++----
> >  include/drm/drm_legacy.h   |  2 +-
> >  2 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
> > index de18b8b78a26..5a51633da033 100644
> > --- a/drivers/gpu/drm/drm_bufs.c
> > +++ b/drivers/gpu/drm/drm_bufs.c
> > @@ -542,15 +542,17 @@ int drm_legacy_rmmap_locked(struct drm_device *dev, struct drm_local_map *map)
> >  }
> >  EXPORT_SYMBOL(drm_legacy_rmmap_locked);
> >  
> > -int drm_legacy_rmmap(struct drm_device *dev, struct drm_local_map *map)
> > +void drm_legacy_rmmap(struct drm_device *dev, struct drm_local_map *map)
> >  {
> > -	int ret;
> > +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> > +	    drm_core_check_feature(dev, DRIVER_MODESET))
> > +		return;
> >  
> >  	mutex_lock(&dev->struct_mutex);
> > -	ret = drm_legacy_rmmap_locked(dev, map);
> > +	drm_legacy_rmmap_locked(dev, map);
> >  	mutex_unlock(&dev->struct_mutex);
> >  
> > -	return ret;
> > +	return;
> >  }
> >  EXPORT_SYMBOL(drm_legacy_rmmap);
> >  
> > @@ -558,6 +560,10 @@ void drm_legacy_master_rmmaps(struct drm_device *dev, struct drm_master *master)
> >  {
> >  	struct drm_map_list *r_list, *list_temp;
> >  
> > +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> > +	    drm_core_check_feature(dev, DRIVER_MODESET))
> > +		return;
> > +
> >  	mutex_lock(&dev->struct_mutex);
> >  	list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) {
> >  		if (r_list->master == master) {
> > diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h
> > index 24504b0eafea..a5ef2c7e40f8 100644
> > --- a/include/drm/drm_legacy.h
> > +++ b/include/drm/drm_legacy.h
> > @@ -154,7 +154,7 @@ struct drm_map_list {
> >  int drm_legacy_addmap(struct drm_device *d, resource_size_t offset,
> >  		      unsigned int size, enum drm_map_type type,
> >  		      enum drm_map_flags flags, struct drm_local_map **map_p);
> > -int drm_legacy_rmmap(struct drm_device *d, struct drm_local_map *map);
> > +void drm_legacy_rmmap(struct drm_device *d, struct drm_local_map *map);
> >  int drm_legacy_rmmap_locked(struct drm_device *d, struct drm_local_map *map);
> >  void drm_legacy_master_rmmaps(struct drm_device *dev,
> >  			      struct drm_master *master);
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
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: [Intel-gfx] [PATCH] v2: Also add a DRIVER_* check like for all other maps functions to really short-circuit the code. And give drm_legacy_rmmap used by the dev unregister code the same treatment.

[Jani Nikula]

Accidental patch?

On Mon, 23 Nov 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> ---
>  drivers/gpu/drm/drm_bufs.c | 14 ++++++++++----
>  include/drm/drm_legacy.h   |  2 +-
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c
> index de18b8b78a26..5a51633da033 100644
> --- a/drivers/gpu/drm/drm_bufs.c
> +++ b/drivers/gpu/drm/drm_bufs.c
> @@ -542,15 +542,17 @@ int drm_legacy_rmmap_locked(struct drm_device *dev, struct drm_local_map *map)
>  }
>  EXPORT_SYMBOL(drm_legacy_rmmap_locked);
>  
> -int drm_legacy_rmmap(struct drm_device *dev, struct drm_local_map *map)
> +void drm_legacy_rmmap(struct drm_device *dev, struct drm_local_map *map)
>  {
> -	int ret;
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> +	    drm_core_check_feature(dev, DRIVER_MODESET))
> +		return;
>  
>  	mutex_lock(&dev->struct_mutex);
> -	ret = drm_legacy_rmmap_locked(dev, map);
> +	drm_legacy_rmmap_locked(dev, map);
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	return ret;
> +	return;
>  }
>  EXPORT_SYMBOL(drm_legacy_rmmap);
>  
> @@ -558,6 +560,10 @@ void drm_legacy_master_rmmaps(struct drm_device *dev, struct drm_master *master)
>  {
>  	struct drm_map_list *r_list, *list_temp;
>  
> +	if (!drm_core_check_feature(dev, DRIVER_KMS_LEGACY_CONTEXT) &&
> +	    drm_core_check_feature(dev, DRIVER_MODESET))
> +		return;
> +
>  	mutex_lock(&dev->struct_mutex);
>  	list_for_each_entry_safe(r_list, list_temp, &dev->maplist, head) {
>  		if (r_list->master == master) {
> diff --git a/include/drm/drm_legacy.h b/include/drm/drm_legacy.h
> index 24504b0eafea..a5ef2c7e40f8 100644
> --- a/include/drm/drm_legacy.h
> +++ b/include/drm/drm_legacy.h
> @@ -154,7 +154,7 @@ struct drm_map_list {
>  int drm_legacy_addmap(struct drm_device *d, resource_size_t offset,
>  		      unsigned int size, enum drm_map_type type,
>  		      enum drm_map_flags flags, struct drm_local_map **map_p);
> -int drm_legacy_rmmap(struct drm_device *d, struct drm_local_map *map);
> +void drm_legacy_rmmap(struct drm_device *d, struct drm_local_map *map);
>  int drm_legacy_rmmap_locked(struct drm_device *d, struct drm_local_map *map);
>  void drm_legacy_master_rmmaps(struct drm_device *dev,
>  			      struct drm_master *master);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/29] drm/armada: Use a private mutex to protect priv->linear

[Russell King - ARM Linux]

On Mon, Nov 23, 2015 at 10:32:46AM +0100, Daniel Vetter wrote:
> diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
> index e3a86b96af2a..a43690ab18b9 100644
> --- a/drivers/gpu/drm/armada/armada_gem.c
> +++ b/drivers/gpu/drm/armada/armada_gem.c
> @@ -46,7 +46,7 @@ static size_t roundup_gem_size(size_t size)
>  	return roundup(size, PAGE_SIZE);
>  }
>  
> -/* dev->struct_mutex is held here */
> +/* dev_priv->linear_lock is held here */
>  void armada_gem_free_object(struct drm_gem_object *obj)

This is wrong (unless there's changes which I'm not aware of.)

This function is called from drm_gem_object_free(), which is called from
drm_gem_object_unreference(), both of which contain:

        WARN_ON(!mutex_is_locked(&dev->struct_mutex));

Now, unless you're changing the conditions on which
drm_gem_object_unreference() to be under dev_priv->linear_lock, the
above comment becomes misleading.

It'd also need drm_gem_object_unreference_unlocked() to become per-
driver, so it can take dev_priv->linear_lock, and I don't see anything
in the patches which I've received which does that.

So, I suspect the new comment is basically false.

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

Re: [PATCH 13/29] drm/armada: Use a private mutex to protect priv->linear

[Daniel Vetter]

On Mon, Nov 23, 2015 at 6:40 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Nov 23, 2015 at 10:32:46AM +0100, Daniel Vetter wrote:
>> diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
>> index e3a86b96af2a..a43690ab18b9 100644
>> --- a/drivers/gpu/drm/armada/armada_gem.c
>> +++ b/drivers/gpu/drm/armada/armada_gem.c
>> @@ -46,7 +46,7 @@ static size_t roundup_gem_size(size_t size)
>>       return roundup(size, PAGE_SIZE);
>>  }
>>
>> -/* dev->struct_mutex is held here */
>> +/* dev_priv->linear_lock is held here */
>>  void armada_gem_free_object(struct drm_gem_object *obj)
>
> This is wrong (unless there's changes which I'm not aware of.)
>
> This function is called from drm_gem_object_free(), which is called from
> drm_gem_object_unreference(), both of which contain:
>
>         WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> Now, unless you're changing the conditions on which
> drm_gem_object_unreference() to be under dev_priv->linear_lock, the
> above comment becomes misleading.
>
> It'd also need drm_gem_object_unreference_unlocked() to become per-
> driver, so it can take dev_priv->linear_lock, and I don't see anything
> in the patches which I've received which does that.
>
> So, I suspect the new comment is basically false.

Well the patch is false. I should have grabbed the new
dev_priv->linear_lock around the cleanup functions in
armada_gem_free_object. Which is possible since armada never calls an
gem unref function while holding the new lock. Sorry for not
double-checking what I've done, I'll revise.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/armada: Use a private mutex to protect priv->linear

[Daniel Vetter]

Reusing the Big DRM Lock just leaks, and the few things left that
dev->struct_mutex protected are very well contained - it's just the
linear drm_mm manager.

With this armada is completely struct_mutex free!

v2: Convert things properly and also take the lock in
armage_gem_free_object, and remove the stale comment (Russell).

Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/armada/armada_debugfs.c |  4 ++--
 drivers/gpu/drm/armada/armada_drm.h     |  3 ++-
 drivers/gpu/drm/armada/armada_drv.c     |  1 +
 drivers/gpu/drm/armada/armada_gem.c     | 14 +++++++++-----
 4 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_debugfs.c b/drivers/gpu/drm/armada/armada_debugfs.c
index 471e45627f1e..d4f7ab0a30d4 100644
--- a/drivers/gpu/drm/armada/armada_debugfs.c
+++ b/drivers/gpu/drm/armada/armada_debugfs.c
@@ -21,9 +21,9 @@ static int armada_debugfs_gem_linear_show(struct seq_file *m, void *data)
 	struct armada_private *priv = dev->dev_private;
 	int ret;
 
-	mutex_lock(&dev->struct_mutex);
+	mutex_lock(&priv->linear_lock);
 	ret = drm_mm_dump_table(m, &priv->linear);
-	mutex_unlock(&dev->struct_mutex);
+	mutex_unlock(&priv->linear_lock);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/armada/armada_drm.h b/drivers/gpu/drm/armada/armada_drm.h
index 4df6f2af2b21..3b2bb6128d40 100644
--- a/drivers/gpu/drm/armada/armada_drm.h
+++ b/drivers/gpu/drm/armada/armada_drm.h
@@ -57,7 +57,8 @@ struct armada_private {
 	DECLARE_KFIFO(fb_unref, struct drm_framebuffer *, 8);
 	struct drm_fb_helper	*fbdev;
 	struct armada_crtc	*dcrtc[2];
-	struct drm_mm		linear;
+	struct drm_mm		linear; /* protected by linear_lock */
+	struct mutex		linear_lock;
 	struct drm_property	*csc_yuv_prop;
 	struct drm_property	*csc_rgb_prop;
 	struct drm_property	*colorkey_prop;
diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 77ab93d60125..3bd7e1cde99e 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -102,6 +102,7 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags)
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.funcs = &armada_drm_mode_config_funcs;
 	drm_mm_init(&priv->linear, mem->start, resource_size(mem));
+	mutex_init(&priv->linear_lock);
 
 	ret = component_bind_all(dev->dev, dev);
 	if (ret)
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index e3a86b96af2a..6e731db31aa4 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -46,22 +46,26 @@ static size_t roundup_gem_size(size_t size)
 	return roundup(size, PAGE_SIZE);
 }
 
-/* dev->struct_mutex is held here */
 void armada_gem_free_object(struct drm_gem_object *obj)
 {
 	struct armada_gem_object *dobj = drm_to_armada_gem(obj);
+	struct armada_private *priv = obj->dev->dev_private;
 
 	DRM_DEBUG_DRIVER("release obj %p\n", dobj);
 
 	drm_gem_free_mmap_offset(&dobj->obj);
 
+	might_lock(&priv->linear_lock);
+
 	if (dobj->page) {
 		/* page backed memory */
 		unsigned int order = get_order(dobj->obj.size);
 		__free_pages(dobj->page, order);
 	} else if (dobj->linear) {
 		/* linear backed memory */
+		mutex_lock(&priv->linear_lock);
 		drm_mm_remove_node(dobj->linear);
+		mutex_unlock(&priv->linear_lock);
 		kfree(dobj->linear);
 		if (dobj->addr)
 			iounmap(dobj->addr);
@@ -144,10 +148,10 @@ armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj)
 		if (!node)
 			return -ENOSPC;
 
-		mutex_lock(&dev->struct_mutex);
+		mutex_lock(&priv->linear_lock);
 		ret = drm_mm_insert_node(&priv->linear, node, size, align,
 					 DRM_MM_SEARCH_DEFAULT);
-		mutex_unlock(&dev->struct_mutex);
+		mutex_unlock(&priv->linear_lock);
 		if (ret) {
 			kfree(node);
 			return ret;
@@ -158,9 +162,9 @@ armada_gem_linear_back(struct drm_device *dev, struct armada_gem_object *obj)
 		/* Ensure that the memory we're returning is cleared. */
 		ptr = ioremap_wc(obj->linear->start, size);
 		if (!ptr) {
-			mutex_lock(&dev->struct_mutex);
+			mutex_lock(&priv->linear_lock);
 			drm_mm_remove_node(obj->linear);
-			mutex_unlock(&dev->struct_mutex);
+			mutex_unlock(&priv->linear_lock);
 			kfree(obj->linear);
 			obj->linear = NULL;
 			return -ENOMEM;
-- 
2.5.1

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

Re: [PATCH 12/29] drm/armada: Drop struct_mutex from cursor paths

[Russell King - ARM Linux]

On Mon, Nov 23, 2015 at 10:32:45AM +0100, Daniel Vetter wrote:
> @@ -957,11 +955,9 @@ static int armada_drm_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>  	if (!dcrtc->variant->has_spu_adv_reg)
>  		return -EFAULT;
>  
> -	mutex_lock(&dev->struct_mutex);
>  	dcrtc->cursor_x = x;
>  	dcrtc->cursor_y = y;
>  	ret = armada_drm_crtc_cursor_update(dcrtc, false);
> -	mutex_unlock(&dev->struct_mutex);
>  
>  	return ret;
>  }

drivers/gpu/drm/armada/armada_crtc.c: In function 'armada_drm_crtc_cursor_move':
drivers/gpu/drm/armada/armada_crtc.c:916:21: warning: unused variable 'dev' [-Wunused-variable]

Shall I add to your patch a change to remove that variable too? :)

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

Re: [PATCH 12/29] drm/armada: Drop struct_mutex from cursor paths

[Daniel Vetter]

On Thu, Dec 03, 2015 at 04:08:45PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 23, 2015 at 10:32:45AM +0100, Daniel Vetter wrote:
> > @@ -957,11 +955,9 @@ static int armada_drm_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
> >  	if (!dcrtc->variant->has_spu_adv_reg)
> >  		return -EFAULT;
> >  
> > -	mutex_lock(&dev->struct_mutex);
> >  	dcrtc->cursor_x = x;
> >  	dcrtc->cursor_y = y;
> >  	ret = armada_drm_crtc_cursor_update(dcrtc, false);
> > -	mutex_unlock(&dev->struct_mutex);
> >  
> >  	return ret;
> >  }
> 
> drivers/gpu/drm/armada/armada_crtc.c: In function 'armada_drm_crtc_cursor_move':
> drivers/gpu/drm/armada/armada_crtc.c:916:21: warning: unused variable 'dev' [-Wunused-variable]
> 
> Shall I add to your patch a change to remove that variable too? :)

Yes please ;-) I guess I should stop relying on 0-day for arm builds and
set up a proper arm toolchain again (the old one went to waste a bit).

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

Re: [PATCH 00/29] dev->struct_mutex crusade, once more

[Daniel Vetter]

On Mon, Nov 23, 2015 at 10:32:33AM +0100, Daniel Vetter wrote:
> Hi all,
> 
> Since Daniel Stone pointed out in the last round that I fumbled one locked vs.
> unlocked case I audited all the drivers once more and uprooted a bunch more
> offenders. They're mostly in error paths, but in case anyone wants to pick them
> up I put them at the beginning of the patch series.
> 
> Again review, comments and acks highly welcome. I'd like to get this all in for
> 4.5 if possible, and will send out a pull request to Dave with the leftovers.

Ping for reviews/acks/merge confirmations please.
-Daniel

> 
> Cheers, Daniel
> 
> Daniel Vetter (29):
>   drm/armada: Use unlocked gem unreferencing
>   drm/nouveau: Use unlocked gem unreferencing
>   drm/omapdrm: Use unlocked gem unreferencing
>   drm/amdgpu: Use unlocked gem unreferencing
>   drm/radeon: Use unlocked gem unreferencing
>   drm/qxl: Use unlocked gem unreferencing
>   drm/tegra: Use unlocked gem unreferencing
>   drm/msm: Use unlocked gem unreferencing
>   drm/udl: Use unlocked gem unreferencing
>   drm/armada: Plug leak in dumb_map_offset
>   drm/armada: Don't grab dev->struct_mutex for in mmap offset ioctl
>   drm/armada: Drop struct_mutex from cursor paths
>   drm/armada: Use a private mutex to protect priv->linear
>   drm/tegra: don't take dev->struct_mutex in mmap offset ioctl
>   drm/tegra: Use drm_gem_object_unreference_unlocked
>   drm/gma500: Use correct unref in the gem bo create function
>   drm/gma500: Drop dev->struct_mutex from modeset code
>   drm/gma500: Drop dev->struct_mutex from fbdev init/teardown code
>   drm/gma500: Drop dev->struct_mutex from mmap offset function
>   drm/gma500: Add driver private mutex for the fault handler
>   drm/nouveau: Drop dev->struct_mutex from fbdev init
>   drm/exynos: Drop dev->struct_mutex from mmap offset function
>   drm/exynos: drop struct_mutex from exynos_gem_map_sgt_with_dma
>   drm/exynos: drop struct_mutex from exynos_drm_gem_get_ioctl
>   drm/exynos: drop struct_mutex from fbdev setup
>   drm/vgem: Simplify dum_map
>   drm/vgem: Move get_pages to gem_create
>   drm/vgem: Drop dev->struct_mutex
>   drm/vma_manage: Drop has_offset
> 
>  drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c    |  2 +-
>  drivers/gpu/drm/armada/armada_crtc.c      |  8 ++-----
>  drivers/gpu/drm/armada/armada_debugfs.c   |  4 ++--
>  drivers/gpu/drm/armada/armada_drm.h       |  3 ++-
>  drivers/gpu/drm/armada/armada_drv.c       |  1 +
>  drivers/gpu/drm/armada/armada_gem.c       | 25 ++++++++++-----------
>  drivers/gpu/drm/drm_gem.c                 | 17 +++++++++++++++
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 22 +++++++------------
>  drivers/gpu/drm/exynos/exynos_drm_gem.c   | 19 +++-------------
>  drivers/gpu/drm/gma500/framebuffer.c      | 12 ++---------
>  drivers/gpu/drm/gma500/gem.c              | 19 ++++++----------
>  drivers/gpu/drm/gma500/gma_display.c      | 13 +++--------
>  drivers/gpu/drm/gma500/gtt.c              |  1 +
>  drivers/gpu/drm/gma500/psb_drv.h          |  2 ++
>  drivers/gpu/drm/i915/i915_gem.c           |  3 ---
>  drivers/gpu/drm/msm/msm_fbdev.c           |  2 +-
>  drivers/gpu/drm/nouveau/nouveau_display.c |  2 +-
>  drivers/gpu/drm/nouveau/nouveau_fbcon.c   |  5 -----
>  drivers/gpu/drm/omapdrm/omap_fbdev.c      |  2 +-
>  drivers/gpu/drm/qxl/qxl_fb.c              |  4 ++--
>  drivers/gpu/drm/radeon/radeon_fb.c        |  2 +-
>  drivers/gpu/drm/tegra/drm.c               | 14 ++++++------
>  drivers/gpu/drm/tegra/gem.c               | 13 ++---------
>  drivers/gpu/drm/udl/udl_fb.c              |  2 +-
>  drivers/gpu/drm/udl/udl_gem.c             |  2 +-
>  drivers/gpu/drm/vgem/vgem_drv.c           | 36 ++++++++++---------------------
>  include/drm/drm_vma_manager.h             | 15 +------------
>  27 files changed, 89 insertions(+), 161 deletions(-)
> 
> -- 
> 2.5.1
> 

-- 
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 27/29] drm/vgem: Move get_pages to gem_create

[poma]

On 23.11.2015 10:33, Daniel Vetter wrote:
> vgem doesn't have a shrinker or anything like that and drops backing
> storage only at object_free time. There's no use in trying to be
> clever and allocating backing storage delayed, it only causes trouble
> by requiring locking.
> 
> Instead grab pages when we allocate the object right away.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/vgem/vgem_drv.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> index 63026d4324ad..1a609347236b 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.c
> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> @@ -154,6 +154,10 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
>  	if (err)
>  		goto out;
>  
> +	ret = vgem_gem_get_pages(to_vgem_bo(obj));
> +	if (ret)
> +		goto out;
> +
>  	err = drm_gem_handle_create(file, gem_object, handle);
>  	if (err)
>  		goto handle_out;
> @@ -216,16 +220,8 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev,
>  
>  	obj->filp->private_data = obj;
>  
> -	ret = vgem_gem_get_pages(to_vgem_bo(obj));
> -	if (ret)
> -		goto fail_get_pages;
> -
>  	*offset = drm_vma_node_offset_addr(&obj->vma_node);
>  
> -	goto unref;
> -
> -fail_get_pages:
> -	drm_gem_free_mmap_offset(obj);
>  unref:
>  	drm_gem_object_unreference(obj);
>  unlock:
> 


drivers/gpu/drm/vgem/vgem_drv.c: In function 'vgem_gem_fault':
drivers/gpu/drm/vgem/vgem_drv.c:92:21: warning: unused variable 'dev' [-Wunused-variable]
  struct drm_device *dev = obj->base.dev;
                     ^
drivers/gpu/drm/vgem/vgem_drv.c: In function 'vgem_gem_create':
drivers/gpu/drm/vgem/vgem_drv.c:153:2: error: 'ret' undeclared (first use in this function)
  ret = vgem_gem_get_pages(to_vgem_bo(obj));
  ^
drivers/gpu/drm/vgem/vgem_drv.c:153:2: note: each undeclared identifier is reported only once for each function it appears in
In file included from include/linux/list.h:8:0,
                 from include/linux/module.h:9,
                 from drivers/gpu/drm/vgem/vgem_drv.c:33:
include/linux/kernel.h:813:48: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
  const typeof( ((type *)0)->member ) *__mptr = (ptr); \
                                                ^
drivers/gpu/drm/vgem/vgem_drv.h:35:23: note: in expansion of macro 'container_of'
 #define to_vgem_bo(x) container_of(x, struct drm_vgem_gem_object, base)
                       ^
drivers/gpu/drm/vgem/vgem_drv.c:153:27: note: in expansion of macro 'to_vgem_bo'
  ret = vgem_gem_get_pages(to_vgem_bo(obj));
                           ^

This breaks the build, therefore "armada" batch cannot be tested entirely.


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