Re: [PATCH v2 3/5] 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] Lost HPD interrupt on system resume causing broken hotplug detection
- [Medium] Unintentionally leaving non-HPD interrupts unmasked during suspend
--

commit 3a88160970f909255e3f0d82a7735b68d8c9df0d
Author: Vinod Govindapillai <vinod.govindapillai@intel.com>

drm/i915/irq: conditional HPD IRQ resets based on PME capability

This commit conditionally skips resetting HPD-related IRQs during suspend
if the device supports generating PME from HPDs. This aims to keep the
device capable of waking from hotplug 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..fc50b19d5ddfe 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
[ ... ]
> @@ -2250,13 +2254,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 (!keep_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]
Since PICAINTERRUPT_IRQ_REGS and SDE_IRQ_REGS contain both HPD and non-HPD
interrupts (like GMBUS, I2C, AUX, and Poison errors), does skipping their
reset here leave those non-HPD interrupts unmasked during suspend? Could
this lead to spurious wakeups or unintended interrupt handling while the
system is suspended?

[Severity: High]
During system resume, if keep_hpd evaluates to false (for example, if PME
capability is cleared before runtime resume), this block will execute
irq_reset() for these registers.

Looking at irq_reset(), it unconditionally clears the IIR:

static void irq_reset(struct intel_display *display, struct intel_irq_regs regs)
{
	intel_de_write(display, regs.imr, 0xffffffff);
	intel_de_posting_read(display, regs.imr);
	...
	/* IIR can theoretically queue up two events. Be paranoid. */
	intel_de_write(display, regs.iir, 0xffffffff);
	intel_de_posting_read(display, regs.iir);
	intel_de_write(display, regs.iir, 0xffffffff);
	intel_de_posting_read(display, regs.iir);
}

Does this permanently destroy the pending HPD event that just woke the system
before the IRQ handlers can process it?

Additionally, even if keep_hpd remained true during resume, wouldn't the
subsequent call to irq_init() during postinstall explicitly clear the IIR
anyway?

static void irq_init(struct intel_display *display, ...)
{
	...
	assert_iir_is_zero(display, regs.iir);
	...
}

Will this cause the hotplug event to be completely ignored, preventing the
display from being brought up after a PME wake?

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