[alsa-devel] [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips
Wang, Xingchao
xingchao.wang at intel.com
Tue Aug 28 10:30:00 CEST 2012
Hi Anssi,
> -----Original Message-----
> From: Anssi Hannula [mailto:anssi.hannula at iki.fi]
> Sent: Tuesday, August 28, 2012 4:00 PM
> To: Wang, Xingchao
> Cc: alsa-devel at alsa-project.org; Wu, Fengguang; tiwai at suse.de;
> alanwww1 at 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 at intel.com>
> > Signed-off-by: Anssi Hannula <anssi.hannula at 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.
>
>
> > @@ -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.
>
>
> > for (i = 0; i < 8; i++) {
> > err = snd_hda_codec_write(codec, pin_nid, 0,
> > AC_VERB_SET_HDMI_CHAN_SLOT,
> > - hdmi_channel_mapping[ca][i]);
> > + non_pcm ? tmp_mapping[i] :
> hdmi_channel_mapping[ca][i]);
> > if (err) {
> > snd_printdd(KERN_NOTICE
> > "HDMI: channel mapping failed\n");
> > @@ -686,7 +698,7 @@ static bool hdmi_infoframe_uptodate(struct
> > hda_codec *codec, hda_nid_t pin_nid,
> > }
> >
> > static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int
> > pin_idx,
> > - struct snd_pcm_substream *substream)
> > + hda_nid_t cvt_nid, struct snd_pcm_substream
> *substream)
> > {
> > struct hdmi_spec *spec = codec->spec;
> > struct hdmi_spec_per_pin *per_pin = &spec->pins[pin_idx];
> > @@ -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.
I tested this patch on my Yamaha receiver, both DTS-HD and TrueHD HBR streams work well, normal DTS/AC3(2 channels) streams work well,
Speaker-test could play correct sound from each speaker.
Thanks
--xingchao
More information about the Alsa-devel
mailing list