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

[sashiko-bot@kernel.org]

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