[Intel-gfx] [PATCH] drm/i915: wait PSR state back to idle when turn PSR off

Fri Oct 23 21:06:05 UTC 2020

On Thu, 2020-10-22 at 13:56 +0000, Lee, Shawn C wrote:
> On Thu, Oct. 22, 2020, 3:24 a.m, Lee Shawn C wrote:
> > On Wed, Oct. 21, 2020, 5:13 p.m, Souza, Jose wrote:
> > > On Wed, 2020-10-21 at 22:24 +0800, Lee Shawn C wrote:
> > > > Driver should refer to commit 'b2fc2252ce41 ("drm/i915/psr:
> > > > Always wait for idle state when disabling PSR")' to wait for idle
> > > > state when turn PSR off. But it did not follow previous method.
> > > > Driver just call intel_psr_exit() in
> > > > intel_psr_invalidate() and psr_force_hw_tracking_exit().
> > > > Then leave the function right away.
> > > > 
> > > > After PSR disabled, we found some user space applications would
> > > > enabled PSR again immediately. That caused particular TCON to get
> > > > into incorrect state machine and can't recognize video data from
> > > > source properly.
> > > 
> > > How? I don't see how this is possible this change is only adding delay between userspace calls.
> > > 
> > > Take a look at intel_psr_work(), PSR will only be enabled again when idle.
> > > 
> > 
> > Thanks for clarification! Per our finding, the problem was found on specific TCON support PSR2.
> > Below is our observation on customer board.
> > 
> > After psr exit called at intel_psr_invalidate(), PSR2_STATUS (0x6f940, bit 31:28) report 0x3 sometimes.
> > Which means source PSR state still active. Then we check sink's DPCD 2008h before re-enable PSR2 in intel_psr_work().
> > DPCD 2008h shows 0x2 (PSR active - display from RFB) sometimes.
> > 
> > Seems problem occurred when source re-enable PSR2 but sink still at PSR2 active state.
> > TCON is not able to recognize video data. And corrupt display shows on eDP panel.
> > Abnormal display is recoverable after modeset.
> > 
> > Looks like my change to wait PSR2 state idle adding more delay here to give more times for TCON back to normal state.
> > Read DPCD 2008h to confirm sink's PSR2 status before re-enable PSR2 in intel_psr_work().
> > It will be 0x4 (Sink device Transition to PSR inactive - capture and display; timing re-sync) always.
> > Then we can't replicate corrupt display issue anymore.
> > 
> > In my opinion, confirm DPCD 2008h moved to 0x4 before re-enable PSR2 may help this customer issue.
> > What do you think?
> > 
> > Best regards,
> > Shawn
> > 
> 
> Per previous comment, it is a little complicated from source to align sink's PSR state.
> Even source PSR2 state already idle. But sink PSR2 state still at "active" sometimes.
> Here is another idea. How about to disable/re-enable sink's PSR2 just like driver did for source as well?
> Sink would back to normal display mode after PSR disabled. Then we can enable PSR again in intel_psr_work()
> before driver try to turn source PSR on.

Source HW already sends in SDP, PSR entry and exit notifications by it self.
This looks like a panel specific problem, we can't add a global workaround for it.
Also do the disable and enable would involve even more changes in the code and more time with PSR disable, losing some power savings.
This code is to handle frontbuffer modifications, modern user spaces should not hit this case, I'm wondering what your costumer is using.

Anyways, give a try with: https://patchwork.freedesktop.org/series/82351/
It is working around a issue that we are seeing in multiple panels 4K, until root caused we are going to use this workaround. Planing to merge it in a
couple of hours.

> 
> Best regards,
> Shawn
> 
> > > > 
> > > > Add this change to wait PSR idle state in intel_psr_invalidate() and
> > > > psr_force_hw_tracking_exit(). This symptom is not able to replicate
> > > > anymore.
> > > > 
> > > > Fixes: b2fc2252ce41 (drm/i915/psr: Always wait for idle state when
> > > > disabling PSR).
> > > > 
> > > > Cc: Manasi Navare <manasi.d.navare at intel.com>
> > > > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > > > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > > > Cc: José Roberto de Souza <jose.souza at intel.com>
> > > > Cc: Cooper Chiou <cooper.chiou at intel.com>
> > > > Cc: Khaled Almahallawy <khaled.almahallawy at intel.com>
> > > > Signed-off-by: Lee Shawn C <shawn.c.lee at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_psr.c | 43
> > > > ++++++++++++++----------
> > > >  1 file changed, 26 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index a591a475f148..83b642a5567e 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -1036,6 +1036,25 @@ void intel_psr_enable(struct intel_dp
> > > > *intel_dp,  mutex_unlock(&dev_priv->psr.lock);
> > > >  }
> > > > 
> > > > 
> > > > 
> > > > 
> > > > +static void intel_psr_wait_idle(struct drm_i915_private *dev_priv) {
> > > > +i915_reg_t psr_status;
> > > > +u32 psr_status_mask;
> > > > +
> > > > +if (dev_priv->psr.psr2_enabled) {
> > > > +psr_status = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
> > > > +psr_status_mask = EDP_PSR2_STATUS_STATE_MASK; } else { psr_status =
> > > > +EDP_PSR_STATUS(dev_priv->psr.transcoder);
> > > > +psr_status_mask = EDP_PSR_STATUS_STATE_MASK; }
> > > > +
> > > > +/* Wait till PSR is idle */
> > > > +if (intel_de_wait_for_clear(dev_priv, psr_status,
> > > > +    psr_status_mask, 2000))
> > > > +drm_err(&dev_priv->drm, "Timed out waiting PSR idle state\n"); }
> > > > +
> > > >  static void intel_psr_exit(struct drm_i915_private *dev_priv)  {
> > > >  u32 val;
> > > > @@ -1076,8 +1095,6 @@ static void intel_psr_exit(struct
> > > > drm_i915_private *dev_priv)  static void
> > > > intel_psr_disable_locked(struct intel_dp *intel_dp)  {  struct
> > > > drm_i915_private *dev_priv = dp_to_i915(intel_dp); -i915_reg_t
> > > > psr_status;
> > > > -u32 psr_status_mask;
> > > > 
> > > > 
> > > > 
> > > > 
> > > >  lockdep_assert_held(&dev_priv->psr.lock);
> > > > 
> > > > 
> > > > 
> > > > 
> > > > @@ -1088,19 +1105,7 @@ static void intel_psr_disable_locked(struct intel_dp *intel_dp)
> > > >      dev_priv->psr.psr2_enabled ? "2" : "1");
> > > > 
> > > > 
> > > > 
> > > > 
> > > >  intel_psr_exit(dev_priv);
> > > > -
> > > > -if (dev_priv->psr.psr2_enabled) {
> > > > -psr_status = EDP_PSR2_STATUS(dev_priv->psr.transcoder);
> > > > -psr_status_mask = EDP_PSR2_STATUS_STATE_MASK; -} else { -psr_status
> > > > = EDP_PSR_STATUS(dev_priv->psr.transcoder);
> > > > -psr_status_mask = EDP_PSR_STATUS_STATE_MASK; -}
> > > > -
> > > > -/* Wait till PSR is idle */
> > > > -if (intel_de_wait_for_clear(dev_priv, psr_status,
> > > > -    psr_status_mask, 2000))
> > > > -drm_err(&dev_priv->drm, "Timed out waiting PSR idle state\n");
> > > > +intel_psr_wait_idle(dev_priv);
> > > > 
> > > > 
> > > > 
> > > > 
> > > >  /* WA 1408330847 */
> > > >  if (dev_priv->psr.psr2_sel_fetch_enabled && @@ -1158,12 +1163,14 @@
> > > > static void psr_force_hw_tracking_exit(struct drm_i915_private *dev_priv)
> > > >   * pipe.
> > > >   */
> > > >  intel_de_write(dev_priv, CURSURFLIVE(dev_priv->psr.pipe), 0); -else
> > > > +else {
> > > >  /*
> > > >   * A write to CURSURFLIVE do not cause HW tracking to exit PSR
> > > >   * on older gens so doing the manual exit instead.
> > > >   */
> > > >  intel_psr_exit(dev_priv);
> > > > +intel_psr_wait_idle(dev_priv);
> > > > +}
> > > >  }
> > > > 
> > > > 
> > > > 
> > > > 
> > > >  void intel_psr2_program_plane_sel_fetch(struct intel_plane *plane,
> > > > @@
> > > > -1593,8 +1600,10 @@ void intel_psr_invalidate(struct drm_i915_private
> > > > *dev_priv,  frontbuffer_bits &=
> > > > INTEL_FRONTBUFFER_ALL_MASK(dev_priv->psr.pipe);
> > > >  dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
> > > > 
> > > > 
> > > > 
> > > > 
> > > > -if (frontbuffer_bits)
> > > > +if (frontbuffer_bits) {
> > > >  intel_psr_exit(dev_priv);
> > > > +intel_psr_wait_idle(dev_priv);
> > > > +}
> > > > 
> > > > 
> > > > 
> > > > 
> > > >  mutex_unlock(&dev_priv->psr.lock);
> > > >  }
> > 
> 