Commit fc8fd40e authored by Daniel Vetter's avatar Daniel Vetter

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: default avatarThierry Reding <treding@nvidia.com>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
parent 42b21049
...@@ -1277,12 +1277,13 @@ static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_ ...@@ -1277,12 +1277,13 @@ static void i915_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_
static bool i915_switcheroo_can_switch(struct pci_dev *pdev) static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
{ {
struct drm_device *dev = pci_get_drvdata(pdev); struct drm_device *dev = pci_get_drvdata(pdev);
bool can_switch;
spin_lock(&dev->count_lock); /*
can_switch = (dev->open_count == 0); * FIXME: open_count is protected by drm_global_mutex but that would lead to
spin_unlock(&dev->count_lock); * locking inversion with the driver load path. And the access here is
return can_switch; * completely racy anyway. So don't bother with locking for now.
*/
return dev->open_count == 0;
} }
static const struct vga_switcheroo_client_ops i915_switcheroo_ops = { static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
......
...@@ -64,12 +64,13 @@ static bool ...@@ -64,12 +64,13 @@ static bool
nouveau_switcheroo_can_switch(struct pci_dev *pdev) nouveau_switcheroo_can_switch(struct pci_dev *pdev)
{ {
struct drm_device *dev = pci_get_drvdata(pdev); struct drm_device *dev = pci_get_drvdata(pdev);
bool can_switch;
spin_lock(&dev->count_lock); /*
can_switch = (dev->open_count == 0); * FIXME: open_count is protected by drm_global_mutex but that would lead to
spin_unlock(&dev->count_lock); * locking inversion with the driver load path. And the access here is
return can_switch; * completely racy anyway. So don't bother with locking for now.
*/
return dev->open_count == 0;
} }
static const struct vga_switcheroo_client_ops static const struct vga_switcheroo_client_ops
......
...@@ -1125,12 +1125,13 @@ static void radeon_switcheroo_set_state(struct pci_dev *pdev, enum vga_switchero ...@@ -1125,12 +1125,13 @@ static void radeon_switcheroo_set_state(struct pci_dev *pdev, enum vga_switchero
static bool radeon_switcheroo_can_switch(struct pci_dev *pdev) static bool radeon_switcheroo_can_switch(struct pci_dev *pdev)
{ {
struct drm_device *dev = pci_get_drvdata(pdev); struct drm_device *dev = pci_get_drvdata(pdev);
bool can_switch;
spin_lock(&dev->count_lock); /*
can_switch = (dev->open_count == 0); * FIXME: open_count is protected by drm_global_mutex but that would lead to
spin_unlock(&dev->count_lock); * locking inversion with the driver load path. And the access here is
return can_switch; * completely racy anyway. So don't bother with locking for now.
*/
return dev->open_count == 0;
} }
static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = { static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = {
......
...@@ -1076,7 +1076,7 @@ struct drm_device { ...@@ -1076,7 +1076,7 @@ struct drm_device {
/** \name Usage Counters */ /** \name Usage Counters */
/*@{ */ /*@{ */
int open_count; /**< Outstanding files open */ int open_count; /**< Outstanding files open, protected by drm_global_mutex. */
int buf_use; /**< Buffers in use -- cannot alloc */ int buf_use; /**< Buffers in use -- cannot alloc */
atomic_t buf_alloc; /**< Buffer allocation in progress */ atomic_t buf_alloc; /**< Buffer allocation in progress */
/*@} */ /*@} */
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment