[PATCH] drm/i915: Disable FDI RX before DDI_BUF_CTL

[PATCH] drm/i915: Disable FDI RX before DDI_BUF_CTL

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Bspec is confused w.r.t. the HSW/BDW FDI disable sequence. It lists
FDI RX disable both as step 13 and step 18 in the sequence. But I dug
up an old BUN mail from Art that moved the FDI RX disable to happen
before DDI_BUF_CTL disable. That BUN did not renumber the steps and just
added a note:
"Workaround: Disable PCH FDI Receiver before disabling DDI_BUF_CTL."

The BUN described the symptoms of the fixed issue as:
"PCH display underflow and a black screen on the analog CRT port that
happened after a FDI re-train"

I suppose later someone tried to renumber the steps to match, but forgot
to remove the FDI RX disable from its old position in the sequence.

They also forgot to update the note describing what should be done in
case of an FDI training failure. Currently it says:
"To retry FDI training, follow the Disable Sequence steps to Disable FDI,
but skip the steps related to clocks and PLLs (16, 19, and 20), ..."

It should really say "17, 20, and 21" with the current sequence because
those are the steps that deal with PLLs and whatnot, after step 13 became
FDI RX disable. And had the step 18 FDI RX disable been removed, as I
suspect it should have, the note should actually say "17, 19, and 20".

So, let's move the FDI RX disable to happen before DDI_BUF_CTL disable,
as that would appear to be the correct order based on the BUN.

Cc: Paulo Zanoni <przanoni@gmail.com>
Cc: Art Runyan <arthur.j.runyan@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 5d20c64d8566..a89a17b7bb76 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -687,6 +687,10 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
 			break;
 		}
 
+		rx_ctl_val &= ~FDI_RX_ENABLE;
+		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
+		POSTING_READ(FDI_RX_CTL(PIPE_A));
+
 		temp = I915_READ(DDI_BUF_CTL(PORT_E));
 		temp &= ~DDI_BUF_CTL_ENABLE;
 		I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
@@ -701,10 +705,6 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
 
 		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
 
-		rx_ctl_val &= ~FDI_RX_ENABLE;
-		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
-		POSTING_READ(FDI_RX_CTL(PIPE_A));
-
 		/* Reset FDI_RX_MISC pwrdn lanes */
 		temp = I915_READ(FDI_RX_MISC(PIPE_A));
 		temp &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
@@ -3094,12 +3094,18 @@ void intel_ddi_fdi_disable(struct drm_crtc *crtc)
 	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
 	uint32_t val;
 
-	intel_ddi_post_disable(intel_encoder);
-
+	/*
+	 * Bspec lists this as both step 13 (before DDI_BUF_CTL disable)
+	 * and step 18 (after clearing PORT_CLK_SEL). Based on a BUN,
+	 * step 13 is the correct place for it. Step 18 is where it was
+	 * originally before the BUN.
+	 */
 	val = I915_READ(FDI_RX_CTL(PIPE_A));
 	val &= ~FDI_RX_ENABLE;
 	I915_WRITE(FDI_RX_CTL(PIPE_A), val);
 
+	intel_ddi_post_disable(intel_encoder);
+
 	val = I915_READ(FDI_RX_MISC(PIPE_A));
 	val &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
 	val |= FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2);
-- 
2.4.10

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

Re: [PATCH] drm/i915: Disable FDI RX before DDI_BUF_CTL

[Daniel Vetter]

On Tue, Dec 08, 2015 at 04:47:55PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Bspec is confused w.r.t. the HSW/BDW FDI disable sequence. It lists
> FDI RX disable both as step 13 and step 18 in the sequence. But I dug
> up an old BUN mail from Art that moved the FDI RX disable to happen
> before DDI_BUF_CTL disable. That BUN did not renumber the steps and just
> added a note:
> "Workaround: Disable PCH FDI Receiver before disabling DDI_BUF_CTL."
> 
> The BUN described the symptoms of the fixed issue as:
> "PCH display underflow and a black screen on the analog CRT port that
> happened after a FDI re-train"

Hm, this doesn't exactly match what I've seen when fighting hsw underruns
when using the crt port. The testcase did do piles of fdi-retraining, and
generally it only happened on the 2nd one onwards, but symptoms have been
different. I observed:
- one-shot underrun on the pch, i.e. underrun immediately cleared and
  didn't happen again.
- 16ms later (so one vblank probably) stuck underrun on the cpu but that
  usually recovered after a few usec too.

No black screen anywhere to be seen.

Art, is this the same bug? If so I guess I need to check on my hsw whether
reverting my underrun hack would work together with this one here.
-Daniel

> 
> I suppose later someone tried to renumber the steps to match, but forgot
> to remove the FDI RX disable from its old position in the sequence.
> 
> They also forgot to update the note describing what should be done in
> case of an FDI training failure. Currently it says:
> "To retry FDI training, follow the Disable Sequence steps to Disable FDI,
> but skip the steps related to clocks and PLLs (16, 19, and 20), ..."
> 
> It should really say "17, 20, and 21" with the current sequence because
> those are the steps that deal with PLLs and whatnot, after step 13 became
> FDI RX disable. And had the step 18 FDI RX disable been removed, as I
> suspect it should have, the note should actually say "17, 19, and 20".
> 
> So, let's move the FDI RX disable to happen before DDI_BUF_CTL disable,
> as that would appear to be the correct order based on the BUN.
> 
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Cc: Art Runyan <arthur.j.runyan@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 5d20c64d8566..a89a17b7bb76 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -687,6 +687,10 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
>  			break;
>  		}
>  
> +		rx_ctl_val &= ~FDI_RX_ENABLE;
> +		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> +		POSTING_READ(FDI_RX_CTL(PIPE_A));
> +
>  		temp = I915_READ(DDI_BUF_CTL(PORT_E));
>  		temp &= ~DDI_BUF_CTL_ENABLE;
>  		I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
> @@ -701,10 +705,6 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
>  
>  		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
>  
> -		rx_ctl_val &= ~FDI_RX_ENABLE;
> -		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> -		POSTING_READ(FDI_RX_CTL(PIPE_A));
> -
>  		/* Reset FDI_RX_MISC pwrdn lanes */
>  		temp = I915_READ(FDI_RX_MISC(PIPE_A));
>  		temp &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
> @@ -3094,12 +3094,18 @@ void intel_ddi_fdi_disable(struct drm_crtc *crtc)
>  	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
>  	uint32_t val;
>  
> -	intel_ddi_post_disable(intel_encoder);
> -
> +	/*
> +	 * Bspec lists this as both step 13 (before DDI_BUF_CTL disable)
> +	 * and step 18 (after clearing PORT_CLK_SEL). Based on a BUN,
> +	 * step 13 is the correct place for it. Step 18 is where it was
> +	 * originally before the BUN.
> +	 */
>  	val = I915_READ(FDI_RX_CTL(PIPE_A));
>  	val &= ~FDI_RX_ENABLE;
>  	I915_WRITE(FDI_RX_CTL(PIPE_A), val);
>  
> +	intel_ddi_post_disable(intel_encoder);
> +
>  	val = I915_READ(FDI_RX_MISC(PIPE_A));
>  	val &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
>  	val |= FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2);
> -- 
> 2.4.10
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Disable FDI RX before DDI_BUF_CTL

[Ville Syrjälä]

On Tue, Dec 08, 2015 at 04:47:55PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Bspec is confused w.r.t. the HSW/BDW FDI disable sequence. It lists
> FDI RX disable both as step 13 and step 18 in the sequence. But I dug
> up an old BUN mail from Art that moved the FDI RX disable to happen
> before DDI_BUF_CTL disable. That BUN did not renumber the steps and just
> added a note:
> "Workaround: Disable PCH FDI Receiver before disabling DDI_BUF_CTL."
> 
> The BUN described the symptoms of the fixed issue as:
> "PCH display underflow and a black screen on the analog CRT port that
> happened after a FDI re-train"
> 
> I suppose later someone tried to renumber the steps to match, but forgot
> to remove the FDI RX disable from its old position in the sequence.
> 
> They also forgot to update the note describing what should be done in
> case of an FDI training failure. Currently it says:
> "To retry FDI training, follow the Disable Sequence steps to Disable FDI,
> but skip the steps related to clocks and PLLs (16, 19, and 20), ..."
> 
> It should really say "17, 20, and 21" with the current sequence because
> those are the steps that deal with PLLs and whatnot, after step 13 became
> FDI RX disable. And had the step 18 FDI RX disable been removed, as I
> suspect it should have, the note should actually say "17, 19, and 20".
> 
> So, let's move the FDI RX disable to happen before DDI_BUF_CTL disable,
> as that would appear to be the correct order based on the BUN.

Ping. Art, I hear you're back now ;) Any thoughts on this change, and
the slight mess in Bspec?


> 
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Cc: Art Runyan <arthur.j.runyan@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 5d20c64d8566..a89a17b7bb76 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -687,6 +687,10 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
>  			break;
>  		}
>  
> +		rx_ctl_val &= ~FDI_RX_ENABLE;
> +		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> +		POSTING_READ(FDI_RX_CTL(PIPE_A));
> +
>  		temp = I915_READ(DDI_BUF_CTL(PORT_E));
>  		temp &= ~DDI_BUF_CTL_ENABLE;
>  		I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
> @@ -701,10 +705,6 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
>  
>  		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
>  
> -		rx_ctl_val &= ~FDI_RX_ENABLE;
> -		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> -		POSTING_READ(FDI_RX_CTL(PIPE_A));
> -
>  		/* Reset FDI_RX_MISC pwrdn lanes */
>  		temp = I915_READ(FDI_RX_MISC(PIPE_A));
>  		temp &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
> @@ -3094,12 +3094,18 @@ void intel_ddi_fdi_disable(struct drm_crtc *crtc)
>  	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
>  	uint32_t val;
>  
> -	intel_ddi_post_disable(intel_encoder);
> -
> +	/*
> +	 * Bspec lists this as both step 13 (before DDI_BUF_CTL disable)
> +	 * and step 18 (after clearing PORT_CLK_SEL). Based on a BUN,
> +	 * step 13 is the correct place for it. Step 18 is where it was
> +	 * originally before the BUN.
> +	 */
>  	val = I915_READ(FDI_RX_CTL(PIPE_A));
>  	val &= ~FDI_RX_ENABLE;
>  	I915_WRITE(FDI_RX_CTL(PIPE_A), val);
>  
> +	intel_ddi_post_disable(intel_encoder);
> +
>  	val = I915_READ(FDI_RX_MISC(PIPE_A));
>  	val &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
>  	val |= FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2);
> -- 
> 2.4.10

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Disable FDI RX before DDI_BUF_CTL

[Runyan, Arthur J]

Some bit rot there.  I'll fix the numbering.  Thanks for pointing it out.
I did keep the second, redundant, FDI RX disable in place to limit some closed source driver changes.  There is no downside to clearing bit 31 twice. 

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Wednesday, February 17, 2016 9:37 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Paulo Zanoni; Runyan, Arthur J
>Subject: Re: [PATCH] drm/i915: Disable FDI RX before DDI_BUF_CTL
>
>On Tue, Dec 08, 2015 at 04:47:55PM +0200, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Bspec is confused w.r.t. the HSW/BDW FDI disable sequence. It lists
>> FDI RX disable both as step 13 and step 18 in the sequence. But I dug
>> up an old BUN mail from Art that moved the FDI RX disable to happen
>> before DDI_BUF_CTL disable. That BUN did not renumber the steps and just
>> added a note:
>> "Workaround: Disable PCH FDI Receiver before disabling DDI_BUF_CTL."
>>
>> The BUN described the symptoms of the fixed issue as:
>> "PCH display underflow and a black screen on the analog CRT port that
>> happened after a FDI re-train"
>>
>> I suppose later someone tried to renumber the steps to match, but forgot
>> to remove the FDI RX disable from its old position in the sequence.
>>
>> They also forgot to update the note describing what should be done in
>> case of an FDI training failure. Currently it says:
>> "To retry FDI training, follow the Disable Sequence steps to Disable FDI,
>> but skip the steps related to clocks and PLLs (16, 19, and 20), ..."
>>
>> It should really say "17, 20, and 21" with the current sequence because
>> those are the steps that deal with PLLs and whatnot, after step 13 became
>> FDI RX disable. And had the step 18 FDI RX disable been removed, as I
>> suspect it should have, the note should actually say "17, 19, and 20".
>>
>> So, let's move the FDI RX disable to happen before DDI_BUF_CTL disable,
>> as that would appear to be the correct order based on the BUN.
>
>Ping. Art, I hear you're back now ;) Any thoughts on this change, and
>the slight mess in Bspec?
>
>
>>
>> Cc: Paulo Zanoni <przanoni@gmail.com>
>> Cc: Art Runyan <arthur.j.runyan@intel.com>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 5d20c64d8566..a89a17b7bb76 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -687,6 +687,10 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
>>  			break;
>>  		}
>>
>> +		rx_ctl_val &= ~FDI_RX_ENABLE;
>> +		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
>> +		POSTING_READ(FDI_RX_CTL(PIPE_A));
>> +
>>  		temp = I915_READ(DDI_BUF_CTL(PORT_E));
>>  		temp &= ~DDI_BUF_CTL_ENABLE;
>>  		I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
>> @@ -701,10 +705,6 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
>>
>>  		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
>>
>> -		rx_ctl_val &= ~FDI_RX_ENABLE;
>> -		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
>> -		POSTING_READ(FDI_RX_CTL(PIPE_A));
>> -
>>  		/* Reset FDI_RX_MISC pwrdn lanes */
>>  		temp = I915_READ(FDI_RX_MISC(PIPE_A));
>>  		temp &= ~(FDI_RX_PWRDN_LANE1_MASK |
>FDI_RX_PWRDN_LANE0_MASK);
>> @@ -3094,12 +3094,18 @@ void intel_ddi_fdi_disable(struct drm_crtc *crtc)
>>  	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
>>  	uint32_t val;
>>
>> -	intel_ddi_post_disable(intel_encoder);
>> -
>> +	/*
>> +	 * Bspec lists this as both step 13 (before DDI_BUF_CTL disable)
>> +	 * and step 18 (after clearing PORT_CLK_SEL). Based on a BUN,
>> +	 * step 13 is the correct place for it. Step 18 is where it was
>> +	 * originally before the BUN.
>> +	 */
>>  	val = I915_READ(FDI_RX_CTL(PIPE_A));
>>  	val &= ~FDI_RX_ENABLE;
>>  	I915_WRITE(FDI_RX_CTL(PIPE_A), val);
>>
>> +	intel_ddi_post_disable(intel_encoder);
>> +
>>  	val = I915_READ(FDI_RX_MISC(PIPE_A));
>>  	val &= ~(FDI_RX_PWRDN_LANE1_MASK |
>FDI_RX_PWRDN_LANE0_MASK);
>>  	val |= FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2);
>> --
>> 2.4.10
>
>--
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Disable FDI RX before DDI_BUF_CTL

[Ville Syrjälä]

On Thu, Feb 18, 2016 at 02:14:17AM +0000, Runyan, Arthur J wrote:
> Some bit rot there.  I'll fix the numbering.  Thanks for pointing it out.
> I did keep the second, redundant, FDI RX disable in place to limit some closed source driver changes.  There is no downside to clearing bit 31 twice. 

Thanks. I quickly scanned the new text and didn't spot any further
inconsistencies.

> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Wednesday, February 17, 2016 9:37 AM
> >To: intel-gfx@lists.freedesktop.org
> >Cc: Paulo Zanoni; Runyan, Arthur J
> >Subject: Re: [PATCH] drm/i915: Disable FDI RX before DDI_BUF_CTL
> >
> >On Tue, Dec 08, 2015 at 04:47:55PM +0200, ville.syrjala@linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Bspec is confused w.r.t. the HSW/BDW FDI disable sequence. It lists
> >> FDI RX disable both as step 13 and step 18 in the sequence. But I dug
> >> up an old BUN mail from Art that moved the FDI RX disable to happen
> >> before DDI_BUF_CTL disable. That BUN did not renumber the steps and just
> >> added a note:
> >> "Workaround: Disable PCH FDI Receiver before disabling DDI_BUF_CTL."
> >>
> >> The BUN described the symptoms of the fixed issue as:
> >> "PCH display underflow and a black screen on the analog CRT port that
> >> happened after a FDI re-train"
> >>
> >> I suppose later someone tried to renumber the steps to match, but forgot
> >> to remove the FDI RX disable from its old position in the sequence.
> >>
> >> They also forgot to update the note describing what should be done in
> >> case of an FDI training failure. Currently it says:
> >> "To retry FDI training, follow the Disable Sequence steps to Disable FDI,
> >> but skip the steps related to clocks and PLLs (16, 19, and 20), ..."
> >>
> >> It should really say "17, 20, and 21" with the current sequence because
> >> those are the steps that deal with PLLs and whatnot, after step 13 became
> >> FDI RX disable. And had the step 18 FDI RX disable been removed, as I
> >> suspect it should have, the note should actually say "17, 19, and 20".
> >>
> >> So, let's move the FDI RX disable to happen before DDI_BUF_CTL disable,
> >> as that would appear to be the correct order based on the BUN.
> >
> >Ping. Art, I hear you're back now ;) Any thoughts on this change, and
> >the slight mess in Bspec?
> >
> >
> >>
> >> Cc: Paulo Zanoni <przanoni@gmail.com>
> >> Cc: Art Runyan <arthur.j.runyan@intel.com>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++------
> >>  1 file changed, 12 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index 5d20c64d8566..a89a17b7bb76 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -687,6 +687,10 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
> >>  			break;
> >>  		}
> >>
> >> +		rx_ctl_val &= ~FDI_RX_ENABLE;
> >> +		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> >> +		POSTING_READ(FDI_RX_CTL(PIPE_A));
> >> +
> >>  		temp = I915_READ(DDI_BUF_CTL(PORT_E));
> >>  		temp &= ~DDI_BUF_CTL_ENABLE;
> >>  		I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
> >> @@ -701,10 +705,6 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
> >>
> >>  		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
> >>
> >> -		rx_ctl_val &= ~FDI_RX_ENABLE;
> >> -		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> >> -		POSTING_READ(FDI_RX_CTL(PIPE_A));
> >> -
> >>  		/* Reset FDI_RX_MISC pwrdn lanes */
> >>  		temp = I915_READ(FDI_RX_MISC(PIPE_A));
> >>  		temp &= ~(FDI_RX_PWRDN_LANE1_MASK |
> >FDI_RX_PWRDN_LANE0_MASK);
> >> @@ -3094,12 +3094,18 @@ void intel_ddi_fdi_disable(struct drm_crtc *crtc)
> >>  	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
> >>  	uint32_t val;
> >>
> >> -	intel_ddi_post_disable(intel_encoder);
> >> -
> >> +	/*
> >> +	 * Bspec lists this as both step 13 (before DDI_BUF_CTL disable)
> >> +	 * and step 18 (after clearing PORT_CLK_SEL). Based on a BUN,
> >> +	 * step 13 is the correct place for it. Step 18 is where it was
> >> +	 * originally before the BUN.
> >> +	 */
> >>  	val = I915_READ(FDI_RX_CTL(PIPE_A));
> >>  	val &= ~FDI_RX_ENABLE;
> >>  	I915_WRITE(FDI_RX_CTL(PIPE_A), val);
> >>
> >> +	intel_ddi_post_disable(intel_encoder);
> >> +
> >>  	val = I915_READ(FDI_RX_MISC(PIPE_A));
> >>  	val &= ~(FDI_RX_PWRDN_LANE1_MASK |
> >FDI_RX_PWRDN_LANE0_MASK);
> >>  	val |= FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2);
> >> --
> >> 2.4.10
> >
> >--
> >Ville Syrjälä
> >Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Disable FDI RX before DDI_BUF_CTL

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Bspec is confused w.r.t. the HSW/BDW FDI disable sequence. It lists
FDI RX disable both as step 13 and step 18 in the sequence. But I dug
up an old BUN mail from Art that moved the FDI RX disable to happen
before DDI_BUF_CTL disable. That BUN did not renumber the steps and just
added a note:
"Workaround: Disable PCH FDI Receiver before disabling DDI_BUF_CTL."

The BUN described the symptoms of the fixed issue as:
"PCH display underflow and a black screen on the analog CRT port that
happened after a FDI re-train"

I suppose later someone tried to renumber the steps to match, but forgot
to remove the FDI RX disable from its old position in the sequence.

They also forgot to update the note describing what should be done in
case of an FDI training failure. Currently it says:
"To retry FDI training, follow the Disable Sequence steps to Disable FDI,
but skip the steps related to clocks and PLLs (16, 19, and 20), ..."

It should really say "17, 20, and 21" with the current sequence because
those are the steps that deal with PLLs and whatnot, after step 13 became
FDI RX disable. And had the step 18 FDI RX disable been removed, as I
suspect it should have, the note should actually say "17, 19, and 20".

So, let's move the FDI RX disable to happen before DDI_BUF_CTL disable,
as that would appear to be the correct order based on the BUN.

Note that Art has since unconfused the spec, and so this patch should
now match the steps listed in the spec.

v2: Add a note that the spec is now correct

Cc: Paulo Zanoni <przanoni@gmail.com>
Cc: Art Runyan <arthur.j.runyan@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 21a9b83f3bfc..fc4ca50f7159 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -629,6 +629,10 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
 			break;
 		}
 
+		rx_ctl_val &= ~FDI_RX_ENABLE;
+		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
+		POSTING_READ(FDI_RX_CTL(PIPE_A));
+
 		temp = I915_READ(DDI_BUF_CTL(PORT_E));
 		temp &= ~DDI_BUF_CTL_ENABLE;
 		I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
@@ -643,10 +647,6 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
 
 		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
 
-		rx_ctl_val &= ~FDI_RX_ENABLE;
-		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
-		POSTING_READ(FDI_RX_CTL(PIPE_A));
-
 		/* Reset FDI_RX_MISC pwrdn lanes */
 		temp = I915_READ(FDI_RX_MISC(PIPE_A));
 		temp &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
@@ -3078,12 +3078,18 @@ void intel_ddi_fdi_disable(struct drm_crtc *crtc)
 	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
 	uint32_t val;
 
-	intel_ddi_post_disable(intel_encoder);
-
+	/*
+	 * Bspec lists this as both step 13 (before DDI_BUF_CTL disable)
+	 * and step 18 (after clearing PORT_CLK_SEL). Based on a BUN,
+	 * step 13 is the correct place for it. Step 18 is where it was
+	 * originally before the BUN.
+	 */
 	val = I915_READ(FDI_RX_CTL(PIPE_A));
 	val &= ~FDI_RX_ENABLE;
 	I915_WRITE(FDI_RX_CTL(PIPE_A), val);
 
+	intel_ddi_post_disable(intel_encoder);
+
 	val = I915_READ(FDI_RX_MISC(PIPE_A));
 	val &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
 	val |= FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2);
-- 
2.4.10

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

✗ Fi.CI.BAT: failure for drm/i915: Disable FDI RX before DDI_BUF_CTL (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Disable FDI RX before DDI_BUF_CTL (rev2)
URL   : https://patchwork.freedesktop.org/series/1582/
State : failure

== Summary ==

Series 1582v2 drm/i915: Disable FDI RX before DDI_BUF_CTL
http://patchwork.freedesktop.org/api/1.0/series/1582/revisions/2/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (hsw-brixbox)
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (snb-x220t)
                pass       -> FAIL       (byt-nuc)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-b:
                pass       -> DMESG-WARN (ilk-hp8440p)
        Subgroup read-crc-pipe-b:
                pass       -> DMESG-WARN (snb-x220t)
        Subgroup suspend-read-crc-pipe-a:
                incomplete -> PASS       (hsw-gt2)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (snb-x220t)

bdw-nuci7        total:169  pass:158  dwarn:0   dfail:0   fail:0   skip:11 
bdw-ultra        total:169  pass:155  dwarn:0   dfail:0   fail:0   skip:14 
bsw-nuc-2        total:169  pass:136  dwarn:2   dfail:0   fail:1   skip:30 
byt-nuc          total:169  pass:143  dwarn:0   dfail:0   fail:1   skip:25 
hsw-brixbox      total:169  pass:154  dwarn:0   dfail:0   fail:0   skip:15 
hsw-gt2          total:169  pass:158  dwarn:0   dfail:0   fail:1   skip:10 
ilk-hp8440p      total:169  pass:116  dwarn:2   dfail:0   fail:1   skip:50 
ivb-t430s        total:169  pass:153  dwarn:0   dfail:0   fail:1   skip:15 
skl-i5k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16 
skl-i7k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16 
snb-dellxps      total:169  pass:144  dwarn:1   dfail:0   fail:1   skip:23 
snb-x220t        total:169  pass:144  dwarn:1   dfail:0   fail:2   skip:22 

Results at /archive/results/CI_IGT_test/Patchwork_1504/

deb861ea08c6d5bbf682fb40ea1f0471a448aafd drm-intel-nightly: 2016y-03m-01d-11h-12m-16s UTC integration manifest
644bb2727416f9dfb6f1b45bb43291e6ac0e65b2 drm/i915: Disable FDI RX before DDI_BUF_CTL

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

Re: ✗ Fi.CI.BAT: failure for drm/i915: Disable FDI RX before DDI_BUF_CTL (rev2)

[Ville Syrjälä]

On Tue, Mar 01, 2016 at 02:55:54PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Disable FDI RX before DDI_BUF_CTL (rev2)
> URL   : https://patchwork.freedesktop.org/series/1582/
> State : failure
> 
> == Summary ==
> 
> Series 1582v2 drm/i915: Disable FDI RX before DDI_BUF_CTL
> http://patchwork.freedesktop.org/api/1.0/series/1582/revisions/2/mbox/
> 
> Test kms_flip:
>         Subgroup basic-flip-vs-dpms:
>                 dmesg-warn -> PASS       (hsw-brixbox)
>         Subgroup basic-flip-vs-modeset:
>                 pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE

[  131.240747] [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO underrun
https://bugs.freedesktop.org/show_bug.cgi?id=93787

>         Subgroup basic-flip-vs-wf_vblank:
>                 fail       -> PASS       (snb-x220t)
>                 pass       -> FAIL       (byt-nuc)

(kms_flip:7050) DEBUG: name = vblank
last_ts = 464.937064 usec
last_received_ts = 464.936789 usec
last_seq = 1693
current_ts = 465.120306 usec
current_received_ts = 465.120267 usec
current_seq = 1703
count = 65
seq_step = 10
https://bugs.freedesktop.org/show_bug.cgi?id=94294

> Test kms_pipe_crc_basic:
>         Subgroup nonblocking-crc-pipe-b:
>                 pass       -> DMESG-WARN (ilk-hp8440p)

[  156.976891] [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe A FIFO underrun
https://bugs.freedesktop.org/show_bug.cgi?id=93787

>         Subgroup read-crc-pipe-b:
>                 pass       -> DMESG-WARN (snb-x220t)

[  369.954769] WARNING: CPU: 0 PID: 6181 at drivers/gpu/drm/i915/intel_drv.h:1468 gen6_write32+0x1dc/0x270 [i915]()
[  369.954777] Device suspended during HW access
https://bugs.freedesktop.org/show_bug.cgi?id=94349

>         Subgroup suspend-read-crc-pipe-a:
>                 incomplete -> PASS       (hsw-gt2)
>         Subgroup suspend-read-crc-pipe-c:
>                 pass       -> DMESG-WARN (bsw-nuc-2)

[  154.000506] ======================================================
[  154.000507] [ INFO: possible circular locking dependency detected ]
[  154.000512] 4.5.0-rc6-gfxbench+ #1 Tainted: G     U         
[  154.000513] -------------------------------------------------------
[  154.000515] rtcwake/5929 is trying to acquire lock:
[  154.000530]  (s_active#6){++++.+}, at: [<ffffffff8124ec70>]
kernfs_remove_by_name_ns+0x40/0xa0
[  154.000531] 
but task is already holding lock:
[  154.000538]  (cpu_hotplug.lock){+.+.+.}, at: [<ffffffff81078c4d>]
cpu_hotplug_begin+0x6d/0xc0
[  154.000539] 
which lock already depends on the new lock.

[  154.000540] 
the existing dependency chain (in reverse order) is:
[  154.000544] 
-> #3 (cpu_hotplug.lock){+.+.+.}:
[  154.000549]        [<ffffffff810cd09b>] lock_acquire+0xdb/0x1f0
[  154.000554]        [<ffffffff817be602>] mutex_lock_nested+0x62/0x3b0
[  154.000557]        [<ffffffff81078911>] get_online_cpus+0x61/0x80
[  154.000562]        [<ffffffff81117f1b>] stop_machine+0x1b/0xe0
[  154.000614]        [<ffffffffa015f53d>]
gen8_ggtt_insert_entries__BKL+0x2d/0x30 [i915]
[  154.000653]        [<ffffffffa0163b26>] ggtt_bind_vma+0x46/0x70

Filed a new bug https://bugs.freedesktop.org/show_bug.cgi?id=94350

> Test pm_rpm:
>         Subgroup basic-pci-d3-state:
>                 pass       -> DMESG-WARN (bsw-nuc-2)

[  166.979787] [drm:intel_runtime_suspend [i915]] *ERROR* Unclaimed access detected prior to suspending
https://bugs.freedesktop.org/show_bug.cgi?id=94164

>         Subgroup basic-rte:
>                 dmesg-warn -> PASS       (snb-x220t)
> 
> bdw-nuci7        total:169  pass:158  dwarn:0   dfail:0   fail:0   skip:11 
> bdw-ultra        total:169  pass:155  dwarn:0   dfail:0   fail:0   skip:14 
> bsw-nuc-2        total:169  pass:136  dwarn:2   dfail:0   fail:1   skip:30 
> byt-nuc          total:169  pass:143  dwarn:0   dfail:0   fail:1   skip:25 
> hsw-brixbox      total:169  pass:154  dwarn:0   dfail:0   fail:0   skip:15 
> hsw-gt2          total:169  pass:158  dwarn:0   dfail:0   fail:1   skip:10 
> ilk-hp8440p      total:169  pass:116  dwarn:2   dfail:0   fail:1   skip:50 
> ivb-t430s        total:169  pass:153  dwarn:0   dfail:0   fail:1   skip:15 
> skl-i5k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16 
> skl-i7k-2        total:169  pass:153  dwarn:0   dfail:0   fail:0   skip:16 
> snb-dellxps      total:169  pass:144  dwarn:1   dfail:0   fail:1   skip:23 
> snb-x220t        total:169  pass:144  dwarn:1   dfail:0   fail:2   skip:22 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1504/
> 
> deb861ea08c6d5bbf682fb40ea1f0471a448aafd drm-intel-nightly: 2016y-03m-01d-11h-12m-16s UTC integration manifest
> 644bb2727416f9dfb6f1b45bb43291e6ac0e65b2 drm/i915: Disable FDI RX before DDI_BUF_CTL

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Disable FDI RX before DDI_BUF_CTL

[Zanoni, Paulo R]

Em Ter, 2016-03-01 às 16:16 +0200, ville.syrjala@linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Bspec is confused w.r.t. the HSW/BDW FDI disable sequence. It lists
> FDI RX disable both as step 13 and step 18 in the sequence. But I dug
> up an old BUN mail from Art that moved the FDI RX disable to happen
> before DDI_BUF_CTL disable. That BUN did not renumber the steps and
> just
> added a note:
> "Workaround: Disable PCH FDI Receiver before disabling DDI_BUF_CTL."
> 
> The BUN described the symptoms of the fixed issue as:
> "PCH display underflow and a black screen on the analog CRT port that
> happened after a FDI re-train"
> 
> I suppose later someone tried to renumber the steps to match, but
> forgot
> to remove the FDI RX disable from its old position in the sequence.
> 
> They also forgot to update the note describing what should be done in
> case of an FDI training failure. Currently it says:
> "To retry FDI training, follow the Disable Sequence steps to Disable
> FDI,
> but skip the steps related to clocks and PLLs (16, 19, and 20), ..."
> 
> It should really say "17, 20, and 21" with the current sequence
> because
> those are the steps that deal with PLLs and whatnot, after step 13
> became
> FDI RX disable. And had the step 18 FDI RX disable been removed, as I
> suspect it should have, the note should actually say "17, 19, and
> 20".
> 
> So, let's move the FDI RX disable to happen before DDI_BUF_CTL
> disable,
> as that would appear to be the correct order based on the BUN.
> 
> Note that Art has since unconfused the spec, and so this patch should
> now match the steps listed in the spec.

The sentence above basically says: "forget all the previous paragraphs
of this commit message since they're just history and go read BSpec
since it's now correct" :)

> 
> v2: Add a note that the spec is now correct

With or without changes:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> 
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Cc: Art Runyan <arthur.j.runyan@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 21a9b83f3bfc..fc4ca50f7159 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -629,6 +629,10 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
>  			break;
>  		}
>  
> +		rx_ctl_val &= ~FDI_RX_ENABLE;
> +		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> +		POSTING_READ(FDI_RX_CTL(PIPE_A));
> +
>  		temp = I915_READ(DDI_BUF_CTL(PORT_E));
>  		temp &= ~DDI_BUF_CTL_ENABLE;
>  		I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
> @@ -643,10 +647,6 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
>  
>  		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
>  
> -		rx_ctl_val &= ~FDI_RX_ENABLE;
> -		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> -		POSTING_READ(FDI_RX_CTL(PIPE_A));
> -
>  		/* Reset FDI_RX_MISC pwrdn lanes */
>  		temp = I915_READ(FDI_RX_MISC(PIPE_A));
>  		temp &= ~(FDI_RX_PWRDN_LANE1_MASK |
> FDI_RX_PWRDN_LANE0_MASK);
> @@ -3078,12 +3078,18 @@ void intel_ddi_fdi_disable(struct drm_crtc
> *crtc)
>  	struct intel_encoder *intel_encoder =
> intel_ddi_get_crtc_encoder(crtc);
>  	uint32_t val;
>  
> -	intel_ddi_post_disable(intel_encoder);
> -
> +	/*
> +	 * Bspec lists this as both step 13 (before DDI_BUF_CTL
> disable)
> +	 * and step 18 (after clearing PORT_CLK_SEL). Based on a
> BUN,
> +	 * step 13 is the correct place for it. Step 18 is where it
> was
> +	 * originally before the BUN.
> +	 */
>  	val = I915_READ(FDI_RX_CTL(PIPE_A));
>  	val &= ~FDI_RX_ENABLE;
>  	I915_WRITE(FDI_RX_CTL(PIPE_A), val);
>  
> +	intel_ddi_post_disable(intel_encoder);
> +
>  	val = I915_READ(FDI_RX_MISC(PIPE_A));
>  	val &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
>  	val |= FDI_RX_PWRDN_LANE1_VAL(2) |
> FDI_RX_PWRDN_LANE0_VAL(2);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Disable FDI RX before DDI_BUF_CTL

[Ville Syrjälä]

On Wed, Mar 23, 2016 at 09:14:34PM +0000, Zanoni, Paulo R wrote:
> Em Ter, 2016-03-01 às 16:16 +0200, ville.syrjala@linux.intel.com
> escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Bspec is confused w.r.t. the HSW/BDW FDI disable sequence. It lists
> > FDI RX disable both as step 13 and step 18 in the sequence. But I dug
> > up an old BUN mail from Art that moved the FDI RX disable to happen
> > before DDI_BUF_CTL disable. That BUN did not renumber the steps and
> > just
> > added a note:
> > "Workaround: Disable PCH FDI Receiver before disabling DDI_BUF_CTL."
> > 
> > The BUN described the symptoms of the fixed issue as:
> > "PCH display underflow and a black screen on the analog CRT port that
> > happened after a FDI re-train"
> > 
> > I suppose later someone tried to renumber the steps to match, but
> > forgot
> > to remove the FDI RX disable from its old position in the sequence.
> > 
> > They also forgot to update the note describing what should be done in
> > case of an FDI training failure. Currently it says:
> > "To retry FDI training, follow the Disable Sequence steps to Disable
> > FDI,
> > but skip the steps related to clocks and PLLs (16, 19, and 20), ..."
> > 
> > It should really say "17, 20, and 21" with the current sequence
> > because
> > those are the steps that deal with PLLs and whatnot, after step 13
> > became
> > FDI RX disable. And had the step 18 FDI RX disable been removed, as I
> > suspect it should have, the note should actually say "17, 19, and
> > 20".
> > 
> > So, let's move the FDI RX disable to happen before DDI_BUF_CTL
> > disable,
> > as that would appear to be the correct order based on the BUN.
> > 
> > Note that Art has since unconfused the spec, and so this patch should
> > now match the steps listed in the spec.
> 
> The sentence above basically says: "forget all the previous paragraphs
> of this commit message since they're just history and go read BSpec
> since it's now correct" :)

Hmm, yeah I guess I was a bit lazy here. I suppose rewriting it a bit
would be warranted. Maybe something like this?

"HSW/BDW FDI disable sequence was revised such that FDI RX should
be disabled already before disabling DDI_BUF_CTL. Let's do that.

Note that the numbering of the FDI disable sequence steps in Bspec
was confusing for the longest time. Basically the numbering was only
partially updated to account for the new sequence, and thus some
parts of the text still referred to things by the old numbers.
Art has fixed that up, and the sequence is now clear."

I could toss in a References: to this mail thread in case someone is
more interested in the acheological details.


One slight concern is that the PRMs aren't uptodate. The HSW one
has the BUN w/a note without the renumbering so it's actually
technically correct. The BDW one has the renumbering which means
it has the incorrect note about which steps to skip the retrying
the FDI training. Rodrigo, do we have some way to get that refreshed?

> 
> > 
> > v2: Add a note that the spec is now correct
> 
> With or without changes:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 
> > 
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Cc: Art Runyan <arthur.j.runyan@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 21a9b83f3bfc..fc4ca50f7159 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -629,6 +629,10 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
> >  			break;
> >  		}
> >  
> > +		rx_ctl_val &= ~FDI_RX_ENABLE;
> > +		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> > +		POSTING_READ(FDI_RX_CTL(PIPE_A));
> > +
> >  		temp = I915_READ(DDI_BUF_CTL(PORT_E));
> >  		temp &= ~DDI_BUF_CTL_ENABLE;
> >  		I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
> > @@ -643,10 +647,6 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
> >  
> >  		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
> >  
> > -		rx_ctl_val &= ~FDI_RX_ENABLE;
> > -		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> > -		POSTING_READ(FDI_RX_CTL(PIPE_A));
> > -
> >  		/* Reset FDI_RX_MISC pwrdn lanes */
> >  		temp = I915_READ(FDI_RX_MISC(PIPE_A));
> >  		temp &= ~(FDI_RX_PWRDN_LANE1_MASK |
> > FDI_RX_PWRDN_LANE0_MASK);
> > @@ -3078,12 +3078,18 @@ void intel_ddi_fdi_disable(struct drm_crtc
> > *crtc)
> >  	struct intel_encoder *intel_encoder =
> > intel_ddi_get_crtc_encoder(crtc);
> >  	uint32_t val;
> >  
> > -	intel_ddi_post_disable(intel_encoder);
> > -
> > +	/*
> > +	 * Bspec lists this as both step 13 (before DDI_BUF_CTL
> > disable)
> > +	 * and step 18 (after clearing PORT_CLK_SEL). Based on a
> > BUN,
> > +	 * step 13 is the correct place for it. Step 18 is where it
> > was
> > +	 * originally before the BUN.
> > +	 */
> >  	val = I915_READ(FDI_RX_CTL(PIPE_A));
> >  	val &= ~FDI_RX_ENABLE;
> >  	I915_WRITE(FDI_RX_CTL(PIPE_A), val);
> >  
> > +	intel_ddi_post_disable(intel_encoder);
> > +
> >  	val = I915_READ(FDI_RX_MISC(PIPE_A));
> >  	val &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK);
> >  	val |= FDI_RX_PWRDN_LANE1_VAL(2) |
> > FDI_RX_PWRDN_LANE0_VAL(2);

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Disable FDI RX before DDI_BUF_CTL

[Zanoni, Paulo R]

Em Qua, 2016-03-23 às 23:57 +0200, Ville Syrjälä escreveu:
> On Wed, Mar 23, 2016 at 09:14:34PM +0000, Zanoni, Paulo R wrote:
> > 
> > Em Ter, 2016-03-01 às 16:16 +0200, ville.syrjala@linux.intel.com
> > escreveu:
> > > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Bspec is confused w.r.t. the HSW/BDW FDI disable sequence. It
> > > lists
> > > FDI RX disable both as step 13 and step 18 in the sequence. But I
> > > dug
> > > up an old BUN mail from Art that moved the FDI RX disable to
> > > happen
> > > before DDI_BUF_CTL disable. That BUN did not renumber the steps
> > > and
> > > just
> > > added a note:
> > > "Workaround: Disable PCH FDI Receiver before disabling
> > > DDI_BUF_CTL."
> > > 
> > > The BUN described the symptoms of the fixed issue as:
> > > "PCH display underflow and a black screen on the analog CRT port
> > > that
> > > happened after a FDI re-train"
> > > 
> > > I suppose later someone tried to renumber the steps to match, but
> > > forgot
> > > to remove the FDI RX disable from its old position in the
> > > sequence.
> > > 
> > > They also forgot to update the note describing what should be
> > > done in
> > > case of an FDI training failure. Currently it says:
> > > "To retry FDI training, follow the Disable Sequence steps to
> > > Disable
> > > FDI,
> > > but skip the steps related to clocks and PLLs (16, 19, and 20),
> > > ..."
> > > 
> > > It should really say "17, 20, and 21" with the current sequence
> > > because
> > > those are the steps that deal with PLLs and whatnot, after step
> > > 13
> > > became
> > > FDI RX disable. And had the step 18 FDI RX disable been removed,
> > > as I
> > > suspect it should have, the note should actually say "17, 19, and
> > > 20".
> > > 
> > > So, let's move the FDI RX disable to happen before DDI_BUF_CTL
> > > disable,
> > > as that would appear to be the correct order based on the BUN.
> > > 
> > > Note that Art has since unconfused the spec, and so this patch
> > > should
> > > now match the steps listed in the spec.
> > The sentence above basically says: "forget all the previous
> > paragraphs
> > of this commit message since they're just history and go read BSpec
> > since it's now correct" :)
> Hmm, yeah I guess I was a bit lazy here. I suppose rewriting it a bit
> would be warranted. Maybe something like this?
> 
> "HSW/BDW FDI disable sequence was revised such that FDI RX should
> be disabled already before disabling DDI_BUF_CTL. Let's do that.
> 
> Note that the numbering of the FDI disable sequence steps in Bspec
> was confusing for the longest time. Basically the numbering was only
> partially updated to account for the new sequence, and thus some
> parts of the text still referred to things by the old numbers.
> Art has fixed that up, and the sequence is now clear."
> 
> I could toss in a References: to this mail thread in case someone is
> more interested in the acheological details.
> 
> 
> One slight concern is that the PRMs aren't uptodate. The HSW one
> has the BUN w/a note without the renumbering so it's actually
> technically correct. The BDW one has the renumbering which means
> it has the incorrect note about which steps to skip the retrying
> the FDI training. Rodrigo, do we have some way to get that refreshed?

This would be an argument in favor of keeping your old commit message,
actually (or writing a third version).

The R-B stands both with the new and the old message, so I'll just
trust you'll decide whatever seems better, even if you decide to change
again, so feel free to merge.

> 
> > 
> > 
> > > 
> > > 
> > > v2: Add a note that the spec is now correct
> > With or without changes:
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > 
> > > 
> > > 
> > > Cc: Paulo Zanoni <przanoni@gmail.com>
> > > Cc: Art Runyan <arthur.j.runyan@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 21a9b83f3bfc..fc4ca50f7159 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -629,6 +629,10 @@ void hsw_fdi_link_train(struct drm_crtc
> > > *crtc)
> > >  			break;
> > >  		}
> > >  
> > > +		rx_ctl_val &= ~FDI_RX_ENABLE;
> > > +		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> > > +		POSTING_READ(FDI_RX_CTL(PIPE_A));
> > > +
> > >  		temp = I915_READ(DDI_BUF_CTL(PORT_E));
> > >  		temp &= ~DDI_BUF_CTL_ENABLE;
> > >  		I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
> > > @@ -643,10 +647,6 @@ void hsw_fdi_link_train(struct drm_crtc
> > > *crtc)
> > >  
> > >  		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
> > >  
> > > -		rx_ctl_val &= ~FDI_RX_ENABLE;
> > > -		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> > > -		POSTING_READ(FDI_RX_CTL(PIPE_A));
> > > -
> > >  		/* Reset FDI_RX_MISC pwrdn lanes */
> > >  		temp = I915_READ(FDI_RX_MISC(PIPE_A));
> > >  		temp &= ~(FDI_RX_PWRDN_LANE1_MASK |
> > > FDI_RX_PWRDN_LANE0_MASK);
> > > @@ -3078,12 +3078,18 @@ void intel_ddi_fdi_disable(struct
> > > drm_crtc
> > > *crtc)
> > >  	struct intel_encoder *intel_encoder =
> > > intel_ddi_get_crtc_encoder(crtc);
> > >  	uint32_t val;
> > >  
> > > -	intel_ddi_post_disable(intel_encoder);
> > > -
> > > +	/*
> > > +	 * Bspec lists this as both step 13 (before DDI_BUF_CTL
> > > disable)
> > > +	 * and step 18 (after clearing PORT_CLK_SEL). Based on a
> > > BUN,
> > > +	 * step 13 is the correct place for it. Step 18 is where
> > > it
> > > was
> > > +	 * originally before the BUN.
> > > +	 */
> > >  	val = I915_READ(FDI_RX_CTL(PIPE_A));
> > >  	val &= ~FDI_RX_ENABLE;
> > >  	I915_WRITE(FDI_RX_CTL(PIPE_A), val);
> > >  
> > > +	intel_ddi_post_disable(intel_encoder);
> > > +
> > >  	val = I915_READ(FDI_RX_MISC(PIPE_A));
> > >  	val &= ~(FDI_RX_PWRDN_LANE1_MASK |
> > > FDI_RX_PWRDN_LANE0_MASK);
> > >  	val |= FDI_RX_PWRDN_LANE1_VAL(2) |
> > > FDI_RX_PWRDN_LANE0_VAL(2);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Disable FDI RX before DDI_BUF_CTL

[Ville Syrjälä]

On Wed, Mar 23, 2016 at 10:58:21PM +0000, Zanoni, Paulo R wrote:
> Em Qua, 2016-03-23 às 23:57 +0200, Ville Syrjälä escreveu:
> > On Wed, Mar 23, 2016 at 09:14:34PM +0000, Zanoni, Paulo R wrote:
> > > 
> > > Em Ter, 2016-03-01 às 16:16 +0200, ville.syrjala@linux.intel.com
> > > escreveu:
> > > > 
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Bspec is confused w.r.t. the HSW/BDW FDI disable sequence. It
> > > > lists
> > > > FDI RX disable both as step 13 and step 18 in the sequence. But I
> > > > dug
> > > > up an old BUN mail from Art that moved the FDI RX disable to
> > > > happen
> > > > before DDI_BUF_CTL disable. That BUN did not renumber the steps
> > > > and
> > > > just
> > > > added a note:
> > > > "Workaround: Disable PCH FDI Receiver before disabling
> > > > DDI_BUF_CTL."
> > > > 
> > > > The BUN described the symptoms of the fixed issue as:
> > > > "PCH display underflow and a black screen on the analog CRT port
> > > > that
> > > > happened after a FDI re-train"
> > > > 
> > > > I suppose later someone tried to renumber the steps to match, but
> > > > forgot
> > > > to remove the FDI RX disable from its old position in the
> > > > sequence.
> > > > 
> > > > They also forgot to update the note describing what should be
> > > > done in
> > > > case of an FDI training failure. Currently it says:
> > > > "To retry FDI training, follow the Disable Sequence steps to
> > > > Disable
> > > > FDI,
> > > > but skip the steps related to clocks and PLLs (16, 19, and 20),
> > > > ..."
> > > > 
> > > > It should really say "17, 20, and 21" with the current sequence
> > > > because
> > > > those are the steps that deal with PLLs and whatnot, after step
> > > > 13
> > > > became
> > > > FDI RX disable. And had the step 18 FDI RX disable been removed,
> > > > as I
> > > > suspect it should have, the note should actually say "17, 19, and
> > > > 20".
> > > > 
> > > > So, let's move the FDI RX disable to happen before DDI_BUF_CTL
> > > > disable,
> > > > as that would appear to be the correct order based on the BUN.
> > > > 
> > > > Note that Art has since unconfused the spec, and so this patch
> > > > should
> > > > now match the steps listed in the spec.
> > > The sentence above basically says: "forget all the previous
> > > paragraphs
> > > of this commit message since they're just history and go read BSpec
> > > since it's now correct" :)
> > Hmm, yeah I guess I was a bit lazy here. I suppose rewriting it a bit
> > would be warranted. Maybe something like this?
> > 
> > "HSW/BDW FDI disable sequence was revised such that FDI RX should
> > be disabled already before disabling DDI_BUF_CTL. Let's do that.
> > 
> > Note that the numbering of the FDI disable sequence steps in Bspec
> > was confusing for the longest time. Basically the numbering was only
> > partially updated to account for the new sequence, and thus some
> > parts of the text still referred to things by the old numbers.
> > Art has fixed that up, and the sequence is now clear."
> > 
> > I could toss in a References: to this mail thread in case someone is
> > more interested in the acheological details.
> > 
> > 
> > One slight concern is that the PRMs aren't uptodate. The HSW one
> > has the BUN w/a note without the renumbering so it's actually
> > technically correct. The BDW one has the renumbering which means
> > it has the incorrect note about which steps to skip the retrying
> > the FDI training. Rodrigo, do we have some way to get that refreshed?
> 
> This would be an argument in favor of keeping your old commit message,
> actually (or writing a third version).
> 
> The R-B stands both with the new and the old message, so I'll just
> trust you'll decide whatever seems better, even if you decide to change
> again, so feel free to merge.

OK, so it's fairly clear by now that I can't come up with anything
good here, so I just went with the original commit v2 message.

Pushed to dinq. Thanks for the review.

> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > v2: Add a note that the spec is now correct
> > > With or without changes:
> > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > 
> > > > 
> > > > 
> > > > Cc: Paulo Zanoni <przanoni@gmail.com>
> > > > Cc: Art Runyan <arthur.j.runyan@intel.com>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++------
> > > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index 21a9b83f3bfc..fc4ca50f7159 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -629,6 +629,10 @@ void hsw_fdi_link_train(struct drm_crtc
> > > > *crtc)
> > > >  			break;
> > > >  		}
> > > >  
> > > > +		rx_ctl_val &= ~FDI_RX_ENABLE;
> > > > +		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> > > > +		POSTING_READ(FDI_RX_CTL(PIPE_A));
> > > > +
> > > >  		temp = I915_READ(DDI_BUF_CTL(PORT_E));
> > > >  		temp &= ~DDI_BUF_CTL_ENABLE;
> > > >  		I915_WRITE(DDI_BUF_CTL(PORT_E), temp);
> > > > @@ -643,10 +647,6 @@ void hsw_fdi_link_train(struct drm_crtc
> > > > *crtc)
> > > >  
> > > >  		intel_wait_ddi_buf_idle(dev_priv, PORT_E);
> > > >  
> > > > -		rx_ctl_val &= ~FDI_RX_ENABLE;
> > > > -		I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
> > > > -		POSTING_READ(FDI_RX_CTL(PIPE_A));
> > > > -
> > > >  		/* Reset FDI_RX_MISC pwrdn lanes */
> > > >  		temp = I915_READ(FDI_RX_MISC(PIPE_A));
> > > >  		temp &= ~(FDI_RX_PWRDN_LANE1_MASK |
> > > > FDI_RX_PWRDN_LANE0_MASK);
> > > > @@ -3078,12 +3078,18 @@ void intel_ddi_fdi_disable(struct
> > > > drm_crtc
> > > > *crtc)
> > > >  	struct intel_encoder *intel_encoder =
> > > > intel_ddi_get_crtc_encoder(crtc);
> > > >  	uint32_t val;
> > > >  
> > > > -	intel_ddi_post_disable(intel_encoder);
> > > > -
> > > > +	/*
> > > > +	 * Bspec lists this as both step 13 (before DDI_BUF_CTL
> > > > disable)
> > > > +	 * and step 18 (after clearing PORT_CLK_SEL). Based on a
> > > > BUN,
> > > > +	 * step 13 is the correct place for it. Step 18 is where
> > > > it
> > > > was
> > > > +	 * originally before the BUN.
> > > > +	 */
> > > >  	val = I915_READ(FDI_RX_CTL(PIPE_A));
> > > >  	val &= ~FDI_RX_ENABLE;
> > > >  	I915_WRITE(FDI_RX_CTL(PIPE_A), val);
> > > >  
> > > > +	intel_ddi_post_disable(intel_encoder);
> > > > +
> > > >  	val = I915_READ(FDI_RX_MISC(PIPE_A));
> > > >  	val &= ~(FDI_RX_PWRDN_LANE1_MASK |
> > > > FDI_RX_PWRDN_LANE0_MASK);
> > > >  	val |= FDI_RX_PWRDN_LANE1_VAL(2) |
> > > > FDI_RX_PWRDN_LANE0_VAL(2);

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx