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ä
2014-10-22T07:17:27Z

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