Re: [PATCH v7 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 2 potential issue(s) to consider:
- [High] Skipping complete reset of SDE_IRQ_REGS and PICAINTERRUPT_IRQ_REGS leaves non-HPD interrupts unmasked during suspend
- [Medium] Use of a runtime PM flag in a system PM path violates context separation rules
--

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

This commit modifies the display IRQ reset sequence to conditionally keep
HPD IRQs active if the device is capable of generating PME from HPDs. This
prevents the loss of HPD wake events during suspend routines.

> 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);
>  
> -	intel_de_write(display, GEN11_DISPLAY_INT_CTL, 0);
> +	if (reset_hpd)
> +		intel_de_write(display, GEN11_DISPLAY_INT_CTL, 0);

[Severity: Medium]
Does using intel_display_rpm_pme_enabled() here violate context separation
rules between system PM and runtime PM?

gen11_display_irq_reset() is called during system suspend (for example, via
xe_irq_suspend()). Evaluating intel_display_rpm_pme_enabled() ultimately reads
xe->pme.enabled, which is a runtime PM flag that is only updated during
runtime suspend and resume.

Because runtime resume typically clears this flag before a system suspend,
won't reset_hpd always evaluate to true during system suspend, thereby
defeating the purpose of preserving HPD wake capability?

>  	if (DISPLAY_VER(display) >= 12) {
>  		enum transcoder trans;

[ ... ]

> @@ -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);
> +	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: High]
Does skipping the complete reset of SDE_IRQ_REGS and PICAINTERRUPT_IRQ_REGS
leave non-HPD interrupts unmasked during suspend?

If reset_hpd evaluates to false (such as when entering runtime suspend with
PME enabled), the code skips resetting these entire IRQ register banks. Since
SDE_IRQ_REGS controls GMBUS and I2C interrupts, and PICAINTERRUPT_IRQ_REGS
controls AUX channel interrupts, won't entirely skipping their resets leave
them fully enabled in the IMR/IER registers?

Could this cause unexpected system wakeups or interrupt storms from unhandled
GMBUS or AUX interrupts while suspended, rather than selectively masking only
the non-HPD bits?

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