Re: [alsa-devel] [PATCH] Re: iec958 switch uneffective while playing ac3 stream
At Thu, 05 Apr 2007 10:47:52 +0200, Dominique Dumont wrote:
Takashi Iwai tiwai@suse.de writes:
The patch is a bit dangerous because it unconditionally resets the DIGI_CONVERT verb for each widget.
Agreed. We could also test the NID before switching on and off DigEn in the DIGI_CONVERT.
OTOH the switch would occur for *all* HDA chips which may not be what you want.
I'm not 100% sure that this particular procedure is necessary for all codecs. So, I prefer having it in the hda-codec part rather in hda-intel part.
The below is my modified version. It doesn't turn off before the controller side reset as you did. So, if my version doesn't work, it implies that the SPDIF reset has to be done before the controller reset. Please give it a try.
It works also ! I've tested AC3 and 44KHz. I cannot test 96KHz or 196Khz (my amp is too old).
Good to hear.
- /* turn off SPDIF once; otherwise the IEC958 bits won't be updated */
I had more the impression that the SPDIF stream is messed up if you change the stream format while the digital converter is working...
- if (codec->spdif_ctls & 1)
Shouldn't you use AC_DIG1_ENABLE for a proper patch ?
if (codec->spdif_ctls & AC_DIG1_ENABLE)
snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_DIGI_CONVERT_1,codec->spdif_ctls & 0xfe);likewise: codec->spdif_ctls & 0xff & ~AC_DIG1_ENABLE ?
- snd_hda_codec_setup_stream(codec, nid, stream_tag, 0, format);
- /* turn on again (if needed) */
- if (codec->spdif_ctls & 1)
if (codec->spdif_ctls & AC_DIG1_ENABLE) ?
Yeah, that'd be better. I'll fix and commit to the HG tree later.
thanks,
Takashi
At Thu, 05 Apr 2007 12:09:22 +0200, I wrote:
Yeah, that'd be better. I'll fix and commit to the HG tree later.
OK, now it's on HG tree.
While we're at it... I did a quick hack to add a new mixer element to choose the SPDIF playback route, either "Analog Duplicate" or "Exclusive PCM". (On AD codecs, it has more items for analog loopbacks.) This will solve one remaining problem, "SPDIF is blocked by analog open".
Of course, this has another problem that user may choose an incorrect value accidentally, but I come to believe it's better than a module option.
Takashi
diff -r 2c71d018644e pci/hda/hda_codec.c --- a/pci/hda/hda_codec.c Thu Apr 05 14:51:48 2007 +0200 +++ b/pci/hda/hda_codec.c Thu Apr 05 15:26:47 2007 +0200 @@ -1968,7 +1968,8 @@ int snd_hda_multi_out_analog_prepare(str int i;
mutex_lock(&codec->spdif_mutex); - if (mout->dig_out_nid && mout->dig_out_used != HDA_DIG_EXCLUSIVE) { + if (mout->dig_out_nid && !mout->dig_out_exclusive && + mout->dig_out_used != HDA_DIG_EXCLUSIVE) { if (chs == 2 && snd_hda_is_supported_format(codec, mout->dig_out_nid, format) && ! (codec->spdif_status & IEC958_AES0_NONAUDIO)) { diff -r 2c71d018644e pci/hda/hda_local.h --- a/pci/hda/hda_local.h Thu Apr 05 14:51:48 2007 +0200 +++ b/pci/hda/hda_local.h Thu Apr 05 14:56:22 2007 +0200 @@ -144,6 +144,7 @@ struct hda_multi_out { hda_nid_t dig_out_nid; /* digital out audio widget */ int max_channels; /* currently supported analog channels */ int dig_out_used; /* current usage of digital out (HDA_DIG_XXX) */ + int dig_out_exclusive; };
int snd_hda_multi_out_dig_open(struct hda_codec *codec, struct hda_multi_out *mout); diff -r 2c71d018644e pci/hda/patch_analog.c --- a/pci/hda/patch_analog.c Thu Apr 05 14:51:48 2007 +0200 +++ b/pci/hda/patch_analog.c Thu Apr 05 15:17:01 2007 +0200 @@ -959,20 +959,25 @@ static struct hda_input_mux ad1983_captu /* * SPDIF playback route */ -static int ad1983_spdif_route_info(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo) -{ - static char *texts[] = { "PCM", "ADC" }; +static int ad1983_spdif_route_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + static char *texts[] = { + "Analog Duplicate", "Exclusive PCM", "Analog Loopback" + };
uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; uinfo->count = 1; - uinfo->value.enumerated.items = 2; - if (uinfo->value.enumerated.item > 1) - uinfo->value.enumerated.item = 1; - strcpy(uinfo->value.enumerated.name, texts[uinfo->value.enumerated.item]); + uinfo->value.enumerated.items = 3; + if (uinfo->value.enumerated.item > 2) + uinfo->value.enumerated.item = 2; + strcpy(uinfo->value.enumerated.name, + texts[uinfo->value.enumerated.item]); return 0; }
-static int ad1983_spdif_route_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) +static int ad1983_spdif_route_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) { struct hda_codec *codec = snd_kcontrol_chip(kcontrol); struct ad198x_spec *spec = codec->spec; @@ -981,18 +986,21 @@ static int ad1983_spdif_route_get(struct return 0; }
-static int ad1983_spdif_route_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) +static int ad1983_spdif_route_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) { struct hda_codec *codec = snd_kcontrol_chip(kcontrol); struct ad198x_spec *spec = codec->spec;
- if (spec->spdif_route != ucontrol->value.enumerated.item[0]) { - spec->spdif_route = ucontrol->value.enumerated.item[0]; - snd_hda_codec_write(codec, spec->multiout.dig_out_nid, 0, - AC_VERB_SET_CONNECT_SEL, spec->spdif_route); - return 1; - } - return 0; + if (spec->spdif_route == ucontrol->value.enumerated.item[0] && + !codec->in_resume) + return 0; + spec->spdif_route = ucontrol->value.enumerated.item[0]; + spec->multiout.dig_out_exclusive = (spec->spdif_route == 1); + snd_hda_codec_write(codec, spec->multiout.dig_out_nid, 0, + AC_VERB_SET_CONNECT_SEL, + (spec->spdif_route > 1) ? 1 : 0); + return 1; }
static struct snd_kcontrol_new ad1983_mixers[] = { @@ -1020,6 +1028,10 @@ static struct snd_kcontrol_new ad1983_mi .get = ad198x_mux_enum_get, .put = ad198x_mux_enum_put, }, + { } /* end */ +}; + +static struct snd_kcontrol_new ad1983_spdif_mixers[] = { { .iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = SNDRV_CTL_NAME_IEC958("",PLAYBACK,NONE) "Source", @@ -1089,8 +1101,9 @@ static int patch_ad1983(struct hda_codec spec->adc_nids = ad1983_adc_nids; spec->capsrc_nids = ad1983_capsrc_nids; spec->input_mux = &ad1983_capture_source; - spec->num_mixers = 1; + spec->num_mixers = 2; spec->mixers[0] = ad1983_mixers; + spec->mixers[1] = ad1983_spdif_mixers; spec->num_init_verbs = 1; spec->init_verbs[0] = ad1983_init_verbs; spec->spdif_route = 0; @@ -1158,14 +1171,6 @@ static struct snd_kcontrol_new ad1981_mi .info = ad198x_mux_enum_info, .get = ad198x_mux_enum_get, .put = ad198x_mux_enum_put, - }, - /* identical with AD1983 */ - { - .iface = SNDRV_CTL_ELEM_IFACE_MIXER, - .name = SNDRV_CTL_NAME_IEC958("",PLAYBACK,NONE) "Source", - .info = ad1983_spdif_route_info, - .get = ad1983_spdif_route_get, - .put = ad1983_spdif_route_put, }, { } /* end */ }; @@ -1423,14 +1428,6 @@ static struct snd_kcontrol_new ad1981_th .get = ad198x_mux_enum_get, .put = ad198x_mux_enum_put, }, - /* identical with AD1983 */ - { - .iface = SNDRV_CTL_ELEM_IFACE_MIXER, - .name = SNDRV_CTL_NAME_IEC958("",PLAYBACK,NONE) "Source", - .info = ad1983_spdif_route_info, - .get = ad1983_spdif_route_get, - .put = ad1983_spdif_route_put, - }, { } /* end */ };
@@ -1529,6 +1526,9 @@ static int patch_ad1981(struct hda_codec codec->patch_ops.unsol_event = ad1981_hp_unsol_event; break; } + if (spec->multiout.dig_out_nid) + /* identical with AD1983 */ + spec->mixers[spec->num_mixers++] = ad1983_spdif_mixers; return 0; }
@@ -1886,14 +1886,16 @@ static int ad1988_spdif_playback_source_ struct snd_ctl_elem_info *uinfo) { static char *texts[] = { - "PCM", "ADC1", "ADC2", "ADC3" + "Analog Duplicate", "Exclusive PCM", + "ADC1", "ADC2", "ADC3" }; uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; uinfo->count = 1; - uinfo->value.enumerated.items = 4; - if (uinfo->value.enumerated.item >= 4) - uinfo->value.enumerated.item = 3; - strcpy(uinfo->value.enumerated.name, texts[uinfo->value.enumerated.item]); + uinfo->value.enumerated.items = 5; + if (uinfo->value.enumerated.item >= 5) + uinfo->value.enumerated.item = 4; + strcpy(uinfo->value.enumerated.name, + texts[uinfo->value.enumerated.item]); return 0; }
@@ -1901,17 +1903,9 @@ static int ad1988_spdif_playback_source_ struct snd_ctl_elem_value *ucontrol) { struct hda_codec *codec = snd_kcontrol_chip(kcontrol); - unsigned int sel; - - sel = snd_hda_codec_read(codec, 0x02, 0, AC_VERB_GET_CONNECT_SEL, 0); - if (sel > 0) { - sel = snd_hda_codec_read(codec, 0x0b, 0, AC_VERB_GET_CONNECT_SEL, 0); - if (sel <= 3) - sel++; - else - sel = 0; - } - ucontrol->value.enumerated.item[0] = sel; + struct ad198x_spec *spec = codec->spec; + + ucontrol->value.enumerated.item[0] = spec->spdif_route; return 0; }
@@ -1919,25 +1913,23 @@ static int ad1988_spdif_playback_source_ struct snd_ctl_elem_value *ucontrol) { struct hda_codec *codec = snd_kcontrol_chip(kcontrol); + struct ad198x_spec *spec = codec->spec; unsigned int sel; - int change; - - sel = snd_hda_codec_read(codec, 0x02, 0, AC_VERB_GET_CONNECT_SEL, 0); - if (! ucontrol->value.enumerated.item[0]) { - change = sel != 0; - if (change) - snd_hda_codec_write(codec, 0x02, 0, AC_VERB_SET_CONNECT_SEL, 0); + + if (ucontrol->value.enumerated.item[0] != spec->spdif_route && + !codec->in_resume) + return 0; + + spec->spdif_route = ucontrol->value.enumerated.item[0]; + if (spec->spdif_route < 2) { + spec->multiout.dig_out_exclusive = spec->spdif_route; + snd_hda_codec_write(codec, 0x02, 0, AC_VERB_SET_CONNECT_SEL, 0); } else { - change = sel == 0; - if (change) - snd_hda_codec_write(codec, 0x02, 0, AC_VERB_SET_CONNECT_SEL, 1); - sel = snd_hda_codec_read(codec, 0x0b, 0, AC_VERB_GET_CONNECT_SEL, 0) + 1; - change |= sel == ucontrol->value.enumerated.item[0]; - if (change) - snd_hda_codec_write(codec, 0x02, 0, AC_VERB_SET_CONNECT_SEL, - ucontrol->value.enumerated.item[0] - 1); + snd_hda_codec_write(codec, 0x02, 0, AC_VERB_SET_CONNECT_SEL, 1); + snd_hda_codec_read(codec, 0x0b, 0, AC_VERB_GET_CONNECT_SEL, + spec->spdif_route - 2); } - return change; + return 1; }
static struct snd_kcontrol_new ad1988_spdif_out_mixers[] = { diff -r 2c71d018644e pci/hda/patch_realtek.c --- a/pci/hda/patch_realtek.c Thu Apr 05 14:51:48 2007 +0200 +++ b/pci/hda/patch_realtek.c Thu Apr 05 15:29:03 2007 +0200 @@ -1098,6 +1098,62 @@ static struct snd_kcontrol_new alc880_un { } /* end */ };
+/* additional SPDIF output controls */ +static int spdif_out_route_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + static char *texts[] = { + "Analog Duplicate", "Exclusive PCM" + }; + + uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; + uinfo->count = 1; + uinfo->value.enumerated.items = 2; + if (uinfo->value.enumerated.item > 1) + uinfo->value.enumerated.item = 1; + strcpy(uinfo->value.enumerated.name, + texts[uinfo->value.enumerated.item]); + return 0; +} + +static int spdif_out_route_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); + struct alc_spec *spec = codec->spec; + + ucontrol->value.enumerated.item[0] = spec->multiout.dig_out_exclusive; + return 0; +} + +static int spdif_out_route_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); + struct alc_spec *spec = codec->spec; + int change; + + mutex_lock(&codec->spdif_mutex); + change = (ucontrol->value.enumerated.item[0] != + spec->multiout.dig_out_exclusive); + if (change || codec->in_resume) + spec->multiout.dig_out_exclusive = + ucontrol->value.enumerated.item[0]; + mutex_unlock(&codec->spdif_mutex); + return change; +} + +static struct snd_kcontrol_new alc_dig_mixes[] = { + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = SNDRV_CTL_NAME_IEC958("",PLAYBACK,NONE) "Source", + .info = spdif_out_route_info, + .get = spdif_out_route_get, + .put = spdif_out_route_put, + }, + { } /* end */ +}; + /* * build control elements */ @@ -1116,6 +1172,9 @@ static int alc_build_controls(struct hda if (spec->multiout.dig_out_nid) { err = snd_hda_create_spdif_out_ctls(codec, spec->multiout.dig_out_nid); + if (err < 0) + return err; + err = snd_hda_add_new_ctls(codec, alc_dig_mixes); if (err < 0) return err; } diff -r 2c71d018644e pci/hda/patch_via.c --- a/pci/hda/patch_via.c Thu Apr 05 14:51:48 2007 +0200 +++ b/pci/hda/patch_via.c Thu Apr 05 15:30:24 2007 +0200 @@ -456,6 +456,62 @@ static struct hda_pcm_stream vt1708_pcm_ .channels_max = 2, };
+/* additional SPDIF output controls */ +static int spdif_out_route_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + static char *texts[] = { + "Analog Duplicate", "Exclusive PCM" + }; + + uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED; + uinfo->count = 1; + uinfo->value.enumerated.items = 2; + if (uinfo->value.enumerated.item > 1) + uinfo->value.enumerated.item = 1; + strcpy(uinfo->value.enumerated.name, + texts[uinfo->value.enumerated.item]); + return 0; +} + +static int spdif_out_route_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); + struct via_spec *spec = codec->spec; + + ucontrol->value.enumerated.item[0] = spec->multiout.dig_out_exclusive; + return 0; +} + +static int spdif_out_route_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); + struct via_spec *spec = codec->spec; + int change; + + mutex_lock(&codec->spdif_mutex); + change = (ucontrol->value.enumerated.item[0] != + spec->multiout.dig_out_exclusive); + if (change || codec->in_resume) + spec->multiout.dig_out_exclusive = + ucontrol->value.enumerated.item[0]; + mutex_unlock(&codec->spdif_mutex); + return change; +} + +static struct snd_kcontrol_new via_dig_mixes[] = { + { + .iface = SNDRV_CTL_ELEM_IFACE_MIXER, + .name = SNDRV_CTL_NAME_IEC958("",PLAYBACK,NONE) "Source", + .info = spdif_out_route_info, + .get = spdif_out_route_get, + .put = spdif_out_route_put, + }, + { } /* end */ +}; + static int via_build_controls(struct hda_codec *codec) { struct via_spec *spec = codec->spec; @@ -471,6 +527,9 @@ static int via_build_controls(struct hda if (spec->multiout.dig_out_nid) { err = snd_hda_create_spdif_out_ctls(codec, spec->multiout.dig_out_nid); + if (err < 0) + return err; + err = snd_hda_add_new_ctls(codec, via_dig_mixes); if (err < 0) return err; }
Takashi Iwai tiwai@suse.de writes:
OK, now it's on HG tree.
Thanks.
While we're at it... I did a quick hack to add a new mixer element to choose the SPDIF playback route, either "Analog Duplicate" or "Exclusive PCM". (On AD codecs, it has more items for analog
Hmm, I hope you won't think I'm nitpicking, but this term "Exclusive PCM" might be misleading as AC3 or DTS streams are not PCM...
May be "Exlusive Digital" would be better... Does any native english speaker have a better suggestion ?
loopbacks.) This will solve one remaining problem, "SPDIF is blocked by analog open".
That's great news ! Thanks a bunch.
Of course, this has another problem that user may choose an incorrect value accidentally, but I come to believe it's better than a module option.
Yes. In order to allow the user to spot unfortunate stteing, it's important to choose carefully label for mixer switches.
Cheers
On 4/6/07, Dominique Dumont domi.dumont@free.fr wrote:
Takashi Iwai tiwai@suse.de writes:
OK, now it's on HG tree.
Thanks.
While we're at it... I did a quick hack to add a new mixer element to choose the SPDIF playback route, either "Analog Duplicate" or "Exclusive PCM". (On AD codecs, it has more items for analog
Hmm, I hope you won't think I'm nitpicking, but this term "Exclusive PCM" might be misleading as AC3 or DTS streams are not PCM...
May be "Exlusive Digital" would be better... Does any native english speaker have a better suggestion ?
How about Digital Mode: { Independent | Shared }? Similar to the existing controls for surround jack mode.
Lee
At Fri, 6 Apr 2007 15:44:06 -0400, Lee Revell wrote:
On 4/6/07, Dominique Dumont domi.dumont@free.fr wrote:
Takashi Iwai tiwai@suse.de writes:
OK, now it's on HG tree.
Thanks.
While we're at it... I did a quick hack to add a new mixer element to choose the SPDIF playback route, either "Analog Duplicate" or "Exclusive PCM". (On AD codecs, it has more items for analog
Hmm, I hope you won't think I'm nitpicking, but this term "Exclusive PCM" might be misleading as AC3 or DTS streams are not PCM...
May be "Exlusive Digital" would be better... Does any native english speaker have a better suggestion ?
How about Digital Mode: { Independent | Shared }? Similar to the existing controls for surround jack mode.
I don't care much about wording, so I'd like to ask you guys which one is better. In general, we have the following modes for SPDIF output:
1. "Exclusive" / "Independent" mode You'll have a dedicated SPDIF stream, suitable for AC3/DTS output.
2. "Duplicate" / "Shared" mode The normal analog output is copied to SPDIF output.
3. (codec-dependent) "Analog Loopback" mode The analog line-in / CD / mic-in (and mixer) is loopbacked to SPDIF output
4. (codec-dependent) "SPDIF Loopback" mode The SPDIF-input is loopbacked to SPDIF output
Takashi
Takashi Iwai tiwai@suse.de writes:
I don't care much about wording, so I'd like to ask you guys which one is better. In general, we have the following modes for SPDIF output:
- "Exclusive" / "Independent" mode
You'll have a dedicated SPDIF stream, suitable for AC3/DTS output.
- "Duplicate" / "Shared" mode
The normal analog output is copied to SPDIF output.
- (codec-dependent) "Analog Loopback" mode
The analog line-in / CD / mic-in (and mixer) is loopbacked to SPDIF output
- (codec-dependent) "SPDIF Loopback" mode
The SPDIF-input is loopbacked to SPDIF output
Since we can have more that one word in the label, we can have a more consistent set :
1. "SPDIF-out only" 2. "Analog-out copy" 3. "Analog-in loopback" 4. "SPDIF-in loopback"
HTH
Takashi Iwai tiwai@suse.de writes:
OK, now it's on HG tree.
Could you also include the following patch that trace verbs sent to the codec and the responses?
These traces were invaluable to figure out what was going on when I hunted for the spdif bug. I believe it will also be valuable if people have to look for other hda bugs.
Cheers
Signed-off by: Dominique Dumont domi.dumont@free.fr
diff -r 42321871a7dc pci/hda/hda_codec.c --- a/pci/hda/hda_codec.c Thu Apr 05 17:08:57 2007 +0200 +++ b/pci/hda/hda_codec.c Mon Apr 09 14:17:55 2007 +0200 @@ -87,6 +87,9 @@ unsigned int snd_hda_codec_read(struct h else res = (unsigned int)-1; mutex_unlock(&codec->bus->cmd_mutex); + snd_printdd(KERN_INFO + "hda_intel codec read: nid %x direct %x verb %x parm %x -> %x\n", + nid, direct, verb, parm, res) ; return res; }
@@ -108,6 +111,9 @@ int snd_hda_codec_write(struct hda_codec unsigned int verb, unsigned int parm) { int err; + snd_printdd(KERN_INFO + "hda_intel codec write: nid %x direct %x verb %x parm %x\n", + nid, direct, verb, parm) ; mutex_lock(&codec->bus->cmd_mutex); err = codec->bus->ops.command(codec, nid, direct, verb, parm); mutex_unlock(&codec->bus->cmd_mutex);
At Mon, 09 Apr 2007 14:53:31 +0200, Dominique Dumont wrote:
Takashi Iwai tiwai@suse.de writes:
OK, now it's on HG tree.
Could you also include the following patch that trace verbs sent to the codec and the responses?
These traces were invaluable to figure out what was going on when I hunted for the spdif bug. I believe it will also be valuable if people have to look for other hda bugs.
I think snd_printdd() isn't appropriate for this because you cannot choose it at runtime but only at build time. My preference is to add a debug module option (with ifdef CONFIG_SND_DEBUG) and check the option value there.
thanks,
Takashi
Cheers
Signed-off by: Dominique Dumont domi.dumont@free.fr
diff -r 42321871a7dc pci/hda/hda_codec.c --- a/pci/hda/hda_codec.c Thu Apr 05 17:08:57 2007 +0200 +++ b/pci/hda/hda_codec.c Mon Apr 09 14:17:55 2007 +0200 @@ -87,6 +87,9 @@ unsigned int snd_hda_codec_read(struct h else res = (unsigned int)-1; mutex_unlock(&codec->bus->cmd_mutex);
- snd_printdd(KERN_INFO
"hda_intel codec read: nid %x direct %x verb %x parm %x -> %x\n",nid, direct, verb, parm, res) ;}
@@ -108,6 +111,9 @@ int snd_hda_codec_write(struct hda_codec unsigned int verb, unsigned int parm) { int err;
- snd_printdd(KERN_INFO
"hda_intel codec write: nid %x direct %x verb %x parm %x\n",nid, direct, verb, parm) ;
Takashi Iwai tiwai@suse.de writes:
These traces were invaluable to figure out what was going on when I hunted for the spdif bug. I believe it will also be valuable if people have to look for other hda bugs.
I think snd_printdd() isn't appropriate for this because you cannot choose it at runtime but only at build time. My preference is to add a debug module option (with ifdef CONFIG_SND_DEBUG) and check the option value there.
ok. I will re-work this patch.
Cheers
Takashi Iwai tiwai@suse.de writes:
These traces were invaluable to figure out what was going on when I hunted for the spdif bug. I believe it will also be valuable if people have to look for other hda bugs.
I think snd_printdd() isn't appropriate for this because you cannot choose it at runtime but only at build time. My preference is to add a debug module option (with ifdef CONFIG_SND_DEBUG) and check the option value there.
Something like this ?
Signed off by: Dominique Dumont domi.dumont@free.fr
diff -r 42321871a7dc pci/hda/hda_codec.c --- a/pci/hda/hda_codec.c Thu Apr 05 17:08:57 2007 +0200 +++ b/pci/hda/hda_codec.c Tue Apr 10 19:52:01 2007 +0200 @@ -38,6 +38,12 @@ MODULE_DESCRIPTION("Universal interface MODULE_DESCRIPTION("Universal interface for High Definition Audio Codec"); MODULE_LICENSE("GPL");
+static int trace_codec_commands = 0 ; + +#ifdef CONFIG_SND_DEBUG +module_param(trace_codec_commands, bool, 0444); +MODULE_PARM_DESC(trace_codec_commands, "Trace communication with hda codec (for debugging only)."); +#endif /* CONFIG_SND_DEBUG */
/* * vendor / preset table @@ -87,6 +93,12 @@ unsigned int snd_hda_codec_read(struct h else res = (unsigned int)-1; mutex_unlock(&codec->bus->cmd_mutex); + + if (trace_codec_commands) + snd_printk(KERN_INFO + "hda_intel codec read: nid %x direct %x verb %x parm %x -> %x\n", + nid, direct, verb, parm, res) ; + return res; }
@@ -108,6 +120,12 @@ int snd_hda_codec_write(struct hda_codec unsigned int verb, unsigned int parm) { int err; + + if (trace_codec_commands) + snd_printk(KERN_INFO + "hda_intel codec write: nid %x direct %x verb %x parm %x\n", + nid, direct, verb, parm) ; + mutex_lock(&codec->bus->cmd_mutex); err = codec->bus->ops.command(codec, nid, direct, verb, parm); mutex_unlock(&codec->bus->cmd_mutex);
At Tue, 10 Apr 2007 20:00:29 +0200, Dominique Dumont wrote:
Takashi Iwai tiwai@suse.de writes:
These traces were invaluable to figure out what was going on when I hunted for the spdif bug. I believe it will also be valuable if people have to look for other hda bugs.
I think snd_printdd() isn't appropriate for this because you cannot choose it at runtime but only at build time. My preference is to add a debug module option (with ifdef CONFIG_SND_DEBUG) and check the option value there.
Something like this ?
Signed off by: Dominique Dumont domi.dumont@free.fr
I prefer having options in snd-hda-intel side. Also, setting permission 0644 makes it possible to turn on/off the debug option dynamically, which is pretty useful.
A test patch is below. How about it?
Takashi
diff -r b31803586d4f pci/hda/hda_intel.c --- a/pci/hda/hda_intel.c Thu Apr 12 13:08:09 2007 +0200 +++ b/pci/hda/hda_intel.c Thu Apr 12 14:24:47 2007 +0200 @@ -72,6 +72,14 @@ module_param(enable_msi, int, 0); module_param(enable_msi, int, 0); MODULE_PARM_DESC(enable_msi, "Enable Message Signaled Interrupt (MSI)");
+#ifdef CONFIG_SND_DEBUG +static int trace_cmd; +module_param(trace_cmd, bool, 0644); +MODULE_PARM_DESC(trace_cmd, + "Trace communication with hda codecs (for debugging only)."); +#else +#define trace_cmd 0 +#endif
/* just for backward compatibility */ static int enable; @@ -637,6 +645,9 @@ static int azx_send_cmd(struct hda_codec unsigned int para) { struct azx *chip = codec->bus->private_data; + if (trace_cmd) + snd_printd("codec cmd: nid %02x direct %x verb %04x parm %02x\n", + nid, direct, verb, para); if (chip->single_cmd) return azx_single_send_cmd(codec, nid, direct, verb, para); else @@ -647,10 +658,15 @@ static unsigned int azx_get_response(str static unsigned int azx_get_response(struct hda_codec *codec) { struct azx *chip = codec->bus->private_data; + unsigned int res; + if (chip->single_cmd) - return azx_single_get_response(codec); + res = azx_single_get_response(codec); else - return azx_rirb_get_response(codec); + res = azx_rirb_get_response(codec); + if (trace_cmd) + snd_printd("codec res: -> %x\n", res); + return res; }
Takashi Iwai tiwai@suse.de writes:
I prefer having options in snd-hda-intel side. Also, setting permission 0644 makes it possible to turn on/off the debug option dynamically, which is pretty useful.
Agreed.
A test patch is below. How about it?
Almost fine with me. The option name is a bit terse. A newbie (or an alsa hacker wanabee like me) might not know which command is involved just by reading the parameter name (trace_cmd).
I'd rather have a slightly longer name like trace_codec_cmd
Thanks
At Thu, 12 Apr 2007 20:14:42 +0200, Dominique Dumont wrote:
Takashi Iwai tiwai@suse.de writes:
I prefer having options in snd-hda-intel side. Also, setting permission 0644 makes it possible to turn on/off the debug option dynamically, which is pretty useful.
Agreed.
A test patch is below. How about it?
Almost fine with me. The option name is a bit terse. A newbie (or an alsa hacker wanabee like me) might not know which command is involved just by reading the parameter name (trace_cmd).
I'd rather have a slightly longer name like trace_codec_cmd
It sounds too long to me :)
Takashi
Takashi Iwai tiwai@suse.de writes:
Almost fine with me. The option name is a bit terse. A newbie (or an alsa hacker wanabee like me) might not know which command is involved just by reading the parameter name (trace_cmd).
I'd rather have a slightly longer name like trace_codec_cmd
It sounds too long to me :)
At the risk of sounding harsh, we're talking about user interface, so our *personnal* preferences matter less than average user needs.
So, what would typical user prefer ? (knowing that the codec communication is a very important part of the driver's behaviour)
Short version:
$ cat /etc/modprobe.d/alsa options snd-hda-intel trace_cmd=1 model=6stack-dig
or longer version :
$ cat /etc/modprobe.d/alsa options snd-hda-intel trace_codec_cmd=1 model=6stack-dig
Last but not least, a longer name may even save some typing: you may have less user questions to answer to... ;-)
That said, alsa is still your baby, so it's your call now.
Cheers
At Thu, 05 Apr 2007 15:36:34 +0200, I wrote:
At Thu, 05 Apr 2007 12:09:22 +0200, I wrote:
Yeah, that'd be better. I'll fix and commit to the HG tree later.
OK, now it's on HG tree.
While we're at it... I did a quick hack to add a new mixer element to choose the SPDIF playback route, either "Analog Duplicate" or "Exclusive PCM". (On AD codecs, it has more items for analog loopbacks.) This will solve one remaining problem, "SPDIF is blocked by analog open".
Of course, this has another problem that user may choose an incorrect value accidentally, but I come to believe it's better than a module option.
After reconsideration, I found it much easier to change the driver to allow access to the dedicated SPDIF stream even while analog dup mode. When the SPDIF stream is opened, the SPDIF is forced to reset and then the stream is reserved for the exclusive access until it's closed.
The below is a test patch. Now you see it's so simple...
Can anyone test it?
thanks,
Takashi
diff -r 5ef5aabbbfe8 pci/hda/hda_codec.c --- a/pci/hda/hda_codec.c Tue Apr 10 11:39:58 2007 +0200 +++ b/pci/hda/hda_codec.c Wed Apr 11 14:18:21 2007 +0200 @@ -1911,10 +1911,9 @@ int snd_hda_multi_out_dig_open(struct hd int snd_hda_multi_out_dig_open(struct hda_codec *codec, struct hda_multi_out *mout) { mutex_lock(&codec->spdif_mutex); - if (mout->dig_out_used) { - mutex_unlock(&codec->spdif_mutex); - return -EBUSY; /* already being used */ - } + if (mout->dig_out_used == HDA_DIG_ANALOG_DUP) + /* already opened as analog dup; reset it once */ + snd_hda_codec_setup_stream(codec, mout->dig_out_nid, 0, 0, 0); mout->dig_out_used = HDA_DIG_EXCLUSIVE; mutex_unlock(&codec->spdif_mutex); return 0;
Takashi Iwai tiwai@suse.de writes:
After reconsideration, I found it much easier to change the driver to allow access to the dedicated SPDIF stream even while analog dup mode. When the SPDIF stream is opened, the SPDIF is forced to reset and then the stream is reserved for the exclusive access until it's closed.
That's fine which me.
The below is a test patch. Now you see it's so simple...
Can anyone test it?
I'll do the test once I get back home.
Thanks
Takashi Iwai tiwai@suse.de writes:
After reconsideration, I found it much easier to change the driver to allow access to the dedicated SPDIF stream even while analog dup mode. When the SPDIF stream is opened, the SPDIF is forced to reset and then the stream is reserved for the exclusive access until it's closed.
The below is a test patch. Now you see it's so simple...
Can anyone test it?
Done. It does not work:
$ ac3dec -C -c 1 dh122.ac3 Using PCM device 'plug:iec958:{AES0 0x2 AES1 0x82 AES2 0x0 AES3 0x2 CARD 1}' snd_pcm_open: Device or resource busy Output open failed
I can't find a trace to explain why in kern.log or in syslog.
If necessary, I'll dig further this week-end.
Cheers
At Thu, 12 Apr 2007 20:25:25 +0200, Dominique Dumont wrote:
Takashi Iwai tiwai@suse.de writes:
After reconsideration, I found it much easier to change the driver to allow access to the dedicated SPDIF stream even while analog dup mode. When the SPDIF stream is opened, the SPDIF is forced to reset and then the stream is reserved for the exclusive access until it's closed.
The below is a test patch. Now you see it's so simple...
Can anyone test it?
Done. It does not work:
$ ac3dec -C -c 1 dh122.ac3 Using PCM device 'plug:iec958:{AES0 0x2 AES1 0x82 AES2 0x0 AES3 0x2 CARD 1}' snd_pcm_open: Device or resource busy Output open failed
I can't find a trace to explain why in kern.log or in syslog.
Are you sure that you are not using the spdif PCM beforehand? Before running ac3dec, check which PCM substream is being opened. You can see the PCM substreams in /proc/asound/card0/pcm*p/*
Takashi
Takashi Iwai tiwai@suse.de writes:
I can't find a trace to explain why in kern.log or in syslog.
Are you sure that you are not using the spdif PCM beforehand? Before running ac3dec, check which PCM substream is being opened.
Duuhh ... Stupid mistake on my side: I've sent both test streams to SPDIF. I must have been more tired than I thought.
I'll test again this week-end.
Sorry for the trouble.
Takashi Iwai tiwai@suse.de writes:
The below is a test patch. Now you see it's so simple...
Can anyone test it?
Done (correctly this time).
Your patch is working fine.
The only hitch is that the analog copy to spdif is not restored once the "exclusive" spdif stream is closed. Although I don't know if this should be fixed or documented.
Thanks for the patch.
At Sat, 14 Apr 2007 12:00:46 +0200, Dominique Dumont wrote:
Takashi Iwai tiwai@suse.de writes:
The below is a test patch. Now you see it's so simple...
Can anyone test it?
Done (correctly this time).
Your patch is working fine.
OK, now it's on HG tree.
The only hitch is that the analog copy to spdif is not restored once the "exclusive" spdif stream is closed. Although I don't know if this should be fixed or documented.
Yes, that's a tiny problem. Fixing this would require a state save and restore, which isn't sexy. So I'll keep it as it is for now.
Takashi
participants (4)
-
Dominique Dumont -
Dominique Dumont -
Lee Revell -
Takashi Iwai