Re: [alsa-devel] iec958 switch uneffective while playing ac3 stream
[Nice analysis cut out..]
At Tue, 03 Apr 2007 10:17:16 +0200, Dominique Dumont wrote:
If the stream format is not valid when the digi converter is set up, there's a chance that the digi converter goes belly up.
I reckon that setting the stream *then* the digi converter would be safer.
OK, now question is whether we need to turn off DIGI_CONV* once before setting it up again.
Anyway, a test patch is below. It forces to set DIGI_CONV* verbs in prepare. Give it a try.
thanks,
Takashi
diff -r 05ecca0fba92 pci/hda/hda_codec.c --- a/pci/hda/hda_codec.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/hda_codec.c Tue Apr 03 15:28:47 2007 +0200 @@ -1083,6 +1083,19 @@ static int snd_hda_spdif_default_put(str return change; }
+static void setup_digi_convert(struct hda_codec *codec, hda_nid_t nid) +{ + unsigned short val = codec->spdif_ctls; + + snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_DIGI_CONVERT_1, + val & 0xff); + if (get_wcaps(codec, nid) & AC_WCAP_OUT_AMP) + snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE, + AC_AMP_SET_RIGHT | AC_AMP_SET_LEFT | + AC_AMP_SET_OUTPUT | + ((val & 1) ? 0 : 0x80)); +} + static int snd_hda_spdif_out_switch_info(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo) { uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN; @@ -1114,10 +1127,7 @@ static int snd_hda_spdif_out_switch_put( change = codec->spdif_ctls != val; if (change || codec->in_resume) { codec->spdif_ctls = val; - snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_DIGI_CONVERT_1, val & 0xff); - snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE, - AC_AMP_SET_RIGHT | AC_AMP_SET_LEFT | - AC_AMP_SET_OUTPUT | ((val & 1) ? 0 : 0x80)); + setup_digi_convert(codec, nid); } mutex_unlock(&codec->spdif_mutex); return change; @@ -1901,6 +1911,21 @@ int snd_hda_multi_out_dig_open(struct hd return 0; }
+int snd_hda_multi_out_dig_prepare(struct hda_codec *codec, + struct hda_multi_out *mout, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + mutex_lock(&codec->spdif_mutex); + snd_hda_codec_setup_stream(codec, mout->dig_out_nid, + stream_tag, 0, format); + /* set up DIGI_* verbs again */ + setup_digi_convert(codec, mout->dig_out_nid); + mutex_unlock(&codec->spdif_mutex); + return 0; +} + /* * release the digital out */ @@ -1949,6 +1974,8 @@ int snd_hda_multi_out_analog_prepare(str mout->dig_out_used = 0; snd_hda_codec_setup_stream(codec, mout->dig_out_nid, 0, 0, 0); } + /* set up DIGI_* verbs again */ + setup_digi_convert(codec, mout->dig_out_nid); } mutex_unlock(&codec->spdif_mutex);
diff -r 05ecca0fba92 pci/hda/hda_local.h --- a/pci/hda/hda_local.h Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/hda_local.h Tue Apr 03 15:31:29 2007 +0200 @@ -148,6 +148,11 @@ struct hda_multi_out {
int snd_hda_multi_out_dig_open(struct hda_codec *codec, struct hda_multi_out *mout); int snd_hda_multi_out_dig_close(struct hda_codec *codec, struct hda_multi_out *mout); +int snd_hda_multi_out_dig_prepare(struct hda_codec *codec, + struct hda_multi_out *mout, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream); int snd_hda_multi_out_analog_open(struct hda_codec *codec, struct hda_multi_out *mout, struct snd_pcm_substream *substream); int snd_hda_multi_out_analog_prepare(struct hda_codec *codec, struct hda_multi_out *mout, diff -r 05ecca0fba92 pci/hda/patch_realtek.c --- a/pci/hda/patch_realtek.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_realtek.c Tue Apr 03 15:32:23 2007 +0200 @@ -1916,6 +1916,17 @@ static int alc880_dig_playback_pcm_open( return snd_hda_multi_out_dig_open(codec, &spec->multiout); }
+static int alc880_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct alc_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, + stream_tag, format, substream); +} + static int alc880_dig_playback_pcm_close(struct hda_pcm_stream *hinfo, struct hda_codec *codec, struct snd_pcm_substream *substream) @@ -1984,7 +1995,8 @@ static struct hda_pcm_stream alc880_pcm_ /* NID is set in alc_build_pcms */ .ops = { .open = alc880_dig_playback_pcm_open, - .close = alc880_dig_playback_pcm_close + .close = alc880_dig_playback_pcm_close, + .prepare = alc880_dig_playback_pcm_prepare }, };
At Tue, 03 Apr 2007 15:37:53 +0200, I wrote:
[Nice analysis cut out..]
At Tue, 03 Apr 2007 10:17:16 +0200, Dominique Dumont wrote:
If the stream format is not valid when the digi converter is set up, there's a chance that the digi converter goes belly up.
I reckon that setting the stream *then* the digi converter would be safer.
OK, now question is whether we need to turn off DIGI_CONV* once before setting it up again.
Anyway, a test patch is below. It forces to set DIGI_CONV* verbs in prepare. Give it a try.
BTW, it might be better first to remove the check of AC_WCAP_OUT_AMP in setup_digi_convert(). I vaguely remember that this amp setup was required for some codecs and it might be somehow broken that doesn't report the amp capability correctly.
Takashi
thanks,
Takashi
diff -r 05ecca0fba92 pci/hda/hda_codec.c --- a/pci/hda/hda_codec.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/hda_codec.c Tue Apr 03 15:28:47 2007 +0200 @@ -1083,6 +1083,19 @@ static int snd_hda_spdif_default_put(str return change; }
+static void setup_digi_convert(struct hda_codec *codec, hda_nid_t nid) +{
- unsigned short val = codec->spdif_ctls;
- snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_DIGI_CONVERT_1,
val & 0xff);- if (get_wcaps(codec, nid) & AC_WCAP_OUT_AMP)
snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE,AC_AMP_SET_RIGHT | AC_AMP_SET_LEFT |AC_AMP_SET_OUTPUT |((val & 1) ? 0 : 0x80));+}
static int snd_hda_spdif_out_switch_info(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo) { uinfo->type = SNDRV_CTL_ELEM_TYPE_BOOLEAN; @@ -1114,10 +1127,7 @@ static int snd_hda_spdif_out_switch_put( change = codec->spdif_ctls != val; if (change || codec->in_resume) { codec->spdif_ctls = val;
snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_DIGI_CONVERT_1, val & 0xff);snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE,AC_AMP_SET_RIGHT | AC_AMP_SET_LEFT |AC_AMP_SET_OUTPUT | ((val & 1) ? 0 : 0x80));
setup_digi_convert(codec, nid);@@ -1901,6 +1911,21 @@ int snd_hda_multi_out_dig_open(struct hd return 0; }
+int snd_hda_multi_out_dig_prepare(struct hda_codec *codec,
struct hda_multi_out *mout,unsigned int stream_tag,unsigned int format,struct snd_pcm_substream *substream)+{
- mutex_lock(&codec->spdif_mutex);
- snd_hda_codec_setup_stream(codec, mout->dig_out_nid,
stream_tag, 0, format);- /* set up DIGI_* verbs again */
- setup_digi_convert(codec, mout->dig_out_nid);
- mutex_unlock(&codec->spdif_mutex);
- return 0;
+}
/*
- release the digital out
*/ @@ -1949,6 +1974,8 @@ int snd_hda_multi_out_analog_prepare(str mout->dig_out_used = 0; snd_hda_codec_setup_stream(codec, mout->dig_out_nid, 0, 0, 0); }
/* set up DIGI_* verbs again */setup_digi_convert(codec, mout->dig_out_nid);diff -r 05ecca0fba92 pci/hda/hda_local.h --- a/pci/hda/hda_local.h Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/hda_local.h Tue Apr 03 15:31:29 2007 +0200 @@ -148,6 +148,11 @@ struct hda_multi_out {
int snd_hda_multi_out_dig_open(struct hda_codec *codec, struct hda_multi_out *mout); int snd_hda_multi_out_dig_close(struct hda_codec *codec, struct hda_multi_out *mout); +int snd_hda_multi_out_dig_prepare(struct hda_codec *codec,
struct hda_multi_out *mout,unsigned int stream_tag,unsigned int format,struct snd_pcm_substream *substream);int snd_hda_multi_out_analog_open(struct hda_codec *codec, struct hda_multi_out *mout, struct snd_pcm_substream *substream); int snd_hda_multi_out_analog_prepare(struct hda_codec *codec, struct hda_multi_out *mout, diff -r 05ecca0fba92 pci/hda/patch_realtek.c --- a/pci/hda/patch_realtek.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_realtek.c Tue Apr 03 15:32:23 2007 +0200 @@ -1916,6 +1916,17 @@ static int alc880_dig_playback_pcm_open( return snd_hda_multi_out_dig_open(codec, &spec->multiout); }
+static int alc880_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
struct hda_codec *codec,unsigned int stream_tag,unsigned int format,struct snd_pcm_substream *substream)+{
- struct alc_spec *spec = codec->spec;
- return snd_hda_multi_out_dig_prepare(codec, &spec->multiout,
stream_tag, format, substream);+}
static int alc880_dig_playback_pcm_close(struct hda_pcm_stream *hinfo, struct hda_codec *codec, struct snd_pcm_substream *substream) @@ -1984,7 +1995,8 @@ static struct hda_pcm_stream alc880_pcm_ /* NID is set in alc_build_pcms */ .ops = { .open = alc880_dig_playback_pcm_open,
.close = alc880_dig_playback_pcm_close
.close = alc880_dig_playback_pcm_close,.prepare = alc880_dig_playback_pcm_prepare};
Takashi Iwai tiwai@suse.de writes:
I reckon that setting the stream *then* the digi converter would be safer.
OK, now question is whether we need to turn off DIGI_CONV* once before setting it up again.
Anyway, a test patch is below. It forces to set DIGI_CONV* verbs in prepare. Give it a try.
Sorry, your patch does not work.
On the other hand I've tested another idea. In azx_pcm_prepare, I switch off iec958 DigEn before setting up stream format, and sending other verbs. At the end of azx_pcm_prepare, I restore the switch status.
And guess what ? It works, I get dolby digital !! :-)
And Dag, guess what also: 44KHz spdif output also works :-D
I've included the patch which implement the behavior described above. Although this diff may be more a hack than a patch ... Feel free to modify it to better suit the driver's design.
Cheers
Dominique Dumont domi.dumont@free.fr writes:
I've included the patch which implement the behavior described above.
Looks like the attachment was ditched by mailman. Here it is ...
--- alsa-driver-1.0.14rc3-old/alsa-kernel/pci/hda/hda_intel.c 2007-03-06 13:26:32.000000000 +0100 +++ alsa-driver-1.0.14rc3/alsa-kernel/pci/hda/hda_intel.c 2007-04-03 19:21:34.000000000 +0200 @@ -1140,6 +1140,8 @@ struct azx_dev *azx_dev = get_azx_dev(substream); struct hda_pcm_stream *hinfo = apcm->hinfo[substream->stream]; struct snd_pcm_runtime *runtime = substream->runtime; + int val ; + int ret ;
azx_dev->bufsize = snd_pcm_lib_buffer_bytes(substream); azx_dev->fragsize = snd_pcm_lib_period_bytes(substream); @@ -1148,12 +1150,22 @@ runtime->channels, runtime->format, hinfo->maxbps); + + + if (! azx_dev->format_val) { snd_printk(KERN_ERR SFX "invalid format_val, rate=%d, ch=%d, format=%d\n", runtime->rate, runtime->channels, runtime->format); return -EINVAL; }
+ /* azx_dev->format_val |= 0x800 ; */ + + /* Switch off iec958 before changing stream format as the digi + converter may misbehave */ + val = apcm->codec->spdif_ctls ; + snd_hda_codec_write(apcm->codec, hinfo->nid, 0, AC_VERB_SET_DIGI_CONVERT_1, val & 0xfe); + snd_printdd("azx_pcm_prepare: bufsize=0x%x, fragsize=0x%x, format=0x%x\n", azx_dev->bufsize, azx_dev->fragsize, azx_dev->format_val); azx_setup_periods(azx_dev); @@ -1163,8 +1175,12 @@ else azx_dev->fifo_size = 0;
- return hinfo->ops.prepare(hinfo, apcm->codec, azx_dev->stream_tag, + ret = hinfo->ops.prepare(hinfo, apcm->codec, azx_dev->stream_tag, azx_dev->format_val, substream); + + /* Restore iec958 switch after all stream and converter are set */ + snd_hda_codec_write(apcm->codec, hinfo->nid, 0, AC_VERB_SET_DIGI_CONVERT_1, val & 0xff); + return ret ; }
static int azx_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
Dominique Dumont domi.dumont@free.fr writes:
Dominique Dumont domi.dumont@free.fr writes:
I've included the patch which implement the behavior described above.
Looks like the attachment was ditched by mailman. Here it is ...
Your patch/hack does the trick here as well - great work!
The initialization problem is gone, and the S/PDIF output locks correctly at 44.1kHz, 48kHz, 96kHz, and 192kHz!
Takashi: Can this fix be worked into the official HDA code? Please tell me if you want me to test any changes!
Dominique Dumont domi.dumont@free.fr writes:
I've included the patch which implement the behavior described above. Although this diff may be more a hack than a patch ...
I forgot to mention that: - my test included Takashi-san patch. I have not yet tested without it. Dag, did you include Takashi-san's patch in your tests ? - I have not tested (yet) analog output.
Cheers
Dominique Dumont domi@komarr.grenoble.hp.com writes:
Dominique Dumont domi.dumont@free.fr writes:
I've included the patch which implement the behavior described above. Although this diff may be more a hack than a patch ...
I forgot to mention that:
- my test included Takashi-san patch. I have not yet tested without it. Dag, did you include Takashi-san's patch in your tests ?
- I have not tested (yet) analog output.
I tested without Takashi's patch - your hack did the trick without any further modifications.
I think Takashi's patch should be included for another reason - it adds a check for NID capabilities before muting/unmuting the digital converter (and the ALC882M reports no mute/unmute capabilities for the digital converter).
At 04 Apr 2007 16:09:48 +0200, Dag Lem wrote:
Dominique Dumont domi@komarr.grenoble.hp.com writes:
Dominique Dumont domi.dumont@free.fr writes:
I've included the patch which implement the behavior described above. Although this diff may be more a hack than a patch ...
I forgot to mention that:
- my test included Takashi-san patch. I have not yet tested without it. Dag, did you include Takashi-san's patch in your tests ?
- I have not tested (yet) analog output.
I tested without Takashi's patch - your hack did the trick without any further modifications.
I think Takashi's patch should be included for another reason - it adds a check for NID capabilities before muting/unmuting the digital converter (and the ALC882M reports no mute/unmute capabilities for the digital converter).
Ah, that's one point I forgot in my last patch. Could you try this patch alone?
thanks,
Takashi
diff -r 05ecca0fba92 pci/hda/hda_codec.c --- a/pci/hda/hda_codec.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/hda_codec.c Wed Apr 04 16:12:25 2007 +0200 @@ -1114,10 +1114,14 @@ static int snd_hda_spdif_out_switch_put( change = codec->spdif_ctls != val; if (change || codec->in_resume) { codec->spdif_ctls = val; - snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_DIGI_CONVERT_1, val & 0xff); - snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE, - AC_AMP_SET_RIGHT | AC_AMP_SET_LEFT | - AC_AMP_SET_OUTPUT | ((val & 1) ? 0 : 0x80)); + snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_DIGI_CONVERT_1, + val & 0xff); + if (get_wcaps(codec, nid) & AC_WCAP_OUT_AMP) + snd_hda_codec_write(codec, nid, 0, + AC_VERB_SET_AMP_GAIN_MUTE, + AC_AMP_SET_RIGHT | AC_AMP_SET_LEFT | + AC_AMP_SET_OUTPUT | + ((val & 1) ? 0 : 0x80)); } mutex_unlock(&codec->spdif_mutex); return change; @@ -1886,6 +1890,21 @@ int snd_hda_input_mux_put(struct hda_cod * Multi-channel / digital-out PCM helper functions */
+/* setup SPDIF output stream */ +static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid, + unsigned int stream_tag, unsigned int format) +{ + /* turn off SPDIF once; otherwise the IEC958 bits won't be updated */ + if (codec->spdif_ctls & 1) + snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_DIGI_CONVERT_1, + codec->spdif_ctls & 0xfe); + snd_hda_codec_setup_stream(codec, nid, stream_tag, 0, format); + /* turn on again (if needed) */ + if (codec->spdif_ctls & 1) + snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_DIGI_CONVERT_1, + codec->spdif_ctls & 0xff); +} + /* * open the digital out in the exclusive mode */ @@ -1897,6 +1916,18 @@ int snd_hda_multi_out_dig_open(struct hd return -EBUSY; /* already being used */ } mout->dig_out_used = HDA_DIG_EXCLUSIVE; + mutex_unlock(&codec->spdif_mutex); + return 0; +} + +int snd_hda_multi_out_dig_prepare(struct hda_codec *codec, + struct hda_multi_out *mout, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + mutex_lock(&codec->spdif_mutex); + setup_dig_out_stream(codec, mout->dig_out_nid, stream_tag, format); mutex_unlock(&codec->spdif_mutex); return 0; } @@ -1942,9 +1973,8 @@ int snd_hda_multi_out_analog_prepare(str snd_hda_is_supported_format(codec, mout->dig_out_nid, format) && ! (codec->spdif_status & IEC958_AES0_NONAUDIO)) { mout->dig_out_used = HDA_DIG_ANALOG_DUP; - /* setup digital receiver */ - snd_hda_codec_setup_stream(codec, mout->dig_out_nid, - stream_tag, 0, format); + setup_dig_out_stream(codec, mout->dig_out_nid, + stream_tag, format); } else { mout->dig_out_used = 0; snd_hda_codec_setup_stream(codec, mout->dig_out_nid, 0, 0, 0); diff -r 05ecca0fba92 pci/hda/hda_local.h --- a/pci/hda/hda_local.h Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/hda_local.h Tue Apr 03 15:31:29 2007 +0200 @@ -148,6 +148,11 @@ struct hda_multi_out {
int snd_hda_multi_out_dig_open(struct hda_codec *codec, struct hda_multi_out *mout); int snd_hda_multi_out_dig_close(struct hda_codec *codec, struct hda_multi_out *mout); +int snd_hda_multi_out_dig_prepare(struct hda_codec *codec, + struct hda_multi_out *mout, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream); int snd_hda_multi_out_analog_open(struct hda_codec *codec, struct hda_multi_out *mout, struct snd_pcm_substream *substream); int snd_hda_multi_out_analog_prepare(struct hda_codec *codec, struct hda_multi_out *mout, diff -r 05ecca0fba92 pci/hda/patch_analog.c --- a/pci/hda/patch_analog.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_analog.c Tue Apr 03 18:21:06 2007 +0200 @@ -192,6 +192,17 @@ static int ad198x_dig_playback_pcm_close return snd_hda_multi_out_dig_close(codec, &spec->multiout); }
+static int ad198x_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct ad198x_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, stream_tag, + format, substream); +} + /* * Analog capture */ @@ -250,7 +261,8 @@ static struct hda_pcm_stream ad198x_pcm_ .nid = 0, /* fill later */ .ops = { .open = ad198x_dig_playback_pcm_open, - .close = ad198x_dig_playback_pcm_close + .close = ad198x_dig_playback_pcm_close, + .prepare = ad198x_dig_playback_pcm_prepare }, };
diff -r 05ecca0fba92 pci/hda/patch_atihdmi.c --- a/pci/hda/patch_atihdmi.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_atihdmi.c Tue Apr 03 18:22:38 2007 +0200 @@ -95,6 +95,17 @@ static int atihdmi_dig_playback_pcm_clos return snd_hda_multi_out_dig_close(codec, &spec->multiout); }
+static int atihdmi_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct atihdmi_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, stream_tag, + format, substream); +} + static struct hda_pcm_stream atihdmi_pcm_digital_playback = { .substreams = 1, .channels_min = 2, @@ -102,7 +113,8 @@ static struct hda_pcm_stream atihdmi_pcm .nid = 0x2, /* NID to query formats and rates and setup streams */ .ops = { .open = atihdmi_dig_playback_pcm_open, - .close = atihdmi_dig_playback_pcm_close + .close = atihdmi_dig_playback_pcm_close, + .prepare = atihdmi_dig_playback_pcm_prepare }, };
diff -r 05ecca0fba92 pci/hda/patch_cmedia.c --- a/pci/hda/patch_cmedia.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_cmedia.c Tue Apr 03 18:21:40 2007 +0200 @@ -497,6 +497,17 @@ static int cmi9880_dig_playback_pcm_clos return snd_hda_multi_out_dig_close(codec, &spec->multiout); }
+static int cmi9880_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct cmi_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, stream_tag, + format, substream); +} + /* * Analog capture */ @@ -556,7 +567,8 @@ static struct hda_pcm_stream cmi9880_pcm /* NID is set in cmi9880_build_pcms */ .ops = { .open = cmi9880_dig_playback_pcm_open, - .close = cmi9880_dig_playback_pcm_close + .close = cmi9880_dig_playback_pcm_close, + .prepare = cmi9880_dig_playback_pcm_prepare }, };
diff -r 05ecca0fba92 pci/hda/patch_conexant.c --- a/pci/hda/patch_conexant.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_conexant.c Tue Apr 03 18:23:15 2007 +0200 @@ -136,6 +136,18 @@ static int conexant_dig_playback_pcm_clo return snd_hda_multi_out_dig_close(codec, &spec->multiout); }
+static int conexant_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct conexant_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, + stream_tag, + format, substream); +} + /* * Analog capture */ @@ -194,7 +206,8 @@ static struct hda_pcm_stream conexant_pc .nid = 0, /* fill later */ .ops = { .open = conexant_dig_playback_pcm_open, - .close = conexant_dig_playback_pcm_close + .close = conexant_dig_playback_pcm_close, + .prepare = conexant_dig_playback_pcm_prepare }, };
diff -r 05ecca0fba92 pci/hda/patch_realtek.c --- a/pci/hda/patch_realtek.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_realtek.c Tue Apr 03 15:32:23 2007 +0200 @@ -1916,6 +1916,17 @@ static int alc880_dig_playback_pcm_open( return snd_hda_multi_out_dig_open(codec, &spec->multiout); }
+static int alc880_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct alc_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, + stream_tag, format, substream); +} + static int alc880_dig_playback_pcm_close(struct hda_pcm_stream *hinfo, struct hda_codec *codec, struct snd_pcm_substream *substream) @@ -1984,7 +1995,8 @@ static struct hda_pcm_stream alc880_pcm_ /* NID is set in alc_build_pcms */ .ops = { .open = alc880_dig_playback_pcm_open, - .close = alc880_dig_playback_pcm_close + .close = alc880_dig_playback_pcm_close, + .prepare = alc880_dig_playback_pcm_prepare }, };
diff -r 05ecca0fba92 pci/hda/patch_sigmatel.c --- a/pci/hda/patch_sigmatel.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_sigmatel.c Tue Apr 03 18:24:01 2007 +0200 @@ -814,6 +814,17 @@ static int stac92xx_dig_playback_pcm_clo return snd_hda_multi_out_dig_close(codec, &spec->multiout); }
+static int stac92xx_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct sigmatel_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, + stream_tag, format, substream); +} +
/* * Analog capture callbacks @@ -848,7 +859,8 @@ static struct hda_pcm_stream stac92xx_pc /* NID is set in stac92xx_build_pcms */ .ops = { .open = stac92xx_dig_playback_pcm_open, - .close = stac92xx_dig_playback_pcm_close + .close = stac92xx_dig_playback_pcm_close, + .prepare = stac92xx_dig_playback_pcm_prepare }, };
diff -r 05ecca0fba92 pci/hda/patch_via.c --- a/pci/hda/patch_via.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_via.c Tue Apr 03 18:25:22 2007 +0200 @@ -378,6 +378,17 @@ static int via_dig_playback_pcm_close(st return snd_hda_multi_out_dig_close(codec, &spec->multiout); }
+static int via_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct via_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, + stream_tag, format, substream); +} + /* * Analog capture */ @@ -434,7 +445,8 @@ static struct hda_pcm_stream vt1708_pcm_ /* NID is set in via_build_pcms */ .ops = { .open = via_dig_playback_pcm_open, - .close = via_dig_playback_pcm_close + .close = via_dig_playback_pcm_close, + .prepare = via_dig_playback_pcm_prepare }, };
Takashi Iwai tiwai@suse.de writes:
At 04 Apr 2007 16:09:48 +0200, Dag Lem wrote:
[...]
I think Takashi's patch should be included for another reason - it adds a check for NID capabilities before muting/unmuting the digital converter (and the ALC882M reports no mute/unmute capabilities for the digital converter).
Ah, that's one point I forgot in my last patch. Could you try this patch alone?
Sorry, Easter got in the way. Please tell me if there is still something you'd like me to test.
Thanks to you and Dominique for working on this issue!
At 11 Apr 2007 10:29:20 +0200, Dag Lem wrote:
Takashi Iwai tiwai@suse.de writes:
At 04 Apr 2007 16:09:48 +0200, Dag Lem wrote:
[...]
I think Takashi's patch should be included for another reason - it adds a check for NID capabilities before muting/unmuting the digital converter (and the ALC882M reports no mute/unmute capabilities for the digital converter).
Ah, that's one point I forgot in my last patch. Could you try this patch alone?
Sorry, Easter got in the way. Please tell me if there is still something you'd like me to test.
The fix is already in HG tree, so please test the latest HG version to confirm whether it's fixed.
thanks,
Takashi
Takashi Iwai tiwai@suse.de writes:
[...]
The fix is already in HG tree, so please test the latest HG version to confirm whether it's fixed.
I finally got around to test the code in the HG tree.
The S/PDIF output still works perfectly on the ALC882M. I ran PCM tests using all available frame rates (44.1kHz, 48kHz, 96kHz, 192kHz). I also tested AC3 (at 48kHz) and DTS (at 44.1kHz and 48kHz).
Good work! That goes for Dominique as well, of course :-)
The only fly in the ointment for the average user will be, I think, that HDA-Intel.pcm.default is using dmix. This effectively cuts most non-experts off from using anything but 48kHz, resulting in inferior sound quality (and unavailability to play back 44.1KHz DTS over S/PDIF) due to resampling.
Perhaps dmix could be configured with more than one destination frame rate, so that resampling would only be used if absolutely necessary? Just a thought.
At Tue, 03 Apr 2007 19:31:36 +0200, Dominique Dumont wrote:
Takashi Iwai tiwai@suse.de writes:
I reckon that setting the stream *then* the digi converter would be safer.
OK, now question is whether we need to turn off DIGI_CONV* once before setting it up again.
Anyway, a test patch is below. It forces to set DIGI_CONV* verbs in prepare. Give it a try.
Sorry, your patch does not work.
On the other hand I've tested another idea. In azx_pcm_prepare, I switch off iec958 DigEn before setting up stream format, and sending other verbs. At the end of azx_pcm_prepare, I restore the switch status.
And guess what ? It works, I get dolby digital !! :-)
And Dag, guess what also: 44KHz spdif output also works :-D
I've included the patch which implement the behavior described above. Although this diff may be more a hack than a patch ... Feel free to modify it to better suit the driver's design.
Good to hear you have a working patch!
The patch is a bit dangerous because it unconditionally resets the DIGI_CONVERT verb for each widget. 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.
thanks,
Takashi
diff -r 05ecca0fba92 pci/hda/hda_codec.c --- a/pci/hda/hda_codec.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/hda_codec.c Wed Apr 04 12:26:15 2007 +0200 @@ -1886,6 +1886,21 @@ int snd_hda_input_mux_put(struct hda_cod * Multi-channel / digital-out PCM helper functions */
+/* setup SPDIF output stream */ +static void setup_dig_out_stream(struct hda_codec *codec, hda_nid_t nid, + unsigned int stream_tag, unsigned int format) +{ + /* turn off SPDIF once; otherwise the IEC958 bits won't be updated */ + if (codec->spdif_ctls & 1) + snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_DIGI_CONVERT_1, + codec->spdif_ctls & 0xfe); + snd_hda_codec_setup_stream(codec, nid, stream_tag, 0, format); + /* turn on again (if needed) */ + if (codec->spdif_ctls & 1) + snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_DIGI_CONVERT_1, + codec->spdif_ctls & 0xff); +} + /* * open the digital out in the exclusive mode */ @@ -1897,6 +1912,18 @@ int snd_hda_multi_out_dig_open(struct hd return -EBUSY; /* already being used */ } mout->dig_out_used = HDA_DIG_EXCLUSIVE; + mutex_unlock(&codec->spdif_mutex); + return 0; +} + +int snd_hda_multi_out_dig_prepare(struct hda_codec *codec, + struct hda_multi_out *mout, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + mutex_lock(&codec->spdif_mutex); + setup_dig_out_stream(codec, mout->dig_out_nid, stream_tag, format); mutex_unlock(&codec->spdif_mutex); return 0; } @@ -1942,9 +1969,8 @@ int snd_hda_multi_out_analog_prepare(str snd_hda_is_supported_format(codec, mout->dig_out_nid, format) && ! (codec->spdif_status & IEC958_AES0_NONAUDIO)) { mout->dig_out_used = HDA_DIG_ANALOG_DUP; - /* setup digital receiver */ - snd_hda_codec_setup_stream(codec, mout->dig_out_nid, - stream_tag, 0, format); + setup_dig_out_stream(codec, mout->dig_out_nid, + stream_tag, format); } else { mout->dig_out_used = 0; snd_hda_codec_setup_stream(codec, mout->dig_out_nid, 0, 0, 0); diff -r 05ecca0fba92 pci/hda/hda_local.h --- a/pci/hda/hda_local.h Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/hda_local.h Tue Apr 03 15:31:29 2007 +0200 @@ -148,6 +148,11 @@ struct hda_multi_out {
int snd_hda_multi_out_dig_open(struct hda_codec *codec, struct hda_multi_out *mout); int snd_hda_multi_out_dig_close(struct hda_codec *codec, struct hda_multi_out *mout); +int snd_hda_multi_out_dig_prepare(struct hda_codec *codec, + struct hda_multi_out *mout, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream); int snd_hda_multi_out_analog_open(struct hda_codec *codec, struct hda_multi_out *mout, struct snd_pcm_substream *substream); int snd_hda_multi_out_analog_prepare(struct hda_codec *codec, struct hda_multi_out *mout, diff -r 05ecca0fba92 pci/hda/patch_analog.c --- a/pci/hda/patch_analog.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_analog.c Tue Apr 03 18:21:06 2007 +0200 @@ -192,6 +192,17 @@ static int ad198x_dig_playback_pcm_close return snd_hda_multi_out_dig_close(codec, &spec->multiout); }
+static int ad198x_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct ad198x_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, stream_tag, + format, substream); +} + /* * Analog capture */ @@ -250,7 +261,8 @@ static struct hda_pcm_stream ad198x_pcm_ .nid = 0, /* fill later */ .ops = { .open = ad198x_dig_playback_pcm_open, - .close = ad198x_dig_playback_pcm_close + .close = ad198x_dig_playback_pcm_close, + .prepare = ad198x_dig_playback_pcm_prepare }, };
diff -r 05ecca0fba92 pci/hda/patch_atihdmi.c --- a/pci/hda/patch_atihdmi.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_atihdmi.c Tue Apr 03 18:22:38 2007 +0200 @@ -95,6 +95,17 @@ static int atihdmi_dig_playback_pcm_clos return snd_hda_multi_out_dig_close(codec, &spec->multiout); }
+static int atihdmi_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct atihdmi_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, stream_tag, + format, substream); +} + static struct hda_pcm_stream atihdmi_pcm_digital_playback = { .substreams = 1, .channels_min = 2, @@ -102,7 +113,8 @@ static struct hda_pcm_stream atihdmi_pcm .nid = 0x2, /* NID to query formats and rates and setup streams */ .ops = { .open = atihdmi_dig_playback_pcm_open, - .close = atihdmi_dig_playback_pcm_close + .close = atihdmi_dig_playback_pcm_close, + .prepare = atihdmi_dig_playback_pcm_prepare }, };
diff -r 05ecca0fba92 pci/hda/patch_cmedia.c --- a/pci/hda/patch_cmedia.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_cmedia.c Tue Apr 03 18:21:40 2007 +0200 @@ -497,6 +497,17 @@ static int cmi9880_dig_playback_pcm_clos return snd_hda_multi_out_dig_close(codec, &spec->multiout); }
+static int cmi9880_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct cmi_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, stream_tag, + format, substream); +} + /* * Analog capture */ @@ -556,7 +567,8 @@ static struct hda_pcm_stream cmi9880_pcm /* NID is set in cmi9880_build_pcms */ .ops = { .open = cmi9880_dig_playback_pcm_open, - .close = cmi9880_dig_playback_pcm_close + .close = cmi9880_dig_playback_pcm_close, + .prepare = cmi9880_dig_playback_pcm_prepare }, };
diff -r 05ecca0fba92 pci/hda/patch_conexant.c --- a/pci/hda/patch_conexant.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_conexant.c Tue Apr 03 18:23:15 2007 +0200 @@ -136,6 +136,18 @@ static int conexant_dig_playback_pcm_clo return snd_hda_multi_out_dig_close(codec, &spec->multiout); }
+static int conexant_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct conexant_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, + stream_tag, + format, substream); +} + /* * Analog capture */ @@ -194,7 +206,8 @@ static struct hda_pcm_stream conexant_pc .nid = 0, /* fill later */ .ops = { .open = conexant_dig_playback_pcm_open, - .close = conexant_dig_playback_pcm_close + .close = conexant_dig_playback_pcm_close, + .prepare = conexant_dig_playback_pcm_prepare }, };
diff -r 05ecca0fba92 pci/hda/patch_realtek.c --- a/pci/hda/patch_realtek.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_realtek.c Tue Apr 03 15:32:23 2007 +0200 @@ -1916,6 +1916,17 @@ static int alc880_dig_playback_pcm_open( return snd_hda_multi_out_dig_open(codec, &spec->multiout); }
+static int alc880_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct alc_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, + stream_tag, format, substream); +} + static int alc880_dig_playback_pcm_close(struct hda_pcm_stream *hinfo, struct hda_codec *codec, struct snd_pcm_substream *substream) @@ -1984,7 +1995,8 @@ static struct hda_pcm_stream alc880_pcm_ /* NID is set in alc_build_pcms */ .ops = { .open = alc880_dig_playback_pcm_open, - .close = alc880_dig_playback_pcm_close + .close = alc880_dig_playback_pcm_close, + .prepare = alc880_dig_playback_pcm_prepare }, };
diff -r 05ecca0fba92 pci/hda/patch_sigmatel.c --- a/pci/hda/patch_sigmatel.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_sigmatel.c Tue Apr 03 18:24:01 2007 +0200 @@ -814,6 +814,17 @@ static int stac92xx_dig_playback_pcm_clo return snd_hda_multi_out_dig_close(codec, &spec->multiout); }
+static int stac92xx_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct sigmatel_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, + stream_tag, format, substream); +} +
/* * Analog capture callbacks @@ -848,7 +859,8 @@ static struct hda_pcm_stream stac92xx_pc /* NID is set in stac92xx_build_pcms */ .ops = { .open = stac92xx_dig_playback_pcm_open, - .close = stac92xx_dig_playback_pcm_close + .close = stac92xx_dig_playback_pcm_close, + .prepare = stac92xx_dig_playback_pcm_prepare }, };
diff -r 05ecca0fba92 pci/hda/patch_via.c --- a/pci/hda/patch_via.c Tue Apr 03 13:20:49 2007 +0200 +++ b/pci/hda/patch_via.c Tue Apr 03 18:25:22 2007 +0200 @@ -378,6 +378,17 @@ static int via_dig_playback_pcm_close(st return snd_hda_multi_out_dig_close(codec, &spec->multiout); }
+static int via_dig_playback_pcm_prepare(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + unsigned int stream_tag, + unsigned int format, + struct snd_pcm_substream *substream) +{ + struct via_spec *spec = codec->spec; + return snd_hda_multi_out_dig_prepare(codec, &spec->multiout, + stream_tag, format, substream); +} + /* * Analog capture */ @@ -434,7 +445,8 @@ static struct hda_pcm_stream vt1708_pcm_ /* NID is set in via_build_pcms */ .ops = { .open = via_dig_playback_pcm_open, - .close = via_dig_playback_pcm_close + .close = via_dig_playback_pcm_close, + .prepare = via_dig_playback_pcm_prepare }, };
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.
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).
- /* 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) ?
I've did not have time to test the last patch you sent.
Cheers & Thanks
participants (4)
-
Dag Lem -
Dominique Dumont -
Dominique Dumont -
Takashi Iwai