Re: [PATCH 6/6] drm/i915/irq: split ILK display irq handling

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

On Fri, Sep 19, 2025 at 12:51:49PM +0300, Jani Nikula wrote:
> Split out display irq handling on ilk. Since the master IRQ enable is in
> DEIIR, we'll need to do this in two parts. First, add
> ilk_display_irq_master_disable() to disable master and south interrupts,
> and second, add (repurposed) ilk_display_irq_handler() to finish display
> irq handling.
> 
> It's not the prettiest thing you ever saw, but improves separation of
> display irq handling. And removes HAS_PCH_NOP() and DISPLAY_VER() checks
> from core irq code.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_display_irq.c  | 48 ++++++++++++++++++-
>  .../gpu/drm/i915/display/intel_display_irq.h  |  4 +-
>  drivers/gpu/drm/i915/i915_irq.c               | 30 ++----------
>  3 files changed, 52 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> index 4d51900123ea..c2320c1718f7 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> @@ -872,7 +872,7 @@ static void ilk_gtt_fault_irq_handler(struct intel_display *display)
>  	}
>  }
>  
> -void ilk_display_irq_handler(struct intel_display *display, u32 de_iir)
> +static void _ilk_display_irq_handler(struct intel_display *display, u32 de_iir)
>  {
>  	enum pipe pipe;
>  	u32 hotplug_trigger = de_iir & DE_DP_A_HOTPLUG;
> @@ -923,7 +923,7 @@ void ilk_display_irq_handler(struct intel_display *display, u32 de_iir)
>  		ilk_display_rps_irq_handler(display);
>  }
>  
> -void ivb_display_irq_handler(struct intel_display *display, u32 de_iir)
> +static void _ivb_display_irq_handler(struct intel_display *display, u32 de_iir)
>  {
>  	enum pipe pipe;
>  	u32 hotplug_trigger = de_iir & DE_DP_A_HOTPLUG_IVB;
> @@ -972,6 +972,50 @@ void ivb_display_irq_handler(struct intel_display *display, u32 de_iir)
>  	}
>  }
>  
> +void ilk_display_irq_master_disable(struct intel_display *display, u32 *de_ier, u32 *sde_ier)
> +{
> +	/* disable master interrupt before clearing iir  */
> +	*de_ier = intel_de_read(display, DEIER);
> +	intel_de_write(display, DEIER, *de_ier & ~DE_MASTER_IRQ_CONTROL);
> +
> +	/*
> +	 * Disable south interrupts. We'll only write to SDEIIR once, so further
> +	 * interrupts will be stored on its back queue, and then we'll be able
> +	 * to process them after we restore SDEIER (as soon as we restore it,
> +	 * we'll get an interrupt if SDEIIR still has something to process due
> +	 * to its back queue).
> +	 */
> +	if (!HAS_PCH_NOP(display)) {
> +		*sde_ier = intel_de_read(display, SDEIER);
> +		intel_de_write(display, SDEIER, 0);
> +	} else {
> +		*sde_ier = 0;
> +	}
> +}
> +
> +bool ilk_display_irq_handler(struct intel_display *display, u32 de_ier, u32 sde_ier)
> +{
> +	u32 de_iir;
> +	bool handled = false;
> +
> +	de_iir = intel_de_read(display, DEIIR);
> +	if (de_iir) {
> +		intel_de_write(display, DEIIR, de_iir);
> +		if (DISPLAY_VER(display) >= 7)
> +			_ivb_display_irq_handler(display, de_iir);
> +		else
> +			_ilk_display_irq_handler(display, de_iir);
> +		handled = true;
> +	}
> +
> +	intel_de_write(display, DEIER, de_ier);
> +
> +	if (sde_ier)
> +		intel_de_write(display, SDEIER, sde_ier);

Maybe the re-enable should be its own function just to make
things a bit less confusing?

> +
> +	return handled;
> +}
> +
>  static u32 gen8_de_port_aux_mask(struct intel_display *display)
>  {
>  	u32 mask;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.h b/drivers/gpu/drm/i915/display/intel_display_irq.h
> index e44d88e0d7e7..778195bd6052 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.h
> @@ -47,8 +47,8 @@ void i965_disable_vblank(struct drm_crtc *crtc);
>  void ilk_disable_vblank(struct drm_crtc *crtc);
>  void bdw_disable_vblank(struct drm_crtc *crtc);
>  
> -void ivb_display_irq_handler(struct intel_display *display, u32 de_iir);
> -void ilk_display_irq_handler(struct intel_display *display, u32 de_iir);
> +void ilk_display_irq_master_disable(struct intel_display *display, u32 *de_ier, u32 *sde_ier);
> +bool ilk_display_irq_handler(struct intel_display *display, u32 de_ier, u32 sde_ier);
>  void gen8_de_irq_handler(struct intel_display *display, u32 master_ctl);
>  void gen11_display_irq_handler(struct intel_display *display);
>  
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 312f7e42931a..65aa35866a5a 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -414,7 +414,7 @@ static irqreturn_t ilk_irq_handler(int irq, void *arg)
>  	struct drm_i915_private *i915 = arg;
>  	struct intel_display *display = i915->display;
>  	void __iomem * const regs = intel_uncore_regs(&i915->uncore);
> -	u32 de_iir, gt_iir, de_ier, sde_ier = 0;
> +	u32 gt_iir, de_ier = 0, sde_ier = 0;
>  	irqreturn_t ret = IRQ_NONE;
>  
>  	if (unlikely(!intel_irqs_enabled(i915)))
> @@ -423,19 +423,8 @@ static irqreturn_t ilk_irq_handler(int irq, void *arg)
>  	/* IRQs are synced during runtime_suspend, we don't require a wakeref */
>  	disable_rpm_wakeref_asserts(&i915->runtime_pm);
>  
> -	/* disable master interrupt before clearing iir  */
> -	de_ier = raw_reg_read(regs, DEIER);
> -	raw_reg_write(regs, DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
> -
> -	/* Disable south interrupts. We'll only write to SDEIIR once, so further
> -	 * interrupts will will be stored on its back queue, and then we'll be
> -	 * able to process them after we restore SDEIER (as soon as we restore
> -	 * it, we'll get an interrupt if SDEIIR still has something to process
> -	 * due to its back queue). */
> -	if (!HAS_PCH_NOP(display)) {
> -		sde_ier = raw_reg_read(regs, SDEIER);
> -		raw_reg_write(regs, SDEIER, 0);
> -	}
> +	/* Disable master and south interrupts */
> +	ilk_display_irq_master_disable(display, &de_ier, &sde_ier);
>  
>  	/* Find, clear, then process each source of interrupt */
>  
> @@ -458,19 +447,8 @@ static irqreturn_t ilk_irq_handler(int irq, void *arg)
>  		}
>  	}
>  
> -	de_iir = raw_reg_read(regs, DEIIR);
> -	if (de_iir) {
> -		raw_reg_write(regs, DEIIR, de_iir);
> -		if (DISPLAY_VER(display) >= 7)
> -			ivb_display_irq_handler(display, de_iir);
> -		else
> -			ilk_display_irq_handler(display, de_iir);
> +	if (ilk_display_irq_handler(display, de_ier, sde_ier))
>  		ret = IRQ_HANDLED;
> -	}
> -
> -	raw_reg_write(regs, DEIER, de_ier);
> -	if (sde_ier)
> -		raw_reg_write(regs, SDEIER, sde_ier);
>  
>  	pmu_irq_stats(i915, ret);
>  
> -- 
> 2.47.3

-- 
Ville Syrjälä
Intel