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

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

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.

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