Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
The main obstacle on these machines is that the panel mode in VBIOS is bogus. Fortunately gmux can switch DDC independently from the display, thereby allowing the inactive GPU to probe the panel's EDID.
In short, vga_switcheroo and apple-gmux are amended with hooks to switch DDC, DRM core is amended with a drm_get_edid_switcheroo() helper, and relevant drivers are amended to call that for LVDS outputs.
The retina MacBook Pro (2012 - present) uses eDP and cannot switch AUX independently from the main link. The main obstacle there is link training, I'm currently working on this, it will be addressed in a future patch set.
This series is also reviewable on GitHub: https://github.com/l1k/linux/commits/mbp_switcheroo_v5
Changes:
* New patch [01/12]: vga_switcheroo handler flags Alex Deucher asked if this series might regress on non-Apple laptops. To address this concern, I let handlers declare their capabilities in a bitmask. DRM drivers call drm_get_edid_switcheroo() only if the handler has set the VGA_SWITCHEROO_CAN_SWITCH_DDC flag. Currently just one other flag is defined which is used on retinas.
* Changed patch [02/12]: vga_switcheroo DDC locking Rename ddc_lock to mux_hw_lock, suggested by Daniel Vetter.
* New patch [03/12]: track switch state of apple-gmux Fixes a bug in previous versions of this series which occurred if the system was suspended while DDC was temporarily switched: On resume DDC was switched to the wrong GPU.
* New patches [09/12 - 12/12]: deferred probing Previously I used connector reprobing if the inactive GPU's driver loaded before gmux. I've ditched that in favor of deferred driver probing, which is much simpler. Thanks to Daniel Vetter for the suggestion.
Caution: Patch [09/12] depends on a new acpi_dev_present() API which will land in 4.5 via Rafael J. Wysocki's tree.
I would particularly be interested in feedback on the handler flags patch [01/12]. I'm not 100% happy with the number of characters required to query the flags (e.g.: if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC)), but failed to come up with something shorter. Thierry Reding used a struct of bools instead of a bitmask for his recent drm_dp_link_caps patches. Maybe use that instead? http://lists.freedesktop.org/archives/dri-devel/2015-December/097025.html
Thanks,
Lukas
Lukas Wunner (12): vga_switcheroo: Add handler flags infrastructure vga_switcheroo: Add support for switching only the DDC apple-gmux: Track switch state apple-gmux: Add switch_ddc support drm/edid: Switch DDC when reading the EDID drm/i915: Switch DDC when reading the EDID drm/nouveau: Switch DDC when reading the EDID drm/radeon: Switch DDC when reading the EDID apple-gmux: Add helper for presence detect drm/i915: Defer probe if gmux is present but its driver isn't drm/nouveau: Defer probe if gmux is present but its driver isn't drm/radeon: Defer probe if gmux is present but its driver isn't
Documentation/DocBook/gpu.tmpl | 5 + drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c | 3 +- drivers/gpu/drm/drm_edid.c | 26 +++++ drivers/gpu/drm/i915/i915_drv.c | 12 +++ drivers/gpu/drm/i915/intel_lvds.c | 8 +- drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +- drivers/gpu/drm/nouveau/nouveau_connector.c | 21 +++- drivers/gpu/drm/nouveau/nouveau_drm.c | 11 +++ drivers/gpu/drm/radeon/radeon_atpx_handler.c | 3 +- drivers/gpu/drm/radeon/radeon_connectors.c | 6 ++ drivers/gpu/drm/radeon/radeon_drv.c | 11 +++ drivers/gpu/vga/vga_switcheroo.c | 119 ++++++++++++++++++++++- drivers/platform/x86/apple-gmux.c | 111 ++++++++++++++++----- include/drm/drm_crtc.h | 2 + include/linux/apple-gmux.h | 39 ++++++++ include/linux/vga_switcheroo.h | 36 ++++++- 16 files changed, 382 insertions(+), 33 deletions(-) create mode 100644 include/linux/apple-gmux.h
Allow handlers to declare their capabilities and allow clients to obtain that information. So far we have these use cases:
* If the handler is able to switch DDC separately, clients need to probe EDID with drm_get_edid_switcheroo(). We should allow them to detect a capable handler to ensure this function only gets called when needed.
* Likewise if the handler is unable to switch AUX separately, the active client needs to communicate link training parameters to the inactive client, which may then skip the AUX handshake and set up its output with these pre-calibrated values (DisplayPort specification v1.1a, section 2.5.3.3). Clients need a way to recognize such a situation.
The flags for the radeon_atpx_handler and amdgpu_atpx_handler are initially set to 0, this can later on be amended with handler_flags |= VGA_SWITCHEROO_CAN_SWITCH_DDC; when a ->switch_ddc callback is added.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] Signed-off-by: Lukas Wunner lukas@wunner.de --- Documentation/DocBook/gpu.tmpl | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c | 3 ++- drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +- drivers/gpu/drm/radeon/radeon_atpx_handler.c | 3 ++- drivers/gpu/vga/vga_switcheroo.c | 22 ++++++++++++++++++- drivers/platform/x86/apple-gmux.c | 2 +- include/linux/vga_switcheroo.h | 28 ++++++++++++++++++++++-- 7 files changed, 54 insertions(+), 7 deletions(-)
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 9e95aa1..88e7c39 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -3648,6 +3648,7 @@ int num_ioctls;</synopsis> </sect1> <sect1> <title>Public constants</title> +!Finclude/linux/vga_switcheroo.h vga_switcheroo_handler_flags_t !Finclude/linux/vga_switcheroo.h vga_switcheroo_client_id !Finclude/linux/vga_switcheroo.h vga_switcheroo_state </sect1> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c index 3c89586..fa948dc 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c @@ -552,13 +552,14 @@ static bool amdgpu_atpx_detect(void) void amdgpu_register_atpx_handler(void) { bool r; + enum vga_switcheroo_handler_flags_t handler_flags = 0;
/* detect if we have any ATPX + 2 VGA in the system */ r = amdgpu_atpx_detect(); if (!r) return;
- vga_switcheroo_register_handler(&amdgpu_atpx_handler); + vga_switcheroo_register_handler(&amdgpu_atpx_handler, handler_flags); }
/** diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c index d5e6938..cdf5227 100644 --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c @@ -314,7 +314,7 @@ void nouveau_register_dsm_handler(void) if (!r) return;
- vga_switcheroo_register_handler(&nouveau_dsm_handler); + vga_switcheroo_register_handler(&nouveau_dsm_handler, 0); }
/* Must be called for Optimus models before the card can be turned off */ diff --git a/drivers/gpu/drm/radeon/radeon_atpx_handler.c b/drivers/gpu/drm/radeon/radeon_atpx_handler.c index c4b4f29..56482e3 100644 --- a/drivers/gpu/drm/radeon/radeon_atpx_handler.c +++ b/drivers/gpu/drm/radeon/radeon_atpx_handler.c @@ -551,13 +551,14 @@ static bool radeon_atpx_detect(void) void radeon_register_atpx_handler(void) { bool r; + enum vga_switcheroo_handler_flags_t handler_flags = 0;
/* detect if we have any ATPX + 2 VGA in the system */ r = radeon_atpx_detect(); if (!r) return;
- vga_switcheroo_register_handler(&radeon_atpx_handler); + vga_switcheroo_register_handler(&radeon_atpx_handler, handler_flags); }
/** diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index d64d905..81da941 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -125,6 +125,7 @@ static DEFINE_MUTEX(vgasr_mutex); * (counting only vga clients, not audio clients) * @clients: list of registered clients * @handler: registered handler + * @handler_flags: flags of registered handler * * vga_switcheroo private data. Currently only one vga_switcheroo instance * per system is supported. @@ -141,6 +142,7 @@ struct vgasr_priv { struct list_head clients;
const struct vga_switcheroo_handler *handler; + enum vga_switcheroo_handler_flags_t handler_flags; };
#define ID_BIT_AUDIO 0x100 @@ -189,13 +191,15 @@ static void vga_switcheroo_enable(void) /** * vga_switcheroo_register_handler() - register handler * @handler: handler callbacks + * @handler_flags: handler flags * * Register handler. Enable vga_switcheroo if two vga clients have already * registered. * * Return: 0 on success, -EINVAL if a handler was already registered. */ -int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler) +int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler, + enum vga_switcheroo_handler_flags_t handler_flags) { mutex_lock(&vgasr_mutex); if (vgasr_priv.handler) { @@ -204,6 +208,7 @@ int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler }
vgasr_priv.handler = handler; + vgasr_priv.handler_flags = handler_flags; if (vga_switcheroo_ready()) { pr_info("enabled\n"); vga_switcheroo_enable(); @@ -221,6 +226,7 @@ EXPORT_SYMBOL(vga_switcheroo_register_handler); void vga_switcheroo_unregister_handler(void) { mutex_lock(&vgasr_mutex); + vgasr_priv.handler_flags = 0; vgasr_priv.handler = NULL; if (vgasr_priv.active) { pr_info("disabled\n"); @@ -231,6 +237,20 @@ void vga_switcheroo_unregister_handler(void) } EXPORT_SYMBOL(vga_switcheroo_unregister_handler);
+/** + * vga_switcheroo_handler_flags() - obtain handler flags + * + * Helper for clients to obtain the handler flags bitmask. + * + * Return: Handler flags. A value of 0 means that no handler is registered + * or that the handler has no special capabilities. + */ +enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(void) +{ + return vgasr_priv.handler_flags; +} +EXPORT_SYMBOL(vga_switcheroo_handler_flags); + static int register_client(struct pci_dev *pdev, const struct vga_switcheroo_client_ops *ops, enum vga_switcheroo_client_id id, bool active, diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index f236250..c401d49 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -705,7 +705,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) init_completion(&gmux_data->powerchange_done); gmux_enable_interrupts(gmux_data);
- if (vga_switcheroo_register_handler(&gmux_handler)) { + if (vga_switcheroo_register_handler(&gmux_handler, 0)) { ret = -ENODEV; goto err_register_handler; } diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h index 69e1d4a1..a745f4f0 100644 --- a/include/linux/vga_switcheroo.h +++ b/include/linux/vga_switcheroo.h @@ -36,6 +36,26 @@ struct pci_dev;
/** + * enum vga_switcheroo_handler_flags_t - handler flags bitmask + * @VGA_SWITCHEROO_CAN_SWITCH_DDC: whether the handler is able to switch the + * DDC lines separately. This signals to clients that they should call + * drm_get_edid_switcheroo() to probe the EDID + * @VGA_SWITCHEROO_NEEDS_EDP_CONFIG: whether the handler is unable to switch + * the AUX channel separately. This signals to clients that the active + * GPU needs to train the link and communicate the link parameters to the + * inactive GPU (mediated by vga_switcheroo). The inactive GPU may then + * skip the AUX handshake and set up its output with these pre-calibrated + * values (DisplayPort specification v1.1a, section 2.5.3.3) + * + * Handler flags bitmask. Used by handlers to declare their capabilities upon + * registering with vga_switcheroo. + */ +enum vga_switcheroo_handler_flags_t { + VGA_SWITCHEROO_CAN_SWITCH_DDC = (1 << 0), + VGA_SWITCHEROO_NEEDS_EDP_CONFIG = (1 << 1), +}; + +/** * enum vga_switcheroo_state - client power state * @VGA_SWITCHEROO_OFF: off * @VGA_SWITCHEROO_ON: on @@ -132,8 +152,10 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev, void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_info *info);
-int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler); +int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler, + enum vga_switcheroo_handler_flags_t handler_flags); void vga_switcheroo_unregister_handler(void); +enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(void);
int vga_switcheroo_process_delayed_switch(void);
@@ -150,11 +172,13 @@ static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {} static inline int vga_switcheroo_register_client(struct pci_dev *dev, const struct vga_switcheroo_client_ops *ops, bool driver_power_control) { return 0; } static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_info *info) {} -static inline int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler) { return 0; } +static inline int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler, + enum vga_switcheroo_handler_flags_t handler_flags) { return 0; } static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev, const struct vga_switcheroo_client_ops *ops, enum vga_switcheroo_client_id id) { return 0; } static inline void vga_switcheroo_unregister_handler(void) {} +static inline enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(void) { return 0; } static inline int vga_switcheroo_process_delayed_switch(void) { return 0; } static inline enum vga_switcheroo_state vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
Originally by Seth Forshee seth.forshee@canonical.com, 2012-10-04: During graphics driver initialization it's useful to be able to mux only the DDC to the inactive client in order to read the EDID. Add a switch_ddc callback to allow capable handlers to provide this functionality, and add vga_switcheroo_switch_ddc() to allow DRM to mux only the DDC.
Modified by Dave Airlie airlied@gmail.com, 2012-12-22: I can't figure out why I didn't like this, but I rewrote this [...] to lock/unlock the ddc lines [...]. I think I'd prefer something like that otherwise the interface got really ugly.
Modified by Lukas Wunner lukas@wunner.de, 2015-04 - 2015-10: Change semantics of ->switch_ddc handler callback to return previous DDC owner. Original version tried to determine previous DDC owner with find_active_client() but this fails if the inactive client registers before the active client.
Don't lock vgasr_mutex in _lock_ddc() / _unlock_ddc(), it can cause deadlocks because (a) during switch (with vgasr_mutex already held), GPU is woken and probes its outputs, tries to re-acquire vgasr_mutex to lock DDC lines; (b) Likewise during switch, GPU is suspended and calls cancel_delayed_work_sync() to stop output polling, if poll task is running at this moment we may wait forever for it to finish.
Instead, lock mux_hw_lock when unregistering the handler because the only reason why we'd want to lock vgasr_mutex in _lock_ddc() / _unlock_ddc() is to block the handler from disappearing while DDC lines are switched.
Also acquire mux_hw_lock in stage2 to avoid race condition where reading the EDID and switching happens simultaneously. Likewise on MIGD / MDIS commands and on runtime suspend.
v2.1: Overhaul locking, squash commits (Daniel Vetter)
v2.2: Readability improvements (Thierry Reding)
v2.3: Overhaul locking once more
v2.4: Retain semantics of ->switchto handler callback to switch all pins, including DDC (Daniel Vetter)
v5: Rename ddc_lock to mux_hw_lock: Since we acquire this both when calling ->switch_ddc and ->switchto, it protects not just access to the DDC lines but to the mux in general. This is in line with the DRM convention to use low-level locks to avoid concurrent hw access (e.g. i2c, dp_aux) which are often called hw_lock (Daniel Vetter)
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] Cc: Seth Forshee seth.forshee@canonical.com Cc: Dave Airlie airlied@gmail.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/vga/vga_switcheroo.c | 97 +++++++++++++++++++++++++++++++++++++++- include/linux/vga_switcheroo.h | 8 ++++ 2 files changed, 103 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index 81da941..56287ae 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -73,9 +73,17 @@ * there can thus be up to three clients: Two vga clients (GPUs) and one audio * client (on the discrete GPU). The code is mostly prepared to support * machines with more than two GPUs should they become available. + * * The GPU to which the outputs are currently switched is called the * active client in vga_switcheroo parlance. The GPU not in use is the - * inactive client. + * inactive client. When the inactive client's DRM driver is loaded, + * it will be unable to probe the panel's EDID and hence depends on + * VBIOS to provide its display modes. If the VBIOS modes are bogus or + * if there is no VBIOS at all (which is common on the MacBook Pro), + * a client may alternatively request that the DDC lines are temporarily + * switched to it, provided that the handler supports this. Switching + * only the DDC lines and not the entire output avoids unnecessary + * flickering. */
/** @@ -126,6 +134,9 @@ static DEFINE_MUTEX(vgasr_mutex); * @clients: list of registered clients * @handler: registered handler * @handler_flags: flags of registered handler + * @mux_hw_lock: protects mux state + * (in particular while DDC lines are temporarily switched) + * @old_ddc_owner: client to which DDC lines will be switched back on unlock * * vga_switcheroo private data. Currently only one vga_switcheroo instance * per system is supported. @@ -143,6 +154,8 @@ struct vgasr_priv {
const struct vga_switcheroo_handler *handler; enum vga_switcheroo_handler_flags_t handler_flags; + struct mutex mux_hw_lock; + int old_ddc_owner; };
#define ID_BIT_AUDIO 0x100 @@ -157,6 +170,7 @@ static void vga_switcheroo_debugfs_fini(struct vgasr_priv *priv); /* only one switcheroo per system */ static struct vgasr_priv vgasr_priv = { .clients = LIST_HEAD_INIT(vgasr_priv.clients), + .mux_hw_lock = __MUTEX_INITIALIZER(vgasr_priv.mux_hw_lock), };
static bool vga_switcheroo_ready(void) @@ -226,6 +240,7 @@ EXPORT_SYMBOL(vga_switcheroo_register_handler); void vga_switcheroo_unregister_handler(void) { mutex_lock(&vgasr_mutex); + mutex_lock(&vgasr_priv.mux_hw_lock); vgasr_priv.handler_flags = 0; vgasr_priv.handler = NULL; if (vgasr_priv.active) { @@ -233,6 +248,7 @@ void vga_switcheroo_unregister_handler(void) vga_switcheroo_debugfs_fini(&vgasr_priv); vgasr_priv.active = false; } + mutex_unlock(&vgasr_priv.mux_hw_lock); mutex_unlock(&vgasr_mutex); } EXPORT_SYMBOL(vga_switcheroo_unregister_handler); @@ -432,6 +448,76 @@ void vga_switcheroo_client_fb_set(struct pci_dev *pdev, EXPORT_SYMBOL(vga_switcheroo_client_fb_set);
/** + * vga_switcheroo_lock_ddc() - temporarily switch DDC lines to a given client + * @pdev: client pci device + * + * Temporarily switch DDC lines to the client identified by @pdev + * (but leave the outputs otherwise switched to where they are). + * This allows the inactive client to probe EDID. The DDC lines must + * afterwards be switched back by calling vga_switcheroo_unlock_ddc(), + * even if this function returns an error. + * + * Return: Previous DDC owner on success or a negative int on error. + * Specifically, %-ENODEV if no handler has registered or if the handler + * does not support switching the DDC lines. Also, a negative value + * returned by the handler is propagated back to the caller. + * The return value has merely an informational purpose for any caller + * which might be interested in it. It is acceptable to ignore the return + * value and simply rely on the result of the subsequent EDID probe, + * which will be %NULL if DDC switching failed. + */ +int vga_switcheroo_lock_ddc(struct pci_dev *pdev) +{ + enum vga_switcheroo_client_id id; + + mutex_lock(&vgasr_priv.mux_hw_lock); + if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) { + vgasr_priv.old_ddc_owner = -ENODEV; + return -ENODEV; + } + + id = vgasr_priv.handler->get_client_id(pdev); + vgasr_priv.old_ddc_owner = vgasr_priv.handler->switch_ddc(id); + return vgasr_priv.old_ddc_owner; +} +EXPORT_SYMBOL(vga_switcheroo_lock_ddc); + +/** + * vga_switcheroo_unlock_ddc() - switch DDC lines back to previous owner + * @pdev: client pci device + * + * Switch DDC lines back to the previous owner after calling + * vga_switcheroo_lock_ddc(). This must be called even if + * vga_switcheroo_lock_ddc() returned an error. + * + * Return: Previous DDC owner on success (i.e. the client identifier of @pdev) + * or a negative int on error. + * Specifically, %-ENODEV if no handler has registered or if the handler + * does not support switching the DDC lines. Also, a negative value + * returned by the handler is propagated back to the caller. + * Finally, invoking this function without calling vga_switcheroo_lock_ddc() + * first is not allowed and will result in %-EINVAL. + */ +int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) +{ + enum vga_switcheroo_client_id id; + int ret = vgasr_priv.old_ddc_owner; + + if (WARN_ON_ONCE(!mutex_is_locked(&vgasr_priv.mux_hw_lock))) + return -EINVAL; + + if (vgasr_priv.old_ddc_owner >= 0) { + id = vgasr_priv.handler->get_client_id(pdev); + if (vgasr_priv.old_ddc_owner != id) + ret = vgasr_priv.handler->switch_ddc( + vgasr_priv.old_ddc_owner); + } + mutex_unlock(&vgasr_priv.mux_hw_lock); + return ret; +} +EXPORT_SYMBOL(vga_switcheroo_unlock_ddc); + +/** * DOC: Manual switching and manual power control * * In this mode of use, the file /sys/kernel/debug/vgaswitcheroo/switch @@ -568,7 +654,9 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) console_unlock(); }
+ mutex_lock(&vgasr_priv.mux_hw_lock); ret = vgasr_priv.handler->switchto(new_client->id); + mutex_unlock(&vgasr_priv.mux_hw_lock); if (ret) return ret;
@@ -683,7 +771,9 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf, vgasr_priv.delayed_switch_active = false;
if (just_mux) { + mutex_lock(&vgasr_priv.mux_hw_lock); ret = vgasr_priv.handler->switchto(client_id); + mutex_unlock(&vgasr_priv.mux_hw_lock); goto out; }
@@ -895,8 +985,11 @@ static int vga_switcheroo_runtime_suspend(struct device *dev) if (ret) return ret; mutex_lock(&vgasr_mutex); - if (vgasr_priv.handler->switchto) + if (vgasr_priv.handler->switchto) { + mutex_lock(&vgasr_priv.mux_hw_lock); vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD); + mutex_unlock(&vgasr_priv.mux_hw_lock); + } vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF); mutex_unlock(&vgasr_mutex); return 0; diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h index a745f4f0..b39a5f3 100644 --- a/include/linux/vga_switcheroo.h +++ b/include/linux/vga_switcheroo.h @@ -102,6 +102,9 @@ enum vga_switcheroo_client_id { * Mandatory. For muxless machines this should be a no-op. Returning 0 * denotes success, anything else failure (in which case the switch is * aborted) + * @switch_ddc: switch DDC lines to given client. + * Optional. Should return the previous DDC owner on success or a + * negative int on failure * @power_state: cut or reinstate power of given client. * Optional. The return value is ignored * @get_client_id: determine if given pci device is integrated or discrete GPU. @@ -113,6 +116,7 @@ enum vga_switcheroo_client_id { struct vga_switcheroo_handler { int (*init)(void); int (*switchto)(enum vga_switcheroo_client_id id); + int (*switch_ddc)(enum vga_switcheroo_client_id id); int (*power_state)(enum vga_switcheroo_client_id id, enum vga_switcheroo_state state); enum vga_switcheroo_client_id (*get_client_id)(struct pci_dev *pdev); @@ -156,6 +160,8 @@ int vga_switcheroo_register_handler(const struct vga_switcheroo_handler *handler enum vga_switcheroo_handler_flags_t handler_flags); void vga_switcheroo_unregister_handler(void); enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(void); +int vga_switcheroo_lock_ddc(struct pci_dev *pdev); +int vga_switcheroo_unlock_ddc(struct pci_dev *pdev);
int vga_switcheroo_process_delayed_switch(void);
@@ -179,6 +185,8 @@ static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev, enum vga_switcheroo_client_id id) { return 0; } static inline void vga_switcheroo_unregister_handler(void) {} static inline enum vga_switcheroo_handler_flags_t vga_switcheroo_handler_flags(void) { return 0; } +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 enum vga_switcheroo_state vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
gmux has 3 switch registers:
* GMUX_PORT_SWITCH_DISPLAY switches the panel * GMUX_PORT_SWITCH_DDC switches the panel's DDC lines (only on pre-retinas; on retinas this is a no-op) * GMUX_PORT_SWITCH_EXTERNAL switches the external DP port(s) (only on models without Thunderbolt, i.e. introduced before 2011; those with Thunderbolt switch only HPD/AUX, not the main link)
Currently we switch all 3 registers in unison.
gmux does not preserve the switch state during suspend, so we currently read GMUX_PORT_SWITCH_DISPLAY before suspend and restore all 3 registers to this value on resume.
With the upcoming ->switch_ddc callback, GMUX_PORT_SWITCH_DDC may temporarily contain a different value than the other 2 registers. If we happen to suspend at this moment, we'll write an incorrect value to GMUX_PORT_SWITCH_DDC on resume.
Also, on models with Thunderbolt the integrated GPU is unable to drive the external DP port(s), so we want to keep GMUX_PORT_SWITCH_EXTERNAL permanently switched to the discrete GPU on those machines.
Consequently we can no longer assume that GMUX_PORT_SWITCH_DISPLAY represents the correct value for all 3 registers on suspend.
Track the state of all 3 registers: Add gmux_read_switch_state() and gmux_write_switch_state(). Instead of reading the switch state on every suspend, read it once on driver initialization so that we know the current switch state all the time. (This allows us to use some optimizations and shortcuts, e.g. we can skip switching DDC if we know that it's already switched to the requested GPU.) Change the ->switchto callback to use gmux_write_switch_state().
Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/platform/x86/apple-gmux.c | 67 +++++++++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 21 deletions(-)
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index c401d49..5c6c708 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -57,7 +57,9 @@ struct apple_gmux_data { /* switcheroo data */ acpi_handle dhandle; int gpe; - enum vga_switcheroo_client_id resume_client_id; + enum vga_switcheroo_client_id switch_state_display; + enum vga_switcheroo_client_id switch_state_ddc; + enum vga_switcheroo_client_id switch_state_external; enum vga_switcheroo_state power_state; struct completion powerchange_done; }; @@ -368,17 +370,49 @@ static const struct backlight_ops gmux_bl_ops = { * for the selected GPU. */
+static void gmux_read_switch_state(struct apple_gmux_data *gmux_data) +{ + if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_DDC) == 1) + gmux_data->switch_state_ddc = VGA_SWITCHEROO_IGD; + else + gmux_data->switch_state_ddc = VGA_SWITCHEROO_DIS; + + if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_DISPLAY) == 2) + gmux_data->switch_state_display = VGA_SWITCHEROO_IGD; + else + gmux_data->switch_state_display = VGA_SWITCHEROO_DIS; + + if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_EXTERNAL) == 2) + gmux_data->switch_state_external = VGA_SWITCHEROO_IGD; + else + gmux_data->switch_state_external = VGA_SWITCHEROO_DIS; +} + +static void gmux_write_switch_state(struct apple_gmux_data *gmux_data) +{ + if (gmux_data->switch_state_ddc == VGA_SWITCHEROO_IGD) + gmux_write8(gmux_data, GMUX_PORT_SWITCH_DDC, 1); + else + gmux_write8(gmux_data, GMUX_PORT_SWITCH_DDC, 2); + + if (gmux_data->switch_state_display == VGA_SWITCHEROO_IGD) + gmux_write8(gmux_data, GMUX_PORT_SWITCH_DISPLAY, 2); + else + gmux_write8(gmux_data, GMUX_PORT_SWITCH_DISPLAY, 3); + + if (gmux_data->switch_state_external == VGA_SWITCHEROO_IGD) + gmux_write8(gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 2); + else + gmux_write8(gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3); +} + static int gmux_switchto(enum vga_switcheroo_client_id id) { - if (id == VGA_SWITCHEROO_IGD) { - gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1); - gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 2); - gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 2); - } else { - gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2); - gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DISPLAY, 3); - gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_EXTERNAL, 3); - } + apple_gmux_data->switch_state_ddc = id; + apple_gmux_data->switch_state_display = id; + apple_gmux_data->switch_state_external = id; + + gmux_write_switch_state(apple_gmux_data);
return 0; } @@ -440,15 +474,6 @@ static int gmux_get_client_id(struct pci_dev *pdev) return VGA_SWITCHEROO_DIS; }
-static enum vga_switcheroo_client_id -gmux_active_client(struct apple_gmux_data *gmux_data) -{ - if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_DISPLAY) == 2) - return VGA_SWITCHEROO_IGD; - - return VGA_SWITCHEROO_DIS; -} - static const struct vga_switcheroo_handler gmux_handler = { .switchto = gmux_switchto, .power_state = gmux_set_power_state, @@ -513,7 +538,6 @@ static int gmux_suspend(struct device *dev) struct pnp_dev *pnp = to_pnp_dev(dev); struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
- gmux_data->resume_client_id = gmux_active_client(gmux_data); gmux_disable_interrupts(gmux_data); return 0; } @@ -524,7 +548,7 @@ static int gmux_resume(struct device *dev) struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
gmux_enable_interrupts(gmux_data); - gmux_switchto(gmux_data->resume_client_id); + gmux_write_switch_state(gmux_data); if (gmux_data->power_state == VGA_SWITCHEROO_OFF) gmux_set_discrete_state(gmux_data, gmux_data->power_state); return 0; @@ -704,6 +728,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) apple_gmux_data = gmux_data; init_completion(&gmux_data->powerchange_done); gmux_enable_interrupts(gmux_data); + gmux_read_switch_state(gmux_data);
if (vga_switcheroo_register_handler(&gmux_handler, 0)) { ret = -ENODEV;
Originally by Seth Forshee seth.forshee@canonical.com, 2012-10-04: The gmux allows muxing the DDC independently from the display, so support this functionality. This will allow reading the EDID for the inactive GPU, fixing issues with machines that either don't have a VBT or have invalid mode data in the VBT.
Modified by Lukas Wunner lukas@wunner.de, 2015-04 - 2015-12: Change semantics of ->switch_ddc handler callback to return previous DDC owner. Original version tried to determine previous DDC owner with find_active_client() in vga_switcheroo but this fails if the inactive client registers before the active client.
v2.4: Retain semantics of ->switchto handler callback to switch all pins, including DDC (Daniel Vetter)
v4: Advertise ->switch_ddc handler callback only on the pre-retina Macbook Pro. The retina uses eDP instead of LVDS and gmux no longer does the muxing itself but merely controls an external mux. That mux is incapable of switching the AUX channel separately from the main link. It's an NXP CBTL06142 (alternate parts: TI HD3SS212, Pericom PI3VDP12412, see datasheets below).
v5: Rebase on "apple-gmux: Track switch state". Rebase on "vga_switcheroo: Add handler flags infrastructure". Rebase on 5d170139eb10 ("Constify vga_switcheroo_handler"), requires 2 structs, 1x with ->switchto for pre-retinas, 1x without for retinas). Add error message if handler registration with vga_switcheroo fails.
Teardowns identifying the mux: http://www.electronicproducts.com/-whatsinside_text-145.aspx http://slideshare.net/jjwu6266/apple-2012-wwdc-apple-macbook-pro-with-retina... http://www.techrepublic.com/blog/cracking-open/teardown-shows-retina-macbook...
Mux Datasheets: http://www.nxp.com/documents/data_sheet/CBTL06141.pdf http://www.ti.com/lit/ds/symlink/hd3ss212.pdf https://www.pericom.com/assets/Datasheets/PI3VDP12412.pdf
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] Cc: Seth Forshee seth.forshee@canonical.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/platform/x86/apple-gmux.c | 45 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index 5c6c708..1384a39 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -417,6 +417,25 @@ static int gmux_switchto(enum vga_switcheroo_client_id id) return 0; }
+static int gmux_switch_ddc(enum vga_switcheroo_client_id id) +{ + enum vga_switcheroo_client_id old_ddc_owner = + apple_gmux_data->switch_state_ddc; + + if (id == old_ddc_owner) + return id; + + pr_debug("Switching DDC from %d to %d\n", old_ddc_owner, id); + apple_gmux_data->switch_state_ddc = id; + + if (id == VGA_SWITCHEROO_IGD) + gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1); + else + gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2); + + return old_ddc_owner; +} + /** * DOC: Power control * @@ -474,12 +493,19 @@ static int gmux_get_client_id(struct pci_dev *pdev) return VGA_SWITCHEROO_DIS; }
-static const struct vga_switcheroo_handler gmux_handler = { +static const struct vga_switcheroo_handler gmux_handler_indexed = { .switchto = gmux_switchto, .power_state = gmux_set_power_state, .get_client_id = gmux_get_client_id, };
+static const struct vga_switcheroo_handler gmux_handler_classic = { + .switchto = gmux_switchto, + .switch_ddc = gmux_switch_ddc, + .power_state = gmux_set_power_state, + .get_client_id = gmux_get_client_id, +}; + /** * DOC: Interrupt * @@ -730,8 +756,21 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id) gmux_enable_interrupts(gmux_data); gmux_read_switch_state(gmux_data);
- if (vga_switcheroo_register_handler(&gmux_handler, 0)) { - ret = -ENODEV; + /* + * Retina MacBook Pros cannot switch the panel's AUX separately + * and need eDP pre-calibration. They are distinguishable from + * pre-retinas by having an "indexed" gmux. + * + * Pre-retina MacBook Pros can switch the panel's DDC separately. + */ + if (gmux_data->indexed) + ret = vga_switcheroo_register_handler(&gmux_handler_indexed, + VGA_SWITCHEROO_NEEDS_EDP_CONFIG); + else + ret = vga_switcheroo_register_handler(&gmux_handler_classic, + VGA_SWITCHEROO_CAN_SWITCH_DDC); + if (ret) { + pr_err("Failed to register vga_switcheroo handler\n"); goto err_register_handler; }
Originally by Seth Forshee seth.forshee@canonical.com, 2012-10-04: Some dual graphics machines support muxing the DDC separately from the display, so make use of this functionality when reading the EDID on the inactive GPU. Also serialize drm_get_edid() with a mutex to avoid races on the DDC mux state.
Modified by Dave Airlie airlied@gmail.com, 2012-12-22: I can't figure out why I didn't like this, but I rewrote this [...] to lock/unlock the ddc lines [...]. I think I'd prefer something like that otherwise the interface got really ugly.
Modified by Lukas Wunner lukas@wunner.de, 2015-04 - 2015-09: v3: Move vga_switcheroo calls to a wrapper around drm_get_edid() which drivers can call on muxed machines. This avoids other drivers having to go through the vga_switcheroo motions even though they are never used on a muxed platform (Thierry Reding, Daniel Vetter, Alex Deucher)
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Tested-by: Pierre Moreau pierre.morrow@free.fr [MBP 5,3 2009 nvidia MCP79 + G96 pre-retina 15"] Tested-by: William Brown william@blackhats.net.au [MBP 8,2 2011 intel SNB + amd turks pre-retina 15"] Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] Cc: Seth Forshee seth.forshee@canonical.com Cc: Dave Airlie airlied@gmail.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/drm_edid.c | 26 ++++++++++++++++++++++++++ include/drm/drm_crtc.h | 2 ++ 2 files changed, 28 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index c214f12..1e8f660 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -32,6 +32,7 @@ #include <linux/hdmi.h> #include <linux/i2c.h> #include <linux/module.h> +#include <linux/vga_switcheroo.h> #include <drm/drmP.h> #include <drm/drm_edid.h> #include <drm/drm_displayid.h> @@ -1389,6 +1390,31 @@ struct edid *drm_get_edid(struct drm_connector *connector, EXPORT_SYMBOL(drm_get_edid);
/** + * drm_get_edid_switcheroo - get EDID data for a vga_switcheroo output + * @connector: connector we're probing + * @adapter: I2C adapter to use for DDC + * + * Wrapper around drm_get_edid() for laptops with dual GPUs using one set of + * outputs. The wrapper adds the requisite vga_switcheroo calls to temporarily + * switch DDC to the GPU which is retrieving EDID. + * + * Return: Pointer to valid EDID or %NULL if we couldn't find any. + */ +struct edid *drm_get_edid_switcheroo(struct drm_connector *connector, + struct i2c_adapter *adapter) +{ + struct pci_dev *pdev = connector->dev->pdev; + struct edid *edid; + + vga_switcheroo_lock_ddc(pdev); + edid = drm_get_edid(connector, adapter); + vga_switcheroo_unlock_ddc(pdev); + + return edid; +} +EXPORT_SYMBOL(drm_get_edid_switcheroo); + +/** * drm_edid_duplicate - duplicate an EDID and the extensions * @edid: EDID to duplicate * diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 3b040b3..65df8e7 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -2278,6 +2278,8 @@ extern void drm_property_destroy_user_blobs(struct drm_device *dev, extern bool drm_probe_ddc(struct i2c_adapter *adapter); extern struct edid *drm_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter); +extern struct edid *drm_get_edid_switcheroo(struct drm_connector *connector, + struct i2c_adapter *adapter); extern struct edid *drm_edid_duplicate(const struct edid *edid); extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid); extern void drm_mode_config_init(struct drm_device *dev);
The pre-retina MacBook Pro uses an LVDS panel and a gmux controller to switch the panel between its two GPUs. The panel mode in VBIOS is notoriously bogus on these machines and some models have no VBIOS at all.
Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS if the vga_switcheroo handler is capable of temporarily switching the panel's DDC lines to the integrated GPU. This allows us to retrieve the EDID if the panel is currently muxed to the discrete GPU.
This only enables EDID probing on the pre-retina MBP (2008 - 2013). The retina MBP (2012 - present) uses eDP and gmux is not capable of switching AUX separately from the main link on these models. This will be addressed in later patches.
List of pre-retina MBPs with dual GPUs, one of them Intel: [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina 15"] [MBP 6,1 2010 intel ILK + nvidia GT216 pre-retina 17"] [MBP 8,2 2011 intel SNB + amd turks pre-retina 15"] [MBP 8,3 2011 intel SNB + amd turks pre-retina 17"] [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"]
v3: Commit newly added due to introduction of drm_get_edid_switcheroo() wrapper which drivers need to opt-in to.
v5: Rebase on "vga_switcheroo: Add handler flags infrastructure", i.e. call drm_get_edid_switcheroo() only if the handler indicates that DDC is switchable.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/i915/intel_lvds.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 0da0240..811ddf7 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -31,6 +31,7 @@ #include <linux/dmi.h> #include <linux/i2c.h> #include <linux/slab.h> +#include <linux/vga_switcheroo.h> #include <drm/drmP.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc.h> @@ -1080,7 +1081,12 @@ void intel_lvds_init(struct drm_device *dev) * preferred mode is the right one. */ mutex_lock(&dev->mode_config.mutex); - edid = drm_get_edid(connector, intel_gmbus_get_adapter(dev_priv, pin)); + if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC) + edid = drm_get_edid_switcheroo(connector, + intel_gmbus_get_adapter(dev_priv, pin)); + else + edid = drm_get_edid(connector, + intel_gmbus_get_adapter(dev_priv, pin)); if (edid) { if (drm_add_edid_modes(connector, edid)) { drm_mode_connector_update_edid_property(connector,
The pre-retina MacBook Pro uses an LVDS panel and a gmux controller to switch the panel between its two GPUs. The panel mode in VBIOS is notoriously bogus on these machines.
Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS if the vga_switcheroo handler is capable of temporarily switching the panel's DDC lines to the discrete GPU. This allows us to retrieve the EDID if the panel is currently muxed to the integrated GPU. Likewise, ask vga_switcheroo to switch DDC before probing LVDS connectors.
This only enables EDID probing on the pre-retina MBP (2008 - 2013). The retina MBP (2012 - present) uses eDP and gmux is not capable of switching AUX separately from the main link on these models. This will be addressed in later patches.
List of pre-retina MBPs with dual GPUs, either or both Nvidia: [MBP 5,1 2008 nvidia MCP79 + G96 pre-retina 15"] [MBP 5,2 2009 nvidia MCP79 + G96 pre-retina 17"] [MBP 5,3 2009 nvidia MCP79 + G96 pre-retina 15"] [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina 15"] [MBP 6,1 2010 intel ILK + nvidia GT216 pre-retina 17"] [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"]
v3: Commit newly added due to introduction of drm_get_edid_switcheroo() wrapper which drivers need to opt-in to.
v5: Rebase on "vga_switcheroo: Add handler flags infrastructure", i.e. call drm_get_edid_switcheroo() only if the handler indicates that DDC is switchable.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/nouveau/nouveau_connector.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index fcebfae..ae96ebc 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -27,6 +27,7 @@ #include <acpi/button.h>
#include <linux/pm_runtime.h> +#include <linux/vga_switcheroo.h>
#include <drm/drmP.h> #include <drm/drm_edid.h> @@ -153,6 +154,17 @@ nouveau_connector_ddc_detect(struct drm_connector *connector) if (ret == 0) break; } else + if ((vga_switcheroo_handler_flags() & + VGA_SWITCHEROO_CAN_SWITCH_DDC) && + nv_encoder->dcb->type == DCB_OUTPUT_LVDS && + nv_encoder->i2c) { + int ret; + vga_switcheroo_lock_ddc(dev->pdev); + ret = nvkm_probe_i2c(nv_encoder->i2c, 0x50); + vga_switcheroo_unlock_ddc(dev->pdev); + if (ret) + break; + } else if (nv_encoder->i2c) { if (nvkm_probe_i2c(nv_encoder->i2c, 0x50)) break; @@ -265,7 +277,14 @@ nouveau_connector_detect(struct drm_connector *connector, bool force)
nv_encoder = nouveau_connector_ddc_detect(connector); if (nv_encoder && (i2c = nv_encoder->i2c) != NULL) { - nv_connector->edid = drm_get_edid(connector, i2c); + if ((vga_switcheroo_handler_flags() & + VGA_SWITCHEROO_CAN_SWITCH_DDC) && + nv_connector->type == DCB_CONNECTOR_LVDS) + nv_connector->edid = drm_get_edid_switcheroo(connector, + i2c); + else + nv_connector->edid = drm_get_edid(connector, i2c); + drm_mode_connector_update_edid_property(connector, nv_connector->edid); if (!nv_connector->edid) {
The pre-retina MacBook Pro uses an LVDS panel and a gmux controller to switch the panel between its two GPUs. The panel mode in VBIOS is notoriously bogus on these machines.
Use drm_get_edid_switcheroo() in lieu of drm_get_edid() on LVDS if the vga_switcheroo handler is capable of temporarily switching the panel's DDC lines to the discrete GPU. This allows us to retrieve the EDID if the panel is currently muxed to the integrated GPU.
This only enables EDID probing on the pre-retina MBP (2008 - 2013). The retina MBP (2012 - present) uses eDP and gmux is not capable of switching AUX separately from the main link on these models. This will be addressed in later patches.
List of pre-retina MBPs with dual GPUs, one of them AMD: [MBP 8,2 2011 intel SNB + amd turks pre-retina 15"] [MBP 8,3 2011 intel SNB + amd turks pre-retina 17"]
v3: Commit newly added due to introduction of drm_get_edid_switcheroo() wrapper which drivers need to opt-in to.
v5: Rebase on "vga_switcheroo: Add handler flags infrastructure", i.e. call drm_get_edid_switcheroo() only if the handler indicates that DDC is switchable.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/radeon/radeon_connectors.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index 340f3f5..cfcc099 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -34,6 +34,7 @@ #include "atom.h"
#include <linux/pm_runtime.h> +#include <linux/vga_switcheroo.h>
static int radeon_dp_handle_hpd(struct drm_connector *connector) { @@ -344,6 +345,11 @@ static void radeon_connector_get_edid(struct drm_connector *connector) else if (radeon_connector->ddc_bus) radeon_connector->edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter); + } else if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC && + connector->connector_type == DRM_MODE_CONNECTOR_LVDS && + radeon_connector->ddc_bus) { + radeon_connector->edid = drm_get_edid_switcheroo(&radeon_connector->base, + &radeon_connector->ddc_bus->adapter); } else if (radeon_connector->ddc_bus) { radeon_connector->edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
Centralize gmux' ACPI HID in a header file and add apple_gmux_present(). This can be used by other drivers to activate quirks specific to dual GPU MacBook Pros & Mac Pros. The alternative would be to hardcode DMI or PCI IDs and amend them whenever Apple introduces a new machine.
Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] Signed-off-by: Lukas Wunner lukas@wunner.de --- Documentation/DocBook/gpu.tmpl | 4 ++++ drivers/platform/x86/apple-gmux.c | 3 ++- include/linux/apple-gmux.h | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 include/linux/apple-gmux.h
diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl index 88e7c39..a5e6e3c 100644 --- a/Documentation/DocBook/gpu.tmpl +++ b/Documentation/DocBook/gpu.tmpl @@ -3677,6 +3677,10 @@ int num_ioctls;</synopsis> <title>Backlight control</title> !Pdrivers/platform/x86/apple-gmux.c Backlight control </sect2> + <sect2> + <title>Public functions</title> +!Iinclude/linux/apple-gmux.h + </sect2> </sect1> </chapter>
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c index 1384a39..4034d2d 100644 --- a/drivers/platform/x86/apple-gmux.c +++ b/drivers/platform/x86/apple-gmux.c @@ -19,6 +19,7 @@ #include <linux/acpi.h> #include <linux/pnp.h> #include <linux/apple_bl.h> +#include <linux/apple-gmux.h> #include <linux/slab.h> #include <linux/delay.h> #include <linux/pci.h> @@ -828,7 +829,7 @@ static void gmux_remove(struct pnp_dev *pnp) }
static const struct pnp_device_id gmux_device_ids[] = { - {"APP000B", 0}, + {GMUX_ACPI_HID, 0}, {"", 0} };
diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h new file mode 100644 index 0000000..feebc28 --- /dev/null +++ b/include/linux/apple-gmux.h @@ -0,0 +1,39 @@ +/* + * apple-gmux.h - microcontroller built into dual GPU MacBook Pro & Mac Pro + * Copyright (C) 2015 Lukas Wunner lukas@wunner.de + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License (version 2) as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see http://www.gnu.org/licenses/. + */ + +#ifndef LINUX_APPLE_GMUX_H +#define LINUX_APPLE_GMUX_H + +#include <linux/acpi.h> + +#define GMUX_ACPI_HID "APP000B" + +/** + * apple_gmux_present() - detect if gmux is built into the machine + * + * Drivers may use this to activate quirks specific to dual GPU MacBook Pros + * and Mac Pros, e.g. for deferred probing, runtime pm and backlight. + * + * Return: %true if gmux is present and the kernel was configured + * with CONFIG_APPLE_GMUX, %false otherwise. + */ +static inline bool apple_gmux_present(void) +{ + return IS_ENABLED(CONFIG_APPLE_GMUX) && acpi_dev_present(GMUX_ACPI_HID); +} + +#endif /* LINUX_APPLE_GMUX_H */
gmux is a microcontroller built into dual GPU MacBook Pros. On pre-retina MBPs, if we're the inactive GPU, we need apple-gmux to temporarily switch DDC so that we can probe the panel's EDID.
The checks for CONFIG_VGA_ARB and CONFIG_VGA_SWITCHEROO are necessary because if either of them is disabled but gmux is present, the driver would never load, even if we're the active GPU. (vga_default_device() would evaluate to NULL and vga_switcheroo_handler_flags() would evaluate to 0.)
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/i915/i915_drv.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 3ac616d..4a5fc5d 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -35,9 +35,12 @@ #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>
static struct drm_driver driver; @@ -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; + return drm_get_pci_dev(pdev, ent, &driver); }
On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
gmux is a microcontroller built into dual GPU MacBook Pros. On pre-retina MBPs, if we're the inactive GPU, we need apple-gmux to temporarily switch DDC so that we can probe the panel's EDID.
The checks for CONFIG_VGA_ARB and CONFIG_VGA_SWITCHEROO are necessary because if either of them is disabled but gmux is present, the driver would never load, even if we're the active GPU. (vga_default_device() would evaluate to NULL and vga_switcheroo_handler_flags() would evaluate to 0.)
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] Signed-off-by: Lukas Wunner lukas@wunner.de
drivers/gpu/drm/i915/i915_drv.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 3ac616d..4a5fc5d 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -35,9 +35,12 @@ #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>
static struct drm_driver driver; @@ -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?
Thanks, Daniel
- return drm_get_pci_dev(pdev, ent, &driver);
}
-- 1.8.5.2 (Apple Git-48)
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 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 | 28 ++++++++++++++++++++++++++++ include/linux/vga_switcheroo.h | 2 ++ 5 files changed, 33 insertions(+), 27 deletions(-)
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) {}
On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner lukas@wunner.de 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);
Hi,
On Sun, Feb 14, 2016 at 01:46:28PM +0100, Daniel Vetter wrote:
On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner lukas@wunner.de 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.
Ok, I've rephrased the kerneldoc to refer to "client" instead of "GPU" and moved the single existing check in an if block for PCI_CLASS_DISPLAY_VGA devices.
- 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?
In principle there's no harm in registering the client first, then checking if probing should be deferred, as long as the client is unregistered before deferring. Thus the language above is intentionally "need not" (muss nicht) rather than "must not" (darf nicht). I didn't want to mandate something that isn't actually required. The above sentence is merely an aid for driver developers who might be confused in which order to call what.
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.
I'm afraid that wouldn't be a good idea. The ->probe hook is potentially called dozens of times during boot until it finally succeeds and vga_switcheroo_register_client() is usually called in a fairly late driver load stage. In i915, we already have a working GEM at that point and an almost fully brought up GPU. Repeating bring up and tear down dozens of times is a nice stress test but nothing I'd inflict on production systems. I imagine it would also severely impact boot time and clutter the kernel log.
So a separate helper seems to be the only sensible option.
- 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.
Ok, I've moved that to a comment.
Updated patch follows after the scissors & perforation.
Thanks for the feedback!
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.)
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)
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 | 28 ++++++++++++++++++++++++++++ include/linux/vga_switcheroo.h | 2 ++ 5 files changed, 33 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index e8d0f17..01ef24a 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>
@@ -970,13 +968,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 56287ae..6108ebe 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> @@ -375,6 +376,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 should invoke this early on in their ->probe callback + * and return %-EPROBE_DEFER if it evaluates to %true. The client need + * not be registered with vga_switcheroo_register_client() beforehand. + * + * 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) {}
On Tue, Feb 16, 2016 at 04:58:20PM +0100, Lukas Wunner wrote:
Hi,
On Sun, Feb 14, 2016 at 01:46:28PM +0100, Daniel Vetter wrote:
On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner lukas@wunner.de 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.
Ok, I've rephrased the kerneldoc to refer to "client" instead of "GPU" and moved the single existing check in an if block for PCI_CLASS_DISPLAY_VGA devices.
- 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?
In principle there's no harm in registering the client first, then checking if probing should be deferred, as long as the client is unregistered before deferring. Thus the language above is intentionally "need not" (muss nicht) rather than "must not" (darf nicht). I didn't want to mandate something that isn't actually required. The above sentence is merely an aid for driver developers who might be confused in which order to call what.
I'd reject any driver that does this, registering, then checking, then unregistering seems extermely unsafe. I'd really stick with mandatory language here to make this clear.
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.
I'm afraid that wouldn't be a good idea. The ->probe hook is potentially called dozens of times during boot until it finally succeeds and vga_switcheroo_register_client() is usually called in a fairly late driver load stage. In i915, we already have a working GEM at that point and an almost fully brought up GPU. Repeating bring up and tear down dozens of times is a nice stress test but nothing I'd inflict on production systems. I imagine it would also severely impact boot time and clutter the kernel log.
So a separate helper seems to be the only sensible option.
Ok, makes sense. I still think adding the check to the client_register function would be good, just as a safety measure. And I don't think you're case of register(), check(), unregister() in case of failure is a valid use-case. Let's not allow anyone to open-code that horror ;-)
Cheers, Daniel
- 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.
Ok, I've moved that to a comment.
Updated patch follows after the scissors & perforation.
Thanks for the feedback!
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.)
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)
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 | 28 ++++++++++++++++++++++++++++ include/linux/vga_switcheroo.h | 2 ++ 5 files changed, 33 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index e8d0f17..01ef24a 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>
@@ -970,13 +968,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 56287ae..6108ebe 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> @@ -375,6 +376,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 should invoke this early on in their ->probe callback
- and return %-EPROBE_DEFER if it evaluates to %true. The client need
- not be registered with vga_switcheroo_register_client() beforehand.
- 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) {}
1.8.5.2 (Apple Git-48)
Hi,
On Tue, Feb 16, 2016 at 05:08:40PM +0100, Daniel Vetter wrote:
On Tue, Feb 16, 2016 at 04:58:20PM +0100, Lukas Wunner wrote:
On Sun, Feb 14, 2016 at 01:46:28PM +0100, Daniel Vetter wrote:
On Sun, Feb 14, 2016 at 1:10 PM, Lukas Wunner lukas@wunner.de wrote:
- 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?
In principle there's no harm in registering the client first, then checking if probing should be deferred, as long as the client is unregistered before deferring. Thus the language above is intentionally "need not" (muss nicht) rather than "must not" (darf nicht). I didn't want to mandate something that isn't actually required. The above sentence is merely an aid for driver developers who might be confused in which order to call what.
I'd reject any driver that does this, registering, then checking, then unregistering seems extermely unsafe. I'd really stick with mandatory language here to make this clear.
Ok, I've made it mandatory in the kerneldoc, updated patch follows below.
Ok, makes sense. I still think adding the check to the client_register function would be good, just as a safety measure.
Hm, the idea of calling vga_switcheroo_client_probe_defer() twice causes me pain in the stomach. It's surprising for drivers which just don't need it at the moment (amdgpu and snd_hda_intel) and it feels like overengineering and pampering driver developers beyond reasonable measure. Also while the single existing check is cheap, we might later on add checks that take more time and slow things down.
Best regards,
Lukas
-- >8 -- Subject: [PATCH] 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)
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 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..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) {}
On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner lukas@wunner.de wrote:
Ok, makes sense. I still think adding the check to the client_register function would be good, just as a safety measure.
Hm, the idea of calling vga_switcheroo_client_probe_defer() twice causes me pain in the stomach. It's surprising for drivers which just don't need it at the moment (amdgpu and snd_hda_intel) and it feels like overengineering and pampering driver developers beyond reasonable measure. Also while the single existing check is cheap, we might later on add checks that take more time and slow things down.
It's motivated by Rusty's API Manifesto:
http://sweng.the-davies.net/Home/rustys-api-design-manifesto
With the mandatory check in _register we reach level 5 - it'll blow up at runtime when we try to register it. Without that the failure is completely silent, and you need to read the right mailing list thread (level 1), but at least the kerneldocs lift it up to level 3.
For more context: We have tons of fun with EPROBE_DEFER handling between i915 and snd-hda, and I'm looking into all possible means to make any api/subsystem using deferred probing as robust as possible by default. One of the ideas is to inject deferred probe failures at runtime, but that's kinda hard to do in a generic way. At least making it as close to impossible to abuse as feasible is the next best option. -Daniel
Hi,
On Thu, Feb 18, 2016 at 10:39:05PM +0100, Daniel Vetter wrote:
On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner lukas@wunner.de wrote:
Ok, makes sense. I still think adding the check to the client_register function would be good, just as a safety measure.
Hm, the idea of calling vga_switcheroo_client_probe_defer() twice causes me pain in the stomach. It's surprising for drivers which just don't need it at the moment (amdgpu and snd_hda_intel) and it feels like overengineering and pampering driver developers beyond reasonable measure. Also while the single existing check is cheap, we might later on add checks that take more time and slow things down.
It's motivated by Rusty's API Manifesto:
http://sweng.the-davies.net/Home/rustys-api-design-manifesto
Interesting, thank you.
With the mandatory check in _register we reach level 5 - it'll blow up at runtime when we try to register it.
The manifesto says "5. Do it right or it will always break at runtime".
However even if we add a WARN_ON(vga_switcheroo_client_probe_defer(pdev)) to register_client(), it will not *always* spew a stacktrace but only on the machines this concerns (MacBook Pros). Since failure to defer breaks GPU switching, level 5 is already reached. Chances are this won't go unnoticed by the user.
For more context: We have tons of fun with EPROBE_DEFER handling between i915 and snd-hda
I don't understand, there is currently not a single occurrence of EPROBE_DEFER in i915, apart from the one I added.
In sound/ there are 88 occurrences of EPROBE_DEFER in soc/, plus 1 in ppc/ and that's it. So not a single one in pci/hda/ where hda_intel.c resides.
Is the fun with EPROBE_DEFER handling caused by the lack thereof?
Best regards,
Lukas
On Thu, Feb 18, 2016 at 11:20 PM, Lukas Wunner lukas@wunner.de wrote:
Hi,
On Thu, Feb 18, 2016 at 10:39:05PM +0100, Daniel Vetter wrote:
On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner lukas@wunner.de wrote:
Ok, makes sense. I still think adding the check to the client_register function would be good, just as a safety measure.
Hm, the idea of calling vga_switcheroo_client_probe_defer() twice causes me pain in the stomach. It's surprising for drivers which just don't need it at the moment (amdgpu and snd_hda_intel) and it feels like overengineering and pampering driver developers beyond reasonable measure. Also while the single existing check is cheap, we might later on add checks that take more time and slow things down.
It's motivated by Rusty's API Manifesto:
http://sweng.the-davies.net/Home/rustys-api-design-manifesto
Interesting, thank you.
With the mandatory check in _register we reach level 5 - it'll blow up at runtime when we try to register it.
The manifesto says "5. Do it right or it will always break at runtime".
However even if we add a WARN_ON(vga_switcheroo_client_probe_defer(pdev)) to register_client(), it will not *always* spew a stacktrace but only on the machines this concerns (MacBook Pros). Since failure to defer breaks GPU switching, level 5 is already reached. Chances are this won't go unnoticed by the user.
If we fail the register hopefully the driver checks for that and might blow up somewhere in untested error handling code. But there's a good chance it'll fail (we can encourage that more by adding must_check to the function declaration). In that case you get a nice bug report with splat from users hitting this.
Without this it'll silently work, and all the reports you get is "linux is shit, gpu switching doesn't work".
In both cases it can sometimes succeed, which is not great indeed. But I'm trying to fix that by injection EDEFER points artificially somehow. Not yet figured out that one.
But irrespective of the precise failure mode making the defer check mandatory by just including it in _register() is better since it makes it impossible to forget to call it when its needed. So imo clearly the more robust API. And that's my metric for evaluating new API - how easy/hard is it to abuse/get wrong.
For more context: We have tons of fun with EPROBE_DEFER handling between i915 and snd-hda
I don't understand, there is currently not a single occurrence of EPROBE_DEFER in i915, apart from the one I added.
In sound/ there are 88 occurrences of EPROBE_DEFER in soc/, plus 1 in ppc/ and that's it. So not a single one in pci/hda/ where hda_intel.c resides.
Is the fun with EPROBE_DEFER handling caused by the lack thereof?
Yes, there's one instance where i915 shoudl defer missing. The real trouble is that snd-hda has some really close ties with i915, and resolves those with probe-defer. And blows up all the time since we started using this, and with hdmi/dp you really always have to test both together in CI, snd-hda is pretty much a part of the intel gfx driver nowadays. Deferred probing is ime real trouble. -Daniel
-----Original Message----- From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Thursday, February 18, 2016 6:11 PM To: Lukas Wunner Cc: dri-devel; platform-driver-x86@vger.kernel.org; intel-gfx; Ben Skeggs; Deucher, Alexander Subject: Re: [PATCH v5 10/12] drm/i915: Defer probe if gmux is present but its driver isn't
On Thu, Feb 18, 2016 at 11:20 PM, Lukas Wunner lukas@wunner.de wrote:
Hi,
On Thu, Feb 18, 2016 at 10:39:05PM +0100, Daniel Vetter wrote:
On Thu, Feb 18, 2016 at 9:34 PM, Lukas Wunner lukas@wunner.de
wrote:
Ok, makes sense. I still think adding the check to the client_register function would be good, just as a safety measure.
Hm, the idea of calling vga_switcheroo_client_probe_defer() twice causes me pain in the stomach. It's surprising for drivers which just don't need it at the moment (amdgpu and snd_hda_intel) and it feels like overengineering and pampering driver developers beyond reasonable measure. Also while the single existing check is cheap, we might later on add checks that take more time and slow things down.
It's motivated by Rusty's API Manifesto:
http://sweng.the-davies.net/Home/rustys-api-design-manifesto
Interesting, thank you.
With the mandatory check in _register we reach level 5 - it'll blow up at runtime when we try to register it.
The manifesto says "5. Do it right or it will always break at runtime".
However even if we add a
WARN_ON(vga_switcheroo_client_probe_defer(pdev))
to register_client(), it will not *always* spew a stacktrace but only on the machines this concerns (MacBook Pros). Since failure to defer breaks GPU switching, level 5 is already reached. Chances are this won't go unnoticed by the user.
If we fail the register hopefully the driver checks for that and might blow up somewhere in untested error handling code. But there's a good chance it'll fail (we can encourage that more by adding must_check to the function declaration). In that case you get a nice bug report with splat from users hitting this.
Without this it'll silently work, and all the reports you get is "linux is shit, gpu switching doesn't work".
In both cases it can sometimes succeed, which is not great indeed. But I'm trying to fix that by injection EDEFER points artificially somehow. Not yet figured out that one.
But irrespective of the precise failure mode making the defer check mandatory by just including it in _register() is better since it makes it impossible to forget to call it when its needed. So imo clearly the more robust API. And that's my metric for evaluating new API - how easy/hard is it to abuse/get wrong.
For more context: We have tons of fun with EPROBE_DEFER handling between i915 and snd-hda
I don't understand, there is currently not a single occurrence of EPROBE_DEFER in i915, apart from the one I added.
In sound/ there are 88 occurrences of EPROBE_DEFER in soc/, plus 1 in ppc/ and that's it. So not a single one in pci/hda/ where hda_intel.c resides.
Is the fun with EPROBE_DEFER handling caused by the lack thereof?
Yes, there's one instance where i915 shoudl defer missing. The real trouble is that snd-hda has some really close ties with i915, and resolves those with probe-defer. And blows up all the time since we started using this, and with hdmi/dp you really always have to test both together in CI, snd-hda is pretty much a part of the intel gfx driver nowadays. Deferred probing is ime real trouble.
To further complicate things, AMD dGPUs have HDA audio on board as well.
Alex
gmux is a microcontroller built into dual GPU MacBook Pros. On pre-retina MBPs, if we're the inactive GPU, we need apple-gmux to temporarily switch DDC so that we can probe the panel's EDID.
The checks for CONFIG_VGA_ARB and CONFIG_VGA_SWITCHEROO are necessary because if either of them is disabled but gmux is present, the driver would never load, even if we're the active GPU. (vga_default_device() would evaluate to NULL and vga_switcheroo_handler_flags() would evaluate to 0.)
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"] Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/nouveau/nouveau_drm.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 2f2f252..bb8498c 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -22,11 +22,13 @@ * 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" @@ -312,6 +314,15 @@ 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()) + return -EPROBE_DEFER; + /* remove conflicting drivers (vesafb, efifb etc) */ aper = alloc_apertures(3); if (!aper)
gmux is a microcontroller built into dual GPU MacBook Pros. On pre-retina MBPs, if we're the inactive GPU, we need apple-gmux to temporarily switch DDC so that we can probe the panel's EDID.
The checks for CONFIG_VGA_ARB and CONFIG_VGA_SWITCHEROO are necessary because if either of them is disabled but gmux is present, the driver would never load, even if we're the active GPU. (vga_default_device() would evaluate to NULL and vga_switcheroo_handler_flags() would evaluate to 0.)
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115 Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/radeon/radeon_drv.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index e266ffc..cad2555 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -34,9 +34,11 @@ #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>
@@ -319,6 +321,15 @@ 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()) + return -EPROBE_DEFER; + /* Get rid of things like offb */ ret = radeon_kick_out_firmware_fb(pdev); if (ret)
Hi,
On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
This series hasn't seen any reviews or acks unfortunately. Any takers?
Merging this would allow fdo #61115 to be closed (currently assigned to intel-gfx).
FWIW this series has in the meantime been tested by more folks:
Tested-by: Pierre Moreau pierre.morrow@free.fr [MBP 5,3 2009 nvidia MCP79 + G96 pre-retina 15"] Tested-by: Paul Hordiienko pvt.gord@gmail.com [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina 15"] Tested-by: William Brown william@blackhats.net.au [MBP 8,2 2011 intel SNB + amd turks pre-retina 15"] Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"]
On the latter three models it worked fine. On Pierre Moreau's machine the discrete nvidia G96 locks up when woken. This happened in the past as well but not on the first wake but only on the 10th or so. Since it works fine on the GT216 and GK107, I'm guessing we've got a regression in the wakeup code for the G96 which is somehow triggered by this patch set (more specifically: triggered by being able to retrieve the proper panel resolution and configure a crtc). It needs to be fixed but isn't a showstopper for this series IMHO. (Arguably being able to retrieve EDID but then locking up on switching isn't really worse than not being able to retrieve EDID in the first place.)
Thanks,
Lukas
The main obstacle on these machines is that the panel mode in VBIOS is bogus. Fortunately gmux can switch DDC independently from the display, thereby allowing the inactive GPU to probe the panel's EDID.
In short, vga_switcheroo and apple-gmux are amended with hooks to switch DDC, DRM core is amended with a drm_get_edid_switcheroo() helper, and relevant drivers are amended to call that for LVDS outputs.
The retina MacBook Pro (2012 - present) uses eDP and cannot switch AUX independently from the main link. The main obstacle there is link training, I'm currently working on this, it will be addressed in a future patch set.
This series is also reviewable on GitHub: https://github.com/l1k/linux/commits/mbp_switcheroo_v5
Changes:
New patch [01/12]: vga_switcheroo handler flags Alex Deucher asked if this series might regress on non-Apple laptops. To address this concern, I let handlers declare their capabilities in a bitmask. DRM drivers call drm_get_edid_switcheroo() only if the handler has set the VGA_SWITCHEROO_CAN_SWITCH_DDC flag. Currently just one other flag is defined which is used on retinas.
Changed patch [02/12]: vga_switcheroo DDC locking Rename ddc_lock to mux_hw_lock, suggested by Daniel Vetter.
New patch [03/12]: track switch state of apple-gmux Fixes a bug in previous versions of this series which occurred if the system was suspended while DDC was temporarily switched: On resume DDC was switched to the wrong GPU.
New patches [09/12 - 12/12]: deferred probing Previously I used connector reprobing if the inactive GPU's driver loaded before gmux. I've ditched that in favor of deferred driver probing, which is much simpler. Thanks to Daniel Vetter for the suggestion.
Caution: Patch [09/12] depends on a new acpi_dev_present() API which will land in 4.5 via Rafael J. Wysocki's tree.
I would particularly be interested in feedback on the handler flags patch [01/12]. I'm not 100% happy with the number of characters required to query the flags (e.g.: if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC)), but failed to come up with something shorter. Thierry Reding used a struct of bools instead of a bitmask for his recent drm_dp_link_caps patches. Maybe use that instead? http://lists.freedesktop.org/archives/dri-devel/2015-December/097025.html
Thanks,
Lukas
Lukas Wunner (12): vga_switcheroo: Add handler flags infrastructure vga_switcheroo: Add support for switching only the DDC apple-gmux: Track switch state apple-gmux: Add switch_ddc support drm/edid: Switch DDC when reading the EDID drm/i915: Switch DDC when reading the EDID drm/nouveau: Switch DDC when reading the EDID drm/radeon: Switch DDC when reading the EDID apple-gmux: Add helper for presence detect drm/i915: Defer probe if gmux is present but its driver isn't drm/nouveau: Defer probe if gmux is present but its driver isn't drm/radeon: Defer probe if gmux is present but its driver isn't
Documentation/DocBook/gpu.tmpl | 5 + drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c | 3 +- drivers/gpu/drm/drm_edid.c | 26 +++++ drivers/gpu/drm/i915/i915_drv.c | 12 +++ drivers/gpu/drm/i915/intel_lvds.c | 8 +- drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +- drivers/gpu/drm/nouveau/nouveau_connector.c | 21 +++- drivers/gpu/drm/nouveau/nouveau_drm.c | 11 +++ drivers/gpu/drm/radeon/radeon_atpx_handler.c | 3 +- drivers/gpu/drm/radeon/radeon_connectors.c | 6 ++ drivers/gpu/drm/radeon/radeon_drv.c | 11 +++ drivers/gpu/vga/vga_switcheroo.c | 119 ++++++++++++++++++++++- drivers/platform/x86/apple-gmux.c | 111 ++++++++++++++++----- include/drm/drm_crtc.h | 2 + include/linux/apple-gmux.h | 39 ++++++++ include/linux/vga_switcheroo.h | 36 ++++++- 16 files changed, 382 insertions(+), 33 deletions(-) create mode 100644 include/linux/apple-gmux.h
-- 1.8.5.2 (Apple Git-48)
On 2 February 2016 at 08:49, Lukas Wunner lukas@wunner.de wrote:
Hi,
On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
This series hasn't seen any reviews or acks unfortunately. Any takers?
Has the tree this depends on been merged? I got these patches and applied them to drm-next and found I needed some acpi patches.
Can you start pushing these to github or somewhere and putting the link here?
Dave.
On 2 February 2016 at 11:10, Dave Airlie airlied@gmail.com wrote:
On 2 February 2016 at 08:49, Lukas Wunner lukas@wunner.de wrote:
Hi,
On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
This series hasn't seen any reviews or acks unfortunately. Any takers?
Has the tree this depends on been merged? I got these patches and applied them to drm-next and found I needed some acpi patches.
Can you start pushing these to github or somewhere and putting the link here?
just noticed you have a link, so it was just if the prereqs are getting merged I'm interested in :-)
Dave.
Hi Dave,
On Tue, Feb 02, 2016 at 11:10:19AM +1000, Dave Airlie wrote:
On 2 February 2016 at 08:49, Lukas Wunner lukas@wunner.de wrote:
On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
This series hasn't seen any reviews or acks unfortunately. Any takers?
Has the tree this depends on been merged? I got these patches and applied them to drm-next and found I needed some acpi patches.
Ugh, sorry, I forgot to mention: The acpi_dev_present() API landed in Linus' tree on January 13th, early on during the merge window. So after Linus' tree gets merged back into drm-next, the patches should compile just fine.
Thanks,
Lukas
Hello all,
On 01 Feb 2016, at 23:49, Lukas Wunner lukas@wunner.de wrote:
Hi,
On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote: Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
This series hasn't seen any reviews or acks unfortunately. Any takers?
Merging this would allow fdo #61115 to be closed (currently assigned to intel-gfx).
FWIW this series has in the meantime been tested by more folks:
Tested-by: Pierre Moreau pierre.morrow@free.fr [MBP 5,3 2009 nvidia MCP79 + G96 pre-retina 15"] Tested-by: Paul Hordiienko pvt.gord@gmail.com [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina 15"] Tested-by: William Brown william@blackhats.net.au [MBP 8,2 2011 intel SNB + amd turks pre-retina 15"] Tested-by: Lukas Wunner lukas@wunner.de [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina 15"]
On the latter three models it worked fine. On Pierre Moreau's machine the discrete nvidia G96 locks up when woken. This happened in the past as well but not on the first wake but only on the 10th or so. Since it works fine on the GT216 and GK107, I'm guessing we've got a regression in the wakeup code for the G96 which is somehow triggered by this patch set (more specifically: triggered by being able to retrieve the proper panel resolution and configure a crtc). It needs to be fixed but isn't a showstopper for this series IMHO. (Arguably being able to retrieve EDID but then locking up on switching isn't really worse than not being able to retrieve EDID in the first place.)
I would say it is slightly worse, since the only "downside" of not retrieving the EDID means TTY is set to a default resolution rather than the screen resolution, but this is fixed when starting X. On the other hand, since DRI_PRIME works fine on the laptop, there isn't much reason to switch between cards.
I'll have a look at the resume this week, now that FOSDEM is off my todo list.
Regards, Pierre
Thanks,
Lukas
The main obstacle on these machines is that the panel mode in VBIOS is bogus. Fortunately gmux can switch DDC independently from the display, thereby allowing the inactive GPU to probe the panel's EDID.
In short, vga_switcheroo and apple-gmux are amended with hooks to switch DDC, DRM core is amended with a drm_get_edid_switcheroo() helper, and relevant drivers are amended to call that for LVDS outputs.
The retina MacBook Pro (2012 - present) uses eDP and cannot switch AUX independently from the main link. The main obstacle there is link training, I'm currently working on this, it will be addressed in a future patch set.
This series is also reviewable on GitHub: https://github.com/l1k/linux/commits/mbp_switcheroo_v5
Changes:
- New patch [01/12]: vga_switcheroo handler flags
Alex Deucher asked if this series might regress on non-Apple laptops. To address this concern, I let handlers declare their capabilities in a bitmask. DRM drivers call drm_get_edid_switcheroo() only if the handler has set the VGA_SWITCHEROO_CAN_SWITCH_DDC flag. Currently just one other flag is defined which is used on retinas.
- Changed patch [02/12]: vga_switcheroo DDC locking
Rename ddc_lock to mux_hw_lock, suggested by Daniel Vetter.
- New patch [03/12]: track switch state of apple-gmux
Fixes a bug in previous versions of this series which occurred if the system was suspended while DDC was temporarily switched: On resume DDC was switched to the wrong GPU.
- New patches [09/12 - 12/12]: deferred probing
Previously I used connector reprobing if the inactive GPU's driver loaded before gmux. I've ditched that in favor of deferred driver probing, which is much simpler. Thanks to Daniel Vetter for the suggestion.
Caution: Patch [09/12] depends on a new acpi_dev_present() API which will land in 4.5 via Rafael J. Wysocki's tree.
I would particularly be interested in feedback on the handler flags patch [01/12]. I'm not 100% happy with the number of characters required to query the flags (e.g.: if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC)), but failed to come up with something shorter. Thierry Reding used a struct of bools instead of a bitmask for his recent drm_dp_link_caps patches. Maybe use that instead? http://lists.freedesktop.org/archives/dri-devel/2015-December/097025.html
Thanks,
Lukas
Lukas Wunner (12): vga_switcheroo: Add handler flags infrastructure vga_switcheroo: Add support for switching only the DDC apple-gmux: Track switch state apple-gmux: Add switch_ddc support drm/edid: Switch DDC when reading the EDID drm/i915: Switch DDC when reading the EDID drm/nouveau: Switch DDC when reading the EDID drm/radeon: Switch DDC when reading the EDID apple-gmux: Add helper for presence detect drm/i915: Defer probe if gmux is present but its driver isn't drm/nouveau: Defer probe if gmux is present but its driver isn't drm/radeon: Defer probe if gmux is present but its driver isn't
Documentation/DocBook/gpu.tmpl | 5 + drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c | 3 +- drivers/gpu/drm/drm_edid.c | 26 +++++ drivers/gpu/drm/i915/i915_drv.c | 12 +++ drivers/gpu/drm/i915/intel_lvds.c | 8 +- drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +- drivers/gpu/drm/nouveau/nouveau_connector.c | 21 +++- drivers/gpu/drm/nouveau/nouveau_drm.c | 11 +++ drivers/gpu/drm/radeon/radeon_atpx_handler.c | 3 +- drivers/gpu/drm/radeon/radeon_connectors.c | 6 ++ drivers/gpu/drm/radeon/radeon_drv.c | 11 +++ drivers/gpu/vga/vga_switcheroo.c | 119 ++++++++++++++++++++++- drivers/platform/x86/apple-gmux.c | 111 ++++++++++++++++----- include/drm/drm_crtc.h | 2 + include/linux/apple-gmux.h | 39 ++++++++ include/linux/vga_switcheroo.h | 36 ++++++- 16 files changed, 382 insertions(+), 33 deletions(-) create mode 100644 include/linux/apple-gmux.h
-- 1.8.5.2 (Apple Git-48)
On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
The main obstacle on these machines is that the panel mode in VBIOS is bogus. Fortunately gmux can switch DDC independently from the display, thereby allowing the inactive GPU to probe the panel's EDID.
In short, vga_switcheroo and apple-gmux are amended with hooks to switch DDC, DRM core is amended with a drm_get_edid_switcheroo() helper, and relevant drivers are amended to call that for LVDS outputs.
The retina MacBook Pro (2012 - present) uses eDP and cannot switch AUX independently from the main link. The main obstacle there is link training, I'm currently working on this, it will be addressed in a future patch set.
This series is also reviewable on GitHub: https://github.com/l1k/linux/commits/mbp_switcheroo_v5
Changes:
New patch [01/12]: vga_switcheroo handler flags Alex Deucher asked if this series might regress on non-Apple laptops. To address this concern, I let handlers declare their capabilities in a bitmask. DRM drivers call drm_get_edid_switcheroo() only if the handler has set the VGA_SWITCHEROO_CAN_SWITCH_DDC flag. Currently just one other flag is defined which is used on retinas.
Changed patch [02/12]: vga_switcheroo DDC locking Rename ddc_lock to mux_hw_lock, suggested by Daniel Vetter.
New patch [03/12]: track switch state of apple-gmux Fixes a bug in previous versions of this series which occurred if the system was suspended while DDC was temporarily switched: On resume DDC was switched to the wrong GPU.
New patches [09/12 - 12/12]: deferred probing Previously I used connector reprobing if the inactive GPU's driver loaded before gmux. I've ditched that in favor of deferred driver probing, which is much simpler. Thanks to Daniel Vetter for the suggestion.
Caution: Patch [09/12] depends on a new acpi_dev_present() API which will land in 4.5 via Rafael J. Wysocki's tree.
I would particularly be interested in feedback on the handler flags patch [01/12]. I'm not 100% happy with the number of characters required to query the flags (e.g.: if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC)), but failed to come up with something shorter. Thierry Reding used a struct of bools instead of a bitmask for his recent drm_dp_link_caps patches. Maybe use that instead? http://lists.freedesktop.org/archives/dri-devel/2015-December/097025.html
No objection from the platform-driver-x86 side. I can pull these separately once the core is in, or these can be included with that core (preferred) with my Reviewed-by for 1, 3, 4, and 9.
Reviewed-by: Darren Hart dvhart@linux.intel.com
On Mon, Feb 08, 2016 at 10:10:00AM -0800, Darren Hart wrote:
On Mon, Jan 11, 2016 at 08:09:20PM +0100, Lukas Wunner wrote:
Enable GPU switching on the pre-retina MacBook Pro (2008 - 2013), v5.
The main obstacle on these machines is that the panel mode in VBIOS is bogus. Fortunately gmux can switch DDC independently from the display, thereby allowing the inactive GPU to probe the panel's EDID.
In short, vga_switcheroo and apple-gmux are amended with hooks to switch DDC, DRM core is amended with a drm_get_edid_switcheroo() helper, and relevant drivers are amended to call that for LVDS outputs.
The retina MacBook Pro (2012 - present) uses eDP and cannot switch AUX independently from the main link. The main obstacle there is link training, I'm currently working on this, it will be addressed in a future patch set.
This series is also reviewable on GitHub: https://github.com/l1k/linux/commits/mbp_switcheroo_v5
Changes:
New patch [01/12]: vga_switcheroo handler flags Alex Deucher asked if this series might regress on non-Apple laptops. To address this concern, I let handlers declare their capabilities in a bitmask. DRM drivers call drm_get_edid_switcheroo() only if the handler has set the VGA_SWITCHEROO_CAN_SWITCH_DDC flag. Currently just one other flag is defined which is used on retinas.
Changed patch [02/12]: vga_switcheroo DDC locking Rename ddc_lock to mux_hw_lock, suggested by Daniel Vetter.
New patch [03/12]: track switch state of apple-gmux Fixes a bug in previous versions of this series which occurred if the system was suspended while DDC was temporarily switched: On resume DDC was switched to the wrong GPU.
New patches [09/12 - 12/12]: deferred probing Previously I used connector reprobing if the inactive GPU's driver loaded before gmux. I've ditched that in favor of deferred driver probing, which is much simpler. Thanks to Daniel Vetter for the suggestion.
Caution: Patch [09/12] depends on a new acpi_dev_present() API which will land in 4.5 via Rafael J. Wysocki's tree.
I would particularly be interested in feedback on the handler flags patch [01/12]. I'm not 100% happy with the number of characters required to query the flags (e.g.: if (vga_switcheroo_handler_flags() & VGA_SWITCHEROO_CAN_SWITCH_DDC)), but failed to come up with something shorter. Thierry Reding used a struct of bools instead of a bitmask for his recent drm_dp_link_caps patches. Maybe use that instead? http://lists.freedesktop.org/archives/dri-devel/2015-December/097025.html
No objection from the platform-driver-x86 side. I can pull these separately once the core is in, or these can be included with that core (preferred) with my Reviewed-by for 1, 3, 4, and 9.
Reviewed-by: Darren Hart dvhart@linux.intel.com
I pulled them all in through drm-misc, thanks. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Darren Hart -
Dave Airlie -
Deucher, Alexander -
Lukas Wunner -
Pierre Moreau