Re: [PATCH v7 13/18] drm/rockchip: dw_dp: Add runtime PM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect ordering of GRF (Video Output General Register File) restoration in `dw_dp_rockchip_runtime_resume` delays HPD signal visibility to the DP hardware, breaking link training.
- [Medium] Missing system sleep callbacks prevent the device and its power domain from suspending during system sleep for native DP configurations.
--

--- Patch [13]: [PATCH v7 13/18] drm/rockchip: dw_dp: Add runtime PM support ---
commit ea12bfaf9ea353b204570fe194d520aa064c1270
Author: Sebastian Reichel <sebastian.reichel@collabora.com>

drm/rockchip: dw_dp: Add runtime PM support

Add support for runtime PM to the Rockchip RK3576/3588 Synopsys
DesignWare DisplayPort driver.

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> index b2e817044c942..01915f7b24d59 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
[ ... ]
> @@ -255,6 +274,34 @@ static void dw_dp_rockchip_remove(struct platform_device *pdev)
[ ... ]
> +static int dw_dp_rockchip_runtime_resume(struct device *dev)
> +{
> +	struct rockchip_dw_dp *dp = dev_get_drvdata(dev);
> +	u32 hpd_reg = dp->pdata->hpd_reg[dp->id];
> +	int ret;
> +
> +	ret = dw_dp_runtime_resume(dp->base);
> +	if (ret)
> +		return ret;
> +
> +	regmap_write(dp->vo_grf, hpd_reg,
> +		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL, dp->hpd_sel) |
> +		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG, dp->hpd_cfg));

[Severity: High]
Does restoring the GRF state after dw_dp_runtime_resume() break link training?

Looking at dw_dp_runtime_resume(), it initializes the hardware and sleeps
for 110ms to debounce the HPD signal. Because the power domain was off, the
VO GRF is in its reset state, so during this 110ms wait, the DP controller
will see HPD as LOW.

Once the GRF is finally written here, the controller sees HPD go HIGH and
starts a new hardware debounce. Any immediate attempt to access the sink
by the display core might fail because the DP controller still believes
HPD is disconnected.

Should the GRF state be restored before calling dw_dp_runtime_resume() so
the hardware debounce sees the correct HPD signal?

> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops dw_dp_pm_ops = {
> +	RUNTIME_PM_OPS(dw_dp_rockchip_runtime_suspend, dw_dp_rockchip_runtime_resume, NULL)
> +};

[Severity: Medium]
Will this prevent the system from entering deeper sleep states during system
suspend?

For native DP configurations, the core dw_dp driver acquires a runtime PM
reference in dw_dp_bind() and holds it indefinitely for Hot-Plug Detect
purposes. During system suspend (S3), since dw_dp_pm_ops is defined using
only RUNTIME_PM_OPS without system sleep callbacks, the PM core won't force
the device into a suspended state.

This means the generic PM domain might see the device as active and refuse
to power off the domain. Should system sleep callbacks be added to handle
this?

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