Re: [PATCH v4 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 3 potential issue(s) to consider:
- [Medium] drm/mediatek: NULL mtk_dp->conn in mtk_dp_report_link_train() on hotplug
- [Medium] drm/mediatek: mtk_dp_bridge_atomic_enable() reports untrained DP link state
- [Medium] drm/mediatek: unsynchronized DP link state reporting in mtk_dp_hpd_event_thread()
--

commit da87270e7bdd066871fe7e9da8a36687e44518d5
Author: Kory Maincent <kory.maincent@bootlin.com>

drm/mediatek: Use dp_connector helpers to expose link training state

This commit adopts the DRM core DisplayPort helpers in mtk_dp by setting
DRM_BRIDGE_OP_DP and populating dp_link_caps at probe time. It also updates
the hotplug, detect, and atomic enable/disable paths to synchronize sink
capabilities and report link training parameters.

> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index f656b85b84212..b87e171d88d83 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -1812,6 +1812,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;
> +	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);
> +}

[Severity: Medium]
Will mtk_dp->conn be NULL when mtk_dp_report_link_train() is called from
mtk_dp_hpd_event_thread() on initial DisplayPort hotplug?

In drivers/gpu/drm/mediatek/mtk_dp.c, mtk_dp->conn is only assigned in
mtk_dp_bridge_atomic_enable():

	mtk_dp->conn = drm_atomic_get_new_connector_for_encoder(state,
								bridge->encoder);

Neither mtk_dp_bdg_detect() nor mtk_dp_bridge_attach() initializes
mtk_dp->conn. Moreover, on initial hotplug, mtk_dp_bridge_mode_valid()
rejects all modes until mtk_dp_training() sets train_info.link_rate and
train_info.lane_count in mtk_dp_hpd_event_thread(), so
mtk_dp_bridge_atomic_enable() cannot have run prior to the first
mtk_dp_report_link_train() call in mtk_dp_hpd_event_thread().

When mtk_dp_hpd_event_thread() runs link training and calls
mtk_dp_report_link_train() -> drm_dp_set_cur_link_params() before a modeset
has enabled the bridge, mtk_dp->conn is still NULL, causing
drm_dp_set_cur_link_params() to return early without recording cur_link_info
or updating sysfs.

>  
>  static int mtk_dp_training(struct mtk_dp *mtk_dp)
>  {
>  	int ret;
> @@ -2015,6 +2026,8 @@ static irqreturn_t mtk_dp_hpd_event_thread(int hpd, void *dev)
>  			ret = mtk_dp_training(mtk_dp);
>  			if (ret)
>  				drm_err(mtk_dp->drm_dev, "Training failed, %d\n", ret);
> +			else
> +				mtk_dp_report_link_train(mtk_dp);
>  
>  			mtk_dp->enabled = true;
>  		}
> @@ -2145,9 +2158,12 @@ mtk_dp_bdg_detect(struct drm_bridge *bridge, struct drm_connector *connector)
>  	struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
>  	enum drm_connector_status ret = connector_status_disconnected;
>  	bool enabled = mtk_dp->enabled;
> +	int err;
>  
> -	if (!mtk_dp->train_info.cable_plugged_in)
> +	if (!mtk_dp->train_info.cable_plugged_in) {
> +		drm_dp_sink_reset_caps(connector);
>  		return ret;
> +	}
>  
>  	if (!enabled)
>  		mtk_dp_aux_panel_poweron(mtk_dp, true);
> @@ -2160,9 +2176,18 @@ mtk_dp_bdg_detect(struct drm_bridge *bridge, struct drm_connector *connector)
>  	 * whether we connect to a sink device.
>  	 */
>  
> -	if (drm_dp_read_sink_count(&mtk_dp->aux) > 0)
> +	if (drm_dp_read_sink_count(&mtk_dp->aux) > 0) {
>  		ret = connector_status_connected;
>  
> +		err = drm_dp_sink_sync_caps(connector, &mtk_dp->aux, NULL);
> +		if (err)
> +			drm_dbg_kms(mtk_dp->aux.drm_dev,
> +				    "%s: error sink caps synchronisation (%d)\n",
> +				    mtk_dp->aux.name, err);
> +	} else {
> +		drm_dp_sink_reset_caps(connector);
> +	}
> +
>  	if (!enabled)
>  		mtk_dp_aux_panel_poweron(mtk_dp, false);

[Severity: Medium]
Can mtk_dp_bdg_detect() race with mtk_dp_hpd_event_thread() during hotplug
processing and corrupt the link training or reported link state?

In mtk_dp_hpd_event_thread(), drm_helper_hpd_irq_event() is called before
mtk_dp_training(), mtk_dp_report_link_train(), and mtk_dp->enabled = true:

	if (status & MTK_DP_THREAD_CABLE_STATE_CHG) {
		if (mtk_dp->bridge.dev)
			drm_helper_hpd_irq_event(mtk_dp->bridge.dev);
		...
		} else {
			mtk_dp_aux_panel_poweron(mtk_dp, true);
			...
			ret = mtk_dp_training(mtk_dp);
			if (ret)
				drm_err(mtk_dp->drm_dev, "Training failed, %d\n", ret);
			else
				mtk_dp_report_link_train(mtk_dp);

			mtk_dp->enabled = true;
		}
	}

Inside drm_helper_hpd_irq_event(), mtk_dp_bdg_detect() runs under
mode_config.mutex, syncs or resets sink capabilities, and then
drm_helper_hpd_irq_event() drops mode_config.mutex and sends a hotplug
uevent to userspace.

Because mtk_dp_hpd_event_thread() performs capability parsing,
mtk_dp_training(), and mtk_dp_report_link_train() without holding
connection_mutex, mode_config.mutex, or any driver lock while
mtk_dp->enabled is still false, a concurrent userspace probe calling
mtk_dp_bdg_detect() sees enabled == false, runs drm_dp_sink_sync_caps() over
AUX concurrently with mtk_dp_training(), and then calls
mtk_dp_aux_panel_poweron(mtk_dp, false), powering down the panel and AUX in
the middle of link training.

In addition, if a branch device asserts HPD with no downstream sink
(cable_plugged_in is true and drm_dp_read_sink_count(&mtk_dp->aux) <= 0)
after mtk_dp->conn has been set, mtk_dp_bdg_detect() calls
drm_dp_sink_reset_caps(connector), after which mtk_dp_hpd_event_thread()
runs mtk_dp_report_link_train(mtk_dp) and repopulates cur_link_info on a
disconnected connector.

> @@ -2390,6 +2415,12 @@ 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);
> +		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) {
> @@ -2398,6 +2429,8 @@ static void mtk_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  		}
>  	}
>  
> +	mtk_dp_report_link_train(mtk_dp);
> +
>  	ret = mtk_dp_video_config(mtk_dp);
>  	if (ret)
>  		goto power_off_aux;

[Severity: Medium]
Should mtk_dp_report_link_train() in mtk_dp_bridge_atomic_enable() be
restricted to eDP, or guarded on successful DisplayPort link training?

In mtk_dp_initialize_priv_data(), mtk_dp->train_info.link_rate is
initialized to DP_LINK_BW_5_4 and mtk_dp->train_info.lane_count is
initialized to mtk_dp->max_lanes.

For full DisplayPort (DRM_MODE_CONNECTOR_DisplayPort), mtk_dp_training() is
called in mtk_dp_hpd_event_thread() rather than in
mtk_dp_bridge_atomic_enable(). However, in mtk_dp_hpd_event_thread(),
drm_helper_hpd_irq_event() is called before mtk_dp_training(), and if
mtk_dp_training() fails, train_info.link_rate and train_info.lane_count
retain their initial defaults or stale values from a previous sink while
mtk_dp->enabled is still set to true.

When a modeset is committed (either via drm_helper_hpd_irq_event() ->
drm_client_dev_hotplug() before mtk_dp_training() runs, or by userspace
after mtk_dp_training() fails), mtk_dp_bridge_atomic_enable()
unconditionally calls mtk_dp_report_link_train(mtk_dp) outside the eDP
branch, exposing untrained or stale train_info parameters via
drm_dp_set_cur_link_params().

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-feat_link_cap-v4-0-a4f3f6e8cad9@bootlin.com?part=8