On Mon, 17 Aug 2026, Vinod Govindapillai <[email protected]> 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 <[email protected]>
> ---
> .../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
