[V2 1/2] ALSA: HDA: use PCI_BASE_CLASS_DISPLAY to replace PCI_CLASS_DISPLAY_VGA

[V2 1/2] ALSA: HDA: use PCI_BASE_CLASS_DISPLAY to replace PCI_CLASS_DISPLAY_VGA

[Jim Qu]

Except PCI_CLASS_DISPLAY_VGA, some PCI class is sometimes
PCI_CLASS_DISPLAY_3D or PCI_CLASS_DISPLAY_OTHER.

Signed-off-by: Jim Qu <Jim.Qu@amd.com>
---
 sound/pci/hda/hda_intel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 1ae1850..14733ff 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1429,7 +1429,7 @@ 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 >> 16) == PCI_BASE_CLASS_DISPLAY)
 					return p;
 				pci_dev_put(p);
 			}
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[V2 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function

[Jim Qu]

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 <Jim.Qu@amd.com>
---
 drivers/gpu/vga/vga_switcheroo.c | 39 +++++++++++++++++++++++++++++----------
 include/linux/vga_switcheroo.h   |  8 ++++----
 sound/pci/hda/hda_intel.c        | 10 +++++-----
 3 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index fc4adf3..2b9ae42 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -103,6 +103,7 @@
  *	runtime pm. If true, writing ON and OFF to the vga_switcheroo debugfs
  *	interface is a no-op so as not to interfere with runtime pm
  * @list: client list
+ * @vga_dev: pci device, indicate which GPU is bound to current audio client
  *
  * Registered client. A client can be either a GPU or an audio device on a GPU.
  * For audio clients, the @fb_info and @active members are bogus.
@@ -116,6 +117,7 @@ struct vga_switcheroo_client {
 	bool active;
 	bool driver_power_control;
 	struct list_head list;
+	struct pci_dev *vga_dev;
 };
 
 /*
@@ -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)
 
 static int vga_switcheroo_debugfs_init(struct vgasr_priv *priv);
@@ -192,14 +193,28 @@ static void vga_switcheroo_enable(void)
 		vgasr_priv.handler->init();
 
 	list_for_each_entry(client, &vgasr_priv.clients, list) {
-		if (client->id != VGA_SWITCHEROO_UNKNOWN_ID)
+		if (!client_is_vga(client) || client_id(client) !=
+			VGA_SWITCHEROO_UNKNOWN_ID)
 			continue;
+
 		ret = vgasr_priv.handler->get_client_id(client->pdev);
 		if (ret < 0)
 			return;
-
 		client->id = ret;
 	}
+
+	list_for_each_entry(client, &vgasr_priv.clients, list) {
+		if (!client_is_audio(client) || client_id(client) !=
+			VGA_SWITCHEROO_UNKNOWN_ID)
+			continue;
+
+		ret = vgasr_priv.handler->get_client_id(client->vga_dev);
+		if (ret < 0)
+			return;
+
+		client->id = ret | ID_BIT_AUDIO;
+	}
+
 	vga_switcheroo_debugfs_init(&vgasr_priv);
 	vgasr_priv.active = true;
 }
@@ -272,7 +287,9 @@ EXPORT_SYMBOL(vga_switcheroo_handler_flags);
 
 static int register_client(struct pci_dev *pdev,
 			   const struct vga_switcheroo_client_ops *ops,
-			   enum vga_switcheroo_client_id id, bool active,
+			   enum vga_switcheroo_client_id id,
+			   struct pci_dev *vga_dev,
+			   bool active,
 			   bool driver_power_control)
 {
 	struct vga_switcheroo_client *client;
@@ -287,6 +304,7 @@ static int register_client(struct pci_dev *pdev,
 	client->id = id;
 	client->active = active;
 	client->driver_power_control = driver_power_control;
+	client->vga_dev = vga_dev;
 
 	mutex_lock(&vgasr_mutex);
 	list_add_tail(&client->list, &vgasr_priv.clients);
@@ -319,7 +337,7 @@ int vga_switcheroo_register_client(struct pci_dev *pdev,
 				   const struct vga_switcheroo_client_ops *ops,
 				   bool driver_power_control)
 {
-	return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID,
+	return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID, NULL,
 			       pdev == vga_default_device(),
 			       driver_power_control);
 }
@@ -329,7 +347,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
+ * @vga_dev:  pci device which is bound to current audio client
  *
  * Register audio client (audio device on a GPU). The client is assumed
  * to use runtime PM. Beforehand, vga_switcheroo_client_probe_defer()
@@ -339,9 +357,10 @@ 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 *vga_dev)
 {
-	return register_client(pdev, ops, id | ID_BIT_AUDIO, false, true);
+	return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID |
+			ID_BIT_AUDIO, vga_dev, 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..a34539b 100644
--- 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,
 	VGA_SWITCHEROO_DIS,
 	VGA_SWITCHEROO_MAX_CLIENTS,
 };
@@ -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 *vga_dev);
 
 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 *vga_dev) { 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 14733ff..d63f656 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1319,15 +1319,15 @@ 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);
+	pci_dev_put(p);
 	if (err < 0)
 		return err;
 	hda->vga_switcheroo_registered = 1;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [V2 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function

[Hui Wang]

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 <Jim.Qu@amd.com>
Is it possible to send this patch to <stable@vger.kernel.org> as well, 
then we will know that it is safe to backport this patch to the linux 
kernels with different version.

Thanks,
Hui.
> ---
>   drivers/gpu/vga/vga_switcheroo.c | 39 +++++++++++++++++++++++++++++----------
>   include/linux/vga_switcheroo.h   |  8 ++++----
>   sound/pci/hda/hda_intel.c        | 10 +++++-----
>   3 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index fc4adf3..2b9ae42 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -103,6 +103,7 @@
>    *	runtime pm. If true, writing ON and OFF to the vga_switcheroo debugfs
>    *	interface is a no-op so as not to interfere with runtime pm
>    * @list: client list
> + * @vga_dev: pci device, indicate which GPU is bound to current audio client
>    *
>    * Registered client. A client can be either a GPU or an audio device on a GPU.
>    * For audio clients, the @fb_info and @active members are bogus.
> @@ -116,6 +117,7 @@ struct vga_switcheroo_client {
>   	bool active;
>   	bool driver_power_control;
>   	struct list_head list;
> +	struct pci_dev *vga_dev;
>   };
>   
>   /*
> @@ -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)
>   
>   static int vga_switcheroo_debugfs_init(struct vgasr_priv *priv);
> @@ -192,14 +193,28 @@ static void vga_switcheroo_enable(void)
>   		vgasr_priv.handler->init();
>   
>   	list_for_each_entry(client, &vgasr_priv.clients, list) {
> -		if (client->id != VGA_SWITCHEROO_UNKNOWN_ID)
> +		if (!client_is_vga(client) || client_id(client) !=
> +			VGA_SWITCHEROO_UNKNOWN_ID)
>   			continue;
> +
>   		ret = vgasr_priv.handler->get_client_id(client->pdev);
>   		if (ret < 0)
>   			return;
> -
>   		client->id = ret;
>   	}
> +
> +	list_for_each_entry(client, &vgasr_priv.clients, list) {
> +		if (!client_is_audio(client) || client_id(client) !=
> +			VGA_SWITCHEROO_UNKNOWN_ID)
> +			continue;
> +
> +		ret = vgasr_priv.handler->get_client_id(client->vga_dev);
> +		if (ret < 0)
> +			return;
> +
> +		client->id = ret | ID_BIT_AUDIO;
> +	}
> +
>   	vga_switcheroo_debugfs_init(&vgasr_priv);
>   	vgasr_priv.active = true;
>   }
> @@ -272,7 +287,9 @@ EXPORT_SYMBOL(vga_switcheroo_handler_flags);
>   
>   static int register_client(struct pci_dev *pdev,
>   			   const struct vga_switcheroo_client_ops *ops,
> -			   enum vga_switcheroo_client_id id, bool active,
> +			   enum vga_switcheroo_client_id id,
> +			   struct pci_dev *vga_dev,
> +			   bool active,
>   			   bool driver_power_control)
>   {
>   	struct vga_switcheroo_client *client;
> @@ -287,6 +304,7 @@ static int register_client(struct pci_dev *pdev,
>   	client->id = id;
>   	client->active = active;
>   	client->driver_power_control = driver_power_control;
> +	client->vga_dev = vga_dev;
>   
>   	mutex_lock(&vgasr_mutex);
>   	list_add_tail(&client->list, &vgasr_priv.clients);
> @@ -319,7 +337,7 @@ int vga_switcheroo_register_client(struct pci_dev *pdev,
>   				   const struct vga_switcheroo_client_ops *ops,
>   				   bool driver_power_control)
>   {
> -	return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID,
> +	return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID, NULL,
>   			       pdev == vga_default_device(),
>   			       driver_power_control);
>   }
> @@ -329,7 +347,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
> + * @vga_dev:  pci device which is bound to current audio client
>    *
>    * Register audio client (audio device on a GPU). The client is assumed
>    * to use runtime PM. Beforehand, vga_switcheroo_client_probe_defer()
> @@ -339,9 +357,10 @@ 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 *vga_dev)
>   {
> -	return register_client(pdev, ops, id | ID_BIT_AUDIO, false, true);
> +	return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID |
> +			ID_BIT_AUDIO, vga_dev, 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..a34539b 100644
> --- 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,
>   	VGA_SWITCHEROO_DIS,
>   	VGA_SWITCHEROO_MAX_CLIENTS,
>   };
> @@ -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 *vga_dev);
>   
>   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 *vga_dev) { 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 14733ff..d63f656 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1319,15 +1319,15 @@ 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);
> +	pci_dev_put(p);
>   	if (err < 0)
>   		return err;
>   	hda->vga_switcheroo_registered = 1;


_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [V2 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function

[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 <Jim.Qu@amd.com>
> 
> Is it possible to send this patch to <stable@vger.kernel.org> 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).

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

[jimqu]

[-- Attachment #1.1: Type: text/plain, Size: 1823 bytes --]



在 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 <Jim.Qu@amd.com>
>> Is it possible to send this patch to <stable@vger.kernel.org> 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


[-- Attachment #1.2: Type: text/html, Size: 10487 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [V2 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function

[Takashi Iwai]

On Mon, 16 Jul 2018 08:06:35 +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 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 <Jim.Qu@amd.com>

Looks good to me.  Lukas, let me know if it's OK for you.

I guess this can go through sound.git tree together with another
change.  In that case, please give ACK from DRM guys.

Or anyone wants this through another tree inevitably, let me know.  In
that case, fell free to take my reviewed-by tag:
  Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi


> ---
>  drivers/gpu/vga/vga_switcheroo.c | 39 +++++++++++++++++++++++++++++----------
>  include/linux/vga_switcheroo.h   |  8 ++++----
>  sound/pci/hda/hda_intel.c        | 10 +++++-----
>  3 files changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index fc4adf3..2b9ae42 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -103,6 +103,7 @@
>   *	runtime pm. If true, writing ON and OFF to the vga_switcheroo debugfs
>   *	interface is a no-op so as not to interfere with runtime pm
>   * @list: client list
> + * @vga_dev: pci device, indicate which GPU is bound to current audio client
>   *
>   * Registered client. A client can be either a GPU or an audio device on a GPU.
>   * For audio clients, the @fb_info and @active members are bogus.
> @@ -116,6 +117,7 @@ struct vga_switcheroo_client {
>  	bool active;
>  	bool driver_power_control;
>  	struct list_head list;
> +	struct pci_dev *vga_dev;
>  };
>  
>  /*
> @@ -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)
>  
>  static int vga_switcheroo_debugfs_init(struct vgasr_priv *priv);
> @@ -192,14 +193,28 @@ static void vga_switcheroo_enable(void)
>  		vgasr_priv.handler->init();
>  
>  	list_for_each_entry(client, &vgasr_priv.clients, list) {
> -		if (client->id != VGA_SWITCHEROO_UNKNOWN_ID)
> +		if (!client_is_vga(client) || client_id(client) !=
> +			VGA_SWITCHEROO_UNKNOWN_ID)
>  			continue;
> +
>  		ret = vgasr_priv.handler->get_client_id(client->pdev);
>  		if (ret < 0)
>  			return;
> -
>  		client->id = ret;
>  	}
> +
> +	list_for_each_entry(client, &vgasr_priv.clients, list) {
> +		if (!client_is_audio(client) || client_id(client) !=
> +			VGA_SWITCHEROO_UNKNOWN_ID)
> +			continue;
> +
> +		ret = vgasr_priv.handler->get_client_id(client->vga_dev);
> +		if (ret < 0)
> +			return;
> +
> +		client->id = ret | ID_BIT_AUDIO;
> +	}
> +
>  	vga_switcheroo_debugfs_init(&vgasr_priv);
>  	vgasr_priv.active = true;
>  }
> @@ -272,7 +287,9 @@ EXPORT_SYMBOL(vga_switcheroo_handler_flags);
>  
>  static int register_client(struct pci_dev *pdev,
>  			   const struct vga_switcheroo_client_ops *ops,
> -			   enum vga_switcheroo_client_id id, bool active,
> +			   enum vga_switcheroo_client_id id,
> +			   struct pci_dev *vga_dev,
> +			   bool active,
>  			   bool driver_power_control)
>  {
>  	struct vga_switcheroo_client *client;
> @@ -287,6 +304,7 @@ static int register_client(struct pci_dev *pdev,
>  	client->id = id;
>  	client->active = active;
>  	client->driver_power_control = driver_power_control;
> +	client->vga_dev = vga_dev;
>  
>  	mutex_lock(&vgasr_mutex);
>  	list_add_tail(&client->list, &vgasr_priv.clients);
> @@ -319,7 +337,7 @@ int vga_switcheroo_register_client(struct pci_dev *pdev,
>  				   const struct vga_switcheroo_client_ops *ops,
>  				   bool driver_power_control)
>  {
> -	return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID,
> +	return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID, NULL,
>  			       pdev == vga_default_device(),
>  			       driver_power_control);
>  }
> @@ -329,7 +347,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
> + * @vga_dev:  pci device which is bound to current audio client
>   *
>   * Register audio client (audio device on a GPU). The client is assumed
>   * to use runtime PM. Beforehand, vga_switcheroo_client_probe_defer()
> @@ -339,9 +357,10 @@ 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 *vga_dev)
>  {
> -	return register_client(pdev, ops, id | ID_BIT_AUDIO, false, true);
> +	return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID |
> +			ID_BIT_AUDIO, vga_dev, 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..a34539b 100644
> --- 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,
>  	VGA_SWITCHEROO_DIS,
>  	VGA_SWITCHEROO_MAX_CLIENTS,
>  };
> @@ -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 *vga_dev);
>  
>  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 *vga_dev) { 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 14733ff..d63f656 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1319,15 +1319,15 @@ 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);
> +	pci_dev_put(p);
>  	if (err < 0)
>  		return err;
>  	hda->vga_switcheroo_registered = 1;
> -- 
> 2.7.4
> 
> 

Re: [V2 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function

[Lukas Wunner]

On Mon, Jul 16, 2018 at 02:06:35PM +0800, Jim Qu wrote:
> +
> +	list_for_each_entry(client, &vgasr_priv.clients, list) {
> +		if (!client_is_audio(client) || client_id(client) !=
> +			VGA_SWITCHEROO_UNKNOWN_ID)

Don't you have to check for

      client_id(client) != VGA_SWITCHEROO_UNKNOWN_ID | ID_BIT_AUDIO

here?  That's the value you assign when the audio client registers,
yet you're checking for something else, so you're always skipping
over audio clients.  Did you actually test this patch?


> 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 <Jim.Qu@amd.com>

Apart from the issue above and that this approach doesn't work if
hda_intel registers after the two GPUs, some nits:

vgaswitchreoo => vga_switcheroo multiple times above and in the subject.


> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -103,6 +103,7 @@
>   *	runtime pm. If true, writing ON and OFF to the vga_switcheroo debugfs
>   *	interface is a no-op so as not to interfere with runtime pm
>   * @list: client list
> + * @vga_dev: pci device, indicate which GPU is bound to current audio client
>   *
>   * Registered client. A client can be either a GPU or an audio device on a GPU.
>   * For audio clients, the @fb_info and @active members are bogus.

Please add a note here that vga_dev is bogus for GPU clients.


> @@ -192,14 +193,28 @@ static void vga_switcheroo_enable(void)
>  		vgasr_priv.handler->init();
>  
>  	list_for_each_entry(client, &vgasr_priv.clients, list) {
> -		if (client->id != VGA_SWITCHEROO_UNKNOWN_ID)
> +		if (!client_is_vga(client) || client_id(client) !=
> +			VGA_SWITCHEROO_UNKNOWN_ID)

Readability might improve if you break the line like this:

		if (!client_is_vga(client) ||
		    client_id(client) != VGA_SWITCHEROO_UNKNOWN_ID)


> @@ -339,9 +357,10 @@ 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 *vga_dev)
>  {
> -	return register_client(pdev, ops, id | ID_BIT_AUDIO, false, true);
> +	return register_client(pdev, ops, VGA_SWITCHEROO_UNKNOWN_ID |
> +			ID_BIT_AUDIO, vga_dev, false, true);

Again, I'd break the line such that the two operands to the bitwise or
are on the same line:

	return register_client(pdev, ops,
			       VGA_SWITCHEROO_UNKNOWN_ID | ID_BIT_AUDIO,
			       vga_dev, false, true);

Lukas

Re: [alsa-devel] [V2 2/2] vgaswitchreoo: set audio client id in vgaswitchreoo enable function

[Takashi Iwai]

On Mon, 16 Jul 2018 16:43:42 +0200,
Lukas Wunner wrote:
> 
> On Mon, Jul 16, 2018 at 02:06:35PM +0800, Jim Qu wrote:
> > +
> > +	list_for_each_entry(client, &vgasr_priv.clients, list) {
> > +		if (!client_is_audio(client) || client_id(client) !=
> > +			VGA_SWITCHEROO_UNKNOWN_ID)
> 
> Don't you have to check for
> 
>       client_id(client) != VGA_SWITCHEROO_UNKNOWN_ID | ID_BIT_AUDIO
> 
> here?  That's the value you assign when the audio client registers,
> yet you're checking for something else, so you're always skipping
> over audio clients.  Did you actually test this patch?

This should be OK, since client_id() is a macro to strip to the core
id number without ID_BIT_AUDIO.  And client_is_vga() &
client_is_audio() macros were rewritten along with it, too.


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

[Lukas Wunner]

On Mon, Jul 16, 2018 at 04:47:11PM +0200, Takashi Iwai wrote:
> On Mon, 16 Jul 2018 16:43:42 +0200,
> Lukas Wunner wrote:
> > 
> > On Mon, Jul 16, 2018 at 02:06:35PM +0800, Jim Qu wrote:
> > > +
> > > +	list_for_each_entry(client, &vgasr_priv.clients, list) {
> > > +		if (!client_is_audio(client) || client_id(client) !=
> > > +			VGA_SWITCHEROO_UNKNOWN_ID)
> > 
> > Don't you have to check for
> > 
> >       client_id(client) != VGA_SWITCHEROO_UNKNOWN_ID | ID_BIT_AUDIO
> > 
> > here?  That's the value you assign when the audio client registers,
> > yet you're checking for something else, so you're always skipping
> > over audio clients.  Did you actually test this patch?
> 
> This should be OK, since client_id() is a macro to strip to the core
> id number without ID_BIT_AUDIO.  And client_is_vga() &
> client_is_audio() macros were rewritten along with it, too.

Okay, thanks for the clarification.

Lukas
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [V2 1/2] ALSA: HDA: use PCI_BASE_CLASS_DISPLAY to replace PCI_CLASS_DISPLAY_VGA

[Takashi Iwai]

On Mon, 16 Jul 2018 08:06:34 +0200,
Jim Qu wrote:
> 
> Except PCI_CLASS_DISPLAY_VGA, some PCI class is sometimes
> PCI_CLASS_DISPLAY_3D or PCI_CLASS_DISPLAY_OTHER.
> 
> Signed-off-by: Jim Qu <Jim.Qu@amd.com>

Applied this one now, as it's basically an individual fix.


thanks,

Takashi
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx