Re: [PATCH RFC v2 4/4] drm/mediatek: Use dp_connector helpers to report link training state

[sashiko-bot@kernel.org]

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 <kory.maincent@bootlin.com>

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/20260619-feat_link_cap-v2-0-a3dec4c02ad9@bootlin.com?part=4