2014-04-28 9:44 GMT-03:00 <ville.syrjala@xxxxxxxxxxxxxxx>:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> On ILK when we disable a particular watermark level, we must
> 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
"_Then_ during merging", I guess.
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> 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.
>
> v2: Split the patch in two (Paulo)
> Use if() instead of & when merging ->enable (Paulo)
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 37 ++++++++++++++++++++++++++++---------
> 1 file changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c722acb..b89fc33 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2242,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];
> @@ -2249,16 +2251,19 @@ static void ilk_merge_wm_level(struct drm_device *dev,
> if (!active->pipe_enabled)
> continue;
>
> + /*
> + * 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.
> + */
> if (!wm->enable)
> - return;
> + ret_wm->enable = false;
>
> 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;
> }
>
> /*
> @@ -2270,6 +2275,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)) &&
> @@ -2285,15 +2291,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;
> }
> }
> @@ -2348,14 +2358,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;
> @@ -2363,6 +2378,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.
> + */
> 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
>
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
