✗ Ro.CI.BAT: failure for vga_switcheroo: Add helper for deferred probing (rev2)

✗ Ro.CI.BAT: failure for vga_switcheroo: Add helper for deferred probing (rev2)

[Patchwork]

== Series Details ==

Series: vga_switcheroo: Add helper for deferred probing (rev2)
URL   : https://patchwork.freedesktop.org/series/7409/
State : failure

== Summary ==

Series 7409v2 vga_switcheroo: Add helper for deferred probing
http://patchwork.freedesktop.org/api/1.0/series/7409/revisions/2/mbox

Test gem_exec_gttfill:
        Subgroup basic:
                skip       -> PASS       (fi-hsw-i7-4770k)
                skip       -> PASS       (fi-hsw-i7-4770r)
                skip       -> FAIL       (fi-bdw-i7-5557u)
                skip       -> FAIL       (fi-skl-i7-6700k)

fi-bdw-i7-5557u  total:209  pass:196  dwarn:0   dfail:0   fail:1   skip:12 
fi-bsw-n3050     total:209  pass:167  dwarn:0   dfail:0   fail:2   skip:40 
fi-hsw-i7-4770k  total:209  pass:190  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-i7-4770r  total:209  pass:186  dwarn:0   dfail:0   fail:0   skip:23 
fi-skl-i7-6700k  total:209  pass:183  dwarn:0   dfail:0   fail:1   skip:25 
fi-snb-i7-2600   total:209  pass:170  dwarn:0   dfail:0   fail:0   skip:39 
ro-bdw-i5-5250u  total:209  pass:171  dwarn:0   dfail:0   fail:0   skip:38 
ro-bdw-i7-5557U  total:209  pass:196  dwarn:0   dfail:0   fail:0   skip:13 
ro-bdw-i7-5600u  total:209  pass:179  dwarn:0   dfail:0   fail:1   skip:29 
ro-bsw-n3050     total:209  pass:167  dwarn:0   dfail:0   fail:2   skip:40 
ro-byt-n2820     total:209  pass:168  dwarn:0   dfail:0   fail:3   skip:38 
ro-hsw-i3-4010u  total:209  pass:186  dwarn:0   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:209  pass:185  dwarn:0   dfail:0   fail:0   skip:24 
ro-ilk-i7-620lm  total:209  pass:144  dwarn:0   dfail:0   fail:2   skip:63 
ro-ilk1-i5-650   total:204  pass:146  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:209  pass:176  dwarn:0   dfail:0   fail:0   skip:33 
ro-ivb2-i7-3770  total:209  pass:180  dwarn:0   dfail:0   fail:0   skip:29 
ro-skl-i7-6700hq total:204  pass:182  dwarn:0   dfail:0   fail:0   skip:22 
ro-snb-i7-2620M  total:209  pass:170  dwarn:0   dfail:0   fail:1   skip:38 
fi-byt-n2820 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_958/

f1eaed1 drm-intel-nightly: 2016y-05m-20d-14h-35m-29s UTC integration manifest
0beadd0 vga_switcheroo: Add helper for deferred probing

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

[PATCH v5] vga_switcheroo: Add helper for deferred probing

[Lukas Wunner]

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)

v4: Rebase on 412c8f7de011 ("drm/radeon: Return -EPROBE_DEFER when
    amdkfd not loaded")

v5: Some Optimus GPUs use PCI_CLASS_DISPLAY_3D, make sure those are
    matched as well. (Emil Velikov)

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 d37c0a6..20d5898 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>
 
@@ -1025,13 +1023,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 11f8dd9..5c4d4df 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"
@@ -315,13 +313,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 b55aa74..a455dc7 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>
 
@@ -340,13 +338,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
 	if (ret == -EPROBE_DEFER)
 		return 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..2df216b3 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 >> 16) == PCI_BASE_CLASS_DISPLAY) {
+		/*
+		 * 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) {}
-- 
2.8.1

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

Re: [PATCH v5] vga_switcheroo: Add helper for deferred probing

[Daniel Vetter]

On Sat, May 21, 2016 at 03:59:16PM +0100, Lukas Wunner wrote:
> 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)
> 
> v4: Rebase on 412c8f7de011 ("drm/radeon: Return -EPROBE_DEFER when
>     amdkfd not loaded")
> 
> v5: Some Optimus GPUs use PCI_CLASS_DISPLAY_3D, make sure those are
>     matched as well. (Emil Velikov)
> 
> 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 d37c0a6..20d5898 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>
>  
> @@ -1025,13 +1023,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 11f8dd9..5c4d4df 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"
> @@ -315,13 +313,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 b55aa74..a455dc7 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>
>  
> @@ -340,13 +338,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
>  	if (ret == -EPROBE_DEFER)
>  		return 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..2df216b3 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.

"shall" or "must"? It thought it's the latter ...
-Daniel
>   *
>   * 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.

I still think we should just smash this into the relevant _register
functions(), for safety. But I forgot the actual result of the previous
discussion ...

Imo if you can replace a bit of documentation with code to enforce an
invariant, always do it.
-Daniel

> + *
> + * Return: %true if probing should be deferred, otherwise %false.
> + */
> +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
> +{
> +	if ((pdev->class >> 16) == PCI_BASE_CLASS_DISPLAY) {
> +		/*
> +		 * 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) {}
> -- 
> 2.8.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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: [PATCH v5] vga_switcheroo: Add helper for deferred probing

[Jani Nikula]

On Mon, 23 May 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, May 21, 2016 at 03:59:16PM +0100, Lukas Wunner wrote:
>> 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)
>> 
>> v4: Rebase on 412c8f7de011 ("drm/radeon: Return -EPROBE_DEFER when
>>     amdkfd not loaded")
>> 
>> v5: Some Optimus GPUs use PCI_CLASS_DISPLAY_3D, make sure those are
>>     matched as well. (Emil Velikov)
>> 
>> 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 d37c0a6..20d5898 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>
>>  
>> @@ -1025,13 +1023,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 11f8dd9..5c4d4df 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"
>> @@ -315,13 +313,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 b55aa74..a455dc7 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>
>>  
>> @@ -340,13 +338,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
>>  	if (ret == -EPROBE_DEFER)
>>  		return 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..2df216b3 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.
>
> "shall" or "must"? It thought it's the latter ...

The meaning of must... RFC 2119 says MUST is equivalent to REQUIRED or
SHALL, and I think for our purposes that SHOULD matter more than
dictionaries. ;)

J.


> -Daniel
>>   *
>>   * 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.
>
> I still think we should just smash this into the relevant _register
> functions(), for safety. But I forgot the actual result of the previous
> discussion ...
>
> Imo if you can replace a bit of documentation with code to enforce an
> invariant, always do it.
> -Daniel
>
>> + *
>> + * Return: %true if probing should be deferred, otherwise %false.
>> + */
>> +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
>> +{
>> +	if ((pdev->class >> 16) == PCI_BASE_CLASS_DISPLAY) {
>> +		/*
>> +		 * 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) {}
>> -- 
>> 2.8.1
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5] vga_switcheroo: Add helper for deferred probing

[Lukas Wunner]

On Mon, May 23, 2016 at 09:23:14AM +0200, Daniel Vetter wrote:
> On Sat, May 21, 2016 at 03:59:16PM +0100, Lukas Wunner wrote:
[snip]
> >  /**
> > + * 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.
> 
> I still think we should just smash this into the relevant _register
> functions(), for safety. But I forgot the actual result of the previous
> discussion ...

I've amended the commit message in v6 to summarize the result of the
previous discussion, which was:

    One might be tempted to check deferred probing conditions in
    vga_switcheroo_register_client(), but this is usually called fairly late
    during driver load. The GPU is fully brought up and ready for switching
    at that point. On boot the ->probe hook is potentially called dozens of
    times until it finally succeeds, and each time we'd repeat bringup and
    teardown of the GPU, lengthening boot time considerably and cluttering
    logfiles. A separate helper is therefore needed which can be called
    right at the beginning of the ->probe hook.

Thanks,

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