On Wed, Aug 03, 2016 at 08:07:38PM -0700, Dhinakaran Pandiyan wrote:

@@ -2588,7 +2592,7 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp, *DP |= DP_LINK_TRAIN_PAT_2_CPT; break; case DP_TRAINING_PATTERN_3:

• 	DRM_ERROR("DP training pattern 3 not supported\n");
  

• 	DRM_ERROR("TPS3 not supported, using TPS2 instead\n");
  *DP |= DP_LINK_TRAIN_PAT_2_CPT;
  break;
  

  }

@@ -2613,7 +2617,7 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp, if (IS_CHERRYVIEW(dev)) { *DP |= DP_LINK_TRAIN_PAT_3_CHV; } else {

• 		DRM_ERROR("DP training pattern 3 not supported\n");
  

• 		DRM_ERROR("TPS3 not supported, using TPS2 instead\n");
  	*DP |= DP_LINK_TRAIN_PAT_2;
  

Given that you have a fallback plan and if the fallback plan fails you alert the user with an error already, these aren't errors but debug. -Chris

On Thu, Aug 04, 2016 at 04:11:06AM +0100, Chris Wilson wrote:

On Wed, Aug 03, 2016 at 08:07:39PM -0700, Dhinakaran Pandiyan wrote:

...

Since a DRM function that reads link DP link status is available, let's use that instead of the i915 clone.

Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 3ca33bd..c5c0201 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2863,17 +2863,6 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder) chv_phy_post_pll_disable(encoder); }

-/*

• • Fetch AUX CH registers 0x202 - 0x207 which contain
• • link status information
• */

-bool -intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE]) -{

• return drm_dp_dpcd_read(&intel_dp->aux, DP_LANE0_1_STATUS, link_status,

• 		DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
  

Or just wrap it here since you add the same code everywhere, and don't care what the invalid length actually is.

I don't think anyone cares actually, and I can't really imagine anyone being happy with a partial link status block. So might be a good idea to just change drm_dp_dpcd_read_link_status() to return success vs. failure only.

On Wed, Aug 03, 2016 at 08:07:40PM -0700, Dhinakaran Pandiyan wrote:

The causes of clock recovery and channel equalization failures are not explicitly printed in debug messages. Help debugging link training failures by printing why it failed.

Doing this in the driver would mean re-implementing some of the drm static functions that decode link status. Let's avoid that with these debug messages in drm.

Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

drivers/gpu/drm/drm_dp_helper.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 091053e..d763b57 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -64,12 +64,16 @@ bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],

lane_align = dp_link_status(link_status, DP_LANE_ALIGN_STATUS_UPDATED);

• if ((lane_align & DP_INTERLANE_ALIGN_DONE) == 0)

• if ((lane_align & DP_INTERLANE_ALIGN_DONE) == 0) {

• DRM_DEBUG_KMS("Inter-lane alignment not done\n");
  

  return false;
• } for (lane = 0; lane < lane_count; lane++) { lane_status = dp_get_lane_status(link_status, lane);

• if ((lane_status & DP_CHANNEL_EQ_BITS) != DP_CHANNEL_EQ_BITS)
  

• if ((lane_status & DP_CHANNEL_EQ_BITS) != DP_CHANNEL_EQ_BITS) {
  

• 	DRM_DEBUG_KMS("Channel equalization not done for lane %d\n", lane);
  return false;
  

• }
  

  } return true;

} @@ -83,8 +87,10 @@ bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE],

for (lane = 0; lane < lane_count; lane++) { lane_status = dp_get_lane_status(link_status, lane);

• if ((lane_status & DP_LANE_CR_DONE) == 0)
  

• if ((lane_status & DP_LANE_CR_DONE) == 0) {
  

• 	DRM_DEBUG_KMS("Clock recovery not done for lane %d\n", lane);
  

Please petition, with patch, for DRM_DEBUG_DP. -Chris

On Thu, Aug 04, 2016 at 04:50:35PM +0000, Pandiyan, Dhinakaran wrote:

On Thu, 2016-08-04 at 04:12 +0100, Chris Wilson wrote:

...

On Wed, Aug 03, 2016 at 08:07:40PM -0700, Dhinakaran Pandiyan wrote:

...

The causes of clock recovery and channel equalization failures are not explicitly printed in debug messages. Help debugging link training failures by printing why it failed.

Doing this in the driver would mean re-implementing some of the drm static functions that decode link status. Let's avoid that with these debug messages in drm.

Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

drivers/gpu/drm/drm_dp_helper.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 091053e..d763b57 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -64,12 +64,16 @@ bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE],

lane_align = dp_link_status(link_status, DP_LANE_ALIGN_STATUS_UPDATED);

• if ((lane_align & DP_INTERLANE_ALIGN_DONE) == 0)

• if ((lane_align & DP_INTERLANE_ALIGN_DONE) == 0) {

• DRM_DEBUG_KMS("Inter-lane alignment not done\n");
  

  return false;
• } for (lane = 0; lane < lane_count; lane++) { lane_status = dp_get_lane_status(link_status, lane);

• if ((lane_status & DP_CHANNEL_EQ_BITS) != DP_CHANNEL_EQ_BITS)
  

• if ((lane_status & DP_CHANNEL_EQ_BITS) != DP_CHANNEL_EQ_BITS) {
  

• 	DRM_DEBUG_KMS("Channel equalization not done for lane %d\n", lane);
  return false;
  

• }
  

  } return true;

} @@ -83,8 +87,10 @@ bool drm_dp_clock_recovery_ok(const u8 link_status[DP_LINK_STATUS_SIZE],

for (lane = 0; lane < lane_count; lane++) { lane_status = dp_get_lane_status(link_status, lane);

• if ((lane_status & DP_LANE_CR_DONE) == 0)
  

• if ((lane_status & DP_LANE_CR_DONE) == 0) {
  

• 	DRM_DEBUG_KMS("Clock recovery not done for lane %d\n", lane);
  

Please petition, with patch, for DRM_DEBUG_DP.

Is that because DRM_DEBUG_KMS in not appropriate or that we have plenty of DP related debug messages that it warrants it's own debug macro?

In the case of link failure, we will have plenty of messages from DP. One debug category just for DP may be overkill, and we may want something more like DRM_DEBUG_KMS_LOWLEVEL (or DRM_DEBUG_KMS_DRIVER, or DRM_DEBUG_KMS_HW etc) that suits all similar link training without cluttering up either the high levels telling what the user selected, and what the driver picked and the control flow through the modesetting. -Chris

On Thu, 04 Aug 2016, Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com wrote:

A full dump of link status can be handy in debugging link training failures. Let's add that to the debug messages when link training fails.

Signed-off-by: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com

drivers/gpu/drm/i915/intel_dp_link_training.c | 11 +++++++++++ drivers/gpu/drm/i915/intel_drv.h | 6 ++++-- 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c index c0a858d..ab7d1a6 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -24,6 +24,15 @@ #include "intel_drv.h"

static void +intel_dp_dump_link_status(const uint8_t link_status[DP_LINK_STATUS_SIZE]) +{

• DRM_DEBUG_KMS("ln0_1:0x%x ln2_3:0x%x align:0x%x sink:0x%x adj_req0_1:0x%x adj_req2_3:0x%x",

•       link_status[0], link_status[1], link_status[2],
  

•       link_status[3], link_status[4], link_status[5]);
  

+}

+static void intel_get_adjust_train(struct intel_dp *intel_dp, const uint8_t link_status[DP_LINK_STATUS_SIZE]) { @@ -170,6 +179,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp) ++loop_tries; if (loop_tries == 5) { DRM_ERROR("too many full retries, give up\n");

• 		intel_dp_dump_link_status(link_status);
  	break;
  }
  intel_dp_reset_link_train(intel_dp,
  

@@ -257,6 +267,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)

if (cr_tries > 5) {
	DRM_ERROR("failed to train DP, aborting\n");

• 	intel_dp_dump_link_status(link_status);
  break;
  

  }

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 87069ba..549a8fd 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1356,8 +1356,6 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, struct intel_connector *intel_connector); void intel_dp_set_link_params(struct intel_dp *intel_dp, const struct intel_crtc_state *pipe_config); -void intel_dp_start_link_train(struct intel_dp *intel_dp); -void intel_dp_stop_link_train(struct intel_dp *intel_dp); void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); void intel_dp_encoder_reset(struct drm_encoder *encoder); void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder); @@ -1409,6 +1407,10 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count) return ~((1 << lane_count) - 1) & 0xf; }

+/* intel_dp_link_training.c */ +void intel_dp_start_link_train(struct intel_dp *intel_dp); +void intel_dp_stop_link_train(struct intel_dp *intel_dp);

Unrelated cleanup change. Should be a (standalone) separate patch, and if it were, it could have been merged already.

Pro-tip: In general, you'll want to organize your series in a way that allows the early patches to be merged even when the review rounds are still in progress on the later patches. Crucial fixes first (so they can be backported without conflicts), trivial and non-controversial things next, and so on. You'll have a feeling of making progress, you'll have fewer rebases and conflicts later on, and it'll be easier for the reviewers too as the number of patches in later versions shrinks.

BR, Jani.

/* intel_dp_aux_backlight.c */ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);