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]>
> ---
> 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
> +}
Ugh. Can we just not add this, because it needs to go real soon now
anyway? Or do you already have patches in your tree removing this after
some juggle?
> +
> 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;
> + }
If we temporarily need driver_is_i915() for the non-functional iterative
approach, seems like a different arrangement of the if ladder would be
benefitial for the eventual removal of driver_is_i915(), maybe something
like:
if (DISPLAY_VER(display) >= 20)
display->mmio.funcs = &lnl_mmio_funcs;
else if (!driver_is_i915())
display->mmio.funcs = &i9xx_mmio_funcs;
else if (display->platform.valleyview || display->platform.cherryview)
display->mmio.funcs = &vlv_mmio_funcs;
else if (DISPLAY_VER(display) >= 7)
display->mmio.funcs = &ivb_mmio_funcs;
else if (DISPLAY_VER(display) >= 6)
display->mmio.funcs = &snb_mmio_funcs;
else
display->mmio.funcs = &i9xx_mmio_funcs;
(This includes changes from later patches.)
I guess I'd like to see the decisions here (and in the vblank code) be
based on platforms rather than driver, even though there's some overlap.
> }
> 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;
> +
I guess the comment begs the question why are we doing the locking also
on display < 20? Just because the lock is (ab)used for other purposes as
well?
> 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
