Re: [PATCH 11/24] drm/i915: Check hw vs. sw watermark state after programming

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 23, 2014 at 06:16:07PM -0300, Paulo Zanoni wrote:
> 2014-03-07 13:32 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Make sure we programmed the watermarks correctly, by reading out the
> > hardware state again after programming and comparing it with the
> > state we supposedly programmed into hardware. Dump the watermark
> > registers after a mismatch, very much like we for the pipe config.
> > The only difference is that we don't dump the entire watermark
> > software tracking state since that's spread around a bit.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This one could also have been split into more than one patch: first
> you extract the functions, then later you add the new callers.
> 
> My only comment is: do we really want to check HW/SW state right after
> we write the register values? Shouldn't  this be done at some other
> time, like the end of the modeset sequence?

There's no guarantee that the WM update has finished there. I guess we
could sprinkle such checks to a few places. The check would have to merge
the active watermarks from all pipes and compare that with the register
contents. So it would check that our hardware state matches the software
active state at some point in time. But it can't really check that our
idea of active watermarks is correct.

This patch just checks that we actually wrote what we inteded into the
registers. So it basically tests that ilk_compute_wm_dirty() works and
that ilk_write_wm_values() didn't neglect to update something that was
deemed dirty.

> 
> Still, the patch seems correct, so: Reviewed-by: Paulo Zanoni
> <paulo.r.zanoni@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 117 ++++++++++++++++++++++++++++------------
> >  1 file changed, 83 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index ba4b23e..e519578a1 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2524,15 +2524,84 @@ static bool _ilk_disable_lp_wm(struct drm_i915_private *dev_priv,
> >         return changed;
> >  }
> >
> > +static void _ilk_pipe_wm_get_hw_state(struct drm_device *dev,
> > +                                     enum pipe pipe,
> > +                                     struct ilk_wm_values *hw)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       static const unsigned int wm0_pipe_reg[] = {
> > +               [PIPE_A] = WM0_PIPEA_ILK,
> > +               [PIPE_B] = WM0_PIPEB_ILK,
> > +               [PIPE_C] = WM0_PIPEC_IVB,
> > +       };
> > +
> > +       hw->wm_pipe[pipe] = I915_READ(wm0_pipe_reg[pipe]);
> > +
> > +       if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > +               hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
> > +}
> > +
> > +static void _ilk_wm_get_hw_state(struct drm_device *dev,
> > +                                struct ilk_wm_values *hw)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       enum pipe pipe;
> > +
> > +       for_each_pipe(pipe)
> > +               _ilk_pipe_wm_get_hw_state(dev, pipe, hw);
> > +
> > +       hw->wm_lp[0] = I915_READ(WM1_LP_ILK);
> > +       hw->wm_lp[1] = I915_READ(WM2_LP_ILK);
> > +       hw->wm_lp[2] = I915_READ(WM3_LP_ILK);
> > +
> > +       hw->wm_lp_spr[0] = I915_READ(WM1S_LP_ILK);
> > +       if (INTEL_INFO(dev)->gen >= 7) {
> > +               hw->wm_lp_spr[1] = I915_READ(WM2S_LP_IVB);
> > +               hw->wm_lp_spr[2] = I915_READ(WM3S_LP_IVB);
> > +       }
> > +
> > +       if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > +               hw->partitioning = (I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6) ?
> > +                       INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2;
> > +       else if (IS_IVYBRIDGE(dev))
> > +               hw->partitioning = (I915_READ(DISP_ARB_CTL2) & DISP_DATA_PARTITION_5_6) ?
> > +                       INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2;
> > +
> > +       hw->enable_fbc_wm =
> > +               !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
> > +}
> > +
> > +static void ilk_dump_wm_values(const struct ilk_wm_values *hw,
> > +                              const char *context)
> > +{
> > +       DRM_DEBUG_KMS("%s watermark values\n", context);
> > +       DRM_DEBUG_KMS("WM_PIPE_A = 0x%08x\n",  hw->wm_pipe[PIPE_A]);
> > +       DRM_DEBUG_KMS("WM_PIPE_B = 0x%08x\n",  hw->wm_pipe[PIPE_B]);
> > +       DRM_DEBUG_KMS("WM_PIPE_C = 0x%08x\n",  hw->wm_pipe[PIPE_C]);
> > +       DRM_DEBUG_KMS("WM_LP_1 = 0x%08x\n", hw->wm_lp[0]);
> > +       DRM_DEBUG_KMS("WM_LP_2 = 0x%08x\n", hw->wm_lp[1]);
> > +       DRM_DEBUG_KMS("WM_LP_3 = 0x%08x\n", hw->wm_lp[2]);
> > +       DRM_DEBUG_KMS("WM_LP_SPR_1 = 0x%08x\n", hw->wm_lp_spr[0]);
> > +       DRM_DEBUG_KMS("WM_LP_SPR_2 = 0x%08x\n", hw->wm_lp_spr[1]);
> > +       DRM_DEBUG_KMS("WM_LP_SPR_3 = 0x%08x\n", hw->wm_lp_spr[2]);
> > +       DRM_DEBUG_KMS("WM_LINETIME_A = 0x%08x\n", hw->wm_linetime[PIPE_A]);
> > +       DRM_DEBUG_KMS("WM_LINETIME_B = 0x%08x\n", hw->wm_linetime[PIPE_B]);
> > +       DRM_DEBUG_KMS("WM_LINETIME_C = 0x%08x\n", hw->wm_linetime[PIPE_C]);
> > +       DRM_DEBUG_KMS("enable FBC watermark = %d\n", hw->enable_fbc_wm);
> > +       DRM_DEBUG_KMS("DDB partitioning = %s\n",
> > +                     hw->partitioning == INTEL_DDB_PART_1_2 ? "1/2" : "5/6");
> > +}
> > +
> >  /*
> >   * The spec says we shouldn't write when we don't need, because every write
> >   * causes WMs to be re-evaluated, expending some power.
> >   */
> >  static void ilk_write_wm_values(struct drm_i915_private *dev_priv,
> > -                               struct ilk_wm_values *results)
> > +                               const struct ilk_wm_values *results)
> >  {
> >         struct drm_device *dev = dev_priv->dev;
> >         struct ilk_wm_values *previous = &dev_priv->wm.hw;
> > +       struct ilk_wm_values hw = {};
> >         unsigned int dirty;
> >         uint32_t val;
> >
> > @@ -2602,6 +2671,14 @@ static void ilk_write_wm_values(struct drm_i915_private *dev_priv,
> >                 I915_WRITE(WM3_LP_ILK, results->wm_lp[2]);
> >
> >         dev_priv->wm.hw = *results;
> > +
> > +       _ilk_wm_get_hw_state(dev, &hw);
> > +
> > +       if (memcmp(results, &hw, sizeof(hw))) {
> > +               WARN(1, "watermark state doesn't match!\n");
> > +               ilk_dump_wm_values(&hw, "[hw state]");
> > +               ilk_dump_wm_values(results, "[sw state]");
> > +       }
> >  }
> >
> >  static bool ilk_disable_lp_wm(struct drm_device *dev)
> > @@ -2683,23 +2760,14 @@ static void ilk_update_sprite_wm(struct drm_plane *plane,
> >         ilk_update_wm(crtc);
> >  }
> >
> > -static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> > +static void _ilk_pipe_wm_hw_to_sw(struct drm_crtc *crtc)
> >  {
> >         struct drm_device *dev = crtc->dev;
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -       struct ilk_wm_values *hw = &dev_priv->wm.hw;
> > +       const struct ilk_wm_values *hw = &dev_priv->wm.hw;
> >         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >         struct intel_pipe_wm *active = &intel_crtc->wm.active;
> >         enum pipe pipe = intel_crtc->pipe;
> > -       static const unsigned int wm0_pipe_reg[] = {
> > -               [PIPE_A] = WM0_PIPEA_ILK,
> > -               [PIPE_B] = WM0_PIPEB_ILK,
> > -               [PIPE_C] = WM0_PIPEC_IVB,
> > -       };
> > -
> > -       hw->wm_pipe[pipe] = I915_READ(wm0_pipe_reg[pipe]);
> > -       if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > -               hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
> >
> >         active->pipe_enabled = intel_crtc_active(crtc);
> >
> > @@ -2733,31 +2801,12 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> >  void ilk_wm_get_hw_state(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > -       struct ilk_wm_values *hw = &dev_priv->wm.hw;
> >         struct drm_crtc *crtc;
> >
> > -       list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> > -               ilk_pipe_wm_get_hw_state(crtc);
> > -
> > -       hw->wm_lp[0] = I915_READ(WM1_LP_ILK);
> > -       hw->wm_lp[1] = I915_READ(WM2_LP_ILK);
> > -       hw->wm_lp[2] = I915_READ(WM3_LP_ILK);
> > -
> > -       hw->wm_lp_spr[0] = I915_READ(WM1S_LP_ILK);
> > -       if (INTEL_INFO(dev)->gen >= 7) {
> > -               hw->wm_lp_spr[1] = I915_READ(WM2S_LP_IVB);
> > -               hw->wm_lp_spr[2] = I915_READ(WM3S_LP_IVB);
> > -       }
> > +       _ilk_wm_get_hw_state(dev, &dev_priv->wm.hw);
> >
> > -       if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > -               hw->partitioning = (I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6) ?
> > -                       INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2;
> > -       else if (IS_IVYBRIDGE(dev))
> > -               hw->partitioning = (I915_READ(DISP_ARB_CTL2) & DISP_DATA_PARTITION_5_6) ?
> > -                       INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2;
> > -
> > -       hw->enable_fbc_wm =
> > -               !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
> > +       list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> > +               _ilk_pipe_wm_hw_to_sw(crtc);
> >  }
> >
> >  /**
> > --
> > 1.8.3.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC