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

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

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