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 <[email protected]> --- 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
