Re: [PATCH RFC v3 8/8] drm/mediatek: Use dp_connector helpers to expose link training state

[sashiko-bot@kernel.org]

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

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/20261012-feat_link_cap-v3-0-7517f4aba0f3@bootlin.com?part=8