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

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

[Todd Previte]

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 p
ossible 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.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c |  8 ++++++--
 drivers/gpu/drm/i915/intel_dp.c      | 10 ++++------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 3 files changed, 11 insertions(+), 9 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..ada8b07 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2705,9 +2705,8 @@ 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);
+	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 +2804,8 @@ 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);
+	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

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

[Jani Nikula]

On Thu, 09 Oct 2014, Todd Previte <tprevite@gmail.com> 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 p
> ossible 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.

There's been some back and forth about the mode set sequence ordering in
the past. There's also some disagreement between some specs about
this. The published display spec at 01.org says,

• Enable ports
• Wait for DPIO phystatus ready in 6014
• Enable pipe A/B
• Enable planes (VGA or hires)

This is in line with the current implementation. However, there's no
details about the link training - perhaps you could try waiting for DPIO
phystatus ready right after intel_dp_enable_port() but before link
training?

BR,
Jani.


PS. Please wrap the commit messages at, say, 72 characters. Below 80
anyway.


>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++++--
>  drivers/gpu/drm/i915/intel_dp.c      | 10 ++++------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  3 files changed, 11 insertions(+), 9 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..ada8b07 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2705,9 +2705,8 @@ 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);
> +	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 +2804,8 @@ 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);
> +	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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Todd Previte]

On 10/10/14 1:04 AM, Jani Nikula wrote:
> On Thu, 09 Oct 2014, Todd Previte<tprevite@gmail.com>  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 p
>> ossible 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.
> There's been some back and forth about the mode set sequence ordering in
> the past. There's also some disagreement between some specs about
> this. The published display spec at 01.org says,
>
> • Enable ports
> • Wait for DPIO phystatus ready in 6014
> • Enable pipe A/B
> • Enable planes (VGA or hires)
>
> This is in line with the current implementation. However, there's no
> details about the link training - perhaps you could try waiting for DPIO
> phystatus ready right after intel_dp_enable_port() but before link
> training?
>
> BR,
> Jani.
>
>
> PS. Please wrap the commit messages at, say, 72 characters. Below 80
> anyway.
Thanks for the feedback, Jani. That might be a good option. That 
essentially moves the wait_port_ready into the intel_enable_dp() 
function, so it would need a platform check there as well. Not a big 
deal, really. Alternatively,  it looks like intel_dp_enable_port() can 
be moved out of intel_enable_dp() and placed right before the call to 
vlv_wait_port_ready(). There's only a few places where intel_enable_dp() 
is called, it shouldn't be too much of an issue to pull 
intel_dp_enable_port() out and place it where necessary. I'll post the 
updated patch as soon as it's ready.

I'll amend the commit message and wrap it under 80 chars, too.

-T

>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c |  8 ++++++--
>>   drivers/gpu/drm/i915/intel_dp.c      | 10 ++++------
>>   drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>   3 files changed, 11 insertions(+), 9 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..ada8b07 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -2705,9 +2705,8 @@ 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);
>> +	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 +2804,8 @@ 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);
>> +	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

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

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

[Todd Previte]

V2 changes:
- Moved the intel_dp_enable_port() call out of intel_dp_enable() and placed it 
  before the calls to intel_dp_enable() and vlv_wait_port_ready()
- Cleaned up a spacing issues with the code indents
- Amended the commit message to be under 80 characters per line and expanded
  on the description of what the patch does

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

[Todd Previte]

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().

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

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

[Daniel Vetter]

On Fri, Oct 17, 2014 at 11:41:12AM -0700, Todd Previte wrote:
> V2 changes:
> - Moved the intel_dp_enable_port() call out of intel_dp_enable() and placed it 
>   before the calls to intel_dp_enable() and vlv_wait_port_ready()
> - Cleaned up a spacing issues with the code indents
> - Amended the commit message to be under 80 characters per line and expanded
>   on the description of what the patch does

The per-patch commit log should be part of the commit message, above the
sob section. Some kernel maintainers want it below claiming it's noise,
but I disagree. In any case it needs to be part of the patch when
submitting it.

The cover letter changelog is just for the big stuff spawning more than
one patch when you have a big series.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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ä]

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

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

[Todd Previte]

On 10/21/2014 7:41 AM, Daniel Vetter wrote:
> On Fri, Oct 17, 2014 at 11:41:12AM -0700, Todd Previte wrote:
>> V2 changes:-+
>> - Moved the intel_dp_enable_port() call out of intel_dp_enable() and placed it
>>    before the calls to intel_dp_enable() and vlv_wait_port_ready()
>> - Cleaned up a spacing issues with the code indents
>> - Amended the commit message to be under 80 characters per line and expanded
>>    on the description of what the patch does
> The per-patch commit log should be part of the commit message, above the
> sob section. Some kernel maintainers want it below claiming it's noise,
> but I disagree. In any case it needs to be part of the patch when
> submitting it.
>
> The cover letter changelog is just for the big stuff spawning more than
> one patch when you have a big series.
> -Daniel

Ville's patch (patch 07/17) in the CHV PPS fix sequence is going to pick 
up most of the necessary changes that are included here. The one aspect 
that is not covered is that link training needs to be skipped when the 
PHYs are down. If that change is integrated into his patch, this patch 
is not necessary. Otherwise, this patch will need to be updated to 
accommodate that change.

-T