[alsa-devel] HDMI audio: TV vs. codec supported sample rates
A user has the following setup:
A GPU which supports audio-over-HDMI. The codec supports sample rates 32000 44100 48000 88200 96000 176400 192000 (from /proc/asound/card1/codec#1). However, the user's TV supports only sample rates 44100 48000 88200 (from /proc/asound/card1/eld*).
When the user plays sound with sample rate 22050, they hear nothing. Sound with sample rates supported by the TV works OK.
My question: Is the HDA codec driver supposed to dynamically adjust its list of supported sample rates based on the ELD content, or is the ALSA library somehow supposed to detect the subset of rates supported in HW and convert the sample rate in SW before sending the audio to the driver?
Thanks for any insight.
On Tue, 3 Aug 2010, Stephen Warren wrote:
A user has the following setup:
A GPU which supports audio-over-HDMI. The codec supports sample rates 32000 44100 48000 88200 96000 176400 192000 (from /proc/asound/card1/codec#1). However, the user's TV supports only sample rates 44100 48000 88200 (from /proc/asound/card1/eld*).
When the user plays sound with sample rate 22050, they hear nothing. Sound with sample rates supported by the TV works OK.
My question: Is the HDA codec driver supposed to dynamically adjust its list of supported sample rates based on the ELD content, or is the ALSA library somehow supposed to detect the subset of rates supported in HW and convert the sample rate in SW before sending the audio to the driver?
The driver must return the correct list of supported sample rates. Otherwise alsa-lib thinks that the invalid sample rate is supported in the hardware or driver.
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
Jaroslav Kysela wrote:
On Tue, 3 Aug 2010, Stephen Warren wrote:
A user has the following setup:
A GPU which supports audio-over-HDMI. The codec supports sample rates 32000 44100 48000 88200 96000 176400 192000 (from /proc/asound/card1/codec#1). However, the user's TV supports only sample rates 44100 48000 88200 (from /proc/asound/card1/eld*).
When the user plays sound with sample rate 22050, they hear nothing. Sound with sample rates supported by the TV works OK.
My question: Is the HDA codec driver supposed to dynamically adjust its list of supported sample rates based on the ELD content, or is the ALSA library somehow supposed to detect the subset of rates supported in HW and convert the sample rate in SW before sending the audio to the driver?
The driver must return the correct list of supported sample rates. Otherwise alsa-lib thinks that the invalid sample rate is supported in the hardware or driver.
OK, that makes sense. Is this the responsibility of the codec driver (e.g. patch_nvhdmi.c) or something in the core HDMI code (e.g. patch_hdmi.c, or hda_*.c)
I don't see anything in patch_intelhdmi.c that relates to sample rate support at all. However, I see that patch_nvhdmi.c contains e.g.:
static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = { .substreams = 1, .channels_min = 2, .rates = SUPPORTED_RATES,
Should .rates not be initialized here, so that some automatic ELD-parsing logic gets triggered to fill this in later?
Thanks.
On Tue, 3 Aug 2010, Stephen Warren wrote:
Jaroslav Kysela wrote:
On Tue, 3 Aug 2010, Stephen Warren wrote:
A user has the following setup:
A GPU which supports audio-over-HDMI. The codec supports sample rates 32000 44100 48000 88200 96000 176400 192000 (from /proc/asound/card1/codec#1). However, the user's TV supports only sample rates 44100 48000 88200 (from /proc/asound/card1/eld*).
When the user plays sound with sample rate 22050, they hear nothing. Sound with sample rates supported by the TV works OK.
My question: Is the HDA codec driver supposed to dynamically adjust its list of supported sample rates based on the ELD content, or is the ALSA library somehow supposed to detect the subset of rates supported in HW and convert the sample rate in SW before sending the audio to the driver?
The driver must return the correct list of supported sample rates. Otherwise alsa-lib thinks that the invalid sample rate is supported in the hardware or driver.
OK, that makes sense. Is this the responsibility of the codec driver (e.g. patch_nvhdmi.c) or something in the core HDMI code (e.g. patch_hdmi.c, or hda_*.c)
I don't see anything in patch_intelhdmi.c that relates to sample rate support at all. However, I see that patch_nvhdmi.c contains e.g.:
static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = { .substreams = 1, .channels_min = 2, .rates = SUPPORTED_RATES,
Should .rates not be initialized here, so that some automatic ELD-parsing logic gets triggered to fill this in later?
Note that nvhdmi_pcm_digital_playback_8ch_89 is just a static template which is copied to the dynamic location in nvhdmi_build_pcms_8ch_89(). Just change rates member in pcm_rec->info->stream[SNDRV_PCM_STREAM_PLAYBACK].rates .
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
Stephen Warren kirjoitti tiistai, 3. elokuuta 2010 19:20:24:
Jaroslav Kysela wrote:
On Tue, 3 Aug 2010, Stephen Warren wrote:
A user has the following setup:
A GPU which supports audio-over-HDMI. The codec supports sample rates 32000 44100 48000 88200 96000 176400 192000 (from /proc/asound/card1/codec#1). However, the user's TV supports only sample rates 44100 48000 88200 (from /proc/asound/card1/eld*).
When the user plays sound with sample rate 22050, they hear nothing. Sound with sample rates supported by the TV works OK.
My question: Is the HDA codec driver supposed to dynamically adjust its list of supported sample rates based on the ELD content, or is the ALSA library somehow supposed to detect the subset of rates supported in HW and convert the sample rate in SW before sending the audio to the driver?
The driver must return the correct list of supported sample rates. Otherwise alsa-lib thinks that the invalid sample rate is supported in the hardware or driver.
OK, that makes sense. Is this the responsibility of the codec driver (e.g. patch_nvhdmi.c) or something in the core HDMI code (e.g. patch_hdmi.c, or hda_*.c)
I don't see anything in patch_intelhdmi.c that relates to sample rate support at all. However, I see that patch_nvhdmi.c contains e.g.:
static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = { .substreams = 1, .channels_min = 2, .rates = SUPPORTED_RATES,
Should .rates not be initialized here,
When .rates is not initialized, they are automatically read from the codec, i.e. exactly the values you referenced above from "/proc/asound/card1/codec#1".
The values seem to differ (32000 vs 22050 is reported as supported), though. If the codec reported values are wrong, the .rates should be kept. Otherwise the rates set in .rates should be fixed or the override just removed.
so that some automatic ELD-parsing logic gets triggered to fill this in later?
Unfortunately not, that has not been implemented yet. AFAICS it would belong to patch_hdmi.c.
The values seem to differ (32000 vs 22050 is reported as supported), though. If the codec reported values are wrong, the .rates should be kept. Otherwise the rates set in .rates should be fixed or the override just removed.
so that some automatic ELD-parsing logic gets triggered to fill this in later?
Unfortunately not, that has not been implemented yet. AFAICS it would belong to patch_hdmi.c.
So yesterday we discussed about detecting the formats supported, today it looks like we need to parse the sample rate. I bet tomorrow someone will want to have information on the number of channels supported...ELD-parsing is really a must-have.
Anssi Hannula kirjoitti tiistai, 3. elokuuta 2010 20:39:20:
Stephen Warren kirjoitti tiistai, 3. elokuuta 2010 19:20:24:
I don't see anything in patch_intelhdmi.c that relates to sample rate support at all. However, I see that patch_nvhdmi.c contains e.g.:
static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = {
.substreams = 1, .channels_min = 2, .rates = SUPPORTED_RATES,Should .rates not be initialized here,
When .rates is not initialized, they are automatically read from the codec, i.e. exactly the values you referenced above from "/proc/asound/card1/codec#1".
The values seem to differ (32000 vs 22050 is reported as supported), though. If the codec reported values are wrong, the .rates should be kept. Otherwise the rates set in .rates should be fixed or the override just removed.
As AFAICS HDMI specification allows 32000Hz but not 22050Hz, I'd say it is likely that the override is wrong and the codec reported rates are right.
(my receiver doesn't support 32000Hz so I can't confirm).
At Thu, 5 Aug 2010 12:49:07 +0300, Anssi Hannula wrote:
Anssi Hannula kirjoitti tiistai, 3. elokuuta 2010 20:39:20:
Stephen Warren kirjoitti tiistai, 3. elokuuta 2010 19:20:24:
I don't see anything in patch_intelhdmi.c that relates to sample rate support at all. However, I see that patch_nvhdmi.c contains e.g.:
static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = {
.substreams = 1, .channels_min = 2, .rates = SUPPORTED_RATES,Should .rates not be initialized here,
When .rates is not initialized, they are automatically read from the codec, i.e. exactly the values you referenced above from "/proc/asound/card1/codec#1".
The values seem to differ (32000 vs 22050 is reported as supported), though. If the codec reported values are wrong, the .rates should be kept. Otherwise the rates set in .rates should be fixed or the override just removed.
As AFAICS HDMI specification allows 32000Hz but not 22050Hz, I'd say it is likely that the override is wrong and the codec reported rates are right.
(my receiver doesn't support 32000Hz so I can't confirm).
So, the codec reports correct values based on EDID? IIRC, the static values there were needed for old Nvidia codecs that don't give the actual values. Maybe newer chips work correctly. If so, rates, maxbps and formats in *_89 (also *_7x, too?) can be removed, indeed.
Can any confirm?
thanks,
Takashi
Takashi Iwai kirjoitti torstai, 5. elokuuta 2010 13:46:39:
At Thu, 5 Aug 2010 12:49:07 +0300,
Anssi Hannula wrote:
Anssi Hannula kirjoitti tiistai, 3. elokuuta 2010 20:39:20:
Stephen Warren kirjoitti tiistai, 3. elokuuta 2010 19:20:24:
I don't see anything in patch_intelhdmi.c that relates to sample rate support at all. However, I see that patch_nvhdmi.c contains e.g.:
static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = {
.substreams = 1, .channels_min = 2, .rates = SUPPORTED_RATES,Should .rates not be initialized here,
When .rates is not initialized, they are automatically read from the codec, i.e. exactly the values you referenced above from "/proc/asound/card1/codec#1".
The values seem to differ (32000 vs 22050 is reported as supported), though. If the codec reported values are wrong, the .rates should be kept. Otherwise the rates set in .rates should be fixed or the override just removed.
As AFAICS HDMI specification allows 32000Hz but not 22050Hz, I'd say it is likely that the override is wrong and the codec reported rates are right.
(my receiver doesn't support 32000Hz so I can't confirm).
So, the codec reports correct values based on EDID?
No. The codec reports all the values it supports, regardless if the attached HDMI sink supports them or not.
IIRC, the static values there were needed for old Nvidia codecs that don't give the actual values. Maybe newer chips work correctly. If so, rates, maxbps and formats in *_89 (also *_7x, too?) can be removed, indeed.
Can any confirm?
I have an older MCP7A codec as well and can confirm that it reports wrong values (only 48000/88200, while I can confirm in reality 192000 works with it). Though I guess the nonsensical 22050 should probably be 32000 (but I can't test since my receiver doesn't support 32000).
At Thu, 5 Aug 2010 13:57:18 +0300, Anssi Hannula wrote:
Takashi Iwai kirjoitti torstai, 5. elokuuta 2010 13:46:39:
At Thu, 5 Aug 2010 12:49:07 +0300,
Anssi Hannula wrote:
Anssi Hannula kirjoitti tiistai, 3. elokuuta 2010 20:39:20:
Stephen Warren kirjoitti tiistai, 3. elokuuta 2010 19:20:24:
I don't see anything in patch_intelhdmi.c that relates to sample rate support at all. However, I see that patch_nvhdmi.c contains e.g.:
static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = {
.substreams = 1, .channels_min = 2, .rates = SUPPORTED_RATES,Should .rates not be initialized here,
When .rates is not initialized, they are automatically read from the codec, i.e. exactly the values you referenced above from "/proc/asound/card1/codec#1".
The values seem to differ (32000 vs 22050 is reported as supported), though. If the codec reported values are wrong, the .rates should be kept. Otherwise the rates set in .rates should be fixed or the override just removed.
As AFAICS HDMI specification allows 32000Hz but not 22050Hz, I'd say it is likely that the override is wrong and the codec reported rates are right.
(my receiver doesn't support 32000Hz so I can't confirm).
So, the codec reports correct values based on EDID?
No. The codec reports all the values it supports, regardless if the attached HDMI sink supports them or not.
OK. Then setting a static value was still needed.
IIRC, the static values there were needed for old Nvidia codecs that don't give the actual values. Maybe newer chips work correctly. If so, rates, maxbps and formats in *_89 (also *_7x, too?) can be removed, indeed.
Can any confirm?
I have an older MCP7A codec as well and can confirm that it reports wrong values (only 48000/88200, while I can confirm in reality 192000 works with it). Though I guess the nonsensical 22050 should probably be 32000 (but I can't test since my receiver doesn't support 32000).
What actually show in eld#* proc file? If this shows the correct values, we can add the code to take over rates & formats from ELD at hotplug.
Takashi
Takashi Iwai kirjoitti torstai, 5. elokuuta 2010 14:02:27:
At Thu, 5 Aug 2010 13:57:18 +0300,
Anssi Hannula wrote:
Takashi Iwai kirjoitti torstai, 5. elokuuta 2010 13:46:39:
At Thu, 5 Aug 2010 12:49:07 +0300,
Anssi Hannula wrote:
Anssi Hannula kirjoitti tiistai, 3. elokuuta 2010 20:39:20:
Stephen Warren kirjoitti tiistai, 3. elokuuta 2010 19:20:24:
I don't see anything in patch_intelhdmi.c that relates to sample rate support at all. However, I see that patch_nvhdmi.c contains e.g.:
static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = {
.substreams = 1, .channels_min = 2, .rates = SUPPORTED_RATES,Should .rates not be initialized here,
When .rates is not initialized, they are automatically read from the codec, i.e. exactly the values you referenced above from "/proc/asound/card1/codec#1".
The values seem to differ (32000 vs 22050 is reported as supported), though. If the codec reported values are wrong, the .rates should be kept. Otherwise the rates set in .rates should be fixed or the override just removed.
As AFAICS HDMI specification allows 32000Hz but not 22050Hz, I'd say it is likely that the override is wrong and the codec reported rates are right.
(my receiver doesn't support 32000Hz so I can't confirm).
So, the codec reports correct values based on EDID?
No. The codec reports all the values it supports, regardless if the attached HDMI sink supports them or not.
OK. Then setting a static value was still needed.
How so (for _89 I mean)?
IIRC, the static values there were needed for old Nvidia codecs that don't give the actual values. Maybe newer chips work correctly. If so, rates, maxbps and formats in *_89 (also *_7x, too?) can be removed, indeed.
Can any confirm?
I have an older MCP7A codec as well and can confirm that it reports wrong values (only 48000/88200, while I can confirm in reality 192000 works with it). Though I guess the nonsensical 22050 should probably be 32000 (but I can't test since my receiver doesn't support 32000).
What actually show in eld#* proc file?
The values that the connected HDMI sink supports.
If this shows the correct values, we can add the code to take over rates & formats from ELD at hotplug.
That's exactly what should be done (the intersection of supported formats of the codec and the sink).
At Thu, 5 Aug 2010 14:17:10 +0300, Anssi Hannula wrote:
Takashi Iwai kirjoitti torstai, 5. elokuuta 2010 14:02:27:
At Thu, 5 Aug 2010 13:57:18 +0300,
Anssi Hannula wrote:
Takashi Iwai kirjoitti torstai, 5. elokuuta 2010 13:46:39:
At Thu, 5 Aug 2010 12:49:07 +0300,
Anssi Hannula wrote:
Anssi Hannula kirjoitti tiistai, 3. elokuuta 2010 20:39:20:
Stephen Warren kirjoitti tiistai, 3. elokuuta 2010 19:20:24: > I don't see anything in patch_intelhdmi.c that relates to sample > rate support at all. However, I see that patch_nvhdmi.c contains > e.g.: > > static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = > { > > .substreams = 1, > .channels_min = 2, > .rates = SUPPORTED_RATES, > > Should .rates not be initialized here,
When .rates is not initialized, they are automatically read from the codec, i.e. exactly the values you referenced above from "/proc/asound/card1/codec#1".
The values seem to differ (32000 vs 22050 is reported as supported), though. If the codec reported values are wrong, the .rates should be kept. Otherwise the rates set in .rates should be fixed or the override just removed.
As AFAICS HDMI specification allows 32000Hz but not 22050Hz, I'd say it is likely that the override is wrong and the codec reported rates are right.
(my receiver doesn't support 32000Hz so I can't confirm).
So, the codec reports correct values based on EDID?
No. The codec reports all the values it supports, regardless if the attached HDMI sink supports them or not.
OK. Then setting a static value was still needed.
How so (for _89 I mean)?
For _89, yeah, that's not needed if it gives really all.
IIRC, the static values there were needed for old Nvidia codecs that don't give the actual values. Maybe newer chips work correctly. If so, rates, maxbps and formats in *_89 (also *_7x, too?) can be removed, indeed.
Can any confirm?
I have an older MCP7A codec as well and can confirm that it reports wrong values (only 48000/88200, while I can confirm in reality 192000 works with it). Though I guess the nonsensical 22050 should probably be 32000 (but I can't test since my receiver doesn't support 32000).
What actually show in eld#* proc file?
The values that the connected HDMI sink supports.
If this shows the correct values, we can add the code to take over rates & formats from ELD at hotplug.
That's exactly what should be done (the intersection of supported formats of the codec and the sink).
OK, what about the patch below (totally untested)?
This updates the PCM info only at PCM open time. Apps usually check parameters only at open (due to hw_params stuff), so it doesn't make much sense to change dynamically at hotplug in the end.
Takashi
--- diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c index d8da18a..c41ba10 100644 --- a/sound/pci/hda/hda_eld.c +++ b/sound/pci/hda/hda_eld.c @@ -596,4 +596,42 @@ void snd_hda_eld_proc_free(struct hda_codec *codec, struct hdmi_eld *eld) } EXPORT_SYMBOL_HDA(snd_hda_eld_proc_free);
+/* update PCM info based on ELD */ +void hdmi_eld_update_pcm_info(struct hdmi_eld *eld, struct hda_pcm_stream *pcm) +{ + int i; + + pcm->rates = 0; + pcm->formats = 0; + pcm->maxbps = 0; + pcm->channels_min = -1; + pcm->channels_max = 0; + for (i = 0; i < eld->sad_count; i++) { + struct cea_sad *a = &eld->sad[i]; + pcm->rates |= a->rates; + if (a->channels < pcm->channels_min) + pcm->channels_min = a->channels; + if (a->channels > pcm->channels_max) + pcm->channels_max = a->channels; + if (a->format == AUDIO_CODING_TYPE_LPCM) { + if (a->sample_bits & AC_SUPPCM_BITS_16) { + pcm->formats |= SNDRV_PCM_FMTBIT_S16_LE; + if (pcm->maxbps < 16) + pcm->maxbps = 16; + } + if (a->sample_bits & AC_SUPPCM_BITS_20) { + pcm->formats |= SNDRV_PCM_FMTBIT_S32_LE; + if (pcm->maxbps < 20) + pcm->maxbps = 20; + } + if (a->sample_bits & AC_SUPPCM_BITS_24) { + pcm->formats |= SNDRV_PCM_FMTBIT_S32_LE; + if (pcm->maxbps < 24) + pcm->maxbps = 24; + } + } + } +} +EXPORT_SYMBOL_HDA(hdmi_eld_update_pcm_info); + #endif /* CONFIG_PROC_FS */ diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 7a97f12..af2699e 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -604,6 +604,7 @@ struct hdmi_eld { int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid); int snd_hdmi_get_eld(struct hdmi_eld *, struct hda_codec *, hda_nid_t); void snd_hdmi_show_eld(struct hdmi_eld *eld); +void hdmi_eld_update_pcm_info(struct hdmi_eld *eld, struct hda_pcm_stream *pcm);
#ifdef CONFIG_PROC_FS int snd_hda_eld_proc_new(struct hda_codec *codec, struct hdmi_eld *eld, diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 522e074..4176e6c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -766,6 +766,29 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t nid, }
/* + * HDA PCM callbacks + */ +static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + struct snd_pcm_substream *substream) +{ + struct hdmi_spec *spec = codec->spec; + struct hdmi_eld *eld; + unsigned int idx; + + for (idx = 0; idx < spec->num_cvts; idx++) + if (hinfo->nid == spec->cvt[idx]) + break; + if (snd_BUG_ON(idx >= spec->num_cvts) || + snd_BUG_ON(idx >= spec->num_pins)) + return -EINVAL; + eld = &spec->sink_eld[idx]; + if (eld->sad_count > 0) + hdmi_eld_update_pcm_info(eld, hinfo); + return 0; +} + +/* * HDA/HDMI auto parsing */
diff --git a/sound/pci/hda/patch_intelhdmi.c b/sound/pci/hda/patch_intelhdmi.c index 5972d5e..d382d3c 100644 --- a/sound/pci/hda/patch_intelhdmi.c +++ b/sound/pci/hda/patch_intelhdmi.c @@ -80,6 +80,7 @@ static struct hda_pcm_stream intel_hdmi_pcm_playback = { .substreams = 1, .channels_min = 2, .ops = { + .open = hdmi_pcm_open, .prepare = intel_hdmi_playback_pcm_prepare, .cleanup = intel_hdmi_playback_pcm_cleanup, }, diff --git a/sound/pci/hda/patch_nvhdmi.c b/sound/pci/hda/patch_nvhdmi.c index a281836..004d1c6 100644 --- a/sound/pci/hda/patch_nvhdmi.c +++ b/sound/pci/hda/patch_nvhdmi.c @@ -351,6 +351,7 @@ static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = { .maxbps = SUPPORTED_MAXBPS, .formats = SUPPORTED_FORMATS, .ops = { + .open = hdmi_pcm_open, .prepare = nvhdmi_dig_playback_pcm_prepare_8ch_89, .cleanup = nvhdmi_playback_pcm_cleanup, },
Takashi Iwai wrote:
Sent: Thursday, August 05, 2010 6:28 AM
At Thu, 5 Aug 2010 14:17:10 +0300, Anssi Hannula wrote:
Takashi Iwai kirjoitti torstai, 5. elokuuta 2010 14:02:27:
At Thu, 5 Aug 2010 13:57:18 +0300,
Anssi Hannula wrote:
Takashi Iwai kirjoitti torstai, 5. elokuuta 2010 13:46:39:
At Thu, 5 Aug 2010 12:49:07 +0300,
Anssi Hannula wrote:
Anssi Hannula kirjoitti tiistai, 3. elokuuta 2010 20:39:20: > Stephen Warren kirjoitti tiistai, 3. elokuuta 2010 19:20:24: > > I don't see anything in patch_intelhdmi.c that relates to sample > > rate support at all. However, I see that patch_nvhdmi.c contains > > e.g.: > > > > static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = > > { > > > > .substreams = 1, > > .channels_min = 2, > > .rates = SUPPORTED_RATES, > > > > Should .rates not be initialized here, > > When .rates is not initialized, they are automatically read from > the codec, i.e. exactly the values you referenced above from > "/proc/asound/card1/codec#1". > > The values seem to differ (32000 vs 22050 is reported as > supported), though. If the codec reported values are wrong, the > .rates should be kept. Otherwise the rates set in .rates should be > fixed or the override just removed.
As AFAICS HDMI specification allows 32000Hz but not 22050Hz, I'd say it is likely that the override is wrong and the codec reported rates are right.
Wei,
Looking at our Windows driver, we should remove 22050 from the supported rates list and add 32000 instead. Can you confirm this? Thanks.
(my receiver doesn't support 32000Hz so I can't confirm).
So, the codec reports correct values based on EDID?
No. The codec reports all the values it supports, regardless if the attached HDMI sink supports them or not.
OK. Then setting a static value was still needed.
How so (for _89 I mean)?
For _89, yeah, that's not needed if it gives really all.
IIRC, the static values there were needed for old Nvidia codecs that don't give the actual values. Maybe newer chips work correctly. If so, rates, maxbps and formats in *_89 (also *_7x, too?) can be removed, indeed.
Can any confirm?
I have an older MCP7A codec as well and can confirm that it reports wrong values (only 48000/88200, while I can confirm in reality 192000 works with it). Though I guess the nonsensical 22050 should probably be 32000 (but I can't test since my receiver doesn't support 32000).
What actually show in eld#* proc file?
The values that the connected HDMI sink supports.
If this shows the correct values, we can add the code to take over rates & formats from ELD at hotplug.
That's exactly what should be done (the intersection of supported formats of the codec and the sink).
OK, what about the patch below (totally untested)?
This updates the PCM info only at PCM open time. Apps usually check parameters only at open (due to hw_params stuff), so it doesn't make much sense to change dynamically at hotplug in the end.
Doesn't that patch only use the ELD to set up the stream's supported parameters? I think the code needs to determine the intersection of what's supported by the codec (from HDA node parsing if that's the correct terminology) *and* the display device (from the ELD), not just one of them.
Takashi
diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c index d8da18a..c41ba10 100644 --- a/sound/pci/hda/hda_eld.c +++ b/sound/pci/hda/hda_eld.c @@ -596,4 +596,42 @@ void snd_hda_eld_proc_free(struct hda_codec *codec, struct hdmi_eld *eld) } EXPORT_SYMBOL_HDA(snd_hda_eld_proc_free);
+/* update PCM info based on ELD */ +void hdmi_eld_update_pcm_info(struct hdmi_eld *eld, struct hda_pcm_stream *pcm) +{
- int i;
- pcm->rates = 0;
- pcm->formats = 0;
- pcm->maxbps = 0;
- pcm->channels_min = -1;
- pcm->channels_max = 0;
- for (i = 0; i < eld->sad_count; i++) {
struct cea_sad *a = &eld->sad[i];pcm->rates |= a->rates;if (a->channels < pcm->channels_min)pcm->channels_min = a->channels;if (a->channels > pcm->channels_max)pcm->channels_max = a->channels;if (a->format == AUDIO_CODING_TYPE_LPCM) {if (a->sample_bits & AC_SUPPCM_BITS_16) {pcm->formats |= SNDRV_PCM_FMTBIT_S16_LE;if (pcm->maxbps < 16)pcm->maxbps = 16;}if (a->sample_bits & AC_SUPPCM_BITS_20) {pcm->formats |= SNDRV_PCM_FMTBIT_S32_LE;if (pcm->maxbps < 20)pcm->maxbps = 20;}if (a->sample_bits & AC_SUPPCM_BITS_24) {pcm->formats |= SNDRV_PCM_FMTBIT_S32_LE;if (pcm->maxbps < 24)pcm->maxbps = 24;}}- }
+} +EXPORT_SYMBOL_HDA(hdmi_eld_update_pcm_info);
#endif /* CONFIG_PROC_FS */ diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 7a97f12..af2699e 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -604,6 +604,7 @@ struct hdmi_eld { int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid); int snd_hdmi_get_eld(struct hdmi_eld *, struct hda_codec *, hda_nid_t); void snd_hdmi_show_eld(struct hdmi_eld *eld); +void hdmi_eld_update_pcm_info(struct hdmi_eld *eld, struct hda_pcm_stream *pcm);
#ifdef CONFIG_PROC_FS int snd_hda_eld_proc_new(struct hda_codec *codec, struct hdmi_eld *eld, diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 522e074..4176e6c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -766,6 +766,29 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t nid, }
/*
- HDA PCM callbacks
- */
+static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
struct hda_codec *codec,struct snd_pcm_substream *substream)+{
- struct hdmi_spec *spec = codec->spec;
- struct hdmi_eld *eld;
- unsigned int idx;
- for (idx = 0; idx < spec->num_cvts; idx++)
if (hinfo->nid == spec->cvt[idx])break;- if (snd_BUG_ON(idx >= spec->num_cvts) ||
snd_BUG_ON(idx >= spec->num_pins))return -EINVAL;- eld = &spec->sink_eld[idx];
- if (eld->sad_count > 0)
hdmi_eld_update_pcm_info(eld, hinfo);- return 0;
+}
+/*
- HDA/HDMI auto parsing
*/
diff --git a/sound/pci/hda/patch_intelhdmi.c b/sound/pci/hda/patch_intelhdmi.c index 5972d5e..d382d3c 100644 --- a/sound/pci/hda/patch_intelhdmi.c +++ b/sound/pci/hda/patch_intelhdmi.c @@ -80,6 +80,7 @@ static struct hda_pcm_stream intel_hdmi_pcm_playback = { .substreams = 1, .channels_min = 2, .ops = {
.open = hdmi_pcm_open,diff --git a/sound/pci/hda/patch_nvhdmi.c b/sound/pci/hda/patch_nvhdmi.c index a281836..004d1c6 100644 --- a/sound/pci/hda/patch_nvhdmi.c +++ b/sound/pci/hda/patch_nvhdmi.c @@ -351,6 +351,7 @@ static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = { .maxbps = SUPPORTED_MAXBPS, .formats = SUPPORTED_FORMATS, .ops = {
.open = hdmi_pcm_open,
At Thu, 5 Aug 2010 07:23:26 -0700, Stephen Warren wrote:
Takashi Iwai wrote:
Sent: Thursday, August 05, 2010 6:28 AM
At Thu, 5 Aug 2010 14:17:10 +0300, Anssi Hannula wrote:
Takashi Iwai kirjoitti torstai, 5. elokuuta 2010 14:02:27:
At Thu, 5 Aug 2010 13:57:18 +0300,
Anssi Hannula wrote:
Takashi Iwai kirjoitti torstai, 5. elokuuta 2010 13:46:39:
At Thu, 5 Aug 2010 12:49:07 +0300,
Anssi Hannula wrote: > Anssi Hannula kirjoitti tiistai, 3. elokuuta 2010 20:39:20: > > Stephen Warren kirjoitti tiistai, 3. elokuuta 2010 19:20:24: > > > I don't see anything in patch_intelhdmi.c that relates to sample > > > rate support at all. However, I see that patch_nvhdmi.c contains > > > e.g.: > > > > > > static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = > > > { > > > > > > .substreams = 1, > > > .channels_min = 2, > > > .rates = SUPPORTED_RATES, > > > > > > Should .rates not be initialized here, > > > > When .rates is not initialized, they are automatically read from > > the codec, i.e. exactly the values you referenced above from > > "/proc/asound/card1/codec#1". > > > > The values seem to differ (32000 vs 22050 is reported as > > supported), though. If the codec reported values are wrong, the > > .rates should be kept. Otherwise the rates set in .rates should be > > fixed or the override just removed. > > As AFAICS HDMI specification allows 32000Hz but not 22050Hz, I'd say > it is likely that the override is wrong and the codec reported rates > are right.
Wei,
Looking at our Windows driver, we should remove 22050 from the supported rates list and add 32000 instead. Can you confirm this? Thanks.
> (my receiver doesn't support 32000Hz so I can't confirm).
So, the codec reports correct values based on EDID?
No. The codec reports all the values it supports, regardless if the attached HDMI sink supports them or not.
OK. Then setting a static value was still needed.
How so (for _89 I mean)?
For _89, yeah, that's not needed if it gives really all.
IIRC, the static values there were needed for old Nvidia codecs that don't give the actual values. Maybe newer chips work correctly. If so, rates, maxbps and formats in *_89 (also *_7x, too?) can be removed, indeed.
Can any confirm?
I have an older MCP7A codec as well and can confirm that it reports wrong values (only 48000/88200, while I can confirm in reality 192000 works with it). Though I guess the nonsensical 22050 should probably be 32000 (but I can't test since my receiver doesn't support 32000).
What actually show in eld#* proc file?
The values that the connected HDMI sink supports.
If this shows the correct values, we can add the code to take over rates & formats from ELD at hotplug.
That's exactly what should be done (the intersection of supported formats of the codec and the sink).
OK, what about the patch below (totally untested)?
This updates the PCM info only at PCM open time. Apps usually check parameters only at open (due to hw_params stuff), so it doesn't make much sense to change dynamically at hotplug in the end.
Doesn't that patch only use the ELD to set up the stream's supported parameters? I think the code needs to determine the intersection of what's supported by the codec (from HDA node parsing if that's the correct terminology) *and* the display device (from the ELD), not just one of them.
Good point. A revised patch is below. Can anyone test it?
thanks,
Takashi
--- diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c index d8da18a..803b298 100644 --- a/sound/pci/hda/hda_eld.c +++ b/sound/pci/hda/hda_eld.c @@ -596,4 +596,53 @@ void snd_hda_eld_proc_free(struct hda_codec *codec, struct hdmi_eld *eld) } EXPORT_SYMBOL_HDA(snd_hda_eld_proc_free);
+/* update PCM info based on ELD */ +void hdmi_eld_update_pcm_info(struct hdmi_eld *eld, struct hda_pcm_stream *pcm, + struct hda_pcm_stream *codec_pars) +{ + int i; + + pcm->rates = 0; + pcm->formats = 0; + pcm->maxbps = 0; + pcm->channels_min = -1; + pcm->channels_max = 0; + for (i = 0; i < eld->sad_count; i++) { + struct cea_sad *a = &eld->sad[i]; + pcm->rates |= a->rates; + if (a->channels < pcm->channels_min) + pcm->channels_min = a->channels; + if (a->channels > pcm->channels_max) + pcm->channels_max = a->channels; + if (a->format == AUDIO_CODING_TYPE_LPCM) { + if (a->sample_bits & AC_SUPPCM_BITS_16) { + pcm->formats |= SNDRV_PCM_FMTBIT_S16_LE; + if (pcm->maxbps < 16) + pcm->maxbps = 16; + } + if (a->sample_bits & AC_SUPPCM_BITS_20) { + pcm->formats |= SNDRV_PCM_FMTBIT_S32_LE; + if (pcm->maxbps < 20) + pcm->maxbps = 20; + } + if (a->sample_bits & AC_SUPPCM_BITS_24) { + pcm->formats |= SNDRV_PCM_FMTBIT_S32_LE; + if (pcm->maxbps < 24) + pcm->maxbps = 24; + } + } + } + + if (!codec_pars) + return; + + /* restrict the parameters by the values the codec provides */ + pcm->rates &= codec_pars->rates; + pcm->formats &= codec_pars->formats; + pcm->channels_min = max(pcm->channels_min, codec_pars->channels_min); + pcm->channels_max = min(pcm->channels_max, codec_pars->channels_max); + pcm->maxbps = min(pcm->maxbps, codec_pars->maxbps); +} +EXPORT_SYMBOL_HDA(hdmi_eld_update_pcm_info); + #endif /* CONFIG_PROC_FS */ diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 7a97f12..28ab4ae 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -604,6 +604,8 @@ struct hdmi_eld { int snd_hdmi_get_eld_size(struct hda_codec *codec, hda_nid_t nid); int snd_hdmi_get_eld(struct hdmi_eld *, struct hda_codec *, hda_nid_t); void snd_hdmi_show_eld(struct hdmi_eld *eld); +void hdmi_eld_update_pcm_info(struct hdmi_eld *eld, struct hda_pcm_stream *pcm, + struct hda_pcm_stream *codec_pars);
#ifdef CONFIG_PROC_FS int snd_hda_eld_proc_new(struct hda_codec *codec, struct hdmi_eld *eld, diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 522e074..2bc0f07 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -46,6 +46,7 @@ struct hdmi_spec { * export one pcm per pipe */ struct hda_pcm pcm_rec[MAX_HDMI_CVTS]; + struct hda_pcm_stream codec_pcm_pars[MAX_HDMI_CVTS];
/* * nvhdmi specific @@ -766,6 +767,47 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t nid, }
/* + * HDA PCM callbacks + */ +static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, + struct hda_codec *codec, + struct snd_pcm_substream *substream) +{ + struct hdmi_spec *spec = codec->spec; + struct hdmi_eld *eld; + struct hda_pcm_stream *codec_pars; + unsigned int idx; + + for (idx = 0; idx < spec->num_cvts; idx++) + if (hinfo->nid == spec->cvt[idx]) + break; + if (snd_BUG_ON(idx >= spec->num_cvts) || + snd_BUG_ON(idx >= spec->num_pins)) + return -EINVAL; + + /* save the PCM info the codec provides */ + codec_pars = &spec->codec_pcm_pars[idx]; + if (!codec_pars->rates) + *codec_pars = *hinfo; + + eld = &spec->sink_eld[idx]; + if (eld->sad_count > 0) { + hdmi_eld_update_pcm_info(eld, hinfo, codec_pars); + if (hinfo->channels_min > hinfo->channels_max || + !hinfo->rates || !hinfo->formats) + return -ENODEV; + } else { + /* fallback to the codec default */ + hinfo->channels_min = codec_pars->channels_min; + hinfo->channels_max = codec_pars->channels_max; + hinfo->rates = codec_pars->rates; + hinfo->formats = codec_pars->formats; + hinfo->maxbps = codec_pars->maxbps; + } + return 0; +} + +/* * HDA/HDMI auto parsing */
diff --git a/sound/pci/hda/patch_intelhdmi.c b/sound/pci/hda/patch_intelhdmi.c index 5972d5e..d382d3c 100644 --- a/sound/pci/hda/patch_intelhdmi.c +++ b/sound/pci/hda/patch_intelhdmi.c @@ -80,6 +80,7 @@ static struct hda_pcm_stream intel_hdmi_pcm_playback = { .substreams = 1, .channels_min = 2, .ops = { + .open = hdmi_pcm_open, .prepare = intel_hdmi_playback_pcm_prepare, .cleanup = intel_hdmi_playback_pcm_cleanup, }, diff --git a/sound/pci/hda/patch_nvhdmi.c b/sound/pci/hda/patch_nvhdmi.c index a281836..55d5248 100644 --- a/sound/pci/hda/patch_nvhdmi.c +++ b/sound/pci/hda/patch_nvhdmi.c @@ -347,10 +347,8 @@ static int nvhdmi_dig_playback_pcm_prepare_2ch(struct hda_pcm_stream *hinfo, static struct hda_pcm_stream nvhdmi_pcm_digital_playback_8ch_89 = { .substreams = 1, .channels_min = 2, - .rates = SUPPORTED_RATES, - .maxbps = SUPPORTED_MAXBPS, - .formats = SUPPORTED_FORMATS, .ops = { + .open = hdmi_pcm_open, .prepare = nvhdmi_dig_playback_pcm_prepare_8ch_89, .cleanup = nvhdmi_playback_pcm_cleanup, },
Takashi Iwai wrote:
At Thu, 5 Aug 2010 07:23:26 -0700, Stephen Warren wrote:
Takashi Iwai wrote:
Sent: Thursday, August 05, 2010 6:28 AM
...
OK, what about the patch below (totally untested)?
This updates the PCM info only at PCM open time. Apps usually check parameters only at open (due to hw_params stuff), so it doesn't make much sense to change dynamically at hotplug in the end.
Doesn't that patch only use the ELD to set up the stream's supported parameters? I think the code needs to determine the intersection of what's supported by the codec (from HDA node parsing if that's the correct terminology) *and* the display device (from the ELD), not just one of them.
Good point. A revised patch is below. Can anyone test it?
FYI, I asked our affected end-user to test this a few days ago, but haven't heard back yet. I'll let you know his results when I get them. I'll also see if we can get this tested internally to NVIDIA. Unfortunately, I don't have the HW to do so at present (although perhaps if I fake the ELD data, I could test it...)
At Mon, 9 Aug 2010 14:31:30 -0700, Stephen Warren wrote:
Takashi Iwai wrote:
At Thu, 5 Aug 2010 07:23:26 -0700, Stephen Warren wrote:
Takashi Iwai wrote:
Sent: Thursday, August 05, 2010 6:28 AM
...
OK, what about the patch below (totally untested)?
This updates the PCM info only at PCM open time. Apps usually check parameters only at open (due to hw_params stuff), so it doesn't make much sense to change dynamically at hotplug in the end.
Doesn't that patch only use the ELD to set up the stream's supported parameters? I think the code needs to determine the intersection of what's supported by the codec (from HDA node parsing if that's the correct terminology) *and* the display device (from the ELD), not just one of them.
Good point. A revised patch is below. Can anyone test it?
FYI, I asked our affected end-user to test this a few days ago, but haven't heard back yet. I'll let you know his results when I get them. I'll also see if we can get this tested internally to NVIDIA. Unfortunately, I don't have the HW to do so at present (although perhaps if I fake the ELD data, I could test it...)
OK, thanks for update. If anyone can give it a try, please let me know the result.
Takashi
Takashi Iwai wrote:
At Mon, 9 Aug 2010 14:31:30 -0700, Stephen Warren wrote:
Takashi Iwai wrote:
At Thu, 5 Aug 2010 07:23:26 -0700, Stephen Warren wrote:
Takashi Iwai wrote:
Sent: Thursday, August 05, 2010 6:28 AM
...
OK, what about the patch below (totally untested)?
This updates the PCM info only at PCM open time. Apps usually check parameters only at open (due to hw_params stuff), so it doesn't make much sense to change dynamically at hotplug in the end.
Doesn't that patch only use the ELD to set up the stream's supported parameters? I think the code needs to determine the intersection of what's supported by the codec (from HDA node parsing if that's the correct terminology) *and* the display device (from the ELD), not just one of them.
Good point. A revised patch is below. Can anyone test it?
FYI, I asked our affected end-user to test this a few days ago, but haven't heard back yet. I'll let you know his results when I get them. I'll also see if we can get this tested internally to NVIDIA. Unfortunately, I don't have the HW to do so at present (although perhaps if I fake the ELD data, I could test it...)
OK, thanks for update. If anyone can give it a try, please let me know the result.
I gave this a try by hacking our display driver to pass some hard-coded SADs to the audio HW. This seems to work OK. Some thoughts:
I tested with a nice utility called alsacap from:
http://www.volkerschatz.com/noise/alsa.html
... slightly hacked to use snd_pcm_hw_params_test_rate for each rate explicitly.
I notice that when the ELD contains no results in common with the codec, alsacap says:
Device 7, ID `NVIDIA HDMI', name `NVIDIA HDMI', 1 subdevices (1 available) Error opening sound device for card 1, device 7: Hotplug device has been removed. Skipping.
... which I guess does make sense, since there's no possibility of actually using the device. However, when there is no device connected, this patch sets the PCM's capabilities to the codec's capabilities. Does this make sense; should ALSA behave the same as above? I suppose for safety, it may be better to attempt to let things work in case there is some bug blocking the ELD data.
----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
At Thu, 12 Aug 2010 14:02:45 -0700, Stephen Warren wrote:
Takashi Iwai wrote:
At Mon, 9 Aug 2010 14:31:30 -0700, Stephen Warren wrote:
Takashi Iwai wrote:
At Thu, 5 Aug 2010 07:23:26 -0700, Stephen Warren wrote:
Takashi Iwai wrote:
Sent: Thursday, August 05, 2010 6:28 AM
...
OK, what about the patch below (totally untested)?
This updates the PCM info only at PCM open time. Apps usually check parameters only at open (due to hw_params stuff), so it doesn't make much sense to change dynamically at hotplug in the end.
Doesn't that patch only use the ELD to set up the stream's supported parameters? I think the code needs to determine the intersection of what's supported by the codec (from HDA node parsing if that's the correct terminology) *and* the display device (from the ELD), not just one of them.
Good point. A revised patch is below. Can anyone test it?
FYI, I asked our affected end-user to test this a few days ago, but haven't heard back yet. I'll let you know his results when I get them. I'll also see if we can get this tested internally to NVIDIA. Unfortunately, I don't have the HW to do so at present (although perhaps if I fake the ELD data, I could test it...)
OK, thanks for update. If anyone can give it a try, please let me know the result.
I gave this a try by hacking our display driver to pass some hard-coded SADs to the audio HW. This seems to work OK. Some thoughts:
I tested with a nice utility called alsacap from:
http://www.volkerschatz.com/noise/alsa.html
... slightly hacked to use snd_pcm_hw_params_test_rate for each rate explicitly.
I notice that when the ELD contains no results in common with the codec, alsacap says:
Device 7, ID `NVIDIA HDMI', name `NVIDIA HDMI', 1 subdevices (1 available) Error opening sound device for card 1, device 7: Hotplug device has been removed. Skipping.
... which I guess does make sense, since there's no possibility of actually using the device. However, when there is no device connected, this patch sets the PCM's capabilities to the codec's capabilities. Does this make sense; should ALSA behave the same as above? I suppose for safety, it may be better to attempt to let things work in case there is some bug blocking the ELD data.
I chose the current behavior for apps (e.g. a sound daemon) that probe devices at start up. That is, I wanted to make probing working without HDMI plugged since most apps assume the static sound configuration, thus they won't update the status.
I'm going to apply the patch now to sound git tree. Let me know if you find any issue with it.
thanks,
Takashi
participants (5)
-
Anssi Hannula -
Jaroslav Kysela -
pl bossart -
Stephen Warren -
Takashi Iwai