2013/8/30 <ville.syrjala@xxxxxxxxxxxxxxx>:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Introduce a new struct intel_pipe_wm which contains all the
> watermarks for a single pipe. Use it to unify the LP0 and LP1+
> watermark computations so that we can just iterate through the
> watermark levels neatly and call ilk_compute_wm_level() for each.
>
> Also add another tool ilk_wm_merge() that merges the LP1+ watermarks
> from all pipes. For that, embed one intel_pipe_wm inside intel_crtc that
> contains the currently valid watermarks for each pipe.
>
> This is mainly preparatory work for pre-computing the watermarks for
> each pipe and merging them at a later time. For now the merging still
> happens immediately.
>From the commit message, it seems this patch could be split in 4 or 5
sub-patches.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_drv.h | 12 +++
> drivers/gpu/drm/i915/intel_pm.c | 190 +++++++++++++++++++++++----------------
> 2 files changed, 126 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8b132e2..664df77 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -293,6 +293,12 @@ struct intel_crtc_config {
> bool ips_enabled;
> };
>
> +struct intel_pipe_wm {
> + struct intel_wm_level wm[5];
> + uint32_t linetime;
> + bool fbc_wm_enabled;
> +};
> +
> struct intel_crtc {
> struct drm_crtc base;
> enum pipe pipe;
> @@ -333,6 +339,12 @@ struct intel_crtc {
> /* Access to these should be protected by dev_priv->irq_lock. */
> bool cpu_fifo_underrun_disabled;
> bool pch_fifo_underrun_disabled;
> +
> + /* per-pipe watermark state */
> + struct {
> + /* watermarks currently being used */
> + struct intel_pipe_wm active;
The fact that we call "active" may imply that we have "inactive" WMs.
> + } wm;
> };
>
> struct intel_plane_wm_parameters {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 163ba74..c6f105f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2440,53 +2440,6 @@ static void ilk_compute_wm_level(struct drm_i915_private *dev_priv,
> result->enable = true;
> }
>
> -static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
> - int level, const struct hsw_wm_maximums *max,
> - const struct hsw_pipe_wm_parameters *params,
> - struct intel_wm_level *result)
> -{
> - enum pipe pipe;
> - struct intel_wm_level res[3];
> -
> - for (pipe = PIPE_A; pipe <= PIPE_C; pipe++)
> - ilk_compute_wm_level(dev_priv, level, ¶ms[pipe], &res[pipe]);
> -
> - result->pri_val = max3(res[0].pri_val, res[1].pri_val, res[2].pri_val);
> - result->spr_val = max3(res[0].spr_val, res[1].spr_val, res[2].spr_val);
> - result->cur_val = max3(res[0].cur_val, res[1].cur_val, res[2].cur_val);
> - result->fbc_val = max3(res[0].fbc_val, res[1].fbc_val, res[2].fbc_val);
> - result->enable = true;
> -
> - return ilk_check_wm(level, max, result);
> -}
> -
> -
> -static uint32_t hsw_compute_wm_pipe(struct drm_device *dev,
> - const struct hsw_pipe_wm_parameters *params)
> -{
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_wm_config config = {
> - .num_pipes_active = 1,
> - .sprites_enabled = params->spr.enabled,
> - .sprites_scaled = params->spr.scaled,
> - };
> - struct hsw_wm_maximums max;
> - struct intel_wm_level res;
> -
> - if (!params->active)
> - return 0;
> -
> - ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> -
> - ilk_compute_wm_level(dev_priv, 0, params, &res);
> -
> - ilk_check_wm(0, &max, &res);
> -
> - return (res.pri_val << WM0_PIPE_PLANE_SHIFT) |
> - (res.spr_val << WM0_PIPE_SPRITE_SHIFT) |
> - res.cur_val;
> -}
> -
> static uint32_t
> hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
> {
> @@ -2670,44 +2623,121 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
> *lp_max_5_6 = *lp_max_1_2;
> }
>
> +/* Compute new watermarks for the pipe */
> +static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> + const struct hsw_pipe_wm_parameters *params,
> + struct intel_pipe_wm *pipe_wm)
> +{
> + struct drm_device *dev = crtc->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + int level, max_level = ilk_wm_max_level(dev);
> + struct intel_wm_config config = {
> + .num_pipes_active = 1,
> + .sprites_enabled = params->spr.enabled,
> + .sprites_scaled = params->spr.scaled,
> + };
> + struct hsw_wm_maximums max;
> +
> + memset(pipe_wm, 0, sizeof(*pipe_wm));
> +
> + ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
Can you please add a nice comment explaining why we're using
INTEL_DDB_PART_1_2 and ignoring the 5_6 partitioning model? I know the
old code doesn't have it, but I had to look at the code again to try
to understand it.
> +
> + for (level = 0; level <= max_level; level++)
> + ilk_compute_wm_level(dev_priv, level, params,
> + &pipe_wm->wm[level]);
> +
> + pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
> +
> + /* At least LP0 must be valid */
> + return ilk_check_wm(0, &max, &pipe_wm->wm[0]);
I think functions like "ilk_check_wm" and "ilk_wm_max" could be
renamed to longer names that really explain what they do. Can you
please do this in a separate patch? Maybe ilk_check_wm could be
ilk_can_enable_wm or something similar, and maybe ilk_wm_max could be
ilk_compute_wm_max_values or ilk_compute_wm_maximums or something else
(the word "max" is used for both "max level" and "maximum values per
level").
> +}
> +
> +/*
> + * Merge the watermarks from all active pipes for a specific level.
> + */
> +static void ilk_merge_wm_level(struct drm_device *dev,
> + int level,
> + struct intel_wm_level *ret_wm)
> +{
> + const struct intel_crtc *intel_crtc;
> +
Don't we need to memset() ret_wm to zero here? Even if it's not needed
now because of some other previous memset, I fear future code changes
may introduce bugs due to non-zero values reaching this point.
> + list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
> + const struct intel_wm_level *wm =
> + &intel_crtc->wm.active.wm[level];
> +
> + if (!wm->enable)
> + return;
> +
> + 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;
Maybe it would be nice to add a comment to explain why we set this to
true, and who would set this to untrue in case it can't be true.
> +}
> +
> +/*
> + * Merge all low power watermarks for all active pipes.
> + */
> +static void ilk_wm_merge(struct drm_device *dev,
> + const struct hsw_wm_maximums *max,
> + struct intel_pipe_wm *merged)
> +{
> + int level, max_level = ilk_wm_max_level(dev);
> +
> + merged->fbc_wm_enabled = true;
> +
> + /* merge each WM1+ level */
> + for (level = 1; level <= max_level; level++) {
> + struct intel_wm_level *wm = &merged->wm[level];
> +
> + ilk_merge_wm_level(dev, level, wm);
> +
> + if (!ilk_check_wm(level, max, wm))
> + break;
> +
> + /*
> + * 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;
> + wm->fbc_val = 0;
> + }
> + }
> +}
> +
> static void hsw_compute_wm_results(struct drm_device *dev,
> const struct hsw_pipe_wm_parameters *params,
> const struct hsw_wm_maximums *lp_maximums,
> struct hsw_wm_values *results)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_crtc *crtc;
> - struct intel_wm_level lp_results[4] = {};
> - enum pipe pipe;
> - int level, max_level, wm_lp;
> + struct intel_crtc *intel_crtc;
> + int level, wm_lp;
> + struct intel_pipe_wm lp_wm = {};
Took me a while to realize that intel_pipe_wm isn't used only as the
WM values for a single pipe (as the name suggests), but also for the
merged WMs. Maybe we should rename the struct. And here maybe rename
from lp_wm to merged_wms?
>
> - for (level = 1; level <= 4; level++)
> - if (!hsw_compute_lp_wm(dev_priv, level,
> - lp_maximums, params,
> - &lp_results[level - 1]))
> - break;
> - max_level = level - 1;
> + list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
> + intel_compute_pipe_wm(&intel_crtc->base,
> + ¶ms[intel_crtc->pipe],
> + &intel_crtc->wm.active);
> +
> + ilk_wm_merge(dev, lp_maximums, &lp_wm);
>
> memset(results, 0, sizeof(*results));
>
> - /* The spec says it is preferred to disable FBC WMs instead of disabling
> - * a WM level. */
> - results->enable_fbc_wm = true;
> - for (level = 1; level <= max_level; level++) {
> - if (lp_results[level - 1].fbc_val > lp_maximums->fbc) {
> - results->enable_fbc_wm = false;
> - lp_results[level - 1].fbc_val = 0;
> - }
> - }
> + results->enable_fbc_wm = lp_wm.fbc_wm_enabled;
>
> + /* LP1+ register values */
> for (wm_lp = 1; wm_lp <= 3; wm_lp++) {
> const struct intel_wm_level *r;
>
> - level = (max_level == 4 && wm_lp > 1) ? wm_lp + 1 : wm_lp;
> - if (level > max_level)
> + level = wm_lp + (wm_lp >= 2 && lp_wm.wm[4].enable);
I find the older version easier to understand since it matches the
spec and doesn't rely on adding a boolean value. Also maybe we should
add a comment explaining that we do this since on HSW we have 4 levels
but only 3 slots (yes, I know, I should have added this comment when I
wrote the original code).
> +
> + r = &lp_wm.wm[level];
> + if (!r->enable)
> break;
>
> - r = &lp_results[level - 1];
> results->wm_lp[wm_lp - 1] = HSW_WM_LP_VAL(level * 2,
> r->fbc_val,
> r->pri_val,
> @@ -2715,13 +2745,21 @@ static void hsw_compute_wm_results(struct drm_device *dev,
> results->wm_lp_spr[wm_lp - 1] = r->spr_val;
> }
>
> - for_each_pipe(pipe)
> - results->wm_pipe[pipe] = hsw_compute_wm_pipe(dev,
> - ¶ms[pipe]);
> + /* LP0 register values */
> + list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
> + enum pipe pipe = intel_crtc->pipe;
> + const struct intel_wm_level *r =
> + &intel_crtc->wm.active.wm[0];
>
> - for_each_pipe(pipe) {
> - crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> - results->wm_linetime[pipe] = hsw_compute_linetime_wm(dev, crtc);
> + if (!r->enable)
> + continue;
Does this mean "pipe not active"? Maybe add a comment explaining. This
is confusing since we're talking about LP0 levels and they shouldn't
really be disabled.
Our WM code is getting quite complex, it's not very easy to follow.
The comments/renames I asked are for stuff that took me a while to
understand. I also wouldn't complain if "v2" comes as a series of
smaller patches.
I did test this patch on my Haswell machine and the values still seem
to be correct.
> +
> + results->wm_linetime[pipe] = intel_crtc->wm.active.linetime;
> +
> + results->wm_pipe[pipe] =
> + (r->pri_val << WM0_PIPE_PLANE_SHIFT) |
> + (r->spr_val << WM0_PIPE_SPRITE_SHIFT) |
> + r->cur_val;
> }
> }
>
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
