Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The DRM_BRIDGE_OP_DP flag is unintentionally overwritten and lost for external DP configurations, breaking the patch's main goal. - [High] Calling DRM DP connector state update helpers from the HPD event thread and atomic_enable without holding the connection mutex triggers kernel WARN_ON traces. - [High] DP link capabilities and parameters are silently dropped because mtk_dp->conn is NULL during the first hotplug event and during probe for eDP. - [Medium] The patch fails to implement the voltage swing and pre-emphasis link training properties claimed in the commit message. --
commit 329f586786239579aef1351b83fdceecb28dd1e1 Author: Kory Maincent <[email protected]> drm/mediatek: Use dp_connector helpers to report link training state Set DRM_BRIDGE_OP_DP and populate dp_link_train_caps with the supported link rates, lane counts, voltage swing and pre-emphasis levels so the bridge connector uses drmm_connector_dp_init() and exposes the link training state properties to userspace. Store per-lane voltage swing and pre-emphasis values in mtk_dp_train_info and report the negotiated link parameters via drm_connector_dp_set_link_train_properties() on training completion. Clear them via drm_connector_dp_reset_link_train_properties() when a disconnect is detected in the HPD event thread. > Set DRM_BRIDGE_OP_DP and populate dp_link_train_caps with the supported > link rates, lane counts, voltage swing and pre-emphasis levels so the > bridge connector uses drmm_connector_dp_init() and exposes the link > training state properties to userspace. > > Store per-lane voltage swing and pre-emphasis values in > mtk_dp_train_info and report the negotiated link parameters via > drm_connector_dp_set_link_train_properties() on training completion. [Severity: Medium] The commit message mentions populating voltage swing and pre-emphasis levels and using drm_connector_dp_set_link_train_properties(). Is this description accurate? The actual patch only sets up lanes and link rates in dp_link_caps, and there are no calls to drm_connector_dp_set_link_train_properties() in the diff. > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c > b/drivers/gpu/drm/mediatek/mtk_dp.c > index c52cc7c2e2006..439b9496632fe 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dp.c > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > @@ -1789,6 +1789,8 @@ static int mtk_dp_parse_capabilities(struct mtk_dp > *mtk_dp) > } > } > > + drm_dp_sink_set_link_caps(mtk_dp->conn, &mtk_dp->aux); > + > return 0; > } [Severity: High] Will mtk_dp->conn be NULL when this is called during eDP probe or the first hotplug event? The connector appears to only be initialized later during mtk_dp_bridge_atomic_enable(). If mtk_dp->conn is NULL, drm_dp_sink_set_link_caps() will silently return early and discard the parsed capabilities. [Severity: High] Also, drm_dp_sink_set_link_caps() expects the caller to hold the connection mutex, enforcing it with: WARN_ON(!drm_modeset_is_locked(&connector->dev->mode_config.connection_mutex)); Are we guaranteed to hold the modeset lock when reaching this from the HPD event thread or eDP probe path? [ ... ] > @@ -1812,6 +1814,17 @@ 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; > + > + 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, false); > + drm_dp_set_max_link_params(mtk_dp->conn, rate, nlanes); > +} [Severity: High] Similar to the capability parsing above, can mtk_dp->conn be NULL here if called from the HPD thread before the first modeset? Additionally, these helper functions also require the connection mutex. Since mtk_dp_report_link_train() is called from mtk_dp_training() which can be reached from the lockless non-blocking atomic_enable path, could this trigger a WARN_ON? [ ... ] > @@ -2004,6 +2018,7 @@ static irqreturn_t mtk_dp_hpd_event_thread(int hpd, > void *dev) > mtk_dp->need_debounce = false; > mod_timer(&mtk_dp->debounce_timer, > jiffies + msecs_to_jiffies(100) - 1); > + drm_dp_sink_reset_link_caps(mtk_dp->conn); > } else { > mtk_dp_aux_panel_poweron(mtk_dp, true); [Severity: High] Calling drm_dp_sink_reset_link_caps() directly from the asynchronous HPD IRQ thread will trigger a kernel warning because the connection mutex is not held. Is there a way to defer this update to a workqueue or safely grab the required modeset locks? [ ... ] > @@ -2809,6 +2830,8 @@ static int mtk_dp_probe(struct platform_device *pdev) > > mtk_dp->bridge.of_node = dev->of_node; > mtk_dp->bridge.type = mtk_dp->data->bridge_type; > + mtk_dp->bridge.dp_link_caps = &dp_link_caps; > + mtk_dp->bridge.ops = DRM_BRIDGE_OP_DP; > > if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) { > /* [Severity: High] Does this assignment get lost for external DP configurations? Further down in mtk_dp_probe(), the non-eDP branch assigns the ops directly using an equals sign instead of a bitwise OR: } else { mtk_dp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD; ... } This appears to completely overwrite the DRM_BRIDGE_OP_DP flag added above, which would prevent the bridge connector from exposing link training properties to userspace for external DP. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
