On Mon, Sep 16, 2013 at 12:07:32PM -0300, Paulo Zanoni wrote:
> 2013/8/30 <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.
>
> >From the commit message, it seems this patch could be split in 4 or 5
> sub-patches.
I think if I split it up any more then then the indiviual patches might
stop making sense. Then you're going to be asking "why are you doing this
change here?" and I have to answer "see patch n+2" or something.
>
>
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_drv.h | 12 +++
> > drivers/gpu/drm/i915/intel_pm.c | 190 +++++++++++++++++++++++----------------
> > 2 files changed, 126 insertions(+), 76 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 8b132e2..664df77 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -293,6 +293,12 @@ struct intel_crtc_config {
> > bool ips_enabled;
> > };
> >
> > +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;
> > @@ -333,6 +339,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;
>
> The fact that we call "active" may imply that we have "inactive" WMs.
Soon enough. Well I call the other guy 'pending' and I wanted to call
this guy 'current', but you can't call anyone by that name in the
kernel, so I had to choose something else.
>
>
> > + } 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 163ba74..c6f105f 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2440,53 +2440,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, ¶ms[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)
> > {
> > @@ -2670,44 +2623,121 @@ 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);
> > + 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));
> > +
> > + ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
>
> Can you please add a nice comment explaining why we're using
> INTEL_DDB_PART_1_2 and ignoring the 5_6 partitioning model? I know the
> old code doesn't have it, but I had to look at the code again to try
> to understand it.
Can do.
>
>
> > +
> > + 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]);
>
> I think functions like "ilk_check_wm" and "ilk_wm_max" could be
> renamed to longer names that really explain what they do. Can you
> please do this in a separate patch? Maybe ilk_check_wm could be
> ilk_can_enable_wm or something similar,
I don't like that name very much. Not that I like ilk_check_wm() either.
Neither conveys the fact that we actually modify the structure. Maybe
ilk_validate_wm_level() or something?
> and maybe ilk_wm_max could be
> ilk_compute_wm_max_values or ilk_compute_wm_maximums or something else
> (the word "max" is used for both "max level" and "maximum values per
> level").
ilk_compute_wm_maximums() sounds reasonably clear to me. I'll make that
change.
>
>
> > +}
> > +
> > +/*
> > + * 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;
> > +
>
> Don't we need to memset() ret_wm to zero here? Even if it's not needed
> now because of some other previous memset, I fear future code changes
> may introduce bugs due to non-zero values reaching this point.
I don't much like sprinkling memsets() around. Makes it look like
no-one knows when they should call the function. Also you might memset()
something by accident that you weren't supposed to, and possibly makes
the code slower. So zero initialization is nicer IMO.
>
>
> > + 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;
> > +
> > + 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;
>
> Maybe it would be nice to add a comment to explain why we set this to
> true, and who would set this to untrue in case it can't be true.
I was almost considering killing 'enable' from intel_wm_level. It should
really be called 'potentially_valid' or something. But I didn't really
check whether killing it completely would cause a lot of needless work to
be done.
> > +}
> > +
> > +/*
> > + * 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 lp_wm = {};
>
> Took me a while to realize that intel_pipe_wm isn't used only as the
> WM values for a single pipe (as the name suggests), but also for the
> merged WMs. Maybe we should rename the struct. And here maybe rename
> from lp_wm to merged_wms?
I think I originally called it 'merged' or something in most places,
but changed it to lp_wm to avoid renaming stuff too much that was
already present in the code.
I'm not sure 'merged' is the best name either. The spec talks about
consolidating watermarks from multiple pipes. Maybe we should use the
same terminology? But 'consolidated' is bit too long for my taste.
>
>
> >
> > - 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,
> > + ¶ms[intel_crtc->pipe],
> > + &intel_crtc->wm.active);
> > +
> > + ilk_wm_merge(dev, lp_maximums, &lp_wm);
> >
> > 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 = lp_wm.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 && lp_wm.wm[4].enable);
>
> I find the older version easier to understand since it matches the
> spec and doesn't rely on adding a boolean value.
What's wrong with adding boolean values?
I could change it to
level = wm_lp + (wm_lp >= 2 && lp_wm.wm[4].enable) ? 1 : 0;
but I think that just looks a bit silly.
> Also maybe we should
> add a comment explaining that we do this since on HSW we have 4 levels
> but only 3 slots (yes, I know, I should have added this comment when I
> wrote the original code).
I'm adding a comment in patch 12/19 when I refactor the computation
to a separate function.
>
>
> > +
> > + r = &lp_wm.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,
> > @@ -2715,13 +2745,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,
> > - ¶ms[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 (!r->enable)
> > + continue;
>
> Does this mean "pipe not active"? Maybe add a comment explaining. This
> is confusing since we're talking about LP0 levels and they shouldn't
> really be disabled.
Yeah. That should really be a WARN_ON() or something to indiate that it
should never happen.
>
>
> Our WM code is getting quite complex, it's not very easy to follow.
> The comments/renames I asked are for stuff that took me a while to
> understand. I also wouldn't complain if "v2" comes as a series of
> smaller patches.
>
>
> I did test this patch on my Haswell machine and the values still seem
> to be correct.
>
>
> > +
> > + 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
