aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorDaniel Vetter <daniel.vetter@ffwll.ch>2013-11-03 14:46:34 -0500
committerDaniel Vetter <daniel.vetter@ffwll.ch>2014-04-23 04:32:33 -0400
commitfc8fd40eb29a936cc689d0008863d39a67741c67 (patch)
tree708dadf9ad9eb22a9a88cf12de128501c63d731c
parent42b21049fc26513ca8e732f47559b1525b04a992 (diff)
drm: Rip out totally bogus vga_switcheroo->can_switch locking
So I just wanted to add a new field to struct drm_device and accidentally stumbled over something. According to comments dev->open_count is protected by dev->count_lock, but that's totally not the case. It's protected by drm_global_mutex. Unfortunately the vga switcheroo callbacks took this comment at face value. The problem is that we can't just take the drm_global_mutex because: - It would lead to a locking inversion with the driver load/unload paths. - It wouldn't actually protect anything, for that we'd need to wrap the entire vga switcheroo code in the drm_global_mutex. And I'm not sure whether that would actually solve anything. What we probably want is a try_to_grab_switcheroo reference kind of thing which is used in the driver's ->open callback. Then we could move all that ->can_switch madness into the vga switcheroo core where it really belongs. But since that would amount to real work take the easy way out and just add a comment. It's definitely not going to make anything worse since doing switcheroo state changes while restarting X just isn't recommended. Even though the delayed switching code does exactly that. v2: - Simplify the ->can_switch implementations more (Thierry) - Fix comment about the dev->open_count locking (Thierry) Cc: Thierry Reding <treding@nvidia.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> (v1) Reviewed-by: Thierry Reding <treding@nvidia.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-rw-r--r--drivers/gpu/drm/i915/i915_dma.c11
-rw-r--r--drivers/gpu/drm/nouveau/nouveau_vga.c11
-rw-r--r--drivers/gpu/drm/radeon/radeon_device.c11
-rw-r--r--include/drm/drmP.h2
4 files changed, 19 insertions, 16 deletions
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 96177eec0a0e..283ff06001bc 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1277,12 +1277,13 @@ static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_
1277static bool i915_switcheroo_can_switch(struct pci_dev *pdev) 1277static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
1278{ 1278{
1279 struct drm_device *dev = pci_get_drvdata(pdev); 1279 struct drm_device *dev = pci_get_drvdata(pdev);
1280 bool can_switch;
1281 1280
1282 spin_lock(&dev->count_lock); 1281 /*
1283 can_switch = (dev->open_count == 0); 1282 * FIXME: open_count is protected by drm_global_mutex but that would lead to
1284 spin_unlock(&dev->count_lock); 1283 * locking inversion with the driver load path. And the access here is
1285 return can_switch; 1284 * completely racy anyway. So don't bother with locking for now.
1285 */
1286 return dev->open_count == 0;
1286} 1287}
1287 1288
1288static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { 1289static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index fb84da3cb50d..4f4c3fec6916 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -64,12 +64,13 @@ static bool
64nouveau_switcheroo_can_switch(struct pci_dev *pdev) 64nouveau_switcheroo_can_switch(struct pci_dev *pdev)
65{ 65{
66 struct drm_device *dev = pci_get_drvdata(pdev); 66 struct drm_device *dev = pci_get_drvdata(pdev);
67 bool can_switch;
68 67
69 spin_lock(&dev->count_lock); 68 /*
70 can_switch = (dev->open_count == 0); 69 * FIXME: open_count is protected by drm_global_mutex but that would lead to
71 spin_unlock(&dev->count_lock); 70 * locking inversion with the driver load path. And the access here is
72 return can_switch; 71 * completely racy anyway. So don't bother with locking for now.
72 */
73 return dev->open_count == 0;
73} 74}
74 75
75static const struct vga_switcheroo_client_ops 76static const struct vga_switcheroo_client_ops
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 511fe26198e4..9aa1afd1786e 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1125,12 +1125,13 @@ static void radeon_switcheroo_set_state(struct pci_dev *pdev, enum vga_switchero
1125static bool radeon_switcheroo_can_switch(struct pci_dev *pdev) 1125static bool radeon_switcheroo_can_switch(struct pci_dev *pdev)
1126{ 1126{
1127 struct drm_device *dev = pci_get_drvdata(pdev); 1127 struct drm_device *dev = pci_get_drvdata(pdev);
1128 bool can_switch;
1129 1128
1130 spin_lock(&dev->count_lock); 1129 /*
1131 can_switch = (dev->open_count == 0); 1130 * FIXME: open_count is protected by drm_global_mutex but that would lead to
1132 spin_unlock(&dev->count_lock); 1131 * locking inversion with the driver load path. And the access here is
1133 return can_switch; 1132 * completely racy anyway. So don't bother with locking for now.
1133 */
1134 return dev->open_count == 0;
1134} 1135}
1135 1136
1136static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = { 1137static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = {
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 9f1fb8d36b67..a20d882ca265 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1076,7 +1076,7 @@ struct drm_device {
1076 1076
1077 /** \name Usage Counters */ 1077 /** \name Usage Counters */
1078 /*@{ */ 1078 /*@{ */
1079 int open_count; /**< Outstanding files open */ 1079 int open_count; /**< Outstanding files open, protected by drm_global_mutex. */
1080 int buf_use; /**< Buffers in use -- cannot alloc */ 1080 int buf_use; /**< Buffers in use -- cannot alloc */
1081 atomic_t buf_alloc; /**< Buffer allocation in progress */ 1081 atomic_t buf_alloc; /**< Buffer allocation in progress */
1082 /*@} */ 1082 /*@} */