[PATCH 00/25] dev->struct_mutex crusade (cont'd)

[PATCH 00/25] dev->struct_mutex crusade (cont'd)

[Daniel Vetter]

Hi all,

So I hoped to get through more drivers, but it's not yet all of them. Anyway,
this gets rid of dev->struct_mutex in a lot more places. Big thing is that core
gem is now completely struct_mutex free, the only reason we still lock it is
that some drivers need it in their ->free_gem_object hook. The plan for that is
to have a ->free_gem_object_unlocked hook for all the drivers which don't need
the lock at all (which are most of them really).

Of the modern drivers the big holdouts are i915, msm and omapdrm. Plus udl still
uses some struct_mutex, but it has overall totally broken locking anyway. All
other drivers are now struct_mutex free, yay!

All per-driver bits are fully independent in this series, so can be pulled into
driver trees without issues.

Cheers, Daniel

Daniel Vetter (25):
  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_reference_unlocked
  drm/vgem: Drop vgem_drm_gem_mmap
  drm/gem: Drop struct_mutex requirement from drm_gem_mmap_obj
  drm/gem: Check locking in drm_gem_object_unreference
  drm/gem: Use container_of in drm_gem_object_free
  drm/gem: Use kref_get_unless_zero for the weak mmap references
  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/radeon: Use rdev->gem.mutex to protect hyperz/cmask owners
  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

 drivers/gpu/drm/armada/armada_crtc.c        |  6 +-
 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         | 21 +++----
 drivers/gpu/drm/drm_gem.c                   | 76 ++++++++++++++-----------
 drivers/gpu/drm/drm_gem_cma_helper.c        |  2 -
 drivers/gpu/drm/drm_vma_manager.c           | 40 ++++---------
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c   | 22 +++-----
 drivers/gpu/drm/exynos/exynos_drm_gem.c     | 15 +----
 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/msm/msm_fbdev.c             |  5 --
 drivers/gpu/drm/msm/msm_gem_prime.c         |  2 -
 drivers/gpu/drm/nouveau/nouveau_fbcon.c     |  5 --
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c   |  2 -
 drivers/gpu/drm/radeon/radeon_kms.c         | 10 +++-
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c |  2 -
 drivers/gpu/drm/tegra/drm.c                 |  4 +-
 drivers/gpu/drm/tegra/gem.c                 | 13 +----
 drivers/gpu/drm/vgem/vgem_drv.c             | 87 ++++-------------------------
 include/drm/drm_gem.h                       |  2 +
 include/drm/drm_vma_manager.h               | 22 ++------
 26 files changed, 121 insertions(+), 270 deletions(-)

-- 
2.5.1

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

[PATCH 01/25] 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 60a688ef81c7..323da46c7ca8 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 02/25] 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 323da46c7ca8..59fafa468347 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 03/25] 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.

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 01ffe9bffe38..d9ad166e34d0 100644
--- a/drivers/gpu/drm/armada/armada_crtc.c
+++ b/drivers/gpu/drm/armada/armada_crtc.c
@@ -852,11 +852,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;
@@ -866,7 +865,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;
 }
@@ -881,11 +879,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 04/25] 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!

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 5f6aef0dca59..494a6e681919 100644
--- a/drivers/gpu/drm/armada/armada_drm.h
+++ b/drivers/gpu/drm/armada/armada_drm.h
@@ -73,7 +73,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 5646b54948c7..90fcec2a44e2 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -156,6 +156,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);
 
 	/* Create all LCD controllers */
 	for (n = 0; n < ARRAY_SIZE(priv->dcrtc); n++) {
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index 59fafa468347..45e12d4c1722 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 05/25] drm/tegra: don't take dev->struct_mutex in mmap offset ioctl

[Daniel Vetter]

Since David Herrmann's mmap vma manager rework we don't need to grab
dev->struct_mutex any more to prevent races when looking up the mmap
offset. Drop it and instead don't forget to use the unref_unlocked
variant (since the drm core still cares).

While at it also fix a leak when this ioctl is called on an imported
buffer.

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

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index 01e16e146bfe..827838e64d6e 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -408,12 +408,9 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
 	struct drm_gem_object *gem;
 	struct tegra_bo *bo;
 
-	mutex_lock(&drm->struct_mutex);
-
 	gem = drm_gem_object_lookup(drm, file, handle);
 	if (!gem) {
 		dev_err(drm->dev, "failed to lookup GEM object\n");
-		mutex_unlock(&drm->struct_mutex);
 		return -EINVAL;
 	}
 
@@ -423,8 +420,6 @@ int tegra_bo_dumb_map_offset(struct drm_file *file, struct drm_device *drm,
 
 	drm_gem_object_unreference(gem);
 
-	mutex_unlock(&drm->struct_mutex);
-
 	return 0;
 }
 
-- 
2.5.1

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

[PATCH 06/25] drm/tegra: Use drm_gem_object_reference_unlocked

[Daniel Vetter]

This only grabs the mutex when really needed, but still has a
might-acquire lockdep check to make sure that's always possible.
With this patch tegra is officially struct_mutex free, yay!

Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/tegra/drm.c | 4 +---
 drivers/gpu/drm/tegra/gem.c | 8 ++------
 2 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 159ef515cab1..5de9a4ead463 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 827838e64d6e..a946259495e1 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -30,9 +30,7 @@ static void tegra_bo_put(struct host1x_bo *bo)
 	struct tegra_bo *obj = host1x_to_tegra_bo(bo);
 	struct drm_device *drm = obj->gem.dev;
 
-	mutex_lock(&drm->struct_mutex);
-	drm_gem_object_unreference(&obj->gem);
-	mutex_unlock(&drm->struct_mutex);
+	drm_gem_object_unreference_unlocked(&obj->gem);
 }
 
 static dma_addr_t tegra_bo_pin(struct host1x_bo *bo, struct sg_table **sgt)
@@ -74,9 +72,7 @@ static struct host1x_bo *tegra_bo_get(struct host1x_bo *bo)
 	struct tegra_bo *obj = host1x_to_tegra_bo(bo);
 	struct drm_device *drm = obj->gem.dev;
 
-	mutex_lock(&drm->struct_mutex);
-	drm_gem_object_reference(&obj->gem);
-	mutex_unlock(&drm->struct_mutex);
+	drm_gem_object_reference_unlocked(&obj->gem);
 
 	return bo;
 }
-- 
2.5.1

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

[PATCH 07/25] drm/vgem: Drop vgem_drm_gem_mmap

[Daniel Vetter]

It's duplicating (without using some of the helpers) drm_gem_mmap with
the addition that it can redirect to drm-buf mmap support. But prime
import/export was dropped in

commit 990ed2720717173bbdea4cfb2bad37cc7aa91495
Author: Rob Clark <robdclark@gmail.com>
Date:   Thu May 21 11:58:30 2015 -0400

    drm/vgem: drop DRIVER_PRIME (v2)

for now, so this is dead code. And since I want to rework the locking
for drm_gem_mmap it seems simpler to de-dupe this code for now and
then start over with the reworked one again, if we want to resurrect
this all indeed.

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

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 27c2c473d9d3..02ae60c395b7 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -235,66 +235,13 @@ unlock:
 	return ret;
 }
 
-int vgem_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
-{
-	struct drm_file *priv = filp->private_data;
-	struct drm_device *dev = priv->minor->dev;
-	struct drm_vma_offset_node *node;
-	struct drm_gem_object *obj;
-	struct drm_vgem_gem_object *vgem_obj;
-	int ret = 0;
-
-	mutex_lock(&dev->struct_mutex);
-
-	node = drm_vma_offset_exact_lookup(dev->vma_offset_manager,
-					   vma->vm_pgoff,
-					   vma_pages(vma));
-	if (!node) {
-		ret = -EINVAL;
-		goto out_unlock;
-	} else if (!drm_vma_node_is_allowed(node, filp)) {
-		ret = -EACCES;
-		goto out_unlock;
-	}
-
-	obj = container_of(node, struct drm_gem_object, vma_node);
-
-	vgem_obj = to_vgem_bo(obj);
-
-	if (obj->dma_buf && vgem_obj->use_dma_buf) {
-		ret = dma_buf_mmap(obj->dma_buf, vma, 0);
-		goto out_unlock;
-	}
-
-	if (!obj->dev->driver->gem_vm_ops) {
-		ret = -EINVAL;
-		goto out_unlock;
-	}
-
-	vma->vm_flags |= VM_IO | VM_MIXEDMAP | VM_DONTEXPAND | VM_DONTDUMP;
-	vma->vm_ops = obj->dev->driver->gem_vm_ops;
-	vma->vm_private_data = vgem_obj;
-	vma->vm_page_prot =
-		pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
-
-	mutex_unlock(&dev->struct_mutex);
-	drm_gem_vm_open(vma);
-	return ret;
-
-out_unlock:
-	mutex_unlock(&dev->struct_mutex);
-
-	return ret;
-}
-
-
 static struct drm_ioctl_desc vgem_ioctls[] = {
 };
 
 static const struct file_operations vgem_driver_fops = {
 	.owner		= THIS_MODULE,
 	.open		= drm_open,
-	.mmap		= vgem_drm_gem_mmap,
+	.mmap		= drm_gem_mmap,
 	.poll		= drm_poll,
 	.read		= drm_read,
 	.unlocked_ioctl = drm_ioctl,
-- 
2.5.1

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

[PATCH 08/25] drm/gem: Drop struct_mutex requirement from drm_gem_mmap_obj

[Daniel Vetter]

Since

commit 131e663bd6f1055caaff128f9aa5071d227eeb72
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jul 9 23:32:33 2015 +0200

    drm/gem: rip out drm vma accounting for gem mmaps

there is no need for this any more.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_gem.c                   | 4 ----
 drivers/gpu/drm/drm_gem_cma_helper.c        | 2 --
 drivers/gpu/drm/msm/msm_fbdev.c             | 5 -----
 drivers/gpu/drm/msm/msm_gem_prime.c         | 2 --
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c   | 2 --
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 2 --
 6 files changed, 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 3c2d4abd71c5..7dc4a8a066a3 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -810,8 +810,6 @@ EXPORT_SYMBOL(drm_gem_vm_close);
  * drm_gem_mmap() prevents unprivileged users from mapping random objects. So
  * callers must verify access restrictions before calling this helper.
  *
- * NOTE: This function has to be protected with dev->struct_mutex
- *
  * Return 0 or success or -EINVAL if the object size is smaller than the VMA
  * size, or if no gem_vm_ops are provided.
  */
@@ -820,8 +818,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 {
 	struct drm_device *dev = obj->dev;
 
-	lockdep_assert_held(&dev->struct_mutex);
-
 	/* Check for valid size. */
 	if (obj_size < vma->vm_end - vma->vm_start)
 		return -EINVAL;
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 86cc793cdf79..4fb4c45d039f 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -484,9 +484,7 @@ int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
 	struct drm_device *dev = obj->dev;
 	int ret;
 
-	mutex_lock(&dev->struct_mutex);
 	ret = drm_gem_mmap_obj(obj, obj->size, vma);
-	mutex_unlock(&dev->struct_mutex);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index f97a1964ef39..3f6ec077b51d 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -68,12 +68,7 @@ static int msm_fbdev_mmap(struct fb_info *info, struct vm_area_struct *vma)
 	if (drm_device_is_unplugged(dev))
 		return -ENODEV;
 
-	mutex_lock(&dev->struct_mutex);
-
 	ret = drm_gem_mmap_obj(drm_obj, drm_obj->size, vma);
-
-	mutex_unlock(&dev->struct_mutex);
-
 	if (ret) {
 		pr_err("%s:drm_gem_mmap_obj fail\n", __func__);
 		return ret;
diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
index 831461bc98a5..121975b07cd4 100644
--- a/drivers/gpu/drm/msm/msm_gem_prime.c
+++ b/drivers/gpu/drm/msm/msm_gem_prime.c
@@ -45,9 +45,7 @@ int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 {
 	int ret;
 
-	mutex_lock(&obj->dev->struct_mutex);
 	ret = drm_gem_mmap_obj(obj, obj->size, vma);
-	mutex_unlock(&obj->dev->struct_mutex);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
index 0cc71c9d08d5..04da8c2ff9fe 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
@@ -146,9 +146,7 @@ static int omap_gem_dmabuf_mmap(struct dma_buf *buffer,
 	if (WARN_ON(!obj->filp))
 		return -EINVAL;
 
-	mutex_lock(&dev->struct_mutex);
 	ret = drm_gem_mmap_obj(obj, omap_gem_mmap_size(obj), vma);
-	mutex_unlock(&dev->struct_mutex);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
index a6d9104f7f15..dc6d16c22db5 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
@@ -82,9 +82,7 @@ int rockchip_gem_mmap_buf(struct drm_gem_object *obj,
 	struct drm_device *drm = obj->dev;
 	int ret;
 
-	mutex_lock(&drm->struct_mutex);
 	ret = drm_gem_mmap_obj(obj, obj->size, vma);
-	mutex_unlock(&drm->struct_mutex);
 	if (ret)
 		return ret;
 
-- 
2.5.1

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

[PATCH 09/25] drm/gem: Check locking in drm_gem_object_unreference

[Daniel Vetter]

Pretty soon only some drivers will need dev->struct_mutex in their
gem_free_object callbacks. Hence it's really important to make sure
everything still keeps getting this right.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 include/drm/drm_gem.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 7a592d7e398b..5b3754864fb0 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -142,6 +142,8 @@ drm_gem_object_reference(struct drm_gem_object *obj)
 static inline void
 drm_gem_object_unreference(struct drm_gem_object *obj)
 {
+	WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
+
 	if (obj != NULL)
 		kref_put(&obj->refcount, drm_gem_object_free);
 }
-- 
2.5.1

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

[PATCH 10/25] drm/gem: Use container_of in drm_gem_object_free

[Daniel Vetter]

Just a random thing I spotted while reading code - better safe than
sorry.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_gem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 7dc4a8a066a3..ab8ea42264f4 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -763,7 +763,8 @@ EXPORT_SYMBOL(drm_gem_object_release);
 void
 drm_gem_object_free(struct kref *kref)
 {
-	struct drm_gem_object *obj = (struct drm_gem_object *) kref;
+	struct drm_gem_object *obj =
+		container_of(kref, struct drm_gem_object, refcount);
 	struct drm_device *dev = obj->dev;
 
 	WARN_ON(!mutex_is_locked(&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/25] drm/gem: Use kref_get_unless_zero for the weak mmap references

[Daniel Vetter]

Compared to wrapping the final kref_put with dev->struct_mutex this
allows us to only acquire the offset manager look both in the final
cleanup and in the lookup. Which has the upside that no locks leak out
of the core abstractions.

Also, this is the final bit which required dev->struct_mutex in gem
core, now modern drivers can be completely struct_mutex free!

This needs a new drm_vma_offset_exact_lookup_locked and makes both
drm_vma_offset_exact_lookup and drm_vma_offset_lookup unused.

Also extract __drm_gem_mmap_obj which is just drm_gem_mmap_obj without
the obj_reference call to share code.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_gem.c         | 69 +++++++++++++++++++++++----------------
 drivers/gpu/drm/drm_vma_manager.c | 40 +++++++----------------
 include/drm/drm_vma_manager.h     | 22 ++++---------
 3 files changed, 58 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index ab8ea42264f4..795d64fa7cb9 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -790,6 +790,27 @@ void drm_gem_vm_close(struct vm_area_struct *vma)
 }
 EXPORT_SYMBOL(drm_gem_vm_close);
 
+static int __drm_gem_mmap_obj(struct drm_gem_object *obj,
+			      unsigned long obj_size,
+			      struct vm_area_struct *vma)
+{
+	struct drm_device *dev = obj->dev;
+
+	/* Check for valid size. */
+	if (obj_size < vma->vm_end - vma->vm_start)
+		return -EINVAL;
+
+	if (!dev->driver->gem_vm_ops)
+		return -EINVAL;
+
+	vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
+	vma->vm_ops = dev->driver->gem_vm_ops;
+	vma->vm_private_data = obj;
+	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
+
+	return 0;
+}
+
 /**
  * drm_gem_mmap_obj - memory map a GEM object
  * @obj: the GEM object to map
@@ -817,19 +838,11 @@ EXPORT_SYMBOL(drm_gem_vm_close);
 int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 		     struct vm_area_struct *vma)
 {
-	struct drm_device *dev = obj->dev;
-
-	/* Check for valid size. */
-	if (obj_size < vma->vm_end - vma->vm_start)
-		return -EINVAL;
-
-	if (!dev->driver->gem_vm_ops)
-		return -EINVAL;
+	int ret;
 
-	vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
-	vma->vm_ops = dev->driver->gem_vm_ops;
-	vma->vm_private_data = obj;
-	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
+	ret = __drm_gem_mmap_obj(obj, obj_size, vma);
+	if (ret)
+		return ret;
 
 	/* Take a ref for this mapping of the object, so that the fault
 	 * handler can dereference the mmap offset's pointer to the object.
@@ -862,31 +875,29 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 {
 	struct drm_file *priv = filp->private_data;
 	struct drm_device *dev = priv->minor->dev;
-	struct drm_gem_object *obj;
+	struct drm_gem_object *obj = NULL;
 	struct drm_vma_offset_node *node;
-	int ret;
 
 	if (drm_device_is_unplugged(dev))
 		return -ENODEV;
 
-	mutex_lock(&dev->struct_mutex);
+	drm_vma_offset_lock_lookup(dev->vma_offset_manager);
+	node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
+						  vma->vm_pgoff,
+						  vma_pages(vma));
+	if (likely(node)) {
+		obj = container_of(node, struct drm_gem_object, vma_node);
+		if (!kref_get_unless_zero(&obj->refcount))
+			obj = NULL;
+	}
+	drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
 
-	node = drm_vma_offset_exact_lookup(dev->vma_offset_manager,
-					   vma->vm_pgoff,
-					   vma_pages(vma));
-	if (!node) {
-		mutex_unlock(&dev->struct_mutex);
+	if (!obj)
 		return -EINVAL;
-	} else if (!drm_vma_node_is_allowed(node, filp)) {
-		mutex_unlock(&dev->struct_mutex);
+	else if (!drm_vma_node_is_allowed(node, filp))
 		return -EACCES;
-	}
-
-	obj = container_of(node, struct drm_gem_object, vma_node);
-	ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT, vma);
 
-	mutex_unlock(&dev->struct_mutex);
-
-	return ret;
+	return __drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT,
+				  vma);
 }
 EXPORT_SYMBOL(drm_gem_mmap);
diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
index 68c1f32fb086..2f2ecde8285b 100644
--- a/drivers/gpu/drm/drm_vma_manager.c
+++ b/drivers/gpu/drm/drm_vma_manager.c
@@ -112,7 +112,7 @@ void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr)
 EXPORT_SYMBOL(drm_vma_offset_manager_destroy);
 
 /**
- * drm_vma_offset_lookup() - Find node in offset space
+ * drm_vma_offset_lookup_locked() - Find node in offset space
  * @mgr: Manager object
  * @start: Start address for object (page-based)
  * @pages: Size of object (page-based)
@@ -122,37 +122,21 @@ EXPORT_SYMBOL(drm_vma_offset_manager_destroy);
  * region and the given node will be returned, as long as the node spans the
  * whole requested area (given the size in number of pages as @pages).
  *
- * RETURNS:
- * Returns NULL if no suitable node can be found. Otherwise, the best match
- * is returned. It's the caller's responsibility to make sure the node doesn't
- * get destroyed before the caller can access it.
- */
-struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
-						  unsigned long start,
-						  unsigned long pages)
-{
-	struct drm_vma_offset_node *node;
-
-	read_lock(&mgr->vm_lock);
-	node = drm_vma_offset_lookup_locked(mgr, start, pages);
-	read_unlock(&mgr->vm_lock);
-
-	return node;
-}
-EXPORT_SYMBOL(drm_vma_offset_lookup);
-
-/**
- * drm_vma_offset_lookup_locked() - Find node in offset space
- * @mgr: Manager object
- * @start: Start address for object (page-based)
- * @pages: Size of object (page-based)
+ * Note that before lookup the vma offset manager lookup lock must be acquired
+ * with drm_vma_offset_lock_lookup(). See there for an example. This can then be
+ * used to implement weakly referenced lookups using kref_get_unless_zero().
  *
- * Same as drm_vma_offset_lookup() but requires the caller to lock offset lookup
- * manually. See drm_vma_offset_lock_lookup() for an example.
+ * Example:
+ *     drm_vma_offset_lock_lookup(mgr);
+ *     node = drm_vma_offset_lookup_locked(mgr);
+ *     if (node)
+ *         kref_get_unless_zero(container_of(node, sth, entr));
+ *     drm_vma_offset_unlock_lookup(mgr);
  *
  * RETURNS:
  * Returns NULL if no suitable node can be found. Otherwise, the best match
- * is returned.
+ * is returned. It's the caller's responsibility to make sure the node doesn't
+ * get destroyed before the caller can access it.
  */
 struct drm_vma_offset_node *drm_vma_offset_lookup_locked(struct drm_vma_offset_manager *mgr,
 							 unsigned long start,
diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
index 74f3b38c43c1..2f63dd5e05eb 100644
--- a/include/drm/drm_vma_manager.h
+++ b/include/drm/drm_vma_manager.h
@@ -54,9 +54,6 @@ void drm_vma_offset_manager_init(struct drm_vma_offset_manager *mgr,
 				 unsigned long page_offset, unsigned long size);
 void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr);
 
-struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
-						  unsigned long start,
-						  unsigned long pages);
 struct drm_vma_offset_node *drm_vma_offset_lookup_locked(struct drm_vma_offset_manager *mgr,
 							   unsigned long start,
 							   unsigned long pages);
@@ -71,25 +68,25 @@ bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,
 			     struct file *filp);
 
 /**
- * drm_vma_offset_exact_lookup() - Look up node by exact address
+ * drm_vma_offset_exact_lookup_locked() - Look up node by exact address
  * @mgr: Manager object
  * @start: Start address (page-based, not byte-based)
  * @pages: Size of object (page-based)
  *
- * Same as drm_vma_offset_lookup() but does not allow any offset into the node.
+ * Same as drm_vma_offset_lookup_locked() but does not allow any offset into the node.
  * It only returns the exact object with the given start address.
  *
  * RETURNS:
  * Node at exact start address @start.
  */
 static inline struct drm_vma_offset_node *
-drm_vma_offset_exact_lookup(struct drm_vma_offset_manager *mgr,
-			    unsigned long start,
-			    unsigned long pages)
+drm_vma_offset_exact_lookup_locked(struct drm_vma_offset_manager *mgr,
+				   unsigned long start,
+				   unsigned long pages)
 {
 	struct drm_vma_offset_node *node;
 
-	node = drm_vma_offset_lookup(mgr, start, pages);
+	node = drm_vma_offset_lookup_locked(mgr, start, pages);
 	return (node && node->vm_node.start == start) ? node : NULL;
 }
 
@@ -108,13 +105,6 @@ drm_vma_offset_exact_lookup(struct drm_vma_offset_manager *mgr,
  * not call any other VMA helpers while holding this lock.
  *
  * Note: You're in atomic-context while holding this lock!
- *
- * Example:
- *     drm_vma_offset_lock_lookup(mgr);
- *     node = drm_vma_offset_lookup_locked(mgr);
- *     if (node)
- *         kref_get_unless_zero(container_of(node, sth, entr));
- *     drm_vma_offset_unlock_lookup(mgr);
  */
 static inline void drm_vma_offset_lock_lookup(struct drm_vma_offset_manager *mgr)
 {
-- 
2.5.1

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

[PATCH 12/25] 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.

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 13/25] 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.

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

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

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

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 2eaf1b31c7bd..c7904fc3d33b 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 15/25] 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.

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 16/25] 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!

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 17/25] 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!

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

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

[PATCH 18/25] drm/radeon: Use rdev->gem.mutex to protect hyperz/cmask owners

[Daniel Vetter]

This removes the last depency of radeon for dev->struct_mutex!

Also the locking scheme for hyperz/cmask owners seems a bit unsound,
there's no protection in the preclose handler (and that never did hold
dev->struct_mutex while being called). So grab the same lock there,
too.

There's also all the checks in the cs checker, but since the overall
design seems to never stall for the previous owner I figured it's ok
if I leave this racy. It was racy even before I touched it after all
too.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/radeon/radeon_kms.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index efa45e502975..893cf1079552 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -181,7 +181,9 @@ static void radeon_set_filp_rights(struct drm_device *dev,
 				   struct drm_file *applier,
 				   uint32_t *value)
 {
-	mutex_lock(&dev->struct_mutex);
+	struct radeon_device *rdev = dev->dev_private;
+
+	mutex_lock(&rdev->gem.mutex);
 	if (*value == 1) {
 		/* wants rights */
 		if (!*owner)
@@ -192,7 +194,7 @@ static void radeon_set_filp_rights(struct drm_device *dev,
 			*owner = NULL;
 	}
 	*value = *owner == applier ? 1 : 0;
-	mutex_unlock(&dev->struct_mutex);
+	mutex_unlock(&rdev->gem.mutex);
 }
 
 /*
@@ -727,10 +729,14 @@ void radeon_driver_preclose_kms(struct drm_device *dev,
 				struct drm_file *file_priv)
 {
 	struct radeon_device *rdev = dev->dev_private;
+
+	mutex_lock(&rdev->gem.mutex);
 	if (rdev->hyperz_filp == file_priv)
 		rdev->hyperz_filp = NULL;
 	if (rdev->cmask_filp == file_priv)
 		rdev->cmask_filp = NULL;
+	mutex_unlock(&rdev->gem.mutex);
+
 	radeon_uvd_free_handles(rdev, file_priv);
 	radeon_vce_free_handles(rdev, file_priv);
 }
-- 
2.5.1

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

[PATCH 19/25] 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.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 407afedb6003..f6c7bc3a7e68 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -437,8 +437,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
@@ -448,16 +446,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);
 	return ret;
 }
 
-- 
2.5.1

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

[PATCH 20/25] 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.

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 f6c7bc3a7e68..cf34c7f9ed93 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -367,16 +367,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

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

[PATCH 21/25] 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.

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 cf34c7f9ed93..cb25125d9110 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -341,12 +341,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;
 	}
 
@@ -356,7 +353,6 @@ int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data,
 	args->size = exynos_gem_obj->size;
 
 	drm_gem_object_unreference(obj);
-	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
 }
-- 
2.5.1

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

[PATCH 22/25] 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!

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 a221f753ad9c..4e2166d4a964 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -137,8 +137,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;
 
 	obj = exynos_drm_gem_create(dev, EXYNOS_BO_CONTIG, size);
@@ -152,10 +150,8 @@ static int exynos_drm_fbdev_create(struct drm_fb_helper *helper,
 		obj = exynos_drm_gem_create(dev, EXYNOS_BO_NONCONTIG, size);
 	}
 
-	if (IS_ERR(obj)) {
-		ret = PTR_ERR(obj);
-		goto out;
-	}
+	if (IS_ERR(obj))
+		return PTR_ERR(obj);
 
 	exynos_fbdev->obj = obj;
 
@@ -170,7 +166,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:
@@ -178,13 +173,12 @@ err_destroy_framebuffer:
 err_destroy_gem:
 	exynos_drm_gem_destroy(obj);
 
-/*
- * 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 23/25] 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

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

[PATCH 24/25] 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

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

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

[Daniel Vetter]

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

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

diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 1a609347236b..b525208375aa 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)
@@ -224,8 +217,7 @@ int vgem_gem_dumb_map(struct drm_file *file, struct drm_device *dev,
 
 unref:
 	drm_gem_object_unreference(obj);
-unlock:
-	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

Re: [PATCH 18/25] drm/radeon: Use rdev->gem.mutex to protect hyperz/cmask owners

[Christian König]

On 15.10.2015 09:36, Daniel Vetter wrote:
> This removes the last depency of radeon for dev->struct_mutex!
>
> Also the locking scheme for hyperz/cmask owners seems a bit unsound,
> there's no protection in the preclose handler (and that never did hold
> dev->struct_mutex while being called). So grab the same lock there,
> too.
>
> There's also all the checks in the cs checker, but since the overall
> design seems to never stall for the previous owner I figured it's ok
> if I leave this racy. It was racy even before I touched it after all
> too.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

I'm not so deep into the pre R600 stuff but this looks completely sane.

Patch is Reviewed-by: Christian König <christian.koenig@amd.com>

Regards,
Christian.

> ---
>   drivers/gpu/drm/radeon/radeon_kms.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index efa45e502975..893cf1079552 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -181,7 +181,9 @@ static void radeon_set_filp_rights(struct drm_device *dev,
>   				   struct drm_file *applier,
>   				   uint32_t *value)
>   {
> -	mutex_lock(&dev->struct_mutex);
> +	struct radeon_device *rdev = dev->dev_private;
> +
> +	mutex_lock(&rdev->gem.mutex);
>   	if (*value == 1) {
>   		/* wants rights */
>   		if (!*owner)
> @@ -192,7 +194,7 @@ static void radeon_set_filp_rights(struct drm_device *dev,
>   			*owner = NULL;
>   	}
>   	*value = *owner == applier ? 1 : 0;
> -	mutex_unlock(&dev->struct_mutex);
> +	mutex_unlock(&rdev->gem.mutex);
>   }
>   
>   /*
> @@ -727,10 +729,14 @@ void radeon_driver_preclose_kms(struct drm_device *dev,
>   				struct drm_file *file_priv)
>   {
>   	struct radeon_device *rdev = dev->dev_private;
> +
> +	mutex_lock(&rdev->gem.mutex);
>   	if (rdev->hyperz_filp == file_priv)
>   		rdev->hyperz_filp = NULL;
>   	if (rdev->cmask_filp == file_priv)
>   		rdev->cmask_filp = NULL;
> +	mutex_unlock(&rdev->gem.mutex);
> +
>   	radeon_uvd_free_handles(rdev, file_priv);
>   	radeon_vce_free_handles(rdev, file_priv);
>   }

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

Re: [PATCH 08/25] drm/gem: Drop struct_mutex requirement from drm_gem_mmap_obj

[David Herrmann]

Hi

On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Since
>
> commit 131e663bd6f1055caaff128f9aa5071d227eeb72
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Jul 9 23:32:33 2015 +0200
>
>     drm/gem: rip out drm vma accounting for gem mmaps
>
> there is no need for this any more.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Looks good to me.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> ---
>  drivers/gpu/drm/drm_gem.c                   | 4 ----
>  drivers/gpu/drm/drm_gem_cma_helper.c        | 2 --
>  drivers/gpu/drm/msm/msm_fbdev.c             | 5 -----
>  drivers/gpu/drm/msm/msm_gem_prime.c         | 2 --
>  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c   | 2 --
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 2 --
>  6 files changed, 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 3c2d4abd71c5..7dc4a8a066a3 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -810,8 +810,6 @@ EXPORT_SYMBOL(drm_gem_vm_close);
>   * drm_gem_mmap() prevents unprivileged users from mapping random objects. So
>   * callers must verify access restrictions before calling this helper.
>   *
> - * NOTE: This function has to be protected with dev->struct_mutex
> - *
>   * Return 0 or success or -EINVAL if the object size is smaller than the VMA
>   * size, or if no gem_vm_ops are provided.
>   */
> @@ -820,8 +818,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  {
>         struct drm_device *dev = obj->dev;
>
> -       lockdep_assert_held(&dev->struct_mutex);
> -
>         /* Check for valid size. */
>         if (obj_size < vma->vm_end - vma->vm_start)
>                 return -EINVAL;
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 86cc793cdf79..4fb4c45d039f 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -484,9 +484,7 @@ int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
>         struct drm_device *dev = obj->dev;
>         int ret;
>
> -       mutex_lock(&dev->struct_mutex);
>         ret = drm_gem_mmap_obj(obj, obj->size, vma);
> -       mutex_unlock(&dev->struct_mutex);
>         if (ret < 0)
>                 return ret;
>
> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
> index f97a1964ef39..3f6ec077b51d 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -68,12 +68,7 @@ static int msm_fbdev_mmap(struct fb_info *info, struct vm_area_struct *vma)
>         if (drm_device_is_unplugged(dev))
>                 return -ENODEV;
>
> -       mutex_lock(&dev->struct_mutex);
> -
>         ret = drm_gem_mmap_obj(drm_obj, drm_obj->size, vma);
> -
> -       mutex_unlock(&dev->struct_mutex);
> -
>         if (ret) {
>                 pr_err("%s:drm_gem_mmap_obj fail\n", __func__);
>                 return ret;
> diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
> index 831461bc98a5..121975b07cd4 100644
> --- a/drivers/gpu/drm/msm/msm_gem_prime.c
> +++ b/drivers/gpu/drm/msm/msm_gem_prime.c
> @@ -45,9 +45,7 @@ int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  {
>         int ret;
>
> -       mutex_lock(&obj->dev->struct_mutex);
>         ret = drm_gem_mmap_obj(obj, obj->size, vma);
> -       mutex_unlock(&obj->dev->struct_mutex);
>         if (ret < 0)
>                 return ret;
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> index 0cc71c9d08d5..04da8c2ff9fe 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> @@ -146,9 +146,7 @@ static int omap_gem_dmabuf_mmap(struct dma_buf *buffer,
>         if (WARN_ON(!obj->filp))
>                 return -EINVAL;
>
> -       mutex_lock(&dev->struct_mutex);
>         ret = drm_gem_mmap_obj(obj, omap_gem_mmap_size(obj), vma);
> -       mutex_unlock(&dev->struct_mutex);
>         if (ret < 0)
>                 return ret;
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index a6d9104f7f15..dc6d16c22db5 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -82,9 +82,7 @@ int rockchip_gem_mmap_buf(struct drm_gem_object *obj,
>         struct drm_device *drm = obj->dev;
>         int ret;
>
> -       mutex_lock(&drm->struct_mutex);
>         ret = drm_gem_mmap_obj(obj, obj->size, vma);
> -       mutex_unlock(&drm->struct_mutex);
>         if (ret)
>                 return ret;
>
> --
> 2.5.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/25] drm/gem: Check locking in drm_gem_object_unreference

[David Herrmann]

Hi

On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Pretty soon only some drivers will need dev->struct_mutex in their
> gem_free_object callbacks. Hence it's really important to make sure
> everything still keeps getting this right.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  include/drm/drm_gem.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index 7a592d7e398b..5b3754864fb0 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -142,6 +142,8 @@ drm_gem_object_reference(struct drm_gem_object *obj)
>  static inline void
>  drm_gem_object_unreference(struct drm_gem_object *obj)
>  {
> +       WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
> +

This doesn't work. mutex_is_locked() is not context-aware, so it does
not check whether the holder is the current thread or someone else.
You *have* to use lockdep if you want negative lock checks.

In other words, if some other thread holds the mutex in parallel to
this being called, you will trigger the WARN_ON.

Thanks
David

>         if (obj != NULL)
>                 kref_put(&obj->refcount, drm_gem_object_free);
>  }
> --
> 2.5.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/25] drm/gem: Use container_of in drm_gem_object_free

[David Herrmann]

Hi

On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Just a random thing I spotted while reading code - better safe than
> sorry.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> ---
>  drivers/gpu/drm/drm_gem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 7dc4a8a066a3..ab8ea42264f4 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -763,7 +763,8 @@ EXPORT_SYMBOL(drm_gem_object_release);
>  void
>  drm_gem_object_free(struct kref *kref)
>  {
> -       struct drm_gem_object *obj = (struct drm_gem_object *) kref;
> +       struct drm_gem_object *obj =
> +               container_of(kref, struct drm_gem_object, refcount);
>         struct drm_device *dev = obj->dev;
>
>         WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> --
> 2.5.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/25] drm/gem: Use kref_get_unless_zero for the weak mmap references

[David Herrmann]

Hi

On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Compared to wrapping the final kref_put with dev->struct_mutex this
> allows us to only acquire the offset manager look both in the final
> cleanup and in the lookup. Which has the upside that no locks leak out
> of the core abstractions.
>
> Also, this is the final bit which required dev->struct_mutex in gem
> core, now modern drivers can be completely struct_mutex free!
>
> This needs a new drm_vma_offset_exact_lookup_locked and makes both
> drm_vma_offset_exact_lookup and drm_vma_offset_lookup unused.
>
> Also extract __drm_gem_mmap_obj which is just drm_gem_mmap_obj without
> the obj_reference call to share code.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_gem.c         | 69 +++++++++++++++++++++++----------------
>  drivers/gpu/drm/drm_vma_manager.c | 40 +++++++----------------
>  include/drm/drm_vma_manager.h     | 22 ++++---------
>  3 files changed, 58 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index ab8ea42264f4..795d64fa7cb9 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -790,6 +790,27 @@ void drm_gem_vm_close(struct vm_area_struct *vma)
>  }
>  EXPORT_SYMBOL(drm_gem_vm_close);
>
> +static int __drm_gem_mmap_obj(struct drm_gem_object *obj,
> +                             unsigned long obj_size,
> +                             struct vm_area_struct *vma)
> +{
> +       struct drm_device *dev = obj->dev;
> +
> +       /* Check for valid size. */
> +       if (obj_size < vma->vm_end - vma->vm_start)
> +               return -EINVAL;
> +
> +       if (!dev->driver->gem_vm_ops)
> +               return -EINVAL;
> +
> +       vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> +       vma->vm_ops = dev->driver->gem_vm_ops;
> +       vma->vm_private_data = obj;

Slightly ugly that this function *consumes* the rec-count of 'obj',
but only in the success case. Which, btw., makes your function below
leak the ref-count.

I'd prefer if you move the assignment of "vm_private_data" into the
caller, which makes the ref-counting obvious.

> +       vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> +
> +       return 0;
> +}
> +
>  /**
>   * drm_gem_mmap_obj - memory map a GEM object
>   * @obj: the GEM object to map
> @@ -817,19 +838,11 @@ EXPORT_SYMBOL(drm_gem_vm_close);
>  int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>                      struct vm_area_struct *vma)
>  {
> -       struct drm_device *dev = obj->dev;
> -
> -       /* Check for valid size. */
> -       if (obj_size < vma->vm_end - vma->vm_start)
> -               return -EINVAL;
> -
> -       if (!dev->driver->gem_vm_ops)
> -               return -EINVAL;
> +       int ret;
>
> -       vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
> -       vma->vm_ops = dev->driver->gem_vm_ops;
> -       vma->vm_private_data = obj;
> -       vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> +       ret = __drm_gem_mmap_obj(obj, obj_size, vma);
> +       if (ret)
> +               return ret;
>
>         /* Take a ref for this mapping of the object, so that the fault
>          * handler can dereference the mmap offset's pointer to the object.
> @@ -862,31 +875,29 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>  {
>         struct drm_file *priv = filp->private_data;
>         struct drm_device *dev = priv->minor->dev;
> -       struct drm_gem_object *obj;
> +       struct drm_gem_object *obj = NULL;
>         struct drm_vma_offset_node *node;
> -       int ret;
>
>         if (drm_device_is_unplugged(dev))
>                 return -ENODEV;
>
> -       mutex_lock(&dev->struct_mutex);
> +       drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> +       node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> +                                                 vma->vm_pgoff,
> +                                                 vma_pages(vma));
> +       if (likely(node)) {
> +               obj = container_of(node, struct drm_gem_object, vma_node);
> +               if (!kref_get_unless_zero(&obj->refcount))
> +                       obj = NULL;
> +       }
> +       drm_vma_offset_unlock_lookup(dev->vma_offset_manager);

Looks good!

>
> -       node = drm_vma_offset_exact_lookup(dev->vma_offset_manager,
> -                                          vma->vm_pgoff,
> -                                          vma_pages(vma));
> -       if (!node) {
> -               mutex_unlock(&dev->struct_mutex);
> +       if (!obj)
>                 return -EINVAL;
> -       } else if (!drm_vma_node_is_allowed(node, filp)) {
> -               mutex_unlock(&dev->struct_mutex);
> +       else if (!drm_vma_node_is_allowed(node, filp))
>                 return -EACCES;

You need to drop the 'obj' reference here.

> -       }
> -
> -       obj = container_of(node, struct drm_gem_object, vma_node);
> -       ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT, vma);
>
> -       mutex_unlock(&dev->struct_mutex);
> -
> -       return ret;
> +       return __drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT,
> +                                 vma);

If this fails, you need to drop the 'obj' reference here.

Thanks
David

>  }
>  EXPORT_SYMBOL(drm_gem_mmap);
> diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
> index 68c1f32fb086..2f2ecde8285b 100644
> --- a/drivers/gpu/drm/drm_vma_manager.c
> +++ b/drivers/gpu/drm/drm_vma_manager.c
> @@ -112,7 +112,7 @@ void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr)
>  EXPORT_SYMBOL(drm_vma_offset_manager_destroy);
>
>  /**
> - * drm_vma_offset_lookup() - Find node in offset space
> + * drm_vma_offset_lookup_locked() - Find node in offset space
>   * @mgr: Manager object
>   * @start: Start address for object (page-based)
>   * @pages: Size of object (page-based)
> @@ -122,37 +122,21 @@ EXPORT_SYMBOL(drm_vma_offset_manager_destroy);
>   * region and the given node will be returned, as long as the node spans the
>   * whole requested area (given the size in number of pages as @pages).
>   *
> - * RETURNS:
> - * Returns NULL if no suitable node can be found. Otherwise, the best match
> - * is returned. It's the caller's responsibility to make sure the node doesn't
> - * get destroyed before the caller can access it.
> - */
> -struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
> -                                                 unsigned long start,
> -                                                 unsigned long pages)
> -{
> -       struct drm_vma_offset_node *node;
> -
> -       read_lock(&mgr->vm_lock);
> -       node = drm_vma_offset_lookup_locked(mgr, start, pages);
> -       read_unlock(&mgr->vm_lock);
> -
> -       return node;
> -}
> -EXPORT_SYMBOL(drm_vma_offset_lookup);
> -
> -/**
> - * drm_vma_offset_lookup_locked() - Find node in offset space
> - * @mgr: Manager object
> - * @start: Start address for object (page-based)
> - * @pages: Size of object (page-based)
> + * Note that before lookup the vma offset manager lookup lock must be acquired
> + * with drm_vma_offset_lock_lookup(). See there for an example. This can then be
> + * used to implement weakly referenced lookups using kref_get_unless_zero().
>   *
> - * Same as drm_vma_offset_lookup() but requires the caller to lock offset lookup
> - * manually. See drm_vma_offset_lock_lookup() for an example.
> + * Example:
> + *     drm_vma_offset_lock_lookup(mgr);
> + *     node = drm_vma_offset_lookup_locked(mgr);
> + *     if (node)
> + *         kref_get_unless_zero(container_of(node, sth, entr));
> + *     drm_vma_offset_unlock_lookup(mgr);
>   *
>   * RETURNS:
>   * Returns NULL if no suitable node can be found. Otherwise, the best match
> - * is returned.
> + * is returned. It's the caller's responsibility to make sure the node doesn't
> + * get destroyed before the caller can access it.
>   */
>  struct drm_vma_offset_node *drm_vma_offset_lookup_locked(struct drm_vma_offset_manager *mgr,
>                                                          unsigned long start,
> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
> index 74f3b38c43c1..2f63dd5e05eb 100644
> --- a/include/drm/drm_vma_manager.h
> +++ b/include/drm/drm_vma_manager.h
> @@ -54,9 +54,6 @@ void drm_vma_offset_manager_init(struct drm_vma_offset_manager *mgr,
>                                  unsigned long page_offset, unsigned long size);
>  void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr);
>
> -struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
> -                                                 unsigned long start,
> -                                                 unsigned long pages);
>  struct drm_vma_offset_node *drm_vma_offset_lookup_locked(struct drm_vma_offset_manager *mgr,
>                                                            unsigned long start,
>                                                            unsigned long pages);
> @@ -71,25 +68,25 @@ bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,
>                              struct file *filp);
>
>  /**
> - * drm_vma_offset_exact_lookup() - Look up node by exact address
> + * drm_vma_offset_exact_lookup_locked() - Look up node by exact address
>   * @mgr: Manager object
>   * @start: Start address (page-based, not byte-based)
>   * @pages: Size of object (page-based)
>   *
> - * Same as drm_vma_offset_lookup() but does not allow any offset into the node.
> + * Same as drm_vma_offset_lookup_locked() but does not allow any offset into the node.
>   * It only returns the exact object with the given start address.
>   *
>   * RETURNS:
>   * Node at exact start address @start.
>   */
>  static inline struct drm_vma_offset_node *
> -drm_vma_offset_exact_lookup(struct drm_vma_offset_manager *mgr,
> -                           unsigned long start,
> -                           unsigned long pages)
> +drm_vma_offset_exact_lookup_locked(struct drm_vma_offset_manager *mgr,
> +                                  unsigned long start,
> +                                  unsigned long pages)
>  {
>         struct drm_vma_offset_node *node;
>
> -       node = drm_vma_offset_lookup(mgr, start, pages);
> +       node = drm_vma_offset_lookup_locked(mgr, start, pages);
>         return (node && node->vm_node.start == start) ? node : NULL;
>  }
>
> @@ -108,13 +105,6 @@ drm_vma_offset_exact_lookup(struct drm_vma_offset_manager *mgr,
>   * not call any other VMA helpers while holding this lock.
>   *
>   * Note: You're in atomic-context while holding this lock!
> - *
> - * Example:
> - *     drm_vma_offset_lock_lookup(mgr);
> - *     node = drm_vma_offset_lookup_locked(mgr);
> - *     if (node)
> - *         kref_get_unless_zero(container_of(node, sth, entr));
> - *     drm_vma_offset_unlock_lookup(mgr);
>   */
>  static inline void drm_vma_offset_lock_lookup(struct drm_vma_offset_manager *mgr)
>  {
> --
> 2.5.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/25] drm/gem: Check locking in drm_gem_object_unreference

[Daniel Vetter]

On Thu, Oct 15, 2015 at 10:36:08AM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Pretty soon only some drivers will need dev->struct_mutex in their
> > gem_free_object callbacks. Hence it's really important to make sure
> > everything still keeps getting this right.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  include/drm/drm_gem.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> > index 7a592d7e398b..5b3754864fb0 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -142,6 +142,8 @@ drm_gem_object_reference(struct drm_gem_object *obj)
> >  static inline void
> >  drm_gem_object_unreference(struct drm_gem_object *obj)
> >  {
> > +       WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
> > +
> 
> This doesn't work. mutex_is_locked() is not context-aware, so it does
> not check whether the holder is the current thread or someone else.
> You *have* to use lockdep if you want negative lock checks.
> 
> In other words, if some other thread holds the mutex in parallel to
> this being called, you will trigger the WARN_ON.

It's the other way round: If another thread holds the lock, but the caller
doesn't, then we won't WARN: It's a false negative, not a false positive.

And since lockdep is a serious hog and no one runs it I prefer this way
round than the "perfect" lockdep_assert_hold.

Not that in the unlocked variant we can't afford this mistake, so there we
do rely upon the perfect lockdep locking tracking and use might_lock.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/25] drm/gem: Check locking in drm_gem_object_unreference

[David Herrmann]

Hi

On Thu, Oct 15, 2015 at 10:55 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Oct 15, 2015 at 10:36:08AM +0200, David Herrmann wrote:
>> Hi
>>
>> On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> > Pretty soon only some drivers will need dev->struct_mutex in their
>> > gem_free_object callbacks. Hence it's really important to make sure
>> > everything still keeps getting this right.
>> >
>> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> > ---
>> >  include/drm/drm_gem.h | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>> > index 7a592d7e398b..5b3754864fb0 100644
>> > --- a/include/drm/drm_gem.h
>> > +++ b/include/drm/drm_gem.h
>> > @@ -142,6 +142,8 @@ drm_gem_object_reference(struct drm_gem_object *obj)
>> >  static inline void
>> >  drm_gem_object_unreference(struct drm_gem_object *obj)
>> >  {
>> > +       WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex));
>> > +
>>
>> This doesn't work. mutex_is_locked() is not context-aware, so it does
>> not check whether the holder is the current thread or someone else.
>> You *have* to use lockdep if you want negative lock checks.
>>
>> In other words, if some other thread holds the mutex in parallel to
>> this being called, you will trigger the WARN_ON.
>
> It's the other way round: If another thread holds the lock, but the caller
> doesn't, then we won't WARN: It's a false negative, not a false positive.
>
> And since lockdep is a serious hog and no one runs it I prefer this way
> round than the "perfect" lockdep_assert_hold.
>
> Not that in the unlocked variant we can't afford this mistake, so there we
> do rely upon the perfect lockdep locking tracking and use might_lock.

Gnah, you're right! I read this as assert, not WARN_ON.. *sigh* Sorry
for the noise.

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

[PATCH] drm/gem: Use kref_get_unless_zero for the weak mmap references

[Daniel Vetter]

Compared to wrapping the final kref_put with dev->struct_mutex this
allows us to only acquire the offset manager look both in the final
cleanup and in the lookup. Which has the upside that no locks leak out
of the core abstractions. But it means that we need to hold a
temporary reference to the object while checking mmap constraints, to
make sure the object doesn't disappear. Extended the critical region
would have worked too, but would result in more leaky locking.

Also, this is the final bit which required dev->struct_mutex in gem
core, now modern drivers can be completely struct_mutex free!

This needs a new drm_vma_offset_exact_lookup_locked and makes both
drm_vma_offset_exact_lookup and drm_vma_offset_lookup unused.

v2: Don't leak object references in failure paths (David).

Cc: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_gem.c         | 30 +++++++++++++++++------------
 drivers/gpu/drm/drm_vma_manager.c | 40 ++++++++++++---------------------------
 include/drm/drm_vma_manager.h     | 22 ++++++---------------
 3 files changed, 36 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index ab8ea42264f4..663fadbe979e 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -862,30 +862,36 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 {
 	struct drm_file *priv = filp->private_data;
 	struct drm_device *dev = priv->minor->dev;
-	struct drm_gem_object *obj;
+	struct drm_gem_object *obj = NULL;
 	struct drm_vma_offset_node *node;
 	int ret;
 
 	if (drm_device_is_unplugged(dev))
 		return -ENODEV;
 
-	mutex_lock(&dev->struct_mutex);
+	drm_vma_offset_lock_lookup(dev->vma_offset_manager);
+	node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
+						  vma->vm_pgoff,
+						  vma_pages(vma));
+	if (likely(node)) {
+		obj = container_of(node, struct drm_gem_object, vma_node);
+		if (!kref_get_unless_zero(&obj->refcount))
+			obj = NULL;
+	}
+	drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
 
-	node = drm_vma_offset_exact_lookup(dev->vma_offset_manager,
-					   vma->vm_pgoff,
-					   vma_pages(vma));
-	if (!node) {
-		mutex_unlock(&dev->struct_mutex);
+	if (!obj)
 		return -EINVAL;
-	} else if (!drm_vma_node_is_allowed(node, filp)) {
-		mutex_unlock(&dev->struct_mutex);
+
+	if (!drm_vma_node_is_allowed(node, filp)) {
+		drm_gem_object_unreference_unlocked(obj);
 		return -EACCES;
 	}
 
-	obj = container_of(node, struct drm_gem_object, vma_node);
-	ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT, vma);
+	ret = drm_gem_mmap_obj(obj, drm_vma_node_size(node) << PAGE_SHIFT,
+			       vma);
 
-	mutex_unlock(&dev->struct_mutex);
+	drm_gem_object_unreference_unlocked(obj);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
index 68c1f32fb086..2f2ecde8285b 100644
--- a/drivers/gpu/drm/drm_vma_manager.c
+++ b/drivers/gpu/drm/drm_vma_manager.c
@@ -112,7 +112,7 @@ void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr)
 EXPORT_SYMBOL(drm_vma_offset_manager_destroy);
 
 /**
- * drm_vma_offset_lookup() - Find node in offset space
+ * drm_vma_offset_lookup_locked() - Find node in offset space
  * @mgr: Manager object
  * @start: Start address for object (page-based)
  * @pages: Size of object (page-based)
@@ -122,37 +122,21 @@ EXPORT_SYMBOL(drm_vma_offset_manager_destroy);
  * region and the given node will be returned, as long as the node spans the
  * whole requested area (given the size in number of pages as @pages).
  *
- * RETURNS:
- * Returns NULL if no suitable node can be found. Otherwise, the best match
- * is returned. It's the caller's responsibility to make sure the node doesn't
- * get destroyed before the caller can access it.
- */
-struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
-						  unsigned long start,
-						  unsigned long pages)
-{
-	struct drm_vma_offset_node *node;
-
-	read_lock(&mgr->vm_lock);
-	node = drm_vma_offset_lookup_locked(mgr, start, pages);
-	read_unlock(&mgr->vm_lock);
-
-	return node;
-}
-EXPORT_SYMBOL(drm_vma_offset_lookup);
-
-/**
- * drm_vma_offset_lookup_locked() - Find node in offset space
- * @mgr: Manager object
- * @start: Start address for object (page-based)
- * @pages: Size of object (page-based)
+ * Note that before lookup the vma offset manager lookup lock must be acquired
+ * with drm_vma_offset_lock_lookup(). See there for an example. This can then be
+ * used to implement weakly referenced lookups using kref_get_unless_zero().
  *
- * Same as drm_vma_offset_lookup() but requires the caller to lock offset lookup
- * manually. See drm_vma_offset_lock_lookup() for an example.
+ * Example:
+ *     drm_vma_offset_lock_lookup(mgr);
+ *     node = drm_vma_offset_lookup_locked(mgr);
+ *     if (node)
+ *         kref_get_unless_zero(container_of(node, sth, entr));
+ *     drm_vma_offset_unlock_lookup(mgr);
  *
  * RETURNS:
  * Returns NULL if no suitable node can be found. Otherwise, the best match
- * is returned.
+ * is returned. It's the caller's responsibility to make sure the node doesn't
+ * get destroyed before the caller can access it.
  */
 struct drm_vma_offset_node *drm_vma_offset_lookup_locked(struct drm_vma_offset_manager *mgr,
 							 unsigned long start,
diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
index 74f3b38c43c1..2f63dd5e05eb 100644
--- a/include/drm/drm_vma_manager.h
+++ b/include/drm/drm_vma_manager.h
@@ -54,9 +54,6 @@ void drm_vma_offset_manager_init(struct drm_vma_offset_manager *mgr,
 				 unsigned long page_offset, unsigned long size);
 void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr);
 
-struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
-						  unsigned long start,
-						  unsigned long pages);
 struct drm_vma_offset_node *drm_vma_offset_lookup_locked(struct drm_vma_offset_manager *mgr,
 							   unsigned long start,
 							   unsigned long pages);
@@ -71,25 +68,25 @@ bool drm_vma_node_is_allowed(struct drm_vma_offset_node *node,
 			     struct file *filp);
 
 /**
- * drm_vma_offset_exact_lookup() - Look up node by exact address
+ * drm_vma_offset_exact_lookup_locked() - Look up node by exact address
  * @mgr: Manager object
  * @start: Start address (page-based, not byte-based)
  * @pages: Size of object (page-based)
  *
- * Same as drm_vma_offset_lookup() but does not allow any offset into the node.
+ * Same as drm_vma_offset_lookup_locked() but does not allow any offset into the node.
  * It only returns the exact object with the given start address.
  *
  * RETURNS:
  * Node at exact start address @start.
  */
 static inline struct drm_vma_offset_node *
-drm_vma_offset_exact_lookup(struct drm_vma_offset_manager *mgr,
-			    unsigned long start,
-			    unsigned long pages)
+drm_vma_offset_exact_lookup_locked(struct drm_vma_offset_manager *mgr,
+				   unsigned long start,
+				   unsigned long pages)
 {
 	struct drm_vma_offset_node *node;
 
-	node = drm_vma_offset_lookup(mgr, start, pages);
+	node = drm_vma_offset_lookup_locked(mgr, start, pages);
 	return (node && node->vm_node.start == start) ? node : NULL;
 }
 
@@ -108,13 +105,6 @@ drm_vma_offset_exact_lookup(struct drm_vma_offset_manager *mgr,
  * not call any other VMA helpers while holding this lock.
  *
  * Note: You're in atomic-context while holding this lock!
- *
- * Example:
- *     drm_vma_offset_lock_lookup(mgr);
- *     node = drm_vma_offset_lookup_locked(mgr);
- *     if (node)
- *         kref_get_unless_zero(container_of(node, sth, entr));
- *     drm_vma_offset_unlock_lookup(mgr);
  */
 static inline void drm_vma_offset_lock_lookup(struct drm_vma_offset_manager *mgr)
 {
-- 
2.5.1

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

Re: [PATCH] drm/gem: Use kref_get_unless_zero for the weak mmap references

[Chris Wilson]

On Thu, Oct 15, 2015 at 11:33:43AM +0200, Daniel Vetter wrote:
> Compared to wrapping the final kref_put with dev->struct_mutex this
> allows us to only acquire the offset manager look both in the final
> cleanup and in the lookup. Which has the upside that no locks leak out
> of the core abstractions. But it means that we need to hold a
> temporary reference to the object while checking mmap constraints, to
> make sure the object doesn't disappear. Extended the critical region
> would have worked too, but would result in more leaky locking.
> 
> Also, this is the final bit which required dev->struct_mutex in gem
> core, now modern drivers can be completely struct_mutex free!
> 
> This needs a new drm_vma_offset_exact_lookup_locked and makes both
> drm_vma_offset_exact_lookup and drm_vma_offset_lookup unused.
> 
> v2: Don't leak object references in failure paths (David).
> 
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_gem.c         | 30 +++++++++++++++++------------
>  drivers/gpu/drm/drm_vma_manager.c | 40 ++++++++++++---------------------------
>  include/drm/drm_vma_manager.h     | 22 ++++++---------------
>  3 files changed, 36 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index ab8ea42264f4..663fadbe979e 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -862,30 +862,36 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>  {
>  	struct drm_file *priv = filp->private_data;
>  	struct drm_device *dev = priv->minor->dev;
> -	struct drm_gem_object *obj;
> +	struct drm_gem_object *obj = NULL;
>  	struct drm_vma_offset_node *node;
>  	int ret;
>  
>  	if (drm_device_is_unplugged(dev))
>  		return -ENODEV;
>  

/* bla bla bla
 * When the object is being freed, after it hits 0-refcnt
 * it acquires the struct_mutex and proceeds to tear down
 * the object. In the process it will attempt to remove
 * the VMA offset and so acquire this mgr->vm_lock.
 * Therefore if we find an object with a 0-refcnt that matches
 * our range, we know it is in the process of being destroyed
 * and will be freed as soon as we release the lock - so
 * we have to check for the 0-refcnted object and treat it as
 * invalid.
 */
> -	mutex_lock(&dev->struct_mutex);
> +	drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> +	node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> +						  vma->vm_pgoff,
> +						  vma_pages(vma));
> +	if (likely(node)) {
> +		obj = container_of(node, struct drm_gem_object, vma_node);
> +		if (!kref_get_unless_zero(&obj->refcount))
> +			obj = NULL;
> +	}
> +	drm_vma_offset_unlock_lookup(dev->vma_offset_manager);

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

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

Re: [PATCH] drm/gem: Use kref_get_unless_zero for the weak mmap references

[Daniel Vetter]

On Thu, Oct 15, 2015 at 11:06:59AM +0100, Chris Wilson wrote:
> On Thu, Oct 15, 2015 at 11:33:43AM +0200, Daniel Vetter wrote:
> > Compared to wrapping the final kref_put with dev->struct_mutex this
> > allows us to only acquire the offset manager look both in the final
> > cleanup and in the lookup. Which has the upside that no locks leak out
> > of the core abstractions. But it means that we need to hold a
> > temporary reference to the object while checking mmap constraints, to
> > make sure the object doesn't disappear. Extended the critical region
> > would have worked too, but would result in more leaky locking.
> > 
> > Also, this is the final bit which required dev->struct_mutex in gem
> > core, now modern drivers can be completely struct_mutex free!
> > 
> > This needs a new drm_vma_offset_exact_lookup_locked and makes both
> > drm_vma_offset_exact_lookup and drm_vma_offset_lookup unused.
> > 
> > v2: Don't leak object references in failure paths (David).
> > 
> > Cc: David Herrmann <dh.herrmann@gmail.com>
> > Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_gem.c         | 30 +++++++++++++++++------------
> >  drivers/gpu/drm/drm_vma_manager.c | 40 ++++++++++++---------------------------
> >  include/drm/drm_vma_manager.h     | 22 ++++++---------------
> >  3 files changed, 36 insertions(+), 56 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index ab8ea42264f4..663fadbe979e 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -862,30 +862,36 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> >  {
> >  	struct drm_file *priv = filp->private_data;
> >  	struct drm_device *dev = priv->minor->dev;
> > -	struct drm_gem_object *obj;
> > +	struct drm_gem_object *obj = NULL;
> >  	struct drm_vma_offset_node *node;
> >  	int ret;
> >  
> >  	if (drm_device_is_unplugged(dev))
> >  		return -ENODEV;
> >  
> 
> /* bla bla bla
>  * When the object is being freed, after it hits 0-refcnt
>  * it acquires the struct_mutex and proceeds to tear down
>  * the object. In the process it will attempt to remove
>  * the VMA offset and so acquire this mgr->vm_lock.
>  * Therefore if we find an object with a 0-refcnt that matches
>  * our range, we know it is in the process of being destroyed
>  * and will be freed as soon as we release the lock - so
>  * we have to check for the 0-refcnted object and treat it as
>  * invalid.
>  */

As discussed on irc struct_mutex is just a digression, so I left that part
out when adding your comment.

> > -	mutex_lock(&dev->struct_mutex);
> > +	drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> > +	node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> > +						  vma->vm_pgoff,
> > +						  vma_pages(vma));
> > +	if (likely(node)) {
> > +		obj = container_of(node, struct drm_gem_object, vma_node);
> > +		if (!kref_get_unless_zero(&obj->refcount))
> > +			obj = NULL;
> > +	}
> > +	drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

All four core gem patches now merged in drm-misc, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/gem: Use kref_get_unless_zero for the weak mmap references

[Daniel Vetter]

On Thu, Oct 15, 2015 at 01:11:36PM +0200, Daniel Vetter wrote:
> On Thu, Oct 15, 2015 at 11:06:59AM +0100, Chris Wilson wrote:
> > On Thu, Oct 15, 2015 at 11:33:43AM +0200, Daniel Vetter wrote:
> > > Compared to wrapping the final kref_put with dev->struct_mutex this
> > > allows us to only acquire the offset manager look both in the final
> > > cleanup and in the lookup. Which has the upside that no locks leak out
> > > of the core abstractions. But it means that we need to hold a
> > > temporary reference to the object while checking mmap constraints, to
> > > make sure the object doesn't disappear. Extended the critical region
> > > would have worked too, but would result in more leaky locking.
> > > 
> > > Also, this is the final bit which required dev->struct_mutex in gem
> > > core, now modern drivers can be completely struct_mutex free!
> > > 
> > > This needs a new drm_vma_offset_exact_lookup_locked and makes both
> > > drm_vma_offset_exact_lookup and drm_vma_offset_lookup unused.
> > > 
> > > v2: Don't leak object references in failure paths (David).
> > > 
> > > Cc: David Herrmann <dh.herrmann@gmail.com>
> > > Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_gem.c         | 30 +++++++++++++++++------------
> > >  drivers/gpu/drm/drm_vma_manager.c | 40 ++++++++++++---------------------------
> > >  include/drm/drm_vma_manager.h     | 22 ++++++---------------
> > >  3 files changed, 36 insertions(+), 56 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index ab8ea42264f4..663fadbe979e 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -862,30 +862,36 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> > >  {
> > >  	struct drm_file *priv = filp->private_data;
> > >  	struct drm_device *dev = priv->minor->dev;
> > > -	struct drm_gem_object *obj;
> > > +	struct drm_gem_object *obj = NULL;
> > >  	struct drm_vma_offset_node *node;
> > >  	int ret;
> > >  
> > >  	if (drm_device_is_unplugged(dev))
> > >  		return -ENODEV;
> > >  
> > 
> > /* bla bla bla
> >  * When the object is being freed, after it hits 0-refcnt
> >  * it acquires the struct_mutex and proceeds to tear down
> >  * the object. In the process it will attempt to remove
> >  * the VMA offset and so acquire this mgr->vm_lock.
> >  * Therefore if we find an object with a 0-refcnt that matches
> >  * our range, we know it is in the process of being destroyed
> >  * and will be freed as soon as we release the lock - so
> >  * we have to check for the 0-refcnted object and treat it as
> >  * invalid.
> >  */
> 
> As discussed on irc struct_mutex is just a digression, so I left that part
> out when adding your comment.
> 
> > > -	mutex_lock(&dev->struct_mutex);
> > > +	drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> > > +	node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> > > +						  vma->vm_pgoff,
> > > +						  vma_pages(vma));
> > > +	if (likely(node)) {
> > > +		obj = container_of(node, struct drm_gem_object, vma_node);
> > > +		if (!kref_get_unless_zero(&obj->refcount))
> > > +			obj = NULL;
> > > +	}
> > > +	drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
> > 
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> All four core gem patches now merged in drm-misc, thanks for the review.

Argh, last patch needs one of the vgem ones as prep, so dropped it again.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] 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>
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

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

[Patrik Jakobsson]

On Thu, Oct 15, 2015 at 2:35 PM, Daniel Vetter <daniel.vetter@ffwll.ch> 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>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.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

Re: [PATCH 12/25] drm/gma500: Use correct unref in the gem bo create function

[Patrik Jakobsson]

On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 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.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>

> ---
>  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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/25] drm/gma500: Drop dev->struct_mutex from modeset code

[Patrik Jakobsson]

On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 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.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>

> ---
>  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
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/25] drm/gma500: Drop dev->struct_mutex from fbdev init/teardown code

[Patrik Jakobsson]

On Thu, Oct 15, 2015 at 9:36 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 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.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>

> ---
>  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 2eaf1b31c7bd..c7904fc3d33b 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 18/25] drm/radeon: Use rdev->gem.mutex to protect hyperz/cmask owners

[Alex Deucher]

On Thu, Oct 15, 2015 at 4:27 AM, Christian König
<deathsimple@vodafone.de> wrote:
> On 15.10.2015 09:36, Daniel Vetter wrote:
>>
>> This removes the last depency of radeon for dev->struct_mutex!
>>
>> Also the locking scheme for hyperz/cmask owners seems a bit unsound,
>> there's no protection in the preclose handler (and that never did hold
>> dev->struct_mutex while being called). So grab the same lock there,
>> too.
>>
>> There's also all the checks in the cs checker, but since the overall
>> design seems to never stall for the previous owner I figured it's ok
>> if I leave this racy. It was racy even before I touched it after all
>> too.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
>
> I'm not so deep into the pre R600 stuff but this looks completely sane.
>
> Patch is Reviewed-by: Christian König <christian.koenig@amd.com>

Applied to my -next branch.

Thanks,

Alex

>
> Regards,
> Christian.
>
>
>> ---
>>   drivers/gpu/drm/radeon/radeon_kms.c | 10 ++++++++--
>>   1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c
>> b/drivers/gpu/drm/radeon/radeon_kms.c
>> index efa45e502975..893cf1079552 100644
>> --- a/drivers/gpu/drm/radeon/radeon_kms.c
>> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
>> @@ -181,7 +181,9 @@ static void radeon_set_filp_rights(struct drm_device
>> *dev,
>>                                    struct drm_file *applier,
>>                                    uint32_t *value)
>>   {
>> -       mutex_lock(&dev->struct_mutex);
>> +       struct radeon_device *rdev = dev->dev_private;
>> +
>> +       mutex_lock(&rdev->gem.mutex);
>>         if (*value == 1) {
>>                 /* wants rights */
>>                 if (!*owner)
>> @@ -192,7 +194,7 @@ static void radeon_set_filp_rights(struct drm_device
>> *dev,
>>                         *owner = NULL;
>>         }
>>         *value = *owner == applier ? 1 : 0;
>> -       mutex_unlock(&dev->struct_mutex);
>> +       mutex_unlock(&rdev->gem.mutex);
>>   }
>>     /*
>> @@ -727,10 +729,14 @@ void radeon_driver_preclose_kms(struct drm_device
>> *dev,
>>                                 struct drm_file *file_priv)
>>   {
>>         struct radeon_device *rdev = dev->dev_private;
>> +
>> +       mutex_lock(&rdev->gem.mutex);
>>         if (rdev->hyperz_filp == file_priv)
>>                 rdev->hyperz_filp = NULL;
>>         if (rdev->cmask_filp == file_priv)
>>                 rdev->cmask_filp = NULL;
>> +       mutex_unlock(&rdev->gem.mutex);
>> +
>>         radeon_uvd_free_handles(rdev, file_priv);
>>         radeon_vce_free_handles(rdev, file_priv);
>>   }
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 15/25] drm/gma500: Drop dev->struct_mutex from mmap offset function

[Patrik Jakobsson]

On Thu, Oct 15, 2015 at 9:36 AM, 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.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

+1 for removing that stale comment as well

Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

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

On Thu, Oct 15, 2015 at 02:35:04PM +0200, Daniel Vetter wrote:
> Since David Herrmann's mmap vma manager rework we don't need to grab
> dev->struct_mutex any more to prevent races when looking up the mmap
> offset. Drop it and instead don't forget to use the unref_unlocked
> variant (since the drm core still cares).
> 
> 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>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/tegra/gem.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)

Looks good to me:

Acked-by: Thierry Reding <treding@nvidia.com>

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

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

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

Re: [PATCH 06/25] drm/tegra: Use drm_gem_object_reference_unlocked

[Thierry Reding]

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

On Thu, Oct 15, 2015 at 09:36:22AM +0200, 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!
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/tegra/drm.c | 4 +---
>  drivers/gpu/drm/tegra/gem.c | 8 ++------
>  2 files changed, 3 insertions(+), 9 deletions(-)

Nit: s/tegra/Tegra/, bit irrespective:

Acked-by: Thierry Reding <treding@nvidia.com>

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

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

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