Re: [PATCH v2 3/5] drm/i915/irq: conditional HPD IRQ resets based on PME capability

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 03 Sep 2026, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> If a device supports generating PME from HPDs, resetting HPD IRQs
> will be counter productive as HPDs itself will be lost. During
> suspend routines, all the IRQs are reset. So if the device is
> capable of generating PME rom HPDs, keep the HPD related IRQs from
> reset based on the PME capability of the device on a target power
> state. PME capability will be assessed and updated separately.
>
> Bspec: 52979, 52980, 68857, 68867, 68970
> Assisted-by: GitHub_Copilot:claude-opus-5
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_display_irq.c  | 20 ++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> index a59b75830bd1..fc50b19d5ddf 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> @@ -22,6 +22,7 @@
>  #include "intel_fdi_regs.h"
>  #include "intel_fifo_underrun.h"
>  #include "intel_gmbus.h"
> +#include "intel_hotplug.h"
>  #include "intel_hotplug_irq.h"
>  #include "intel_lpe_audio.h"
>  #include "intel_parent.h"
> @@ -2217,8 +2218,11 @@ 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);
> +	/* If HPD can generate PME, we don't want to reset HPD related IRQs */

Please don't add comments in the middle of the declarations. IMO it's a
distraction rather than helpful.

To be fair, one comment like this here or there would be fine, but it
takes only a few to set an example both for humans and the language
models to start littering the declarations with comments, and I don't
want that.

> +	bool keep_hpd = intel_hpd_can_generate_pme(display);

	bool reset_hpd = !intel_hpd_can_generate_pme(display);

IMO this would be more clear.

> -	intel_de_write(display, GEN11_DISPLAY_INT_CTL, 0);
> +	if (!keep_hpd)
> +		intel_de_write(display, GEN11_DISPLAY_INT_CTL, 0);
>  
>  	if (DISPLAY_VER(display) >= 12) {
>  		enum transcoder trans;
> @@ -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);
> +	}
>  }
>  
>  void gen8_irq_power_well_post_enable(struct intel_display *display,

-- 
Jani Nikula, Intel