Hi
2012/10/26 Daniel Vetter <daniel.vetter at 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 at 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 at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
