Re: [PATCH 15/15] drm/i915/irq: add intel_display_irq_handler() to irq funcs

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 30, 2026 at 10:59:11AM +0300, Jani Nikula wrote:
> On Wed, 29 Apr 2026, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Apr 29, 2026 at 01:24:55PM +0300, Jani Nikula wrote:
> >> @@ -2088,6 +2123,28 @@ static void vlv_display_irq_ack(struct intel_display *display,
> >>  	i9xx_pipestat_irq_ack(display, state->iir, state->pipe_stats);
> >>  }
> >>  
> >> +static bool vlv_display_irq_handler(struct intel_display *display,
> >> +				    const struct intel_display_irq_state *state)
> >> +{
> >> +	u32 lpe_mask = I915_LPE_PIPE_A_INTERRUPT | I915_LPE_PIPE_B_INTERRUPT;
> >> +
> >> +	if (display->platform.cherryview)
> >> +		lpe_mask |= I915_LPE_PIPE_C_INTERRUPT;
> >
> > I would prefer a function rather than the extra variable. The other
> > option is to just use the CHV mask always. There is nothing on the
> > extra bit on VLV so we never unmask it.
> 
> Hmm. reset and postinstall also have slight variations for VLV vs. CHV,
> but only very slight. I think they're overall close enough that
> splitting out a funcs struct and separate funcs for CHV is more
> distracting and duplication than helpful.

I think with DPINVGTT we could just switch to using full 16bit masks
and get rid of the vlv/chv difference. I should check if the unused
bits are all tied to 0...

> 
> Or would something like this be satisfactory? Just a couple of small
> wrapper functions around intel_lpe_audio_irq_handler() here that the
> compiler would just inline:
> 
> 	if (display->platform.cherryview)
> 		chv_lpe_audio_irq_handler(display, state->iir);		
> 	else
> 		vlv_lpe_audio_irq_handler(display, state->iir);		
> 
> Of course this would need to move to the ack part per your other review
> comment.

I was just thinking of having something like 
vlv_lpe_irq_mask()
{
	if (chv)
		return ...;
	else
		return ...;
}

> 
> 
> >> @@ -51,14 +51,12 @@ void bdw_disable_vblank(struct drm_crtc *crtc);
> >>  
> >>  void ilk_display_irq_master_disable(struct intel_display *display, u32 *de_ier, u32 *sde_ier);
> >>  void ilk_display_irq_master_enable(struct intel_display *display, u32 de_ier, u32 sde_ier);
> >> -bool ilk_display_irq_handler(struct intel_display *display);
> >> -void gen8_de_irq_handler(struct intel_display *display, u32 master_ctl);
> >> -void gen11_display_irq_handler(struct intel_display *display);
> >>  
> >>  u32 gen11_gu_misc_irq_ack(struct intel_display *display, const u32 master_ctl);
> >>  void gen11_gu_misc_irq_handler(struct intel_display *display, const u32 iir);
> >>  
> >>  struct intel_display_irq_state {
> >> +	u32 master_ctl;
> >
> > Ideally I'd like a separate structs for different platforms,
> > but until I resurrect my old ack vs. handle split for all
> > platforms I guess we don't need anything else here for
> > ilk+. So good enough for now I suppose.
> 
> You mean per-platform substructs/unions within struct
> intel_display_irq_state? I can do that.

Yeah something like that. Not super important right since
we just have the master_ctl for non-gmch platforms.

-- 
Ville Syrjälä
Intel