[Intel-gfx] [PATCH 05/24] drm/i915: Merge LP1+ watermarks in safer way
Paulo Zanoni
przanoni at gmail.com
Wed Apr 23 21:13:25 CEST 2014
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?
> 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? 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? 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.
> + 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.
>
> 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?
Also, shouldn't this change be on its own separate patch with a
separate explanation?
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.
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
More information about the Intel-gfx
mailing list