Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Clearing all interrupts in `xe_irq_resume` unconditionally discards pending HPD events (especially DP short pulses) that woke the system. - [Medium] `gen11_display_irq_reset()` leaves non-HPD interrupts enabled when `keep_hpd` is true by bypassing entire register bank resets. --
commit 4b2992909ca6b4f24f513b4a4b9fb59c8de79fa9 Author: Vinod Govindapillai <[email protected]> drm/i915/hpd: plumb keep_hpd through irq reset This patch updates the display interrupt reset paths in i915 and xe to conditionally preserve HPD-related interrupt registers. When the keep_hpd flag is true, HPD interrupts remain active during suspend (D3hot) to allow hotplug events to wake the device. In all other cases, they are reset as usual. > diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c > b/drivers/gpu/drm/i915/display/intel_display_irq.c > index a59b75830bd13..26fc90ff7299d 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_irq.c > +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c [ ... ] > @@ -2250,13 +2259,20 @@ 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); > + /* > + * When runtime suspending to D3hot with PME armed, leave the HPD > + * interrupt registers programmed so that a hotplug can still > + * generate the wake event. Everything else is reset as usual. > + */ > + 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] Does bypassing these register banks entirely when keep_hpd is true leave non-HPD interrupts enabled during suspend? The SDE and PICA registers contain non-HPD interrupts such as GMBUS, Audio, and AUX. If these are not reset, could they generate unexpected transitions on the bus and trigger spurious PME wakeups while the device is in D3hot? [ ... ] > diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c > index 9e49e2241da41..eb240615455ce 100644 > --- a/drivers/gpu/drm/xe/xe_irq.c > +++ b/drivers/gpu/drm/xe/xe_irq.c [ ... ] > @@ -866,7 +866,11 @@ void xe_irq_resume(struct xe_device *xe) > * 2. display is not yet resumed > */ > atomic_set(&xe->irq.enabled, 1); > - xe_irq_reset(xe); > + /* > + * Always a full reset here: once awake, HPD is delivered as a normal > + * interrupt again and the postinstall below re-arms that path. > + */ > + xe_irq_reset(xe, false); [Severity: High] Does unconditionally clearing all interrupts here discard pending edge-triggered HPD events (like DisplayPort short pulses) that just woke the system? The comment mentions that HPD is delivered as a normal interrupt once awake. However, since xe_irq_reset(xe, false) writes to the display IIR to clear the latched status before the interrupt handler runs, wouldn't the event be permanently lost since edge-triggered events are not re-asserted? This might cause the driver to miss connection changes or link retraining requests. > xe_irq_postinstall(xe); /* turn irqs on */ > > for_each_gt(gt, xe, id) -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
