| Message ID | 20201021142450.7758-1-shawn.c.lee@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: wait PSR state back to idle when turn PSR off | expand |
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. > > 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@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Cooper Chiou <cooper.chiou@intel.com> > Cc: Khaled Almahallawy <khaled.almahallawy@intel.com> > Signed-off-by: Lee Shawn C <shawn.c.lee@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); > }
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 >> >> 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@intel.com> >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Cc: José Roberto de Souza <jose.souza@intel.com> >> Cc: Cooper Chiou <cooper.chiou@intel.com> >> Cc: Khaled Almahallawy <khaled.almahallawy@intel.com> >> Signed-off-by: Lee Shawn C <shawn.c.lee@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); >> }
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. 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@intel.com> >>> Cc: Jani Nikula <jani.nikula@linux.intel.com> >>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >>> Cc: José Roberto de Souza <jose.souza@intel.com> >>> Cc: Cooper Chiou <cooper.chiou@intel.com> >>> Cc: Khaled Almahallawy <khaled.almahallawy@intel.com> >>> Signed-off-by: Lee Shawn C <shawn.c.lee@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); >>> } >
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@intel.com> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > Cc: Cooper Chiou <cooper.chiou@intel.com> > > > > Cc: Khaled Almahallawy <khaled.almahallawy@intel.com> > > > > Signed-off-by: Lee Shawn C <shawn.c.lee@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); > > > > } > > >
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@intel.com> > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > > Cc: Cooper Chiou <cooper.chiou@intel.com> > > > > > Cc: Khaled Almahallawy <khaled.almahallawy@intel.com> > > > > > Signed-off-by: Lee Shawn C <shawn.c.lee@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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); }
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. 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@intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Cc: Cooper Chiou <cooper.chiou@intel.com> Cc: Khaled Almahallawy <khaled.almahallawy@intel.com> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 43 ++++++++++++++---------- 1 file changed, 26 insertions(+), 17 deletions(-)