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 <[email protected]> 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/[email protected]?part=3
