Re: [PATCH v2 01/16] drm/i915: Add intel_pipe_wm and prepare for watermark pre-compute

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

On Thu, Oct 10, 2013 at 06:43:55PM -0300, Paulo Zanoni wrote:
> 2013/10/9  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Introduce a new struct intel_pipe_wm which contains all the
> > watermarks for a single pipe. Use it to unify the LP0 and LP1+
> > watermark computations so that we can just iterate through the
> > watermark levels neatly and call ilk_compute_wm_level() for each.
> >
> > Also add another tool ilk_wm_merge() that merges the LP1+ watermarks
> > from all pipes. For that, embed one intel_pipe_wm inside intel_crtc that
> > contains the currently valid watermarks for each pipe.
> >
> > This is mainly preparatory work for pre-computing the watermarks for
> > each pipe and merging them at a later time. For now the merging still
> > happens immediately.
> >
> > v2: Add some comments about level 0 DDB split and intel_wm_config
> >     Add WARN_ON for level 0 being disabled
> >     s/lp_wm/merged
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  12 +++
> >  drivers/gpu/drm/i915/intel_pm.c  | 192 +++++++++++++++++++++++----------------
> >  2 files changed, 128 insertions(+), 76 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 71cfabd..3325b0b 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -309,6 +309,12 @@ struct intel_crtc_config {
> >         bool double_wide;
> >  };
> >
> > +struct intel_pipe_wm {
> > +       struct intel_wm_level wm[5];
> > +       uint32_t linetime;
> > +       bool fbc_wm_enabled;
> > +};
> > +
> >  struct intel_crtc {
> >         struct drm_crtc base;
> >         enum pipe pipe;
> > @@ -349,6 +355,12 @@ struct intel_crtc {
> >         /* Access to these should be protected by dev_priv->irq_lock. */
> >         bool cpu_fifo_underrun_disabled;
> >         bool pch_fifo_underrun_disabled;
> > +
> > +       /* per-pipe watermark state */
> > +       struct {
> > +               /* watermarks currently being used  */
> > +               struct intel_pipe_wm active;
> > +       } wm;
> >  };
> >
> >  struct intel_plane_wm_parameters {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 5e743ec..30b380c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2458,53 +2458,6 @@ static void ilk_compute_wm_level(struct drm_i915_private *dev_priv,
> >         result->enable = true;
> >  }
> >
> > -static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
> > -                             int level, const struct hsw_wm_maximums *max,
> > -                             const struct hsw_pipe_wm_parameters *params,
> > -                             struct intel_wm_level *result)
> > -{
> > -       enum pipe pipe;
> > -       struct intel_wm_level res[3];
> > -
> > -       for (pipe = PIPE_A; pipe <= PIPE_C; pipe++)
> > -               ilk_compute_wm_level(dev_priv, level, &params[pipe], &res[pipe]);
> > -
> > -       result->pri_val = max3(res[0].pri_val, res[1].pri_val, res[2].pri_val);
> > -       result->spr_val = max3(res[0].spr_val, res[1].spr_val, res[2].spr_val);
> > -       result->cur_val = max3(res[0].cur_val, res[1].cur_val, res[2].cur_val);
> > -       result->fbc_val = max3(res[0].fbc_val, res[1].fbc_val, res[2].fbc_val);
> > -       result->enable = true;
> > -
> > -       return ilk_check_wm(level, max, result);
> > -}
> > -
> > -
> > -static uint32_t hsw_compute_wm_pipe(struct drm_device *dev,
> > -                                   const struct hsw_pipe_wm_parameters *params)
> > -{
> > -       struct drm_i915_private *dev_priv = dev->dev_private;
> > -       struct intel_wm_config config = {
> > -               .num_pipes_active = 1,
> > -               .sprites_enabled = params->spr.enabled,
> > -               .sprites_scaled = params->spr.scaled,
> > -       };
> > -       struct hsw_wm_maximums max;
> > -       struct intel_wm_level res;
> > -
> > -       if (!params->active)
> > -               return 0;
> > -
> > -       ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> > -
> > -       ilk_compute_wm_level(dev_priv, 0, params, &res);
> > -
> > -       ilk_check_wm(0, &max, &res);
> > -
> > -       return (res.pri_val << WM0_PIPE_PLANE_SHIFT) |
> > -              (res.spr_val << WM0_PIPE_SPRITE_SHIFT) |
> > -              res.cur_val;
> > -}
> > -
> >  static uint32_t
> >  hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
> >  {
> > @@ -2687,44 +2640,123 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
> >                 *lp_max_5_6 = *lp_max_1_2;
> >  }
> >
> > +/* Compute new watermarks for the pipe */
> > +static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> > +                                 const struct hsw_pipe_wm_parameters *params,
> > +                                 struct intel_pipe_wm *pipe_wm)
> > +{
> > +       struct drm_device *dev = crtc->dev;
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       int level, max_level = ilk_wm_max_level(dev);
> > +       /* LP0 watermark maximums depend on this pipe alone */
> > +       struct intel_wm_config config = {
> > +               .num_pipes_active = 1,
> > +               .sprites_enabled = params->spr.enabled,
> > +               .sprites_scaled = params->spr.scaled,
> > +       };
> > +       struct hsw_wm_maximums max;
> > +
> > +       memset(pipe_wm, 0, sizeof(*pipe_wm));
> > +
> > +       /* LP0 watermarks always use 1/2 DDB partitioning */
> > +       ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> > +
> > +       for (level = 0; level <= max_level; level++)
> > +               ilk_compute_wm_level(dev_priv, level, params,
> > +                                    &pipe_wm->wm[level]);
> > +
> > +       pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
> > +
> > +       /* At least LP0 must be valid */
> > +       return ilk_check_wm(0, &max, &pipe_wm->wm[0]);
> > +}
> > +
> > +/*
> > + * Merge the watermarks from all active pipes for a specific level.
> > + */
> > +static void ilk_merge_wm_level(struct drm_device *dev,
> > +                              int level,
> > +                              struct intel_wm_level *ret_wm)
> > +{
> > +       const struct intel_crtc *intel_crtc;
> > +
> > +       list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
> > +               const struct intel_wm_level *wm =
> > +                       &intel_crtc->wm.active.wm[level];
> > +
> > +               if (!wm->enable)
> > +                       return;
> 
> Why exactly is this check here and what does it mean? Why not a
> "break" nor a "continue"? Do we expect to ever return at this point?

It means this watermark level wasn't enabled for this pipe, hence this
level (and bigger levels) must not be enabled in the merged watermarks
either.

> 
> It seems intel_compute_pipe_wm calls ilk_compute_wm_level which sets
> "result->enable = true" for everything. Then only level 0 goes through
> ilk_check_wm (which may turn result->enable to zero), but the level1+
> watermarks (which are the ones used in the merge function) never get a
> chance to set wm->enable to false. So the check would be useless.
> Maybe I'm missing something?

Yeah I guess with HSW it would never happen since it doesn't support
sprite scaling, and sprites in general don't restrict watermark levels.

Once we get to to use the same code for older generations, sprites
may cause some watermark levels to be disabled, so this could happen
there. But as multi-pipe LP1+ watermarks are a HSW only feature, we
should never get here on those older generations.

So I guess we could just drop that check or make it a WARN_ON to make
sure we catch problems. Or we could keep it as is, and maybe add an extra
knob to debugfs (or a module param) that would allow the user to
artifically limit watermarks levels. That could be a useful testing
aid in case we run into underrus. I think I actually have a
patch somewhere that does that.

> 
> With the questions above (answered || fixed || clarified || patched):
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > +
> > +               ret_wm->pri_val = max(ret_wm->pri_val, wm->pri_val);
> > +               ret_wm->spr_val = max(ret_wm->spr_val, wm->spr_val);
> > +               ret_wm->cur_val = max(ret_wm->cur_val, wm->cur_val);
> > +               ret_wm->fbc_val = max(ret_wm->fbc_val, wm->fbc_val);
> > +       }
> > +
> > +       ret_wm->enable = true;
> > +}
> > +
> > +/*
> > + * Merge all low power watermarks for all active pipes.
> > + */
> > +static void ilk_wm_merge(struct drm_device *dev,
> > +                        const struct hsw_wm_maximums *max,
> > +                        struct intel_pipe_wm *merged)
> > +{
> > +       int level, max_level = ilk_wm_max_level(dev);
> > +
> > +       merged->fbc_wm_enabled = true;
> > +
> > +       /* merge each WM1+ level */
> > +       for (level = 1; level <= max_level; level++) {
> > +               struct intel_wm_level *wm = &merged->wm[level];
> > +
> > +               ilk_merge_wm_level(dev, level, wm);
> > +
> > +               if (!ilk_check_wm(level, max, wm))
> > +                       break;
> > +
> > +               /*
> > +                * The spec says it is preferred to disable
> > +                * FBC WMs instead of disabling a WM level.
> > +                */
> > +               if (wm->fbc_val > max->fbc) {
> > +                       merged->fbc_wm_enabled = false;
> > +                       wm->fbc_val = 0;
> > +               }
> > +       }
> > +}
> > +
> >  static void hsw_compute_wm_results(struct drm_device *dev,
> >                                    const struct hsw_pipe_wm_parameters *params,
> >                                    const struct hsw_wm_maximums *lp_maximums,
> >                                    struct hsw_wm_values *results)
> >  {
> > -       struct drm_i915_private *dev_priv = dev->dev_private;
> > -       struct drm_crtc *crtc;
> > -       struct intel_wm_level lp_results[4] = {};
> > -       enum pipe pipe;
> > -       int level, max_level, wm_lp;
> > +       struct intel_crtc *intel_crtc;
> > +       int level, wm_lp;
> > +       struct intel_pipe_wm merged = {};
> >
> > -       for (level = 1; level <= 4; level++)
> > -               if (!hsw_compute_lp_wm(dev_priv, level,
> > -                                      lp_maximums, params,
> > -                                      &lp_results[level - 1]))
> > -                       break;
> > -       max_level = level - 1;
> > +       list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
> > +               intel_compute_pipe_wm(&intel_crtc->base,
> > +                                     &params[intel_crtc->pipe],
> > +                                     &intel_crtc->wm.active);
> > +
> > +       ilk_wm_merge(dev, lp_maximums, &merged);
> >
> >         memset(results, 0, sizeof(*results));
> >
> > -       /* The spec says it is preferred to disable FBC WMs instead of disabling
> > -        * a WM level. */
> > -       results->enable_fbc_wm = true;
> > -       for (level = 1; level <= max_level; level++) {
> > -               if (lp_results[level - 1].fbc_val > lp_maximums->fbc) {
> > -                       results->enable_fbc_wm = false;
> > -                       lp_results[level - 1].fbc_val = 0;
> > -               }
> > -       }
> > +       results->enable_fbc_wm = merged.fbc_wm_enabled;
> >
> > +       /* LP1+ register values */
> >         for (wm_lp = 1; wm_lp <= 3; wm_lp++) {
> >                 const struct intel_wm_level *r;
> >
> > -               level = (max_level == 4 && wm_lp > 1) ? wm_lp + 1 : wm_lp;
> > -               if (level > max_level)
> > +               level = wm_lp + (wm_lp >= 2 && merged.wm[4].enable);
> > +
> > +               r = &merged.wm[level];
> > +               if (!r->enable)
> >                         break;
> >
> > -               r = &lp_results[level - 1];
> >                 results->wm_lp[wm_lp - 1] = HSW_WM_LP_VAL(level * 2,
> >                                                           r->fbc_val,
> >                                                           r->pri_val,
> > @@ -2732,13 +2764,21 @@ static void hsw_compute_wm_results(struct drm_device *dev,
> >                 results->wm_lp_spr[wm_lp - 1] = r->spr_val;
> >         }
> >
> > -       for_each_pipe(pipe)
> > -               results->wm_pipe[pipe] = hsw_compute_wm_pipe(dev,
> > -                                                            &params[pipe]);
> > +       /* LP0 register values */
> > +       list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
> > +               enum pipe pipe = intel_crtc->pipe;
> > +               const struct intel_wm_level *r =
> > +                       &intel_crtc->wm.active.wm[0];
> >
> > -       for_each_pipe(pipe) {
> > -               crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > -               results->wm_linetime[pipe] = hsw_compute_linetime_wm(dev, crtc);
> > +               if (WARN_ON(!r->enable))
> > +                       continue;
> > +
> > +               results->wm_linetime[pipe] = intel_crtc->wm.active.linetime;
> > +
> > +               results->wm_pipe[pipe] =
> > +                       (r->pri_val << WM0_PIPE_PLANE_SHIFT) |
> > +                       (r->spr_val << WM0_PIPE_SPRITE_SHIFT) |
> > +                       r->cur_val;
> >         }
> >  }
> >
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC