Re: [Intel-gfx] [PATCH v5] vga_switcheroo: Add helper for deferred probing
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 ___ 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
On Mon, 23 May 2016, Daniel Vetterwrote: > 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 >> Cc: Ben Skeggs >> Cc: Alex Deucher >> Signed-off-by: Lukas Wunner >> --- >> 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 >> #include >> #include >> #include >> -#include >> #include >> #include >> >> @@ -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, ); >> 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 >> #include >> #include >> #include >> #include >> #include >> -#include >> #include >> >> #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 >> -#include >> #include >> #include >> #include >> -#include >> #include >> #include >> >> @@ -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 >>
Re: [Intel-gfx] [PATCH v5] vga_switcheroo: Add helper for deferred probing
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> Cc: Ben Skeggs > Cc: Alex Deucher > Signed-off-by: Lukas Wunner > --- > 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 > #include > #include > #include > -#include > #include > #include > > @@ -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, ); > 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 > #include > #include > #include > #include > #include > -#include > #include > > #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 > -#include > #include > #include > #include > -#include > #include > #include > > @@ -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 @@ > >
[Intel-gfx] [PATCH v5] 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) 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 VetterCc: Ben Skeggs Cc: Alex Deucher Signed-off-by: Lukas Wunner --- 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 #include #include #include -#include #include #include @@ -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, ); 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 #include #include #include #include #include -#include #include #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 -#include #include #include #include -#include #include #include @@ -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 #include #include #include @@ -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