[Intel-gfx] [PATCH v3] drm/i915/display: Wait PSR2 get out of deep sleep to update pipe
Gwan-gyeong Mun
gwan-gyeong.mun at intel.com
Thu Oct 21 13:06:56 UTC 2021
On 10/14/21 12:03 AM, Souza, Jose wrote:
> 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 at linux.intel.com>
>>>>>>> Cc: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
>>>>>>> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
>>>>>>> ---
>>>>>>> .../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.
>
Hi, today I got the confirmation from HW's responsible developer.
Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
And if you don't mind could you add the below comments to the commit
messages or comments?
The timing generator affects the return value of PIPE_SCANLINE/PIPEDSL.
When the PSR2 is going to Deep Sleep state, the timing generator turns off.
And after the time to wake the link back up (it depends on PSR2_CTL[TP2
Time]), the timing generator will be turned back.
the below-listed states are the PSR2 states where the timing generator
is still running.
therefore it is safe to use the value of PIPE_SCANLINE/PIPEDSL when
PSR2_STATUS[PSR2 State] indicates these states.
: IDLE, CAPTURE, CPTURE_FS, SLEEP, BUFON_FW, ML_UP, SU_STANDBY.
Thanks,
G.G.
>>
>> 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
>>>>>>>
>>>>>
>>>
>
More information about the Intel-gfx
mailing list