From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: check fdi B/C lane sharing constraint Date: Mon, 29 Oct 2012 18:42:05 +0100 Message-ID: <20121029174205.GP5691@phenom.ffwll.local> References: <1351241899-7870-10-git-send-email-daniel.vetter@ffwll.ch> <1351346320-32740-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 mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 6338A9E9D9 for ; Mon, 29 Oct 2012 10:41:01 -0700 (PDT) Received: by mail-ee0-f49.google.com with SMTP id c1so2298198eek.36 for ; Mon, 29 Oct 2012 10:41:00 -0700 (PDT) Content-Disposition: inline In-Reply-To: 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: Paulo Zanoni Cc: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Mon, Oct 29, 2012 at 10:06:56AM -0200, Paulo Zanoni wrote: > Hi > > 2012/10/27 Daniel Vetter : > > And properly toggle the chicken bit in the pch to enable/disable fdi C > > rx. If we don't set this bit correctly, the rx gets confused in link > > training, which can result in an fdi link that silently fails to train > > the link (since the corresponding register reports success). Note that > > both fdi link B and C can suffer when this bit is not set correctly. > > > > The code as-is has a few deficiencies: > > - We presume all pipes use the pch which is not the case for cpu edp. > > - We don't bother with disabling both pipes when we could make things > > work, e.g. when pipe B switched from 4 to 2 lanes due to a mode > > change, we don't bother updating the w/a bit. > > - It's ugly. > > > > All of these are because we compute ->fdi_lanes way too late, when > > we're already setting up individual pipes. We need to have this > > information in ->modeset_global_resources already, to set things up > > correctly. But that is a much larger reorg of the code. > > > > Note that we actually hit the 2 lanes limit in practice rather > > quickly: Even though the 1920x1200 mode native mode of my screen fits > > into 2 lanes, it needs 3 lanes for the 1920x1080 (since that somehow > > has much more blanking ...). Not obeying this restriction seems to > > results in cute-looking digital noise. > > > > v2: Only ever clear the chicken bit when both pipes are off. > > > > v3: Use the new ->modeset_global_resources callback. > > > > v4: Move the WARNs to the right place. Oh how I hate hacks. > > > > v5: Fix spelling, noticed by Paulo Zanoni. > > > > Signed-off-by: Daniel Vetter > > Reviewed-by: Paulo Zanoni Ok, I've slurped in this patch series, safe for the 2 patches that still need some more work. I'll resend those. I've also included the one follow up patch with a clarifiying comment. Thanks for the review, Daniel > > > --- > > drivers/gpu/drm/i915/i915_reg.h | 5 +- > > drivers/gpu/drm/i915/intel_display.c | 122 +++++++++++++++++++++++++++++++++-- > > 2 files changed, 121 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 84b09ee..f681d01 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3810,8 +3810,9 @@ > > #define SOUTH_CHICKEN1 0xc2000 > > #define FDIA_PHASE_SYNC_SHIFT_OVR 19 > > #define FDIA_PHASE_SYNC_SHIFT_EN 18 > > -#define FDI_PHASE_SYNC_OVR(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_OVR - ((pipe) * 2))) > > -#define FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2))) > > +#define FDI_PHASE_SYNC_OVR(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_OVR - ((pipe) * 2))) > > +#define FDI_PHASE_SYNC_EN(pipe) (1<<(FDIA_PHASE_SYNC_SHIFT_EN - ((pipe) * 2))) > > +#define FDI_BC_BIFURCATION_SELECT (1 << 12) > > #define SOUTH_CHICKEN2 0xc2004 > > #define DPLS_EDP_PPS_FIX_DIS (1<<0) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 43ac99b..befc06c 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2324,6 +2324,29 @@ static void cpt_phase_pointer_enable(struct drm_device *dev, int pipe) > > POSTING_READ(SOUTH_CHICKEN1); > > } > > > > +static void ivb_modeset_global_resources(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + struct intel_crtc *pipe_B_crtc = > > + to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]); > > + struct intel_crtc *pipe_C_crtc = > > + to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_C]); > > + uint32_t temp; > > + > > + /* When everything is off disable fdi C so that we could enable fdi B > > + * with all lanes. XXX: This misses the case where a pipe is not using > > + * any pch resources and so doesn't need any fdi lanes. */ > > + if (!pipe_B_crtc->base.enabled && !pipe_C_crtc->base.enabled) { > > + WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE); > > + WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE); > > + > > + temp = I915_READ(SOUTH_CHICKEN1); > > + temp &= ~FDI_BC_BIFURCATION_SELECT; > > + DRM_DEBUG_KMS("disabling fdi C rx\n"); > > + I915_WRITE(SOUTH_CHICKEN1, temp); > > + } > > +} > > + > > /* The FDI link training functions for ILK/Ibexpeak. */ > > static void ironlake_fdi_link_train(struct drm_crtc *crtc) > > { > > @@ -2580,6 +2603,9 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc) > > POSTING_READ(reg); > > udelay(150); > > > > + DRM_DEBUG_KMS("FDI_RX_IIR before link train 0x%x\n", > > + I915_READ(FDI_RX_IIR(pipe))); > > + > > /* enable CPU FDI TX and PCH FDI RX */ > > reg = FDI_TX_CTL(pipe); > > temp = I915_READ(reg); > > @@ -2625,7 +2651,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc) > > if (temp & FDI_RX_BIT_LOCK || > > (I915_READ(reg) & FDI_RX_BIT_LOCK)) { > > I915_WRITE(reg, temp | FDI_RX_BIT_LOCK); > > - DRM_DEBUG_KMS("FDI train 1 done.\n"); > > + DRM_DEBUG_KMS("FDI train 1 done, level %i.\n", i); > > break; > > } > > } > > @@ -2666,7 +2692,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc) > > > > if (temp & FDI_RX_SYMBOL_LOCK) { > > I915_WRITE(reg, temp | FDI_RX_SYMBOL_LOCK); > > - DRM_DEBUG_KMS("FDI train 2 done.\n"); > > + DRM_DEBUG_KMS("FDI train 2 done, level %i.\n", i); > > break; > > } > > } > > @@ -4878,6 +4904,88 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc, > > return true; > > } > > > > +static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + uint32_t temp; > > + > > + temp = I915_READ(SOUTH_CHICKEN1); > > + if (temp & FDI_BC_BIFURCATION_SELECT) > > + return; > > + > > + WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) & FDI_RX_ENABLE); > > + WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) & FDI_RX_ENABLE); > > + > > + temp |= FDI_BC_BIFURCATION_SELECT; > > + DRM_DEBUG_KMS("enabling fdi C rx\n"); > > + I915_WRITE(SOUTH_CHICKEN1, temp); > > + POSTING_READ(SOUTH_CHICKEN1); > > +} > > + > > +static bool ironlake_check_fdi_lanes(struct intel_crtc *intel_crtc) > > +{ > > + struct drm_device *dev = intel_crtc->base.dev; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + struct intel_crtc *pipe_B_crtc = > > + to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_B]); > > + > > + DRM_DEBUG_KMS("checking fdi config on pipe %i, lanes %i\n", > > + intel_crtc->pipe, intel_crtc->fdi_lanes); > > + if (intel_crtc->fdi_lanes > 4) { > > + DRM_DEBUG_KMS("invalid fdi lane config on pipe %i: %i lanes\n", > > + intel_crtc->pipe, intel_crtc->fdi_lanes); > > + /* Clamp lanes to avoid programming the hw with bogus values. */ > > + intel_crtc->fdi_lanes = 4; > > + > > + return false; > > + } > > + > > + if (dev_priv->num_pipe == 2) > > + return true; > > + > > + switch (intel_crtc->pipe) { > > + case PIPE_A: > > + return true; > > + case PIPE_B: > > + if (dev_priv->pipe_to_crtc_mapping[PIPE_C]->enabled && > > + intel_crtc->fdi_lanes > 2) { > > + DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %i: %i lanes\n", > > + intel_crtc->pipe, intel_crtc->fdi_lanes); > > + /* Clamp lanes to avoid programming the hw with bogus values. */ > > + intel_crtc->fdi_lanes = 2; > > + > > + return false; > > + } > > + > > + if (intel_crtc->fdi_lanes > 2) > > + WARN_ON(I915_READ(SOUTH_CHICKEN1) & FDI_BC_BIFURCATION_SELECT); > > + else > > + cpt_enable_fdi_bc_bifurcation(dev); > > + > > + return true; > > + case PIPE_C: > > + if (!pipe_B_crtc->base.enabled || pipe_B_crtc->fdi_lanes <= 2) { > > + if (intel_crtc->fdi_lanes > 2) { > > + DRM_DEBUG_KMS("invalid shared fdi lane config on pipe %i: %i lanes\n", > > + intel_crtc->pipe, intel_crtc->fdi_lanes); > > + /* Clamp lanes to avoid programming the hw with bogus values. */ > > + intel_crtc->fdi_lanes = 2; > > + > > + return false; > > + } > > + } else { > > + DRM_DEBUG_KMS("fdi link B uses too many lanes to enable link C\n"); > > + return false; > > + } > > + > > + cpt_enable_fdi_bc_bifurcation(dev); > > + > > + return true; > > + default: > > + BUG(); > > + } > > +} > > + > > static void ironlake_set_m_n(struct drm_crtc *crtc, > > struct drm_display_mode *mode, > > struct drm_display_mode *adjusted_mode) > > @@ -5076,7 +5184,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > > struct intel_encoder *encoder; > > u32 temp; > > int ret; > > - bool dither; > > + bool dither, fdi_config_ok; > > > > for_each_encoder_on_crtc(dev, crtc, encoder) { > > switch (encoder->type) { > > @@ -5213,8 +5321,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > > > > intel_set_pipe_timings(intel_crtc, mode, adjusted_mode); > > > > + /* Note, this also computes intel_crtc->fdi_lanes which is used below in > > + * ironlake_check_fdi_lanes. */ > > ironlake_set_m_n(crtc, mode, adjusted_mode); > > > > + fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc); > > + > > if (is_cpu_edp) > > ironlake_set_pll_edp(crtc, adjusted_mode->clock); > > > > @@ -5232,7 +5344,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > > > > intel_update_linetime_watermarks(dev, pipe, adjusted_mode); > > > > - return ret; > > + return fdi_config_ok ? ret : -EINVAL; > > } > > > > static int haswell_crtc_mode_set(struct drm_crtc *crtc, > > @@ -8188,6 +8300,8 @@ static void intel_init_display(struct drm_device *dev) > > /* FIXME: detect B0+ stepping and use auto training */ > > dev_priv->display.fdi_link_train = ivb_manual_fdi_link_train; > > dev_priv->display.write_eld = ironlake_write_eld; > > + dev_priv->display.modeset_global_resources = > > + ivb_modeset_global_resources; > > } else if (IS_HASWELL(dev)) { > > dev_priv->display.fdi_link_train = hsw_fdi_link_train; > > dev_priv->display.write_eld = haswell_write_eld; > > -- > > 1.7.11.4 > > > > > > -- > Paulo Zanoni -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch