Re: [PATCH 5/9] drm/i915/de: Introduce display->mmio.lock

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

On Thu, 05 Nov 2026, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Introduce our own display specific mmio.lock, and wrap
> the more expensive non-fw register accesses with it. And
> everything that explicitly takes the uncore.lock currently,
> switch over to mmio.lock as well.
>
> This will preserve the same serialization behavior between
> display registers as the uncore.lock is currently providing.
> Once uncore.lock has actually been eliminated from display
> registers (not the case yet) display vs. non-display
> register accesses could happen in parallel. But that should
> be fine since generally the registers for different hardware
> units don't live on the same cacheline, so even concerns
> about parallel accesses to the same cacheline potentially
> hanging the machine (on IVB/HSW) should be assuaged.
>
> Currently uncore.lock usage is limited to SNB+ on i915.
> The xe driver, and older platforms on the i915 driver
> don't use uncore.lock. Follow the same rule for the mmio.lock
> initially.
>
> With this we now end up with a double lock, or even a
> triple lock due to i915's mmio debug lock. But we'll eliminate
> the extra locks shortly. The end goal is either just mmio.lock
> or no locks whatsoever. Well, ignoring the DMC wakelock stuff
> that uses an additional lock as well.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

> ---
>  drivers/gpu/drm/i915/display/i9xx_wm.c        |  9 ++-
>  drivers/gpu/drm/i915/display/intel_de.c       | 69 +++++++++++++++++--
>  .../gpu/drm/i915/display/intel_display_core.h |  9 +++
>  drivers/gpu/drm/i915/display/intel_vblank.c   | 16 ++---
>  4 files changed, 83 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/i9xx_wm.c b/drivers/gpu/drm/i915/display/i9xx_wm.c
> index 0233f5053d95..46bfada7c060 100644
> --- a/drivers/gpu/drm/i915/display/i9xx_wm.c
> +++ b/drivers/gpu/drm/i915/display/i9xx_wm.c
> @@ -1863,7 +1863,6 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>  				   struct intel_crtc *crtc)
>  {
>  	struct intel_display *display = to_intel_display(crtc);
> -	struct intel_uncore *uncore = to_intel_uncore(display->drm);
>  	const struct intel_crtc_state *crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
>  	const struct vlv_fifo_state *fifo_state =
> @@ -1884,15 +1883,15 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>  	trace_vlv_fifo_size(crtc, sprite0_start, sprite1_start, fifo_size);
>  
>  	/*
> -	 * uncore.lock serves a double purpose here. It allows us to
> -	 * use the less expensive I915_{READ,WRITE}_FW() functions, and
> +	 * display->mmio.lock serves a double purpose here. It allows us to
> +	 * use the less expensive intel_de_{read,write}_fw() functions, and
>  	 * it protects the DSPARB registers from getting clobbered by
>  	 * parallel updates from multiple pipes.
>  	 *
>  	 * intel_pipe_update_start() has already disabled interrupts
>  	 * for us, so a plain spin_lock() is sufficient here.
>  	 */
> -	spin_lock(&uncore->lock);
> +	spin_lock(&display->mmio.lock);
>  
>  	switch (crtc->pipe) {
>  	case PIPE_A:
> @@ -1952,7 +1951,7 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>  
>  	intel_de_read_fw(display, DSPARB(display));
>  
> -	spin_unlock(&uncore->lock);
> +	spin_unlock(&display->mmio.lock);
>  }
>  
>  #undef VLV_FIFO
> diff --git a/drivers/gpu/drm/i915/display/intel_de.c b/drivers/gpu/drm/i915/display/intel_de.c
> index 66de0d6be569..8b7a4e696ff9 100644
> --- a/drivers/gpu/drm/i915/display/intel_de.c
> +++ b/drivers/gpu/drm/i915/display/intel_de.c
> @@ -164,6 +164,7 @@ u16 intel_de_read16(struct intel_display *display, intel_reg_t reg)
>  }
>  
>  #define __MMIO_WAKELOCK		BIT(0)
> +#define __MMIO_LOCK		BIT(1)
>  
>  static __always_inline void
>  __mmio_wakelock_pre(struct intel_display *display, intel_reg_t reg,
> @@ -181,16 +182,35 @@ __mmio_wakelock_post(struct intel_display *display, intel_reg_t reg,
>  		intel_dmc_wl_put(display, reg);
>  }
>  
> +static __always_inline void
> +__mmio_lock_pre(struct intel_display *display, unsigned long *irqflags,
> +		const unsigned int mmio_flags)
> +{
> +	if (mmio_flags & __MMIO_LOCK)
> +		spin_lock_irqsave(&display->mmio.lock, *irqflags);
> +}
> +
> +static __always_inline void
> +__mmio_lock_post(struct intel_display *display, unsigned long irqflags,
> +		 const unsigned int mmio_flags)
> +{
> +	if (mmio_flags & __MMIO_LOCK)
> +		spin_unlock_irqrestore(&display->mmio.lock, irqflags);
> +}
> +
>  static __always_inline u32
>  __intel_de_read(struct intel_display *display, intel_reg_t reg,
>  		const unsigned int mmio_flags)
>  {
> +	unsigned long irqflags;
>  	u32 val;
>  
>  	__mmio_wakelock_pre(display, reg, mmio_flags);
> +	__mmio_lock_pre(display, &irqflags, mmio_flags);
>  
>  	val = intel_uncore_read(__to_uncore(display), reg);
>  
> +	__mmio_lock_post(display, irqflags, mmio_flags);
>  	__mmio_wakelock_post(display, reg, mmio_flags);
>  
>  	return val;
> @@ -200,10 +220,14 @@ static __always_inline void
>  __intel_de_write(struct intel_display *display, intel_reg_t reg, u32 val,
>  		 const unsigned int mmio_flags)
>  {
> +	unsigned long irqflags;
> +
>  	__mmio_wakelock_pre(display, reg, mmio_flags);
> +	__mmio_lock_pre(display, &irqflags, mmio_flags);
>  
>  	intel_uncore_write(__to_uncore(display), reg, val);
>  
> +	__mmio_lock_post(display, irqflags, mmio_flags);
>  	__mmio_wakelock_post(display, reg, mmio_flags);
>  }
>  
> @@ -226,6 +250,25 @@ static const struct intel_mmio_funcs i9xx_mmio_funcs = {
>  	.write = i9xx_mmio_write,
>  };
>  
> +#define SNB_MMIO_FLAGS __MMIO_LOCK
> +
> +static u32 snb_mmio_read(struct intel_display *display, intel_reg_t reg)
> +{
> +	return __intel_de_read(display, reg, SNB_MMIO_FLAGS);
> +}
> +
> +static void snb_mmio_write(struct intel_display *display, intel_reg_t reg, u32 val)
> +{
> +	__intel_de_write(display, reg, val, SNB_MMIO_FLAGS);
> +}
> +
> +#undef SNB_MMIO_FLAGS
> +
> +static const struct intel_mmio_funcs snb_mmio_funcs = {
> +	.read = snb_mmio_read,
> +	.write = snb_mmio_write,
> +};
> +
>  #define LNL_MMIO_FLAGS __MMIO_WAKELOCK
>  
>  static u32 lnl_mmio_read(struct intel_display *display, intel_reg_t reg)
> @@ -245,12 +288,30 @@ static const struct intel_mmio_funcs lnl_mmio_funcs = {
>  	.write = lnl_mmio_write,
>  };
>  
> +static bool driver_is_i915(void)
> +{
> +#ifdef I915
> +	return true;
> +#else
> +	return false;
> +#endif
> +}
> +
>  void intel_de_init(struct intel_display *display)
>  {
>  	display->mmio.uncore = to_intel_uncore(display->drm);
>  
> -	if (DISPLAY_VER(display) >= 20)
> -		display->mmio.funcs = &lnl_mmio_funcs;
> -	else
> -		display->mmio.funcs = &i9xx_mmio_funcs;
> +	spin_lock_init(&display->mmio.lock);
> +
> +	if (driver_is_i915()) {
> +		if (DISPLAY_VER(display) >= 6)
> +			display->mmio.funcs = &snb_mmio_funcs;
> +		else
> +			display->mmio.funcs = &i9xx_mmio_funcs;
> +	} else {
> +		if (DISPLAY_VER(display) >= 20)
> +			display->mmio.funcs = &lnl_mmio_funcs;
> +		else
> +			display->mmio.funcs = &i9xx_mmio_funcs;
> +	}
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
> index 3a48119f7890..aded2af8c92a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> @@ -533,6 +533,15 @@ struct intel_display {
>  	} modeset;
>  
>  	struct {
> +		/*
> +		 * Simultaneous accesses to the same cacheline of
> +		 * registers may hang IVB/HSW systems. This lock
> +		 * serializes all display register accesses done
> +		 * using intel_de_{read,write}(), but not those
> +		 * done using intel_de_{read,write}_fw().
> +		 */
> +		spinlock_t lock;
> +
>  		struct intel_uncore *uncore;
>  		const struct intel_mmio_funcs *funcs;
>  	} mmio;
> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 1d4753605675..fc9d1e14ec87 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -281,10 +281,6 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  }
>  
>  /*
> - * The uncore version of the spin lock functions is used to decide
> - * whether we need to lock the uncore lock or not.  This is only
> - * needed in i915, not in Xe.
> - *
>   * This lock in i915 is needed because some old platforms (at least
>   * IVB and possibly HSW as well), which are not supported in Xe, need
>   * all register accesses to the same cacheline to be serialized,
> @@ -292,17 +288,15 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>   */
>  #ifdef I915
>  static void intel_vblank_section_enter(struct intel_display *display)
> -	__acquires(uncore->lock)
> +	__acquires(display->mmio.lock)
>  {
> -	struct intel_uncore *uncore = to_intel_uncore(display->drm);
> -	spin_lock(&uncore->lock);
> +	spin_lock(&display->mmio.lock);
>  }
>  
>  static void intel_vblank_section_exit(struct intel_display *display)
> -	__releases(uncore->lock)
> +	__releases(display->mmio.lock)
>  {
> -	struct intel_uncore *uncore = to_intel_uncore(display->drm);
> -	spin_unlock(&uncore->lock);
> +	spin_unlock(&display->mmio.lock);
>  }
>  #else
>  static void intel_vblank_section_enter(struct intel_display *display)
> @@ -550,7 +544,7 @@ void intel_crtc_update_active_timings(const struct intel_crtc_state *crtc_state,
>  	 * consistent state during fastset seamless refresh rate changes.
>  	 *
>  	 * vblank_time_lock takes care of all drm_vblank.c stuff, and
> -	 * uncore.lock takes care of __intel_get_crtc_scanline() which
> +	 * display->mmio.lock takes care of __intel_get_crtc_scanline() which
>  	 * may get called elsewhere as well.
>  	 *
>  	 * TODO maybe just protect everything (including

-- 
Jani Nikula, Intel