[Intel-gfx] [PATCH 05/24] drm/i915: Merge LP1+ watermarks in safer way
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Apr 23 22:28:35 CEST 2014
On Wed, Apr 23, 2014 at 04:13:25PM -0300, Paulo Zanoni wrote:
> 2014-03-07 13:32 GMT-03:00 <ville.syrjala at linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > On ILK when we disable a particular watermark level, we must
>
> Just to be clear: do you mean ILK or ILK+ here?
IIRC ILK was where I saw it really easily. IVB didn't seem quite as
sensitive to it.
>
> > maintain the actual watermark values for that level for some time
> > (until the next vblank possibly). Otherwise we risk underruns.
> >
> > In order to achieve that result we must merge the LP1+ watermarks a
> > bit differently since we must also merge levels that are to be
> > disabled. We must also make sure we don't overflow the fields in the
> > watermark registers in case the calculated watermarks come out too
> > big to fit.
> >
> > As early as possbile we mark all computed watermark levels as
> > disabled if they would exceed the register maximums. We make sure
> > to leave the actual watermarks for such levels zeroed out. The during
> > merging, we take the maxium values for every level, regardless if
> > they're disabled or not. That may seem a bit pointless since at the
> > moment all the watermark levels we merge should have their values
> > zeroed if the level is already disabled. However soon we will be
> > dealing with intermediate watermarks that, in addition to the new
> > watermark values, also contain the previous watermark values, and so
> > levels that are disabled may no longer be zeroed out.
>
> I am having a hard time here. Watermarks code is extremely complex
> these days, and my brain does not have enough memory to think about
> all the implications and side effects of all the changes you did here.
> I have stared a this patch for a long time, and I don't think I fully
> understand it. I think you should probably try to break this change
> into some smaller steps, with nice commit messages. Some stupid
> questions to help me clarify:
>
> As far as I understood, the biggest change of this patch is that when
> some WM level is disabled, we won't write 0x0 to the WM register, but
> we will write actual values, but with bit 31 set to zero, right?
That's the idea.
> On
> the first sentence of the commit message, you say we must maintain the
> old values for that level for some time, but on the current code we
> won't keep the old values: we will generate new values, and if a level
> that was previously enabled needs to be disabled, there's no guarantee
> that the new value we will generate will be exactly the same as the
> old-value-but-with-bit-31-disabled. Why does that work?
It's not really hooked up yet. There's going to be another patch down
the line that calculates the "intermediate" watermarks for an individual
pipe as max(old,new). Those intermediate watermarks get applied to the
affected pipe, and then as we merge the LP1+ watermarks from all pipes
we must use similar logic. Ie. if one pipe has/had LP2=100 but
another pipe has/had LP2 disabled we must write 100 to the register
(w/o the enable bit set of course).
So this patch just modifies the LP1+ merge logic to make that happen.
Another patch will deal with the old vs. new situation.
> Since it
> doesn't make sense to me, I probably understood something very wrong
> here...
>
> A little more below...
>
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 81 ++++++++++++++++++++++++++++++++---------
> > 1 file changed, 64 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index f061ef1..ba4b23e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -1921,6 +1921,16 @@ static void ilk_compute_wm_maximums(const struct drm_device *dev,
> > max->fbc = ilk_fbc_wm_reg_max(dev);
> > }
> >
> > +static void ilk_compute_wm_reg_maximums(struct drm_device *dev,
> > + int level,
> > + struct ilk_wm_maximums *max)
> > +{
> > + max->pri = ilk_plane_wm_reg_max(dev, level, false);
> > + max->spr = ilk_plane_wm_reg_max(dev, level, true);
> > + max->cur = ilk_cursor_wm_reg_max(dev, level);
> > + max->fbc = ilk_fbc_wm_reg_max(dev);
> > +}
> > +
> > static bool ilk_validate_wm_level(int level,
> > const struct ilk_wm_maximums *max,
> > struct intel_wm_level *result)
> > @@ -2178,9 +2188,6 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> > };
> > struct ilk_wm_maximums max;
> >
> > - /* LP0 watermarks always use 1/2 DDB partitioning */
> > - ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> > -
> > pipe_wm->pipe_enabled = params->active;
> > pipe_wm->sprites_enabled = params->spr.enabled;
> > pipe_wm->sprites_scaled = params->spr.scaled;
> > @@ -2193,15 +2200,37 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> > if (params->spr.scaled)
> > max_level = 0;
> >
> > - for (level = 0; level <= max_level; level++)
> > - ilk_compute_wm_level(dev_priv, level, params,
> > - &pipe_wm->wm[level]);
> > + ilk_compute_wm_level(dev_priv, 0, params, &pipe_wm->wm[0]);
> >
> > if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
> >
> > + /* LP0 watermarks always use 1/2 DDB partitioning */
> > + ilk_compute_wm_maximums(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> > +
> > /* At least LP0 must be valid */
> > - return ilk_validate_wm_level(0, &max, &pipe_wm->wm[0]);
> > + if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0]))
> > + return false;
> > +
>
> For example, this chunk introduces an early return to the function. I
> really think this should be on its own patch, with a nice explanation
> of why it is needed, and what are the consequences.
Hmm. I suppose this patch could be split in two so that this part would
come first. I already had tons of pain trying to split these up, but I
guess I missed some opportunities.
This change was here to make sure we never have any watermark values
stored in pipe_wm that would exceed the maximum allowed in the registers.
That would totally mess up the merging of the watermarks since
wm->enabled==false will no longer prevent us from using those values
in the max() calculation, so we'd end up overflowing the bits in
the register.
>
> > + ilk_compute_wm_reg_maximums(dev, 1, &max);
> > +
> > + for (level = 1; level <= max_level; level++) {
> > + struct intel_wm_level wm = {};
> > +
> > + ilk_compute_wm_level(dev_priv, level, params, &wm);
> > +
> > + /*
> > + * Disable any watermark level that exceeds the
> > + * register maximums since such watermarks are
> > + * always invalid.
> > + */
> > + if (!ilk_validate_wm_level(level, &max, &wm))
> > + break;
> > +
> > + pipe_wm->wm[level] = wm;
> > + }
> > +
> > + return true;
> > }
> >
> > /*
> > @@ -2213,6 +2242,8 @@ static void ilk_merge_wm_level(struct drm_device *dev,
> > {
> > const struct intel_crtc *intel_crtc;
> >
> > + ret_wm->enable = true;
> > +
> > list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
> > const struct intel_pipe_wm *active = &intel_crtc->wm.active;
> > const struct intel_wm_level *wm = &active->wm[level];
> > @@ -2220,16 +2251,18 @@ static void ilk_merge_wm_level(struct drm_device *dev,
> > if (!active->pipe_enabled)
> > continue;
> >
> > - if (!wm->enable)
> > - return;
> > + /*
> > + * The watermark values may have been used in the past,
> > + * so we must maintain them in the registers for some
> > + * time even if the level is now disabled.
> > + */
> > + ret_wm->enable &= wm->enable;
>
> I really prefer the "if (!wm->enable) ret_wm->enable = false;" form.
> Even if it's less efficient, it's much easier to read and understand.
> By the time you make sure what the "&=" statement is supposed to mean,
> you already lost all the previous context you were following... And
> there's also the possibility that GCC will do the best thing
> automagically.
So you're an optimist :)
>
>
> >
> > 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;
> > }
> >
> > /*
> > @@ -2241,6 +2274,7 @@ static void ilk_wm_merge(struct drm_device *dev,
> > struct intel_pipe_wm *merged)
> > {
> > int level, max_level = ilk_wm_max_level(dev);
> > + int last_enabled_level = max_level;
> >
> > /* ILK/SNB/IVB: LP1+ watermarks only w/ single pipe */
> > if ((INTEL_INFO(dev)->gen <= 6 || IS_IVYBRIDGE(dev)) &&
> > @@ -2256,15 +2290,19 @@ static void ilk_wm_merge(struct drm_device *dev,
> >
> > ilk_merge_wm_level(dev, level, wm);
> >
> > - if (!ilk_validate_wm_level(level, max, wm))
> > - break;
> > + if (level > last_enabled_level)
> > + wm->enable = false;
> > + else if (!ilk_validate_wm_level(level, max, wm))
> > + /* make sure all following levels get disabled */
> > + last_enabled_level = level - 1;
> >
> > /*
> > * 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;
> > + if (wm->enable)
> > + merged->fbc_wm_enabled = false;
> > wm->fbc_val = 0;
> > }
> > }
> > @@ -2319,14 +2357,19 @@ static void ilk_compute_wm_results(struct drm_device *dev,
> > level = ilk_wm_lp_to_level(wm_lp, merged);
> >
> > r = &merged->wm[level];
> > - if (!r->enable)
> > - break;
> >
> > - results->wm_lp[wm_lp - 1] = WM3_LP_EN |
> > + /*
> > + * Maintain the watermark values even if the level is
> > + * disabled. Doing otherwise could cause underruns.
> > + */
> > + results->wm_lp[wm_lp - 1] =
> > (ilk_wm_lp_latency(dev, level) << WM1_LP_LATENCY_SHIFT) |
> > (r->pri_val << WM1_LP_SR_SHIFT) |
> > r->cur_val;
> >
> > + if (r->enable)
> > + results->wm_lp[wm_lp - 1] |= WM1_LP_SR_EN;
> > +
> > if (INTEL_INFO(dev)->gen >= 8)
> > results->wm_lp[wm_lp - 1] |=
> > r->fbc_val << WM1_LP_FBC_SHIFT_BDW;
> > @@ -2334,6 +2377,10 @@ static void ilk_compute_wm_results(struct drm_device *dev,
> > results->wm_lp[wm_lp - 1] |=
> > r->fbc_val << WM1_LP_FBC_SHIFT;
> >
> > + /*
> > + * Always set WM1S_LP_EN when spr_val != 0, even if the
> > + * level is disabled. Doing otherwise could cause underruns.
> > + */
>
> Shouldn't this comment be on top of the "if (r->enable)" chunk above?
No, this is about the sprite WM enable bit. That seems to behave like
the actual watermark values in the register. Ie. if we had the sprite
enabled with LP1 watermarks previously but now we're about to disable
the LP1 watermark, we must leave the sprite WM enable bit set just
like we leave the watermark value set in the register.
> Also, shouldn't this change be on its own separate patch with a
> separate explanation?
I have sprinkled similar comments to other parts of intel_pm.c. This
hunk could maybe have been part of those patches except it doesn't
really make sense until the break statement was eliminated from the
loop. I guess it could be split up into a patch of its own. But that
seems a bit pointless.
>
>
> General comments (not specific to this patch) based on my difficulty
> to review the code:
>
> The watermarks code got so complex that I really think we should add
> documentation to most of the functions explaining "this function takes
> X and Y structs as parameters, and writes the content of Z struct
> based on it". Also, I really think we should document and rename a lot
> of our structs, because I often can't think what they are supposed to
> contain just based on their names, and how some one structure should
> be different from the other. And some variable names are extemelly
> short, they could certainly be renamed to something with more meaning.
Yeah the code does have too many structs for various things, and there's
some duplication between them in some cases. And I still keep adding more
to make the intermediate watermarks stuff work. My plan is to shovel some
of that into some plane config thing eventually. That should cut it down
a bit and eliminate some duplicated information. Though in some cases we
need to the duplication since we need to track the watermarks in time.
I have no objections to documentation. I guess I need to find the time
to write it...
>
> Thanks,
> Paulo
>
> > if (INTEL_INFO(dev)->gen <= 6 && r->spr_val) {
> > WARN_ON(wm_lp != 1);
> > results->wm_lp_spr[wm_lp - 1] = WM1S_LP_EN | r->spr_val;
> > --
> > 1.8.3.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list