Re: [PATCH v8 4/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 4 potential issue(s) to consider:
- [High] irq: HPD events lost during runtime resume due to cleared IIR
- [High] pm: violating context separation by using runtime PM flag in system PM
- [Medium] irq: spurious wakeups due to unmasked GMBUS and AUX interrupts
- [Medium] pm: missing pme_enabled callback renders feature inactive on i915
--

commit 726361eac47c46981fd133dfd6565522bf0f1000
Author: Vinod Govindapillai <vinod.govindapillai@intel.com>

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

This commit skips resetting HPD-related IRQs during suspend if the device
is capable of generating PME from HPDs. This prevents dropping hotplug
events that should wake the system, as a full reset would clear them.

> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> index a59b75830bd13..8ce30112de974 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> @@ -2217,8 +2217,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: Medium]
Will this feature ever be active on i915?

intel_display_rpm_pme_enabled() checks rpm->pme_enabled. While xe implements
this callback, i915_display_rpm_interface (in intel_runtime_pm.c) does not:

const struct intel_display_rpm_interface i915_display_rpm_interface = {
	.get = i915_display_rpm_get,
        ...

This means intel_display_rpm_pme_enabled() will unconditionally return false on
all i915 platforms, causing reset_hpd to always be true.

[Severity: High]
Does this violate PM context separation by using a runtime PM flag in a
system PM path?

gen11_display_irq_reset() is a shared IRQ reset function called by both runtime
PM and system PM. The xe->pme.enabled flag queried by
intel_display_rpm_pme_enabled() is a runtime PM flag.

If this evaluates to true during a system PM resume, gen11_display_irq_reset()
will skip zeroing GEN11_DISPLAY_INT_CTL and resetting HPD IRQs, leaving the
hardware in an undefined power-on state since system suspend always loses
power and requires full reinitialization.

[Severity: High]
Does this logic inadvertently clear the HPD event that woke up the system?

When a PME wakes the system, xe_pci_runtime_resume() immediately disables PME
before resuming:

xe_pci_runtime_resume()
    xe_pm_update_pme_enabled(xe, false);
    xe_pm_runtime_resume(xe);
        ... -> gen11_display_irq_reset()

Since PME is now false, reset_hpd evaluates to true during the resume phase.
This causes irq_reset() to write 0xffffffff to the IIR for the HPD registers
later in this function, completely clearing the pending hardware HPD event
before the interrupt handler can be installed and process it.

>  
> -	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) {
>  		enum transcoder trans;

[ ... ]

> @@ -2250,13 +2252,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: Medium]
Will bypassing irq_reset() for these registers cause spurious wakeups?

The PICAINTERRUPT_IRQ_REGS and SDE_IRQ_REGS control multiple interrupts, not
just HPD. For instance, SDE_IRQ_REGS enables SDE_GMBUS_ICP (GMBUS/I2C), and
PICAINTERRUPT_IRQ_REGS enables TC AUX interrupts.

By completely skipping the register reset instead of explicitly preserving just
the HPD bits, GMBUS and AUX interrupts remain unmasked while the device is in
D3hot. Any activity or electrical noise on these buses could trigger an
interrupt, generating a PME and waking the system spuriously, causing unwanted
power consumption.

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

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