Re: [PATCH] vgaswitchroo: set audio client id according to bound gpu client id

["Zhang, Jerry (Junwei)" <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>]

On 06/29/2018 12:01 PM, Alex Deucher wrote:
> On Thu, Jun 28, 2018 at 10:54 PM, Zhang, Jerry (Junwei)
> <Jerry.Zhang@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.

Yeah, I can get that.

While the atpx detection also confuse me then.
It seems must have one VGA and one OTHER, but OTHER(dGPU) could also be connected
to a external display(e.g. via HDMI).
Or should we say OTHER could has display but not default one?

Jerry

>
> Alex
>
>>
>>
>>>
>>> The patch also combine the HDA change to avoid break
>>> building.
>>>
>>> Change-Id: I9906c1bd4dd5b36108d7133bb1cf724d13f1cd6d
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>> Signed-off-by: Jim Qu <Jim.Qu@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@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx