On Thu, 05 Nov 2026, Ville Syrjala <[email protected]> wrote: > From: Ville Syrjälä <[email protected]> > > 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ä <[email protected]>
Reviewed-by: Jani Nikula <[email protected]> > --- > 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
