On Mon, 21 Oct 2024, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote: > Some display MMIO transactions for offsets in the range that requires > the DMC wakelock happen in atomic context (this has been confirmed > during tests on PTL). That means that we need to use a non-sleeping > variant of MMIO waiting function. > > Implement __intel_de_wait_for_register_atomic_nowl() and use it when > waiting for acknowledgment of acquire/release. > > Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx> > --- > drivers/gpu/drm/i915/display/intel_de.h | 11 +++++++++++ > drivers/gpu/drm/i915/display/intel_dmc_wl.c | 20 ++++++++++++-------- > 2 files changed, 23 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_de.h b/drivers/gpu/drm/i915/display/intel_de.h > index e017cd4a8168..4116783a62dd 100644 > --- a/drivers/gpu/drm/i915/display/intel_de.h > +++ b/drivers/gpu/drm/i915/display/intel_de.h > @@ -121,6 +121,17 @@ ____intel_de_wait_for_register_nowl(struct intel_display *display, > } > #define __intel_de_wait_for_register_nowl(p,...) ____intel_de_wait_for_register_nowl(__to_intel_display(p), __VA_ARGS__) > > +static inline int > +____intel_de_wait_for_register_atomic_nowl(struct intel_display *display, > + i915_reg_t reg, > + u32 mask, u32 value, > + unsigned int fast_timeout_us) > +{ > + return __intel_wait_for_register(__to_uncore(display), reg, mask, > + value, fast_timeout_us, 0, NULL); > +} > +#define __intel_de_wait_for_register_atomic_nowl(p,...) ____intel_de_wait_for_register_atomic_nowl(__to_intel_display(p), __VA_ARGS__) > + There's no need to add the wrapper when all users pass struct intel_display. And we don't want new users that pass i915. And why are we adding new stuff and users with double underscores? > static inline int > __intel_de_wait(struct intel_display *display, i915_reg_t reg, > u32 mask, u32 value, unsigned int timeout) > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c > index 5634ff07269d..8056a3c8666c 100644 > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c > @@ -39,7 +39,7 @@ > * potential future use. > */ > > -#define DMC_WAKELOCK_CTL_TIMEOUT 5 > +#define DMC_WAKELOCK_CTL_TIMEOUT_US 5000 > #define DMC_WAKELOCK_HOLD_TIME 50 > > struct intel_dmc_wl_range { > @@ -78,9 +78,9 @@ static void intel_dmc_wl_work(struct work_struct *work) > > __intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, DMC_WAKELOCK_CTL_REQ, 0); > > - if (__intel_de_wait_for_register_nowl(display, DMC_WAKELOCK1_CTL, > - DMC_WAKELOCK_CTL_ACK, 0, > - DMC_WAKELOCK_CTL_TIMEOUT)) { > + if (__intel_de_wait_for_register_atomic_nowl(display, DMC_WAKELOCK1_CTL, > + DMC_WAKELOCK_CTL_ACK, 0, > + DMC_WAKELOCK_CTL_TIMEOUT_US)) { > WARN_RATELIMIT(1, "DMC wakelock release timed out"); > goto out_unlock; > } > @@ -216,10 +216,14 @@ void intel_dmc_wl_get(struct intel_display *display, i915_reg_t reg) > __intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, 0, > DMC_WAKELOCK_CTL_REQ); > > - if (__intel_de_wait_for_register_nowl(display, DMC_WAKELOCK1_CTL, > - DMC_WAKELOCK_CTL_ACK, > - DMC_WAKELOCK_CTL_ACK, > - DMC_WAKELOCK_CTL_TIMEOUT)) { > + /* > + * We need to use the atomic variant of the waiting routine > + * because the DMC wakelock is also taken in atomic context. > + */ > + if (__intel_de_wait_for_register_atomic_nowl(display, DMC_WAKELOCK1_CTL, > + DMC_WAKELOCK_CTL_ACK, > + DMC_WAKELOCK_CTL_ACK, > + DMC_WAKELOCK_CTL_TIMEOUT_US)) { > WARN_RATELIMIT(1, "DMC wakelock ack timed out"); > goto out_unlock; > } -- Jani Nikula, Intel