Re: [PATCH v11 13/21] drm/rockchip: dw_dp: Implement out-of-band HPD handling

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing runtime PM acquisition in out-of-band HPD callbacks defeats the patch's purpose.
- [Medium] dw_dp_rockchip_get_vo_grf() hard-fails instead of implementing the documented DT fallback for future platforms.
--

--- Patch [13]: [PATCH v11 13/21] drm/rockchip: dw_dp: Implement out-of-band HPD handling ---
commit 7506de24244286fa5e8735e9092296ca64dbef67
Author: Sebastian Reichel <[email protected]>

drm/rockchip: dw_dp: Implement out-of-band HPD handling

This commit implements out-of-band hotplug handling to receive external
hotplug information from the USB-C state machine. It aims to prevent
massive SError problems once runtime PM is implemented in the DP driver
by moving the register access into the DP driver.

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index 38e8fe75718e4..9e49e7dbf420f 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c

[ ... ]

> @@ -23,12 +26,48 @@
>  
>  #include "rockchip_drm_drv.h"
>  
> +#define ROCKCHIP_MAX_CTRLS 2
> +
> +#define ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL BIT(10)
> +#define ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG BIT(11)

[ ... ]

> +static void dw_dp_rockchip_hpd_sw_sel(void *data, bool force_hpd_from_sw)
> +{
> +	struct rockchip_dw_dp *dp = data;
> +	u32 hpd_reg = dp->pdata->hpd_reg[dp->id];
> +
> +	regmap_write(dp->vo_grf, hpd_reg,
> +		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL, force_hpd_from_sw));

[Severity: Medium]
Does this need to acquire a runtime PM reference before accessing the register?

Since this callback can be triggered asynchronously by an out-of-band event
from the Type-C TCPM, writing to the vo_grf regmap while the DP controller
is suspended might trigger the exact SError this patch intends to fix,
once runtime PM is enabled.

> +}
> +
> +static void dw_dp_rockchip_hpd_sw_cfg(void *data, bool hpd)
> +{
> +	struct rockchip_dw_dp *dp = data;
> +	u32 hpd_reg = dp->pdata->hpd_reg[dp->id];
> +
> +	dev_dbg(dp->dev, "Force HPD connected=%s\n", str_yes_no(hpd));
> +
> +	regmap_write(dp->vo_grf, hpd_reg,
> +		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG, hpd));

[Severity: Medium]
Similarly, does this asynchronous callback need to ensure the DP power
domain is active?

If an asynchronous hotplug event occurs without a runtime PM guarantee,
could this register access crash the system?

> +}

[ ... ]

> +static struct regmap *dw_dp_rockchip_get_vo_grf(struct rockchip_dw_dp *dp)
> +{
> +	struct device_node *np = dev_of_node(dp->dev);
> +	struct of_phandle_args args;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	ret = of_parse_phandle_with_args(np, "phys", "#phy-cells", 0, &args);
> +	if (ret)
> +		return ERR_PTR(-ENODEV);
> +
> +	/*
> +	 * Limit this workaround to RK3576 and RK3588, potential future platforms
> +	 * reusing the driver should just add a VO GRF phandle in the DisplayPort
> +	 * controller DT node.
> +	 */
> +	if (!of_device_is_compatible(args.np, "rockchip,rk3576-usbdp-phy") &&
> +	    !of_device_is_compatible(args.np, "rockchip,rk3588-usbdp-phy")) {
> +		regmap = ERR_PTR(-ENODEV);
> +		goto out_put_node;
> +	}

[Severity: Medium]
Does this hard-fail prevent the documented fallback? 

The comment mentions future platforms should add a VO GRF phandle directly
in the DisplayPort controller node, but it looks like we return
ERR_PTR(-ENODEV) here without ever attempting to parse the VO GRF property
from np.

If a future platform is missing the legacy phys property or uses a different
PHY compatible string, it will hit these ERR_PTR(-ENODEV) returns and probe
will fail instead of falling back to the documented scheme.

> +
> +	regmap = syscon_regmap_lookup_by_phandle(args.np, "rockchip,vo-grf");
> +
> +out_put_node:
> +	of_node_put(args.np);
> +	return regmap;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-synopsys-dw-dp-improvements-v11-0-0d508505f383@collabora.com?part=13