[alsa-devel] [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI
Since I'm a little new in HDMI land and the patch is non-trivial, I wouldn't mind some comments on this one. Also thanks to Jaroslav who cleared a few things out about the logical and physical devices which helped me figure out what I should do about it :-)
Anyway, here's the patch.
Some newer chips have more than one HDMI output, but usually not all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningsson david.henningsson@canonical.com
At Fri, 12 Nov 2010 16:46:34 +0100, David Henningsson wrote:
Since I'm a little new in HDMI land and the patch is non-trivial, I wouldn't mind some comments on this one. Also thanks to Jaroslav who cleared a few things out about the logical and physical devices which helped me figure out what I should do about it :-)
Anyway, here's the patch.
Some newer chips have more than one HDMI output, but usually not all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningsson david.henningsson@canonical.com
The patch looks mostly OK. But this gives a few errors and warnings via checkpatch.pl. Could you fix errors, at least?
Also some review comments:
static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t nid) {
- int i; struct hdmi_spec *spec = codec->spec;
- if (spec->num_cvts >= MAX_HDMI_CVTS) {
snd_printk(KERN_WARNING"HDMI: no space for converter %d\n", nid);
- for (i = 0; spec->pin_cvt[i] != nid; i++)
I'd write with a normal loop like for (i = 0; i < spec->num_pins; i++) This is easier to read.
if (!spec->pin[i]) {snd_printd("HDMI: Skipping node %d (no connection)\n", nid);
Better to use snd_printdd(). snd_printd() is enabled on many distros, and such an unnecessary kernel message may make user worry.
@@ -951,17 +954,33 @@ static int hdmi_parse_codec(struct hda_codec *codec)
...
case AC_WID_PIN: caps = snd_hda_param_read(codec, nid, AC_PAR_PIN_CAP); if (!(caps & (AC_PINCAP_HDMI | AC_PINCAP_DP))) continue;
config = snd_hda_codec_read(codec, nid, 0,AC_VERB_GET_CONFIG_DEFAULT, 0);if (((config & AC_DEFCFG_PORT_CONN) >>AC_DEFCFG_PORT_CONN_SHIFT) == AC_JACK_PORT_NONE)
You can use get_defcfg_connect() here.
thanks,
Takashi
On 2010-11-23 08:12, Takashi Iwai wrote:
At Fri, 12 Nov 2010 16:46:34 +0100, David Henningsson wrote:
Since I'm a little new in HDMI land and the patch is non-trivial, I wouldn't mind some comments on this one. Also thanks to Jaroslav who cleared a few things out about the logical and physical devices which helped me figure out what I should do about it :-)
Anyway, here's the patch.
Some newer chips have more than one HDMI output, but usually not all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningssondavid.henningsson@canonical.com
The patch looks mostly OK. But this gives a few errors and warnings via checkpatch.pl. Could you fix errors, at least?
Also some review comments:
static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t nid) {
- int i; struct hdmi_spec *spec = codec->spec;
- if (spec->num_cvts>= MAX_HDMI_CVTS) {
snd_printk(KERN_WARNING"HDMI: no space for converter %d\n", nid);
- for (i = 0; spec->pin_cvt[i] != nid; i++)
I'd write with a normal loop like for (i = 0; i< spec->num_pins; i++) This is easier to read.
if (!spec->pin[i]) {snd_printd("HDMI: Skipping node %d (no connection)\n", nid);Better to use snd_printdd(). snd_printd() is enabled on many distros, and such an unnecessary kernel message may make user worry.
@@ -951,17 +954,33 @@ static int hdmi_parse_codec(struct hda_codec *codec)
...
case AC_WID_PIN: caps = snd_hda_param_read(codec, nid, AC_PAR_PIN_CAP); if (!(caps& (AC_PINCAP_HDMI | AC_PINCAP_DP))) continue;
config = snd_hda_codec_read(codec, nid, 0,AC_VERB_GET_CONFIG_DEFAULT, 0);if (((config& AC_DEFCFG_PORT_CONN)>>AC_DEFCFG_PORT_CONN_SHIFT) == AC_JACK_PORT_NONE)You can use get_defcfg_connect() here.
Thanks for the review, Takashi! I usually run checkpatch.pl these days but this one must have slipped. What do you think of this patch (untested)?
On Tue, Nov 23, 2010 at 10:29:12AM +0100, David Henningsson wrote:
On 2010-11-23 08:12, Takashi Iwai wrote:
At Fri, 12 Nov 2010 16:46:34 +0100, David Henningsson wrote:
Since I'm a little new in HDMI land and the patch is non-trivial, I wouldn't mind some comments on this one. Also thanks to Jaroslav who cleared a few things out about the logical and physical devices which helped me figure out what I should do about it :-)
Anyway, here's the patch.
Some newer chips have more than one HDMI output, but usually not all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningssondavid.henningsson@canonical.com
The patch looks mostly OK. But this gives a few errors and warnings via checkpatch.pl. Could you fix errors, at least?
Also some review comments:
static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t nid) {
- int i; struct hdmi_spec *spec = codec->spec;
- if (spec->num_cvts>= MAX_HDMI_CVTS) {
snd_printk(KERN_WARNING"HDMI: no space for converter %d\n", nid);
- for (i = 0; spec->pin_cvt[i] != nid; i++)
I'd write with a normal loop like for (i = 0; i< spec->num_pins; i++) This is easier to read.
if (!spec->pin[i]) {snd_printd("HDMI: Skipping node %d (no connection)\n", nid);Better to use snd_printdd(). snd_printd() is enabled on many distros, and such an unnecessary kernel message may make user worry.
@@ -951,17 +954,33 @@ static int hdmi_parse_codec(struct hda_codec *codec)
...
case AC_WID_PIN: caps = snd_hda_param_read(codec, nid, AC_PAR_PIN_CAP); if (!(caps& (AC_PINCAP_HDMI | AC_PINCAP_DP))) continue;
config = snd_hda_codec_read(codec, nid, 0,AC_VERB_GET_CONFIG_DEFAULT, 0);if (((config& AC_DEFCFG_PORT_CONN)>>AC_DEFCFG_PORT_CONN_SHIFT) == AC_JACK_PORT_NONE)You can use get_defcfg_connect() here.
Thanks for the review, Takashi! I usually run checkpatch.pl these days but this one must have slipped. What do you think of this patch (untested)?
Thanks for the patch, I'll help test it out :)
I didn't do such feature because I'm not sure if the BIOS provided info can be trusted, or if that's merely the default configuration that may be changed runtime on user request (eg. switching from A+A view to A+B view etc.).
Thanks, Fengguang
At Tue, 23 Nov 2010 19:06:39 +0800, Wu Fengguang wrote:
On Tue, Nov 23, 2010 at 10:29:12AM +0100, David Henningsson wrote:
On 2010-11-23 08:12, Takashi Iwai wrote:
At Fri, 12 Nov 2010 16:46:34 +0100, David Henningsson wrote:
Since I'm a little new in HDMI land and the patch is non-trivial, I wouldn't mind some comments on this one. Also thanks to Jaroslav who cleared a few things out about the logical and physical devices which helped me figure out what I should do about it :-)
Anyway, here's the patch.
Some newer chips have more than one HDMI output, but usually not all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningssondavid.henningsson@canonical.com
The patch looks mostly OK. But this gives a few errors and warnings via checkpatch.pl. Could you fix errors, at least?
Also some review comments:
static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t nid) {
- int i; struct hdmi_spec *spec = codec->spec;
- if (spec->num_cvts>= MAX_HDMI_CVTS) {
snd_printk(KERN_WARNING"HDMI: no space for converter %d\n", nid);
- for (i = 0; spec->pin_cvt[i] != nid; i++)
I'd write with a normal loop like for (i = 0; i< spec->num_pins; i++) This is easier to read.
if (!spec->pin[i]) {snd_printd("HDMI: Skipping node %d (no connection)\n", nid);Better to use snd_printdd(). snd_printd() is enabled on many distros, and such an unnecessary kernel message may make user worry.
@@ -951,17 +954,33 @@ static int hdmi_parse_codec(struct hda_codec *codec)
...
case AC_WID_PIN: caps = snd_hda_param_read(codec, nid, AC_PAR_PIN_CAP); if (!(caps& (AC_PINCAP_HDMI | AC_PINCAP_DP))) continue;
config = snd_hda_codec_read(codec, nid, 0,AC_VERB_GET_CONFIG_DEFAULT, 0);if (((config& AC_DEFCFG_PORT_CONN)>>AC_DEFCFG_PORT_CONN_SHIFT) == AC_JACK_PORT_NONE)You can use get_defcfg_connect() here.
Thanks for the review, Takashi! I usually run checkpatch.pl these days but this one must have slipped. What do you think of this patch (untested)?
Thanks for the patch, I'll help test it out :)
I didn't do such feature because I'm not sure if the BIOS provided info can be trusted, or if that's merely the default configuration that may be changed runtime on user request (eg. switching from A+A view to A+B view etc.).
Well, whether this BIOS setup is really reliable is still an open question. David, do you know (or have) the case whether your patch works on a real machine, right?
Other than that, I don't see any problem in the patch. I'm going to apply it (but for 2.6.38) later.
thanks,
Takashi
On 2010-11-23 14:51, Takashi Iwai wrote:
At Tue, 23 Nov 2010 19:06:39 +0800, Wu Fengguang wrote:
On Tue, Nov 23, 2010 at 10:29:12AM +0100, David Henningsson wrote:
On 2010-11-23 08:12, Takashi Iwai wrote:
At Fri, 12 Nov 2010 16:46:34 +0100, David Henningsson wrote:
Since I'm a little new in HDMI land and the patch is non-trivial, I wouldn't mind some comments on this one. Also thanks to Jaroslav who cleared a few things out about the logical and physical devices which helped me figure out what I should do about it :-)
Anyway, here's the patch.
Some newer chips have more than one HDMI output, but usually not all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningssondavid.henningsson@canonical.com
The patch looks mostly OK. But this gives a few errors and warnings via checkpatch.pl. Could you fix errors, at least?
Also some review comments:
static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t nid) {
- int i; struct hdmi_spec *spec = codec->spec;
- if (spec->num_cvts>= MAX_HDMI_CVTS) {
snd_printk(KERN_WARNING"HDMI: no space for converter %d\n", nid);
- for (i = 0; spec->pin_cvt[i] != nid; i++)
I'd write with a normal loop like for (i = 0; i< spec->num_pins; i++) This is easier to read.
if (!spec->pin[i]) {snd_printd("HDMI: Skipping node %d (no connection)\n", nid);Better to use snd_printdd(). snd_printd() is enabled on many distros, and such an unnecessary kernel message may make user worry.
@@ -951,17 +954,33 @@ static int hdmi_parse_codec(struct hda_codec *codec)
...
case AC_WID_PIN: caps = snd_hda_param_read(codec, nid, AC_PAR_PIN_CAP); if (!(caps& (AC_PINCAP_HDMI | AC_PINCAP_DP))) continue;
config = snd_hda_codec_read(codec, nid, 0,AC_VERB_GET_CONFIG_DEFAULT, 0);if (((config& AC_DEFCFG_PORT_CONN)>>AC_DEFCFG_PORT_CONN_SHIFT) == AC_JACK_PORT_NONE)You can use get_defcfg_connect() here.
Thanks for the review, Takashi! I usually run checkpatch.pl these days but this one must have slipped. What do you think of this patch (untested)?
Thanks for the patch, I'll help test it out :)
I didn't do such feature because I'm not sure if the BIOS provided info can be trusted,
Perhaps Wu knows what Intel's default is, would that be all outputs configured as Jacks, or as N/A? I'm thinking that if it is Jacks, then BIOS changing that to N/A probably knows what it's doing. If it is the other way around - perhaps we should add something saying that if it's all N/A, ignore the extra check and enable all outputs.
Anyway it's overridable through user_pin_configs / driver_pin_configs.
or if that's merely the default configuration that may be changed runtime on user request (eg. switching from A+A view to A+B view etc.).
Ah, interesting. But if A+B view is possible, wouldn't BIOS then configure both HDMI outputs as Jacks?
Well, whether this BIOS setup is really reliable is still an open question. David, do you know (or have) the case whether your patch works on a real machine, right?
Yes; although this latest version of the patch has not been tested on that machine yet.
Other than that, I don't see any problem in the patch. I'm going to apply it (but for 2.6.38) later.
Just to clarify, will it be applied to the sound-2.6 tree soon (i e the branch that you create your daily snapshots from)?
On Tue, Nov 23, 2010 at 03:19:58PM +0100, David Henningsson wrote:
On 2010-11-23 14:51, Takashi Iwai wrote:
At Tue, 23 Nov 2010 19:06:39 +0800, Wu Fengguang wrote:
On Tue, Nov 23, 2010 at 10:29:12AM +0100, David Henningsson wrote:
On 2010-11-23 08:12, Takashi Iwai wrote:
At Fri, 12 Nov 2010 16:46:34 +0100, David Henningsson wrote:
Since I'm a little new in HDMI land and the patch is non-trivial, I wouldn't mind some comments on this one. Also thanks to Jaroslav who cleared a few things out about the logical and physical devices which helped me figure out what I should do about it :-)
Anyway, here's the patch.
Some newer chips have more than one HDMI output, but usually not all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningssondavid.henningsson@canonical.com
The patch looks mostly OK. But this gives a few errors and warnings via checkpatch.pl. Could you fix errors, at least?
Also some review comments:
static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t nid) {
- int i; struct hdmi_spec *spec = codec->spec;
- if (spec->num_cvts>= MAX_HDMI_CVTS) {
snd_printk(KERN_WARNING"HDMI: no space for converter %d\n", nid);
- for (i = 0; spec->pin_cvt[i] != nid; i++)
I'd write with a normal loop like for (i = 0; i< spec->num_pins; i++) This is easier to read.
if (!spec->pin[i]) {snd_printd("HDMI: Skipping node %d (no connection)\n", nid);Better to use snd_printdd(). snd_printd() is enabled on many distros, and such an unnecessary kernel message may make user worry.
@@ -951,17 +954,33 @@ static int hdmi_parse_codec(struct hda_codec *codec)
...
case AC_WID_PIN: caps = snd_hda_param_read(codec, nid, AC_PAR_PIN_CAP); if (!(caps& (AC_PINCAP_HDMI | AC_PINCAP_DP))) continue;
config = snd_hda_codec_read(codec, nid, 0,AC_VERB_GET_CONFIG_DEFAULT, 0);if (((config& AC_DEFCFG_PORT_CONN)>>AC_DEFCFG_PORT_CONN_SHIFT) == AC_JACK_PORT_NONE)You can use get_defcfg_connect() here.
Thanks for the review, Takashi! I usually run checkpatch.pl these days but this one must have slipped. What do you think of this patch (untested)?
Thanks for the patch, I'll help test it out :)
I didn't do such feature because I'm not sure if the BIOS provided info can be trusted,
Perhaps Wu knows what Intel's default is, would that be all outputs configured as Jacks, or as N/A? I'm thinking that if it is Jacks, then BIOS changing that to N/A probably knows what it's doing. If it is the other way around - perhaps we should add something saying that if it's all N/A, ignore the extra check and enable all outputs.
Anyway it's overridable through user_pin_configs / driver_pin_configs.
Right. Some time ago I have to do a hacking patch to override the (alpha-)BIOS pin connections to get HDMI audio. But your patch makes sense for the normal cases.
or if that's merely the default configuration that may be changed runtime on user request (eg. switching from A+A view to A+B view etc.).
Ah, interesting. But if A+B view is possible, wouldn't BIOS then configure both HDMI outputs as Jacks?
Sounds reasonable.
Well, whether this BIOS setup is really reliable is still an open question. David, do you know (or have) the case whether your patch works on a real machine, right?
Yes; although this latest version of the patch has not been tested on that machine yet.
What's your codec# file? I have saved copies for Intel eaglelake, ibexpeak, sandybridge codecs, and all of their pins show up as
Pin Default 0x185600*0: [Jack] Digital Out at Int HDMI
The highest two bits are AC_JACK_PORT_COMPLEX=0. So your patch should not make any difference for them.
Thanks, Fengguang
At Tue, 23 Nov 2010 15:19:58 +0100, David Henningsson wrote:
Other than that, I don't see any problem in the patch. I'm going to apply it (but for 2.6.38) later.
Just to clarify, will it be applied to the sound-2.6 tree soon (i e the branch that you create your daily snapshots from)?
Yes, I'm going to apply it topic/hda branch. This is merged to master (and for-next) branch, then the snapshot is created from master.
Takashi
From ca84aa8edbfb66e46266677249b141b5419d6e0a Mon Sep 17 00:00:00 2001
From: David Henningsson david.henningsson@canonical.com Date: Tue, 23 Nov 2010 10:23:40 +0100 Subject: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI
Some newer chips have more than one HDMI output, but usually not
Please point out the model name here (where this patch actually makes a difference)?
all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningsson david.henningsson@canonical.com
sound/pci/hda/patch_hdmi.c | 41 ++++++++++++++++++++++++++++++++--------- 1 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index d3e49aa..14a1087 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -905,23 +905,28 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid) spec->pin[spec->num_pins] = pin_nid; spec->num_pins++;
- /*
* It is assumed that converter nodes come first in the node list and* hence have been registered and usable now.*/}
static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t nid) {
- int i, found_pin = 0; struct hdmi_spec *spec = codec->spec;
- if (spec->num_cvts >= MAX_HDMI_CVTS) {
snd_printk(KERN_WARNING"HDMI: no space for converter %d\n", nid);return -E2BIG;
- for (i = 0; i < spec->num_pins; i++)
if (nid == spec->pin_cvt[i]) {found_pin = 1;break;}- if (!found_pin) {
Can test this instead:
if (hda_node_index(spec->pin_cvt, nid) < 0) {
snd_printdd("HDMI: Skipping node %d (no connection)\n", nid);return -EINVAL;
The above return actually will hide the device for cvt 3:
+---- pin 4 / cvt 2 ------ pin 5 \ +---- pin 6
cvt 3
Which is the default connection for Intel ibexpeak/sandybridge codecs. It might be a problem when the user uses dual displays (which may be configured at runtime). I have no such a setup, so cannot assure things will work or not work..
Thanks, Fengguang
On 2010-11-23 16:15, Wu Fengguang wrote:
From ca84aa8edbfb66e46266677249b141b5419d6e0a Mon Sep 17 00:00:00 2001
From: David Henningssondavid.henningsson@canonical.com Date: Tue, 23 Nov 2010 10:23:40 +0100 Subject: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI
Some newer chips have more than one HDMI output, but usually not
Please point out the model name here (where this patch actually makes a difference)?
I'm attaching the codec-proc file for the relevant machine, which lists as "Intel Cougarpoint HDMI".
all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningssondavid.henningsson@canonical.com
sound/pci/hda/patch_hdmi.c | 41 ++++++++++++++++++++++++++++++++--------- 1 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index d3e49aa..14a1087 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -905,23 +905,28 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid) spec->pin[spec->num_pins] = pin_nid; spec->num_pins++;
/*
* It is assumed that converter nodes come first in the node list and* hence have been registered and usable now.*/return hdmi_read_pin_conn(codec, pin_nid); }
static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t nid) {
- int i, found_pin = 0; struct hdmi_spec *spec = codec->spec;
- if (spec->num_cvts>= MAX_HDMI_CVTS) {
snd_printk(KERN_WARNING"HDMI: no space for converter %d\n", nid);return -E2BIG;
- for (i = 0; i< spec->num_pins; i++)
if (nid == spec->pin_cvt[i]) {found_pin = 1;break;}- if (!found_pin) {
Can test this instead:
if (hda_node_index(spec->pin_cvt, nid)< 0) {
Yes, that would probably be simpler.
snd_printdd("HDMI: Skipping node %d (no connection)\n", nid);return -EINVAL;The above return actually will hide the device for cvt 3:
+---- pin 4 / cvt 2 ------ pin 5 \ +---- pin 6 cvt 3Which is the default connection for Intel ibexpeak/sandybridge codecs. It might be a problem when the user uses dual displays (which may be configured at runtime). I have no such a setup, so cannot assure things will work or not work..
Hmm, is this really relevant? Looking at the current patch_hdmi.c, I can't see that it ever tries to change pin <-> cvt connections, but I might be missing something. In fact, I have yet to see an HDMI codec where you can actually perform that change, perhaps you can post such a codec-proc file?
On Tue, Nov 23, 2010 at 04:40:49PM +0100, David Henningsson wrote:
On 2010-11-23 16:15, Wu Fengguang wrote:
From ca84aa8edbfb66e46266677249b141b5419d6e0a Mon Sep 17 00:00:00 2001
From: David Henningssondavid.henningsson@canonical.com Date: Tue, 23 Nov 2010 10:23:40 +0100 Subject: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI
Some newer chips have more than one HDMI output, but usually not
Please point out the model name here (where this patch actually makes a difference)?
I'm attaching the codec-proc file for the relevant machine, which lists as "Intel Cougarpoint HDMI".
Yes it's different from old models:
Pin Default 0x58560010: [N/A] Digital Out at Int HDMI Pin Default 0x58560020: [N/A] Digital Out at Int HDMI Pin Default 0x18560030: [Jack] Digital Out at Int HDMI
all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningssondavid.henningsson@canonical.com
sound/pci/hda/patch_hdmi.c | 41 ++++++++++++++++++++++++++++++++--------- 1 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index d3e49aa..14a1087 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -905,23 +905,28 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid) spec->pin[spec->num_pins] = pin_nid; spec->num_pins++;
- /*
* It is assumed that converter nodes come first in the node list and* hence have been registered and usable now.*/}
static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t nid) {
- int i, found_pin = 0; struct hdmi_spec *spec = codec->spec;
- if (spec->num_cvts>= MAX_HDMI_CVTS) {
snd_printk(KERN_WARNING"HDMI: no space for converter %d\n", nid);return -E2BIG;
- for (i = 0; i< spec->num_pins; i++)
if (nid == spec->pin_cvt[i]) {found_pin = 1;break;}- if (!found_pin) {
Can test this instead:
if (hda_node_index(spec->pin_cvt, nid)< 0) {Yes, that would probably be simpler.
snd_printdd("HDMI: Skipping node %d (no connection)\n", nid);return -EINVAL;The above return actually will hide the device for cvt 3:
+---- pin 4 / cvt 2 ------ pin 5 \ +---- pin 6 cvt 3Which is the default connection for Intel ibexpeak/sandybridge codecs. It might be a problem when the user uses dual displays (which may be configured at runtime). I have no such a setup, so cannot assure things will work or not work..
Hmm, is this really relevant? Looking at the current patch_hdmi.c, I can't see that it ever tries to change pin <-> cvt connections, but I might be missing something.
I just feel uncertain. Perhaps when there comes a need in future, we can further do a patch to "restore" the hidden device at runtime?
In fact, I have yet to see an HDMI codec where you can actually perform that change, perhaps you can post such a codec-proc file?
Here is the SandyBridge model. IbexPeak is almost the same.
Thanks, Fengguang ---
Codec: Intel SandyBridge HDMI Address: 3 Function Id: 0x1 Vendor Id: 0x80862805 Subsystem Id: 0x80860101 Revision Id: 0x100000 No Modem Function Group found Default PCM: rates [0x0]: bits [0x0]: formats [0x0]: Default Amp-In caps: N/A Default Amp-Out caps: N/A GPIO: io=0, o=0, i=0, unsolicited=0, wake=0 Node 0x02 [Audio Output] wcaps 0x6611: 8-Channels Digital Control: name="IEC958 Playback Con Mask", index=0, device=0 Control: name="IEC958 Playback Pro Mask", index=0, device=0 Control: name="IEC958 Playback Default", index=0, device=0 Control: name="IEC958 Playback Switch", index=0, device=0 Device: name="INTEL HDMI 0", type="HDMI", device=3 Converter: stream=0, channel=0 Digital: Enabled Digital category: 0x0 PCM: rates [0x7f0]: 32000 44100 48000 88200 96000 176400 192000 bits [0x1e]: 16 20 24 32 formats [0x5]: PCM AC3 Power states: D0 D3 EPSS Power: setting=D0, actual=D0 Node 0x03 [Audio Output] wcaps 0x6611: 8-Channels Digital Control: name="IEC958 Playback Con Mask", index=1, device=0 Control: name="IEC958 Playback Pro Mask", index=1, device=0 Control: name="IEC958 Playback Default", index=1, device=0 Control: name="IEC958 Playback Switch", index=1, device=0 Device: name="INTEL HDMI 1", type="HDMI", device=7 Converter: stream=0, channel=0 Digital: Enabled Digital category: 0x0 PCM: rates [0x7f0]: 32000 44100 48000 88200 96000 176400 192000 bits [0x1e]: 16 20 24 32 formats [0x5]: PCM AC3 Power states: D0 D3 EPSS Power: setting=D0, actual=D0 Node 0x04 [Audio Output] wcaps 0x6611: 8-Channels Digital Control: name="IEC958 Playback Con Mask", index=2, device=0 Control: name="IEC958 Playback Pro Mask", index=2, device=0 Control: name="IEC958 Playback Default", index=2, device=0 Control: name="IEC958 Playback Switch", index=2, device=0 Converter: stream=0, channel=0 Digital: Enabled Digital category: 0x0 PCM: rates [0x7f0]: 32000 44100 48000 88200 96000 176400 192000 bits [0x1e]: 16 20 24 32 formats [0x5]: PCM AC3 Power states: D0 D3 EPSS Power: setting=D0, actual=D0 Node 0x05 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1 Amp-Out vals: [0x00 0x00] Pincap 0x09000094: OUT Detect HBR HDMI DP Pin Default 0x18560010: [Jack] Digital Out at Int HDMI Conn = Digital, Color = Unknown DefAssociation = 0x1, Sequence = 0x0 Pin-ctls: 0x40: OUT Unsolicited: tag=05, enabled=1 Power states: D0 D3 EPSS Power: setting=D0, actual=D0 Connection: 2 0x02* 0x03 Node 0x06 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1 Amp-Out vals: [0x00 0x00] Pincap 0x09000094: OUT Detect HBR HDMI DP Pin Default 0x18560010: [Jack] Digital Out at Int HDMI Conn = Digital, Color = Unknown DefAssociation = 0x1, Sequence = 0x0 Pin-ctls: 0x40: OUT Unsolicited: tag=06, enabled=1 Power states: D0 D3 EPSS Power: setting=D0, actual=D0 Connection: 2 0x02* 0x03 Node 0x07 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1 Amp-Out vals: [0x00 0x00] Pincap 0x09000094: OUT Detect HBR HDMI DP Pin Default 0x18560010: [Jack] Digital Out at Int HDMI Conn = Digital, Color = Unknown DefAssociation = 0x1, Sequence = 0x0 Pin-ctls: 0x40: OUT Unsolicited: tag=07, enabled=1 Power states: D0 D3 EPSS Power: setting=D0, actual=D0 Connection: 2 0x02* 0x03 Node 0x08 [Vendor Defined Widget] wcaps 0xf00000: Mono
On 2010-11-23 16:54, Wu Fengguang wrote:
On Tue, Nov 23, 2010 at 04:40:49PM +0100, David Henningsson wrote:
On 2010-11-23 16:15, Wu Fengguang wrote:
From ca84aa8edbfb66e46266677249b141b5419d6e0a Mon Sep 17 00:00:00 2001
From: David Henningssondavid.henningsson@canonical.com Date: Tue, 23 Nov 2010 10:23:40 +0100 Subject: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI
Some newer chips have more than one HDMI output, but usually not
Please point out the model name here (where this patch actually makes a difference)?
I'm attaching the codec-proc file for the relevant machine, which lists as "Intel Cougarpoint HDMI".
Yes it's different from old models:
Pin Default 0x58560010: [N/A] Digital Out at Int HDMI Pin Default 0x58560020: [N/A] Digital Out at Int HDMI Pin Default 0x18560030: [Jack] Digital Out at Int HDMI
Seems like the BIOS writers actually did what they should in this case, as opposed to leaving them all as "Jack"s?
all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningssondavid.henningsson@canonical.com
sound/pci/hda/patch_hdmi.c | 41 ++++++++++++++++++++++++++++++++--------- 1 files changed, 32 insertions(+), 9 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index d3e49aa..14a1087 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -905,23 +905,28 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid) spec->pin[spec->num_pins] = pin_nid; spec->num_pins++;
/*
* It is assumed that converter nodes come first in the node list and* hence have been registered and usable now.*/return hdmi_read_pin_conn(codec, pin_nid); }
static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t nid) {
- int i, found_pin = 0; struct hdmi_spec *spec = codec->spec;
- if (spec->num_cvts>= MAX_HDMI_CVTS) {
snd_printk(KERN_WARNING"HDMI: no space for converter %d\n", nid);return -E2BIG;
- for (i = 0; i< spec->num_pins; i++)
if (nid == spec->pin_cvt[i]) {found_pin = 1;break;}- if (!found_pin) {
Can test this instead:
if (hda_node_index(spec->pin_cvt, nid)< 0) {Yes, that would probably be simpler.
snd_printdd("HDMI: Skipping node %d (no connection)\n", nid);return -EINVAL;The above return actually will hide the device for cvt 3:
+---- pin 4 / cvt 2 ------ pin 5 \ +---- pin 6 cvt 3Which is the default connection for Intel ibexpeak/sandybridge codecs. It might be a problem when the user uses dual displays (which may be configured at runtime). I have no such a setup, so cannot assure things will work or not work..
Hmm, is this really relevant? Looking at the current patch_hdmi.c, I can't see that it ever tries to change pin<-> cvt connections, but I might be missing something.
I just feel uncertain. Perhaps when there comes a need in future, we can further do a patch to "restore" the hidden device at runtime?
Ok. Well, we could just disable cvt's that has no possible connections to a Jack (as compared to no *active* connections), but on the other hand - the day we need to change pin-cvt connections at runtime, we'll probably have to revisit this patch, as well as a lot of other code in patch_hdmi.c.
It seems like an unusual use case to me to need two different HDMI audio streams to two different displays, but who knows...
In fact, I have yet to see an HDMI codec where you can actually perform that change, perhaps you can post such a codec-proc file?
Here is the SandyBridge model. IbexPeak is almost the same.
Thanks!
On Tue, Nov 23, 2010 at 04:40:49PM +0100, David Henningsson wrote:
On 2010-11-23 16:15, Wu Fengguang wrote:
From ca84aa8edbfb66e46266677249b141b5419d6e0a Mon Sep 17 00:00:00 2001
From: David Henningssondavid.henningsson@canonical.com Date: Tue, 23 Nov 2010 10:23:40 +0100 Subject: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI
Some newer chips have more than one HDMI output, but usually not
Please point out the model name here (where this patch actually makes a difference)?
I'm attaching the codec-proc file for the relevant machine, which lists as "Intel Cougarpoint HDMI".
For the record, here is the one I have.
Pin Default 0x18560010: [Jack] Digital Out at Int HDMI Pin Default 0x18560020: [Jack] Digital Out at Int HDMI Pin Default 0x18560030: [Jack] Digital Out at Int HDMI
Thanks, Fengguang ---
Codec: Intel CougarPoint HDMI Address: 3 AFG Function Id: 0x1 (unsol 0) Vendor Id: 0x80862805 Subsystem Id: 0x80860101 Revision Id: 0x100000 No Modem Function Group found Default PCM: rates [0x0]: bits [0x0]: formats [0x0]: Default Amp-In caps: N/A Default Amp-Out caps: N/A GPIO: io=0, o=0, i=0, unsolicited=0, wake=0 Node 0x02 [Audio Output] wcaps 0x6611: 8-Channels Digital Control: name="IEC958 Playback Con Mask", index=0, device=0 Control: name="IEC958 Playback Pro Mask", index=0, device=0 Control: name="IEC958 Playback Default", index=0, device=0 Control: name="IEC958 Playback Switch", index=0, device=0 Device: name="HDMI 0", type="HDMI", device=3 Converter: stream=0, channel=0 Digital: Enabled Digital category: 0x0 PCM: rates [0x7f0]: 32000 44100 48000 88200 96000 176400 192000 bits [0x1e]: 16 20 24 32 formats [0x5]: PCM AC3 Power states: D0 D3 EPSS Power: setting=D0, actual=D0 Node 0x03 [Audio Output] wcaps 0x6611: 8-Channels Digital Control: name="IEC958 Playback Con Mask", index=1, device=0 Control: name="IEC958 Playback Pro Mask", index=1, device=0 Control: name="IEC958 Playback Default", index=1, device=0 Control: name="IEC958 Playback Switch", index=1, device=0 Device: name="HDMI 1", type="HDMI", device=7 Converter: stream=0, channel=0 Digital: Enabled Digital category: 0x0 PCM: rates [0x7f0]: 32000 44100 48000 88200 96000 176400 192000 bits [0x1e]: 16 20 24 32 formats [0x5]: PCM AC3 Power states: D0 D3 EPSS Power: setting=D0, actual=D0 Node 0x04 [Audio Output] wcaps 0x6611: 8-Channels Digital Control: name="IEC958 Playback Con Mask", index=2, device=0 Control: name="IEC958 Playback Pro Mask", index=2, device=0 Control: name="IEC958 Playback Default", index=2, device=0 Control: name="IEC958 Playback Switch", index=2, device=0 Device: name="HDMI 2", type="HDMI", device=8 Converter: stream=8, channel=0 Digital: Enabled Digital category: 0x0 PCM: rates [0x7f0]: 32000 44100 48000 88200 96000 176400 192000 bits [0x1e]: 16 20 24 32 formats [0x5]: PCM AC3 Power states: D0 D3 EPSS Power: setting=D0, actual=D0 Node 0x05 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1 Amp-Out vals: [0x00 0x00] Pincap 0x09000094: OUT Detect HBR HDMI DP Pin Default 0x18560010: [Jack] Digital Out at Int HDMI Conn = Digital, Color = Unknown DefAssociation = 0x1, Sequence = 0x0 Pin-ctls: 0x40: OUT Unsolicited: tag=05, enabled=1 Power states: D0 D3 EPSS Power: setting=D0, actual=D0 Connection: 1 0x02 Node 0x06 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1 Amp-Out vals: [0x00 0x00] Pincap 0x09000094: OUT Detect HBR HDMI DP Pin Default 0x18560020: [Jack] Digital Out at Int HDMI Conn = Digital, Color = Unknown DefAssociation = 0x2, Sequence = 0x0 Pin-ctls: 0x40: OUT Unsolicited: tag=06, enabled=1 Power states: D0 D3 EPSS Power: setting=D0, actual=D0 Connection: 1 0x03 Node 0x07 [Pin Complex] wcaps 0x40778d: 8-Channels Digital Amp-Out CP Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1 Amp-Out vals: [0x00 0x00] Pincap 0x09000094: OUT Detect HBR HDMI DP Pin Default 0x18560030: [Jack] Digital Out at Int HDMI Conn = Digital, Color = Unknown DefAssociation = 0x3, Sequence = 0x0 Pin-ctls: 0x40: OUT Unsolicited: tag=07, enabled=1 Power states: D0 D3 EPSS Power: setting=D0, actual=D0 Connection: 1 0x04 Node 0x08 [Vendor Defined Widget] wcaps 0xf00000: Mono
At Fri, 12 Nov 2010 16:46:34 +0100, David Henningsson wrote:
Since I'm a little new in HDMI land and the patch is non-trivial, I wouldn't mind some comments on this one. Also thanks to Jaroslav who cleared a few things out about the logical and physical devices which helped me figure out what I should do about it :-)
Anyway, here's the patch.
Some newer chips have more than one HDMI output, but usually not all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningsson david.henningsson@canonical.com
Should I apply this now, or would you make another small fix Fengguang suggested?
thanks,
Takashi
Takashi Iwai <tiwai <at> suse.de> writes:
Should I apply this now, or would you make another small fix Fengguang suggested?
I still have this problem... https://bugtrack.alsa-project.org/alsa-bug/view.php?id=5522
what can we do about it?
-arne
participants (4)
-
David Henningsson -
RIDDICC -
Takashi Iwai -
Wu Fengguang