On Wed, 2021-10-13 at 23:39 +0300, Gwan-gyeong Mun wrote:
>
> On 10/11/21 11:53 PM, Souza, Jose wrote:
> > On Thu, 2021-10-07 at 12:31 +0300, Gwan-gyeong Mun wrote:
> > >
> > > On 10/6/21 11:04 PM, Souza, Jose wrote:
> > > > On Wed, 2021-10-06 at 11:50 +0300, Gwan-gyeong Mun wrote:
> > > > >
> > > > > On 10/6/21 2:18 AM, José Roberto de Souza wrote:
> > > > > > Alderlake-P was getting 'max time under evasion' messages when PSR2
> > > > > > is 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(ready to send selective updates).
> > > > > > Waiting for any state below deep sleep proved to be effective in
> > > > > > avoiding the evasion messages and also not wasted a lot of time.
> > > > > >
> > > > > > v2:
> > > > > > - dropping the additional wait_for loops, only the _wait_for_atomic()
> > > > > > is necessary
> > > > > > - waiting for states below EDP_PSR2_STATUS_STATE_DEEP_SLEEP
> > > > > >
> > > > > > v3:
> > > > > > - dropping intel_wait_for_condition_atomic() function
> > > > > >
> > > > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > > > > ---
> > > > > > .../drm/i915/display/intel_display_debugfs.c | 3 +-
> > > > > > drivers/gpu/drm/i915/display/intel_psr.c | 52 +++++++++++--------
> > > > > > drivers/gpu/drm/i915/i915_reg.h | 10 ++--
> > > > > > 3 files changed, 36 insertions(+), 29 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > > > > > index 309d74fd86ce1..d7dd3a57c6170 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 7a205fd5023bb..ade514fc0a24d 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > > > @@ -1809,15 +1809,21 @@ 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 int _psr2_ready_for_pipe_update_locked(struct intel_dp *intel_dp)
> > > > > > +{
> > > > > > +struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > > > +
> > > > > > +/*
> > > > > > + * Any state lower than EDP_PSR2_STATUS_STATE_DEEP_SLEEP is enough.
> > > > > > + * As all higher states has bit 4 of PSR2 state set we can just wait for
> > > > > > + * EDP_PSR2_STATUS_STATE_DEEP_SLEEP to be cleared.
> > > > > > + */
> > > > > > +return intel_de_wait_for_clear(dev_priv,
> > > > > > + EDP_PSR2_STATUS(intel_dp->psr.transcoder),
> > > > > > + EDP_PSR2_STATUS_STATE_DEEP_SLEEP, 50);
> > > > > Under the DEEP_SLEEP state, there are IDLE, CAPTURE, CPTURE_FS, SLEEP,
> > > > > BUFON_FW, ML_UP, SU_STANDBY, etc. In this case, whether the evasion
> > > > > messages are completely tested in the state that changes quickly I think
> > > > > the test period is a little insufficient.
> > > >
> > > > What is your suggestion of test for this?
> > > >
> > > > I left my Alderlake-P running overnight(more than 12 hours) with a News website open.
> > > > This website reloads the page at every 5 minutes, so it entered and exited DC5/6 states several times without any evasion messages.
> > > >
> > > > > I think it may be necessary to test a little more or to have
> > > > > confirmation from the HW person in charge.
> > > >
> > > > I can file an issue for this but it will probably several weeks to get an answer.
> > > >
> > > Yes, I am not disparaging what you tested.
> > > However, since the current code confirms that only the 31st bit of the
> > > PSR2_STATUS register is changed to 0 operationally,
> > > it does not guarantee that the tested use cases have been tested for
> > > IDLE, CAPTURE, CPTURE_FS, SLEEP, BUFON_FW, ML_UP, SU_STANDBY, and
> > > FAST_SLEEP states.
> > >
> > > I can't think of a way to test each of the above states right now, but
> > > what I can suggest is that "intel_de_wait_for_clear(dev_priv,
> > > EDP_PSR2_STATUS(intel_dp->psr.transcoder),
> > > EDP_PSR2_STATUS_STATE_DEEP_SLEEP, 50)" works normally. After that, can
> > > you put a code that prints the current PSR2 status?
> > >
> > > If so, I think it will be easy to analyze the problem in case evasion
> > > messages occur again after this code is applied later.
> > > If additional confirmation from the responsible HW developer is received
> > > at a later time, it is thought that future work such as deleting the
> > > code that outputs the newly added current PSR Status at that time will
> > > be possible.
> >
> > Print the PSR status at every flip is too verbose.
> >
> > Other option would be print the PSR status in case a evasion happened but that would not give us much information as the status would have changed
> > between intel_pipe_update_start() and intel_pipe_update_end().
> >
> > The current solution is better than no wait and if evasion messages comes back we can be more restrictive and make it wait for idle or sleep PSR2
> > states.
> > Rather than not waiting here, I agree to wait.
> But unless you're just waiting for an IDLE state here,
> when an evasion message occurs in CAPTURE, CPTURE_FS, SLEEP, BUFON_FW,
> ML_UP, SU_STANDBY, and FAST_SLEEP states, isn't it hard to know what
> caused the problem?
> (If there is a problem, we need to reproduce the problem again, but if
> there is a problem at a certain time, you know that it is difficult to
> reproduce.)
We can't only wait for idle, when PSR2 is active it stays at sleep state.
If a evasion happens and state is in CAPTURE do you know for sure what are the previous states of CAPTURE?
I can have some assumptions but not sure and specification don't have this information too.
ChromeOS reported evasion errors in their end today, their branch did not had this patch so we can't wait several weeks to get an answer from HW team.
Windows driver uses DMC queue to do flips, so we can't even check what Windows driver does.
>
> If you can't add additional debugging information here, IMHO how about
> applying this patch after getting confirmation from the HW person as
> mentioned in the previous email?
>
> Br,
> G.G.
> > >
> > > Br,
> > > G.G.
> > > > >
> > > > > [PSR2_STATUS]
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > Value | Name | Description |
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > 0000b| IDLE | Reset state |
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > 0001b| CAPTURE | Send capture frame |
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > 0010b| CPTURE_FS | Fast sleep after capture frame is sent |
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > 0011b| SLEEP | Selective Update |
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > 0100b| BUFON_FW | Turn Buffer on and Send Fast wake |
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > 0101b| ML_UP | Turn Main link up and send SR |
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > 0110b| SU_STANDBY | Selective update or Standby state |
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > 0111b| FAST_SLEEP | Send Fast sleep |
> > > > >
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > 1000b| DEEP_SLEEP | Enter Deep sleep |
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > 1001b| BUF_ON | Turn ON IO Buffer |
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > 1010b| TG_ON | Turn ON Timing Generator |
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > 1011b| BUFON_FW_2 |Turn Buffer on and Send Fast wake for 3 BlockCase|
> > > > > +-------+------------+-------------------------------------------------+
> > > > > > +}
> > > > > > +
> > > > > > +static int _psr1_ready_for_pipe_update_locked(struct intel_dp *intel_dp)
> > > > > > {
> > > > > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > > >
> > > > > > @@ -1827,15 +1833,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
> > > > > > @@ -1852,19 +1856,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 a897f4abea0c3..e101579d3a4d8 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -4700,11 +4700,11 @@ enum {
> > > > > > #define PSR_EVENT_LPSP_MODE_EXIT(1 << 1)
> > > > > > #define PSR_EVENT_PSR_DISABLE(1 << 0)
> > > > > >
> > > > > > -#define _PSR2_STATUS_A0x60940
> > > > > > -#define _PSR2_STATUS_EDP0x6f940
> > > > > > -#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 _PSR2_STATUS_A0x60940
> > > > > > +#define _PSR2_STATUS_EDP0x6f940
> > > > > > +#define EDP_PSR2_STATUS(tran)_MMIO_TRANS2(tran, _PSR2_STATUS_A)
> > > > > > +#define EDP_PSR2_STATUS_STATE_MASKREG_GENMASK(31, 28)
> > > > > > +#define EDP_PSR2_STATUS_STATE_DEEP_SLEEPREG_FIELD_PREP(EDP_PSR2_STATUS_STATE_MASK, 0x8)
> > > > > >
> > > > > > #define _PSR2_SU_STATUS_A0x60914
> > > > > > #define _PSR2_SU_STATUS_EDP0x6f914
> > > > > >
> > > >
> >
