Re: [PATCH 3/4] hda: HDMI channel allocations for audio infoframe

[Wu Fengguang <wfg@linux.intel.com>]

On Tue, Nov 18, 2008 at 11:57:08AM +0100, Takashi Iwai wrote:
> At Tue, 18 Nov 2008 16:57:16 +0800,
> Wu Fengguang wrote:
> > 
> > To play a 3+ channels LPCM/DSD stream via HDMI,
> > 
> > 	- HDMI sink must tell HDMI source about its speaker placements
> > 	  (via ELD, speaker-allocation field)
> > 	- HDMI source must tell the HDMI sink about channel allocation
> > 	  (via audio infoframe, channel-allocation field)
> > 
> > (related docs: HDMI 1.3a spec section 7.4, CEA-861-D section 7.5.3 and 6.6)
> > 
> > This patch attempts to set the CA(channel-allocation) byte in the audio infoframe
> > according to
> > 	- the number of channels in the current stream
> > 	- the speakers attached to the HDMI sink
> > 
> > A channel_allocations[] line must meet the following two criterions to be
> > considered as a valid candidate for CA:
> > 	1) its number of allocated channels = substream->runtime->channels
> > 	2) its speakers are a subset of the available ones on the sink side
> > 
> > If there are multiple candidates, the first one is selected.  This simple
> > policy shall cheat the sink into playing music, but may direct data to the
> > wrong speakers.
> > 
> > Sorry, this last step is not obvious to me. Any domain experts, please?
> 
> The problem is that the speaker positioning isn't defined in the ALSA
> API yet.  I think your implementation will work in practice.

OK. Let's forget it until (possibly) some one complains ;-)

> > +static struct cea_channel_speaker_allocation channel_allocations[] = {
> > +/* channel number:  8      7     6     5     4      3     2     1 */
> 
> Do we need to set ca_index explicitly?
> Any case ca_index != array index?

Yes, since we stop at the first matching item, the lines could be
reordered somewhere to give priority to some popular ones.

> > +static void init_channel_allocations(void)
> > +{
> > +	int i, j;
> > +	struct cea_channel_speaker_allocation *p;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(channel_allocations); i++) {
> > +		p = channel_allocations + i;
> 
> You should zero-initialize p->channels and p->spk_mask here...
> 
> > +		for (j = 0; j < ARRAY_SIZE(p->speakers); j++)
> > +			if (p->speakers[j]) {
> > +				p->channels++;
> > +				p->spk_mask |= p->speakers[j];
> > +			}
> > +	}
> 
> ... because patch_intel_hdmi() may be called multiple times (either
> multiple codecs or reconfiguration via hwdep) as below.

Got it, thanks.

> > @@ -455,6 +642,8 @@ static int patch_intel_hdmi(struct hda_c
> >  
> >  	snd_hda_eld_proc_new(codec, &spec->sink);
> >  
> > +	init_channel_allocations();
> > +
> >  	return 0;
> >  }

Thank you,
Fengguang