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

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

On Fri, 2026-09-04 at 12:56 +0300, Jani Nikula wrote:
> 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.

Ack. Forgot to remove that!


> 
> 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.

Okay. Will update.

BR
vinod

> 
> > -	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,
>