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

[Wu Fengguang <wfg@linux.intel.com>]

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