Re: [PATCH v7 19/35] drm/msm/dp: add MST connector creation and topology callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/msm/dp: atomic state corruption in msm_dp_mst_atomic_best_encoder()
- [High] drm/msm/dp: pm_runtime misuse on wrong device in msm_dp_mst_connector_detect()
--

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

drm/msm/dp: add MST connector creation and topology callbacks

This commit registers the add_connector topology callback with the DRM MST
manager to dynamically create DRM connectors for MST ports. It also implements
the necessary connector operations and DRM MST callbacks to expose MST sink
ports.

> diff --git a/drivers/gpu/drm/msm/dp/dp_mst_drm.c b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> index 5d22aa34c521c..169b34f985c0e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_mst_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_mst_drm.c
[ ... ]
> +static int
> +msm_dp_mst_connector_detect(struct drm_connector *connector,
> +			    struct drm_modeset_acquire_ctx *ctx,
> +			    bool force)
> +{
> +	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 device *dev = dp_display->drm_dev->dev;

[Severity: High]
Does this use the correct device for PM runtime?

By using the top-level DRM device (dp_display->drm_dev->dev), it appears the
detect function acquires PM references for the MDSS parent instead of the DP
controller (dp_display->pdev->dev).

> +	enum drm_connector_status status = connector_status_disconnected;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);

[Severity: High]
Can this cause excessive PHY teardowns during detection?

Because the DP controller's PM is not explicitly held open, each underlying
AUX transfer during drm_dp_mst_detect_port() will individually resume and
synchronously suspend the DP controller (via msm_dp_aux_transfer()).

Could this tear down and reinitialize the DP PHY for every single AUX message,
causing overhead and risking link training or topology discovery failures?

> +	if (ret < 0)
> +		return status;
> +
> +	if (dp_display->mst_active)
> +		status = drm_dp_mst_detect_port(connector,
> +						ctx, &mst->mst_mgr, mst_conn->mst_port);
> +
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return status;
> +}
[ ... ]
> +static struct drm_encoder *
> +msm_dp_mst_atomic_best_encoder(struct drm_connector *connector, struct drm_atomic_commit *state)
> +{
> +	struct msm_dp_mst_connector *mst_conn = to_dp_mst_connector(connector);
> +	struct msm_dp_mst *mst = mst_conn->dp_mst;
> +	struct drm_connector_state *conn_state;
> +	struct drm_connector *iter;
> +	struct drm_connector_list_iter conn_iter;
> +	u32 stream_mask = 0;
> +	u32 i;
> +
> +	conn_state = drm_atomic_get_new_connector_state(state, connector);
> +	if (!conn_state)
> +		return NULL;
> +
> +	if (conn_state->best_encoder)
> +		return conn_state->best_encoder;
> +
> +	drm_connector_list_iter_begin(connector->dev, &conn_iter);
> +	drm_for_each_connector_iter(iter, &conn_iter) {
> +		struct drm_connector_state *peer_state;
> +		int stream_id;
> +
> +		if (iter == connector ||
> +		    iter->funcs != &msm_dp_drm_mst_connector_funcs ||
> +		    to_dp_mst_connector(iter)->dp_mst != mst)
> +			continue;
> +
> +		peer_state = drm_atomic_get_new_connector_state(state, iter) ?: iter->state;
> +		if (!peer_state || !peer_state->crtc || !peer_state->best_encoder)
> +			continue;
> +
> +		stream_id = msm_dp_mst_encoder_stream_id(mst, peer_state->best_encoder);
> +		if (stream_id >= 0 && stream_id < mst->max_streams)
> +			stream_mask |= BIT(stream_id);
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
> +	for (i = 0; i < mst->max_streams; i++) {
> +		if (!(stream_mask & BIT(i))) {
> +			conn_state->best_encoder = mst->mst_encoders[i].enc;

[Severity: High]
Does directly modifying the atomic state here hide the routing change from the
DRM core?

By overwriting conn_state->best_encoder, the update_connector_routing() check
if (new_encoder == new_connector_state->best_encoder) evaluates to true and
returns early.

This causes steal_encoder() to be skipped, leaving the old CRTC's
encoder_mask with the old encoder, and connectors_changed is never set on
the new CRTC. Could this leave the DRM core's atomic state tracking
corrupted and cause hardware conflicts where multiple connectors claim the
same encoder?

> +			return mst->mst_encoders[i].enc;
> +		}
> +	}
> +
> +	return NULL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-msm-dp-mst-v7-0-4b81473185e7@oss.qualcomm.com?part=19