Re: 答复: 答复: [alsa-devel] 答复: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id
On 2018年07月10日 16:01, Takashi Iwai wrote: On Tue, 10 Jul 2018 09:44:42 +0200, Qu, Jim wrote: Hi Takashi, I am not expert in audio driver, so I just update some test result, please help triage the issue. With audio runtime pm: What does this mean? Is it the stock 4.17.x (or 4.18-rc)? Please clarify your test environment at first: which kernel, which hardware setup. This is v4.15 which backport Lukas' patches and also one bug fix https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?id=57cb54e53bdd. The platform is AMDAPU(with audio) + AMD dGPU , it is a hybird GFX platform. In generic, dGPU will always be suspended. 1. ubuntu audio setting use pactl to query audio card/output sink. Attachment is pactl output with audio runtime pm. 2. cat /proc/asound/card0/eld* can get nothing. Without audio runtime pm: 1. pactl can get available audio output/sink 2. can get eld info from eld#0.1 IMO, the issue should be: Current operations like HDMI hotplug in/out, pactl command, they do not access audio HW, so the audio could not resume back at this period. Sorry, not understood. Why they don't access the audio hardware? This is my guess, since I do not get azx_runtime_resume() print info which I added. it maybe access HW during this period, but do not trigger audio resume progress. <https://elixir.bootlin.com/linux/v4.18-rc3/ident/azx_runtime_resume> Therefore, upper software could not get HDMI ELD info, can select a available card/output sink. I am also confuse that if there is no ELD info, how driver to steam audio device to wake up itself? It's sort of a chicken-or-egg question. As long as it's i915 and HD-audio, the hotplug and ELD info transfer doesn't trigger the runtime PM since it's done via the direct callback. And ELD value is cached in HD-audio side. Yeah, I have reviewed these code, it constructs ELD after reading EDID, and write them into HW buffer when set display mode. The exception is that if the notification is done during the PM operation. But, the connection and ELD query is performed at the end of codec resume, hence it'll be covered there. So in theory, ELD information should be passed from the GPU to HD-audio no matter whether runtime PM is enabled or not. BTW, please stop top-posting. It makes the thread readability awfully worse. Sorry, I justed used outlook client. thanks, Takashi Thanks JimQu 发件人: Takashi Iwai 发送时间: 2018年7月10日 1:09 收件人: Qu, Jim 抄送: Daniel Vetter; Alex Deucher; alsa-de...@alsa-project.org; amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Deucher, Alexander 主题: Re: 答复: [alsa-devel]答复: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id On Mon, 09 Jul 2018 18:05:09 +0200, Qu, Jim wrote: Hi Takashi, Not intel, but it is AMD APU+ AMD GFX, the APU has a local HDMI port for extension. And dGPU is only for offloading render via PRIME. Originally, the HDA driver before v4.17, there is a bug, that all the audio is set to CLIENT_DIS, so the when the dGPU suspend, the audio which is bound to iGPU will also be suspend. Now, after Lukas' patches. The audio will auto suspend. It make ubuntu audio setting could not detect HDMI audio, even if HDMI has light up. OK, and it has all necessary patches including the one Lukas suggested, right? If so, figure out what you're actually seeing as "PA could not detect HDMI audio". For example, is it the HDMI (jack) detection (giving false even if it's connected)? Or is it an error at opening the given device? Does the driver give the proper ELD bytes as well as the jack state? Takashi Thanks JimQu -邮件原件- 发件人: Takashi Iwai 发送时间: 2018年7月9日 23:58 收件人: Daniel Vetter 抄送: Alex Deucher ; alsa-de...@alsa-project.org; amd-...@lists.freedesktop.org; Qu, Jim ; dri-devel@lists.freedesktop.org; Deucher, Alexander 主题: Re: [alsa-devel] 答复: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id On Mon, 09 Jul 2018 17:56:43 +0200, Daniel Vetter wrote: On Mon, Jul 09, 2018 at 05:04:22PM +0200, Takashi Iwai wrote: On Mon, 09 Jul 2018 15:58:51 +0200, Alex Deucher wrote: On Mon, Jul 9, 2018 at 6:16 AM, Qu, Jim wrote: Hi Lukas, Thanks to your explanation, and see comments in line. Do you need to runtime resume the HDA controller even if user space isn't streaming audio? Why, and in which situation exactly? Jim: OEM system uses pactl to queiry audio card and audio output sink, since the audio has power down by runtime pm, so the audio card and output sink are all unavailable. they could not select the available HDMI audio for audio playing. You're saying above that the HDA controller isn't runtime resumed on hotplug of a display. Is that necessary to retrieve ELD or something? I'm not sure if there's code in the HDA driver to acquire a runtime PM ref on HPD, but maybe it
Re: 答复: 答复: [alsa-devel] 答复: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id
On 2018年07月10日 17:50, Takashi Iwai wrote: On Tue, 10 Jul 2018 11:13:27 +0200, jimqu wrote: On 2018年07月10日 16:01, Takashi Iwai wrote: On Tue, 10 Jul 2018 09:44:42 +0200, Qu, Jim wrote: Hi Takashi, I am not expert in audio driver, so I just update some test result, please help triage the issue. With audio runtime pm: What does this mean? Is it the stock 4.17.x (or 4.18-rc)? Please clarify your test environment at first: which kernel, which hardware setup. This is v4.15 which backport Lukas' patches and also one bug fix https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/commit/?id=57cb54e53bdd. The platform is AMDAPU(with audio) + AMD dGPU , it is a hybird GFX platform. In generic, dGPU will always be suspended. Ah, so it's AMD+AMD, not Intel+AMD combo. Now I understand the situation better, thanks. 1. ubuntu audio setting use pactl to query audio card/output sink. Attachment is pactl output with audio runtime pm. 2. cat /proc/asound/card0/eld* can get nothing. Without audio runtime pm: 1. pactl can get available audio output/sink 2. can get eld info from eld#0.1 IMO, the issue should be: Current operations like HDMI hotplug in/out, pactl command, they do not access audio HW, so the audio could not resume back at this period. Sorry, not understood. Why they don't access the audio hardware? This is my guess, since I do not get azx_runtime_resume() print info which I added. it maybe access HW during this period, but do not trigger audio resume progress. <https://elixir.bootlin.com/linux/v4.18-rc3/ident/azx_runtime_resume> Therefore, upper software could not get HDMI ELD info, can select a available card/output sink. I am also confuse that if there is no ELD info, how driver to steam audio device to wake up itself? It's sort of a chicken-or-egg question. As long as it's i915 and HD-audio, the hotplug and ELD info transfer doesn't trigger the runtime PM since it's done via the direct callback. And ELD value is cached in HD-audio side. Yeah, I have reviewed these code, it constructs ELD after reading EDID, and write them into HW buffer when set display mode. If the controller chip supports "wake-enable" feature (HD-audio WAKEEN register), it could be woken up at unsolicited event even during the link power down. But, your report implies that AMD controller doesn't do this, or something missing there. That's the likely cause of the missing hotplug event. Is there any method to confirm it? So, if my understanding is right, the situation won't be different even if you have a single AMD GPU. And possibly a side-effect of the latest fix to force link-down for AMD GPU. Need to check it... Before Lukas' patches, it is relative with dGPU, because audio power is controlled by vgaswitchreroo and GFX driver, but now it won't. revert the fix of amdgpu suspend issue, audio issue also can be observed. Takashi The exception is that if the notification is done during the PM operation. But, the connection and ELD query is performed at the end of codec resume, hence it'll be covered there. So in theory, ELD information should be passed from the GPU to HD-audio no matter whether runtime PM is enabled or not. BTW, please stop top-posting. It makes the thread readability awfully worse. Sorry, I justed used outlook client. thanks, Takashi Thanks JimQu 发件人: Takashi Iwai 发送时间: 2018年7月10日 1:09 收件人: Qu, Jim 抄送: Daniel Vetter; Alex Deucher; alsa-de...@alsa-project.org; amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Deucher, Alexander 主题: Re: 答复: [alsa-devel]答复: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id On Mon, 09 Jul 2018 18:05:09 +0200, Qu, Jim wrote: Hi Takashi, Not intel, but it is AMD APU+ AMD GFX, the APU has a local HDMI port for extension. And dGPU is only for offloading render via PRIME. Originally, the HDA driver before v4.17, there is a bug, that all the audio is set to CLIENT_DIS, so the when the dGPU suspend, the audio which is bound to iGPU will also be suspend. Now, after Lukas' patches. The audio will auto suspend. It make ubuntu audio setting could not detect HDMI audio, even if HDMI has light up. OK, and it has all necessary patches including the one Lukas suggested, right? If so, figure out what you're actually seeing as "PA could not detect HDMI audio". For example, is it the HDMI (jack) detection (giving false even if it's connected)? Or is it an error at opening the given device? Does the driver give the proper ELD bytes as well as the jack state? Takashi Thanks JimQu -邮件原件- 发件人: Takashi Iwai 发送时间: 2018年7月9日 23:58 收件人: Daniel Vetter 抄送: Alex Deucher ; alsa-de...@alsa-project.org; amd-...@lists.freedesktop.org; Qu, Jim ; dri-devel@lists.freedesktop.org; Deucher, Alexander 主题: Re: [alsa-devel] 答复: [PATCH] vgasw
Re: 答复: 答复: [alsa-devel] 答复: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id
On 2018年07月11日 15:19, Takashi Iwai wrote: On Tue, 10 Jul 2018 13:21:00 +0200, Takashi Iwai wrote: revert the fix of amdgpu suspend issue, audio issue also can be observed. Did you check the behavior with the single AMD GPU hardware? If confirmed, we can forget about vga_switcheroo. ... and taking a look back at the recent changes, I guess it can be the forced runtime PM enablement, not directly with vga_switcheroo action itself. Yeah, the function vga_switcheroo_set_dynamic_switch() has discarded, so there is no way GFX driver to control audio power. However, keep in mind, current audio is bound to iGPU, that mean the issue should be nothing about vgaswtichreoo. since current audio pci bus is different from dGPU, that means the pci_bus_set_current_state() in vga_switcheroo_runtime_suspend() and pci_wakeup_bus() in vga_switcheroo_runtime_resume() could not touch the audio pci power state from dGPU instance. This is a feedback got from our OEM developer, it is the overview of audio detect process. First, the kernel audio driver will be triggered to read ELD, if the >> ELD is valid, it will report a jack event (on or available) to sound >> core driver; the pulseaudio subscribe all jack events, if it is told >> that the hdmi jack is plugged in (on), the pulseaudio will set this >> port to available, then the pa-card or pa-sink has available port, it >> can be selected (manually, some daemons or policy in >> /usr/share/pulseaudio/alsa-mixer/) as default output card/default sink. If the description is correct. I think there are maybe two problems. 1. audio will auto power off after setup device link duo to usage_count=0. 2. duo to audio is power down, it could not get the HDMI jack insert event. How do you think? Jim, could you tell me which PCI devices are handled as vga_switcheroo audio client? The kernel should show all messages "xxx: Handle vga_switcheroo audio client". [ 4.311095] snd_hda_intel :06:00.1: enabling device ( -> 0002) [ 4.314286] snd_hda_intel :06:00.1: Handle vga_switcheroo audio client [ 4.314822] snd_hda_intel :06:00.6: enabling device ( -> 0002) 01:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. [AMD/ATI] Device [1002:699f] (rev c3) 06:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. [AMD/ATI] Device [1002:15dd] (rev d1) 06:00.1 Audio device [0403]: Advanced Micro Devices, Inc. [AMD/ATI] Device [1002:15de] Thanks JimQu At best, give the full dmesg output and the lspci -nv output. thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: 答复: 答复: [alsa-devel] 答复: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id
On 2018年07月11日 17:04, Takashi Iwai wrote: On Wed, 11 Jul 2018 10:41:38 +0200, jimqu wrote: On 2018年07月11日 15:19, Takashi Iwai wrote: On Tue, 10 Jul 2018 13:21:00 +0200, Takashi Iwai wrote: revert the fix of amdgpu suspend issue, audio issue also can be observed. Did you check the behavior with the single AMD GPU hardware? If confirmed, we can forget about vga_switcheroo. ... and taking a look back at the recent changes, I guess it can be the forced runtime PM enablement, not directly with vga_switcheroo action itself. Yeah, the function vga_switcheroo_set_dynamic_switch() has discarded, so there is no way GFX driver to control audio power. However, keep in mind, current audio is bound to iGPU, that mean the issue should be nothing about vgaswtichreoo. since current audio pci bus is different from dGPU, that means the pci_bus_set_current_state() in vga_switcheroo_runtime_suspend() and pci_wakeup_bus() in vga_switcheroo_runtime_resume() could not touch the audio pci power state from dGPU instance. This is a feedback got from our OEM developer, it is the overview of audio detect process. First, the kernel audio driver will be triggered to read ELD, if the ELD is valid, it will report a jack event (on or available) to sound core driver; the pulseaudio subscribe all jack events, if it is told that the hdmi jack is plugged in (on), the pulseaudio will set this port to available, then the pa-card or pa-sink has available port, it can be selected (manually, some daemons or policy in /usr/share/pulseaudio/alsa-mixer/) as default output card/default sink. If the description is correct. I think there are maybe two problems. 1. audio will auto power off after setup device link duo to usage_count=0. 2. duo to audio is power down, it could not get the HDMI jack insert event. How do you think? Jim, could you tell me which PCI devices are handled as vga_switcheroo audio client? The kernel should show all messages "xxx: Handle vga_switcheroo audio client". [ 4.311095] snd_hda_intel :06:00.1: enabling device ( -> 0002) [ 4.314286] snd_hda_intel :06:00.1: Handle vga_switcheroo audio client [ 4.314822] snd_hda_intel :06:00.6: enabling device ( -> 0002) 01:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. [AMD/ATI] Device [1002:699f] (rev c3) 06:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. [AMD/ATI] Device [1002:15dd] (rev d1) 06:00.1 Audio device [0403]: Advanced Micro Devices, Inc. [AMD/ATI] Device [1002:15de] OK this sheds a brighter light, finally. If my understanding is correct, the issue is a false vga_switcheroo audio detection, after all. This is the primary GPU and it shouldn't be registered as a vga_switcheroo discrete GPU. Below is a very ugly workaround for this particular case. It assumes that the AMD+AMD combo will never have audio outputs on both but only for the primary, and it's possibly wrong. Is there a handy way to identify whether the given VGA PCI entry is a discrete GPU or not? The amdgpu and radeon seem checking ATPX ACPI. This is no issue about this topic, in amdgpu driver, both iGPU/dGPU will register as VGA_SWITCHEROO_UNKNOWN_ID, and the client id will be re-initialized in vgaswitchreoo_enable() via ATPX call. Then, iGPU will set as VGA_SWITCHEROO_IGD, and dGPU will set as VGA_SWITCHEROO_DIS. I think current focus should be how to detect HDMI audio device under audio suspend state. Thanks JimQu Takashi --- --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1418,8 +1418,18 @@ static int azx_dev_free(struct snd_device *device) */ static struct pci_dev *get_bound_vga(struct pci_dev *pci) { + static const struct pci_device_id ids[] = { + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_ANY_ID), + .class = PCI_BASE_CLASS_DISPLAY << 16, + .class_mask = 0xff << 16 }, + {} + }; struct pci_dev *p; + /* check whether Intel graphics is present as primary GPU */ + if (!pci_dev_present(ids)) + return NULL; + /* check only discrete GPU */ switch (pci->vendor) { case PCI_VENDOR_ID_ATI: ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: 答复: 答复: [alsa-devel] 答复: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id
On 2018年07月11日 17:53, Takashi Iwai wrote: On Wed, 11 Jul 2018 11:26:11 +0200, jimqu wrote: On 2018年07月11日 17:04, Takashi Iwai wrote: On Wed, 11 Jul 2018 10:41:38 +0200, jimqu wrote: On 2018年07月11日 15:19, Takashi Iwai wrote: On Tue, 10 Jul 2018 13:21:00 +0200, Takashi Iwai wrote: revert the fix of amdgpu suspend issue, audio issue also can be observed. Did you check the behavior with the single AMD GPU hardware? If confirmed, we can forget about vga_switcheroo. ... and taking a look back at the recent changes, I guess it can be the forced runtime PM enablement, not directly with vga_switcheroo action itself. Yeah, the function vga_switcheroo_set_dynamic_switch() has discarded, so there is no way GFX driver to control audio power. However, keep in mind, current audio is bound to iGPU, that mean the issue should be nothing about vgaswtichreoo. since current audio pci bus is different from dGPU, that means the pci_bus_set_current_state() in vga_switcheroo_runtime_suspend() and pci_wakeup_bus() in vga_switcheroo_runtime_resume() could not touch the audio pci power state from dGPU instance. This is a feedback got from our OEM developer, it is the overview of audio detect process. First, the kernel audio driver will be triggered to read ELD, if the ELD is valid, it will report a jack event (on or available) to sound core driver; the pulseaudio subscribe all jack events, if it is told that the hdmi jack is plugged in (on), the pulseaudio will set this port to available, then the pa-card or pa-sink has available port, it can be selected (manually, some daemons or policy in /usr/share/pulseaudio/alsa-mixer/) as default output card/default sink. If the description is correct. I think there are maybe two problems. 1. audio will auto power off after setup device link duo to usage_count=0. 2. duo to audio is power down, it could not get the HDMI jack insert event. How do you think? Jim, could you tell me which PCI devices are handled as vga_switcheroo audio client? The kernel should show all messages "xxx: Handle vga_switcheroo audio client". [ 4.311095] snd_hda_intel :06:00.1: enabling device ( -> 0002) [ 4.314286] snd_hda_intel :06:00.1: Handle vga_switcheroo audio client [ 4.314822] snd_hda_intel :06:00.6: enabling device ( -> 0002) 01:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. [AMD/ATI] Device [1002:699f] (rev c3) 06:00.0 VGA compatible controller [0300]: Advanced Micro Devices, Inc. [AMD/ATI] Device [1002:15dd] (rev d1) 06:00.1 Audio device [0403]: Advanced Micro Devices, Inc. [AMD/ATI] Device [1002:15de] OK this sheds a brighter light, finally. If my understanding is correct, the issue is a false vga_switcheroo audio detection, after all. This is the primary GPU and it shouldn't be registered as a vga_switcheroo discrete GPU. Below is a very ugly workaround for this particular case. It assumes that the AMD+AMD combo will never have audio outputs on both but only for the primary, and it's possibly wrong. Is there a handy way to identify whether the given VGA PCI entry is a discrete GPU or not? The amdgpu and radeon seem checking ATPX ACPI. This is no issue about this topic, in amdgpu driver, both iGPU/dGPU will register as VGA_SWITCHEROO_UNKNOWN_ID, and the client id will be re-initialized in vgaswitchreoo_enable() via ATPX call. Then, iGPU will set as VGA_SWITCHEROO_IGD, and dGPU will set as VGA_SWITCHEROO_DIS. Please check the patch. It's not about AMDGPU driver but the vga switcheroo detection in HD-audio driver. Not the GPU side but the audio side. The issues are two folds: - We register each AMD controller associated with a GPU always as a discrete GPU vga_switcheroo audio. - And when it's registered as a vga_switcheroo client, we forcibly enable runtime PM of the controller, since discrete GPU needs the runtime suspend. So a workaround in your case is just not to register as a vga switcheroo audio client. Then the runtime PM isn't enabled in the AMD HDMI audio controller, and the HDMI detection remains active. Takashi OK, I got your point. Let me have a try. BTW, what about the patch in this thread, I think it is also need for current code. although audio power is not controlled by vgaswitchreoo, we also can encounter the issue(audio which is bound to iGPU will suspend with dGPU) if user debugfs to control the client power. Thanks JimQu Thanks JimQu Takashi --- --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -1418,8 +1418,18 @@ static int azx_dev_free(struct snd_device *device) */ static struct pci_dev *get_bound_vga(struct pci_dev *pci) { + static const struct pci_device_id ids[] = { + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_ANY_ID), + .class = PCI_BASE_CLASS_DISPLAY << 16, + .class_mask = 0xff << 16 }, + {} + }; struct pci_dev *p;
Re: [alsa-devel] [PATCH 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function
在 2018/7/13 16:46, Takashi Iwai 写道: On Fri, 13 Jul 2018 10:06:02 +0200, Jim Qu wrote: On modern laptop, there are more and more platforms have two GPUs, and each of them maybe have audio codec for HDMP/DP output. For some dGPU which is no output, audio codec usually is disabled. In currect HDA audio driver, it will set all codec as VGA_SWITCHEROO_DIS, the audio which is binded to UMA will be suspended if user use debugfs to contorl power In HDA driver side, it is difficult to know which GPU the audio has binded to. So set the bound gpu pci dev to vgaswitchroo, the correct audio id will be set in vgaswitchreoo enable function. Signed-off-by: Jim Qu The idea looks good to me. Just a nitpick: @@ -1319,15 +1319,16 @@ static const struct vga_switcheroo_client_ops azx_vs_ops = { static int register_vga_switcheroo(struct azx *chip) { struct hda_intel *hda = container_of(chip, struct hda_intel, chip); + struct pci_dev * p; int err; if (!hda->use_vga_switcheroo) return 0; - /* FIXME: currently only handling DIS controller -* is there any machine with two switchable HDMI audio controllers? -*/ - err = vga_switcheroo_register_audio_client(chip->pci, &azx_vs_ops, - VGA_SWITCHEROO_DIS); + + p = get_bound_vga(chip->pci); + err = vga_switcheroo_register_audio_client(chip->pci, &azx_vs_ops, p); + if (p) + pci_dev_put(p); Passing a NULL vga_dev won't work, so the NULL check after calling vga_switcheroo_register_audio_client() is moot. If any, it should be checked before that. The hda->use_vga_switcheroo is set to 1 in init_vga_switcheroo() if bound vga pci dev is not NULL. Since the hda->use_vga_switcheroo is checked at beginning of the function. so the p which pass into vga_switcheroo_register_audio_client() is constant valid value. Thanks JimQu thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function
在 2018/7/13 17:27, Lukas Wunner 写道: On Fri, Jul 13, 2018 at 04:06:02PM +0800, Jim Qu wrote: On modern laptop, there are more and more platforms have two GPUs, and each of them maybe have audio codec for HDMP/DP output. For some dGPU which is no output, audio codec usually is disabled. In currect HDA audio driver, it will set all codec as VGA_SWITCHEROO_DIS, the audio which is binded to UMA will be suspended if user use debugfs to contorl power In HDA driver side, it is difficult to know which GPU the audio has binded to. So set the bound gpu pci dev to vgaswitchroo, the correct audio id will be set in vgaswitchreoo enable function. Signed-off-by: Jim Qu The approach you've taken in this patch won't work if the HDA controller is bound to a driver after both GPUs have already been bound to a driver and the handler has already been registered. That's because vga_switcheroo_enable() is run when two GPUs have registered as clients and the handler has also registered. If the HDA controller is probed afterwards, its ID will be stuck at VGA_SWITCHEROO_UNKNOWN_ID. Before resubmitting this patch, please run it through scripts/checkpatch.pl, there are multiple trivial issues here. Also, please use scripts/get_maintainers.pl on all files you're touching and cc the patch to the listed maintainers and reviewers. @@ -161,9 +163,8 @@ struct vgasr_priv { }; #define ID_BIT_AUDIO 0x100 -#define client_is_audio(c) ((c)->id & ID_BIT_AUDIO) -#define client_is_vga(c) ((c)->id == VGA_SWITCHEROO_UNKNOWN_ID || \ -!client_is_audio(c)) +#define client_is_audio(c) ((c)->id & ID_BIT_AUDIO) +#define client_is_vga(c) (!client_is_audio(c)) #define client_id(c) ((c)->id & ~ID_BIT_AUDIO) There's a spurious whitespace change in the client_is_audio() line, please get rid of it. Yeah, I will correct it in later update. --- a/include/linux/vga_switcheroo.h +++ b/include/linux/vga_switcheroo.h @@ -84,8 +84,8 @@ enum vga_switcheroo_state { * Client identifier. Audio clients use the same identifier & 0x100. */ enum vga_switcheroo_client_id { - VGA_SWITCHEROO_UNKNOWN_ID = -1, - VGA_SWITCHEROO_IGD, + VGA_SWITCHEROO_UNKNOWN_ID = 0x1000, + VGA_SWITCHEROO_IGD = 0, Hm, why is this change necessary? Before, we suppose VGA_SWITCHEROO_UNKNOWN_ID can be only used for gpu client, but now we also use it on audio client. Because of -1 in memory is presented as 0x, so the 0x & ID_BIT_AUDIO = ID_BIT_AUDIO, this will confuse people if it used on gpu client. that audio identify bit is set. Thanks JimQu Thanks, Lukas ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [alsa-devel] [PATCH 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function
在 2018/7/14 0:38, Takashi Iwai 写道: On Fri, 13 Jul 2018 17:25:39 +0200, jimqu wrote: 在 2018/7/13 16:46, Takashi Iwai 写道: On Fri, 13 Jul 2018 10:06:02 +0200, Jim Qu wrote: On modern laptop, there are more and more platforms have two GPUs, and each of them maybe have audio codec for HDMP/DP output. For some dGPU which is no output, audio codec usually is disabled. In currect HDA audio driver, it will set all codec as VGA_SWITCHEROO_DIS, the audio which is binded to UMA will be suspended if user use debugfs to contorl power In HDA driver side, it is difficult to know which GPU the audio has binded to. So set the bound gpu pci dev to vgaswitchroo, the correct audio id will be set in vgaswitchreoo enable function. Signed-off-by: Jim Qu The idea looks good to me. Just a nitpick: @@ -1319,15 +1319,16 @@ static const struct vga_switcheroo_client_ops azx_vs_ops = { static int register_vga_switcheroo(struct azx *chip) { struct hda_intel *hda = container_of(chip, struct hda_intel, chip); + struct pci_dev * p; int err; if (!hda->use_vga_switcheroo) return 0; - /* FIXME: currently only handling DIS controller -* is there any machine with two switchable HDMI audio controllers? -*/ - err = vga_switcheroo_register_audio_client(chip->pci, &azx_vs_ops, - VGA_SWITCHEROO_DIS); + + p = get_bound_vga(chip->pci); + err = vga_switcheroo_register_audio_client(chip->pci, &azx_vs_ops, p); + if (p) + pci_dev_put(p); Passing a NULL vga_dev won't work, so the NULL check after calling vga_switcheroo_register_audio_client() is moot. If any, it should be checked before that. The hda->use_vga_switcheroo is set to 1 in init_vga_switcheroo() if bound vga pci dev is not NULL. Since the hda->use_vga_switcheroo is checked at beginning of the function. so the p which pass into vga_switcheroo_register_audio_client() is constant valid value. So the NULL-check in the patch is also useless, no? :) Ha-ha, indeed, the NULL-check of p is unnecessary. will update in v2. Thanks JimQu Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function
在 2018/7/13 17:27, Lukas Wunner 写道: On Fri, Jul 13, 2018 at 04:06:02PM +0800, Jim Qu wrote: On modern laptop, there are more and more platforms have two GPUs, and each of them maybe have audio codec for HDMP/DP output. For some dGPU which is no output, audio codec usually is disabled. In currect HDA audio driver, it will set all codec as VGA_SWITCHEROO_DIS, the audio which is binded to UMA will be suspended if user use debugfs to contorl power In HDA driver side, it is difficult to know which GPU the audio has binded to. So set the bound gpu pci dev to vgaswitchroo, the correct audio id will be set in vgaswitchreoo enable function. Signed-off-by: Jim Qu The approach you've taken in this patch won't work if the HDA controller is bound to a driver after both GPUs have already been bound to a driver and the handler has already been registered. That's because vga_switcheroo_enable() is run when two GPUs have registered as clients and the handler has also registered. If the HDA controller is probed afterwards, its ID will be stuck at VGA_SWITCHEROO_UNKNOWN_ID. In genernic speaking, there are two cases , a. audio client is the third client. b. audio client is not the third client. if audio is third client. vga_switcheroo_ready() is true, vga_switcheroo_enable() can be called in audio client register fuction. if audio is not the third client, vga_switcheroo_enable() will be called in the second GFX client register. In vga_switcheroo_enable() , the first list loop will set two GPU clients' id, and the second list loop will select audio client, set id by its bound gpu pci dev. Before resubmitting this patch, please run it through scripts/checkpatch.pl, there are multiple trivial issues here. Also, please use scripts/get_maintainers.pl on all files you're touching and cc the patch to the listed maintainers and reviewers. OK, will do it in next v2. @@ -161,9 +163,8 @@ struct vgasr_priv { }; #define ID_BIT_AUDIO 0x100 -#define client_is_audio(c) ((c)->id & ID_BIT_AUDIO) -#define client_is_vga(c) ((c)->id == VGA_SWITCHEROO_UNKNOWN_ID || \ -!client_is_audio(c)) +#define client_is_audio(c) ((c)->id & ID_BIT_AUDIO) +#define client_is_vga(c) (!client_is_audio(c)) #define client_id(c) ((c)->id & ~ID_BIT_AUDIO) There's a spurious whitespace change in the client_is_audio() line, please get rid of it. Will update it in v2. --- a/include/linux/vga_switcheroo.h +++ b/include/linux/vga_switcheroo.h @@ -84,8 +84,8 @@ enum vga_switcheroo_state { * Client identifier. Audio clients use the same identifier & 0x100. */ enum vga_switcheroo_client_id { - VGA_SWITCHEROO_UNKNOWN_ID = -1, - VGA_SWITCHEROO_IGD, + VGA_SWITCHEROO_UNKNOWN_ID = 0x1000, + VGA_SWITCHEROO_IGD = 0, Hm, why is this change necessary? Before , we suppose VGA_SWITCHEROO_UNKNOWN_ID can be only used for gpu client, and now we also use it on the audio client. if we set VGA_SWITCHEROO_UNKNOWN_ID to gpu clients, client_is_audio(client) will be true, since -1 in memory is presented as 0x(if it is 64bit asic). thanks JimQu Thanks, Lukas ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: 答复: 答复: [alsa-devel] 答复: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id
在 2018/7/13 23:07, Takashi Iwai 写道: On Wed, 11 Jul 2018 13:12:01 +0200, Takashi Iwai wrote: And the forced runtime PM is still an issue, and this would need the other notification mechanism than the HD-audio unsolicited event as AMD HDMI controller doesn't honor the HD-audio WAKEEN bit. Since we had a nice "hack week" in this week at SUSE, I spent some time to write some patches for the support of the direct hotplug notification / ELD query between HD-audio and radeon/amdgpu. It re-utilizes the audio component framework for i915 but in a slightly more flexible way. The patches are found in topic/hda-acomp branch of my sound.git tree: git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git The following commits are relevant: drm/i915: Split audio component to a generic type ALSA: hda/i915: Allow delayed i915 audio component binding ALSA: hda/i915: Associate audio component with devres ALSA: hda: Make audio component support more generic ALSA: hda/hdmi: Allow audio component for AMD/ATI HDMI ALSA: hda/hdmi: Use single mutex unlock in error paths drm/radeon: Add audio component support drm/amdgpu: Add audio component support The branch should be cleanly pullable onto the latest 4.18-rc. I couldn't test amdgpu but the test with a radeon driver on an old laptop seemed working through a very quick test. Please give it a try. That is really wonderful work. I will check it on our AMD platform. BTW, For display, AMD has moved to use DC to support new asics. so there also need a patch for amdgpu in DC code. Thanks JimQu thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function
在 2018/7/16 22:02, Lukas Wunner 写道: On Sat, Jul 14, 2018 at 02:15:24PM +0800, jimqu wrote: ??? 2018/7/13 17:27, Lukas Wunner ??: On Fri, Jul 13, 2018 at 04:06:02PM +0800, Jim Qu wrote: On modern laptop, there are more and more platforms have two GPUs, and each of them maybe have audio codec for HDMP/DP output. For some dGPU which is no output, audio codec usually is disabled. In currect HDA audio driver, it will set all codec as VGA_SWITCHEROO_DIS, the audio which is binded to UMA will be suspended if user use debugfs to contorl power In HDA driver side, it is difficult to know which GPU the audio has binded to. So set the bound gpu pci dev to vgaswitchroo, the correct audio id will be set in vgaswitchreoo enable function. Signed-off-by: Jim Qu The approach you've taken in this patch won't work if the HDA controller is bound to a driver after both GPUs have already been bound to a driver and the handler has already been registered. That's because vga_switcheroo_enable() is run when two GPUs have registered as clients and the handler has also registered. If the HDA controller is probed afterwards, its ID will be stuck at VGA_SWITCHEROO_UNKNOWN_ID. In genernic speaking, there are two cases , a. audio client is the third client. b. audio client is not the third client. if audio is third client. vga_switcheroo_ready() is true, vga_switcheroo_enable() can be called in audio client register fuction. if audio is not the third client, vga_switcheroo_enable() will be called in the second GFX client register. In vga_switcheroo_enable() , the first list loop will set two GPU clients' id, and the second list loop will select audio client, set id by its bound gpu pci dev. No, if audio is the third client, vga_switcheroo_ready() is *not* true because vgasr_priv.active is true. This is set to true once the two GPUs and the handler have registered. If the audio device registers afterwards, vga_switcheroo_enable() will already have been called. It's never called twice because it sets vgasr_priv.active = true. As a result, the audio device is stuck at VGA_SWITCHEROO_UNKNOWN_ID. Indeed, I almost ignore the vgasr_priv.active, since I added amdgpu to blacklist, audio is always the first client. :). not so bad, Let me rework the patch tomorrow. Thanks JimQu Thanks, Lukas ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [alsa-devel] [V2 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function
在 2018/7/16 22:16, Lukas Wunner 写道: On Mon, Jul 16, 2018 at 03:52:45PM +0800, Hui Wang wrote: On 2018???07???16??? 14:06, Jim Qu wrote: On modern laptop, there are more and more platforms have two GPUs, and each of them maybe have audio codec for HDMP/DP output. For some dGPU which is no output, audio codec usually is disabled. In currect HDA audio driver, it will set all codec as VGA_SWITCHEROO_DIS, the audio which is binded to UMA will be suspended if user use debugfs to control power In HDA driver side, it is difficult to know which GPU the audio has bound to. So set the bound gpu pci dev to vgaswitchreoo, the correct audio id will be set in vgaswitchreoo enable function. Signed-off-by: Jim Qu Is it possible to send this patch to as well, then we will know that it is safe to backport this patch to the linux kernels with different version. For which hardware do you want it in stable kernels? This patch pertains to manual power control, which is only used on the MacBook Pro by default. And there are no MacBook Pros with AMD APUs. On all other hardware, users have to specify "amdgpu.runpm=0" on the command line to see any benefit from this patch. Also, manual power control will likely be removed once we get runtime PM working with muxed systems (such as the MacBook Pro). The patch idea will deal with the issues 1) manual power control after v4.17. 2) driver control before v4.17. As we know current a lot of mainstream OS, such like ubuntu 16.04.4 (kernel is v4.15), ubuntu 18.04(kernel is v4.15), RHEL7.x (kernel is 3.1x). BTW, I believe you will see more and more products with AMD next generation of APUs in the near future. Thanks JimQu Lukas ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [V3] vga_switcheroo: set audio client id according to bound GPU id
On 2018年07月17日 16:52, Takashi Iwai wrote: On Tue, 17 Jul 2018 10:38:58 +0200, Lukas Wunner wrote: On Tue, Jul 17, 2018 at 04:20:50PM +0800, Jim Qu wrote: On modern laptop, there are more and more platforms have two GPUs, and each of them maybe have audio codec for HDMP/DP output. For some dGPU which is no output, audio codec usually is disabled. In currect HDA audio driver, it will set all codec as VGA_SWITCHEROO_DIS, the audio which is binded to UMA will be suspended if user use debugfs to contorl power In HDA driver side, it is difficult to know which GPU the audio has binded to. So set the bound gpu pci dev to vga_switcheroo. if the audio client is not the third registration, audio id will set in vga_switcheroo enable function. if the audio client is the last registration when vga_switcheroo _ready() get true, we should get audio client id from bound GPU directly. Signed-off-by: Jim Qu Reviewed-by: Lukas Wunner @Takashi, any preference which tree to merge this through? sound or drm-misc, either way would seem fine to me. I think there's going to be one final drm-misc pull sent to Dave this week, after that it's 4.20. Since it's basically an audio problem and I'd love to merge it for 4.19, I'd prefer taking through sound git tree, unless anyone has objection. Thanks to Takashi and Lukas great help. Please kindly help merge the patch into suitable branch. Thanks JimQu thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: çå¤: [PATCH 3/4] drm/radeon: Add audio component support
On 2018å¹´07æ25æ¥ 13:28, Takashi Iwai wrote: On Wed, 25 Jul 2018 05:32:52 +0200, Qu, Jim wrote: @@ -269,6 +271,10 @@ static void radeon_audio_enable(struct radeon_device *rdev, if (rdev->audio.funcs->enable) rdev->audio.funcs->enable(rdev, pin, enable_mask); + + if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify) + acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, +pin->id, -1); Jim: radeon_audio_enable() can enable audios and also disable them, so eld noitfy callback should be called when enable_mask is true. It's intentional. The notifier needs to be called at disablement time, too. The audio driver has to follow the HDMI audio disablement, and notifier receives both on and off case. The actual state is inquired via get_eld call by HD-audio followed after the notification. thanks, Takashi OK, I got your point. Thanks JimQu ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: çå¤: [PATCH 4/4] drm/amdgpu: Add audio component support
On 2018å¹´07æ25æ¥ 13:46, Takashi Iwai wrote: On Wed, 25 Jul 2018 07:38:37 +0200, Qu, Jim wrote: Jim: Just like Alex said, we want driver can get eld info when hotplug in new device. amdgpu driver is a bit difference from radeon driver, it is not a suitable place to call notify() function in *_audio_enable() , since they are not in the hotplug process context like them in radeon driver, but the mode setting. IMO, the right place to call notify() function is also in the amdgpu_connector__detect() in amdgpu_connector.c. Hm, but by the modesetting, it actually enables / disables the audio as well, no? If so, the notifier is exactly for that purpose. The audio driver needs to know not only about the physical connection but whether the audio can be actually driven. That is, even if the monitor is connected, the audio won't come out if the mode is off. That is equivalent with the unplugged state for the audio driver. The i915 driver deals with the notifier just like the above, so the behavior is intentional. thanks, Takashi I am afraid if device hotplug out, how is audio state if it follow up eld info? Since the modesetting is never performed for the display which is plugged out, so there is no notify() call on it. Thanks JimQu With None-DC driver, the detect() of connectors will to be preformed in drm_helper_hpd_irq_event() when there is device hotplug. However, amdgpu_connector__detect() maybe need to be modified to fit your change. Thanks JimQu --- drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/amd/amdgpu/Makefile | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 + drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c | 97 +++ drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 3 + drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 6 ++ drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 6 ++ drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 6 ++ drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 ++ 9 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 2c7112ddfed4..fbe7216c5c56 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -193,6 +193,7 @@ config DRM_AMDGPU select BACKLIGHT_LCD_SUPPORT select INTERVAL_TREE select CHASH + select SND_HDA_COMPONENT if SND_HDA_CORE help Choose this option if you have a recent AMD Radeon graphics card. diff --git a/drivers/gpu/drm/amd/amdgpu/Makefile b/drivers/gpu/drm/amd/amdgpu/Makefile index bfd332c95b61..9c26facddb17 100644 --- a/drivers/gpu/drm/amd/amdgpu/Makefile +++ b/drivers/gpu/drm/amd/amdgpu/Makefile @@ -52,7 +52,7 @@ amdgpu-y += amdgpu_device.o amdgpu_kms.o \ amdgpu_ucode.o amdgpu_bo_list.o amdgpu_ctx.o amdgpu_sync.o \ amdgpu_gtt_mgr.o amdgpu_vram_mgr.o amdgpu_virt.o amdgpu_atomfirmware.o \ amdgpu_queue_mgr.o amdgpu_vf_error.o amdgpu_sched.o amdgpu_debugfs.o \ - amdgpu_ids.o + amdgpu_ids.o amdgpu_audio.o # add asic specific block amdgpu-$(CONFIG_DRM_AMDGPU_CIK)+= cik.o cik_ih.o kv_smc.o kv_dpm.o \ diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index a59c07590cee..203d2584c989 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -1957,5 +1957,9 @@ int amdgpu_dm_display_resume(struct amdgpu_device *adev ); static inline int amdgpu_dm_display_resume(struct amdgpu_device *adev) { return 0; } #endif +int amdgpu_audio_component_init(struct amdgpu_device *adev); +void amdgpu_audio_component_fini(struct amdgpu_device *adev); +void amdgpu_audio_eld_notify(struct amdgpu_device *adev, int pin); + #include "amdgpu_object.h" #endif diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c new file mode 100644 index ..39256e2f84b3 --- /dev/null +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_audio.c @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: MIT + +#include +#include "amdgpu.h" + +static int amdgpu_audio_component_get_eld(struct device *kdev, int port, + int pipe, bool *enabled, + unsigned char *buf, int max_bytes) +{ + struct drm_device *dev = dev_get_drvdata(kdev); + struct drm_encoder *encoder; + struct amdgpu_encoder *amdgpu_encoder; + struct amdgpu_encoder_atom_dig *dig; + struct drm_connector *connector; + int ret = 0; + + *enabled = 0; + list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { + amdgpu_encoder = to_amdgpu_encoder(encoder); + dig = amdgpu_encoder->enc_priv; + if (!dig |
Re: y4T�TCH 4/4] drm/amdgpu: Add audio component support
On 2018å¹´07æ25æ¥ 16:20, Takashi Iwai wrote: On Wed, 25 Jul 2018 10:02:34 +0200, jimqu wrote: On 2018å¹´07æ25æ¥ 13:46, Takashi Iwai wrote: On Wed, 25 Jul 2018 07:38:37 +0200, Qu, Jim wrote: Jim: Just like Alex said, we want driver can get eld info when hotplug in new device. amdgpu driver is a bit difference from radeon driver, it is not a suitable place to call notify() function in *_audio_enable() , since they are not in the hotplug process context like them in radeon driver, but the mode setting. IMO, the right place to call notify() function is also in the amdgpu_connector__detect() in amdgpu_connector.c. Hm, but by the modesetting, it actually enables / disables the audio as well, no? If so, the notifier is exactly for that purpose. The audio driver needs to know not only about the physical connection but whether the audio can be actually driven. That is, even if the monitor is connected, the audio won't come out if the mode is off. That is equivalent with the unplugged state for the audio driver. The i915 driver deals with the notifier just like the above, so the behavior is intentional. thanks, Takashi I am afraid if device hotplug out, how is audio state if it follow up eld info? Since the modesetting is never performed for the display which is plugged out, so there is no notify() call on it. In principle, the HDMI audio just needs to follows the video state, and it doesn't need to care actual physical connections. As long as video can go out, it's fine, audio can, too. When video is disabled (even if connected), audio can't be used as well, so it must follow to off, too. The notifier is used to follow this video state change. Practically seen, the user-space shall switch off the video accordingly upon hot unplug, then the audio notifier is sent, and the audio gets off, too. thanks, Takashi Ok, that means there are other code pathes to update audio state. Anyway, for patch 3/4 Acked-by: Jim Qu There may be other concerns from Alex/Christian. Thanks JimQu ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel