* [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI @ 2010-11-12 15:46 David Henningsson 2010-11-23 7:12 ` Takashi Iwai 2010-12-07 19:10 ` Takashi Iwai 0 siblings, 2 replies; 15+ messages in thread From: David Henningsson @ 2010-11-12 15:46 UTC (permalink / raw) To: ALSA Development Mailing List, Takashi Iwai; +Cc: wfg [-- 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-12 15:46 [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI David Henningsson @ 2010-11-23 7:12 ` Takashi Iwai 2010-11-23 9:29 ` David Henningsson 2010-12-07 19:10 ` Takashi Iwai 1 sibling, 1 reply; 15+ messages in thread From: Takashi Iwai @ 2010-11-23 7:12 UTC (permalink / raw) To: David Henningsson; +Cc: ALSA Development Mailing List, wfg 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-23 7:12 ` Takashi Iwai @ 2010-11-23 9:29 ` David Henningsson 2010-11-23 11:06 ` Wu Fengguang 2010-11-23 15:15 ` Wu Fengguang 0 siblings, 2 replies; 15+ messages in thread From: David Henningsson @ 2010-11-23 9:29 UTC (permalink / raw) To: Takashi Iwai; +Cc: ALSA Development Mailing List, wfg [-- 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-23 9:29 ` David Henningsson @ 2010-11-23 11:06 ` Wu Fengguang 2010-11-23 13:51 ` Takashi Iwai 2010-11-23 15:15 ` Wu Fengguang 1 sibling, 1 reply; 15+ messages in thread From: Wu Fengguang @ 2010-11-23 11:06 UTC (permalink / raw) To: David Henningsson; +Cc: Takashi Iwai, alsa-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-23 11:06 ` Wu Fengguang @ 2010-11-23 13:51 ` Takashi Iwai 2010-11-23 14:19 ` David Henningsson 0 siblings, 1 reply; 15+ messages in thread From: Takashi Iwai @ 2010-11-23 13:51 UTC (permalink / raw) To: Wu Fengguang; +Cc: alsa-devel, David Henningsson 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-23 13:51 ` Takashi Iwai @ 2010-11-23 14:19 ` David Henningsson 2010-11-23 14:58 ` Wu Fengguang 2010-11-24 9:39 ` Takashi Iwai 0 siblings, 2 replies; 15+ messages in thread From: David Henningsson @ 2010-11-23 14:19 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, Wu Fengguang 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-23 14:19 ` David Henningsson @ 2010-11-23 14:58 ` Wu Fengguang 2010-11-24 9:39 ` Takashi Iwai 1 sibling, 0 replies; 15+ messages in thread From: Wu Fengguang @ 2010-11-23 14:58 UTC (permalink / raw) To: David Henningsson; +Cc: Takashi Iwai, alsa-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-23 14:19 ` David Henningsson 2010-11-23 14:58 ` Wu Fengguang @ 2010-11-24 9:39 ` Takashi Iwai 1 sibling, 0 replies; 15+ messages in thread From: Takashi Iwai @ 2010-11-24 9:39 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel, Wu 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-23 9:29 ` David Henningsson 2010-11-23 11:06 ` Wu Fengguang @ 2010-11-23 15:15 ` Wu Fengguang 2010-11-23 15:40 ` David Henningsson 1 sibling, 1 reply; 15+ messages in thread From: Wu Fengguang @ 2010-11-23 15:15 UTC (permalink / raw) To: David Henningsson; +Cc: Takashi Iwai, ALSA Development Mailing List > >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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-23 15:15 ` Wu Fengguang @ 2010-11-23 15:40 ` David Henningsson 2010-11-23 15:54 ` Wu Fengguang 2010-11-23 16:16 ` Wu Fengguang 0 siblings, 2 replies; 15+ messages in thread From: David Henningsson @ 2010-11-23 15:40 UTC (permalink / raw) To: Wu Fengguang; +Cc: Takashi Iwai, ALSA Development Mailing List [-- 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-23 15:40 ` David Henningsson @ 2010-11-23 15:54 ` Wu Fengguang 2010-11-24 13:50 ` David Henningsson 2010-11-23 16:16 ` Wu Fengguang 1 sibling, 1 reply; 15+ messages in thread From: Wu Fengguang @ 2010-11-23 15:54 UTC (permalink / raw) To: David Henningsson; +Cc: Takashi Iwai, ALSA Development Mailing List 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-23 15:54 ` Wu Fengguang @ 2010-11-24 13:50 ` David Henningsson 0 siblings, 0 replies; 15+ messages in thread From: David Henningsson @ 2010-11-24 13:50 UTC (permalink / raw) To: Wu Fengguang; +Cc: Takashi Iwai, ALSA Development Mailing List 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-23 15:40 ` David Henningsson 2010-11-23 15:54 ` Wu Fengguang @ 2010-11-23 16:16 ` Wu Fengguang 1 sibling, 0 replies; 15+ messages in thread From: Wu Fengguang @ 2010-11-23 16:16 UTC (permalink / raw) To: David Henningsson; +Cc: Takashi Iwai, ALSA Development Mailing List 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-11-12 15:46 [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI David Henningsson 2010-11-23 7:12 ` Takashi Iwai @ 2010-12-07 19:10 ` Takashi Iwai 2012-02-01 12:38 ` RIDDICC 1 sibling, 1 reply; 15+ messages in thread From: Takashi Iwai @ 2010-12-07 19:10 UTC (permalink / raw) To: David Henningsson; +Cc: ALSA Development Mailing List, wfg 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI 2010-12-07 19:10 ` Takashi Iwai @ 2012-02-01 12:38 ` RIDDICC 0 siblings, 0 replies; 15+ messages in thread From: RIDDICC @ 2012-02-01 12:38 UTC (permalink / raw) To: alsa-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-02-01 14:15 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-11-12 15:46 [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI David Henningsson 2010-11-23 7:12 ` Takashi Iwai 2010-11-23 9:29 ` David Henningsson 2010-11-23 11:06 ` Wu Fengguang 2010-11-23 13:51 ` Takashi Iwai 2010-11-23 14:19 ` David Henningsson 2010-11-23 14:58 ` Wu Fengguang 2010-11-24 9:39 ` Takashi Iwai 2010-11-23 15:15 ` Wu Fengguang 2010-11-23 15:40 ` David Henningsson 2010-11-23 15:54 ` Wu Fengguang 2010-11-24 13:50 ` David Henningsson 2010-11-23 16:16 ` Wu Fengguang 2010-12-07 19:10 ` Takashi Iwai 2012-02-01 12:38 ` RIDDICC
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).