Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm: connection_mutex not held during atomic_enable/disable DP sink helpers - [Medium] drm: missed DP link parameter reporting on initial hotplug --
commit db3d1199eb9fddde4ac0ef014ef0885a214a3723 Author: Kory Maincent <[email protected]> drm/mediatek: Use dp_connector helpers to expose link training state Adopt the DRM core DisplayPort helpers by setting DRM_BRIDGE_OP_DP and populating the bridge dp_link_caps at probe time. Report the sink capabilities on hotplug, reset them on disconnect, and report the link parameters once link training succeeds. > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c > b/drivers/gpu/drm/mediatek/mtk_dp.c > index f656b85b84212..2e2596eaa342e 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dp.c > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c [ ... ] > @@ -1812,6 +1812,35 @@ static void mtk_dp_train_change_mode(struct mtk_dp > *mtk_dp) > mtk_dp_reset_swing_pre_emphasis(mtk_dp); > } > > +static void _mtk_dp_report_link_train(struct mtk_dp *mtk_dp) > +{ > + u32 rate, nlanes; > + bool dsc = false; > + > + rate = drm_dp_bw_code_to_link_rate(mtk_dp->train_info.link_rate); > + nlanes = mtk_dp->train_info.lane_count; > + > + drm_dp_set_cur_link_params(mtk_dp->conn, rate, nlanes, dsc); > +} > + > +static void mtk_dp_report_link_train(struct mtk_dp *mtk_dp) > +{ > + struct drm_modeset_lock *lock; > + int ret; > + > + if (!mtk_dp->conn) > + return; [Severity: Medium] Will this silently discard the link training parameters on the initial hotplug? Because the initial HPD interrupt fires before any atomic modeset has occurred, mtk_dp->conn is only initialized later in mtk_dp_bridge_atomic_enable(). At this point, it will be NULL, causing mtk_dp_report_link_train() to return immediately. Furthermore, while mtk_dp_bridge_atomic_enable() does report link training, it restricts it to eDP. Will standard DP connections ever report their current link parameters to the DRM core for the initial connection? [ ... ] > @@ -2390,12 +2433,20 @@ static void mtk_dp_bridge_atomic_enable(struct > drm_bridge *bridge, > if (mtk_dp->data->bridge_type == DRM_MODE_CONNECTOR_eDP) { > mtk_dp_aux_panel_poweron(mtk_dp, true); > > + ret = drm_dp_sink_sync_caps(mtk_dp->conn, &mtk_dp->aux, NULL); [Severity: High] Does this code call drm_dp_sink_sync_caps() while the modeset locks are dropped? During a non-blocking atomic commit, the atomic_enable and atomic_disable callbacks are executed in a worker thread asynchronously, and all modeset locks (including connection_mutex) are intentionally dropped by the submitter thread beforehand. Since drm_dp_sink_sync_caps() explicitly enforces that the connector's connection_mutex is held by asserting drm_modeset_lock_assert_held(&connector->dev->mode_config.connection_mutex), will this trigger a lockdep splat? > + if (ret) > + drm_dbg_kms(mtk_dp->aux.drm_dev, > + "%s: error sink caps synchronisation > (%d)\n", > + mtk_dp->aux.name, ret); > + > /* Training */ > ret = mtk_dp_training(mtk_dp); > if (ret) { > drm_err(mtk_dp->drm_dev, "Training failed, %d\n", ret); > goto power_off_aux; > } > + > + _mtk_dp_report_link_train(mtk_dp); [Severity: High] Does calling _mtk_dp_report_link_train() here bypass the locking that is normally handled by the mtk_dp_report_link_train() wrapper? _mtk_dp_report_link_train() eventually calls drm_dp_set_cur_link_params(), which also asserts that connection_mutex is held. Since the lock is dropped in atomic_enable, is it safe to expose the connector's properties to concurrent data races without the lock? > } > > ret = mtk_dp_video_config(mtk_dp); [ ... ] > @@ -2435,6 +2486,7 @@ static void mtk_dp_bridge_atomic_disable(struct > drm_bridge *bridge, > if (mtk_dp->data->bridge_type == DRM_MODE_CONNECTOR_eDP) { > mtk_dp->enabled = false; > mtk_dp_aux_panel_poweron(mtk_dp, false); > + drm_dp_sink_reset_caps(mtk_dp->conn); [Severity: High] Similar to the issue above, does this call drm_dp_sink_reset_caps() in an unlocked context? This function also asserts that connection_mutex is held, which would lead to a lockdep splat during an asynchronous atomic commit. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
