| Message ID | 1351241899-7870-10-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi 2012/10/26 Daniel Vetter <daniel.vetter@ffwll.ch>: > 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. > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> You have 2 Signed-off-by tags on this patch ^ > > v3: Use the new ->modeset_global_resources callback. > > v4: Move the WARNs to the right place. Oh how I hate hacks. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > 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 7009c0f..90617cf 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_gloabl_resources(struct drm_device *dev) s/_gloabl_/_global_/ > +{ > + 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 enabled fdi B s/could enabled/could enable/ ? > + * 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; > } > } > @@ -4875,6 +4901,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) > @@ -5073,7 +5181,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) { > @@ -5210,8 +5318,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); > > @@ -5229,7 +5341,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; After reading your patch everything looks correct (even though it's complicated, so a proper testing is better than 1000 reviews here). My only worry is: do we properly disable all the resources we need to disable when we fail here? I am assuming you tested this :) My only last bikeshed would be to try to reuse "ret" instead of "fdi_config_ok" :) With the typos fixed, the double signed-off-by removed, the failure path properly tested and, optionally, the fdi_config_ok variable removed: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > } > > static int haswell_crtc_mode_set(struct drm_crtc *crtc, > @@ -8185,6 +8297,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_gloabl_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 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sat, Oct 27, 2012 at 2:56 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > After reading your patch everything looks correct (even though it's > complicated, so a proper testing is better than 1000 reviews here). My > only worry is: do we properly disable all the resources we need to > disable when we fail here? I am assuming you tested this :) I've hoped to document/check the requirements with WARN_ONs. And I've hit them until the code seemed right. So I think I'd be good if you could review those, and if you see any spot that isn't properly checked with WARNs. > My only last bikeshed would be to try to reuse "ret" instead of > "fdi_config_ok" :) I've had that early, but then dropped it again. My fear is that we can't just bail out in the middle of the modeset sequence, which is why I've sanitized the fdi_lanes, and let the entire sequence run through. But at the end we still need to check whether the mode fits and return an error (so that the caller can restore things). Hence a separate error value. btw, I've tested this, and it seems to work as advertised. > With the typos fixed, the double signed-off-by removed, the failure > path properly tested and, optionally, the fdi_config_ok variable > removed. Will do. -Daniel
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 7009c0f..90617cf 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_gloabl_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 enabled 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; } } @@ -4875,6 +4901,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) @@ -5073,7 +5181,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) { @@ -5210,8 +5318,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); @@ -5229,7 +5341,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, @@ -8185,6 +8297,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_gloabl_resources; } else if (IS_HASWELL(dev)) { dev_priv->display.fdi_link_train = hsw_fdi_link_train; dev_priv->display.write_eld = haswell_write_eld;