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

Ville Syrjälä
2026-04-30T10:28:16Z

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