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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Acquiring a sleeping mutex lock and calling sleeping polling functions inside the encoder `atomic_enable` and `atomic_disable` callbacks causes sleep-in-atomic bugs.
- [High] Premature call to `drm_dp_remove_payload_part2()` before hardware update and ACT check in `msm_dp_mst_stream_disable()`.
- [High] Early error returns in `msm_dp_mst_stream_enable()` corrupt internal stream tracking and MST manager state.
--

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

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

This patch adds atomic check, enable, and disable operations for MST
streams in the MSM DP driver. It reserves VC payload bandwidth, updates
payloads when toggling streams, and uses a mutex to serialize link state.

> diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> index 93536b6936cbd..03f9c95ca6860 100644
> --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
[ ... ]
> @@ -145,6 +160,152 @@ static int msm_dp_mst_encoder_stream_id(struct msm_dp_mst *mst,
>  	return -1;
>  }
>  
> +void msm_dp_mst_stream_enable(struct drm_encoder *encoder,
> +			      struct drm_atomic_commit *state)
> +{
[ ... ]
> +	struct drm_dp_mst_atomic_payload *payload =
> +		drm_atomic_get_mst_payload_state(mst_state, port);
> +	int rc;
> +
> +	guard(mutex)(&mst->mst_lock);

[Severity: High]
Does this acquire a sleeping mutex from an atomic context?

The DRM subsystem guidelines designate encoder atomic_enable and
atomic_disable callbacks as executing in an atomic context where sleeping
is strictly forbidden. A userspace modeset targeting an MST stream will
invoke msm_dp_mst_stream_enable() via dpu_encoder_mst_atomic_enable().

Can this lead to kernel warnings, priority inversion, and potential
system deadlocks?

> +
> +	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);
> +		msm_dp_display_unprepare(dp_display);
> +		return;
> +	}

[Severity: High]
Are these early returns going to cause state corruption when the disable
path runs?

If msm_dp_display_prepare_link() fails, msm_dp_mst_stream_enable() returns
early before completing payload allocation. Since the atomic_enable
callback returns void, the DRM core still registers the encoder as active.

When atomic_disable is later called, msm_dp_mst_stream_disable() will
unconditionally decrement payload counts that were never incremented.
Could this lead to integer underflows in the MST topology manager state?

> +
> +	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;

[Severity: High]
Will this early return also cause similar underflow issues in the MST
manager state if drm_dp_add_payload_part1() fails? The atomic_disable
path will still run later and unconditionally decrement the payload count.

> +
> +	msm_dp_display_set_stream_info(mst->msm_dp, panel,
> +				       payload->vc_start_slot,
> +				       payload->time_slots, payload->pbn);
> +	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);
> +
> +	msm_dp_display_enable_helper(dp_display, panel);
> +
> +	drm_dp_check_act_status(&mst->mst_mgr);

[Severity: High]
Is it safe to call drm_dp_check_act_status() from within the atomic_enable
path?

This is a hardware polling function that can sleep for up to 3 seconds.
Since msm_dp_mst_stream_enable() runs in an atomic context, will this
cause sleep-in-atomic bugs?

> +
> +	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_dp_mst_atomic_payload *new_payload =
> +		drm_atomic_get_mst_payload_state(new_mst_state, mst_conn->mst_port);
> +
> +	guard(mutex)(&mst->mst_lock);

[Severity: High]
Does this acquire a sleeping mutex from an atomic context?

Similar to the enable path, msm_dp_mst_stream_disable() is called during
encoder atomic_disable callbacks, which strictly forbid sleeping.

> +
> +	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]
Is this call to drm_dp_remove_payload_part2() premature?

This API call frees the VCPI slots in the software topology manager before
msm_dp_display_disable_helper() executes and the Allocation Change Trigger
is acknowledged by the sink.

Could this allow concurrent atomic commits to reallocate the same slots
while the hardware is still executing the removal sequence, leading to
topological corruption?

> +
> +	msm_dp_display_set_stream_info(mst->msm_dp, panel, 0, 0, 0);
> +	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);
> +
> +	msm_dp_display_disable_helper(mst->msm_dp, panel);
> +
> +	drm_dp_check_act_status(&mst->mst_mgr);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-msm-dp-mst-v6-0-c91d35d6fb9e@oss.qualcomm.com?part=23