[PATCH 35/50] drm: Rip out totally bogus vga_switcheroo->can_switch locking

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
---
 drivers/gpu/drm/i915/i915_dma.c        | 7 +++++--
 drivers/gpu/drm/nouveau/nouveau_vga.c  | 7 +++++--
 drivers/gpu/drm/radeon/radeon_device.c | 7 +++++--
 include/drm/drmP.h                     | 2 +-
 4 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 7f653ab59db4..2babb597d23c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1269,9 +1269,12 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	bool can_switch;
 
-	spin_lock(&dev->count_lock);
+	/*
+	 * FIXME: open_count is protected by drm_global_mutex but that would lead to
+	 * locking inversion with the driver load path. And the access here is
+	 * completely racy anyway. So don't bother with locking for now.
+	 */
 	can_switch = (dev->open_count == 0);
-	spin_unlock(&dev->count_lock);
 	return can_switch;
 }
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c b/drivers/gpu/drm/nouveau/nouveau_vga.c
index 81638d7f2eff..db721b4d778b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -64,9 +64,12 @@ nouveau_switcheroo_can_switch(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	bool can_switch;
 
-	spin_lock(&dev->count_lock);
+	/*
+	 * FIXME: open_count is protected by drm_global_mutex but that would lead to
+	 * locking inversion with the driver load path. And the access here is
+	 * completely racy anyway. So don't bother with locking for now.
+	 */
 	can_switch = (dev->open_count == 0);
-	spin_unlock(&dev->count_lock);
 	return can_switch;
 }
 
diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
index 39b033b441d2..d67140811ca5 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1119,9 +1119,12 @@ static bool radeon_switcheroo_can_switch(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	bool can_switch;
 
-	spin_lock(&dev->count_lock);
+	/*
+	 * FIXME: open_count is protected by drm_global_mutex but that would lead to
+	 * locking inversion with the driver load path. And the access here is
+	 * completely racy anyway. So don't bother with locking for now.
+	 */
 	can_switch = (dev->open_count == 0);
-	spin_unlock(&dev->count_lock);
 	return can_switch;
 }
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 1016ecc8de95..ee65c25e25d3 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1085,7 +1085,7 @@ struct drm_device {
 
 	/** \name Usage Counters */
 	/*@{ */
-	int open_count;			/**< Outstanding files open */
+	int open_count;			/**< Outstanding files open, protected by drm_global_lock. */
 	atomic_t ioctl_count;		/**< Outstanding IOCTLs pending */
 	atomic_t vma_count;		/**< Outstanding vma areas open */
 	int buf_use;			/**< Buffers in use -- cannot alloc */
-- 
1.8.4.3

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux