linux-kernel - Re: [PATCH v4 2/2] ALSA: hda: Allow HDA to be runtime suspended when dGPU is not bound to a driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <F3E69B3D-E11B-4D99-905A-CC5927D61D6C@canonical.com>
Date:   Tue, 8 Oct 2019 02:49:56 +0800
From:   Kai-Heng Feng <kai.heng.feng@...onical.com>
To:     tiwai@...e.com
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        Linux PCI <linux-pci@...r.kernel.org>,
        alsa-devel@...a-project.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] ALSA: hda: Allow HDA to be runtime suspended when
 dGPU is not bound to a driver

Hi Takashi,

> On Sep 25, 2019, at 19:32, Kai-Heng Feng <kai.heng.feng@...onical.com> wrote:
> 
> 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@...onical.com>

Do you still have any concern on this patch?
Please merge [v5 1/2] and this patch [v4 2/2] if you think it's good.

Thanks!

Kai-Heng

> ---
> v4:
> - Find upstream port, it's callee's responsibility now.
> v3:
> - Make changelog more clear.
> v2:
> - Change wording.
> - Rebase to Tiwai's branch.
> sound/pci/hda/hda_intel.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 240f4ca76391..e63b871343e5 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1280,11 +1280,17 @@ static void init_vga_switcheroo(struct azx *chip)
> {
> 	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
> 	struct pci_dev *p = get_bound_vga(chip->pci);
> +	struct pci_dev *parent;
> 	if (p) {
> 		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
> +		 * upstream port has _PR3 (i.e. dGPU).
> +		 */
> +		parent = pci_upstream_bridge(p);
> +		chip->bus.keep_power = parent ? !pci_pr3_present(parent) : 1;
> 		chip->driver_caps |= AZX_DCAPS_PM_RUNTIME;
> 		pci_dev_put(p);
> 	}
> -- 
> 2.17.1
>