Re: [PATCH v6 17/18] drm/bridge: analogix_dp: Handle HPD notification from downstream bridge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] drm/bridge: analogix_dp: analogix_dp_supports_hpd() ignores
  hpd_notify_mode
- [Medium] drm/bridge: analogix_dp: DRM_BRIDGE_OP_HPD set before
  drm_bridge_attach()
- [Medium] drm/bridge: analogix_dp: detect hpd_notify() echo races with
  HPD notifications
--

commit c237458f9c91bcd96060ebf17f0f83f06f94643c
Author: Damon Ding <damon.ding@rock-chips.com>

drm/bridge: analogix_dp: Handle HPD notification from downstream bridge

This commit adds an hpd_notify_mode to analogix_dp so that hotplug status is
tracked via .hpd_notify instead of native HPD interrupts or register polling
when a downstream bridge provides HPD without detection.

Link: https://lore.kernel.org/all/5609825.iZASKD2KPV@diego/

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 9f50163aecf9f..6db2893443758 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -52,7 +52,8 @@ static const bool verify_fast_training;
>  static bool analogix_dp_require_pm_for_hpd_irq(struct analogix_dp_device *dp)
>  {
>  	return (dp->plat_data->dev_type == RK3576_EDP || dp->plat_data->dev_type == RK3588_EDP) &&
> -	       !dp->hpd_gpiod && !dp->force_hpd && !dp->force_connected_mode;
> +	       !dp->hpd_gpiod && !dp->force_hpd && !dp->force_connected_mode &&
> +	       !dp->hpd_notify_mode;
>  }

[Severity: Medium]
Should analogix_dp_supports_hpd() also check !dp->hpd_notify_mode?

drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_supports_hpd() {
	if (dp->force_connected_mode || dp->force_hpd)
		return false;

	if (dp->hpd_gpiod)
		return true;

	return dp->plat_data->dev_type == RK3576_EDP || dp->plat_data->dev_type == RK3588_EDP;
}

In analogix_dp_bind(), analogix_dp_require_pm_for_hpd_irq(dp) now returns
false when dp->hpd_notify_mode is set, skipping pm_runtime_resume_and_get()
and leaving native HPD interrupts unmuted in hardware because
analogix_dp_config_interrupt() has not run.  Right after that,
analogix_dp_supports_hpd(dp) still returns true on RK3576_EDP and RK3588_EDP
and calls enable_irq(dp->irq), so any assertion on the unmuted native IRQ
line wakes analogix_dp_irq_thread() and calls drm_bridge_hpd_notify().

>  
>  /*
> @@ -946,6 +947,10 @@ analogix_dp_bridge_detect(struct drm_bridge *bridge, struct drm_connector *conne
>  	enum drm_connector_status status = connector_status_disconnected;
>  	int ret;
>  
> +	if (dp->hpd_notify_mode)
> +		return dp->connection_notified ? connector_status_connected :
> +						 connector_status_disconnected;
> +
>  	if (dp->force_connected_mode)
>  		return connector_status_connected;

[ ... ]

> @@ -1008,6 +1028,7 @@ static int analogix_dp_bridge_attach(struct drm_bridge *bridge,
>  		 * accurate hotplug state.
>  		 */
>  		dp->force_connected_mode = analogix_dp_bridge_force_connected(dp);
> +		dp->hpd_notify_mode = analogix_dp_has_downstream_hpd(dp);
>  	}
>  
>  	return 0;

[Severity: Medium]
Does analogix_dp_bind() check analogix_dp_supports_hpd() too early when
setting DRM_BRIDGE_OP_HPD?

drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:analogix_dp_bind() {
    ...
	bridge->ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
	if (analogix_dp_supports_hpd(dp))
		bridge->ops |= DRM_BRIDGE_OP_HPD;
	bridge->of_node = dp->dev->of_node;
	bridge->type = DRM_MODE_CONNECTOR_eDP;
	ret = devm_drm_bridge_add(dp->dev, &dp->bridge);
	if (ret)
		goto err_unregister_aux;

	ret = drm_bridge_attach(dp->encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
    ...
}

dp->force_connected_mode and dp->hpd_notify_mode are only computed inside
analogix_dp_bridge_attach() when drm_bridge_attach() runs.  When
analogix_dp_supports_hpd(dp) is evaluated before drm_bridge_attach(), both
flags are still false, so bridge->ops advertises DRM_BRIDGE_OP_HPD on
RK3576_EDP and RK3588_EDP (or when dp->hpd_gpiod is set) even for fixed
panels where enable_irq(dp->irq) is later skipped.

> @@ -1304,6 +1325,14 @@ static void analogix_dp_bridge_atomic_post_disable(struct drm_bridge *bridge,
>  		DRM_ERROR("Failed to enable psr (%d)\n", ret);
>  }
>  
> +static void analogix_dp_bridge_notify(struct drm_bridge *bridge, struct drm_connector *connector,
> +				      enum drm_connector_status status)
> +{
> +	struct analogix_dp_device *dp = to_dp(bridge);
> +
> +	dp->connection_notified = (status == connector_status_connected);
> +}

[Severity: Medium]
Can the .hpd_notify() call at the end of drm_bridge_connector_detect_ctx()
overwrite a concurrent hotplug notification in dp->connection_notified?

drm_bridge_connector_detect_ctx() calls detect->funcs->detect() and then
passes the returned status to drm_bridge_connector_hpd_notify().  Meanwhile,
drm_bridge_connector_handle_hpd() drops dev->mode_config.mutex before calling
drm_bridge_connector_hpd_notify() and
drm_kms_helper_connector_hotplug_event():

CPU0                                  CPU1
drm_bridge_connector_detect_ctx()
  analogix_dp_bridge_detect()
    reads connection_notified (false)
                                      drm_bridge_connector_handle_hpd()
                                        drm_bridge_connector_hpd_notify(
                                          ..., status_connected)
                                          analogix_dp_bridge_notify()
                                            connection_notified = true
  drm_bridge_connector_hpd_notify(
    ..., status_disconnected)
    analogix_dp_bridge_notify()
      connection_notified = false  <-- overwrites connected state
                                        drm_kms_helper_connector_hotplug_event()
                                          ...
                                          analogix_dp_bridge_detect()
                                            returns status_disconnected

When the hotplug event on CPU1 re-probes the connector,
analogix_dp_bridge_detect() sees dp->connection_notified == false and reports
the newly connected sink as disconnected, and subsequent AUX transfers fail in
analogix_dp_detect_hpd().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113082305.541652-1-damon.ding@rock-chips.com?part=17