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

Mun, Gwan-gyeong gwan-gyeong.mun at intel.com
Wed Dec 2 17:17:10 UTC 2020 

On Fri, 2020-10-23 at 21:06 +0000, Souza, Jose wrote:
> 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);
> > > > >  }
> 
Hi Shawn,
I agree with Jose's opinion.
If the patch mitigates your PSR2 Panel issues, it might be a workaround
hotfix.
But we need to clarify which device raises the issue between sink
(specific tcon) and source (gen9). After then we can suggest the proper
solution.
And please check the last vsc sdp which is sent from source, and sink's
psr state machine state.

> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

More information about the Intel-gfx mailing list