On Tue, 27 Aug 2019 15:47:56 +0200, Kai-Heng Feng wrote:

It's a common practice to let dGPU unbound and use PCI port PM to disable its power through _PR3. When the dGPU comes with an HDA function, the HDA won't be suspended if the dGPU is unbound, so the dGPU power can't be disabled.

Commit 37a3a98ef601 ("ALSA: hda - Enable runtime PM only for discrete GPU") only allows HDA to be runtime-suspended once GPU is bound, to keep APU's HDA working.

However, HDA on dGPU isn't that useful if dGPU is unbound. So let relax the runtime suspend requirement for dGPU's HDA function, to save lots of power.

BugLink: https://bugs.launchpad.net/bugs/1840835 Signed-off-by: Kai-Heng Feng kai.heng.feng@canonical.com

---

sound/pci/hda/hda_intel.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 99fc0917339b..d4ee070e1a29 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1285,7 +1285,8 @@ static void init_vga_switcheroo(struct azx *chip) dev_info(chip->card->dev, "Handle vga_switcheroo audio client\n"); hda->use_vga_switcheroo = 1;

• hda->need_eld_notify_link = 1; /* cleared in gpu_bound op */
  

• /* cleared in gpu_bound op */
  

• hda->need_eld_notify_link = !pci_pr3_present(p);
  

Oh, right now I have a fix patch to submit for turning on the runtime PM behavior upon the audio component registration, essentially for amdgpu and nouveau. My fix includes the movement of this flag into hda_bus object, so this patch would become inapplicable (although it's trivial).

So I can apply this patch with the correction to sound git tree if the first patch gets ack from PCI maintainers (and they agree to apply over sound git tree).

In anyway, I'm going to post my patch that will conflict with this.

thanks,

Takashi

Hi Bjorn,

at 06:31, Bjorn Helgaas helgaas@kernel.org wrote:

On Tue, Aug 27, 2019 at 09:47:56PM +0800, Kai-Heng Feng wrote:

...

It's a common practice to let dGPU unbound and use PCI port PM to disable its power through _PR3. When the dGPU comes with an HDA function, the HDA won't be suspended if the dGPU is unbound, so the dGPU power can't be disabled.

Just a terminology question:

I thought "using PCI port PM" meant using the PCI Power Management Capability in config space to directly change the device's power state, e.g., in pci_raw_set_power_state().

What I meant is to use pcieport.ko directly.

And I thought using _PS3, _PR3, etc would be part of "platform power management”?

Ok, will update the wording.

And AFAICT, _PR3 merely returns a list of power resources; it doesn't disable power itself.

Yes, through its _PS3 and _OFF. I’ll update the wording.

Kai-Heng

...

Commit 37a3a98ef601 ("ALSA: hda - Enable runtime PM only for discrete GPU") only allows HDA to be runtime-suspended once GPU is bound, to keep APU's HDA working.

However, HDA on dGPU isn't that useful if dGPU is unbound. So let relax the runtime suspend requirement for dGPU's HDA function, to save lots of power.

BugLink: https://bugs.launchpad.net/bugs/1840835 Signed-off-by: Kai-Heng Feng kai.heng.feng@canonical.com

---

sound/pci/hda/hda_intel.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 99fc0917339b..d4ee070e1a29 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1285,7 +1285,8 @@ static void init_vga_switcheroo(struct azx *chip) dev_info(chip->card->dev, "Handle vga_switcheroo audio client\n"); hda->use_vga_switcheroo = 1;

• hda->need_eld_notify_link = 1; /* cleared in gpu_bound op */
  

• /* cleared in gpu_bound op */
  

• hda->need_eld_notify_link = !pci_pr3_present(p);
  

  chip->driver_caps |= AZX_DCAPS_PM_RUNTIME; pci_dev_put(p); }

-- 2.17.1

On Thu, Aug 29, 2019 at 02:01:28AM +0800, Kai-Heng Feng wrote:

It's a common practice to let dGPU unbound and use PCI platform power management to disable its power through _OFF method of power resource, which is listed by _PR3. When the dGPU comes with an HDA function, the HDA won't be suspended if the dGPU is unbound, so the power resource can't be turned off.

I'm not involved in applying this patch, but from the peanut gallery:

- The above looks like it might be two paragraphs missing a blank line between? Or maybe it's one paragraph that needs to be rewrapped?

- I can't parse the first sentence. I guess "dGPU" and "HDA" are hardware devices, but I don't know what "unbound" means. Is that something to do with a driver being bound to the dGPU? Or is it some connection between the dGPU and the HDA?

- "PCI platform power management" is still confusing -- I think we either have "PCI power management" that uses the PCI PM Capability (i.e., writing PCI_PM_CTRL as in pci_raw_set_power_state()) OR we have "platform power management" that uses some other method, typically ACPI. Since you refer to _OFF and _PR3, I guess you're referring to platform power management here.

- "It's common practice to let dGPU unbound" -- does that refer to some programming technique common in drivers, or some user-level thing like unloading a driver, or ...? My guess is it probably means "unbind the driver from the dGPU" but I still don't know what makes it common practice.

This probably all makes perfect sense to the graphics cognoscenti, but for the rest of us there are a lot of dots here that are not connected.

Commit 37a3a98ef601 ("ALSA: hda - Enable runtime PM only for discrete GPU") only allows HDA to be runtime-suspended once GPU is bound, to keep APU's HDA working.

However, HDA on dGPU isn't that useful if dGPU is unbound. So let's relax the runtime suspend requirement for dGPU's HDA function, to save lots of power.

BugLink: https://bugs.launchpad.net/bugs/1840835 Fixes: b516ea586d71 ("PCI: Enable NVIDIA HDA controllers”) Signed-off-by: Kai-Heng Feng kai.heng.feng@canonical.com

---

v2:

• Change wording.
• Rebase to Tiwai's branch.

sound/pci/hda/hda_intel.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 91e71be42fa4..c3654d22795a 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1284,7 +1284,11 @@ 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->bus.keep_power = 1; /* cleared in either gpu_bound op or codec probe */
  

• /* cleared in either gpu_bound op or codec probe, or when its
  

•  * root port has _PR3 (i.e. dGPU).
  

•  */
  

• chip->bus.keep_power = !pci_pr3_present(p);
  

  chip->driver_caps |= AZX_DCAPS_PM_RUNTIME; pci_dev_put(p); }

-- 2.17.1

Nvidia proprietary driver doesn't support runtime power management, so when a user only wants to use the integrated GPU, it's a common practice to let dGPU not to bind any driver, and let its upstream port to be runtime suspended. At the end of runtime suspension the port uses platform power management to disable power through _OFF method of power resource, which is listed by _PR3.

After commit b516ea586d71 ("PCI: Enable NVIDIA HDA controllers"), when the dGPU comes with an HDA function, the HDA won't be suspended if the dGPU is unbound, so the power resource can't be turned off by its upstream port driver.

Commit 37a3a98ef601 ("ALSA: hda - Enable runtime PM only for discrete GPU") only allows HDA to be runtime suspended once GPU is bound, to keep APU's HDA working.

However, HDA on dGPU isn't that useful if dGPU is not bound to any driver. So let's relax the runtime suspend requirement for dGPU's HDA function, to disable the power source to save lots of power.

BugLink: https://bugs.launchpad.net/bugs/1840835 Fixes: b516ea586d71 ("PCI: Enable NVIDIA HDA controllers") Signed-off-by: Kai-Heng Feng kai.heng.feng@canonical.com --- v3: - Make changelog more clear. v2: - Change wording. - Rebase to Tiwai's branch.

sound/pci/hda/hda_intel.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 91e71be42fa4..c3654d22795a 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1284,7 +1284,11 @@ 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->bus.keep_power = 1; /* cleared in either gpu_bound op or codec probe */ + + /* cleared in either gpu_bound op or codec probe, or when its + * root port has _PR3 (i.e. dGPU). + */ + chip->bus.keep_power = !pci_pr3_present(p); chip->driver_caps |= AZX_DCAPS_PM_RUNTIME; pci_dev_put(p); }

Hi Bjorn,

I didn't find your reply in my mailbox earlier.

On Mon, Sep 9, 2019 at 1:41 PM Bjorn Helgaas helgaas@kernel.org wrote:

Maybe:

PCI: Add pci_pr3_present() to check for Power Resources for D3hot

Ok, this is a good title.

On Tue, Aug 27, 2019 at 09:47:55PM +0800, Kai-Heng Feng wrote:

...

A driver may want to know the existence of _PR3, to choose different runtime suspend behavior. A user will be add in next patch.

Maybe include something like this in the commit lot?

Add pci_pr3_present() to check whether the platform supplies _PR3 to tell us which power resources the device depends on when in D3hot.

Ok.

...

This is mostly the same as nouveau_pr3_present().

Signed-off-by: Kai-Heng Feng kai.heng.feng@canonical.com

drivers/pci/pci.c | 20 ++++++++++++++++++++ include/linux/pci.h | 2 ++ 2 files changed, 22 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 1b27b5af3d55..776af15b92c2 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5856,6 +5856,26 @@ int pci_set_vga_state(struct pci_dev *dev, bool decode, return 0; }

+bool pci_pr3_present(struct pci_dev *pdev) +{

• struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
  

• struct acpi_device *parent_adev;
  

• if (acpi_disabled)
  

•         return false;
  

• if (!parent_pdev)
  

•         return false;
  

• parent_adev = ACPI_COMPANION(&parent_pdev->dev);
  

• if (!parent_adev)
  

•         return false;
  

• return parent_adev->power.flags.power_resources &&
  

•         acpi_has_method(parent_adev->handle, "_PR3");
  

I think this is generally OK, but it doesn't actually check whether *pdev* has a _PR3; it checks whether pdev's *parent* does. So does that mean this is dependent on the GPU topology, i.e., does it assume that there is an upstream bridge and that power for everything under that bridge can be managed together?

Yes, the power resource is managed by its upstream port.

I'm wondering whether the "parent_pdev = pci_upstream_bridge()" part should be in the caller rather than in pci_pr3_present()?

This will make the function more align to its name, but needs more work from caller side. How about rename the function to pci_upstream_pr3_present()?

I can't connect any of the dots from _PR3 through to "need_eld_notify_link" (whatever "eld" is :)) and the uses of hda_intel.need_eld_notify_link (and needs_eld_notify_link()).

But that's beyond the scope of *this* patch and it makes sense that you do want to discover the _PR3 existence, so I'm fine with this once we figure out the pdev vs parent question.

Thanks for your review.

Kai-Heng

...

+} +EXPORT_SYMBOL_GPL(pci_pr3_present);

/**

• pci_add_dma_alias - Add a DMA devfn alias for a device
• @dev: the PCI device for which alias is added

diff --git a/include/linux/pci.h b/include/linux/pci.h index 82e4cd1b7ac3..9b6f7b67fac9 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -2348,9 +2348,11 @@ struct irq_domain *pci_host_bridge_acpi_msi_domain(struct pci_bus *bus);

void pci_msi_register_fwnode_provider(struct fwnode_handle *(*fn)(struct device *)); +bool pci_pr3_present(struct pci_dev *pdev); #else static inline struct irq_domain * pci_host_bridge_acpi_msi_domain(struct pci_bus *bus) { return NULL; } +static bool pci_pr3_present(struct pci_dev *pdev) { return false; } #endif

#ifdef CONFIG_EEH

2.17.1