[PATCH 0/3] drm/msm/dp: fix dead link-training fallback on Type-C boards

[PATCH 0/3] drm/msm/dp: fix dead link-training fallback on Type-C boards

[Jean-Francois Bobier]

These three patches come out of bringing up external DisplayPort over
USB-C on a OnePlus 8T (SM8250, msm DP driver). They are not
device-specific: all three are bugs in code paths that run on every
Type-C DP board using this driver, which today includes the Snapdragon
X Elite (x1e80100) laptops as well as phones.

Patch 1 matters most. The link-training rate/lane fallback in
msm_dp_ctrl_setup_main_link() is dead code on every Type-C board, because
its liveness check reads the DP controller's own HPD register, which
never reflects a real signal when HPD arrives out of band through a
drm_dp_hpd_bridge -- which is how every Type-C port gets its HPD. It is
latent: fallback only runs when training fails at full rate, so a good
cable never exercises it. Found with a cable whose lanes 2 and 3 never
achieve clock recovery at any rate.

Patch 2 demotes the per-attempt log lines in that same fallback loop
(plus one unrelated AUX interrupt message) from error to debug level.
Without it, a monitor that settles on two lanes after three attempts
prints nine error lines on a completely healthy attach.

Patch 3 sizes modes in msm_dp_display_mode_valid() against the link's
trained rate/lane count instead of the sink's advertised maximum, so a
fallback doesn't leave the mode list advertising bandwidth the link no
longer carries. It is a partial fix -- making userspace re-probe after a
fallback needs the DRM link-status property, which is the existing
// TODO in msm_dp_bridge_atomic_enable() and is not done here. Happy to
pick that up too if review wants the complete fix before this lands.

All three were found and verified on hardware with a cable that
reliably forces the fallback path (lanes 2 and 3 never complete clock
recovery). DPCD captures and the full fallback sequence are available on
request.

Verified against msm-next as of 5e4a3f7b2 ("drm/msm/dp: add
stream-aware link register accessors"), individually and applied
together in sequence.

Jean-Francois Bobier (3):
  drm/msm/dp: don't gate link-training fallback on the controller's HPD
    block
  drm/msm/dp: don't log an expected link-training fallback as errors
  drm/msm/dp: validate modes against the trained link, not the sink's
    claim

 drivers/gpu/drm/msm/dp/dp_aux.c     |  9 ++++++-
 drivers/gpu/drm/msm/dp/dp_ctrl.c    | 39 +++++++++++++++++++----------
 drivers/gpu/drm/msm/dp/dp_display.c | 38 ++++++++++++++++++++++++++--
 3 files changed, 70 insertions(+), 16 deletions(-)

-- 
2.55.0

[PATCH 1/3] drm/msm/dp: don't gate link-training fallback on the controller's HPD block

[Jean-Francois Bobier]

msm_dp_ctrl_setup_main_link()'s retry loop is what implements DP link
fallback: on a training failure it shifts the link rate down HBR3 -> HBR2
-> HBR -> RBR and then, if clock recovery succeeded on at least half the
lanes, reduces the lane count. Both failure branches guard that with

	if (!msm_dp_aux_is_link_connected(ctrl->aux))
		break;

which reads REG_DP_DP_HPD_INT_STATUS, i.e. the DP controller's own HPD
block. That is only meaningful when the block is wired to a real HPD
signal.

On a USB-C port it is not. HPD arrives out of band: the Type-C port
driver owns it and hands it over through a drm_dp_hpd_bridge, and the
controller's HPD register reads DISCONNECTED for the entire time a sink
is attached. msm_dp_bridge_hpd_notify() already shows this, logging the
register beside the bridge's own verdict -- on sm8250 (OnePlus 8T) every
attach reports

	msm_dp_bridge_hpd_notify: type=10 link hpd_link_status=0x0, status=1

So the guard is always true, the loop always breaks on the first failed
attempt, and the rate/lane fallback is dead on every Type-C DP board.
msm_dp_ctrl_setup_main_link() then returns -ECONNRESET and the mode set
fails outright.

The cost is real. With a 4K sink attached directly (4 DP lanes, no USB3),
a board that can only carry two DP lanes to the connector trains lanes 0
and 1 and never lanes 2 and 3:

	v_level 3: DPCD 0x202 = 0x11 (lanes 0,1 CR_DONE)
	           DPCD 0x203 = 0x00 (lanes 2,3 nothing)
	link training #1 on phy 0 failed. ret=-11
	Failed link training (rc=-104)

A fallback to two lanes would have trained -- clock recovery was already
good on exactly the lanes that remain, so
msm_dp_ctrl_clock_recovery_any_ok() would have passed -- but it is never
reached. The same sink works through a dock, because the Type-C alternate
mode negotiates two DP lanes up front and the first attempt succeeds.

Use the DPCD read the loop already performs as the liveness test instead.
It expresses the intent directly -- a sink that has gone away cannot
answer AUX -- and it carries no assumption about where HPD comes from.
drm_dp_dpcd_read_link_status() returns 0 on success and a negative errno
on failure.

Signed-off-by: Jean-Francois Bobier <jean-francois.bobier@laposte.net>
---
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index e1614bc49..d6219d87d 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -2426,12 +2426,22 @@ int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl,
 			/* training completed successfully */
 			break;
 		} else if (training_step == DP_TRAINING_1) {
-			/* link train_1 failed */
-			if (!msm_dp_aux_is_link_connected(ctrl->aux))
+			/*
+			 * link train_1 failed
+			 *
+			 * Use the DPCD read itself to decide whether the sink
+			 * is still there. msm_dp_aux_is_link_connected() reads
+			 * the DP controller's own HPD block, which is only
+			 * meaningful when that block sees a real HPD signal --
+			 * on a Type-C port HPD arrives out of band through a
+			 * drm_dp_hpd_bridge and the register reads
+			 * DISCONNECTED the whole time the sink is attached.
+			 * Gating the retry on it therefore skipped the entire
+			 * rate/lane fallback on those boards.
+			 */
+			if (drm_dp_dpcd_read_link_status(ctrl->aux, link_status) < 0)
 				break;
 
-			drm_dp_dpcd_read_link_status(ctrl->aux, link_status);
-
 			rc = msm_dp_ctrl_link_rate_down_shift(ctrl);
 			if (rc < 0) { /* already in RBR = 1.6G */
 				if (msm_dp_ctrl_clock_recovery_any_ok(link_status,
@@ -2451,12 +2461,10 @@ int msm_dp_ctrl_on_link(struct msm_dp_ctrl *msm_dp_ctrl,
 				}
 			}
 		} else if (training_step == DP_TRAINING_2) {
-			/* link train_2 failed */
-			if (!msm_dp_aux_is_link_connected(ctrl->aux))
+			/* link train_2 failed; same reasoning as above */
+			if (drm_dp_dpcd_read_link_status(ctrl->aux, link_status) < 0)
 				break;
 
-			drm_dp_dpcd_read_link_status(ctrl->aux, link_status);
-
 			if (!drm_dp_clock_recovery_ok(link_status,
 					ctrl->link->link_params.num_lanes))
 				rc = msm_dp_ctrl_link_rate_down_shift(ctrl);
-- 
2.55.0

[PATCH 2/3] drm/msm/dp: don't log an expected link-training fallback as errors

[Jean-Francois Bobier]

msm_dp_ctrl_on_link() implements DP link fallback: it retries
msm_dp_ctrl_setup_main_link() up to five times, stepping the link rate
down HBR3 -> HBR2 -> HBR -> RBR and then reducing the lane count, and
breaks as soon as training succeeds. Every step of that search is
therefore expected to fail before the one that works.

Each failed attempt currently logs at error level, from four places:

  max v_level reached
  link training #1 on phy %d failed. ret=%d
  link training #2 on phy %d failed. ret=%d
  link training on sink failed. ret=%d

A monitor that settles on two lanes after three attempts prints nine
error lines on a completely successful attach -- which is how a cable
that merely has two bad lanes looks identical in the log to a sink that
never trains at all.

Nothing is lost by demoting them. When the retry loop really does
exhaust itself, msm_dp_ctrl_on_link() returns the error and
msm_dp_display_process_hpd_high() already reports it once, at error
level, as "Failed link training (rc=%d)" -- which is the correct place,
because it is the only one that knows the search is over.

Demote the per-attempt messages to drm_dbg_dp() so they remain available
with drm.debug, and keep the final verdict where it is.

Also demote the unsolicited AUX interrupt message. msm_dp_aux_isr()
returns IRQ_NONE when no AUX transaction is outstanding, which is the
whole of the correct response for an interrupt that is not ours -- the
AUX block shares its interrupt and sinks raise AUX status asynchronously
around hotplug, so this fires twice on a normal DisplayPort attach.

Signed-off-by: Jean-Francois Bobier <jean-francois.bobier@laposte.net>
---
 drivers/gpu/drm/msm/dp/dp_aux.c  |  9 ++++++++-
 drivers/gpu/drm/msm/dp/dp_ctrl.c | 15 ++++++++++-----
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 3825a2fb4..2c0bc719a 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -458,7 +458,14 @@ irqreturn_t msm_dp_aux_isr(struct drm_dp_aux *msm_dp_aux, u32 isr)
 	aux = container_of(msm_dp_aux, struct msm_dp_aux_private, msm_dp_aux);
 
 	if (!aux->cmd_busy) {
-		DRM_ERROR("Unexpected DP AUX IRQ %#010x when not busy\n", isr);
+		/*
+		 * Not ours: the AUX block shares its interrupt, and sinks
+		 * raise AUX status asynchronously around hotplug. Returning
+		 * IRQ_NONE is the whole of the correct response, so this is
+		 * a trace point and not a fault.
+		 */
+		drm_dbg_dp(msm_dp_aux->drm_dev,
+			   "Unexpected DP AUX IRQ %#010x when not busy\n", isr);
 		return IRQ_NONE;
 	}
 
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index d6219d87d..ccc6cf63d 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1534,7 +1534,8 @@ static int msm_dp_ctrl_link_train_1(struct msm_dp_ctrl_private *ctrl,
 
 		if (ctrl->link->phy_params.v_level >=
 			DP_TRAIN_LEVEL_MAX) {
-			DRM_ERROR_RATELIMITED("max v_level reached\n");
+			drm_dbg_dp(ctrl->drm_dev,
+				   "max v_level reached, retrying at a lower rate or lane count\n");
 			return -EAGAIN;
 		}
 
@@ -1681,14 +1682,16 @@ static int msm_dp_ctrl_link_train_1_2(struct msm_dp_ctrl_private *ctrl,
 
 	ret = msm_dp_ctrl_link_train_1(ctrl, panel, training_step, dp_phy);
 	if (ret) {
-		DRM_ERROR("link training #1 on phy %d failed. ret=%d\n", dp_phy, ret);
+		drm_dbg_dp(ctrl->drm_dev,
+			   "link training #1 on phy %d failed. ret=%d\n", dp_phy, ret);
 		return ret;
 	}
 	drm_dbg_dp(ctrl->drm_dev, "link training #1 on phy %d successful\n", dp_phy);
 
 	ret = msm_dp_ctrl_link_train_2(ctrl, panel, training_step, dp_phy);
 	if (ret) {
-		DRM_ERROR("link training #2 on phy %d failed. ret=%d\n", dp_phy, ret);
+		drm_dbg_dp(ctrl->drm_dev,
+			   "link training #2 on phy %d failed. ret=%d\n", dp_phy, ret);
 		return ret;
 	}
 	drm_dbg_dp(ctrl->drm_dev, "link training #2 on phy %d successful\n", dp_phy);
@@ -1739,13 +1742,15 @@ static int msm_dp_ctrl_link_train(struct msm_dp_ctrl_private *ctrl,
 	}
 
 	if (ret) {
-		DRM_ERROR("link training of LTTPR(s) failed. ret=%d\n", ret);
+		drm_dbg_dp(ctrl->drm_dev,
+			   "link training of LTTPR(s) failed. ret=%d\n", ret);
 		goto end;
 	}
 
 	ret = msm_dp_ctrl_link_train_1_2(ctrl, panel, training_step, DP_PHY_DPRX);
 	if (ret) {
-		DRM_ERROR("link training on sink failed. ret=%d\n", ret);
+		drm_dbg_dp(ctrl->drm_dev,
+			   "link training on sink failed. ret=%d\n", ret);
 		goto end;
 	}
 
-- 
2.55.0

[PATCH 3/3] drm/msm/dp: validate modes against the trained link, not the sink's claim

[Jean-Francois Bobier]

msm_dp_display_mode_valid() sizes every mode against
panel->link_info, which is what the sink advertises in DPCD. That is not
necessarily what the link carries. msm_dp_ctrl_on_link() walks the rate
down HBR3 -> HBR2 -> HBR -> RBR and then reduces the lane count until
training succeeds, so a cable with two unusable lanes trains at half the
bandwidth the sink claims.

The validator does not know that, so it keeps advertising modes sized for
the sink's maximum. Userspace picks one, training "succeeds" at the
reduced parameters, the mode does not fit in them, and the sink shows a
black screen -- with nothing in the log to explain it, because from the
driver's point of view everything worked.

Observed on an sm8250 phone with a 4K monitor whose cable only carries
two lanes: the connector offers 3840x2160, the link trains at 2 lanes
HBR2 (8.6 Gbps after 8b/10b against the 12.5 Gbps the mode needs at 24
bpp, more at 30), and the screen stays black until the user picks
2560x1440 by hand.

link->link_params holds the trained values and is reset to the sink's
maximum at the top of every msm_dp_ctrl_on_link(), so taking the lower of
the two is accurate after a fallback and a no-op before the first
training. The change can only ever remove modes from the list, never add
one, so it cannot cause the link to attempt anything it was not already
attempting.

This does not by itself make userspace re-probe after a fallback -- that
needs the link-status property, which cannot be set from
msm_dp_bridge_atomic_enable() because the modeset locks are held, and is
the deferred work the existing TODO there refers to. What it does ensure
is that any probe after a fallback offers only modes the link can
actually drive.

Also log the rejection at debug level; a mode silently disappearing from
the list is otherwise hard to account for.

Signed-off-by: Jean-Francois Bobier <jean-francois.bobier@laposte.net>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 38 +++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bce641d7d..634d825aa 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -750,8 +750,10 @@ enum drm_mode_status msm_dp_display_mode_valid(struct msm_dp *dp,
 {
 	const u32 num_components = 3, default_bpp = 24;
 	struct msm_dp_display_private *msm_dp_display;
+	const struct msm_dp_link_info *trained;
 	struct msm_dp_link_info *link_info;
 	u32 mode_rate_khz = 0, supported_rate_khz = 0, mode_bpp = 0;
+	unsigned int num_lanes, rate;
 	int mode_pclk_khz = mode->clock;
 	int link_pclk_khz;
 	bool is_yuv_420;
@@ -789,10 +791,42 @@ enum drm_mode_status msm_dp_display_mode_valid(struct msm_dp *dp,
 			mode_bpp, link_pclk_khz);
 
 	mode_rate_khz = link_pclk_khz * mode_bpp;
-	supported_rate_khz = link_info->num_lanes * link_info->rate * 8;
 
-	if (mode_rate_khz > supported_rate_khz)
+	/*
+	 * link_info is what the sink advertises in DPCD, which is not
+	 * necessarily what we managed to train. msm_dp_ctrl_on_link() walks
+	 * the rate down HBR3 -> HBR2 -> HBR -> RBR and then halves the lane
+	 * count until training succeeds, so a cable with two unusable lanes
+	 * ends up carrying half the bandwidth the sink claims.
+	 *
+	 * Sizing modes against the sink's maximum then advertises modes the
+	 * link cannot carry: training "succeeds" at the reduced parameters,
+	 * the mode does not fit in them, and the sink shows a black screen
+	 * with nothing in the log to say why.
+	 *
+	 * link->link_params holds the trained values, and is reset to the
+	 * sink's maximum at the top of every msm_dp_ctrl_on_link(), so taking
+	 * the lower of the two is accurate after a fallback and a no-op
+	 * before the first training.
+	 */
+	num_lanes = link_info->num_lanes;
+	rate = link_info->rate;
+
+	trained = &msm_dp_display->link->link_params;
+	if (trained->num_lanes && trained->num_lanes < num_lanes)
+		num_lanes = trained->num_lanes;
+	if (trained->rate && trained->rate < rate)
+		rate = trained->rate;
+
+	supported_rate_khz = num_lanes * rate * 8;
+
+	if (mode_rate_khz > supported_rate_khz) {
+		drm_dbg_dp(dp->drm_dev,
+			   "mode %s rejected: needs %u kHz, link carries %u kHz (%u lanes @ %u)\n",
+			   mode->name, mode_rate_khz, supported_rate_khz,
+			   num_lanes, rate);
 		return MODE_BAD;
+	}
 
 	return MODE_OK;
 }
-- 
2.55.0

Re: [PATCH 3/3] drm/msm/dp: validate modes against the trained link, not the sink's claim

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm/dp: stale link training parameters artificially restrict modes after hotplug
- [Medium] drm/msm/dp: data race and TOCTOU in mode validation reading link_params
--

commit 69ad059161eff4e2e43af0ffa4e037a24d8b918c
Author: Jean-Francois Bobier <jean-francois.bobier@laposte.net>

drm/msm/dp: validate modes against the trained link, not the sink's claim

This commit updates msm_dp_display_mode_valid() to validate modes against
the trained link parameters rather than the sink's advertised capabilities.
This prevents advertising modes that the link cannot carry after a training
fallback.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 6abe30bdff120..d1e111c6a5fb8 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -793,10 +795,42 @@ enum drm_mode_status msm_dp_display_mode_valid(struct msm_dp *dp,
>  			mode_bpp, link_pclk_khz);
>  
>  	mode_rate_khz = link_pclk_khz * mode_bpp;
> -	supported_rate_khz = link_info->num_lanes * link_info->rate * 8;
>  
> -	if (mode_rate_khz > supported_rate_khz)
> +	/*
> +	 * link_info is what the sink advertises in DPCD, which is not
> +	 * necessarily what we managed to train. msm_dp_ctrl_on_link() walks
> +	 * the rate down HBR3 -> HBR2 -> HBR -> RBR and then halves the lane
> +	 * count until training succeeds, so a cable with two unusable lanes
> +	 * ends up carrying half the bandwidth the sink claims.
> +	 *
> +	 * Sizing modes against the sink's maximum then advertises modes the
> +	 * link cannot carry: training "succeeds" at the reduced parameters,
> +	 * the mode does not fit in them, and the sink shows a black screen
> +	 * with nothing in the log to say why.
> +	 *
> +	 * link->link_params holds the trained values, and is reset to the
> +	 * sink's maximum at the top of every msm_dp_ctrl_on_link(), so taking
> +	 * the lower of the two is accurate after a fallback and a no-op
> +	 * before the first training.
> +	 */
> +	num_lanes = link_info->num_lanes;
> +	rate = link_info->rate;
> +
> +	trained = &msm_dp_display->link->link_params;
> +	if (trained->num_lanes && trained->num_lanes < num_lanes)
> +		num_lanes = trained->num_lanes;
> +	if (trained->rate && trained->rate < rate)
> +		rate = trained->rate;

[Severity: High]
Will this improperly restrict modes for new monitors after a hotplug?

When a monitor is unplugged, msm_dp_hpd_unplug_handle() and
msm_dp_link_reset_data() do not clear or invalidate link_params.

If the previous monitor fell back to a lower lane count or rate, does this
mean the new monitor's modes will be evaluated against the old monitor's
degraded parameters?

[Severity: Medium]
Is it safe to read trained->num_lanes and trained->rate locklessly here?

During an asynchronous non-blocking modeset, msm_dp_ctrl_on_link() can
modify these values concurrently:

drivers/gpu/drm/msm/dp/dp_ctrl.c:msm_dp_ctrl_on_link() {
    ...
		ctrl->link->link_params.rate = rate;
		ctrl->link->link_params.num_lanes = panel->link_info.num_lanes;
    ...
}

Since userspace can concurrently read the EDID and trigger mode validation,
could the lack of locking or READ_ONCE() here lead to a data race or a TOCTOU
issue where the condition check and subsequent assignment see different
values?

> +
> +	supported_rate_khz = num_lanes * rate * 8;
> +
> +	if (mode_rate_khz > supported_rate_khz) {
> +		drm_dbg_dp(dp->drm_dev,
> +			   "mode %s rejected: needs %u kHz, link carries %u kHz (%u lanes @ %u)\n",
> +			   mode->name, mode_rate_khz, supported_rate_khz,
> +			   num_lanes, rate);
>  		return MODE_BAD;
> +	}
>  
>  	return MODE_OK;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005131432.22840-1-jean-francois.bobier@laposte.net?part=3

[PATCH v2 3/3] drm/msm/dp: validate modes against the trained link, not the sink's claim

[Jean-Francois Bobier]

msm_dp_display_mode_valid() sizes every mode against
panel->link_info, which is what the sink advertises in DPCD. That is not
necessarily what the link carries. msm_dp_ctrl_on_link() walks the rate
down HBR3 -> HBR2 -> HBR -> RBR and then reduces the lane count until
training succeeds, so a cable with two unusable lanes trains at half the
bandwidth the sink claims.

The validator does not know that, so it keeps advertising modes sized for
the sink's maximum. Userspace picks one, training "succeeds" at the
reduced parameters, the mode does not fit in them, and the sink shows a
black screen -- with nothing in the log to explain it, because from the
driver's point of view everything worked.

Observed on an sm8250 phone with a 4K monitor whose cable only carries
two lanes: the connector offers 3840x2160, the link trains at 2 lanes
HBR2 (8.6 Gbps after 8b/10b against the 12.5 Gbps the mode needs at 24
bpp, more at 30), and the screen stays black until the user picks
2560x1440 by hand.

link->link_params holds the trained values, but is only meaningful for
the sink that produced them. msm_dp_hpd_unplug_handle() now clears it on
disconnect, and msm_dp_ctrl_on_link() resets it to the new sink's own
maximum before training, so taking the lower of link_params and
link_info is accurate once a sink has been trained and a safe no-op
before that -- never the previous sink's leftover values.

This does not by itself make userspace re-probe after a fallback -- that
needs the link-status property, which cannot be set from
msm_dp_bridge_atomic_enable() because the modeset locks are held, and is
the deferred work the existing TODO there refers to. What it does ensure
is that any probe after a fallback offers only modes the link can
actually drive.

Also log the rejection at debug level; a mode silently disappearing from
the list is otherwise hard to account for.

Signed-off-by: Jean-Francois Bobier <jean-francois.bobier@laposte.net>
---
Changes in v2:
- Clear link_params on disconnect in msm_dp_hpd_unplug_handle(), and
  updated the comment in mode_valid() to say so. v1 reset link_params to
  the sink's maximum at the start of training, but never on disconnect,
  so mode_valid() for a freshly plugged sink -- which runs during EDID
  read, before that sink's own link has ever been trained -- could size
  its modes against whatever the *previous* sink's link had trained
  down to. The num_lanes/rate == 0 guard already in v1's mode_valid()
  now does what it was presumably meant to do from the start: treat
  "not yet trained for this sink" as "don't restrict," rather than
  "not yet trained since driver load."
- Caught by this list's automated review on v1, not by hand -- credit
  where it's due.

 drivers/gpu/drm/msm/dp/dp_display.c | 53 +++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bce641d7d..d0613ef98 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -460,6 +460,19 @@ static int msm_dp_hpd_unplug_handle(struct msm_dp_display_private *dp)
 
 	dp->panel->video_test = false;
 
+	/*
+	 * Clear the trained link parameters along with everything else this
+	 * function already resets on disconnect. msm_dp_ctrl_on_link() does
+	 * not run again until a mode is actually committed for the next
+	 * sink, but msm_dp_display_mode_valid() reads link_params to size
+	 * the mode list as soon as the next sink's EDID is read -- well
+	 * before that training happens. Leaving the previous sink's trained
+	 * values in place would size the new sink's modes against a link it
+	 * has never been on.
+	 */
+	dp->link->link_params.num_lanes = 0;
+	dp->link->link_params.rate = 0;
+
 	msm_dp_aux_enable_xfers(dp->aux, false);
 
 	drm_dbg_dp(dp->drm_dev, "Before, type=%d sink_count=%d\n",
@@ -750,8 +763,10 @@ enum drm_mode_status msm_dp_display_mode_valid(struct msm_dp *dp,
 {
 	const u32 num_components = 3, default_bpp = 24;
 	struct msm_dp_display_private *msm_dp_display;
+	const struct msm_dp_link_info *trained;
 	struct msm_dp_link_info *link_info;
 	u32 mode_rate_khz = 0, supported_rate_khz = 0, mode_bpp = 0;
+	unsigned int num_lanes, rate;
 	int mode_pclk_khz = mode->clock;
 	int link_pclk_khz;
 	bool is_yuv_420;
@@ -789,10 +804,44 @@ enum drm_mode_status msm_dp_display_mode_valid(struct msm_dp *dp,
 			mode_bpp, link_pclk_khz);
 
 	mode_rate_khz = link_pclk_khz * mode_bpp;
-	supported_rate_khz = link_info->num_lanes * link_info->rate * 8;
 
-	if (mode_rate_khz > supported_rate_khz)
+	/*
+	 * link_info is what the sink advertises in DPCD, which is not
+	 * necessarily what we managed to train. msm_dp_ctrl_on_link() walks
+	 * the rate down HBR3 -> HBR2 -> HBR -> RBR and then halves the lane
+	 * count until training succeeds, so a cable with two unusable lanes
+	 * ends up carrying half the bandwidth the sink claims.
+	 *
+	 * Sizing modes against the sink's maximum then advertises modes the
+	 * link cannot carry: training "succeeds" at the reduced parameters,
+	 * the mode does not fit in them, and the sink shows a black screen
+	 * with nothing in the log to say why.
+	 *
+	 * link->link_params holds the trained values. It is reset to the
+	 * sink's maximum at the top of every msm_dp_ctrl_on_link(), and
+	 * cleared to zero by msm_dp_hpd_unplug_handle() on disconnect, so
+	 * taking the lower of the two is accurate once this sink has been
+	 * trained, a safe no-op before that, and never a leftover from
+	 * whatever sink was attached previously.
+	 */
+	num_lanes = link_info->num_lanes;
+	rate = link_info->rate;
+
+	trained = &msm_dp_display->link->link_params;
+	if (trained->num_lanes && trained->num_lanes < num_lanes)
+		num_lanes = trained->num_lanes;
+	if (trained->rate && trained->rate < rate)
+		rate = trained->rate;
+
+	supported_rate_khz = num_lanes * rate * 8;
+
+	if (mode_rate_khz > supported_rate_khz) {
+		drm_dbg_dp(dp->drm_dev,
+			   "mode %s rejected: needs %u kHz, link carries %u kHz (%u lanes @ %u)\n",
+			   mode->name, mode_rate_khz, supported_rate_khz,
+			   num_lanes, rate);
 		return MODE_BAD;
+	}
 
 	return MODE_OK;
 }
-- 
2.56.0