* Multiple speaker jacks (on an IDT codec) @ 2012-09-05 22:59 David Henningsson 2012-09-06 6:14 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: David Henningsson @ 2012-09-05 22:59 UTC (permalink / raw) To: alsa-devel@alsa-project.org; +Cc: Takashi Iwai Hi, I'm having a problem with a machine where the parser creates indexed speaker controls. For reference, codecs/idt92hd206-ecs-gf7100pvt shows the same problem. I could use some input on how to fix it in the best way. My first thought was that "[Jack] Speaker" is stupid in itself, and could be fixed like this: diff --git a/sound/pci/hda/hda_auto_parser.c b/sound/pci/hda/hda_auto_parser.c index 4f7d2df..8e32ee5 100644 --- a/sound/pci/hda/hda_auto_parser.c +++ b/sound/pci/hda/hda_auto_parser.c @@ -171,6 +171,10 @@ int snd_hda_parse_pin_defcfg(struct hda_codec *codec, if (conn == AC_JACK_PORT_FIXED) dev = AC_JACK_SPEAKER; } + else if (dev == AC_JACK_SPEAKER) { + if (conn == AC_JACK_PORT_COMPLEX) + dev = AC_JACK_LINE_OUT; + } switch (dev) { case AC_JACK_LINE_OUT: ...but changing generic code is always a bit scary and regression prone (even though I could not think of a good counterexample where that would happen), so not sure if that's the best idea. Next attempt is to modify the control creating function for IDT: diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c index 9db3056..c16d0f8 100644 --- a/sound/pci/hda/patch_sigmatel.c +++ b/sound/pci/hda/patch_sigmatel.c @@ -3226,9 +3226,12 @@ static int create_multi_out_ctls(struct hda_codec *codec, int num_outs, idx = i; break; case AUTO_PIN_SPEAKER_OUT: - name = "Speaker"; - idx = i; - break; + if (num_outs <= 1) { + name = "Speaker"; + idx = i; + break; + } + /* Fall through in case of multi speaker outs */ default: name = chname[i]; idx = 0; ...since the current behaviour is inconsistent anyway - you get indices for 1 and 3, but "Center"/"LFE" for what should otherwise have been index 2. Another way would to just write a pin fix to turn these speakers into line outs, which is what they really are anyway. What do you think? -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Multiple speaker jacks (on an IDT codec) 2012-09-05 22:59 Multiple speaker jacks (on an IDT codec) David Henningsson @ 2012-09-06 6:14 ` Takashi Iwai 2012-09-06 9:33 ` David Henningsson 0 siblings, 1 reply; 4+ messages in thread From: Takashi Iwai @ 2012-09-06 6:14 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel@alsa-project.org At Thu, 06 Sep 2012 00:59:15 +0200, David Henningsson wrote: > > Hi, > > I'm having a problem with a machine where the parser creates indexed > speaker controls. For reference, codecs/idt92hd206-ecs-gf7100pvt shows > the same problem. > > I could use some input on how to fix it in the best way. > > My first thought was that "[Jack] Speaker" is stupid in itself, and > could be fixed like this: > > diff --git a/sound/pci/hda/hda_auto_parser.c > b/sound/pci/hda/hda_auto_parser.c > index 4f7d2df..8e32ee5 100644 > --- a/sound/pci/hda/hda_auto_parser.c > +++ b/sound/pci/hda/hda_auto_parser.c > @@ -171,6 +171,10 @@ int snd_hda_parse_pin_defcfg(struct hda_codec *codec, > if (conn == AC_JACK_PORT_FIXED) > dev = AC_JACK_SPEAKER; > } > + else if (dev == AC_JACK_SPEAKER) { > + if (conn == AC_JACK_PORT_COMPLEX) > + dev = AC_JACK_LINE_OUT; > + } > > switch (dev) { > case AC_JACK_LINE_OUT: > > ...but changing generic code is always a bit scary and regression prone > (even though I could not think of a good counterexample where that would > happen), so not sure if that's the best idea. Right. I remember that there are more devices declaring the speakers in that way. > Next attempt is to modify the control creating function for IDT: > > diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c > index 9db3056..c16d0f8 100644 > --- a/sound/pci/hda/patch_sigmatel.c > +++ b/sound/pci/hda/patch_sigmatel.c > @@ -3226,9 +3226,12 @@ static int create_multi_out_ctls(struct hda_codec > *codec, int num_outs, > idx = i; > break; > case AUTO_PIN_SPEAKER_OUT: > - name = "Speaker"; > - idx = i; > - break; > + if (num_outs <= 1) { > + name = "Speaker"; > + idx = i; > + break; > + } > + /* Fall through in case of multi speaker > outs */ > default: > name = chname[i]; > idx = 0; > > > ...since the current behaviour is inconsistent anyway - you get indices > for 1 and 3, but "Center"/"LFE" for what should otherwise have been index 2. Yeah, certainly something fishy there. IIRC, the exception was introduced to handle the bass speaker. But I guess it's already broken in some level, so we need to revisit the implementation. A concern for the change like above is whether it handles correctly the case where machines has multiple built-in speakers with a single headphone jack. We need to check this. > Another way would to just write a pin fix to turn these speakers into > line outs, which is what they really are anyway. > > What do you think? This might be a safer option. Takashi ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Multiple speaker jacks (on an IDT codec) 2012-09-06 6:14 ` Takashi Iwai @ 2012-09-06 9:33 ` David Henningsson 2012-09-06 9:44 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: David Henningsson @ 2012-09-06 9:33 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel@alsa-project.org [-- Attachment #1: Type: text/plain, Size: 4085 bytes --] On 09/06/2012 08:14 AM, Takashi Iwai wrote: > At Thu, 06 Sep 2012 00:59:15 +0200, > David Henningsson wrote: >> >> Hi, >> >> I'm having a problem with a machine where the parser creates indexed >> speaker controls. For reference, codecs/idt92hd206-ecs-gf7100pvt shows >> the same problem. >> >> I could use some input on how to fix it in the best way. >> >> My first thought was that "[Jack] Speaker" is stupid in itself, and >> could be fixed like this: >> >> diff --git a/sound/pci/hda/hda_auto_parser.c >> b/sound/pci/hda/hda_auto_parser.c >> index 4f7d2df..8e32ee5 100644 >> --- a/sound/pci/hda/hda_auto_parser.c >> +++ b/sound/pci/hda/hda_auto_parser.c >> @@ -171,6 +171,10 @@ int snd_hda_parse_pin_defcfg(struct hda_codec *codec, >> if (conn == AC_JACK_PORT_FIXED) >> dev = AC_JACK_SPEAKER; >> } >> + else if (dev == AC_JACK_SPEAKER) { >> + if (conn == AC_JACK_PORT_COMPLEX) >> + dev = AC_JACK_LINE_OUT; >> + } >> >> switch (dev) { >> case AC_JACK_LINE_OUT: >> >> ...but changing generic code is always a bit scary and regression prone >> (even though I could not think of a good counterexample where that would >> happen), so not sure if that's the best idea. > > Right. I remember that there are more devices declaring the speakers > in that way. I grepped a little through the alsa-infos and found a "[Jack] Speaker at Int N/A", which is probably an internal speaker rather than a line out...covering all cases is tricky. >> Next attempt is to modify the control creating function for IDT: >> >> diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c >> index 9db3056..c16d0f8 100644 >> --- a/sound/pci/hda/patch_sigmatel.c >> +++ b/sound/pci/hda/patch_sigmatel.c >> @@ -3226,9 +3226,12 @@ static int create_multi_out_ctls(struct hda_codec >> *codec, int num_outs, >> idx = i; >> break; >> case AUTO_PIN_SPEAKER_OUT: >> - name = "Speaker"; >> - idx = i; >> - break; >> + if (num_outs <= 1) { >> + name = "Speaker"; >> + idx = i; >> + break; >> + } >> + /* Fall through in case of multi speaker >> outs */ >> default: >> name = chname[i]; >> idx = 0; >> >> >> ...since the current behaviour is inconsistent anyway - you get indices >> for 1 and 3, but "Center"/"LFE" for what should otherwise have been index 2. > > Yeah, certainly something fishy there. > IIRC, the exception was introduced to handle the bass speaker. > But I guess it's already broken in some level, so we need to revisit > the implementation. > > A concern for the change like above is whether it handles correctly > the case where machines has multiple built-in speakers with a single > headphone jack. We need to check this. I'm not exactly sure what you're suspecting here, but the only codec I found in hda-emu with this configuration was stac9221-macbook-pro-21, which is equally broken before and after my patch (before, headphones are controlled by speaker index 1, and after, it's controlled by surround). >> Another way would to just write a pin fix to turn these speakers into >> line outs, which is what they really are anyway. >> >> What do you think? > > This might be a safer option. Having seen that there are several STAC/IDT codecs which have the "[Jack] Speaker" construct, I think it would be nice to fix them all while at it. So I'm leaning towards the multi speaker out rename patch. Attaching that one. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic [-- Attachment #2: 0001-ALSA-hda-fix-control-names-for-multiple-speaker-out-.patch --] [-- Type: text/x-patch, Size: 1345 bytes --] >From 08f1e126e3e53e39f5ab25d27db09b9795e125b8 Mon Sep 17 00:00:00 2001 From: David Henningsson <david.henningsson@canonical.com> Date: Thu, 6 Sep 2012 11:17:58 +0200 Subject: [PATCH] ALSA: hda - fix control names for multiple speaker out on IDT/STAC For multiple speaker outs, the names were previously "Speaker,0", "Speaker,1", "Center"/"LFE", "Speaker,3". This is inconsistent, confusing, and is not picked up correctly by PulseAudio. Instead use "Front", "Surround", "Center"/"LFE", "Side" which is more standard. BugLink: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1046734 Signed-off-by: David Henningsson <david.henningsson@canonical.com> --- sound/pci/hda/patch_sigmatel.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c index 9db3056..c16d0f8 100644 --- a/sound/pci/hda/patch_sigmatel.c +++ b/sound/pci/hda/patch_sigmatel.c @@ -3226,9 +3226,12 @@ static int create_multi_out_ctls(struct hda_codec *codec, int num_outs, idx = i; break; case AUTO_PIN_SPEAKER_OUT: - name = "Speaker"; - idx = i; - break; + if (num_outs <= 1) { + name = "Speaker"; + idx = i; + break; + } + /* Fall through in case of multi speaker outs */ default: name = chname[i]; idx = 0; -- 1.7.9.5 [-- Attachment #3: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Multiple speaker jacks (on an IDT codec) 2012-09-06 9:33 ` David Henningsson @ 2012-09-06 9:44 ` Takashi Iwai 0 siblings, 0 replies; 4+ messages in thread From: Takashi Iwai @ 2012-09-06 9:44 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel@alsa-project.org At Thu, 06 Sep 2012 11:33:22 +0200, David Henningsson wrote: > > On 09/06/2012 08:14 AM, Takashi Iwai wrote: > > At Thu, 06 Sep 2012 00:59:15 +0200, > > David Henningsson wrote: > >> > >> Hi, > >> > >> I'm having a problem with a machine where the parser creates indexed > >> speaker controls. For reference, codecs/idt92hd206-ecs-gf7100pvt shows > >> the same problem. > >> > >> I could use some input on how to fix it in the best way. > >> > >> My first thought was that "[Jack] Speaker" is stupid in itself, and > >> could be fixed like this: > >> > >> diff --git a/sound/pci/hda/hda_auto_parser.c > >> b/sound/pci/hda/hda_auto_parser.c > >> index 4f7d2df..8e32ee5 100644 > >> --- a/sound/pci/hda/hda_auto_parser.c > >> +++ b/sound/pci/hda/hda_auto_parser.c > >> @@ -171,6 +171,10 @@ int snd_hda_parse_pin_defcfg(struct hda_codec *codec, > >> if (conn == AC_JACK_PORT_FIXED) > >> dev = AC_JACK_SPEAKER; > >> } > >> + else if (dev == AC_JACK_SPEAKER) { > >> + if (conn == AC_JACK_PORT_COMPLEX) > >> + dev = AC_JACK_LINE_OUT; > >> + } > >> > >> switch (dev) { > >> case AC_JACK_LINE_OUT: > >> > >> ...but changing generic code is always a bit scary and regression prone > >> (even though I could not think of a good counterexample where that would > >> happen), so not sure if that's the best idea. > > > > Right. I remember that there are more devices declaring the speakers > > in that way. > > I grepped a little through the alsa-infos and found a "[Jack] Speaker at > Int N/A", which is probably an internal speaker rather than a line > out...covering all cases is tricky. > > >> Next attempt is to modify the control creating function for IDT: > >> > >> diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c > >> index 9db3056..c16d0f8 100644 > >> --- a/sound/pci/hda/patch_sigmatel.c > >> +++ b/sound/pci/hda/patch_sigmatel.c > >> @@ -3226,9 +3226,12 @@ static int create_multi_out_ctls(struct hda_codec > >> *codec, int num_outs, > >> idx = i; > >> break; > >> case AUTO_PIN_SPEAKER_OUT: > >> - name = "Speaker"; > >> - idx = i; > >> - break; > >> + if (num_outs <= 1) { > >> + name = "Speaker"; > >> + idx = i; > >> + break; > >> + } > >> + /* Fall through in case of multi speaker > >> outs */ > >> default: > >> name = chname[i]; > >> idx = 0; > >> > >> > >> ...since the current behaviour is inconsistent anyway - you get indices > >> for 1 and 3, but "Center"/"LFE" for what should otherwise have been index 2. > > > > Yeah, certainly something fishy there. > > IIRC, the exception was introduced to handle the bass speaker. > > But I guess it's already broken in some level, so we need to revisit > > the implementation. > > > > A concern for the change like above is whether it handles correctly > > the case where machines has multiple built-in speakers with a single > > headphone jack. We need to check this. > > I'm not exactly sure what you're suspecting here, but the only codec I > found in hda-emu with this configuration was stac9221-macbook-pro-21, > which is equally broken before and after my patch (before, headphones > are controlled by speaker index 1, and after, it's controlled by surround). Not that one. When a system has a headphone, a mic in and a line in jacks, they can be used for 5.1 outputs by retasking. If a machine has 5.1 built-in speakers in addition, the name conflicts. But, I think this isn't the case for patch_sigmatel.c, as it doesn't support well the retasking like above well. (The scenario was seen in Realtek, some Acer Aspire laptops, IIRC.) BTW, the missing headphone control like the case above is handled better in the case of Realtek by calculating the badness for each pin / DAC map. This should be deployed for generic auto-parser. > >> Another way would to just write a pin fix to turn these speakers into > >> line outs, which is what they really are anyway. > >> > >> What do you think? > > > > This might be a safer option. > > Having seen that there are several STAC/IDT codecs which have the > "[Jack] Speaker" construct, I think it would be nice to fix them all > while at it. > > So I'm leaning towards the multi speaker out rename patch. Attaching > that one. OK, I'll take that one. thanks, Takashi ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-09-06 9:44 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-05 22:59 Multiple speaker jacks (on an IDT codec) David Henningsson 2012-09-06 6:14 ` Takashi Iwai 2012-09-06 9:33 ` David Henningsson 2012-09-06 9:44 ` Takashi Iwai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.