| Message ID | 20191127112536.28791-1-kai.vehmanen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda: hdmi - fix regression in connect list handling | expand |
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
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
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
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;
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(-)