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

[Manasi Navare <manasi.d.navare@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx