Re: [PATCH v6 6/7] drm/i915/irq: conditional HPD IRQ resets based on PME capability

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The patch uses a runtime PM flag (`xe->pme.enabled` via `intel_display_rpm_pme_enabled`) to conditionally skip HPD IRQ resets in a path shared by both system PM and runtime PM, violating subsystem guidelines and breaking system PM behavior.
- [High] Pending HPD interrupts that woke the system from runtime suspend are cleared and lost during the resume path because the IIR register is reset before they can be handled.
- [Medium] Skipping the reset of `SDE_IRQ_REGS` entirely leaves all non-HPD South Display Engine (SDE) interrupts active during suspend, which can cause spurious wakeups.
--

commit 52d852459481153001bd323a577aaf67b9baae9c
Author: Vinod Govindapillai <vinod.govindapillai@intel.com>
Subject: drm/i915/irq: conditional HPD IRQ resets based on PME capability

This patch conditionally skips HPD IRQ resets during suspend routines
based on the PME capability of the device. If the device supports
generating PME from HPDs, the HPD-related IRQs are left active to
prevent losing the HPD events.

> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> index a59b75830bd13..7e71c96c663fa 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c

[ ... ]

> @@ -2217,8 +2218,10 @@ static void gen11_display_irq_reset(struct intel_display *display)
>  	enum pipe pipe;
>  	u32 trans_mask = BIT(TRANSCODER_A) | BIT(TRANSCODER_B) |
>  		BIT(TRANSCODER_C) | BIT(TRANSCODER_D);
> +	bool reset_hpd = !intel_display_rpm_pme_enabled(display);

[Severity: High]
Does this violate context separation guidelines by using a runtime PM flag
to control a path shared with system PM?

During system suspend, the device is typically runtime-resumed first or
already active, meaning the runtime flag accessed via
intel_display_rpm_pme_enabled() evaluates to false. This results in
reset_hpd being true, which unconditionally wipes HPD IRQs during S3
suspend and defeats the intended PME wakeup behavior.

Additionally, in the i915 driver, the .pme_enabled callback appears to be
unimplemented in i915_display_rpm_interface, which would mean it always
returns false, rendering this logic non-functional for i915.

>  
> -	intel_de_write(display, GEN11_DISPLAY_INT_CTL, 0);
> +	if (reset_hpd)
> +		intel_de_write(display, GEN11_DISPLAY_INT_CTL, 0);
>  
>  	if (DISPLAY_VER(display) >= 12) {

[ ... ]

> @@ -2250,13 +2253,15 @@ static void gen11_display_irq_reset(struct intel_display *display)
>  	irq_reset(display, GEN8_DE_PORT_IRQ_REGS);
>  	irq_reset(display, GEN8_DE_MISC_IRQ_REGS);
>  
> -	if (DISPLAY_VER(display) >= 14)
> -		irq_reset(display, PICAINTERRUPT_IRQ_REGS);
> -	else
> -		irq_reset(display, GEN11_DE_HPD_IRQ_REGS);

[Severity: High]
Does this clear and lose pending HPD interrupts that woke the system from
runtime suspend?

When an HPD event triggers a PME wakeup from runtime suspend, the hardware
sets the corresponding bit in the IIR register. During runtime resume, the
PCI driver first clears the PME capability flag, and then eventually calls
gen11_display_irq_reset().

Because the PME enabled flag was just cleared,
intel_display_rpm_pme_enabled() evaluates to false, making reset_hpd
evaluate to true. This causes the driver to write 0xffffffff to the IIR
register here in irq_reset(), destroying the pending HPD event before the
interrupt handler is re-enabled to process it.

> +	if (reset_hpd) {
> +		if (DISPLAY_VER(display) >= 14)
> +			irq_reset(display, PICAINTERRUPT_IRQ_REGS);
> +		else
> +			irq_reset(display, GEN11_DE_HPD_IRQ_REGS);
>  
> -	if (INTEL_PCH_TYPE(display) >= PCH_ICP)
> -		irq_reset(display, SDE_IRQ_REGS);
> +		if (INTEL_PCH_TYPE(display) >= PCH_ICP)
> +			irq_reset(display, SDE_IRQ_REGS);

[Severity: Medium]
By skipping the reset of SDE_IRQ_REGS when reset_hpd is false, does this
leave all non-HPD South Display Engine (SDE) interrupts active during
suspend?

SDE_IRQ_REGS controls many interrupts beyond HPD, such as GMBUS (I2C) and
Audio. Because the master interrupt control (GEN11_DISPLAY_INT_CTL) is
also intentionally left active to support PME, any activity on these
non-HPD sources during suspend could trigger the master interrupt and
spuriously wake the system.

Should this selectively mask out non-HPD sources via read-modify-write to
the IMR registers rather than bypassing the reset completely?

> +	}
>  }
>  
>  void gen8_irq_power_well_post_enable(struct intel_display *display,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914194737.272679-1-vinod.govindapillai@intel.com?part=6