[alsa-devel] HDMI audio: TV vs. codec supported sample rates

[Stephen Warren]

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,
>  		.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,
>  	},

-- 
nvpublic