[PATCH] ALSA: hda/hdmi: Don't share an assigned converter on NVIDIA codecs

[PATCH] ALSA: hda/hdmi: Don't share an assigned converter on NVIDIA codecs

[Razvan Dinculescu]

All pins of the NVIDIA HDMI/DP codecs share one connection list, so a
converter can be selected by several pins at once. Unlike the Intel
codecs (intel_not_share_assigned_cvt()), the NVIDIA ones never move an
idle pin off the converter that another pin streams on, and the display
that should play stays silent.

Seen on an RTX 5090 (codec 0x10de00aa) with a DP monitor on pin 0x04
and an HDMI TV on pin 0x05. PipeWire probes every PCM at start-up: the
monitor's PCM got converter 0x08 and kept it selected after closing,
then the TV's PCM got 0x08 as well when it was opened. With both pins
on 0x08 the TV played nothing, although its PCM was running and its
ELD was valid. Opening the monitor's PCM while the TV's was running
moved the monitor to 0x09 and the TV played at once. Restarting
PipeWire put both pins back on 0x08.

Implement pin_cvt_fixup for the generic NVIDIA model as Intel does:
when a converter is assigned to a pin, move every other connected pin
that selects it to an unassigned converter. Look the converter up in
each pin's own connection list instead of assuming the same order on
every pin.

With this change, three PipeWire restarts in a row left the monitor on
0x09 and the TV on 0x08, and the monitor pin forced back onto 0x08 with
hda-verb was moved off it at the next prepare.

Signed-off-by: Razvan Dinculescu <razvandinculescu@gmail.com>
---
 sound/hda/codecs/hdmi/nvhdmi.c | 76 ++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/sound/hda/codecs/hdmi/nvhdmi.c b/sound/hda/codecs/hdmi/nvhdmi.c
index 94671ad..95ae773 100644
--- a/sound/hda/codecs/hdmi/nvhdmi.c
+++ b/sound/hda/codecs/hdmi/nvhdmi.c
@@ -66,6 +66,81 @@ static const struct drm_audio_component_audio_ops nvhdmi_audio_ops = {
 	.master_unbind = snd_hda_hdmi_acomp_master_unbind,
 };
 
+/*
+ * All pins of these codecs share one connection list, as on Intel.  An idle
+ * pin left selecting the converter that another pin streams on takes that
+ * stream: the display that plays stays silent and the audio goes to the
+ * other display instead.  Seen on GB202 (0x10de00aa) with an HDMI TV and a
+ * DP monitor: whenever the monitor's PCM was opened first (PipeWire probes
+ * every PCM at start-up), its pin kept the converter the TV got next.
+ *
+ * Move every other connected pin off the converter just assigned, to a
+ * converter nobody uses.
+ */
+static void nvhdmi_not_share_assigned_cvt(struct hda_codec *codec,
+					  hda_nid_t pin_nid, int dev_id,
+					  hda_nid_t cvt_nid)
+{
+	struct hdmi_spec *spec = codec->spec;
+	struct hdmi_spec_per_pin *per_pin;
+	struct hdmi_spec_per_cvt *per_cvt;
+	int pin_idx, cvt_idx, mux_idx, curr, dev_id_saved;
+
+	if (!cvt_nid)
+		return;
+
+	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
+		per_pin = get_pin(spec, pin_idx);
+		/* no display on this pin */
+		if (!per_pin->pcm)
+			continue;
+		if (per_pin->pin_nid == pin_nid && per_pin->dev_id == dev_id)
+			continue;
+		if (per_pin->dev_id >=
+		    snd_hda_get_num_devices(codec, per_pin->pin_nid) + 1)
+			continue;
+
+		dev_id_saved = snd_hda_get_dev_select(codec, per_pin->pin_nid);
+		snd_hda_set_dev_select(codec, per_pin->pin_nid, per_pin->dev_id);
+		curr = snd_hda_codec_read(codec, per_pin->pin_nid, 0,
+					  AC_VERB_GET_CONNECT_SEL, 0);
+		if (curr < 0 || curr >= per_pin->num_mux_nids ||
+		    per_pin->mux_nids[curr] != cvt_nid)
+			goto restore;
+
+		for (cvt_idx = 0; cvt_idx < spec->num_cvts; cvt_idx++) {
+			per_cvt = get_cvt(spec, cvt_idx);
+			if (per_cvt->assigned || per_cvt->cvt_nid == cvt_nid)
+				continue;
+			for (mux_idx = 0; mux_idx < per_pin->num_mux_nids; mux_idx++)
+				if (per_pin->mux_nids[mux_idx] == per_cvt->cvt_nid)
+					break;
+			if (mux_idx == per_pin->num_mux_nids)
+				continue;
+			codec_dbg(codec,
+				  "hdmi: move pin 0x%x off cvt 0x%x to cvt 0x%x\n",
+				  per_pin->pin_nid, cvt_nid, per_cvt->cvt_nid);
+			snd_hda_codec_write_cache(codec, per_pin->pin_nid, 0,
+						  AC_VERB_SET_CONNECT_SEL,
+						  mux_idx);
+			break;
+		}
+restore:
+		snd_hda_set_dev_select(codec, per_pin->pin_nid, dev_id_saved);
+	}
+}
+
+static void nvhdmi_pin_cvt_fixup(struct hda_codec *codec,
+				 struct hdmi_spec_per_pin *per_pin,
+				 hda_nid_t cvt_nid)
+{
+	if (per_pin)
+		nvhdmi_not_share_assigned_cvt(codec, per_pin->pin_nid,
+					      per_pin->dev_id, per_pin->cvt_nid);
+	else
+		nvhdmi_not_share_assigned_cvt(codec, 0, 0, cvt_nid);
+}
+
 static int probe_generic(struct hda_codec *codec)
 {
 	struct hdmi_spec *spec;
@@ -77,6 +152,7 @@ static int probe_generic(struct hda_codec *codec)
 	codec->dp_mst = true;
 
 	spec = codec->spec;
+	spec->ops.pin_cvt_fixup = nvhdmi_pin_cvt_fixup;
 
 	err = snd_hda_hdmi_parse_codec(codec);
 	if (err < 0) {
-- 
2.56.0

Re: [PATCH] ALSA: hda/hdmi: Don't share an assigned converter on NVIDIA codecs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ALSA: hda/hdmi: missing active pin mux verification in nvhdmi_pin_cvt_fixup()
--

commit ee77c613ae50ec059d4a1e50bc80fe9f42246e54
Author: Razvan Dinculescu <razvandinculescu@gmail.com>

ALSA: hda/hdmi: Don't share an assigned converter on NVIDIA codecs

This commit implements the pin_cvt_fixup callback for generic NVIDIA HDMI/DP
codecs to move other connected pins off a newly assigned converter so idle
pins do not steal the active audio stream.

> diff --git a/sound/hda/codecs/hdmi/nvhdmi.c b/sound/hda/codecs/hdmi/nvhdmi.c
> index 94671ad24b5eb..95ae7730eed5b 100644
> --- a/sound/hda/codecs/hdmi/nvhdmi.c
> +++ b/sound/hda/codecs/hdmi/nvhdmi.c
> @@ -66,6 +66,81 @@ static const struct drm_audio_component_audio_ops nvhdmi_audio_ops = {
>  	.master_unbind = snd_hda_hdmi_acomp_master_unbind,
>  };
>  
> +/*
> + * All pins of these codecs share one connection list, as on Intel.  An idle
> + * pin left selecting the converter that another pin streams on takes that
> + * stream: the display that plays stays silent and the audio goes to the
> + * other display instead.  Seen on GB202 (0x10de00aa) with an HDMI TV and a
> + * DP monitor: whenever the monitor's PCM was opened first (PipeWire probes
> + * every PCM at start-up), its pin kept the converter the TV got next.
> + *
> + * Move every other connected pin off the converter just assigned, to a
> + * converter nobody uses.
> + */
> +static void nvhdmi_not_share_assigned_cvt(struct hda_codec *codec,
> +					  hda_nid_t pin_nid, int dev_id,
> +					  hda_nid_t cvt_nid)
> +{

[ ... ]

> +}
> +
> +static void nvhdmi_pin_cvt_fixup(struct hda_codec *codec,
> +				 struct hdmi_spec_per_pin *per_pin,
> +				 hda_nid_t cvt_nid)
> +{
> +	if (per_pin)
> +		nvhdmi_not_share_assigned_cvt(codec, per_pin->pin_nid,
> +					      per_pin->dev_id, per_pin->cvt_nid);
> +	else
> +		nvhdmi_not_share_assigned_cvt(codec, 0, 0, cvt_nid);
> +}

[Severity: Medium]
Should nvhdmi_pin_cvt_fixup() also select per_pin->dev_id and verify that
per_pin's own AC_VERB_GET_CONNECT_SEL matches per_pin->mux_idx when per_pin
is non-NULL?

In sound/hda/codecs/hdmi/hdmi.c, snd_hda_hdmi_generic_pcm_prepare() does not
write AC_VERB_SET_CONNECT_SEL directly; it relies on pin_cvt_fixup() to
verify and restore the active pin's converter selection (for example, after
S3 resume or a display reset where hardware resets pin converter selections
to the default converter):

sound/hda/codecs/hdmi/hdmi.c:snd_hda_hdmi_generic_pcm_prepare() {
	...
	/* Verify pin:cvt selections to avoid silent audio after S3.
	 * After S3, the audio driver restores pin:cvt selections
	 * but this can happen before gfx is ready and such selection
	 * is overlooked by HW. Thus multiple pins can share a same
	 * default converter and mute control will affect each other,
	 * which can cause a resumed audio playback become silent
	 * after S3.
	 */
	pin_cvt_fixup(codec, per_pin, 0);
	...
}

On Intel codecs, i915_pin_cvt_fixup() in sound/hda/codecs/hdmi/intelhdmi.c
selects per_pin->dev_id and calls intel_verify_pin_cvt_connect() to restore
per_pin->mux_idx before moving other pins off the converter:

sound/hda/codecs/hdmi/intelhdmi.c:i915_pin_cvt_fixup() {
	if (per_pin) {
		haswell_verify_D0(codec, per_pin->cvt_nid, per_pin->pin_nid);
		snd_hda_set_dev_select(codec, per_pin->pin_nid,
			       per_pin->dev_id);
		intel_verify_pin_cvt_connect(codec, per_pin);
		intel_not_share_assigned_cvt(codec, per_pin->pin_nid,
				     per_pin->dev_id, per_pin->mux_idx);
	} else {
		intel_not_share_assigned_cvt_nid(codec, 0, 0, cvt_nid);
	}
}

Because nvhdmi_pin_cvt_fixup() only calls nvhdmi_not_share_assigned_cvt() and
skips per_pin itself, if hardware resets pin connections while two PCMs are
open (say pin A assigned to converter 0x08 and pin B assigned to converter
0x09, with both reset in hardware to 0x08), preparing pin A first will cause
nvhdmi_not_share_assigned_cvt() to move pin B to an unassigned converter
(such as 0x0a, since 0x09 is marked assigned), and preparing pin B next will
never restore pin B's AC_VERB_SET_CONNECT_SEL back to per_pin->mux_idx for
converter 0x09.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102182213.2154560-1-razvandinculescu@gmail.com?part=1