diff mbox

[5/5] drm/i915: Track whether the DP link is trained or not

Message ID 20180117192149.17760-5-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä Jan. 17, 2018, 7:21 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

LSPCON likes to throw short HPDs during the enable seqeunce prior to the
link being trained. These obviously result in the channel CR/EQ check
failing and thus we schedule a pointless hotplug work to retrain the
link. Avoid that by ignoring the bad CR/EQ status until we've actually
initially trained the link.

I've not actually investigated to see what LSPCON is trying to signal
with the short pulse. But as long as it signals anything I think we're
supposed to check the link status anyway, so I don't really see other
good ways to solve this. I've not seen these short pulses being
generated by normal DP sinks.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c              |  2 ++
 drivers/gpu/drm/i915/intel_dp.c               | 10 +++++++---
 drivers/gpu/drm/i915/intel_dp_link_training.c |  2 ++
 drivers/gpu/drm/i915/intel_drv.h              |  1 +
 4 files changed, 12 insertions(+), 3 deletions(-)

Comments

Lyude Paul Jan. 30, 2018, 11:19 p.m. UTC | #1 
Reviewed-by: Lyude Paul <lyude@redhat.com>

On Wed, 2018-01-17 at 21:21 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> LSPCON likes to throw short HPDs during the enable seqeunce prior to the
> link being trained. These obviously result in the channel CR/EQ check
> failing and thus we schedule a pointless hotplug work to retrain the
> link. Avoid that by ignoring the bad CR/EQ status until we've actually
> initially trained the link.
> 
> I've not actually investigated to see what LSPCON is trying to signal
> with the short pulse. But as long as it signals anything I think we're
> supposed to check the link status anyway, so I don't really see other
> good ways to solve this. I've not seen these short pulses being
> generated by normal DP sinks.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c              |  2 ++
>  drivers/gpu/drm/i915/intel_dp.c               | 10 +++++++---
>  drivers/gpu/drm/i915/intel_dp_link_training.c |  2 ++
>  drivers/gpu/drm/i915/intel_drv.h              |  1 +
>  4 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 5f3d58f1ae6e..7a4c5a2d36ed 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2466,6 +2466,8 @@ static void intel_disable_ddi_dp(struct intel_encoder
> *encoder,
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>  
> +	intel_dp->link_trained = false;
> +
>  	if (old_crtc_state->has_audio)
>  		intel_audio_codec_disable(encoder,
>  					  old_crtc_state, old_conn_state);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 152016e09a11..0cf92aa60f3e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1854,6 +1854,7 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp,
>  			      int link_rate, uint8_t lane_count,
>  			      bool link_mst)
>  {
> +	intel_dp->link_trained = false;
>  	intel_dp->link_rate = link_rate;
>  	intel_dp->lane_count = lane_count;
>  	intel_dp->link_mst = link_mst;
> @@ -2702,6 +2703,8 @@ static void intel_disable_dp(struct intel_encoder
> *encoder,
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>  
> +	intel_dp->link_trained = false;
> +
>  	if (old_crtc_state->has_audio)
>  		intel_audio_codec_disable(encoder,
>  					  old_crtc_state, old_conn_state);
> @@ -4280,10 +4283,11 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
>  {
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
> -	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> -		DRM_ERROR("Failed to get link status\n");
> +	if (!intel_dp->link_trained)
> +		return false;
> +
> +	if (!intel_dp_get_link_status(intel_dp, link_status))
>  		return false;
> -	}
>  
>  	/*
>  	 * Validate the cached values of intel_dp->link_rate and
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index 1314f5d87d7d..78f1fe934da3 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -307,6 +307,8 @@ intel_dp_link_training_channel_equalization(struct
> intel_dp *intel_dp)
>  
>  void intel_dp_stop_link_train(struct intel_dp *intel_dp)
>  {
> +	intel_dp->link_trained = true;
> +
>  	intel_dp_set_link_train(intel_dp,
>  				DP_TRAINING_PATTERN_DISABLE);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 1d018869ad02..7a45ffb9e524 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1043,6 +1043,7 @@ struct intel_dp {
>  	uint8_t lane_count;
>  	uint8_t sink_count;
>  	bool link_mst;
> +	bool link_trained;
>  	bool has_audio;
>  	bool detect_done;
>  	bool reset_link_params;
Navare, Manasi Feb. 28, 2018, 8:08 p.m. UTC | #2 
On Wed, Jan 17, 2018 at 09:21:49PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> LSPCON likes to throw short HPDs during the enable seqeunce prior to the
> link being trained. These obviously result in the channel CR/EQ check
> failing and thus we schedule a pointless hotplug work to retrain the
> link. Avoid that by ignoring the bad CR/EQ status until we've actually
> initially trained the link.
> 
> I've not actually investigated to see what LSPCON is trying to signal
> with the short pulse. But as long as it signals anything I think we're
> supposed to check the link status anyway, so I don't really see other
> good ways to solve this. I've not seen these short pulses being
> generated by normal DP sinks.
>

I agree with avoiding the retraining of the link through these HPDs
when its not trained even for the first time.
The only concern I have here is that we probably shouldnt set the link_trained to true
unless it has been sucessfully trained. So move it to the intel_dp_start_link_train
before failure handling. This will also avoid the case where we are in the retraining
and we get these short HPDs.
Also the link_trained should be set to false in failure_handling in intel_dp_start_link_train()
before scheduling the retry modeset work function.
Thoughts?
 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c              |  2 ++
>  drivers/gpu/drm/i915/intel_dp.c               | 10 +++++++---
>  drivers/gpu/drm/i915/intel_dp_link_training.c |  2 ++
>  drivers/gpu/drm/i915/intel_drv.h              |  1 +
>  4 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 5f3d58f1ae6e..7a4c5a2d36ed 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2466,6 +2466,8 @@ static void intel_disable_ddi_dp(struct intel_encoder *encoder,
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>  
> +	intel_dp->link_trained = false;
> +
>  	if (old_crtc_state->has_audio)
>  		intel_audio_codec_disable(encoder,
>  					  old_crtc_state, old_conn_state);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 152016e09a11..0cf92aa60f3e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1854,6 +1854,7 @@ void intel_dp_set_link_params(struct intel_dp *intel_dp,
>  			      int link_rate, uint8_t lane_count,
>  			      bool link_mst)
>  {
> +	intel_dp->link_trained = false;
>  	intel_dp->link_rate = link_rate;
>  	intel_dp->lane_count = lane_count;
>  	intel_dp->link_mst = link_mst;
> @@ -2702,6 +2703,8 @@ static void intel_disable_dp(struct intel_encoder *encoder,
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>  
> +	intel_dp->link_trained = false;
> +
>  	if (old_crtc_state->has_audio)
>  		intel_audio_codec_disable(encoder,
>  					  old_crtc_state, old_conn_state);
> @@ -4280,10 +4283,11 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
>  {
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
> -	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> -		DRM_ERROR("Failed to get link status\n");
> +	if (!intel_dp->link_trained)
> +		return false;
> +
> +	if (!intel_dp_get_link_status(intel_dp, link_status))
>  		return false;
> -	}
>  
>  	/*
>  	 * Validate the cached values of intel_dp->link_rate and
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index 1314f5d87d7d..78f1fe934da3 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -307,6 +307,8 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>  
>  void intel_dp_stop_link_train(struct intel_dp *intel_dp)
>  {
> +	intel_dp->link_trained = true;
> +

Move to intel_dp_start_link_train() on successful link train?

Manasi

>  	intel_dp_set_link_train(intel_dp,
>  				DP_TRAINING_PATTERN_DISABLE);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1d018869ad02..7a45ffb9e524 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1043,6 +1043,7 @@ struct intel_dp {
>  	uint8_t lane_count;
>  	uint8_t sink_count;
>  	bool link_mst;
> +	bool link_trained;
>  	bool has_audio;
>  	bool detect_done;
>  	bool reset_link_params;
> -- 
> 2.13.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 5f3d58f1ae6e..7a4c5a2d36ed 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2466,6 +2466,8 @@  static void intel_disable_ddi_dp(struct intel_encoder *encoder,
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 
+	intel_dp->link_trained = false;
+
 	if (old_crtc_state->has_audio)
 		intel_audio_codec_disable(encoder,
 					  old_crtc_state, old_conn_state);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 152016e09a11..0cf92aa60f3e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1854,6 +1854,7 @@  void intel_dp_set_link_params(struct intel_dp *intel_dp,
 			      int link_rate, uint8_t lane_count,
 			      bool link_mst)
 {
+	intel_dp->link_trained = false;
 	intel_dp->link_rate = link_rate;
 	intel_dp->lane_count = lane_count;
 	intel_dp->link_mst = link_mst;
@@ -2702,6 +2703,8 @@  static void intel_disable_dp(struct intel_encoder *encoder,
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 
+	intel_dp->link_trained = false;
+
 	if (old_crtc_state->has_audio)
 		intel_audio_codec_disable(encoder,
 					  old_crtc_state, old_conn_state);
@@ -4280,10 +4283,11 @@  intel_dp_needs_link_retrain(struct intel_dp *intel_dp)
 {
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
-	if (!intel_dp_get_link_status(intel_dp, link_status)) {
-		DRM_ERROR("Failed to get link status\n");
+	if (!intel_dp->link_trained)
+		return false;
+
+	if (!intel_dp_get_link_status(intel_dp, link_status))
 		return false;
-	}
 
 	/*
 	 * Validate the cached values of intel_dp->link_rate and
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 1314f5d87d7d..78f1fe934da3 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -307,6 +307,8 @@  intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 
 void intel_dp_stop_link_train(struct intel_dp *intel_dp)
 {
+	intel_dp->link_trained = true;
+
 	intel_dp_set_link_train(intel_dp,
 				DP_TRAINING_PATTERN_DISABLE);
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1d018869ad02..7a45ffb9e524 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1043,6 +1043,7 @@  struct intel_dp {
 	uint8_t lane_count;
 	uint8_t sink_count;
 	bool link_mst;
+	bool link_trained;
 	bool has_audio;
 	bool detect_done;
 	bool reset_link_params;