[Intel-gfx] [PATCH 05.1/24] drm/i915: Make sure computed watermarks never overflow the registers

[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>
>
> When we calculate the watermarks for a pipe make sure we leave any
> level fully zeroed out if it would exceed any of the maximum values
> that fit in the registers.
>
> This will be important later when we start to use also disabled
> watermark levels during LP1+ merging.

Thanks for splitting the patch! It's much easier to review now :)

>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 43 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f061ef1..c722acb 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;

The only caller of this function does not really check its return
value. OTOH, fixing this is outside of the scope of your patch, I'm
just mentioning in case you have some watermarks TODO list :)

Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

> +
> +       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;
>  }
>
>  /*
> --
> 1.8.3.2
>



-- 
Paulo Zanoni