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@xxxxxxxxxxxxxxx > > > escreveu: > > > > > > > > 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. > > > > > > > > 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@xxxxxxxxx> > > > > > > > > > > > > > > > > > > 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 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
