Re: [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips

[Takashi Iwai <tiwai@suse.de>]

At Tue, 28 Aug 2012 08:30:00 +0000,
Wang, Xingchao wrote:
> 
> Hi Anssi,
> 
> > -----Original Message-----
> > From: Anssi Hannula [mailto:anssi.hannula@iki.fi]
> > Sent: Tuesday, August 28, 2012 4:00 PM
> > To: Wang, Xingchao
> > Cc: alsa-devel@alsa-project.org; Wu, Fengguang; tiwai@suse.de;
> > alanwww1@xbmc.org
> > Subject: Re: [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips
> > 
> > On 28.08.2012 09:46, Wang Xingchao wrote:
> > > HDMI channel remapping apparently effects HBR packets on Intel's
> > > chips.
> > > For compressed non-PCM audio, use "straight-through" channel mapping.
> > > For uncompressed multi-channel pcm audio, use normal mapping.
> > >
> > > Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
> > > Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> > 
> > For the record, I haven't acked this patch yet (comments below).
> > I guess Wang Xingchao wanted to give me credit for my idea behind the patch.
> 
> Yeah, that's true. :)
> > 
> > > ---
> > >  sound/pci/hda/patch_hdmi.c |   50
> > > +++++++++++++++++++++++++++-----------------
> > >  1 file changed, 31 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > > index d9439c5..fda89e3 100644
> > > --- a/sound/pci/hda/patch_hdmi.c
> > > +++ b/sound/pci/hda/patch_hdmi.c
> > > @@ -231,6 +231,11 @@ static struct cea_channel_speaker_allocation
> > > channel_allocations[] = {
> > >  { .ca_index = 0x01,  .speakers = {   0,    0,   0,   0,   0,  LFE,
> > > FR,  FL } },
> > >  				 /* Dolby Surround */
> > >  { .ca_index = 0x02,  .speakers = {   0,    0,   0,   0,  FC,    0,
> > > FR,  FL } },
> > > +{ .ca_index = 0x03,  .speakers = {   0,    0,   0,   0,  FC,  LFE,
> > > FR,  FL } },
> > > +{ .ca_index = 0x04,  .speakers = {   0,    0,   0,  RC,   0,    0,
> > > FR,  FL } },
> > > +{ .ca_index = 0x05,  .speakers = {   0,    0,   0,  RC,   0,  LFE,
> > > FR,  FL } },
> > > +{ .ca_index = 0x06,  .speakers = {   0,    0,   0,  RC,  FC,    0,
> > > FR,  FL } },
> > > +{ .ca_index = 0x07,  .speakers = {   0,    0,   0,  RC,  FC,  LFE,
> > > FR,  FL } },
> > >  				 /* surround40 */
> > >  { .ca_index = 0x08,  .speakers = {   0,    0,  RR,  RL,   0,    0,
> > > FR,  FL } },
> > >  				 /* surround41 */
> > > @@ -239,22 +244,15 @@ static struct cea_channel_speaker_allocation
> > > channel_allocations[] = {
> > >  { .ca_index = 0x0a,  .speakers = {   0,    0,  RR,  RL,  FC,    0,
> > > FR,  FL } },
> > >  				 /* surround51 */
> > >  { .ca_index = 0x0b,  .speakers = {   0,    0,  RR,  RL,  FC,  LFE,
> > > FR,  FL } },
> > > -				 /* 6.1 */
> > > -{ .ca_index = 0x0f,  .speakers = {   0,   RC,  RR,  RL,  FC,  LFE,
> > > FR,  FL } },
> > > -				 /* surround71 */
> > > -{ .ca_index = 0x13,  .speakers = { RRC,  RLC,  RR,  RL,  FC,  LFE,
> > > FR,  FL } },
> > > -
> > > -{ .ca_index = 0x03,  .speakers = {   0,    0,   0,   0,  FC,  LFE,
> > > FR,  FL } },
> > > -{ .ca_index = 0x04,  .speakers = {   0,    0,   0,  RC,   0,    0,
> > > FR,  FL } },
> > > -{ .ca_index = 0x05,  .speakers = {   0,    0,   0,  RC,   0,  LFE,
> > > FR,  FL } },
> > > -{ .ca_index = 0x06,  .speakers = {   0,    0,   0,  RC,  FC,    0,
> > > FR,  FL } },
> > > -{ .ca_index = 0x07,  .speakers = {   0,    0,   0,  RC,  FC,  LFE,
> > > FR,  FL } },
> > >  { .ca_index = 0x0c,  .speakers = {   0,   RC,  RR,  RL,   0,    0,
> > > FR,  FL } },
> > >  { .ca_index = 0x0d,  .speakers = {   0,   RC,  RR,  RL,   0,  LFE,
> > > FR,  FL } },
> > >  { .ca_index = 0x0e,  .speakers = {   0,   RC,  RR,  RL,  FC,    0,
> > > FR,  FL } },
> > > +{ .ca_index = 0x0f,  .speakers = {   0,   RC,  RR,  RL,  FC,  LFE,
> > > FR,  FL } },
> > >  { .ca_index = 0x10,  .speakers = { RRC,  RLC,  RR,  RL,   0,    0,
> > > FR,  FL } },
> > >  { .ca_index = 0x11,  .speakers = { RRC,  RLC,  RR,  RL,   0,  LFE,
> > > FR,  FL } },
> > >  { .ca_index = 0x12,  .speakers = { RRC,  RLC,  RR,  RL,  FC,    0,
> > > FR,  FL } },
> > > +				 /* surround71 */
> > > +{ .ca_index = 0x13,  .speakers = { RRC,  RLC,  RR,  RL,  FC,  LFE,
> > > FR,  FL } },
> > >  { .ca_index = 0x14,  .speakers = { FRC,  FLC,   0,   0,   0,    0,
> > > FR,  FL } },
> > >  { .ca_index = 0x15,  .speakers = { FRC,  FLC,   0,   0,   0,  LFE,
> > > FR,  FL } },
> > >  { .ca_index = 0x16,  .speakers = { FRC,  FLC,   0,   0,  FC,    0,
> > > FR,  FL } },
> > 
> > Doesn't this reordering change the selected CAs? (they are listed in priority
> > order)
> 
> I changed this order because there's a bug during my test.
> In hdmi_setup_channel_mapping(), when ca=0x13 for 7.1 channels, channel_allocations[ca].channels actually be 7 not 8.
> So I think both array channel_allocation[] and channel_mapping[] should have the same order.

In general it's bad to mix up the fixes for different things in a
single patch.  And it's much worse if you do it secretly.
So, don't do that.  Make it a separate patch.


> 
> > 
> > 
> > > @@ -537,22 +535,36 @@ static void hdmi_debug_channel_mapping(struct
> > > hda_codec *codec,
> > >
> > >  static void hdmi_setup_channel_mapping(struct hda_codec *codec,
> > >  				       hda_nid_t pin_nid,
> > > +				       hda_nid_t cvt_nid,
> > >  				       int ca)
> > >  {
> > >  	int i;
> > >  	int err;
> > > -
> > > -	if (hdmi_channel_mapping[ca][1] == 0) {
> > > -		for (i = 0; i < channel_allocations[ca].channels; i++)
> > > -			hdmi_channel_mapping[ca][i] = i | (i << 4);
> > > +	unsigned short val;
> > > +	int tmp_mapping[8];
> > > +	bool non_pcm = false;
> > > +
> > > +	val = snd_hda_codec_read(codec, cvt_nid, 0,
> > > AC_VERB_GET_DIGI_CONVERT_1, 0);
> > > +	non_pcm = !!(val & AC_DIG1_NONAUDIO);
> > 
> > AFAICS you could use snd_hda_spdif_out_of_nid() and spdif->status here
> > to avoid
> > having to read the value from HW.
> 
> Good idea, will change to use that in next version.
> 
> > 
> > > +
> > > +	if (hdmi_channel_mapping[ca][1] == 0 || non_pcm) {
> > > +		for (i = 0; i < channel_allocations[ca].channels; i++) {
> > > +			if (non_pcm)
> > > +				tmp_mapping[i] = i | (i << 4);
> > > +			else
> > > +				hdmi_channel_mapping[ca][i] = i | (i << 4);
> > > +		}
> > >  		for (; i < 8; i++)
> > > -			hdmi_channel_mapping[ca][i] = 0xf | (i << 4);
> > > +			if (non_pcm)
> > > +				tmp_mapping[i] = 0xf | (i << 4);
> > > +			else
> > > +				hdmi_channel_mapping[ca][i] = 0xf | (i << 4);
> > >  	}
> > 
> > I don't think it makes much sense to create the mapping dynamically,
> > since it
> > is always 0x00 0x11 0x22 0x33 0x44 0x55 0x66 0x77. It also complicates
> > the code.
> 
> Above channels mapping is good for 8 channels compressed audio stream, but will make "speaker-test -c8"
> generate sound from wrong speaker position. i.e. you will heard "Front_center" from another speaker, while the real front_center speaker
> play another position audio. 
> 
> So I think the default hdmi_channel_mapping should be left as default and use dynamically channel_mapping for compressed audio, not only 8 channels.

Yes, but still the code looks more complex than needed, IMO.
You don't have to touch the code creating hdmi_channel_mapping[].
Simply add the block for non_pcm like

	if (non_pcm) {
		for (i = 0; i < channel_allocations[ca].channels; i++)
			tmp_mapping[i] = i | (i << 4);
		for (; i < 8; i++)
			tmp_mapping[i] = 0xf | (i << 4);
	}

(snip)
> > > @@ -731,13 +743,13 @@ static void hdmi_setup_audio_infoframe(struct
> > > hda_codec *codec, int pin_idx,
> > >  	 * sizeof(*dp_ai) to avoid partial match/update problems when
> > >  	 * the user switches between HDMI/DP monitors.
> > >  	 */
> > > +	hdmi_setup_channel_mapping(codec, pin_nid, cvt_nid, ca);
> > >  	if (!hdmi_infoframe_uptodate(codec, pin_nid, ai.bytes,
> > >  					sizeof(ai))) {
> > >  		snd_printdd("hdmi_setup_audio_infoframe: "
> > >  			    "pin=%d channels=%d\n",
> > >  			    pin_nid,
> > >  			    channels);
> > > -		hdmi_setup_channel_mapping(codec, pin_nid, ca);
> > >  		hdmi_stop_infoframe_trans(codec, pin_nid);
> > >  		hdmi_fill_audio_infoframe(codec, pin_nid,
> > >  					    ai.bytes, sizeof(ai));
> > 
> > I wonder if we could avoid always rewriting channel mapping... I'm not
> > immediately sure of the best way of doing that (or even if it is worth
> > it),
> > though.
> 
> It's another bug. When switch test between "speaker-test -c8" and HBR stream, there's no audio_infoframe update 
> thus "speaker-test -c8" use wrong channel mapping, as described above.

Then you should check whether the channel map is actually changed or
not.  The driver knows what were written beforehand, and what to be
written now.  Unconditionally writing the verb doesn't sound good.


thanks,

Takashi