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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2013/10/11 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>:
> On Thu, Oct 10, 2013 at 06:43:55PM -0300, Paulo Zanoni wrote:
>> 2013/10/9  <ville.syrjala@xxxxxxxxxxxxxxx>:
>> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >
>> > 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@xxxxxxxxxxxxxxx>
>> > ---
>> >  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.
>

Thanks for the clarification. Since it seems correct, it makes sense
to merge this patch and then do any additional work as future patches.

>>
>> With the questions above (answered || fixed || clarified || patched):
>>
>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> > +
>> > +               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@xxxxxxxxxxxxxxxxxxxxx
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Paulo Zanoni
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux