| Message ID | 1493373489-31087-1-git-send-email-shawn.c.lee@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28/04/2017 10:58, Lee, Shawn C wrote: > From: "Lee, Shawn C" <shawn.c.lee@intel.com> > > Display driver read DPCD register 0x202, 0x203 and 0x204 to identify > eDP sink status. If PSR exit is ongoing at eDP sink, and eDP source > read these registers at the same time. Panel will report EQ & symbol > lock not done. It will cause panel display flicking. > So driver have to make sure PSR already exit before read link status. > > Change log: > v2: > - Use intel_wait_for_register() to replace I915_READ(). > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99639 > TEST=Reboot DUT and no flicking on local display at login screen > > Cc: Cooper Chiou <cooper.chiou@intel.com> > Cc: Gary C Wang <gary.c.wang@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Jim Bride <jim.bride@intel.com> > Cc: Ryan Lin <ryan.lin@intel.com> > > Signed-off-by: Shawn Lee <shawn.c.lee@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 33 ++++++++++++++++++++++++++++----- > 1 file changed, 28 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 08834f74d396..099b6c0605a7 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4252,19 +4252,34 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > } > > static void > +intel_edp_wait_PSR_exit(struct intel_dp *intel_dp) > +{ > + struct drm_device *dev = intel_dp_to_dev(intel_dp); > + struct drm_i915_private *dev_priv = dev->dev_private; ... dev_priv = to_i915(dev); > + u16 count = 100; > + > + while (count--) > + if (!intel_wait_for_register(dev_priv, > + EDP_PSR_STATUS_CTL, > + (EDP_PSR_STATUS_SENDING_TP1 | > + EDP_PSR_STATUS_SENDING_TP2_TP3 | > + EDP_PSR_STATUS_SENDING_IDLE | > + EDP_PSR_STATUS_AUX_SENDING), > + 0, > + 1)) > + return; > +} You don't need your own retry loop - intel_wait_for_register will do that for you. But it will only check every millisecond, if the inital busy wait for 2us wasn't successful. I've noticed that your initial patch had a 10ms timeout with a 100us period. If you had a good reason for that, then with this conversion you only have a 1ms timeout with a 1ms period, so if you need a shorter period, this solution is not ideal for you. I've complicated the explanation a bit - but a bottom line is, v1 and v2 are a lot different. Depending on the typical response time one of the two will have a much worse latency. > + > +static void > intel_dp_check_link_status(struct intel_dp *intel_dp) > { > struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; > struct drm_device *dev = intel_dp_to_dev(intel_dp); > + struct drm_i915_private *dev_priv = dev->dev_private; to_i915 also. > u8 link_status[DP_LINK_STATUS_SIZE]; > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); > > - if (!intel_dp_get_link_status(intel_dp, link_status)) { > - DRM_ERROR("Failed to get link status\n"); > - return; > - } > - > if (!intel_encoder->base.crtc) > return; > > @@ -4278,6 +4293,14 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) > if (!intel_dp_link_params_valid(intel_dp)) > return; > > + if (is_edp(intel_dp) && dev_priv->psr.enabled) > + intel_edp_wait_PSR_exit(intel_dp); > + > + if (!intel_dp_get_link_status(intel_dp, link_status)) { > + DRM_ERROR("Failed to get link status\n"); > + return; > + } > + > /* Retrain if Channel EQ or CR not ok */ > if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { > DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", > Regards, Tvrtko
On 28/04/2017 10:58, Lee, Shawn C wrote: >> From: "Lee, Shawn C" <shawn.c.lee@intel.com> >> >> Display driver read DPCD register 0x202, 0x203 and 0x204 to identify >> eDP sink status. If PSR exit is ongoing at eDP sink, and eDP source >> read these registers at the same time. Panel will report EQ & symbol >> lock not done. It will cause panel display flicking. >> So driver have to make sure PSR already exit before read link status. >> >> Change log: >> v2: >> - Use intel_wait_for_register() to replace I915_READ(). >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99639 >> TEST=Reboot DUT and no flicking on local display at login screen >> >> Cc: Cooper Chiou <cooper.chiou@intel.com> >> Cc: Gary C Wang <gary.c.wang@intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Jim Bride <jim.bride@intel.com> >> Cc: Ryan Lin <ryan.lin@intel.com> >> >> Signed-off-by: Shawn Lee <shawn.c.lee@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 33 ++++++++++++++++++++++++++++----- >> 1 file changed, 28 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c >> b/drivers/gpu/drm/i915/intel_dp.c index 08834f74d396..099b6c0605a7 >> 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4252,19 +4252,34 @@ static void >> intel_dp_handle_test_request(struct intel_dp *intel_dp) } >> >> static void >> +intel_edp_wait_PSR_exit(struct intel_dp *intel_dp) { >> + struct drm_device *dev = intel_dp_to_dev(intel_dp); >> + struct drm_i915_private *dev_priv = dev->dev_private; > >.... dev_priv = to_i915(dev); > I will use to_i915() to replace it at patch v3. >> + u16 count = 100; >> + >> + while (count--) >> + if (!intel_wait_for_register(dev_priv, >> + EDP_PSR_STATUS_CTL, >> + (EDP_PSR_STATUS_SENDING_TP1 | >> + EDP_PSR_STATUS_SENDING_TP2_TP3 | >> + EDP_PSR_STATUS_SENDING_IDLE | >> + EDP_PSR_STATUS_AUX_SENDING), >> + 0, >> + 1)) >> + return; >> +} > >You don't need your own retry loop - intel_wait_for_register will do that for you. But it will only check every millisecond, if the inital busy wait for 2us wasn't successful. > >I've noticed that your initial patch had a 10ms timeout with a 100us period. If you had a good reason for that, then with this conversion you only have a 1ms timeout with a 1ms period, so if you need a shorter period, this solution is not ideal for you. > >I've complicated the explanation a bit - but a bottom line is, v1 and v2 are a lot different. Depending on the typical response time one of the two will have a much worse latency. > Here is a little tricky to know when sink's PSR state transit to "PSR inactive" and timing re-sync is complete after PSR exit. So if SRD_STATUS shows TP1, TP2, TP3 or idle pattern was not sent to sink side. That means Main-Link Training is not ongoing. But the time for link training is hard to predict. So use a loop to monitor it every 1ms until link training was done. >> + >> +static void >> intel_dp_check_link_status(struct intel_dp *intel_dp) { >> struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> + struct drm_i915_private *dev_priv = dev->dev_private; > >to_i915 also. > I will use to_i915() to replace it at patch v3. >> u8 link_status[DP_LINK_STATUS_SIZE]; >> >> WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); >> >> - if (!intel_dp_get_link_status(intel_dp, link_status)) { >> - DRM_ERROR("Failed to get link status\n"); >> - return; >> - } >> - >> if (!intel_encoder->base.crtc) >> return; >> >> @@ -4278,6 +4293,14 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) >> if (!intel_dp_link_params_valid(intel_dp)) >> return; >> >> + if (is_edp(intel_dp) && dev_priv->psr.enabled) >> + intel_edp_wait_PSR_exit(intel_dp); >> + >> + if (!intel_dp_get_link_status(intel_dp, link_status)) { >> + DRM_ERROR("Failed to get link status\n"); >> + return; >> + } >> + >> /* Retrain if Channel EQ or CR not ok */ >> if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { >> DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", > > >Regards, > >Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 08834f74d396..099b6c0605a7 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4252,19 +4252,34 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) } static void +intel_edp_wait_PSR_exit(struct intel_dp *intel_dp) +{ + struct drm_device *dev = intel_dp_to_dev(intel_dp); + struct drm_i915_private *dev_priv = dev->dev_private; + u16 count = 100; + + while (count--) + if (!intel_wait_for_register(dev_priv, + EDP_PSR_STATUS_CTL, + (EDP_PSR_STATUS_SENDING_TP1 | + EDP_PSR_STATUS_SENDING_TP2_TP3 | + EDP_PSR_STATUS_SENDING_IDLE | + EDP_PSR_STATUS_AUX_SENDING), + 0, + 1)) + return; +} + +static void intel_dp_check_link_status(struct intel_dp *intel_dp) { struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; struct drm_device *dev = intel_dp_to_dev(intel_dp); + struct drm_i915_private *dev_priv = dev->dev_private; u8 link_status[DP_LINK_STATUS_SIZE]; WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); - if (!intel_dp_get_link_status(intel_dp, link_status)) { - DRM_ERROR("Failed to get link status\n"); - return; - } - if (!intel_encoder->base.crtc) return; @@ -4278,6 +4293,14 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) if (!intel_dp_link_params_valid(intel_dp)) return; + if (is_edp(intel_dp) && dev_priv->psr.enabled) + intel_edp_wait_PSR_exit(intel_dp); + + if (!intel_dp_get_link_status(intel_dp, link_status)) { + DRM_ERROR("Failed to get link status\n"); + return; + } + /* Retrain if Channel EQ or CR not ok */ if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",