Re: [PATCH 1/4] drm/i915/hpd: plumb keep_hpd through irq reset

[sashiko-bot@kernel.org]

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 <vinod.govindapillai@intel.com>
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/20260826225338.804902-1-vinod.govindapillai@intel.com?part=1