[PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 736 bytes --]

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>

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

[-- Attachment #2: 0001-ALSA-HDA-Remove-unconnected-PCM-devices-for-Intel-HD.patch --]
[-- Type: text/x-patch, Size: 3265 bytes --]

>From db7911afdd02648b1faf6e2e7c8024da20d88257 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Thu, 11 Nov 2010 16:27:37 +0100
Subject: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

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>
---
 hda/patch_hdmi.c |   37 ++++++++++++++++++++++++++++---------
 1 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/hda/patch_hdmi.c b/hda/patch_hdmi.c
index d3e49aa..6d964f4 100644
--- a/hda/patch_hdmi.c
+++ b/hda/patch_hdmi.c
@@ -905,22 +905,22 @@ 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;
 	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++) 
+		if (!spec->pin[i]) {
+			snd_printd("HDMI: Skipping node %d (no connection)\n", nid);
+			return -EINVAL;
+		}
+
+	if (snd_BUG_ON(spec->num_cvts >= MAX_HDMI_CVTS))
 		return -E2BIG;
-	}
 
 	spec->cvt[spec->num_cvts] = nid;
 	spec->num_cvts++;
@@ -932,6 +932,8 @@ static int hdmi_parse_codec(struct hda_codec *codec)
 {
 	hda_nid_t nid;
 	int i, nodes;
+	int num_tmp_cvts = 0;
+	hda_nid_t tmp_cvt[MAX_HDMI_CVTS];
 
 	nodes = snd_hda_get_sub_nodes(codec, codec->afg, &nid);
 	if (!nid || nodes < 0) {
@@ -942,6 +944,7 @@ static int hdmi_parse_codec(struct hda_codec *codec)
 	for (i = 0; i < nodes; i++, nid++) {
 		unsigned int caps;
 		unsigned int type;
+		unsigned int config;
 
 		caps = snd_hda_param_read(codec, nid, AC_PAR_AUDIO_WIDGET_CAP);
 		type = get_wcaps_type(caps);
@@ -951,17 +954,33 @@ static int hdmi_parse_codec(struct hda_codec *codec)
 
 		switch (type) {
 		case AC_WID_AUD_OUT:
-			hdmi_add_cvt(codec, nid);
+			if (num_tmp_cvts >= MAX_HDMI_CVTS) {
+				snd_printk(KERN_WARNING
+					   "HDMI: no space for converter %d\n", nid);
+				continue;
+			}
+			tmp_cvt[num_tmp_cvts] = nid;
+			num_tmp_cvts++;
 			break;
 		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)
+				continue;
+
 			hdmi_add_pin(codec, nid);
 			break;
 		}
 	}
 
+	for (i = 0; i < num_tmp_cvts; i++) 
+		hdmi_add_cvt(codec, tmp_cvt[i]);
+
 	/*
 	 * G45/IbexPeak don't support EPSS: the unsolicited pin hot plug event
 	 * can be lost and presence sense verb will become inaccurate if the
-- 
1.7.1


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[Takashi Iwai]

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

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

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 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 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)?

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

[-- Attachment #2: 0001-ALSA-HDA-Remove-unconnected-PCM-devices-for-Intel-HD.patch --]
[-- Type: text/x-patch, Size: 3363 bytes --]

>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
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.
-	 */
 	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) {
+		snd_printdd("HDMI: Skipping node %d (no connection)\n", nid);
+		return -EINVAL;
 	}
 
+	if (snd_BUG_ON(spec->num_cvts >= MAX_HDMI_CVTS))
+		return -E2BIG;
+
 	spec->cvt[spec->num_cvts] = nid;
 	spec->num_cvts++;
 
@@ -932,6 +937,8 @@ static int hdmi_parse_codec(struct hda_codec *codec)
 {
 	hda_nid_t nid;
 	int i, nodes;
+	int num_tmp_cvts = 0;
+	hda_nid_t tmp_cvt[MAX_HDMI_CVTS];
 
 	nodes = snd_hda_get_sub_nodes(codec, codec->afg, &nid);
 	if (!nid || nodes < 0) {
@@ -942,6 +949,7 @@ static int hdmi_parse_codec(struct hda_codec *codec)
 	for (i = 0; i < nodes; i++, nid++) {
 		unsigned int caps;
 		unsigned int type;
+		unsigned int config;
 
 		caps = snd_hda_param_read(codec, nid, AC_PAR_AUDIO_WIDGET_CAP);
 		type = get_wcaps_type(caps);
@@ -951,17 +959,32 @@ static int hdmi_parse_codec(struct hda_codec *codec)
 
 		switch (type) {
 		case AC_WID_AUD_OUT:
-			hdmi_add_cvt(codec, nid);
+			if (num_tmp_cvts >= MAX_HDMI_CVTS) {
+				snd_printk(KERN_WARNING
+					   "HDMI: no space for converter %d\n", nid);
+				continue;
+			}
+			tmp_cvt[num_tmp_cvts] = nid;
+			num_tmp_cvts++;
 			break;
 		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 (get_defcfg_connect(config) == AC_JACK_PORT_NONE)
+				continue;
+
 			hdmi_add_pin(codec, nid);
 			break;
 		}
 	}
 
+	for (i = 0; i < num_tmp_cvts; i++)
+		hdmi_add_cvt(codec, tmp_cvt[i]);
+
 	/*
 	 * G45/IbexPeak don't support EPSS: the unsolicited pin hot plug event
 	 * can be lost and presence sense verb will become inaccurate if the
-- 
1.7.1


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[Wu Fengguang]

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 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 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

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[Takashi Iwai]

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 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 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

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[David Henningsson]

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 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 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)?

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[Wu Fengguang]

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 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 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

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[Wu Fengguang]

> >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.
> -	 */
>  	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) {


> +		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

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 3078 bytes --]

On 2010-11-23 16:15, Wu Fengguang wrote:
>> > 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)?

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 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.
>> -	 */
>>   	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 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..

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?

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

[-- Attachment #2: hdmi-codec.txt --]
[-- Type: text/plain, Size: 3656 bytes --]

Codec: Intel CougarPoint HDMI
Address: 3
Function Id: 0x1
Vendor Id: 0x80862805
Subsystem Id: 0x80862805
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=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 0", type="HDMI", device=3
  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 0x03 [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="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=3, device=0
  Control: name="IEC958 Playback Pro Mask", index=3, device=0
  Control: name="IEC958 Playback Default", index=3, device=0
  Control: name="IEC958 Playback Switch", index=3, device=0
  Device: name="INTEL HDMI 2", type="HDMI", device=8
  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 0x58560010: [N/A] 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 0x58560020: [N/A] 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


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[Wu Fengguang]

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 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)?
> 
> 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 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.
> >>-	 */
> >>  	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 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..
> 
> 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

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[Wu Fengguang]

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 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)?
>
> 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

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[Takashi Iwai]

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

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[David Henningsson]

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 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)?
>>
>> 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 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.
>>>> -	 */
>>>>   	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 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..
>>
>> 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!

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[Takashi Iwai]

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

Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[RIDDICC]

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