[PATCH 0/5] drm/nouveau: recover DP links the sink drops after waking

[PATCH 0/5] drm/nouveau: recover DP links the sink drops after waking

[David R. Hedges via B4 Relay]

With nouveau on a GA102 (GSP-RM 570.144) driving an IGM L28TN4K at
3840x2160@60 over DP, the monitor shows "No input signal" after DPMS
off/on, after S3 resume and after a cable replug, and stays dark until
the cable has been replugged, often several times [1].  The
proprietary driver doesn't have the problem.

drm.debug shows the same sequence every time:

 - the wake modeset trains the link at 4 x HBR2 without error;
 - about 2s later the monitor raises IRQ_HPD, with its link status
   reporting all lanes locked and aligned and the sink in sync;
 - about 2s after that it drops HPD for under a second and reasserts
   it;
 - nouveau reports a disconnect and a reconnect, mutter sees the same
   monitor and mode and doesn't modeset, and nothing trains the link
   again.

Whenever the IRQ_HPD arrived before link training instead, the picture
came up.  The monitor apparently needs the link trained after it has
finished waking, and drops HPD to ask for that.  NVIDIA's DisplayPort
library retrains the link on every long HPD pulse from a connected SST
sink (notifyLongPulseInternal() calls assessLink()), which would
explain why the proprietary driver copes.

Getting there takes several changes:

 - 4 makes the IRQ_HPD link check work on nv50+ again.  It has done
   nothing since v6.7: the DP encoder it relies on is cached before the
   encoder is attached to the connector, so the cached pointer is
   always NULL.
 - Retraining an active link in place, which that link check then
   does, isn't safe on GSP-RM.  NVIDIA's library wraps it in SOR flush
   mode, which nouveau doesn't implement, and an in-place retrain here
   left artefacts and hung the display engine at the next modeset.  3
   makes GSP-RM restart a lost link with a full modeset instead, using
   drm_atomic_helper_reset_crtc(), as i915 does.  Without GSP-RM, links
   are still retrained in place.
 - 5 restarts the link the same way when a DP sink reasserts HPD while
   its CRTC is still active, since userspace won't modeset if nothing
   about the display has changed.  This is what brings the monitor
   back.  In case a sink drops HPD every time its link is trained,
   restarts stop after five in quick succession.
 - 1 and 2 deal with a sink whose capabilities have dropped below what
   the mode needs, or are unknown because it stopped answering AUX.
   Switched on with its power button, this monitor first advertises
   DPCD 1.1 and HBR only, and while switched off it keeps HPD asserted
   without answering AUX.  Enabling the output in either state hung the
   display engine.  1 rejects modesets the link can't carry.  2 handles
   a plain DPMS on, which mustn't fail atomic check, by leaving the SOR
   detached, setting link-status to BAD and sending a hotplug event.

1-3 have to go in no later than 4.  Nothing here is tagged for stable.
4 has Fixes: tags, as it fixes a regression, but it switches on code
that hasn't run since v6.7 (and, for the MST atomic check, ever), so
I'd rather it wasn't backported until it has been in a release.

This is based on drm-misc-next plus "drm/nouveau: GSP DP control call
fixes and two KMS fixes" [2], which is meant for drm-misc-fixes.  5
builds on the last patch there.

I'd welcome opinions on:

 - 5 restarts the link on every long HPD pulse while the CRTC is
   active, without consulting the link status, because here the link
   status looks fine throughout.  Is that acceptable for all sinks, or
   would you rather have it behind a quirk?
 - 2 leaves the head running with no SOR attached when there is
   nothing to train.  That avoided the hang here, with and without
   GSP-RM, but the head produces no vblanks in that state, so every
   commit waits out the 2s window timeout ("wndw-0: timeout") until
   the sink is back.  Is there a better way to park the head, and is
   this safe on every display generation?
 - mutter doesn't read link-status.  It recovers here because the probe
   drops the modes the link can't carry and it gets a hotplug event;
   with a sink whose mode list doesn't change, it would stay dark until
   the next hotplug.

Testing, on the GA102 above with Ubuntu's 7.3.0-9-generic kernel and
nouveau built from v7.3 plus this series and [2], with
drm.debug=0x106:

 - DPMS off/on, cable unplug/replug, and suspend/resume (S3): the
   monitor goes through the IRQ_HPD and HPD sequence above, and the
   picture comes back four to five seconds after the wake, replug or
   resume, once the link has been restarted;
 - power button off/on: the restart is rejected, as the monitor first
   advertises 4 x HBR only; link-status goes BAD, mutter switches to a
   lower mode, and about two seconds later it's back at 3840x2160@60;
 - monitor switched off with its power button, screen blanked and then
   woken while it was still off: the DPMS on succeeds with the output
   left detached and link-status BAD, nothing hangs, and the picture
   comes back as above once the monitor is switched on;
 - resolution and refresh rate changes before and after a recovery
   work;
 - none of this logged a warning, a display engine hang or a failed
   link restart, and the restart limit was never reached.

With GSP-RM disabled (nouveau.config=NvGspRm=0), which runs the GA102's
display through nvkm's own code, everything above apart from suspend
behaves the same.  Suspend hangs that way on this machine whether or
not this series is applied (screen off, the system still running but
unresponsive; tried with the series on 7.3.0-9 and with Ubuntu's
nouveau on 7.3.0-10), so it wasn't tested without GSP-RM.

Both series build with W=1 on drm-misc-next, with no new W=1 or sparse
warnings in the touched files.  checkpatch --strict is clean.

Not tested: GPUs that predate GSP-RM, although running the GA102
without GSP-RM covers the same display code, except for the in-place
retrain on link loss, which this monitor never triggered; MST hubs (4
enables drm_dp_mst_root_conn_atomic_check() for MST-capable ports);
and eDP.

This series was developed with the help of Claude Code (Claude Opus
5.5).  It analysed the kernel logs, read the nouveau, DRM and NVIDIA
open-gpu-kernel-modules sources, drafted the patches and changelogs,
and ran the build and checkpatch checks.  I provided evidence and
direction, reviewed changes, and ran the hardware tests described
here.

[1] https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515
[2] https://lore.kernel.org/nouveau/20261103-nouveau-dp-fixes-v1-0-892f4bd357cd@thelittleman.net/T/

Signed-off-by: David R. Hedges <linuxdev844@thelittleman.net>
---
David R. Hedges (5):
      drm/nouveau/kms/nv50-: reject DP modesets the link can't carry
      drm/nouveau/kms/nv50-: don't enable a DP output with no trained link
      drm/nouveau/dp: restart lost links with a modeset on GSP-RM
      drm/nouveau/kms: cache the DP encoder after it has been attached
      drm/nouveau/dp: restart an active link when the sink reasserts HPD

 drivers/gpu/drm/nouveau/dispnv50/disp.c     |  34 +++++++--
 drivers/gpu/drm/nouveau/nouveau_connector.c |  20 +++++-
 drivers/gpu/drm/nouveau/nouveau_connector.h |   6 ++
 drivers/gpu/drm/nouveau/nouveau_display.c   | 108 +++++++++++++++++++++++++++-
 drivers/gpu/drm/nouveau/nouveau_dp.c        |  34 +++++++--
 drivers/gpu/drm/nouveau/nouveau_encoder.h   |   4 +-
 6 files changed, 188 insertions(+), 18 deletions(-)
---
base-commit: 2ebbeea89df4dae6a7d8a2a9b2d698d893beb283
change-id: 20261103-nouveau-dp-relink-89c8d8f644a1
prerequisite-message-id: <20261103-nouveau-dp-fixes-v1-0-892f4bd357cd@thelittleman.net>
prerequisite-patch-id: 2979d80699fb4b68d7465019ec928fd7bd47dac9
prerequisite-patch-id: 1389b4709a500702ced0d91bec6f9ec74d42f0c7
prerequisite-patch-id: 4b7b0a68f43fec590886ef8bc4588a8cd472cdf9
prerequisite-patch-id: 97b5695d71e8732b28be69331687ab21691173c0
prerequisite-patch-id: ca7799aa8843800d1d9d1649ca75cc04e6c3266a

Best regards,
--  
David R. Hedges <linuxdev844@thelittleman.net>

[PATCH 0/5] drm/nouveau: recover DP links the sink drops after waking

[David R. Hedges]

With nouveau on a GA102 (GSP-RM 570.144) driving an IGM L28TN4K at
3840x2160@60 over DP, the monitor shows "No input signal" after DPMS
off/on, after S3 resume and after a cable replug, and stays dark until
the cable has been replugged, often several times [1].  The
proprietary driver doesn't have the problem.

drm.debug shows the same sequence every time:

 - the wake modeset trains the link at 4 x HBR2 without error;
 - about 2s later the monitor raises IRQ_HPD, with its link status
   reporting all lanes locked and aligned and the sink in sync;
 - about 2s after that it drops HPD for under a second and reasserts
   it;
 - nouveau reports a disconnect and a reconnect, mutter sees the same
   monitor and mode and doesn't modeset, and nothing trains the link
   again.

Whenever the IRQ_HPD arrived before link training instead, the picture
came up.  The monitor apparently needs the link trained after it has
finished waking, and drops HPD to ask for that.  NVIDIA's DisplayPort
library retrains the link on every long HPD pulse from a connected SST
sink (notifyLongPulseInternal() calls assessLink()), which would
explain why the proprietary driver copes.

Getting there takes several changes:

 - 4 makes the IRQ_HPD link check work on nv50+ again.  It has done
   nothing since v6.7: the DP encoder it relies on is cached before the
   encoder is attached to the connector, so the cached pointer is
   always NULL.
 - Retraining an active link in place, which that link check then
   does, isn't safe on GSP-RM.  NVIDIA's library wraps it in SOR flush
   mode, which nouveau doesn't implement, and an in-place retrain here
   left artefacts and hung the display engine at the next modeset.  3
   makes GSP-RM restart a lost link with a full modeset instead, using
   drm_atomic_helper_reset_crtc(), as i915 does.  Without GSP-RM, links
   are still retrained in place.
 - 5 restarts the link the same way when a DP sink reasserts HPD while
   its CRTC is still active, since userspace won't modeset if nothing
   about the display has changed.  This is what brings the monitor
   back.  In case a sink drops HPD every time its link is trained,
   restarts stop after five in quick succession.
 - 1 and 2 deal with a sink whose capabilities have dropped below what
   the mode needs, or are unknown because it stopped answering AUX.
   Switched on with its power button, this monitor first advertises
   DPCD 1.1 and HBR only, and while switched off it keeps HPD asserted
   without answering AUX.  Enabling the output in either state hung the
   display engine.  1 rejects modesets the link can't carry.  2 handles
   a plain DPMS on, which mustn't fail atomic check, by leaving the SOR
   detached, setting link-status to BAD and sending a hotplug event.

1-3 have to go in no later than 4.  Nothing here is tagged for stable.
4 has Fixes: tags, as it fixes a regression, but it switches on code
that hasn't run since v6.7 (and, for the MST atomic check, ever), so
I'd rather it wasn't backported until it has been in a release.

This is based on drm-misc-next plus "drm/nouveau: GSP DP control call
fixes and two KMS fixes" [2], which is meant for drm-misc-fixes.  5
builds on the last patch there.

I'd welcome opinions on:

 - 5 restarts the link on every long HPD pulse while the CRTC is
   active, without consulting the link status, because here the link
   status looks fine throughout.  Is that acceptable for all sinks, or
   would you rather have it behind a quirk?
 - 2 leaves the head running with no SOR attached when there is
   nothing to train.  That avoided the hang here, with and without
   GSP-RM, but the head produces no vblanks in that state, so every
   commit waits out the 2s window timeout ("wndw-0: timeout") until
   the sink is back.  Is there a better way to park the head, and is
   this safe on every display generation?
 - mutter doesn't read link-status.  It recovers here because the probe
   drops the modes the link can't carry and it gets a hotplug event;
   with a sink whose mode list doesn't change, it would stay dark until
   the next hotplug.

Testing, on the GA102 above with Ubuntu's 7.3.0-9-generic kernel and
nouveau built from v7.3 plus this series and [2], with
drm.debug=0x106:

 - DPMS off/on, cable unplug/replug, and suspend/resume (S3): the
   monitor goes through the IRQ_HPD and HPD sequence above, and the
   picture comes back four to five seconds after the wake, replug or
   resume, once the link has been restarted;
 - power button off/on: the restart is rejected, as the monitor first
   advertises 4 x HBR only; link-status goes BAD, mutter switches to a
   lower mode, and about two seconds later it's back at 3840x2160@60;
 - monitor switched off with its power button, screen blanked and then
   woken while it was still off: the DPMS on succeeds with the output
   left detached and link-status BAD, nothing hangs, and the picture
   comes back as above once the monitor is switched on;
 - resolution and refresh rate changes before and after a recovery
   work;
 - none of this logged a warning, a display engine hang or a failed
   link restart, and the restart limit was never reached.

With GSP-RM disabled (nouveau.config=NvGspRm=0), which runs the GA102's
display through nvkm's own code, everything above apart from suspend
behaves the same.  Suspend hangs that way on this machine whether or
not this series is applied (screen off, the system still running but
unresponsive; tried with the series on 7.3.0-9 and with Ubuntu's
nouveau on 7.3.0-10), so it wasn't tested without GSP-RM.

Both series build with W=1 on drm-misc-next, with no new W=1 or sparse
warnings in the touched files.  checkpatch --strict is clean.

Not tested: GPUs that predate GSP-RM, although running the GA102
without GSP-RM covers the same display code, except for the in-place
retrain on link loss, which this monitor never triggered; MST hubs (4
enables drm_dp_mst_root_conn_atomic_check() for MST-capable ports);
and eDP.

This series was developed with the help of Claude Code (Claude Opus
5.5).  It analysed the kernel logs, read the nouveau, DRM and NVIDIA
open-gpu-kernel-modules sources, drafted the patches and changelogs,
and ran the build and checkpatch checks.  I provided evidence and
direction, reviewed changes, and ran the hardware tests described
here.

[1] https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515
[2] https://lore.kernel.org/nouveau/20261103-nouveau-dp-fixes-v1-0-892f4bd357cd@thelittleman.net/T/

Signed-off-by: David R. Hedges <linuxdev844@thelittleman.net>
---
David R. Hedges (5):
      drm/nouveau/kms/nv50-: reject DP modesets the link can't carry
      drm/nouveau/kms/nv50-: don't enable a DP output with no trained link
      drm/nouveau/dp: restart lost links with a modeset on GSP-RM
      drm/nouveau/kms: cache the DP encoder after it has been attached
      drm/nouveau/dp: restart an active link when the sink reasserts HPD

 drivers/gpu/drm/nouveau/dispnv50/disp.c     |  34 +++++++--
 drivers/gpu/drm/nouveau/nouveau_connector.c |  20 +++++-
 drivers/gpu/drm/nouveau/nouveau_connector.h |   6 ++
 drivers/gpu/drm/nouveau/nouveau_display.c   | 108 +++++++++++++++++++++++++++-
 drivers/gpu/drm/nouveau/nouveau_dp.c        |  34 +++++++--
 drivers/gpu/drm/nouveau/nouveau_encoder.h   |   4 +-
 6 files changed, 188 insertions(+), 18 deletions(-)
---
base-commit: 2ebbeea89df4dae6a7d8a2a9b2d698d893beb283
change-id: 20261103-nouveau-dp-relink-89c8d8f644a1
prerequisite-message-id: <20261103-nouveau-dp-fixes-v1-0-892f4bd357cd@thelittleman.net>
prerequisite-patch-id: 2979d80699fb4b68d7465019ec928fd7bd47dac9
prerequisite-patch-id: 1389b4709a500702ced0d91bec6f9ec74d42f0c7
prerequisite-patch-id: 4b7b0a68f43fec590886ef8bc4588a8cd472cdf9
prerequisite-patch-id: 97b5695d71e8732b28be69331687ab21691173c0
prerequisite-patch-id: ca7799aa8843800d1d9d1649ca75cc04e6c3266a

Best regards,
--  
David R. Hedges <linuxdev844@thelittleman.net>

[PATCH 1/5] drm/nouveau/kms/nv50-: reject DP modesets the link can't carry

[David R. Hedges via B4 Relay]

From: "David R. Hedges" <linuxdev844@thelittleman.net>

nv50_outp_atomic_fix_depth() lowers the bpc of a DP mode until it fits
the link's capabilities, but carries on at 6 bpc if it still doesn't.
nouveau_dp_train() then finds no lane count and link rate fast enough,
trains nothing, and the output is enabled anyway.

Modes are checked against the link when they're probed, so this takes
a sink whose capabilities have dropped since, or that couldn't be
probed at all.  An IGM L28TN4K on a GA102 (GSP-RM 570.144) manages
both: for a moment after being switched on with its power button it
advertises DPCD 1.1 and HBR rather than DPCD 1.2 and HBR2, and the
3840x2160@60 mode it had been running needs 1336500 kB/s even at
6 bpc, against 1080000 kB/s for 4 x HBR; and while it's switched off
it keeps HPD asserted but doesn't answer AUX, which leaves its link
capabilities at zero.  Enabling the output in either state ended in
"core notifier timeout", after which GSP stopped answering RPCs.

Fail the modeset in that case, so that userspace can pick a mode that
fits.  A commit that only turns the CRTC back on mustn't fail (see
struct drm_crtc_state), so that case is left to the commit path, which
the next patch deals with.

Link: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515
Signed-off-by: David R. Hedges <linuxdev844@thelittleman.net>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 7daecaecccac..8af01de822d5 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -373,7 +373,8 @@ nv50_outp_atomic_check_view(struct drm_encoder *encoder,
 	return 0;
 }
 
-static void
+/* Returns false if the link can't carry the mode even at the lowest bpc. */
+static bool
 nv50_outp_atomic_fix_depth(struct drm_encoder *encoder, struct drm_crtc_state *crtc_state)
 {
 	struct nv50_head_atom *asyh = nv50_head_atom(crtc_state);
@@ -389,17 +390,26 @@ nv50_outp_atomic_fix_depth(struct drm_encoder *encoder, struct drm_crtc_state *c
 		asyh->or.bpc = min_t(u8, asyh->or.bpc, 10);
 
 		/* reduce the bpc until it works out */
-		while (asyh->or.bpc > 6) {
+		for (;;) {
 			mode_rate = DIV_ROUND_UP(mode->clock * asyh->or.bpc * 3, 8);
 			if (mode_rate <= max_rate)
 				break;
 
+			if (asyh->or.bpc <= 6) {
+				drm_dbg_kms(encoder->dev,
+					    "[ENCODER:%d:%s] mode needs %u kB/s, link carries %u kB/s\n",
+					    encoder->base.id, encoder->name, mode_rate, max_rate);
+				return false;
+			}
+
 			asyh->or.bpc -= 2;
 		}
 		break;
 	default:
 		break;
 	}
+
+	return true;
 }
 
 static int
@@ -423,8 +433,13 @@ nv50_outp_atomic_check(struct drm_encoder *encoder,
 	if (crtc_state->mode_changed || crtc_state->connectors_changed)
 		asyh->or.bpc = connector->display_info.bpc ?: 8;
 
-	/* We might have to reduce the bpc */
-	nv50_outp_atomic_fix_depth(encoder, crtc_state);
+	/* We might have to reduce the bpc.  If the link can't carry the mode
+	 * at all, fail a modeset, but not a plain DPMS on, which userspace
+	 * expects to always succeed.
+	 */
+	if (!nv50_outp_atomic_fix_depth(encoder, crtc_state) &&
+	    (crtc_state->mode_changed || crtc_state->connectors_changed))
+		return -EINVAL;
 
 	return 0;
 }

-- 
2.55.0

[PATCH 1/5] drm/nouveau/kms/nv50-: reject DP modesets the link can't carry

[David R. Hedges]

nv50_outp_atomic_fix_depth() lowers the bpc of a DP mode until it fits
the link's capabilities, but carries on at 6 bpc if it still doesn't.
nouveau_dp_train() then finds no lane count and link rate fast enough,
trains nothing, and the output is enabled anyway.

Modes are checked against the link when they're probed, so this takes
a sink whose capabilities have dropped since, or that couldn't be
probed at all.  An IGM L28TN4K on a GA102 (GSP-RM 570.144) manages
both: for a moment after being switched on with its power button it
advertises DPCD 1.1 and HBR rather than DPCD 1.2 and HBR2, and the
3840x2160@60 mode it had been running needs 1336500 kB/s even at
6 bpc, against 1080000 kB/s for 4 x HBR; and while it's switched off
it keeps HPD asserted but doesn't answer AUX, which leaves its link
capabilities at zero.  Enabling the output in either state ended in
"core notifier timeout", after which GSP stopped answering RPCs.

Fail the modeset in that case, so that userspace can pick a mode that
fits.  A commit that only turns the CRTC back on mustn't fail (see
struct drm_crtc_state), so that case is left to the commit path, which
the next patch deals with.

Link: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515
Signed-off-by: David R. Hedges <linuxdev844@thelittleman.net>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 7daecaecccac..8af01de822d5 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -373,7 +373,8 @@ nv50_outp_atomic_check_view(struct drm_encoder *encoder,
 	return 0;
 }
 
-static void
+/* Returns false if the link can't carry the mode even at the lowest bpc. */
+static bool
 nv50_outp_atomic_fix_depth(struct drm_encoder *encoder, struct drm_crtc_state *crtc_state)
 {
 	struct nv50_head_atom *asyh = nv50_head_atom(crtc_state);
@@ -389,17 +390,26 @@ nv50_outp_atomic_fix_depth(struct drm_encoder *encoder, struct drm_crtc_state *c
 		asyh->or.bpc = min_t(u8, asyh->or.bpc, 10);
 
 		/* reduce the bpc until it works out */
-		while (asyh->or.bpc > 6) {
+		for (;;) {
 			mode_rate = DIV_ROUND_UP(mode->clock * asyh->or.bpc * 3, 8);
 			if (mode_rate <= max_rate)
 				break;
 
+			if (asyh->or.bpc <= 6) {
+				drm_dbg_kms(encoder->dev,
+					    "[ENCODER:%d:%s] mode needs %u kB/s, link carries %u kB/s\n",
+					    encoder->base.id, encoder->name, mode_rate, max_rate);
+				return false;
+			}
+
 			asyh->or.bpc -= 2;
 		}
 		break;
 	default:
 		break;
 	}
+
+	return true;
 }
 
 static int
@@ -423,8 +433,13 @@ nv50_outp_atomic_check(struct drm_encoder *encoder,
 	if (crtc_state->mode_changed || crtc_state->connectors_changed)
 		asyh->or.bpc = connector->display_info.bpc ?: 8;
 
-	/* We might have to reduce the bpc */
-	nv50_outp_atomic_fix_depth(encoder, crtc_state);
+	/* We might have to reduce the bpc.  If the link can't carry the mode
+	 * at all, fail a modeset, but not a plain DPMS on, which userspace
+	 * expects to always succeed.
+	 */
+	if (!nv50_outp_atomic_fix_depth(encoder, crtc_state) &&
+	    (crtc_state->mode_changed || crtc_state->connectors_changed))
+		return -EINVAL;
 
 	return 0;
 }

-- 
2.55.0

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

[David R. Hedges via B4 Relay]

From: "David R. Hedges" <linuxdev844@thelittleman.net>

A commit that only turns a CRTC back on mustn't fail atomic check, so
it still reaches nv50_sor_atomic_enable() when the sink's link
capabilities have dropped below what the mode needs, or are unknown
because the sink stopped answering AUX.  nouveau_dp_train() then has
nothing to train, and the SOR is attached to the head regardless.  On
a GA102 with GSP-RM 570.144, mutter turning the screen back on while
the monitor was switched off (HPD still asserted, AUX not answering)
did exactly that, and the display engine hung: "core notifier
timeout", after which GSP stopped answering RPCs.

Have nouveau_dp_train() say why it failed.  When no lane count and
link rate can carry the mode, leave the SOR detached, set the
connector's link-status to BAD and send a hotplug event, as the
link-status property's documentation describes.  Userspace that
handles link-status can pick a new mode; userspace that doesn't still
gets the hotplug event and reprobes.  When training is attempted and
fails, the output is enabled as before; only the case where there was
nothing to train changes.

The head produces no vblanks while the SOR is detached, so until the
sink is back and a modeset restarts the link, each commit to it waits
out the window channel's 2s timeout ("wndw-0: timeout").  On the
GA102 above that was the only side effect, with and without GSP-RM.

Link: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515
Signed-off-by: David R. Hedges <linuxdev844@thelittleman.net>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c     | 10 +++++++++-
 drivers/gpu/drm/nouveau/nouveau_connector.c | 18 ++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_connector.h |  1 +
 drivers/gpu/drm/nouveau/nouveau_dp.c        | 13 +++++++++----
 drivers/gpu/drm/nouveau/nouveau_encoder.h   |  2 +-
 5 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 8af01de822d5..7f55c016d846 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;
+		}
 		nv50_sor_dp_watermark_sst(nv_encoder, head, asyh);
 		depth = nv50_dp_bpc_to_depth(asyh->or.bpc);
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index cc3caf8de64b..cae4e35c30aa 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -398,6 +398,7 @@ nouveau_connector_destroy(struct drm_connector *connector)
 	nvif_event_dtor(&nv_connector->irq);
 	nvif_event_dtor(&nv_connector->hpd);
 	cancel_work_sync(&nv_connector->irq_work);
+	cancel_work_sync(&nv_connector->link_bad_work);
 	drm_edid_free(nv_connector->drm_edid);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
@@ -1261,6 +1262,22 @@ nouveau_connector_funcs_lvds = {
 	.early_unregister = nouveau_connector_early_unregister,
 };
 
+/* The sink isn't getting a picture with the current mode, and userspace may
+ * need to pick another one to get it back.
+ */
+static void
+nouveau_connector_link_bad(struct work_struct *work)
+{
+	struct nouveau_connector *nv_connector =
+		container_of(work, typeof(*nv_connector), link_bad_work);
+	struct drm_connector *connector = &nv_connector->base;
+
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] link-status bad\n",
+		    connector->base.id, connector->name);
+	drm_connector_set_link_status_property(connector, DRM_MODE_LINK_STATUS_BAD);
+	drm_kms_helper_connector_hotplug_event(connector);
+}
+
 void
 nouveau_connector_hpd(struct nouveau_connector *nv_connector, u64 bits)
 {
@@ -1383,6 +1400,7 @@ nouveau_connector_create(struct drm_device *dev, int index)
 	connector = &nv_connector->base;
 	nv_connector->index = index;
 	INIT_WORK(&nv_connector->irq_work, nouveau_dp_irq);
+	INIT_WORK(&nv_connector->link_bad_work, nouveau_connector_link_bad);
 
 	if (disp->disp.conn_mask & BIT(nv_connector->index)) {
 		ret = nvif_conn_ctor(&disp->disp, nv_connector->base.name, nv_connector->index,
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.h b/drivers/gpu/drm/nouveau/nouveau_connector.h
index eb292d2ba4bc..3109278d36ea 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.h
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.h
@@ -134,6 +134,7 @@ struct nouveau_connector {
 	struct nvif_event hpd;
 	struct nvif_event irq;
 	struct work_struct irq_work;
+	struct work_struct link_bad_work;
 
 	struct drm_dp_aux aux;
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
index 019af307c669..9bc24f3914bd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dp.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
@@ -401,14 +401,17 @@ nouveau_dp_train_link(struct nouveau_encoder *outp, bool retrain)
 	return ret == 0;
 }
 
-bool
+/* Returns -EINVAL if no lane count and link rate can carry the mode, in which
+ * case nothing has been trained, or -EIO if training failed.
+ */
+int
 nouveau_dp_train(struct nouveau_encoder *outp, bool mst, u32 khz, u8 bpc)
 {
 	struct nouveau_drm *drm = nouveau_drm(outp->base.base.dev);
 	struct drm_dp_aux *aux = &outp->conn->aux;
 	u32 min_rate;
 	u8 pwr;
-	bool ret = true;
+	int ret = -EINVAL;
 
 	if (mst)
 		min_rate = outp->dp.link_nr * outp->dp.rate[0].rate;
@@ -434,13 +437,15 @@ nouveau_dp_train(struct nouveau_encoder *outp, bool mst, u32 khz, u8 bpc)
 				outp->dp.lt.nr = nr;
 				outp->dp.lt.bw = outp->dp.rate[rate].rate;
 				outp->dp.lt.mst = mst;
-				if (nouveau_dp_train_link(outp, false))
+				if (nouveau_dp_train_link(outp, false)) {
+					ret = 0;
 					goto done;
+				}
+				ret = -EIO;
 			}
 		}
 	}
 
-	ret = false;
 done:
 	mutex_unlock(&outp->dp.hpd_irq_lock);
 	return ret;
diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h
index 4422c6185d49..9366678e7d05 100644
--- a/drivers/gpu/drm/nouveau/nouveau_encoder.h
+++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h
@@ -156,7 +156,7 @@ enum nouveau_dp_status {
 };
 
 int nouveau_dp_detect(struct nouveau_connector *, struct nouveau_encoder *);
-bool nouveau_dp_train(struct nouveau_encoder *, bool mst, u32 khz, u8 bpc);
+int nouveau_dp_train(struct nouveau_encoder *outp, bool mst, u32 khz, u8 bpc);
 void nouveau_dp_power_down(struct nouveau_encoder *);
 bool nouveau_dp_link_check(struct nouveau_connector *);
 void nouveau_dp_irq(struct work_struct *);

-- 
2.55.0

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

[David R. Hedges]

A commit that only turns a CRTC back on mustn't fail atomic check, so
it still reaches nv50_sor_atomic_enable() when the sink's link
capabilities have dropped below what the mode needs, or are unknown
because the sink stopped answering AUX.  nouveau_dp_train() then has
nothing to train, and the SOR is attached to the head regardless.  On
a GA102 with GSP-RM 570.144, mutter turning the screen back on while
the monitor was switched off (HPD still asserted, AUX not answering)
did exactly that, and the display engine hung: "core notifier
timeout", after which GSP stopped answering RPCs.

Have nouveau_dp_train() say why it failed.  When no lane count and
link rate can carry the mode, leave the SOR detached, set the
connector's link-status to BAD and send a hotplug event, as the
link-status property's documentation describes.  Userspace that
handles link-status can pick a new mode; userspace that doesn't still
gets the hotplug event and reprobes.  When training is attempted and
fails, the output is enabled as before; only the case where there was
nothing to train changes.

The head produces no vblanks while the SOR is detached, so until the
sink is back and a modeset restarts the link, each commit to it waits
out the window channel's 2s timeout ("wndw-0: timeout").  On the
GA102 above that was the only side effect, with and without GSP-RM.

Link: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515
Signed-off-by: David R. Hedges <linuxdev844@thelittleman.net>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c     | 10 +++++++++-
 drivers/gpu/drm/nouveau/nouveau_connector.c | 18 ++++++++++++++++++
 drivers/gpu/drm/nouveau/nouveau_connector.h |  1 +
 drivers/gpu/drm/nouveau/nouveau_dp.c        | 13 +++++++++----
 drivers/gpu/drm/nouveau/nouveau_encoder.h   |  2 +-
 5 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 8af01de822d5..7f55c016d846 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;
+		}
 		nv50_sor_dp_watermark_sst(nv_encoder, head, asyh);
 		depth = nv50_dp_bpc_to_depth(asyh->or.bpc);
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index cc3caf8de64b..cae4e35c30aa 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -398,6 +398,7 @@ nouveau_connector_destroy(struct drm_connector *connector)
 	nvif_event_dtor(&nv_connector->irq);
 	nvif_event_dtor(&nv_connector->hpd);
 	cancel_work_sync(&nv_connector->irq_work);
+	cancel_work_sync(&nv_connector->link_bad_work);
 	drm_edid_free(nv_connector->drm_edid);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
@@ -1261,6 +1262,22 @@ nouveau_connector_funcs_lvds = {
 	.early_unregister = nouveau_connector_early_unregister,
 };
 
+/* The sink isn't getting a picture with the current mode, and userspace may
+ * need to pick another one to get it back.
+ */
+static void
+nouveau_connector_link_bad(struct work_struct *work)
+{
+	struct nouveau_connector *nv_connector =
+		container_of(work, typeof(*nv_connector), link_bad_work);
+	struct drm_connector *connector = &nv_connector->base;
+
+	drm_dbg_kms(connector->dev, "[CONNECTOR:%d:%s] link-status bad\n",
+		    connector->base.id, connector->name);
+	drm_connector_set_link_status_property(connector, DRM_MODE_LINK_STATUS_BAD);
+	drm_kms_helper_connector_hotplug_event(connector);
+}
+
 void
 nouveau_connector_hpd(struct nouveau_connector *nv_connector, u64 bits)
 {
@@ -1383,6 +1400,7 @@ nouveau_connector_create(struct drm_device *dev, int index)
 	connector = &nv_connector->base;
 	nv_connector->index = index;
 	INIT_WORK(&nv_connector->irq_work, nouveau_dp_irq);
+	INIT_WORK(&nv_connector->link_bad_work, nouveau_connector_link_bad);
 
 	if (disp->disp.conn_mask & BIT(nv_connector->index)) {
 		ret = nvif_conn_ctor(&disp->disp, nv_connector->base.name, nv_connector->index,
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.h b/drivers/gpu/drm/nouveau/nouveau_connector.h
index eb292d2ba4bc..3109278d36ea 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.h
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.h
@@ -134,6 +134,7 @@ struct nouveau_connector {
 	struct nvif_event hpd;
 	struct nvif_event irq;
 	struct work_struct irq_work;
+	struct work_struct link_bad_work;
 
 	struct drm_dp_aux aux;
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
index 019af307c669..9bc24f3914bd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dp.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
@@ -401,14 +401,17 @@ nouveau_dp_train_link(struct nouveau_encoder *outp, bool retrain)
 	return ret == 0;
 }
 
-bool
+/* Returns -EINVAL if no lane count and link rate can carry the mode, in which
+ * case nothing has been trained, or -EIO if training failed.
+ */
+int
 nouveau_dp_train(struct nouveau_encoder *outp, bool mst, u32 khz, u8 bpc)
 {
 	struct nouveau_drm *drm = nouveau_drm(outp->base.base.dev);
 	struct drm_dp_aux *aux = &outp->conn->aux;
 	u32 min_rate;
 	u8 pwr;
-	bool ret = true;
+	int ret = -EINVAL;
 
 	if (mst)
 		min_rate = outp->dp.link_nr * outp->dp.rate[0].rate;
@@ -434,13 +437,15 @@ nouveau_dp_train(struct nouveau_encoder *outp, bool mst, u32 khz, u8 bpc)
 				outp->dp.lt.nr = nr;
 				outp->dp.lt.bw = outp->dp.rate[rate].rate;
 				outp->dp.lt.mst = mst;
-				if (nouveau_dp_train_link(outp, false))
+				if (nouveau_dp_train_link(outp, false)) {
+					ret = 0;
 					goto done;
+				}
+				ret = -EIO;
 			}
 		}
 	}
 
-	ret = false;
 done:
 	mutex_unlock(&outp->dp.hpd_irq_lock);
 	return ret;
diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h
index 4422c6185d49..9366678e7d05 100644
--- a/drivers/gpu/drm/nouveau/nouveau_encoder.h
+++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h
@@ -156,7 +156,7 @@ enum nouveau_dp_status {
 };
 
 int nouveau_dp_detect(struct nouveau_connector *, struct nouveau_encoder *);
-bool nouveau_dp_train(struct nouveau_encoder *, bool mst, u32 khz, u8 bpc);
+int nouveau_dp_train(struct nouveau_encoder *outp, bool mst, u32 khz, u8 bpc);
 void nouveau_dp_power_down(struct nouveau_encoder *);
 bool nouveau_dp_link_check(struct nouveau_connector *);
 void nouveau_dp_irq(struct work_struct *);

-- 
2.55.0

[PATCH 3/5] drm/nouveau/dp: restart lost links with a modeset on GSP-RM

[David R. Hedges via B4 Relay]

From: "David R. Hedges" <linuxdev844@thelittleman.net>

When a sink reports through IRQ_HPD that its link has lost lock,
nouveau_dp_link_check() retrains the link in place while the stream
keeps running.  Without GSP-RM, nvkm_dp_train() has a path for that
which leaves the source configuration alone.  On GSP-RM,
r535_dp_train() ignores its retrain argument and issues the same
NV0073_CTRL_CMD_DP_CTRL as for a new link.  NVIDIA's own DisplayPort
library never retrains an active link like that: it puts the SOR into
flush mode, retrains, configures the stream again and takes the SOR
out of flush mode, and warns that skipping part of that sequence can
hang the display engine.

That holds on a GA102 with GSP-RM 570.144.  After an experimental
patch retrained an active 4 x HBR2 link in place, the picture came
back with horizontal artefacts, and the next modeset, a refresh rate
change, never completed: "core notifier timeout", after which GSP
stopped answering RPCs.

nouveau doesn't implement flush mode, so on GSP-RM restart the pipe
with a full modeset of the CRTC driving the connector instead, using
drm_atomic_helper_reset_crtc().  That trains the link, configures the
stream and attaches the SOR in the same order as any other modeset,
which is also how i915 retrains a link.  The HPD worker does this after
dropping mode_config.mutex, so that probing isn't held up while it
runs.  If the sink now advertises less than the mode needs, the
modeset fails atomic check, and the connector's link-status is set to
BAD, as when a DPMS on finds the same.

The link check is only reached once the connector's DP encoder has
been cached, which on nv50+ it currently never is, so this has no
effect until the next patch.

Link: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515
Signed-off-by: David R. Hedges <linuxdev844@thelittleman.net>
---
 drivers/gpu/drm/nouveau/nouveau_display.c | 65 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/nouveau/nouveau_dp.c      | 21 ++++++++--
 drivers/gpu/drm/nouveau/nouveau_encoder.h |  2 +-
 3 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 4398cf0fc541..c5e914602baf 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -418,6 +418,53 @@ nouveau_display_hpd_resume(struct nouveau_drm *drm)
 	schedule_work(&drm->hpd_work);
 }
 
+/* Restart the pipe driving @connector with a full modeset, so that its link is
+ * trained again along with everything else.
+ */
+static void
+nouveau_display_relink(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_crtc *crtc;
+	int ret;
+
+	drm_modeset_acquire_init(&ctx, 0);
+retry:
+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
+	if (ret)
+		goto out;
+
+	crtc = connector->state->crtc;
+	if (!crtc)
+		goto out;
+
+	ret = drm_modeset_lock(&crtc->mutex, &ctx);
+	if (ret)
+		goto out;
+
+	if (crtc->state->active) {
+		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] restarting link\n",
+			    connector->base.id, connector->name);
+		ret = drm_atomic_helper_reset_crtc(crtc, &ctx);
+	}
+out:
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	/* The sink may now advertise less than the mode needs. */
+	if (ret == -EINVAL)
+		schedule_work(&nouveau_connector(connector)->link_bad_work);
+	else if (ret)
+		NV_ERROR(nouveau_drm(dev), "%s: failed to restart link: %d\n",
+			 connector->name, ret);
+}
+
 static void
 nouveau_display_hpd_work(struct work_struct *work)
 {
@@ -425,7 +472,7 @@ nouveau_display_hpd_work(struct work_struct *work)
 	struct drm_device *dev = drm->dev;
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
-	u32 pending;
+	u32 pending, relink = 0;
 	int changed = 0;
 	struct drm_connector *first_changed_connector = NULL;
 
@@ -447,6 +494,7 @@ nouveau_display_hpd_work(struct work_struct *work)
 		struct nouveau_connector *nv_connector = nouveau_connector(connector);
 		enum drm_connector_status old_status = connector->status;
 		u64 bits, old_epoch_counter = connector->epoch_counter;
+		bool modeset = false;
 
 		if (!(pending & drm_connector_mask(connector)))
 			continue;
@@ -468,11 +516,14 @@ nouveau_display_hpd_work(struct work_struct *work)
 		 */
 		if ((bits & NVIF_CONN_EVENT_V0_IRQ) &&
 		    !(bits & (NVIF_CONN_EVENT_V0_PLUG | NVIF_CONN_EVENT_V0_UNPLUG))) {
-			if (nouveau_dp_link_check(nv_connector))
+			if (nouveau_dp_link_check(nv_connector, &modeset))
 				continue;
 		}
 
 		connector->status = drm_helper_probe_detect(connector, NULL, false);
+		if (modeset && connector->status == connector_status_connected)
+			relink |= drm_connector_mask(connector);
+
 		if (old_epoch_counter == connector->epoch_counter)
 			continue;
 
@@ -492,6 +543,16 @@ nouveau_display_hpd_work(struct work_struct *work)
 	drm_connector_list_iter_end(&conn_iter);
 	mutex_unlock(&dev->mode_config.mutex);
 
+	/* A restart is a full modeset, so don't hold up probing while it runs. */
+	if (relink) {
+		drm_connector_list_iter_begin(dev, &conn_iter);
+		nouveau_for_each_non_mst_connector_iter(connector, &conn_iter) {
+			if (relink & drm_connector_mask(connector))
+				nouveau_display_relink(connector);
+		}
+		drm_connector_list_iter_end(&conn_iter);
+	}
+
 	if (changed == 1)
 		drm_kms_helper_connector_hotplug_event(first_changed_connector);
 	else if (changed > 0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
index 9bc24f3914bd..99eeb66cc6a8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dp.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
@@ -31,6 +31,8 @@
 
 #include <nvif/if0011.h>
 
+#include <subdev/gsp.h>
+
 MODULE_PARM_DESC(mst, "Enable DisplayPort multi-stream (default: enabled)");
 static int nouveau_mst = 1;
 module_param_named(mst, nouveau_mst, int, 0400);
@@ -452,7 +454,7 @@ nouveau_dp_train(struct nouveau_encoder *outp, bool mst, u32 khz, u8 bpc)
 }
 
 static bool
-nouveau_dp_link_check_locked(struct nouveau_encoder *outp)
+nouveau_dp_link_check_locked(struct nouveau_encoder *outp, bool *modeset)
 {
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
@@ -465,11 +467,24 @@ nouveau_dp_link_check_locked(struct nouveau_encoder *outp)
 	if (drm_dp_channel_eq_ok(link_status, outp->dp.lt.nr))
 		return true;
 
+	/* NVIDIA's own DisplayPort library only retrains an active link with
+	 * the SOR in flush mode, which we don't implement, and retraining it
+	 * without that on GSP-RM can hang the display engine.  Leave the link
+	 * to be restarted with a modeset instead.
+	 */
+	if (nvkm_gsp_rm(nvxx_device(nouveau_drm(outp->base.base.dev))->gsp)) {
+		*modeset = true;
+		return false;
+	}
+
 	return nouveau_dp_train_link(outp, true);
 }
 
+/* Returns false if the link has been lost, setting *modeset if it has to be
+ * restarted with a modeset rather than having been retrained here.
+ */
 bool
-nouveau_dp_link_check(struct nouveau_connector *nv_connector)
+nouveau_dp_link_check(struct nouveau_connector *nv_connector, bool *modeset)
 {
 	struct nouveau_encoder *outp = nv_connector->dp_encoder;
 	bool link_ok = true;
@@ -477,7 +492,7 @@ nouveau_dp_link_check(struct nouveau_connector *nv_connector)
 	if (outp) {
 		mutex_lock(&outp->dp.hpd_irq_lock);
 		if (outp->dp.lt.nr)
-			link_ok = nouveau_dp_link_check_locked(outp);
+			link_ok = nouveau_dp_link_check_locked(outp, modeset);
 		mutex_unlock(&outp->dp.hpd_irq_lock);
 	}
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h
index 9366678e7d05..1639b7e41c1d 100644
--- a/drivers/gpu/drm/nouveau/nouveau_encoder.h
+++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h
@@ -158,7 +158,7 @@ enum nouveau_dp_status {
 int nouveau_dp_detect(struct nouveau_connector *, struct nouveau_encoder *);
 int nouveau_dp_train(struct nouveau_encoder *outp, bool mst, u32 khz, u8 bpc);
 void nouveau_dp_power_down(struct nouveau_encoder *);
-bool nouveau_dp_link_check(struct nouveau_connector *);
+bool nouveau_dp_link_check(struct nouveau_connector *nv_connector, bool *modeset);
 void nouveau_dp_irq(struct work_struct *);
 enum drm_mode_status nv50_dp_mode_valid(struct nouveau_encoder *,
 					const struct drm_display_mode *,

-- 
2.55.0

[PATCH 3/5] drm/nouveau/dp: restart lost links with a modeset on GSP-RM

[David R. Hedges]

When a sink reports through IRQ_HPD that its link has lost lock,
nouveau_dp_link_check() retrains the link in place while the stream
keeps running.  Without GSP-RM, nvkm_dp_train() has a path for that
which leaves the source configuration alone.  On GSP-RM,
r535_dp_train() ignores its retrain argument and issues the same
NV0073_CTRL_CMD_DP_CTRL as for a new link.  NVIDIA's own DisplayPort
library never retrains an active link like that: it puts the SOR into
flush mode, retrains, configures the stream again and takes the SOR
out of flush mode, and warns that skipping part of that sequence can
hang the display engine.

That holds on a GA102 with GSP-RM 570.144.  After an experimental
patch retrained an active 4 x HBR2 link in place, the picture came
back with horizontal artefacts, and the next modeset, a refresh rate
change, never completed: "core notifier timeout", after which GSP
stopped answering RPCs.

nouveau doesn't implement flush mode, so on GSP-RM restart the pipe
with a full modeset of the CRTC driving the connector instead, using
drm_atomic_helper_reset_crtc().  That trains the link, configures the
stream and attaches the SOR in the same order as any other modeset,
which is also how i915 retrains a link.  The HPD worker does this after
dropping mode_config.mutex, so that probing isn't held up while it
runs.  If the sink now advertises less than the mode needs, the
modeset fails atomic check, and the connector's link-status is set to
BAD, as when a DPMS on finds the same.

The link check is only reached once the connector's DP encoder has
been cached, which on nv50+ it currently never is, so this has no
effect until the next patch.

Link: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515
Signed-off-by: David R. Hedges <linuxdev844@thelittleman.net>
---
 drivers/gpu/drm/nouveau/nouveau_display.c | 65 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/nouveau/nouveau_dp.c      | 21 ++++++++--
 drivers/gpu/drm/nouveau/nouveau_encoder.h |  2 +-
 3 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 4398cf0fc541..c5e914602baf 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -418,6 +418,53 @@ nouveau_display_hpd_resume(struct nouveau_drm *drm)
 	schedule_work(&drm->hpd_work);
 }
 
+/* Restart the pipe driving @connector with a full modeset, so that its link is
+ * trained again along with everything else.
+ */
+static void
+nouveau_display_relink(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_modeset_acquire_ctx ctx;
+	struct drm_crtc *crtc;
+	int ret;
+
+	drm_modeset_acquire_init(&ctx, 0);
+retry:
+	ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
+	if (ret)
+		goto out;
+
+	crtc = connector->state->crtc;
+	if (!crtc)
+		goto out;
+
+	ret = drm_modeset_lock(&crtc->mutex, &ctx);
+	if (ret)
+		goto out;
+
+	if (crtc->state->active) {
+		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] restarting link\n",
+			    connector->base.id, connector->name);
+		ret = drm_atomic_helper_reset_crtc(crtc, &ctx);
+	}
+out:
+	if (ret == -EDEADLK) {
+		drm_modeset_backoff(&ctx);
+		goto retry;
+	}
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+
+	/* The sink may now advertise less than the mode needs. */
+	if (ret == -EINVAL)
+		schedule_work(&nouveau_connector(connector)->link_bad_work);
+	else if (ret)
+		NV_ERROR(nouveau_drm(dev), "%s: failed to restart link: %d\n",
+			 connector->name, ret);
+}
+
 static void
 nouveau_display_hpd_work(struct work_struct *work)
 {
@@ -425,7 +472,7 @@ nouveau_display_hpd_work(struct work_struct *work)
 	struct drm_device *dev = drm->dev;
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
-	u32 pending;
+	u32 pending, relink = 0;
 	int changed = 0;
 	struct drm_connector *first_changed_connector = NULL;
 
@@ -447,6 +494,7 @@ nouveau_display_hpd_work(struct work_struct *work)
 		struct nouveau_connector *nv_connector = nouveau_connector(connector);
 		enum drm_connector_status old_status = connector->status;
 		u64 bits, old_epoch_counter = connector->epoch_counter;
+		bool modeset = false;
 
 		if (!(pending & drm_connector_mask(connector)))
 			continue;
@@ -468,11 +516,14 @@ nouveau_display_hpd_work(struct work_struct *work)
 		 */
 		if ((bits & NVIF_CONN_EVENT_V0_IRQ) &&
 		    !(bits & (NVIF_CONN_EVENT_V0_PLUG | NVIF_CONN_EVENT_V0_UNPLUG))) {
-			if (nouveau_dp_link_check(nv_connector))
+			if (nouveau_dp_link_check(nv_connector, &modeset))
 				continue;
 		}
 
 		connector->status = drm_helper_probe_detect(connector, NULL, false);
+		if (modeset && connector->status == connector_status_connected)
+			relink |= drm_connector_mask(connector);
+
 		if (old_epoch_counter == connector->epoch_counter)
 			continue;
 
@@ -492,6 +543,16 @@ nouveau_display_hpd_work(struct work_struct *work)
 	drm_connector_list_iter_end(&conn_iter);
 	mutex_unlock(&dev->mode_config.mutex);
 
+	/* A restart is a full modeset, so don't hold up probing while it runs. */
+	if (relink) {
+		drm_connector_list_iter_begin(dev, &conn_iter);
+		nouveau_for_each_non_mst_connector_iter(connector, &conn_iter) {
+			if (relink & drm_connector_mask(connector))
+				nouveau_display_relink(connector);
+		}
+		drm_connector_list_iter_end(&conn_iter);
+	}
+
 	if (changed == 1)
 		drm_kms_helper_connector_hotplug_event(first_changed_connector);
 	else if (changed > 0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c b/drivers/gpu/drm/nouveau/nouveau_dp.c
index 9bc24f3914bd..99eeb66cc6a8 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dp.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
@@ -31,6 +31,8 @@
 
 #include <nvif/if0011.h>
 
+#include <subdev/gsp.h>
+
 MODULE_PARM_DESC(mst, "Enable DisplayPort multi-stream (default: enabled)");
 static int nouveau_mst = 1;
 module_param_named(mst, nouveau_mst, int, 0400);
@@ -452,7 +454,7 @@ nouveau_dp_train(struct nouveau_encoder *outp, bool mst, u32 khz, u8 bpc)
 }
 
 static bool
-nouveau_dp_link_check_locked(struct nouveau_encoder *outp)
+nouveau_dp_link_check_locked(struct nouveau_encoder *outp, bool *modeset)
 {
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
@@ -465,11 +467,24 @@ nouveau_dp_link_check_locked(struct nouveau_encoder *outp)
 	if (drm_dp_channel_eq_ok(link_status, outp->dp.lt.nr))
 		return true;
 
+	/* NVIDIA's own DisplayPort library only retrains an active link with
+	 * the SOR in flush mode, which we don't implement, and retraining it
+	 * without that on GSP-RM can hang the display engine.  Leave the link
+	 * to be restarted with a modeset instead.
+	 */
+	if (nvkm_gsp_rm(nvxx_device(nouveau_drm(outp->base.base.dev))->gsp)) {
+		*modeset = true;
+		return false;
+	}
+
 	return nouveau_dp_train_link(outp, true);
 }
 
+/* Returns false if the link has been lost, setting *modeset if it has to be
+ * restarted with a modeset rather than having been retrained here.
+ */
 bool
-nouveau_dp_link_check(struct nouveau_connector *nv_connector)
+nouveau_dp_link_check(struct nouveau_connector *nv_connector, bool *modeset)
 {
 	struct nouveau_encoder *outp = nv_connector->dp_encoder;
 	bool link_ok = true;
@@ -477,7 +492,7 @@ nouveau_dp_link_check(struct nouveau_connector *nv_connector)
 	if (outp) {
 		mutex_lock(&outp->dp.hpd_irq_lock);
 		if (outp->dp.lt.nr)
-			link_ok = nouveau_dp_link_check_locked(outp);
+			link_ok = nouveau_dp_link_check_locked(outp, modeset);
 		mutex_unlock(&outp->dp.hpd_irq_lock);
 	}
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_encoder.h b/drivers/gpu/drm/nouveau/nouveau_encoder.h
index 9366678e7d05..1639b7e41c1d 100644
--- a/drivers/gpu/drm/nouveau/nouveau_encoder.h
+++ b/drivers/gpu/drm/nouveau/nouveau_encoder.h
@@ -158,7 +158,7 @@ enum nouveau_dp_status {
 int nouveau_dp_detect(struct nouveau_connector *, struct nouveau_encoder *);
 int nouveau_dp_train(struct nouveau_encoder *outp, bool mst, u32 khz, u8 bpc);
 void nouveau_dp_power_down(struct nouveau_encoder *);
-bool nouveau_dp_link_check(struct nouveau_connector *);
+bool nouveau_dp_link_check(struct nouveau_connector *nv_connector, bool *modeset);
 void nouveau_dp_irq(struct work_struct *);
 enum drm_mode_status nv50_dp_mode_valid(struct nouveau_encoder *,
 					const struct drm_display_mode *,

-- 
2.55.0

[PATCH 4/5] drm/nouveau/kms: cache the DP encoder after it has been attached

[David R. Hedges via B4 Relay]

From: "David R. Hedges" <linuxdev844@thelittleman.net>

nouveau_connector_create() caches the connector's DP encoder in
nv_connector->dp_encoder using find_encoder().  On nv50+ it's called
from nv50_display_create() before nv50_sor_create() has created the
encoder and attached it to the connector, so possible_encoders is
still empty, find_encoder() returns NULL, and dp_encoder is never set.
Two things depend on it:

 - nouveau_dp_link_check(), which the HPD worker calls on IRQ_HPD,
   reports the link as fine without looking at it when there's no DP
   encoder.  Until commit 633716501c94 ("drm/nouveau/disp: add dp
   train method") it looked the encoder up each time; since then, a
   sink that reports link loss with IRQ_HPD hasn't been retrained.

 - nouveau_connector_atomic_check() has never called
   drm_dp_mst_root_conn_atomic_check() for MST-capable root
   connectors.

Set dp_encoder in nv50_sor_create() right after the SOR is attached to
the connector, and drop the assignment that can't succeed.  This also
covers eDP, whose SOR is a DCB_OUTPUT_DP encoder too.  On a GA102 with
GSP-RM 570.144, an IRQ_HPD now leads to a read of the sink's link
status, where before it produced no AUX traffic at all.

This depends on "drm/nouveau/kms/nv50-: reject DP modesets the link
can't carry" and "drm/nouveau/dp: restart lost links with a modeset on
GSP-RM".  Without them, enabling the link check would have nouveau
retrain active links in place on GSP-RM, or restart them with a mode
the link can no longer carry, either of which can hang the display
engine.  The MST atomic check this enables hasn't been tested.

Fixes: 11d2738940eb ("drm/nouveau/kms: Cache DP encoders in nouveau_connector")
Fixes: 633716501c94 ("drm/nouveau/disp: add dp train method")
Link: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515
Signed-off-by: David R. Hedges <linuxdev844@thelittleman.net>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c     | 1 +
 drivers/gpu/drm/nouveau/nouveau_connector.c | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 7f55c016d846..583368914bf0 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1941,6 +1941,7 @@ nv50_sor_create(struct nouveau_encoder *nv_encoder)
 
 	if (dcbe->type == DCB_OUTPUT_DP) {
 		mutex_init(&nv_encoder->dp.hpd_irq_lock);
+		nv_connector->dp_encoder = nv_encoder;
 
 		if (disp->disp->object.oclass < GF110_DISP) {
 			/* HW has no support for address-only
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index cae4e35c30aa..845f284f3a20 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -1573,8 +1573,6 @@ nouveau_connector_create(struct drm_device *dev, int index)
 
 	switch (type) {
 	case DRM_MODE_CONNECTOR_DisplayPort:
-		nv_connector->dp_encoder = find_encoder(&nv_connector->base, DCB_OUTPUT_DP);
-		fallthrough;
 	case DRM_MODE_CONNECTOR_eDP:
 		drm_dp_cec_register_connector(&nv_connector->aux, connector);
 		break;

-- 
2.55.0

[PATCH 4/5] drm/nouveau/kms: cache the DP encoder after it has been attached

[David R. Hedges]

nouveau_connector_create() caches the connector's DP encoder in
nv_connector->dp_encoder using find_encoder().  On nv50+ it's called
from nv50_display_create() before nv50_sor_create() has created the
encoder and attached it to the connector, so possible_encoders is
still empty, find_encoder() returns NULL, and dp_encoder is never set.
Two things depend on it:

 - nouveau_dp_link_check(), which the HPD worker calls on IRQ_HPD,
   reports the link as fine without looking at it when there's no DP
   encoder.  Until commit 633716501c94 ("drm/nouveau/disp: add dp
   train method") it looked the encoder up each time; since then, a
   sink that reports link loss with IRQ_HPD hasn't been retrained.

 - nouveau_connector_atomic_check() has never called
   drm_dp_mst_root_conn_atomic_check() for MST-capable root
   connectors.

Set dp_encoder in nv50_sor_create() right after the SOR is attached to
the connector, and drop the assignment that can't succeed.  This also
covers eDP, whose SOR is a DCB_OUTPUT_DP encoder too.  On a GA102 with
GSP-RM 570.144, an IRQ_HPD now leads to a read of the sink's link
status, where before it produced no AUX traffic at all.

This depends on "drm/nouveau/kms/nv50-: reject DP modesets the link
can't carry" and "drm/nouveau/dp: restart lost links with a modeset on
GSP-RM".  Without them, enabling the link check would have nouveau
retrain active links in place on GSP-RM, or restart them with a mode
the link can no longer carry, either of which can hang the display
engine.  The MST atomic check this enables hasn't been tested.

Fixes: 11d2738940eb ("drm/nouveau/kms: Cache DP encoders in nouveau_connector")
Fixes: 633716501c94 ("drm/nouveau/disp: add dp train method")
Link: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515
Signed-off-by: David R. Hedges <linuxdev844@thelittleman.net>
---
 drivers/gpu/drm/nouveau/dispnv50/disp.c     | 1 +
 drivers/gpu/drm/nouveau/nouveau_connector.c | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 7f55c016d846..583368914bf0 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -1941,6 +1941,7 @@ nv50_sor_create(struct nouveau_encoder *nv_encoder)
 
 	if (dcbe->type == DCB_OUTPUT_DP) {
 		mutex_init(&nv_encoder->dp.hpd_irq_lock);
+		nv_connector->dp_encoder = nv_encoder;
 
 		if (disp->disp->object.oclass < GF110_DISP) {
 			/* HW has no support for address-only
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index cae4e35c30aa..845f284f3a20 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -1573,8 +1573,6 @@ nouveau_connector_create(struct drm_device *dev, int index)
 
 	switch (type) {
 	case DRM_MODE_CONNECTOR_DisplayPort:
-		nv_connector->dp_encoder = find_encoder(&nv_connector->base, DCB_OUTPUT_DP);
-		fallthrough;
 	case DRM_MODE_CONNECTOR_eDP:
 		drm_dp_cec_register_connector(&nv_connector->aux, connector);
 		break;

-- 
2.55.0

[PATCH 5/5] drm/nouveau/dp: restart an active link when the sink reasserts HPD

[David R. Hedges via B4 Relay]

From: "David R. Hedges" <linuxdev844@thelittleman.net>

When a DP sink drops HPD for longer than an IRQ_HPD pulse, it may have
reset its end of the link.  nouveau handles the unplug and plug by
re-running connector detection and sending hotplug events, but if the
same display comes back, userspace sees nothing that needs a modeset
and leaves the CRTC as it was.  Nothing trains the link again, and the
sink is left without a picture.

That's what happens with an IGM L28TN4K on a GA102 (GSP-RM 570.144)
after waking it from DPMS off.  If the link is trained before the
monitor has finished waking up, training succeeds at 4 x HBR2, but
about two seconds later the monitor raises IRQ_HPD and shows "No input
signal", and two seconds after that it drops HPD briefly and reasserts
it.  The link status read on the IRQ_HPD is 77 77 01 03 (DPCD
0x202-0x205: all lanes locked, interlane-aligned, sink in sync), and
DP_SET_POWER still reads D0 once HPD is back, so there's nothing to
check that would show the problem.  Whenever the IRQ_HPD came before
link training instead, the picture came up normally.  The same happens
after replugging the cable and after resuming from S3, and the screen
stays dark until the cable is replugged, often several times.

When a plug or unplug event leaves a DP connector connected while it
still drives an active CRTC, restart the link with a modeset, using
the restart added for lost links on GSP-RM, on any GPU.  NVIDIA's own
DisplayPort library does the equivalent:
ConnectorImpl::notifyLongPulseInternal() calls assessLink(), which
retrains the link, for an SST sink that's still connected.  i915, by
contrast, only retrains on hotplug if the link status says the link is
bad.  With this applied, the monitor above comes back four to five
seconds after being woken, replugged or resumed.  On resume,
nouveau_display_hpd_resume() schedules the HPD worker without any
events, so this doesn't add a restart to the resume modeset.

Switched on with its power button, which it reports only as a plug
event, the same monitor first advertises DPCD 1.1 and HBR, which can't
carry the 3840x2160@60 mode its CRTC still has.  The restart then fails
atomic check, link-status is set to BAD, and userspace, which gets a
hotplug event, picks a mode that fits.  About a second later the
monitor drops and reasserts HPD with its full capabilities, and
userspace goes back to 3840x2160@60.

A sink that dropped HPD every time its link was trained would have the
link restarted for ever, so stop after five restarts that each came
within ten seconds of the one before, until ten seconds have passed
without one.  If userspace has already responded to a hotplug event
with a modeset of its own by the time the HPD worker runs, the link is
restarted a second time, which costs another brief blank but is
otherwise harmless.

Closes: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515
Signed-off-by: David R. Hedges <linuxdev844@thelittleman.net>
---
 drivers/gpu/drm/nouveau/nouveau_connector.h |  5 +++
 drivers/gpu/drm/nouveau/nouveau_display.c   | 51 ++++++++++++++++++++++++++---
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.h b/drivers/gpu/drm/nouveau/nouveau_connector.h
index 3109278d36ea..914e86ad2df2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.h
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.h
@@ -135,6 +135,11 @@ struct nouveau_connector {
 	struct nvif_event irq;
 	struct work_struct irq_work;
 	struct work_struct link_bad_work;
+	/* Recent link restarts, see nouveau_display_relink() */
+	struct {
+		unsigned long last;
+		unsigned int count;
+	} relink;
 
 	struct drm_dp_aux aux;
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index c5e914602baf..a52db6129962 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -418,6 +418,33 @@ nouveau_display_hpd_resume(struct nouveau_drm *drm)
 	schedule_work(&drm->hpd_work);
 }
 
+/* A sink that responds to link training by dropping HPD would otherwise have
+ * its link restarted for ever.  Stop after a few restarts that each came soon
+ * after the one before, until things have been quiet for a while.
+ */
+#define NOUVEAU_RELINK_MAX	5
+#define NOUVEAU_RELINK_QUIET	(10 * HZ)
+
+static bool
+nouveau_display_relink_allowed(struct drm_connector *connector)
+{
+	struct nouveau_connector *nv_connector = nouveau_connector(connector);
+	unsigned long now = jiffies;
+
+	if (time_after(now, nv_connector->relink.last + NOUVEAU_RELINK_QUIET))
+		nv_connector->relink.count = 0;
+	nv_connector->relink.last = now;
+
+	if (nv_connector->relink.count++ < NOUVEAU_RELINK_MAX)
+		return true;
+
+	if (nv_connector->relink.count == NOUVEAU_RELINK_MAX + 1)
+		NV_WARN(nouveau_drm(connector->dev),
+			"%s: link restarted %d times in quick succession, giving up\n",
+			connector->name, NOUVEAU_RELINK_MAX);
+	return false;
+}
+
 /* Restart the pipe driving @connector with a full modeset, so that its link is
  * trained again along with everything else.
  */
@@ -427,6 +454,7 @@ nouveau_display_relink(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_crtc *crtc;
+	bool counted = false;
 	int ret;
 
 	drm_modeset_acquire_init(&ctx, 0);
@@ -443,11 +471,18 @@ nouveau_display_relink(struct drm_connector *connector)
 	if (ret)
 		goto out;
 
-	if (crtc->state->active) {
-		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] restarting link\n",
-			    connector->base.id, connector->name);
-		ret = drm_atomic_helper_reset_crtc(crtc, &ctx);
+	if (!crtc->state->active)
+		goto out;
+
+	if (!counted) {
+		if (!nouveau_display_relink_allowed(connector))
+			goto out;
+		counted = true;
 	}
+
+	drm_dbg_kms(dev, "[CONNECTOR:%d:%s] restarting link\n",
+		    connector->base.id, connector->name);
+	ret = drm_atomic_helper_reset_crtc(crtc, &ctx);
 out:
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(&ctx);
@@ -510,6 +545,14 @@ nouveau_display_hpd_work(struct work_struct *work)
 			    !!(bits & NVIF_CONN_EVENT_V0_UNPLUG),
 			    !!(bits & NVIF_CONN_EVENT_V0_IRQ));
 
+		/* If a DP sink dropped HPD while we were driving it, it may have
+		 * reset its end of the link whatever its link status says, and
+		 * userspace won't modeset again if nothing else has changed.
+		 */
+		if (nv_connector->dp_encoder &&
+		    (bits & (NVIF_CONN_EVENT_V0_PLUG | NVIF_CONN_EVENT_V0_UNPLUG)))
+			modeset = true;
+
 		/* A plug or unplug event pending alongside the IRQ, such as a
 		 * sink count change reported by nouveau_dp_irq(), still needs
 		 * connector detection.

-- 
2.55.0

[PATCH 5/5] drm/nouveau/dp: restart an active link when the sink reasserts HPD

[David R. Hedges]

When a DP sink drops HPD for longer than an IRQ_HPD pulse, it may have
reset its end of the link.  nouveau handles the unplug and plug by
re-running connector detection and sending hotplug events, but if the
same display comes back, userspace sees nothing that needs a modeset
and leaves the CRTC as it was.  Nothing trains the link again, and the
sink is left without a picture.

That's what happens with an IGM L28TN4K on a GA102 (GSP-RM 570.144)
after waking it from DPMS off.  If the link is trained before the
monitor has finished waking up, training succeeds at 4 x HBR2, but
about two seconds later the monitor raises IRQ_HPD and shows "No input
signal", and two seconds after that it drops HPD briefly and reasserts
it.  The link status read on the IRQ_HPD is 77 77 01 03 (DPCD
0x202-0x205: all lanes locked, interlane-aligned, sink in sync), and
DP_SET_POWER still reads D0 once HPD is back, so there's nothing to
check that would show the problem.  Whenever the IRQ_HPD came before
link training instead, the picture came up normally.  The same happens
after replugging the cable and after resuming from S3, and the screen
stays dark until the cable is replugged, often several times.

When a plug or unplug event leaves a DP connector connected while it
still drives an active CRTC, restart the link with a modeset, using
the restart added for lost links on GSP-RM, on any GPU.  NVIDIA's own
DisplayPort library does the equivalent:
ConnectorImpl::notifyLongPulseInternal() calls assessLink(), which
retrains the link, for an SST sink that's still connected.  i915, by
contrast, only retrains on hotplug if the link status says the link is
bad.  With this applied, the monitor above comes back four to five
seconds after being woken, replugged or resumed.  On resume,
nouveau_display_hpd_resume() schedules the HPD worker without any
events, so this doesn't add a restart to the resume modeset.

Switched on with its power button, which it reports only as a plug
event, the same monitor first advertises DPCD 1.1 and HBR, which can't
carry the 3840x2160@60 mode its CRTC still has.  The restart then fails
atomic check, link-status is set to BAD, and userspace, which gets a
hotplug event, picks a mode that fits.  About a second later the
monitor drops and reasserts HPD with its full capabilities, and
userspace goes back to 3840x2160@60.

A sink that dropped HPD every time its link was trained would have the
link restarted for ever, so stop after five restarts that each came
within ten seconds of the one before, until ten seconds have passed
without one.  If userspace has already responded to a hotplug event
with a modeset of its own by the time the HPD worker runs, the link is
restarted a second time, which costs another brief blank but is
otherwise harmless.

Closes: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515
Signed-off-by: David R. Hedges <linuxdev844@thelittleman.net>
---
 drivers/gpu/drm/nouveau/nouveau_connector.h |  5 +++
 drivers/gpu/drm/nouveau/nouveau_display.c   | 51 ++++++++++++++++++++++++++---
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.h b/drivers/gpu/drm/nouveau/nouveau_connector.h
index 3109278d36ea..914e86ad2df2 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.h
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.h
@@ -135,6 +135,11 @@ struct nouveau_connector {
 	struct nvif_event irq;
 	struct work_struct irq_work;
 	struct work_struct link_bad_work;
+	/* Recent link restarts, see nouveau_display_relink() */
+	struct {
+		unsigned long last;
+		unsigned int count;
+	} relink;
 
 	struct drm_dp_aux aux;
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index c5e914602baf..a52db6129962 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -418,6 +418,33 @@ nouveau_display_hpd_resume(struct nouveau_drm *drm)
 	schedule_work(&drm->hpd_work);
 }
 
+/* A sink that responds to link training by dropping HPD would otherwise have
+ * its link restarted for ever.  Stop after a few restarts that each came soon
+ * after the one before, until things have been quiet for a while.
+ */
+#define NOUVEAU_RELINK_MAX	5
+#define NOUVEAU_RELINK_QUIET	(10 * HZ)
+
+static bool
+nouveau_display_relink_allowed(struct drm_connector *connector)
+{
+	struct nouveau_connector *nv_connector = nouveau_connector(connector);
+	unsigned long now = jiffies;
+
+	if (time_after(now, nv_connector->relink.last + NOUVEAU_RELINK_QUIET))
+		nv_connector->relink.count = 0;
+	nv_connector->relink.last = now;
+
+	if (nv_connector->relink.count++ < NOUVEAU_RELINK_MAX)
+		return true;
+
+	if (nv_connector->relink.count == NOUVEAU_RELINK_MAX + 1)
+		NV_WARN(nouveau_drm(connector->dev),
+			"%s: link restarted %d times in quick succession, giving up\n",
+			connector->name, NOUVEAU_RELINK_MAX);
+	return false;
+}
+
 /* Restart the pipe driving @connector with a full modeset, so that its link is
  * trained again along with everything else.
  */
@@ -427,6 +454,7 @@ nouveau_display_relink(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_crtc *crtc;
+	bool counted = false;
 	int ret;
 
 	drm_modeset_acquire_init(&ctx, 0);
@@ -443,11 +471,18 @@ nouveau_display_relink(struct drm_connector *connector)
 	if (ret)
 		goto out;
 
-	if (crtc->state->active) {
-		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] restarting link\n",
-			    connector->base.id, connector->name);
-		ret = drm_atomic_helper_reset_crtc(crtc, &ctx);
+	if (!crtc->state->active)
+		goto out;
+
+	if (!counted) {
+		if (!nouveau_display_relink_allowed(connector))
+			goto out;
+		counted = true;
 	}
+
+	drm_dbg_kms(dev, "[CONNECTOR:%d:%s] restarting link\n",
+		    connector->base.id, connector->name);
+	ret = drm_atomic_helper_reset_crtc(crtc, &ctx);
 out:
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(&ctx);
@@ -510,6 +545,14 @@ nouveau_display_hpd_work(struct work_struct *work)
 			    !!(bits & NVIF_CONN_EVENT_V0_UNPLUG),
 			    !!(bits & NVIF_CONN_EVENT_V0_IRQ));
 
+		/* If a DP sink dropped HPD while we were driving it, it may have
+		 * reset its end of the link whatever its link status says, and
+		 * userspace won't modeset again if nothing else has changed.
+		 */
+		if (nv_connector->dp_encoder &&
+		    (bits & (NVIF_CONN_EVENT_V0_PLUG | NVIF_CONN_EVENT_V0_UNPLUG)))
+			modeset = true;
+
 		/* A plug or unplug event pending alongside the IRQ, such as a
 		 * sink count change reported by nouveau_dp_irq(), still needs
 		 * connector detection.

-- 
2.55.0

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

[sashiko-bot]

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

Re: [PATCH 5/5] drm/nouveau/dp: restart an active link when the sink reasserts HPD

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] drm/nouveau: uncancelled link_bad_work and irq_work in nouveau_display_fini()
- [Medium] drm/nouveau: async link_bad_work in nouveau_display_relink() races with hotplug
- [Medium] drm/nouveau: nouveau_display_relink() leaves BAD link_status after recovery
- [Medium] drm/nouveau: nv50_outp_atomic_check() fails drm_atomic_helper_resume() commits
- [Medium] drm/nouveau: nv50_disp_atomic_commit_tail() enables audio on aborted SOR enable
- [Medium] drm/nouveau: nv50_pior_create() does not set nv_connector->dp_encoder for DP
- [Medium] drm/nouveau/dp: check active and detected DP encoder before relinking
--

commit 87d2a7cb9667f947ddf67914d19f058419e967fd
Author: David R. Hedges <linuxdev844@thelittleman.net>

drm/nouveau/dp: restart an active link when the sink reasserts HPD

This commit restarts active DisplayPort links with a full CRTC modeset when
the sink signals a plug or unplug HPD event and remains connected, bounded by
a rate limit of five restarts within ten-second intervals.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index c5e914602baff..a52db61299626 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c

[ ... ]

> @@ -443,11 +471,18 @@ nouveau_display_relink(struct drm_connector *connector)
>  	if (ret)
>  		goto out;
>  
> -	if (crtc->state->active) {
> -		drm_dbg_kms(dev, "[CONNECTOR:%d:%s] restarting link\n",
> -			    connector->base.id, connector->name);
> -		ret = drm_atomic_helper_reset_crtc(crtc, &ctx);
> +	if (!crtc->state->active)
> +		goto out;
> +
> +	if (!counted) {
> +		if (!nouveau_display_relink_allowed(connector))
> +			goto out;
> +		counted = true;
>  	}
> +
> +	drm_dbg_kms(dev, "[CONNECTOR:%d:%s] restarting link\n",
> +		    connector->base.id, connector->name);
> +	ret = drm_atomic_helper_reset_crtc(crtc, &ctx);

[Severity: Medium]
What happens if connector->state->link_status was previously set to
DRM_MODE_LINK_STATUS_BAD (for example after nv50_sor_atomic_enable() or an
earlier nouveau_display_relink() failed when the monitor briefly advertised
reduced capabilities) and a subsequent HPD event triggers
nouveau_display_relink() once full link capabilities are restored?

In drivers/gpu/drm/nouveau/nouveau_display.c:nouveau_display_relink(),
drm_atomic_helper_reset_crtc() duplicates connector->state via
nouveau_conn_atomic_duplicate_state() without resetting link_status to
DRM_MODE_LINK_STATUS_GOOD and without cancelling any pending link_bad_work.

Could a successful link restart in nouveau_display_relink() leave
connector->state->link_status stuck at DRM_MODE_LINK_STATUS_BAD when
userspace processes the hotplug event?

[Severity: Medium]
Can scheduling link_bad_work asynchronously in nouveau_display_relink() race
with the hotplug event sent by nouveau_display_hpd_work()?

In drivers/gpu/drm/nouveau/nouveau_display.c:nouveau_display_relink():

	drm_modeset_drop_locks(&ctx);
	drm_modeset_acquire_fini(&ctx);

	/* The sink may now advertise less than the mode needs. */
	if (ret == -EINVAL)
		schedule_work(&nouveau_connector(connector)->link_bad_work);

When a DP sink asserts HPD with reduced capabilities that cannot carry the
current CRTC mode, drm_atomic_helper_reset_crtc() fails
nv50_outp_atomic_check() with -EINVAL. nouveau_display_relink() drops
dev->mode_config.connection_mutex and schedules link_bad_work, and then
nouveau_display_hpd_work() immediately sends the hotplug event:

	if (relink) {
		drm_connector_list_iter_begin(dev, &conn_iter);
		nouveau_for_each_non_mst_connector_iter(connector, &conn_iter) {
			if (relink & drm_connector_mask(connector))
				nouveau_display_relink(connector);
		}
		drm_connector_list_iter_end(&conn_iter);
	}

	if (changed == 1)
		drm_kms_helper_connector_hotplug_event(first_changed_connector);
	else if (changed > 0)
		drm_kms_helper_hotplug_event(dev);

Because link_bad_work runs asynchronously in
drivers/gpu/drm/nouveau/nouveau_connector.c:nouveau_connector_link_bad():

	drm_connector_set_link_status_property(connector, DRM_MODE_LINK_STATUS_BAD);
	drm_kms_helper_connector_hotplug_event(connector);

userspace can receive the initial hotplug event from
nouveau_display_hpd_work() before connector->state->link_status has been set
to DRM_MODE_LINK_STATUS_BAD. Furthermore, if userspace or an in-kernel DRM
client completes a valid modeset (or a subsequent HPD event retrains the
link) before link_bad_work executes, wouldn't nouveau_connector_link_bad()
still overwrite connector->state->link_status with DRM_MODE_LINK_STATUS_BAD
on the working link?

Would it avoid this race to update connector->state->link_status directly
while nouveau_display_relink() already holds dev->mode_config.connection_mutex
in ctx?

> @@ -510,6 +545,14 @@ nouveau_display_hpd_work(struct work_struct *work)
>  			    !!(bits & NVIF_CONN_EVENT_V0_UNPLUG),
>  			    !!(bits & NVIF_CONN_EVENT_V0_IRQ));
>  
> +		/* If a DP sink dropped HPD while we were driving it, it may have
> +		 * reset its end of the link whatever its link status says, and
> +		 * userspace won't modeset again if nothing else has changed.
> +		 */
> +		if (nv_connector->dp_encoder &&
> +		    (bits & (NVIF_CONN_EVENT_V0_PLUG | NVIF_CONN_EVENT_V0_UNPLUG)))
> +			modeset = true;

[Severity: Medium]
On Dual-Mode DisplayPort (DP++) connectors, both a DCB_OUTPUT_DP encoder and
a DCB_OUTPUT_TMDS encoder are attached to the same struct nouveau_connector
with nv_connector->dp_encoder set to the DP encoder in nv50_sor_create().

If a passive HDMI/DVI (TMDS) sink is connected on a DP++ port, or if a user
swaps between a native DP sink and a passive DP++ TMDS adapter while the CRTC
is active, nouveau_display_hpd_work() sets modeset = true based only on
nv_connector->dp_encoder being non-NULL, and nouveau_display_relink() calls
drm_atomic_helper_reset_crtc() before userspace receives the hotplug event:

nouveau_display_hpd_work()
  -> nouveau_display_relink()
       -> drm_atomic_helper_reset_crtc()

Because drm_atomic_helper_reset_crtc() only adds the CRTC to the atomic state
before drm_atomic_helper_check_modeset() runs update_connector_routing(),
new_connector_state->best_encoder remains old_connector_state->best_encoder.
When driving a passive TMDS sink, every HPD plug/unplug event triggers an
unnecessary full modeset; when switching between DP and TMDS sinks on the
same connector, drm_atomic_helper_reset_crtc() runs against the stale
best_encoder (either failing nv50_outp_atomic_check() with -EINVAL and
marking link-status BAD, or committing the wrong encoder protocol for the
newly detected sink).

Should nouveau_display_hpd_work() and nouveau_display_relink() verify that
both nv_connector->detected_encoder and connector->state->best_encoder match
nv_connector->dp_encoder before restarting the link?

[Severity: Medium]
In the earlier patch in this series, commit bef9030b7486 ("drm/nouveau/kms:
cache the DP encoder after it has been attached"), the find_encoder() lookup
for nv_connector->dp_encoder was removed from nouveau_connector_create() and
added to nv50_sor_create():

drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_sor_create() {
	if (dcbe->type == DCB_OUTPUT_DP) {
		mutex_init(&nv_encoder->dp.hpd_irq_lock);
		nv_connector->dp_encoder = nv_encoder;
		...
	}
}

However, drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_pior_create() also
creates DCB_OUTPUT_DP encoders for external DisplayPort encoders driven via
PIOR on NV50-family GPUs:

drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_pior_create() {
	switch (dcbe->type) {
	...
	case DCB_OUTPUT_DP:
		aux  = nvkm_i2c_aux_find(i2c, nv_encoder->outp.info.dp.aux);
		ddc  = aux ? &aux->i2c : NULL;
		type = DRM_MODE_ENCODER_TMDS;
		break;
	default:
		return -ENODEV;
	}

	nv_encoder->i2c = ddc;

	mutex_init(&nv_encoder->dp.hpd_irq_lock);
}

Without setting nv_connector->dp_encoder = nv_encoder in nv50_pior_create(),
won't nv_connector->dp_encoder remain NULL on PIOR DP connectors, causing
both nouveau_dp_link_check() and this new HPD relink check in
nouveau_display_hpd_work() to be skipped?

[Severity: High]
With nouveau_display_hpd_work() now calling nouveau_display_relink() on DP
plug and unplug events across all GPUs (which schedules
nv_connector->link_bad_work when drm_atomic_helper_reset_crtc() returns
-EINVAL), how are nv_connector->link_bad_work and nv_connector->irq_work
synchronized during suspend and driver teardown in nouveau_display_fini()?

Looking at drivers/gpu/drm/nouveau/nouveau_display.c:nouveau_display_fini():

	/* disable hotplug interrupts */
	drm_connector_list_iter_begin(dev, &conn_iter);
	nouveau_for_each_non_mst_connector_iter(connector, &conn_iter) {
		struct nouveau_connector *conn = nouveau_connector(connector);
		nvif_event_block(&conn->irq);
		nvif_event_block(&conn->hpd);
	}
	drm_connector_list_iter_end(&conn_iter);

	if (!runtime && !drm->headless)
		cancel_work_sync(&drm->hpd_work);

conn->irq_work and conn->link_bad_work are only cancelled later in
drivers/gpu/drm/nouveau/nouveau_connector.c:nouveau_connector_destroy():

	nvif_event_dtor(&nv_connector->irq);
	nvif_event_dtor(&nv_connector->hpd);
	cancel_work_sync(&nv_connector->irq_work);
	cancel_work_sync(&nv_connector->link_bad_work);

This appears to leave two regressions:

1. During system suspend (nouveau_display_suspend() ->
   nouveau_display_fini()), neither conn->irq_work nor conn->link_bad_work is
   cancelled. If conn->irq_work was queued just before nvif_event_block(),
   nouveau_dp_irq() can run during suspend and re-schedule drm->hpd_work
   after cancel_work_sync(&drm->hpd_work) has returned. Similarly, if
   drm->hpd_work or nv50_sor_atomic_enable() queued conn->link_bad_work,
   nouveau_connector_link_bad() can run after drm_atomic_helper_suspend() and
   nouveau_display_fini() have completed, mutating connector->state and
   sending hotplug events on a suspended device.

2. During driver unload, nouveau_display_destroy() calls
   drm_mode_config_cleanup(), which destroys and frees all drm_encoder
   instances (nv50_sor_destroy(), nv50_pior_destroy()) before destroying
   connectors in nouveau_connector_destroy(). If conn->irq_work is still
   running during drm_mode_config_cleanup(), nouveau_dp_irq() can access a
   freed nouveau_encoder and its destroyed hpd_irq_lock, and re-schedule
   drm->hpd_work after nouveau_display_fini() already cancelled it.

Should nouveau_display_fini() cancel conn->irq_work before
cancel_work_sync(&drm->hpd_work) and cancel conn->link_bad_work after it?

[Severity: Medium]
In commit e5f61700807a ("drm/nouveau/kms/nv50-: reject DP modesets the link
can't carry") in this series, nv50_outp_atomic_check() was updated to reject
modesets when nv50_outp_atomic_fix_depth() returns false:

drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_outp_atomic_check() {
	if (!nv50_outp_atomic_fix_depth(encoder, crtc_state) &&
	    (crtc_state->mode_changed || crtc_state->connectors_changed))
		return -EINVAL;
}

During system resume, nouveau_display_resume() calls
drm_atomic_helper_resume() before nouveau_display_hpd_resume().
drm_atomic_helper_resume() resets mode_config state (setting
crtc->state->enable = false) and commits the state saved by
drm_atomic_helper_suspend() (which has state->duplicated == true). If a DP
sink was powered off or disconnected prior to resume (so
nouveau_dp_probe_dpcd() zeroed outp->dp.link_nr and outp->dp.link_bw, or
cached reduced capabilities) while its CRTC had enable == true when
suspended (for example in DPMS off with active == false, or after
nv50_sor_atomic_enable() aborted on -EINVAL), drm_atomic_helper_check_modeset()
sees old_crtc_state->enable != new_crtc_state->enable and sets
mode_changed = true and connectors_changed = true even when
new_crtc_state->active is false.

Unlike nv50_msto_atomic_check(), which guards its check with
!state->duplicated, won't nv50_outp_atomic_check() return -EINVAL here and
cause drm_atomic_helper_resume() to fail the entire multi-CRTC resume commit?

[Severity: Medium]
In commit 9c72b9cc42ad ("drm/nouveau/kms/nv50-: don't enable a DP output with
no trained link") in this series, nv50_sor_atomic_enable() returns early when
nouveau_dp_train() returns -EINVAL:

drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_sor_atomic_enable() {
	case DCB_OUTPUT_DP:
		if (nouveau_dp_train(nv_encoder, false, mode->clock, asyh->or.bpc) == -EINVAL) {
			schedule_work(&nv_connector->link_bad_work);
			return;
		}
}

However, its caller in
drivers/gpu/drm/nouveau/dispnv50/disp.c:nv50_disp_atomic_commit_tail() still
unconditionally marks outp->enabled = true:

	list_for_each_entry(outp, &atom->outp, head) {
		...
		if (outp->set.mask) {
			help->atomic_enable(encoder, state);
			outp->enabled = true;
			interlock[NV50_DISP_INTERLOCK_CORE] = 1;
		}
	}

When nv50_disp_atomic_commit_core() then runs:

	list_for_each_entry(outp, &atom->outp, head) {
		if (outp->encoder->encoder_type != DRM_MODE_ENCODER_DPMST) {
			struct nouveau_encoder *nv_encoder = nouveau_encoder(outp->encoder);

			if (outp->enabled) {
				nv50_audio_enable(outp->encoder, nouveau_crtc(nv_encoder->crtc),
						  nv_encoder->conn, NULL, NULL);
				outp->enabled = outp->disabled = false;
			}

doesn't this call nv50_audio_enable() (invoking nvif_outp_hda_eld() and
notifying the ALSA audio component) on an untrained, detached SOR whenever
the connector's cached display_info.has_audio is true?

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

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

[David R. Hedges]

On 2026-11-03 14:27 -0600, sashiko-bot@kernel.org wrote:
> When nv50_sor_atomic_enable() returns early here on -EINVAL, does this 
> still
> end up enabling DP audio on the untrained and detached SOR?

[...]

Yeah, nv50_disp_atomic_commit_tail() marks the output enabled whatever
nv50_sor_atomic_enable() did, and nv50_disp_atomic_commit_core() then
enables audio on it.  In v2, I'll have nouveau_dp_train() clear dp.lt.nr
before it starts, so an output left detached has no trained link, and
nv50_audio_enable() skips a DP output without one.

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

[David R. Hedges]

On 2026-11-03 14:27 -0600, sashiko-bot@kernel.org wrote:
> When nv50_sor_atomic_enable() returns early here on -EINVAL, does this 
> still
> end up enabling DP audio on the untrained and detached SOR?

[...]

Yeah, nv50_disp_atomic_commit_tail() marks the output enabled whatever
nv50_sor_atomic_enable() did, and nv50_disp_atomic_commit_core() then
enables audio on it.  In v2, I'll have nouveau_dp_train() clear dp.lt.nr
before it starts, so an output left detached has no trained link, and
nv50_audio_enable() skips a DP output without one.

Re: [PATCH 5/5] drm/nouveau/dp: restart an active link when the sink reasserts HPD

[David R. Hedges]

On 2026-11-03 14:36 -0600, sashiko-bot@kernel.org wrote:

> Could a successful link restart in nouveau_display_relink() leave
> connector->state->link_status stuck at DRM_MODE_LINK_STATUS_BAD when
> userspace processes the hotplug event?

It could, and I've left that as-is. The link-status documentation has
userspace set it back to GOOD, with the legacy SETCRTC path doing it
for non-atomic userspace, and no driver sets it to GOOD itself, i915
included. Userspace that reads link-status does one more modeset than
it needs to; nouveau doesn't act on the property. The v2 cover letter
I'm drafting mentions it. v2 will cancel a pending link_bad_work when
a later enable, such as the restart, trains the link.

> Would it avoid this race to update connector->state->link_status 
> directly
> while nouveau_display_relink() already holds 
> dev->mode_config.connection_mutex
> in ctx?

Yes, v2 will do that, and send the hotplug event from
nouveau_display_relink() once the locks are dropped. The work item is
only used from nv50_sor_atomic_enable() now, which can't take the
connection mutex, and a later enable that trains the link cancels it.

> Should nouveau_display_hpd_work() and nouveau_display_relink() verify 
> that
> both nv_connector->detected_encoder and connector->state->best_encoder 
> match
> nv_connector->dp_encoder before restarting the link?

Yes. v2 checks detected_encoder in the HPD worker and best_encoder in
nouveau_display_relink(), and leaves the other cases to userspace and
the hotplug event.

> Without setting nv_connector->dp_encoder = nv_encoder in 
> nv50_pior_create(),
> won't nv_connector->dp_encoder remain NULL on PIOR DP connectors, 
> causing
> both nouveau_dp_link_check() and this new HPD relink check in
> nouveau_display_hpd_work() to be skipped?

I'm not familiar with PIOR beyond some poking in the code--which
I now know is the external ANX9805 transmitter some NV50-family
boards use--and I have no such board to test it on, but I believe
the answer is yes. Before 633716501c94, the link check looked up any
DP encoder, including one on a PIOR, so my v2 draft sets dp_encoder
in nv50_pior_create() as well. nouveau doesn't get IRQ_HPD from that
chip, so what this enables there is the restart on HPD reassert.

> Should nouveau_display_fini() cancel conn->irq_work before
> cancel_work_sync(&drm->hpd_work) and cancel conn->link_bad_work after 
> it?

Yes for both. The irq_work part predates this series, so I'll add a
separate patch for it in v2, fixing 773eb04d14a1 ("drm/nouveau/disp:
expose conn event class"). f60849975a88 cancels it in
nouveau_connector_destroy(), but that's after drm_mode_config_cleanup()
has freed the encoders. link_bad_work is cancelled in the same loop,
since in v2 hpd_work no longer schedules it, and only on system
suspend and unload: its hotplug event can probe the connector, which
waits for runtime PM, and that's also why hpd_work isn't cancelled
on runtime suspend.

> Unlike nv50_msto_atomic_check(), which guards its check with
> !state->duplicated, won't nv50_outp_atomic_check() return -EINVAL here 
> and
> cause drm_atomic_helper_resume() to fail the entire multi-CRTC resume 
> commit?

Yes; good catch: drm_mode_config_reset() makes the restore a modeset,
and failing it leaves every output dark after resume. My draft for v2
doesn't fail a duplicated state, as nv50_msto_atomic_check() does, and
leaves a link that can't carry the mode to the enable path, as for a
DPMS on.

> doesn't this call nv50_audio_enable() (invoking nvif_outp_hda_eld() and
> notifying the ALSA audio component) on an untrained, detached SOR 
> whenever
> the connector's cached display_info.has_audio is true?

Yes, the same as in your review of patch 2, and I'm fixing in v2 the
same way.

Re: [PATCH 5/5] drm/nouveau/dp: restart an active link when the sink reasserts HPD

[David R. Hedges]

On 2026-11-03 14:36 -0600, sashiko-bot@kernel.org wrote:

> Could a successful link restart in nouveau_display_relink() leave
> connector->state->link_status stuck at DRM_MODE_LINK_STATUS_BAD when
> userspace processes the hotplug event?

It could, and I've left that as-is. The link-status documentation has
userspace set it back to GOOD, with the legacy SETCRTC path doing it
for non-atomic userspace, and no driver sets it to GOOD itself, i915
included. Userspace that reads link-status does one more modeset than
it needs to; nouveau doesn't act on the property. The v2 cover letter
I'm drafting mentions it. v2 will cancel a pending link_bad_work when
a later enable, such as the restart, trains the link.

> Would it avoid this race to update connector->state->link_status 
> directly
> while nouveau_display_relink() already holds 
> dev->mode_config.connection_mutex
> in ctx?

Yes, v2 will do that, and send the hotplug event from
nouveau_display_relink() once the locks are dropped. The work item is
only used from nv50_sor_atomic_enable() now, which can't take the
connection mutex, and a later enable that trains the link cancels it.

> Should nouveau_display_hpd_work() and nouveau_display_relink() verify 
> that
> both nv_connector->detected_encoder and connector->state->best_encoder 
> match
> nv_connector->dp_encoder before restarting the link?

Yes. v2 checks detected_encoder in the HPD worker and best_encoder in
nouveau_display_relink(), and leaves the other cases to userspace and
the hotplug event.

> Without setting nv_connector->dp_encoder = nv_encoder in 
> nv50_pior_create(),
> won't nv_connector->dp_encoder remain NULL on PIOR DP connectors, 
> causing
> both nouveau_dp_link_check() and this new HPD relink check in
> nouveau_display_hpd_work() to be skipped?

I'm not familiar with PIOR beyond some poking in the code--which
I now know is the external ANX9805 transmitter some NV50-family
boards use--and I have no such board to test it on, but I believe
the answer is yes. Before 633716501c94, the link check looked up any
DP encoder, including one on a PIOR, so my v2 draft sets dp_encoder
in nv50_pior_create() as well. nouveau doesn't get IRQ_HPD from that
chip, so what this enables there is the restart on HPD reassert.

> Should nouveau_display_fini() cancel conn->irq_work before
> cancel_work_sync(&drm->hpd_work) and cancel conn->link_bad_work after 
> it?

Yes for both. The irq_work part predates this series, so I'll add a
separate patch for it in v2, fixing 773eb04d14a1 ("drm/nouveau/disp:
expose conn event class"). f60849975a88 cancels it in
nouveau_connector_destroy(), but that's after drm_mode_config_cleanup()
has freed the encoders. link_bad_work is cancelled in the same loop,
since in v2 hpd_work no longer schedules it, and only on system
suspend and unload: its hotplug event can probe the connector, which
waits for runtime PM, and that's also why hpd_work isn't cancelled
on runtime suspend.

> Unlike nv50_msto_atomic_check(), which guards its check with
> !state->duplicated, won't nv50_outp_atomic_check() return -EINVAL here 
> and
> cause drm_atomic_helper_resume() to fail the entire multi-CRTC resume 
> commit?

Yes; good catch: drm_mode_config_reset() makes the restore a modeset,
and failing it leaves every output dark after resume. My draft for v2
doesn't fail a duplicated state, as nv50_msto_atomic_check() does, and
leaves a link that can't carry the mode to the enable path, as for a
DPMS on.

> doesn't this call nv50_audio_enable() (invoking nvif_outp_hda_eld() and
> notifying the ALSA audio component) on an untrained, detached SOR 
> whenever
> the connector's cached display_info.has_audio is true?

Yes, the same as in your review of patch 2, and I'm fixing in v2 the
same way.