[PATCH] ALSA: HDA: Fix jack creation for codecs with front and rear Line In

[PATCH] ALSA: HDA: Fix jack creation for codecs with front and rear Line In

[David Henningsson]

If a codec has both a front and a rear Line In, two controls both
named "Line Jack" will be created, which causes parsing to fail.
While a long term solution might be to name the jacks differently,
this extra check is consistent with what is already being done in many
auto-parsers, and will also protect against other cases when two
inputs have the same label.

BugLink: https://bugs.launchpad.net/bugs/923409
Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/hda_jack.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
index d8a35da..9d819c4 100644
--- a/sound/pci/hda/hda_jack.c
+++ b/sound/pci/hda/hda_jack.c
@@ -282,7 +282,8 @@ int snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid,
 EXPORT_SYMBOL_HDA(snd_hda_jack_add_kctl);
 
 static int add_jack_kctl(struct hda_codec *codec, hda_nid_t nid,
-			 const struct auto_pin_cfg *cfg)
+			 const struct auto_pin_cfg *cfg,
+			 char *lastname, int *lastidx)
 {
 	unsigned int def_conf, conn;
 	char name[44];
@@ -298,6 +299,10 @@ static int add_jack_kctl(struct hda_codec *codec, hda_nid_t nid,
 		return 0;
 
 	snd_hda_get_pin_label(codec, nid, cfg, name, sizeof(name), &idx);
+	if (!strcmp(name, lastname) && idx == *lastidx)
+		idx++;
+	strncpy(lastname, name, 44);
+	*lastidx = idx;
 	err = snd_hda_jack_add_kctl(codec, nid, name, idx);
 	if (err < 0)
 		return err;
@@ -311,41 +316,42 @@ int snd_hda_jack_add_kctls(struct hda_codec *codec,
 			   const struct auto_pin_cfg *cfg)
 {
 	const hda_nid_t *p;
-	int i, err;
+	int i, err, lastidx = 0;
+	char lastname[44] = "";
 
 	for (i = 0, p = cfg->line_out_pins; i < cfg->line_outs; i++, p++) {
-		err = add_jack_kctl(codec, *p, cfg);
+		err = add_jack_kctl(codec, *p, cfg, lastname, &lastidx);
 		if (err < 0)
 			return err;
 	}
 	for (i = 0, p = cfg->hp_pins; i < cfg->hp_outs; i++, p++) {
 		if (*p == *cfg->line_out_pins) /* might be duplicated */
 			break;
-		err = add_jack_kctl(codec, *p, cfg);
+		err = add_jack_kctl(codec, *p, cfg, lastname, &lastidx);
 		if (err < 0)
 			return err;
 	}
 	for (i = 0, p = cfg->speaker_pins; i < cfg->speaker_outs; i++, p++) {
 		if (*p == *cfg->line_out_pins) /* might be duplicated */
 			break;
-		err = add_jack_kctl(codec, *p, cfg);
+		err = add_jack_kctl(codec, *p, cfg, lastname, &lastidx);
 		if (err < 0)
 			return err;
 	}
 	for (i = 0; i < cfg->num_inputs; i++) {
-		err = add_jack_kctl(codec, cfg->inputs[i].pin, cfg);
+		err = add_jack_kctl(codec, cfg->inputs[i].pin, cfg, lastname, &lastidx);
 		if (err < 0)
 			return err;
 	}
 	for (i = 0, p = cfg->dig_out_pins; i < cfg->dig_outs; i++, p++) {
-		err = add_jack_kctl(codec, *p, cfg);
+		err = add_jack_kctl(codec, *p, cfg, lastname, &lastidx);
 		if (err < 0)
 			return err;
 	}
-	err = add_jack_kctl(codec, cfg->dig_in_pin, cfg);
+	err = add_jack_kctl(codec, cfg->dig_in_pin, cfg, lastname, &lastidx);
 	if (err < 0)
 		return err;
-	err = add_jack_kctl(codec, cfg->mono_out_pin, cfg);
+	err = add_jack_kctl(codec, cfg->mono_out_pin, cfg, lastname, &lastidx);
 	if (err < 0)
 		return err;
 	return 0;
-- 
1.7.8.3

Re: [PATCH] ALSA: HDA: Fix jack creation for codecs with front and rear Line In

[Raymond Yau]

2012/1/31, David Henningsson <david.henningsson@canonical.com>:
> If a codec has both a front and a rear Line In, two controls both
> named "Line Jack" will be created, which causes parsing to fail.
> While a long term solution might be to name the jacks differently,
> this extra check is consistent with what is already being done in many
> auto-parsers, and will also protect against other cases when two
> inputs have the same label.
>
> BugLink: https://bugs.launchpad.net/bugs/923409
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---

I have doubt since most of ad1988 6stack-dig quirks have two Line In (
one is ext front and the other is int ATAPI and the "Input Source"
controls are broken

Re: [PATCH] ALSA: HDA: Fix jack creation for codecs with front and rear Line In

[David Henningsson]

On 01/31/2012 09:29 AM, Raymond Yau wrote:
> 2012/1/31, David Henningsson<david.henningsson@canonical.com>:
>> If a codec has both a front and a rear Line In, two controls both
>> named "Line Jack" will be created, which causes parsing to fail.
>> While a long term solution might be to name the jacks differently,
>> this extra check is consistent with what is already being done in many
>> auto-parsers, and will also protect against other cases when two
>> inputs have the same label.
>>
>> BugLink: https://bugs.launchpad.net/bugs/923409
>> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
>> ---
>
> I have doubt since most of ad1988 6stack-dig quirks have two Line In (
> one is ext front and the other is int ATAPI and the "Input Source"
> controls are broken

Well, it won't fix any "Input Source" controls, of course. Only jack 
creation later, assuming you have your autocfg struct set up correctly.


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

Re: [PATCH] ALSA: HDA: Fix jack creation for codecs with front and rear Line In

[Raymond Yau]

2012/1/31, David Henningsson <david.henningsson@canonical.com>:
> On 01/31/2012 09:29 AM, Raymond Yau wrote:
>> 2012/1/31, David Henningsson<david.henningsson@canonical.com>:
>>> If a codec has both a front and a rear Line In, two controls both
>>> named "Line Jack" will be created, which causes parsing to fail.
>>> While a long term solution might be to name the jacks differently,
>>> this extra check is consistent with what is already being done in many
>>> auto-parsers, and will also protect against other cases when two
>>> inputs have the same label.
>>>
>>> BugLink: https://bugs.launchpad.net/bugs/923409
>>> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
>>> ---
>>
>> I have doubt since most of ad1988 6stack-dig quirks have two Line In (
>> one is ext front and the other is int ATAPI and the "Input Source"
>> controls are broken
>
> Well, it won't fix any "Input Source" controls, of course. Only jack
> creation later, assuming you have your autocfg struct set up correctly.
>

There is another bug in snd_hda_parse_def_pincfg() when bios assigned
two Line Out at ext front and rear 0x14 and 0x1b

autoconfig: line_outs=1 (0x14/0x0/0x0/0x0/0x0) type:line
   speaker_outs=0 (0x0/0x0/0x0/0x0/0x0)
   hp_outs=0 (0x0/0x0/0x0/0x0/0x0)
   mono: mono_out=0x0
   dig-out=0x1e/0x0
   inputs: Rear Mic=0x18 Front Mic=0x19 Line=0x1a


https://bugtrack.alsa-project.org/alsa-bug/view.php?id=5518

Re: [PATCH] ALSA: HDA: Fix jack creation for codecs with front and rear Line In

[Takashi Iwai]

At Tue, 31 Jan 2012 09:04:15 +0100,
David Henningsson wrote:
> 
> If a codec has both a front and a rear Line In, two controls both
> named "Line Jack" will be created, which causes parsing to fail.
> While a long term solution might be to name the jacks differently,
> this extra check is consistent with what is already being done in many
> auto-parsers, and will also protect against other cases when two
> inputs have the same label.
> 
> BugLink: https://bugs.launchpad.net/bugs/923409
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>

Thanks, applied now.

BTW, any chance to change apport to gather alsa-info.sh output?
It'd be a great help, so that we can check it via hda-emu.


Takashi


> ---
>  sound/pci/hda/hda_jack.c |   24 +++++++++++++++---------
>  1 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
> index d8a35da..9d819c4 100644
> --- a/sound/pci/hda/hda_jack.c
> +++ b/sound/pci/hda/hda_jack.c
> @@ -282,7 +282,8 @@ int snd_hda_jack_add_kctl(struct hda_codec *codec, hda_nid_t nid,
>  EXPORT_SYMBOL_HDA(snd_hda_jack_add_kctl);
>  
>  static int add_jack_kctl(struct hda_codec *codec, hda_nid_t nid,
> -			 const struct auto_pin_cfg *cfg)
> +			 const struct auto_pin_cfg *cfg,
> +			 char *lastname, int *lastidx)
>  {
>  	unsigned int def_conf, conn;
>  	char name[44];
> @@ -298,6 +299,10 @@ static int add_jack_kctl(struct hda_codec *codec, hda_nid_t nid,
>  		return 0;
>  
>  	snd_hda_get_pin_label(codec, nid, cfg, name, sizeof(name), &idx);
> +	if (!strcmp(name, lastname) && idx == *lastidx)
> +		idx++;
> +	strncpy(lastname, name, 44);
> +	*lastidx = idx;
>  	err = snd_hda_jack_add_kctl(codec, nid, name, idx);
>  	if (err < 0)
>  		return err;
> @@ -311,41 +316,42 @@ int snd_hda_jack_add_kctls(struct hda_codec *codec,
>  			   const struct auto_pin_cfg *cfg)
>  {
>  	const hda_nid_t *p;
> -	int i, err;
> +	int i, err, lastidx = 0;
> +	char lastname[44] = "";
>  
>  	for (i = 0, p = cfg->line_out_pins; i < cfg->line_outs; i++, p++) {
> -		err = add_jack_kctl(codec, *p, cfg);
> +		err = add_jack_kctl(codec, *p, cfg, lastname, &lastidx);
>  		if (err < 0)
>  			return err;
>  	}
>  	for (i = 0, p = cfg->hp_pins; i < cfg->hp_outs; i++, p++) {
>  		if (*p == *cfg->line_out_pins) /* might be duplicated */
>  			break;
> -		err = add_jack_kctl(codec, *p, cfg);
> +		err = add_jack_kctl(codec, *p, cfg, lastname, &lastidx);
>  		if (err < 0)
>  			return err;
>  	}
>  	for (i = 0, p = cfg->speaker_pins; i < cfg->speaker_outs; i++, p++) {
>  		if (*p == *cfg->line_out_pins) /* might be duplicated */
>  			break;
> -		err = add_jack_kctl(codec, *p, cfg);
> +		err = add_jack_kctl(codec, *p, cfg, lastname, &lastidx);
>  		if (err < 0)
>  			return err;
>  	}
>  	for (i = 0; i < cfg->num_inputs; i++) {
> -		err = add_jack_kctl(codec, cfg->inputs[i].pin, cfg);
> +		err = add_jack_kctl(codec, cfg->inputs[i].pin, cfg, lastname, &lastidx);
>  		if (err < 0)
>  			return err;
>  	}
>  	for (i = 0, p = cfg->dig_out_pins; i < cfg->dig_outs; i++, p++) {
> -		err = add_jack_kctl(codec, *p, cfg);
> +		err = add_jack_kctl(codec, *p, cfg, lastname, &lastidx);
>  		if (err < 0)
>  			return err;
>  	}
> -	err = add_jack_kctl(codec, cfg->dig_in_pin, cfg);
> +	err = add_jack_kctl(codec, cfg->dig_in_pin, cfg, lastname, &lastidx);
>  	if (err < 0)
>  		return err;
> -	err = add_jack_kctl(codec, cfg->mono_out_pin, cfg);
> +	err = add_jack_kctl(codec, cfg->mono_out_pin, cfg, lastname, &lastidx);
>  	if (err < 0)
>  		return err;
>  	return 0;
> -- 
> 1.7.8.3
> 

Re: [PATCH] ALSA: HDA: Fix jack creation for codecs with front and rear Line In

[David Henningsson]

On 01/31/2012 03:17 PM, Takashi Iwai wrote:
> At Tue, 31 Jan 2012 09:04:15 +0100,
> David Henningsson wrote:
>>
>> If a codec has both a front and a rear Line In, two controls both
>> named "Line Jack" will be created, which causes parsing to fail.
>> While a long term solution might be to name the jacks differently,
>> this extra check is consistent with what is already being done in many
>> auto-parsers, and will also protect against other cases when two
>> inputs have the same label.
>>
>> BugLink: https://bugs.launchpad.net/bugs/923409
>> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
>
> Thanks, applied now.
>
> BTW, any chance to change apport to gather alsa-info.sh output?
> It'd be a great help, so that we can check it via hda-emu.

I have asked for this particular bug reporter to supply alsa-info as 
well, but may I ask you...do you have some kind of regression test 
script, and if so, are they public? Or could it a part of hda-emu?

It would be nice if you could run "make tests" or something from the 
hda-emu source directory, and it would parse all codecs and report error 
if parsing fails (or pm fails, etc).

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

Re: [PATCH] ALSA: HDA: Fix jack creation for codecs with front and rear Line In

[Takashi Iwai]

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

At Tue, 31 Jan 2012 16:28:14 +0100,
David Henningsson wrote:
> 
> On 01/31/2012 03:17 PM, Takashi Iwai wrote:
> > At Tue, 31 Jan 2012 09:04:15 +0100,
> > David Henningsson wrote:
> >>
> >> If a codec has both a front and a rear Line In, two controls both
> >> named "Line Jack" will be created, which causes parsing to fail.
> >> While a long term solution might be to name the jacks differently,
> >> this extra check is consistent with what is already being done in many
> >> auto-parsers, and will also protect against other cases when two
> >> inputs have the same label.
> >>
> >> BugLink: https://bugs.launchpad.net/bugs/923409
> >> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
> >
> > Thanks, applied now.
> >
> > BTW, any chance to change apport to gather alsa-info.sh output?
> > It'd be a great help, so that we can check it via hda-emu.
> 
> I have asked for this particular bug reporter to supply alsa-info as 
> well, but may I ask you...do you have some kind of regression test 
> script, and if so, are they public? Or could it a part of hda-emu?

The checks I often do are scripts like below.  Run for all files
and check the difference before and after the patch.

> It would be nice if you could run "make tests" or something from the 
> hda-emu source directory, and it would parse all codecs and report error 
> if parsing fails (or pm fails, etc).

Yeah, any patch is welcome ;)


Takashi

[-- Attachment #2: do-test --]
[-- Type: application/octet-stream, Size: 228 bytes --]

#!/bin/sh
rm -rf check1
mkdir -p check1
opt=""
case "$1" in
    -*)
	opt="$1"
	shift;;
esac

if [ -z "$1" ]; then
  set codecs/*
fi
for i in $*; do
    f=${i##*/}
    echo q | ./hda-emu $opt -l2 $i > check1/$f 2> /dev/null
done

[-- Attachment #3: do-test2 --]
[-- Type: application/octet-stream, Size: 232 bytes --]

#!/bin/sh
rm -rf check2
mkdir -p check2
opt=""
case "$1" in
    -*)
	opt="$1"
	shift;;
esac

if [ -z "$1" ]; then
  set codecs/*
fi
for i in $*; do
    f=${i##*/}
    echo "d 0 check2/$f
q" | ./hda-emu $opt -l0 $i 2> /dev/null
done

[-- Attachment #4: do-test3 --]
[-- Type: application/octet-stream, Size: 235 bytes --]

#!/bin/sh
rm -rf check3
mkdir -p check3
opt=""
case "$1" in
    -*)
	opt="$1"
	shift;;
esac

if [ -z "$1" ]; then
  set codecs/*
fi
for i in $*; do
    f=${i##*/}
    echo "l
q" | ./hda-emu $opt -l3 -M $i > check3/$f 2> /dev/null
done

[-- Attachment #5: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ALSA: HDA: Fix jack creation for codecs with front and rear Line In

[David Henningsson]

On 01/31/2012 04:40 PM, Takashi Iwai wrote:
> At Tue, 31 Jan 2012 16:28:14 +0100,
> David Henningsson wrote:
>>
>> On 01/31/2012 03:17 PM, Takashi Iwai wrote:
>>> At Tue, 31 Jan 2012 09:04:15 +0100,
>>> David Henningsson wrote:
>>>>
>>>> If a codec has both a front and a rear Line In, two controls both
>>>> named "Line Jack" will be created, which causes parsing to fail.
>>>> While a long term solution might be to name the jacks differently,
>>>> this extra check is consistent with what is already being done in many
>>>> auto-parsers, and will also protect against other cases when two
>>>> inputs have the same label.
>>>>
>>>> BugLink: https://bugs.launchpad.net/bugs/923409
>>>> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
>>>
>>> Thanks, applied now.
>>>
>>> BTW, any chance to change apport to gather alsa-info.sh output?

This has now been provided: 
http://www.alsa-project.org/db/?f=f4a8312f34573a24f60011109e18f0c7371129e0

>>> It'd be a great help, so that we can check it via hda-emu.
>>
>> I have asked for this particular bug reporter to supply alsa-info as
>> well, but may I ask you...do you have some kind of regression test
>> script, and if so, are they public? Or could it a part of hda-emu?
>
> The checks I often do are scripts like below.  Run for all files
> and check the difference before and after the patch.
>
>> It would be nice if you could run "make tests" or something from the
>> hda-emu source directory, and it would parse all codecs and report error
>> if parsing fails (or pm fails, etc).
>
> Yeah, any patch is welcome ;)

Thanks for the small test scripts! It's good to be on the same page. 
Hopefully I get around some day to write a "make tests", but not right 
now unfortunately.

I quickly ran test2 and noticed that
  - We have quite a lot of work to do before all errors are fixed!
  - The error I got from hda-emu for the codec above, 'Control element 
Line Jack:0 already exists!' was already present for one of the codecs.

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

Re: [PATCH] ALSA: HDA: Fix jack creation for codecs with front and rear Line In

[Takashi Iwai]

At Wed, 01 Feb 2012 09:19:25 +0100,
David Henningsson wrote:
> 
> On 01/31/2012 04:40 PM, Takashi Iwai wrote:
> > At Tue, 31 Jan 2012 16:28:14 +0100,
> > David Henningsson wrote:
> >>
> >> On 01/31/2012 03:17 PM, Takashi Iwai wrote:
> >>> At Tue, 31 Jan 2012 09:04:15 +0100,
> >>> David Henningsson wrote:
> >>>>
> >>>> If a codec has both a front and a rear Line In, two controls both
> >>>> named "Line Jack" will be created, which causes parsing to fail.
> >>>> While a long term solution might be to name the jacks differently,
> >>>> this extra check is consistent with what is already being done in many
> >>>> auto-parsers, and will also protect against other cases when two
> >>>> inputs have the same label.
> >>>>
> >>>> BugLink: https://bugs.launchpad.net/bugs/923409
> >>>> Signed-off-by: David Henningsson<david.henningsson@canonical.com>
> >>>
> >>> Thanks, applied now.
> >>>
> >>> BTW, any chance to change apport to gather alsa-info.sh output?
> 
> This has now been provided: 
> http://www.alsa-project.org/db/?f=f4a8312f34573a24f60011109e18f0c7371129e0
> 
> >>> It'd be a great help, so that we can check it via hda-emu.
> >>
> >> I have asked for this particular bug reporter to supply alsa-info as
> >> well, but may I ask you...do you have some kind of regression test
> >> script, and if so, are they public? Or could it a part of hda-emu?
> >
> > The checks I often do are scripts like below.  Run for all files
> > and check the difference before and after the patch.
> >
> >> It would be nice if you could run "make tests" or something from the
> >> hda-emu source directory, and it would parse all codecs and report error
> >> if parsing fails (or pm fails, etc).
> >
> > Yeah, any patch is welcome ;)
> 
> Thanks for the small test scripts! It's good to be on the same page. 
> Hopefully I get around some day to write a "make tests", but not right 
> now unfortunately.
> 
> I quickly ran test2 and noticed that
>   - We have quite a lot of work to do before all errors are fixed!

Many errors come from the static quirks and they are harmless.
You can pass -mauto to test scripts for checking the auto-parser.

>   - The error I got from hda-emu for the codec above, 'Control element 
> Line Jack:0 already exists!' was already present for one of the codecs.

What a shame...


thanks,

Takashi