Re: [PATCH v12 12/23] drm/bridge: synopsys: dw-dp: Support software triggered OOB HPD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/bridge: dw-dp: missing HPD debounce wait in
  dw_dp_bridge_oob_notify()
- [Low] drm/bridge: dw-dp: split __free() declaration in
  dw_dp_is_routed_to_usb_c()
--

commit 8854deef3437dfb8f0f36f50f761f07577702cde
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/bridge: synopsys: dw-dp: Support software triggered OOB HPD

This commit adds support for USB-C DisplayPort AltMode out-of-band hotplug
notifications to the DesignWare DisplayPort bridge driver via platform
callbacks.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-dp.c b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> index 74ba1306caee6..b4eed4d0a5205 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-dp.c
> @@ -1875,6 +1875,19 @@ static struct drm_bridge_state *dw_dp_bridge_atomic_duplicate_state(struct drm_b
>  	return &state->base;
>  }
>  
> +static bool dw_dp_is_routed_to_usb_c(struct drm_encoder *encoder)
> +{
> +	struct drm_bridge *last_bridge __free(drm_bridge_put) = NULL;
> +	struct fwnode_handle *fwnode;
> +
> +	last_bridge = drm_bridge_chain_get_last_bridge(encoder);

[Severity: Low]
This isn't a bug, but per the Cleanup API conventions, last_bridge can be
defined and initialized in a single statement rather than declared as NULL
and assigned later:

	struct drm_bridge *last_bridge __free(drm_bridge_put) =
		drm_bridge_chain_get_last_bridge(encoder);

[ ... ]

> @@ -1923,6 +1943,22 @@ static void dw_dp_bridge_detach(struct drm_bridge *bridge)
>  	drm_dp_aux_unregister(&dp->aux);
>  }
>  
> +static void dw_dp_bridge_oob_notify(struct drm_bridge *bridge,
> +				    struct drm_connector *connector,
> +				    enum drm_connector_status status)
> +{
> +	bool hpd_high = status != connector_status_disconnected;
> +	struct dw_dp *dp = bridge_to_dp(bridge);
> +
> +	if (!dp->plat_data.hpd_sw_cfg) {
> +		dev_err_once(dp->dev, "Missing platform handler for OOB HPD handling\n");
> +		return;
> +	}
> +
> +	dev_dbg(dp->dev, "Force HPD connected=%s\n", str_yes_no(hpd_high));
> +	dp->plat_data.hpd_sw_cfg(dp->plat_data.data, hpd_high);
> +}

[Severity: Medium]
Does dw_dp_bridge_oob_notify() return before the DW-DP controller has
debounced the forced HPD signal in DW_DP_HPD_STATUS?

Right after bridge->funcs->oob_notify() returns,
drm_bridge_connector_oob_hotplug_event() calls
drm_bridge_connector_handle_hpd() -> drm_kms_helper_connector_hotplug_event(),
which probes the connector via dw_dp_bridge_detect() -> dw_dp_hpd_detect():

drivers/gpu/drm/bridge/synopsys/dw-dp.c:dw_dp_hpd_detect() {
	...
	regmap_read(dp->regmap, DW_DP_HPD_STATUS, &value);

	return FIELD_GET(HPD_STATE, value) == DW_DP_HPD_STATE_PLUG;
}

As seen in dw_dp_reset() (which polls DW_DP_HPD_STATUS for up to 200 ms),
the hardware requires ~100 ms of HPD held high before asserting
HPD_HOT_PLUG and updating HPD_STATE to DW_DP_HPD_STATE_PLUG. Returning
immediately after hpd_sw_cfg() causes synchronous detection on plug-in to
read a stale HPD_STATE and overwrite connector->status back to
disconnected.

Even with the later commit "drm/bridge: synopsys: dw-dp: Add Runtime PM
support", which adds msleep(110) in dw_dp_runtime_resume(), that delay only
runs when waking from runtime suspend; if the device is already
runtime-active when dw_dp_bridge_oob_notify() is called (for example during
the 500 ms autosuspend window or while a stream is active),
dw_dp_runtime_resume() is not called and hpd_sw_cfg() still returns before
HPD_STATE updates.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-synopsys-dw-dp-improvements-v12-0-d42d3dc5bdd4@collabora.com?part=12