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@xxxxxxxxxxxxxxx] > >Sent: Wednesday, February 17, 2016 9:37 AM > >To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > >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@xxxxxxxxxxxxxxx wrote: > >> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >> > >> 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@xxxxxxxxx> > >> Cc: Art Runyan <arthur.j.runyan@xxxxxxxxx> > >> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >> --- > >> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
