Re: [PATCH 2/5] drm/nouveau/kms/nv50-: don't enable a DP output with no trained link

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/nouveau/kms/nv50-: nv50_audio_enable() called on untrained detached DP output
--

commit 9c72b9cc42ad5c2146710feb85347fdc8865d8f1
Author: David R. Hedges <linuxdev844@thelittleman.net>

drm/nouveau/kms/nv50-: don't enable a DP output with no trained link

This commit changes nouveau_dp_train() to return an error code and updates
nv50_sor_atomic_enable() to leave the SOR detached, mark the connector
link-status as bad, and schedule a hotplug event when no lane count and
link rate can carry the mode.

Link: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515

> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 8af01de822d52..7f55c016d8465 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1843,7 +1843,15 @@ nv50_sor_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_commit *st
>  		nvif_outp_lvds(&nv_encoder->outp, lvds_dual, lvds_8bpc);
>  		break;
>  	case DCB_OUTPUT_DP:
> -		nouveau_dp_train(nv_encoder, false, mode->clock, asyh->or.bpc);
> +		/* Attaching the SOR to a link that hasn't been trained can hang
> +		 * the display engine.  If the link can't carry the mode at all,
> +		 * which atomic check only rules out for a modeset, leave the
> +		 * output detached and tell userspace the link is bad.
> +		 */
> +		if (nouveau_dp_train(nv_encoder, false, mode->clock, asyh->or.bpc) == -EINVAL) {
> +			schedule_work(&nv_connector->link_bad_work);
> +			return;
> +		}

[Severity: Medium]
When nv50_sor_atomic_enable() returns early here on -EINVAL, does this still
end up enabling DP audio on the untrained and detached SOR?

Earlier in nv50_sor_atomic_enable(), nv_encoder->crtc is assigned and the SOR
is acquired with hda enabled when nv_connector->base.display_info.has_audio is
true:

drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_sor_atomic_enable() {
	...
	nv_connector = nv50_outp_get_new_connector(state, nv_encoder);
	nv_encoder->crtc = &nv_crtc->base;

	if ((disp->disp->object.oclass == GT214_DISP ||
	     disp->disp->object.oclass >= GF110_DISP) &&
	    nv_encoder->dcb->type != DCB_OUTPUT_LVDS &&
	    nv_connector->base.display_info.has_audio)
		hda = true;

	if (!nvif_outp_acquired(outp))
		nvif_outp_acquire_sor(outp, hda);
	...
}

In the caller nv50_disp_atomic_commit_tail(), outp->enabled is set to true
unconditionally after help->atomic_enable() returns:

drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_disp_atomic_commit_tail() {
	...
		if (outp->set.mask) {
			help->atomic_enable(encoder, state);
			outp->enabled = true;
			interlock[NV50_DISP_INTERLOCK_CORE] = 1;
		}
	...
}

Then in nv50_disp_atomic_commit_core(), the post-update loop sees
outp->enabled and calls nv50_audio_enable():

drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_disp_atomic_commit_core() {
	...
			if (outp->enabled) {
				nv50_audio_enable(outp->encoder, nouveau_crtc(nv_encoder->crtc),
						  nv_encoder->conn, NULL, NULL);
				outp->enabled = outp->disabled = false;
			}
	...
}

When a DP sink with audio in its EDID has link capabilities below what the
mode needs (or cleared after AUX stopped answering while HPD remained
asserted) and a commit turns the CRTC back on, nv50_audio_enable() will call
nvif_outp_hda_eld() on an output whose link was never trained, whose DP stream
was never configured, and whose SOR was left detached from the head.

On non-GSP GPUs (gt215_sor_dp_audio, gf119_sor_dp_audio, gv100_sor_dp_audio),
that triggers DP_AUDIO_CTRL on the unattached SOR and busy-polls for up to
2000ms waiting for the new-settings trigger bit to clear. On GSP-RM
(r535_sor_dp_audio, r535_sor_hda_eld), it issues
NV0073_CTRL_CMD_DFP_SET_AUDIO_ENABLE, NV0073_CTRL_CMD_DP_SET_AUDIO_MUTESTREAM,
and NV0073_CTRL_CMD_DFP_SET_ELD_AUDIO_CAPS (each wrapped in WARN_ON) on an
unconfigured DP stream and notifies ALSA that audio is active.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-nouveau-dp-relink-v1-0-7ee88c7f1e8a@thelittleman.net?part=2