[Intel-gfx] [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 at linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala at 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 at intel.com>


> 
> Cc: Paulo Zanoni <przanoni at gmail.com>
> Cc: Art Runyan <arthur.j.runyan at intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala at 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);