ALSA: hda - Fix wrong detection of "Headphone+LO" or "Speaker+LO"

PATCH In review Ingo Brückl 3 participants 7 messages Mar 3, 2015 – Mar 5, 2015 Sound

Share & tools

David Henningsson

2015-03-03 13:24 UTC #1 lore raw

Hi Ingo and thanks for trying to improve the driver,

I've been reading through the alsa-devel thread but your alsa-info was removed when it was posted to the list so it's a bit hard to see the context.

If I understand correctly, you have three DACs, one internal speaker, one headphone jack, and three jacks that are both used for 5.1 surround and line out/mic/line in. Is this correct?

How do the DACs get assigned in this case? One would assume that you'd get 02 -> Front LO, HP, Speaker, 03 -> Rear LO, 04 -> CLFE LO. And then the volume/mute control for DAC node 02 would be called "PCM" (since both hp_lo_shared and spk_lo_shared are true), but in fact it would be more appropriate to call it "Front".

Anyhow, I'd say that the typical case where we want the "Headphone+LO" names is where we have only one LO, and then multiout.num_dacs would be 1, not 2. (I think, it was a while since I looked into that part of the driver...)

// David

Quoted 24 lines · Ingo Brückl

Add the constraint mentioned in the comment.
It does not apply to a scenario with three DACs and multi-IO where we would normally get a "Master Playback Volume" and a "Front Playback Volume" (prevented without checking for num_dacs).
Signed-off-by Ingo Brückl <[email protected]>
sound/pci/hda/hda_generic.c +1 -2
index ecee349..4c8910c 100644
@@ -1097,7 +1097,7 @@ static const char *get_line_out_pfx(struct hda_codec *codec, int ch,
  	case AUTO_PIN_LINE_OUT:
  		/* This deals with the case where we have two DACs and
  		 * one LO, one HP and one Speaker */
-		if (!ch && cfg->speaker_outs && cfg->hp_outs) {
+		if (!ch && spec->multiout.num_dacs == 2 && cfg->speaker_outs && cfg->hp_outs) {
  			bool hp_lo_shared = !path_has_mixer(codec, spec->hp_paths[0], ctl_type);
  			bool spk_lo_shared = !path_has_mixer(codec, spec->speaker_paths[0], ctl_type);
  			if (hp_lo_shared && spk_lo_shared)
--
1.7.10

Signature · 2 lines

David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Ingo Brückl

author

2015-03-03 15:06 UTC #2 lore raw

David Henningsson wrote on Tue, 03 Mar 2015 14:24:28 +0100:

Quoted 3 lines

If I understand correctly, you have three DACs, one internal speaker, one headphone jack, and three jacks that are both used for 5.1 surround and line out/mic/line in. Is this correct?

This is correct. In addition I have a front mic jack.

Quoted 2 lines

How do the DACs get assigned in this case? One would assume that you'd get 02 -> Front LO, HP, Speaker, 03 -> Rear LO, 04 -> CLFE LO.

With my private patch to enforce multi-io I seem to lose the internal speaker which isn't bad because it isn't connected (and probably never won't be).

You are right concerning the remaining assignments:

multi_outs = 14/0/0/0 : 2/3/4/0 (type LO)
out path: depth=3 '02:0c:14'
multi_ios(2) = 1a/18 : 3/4
mio path: depth=3 '03:0d:1a'
mio path: depth=3 '04:0e:18'
hp_outs = 1b/0/0/0 : 2/0/0/0
hp path: depth=3 '02:0c:1b'
spk_outs = 15/0/0/0 : 0/0/0/0

Quoted 3 lines

And then the volume/mute control for DAC node 02 would be called "PCM" (since both hp_lo_shared and spk_lo_shared are true), but in fact it would be more appropriate to call it "Front".

I've got a Master that would only affect the Front (PCM) and a PCM that affected Surround/CLFE which was very unpleasant.

With the patch I'm getting a real Master (for all Front/Surround/CLFE) and a separate Front Volume Control in addition to Surround, Center and LFE which is exactly how it should be.

Quoted 4 lines

Anyhow, I'd say that the typical case where we want the "Headphone+LO" names is where we have only one LO, and then multiout.num_dacs would be 1, not 2. (I think, it was a while since I looked into that part of the driver...)

I'm fine with everything below 3. :-) Just tell me.

Ingo

David Henningsson

2015-03-03 16:02 UTC #3 lore raw

Quoted 7 lines · Ingo Brückl

David Henningsson wrote on Tue, 03 Mar 2015 14:24:28 +0100:
Quoted 3 lines
If I understand correctly, you have three DACs, one internal speaker, one headphone jack, and three jacks that are both used for 5.1 surround and line out/mic/line in. Is this correct?
This is correct. In addition I have a front mic jack.

Could you upload/pastebin your alsa-info somewhere? Then we could run it through the emulator and reproduce the problem.

Quoted 5 lines

Quoted 2 lines
How do the DACs get assigned in this case? One would assume that you'd get 02 -> Front LO, HP, Speaker, 03 -> Rear LO, 04 -> CLFE LO.
With my private patch to enforce multi-io I seem to lose the internal speaker which isn't bad because it isn't connected (and probably never won't be).

Since you're using private patches you're somewhat out on "unsupported" ground, but anyhow...

Quoted 16 lines

You are right concerning the remaining assignments:
multi_outs = 14/0/0/0 : 2/3/4/0 (type LO)
out path: depth=3 '02:0c:14'
multi_ios(2) = 1a/18 : 3/4
mio path: depth=3 '03:0d:1a'
mio path: depth=3 '04:0e:18'
hp_outs = 1b/0/0/0 : 2/0/0/0
hp path: depth=3 '02:0c:1b'
spk_outs = 15/0/0/0 : 0/0/0/0
Quoted 3 lines
And then the volume/mute control for DAC node 02 would be called "PCM" (since both hp_lo_shared and spk_lo_shared are true), but in fact it would be more appropriate to call it "Front".

Quoted 6 lines

I've got a Master that would only affect the Front (PCM) and a PCM that affected Surround/CLFE which was very unpleasant.
With the patch I'm getting a real Master (for all Front/Surround/CLFE) and a separate Front Volume Control in addition to Surround, Center and LFE which is exactly how it should be.

Hmm. What do you think of the attached patch - would it work as well? It removes the part that returns early for all three, letting it fall through to the part that returns "Front" etc.

Signature · 2 lines

David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

sound/pci/hda/hda_generic.c +2 -4

index b680b4e..9f0be7c 100644
@@ -1100,11 +1100,9 @@ static const char *get_line_out_pfx(struct hda_codec *codec, int ch,
 		if (!ch && cfg->speaker_outs && cfg->hp_outs) {
 			bool hp_lo_shared = !path_has_mixer(codec, spec->hp_paths[0], ctl_type);
 			bool spk_lo_shared = !path_has_mixer(codec, spec->speaker_paths[0], ctl_type);
-			if (hp_lo_shared && spk_lo_shared)
-				return spec->vmaster_mute.hook ? "PCM" : "Master";
-			if (hp_lo_shared)
+			if (hp_lo_shared && !spk_lo_shared)
 				return "Headphone+LO";
-			if (spk_lo_shared)
+			if (spk_lo_shared && !hp_lo_shared)
 				return "Speaker+LO";
 		}
 	}

Ingo Brückl

author

2015-03-05 10:31 UTC #4 lore raw

David Henningsson wrote on Tue, 03 Mar 2015 17:02:19 +0100:

Quoted 19 lines

Hmm. What do you think of the attached patch - would it work as well? It removes the part that returns early for all three, letting it fall through to the part that returns "Front" etc.

+++ b/sound/pci/hda/hda_generic.c

sound/pci/hda/hda_generic.c +2 -4

@@ -1100,11 +1100,9 @@ static const char *get_line_out_pfx(struct
 hda_codec *codec, int ch,
                if (!ch && cfg->speaker_outs && cfg->hp_outs) {
                        bool hp_lo_shared = !path_has_mixer(codec, spec->hp_paths[0], ctl_type);
                        bool spk_lo_shared = !path_has_mixer(codec, spec->speaker_paths[0], ctl_type);
-                       if (hp_lo_shared && spk_lo_shared)
-                               return spec->vmaster_mute.hook ? "PCM" : "Master";
-                       if (hp_lo_shared)
+                       if (hp_lo_shared && !spk_lo_shared)
                                return "Headphone+LO";
-                       if (spk_lo_shared)
+                       if (spk_lo_shared && !hp_lo_shared)
                                return "Speaker+LO";
                }

Quoted 1 line

After a quick check, this seems better. Master is a real master now and there is a Front control.

However, due to the speaker having no path with my forced config, I cannot say whether this will work if the parser could find the multi-io config by itself (and until then there probably shouldn't be patches I suggested). And, by the way, as a result of the no-path I'm getting a Speaker+LO control.

Here a little additional debug output during the get_line_out_pfx() calls. The "path_has_mixer" output refers to a call which output is in the following line, i. e. the call for getting spk_lo_shared:

[    0.648407] sound hdaudioC1D0: get_line_out_pfx, AUTO_PIN_LINE_OUT: hp_lo_shared=1
[    0.648407] sound hdaudioC1D0: path_has_mixer: no path!
[    0.648408] sound hdaudioC1D0: get_line_out_pfx, AUTO_PIN_LINE_OUT: spk_lo_shared=1
[    0.648409] sound hdaudioC1D0: get_line_out_pfx, AUTO_PIN_LINE_OUT: hp_lo_shared=0
[    0.648410] sound hdaudioC1D0: path_has_mixer: no path!
[    0.648410] sound hdaudioC1D0: get_line_out_pfx, AUTO_PIN_LINE_OUT: spk_lo_shared=1

Ingo

Raymond Yau

2015-03-05 11:43 UTC #5 lore raw

Quoted 10 lines

Review static const char *get_line_out_pfx(struct · line 1102
Quoted 10 lines
Hmm. What do you think of the attached patch - would it work as well? It removes the part that returns early for all three, letting it fall through to the part that returns "Front" etc.
+++ b/sound/pci/hda/hda_generic.c
(quoted diff) +0 -0
@@ -1100,11 +1100,9 @@ static const char *get_line_out_pfx(struct
 hda_codec *codec, int ch,
                if (!ch && cfg->speaker_outs && cfg->hp_outs) {
                        bool hp_lo_shared = !path_has_mixer(codec,

spec->hp_paths[0], ctl_type);

Quoted 1 line

Review static const char *get_line_out_pfx(struct · line 1103
Quoted 1 line
(quoted diff) +0 -0
                        bool spk_lo_shared = !path_has_mixer(codec,

spec->speaker_paths[0], ctl_type);

Quoted 2 lines

Review static const char *get_line_out_pfx(struct · line 1105
Quoted 2 lines
(quoted diff) +0 -2
-                       if (hp_lo_shared && spk_lo_shared)
-                               return spec->vmaster_mute.hook ? "PCM"

: "Master";

Quoted 10 lines

Quoted 8 lines

sound/pci/hda/hda_generic.c +2 -2

-                       if (hp_lo_shared)
+                       if (hp_lo_shared && !spk_lo_shared)
                                return "Headphone+LO";
-                       if (spk_lo_shared)
+                       if (spk_lo_shared && !hp_lo_shared)
                                return "Speaker+LO";
                }

After a quick check, this seems better. Master is a real master now and

there

Quoted 3 lines

is a Front control.
Quoted 1 line
Quoted 1 line
Quoted 1 line
With the patch I'm getting a real Master (for all

Front/Surround/CLFE) and a

Quoted 1 line

Quoted 1 line
separate Front Volume Control in addition to Surround, Center and LFE

which >> is exactly how it should be.

Quoted 1 line

Quoted 1 line
Quoted 1 line
Hmm. What do you think of the attached patch - would it work as well?

It removes the part that returns early for all three, letting it fall through to the part that returns "Front" etc.

http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/src/modules/alsa/mixer/paths/analog-output-headphones.conf?id=
96369919e5100865e2469e42fb8f4b8e38e41aef

Do you mean front playback volume is set to maximum when headphone is plugged?

How do Pulseaudio calculate dB min of headphone path and lineout path?

Do it just put dB min of front playback volume in headphone path only when there is no headphone playback volume?

BTW codec alc888 in buglink can use amp out in node 0x26 as Headphone Playback volume

Quoted 4 lines

However, due to the speaker having no path with my forced config, I cannot say whether this will work if the parser could find the multi-io config by itself (and until then there probably shouldn't be patches I suggested).

And,

Quoted 4 lines

by the way, as a result of the no-path I'm getting a Speaker+LO control.
Here a little additional debug output during the get_line_out_pfx() calls. The "path_has_mixer" output refers to a call which output is in the

following

Quoted 3 lines

line, i. e. the call for getting spk_lo_shared:
[    0.648407] sound hdaudioC1D0: get_line_out_pfx, AUTO_PIN_LINE_OUT:

hp_lo_shared=1

Quoted 2 lines

[    0.648407] sound hdaudioC1D0: path_has_mixer: no path!
[    0.648408] sound hdaudioC1D0: get_line_out_pfx, AUTO_PIN_LINE_OUT:

spk_lo_shared=1

Quoted 1 line

[    0.648409] sound hdaudioC1D0: get_line_out_pfx, AUTO_PIN_LINE_OUT:

hp_lo_shared=0

Quoted 2 lines

[    0.648410] sound hdaudioC1D0: path_has_mixer: no path!
[    0.648410] sound hdaudioC1D0: get_line_out_pfx, AUTO_PIN_LINE_OUT:

spk_lo_shared=1

Quoted 2 lines

Ingo

Raymond Yau

2015-03-03 15:18 UTC #6 lore raw

In reply to Ingo Brückl

sound/pci/hda/hda_generic.c static const char *get_line_out_pfx(struct

Quoted 14 lines

Add the constraint mentioned in the comment.
It does not apply to a scenario with three DACs and multi-IO where we would normally get a "Master Playback Volume" and a "Front Playback Volume" (prevented without checking for num_dacs).
Signed-off-by Ingo Brückl <[email protected]>
sound/pci/hda/hda_generic.c +0 -0
index ecee349..4c8910c 100644
@@ -1097,7 +1097,7 @@ static const char *get_line_out_pfx(struct

hda_codec *codec, int ch,

sound/pci/hda/hda_generic.c static const char *get_line_out_pfx(struct hda_codec *codec, int ch,

Quoted 5 lines

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

        case AUTO_PIN_LINE_OUT:
                /* This deals with the case where we have two DACs and
                 * one LO, one HP and one Speaker */
-               if (!ch && cfg->speaker_outs && cfg->hp_outs) {
+               if (!ch && spec->multiout.num_dacs == 2 &&

cfg->speaker_outs && cfg->hp_outs) {

spec->multiout.num_dacs == 2 mean support 4 channels

You need to find out the number of analog dacs from spec->all_dacs[] exclude the digital output

sound/pci/hda/hda_generic.c static const char *get_line_out_pfx(struct · line 1101

Quoted 1 line

sound/pci/hda/hda_generic.c +0 -0

                        bool hp_lo_shared = !path_has_mixer(codec,

spec->hp_paths[0], ctl_type);

sound/pci/hda/hda_generic.c static const char *get_line_out_pfx(struct · line 1102

Quoted 1 line

sound/pci/hda/hda_generic.c +0 -0

                        bool spk_lo_shared = !path_has_mixer(codec,

spec->speaker_paths[0], ctl_type);

Quoted 3 lines

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

                        if (hp_lo_shared && spk_lo_shared)
--

1.7.10

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel