ALSA: HDA: use PCI_BASE_CLASS_DISPLAY to cover more display adapters

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 <[email protected]>

sound/pci/hda/hda_intel.c +1 -0

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

Quoted 9 lines · 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

Quoted 6 lines

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 <[email protected]>

The idea looks good to me. Just a nitpick:

Quoted 7 lines

sound/pci/hda/hda_intel.c +1 -0

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

sound/pci/hda/hda_intel.c static const struct vga_switcheroo_client_ops azx_vs_ops = { · line 1331

Quoted 12 lines

sound/pci/hda/hda_intel.c +5 -5

  	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

Quoted 32 lines · Takashi Iwai

Quoted 9 lines · 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

Quoted 6 lines

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 <[email protected]>

The idea looks good to me. Just a nitpick:

Quoted 13 lines

sound/pci/hda/hda_intel.c +1 -5

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

Quoted 9 lines

sound/pci/hda/hda_intel.c static const struct vga_switcheroo_client_ops azx_vs_ops = { · line 1331

Quoted 5 lines

sound/pci/hda/hda_intel.c +5 -0

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

Quoted 44 lines · jimqu

Quoted 41 lines · Takashi Iwai

Quoted 9 lines · 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

Quoted 6 lines

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 <[email protected]>

The idea looks good to me. Just a nitpick:

Quoted 8 lines

(quoted diff) +1 -0

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

Review static const struct vga_switcheroo_client_ops azx_vs_ops = { · line 1330

Quoted 10 lines

(quoted diff) +5 -5

-	/* 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.

Quoted 4 lines

 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

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 <[email protected]>

Quoted 9 lines · 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

Quoted 6 lines

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 <[email protected]>

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.

Quoted 3 lines

(quoted diff) +0 -0

@@ -161,9 +163,8 @@ struct vgasr_priv {
  };
 

drivers/gpu/vga/vga_switcheroo.c struct vgasr_priv { · line 168

Quoted 7 lines

drivers/gpu/vga/vga_switcheroo.c +2 -3

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

include/linux/vga_switcheroo.h enum vga_switcheroo_state { · line 88

Quoted 10 lines

include/linux/vga_switcheroo.h +2 -2

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

Quoted 14 lines · 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.

Quoted 1 line

Signed-off-by Jim Qu <[email protected]>

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.

drivers/gpu/vga/vga_switcheroo.c struct vgasr_priv { · line 168

Quoted 10 lines

drivers/gpu/vga/vga_switcheroo.c +2 -3

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

include/linux/vga_switcheroo.h enum vga_switcheroo_state { · line 88

Quoted 10 lines

include/linux/vga_switcheroo.h +2 -2

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

??? 2018/7/13 17:27, Lukas Wunner ??????:

Quoted 15 lines · Jim Qu

Quoted 14 lines

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.

Quoted 8 lines

Quoted 1 line

Signed-off-by Jim Qu <[email protected]>

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.

Quoted 27 lines · jimqu

??? 2018/7/13 17:27, Lukas Wunner ??????:

Quoted 15 lines · Jim Qu

Quoted 14 lines

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.

Quoted 8 lines

Quoted 1 line

Signed-off-by Jim Qu <[email protected]>

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.

Quoted 7 lines

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

Signed-off-by Jim Qu <[email protected]>

Signed-off-by Jim Qu <[email protected]>

sound/pci/hda/hda_intel.c +1 -1

@@ -31,7 +31,7 @@
  *  CHANGES:
  *
  *  2004.12.01	Major rewrite by tiwai, merged the work of pshou
- * 
+ *
  */
 
 #include <linux/delay.h>