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

Commit Message

Lukas Wunner Feb. 14, 2016, 12:10 p.m. UTC

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 <[e-mail]>
Cc: Ben Skeggs <[e-mail]>
Cc: Alex Deucher <[e-mail]>
Signed-off-by: Lukas Wunner <[e-mail]>
---
 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(-)

Comments

Daniel Vetter Feb. 14, 2016, 12:46 p.m. UTC | #1

On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner <[e-mail]> 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);
> +

Patch

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) {}