On Tue, Nov 23, 2010 at 03:19:58PM +0100, David Henningsson wrote:
On 2010-11-23 14:51, Takashi Iwai wrote:
At Tue, 23 Nov 2010 19:06:39 +0800, Wu Fengguang wrote:
On Tue, Nov 23, 2010 at 10:29:12AM +0100, David Henningsson wrote:
On 2010-11-23 08:12, Takashi Iwai wrote:
At Fri, 12 Nov 2010 16:46:34 +0100, David Henningsson wrote:
Since I'm a little new in HDMI land and the patch is non-trivial, I wouldn't mind some comments on this one. Also thanks to Jaroslav who cleared a few things out about the logical and physical devices which helped me figure out what I should do about it :-)
Anyway, here's the patch.
Some newer chips have more than one HDMI output, but usually not all of them are exposed as physical jacks. Removing the unused PCM devices (as indicated by BIOS in the pin config default) will reduce user confusion as they currently have to choose between several HDMI devices, some of them not working anyway.
Signed-off-by: David Henningssondavid.henningsson@canonical.com
The patch looks mostly OK. But this gives a few errors and warnings via checkpatch.pl. Could you fix errors, at least?
Also some review comments:
static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t nid) {
- int i; struct hdmi_spec *spec = codec->spec;
- if (spec->num_cvts>= MAX_HDMI_CVTS) {
snd_printk(KERN_WARNING"HDMI: no space for converter %d\n", nid);
- for (i = 0; spec->pin_cvt[i] != nid; i++)
I'd write with a normal loop like for (i = 0; i< spec->num_pins; i++) This is easier to read.
if (!spec->pin[i]) {snd_printd("HDMI: Skipping node %d (no connection)\n", nid);Better to use snd_printdd(). snd_printd() is enabled on many distros, and such an unnecessary kernel message may make user worry.
@@ -951,17 +954,33 @@ static int hdmi_parse_codec(struct hda_codec *codec)
...
case AC_WID_PIN: caps = snd_hda_param_read(codec, nid, AC_PAR_PIN_CAP); if (!(caps& (AC_PINCAP_HDMI | AC_PINCAP_DP))) continue;
config = snd_hda_codec_read(codec, nid, 0,AC_VERB_GET_CONFIG_DEFAULT, 0);if (((config& AC_DEFCFG_PORT_CONN)>>AC_DEFCFG_PORT_CONN_SHIFT) == AC_JACK_PORT_NONE)You can use get_defcfg_connect() here.
Thanks for the review, Takashi! I usually run checkpatch.pl these days but this one must have slipped. What do you think of this patch (untested)?
Thanks for the patch, I'll help test it out :)
I didn't do such feature because I'm not sure if the BIOS provided info can be trusted,
Perhaps Wu knows what Intel's default is, would that be all outputs configured as Jacks, or as N/A? I'm thinking that if it is Jacks, then BIOS changing that to N/A probably knows what it's doing. If it is the other way around - perhaps we should add something saying that if it's all N/A, ignore the extra check and enable all outputs.
Anyway it's overridable through user_pin_configs / driver_pin_configs.
Right. Some time ago I have to do a hacking patch to override the (alpha-)BIOS pin connections to get HDMI audio. But your patch makes sense for the normal cases.
or if that's merely the default configuration that may be changed runtime on user request (eg. switching from A+A view to A+B view etc.).
Ah, interesting. But if A+B view is possible, wouldn't BIOS then configure both HDMI outputs as Jacks?
Sounds reasonable.
Well, whether this BIOS setup is really reliable is still an open question. David, do you know (or have) the case whether your patch works on a real machine, right?
Yes; although this latest version of the patch has not been tested on that machine yet.
What's your codec# file? I have saved copies for Intel eaglelake, ibexpeak, sandybridge codecs, and all of their pins show up as
Pin Default 0x185600*0: [Jack] Digital Out at Int HDMI
The highest two bits are AC_JACK_PORT_COMPLEX=0. So your patch should not make any difference for them.
Thanks, Fengguang