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 <[email protected]>
---
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 <[email protected]>
---
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 <[email protected]> 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 <[email protected]>
> —
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 <[email protected]>
> ---
> 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, 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 <[email protected]> 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/[email protected]
On Wed, Aug 28, 2019 at 12:58:28AM +0800, Kai-Heng Feng wrote:
> at 23:25, Takashi Iwai <[email protected]> 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 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 <[email protected]>
> ---
> 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, Aug 27, 2019 at 05:13:21PM -0500, Bjorn Helgaas wrote:
> [+cc Peter, Mika, Dave]
>
> https://lore.kernel.org/r/[email protected]
>
> On Wed, Aug 28, 2019 at 12:58:28AM +0800, Kai-Heng Feng wrote:
> > at 23:25, Takashi Iwai <[email protected]> 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/ssdt3.dsl#L454-L471
with this legacy _PS0/_PS3 code:
https://github.com/Lekensteyn/acpi-stuff/blob/b55f6bdb/dsl/Clevo_P651RA/ssdt7.dsl#L113-L142
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 <[email protected]>
--
Kind regards,
Peter Wu
https://lekensteyn.nl
Hi Bjorn,
at 06:31, Bjorn Helgaas <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
---
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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,
On Thu, Sep 5, 2019 at 11:35 PM Bjorn Helgaas <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
---
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
Hi Bjorn,
I didn't find your reply in my mailbox earlier.
On Mon, Sep 9, 2019 at 1:41 PM Bjorn Helgaas <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]> 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
> > >