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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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