[v2,2/2] ALSA: hda: Allow HDA to be runtime suspended when dGPU is not bound

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(-)

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