Re: [PATCH] ALSA: hda - Do not wrongly restrict min_channels based on ELD

[Anssi Hannula <anssi.hannula@iki.fi>]

On 07.12.2010 19:26, Stephen Warren wrote:
> Anssi Hannula wrote:
>>
>> 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.
[...]
>>>> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
>>>> Reported-by: Jean-Yves Avenard <jyavenard@gmail.com>
>>>> Tested-by: Jean-Yves Avenard <jyavenard@gmail.com>
>>>> Cc: stable@kernel.org
[...]
>>>>  	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.
> 
> OK. Doesn't the same argument apply to the following 3 lines too then?

Nope. hinfo is persistent across open() calls, so we need to reset them
if they were ever updated from ELD earlier.

>>>>  		hinfo->channels_max = codec_pars->channels_max;
>>>>  		hinfo->rates = codec_pars->rates;
>>>>  		hinfo->formats = codec_pars->formats;
>>>> --
>>>> 1.7.3
> 


-- 
Anssi Hannula