[PATCH v4] vga_switcheroo: Add helper for deferred probing

[PATCH v4] 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")

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 dba03c0..61bf5a9 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>
 
@@ -1030,13 +1028,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 db5c7d0..a0bb1d0 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..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) {}
-- 
2.8.1

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

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

[Emil Velikov]

Hi Lukas,

On 19 May 2016 at 15:39, Lukas Wunner <lukas@wunner.de> wrote:

> +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
> +{
> +       if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) {
Not sure if we want/need this, yet at least. This changes behaviour
which is not what refactoring patches should be doing, right ? if
needed it ought to be a separate patch, imho.

Furthermore on nouveau and AMD (iirc) front, some dual-gpu/optimus
systems use PCI_CLASS_DISPLAY_3D. So if we add DISPLAY_VGA perhaps we
should also check for DISPLAY_3D ?

Regards,
Emil
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Emil Velikov]

On 21 May 2016 at 15:08, Lukas Wunner <lukas@wunner.de> wrote:
> Hi Emil,
>
> On Fri, May 20, 2016 at 12:41:04AM +0100, Emil Velikov wrote:
>> On 19 May 2016 at 15:39, Lukas Wunner <lukas@wunner.de> wrote:
>> > +bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev)
>> > +{
>> > +       if ((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) {
>> Not sure if we want/need this, yet at least. This changes behaviour
>> which is not what refactoring patches should be doing, right ? if
>> needed it ought to be a separate patch, imho.
>
> Well, the commit message doesn't claim "no functional change", does it?
>
It does implicitly via "vga_switcheroo: Add helper for deferred
probing". If you look through the kernel (and other projects) you'll
notice that very few patches have these three magic words. Even when
they don't have any functional changes.

> Daniel Vetter explicitly wanted the ability to use the helper in
> vga_switcheroo audio clients, and those shouldn't run the apple-gmux
> test. I think it makes sense to enclose it in the above-quoted if-block
> *now* even though it's not needed. Once someone adds a check for an
> audio client, chances are they'll forget to add this if-block.
>
Absolutely no arguments against any of that. Having a helper makes
sense even without any of the above arguments - it's a 'nasty looking'
if statement, duplicated multiple times. Speaking of which ...
shouldn't there be a similar hunk for amdgpu ?

Then again throwing everything into one patch does _not_ make sense.
Don't know why people would insist on having re-factoring and
functionality change as a single commit. Esp. in a place where hw
combinations, and thus chances of things going wrong, are pretty high.

>> Furthermore on nouveau and AMD (iirc) front, some dual-gpu/optimus
>> systems use PCI_CLASS_DISPLAY_3D. So if we add DISPLAY_VGA perhaps we
>> should also check for DISPLAY_3D ?
>
> Fair enough, I've changed it to match PCI_BASE_CLASS_DISPLAY
> and sent it out as v5 a few minutes ago.
>
Tweaking the exact class id (bitfield) is likely to end up drawn out
and obnoxious. That's why I'm suggesting to keep it separate.

Regards,
Emil
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

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

[Lukas Wunner]

On Mon, May 23, 2016 at 09:40:59AM +0100, Emil Velikov wrote:
> On 21 May 2016 at 15:08, Lukas Wunner <lukas@wunner.de> wrote:
> > Daniel Vetter explicitly wanted the ability to use the helper in
> > vga_switcheroo audio clients, and those shouldn't run the apple-gmux
> > test. I think it makes sense to enclose it in the above-quoted if-block
> > *now* even though it's not needed. Once someone adds a check for an
> > audio client, chances are they'll forget to add this if-block.
> >
> Absolutely no arguments against any of that. Having a helper makes
> sense even without any of the above arguments - it's a 'nasty looking'
> if statement, duplicated multiple times. Speaking of which ...
> shouldn't there be a similar hunk for amdgpu ?

Currently vga_switcheroo_client_probe_defer() is only needed for the
MacBook Pro and the AMD GPUs used by Apple are only supported by radeon.
I've amended the commit message in v6 to explain that.


> Then again throwing everything into one patch does _not_ make sense.
> Don't know why people would insist on having re-factoring and
> functionality change as a single commit. Esp. in a place where hw
> combinations, and thus chances of things going wrong, are pretty high.

Noone insists, v1 of this patch didn't contain the functional change
you're taking exception to, I added it in v2 on Daniel's request and
didn't want to complicate things further, seemed like a trivial change
to me. Nevertheless I've spun this out into a separate commit now,
no problem at all.

Best regards,

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