Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 10 Jan 2020 10:56:03 +0100
Message-ID: <s5ha76vmy8c.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Jaroslav Kysela <perex@perex.cz>
Cc:     Kai-Heng Feng <kai.heng.feng@canonical.com>, bhelgaas@google.com,
        tiwai@suse.com, linux-pci@vger.kernel.org,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org
Subject: Re: [alsa-devel] [PATCH v6 2/2] ALSA: hda: Allow HDA to be runtime suspended when dGPU is not bound to a driver
In-Reply-To: <10e35320-b7a8-0bcf-92d1-61aa5c057f58@perex.cz>
References: <20191018073848.14590-1-kai.heng.feng@canonical.com>
        <20191018073848.14590-2-kai.heng.feng@canonical.com>
        <10e35320-b7a8-0bcf-92d1-61aa5c057f58@perex.cz>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 10 Jan 2020 10:43:26 +0100,
Jaroslav Kysela wrote:
> 
> Dne 18. 10. 19 v 9:38 Kai-Heng Feng napsal(a):
> > 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.
> 
> This patch breaks the HDMI audio detection at least on some platforms
> (Lenovo P50 for example) with nouveau and the proprietary nvidia
> driver. Those laptops have the external HDMI/DP ports connected to
> dGPU instead the iGPU. The ACPI PR3 is set.
> 
> The runtime PM off fixes this problem:
> 
> echo on > /sys/bus/pci/devices/0000\:01\:00.1/power/control

But this will keep the power of the graphics chip on, and that's what
the patch was supposed to "fix".

> But I don't think that it's the best solution. My proposal is to
> create a pr3 check blacklist to keep power for the HDMI audio for
> those machines. Also we may add a new module parameter for
> snd-hda-intel to control this. Other ideas?

For nouveau, the best fix is to merge the audio component patch.
This will make things working without fiddling with the power
up/down.  The patch has been pending over months under review in DRM
side, unfortunately...  Please pinging them for driving ahead.

For Nvidia, though, it's no path a binary-only stuff can go with, due
to the GPL symbol of the component framework.  Those guys know of it
well, and they seem adding the temporary power up/down procedure by
poking the proc file from the user-space side at the HDMI connection.

About a module option: I don't think it's much better than the sysfs
toggle.  You can set up a simple udev rule if needed, too.


thanks,

Takashi

> 
> 					Jaroslav
> 
> 
> > 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>
> > ---
> > v5, v6:
> > - No change.
> > 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);
> >   	}
> >
> 
> 
> -- 
> Jaroslav Kysela <perex@perex.cz>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>