Re: [Intel-gfx] [PATCH 1/4] drm/i915/dp: Add debug messages to print DP link training pattern

Re: [Intel-gfx] [PATCH 1/4] drm/i915/dp: Add debug messages to print DP link training pattern

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/4] Improve logging for DP link training

[Dhinakaran Pandiyan]

We do not currently output enough information to help debugging DP link
training issues. For e.g., training pattern and link status information.
This series aims to correct that by adding debug messages that can help
developers.

Dhinakaran Pandiyan (4):
  drm/i915/dp: Add debug messages to print DP link training pattern
  drm/i915/dp: Switch to using the DRM function for reading DP link
    status
  drm/dp: Clarify clock recovery and channel equalization failures
  drm/i915/dp: Dump DP link status when link training stages fails

 drivers/gpu/drm/drm_dp_helper.c               | 12 +++++++++---
 drivers/gpu/drm/i915/intel_dp.c               | 28 ++++++++++-----------------
 drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h              |  8 ++++----
 4 files changed, 42 insertions(+), 28 deletions(-)

-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] drm/i915/dp: Add debug messages to print DP link training pattern

[Dhinakaran Pandiyan]

Currently we do not print the training pattern used in any of the DP link
training stages. Including this piece of information in debug messages will
help debugging.

Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 21b04c3..3ca33bd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2547,6 +2547,10 @@ _intel_dp_set_link_train(struct intel_dp *intel_dp,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	enum port port = intel_dig_port->port;
 
+	if (dp_train_pat & DP_TRAINING_PATTERN_MASK)
+		DRM_DEBUG_KMS("Using DP training pattern TPS%d\n",
+			      dp_train_pat & DP_TRAINING_PATTERN_MASK);
+
 	if (HAS_DDI(dev)) {
 		uint32_t temp = I915_READ(DP_TP_CTL(port));
 
@@ -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;
 			}
 			break;
@@ -2629,11 +2633,8 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
 		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
 
 	/* enable with pattern 1 (as per spec) */
-	_intel_dp_set_link_train(intel_dp, &intel_dp->DP,
-				 DP_TRAINING_PATTERN_1);
 
-	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
-	POSTING_READ(intel_dp->output_reg);
+	intel_dp_program_link_training_pattern(intel_dp, DP_TRAINING_PATTERN_1);
 
 	/*
 	 * Magic for VLV/CHV. We _must_ first set up the register
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915/dp: Switch to using the DRM function for reading DP link status

[Dhinakaran Pandiyan]

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;
-}
-
 /* These are source-specific values. */
 uint8_t
 intel_dp_voltage_max(struct intel_dp *intel_dp)
@@ -3869,10 +3858,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	u8 link_status[DP_LINK_STATUS_SIZE];
+	int len;
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+	len = drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status);
+	if (len != DP_LINK_STATUS_SIZE) {
 		DRM_ERROR("Failed to get link status\n");
 		return;
 	}
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 60fb39c..c0a858d 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -107,7 +107,7 @@ intel_dp_update_link_train(struct intel_dp *intel_dp)
 static void
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
-	int i;
+	int i, len;
 	uint8_t voltage;
 	int voltage_tries, loop_tries;
 	uint8_t link_config[2];
@@ -150,7 +150,9 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		uint8_t link_status[DP_LINK_STATUS_SIZE];
 
 		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
-		if (!intel_dp_get_link_status(intel_dp, link_status)) {
+
+		len = drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status);
+		if (len != DP_LINK_STATUS_SIZE) {
 			DRM_ERROR("failed to get link status\n");
 			break;
 		}
@@ -235,6 +237,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 	bool channel_eq = false;
 	int tries, cr_tries;
 	u32 training_pattern;
+	int len;
 
 	training_pattern = intel_dp_training_pattern(intel_dp);
 
@@ -258,7 +261,9 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 		}
 
 		drm_dp_link_train_channel_eq_delay(intel_dp->dpcd);
-		if (!intel_dp_get_link_status(intel_dp, link_status)) {
+
+		len = drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status);
+		if (len != DP_LINK_STATUS_SIZE) {
 			DRM_ERROR("failed to get link status\n");
 			break;
 		}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e74d851..87069ba 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1403,8 +1403,6 @@ intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing);
 void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
 			   uint8_t *link_bw, uint8_t *rate_select);
 bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp);
-bool
-intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE]);
 
 static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 {
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/dp: Clarify clock recovery and channel equalization failures

[Dhinakaran Pandiyan]

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);
 			return false;
+		}
 	}
 	return true;
 }
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915/dp: Dump DP link status when link training stages fails

[Dhinakaran Pandiyan]

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);
+
 /* intel_dp_aux_backlight.c */
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
 
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915/dp: Switch to using the DRM function for reading DP link status

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/dp: Clarify clock recovery and channel equalization failures

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915/dp: Switch to using the DRM function for reading DP link status

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for Improve logging for DP link training

[Patchwork]

== Series Details ==

Series: Improve logging for DP link training
URL   : https://patchwork.freedesktop.org/series/10637/
State : failure

== Summary ==

Series 10637v1 Improve logging for DP link training
http://patchwork.freedesktop.org/api/1.0/series/10637/revisions/1/mbox

Test gem_exec_gttfill:
        Subgroup basic:
                skip       -> PASS       (fi-snb-i7-2600)
Test kms_cursor_legacy:
        Subgroup basic-flip-vs-cursor-legacy:
                fail       -> PASS       (ro-hsw-i7-4770r)
                fail       -> PASS       (ro-bdw-i7-5600u)
                pass       -> FAIL       (ro-bdw-i5-5250u)
                fail       -> DMESG-FAIL (ro-skl3-i5-6260u)
        Subgroup basic-flip-vs-cursor-varying-size:
                pass       -> FAIL       (ro-bdw-i5-5250u)
                pass       -> FAIL       (fi-skl-i7-6700k)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (ro-byt-n2820)

fi-hsw-i7-4770k  total:240  pass:218  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-qkkr      total:240  pass:182  dwarn:29  dfail:0   fail:3   skip:26 
fi-skl-i5-6260u  total:240  pass:224  dwarn:0   dfail:0   fail:2   skip:14 
fi-skl-i7-6700k  total:240  pass:208  dwarn:0   dfail:0   fail:4   skip:28 
fi-snb-i7-2600   total:240  pass:198  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:218  dwarn:4   dfail:0   fail:2   skip:16 
ro-bdw-i7-5557U  total:240  pass:224  dwarn:0   dfail:0   fail:0   skip:16 
ro-bdw-i7-5600u  total:240  pass:207  dwarn:0   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:195  dwarn:0   dfail:0   fail:3   skip:42 
ro-byt-n2820     total:240  pass:196  dwarn:0   dfail:0   fail:4   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-ilk-i7-620lm  total:240  pass:173  dwarn:1   dfail:0   fail:1   skip:65 
ro-ilk1-i5-650   total:235  pass:173  dwarn:0   dfail:0   fail:2   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:1   fail:3   skip:14 
ro-snb-i7-2620M  total:240  pass:198  dwarn:0   dfail:0   fail:1   skip:41 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1695/

4d6f224 drm-intel-nightly: 2016y-08m-03d-15h-38m-31s UTC integration manifest
a3ead7b drm/i915/dp: Dump DP link status when link training stages fails
d75ca85 drm/dp: Clarify clock recovery and channel equalization failures
ac56b3b drm/i915/dp: Switch to using the DRM function for reading DP link status
13b4410 drm/i915/dp: Add debug messages to print DP link training pattern

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/i915/dp: Dump DP link status when link training stages fails

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/i915/dp: Dump DP link status when link training stages fails

[Pandiyan, Dhinakaran]

On Thu, 2016-08-04 at 10:46 +0300, Jani Nikula wrote:
> 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.
> 

Thanks for the tip Jani, will keep that in mind. I will split this patch
when I send the next version of the series.
> > +
> >  /* intel_dp_aux_backlight.c */
> >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/dp: Clarify clock recovery and channel equalization failures

[Pandiyan, Dhinakaran]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915/dp: Add debug messages to print DP link training pattern

[Pandiyan, Dhinakaran]

On Thu, 2016-08-04 at 04:07 +0100, Chris Wilson wrote:
> 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
> 
I will make that change. Thanks for the review.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/4] drm/dp: Clarify clock recovery and channel equalization failures

[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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/i915/dp: Switch to using the DRM function for reading DP link status

[Dhinakaran Pandiyan]

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

drm_dp_dpcd_read_link_status() returns a negative error code if the number
of bytes read is not DP_LINK_STATUS_SIZE, drm_dp_dpcd_access() does the
length check.

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

v2: Eliminated redundant DP_LINK_STATUS_SIZE length check.
---
 drivers/gpu/drm/i915/intel_dp.c               | 15 ++-------------
 drivers/gpu/drm/i915/intel_dp_link_training.c |  8 ++++++--
 drivers/gpu/drm/i915/intel_drv.h              |  2 --
 3 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8fe2afa..8eff57e 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;
-}
-
 /* These are source-specific values. */
 uint8_t
 intel_dp_voltage_max(struct intel_dp *intel_dp)
@@ -3896,8 +3885,8 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!intel_dp_get_link_status(intel_dp, link_status)) {
-		DRM_ERROR("Failed to get link status\n");
+	if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) <= 0) {
+		DRM_ERROR("failed to get link status\n");
 		return;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 60fb39c..8e60e7c 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -150,7 +150,9 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		uint8_t link_status[DP_LINK_STATUS_SIZE];
 
 		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
-		if (!intel_dp_get_link_status(intel_dp, link_status)) {
+
+		if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status)
+						 <= 0) {
 			DRM_ERROR("failed to get link status\n");
 			break;
 		}
@@ -258,7 +260,9 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 		}
 
 		drm_dp_link_train_channel_eq_delay(intel_dp->dpcd);
-		if (!intel_dp_get_link_status(intel_dp, link_status)) {
+
+		if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status)
+						 <= 0) {
 			DRM_ERROR("failed to get link status\n");
 			break;
 		}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b1fc67e..a3a2cb9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1394,8 +1394,6 @@ intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing);
 void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
 			   uint8_t *link_bw, uint8_t *rate_select);
 bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp);
-bool
-intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE]);
 
 static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 {
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/dp: Switch to using the DRM function for reading DP link status

[Pandiyan, Dhinakaran]

Please ignore this, I am resubmitting this as an independent patch.
-DK

On Thu, 2016-08-11 at 13:49 -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.
> 
> drm_dp_dpcd_read_link_status() returns a negative error code if the number
> of bytes read is not DP_LINK_STATUS_SIZE, drm_dp_dpcd_access() does the
> length check.
> 
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> v2: Eliminated redundant DP_LINK_STATUS_SIZE length check.
> ---
>  drivers/gpu/drm/i915/intel_dp.c               | 15 ++-------------
>  drivers/gpu/drm/i915/intel_dp_link_training.c |  8 ++++++--
>  drivers/gpu/drm/i915/intel_drv.h              |  2 --
>  3 files changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8fe2afa..8eff57e 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;
> -}
> -
>  /* These are source-specific values. */
>  uint8_t
>  intel_dp_voltage_max(struct intel_dp *intel_dp)
> @@ -3896,8 +3885,8 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
> -	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> -		DRM_ERROR("Failed to get link status\n");
> +	if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) <= 0) {
> +		DRM_ERROR("failed to get link status\n");
>  		return;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index 60fb39c..8e60e7c 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -150,7 +150,9 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>  		uint8_t link_status[DP_LINK_STATUS_SIZE];
>  
>  		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
> -		if (!intel_dp_get_link_status(intel_dp, link_status)) {
> +
> +		if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status)
> +						 <= 0) {
>  			DRM_ERROR("failed to get link status\n");
>  			break;
>  		}
> @@ -258,7 +260,9 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>  		}
>  
>  		drm_dp_link_train_channel_eq_delay(intel_dp->dpcd);
> -		if (!intel_dp_get_link_status(intel_dp, link_status)) {
> +
> +		if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status)
> +						 <= 0) {
>  			DRM_ERROR("failed to get link status\n");
>  			break;
>  		}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b1fc67e..a3a2cb9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1394,8 +1394,6 @@ intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing);
>  void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
>  			   uint8_t *link_bw, uint8_t *rate_select);
>  bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp);
> -bool
> -intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE]);
>  
>  static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
>  {

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx