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

Jani Nikula
2026-04-30T07:59:26Z

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.

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.

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

BR,
Jani.

--
Jani Nikula, Intel