[PATCH 1/2] ALSA: HDA: use PCI_BASE_CLASS_DISPLAY to cover more display adapters

[PATCH 1/2] ALSA: HDA: use PCI_BASE_CLASS_DISPLAY to cover more display adapters

[Jim Qu]

Signed-off-by: Jim Qu <Jim.Qu@amd.com>
---
 sound/pci/hda/hda_intel.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 1ae1850..e0064567 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -31,7 +31,7 @@
  *  CHANGES:
  *
  *  2004.12.01	Major rewrite by tiwai, merged the work of pshou
- * 
+ *
  */
 
 #include <linux/delay.h>
@@ -390,8 +390,8 @@ static char *driver_short_names[] = {
 	[AZX_DRIVER_SIS] = "HDA SIS966",
 	[AZX_DRIVER_ULI] = "HDA ULI M5461",
 	[AZX_DRIVER_NVIDIA] = "HDA NVidia",
-	[AZX_DRIVER_TERA] = "HDA Teradici", 
-	[AZX_DRIVER_CTX] = "HDA Creative", 
+	[AZX_DRIVER_TERA] = "HDA Teradici",
+	[AZX_DRIVER_CTX] = "HDA Creative",
 	[AZX_DRIVER_CTHDA] = "HDA Creative",
 	[AZX_DRIVER_CMEDIA] = "HDA C-Media",
 	[AZX_DRIVER_GENERIC] = "HD-Audio Generic",
@@ -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

[PATCH 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 contorl power

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, 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 | 37 +++++++++++++++++++++++++++----------
 include/linux/vga_switcheroo.h   |  8 ++++----
 sound/pci/hda/hda_intel.c        | 11 ++++++-----
 3 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index fc4adf3..a6fd4f5 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,8 @@ 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 +303,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 +336,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 +346,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 +356,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 e0064567..1d3fc5a 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1319,15 +1319,16 @@ 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;
-- 
2.7.4

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

Re: [PATCH 1/2] ALSA: HDA: use PCI_BASE_CLASS_DISPLAY to cover more display adapters

[Lukas Wunner]

On Fri, Jul 13, 2018 at 04:06:01PM +0800, Jim Qu wrote:
> Signed-off-by: Jim Qu <Jim.Qu@amd.com>

Empty commit message.  Please add an explanation why the change is
necessary so folks browsing the git history understand it's motivation.
I think here the motivation is that the PCI class is sometimes
PCI_CLASS_DISPLAY_3D or PCI_CLASS_DISPLAY_OTHER.


> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -31,7 +31,7 @@
>   *  CHANGES:
>   *
>   *  2004.12.01	Major rewrite by tiwai, merged the work of pshou
> - * 
> + *

Spurious unrelated change, please get rid of it, this clutters up
the git history unnecessarily.


> @@ -390,8 +390,8 @@ static char *driver_short_names[] = {
>  	[AZX_DRIVER_SIS] = "HDA SIS966",
>  	[AZX_DRIVER_ULI] = "HDA ULI M5461",
>  	[AZX_DRIVER_NVIDIA] = "HDA NVidia",
> -	[AZX_DRIVER_TERA] = "HDA Teradici", 
> -	[AZX_DRIVER_CTX] = "HDA Creative", 
> +	[AZX_DRIVER_TERA] = "HDA Teradici",
> +	[AZX_DRIVER_CTX] = "HDA Creative",

Ditto.

Thanks,

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

Re: [alsa-devel] [PATCH 1/2] ALSA: HDA: use PCI_BASE_CLASS_DISPLAY to cover more display adapters

[Takashi Iwai]

On Fri, 13 Jul 2018 10:06:01 +0200,
Jim Qu wrote:
> 
> Signed-off-by: Jim Qu <Jim.Qu@amd.com>

More explanations please.  e.g. why is this needed?

The code change itself looks good, but ...

> @@ -31,7 +31,7 @@
>   *  CHANGES:
>   *
>   *  2004.12.01	Major rewrite by tiwai, merged the work of pshou
> - * 
> + *
>   */
>  
>  #include <linux/delay.h>
> @@ -390,8 +390,8 @@ static char *driver_short_names[] = {
>  	[AZX_DRIVER_SIS] = "HDA SIS966",
>  	[AZX_DRIVER_ULI] = "HDA ULI M5461",
>  	[AZX_DRIVER_NVIDIA] = "HDA NVidia",
> -	[AZX_DRIVER_TERA] = "HDA Teradici", 
> -	[AZX_DRIVER_CTX] = "HDA Creative", 
> +	[AZX_DRIVER_TERA] = "HDA Teradici",
> +	[AZX_DRIVER_CTX] = "HDA Creative",
>  	[AZX_DRIVER_CTHDA] = "HDA Creative",
>  	[AZX_DRIVER_CMEDIA] = "HDA C-Media",
>  	[AZX_DRIVER_GENERIC] = "HD-Audio Generic",

These two hunks are utterly irrelevant with the fix.  Please drop
them.


thanks,

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

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

[Takashi Iwai]

On Fri, 13 Jul 2018 10:06:02 +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 contorl power
> 
> 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, the correct audio id will be set in
> vgaswitchreoo enable function.
> 
> Signed-off-by: Jim Qu <Jim.Qu@amd.com>

The idea looks good to me.  Just a nitpick:

> @@ -1319,15 +1319,16 @@ 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);

Passing a NULL vga_dev won't work, so the NULL check after calling
vga_switcheroo_register_audio_client() is moot.  If any, it should be
checked before that.


thanks,

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

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

[Lukas Wunner]

On Fri, Jul 13, 2018 at 04:06:02PM +0800, 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 contorl power
> 
> 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, the correct audio id will be set in
> vgaswitchreoo enable function.
> 
> Signed-off-by: Jim Qu <Jim.Qu@amd.com>

The approach you've taken in this patch won't work if the HDA controller
is bound to a driver after both GPUs have already been bound to a driver
and the handler has already been registered.

That's because vga_switcheroo_enable() is run when two GPUs have registered
as clients and the handler has also registered.  If the HDA controller is
probed afterwards, its ID will be stuck at VGA_SWITCHEROO_UNKNOWN_ID.

Before resubmitting this patch, please run it through scripts/checkpatch.pl,
there are multiple trivial issues here.

Also, please use scripts/get_maintainers.pl on all files you're touching
and cc the patch to the listed maintainers and reviewers.


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

There's a spurious whitespace change in the client_is_audio() line,
please get rid of it.


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

Hm, why is this change necessary?


Thanks,

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

Re: [alsa-devel] [PATCH 1/2] ALSA: HDA: use PCI_BASE_CLASS_DISPLAY to cover more display adapters

[Qu, Jim]

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

See comments in line

thanks
JimQu

获取 Outlook for Android<https://aka.ms/ghei36>



发件人: Takashi Iwai
发送时间: 7月13日星期五 16:33
主题: Re: [alsa-devel] [PATCH 1/2] ALSA: HDA: use PCI_BASE_CLASS_DISPLAY tocover more display adapters
收件人: Qu, Jim
抄送: alsa-devel@alsa-project.org, dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org


On Fri, 13 Jul 2018 10:06:01 +0200, Jim Qu wrote: > > Signed-off-by: Jim Qu More explanations please. e.g. why is this needed? The code change itself looks good, but ... > @@ -31,7 +31,7 @@ > * CHANGES: > * > * 2004.12.01 Major rewrite by tiwai, merged the work of pshou > - * > + * > */ > > #include > @@ -390,8 +390,8 @@ static char *driver_short_names[] = { > [AZX_DRIVER_SIS] = "HDA SIS966", > [AZX_DRIVER_ULI] = "HDA ULI M5461", > [AZX_DRIVER_NVIDIA] = "HDA NVidia", > - [AZX_DRIVER_TERA] = "HDA Teradici", > - [AZX_DRIVER_CTX] = "HDA Creative", > + [AZX_DRIVER_TERA] = "HDA Teradici", > + [AZX_DRIVER_CTX] = "HDA Creative", > [AZX_DRIVER_CTHDA] = "HDA Creative", > [AZX_DRIVER_CMEDIA] = "HDA C-Media", > [AZX_DRIVER_GENERIC] = "HD-Audio Generic", These two hunks are utterly irrelevant with the fix. Please drop them.

Jim: Yeah, I forget to generate a new one. I will delete them when push the patch.

thanks, Takashi


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

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

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

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

[jimqu]

在 2018/7/13 16:46, Takashi Iwai 写道:
> On Fri, 13 Jul 2018 10:06:02 +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 contorl power
>>
>> 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, the correct audio id will be set in
>> vgaswitchreoo enable function.
>>
>> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
> The idea looks good to me.  Just a nitpick:
>
>> @@ -1319,15 +1319,16 @@ 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);
> Passing a NULL vga_dev won't work, so the NULL check after calling
> vga_switcheroo_register_audio_client() is moot.  If any, it should be
> checked before that.
>

  The hda->use_vga_switcheroo is set to 1 in init_vga_switcheroo() if 
bound vga pci dev is not NULL. Since the hda->use_vga_switcheroo is 
checked at beginning of the function. so the p which pass into 
vga_switcheroo_register_audio_client() is constant valid value.

Thanks
JimQu
> thanks,
>
> Takashi

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

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

[jimqu]

在 2018/7/13 17:27, Lukas Wunner 写道:
> On Fri, Jul 13, 2018 at 04:06:02PM +0800, 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 contorl power
>>
>> 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, the correct audio id will be set in
>> vgaswitchreoo enable function.
>>
>> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
> The approach you've taken in this patch won't work if the HDA controller
> is bound to a driver after both GPUs have already been bound to a driver
> and the handler has already been registered.
>
> That's because vga_switcheroo_enable() is run when two GPUs have registered
> as clients and the handler has also registered.  If the HDA controller is
> probed afterwards, its ID will be stuck at VGA_SWITCHEROO_UNKNOWN_ID.
>
> Before resubmitting this patch, please run it through scripts/checkpatch.pl,
> there are multiple trivial issues here.
>
> Also, please use scripts/get_maintainers.pl on all files you're touching
> and cc the patch to the listed maintainers and reviewers.
>
>
>> @@ -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)
> There's a spurious whitespace change in the client_is_audio() line,
> please get rid of it.
>

Yeah, I will correct it in later update.

>> --- 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,
> Hm, why is this change necessary?

Before, we suppose VGA_SWITCHEROO_UNKNOWN_ID can be only used for gpu 
client, but now we also use it on audio client. Because of -1 in memory 
is presented as 0xffffffffffffffff, so the 0xffffffffffffffff & 
ID_BIT_AUDIO = ID_BIT_AUDIO, this will confuse people if it used on gpu 
client. that audio identify bit is set.

Thanks
JimQu
>
> Thanks,
>
> Lukas

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

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

[Takashi Iwai]

On Fri, 13 Jul 2018 17:25:39 +0200,
jimqu wrote:
> 
> 
> 
> 在 2018/7/13 16:46, Takashi Iwai 写道:
> > On Fri, 13 Jul 2018 10:06:02 +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 contorl power
> >>
> >> 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, the correct audio id will be set in
> >> vgaswitchreoo enable function.
> >>
> >> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
> > The idea looks good to me.  Just a nitpick:
> >
> >> @@ -1319,15 +1319,16 @@ 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);
> > Passing a NULL vga_dev won't work, so the NULL check after calling
> > vga_switcheroo_register_audio_client() is moot.  If any, it should be
> > checked before that.
> >
> 
>  The hda->use_vga_switcheroo is set to 1 in init_vga_switcheroo() if
> bound vga pci dev is not NULL. Since the hda->use_vga_switcheroo is
> checked at beginning of the function. so the p which pass into
> vga_switcheroo_register_audio_client() is constant valid value.

So the NULL-check in the patch is also useless, no? :)


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

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

[jimqu]

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



在 2018/7/14 0:38, Takashi Iwai 写道:
> On Fri, 13 Jul 2018 17:25:39 +0200,
> jimqu wrote:
>>
>>
>> 在 2018/7/13 16:46, Takashi Iwai 写道:
>>> On Fri, 13 Jul 2018 10:06:02 +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 contorl power
>>>>
>>>> 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, the correct audio id will be set in
>>>> vgaswitchreoo enable function.
>>>>
>>>> Signed-off-by: Jim Qu <Jim.Qu-5C7GfCeVMHo@public.gmane.org>
>>> The idea looks good to me.  Just a nitpick:
>>>
>>>> @@ -1319,15 +1319,16 @@ 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);
>>> Passing a NULL vga_dev won't work, so the NULL check after calling
>>> vga_switcheroo_register_audio_client() is moot.  If any, it should be
>>> checked before that.
>>>
>>   The hda->use_vga_switcheroo is set to 1 in init_vga_switcheroo() if
>> bound vga pci dev is not NULL. Since the hda->use_vga_switcheroo is
>> checked at beginning of the function. so the p which pass into
>> vga_switcheroo_register_audio_client() is constant valid value.
> So the NULL-check in the patch is also useless, no? :)

Ha-ha, indeed, the NULL-check of p is unnecessary. will update in v2.

Thanks
JimQu
> Takashi


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

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

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

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

[jimqu]

在 2018/7/13 17:27, Lukas Wunner 写道:
> On Fri, Jul 13, 2018 at 04:06:02PM +0800, 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 contorl power
>>
>> 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, the correct audio id will be set in
>> vgaswitchreoo enable function.
>>
>> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
> The approach you've taken in this patch won't work if the HDA controller
> is bound to a driver after both GPUs have already been bound to a driver
> and the handler has already been registered.
>
> That's because vga_switcheroo_enable() is run when two GPUs have registered
> as clients and the handler has also registered.  If the HDA controller is
> probed afterwards, its ID will be stuck at VGA_SWITCHEROO_UNKNOWN_ID.

In genernic speaking, there are two cases , a. audio client is the third 
client. b. audio client is not the third client.

if audio is third client. vga_switcheroo_ready() is true, 
vga_switcheroo_enable() can be called in audio client register fuction.
if audio is not the third client, vga_switcheroo_enable() will be called 
in the second GFX client register.
In vga_switcheroo_enable() , the first list loop will set two GPU 
clients' id, and the second list loop will select audio client, set id 
by its bound gpu pci dev.

> Before resubmitting this patch, please run it through scripts/checkpatch.pl,
> there are multiple trivial issues here.
>
> Also, please use scripts/get_maintainers.pl on all files you're touching
> and cc the patch to the listed maintainers and reviewers.
>
OK, will do it in next v2.
>> @@ -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)
> There's a spurious whitespace change in the client_is_audio() line,
> please get rid of it.
>

Will update it in v2.

>> --- 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,
> Hm, why is this change necessary?
Before , we suppose  VGA_SWITCHEROO_UNKNOWN_ID can be only used for gpu 
client, and now we also use it on the audio client. if we set 
VGA_SWITCHEROO_UNKNOWN_ID to gpu clients, client_is_audio(client) will 
be true, since -1 in memory is presented as 0xffffffffffffffff(if it is 
64bit asic).

thanks
JimQu
>
> Thanks,
>
> Lukas

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

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

[Lukas Wunner]

On Sat, Jul 14, 2018 at 02:15:24PM +0800, jimqu wrote:
> ??? 2018/7/13 17:27, Lukas Wunner ??????:
> >On Fri, Jul 13, 2018 at 04:06:02PM +0800, 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 contorl power
> >>
> >>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, the correct audio id will be set in
> >>vgaswitchreoo enable function.
> >>
> >>Signed-off-by: Jim Qu <Jim.Qu@amd.com>
> >The approach you've taken in this patch won't work if the HDA controller
> >is bound to a driver after both GPUs have already been bound to a driver
> >and the handler has already been registered.
> >
> >That's because vga_switcheroo_enable() is run when two GPUs have registered
> >as clients and the handler has also registered.  If the HDA controller is
> >probed afterwards, its ID will be stuck at VGA_SWITCHEROO_UNKNOWN_ID.
> 
> In genernic speaking, there are two cases , a. audio client is the third
> client. b. audio client is not the third client.
> 
> if audio is third client. vga_switcheroo_ready() is true,
> vga_switcheroo_enable() can be called in audio client register fuction.
> if audio is not the third client, vga_switcheroo_enable() will be called in
> the second GFX client register.
> In vga_switcheroo_enable() , the first list loop will set two GPU clients'
> id, and the second list loop will select audio client, set id by its bound
> gpu pci dev.

No, if audio is the third client, vga_switcheroo_ready() is *not* true
because vgasr_priv.active is true.  This is set to true once the two
GPUs and the handler have registered.  If the audio device registers
afterwards, vga_switcheroo_enable() will already have been called.
It's never called twice because it sets vgasr_priv.active = true.

As a result, the audio device is stuck at VGA_SWITCHEROO_UNKNOWN_ID.

Thanks,

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

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

[jimqu]

在 2018/7/16 22:02, Lukas Wunner 写道:
> On Sat, Jul 14, 2018 at 02:15:24PM +0800, jimqu wrote:
>> ??? 2018/7/13 17:27, Lukas Wunner ??????:
>>> On Fri, Jul 13, 2018 at 04:06:02PM +0800, 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 contorl power
>>>>
>>>> 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, the correct audio id will be set in
>>>> vgaswitchreoo enable function.
>>>>
>>>> Signed-off-by: Jim Qu <Jim.Qu@amd.com>
>>> The approach you've taken in this patch won't work if the HDA controller
>>> is bound to a driver after both GPUs have already been bound to a driver
>>> and the handler has already been registered.
>>>
>>> That's because vga_switcheroo_enable() is run when two GPUs have registered
>>> as clients and the handler has also registered.  If the HDA controller is
>>> probed afterwards, its ID will be stuck at VGA_SWITCHEROO_UNKNOWN_ID.
>> In genernic speaking, there are two cases , a. audio client is the third
>> client. b. audio client is not the third client.
>>
>> if audio is third client. vga_switcheroo_ready() is true,
>> vga_switcheroo_enable() can be called in audio client register fuction.
>> if audio is not the third client, vga_switcheroo_enable() will be called in
>> the second GFX client register.
>> In vga_switcheroo_enable() , the first list loop will set two GPU clients'
>> id, and the second list loop will select audio client, set id by its bound
>> gpu pci dev.
> No, if audio is the third client, vga_switcheroo_ready() is *not* true
> because vgasr_priv.active is true.  This is set to true once the two
> GPUs and the handler have registered.  If the audio device registers
> afterwards, vga_switcheroo_enable() will already have been called.
> It's never called twice because it sets vgasr_priv.active = true.
>
> As a result, the audio device is stuck at VGA_SWITCHEROO_UNKNOWN_ID.

Indeed, I almost ignore the vgasr_priv.active, since I added amdgpu to 
blacklist, audio is always the first client. :).
not so bad, Let me rework the patch tomorrow.

Thanks
JimQu

> Thanks,
>
> Lukas

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