From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH] drm/i915: CPT/PPT pch dp transcoder workaround Date: Thu, 1 Nov 2012 07:37:36 -0700 Message-ID: <20121101073736.49cc9c43@jbarnes-desktop> References: <1351720351-2688-4-git-send-email-daniel.vetter@ffwll.ch> <1351757730-11463-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from oproxy6-pub.bluehost.com (oproxy6-pub.bluehost.com [67.222.54.6]) by gabe.freedesktop.org (Postfix) with SMTP id 821D09E75A for ; Thu, 1 Nov 2012 07:37:27 -0700 (PDT) In-Reply-To: <1351757730-11463-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: Intel Graphics Development , Arthur Ranyan List-Id: intel-gfx@lists.freedesktop.org On Thu, 1 Nov 2012 09:15:30 +0100 Daniel Vetter wrote: > We need to set the timing override chicken bit after fdi link training > has completed and before we enable the dp transcoder. We also have to > clear that bit again after disabling the pch dp transcoder. > > See "Graphics BSpec: vol4g North Display Engine Registers [IVB], > Display Mode Set Sequence" and "Graphics BSpec: vol4h South Display > Engine Registers [CPT, PPT], South Display Engine Transcoder and FDI > Control, Transcoder Debug and DFT, TRANS_CHICKEN_2" bit 31: > > "Workaround : Enable the override prior to enabling the transcoder. > Disable the override after disabling the transcoder." > > While at it, use the _PIPE macro for the other TRANS_DP register. > > v2: Keep the w/a as-is, but kill the original (but wrongly placed) > workaround introduced in > > commit 3bcf603f6d5d18bd9d076dc280de71f48add4101 > Author: Jesse Barnes > Date: Wed Jul 27 11:51:40 2011 -0700 > > drm/i915: apply timing generator bug workaround on CPT and PPT > > and > > commit d4270e57efe9e2536798c59e1ed2fd0a1e5cdfcf > Author: Jesse Barnes > Date: Tue Oct 11 10:43:02 2011 -0700 > > drm/i915: export a CPT mode set verification function > > Note that this old code has unconditionally set the w/a, which might > explain why fdi link training sometimes silently fails, and especially > why the auto-train did not seem to work properly. > > v3: Paulo Zanoni pointed out that this workaround is also required on > the LPT PCH. And Arthur Ranyan confirmed that this workaround is > requierd for all ports on the pch, not just DP: The important part > is that the bit is set whenever the pch transcoder is enabled, and > that it is _not_ set while the fdi link is trained. It is also > important that the pch transcoder is fully disabled, i.e. we have to > wait for bit 30 to clear before clearing the w/a bit. > > Hence move to workaround into enable/disable_transcoder, where the pch > transcoder gets enabled/disabled. > > v4: Whitespace changes dropped. > > v5: Don't run the w/a on IBX, we only need it on CPT/PPT and LPT. > > Cc: Jesse Barnes > Cc: Paulo Zanoni > Cc: Arthur Ranyan > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_reg.h | 5 +++-- > drivers/gpu/drm/i915/intel_display.c | 32 +++++++++++++++++++++++--------- > drivers/gpu/drm/i915/intel_pm.c | 4 ---- > 3 files changed, 26 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 2dd880f..f1fe3a0 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3806,7 +3806,8 @@ > #define _TRANSA_CHICKEN2 0xf0064 > #define _TRANSB_CHICKEN2 0xf1064 > #define TRANS_CHICKEN2(pipe) _PIPE(pipe, _TRANSA_CHICKEN2, _TRANSB_CHICKEN2) > -#define TRANS_AUTOTRAIN_GEN_STALL_DIS (1<<31) > +#define TRANS_CHICKEN2_TIMING_OVERRIDE (1<<31) > + > > #define SOUTH_CHICKEN1 0xc2000 > #define FDIA_PHASE_SYNC_SHIFT_OVR 19 > @@ -4064,7 +4065,7 @@ > #define TRANS_DP_CTL_A 0xe0300 > #define TRANS_DP_CTL_B 0xe1300 > #define TRANS_DP_CTL_C 0xe2300 > -#define TRANS_DP_CTL(pipe) (TRANS_DP_CTL_A + (pipe) * 0x01000) > +#define TRANS_DP_CTL(pipe) _PIPE(pipe, TRANS_DP_CTL_A, TRANS_DP_CTL_B) > #define TRANS_DP_OUTPUT_ENABLE (1<<31) > #define TRANS_DP_PORT_SEL_B (0<<29) > #define TRANS_DP_PORT_SEL_C (1<<29) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 129059b..675079a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -1673,9 +1673,9 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc) > static void intel_enable_transcoder(struct drm_i915_private *dev_priv, > enum pipe pipe) > { > - int reg; > - u32 val, pipeconf_val; > + struct drm_device *dev = dev_priv->dev; > struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > + uint32_t reg, val, pipeconf_val; > > /* PCH only available on ILK+ */ > BUG_ON(dev_priv->info->gen < 5); > @@ -1693,6 +1693,16 @@ static void intel_enable_transcoder(struct drm_i915_private *dev_priv, > DRM_ERROR("Attempting to enable transcoder on Haswell with pipe > 0\n"); > return; > } > + > + if (!HAS_PCH_IBX(dev)) { > + /* Workaround: Set the timing override bit before enabling the > + * pch transcoder. */ > + reg = TRANS_CHICKEN2(pipe); > + val = I915_READ(reg); > + val |= TRANS_CHICKEN2_TIMING_OVERRIDE; > + I915_WRITE(reg, val); > + } I'd like this better if it were HAS_PCH_CPT; we use that as a synonym for PPT elsehwere, and it shouldn't apply to LPT right? I see LPT has the bit, but I don't know if it's needed (the changelong and summary are misleading if so). > + > reg = TRANSCONF(pipe); > val = I915_READ(reg); > pipeconf_val = I915_READ(PIPECONF(pipe)); > @@ -1724,8 +1734,8 @@ static void intel_enable_transcoder(struct drm_i915_private *dev_priv, > static void intel_disable_transcoder(struct drm_i915_private *dev_priv, > enum pipe pipe) > { > - int reg; > - u32 val; > + struct drm_device *dev = dev_priv->dev; > + uint32_t reg, val; > > /* FDI relies on the transcoder */ > assert_fdi_tx_disabled(dev_priv, pipe); > @@ -1741,6 +1751,14 @@ static void intel_disable_transcoder(struct drm_i915_private *dev_priv, > /* wait for PCH transcoder off, transcoder state */ > if (wait_for((I915_READ(reg) & TRANS_STATE_ENABLE) == 0, 50)) > DRM_ERROR("failed to disable transcoder %d\n", pipe); > + > + if (!HAS_PCH_IBX(dev)) { > + /* Workaround: Clear the timing override chicken bit again. */ > + reg = TRANS_CHICKEN2(pipe); > + val = I915_READ(reg); > + val &= ~TRANS_CHICKEN2_TIMING_OVERRIDE; > + I915_WRITE(reg, val); > + } > } > > /** > @@ -3217,16 +3235,12 @@ prepare: /* separate function? */ > void intel_cpt_verify_modeset(struct drm_device *dev, int pipe) > { > struct drm_i915_private *dev_priv = dev->dev_private; > - int dslreg = PIPEDSL(pipe), tc2reg = TRANS_CHICKEN2(pipe); > + int dslreg = PIPEDSL(pipe); > u32 temp; > > temp = I915_READ(dslreg); > udelay(500); > if (wait_for(I915_READ(dslreg) != temp, 5)) { > - /* Without this, mode sets may fail silently on FDI */ > - I915_WRITE(tc2reg, TRANS_AUTOTRAIN_GEN_STALL_DIS); > - udelay(250); > - I915_WRITE(tc2reg, 0); > if (wait_for(I915_READ(dslreg) != temp, 5)) > DRM_ERROR("mode set failed: pipe %d stuck\n", pipe); > } Note this was an older version of the workaround. "wrongly placed" indeed. -- Jesse Barnes, Intel Open Source Technology Center