[PATCH v3 0/6] Enable gpu switching on the MacBook Pro

[PATCH v3 0/6] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
to switch the panel between its two GPUs. The panel mode in VBIOS
is notoriously bogus on these machines and some models have no
VBIOS at all, so the inactive GPU cannot set up its LVDS output.

Extend vga_switcheroo to support switching only the DDC lines.
Introduce a drm_get_edid_switcheroo() helper which uses this feature.
Amend i915, nouveau and radeon to call it for LVDS connectors.

This only enables EDID probing on the pre-retina MBP (2008 - 2013),
and only under the condition that apple-gmux loads before the DRM
drivers. Later patches will address reprobing of the DRM drivers
if apple-gmux loads late.

The retina MBP (2012 - present) uses eDP and is apparently not
capable of switching AUX separately from the main link.
This will also be addressed in later patches.


Previous installments:
v1: http://lists.freedesktop.org/archives/dri-devel/2015-April/081515.html
v2: http://lists.freedesktop.org/archives/dri-devel/2015-August/088156.html


Changes since v2:
  * Previously the DDC locking happened in drm_get_edid() and thus
    was done for all DRM drivers, regardless if they are ever used
    on muxed machines. Now this is moved to a separate helper which
    is only called by relevant drivers and only for LVDS connectors.
    (Suggested by Thierry Reding and seconded by Alex Deucher and
    Daniel Vetter.)
  * Squashed commits, overhauled locking, added kernel-doc for new
    public functions and locks.
    (Suggested by Daniel Vetter.)


Thanks a lot to the reviewers and testers for your valuable feedback.


Lukas Wunner (6):
  vga_switcheroo: Add support for switching only the DDC
  apple-gmux: Add switch_ddc support
  drm/edid: Switch DDC when reading the EDID
  drm/i915: Switch DDC when reading the EDID
  drm/nouveau: Switch DDC when reading the EDID
  drm/radeon: Switch DDC when reading the EDID

 drivers/gpu/drm/drm_edid.c                  | 26 ++++++++
 drivers/gpu/drm/i915/intel_lvds.c           |  3 +-
 drivers/gpu/drm/nouveau/nouveau_connector.c | 13 +++-
 drivers/gpu/drm/radeon/radeon_connectors.c  |  4 ++
 drivers/gpu/vga/vga_switcheroo.c            | 98 ++++++++++++++++++++++++++++-
 drivers/platform/x86/apple-gmux.c           | 23 +++++++
 include/drm/drm_crtc.h                      |  2 +
 include/linux/vga_switcheroo.h              |  9 +++
 8 files changed, 173 insertions(+), 5 deletions(-)

-- 
1.8.5.2 (Apple Git-48)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 1/6] vga_switcheroo: Add support for switching only the DDC

[Lukas Wunner]

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 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 <daniel.vetter@ffwll.ch>)

v2.2: Readability improvements
    (requested by Thierry Reding <thierry.reding@gmail.com>)

v2.3: Overhaul locking once more

v2.4: Retain semantics of ->switchto handler callback to switch all pins
    (requested by Daniel Vetter <daniel.vetter@ffwll.ch>)

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Pierre Moreau <pierre.morrow@free.fr>
    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

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 | 98 +++++++++++++++++++++++++++++++++++++++-
 include/linux/vga_switcheroo.h   |  9 ++++
 2 files changed, 105 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index aa077aa..9b6946e 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -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;
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index b2a3dda..6edaacc 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -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,
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v3 2/6] apple-gmux: Add switch_ddc support

[Lukas Wunner]

Originally by Seth Forshee <seth.forshee@canonical.com>, 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 <lukas@wunner.de>, 2015-08-28:
    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() in vga_switcheroo but
    this fails if the inactive client registers before the active
    client.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Pierre Moreau <pierre.morrow@free.fr>
    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Cc: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/platform/x86/apple-gmux.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 0dec3f5..5bb231b 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -273,6 +273,28 @@ static const struct backlight_ops gmux_bl_ops = {
 	.update_status = gmux_update_status,
 };
 
+static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
+{
+	enum vga_switcheroo_client_id old_ddc_owner;
+
+	if (gmux_read8(apple_gmux_data, GMUX_PORT_SWITCH_DDC) == 1)
+		old_ddc_owner = VGA_SWITCHEROO_IGD;
+	else
+		old_ddc_owner = VGA_SWITCHEROO_DIS;
+
+	pr_debug("Switching gmux DDC from %d to %d\n", old_ddc_owner, id);
+
+	if (id == old_ddc_owner)
+		return old_ddc_owner;
+
+	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 old_ddc_owner;
+}
+
 static int gmux_switchto(enum vga_switcheroo_client_id id)
 {
 	if (id == VGA_SWITCHEROO_IGD) {
@@ -348,6 +370,7 @@ gmux_active_client(struct apple_gmux_data *gmux_data)
 
 static struct vga_switcheroo_handler gmux_handler = {
 	.switchto = gmux_switchto,
+	.switch_ddc = gmux_switch_ddc,
 	.power_state = gmux_set_power_state,
 	.get_client_id = gmux_get_client_id,
 };
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v3 3/6] drm/edid: Switch DDC when reading the EDID

[Lukas Wunner]

Originally by Seth Forshee <seth.forshee@canonical.com>, 2012-10-04:
    Some dual graphics machines support muxing the DDC separately from
    the display, so make use of this functionality when reading the EDID
    on the inactive GPU. Also serialize drm_get_edid() with a mutex to
    avoid races on the DDC mux state.

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-09-11:
    Move vga_switcheroo calls to a wrapper around drm_get_edid() which
    drivers can call on muxed machines. This avoids other drivers having
    to go through the vga_switcheroo motions even though they are never
    used on a muxed platform.
    (suggested by Thierry Reding <treding@nvidia.com>)

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Pierre Moreau <pierre.morrow@free.fr>
    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Cc: Seth Forshee <seth.forshee@canonical.com>
Cc: Dave Airlie <airlied@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/drm_edid.c | 26 ++++++++++++++++++++++++++
 include/drm/drm_crtc.h     |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index d895556..3982aa7 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -32,6 +32,7 @@
 #include <linux/hdmi.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/vga_switcheroo.h>
 #include <drm/drmP.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_displayid.h>
@@ -1389,6 +1390,31 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 EXPORT_SYMBOL(drm_get_edid);
 
 /**
+ * drm_get_edid_switcheroo - get EDID data for a vga_switcheroo output
+ * @connector: connector we're probing
+ * @adapter: I2C adapter to use for DDC
+ *
+ * Wrapper around drm_get_edid() for laptops with dual GPUs using one set of
+ * outputs. The wrapper adds the requisite vga_switcheroo calls to temporarily
+ * switch DDC to the GPU which is retrieving EDID.
+ *
+ * Return: Pointer to valid EDID or NULL if we couldn't find any.
+ */
+struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
+				     struct i2c_adapter *adapter)
+{
+	struct pci_dev *pdev = connector->dev->pdev;
+	struct edid *edid;
+
+	vga_switcheroo_lock_ddc(pdev);
+	edid = drm_get_edid(connector, adapter);
+	vga_switcheroo_unlock_ddc(pdev);
+
+	return edid;
+}
+EXPORT_SYMBOL(drm_get_edid_switcheroo);
+
+/**
  * drm_edid_duplicate - duplicate an EDID and the extensions
  * @edid: EDID to duplicate
  *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 683f142..022efe7 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1295,6 +1295,8 @@ extern void drm_property_destroy_user_blobs(struct drm_device *dev,
 extern bool drm_probe_ddc(struct i2c_adapter *adapter);
 extern struct edid *drm_get_edid(struct drm_connector *connector,
 				 struct i2c_adapter *adapter);
+extern struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
+					    struct i2c_adapter *adapter);
 extern struct edid *drm_edid_duplicate(const struct edid *edid);
 extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 extern void drm_mode_config_init(struct drm_device *dev);
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v3 4/6] drm/i915: Switch DDC when reading the EDID

[Lukas Wunner]

The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
to switch the panel between its two GPUs. The panel mode in VBIOS
is notoriously bogus on these machines and some models have no
VBIOS at all.

Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS.
This allows us to retrieve the EDID if the outputs are currently
muxed to the discrete GPU by temporarily switching the panel's DDC
lines to the integrated GPU.

This only enables EDID probing on the pre-retina MBP (2008 - 2013).
The retina MBP (2012 - present) uses eDP and gmux is apparently not
capable of switching AUX separately from the main link on these models.
This will be addressed in later patches.

List of pre-retina MBPs with dual GPUs, one of them Intel:
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina  15"]
    [MBP  6,1 2010  intel ILK + nvidia GT216  pre-retina  17"]
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
    [MBP  8,3 2011  intel SNB + amd turks     pre-retina  17"]
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/intel_lvds.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 2c2d1f0..dbc2682 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -1082,7 +1082,8 @@ void intel_lvds_init(struct drm_device *dev)
 	 * preferred mode is the right one.
 	 */
 	mutex_lock(&dev->mode_config.mutex);
-	edid = drm_get_edid(connector, intel_gmbus_get_adapter(dev_priv, pin));
+	edid = drm_get_edid_switcheroo(connector,
+				       intel_gmbus_get_adapter(dev_priv, pin));
 	if (edid) {
 		if (drm_add_edid_modes(connector, edid)) {
 			drm_mode_connector_update_edid_property(connector,
-- 
1.8.5.2 (Apple Git-48)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 5/6] drm/nouveau: Switch DDC when reading the EDID

[Lukas Wunner]

The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
to switch the panel between its two GPUs. The panel mode in VBIOS
is notoriously bogus on these machines.

Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS.
This allows us to retrieve the EDID if the outputs are currently
muxed to the other GPU by temporarily switching the panel's DDC
lines. Likewise, ask vga_switcheroo to switch DDC before probing
LVDS connectors.

This only enables EDID probing on the pre-retina MBP (2008 - 2013).
The retina MBP (2012 - present) uses eDP and gmux is apparently not
capable of switching AUX separately from the main link on these models.
This will be addressed in later patches.

List of pre-retina MBPs with dual GPUs, either or both Nvidia:
    [MBP  5,1 2008  nvidia MCP79 + G96        pre-retina  15"]
    [MBP  5,2 2009  nvidia MCP79 + G96        pre-retina  17"]
    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina  15"]
    [MBP  6,1 2010  intel ILK + nvidia GT216  pre-retina  17"]
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Pierre Moreau <pierre.morrow@free.fr>
    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 2e7cbe9..b3b0ca5 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -27,6 +27,7 @@
 #include <acpi/button.h>
 
 #include <linux/pm_runtime.h>
+#include <linux/vga_switcheroo.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_edid.h>
@@ -148,7 +149,13 @@ nouveau_connector_ddc_detect(struct drm_connector *connector)
 				break;
 		} else
 		if (nv_encoder->i2c) {
-			if (nvkm_probe_i2c(nv_encoder->i2c, 0x50))
+			int ret;
+			if (nv_encoder->dcb->type == DCB_OUTPUT_LVDS)
+				vga_switcheroo_lock_ddc(dev->pdev);
+			ret = nvkm_probe_i2c(nv_encoder->i2c, 0x50);
+			if (nv_encoder->dcb->type == DCB_OUTPUT_LVDS)
+				vga_switcheroo_unlock_ddc(dev->pdev);
+			if (ret)
 				break;
 		}
 	}
@@ -259,7 +266,9 @@ nouveau_connector_detect(struct drm_connector *connector, bool force)
 
 	nv_encoder = nouveau_connector_ddc_detect(connector);
 	if (nv_encoder && (i2c = nv_encoder->i2c) != NULL) {
-		nv_connector->edid = drm_get_edid(connector, i2c);
+		nv_connector->edid = nv_encoder->dcb->type == DCB_OUTPUT_LVDS ?
+				     drm_get_edid_switcheroo(connector, i2c) :
+				     drm_get_edid(connector, i2c);
 		drm_mode_connector_update_edid_property(connector,
 							nv_connector->edid);
 		if (!nv_connector->edid) {
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v3 6/6] drm/radeon: Switch DDC when reading the EDID

[Lukas Wunner]

The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
to switch the panel between its two GPUs. The panel mode in VBIOS
is notoriously bogus on these machines.

Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS.
This allows us to retrieve the EDID if the outputs are currently
muxed to the integrated GPU by temporarily switching the panel's
DDC lines to the discrete GPU.

This only enables EDID probing on the pre-retina MBP (2008 - 2013).
The retina MBP (2012 - present) uses eDP and gmux is apparently not
capable of switching AUX separately from the main link on these models.
This will be addressed in later patches.

List of pre-retina MBPs with dual GPUs, one of them AMD:
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
    [MBP  8,3 2011  intel SNB + amd turks     pre-retina  17"]

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/radeon/radeon_connectors.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index 5a2cafb..569f63c 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -344,6 +344,10 @@ static void radeon_connector_get_edid(struct drm_connector *connector)
 		else if (radeon_connector->ddc_bus)
 			radeon_connector->edid = drm_get_edid(&radeon_connector->base,
 							      &radeon_connector->ddc_bus->adapter);
+	} else if (connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
+		   radeon_connector->ddc_bus) {
+		radeon_connector->edid = drm_get_edid_switcheroo(&radeon_connector->base,
+								 &radeon_connector->ddc_bus->adapter);
 	} else if (radeon_connector->ddc_bus) {
 		radeon_connector->edid = drm_get_edid(&radeon_connector->base,
 						      &radeon_connector->ddc_bus->adapter);
-- 
1.8.5.2 (Apple Git-48)

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

Re: [PATCH v3 6/6] drm/radeon: Switch DDC when reading the EDID

[Alex Deucher]

On Sat, Sep 12, 2015 at 3:44 AM, Lukas Wunner <lukas@wunner.de> wrote:
> The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
> to switch the panel between its two GPUs. The panel mode in VBIOS
> is notoriously bogus on these machines.
>
> Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS.
> This allows us to retrieve the EDID if the outputs are currently
> muxed to the integrated GPU by temporarily switching the panel's
> DDC lines to the discrete GPU.
>
> This only enables EDID probing on the pre-retina MBP (2008 - 2013).
> The retina MBP (2012 - present) uses eDP and gmux is apparently not
> capable of switching AUX separately from the main link on these models.
> This will be addressed in later patches.
>
> List of pre-retina MBPs with dual GPUs, one of them AMD:
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
>     [MBP  8,3 2011  intel SNB + amd turks     pre-retina  17"]
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: William Brown <william@blackhats.net.au>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

I'm not opposed to this series, but I have a few questions.

1.  Has any of this been tested on non-Apple hybrid laptops to make
sure nothing has regressed?  I think it would be good to verify that
since there are more of them in the wild.
2.  This only does the ddc switching for LVDS.  Newer systems almost
certainly use DP (either eDP or DP to LVDS bridges).  What about those
systems?
3.  Most muxed hybrid laptops also mux external displays connectors as
well (VGA, DVI, HDMI, DP, etc.).  Do you have any plans to extend this
to those cases?
4. Some desktop environments (GNOME IIRC, but possibly KDE as well)
rely on the fact that ddc doesn't work on one of the GPUs when it's
not selected.  They don't know how to deal with muxes and can't deal
with the same port showing up as connected on two GPUs.  I suspect
this patch set may break that.  radeon has always been able to report
the panel information on hybrid laptops even when the mux is not
switched since the info is also stored in the vbios.  At the behest of
those desktop environments there is actually code in the driver to not
report the edid for the unselected gpu on hybrid laptops even though
we could report it.  I suspect this patch may break that.

Alex

> ---
>  drivers/gpu/drm/radeon/radeon_connectors.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
> index 5a2cafb..569f63c 100644
> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> @@ -344,6 +344,10 @@ static void radeon_connector_get_edid(struct drm_connector *connector)
>                 else if (radeon_connector->ddc_bus)
>                         radeon_connector->edid = drm_get_edid(&radeon_connector->base,
>                                                               &radeon_connector->ddc_bus->adapter);
> +       } else if (connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
> +                  radeon_connector->ddc_bus) {
> +               radeon_connector->edid = drm_get_edid_switcheroo(&radeon_connector->base,
> +                                                                &radeon_connector->ddc_bus->adapter);
>         } else if (radeon_connector->ddc_bus) {
>                 radeon_connector->edid = drm_get_edid(&radeon_connector->base,
>                                                       &radeon_connector->ddc_bus->adapter);
> --
> 1.8.5.2 (Apple Git-48)
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/6] drm/radeon: Switch DDC when reading the EDID

[Lukas Wunner]

Hi Alex,

On Wed, Oct 07, 2015 at 11:14:43PM -0400, Alex Deucher wrote:
> On Sat, Sep 12, 2015 at 3:44 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
> > to switch the panel between its two GPUs. The panel mode in VBIOS
> > is notoriously bogus on these machines.
> >
> > Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS.
> > This allows us to retrieve the EDID if the outputs are currently
> > muxed to the integrated GPU by temporarily switching the panel's
> > DDC lines to the discrete GPU.
> >
> > This only enables EDID probing on the pre-retina MBP (2008 - 2013).
> > The retina MBP (2012 - present) uses eDP and gmux is apparently not
> > capable of switching AUX separately from the main link on these models.
> > This will be addressed in later patches.
> >
> > List of pre-retina MBPs with dual GPUs, one of them AMD:
> >     [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
> >     [MBP  8,3 2011  intel SNB + amd turks     pre-retina  17"]
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> > Tested-by: William Brown <william@blackhats.net.au>
> >     [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
> >
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> 
> I'm not opposed to this series, but I have a few questions.
> 
> 1.  Has any of this been tested on non-Apple hybrid laptops to make
> sure nothing has regressed?  I think it would be good to verify that
> since there are more of them in the wild.

It hasn't been tested on such a machine because I don't have one and
don't know anyone who has one.

However I would be surprised if the series broke anything on non-Apple
laptops since apple-gmux is the only handler declaring a ->switch_ddc
callback. If that callback is not defined, the behaviour should be
identical to just calling drm_get_edid().

(With the only exception that EDID reads are serialized on LVDS connectors
due to the locking in vga_switcheroo. And that should be irrelevant since
laptops usually have only *one* LVDS panel.)


> 2.  This only does the ddc switching for LVDS.  Newer systems almost
> certainly use DP (either eDP or DP to LVDS bridges).  What about those
> systems?

As noted in the commit message above, the *retina* MacBook Pro uses eDP
instead of LVDS and is not capable of switching AUX separately from the
main link. I've gotten this to work by proxying the AUX communication
via the currently active GPU, but the patches for this are still in a
very experimental state. They will be submitted in a future series.

The drivers will then be amended to call drm_get_edid_switcheroo()
for eDP connectors, and drm_get_edid_switcheroo() will be amended
by the proxying code.

There will probably also be separate proxying-enabled helpers for
drm_dp_dpcd_read() and _write() which the drivers need to call for
eDP connectors (right now the proxying code is hacked directly into
those two functions).


> 3.  Most muxed hybrid laptops also mux external displays connectors as
> well (VGA, DVI, HDMI, DP, etc.).  Do you have any plans to extend this
> to those cases?

As far as the MacBook Pro is concerned, only the earliest two generations
support this (MacBookPro5 2008/09 with dual Nvidia GPUs and MacBookPro6
2010 with Intel/Nvidia). They have a single external switchable DP port.

I was thinking about hardcoding the DMIs of these few laptops in the
drivers and call drm_get_edid_switcheroo() for the DP port. But this
is not a high priority item. I don't have a clear plan yet.


> 4. Some desktop environments (GNOME IIRC, but possibly KDE as well)
> rely on the fact that ddc doesn't work on one of the GPUs when it's
> not selected.  They don't know how to deal with muxes and can't deal
> with the same port showing up as connected on two GPUs.  I suspect
> this patch set may break that.  radeon has always been able to report
> the panel information on hybrid laptops even when the mux is not
> switched since the info is also stored in the vbios.  At the behest of
> those desktop environments there is actually code in the driver to not
> report the edid for the unselected gpu on hybrid laptops even though
> we could report it.  I suspect this patch may break that.

I assume you're referring to:
http://lists.freedesktop.org/archives/dri-devel/2014-November/072032.html
https://bugzilla.opensuse.org/show_bug.cgi?id=904417

After perusing the bug report at length I'm under the impression that
it's not GNOME getting confused by two LVDS outputs, rather this seems
like a runtime pm issue since radeon.runpm=0 seemed to fix it.

Maybe gdm was triggering an ioctl() that didn't wake up the GPU?
I've briefly looked at the code and noticed that radeon_compat_ioctl()
doesn't call pm_runtime_get_sync(). Maybe the reporter of the bug had
a 32 bit GNOME installed? Or maybe the GPU failed to wake up from D0
for some reason? There's no dmesg output attached to the bug report
so it's impossible to tell.

It's unfortunate that the actual root cause was not clearly identified.
In my opinion 134857943356 should be reverted and replaced with a proper
fix. I don't understand why this got into mainline in the first place,
you've (astutely) described this solution as a hack in the above-linked
thread.

Thank you and best regards,

Lukas

> 
> Alex
> 
> > ---
> >  drivers/gpu/drm/radeon/radeon_connectors.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
> > index 5a2cafb..569f63c 100644
> > --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> > +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> > @@ -344,6 +344,10 @@ static void radeon_connector_get_edid(struct drm_connector *connector)
> >                 else if (radeon_connector->ddc_bus)
> >                         radeon_connector->edid = drm_get_edid(&radeon_connector->base,
> >                                                               &radeon_connector->ddc_bus->adapter);
> > +       } else if (connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
> > +                  radeon_connector->ddc_bus) {
> > +               radeon_connector->edid = drm_get_edid_switcheroo(&radeon_connector->base,
> > +                                                                &radeon_connector->ddc_bus->adapter);
> >         } else if (radeon_connector->ddc_bus) {
> >                 radeon_connector->edid = drm_get_edid(&radeon_connector->base,
> >                                                       &radeon_connector->ddc_bus->adapter);
> > --
> > 1.8.5.2 (Apple Git-48)
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 6/6] drm/radeon: Switch DDC when reading the EDID

[Alex Deucher]

On Thu, Oct 8, 2015 at 10:53 AM, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Alex,
>
> On Wed, Oct 07, 2015 at 11:14:43PM -0400, Alex Deucher wrote:
>> On Sat, Sep 12, 2015 at 3:44 AM, Lukas Wunner <lukas@wunner.de> wrote:
>> > The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
>> > to switch the panel between its two GPUs. The panel mode in VBIOS
>> > is notoriously bogus on these machines.
>> >
>> > Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS.
>> > This allows us to retrieve the EDID if the outputs are currently
>> > muxed to the integrated GPU by temporarily switching the panel's
>> > DDC lines to the discrete GPU.
>> >
>> > This only enables EDID probing on the pre-retina MBP (2008 - 2013).
>> > The retina MBP (2012 - present) uses eDP and gmux is apparently not
>> > capable of switching AUX separately from the main link on these models.
>> > This will be addressed in later patches.
>> >
>> > List of pre-retina MBPs with dual GPUs, one of them AMD:
>> >     [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
>> >     [MBP  8,3 2011  intel SNB + amd turks     pre-retina  17"]
>> >
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
>> > Tested-by: William Brown <william@blackhats.net.au>
>> >     [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
>> >
>> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
>>
>> I'm not opposed to this series, but I have a few questions.
>>
>> 1.  Has any of this been tested on non-Apple hybrid laptops to make
>> sure nothing has regressed?  I think it would be good to verify that
>> since there are more of them in the wild.
>
> It hasn't been tested on such a machine because I don't have one and
> don't know anyone who has one.
>
> However I would be surprised if the series broke anything on non-Apple
> laptops since apple-gmux is the only handler declaring a ->switch_ddc
> callback. If that callback is not defined, the behaviour should be
> identical to just calling drm_get_edid().

Ah, I missed that part.  There is an ATPX method to switch ddc
separately from the encoder data path.  That could probably be hooked
up as well at some point if someone wants to test it.

>
> (With the only exception that EDID reads are serialized on LVDS connectors
> due to the locking in vga_switcheroo. And that should be irrelevant since
> laptops usually have only *one* LVDS panel.)
>
>
>> 2.  This only does the ddc switching for LVDS.  Newer systems almost
>> certainly use DP (either eDP or DP to LVDS bridges).  What about those
>> systems?
>
> As noted in the commit message above, the *retina* MacBook Pro uses eDP
> instead of LVDS and is not capable of switching AUX separately from the
> main link. I've gotten this to work by proxying the AUX communication
> via the currently active GPU, but the patches for this are still in a
> very experimental state. They will be submitted in a future series.
>
> The drivers will then be amended to call drm_get_edid_switcheroo()
> for eDP connectors, and drm_get_edid_switcheroo() will be amended
> by the proxying code.
>
> There will probably also be separate proxying-enabled helpers for
> drm_dp_dpcd_read() and _write() which the drivers need to call for
> eDP connectors (right now the proxying code is hacked directly into
> those two functions).


OK.

>
>
>> 3.  Most muxed hybrid laptops also mux external displays connectors as
>> well (VGA, DVI, HDMI, DP, etc.).  Do you have any plans to extend this
>> to those cases?
>
> As far as the MacBook Pro is concerned, only the earliest two generations
> support this (MacBookPro5 2008/09 with dual Nvidia GPUs and MacBookPro6
> 2010 with Intel/Nvidia). They have a single external switchable DP port.
>
> I was thinking about hardcoding the DMIs of these few laptops in the
> drivers and call drm_get_edid_switcheroo() for the DP port. But this
> is not a high priority item. I don't have a clear plan yet.
>

OK.

>
>> 4. Some desktop environments (GNOME IIRC, but possibly KDE as well)
>> rely on the fact that ddc doesn't work on one of the GPUs when it's
>> not selected.  They don't know how to deal with muxes and can't deal
>> with the same port showing up as connected on two GPUs.  I suspect
>> this patch set may break that.  radeon has always been able to report
>> the panel information on hybrid laptops even when the mux is not
>> switched since the info is also stored in the vbios.  At the behest of
>> those desktop environments there is actually code in the driver to not
>> report the edid for the unselected gpu on hybrid laptops even though
>> we could report it.  I suspect this patch may break that.
>
> I assume you're referring to:
> http://lists.freedesktop.org/archives/dri-devel/2014-November/072032.html
> https://bugzilla.opensuse.org/show_bug.cgi?id=904417
>
> After perusing the bug report at length I'm under the impression that
> it's not GNOME getting confused by two LVDS outputs, rather this seems
> like a runtime pm issue since radeon.runpm=0 seemed to fix it.
>
> Maybe gdm was triggering an ioctl() that didn't wake up the GPU?
> I've briefly looked at the code and noticed that radeon_compat_ioctl()
> doesn't call pm_runtime_get_sync(). Maybe the reporter of the bug had
> a 32 bit GNOME installed? Or maybe the GPU failed to wake up from D0
> for some reason? There's no dmesg output attached to the bug report
> so it's impossible to tell.

I'm guessing it was an issue with gnome attempting to decide if it
should start at all.  We've had a lot of problems with gnome with how
it mis-detects hw vs sw rendering and seems to fail to start even
though everything works.

>
> It's unfortunate that the actual root cause was not clearly identified.
> In my opinion 134857943356 should be reverted and replaced with a proper
> fix. I don't understand why this got into mainline in the first place,
> you've (astutely) described this solution as a hack in the above-linked
> thread.

We'll it's not really nice to regress users if you can avoid it.  I
never really had the time to dig into issues with muxed hybrid laptops
especially since pretty much everything went muxless a while ago.

Alex

>
> Thank you and best regards,
>
> Lukas
>
>>
>> Alex
>>
>> > ---
>> >  drivers/gpu/drm/radeon/radeon_connectors.c | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
>> > index 5a2cafb..569f63c 100644
>> > --- a/drivers/gpu/drm/radeon/radeon_connectors.c
>> > +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
>> > @@ -344,6 +344,10 @@ static void radeon_connector_get_edid(struct drm_connector *connector)
>> >                 else if (radeon_connector->ddc_bus)
>> >                         radeon_connector->edid = drm_get_edid(&radeon_connector->base,
>> >                                                               &radeon_connector->ddc_bus->adapter);
>> > +       } else if (connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
>> > +                  radeon_connector->ddc_bus) {
>> > +               radeon_connector->edid = drm_get_edid_switcheroo(&radeon_connector->base,
>> > +                                                                &radeon_connector->ddc_bus->adapter);
>> >         } else if (radeon_connector->ddc_bus) {
>> >                 radeon_connector->edid = drm_get_edid(&radeon_connector->base,
>> >                                                       &radeon_connector->ddc_bus->adapter);
>> > --
>> > 1.8.5.2 (Apple Git-48)
>> >
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] vga_switcheroo: Add support for switching only the DDC

[Daniel Vetter]

On Fri, Aug 14, 2015 at 06:50:15PM +0200, Lukas Wunner wrote:
> 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 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.

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.

Otherwise patch looks good.

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.

Thanks, Daniel

>     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 <daniel.vetter@ffwll.ch>)
> 
> v2.2: Readability improvements
>     (requested by Thierry Reding <thierry.reding@gmail.com>)
> 
> v2.3: Overhaul locking once more
> 
> v2.4: Retain semantics of ->switchto handler callback to switch all pins
>     (requested by Daniel Vetter <daniel.vetter@ffwll.ch>)
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: Pierre Moreau <pierre.morrow@free.fr>
>     [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
> Tested-by: William Brown <william@blackhats.net.au>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
> Tested-by: Lukas Wunner <lukas@wunner.de>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
> 
> 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 | 98 +++++++++++++++++++++++++++++++++++++++-
>  include/linux/vga_switcheroo.h   |  9 ++++
>  2 files changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index aa077aa..9b6946e 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -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;
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index b2a3dda..6edaacc 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -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,
> -- 
> 1.8.5.2 (Apple Git-48)
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] vga_switcheroo: Add support for switching only the DDC

[Lukas Wunner]

Hi Daniel,

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

On Tue, Oct 06, 2015 at 09:27:24AM +0200, Daniel Vetter wrote:
> On Fri, Aug 14, 2015 at 06:50:15PM +0200, Lukas Wunner wrote:
> >     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.
> 
> 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().


> 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.


> 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.


> 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


Kind regards,

Lukas


> >     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 <daniel.vetter@ffwll.ch>)
> > 
> > v2.2: Readability improvements
> >     (requested by Thierry Reding <thierry.reding@gmail.com>)
> > 
> > v2.3: Overhaul locking once more
> > 
> > v2.4: Retain semantics of ->switchto handler callback to switch all pins
> >     (requested by Daniel Vetter <daniel.vetter@ffwll.ch>)
> > 
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> > Tested-by: Pierre Moreau <pierre.morrow@free.fr>
> >     [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
> > Tested-by: William Brown <william@blackhats.net.au>
> >     [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
> > Tested-by: Lukas Wunner <lukas@wunner.de>
> >     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
> > 
> > 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 | 98 +++++++++++++++++++++++++++++++++++++++-
> >  include/linux/vga_switcheroo.h   |  9 ++++
> >  2 files changed, 105 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> > index aa077aa..9b6946e 100644
> > --- a/drivers/gpu/vga/vga_switcheroo.c
> > +++ b/drivers/gpu/vga/vga_switcheroo.c
> > @@ -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;
> > diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> > index b2a3dda..6edaacc 100644
> > --- a/include/linux/vga_switcheroo.h
> > +++ b/include/linux/vga_switcheroo.h
> > @@ -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,
> > -- 
> > 1.8.5.2 (Apple Git-48)
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] vga_switcheroo: Add support for switching only the DDC

[Daniel Vetter]

On Tue, Oct 06, 2015 at 12:10:48PM +0200, Lukas Wunner wrote:
> Hi Daniel,
> 
> thank you for taking a look at the patch set and shepherding this
> through the review process.
> 
> On Tue, Oct 06, 2015 at 09:27:24AM +0200, Daniel Vetter wrote:
> > On Fri, Aug 14, 2015 at 06:50:15PM +0200, Lukas Wunner wrote:
> > >     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.
> > 
> > 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().
> 
> 
> > 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.

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).
> 
> 
> > 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.
> 
> 
> > 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.

> 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

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.

Also when resending patches unchanged please directly add r-b tags you've
collected already - replying top-level with them all again makes it a bit
harder to sort everything our here for me. And one small nit: The usual
style for patch bombing is flat threading, not nested. It should be the
default for new-ish git, but if that's not the case please use git
send-email --no-chain-reply-to. Note also if you generate patches first
with git format-patch that has a separate knob for deep threading, you
need to disable both iirc to avoid deep threading, there it's git
format-patch --thread=shallow.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] vga_switcheroo: Add support for switching only the DDC

[Lukas Wunner]

Hi Daniel,

On Tue, Oct 06, 2015 at 01:10:00PM +0200, Daniel Vetter wrote:
> On Tue, Oct 06, 2015 at 12:10:48PM +0200, Lukas Wunner wrote:
> > On Tue, Oct 06, 2015 at 09:27:24AM +0200, Daniel Vetter wrote:
> > > 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.
> 
> 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.


> > > 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?


> > 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
> 
> 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.


Thanks,

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

Re: [PATCH v3 1/6] vga_switcheroo: Add support for switching only the DDC

[Daniel Vetter]

On Tue, Oct 06, 2015 at 08:27:44PM +0200, Lukas Wunner wrote:
> Hi Daniel,
> 
> On Tue, Oct 06, 2015 at 01:10:00PM +0200, Daniel Vetter wrote:
> > On Tue, Oct 06, 2015 at 12:10:48PM +0200, Lukas Wunner wrote:
> > > On Tue, Oct 06, 2015 at 09:27:24AM +0200, Daniel Vetter wrote:
> > > > 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.
> > 
> > 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.

I think switching everything together makes some sense, but either way
should be find.

> > > > 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?

DRM has essentially become the gfx subsystem of the kernel, the name is
purely historical accident. And DRM maintainers generally take care of
everything under drivers/gpu/* so I think it makes sense to include it
there.

If you think the DRM in the docbook is confusing then we can fix that
easily by renaming it to gpu.tmpl and GPU Developer's Guide. Actually that
seems like a decent idea, so let me write that patch.

> > > 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
> > 
> > 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.

Mailing list is highly preferred.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/9] vga_switcheroo cleanup

[Lukas Wunner]

Hi Daniel,

as requested here's a resend of the 4 pending vga_switcheroo
cleanup patches (originally posted Sep 17), 2 of them now with
Reviewed-by tags.

On top, a new one for i915 and 4 new ones for Alex.

Also available on GitHub:
https://github.com/l1k/linux/commits/vga_switcheroo_cleanup

Thank you & best regards,

Lukas


Lukas Wunner (9):
  vga_switcheroo: Use enum vga_switcheroo_state instead of int
  vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead of -1
  vga_switcheroo: Use enum vga_switcheroo_client_id instead of int
  ALSA: hda - Spell vga_switcheroo consistently
  drm/i915: Drop unnecessary #include <linux/vga_switcheroo.h>
  drm/radeon: Drop unnecessary #include <linux/vga_switcheroo.h>
  drm/amdgpu: Drop unnecessary #include <linux/vga_switcheroo.h>
  drm/radeon: Fix kernel-doc copy/paste snafu
  drm/amdgpu: Fix kernel-doc copy/paste snafu

 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c |  1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c  |  4 ++--
 drivers/gpu/drm/i915/intel_acpi.c        |  1 -
 drivers/gpu/drm/radeon/radeon_acpi.c     |  1 -
 drivers/gpu/drm/radeon/radeon_asic.c     |  1 -
 drivers/gpu/drm/radeon/radeon_bios.c     |  1 -
 drivers/gpu/drm/radeon/radeon_kms.c      |  4 ++--
 drivers/gpu/vga/vga_switcheroo.c         | 36 ++++++++++++++++++--------------
 include/linux/vga_switcheroo.h           | 14 ++++++++-----
 sound/pci/hda/hda_controller.h           |  2 +-
 sound/pci/hda/hda_intel.c                | 12 +++++------
 sound/pci/hda/hda_intel.h                |  2 +-
 13 files changed, 41 insertions(+), 39 deletions(-)

-- 
1.8.5.2 (Apple Git-48)

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

[PATCH 1/9] vga_switcheroo: Use enum vga_switcheroo_state instead of int

[Lukas Wunner]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/vga/vga_switcheroo.c | 6 +++---
 include/linux/vga_switcheroo.h   | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 1acbe20..a7870d2 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -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;
 
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 3764991..e636617 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -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
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] vga_switcheroo: Use enum vga_switcheroo_state instead of int

[Alex Deucher]

On Fri, Aug 28, 2015 at 5:56 AM, Lukas Wunner <lukas@wunner.de> wrote:
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/vga/vga_switcheroo.c | 6 +++---
>  include/linux/vga_switcheroo.h   | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 1acbe20..a7870d2 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -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;
>
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index 3764991..e636617 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -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
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/9] vga_switcheroo: Use enum vga_switcheroo_client_id instead of int

[Lukas Wunner]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/vga/vga_switcheroo.c | 17 ++++++++++-------
 include/linux/vga_switcheroo.h   |  6 +++---
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 9896305..af0d372 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -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)
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 88909a8..c557511 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -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
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/9] vga_switcheroo: Use enum vga_switcheroo_client_id instead of int

[Alex Deucher]

On Fri, Aug 28, 2015 at 6:54 AM, Lukas Wunner <lukas@wunner.de> wrote:
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/vga/vga_switcheroo.c | 17 ++++++++++-------
>  include/linux/vga_switcheroo.h   |  6 +++---
>  2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 9896305..af0d372 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -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)
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index 88909a8..c557511 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -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
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/9] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead of -1

[Lukas Wunner]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/vga/vga_switcheroo.c | 17 +++++++++--------
 include/linux/vga_switcheroo.h   |  4 ++++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index a7870d2..9896305 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -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)
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index e636617..88909a8 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -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
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/9] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead of -1

[Alex Deucher]

On Fri, Aug 28, 2015 at 7:30 AM, Lukas Wunner <lukas@wunner.de> wrote:
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/vga/vga_switcheroo.c | 17 +++++++++--------
>  include/linux/vga_switcheroo.h   |  4 ++++
>  2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index a7870d2..9896305 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -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)
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index e636617..88909a8 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -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
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/9] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead of -1

[Daniel Vetter]

On Mon, Oct 12, 2015 at 12:09:53PM -0400, Alex Deucher wrote:
> On Fri, Aug 28, 2015 at 7:30 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

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

> 
> > ---
> >  drivers/gpu/vga/vga_switcheroo.c | 17 +++++++++--------
> >  include/linux/vga_switcheroo.h   |  4 ++++
> >  2 files changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> > index a7870d2..9896305 100644
> > --- a/drivers/gpu/vga/vga_switcheroo.c
> > +++ b/drivers/gpu/vga/vga_switcheroo.c
> > @@ -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)
> > diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> > index e636617..88909a8 100644
> > --- a/include/linux/vga_switcheroo.h
> > +++ b/include/linux/vga_switcheroo.h
> > @@ -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
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/9] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead of -1

[Lukas Wunner]

Hi Daniel,

On Tue, Oct 13, 2015 at 09:28:13AM +0200, Daniel Vetter wrote:
> On Mon, Oct 12, 2015 at 12:09:53PM -0400, Alex Deucher wrote:
> > On Fri, Aug 28, 2015 at 7:30 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> 
> 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: <s5hpp0zg2z3.wl-tiwai@suse.de>
Date: Wed, 30 Sep 2015 20:46:08 +0200


Thanks & best regards,

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

Re: [PATCH 2/9] vga_switcheroo: Use VGA_SWITCHEROO_UNKNOWN_ID instead of -1

[Daniel Vetter]

On Thu, Oct 15, 2015 at 08:14:20PM +0200, Lukas Wunner wrote:
> Hi Daniel,
> 
> On Tue, Oct 13, 2015 at 09:28:13AM +0200, Daniel Vetter wrote:
> > On Mon, Oct 12, 2015 at 12:09:53PM -0400, Alex Deucher wrote:
> > > On Fri, Aug 28, 2015 at 7:30 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> > 
> > 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: <s5hpp0zg2z3.wl-tiwai@suse.de>
> Date: Wed, 30 Sep 2015 20:46:08 +0200
> 
> 
> Thanks & best regards,

Right, dunno why I missed that. Merged to drm-misc now, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/9] ALSA: hda - Spell vga_switcheroo consistently

[Lukas Wunner]

Currently everyone and their dog has their own favourite spelling
for vga_switcheroo. This makes it hard to grep dmesg for log entries
relating to vga_switcheroo. It also makes it hard to find related
source files in the tree.

vga_switcheroo.c uses pr_fmt "vga_switcheroo". Use that everywhere.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_controller.h |  2 +-
 sound/pci/hda/hda_intel.c      | 12 ++++++------
 sound/pci/hda/hda_intel.h      |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index 314105c..7b635d6 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -153,7 +153,7 @@ struct azx {
 	unsigned int snoop:1;
 	unsigned int align_buffer_size:1;
 	unsigned int region_requested:1;
-	unsigned int disabled:1; /* disabled by VGA-switcher */
+	unsigned int disabled:1; /* disabled by vga_switcheroo */
 
 #ifdef CONFIG_SND_HDA_DSP_LOADER
 	struct azx_dev saved_azx_dev;
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e819013..45233731 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -337,7 +337,7 @@ enum {
 	 AZX_DCAPS_4K_BDLE_BOUNDARY | AZX_DCAPS_SNOOP_OFF)
 
 /*
- * VGA-switcher support
+ * vga_switcheroo support
  */
 #ifdef SUPPORT_VGA_SWITCHEROO
 #define use_vga_switcheroo(chip)	((chip)->use_vga_switcheroo)
@@ -1076,12 +1076,12 @@ static void azx_vs_set_state(struct pci_dev *pci,
 			}
 		}
 	} else {
-		dev_info(chip->card->dev, "%s via VGA-switcheroo\n",
+		dev_info(chip->card->dev, "%s via vga_switcheroo\n",
 			 disabled ? "Disabling" : "Enabling");
 		if (disabled) {
 			pm_runtime_put_sync_suspend(card->dev);
 			azx_suspend(card->dev);
-			/* when we get suspended by vga switcheroo we end up in D3cold,
+			/* when we get suspended by vga_switcheroo we end up in D3cold,
 			 * however we have no ACPI handle, so pci/acpi can't put us there,
 			 * put ourselves there */
 			pci->current_state = PCI_D3cold;
@@ -1121,7 +1121,7 @@ static void init_vga_switcheroo(struct azx *chip)
 	struct pci_dev *p = get_bound_vga(chip->pci);
 	if (p) {
 		dev_info(chip->card->dev,
-			 "Handle VGA-switcheroo audio client\n");
+			 "Handle vga_switcheroo audio client\n");
 		hda->use_vga_switcheroo = 1;
 		pci_dev_put(p);
 	}
@@ -1232,7 +1232,7 @@ static int azx_dev_free(struct snd_device *device)
 
 #ifdef SUPPORT_VGA_SWITCHEROO
 /*
- * Check of disabled HDMI controller by vga-switcheroo
+ * Check of disabled HDMI controller by vga_switcheroo
  */
 static struct pci_dev *get_bound_vga(struct pci_dev *pci)
 {
@@ -1917,7 +1917,7 @@ static int azx_probe(struct pci_dev *pci,
 
 	err = register_vga_switcheroo(chip);
 	if (err < 0) {
-		dev_err(card->dev, "Error registering VGA-switcheroo client\n");
+		dev_err(card->dev, "Error registering vga_switcheroo client\n");
 		goto out_free;
 	}
 
diff --git a/sound/pci/hda/hda_intel.h b/sound/pci/hda/hda_intel.h
index 354f0bb..ff0c4d6 100644
--- a/sound/pci/hda/hda_intel.h
+++ b/sound/pci/hda/hda_intel.h
@@ -35,7 +35,7 @@ struct hda_intel {
 	unsigned int irq_pending_warned:1;
 	unsigned int probe_continued:1;
 
-	/* VGA-switcheroo setup */
+	/* vga_switcheroo setup */
 	unsigned int use_vga_switcheroo:1;
 	unsigned int vga_switcheroo_registered:1;
 	unsigned int init_failed:1; /* delayed init failed */
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH 5/9] drm/i915: Drop unnecessary #include <linux/vga_switcheroo.h>

[Lukas Wunner]

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 <lukas@wunner.de>
---
 drivers/gpu/drm/i915/intel_acpi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
index d96eee1..68119b6 100644
--- a/drivers/gpu/drm/i915/intel_acpi.c
+++ b/drivers/gpu/drm/i915/intel_acpi.c
@@ -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"
 
-- 
1.8.5.2 (Apple Git-48)

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

Re: [PATCH 5/9] drm/i915: Drop unnecessary #include <linux/vga_switcheroo.h>

[Daniel Vetter]

On Mon, Oct 12, 2015 at 10:57:43AM +0200, Lukas Wunner wrote:
> 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 <lukas@wunner.de>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_acpi.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
> index d96eee1..68119b6 100644
> --- a/drivers/gpu/drm/i915/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/intel_acpi.c
> @@ -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"
>  
> -- 
> 1.8.5.2 (Apple Git-48)
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/9] drm/i915: Drop unnecessary #include <linux/vga_switcheroo.h>

[Jani Nikula]

On Mon, 12 Oct 2015, Lukas Wunner <lukas@wunner.de> wrote:
> 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 <lukas@wunner.de>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_acpi.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_acpi.c b/drivers/gpu/drm/i915/intel_acpi.c
> index d96eee1..68119b6 100644
> --- a/drivers/gpu/drm/i915/intel_acpi.c
> +++ b/drivers/gpu/drm/i915/intel_acpi.c
> @@ -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"
>  
> -- 
> 1.8.5.2 (Apple Git-48)
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/9] drm/radeon: Drop unnecessary #include <linux/vga_switcheroo.h>

[Lukas Wunner]

This was added to three files even though they don't use any
vga_switcheroo symbols:

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

Added to radeon_asic.c by commit 0a10c85129c2 ("drm/radeon: create
radeon_asic.c").

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

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/radeon/radeon_acpi.c | 1 -
 drivers/gpu/drm/radeon/radeon_asic.c | 1 -
 drivers/gpu/drm/radeon/radeon_bios.c | 1 -
 3 files changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c b/drivers/gpu/drm/radeon/radeon_acpi.c
index 77e9d07..59acd0e 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -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>
diff --git a/drivers/gpu/drm/radeon/radeon_asic.c b/drivers/gpu/drm/radeon/radeon_asic.c
index f2421bc..1d4d452 100644
--- a/drivers/gpu/drm/radeon/radeon_asic.c
+++ b/drivers/gpu/drm/radeon/radeon_asic.c
@@ -31,7 +31,6 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/radeon_drm.h>
 #include <linux/vgaarb.h>
-#include <linux/vga_switcheroo.h>
 #include "radeon_reg.h"
 #include "radeon.h"
 #include "radeon_asic.h"
diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
index d27e4cc..21b6732 100644
--- a/drivers/gpu/drm/radeon/radeon_bios.c
+++ b/drivers/gpu/drm/radeon/radeon_bios.c
@@ -30,7 +30,6 @@
 #include "radeon.h"
 #include "atom.h"
 
-#include <linux/vga_switcheroo.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
 /*
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH 8/9] drm/radeon: Fix kernel-doc copy/paste snafu

[Lukas Wunner]

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

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/radeon/radeon_kms.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index 4e2780f..ee5cc14 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -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
  *
  * @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)

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

Re: [PATCH 8/9] drm/radeon: Fix kernel-doc copy/paste snafu

[Alex Deucher]

On Mon, Oct 12, 2015 at 5:44 AM, Lukas Wunner <lukas@wunner.de> wrote:
> Introduced by f482a1419545 ("drm/radeon: document radeon_kms.c").
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/drm/radeon/radeon_kms.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index 4e2780f..ee5cc14 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -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.

Alex

>   *
>   * @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)
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 8/9] drm/radeon: Fix kernel-doc copy/paste snafu

[Lukas Wunner]

Hi Alex,

On Mon, Oct 12, 2015 at 12:02:13PM -0400, Alex Deucher wrote:
> On Mon, Oct 12, 2015 at 5:44 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > Introduced by f482a1419545 ("drm/radeon: document radeon_kms.c").
> >
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > ---
> >  drivers/gpu/drm/radeon/radeon_kms.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> > index 4e2780f..ee5cc14 100644
> > --- a/drivers/gpu/drm/radeon/radeon_kms.c
> > +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> > @@ -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

Nice! I missed that because it's not yet in drm-next.


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

Not that important, so waived.

Thanks a lot!

Lukas


> >   *
> >   * @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)
> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/9] drm/amdgpu: Drop unnecessary #include <linux/vga_switcheroo.h>

[Lukas Wunner]

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 <lukas@wunner.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index aef4a7a..a142d5a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -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>
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
index 02add0a..c44c0c6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
@@ -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)

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

Re: [PATCH 7/9] drm/amdgpu: Drop unnecessary #include <linux/vga_switcheroo.h>

[Alex Deucher]

On Mon, Oct 12, 2015 at 5:54 AM, Lukas Wunner <lukas@wunner.de> wrote:
> 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 <lukas@wunner.de>

Applied 6 and 7.  thanks!

Alex


> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 1 -
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 1 -
>  2 files changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> index aef4a7a..a142d5a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
> @@ -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>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> index 02add0a..c44c0c6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> @@ -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)
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 9/9] drm/amdgpu: Fix kernel-doc copy/paste snafu

[Lukas Wunner]

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

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 371f015..b1d499e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -485,11 +485,11 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
  * Outdated mess for old drm with Xorg being in charge (void function now).
  */
 /**
- * amdgpu_driver_firstopen_kms - drm callback for last close
+ * amdgpu_driver_lastclose_kms - drm callback for last close
  *
  * @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)

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

Re: [PATCH 9/9] drm/amdgpu: Fix kernel-doc copy/paste snafu

[Alex Deucher]

On Mon, Oct 12, 2015 at 6:00 AM, Lukas Wunner <lukas@wunner.de> wrote:
> Introduced by f482a1419545 ("drm/radeon: document radeon_kms.c")
> and inherited by the amdgpu fork.
>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 371f015..b1d499e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -485,11 +485,11 @@ static int amdgpu_info_ioctl(struct drm_device *dev, void *data, struct drm_file
>   * Outdated mess for old drm with Xorg being in charge (void function now).
>   */
>  /**
> - * amdgpu_driver_firstopen_kms - drm callback for last close
> + * amdgpu_driver_lastclose_kms - drm callback for last close

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

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

Alex

>   *
>   * @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)
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 0/6] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

Changes since v3:
  * Previously when switching the display, the DDC lines were switched
    as well. But actually this is not necessary: If a GPU probes EDID,
    DDC must be switched and locked to the GPU anyway, so why switch
    DDC preemptively? Also, previously the DDC lines were switched back
    to the previous owner on unlock. This is unnecessary for the same
    reason. So I did away with all that and it simplifies the code
    considerably. This works fine as long as anything accessing DDC
    calls vga_switcheroo_lock_ddc() first. If something in the kernel
    or a VM or in user space accesses DDC and omits locking it first,
    things may go awry. (However, if locking is omitted, things could
    go awry with the previous behaviour as well.) I'm curious if this
    new behaviour provokes objections.
  * I now have conclusive evidence that only the pre-retina MacBook Pro
    can switch DDC. The mux in the retina is incapable of that. So the
    ->switch_ddc callback is no longer advertised to vga_switcheroo
    on retinas.


Also available on GitHub for easier reviewing:
https://github.com/l1k/linux/commits/mbp_switcheroo_v4


For those who haven't been following this series so far:

The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
to switch the panel between its two GPUs. The panel mode in VBIOS
is notoriously bogus on these machines and some models have no
VBIOS at all, so the inactive GPU cannot set up its LVDS output.

Extend vga_switcheroo to support switching only the DDC lines.
Introduce a drm_get_edid_switcheroo() helper which uses this feature.
Amend i915, nouveau and radeon to call it for LVDS connectors.

This only enables EDID probing on the pre-retina MBP (2008 - 2013),
and only under the condition that apple-gmux loads before the DRM
drivers. Later patches will address reprobing of the DRM drivers
if apple-gmux loads late.

The retina MBP (2012 - present) uses eDP and is apparently not
capable of switching AUX separately from the main link.
This will also be addressed in later patches.


Previous installments:
v1: http://lists.freedesktop.org/archives/dri-devel/2015-April/081515.html
v2: http://lists.freedesktop.org/archives/dri-devel/2015-August/088156.html
v3: http://lists.freedesktop.org/archives/dri-devel/2015-October/091741.html


Lukas Wunner (6):
  vga_switcheroo: Add support for switching only the DDC
  apple-gmux: Add switch_ddc support
  drm/edid: Switch DDC when reading the EDID
  drm/i915: Switch DDC when reading the EDID
  drm/nouveau: Switch DDC when reading the EDID
  drm/radeon: Switch DDC when reading the EDID

 drivers/gpu/drm/drm_edid.c                  | 26 ++++++++++++
 drivers/gpu/drm/i915/intel_lvds.c           |  3 +-
 drivers/gpu/drm/nouveau/nouveau_connector.c | 13 +++++-
 drivers/gpu/drm/radeon/radeon_connectors.c  |  4 ++
 drivers/gpu/vga/vga_switcheroo.c            | 66 ++++++++++++++++++++++++++++-
 drivers/platform/x86/apple-gmux.c           | 21 ++++++++-
 include/drm/drm_crtc.h                      |  2 +
 include/linux/vga_switcheroo.h              | 15 +++++--
 8 files changed, 141 insertions(+), 9 deletions(-)

-- 
1.8.5.2 (Apple Git-48)

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v4 5/6] drm/nouveau: Switch DDC when reading the EDID

[Lukas Wunner]

The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
to switch the panel between its two GPUs. The panel mode in VBIOS
is notoriously bogus on these machines.

Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS.
This allows us to retrieve the EDID if the outputs are currently
muxed to the other GPU by temporarily switching the panel's DDC
lines. Likewise, ask vga_switcheroo to switch DDC before probing
LVDS connectors.

This only enables EDID probing on the pre-retina MBP (2008 - 2013).
The retina MBP (2012 - present) uses eDP and gmux is not capable
of switching AUX separately from the main link on these models.
This will be addressed in later patches.

List of pre-retina MBPs with dual GPUs, either or both Nvidia:
    [MBP  5,1 2008  nvidia MCP79 + G96        pre-retina  15"]
    [MBP  5,2 2009  nvidia MCP79 + G96        pre-retina  17"]
    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina  15"]
    [MBP  6,1 2010  intel ILK + nvidia GT216  pre-retina  17"]
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

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  15"]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 2e7cbe9..fb12c9e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -27,6 +27,7 @@
 #include <acpi/button.h>
 
 #include <linux/pm_runtime.h>
+#include <linux/vga_switcheroo.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_edid.h>
@@ -148,7 +149,13 @@ nouveau_connector_ddc_detect(struct drm_connector *connector)
 				break;
 		} else
 		if (nv_encoder->i2c) {
-			if (nvkm_probe_i2c(nv_encoder->i2c, 0x50))
+			int ret;
+			if (nv_encoder->dcb->type == DCB_OUTPUT_LVDS)
+				vga_switcheroo_lock_ddc(dev->pdev);
+			ret = nvkm_probe_i2c(nv_encoder->i2c, 0x50);
+			if (nv_encoder->dcb->type == DCB_OUTPUT_LVDS)
+				vga_switcheroo_unlock_ddc();
+			if (ret)
 				break;
 		}
 	}
@@ -259,7 +266,9 @@ nouveau_connector_detect(struct drm_connector *connector, bool force)
 
 	nv_encoder = nouveau_connector_ddc_detect(connector);
 	if (nv_encoder && (i2c = nv_encoder->i2c) != NULL) {
-		nv_connector->edid = drm_get_edid(connector, i2c);
+		nv_connector->edid = nv_encoder->dcb->type == DCB_OUTPUT_LVDS ?
+				     drm_get_edid_switcheroo(connector, i2c) :
+				     drm_get_edid(connector, i2c);
 		drm_mode_connector_update_edid_property(connector,
 							nv_connector->edid);
 		if (!nv_connector->edid) {
-- 
1.8.5.2 (Apple Git-48)

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH v4 2/6] apple-gmux: Add switch_ddc support

[Lukas Wunner]

Originally by Seth Forshee <seth.forshee@canonical.com>, 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 <lukas@wunner.de>, 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 <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Cc: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/platform/x86/apple-gmux.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 0dec3f5..78997b7 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -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 {
-		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);
 	}
@@ -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
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/6] apple-gmux: Add switch_ddc support

[Alex Deucher]

On Fri, Aug 14, 2015 at 12:18 PM, Lukas Wunner <lukas@wunner.de> wrote:
> Originally by Seth Forshee <seth.forshee@canonical.com>, 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 <lukas@wunner.de>, 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 <lukas@wunner.de>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
>
> Cc: Seth Forshee <seth.forshee@canonical.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/platform/x86/apple-gmux.c | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 0dec3f5..78997b7 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -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 {
> -               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


> @@ -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
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/6] apple-gmux: Add switch_ddc support

[Alex Deucher]

On Mon, Oct 12, 2015 at 5:07 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Fri, Aug 14, 2015 at 12:18 PM, Lukas Wunner <lukas@wunner.de> wrote:
>> Originally by Seth Forshee <seth.forshee@canonical.com>, 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 <lukas@wunner.de>, 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 <lukas@wunner.de>
>>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
>>
>> Cc: Seth Forshee <seth.forshee@canonical.com>
>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
>> ---
>>  drivers/platform/x86/apple-gmux.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
>> index 0dec3f5..78997b7 100644
>> --- a/drivers/platform/x86/apple-gmux.c
>> +++ b/drivers/platform/x86/apple-gmux.c
>> @@ -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 {
>> -               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

>
> Alex
>
>
>> @@ -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
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/6] apple-gmux: Add switch_ddc support

[Daniel Vetter]

On Mon, Oct 12, 2015 at 05:10:20PM -0400, Alex Deucher wrote:
> On Mon, Oct 12, 2015 at 5:07 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> > On Fri, Aug 14, 2015 at 12:18 PM, Lukas Wunner <lukas@wunner.de> wrote:
> >> Originally by Seth Forshee <seth.forshee@canonical.com>, 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 <lukas@wunner.de>, 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 <lukas@wunner.de>
> >>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
> >>
> >> Cc: Seth Forshee <seth.forshee@canonical.com>
> >> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> >> ---
> >>  drivers/platform/x86/apple-gmux.c | 21 +++++++++++++++++++--
> >>  1 file changed, 19 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> >> index 0dec3f5..78997b7 100644
> >> --- a/drivers/platform/x86/apple-gmux.c
> >> +++ b/drivers/platform/x86/apple-gmux.c
> >> @@ -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 {
> >> -               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.

Yeah I think the old desing was less surprising. And the
s/ddc_lock/hw_lock/ change would the make sense again.

Also when resending please also keep a per-patch changelog, not only in
the cover letter. Otherwise you have to jump back&forth all the time
between patches and cover letter. And the kerneldoc still seems to be
missing in this resend, so looks incomplete (or I'm missing something).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/6] apple-gmux: Add switch_ddc support

[Lukas Wunner]

Hi Alex,

On Mon, Oct 12, 2015 at 05:10:20PM -0400, Alex Deucher wrote:
> On Mon, Oct 12, 2015 at 5:07 PM, Alex Deucher <alexdeucher@gmail.com> wrote:
> > On Fri, Aug 14, 2015 at 12:18 PM, Lukas Wunner <lukas@wunner.de> wrote:
> >> Originally by Seth Forshee <seth.forshee@canonical.com>, 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 <lukas@wunner.de>, 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 <lukas@wunner.de>
> >>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
> >>
> >> Cc: Seth Forshee <seth.forshee@canonical.com>
> >> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> >> ---
> >>  drivers/platform/x86/apple-gmux.c | 21 +++++++++++++++++++--
> >>  1 file changed, 19 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> >> index 0dec3f5..78997b7 100644
> >> --- a/drivers/platform/x86/apple-gmux.c
> >> +++ b/drivers/platform/x86/apple-gmux.c
> >> @@ -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 {
> >> -               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.

I had to do some experimenting and research to clarify this definitively,
hence the belated reply.

Answer is no, GMUX_PORT_SWITCH_DDC only switches DDC of the panel.

The first two generations of the MacBook Pro (MBP 5 2008/09, MBP 6 2010)
are able to switch the external DP port between GPUs, but only in its
entirety. The mux used for this (NXP CBTL06141) is not capable of
switching DDC separately. It's controlled by GMUX_PORT_SWITCH_EXTERNAL.

The following two generations (MBP 8 2011, MBP 9 2012) replaced the
external DP port with a combined DP/Thunderbolt port which can drive
either a single "dumb" DP display or two daisy-chained Thunderbolt
displays using DP-over-Thunderbolt. The ability to switch the external
port in its entirety was given up but the AUX channel is still switchable,
under the control of GMUX_PORT_SWITCH_EXTERNAL.

The port appears disabled to the Intel GPU, but I hacked i915 to
force-enable pipe B and all DP ports and was then able to retrieve
DPCD and EDID from an external display.

The retina MBP gained a second DP/Thunderbolt port and an HDMI port
and is still able to switch AUX to the integrated GPU on both DP ports!
I wonder why Apple went to these lengths. Maybe they found that HPD is
unreliable, so they let the Intel GPU poll AUX and wake up the discrete
GPU if anything is detected?

Best regards,

Lukas

> 
> Alex
> 
> >
> > Alex
> >
> >
> >> @@ -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
> >> dri-devel@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/6] apple-gmux: Add switch_ddc support

[Darren Hart]

On Fri, Aug 14, 2015 at 06:18:55PM +0200, Lukas Wunner wrote:
> Originally by Seth Forshee <seth.forshee@canonical.com>, 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 <lukas@wunner.de>, 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 <lukas@wunner.de>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
> 
> Cc: Seth Forshee <seth.forshee@canonical.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

For platform/drivers/x86:

Acked-by: Darren Hart <dvhart@linux.intel.com>

Daniel, I presume this will ultimately get pulled in by you with the rest of the
series?
-- 
Darren Hart
Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/6] apple-gmux: Add switch_ddc support

[Daniel Vetter]

On Wed, Oct 14, 2015 at 09:51:13PM -0700, Darren Hart wrote:
> On Fri, Aug 14, 2015 at 06:18:55PM +0200, Lukas Wunner wrote:
> > Originally by Seth Forshee <seth.forshee@canonical.com>, 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 <lukas@wunner.de>, 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 <lukas@wunner.de>
> >     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]
> > 
> > Cc: Seth Forshee <seth.forshee@canonical.com>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> 
> For platform/drivers/x86:
> 
> Acked-by: Darren Hart <dvhart@linux.intel.com>
> 
> Daniel, I presume this will ultimately get pulled in by you with the rest of the
> series?

Yeah I can collect them all in drm-misc, if there's relevant acks. Thanks
for yours.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 3/6] drm/edid: Switch DDC when reading the EDID

[Lukas Wunner]

Originally by Seth Forshee <seth.forshee@canonical.com>, 2012-10-04:
    Some dual graphics machines support muxing the DDC separately from
    the display, so make use of this functionality when reading the EDID
    on the inactive GPU. Also serialize drm_get_edid() with a mutex to
    avoid races on the DDC mux state.

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-09-11:
    Move vga_switcheroo calls to a wrapper around drm_get_edid() which
    drivers can call on muxed machines. This avoids other drivers having
    to go through the vga_switcheroo motions even though they are never
    used on a muxed platform.
    (suggested by Thierry Reding <treding@nvidia.com>)

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  15"]

Cc: Seth Forshee <seth.forshee@canonical.com>
Cc: Dave Airlie <airlied@gmail.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/drm_edid.c | 26 ++++++++++++++++++++++++++
 include/drm/drm_crtc.h     |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index d5d2c03..f28385c 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -32,6 +32,7 @@
 #include <linux/hdmi.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/vga_switcheroo.h>
 #include <drm/drmP.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_displayid.h>
@@ -1389,6 +1390,31 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 EXPORT_SYMBOL(drm_get_edid);
 
 /**
+ * drm_get_edid_switcheroo - get EDID data for a vga_switcheroo output
+ * @connector: connector we're probing
+ * @adapter: I2C adapter to use for DDC
+ *
+ * Wrapper around drm_get_edid() for laptops with dual GPUs using one set of
+ * outputs. The wrapper adds the requisite vga_switcheroo calls to temporarily
+ * switch DDC to the GPU which is retrieving EDID.
+ *
+ * Return: Pointer to valid EDID or NULL if we couldn't find any.
+ */
+struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
+				     struct i2c_adapter *adapter)
+{
+	struct pci_dev *pdev = connector->dev->pdev;
+	struct edid *edid;
+
+	vga_switcheroo_lock_ddc(pdev);
+	edid = drm_get_edid(connector, adapter);
+	vga_switcheroo_unlock_ddc();
+
+	return edid;
+}
+EXPORT_SYMBOL(drm_get_edid_switcheroo);
+
+/**
  * drm_edid_duplicate - duplicate an EDID and the extensions
  * @edid: EDID to duplicate
  *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 33ddedd..483f70c 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1297,6 +1297,8 @@ extern void drm_property_destroy_user_blobs(struct drm_device *dev,
 extern bool drm_probe_ddc(struct i2c_adapter *adapter);
 extern struct edid *drm_get_edid(struct drm_connector *connector,
 				 struct i2c_adapter *adapter);
+extern struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
+					    struct i2c_adapter *adapter);
 extern struct edid *drm_edid_duplicate(const struct edid *edid);
 extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 extern void drm_mode_config_init(struct drm_device *dev);
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v4 1/6] vga_switcheroo: Add support for switching only the DDC

[Lukas Wunner]

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 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.

    Don't switch DDC lines in stage2, it's pointless because if DDC is
    accessed, _lock_ddc() needs to be called anyway. And for consistency
    we'd also have to switch DDC on MIGD / MDIS commands and on runtime
    suspend, needlessly inflating the code. For the same reason it's
    unnecessary to switch DDC back to the previous owner on unlock.

v2.1: Overhaul locking, squash commits
    (requested by Daniel Vetter <daniel.vetter@ffwll.ch>)

v2.2: Readability improvements
    (requested by Thierry Reding <thierry.reding@gmail.com>)

v2.3: Overhaul locking once more

v2.4: Retain semantics of ->switchto handler callback to switch all pins
    (requested by Daniel Vetter <daniel.vetter@ffwll.ch>)

v4:   Drop DDC switch in stage2, MIGD / MDIS and runtime suspend

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  15"]

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 | 66 +++++++++++++++++++++++++++++++++++++++-
 include/linux/vga_switcheroo.h   | 15 +++++++--
 2 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index af0d372..7aaaa57 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -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,7 @@ 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
  *
  * vga_switcheroo private data. Currently only one vga_switcheroo instance
  * per system is supported.
@@ -141,6 +152,7 @@ struct vgasr_priv {
 	struct list_head clients;
 
 	struct vga_switcheroo_handler *handler;
+	struct mutex ddc_lock;
 };
 
 #define ID_BIT_AUDIO		0x100
@@ -155,6 +167,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 +234,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);
@@ -414,6 +429,55 @@ 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 released by calling vga_switcheroo_unlock_ddc(),
+ * even if this function returns an error.
+ *
+ * Return: 0 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)
+		return -ENODEV;
+
+	id = vgasr_priv.handler->get_client_id(pdev);
+	return vgasr_priv.handler->switch_ddc(id);
+}
+EXPORT_SYMBOL(vga_switcheroo_lock_ddc);
+
+/**
+ * vga_switcheroo_unlock_ddc() - release DDC lines after a temporary switch
+ *
+ * Release DDC lines after having temporarily switched them to a client.
+ * This must be called even if vga_switcheroo_lock_ddc() returned an error.
+ * Invoking this function without calling vga_switcheroo_lock_ddc() first
+ * is not allowed.
+ */
+void vga_switcheroo_unlock_ddc(void)
+{
+	if (WARN_ON_ONCE(!mutex_is_locked(&vgasr_priv.ddc_lock)))
+		return;
+
+	mutex_unlock(&vgasr_priv.ddc_lock);
+}
+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
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index c557511..f5899b6 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -84,9 +84,12 @@ enum vga_switcheroo_client_id {
  * 	vga clients. Currently only the radeon and amdgpu drivers use this.
  * 	The return value is ignored
  * @switchto: switch outputs to given client.
- * 	Mandatory. For muxless machines this should be a no-op. Returning 0
- * 	denotes success, anything else failure (in which case the switch is
- * 	aborted)
+ * 	Mandatory. For muxless machines this should be a no-op. If the mux
+ * 	is able to switch DDC separately, this should switch all pins except
+ * 	for the DDC lines. 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 0 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.
@@ -98,6 +101,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);
@@ -140,6 +144,9 @@ void vga_switcheroo_client_fb_set(struct pci_dev *dev,
 int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler);
 void vga_switcheroo_unregister_handler(void);
 
+int vga_switcheroo_lock_ddc(struct pci_dev *pdev);
+void vga_switcheroo_unlock_ddc(void);
+
 int vga_switcheroo_process_delayed_switch(void);
 
 enum vga_switcheroo_state vga_switcheroo_get_client_state(struct pci_dev *dev);
@@ -160,6 +167,8 @@ static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 	const struct vga_switcheroo_client_ops *ops,
 	enum vga_switcheroo_client_id id) { return 0; }
 static inline void vga_switcheroo_unregister_handler(void) {}
+static inline int vga_switcheroo_lock_ddc(struct pci_dev *pdev) { return 0; }
+static inline void vga_switcheroo_unlock_ddc(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
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 4/6] drm/i915: Switch DDC when reading the EDID

[Lukas Wunner]

The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
to switch the panel between its two GPUs. The panel mode in VBIOS
is notoriously bogus on these machines and some models have no
VBIOS at all.

Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS.
This allows us to retrieve the EDID if the outputs are currently
muxed to the discrete GPU by temporarily switching the panel's DDC
lines to the integrated GPU.

This only enables EDID probing on the pre-retina MBP (2008 - 2013).
The retina MBP (2012 - present) uses eDP and gmux is not capable
of switching AUX separately from the main link on these models.
This will be addressed in later patches.

List of pre-retina MBPs with dual GPUs, one of them Intel:
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina  15"]
    [MBP  6,1 2010  intel ILK + nvidia GT216  pre-retina  17"]
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
    [MBP  8,3 2011  intel SNB + amd turks     pre-retina  17"]
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  15"]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/intel_lvds.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 881b5d1..a84727f 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -1067,7 +1067,8 @@ void intel_lvds_init(struct drm_device *dev)
 	 * preferred mode is the right one.
 	 */
 	mutex_lock(&dev->mode_config.mutex);
-	edid = drm_get_edid(connector, intel_gmbus_get_adapter(dev_priv, pin));
+	edid = drm_get_edid_switcheroo(connector,
+				       intel_gmbus_get_adapter(dev_priv, pin));
 	if (edid) {
 		if (drm_add_edid_modes(connector, edid)) {
 			drm_mode_connector_update_edid_property(connector,
-- 
1.8.5.2 (Apple Git-48)

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

[PATCH v4 6/6] drm/radeon: Switch DDC when reading the EDID

[Lukas Wunner]

The pre-retina MacBook Pro uses an LVDS panel and a gmux controller
to switch the panel between its two GPUs. The panel mode in VBIOS
is notoriously bogus on these machines.

Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS.
This allows us to retrieve the EDID if the outputs are currently
muxed to the integrated GPU by temporarily switching the panel's
DDC lines to the discrete GPU.

This only enables EDID probing on the pre-retina MBP (2008 - 2013).
The retina MBP (2012 - present) uses eDP and gmux is not capable
of switching AUX separately from the main link on these models.
This will be addressed in later patches.

List of pre-retina MBPs with dual GPUs, one of them AMD:
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
    [MBP  8,3 2011  intel SNB + amd turks     pre-retina  17"]

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/radeon/radeon_connectors.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index 5a2cafb..569f63c 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -344,6 +344,10 @@ static void radeon_connector_get_edid(struct drm_connector *connector)
 		else if (radeon_connector->ddc_bus)
 			radeon_connector->edid = drm_get_edid(&radeon_connector->base,
 							      &radeon_connector->ddc_bus->adapter);
+	} else if (connector->connector_type == DRM_MODE_CONNECTOR_LVDS &&
+		   radeon_connector->ddc_bus) {
+		radeon_connector->edid = drm_get_edid_switcheroo(&radeon_connector->base,
+								 &radeon_connector->ddc_bus->adapter);
 	} else if (radeon_connector->ddc_bus) {
 		radeon_connector->edid = drm_get_edid(&radeon_connector->base,
 						      &radeon_connector->ddc_bus->adapter);
-- 
1.8.5.2 (Apple Git-48)

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

Re: [PATCH v3 0/6] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

Hi,

I've also pushed this series to GitHub now to ease reviewing:
https://github.com/l1k/linux/commits/mbp_switcheroo_v3

Thanks,

Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/6] Enable gpu switching on the MacBook Pro

[Evan Foss]

On Mon, Oct 5, 2015 at 9:23 AM, Lukas Wunner <lukas@wunner.de> wrote:
> Hi,
>
> I've also pushed this series to GitHub now to ease reviewing:
> https://github.com/l1k/linux/commits/mbp_switcheroo_v3

So to test this all someone has to do is pull this and try it? No
patching required?

> Thanks,
>
> Lukas
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau



-- 
Home
http://evanfoss.googlepages.com/
Work
http://forge.abcd.harvard.edu/gf/project/epl_engineering/wiki/
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v3 0/6] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

Hi Evan,

On Mon, Oct 05, 2015 at 10:15:53AM -0400, Evan Foss wrote:
> On Mon, Oct 5, 2015 at 9:23 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > I've also pushed this series to GitHub now to ease reviewing:
> > https://github.com/l1k/linux/commits/mbp_switcheroo_v3
> 
> So to test this all someone has to do is pull this and try it? No
> patching required?

Yes. But if you don't already have a kernel git repo, this will be
a > 1 GByte download.

Which MacBook Pro model do you want to test this on? As noted in the
cover letter, this will only work on pre-retinas and only if the
apple-gmux module loads before the DRM drivers. You can try to ensure
the latter by editing modules.dep, or grab this tarball which contains
reprobing patches on top:
http://wunner.de/mbp_switcheroo_v3_reprobe_4.3-rc4pre.tar.gz

Best regards,

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

Re: [PATCH v3 0/6] Enable gpu switching on the MacBook Pro

[Evan Foss]

On Mon, Oct 5, 2015 at 11:17 AM, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Evan,
>
> On Mon, Oct 05, 2015 at 10:15:53AM -0400, Evan Foss wrote:
>> On Mon, Oct 5, 2015 at 9:23 AM, Lukas Wunner <lukas@wunner.de> wrote:
>> > I've also pushed this series to GitHub now to ease reviewing:
>> > https://github.com/l1k/linux/commits/mbp_switcheroo_v3
>>
>> So to test this all someone has to do is pull this and try it? No
>> patching required?
>
> Yes. But if you don't already have a kernel git repo, this will be
> a > 1 GByte download.

2012
Macbook Pro 9,2

$ dmesg|grep Apple
[    0.000000] efi: EFI v1.10 by Apple
[    0.000000] DMI: Apple Inc. MacBookPro9,1/Mac-
4B7AC7E43945597E,
BIOS MBP91.88Z.00D3.B08.1208081132 08/08/2012

> Which MacBook Pro model do you want to test this on? As noted in the
> cover letter, this will only work on pre-retinas and only if the
> apple-gmux module loads before the DRM drivers. You can try to ensure
> the latter by editing modules.dep, or grab this tarball which contains
> reprobing patches on top:
> http://wunner.de/mbp_switcheroo_v3_reprobe_4.3-rc4pre.tar.gz

Ok thanks

> Best regards,
>
> Lukas

Evan

-- 
Home
http://evanfoss.googlepages.com/
Work
http://forge.abcd.harvard.edu/gf/project/epl_engineering/wiki/
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v3 0/6] Enable gpu switching on the MacBook Pro

[Lukas Wunner]

Hi Evan,

On Mon, Oct 05, 2015 at 11:23:21AM -0400, Evan Foss wrote:
> $ dmesg|grep Apple
> [    0.000000] efi: EFI v1.10 by Apple
> [    0.000000] DMI: Apple Inc. MacBookPro9,1/Mac-
> 4B7AC7E43945597E,
> BIOS MBP91.88Z.00D3.B08.1208081132 08/08/2012

That was the last of the pre-retinas. I have exactly the same machine,
so yes, it should work just fine on that one.

Best regards,

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

Re: [PATCH v3 0/6] Enable gpu switching on the MacBook Pro

[Pierre Moreau]

The repo is a fork of Linus' tree, with the patches on top of it. So you just need to build that modified version of the kernel and boot it. :-)

Pierre

> On 05 Oct 2015, at 16:15, Evan Foss <evanfoss@gmail.com> wrote:
> 
>> On Mon, Oct 5, 2015 at 9:23 AM, Lukas Wunner <lukas@wunner.de> wrote:
>> Hi,
>> 
>> I've also pushed this series to GitHub now to ease reviewing:
>> https://github.com/l1k/linux/commits/mbp_switcheroo_v3
> 
> So to test this all someone has to do is pull this and try it? No
> patching required?
> 
>> Thanks,
>> 
>> Lukas
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/nouveau
> 
> 
> 
> -- 
> Home
> http://evanfoss.googlepages.com/
> Work
> http://forge.abcd.harvard.edu/gf/project/epl_engineering/wiki/
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau