Enable gpu switching on the MacBook Pro

Originally by Seth Forshee <[email protected]>, 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 <[email protected]>, 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 <[email protected]>, 2015-08-14:
Don't lock vgasr_mutex in _lock_ddc() / _unlock_ddc(), it can cause deadlocks because (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.

Instead, lock ddc_lock when unregistering the handler because the only reason why we'd want to lock vgasr_mutex in _lock_ddc() / _unlock_ddc() is to block the handler from disappearing while DDC lines are switched.

Also lock ddc_lock in stage2 to avoid race condition where reading the EDID and switching happens simultaneously. Likewise on MIGD / MDIS commands and on runtime suspend.

Retain semantics of ->switchto handler callback to switch all pins, including DDC. Change semantics of ->switch_ddc handler callback to return previous DDC owner. Original version tried to determine previous DDC owner with find_active_client() but this fails if the inactive client registers before the active client.

v2.1: Overhaul locking, squash commits
(requested by Daniel Vetter <[email protected]>)

v2.2: Readability improvements
(requested by Thierry Reding <[email protected]>)

v2.3: Overhaul locking once more

v2.4: Retain semantics of ->switchto handler callback to switch all pins
(requested by Daniel Vetter <[email protected]>)

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115

Tested-by Pierre Moreau <[email protected]>

    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]

Tested-by William Brown <[email protected]>

[MBP 8,2 2011 intel SNB + amd turks pre-retina 15"]

Tested-by Lukas Wunner <[email protected]>

    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Cc Seth Forshee <[email protected]>

Cc Dave Airlie <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/vga/vga_switcheroo.c | 98 +++++++++++++++++++++++++++++++++++++++-
 include/linux/vga_switcheroo.h   |  9 ++++
 2 files changed, 105 insertions(+), 2 deletions(-)

drivers/gpu/vga/vga_switcheroo.c +96 -2

index aa077aa..9b6946e 100644
@@ -73,9 +73,19 @@
  * there can thus be up to three clients: Two vga clients (GPUs) and one audio
  * client (on the discrete GPU). The code is mostly prepared to support
  * machines with more than two GPUs should they become available.
+ *
  * The GPU to which the outputs are currently switched is called the
  * active client in vga_switcheroo parlance. The GPU not in use is the
- * inactive client.
+ * inactive client. When the inactive client's DRM driver is loaded,
+ * it will be unable to probe the panel's EDID and hence depends on
+ * VBIOS to provide its display modes. If the VBIOS modes are bogus or
+ * if there is no VBIOS at all (which is common on the MacBook Pro),
+ * a client may alternatively request that the DDC lines are temporarily
+ * switched to it, provided that the handler supports this. Switching
+ * only the DDC lines and not the entire output avoids unnecessary
+ * flickering. On machines which are able to mux external connectors,
+ * VBIOS cannot know of attached displays so switching the DDC lines
+ * is the only option to retrieve the displays' EDID.
  */
 
 /**

@@ -125,6 +135,8 @@ static DEFINE_MUTEX(vgasr_mutex);
  * 	(counting only vga clients, not audio clients)
  * @clients: list of registered clients
  * @handler: registered handler
+ * @ddc_lock: protects access to DDC lines while they are temporarily switched
+ * @old_ddc_owner: client to which DDC lines will be switched back on unlock
  *
  * vga_switcheroo private data. Currently only one vga_switcheroo instance
  * per system is supported.

@@ -141,6 +153,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

@@ -155,6 +169,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)

@@ -221,12 +236,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("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);

@@ -412,6 +429,76 @@ void vga_switcheroo_client_fb_set(struct pci_dev *pdev,
 EXPORT_SYMBOL(vga_switcheroo_client_fb_set);
 
 /**
+ * vga_switcheroo_lock_ddc() - temporarily switch DDC lines to a given client
+ * @pdev: client pci device
+ *
+ * Temporarily switch DDC lines to the client identified by @pdev
+ * (but leave the outputs otherwise switched to where they are).
+ * This allows the inactive client to probe EDID. The DDC lines must
+ * afterwards be switched back by calling vga_switcheroo_unlock_ddc(),
+ * even if this function returns an error.
+ *
+ * Return: Previous DDC owner on success or a negative int on error.
+ * Specifically, -ENODEV if no handler has registered or if the handler
+ * does not support switching the DDC lines. Also, a negative value
+ * returned by the handler is propagated back to the caller.
+ * The return value has merely an informational purpose for any caller
+ * which might be interested in it. It is acceptable to ignore the return
+ * value and simply rely on the result of the subsequent EDID probe,
+ * which will be NULL if DDC switching failed.
+ */
+int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
+{
+	enum vga_switcheroo_client_id id;
+
+	mutex_lock(&vgasr_priv.ddc_lock);
+	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
+		vgasr_priv.old_ddc_owner = -ENODEV;
+		return -ENODEV;
+	}
+
+	id = vgasr_priv.handler->get_client_id(pdev);
+	vgasr_priv.old_ddc_owner = vgasr_priv.handler->switch_ddc(id);
+	return vgasr_priv.old_ddc_owner;
+}
+EXPORT_SYMBOL(vga_switcheroo_lock_ddc);
+
+/**
+ * vga_switcheroo_unlock_ddc() - switch DDC lines back to previous owner
+ * @pdev: client pci device
+ *
+ * Switch DDC lines back to the previous owner after calling
+ * vga_switcheroo_lock_ddc(). This must be called even if
+ * vga_switcheroo_lock_ddc() returned an error.
+ *
+ * Return: Previous DDC owner on success (i.e. the client identifier of @pdev)
+ * or a negative int on error.
+ * Specifically, -ENODEV if no handler has registered or if the handler
+ * does not support switching the DDC lines. Also, a negative value
+ * returned by the handler is propagated back to the caller.
+ * Finally, invoking this function without calling vga_switcheroo_lock_ddc()
+ * first is not allowed and will result in -EINVAL.
+ */
+int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
+{
+	enum vga_switcheroo_client_id id;
+	int ret = vgasr_priv.old_ddc_owner;
+
+	if (WARN_ON_ONCE(!mutex_is_locked(&vgasr_priv.ddc_lock)))
+		return -EINVAL;
+
+	if (vgasr_priv.old_ddc_owner >= 0) {
+		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);
+	}
+	mutex_unlock(&vgasr_priv.ddc_lock);
+	return ret;
+}
+EXPORT_SYMBOL(vga_switcheroo_unlock_ddc);
+
+/**
  * DOC: Manual switching and manual power control
  *
  * In this mode of use, the file /sys/kernel/debug/vgaswitcheroo/switch

@@ -548,7 +635,9 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 		console_unlock();
 	}
 
+	mutex_lock(&vgasr_priv.ddc_lock);
 	ret = vgasr_priv.handler->switchto(new_client->id);
+	mutex_unlock(&vgasr_priv.ddc_lock);
 	if (ret)
 		return ret;
 

@@ -663,7 +752,9 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
 	vgasr_priv.delayed_switch_active = false;
 
 	if (just_mux) {
+		mutex_lock(&vgasr_priv.ddc_lock);
 		ret = vgasr_priv.handler->switchto(client_id);
+		mutex_unlock(&vgasr_priv.ddc_lock);
 		goto out;
 	}
 

@@ -875,8 +966,11 @@ static int vga_switcheroo_runtime_suspend(struct device *dev)
 	if (ret)
 		return ret;
 	mutex_lock(&vgasr_mutex);
-	if (vgasr_priv.handler->switchto)
+	if (vgasr_priv.handler->switchto) {
+		mutex_lock(&vgasr_priv.ddc_lock);
 		vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD);
+		mutex_unlock(&vgasr_priv.ddc_lock);
+	}
 	vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF);
 	mutex_unlock(&vgasr_mutex);
 	return 0;

include/linux/vga_switcheroo.h +9 -0

index b2a3dda..6edaacc 100644
@@ -82,6 +82,9 @@ enum vga_switcheroo_client_id {
  * 	Mandatory. For muxless machines this should be a no-op. Returning 0
  * 	denotes success, anything else failure (in which case the switch is
  * 	aborted)
+ * @switch_ddc: switch DDC lines to given client.
+ * 	Optional. Should return the previous DDC owner on success or a
+ * 	negative int on failure
  * @power_state: cut or reinstate power of given client.
  * 	Optional. The return value is ignored
  * @get_client_id: determine if given pci device is integrated or discrete GPU.

@@ -93,6 +96,7 @@ enum vga_switcheroo_client_id {
 struct vga_switcheroo_handler {
 	int (*init)(void);
 	int (*switchto)(enum vga_switcheroo_client_id id);
+	int (*switch_ddc)(enum vga_switcheroo_client_id id);
 	int (*power_state)(enum vga_switcheroo_client_id id,
 			   enum vga_switcheroo_state state);
 	enum vga_switcheroo_client_id (*get_client_id)(struct pci_dev *pdev);

@@ -132,6 +136,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);
 

@@ -150,6 +157,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,

Signature · 6 lines

1.8.5.2 (Apple Git-48)

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 7 lines · Lukas Wunner

Don't lock vgasr_mutex in _lock_ddc() / _unlock_ddc(), it can cause deadlocks because (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.

Quoted 4 lines

Instead, lock ddc_lock when unregistering the handler because the only reason why we'd want to lock vgasr_mutex in _lock_ddc() / _unlock_ddc() is to block the handler from disappearing while DDC lines are switched.

Shouldn't we also take the new look in register_handler, for consistency? I know that if you look the mux provider too late it's fairly hopeless ...

Also while reading the patch I realized that the new lock really protects hw state (instead of our software-side tracking and driver resume/suspend code). And at least myself I have no idea what vgasr means, so what about renaming it to hw_mutex? We have this pattern of low-level locks to avoid concurrent hw access in a lot of places like i2c, dp_aux, and it's very often called hw_lock or similar.

Wrt the overall patch series, can you pls haggle driver-maintainers (gmux, radeon, nouveau) for acks/reviews? scripts/get_maintainers.pl should help.

Also, the revised docbook patch seems to be missing from this iteration, please follow up with that one.

Quoted 258 lines

Also lock ddc_lock in stage2 to avoid race condition where reading the EDID and switching happens simultaneously. Likewise on MIGD / MDIS commands and on runtime suspend.

Retain semantics of ->switchto handler callback to switch all pins, including DDC. Change semantics of ->switch_ddc handler callback to return previous DDC owner. Original version tried to determine previous DDC owner with find_active_client() but this fails if the inactive client registers before the active client.

v2.1: Overhaul locking, squash commits
(requested by Daniel Vetter <[email protected]>)

v2.2: Readability improvements
(requested by Thierry Reding <[email protected]>)

v2.3: Overhaul locking once more

v2.4: Retain semantics of ->switchto handler callback to switch all pins
(requested by Daniel Vetter <[email protected]>)

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115

Tested-by Pierre Moreau <[email protected]>

    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]

Tested-by William Brown <[email protected]>

[MBP 8,2 2011 intel SNB + amd turks pre-retina 15"]

Tested-by Lukas Wunner <[email protected]>

    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Cc Seth Forshee <[email protected]>

Cc Dave Airlie <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/vga/vga_switcheroo.c | 98 +++++++++++++++++++++++++++++++++++++++-
 include/linux/vga_switcheroo.h   |  9 ++++
 2 files changed, 105 insertions(+), 2 deletions(-)

drivers/gpu/vga/vga_switcheroo.c +96 -2

index aa077aa..9b6946e 100644
@@ -73,9 +73,19 @@
  * there can thus be up to three clients: Two vga clients (GPUs) and one audio
  * client (on the discrete GPU). The code is mostly prepared to support
  * machines with more than two GPUs should they become available.
+ *
  * The GPU to which the outputs are currently switched is called the
  * active client in vga_switcheroo parlance. The GPU not in use is the
- * inactive client.
+ * inactive client. When the inactive client's DRM driver is loaded,
+ * it will be unable to probe the panel's EDID and hence depends on
+ * VBIOS to provide its display modes. If the VBIOS modes are bogus or
+ * if there is no VBIOS at all (which is common on the MacBook Pro),
+ * a client may alternatively request that the DDC lines are temporarily
+ * switched to it, provided that the handler supports this. Switching
+ * only the DDC lines and not the entire output avoids unnecessary
+ * flickering. On machines which are able to mux external connectors,
+ * VBIOS cannot know of attached displays so switching the DDC lines
+ * is the only option to retrieve the displays' EDID.
  */
 
 /**

@@ -125,6 +135,8 @@ static DEFINE_MUTEX(vgasr_mutex);
  * 	(counting only vga clients, not audio clients)
  * @clients: list of registered clients
  * @handler: registered handler
+ * @ddc_lock: protects access to DDC lines while they are temporarily switched
+ * @old_ddc_owner: client to which DDC lines will be switched back on unlock
  *
  * vga_switcheroo private data. Currently only one vga_switcheroo instance
  * per system is supported.

@@ -141,6 +153,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

@@ -155,6 +169,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)

@@ -221,12 +236,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("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);

@@ -412,6 +429,76 @@ void vga_switcheroo_client_fb_set(struct pci_dev *pdev,
 EXPORT_SYMBOL(vga_switcheroo_client_fb_set);
 
 /**
+ * vga_switcheroo_lock_ddc() - temporarily switch DDC lines to a given client
+ * @pdev: client pci device
+ *
+ * Temporarily switch DDC lines to the client identified by @pdev
+ * (but leave the outputs otherwise switched to where they are).
+ * This allows the inactive client to probe EDID. The DDC lines must
+ * afterwards be switched back by calling vga_switcheroo_unlock_ddc(),
+ * even if this function returns an error.
+ *
+ * Return: Previous DDC owner on success or a negative int on error.
+ * Specifically, -ENODEV if no handler has registered or if the handler
+ * does not support switching the DDC lines. Also, a negative value
+ * returned by the handler is propagated back to the caller.
+ * The return value has merely an informational purpose for any caller
+ * which might be interested in it. It is acceptable to ignore the return
+ * value and simply rely on the result of the subsequent EDID probe,
+ * which will be NULL if DDC switching failed.
+ */
+int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
+{
+	enum vga_switcheroo_client_id id;
+
+	mutex_lock(&vgasr_priv.ddc_lock);
+	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
+		vgasr_priv.old_ddc_owner = -ENODEV;
+		return -ENODEV;
+	}
+
+	id = vgasr_priv.handler->get_client_id(pdev);
+	vgasr_priv.old_ddc_owner = vgasr_priv.handler->switch_ddc(id);
+	return vgasr_priv.old_ddc_owner;
+}
+EXPORT_SYMBOL(vga_switcheroo_lock_ddc);
+
+/**
+ * vga_switcheroo_unlock_ddc() - switch DDC lines back to previous owner
+ * @pdev: client pci device
+ *
+ * Switch DDC lines back to the previous owner after calling
+ * vga_switcheroo_lock_ddc(). This must be called even if
+ * vga_switcheroo_lock_ddc() returned an error.
+ *
+ * Return: Previous DDC owner on success (i.e. the client identifier of @pdev)
+ * or a negative int on error.
+ * Specifically, -ENODEV if no handler has registered or if the handler
+ * does not support switching the DDC lines. Also, a negative value
+ * returned by the handler is propagated back to the caller.
+ * Finally, invoking this function without calling vga_switcheroo_lock_ddc()
+ * first is not allowed and will result in -EINVAL.
+ */
+int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
+{
+	enum vga_switcheroo_client_id id;
+	int ret = vgasr_priv.old_ddc_owner;
+
+	if (WARN_ON_ONCE(!mutex_is_locked(&vgasr_priv.ddc_lock)))
+		return -EINVAL;
+
+	if (vgasr_priv.old_ddc_owner >= 0) {
+		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);
+	}
+	mutex_unlock(&vgasr_priv.ddc_lock);
+	return ret;
+}
+EXPORT_SYMBOL(vga_switcheroo_unlock_ddc);
+
+/**
  * DOC: Manual switching and manual power control
  *
  * In this mode of use, the file /sys/kernel/debug/vgaswitcheroo/switch

@@ -548,7 +635,9 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 		console_unlock();
 	}
 
+	mutex_lock(&vgasr_priv.ddc_lock);
 	ret = vgasr_priv.handler->switchto(new_client->id);
+	mutex_unlock(&vgasr_priv.ddc_lock);
 	if (ret)
 		return ret;
 

@@ -663,7 +752,9 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
 	vgasr_priv.delayed_switch_active = false;
 
 	if (just_mux) {
+		mutex_lock(&vgasr_priv.ddc_lock);
 		ret = vgasr_priv.handler->switchto(client_id);
+		mutex_unlock(&vgasr_priv.ddc_lock);
 		goto out;
 	}
 

@@ -875,8 +966,11 @@ static int vga_switcheroo_runtime_suspend(struct device *dev)
 	if (ret)
 		return ret;
 	mutex_lock(&vgasr_mutex);
-	if (vgasr_priv.handler->switchto)
+	if (vgasr_priv.handler->switchto) {
+		mutex_lock(&vgasr_priv.ddc_lock);
 		vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD);
+		mutex_unlock(&vgasr_priv.ddc_lock);
+	}
 	vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF);
 	mutex_unlock(&vgasr_mutex);
 	return 0;

include/linux/vga_switcheroo.h +9 -0

index b2a3dda..6edaacc 100644
@@ -82,6 +82,9 @@ enum vga_switcheroo_client_id {
  * 	Mandatory. For muxless machines this should be a no-op. Returning 0
  * 	denotes success, anything else failure (in which case the switch is
  * 	aborted)
+ * @switch_ddc: switch DDC lines to given client.
+ * 	Optional. Should return the previous DDC owner on success or a
+ * 	negative int on failure
  * @power_state: cut or reinstate power of given client.
  * 	Optional. The return value is ignored
  * @get_client_id: determine if given pci device is integrated or discrete GPU.

@@ -93,6 +96,7 @@ enum vga_switcheroo_client_id {
 struct vga_switcheroo_handler {
 	int (*init)(void);
 	int (*switchto)(enum vga_switcheroo_client_id id);
+	int (*switch_ddc)(enum vga_switcheroo_client_id id);
 	int (*power_state)(enum vga_switcheroo_client_id id,
 			   enum vga_switcheroo_state state);
 	enum vga_switcheroo_client_id (*get_client_id)(struct pci_dev *pdev);

@@ -132,6 +136,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);
 

@@ -150,6 +157,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,

Signature · 1 line

1.8.5.2 (Apple Git-48)

Quoted 4 lines

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Hi Daniel,

thank you for taking a look at the patch set and shepherding this through the review process.

Quoted 8 lines · Daniel Vetter

Quoted 7 lines · Lukas Wunner

Don't lock vgasr_mutex in _lock_ddc() / _unlock_ddc(), it can cause deadlocks because (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.

Quoted 7 lines

Quoted 4 lines

Instead, lock ddc_lock when unregistering the handler because the only reason why we'd want to lock vgasr_mutex in _lock_ddc() / _unlock_ddc() is to block the handler from disappearing while DDC lines are switched.

Shouldn't we also take the new look in register_handler, for consistency? I know that if you look the mux provider too late it's fairly hopeless ...

With the chosen approach that's not necessary: vga_switcheroo_lock_ddc() records in vgasr_priv.old_ddc_owner if there's no mux:

	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
		vgasr_priv.old_ddc_owner = -ENODEV;
		return -ENODEV;
	}

And vga_switcheroo_unlock_ddc() does nothing if vgasr_priv.old_ddc_owner is negative, it just releases the lock and returns:

	if (vgasr_priv.old_ddc_owner >= 0) {
		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);
	}
	mutex_unlock(&vgasr_priv.ddc_lock);

So it has no consequences if the mux registers between the calls to _lock_ddc() and _unlock_ddc().

Quoted 6 lines

Also while reading the patch I realized that the new lock really protects hw state (instead of our software-side tracking and driver resume/suspend code). And at least myself I have no idea what vgasr means, so what about renaming it to hw_mutex? We have this pattern of low-level locks to avoid concurrent hw access in a lot of places like i2c, dp_aux, and it's very often called hw_lock or similar.

Hm, hw_lock sounds a bit ambiguous.

vgasr_mutex is really a catch-all that protects access to the vgasr_priv structure and also protects various hardware state. (Power state of each GPU, mux state.) Up until now this approach worked fine, it looks like there was no need for additional locks. We may need to move to more fine-grained locking as new features get added to vga_switcheroo. The newly introduced ddc_lock is a first step and I think is aptly named since it only protects the mux state for the DDC lines.

Quoted 4 lines

Wrt the overall patch series, can you pls haggle driver-maintainers (gmux, radeon, nouveau) for acks/reviews? scripts/get_maintainers.pl should help.

Will do.

Quoted 2 lines

Also, the revised docbook patch seems to be missing from this iteration, please follow up with that one.

The docbook patch posted September 17 automatically picks up the kernel-doc for the new functions through the !E directive.

However I used markdown syntax for the unsorted lists in the DOC sections, so it will look a bit funny unless markdown gets merged, which as we all know is contentious. (Which is sad, though I can understand Jonathan Corbet's concerns.)

By the way, Jani has kindly provided a Reviewed-By: for patch 6 of the vga_switcheroo docs series. The patch is not dependent on the preceding patch 5 which is awaiting an ack from Takashi, so could be merged now:
[PATCH 06/15] vga_switcheroo: Use enum vga_switcheroo_state instead

Patches 7 and 8 are similarly trivial cleanups:
[PATCH 07/15] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead [PATCH 08/15] vga_switcheroo: Use enum vga_switcheroo_client_id

And patch 10 has gotten a Reviewed-By: from Takashi:
[PATCH 10/15] ALSA: hda - Spell vga_switcheroo consistently

Quoted 10 lines · Lukas Wunner

Quoted 6 lines · Daniel Vetter

Also while reading the patch I realized that the new lock really protects hw state (instead of our software-side tracking and driver resume/suspend code). And at least myself I have no idea what vgasr means, so what about renaming it to hw_mutex? We have this pattern of low-level locks to avoid concurrent hw access in a lot of places like i2c, dp_aux, and it's very often called hw_lock or similar.

Hm, hw_lock sounds a bit ambiguous.

Quoted 7 lines

vgasr_mutex is really a catch-all that protects access to the vgasr_priv structure and also protects various hardware state. (Power state of each GPU, mux state.) Up until now this approach worked fine, it looks like there was no need for additional locks. We may need to move to more fine-grained locking as new features get added to vga_switcheroo. The newly introduced ddc_lock is a first step and I think is aptly named since it only protects the mux state for the DDC lines.

Oops sorry mixed up the names. I meant rename the ddc_lock to hw_lock since it protects a bit more than just the ddc (it protects the entire hw switch state since we grab it both for dcc switching and for full-blown switching of everything).

Quoted 5 lines

Quoted 2 lines

Also, the revised docbook patch seems to be missing from this iteration, please follow up with that one.

The docbook patch posted September 17 automatically picks up the kernel-doc for the new functions through the !E directive.

I asked for the inclusion to be moved to drm.tmpl instead of creating a new docbook.

Quoted 10 lines

By the way, Jani has kindly provided a Reviewed-By: for patch 6 of the vga_switcheroo docs series. The patch is not dependent on the preceding patch 5 which is awaiting an ack from Takashi, so could be merged now:
[PATCH 06/15] vga_switcheroo: Use enum vga_switcheroo_state instead

Patches 7 and 8 are similarly trivial cleanups:
[PATCH 07/15] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead [PATCH 08/15] vga_switcheroo: Use enum vga_switcheroo_client_id

Quoted 2 lines

And patch 10 has gotten a Reviewed-By: from Takashi:
[PATCH 10/15] ALSA: hda - Spell vga_switcheroo consistently

Hm those patches aren't in this series, that makes it really hard for me to know what's still pending. I think it would be best to resend _everything_, so including docs and cleanups and all that. Otherwise I think this will take a lot longer than necessary to pull in.

Originally by Seth Forshee <[email protected]>, 2012-10-04:
The gmux allows muxing the DDC independently from the display, so support this functionality. This will allow reading the EDID for the inactive GPU, fixing issues with machines that either don't have a VBT or have invalid mode data in the VBT.

Modified by Lukas Wunner <[email protected]>, 2015-10-07:

    Advertise ->switch_ddc handler callback only on the pre-retina
    Macbook Pro. The retina uses eDP and its mux is incapable of
    switching the AUX channel separately from the main link.
    It's an NXP CBTL06142 (alternate parts: TI HD3SS212,
    Pericom PPI3VDP12412).

    Sources:
    http://www.electronicproducts.com/-whatsinside_text-145.aspx
    http://slideshare.net/jjwu6266/apple-2012-wwdc-apple-macbook-pro-with-retina-display
    http://www.techrepublic.com/blog/cracking-open/teardown-shows-retina-macbook-pro-is-nearly-impossible-to-upgrade-difficult-to-work-on/

    Datasheets:
    http://www.nxp.com/documents/data_sheet/CBTL06141.pdf
    http://www.ti.com/lit/ds/symlink/hd3ss212.pdf
    https://www.pericom.com/assets/Datasheets/PI3VDP12412.pdf

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115

Tested-by Lukas Wunner <[email protected]>

    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Cc Seth Forshee <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/platform/x86/apple-gmux.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

drivers/platform/x86/apple-gmux.c +0 -0

index 0dec3f5..78997b7 100644
@@ -276,11 +276,9 @@ static const struct backlight_ops gmux_bl_ops = {
 static int gmux_switchto(enum vga_switcheroo_client_id id)

drivers/platform/x86/apple-gmux.c +19 -1

@@ -288,6 +286,18 @@ static int gmux_switchto(enum vga_switcheroo_client_id id)
        return 0;
 }
 
+static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
+{
+       pr_debug("Switching gmux DDC to %d\n", id);
+
+       if (id == VGA_SWITCHEROO_IGD)
+               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
+       else
+               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
+
+       return 0;
+}
+
 static int gmux_set_discrete_state(struct apple_gmux_data *gmux_data,
                                   enum vga_switcheroo_state state)
 {

@@ -588,6 +598,13 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
                gmux_data->gpe = -1;
        }
 
+       /*
+        * The gmux in pre-retina MacBook Pros can switch DDC separately
+        * from the other pins of the outputs. It's distinguishable from
+        * the gmux in retinas by being non-indexed.
+        */
+       if (!gmux_data->indexed)
+               gmux_handler.switch_ddc = gmux_switch_ddc;
        if (vga_switcheroo_register_handler(&gmux_handler)) {
                ret = -ENODEV;
                goto err_register_handler;
--

1.8.5.2 (Apple Git-48)

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 46 lines · Lukas Wunner

Originally by Seth Forshee <[email protected]>, 2012-10-04:
The gmux allows muxing the DDC independently from the display, so support this functionality. This will allow reading the EDID for the inactive GPU, fixing issues with machines that either don't have a VBT or have invalid mode data in the VBT.

Modified by Lukas Wunner <[email protected]>, 2015-10-07:

    Advertise ->switch_ddc handler callback only on the pre-retina
    Macbook Pro. The retina uses eDP and its mux is incapable of
    switching the AUX channel separately from the main link.
    It's an NXP CBTL06142 (alternate parts: TI HD3SS212,
    Pericom PPI3VDP12412).

    Sources:
    http://www.electronicproducts.com/-whatsinside_text-145.aspx
    http://slideshare.net/jjwu6266/apple-2012-wwdc-apple-macbook-pro-with-retina-display
    http://www.techrepublic.com/blog/cracking-open/teardown-shows-retina-macbook-pro-is-nearly-impossible-to-upgrade-difficult-to-work-on/

    Datasheets:
    http://www.nxp.com/documents/data_sheet/CBTL06141.pdf
    http://www.ti.com/lit/ds/symlink/hd3ss212.pdf
    https://www.pericom.com/assets/Datasheets/PI3VDP12412.pdf

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115

Tested-by Lukas Wunner <[email protected]>

    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Cc Seth Forshee <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/platform/x86/apple-gmux.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

drivers/platform/x86/apple-gmux.c +0 -1

index 0dec3f5..78997b7 100644
@@ -276,11 +276,9 @@ static const struct backlight_ops gmux_bl_ops = {
 static int gmux_switchto(enum vga_switcheroo_client_id id)
 {
        if (id == VGA_SWITCHEROO_IGD) {
-               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
                gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 2);
                gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 2);
        } else {

drivers/platform/x86/apple-gmux.c static const struct backlight_ops gmux_bl_ops = { · line 284

Quoted 4 lines

drivers/platform/x86/apple-gmux.c +0 -1

-               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
                gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 3);
                gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3);
        }

Won't this hunk break manual switching until the later patches land? Seems like you might want to break this out as a separate patch later in the series.

Alex

Quoted 36 lines

drivers/platform/x86/apple-gmux.c +19 -1

@@ -288,6 +286,18 @@ static int gmux_switchto(enum vga_switcheroo_client_id id)
        return 0;
 }
 
+static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
+{
+       pr_debug("Switching gmux DDC to %d\n", id);
+
+       if (id == VGA_SWITCHEROO_IGD)
+               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
+       else
+               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
+
+       return 0;
+}
+
 static int gmux_set_discrete_state(struct apple_gmux_data *gmux_data,
                                   enum vga_switcheroo_state state)
 {

@@ -588,6 +598,13 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
                gmux_data->gpe = -1;
        }
 
+       /*
+        * The gmux in pre-retina MacBook Pros can switch DDC separately
+        * from the other pins of the outputs. It's distinguishable from
+        * the gmux in retinas by being non-indexed.
+        */
+       if (!gmux_data->indexed)
+               gmux_handler.switch_ddc = gmux_switch_ddc;
        if (vga_switcheroo_register_handler(&gmux_handler)) {
                ret = -ENODEV;
                goto err_register_handler;
--

1.8.5.2 (Apple Git-48)

Quoted 4 lines

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 47 lines · Alex Deucher

Quoted 46 lines · Lukas Wunner

Originally by Seth Forshee <[email protected]>, 2012-10-04:
The gmux allows muxing the DDC independently from the display, so support this functionality. This will allow reading the EDID for the inactive GPU, fixing issues with machines that either don't have a VBT or have invalid mode data in the VBT.

Modified by Lukas Wunner <[email protected]>, 2015-10-07:

    Advertise ->switch_ddc handler callback only on the pre-retina
    Macbook Pro. The retina uses eDP and its mux is incapable of
    switching the AUX channel separately from the main link.
    It's an NXP CBTL06142 (alternate parts: TI HD3SS212,
    Pericom PPI3VDP12412).

    Sources:
    http://www.electronicproducts.com/-whatsinside_text-145.aspx
    http://slideshare.net/jjwu6266/apple-2012-wwdc-apple-macbook-pro-with-retina-display
    http://www.techrepublic.com/blog/cracking-open/teardown-shows-retina-macbook-pro-is-nearly-impossible-to-upgrade-difficult-to-work-on/

    Datasheets:
    http://www.nxp.com/documents/data_sheet/CBTL06141.pdf
    http://www.ti.com/lit/ds/symlink/hd3ss212.pdf
    https://www.pericom.com/assets/Datasheets/PI3VDP12412.pdf

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115

Tested-by Lukas Wunner <[email protected]>

    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Cc Seth Forshee <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/platform/x86/apple-gmux.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

drivers/platform/x86/apple-gmux.c +0 -1

index 0dec3f5..78997b7 100644
@@ -276,11 +276,9 @@ static const struct backlight_ops gmux_bl_ops = {
 static int gmux_switchto(enum vga_switcheroo_client_id id)
 {
        if (id == VGA_SWITCHEROO_IGD) {
-               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
                gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 2);
                gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 2);
        } else {

Quoted 8 lines

drivers/platform/x86/apple-gmux.c static const struct backlight_ops gmux_bl_ops = { · line 284

Quoted 4 lines

drivers/platform/x86/apple-gmux.c +0 -1

-               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
                gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 3);
                gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3);
        }

Won't this hunk break manual switching until the later patches land? Seems like you might want to break this out as a separate patch later in the series.

Also. I'm not sure how the gmux works, but this might break ddc on external displays that are muxed.

Quoted 47 lines · Alex Deucher

Quoted 46 lines · Lukas Wunner

Originally by Seth Forshee <[email protected]>, 2012-10-04:
The gmux allows muxing the DDC independently from the display, so support this functionality. This will allow reading the EDID for the inactive GPU, fixing issues with machines that either don't have a VBT or have invalid mode data in the VBT.

Modified by Lukas Wunner <[email protected]>, 2015-10-07:

    Advertise ->switch_ddc handler callback only on the pre-retina
    Macbook Pro. The retina uses eDP and its mux is incapable of
    switching the AUX channel separately from the main link.
    It's an NXP CBTL06142 (alternate parts: TI HD3SS212,
    Pericom PPI3VDP12412).

    Sources:
    http://www.electronicproducts.com/-whatsinside_text-145.aspx
    http://slideshare.net/jjwu6266/apple-2012-wwdc-apple-macbook-pro-with-retina-display
    http://www.techrepublic.com/blog/cracking-open/teardown-shows-retina-macbook-pro-is-nearly-impossible-to-upgrade-difficult-to-work-on/

    Datasheets:
    http://www.nxp.com/documents/data_sheet/CBTL06141.pdf
    http://www.ti.com/lit/ds/symlink/hd3ss212.pdf
    https://www.pericom.com/assets/Datasheets/PI3VDP12412.pdf

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115

Tested-by Lukas Wunner <[email protected]>

    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Cc Seth Forshee <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/platform/x86/apple-gmux.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

drivers/platform/x86/apple-gmux.c +0 -1

index 0dec3f5..78997b7 100644
@@ -276,11 +276,9 @@ static const struct backlight_ops gmux_bl_ops = {
 static int gmux_switchto(enum vga_switcheroo_client_id id)
 {
        if (id == VGA_SWITCHEROO_IGD) {
-               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
                gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 2);
                gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 2);
        } else {

Quoted 8 lines

drivers/platform/x86/apple-gmux.c static const struct backlight_ops gmux_bl_ops = { · line 284

Quoted 4 lines

drivers/platform/x86/apple-gmux.c +0 -1

-               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
                gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 3);
                gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3);
        }

Won't this hunk break manual switching until the later patches land? Seems like you might want to break this out as a separate patch later in the series.

Also. I'm not sure how the gmux works, but this might break ddc on external displays that are muxed.

Alex

Quoted 40 lines

Alex

Quoted 36 lines

drivers/platform/x86/apple-gmux.c +19 -1

@@ -288,6 +286,18 @@ static int gmux_switchto(enum vga_switcheroo_client_id id)
        return 0;
 }
 
+static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
+{
+       pr_debug("Switching gmux DDC to %d\n", id);
+
+       if (id == VGA_SWITCHEROO_IGD)
+               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
+       else
+               gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
+
+       return 0;
+}
+
 static int gmux_set_discrete_state(struct apple_gmux_data *gmux_data,
                                   enum vga_switcheroo_state state)
 {

@@ -588,6 +598,13 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
                gmux_data->gpe = -1;
        }
 
+       /*
+        * The gmux in pre-retina MacBook Pros can switch DDC separately
+        * from the other pins of the outputs. It's distinguishable from
+        * the gmux in retinas by being non-indexed.
+        */
+       if (!gmux_data->indexed)
+               gmux_handler.switch_ddc = gmux_switch_ddc;
        if (vga_switcheroo_register_handler(&gmux_handler)) {
                ret = -ENODEV;
                goto err_register_handler;
--

1.8.5.2 (Apple Git-48)

Quoted 4 lines

Quoted 4 lines

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Originally by Seth Forshee <[email protected]>, 2012-10-04:
The gmux allows muxing the DDC independently from the display, so support this functionality. This will allow reading the EDID for the inactive GPU, fixing issues with machines that either don't have a VBT or have invalid mode data in the VBT.

Modified by Lukas Wunner <[email protected]>, 2015-10-07:

    Advertise ->switch_ddc handler callback only on the pre-retina
    Macbook Pro. The retina uses eDP and its mux is incapable of
    switching the AUX channel separately from the main link.
    It's an NXP CBTL06142 (alternate parts: TI HD3SS212,
    Pericom PPI3VDP12412).

    Sources:
    http://www.electronicproducts.com/-whatsinside_text-145.aspx
    http://slideshare.net/jjwu6266/apple-2012-wwdc-apple-macbook-pro-with-retina-display
    http://www.techrepublic.com/blog/cracking-open/teardown-shows-retina-macbook-pro-is-nearly-impossible-to-upgrade-difficult-to-work-on/

    Datasheets:
    http://www.nxp.com/documents/data_sheet/CBTL06141.pdf
    http://www.ti.com/lit/ds/symlink/hd3ss212.pdf
    https://www.pericom.com/assets/Datasheets/PI3VDP12412.pdf

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115

Tested-by Lukas Wunner <[email protected]>

    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Cc Seth Forshee <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

Quoted 23 lines · Lukas Wunner

Originally by Seth Forshee <[email protected]>, 2012-10-04:
The gmux allows muxing the DDC independently from the display, so support this functionality. This will allow reading the EDID for the inactive GPU, fixing issues with machines that either don't have a VBT or have invalid mode data in the VBT.

Modified by Lukas Wunner <[email protected]>, 2015-10-07:

    Advertise ->switch_ddc handler callback only on the pre-retina
    Macbook Pro. The retina uses eDP and its mux is incapable of
    switching the AUX channel separately from the main link.
    It's an NXP CBTL06142 (alternate parts: TI HD3SS212,
    Pericom PPI3VDP12412).

    Sources:
    http://www.electronicproducts.com/-whatsinside_text-145.aspx
    http://slideshare.net/jjwu6266/apple-2012-wwdc-apple-macbook-pro-with-retina-display
    http://www.techrepublic.com/blog/cracking-open/teardown-shows-retina-macbook-pro-is-nearly-impossible-to-upgrade-difficult-to-work-on/

    Datasheets:
    http://www.nxp.com/documents/data_sheet/CBTL06141.pdf
    http://www.ti.com/lit/ds/symlink/hd3ss212.pdf
    https://www.pericom.com/assets/Datasheets/PI3VDP12412.pdf

Quoted 7 lines

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115

Tested-by Lukas Wunner <[email protected]>

    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Cc Seth Forshee <[email protected]>

Signed-off-by Lukas Wunner <[email protected]>

For platform/drivers/x86:

Acked-by Darren Hart <[email protected]>

Daniel, I presume this will ultimately get pulled in by you with the rest of the series?

Hi Daniel,

Quoted 19 lines · Daniel Vetter

Quoted 10 lines · Lukas Wunner

Quoted 6 lines · Daniel Vetter

Also while reading the patch I realized that the new lock really protects hw state (instead of our software-side tracking and driver resume/suspend code). And at least myself I have no idea what vgasr means, so what about renaming it to hw_mutex? We have this pattern of low-level locks to avoid concurrent hw access in a lot of places like i2c, dp_aux, and it's very often called hw_lock or similar.

Hm, hw_lock sounds a bit ambiguous.

Quoted 7 lines

vgasr_mutex is really a catch-all that protects access to the vgasr_priv structure and also protects various hardware state. (Power state of each GPU, mux state.) Up until now this approach worked fine, it looks like there was no need for additional locks. We may need to move to more fine-grained locking as new features get added to vga_switcheroo. The newly introduced ddc_lock is a first step and I think is aptly named since it only protects the mux state for the DDC lines.

Quoted 4 lines

Oops sorry mixed up the names. I meant rename the ddc_lock to hw_lock since it protects a bit more than just the ddc (it protects the entire hw switch state since we grab it both for dcc switching and for full-blown switching of everything).

Interesting observation. Actually the intention is to use ddc_lock to only lock hardware state of the DDC switch, but you're right, it also locks hardware state of the panel switch. That's an unintended consequence of the ->switchto callback in apple-gmux also switching DDC, and the need to avoid races because of this.

Now that you mention it I'm contemplating removing DDC switching from the ->switchto callback in apple-gmux. Then I don't need to take the ddc_lock when switching the full panel.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

Also, the revised docbook patch seems to be missing from this iteration, please follow up with that one.

The docbook patch posted September 17 automatically picks up the kernel-doc for the new functions through the !E directive.

I asked for the inclusion to be moved to drm.tmpl instead of creating a new docbook.

Your argument was that including it in drm.tmpl will increase the chances it's read. Quite honestly I think that reasoning is a bit thin. One might as well argue that people won't find the information in the juggernaut of 180 kByte XML text that is drm.tmpl.

The (honest) question is this, vga_switcheroo is not located in the DRM tree, so it's a separate subsystem. Then why should someone be looking for its documentation in the DRM DocBook?

Quoted 10 lines

Quoted 10 lines

By the way, Jani has kindly provided a Reviewed-By: for patch 6 of the vga_switcheroo docs series. The patch is not dependent on the preceding patch 5 which is awaiting an ack from Takashi, so could be merged now:
[PATCH 06/15] vga_switcheroo: Use enum vga_switcheroo_state instead

Patches 7 and 8 are similarly trivial cleanups:
[PATCH 07/15] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead [PATCH 08/15] vga_switcheroo: Use enum vga_switcheroo_client_id

Quoted 7 lines

Quoted 2 lines

And patch 10 has gotten a Reviewed-By: from Takashi:
[PATCH 10/15] ALSA: hda - Spell vga_switcheroo consistently

Hm those patches aren't in this series, that makes it really hard for me to know what's still pending. I think it would be best to resend _everything_, so including docs and cleanups and all that. Otherwise I think this will take a lot longer than necessary to pull in.

I updated my vga_switcheroo_docs branch on GitHub as the Reviewed-Bys came in and just rebased it to current topic/drm-misc, so you can pull from there if you're comfortable with that:
https://github.com/l1k/linux/commits/vga_switcheroo_docs

If on the other hand you prefer merging stuff via the mailing list, I'll be happy to resend.

Signed-off-by Lukas Wunner <[email protected]>

Reviewed-by Jani Nikula <[email protected]>

---
 drivers/gpu/vga/vga_switcheroo.c | 6 +++---
 include/linux/vga_switcheroo.h   | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

drivers/gpu/vga/vga_switcheroo.c +3 -3

index 1acbe20..a7870d2 100644
@@ -100,7 +100,7 @@
 struct vga_switcheroo_client {
        struct pci_dev *pdev;
        struct fb_info *fb_info;
-       int pwr_state;
+       enum vga_switcheroo_state pwr_state;
        const struct vga_switcheroo_client_ops *ops;
        int id;
        bool active;

@@ -344,7 +344,7 @@ find_active_client(struct list_head *head)
  *
  * Return: Power state.
  */
-int vga_switcheroo_get_client_state(struct pci_dev *pdev)
+enum vga_switcheroo_state vga_switcheroo_get_client_state(struct pci_dev *pdev)
 {
        struct vga_switcheroo_client *client;
        enum vga_switcheroo_state ret;

@@ -496,7 +496,7 @@ static int vga_switchoff(struct vga_switcheroo_client *client)
        return 0;
 }
 
-static void set_audio_state(int id, int state)
+static void set_audio_state(int id, enum vga_switcheroo_state state)
 {
        struct vga_switcheroo_client *client;
 

include/linux/vga_switcheroo.h +2 -3

index 3764991..e636617 100644
@@ -138,7 +138,7 @@ void vga_switcheroo_unregister_handler(void);
 
 int vga_switcheroo_process_delayed_switch(void);
 
-int vga_switcheroo_get_client_state(struct pci_dev *dev);
+enum vga_switcheroo_state vga_switcheroo_get_client_state(struct pci_dev *dev);
 
 void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic);
 

@@ -157,7 +157,7 @@ static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
        int id) { return 0; }
 static inline void vga_switcheroo_unregister_handler(void) {}
 static inline int vga_switcheroo_process_delayed_switch(void) { return 0; }
-static inline int vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
+static inline enum vga_switcheroo_state vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
 
 static inline void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic) {}
 
--

1.8.5.2 (Apple Git-48)

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/vga/vga_switcheroo.c | 17 +++++++++--------
 include/linux/vga_switcheroo.h   |  4 ++++
 2 files changed, 13 insertions(+), 8 deletions(-)

drivers/gpu/vga/vga_switcheroo.c +9 -8

index a7870d2..9896305 100644
@@ -84,9 +84,9 @@
  * @fb_info: framebuffer to which console is remapped on switching
  * @pwr_state: current power state
  * @ops: client callbacks
- * @id: client identifier, see enum vga_switcheroo_client_id.
- *     Determining the id requires the handler, so GPUs are initially
- *     assigned -1 and later given their true id in vga_switcheroo_enable()
+ * @id: client identifier. Determining the id requires the handler,
+ *     so gpus are initially assigned VGA_SWITCHEROO_UNKNOWN_ID
+ *     and later given their true id in vga_switcheroo_enable()
  * @active: whether the outputs are currently switched to this client
  * @driver_power_control: whether power state is controlled by the driver's
  *     runtime pm. If true, writing ON and OFF to the vga_switcheroo debugfs

@@ -145,7 +145,8 @@ struct vgasr_priv {
 
 #define ID_BIT_AUDIO           0x100
 #define client_is_audio(c)     ((c)->id & ID_BIT_AUDIO)
-#define client_is_vga(c)       ((c)->id == -1 || !client_is_audio(c))
+#define client_is_vga(c)       ((c)->id == VGA_SWITCHEROO_UNKNOWN_ID || \
+                                !client_is_audio(c))
 #define client_id(c)           ((c)->id & ~ID_BIT_AUDIO)
 
 static int vga_switcheroo_debugfs_init(struct vgasr_priv *priv);

@@ -173,7 +174,7 @@ static void vga_switcheroo_enable(void)
                vgasr_priv.handler->init();
 
        list_for_each_entry(client, &vgasr_priv.clients, list) {
-               if (client->id != -1)
+               if (client->id != VGA_SWITCHEROO_UNKNOWN_ID)
                        continue;
                ret = vgasr_priv.handler->get_client_id(client->pdev);
                if (ret < 0)

@@ -277,7 +278,7 @@ int vga_switcheroo_register_client(struct pci_dev *pdev,
                                   const struct vga_switcheroo_client_ops *ops,
                                   bool driver_power_control)
 {
-       return register_client(pdev, ops, -1,
+       return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID,
                               pdev == vga_default_device(),
                               driver_power_control);
 }

@@ -583,7 +584,7 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
        int ret;
        bool delay = false, can_switch;
        bool just_mux = false;
-       int client_id = -1;
+       int client_id = VGA_SWITCHEROO_UNKNOWN_ID;
        struct vga_switcheroo_client *client = NULL;
 
        if (cnt > 63)

@@ -652,7 +653,7 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
                client_id = VGA_SWITCHEROO_DIS;
        }
 
-       if (client_id == -1)
+       if (client_id == VGA_SWITCHEROO_UNKNOWN_ID)
                goto out;
        client = find_client_from_id(&vgasr_priv.clients, client_id);
        if (!client)

include/linux/vga_switcheroo.h +4 -1

index e636617..88909a8 100644
@@ -59,6 +59,9 @@ enum vga_switcheroo_state {
 
 /**
  * enum vga_switcheroo_client_id - client identifier
+ * @VGA_SWITCHEROO_UNKNOWN_ID: initial identifier assigned to vga clients.
+ *     Determining the id requires the handler, so GPUs are given their
+ *     true id in a delayed fashion in vga_switcheroo_enable()
  * @VGA_SWITCHEROO_IGD: integrated graphics device
  * @VGA_SWITCHEROO_DIS: discrete graphics device
  * @VGA_SWITCHEROO_MAX_CLIENTS: currently no more than two GPUs are supported

@@ -66,6 +69,7 @@ enum vga_switcheroo_state {
  * Client identifier. Audio clients use the same identifier & 0x100.
  */
 enum vga_switcheroo_client_id {
+       VGA_SWITCHEROO_UNKNOWN_ID = -1,
        VGA_SWITCHEROO_IGD,
        VGA_SWITCHEROO_DIS,
        VGA_SWITCHEROO_MAX_CLIENTS,
--

1.8.5.2 (Apple Git-48)

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 1 line · Lukas Wunner

Signed-off-by Lukas Wunner <[email protected]>

Reviewed-by Alex Deucher <[email protected]>

Quoted 98 lines

---
 drivers/gpu/vga/vga_switcheroo.c | 17 +++++++++--------
 include/linux/vga_switcheroo.h   |  4 ++++
 2 files changed, 13 insertions(+), 8 deletions(-)

drivers/gpu/vga/vga_switcheroo.c +9 -8

index a7870d2..9896305 100644
@@ -84,9 +84,9 @@
  * @fb_info: framebuffer to which console is remapped on switching
  * @pwr_state: current power state
  * @ops: client callbacks
- * @id: client identifier, see enum vga_switcheroo_client_id.
- *     Determining the id requires the handler, so GPUs are initially
- *     assigned -1 and later given their true id in vga_switcheroo_enable()
+ * @id: client identifier. Determining the id requires the handler,
+ *     so gpus are initially assigned VGA_SWITCHEROO_UNKNOWN_ID
+ *     and later given their true id in vga_switcheroo_enable()
  * @active: whether the outputs are currently switched to this client
  * @driver_power_control: whether power state is controlled by the driver's
  *     runtime pm. If true, writing ON and OFF to the vga_switcheroo debugfs

@@ -145,7 +145,8 @@ struct vgasr_priv {
 
 #define ID_BIT_AUDIO           0x100
 #define client_is_audio(c)     ((c)->id & ID_BIT_AUDIO)
-#define client_is_vga(c)       ((c)->id == -1 || !client_is_audio(c))
+#define client_is_vga(c)       ((c)->id == VGA_SWITCHEROO_UNKNOWN_ID || \
+                                !client_is_audio(c))
 #define client_id(c)           ((c)->id & ~ID_BIT_AUDIO)
 
 static int vga_switcheroo_debugfs_init(struct vgasr_priv *priv);

@@ -173,7 +174,7 @@ static void vga_switcheroo_enable(void)
                vgasr_priv.handler->init();
 
        list_for_each_entry(client, &vgasr_priv.clients, list) {
-               if (client->id != -1)
+               if (client->id != VGA_SWITCHEROO_UNKNOWN_ID)
                        continue;
                ret = vgasr_priv.handler->get_client_id(client->pdev);
                if (ret < 0)

@@ -277,7 +278,7 @@ int vga_switcheroo_register_client(struct pci_dev *pdev,
                                   const struct vga_switcheroo_client_ops *ops,
                                   bool driver_power_control)
 {
-       return register_client(pdev, ops, -1,
+       return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID,
                               pdev == vga_default_device(),
                               driver_power_control);
 }

@@ -583,7 +584,7 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
        int ret;
        bool delay = false, can_switch;
        bool just_mux = false;
-       int client_id = -1;
+       int client_id = VGA_SWITCHEROO_UNKNOWN_ID;
        struct vga_switcheroo_client *client = NULL;
 
        if (cnt > 63)

@@ -652,7 +653,7 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
                client_id = VGA_SWITCHEROO_DIS;
        }
 
-       if (client_id == -1)
+       if (client_id == VGA_SWITCHEROO_UNKNOWN_ID)
                goto out;
        client = find_client_from_id(&vgasr_priv.clients, client_id);
        if (!client)

include/linux/vga_switcheroo.h +4 -1

index e636617..88909a8 100644
@@ -59,6 +59,9 @@ enum vga_switcheroo_state {
 
 /**
  * enum vga_switcheroo_client_id - client identifier
+ * @VGA_SWITCHEROO_UNKNOWN_ID: initial identifier assigned to vga clients.
+ *     Determining the id requires the handler, so GPUs are given their
+ *     true id in a delayed fashion in vga_switcheroo_enable()
  * @VGA_SWITCHEROO_IGD: integrated graphics device
  * @VGA_SWITCHEROO_DIS: discrete graphics device
  * @VGA_SWITCHEROO_MAX_CLIENTS: currently no more than two GPUs are supported

@@ -66,6 +69,7 @@ enum vga_switcheroo_state {
  * Client identifier. Audio clients use the same identifier & 0x100.
  */
 enum vga_switcheroo_client_id {
+       VGA_SWITCHEROO_UNKNOWN_ID = -1,
        VGA_SWITCHEROO_IGD,
        VGA_SWITCHEROO_DIS,
        VGA_SWITCHEROO_MAX_CLIENTS,
--

1.8.5.2 (Apple Git-48)

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 3 lines · Alex Deucher

Quoted 1 line · Lukas Wunner

Signed-off-by Lukas Wunner <[email protected]>

Reviewed-by Alex Deucher <[email protected]>

Merged the 3 cleanup patches to drm-misc.
-Daniel

Hi Daniel,

Quoted 7 lines · Daniel Vetter

Quoted 3 lines · Alex Deucher

Quoted 1 line · Lukas Wunner

Signed-off-by Lukas Wunner <[email protected]>

Reviewed-by Alex Deucher <[email protected]>

Merged the 3 cleanup patches to drm-misc.
-Daniel

Thanks a lot. Could you also merge the fourth patch:

Subject: [PATCH 4/9] ALSA: hda - Spell vga_switcheroo consistently
Message-Id: <9b0175319ce78d831acfcf11e4c6c760f826b0e3.1444663039.git.lukas@wunner.de>
Date: Fri, 4 Sep 2015 20:49:36 +0200

Takashi gave it a Reviewed-by tag (which I've included in the above resend) and asked for it to be merged via drm-misc in this off-list e-mail:

Subject: Re: [PATCH 10/15] ALSA: hda - Spell vga_switcheroo consistently (fwd)
Message-ID: <[email protected]>
Date: Wed, 30 Sep 2015 20:46:08 +0200

Thanks & best regards,

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/vga/vga_switcheroo.c | 17 ++++++++++-------
 include/linux/vga_switcheroo.h   |  6 +++---
 2 files changed, 13 insertions(+), 10 deletions(-)

drivers/gpu/vga/vga_switcheroo.c +10 -7

index 9896305..af0d372 100644
@@ -102,7 +102,7 @@ struct vga_switcheroo_client {
        struct fb_info *fb_info;
        enum vga_switcheroo_state pwr_state;
        const struct vga_switcheroo_client_ops *ops;
-       int id;
+       enum vga_switcheroo_client_id id;
        bool active;
        bool driver_power_control;
        struct list_head list;

@@ -233,7 +233,8 @@ EXPORT_SYMBOL(vga_switcheroo_unregister_handler);
 
 static int register_client(struct pci_dev *pdev,
                           const struct vga_switcheroo_client_ops *ops,
-                          int id, bool active, bool driver_power_control)
+                          enum vga_switcheroo_client_id id, bool active,
+                          bool driver_power_control)
 {
        struct vga_switcheroo_client *client;
 

@@ -288,7 +289,7 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
  * vga_switcheroo_register_audio_client - register audio client
  * @pdev: client pci device
  * @ops: client callbacks
- * @id: client identifier, see enum vga_switcheroo_client_id
+ * @id: client identifier
  *
  * Register audio client (audio device on a GPU). The power state of the
  * client is assumed to be ON.

@@ -297,7 +298,7 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
  */
 int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
                                         const struct vga_switcheroo_client_ops *ops,
-                                        int id)
+                                        enum vga_switcheroo_client_id id)
 {
        return register_client(pdev, ops, id | ID_BIT_AUDIO, false, false);
 }

@@ -315,7 +316,8 @@ find_client_from_pci(struct list_head *head, struct pci_dev *pdev)
 }
 
 static struct vga_switcheroo_client *
-find_client_from_id(struct list_head *head, int client_id)
+find_client_from_id(struct list_head *head,
+                   enum vga_switcheroo_client_id client_id)
 {
        struct vga_switcheroo_client *client;
 

@@ -497,7 +499,8 @@ static int vga_switchoff(struct vga_switcheroo_client *client)
        return 0;
 }
 
-static void set_audio_state(int id, enum vga_switcheroo_state state)
+static void set_audio_state(enum vga_switcheroo_client_id id,
+                           enum vga_switcheroo_state state)
 {
        struct vga_switcheroo_client *client;
 

@@ -584,7 +587,7 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
        int ret;
        bool delay = false, can_switch;
        bool just_mux = false;
-       int client_id = VGA_SWITCHEROO_UNKNOWN_ID;
+       enum vga_switcheroo_client_id client_id = VGA_SWITCHEROO_UNKNOWN_ID;
        struct vga_switcheroo_client *client = NULL;
 
        if (cnt > 63)

include/linux/vga_switcheroo.h +3 -4

index 88909a8..c557511 100644
@@ -100,7 +100,7 @@ struct vga_switcheroo_handler {
        int (*switchto)(enum vga_switcheroo_client_id id);
        int (*power_state)(enum vga_switcheroo_client_id id,
                           enum vga_switcheroo_state state);
-       int (*get_client_id)(struct pci_dev *pdev);
+       enum vga_switcheroo_client_id (*get_client_id)(struct pci_dev *pdev);
 };
 
 /**

@@ -132,7 +132,7 @@ int vga_switcheroo_register_client(struct pci_dev *dev,
                                   bool driver_power_control);
 int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
                                         const struct vga_switcheroo_client_ops *ops,
-                                        int id);
+                                        enum vga_switcheroo_client_id id);
 
 void vga_switcheroo_client_fb_set(struct pci_dev *dev,
                                  struct fb_info *info);

@@ -158,7 +158,7 @@ static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_i
 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,
-       int id) { return 0; }
+       enum vga_switcheroo_client_id id) { return 0; }
 static inline void vga_switcheroo_unregister_handler(void) {}
 static inline int vga_switcheroo_process_delayed_switch(void) { return 0; }
 static inline enum vga_switcheroo_state vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
--

1.8.5.2 (Apple Git-48)

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Commit 599bbb9de0fe ("drm/i915: i915 cannot provide switcher services.") removed all remaining vga_switcheroo symbols from intel_acpi.c but left the include. Drop it.

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/drm/i915/intel_acpi.c | 1 -
 1 file changed, 1 deletion(-)

drivers/gpu/drm/i915/intel_acpi.c +0 -1

index d96eee1..68119b6 100644
@@ -5,7 +5,6 @@
  */
 #include <linux/pci.h>
 #include <linux/acpi.h>
-#include <linux/vga_switcheroo.h>
 #include <drm/drmP.h>
 #include "i915_drv.h"
 

Signature · 1 line

1.8.5.2 (Apple Git-48)

Commit 599bbb9de0fe ("drm/i915: i915 cannot provide switcher services.") removed all remaining vga_switcheroo symbols from intel_acpi.c but left the include. Drop it.

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/drm/i915/intel_acpi.c | 1 -
 1 file changed, 1 deletion(-)

drivers/gpu/drm/i915/intel_acpi.c +0 -1

index d96eee1..68119b6 100644
@@ -5,7 +5,6 @@
  */
 #include <linux/pci.h>
 #include <linux/acpi.h>
-#include <linux/vga_switcheroo.h>
 #include <drm/drmP.h>
 #include "i915_drv.h"
 

Signature · 6 lines

1.8.5.2 (Apple Git-48)

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

This was added to two radeon files even though they don't use any vga_switcheroo symbols, the amdgpu fork inherited them:

Added to amdgpu_acpi.c by commit d7a2952f1ade ("drm/radeon: Add support for the ATIF ACPI method to the radeon driver").

Added to amdgpu_bios.c by commit 6a9ee8af344e ("vga_switcheroo:
initial implementation (v15)").

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 1 -
 2 files changed, 2 deletions(-)

drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c +0 -1

index aef4a7a..a142d5a 100644
@@ -25,7 +25,6 @@
 #include <linux/acpi.h>
 #include <linux/slab.h>
 #include <linux/power_supply.h>
-#include <linux/vga_switcheroo.h>
 #include <acpi/video.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>

drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c +0 -2

index 02add0a..c44c0c6 100644
@@ -29,7 +29,6 @@
 #include "amdgpu.h"
 #include "atom.h"
 
-#include <linux/vga_switcheroo.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
 /*
--

1.8.5.2 (Apple Git-48)

Introduced by f482a1419545 ("drm/radeon: document radeon_kms.c").

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/drm/radeon/radeon_kms.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/gpu/drm/radeon/radeon_kms.c +0 -0

index 4e2780f..ee5cc14 100644

drivers/gpu/drm/radeon/radeon_kms.c +1 -2

  *
  * @dev: drm dev pointer
  *
- * Switch vga switcheroo state after last close (all asics).
+ * Switch vga_switcheroo state after last close (all asics).
  */
 void radeon_driver_lastclose_kms(struct drm_device *dev)
 {
--

1.8.5.2 (Apple Git-48)

Quoted 7 lines · Lukas Wunner

Introduced by f482a1419545 ("drm/radeon: document radeon_kms.c").

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/drm/radeon/radeon_kms.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/gpu/drm/radeon/radeon_kms.c static int radeon_info_ioctl(struct drm_device *dev, void *data, struct drm_file · line 601

Quoted 10 lines

drivers/gpu/drm/radeon/radeon_kms.c +1 -1

index 4e2780f..ee5cc14 100644
@@ -598,11 +598,11 @@ static int radeon_info_ioctl(struct drm_device *dev, void *data, struct drm_file
  * Outdated mess for old drm with Xorg being in charge (void function now).
  */
 /**
- * radeon_driver_firstopen_kms - drm callback for last close
+ * radeon_driver_lastclose_kms - drm callback for last close

Already fixes as part of this patch:
http://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-fixes&id=8c70e1cda04b966b50ddfefafbd0ea376ed8edd5

Up to you if you want to respin the second hunk or not.

Quoted 11 lines

drivers/gpu/drm/radeon/radeon_kms.c +1 -2

  *
  * @dev: drm dev pointer
  *
- * Switch vga switcheroo state after last close (all asics).
+ * Switch vga_switcheroo state after last close (all asics).
  */
 void radeon_driver_lastclose_kms(struct drm_device *dev)
 {
--

1.8.5.2 (Apple Git-48)

Introduced by f482a1419545 ("drm/radeon: document radeon_kms.c") and inherited by the amdgpu fork.

Signed-off-by Lukas Wunner <[email protected]>

---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +0 -0

index 371f015..b1d499e 100644

drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +1 -2

  *
  * @dev: drm dev pointer
  *
- * Switch vga switcheroo state after last close (all asics).
+ * Switch vga_switcheroo state after last close (all asics).
  */
 void amdgpu_driver_lastclose_kms(struct drm_device *dev)
 {
--

1.8.5.2 (Apple Git-48)