On 06/29/2018 04:06 PM, Qu, Jim wrote:
> I mean if the VGA_OTHER is only for headless, and there is no audio on the headless GFX. therefore, the PCI_CLASS_DISPLAY_OTHER is unnecessary in get_bound_gpu() on this case.
>
> The Key pointer is the problem: does it exist the GFX device is defined as PCI_CLASS_DISPLAY_OTHER with audio codec.
In my view, headless is likely to have no audio,
while dGPU with display could be registered as OTHER, in this case, audio is controlled by HDA driver.
To my understanding, this issue is about the dGPU has audio codec registered as OTHER, right?
Jerry
>
> Thanks
> JimQu
>
> ________________________________________
> å??件人: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> 代表 Zhang, Jerry (Junwei) <Jerry.Zhang at amd.com>
> å??é??æ?¶é?´: 2018å¹´6æ??29æ?¥ 15:40:14
> �件人: Qu, Jim; Alex Deucher
> æ??é??: Deucher, Alexander; amd-gfx list
> 主é¢?: Re: ç?å¤?: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id
>
> On 06/29/2018 01:59 PM, Qu, Jim wrote:
>> If the GFX is headless, the audio codec should be disabled on PCIE bus. the the HDA driver will never probe the audio. right?
>
> After checking the HDA a little, looks HDA could also handle many other audio devices.
> In this case, HDA may probe other audio device, e.g. from Braswell.
>
> Just a guess.
>
> Jerry
>
>>
>> Thanks
>> JimQu
>>
>> ________________________________________
>> å??件人: Alex Deucher <alexdeucher at gmail.com>
>> å??é??æ?¶é?´: 2018å¹´6æ??29æ?¥ 12:01
>> �件人: Zhang, Jerry
>> æ??é??: Qu, Jim; amd-gfx list; Deucher, Alexander
>> 主�: Re: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id
>>
>> On Thu, Jun 28, 2018 at 10:54 PM, Zhang, Jerry (Junwei)
>> <Jerry.Zhang at amd.com> wrote:
>>> On 06/28/2018 02:22 PM, 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, so if system runtime pm is enabled,
>>>> the audio which is binded to UMA will be suspended.
>>>>
>>>> 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, let vgaswitchroo make decision.
>>>>
>>>> Newer dGPUs use PCI_CLASS_DISPLAY_OTHER rather than
>>>> PCI_CLASS_DISPLAY_VGA. Make sure we check for both when
>>>> checking for the dGPU in get_bound_vga()
>>>
>>>
>>> IIRC, VGA is for iGPU, OTHER is for dGPU.
>>> if not, please correct me.
>>
>> I think the dGPU can be either VGA or OTHER depending on whether there
>> are displays wired to it. E.g., iceland and hainan don't even have
>> VGA hw on them since they are headless.
>>
>> Alex
>>
>>>
>>>
>>>>
>>>> The patch also combine the HDA change to avoid break
>>>> building.
>>>>
>>>> Change-Id: I9906c1bd4dd5b36108d7133bb1cf724d13f1cd6d
>>>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>>>> Signed-off-by: Jim Qu <Jim.Qu at amd.com>
>>>> ---
>>>> drivers/gpu/vga/vga_switcheroo.c | 11 +++++++++--
>>>> include/linux/vga_switcheroo.h | 4 ++--
>>>> sound/pci/hda/hda_intel.c | 15 +++++++++------
>>>> 3 files changed, 20 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/vga/vga_switcheroo.c
>>>> b/drivers/gpu/vga/vga_switcheroo.c
>>>> index fc4adf3..7b7b0fd 100644
>>>> --- a/drivers/gpu/vga/vga_switcheroo.c
>>>> +++ b/drivers/gpu/vga/vga_switcheroo.c
>>>> @@ -329,7 +329,7 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
>>>> * vga_switcheroo_register_audio_client - register audio client
>>>> * @pdev: client pci device
>>>> * @ops: client callbacks
>>>> - * @id: client identifier
>>>> + * @bound_vga: client bound vga pci device
>>>> *
>>>> * Register audio client (audio device on a GPU). The client is assumed
>>>> * to use runtime PM. Beforehand, vga_switcheroo_client_probe_defer()
>>>> @@ -339,8 +339,15 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
>>>> */
>>>> int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>>>> const struct vga_switcheroo_client_ops *ops,
>>>> - enum vga_switcheroo_client_id id)
>>>> + struct pci_dev *bound_vga)
>>>> {
>>>> + enum vga_switcheroo_client_id id = VGA_SWITCHEROO_DIS;
>>>> +
>>>> + if (bound_vga) {
>>>> + if (vgasr_priv.handler->get_client_id)
>>>
>>>
>>> We may combine 2 "if" together.
>>>
>>> Anyway, the patch looks fine for me.
>>>
>>> Reviewed-by: Junwei Zhang <Jerry.Zhang at amd.com>
>>>
>>> Additionally, better to includes Intel guys mail list and sound mail list if
>>> any.
>>>
>>> Jerry
>>>
>>>
>>>> + id = vgasr_priv.handler->get_client_id(bound_vga);
>>>> + }
>>>> +
>>>> return register_client(pdev, ops, id | ID_BIT_AUDIO, false, true);
>>>> }
>>>> EXPORT_SYMBOL(vga_switcheroo_register_audio_client);
>>>> diff --git a/include/linux/vga_switcheroo.h
>>>> b/include/linux/vga_switcheroo.h
>>>> index 77f0f0a..a0c7b86 100644
>>>> --- a/include/linux/vga_switcheroo.h
>>>> +++ b/include/linux/vga_switcheroo.h
>>>> @@ -151,7 +151,7 @@ int vga_switcheroo_register_client(struct pci_dev
>>>> *dev,
>>>> bool driver_power_control);
>>>> int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>>>> const struct
>>>> vga_switcheroo_client_ops *ops,
>>>> - enum vga_switcheroo_client_id
>>>> id);
>>>> + struct pci_dev *bound_vga);
>>>>
>>>> void vga_switcheroo_client_fb_set(struct pci_dev *dev,
>>>> struct fb_info *info);
>>>> @@ -180,7 +180,7 @@ static inline int
>>>> vga_switcheroo_register_handler(const struct vga_switcheroo_ha
>>>> enum vga_switcheroo_handler_flags_t handler_flags) {
>>>> return 0; }
>>>> static inline int vga_switcheroo_register_audio_client(struct pci_dev
>>>> *pdev,
>>>> const struct vga_switcheroo_client_ops *ops,
>>>> - enum vga_switcheroo_client_id id) { return 0; }
>>>> + struct pci_dev *bound_vga) { return 0; }
>>>> static inline void vga_switcheroo_unregister_handler(void) {}
>>>> static inline enum vga_switcheroo_handler_flags_t
>>>> vga_switcheroo_handler_flags(void) { return 0; }
>>>> static inline int vga_switcheroo_lock_ddc(struct pci_dev *pdev) { return
>>>> -ENODEV; }
>>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>>>> index 1ae1850..8f992e6 100644
>>>> --- a/sound/pci/hda/hda_intel.c
>>>> +++ b/sound/pci/hda/hda_intel.c
>>>> @@ -1319,15 +1319,17 @@ 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);
>>>> if (err < 0)
>>>> return err;
>>>> hda->vga_switcheroo_registered = 1;
>>>> @@ -1429,7 +1431,8 @@ static struct pci_dev *get_bound_vga(struct pci_dev
>>>> *pci)
>>>> p =
>>>> pci_get_domain_bus_and_slot(pci_domain_nr(pci->bus),
>>>> pci->bus->number,
>>>> 0);
>>>> if (p) {
>>>> - if ((p->class >> 8) ==
>>>> PCI_CLASS_DISPLAY_VGA)
>>>> + if ((p->class >> 8) ==
>>>> PCI_CLASS_DISPLAY_VGA ||
>>>> + (p->class >> 8) ==
>>>> PCI_CLASS_DISPLAY_OTHER)
>>>> return p;
>>>> pci_dev_put(p);
>>>> }
>>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
