[PATCH v5 00/12] Enable GPU switching on pre-retina MacBook Pro

[PATCH v5 00/12] Enable GPU switching on pre-retina MacBook Pro

[Lukas Wunner]

Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.

The main obstacle on these machines is that the panel mode in VBIOS
is bogus. Fortunately gmux can switch DDC independently from the
display, thereby allowing the inactive GPU to probe the panel's EDID.

In short, vga_switcheroo and apple-gmux are amended with hooks to
switch DDC, DRM core is amended with a drm_get_edid_switcheroo() helper,
and relevant drivers are amended to call that for LVDS outputs.

The retina MacBook Pro (2012 - present) uses eDP and cannot switch
AUX independently from the main link. The main obstacle there is link
training, I'm currently working on this, it will be addressed in a
future patch set.

This series is also reviewable on GitHub:
https://github.com/l1k/linux/commits/mbp_switcheroo_v5

Changes:

* New patch [01/12]: vga_switcheroo handler flags
  Alex Deucher asked if this series might regress on non-Apple laptops.
  To address this concern, I let handlers declare their capabilities in
  a bitmask. DRM drivers call drm_get_edid_switcheroo() only if the
  handler has set the VGA_SWITCHEROO_CAN_SWITCH_DDC flag.
  Currently just one other flag is defined which is used on retinas.

* Changed patch [02/12]: vga_switcheroo DDC locking
  Rename ddc_lock to mux_hw_lock, suggested by Daniel Vetter.

* New patch [03/12]: track switch state of apple-gmux
  Fixes a bug in previous versions of this series which occurred if
  the system was suspended while DDC was temporarily switched:
  On resume DDC was switched to the wrong GPU.

* New patches [09/12 - 12/12]: deferred probing
  Previously I used connector reprobing if the inactive GPU's driver
  loaded before gmux. I've ditched that in favor of deferred driver
  probing, which is much simpler. Thanks to Daniel Vetter for the
  suggestion.

Caution: Patch [09/12] depends on a new acpi_dev_present() API which
will land in 4.5 via Rafael J. Wysocki's tree.

I would particularly be interested in feedback on the handler flags
patch [01/12]. I'm not 100% happy with the number of characters
required to query the flags (e.g.: if (vga_switcheroo_handler_flags() &
VGA_SWITCHEROO_CAN_SWITCH_DDC)), but failed to come up with something
shorter. Thierry Reding used a struct of bools instead of a bitmask
for his recent drm_dp_link_caps patches. Maybe use that instead?
http://lists.freedesktop.org/archives/dri-devel/2015-December/097025.html

Thanks,

Lukas


Lukas Wunner (12):
  vga_switcheroo: Add handler flags infrastructure
  vga_switcheroo: Add support for switching only the DDC
  apple-gmux: Track switch state
  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
  apple-gmux: Add helper for presence detect
  drm/i915: Defer probe if gmux is present but its driver isn't
  drm/nouveau: Defer probe if gmux is present but its driver isn't
  drm/radeon: Defer probe if gmux is present but its driver isn't

 Documentation/DocBook/gpu.tmpl                   |   5 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c |   3 +-
 drivers/gpu/drm/drm_edid.c                       |  26 +++++
 drivers/gpu/drm/i915/i915_drv.c                  |  12 +++
 drivers/gpu/drm/i915/intel_lvds.c                |   8 +-
 drivers/gpu/drm/nouveau/nouveau_acpi.c           |   2 +-
 drivers/gpu/drm/nouveau/nouveau_connector.c      |  21 +++-
 drivers/gpu/drm/nouveau/nouveau_drm.c            |  11 +++
 drivers/gpu/drm/radeon/radeon_atpx_handler.c     |   3 +-
 drivers/gpu/drm/radeon/radeon_connectors.c       |   6 ++
 drivers/gpu/drm/radeon/radeon_drv.c              |  11 +++
 drivers/gpu/vga/vga_switcheroo.c                 | 119 ++++++++++++++++++++++-
 drivers/platform/x86/apple-gmux.c                | 111 ++++++++++++++++-----
 include/drm/drm_crtc.h                           |   2 +
 include/linux/apple-gmux.h                       |  39 ++++++++
 include/linux/vga_switcheroo.h                   |  36 ++++++-
 16 files changed, 382 insertions(+), 33 deletions(-)
 create mode 100644 include/linux/apple-gmux.h

-- 
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 v5 06/12] 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
if the vga_switcheroo handler is capable of temporarily switching
the panel's DDC lines to the integrated GPU. This allows us to
retrieve the EDID if the panel is currently muxed 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 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"]

v3: Commit newly added due to introduction of drm_get_edid_switcheroo()
    wrapper which drivers need to opt-in to.

v5: Rebase on "vga_switcheroo: Add handler flags infrastructure",
    i.e. call drm_get_edid_switcheroo() only if the handler
    indicates that DDC is switchable.

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/i915/intel_lvds.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 0da0240..811ddf7 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -31,6 +31,7 @@
 #include <linux/dmi.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
+#include <linux/vga_switcheroo.h>
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
@@ -1080,7 +1081,12 @@ 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));
+	if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC)
+		edid = drm_get_edid_switcheroo(connector,
+				    intel_gmbus_get_adapter(dev_priv, pin));
+	else
+		edid = drm_get_edid(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)

[PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

gmux is a microcontroller built into dual GPU MacBook Pros.
On pre-retina MBPs, if we're the inactive GPU, we need apple-gmux
to temporarily switch DDC so that we can probe the panel's EDID.

The checks for CONFIG_VGA_ARB and CONFIG_VGA_SWITCHEROO are necessary
because if either of them is disabled but gmux is present, the driver
would never load, even if we're the active GPU. (vga_default_device()
would evaluate to NULL and vga_switcheroo_handler_flags() would
evaluate to 0.)

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/i915/i915_drv.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3ac616d..4a5fc5d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,9 +35,12 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/vgaarb.h>
+#include <linux/vga_switcheroo.h>
 #include <drm/drm_crtc_helper.h>
 
 static struct drm_driver driver;
@@ -967,6 +970,15 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (PCI_FUNC(pdev->devfn))
 		return -ENODEV;
 
+	/*
+	 * apple-gmux is needed on dual GPU MacBook Pro
+	 * to probe the panel if we're the inactive GPU.
+	 */
+	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
+	    apple_gmux_present() && pdev != vga_default_device() &&
+	    !vga_switcheroo_handler_flags())
+		return -EPROBE_DEFER;
+
 	return drm_get_pci_dev(pdev, ent, &driver);
 }
 
-- 
1.8.5.2 (Apple Git-48)

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

Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Daniel Vetter]

On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
> gmux is a microcontroller built into dual GPU MacBook Pros.
> On pre-retina MBPs, if we're the inactive GPU, we need apple-gmux
> to temporarily switch DDC so that we can probe the panel's EDID.
> 
> The checks for CONFIG_VGA_ARB and CONFIG_VGA_SWITCHEROO are necessary
> because if either of them is disabled but gmux is present, the driver
> would never load, even if we're the active GPU. (vga_default_device()
> would evaluate to NULL and vga_switcheroo_handler_flags() would
> evaluate to 0.)
> 
> 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/i915/i915_drv.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3ac616d..4a5fc5d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -35,9 +35,12 @@
>  #include "i915_trace.h"
>  #include "intel_drv.h"
>  
> +#include <linux/apple-gmux.h>
>  #include <linux/console.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/vgaarb.h>
> +#include <linux/vga_switcheroo.h>
>  #include <drm/drm_crtc_helper.h>
>  
>  static struct drm_driver driver;
> @@ -967,6 +970,15 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (PCI_FUNC(pdev->devfn))
>  		return -ENODEV;
>  
> +	/*
> +	 * apple-gmux is needed on dual GPU MacBook Pro
> +	 * to probe the panel if we're the inactive GPU.
> +	 */
> +	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
> +	    apple_gmux_present() && pdev != vga_default_device() &&
> +	    !vga_switcheroo_handler_flags())
> +		return -EPROBE_DEFER;

I pulled in all patches to drm-misc, but this here is imo ugly and needs
to be polished a bit. What about adding a vga_switcheroo_ready() function
which contains this check (and might in the future contain even more
checks)? Then i915/radeon/nouveau would just have a simple

	if (!vga_switcheroo_ready())
		return -EPROBE_DEFER;

and instead of duplicating the same comment 3 times we could have it once
in one place. Plus some neat kerneldoc for this new helper to describe how
it's supposed to be used. Plus better encapsulation of concepts.

Can you pls follow up with a patch/series to do that?

Thanks, Daniel

> +
>  	return drm_get_pci_dev(pdev, ent, &driver);
>  }
>  
> -- 
> 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
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

Hi,

On Tue, Feb 09, 2016 at 10:04:01AM +0100, Daniel Vetter wrote:
> On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
[...]
> > @@ -967,6 +970,15 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >     if (PCI_FUNC(pdev->devfn))
> >             return -ENODEV;
> >  
> > +   /*
> > +    * apple-gmux is needed on dual GPU MacBook Pro
> > +    * to probe the panel if we're the inactive GPU.
> > +    */
> > +   if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
> > +       apple_gmux_present() && pdev != vga_default_device() &&
> > +       !vga_switcheroo_handler_flags())
> > +           return -EPROBE_DEFER;
> 
> I pulled in all patches to drm-misc, but this here is imo ugly and needs
> to be polished a bit. What about adding a vga_switcheroo_ready() function
> which contains this check (and might in the future contain even more
> checks)? Then i915/radeon/nouveau would just have a simple
> 
> 	if (!vga_switcheroo_ready())
> 		return -EPROBE_DEFER;
> 
> and instead of duplicating the same comment 3 times we could have it once
> in one place. Plus some neat kerneldoc for this new helper to describe how
> it's supposed to be used. Plus better encapsulation of concepts.
> 
> Can you pls follow up with a patch/series to do that?

You're right, this is indeed much better. It also allows me to drop the
IS_ENABLED(CONFIG_VGA_ARB) and IS_ENABLED(CONFIG_VGA_SWITCHEROO) checks.

A patch follows below after the scissors.

The name vga_switcheroo_ready() was already taken by a static function
in vga_switcheroo.c, so I've named it vga_switcheroo_client_probe_defer().
If anyone has a suggestion for a better name I'll be happy to amend the
patch.

I've switched all three drivers to the new helper within the same patch
but will gladly spin this out into one patch per driver if preferred.

Thank you!

Lukas

-- >8 --
Subject: [PATCH] vga_switcheroo: Allow clients to determine whether to defer
 probing

So far we've got one condition when DRM drivers need to defer probing
on a dual GPU system and it's coded separately into each of the relevant
drivers. As suggested by Daniel Vetter, deduplicate that code in the
drivers and move it to a new vga_switcheroo helper. This yields better
encapsulation of concepts and lets us add further checks in a central
place. (The existing check pertains to pre-retina MacBook Pros and an
additional check is expected to be needed for retinas.)

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/i915_drv.c       | 10 +---------
 drivers/gpu/drm/nouveau/nouveau_drm.c | 10 +---------
 drivers/gpu/drm/radeon/radeon_drv.c   | 10 +---------
 drivers/gpu/vga/vga_switcheroo.c      | 28 ++++++++++++++++++++++++++++
 include/linux/vga_switcheroo.h        |  2 ++
 5 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 44912ec..80cfd73 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,11 +35,9 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
-#include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 #include <drm/drm_crtc_helper.h>
 
@@ -972,13 +970,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (PCI_FUNC(pdev->devfn))
 		return -ENODEV;
 
-	/*
-	 * apple-gmux is needed on dual GPU MacBook Pro
-	 * to probe the panel if we're the inactive GPU.
-	 */
-	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-	    apple_gmux_present() && pdev != vga_default_device() &&
-	    !vga_switcheroo_handler_flags())
+	if (vga_switcheroo_client_probe_defer(pdev))
 		return -EPROBE_DEFER;
 
 	return drm_get_pci_dev(pdev, ent, &driver);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index bb8498c..9141bcd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,13 +22,11 @@
  * Authors: Ben Skeggs
  */
 
-#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/pm_runtime.h>
-#include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 
 #include "drmP.h"
@@ -314,13 +312,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
 	bool boot = false;
 	int ret;
 
-	/*
-	 * apple-gmux is needed on dual GPU MacBook Pro
-	 * to probe the panel if we're the inactive GPU.
-	 */
-	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-	    apple_gmux_present() && pdev != vga_default_device() &&
-	    !vga_switcheroo_handler_flags())
+	if (vga_switcheroo_client_probe_defer(pdev))
 		return -EPROBE_DEFER;
 
 	/* remove conflicting drivers (vesafb, efifb etc) */
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index cad2555..7be0c38 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -34,11 +34,9 @@
 #include "radeon_drv.h"
 
 #include <drm/drm_pciids.h>
-#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
-#include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 #include <drm/drm_gem.h>
 
@@ -321,13 +319,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
 {
 	int ret;
 
-	/*
-	 * apple-gmux is needed on dual GPU MacBook Pro
-	 * to probe the panel if we're the inactive GPU.
-	 */
-	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-	    apple_gmux_present() && pdev != vga_default_device() &&
-	    !vga_switcheroo_handler_flags())
+	if (vga_switcheroo_client_probe_defer(pdev))
 		return -EPROBE_DEFER;
 
 	/* Get rid of things like offb */
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index cbd7c98..a8cebd0 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -30,6 +30,7 @@
 
 #define pr_fmt(fmt) "vga_switcheroo: " fmt
 
+#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/debugfs.h>
 #include <linux/fb.h>
@@ -376,6 +377,33 @@ find_active_client(struct list_head *head)
 }
 
 /**
+ * vga_switcheroo_client_probe_defer() - whether to defer probing a given GPU
+ * @pdev: pci device of GPU
+ *
+ * Determine whether any prerequisites are not fulfilled to probe a given GPU.
+ * DRM drivers should invoke this early on in their ->probe callback and return
+ * %-EPROBE_DEFER if it evaluates to %true. The GPU need not be registered with
+ * vga_switcheroo_register_client() beforehand.
+ *
+ * Return: %false unless one of the following applies:
+ *
+ * * On pre-retina MacBook Pros, the apple-gmux driver is needed to temporarily
+ *   switch DDC to the inactive GPU so that it can probe the panel's EDID.
+ *   Therefore return %true if gmux is built into the machine, @pdev is the
+ *   inactive GPU and the apple-gmux driver has not registered its handler
+ *   flags, signifying it has not yet loaded or has not finished initializing.
+ */
+bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
+{
+	if (apple_gmux_present() && pdev != vga_default_device() &&
+	    !vgasr_priv.handler_flags)
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL(vga_switcheroo_client_probe_defer);
+
+/**
  * vga_switcheroo_get_client_state() - obtain power state of a given client
  * @pdev: client pci device
  *
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index b39a5f3..960bedb 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -165,6 +165,7 @@ int vga_switcheroo_unlock_ddc(struct pci_dev *pdev);
 
 int vga_switcheroo_process_delayed_switch(void);
 
+bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev);
 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);
@@ -188,6 +189,7 @@ static inline enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(v
 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_process_delayed_switch(void) { return 0; }
+static inline bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev) { return false; }
 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) {}
-- 
2.1.0

Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Daniel Vetter]

On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner <lukas@wunner.de> wrote:
>  /**
> + * vga_switcheroo_client_probe_defer() - whether to defer probing a given GPU
> + * @pdev: pci device of GPU
> + *
> + * Determine whether any prerequisites are not fulfilled to probe a given GPU.

I'd phrase this as "Determine whether the vgaswitcheroor support is
fully loaded" and drop the GPU part - it could be needed by snd
drivers eventually too.

> + * DRM drivers should invoke this early on in their ->probe callback and return
> + * %-EPROBE_DEFER if it evaluates to %true. The GPU need not be registered with
> + * vga_switcheroo_register_client() beforehand.

s/need not/must not/ ... is your native language German by any chance?

Aside from that ... should vga_switcheroo_register_client call this
helper instead directly and return the appropriate -EDEFER_PROBE
error? I realize for some drivers it might be way too late to unrol
from that point on, but e.g. i915 already uses -EDEFER_PROBE. And
calling it unconditionally will make sure that it's easier to notice
when people forget this. So maybe call it both from the register
functions, and export as a separate hook? And for i915 we should be
able to remove that early check entirely.

> + *
> + * Return: %false unless one of the following applies:
> + *
> + * * On pre-retina MacBook Pros, the apple-gmux driver is needed to temporarily
> + *   switch DDC to the inactive GPU so that it can probe the panel's EDID.
> + *   Therefore return %true if gmux is built into the machine, @pdev is the
> + *   inactive GPU and the apple-gmux driver has not registered its handler
> + *   flags, signifying it has not yet loaded or has not finished initializing.

I think the apple-specific comment here should be a separate comment
right above the if condition below - it doesn't explain the interface,
only one specific case. And we might grow more of those.
-Daniel

> + */
> +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
> +{
> +       if (apple_gmux_present() && pdev != vga_default_device() &&
> +           !vgasr_priv.handler_flags)
> +               return true;
> +
> +       return false;
> +}
> +EXPORT_SYMBOL(vga_switcheroo_client_probe_defer);
> +




-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

Hi,

On Sun, Feb 14, 2016 at 01:46:28PM +0100, Daniel Vetter wrote:
> On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner <lukas@wunner.de> wrote:
> >  /**
> > + * vga_switcheroo_client_probe_defer() - whether to defer probing a given GPU
> > + * @pdev: pci device of GPU
> > + *
> > + * Determine whether any prerequisites are not fulfilled to probe a given GPU.
> 
> I'd phrase this as "Determine whether the vgaswitcheroor support is
> fully loaded" and drop the GPU part - it could be needed by snd
> drivers eventually too.

Ok, I've rephrased the kerneldoc to refer to "client" instead of "GPU"
and moved the single existing check in an if block for
PCI_CLASS_DISPLAY_VGA devices.


> > + * DRM drivers should invoke this early on in their ->probe callback and return
> > + * %-EPROBE_DEFER if it evaluates to %true. The GPU need not be registered with
> > + * vga_switcheroo_register_client() beforehand.
> 
> s/need not/must not/ ... is your native language German by any chance?

In principle there's no harm in registering the client first,
then checking if probing should be deferred, as long as the
client is unregistered before deferring. Thus the language
above is intentionally "need not" (muss nicht) rather than
"must not" (darf nicht). I didn't want to mandate something
that isn't actually required. The above sentence is merely
an aid for driver developers who might be confused in which
order to call what.


> Aside from that ... should vga_switcheroo_register_client call this
> helper instead directly and return the appropriate -EDEFER_PROBE
> error? I realize for some drivers it might be way too late to unrol
> from that point on, but e.g. i915 already uses -EDEFER_PROBE. And
> calling it unconditionally will make sure that it's easier to notice
> when people forget this. So maybe call it both from the register
> functions, and export as a separate hook? And for i915 we should be
> able to remove that early check entirely.

I'm afraid that wouldn't be a good idea. The ->probe hook is
potentially called dozens of times during boot until it finally
succeeds and vga_switcheroo_register_client() is usually called
in a fairly late driver load stage. In i915, we already have a
working GEM at that point and an almost fully brought up GPU.
Repeating bring up and tear down dozens of times is a nice
stress test but nothing I'd inflict on production systems.
I imagine it would also severely impact boot time and
clutter the kernel log.

So a separate helper seems to be the only sensible option.


> > + *
> > + * Return: %false unless one of the following applies:
> > + *
> > + * * On pre-retina MacBook Pros, the apple-gmux driver is needed to temporarily
> > + *   switch DDC to the inactive GPU so that it can probe the panel's EDID.
> > + *   Therefore return %true if gmux is built into the machine, @pdev is the
> > + *   inactive GPU and the apple-gmux driver has not registered its handler
> > + *   flags, signifying it has not yet loaded or has not finished initializing.
> 
> I think the apple-specific comment here should be a separate comment
> right above the if condition below - it doesn't explain the interface,
> only one specific case. And we might grow more of those.

Ok, I've moved that to a comment.

Updated patch follows after the scissors & perforation.

Thanks for the feedback!

Lukas

-- >8 --
Subject: [PATCH] vga_switcheroo: Allow clients to determine whether to defer
 probing

So far we've got one condition when DRM drivers need to defer probing
on a dual GPU system and it's coded separately into each of the relevant
drivers. As suggested by Daniel Vetter, deduplicate that code in the
drivers and move it to a new vga_switcheroo helper. This yields better
encapsulation of concepts and lets us add further checks in a central
place. (The existing check pertains to pre-retina MacBook Pros and an
additional check is expected to be needed for retinas.)

v2: This helper could eventually be used by audio clients as well,
    so rephrase kerneldoc to refer to "client" instead of "GPU"
    and move the single existing check in an if block specific
    to PCI_CLASS_DISPLAY_VGA devices. Move documentation on
    that check from kerneldoc to a comment. (Daniel Vetter)

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/i915_drv.c       | 10 +---------
 drivers/gpu/drm/nouveau/nouveau_drm.c | 10 +---------
 drivers/gpu/drm/radeon/radeon_drv.c   | 10 +---------
 drivers/gpu/vga/vga_switcheroo.c      | 28 ++++++++++++++++++++++++++++
 include/linux/vga_switcheroo.h        |  2 ++
 5 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e8d0f17..01ef24a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,11 +35,9 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
-#include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 #include <drm/drm_crtc_helper.h>
 
@@ -970,13 +968,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (PCI_FUNC(pdev->devfn))
 		return -ENODEV;
 
-	/*
-	 * apple-gmux is needed on dual GPU MacBook Pro
-	 * to probe the panel if we're the inactive GPU.
-	 */
-	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-	    apple_gmux_present() && pdev != vga_default_device() &&
-	    !vga_switcheroo_handler_flags())
+	if (vga_switcheroo_client_probe_defer(pdev))
 		return -EPROBE_DEFER;
 
 	return drm_get_pci_dev(pdev, ent, &driver);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index bb8498c..9141bcd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,13 +22,11 @@
  * Authors: Ben Skeggs
  */
 
-#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/pm_runtime.h>
-#include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 
 #include "drmP.h"
@@ -314,13 +312,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
 	bool boot = false;
 	int ret;
 
-	/*
-	 * apple-gmux is needed on dual GPU MacBook Pro
-	 * to probe the panel if we're the inactive GPU.
-	 */
-	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-	    apple_gmux_present() && pdev != vga_default_device() &&
-	    !vga_switcheroo_handler_flags())
+	if (vga_switcheroo_client_probe_defer(pdev))
 		return -EPROBE_DEFER;
 
 	/* remove conflicting drivers (vesafb, efifb etc) */
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index cad2555..7be0c38 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -34,11 +34,9 @@
 #include "radeon_drv.h"
 
 #include <drm/drm_pciids.h>
-#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
-#include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 #include <drm/drm_gem.h>
 
@@ -321,13 +319,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
 {
 	int ret;
 
-	/*
-	 * apple-gmux is needed on dual GPU MacBook Pro
-	 * to probe the panel if we're the inactive GPU.
-	 */
-	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-	    apple_gmux_present() && pdev != vga_default_device() &&
-	    !vga_switcheroo_handler_flags())
+	if (vga_switcheroo_client_probe_defer(pdev))
 		return -EPROBE_DEFER;
 
 	/* Get rid of things like offb */
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 56287ae..6108ebe 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -30,6 +30,7 @@
 
 #define pr_fmt(fmt) "vga_switcheroo: " fmt
 
+#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/debugfs.h>
 #include <linux/fb.h>
@@ -375,6 +376,33 @@ find_active_client(struct list_head *head)
 }
 
 /**
+ * vga_switcheroo_client_probe_defer() - whether to defer probing a given client
+ * @pdev: client pci device
+ *
+ * Determine whether any prerequisites are not fulfilled to probe a given
+ * client. Drivers should invoke this early on in their ->probe callback
+ * and return %-EPROBE_DEFER if it evaluates to %true. The client need
+ * not be registered with vga_switcheroo_register_client() beforehand.
+ *
+ * Return: %true if probing should be deferred, otherwise %false.
+ */
+bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
+{
+	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) {
+		/*
+		 * apple-gmux is needed on pre-retina MacBook Pro
+		 * to probe the panel if pdev is the inactive GPU.
+		 */
+		if (apple_gmux_present() && pdev != vga_default_device() &&
+		    !vgasr_priv.handler_flags)
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(vga_switcheroo_client_probe_defer);
+
+/**
  * vga_switcheroo_get_client_state() - obtain power state of a given client
  * @pdev: client pci device
  *
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index b39a5f3..960bedb 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -165,6 +165,7 @@ int vga_switcheroo_unlock_ddc(struct pci_dev *pdev);
 
 int vga_switcheroo_process_delayed_switch(void);
 
+bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev);
 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);
@@ -188,6 +189,7 @@ static inline enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(v
 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_process_delayed_switch(void) { return 0; }
+static inline bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev) { return false; }
 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
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Daniel Vetter]

On Tue, Feb 16, 2016 at 04:58:20PM +0100, Lukas Wunner wrote:
> Hi,
> 
> On Sun, Feb 14, 2016 at 01:46:28PM +0100, Daniel Vetter wrote:
> > On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > >  /**
> > > + * vga_switcheroo_client_probe_defer() - whether to defer probing a given GPU
> > > + * @pdev: pci device of GPU
> > > + *
> > > + * Determine whether any prerequisites are not fulfilled to probe a given GPU.
> > 
> > I'd phrase this as "Determine whether the vgaswitcheroor support is
> > fully loaded" and drop the GPU part - it could be needed by snd
> > drivers eventually too.
> 
> Ok, I've rephrased the kerneldoc to refer to "client" instead of "GPU"
> and moved the single existing check in an if block for
> PCI_CLASS_DISPLAY_VGA devices.
> 
> 
> > > + * DRM drivers should invoke this early on in their ->probe callback and return
> > > + * %-EPROBE_DEFER if it evaluates to %true. The GPU need not be registered with
> > > + * vga_switcheroo_register_client() beforehand.
> > 
> > s/need not/must not/ ... is your native language German by any chance?
> 
> In principle there's no harm in registering the client first,
> then checking if probing should be deferred, as long as the
> client is unregistered before deferring. Thus the language
> above is intentionally "need not" (muss nicht) rather than
> "must not" (darf nicht). I didn't want to mandate something
> that isn't actually required. The above sentence is merely
> an aid for driver developers who might be confused in which
> order to call what.

I'd reject any driver that does this, registering, then checking, then
unregistering seems extermely unsafe. I'd really stick with mandatory
language here to make this clear.

> > Aside from that ... should vga_switcheroo_register_client call this
> > helper instead directly and return the appropriate -EDEFER_PROBE
> > error? I realize for some drivers it might be way too late to unrol
> > from that point on, but e.g. i915 already uses -EDEFER_PROBE. And
> > calling it unconditionally will make sure that it's easier to notice
> > when people forget this. So maybe call it both from the register
> > functions, and export as a separate hook? And for i915 we should be
> > able to remove that early check entirely.
> 
> I'm afraid that wouldn't be a good idea. The ->probe hook is
> potentially called dozens of times during boot until it finally
> succeeds and vga_switcheroo_register_client() is usually called
> in a fairly late driver load stage. In i915, we already have a
> working GEM at that point and an almost fully brought up GPU.
> Repeating bring up and tear down dozens of times is a nice
> stress test but nothing I'd inflict on production systems.
> I imagine it would also severely impact boot time and
> clutter the kernel log.
> 
> So a separate helper seems to be the only sensible option.

Ok, makes sense. I still think adding the check to the client_register
function would be good, just as a safety measure. And I don't think you're
case of register(), check(), unregister() in case of failure is a valid
use-case. Let's not allow anyone to open-code that horror ;-)

Cheers, Daniel

> > > + *
> > > + * Return: %false unless one of the following applies:
> > > + *
> > > + * * On pre-retina MacBook Pros, the apple-gmux driver is needed to temporarily
> > > + *   switch DDC to the inactive GPU so that it can probe the panel's EDID.
> > > + *   Therefore return %true if gmux is built into the machine, @pdev is the
> > > + *   inactive GPU and the apple-gmux driver has not registered its handler
> > > + *   flags, signifying it has not yet loaded or has not finished initializing.
> > 
> > I think the apple-specific comment here should be a separate comment
> > right above the if condition below - it doesn't explain the interface,
> > only one specific case. And we might grow more of those.
> 
> Ok, I've moved that to a comment.
> 
> Updated patch follows after the scissors & perforation.
> 
> Thanks for the feedback!
> 
> Lukas
> 
> -- >8 --
> Subject: [PATCH] vga_switcheroo: Allow clients to determine whether to defer
>  probing
> 
> So far we've got one condition when DRM drivers need to defer probing
> on a dual GPU system and it's coded separately into each of the relevant
> drivers. As suggested by Daniel Vetter, deduplicate that code in the
> drivers and move it to a new vga_switcheroo helper. This yields better
> encapsulation of concepts and lets us add further checks in a central
> place. (The existing check pertains to pre-retina MacBook Pros and an
> additional check is expected to be needed for retinas.)
> 
> v2: This helper could eventually be used by audio clients as well,
>     so rephrase kerneldoc to refer to "client" instead of "GPU"
>     and move the single existing check in an if block specific
>     to PCI_CLASS_DISPLAY_VGA devices. Move documentation on
>     that check from kerneldoc to a comment. (Daniel Vetter)
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/drm/i915/i915_drv.c       | 10 +---------
>  drivers/gpu/drm/nouveau/nouveau_drm.c | 10 +---------
>  drivers/gpu/drm/radeon/radeon_drv.c   | 10 +---------
>  drivers/gpu/vga/vga_switcheroo.c      | 28 ++++++++++++++++++++++++++++
>  include/linux/vga_switcheroo.h        |  2 ++
>  5 files changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e8d0f17..01ef24a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -35,11 +35,9 @@
>  #include "i915_trace.h"
>  #include "intel_drv.h"
>  
> -#include <linux/apple-gmux.h>
>  #include <linux/console.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> -#include <linux/vgaarb.h>
>  #include <linux/vga_switcheroo.h>
>  #include <drm/drm_crtc_helper.h>
>  
> @@ -970,13 +968,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (PCI_FUNC(pdev->devfn))
>  		return -ENODEV;
>  
> -	/*
> -	 * apple-gmux is needed on dual GPU MacBook Pro
> -	 * to probe the panel if we're the inactive GPU.
> -	 */
> -	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
> -	    apple_gmux_present() && pdev != vga_default_device() &&
> -	    !vga_switcheroo_handler_flags())
> +	if (vga_switcheroo_client_probe_defer(pdev))
>  		return -EPROBE_DEFER;
>  
>  	return drm_get_pci_dev(pdev, ent, &driver);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index bb8498c..9141bcd 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -22,13 +22,11 @@
>   * Authors: Ben Skeggs
>   */
>  
> -#include <linux/apple-gmux.h>
>  #include <linux/console.h>
>  #include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/pm_runtime.h>
> -#include <linux/vgaarb.h>
>  #include <linux/vga_switcheroo.h>
>  
>  #include "drmP.h"
> @@ -314,13 +312,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
>  	bool boot = false;
>  	int ret;
>  
> -	/*
> -	 * apple-gmux is needed on dual GPU MacBook Pro
> -	 * to probe the panel if we're the inactive GPU.
> -	 */
> -	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
> -	    apple_gmux_present() && pdev != vga_default_device() &&
> -	    !vga_switcheroo_handler_flags())
> +	if (vga_switcheroo_client_probe_defer(pdev))
>  		return -EPROBE_DEFER;
>  
>  	/* remove conflicting drivers (vesafb, efifb etc) */
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index cad2555..7be0c38 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -34,11 +34,9 @@
>  #include "radeon_drv.h"
>  
>  #include <drm/drm_pciids.h>
> -#include <linux/apple-gmux.h>
>  #include <linux/console.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> -#include <linux/vgaarb.h>
>  #include <linux/vga_switcheroo.h>
>  #include <drm/drm_gem.h>
>  
> @@ -321,13 +319,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
>  {
>  	int ret;
>  
> -	/*
> -	 * apple-gmux is needed on dual GPU MacBook Pro
> -	 * to probe the panel if we're the inactive GPU.
> -	 */
> -	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
> -	    apple_gmux_present() && pdev != vga_default_device() &&
> -	    !vga_switcheroo_handler_flags())
> +	if (vga_switcheroo_client_probe_defer(pdev))
>  		return -EPROBE_DEFER;
>  
>  	/* Get rid of things like offb */
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 56287ae..6108ebe 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -30,6 +30,7 @@
>  
>  #define pr_fmt(fmt) "vga_switcheroo: " fmt
>  
> +#include <linux/apple-gmux.h>
>  #include <linux/console.h>
>  #include <linux/debugfs.h>
>  #include <linux/fb.h>
> @@ -375,6 +376,33 @@ find_active_client(struct list_head *head)
>  }
>  
>  /**
> + * vga_switcheroo_client_probe_defer() - whether to defer probing a given client
> + * @pdev: client pci device
> + *
> + * Determine whether any prerequisites are not fulfilled to probe a given
> + * client. Drivers should invoke this early on in their ->probe callback
> + * and return %-EPROBE_DEFER if it evaluates to %true. The client need
> + * not be registered with vga_switcheroo_register_client() beforehand.
> + *
> + * Return: %true if probing should be deferred, otherwise %false.
> + */
> +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
> +{
> +	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) {
> +		/*
> +		 * apple-gmux is needed on pre-retina MacBook Pro
> +		 * to probe the panel if pdev is the inactive GPU.
> +		 */
> +		if (apple_gmux_present() && pdev != vga_default_device() &&
> +		    !vgasr_priv.handler_flags)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(vga_switcheroo_client_probe_defer);
> +
> +/**
>   * vga_switcheroo_get_client_state() - obtain power state of a given client
>   * @pdev: client pci device
>   *
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index b39a5f3..960bedb 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -165,6 +165,7 @@ int vga_switcheroo_unlock_ddc(struct pci_dev *pdev);
>  
>  int vga_switcheroo_process_delayed_switch(void);
>  
> +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev);
>  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);
> @@ -188,6 +189,7 @@ static inline enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(v
>  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_process_delayed_switch(void) { return 0; }
> +static inline bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev) { return false; }
>  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)
> 

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

Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

Hi,

On Tue, Feb 16, 2016 at 05:08:40PM +0100, Daniel Vetter wrote:
> On Tue, Feb 16, 2016 at 04:58:20PM +0100, Lukas Wunner wrote:
> > On Sun, Feb 14, 2016 at 01:46:28PM +0100, Daniel Vetter wrote:
> > > On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > > > + * DRM drivers should invoke this early on in their ->probe callback and return
> > > > + * %-EPROBE_DEFER if it evaluates to %true. The GPU need not be registered with
> > > > + * vga_switcheroo_register_client() beforehand.
> > > 
> > > s/need not/must not/ ... is your native language German by any chance?
> > 
> > In principle there's no harm in registering the client first,
> > then checking if probing should be deferred, as long as the
> > client is unregistered before deferring. Thus the language
> > above is intentionally "need not" (muss nicht) rather than
> > "must not" (darf nicht). I didn't want to mandate something
> > that isn't actually required. The above sentence is merely
> > an aid for driver developers who might be confused in which
> > order to call what.
> 
> I'd reject any driver that does this, registering, then checking, then
> unregistering seems extermely unsafe. I'd really stick with mandatory
> language here to make this clear.

Ok, I've made it mandatory in the kerneldoc, updated patch follows below.


> Ok, makes sense. I still think adding the check to the client_register
> function would be good, just as a safety measure.

Hm, the idea of calling vga_switcheroo_client_probe_defer() twice
causes me pain in the stomach. It's surprising for drivers which
just don't need it at the moment (amdgpu and snd_hda_intel) and
it feels like overengineering and pampering driver developers
beyond reasonable measure. Also while the single existing check is
cheap, we might later on add checks that take more time and slow
things down.

Best regards,

Lukas

-- >8 --
Subject: [PATCH] vga_switcheroo: Add helper for deferred probing

So far we've got one condition when DRM drivers need to defer probing
on a dual GPU system and it's coded separately into each of the relevant
drivers. As suggested by Daniel Vetter, deduplicate that code in the
drivers and move it to a new vga_switcheroo helper. This yields better
encapsulation of concepts and lets us add further checks in a central
place. (The existing check pertains to pre-retina MacBook Pros and an
additional check is expected to be needed for retinas.)

v2: This helper could eventually be used by audio clients as well,
    so rephrase kerneldoc to refer to "client" instead of "GPU"
    and move the single existing check in an if block specific
    to PCI_CLASS_DISPLAY_VGA devices. Move documentation on
    that check from kerneldoc to a comment. (Daniel Vetter)

v3: Mandate in kerneldoc that registration of client shall only
    happen after calling this helper. (Daniel Vetter)

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/i915_drv.c       | 10 +---------
 drivers/gpu/drm/nouveau/nouveau_drm.c | 10 +---------
 drivers/gpu/drm/radeon/radeon_drv.c   | 10 +---------
 drivers/gpu/vga/vga_switcheroo.c      | 34 ++++++++++++++++++++++++++++++++--
 include/linux/vga_switcheroo.h        |  2 ++
 5 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 44912ec..80cfd73 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,11 +35,9 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
-#include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 #include <drm/drm_crtc_helper.h>
 
@@ -972,13 +970,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (PCI_FUNC(pdev->devfn))
 		return -ENODEV;
 
-	/*
-	 * apple-gmux is needed on dual GPU MacBook Pro
-	 * to probe the panel if we're the inactive GPU.
-	 */
-	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-	    apple_gmux_present() && pdev != vga_default_device() &&
-	    !vga_switcheroo_handler_flags())
+	if (vga_switcheroo_client_probe_defer(pdev))
 		return -EPROBE_DEFER;
 
 	return drm_get_pci_dev(pdev, ent, &driver);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index bb8498c..9141bcd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,13 +22,11 @@
  * Authors: Ben Skeggs
  */
 
-#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/pm_runtime.h>
-#include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 
 #include "drmP.h"
@@ -314,13 +312,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
 	bool boot = false;
 	int ret;
 
-	/*
-	 * apple-gmux is needed on dual GPU MacBook Pro
-	 * to probe the panel if we're the inactive GPU.
-	 */
-	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-	    apple_gmux_present() && pdev != vga_default_device() &&
-	    !vga_switcheroo_handler_flags())
+	if (vga_switcheroo_client_probe_defer(pdev))
 		return -EPROBE_DEFER;
 
 	/* remove conflicting drivers (vesafb, efifb etc) */
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index cad2555..7be0c38 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -34,11 +34,9 @@
 #include "radeon_drv.h"
 
 #include <drm/drm_pciids.h>
-#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
-#include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 #include <drm/drm_gem.h>
 
@@ -321,13 +319,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
 {
 	int ret;
 
-	/*
-	 * apple-gmux is needed on dual GPU MacBook Pro
-	 * to probe the panel if we're the inactive GPU.
-	 */
-	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
-	    apple_gmux_present() && pdev != vga_default_device() &&
-	    !vga_switcheroo_handler_flags())
+	if (vga_switcheroo_client_probe_defer(pdev))
 		return -EPROBE_DEFER;
 
 	/* Get rid of things like offb */
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index cbd7c98..101e14c 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -30,6 +30,7 @@
 
 #define pr_fmt(fmt) "vga_switcheroo: " fmt
 
+#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/debugfs.h>
 #include <linux/fb.h>
@@ -308,7 +309,8 @@ static int register_client(struct pci_dev *pdev,
  *
  * Register vga client (GPU). Enable vga_switcheroo if another GPU and a
  * handler have already registered. The power state of the client is assumed
- * to be ON.
+ * to be ON. Beforehand, vga_switcheroo_client_probe_defer() shall be called
+ * to ensure that all prerequisites are met.
  *
  * Return: 0 on success, -ENOMEM on memory allocation error.
  */
@@ -329,7 +331,8 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
  * @id: client identifier
  *
  * Register audio client (audio device on a GPU). The power state of the
- * client is assumed to be ON.
+ * client is assumed to be ON. Beforehand, vga_switcheroo_client_probe_defer()
+ * shall be called to ensure that all prerequisites are met.
  *
  * Return: 0 on success, -ENOMEM on memory allocation error.
  */
@@ -376,6 +379,33 @@ find_active_client(struct list_head *head)
 }
 
 /**
+ * vga_switcheroo_client_probe_defer() - whether to defer probing a given client
+ * @pdev: client pci device
+ *
+ * Determine whether any prerequisites are not fulfilled to probe a given
+ * client. Drivers shall invoke this early on in their ->probe callback
+ * and return %-EPROBE_DEFER if it evaluates to %true. Thou shalt not
+ * register the client ere thou hast called this.
+ *
+ * Return: %true if probing should be deferred, otherwise %false.
+ */
+bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
+{
+	if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) {
+		/*
+		 * apple-gmux is needed on pre-retina MacBook Pro
+		 * to probe the panel if pdev is the inactive GPU.
+		 */
+		if (apple_gmux_present() && pdev != vga_default_device() &&
+		    !vgasr_priv.handler_flags)
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(vga_switcheroo_client_probe_defer);
+
+/**
  * vga_switcheroo_get_client_state() - obtain power state of a given client
  * @pdev: client pci device
  *
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index b39a5f3..960bedb 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -165,6 +165,7 @@ int vga_switcheroo_unlock_ddc(struct pci_dev *pdev);
 
 int vga_switcheroo_process_delayed_switch(void);
 
+bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev);
 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);
@@ -188,6 +189,7 @@ static inline enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(v
 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_process_delayed_switch(void) { return 0; }
+static inline bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev) { return false; }
 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)

Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Daniel Vetter]

On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner <lukas@wunner.de> wrote:
>
>> Ok, makes sense. I still think adding the check to the client_register
>> function would be good, just as a safety measure.
>
> Hm, the idea of calling vga_switcheroo_client_probe_defer() twice
> causes me pain in the stomach. It's surprising for drivers which
> just don't need it at the moment (amdgpu and snd_hda_intel) and
> it feels like overengineering and pampering driver developers
> beyond reasonable measure. Also while the single existing check is
> cheap, we might later on add checks that take more time and slow
> things down.

It's motivated by Rusty's API Manifesto:

http://sweng.the-davies.net/Home/rustys-api-design-manifesto

With the mandatory check in _register we reach level 5 - it'll blow up
at runtime when we try to register it. Without that the failure is
completely silent, and you need to read the right mailing list thread
(level 1), but at least the kerneldocs lift it up to level 3.

For more context: We have tons of fun with EPROBE_DEFER handling
between i915 and snd-hda, and I'm looking into all possible means to
make any api/subsystem using deferred probing as robust as possible by
default. One of the ideas is to inject deferred probe failures at
runtime, but that's kinda hard to do in a generic way. At least making
it as close to impossible to abuse as feasible is the next best
option.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

Hi,

On Thu, Feb 18, 2016 at 10:39:05PM +0100, Daniel Vetter wrote:
> On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner <lukas@wunner.de> wrote:
> >
> >> Ok, makes sense. I still think adding the check to the client_register
> >> function would be good, just as a safety measure.
> >
> > Hm, the idea of calling vga_switcheroo_client_probe_defer() twice
> > causes me pain in the stomach. It's surprising for drivers which
> > just don't need it at the moment (amdgpu and snd_hda_intel) and
> > it feels like overengineering and pampering driver developers
> > beyond reasonable measure. Also while the single existing check is
> > cheap, we might later on add checks that take more time and slow
> > things down.
> 
> It's motivated by Rusty's API Manifesto:
> 
> http://sweng.the-davies.net/Home/rustys-api-design-manifesto

Interesting, thank you.


> With the mandatory check in _register we reach level 5 - it'll blow up
> at runtime when we try to register it.

The manifesto says "5. Do it right or it will always break at runtime".

However even if we add a WARN_ON(vga_switcheroo_client_probe_defer(pdev))
to register_client(), it will not *always* spew a stacktrace but only on
the machines this concerns (MacBook Pros). Since failure to defer breaks
GPU switching, level 5 is already reached. Chances are this won't go
unnoticed by the user.


> For more context: We have tons of fun with EPROBE_DEFER handling
> between i915 and snd-hda

I don't understand, there is currently not a single occurrence of
EPROBE_DEFER in i915, apart from the one I added.

In sound/ there are 88 occurrences of EPROBE_DEFER in soc/, plus 1 in
ppc/ and that's it. So not a single one in pci/hda/ where hda_intel.c
resides.

Is the fun with EPROBE_DEFER handling caused by the lack thereof?


Best regards,

Lukas

Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Daniel Vetter]

On Thu, Feb 18, 2016 at 11:20 PM, Lukas Wunner <lukas@wunner.de> wrote:
> Hi,
>
> On Thu, Feb 18, 2016 at 10:39:05PM +0100, Daniel Vetter wrote:
>> On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner <lukas@wunner.de> wrote:
>> >
>> >> Ok, makes sense. I still think adding the check to the client_register
>> >> function would be good, just as a safety measure.
>> >
>> > Hm, the idea of calling vga_switcheroo_client_probe_defer() twice
>> > causes me pain in the stomach. It's surprising for drivers which
>> > just don't need it at the moment (amdgpu and snd_hda_intel) and
>> > it feels like overengineering and pampering driver developers
>> > beyond reasonable measure. Also while the single existing check is
>> > cheap, we might later on add checks that take more time and slow
>> > things down.
>>
>> It's motivated by Rusty's API Manifesto:
>>
>> http://sweng.the-davies.net/Home/rustys-api-design-manifesto
>
> Interesting, thank you.
>
>
>> With the mandatory check in _register we reach level 5 - it'll blow up
>> at runtime when we try to register it.
>
> The manifesto says "5. Do it right or it will always break at runtime".
>
> However even if we add a WARN_ON(vga_switcheroo_client_probe_defer(pdev))
> to register_client(), it will not *always* spew a stacktrace but only on
> the machines this concerns (MacBook Pros). Since failure to defer breaks
> GPU switching, level 5 is already reached. Chances are this won't go
> unnoticed by the user.

If we fail the register hopefully the driver checks for that and might
blow up somewhere in untested error handling code. But there's a good
chance it'll fail (we can encourage that more by adding must_check to
the function declaration). In that case you get a nice bug report with
splat from users hitting this.

Without this it'll silently work, and all the reports you get is
"linux is shit, gpu switching doesn't work".

In both cases it can sometimes succeed, which is not great indeed. But
I'm trying to fix that by injection EDEFER points artificially
somehow. Not yet figured out that one.

But irrespective of the precise failure mode making the defer check
mandatory by just including it in _register() is better since it makes
it impossible to forget to call it when its needed. So imo clearly the
more robust API. And that's my metric for evaluating new API - how
easy/hard is it to abuse/get wrong.

>> For more context: We have tons of fun with EPROBE_DEFER handling
>> between i915 and snd-hda
>
> I don't understand, there is currently not a single occurrence of
> EPROBE_DEFER in i915, apart from the one I added.
>
> In sound/ there are 88 occurrences of EPROBE_DEFER in soc/, plus 1 in
> ppc/ and that's it. So not a single one in pci/hda/ where hda_intel.c
> resides.
>
> Is the fun with EPROBE_DEFER handling caused by the lack thereof?

Yes, there's one instance where i915 shoudl defer missing. The real
trouble is that snd-hda has some really close ties with i915, and
resolves those with probe-defer. And blows up all the time since we
started using this, and with hdmi/dp you really always have to test
both together in CI, snd-hda is pretty much a part of the intel gfx
driver nowadays. Deferred probing is ime real trouble.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

RE: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't

[Deucher, Alexander]

> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Thursday, February 18, 2016 6:11 PM
> To: Lukas Wunner
> Cc: dri-devel; platform-driver-x86@vger.kernel.org; intel-gfx; Ben Skeggs;
> Deucher, Alexander
> Subject: Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but
> its driver isn't
> 
> On Thu, Feb 18, 2016 at 11:20 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > Hi,
> >
> > On Thu, Feb 18, 2016 at 10:39:05PM +0100, Daniel Vetter wrote:
> >> On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner <lukas@wunner.de>
> wrote:
> >> >
> >> >> Ok, makes sense. I still think adding the check to the client_register
> >> >> function would be good, just as a safety measure.
> >> >
> >> > Hm, the idea of calling vga_switcheroo_client_probe_defer() twice
> >> > causes me pain in the stomach. It's surprising for drivers which
> >> > just don't need it at the moment (amdgpu and snd_hda_intel) and
> >> > it feels like overengineering and pampering driver developers
> >> > beyond reasonable measure. Also while the single existing check is
> >> > cheap, we might later on add checks that take more time and slow
> >> > things down.
> >>
> >> It's motivated by Rusty's API Manifesto:
> >>
> >> http://sweng.the-davies.net/Home/rustys-api-design-manifesto
> >
> > Interesting, thank you.
> >
> >
> >> With the mandatory check in _register we reach level 5 - it'll blow up
> >> at runtime when we try to register it.
> >
> > The manifesto says "5. Do it right or it will always break at runtime".
> >
> > However even if we add a
> WARN_ON(vga_switcheroo_client_probe_defer(pdev))
> > to register_client(), it will not *always* spew a stacktrace but only on
> > the machines this concerns (MacBook Pros). Since failure to defer breaks
> > GPU switching, level 5 is already reached. Chances are this won't go
> > unnoticed by the user.
> 
> If we fail the register hopefully the driver checks for that and might
> blow up somewhere in untested error handling code. But there's a good
> chance it'll fail (we can encourage that more by adding must_check to
> the function declaration). In that case you get a nice bug report with
> splat from users hitting this.
> 
> Without this it'll silently work, and all the reports you get is
> "linux is shit, gpu switching doesn't work".
> 
> In both cases it can sometimes succeed, which is not great indeed. But
> I'm trying to fix that by injection EDEFER points artificially
> somehow. Not yet figured out that one.
> 
> But irrespective of the precise failure mode making the defer check
> mandatory by just including it in _register() is better since it makes
> it impossible to forget to call it when its needed. So imo clearly the
> more robust API. And that's my metric for evaluating new API - how
> easy/hard is it to abuse/get wrong.
> 
> >> For more context: We have tons of fun with EPROBE_DEFER handling
> >> between i915 and snd-hda
> >
> > I don't understand, there is currently not a single occurrence of
> > EPROBE_DEFER in i915, apart from the one I added.
> >
> > In sound/ there are 88 occurrences of EPROBE_DEFER in soc/, plus 1 in
> > ppc/ and that's it. So not a single one in pci/hda/ where hda_intel.c
> > resides.
> >
> > Is the fun with EPROBE_DEFER handling caused by the lack thereof?
> 
> Yes, there's one instance where i915 shoudl defer missing. The real
> trouble is that snd-hda has some really close ties with i915, and
> resolves those with probe-defer. And blows up all the time since we
> started using this, and with hdmi/dp you really always have to test
> both together in CI, snd-hda is pretty much a part of the intel gfx
> driver nowadays. Deferred probing is ime real trouble.

To further complicate things, AMD dGPUs have HDA audio on board as well.

Alex

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

[PATCH v5 11/12] drm/nouveau: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

gmux is a microcontroller built into dual GPU MacBook Pros.
On pre-retina MBPs, if we're the inactive GPU, we need apple-gmux
to temporarily switch DDC so that we can probe the panel's EDID.

The checks for CONFIG_VGA_ARB and CONFIG_VGA_SWITCHEROO are necessary
because if either of them is disabled but gmux is present, the driver
would never load, even if we're the active GPU. (vga_default_device()
would evaluate to NULL and vga_switcheroo_handler_flags() would
evaluate to 0.)

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_drm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 2f2f252..bb8498c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,11 +22,13 @@
  * Authors: Ben Skeggs
  */
 
+#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/pm_runtime.h>
+#include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 
 #include "drmP.h"
@@ -312,6 +314,15 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
 	bool boot = false;
 	int ret;
 
+	/*
+	 * apple-gmux is needed on dual GPU MacBook Pro
+	 * to probe the panel if we're the inactive GPU.
+	 */
+	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
+	    apple_gmux_present() && pdev != vga_default_device() &&
+	    !vga_switcheroo_handler_flags())
+		return -EPROBE_DEFER;
+
 	/* remove conflicting drivers (vesafb, efifb etc) */
 	aper = alloc_apertures(3);
 	if (!aper)
-- 
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 v5 08/12] 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
if the vga_switcheroo handler is capable of temporarily switching
the panel's DDC lines to the discrete GPU. This allows us to retrieve
the EDID if the panel is currently muxed 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 AMD:
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina  15"]
    [MBP  8,3 2011  intel SNB + amd turks     pre-retina  17"]

v3: Commit newly added due to introduction of drm_get_edid_switcheroo()
    wrapper which drivers need to opt-in to.

v5: Rebase on "vga_switcheroo: Add handler flags infrastructure",
    i.e. call drm_get_edid_switcheroo() only if the handler
    indicates that DDC is switchable.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/radeon/radeon_connectors.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index 340f3f5..cfcc099 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -34,6 +34,7 @@
 #include "atom.h"
 
 #include <linux/pm_runtime.h>
+#include <linux/vga_switcheroo.h>
 
 static int radeon_dp_handle_hpd(struct drm_connector *connector)
 {
@@ -344,6 +345,11 @@ 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 (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC &&
+		   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

[PATCH v5 12/12] drm/radeon: Defer probe if gmux is present but its driver isn't

[Lukas Wunner]

gmux is a microcontroller built into dual GPU MacBook Pros.
On pre-retina MBPs, if we're the inactive GPU, we need apple-gmux
to temporarily switch DDC so that we can probe the panel's EDID.

The checks for CONFIG_VGA_ARB and CONFIG_VGA_SWITCHEROO are necessary
because if either of them is disabled but gmux is present, the driver
would never load, even if we're the active GPU. (vga_default_device()
would evaluate to NULL and vga_switcheroo_handler_flags() would
evaluate to 0.)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/radeon/radeon_drv.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index e266ffc..cad2555 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -34,9 +34,11 @@
 #include "radeon_drv.h"
 
 #include <drm/drm_pciids.h>
+#include <linux/apple-gmux.h>
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 #include <drm/drm_gem.h>
 
@@ -319,6 +321,15 @@ static int radeon_pci_probe(struct pci_dev *pdev,
 {
 	int ret;
 
+	/*
+	 * apple-gmux is needed on dual GPU MacBook Pro
+	 * to probe the panel if we're the inactive GPU.
+	 */
+	if (IS_ENABLED(CONFIG_VGA_ARB) && IS_ENABLED(CONFIG_VGA_SWITCHEROO) &&
+	    apple_gmux_present() && pdev != vga_default_device() &&
+	    !vga_switcheroo_handler_flags())
+		return -EPROBE_DEFER;
+
 	/* Get rid of things like offb */
 	ret = radeon_kick_out_firmware_fb(pdev);
 	if (ret)
-- 
1.8.5.2 (Apple Git-48)

[PATCH v5 07/12] 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
if the vga_switcheroo handler is capable of temporarily switching
the panel's DDC lines to the discrete GPU. This allows us to retrieve
the EDID if the panel is currently muxed to the integrated GPU.
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"]

v3: Commit newly added due to introduction of drm_get_edid_switcheroo()
    wrapper which drivers need to opt-in to.

v5: Rebase on "vga_switcheroo: Add handler flags infrastructure",
    i.e. call drm_get_edid_switcheroo() only if the handler
    indicates that DDC is switchable.

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 | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index fcebfae..ae96ebc 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>
@@ -153,6 +154,17 @@ nouveau_connector_ddc_detect(struct drm_connector *connector)
 			if (ret == 0)
 				break;
 		} else
+		if ((vga_switcheroo_handler_flags() &
+		     VGA_SWITCHEROO_CAN_SWITCH_DDC) &&
+		    nv_encoder->dcb->type == DCB_OUTPUT_LVDS &&
+		    nv_encoder->i2c) {
+			int ret;
+			vga_switcheroo_lock_ddc(dev->pdev);
+			ret = nvkm_probe_i2c(nv_encoder->i2c, 0x50);
+			vga_switcheroo_unlock_ddc(dev->pdev);
+			if (ret)
+				break;
+		} else
 		if (nv_encoder->i2c) {
 			if (nvkm_probe_i2c(nv_encoder->i2c, 0x50))
 				break;
@@ -265,7 +277,14 @@ 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);
+		if ((vga_switcheroo_handler_flags() &
+		     VGA_SWITCHEROO_CAN_SWITCH_DDC) &&
+		    nv_connector->type == DCB_CONNECTOR_LVDS)
+			nv_connector->edid = drm_get_edid_switcheroo(connector,
+								     i2c);
+		else
+			nv_connector->edid = 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 v5 09/12] apple-gmux: Add helper for presence detect

[Lukas Wunner]

Centralize gmux' ACPI HID in a header file and add apple_gmux_present().
This can be used by other drivers to activate quirks specific to dual
GPU MacBook Pros & Mac Pros. The alternative would be to hardcode DMI
or PCI IDs and amend them whenever Apple introduces a new machine.

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>
---
 Documentation/DocBook/gpu.tmpl    |  4 ++++
 drivers/platform/x86/apple-gmux.c |  3 ++-
 include/linux/apple-gmux.h        | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 1 deletion(-)
 create mode 100644 include/linux/apple-gmux.h

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index 88e7c39..a5e6e3c 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -3677,6 +3677,10 @@ int num_ioctls;</synopsis>
         <title>Backlight control</title>
 !Pdrivers/platform/x86/apple-gmux.c Backlight control
       </sect2>
+      <sect2>
+        <title>Public functions</title>
+!Iinclude/linux/apple-gmux.h
+      </sect2>
     </sect1>
   </chapter>
 
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 1384a39..4034d2d 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -19,6 +19,7 @@
 #include <linux/acpi.h>
 #include <linux/pnp.h>
 #include <linux/apple_bl.h>
+#include <linux/apple-gmux.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/pci.h>
@@ -828,7 +829,7 @@ static void gmux_remove(struct pnp_dev *pnp)
 }
 
 static const struct pnp_device_id gmux_device_ids[] = {
-	{"APP000B", 0},
+	{GMUX_ACPI_HID, 0},
 	{"", 0}
 };
 
diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
new file mode 100644
index 0000000..feebc28
--- /dev/null
+++ b/include/linux/apple-gmux.h
@@ -0,0 +1,39 @@
+/*
+ * apple-gmux.h - microcontroller built into dual GPU MacBook Pro & Mac Pro
+ * Copyright (C) 2015 Lukas Wunner <lukas@wunner.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License (version 2) as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef LINUX_APPLE_GMUX_H
+#define LINUX_APPLE_GMUX_H
+
+#include <linux/acpi.h>
+
+#define GMUX_ACPI_HID "APP000B"
+
+/**
+ * apple_gmux_present() - detect if gmux is built into the machine
+ *
+ * Drivers may use this to activate quirks specific to dual GPU MacBook Pros
+ * and Mac Pros, e.g. for deferred probing, runtime pm and backlight.
+ *
+ * Return: %true if gmux is present and the kernel was configured
+ * with CONFIG_APPLE_GMUX, %false otherwise.
+ */
+static inline bool apple_gmux_present(void)
+{
+	return IS_ENABLED(CONFIG_APPLE_GMUX) && acpi_dev_present(GMUX_ACPI_HID);
+}
+
+#endif /* LINUX_APPLE_GMUX_H */
-- 
1.8.5.2 (Apple Git-48)

[PATCH v5 01/12] vga_switcheroo: Add handler flags infrastructure

[Lukas Wunner]

Allow handlers to declare their capabilities and allow clients to
obtain that information. So far we have these use cases:

* If the handler is able to switch DDC separately, clients need to
  probe EDID with drm_get_edid_switcheroo(). We should allow them
  to detect a capable handler to ensure this function only gets
  called when needed.

* Likewise if the handler is unable to switch AUX separately, the active
  client needs to communicate link training parameters to the inactive
  client, which may then skip the AUX handshake and set up its output
  with these pre-calibrated values (DisplayPort specification v1.1a,
  section 2.5.3.3). Clients need a way to recognize such a situation.

The flags for the radeon_atpx_handler and amdgpu_atpx_handler are
initially set to 0, this can later on be amended with
  handler_flags |= VGA_SWITCHEROO_CAN_SWITCH_DDC;
when a ->switch_ddc callback is added.

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>
---
 Documentation/DocBook/gpu.tmpl                   |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c |  3 ++-
 drivers/gpu/drm/nouveau/nouveau_acpi.c           |  2 +-
 drivers/gpu/drm/radeon/radeon_atpx_handler.c     |  3 ++-
 drivers/gpu/vga/vga_switcheroo.c                 | 22 ++++++++++++++++++-
 drivers/platform/x86/apple-gmux.c                |  2 +-
 include/linux/vga_switcheroo.h                   | 28 ++++++++++++++++++++++--
 7 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
index 9e95aa1..88e7c39 100644
--- a/Documentation/DocBook/gpu.tmpl
+++ b/Documentation/DocBook/gpu.tmpl
@@ -3648,6 +3648,7 @@ int num_ioctls;</synopsis>
     </sect1>
     <sect1>
       <title>Public constants</title>
+!Finclude/linux/vga_switcheroo.h vga_switcheroo_handler_flags_t
 !Finclude/linux/vga_switcheroo.h vga_switcheroo_client_id
 !Finclude/linux/vga_switcheroo.h vga_switcheroo_state
     </sect1>
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c
index 3c89586..fa948dc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c
@@ -552,13 +552,14 @@ static bool amdgpu_atpx_detect(void)
 void amdgpu_register_atpx_handler(void)
 {
 	bool r;
+	enum vga_switcheroo_handler_flags_t handler_flags = 0;
 
 	/* detect if we have any ATPX + 2 VGA in the system */
 	r = amdgpu_atpx_detect();
 	if (!r)
 		return;
 
-	vga_switcheroo_register_handler(&amdgpu_atpx_handler);
+	vga_switcheroo_register_handler(&amdgpu_atpx_handler, handler_flags);
 }
 
 /**
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index d5e6938..cdf5227 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -314,7 +314,7 @@ void nouveau_register_dsm_handler(void)
 	if (!r)
 		return;
 
-	vga_switcheroo_register_handler(&nouveau_dsm_handler);
+	vga_switcheroo_register_handler(&nouveau_dsm_handler, 0);
 }
 
 /* Must be called for Optimus models before the card can be turned off */
diff --git a/drivers/gpu/drm/radeon/radeon_atpx_handler.c b/drivers/gpu/drm/radeon/radeon_atpx_handler.c
index c4b4f29..56482e3 100644
--- a/drivers/gpu/drm/radeon/radeon_atpx_handler.c
+++ b/drivers/gpu/drm/radeon/radeon_atpx_handler.c
@@ -551,13 +551,14 @@ static bool radeon_atpx_detect(void)
 void radeon_register_atpx_handler(void)
 {
 	bool r;
+	enum vga_switcheroo_handler_flags_t handler_flags = 0;
 
 	/* detect if we have any ATPX + 2 VGA in the system */
 	r = radeon_atpx_detect();
 	if (!r)
 		return;
 
-	vga_switcheroo_register_handler(&radeon_atpx_handler);
+	vga_switcheroo_register_handler(&radeon_atpx_handler, handler_flags);
 }
 
 /**
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index d64d905..81da941 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -125,6 +125,7 @@ static DEFINE_MUTEX(vgasr_mutex);
  * 	(counting only vga clients, not audio clients)
  * @clients: list of registered clients
  * @handler: registered handler
+ * @handler_flags: flags of registered handler
  *
  * vga_switcheroo private data. Currently only one vga_switcheroo instance
  * per system is supported.
@@ -141,6 +142,7 @@ struct vgasr_priv {
 	struct list_head clients;
 
 	const struct vga_switcheroo_handler *handler;
+	enum vga_switcheroo_handler_flags_t handler_flags;
 };
 
 #define ID_BIT_AUDIO		0x100
@@ -189,13 +191,15 @@ static void vga_switcheroo_enable(void)
 /**
  * vga_switcheroo_register_handler() - register handler
  * @handler: handler callbacks
+ * @handler_flags: handler flags
  *
  * Register handler. Enable vga_switcheroo if two vga clients have already
  * registered.
  *
  * Return: 0 on success, -EINVAL if a handler was already registered.
  */
-int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler)
+int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler,
+				    enum vga_switcheroo_handler_flags_t handler_flags)
 {
 	mutex_lock(&vgasr_mutex);
 	if (vgasr_priv.handler) {
@@ -204,6 +208,7 @@ int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler
 	}
 
 	vgasr_priv.handler = handler;
+	vgasr_priv.handler_flags = handler_flags;
 	if (vga_switcheroo_ready()) {
 		pr_info("enabled\n");
 		vga_switcheroo_enable();
@@ -221,6 +226,7 @@ EXPORT_SYMBOL(vga_switcheroo_register_handler);
 void vga_switcheroo_unregister_handler(void)
 {
 	mutex_lock(&vgasr_mutex);
+	vgasr_priv.handler_flags = 0;
 	vgasr_priv.handler = NULL;
 	if (vgasr_priv.active) {
 		pr_info("disabled\n");
@@ -231,6 +237,20 @@ void vga_switcheroo_unregister_handler(void)
 }
 EXPORT_SYMBOL(vga_switcheroo_unregister_handler);
 
+/**
+ * vga_switcheroo_handler_flags() - obtain handler flags
+ *
+ * Helper for clients to obtain the handler flags bitmask.
+ *
+ * Return: Handler flags. A value of 0 means that no handler is registered
+ * or that the handler has no special capabilities.
+ */
+enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(void)
+{
+	return vgasr_priv.handler_flags;
+}
+EXPORT_SYMBOL(vga_switcheroo_handler_flags);
+
 static int register_client(struct pci_dev *pdev,
 			   const struct vga_switcheroo_client_ops *ops,
 			   enum vga_switcheroo_client_id id, bool active,
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index f236250..c401d49 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -705,7 +705,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	init_completion(&gmux_data->powerchange_done);
 	gmux_enable_interrupts(gmux_data);
 
-	if (vga_switcheroo_register_handler(&gmux_handler)) {
+	if (vga_switcheroo_register_handler(&gmux_handler, 0)) {
 		ret = -ENODEV;
 		goto err_register_handler;
 	}
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 69e1d4a1..a745f4f0 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -36,6 +36,26 @@
 struct pci_dev;
 
 /**
+ * enum vga_switcheroo_handler_flags_t - handler flags bitmask
+ * @VGA_SWITCHEROO_CAN_SWITCH_DDC: whether the handler is able to switch the
+ * 	DDC lines separately. This signals to clients that they should call
+ * 	drm_get_edid_switcheroo() to probe the EDID
+ * @VGA_SWITCHEROO_NEEDS_EDP_CONFIG: whether the handler is unable to switch
+ * 	the AUX channel separately. This signals to clients that the active
+ * 	GPU needs to train the link and communicate the link parameters to the
+ * 	inactive GPU (mediated by vga_switcheroo). The inactive GPU may then
+ * 	skip the AUX handshake and set up its output with these pre-calibrated
+ * 	values (DisplayPort specification v1.1a, section 2.5.3.3)
+ *
+ * Handler flags bitmask. Used by handlers to declare their capabilities upon
+ * registering with vga_switcheroo.
+ */
+enum vga_switcheroo_handler_flags_t {
+	VGA_SWITCHEROO_CAN_SWITCH_DDC	= (1 << 0),
+	VGA_SWITCHEROO_NEEDS_EDP_CONFIG	= (1 << 1),
+};
+
+/**
  * enum vga_switcheroo_state - client power state
  * @VGA_SWITCHEROO_OFF: off
  * @VGA_SWITCHEROO_ON: on
@@ -132,8 +152,10 @@ 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_register_handler(const struct vga_switcheroo_handler *handler);
+int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler,
+				    enum vga_switcheroo_handler_flags_t handler_flags);
 void vga_switcheroo_unregister_handler(void);
+enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(void);
 
 int vga_switcheroo_process_delayed_switch(void);
 
@@ -150,11 +172,13 @@ 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_register_handler(const struct vga_switcheroo_handler *handler) { return 0; }
+static inline int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler,
+		enum vga_switcheroo_handler_flags_t handler_flags) { return 0; }
 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 enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(void) { return 0; }
 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)

[PATCH v5 05/12] 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-04 - 2015-09:
    v3:   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
          (Thierry Reding, Daniel Vetter, Alex Deucher)

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 c214f12..1e8f660 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 3b040b3..65df8e7 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2278,6 +2278,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)

[PATCH v5 04/12] 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-04 - 2015-12:
    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.

    v2.4: Retain semantics of ->switchto handler callback to switch all
          pins, including DDC (Daniel Vetter)

    v4:   Advertise ->switch_ddc handler callback only on the pre-retina
          Macbook Pro. The retina uses eDP instead of LVDS and gmux no
          longer does the muxing itself but merely controls an external
          mux. That mux is incapable of switching the AUX channel
          separately from the main link. It's an NXP CBTL06142
          (alternate parts: TI HD3SS212, Pericom PI3VDP12412,
          see datasheets below).

    v5:   Rebase on "apple-gmux: Track switch state".
          Rebase on "vga_switcheroo: Add handler flags infrastructure".
          Rebase on 5d170139eb10 ("Constify vga_switcheroo_handler"),
          requires 2 structs, 1x with ->switchto for pre-retinas,
          1x without for retinas).
          Add error message if handler registration with vga_switcheroo
          fails.

    Teardowns identifying the mux:
    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/

    Mux 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 | 45 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 5c6c708..1384a39 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -417,6 +417,25 @@ static int gmux_switchto(enum vga_switcheroo_client_id id)
 	return 0;
 }
 
+static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
+{
+	enum vga_switcheroo_client_id old_ddc_owner =
+		apple_gmux_data->switch_state_ddc;
+
+	if (id == old_ddc_owner)
+		return id;
+
+	pr_debug("Switching DDC from %d to %d\n", old_ddc_owner, id);
+	apple_gmux_data->switch_state_ddc = 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 old_ddc_owner;
+}
+
 /**
  * DOC: Power control
  *
@@ -474,12 +493,19 @@ static int gmux_get_client_id(struct pci_dev *pdev)
 		return VGA_SWITCHEROO_DIS;
 }
 
-static const struct vga_switcheroo_handler gmux_handler = {
+static const struct vga_switcheroo_handler gmux_handler_indexed = {
 	.switchto = gmux_switchto,
 	.power_state = gmux_set_power_state,
 	.get_client_id = gmux_get_client_id,
 };
 
+static const struct vga_switcheroo_handler gmux_handler_classic = {
+	.switchto = gmux_switchto,
+	.switch_ddc = gmux_switch_ddc,
+	.power_state = gmux_set_power_state,
+	.get_client_id = gmux_get_client_id,
+};
+
 /**
  * DOC: Interrupt
  *
@@ -730,8 +756,21 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	gmux_enable_interrupts(gmux_data);
 	gmux_read_switch_state(gmux_data);
 
-	if (vga_switcheroo_register_handler(&gmux_handler, 0)) {
-		ret = -ENODEV;
+	/*
+	 * Retina MacBook Pros cannot switch the panel's AUX separately
+	 * and need eDP pre-calibration. They are distinguishable from
+	 * pre-retinas by having an "indexed" gmux.
+	 *
+	 * Pre-retina MacBook Pros can switch the panel's DDC separately.
+	 */
+	if (gmux_data->indexed)
+		ret = vga_switcheroo_register_handler(&gmux_handler_indexed,
+					      VGA_SWITCHEROO_NEEDS_EDP_CONFIG);
+	else
+		ret = vga_switcheroo_register_handler(&gmux_handler_classic,
+					      VGA_SWITCHEROO_CAN_SWITCH_DDC);
+	if (ret) {
+		pr_err("Failed to register vga_switcheroo handler\n");
 		goto err_register_handler;
 	}
 
-- 
1.8.5.2 (Apple Git-48)

[PATCH v5 02/12] 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-04 - 2015-10:
    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.

    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 mux_hw_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 acquire mux_hw_lock in stage2 to avoid race condition where
    reading the EDID and switching happens simultaneously. Likewise on
    MIGD / MDIS commands and on runtime suspend.

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

    v2.2: Readability improvements (Thierry Reding)

    v2.3: Overhaul locking once more

    v2.4: Retain semantics of ->switchto handler callback to switch all
          pins, including DDC (Daniel Vetter)

    v5:   Rename ddc_lock to mux_hw_lock: Since we acquire this both
          when calling ->switch_ddc and ->switchto, it protects not just
          access to the DDC lines but to the mux in general. This is in
          line with the DRM convention to use low-level locks to avoid
          concurrent hw access (e.g. i2c, dp_aux) which are often called
          hw_lock (Daniel Vetter)

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina  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 | 97 +++++++++++++++++++++++++++++++++++++++-
 include/linux/vga_switcheroo.h   |  8 ++++
 2 files changed, 103 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 81da941..56287ae 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -73,9 +73,17 @@
  * 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.
  */
 
 /**
@@ -126,6 +134,9 @@ static DEFINE_MUTEX(vgasr_mutex);
  * @clients: list of registered clients
  * @handler: registered handler
  * @handler_flags: flags of registered handler
+ * @mux_hw_lock: protects mux state
+ *	(in particular while DDC lines 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.
@@ -143,6 +154,8 @@ struct vgasr_priv {
 
 	const struct vga_switcheroo_handler *handler;
 	enum vga_switcheroo_handler_flags_t handler_flags;
+	struct mutex mux_hw_lock;
+	int old_ddc_owner;
 };
 
 #define ID_BIT_AUDIO		0x100
@@ -157,6 +170,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),
+	.mux_hw_lock = __MUTEX_INITIALIZER(vgasr_priv.mux_hw_lock),
 };
 
 static bool vga_switcheroo_ready(void)
@@ -226,6 +240,7 @@ EXPORT_SYMBOL(vga_switcheroo_register_handler);
 void vga_switcheroo_unregister_handler(void)
 {
 	mutex_lock(&vgasr_mutex);
+	mutex_lock(&vgasr_priv.mux_hw_lock);
 	vgasr_priv.handler_flags = 0;
 	vgasr_priv.handler = NULL;
 	if (vgasr_priv.active) {
@@ -233,6 +248,7 @@ void vga_switcheroo_unregister_handler(void)
 		vga_switcheroo_debugfs_fini(&vgasr_priv);
 		vgasr_priv.active = false;
 	}
+	mutex_unlock(&vgasr_priv.mux_hw_lock);
 	mutex_unlock(&vgasr_mutex);
 }
 EXPORT_SYMBOL(vga_switcheroo_unregister_handler);
@@ -432,6 +448,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.mux_hw_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.mux_hw_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.mux_hw_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
@@ -568,7 +654,9 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 		console_unlock();
 	}
 
+	mutex_lock(&vgasr_priv.mux_hw_lock);
 	ret = vgasr_priv.handler->switchto(new_client->id);
+	mutex_unlock(&vgasr_priv.mux_hw_lock);
 	if (ret)
 		return ret;
 
@@ -683,7 +771,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.mux_hw_lock);
 		ret = vgasr_priv.handler->switchto(client_id);
+		mutex_unlock(&vgasr_priv.mux_hw_lock);
 		goto out;
 	}
 
@@ -895,8 +985,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.mux_hw_lock);
 		vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD);
+		mutex_unlock(&vgasr_priv.mux_hw_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 a745f4f0..b39a5f3 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -102,6 +102,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.
@@ -113,6 +116,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);
@@ -156,6 +160,8 @@ int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler
 				    enum vga_switcheroo_handler_flags_t handler_flags);
 void vga_switcheroo_unregister_handler(void);
 enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(void);
+int vga_switcheroo_lock_ddc(struct pci_dev *pdev);
+int vga_switcheroo_unlock_ddc(struct pci_dev *pdev);
 
 int vga_switcheroo_process_delayed_switch(void);
 
@@ -179,6 +185,8 @@ static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 	enum vga_switcheroo_client_id id) { return 0; }
 static inline void vga_switcheroo_unregister_handler(void) {}
 static inline enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(void) { return 0; }
+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_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)

[PATCH v5 03/12] apple-gmux: Track switch state

[Lukas Wunner]

gmux has 3 switch registers:

* GMUX_PORT_SWITCH_DISPLAY switches the panel
* GMUX_PORT_SWITCH_DDC switches the panel's DDC lines
  (only on pre-retinas; on retinas this is a no-op)
* GMUX_PORT_SWITCH_EXTERNAL switches the external DP port(s)
  (only on models without Thunderbolt, i.e. introduced before 2011;
  those with Thunderbolt switch only HPD/AUX, not the main link)

Currently we switch all 3 registers in unison.

gmux does not preserve the switch state during suspend, so we currently
read GMUX_PORT_SWITCH_DISPLAY before suspend and restore all 3 registers
to this value on resume.

With the upcoming ->switch_ddc callback, GMUX_PORT_SWITCH_DDC may
temporarily contain a different value than the other 2 registers.
If we happen to suspend at this moment, we'll write an incorrect
value to GMUX_PORT_SWITCH_DDC on resume.

Also, on models with Thunderbolt the integrated GPU is unable to drive
the external DP port(s), so we want to keep GMUX_PORT_SWITCH_EXTERNAL
permanently switched to the discrete GPU on those machines.

Consequently we can no longer assume that GMUX_PORT_SWITCH_DISPLAY
represents the correct value for all 3 registers on suspend.

Track the state of all 3 registers: Add gmux_read_switch_state() and
gmux_write_switch_state(). Instead of reading the switch state on
every suspend, read it once on driver initialization so that we know
the current switch state all the time. (This allows us to use some
optimizations and shortcuts, e.g. we can skip switching DDC if we
know that it's already switched to the requested GPU.) Change the
->switchto callback to use gmux_write_switch_state().

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/platform/x86/apple-gmux.c | 67 +++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 21 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index c401d49..5c6c708 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -57,7 +57,9 @@ struct apple_gmux_data {
 	/* switcheroo data */
 	acpi_handle dhandle;
 	int gpe;
-	enum vga_switcheroo_client_id resume_client_id;
+	enum vga_switcheroo_client_id switch_state_display;
+	enum vga_switcheroo_client_id switch_state_ddc;
+	enum vga_switcheroo_client_id switch_state_external;
 	enum vga_switcheroo_state power_state;
 	struct completion powerchange_done;
 };
@@ -368,17 +370,49 @@ static const struct backlight_ops gmux_bl_ops = {
  * for the selected GPU.
  */
 
+static void gmux_read_switch_state(struct apple_gmux_data *gmux_data)
+{
+	if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_DDC) == 1)
+		gmux_data->switch_state_ddc = VGA_SWITCHEROO_IGD;
+	else
+		gmux_data->switch_state_ddc = VGA_SWITCHEROO_DIS;
+
+	if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_DISPLAY) == 2)
+		gmux_data->switch_state_display = VGA_SWITCHEROO_IGD;
+	else
+		gmux_data->switch_state_display = VGA_SWITCHEROO_DIS;
+
+	if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_EXTERNAL) == 2)
+		gmux_data->switch_state_external = VGA_SWITCHEROO_IGD;
+	else
+		gmux_data->switch_state_external = VGA_SWITCHEROO_DIS;
+}
+
+static void gmux_write_switch_state(struct apple_gmux_data *gmux_data)
+{
+	if (gmux_data->switch_state_ddc == VGA_SWITCHEROO_IGD)
+		gmux_write8(gmux_data, GMUX_PORT_SWITCH_DDC, 1);
+	else
+		gmux_write8(gmux_data, GMUX_PORT_SWITCH_DDC, 2);
+
+	if (gmux_data->switch_state_display == VGA_SWITCHEROO_IGD)
+		gmux_write8(gmux_data, GMUX_PORT_SWITCH_DISPLAY, 2);
+	else
+		gmux_write8(gmux_data, GMUX_PORT_SWITCH_DISPLAY, 3);
+
+	if (gmux_data->switch_state_external == VGA_SWITCHEROO_IGD)
+		gmux_write8(gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 2);
+	else
+		gmux_write8(gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3);
+}
+
 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);
-	}
+	apple_gmux_data->switch_state_ddc = id;
+	apple_gmux_data->switch_state_display = id;
+	apple_gmux_data->switch_state_external = id;
+
+	gmux_write_switch_state(apple_gmux_data);
 
 	return 0;
 }
@@ -440,15 +474,6 @@ static int gmux_get_client_id(struct pci_dev *pdev)
 		return VGA_SWITCHEROO_DIS;
 }
 
-static enum vga_switcheroo_client_id
-gmux_active_client(struct apple_gmux_data *gmux_data)
-{
-	if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_DISPLAY) == 2)
-		return VGA_SWITCHEROO_IGD;
-
-	return VGA_SWITCHEROO_DIS;
-}
-
 static const struct vga_switcheroo_handler gmux_handler = {
 	.switchto = gmux_switchto,
 	.power_state = gmux_set_power_state,
@@ -513,7 +538,6 @@ static int gmux_suspend(struct device *dev)
 	struct pnp_dev *pnp = to_pnp_dev(dev);
 	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
 
-	gmux_data->resume_client_id = gmux_active_client(gmux_data);
 	gmux_disable_interrupts(gmux_data);
 	return 0;
 }
@@ -524,7 +548,7 @@ static int gmux_resume(struct device *dev)
 	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
 
 	gmux_enable_interrupts(gmux_data);
-	gmux_switchto(gmux_data->resume_client_id);
+	gmux_write_switch_state(gmux_data);
 	if (gmux_data->power_state == VGA_SWITCHEROO_OFF)
 		gmux_set_discrete_state(gmux_data, gmux_data->power_state);
 	return 0;
@@ -704,6 +728,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	apple_gmux_data = gmux_data;
 	init_completion(&gmux_data->powerchange_done);
 	gmux_enable_interrupts(gmux_data);
+	gmux_read_switch_state(gmux_data);
 
 	if (vga_switcheroo_register_handler(&gmux_handler, 0)) {
 		ret = -ENODEV;
-- 
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 v5 00/12] Enable GPU switching on pre-retina MacBook Pro

[Lukas Wunner]

Hi,

On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
> Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.

This series hasn't seen any reviews or acks unfortunately.
Any takers?

Merging this would allow fdo #61115 to be closed
(currently assigned to intel-gfx).

FWIW this series has in the meantime been tested by more folks:

Tested-by: Pierre Moreau <pierre.morrow@free.fr>
    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  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"]

On the latter three models it worked fine. On Pierre Moreau's machine
the discrete nvidia G96 locks up when woken. This happened in the past
as well but not on the first wake but only on the 10th or so. Since it
works fine on the GT216 and GK107, I'm guessing we've got a regression
in the wakeup code for the G96 which is somehow triggered by this patch
set (more specifically: triggered by being able to retrieve the proper
panel resolution and configure a crtc). It needs to be fixed but isn't
a showstopper for this series IMHO. (Arguably being able to retrieve
EDID but then locking up on switching isn't really worse than not being
able to retrieve EDID in the first place.)

Thanks,

Lukas

> 
> The main obstacle on these machines is that the panel mode in VBIOS
> is bogus. Fortunately gmux can switch DDC independently from the
> display, thereby allowing the inactive GPU to probe the panel's EDID.
> 
> In short, vga_switcheroo and apple-gmux are amended with hooks to
> switch DDC, DRM core is amended with a drm_get_edid_switcheroo() helper,
> and relevant drivers are amended to call that for LVDS outputs.
> 
> The retina MacBook Pro (2012 - present) uses eDP and cannot switch
> AUX independently from the main link. The main obstacle there is link
> training, I'm currently working on this, it will be addressed in a
> future patch set.
> 
> This series is also reviewable on GitHub:
> https://github.com/l1k/linux/commits/mbp_switcheroo_v5
> 
> Changes:
> 
> * New patch [01/12]: vga_switcheroo handler flags
>   Alex Deucher asked if this series might regress on non-Apple laptops.
>   To address this concern, I let handlers declare their capabilities in
>   a bitmask. DRM drivers call drm_get_edid_switcheroo() only if the
>   handler has set the VGA_SWITCHEROO_CAN_SWITCH_DDC flag.
>   Currently just one other flag is defined which is used on retinas.
> 
> * Changed patch [02/12]: vga_switcheroo DDC locking
>   Rename ddc_lock to mux_hw_lock, suggested by Daniel Vetter.
> 
> * New patch [03/12]: track switch state of apple-gmux
>   Fixes a bug in previous versions of this series which occurred if
>   the system was suspended while DDC was temporarily switched:
>   On resume DDC was switched to the wrong GPU.
> 
> * New patches [09/12 - 12/12]: deferred probing
>   Previously I used connector reprobing if the inactive GPU's driver
>   loaded before gmux. I've ditched that in favor of deferred driver
>   probing, which is much simpler. Thanks to Daniel Vetter for the
>   suggestion.
> 
> Caution: Patch [09/12] depends on a new acpi_dev_present() API which
> will land in 4.5 via Rafael J. Wysocki's tree.
> 
> I would particularly be interested in feedback on the handler flags
> patch [01/12]. I'm not 100% happy with the number of characters
> required to query the flags (e.g.: if (vga_switcheroo_handler_flags() &
> VGA_SWITCHEROO_CAN_SWITCH_DDC)), but failed to come up with something
> shorter. Thierry Reding used a struct of bools instead of a bitmask
> for his recent drm_dp_link_caps patches. Maybe use that instead?
> http://lists.freedesktop.org/archives/dri-devel/2015-December/097025.html
> 
> Thanks,
> 
> Lukas
> 
> 
> Lukas Wunner (12):
>   vga_switcheroo: Add handler flags infrastructure
>   vga_switcheroo: Add support for switching only the DDC
>   apple-gmux: Track switch state
>   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
>   apple-gmux: Add helper for presence detect
>   drm/i915: Defer probe if gmux is present but its driver isn't
>   drm/nouveau: Defer probe if gmux is present but its driver isn't
>   drm/radeon: Defer probe if gmux is present but its driver isn't
> 
>  Documentation/DocBook/gpu.tmpl                   |   5 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c |   3 +-
>  drivers/gpu/drm/drm_edid.c                       |  26 +++++
>  drivers/gpu/drm/i915/i915_drv.c                  |  12 +++
>  drivers/gpu/drm/i915/intel_lvds.c                |   8 +-
>  drivers/gpu/drm/nouveau/nouveau_acpi.c           |   2 +-
>  drivers/gpu/drm/nouveau/nouveau_connector.c      |  21 +++-
>  drivers/gpu/drm/nouveau/nouveau_drm.c            |  11 +++
>  drivers/gpu/drm/radeon/radeon_atpx_handler.c     |   3 +-
>  drivers/gpu/drm/radeon/radeon_connectors.c       |   6 ++
>  drivers/gpu/drm/radeon/radeon_drv.c              |  11 +++
>  drivers/gpu/vga/vga_switcheroo.c                 | 119 ++++++++++++++++++++++-
>  drivers/platform/x86/apple-gmux.c                | 111 ++++++++++++++++-----
>  include/drm/drm_crtc.h                           |   2 +
>  include/linux/apple-gmux.h                       |  39 ++++++++
>  include/linux/vga_switcheroo.h                   |  36 ++++++-
>  16 files changed, 382 insertions(+), 33 deletions(-)
>  create mode 100644 include/linux/apple-gmux.h
> 
> -- 
> 1.8.5.2 (Apple Git-48)
> 
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina MacBook Pro

[Dave Airlie]

On 2 February 2016 at 08:49, Lukas Wunner <lukas@wunner.de> wrote:
> Hi,
>
> On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
>> Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
>
> This series hasn't seen any reviews or acks unfortunately.
> Any takers?

Has the tree this depends on been merged? I got these patches and applied
them to drm-next and found I needed some acpi patches.

Can you start pushing these to github or somewhere and putting the link
here?

Dave.

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina MacBook Pro

[Dave Airlie]

On 2 February 2016 at 11:10, Dave Airlie <airlied@gmail.com> wrote:
> On 2 February 2016 at 08:49, Lukas Wunner <lukas@wunner.de> wrote:
>> Hi,
>>
>> On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
>>> Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
>>
>> This series hasn't seen any reviews or acks unfortunately.
>> Any takers?
>
> Has the tree this depends on been merged? I got these patches and applied
> them to drm-next and found I needed some acpi patches.
>
> Can you start pushing these to github or somewhere and putting the link
> here?

just noticed you have a link, so it was just if the prereqs are
getting merged I'm interested in :-)

Dave.

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina MacBook Pro

[Lukas Wunner]

Hi Dave,

On Tue, Feb 02, 2016 at 11:10:19AM +1000, Dave Airlie wrote:
> On 2 February 2016 at 08:49, Lukas Wunner <lukas@wunner.de> wrote:
> > On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
> >> Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
> >
> > This series hasn't seen any reviews or acks unfortunately.
> > Any takers?
> 
> Has the tree this depends on been merged? I got these patches and applied
> them to drm-next and found I needed some acpi patches.

Ugh, sorry, I forgot to mention: The acpi_dev_present() API landed
in Linus' tree on January 13th, early on during the merge window.
So after Linus' tree gets merged back into drm-next, the patches
should compile just fine.

Thanks,

Lukas

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina MacBook Pro

[Pierre Moreau]

Hello all,

> On 01 Feb 2016, at 23:49, Lukas Wunner <lukas@wunner.de> wrote:
> 
> Hi,
> 
>> On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
>> Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
> 
> This series hasn't seen any reviews or acks unfortunately.
> Any takers?
> 
> Merging this would allow fdo #61115 to be closed
> (currently assigned to intel-gfx).
> 
> FWIW this series has in the meantime been tested by more folks:
> 
> Tested-by: Pierre Moreau <pierre.morrow@free.fr>
>    [MBP  5,3 2009  nvidia MCP79 + G96        pre-retina  15"]
> Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
>    [MBP  6,2 2010  intel ILK + nvidia GT216  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"]
> 
> On the latter three models it worked fine. On Pierre Moreau's machine
> the discrete nvidia G96 locks up when woken. This happened in the past
> as well but not on the first wake but only on the 10th or so. Since it
> works fine on the GT216 and GK107, I'm guessing we've got a regression
> in the wakeup code for the G96 which is somehow triggered by this patch
> set (more specifically: triggered by being able to retrieve the proper
> panel resolution and configure a crtc). It needs to be fixed but isn't
> a showstopper for this series IMHO. (Arguably being able to retrieve
> EDID but then locking up on switching isn't really worse than not being
> able to retrieve EDID in the first place.)

I would say it is slightly worse, since the only "downside" of not retrieving the EDID means  TTY is set to a default resolution rather than the screen resolution, but this is fixed when starting X.
On the other hand, since DRI_PRIME works fine on the laptop, there isn't much reason to switch between cards. 

I'll have a look at the resume this week, now that FOSDEM is off my todo list. 

Regards,
Pierre


> 
> Thanks,
> 
> Lukas
> 
>> 
>> The main obstacle on these machines is that the panel mode in VBIOS
>> is bogus. Fortunately gmux can switch DDC independently from the
>> display, thereby allowing the inactive GPU to probe the panel's EDID.
>> 
>> In short, vga_switcheroo and apple-gmux are amended with hooks to
>> switch DDC, DRM core is amended with a drm_get_edid_switcheroo() helper,
>> and relevant drivers are amended to call that for LVDS outputs.
>> 
>> The retina MacBook Pro (2012 - present) uses eDP and cannot switch
>> AUX independently from the main link. The main obstacle there is link
>> training, I'm currently working on this, it will be addressed in a
>> future patch set.
>> 
>> This series is also reviewable on GitHub:
>> https://github.com/l1k/linux/commits/mbp_switcheroo_v5
>> 
>> Changes:
>> 
>> * New patch [01/12]: vga_switcheroo handler flags
>>  Alex Deucher asked if this series might regress on non-Apple laptops.
>>  To address this concern, I let handlers declare their capabilities in
>>  a bitmask. DRM drivers call drm_get_edid_switcheroo() only if the
>>  handler has set the VGA_SWITCHEROO_CAN_SWITCH_DDC flag.
>>  Currently just one other flag is defined which is used on retinas.
>> 
>> * Changed patch [02/12]: vga_switcheroo DDC locking
>>  Rename ddc_lock to mux_hw_lock, suggested by Daniel Vetter.
>> 
>> * New patch [03/12]: track switch state of apple-gmux
>>  Fixes a bug in previous versions of this series which occurred if
>>  the system was suspended while DDC was temporarily switched:
>>  On resume DDC was switched to the wrong GPU.
>> 
>> * New patches [09/12 - 12/12]: deferred probing
>>  Previously I used connector reprobing if the inactive GPU's driver
>>  loaded before gmux. I've ditched that in favor of deferred driver
>>  probing, which is much simpler. Thanks to Daniel Vetter for the
>>  suggestion.
>> 
>> Caution: Patch [09/12] depends on a new acpi_dev_present() API which
>> will land in 4.5 via Rafael J. Wysocki's tree.
>> 
>> I would particularly be interested in feedback on the handler flags
>> patch [01/12]. I'm not 100% happy with the number of characters
>> required to query the flags (e.g.: if (vga_switcheroo_handler_flags() &
>> VGA_SWITCHEROO_CAN_SWITCH_DDC)), but failed to come up with something
>> shorter. Thierry Reding used a struct of bools instead of a bitmask
>> for his recent drm_dp_link_caps patches. Maybe use that instead?
>> http://lists.freedesktop.org/archives/dri-devel/2015-December/097025.html
>> 
>> Thanks,
>> 
>> Lukas
>> 
>> 
>> Lukas Wunner (12):
>>  vga_switcheroo: Add handler flags infrastructure
>>  vga_switcheroo: Add support for switching only the DDC
>>  apple-gmux: Track switch state
>>  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
>>  apple-gmux: Add helper for presence detect
>>  drm/i915: Defer probe if gmux is present but its driver isn't
>>  drm/nouveau: Defer probe if gmux is present but its driver isn't
>>  drm/radeon: Defer probe if gmux is present but its driver isn't
>> 
>> Documentation/DocBook/gpu.tmpl                   |   5 +
>> drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c |   3 +-
>> drivers/gpu/drm/drm_edid.c                       |  26 +++++
>> drivers/gpu/drm/i915/i915_drv.c                  |  12 +++
>> drivers/gpu/drm/i915/intel_lvds.c                |   8 +-
>> drivers/gpu/drm/nouveau/nouveau_acpi.c           |   2 +-
>> drivers/gpu/drm/nouveau/nouveau_connector.c      |  21 +++-
>> drivers/gpu/drm/nouveau/nouveau_drm.c            |  11 +++
>> drivers/gpu/drm/radeon/radeon_atpx_handler.c     |   3 +-
>> drivers/gpu/drm/radeon/radeon_connectors.c       |   6 ++
>> drivers/gpu/drm/radeon/radeon_drv.c              |  11 +++
>> drivers/gpu/vga/vga_switcheroo.c                 | 119 ++++++++++++++++++++++-
>> drivers/platform/x86/apple-gmux.c                | 111 ++++++++++++++++-----
>> include/drm/drm_crtc.h                           |   2 +
>> include/linux/apple-gmux.h                       |  39 ++++++++
>> include/linux/vga_switcheroo.h                   |  36 ++++++-
>> 16 files changed, 382 insertions(+), 33 deletions(-)
>> create mode 100644 include/linux/apple-gmux.h
>> 
>> -- 
>> 1.8.5.2 (Apple Git-48)
>> 
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina MacBook Pro

[Darren Hart]

On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
> Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
> 
> The main obstacle on these machines is that the panel mode in VBIOS
> is bogus. Fortunately gmux can switch DDC independently from the
> display, thereby allowing the inactive GPU to probe the panel's EDID.
> 
> In short, vga_switcheroo and apple-gmux are amended with hooks to
> switch DDC, DRM core is amended with a drm_get_edid_switcheroo() helper,
> and relevant drivers are amended to call that for LVDS outputs.
> 
> The retina MacBook Pro (2012 - present) uses eDP and cannot switch
> AUX independently from the main link. The main obstacle there is link
> training, I'm currently working on this, it will be addressed in a
> future patch set.
> 
> This series is also reviewable on GitHub:
> https://github.com/l1k/linux/commits/mbp_switcheroo_v5
> 
> Changes:
> 
> * New patch [01/12]: vga_switcheroo handler flags
>   Alex Deucher asked if this series might regress on non-Apple laptops.
>   To address this concern, I let handlers declare their capabilities in
>   a bitmask. DRM drivers call drm_get_edid_switcheroo() only if the
>   handler has set the VGA_SWITCHEROO_CAN_SWITCH_DDC flag.
>   Currently just one other flag is defined which is used on retinas.
> 
> * Changed patch [02/12]: vga_switcheroo DDC locking
>   Rename ddc_lock to mux_hw_lock, suggested by Daniel Vetter.
> 
> * New patch [03/12]: track switch state of apple-gmux
>   Fixes a bug in previous versions of this series which occurred if
>   the system was suspended while DDC was temporarily switched:
>   On resume DDC was switched to the wrong GPU.
> 
> * New patches [09/12 - 12/12]: deferred probing
>   Previously I used connector reprobing if the inactive GPU's driver
>   loaded before gmux. I've ditched that in favor of deferred driver
>   probing, which is much simpler. Thanks to Daniel Vetter for the
>   suggestion.
> 
> Caution: Patch [09/12] depends on a new acpi_dev_present() API which
> will land in 4.5 via Rafael J. Wysocki's tree.
> 
> I would particularly be interested in feedback on the handler flags
> patch [01/12]. I'm not 100% happy with the number of characters
> required to query the flags (e.g.: if (vga_switcheroo_handler_flags() &
> VGA_SWITCHEROO_CAN_SWITCH_DDC)), but failed to come up with something
> shorter. Thierry Reding used a struct of bools instead of a bitmask
> for his recent drm_dp_link_caps patches. Maybe use that instead?
> http://lists.freedesktop.org/archives/dri-devel/2015-December/097025.html

No objection from the platform-driver-x86 side. I can pull these separately once
the core is in, or these can be included with that core (preferred) with my
Reviewed-by for 1, 3, 4, and 9.

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

-- 
Darren Hart
Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina MacBook Pro

[Daniel Vetter]

On Mon, Feb 08, 2016 at 10:10:00AM -0800, Darren Hart wrote:
> On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
> > Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
> > 
> > The main obstacle on these machines is that the panel mode in VBIOS
> > is bogus. Fortunately gmux can switch DDC independently from the
> > display, thereby allowing the inactive GPU to probe the panel's EDID.
> > 
> > In short, vga_switcheroo and apple-gmux are amended with hooks to
> > switch DDC, DRM core is amended with a drm_get_edid_switcheroo() helper,
> > and relevant drivers are amended to call that for LVDS outputs.
> > 
> > The retina MacBook Pro (2012 - present) uses eDP and cannot switch
> > AUX independently from the main link. The main obstacle there is link
> > training, I'm currently working on this, it will be addressed in a
> > future patch set.
> > 
> > This series is also reviewable on GitHub:
> > https://github.com/l1k/linux/commits/mbp_switcheroo_v5
> > 
> > Changes:
> > 
> > * New patch [01/12]: vga_switcheroo handler flags
> >   Alex Deucher asked if this series might regress on non-Apple laptops.
> >   To address this concern, I let handlers declare their capabilities in
> >   a bitmask. DRM drivers call drm_get_edid_switcheroo() only if the
> >   handler has set the VGA_SWITCHEROO_CAN_SWITCH_DDC flag.
> >   Currently just one other flag is defined which is used on retinas.
> > 
> > * Changed patch [02/12]: vga_switcheroo DDC locking
> >   Rename ddc_lock to mux_hw_lock, suggested by Daniel Vetter.
> > 
> > * New patch [03/12]: track switch state of apple-gmux
> >   Fixes a bug in previous versions of this series which occurred if
> >   the system was suspended while DDC was temporarily switched:
> >   On resume DDC was switched to the wrong GPU.
> > 
> > * New patches [09/12 - 12/12]: deferred probing
> >   Previously I used connector reprobing if the inactive GPU's driver
> >   loaded before gmux. I've ditched that in favor of deferred driver
> >   probing, which is much simpler. Thanks to Daniel Vetter for the
> >   suggestion.
> > 
> > Caution: Patch [09/12] depends on a new acpi_dev_present() API which
> > will land in 4.5 via Rafael J. Wysocki's tree.
> > 
> > I would particularly be interested in feedback on the handler flags
> > patch [01/12]. I'm not 100% happy with the number of characters
> > required to query the flags (e.g.: if (vga_switcheroo_handler_flags() &
> > VGA_SWITCHEROO_CAN_SWITCH_DDC)), but failed to come up with something
> > shorter. Thierry Reding used a struct of bools instead of a bitmask
> > for his recent drm_dp_link_caps patches. Maybe use that instead?
> > http://lists.freedesktop.org/archives/dri-devel/2015-December/097025.html
> 
> No objection from the platform-driver-x86 side. I can pull these separately once
> the core is in, or these can be included with that core (preferred) with my
> Reviewed-by for 1, 3, 4, and 9.
> 
> Reviewed-by: Darren Hart <dvhart@linux.intel.com>

I pulled them all in through drm-misc, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Intel-gfx] [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Lukas Wunner]

Hi Bastien,

On Fri, Mar 04, 2016 at 04:12:27PM +0000, Bastien Nocera wrote:
> Lukas Wunner <lukas <at> wunner.de> writes:
> > Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
> 
> I've tested your patchset on a MacBookPro8,1, with an integrated Intel and
> discrete AMD/ATI GPUs.

Hm, it must be either an 8,2 or 8,3. The 8,1 was a 13" machine and only
had an integrated GPU.


> I've used the COPR repository here to cut down on my compilation time:
> https://copr.fedorainfracloud.org/coprs/firstyear/kernel-mbp/
> 
> I'm not certain how to test out your changes, or what the consequences should
> be on a stock Fedora 23/GNOME 3.18 installation. After booting (note that I
> did not change any command-line options in grub), a gnome-shell/gdm X11
> session comes up (I disabled Wayland, to rule out behavioural changes), I'd
> log in to GNOME and gnome-shell (which starts another X11 session on
> another VT).

Switching and power control currently requires manual intervention
by echoing commands to /sys/kernel/debug/vgaswitcheroo/switch
as documented here:
https://01.org/linuxgraphics/gfx-docs/drm/modes_of_use.html

As you've correctly observed, the machine is initially switched to
the discrete GPU and both GPUs are turned on. By echoing "IGD" to
the sysfs file, you'll switch to the integrated GPU and turn off
the discrete GPU.

It's possible to let the EFI firmware switch to the integrated GPU
on boot by using this tool: https://github.com/0xbb/gpu-switch
However still both GPUs will be powered up, so you have to issue
the "OFF" command to sysfs to power the discrete GPU down. Also,
once you boot into OS X, the setting made by the gpu-switch tool
will be overwritten and the machine will be switched to the discrete
GPU again the next time you boot Linux.

Note that switching is only possible from the text console, with
X11/Wayland shut down. Obviously this is not great in terms of UX.
A few years ago there was a GSoC proposal to get hot GPU switching
to work on Linux (akin to what OS X does) but nothing ever came of it:
http://www.phoronix.com/scan.php?page=news_item&px=OTIyMQ
https://lists.x.org/archives/xorg/2011-March/052522.html

Unfortunately this seems to be a low priority item for kernel graphics
developers since nowadays most dual GPU notebooks no longer have a mux
and cannot switch. The MacBook Pro seems to be the last one supporting
this but I've witnessed a bit of an anti-Apple sentiment among kernel
graphics developers since everything is non-standard there. Which is
unfortunate because these machines have a large market share and Apple
software quality is deteriorating rapidly so a lot of Mac users are
ripe for converting to Linux.

Anyway, one short-term improvement will be to add runtime pm support
(called "Driver power control" in the vga_switcheroo documentation
linked above). That way it'll no longer be necessary to power the
discrete GPU up and down manually, this will happen automatically
as needed (when switching or using render offloading with DRI PRIME).
I have patches to enable this for radeon but they're completely untested:

http://wunner.de/mbp_switcheroo_v5-4.5.tar.gz       => gpu switching for 4.5
http://wunner.de/mbp_switcheroo_v5-4.5-runpm.tar.gz => runtime pm for radeon

I have an Nvidia based machine and runtime pm doesn't work there yet
because of bugs in nouveau that I haven't had the time to look into.


> I did not use any external screens to test this.

Since your machine has Thunderbolt, the external port is no longer
switchable between GPUs, it can only be driven by the discrete GPU.
So you need to power it up manually for this to work. You don't need
to switch to it, but it's probably recommendable to save energy.
(Otherwise both GPUs are on with the integrated GPU driving the panel
and the discrete GPU driving the DP port.)

The runpm tarball linked above contains a patch to automatically
wake the discrete GPU on hotplug.

I've heard that the AMD GPU is picky about external monitors and
doesn't recognize them unless they're plugged in at exactly the
right moment, so you may need to retry a couple of times until it
works.

Best regards,

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

Re: [Intel-gfx] [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Bastien Nocera]

On Sat, 2016-03-05 at 15:16 +0100, Lukas Wunner wrote:
> Hi Bastien,
> 
> On Fri, Mar 04, 2016 at 04:12:27PM +0000, Bastien Nocera wrote:
> > 
> > Lukas Wunner <lukas <at> wunner.de> writes:
> > > 
> > > Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013),
> > > v5.
> > I've tested your patchset on a MacBookPro8,1, with an integrated
> > Intel and
> > discrete AMD/ATI GPUs.
> Hm, it must be either an 8,2 or 8,3. The 8,1 was a 13" machine and
> only
> had an integrated GPU.

Sorry, it's an "8,2". That's what I get for having not having my mail
on that machine.

> > 
> > I've used the COPR repository here to cut down on my compilation
> > time:
> > https://copr.fedorainfracloud.org/coprs/firstyear/kernel-mbp/
> > 
> > I'm not certain how to test out your changes, or what the
> > consequences should
> > be on a stock Fedora 23/GNOME 3.18 installation. After booting
> > (note that I
> > did not change any command-line options in grub), a gnome-shell/gdm 
> > X11
> > session comes up (I disabled Wayland, to rule out behavioural
> > changes), I'd
> > log in to GNOME and gnome-shell (which starts another X11 session
> > on
> > another VT).
> Switching and power control currently requires manual intervention
> by echoing commands to /sys/kernel/debug/vgaswitcheroo/switch
> as documented here:
> https://01.org/linuxgraphics/gfx-docs/drm/modes_of_use.html

Right, though I would intend on automating this.

> As you've correctly observed, the machine is initially switched to
> the discrete GPU and both GPUs are turned on. By echoing "IGD" to
> the sysfs file, you'll switch to the integrated GPU and turn off
> the discrete GPU.
> 
> It's possible to let the EFI firmware switch to the integrated GPU
> on boot by using this tool: https://github.com/0xbb/gpu-switch
> However still both GPUs will be powered up, so you have to issue
> the "OFF" command to sysfs to power the discrete GPU down. Also,
> once you boot into OS X, the setting made by the gpu-switch tool
> will be overwritten and the machine will be switched to the discrete
> GPU again the next time you boot Linux.

We could, on boot, force using the integrated GPU, turning off the
discrete GPU that we're not interested in.
So I'd push DIGD to the switch sysfs entry on boot. But I'm guessing
that won't turn off the other output we're not interested in.

> Note that switching is only possible from the text console, with
> X11/Wayland shut down. Obviously this is not great in terms of UX.
> A few years ago there was a GSoC proposal to get hot GPU switching
> to work on Linux (akin to what OS X does) but nothing ever came of
> it:
> http://www.phoronix.com/scan.php?page=news_item&px=OTIyMQ
> https://lists.x.org/archives/xorg/2011-March/052522.html
> 
> Unfortunately this seems to be a low priority item for kernel
> graphics
> developers since nowadays most dual GPU notebooks no longer have a
> mux
> and cannot switch. The MacBook Pro seems to be the last one
> supporting
> this but I've witnessed a bit of an anti-Apple sentiment among kernel
> graphics developers since everything is non-standard there. Which is
> unfortunate because these machines have a large market share and
> Apple
> software quality is deteriorating rapidly so a lot of Mac users are
> ripe for converting to Linux.

DIGD and DDIS should help (you need to log out/log in again), and if
the current GPU is the integrated one, you could force running, say, a
game on the discrete GPU using DRI_PRIME=1, correct?

FWIW, using OFF at runtime made my machine hang, and without any ways
for me to get debug output.

> Anyway, one short-term improvement will be to add runtime pm support
> (called "Driver power control" in the vga_switcheroo documentation
> linked above). That way it'll no longer be necessary to power the
> discrete GPU up and down manually, this will happen automatically
> as needed (when switching or using render offloading with DRI PRIME).

Ok, so using GIGD or DDIS would just switch the output, but not turn
off the unused GPU without runtime PM management.

> I have patches to enable this for radeon but they're completely
> untested:
> 
> http://wunner.de/mbp_switcheroo_v5-4.5.tar.gz       => gpu switching
> for 4.5

That's the same patch that's already applied to the kernel above (the
ones from this patchset thread), right?

> http://wunner.de/mbp_switcheroo_v5-4.5-runpm.tar.gz => runtime pm for
> radeon

OK, will test those out.

> I have an Nvidia based machine and runtime pm doesn't work there yet
> because of bugs in nouveau that I haven't had the time to look into.
> 
> 
> > 
> > I did not use any external screens to test this.
> Since your machine has Thunderbolt, the external port is no longer
> switchable between GPUs, it can only be driven by the discrete GPU.
> So you need to power it up manually for this to work. You don't need
> to switch to it, but it's probably recommendable to save energy.
> (Otherwise both GPUs are on with the integrated GPU driving the panel
> and the discrete GPU driving the DP port.)
> 
> The runpm tarball linked above contains a patch to automatically
> wake the discrete GPU on hotplug.
> 
> I've heard that the AMD GPU is picky about external monitors and
> doesn't recognize them unless they're plugged in at exactly the
> right moment, so you may need to retry a couple of times until it
> works.

To sum up, and if we take the above patches into consideration:
- on boot, one or more GPUs are powered on, an init script would queue
a GPU switch to the integrated one. The other GPU would be switched off
(automatically?)
- when X/wayland is running, queue the requests using DIGD or DDIS. If
the integrated GPU is used, allow offloading to the discrete GPU with
DRI_PRIME (which will again power up automatically thanks to the
runtime PM patches above).

My concerns here would be:
- Do we know how to turn off the integrated GPU automatically, if the
user only used the internal screen and wanted to use the discrete GPU?
- If only the discrete GPU is powered on, do we know how to power on
the integrated GPU so it can drive the external screen when plugged in?
- While plymouth is short-lived at boot, Wayland and X11 GNOME sessions
use the GPU. The first user session will run on a VT that's separate
from the greeter to implement fast-user switching. So, if we wanted to
force using the other GPU for future sessions, we'd need to tell gdm to
kill its graphical session and to respawn it so it doesn't hog the GPU
and avoid the switch happening. Correct?

FWIW, this is what I had written down a couple of months ago, about
supporting dual-GPU computers with GNOME:
https://wiki.gnome.org/Design/OS/DualGPU

Those use-cases are a lot simpler than what could be achieved with the
vga_switcheroo sub-system, but they probably cover the "90%" use cases
we're interested in.

Once I've managed to get the MacBook Pro into a good state, I also have
a Lenovo machine around with dual GPU, though I couldn't tell you what
the discrete one is.

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

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Dave Airlie]

>
> To sum up, and if we take the above patches into consideration:
> - on boot, one or more GPUs are powered on, an init script would queue
> a GPU switch to the integrated one. The other GPU would be switched off
> (automatically?)
> - when X/wayland is running, queue the requests using DIGD or DDIS. If
> the integrated GPU is used, allow offloading to the discrete GPU with
> DRI_PRIME (which will again power up automatically thanks to the
> runtime PM patches above).

> My concerns here would be:
> - Do we know how to turn off the integrated GPU automatically, if the
> user only used the internal screen and wanted to use the discrete GPU?
> - If only the discrete GPU is powered on, do we know how to power on
> the integrated GPU so it can drive the external screen when plugged in?
> - While plymouth is short-lived at boot, Wayland and X11 GNOME sessions
> use the GPU. The first user session will run on a VT that's separate
> from the greeter to implement fast-user switching. So, if we wanted to
> force using the other GPU for future sessions, we'd need to tell gdm to
> kill its graphical session and to respawn it so it doesn't hog the GPU
> and avoid the switch happening. Correct?
>
> FWIW, this is what I had written down a couple of months ago, about
> supporting dual-GPU computers with GNOME:
> https://wiki.gnome.org/Design/OS/DualGPU
>
> Those use-cases are a lot simpler than what could be achieved with the
> vga_switcheroo sub-system, but they probably cover the "90%" use cases
> we're interested in.
>
> Once I've managed to get the MacBook Pro into a good state, I also have
> a Lenovo machine around with dual GPU, though I couldn't tell you what
> the discrete one is.

Okay so I'm not sure you are heading in the best direction here.

My first suggestion is to stop using the MBP, start using the Lenovo.
At least from a Fedora perspective, that is the hw we have more installs of and
care more about.

Apple HW is not the same as PC hw in this case and we aren't going to achieve
the same level of integration that OSX has, not without some serious rewrites of
mutter and the whole X stack.

You shouldn't be caring about the MUX. MUXed hw apart from the MBP is pretty
much gone since Windows 7 timeframes. So I don't think we should be putting
too much effort into the MUX yet. With the current way we keep gdm running,
we can't do MUX switch on logout anymore. It was only ever a hack. So I'd just
not send commands to vga switcheroo at all.

So I'm missing what the overall goal here is. To provide better
support for dual-gpu
laptops and hotpluggable USB devices in the DE?

Under X, Fedora carries a server patch to autoconfigure providers,
we'd need to drop
that and have something in the DE notice when a new provider shows up
and configures it,
perhaps something to allow removal of providers that are already bound
(so we could detach
a secondary GPU for boxes to passthrough).

Then we need something in the DE to allow us to launch or have some
app info that would
decide to launch certain 3D using apps on the more powerful processor.
However since
nouveau doesn't quite reclock most of the secondary GPUs that can
often end up not being
that much more powerful.

We also want reverse prime to work properly, so if you plug in an
external monitor to
a port connected to the secondary GPU that we can pick it up and
configure it just like
all the other monitors.

As for the MBP, if we want to spend time chasing the rainbow of OS X,
then we've a lot of work
to do. OSX can smoothly switch the compositor from rendering on the
intel gpu to the nvidia
gpu in a vblank. It's truly seamless. To do that we'd need to a) move
to wayland, b) get mutter
to be a lot smarter than mutter currently is.

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

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Bastien Nocera]

Hey Dave,

On Thu, 2016-03-10 at 09:30 +1000, Dave Airlie wrote:
<snip>
> Okay so I'm not sure you are heading in the best direction here.
> 
> My first suggestion is to stop using the MBP, start using the Lenovo.
> At least from a Fedora perspective, that is the hw we have more
> installs of and
> care more about.

The Lenovo has an NVidia GPU, and there's no runtime PM support for
nouveau.

> Apple HW is not the same as PC hw in this case and we aren't going to
> achieve
> the same level of integration that OSX has, not without some serious
> rewrites of
> mutter and the whole X stack.

That's not the target goals. Did you read the wiki page I pointed to
listing the goals?

https://wiki.gnome.org/Design/OS/DualGPU

> You shouldn't be caring about the MUX.

I never talked about the MUX, didn't plan on using it either.

<snip>

> So I'm missing what the overall goal here is. To provide better
> support for dual-gpu
> laptops and hotpluggable USB devices in the DE?

Just dual-GPU devices for now.

I'd be interested in supporting USB displays, but I only have
proprietary drivers for my USB3 DisplayLink dock, and possibly
networked display devices, but the AirTame I have is also still using
an undocumented protocol.

> Under X, Fedora carries a server patch to autoconfigure providers,
> we'd need to drop
> that and have something in the DE notice when a new provider shows up
> and configures it,
> perhaps something to allow removal of providers that are already
> bound
> (so we could detach
> a secondary GPU for boxes to passthrough).

I'd rather have that be automated so that Boxes can tell you what is
using the 2nd GPU, not requiring any manual intervention.

> Then we need something in the DE to allow us to launch or have some
> app info that would
> decide to launch certain 3D using apps on the more powerful
> processor.

That's what I started working on, exporting the fact that 2 GPUs are
available through a D-Bus service, which also ensures that we only 

> However since
> nouveau doesn't quite reclock most of the secondary GPUs that can
> often end up not being
> that much more powerful.

There are supported laptops with Radeon GPUs as well, not sure whether
that's more powerful.

> We also want reverse prime to work properly, so if you plug in an
> external monitor to
> a port connected to the secondary GPU that we can pick it up and
> configure it just like
> all the other monitors.

I don't think I have any hardware that works this way.

> As for the MBP, if we want to spend time chasing the rainbow of OS X,
> then we've a lot of work
> to do. OSX can smoothly switch the compositor from rendering on the
> intel gpu to the nvidia
> gpu in a vblank. It's truly seamless. To do that we'd need to a) move
> to wayland, b) get mutter
> to be a lot smarter than mutter currently is.

That's not what I'm aiming for right now.

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

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Bastien Nocera]

On Thu, 2016-03-10 at 16:29 +0100, Bastien Nocera wrote:
> > Then we need something in the DE to allow us to launch or have some
> > app info that would
> > decide to launch certain 3D using apps on the more powerful
> > processor.
> 
> That's what I started working on, exporting the fact that 2 GPUs are
> available through a D-Bus service, which also ensures that we only 

And I missed a bit of text there:

which also ensures that we only enable and use the internal GPU by
default:
https://github.com/hadess/switcheroo-control/blob/master/src/switcheroo-control.c#L196

The full D-Bus service is at:
https://github.com/hadess/switcheroo-control

I guess I should start with disabling the Fedora specific X patch to
autoconfigure outputs and start adding code to mutter to set those up.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Lukas Wunner]

Hi Bastien,

sorry for the delay.

On Sat, Mar 05, 2016 at 05:31:55PM +0100, Bastien Nocera wrote:
> We could, on boot, force using the integrated GPU, turning off the
> discrete GPU that we're not interested in.

Yes, many people "solve" this by having grub write the requisite commands
to gmux' I/O ports, however this approach won't work with gummiboot.
Also, the commands that need to be sent to gmux differ between retinas
and pre-retinas.


> So I'd push DIGD to the switch sysfs entry on boot. But I'm guessing
> that won't turn off the other output we're not interested in.

IGD and DIGD switch to the integrated GPU and also turn off the discrete
GPU. However if the machine is already switched to the integrated GPU,
the commands are no-ops and the discrete GPU is not turned off.

In other words you need to check (by reading the switch file) which GPU
the machine is switched to. If it's the integrated GPU, write OFF to the
switch file. If it's the discrete GPU, write DIGD to the switch file.

Once runtime pm works, writing DIGD is sufficient.


> DIGD and DDIS should help (you need to log out/log in again), and if
> the current GPU is the integrated one, you could force running, say, a
> game on the discrete GPU using DRI_PRIME=1, correct?

If runtime pm works, then yes. Otherwise you'd have to manually power up
the GPU before using DRI_PRIME=1 and power it down afterwards.


> FWIW, using OFF at runtime made my machine hang, and without any ways
> for me to get debug output.

Which GPU were you switched to? Did you issue the command while X11 was
running? Since the machine is switched to the AMD on boot, I guess you
were powering down the Intel GPU. This works on my machine, I get a log
entry "i915: switched off". If this doesn't work on your machine it might
be an i915 bug on your Sandy Bridge GPU or it might be because X11 is
running.

Try booting with "drm.debug=0xf log_buf_len=10M" and see if this gets you
log output. If it doesn't, netconsole might help if the hang occurs while
the console is locked.


> Ok, so using GIGD or DDIS would just switch the output, but not turn
> off the unused GPU without runtime PM management.

No. They do switch off the other GPU if runtime pm is disabled.


> > http://wunner.de/mbp_switcheroo_v5-4.5.tar.gz       => gpu switching
> > for 4.5
> That's the same patch that's already applied to the kernel above (the
> ones from this patchset thread), right?

I'm not sure, the patches in the copr repository might be an earlier
test version.


> My concerns here would be:
> - Do we know how to turn off the integrated GPU automatically, if the
> user only used the internal screen and wanted to use the discrete GPU?

Runtime pm is currently disabled by default for i915. The Intel folks
are on it. Until that works, the integrated GPU will be powered down
when the user manually switches to the discrete GPU with DIS / DDIS
and powered up when switching back with IGD / DIGD.


> - If only the discrete GPU is powered on, do we know how to power on
> the integrated GPU so it can drive the external screen when plugged in?

On the MBP5 2008/09 and MBP6 2010, the external DP port can be switched
between GPUs and we switch it together with the panel. So if you switch
to the discrete GPU, you can also drive the external DP port on these
models and the integrated GPU can be turned off.

On the MBP8 2011 and newer, external ports are combined DP/Thunderbolt
ports which can only be driven by the discrete GPU. For some reason the
HPD/AUX pins of the ports can still be switched but not the other pins.
We should nail these ports to the discrete GPU and not switch those pins
together with the panel as we currently do. There's a patch in
mbp_switcheroo_v5-4.5-runpm.tar.gz which fixes that. The patch also wakes
up the discrete GPU on hotplug, which obviously needs runtime pm.


> - While plymouth is short-lived at boot, Wayland and X11 GNOME sessions
> use the GPU. The first user session will run on a VT that's separate
> from the greeter to implement fast-user switching. So, if we wanted to
> force using the other GPU for future sessions, we'd need to tell gdm to
> kill its graphical session and to respawn it so it doesn't hog the GPU
> and avoid the switch happening. Correct?

I think so, yes.


> On the 8,2, still, and with the kernel from the COPR repo[1].
> 
> I tried running:
> echo DIGD > switch
> 
> to (later) switch to the integrated GPU. Log out, get to gdm, log back
> in to a black screen with the cursor visible and nothing else.
> 
> I'm wondering if it's the gdm X11 session running in the background
> that makes this fail.

That's possible, I have no idea. I have zero issues with GPU switching
on my Nvidia-based MacBook Pro, though I only switch on the console with
no X11 running. On the AMD-based MBP8 that you're using, the patches have
only been tested by William Brown, a colleague of yours at Red Hat
Australia.

If you send me dmesg output with "drm.debug=0xf log_buf_len=10M"
I can see if I spot any issues.


> I'd like to try and get this working properly before bringing the
> runtime PM support into it.

Absolutely.


Best regards,

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

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Bastien Nocera]

On Mon, 2016-03-14 at 13:41 +0100, Lukas Wunner wrote:
> Hi Bastien,
> 
> sorry for the delay.
> 
> On Sat, Mar 05, 2016 at 05:31:55PM +0100, Bastien Nocera wrote:
> > 
> > We could, on boot, force using the integrated GPU, turning off the
> > discrete GPU that we're not interested in.
> Yes, many people "solve" this by having grub write the requisite
> commands
> to gmux' I/O ports, however this approach won't work with gummiboot.
> Also, the commands that need to be sent to gmux differ between
> retinas
> and pre-retinas.

Which is why I'd be fine with having user-space doing it later on. I
don't think users should have to poke at the boot parameters under
normal circumstances.

> > So I'd push DIGD to the switch sysfs entry on boot. But I'm
> > guessing
> > that won't turn off the other output we're not interested in.
> IGD and DIGD switch to the integrated GPU and also turn off the
> discrete
> GPU. However if the machine is already switched to the integrated
> GPU,
> the commands are no-ops and the discrete GPU is not turned off.
> 
> In other words you need to check (by reading the switch file) which
> GPU
> the machine is switched to. If it's the integrated GPU, write OFF to
> the
> switch file. If it's the discrete GPU, write DIGD to the switch file.
> 
> Once runtime pm works, writing DIGD is sufficient.

This isn't the greatest API, but let me make a note of that:
https://github.com/hadess/switcheroo-control/issues/1

I guess that's only useful until we get runtime PM support.

> > DIGD and DDIS should help (you need to log out/log in again), and
> > if
> > the current GPU is the integrated one, you could force running,
> > say, a
> > game on the discrete GPU using DRI_PRIME=1, correct?
> If runtime pm works, then yes. Otherwise you'd have to manually power
> up
> the GPU before using DRI_PRIME=1 and power it down afterwards.

Another need for run-time PM.

> > FWIW, using OFF at runtime made my machine hang, and without any
> > ways
> > for me to get debug output.
> Which GPU were you switched to? Did you issue the command while X11
> was
> running? Since the machine is switched to the AMD on boot, I guess
> you
> were powering down the Intel GPU. This works on my machine, I get a
> log
> entry "i915: switched off". If this doesn't work on your machine it
> might
> be an i915 bug on your Sandy Bridge GPU or it might be because X11 is
> running.
> 
> Try booting with "drm.debug=0xf log_buf_len=10M" and see if this gets
> you
> log output. If it doesn't, netconsole might help if the hang occurs
> while
> the console is locked.

I'll try it out with your runtime PM patches on top of the latest
upstream one.

> > Ok, so using GIGD or DDIS would just switch the output, but not
> > turn
> > off the unused GPU without runtime PM management.
> No. They do switch off the other GPU if runtime pm is disabled.
> 
> 
> > 
> > > 
> > > http://wunner.de/mbp_switcheroo_v5-4.5.tar.gz       => gpu
> > > switching
> > > for 4.5
> > That's the same patch that's already applied to the kernel above
> > (the
> > ones from this patchset thread), right?
> I'm not sure, the patches in the copr repository might be an earlier
> test version.

Might explain the problems I had.

> > My concerns here would be:
> > - Do we know how to turn off the integrated GPU automatically, if
> > the
> > user only used the internal screen and wanted to use the discrete
> > GPU?
> Runtime pm is currently disabled by default for i915. The Intel folks
> are on it. Until that works, the integrated GPU will be powered down
> when the user manually switches to the discrete GPU with DIS / DDIS
> and powered up when switching back with IGD / DIGD.

Do we have a way to know whether runtime PM is available for one/both
GPUs in the machine? Otherwise this really explodes the testing grid,
and it really makes everything harder.

> > - If only the discrete GPU is powered on, do we know how to power
> > on
> > the integrated GPU so it can drive the external screen when plugged
> > in?
> On the MBP5 2008/09 and MBP6 2010, the external DP port can be
> switched
> between GPUs and we switch it together with the panel. So if you
> switch
> to the discrete GPU, you can also drive the external DP port on these
> models and the integrated GPU can be turned off.
> 
> On the MBP8 2011 and newer, external ports are combined
> DP/Thunderbolt
> ports which can only be driven by the discrete GPU. For some reason
> the
> HPD/AUX pins of the ports can still be switched but not the other
> pins.
> We should nail these ports to the discrete GPU and not switch those
> pins
> together with the panel as we currently do. There's a patch in
> mbp_switcheroo_v5-4.5-runpm.tar.gz which fixes that. The patch also
> wakes
> up the discrete GPU on hotplug, which obviously needs runtime pm.

So that's something else that we can't handle properly without runtime
PM support.

> > - While plymouth is short-lived at boot, Wayland and X11 GNOME
> > sessions
> > use the GPU. The first user session will run on a VT that's
> > separate
> > from the greeter to implement fast-user switching. So, if we wanted
> > to
> > force using the other GPU for future sessions, we'd need to tell
> > gdm to
> > kill its graphical session and to respawn it so it doesn't hog the
> > GPU
> > and avoid the switch happening. Correct?
> I think so, yes.

After looking at our use cases in the GNOME wiki, I think that might
not be necessary as we'll want to always run the desktop on the
integrated GPU. That'll something to keep in mind if we ever want to
support running the desktop on the discrete GPU.

> > On the 8,2, still, and with the kernel from the COPR repo[1].
> > 
> > I tried running:
> > echo DIGD > switch
> > 
> > to (later) switch to the integrated GPU. Log out, get to gdm, log
> > back
> > in to a black screen with the cursor visible and nothing else.
> > 
> > I'm wondering if it's the gdm X11 session running in the background
> > that makes this fail.
> That's possible, I have no idea. I have zero issues with GPU
> switching
> on my Nvidia-based MacBook Pro, though I only switch on the console
> with
> no X11 running. On the AMD-based MBP8 that you're using, the patches
> have
> only been tested by William Brown, a colleague of yours at Red Hat
> Australia.
> 
> If you send me dmesg output with "drm.debug=0xf log_buf_len=10M"
> I can see if I spot any issues.

OK. Something else to test on my MBP then.

> > I'd like to try and get this working properly before bringing the
> > runtime PM support into it.
> Absolutely.

Thanks for your help.

Reading through the whole mail it seems to me that it's close to
impossible to implement a decent integration without runtime PM
support:
- DRI_PRIME wouldn't work
- no external display detection on some machines

Do you have references for the i915 runtime PM support, a bugzilla or
mailing-list thread?

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

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Daniel Vetter]

On Mon, Mar 14, 2016 at 02:37:44PM +0100, Bastien Nocera wrote:
> Do you have references for the i915 runtime PM support, a bugzilla or
> mailing-list thread?

i915.ko has runtime PM support, it's just not yet enabled by default due
to some funky corner cases. If you enable it you might hit a bunch of
sanity check warnings in dmesg. But besides those it should mostly work.
I didn't read the full context, just figured I'll throw this in.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Dave Airlie]

>
> I guess that's only useful until we get runtime PM support.

For the discrete GPUs on regular laptops we have runtime PM support for
powerdown already. Some newer laptops need a bit of work in the PCIE layer
but for most things we have it covered. The known broken ones are Apple
laptops. If the apple-gmux code is working well enough to power off GPUs,
then it should be possible to hook up runtime-pm on those machines pretty
simply.

So there shouldn't really be a case we care about.

runtime PM for the Intel GPU isn't as important. We don't even want to
turn the i915 fully off anymore.

>
> After looking at our use cases in the GNOME wiki, I think that might
> not be necessary as we'll want to always run the desktop on the
> integrated GPU. That'll something to keep in mind if we ever want to
>
> Reading through the whole mail it seems to me that it's close to
> impossible to implement a decent integration without runtime PM
> support:
> - DRI_PRIME wouldn't work
> - no external display detection on some machines
>
> Do you have references for the i915 runtime PM support, a bugzilla or
> mailing-list thread?

the i915 runtime PM doesn't matter for this. Only nouveau/radeon runtime
PM matters for this, and that should work on most Windows compatible hw right
now. For Windows 10 machines there are some patches going around to make
things work. For Apple I'm pretty much in the it'll catch up or it
won't, but don't
block on it.

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

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Bastien Nocera]

On Tue, 2016-03-15 at 21:10 +1000, Dave Airlie wrote:
> > 
> > 
> > I guess that's only useful until we get runtime PM support.
> For the discrete GPUs on regular laptops we have runtime PM support
> for
> powerdown already. Some newer laptops need a bit of work in the PCIE
> layer
> but for most things we have it covered. The known broken ones are
> Apple
> laptops. If the apple-gmux code is working well enough to power off
> GPUs,
> then it should be possible to hook up runtime-pm on those machines
> pretty
> simply.
> 
> So there shouldn't really be a case we care about.
> 
> runtime PM for the Intel GPU isn't as important. We don't even want
> to
> turn the i915 fully off anymore.

Fair enough. And it's not that big a problem if we want to try and run
the compositor on the integrated card by default either.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Bastien Nocera]

On Mon, 2016-03-14 at 13:41 +0100, Lukas Wunner wrote:
> 
<snip>
> > So I'd push DIGD to the switch sysfs entry on boot. But I'm
> > guessing
> > that won't turn off the other output we're not interested in.
> IGD and DIGD switch to the integrated GPU and also turn off the
> discrete
> GPU. However if the machine is already switched to the integrated
> GPU,
> the commands are no-ops and the discrete GPU is not turned off.
> 
> In other words you need to check (by reading the switch file) which
> GPU
> the machine is switched to. If it's the integrated GPU, write OFF to
> the
> switch file. If it's the discrete GPU, write DIGD to the switch file.
> 
> Once runtime pm works, writing DIGD is sufficient.

I tested the runtime patches for Radeon on top of 4.6.0-rc2, and
writing DIGD failed. I also saw a number of messages with the
vga_switcheroo core in the kernel trying to switch GPUs but failed
because "client 1" was keeping it busy.

Is there any way to see what this "client 1" actually is? I'm guessing
plymouth. In any case, once I'm logged in, the "DIS" is powered and
used, and the IGD is powered as well.

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

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Lukas Wunner]

Hi Bastien,

On Tue, Apr 05, 2016 at 06:59:40PM +0200, Bastien Nocera wrote:
> I tested the runtime patches for Radeon on top of 4.6.0-rc2, and
> writing DIGD failed. I also saw a number of messages with the
> vga_switcheroo core in the kernel trying to switch GPUs but failed
> because "client 1" was keeping it busy.
> 
> Is there any way to see what this "client 1" actually is? I'm guessing
> plymouth. In any case, once I'm logged in, the "DIS" is powered and
> used, and the IGD is powered as well.

Client 1 is the discrete GPU, see enum vga_switcheroo_client_id in
include/linux/vga_switcheroo.h:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/linux/vga_switcheroo.h#n75

The vga_switcheroo documentation explains how to find out which
user space process is blocking the switch:
https://01.org/linuxgraphics/gfx-docs/drm/modes_of_use.html

"Prerequisite is that no user space processes (e.g. Xorg, alsactl)
have opened device files of the GPUs or the audio client. If the
switch fails, the user may invoke lsof(8) or fuser(1) on /dev/dri/
and /dev/snd/controlC1 to identify processes blocking the switch."

HTH,

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

Re: [Intel-gfx] [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Bastien Nocera]

On Sat, 2016-03-05 at 15:16 +0100, Lukas Wunner wrote:
> Hi Bastien,
> 
> On Fri, Mar 04, 2016 at 04:12:27PM +0000, Bastien Nocera wrote:
> > 
> > Lukas Wunner <lukas <at> wunner.de> writes:
> > > 
> > > Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013),
> > > v5.
> > I've tested your patchset on a MacBookPro8,1, with an integrated
> > Intel and
> > discrete AMD/ATI GPUs.
> Hm, it must be either an 8,2 or 8,3. The 8,1 was a 13" machine and
> only
> had an integrated GPU.

On the 8,2, still, and with the kernel from the COPR repo[1].

I tried running:
echo DIGD > switch

to (later) switch to the integrated GPU. Log out, get to gdm, log back
in to a black screen with the cursor visible and nothing else.

I'm wondering if it's the gdm X11 session running in the background
that makes this fail.

I'd like to try and get this working properly before bringing the
runtime PM support into it.

[1]: That's the list of patches in the kernel:
http://copr-dist-git.fedorainfracloud.org/cgit/firstyear/kernel-mbp/kernel.git/tree/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Alex Deucher]

On Sat, Mar 5, 2016 at 9:16 AM, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Bastien,
>
> On Fri, Mar 04, 2016 at 04:12:27PM +0000, Bastien Nocera wrote:
>> Lukas Wunner <lukas <at> wunner.de> writes:
>> > Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
>>
>> I've tested your patchset on a MacBookPro8,1, with an integrated Intel and
>> discrete AMD/ATI GPUs.
>
> Hm, it must be either an 8,2 or 8,3. The 8,1 was a 13" machine and only
> had an integrated GPU.
>
>
>> I've used the COPR repository here to cut down on my compilation time:
>> https://copr.fedorainfracloud.org/coprs/firstyear/kernel-mbp/
>>
>> I'm not certain how to test out your changes, or what the consequences should
>> be on a stock Fedora 23/GNOME 3.18 installation. After booting (note that I
>> did not change any command-line options in grub), a gnome-shell/gdm X11
>> session comes up (I disabled Wayland, to rule out behavioural changes), I'd
>> log in to GNOME and gnome-shell (which starts another X11 session on
>> another VT).
>
> Switching and power control currently requires manual intervention
> by echoing commands to /sys/kernel/debug/vgaswitcheroo/switch
> as documented here:
> https://01.org/linuxgraphics/gfx-docs/drm/modes_of_use.html
>
> As you've correctly observed, the machine is initially switched to
> the discrete GPU and both GPUs are turned on. By echoing "IGD" to
> the sysfs file, you'll switch to the integrated GPU and turn off
> the discrete GPU.
>
> It's possible to let the EFI firmware switch to the integrated GPU
> on boot by using this tool: https://github.com/0xbb/gpu-switch
> However still both GPUs will be powered up, so you have to issue
> the "OFF" command to sysfs to power the discrete GPU down. Also,
> once you boot into OS X, the setting made by the gpu-switch tool
> will be overwritten and the machine will be switched to the discrete
> GPU again the next time you boot Linux.
>
> Note that switching is only possible from the text console, with
> X11/Wayland shut down. Obviously this is not great in terms of UX.
> A few years ago there was a GSoC proposal to get hot GPU switching
> to work on Linux (akin to what OS X does) but nothing ever came of it:
> http://www.phoronix.com/scan.php?page=news_item&px=OTIyMQ
> https://lists.x.org/archives/xorg/2011-March/052522.html
>
> Unfortunately this seems to be a low priority item for kernel graphics
> developers since nowadays most dual GPU notebooks no longer have a mux
> and cannot switch. The MacBook Pro seems to be the last one supporting
> this but I've witnessed a bit of an anti-Apple sentiment among kernel
> graphics developers since everything is non-standard there. Which is
> unfortunate because these machines have a large market share and Apple
> software quality is deteriorating rapidly so a lot of Mac users are
> ripe for converting to Linux.

Is there any reason to make use of the mux?  The usage model and
amount of stack work for non-mux systems is a lot easier to deal with
and covers a lot more systems overall.  runtime pm generally works
pretty seemlessly for mux-less systems.  Properly handling the mux is
a lot of work for relatively little gain as there are very few systems
that use them.


>
> Anyway, one short-term improvement will be to add runtime pm support
> (called "Driver power control" in the vga_switcheroo documentation
> linked above). That way it'll no longer be necessary to power the
> discrete GPU up and down manually, this will happen automatically
> as needed (when switching or using render offloading with DRI PRIME).
> I have patches to enable this for radeon but they're completely untested:
>
> http://wunner.de/mbp_switcheroo_v5-4.5.tar.gz       => gpu switching for 4.5
> http://wunner.de/mbp_switcheroo_v5-4.5-runpm.tar.gz => runtime pm for radeon
>
> I have an Nvidia based machine and runtime pm doesn't work there yet
> because of bugs in nouveau that I haven't had the time to look into.
>
>
>> I did not use any external screens to test this.
>
> Since your machine has Thunderbolt, the external port is no longer
> switchable between GPUs, it can only be driven by the discrete GPU.
> So you need to power it up manually for this to work. You don't need
> to switch to it, but it's probably recommendable to save energy.
> (Otherwise both GPUs are on with the integrated GPU driving the panel
> and the discrete GPU driving the DP port.)
>
> The runpm tarball linked above contains a patch to automatically
> wake the discrete GPU on hotplug.
>
> I've heard that the AMD GPU is picky about external monitors and
> doesn't recognize them unless they're plugged in at exactly the
> right moment, so you may need to retry a couple of times until it
> works.
>

Are talking about some issue specific to these muxed apple systems or
in general?  If you are having issues, please file a bug.

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

Re: [Intel-gfx] [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Lukas Wunner]

Hi Alex,

On Sat, Mar 05, 2016 at 01:10:56PM -0500, Alex Deucher wrote:
> Is there any reason to make use of the mux?

Performance (lower latency => no need for framebuffer writes over PCIe),
improved battery life (no need to use 2 GPUs simultaneously).

Technically you can't just ignore that the mux is there on the MBP
because the kernel has no control over the GPU used on boot.
(It's determined by EFI).


> > I've heard that the AMD GPU is picky about external monitors and
> > doesn't recognize them unless they're plugged in at exactly the
> > right moment, so you may need to retry a couple of times until it
> > works.
> 
> Are talking about some issue specific to these muxed apple systems or
> in general?

Feedback I got from William Brown of Red Hat who tested the GPU switching
patches on an MBP8,2 and reported that (independently of the patches),
a display connected with an original Apple DP-to-DVI adapter would only
be recognized if plugged in at exactly the right moment and in the correct
order (first adapter, then display). However it doesn't seem to work
better on OS X.


> If you are having issues, please file a bug.

I'm not having issues so can't file a bug. Besides, filing a bug is no
guarantee that things get fixed. He had opened a bug for GPU switching
3 years ago (https://bugs.freedesktop.org/show_bug.cgi?id=61115) and
nobody did a thing. Obviously whether something gets fixed is a function
of the perceived importance by maintainers, unless a volunteer comes
along and does the dirty work.

Best regards,

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

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Alex Deucher]

On Tue, Mar 15, 2016 at 1:54 PM, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Alex,
>
> On Sat, Mar 05, 2016 at 01:10:56PM -0500, Alex Deucher wrote:
>> Is there any reason to make use of the mux?
>
> Performance (lower latency => no need for framebuffer writes over PCIe),
> improved battery life (no need to use 2 GPUs simultaneously).
>
> Technically you can't just ignore that the mux is there on the MBP
> because the kernel has no control over the GPU used on boot.
> (It's determined by EFI).
>

Is GPU power switching also handled by the mux?  Is it independent of
the display mux?

>
>> > I've heard that the AMD GPU is picky about external monitors and
>> > doesn't recognize them unless they're plugged in at exactly the
>> > right moment, so you may need to retry a couple of times until it
>> > works.
>>
>> Are talking about some issue specific to these muxed apple systems or
>> in general?
>
> Feedback I got from William Brown of Red Hat who tested the GPU switching
> patches on an MBP8,2 and reported that (independently of the patches),
> a display connected with an original Apple DP-to-DVI adapter would only
> be recognized if plugged in at exactly the right moment and in the correct
> order (first adapter, then display). However it doesn't seem to work
> better on OS X.

Sounds like a issue with their adapter.

>
>
>> If you are having issues, please file a bug.
>
> I'm not having issues so can't file a bug. Besides, filing a bug is no
> guarantee that things get fixed. He had opened a bug for GPU switching
> 3 years ago (https://bugs.freedesktop.org/show_bug.cgi?id=61115) and
> nobody did a thing. Obviously whether something gets fixed is a function
> of the perceived importance by maintainers, unless a volunteer comes
> along and does the dirty work.

Well, of course everyone is busy and developers will prioritize
issues.  However, bugs that are not reported have substantially less
chance of getting fixed.

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

Re: [PATCH v5 00/12] Enable GPU switching on pre-retina?MacBook Pro

[Lukas Wunner]

Hi Alex,

On Tue, Mar 15, 2016 at 02:33:56PM -0400, Alex Deucher wrote:
> On Tue, Mar 15, 2016 at 1:54 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Sat, Mar 05, 2016 at 01:10:56PM -0500, Alex Deucher wrote:
> >> Is there any reason to make use of the mux?
> >
> > Performance (lower latency => no need for framebuffer writes over PCIe),
> > improved battery life (no need to use 2 GPUs simultaneously).
> >
> > Technically you can't just ignore that the mux is there on the MBP
> > because the kernel has no control over the GPU used on boot.
> > (It's determined by EFI).
> >
> Is GPU power switching also handled by the mux?  Is it independent of
> the display mux?

Yes and yes.

Best regards,

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