drm/i915/edp: Be less aggressive about changing link config on eDP

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

Cc Chris Wilson <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Manasi D Navare <[email protected]>

Cc Mika Kahola <[email protected]>

Cc Jani Nikula <[email protected]>

Fixes 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")

Signed-off-by Jim Bride <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c               |  4 +++-
 drivers/gpu/drm/i915/intel_dp_link_training.c | 15 ++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h              |  2 ++
 3 files changed, 19 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +3 -1

index 76c8a0b..4bd409c 100644
@@ -106,7 +106,7 @@ static const int default_rates[] = { 162000, 270000, 540000 };
  * If a CPU or PCH DP output is attached to an eDP panel, this function
  * will return true, and false otherwise.
  */
-static bool is_edp(struct intel_dp *intel_dp)
+bool is_edp(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 

@@ -4711,6 +4711,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);

@@ -5979,6 +5980,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;
 

drivers/gpu/drm/i915/intel_dp_link_training.c +13 -1

index 05907fa..67032cf 100644
@@ -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 = 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;
 	}
 

drivers/gpu/drm/i915/intel_drv.h +2 -0

index f91de9c..792bf547 100644
@@ -994,6 +994,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;

@@ -1548,6 +1549,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,

Signature · 1 line

2.7.4

Quoted 127 lines · Jim Bride

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

Cc Chris Wilson <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Manasi D Navare <[email protected]>

Cc Mika Kahola <[email protected]>

Cc Jani Nikula <[email protected]>

Fixes 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")

Signed-off-by Jim Bride <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c               |  4 +++-
 drivers/gpu/drm/i915/intel_dp_link_training.c | 15 ++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h              |  2 ++
 3 files changed, 19 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +3 -1

index 76c8a0b..4bd409c 100644
@@ -106,7 +106,7 @@ static const int default_rates[] = { 162000, 270000, 540000 };
  * If a CPU or PCH DP output is attached to an eDP panel, this function
  * will return true, and false otherwise.
  */
-static bool is_edp(struct intel_dp *intel_dp)
+bool is_edp(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 

@@ -4711,6 +4711,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);

@@ -5979,6 +5980,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;
 

drivers/gpu/drm/i915/intel_dp_link_training.c +13 -1

index 05907fa..67032cf 100644
@@ -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 = 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;
 	}
 

drivers/gpu/drm/i915/intel_dp_link_training.c intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) · line 313

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

@@ -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");
 	}

There are other cases in the channel equalization loop of 5 tries where if drm_dp_clock_recover_ok is not true then it just breaks from the for loop. Same in case of failure to update link train or failure to get link status. In this case we dont set train_set_valid to false sinec the tries is not 5 yet In this case since the train_set_valid was true when it entered this function, it will stay true without actually succeeding at ch eq.
Shouldnt we be setting train_set_valid to false in these cases?
Or is there something I am missing here?

Regards
Manasi

Quoted 16 lines

drivers/gpu/drm/i915/intel_drv.h +1 -0

index f91de9c..792bf547 100644
@@ -994,6 +994,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;

@@ -1548,6 +1549,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 }
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_drv.h +1 -0

 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,

Signature · 1 line

2.7.4

Quoted 128 lines · Manasi Navare

Quoted 127 lines · Jim Bride

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

Cc Chris Wilson <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Manasi D Navare <[email protected]>

Cc Mika Kahola <[email protected]>

Cc Jani Nikula <[email protected]>

Fixes 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")

Signed-off-by Jim Bride <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c               |  4 +++-
 drivers/gpu/drm/i915/intel_dp_link_training.c | 15 ++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h              |  2 ++
 3 files changed, 19 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +3 -1

index 76c8a0b..4bd409c 100644
@@ -106,7 +106,7 @@ static const int default_rates[] = { 162000, 270000, 540000 };
  * If a CPU or PCH DP output is attached to an eDP panel, this function
  * will return true, and false otherwise.
  */
-static bool is_edp(struct intel_dp *intel_dp)
+bool is_edp(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 

@@ -4711,6 +4711,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);

@@ -5979,6 +5980,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;
 

drivers/gpu/drm/i915/intel_dp_link_training.c +13 -1

index 05907fa..67032cf 100644
@@ -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 = 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;
 	}
 

Quoted 15 lines

drivers/gpu/drm/i915/intel_dp_link_training.c intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) · line 313

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

@@ -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");
 	}

There are other cases in the channel equalization loop of 5 tries where if drm_dp_clock_recover_ok is not true then it just breaks from the for loop. Same in case of failure to update link train or failure to get link status. In this case we dont set train_set_valid to false sinec the tries is not 5 yet In this case since the train_set_valid was true when it entered this function, it will stay true without actually succeeding at ch eq.
Shouldnt we be setting train_set_valid to false in these cases?
Or is there something I am missing here?

It's set to false before all of those, and then set back to true if the retrain works and we're on eDP. Making the change to do this was some review feedback from Chris on an earlier version of the patch.

Jim

Quoted 19 lines

Regards
Manasi

Quoted 16 lines

drivers/gpu/drm/i915/intel_drv.h +1 -0

index f91de9c..792bf547 100644
@@ -994,6 +994,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;

@@ -1548,6 +1549,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 }
 

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_drv.h +1 -0

 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,

Signature · 1 line

2.7.4

Quoted 51 lines · Jim Bride

Quoted 50 lines · Manasi Navare

Quoted 49 lines · Jim Bride

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

Cc Chris Wilson <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Manasi D Navare <[email protected]>

Cc Mika Kahola <[email protected]>

Cc Jani Nikula <[email protected]>

Fixes 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")

Signed-off-by Jim Bride <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c               |  4 +++-
 drivers/gpu/drm/i915/intel_dp_link_training.c | 15 ++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h              |  2 ++
 3 files changed, 19 insertions(+), 2 deletions(-)

Quoted 10 lines

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c static const int default_rates[] = { 162000, 270000, 540000 }; · line 109

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +1 -1

index 76c8a0b..4bd409c 100644
@@ -106,7 +106,7 @@ static const int default_rates[] = { 162000, 270000, 540000 };
  * If a CPU or PCH DP output is attached to an eDP panel, this function
  * will return true, and false otherwise.
  */
-static bool is_edp(struct intel_dp *intel_dp)
+bool is_edp(struct intel_dp *intel_dp)

Quoted 84 lines

Quoted 68 lines

Quoted 68 lines

drivers/gpu/drm/i915/intel_dp.c +2 -0

 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 

@@ -4711,6 +4711,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);

@@ -5979,6 +5980,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;
 

drivers/gpu/drm/i915/intel_dp_link_training.c +13 -1

index 05907fa..67032cf 100644
@@ -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 = 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;
 	}
 

Quoted 15 lines

drivers/gpu/drm/i915/intel_dp_link_training.c intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) · line 313

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

@@ -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");
 	}

There are other cases in the channel equalization loop of 5 tries where if drm_dp_clock_recover_ok is not true then it just breaks from the for loop. Same in case of failure to update link train or failure to get link status. In this case we dont set train_set_valid to false sinec the tries is not 5 yet In this case since the train_set_valid was true when it entered this function, it will stay true without actually succeeding at ch eq.
Shouldnt we be setting train_set_valid to false in these cases?
Or is there something I am missing here?

Quoted 7 lines

It's set to false before all of those, and then set back to true if the retrain works and we're on eDP. Making the change to do this was some review feedback from Chris on an earlier version of the patch.

Jim

I see that it is set to false before starting the clock recovery but after clock recovery succeeds, this is set to true and only then you will proceed with channel equalization. And in channel equalization, it never gets set to false in case of the failures I mentioned above since we just break there.
It then gets set to false only if the tries are 5 but it should also be set to false in case of other failures that I mentioned else we will end up leaving it to true even when channel EQ never succeeded.

Manasi

Quoted 19 lines

Quoted 19 lines

Regards
Manasi

Quoted 16 lines

drivers/gpu/drm/i915/intel_drv.h +1 -0

index f91de9c..792bf547 100644
@@ -994,6 +994,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;

@@ -1548,6 +1549,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 }
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_drv.h +1 -0

 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,

Signature · 1 line

2.7.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 145 lines · Jim Bride

Quoted 128 lines · Manasi Navare

Quoted 127 lines · Jim Bride

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

Cc Chris Wilson <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Manasi D Navare <[email protected]>

Cc Mika Kahola <[email protected]>

Cc Jani Nikula <[email protected]>

Fixes 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")

Signed-off-by Jim Bride <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c               |  4 +++-
 drivers/gpu/drm/i915/intel_dp_link_training.c | 15 ++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h              |  2 ++
 3 files changed, 19 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +3 -1

index 76c8a0b..4bd409c 100644
@@ -106,7 +106,7 @@ static const int default_rates[] = { 162000, 270000, 540000 };
  * If a CPU or PCH DP output is attached to an eDP panel, this function
  * will return true, and false otherwise.
  */
-static bool is_edp(struct intel_dp *intel_dp)
+bool is_edp(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 

@@ -4711,6 +4711,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);

@@ -5979,6 +5980,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;
 

drivers/gpu/drm/i915/intel_dp_link_training.c +13 -1

index 05907fa..67032cf 100644
@@ -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 = 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;
 	}
 

Quoted 15 lines

drivers/gpu/drm/i915/intel_dp_link_training.c intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp) · line 313

Quoted 7 lines

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

@@ -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");
 	}

There are other cases in the channel equalization loop of 5 tries where if drm_dp_clock_recover_ok is not true then it just breaks from the for loop. Same in case of failure to update link train or failure to get link status. In this case we dont set train_set_valid to false sinec the tries is not 5 yet In this case since the train_set_valid was true when it entered this function, it will stay true without actually succeeding at ch eq.
Shouldnt we be setting train_set_valid to false in these cases?
Or is there something I am missing here?

Quoted 7 lines

It's set to false before all of those, and then set back to true if the retrain works and we're on eDP. Making the change to do this was some review feedback from Chris on an earlier version of the patch.

Jim

I see that it is set to false before starting the clock recovery but after clock recovery succeeds, this is set to true and only then you will proceed with channel equalization. And in channel equalization, it never gets set to false in case of the failures I mentioned above since we just break there.
It then gets set to false only if the tries are 5 but it should also be set to false in case of other failures that I mentioned else we will end up leaving it to true even when channel EQ never succeeded.

Manasi

Quoted 19 lines

Quoted 19 lines

Regards
Manasi

Quoted 16 lines

drivers/gpu/drm/i915/intel_drv.h +1 -0

index f91de9c..792bf547 100644
@@ -994,6 +994,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;

@@ -1548,6 +1549,7 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 }
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_drv.h +1 -0

 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,

Signature · 1 line

2.7.4

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 <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Manasi D Navare <[email protected]>

Cc Mika Kahola <[email protected]>

Cc Jani Nikula <[email protected]>

Fixes 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")

Signed-off-by Jim Bride <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/intel_dp.c +2 -0

index e385658..38bc7e0 100644
@@ -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;
 

drivers/gpu/drm/i915/intel_dp_link_training.c +11 -1

index 05907fa..79fe369 100644
@@ -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 (;;) {

drivers/gpu/drm/i915/intel_dp_link_training.c +2 -0

 			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");
 	}
 

drivers/gpu/drm/i915/intel_drv.h +1 -0

index 2940d39..891c892 100644
@@ -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;

Quoted 106 lines · Jim Bride

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 <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Manasi D Navare <[email protected]>

Cc Mika Kahola <[email protected]>

Cc Jani Nikula <[email protected]>

Fixes 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")

Signed-off-by Jim Bride <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/intel_dp.c +2 -0

index e385658..38bc7e0 100644
@@ -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;
 

drivers/gpu/drm/i915/intel_dp_link_training.c +11 -1

index 05907fa..79fe369 100644
@@ -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)
 

drivers/gpu/drm/i915/intel_dp_link_training.c intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) · line 192

Quoted 3 lines

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

 		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...

Quoted 34 lines

drivers/gpu/drm/i915/intel_dp_link_training.c +2 -0

 			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");
 	}
 

drivers/gpu/drm/i915/intel_drv.h +1 -0

index 2940d39..891c892 100644
@@ -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)
 }
 

drivers/gpu/drm/i915/intel_drv.h static inline unsigned int intel_dp_unused_lane_mask(int lane_count) · line 1553

Quoted 2 lines

drivers/gpu/drm/i915/intel_drv.h +1 -0

 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...

Quoted 3 lines

drivers/gpu/drm/i915/intel_drv.h +0 -0

 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
v7: * Remove extraneous is_edp() prototype (Rodrigo)

Cc Chris Wilson <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Manasi D Navare <[email protected]>

Cc Mika Kahola <[email protected]>

Cc Jani Nikula <[email protected]>

Fixes 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")

Signed-off-by Jim Bride <[email protected]>

---
 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              |  1 +
 3 files changed, 17 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_dp.c +2 -0

index e385658..38bc7e0 100644
@@ -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;
 

drivers/gpu/drm/i915/intel_dp_link_training.c +14 -1

index 05907fa..79fe369 100644
@@ -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");
 	}
 

drivers/gpu/drm/i915/intel_drv.h +1 -0

index 2940d39..f4354ec 100644
@@ -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;

Signature · 1 line

2.7.4

The patch looks good for eDP link training optimizations.

Reviewed-by Manasi Navare <[email protected]>

Manasi

Quoted 140 lines · Jim Bride

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
v7: * Remove extraneous is_edp() prototype (Rodrigo)

Cc Chris Wilson <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Manasi D Navare <[email protected]>

Cc Mika Kahola <[email protected]>

Cc Jani Nikula <[email protected]>

Fixes 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")

Signed-off-by Jim Bride <[email protected]>

---
 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              |  1 +
 3 files changed, 17 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_dp.c +2 -0

index e385658..38bc7e0 100644
@@ -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;
 

drivers/gpu/drm/i915/intel_dp_link_training.c +14 -1

index 05907fa..79fe369 100644
@@ -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");
 	}
 

drivers/gpu/drm/i915/intel_drv.h +1 -0

index 2940d39..f4354ec 100644
@@ -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;

Quoted 4 lines

drivers/gpu/drm/i915/intel_drv.h +0 -0

 	int panel_power_cycle_delay;

Signature · 1 line

2.7.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 3 lines · Manasi Navare

The patch looks good for eDP link training optimizations.

Reviewed-by Manasi Navare <[email protected]>

I haven't merged this patch yet because I'd like an Ack from Jani.

Also I'd like to hear from Mika if he believes it is safe or not.

"It has been found out that in some HW combination the DisplayPort
fast link training feature caused screen flickering. Let's revert this feature for now until we can ensure that the feature works for all platforms."

I don't want to merge this patch to fix a feature that is disabled by default with the risk of bringing flickerings back.

But even if we decide to go ahead and merge I believe we need to resend the test and collect a full round of CI that now runs all IGT tests.

Thanks,
Rodrigo.

Quoted 152 lines

Manasi

Quoted 144 lines · Jim Bride

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
v7: * Remove extraneous is_edp() prototype (Rodrigo)

Cc Chris Wilson <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Manasi D Navare <[email protected]>

Cc Mika Kahola <[email protected]>

Cc Jani Nikula <[email protected]>

Fixes 34511dce4 ("drm/i915: Revert DisplayPort fast link training feature")

Signed-off-by Jim Bride <[email protected]>

---
 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              |  1 +
 3 files changed, 17 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_dp.c +2 -0

index e385658..38bc7e0 100644
@@ -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;
 

drivers/gpu/drm/i915/intel_dp_link_training.c +14 -1

index 05907fa..79fe369 100644
@@ -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");
 	}
 

drivers/gpu/drm/i915/intel_drv.h +1 -0

index 2940d39..f4354ec 100644
@@ -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;

Signature · 1 line

2.7.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 4 lines · Manasi Navare

The patch looks good for eDP link training optimizations.

Reviewed-by Manasi Navare <[email protected]>

I haven't merged this patch yet because I'd like an Ack from Jani.

Also I'd like to hear from Mika if he believes it is safe or not.

On his revert commit he wrote:
"It has been found out that in some HW combination the DisplayPort fast link training feature caused screen flickering. Let's revert this feature for now until we can ensure that the feature works for all platforms."

I don't want to merge this patch to fix a feature that is disabled by default with the risk of bringing flickerings back.

But even if we decide to go ahead and merge I believe we need to resend the test and collect a full round of CI that now runs all IGT tests.

Thanks,
Rodrigo.

Quoted 190 lines

Manasi

Quoted 185 lines · Jim Bride

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
v7: * Remove extraneous is_edp() prototype (Rodrigo)

Cc Chris Wilson <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Manasi D Navare <[email protected]>

Cc Mika Kahola <[email protected]>

Cc Jani Nikula <[email protected]>

Fixes 34511dce4 ("drm/i915: Revert DisplayPort fast link

training feature")

Signed-off-by Jim Bride <[email protected]>

---
 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              |  1 +

 3 files changed, 17 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_dp.c +0 -0

index e385658..38bc7e0 100644
@@ -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;

drivers/gpu/drm/i915/intel_dp.c +1 -0

+		intel_dp->train_set_valid = false;

  }

  intel_dp_print_rates(intel_dp);

drivers/gpu/drm/i915/intel_dp.c +0 -0

@@ -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;

drivers/gpu/drm/i915/intel_dp.c +1 -0

+	intel_dp->train_set_valid = false;

  intel_dp->pps_pipe = INVALID_PIPE;
  intel_dp->active_pipe = INVALID_PIPE;

drivers/gpu/drm/i915/intel_dp_link_training.c +2 -1

index 05907fa..79fe369 100644
@@ -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); }

drivers/gpu/drm/i915/intel_dp_link_training.c +0 -0

@@ -162,9 +163,18 @@ intel_dp_link_training_clock_recovery(struct
 intel_dp *intel_dp)

         DP_TRAINING_PATTERN_1 | DP_LINK_SCRAMBLING_DISABL E)) {
  DRM_ERROR("failed to enable link training\n");

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+		intel_dp->train_set_valid = false;

  return false;
  }

drivers/gpu/drm/i915/intel_dp_link_training.c +2 -0

+	/*
+	 * The initial set of link parameters are set by this

point, so go

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+	 * ahead and set intel_dp->train_set_valid to false in

case any of

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+	 * the succeeding steps fail.  It will be set back to

true if we were

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+	 * able to achieve clock recovery in the specified

configuration.

drivers/gpu/drm/i915/intel_dp_link_training.c +3 -0

+	 */
+	intel_dp->train_set_valid = false;
+

  voltage_tries = 1;
  max_vswing_tries = 0;
  for (;;) {

drivers/gpu/drm/i915/intel_dp_link_training.c +0 -0

@@ -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");

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+			intel_dp->train_set_valid =

intel_dp_is_edp(intel_dp);
  return true;
  }

drivers/gpu/drm/i915/intel_dp_link_training.c +0 -0

@@ -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");

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+		intel_dp->train_set_valid = false;

  return false;
  }

drivers/gpu/drm/i915/intel_dp_link_training.c +0 -0

@@ -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);

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+		intel_dp->train_set_valid = false;

  DRM_DEBUG_KMS("Channel equalization failed 5 times\n");
  }

drivers/gpu/drm/i915/intel_drv.h +1 -0

index 2940d39..f4354ec 100644
@@ -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;

Quoted 7 lines · Rodrigo Vivi

Quoted 4 lines · Manasi Navare

The patch looks good for eDP link training optimizations.

Reviewed-by Manasi Navare <[email protected]>

I haven't merged this patch yet because I'd like an Ack from Jani.

Quoted 7 lines

Also I'd like to hear from Mika if he believes it is safe or not.

On his revert commit he wrote:
"It has been found out that in some HW combination the DisplayPort fast link training feature caused screen flickering. Let's revert this feature for now until we can ensure that the feature works for all platforms."

At the time of this revert there were quite a few bug reports of flickering. The community felt that this "fast link training" was to blame for flickering issues and therefore we decided to revert that. However, it turned out later on that this revert didn't solve the flickering issues. Flickering was gone once watermark work landed.

Therefore, I feel that this feature is safe to re-enable.

Quoted 195 lines

I don't want to merge this patch to fix a feature that is disabled by default with the risk of bringing flickerings back.

But even if we decide to go ahead and merge I believe we need to resend the test and collect a full round of CI that now runs all IGT tests.

Thanks,
Rodrigo.

Quoted 182 lines

Manasi

Quoted 177 lines · Jim Bride

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
v7: * Remove extraneous is_edp() prototype (Rodrigo)

Cc Chris Wilson <[email protected]>

Cc Rodrigo Vivi <[email protected]>

Cc Paulo Zanoni <[email protected]>

Cc Manasi D Navare <[email protected]>

Cc Mika Kahola <[email protected]>

Cc Jani Nikula <[email protected]>

Fixes 34511dce4 ("drm/i915: Revert DisplayPort fast link

training feature")

Signed-off-by Jim Bride <[email protected]>

---
 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              |  1 +

 3 files changed, 17 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_dp.c +0 -0

index e385658..38bc7e0 100644
@@ -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;

drivers/gpu/drm/i915/intel_dp.c +1 -0

+		intel_dp->train_set_valid = false;

  }

  intel_dp_print_rates(intel_dp);

drivers/gpu/drm/i915/intel_dp.c +0 -0

@@ -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;

drivers/gpu/drm/i915/intel_dp.c +1 -0

+	intel_dp->train_set_valid = false;

  intel_dp->pps_pipe = INVALID_PIPE;
  intel_dp->active_pipe = INVALID_PIPE;

drivers/gpu/drm/i915/intel_dp_link_training.c +2 -1

index 05907fa..79fe369 100644
@@ -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); }

drivers/gpu/drm/i915/intel_dp_link_training.c +0 -0

@@ -162,9 +163,18 @@ intel_dp_link_training_clock_recovery(struct
 intel_dp *intel_dp)

         DP_TRAINING_PATTERN_1 | DP_LINK_SCRAMBLING_DISABL E)) {
  DRM_ERROR("failed to enable link training\n");

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+		intel_dp->train_set_valid = false;

  return false;
  }

drivers/gpu/drm/i915/intel_dp_link_training.c +2 -0

+	/*
+	 * The initial set of link parameters are set by this

point, so go

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+	 * ahead and set intel_dp->train_set_valid to false in

case any of

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+	 * the succeeding steps fail.  It will be set back to

true if we were

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+	 * able to achieve clock recovery in the specified

configuration.

drivers/gpu/drm/i915/intel_dp_link_training.c +3 -0

+	 */
+	intel_dp->train_set_valid = false;
+

  voltage_tries = 1;
  max_vswing_tries = 0;
  for (;;) {

drivers/gpu/drm/i915/intel_dp_link_training.c +0 -0

@@ -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");

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+			intel_dp->train_set_valid =

intel_dp_is_edp(intel_dp);
  return true;
  }

drivers/gpu/drm/i915/intel_dp_link_training.c +0 -0

@@ -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");

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+		intel_dp->train_set_valid = false;

  return false;
  }

drivers/gpu/drm/i915/intel_dp_link_training.c +0 -0

@@ -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);

drivers/gpu/drm/i915/intel_dp_link_training.c +1 -0

+		intel_dp->train_set_valid = false;

  DRM_DEBUG_KMS("Channel equalization failed 5 times\n");
  }

drivers/gpu/drm/i915/intel_drv.h +0 -0

index 2940d39..f4354ec 100644

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_drv.h +1 -0

@@ -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;

Signature · 1 line

Mika Kahola - Intel OTC