[PATCH v2.1 1/3] vga_switcheroo: Add support for switching only the DDC

* [PATCH v2.1 1/3] vga_switcheroo: Add support for switching only the DDC
@ 2015-08-14 16:50 Lukas Wunner
  2015-08-14 16:18 ` [PATCH v2.1 2/3] apple-gmux: Add switch_ddc support Lukas Wunner
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Lukas Wunner @ 2015-08-14 16:50 UTC (permalink / raw)
  To: dri-devel; +Cc: Daniel Vetter, Dave Airlie, Seth Forshee

Originally by Seth Forshee <seth.forshee@canonical.com>, 2012-10-04:
During graphics driver initialization it's useful to be able to mux only
the DDC to the inactive client in order to read the EDID. Add a
switch_ddc callback to allow capable handlers to provide this
functionality, and add vga_switcheroo_switch_ddc() to allow DRM to mux
only the DDC.

Modified by Dave Airlie <airlied@gmail.com>, 2012-12-22:
I can't figure out why I didn't like this, but I rewrote this [...] to
lock/unlock the ddc lines [...]. I think I'd prefer something like that
otherwise the interface got really ugly.

Modified by Lukas Wunner <lukas@wunner.de>, 2015-08-14:
Don't grab vgasr_mutex in lock_ddc/unlock_ddc to avoid the following
deadlock scenarios: (a) During switch (with vgasr_mutex already held),
GPU is woken and probes its outputs, tries to re-acquire vgasr_mutex
to lock DDC lines. (b) Likewise during switch, GPU is suspended and
calls cancel_delayed_work_sync to stop output polling, if poll task
is running at this moment we may wait forever for it to finish.
If we don't grab vgasr_mutex the only bad thing that can happen is
that the handler suddenly disappears. So block handler deregistration
until DDC lines are unlocked again.

WARN_ON_ONCE if unlock_ddc is called without calling lock_ddc first.
Lock ddc_lock in stage2 to avoid race condition where reading the
EDID and switching happens simultaneously. Switch DDC lines on
MIGD / MDIS commands and on runtime suspend. Fix bug in stage2
where no client had the active attribute set if switching failed.
Fix erroneous interface documentation.

If the inactive client registers before the active client then
old_ddc_owner cannot be determined with find_active_client()
(null pointer dereference). Therefore change semantics of the
->switch_ddc handler callback to return old_ddc_owner.

v2.1: Overhaul locking, squash commits (requested by Daniel Vetter)

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]

Cc: Seth Forshee <seth.forshee@canonical.com>
Cc: Dave Airlie <airlied@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/vga/vga_switcheroo.c | 73 +++++++++++++++++++++++++++++++++++++---
 include/linux/vga_switcheroo.h   |  6 ++++
 2 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 37ac7b5..ac4ac12 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -9,12 +9,13 @@
 
  Switcher interface - methods require for ATPX and DCM
  - switchto - this throws the output MUX switch
- - discrete_set_power - sets the power state for the discrete card
+ - switch_ddc - switch only DDC lines, return old DDC owner
+ - power_state - sets the power state for either GPU
 
  GPU driver interface
  - set_gpu_state - this should do the equiv of s/r for the card
 		  - this should *not* set the discrete power state
- - switch_check  - check if the device is in a position to switch now
+ - can_switch - check if the device is in a position to switch now
  */
 
 #include <linux/module.h>
@@ -57,6 +58,8 @@ struct vgasr_priv {
 	struct list_head clients;
 
 	struct vga_switcheroo_handler *handler;
+	struct mutex ddc_lock;
+	int old_ddc_owner;
 };
 
 #define ID_BIT_AUDIO		0x100
@@ -70,6 +73,7 @@ static void vga_switcheroo_debugfs_fini(struct vgasr_priv *priv);
 /* only one switcheroo per system */
 static struct vgasr_priv vgasr_priv = {
 	.clients = LIST_HEAD_INIT(vgasr_priv.clients),
+	.ddc_lock = __MUTEX_INITIALIZER(vgasr_priv.ddc_lock),
 };
 
 static bool vga_switcheroo_ready(void)
@@ -122,12 +126,14 @@ EXPORT_SYMBOL(vga_switcheroo_register_handler);
 void vga_switcheroo_unregister_handler(void)
 {
 	mutex_lock(&vgasr_mutex);
+	mutex_lock(&vgasr_priv.ddc_lock);
 	vgasr_priv.handler = NULL;
 	if (vgasr_priv.active) {
 		pr_info("vga_switcheroo: disabled\n");
 		vga_switcheroo_debugfs_fini(&vgasr_priv);
 		vgasr_priv.active = false;
 	}
+	mutex_unlock(&vgasr_priv.ddc_lock);
 	mutex_unlock(&vgasr_mutex);
 }
 EXPORT_SYMBOL(vga_switcheroo_unregister_handler);
@@ -256,6 +262,43 @@ void vga_switcheroo_client_fb_set(struct pci_dev *pdev,
 }
 EXPORT_SYMBOL(vga_switcheroo_client_fb_set);
 
+int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
+{
+	int id;
+
+	mutex_lock(&vgasr_priv.ddc_lock);
+
+	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc)
+		return vgasr_priv.old_ddc_owner = -ENODEV;
+
+	id = vgasr_priv.handler->get_client_id(pdev);
+	return vgasr_priv.old_ddc_owner = vgasr_priv.handler->switch_ddc(id);
+}
+EXPORT_SYMBOL(vga_switcheroo_lock_ddc);
+
+int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
+{
+	int ret, id;
+
+	if (WARN_ON_ONCE(!mutex_is_locked(&vgasr_priv.ddc_lock)))
+		return -EINVAL;
+
+	if (vgasr_priv.old_ddc_owner < 0) {
+		mutex_unlock(&vgasr_priv.ddc_lock);
+		return -ENODEV;
+	}
+
+	id = vgasr_priv.handler->get_client_id(pdev);
+	if (vgasr_priv.old_ddc_owner != id)
+		ret = vgasr_priv.handler->switch_ddc(vgasr_priv.old_ddc_owner);
+	else
+		ret = vgasr_priv.old_ddc_owner;
+	mutex_unlock(&vgasr_priv.ddc_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(vga_switcheroo_unlock_ddc);
+
 static int vga_switcheroo_show(struct seq_file *m, void *v)
 {
 	struct vga_switcheroo_client *client;
@@ -341,8 +384,6 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 	if (!active)
 		return 0;
 
-	active->active = false;
-
 	set_audio_state(active->id, VGA_SWITCHEROO_OFF);
 
 	if (new_client->fb_info) {
@@ -353,9 +394,21 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 		console_unlock();
 	}
 
+	if (vgasr_priv.handler->switch_ddc) {
+		mutex_lock(&vgasr_priv.ddc_lock);
+		vgasr_priv.handler->switch_ddc(new_client->id);
+		mutex_unlock(&vgasr_priv.ddc_lock);
+	}
 	ret = vgasr_priv.handler->switchto(new_client->id);
-	if (ret)
+	if (ret) {
+		if (vgasr_priv.handler->switch_ddc) {
+			mutex_lock(&vgasr_priv.ddc_lock);
+			vgasr_priv.handler->switch_ddc(active->id);
+			mutex_unlock(&vgasr_priv.ddc_lock);
+		}
 		return ret;
+	}
+	active->active = false;
 
 	if (new_client->ops->reprobe)
 		new_client->ops->reprobe(new_client->pdev);
@@ -468,6 +521,11 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
 	vgasr_priv.delayed_switch_active = false;
 
 	if (just_mux) {
+		if (vgasr_priv.handler->switch_ddc) {
+			mutex_lock(&vgasr_priv.ddc_lock);
+			vgasr_priv.handler->switch_ddc(client_id);
+			mutex_unlock(&vgasr_priv.ddc_lock);
+		}
 		ret = vgasr_priv.handler->switchto(client_id);
 		goto out;
 	}
@@ -623,6 +681,11 @@ static int vga_switcheroo_runtime_suspend(struct device *dev)
 	ret = dev->bus->pm->runtime_suspend(dev);
 	if (ret)
 		return ret;
+	if (vgasr_priv.handler->switch_ddc) {
+		mutex_lock(&vgasr_priv.ddc_lock);
+		vgasr_priv.handler->switch_ddc(VGA_SWITCHEROO_IGD);
+		mutex_unlock(&vgasr_priv.ddc_lock);
+	}
 	if (vgasr_priv.handler->switchto)
 		vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD);
 	vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF);
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index b483abd..62f303f 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -29,6 +29,7 @@ enum vga_switcheroo_client_id {
 };
 
 struct vga_switcheroo_handler {
+	int (*switch_ddc)(enum vga_switcheroo_client_id id);
 	int (*switchto)(enum vga_switcheroo_client_id id);
 	int (*power_state)(enum vga_switcheroo_client_id id,
 			   enum vga_switcheroo_state state);
@@ -54,6 +55,9 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 void vga_switcheroo_client_fb_set(struct pci_dev *dev,
 				  struct fb_info *info);
 
+int vga_switcheroo_lock_ddc(struct pci_dev *pdev);
+int vga_switcheroo_unlock_ddc(struct pci_dev *pdev);
+
 int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler);
 void vga_switcheroo_unregister_handler(void);
 
@@ -72,6 +76,8 @@ static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
 static inline int vga_switcheroo_register_client(struct pci_dev *dev,
 		const struct vga_switcheroo_client_ops *ops, bool driver_power_control) { return 0; }
 static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_info *info) {}
+static inline int vga_switcheroo_lock_ddc(struct pci_dev *pdev) { return -ENODEV; }
+static inline int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { return -ENODEV; }
 static inline int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) { return 0; }
 static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 	const struct vga_switcheroo_client_ops *ops,
-- 
2.1.0

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread