linux-kernel - Re: [alsa-devel] [PATCH v6 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] [day] [month] [year] [list]

Message-ID: <fd9d174e-febe-27ef-cc4c-f19007e21a1a@perex.cz>
Date:   Fri, 10 Jan 2020 11:31:29 +0100
From:   Jaroslav Kysela <perex@...ex.cz>
To:     Takashi Iwai <tiwai@...e.de>
Cc:     Kai-Heng Feng <kai.heng.feng@...onical.com>, bhelgaas@...gle.com,
        tiwai@...e.com, linux-pci@...r.kernel.org,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.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

Dne 10. 01. 20 v 10:56 Takashi Iwai napsal(a):
> 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.

Adding Cc: to dri-devel. You probably mean this thread:

https://lists.freedesktop.org/archives/dri-devel/2019-July/thread.html#227423

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

Wow.

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

Ok, it's a bit nightmare to maintain those extra settings in the distribution.

					Jaroslav

> 
> 
> 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@...onical.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@...ex.cz>
>> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
>>


-- 
Jaroslav Kysela <perex@...ex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.