Re: [RFC PATCH 5/5] drm/i915/display: skip HPD polling if PME is supported

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Skipping `intel_hpd_init()` on runtime resume for PME-capable devices leaves hardware HPD interrupts permanently disabled, breaking hotplug detection while the device is awake.
--

--- Patch [5]: [RFC PATCH 5/5] drm/i915/display: skip HPD polling if PME is supported ---
commit f6a80f590c41d181052f2d40a190e9c030e50eb2
Author: Vinod Govindapillai <[email protected]>
drm/i915/display: skip HPD polling if PME is supported

This commit prevents the display driver from enabling HPD polling during
runtime suspend if the device supports PM_PME. It also updates the resume
path to selectively initialize HPD only if polling was actually enabled.

> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 70d112fd4287a..b7e25b7ea32ed 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c

[ ... ]

> @@ -951,10 +956,20 @@ void intel_display_driver_pm_runtime_resume(struct intel_display *display)
>  	 * power well, so hpd is reinitialized from there. For
>  	 * everyone else do it here.
>  	 */
> -	if (!display->platform.valleyview && !display->platform.cherryview) {
> +	if (display->platform.valleyview || display->platform.cherryview)
> +		goto out;
> +
> +	/* For PME capable devices we would not have resorted into HPD polling */
> +	if (intel_hpd_polling_enabled(display))
>  		intel_hpd_init(display);

[Severity: High]
Does skipping intel_hpd_init() here permanently break hardware hotplug
detection for PME-capable devices?

When a PME-capable device enters runtime suspend, it skips
intel_hpd_poll_enable(), so intel_hpd_polling_enabled() will return false. 
On resume, this condition causes intel_hpd_init() to be skipped.

However, looking at intel_hpd_init() in intel_hotplug.c, it does more than
just polling setup. It also programs the hardware registers to re-enable
hardware interrupts:

void intel_hpd_init(struct intel_display *display)
{
    ...
	spin_lock_irq(&display->irq.lock);
	intel_hpd_irq_setup(display);
	spin_unlock_irq(&display->irq.lock);
}

Without calling intel_hpd_irq_setup(), the hardware HPD interrupts will
remain masked. Will any subsequent monitor hotplugs or unplugs that happen
while the device is awake be silently ignored?

> -		intel_hpd_poll_disable(display);
> -	}
>  
> +	/*
> +	 * Regardless of PME capable path, call the HPD polling disable, the
> +	 * poll_init_work, i915_hpd_poll_detect_connectors reprobe catches the
> +	 * hotplug that occurred while suspended
> +	 */
> +	intel_hpd_poll_disable(display);
> +
> +out:
>  	skl_watermark_ipc_update(display);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5