On Mon, 23 May 2016, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxx> >> Cc: Ben Skeggs <bskeggs@xxxxxxxxxx> >> Cc: Alex Deucher <alexander.deucher@xxxxxxx> >> Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx