[Intel-gfx] [PATCH v2 05.2/24] drm/i915: Merge LP1+ watermarks in safer way

[Paulo Zanoni]

2014-04-28 9:44 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
> 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 at intel.com>

> 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 at linux.intel.com>
> ---
>  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