| Message ID | 1503349416-29151-1-git-send-email-jim.bride@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2017-08-21 at 14:03 -0700, Jim Bride wrote: > This set of changes has some history to them. There were several attempts > to add what was called "fast link training" to i915, which actually wasn't > fast link training as per the DP spec. These changes were: > > commit 5fa836a9d859 ("drm/i915: DP link training optimization") > commit 4e96c97742f4 ("drm/i915: eDP link training optimization") > > which were eventually hand-reverted by: > > commit 34511dce4b35 ("drm/i915: Revert DisplayPort fast link training > feature") > > in kernel 4.7-rc4. The eDP pieces of the above revert, however, had some > very bad side-effects on PSR functionality on Skylake. The issue at > hand is that when PSR exits i915 briefly emits TP1 followed by TP2/3 > (depending on the original link configuration) in order to quickly get > the source and sink back in synchronization across the link before handing > control back to the i915. There's an assumption that none of the link > configuration information has changed (and thus it's still valid) since the > last full link training operation. The revert above was identified via a > bisect as the cause of some of Skylake's PSR woes. This patch, largely > based on commit 4e96c97742f4 ("drm/i915: eDP link training optimization") > puts the eDP portions of this patch back in place. None of the flickering > issues that spurred the revert have been seen, and I suspect the real > culprits here were addressed by some of the recent link training changes > that Manasi has implemented, and PSR on Skylake is definitely more happy > with these changes in-place. > > v2 and v3: Rebase > v4: * Clean up accesses to train_set_valid a bit for easier > reading. (Chris) > * Rebase > v5: * Checkpatch cleanup > * Rebase > v6: * is_edp() => intel_dp_is_edp() > * rebase > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Manasi D Navare <manasi.d.navare@intel.com> > Cc: Mika Kahola <mika.kahola@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature") > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 2 ++ > drivers/gpu/drm/i915/intel_dp_link_training.c | 15 ++++++++++++++- > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > 3 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index e385658..38bc7e0 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4750,6 +4750,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp); > > intel_dp->reset_link_params = false; > + intel_dp->train_set_valid = false; > } > > intel_dp_print_rates(intel_dp); > @@ -6019,6 +6020,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > intel_dp_set_source_rates(intel_dp); > > intel_dp->reset_link_params = true; > + intel_dp->train_set_valid = false; > intel_dp->pps_pipe = INVALID_PIPE; > intel_dp->active_pipe = INVALID_PIPE; > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > index 05907fa..79fe369 100644 > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > @@ -94,7 +94,8 @@ static bool > intel_dp_reset_link_train(struct intel_dp *intel_dp, > uint8_t dp_train_pat) > { > - memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > + if (!intel_dp->train_set_valid) > + memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > intel_dp_set_signal_levels(intel_dp); > return intel_dp_set_link_train(intel_dp, dp_train_pat); > } > @@ -162,9 +163,18 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) > DP_TRAINING_PATTERN_1 | > DP_LINK_SCRAMBLING_DISABLE)) { > DRM_ERROR("failed to enable link training\n"); > + intel_dp->train_set_valid = false; > return false; > } > > + /* > + * The initial set of link parameters are set by this point, so go > + * ahead and set intel_dp->train_set_valid to false in case any of > + * the succeeding steps fail. It will be set back to true if we were > + * able to achieve clock recovery in the specified configuration. > + */ > + intel_dp->train_set_valid = false; > + > voltage_tries = 1; > max_vswing_tries = 0; > for (;;) { > @@ -179,6 +189,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) > > if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) { > DRM_DEBUG_KMS("clock recovery OK\n"); > + intel_dp->train_set_valid = intel_dp_is_edp(intel_dp); Is this one really enough for this case? I wonder if we should merge is_edp and intel_dp_is_edp somehow instead of fighting with names and their meanings... > return true; > } > > @@ -256,6 +267,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) > training_pattern | > DP_LINK_SCRAMBLING_DISABLE)) { > DRM_ERROR("failed to start channel equalization\n"); > + intel_dp->train_set_valid = false; > return false; > } > > @@ -296,6 +308,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) > /* Try 5 times, else fail and try at lower BW */ > if (tries == 5) { > intel_dp_dump_link_status(link_status); > + intel_dp->train_set_valid = false; > DRM_DEBUG_KMS("Channel equalization failed 5 times\n"); > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 2940d39..891c892 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -995,6 +995,7 @@ struct intel_dp { > struct drm_dp_aux aux; > enum intel_display_power_domain aux_power_domain; > uint8_t train_set[4]; > + bool train_set_valid; > int panel_power_up_delay; > int panel_power_down_delay; > int panel_power_cycle_delay; > @@ -1549,6 +1550,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count) > } > > bool intel_dp_read_dpcd(struct intel_dp *intel_dp); > +bool is_edp(struct intel_dp *intel_dp); it seems you don't need this anymore... > int intel_dp_link_required(int pixel_clock, int bpp); > int intel_dp_max_data_rate(int max_link_clock, int max_lanes); > bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
On Mon, Aug 21, 2017 at 11:27:37PM +0000, Vivi, Rodrigo wrote: > On Mon, 2017-08-21 at 14:03 -0700, Jim Bride wrote: > > This set of changes has some history to them. There were several attempts > > to add what was called "fast link training" to i915, which actually wasn't > > fast link training as per the DP spec. These changes were: > > > > commit 5fa836a9d859 ("drm/i915: DP link training optimization") > > commit 4e96c97742f4 ("drm/i915: eDP link training optimization") > > > > which were eventually hand-reverted by: > > > > commit 34511dce4b35 ("drm/i915: Revert DisplayPort fast link training > > feature") > > > > in kernel 4.7-rc4. The eDP pieces of the above revert, however, had some > > very bad side-effects on PSR functionality on Skylake. The issue at > > hand is that when PSR exits i915 briefly emits TP1 followed by TP2/3 > > (depending on the original link configuration) in order to quickly get > > the source and sink back in synchronization across the link before handing > > control back to the i915. There's an assumption that none of the link > > configuration information has changed (and thus it's still valid) since the > > last full link training operation. The revert above was identified via a > > bisect as the cause of some of Skylake's PSR woes. This patch, largely > > based on commit 4e96c97742f4 ("drm/i915: eDP link training optimization") > > puts the eDP portions of this patch back in place. None of the flickering > > issues that spurred the revert have been seen, and I suspect the real > > culprits here were addressed by some of the recent link training changes > > that Manasi has implemented, and PSR on Skylake is definitely more happy > > with these changes in-place. > > > > v2 and v3: Rebase > > v4: * Clean up accesses to train_set_valid a bit for easier > > reading. (Chris) > > * Rebase > > v5: * Checkpatch cleanup > > * Rebase > > v6: * is_edp() => intel_dp_is_edp() > > * rebase > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Manasi D Navare <manasi.d.navare@intel.com> > > Cc: Mika Kahola <mika.kahola@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature") > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 2 ++ > > drivers/gpu/drm/i915/intel_dp_link_training.c | 15 ++++++++++++++- > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > 3 files changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index e385658..38bc7e0 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4750,6 +4750,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > > intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp); > > > > intel_dp->reset_link_params = false; > > + intel_dp->train_set_valid = false; > > } > > > > intel_dp_print_rates(intel_dp); > > @@ -6019,6 +6020,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > intel_dp_set_source_rates(intel_dp); > > > > intel_dp->reset_link_params = true; > > + intel_dp->train_set_valid = false; > > intel_dp->pps_pipe = INVALID_PIPE; > > intel_dp->active_pipe = INVALID_PIPE; > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > > index 05907fa..79fe369 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > @@ -94,7 +94,8 @@ static bool > > intel_dp_reset_link_train(struct intel_dp *intel_dp, > > uint8_t dp_train_pat) > > { > > - memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > > + if (!intel_dp->train_set_valid) > > + memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); > > intel_dp_set_signal_levels(intel_dp); > > return intel_dp_set_link_train(intel_dp, dp_train_pat); > > } > > @@ -162,9 +163,18 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) > > DP_TRAINING_PATTERN_1 | > > DP_LINK_SCRAMBLING_DISABLE)) { > > DRM_ERROR("failed to enable link training\n"); > > + intel_dp->train_set_valid = false; > > return false; > > } > > > > + /* > > + * The initial set of link parameters are set by this point, so go > > + * ahead and set intel_dp->train_set_valid to false in case any of > > + * the succeeding steps fail. It will be set back to true if we were > > + * able to achieve clock recovery in the specified configuration. > > + */ > > + intel_dp->train_set_valid = false; > > + > > voltage_tries = 1; > > max_vswing_tries = 0; > > for (;;) { > > @@ -179,6 +189,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) > > > > if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) { > > DRM_DEBUG_KMS("clock recovery OK\n"); > > + intel_dp->train_set_valid = intel_dp_is_edp(intel_dp); > > Is this one really enough for this case? Yep, it's the only success path. > I wonder if we should merge is_edp and intel_dp_is_edp somehow instead > of fighting with names and their meanings... Jani patched the two functions while I was wrestling with how to rename is_edp(). Even though it's potentially confusing, I do think intel_dp_is_edp() is probably as good as it gets. > > return true; > > } > > > > @@ -256,6 +267,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) > > training_pattern | > > DP_LINK_SCRAMBLING_DISABLE)) { > > DRM_ERROR("failed to start channel equalization\n"); > > + intel_dp->train_set_valid = false; > > return false; > > } > > > > @@ -296,6 +308,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) > > /* Try 5 times, else fail and try at lower BW */ > > if (tries == 5) { > > intel_dp_dump_link_status(link_status); > > + intel_dp->train_set_valid = false; > > DRM_DEBUG_KMS("Channel equalization failed 5 times\n"); > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 2940d39..891c892 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -995,6 +995,7 @@ struct intel_dp { > > struct drm_dp_aux aux; > > enum intel_display_power_domain aux_power_domain; > > uint8_t train_set[4]; > > + bool train_set_valid; > > int panel_power_up_delay; > > int panel_power_down_delay; > > int panel_power_cycle_delay; > > @@ -1549,6 +1550,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count) > > } > > > > bool intel_dp_read_dpcd(struct intel_dp *intel_dp); > > +bool is_edp(struct intel_dp *intel_dp); Whoops. I'll fix this. > it seems you don't need this anymore... > > > int intel_dp_link_required(int pixel_clock, int bpp); > > int intel_dp_max_data_rate(int max_link_clock, int max_lanes); > > bool intel_digital_port_connected(struct drm_i915_private *dev_priv, >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index e385658..38bc7e0 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4750,6 +4750,7 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp); intel_dp->reset_link_params = false; + intel_dp->train_set_valid = false; } intel_dp_print_rates(intel_dp); @@ -6019,6 +6020,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, intel_dp_set_source_rates(intel_dp); intel_dp->reset_link_params = true; + intel_dp->train_set_valid = false; intel_dp->pps_pipe = INVALID_PIPE; intel_dp->active_pipe = INVALID_PIPE; diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c index 05907fa..79fe369 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -94,7 +94,8 @@ static bool intel_dp_reset_link_train(struct intel_dp *intel_dp, uint8_t dp_train_pat) { - memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); + if (!intel_dp->train_set_valid) + memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set)); intel_dp_set_signal_levels(intel_dp); return intel_dp_set_link_train(intel_dp, dp_train_pat); } @@ -162,9 +163,18 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) DP_TRAINING_PATTERN_1 | DP_LINK_SCRAMBLING_DISABLE)) { DRM_ERROR("failed to enable link training\n"); + intel_dp->train_set_valid = false; return false; } + /* + * The initial set of link parameters are set by this point, so go + * ahead and set intel_dp->train_set_valid to false in case any of + * the succeeding steps fail. It will be set back to true if we were + * able to achieve clock recovery in the specified configuration. + */ + intel_dp->train_set_valid = false; + voltage_tries = 1; max_vswing_tries = 0; for (;;) { @@ -179,6 +189,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) { DRM_DEBUG_KMS("clock recovery OK\n"); + intel_dp->train_set_valid = intel_dp_is_edp(intel_dp); return true; } @@ -256,6 +267,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) training_pattern | DP_LINK_SCRAMBLING_DISABLE)) { DRM_ERROR("failed to start channel equalization\n"); + intel_dp->train_set_valid = false; return false; } @@ -296,6 +308,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) /* Try 5 times, else fail and try at lower BW */ if (tries == 5) { intel_dp_dump_link_status(link_status); + intel_dp->train_set_valid = false; DRM_DEBUG_KMS("Channel equalization failed 5 times\n"); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 2940d39..891c892 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -995,6 +995,7 @@ struct intel_dp { struct drm_dp_aux aux; enum intel_display_power_domain aux_power_domain; uint8_t train_set[4]; + bool train_set_valid; int panel_power_up_delay; int panel_power_down_delay; int panel_power_cycle_delay; @@ -1549,6 +1550,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count) } bool intel_dp_read_dpcd(struct intel_dp *intel_dp); +bool is_edp(struct intel_dp *intel_dp); int intel_dp_link_required(int pixel_clock, int bpp); int intel_dp_max_data_rate(int max_link_clock, int max_lanes); bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
This set of changes has some history to them. There were several attempts to add what was called "fast link training" to i915, which actually wasn't fast link training as per the DP spec. These changes were: commit 5fa836a9d859 ("drm/i915: DP link training optimization") commit 4e96c97742f4 ("drm/i915: eDP link training optimization") which were eventually hand-reverted by: commit 34511dce4b35 ("drm/i915: Revert DisplayPort fast link training feature") in kernel 4.7-rc4. The eDP pieces of the above revert, however, had some very bad side-effects on PSR functionality on Skylake. The issue at hand is that when PSR exits i915 briefly emits TP1 followed by TP2/3 (depending on the original link configuration) in order to quickly get the source and sink back in synchronization across the link before handing control back to the i915. There's an assumption that none of the link configuration information has changed (and thus it's still valid) since the last full link training operation. The revert above was identified via a bisect as the cause of some of Skylake's PSR woes. This patch, largely based on commit 4e96c97742f4 ("drm/i915: eDP link training optimization") puts the eDP portions of this patch back in place. None of the flickering issues that spurred the revert have been seen, and I suspect the real culprits here were addressed by some of the recent link training changes that Manasi has implemented, and PSR on Skylake is definitely more happy with these changes in-place. v2 and v3: Rebase v4: * Clean up accesses to train_set_valid a bit for easier reading. (Chris) * Rebase v5: * Checkpatch cleanup * Rebase v6: * is_edp() => intel_dp_is_edp() * rebase Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Manasi D Navare <manasi.d.navare@intel.com> Cc: Mika Kahola <mika.kahola@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Fixes: 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature") Signed-off-by: Jim Bride <jim.bride@linux.intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 2 ++ drivers/gpu/drm/i915/intel_dp_link_training.c | 15 ++++++++++++++- drivers/gpu/drm/i915/intel_drv.h | 2 ++ 3 files changed, 18 insertions(+), 1 deletion(-)