Re: [PATCH] drm/i915: check fdi B/C lane sharing constraint

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 29, 2012 at 10:06:56AM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2012/10/27 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.
> >
> > 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 <daniel.vetter@ffwll.ch>
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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