Re: [PATCH v2 1/3] drm/{i915,xe}/display: duplicate gen2 irq/error init/reset in display irq

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

On Tue, Nov 11, 2025 at 09:34:02AM +0200, Jani Nikula wrote:
> Duplicate gen2_irq_reset(), gen2_assert_iir_is_zero(), gen2_irq_init(),
> gen2_error_reset(), and gen2_error_init() in intel_display_irq.c.
> 
> This allows us to drop the duplicates from xe, and prepares for future
> cleanups. Although duplication is undesirable in general, in this case
> the local duplicates lead to a cleaner end result.
> 
> There's a slight wrinkle in gen2_assert_iir_is_zero(). We need to use
> non-device based logging until we pass in struct intel_display in a
> separate change.
> 
> v2:
> - Keep xe compat stuff due to series reorder and rebase
> - Keep the WARN as regular WARN
> - Rename the functions in the same go
> 
> Suggested-by: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> ---
> 
> Note: 'git show --color-moved' helps review
> ---
>  .../gpu/drm/i915/display/intel_display_irq.c  | 82 +++++++++++++++++--
>  drivers/gpu/drm/xe/display/ext/i915_irq.c     | 67 ---------------
>  2 files changed, 73 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> index 43b27deb4a26..acfaff13c3ba 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> @@ -33,6 +33,72 @@
>  #include "intel_psr_regs.h"
>  #include "intel_uncore.h"
>  
> +static void irq_reset(struct intel_uncore *uncore, struct i915_irq_regs regs)
> +{
> +	intel_uncore_write(uncore, regs.imr, 0xffffffff);
> +	intel_uncore_posting_read(uncore, regs.imr);
> +
> +	intel_uncore_write(uncore, regs.ier, 0);
> +
> +	/* IIR can theoretically queue up two events. Be paranoid. */
> +	intel_uncore_write(uncore, regs.iir, 0xffffffff);
> +	intel_uncore_posting_read(uncore, regs.iir);
> +	intel_uncore_write(uncore, regs.iir, 0xffffffff);
> +	intel_uncore_posting_read(uncore, regs.iir);
> +}
> +
> +/*
> + * We should clear IMR at preinstall/uninstall, and just check at postinstall.
> + */
> +static void assert_iir_is_zero(struct intel_uncore *uncore, i915_reg_t reg)
> +{
> +	u32 val = intel_uncore_read(uncore, reg);
> +
> +	if (val == 0)
> +		return;
> +
> +	WARN(1,
> +		 "Interrupt register 0x%x is not zero: 0x%08x\n",
> +		 i915_mmio_reg_offset(reg), val);
> +	intel_uncore_write(uncore, reg, 0xffffffff);
> +	intel_uncore_posting_read(uncore, reg);
> +	intel_uncore_write(uncore, reg, 0xffffffff);
> +	intel_uncore_posting_read(uncore, reg);
> +}
> +
> +static void irq_init(struct intel_uncore *uncore, struct i915_irq_regs regs,
> +		     u32 imr_val, u32 ier_val)
> +{
> +	assert_iir_is_zero(uncore, regs.iir);
> +
> +	intel_uncore_write(uncore, regs.ier, ier_val);
> +	intel_uncore_write(uncore, regs.imr, imr_val);
> +	intel_uncore_posting_read(uncore, regs.imr);
> +}
> +
> +static void error_reset(struct intel_uncore *uncore, struct i915_error_regs regs)
> +{
> +	intel_uncore_write(uncore, regs.emr, 0xffffffff);
> +	intel_uncore_posting_read(uncore, regs.emr);
> +
> +	intel_uncore_write(uncore, regs.eir, 0xffffffff);
> +	intel_uncore_posting_read(uncore, regs.eir);
> +	intel_uncore_write(uncore, regs.eir, 0xffffffff);
> +	intel_uncore_posting_read(uncore, regs.eir);
> +}
> +
> +static void error_init(struct intel_uncore *uncore, struct i915_error_regs regs,
> +		       u32 emr_val)
> +{
> +	intel_uncore_write(uncore, regs.eir, 0xffffffff);
> +	intel_uncore_posting_read(uncore, regs.eir);
> +	intel_uncore_write(uncore, regs.eir, 0xffffffff);
> +	intel_uncore_posting_read(uncore, regs.eir);
> +
> +	intel_uncore_write(uncore, regs.emr, emr_val);
> +	intel_uncore_posting_read(uncore, regs.emr);
> +}
> +
>  static void
>  intel_display_irq_regs_init(struct intel_display *display, struct i915_irq_regs regs,
>  			    u32 imr_val, u32 ier_val)
> @@ -41,7 +107,7 @@ intel_display_irq_regs_init(struct intel_display *display, struct i915_irq_regs
>  	intel_dmc_wl_get(display, regs.ier);
>  	intel_dmc_wl_get(display, regs.iir);
>  
> -	gen2_irq_init(to_intel_uncore(display->drm), regs, imr_val, ier_val);
> +	irq_init(to_intel_uncore(display->drm), regs, imr_val, ier_val);
>  
>  	intel_dmc_wl_put(display, regs.iir);
>  	intel_dmc_wl_put(display, regs.ier);
> @@ -55,7 +121,7 @@ intel_display_irq_regs_reset(struct intel_display *display, struct i915_irq_regs
>  	intel_dmc_wl_get(display, regs.ier);
>  	intel_dmc_wl_get(display, regs.iir);
>  
> -	gen2_irq_reset(to_intel_uncore(display->drm), regs);
> +	irq_reset(to_intel_uncore(display->drm), regs);
>  
>  	intel_dmc_wl_put(display, regs.iir);
>  	intel_dmc_wl_put(display, regs.ier);
> @@ -67,7 +133,7 @@ intel_display_irq_regs_assert_irr_is_zero(struct intel_display *display, i915_re
>  {
>  	intel_dmc_wl_get(display, reg);
>  
> -	gen2_assert_iir_is_zero(to_intel_uncore(display->drm), reg);
> +	assert_iir_is_zero(to_intel_uncore(display->drm), reg);
>  
>  	intel_dmc_wl_put(display, reg);
>  }
> @@ -1918,8 +1984,7 @@ static void _vlv_display_irq_reset(struct intel_display *display)
>  	else
>  		intel_de_write(display, DPINVGTT, DPINVGTT_STATUS_MASK_VLV);
>  
> -	gen2_error_reset(to_intel_uncore(display->drm),
> -			 VLV_ERROR_REGS);
> +	error_reset(to_intel_uncore(display->drm), VLV_ERROR_REGS);
>  
>  	i915_hotplug_interrupt_update_locked(display, 0xffffffff, 0);
>  	intel_de_rmw(display, PORT_HOTPLUG_STAT(display), 0, 0);
> @@ -2014,8 +2079,7 @@ static void _vlv_display_irq_postinstall(struct intel_display *display)
>  			       DPINVGTT_STATUS_MASK_VLV |
>  			       DPINVGTT_EN_MASK_VLV);
>  
> -	gen2_error_init(to_intel_uncore(display->drm),
> -			VLV_ERROR_REGS, ~vlv_error_mask());
> +	error_init(to_intel_uncore(display->drm), VLV_ERROR_REGS, ~vlv_error_mask());
>  
>  	pipestat_mask = PIPE_CRC_DONE_INTERRUPT_STATUS;
>  
> @@ -2054,7 +2118,7 @@ static void ibx_display_irq_reset(struct intel_display *display)
>  	if (HAS_PCH_NOP(display))
>  		return;
>  
> -	gen2_irq_reset(to_intel_uncore(display->drm), SDE_IRQ_REGS);
> +	irq_reset(to_intel_uncore(display->drm), SDE_IRQ_REGS);
>  
>  	if (HAS_PCH_CPT(display) || HAS_PCH_LPT(display))
>  		intel_de_write(display, SERR_INT, 0xffffffff);
> @@ -2064,7 +2128,7 @@ void ilk_display_irq_reset(struct intel_display *display)
>  {
>  	struct intel_uncore *uncore = to_intel_uncore(display->drm);
>  
> -	gen2_irq_reset(uncore, DE_IRQ_REGS);
> +	irq_reset(uncore, DE_IRQ_REGS);
>  	display->irq.ilk_de_imr_mask = ~0u;
>  
>  	if (DISPLAY_VER(display) == 7)
> diff --git a/drivers/gpu/drm/xe/display/ext/i915_irq.c b/drivers/gpu/drm/xe/display/ext/i915_irq.c
> index 3c6bca66ddab..1011c1c754d0 100644
> --- a/drivers/gpu/drm/xe/display/ext/i915_irq.c
> +++ b/drivers/gpu/drm/xe/display/ext/i915_irq.c
> @@ -7,73 +7,6 @@
>  #include "i915_reg.h"
>  #include "intel_uncore.h"
>  
> -void gen2_irq_reset(struct intel_uncore *uncore, struct i915_irq_regs regs)
> -{
> -	intel_uncore_write(uncore, regs.imr, 0xffffffff);
> -	intel_uncore_posting_read(uncore, regs.imr);
> -
> -	intel_uncore_write(uncore, regs.ier, 0);
> -
> -	/* IIR can theoretically queue up two events. Be paranoid. */
> -	intel_uncore_write(uncore, regs.iir, 0xffffffff);
> -	intel_uncore_posting_read(uncore, regs.iir);
> -	intel_uncore_write(uncore, regs.iir, 0xffffffff);
> -	intel_uncore_posting_read(uncore, regs.iir);
> -}
> -
> -/*
> - * We should clear IMR at preinstall/uninstall, and just check at postinstall.
> - */
> -void gen2_assert_iir_is_zero(struct intel_uncore *uncore, i915_reg_t reg)
> -{
> -	struct xe_device *xe = container_of(uncore, struct xe_device, uncore);
> -	u32 val = intel_uncore_read(uncore, reg);
> -
> -	if (val == 0)
> -		return;
> -
> -	drm_WARN(&xe->drm, 1,
> -		 "Interrupt register 0x%x is not zero: 0x%08x\n",
> -		 i915_mmio_reg_offset(reg), val);
> -	intel_uncore_write(uncore, reg, 0xffffffff);
> -	intel_uncore_posting_read(uncore, reg);
> -	intel_uncore_write(uncore, reg, 0xffffffff);
> -	intel_uncore_posting_read(uncore, reg);
> -}
> -
> -void gen2_irq_init(struct intel_uncore *uncore, struct i915_irq_regs regs,
> -		   u32 imr_val, u32 ier_val)
> -{
> -	gen2_assert_iir_is_zero(uncore, regs.iir);
> -
> -	intel_uncore_write(uncore, regs.ier, ier_val);
> -	intel_uncore_write(uncore, regs.imr, imr_val);
> -	intel_uncore_posting_read(uncore, regs.imr);
> -}
> -
> -void gen2_error_reset(struct intel_uncore *uncore, struct i915_error_regs regs)
> -{
> -	intel_uncore_write(uncore, regs.emr, 0xffffffff);
> -	intel_uncore_posting_read(uncore, regs.emr);
> -
> -	intel_uncore_write(uncore, regs.eir, 0xffffffff);
> -	intel_uncore_posting_read(uncore, regs.eir);
> -	intel_uncore_write(uncore, regs.eir, 0xffffffff);
> -	intel_uncore_posting_read(uncore, regs.eir);
> -}
> -
> -void gen2_error_init(struct intel_uncore *uncore, struct i915_error_regs regs,
> -		     u32 emr_val)
> -{
> -	intel_uncore_write(uncore, regs.eir, 0xffffffff);
> -	intel_uncore_posting_read(uncore, regs.eir);
> -	intel_uncore_write(uncore, regs.eir, 0xffffffff);
> -	intel_uncore_posting_read(uncore, regs.eir);
> -
> -	intel_uncore_write(uncore, regs.emr, emr_val);
> -	intel_uncore_posting_read(uncore, regs.emr);
> -}
> -
>  bool intel_irqs_enabled(struct xe_device *xe)
>  {
>  	return atomic_read(&xe->irq.enabled);
> -- 
> 2.47.3

-- 
Ville Syrjälä
Intel