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 <[email protected]> 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/[email protected]?part=19
