答复: 答复: [alsa-devel] 答复: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id
diff mbox

Message ID s5h1scap2h2.wl-tiwai@suse.de
State New
Headers show

Commit Message

Takashi Iwai July 11, 2018, 9:04 a.m. UTC
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 0000:06:00.1: enabling device (0000 -> 0002)
> [    4.314286] snd_hda_intel 0000:06:00.1: Handle vga_switcheroo audio
> client
> [    4.314822] snd_hda_intel 0000:06:00.6: enabling device (0000 -> 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.


Takashi

---

Comments

jimqu July 11, 2018, 9:26 a.m. UTC | #1
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 0000:06:00.1: enabling device (0000 -> 0002)
>> [    4.314286] snd_hda_intel 0000:06:00.1: Handle vga_switcheroo audio
>> client
>> [    4.314822] snd_hda_intel 0000:06:00.6: enabling device (0000 -> 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:
Takashi Iwai July 11, 2018, 9:53 a.m. UTC | #2
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 0000:06:00.1: enabling device (0000 -> 0002)
> >> [    4.314286] snd_hda_intel 0000:06:00.1: Handle vga_switcheroo audio
> >> client
> >> [    4.314822] snd_hda_intel 0000:06:00.6: enabling device (0000 -> 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

> 
> 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:
>
jimqu July 11, 2018, 10:21 a.m. UTC | #3
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 0000:06:00.1: enabling device (0000 -> 0002)
>>>> [    4.314286] snd_hda_intel 0000:06:00.1: Handle vga_switcheroo audio
>>>> client
>>>> [    4.314822] snd_hda_intel 0000:06:00.6: enabling device (0000 -> 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;
>>>    +	/* 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:
Takashi Iwai July 11, 2018, 11:12 a.m. UTC | #4
On Wed, 11 Jul 2018 12:21:03 +0200,
jimqu wrote:
> 
> 
> 
> 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 0000:06:00.1: enabling device (0000 -> 0002)
> >>>> [    4.314286] snd_hda_intel 0000:06:00.1: Handle vga_switcheroo audio
> >>>> client
> >>>> [    4.314822] snd_hda_intel 0000:06:00.6: enabling device (0000 -> 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.

That'd do a trick in most cases, too, but I guess one scenario is
overseen: when HD-audio driver is probed / registered before the
graphics driver, it may get a false client id, too.

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.


Takashi
Takashi Iwai July 13, 2018, 3:07 p.m. UTC | #5
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.


thanks,

Takashi
jimqu July 14, 2018, 12:03 p.m. UTC | #6
在 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
Takashi Iwai July 14, 2018, 4:31 p.m. UTC | #7
On Sat, 14 Jul 2018 14:03:26 +0200,
jimqu wrote:
> 
> 
> 
> 在 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.

Thanks, it'll be great if you can check whether the current code works
or not.  I'd love to push the stuff for 4.19.  Hopefully I'll start
submitting the preparation patches in the next week.

Basically this also opens the door of the similar capability for
nouveau, and I guess it's also trivial enough.

> BTW, For display, AMD has moved to use DC to support new
> asics. so there also need a patch for amdgpu in DC code.

Could you give a more hint?  I'll try adapt the code if such a change
is already in upstream tree.


Takashi
Alex Deucher July 15, 2018, 2:36 p.m. UTC | #8
On Sat, Jul 14, 2018 at 12:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Sat, 14 Jul 2018 14:03:26 +0200,
> jimqu wrote:
>>
>>
>>
>> 在 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.
>
> Thanks, it'll be great if you can check whether the current code works
> or not.  I'd love to push the stuff for 4.19.  Hopefully I'll start
> submitting the preparation patches in the next week.
>
> Basically this also opens the door of the similar capability for
> nouveau, and I guess it's also trivial enough.
>
>> BTW, For display, AMD has moved to use DC to support new
>> asics. so there also need a patch for amdgpu in DC code.
>
> Could you give a more hint?  I'll try adapt the code if such a change
> is already in upstream tree.

The new code is in drivers/gpu/drm/amd/display.

Alex
Harry Wentland July 16, 2018, 3:10 p.m. UTC | #9
On 2018-07-15 10:36 AM, Alex Deucher wrote:
> On Sat, Jul 14, 2018 at 12:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> On Sat, 14 Jul 2018 14:03:26 +0200,
>> jimqu wrote:
>>>
>>>
>>>
>>> 在 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.
>>
>> Thanks, it'll be great if you can check whether the current code works
>> or not.  I'd love to push the stuff for 4.19.  Hopefully I'll start
>> submitting the preparation patches in the next week.
>>
>> Basically this also opens the door of the similar capability for
>> nouveau, and I guess it's also trivial enough.
>>
>>> BTW, For display, AMD has moved to use DC to support new
>>> asics. so there also need a patch for amdgpu in DC code.
>>
>> Could you give a more hint?  I'll try adapt the code if such a change
>> is already in upstream tree.
> 
> The new code is in drivers/gpu/drm/amd/display.
> 

In particular, I imagine all you need should be in display/amdgpu_dm, although there's chance you might have to touch display/dc/dce/dce_audio.c if you have to do anything with the unsolicited event.

I see you're using the GPL license, rather than MIT in amdgpu_audio.c. Since the code in display/dc is shared with other OSes and internal test frameworks it has to be MIT. Not entirely sure about display/amdgpu_dm, but if GPL is fine in amd/amdgpu it's probably fine there as well.

Harry

> Alex
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Takashi Iwai July 16, 2018, 3:25 p.m. UTC | #10
On Mon, 16 Jul 2018 17:10:43 +0200,
Harry Wentland wrote:
> 
> 
> 
> On 2018-07-15 10:36 AM, Alex Deucher wrote:
> > On Sat, Jul 14, 2018 at 12:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> On Sat, 14 Jul 2018 14:03:26 +0200,
> >> jimqu wrote:
> >>>
> >>>
> >>>
> >>> 在 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.
> >>
> >> Thanks, it'll be great if you can check whether the current code works
> >> or not.  I'd love to push the stuff for 4.19.  Hopefully I'll start
> >> submitting the preparation patches in the next week.
> >>
> >> Basically this also opens the door of the similar capability for
> >> nouveau, and I guess it's also trivial enough.
> >>
> >>> BTW, For display, AMD has moved to use DC to support new
> >>> asics. so there also need a patch for amdgpu in DC code.
> >>
> >> Could you give a more hint?  I'll try adapt the code if such a change
> >> is already in upstream tree.
> > 
> > The new code is in drivers/gpu/drm/amd/display.
> > 
> 
> In particular, I imagine all you need should be in display/amdgpu_dm, although there's chance you might have to touch display/dc/dce/dce_audio.c if you have to do anything with the unsolicited event.

Thanks, I'm currently struggling to read down the whole complex DC
code tree, and it's a very helpful hint.

How is the audio enable/disable call associated with the pipe index?
IOW, if I add a hook to call a notifier callback to pass the pipe
index that is enabled/disabled, how can it be deduced?

Similarly, when DC driver gets the query about the ELD on the given
pipe, where can I convert it to which object?

DC is quite another beast, so I still haven't figured out the
structures yet...


> I see you're using the GPL license, rather than MIT in amdgpu_audio.c. Since the code in display/dc is shared with other OSes and internal test frameworks it has to be MIT. Not entirely sure about display/amdgpu_dm, but if GPL is fine in amd/amdgpu it's probably fine there as well.

Oh I just didn't know that amdgpu code was with MIT.  Indeed the
driver module is provided via "GPL and additional rights".

I'm fine to change the license to follow other code bits there.  What
exactly SPDX tag should be put there?


thanks,

Takashi
Alex Deucher July 16, 2018, 4:56 p.m. UTC | #11
On Mon, Jul 16, 2018 at 11:25 AM, Takashi Iwai <tiwai@suse.de> wrote:
> On Mon, 16 Jul 2018 17:10:43 +0200,
> Harry Wentland wrote:
>>
>>
>>
>> On 2018-07-15 10:36 AM, Alex Deucher wrote:
>> > On Sat, Jul 14, 2018 at 12:31 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> >> On Sat, 14 Jul 2018 14:03:26 +0200,
>> >> jimqu wrote:
>> >>>
>> >>>
>> >>>
>> >>> 在 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.
>> >>
>> >> Thanks, it'll be great if you can check whether the current code works
>> >> or not.  I'd love to push the stuff for 4.19.  Hopefully I'll start
>> >> submitting the preparation patches in the next week.
>> >>
>> >> Basically this also opens the door of the similar capability for
>> >> nouveau, and I guess it's also trivial enough.
>> >>
>> >>> BTW, For display, AMD has moved to use DC to support new
>> >>> asics. so there also need a patch for amdgpu in DC code.
>> >>
>> >> Could you give a more hint?  I'll try adapt the code if such a change
>> >> is already in upstream tree.
>> >
>> > The new code is in drivers/gpu/drm/amd/display.
>> >
>>
>> In particular, I imagine all you need should be in display/amdgpu_dm, although there's chance you might have to touch display/dc/dce/dce_audio.c if you have to do anything with the unsolicited event.
>
> Thanks, I'm currently struggling to read down the whole complex DC
> code tree, and it's a very helpful hint.
>
> How is the audio enable/disable call associated with the pipe index?
> IOW, if I add a hook to call a notifier callback to pass the pipe
> index that is enabled/disabled, how can it be deduced?
>
> Similarly, when DC driver gets the query about the ELD on the given
> pipe, where can I convert it to which object?
>
> DC is quite another beast, so I still haven't figured out the
> structures yet...
>
>
>> I see you're using the GPL license, rather than MIT in amdgpu_audio.c. Since the code in display/dc is shared with other OSes and internal test frameworks it has to be MIT. Not entirely sure about display/amdgpu_dm, but if GPL is fine in amd/amdgpu it's probably fine there as well.
>
> Oh I just didn't know that amdgpu code was with MIT.  Indeed the
> driver module is provided via "GPL and additional rights".
>
> I'm fine to change the license to follow other code bits there.  What
> exactly SPDX tag should be put there?

Preferably:
SPDX-License-Identifier: MIT
or I suppose dual licensing (MIT or GPL 2.0) is ok too.

Alex

>
>
> thanks,
>
> Takashi

Patch
diff mbox

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