| Message ID | 1354195773-4022-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 29, 2012 at 11:29:31AM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > We were previously doing exactly what the "mode set sequence for CRT" > document mandates, but whenever we failed to train the link in the > first tentative, all the other subsequent retries always failed. In > one of my monitors that has 47 modes, I was usually getting around 3 > failures when running "testdisplay -a". > > After this patch, even if we fail in the first tentative, we can > succeed in the next ones. So now when running "testdisplay -a" I see > around 3 times the message "FDI link training done on step 1" and no > failures. > > Notice that now the "retry" code looks a lot like the DP retry code. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 43 +++++++++++++++++++++++++--------------- > 1 file changed, 27 insertions(+), 16 deletions(-) > > I'd like this to go to 3.8 somehow. > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 852012b..3264cb4 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -138,6 +138,19 @@ static const long hsw_ddi_buf_ctl_values[] = { > DDI_BUF_EMP_800MV_3_5DB_HSW > }; > > +static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, > + enum port port) > +{ > + uint32_t reg = DDI_BUF_CTL(port); > + int i; > + > + for (i = 0; i < 8; i++) { > + udelay(1); > + if (I915_READ(reg) & DDI_BUF_IS_IDLE) > + return; > + } > + DRM_ERROR("Timeout waiting for DDI BUF %c idle bit\n", port_name(port)); > +} > > /* Starting with Haswell, different DDI ports can work in FDI mode for > * connection to the PCH-located connectors. For this, it is necessary to train > @@ -231,18 +244,30 @@ void hsw_fdi_link_train(struct drm_crtc *crtc) > return; > } > > + temp = I915_READ(DDI_BUF_CTL(PORT_E)); > + temp &= ~DDI_BUF_CTL_ENABLE; > + I915_WRITE(DDI_BUF_CTL(PORT_E), temp); > + POSTING_READ(DDI_BUF_CTL(PORT_E)); > + > /* Disable DP_TP_CTL and FDI_RX_CTL and retry */ > - I915_WRITE(DP_TP_CTL(PORT_E), > - I915_READ(DP_TP_CTL(PORT_E)) & ~DP_TP_CTL_ENABLE); > + temp = I915_READ(DP_TP_CTL(PORT_E)); > + temp &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK); > + temp |= DP_TP_CTL_LINK_TRAIN_PAT1; > + I915_WRITE(DP_TP_CTL(PORT_E), temp); > + POSTING_READ(DP_TP_CTL(PORT_E)); > + > + intel_wait_ddi_buf_idle(dev_priv, PORT_E); > > rx_ctl_val &= ~FDI_RX_ENABLE; > I915_WRITE(_FDI_RXA_CTL, rx_ctl_val); > + POSTING_READ(_FDI_RXA_CTL); > > /* Reset FDI_RX_MISC pwrdn lanes */ > temp = I915_READ(_FDI_RXA_MISC); > temp &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK); > temp |= FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2); > I915_WRITE(_FDI_RXA_MISC, temp); > + POSTING_READ(_FDI_RXA_MISC); What now slightly irks me here is that this sequence and the one in intel_ddi_fdi_disable don't match exactly. Imo it would make sense to have both the same (after all, we disable the same piece of hw) - have you tried that out (there's obviously some slight unification required first)? -Daniel > } > > DRM_ERROR("FDI link training failed!\n"); > @@ -1222,20 +1247,6 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder) > } > } > > -static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, > - enum port port) > -{ > - uint32_t reg = DDI_BUF_CTL(port); > - int i; > - > - for (i = 0; i < 8; i++) { > - udelay(1); > - if (I915_READ(reg) & DDI_BUF_IS_IDLE) > - return; > - } > - DRM_ERROR("Timeout waiting for DDI BUF %c idle bit\n", port_name(port)); > -} > - > static void intel_ddi_post_disable(struct intel_encoder *intel_encoder) > { > struct drm_encoder *encoder = &intel_encoder->base; > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi 2012/11/29 Daniel Vetter <daniel@ffwll.ch>: > On Thu, Nov 29, 2012 at 11:29:31AM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> We were previously doing exactly what the "mode set sequence for CRT" >> document mandates, but whenever we failed to train the link in the >> first tentative, all the other subsequent retries always failed. In >> one of my monitors that has 47 modes, I was usually getting around 3 >> failures when running "testdisplay -a". >> >> After this patch, even if we fail in the first tentative, we can >> succeed in the next ones. So now when running "testdisplay -a" I see >> around 3 times the message "FDI link training done on step 1" and no >> failures. >> >> Notice that now the "retry" code looks a lot like the DP retry code. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 43 +++++++++++++++++++++++++--------------- >> 1 file changed, 27 insertions(+), 16 deletions(-) >> >> I'd like this to go to 3.8 somehow. >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index 852012b..3264cb4 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -138,6 +138,19 @@ static const long hsw_ddi_buf_ctl_values[] = { >> DDI_BUF_EMP_800MV_3_5DB_HSW >> }; >> >> +static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, >> + enum port port) >> +{ >> + uint32_t reg = DDI_BUF_CTL(port); >> + int i; >> + >> + for (i = 0; i < 8; i++) { >> + udelay(1); >> + if (I915_READ(reg) & DDI_BUF_IS_IDLE) >> + return; >> + } >> + DRM_ERROR("Timeout waiting for DDI BUF %c idle bit\n", port_name(port)); >> +} >> >> /* Starting with Haswell, different DDI ports can work in FDI mode for >> * connection to the PCH-located connectors. For this, it is necessary to train >> @@ -231,18 +244,30 @@ void hsw_fdi_link_train(struct drm_crtc *crtc) >> return; >> } >> >> + temp = I915_READ(DDI_BUF_CTL(PORT_E)); >> + temp &= ~DDI_BUF_CTL_ENABLE; >> + I915_WRITE(DDI_BUF_CTL(PORT_E), temp); >> + POSTING_READ(DDI_BUF_CTL(PORT_E)); >> + >> /* Disable DP_TP_CTL and FDI_RX_CTL and retry */ >> - I915_WRITE(DP_TP_CTL(PORT_E), >> - I915_READ(DP_TP_CTL(PORT_E)) & ~DP_TP_CTL_ENABLE); >> + temp = I915_READ(DP_TP_CTL(PORT_E)); >> + temp &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK); >> + temp |= DP_TP_CTL_LINK_TRAIN_PAT1; >> + I915_WRITE(DP_TP_CTL(PORT_E), temp); >> + POSTING_READ(DP_TP_CTL(PORT_E)); >> + >> + intel_wait_ddi_buf_idle(dev_priv, PORT_E); >> >> rx_ctl_val &= ~FDI_RX_ENABLE; >> I915_WRITE(_FDI_RXA_CTL, rx_ctl_val); >> + POSTING_READ(_FDI_RXA_CTL); >> >> /* Reset FDI_RX_MISC pwrdn lanes */ >> temp = I915_READ(_FDI_RXA_MISC); >> temp &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK); >> temp |= FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2); >> I915_WRITE(_FDI_RXA_MISC, temp); >> + POSTING_READ(_FDI_RXA_MISC); > > What now slightly irks me here is that this sequence and the one in > intel_ddi_fdi_disable don't match exactly. Imo it would make sense to have > both the same (after all, we disable the same piece of hw) - have you > tried that out (there's obviously some slight unification required first)? The "FDI retrain" and "FDI disable" sequences don't match, just like the "DP retrain" and "DP disable" sequences also don't match. One is a full disable, the other is just a link retrain in the middle of the enable sequence, disabling only what's needed. We're following the spec for the disable sequences and we're also following the spec for the "retrain" sequence (except for the lack of the "disable DDI_BUF_CTL" instruction introduced by this patch). What do you suggest to change? > -Daniel > >> } >> >> DRM_ERROR("FDI link training failed!\n"); >> @@ -1222,20 +1247,6 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder) >> } >> } >> >> -static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, >> - enum port port) >> -{ >> - uint32_t reg = DDI_BUF_CTL(port); >> - int i; >> - >> - for (i = 0; i < 8; i++) { >> - udelay(1); >> - if (I915_READ(reg) & DDI_BUF_IS_IDLE) >> - return; >> - } >> - DRM_ERROR("Timeout waiting for DDI BUF %c idle bit\n", port_name(port)); >> -} >> - >> static void intel_ddi_post_disable(struct intel_encoder *intel_encoder) >> { >> struct drm_encoder *encoder = &intel_encoder->base; >> -- >> 1.7.11.7 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Fri, Nov 30, 2012 at 4:39 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > The "FDI retrain" and "FDI disable" sequences don't match, just like > the "DP retrain" and "DP disable" sequences also don't match. One is a > full disable, the other is just a link retrain in the middle of the > enable sequence, disabling only what's needed. We're following the > spec for the disable sequences and we're also following the spec for > the "retrain" sequence (except for the lack of the "disable > DDI_BUF_CTL" instruction introduced by this patch). What do you > suggest to change? It's just that I've sacrified too many kittens yesterday trying to get fdi link training to work on ivb (to no avail), so when I see too much magic code I start to freak out. And on a quick hunch I don't see why disabling fdi when the link train fails or when we disable the thing for other reasons should be different. But I've you're convinced that this is the right thing, I'm ok. -Daniel
On Thu, Nov 29, 2012 at 11:29:31AM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > We were previously doing exactly what the "mode set sequence for CRT" > document mandates, but whenever we failed to train the link in the > first tentative, all the other subsequent retries always failed. In > one of my monitors that has 47 modes, I was usually getting around 3 > failures when running "testdisplay -a". > > After this patch, even if we fail in the first tentative, we can > succeed in the next ones. So now when running "testdisplay -a" I see > around 3 times the message "FDI link training done on step 1" and no > failures. > > Notice that now the "retry" code looks a lot like the DP retry code. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Picked up for -fixes, thanks for the patch. Upon re-reading it still irks me midly that the disable sequence here matches the begining of ddi_post_disable. Whatever. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 852012b..3264cb4 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -138,6 +138,19 @@ static const long hsw_ddi_buf_ctl_values[] = { DDI_BUF_EMP_800MV_3_5DB_HSW }; +static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, + enum port port) +{ + uint32_t reg = DDI_BUF_CTL(port); + int i; + + for (i = 0; i < 8; i++) { + udelay(1); + if (I915_READ(reg) & DDI_BUF_IS_IDLE) + return; + } + DRM_ERROR("Timeout waiting for DDI BUF %c idle bit\n", port_name(port)); +} /* Starting with Haswell, different DDI ports can work in FDI mode for * connection to the PCH-located connectors. For this, it is necessary to train @@ -231,18 +244,30 @@ void hsw_fdi_link_train(struct drm_crtc *crtc) return; } + temp = I915_READ(DDI_BUF_CTL(PORT_E)); + temp &= ~DDI_BUF_CTL_ENABLE; + I915_WRITE(DDI_BUF_CTL(PORT_E), temp); + POSTING_READ(DDI_BUF_CTL(PORT_E)); + /* Disable DP_TP_CTL and FDI_RX_CTL and retry */ - I915_WRITE(DP_TP_CTL(PORT_E), - I915_READ(DP_TP_CTL(PORT_E)) & ~DP_TP_CTL_ENABLE); + temp = I915_READ(DP_TP_CTL(PORT_E)); + temp &= ~(DP_TP_CTL_ENABLE | DP_TP_CTL_LINK_TRAIN_MASK); + temp |= DP_TP_CTL_LINK_TRAIN_PAT1; + I915_WRITE(DP_TP_CTL(PORT_E), temp); + POSTING_READ(DP_TP_CTL(PORT_E)); + + intel_wait_ddi_buf_idle(dev_priv, PORT_E); rx_ctl_val &= ~FDI_RX_ENABLE; I915_WRITE(_FDI_RXA_CTL, rx_ctl_val); + POSTING_READ(_FDI_RXA_CTL); /* Reset FDI_RX_MISC pwrdn lanes */ temp = I915_READ(_FDI_RXA_MISC); temp &= ~(FDI_RX_PWRDN_LANE1_MASK | FDI_RX_PWRDN_LANE0_MASK); temp |= FDI_RX_PWRDN_LANE1_VAL(2) | FDI_RX_PWRDN_LANE0_VAL(2); I915_WRITE(_FDI_RXA_MISC, temp); + POSTING_READ(_FDI_RXA_MISC); } DRM_ERROR("FDI link training failed!\n"); @@ -1222,20 +1247,6 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder) } } -static void intel_wait_ddi_buf_idle(struct drm_i915_private *dev_priv, - enum port port) -{ - uint32_t reg = DDI_BUF_CTL(port); - int i; - - for (i = 0; i < 8; i++) { - udelay(1); - if (I915_READ(reg) & DDI_BUF_IS_IDLE) - return; - } - DRM_ERROR("Timeout waiting for DDI BUF %c idle bit\n", port_name(port)); -} - static void intel_ddi_post_disable(struct intel_encoder *intel_encoder) { struct drm_encoder *encoder = &intel_encoder->base;