Modernize vga_switcheroo by using a device link to enforce a runtime PM dependency from an HDA controller to the GPU it's integrated into, v2.
Changes since v1:
- Replace patch [1/7] to use pci_save_state() / pci_restore_state() for consistency between runtime PM code path of bound and unbound devices. (Rafael, Bjorn)
- Patch [5/7]: Drop an unnecessary initialization. (Bjorn) Rephrase error message on failed link addition for clarity.
Link to v1: https://www.spinics.net/lists/dri-devel/msg165889.html
Testing on more machines would be greatly appreciated, particularly Nvidia Optimus or AMD PowerXpress.
The series is based on 4.16-rc3. To test it on 4.15, you need to cherry-pick 7506dc798993 and 2fa6d6cdaf28. For your convenience I've pushed a 4.15-based branch to: https://github.com/l1k/linux/commits/switcheroo_devlink_v2
Minimal test procedure:
- Note well: Recent Optimus require that a Mini-DP or HDMI cable is plugged in on boot for the HDA device to be present.
- Check that HDA, GPU and root port autosuspend when not in use: cat /sys/bus/pci/devices/0000:01:00.1/power/runtime_status # HDA cat /sys/bus/pci/devices/0000:01:00.0/power/runtime_status # GPU cat /sys/bus/pci/devices/0000:00:01.0/power/runtime_status # Root Port
- Check that all three autoresume when accessing the HDA: hdajacksensetest -c 1
- Unbind the HDA controller: echo 0000:01:00.1 > /sys/bus/pci/drivers/snd_hda_intel/unbind Wait for GPU to power off, then rebind the HDA controller: echo 0000:01:00.1 > /sys/bus/pci/drivers/snd_hda_intel/bind Check dmesg for errors, try accessing HDA with hdajacksensetest.
- If your laptop uses the root port's _PR3 to cut power to the GPU: Unbind the GPU: echo 0000:01:00.0 > /sys/bus/pci/drivers/{nouveau,amdgpu,radeon}/unbind Allow runtime PM on the GPU: echo auto > /sys/bus/pci/devices/0000:01:00.0/power/control Wait for GPU to power off, then rebind it: echo 0000:01:00.0 > /sys/bus/pci/drivers/{nouveau,amdgpu,radeon}/bind Check dmesg for errors. If you see any then we may need to perform further actions in pci_pm_runtime_resume(), see patch [1/7].
Thanks,
Lukas
Lukas Wunner (6): PCI: Make pci_wakeup_bus() & pci_bus_set_current_state() public vga_switcheroo: Update PCI current_state on power change vga_switcheroo: Deduplicate power state tracking vga_switcheroo: Use device link for HDA controller vga_switcheroo: Let HDA autosuspend on mux change drm/nouveau: Runtime suspend despite HDA being unbound
Rafael J. Wysocki (1): PCI: Restore config space on runtime resume despite being unbound
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 - drivers/gpu/drm/nouveau/nouveau_drm.c | 46 ---------- drivers/gpu/drm/nouveau/nouveau_drv.h | 1 - drivers/gpu/drm/radeon/radeon_drv.c | 2 - drivers/gpu/vga/vga_switcheroo.c | 152 ++++++++------------------------ drivers/pci/pci-driver.c | 17 ++-- drivers/pci/pci.c | 8 +- drivers/pci/quirks.c | 39 ++++++++ include/linux/pci.h | 2 + include/linux/pci_ids.h | 1 + include/linux/vga_switcheroo.h | 6 -- include/sound/hdaudio.h | 3 - sound/pci/hda/hda_intel.c | 36 +++++--- sound/pci/hda/hda_intel.h | 3 - 14 files changed, 117 insertions(+), 201 deletions(-)
From: Rafael J. Wysocki rjw@rjwysocki.net
We leave PCI devices not bound to a driver in D0 during runtime suspend. But they may have a parent which is bound and can be transitioned to D3cold at runtime. Once the parent goes to D3cold, the unbound child may go to D3cold as well. When the child comes out of D3cold, its BARs are uninitialized and thus inaccessible when a driver tries to probe. Moreover configuration done during enumeration, e.g. ASPM and MPS, will be lost.
One example are recent hybrid graphics laptops which cut power to the discrete GPU when the root port above it goes to ACPI power state D3. Users may provoke this by unbinding the GPU driver and allowing runtime PM on the GPU via sysfs: The PM core will then treat the GPU as "suspended", which in turn allows the root port to runtime suspend, causing the power resources listed in its _PR3 object to be powered off. The GPU's BARs will be uninitialized when a driver later probes it.
Another example are hybrid graphics laptops where the GPU itself (rather than the root port) is capable of runtime suspending to D3cold. If the GPU's integrated HDA controller is not bound and the GPU's driver decides to runtime suspend to D3cold, the HDA controller's BARs will be uninitialized when a driver later probes it.
Fix by saving and restoring config space over a runtime suspend cycle even if the device is not bound.
Cc: Bjorn Helgaas bhelgaas@google.com Signed-off-by: Rafael J. Wysocki rafael.j.wysocki@intel.com [lukas: add commit message, bikeshed code comments for clarity] Signed-off-by: Lukas Wunner lukas@wunner.de --- Changes since v1: - Replace patch to use pci_save_state() / pci_restore_state() for consistency between runtime PM code path of bound and unbound devices. (Rafael, Bjorn)
drivers/pci/pci-driver.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 3bed6beda051..6a67cdbd0e6a 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -1224,11 +1224,14 @@ static int pci_pm_runtime_suspend(struct device *dev) int error;
/* - * If pci_dev->driver is not set (unbound), the device should - * always remain in D0 regardless of the runtime PM status + * If pci_dev->driver is not set (unbound), we leave the device in D0, + * but it may go to D3cold when the bridge above it runtime suspends. + * Save its config space in case that happens. */ - if (!pci_dev->driver) + if (!pci_dev->driver) { + pci_save_state(pci_dev); return 0; + }
if (!pm || !pm->runtime_suspend) return -ENOSYS; @@ -1276,16 +1279,18 @@ static int pci_pm_runtime_resume(struct device *dev) const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
/* - * If pci_dev->driver is not set (unbound), the device should - * always remain in D0 regardless of the runtime PM status + * Restoring config space is necessary even if the device is not bound + * to a driver because although we left it in D0, it may have gone to + * D3cold when the bridge above it runtime suspended. */ + pci_restore_standard_config(pci_dev); + if (!pci_dev->driver) return 0;
if (!pm || !pm->runtime_resume) return -ENOSYS;
- pci_restore_standard_config(pci_dev); pci_fixup_device(pci_fixup_resume_early, pci_dev); pci_enable_wake(pci_dev, PCI_D0, false); pci_fixup_device(pci_fixup_resume, pci_dev);
On Sat, Mar 03, 2018 at 10:53:24AM +0100, Lukas Wunner wrote:
From: Rafael J. Wysocki rjw@rjwysocki.net
We leave PCI devices not bound to a driver in D0 during runtime suspend. But they may have a parent which is bound and can be transitioned to D3cold at runtime. Once the parent goes to D3cold, the unbound child may go to D3cold as well. When the child comes out of D3cold, its BARs are uninitialized and thus inaccessible when a driver tries to probe.
There's no clear way to tell whether a BAR is uninitialized. At power-up, the writable bits will be zero, which is a valid BAR value. If enabled in PCI_COMMAND, the BAR is accessible and may conflict with other devices.
Possible alternate wording:
When the child goes to D3cold, its internal state, including configuration of BARs, MSI, ASPM, MPS, etc., is lost.
Moreover configuration done during enumeration, e.g. ASPM and MPS, will be lost.
One example are recent hybrid graphics laptops which cut power to the discrete GPU when the root port above it goes to ACPI power state D3. Users may provoke this by unbinding the GPU driver and allowing runtime PM on the GPU via sysfs: The PM core will then treat the GPU as "suspended", which in turn allows the root port to runtime suspend, causing the power resources listed in its _PR3 object to be powered off. The GPU's BARs will be uninitialized when a driver later probes it.
Another example are hybrid graphics laptops where the GPU itself (rather than the root port) is capable of runtime suspending to D3cold. If the GPU's integrated HDA controller is not bound and the GPU's driver decides to runtime suspend to D3cold, the HDA controller's BARs will be uninitialized when a driver later probes it.
Fix by saving and restoring config space over a runtime suspend cycle even if the device is not bound.
Cc: Bjorn Helgaas bhelgaas@google.com Signed-off-by: Rafael J. Wysocki rafael.j.wysocki@intel.com [lukas: add commit message, bikeshed code comments for clarity] Signed-off-by: Lukas Wunner lukas@wunner.de
Acked-by: Bjorn Helgaas bhelgaas@google.com
Changes since v1:
- Replace patch to use pci_save_state() / pci_restore_state() for consistency between runtime PM code path of bound and unbound devices. (Rafael, Bjorn)
drivers/pci/pci-driver.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 3bed6beda051..6a67cdbd0e6a 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -1224,11 +1224,14 @@ static int pci_pm_runtime_suspend(struct device *dev) int error;
/*
* If pci_dev->driver is not set (unbound), the device should* always remain in D0 regardless of the runtime PM status
* If pci_dev->driver is not set (unbound), we leave the device in D0,* but it may go to D3cold when the bridge above it runtime suspends.* Save its config space in case that happens.
Thanks for this clarification.
*/
- if (!pci_dev->driver)
if (!pci_dev->driver) {
pci_save_state(pci_dev);return 0;
}
if (!pm || !pm->runtime_suspend) return -ENOSYS;
@@ -1276,16 +1279,18 @@ static int pci_pm_runtime_resume(struct device *dev) const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
/*
* If pci_dev->driver is not set (unbound), the device should* always remain in D0 regardless of the runtime PM status
* Restoring config space is necessary even if the device is not bound* to a driver because although we left it in D0, it may have gone to* D3cold when the bridge above it runtime suspended.*/
pci_restore_standard_config(pci_dev);
if (!pci_dev->driver) return 0;
if (!pm || !pm->runtime_resume) return -ENOSYS;
- pci_restore_standard_config(pci_dev); pci_fixup_device(pci_fixup_resume_early, pci_dev); pci_enable_wake(pci_dev, PCI_D0, false); pci_fixup_device(pci_fixup_resume, pci_dev);
-- 2.15.1
There are PCI devices which are power-manageable by a nonstandard means, such as a custom ACPI method. One example are discrete GPUs in hybrid graphics laptops, another are Thunderbolt controllers in Macs.
Such devices can't be put into D3cold with pci_set_power_state() because pci_platform_power_transition() fails with -ENODEV. Instead they're put into D3hot by pci_set_power_state() and subsequently into D3cold by invoking the nonstandard means. However as a consequence the cached current_state is incorrectly left at D3hot.
What we need to do is walk the hierarchy below such a PCI device on powerdown and update the current_state to D3cold. On powerup the PCI device itself and the hierarchy below it is in D0uninitialized, so we need to walk the hierarchy again and wake all devices, causing them to be put into D0active and then letting them autosuspend as they see fit.
To this end make pci_wakeup_bus() & pci_bus_set_current_state() public so PCI drivers don't have to reinvent the wheel.
Cc: Rafael J. Wysocki rafael.j.wysocki@intel.com Acked-by: Bjorn Helgaas bhelgaas@google.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/pci/pci.c | 8 ++++---- include/linux/pci.h | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index f6a4dd10d9b0..bd6f156dc3cf 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -800,7 +800,7 @@ static int pci_wakeup(struct pci_dev *pci_dev, void *ign) * pci_wakeup_bus - Walk given bus and wake up devices on it * @bus: Top bus of the subtree to walk. */ -static void pci_wakeup_bus(struct pci_bus *bus) +void pci_wakeup_bus(struct pci_bus *bus) { if (bus) pci_walk_bus(bus, pci_wakeup, NULL); @@ -850,11 +850,11 @@ static int __pci_dev_set_current_state(struct pci_dev *dev, void *data) }
/** - * __pci_bus_set_current_state - Walk given bus and set current state of devices + * pci_bus_set_current_state - Walk given bus and set current state of devices * @bus: Top bus of the subtree to walk. * @state: state to be set */ -static void __pci_bus_set_current_state(struct pci_bus *bus, pci_power_t state) +void pci_bus_set_current_state(struct pci_bus *bus, pci_power_t state) { if (bus) pci_walk_bus(bus, __pci_dev_set_current_state, &state); @@ -876,7 +876,7 @@ int __pci_complete_power_transition(struct pci_dev *dev, pci_power_t state) ret = pci_platform_power_transition(dev, state); /* Power off the bridge may power off the whole hierarchy */ if (!ret && state == PCI_D3cold) - __pci_bus_set_current_state(dev->subordinate, PCI_D3cold); + pci_bus_set_current_state(dev->subordinate, PCI_D3cold); return ret; } EXPORT_SYMBOL_GPL(__pci_complete_power_transition); diff --git a/include/linux/pci.h b/include/linux/pci.h index 024a1beda008..ae42289662df 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1147,6 +1147,8 @@ void pci_pme_wakeup_bus(struct pci_bus *bus); void pci_d3cold_enable(struct pci_dev *dev); void pci_d3cold_disable(struct pci_dev *dev); bool pcie_relaxed_ordering_enabled(struct pci_dev *dev); +void pci_wakeup_bus(struct pci_bus *bus); +void pci_bus_set_current_state(struct pci_bus *bus, pci_power_t state);
/* PCI Virtual Channel */ int pci_save_vc_state(struct pci_dev *dev);
When cutting power to a GPU and its integrated HDA controller, their cached current_state should be updated to D3cold to reflect reality.
We currently rely on the DRM and HDA drivers to do that, however:
- The HDA driver updates the current_state in azx_vs_set_state(), which will no longer be called with driver power control once we migrate to device links. (It will still be called with manual power control.)
- If the HDA device is not bound, its current_state remains at D0 even though the GPU driver may decide to go to D3cold.
- The DRM drivers update the current_state using pci_set_power_state() which can't put the device into a deeper power state than D3hot if the GPU is not deemed power-manageable by the platform (even though it *is* power-manageable by some nonstandard means, such as a _DSM).
Centralize updating the current_state of the GPU and HDA controller in vga_switcheroo's ->runtime_suspend hook to overcome these deficiencies.
The GPU and HDA controller are two functions of the same PCI device (VGA class device on function 0 and audio device on function 1) and no other PCI devices reside on the same bus since this is a PCIe point-to-point link, so we can just walk the bus and update the current_state of all devices.
On ->runtime_resume, the HDA controller is in D0uninitialized state. Resume to D0active and then let it autosuspend as it sees fit.
Note that vga_switcheroo_init_domain_pm_ops() is not supposed to be called by hybrid graphics laptops which power down the GPU via its root port's _PR3 resources and consequently vga_switcheroo_runtime_suspend() is not used. On those laptops, the root port is power-manageable by the platform (instead of by a nonstandard means) and the current_state is therefore updated by the PCI core through the following call chain:
pci_set_power_state() __pci_complete_power_transition() pci_bus_set_current_state()
Resuming to D0active happens through:
pci_set_power_state() __pci_start_power_transition() pci_wakeup_bus()
Cc: Dave Airlie airlied@redhat.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Takashi Iwai tiwai@suse.de Cc: Peter Wu peter@lekensteyn.nl Cc: Alex Deucher alexander.deucher@amd.com Cc: Bjorn Helgaas bhelgaas@google.com Cc: Rafael J. Wysocki rafael.j.wysocki@intel.com Tested-by: Mike Lothian mike@fireburn.co.uk Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/vga/vga_switcheroo.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index 3cd153c6d271..09dd40dd1dbe 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -1022,6 +1022,7 @@ static int vga_switcheroo_runtime_suspend(struct device *dev) vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD); mutex_unlock(&vgasr_priv.mux_hw_lock); } + pci_bus_set_current_state(pdev->bus, PCI_D3cold); vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF); mutex_unlock(&vgasr_mutex); return 0; @@ -1035,6 +1036,7 @@ static int vga_switcheroo_runtime_resume(struct device *dev) mutex_lock(&vgasr_mutex); vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_ON); mutex_unlock(&vgasr_mutex); + pci_wakeup_bus(pdev->bus); ret = dev->bus->pm->runtime_resume(dev); if (ret) return ret;
If DRM drivers use runtime PM, they currently notify vga_switcheroo whenever they ->runtime_suspend or ->runtime_resume to update vga_switcheroo's internal power state tracking.
That's essentially a duplication of a functionality performed by the PM core as it already tracks the GPU's power state and vga_switcheroo can always query it.
Introduce a new internal helper vga_switcheroo_pwr_state() which does just that if runtime PM is used, or falls back to vga_switcheroo's internal power state tracking if manual power control is used. Drop a redundant power state check in set_audio_state() while at it.
This removes one of the two purposes of the notification mechanism implemented by vga_switcheroo_set_dynamic_switch(). The other one is power management of the audio device and we'll remove that next.
Cc: Dave Airlie airlied@redhat.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Takashi Iwai tiwai@suse.de Cc: Peter Wu peter@lekensteyn.nl Cc: Alex Deucher alexander.deucher@amd.com Cc: Rafael J. Wysocki rafael.j.wysocki@intel.com Tested-by: Mike Lothian mike@fireburn.co.uk Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/vga/vga_switcheroo.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index 09dd40dd1dbe..2488af797020 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -92,7 +92,8 @@ * struct vga_switcheroo_client - registered client * @pdev: client pci device * @fb_info: framebuffer to which console is remapped on switching - * @pwr_state: current power state + * @pwr_state: current power state if manual power control is used. + * For driver power control, call vga_switcheroo_pwr_state(). * @ops: client callbacks * @id: client identifier. Determining the id requires the handler, * so gpus are initially assigned VGA_SWITCHEROO_UNKNOWN_ID @@ -406,6 +407,19 @@ bool vga_switcheroo_client_probe_defer(struct pci_dev *pdev) } EXPORT_SYMBOL(vga_switcheroo_client_probe_defer);
+static enum vga_switcheroo_state +vga_switcheroo_pwr_state(struct vga_switcheroo_client *client) +{ + if (client->driver_power_control) + if (pm_runtime_enabled(&client->pdev->dev) && + pm_runtime_active(&client->pdev->dev)) + return VGA_SWITCHEROO_ON; + else + return VGA_SWITCHEROO_OFF; + else + return client->pwr_state; +} + /** * vga_switcheroo_get_client_state() - obtain power state of a given client * @pdev: client pci device @@ -425,7 +439,7 @@ enum vga_switcheroo_state vga_switcheroo_get_client_state(struct pci_dev *pdev) if (!client) ret = VGA_SWITCHEROO_NOT_FOUND; else - ret = client->pwr_state; + ret = vga_switcheroo_pwr_state(client); mutex_unlock(&vgasr_mutex); return ret; } @@ -598,7 +612,7 @@ static int vga_switcheroo_show(struct seq_file *m, void *v) client_is_vga(client) ? "" : "-Audio", client->active ? '+' : ' ', client->driver_power_control ? "Dyn" : "", - client->pwr_state ? "Pwr" : "Off", + vga_switcheroo_pwr_state(client) ? "Pwr" : "Off", pci_name(client->pdev)); i++; } @@ -641,7 +655,7 @@ static void set_audio_state(enum vga_switcheroo_client_id id, struct vga_switcheroo_client *client;
client = find_client_from_id(&vgasr_priv.clients, id | ID_BIT_AUDIO); - if (client && client->pwr_state != state) { + if (client) { client->ops->set_gpu_state(client->pdev, state); client->pwr_state = state; } @@ -656,7 +670,7 @@ static int vga_switchto_stage1(struct vga_switcheroo_client *new_client) if (!active) return 0;
- if (new_client->pwr_state == VGA_SWITCHEROO_OFF) + if (vga_switcheroo_pwr_state(new_client) == VGA_SWITCHEROO_OFF) vga_switchon(new_client);
vga_set_default_device(new_client->pdev); @@ -695,7 +709,7 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) if (new_client->ops->reprobe) new_client->ops->reprobe(new_client->pdev);
- if (active->pwr_state == VGA_SWITCHEROO_ON) + if (vga_switcheroo_pwr_state(active) == VGA_SWITCHEROO_ON) vga_switchoff(active);
set_audio_state(new_client->id, VGA_SWITCHEROO_ON); @@ -940,8 +954,7 @@ EXPORT_SYMBOL(vga_switcheroo_process_delayed_switch); * command line disables it. * * When the driver decides to power up or down, it notifies vga_switcheroo - * thereof so that it can (a) power the audio device on the GPU up or down, - * and (b) update its internal power state representation for the device. + * thereof so that it can power the audio device on the GPU up or down. * This is achieved by vga_switcheroo_set_dynamic_switch(). * * After the GPU has been suspended, the handler needs to be called to cut @@ -985,9 +998,8 @@ static void vga_switcheroo_power_switch(struct pci_dev *pdev, * * Helper for GPUs whose power state is controlled by the driver's runtime pm. * When the driver decides to power up or down, it notifies vga_switcheroo - * thereof using this helper so that it can (a) power the audio device on - * the GPU up or down, and (b) update its internal power state representation - * for the device. + * thereof using this helper so that it can power the audio device on the GPU + * up or down. */ void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, enum vga_switcheroo_state dynamic) @@ -1001,7 +1013,6 @@ void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, return; }
- client->pwr_state = dynamic; set_audio_state(client->id, dynamic); mutex_unlock(&vgasr_mutex); }
Back in 2013, runtime PM for GPUs with integrated HDA controller was introduced with commits 0d69704ae348 ("gpu/vga_switcheroo: add driver control power feature. (v3)") and 246efa4a072f ("snd/hda: add runtime suspend/resume on optimus support (v4)").
Briefly, the idea was that the HDA controller is forced on and off in unison with the GPU.
The original code is mostly still in place even though it was never a 100% perfect solution: E.g. on access to the HDA controller, the GPU is powered up via vga_switcheroo_runtime_resume_hdmi_audio() but there are no provisions to keep it resumed until access to the HDA controller has ceased: The GPU autosuspends after 5 seconds, rendering the HDA controller inaccessible.
Additionally, a kludge is required when hda_intel.c probes: It has to check whether the GPU is powered down (check_hdmi_disabled()) and defer probing if so.
However in the meantime (in v4.10) the driver core has gained a feature called device links which promises to solve such issues in a clean way: It allows us to declare a dependency from the HDA controller (consumer) to the GPU (supplier). The PM core then automagically ensures that the GPU is runtime resumed as long as the HDA controller's ->probe hook is executed and whenever the HDA controller is accessed.
By default, the HDA controller has a dependency on its parent, a PCIe Root Port. Adding a device link creates another dependency on its sibling:
PCIe Root Port ^ ^ | | | | HDA ===> GPU
The device link is not only used for runtime PM, it also guarantees that on system sleep, the HDA controller suspends before the GPU and resumes after the GPU, and on system shutdown the HDA controller's ->shutdown hook is executed before the one of the GPU. It is a complete solution.
Using this functionality is as simple as calling device_link_add(), which results in a dmesg entry like this:
pci 0000:01:00.1: Linked as a consumer to 0000:01:00.0
The code for the GPU-governed audio power management can thus be removed (except where it's still needed for legacy manual power control).
The device link is added in a PCI quirk rather than in hda_intel.c. It is therefore legal for the GPU to runtime suspend to D3cold even if the HDA controller is not bound to a driver or if CONFIG_SND_HDA_INTEL is not enabled, for accesses to the HDA controller will cause the GPU to wake up regardless if they're occurring outside of hda_intel.c (think config space readout via sysfs).
Contrary to the previous implementation, the HDA controller's power state is now self-governed, rather than GPU-governed, whereas the GPU's power state is no longer fully self-governed. (The HDA controller needs to runtime suspend before the GPU can.)
It is thus crucial that runtime PM is always activated on the HDA controller even if CONFIG_SND_HDA_POWER_SAVE_DEFAULT is set to 0 (which is the default), lest the GPU stays awake. This is achieved by setting the auto_runtime_pm flag on every codec and the AZX_DCAPS_PM_RUNTIME flag on the HDA controller.
A side effect is that power consumption might be reduced if the GPU is in use but the HDA controller is not, because the HDA controller is now allowed to go to D3hot. Before, it was forced to stay in D0 as long as the GPU was in use. (There is no reduction in power consumption on my Nvidia GK107, but there might be on other chips.)
The code paths for legacy manual power control are adjusted such that runtime PM is disabled during power off, thereby preventing the PM core from resuming the HDA controller.
Note that the device link is not only added on vga_switcheroo capable systems, but for *any* GPU with integrated HDA controller. The idea is that the HDA controller streams audio via connectors located on the GPU, so the GPU needs to be on for the HDA controller to do anything useful.
This commit implicitly fixes an unbalanced runtime PM ref upon unbind of hda_intel.c: On ->probe, a runtime PM ref was previously released under the condition "azx_has_pm_runtime(chip) || hda->use_vga_switcheroo", but on ->remove a runtime PM ref was only acquired under the first of those conditions. Thus, binding and unbinding the driver twice on a vga_switcheroo capable system caused the runtime PM refcount to drop below zero. The issue is resolved because the AZX_DCAPS_PM_RUNTIME flag is now always set if use_vga_switcheroo is true.
For more information on device links please refer to: https://www.kernel.org/doc/html/latest/driver-api/device_link.html Documentation/driver-api/device_link.rst
Cc: Dave Airlie airlied@redhat.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Peter Wu peter@lekensteyn.nl Cc: Alex Deucher alexander.deucher@amd.com Cc: Rafael J. Wysocki rafael.j.wysocki@intel.com Acked-by: Bjorn Helgaas bhelgaas@google.com Reviewed-by: Takashi Iwai tiwai@suse.de Tested-by: Mike Lothian mike@fireburn.co.uk Signed-off-by: Lukas Wunner lukas@wunner.de --- Changes since v1: - Drop an unnecessary initialization. (Bjorn) Rephrase error message on failed link addition for clarity.
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 - drivers/gpu/drm/nouveau/nouveau_drm.c | 2 - drivers/gpu/drm/radeon/radeon_drv.c | 2 - drivers/gpu/vga/vga_switcheroo.c | 115 +++----------------------------- drivers/pci/quirks.c | 39 +++++++++++ include/linux/pci_ids.h | 1 + include/linux/vga_switcheroo.h | 6 -- include/sound/hdaudio.h | 3 - sound/pci/hda/hda_intel.c | 36 +++++++--- sound/pci/hda/hda_intel.h | 3 - 10 files changed, 73 insertions(+), 136 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 50afcf65181a..ba4335fd4f65 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -720,7 +720,6 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING; drm_kms_helper_poll_disable(drm_dev); - vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF);
ret = amdgpu_device_suspend(drm_dev, false, false); pci_save_state(pdev); @@ -757,7 +756,6 @@ static int amdgpu_pmops_runtime_resume(struct device *dev)
ret = amdgpu_device_resume(drm_dev, false, false); drm_kms_helper_poll_enable(drm_dev); - vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_ON); drm_dev->switch_power_state = DRM_SWITCH_POWER_ON; return 0; } diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 3e293029e3a6..6959951d45d6 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -856,7 +856,6 @@ nouveau_pmops_runtime_suspend(struct device *dev) }
drm_kms_helper_poll_disable(drm_dev); - vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF); nouveau_switcheroo_optimus_dsm(); ret = nouveau_do_suspend(drm_dev, true); pci_save_state(pdev); @@ -891,7 +890,6 @@ nouveau_pmops_runtime_resume(struct device *dev)
/* do magic */ nvif_mask(&device->object, 0x088488, (1 << 25), (1 << 25)); - vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_ON); drm_dev->switch_power_state = DRM_SWITCH_POWER_ON;
/* Monitors may have been connected / disconnected during suspend */ diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index 31dd04f6baa1..b28288a781ef 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -415,7 +415,6 @@ static int radeon_pmops_runtime_suspend(struct device *dev)
drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING; drm_kms_helper_poll_disable(drm_dev); - vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_OFF);
ret = radeon_suspend_kms(drm_dev, false, false, false); pci_save_state(pdev); @@ -452,7 +451,6 @@ static int radeon_pmops_runtime_resume(struct device *dev)
ret = radeon_resume_kms(drm_dev, false, false); drm_kms_helper_poll_enable(drm_dev); - vga_switcheroo_set_dynamic_switch(pdev, VGA_SWITCHEROO_ON); drm_dev->switch_power_state = DRM_SWITCH_POWER_ON; return 0; } diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index 2488af797020..4ee0ed642386 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -105,8 +105,7 @@ * @list: client list * * Registered client. A client can be either a GPU or an audio device on a GPU. - * For audio clients, the @fb_info, @active and @driver_power_control members - * are bogus. + * For audio clients, the @fb_info and @active members are bogus. */ struct vga_switcheroo_client { struct pci_dev *pdev; @@ -332,8 +331,8 @@ EXPORT_SYMBOL(vga_switcheroo_register_client); * @ops: client callbacks * @id: client identifier * - * Register audio client (audio device on a GPU). The power state of the - * client is assumed to be ON. Beforehand, vga_switcheroo_client_probe_defer() + * Register audio client (audio device on a GPU). The client is assumed + * to use runtime PM. 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. @@ -342,7 +341,7 @@ int vga_switcheroo_register_audio_client(struct pci_dev *pdev, const struct vga_switcheroo_client_ops *ops, enum vga_switcheroo_client_id id) { - return register_client(pdev, ops, id | ID_BIT_AUDIO, false, false); + return register_client(pdev, ops, id | ID_BIT_AUDIO, false, true); } EXPORT_SYMBOL(vga_switcheroo_register_audio_client);
@@ -655,10 +654,8 @@ static void set_audio_state(enum vga_switcheroo_client_id id, struct vga_switcheroo_client *client;
client = find_client_from_id(&vgasr_priv.clients, id | ID_BIT_AUDIO); - if (client) { + if (client) client->ops->set_gpu_state(client->pdev, state); - client->pwr_state = state; - } }
/* stage one happens before delay */ @@ -953,10 +950,6 @@ EXPORT_SYMBOL(vga_switcheroo_process_delayed_switch); * Specifying nouveau.runpm=0, radeon.runpm=0 or amdgpu.runpm=0 on the kernel * command line disables it. * - * When the driver decides to power up or down, it notifies vga_switcheroo - * thereof so that it can power the audio device on the GPU up or down. - * This is achieved by vga_switcheroo_set_dynamic_switch(). - * * After the GPU has been suspended, the handler needs to be called to cut * power to the GPU. Likewise it needs to reinstate power before the GPU * can resume. This is achieved by vga_switcheroo_init_domain_pm_ops(), @@ -964,8 +957,9 @@ EXPORT_SYMBOL(vga_switcheroo_process_delayed_switch); * calls to the handler. * * When the audio device resumes, the GPU needs to be woken. This is achieved - * by vga_switcheroo_init_domain_pm_optimus_hdmi_audio(), which augments the - * audio device's resume function. + * by a PCI quirk which calls device_link_add() to declare a dependency on the + * GPU. That way, the GPU is kept awake whenever and as long as the audio + * device is in use. * * On muxed machines, if the mux is initially switched to the discrete GPU, * the user ends up with a black screen when the GPU powers down after boot. @@ -991,33 +985,6 @@ static void vga_switcheroo_power_switch(struct pci_dev *pdev, vgasr_priv.handler->power_state(client->id, state); }
-/** - * vga_switcheroo_set_dynamic_switch() - helper for driver power control - * @pdev: client pci device - * @dynamic: new power state - * - * Helper for GPUs whose power state is controlled by the driver's runtime pm. - * When the driver decides to power up or down, it notifies vga_switcheroo - * thereof using this helper so that it can power the audio device on the GPU - * up or down. - */ -void vga_switcheroo_set_dynamic_switch(struct pci_dev *pdev, - enum vga_switcheroo_state dynamic) -{ - struct vga_switcheroo_client *client; - - mutex_lock(&vgasr_mutex); - client = find_client_from_pci(&vgasr_priv.clients, pdev); - if (!client || !client->driver_power_control) { - mutex_unlock(&vgasr_mutex); - return; - } - - set_audio_state(client->id, dynamic); - mutex_unlock(&vgasr_mutex); -} -EXPORT_SYMBOL(vga_switcheroo_set_dynamic_switch); - /* switcheroo power domain */ static int vga_switcheroo_runtime_suspend(struct device *dev) { @@ -1089,69 +1056,3 @@ void vga_switcheroo_fini_domain_pm_ops(struct device *dev) dev_pm_domain_set(dev, NULL); } EXPORT_SYMBOL(vga_switcheroo_fini_domain_pm_ops); - -static int vga_switcheroo_runtime_resume_hdmi_audio(struct device *dev) -{ - struct pci_dev *pdev = to_pci_dev(dev); - struct vga_switcheroo_client *client; - struct device *video_dev = NULL; - int ret; - - /* we need to check if we have to switch back on the video - * device so the audio device can come back - */ - mutex_lock(&vgasr_mutex); - list_for_each_entry(client, &vgasr_priv.clients, list) { - if (PCI_SLOT(client->pdev->devfn) == PCI_SLOT(pdev->devfn) && - client_is_vga(client)) { - video_dev = &client->pdev->dev; - break; - } - } - mutex_unlock(&vgasr_mutex); - - if (video_dev) { - ret = pm_runtime_get_sync(video_dev); - if (ret && ret != 1) - return ret; - } - ret = dev->bus->pm->runtime_resume(dev); - - /* put the reference for the gpu */ - if (video_dev) { - pm_runtime_mark_last_busy(video_dev); - pm_runtime_put_autosuspend(video_dev); - } - return ret; -} - -/** - * vga_switcheroo_init_domain_pm_optimus_hdmi_audio() - helper for driver - * power control - * @dev: audio client device - * @domain: power domain - * - * Helper for GPUs whose power state is controlled by the driver's runtime pm. - * When the audio device resumes, the GPU needs to be woken. This helper - * augments the audio device's resume function to do that. - * - * Return: 0 on success, -EINVAL if no power management operations are - * defined for this device. - */ -int -vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, - struct dev_pm_domain *domain) -{ - /* copy over all the bus versions */ - if (dev->bus && dev->bus->pm) { - domain->ops = *dev->bus->pm; - domain->ops.runtime_resume = - vga_switcheroo_runtime_resume_hdmi_audio; - - dev_pm_domain_set(dev, domain); - return 0; - } - dev_pm_domain_set(dev, NULL); - return -EINVAL; -} -EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_optimus_hdmi_audio); diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index fc734014206f..ec582d37c189 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -26,6 +26,7 @@ #include <linux/ktime.h> #include <linux/mm.h> #include <linux/platform_data/x86/apple.h> +#include <linux/pm_runtime.h> #include <asm/dma.h> /* isa_dma_bridge_buggy */ #include "pci.h"
@@ -4832,3 +4833,41 @@ static void quirk_fsl_no_msi(struct pci_dev *pdev) pdev->no_msi = 1; } DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, quirk_fsl_no_msi); + +/* + * GPUs with integrated HDA controller for streaming audio to attached displays + * need a device link from the HDA controller (consumer) to the GPU (supplier) + * so that the GPU is powered up whenever the HDA controller is accessed. + * The GPU and HDA controller are functions 0 and 1 of the same PCI device. + * The device link stays in place until shutdown (or removal of the PCI device + * if it's hotplugged). Runtime PM is allowed by default on the HDA controller + * to prevent it from permanently keeping the GPU awake. + */ +static void quirk_gpu_hda(struct pci_dev *hda) +{ + struct pci_dev *gpu; + + if (PCI_FUNC(hda->devfn) != 1) + return; + + gpu = pci_get_domain_bus_and_slot(pci_domain_nr(hda->bus), + hda->bus->number, + PCI_DEVFN(PCI_SLOT(hda->devfn), 0)); + if (!gpu || (gpu->class >> 16) != PCI_BASE_CLASS_DISPLAY) { + pci_dev_put(gpu); + return; + } + + if (!device_link_add(&hda->dev, &gpu->dev, + DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME)) + pci_err(hda, "cannot link HDA to GPU %s\n", pci_name(gpu)); + + pm_runtime_allow(&hda->dev); + pci_dev_put(gpu); +} +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID, + PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda); +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMD, PCI_ANY_ID, + PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda); +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, + PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda); diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h index a6b30667a331..a637a7d8ce5b 100644 --- a/include/linux/pci_ids.h +++ b/include/linux/pci_ids.h @@ -45,6 +45,7 @@ #define PCI_CLASS_MULTIMEDIA_VIDEO 0x0400 #define PCI_CLASS_MULTIMEDIA_AUDIO 0x0401 #define PCI_CLASS_MULTIMEDIA_PHONE 0x0402 +#define PCI_CLASS_MULTIMEDIA_HD_AUDIO 0x0403 #define PCI_CLASS_MULTIMEDIA_OTHER 0x0480
#define PCI_BASE_CLASS_MEMORY 0x05 diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h index 960bedbdec87..77f0f0af3a71 100644 --- a/include/linux/vga_switcheroo.h +++ b/include/linux/vga_switcheroo.h @@ -168,11 +168,8 @@ 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); - int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain); void vga_switcheroo_fini_domain_pm_ops(struct device *dev); -int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain); #else
static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {} @@ -192,11 +189,8 @@ 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) {} - static inline int vga_switcheroo_init_domain_pm_ops(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; } static inline void vga_switcheroo_fini_domain_pm_ops(struct device *dev) {} -static inline int vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev, struct dev_pm_domain *domain) { return -EINVAL; }
#endif #endif /* _LINUX_VGA_SWITCHEROO_H_ */ diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 68169e3749de..5b2ed12f58ce 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -227,9 +227,6 @@ struct hdac_io_ops { #define HDA_UNSOL_QUEUE_SIZE 64 #define HDA_MAX_CODECS 8 /* limit by controller side */
-/* HD Audio class code */ -#define PCI_CLASS_MULTIMEDIA_HD_AUDIO 0x0403 - /* * CORB/RIRB * diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index c71dcacea807..ec4e6b829ee2 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1227,6 +1227,7 @@ static void azx_vs_set_state(struct pci_dev *pci, struct snd_card *card = pci_get_drvdata(pci); struct azx *chip = card->private_data; struct hda_intel *hda = container_of(chip, struct hda_intel, chip); + struct hda_codec *codec; bool disabled;
wait_for_completion(&hda->probe_wait); @@ -1251,8 +1252,12 @@ static void azx_vs_set_state(struct pci_dev *pci, dev_info(chip->card->dev, "%s via vga_switcheroo\n", disabled ? "Disabling" : "Enabling"); if (disabled) { - pm_runtime_put_sync_suspend(card->dev); - azx_suspend(card->dev); + list_for_each_codec(codec, &chip->bus) { + pm_runtime_suspend(hda_codec_dev(codec)); + pm_runtime_disable(hda_codec_dev(codec)); + } + pm_runtime_suspend(card->dev); + pm_runtime_disable(card->dev); /* when we get suspended by vga_switcheroo we end up in D3cold, * however we have no ACPI handle, so pci/acpi can't put us there, * put ourselves there */ @@ -1263,9 +1268,12 @@ static void azx_vs_set_state(struct pci_dev *pci, "Cannot lock devices!\n"); } else { snd_hda_unlock_devices(&chip->bus); - pm_runtime_get_noresume(card->dev); chip->disabled = false; - azx_resume(card->dev); + pm_runtime_enable(card->dev); + list_for_each_codec(codec, &chip->bus) { + pm_runtime_enable(hda_codec_dev(codec)); + pm_runtime_resume(hda_codec_dev(codec)); + } } } } @@ -1295,6 +1303,7 @@ static void init_vga_switcheroo(struct azx *chip) dev_info(chip->card->dev, "Handle vga_switcheroo audio client\n"); hda->use_vga_switcheroo = 1; + chip->driver_caps |= AZX_DCAPS_PM_RUNTIME; pci_dev_put(p); } } @@ -1320,9 +1329,6 @@ static int register_vga_switcheroo(struct azx *chip) return err; hda->vga_switcheroo_registered = 1;
- /* register as an optimus hdmi audio power domain */ - vga_switcheroo_init_domain_pm_optimus_hdmi_audio(chip->card->dev, - &hda->hdmi_pm_domain); return 0; } #else @@ -1351,10 +1357,8 @@ static int azx_free(struct azx *chip) if (use_vga_switcheroo(hda)) { if (chip->disabled && hda->probe_continued) snd_hda_unlock_devices(&chip->bus); - if (hda->vga_switcheroo_registered) { + if (hda->vga_switcheroo_registered) vga_switcheroo_unregister_client(chip->pci); - vga_switcheroo_fini_domain_pm_ops(chip->card->dev); - } }
if (bus->chip_init) { @@ -2197,6 +2201,7 @@ static int azx_probe_continue(struct azx *chip) struct hda_intel *hda = container_of(chip, struct hda_intel, chip); struct hdac_bus *bus = azx_bus(chip); struct pci_dev *pci = chip->pci; + struct hda_codec *codec; int dev = chip->dev_index; int err;
@@ -2278,8 +2283,17 @@ static int azx_probe_continue(struct azx *chip)
chip->running = 1; azx_add_card_list(chip); + + /* + * The discrete GPU cannot power down unless the HDA controller runtime + * suspends, so activate runtime PM on codecs even if power_save == 0. + */ + if (use_vga_switcheroo(hda)) + list_for_each_codec(codec, &chip->bus) + codec->auto_runtime_pm = 1; + snd_hda_set_power_save(&chip->bus, power_save * 1000); - if (azx_has_pm_runtime(chip) || hda->use_vga_switcheroo) + if (azx_has_pm_runtime(chip)) pm_runtime_put_autosuspend(&pci->dev);
out_free: diff --git a/sound/pci/hda/hda_intel.h b/sound/pci/hda/hda_intel.h index ff0c4d617bc1..e3a3d318d2e5 100644 --- a/sound/pci/hda/hda_intel.h +++ b/sound/pci/hda/hda_intel.h @@ -40,9 +40,6 @@ struct hda_intel { unsigned int vga_switcheroo_registered:1; unsigned int init_failed:1; /* delayed init failed */
- /* secondary power domain for hdmi audio under vga device */ - struct dev_pm_domain hdmi_pm_domain; - bool need_i915_power:1; /* the hda controller needs i915 power */ };
When switching the display on muxed machines, we currently force the HDA controller into runtime suspend on the previously used GPU and into runtime active state on the newly used GPU.
That's unnecessary if the GPU uses driver power control, we can just let the audio device autosuspend or autoresume as it sees fit.
Cc: Dave Airlie airlied@redhat.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Takashi Iwai tiwai@suse.de Cc: Peter Wu peter@lekensteyn.nl Cc: Alex Deucher alexander.deucher@amd.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/vga/vga_switcheroo.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c index 4ee0ed642386..fc4adf3d34e8 100644 --- a/drivers/gpu/vga/vga_switcheroo.c +++ b/drivers/gpu/vga/vga_switcheroo.c @@ -686,7 +686,9 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
active->active = false;
- set_audio_state(active->id, VGA_SWITCHEROO_OFF); + /* let HDA controller autosuspend if GPU uses driver power control */ + if (!active->driver_power_control) + set_audio_state(active->id, VGA_SWITCHEROO_OFF);
if (new_client->fb_info) { struct fb_event event; @@ -709,7 +711,9 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client) if (vga_switcheroo_pwr_state(active) == VGA_SWITCHEROO_ON) vga_switchoff(active);
- set_audio_state(new_client->id, VGA_SWITCHEROO_ON); + /* let HDA controller autoresume if GPU uses driver power control */ + if (!new_client->driver_power_control) + set_audio_state(new_client->id, VGA_SWITCHEROO_ON);
new_client->active = true; return 0;
Commit 5addcf0a5f0f ("nouveau: add runtime PM support (v0.9)") prevents runtime suspend of the GPU if its integrated HDA controller is not bound to a driver. The rationale appears to be that probing the HDA fails if the GPU is in D3cold.
However we now use a device link to ensure that the GPU is runtime resumed while the HDA controller is probed, rendering this safety measure obsolete. Remove it.
Cc: Dave Airlie airlied@redhat.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Takashi Iwai tiwai@suse.de Cc: Peter Wu peter@lekensteyn.nl Cc: Alex Deucher alexander.deucher@amd.com Cc: Rafael J. Wysocki rafael.j.wysocki@intel.com Signed-off-by: Lukas Wunner lukas@wunner.de --- drivers/gpu/drm/nouveau/nouveau_drm.c | 44 ----------------------------------- drivers/gpu/drm/nouveau/nouveau_drv.h | 1 - 2 files changed, 45 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 6959951d45d6..bbbf353682e1 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -510,37 +510,6 @@ static int nouveau_drm_probe(struct pci_dev *pdev, return 0; }
-#define PCI_CLASS_MULTIMEDIA_HD_AUDIO 0x0403 - -static void -nouveau_get_hdmi_dev(struct nouveau_drm *drm) -{ - struct pci_dev *pdev = drm->dev->pdev; - - if (!pdev) { - NV_DEBUG(drm, "not a PCI device; no HDMI\n"); - drm->hdmi_device = NULL; - return; - } - - /* subfunction one is a hdmi audio device? */ - drm->hdmi_device = pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus), - (unsigned int)pdev->bus->number, - PCI_DEVFN(PCI_SLOT(pdev->devfn), 1)); - - if (!drm->hdmi_device) { - NV_DEBUG(drm, "hdmi device not found %d %d %d\n", pdev->bus->number, PCI_SLOT(pdev->devfn), 1); - return; - } - - if ((drm->hdmi_device->class >> 8) != PCI_CLASS_MULTIMEDIA_HD_AUDIO) { - NV_DEBUG(drm, "possible hdmi device not audio %d\n", drm->hdmi_device->class); - pci_dev_put(drm->hdmi_device); - drm->hdmi_device = NULL; - return; - } -} - static int nouveau_drm_load(struct drm_device *dev, unsigned long flags) { @@ -568,8 +537,6 @@ nouveau_drm_load(struct drm_device *dev, unsigned long flags) INIT_LIST_HEAD(&drm->clients); spin_lock_init(&drm->tile.lock);
- nouveau_get_hdmi_dev(drm); - /* workaround an odd issue on nvc1 by disabling the device's * nosnoop capability. hopefully won't cause issues until a * better fix is found - assuming there is one... @@ -655,8 +622,6 @@ nouveau_drm_unload(struct drm_device *dev) nouveau_ttm_fini(drm); nouveau_vga_fini(drm);
- if (drm->hdmi_device) - pci_dev_put(drm->hdmi_device); nouveau_cli_fini(&drm->client); nouveau_cli_fini(&drm->master); kfree(drm); @@ -911,15 +876,6 @@ nouveau_pmops_runtime_idle(struct device *dev) return -EBUSY; }
- /* if we have a hdmi audio device - make sure it has a driver loaded */ - if (drm->hdmi_device) { - if (!drm->hdmi_device->driver) { - DRM_DEBUG_DRIVER("failing to power off - no HDMI audio driver loaded\n"); - pm_runtime_mark_last_busy(dev); - return -EBUSY; - } - } - list_for_each_entry(crtc, &drm->dev->mode_config.crtc_list, head) { if (crtc->enabled) { DRM_DEBUG_DRIVER("failing to power off - crtc active\n"); diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h index 96f6bd8aee5d..881b44b89a01 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drv.h +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h @@ -208,7 +208,6 @@ struct nouveau_drm { bool have_disp_power_ref;
struct dev_pm_domain vga_pm_domain; - struct pci_dev *hdmi_device; };
static inline struct nouveau_drm *
Hi Lukas,
Sorry for the delay, I finally found some time to reviewd and test the patches and found some issues (some of them might already be present in v4.15 without your patches though, I did not try).
Test environment: - Branch switcheroo_devlink_v2 (commit v4.15-20-gb33d50c5c6ad) - Laptop: Clevo P651RA (nouveau uses PR3), lspci: 00:01.0 PCI bridge [0604]: Intel Corporation Xeon E3-1200 v5/E3-1500 v5/6th Gen Core Processor PCIe Controller (x16) [8086:1901] (rev 07) 01:00.0 VGA compatible controller [0300]: NVIDIA Corporation GM204M [GeForce GTX 965M] [10de:13d9] (rev a1) 01:00.1 Audio device [0403]: NVIDIA Corporation GM204 High Definition Audio Controller [10de:0fbb] (rev a1) - Distribution: Arch Linux x86_64 - Tested from (1) console and (2) Xorg 1.19.3-2 (Openbox) - Booted with HDMI cable connected and nouveau/snd-hda-intel unloaded.
To check the runtime PM status, I used this "rpm-status" script:
grep --color . /sys/bus/pci/devices/0000:00:01.0/{0000:01:00.0/,0000:01:00.1/,}power/control grep --color . /sys/bus/pci/devices/0000:00:01.0/{0000:01:00.0/,0000:01:00.1/,}power/runtime_{enabled,usage,active_kids,status}
To test audio output (via HDMI; DP port does not seem to support audio):
speaker-test -Dhdmi:CARD=NVidia,DEV=0 -c2
Below I first list some issues, then some good news.
Issue 1 - GPU does not suspend on text console. When present at the text console and an external monitor is connected through HDMI or DP, the RPM counter is 1. Only when the cable is removed (or when "echo off > /sys/class/drm/card1-HDMI-A-1/status"), the RPM count drops to 0 and the GPU device suspends. When Xorg is started (startx), the RPM counter also drops to 0 though.
Issue 2 - RPM counter for audio function drops below 0 on system sleep. When both nouveau and snd-hda-intel are loaded and a HDMI (or DP?) cable is connected, the RPM counter becomes one after suspend/resume. This happens both with text console and Xorg.
Issue 3 - invalid PCI config reads to audio device if disconnected. When no HDMI/DP cable is connected, the HDMI audio function will be inaccessible after runtime/system resume. Assume nouveau loaded before s/r, then loading snd-hda-intel will fail with:
0]: pam_unix(sudo:session): session closed for user root hdaudio hdaudioC1D0: no AFG or MFG node found hdaudio hdaudioC1D1: no AFG or MFG node found hdaudio hdaudioC1D2: no AFG or MFG node found hdaudio hdaudioC1D3: no AFG or MFG node found hdaudio hdaudioC1D4: no AFG or MFG node found hdaudio hdaudioC1D5: no AFG or MFG node found hdaudio hdaudioC1D6: no AFG or MFG node found hdaudio hdaudioC1D7: no AFG or MFG node found snd_hda_intel 0000:01:00.1: no codecs initialized
After rmmod snd-hda-intel and system suspend/resume:
pci 0000:01:00.1: restoring config space at offset 0x3c (was 0xffffffff, writing 0x200) pci 0000:01:00.1: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0) pci 0000:01:00.1: restoring config space at offset 0x34 (was 0xffffffff, writing 0x60)
This persists until removing both PCI devices and rescanning the root port. (When no HDMI/DP cable is connected, the audio function will not appear; remove+rescan is required to recover.)
Issue 4 - runtime_active_kids leak with audio function. After the above issue, the audio device never entered the suspended state even though the runtime_usage counter reached 0. It turned out that runtime_active_kids was 4. Every time snd-hda-intel is loaded (and fails to initialize due to the above issue), this counter is increased.
Issue 5 - audio breaks after system sleep or stopping Xorg. When Xorg is stopped or the system sleep/resumes while speaker-test is active (e.g. in GNU screen), audio stops playing and speaker-test exits.
Issue 6 - wrong pin status reported / no audio (Possibly "working as expected" since audio is tied to GPU function.) Scenario: HDMI cable is connected but GPU is unused ("echo off > /sys/class/drm/card1-HDMI-1-1/status" from console or with "xrandr --output HDMI-A-1 --off"). hdajacksensetest reports no HDMI pin presence even if connected, dmesg reports:
nouveau 0000:01:00.0: DRM: DDC responded, but no EDID for HDMI-A-1
Using "speaker-test", the program does not fail but no sound can be heard either.
Issue 7 - nouveau: warning after unloading after stopping Xorg (Issue in nouveau, likely not related to this patch set.) After "xrandr --output HDMI-1-1 --mode 2560x1440" in Xorg, stopping Xorg (and possibly "echo off > /sys/class/drm/card1-HDMI-A-1/status"), removing nouveau triggered:
WARNING: CPU: 7 PID: 5475 at drivers/gpu/drm/drm_mode_config.c:439 drm_mode_config_cleanup+0x1fa/0x260 CPU: 7 PID: 5475 Comm: rmmod Not tainted 4.15.0testing-00020-gb33d50c5c6ad #55 RIP: 0010:drm_mode_config_cleanup+0x1fa/0x260 [..] Call Trace: nouveau_display_destroy+0x41/0x80 [nouveau] nouveau_drm_unload+0x6b/0xd0 [nouveau] drm_dev_unregister+0x3c/0xe0 drm_put_dev+0x2e/0x60 nouveau_drm_device_remove+0x37/0x50 [nouveau] pci_device_remove+0x36/0xb0 device_release_driver_internal+0x160/0x230 driver_detach+0x3a/0x70 bus_remove_driver+0x58/0xd0 pci_unregister_driver+0x3b/0x90 nouveau_drm_exit+0x15/0x432 [nouveau] SyS_delete_module+0x16c/0x230
Issue 8 - acpi: sleeping function in atomic context. (Issue is likely not related to this patch set.) At some point I also got a BUG, nouveau was already unloaded and I ran: "echo 1 | tee /sys/bus/pci/devices/0000:01:00.{0,1}/remove"
BUG: sleeping function called from invalid context at /home/peter/linux/mm/slab.h:419 in_atomic(): 1, irqs_disabled(): 0, pid: 4844, name: kworker/3:4 INFO: lockdep is turned off. CPU: 3 PID: 4844 Comm: kworker/3:4 Tainted: G W 4.15.0testing-00020-gb33d50c5c6ad #55 Hardware name: Notebook P65_P67RGRERA/P65_P67RGRERA, BIOS 1.05.16 05/16/2016 Workqueue: events_power_efficient srcu_invoke_callbacks Call Trace: dump_stack+0x5f/0x86 ___might_sleep+0x20c/0x240 kmem_cache_alloc_trace+0x4d/0x230 acpi_ut_evaluate_object+0x68/0x23c ? srcu_invoke_callbacks+0xa2/0x150 acpi_rs_get_prt_method_data+0x42/0xa2 acpi_get_irq_routing_table+0x70/0x9f ? __slab_free+0x11c/0x380 acpi_pci_irq_find_prt_entry+0x83/0x330 ? srcu_invoke_callbacks+0xa2/0x150 acpi_pci_irq_lookup+0x27/0x2e0 acpi_pci_irq_disable+0x45/0xb0 pci_release_dev+0x29/0x60 device_release+0x2d/0x80 kobject_put+0xb7/0x190 __device_link_free_srcu+0x32/0x40 srcu_invoke_callbacks+0xba/0x150 process_one_work+0x273/0x670 worker_thread+0x4a/0x400 kthread+0x100/0x140 ? process_one_work+0x670/0x670 ? kthread_create_worker_on_cpu+0x50/0x50 ? do_syscall_64+0x56/0x1a0 ? SyS_exit_group+0x10/0x10
Issue 9 - potential memory corruption. At some point (possibly after issue 7, but I am not fully sure), I saw an artifact in the text console which would persist even when switching between consoles. It was gone after system sleep/resume. If I remember correctly, it looked like something from a Xorg session which I killed before.
That was the bad news, the good news: - Loading nouveau and snd-hda-intel (in any order) while RPM is enabled and the port was in D3cold works. - RPM interaction between audio and GPU seems good, when audio resumes, the GPU RPM counter increments, when audio suspends it decrements. - As the GPU enters D3cold, I can observe significant power savings through /sys/class/power_supply/BAT0/ (no regressions here). - In a default configuration I have no audio function (see also nouveau bug https://bugs.freedesktop.org/show_bug.cgi?id=75985) so most of the above issues should not occur.
Hope it helps, and if desired you can add: Tested-by: Peter Wu peter@lekensteyn.nl
For the following patches, you can also add my Reviewed-by:
vga_switcheroo: Update PCI current_state on power change vga_switcheroo: Deduplicate power state tracking vga_switcheroo: Use device link for HDA controller vga_switcheroo: Let HDA autosuspend on mux change drm/nouveau: Runtime suspend despite HDA being unbound
The two other PCI patches look fine as well.
Kind regards, Peter
On Sat, Mar 03, 2018 at 10:53:24AM +0100, Lukas Wunner wrote:
Modernize vga_switcheroo by using a device link to enforce a runtime PM dependency from an HDA controller to the GPU it's integrated into, v2.
Changes since v1:
Replace patch [1/7] to use pci_save_state() / pci_restore_state() for consistency between runtime PM code path of bound and unbound devices. (Rafael, Bjorn)
Patch [5/7]: Drop an unnecessary initialization. (Bjorn) Rephrase error message on failed link addition for clarity.
Link to v1: https://www.spinics.net/lists/dri-devel/msg165889.html
Testing on more machines would be greatly appreciated, particularly Nvidia Optimus or AMD PowerXpress.
The series is based on 4.16-rc3. To test it on 4.15, you need to cherry-pick 7506dc798993 and 2fa6d6cdaf28. For your convenience I've pushed a 4.15-based branch to: https://github.com/l1k/linux/commits/switcheroo_devlink_v2
Minimal test procedure:
Note well: Recent Optimus require that a Mini-DP or HDMI cable is plugged in on boot for the HDA device to be present.
Check that HDA, GPU and root port autosuspend when not in use: cat /sys/bus/pci/devices/0000:01:00.1/power/runtime_status # HDA cat /sys/bus/pci/devices/0000:01:00.0/power/runtime_status # GPU cat /sys/bus/pci/devices/0000:00:01.0/power/runtime_status # Root Port
Check that all three autoresume when accessing the HDA: hdajacksensetest -c 1
Unbind the HDA controller: echo 0000:01:00.1 > /sys/bus/pci/drivers/snd_hda_intel/unbind Wait for GPU to power off, then rebind the HDA controller: echo 0000:01:00.1 > /sys/bus/pci/drivers/snd_hda_intel/bind Check dmesg for errors, try accessing HDA with hdajacksensetest.
If your laptop uses the root port's _PR3 to cut power to the GPU: Unbind the GPU: echo 0000:01:00.0 > /sys/bus/pci/drivers/{nouveau,amdgpu,radeon}/unbind Allow runtime PM on the GPU: echo auto > /sys/bus/pci/devices/0000:01:00.0/power/control Wait for GPU to power off, then rebind it: echo 0000:01:00.0 > /sys/bus/pci/drivers/{nouveau,amdgpu,radeon}/bind Check dmesg for errors. If you see any then we may need to perform further actions in pci_pm_runtime_resume(), see patch [1/7].
Thanks,
Lukas
Lukas Wunner (6): PCI: Make pci_wakeup_bus() & pci_bus_set_current_state() public vga_switcheroo: Update PCI current_state on power change vga_switcheroo: Deduplicate power state tracking vga_switcheroo: Use device link for HDA controller vga_switcheroo: Let HDA autosuspend on mux change drm/nouveau: Runtime suspend despite HDA being unbound
Rafael J. Wysocki (1): PCI: Restore config space on runtime resume despite being unbound
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 - drivers/gpu/drm/nouveau/nouveau_drm.c | 46 ---------- drivers/gpu/drm/nouveau/nouveau_drv.h | 1 - drivers/gpu/drm/radeon/radeon_drv.c | 2 - drivers/gpu/vga/vga_switcheroo.c | 152 ++++++++------------------------ drivers/pci/pci-driver.c | 17 ++-- drivers/pci/pci.c | 8 +- drivers/pci/quirks.c | 39 ++++++++ include/linux/pci.h | 2 + include/linux/pci_ids.h | 1 + include/linux/vga_switcheroo.h | 6 -- include/sound/hdaudio.h | 3 - sound/pci/hda/hda_intel.c | 36 +++++--- sound/pci/hda/hda_intel.h | 3 - 14 files changed, 117 insertions(+), 201 deletions(-)
-- 2.15.1
Hi Peter,
thanks a million for the extensive testing and reviewing.
I'll go through the issues you've found below, but it appears to me that they're all either in the "works as intended" category or are caused by something unrelated to this series:
On Mon, Mar 05, 2018 at 09:58:31PM +0100, Peter Wu wrote:
Issue 1 - GPU does not suspend on text console. When present at the text console and an external monitor is connected through HDMI or DP, the RPM counter is 1. Only when the cable is removed (or when "echo off > /sys/class/drm/card1-HDMI-A-1/status"), the RPM count drops to 0 and the GPU device suspends. When Xorg is started (startx), the RPM counter also drops to 0 though.
Yes, that behavior is intended: The GPU device is kept resumed as long as it has an active crtc, and it has an active crtc if it's used as text console.
Issue 2 - RPM counter for audio function drops below 0 on system sleep. When both nouveau and snd-hda-intel are loaded and a HDMI (or DP?) cable is connected, the RPM counter becomes one after suspend/resume. This happens both with text console and Xorg.
Which RPM counter becomes one? That of the GPU? If so, it would be caused by the GPU activating a crtc on hotplug to light up the display, hence staying resumed.
I don't quite understand what you mean by "RPM counter for audio function drops below 0 on system sleep". I've gone through the HDA code and don't see an unbalanced pm_runtime_get / _put. However the PM core changes the usage count a couple of times over a system sleep cycle by acquiring/releasing a runtime PM ref or enabling/disabling runtime PM (see drivers/base/power/main.c), maybe that's what you observed.
Issue 3 - invalid PCI config reads to audio device if disconnected. When no HDMI/DP cable is connected, the HDMI audio function will be inaccessible after runtime/system resume. Assume nouveau loaded before s/r, then loading snd-hda-intel will fail with:
hdaudio hdaudioC1D0: no AFG or MFG node found hdaudio hdaudioC1D1: no AFG or MFG node found hdaudio hdaudioC1D2: no AFG or MFG node found hdaudio hdaudioC1D3: no AFG or MFG node found hdaudio hdaudioC1D4: no AFG or MFG node found hdaudio hdaudioC1D5: no AFG or MFG node found hdaudio hdaudioC1D6: no AFG or MFG node found hdaudio hdaudioC1D7: no AFG or MFG node found snd_hda_intel 0000:01:00.1: no codecs initialized
Okay, that's really bad, but it appears to be caused by the BIOS hiding the audio function on resume if no cable is connected. I've attached 3 patches to this bugzilla to fix this issue: https://bugs.freedesktop.org/show_bug.cgi?id=75985
I've extensively tested system sleep in conjunction with the switcheroo_devlink patches and the above never occurred on my machine because the audio function is never hidden.
I don't consider this issue a blocker for the switcheroo_devlink patches because I would expect it to occur regardless. Quite to the contrary, the switcheroo_devlink patches are a requirement to fix the issue because the audio function needs to be exposed either from the GPU driver or a PCI quirk applied to the GPU, and the audio function needs to delay resume until that has happened, which is achieved by the device link.
Issue 4 - runtime_active_kids leak with audio function. After the above issue, the audio device never entered the suspended state even though the runtime_usage counter reached 0. It turned out that runtime_active_kids was 4. Every time snd-hda-intel is loaded (and fails to initialize due to the above issue), this counter is increased.
Yes, the codec devices are created as children of the HDA device and keep it awake because initialization failed. This won't occur once issue 3 is fixed.
Issue 5 - audio breaks after system sleep or stopping Xorg. When Xorg is stopped or the system sleep/resumes while speaker-test is active (e.g. in GNU screen), audio stops playing and speaker-test exits.
That is odd, I don't have an explanation for this but suspect a bug in the sound code.
Issue 6 - wrong pin status reported / no audio (Possibly "working as expected" since audio is tied to GPU function.) Scenario: HDMI cable is connected but GPU is unused ("echo off > /sys/class/drm/card1-HDMI-1-1/status" from console or with "xrandr --output HDMI-A-1 --off"). hdajacksensetest reports no HDMI pin presence even if connected, dmesg reports:
nouveau 0000:01:00.0: DRM: DDC responded, but no EDID for HDMI-A-1
Interesting, and yes, that would seem to be working as expected.
Issue 7 - nouveau: warning after unloading after stopping Xorg (Issue in nouveau, likely not related to this patch set.) After "xrandr --output HDMI-1-1 --mode 2560x1440" in Xorg, stopping Xorg (and possibly "echo off > /sys/class/drm/card1-HDMI-A-1/status"), removing nouveau triggered:
WARNING: CPU: 7 PID: 5475 at drivers/gpu/drm/drm_mode_config.c:439 drm_mode_config_cleanup+0x1fa/0x260
Okay, the driver was unloaded while the connector was still active. You should also see a "connector HDMI-1-1 leaked!" message in dmesg. I thought I had fixed this two years ago with commit 523872f6b072 ("drm/nouveau: Turn off CRTCs on driver unload"). Either that commit didn't work as it should or the issue reappeared. :-(
So yes, it's a bug, but unrelated to the switcheroo_devlink patches.
Issue 8 - acpi: sleeping function in atomic context. (Issue is likely not related to this patch set.) At some point I also got a BUG, nouveau was already unloaded and I ran: "echo 1 | tee /sys/bus/pci/devices/0000:01:00.{0,1}/remove"
BUG: sleeping function called from invalid context at /home/peter/linux/mm/slab.h:419
Wow, that would seem to be a bug in ACPI code, sorry, I'm clueless about that one. :-)
Once again thanks and let me know if unhiding the audio function as per the patches I attached to the above-linked bugzilla fixes issue 3.
Lukas
On Sat, Mar 03, 2018 at 10:53:24AM +0100, Lukas Wunner wrote:
Modernize vga_switcheroo by using a device link to enforce a runtime PM dependency from an HDA controller to the GPU it's integrated into, v2.
Changes since v1:
Replace patch [1/7] to use pci_save_state() / pci_restore_state() for consistency between runtime PM code path of bound and unbound devices. (Rafael, Bjorn)
Patch [5/7]: Drop an unnecessary initialization. (Bjorn) Rephrase error message on failed link addition for clarity.
Link to v1: https://www.spinics.net/lists/dri-devel/msg165889.html
Testing on more machines would be greatly appreciated, particularly Nvidia Optimus or AMD PowerXpress.
The series is based on 4.16-rc3. To test it on 4.15, you need to cherry-pick 7506dc798993 and 2fa6d6cdaf28. For your convenience I've pushed a 4.15-based branch to: https://github.com/l1k/linux/commits/switcheroo_devlink_v2
Minimal test procedure:
Note well: Recent Optimus require that a Mini-DP or HDMI cable is plugged in on boot for the HDA device to be present.
Check that HDA, GPU and root port autosuspend when not in use: cat /sys/bus/pci/devices/0000:01:00.1/power/runtime_status # HDA cat /sys/bus/pci/devices/0000:01:00.0/power/runtime_status # GPU cat /sys/bus/pci/devices/0000:00:01.0/power/runtime_status # Root Port
Check that all three autoresume when accessing the HDA: hdajacksensetest -c 1
Unbind the HDA controller: echo 0000:01:00.1 > /sys/bus/pci/drivers/snd_hda_intel/unbind Wait for GPU to power off, then rebind the HDA controller: echo 0000:01:00.1 > /sys/bus/pci/drivers/snd_hda_intel/bind Check dmesg for errors, try accessing HDA with hdajacksensetest.
If your laptop uses the root port's _PR3 to cut power to the GPU: Unbind the GPU: echo 0000:01:00.0 > /sys/bus/pci/drivers/{nouveau,amdgpu,radeon}/unbind Allow runtime PM on the GPU: echo auto > /sys/bus/pci/devices/0000:01:00.0/power/control Wait for GPU to power off, then rebind it: echo 0000:01:00.0 > /sys/bus/pci/drivers/{nouveau,amdgpu,radeon}/bind Check dmesg for errors. If you see any then we may need to perform further actions in pci_pm_runtime_resume(), see patch [1/7].
This all looks really reasonable and like a good cleanup, but it's a bit too much detail so I'll punt review to someone else with more clue. -Daniel
Thanks,
Lukas
Lukas Wunner (6): PCI: Make pci_wakeup_bus() & pci_bus_set_current_state() public vga_switcheroo: Update PCI current_state on power change vga_switcheroo: Deduplicate power state tracking vga_switcheroo: Use device link for HDA controller vga_switcheroo: Let HDA autosuspend on mux change drm/nouveau: Runtime suspend despite HDA being unbound
Rafael J. Wysocki (1): PCI: Restore config space on runtime resume despite being unbound
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 - drivers/gpu/drm/nouveau/nouveau_drm.c | 46 ---------- drivers/gpu/drm/nouveau/nouveau_drv.h | 1 - drivers/gpu/drm/radeon/radeon_drv.c | 2 - drivers/gpu/vga/vga_switcheroo.c | 152 ++++++++------------------------ drivers/pci/pci-driver.c | 17 ++-- drivers/pci/pci.c | 8 +- drivers/pci/quirks.c | 39 ++++++++ include/linux/pci.h | 2 + include/linux/pci_ids.h | 1 + include/linux/vga_switcheroo.h | 6 -- include/sound/hdaudio.h | 3 - sound/pci/hda/hda_intel.c | 36 +++++--- sound/pci/hda/hda_intel.h | 3 - 14 files changed, 117 insertions(+), 201 deletions(-)
-- 2.15.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Mar 06, 2018 at 11:29:40AM +0100, Daniel Vetter wrote:
On Sat, Mar 03, 2018 at 10:53:24AM +0100, Lukas Wunner wrote:
Modernize vga_switcheroo by using a device link to enforce a runtime PM dependency from an HDA controller to the GPU it's integrated into, v2.
This all looks really reasonable and like a good cleanup, but it's a bit too much detail so I'll punt review to someone else with more clue.
Patches [3/7] to [7/7] were reviewed by Peter Wu, the HDA bits in patch [5/7] additionally by Takashi.
Patch [2/7] was acked by Bjorn. There was no ack for patch [1/7] (authored by Rafael), but it adressed the objection Bjorn raised against my original patch, so I'm assuming Bjorn is okay with it. (Bjorn, please let me know if that isn't the case.)
The series has been tested on 5 systems, which raises the confidence: 2x AMD PowerXpress (Mike Lothian, Kai Heng Feng) 2x Nvidia Optimus (Denis Lisov, Peter Wu) 1x MacBook Pro
The issues found during Peter Wu's thorough testing appear to all be unrelated to this series, as per my e-mail yesterday.
If there are no objections, I plan to push the series to drm-misc-next by the middle of the coming week so that it would still catch the last train to 4.17.
Thanks,
Lukas
On Sun, Mar 11, 2018 at 04:55:49PM +0100, Lukas Wunner wrote:
On Tue, Mar 06, 2018 at 11:29:40AM +0100, Daniel Vetter wrote:
On Sat, Mar 03, 2018 at 10:53:24AM +0100, Lukas Wunner wrote:
Modernize vga_switcheroo by using a device link to enforce a runtime PM dependency from an HDA controller to the GPU it's integrated into, v2.
This all looks really reasonable and like a good cleanup, but it's a bit too much detail so I'll punt review to someone else with more clue.
Patches [3/7] to [7/7] were reviewed by Peter Wu, the HDA bits in patch [5/7] additionally by Takashi.
Patch [2/7] was acked by Bjorn. There was no ack for patch [1/7] (authored by Rafael), but it adressed the objection Bjorn raised against my original patch, so I'm assuming Bjorn is okay with it. (Bjorn, please let me know if that isn't the case.)
Since it's written by someone else your s-o-b counts as full review (wrt drm-misc rules at least as implemented by dim). So sounds like you have review for all the bits.
The series has been tested on 5 systems, which raises the confidence: 2x AMD PowerXpress (Mike Lothian, Kai Heng Feng) 2x Nvidia Optimus (Denis Lisov, Peter Wu) 1x MacBook Pro
The issues found during Peter Wu's thorough testing appear to all be unrelated to this series, as per my e-mail yesterday.
If there are no objections, I plan to push the series to drm-misc-next by the middle of the coming week so that it would still catch the last train to 4.17.
Please make sure all maintainers of other bits are ok with that and have given their formal ack for merging through drm-misc. With that you have until end of this week (but don't cut it too short) to sneak it into 4.17. Otherwise just push and it'll land in 4.18.
I think you've got all the other pieces. -Daniel
On Mon, Mar 12, 2018 at 05:54:47PM +0100, Daniel Vetter wrote:
On Sun, Mar 11, 2018 at 04:55:49PM +0100, Lukas Wunner wrote:
On Sat, Mar 03, 2018 at 10:53:24AM +0100, Lukas Wunner wrote:
Modernize vga_switcheroo by using a device link to enforce a runtime PM dependency from an HDA controller to the GPU it's integrated into, v2.
Patches [3/7] to [7/7] were reviewed by Peter Wu, the HDA bits in patch [5/7] additionally by Takashi.
Patch [2/7] was acked by Bjorn. There was no ack for patch [1/7] (authored by Rafael), but it adressed the objection Bjorn raised against my original patch, so I'm assuming Bjorn is okay with it. (Bjorn, please let me know if that isn't the case.)
If there are no objections, I plan to push the series to drm-misc-next by the middle of the coming week so that it would still catch the last train to 4.17.
Please make sure all maintainers of other bits are ok with that and have given their formal ack for merging through drm-misc.
Oh, I had thought Bjorn's acks for patches [2/7] and [5/7] and Takashi's reviewed-by for the HDA-related patches were sufficient.
Bjorn, Takashi, any objections against merging this through drm-misc?
Thanks,
Lukas
On Mon, 12 Mar 2018 21:55:50 +0100, Lukas Wunner wrote:
On Mon, Mar 12, 2018 at 05:54:47PM +0100, Daniel Vetter wrote:
On Sun, Mar 11, 2018 at 04:55:49PM +0100, Lukas Wunner wrote:
On Sat, Mar 03, 2018 at 10:53:24AM +0100, Lukas Wunner wrote:
Modernize vga_switcheroo by using a device link to enforce a runtime PM dependency from an HDA controller to the GPU it's integrated into, v2.
Patches [3/7] to [7/7] were reviewed by Peter Wu, the HDA bits in patch [5/7] additionally by Takashi.
Patch [2/7] was acked by Bjorn. There was no ack for patch [1/7] (authored by Rafael), but it adressed the objection Bjorn raised against my original patch, so I'm assuming Bjorn is okay with it. (Bjorn, please let me know if that isn't the case.)
If there are no objections, I plan to push the series to drm-misc-next by the middle of the coming week so that it would still catch the last train to 4.17.
Please make sure all maintainers of other bits are ok with that and have given their formal ack for merging through drm-misc.
Oh, I had thought Bjorn's acks for patches [2/7] and [5/7] and Takashi's reviewed-by for the HDA-related patches were sufficient.
Bjorn, Takashi, any objections against merging this through drm-misc?
No problem for me.
thanks,
Takashi
On Mon, Mar 12, 2018 at 09:55:50PM +0100, Lukas Wunner wrote:
On Mon, Mar 12, 2018 at 05:54:47PM +0100, Daniel Vetter wrote:
On Sun, Mar 11, 2018 at 04:55:49PM +0100, Lukas Wunner wrote:
On Sat, Mar 03, 2018 at 10:53:24AM +0100, Lukas Wunner wrote:
Modernize vga_switcheroo by using a device link to enforce a runtime PM dependency from an HDA controller to the GPU it's integrated into, v2.
Patches [3/7] to [7/7] were reviewed by Peter Wu, the HDA bits in patch [5/7] additionally by Takashi.
Patch [2/7] was acked by Bjorn. There was no ack for patch [1/7] (authored by Rafael), but it adressed the objection Bjorn raised against my original patch, so I'm assuming Bjorn is okay with it. (Bjorn, please let me know if that isn't the case.)
If there are no objections, I plan to push the series to drm-misc-next by the middle of the coming week so that it would still catch the last train to 4.17.
Please make sure all maintainers of other bits are ok with that and have given their formal ack for merging through drm-misc.
Oh, I had thought Bjorn's acks for patches [2/7] and [5/7] and Takashi's reviewed-by for the HDA-related patches were sufficient.
There's a huge confusion around acked-by "I think this looks reasonable", acked-by "this is reviewed but I don't feel like a full review tag" and acked-by "for merging through some other tree" and a few other meanings. Always good to double-check :-) -Daniel
Bjorn, Takashi, any objections against merging this through drm-misc?
Thanks,
Lukas
On Sun, Mar 11, 2018 at 04:55:49PM +0100, Lukas Wunner wrote:
On Tue, Mar 06, 2018 at 11:29:40AM +0100, Daniel Vetter wrote:
On Sat, Mar 03, 2018 at 10:53:24AM +0100, Lukas Wunner wrote:
Modernize vga_switcheroo by using a device link to enforce a runtime PM dependency from an HDA controller to the GPU it's integrated into, v2.
This all looks really reasonable and like a good cleanup, but it's a bit too much detail so I'll punt review to someone else with more clue.
Patches [3/7] to [7/7] were reviewed by Peter Wu, the HDA bits in patch [5/7] additionally by Takashi.
Patch [2/7] was acked by Bjorn. There was no ack for patch [1/7] (authored by Rafael), but it adressed the objection Bjorn raised against my original patch, so I'm assuming Bjorn is okay with it. (Bjorn, please let me know if that isn't the case.)
I am OK with it. I sent an ack and possible minor changelog tweak.
I expect that you'll merge the whole series via drm-misc.
The series has been tested on 5 systems, which raises the confidence: 2x AMD PowerXpress (Mike Lothian, Kai Heng Feng) 2x Nvidia Optimus (Denis Lisov, Peter Wu) 1x MacBook Pro
The issues found during Peter Wu's thorough testing appear to all be unrelated to this series, as per my e-mail yesterday.
If there are no objections, I plan to push the series to drm-misc-next by the middle of the coming week so that it would still catch the last train to 4.17.
Thanks,
Lukas
On Sun, Mar 11, 2018 at 04:55:49PM +0100, Lukas Wunner wrote:
On Sat, Mar 03, 2018 at 10:53:24AM +0100, Lukas Wunner wrote:
Modernize vga_switcheroo by using a device link to enforce a runtime PM dependency from an HDA controller to the GPU it's integrated into, v2.
If there are no objections, I plan to push the series to drm-misc-next by the middle of the coming week so that it would still catch the last train to 4.17.
Pushed to drm-misc-next now with Bjorn's changelog tweak and ack for patch [1/1].
Thanks a lot everyone for the reviews, acks, testing & comments.
Lukas
dri-devel@lists.freedesktop.org
-
Bjorn Helgaas -
Daniel Vetter -
Lukas Wunner -
Peter Wu -
Takashi Iwai