* [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips
@ 2012-08-28 6:46 Wang Xingchao
2012-08-28 7:53 ` Fengguang Wu
2012-08-28 8:00 ` Anssi Hannula
0 siblings, 2 replies; 8+ messages in thread
From: Wang Xingchao @ 2012-08-28 6:46 UTC (permalink / raw)
To: alsa-devel; +Cc: tiwai, anssi.hannula, fengguang.wu, alanwww1, Wang Xingchao
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>
---
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 } },
@@ -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);
+
+ 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);
}
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));
@@ -1151,7 +1163,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
hdmi_set_channel_count(codec, cvt_nid, substream->runtime->channels);
- hdmi_setup_audio_infoframe(codec, pin_idx, substream);
+ hdmi_setup_audio_infoframe(codec, pin_idx, cvt_nid, substream);
pinctl = snd_hda_codec_read(codec, pin_nid, 0,
AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips
2012-08-28 6:46 [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips Wang Xingchao
@ 2012-08-28 7:53 ` Fengguang Wu
2012-08-28 8:45 ` Wang, Xingchao
2012-08-28 8:00 ` Anssi Hannula
1 sibling, 1 reply; 8+ messages in thread
From: Fengguang Wu @ 2012-08-28 7:53 UTC (permalink / raw)
To: Wang Xingchao; +Cc: tiwai, anssi.hannula, alsa-devel, alanwww1
On Tue, Aug 28, 2012 at 02:46:21PM +0800, 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.
Xingchao, have you also tested the legacy 2/5/6/8-channel HDMI PCM playbacks?
> Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> ---
> 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 } },
What are you doing here? Please check the comment above
channel_allocations[] for the ordering rule.
> /* 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 } },
> @@ -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];
Better to use pre-initialized hbr_mapping[] = { ... } and avoid changing code below.
> + 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);
> +
> + 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);
> }
>
> 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);
Why move hdmi_setup_channel_mapping() up? This might re-introduce the
half-second silence bug. Please do retest if you really need the move.
Background info:
commit ef18beded8ddbaafdf4914bab209f77e60ae3a18
Author: Wu Fengguang <fengguang.wu@intel.com>
Date: Fri Dec 25 13:14:27 2009 +0800
ALSA: hda - HDMI sticky stream tag support
When we run the following commands in turn (with
CONFIG_SND_HDA_POWER_SAVE_DEFAULT=0),
speaker-test -Dhw:0,3 -c2 -twav # HDMI
speaker-test -Dhw:0,0 -c2 -twav # Analog
The second command will produce sound in the analog lineout _as well as_
HDMI sink. The root cause is, device 0 "reuses" the same stream tag that
was used by device 3, and the "intelhdmi - sticky stream id" patch leaves
the HDMI codec in a functional state. So the HDMI codec happily accepts
the audio samples which reuse its stream tag.
The proposed solution is to remember the last device each azx_dev was
assigned to, and prefer to
1) reuse the azx_dev (and hence the stream tag) the HDMI codec last used
2) or assign a never-used azx_dev for HDMI
With this patch and the above two speaker-test commands,
HDMI codec will use stream tag 8 and Analog codec will use 5.
The stream tag used by HDMI codec won't be reused by others, as long
as we don't run out of the 4 playback azx_dev's. The legacy Analog
codec will continue to use stream tag 5 because its device id is 0
(this is a bit tricky).
> hdmi_stop_infoframe_trans(codec, pin_nid);
> hdmi_fill_audio_infoframe(codec, pin_nid,
> ai.bytes, sizeof(ai));
> @@ -1151,7 +1163,7 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>
> hdmi_set_channel_count(codec, cvt_nid, substream->runtime->channels);
>
> - hdmi_setup_audio_infoframe(codec, pin_idx, substream);
> + hdmi_setup_audio_infoframe(codec, pin_idx, cvt_nid, substream);
>
> pinctl = snd_hda_codec_read(codec, pin_nid, 0,
> AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
> --
> 1.7.9.5
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips
2012-08-28 6:46 [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips Wang Xingchao
2012-08-28 7:53 ` Fengguang Wu
@ 2012-08-28 8:00 ` Anssi Hannula
2012-08-28 8:30 ` Wang, Xingchao
1 sibling, 1 reply; 8+ messages in thread
From: Anssi Hannula @ 2012-08-28 8:00 UTC (permalink / raw)
To: Wang Xingchao; +Cc: tiwai, alsa-devel, fengguang.wu, alanwww1
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.
> ---
> 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)
> @@ -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.
> +
> + 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.
> 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.
> @@ -1151,7 +1163,7 @@ static int
> generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>
> hdmi_set_channel_count(codec, cvt_nid,
> substream->runtime->channels);
>
> - hdmi_setup_audio_infoframe(codec, pin_idx, substream);
> + hdmi_setup_audio_infoframe(codec, pin_idx, cvt_nid, substream);
>
> pinctl = snd_hda_codec_read(codec, pin_nid, 0,
> AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
--
Anssi Hannula
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips
2012-08-28 8:00 ` Anssi Hannula
@ 2012-08-28 8:30 ` Wang, Xingchao
2012-09-03 8:36 ` Takashi Iwai
0 siblings, 1 reply; 8+ messages in thread
From: Wang, Xingchao @ 2012-08-28 8:30 UTC (permalink / raw)
To: Anssi Hannula
Cc: tiwai@suse.de, alsa-devel@alsa-project.org, Wu, Fengguang,
alanwww1@xbmc.org
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.
>
>
> > @@ -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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips
2012-08-28 7:53 ` Fengguang Wu
@ 2012-08-28 8:45 ` Wang, Xingchao
0 siblings, 0 replies; 8+ messages in thread
From: Wang, Xingchao @ 2012-08-28 8:45 UTC (permalink / raw)
To: Wu, Fengguang
Cc: tiwai@suse.de, anssi.hannula@iki.fi, alsa-devel@alsa-project.org,
alanwww1@xbmc.org
Hi Fengguang,
> -----Original Message-----
> From: Wu, Fengguang
> Sent: Tuesday, August 28, 2012 3:53 PM
> To: Wang, Xingchao
> Cc: alsa-devel@alsa-project.org; tiwai@suse.de; anssi.hannula@iki.fi;
> alanwww1@xbmc.org
> Subject: Re: [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips
>
> On Tue, Aug 28, 2012 at 02:46:21PM +0800, 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.
>
> Xingchao, have you also tested the legacy 2/5/6/8-channel HDMI PCM
> playbacks?
I tested 6/8 channels HDMI PCM, did not test 2/5.
>
> > Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
> > Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> > ---
> > 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 } },
>
> What are you doing here? Please check the comment above
> channel_allocations[] for the ordering rule.
It's a bug fix, please see comments in another mail thread.
>
> > /* 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 } },
> > @@ -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];
>
> Better to use pre-initialized hbr_mapping[] = { ... } and avoid changing code
> below.
Hm, I use tmp_mapping[] here for *ALL* compressed audio, not only HBR.
If it's only for HBR, I prefer your suggestion above.
>
> > + 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);
> > +
> > + 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);
> > }
> >
> > 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);
>
> Why move hdmi_setup_channel_mapping() up? This might re-introduce the
> half-second silence bug. Please do retest if you really need the move.
>
Yes, I noticed the harf-second silence, it's "Front_Left" sound missing at first time.
I force hdmi_setup_channel_mapping() everytime because I have not find a better idea
to switch between playing compressed audio stream and normal pcm stream. There should be the channel mapping update between
the switch but actually not.
Anyway here need a change, thanks Fengguang.
--thanks
--xingchao
> Background info:
>
> commit ef18beded8ddbaafdf4914bab209f77e60ae3a18
> Author: Wu Fengguang <fengguang.wu@intel.com>
> Date: Fri Dec 25 13:14:27 2009 +0800
>
> ALSA: hda - HDMI sticky stream tag support
>
> When we run the following commands in turn (with
> CONFIG_SND_HDA_POWER_SAVE_DEFAULT=0),
>
> speaker-test -Dhw:0,3 -c2 -twav # HDMI
> speaker-test -Dhw:0,0 -c2 -twav # Analog
>
> The second command will produce sound in the analog lineout _as well as_
> HDMI sink. The root cause is, device 0 "reuses" the same stream tag that
> was used by device 3, and the "intelhdmi - sticky stream id" patch leaves
> the HDMI codec in a functional state. So the HDMI codec happily accepts
> the audio samples which reuse its stream tag.
>
> The proposed solution is to remember the last device each azx_dev was
> assigned to, and prefer to
> 1) reuse the azx_dev (and hence the stream tag) the HDMI codec last
> used
> 2) or assign a never-used azx_dev for HDMI
>
> With this patch and the above two speaker-test commands,
> HDMI codec will use stream tag 8 and Analog codec will use 5.
>
> The stream tag used by HDMI codec won't be reused by others, as long
> as we don't run out of the 4 playback azx_dev's. The legacy Analog
> codec will continue to use stream tag 5 because its device id is 0
> (this is a bit tricky).
>
> > hdmi_stop_infoframe_trans(codec, pin_nid);
> > hdmi_fill_audio_infoframe(codec, pin_nid,
> > ai.bytes, sizeof(ai));
> > @@ -1151,7 +1163,7 @@ static int
> > generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
> >
> > hdmi_set_channel_count(codec, cvt_nid,
> > substream->runtime->channels);
> >
> > - hdmi_setup_audio_infoframe(codec, pin_idx, substream);
> > + hdmi_setup_audio_infoframe(codec, pin_idx, cvt_nid, substream);
> >
> > pinctl = snd_hda_codec_read(codec, pin_nid, 0,
> > AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
> > --
> > 1.7.9.5
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips
2012-08-28 8:30 ` Wang, Xingchao
@ 2012-09-03 8:36 ` Takashi Iwai
2012-09-05 5:08 ` Wang Xingchao
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2012-09-03 8:36 UTC (permalink / raw)
To: Wang, Xingchao
Cc: Anssi Hannula, alsa-devel@alsa-project.org, Wu, Fengguang,
alanwww1@xbmc.org
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips
2012-09-03 8:36 ` Takashi Iwai
@ 2012-09-05 5:08 ` Wang Xingchao
2012-09-05 5:54 ` Takashi Iwai
0 siblings, 1 reply; 8+ messages in thread
From: Wang Xingchao @ 2012-09-05 5:08 UTC (permalink / raw)
To: Takashi Iwai
Cc: Anssi Hannula, alsa-devel@alsa-project.org, Wu, Fengguang,
alanwww1@xbmc.org, Wang, Xingchao
2012/9/3 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.
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips
2012-09-05 5:08 ` Wang Xingchao
@ 2012-09-05 5:54 ` Takashi Iwai
0 siblings, 0 replies; 8+ messages in thread
From: Takashi Iwai @ 2012-09-05 5:54 UTC (permalink / raw)
To: Wang Xingchao
Cc: Anssi Hannula, alsa-devel@alsa-project.org, Wu, Fengguang,
alanwww1@xbmc.org, Wang, Xingchao
At Wed, 5 Sep 2012 13:08:59 +0800,
Wang Xingchao wrote:
>
> 2012/9/3 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.
>
> 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.
Yes, this should work.
> 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?
Sure, it's appreciated.
thanks,
Takashi
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-09-05 5:54 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-08-28 6:46 [PATCH] ALSA: HDMI - Enable HBR feature on Intel chips Wang Xingchao
2012-08-28 7:53 ` Fengguang Wu
2012-08-28 8:45 ` Wang, Xingchao
2012-08-28 8:00 ` Anssi Hannula
2012-08-28 8:30 ` Wang, Xingchao
2012-09-03 8:36 ` Takashi Iwai
2012-09-05 5:08 ` Wang Xingchao
2012-09-05 5:54 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.