Re: [PATCH v8 18/29] drm/msm/dp: add atomic stream handling for MST

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] drm/msm/dp: link and runtime PM leak in
  msm_dp_mst_stream_post_disable()
- [High] drm/msm/dp: MST payload state corruption on
  msm_dp_mst_stream_enable() error
- [High] drm/msm/dp: broken DPMS payload tracking in
  msm_dp_mst_stream_atomic_check()
- [Medium] drm/msm/dp: SST panel and connector queried in
  dpu_encoder_mst_atomic_enable()
- [Medium] drm/msm/dp: zero pbn_div in msm_dp_mst_stream_atomic_check()
  causes div-by-zero
--

commit 826c29c40882a969d4c35d32504417d642b6f995
Author: Yongxing Mou <yongxing.mou@oss.qualcomm.com>

drm/msm/dp: add atomic stream handling for MST

This commit adds atomic check, enable, and disable operations for DP MST
streams, reserving VC payload bandwidth and updating MST payloads across
stream state transitions.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 802cb62fc0102..edf30d7d3d169 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2745,16 +2745,27 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
>  static void dpu_encoder_mst_atomic_enable(struct drm_encoder *enc,
>  				      struct drm_atomic_commit *state)
>  {
> +	msm_dp_mst_stream_enable(enc, state);
>  	dpu_encoder_virt_atomic_enable(enc, state);
>  }

[Severity: Medium]
Does dpu_encoder_virt_atomic_enable() end up querying the SST panel and
connector instead of the active MST stream's panel and connector?

msm_dp_mst_stream_enable() configures the per-stream panel
(mst->mst_encoders[i].dp_panel) and binds it to the MST connector.
However, dpu_encoder_virt_atomic_enable() then calls
msm_dp_wide_bus_available() and msm_dp_needs_periph_flush(), which
inspect dp->panel and msm_dp_display->connector:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_needs_periph_flush() {
	return drm_mode_is_420_only(&msm_dp_display->connector->display_info, mode);
}

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_wide_bus_available() {
	...
	if (dp->panel->msm_dp_mode.out_fmt_is_yuv_420)
		return false;

	return dp->wide_bus_supported;
}

If a YUV420 mode is active on an MST stream (or was previously set on the
SST connector before switching to MST), the DPU encoder and DP controller
disagree on widebus enablement and peripheral flush requirements, breaking
display output.

[ ... ]

> diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> index ef5fb70863b81..eae82e857c849 100644
> --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
[ ... ]
> +void msm_dp_mst_stream_enable(struct drm_encoder *encoder,
> +			       struct drm_atomic_commit *state)
> +{
> +	struct drm_connector *connector =
> +		drm_atomic_get_new_connector_for_encoder(state, encoder);
> +	struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(connector);
> +	struct msm_dp_mst *mst = mst_conn->dp_mst;
> +	struct msm_dp *dp_display = mst->msm_dp;
> +	struct msm_dp_panel *panel = msm_dp_mst_panel_from_encoder(mst, encoder);
> +	struct drm_dp_mst_port *port = mst_conn->mst_port;
> +	struct drm_dp_mst_topology_state *mst_state =
> +		drm_atomic_get_new_mst_topology_state(state, &mst->mst_mgr);
> +	struct drm_dp_mst_atomic_payload *payload =
> +		drm_atomic_get_mst_payload_state(mst_state, port);
> +	struct msm_dp_mst_slot_info slots[DP_STREAM_MAX] = {};
> +	int rc;
> +
> +	guard(mutex)(&mst->mst_lock);
> +
> +	drm_connector_get(connector);
> +	panel->connector = connector;
> +
> +	rc = msm_dp_display_set_mode_helper(dp_display, state, encoder, panel);
> +	if (rc) {
> +		drm_err(dp_display->drm_dev,
> +			"[MST] stream:%u set_mode failed rc=%d\n", panel->stream_id, rc);
> +		return;
> +	}
> +
> +	rc = msm_dp_display_prepare_link(dp_display);
> +	if (rc) {
> +		drm_err(dp_display->drm_dev,
> +			"[MST] stream:%u prepare_link failed rc=%d\n", panel->stream_id, rc);
> +		return;
> +	}
> +
> +	drm_dp_mst_update_slots(mst_state, DP_CAP_ANSI_8B10B);
> +
> +	rc = drm_dp_add_payload_part1(&mst->mst_mgr, mst_state, payload);
> +	if (rc)
> +		return;
> +
> +	msm_dp_mst_collect_slots(mst, mst_state, slots);
> +
> +	drm_dbg_kms(dp_display->drm_dev,
> +		    "[MST] stream:%u timeslots vc_start:%d slots:%d pbn:%d\n",
> +		    panel->stream_id, payload->vc_start_slot,
> +		    payload->time_slots, payload->pbn);
> +
> +	rc = msm_dp_display_enable_helper(dp_display, panel, slots);
> +	if (rc)
> +		return;
> +
> +	drm_dp_check_act_status(&mst->mst_mgr);
> +
> +	drm_dp_add_payload_part2(&mst->mst_mgr, payload);
> +}
> +
> +void msm_dp_mst_stream_disable(struct drm_encoder *encoder,
> +			       struct drm_atomic_commit *state)
> +{
> +	struct drm_connector *connector = drm_atomic_get_old_connector_for_encoder(state, encoder);
> +	struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(connector);
> +	struct msm_dp_mst *mst = mst_conn->dp_mst;
> +	struct msm_dp_panel *panel = msm_dp_mst_panel_from_encoder(mst, encoder);
> +	struct drm_dp_mst_topology_state *old_mst_state =
> +		drm_atomic_get_old_mst_topology_state(state, &mst->mst_mgr);
> +	struct drm_dp_mst_topology_state *new_mst_state =
> +		drm_atomic_get_new_mst_topology_state(state, &mst->mst_mgr);
> +	struct drm_dp_mst_atomic_payload *old_payload =
> +		drm_atomic_get_mst_payload_state(old_mst_state, mst_conn->mst_port);
> +	struct drm_dp_mst_atomic_payload *new_payload =
> +		drm_atomic_get_mst_payload_state(new_mst_state, mst_conn->mst_port);
> +	struct msm_dp_mst_slot_info slots[DP_STREAM_MAX] = {};
> +
> +	guard(mutex)(&mst->mst_lock);
> +
> +	drm_dp_remove_payload_part1(&mst->mst_mgr, new_mst_state, new_payload);
> +
> +	drm_dp_remove_payload_part2(&mst->mst_mgr, new_mst_state, old_payload, new_payload);

[Severity: High]
If msm_dp_mst_stream_enable() returns early when
msm_dp_display_set_mode_helper() or msm_dp_display_prepare_link() fails,
will msm_dp_mst_stream_disable() corrupt the MST topology payload state?

When enable returns before drm_dp_add_payload_part1(), payload->vc_start_slot
stays at -1 and mst_mgr.payload_count is not incremented.  When the stream
is later disabled, drm_dp_remove_payload_part2() sees
new_payload->vc_start_slot == -1, subtracts old_payload->time_slots from
every active payload with vc_start_slot > -1, and underflows
mst_mgr.payload_count and mst_mgr.next_start_slot.

> +	msm_dp_mst_collect_slots(mst, new_mst_state, slots);
> +
> +	drm_dbg_kms(mst->msm_dp->drm_dev,
> +		    "[MST] stream:%u timeslots vc_start:%d slots:%d pbn:%d\n",
> +		    panel->stream_id, new_payload->vc_start_slot,
> +		    new_payload->time_slots, new_payload->pbn);
> +
> +	if (!msm_dp_display_is_plugged(mst->msm_dp))
> +		return;
> +
> +	msm_dp_display_disable_helper(mst->msm_dp, panel, slots);
> +
> +	drm_dp_check_act_status(&mst->mst_mgr);
> +}
> +
> +void msm_dp_mst_stream_post_disable(struct drm_encoder *encoder,
> +				    struct drm_atomic_commit *state)
> +{
> +	struct drm_connector *connector = drm_atomic_get_old_connector_for_encoder(state, encoder);
> +	struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(connector);
> +	struct msm_dp_mst *mst = mst_conn->dp_mst;
> +	struct msm_dp_panel *panel = msm_dp_mst_panel_from_encoder(mst, encoder);
> +
> +	guard(mutex)(&mst->mst_lock);
> +
> +	msm_dp_display_atomic_post_disable_helper(mst->msm_dp, panel);
> +
> +	if (!mst->msm_dp->mst_active)
> +		msm_dp_display_unprepare(mst->msm_dp);

[Severity: High]
Does gating msm_dp_display_unprepare() on !mst->msm_dp->mst_active leak the
link clocks, PHY power, and runtime PM reference when all MST streams are
disabled while the MST branch device remains connected?

mst_active stays true as long as the MST hub is plugged in, whereas
msm_dp_display_unprepare() already checks active_stream_mask before tearing
down the link:

drivers/gpu/drm/msm/dp/dp_display.c:msm_dp_display_unprepare() {
	...
	if (msm_dp_display->active_stream_mask) {
		drm_dbg_dp(dp->drm_dev, "stream still active, return\n");
		return;
	}
	...
}

Skipping msm_dp_display_unprepare() when the last stream is disabled leaves
the mainlink clocks on, leaks the runtime PM reference from
msm_dp_display_prepare_link(), and leaves link_ready set to true so later
connections skip link training.  msm_dp_display_unprepare() should be called
unconditionally here (and on error in msm_dp_mst_stream_enable() after
msm_dp_display_prepare_link() succeeds).

> +
> +	panel->connector = NULL;
> +
> +	drm_connector_put(connector);
> +}
> +
> +int msm_dp_mst_stream_atomic_check(struct drm_encoder *enc,
> +				   struct drm_crtc_state *crtc_state,
> +				   struct drm_connector_state *conn_state)
> +{
> +	struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(conn_state->connector);
> +	struct msm_dp_mst *mst = mst_conn->dp_mst;
> +	struct drm_dp_mst_topology_state *mst_state;
> +	int bpp, pbn, slots;
> +
> +	if (!conn_state->crtc)
> +		return 0;
> +
> +	if (!drm_atomic_crtc_needs_modeset(crtc_state) || !crtc_state->active)
> +		return 0;

[Severity: High]
Does returning early when !crtc_state->active break DRM MST payload tracking
across DPMS transitions and inactive modesets?

msm_dp_mst_connector_atomic_check() calls drm_dp_atomic_release_time_slots(),
which tracks payloads by connector-to-CRTC attachment (conn_state->crtc)
rather than crtc_state->active.  If a CRTC is enabled with active == false,
msm_dp_mst_stream_atomic_check() skips drm_dp_atomic_find_time_slots(), so a
later commit that detaches the connector or changes the mode hits
WARN_ON(!payload) in drm_dp_atomic_release_time_slots() and fails with
-EINVAL.  Conversely, transitioning an active MST CRTC to DPMS OFF
(enable == true, active == false) and then disabling the CRTC leaks the
port's malloc_kref reference because drm_dp_remove_payload_part2() runs when
new_payload->delete is still false and is not called again when the already
inactive CRTC is disabled.

> +
> +	bpp = (conn_state->connector->display_info.bpc * 3) ?: 24; /* fallback: assume 8bpc */
> +	pbn = drm_dp_calc_pbn_mode(crtc_state->mode.clock, bpp << 4);
> +
> +	mst_state = drm_atomic_get_mst_topology_state(crtc_state->state, &mst->mst_mgr);
> +	if (IS_ERR(mst_state))
> +		return PTR_ERR(mst_state);
> +
> +	if (!dfixed_trunc(mst_state->pbn_div)) {
> +		mst_state->pbn_div =
> +			drm_dp_get_vc_payload_bw(mst->link->link_caps.rate,
> +						 mst->link->link_caps.num_lanes);
> +	}
> +
> +	slots = drm_dp_atomic_find_time_slots(crtc_state->state, &mst->mst_mgr,
> +					      mst_conn->mst_port, pbn);

[Severity: Medium]
Can mst_state->pbn_div.full be zero here and cause a division-by-zero in
drm_dp_atomic_find_time_slots()?

msm_dp_panel_read_dpcd() zeroes mst->link->link_caps with memset() before
reading DPCD over AUX without synchronization against atomic_check:

drivers/gpu/drm/msm/dp/dp_panel.c:msm_dp_panel_read_dpcd() {
	...
	link_info = &link->link_caps;
	memset(link_info, 0, sizeof(*link_info));
	...
}

If an HPD event runs msm_dp_panel_read_dpcd() concurrently with
msm_dp_mst_stream_atomic_check() (or if the DPCD read fails and leaves
link_caps zeroed while an MST connector is still reachable),
drm_dp_get_vc_payload_bw() returns 0 and drm_dp_atomic_find_time_slots()
divides by topology_state->pbn_div.full == 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-msm-dp-mst-v8-0-d040fb33db5f@oss.qualcomm.com?part=18