[alsa-devel] [PATCH 1/2] PCI: Add a helper to check Power Resource Requirements _PR3 existence
A driver may want to know the existence of _PR3, to choose different runtime suspend behavior. A user will be add in next patch. 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"); +} +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
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); chip->driver_caps |= AZX_DCAPS_PM_RUNTIME; pci_dev_put(p); } -- 2.17.1
at 21:47, Kai-Heng Feng <kai.heng.feng@canonical.com> 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> —
Forgot to mention that for some platforms this issue happen after commit b516ea586d71 ("PCI: Enable NVIDIA HDA controllers”) which starts to unhide the “hidden” HDA. Kai-Heng
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 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
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(). And I thought using _PS3, _PR3, etc would be part of "platform power management"? And AFAICT, _PR3 merely returns a list of power resources; it doesn't disable power itself.
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
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
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. 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
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
Hi Bjorn, On Thu, Sep 5, 2019 at 11:35 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
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 think it can be both. I'll rephrase it.
- 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?
Yes, "unbound" in this context means the dGPU isn't bound to a driver.
- "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.
Yes, I'll make it clearer in next version. It's indeed referring to platform power management.
- "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.
Yes it means keep driver for dGPU unloaded. It's a common practice since the proprietary nvidia.ko doesn't support runtime power management, so when users are using integrated GPU, unload the dGPU driver can make PCI core use platform power management to disable the power source to save power.
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.
Will send a v2 with clearer description. 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'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); } -- 2.17.1
On Tue, 27 Aug 2019 15:47:55 +0200, 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.
This is mostly the same as nouveau_pr3_present().
Then it'd be nice to clean up the nouveau part, too? thanks, Takashi
at 23:25, Takashi Iwai <tiwai@suse.de> wrote:
On Tue, 27 Aug 2019 15:47:55 +0200, 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.
This is mostly the same as nouveau_pr3_present().
Then it'd be nice to clean up the nouveau part, too?
nouveau_pr3_present() may call pci_d3cold_disable(), and my intention is to only check the presence of _PR3 (i.e. a dGPU) without touching anything. Kai-Heng
thanks,
Takashi
[+cc Peter, Mika, Dave] https://lore.kernel.org/r/20190827134756.10807-1-kai.heng.feng@canonical.com On Wed, Aug 28, 2019 at 12:58:28AM +0800, Kai-Heng Feng wrote:
at 23:25, Takashi Iwai <tiwai@suse.de> wrote:
On Tue, 27 Aug 2019 15:47:55 +0200, 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.
This is mostly the same as nouveau_pr3_present().
Then it'd be nice to clean up the nouveau part, too?
nouveau_pr3_present() may call pci_d3cold_disable(), and my intention is to only check the presence of _PR3 (i.e. a dGPU) without touching anything.
It looks like Peter added that code with 279cf3f23870 ("drm/nouveau/acpi: use DSM if bridge does not support D3cold"). I don't understand the larger picture, but it is somewhat surprising that nouveau_pr3_present() *looks* like a simple predicate with no side-effects, but in fact it disables the use of D3cold in some cases. If the disable were moved to the caller, Kai-Heng's new interface could be used both places.
On Tue, Aug 27, 2019 at 05:13:21PM -0500, Bjorn Helgaas wrote:
[+cc Peter, Mika, Dave]
https://lore.kernel.org/r/20190827134756.10807-1-kai.heng.feng@canonical.com
On Wed, Aug 28, 2019 at 12:58:28AM +0800, Kai-Heng Feng wrote:
at 23:25, Takashi Iwai <tiwai@suse.de> wrote:
On Tue, 27 Aug 2019 15:47:55 +0200, 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.
This is mostly the same as nouveau_pr3_present().
Then it'd be nice to clean up the nouveau part, too?
nouveau_pr3_present() may call pci_d3cold_disable(), and my intention is to only check the presence of _PR3 (i.e. a dGPU) without touching anything.
It looks like Peter added that code with 279cf3f23870 ("drm/nouveau/acpi: use DSM if bridge does not support D3cold").
I don't understand the larger picture, but it is somewhat surprising that nouveau_pr3_present() *looks* like a simple predicate with no side-effects, but in fact it disables the use of D3cold in some cases.
The reason for disabling _PR3 from that point on is because mixing the ACPI firmware code that uses power resources (_PR3) with the legacy _DSM/_PS0/_PS3 methods to manage power states could break as that combination is unlikely to be supported nor tested by firmware authors. If a user sets /sys/bus/pci/devices/.../d3cold_allowed to 0, then the pci_d3cold_disable call ensures that this action is remembered and prevents power resources from being used again. For example, compare this power resource _OFF code: https://github.com/Lekensteyn/acpi-stuff/blob/b55f6bdb/dsl/Clevo_P651RA/ssdt... with this legacy _PS0/_PS3 code: https://github.com/Lekensteyn/acpi-stuff/blob/b55f6bdb/dsl/Clevo_P651RA/ssdt... The power resource code checks the "MSD3" variable to check whether a transition to OFF is required while the legacy _PS3 checks "DGPS". The sequence PG00._OFF followed by _DSM (to to change "OPCE") and _PS3 might trigger some device-specific code twice and could lead to lockups (infinite loops polling for power state) or worse. I am not sure if I have ever tested this scenario however.
If the disable were moved to the caller, Kai-Heng's new interface could be used both places.
Moving the pci_d3cold_disable call to the caller looks reasonable to me. After the first patch gets merged, nouveau could use something like: *has_pr3 = pci_pr3_present(pdev); if (*has_pr3 && !pdev->bridge_d3) { /* * ... */ pci_d3cold_disable(pdev); *has_pr3 = false; } For the 1/2 patch, Reviewed-by: Peter Wu <peter@lekensteyn.nl> -- Kind regards, Peter Wu https://lekensteyn.nl
Maybe: PCI: Add pci_pr3_present() to check for Power Resources for D3hot 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.
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? I'm wondering whether the "parent_pdev = pci_upstream_bridge()" part should be in the caller rather than in pci_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.
+} +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
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
[+cc Rafael] On Fri, Sep 20, 2019 at 01:23:20PM +0200, Kai-Heng Feng wrote:
On Mon, Sep 9, 2019 at 1:41 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
+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 cc'd Rafael because he knows all about how this stuff works, and I don't. _PR3 is defined in terms of the device itself and the doc (ACPI v6.3, sec 7.3.11) doesn't mention any hierarchy. I assume it would be legal for firmware to supply a _PR3 for "pdev" as well as for "parent_pdev"? If that is legal, I think it would be appropriate for the caller to look up the upstream bridge. That way this interface could be used for both "pdev" and an upstream bridge. If we look up the bridge internally, we would have to add a second interface if we actually want to know about _PR3 for the device itself.
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
participants (4)
-
Bjorn Helgaas -
Kai-Heng Feng -
Peter Wu -
Takashi Iwai