[PATCH 0/7] Modernize vga_switcheroo by using device link for HDA

[PATCH 0/7] Modernize vga_switcheroo by using device link for HDA

[Lukas Wunner]

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.
Remove thereby obsoleted code and fix a bunch of bugs.
Device links were introduced in v4.10.

Users might see a small power saving if the discrete GPU is in use and
its HDA controller is not, because the HDA controller is now allowed
to runtime suspend to D3hot.  Probing and accessing the HDA controller
while the GPU is in D3cold should be very robust, unlike before.
Under the hood things become quite a bit leaner.

Also, this series gets us one step closer to supporting runtime PM on
muxed laptops such as the MacBook Pro because it fixes a deadlock
occurring when runtime resuming the discrete GPU on switching the mux.
(The deadlock occurs in vga_switcheroo_set_dynamic_switch() and that
function is obsoleted and removed by this series.)

The meat of the series is in patch [5/7], read its commit message for
details.  The other patches contain prep and cleanup work.

Patches [1/7], [2/7] and [5/7] require an ack from Bjorn (and Rafael?),
patch [5/7] requires an ack from Takashi.
Additionally I'd appreciate a Tested-by and/or Acked-by from Peter Wu,
the resident Nvidia Optimus expert, and from Alex for AMD PowerXpress
because my own testing only covers the MacBook Pro.
Testing and comments from anyone else are most welcome of course.

The series is based on 4.16-rc1.  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_v1

Minimal test procedure for non-Macs:

- 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 (7):
  PCI: Restore BARs on runtime resume despite being unbound
  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

 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                |   8 +-
 drivers/pci/pci.c                       |  10 +--
 drivers/pci/pci.h                       |   1 +
 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 -
 15 files changed, 114 insertions(+), 198 deletions(-)

-- 
2.15.1

[PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Lukas Wunner]

PCI devices not bound to a driver are supposed to stay 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.

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 restoring the BARs on runtime resume if the device is not bound.
This is sufficient to fix the above-mentioned use cases.  Other use
cases might require a full-blown pci_save_state() / pci_restore_state()
or execution of fixups.  We can add that once use cases materialize,
let's not inflate the code unnecessarily.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci-driver.c | 8 ++++++--
 drivers/pci/pci.c        | 2 +-
 drivers/pci/pci.h        | 1 +
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 3bed6beda051..51b11cbd48f6 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1277,10 +1277,14 @@ static int pci_pm_runtime_resume(struct device *dev)
 
 	/*
 	 * If pci_dev->driver is not set (unbound), the device should
-	 * always remain in D0 regardless of the runtime PM status
+	 * always remain in D0 regardless of the runtime PM status.
+	 * But if its parent can go to D3cold, this device may have
+	 * been in D3cold as well and require restoration of its BARs.
 	 */
-	if (!pci_dev->driver)
+	if (!pci_dev->driver) {
+		pci_restore_bars(pci_dev);
 		return 0;
+	}
 
 	if (!pm || !pm->runtime_resume)
 		return -ENOSYS;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f6a4dd10d9b0..f694650235f2 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -563,7 +563,7 @@ int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask)
  * Restore the BAR values for a given device, so as to make it
  * accessible by its driver.
  */
-static void pci_restore_bars(struct pci_dev *dev)
+void pci_restore_bars(struct pci_dev *dev)
 {
 	int i;
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fcd81911b127..29dc15bbe3bf 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -83,6 +83,7 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev);
 void pci_free_cap_save_buffers(struct pci_dev *dev);
 bool pci_bridge_d3_possible(struct pci_dev *dev);
 void pci_bridge_d3_update(struct pci_dev *dev);
+void pci_restore_bars(struct pci_dev *dev);
 
 static inline void pci_wakeup_event(struct pci_dev *dev)
 {
-- 
2.15.1

[PATCH 5/7] vga_switcheroo: Use device link for HDA controller

[Lukas Wunner]

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: 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>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 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..71338cc16909 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 = NULL;
+
+	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 add device link to %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 */
 };
 
-- 
2.15.1

[PATCH 2/7] PCI: Make pci_wakeup_bus() & pci_bus_set_current_state() public

[Lukas Wunner]

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: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.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 f694650235f2..6e6e322a5a7d 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);
-- 
2.15.1

[PATCH 3/7] vga_switcheroo: Update PCI current_state on power change

[Lukas Wunner]

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>
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;
-- 
2.15.1

[PATCH 4/7] vga_switcheroo: Deduplicate power state tracking

[Lukas Wunner]

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>
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);
 }
-- 
2.15.1

[PATCH 6/7] vga_switcheroo: Let HDA autosuspend on mux change

[Lukas Wunner]

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;
-- 
2.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/7] drm/nouveau: Runtime suspend despite HDA being unbound

[Lukas Wunner]

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 *
-- 
2.15.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/7] Modernize vga_switcheroo by using device link for HDA

[Lukas Wunner]

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.
Remove thereby obsoleted code and fix a bunch of bugs.
Device links were introduced in v4.10.

Users might see a small power saving if the discrete GPU is in use and
its HDA controller is not, because the HDA controller is now allowed
to runtime suspend to D3hot.  Probing and accessing the HDA controller
while the GPU is in D3cold should be very robust, unlike before.
Under the hood things become quite a bit leaner.

Also, this series gets us one step closer to supporting runtime PM on
muxed laptops such as the MacBook Pro because it fixes a deadlock
occurring when runtime resuming the discrete GPU on switching the mux.
(The deadlock occurs in vga_switcheroo_set_dynamic_switch() and that
function is obsoleted and removed by this series.)

The meat of the series is in patch [5/7], read its commit message for
details.  The other patches contain prep and cleanup work.

Patches [1/7], [2/7] and [5/7] require an ack from Bjorn (and Rafael?),
patch [5/7] requires an ack from Takashi.
Additionally I'd appreciate a Tested-by and/or Acked-by from Peter Wu,
the resident Nvidia Optimus expert, and from Alex for AMD PowerXpress
because my own testing only covers the MacBook Pro.
Testing and comments from anyone else are most welcome of course.

The series is based on 4.16-rc1.  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_v1

Minimal test procedure for non-Macs:

- 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 (7):
  PCI: Restore BARs on runtime resume despite being unbound
  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

 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                |   8 +-
 drivers/pci/pci.c                       |  10 +--
 drivers/pci/pci.h                       |   1 +
 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 -
 15 files changed, 114 insertions(+), 198 deletions(-)

-- 
2.15.1

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

[PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Lukas Wunner]

PCI devices not bound to a driver are supposed to stay 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.

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 restoring the BARs on runtime resume if the device is not bound.
This is sufficient to fix the above-mentioned use cases.  Other use
cases might require a full-blown pci_save_state() / pci_restore_state()
or execution of fixups.  We can add that once use cases materialize,
let's not inflate the code unnecessarily.

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/pci-driver.c | 8 ++++++--
 drivers/pci/pci.c        | 2 +-
 drivers/pci/pci.h        | 1 +
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 3bed6beda051..51b11cbd48f6 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1277,10 +1277,14 @@ static int pci_pm_runtime_resume(struct device *dev)
 
 	/*
 	 * If pci_dev->driver is not set (unbound), the device should
-	 * always remain in D0 regardless of the runtime PM status
+	 * always remain in D0 regardless of the runtime PM status.
+	 * But if its parent can go to D3cold, this device may have
+	 * been in D3cold as well and require restoration of its BARs.
 	 */
-	if (!pci_dev->driver)
+	if (!pci_dev->driver) {
+		pci_restore_bars(pci_dev);
 		return 0;
+	}
 
 	if (!pm || !pm->runtime_resume)
 		return -ENOSYS;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index f6a4dd10d9b0..f694650235f2 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -563,7 +563,7 @@ int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask)
  * Restore the BAR values for a given device, so as to make it
  * accessible by its driver.
  */
-static void pci_restore_bars(struct pci_dev *dev)
+void pci_restore_bars(struct pci_dev *dev)
 {
 	int i;
 
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index fcd81911b127..29dc15bbe3bf 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -83,6 +83,7 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev);
 void pci_free_cap_save_buffers(struct pci_dev *dev);
 bool pci_bridge_d3_possible(struct pci_dev *dev);
 void pci_bridge_d3_update(struct pci_dev *dev);
+void pci_restore_bars(struct pci_dev *dev);
 
 static inline void pci_wakeup_event(struct pci_dev *dev)
 {
-- 
2.15.1

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

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Rafael J. Wysocki]

On Sun, Feb 18, 2018 at 9:38 AM, Lukas Wunner <lukas@wunner.de> wrote:
> PCI devices not bound to a driver are supposed to stay 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.
>
> 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 restoring the BARs on runtime resume if the device is not bound.
> This is sufficient to fix the above-mentioned use cases.  Other use
> cases might require a full-blown pci_save_state() / pci_restore_state()
> or execution of fixups.  We can add that once use cases materialize,
> let's not inflate the code unnecessarily.
>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/pci-driver.c | 8 ++++++--
>  drivers/pci/pci.c        | 2 +-
>  drivers/pci/pci.h        | 1 +
>  3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3bed6beda051..51b11cbd48f6 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1277,10 +1277,14 @@ static int pci_pm_runtime_resume(struct device *dev)
>
>         /*
>          * If pci_dev->driver is not set (unbound), the device should
> -        * always remain in D0 regardless of the runtime PM status
> +        * always remain in D0 regardless of the runtime PM status.
> +        * But if its parent can go to D3cold, this device may have
> +        * been in D3cold as well and require restoration of its BARs.
>          */
> -       if (!pci_dev->driver)
> +       if (!pci_dev->driver) {
> +               pci_restore_bars(pci_dev);
>                 return 0;
> +       }
>
>         if (!pm || !pm->runtime_resume)
>                 return -ENOSYS;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f6a4dd10d9b0..f694650235f2 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -563,7 +563,7 @@ int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask)
>   * Restore the BAR values for a given device, so as to make it
>   * accessible by its driver.
>   */
> -static void pci_restore_bars(struct pci_dev *dev)
> +void pci_restore_bars(struct pci_dev *dev)
>  {
>         int i;
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index fcd81911b127..29dc15bbe3bf 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -83,6 +83,7 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev);
>  void pci_free_cap_save_buffers(struct pci_dev *dev);
>  bool pci_bridge_d3_possible(struct pci_dev *dev);
>  void pci_bridge_d3_update(struct pci_dev *dev);
> +void pci_restore_bars(struct pci_dev *dev);
>
>  static inline void pci_wakeup_event(struct pci_dev *dev)
>  {
> --
> 2.15.1
>

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Rafael J. Wysocki]

On Sun, Feb 18, 2018 at 9:38 AM, Lukas Wunner <lukas@wunner.de> wrote:
> PCI devices not bound to a driver are supposed to stay 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.
>
> 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 restoring the BARs on runtime resume if the device is not bound.
> This is sufficient to fix the above-mentioned use cases.  Other use
> cases might require a full-blown pci_save_state() / pci_restore_state()
> or execution of fixups.  We can add that once use cases materialize,
> let's not inflate the code unnecessarily.
>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/pci-driver.c | 8 ++++++--
>  drivers/pci/pci.c        | 2 +-
>  drivers/pci/pci.h        | 1 +
>  3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3bed6beda051..51b11cbd48f6 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1277,10 +1277,14 @@ static int pci_pm_runtime_resume(struct device *dev)
>
>         /*
>          * If pci_dev->driver is not set (unbound), the device should
> -        * always remain in D0 regardless of the runtime PM status
> +        * always remain in D0 regardless of the runtime PM status.
> +        * But if its parent can go to D3cold, this device may have
> +        * been in D3cold as well and require restoration of its BARs.
>          */
> -       if (!pci_dev->driver)
> +       if (!pci_dev->driver) {
> +               pci_restore_bars(pci_dev);
>                 return 0;
> +       }
>
>         if (!pm || !pm->runtime_resume)
>                 return -ENOSYS;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f6a4dd10d9b0..f694650235f2 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -563,7 +563,7 @@ int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask)
>   * Restore the BAR values for a given device, so as to make it
>   * accessible by its driver.
>   */
> -static void pci_restore_bars(struct pci_dev *dev)
> +void pci_restore_bars(struct pci_dev *dev)
>  {
>         int i;
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index fcd81911b127..29dc15bbe3bf 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -83,6 +83,7 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev);
>  void pci_free_cap_save_buffers(struct pci_dev *dev);
>  bool pci_bridge_d3_possible(struct pci_dev *dev);
>  void pci_bridge_d3_update(struct pci_dev *dev);
> +void pci_restore_bars(struct pci_dev *dev);
>
>  static inline void pci_wakeup_event(struct pci_dev *dev)
>  {
> --
> 2.15.1
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Bjorn Helgaas]

On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> PCI devices not bound to a driver are supposed to stay in D0 during
> runtime suspend.

Doesn't "runtime suspend" mean an individual device is suspended while
the rest of the system remains active?

If so, maybe it would be more direct to say "PCI devices not bound to
a driver cannot be runtime suspended"?

(It's a separate question not relevant to this patch, but I'm not
convinced that "PCI devices without a driver cannot be suspended"
should be accepted as a rule.  If it is a rule, we should be able to
deduce it from the specs.)

> 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.
> 
> 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 restoring the BARs on runtime resume if the device is not bound.
> This is sufficient to fix the above-mentioned use cases.  Other use
> cases might require a full-blown pci_save_state() / pci_restore_state()
> or execution of fixups.  We can add that once use cases materialize,
> let's not inflate the code unnecessarily.

Why would we not do a full-blown restore?  With this patch, I think
some configuration done during enumeration, e.g., ASPM and MPS, will
be lost.  "lspci -vv" of the HDA before and after the suspend may show
different things, which seems counter-intuitive.

I wouldn't think of a full-blown restore as "inflating the code"; I
would think of it as "having fewer special cases", i.e., we always use
the same restore path instead of having the main one plus a special
one for unbound devices.

> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/pci/pci-driver.c | 8 ++++++--
>  drivers/pci/pci.c        | 2 +-
>  drivers/pci/pci.h        | 1 +
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3bed6beda051..51b11cbd48f6 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1277,10 +1277,14 @@ static int pci_pm_runtime_resume(struct device *dev)
>  
>  	/*
>  	 * If pci_dev->driver is not set (unbound), the device should
> -	 * always remain in D0 regardless of the runtime PM status
> +	 * always remain in D0 regardless of the runtime PM status.
> +	 * But if its parent can go to D3cold, this device may have
> +	 * been in D3cold as well and require restoration of its BARs.
>  	 */
> -	if (!pci_dev->driver)
> +	if (!pci_dev->driver) {
> +		pci_restore_bars(pci_dev);

If we do decide not to do a full-blown restore, how do we decide
whether to use pci_restore_bars() or pci_restore_config_space()?

I'm not sure why we have both.  The pci_restore_bars() path looks a
little smarter in that it is more careful when updating 64-bit BARs
that can't be updated atomically.

>  		return 0;
> +	}
>  
>  	if (!pm || !pm->runtime_resume)
>  		return -ENOSYS;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f6a4dd10d9b0..f694650235f2 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -563,7 +563,7 @@ int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask)
>   * Restore the BAR values for a given device, so as to make it
>   * accessible by its driver.
>   */
> -static void pci_restore_bars(struct pci_dev *dev)
> +void pci_restore_bars(struct pci_dev *dev)
>  {
>  	int i;
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index fcd81911b127..29dc15bbe3bf 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -83,6 +83,7 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev);
>  void pci_free_cap_save_buffers(struct pci_dev *dev);
>  bool pci_bridge_d3_possible(struct pci_dev *dev);
>  void pci_bridge_d3_update(struct pci_dev *dev);
> +void pci_restore_bars(struct pci_dev *dev);
>  
>  static inline void pci_wakeup_event(struct pci_dev *dev)
>  {
> -- 
> 2.15.1
> 

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Bjorn Helgaas]

On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> PCI devices not bound to a driver are supposed to stay in D0 during
> runtime suspend.

Doesn't "runtime suspend" mean an individual device is suspended while
the rest of the system remains active?

If so, maybe it would be more direct to say "PCI devices not bound to
a driver cannot be runtime suspended"?

(It's a separate question not relevant to this patch, but I'm not
convinced that "PCI devices without a driver cannot be suspended"
should be accepted as a rule.  If it is a rule, we should be able to
deduce it from the specs.)

> 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.
> 
> 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 restoring the BARs on runtime resume if the device is not bound.
> This is sufficient to fix the above-mentioned use cases.  Other use
> cases might require a full-blown pci_save_state() / pci_restore_state()
> or execution of fixups.  We can add that once use cases materialize,
> let's not inflate the code unnecessarily.

Why would we not do a full-blown restore?  With this patch, I think
some configuration done during enumeration, e.g., ASPM and MPS, will
be lost.  "lspci -vv" of the HDA before and after the suspend may show
different things, which seems counter-intuitive.

I wouldn't think of a full-blown restore as "inflating the code"; I
would think of it as "having fewer special cases", i.e., we always use
the same restore path instead of having the main one plus a special
one for unbound devices.

> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/pci/pci-driver.c | 8 ++++++--
>  drivers/pci/pci.c        | 2 +-
>  drivers/pci/pci.h        | 1 +
>  3 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3bed6beda051..51b11cbd48f6 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1277,10 +1277,14 @@ static int pci_pm_runtime_resume(struct device *dev)
>  
>  	/*
>  	 * If pci_dev->driver is not set (unbound), the device should
> -	 * always remain in D0 regardless of the runtime PM status
> +	 * always remain in D0 regardless of the runtime PM status.
> +	 * But if its parent can go to D3cold, this device may have
> +	 * been in D3cold as well and require restoration of its BARs.
>  	 */
> -	if (!pci_dev->driver)
> +	if (!pci_dev->driver) {
> +		pci_restore_bars(pci_dev);

If we do decide not to do a full-blown restore, how do we decide
whether to use pci_restore_bars() or pci_restore_config_space()?

I'm not sure why we have both.  The pci_restore_bars() path looks a
little smarter in that it is more careful when updating 64-bit BARs
that can't be updated atomically.

>  		return 0;
> +	}
>  
>  	if (!pm || !pm->runtime_resume)
>  		return -ENOSYS;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index f6a4dd10d9b0..f694650235f2 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -563,7 +563,7 @@ int pci_wait_for_pending(struct pci_dev *dev, int pos, u16 mask)
>   * Restore the BAR values for a given device, so as to make it
>   * accessible by its driver.
>   */
> -static void pci_restore_bars(struct pci_dev *dev)
> +void pci_restore_bars(struct pci_dev *dev)
>  {
>  	int i;
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index fcd81911b127..29dc15bbe3bf 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -83,6 +83,7 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev);
>  void pci_free_cap_save_buffers(struct pci_dev *dev);
>  bool pci_bridge_d3_possible(struct pci_dev *dev);
>  void pci_bridge_d3_update(struct pci_dev *dev);
> +void pci_restore_bars(struct pci_dev *dev);
>  
>  static inline void pci_wakeup_event(struct pci_dev *dev)
>  {
> -- 
> 2.15.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] vga_switcheroo: Use device link for HDA controller

[Bjorn Helgaas]

On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> 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).

I guess this GPU wakeup happens *if* the path accessing the HDA
controller calls pm_runtime_get_sync() or similar, right?  We do have
that in the sysfs config accessors via pci_config_pm_runtime_get(),
but not in the sysfs mmap paths.  I think that's a latent PCI core
defect independent of this series.

We also don't have that in PCI core config accessors.  That maybe
doesn't matter because the core probably shouldn't be touching devices
after enumeration (although there might be holes there for things like
ASPM where a sysfs file can cause reconfiguration of several devices).

> 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: 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>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

(Trivial nit below.)

> ---
>  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..71338cc16909 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 = NULL;

Unnecessary initialization.

> +	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 add device link to %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 */
>  };
>  
> -- 
> 2.15.1
> 

Re: [PATCH 5/7] vga_switcheroo: Use device link for HDA controller

[Bjorn Helgaas]

On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> 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).

I guess this GPU wakeup happens *if* the path accessing the HDA
controller calls pm_runtime_get_sync() or similar, right?  We do have
that in the sysfs config accessors via pci_config_pm_runtime_get(),
but not in the sysfs mmap paths.  I think that's a latent PCI core
defect independent of this series.

We also don't have that in PCI core config accessors.  That maybe
doesn't matter because the core probably shouldn't be touching devices
after enumeration (although there might be holes there for things like
ASPM where a sysfs file can cause reconfiguration of several devices).

> 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: 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>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

(Trivial nit below.)

> ---
>  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..71338cc16909 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 = NULL;

Unnecessary initialization.

> +	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 add device link to %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 */
>  };
>  
> -- 
> 2.15.1
> 
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Rafael J. Wysocki]

On Tue, Feb 20, 2018 at 10:29 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
>> PCI devices not bound to a driver are supposed to stay in D0 during
>> runtime suspend.
>
> Doesn't "runtime suspend" mean an individual device is suspended while
> the rest of the system remains active?
>
> If so, maybe it would be more direct to say "PCI devices not bound to
> a driver cannot be runtime suspended"?
>
> (It's a separate question not relevant to this patch, but I'm not
> convinced that "PCI devices without a driver cannot be suspended"
> should be accepted as a rule.  If it is a rule, we should be able to
> deduce it from the specs.)
>
>> 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.
>>
>> 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 restoring the BARs on runtime resume if the device is not bound.
>> This is sufficient to fix the above-mentioned use cases.  Other use
>> cases might require a full-blown pci_save_state() / pci_restore_state()
>> or execution of fixups.  We can add that once use cases materialize,
>> let's not inflate the code unnecessarily.
>
> Why would we not do a full-blown restore?  With this patch, I think
> some configuration done during enumeration, e.g., ASPM and MPS, will
> be lost.  "lspci -vv" of the HDA before and after the suspend may show
> different things, which seems counter-intuitive.

Right.

> I wouldn't think of a full-blown restore as "inflating the code"; I
> would think of it as "having fewer special cases", i.e., we always use
> the same restore path instead of having the main one plus a special
> one for unbound devices.

That is a fair point, but if you look at pci_pm_runtime_suspend(), it
doesn't actually save anything for devices without drivers, so we'd
just restore the original initial state for them every time.

If we are to restore the entire state on runtime resume,
pci_pm_runtime_suspend() should be changed to call pci_save_state()
before returning 0 in the !dev->driver case AFAICS.

>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
>> ---
>>  drivers/pci/pci-driver.c | 8 ++++++--
>>  drivers/pci/pci.c        | 2 +-
>>  drivers/pci/pci.h        | 1 +
>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 3bed6beda051..51b11cbd48f6 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -1277,10 +1277,14 @@ static int pci_pm_runtime_resume(struct device *dev)
>>
>>       /*
>>        * If pci_dev->driver is not set (unbound), the device should
>> -      * always remain in D0 regardless of the runtime PM status
>> +      * always remain in D0 regardless of the runtime PM status.
>> +      * But if its parent can go to D3cold, this device may have
>> +      * been in D3cold as well and require restoration of its BARs.
>>        */
>> -     if (!pci_dev->driver)
>> +     if (!pci_dev->driver) {
>> +             pci_restore_bars(pci_dev);
>
> If we do decide not to do a full-blown restore, how do we decide
> whether to use pci_restore_bars() or pci_restore_config_space()?
>
> I'm not sure why we have both.

Me neither.

> The pci_restore_bars() path looks a
> little smarter in that it is more careful when updating 64-bit BARs
> that can't be updated atomically.
>
>>               return 0;
>> +     }
>>
>>       if (!pm || !pm->runtime_resume)
>>               return -ENOSYS;

So if pci_pm_runtime_suspend() is modified to call pci_save_state()
before returning 0 in the !dev->driver case, we can just move the
pci_restore_standard_config() invocation in pci_pm_runtime_resume() up
to the very top and check dev->driver later.

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Rafael J. Wysocki]

On Tue, Feb 20, 2018 at 10:29 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
>> PCI devices not bound to a driver are supposed to stay in D0 during
>> runtime suspend.
>
> Doesn't "runtime suspend" mean an individual device is suspended while
> the rest of the system remains active?
>
> If so, maybe it would be more direct to say "PCI devices not bound to
> a driver cannot be runtime suspended"?
>
> (It's a separate question not relevant to this patch, but I'm not
> convinced that "PCI devices without a driver cannot be suspended"
> should be accepted as a rule.  If it is a rule, we should be able to
> deduce it from the specs.)
>
>> 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.
>>
>> 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 restoring the BARs on runtime resume if the device is not bound.
>> This is sufficient to fix the above-mentioned use cases.  Other use
>> cases might require a full-blown pci_save_state() / pci_restore_state()
>> or execution of fixups.  We can add that once use cases materialize,
>> let's not inflate the code unnecessarily.
>
> Why would we not do a full-blown restore?  With this patch, I think
> some configuration done during enumeration, e.g., ASPM and MPS, will
> be lost.  "lspci -vv" of the HDA before and after the suspend may show
> different things, which seems counter-intuitive.

Right.

> I wouldn't think of a full-blown restore as "inflating the code"; I
> would think of it as "having fewer special cases", i.e., we always use
> the same restore path instead of having the main one plus a special
> one for unbound devices.

That is a fair point, but if you look at pci_pm_runtime_suspend(), it
doesn't actually save anything for devices without drivers, so we'd
just restore the original initial state for them every time.

If we are to restore the entire state on runtime resume,
pci_pm_runtime_suspend() should be changed to call pci_save_state()
before returning 0 in the !dev->driver case AFAICS.

>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
>> ---
>>  drivers/pci/pci-driver.c | 8 ++++++--
>>  drivers/pci/pci.c        | 2 +-
>>  drivers/pci/pci.h        | 1 +
>>  3 files changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 3bed6beda051..51b11cbd48f6 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -1277,10 +1277,14 @@ static int pci_pm_runtime_resume(struct device *dev)
>>
>>       /*
>>        * If pci_dev->driver is not set (unbound), the device should
>> -      * always remain in D0 regardless of the runtime PM status
>> +      * always remain in D0 regardless of the runtime PM status.
>> +      * But if its parent can go to D3cold, this device may have
>> +      * been in D3cold as well and require restoration of its BARs.
>>        */
>> -     if (!pci_dev->driver)
>> +     if (!pci_dev->driver) {
>> +             pci_restore_bars(pci_dev);
>
> If we do decide not to do a full-blown restore, how do we decide
> whether to use pci_restore_bars() or pci_restore_config_space()?
>
> I'm not sure why we have both.

Me neither.

> The pci_restore_bars() path looks a
> little smarter in that it is more careful when updating 64-bit BARs
> that can't be updated atomically.
>
>>               return 0;
>> +     }
>>
>>       if (!pm || !pm->runtime_resume)
>>               return -ENOSYS;

So if pci_pm_runtime_suspend() is modified to call pci_save_state()
before returning 0 in the !dev->driver case, we can just move the
pci_restore_standard_config() invocation in pci_pm_runtime_resume() up
to the very top and check dev->driver later.
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Rafael J. Wysocki]

On Wednesday, February 21, 2018 10:57:14 AM CET Rafael J. Wysocki wrote:
> On Tue, Feb 20, 2018 at 10:29 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> >> PCI devices not bound to a driver are supposed to stay in D0 during
> >> runtime suspend.
> >
> > Doesn't "runtime suspend" mean an individual device is suspended while
> > the rest of the system remains active?
> >
> > If so, maybe it would be more direct to say "PCI devices not bound to
> > a driver cannot be runtime suspended"?
> >
> > (It's a separate question not relevant to this patch, but I'm not
> > convinced that "PCI devices without a driver cannot be suspended"
> > should be accepted as a rule.  If it is a rule, we should be able to
> > deduce it from the specs.)
> >
> >> 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.
> >>
> >> 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 restoring the BARs on runtime resume if the device is not bound.
> >> This is sufficient to fix the above-mentioned use cases.  Other use
> >> cases might require a full-blown pci_save_state() / pci_restore_state()
> >> or execution of fixups.  We can add that once use cases materialize,
> >> let's not inflate the code unnecessarily.
> >
> > Why would we not do a full-blown restore?  With this patch, I think
> > some configuration done during enumeration, e.g., ASPM and MPS, will
> > be lost.  "lspci -vv" of the HDA before and after the suspend may show
> > different things, which seems counter-intuitive.
> 
> Right.
> 
> > I wouldn't think of a full-blown restore as "inflating the code"; I
> > would think of it as "having fewer special cases", i.e., we always use
> > the same restore path instead of having the main one plus a special
> > one for unbound devices.
> 
> That is a fair point, but if you look at pci_pm_runtime_suspend(), it
> doesn't actually save anything for devices without drivers, so we'd
> just restore the original initial state for them every time.
> 
> If we are to restore the entire state on runtime resume,
> pci_pm_runtime_suspend() should be changed to call pci_save_state()
> before returning 0 in the !dev->driver case AFAICS.
> 
> >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> >> ---
> >>  drivers/pci/pci-driver.c | 8 ++++++--
> >>  drivers/pci/pci.c        | 2 +-
> >>  drivers/pci/pci.h        | 1 +
> >>  3 files changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> >> index 3bed6beda051..51b11cbd48f6 100644
> >> --- a/drivers/pci/pci-driver.c
> >> +++ b/drivers/pci/pci-driver.c
> >> @@ -1277,10 +1277,14 @@ static int pci_pm_runtime_resume(struct device *dev)
> >>
> >>       /*
> >>        * If pci_dev->driver is not set (unbound), the device should
> >> -      * always remain in D0 regardless of the runtime PM status
> >> +      * always remain in D0 regardless of the runtime PM status.
> >> +      * But if its parent can go to D3cold, this device may have
> >> +      * been in D3cold as well and require restoration of its BARs.
> >>        */
> >> -     if (!pci_dev->driver)
> >> +     if (!pci_dev->driver) {
> >> +             pci_restore_bars(pci_dev);
> >
> > If we do decide not to do a full-blown restore, how do we decide
> > whether to use pci_restore_bars() or pci_restore_config_space()?
> >
> > I'm not sure why we have both.
> 
> Me neither.
> 
> > The pci_restore_bars() path looks a
> > little smarter in that it is more careful when updating 64-bit BARs
> > that can't be updated atomically.
> >
> >>               return 0;
> >> +     }
> >>
> >>       if (!pm || !pm->runtime_resume)
> >>               return -ENOSYS;
> 
> So if pci_pm_runtime_suspend() is modified to call pci_save_state()
> before returning 0 in the !dev->driver case, we can just move the
> pci_restore_standard_config() invocation in pci_pm_runtime_resume() up
> to the very top and check dev->driver later.

I mean something like the patch below, overall (untested).

Tentatively-signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci-driver.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -1224,11 +1224,14 @@ static int pci_pm_runtime_suspend(struct
 	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), the device may go to D3cold,
+	 * but only if the bridge above it is suspended.  In case that happens,
+	 * save its config space.
 	 */
-	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,17 @@ static int pci_pm_runtime_resume(struct
 	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
+	 * Regardless of whether or not the driver is there, the device might
+	 * have been put into D3cold as a result of suspending the bridge above
+	 * it, so restore the config spaces of all devices here.
 	 */
+	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);

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Rafael J. Wysocki]

On Wednesday, February 21, 2018 10:57:14 AM CET Rafael J. Wysocki wrote:
> On Tue, Feb 20, 2018 at 10:29 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> >> PCI devices not bound to a driver are supposed to stay in D0 during
> >> runtime suspend.
> >
> > Doesn't "runtime suspend" mean an individual device is suspended while
> > the rest of the system remains active?
> >
> > If so, maybe it would be more direct to say "PCI devices not bound to
> > a driver cannot be runtime suspended"?
> >
> > (It's a separate question not relevant to this patch, but I'm not
> > convinced that "PCI devices without a driver cannot be suspended"
> > should be accepted as a rule.  If it is a rule, we should be able to
> > deduce it from the specs.)
> >
> >> 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.
> >>
> >> 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 restoring the BARs on runtime resume if the device is not bound.
> >> This is sufficient to fix the above-mentioned use cases.  Other use
> >> cases might require a full-blown pci_save_state() / pci_restore_state()
> >> or execution of fixups.  We can add that once use cases materialize,
> >> let's not inflate the code unnecessarily.
> >
> > Why would we not do a full-blown restore?  With this patch, I think
> > some configuration done during enumeration, e.g., ASPM and MPS, will
> > be lost.  "lspci -vv" of the HDA before and after the suspend may show
> > different things, which seems counter-intuitive.
> 
> Right.
> 
> > I wouldn't think of a full-blown restore as "inflating the code"; I
> > would think of it as "having fewer special cases", i.e., we always use
> > the same restore path instead of having the main one plus a special
> > one for unbound devices.
> 
> That is a fair point, but if you look at pci_pm_runtime_suspend(), it
> doesn't actually save anything for devices without drivers, so we'd
> just restore the original initial state for them every time.
> 
> If we are to restore the entire state on runtime resume,
> pci_pm_runtime_suspend() should be changed to call pci_save_state()
> before returning 0 in the !dev->driver case AFAICS.
> 
> >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> >> ---
> >>  drivers/pci/pci-driver.c | 8 ++++++--
> >>  drivers/pci/pci.c        | 2 +-
> >>  drivers/pci/pci.h        | 1 +
> >>  3 files changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> >> index 3bed6beda051..51b11cbd48f6 100644
> >> --- a/drivers/pci/pci-driver.c
> >> +++ b/drivers/pci/pci-driver.c
> >> @@ -1277,10 +1277,14 @@ static int pci_pm_runtime_resume(struct device *dev)
> >>
> >>       /*
> >>        * If pci_dev->driver is not set (unbound), the device should
> >> -      * always remain in D0 regardless of the runtime PM status
> >> +      * always remain in D0 regardless of the runtime PM status.
> >> +      * But if its parent can go to D3cold, this device may have
> >> +      * been in D3cold as well and require restoration of its BARs.
> >>        */
> >> -     if (!pci_dev->driver)
> >> +     if (!pci_dev->driver) {
> >> +             pci_restore_bars(pci_dev);
> >
> > If we do decide not to do a full-blown restore, how do we decide
> > whether to use pci_restore_bars() or pci_restore_config_space()?
> >
> > I'm not sure why we have both.
> 
> Me neither.
> 
> > The pci_restore_bars() path looks a
> > little smarter in that it is more careful when updating 64-bit BARs
> > that can't be updated atomically.
> >
> >>               return 0;
> >> +     }
> >>
> >>       if (!pm || !pm->runtime_resume)
> >>               return -ENOSYS;
> 
> So if pci_pm_runtime_suspend() is modified to call pci_save_state()
> before returning 0 in the !dev->driver case, we can just move the
> pci_restore_standard_config() invocation in pci_pm_runtime_resume() up
> to the very top and check dev->driver later.

I mean something like the patch below, overall (untested).

Tentatively-signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/pci/pci-driver.c |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -1224,11 +1224,14 @@ static int pci_pm_runtime_suspend(struct
 	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), the device may go to D3cold,
+	 * but only if the bridge above it is suspended.  In case that happens,
+	 * save its config space.
 	 */
-	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,17 @@ static int pci_pm_runtime_resume(struct
 	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
+	 * Regardless of whether or not the driver is there, the device might
+	 * have been put into D3cold as a result of suspending the bridge above
+	 * it, so restore the config spaces of all devices here.
 	 */
+	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);

Re: [PATCH 2/7] PCI: Make pci_wakeup_bus() & pci_bus_set_current_state() public

[Bjorn Helgaas]

On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> 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: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  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 f694650235f2..6e6e322a5a7d 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);
> -- 
> 2.15.1
> 

Re: [PATCH 2/7] PCI: Make pci_wakeup_bus() & pci_bus_set_current_state() public

[Bjorn Helgaas]

On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> 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: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

> ---
>  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 f694650235f2..6e6e322a5a7d 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);
> -- 
> 2.15.1
> 
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 5/7] vga_switcheroo: Use device link for HDA controller

[Lukas Wunner]

On Tue, Feb 20, 2018 at 04:20:59PM -0600, Bjorn Helgaas wrote:
> On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> > 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).
> 
> I guess this GPU wakeup happens *if* the path accessing the HDA
> controller calls pm_runtime_get_sync() or similar, right?

Right.

> We do have
> that in the sysfs config accessors via pci_config_pm_runtime_get(),
> but not in the sysfs mmap paths.  I think that's a latent PCI core
> defect independent of this series.

Yes, there may be a few places where the parent of the device is
erroneously not runtime resumed when sysfs files are accessed.
I've never used the sysfs mmap feature, so never witnessed issues
with it.

> We also don't have that in PCI core config accessors.  That maybe
> doesn't matter because the core probably shouldn't be touching devices
> after enumeration (although there might be holes there for things like
> ASPM where a sysfs file can cause reconfiguration of several devices).

I assume with PCI core config accessors you're referring to
pci_read_config_word() and friends?  Those are arguably hotpaths
where we wouldn't want the overhead to check runtime PM status
everytime.  They might also be called from atomic context, I'm
not sure, and the runtime PM callbacks may sleep by default
(unless pm_runtime_irq_safe() was called).

> > +static void quirk_gpu_hda(struct pci_dev *hda)
> > +{
> > +	struct pci_dev *gpu = NULL;
> > +
> > +	if (PCI_FUNC(hda->devfn) != 1)
> > +		return;
> > +
> > +	gpu = pci_get_domain_bus_and_slot(pci_domain_nr(hda->bus),
> 
> Unnecessary initialization.

Thanks for spotting this, it's a remnant of an earlier version of the
patch which called pci_dev_put(gpu) in the (PCI_FUNC(hda->devfn) != 1)
case.

Best regards,

Lukas

Re: [PATCH 5/7] vga_switcheroo: Use device link for HDA controller

[Lukas Wunner]

On Tue, Feb 20, 2018 at 04:20:59PM -0600, Bjorn Helgaas wrote:
> On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> > 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).
> 
> I guess this GPU wakeup happens *if* the path accessing the HDA
> controller calls pm_runtime_get_sync() or similar, right?

Right.

> We do have
> that in the sysfs config accessors via pci_config_pm_runtime_get(),
> but not in the sysfs mmap paths.  I think that's a latent PCI core
> defect independent of this series.

Yes, there may be a few places where the parent of the device is
erroneously not runtime resumed when sysfs files are accessed.
I've never used the sysfs mmap feature, so never witnessed issues
with it.

> We also don't have that in PCI core config accessors.  That maybe
> doesn't matter because the core probably shouldn't be touching devices
> after enumeration (although there might be holes there for things like
> ASPM where a sysfs file can cause reconfiguration of several devices).

I assume with PCI core config accessors you're referring to
pci_read_config_word() and friends?  Those are arguably hotpaths
where we wouldn't want the overhead to check runtime PM status
everytime.  They might also be called from atomic context, I'm
not sure, and the runtime PM callbacks may sleep by default
(unless pm_runtime_irq_safe() was called).

> > +static void quirk_gpu_hda(struct pci_dev *hda)
> > +{
> > +	struct pci_dev *gpu = NULL;
> > +
> > +	if (PCI_FUNC(hda->devfn) != 1)
> > +		return;
> > +
> > +	gpu = pci_get_domain_bus_and_slot(pci_domain_nr(hda->bus),
> 
> Unnecessary initialization.

Thanks for spotting this, it's a remnant of an earlier version of the
patch which called pci_dev_put(gpu) in the (PCI_FUNC(hda->devfn) != 1)
case.

Best regards,

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

Re: [PATCH 5/7] vga_switcheroo: Use device link for HDA controller

[Bjorn Helgaas]

[+cc George]

On Fri, Feb 23, 2018 at 10:51:47AM +0100, Lukas Wunner wrote:
> On Tue, Feb 20, 2018 at 04:20:59PM -0600, Bjorn Helgaas wrote:
> > On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> > > 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).
> > 
> > I guess this GPU wakeup happens *if* the path accessing the HDA
> > controller calls pm_runtime_get_sync() or similar, right?
> 
> Right.
> 
> > We do have
> > that in the sysfs config accessors via pci_config_pm_runtime_get(),
> > but not in the sysfs mmap paths.  I think that's a latent PCI core
> > defect independent of this series.
> 
> Yes, there may be a few places where the parent of the device is
> erroneously not runtime resumed when sysfs files are accessed.
> I've never used the sysfs mmap feature, so never witnessed issues
> with it.
> 
> > We also don't have that in PCI core config accessors.  That maybe
> > doesn't matter because the core probably shouldn't be touching devices
> > after enumeration (although there might be holes there for things like
> > ASPM where a sysfs file can cause reconfiguration of several devices).
> 
> I assume with PCI core config accessors you're referring to
> pci_read_config_word() and friends?  Those are arguably hotpaths
> where we wouldn't want the overhead to check runtime PM status
> everytime.  They might also be called from atomic context, I'm
> not sure, and the runtime PM callbacks may sleep by default
> (unless pm_runtime_irq_safe() was called).

Yes, I was thinking of pci_read_config_word(), etc.  They're used by
the core during enumeration and occasionally by drivers, and both
cases already pay attention to PM.  I think we have a few holes in the
runtime sysfs area, but I think it's reasonable to deal with those in
the callers.

So I don't think those need to actually *do* anything with PM status,
although it might be interesting to add some (optional) checking
there.  George Cherian is turning up some issues in this area because
he has a root port that reports errors with an exception instead of
silently synthesizing all ones data like most hardware does [1].

Bjorn

[1] https://lkml.kernel.org/r/1517554846-16703-1-git-send-email-george.cherian@cavium.com

Re: [PATCH 5/7] vga_switcheroo: Use device link for HDA controller

[Bjorn Helgaas]

[+cc George]

On Fri, Feb 23, 2018 at 10:51:47AM +0100, Lukas Wunner wrote:
> On Tue, Feb 20, 2018 at 04:20:59PM -0600, Bjorn Helgaas wrote:
> > On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> > > 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).
> > 
> > I guess this GPU wakeup happens *if* the path accessing the HDA
> > controller calls pm_runtime_get_sync() or similar, right?
> 
> Right.
> 
> > We do have
> > that in the sysfs config accessors via pci_config_pm_runtime_get(),
> > but not in the sysfs mmap paths.  I think that's a latent PCI core
> > defect independent of this series.
> 
> Yes, there may be a few places where the parent of the device is
> erroneously not runtime resumed when sysfs files are accessed.
> I've never used the sysfs mmap feature, so never witnessed issues
> with it.
> 
> > We also don't have that in PCI core config accessors.  That maybe
> > doesn't matter because the core probably shouldn't be touching devices
> > after enumeration (although there might be holes there for things like
> > ASPM where a sysfs file can cause reconfiguration of several devices).
> 
> I assume with PCI core config accessors you're referring to
> pci_read_config_word() and friends?  Those are arguably hotpaths
> where we wouldn't want the overhead to check runtime PM status
> everytime.  They might also be called from atomic context, I'm
> not sure, and the runtime PM callbacks may sleep by default
> (unless pm_runtime_irq_safe() was called).

Yes, I was thinking of pci_read_config_word(), etc.  They're used by
the core during enumeration and occasionally by drivers, and both
cases already pay attention to PM.  I think we have a few holes in the
runtime sysfs area, but I think it's reasonable to deal with those in
the callers.

So I don't think those need to actually *do* anything with PM status,
although it might be interesting to add some (optional) checking
there.  George Cherian is turning up some issues in this area because
he has a root port that reports errors with an exception instead of
silently synthesizing all ones data like most hardware does [1].

Bjorn

[1] https://lkml.kernel.org/r/1517554846-16703-1-git-send-email-george.cherian@cavium.com
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Lukas Wunner]

[trimming cc a bit to avoid spamming folks likely uninterested in PCI PM
intricacies]

On Wed, Feb 21, 2018 at 10:57:14AM +0100, Rafael J. Wysocki wrote:
> On Tue, Feb 20, 2018 at 10:29 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Sun, Feb 18, 2018 at 09:38:32AM +0100, Lukas Wunner wrote:
> > > --- a/drivers/pci/pci-driver.c
> > > +++ b/drivers/pci/pci-driver.c
> > > @@ -1277,10 +1277,14 @@ static int pci_pm_runtime_resume(struct device *dev)
> > >
> > >       /*
> > >        * If pci_dev->driver is not set (unbound), the device should
> > > -      * always remain in D0 regardless of the runtime PM status
> > > +      * always remain in D0 regardless of the runtime PM status.
> > > +      * But if its parent can go to D3cold, this device may have
> > > +      * been in D3cold as well and require restoration of its BARs.
> > >        */
> > > -     if (!pci_dev->driver)
> > > +     if (!pci_dev->driver) {
> > > +             pci_restore_bars(pci_dev);
> >
> > If we do decide not to do a full-blown restore, how do we decide
> > whether to use pci_restore_bars() or pci_restore_config_space()?
> >
> > I'm not sure why we have both.
> 
> Me neither.

pci_restore_config_space() configures the BARs from saved_config_space[]
in struct pci_dev.  This array needs to have been populated beforehand.
If it's never been populated, the function can't be used.  The sole
caller of that function, pci_restore_state(), therefore checks whether
the state_saved bit in stuct pci_dev is true.

According to the code comment in the sole caller of pci_restore_bars(),
pci_raw_set_power_state(), there are systems where device are in D3hot
on boot.  Moreover devices where the No_Soft_Reset bit is 0 are in
D0uninitialized when coming out of D3hot.

For those devices, pci_restore_bars() is called to configure the BARs
from resource[] in struct pci_dev, which was populated on bus scan.
pci_restore_config_space() can't be used here because the first time
the devices are brought into D0, saved_config_space[] hasn't been
populated yet.  It's normally only populated when the device is suspended.

It might be possible to replace the invocation of pci_restore_bars()
with pci_restore_state() if pci_save_state() is called on bus scan for
devices in D3hot whose No_Soft_Reset bit is 0.

An alternative approach would be to avoid storing BARs in
saved_config_space[], and modify pci_restore_config_space() to restore
those from resource[].  That would save 24 bytes in struct pci_dev.
But it would only work if the BARs are always in sync with resource[],
I'm not sure if there are situations when this isn't the case.

Thanks,

Lukas

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Lukas Wunner]

On Wed, Feb 21, 2018 at 01:39:34PM +0100, Rafael J. Wysocki wrote:
> On Wednesday, February 21, 2018 10:57:14 AM CET Rafael J. Wysocki wrote:
> > So if pci_pm_runtime_suspend() is modified to call pci_save_state()
> > before returning 0 in the !dev->driver case, we can just move the
> > pci_restore_standard_config() invocation in pci_pm_runtime_resume() up
> > to the very top and check dev->driver later.
> 
> I mean something like the patch below, overall (untested).
> 
> Tentatively-signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Okay I've tested this successfully now.  I'll have to respin the series
at least one more time to address the unnecessary initialization Bjorn
spotted in patch [5/7] and will then replace patch [1/7] with this one.

I'll wait a few more days before respinning to allow for further
comments, in particular I'm hoping for feedback from Takashi and
someone testing this on Optimus/ATPX.

Thanks!

Lukas

> ---
>  drivers/pci/pci-driver.c |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -1224,11 +1224,14 @@ static int pci_pm_runtime_suspend(struct
>  	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), the device may go to D3cold,
> +	 * but only if the bridge above it is suspended.  In case that happens,
> +	 * save its config space.
>  	 */
> -	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,17 @@ static int pci_pm_runtime_resume(struct
>  	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
> +	 * Regardless of whether or not the driver is there, the device might
> +	 * have been put into D3cold as a result of suspending the bridge above
> +	 * it, so restore the config spaces of all devices here.
>  	 */
> +	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);
> 

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Lukas Wunner]

On Wed, Feb 21, 2018 at 01:39:34PM +0100, Rafael J. Wysocki wrote:
> On Wednesday, February 21, 2018 10:57:14 AM CET Rafael J. Wysocki wrote:
> > So if pci_pm_runtime_suspend() is modified to call pci_save_state()
> > before returning 0 in the !dev->driver case, we can just move the
> > pci_restore_standard_config() invocation in pci_pm_runtime_resume() up
> > to the very top and check dev->driver later.
> 
> I mean something like the patch below, overall (untested).
> 
> Tentatively-signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Okay I've tested this successfully now.  I'll have to respin the series
at least one more time to address the unnecessary initialization Bjorn
spotted in patch [5/7] and will then replace patch [1/7] with this one.

I'll wait a few more days before respinning to allow for further
comments, in particular I'm hoping for feedback from Takashi and
someone testing this on Optimus/ATPX.

Thanks!

Lukas

> ---
>  drivers/pci/pci-driver.c |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -1224,11 +1224,14 @@ static int pci_pm_runtime_suspend(struct
>  	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), the device may go to D3cold,
> +	 * but only if the bridge above it is suspended.  In case that happens,
> +	 * save its config space.
>  	 */
> -	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,17 @@ static int pci_pm_runtime_resume(struct
>  	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
> +	 * Regardless of whether or not the driver is there, the device might
> +	 * have been put into D3cold as a result of suspending the bridge above
> +	 * it, so restore the config spaces of all devices here.
>  	 */
> +	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);
> 

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Takashi Iwai]

On Sun, 25 Feb 2018 09:59:00 +0100,
Lukas Wunner wrote:
> 
> On Wed, Feb 21, 2018 at 01:39:34PM +0100, Rafael J. Wysocki wrote:
> > On Wednesday, February 21, 2018 10:57:14 AM CET Rafael J. Wysocki wrote:
> > > So if pci_pm_runtime_suspend() is modified to call pci_save_state()
> > > before returning 0 in the !dev->driver case, we can just move the
> > > pci_restore_standard_config() invocation in pci_pm_runtime_resume() up
> > > to the very top and check dev->driver later.
> > 
> > I mean something like the patch below, overall (untested).
> > 
> > Tentatively-signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Okay I've tested this successfully now.  I'll have to respin the series
> at least one more time to address the unnecessary initialization Bjorn
> spotted in patch [5/7] and will then replace patch [1/7] with this one.
> 
> I'll wait a few more days before respinning to allow for further
> comments, in particular I'm hoping for feedback from Takashi and
> someone testing this on Optimus/ATPX.

Sorry for the delay.  The patches look like a good cleanup to
straighten the code as well.  Unfortunately I have no hardware to test
right now, but feel free to take my ack for HD-audio related patches:
  Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH 1/7] PCI: Restore BARs on runtime resume despite being unbound

[Takashi Iwai]

On Sun, 25 Feb 2018 09:59:00 +0100,
Lukas Wunner wrote:
> 
> On Wed, Feb 21, 2018 at 01:39:34PM +0100, Rafael J. Wysocki wrote:
> > On Wednesday, February 21, 2018 10:57:14 AM CET Rafael J. Wysocki wrote:
> > > So if pci_pm_runtime_suspend() is modified to call pci_save_state()
> > > before returning 0 in the !dev->driver case, we can just move the
> > > pci_restore_standard_config() invocation in pci_pm_runtime_resume() up
> > > to the very top and check dev->driver later.
> > 
> > I mean something like the patch below, overall (untested).
> > 
> > Tentatively-signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Okay I've tested this successfully now.  I'll have to respin the series
> at least one more time to address the unnecessary initialization Bjorn
> spotted in patch [5/7] and will then replace patch [1/7] with this one.
> 
> I'll wait a few more days before respinning to allow for further
> comments, in particular I'm hoping for feedback from Takashi and
> someone testing this on Optimus/ATPX.

Sorry for the delay.  The patches look like a good cleanup to
straighten the code as well.  Unfortunately I have no hardware to test
right now, but feel free to take my ack for HD-audio related patches:
  Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

Re: [PATCH 0/7] Modernize vga_switcheroo by using device link for HDA

[Mike Lothian]

Hi

I've not seen anything untoward with these patches with my AMD PRIME system here

Tested-by: Mike Lothian <mike@fireburn.co.uk>

Cheers

Mike

Re: [PATCH 0/7] Modernize vga_switcheroo by using device link for HDA

[Mike Lothian]

Hi

I've not seen anything untoward with these patches with my AMD PRIME system here

Tested-by: Mike Lothian <mike@fireburn.co.uk>

Cheers

Mike
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel