[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@xxxxxxxxxxx>
---
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