| Message ID | 20190828180128.1732-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | None | expand |
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 > >
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); }
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(-)