Re: [PATCH 2/3] drm/i915: Change order of operations for VLV/CHV to not train DP link before PHYs are ready

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Oct 17, 2014 at 11:41:13AM -0700, Todd Previte wrote:
> Reorder the function calls in chv/vlv_pre_enable_dp() such that link training
> is not initiated before the PHYs come up out of reset. Also check the status
> of vlv_wait_port_ready() and only attempt to train if the PHYs are actually
> running.
> 
> The specification lists the wait for the PHYs as one of the final steps in
> enabling the Displayport hardware for use.  While the PHYs are in reset, no
> communication is possible across the link. Attempting to train the link while
> the PHYs are in reset will result in link training failure with one or more
> WARN() in the logs. Moving the intel_enable_dp() function after
> vlv_wait_port_ready() and only when the PHYs are ready helps ensure reliable
> operation of the Displayport link.
> 
> To comply with the specification, the call to enable_port() has been moved of
> enable_dp() and placed before the wait functions for the PHYs and prior to
> the call to enable_dp().

This is going to conflict with my PPS series. I have a similar patch in
there, except it doesn't skip the link training. I'm not sure we should
bother doing that since the wait_port_ready() problem should never ever
happen as long as we do things correctly, which we should do after my
series lands.

> 
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++++--
>  drivers/gpu/drm/i915/intel_dp.c      | 16 ++++++++--------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  3 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c51d950..4b280c1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1723,7 +1723,7 @@ static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
>  	mutex_unlock(&dev_priv->dpio_lock);
>  }
>  
> -void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> +int vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>  		struct intel_digital_port *dport)
>  {
>  	u32 port_mask;
> @@ -1746,9 +1746,13 @@ void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>  		BUG();
>  	}
>  
> -	if (wait_for((I915_READ(dpll_reg) & port_mask) == 0, 1000))
> +	if (wait_for((I915_READ(dpll_reg) & port_mask) == 0, 1000)) {
>  		WARN(1, "timed out waiting for port %c ready: 0x%08x\n",
>  		     port_name(dport->port), I915_READ(dpll_reg));
> +		return -EIO;
> +	}
> +
> +	return 0;
>  }
>  
>  static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a8352c4..c1ce738 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2532,7 +2532,7 @@ static void intel_dp_enable_port(struct intel_dp *intel_dp)
>  	POSTING_READ(intel_dp->output_reg);
>  }
>  
> -static void intel_enable_dp(struct intel_encoder *encoder)
> +static bool intel_enable_dp(struct intel_encoder *encoder)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>  	struct drm_device *dev = encoder->base.dev;
> @@ -2544,7 +2544,6 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>  	if (WARN_ON(dp_reg & DP_PORT_EN))
>  		return false;
>  
> -	intel_dp_enable_port(intel_dp);
>  	intel_edp_panel_vdd_on(intel_dp);
>  	intel_edp_panel_on(intel_dp);
>  	intel_edp_panel_vdd_off(intel_dp, true);
> @@ -2576,6 +2575,7 @@ static void g4x_enable_dp(struct intel_encoder *encoder)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>  
> +	intel_dp_enable_port(intel_dp);
>  	intel_enable_dp(encoder);
>  	intel_edp_backlight_on(intel_dp);
>  }
> @@ -2705,9 +2705,9 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
>  		pps_unlock(intel_dp);
>  	}
>  
> -	intel_enable_dp(encoder);
> -
> -	vlv_wait_port_ready(dev_priv, dport);
> +	intel_dp_enable_port(intel_dp);
> +	if (vlv_wait_port_ready(dev_priv, dport) == 0)
> +		intel_enable_dp(encoder);
>  }
>  
>  static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
> @@ -2805,9 +2805,9 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder)
>  		pps_unlock(intel_dp);
>  	}
>  
> -	intel_enable_dp(encoder);
> -
> -	vlv_wait_port_ready(dev_priv, dport);
> +	intel_dp_enable_port(intel_dp);
> +	if (vlv_wait_port_ready(dev_priv, dport) == 0)
> +		intel_enable_dp(encoder);
>  }
>  
>  static void chv_dp_pre_pll_enable(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index dc80444..2ff2c8c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -876,7 +876,7 @@ intel_wait_for_vblank(struct drm_device *dev, int pipe)
>  	drm_wait_one_vblank(dev, pipe);
>  }
>  int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
> -void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> +int vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>  			 struct intel_digital_port *dport);
>  bool intel_get_load_detect_pipe(struct drm_connector *connector,
>  				struct drm_display_mode *mode,
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC