[PATCH 1/3] drm/i915: fix hsw_fdi_link_train "retry" code

[PATCH 1/3] drm/i915: fix hsw_fdi_link_train "retry" code

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We were previously doing exactly what the "mode set sequence for CRT"
document mandates, but whenever we failed to train the link in the
first tentative, all the other subsequent retries always failed. In
one of my monitors that has 47 modes, I was usually getting around 3
failures when running "testdisplay -a".

After this patch, even if we fail in the first tentative, we can
succeed in the next ones. So now when running "testdisplay -a" I see
around 3 times the message "FDI link training done on step 1" and no
failures.

Notice that now the "retry" code looks a lot like the DP retry code.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 43 +++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 16 deletions(-)

I'd like this to go to 3.8 somehow.

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 852012b..3264cb4 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -138,6 +138,19 @@ static const long hsw_ddi_buf_ctl_values[] = {
 	DDI_BUF_EMP_800MV_3_5DB_HSW
 };
 
+static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
+				    enum port port)
+{
+	uint32_t reg = DDI_BUF_CTL(port);
+	int i;
+
+	for (i = 0; i < 8; i++) {
+		udelay(1);
+		if (I915_READ(reg) & DDI_BUF_IS_IDLE)
+			return;
+	}
+	DRM_ERROR("Timeout waiting for DDI BUF %c idle bit\n", port_name(port));
+}
 
 /* Starting with Haswell, different DDI ports can work in FDI mode for
  * connection to the PCH-located connectors. For this, it is necessary to train
@@ -231,18 +244,30 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
 			return;
 		}
 
+		temp = I915_READ(DDI_BUF_CTL(PORT_E));
+		temp &= ~DDI_BUF_CTL_ENABLE;
+		I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
+		POSTING_READ(DDI_BUF_CTL(PORT_E));
+
 		/* Disable DP_TP_CTL and FDI_RX_CTL and retry */
-		I915_WRITE(DP_TP_CTL(PORT_E),
-			   I915_READ(DP_TP_CTL(PORT_E)) & ~DP_TP_CTL_ENABLE);
+		temp = I915_READ(DP_TP_CTL(PORT_E));
+		temp &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
+		temp |= DP_TP_CTL_LINK_TRAIN_PAT1;
+		I915_WRITE(DP_TP_CTL(PORT_E), temp);
+		POSTING_READ(DP_TP_CTL(PORT_E));
+
+		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
 
 		rx_ctl_val &= ~FDI_RX_ENABLE;
 		I915_WRITE(_FDI_RXA_CTL, rx_ctl_val);
+		POSTING_READ(_FDI_RXA_CTL);
 
 		/* Reset FDI_RX_MISC pwrdn lanes */
 		temp = I915_READ(_FDI_RXA_MISC);
 		temp &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
 		temp |= FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2);
 		I915_WRITE(_FDI_RXA_MISC, temp);
+		POSTING_READ(_FDI_RXA_MISC);
 	}
 
 	DRM_ERROR("FDI link training failed!\n");
@@ -1222,20 +1247,6 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
 	}
 }
 
-static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
-				    enum port port)
-{
-	uint32_t reg = DDI_BUF_CTL(port);
-	int i;
-
-	for (i = 0; i < 8; i++) {
-		udelay(1);
-		if (I915_READ(reg) & DDI_BUF_IS_IDLE)
-			return;
-	}
-	DRM_ERROR("Timeout waiting for DDI BUF %c idle bit\n", port_name(port));
-}
-
 static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
 {
 	struct drm_encoder *encoder = &intel_encoder->base;
-- 
1.7.11.7

[PATCH 2/3] drm/i915: reject modes the LPT FDI receiver can't handle

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

More specifically, the LPT FDI RX only supports 8bpc and a maximum of
2 lanes, so anything above that won't work and should be rejected.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c     |  5 +++++
 drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++---------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 3 files changed, 20 insertions(+), 9 deletions(-)

This one could go to 3.8 too.

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 5c77743..3084d01 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -198,6 +198,11 @@ static int intel_crt_mode_valid(struct drm_connector *connector,
 	if (mode->clock > max_clock)
 		return MODE_CLOCK_HIGH;
 
+	/* The FDI receiver on LPT only supports 8bpc and only has 2 lanes. */
+	if (HAS_PCH_LPT(dev) &&
+	    (ironlake_get_lanes_required(mode->clock, 270000, 24) > 2))
+		return MODE_CLOCK_HIGH;
+
 	return MODE_OK;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 78d12c4..8d86a39 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5228,6 +5228,17 @@ static bool ironlake_check_fdi_lanes(struct intel_crtc *intel_crtc)
 	}
 }
 
+int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp)
+{
+	/*
+	 * Account for spread spectrum to avoid
+	 * oversubscribing the link. Max center spread
+	 * is 2.5%; use 5% for safety's sake.
+	 */
+	u32 bps = target_clock * bpp * 21 / 20;
+	return bps / (link_bw * 8) + 1;
+}
+
 static void ironlake_set_m_n(struct drm_crtc *crtc,
 			     struct drm_display_mode *mode,
 			     struct drm_display_mode *adjusted_mode)
@@ -5281,15 +5292,9 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
 	else
 		target_clock = adjusted_mode->clock;
 
-	if (!lane) {
-		/*
-		 * Account for spread spectrum to avoid
-		 * oversubscribing the link. Max center spread
-		 * is 2.5%; use 5% for safety's sake.
-		 */
-		u32 bps = target_clock * intel_crtc->bpp * 21 / 20;
-		lane = bps / (link_bw * 8) + 1;
-	}
+	if (!lane)
+		lane = ironlake_get_lanes_required(target_clock, link_bw,
+						   intel_crtc->bpp);
 
 	intel_crtc->fdi_lanes = lane;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 522061c..a468a07 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -556,6 +556,7 @@ intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
 			     enum pipe pipe);
 extern void intel_wait_for_vblank(struct drm_device *dev, int pipe);
 extern void intel_wait_for_pipe_off(struct drm_device *dev, int pipe);
+extern int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
 
 struct intel_load_detect_pipe {
 	struct drm_framebuffer *release_fb;
-- 
1.7.11.7

[PATCH 3/3] drm/i915: fix FDI lane calculation

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The previous code was making the bps value 5% higher than what the
spec says, which was enough to make certain VGA modes require 3 lanes
instead of 2, which makes us reject these modes on Haswell since it
only has 2 FDI lanes. For previous gens this was not much of a
problem, since they had 4 lanes, and requiring more lanes than the
needed is ok, as long as you have all the lanes.

Notice that this might improve the case where we use pipes B and C on
Ivy Bridge since both pipes only have 4 lanes to share (see
ironlake_check_fdi_lanes).

Cc: Adam Jackson <ajax@redhat.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

As it is, this one will make the list of supported modes on Haswell VGA bigger,
so we could skip 3.8 and send this through 3.9, so we have plently of time to
get confident this won't break older platforms.

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8d86a39..1825ae7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5231,12 +5231,10 @@ static bool ironlake_check_fdi_lanes(struct intel_crtc *intel_crtc)
 int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp)
 {
 	/*
-	 * Account for spread spectrum to avoid
-	 * oversubscribing the link. Max center spread
-	 * is 2.5%; use 5% for safety's sake.
+	 * The spec says:
+	 * Number of lanes >= INT(dot clock * bytes per pixel / ls_clk)
 	 */
-	u32 bps = target_clock * bpp * 21 / 20;
-	return bps / (link_bw * 8) + 1;
+	return DIV_ROUND_UP(target_clock * bpp, link_bw * 8);
 }
 
 static void ironlake_set_m_n(struct drm_crtc *crtc,
@@ -5296,6 +5294,8 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
 		lane = ironlake_get_lanes_required(target_clock, link_bw,
 						   intel_crtc->bpp);
 
+	DRM_DEBUG_KMS("Using %d FDI lanes on pipe %c\n", lane,
+		      pipe_name(intel_crtc->pipe));
 	intel_crtc->fdi_lanes = lane;
 
 	if (pixel_multiplier > 1)
-- 
1.7.11.7

Re: [PATCH 3/3] drm/i915: fix FDI lane calculation

[Chris Wilson]

On Thu, 29 Nov 2012 11:29:33 -0200, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The previous code was making the bps value 5% higher than what the
> spec says, which was enough to make certain VGA modes require 3 lanes
> instead of 2, which makes us reject these modes on Haswell since it
> only has 2 FDI lanes. For previous gens this was not much of a
> problem, since they had 4 lanes, and requiring more lanes than the
> needed is ok, as long as you have all the lanes.
> 
> Notice that this might improve the case where we use pipes B and C on
> Ivy Bridge since both pipes only have 4 lanes to share (see
> ironlake_check_fdi_lanes).
> 
> Cc: Adam Jackson <ajax@redhat.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> As it is, this one will make the list of supported modes on Haswell VGA bigger,
> so we could skip 3.8 and send this through 3.9, so we have plently of time to
> get confident this won't break older platforms.
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8d86a39..1825ae7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5231,12 +5231,10 @@ static bool ironlake_check_fdi_lanes(struct intel_crtc *intel_crtc)
>  int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp)
>  {
>  	/*
> -	 * Account for spread spectrum to avoid
> -	 * oversubscribing the link. Max center spread
> -	 * is 2.5%; use 5% for safety's sake.
> +	 * The spec says:
> +	 * Number of lanes >= INT(dot clock * bytes per pixel / ls_clk)
>  	 */
> -	u32 bps = target_clock * bpp * 21 / 20;
> -	return bps / (link_bw * 8) + 1;
> +	return DIV_ROUND_UP(target_clock * bpp, link_bw * 8);

Can you split this into two patches, one for using DIV_ROUND_UP and for
removingthe oversubscription, as the DIV_ROUND_UP looks to be a separate
issue worth testing. (Handling the case where bps % (link_bw * 8) == 0.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: fix FDI lane calculation

[Adam Jackson]

On 11/29/12 8:29 AM, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> The previous code was making the bps value 5% higher than what the
> spec says, which was enough to make certain VGA modes require 3 lanes
> instead of 2, which makes us reject these modes on Haswell since it
> only has 2 FDI lanes. For previous gens this was not much of a
> problem, since they had 4 lanes, and requiring more lanes than the
> needed is ok, as long as you have all the lanes.
>
> Notice that this might improve the case where we use pipes B and C on
> Ivy Bridge since both pipes only have 4 lanes to share (see
> ironlake_check_fdi_lanes).

Fine with me.  I'm not entirely sure the SS check I had there was 
necessary; I do remember the docs saying to account for it, but I'm not 
sure the check we had there was correct.

Reviewed-by: Adam Jackson <ajax@redhat.com>

- ajax

Re: [PATCH 1/3] drm/i915: fix hsw_fdi_link_train "retry" code

[Daniel Vetter]

On Thu, Nov 29, 2012 at 11:29:31AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We were previously doing exactly what the "mode set sequence for CRT"
> document mandates, but whenever we failed to train the link in the
> first tentative, all the other subsequent retries always failed. In
> one of my monitors that has 47 modes, I was usually getting around 3
> failures when running "testdisplay -a".
> 
> After this patch, even if we fail in the first tentative, we can
> succeed in the next ones. So now when running "testdisplay -a" I see
> around 3 times the message "FDI link training done on step 1" and no
> failures.
> 
> Notice that now the "retry" code looks a lot like the DP retry code.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 43 +++++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 16 deletions(-)
> 
> I'd like this to go to 3.8 somehow.
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 852012b..3264cb4 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -138,6 +138,19 @@ static const long hsw_ddi_buf_ctl_values[] = {
>  	DDI_BUF_EMP_800MV_3_5DB_HSW
>  };
>  
> +static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
> +				    enum port port)
> +{
> +	uint32_t reg = DDI_BUF_CTL(port);
> +	int i;
> +
> +	for (i = 0; i < 8; i++) {
> +		udelay(1);
> +		if (I915_READ(reg) & DDI_BUF_IS_IDLE)
> +			return;
> +	}
> +	DRM_ERROR("Timeout waiting for DDI BUF %c idle bit\n", port_name(port));
> +}
>  
>  /* Starting with Haswell, different DDI ports can work in FDI mode for
>   * connection to the PCH-located connectors. For this, it is necessary to train
> @@ -231,18 +244,30 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
>  			return;
>  		}
>  
> +		temp = I915_READ(DDI_BUF_CTL(PORT_E));
> +		temp &= ~DDI_BUF_CTL_ENABLE;
> +		I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
> +		POSTING_READ(DDI_BUF_CTL(PORT_E));
> +
>  		/* Disable DP_TP_CTL and FDI_RX_CTL and retry */
> -		I915_WRITE(DP_TP_CTL(PORT_E),
> -			   I915_READ(DP_TP_CTL(PORT_E)) & ~DP_TP_CTL_ENABLE);
> +		temp = I915_READ(DP_TP_CTL(PORT_E));
> +		temp &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
> +		temp |= DP_TP_CTL_LINK_TRAIN_PAT1;
> +		I915_WRITE(DP_TP_CTL(PORT_E), temp);
> +		POSTING_READ(DP_TP_CTL(PORT_E));
> +
> +		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
>  
>  		rx_ctl_val &= ~FDI_RX_ENABLE;
>  		I915_WRITE(_FDI_RXA_CTL, rx_ctl_val);
> +		POSTING_READ(_FDI_RXA_CTL);
>  
>  		/* Reset FDI_RX_MISC pwrdn lanes */
>  		temp = I915_READ(_FDI_RXA_MISC);
>  		temp &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
>  		temp |= FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2);
>  		I915_WRITE(_FDI_RXA_MISC, temp);
> +		POSTING_READ(_FDI_RXA_MISC);

What now slightly irks me here is that this sequence and the one in
intel_ddi_fdi_disable don't match exactly. Imo it would make sense to have
both the same (after all, we disable the same piece of hw) - have you
tried that out (there's obviously some slight unification required first)?
-Daniel

>  	}
>  
>  	DRM_ERROR("FDI link training failed!\n");
> @@ -1222,20 +1247,6 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>  	}
>  }
>  
> -static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
> -				    enum port port)
> -{
> -	uint32_t reg = DDI_BUF_CTL(port);
> -	int i;
> -
> -	for (i = 0; i < 8; i++) {
> -		udelay(1);
> -		if (I915_READ(reg) & DDI_BUF_IS_IDLE)
> -			return;
> -	}
> -	DRM_ERROR("Timeout waiting for DDI BUF %c idle bit\n", port_name(port));
> -}
> -
>  static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>  {
>  	struct drm_encoder *encoder = &intel_encoder->base;
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/3] drm/i915: fix hsw_fdi_link_train "retry" code

[Paulo Zanoni]

Hi

2012/11/29 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Nov 29, 2012 at 11:29:31AM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> We were previously doing exactly what the "mode set sequence for CRT"
>> document mandates, but whenever we failed to train the link in the
>> first tentative, all the other subsequent retries always failed. In
>> one of my monitors that has 47 modes, I was usually getting around 3
>> failures when running "testdisplay -a".
>>
>> After this patch, even if we fail in the first tentative, we can
>> succeed in the next ones. So now when running "testdisplay -a" I see
>> around 3 times the message "FDI link training done on step 1" and no
>> failures.
>>
>> Notice that now the "retry" code looks a lot like the DP retry code.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 43 +++++++++++++++++++++++++---------------
>>  1 file changed, 27 insertions(+), 16 deletions(-)
>>
>> I'd like this to go to 3.8 somehow.
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 852012b..3264cb4 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -138,6 +138,19 @@ static const long hsw_ddi_buf_ctl_values[] = {
>>       DDI_BUF_EMP_800MV_3_5DB_HSW
>>  };
>>
>> +static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
>> +                                 enum port port)
>> +{
>> +     uint32_t reg = DDI_BUF_CTL(port);
>> +     int i;
>> +
>> +     for (i = 0; i < 8; i++) {
>> +             udelay(1);
>> +             if (I915_READ(reg) & DDI_BUF_IS_IDLE)
>> +                     return;
>> +     }
>> +     DRM_ERROR("Timeout waiting for DDI BUF %c idle bit\n", port_name(port));
>> +}
>>
>>  /* Starting with Haswell, different DDI ports can work in FDI mode for
>>   * connection to the PCH-located connectors. For this, it is necessary to train
>> @@ -231,18 +244,30 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
>>                       return;
>>               }
>>
>> +             temp = I915_READ(DDI_BUF_CTL(PORT_E));
>> +             temp &= ~DDI_BUF_CTL_ENABLE;
>> +             I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
>> +             POSTING_READ(DDI_BUF_CTL(PORT_E));
>> +
>>               /* Disable DP_TP_CTL and FDI_RX_CTL and retry */
>> -             I915_WRITE(DP_TP_CTL(PORT_E),
>> -                        I915_READ(DP_TP_CTL(PORT_E)) & ~DP_TP_CTL_ENABLE);
>> +             temp = I915_READ(DP_TP_CTL(PORT_E));
>> +             temp &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK);
>> +             temp |= DP_TP_CTL_LINK_TRAIN_PAT1;
>> +             I915_WRITE(DP_TP_CTL(PORT_E), temp);
>> +             POSTING_READ(DP_TP_CTL(PORT_E));
>> +
>> +             intel_wait_ddi_buf_idle(dev_priv, PORT_E);
>>
>>               rx_ctl_val &= ~FDI_RX_ENABLE;
>>               I915_WRITE(_FDI_RXA_CTL, rx_ctl_val);
>> +             POSTING_READ(_FDI_RXA_CTL);
>>
>>               /* Reset FDI_RX_MISC pwrdn lanes */
>>               temp = I915_READ(_FDI_RXA_MISC);
>>               temp &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
>>               temp |= FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2);
>>               I915_WRITE(_FDI_RXA_MISC, temp);
>> +             POSTING_READ(_FDI_RXA_MISC);
>
> What now slightly irks me here is that this sequence and the one in
> intel_ddi_fdi_disable don't match exactly. Imo it would make sense to have
> both the same (after all, we disable the same piece of hw) - have you
> tried that out (there's obviously some slight unification required first)?

The "FDI retrain" and "FDI disable" sequences don't match, just like
the "DP retrain" and "DP disable" sequences also don't match. One is a
full disable, the other is just a link retrain in the middle of the
enable sequence, disabling only what's needed. We're following the
spec for the disable sequences and we're also following the spec for
the "retrain" sequence (except for the lack of the "disable
DDI_BUF_CTL" instruction introduced by this patch). What do you
suggest to change?

> -Daniel
>
>>       }
>>
>>       DRM_ERROR("FDI link training failed!\n");
>> @@ -1222,20 +1247,6 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>>       }
>>  }
>>
>> -static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv,
>> -                                 enum port port)
>> -{
>> -     uint32_t reg = DDI_BUF_CTL(port);
>> -     int i;
>> -
>> -     for (i = 0; i < 8; i++) {
>> -             udelay(1);
>> -             if (I915_READ(reg) & DDI_BUF_IS_IDLE)
>> -                     return;
>> -     }
>> -     DRM_ERROR("Timeout waiting for DDI BUF %c idle bit\n", port_name(port));
>> -}
>> -
>>  static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>>  {
>>       struct drm_encoder *encoder = &intel_encoder->base;
>> --
>> 1.7.11.7
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 1/3] drm/i915: fix hsw_fdi_link_train "retry" code

[Daniel Vetter]

On Fri, Nov 30, 2012 at 4:39 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> The "FDI retrain" and "FDI disable" sequences don't match, just like
> the "DP retrain" and "DP disable" sequences also don't match. One is a
> full disable, the other is just a link retrain in the middle of the
> enable sequence, disabling only what's needed. We're following the
> spec for the disable sequences and we're also following the spec for
> the "retrain" sequence (except for the lack of the "disable
> DDI_BUF_CTL" instruction introduced by this patch). What do you
> suggest to change?

It's just that I've sacrified too many kittens yesterday trying to get
fdi link training to work on ivb (to no avail), so when I see too much
magic code I start to freak out. And on a quick hunch I don't see why
disabling fdi when the link train fails or when we disable the thing
for other reasons should be different. But I've you're convinced that
this is the right thing, I'm ok.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/3] drm/i915: fix hsw_fdi_link_train "retry" code

[Daniel Vetter]

On Thu, Nov 29, 2012 at 11:29:31AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We were previously doing exactly what the "mode set sequence for CRT"
> document mandates, but whenever we failed to train the link in the
> first tentative, all the other subsequent retries always failed. In
> one of my monitors that has 47 modes, I was usually getting around 3
> failures when running "testdisplay -a".
> 
> After this patch, even if we fail in the first tentative, we can
> succeed in the next ones. So now when running "testdisplay -a" I see
> around 3 times the message "FDI link training done on step 1" and no
> failures.
> 
> Notice that now the "retry" code looks a lot like the DP retry code.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Picked up for -fixes, thanks for the patch. Upon re-reading it still irks
me midly that the disable sequence here matches the begining of
ddi_post_disable. Whatever.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/3] drm/i915: reject modes the LPT FDI receiver can't handle

[Daniel Vetter]

On Thu, Nov 29, 2012 at 11:29:32AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> More specifically, the LPT FDI RX only supports 8bpc and a maximum of
> 2 lanes, so anything above that won't work and should be rejected.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Picked up for -fixes, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch