ALSA: hda: hdmi - fix regression in connect list handling
diff mbox series

Message ID 20191127112536.28791-1-kai.vehmanen@linux.intel.com
State New
Headers show
Series
  • ALSA: hda: hdmi - fix regression in connect list handling
Related show

Commit Message

Kai Vehmanen Nov. 27, 2019, 11:25 a.m. UTC
Fix regression in how intel_haswell_fixup_connect_list()
results are used in hda_read_pin_conn(). Use of
snd_hda_get_raw_connections() in hda_read_pin_conn() bypasses
the cache and thus also bypasses the overridden pin connection
list. On platforms that require the connection list fixup,
mux list will be empty and HDMI playback will fail to -EBUSY
at open.

Move intel_haswell_fixup_connect_list() call to hda_read_pin_conn()
as it's not used elsewhere.

Fixes: 9c32fea83692 ("ALSA: hda - Add DP-MST support for non-acomp codecs")
BugLink: https://github.com/thesofproject/linux/issues/1537
Cc: Nikhil Mahale <nmahale@nvidia.com>
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
---
 sound/pci/hda/patch_hdmi.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comments

Kai Vehmanen Nov. 27, 2019, 11:30 a.m. UTC | #1
Hi Takashi and Nikhil,

On Wed, 27 Nov 2019, Kai Vehmanen wrote:

> Fix regression in how intel_haswell_fixup_connect_list()
> results are used in hda_read_pin_conn(). Use of
> snd_hda_get_raw_connections() in hda_read_pin_conn() bypasses
> the cache and thus also bypasses the overridden pin connection

the original series did pass our SOF CI, but as we have now results from 
wider testing, it unfortunately seems a regression slipped through.
Re-reviewing the code, this bit seems clearly wrong, we can't use
get_raw_connections() if we depend on the connection overrides.

This patch should not affect non-Intel platforms, but please review
in any case.

Br, Kai
Takashi Iwai Nov. 27, 2019, 11:47 a.m. UTC | #2
On Wed, 27 Nov 2019 12:25:36 +0100,
Kai Vehmanen wrote:
> 
> Fix regression in how intel_haswell_fixup_connect_list()
> results are used in hda_read_pin_conn(). Use of
> snd_hda_get_raw_connections() in hda_read_pin_conn() bypasses
> the cache and thus also bypasses the overridden pin connection
> list. On platforms that require the connection list fixup,
> mux list will be empty and HDMI playback will fail to -EBUSY
> at open.

Ah, thanks for catching this.  This is an obvious overlook.

> @@ -1312,10 +1316,19 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
>  
>  	snd_hda_set_dev_select(codec, pin_nid, dev_id);
>  
> +	if (spec->intel_hsw_fixup) {
> +		intel_haswell_fixup_connect_list(codec, pin_nid);
> +		conns = snd_hda_get_connections(codec, pin_nid,
> +						per_pin->mux_nids,
> +						HDA_MAX_CONNECTIONS);
> +	} else {
> +		conns = snd_hda_get_raw_connections(codec, pin_nid,
> +						    per_pin->mux_nids,
> +						    HDA_MAX_CONNECTIONS);
> +	}

Actually intel_haswell_fixup_connect_list() doesn't influence on the
hardware setup but just updates the software cache.  So, basically we
can copy the values directly from spec->cvt_nids here without the
override hack as we have now.

That is, something like

	if (spec->intel_hsw_fixup) {
		conns = spec->cvt_nids;
		memcpy(per_pin->mux_nids, spec->num_cvts,
		       sizeof(hda_nid_t) * conns);
	} else {
	  	snd_hda_set_dev_select(codec, pin_nid, dev_id);
		conns = snd_hda_get_raw_connections(codec, pin_nid,
						    per_pin->mux_nids,
						    HDA_MAX_CONNECTIONS);
	}

Could you check whether this works?	


thanks,

Takashi
Kai Vehmanen Nov. 27, 2019, 12:14 p.m. UTC | #3
Hi,

On Wed, 27 Nov 2019, Takashi Iwai wrote:

> On Wed, 27 Nov 2019 12:25:36 +0100, Kai Vehmanen wrote:
> > @@ -1312,10 +1316,19 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
[...]
> > +	if (spec->intel_hsw_fixup) {
> > +		intel_haswell_fixup_connect_list(codec, pin_nid);
> > +		conns = snd_hda_get_connections(codec, pin_nid,
> > +						per_pin->mux_nids,
> > +						HDA_MAX_CONNECTIONS);
> > +	} else {
> > +		conns = snd_hda_get_raw_connections(codec, pin_nid,
> > +						    per_pin->mux_nids,
> > +						    HDA_MAX_CONNECTIONS);
> > +	}
> 
> Actually intel_haswell_fixup_connect_list() doesn't influence on the
> hardware setup but just updates the software cache.  So, basically we
> can copy the values directly from spec->cvt_nids here without the
> override hack as we have now.
> 
> That is, something like
> 
> 	if (spec->intel_hsw_fixup) {
> 		conns = spec->cvt_nids;
> 		memcpy(per_pin->mux_nids, spec->num_cvts,
> 		       sizeof(hda_nid_t) * conns);
> 	} else {
> 	  	snd_hda_set_dev_select(codec, pin_nid, dev_id);
> 		conns = snd_hda_get_raw_connections(codec, pin_nid,
> 						    per_pin->mux_nids,
> 						    HDA_MAX_CONNECTIONS);
> 	}
> 
> Could you check whether this works?	

hmm, you are right, this should work. spec->cvt_nids and spec->num_cvts 
were reversed in the proto code :), but otherwise looks ok based on
a few tests. I'll send a v2 patch shortly. This simplifies the code as 
well.

Br, Kai

Patch
diff mbox series

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 55d20e40a195..c111bdd566c9 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1293,6 +1293,9 @@  static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
 	return err;
 }
 
+static void intel_haswell_fixup_connect_list(struct hda_codec *codec,
+					     hda_nid_t nid);
+
 /*
  * HDA/HDMI auto parsing
  */
@@ -1302,6 +1305,7 @@  static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
 	struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
 	hda_nid_t pin_nid = per_pin->pin_nid;
 	int dev_id = per_pin->dev_id;
+	int conns;
 
 	if (!(get_wcaps(codec, pin_nid) & AC_WCAP_CONN_LIST)) {
 		codec_warn(codec,
@@ -1312,10 +1316,19 @@  static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx)
 
 	snd_hda_set_dev_select(codec, pin_nid, dev_id);
 
+	if (spec->intel_hsw_fixup) {
+		intel_haswell_fixup_connect_list(codec, pin_nid);
+		conns = snd_hda_get_connections(codec, pin_nid,
+						per_pin->mux_nids,
+						HDA_MAX_CONNECTIONS);
+	} else {
+		conns = snd_hda_get_raw_connections(codec, pin_nid,
+						    per_pin->mux_nids,
+						    HDA_MAX_CONNECTIONS);
+	}
+
 	/* all the device entries on the same pin have the same conn list */
-	per_pin->num_mux_nids =
-		snd_hda_get_raw_connections(codec, pin_nid, per_pin->mux_nids,
-					    HDA_MAX_CONNECTIONS);
+	per_pin->num_mux_nids = conns;
 
 	return 0;
 }
@@ -1713,9 +1726,6 @@  static void hdmi_repoll_eld(struct work_struct *work)
 	mutex_unlock(&spec->pcm_lock);
 }
 
-static void intel_haswell_fixup_connect_list(struct hda_codec *codec,
-					     hda_nid_t nid);
-
 static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 {
 	struct hdmi_spec *spec = codec->spec;
@@ -1790,8 +1800,6 @@  static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid)
 		per_pin->dev_id = i;
 		per_pin->non_pcm = false;
 		snd_hda_set_dev_select(codec, pin_nid, i);
-		if (spec->intel_hsw_fixup)
-			intel_haswell_fixup_connect_list(codec, pin_nid);
 		err = hdmi_read_pin_conn(codec, pin_idx);
 		if (err < 0)
 			return err;