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 +} + 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 -- 2.56.0
