On Thu, 2015-08-20 at 18:12 -0700, Matt Roper wrote:
> In addition to calculating final watermarks, let's also pre-calculate a
> set of intermediate watermark values at atomic check time. These
> intermediate watermarks are a combination of the watermarks for the old
> state and the new state; they should satisfy the requirements of both
> states which means they can be programmed immediately when we commit the
> atomic state (without waiting for a vblank). Once the vblank does
> happen, we can then re-program watermarks to the more optimal final
> value.
>
> v2: Significant rebasing/rewriting.
>
> v3:
> - Move 'need_postvbl_update' flag to CRTC state (Daniel)
> - Don't forget to check intermediate watermark values for validity
> (Maarten)
> - Don't due async watermark optimization; just do it at the end of the
> atomic transaction, after waiting for vblanks. We do want it to be
> async eventually, but adding that now will cause more trouble for
> Maarten's in-progress work. (Maarten)
> - Don't allocate space in crtc_state for intermediate watermarks on
> platforms that don't need it (gen9+).
> - Move WaCxSRDisabledForSpriteScaling:ivb into intel_begin_crtc_commit
> now that ilk_update_wm is gone.
>
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 5 ++
> drivers/gpu/drm/i915/intel_display.c | 77 ++++++++++++++++++-
> drivers/gpu/drm/i915/intel_drv.h | 33 ++++++--
> drivers/gpu/drm/i915/intel_pm.c | 144 ++++++++++++++++++++++++-----------
> 4 files changed, 208 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ac13cd7..be42dd8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -622,6 +622,11 @@ struct drm_i915_display_funcs {
> struct dpll *best_clock);
> int (*compute_pipe_wm)(struct drm_crtc *crtc,
> struct drm_atomic_state *state);
> + int (*compute_intermediate_wm)(struct drm_device *dev,
> + struct intel_crtc_state *newstate,
> + const struct intel_crtc_state *oldstate);
> + void (*program_watermarks)(struct drm_i915_private *dev_priv);
> + void (*optimize_watermarks)(struct intel_crtc_state *cstate);
> void (*update_wm)(struct drm_crtc *crtc);
> int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8e9d87a..f929676 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11634,6 +11634,11 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> }
> } else if (intel_wm_need_update(plane, plane_state)) {
> intel_crtc->atomic.update_wm_pre = true;
> +
> + /* Pre-gen9 platforms need two-step watermark updates */
> + if (INTEL_INFO(dev)->gen < 9 &&
> + dev_priv->display.program_watermarks)
> + cstate->wm.need_postvbl_update = true;
> }
>
> if (visible)
> @@ -11769,6 +11774,8 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct intel_crtc_state *pipe_config =
> to_intel_crtc_state(crtc_state);
> + struct intel_crtc_state *old_pipe_config =
> + to_intel_crtc_state(crtc->state);
> struct drm_atomic_state *state = crtc_state->state;
> int ret;
> bool mode_changed = needs_modeset(crtc_state);
> @@ -11793,8 +11800,28 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
> ret = 0;
> if (dev_priv->display.compute_pipe_wm) {
> ret = dev_priv->display.compute_pipe_wm(crtc, state);
> - if (ret)
> + if (ret) {
> + DRM_DEBUG_KMS("Target pipe watermarks are invalid\n");
> + return ret;
> + }
> + }
> +
> + if (dev_priv->display.compute_intermediate_wm) {
> + if (WARN_ON(!dev_priv->display.compute_pipe_wm))
> + return 0;
> +
> + /*
> + * Calculate 'intermediate' watermarks that satisfy both the
> + * old state and the new state. We can program these
> + * immediately.
> + */
> + ret = dev_priv->display.compute_intermediate_wm(crtc->dev,
> + pipe_config,
> + old_pipe_config);
> + if (ret) {
> + DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
> return ret;
> + }
> }
>
> if (INTEL_INFO(dev)->gen >= 9) {
> @@ -13149,6 +13176,7 @@ static int intel_atomic_commit(struct drm_device *dev,
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
> + struct intel_crtc_state *intel_cstate;
> int ret = 0;
> int i;
> bool any_ms = false;
> @@ -13213,6 +13241,21 @@ static int intel_atomic_commit(struct drm_device *dev,
> /* FIXME: add subpixel order */
>
> drm_atomic_helper_wait_for_vblanks(dev, state);
> +
> + /*
> + * Now that the vblank has passed, we can go ahead and program the
> + * optimal watermarks on platforms that need two-step watermark
> + * programming.
> + *
> + * TODO: Move this (and other cleanup) to an async worker eventually.
> + */
> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> + intel_cstate = to_intel_crtc_state(crtc->state);
> +
> + if (intel_cstate->wm.need_postvbl_update)
> + dev_priv->display.optimize_watermarks(intel_cstate);
> + }
> +
> drm_atomic_helper_cleanup_planes(dev, state);
>
> if (any_ms)
> @@ -13544,10 +13587,40 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
> struct drm_crtc_state *old_crtc_state)
> {
> struct drm_device *dev = crtc->dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>
> - if (intel_crtc->atomic.update_wm_pre)
> + /*
> + * IVB workaround: must disable low power watermarks for at least
> + * one frame before enabling scaling. LP watermarks can be re-enabled
> + * when scaling is disabled.
> + *
> + * WaCxSRDisabledForSpriteScaling:ivb
> + */
> + if (to_intel_crtc_state(crtc->state)->disable_lp_wm) {
> + ilk_disable_lp_wm(crtc->dev);
> + intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
> + }
> +
> + /*
> + * For platforms that support atomic watermarks, program the 'active'
> + * watermarks immediately. On pre-gen9 platforms, these will be the
> + * intermediate values that are safe for both pre- and post- vblank;
> + * when vblank happens, the 'active' values will be set to the final
> + * 'target' values and we'll do this again to get the optimal
> + * watermarks. For gen9+ platforms, the values we program here will be
> + * the final target values which will get automatically latched at
> + * vblank time; no further programming will be necessary.
> + *
> + * If a platform hasn't been transitioned to atomic watermarks yet,
> + * we'll continue to update watermarks the old way, if flags tell
> + * us to.
> + */
> + if (dev_priv->display.program_watermarks != NULL) {
> + dev_priv->display.program_watermarks(dev_priv);
> + } else if (intel_crtc->atomic.update_wm_pre) {
> intel_update_watermarks(crtc);
> + }
>
> /* Perform vblank evasion around commit operation */
> if (crtc->state->active)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6ac1010c..6a73a53 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -476,14 +476,34 @@ struct intel_crtc_state {
> bool disable_lp_wm;
>
> struct {
> - /*
> - * final watermarks, programmed post-vblank when this state
> - * is committed
> - */
> union {
> - struct intel_pipe_wm ilk;
> + struct {
> + /*
> + * Final, target watermarks for state; this
> + * might not have been programmed yet if a
> + * vblank hasn't happened since this state
> + * was committed.
> + */
> + struct intel_pipe_wm target;
> +
> + /*
> + * currently programmed watermark; this will be
> + * the intermediate values before vblank then
> + * switch to match 'target' after vblank fires
> + */
> + struct intel_pipe_wm active;
> + } ilk;
> +
> struct skl_pipe_wm skl;
> - } active;
> + };
> +
> + /*
> + * Platforms with two-step watermark programming will need to
> + * update watermark programming post-vblank to switch from the
> + * safe intermediate watermarks to the optimal final
> + * watermarks.
> + */
> + bool need_postvbl_update;
> } wm;
> };
>
> @@ -1385,6 +1405,7 @@ void skl_wm_get_hw_state(struct drm_device *dev);
> void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
> struct skl_ddb_allocation *ddb /* out */);
> uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
> +bool ilk_disable_lp_wm(struct drm_device *dev);
>
> /* intel_sdvo.c */
> bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b32d6b0..e0a4c4d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2315,6 +2315,29 @@ static void skl_setup_wm_latency(struct drm_device *dev)
> intel_print_wm_latency(dev, "Gen9 Plane", dev_priv->wm.skl_latency);
> }
>
> +static bool ilk_validate_pipe_wm(struct drm_device *dev,
> + struct intel_pipe_wm *pipe_wm)
> +{
> + /* LP0 watermark maximums depend on this pipe alone */
> + const struct intel_wm_config config = {
> + .num_pipes_active = 1,
> + .sprites_enabled = pipe_wm->sprites_enabled,
> + .sprites_scaled = pipe_wm->sprites_scaled,
> + };
> + 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);
> +
> + /* At least LP0 must be valid */
> + if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0])) {
> + DRM_DEBUG_KMS("LP0 watermark invalid\n");
> + return false;
> + }
> +
> + return true;
> +}
> +
> /* Compute new watermarks for the pipe */
> static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> @@ -2331,10 +2354,6 @@ static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
> struct intel_plane_state *sprstate = NULL;
> struct intel_plane_state *curstate = NULL;
> int level, max_level = ilk_wm_max_level(dev);
> - /* LP0 watermark maximums depend on this pipe alone */
> - struct intel_wm_config config = {
> - .num_pipes_active = 1,
> - };
> struct ilk_wm_maximums max;
>
> cs = drm_atomic_get_crtc_state(state, crtc);
> @@ -2343,7 +2362,7 @@ static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
> else
> cstate = to_intel_crtc_state(cs);
>
> - pipe_wm = &cstate->wm.active.ilk;
> + pipe_wm = &cstate->wm.ilk.target;
>
> for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> ps = drm_atomic_get_plane_state(state,
> @@ -2359,21 +2378,18 @@ static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
> curstate = to_intel_plane_state(ps);
> }
>
> - config.sprites_enabled = sprstate->visible;
> - config.sprites_scaled =
> + pipe_wm->pipe_enabled = cstate->base.active;
> + pipe_wm->sprites_enabled = sprstate->visible;
> + pipe_wm->sprites_scaled =
> drm_rect_width(&sprstate->dst) != drm_rect_width(&sprstate->src) >> 16 ||
> drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16;
>
> - pipe_wm->pipe_enabled = cstate->base.active;
> - pipe_wm->sprites_enabled = config.sprites_enabled;
> - pipe_wm->sprites_scaled = config.sprites_scaled;
> -
> /* ILK/SNB: LP2+ watermarks only w/o sprites */
> if (INTEL_INFO(dev)->gen <= 6 && sprstate->visible)
> max_level = 1;
>
> /* ILK/SNB/IVB: LP1+ watermarks only w/o scaling */
> - if (config.sprites_scaled)
> + if (pipe_wm->sprites_scaled)
> max_level = 0;
>
> ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
> @@ -2382,12 +2398,8 @@ static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
> 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 */
> - if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0]))
> - return -EINVAL;
> + if (!ilk_validate_pipe_wm(dev, pipe_wm))
> + return false;
>
> ilk_compute_wm_reg_maximums(dev, 1, &max);
>
> @@ -2412,6 +2424,60 @@ static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
> }
>
> /*
> + * Build a set of 'intermediate' watermark values that satisfy both the old
> + * state and the new state. These can be programmed to the hardware
> + * immediately.
> + */
> +static int ilk_compute_intermediate_wm(struct drm_device *dev,
> + struct intel_crtc_state *newstate,
> + const struct intel_crtc_state *oldstate)
> +{
> + struct intel_pipe_wm *a = &newstate->wm.ilk.active;
Shouldn't this be ilk.target?
Also, I get a black screen with this patch on my HSW laptop. On boot, the level 0 wm has spr_val set
to 56, but the the hw state readout code doesn't set sprites_enabled properly. When the merged
active and target watermarks are checked for validity, the max value allowed for spr_val is 0
because sprites_enabled is 0, and that causes every modeset to fail.
Ander
> + const struct intel_pipe_wm *b = &oldstate->wm.ilk.active;
> + int level, max_level = ilk_wm_max_level(dev);
> +
> + /*
> + * Start with the final, target watermarks, then combine with the
> + * current state's watermarks to get values that are safe both
> + * before and after the vblank.
> + */
> + *a = newstate->wm.ilk.target;
> + a->pipe_enabled |= b->pipe_enabled;
> + a->sprites_enabled |= b->sprites_enabled;
> + a->sprites_scaled |= b->sprites_scaled;
> +
> + for (level = 0; level <= max_level; level++) {
> + struct intel_wm_level *a_wm = &a->wm[level];
> + const struct intel_wm_level *b_wm = &b->wm[level];
> +
> + a_wm->enable &= b_wm->enable;
> + a_wm->pri_val = max(a_wm->pri_val, b_wm->pri_val);
> + a_wm->spr_val = max(a_wm->spr_val, b_wm->spr_val);
> + a_wm->cur_val = max(a_wm->cur_val, b_wm->cur_val);
> + a_wm->fbc_val = max(a_wm->fbc_val, b_wm->fbc_val);
> + }
> +
> + /*
> + * We need to make sure that these merged watermark values are
> + * actually a valid configuration themselves. If they're not,
> + * there's no safe way to transition from the old state to
> + * the new state, so we need to fail the atomic transaction.
> + */
> + if (!ilk_validate_pipe_wm(dev, a))
> + return -EINVAL;
> +
> + /*
> + * If our intermediate WM are identical to the final WM, then we can
> + * omit the post-vblank programming.
> + */
> + if (!memcmp(&newstate->wm.ilk.active, &newstate->wm.ilk.target,
> + sizeof *a))
> + newstate->wm.need_postvbl_update = false;
> +
> + return 0;
> +}
> +
> +/*
> * Merge the watermarks from all active pipes for a specific level.
> */
> static void ilk_merge_wm_level(struct drm_device *dev,
> @@ -2425,7 +2491,7 @@ static void ilk_merge_wm_level(struct drm_device *dev,
> for_each_intel_crtc(dev, intel_crtc) {
> const struct intel_crtc_state *cstate =
> to_intel_crtc_state(intel_crtc->base.state);
> - const struct intel_pipe_wm *active = &cstate->wm.active.ilk;
> + const struct intel_pipe_wm *active = &cstate->wm.ilk.active;
> const struct intel_wm_level *wm = &active->wm[level];
>
> if (!active->pipe_enabled)
> @@ -2576,12 +2642,12 @@ static void ilk_compute_wm_results(struct drm_device *dev,
> const struct intel_crtc_state *cstate =
> to_intel_crtc_state(intel_crtc->base.state);
> enum pipe pipe = intel_crtc->pipe;
> - const struct intel_wm_level *r = &cstate->wm.active.ilk.wm[0];
> + const struct intel_wm_level *r = &cstate->wm.ilk.active.wm[0];
>
> if (WARN_ON(!r->enable))
> continue;
>
> - results->wm_linetime[pipe] = cstate->wm.active.ilk.linetime;
> + results->wm_linetime[pipe] = cstate->wm.ilk.active.linetime;
>
> results->wm_pipe[pipe] =
> (r->pri_val << WM0_PIPE_PLANE_SHIFT) |
> @@ -2788,7 +2854,7 @@ static void ilk_write_wm_values(struct drm_i915_private *dev_priv,
> dev_priv->wm.hw = *results;
> }
>
> -static bool ilk_disable_lp_wm(struct drm_device *dev)
> +bool ilk_disable_lp_wm(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> @@ -3544,10 +3610,10 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
> skl_allocate_pipe_ddb(crtc, cstate, ddb);
> skl_compute_pipe_wm(crtc, ddb, cstate, pipe_wm);
>
> - if (!memcmp(&cstate->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
> + if (!memcmp(&cstate->wm.skl, pipe_wm, sizeof(*pipe_wm)))
> return false;
>
> - cstate->wm.active.skl = *pipe_wm;
> + cstate->wm.skl = *pipe_wm;
>
> return true;
> }
> @@ -3653,24 +3719,11 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
> ilk_write_wm_values(dev_priv, &results);
> }
>
> -static void ilk_update_wm(struct drm_crtc *crtc)
> +static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
> {
> - struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> -
> - /*
> - * IVB workaround: must disable low power watermarks for at least
> - * one frame before enabling scaling. LP watermarks can be re-enabled
> - * when scaling is disabled.
> - *
> - * WaCxSRDisabledForSpriteScaling:ivb
> - */
> - if (cstate->disable_lp_wm) {
> - ilk_disable_lp_wm(crtc->dev);
> - intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
> - }
> + struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
>
> + cstate->wm.ilk.active = cstate->wm.ilk.target;
> ilk_program_watermarks(dev_priv);
> }
>
> @@ -3725,7 +3778,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> struct skl_wm_values *hw = &dev_priv->wm.skl_hw;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> - struct skl_pipe_wm *active = &cstate->wm.active.skl;
> + struct skl_pipe_wm *active = &cstate->wm.skl;
> enum pipe pipe = intel_crtc->pipe;
> int level, i, max_level;
> uint32_t temp;
> @@ -3789,7 +3842,7 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> struct ilk_wm_values *hw = &dev_priv->wm.hw;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> - struct intel_pipe_wm *active = &cstate->wm.active.ilk;
> + struct intel_pipe_wm *active = &cstate->wm.ilk.active;
> enum pipe pipe = intel_crtc->pipe;
> static const unsigned int wm0_pipe_reg[] = {
> [PIPE_A] = WM0_PIPEA_ILK,
> @@ -3828,6 +3881,8 @@ static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> for (level = 0; level <= max_level; level++)
> active->wm[level].enable = true;
> }
> +
> + cstate->wm.ilk.target = *active;
> }
>
> #define _FW_WM(value, plane) \
> @@ -6949,8 +7004,11 @@ void intel_init_pm(struct drm_device *dev)
> dev_priv->wm.spr_latency[1] && dev_priv->wm.cur_latency[1]) ||
> (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] &&
> dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
> - dev_priv->display.update_wm = ilk_update_wm;
> dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
> + dev_priv->display.compute_intermediate_wm =
> + ilk_compute_intermediate_wm;
> + dev_priv->display.program_watermarks = ilk_program_watermarks;
> + dev_priv->display.optimize_watermarks = ilk_optimize_watermarks;
> } else {
> DRM_DEBUG_KMS("Failed to read display plane latency. "
> "Disable CxSR\n");
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
