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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 17 Aug 2026, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> A display hotplug can generate a PM_PME which in turn will
> trigger pm_runtime_resume call on a device. So we don't need
> to resort to HPD polling on pm_runtime_suspend calls. But on
> pm_runtime_resume call we would need to still resort to the
> intel_hpd_poll_disable() as the i915_hpd_poll_detect_connectors()
> catches the hotplugs that occurred during suspend.
>
> Assisted-by: GitHub_Copilot:claude-opus-5
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  .../drm/i915/display/intel_display_driver.c   | 21 ++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 70d112fd4287..b7e25b7ea32e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -34,6 +34,7 @@
>  #include "intel_display_driver.h"
>  #include "intel_display_irq.h"
>  #include "intel_display_power.h"
> +#include "intel_display_rpm.h"
>  #include "intel_display_types.h"
>  #include "intel_display_utils.h"
>  #include "intel_display_wa.h"
> @@ -931,6 +932,10 @@ void intel_display_driver_pm_runtime_suspend_late(struct intel_display *display)
>  		intel_opregion_notify_adapter(display, PCI_D1);
>  	}
>  
> +	/* No need of HPD polling if the device is PME capable */
> +	if (intel_display_rpm_pme_capable(display))
> +		return;
> +

It's really messy to just return here. We may add more stuff into this
function, and it will be a PITA.

>  	if (!display->platform.valleyview && !display->platform.cherryview)
>  		intel_hpd_poll_enable(display);
>  }
> @@ -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;

Ditto. The function is about runtime resume. Don't make it about dodging
hpd poll.

> +
> +	/* For PME capable devices we would not have resorted into HPD polling */

These comments are a nuisance. This makes you stop for more questions
than what it answers.

> +	if (intel_hpd_polling_enabled(display))
>  		intel_hpd_init(display);

Why is this not called unconditionally?

> -		intel_hpd_poll_disable(display);
> -	}

Maybe PME is an hpd implementation detail that should be hidden inside
intel_hpd_poll_enable/disable? Maybe it should not be poll
enable/disable at all, but hpd runtime suspend/resume, and that can do
whatever needs doing internally?

>  
> +	/*
> +	 * 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

Ditto about the comment. It's nonsensical.

> +	 */
> +	intel_hpd_poll_disable(display);
> +
> +out:
>  	skl_watermark_ipc_update(display);
>  }

-- 
Jani Nikula, Intel