[Intel-gfx] [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 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" :)

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 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);

-- 
Ville Syrjälä
Intel OTC