On 07.12.2010 18:58, Stephen Warren wrote:
> Anssi Hannula wrote:
>> Commit bbbe33900d1f3c added functionality to restrict PCM parameters
>> based on ELD info (derived from EDID data) of the audio sink.
>>
>> However, it wrongly assumes that the bits 0-2 of the first byte of
>> CEA Short Audio Descriptors mean a supported number of channels. In
>> reality, they mean the maximum number of channels (as per CEA-861-D
>> 7.5.2). This means that the channel count can only be used to restrict
>> max_channels, not min_channels.
>>
>> Restricting min_channels causes us to deny opening the device in stereo
>> mode if the sink only has SADs that declare larger numbers of channels
>> (like Primare SP32 AV Processor does).
>>
>> Fix that by not restricting min_channels based on ELD information.
>
> Yes, re-reading the CEA-861-D spec, that makes sense. Comments inline
> though.
>
> BTW, I noticed that no SADs are required if only basic audio is
> supported. Is that case handled by the ELD parser? I.e. instead of:
>
> pcm->channels_max = 0;
>
> Should it be:
>
> pcm->channels_max = 0;
> if supports basic audio:
> pcm->channels_max = 2;
>
> or something like that?
Yes, same for rates. I'll followup on that with a separate patch, thanks :)
>> Signed-off-by: Anssi Hannula <anssi.hannula@xxxxxx>
>> Reported-by: Jean-Yves Avenard <jyavenard@xxxxxxxxx>
>> Tested-by: Jean-Yves Avenard <jyavenard@xxxxxxxxx>
>> Cc: stable@xxxxxxxxxx
>> ---
>>
>> I think this should go to 2.6.36 stable tree as well. It does not affect
>> earlier stable releases.
>>
>> sound/pci/hda/hda_eld.c | 4 ----
>> sound/pci/hda/patch_hdmi.c | 1 -
>> 2 files changed, 0 insertions(+), 5 deletions(-)
>>
>> diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c
>> index cb0c23a..47ef8aa 100644
>> --- a/sound/pci/hda/hda_eld.c
>> +++ b/sound/pci/hda/hda_eld.c
>> @@ -601,13 +601,10 @@ void hdmi_eld_update_pcm_info(struct hdmi_eld *eld, struct
>> hda_pcm_stream *pcm,
>> 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) {
>
> That all looks OK.
>
>> @@ -635,7 +632,6 @@ void hdmi_eld_update_pcm_info(struct hdmi_eld *eld, struct
>> hda_pcm_stream *pcm,
>> /* 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);
>
> Shouldn't that be:
>
> pcm->channels_min = pcm->channels_min;
>
> Or, is that assignment already made earlier as a default?
pcm->channels_min = pcm->channels_min;
does not make sense, I assume you meant
pcm->channels_min = codec_pars->channels_min;
*codec_pars is originally copied from *pcm in hdmi_pcm_open() of
patch_hdmi.c, so yes, the assignment would be superfluous.
>> pcm->channels_max = min(pcm->channels_max, codec_pars->channels_max);
>> pcm->maxbps = min(pcm->maxbps, codec_pars->maxbps);
>> }
>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
>> index d3e49aa..31df774 100644
>> --- a/sound/pci/hda/patch_hdmi.c
>> +++ b/sound/pci/hda/patch_hdmi.c
>> @@ -834,7 +834,6 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
>> return -ENODEV;
>> } else {
>> /* fallback to the codec default */
>> - hinfo->channels_min = codec_pars->channels_min;
>
> Isn't this still required to default the value?
Not AFAICS. By default *hinfo has the codec provided parameters, and if
we never update it from the ELD info, we don't need to restore it either.
>> hinfo->channels_max = codec_pars->channels_max;
>> hinfo->rates = codec_pars->rates;
>> hinfo->formats = codec_pars->formats;
>> --
>> 1.7.3
>
--
Anssi Hannula
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
