| Message ID | 20210923194617.69136-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] drm/i915/display: Wait PSR2 get out of deep sleep to update pipe | expand |
On Thu, Sep 23, 2021 at 12:46:11PM -0700, José Roberto de Souza wrote: > Alderlake-P was getting 'max time under evasion' messages when PSR2 > was enabled, this is due PIPE_SCANLINE/PIPEDSL returning 0 over a > period of time longer than VBLANK_EVASION_TIME_US. > > For PSR1 we had the same issue so intel_psr_wait_for_idle() was > implemented to wait for PSR1 to get into idle state but nothing was > done for PSR2. > > For PSR2 we can't only wait for idle state as PSR2 tends to keep > into sleep state that means it is ready to send selective updates. > > To do so it was necessary to add intel_wait_for_condition(), this > takes as parameter a function that will return true when the desidered > condition is meet. > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > .../drm/i915/display/intel_display_debugfs.c | 3 +- > drivers/gpu/drm/i915/display/intel_psr.c | 64 ++++++++++++------- > drivers/gpu/drm/i915/i915_reg.h | 5 +- > drivers/gpu/drm/i915/intel_uncore.c | 47 ++++++++++++++ > drivers/gpu/drm/i915/intel_uncore.h | 7 ++ > 5 files changed, 100 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > index 68f4ba8c46e75..662596adb1da6 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > @@ -303,8 +303,7 @@ psr_source_status(struct intel_dp *intel_dp, struct seq_file *m) > }; > val = intel_de_read(dev_priv, > EDP_PSR2_STATUS(intel_dp->psr.transcoder)); > - status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >> > - EDP_PSR2_STATUS_STATE_SHIFT; > + status_val = REG_FIELD_GET(EDP_PSR2_STATUS_STATE_MASK, val); > if (status_val < ARRAY_SIZE(live_status)) > status = live_status[status_val]; > } else { > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > index 19a96d3c4acf4..a2e4ef42be60a 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -1796,15 +1796,33 @@ void intel_psr_post_plane_update(const struct intel_atomic_state *state) > _intel_psr_post_plane_update(state, crtc_state); > } > > -/** > - * psr_wait_for_idle - wait for PSR1 to idle > - * @intel_dp: Intel DP > - * @out_value: PSR status in case of failure > - * > - * Returns: 0 on success or -ETIMEOUT if PSR status does not idle. > - * > - */ > -static int psr_wait_for_idle(struct intel_dp *intel_dp, u32 *out_value) > +static bool _is_psr2_read_for_pipe_update(void *data) > +{ > + struct intel_dp *intel_dp = data; > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + u32 val; > + > + val = intel_uncore_read_fw(&dev_priv->uncore, > + EDP_PSR2_STATUS(intel_dp->psr.transcoder)); > + val &= EDP_PSR2_STATUS_STATE_MASK; > + > + return val == EDP_PSR2_STATUS_STATE_SLEEP || val == EDP_PSR2_STATUS_STATE_IDLE; > +} > + > +static int _psr2_ready_for_pipe_update_locked(struct intel_dp *intel_dp) > +{ > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + unsigned int fw; > + > + fw = intel_uncore_forcewake_for_reg(&dev_priv->uncore, > + EDP_PSR2_STATUS(intel_dp->psr.transcoder), > + FW_REG_READ); > + return intel_wait_for_condition(&dev_priv->uncore, > + _is_psr2_read_for_pipe_update, > + intel_dp, fw, 50); > +} > + > +static int _psr1_ready_for_pipe_update_locked(struct intel_dp *intel_dp) > { > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > @@ -1814,15 +1832,13 @@ static int psr_wait_for_idle(struct intel_dp *intel_dp, u32 *out_value) > * exit training time + 1.5 ms of aux channel handshake. 50 ms is > * defensive enough to cover everything. > */ > - return __intel_wait_for_register(&dev_priv->uncore, > - EDP_PSR_STATUS(intel_dp->psr.transcoder), > - EDP_PSR_STATUS_STATE_MASK, > - EDP_PSR_STATUS_STATE_IDLE, 2, 50, > - out_value); > + return intel_de_wait_for_clear(dev_priv, > + EDP_PSR_STATUS(intel_dp->psr.transcoder), > + EDP_PSR_STATUS_STATE_MASK, 50); > } > > /** > - * intel_psr_wait_for_idle - wait for PSR1 to idle > + * intel_psr_wait_for_idle - wait for PSR be ready for a pipe update > * @new_crtc_state: new CRTC state > * > * This function is expected to be called from pipe_update_start() where it is > @@ -1839,19 +1855,23 @@ void intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state) > for_each_intel_encoder_mask_with_psr(&dev_priv->drm, encoder, > new_crtc_state->uapi.encoder_mask) { > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > - u32 psr_status; > + int ret; > > mutex_lock(&intel_dp->psr.lock); > - if (!intel_dp->psr.enabled || intel_dp->psr.psr2_enabled) { > + > + if (!intel_dp->psr.enabled) { > mutex_unlock(&intel_dp->psr.lock); > continue; > } > > - /* when the PSR1 is enabled */ > - if (psr_wait_for_idle(intel_dp, &psr_status)) > - drm_err(&dev_priv->drm, > - "PSR idle timed out 0x%x, atomic update may fail\n", > - psr_status); > + if (intel_dp->psr.psr2_enabled) > + ret = _psr2_ready_for_pipe_update_locked(intel_dp); > + else > + ret = _psr1_ready_for_pipe_update_locked(intel_dp); > + > + if (ret) > + drm_err(&dev_priv->drm, "PSR wait timed out, atomic update may fail\n"); > + > mutex_unlock(&intel_dp->psr.lock); > } > } > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index cad84c3b864bf..a827f5bf973cb 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -4698,8 +4698,9 @@ enum { > #define _PSR2_STATUS_A 0x60940 > #define _PSR2_STATUS_EDP 0x6f940 > #define EDP_PSR2_STATUS(tran) _MMIO_TRANS2(tran, _PSR2_STATUS_A) > -#define EDP_PSR2_STATUS_STATE_MASK (0xf << 28) > -#define EDP_PSR2_STATUS_STATE_SHIFT 28 > +#define EDP_PSR2_STATUS_STATE_MASK REG_GENMASK(31, 28) > +#define EDP_PSR2_STATUS_STATE_SLEEP REG_FIELD_PREP(EDP_PSR2_STATUS_STATE_MASK, 0x3) > +#define EDP_PSR2_STATUS_STATE_IDLE REG_FIELD_PREP(EDP_PSR2_STATUS_STATE_MASK, 0x0) > > #define _PSR2_SU_STATUS_A 0x60914 > #define _PSR2_SU_STATUS_EDP 0x6f914 > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 678a99de07fee..1b3ea7318c2d5 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -2383,6 +2383,28 @@ int __intel_wait_for_register_fw(struct intel_uncore *uncore, > #undef done > } > > +static int __intel_wait_for_condition_fw(bool (*func)(void *data), void *data, > + unsigned int fast_timeout_us, > + unsigned int slow_timeout_ms) > +{ > +#define done (func(data)) > + int ret; > + > + /* Catch any overuse of this function */ > + might_sleep_if(slow_timeout_ms); > + GEM_BUG_ON(fast_timeout_us > 20000); > + GEM_BUG_ON(!fast_timeout_us && !slow_timeout_ms); > + > + ret = -ETIMEDOUT; > + if (fast_timeout_us && fast_timeout_us <= 20000) > + ret = _wait_for_atomic(done, fast_timeout_us, 0); > + if (ret && slow_timeout_ms) > + ret = wait_for(done, slow_timeout_ms); Is there a particular reason for these complicated wrappers instead of just using wait_for() directly? > + > + return ret; > +#undef done > +} > + > /** > * __intel_wait_for_register - wait until register matches expected state > * @uncore: the struct intel_uncore > @@ -2442,6 +2464,31 @@ int __intel_wait_for_register(struct intel_uncore *uncore, > return ret; > } > > +int intel_wait_for_condition(struct intel_uncore *uncore, > + bool (*func)(void *data), > + void *data, > + unsigned int fw, > + unsigned int slow_timeout_ms) > +{ > + unsigned int fast_timeout_us = 2; > + int ret; > + > + might_sleep_if(slow_timeout_ms); > + > + spin_lock_irq(&uncore->lock); > + intel_uncore_forcewake_get__locked(uncore, fw); > + > + ret = __intel_wait_for_condition_fw(func, data, fast_timeout_us, 0); > + > + intel_uncore_forcewake_put__locked(uncore, fw); > + spin_unlock_irq(&uncore->lock); > + > + if (ret && slow_timeout_ms) > + ret = __wait_for(, (func(data)), slow_timeout_ms * 1000, 10, 1000); > + > + return ret; > +} > + > bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore) > { > bool ret; > diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h > index 2f31c50eeae24..d6455824cbc56 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.h > +++ b/drivers/gpu/drm/i915/intel_uncore.h > @@ -282,6 +282,7 @@ int __intel_wait_for_register_fw(struct intel_uncore *uncore, > unsigned int fast_timeout_us, > unsigned int slow_timeout_ms, > u32 *out_value); > + > static inline int > intel_wait_for_register_fw(struct intel_uncore *uncore, > i915_reg_t reg, > @@ -293,6 +294,12 @@ intel_wait_for_register_fw(struct intel_uncore *uncore, > 2, timeout_ms, NULL); > } > > +int intel_wait_for_condition(struct intel_uncore *uncore, > + bool (*func)(void *data), > + void *data, > + unsigned int fw, > + unsigned int timeout_ms); > + > /* register access functions */ > #define __raw_read(x__, s__) \ > static inline u##x__ __raw_uncore_read##x__(const struct intel_uncore *uncore, \ > -- > 2.33.0
On Fri, 2021-09-24 at 17:35 +0300, Ville Syrjälä wrote: > On Thu, Sep 23, 2021 at 12:46:11PM -0700, José Roberto de Souza wrote: > > Alderlake-P was getting 'max time under evasion' messages when PSR2 > > was enabled, this is due PIPE_SCANLINE/PIPEDSL returning 0 over a > > period of time longer than VBLANK_EVASION_TIME_US. > > > > For PSR1 we had the same issue so intel_psr_wait_for_idle() was > > implemented to wait for PSR1 to get into idle state but nothing was > > done for PSR2. > > > > For PSR2 we can't only wait for idle state as PSR2 tends to keep > > into sleep state that means it is ready to send selective updates. > > > > To do so it was necessary to add intel_wait_for_condition(), this > > takes as parameter a function that will return true when the desidered > > condition is meet. > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > .../drm/i915/display/intel_display_debugfs.c | 3 +- > > drivers/gpu/drm/i915/display/intel_psr.c | 64 ++++++++++++------- > > drivers/gpu/drm/i915/i915_reg.h | 5 +- > > drivers/gpu/drm/i915/intel_uncore.c | 47 ++++++++++++++ > > drivers/gpu/drm/i915/intel_uncore.h | 7 ++ > > 5 files changed, 100 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > index 68f4ba8c46e75..662596adb1da6 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > @@ -303,8 +303,7 @@ psr_source_status(struct intel_dp *intel_dp, struct seq_file *m) > > }; > > val = intel_de_read(dev_priv, > > EDP_PSR2_STATUS(intel_dp->psr.transcoder)); > > - status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >> > > - EDP_PSR2_STATUS_STATE_SHIFT; > > + status_val = REG_FIELD_GET(EDP_PSR2_STATUS_STATE_MASK, val); > > if (status_val < ARRAY_SIZE(live_status)) > > status = live_status[status_val]; > > } else { > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > index 19a96d3c4acf4..a2e4ef42be60a 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -1796,15 +1796,33 @@ void intel_psr_post_plane_update(const struct intel_atomic_state *state) > > _intel_psr_post_plane_update(state, crtc_state); > > } > > > > -/** > > - * psr_wait_for_idle - wait for PSR1 to idle > > - * @intel_dp: Intel DP > > - * @out_value: PSR status in case of failure > > - * > > - * Returns: 0 on success or -ETIMEOUT if PSR status does not idle. > > - * > > - */ > > -static int psr_wait_for_idle(struct intel_dp *intel_dp, u32 *out_value) > > +static bool _is_psr2_read_for_pipe_update(void *data) > > +{ > > + struct intel_dp *intel_dp = data; > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + u32 val; > > + > > + val = intel_uncore_read_fw(&dev_priv->uncore, > > + EDP_PSR2_STATUS(intel_dp->psr.transcoder)); > > + val &= EDP_PSR2_STATUS_STATE_MASK; > > + > > + return val == EDP_PSR2_STATUS_STATE_SLEEP || val == EDP_PSR2_STATUS_STATE_IDLE; > > +} > > + > > +static int _psr2_ready_for_pipe_update_locked(struct intel_dp *intel_dp) > > +{ > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + unsigned int fw; > > + > > + fw = intel_uncore_forcewake_for_reg(&dev_priv->uncore, > > + EDP_PSR2_STATUS(intel_dp->psr.transcoder), > > + FW_REG_READ); > > + return intel_wait_for_condition(&dev_priv->uncore, > > + _is_psr2_read_for_pipe_update, > > + intel_dp, fw, 50); > > +} > > + > > +static int _psr1_ready_for_pipe_update_locked(struct intel_dp *intel_dp) > > { > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > > @@ -1814,15 +1832,13 @@ static int psr_wait_for_idle(struct intel_dp *intel_dp, u32 *out_value) > > * exit training time + 1.5 ms of aux channel handshake. 50 ms is > > * defensive enough to cover everything. > > */ > > - return __intel_wait_for_register(&dev_priv->uncore, > > - EDP_PSR_STATUS(intel_dp->psr.transcoder), > > - EDP_PSR_STATUS_STATE_MASK, > > - EDP_PSR_STATUS_STATE_IDLE, 2, 50, > > - out_value); > > + return intel_de_wait_for_clear(dev_priv, > > + EDP_PSR_STATUS(intel_dp->psr.transcoder), > > + EDP_PSR_STATUS_STATE_MASK, 50); > > } > > > > /** > > - * intel_psr_wait_for_idle - wait for PSR1 to idle > > + * intel_psr_wait_for_idle - wait for PSR be ready for a pipe update > > * @new_crtc_state: new CRTC state > > * > > * This function is expected to be called from pipe_update_start() where it is > > @@ -1839,19 +1855,23 @@ void intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state) > > for_each_intel_encoder_mask_with_psr(&dev_priv->drm, encoder, > > new_crtc_state->uapi.encoder_mask) { > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > - u32 psr_status; > > + int ret; > > > > mutex_lock(&intel_dp->psr.lock); > > - if (!intel_dp->psr.enabled || intel_dp->psr.psr2_enabled) { > > + > > + if (!intel_dp->psr.enabled) { > > mutex_unlock(&intel_dp->psr.lock); > > continue; > > } > > > > - /* when the PSR1 is enabled */ > > - if (psr_wait_for_idle(intel_dp, &psr_status)) > > - drm_err(&dev_priv->drm, > > - "PSR idle timed out 0x%x, atomic update may fail\n", > > - psr_status); > > + if (intel_dp->psr.psr2_enabled) > > + ret = _psr2_ready_for_pipe_update_locked(intel_dp); > > + else > > + ret = _psr1_ready_for_pipe_update_locked(intel_dp); > > + > > + if (ret) > > + drm_err(&dev_priv->drm, "PSR wait timed out, atomic update may fail\n"); > > + > > mutex_unlock(&intel_dp->psr.lock); > > } > > } > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index cad84c3b864bf..a827f5bf973cb 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -4698,8 +4698,9 @@ enum { > > #define _PSR2_STATUS_A 0x60940 > > #define _PSR2_STATUS_EDP 0x6f940 > > #define EDP_PSR2_STATUS(tran) _MMIO_TRANS2(tran, _PSR2_STATUS_A) > > -#define EDP_PSR2_STATUS_STATE_MASK (0xf << 28) > > -#define EDP_PSR2_STATUS_STATE_SHIFT 28 > > +#define EDP_PSR2_STATUS_STATE_MASK REG_GENMASK(31, 28) > > +#define EDP_PSR2_STATUS_STATE_SLEEP REG_FIELD_PREP(EDP_PSR2_STATUS_STATE_MASK, 0x3) > > +#define EDP_PSR2_STATUS_STATE_IDLE REG_FIELD_PREP(EDP_PSR2_STATUS_STATE_MASK, 0x0) > > > > #define _PSR2_SU_STATUS_A 0x60914 > > #define _PSR2_SU_STATUS_EDP 0x6f914 > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > > index 678a99de07fee..1b3ea7318c2d5 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -2383,6 +2383,28 @@ int __intel_wait_for_register_fw(struct intel_uncore *uncore, > > #undef done > > } > > > > +static int __intel_wait_for_condition_fw(bool (*func)(void *data), void *data, > > + unsigned int fast_timeout_us, > > + unsigned int slow_timeout_ms) > > +{ > > +#define done (func(data)) > > + int ret; > > + > > + /* Catch any overuse of this function */ > > + might_sleep_if(slow_timeout_ms); > > + GEM_BUG_ON(fast_timeout_us > 20000); > > + GEM_BUG_ON(!fast_timeout_us && !slow_timeout_ms); > > + > > + ret = -ETIMEDOUT; > > + if (fast_timeout_us && fast_timeout_us <= 20000) > > + ret = _wait_for_atomic(done, fast_timeout_us, 0); > > + if (ret && slow_timeout_ms) > > + ret = wait_for(done, slow_timeout_ms); > > Is there a particular reason for these complicated wrappers > instead of just using wait_for() directly? Just replicated what __intel_wait_for_register_fw() do. I guess the first one sleep less for cases that condition is meet in a few usecs. > > > + > > + return ret; > > +#undef done > > +} > > + > > /** > > * __intel_wait_for_register - wait until register matches expected state > > * @uncore: the struct intel_uncore > > @@ -2442,6 +2464,31 @@ int __intel_wait_for_register(struct intel_uncore *uncore, > > return ret; > > } > > > > +int intel_wait_for_condition(struct intel_uncore *uncore, > > + bool (*func)(void *data), > > + void *data, > > + unsigned int fw, > > + unsigned int slow_timeout_ms) > > +{ > > + unsigned int fast_timeout_us = 2; > > + int ret; > > + > > + might_sleep_if(slow_timeout_ms); > > + > > + spin_lock_irq(&uncore->lock); > > + intel_uncore_forcewake_get__locked(uncore, fw); > > + > > + ret = __intel_wait_for_condition_fw(func, data, fast_timeout_us, 0); > > + > > + intel_uncore_forcewake_put__locked(uncore, fw); > > + spin_unlock_irq(&uncore->lock); > > + > > + if (ret && slow_timeout_ms) > > + ret = __wait_for(, (func(data)), slow_timeout_ms * 1000, 10, 1000); > > + > > + return ret; > > +} > > + > > bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore) > > { > > bool ret; > > diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h > > index 2f31c50eeae24..d6455824cbc56 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.h > > +++ b/drivers/gpu/drm/i915/intel_uncore.h > > @@ -282,6 +282,7 @@ int __intel_wait_for_register_fw(struct intel_uncore *uncore, > > unsigned int fast_timeout_us, > > unsigned int slow_timeout_ms, > > u32 *out_value); > > + > > static inline int > > intel_wait_for_register_fw(struct intel_uncore *uncore, > > i915_reg_t reg, > > @@ -293,6 +294,12 @@ intel_wait_for_register_fw(struct intel_uncore *uncore, > > 2, timeout_ms, NULL); > > } > > > > +int intel_wait_for_condition(struct intel_uncore *uncore, > > + bool (*func)(void *data), > > + void *data, > > + unsigned int fw, > > + unsigned int timeout_ms); > > + > > /* register access functions */ > > #define __raw_read(x__, s__) \ > > static inline u##x__ __raw_uncore_read##x__(const struct intel_uncore *uncore, \ > > -- > > 2.33.0 >
On Fri, Sep 24, 2021 at 05:10:44PM +0000, Souza, Jose wrote: > On Fri, 2021-09-24 at 17:35 +0300, Ville Syrjälä wrote: > > On Thu, Sep 23, 2021 at 12:46:11PM -0700, José Roberto de Souza wrote: > > > Alderlake-P was getting 'max time under evasion' messages when PSR2 > > > was enabled, this is due PIPE_SCANLINE/PIPEDSL returning 0 over a > > > period of time longer than VBLANK_EVASION_TIME_US. > > > > > > For PSR1 we had the same issue so intel_psr_wait_for_idle() was > > > implemented to wait for PSR1 to get into idle state but nothing was > > > done for PSR2. > > > > > > For PSR2 we can't only wait for idle state as PSR2 tends to keep > > > into sleep state that means it is ready to send selective updates. > > > > > > To do so it was necessary to add intel_wait_for_condition(), this > > > takes as parameter a function that will return true when the desidered > > > condition is meet. > > > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > .../drm/i915/display/intel_display_debugfs.c | 3 +- > > > drivers/gpu/drm/i915/display/intel_psr.c | 64 ++++++++++++------- > > > drivers/gpu/drm/i915/i915_reg.h | 5 +- > > > drivers/gpu/drm/i915/intel_uncore.c | 47 ++++++++++++++ > > > drivers/gpu/drm/i915/intel_uncore.h | 7 ++ > > > 5 files changed, 100 insertions(+), 26 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > > index 68f4ba8c46e75..662596adb1da6 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c > > > @@ -303,8 +303,7 @@ psr_source_status(struct intel_dp *intel_dp, struct seq_file *m) > > > }; > > > val = intel_de_read(dev_priv, > > > EDP_PSR2_STATUS(intel_dp->psr.transcoder)); > > > - status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >> > > > - EDP_PSR2_STATUS_STATE_SHIFT; > > > + status_val = REG_FIELD_GET(EDP_PSR2_STATUS_STATE_MASK, val); > > > if (status_val < ARRAY_SIZE(live_status)) > > > status = live_status[status_val]; > > > } else { > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > > index 19a96d3c4acf4..a2e4ef42be60a 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -1796,15 +1796,33 @@ void intel_psr_post_plane_update(const struct intel_atomic_state *state) > > > _intel_psr_post_plane_update(state, crtc_state); > > > } > > > > > > -/** > > > - * psr_wait_for_idle - wait for PSR1 to idle > > > - * @intel_dp: Intel DP > > > - * @out_value: PSR status in case of failure > > > - * > > > - * Returns: 0 on success or -ETIMEOUT if PSR status does not idle. > > > - * > > > - */ > > > -static int psr_wait_for_idle(struct intel_dp *intel_dp, u32 *out_value) > > > +static bool _is_psr2_read_for_pipe_update(void *data) > > > +{ > > > + struct intel_dp *intel_dp = data; > > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > + u32 val; > > > + > > > + val = intel_uncore_read_fw(&dev_priv->uncore, > > > + EDP_PSR2_STATUS(intel_dp->psr.transcoder)); > > > + val &= EDP_PSR2_STATUS_STATE_MASK; > > > + > > > + return val == EDP_PSR2_STATUS_STATE_SLEEP || val == EDP_PSR2_STATUS_STATE_IDLE; > > > +} > > > + > > > +static int _psr2_ready_for_pipe_update_locked(struct intel_dp *intel_dp) > > > +{ > > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > + unsigned int fw; > > > + > > > + fw = intel_uncore_forcewake_for_reg(&dev_priv->uncore, > > > + EDP_PSR2_STATUS(intel_dp->psr.transcoder), > > > + FW_REG_READ); > > > + return intel_wait_for_condition(&dev_priv->uncore, > > > + _is_psr2_read_for_pipe_update, > > > + intel_dp, fw, 50); > > > +} > > > + > > > +static int _psr1_ready_for_pipe_update_locked(struct intel_dp *intel_dp) > > > { > > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > > > > @@ -1814,15 +1832,13 @@ static int psr_wait_for_idle(struct intel_dp *intel_dp, u32 *out_value) > > > * exit training time + 1.5 ms of aux channel handshake. 50 ms is > > > * defensive enough to cover everything. > > > */ > > > - return __intel_wait_for_register(&dev_priv->uncore, > > > - EDP_PSR_STATUS(intel_dp->psr.transcoder), > > > - EDP_PSR_STATUS_STATE_MASK, > > > - EDP_PSR_STATUS_STATE_IDLE, 2, 50, > > > - out_value); > > > + return intel_de_wait_for_clear(dev_priv, > > > + EDP_PSR_STATUS(intel_dp->psr.transcoder), > > > + EDP_PSR_STATUS_STATE_MASK, 50); > > > } > > > > > > /** > > > - * intel_psr_wait_for_idle - wait for PSR1 to idle > > > + * intel_psr_wait_for_idle - wait for PSR be ready for a pipe update > > > * @new_crtc_state: new CRTC state > > > * > > > * This function is expected to be called from pipe_update_start() where it is > > > @@ -1839,19 +1855,23 @@ void intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state) > > > for_each_intel_encoder_mask_with_psr(&dev_priv->drm, encoder, > > > new_crtc_state->uapi.encoder_mask) { > > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder); > > > - u32 psr_status; > > > + int ret; > > > > > > mutex_lock(&intel_dp->psr.lock); > > > - if (!intel_dp->psr.enabled || intel_dp->psr.psr2_enabled) { > > > + > > > + if (!intel_dp->psr.enabled) { > > > mutex_unlock(&intel_dp->psr.lock); > > > continue; > > > } > > > > > > - /* when the PSR1 is enabled */ > > > - if (psr_wait_for_idle(intel_dp, &psr_status)) > > > - drm_err(&dev_priv->drm, > > > - "PSR idle timed out 0x%x, atomic update may fail\n", > > > - psr_status); > > > + if (intel_dp->psr.psr2_enabled) > > > + ret = _psr2_ready_for_pipe_update_locked(intel_dp); > > > + else > > > + ret = _psr1_ready_for_pipe_update_locked(intel_dp); > > > + > > > + if (ret) > > > + drm_err(&dev_priv->drm, "PSR wait timed out, atomic update may fail\n"); > > > + > > > mutex_unlock(&intel_dp->psr.lock); > > > } > > > } > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index cad84c3b864bf..a827f5bf973cb 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -4698,8 +4698,9 @@ enum { > > > #define _PSR2_STATUS_A 0x60940 > > > #define _PSR2_STATUS_EDP 0x6f940 > > > #define EDP_PSR2_STATUS(tran) _MMIO_TRANS2(tran, _PSR2_STATUS_A) > > > -#define EDP_PSR2_STATUS_STATE_MASK (0xf << 28) > > > -#define EDP_PSR2_STATUS_STATE_SHIFT 28 > > > +#define EDP_PSR2_STATUS_STATE_MASK REG_GENMASK(31, 28) > > > +#define EDP_PSR2_STATUS_STATE_SLEEP REG_FIELD_PREP(EDP_PSR2_STATUS_STATE_MASK, 0x3) > > > +#define EDP_PSR2_STATUS_STATE_IDLE REG_FIELD_PREP(EDP_PSR2_STATUS_STATE_MASK, 0x0) > > > > > > #define _PSR2_SU_STATUS_A 0x60914 > > > #define _PSR2_SU_STATUS_EDP 0x6f914 > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > > > index 678a99de07fee..1b3ea7318c2d5 100644 > > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > > @@ -2383,6 +2383,28 @@ int __intel_wait_for_register_fw(struct intel_uncore *uncore, > > > #undef done > > > } > > > > > > +static int __intel_wait_for_condition_fw(bool (*func)(void *data), void *data, > > > + unsigned int fast_timeout_us, > > > + unsigned int slow_timeout_ms) > > > +{ > > > +#define done (func(data)) > > > + int ret; > > > + > > > + /* Catch any overuse of this function */ > > > + might_sleep_if(slow_timeout_ms); > > > + GEM_BUG_ON(fast_timeout_us > 20000); > > > + GEM_BUG_ON(!fast_timeout_us && !slow_timeout_ms); > > > + > > > + ret = -ETIMEDOUT; > > > + if (fast_timeout_us && fast_timeout_us <= 20000) > > > + ret = _wait_for_atomic(done, fast_timeout_us, 0); > > > + if (ret && slow_timeout_ms) > > > + ret = wait_for(done, slow_timeout_ms); > > > > Is there a particular reason for these complicated wrappers > > instead of just using wait_for() directly? > > Just replicated what __intel_wait_for_register_fw() do. > I guess the first one sleep less for cases that condition is meet in a few usecs. wait_for() should already start with a short delay with exponential backoff the longer it has to wait. So unless there is a demonstrable problem with wait_for() I don't think we should be adding even more complex wait macros.
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c index 68f4ba8c46e75..662596adb1da6 100644 --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c @@ -303,8 +303,7 @@ psr_source_status(struct intel_dp *intel_dp, struct seq_file *m) }; val = intel_de_read(dev_priv, EDP_PSR2_STATUS(intel_dp->psr.transcoder)); - status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >> - EDP_PSR2_STATUS_STATE_SHIFT; + status_val = REG_FIELD_GET(EDP_PSR2_STATUS_STATE_MASK, val); if (status_val < ARRAY_SIZE(live_status)) status = live_status[status_val]; } else { diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index 19a96d3c4acf4..a2e4ef42be60a 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -1796,15 +1796,33 @@ void intel_psr_post_plane_update(const struct intel_atomic_state *state) _intel_psr_post_plane_update(state, crtc_state); } -/** - * psr_wait_for_idle - wait for PSR1 to idle - * @intel_dp: Intel DP - * @out_value: PSR status in case of failure - * - * Returns: 0 on success or -ETIMEOUT if PSR status does not idle. - * - */ -static int psr_wait_for_idle(struct intel_dp *intel_dp, u32 *out_value) +static bool _is_psr2_read_for_pipe_update(void *data) +{ + struct intel_dp *intel_dp = data; + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); + u32 val; + + val = intel_uncore_read_fw(&dev_priv->uncore, + EDP_PSR2_STATUS(intel_dp->psr.transcoder)); + val &= EDP_PSR2_STATUS_STATE_MASK; + + return val == EDP_PSR2_STATUS_STATE_SLEEP || val == EDP_PSR2_STATUS_STATE_IDLE; +} + +static int _psr2_ready_for_pipe_update_locked(struct intel_dp *intel_dp) +{ + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); + unsigned int fw; + + fw = intel_uncore_forcewake_for_reg(&dev_priv->uncore, + EDP_PSR2_STATUS(intel_dp->psr.transcoder), + FW_REG_READ); + return intel_wait_for_condition(&dev_priv->uncore, + _is_psr2_read_for_pipe_update, + intel_dp, fw, 50); +} + +static int _psr1_ready_for_pipe_update_locked(struct intel_dp *intel_dp) { struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); @@ -1814,15 +1832,13 @@ static int psr_wait_for_idle(struct intel_dp *intel_dp, u32 *out_value) * exit training time + 1.5 ms of aux channel handshake. 50 ms is * defensive enough to cover everything. */ - return __intel_wait_for_register(&dev_priv->uncore, - EDP_PSR_STATUS(intel_dp->psr.transcoder), - EDP_PSR_STATUS_STATE_MASK, - EDP_PSR_STATUS_STATE_IDLE, 2, 50, - out_value); + return intel_de_wait_for_clear(dev_priv, + EDP_PSR_STATUS(intel_dp->psr.transcoder), + EDP_PSR_STATUS_STATE_MASK, 50); } /** - * intel_psr_wait_for_idle - wait for PSR1 to idle + * intel_psr_wait_for_idle - wait for PSR be ready for a pipe update * @new_crtc_state: new CRTC state * * This function is expected to be called from pipe_update_start() where it is @@ -1839,19 +1855,23 @@ void intel_psr_wait_for_idle(const struct intel_crtc_state *new_crtc_state) for_each_intel_encoder_mask_with_psr(&dev_priv->drm, encoder, new_crtc_state->uapi.encoder_mask) { struct intel_dp *intel_dp = enc_to_intel_dp(encoder); - u32 psr_status; + int ret; mutex_lock(&intel_dp->psr.lock); - if (!intel_dp->psr.enabled || intel_dp->psr.psr2_enabled) { + + if (!intel_dp->psr.enabled) { mutex_unlock(&intel_dp->psr.lock); continue; } - /* when the PSR1 is enabled */ - if (psr_wait_for_idle(intel_dp, &psr_status)) - drm_err(&dev_priv->drm, - "PSR idle timed out 0x%x, atomic update may fail\n", - psr_status); + if (intel_dp->psr.psr2_enabled) + ret = _psr2_ready_for_pipe_update_locked(intel_dp); + else + ret = _psr1_ready_for_pipe_update_locked(intel_dp); + + if (ret) + drm_err(&dev_priv->drm, "PSR wait timed out, atomic update may fail\n"); + mutex_unlock(&intel_dp->psr.lock); } } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index cad84c3b864bf..a827f5bf973cb 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4698,8 +4698,9 @@ enum { #define _PSR2_STATUS_A 0x60940 #define _PSR2_STATUS_EDP 0x6f940 #define EDP_PSR2_STATUS(tran) _MMIO_TRANS2(tran, _PSR2_STATUS_A) -#define EDP_PSR2_STATUS_STATE_MASK (0xf << 28) -#define EDP_PSR2_STATUS_STATE_SHIFT 28 +#define EDP_PSR2_STATUS_STATE_MASK REG_GENMASK(31, 28) +#define EDP_PSR2_STATUS_STATE_SLEEP REG_FIELD_PREP(EDP_PSR2_STATUS_STATE_MASK, 0x3) +#define EDP_PSR2_STATUS_STATE_IDLE REG_FIELD_PREP(EDP_PSR2_STATUS_STATE_MASK, 0x0) #define _PSR2_SU_STATUS_A 0x60914 #define _PSR2_SU_STATUS_EDP 0x6f914 diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index 678a99de07fee..1b3ea7318c2d5 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -2383,6 +2383,28 @@ int __intel_wait_for_register_fw(struct intel_uncore *uncore, #undef done } +static int __intel_wait_for_condition_fw(bool (*func)(void *data), void *data, + unsigned int fast_timeout_us, + unsigned int slow_timeout_ms) +{ +#define done (func(data)) + int ret; + + /* Catch any overuse of this function */ + might_sleep_if(slow_timeout_ms); + GEM_BUG_ON(fast_timeout_us > 20000); + GEM_BUG_ON(!fast_timeout_us && !slow_timeout_ms); + + ret = -ETIMEDOUT; + if (fast_timeout_us && fast_timeout_us <= 20000) + ret = _wait_for_atomic(done, fast_timeout_us, 0); + if (ret && slow_timeout_ms) + ret = wait_for(done, slow_timeout_ms); + + return ret; +#undef done +} + /** * __intel_wait_for_register - wait until register matches expected state * @uncore: the struct intel_uncore @@ -2442,6 +2464,31 @@ int __intel_wait_for_register(struct intel_uncore *uncore, return ret; } +int intel_wait_for_condition(struct intel_uncore *uncore, + bool (*func)(void *data), + void *data, + unsigned int fw, + unsigned int slow_timeout_ms) +{ + unsigned int fast_timeout_us = 2; + int ret; + + might_sleep_if(slow_timeout_ms); + + spin_lock_irq(&uncore->lock); + intel_uncore_forcewake_get__locked(uncore, fw); + + ret = __intel_wait_for_condition_fw(func, data, fast_timeout_us, 0); + + intel_uncore_forcewake_put__locked(uncore, fw); + spin_unlock_irq(&uncore->lock); + + if (ret && slow_timeout_ms) + ret = __wait_for(, (func(data)), slow_timeout_ms * 1000, 10, 1000); + + return ret; +} + bool intel_uncore_unclaimed_mmio(struct intel_uncore *uncore) { bool ret; diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h index 2f31c50eeae24..d6455824cbc56 100644 --- a/drivers/gpu/drm/i915/intel_uncore.h +++ b/drivers/gpu/drm/i915/intel_uncore.h @@ -282,6 +282,7 @@ int __intel_wait_for_register_fw(struct intel_uncore *uncore, unsigned int fast_timeout_us, unsigned int slow_timeout_ms, u32 *out_value); + static inline int intel_wait_for_register_fw(struct intel_uncore *uncore, i915_reg_t reg, @@ -293,6 +294,12 @@ intel_wait_for_register_fw(struct intel_uncore *uncore, 2, timeout_ms, NULL); } +int intel_wait_for_condition(struct intel_uncore *uncore, + bool (*func)(void *data), + void *data, + unsigned int fw, + unsigned int timeout_ms); + /* register access functions */ #define __raw_read(x__, s__) \ static inline u##x__ __raw_uncore_read##x__(const struct intel_uncore *uncore, \
Alderlake-P was getting 'max time under evasion' messages when PSR2 was enabled, this is due PIPE_SCANLINE/PIPEDSL returning 0 over a period of time longer than VBLANK_EVASION_TIME_US. For PSR1 we had the same issue so intel_psr_wait_for_idle() was implemented to wait for PSR1 to get into idle state but nothing was done for PSR2. For PSR2 we can't only wait for idle state as PSR2 tends to keep into sleep state that means it is ready to send selective updates. To do so it was necessary to add intel_wait_for_condition(), this takes as parameter a function that will return true when the desidered condition is meet. Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- .../drm/i915/display/intel_display_debugfs.c | 3 +- drivers/gpu/drm/i915/display/intel_psr.c | 64 ++++++++++++------- drivers/gpu/drm/i915/i915_reg.h | 5 +- drivers/gpu/drm/i915/intel_uncore.c | 47 ++++++++++++++ drivers/gpu/drm/i915/intel_uncore.h | 7 ++ 5 files changed, 100 insertions(+), 26 deletions(-)