Re: [PATCH 11/16] drm/i915: Init HSW watermark tracking in intel_modeset_setup_hw_state()

[Paulo Zanoni <przanoni@gmail.com>]

2013/10/9  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Fill out the HSW watermark s/w tracking structures with the current
> hardware state in intel_modeset_setup_hw_state(). This allows us to skip
> the HW state readback during watermark programming and just use the values
> we keep around in dev_priv->wm. Reduces the overhead of the watermark
> programming quite a bit.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   3 +
>  drivers/gpu/drm/i915/intel_drv.h     |   1 +
>  drivers/gpu/drm/i915/intel_pm.c      | 104 ++++++++++++++++++++++++++---------
>  3 files changed, 82 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 27f98bc..194f933 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10820,6 +10820,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>                 pll->on = false;
>         }
>
> +       if (IS_HASWELL(dev))
> +               ilk_init_wm(dev);

If is_HSW, then ILK_something is quite confusing :) Not everybody is
aware of your greater plans for total watermarks domination.


> +
>         if (force_restore) {
>                 i915_redisable_vga(dev);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3325b0b..bdb1708 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -818,6 +818,7 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv);
>  void gen6_rps_boost(struct drm_i915_private *dev_priv);
>  void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
>  void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
> +void ilk_init_wm(struct drm_device *dev);
>
>
>  /* intel_sdvo.c */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 5bd8c73..cebd9b4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2840,37 +2840,19 @@ static unsigned int ilk_compute_wm_dirty(struct drm_device *dev,
>  static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
>                                 struct hsw_wm_values *results)
>  {
> -       struct hsw_wm_values previous;
> +       struct hsw_wm_values *previous = &dev_priv->wm.hw;
>         unsigned int dirty;
>         uint32_t val;
>
> -       previous.wm_pipe[0] = I915_READ(WM0_PIPEA_ILK);
> -       previous.wm_pipe[1] = I915_READ(WM0_PIPEB_ILK);
> -       previous.wm_pipe[2] = I915_READ(WM0_PIPEC_IVB);
> -       previous.wm_lp[0] = I915_READ(WM1_LP_ILK);
> -       previous.wm_lp[1] = I915_READ(WM2_LP_ILK);
> -       previous.wm_lp[2] = I915_READ(WM3_LP_ILK);
> -       previous.wm_lp_spr[0] = I915_READ(WM1S_LP_ILK);
> -       previous.wm_lp_spr[1] = I915_READ(WM2S_LP_IVB);
> -       previous.wm_lp_spr[2] = I915_READ(WM3S_LP_IVB);
> -       previous.wm_linetime[0] = I915_READ(PIPE_WM_LINETIME(PIPE_A));
> -       previous.wm_linetime[1] = I915_READ(PIPE_WM_LINETIME(PIPE_B));
> -       previous.wm_linetime[2] = I915_READ(PIPE_WM_LINETIME(PIPE_C));
> -
> -       previous.partitioning = (I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6) ?
> -                               INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2;
> -
> -       previous.enable_fbc_wm = !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
> -
> -       dirty = ilk_compute_wm_dirty(dev_priv->dev, &previous, results);
> +       dirty = ilk_compute_wm_dirty(dev_priv->dev, previous, results);
>         if (!dirty)
>                 return;
>
> -       if (dirty & WM_DIRTY_LP(3) && previous.wm_lp[2] != 0)
> +       if (dirty & WM_DIRTY_LP(3) && previous->wm_lp[2] != 0)
>                 I915_WRITE(WM3_LP_ILK, 0);
> -       if (dirty & WM_DIRTY_LP(2) && previous.wm_lp[1] != 0)
> +       if (dirty & WM_DIRTY_LP(2) && previous->wm_lp[1] != 0)
>                 I915_WRITE(WM2_LP_ILK, 0);
> -       if (dirty & WM_DIRTY_LP(1) && previous.wm_lp[0] != 0)
> +       if (dirty & WM_DIRTY_LP(1) && previous->wm_lp[0] != 0)
>                 I915_WRITE(WM1_LP_ILK, 0);
>
>         if (dirty & WM_DIRTY_PIPE(PIPE_A))
> @@ -2905,11 +2887,11 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
>                 I915_WRITE(DISP_ARB_CTL, val);
>         }
>
> -       if (dirty & WM_DIRTY_LP(1) && previous.wm_lp_spr[0] != results->wm_lp_spr[0])
> +       if (dirty & WM_DIRTY_LP(1) && previous->wm_lp_spr[0] != results->wm_lp_spr[0])
>                 I915_WRITE(WM1S_LP_ILK, results->wm_lp_spr[0]);
> -       if (dirty & WM_DIRTY_LP(2) && previous.wm_lp_spr[1] != results->wm_lp_spr[1])
> +       if (dirty & WM_DIRTY_LP(2) && previous->wm_lp_spr[1] != results->wm_lp_spr[1])
>                 I915_WRITE(WM2S_LP_IVB, results->wm_lp_spr[1]);
> -       if (dirty & WM_DIRTY_LP(3) && previous.wm_lp_spr[2] != results->wm_lp_spr[2])
> +       if (dirty & WM_DIRTY_LP(3) && previous->wm_lp_spr[2] != results->wm_lp_spr[2])
>                 I915_WRITE(WM3S_LP_IVB, results->wm_lp_spr[2]);
>
>         if (dirty & WM_DIRTY_LP(1) && results->wm_lp[0] != 0)
> @@ -3142,6 +3124,76 @@ static void sandybridge_update_sprite_wm(struct drm_plane *plane,
>         I915_WRITE(WM3S_LP_IVB, sprite_wm);
>  }
>
> +static void ilk_init_pipe_wm(struct drm_crtc *crtc)
> +{
> +       struct drm_device *dev = crtc->dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct hsw_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]);
> +       hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
> +
> +       /* Assume sprites are disabled */

Why? Please write in the comment.


> +
> +       if (intel_crtc_active(crtc)) {
> +               u32 tmp = hw->wm_pipe[pipe];
> +
> +               /*
> +                * For active pipes LP0 watermark is marked as
> +                * enabled, and LP1+ watermaks as disabled since
> +                * we can't really reverse compute them in case
> +                * multiple pipes are active.
> +                */
> +               active->wm[0].enable = true;
> +               active->wm[0].pri_val = (tmp & WM0_PIPE_PLANE_MASK) >> WM0_PIPE_PLANE_SHIFT;
> +               active->wm[0].spr_val = (tmp & WM0_PIPE_SPRITE_MASK) >> WM0_PIPE_SPRITE_SHIFT;
> +               active->wm[0].cur_val = tmp & WM0_PIPE_CURSOR_MASK;
> +               active->linetime = hw->wm_linetime[pipe];
> +       } else {
> +               int level, max_level = ilk_wm_max_level(dev);
> +
> +               /*
> +                * For inactive pipes, all watermark levels
> +                * should be marked as enabled but zeroed,
> +                * which is what we'd comoute them to.
> +                */
> +               for (level = 0; level <= max_level; level++)
> +                       active->wm[level].enable = true;

Why exactly do we compute them like this?

One thing that I noticed is that, both on current -nightly (without
your series) and with your series, when we disable all the screens we
zero all the watermarks, but leave the "enable" bits of the LP
watermarks enabled. IMHO we should treat this as a bug and fix it. I
wonder if the comment above is related with this problem.


> +       }
> +}
> +
> +void ilk_init_wm(struct drm_device *dev)

IMHO, maintaining the _get_hw_state nomenclature would be an improvement.


> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct hsw_wm_values *hw = &dev_priv->wm.hw;
> +       struct drm_crtc *crtc;
> +
> +       list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> +               ilk_init_pipe_wm(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);
> +       hw->wm_lp_spr[1] = I915_READ(WM2S_LP_IVB);
> +       hw->wm_lp_spr[2] = I915_READ(WM3S_LP_IVB);
> +
> +       hw->partitioning =
> +               !!(I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6);

This is a little bit dangerous... We never know if we're not going to
add a 4_6 partition type in the middle of the enum for Gen 17. And if
we add it, I'm 100% sure we'll forget to patch this line. I usually
try to avoid these things.


> +
> +       hw->enable_fbc_wm =
> +               !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
> +}
> +
>  /**
>   * intel_update_watermarks - update FIFO watermark values based on current modes

Also, this patch makes me wonder about those places where we change
the HW state directly (init_clock_gating, for example) without
touching the struct you just added. That specific init_clock_gating is
run before we call ilk_init_wm, so it shouldn't be a problem on
boot/resume, but it still leaves me worried...

Also, perhaps we could have one of those functions that try to check
if the tracked state is really the HW state...

>   *
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni