[alsa-devel] [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips
Wang Xingchao
wangxingchao2011 at gmail.com
Wed Sep 5 07:08:59 CEST 2012
2012/9/3 Takashi Iwai <tiwai at suse.de>:
> At Tue, 28 Aug 2012 08:30:00 +0000,
> Wang, Xingchao wrote:
>>
>> 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.
>
> 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.
Thanks for clarification, i had wrote single patch to fix bugs and add
new features.
>
>
>>
>> >
>> >
>> > > @@ -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)
Looks better, i changed the code in your type already.
>> > > @@ -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.
>
Hmm, i do not have a better idea to implement that. I added a bool
"non_pcm" in hdmi_spec_per_cvt to remember
the last time audio type, pcm or non-pcm. And compare with current
audio type, then only update channel mapping
when there's a audio type change.
Because i have no A/V receiver handy, i could not test the patches
right now. Takashi, could i send the patches out in RFC version for
review first to gather some ideas?
thanks
--xingchao
More information about the Alsa-devel
mailing list