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