Re: [PATCH 3/4] drm/i915: Use intel_de_{read, write})fw() in VLV/CHV irq handlers

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 22 Oct 2026, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> MMIO is rather expensive on VLV and CHV. MMIO read in particular
> seems to take around 450 ns on CHV. VLV is a bit better
> in the 200-300 ns range. The use of the non-fw() register
> accessors means that each actual register access is bracketed
> by two CLAIM_ER reads. I'm thinking we don't want that extra
> cost in the irq handlers, so switch over to the fw() accessors.
>
> On VLV/CHV this also means we lose the uncore.lock protection
> around the MMIO accesses, but that is fine as these platforms
> do not need any serialization of parallel MMIO accesses.
>
> This actually changes all GMCH platofors since the irq

*platforms

> handlers are shared between the platforms. But that doesn't
> actually cause significant functional changes on the older
> platforms as the non-fw() accessors are identical to the fw()
> versions, apart from some rpm asserts.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Subject needs one s/)/_/

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  .../gpu/drm/i915/display/intel_display_irq.c  | 32 +++++++++----------
>  .../gpu/drm/i915/display/intel_hotplug_irq.c  |  8 ++---
>  2 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_irq.c b/drivers/gpu/drm/i915/display/intel_display_irq.c
> index d2d45767bfd2..25b899990475 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_irq.c
> @@ -501,19 +501,19 @@ static void i9xx_pipe_crc_irq_handler(struct intel_display *display,
>  	u32 res1, res2;
>  
>  	if (DISPLAY_VER(display) >= 3)
> -		res1 = intel_de_read(display, PIPE_CRC_RES_RES1_I915(display, pipe));
> +		res1 = intel_de_read_fw(display, PIPE_CRC_RES_RES1_I915(display, pipe));
>  	else
>  		res1 = 0;
>  
>  	if (DISPLAY_VER(display) >= 5 || display->platform.g4x)
> -		res2 = intel_de_read(display, PIPE_CRC_RES_RES2_G4X(display, pipe));
> +		res2 = intel_de_read_fw(display, PIPE_CRC_RES_RES2_G4X(display, pipe));
>  	else
>  		res2 = 0;
>  
>  	display_pipe_crc_irq_handler(display, pipe,
> -				     intel_de_read(display, PIPE_CRC_RES_RED(display, pipe)),
> -				     intel_de_read(display, PIPE_CRC_RES_GREEN(display, pipe)),
> -				     intel_de_read(display, PIPE_CRC_RES_BLUE(display, pipe)),
> +				     intel_de_read_fw(display, PIPE_CRC_RES_RED(display, pipe)),
> +				     intel_de_read_fw(display, PIPE_CRC_RES_GREEN(display, pipe)),
> +				     intel_de_read_fw(display, PIPE_CRC_RES_BLUE(display, pipe)),
>  				     res1, res2);
>  }
>  
> @@ -577,7 +577,7 @@ static void i9xx_pipestat_irq_ack(struct intel_display *display,
>  			continue;
>  
>  		reg = PIPESTAT(display, pipe);
> -		pipe_stats[pipe] = intel_de_read(display, reg) & status_mask;
> +		pipe_stats[pipe] = intel_de_read_fw(display, reg) & status_mask;
>  		enable_mask = i915_pipestat_enable_mask(display, pipe);
>  
>  		/*
> @@ -590,8 +590,8 @@ static void i9xx_pipestat_irq_ack(struct intel_display *display,
>  		 * an interrupt is still pending.
>  		 */
>  		if (pipe_stats[pipe]) {
> -			intel_de_write(display, reg, pipe_stats[pipe]);
> -			intel_de_write(display, reg, enable_mask);
> +			intel_de_write_fw(display, reg, pipe_stats[pipe]);
> +			intel_de_write_fw(display, reg, enable_mask);
>  		}
>  	}
>  	spin_unlock(&display->irq.lock);
> @@ -1893,7 +1893,7 @@ static void vlv_page_table_error_irq_ack(struct intel_display *display, u32 *dpi
>  {
>  	u32 status, enable, tmp;
>  
> -	tmp = intel_de_read(display, DPINVGTT);
> +	tmp = intel_de_read_fw(display, DPINVGTT);
>  
>  	enable = tmp >> 16;
>  	status = tmp & 0xffff;
> @@ -1909,8 +1909,8 @@ static void vlv_page_table_error_irq_ack(struct intel_display *display, u32 *dpi
>  	enable &= ~status;
>  
>  	/* customary ack+disable then re-enable to guarantee an edge */
> -	intel_de_write(display, DPINVGTT, status);
> -	intel_de_write(display, DPINVGTT, enable << 16);
> +	intel_de_write_fw(display, DPINVGTT, status);
> +	intel_de_write_fw(display, DPINVGTT, enable << 16);
>  }
>  
>  static void vlv_page_table_error_irq_handler(struct intel_display *display, u32 dpinvgtt)
> @@ -1932,21 +1932,21 @@ static void vlv_display_error_irq_ack(struct intel_display *display,
>  {
>  	u32 emr;
>  
> -	*eir = intel_de_read(display, VLV_EIR);
> +	*eir = intel_de_read_fw(display, VLV_EIR);
>  
>  	if (*eir & VLV_ERROR_PAGE_TABLE)
>  		vlv_page_table_error_irq_ack(display, dpinvgtt);
>  
> -	intel_de_write(display, VLV_EIR, *eir);
> +	intel_de_write_fw(display, VLV_EIR, *eir);
>  
>  	/*
>  	 * Toggle all EMR bits to make sure we get an edge
>  	 * in the ISR master error bit if we don't clear
>  	 * all the EIR bits.
>  	 */
> -	emr = intel_de_read(display, VLV_EMR);
> -	intel_de_write(display, VLV_EMR, 0xffffffff);
> -	intel_de_write(display, VLV_EMR, emr);
> +	emr = intel_de_read_fw(display, VLV_EMR);
> +	intel_de_write_fw(display, VLV_EMR, 0xffffffff);
> +	intel_de_write_fw(display, VLV_EMR, emr);
>  }
>  
>  static void vlv_display_error_irq_handler(struct intel_display *display,
> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c
> index 913fcd2a1723..ce0c8f56106a 100644
> --- a/drivers/gpu/drm/i915/display/intel_hotplug_irq.c
> +++ b/drivers/gpu/drm/i915/display/intel_hotplug_irq.c
> @@ -439,15 +439,15 @@ u32 i9xx_hpd_irq_ack(struct intel_display *display)
>  	 * bits can itself generate a new hotplug interrupt :(
>  	 */
>  	for (i = 0; i < 10; i++) {
> -		u32 tmp = intel_de_read(display,
> -					PORT_HOTPLUG_STAT(display)) & hotplug_status_mask;
> +		u32 tmp = intel_de_read_fw(display,
> +					   PORT_HOTPLUG_STAT(display)) & hotplug_status_mask;
>  
>  		if (tmp == 0)
>  			return hotplug_status;
>  
>  		hotplug_status |= tmp;
> -		intel_de_write(display, PORT_HOTPLUG_STAT(display),
> -			       hotplug_status);
> +		intel_de_write_fw(display, PORT_HOTPLUG_STAT(display),
> +				  hotplug_status);
>  	}
>  
>  	drm_WARN_ONCE(display->drm, 1,

-- 
Jani Nikula, Intel