diff options
author | Daniel Vetter <daniel.vetter@ffwll.ch> | 2013-11-03 14:46:34 -0500 |
---|---|---|
committer | Daniel Vetter <daniel.vetter@ffwll.ch> | 2014-04-23 04:32:33 -0400 |
commit | fc8fd40eb29a936cc689d0008863d39a67741c67 (patch) | |
tree | 708dadf9ad9eb22a9a88cf12de128501c63d731c | |
parent | 42b21049fc26513ca8e732f47559b1525b04a992 (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.c | 11 | ||||
-rw-r--r-- | drivers/gpu/drm/nouveau/nouveau_vga.c | 11 | ||||
-rw-r--r-- | drivers/gpu/drm/radeon/radeon_device.c | 11 | ||||
-rw-r--r-- | include/drm/drmP.h | 2 |
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_ | |||
1277 | static bool i915_switcheroo_can_switch(struct pci_dev *pdev) | 1277 | static 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 | ||
1288 | static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { | 1289 | static 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 | |||
64 | nouveau_switcheroo_can_switch(struct pci_dev *pdev) | 64 | nouveau_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 | ||
75 | static const struct vga_switcheroo_client_ops | 76 | static 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 | |||
1125 | static bool radeon_switcheroo_can_switch(struct pci_dev *pdev) | 1125 | static 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 | ||
1136 | static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = { | 1137 | static 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 | /*@} */ |