intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Add two-stage ILK-style watermark programming (v11)
@ 2016-02-24  1:20 Matt Roper
  2016-02-24  1:24 ` Matt Roper
  2016-02-25 16:48 ` Matt Roper
  0 siblings, 2 replies; 7+ messages in thread
From: Matt Roper @ 2016-02-24  1:20 UTC (permalink / raw)
  To: intel-gfx

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.

v4:
 - Add a wm_mutex to cover updates to intel_crtc->active and the
   need_postvbl_update flag.  Since we don't have async yet it isn't
   terribly important yet, but might as well add it now.
 - Change interface to program watermarks.  Platforms will now expose
   .initial_watermarks() and .optimize_watermarks() functions to do
   watermark programming.  These should lock wm_mutex, copy the
   appropriate state values into intel_crtc->active, and then call
   the internal program watermarks function.

v5:
 - Skip intermediate watermark calculation/check during initial hardware
   readout since we don't trust the existing HW values (and don't have
   valid values of our own yet).
 - Don't try to call .optimize_watermarks() on platforms that don't have
   atomic watermarks yet.  (Maarten)

v6:
 - Rebase

v7:
 - Further rebase

v8:
 - A few minor indentation and line length fixes

v9:
 - Yet another rebase since Maarten's patches reworked a bunch of the
   code (wm_pre, wm_post, etc.) that this was previously based on.

v10:
 - Move wm_mutex to dev_priv to protect against racing commits against
   disjoint CRTC sets. (Maarten)
 - Drop unnecessary clearing of cstate->wm.need_postvbl_update (Maarten)

v11:
 - Now that we've moved to atomic watermark updates, make sure we call
   the proper function to program watermarks in
   {ironlake,haswell}_crtc_enable(); the failure to do so on the
   previous patch iteration led to us not actually programming the
   watermarks before turning on the CRTC, which was the cause of the
   underruns that the CI system was seeing.
 - Fix inverted logic for determining when to optimize watermarks.  We
   were needlessly optimizing when the intermediate/optimal values were
   the same (harmless), but not actually optimizing when they differed
   (also harmless, but wasteful from a power/bandwidth perspective).

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |   1 +
 drivers/gpu/drm/i915/i915_drv.h      |  13 ++-
 drivers/gpu/drm/i915/intel_atomic.c  |   1 +
 drivers/gpu/drm/i915/intel_display.c |  97 +++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |  28 +++++-
 drivers/gpu/drm/i915/intel_pm.c      | 162 ++++++++++++++++++++++++-----------
 6 files changed, 244 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 1c6d227..36c0cf1 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1010,6 +1010,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	mutex_init(&dev_priv->sb_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
 	mutex_init(&dev_priv->av_mutex);
+	mutex_init(&dev_priv->wm.wm_mutex);
 
 	ret = i915_workqueues_init(dev_priv);
 	if (ret < 0)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9e76bfc..cf17d62 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -631,7 +631,11 @@ struct drm_i915_display_funcs {
 			  struct dpll *best_clock);
 	int (*compute_pipe_wm)(struct intel_crtc *crtc,
 			       struct drm_atomic_state *state);
-	void (*program_watermarks)(struct intel_crtc_state *cstate);
+	int (*compute_intermediate_wm)(struct drm_device *dev,
+				       struct intel_crtc *intel_crtc,
+				       struct intel_crtc_state *newstate);
+	void (*initial_watermarks)(struct intel_crtc_state *cstate);
+	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);
@@ -1980,6 +1984,13 @@ struct drm_i915_private {
 		};
 
 		uint8_t max_level;
+
+		/*
+		 * Should be held around atomic WM register writing; also
+		 * protects * intel_crtc->wm.active and
+		 * cstate->wm.need_postvbl_update.
+		 */
+		struct mutex wm_mutex;
 	} wm;
 
 	struct i915_runtime_pm pm;
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 4625f8a..9682d94 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 	crtc_state->disable_lp_wm = false;
 	crtc_state->disable_cxsr = false;
 	crtc_state->wm_changed = false;
+	crtc_state->wm.need_postvbl_update = false;
 
 	return &crtc_state->base;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index deee560..423b99e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4846,7 +4846,42 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 			intel_set_memory_cxsr(dev_priv, false);
 	}
 
-	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
+	/*
+	 * 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 (pipe_config->disable_lp_wm) {
+		ilk_disable_lp_wm(dev);
+		intel_wait_for_vblank(dev, crtc->pipe);
+	}
+
+	/*
+	 * If we're doing a modeset, we're done.  No need to do any pre-vblank
+	 * watermark programming here.
+	 */
+	if (needs_modeset(&pipe_config->base))
+		return;
+
+	/*
+	 * For platforms that support atomic watermarks, program the
+	 * 'intermediate' 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.initial_watermarks != NULL)
+		dev_priv->display.initial_watermarks(pipe_config);
+	else if (pipe_config->wm_changed)
 		intel_update_watermarks(&crtc->base);
 }
 
@@ -4925,7 +4960,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	 */
 	intel_crtc_load_lut(crtc);
 
-	intel_update_watermarks(crtc);
+	dev_priv->display.initial_watermarks(intel_crtc->config);
 	intel_enable_pipe(intel_crtc);
 
 	if (intel_crtc->config->has_pch_encoder)
@@ -5024,7 +5059,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	if (!intel_crtc->config->has_dsi_encoder)
 		intel_ddi_enable_transcoder_func(crtc);
 
-	intel_update_watermarks(crtc);
+	dev_priv->display.initial_watermarks(pipe_config);
 	intel_enable_pipe(intel_crtc);
 
 	if (intel_crtc->config->has_pch_encoder)
@@ -11800,6 +11835,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_plane *plane = plane_state->plane;
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane_state *old_plane_state =
 		to_intel_plane_state(plane->state);
 	int idx = intel_crtc->base.base.id, ret;
@@ -11859,6 +11895,11 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 		pipe_config->wm_changed = true;
 	}
 
+	/* Pre-gen9 platforms need two-step watermark updates */
+	if (pipe_config->wm_changed && INTEL_INFO(dev)->gen < 9 &&
+	    dev_priv->display.optimize_watermarks)
+		to_intel_crtc_state(crtc_state)->wm.need_postvbl_update = true;
+
 	if (visible || was_visible)
 		intel_crtc->atomic.fb_bits |=
 			to_intel_plane(plane)->frontbuffer_bit;
@@ -11983,8 +12024,29 @@ 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(intel_crtc, state);
-		if (ret)
+		if (ret) {
+			DRM_DEBUG_KMS("Target pipe watermarks are invalid\n");
 			return ret;
+		}
+	}
+
+	if (dev_priv->display.compute_intermediate_wm &&
+	    !to_intel_atomic_state(state)->skip_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,
+								intel_crtc,
+								pipe_config);
+		if (ret) {
+			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
+			return ret;
+		}
 	}
 
 	if (INTEL_INFO(dev)->gen >= 9) {
@@ -13452,6 +13514,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
+	struct intel_crtc_state *intel_cstate;
 	int ret = 0, i;
 	bool hw_check = intel_state->modeset;
 
@@ -13555,6 +13618,20 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	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 (dev_priv->display.optimize_watermarks)
+			dev_priv->display.optimize_watermarks(intel_cstate);
+	}
+
 	mutex_lock(&dev->struct_mutex);
 	drm_atomic_helper_cleanup_planes(dev, state);
 	mutex_unlock(&dev->struct_mutex);
@@ -15225,7 +15302,7 @@ static void sanitize_watermarks(struct drm_device *dev)
 	int i;
 
 	/* Only supported on platforms that use atomic watermark design */
-	if (!dev_priv->display.program_watermarks)
+	if (!dev_priv->display.optimize_watermarks)
 		return;
 
 	/*
@@ -15246,6 +15323,13 @@ retry:
 	if (WARN_ON(IS_ERR(state)))
 		goto fail;
 
+	/*
+	 * Hardware readout is the only time we don't want to calculate
+	 * intermediate watermarks (since we don't trust the current
+	 * watermarks).
+	 */
+	to_intel_atomic_state(state)->skip_intermediate_wm = true;
+
 	ret = intel_atomic_check(dev, state);
 	if (ret) {
 		/*
@@ -15268,7 +15352,8 @@ retry:
 	for_each_crtc_in_state(state, crtc, cstate, i) {
 		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
 
-		dev_priv->display.program_watermarks(cs);
+		cs->wm.need_postvbl_update = true;
+		dev_priv->display.optimize_watermarks(cs);
 	}
 
 	drm_atomic_state_free(state);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4852049..a9f7641 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -260,6 +260,12 @@ struct intel_atomic_state {
 
 	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
 	struct intel_wm_config wm_config;
+
+	/*
+	 * Current watermarks can't be trusted during hardware readout, so
+	 * don't bother calculating intermediate watermarks.
+	 */
+	bool skip_intermediate_wm;
 };
 
 struct intel_plane_state {
@@ -509,13 +515,29 @@ struct intel_crtc_state {
 
 	struct {
 		/*
-		 * optimal watermarks, programmed post-vblank when this state
-		 * is committed
+		 * Optimal watermarks, programmed post-vblank when this state
+		 * is committed.
 		 */
 		union {
 			struct intel_pipe_wm ilk;
 			struct skl_pipe_wm skl;
 		} optimal;
+
+		/*
+		 * Intermediate watermarks; these can be programmed immediately
+		 * since they satisfy both the current configuration we're
+		 * switching away from and the new configuration we're switching
+		 * to.
+		 */
+		struct intel_pipe_wm intermediate;
+
+		/*
+		 * 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;
 };
 
@@ -601,6 +623,7 @@ struct intel_crtc {
 			struct intel_pipe_wm ilk;
 			struct skl_pipe_wm skl;
 		} active;
+
 		/* allow CxSR on this pipe */
 		bool cxsr_allowed;
 	} wm;
@@ -1566,6 +1589,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);
 int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6);
 
 /* intel_sdvo.c */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 347d4df..ccdb581 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2278,6 +2278,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 intel_crtc *intel_crtc,
 			       struct drm_atomic_state *state)
@@ -2292,10 +2315,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_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;
 
 	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
@@ -2319,21 +2338,18 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 			curstate = to_intel_plane_state(ps);
 	}
 
-	config.sprites_enabled = sprstate->visible;
-	config.sprites_scaled = sprstate->visible &&
+	pipe_wm->pipe_enabled = cstate->base.active;
+	pipe_wm->sprites_enabled = sprstate->visible;
+	pipe_wm->sprites_scaled = sprstate->visible &&
 		(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,
@@ -2342,12 +2358,8 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
 	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		pipe_wm->linetime = hsw_compute_linetime_wm(dev, cstate);
 
-	/* 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);
 
@@ -2372,6 +2384,59 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_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 *intel_crtc,
+				       struct intel_crtc_state *newstate)
+{
+	struct intel_pipe_wm *a = &newstate->wm.intermediate;
+	struct intel_pipe_wm *b = &intel_crtc->wm.active.ilk;
+	int level, max_level = ilk_wm_max_level(dev);
+
+	/*
+	 * Start with the final, target watermarks, then combine with the
+	 * currently active watermarks to get values that are safe both before
+	 * and after the vblank.
+	 */
+	*a = newstate->wm.optimal.ilk;
+	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; only update if it's different.
+	 */
+	if (memcmp(a, &newstate->wm.optimal.ilk, sizeof(*a)) == 0)
+		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,
@@ -2383,9 +2448,7 @@ static void ilk_merge_wm_level(struct drm_device *dev,
 	ret_wm->enable = true;
 
 	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.optimal.ilk;
+		const struct intel_pipe_wm *active = &intel_crtc->wm.active.ilk;
 		const struct intel_wm_level *wm = &active->wm[level];
 
 		if (!active->pipe_enabled)
@@ -2533,15 +2596,14 @@ static void ilk_compute_wm_results(struct drm_device *dev,
 
 	/* LP0 register values */
 	for_each_intel_crtc(dev, intel_crtc) {
-		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.optimal.ilk.wm[0];
+		const struct intel_wm_level *r =
+			&intel_crtc->wm.active.ilk.wm[0];
 
 		if (WARN_ON(!r->enable))
 			continue;
 
-		results->wm_linetime[pipe] = cstate->wm.optimal.ilk.linetime;
+		results->wm_linetime[pipe] = intel_crtc->wm.active.ilk.linetime;
 
 		results->wm_pipe[pipe] =
 			(r->pri_val << WM0_PIPE_PLANE_SHIFT) |
@@ -2748,7 +2810,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;
 
@@ -3643,11 +3705,9 @@ static void ilk_compute_wm_config(struct drm_device *dev,
 	}
 }
 
-static void ilk_program_watermarks(struct intel_crtc_state *cstate)
+static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
 {
-	struct drm_crtc *crtc = cstate->base.crtc;
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_device *dev = dev_priv->dev;
 	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
 	struct ilk_wm_maximums max;
 	struct intel_wm_config config = {};
@@ -3678,28 +3738,28 @@ static void ilk_program_watermarks(struct intel_crtc_state *cstate)
 	ilk_write_wm_values(dev_priv, &results);
 }
 
-static void ilk_update_wm(struct drm_crtc *crtc)
+static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
-
-	WARN_ON(cstate->base.active != intel_crtc->active);
+	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
 
-	/*
-	 * 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);
-	}
+	mutex_lock(&dev_priv->wm.wm_mutex);
+	intel_crtc->wm.active.ilk = cstate->wm.intermediate;
+	ilk_program_watermarks(dev_priv);
+	mutex_unlock(&dev_priv->wm.wm_mutex);
+}
 
-	intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
+static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
+{
+	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
 
-	ilk_program_watermarks(cstate);
+	mutex_lock(&dev_priv->wm.wm_mutex);
+	if (cstate->wm.need_postvbl_update) {
+		intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
+		ilk_program_watermarks(dev_priv);
+	}
+	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
 
 static void skl_pipe_wm_active_state(uint32_t val,
@@ -7076,9 +7136,13 @@ 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.program_watermarks = ilk_program_watermarks;
+			dev_priv->display.compute_intermediate_wm =
+				ilk_compute_intermediate_wm;
+			dev_priv->display.initial_watermarks =
+				ilk_initial_watermarks;
+			dev_priv->display.optimize_watermarks =
+				ilk_optimize_watermarks;
 		} else {
 			DRM_DEBUG_KMS("Failed to read display plane latency. "
 				      "Disable CxSR\n");
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/i915: Add two-stage ILK-style watermark programming (v11)
  2016-02-24  1:20 [PATCH] drm/i915: Add two-stage ILK-style watermark programming (v11) Matt Roper
@ 2016-02-24  1:24 ` Matt Roper
  2016-02-24  8:20   ` Maarten Lankhorst
  2016-02-25 16:48 ` Matt Roper
  1 sibling, 1 reply; 7+ messages in thread
From: Matt Roper @ 2016-02-24  1:24 UTC (permalink / raw)
  To: intel-gfx

On Tue, Feb 23, 2016 at 05:20:13PM -0800, 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.
> 
> v4:
>  - Add a wm_mutex to cover updates to intel_crtc->active and the
>    need_postvbl_update flag.  Since we don't have async yet it isn't
>    terribly important yet, but might as well add it now.
>  - Change interface to program watermarks.  Platforms will now expose
>    .initial_watermarks() and .optimize_watermarks() functions to do
>    watermark programming.  These should lock wm_mutex, copy the
>    appropriate state values into intel_crtc->active, and then call
>    the internal program watermarks function.
> 
> v5:
>  - Skip intermediate watermark calculation/check during initial hardware
>    readout since we don't trust the existing HW values (and don't have
>    valid values of our own yet).
>  - Don't try to call .optimize_watermarks() on platforms that don't have
>    atomic watermarks yet.  (Maarten)
> 
> v6:
>  - Rebase
> 
> v7:
>  - Further rebase
> 
> v8:
>  - A few minor indentation and line length fixes
> 
> v9:
>  - Yet another rebase since Maarten's patches reworked a bunch of the
>    code (wm_pre, wm_post, etc.) that this was previously based on.
> 
> v10:
>  - Move wm_mutex to dev_priv to protect against racing commits against
>    disjoint CRTC sets. (Maarten)
>  - Drop unnecessary clearing of cstate->wm.need_postvbl_update (Maarten)
> 
> v11:
>  - Now that we've moved to atomic watermark updates, make sure we call
>    the proper function to program watermarks in
>    {ironlake,haswell}_crtc_enable(); the failure to do so on the
>    previous patch iteration led to us not actually programming the
>    watermarks before turning on the CRTC, which was the cause of the
>    underruns that the CI system was seeing.
>  - Fix inverted logic for determining when to optimize watermarks.  We
>    were needlessly optimizing when the intermediate/optimal values were
>    the same (harmless), but not actually optimizing when they differed
>    (also harmless, but wasteful from a power/bandwidth perspective).
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---

To assist with review, the non-rebasing changes in this iteration vs the
last one are:

> @@ -4925,7 +4960,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	 */
>  	intel_crtc_load_lut(crtc);
>  
> -	intel_update_watermarks(crtc);
> +	dev_priv->display.initial_watermarks(intel_crtc->config);
>  	intel_enable_pipe(intel_crtc);
>  
>  	if (intel_crtc->config->has_pch_encoder)
> @@ -5024,7 +5059,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	if (!intel_crtc->config->has_dsi_encoder)
>  		intel_ddi_enable_transcoder_func(crtc);
>  
> -	intel_update_watermarks(crtc);
> +	dev_priv->display.initial_watermarks(pipe_config);
>  	intel_enable_pipe(intel_crtc);
>  
>  	if (intel_crtc->config->has_pch_encoder)

(both new additions to the patch)

and:

> +	/*
> +	 * If our intermediate WM are identical to the final WM, then we can
> +	 * omit the post-vblank programming; only update if it's different.
> +	 */
> +	if (memcmp(a, &newstate->wm.optimal.ilk, sizeof(*a)) == 0)
> +		newstate->wm.need_postvbl_update = false;

(replacement of a "!=" with "==")


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/i915: Add two-stage ILK-style watermark programming (v11)
  2016-02-24  1:24 ` Matt Roper
@ 2016-02-24  8:20   ` Maarten Lankhorst
  0 siblings, 0 replies; 7+ messages in thread
From: Maarten Lankhorst @ 2016-02-24  8:20 UTC (permalink / raw)
  To: Matt Roper, intel-gfx

Op 24-02-16 om 02:24 schreef Matt Roper:
> On Tue, Feb 23, 2016 at 05:20:13PM -0800, 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.
>>
>> v4:
>>  - Add a wm_mutex to cover updates to intel_crtc->active and the
>>    need_postvbl_update flag.  Since we don't have async yet it isn't
>>    terribly important yet, but might as well add it now.
>>  - Change interface to program watermarks.  Platforms will now expose
>>    .initial_watermarks() and .optimize_watermarks() functions to do
>>    watermark programming.  These should lock wm_mutex, copy the
>>    appropriate state values into intel_crtc->active, and then call
>>    the internal program watermarks function.
>>
>> v5:
>>  - Skip intermediate watermark calculation/check during initial hardware
>>    readout since we don't trust the existing HW values (and don't have
>>    valid values of our own yet).
>>  - Don't try to call .optimize_watermarks() on platforms that don't have
>>    atomic watermarks yet.  (Maarten)
>>
>> v6:
>>  - Rebase
>>
>> v7:
>>  - Further rebase
>>
>> v8:
>>  - A few minor indentation and line length fixes
>>
>> v9:
>>  - Yet another rebase since Maarten's patches reworked a bunch of the
>>    code (wm_pre, wm_post, etc.) that this was previously based on.
>>
>> v10:
>>  - Move wm_mutex to dev_priv to protect against racing commits against
>>    disjoint CRTC sets. (Maarten)
>>  - Drop unnecessary clearing of cstate->wm.need_postvbl_update (Maarten)
>>
>> v11:
>>  - Now that we've moved to atomic watermark updates, make sure we call
>>    the proper function to program watermarks in
>>    {ironlake,haswell}_crtc_enable(); the failure to do so on the
>>    previous patch iteration led to us not actually programming the
>>    watermarks before turning on the CRTC, which was the cause of the
>>    underruns that the CI system was seeing.
>>  - Fix inverted logic for determining when to optimize watermarks.  We
>>    were needlessly optimizing when the intermediate/optimal values were
>>    the same (harmless), but not actually optimizing when they differed
>>    (also harmless, but wasteful from a power/bandwidth perspective).
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> ---
> To assist with review, the non-rebasing changes in this iteration vs the
> last one are:
>
>> @@ -4925,7 +4960,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>>  	 */
>>  	intel_crtc_load_lut(crtc);
>>  
>> -	intel_update_watermarks(crtc);
>> +	dev_priv->display.initial_watermarks(intel_crtc->config);
>>  	intel_enable_pipe(intel_crtc);
>>  
>>  	if (intel_crtc->config->has_pch_encoder)
>> @@ -5024,7 +5059,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>>  	if (!intel_crtc->config->has_dsi_encoder)
>>  		intel_ddi_enable_transcoder_func(crtc);
>>  
>> -	intel_update_watermarks(crtc);
>> +	dev_priv->display.initial_watermarks(pipe_config);
>>  	intel_enable_pipe(intel_crtc);
>>  
>>  	if (intel_crtc->config->has_pch_encoder)
Are the intermediate watermarks identical to optimal watermarks during modeset?
> (both new additions to the patch)
>
> and:
>
>> +	/*
>> +	 * If our intermediate WM are identical to the final WM, then we can
>> +	 * omit the post-vblank programming; only update if it's different.
>> +	 */
>> +	if (memcmp(a, &newstate->wm.optimal.ilk, sizeof(*a)) == 0)
>> +		newstate->wm.need_postvbl_update = false;
> (replacement of a "!=" with "==")
Ah, good catch!

Lets just wait for CI before applying again
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/i915: Add two-stage ILK-style watermark programming (v11)
  2016-02-24  1:20 [PATCH] drm/i915: Add two-stage ILK-style watermark programming (v11) Matt Roper
  2016-02-24  1:24 ` Matt Roper
@ 2016-02-25 16:48 ` Matt Roper
  2016-02-29  8:23   ` Maarten Lankhorst
  2016-02-29 19:52   ` Imre Deak
  1 sibling, 2 replies; 7+ messages in thread
From: Matt Roper @ 2016-02-25 16:48 UTC (permalink / raw)
  To: intel-gfx

I never got a CI email for this (probably because fdo was down for a while),
but I can see the results below in patchwork

>  Test gem_cs_prefetch:
>          Subgroup basic-default:
>                  incomplete -> PASS       (ilk-hp8440p)
>  Test kms_flip:
>          Subgroup basic-flip-vs-dpms:
>                  pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE

Pre-existing watermark bug here:
        https://bugs.freedesktop.org/show_bug.cgi?id=93787

I had hoped this patch might also fix that problem, but I guess further
investigation will be needed; it seems to be something very
ILK-specific, not seen on SNB, IVB, etc.


>  Test kms_force_connector_basic:
>          Subgroup force-edid:
>                  skip       -> PASS       (snb-x220t)
>                  pass       -> SKIP       (ivb-t430s)

Pre-existing:
        https://bugs.freedesktop.org/show_bug.cgi?id=93769

>          Subgroup force-load-detect:
>                  dmesg-fail -> FAIL       (snb-x220t)
>                  dmesg-fail -> FAIL       (hsw-gt2)

The dmesg warning is gone now (a locking warning that I don't think my
patch should have affected); the remaining test failure is identical to
what it was before (temp->connection != DRM_MODE_UNKNOWNCONNECTION) and
unrelated.

I don't see a bugzilla for this, which seems strange since it's a
pre-existing defect.  Am I overlooking it or do I need to file a new
one?

>                  fail       -> SKIP       (ilk-hp8440p)

Not sure what is actually plugged into the CI machine, so the skip may
be legitimate/expected?  Doesn't seem related to anything in my patch
anyway.

>  Test kms_pipe_crc_basic:
>          Subgroup suspend-read-crc-pipe-c:
>                  pass       -> DMESG-WARN (bsw-nuc-2)

This one was happening sporadically before my patch; I think it's the
same bug filed here:

  https://bugs.freedesktop.org/show_bug.cgi?id=94164

I'll update the bugzilla to indicate it also happens on this test.

>  Test pm_rpm:
>          Subgroup basic-rte:
>                  pass       -> DMESG-WARN (bsw-nuc-2)
>                  fail       -> PASS       (hsw-brixbox)

Same bug mentioned above (94164)



Matt


On Tue, Feb 23, 2016 at 05:20:13PM -0800, 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.
> 
> v4:
>  - Add a wm_mutex to cover updates to intel_crtc->active and the
>    need_postvbl_update flag.  Since we don't have async yet it isn't
>    terribly important yet, but might as well add it now.
>  - Change interface to program watermarks.  Platforms will now expose
>    .initial_watermarks() and .optimize_watermarks() functions to do
>    watermark programming.  These should lock wm_mutex, copy the
>    appropriate state values into intel_crtc->active, and then call
>    the internal program watermarks function.
> 
> v5:
>  - Skip intermediate watermark calculation/check during initial hardware
>    readout since we don't trust the existing HW values (and don't have
>    valid values of our own yet).
>  - Don't try to call .optimize_watermarks() on platforms that don't have
>    atomic watermarks yet.  (Maarten)
> 
> v6:
>  - Rebase
> 
> v7:
>  - Further rebase
> 
> v8:
>  - A few minor indentation and line length fixes
> 
> v9:
>  - Yet another rebase since Maarten's patches reworked a bunch of the
>    code (wm_pre, wm_post, etc.) that this was previously based on.
> 
> v10:
>  - Move wm_mutex to dev_priv to protect against racing commits against
>    disjoint CRTC sets. (Maarten)
>  - Drop unnecessary clearing of cstate->wm.need_postvbl_update (Maarten)
> 
> v11:
>  - Now that we've moved to atomic watermark updates, make sure we call
>    the proper function to program watermarks in
>    {ironlake,haswell}_crtc_enable(); the failure to do so on the
>    previous patch iteration led to us not actually programming the
>    watermarks before turning on the CRTC, which was the cause of the
>    underruns that the CI system was seeing.
>  - Fix inverted logic for determining when to optimize watermarks.  We
>    were needlessly optimizing when the intermediate/optimal values were
>    the same (harmless), but not actually optimizing when they differed
>    (also harmless, but wasteful from a power/bandwidth perspective).
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |   1 +
>  drivers/gpu/drm/i915/i915_drv.h      |  13 ++-
>  drivers/gpu/drm/i915/intel_atomic.c  |   1 +
>  drivers/gpu/drm/i915/intel_display.c |  97 +++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h     |  28 +++++-
>  drivers/gpu/drm/i915/intel_pm.c      | 162 ++++++++++++++++++++++++-----------
>  6 files changed, 244 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 1c6d227..36c0cf1 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1010,6 +1010,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	mutex_init(&dev_priv->sb_lock);
>  	mutex_init(&dev_priv->modeset_restore_lock);
>  	mutex_init(&dev_priv->av_mutex);
> +	mutex_init(&dev_priv->wm.wm_mutex);
>  
>  	ret = i915_workqueues_init(dev_priv);
>  	if (ret < 0)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9e76bfc..cf17d62 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -631,7 +631,11 @@ struct drm_i915_display_funcs {
>  			  struct dpll *best_clock);
>  	int (*compute_pipe_wm)(struct intel_crtc *crtc,
>  			       struct drm_atomic_state *state);
> -	void (*program_watermarks)(struct intel_crtc_state *cstate);
> +	int (*compute_intermediate_wm)(struct drm_device *dev,
> +				       struct intel_crtc *intel_crtc,
> +				       struct intel_crtc_state *newstate);
> +	void (*initial_watermarks)(struct intel_crtc_state *cstate);
> +	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);
> @@ -1980,6 +1984,13 @@ struct drm_i915_private {
>  		};
>  
>  		uint8_t max_level;
> +
> +		/*
> +		 * Should be held around atomic WM register writing; also
> +		 * protects * intel_crtc->wm.active and
> +		 * cstate->wm.need_postvbl_update.
> +		 */
> +		struct mutex wm_mutex;
>  	} wm;
>  
>  	struct i915_runtime_pm pm;
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 4625f8a..9682d94 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  	crtc_state->disable_lp_wm = false;
>  	crtc_state->disable_cxsr = false;
>  	crtc_state->wm_changed = false;
> +	crtc_state->wm.need_postvbl_update = false;
>  
>  	return &crtc_state->base;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index deee560..423b99e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4846,7 +4846,42 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
>  			intel_set_memory_cxsr(dev_priv, false);
>  	}
>  
> -	if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
> +	/*
> +	 * 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 (pipe_config->disable_lp_wm) {
> +		ilk_disable_lp_wm(dev);
> +		intel_wait_for_vblank(dev, crtc->pipe);
> +	}
> +
> +	/*
> +	 * If we're doing a modeset, we're done.  No need to do any pre-vblank
> +	 * watermark programming here.
> +	 */
> +	if (needs_modeset(&pipe_config->base))
> +		return;
> +
> +	/*
> +	 * For platforms that support atomic watermarks, program the
> +	 * 'intermediate' 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.initial_watermarks != NULL)
> +		dev_priv->display.initial_watermarks(pipe_config);
> +	else if (pipe_config->wm_changed)
>  		intel_update_watermarks(&crtc->base);
>  }
>  
> @@ -4925,7 +4960,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	 */
>  	intel_crtc_load_lut(crtc);
>  
> -	intel_update_watermarks(crtc);
> +	dev_priv->display.initial_watermarks(intel_crtc->config);
>  	intel_enable_pipe(intel_crtc);
>  
>  	if (intel_crtc->config->has_pch_encoder)
> @@ -5024,7 +5059,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	if (!intel_crtc->config->has_dsi_encoder)
>  		intel_ddi_enable_transcoder_func(crtc);
>  
> -	intel_update_watermarks(crtc);
> +	dev_priv->display.initial_watermarks(pipe_config);
>  	intel_enable_pipe(intel_crtc);
>  
>  	if (intel_crtc->config->has_pch_encoder)
> @@ -11800,6 +11835,7 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct drm_plane *plane = plane_state->plane;
>  	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane_state *old_plane_state =
>  		to_intel_plane_state(plane->state);
>  	int idx = intel_crtc->base.base.id, ret;
> @@ -11859,6 +11895,11 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  		pipe_config->wm_changed = true;
>  	}
>  
> +	/* Pre-gen9 platforms need two-step watermark updates */
> +	if (pipe_config->wm_changed && INTEL_INFO(dev)->gen < 9 &&
> +	    dev_priv->display.optimize_watermarks)
> +		to_intel_crtc_state(crtc_state)->wm.need_postvbl_update = true;
> +
>  	if (visible || was_visible)
>  		intel_crtc->atomic.fb_bits |=
>  			to_intel_plane(plane)->frontbuffer_bit;
> @@ -11983,8 +12024,29 @@ 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(intel_crtc, state);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEBUG_KMS("Target pipe watermarks are invalid\n");
>  			return ret;
> +		}
> +	}
> +
> +	if (dev_priv->display.compute_intermediate_wm &&
> +	    !to_intel_atomic_state(state)->skip_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,
> +								intel_crtc,
> +								pipe_config);
> +		if (ret) {
> +			DRM_DEBUG_KMS("No valid intermediate pipe watermarks are possible\n");
> +			return ret;
> +		}
>  	}
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> @@ -13452,6 +13514,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_crtc *crtc;
> +	struct intel_crtc_state *intel_cstate;
>  	int ret = 0, i;
>  	bool hw_check = intel_state->modeset;
>  
> @@ -13555,6 +13618,20 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  	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 (dev_priv->display.optimize_watermarks)
> +			dev_priv->display.optimize_watermarks(intel_cstate);
> +	}
> +
>  	mutex_lock(&dev->struct_mutex);
>  	drm_atomic_helper_cleanup_planes(dev, state);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -15225,7 +15302,7 @@ static void sanitize_watermarks(struct drm_device *dev)
>  	int i;
>  
>  	/* Only supported on platforms that use atomic watermark design */
> -	if (!dev_priv->display.program_watermarks)
> +	if (!dev_priv->display.optimize_watermarks)
>  		return;
>  
>  	/*
> @@ -15246,6 +15323,13 @@ retry:
>  	if (WARN_ON(IS_ERR(state)))
>  		goto fail;
>  
> +	/*
> +	 * Hardware readout is the only time we don't want to calculate
> +	 * intermediate watermarks (since we don't trust the current
> +	 * watermarks).
> +	 */
> +	to_intel_atomic_state(state)->skip_intermediate_wm = true;
> +
>  	ret = intel_atomic_check(dev, state);
>  	if (ret) {
>  		/*
> @@ -15268,7 +15352,8 @@ retry:
>  	for_each_crtc_in_state(state, crtc, cstate, i) {
>  		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
>  
> -		dev_priv->display.program_watermarks(cs);
> +		cs->wm.need_postvbl_update = true;
> +		dev_priv->display.optimize_watermarks(cs);
>  	}
>  
>  	drm_atomic_state_free(state);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4852049..a9f7641 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -260,6 +260,12 @@ struct intel_atomic_state {
>  
>  	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
>  	struct intel_wm_config wm_config;
> +
> +	/*
> +	 * Current watermarks can't be trusted during hardware readout, so
> +	 * don't bother calculating intermediate watermarks.
> +	 */
> +	bool skip_intermediate_wm;
>  };
>  
>  struct intel_plane_state {
> @@ -509,13 +515,29 @@ struct intel_crtc_state {
>  
>  	struct {
>  		/*
> -		 * optimal watermarks, programmed post-vblank when this state
> -		 * is committed
> +		 * Optimal watermarks, programmed post-vblank when this state
> +		 * is committed.
>  		 */
>  		union {
>  			struct intel_pipe_wm ilk;
>  			struct skl_pipe_wm skl;
>  		} optimal;
> +
> +		/*
> +		 * Intermediate watermarks; these can be programmed immediately
> +		 * since they satisfy both the current configuration we're
> +		 * switching away from and the new configuration we're switching
> +		 * to.
> +		 */
> +		struct intel_pipe_wm intermediate;
> +
> +		/*
> +		 * 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;
>  };
>  
> @@ -601,6 +623,7 @@ struct intel_crtc {
>  			struct intel_pipe_wm ilk;
>  			struct skl_pipe_wm skl;
>  		} active;
> +
>  		/* allow CxSR on this pipe */
>  		bool cxsr_allowed;
>  	} wm;
> @@ -1566,6 +1589,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);
>  int sanitize_rc6_option(const struct drm_device *dev, int enable_rc6);
>  
>  /* intel_sdvo.c */
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 347d4df..ccdb581 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2278,6 +2278,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 intel_crtc *intel_crtc,
>  			       struct drm_atomic_state *state)
> @@ -2292,10 +2315,6 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_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;
>  
>  	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> @@ -2319,21 +2338,18 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  			curstate = to_intel_plane_state(ps);
>  	}
>  
> -	config.sprites_enabled = sprstate->visible;
> -	config.sprites_scaled = sprstate->visible &&
> +	pipe_wm->pipe_enabled = cstate->base.active;
> +	pipe_wm->sprites_enabled = sprstate->visible;
> +	pipe_wm->sprites_scaled = sprstate->visible &&
>  		(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,
> @@ -2342,12 +2358,8 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_crtc,
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		pipe_wm->linetime = hsw_compute_linetime_wm(dev, cstate);
>  
> -	/* 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);
>  
> @@ -2372,6 +2384,59 @@ static int ilk_compute_pipe_wm(struct intel_crtc *intel_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 *intel_crtc,
> +				       struct intel_crtc_state *newstate)
> +{
> +	struct intel_pipe_wm *a = &newstate->wm.intermediate;
> +	struct intel_pipe_wm *b = &intel_crtc->wm.active.ilk;
> +	int level, max_level = ilk_wm_max_level(dev);
> +
> +	/*
> +	 * Start with the final, target watermarks, then combine with the
> +	 * currently active watermarks to get values that are safe both before
> +	 * and after the vblank.
> +	 */
> +	*a = newstate->wm.optimal.ilk;
> +	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; only update if it's different.
> +	 */
> +	if (memcmp(a, &newstate->wm.optimal.ilk, sizeof(*a)) == 0)
> +		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,
> @@ -2383,9 +2448,7 @@ static void ilk_merge_wm_level(struct drm_device *dev,
>  	ret_wm->enable = true;
>  
>  	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.optimal.ilk;
> +		const struct intel_pipe_wm *active = &intel_crtc->wm.active.ilk;
>  		const struct intel_wm_level *wm = &active->wm[level];
>  
>  		if (!active->pipe_enabled)
> @@ -2533,15 +2596,14 @@ static void ilk_compute_wm_results(struct drm_device *dev,
>  
>  	/* LP0 register values */
>  	for_each_intel_crtc(dev, intel_crtc) {
> -		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.optimal.ilk.wm[0];
> +		const struct intel_wm_level *r =
> +			&intel_crtc->wm.active.ilk.wm[0];
>  
>  		if (WARN_ON(!r->enable))
>  			continue;
>  
> -		results->wm_linetime[pipe] = cstate->wm.optimal.ilk.linetime;
> +		results->wm_linetime[pipe] = intel_crtc->wm.active.ilk.linetime;
>  
>  		results->wm_pipe[pipe] =
>  			(r->pri_val << WM0_PIPE_PLANE_SHIFT) |
> @@ -2748,7 +2810,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;
>  
> @@ -3643,11 +3705,9 @@ static void ilk_compute_wm_config(struct drm_device *dev,
>  	}
>  }
>  
> -static void ilk_program_watermarks(struct intel_crtc_state *cstate)
> +static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
>  {
> -	struct drm_crtc *crtc = cstate->base.crtc;
> -	struct drm_device *dev = crtc->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_device *dev = dev_priv->dev;
>  	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
>  	struct ilk_wm_maximums max;
>  	struct intel_wm_config config = {};
> @@ -3678,28 +3738,28 @@ static void ilk_program_watermarks(struct intel_crtc_state *cstate)
>  	ilk_write_wm_values(dev_priv, &results);
>  }
>  
> -static void ilk_update_wm(struct drm_crtc *crtc)
> +static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
>  {
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> -
> -	WARN_ON(cstate->base.active != intel_crtc->active);
> +	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
>  
> -	/*
> -	 * 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);
> -	}
> +	mutex_lock(&dev_priv->wm.wm_mutex);
> +	intel_crtc->wm.active.ilk = cstate->wm.intermediate;
> +	ilk_program_watermarks(dev_priv);
> +	mutex_unlock(&dev_priv->wm.wm_mutex);
> +}
>  
> -	intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
> +static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
>  
> -	ilk_program_watermarks(cstate);
> +	mutex_lock(&dev_priv->wm.wm_mutex);
> +	if (cstate->wm.need_postvbl_update) {
> +		intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
> +		ilk_program_watermarks(dev_priv);
> +	}
> +	mutex_unlock(&dev_priv->wm.wm_mutex);
>  }
>  
>  static void skl_pipe_wm_active_state(uint32_t val,
> @@ -7076,9 +7136,13 @@ 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.program_watermarks = ilk_program_watermarks;
> +			dev_priv->display.compute_intermediate_wm =
> +				ilk_compute_intermediate_wm;
> +			dev_priv->display.initial_watermarks =
> +				ilk_initial_watermarks;
> +			dev_priv->display.optimize_watermarks =
> +				ilk_optimize_watermarks;
>  		} else {
>  			DRM_DEBUG_KMS("Failed to read display plane latency. "
>  				      "Disable CxSR\n");
> -- 
> 2.1.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/i915: Add two-stage ILK-style watermark programming (v11)
  2016-02-25 16:48 ` Matt Roper
@ 2016-02-29  8:23   ` Maarten Lankhorst
  2016-02-29 16:42     ` Matt Roper
  2016-02-29 19:52   ` Imre Deak
  1 sibling, 1 reply; 7+ messages in thread
From: Maarten Lankhorst @ 2016-02-29  8:23 UTC (permalink / raw)
  To: Matt Roper, intel-gfx

Op 25-02-16 om 17:48 schreef Matt Roper:
> I never got a CI email for this (probably because fdo was down for a while),
> but I can see the results below in patchwork
>
>>  Test gem_cs_prefetch:
>>          Subgroup basic-default:
>>                  incomplete -> PASS       (ilk-hp8440p)
>>  Test kms_flip:
>>          Subgroup basic-flip-vs-dpms:
>>                  pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
> Pre-existing watermark bug here:
>         https://bugs.freedesktop.org/show_bug.cgi?id=93787
>
> I had hoped this patch might also fix that problem, but I guess further
> investigation will be needed; it seems to be something very
> ILK-specific, not seen on SNB, IVB, etc.
>
>
>>  Test kms_force_connector_basic:
>>          Subgroup force-edid:
>>                  skip       -> PASS       (snb-x220t)
>>                  pass       -> SKIP       (ivb-t430s)
> Pre-existing:
>         https://bugs.freedesktop.org/show_bug.cgi?id=93769
>
>>          Subgroup force-load-detect:
>>                  dmesg-fail -> FAIL       (snb-x220t)
>>                  dmesg-fail -> FAIL       (hsw-gt2)
> The dmesg warning is gone now (a locking warning that I don't think my
> patch should have affected); the remaining test failure is identical to
> what it was before (temp->connection != DRM_MODE_UNKNOWNCONNECTION) and
> unrelated.
>
> I don't see a bugzilla for this, which seems strange since it's a
> pre-existing defect.  Am I overlooking it or do I need to file a new
> one?
>
>>                  fail       -> SKIP       (ilk-hp8440p)
> Not sure what is actually plugged into the CI machine, so the skip may
> be legitimate/expected?  Doesn't seem related to anything in my patch
> anyway.
>
>>  Test kms_pipe_crc_basic:
>>          Subgroup suspend-read-crc-pipe-c:
>>                  pass       -> DMESG-WARN (bsw-nuc-2)
> This one was happening sporadically before my patch; I think it's the
> same bug filed here:
>
>   https://bugs.freedesktop.org/show_bug.cgi?id=94164
>
> I'll update the bugzilla to indicate it also happens on this test.
>
>>  Test pm_rpm:
>>          Subgroup basic-rte:
>>                  pass       -> DMESG-WARN (bsw-nuc-2)
>>                  fail       -> PASS       (hsw-brixbox)
> Same bug mentioned above (94164)
>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/i915: Add two-stage ILK-style watermark programming (v11)
  2016-02-29  8:23   ` Maarten Lankhorst
@ 2016-02-29 16:42     ` Matt Roper
  0 siblings, 0 replies; 7+ messages in thread
From: Matt Roper @ 2016-02-29 16:42 UTC (permalink / raw)
  To: Maarten Lankhorst; +Cc: intel-gfx

On Mon, Feb 29, 2016 at 09:23:14AM +0100, Maarten Lankhorst wrote:
> Op 25-02-16 om 17:48 schreef Matt Roper:
> > I never got a CI email for this (probably because fdo was down for a while),
> > but I can see the results below in patchwork
> >
> >>  Test gem_cs_prefetch:
> >>          Subgroup basic-default:
> >>                  incomplete -> PASS       (ilk-hp8440p)
> >>  Test kms_flip:
> >>          Subgroup basic-flip-vs-dpms:
> >>                  pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
> > Pre-existing watermark bug here:
> >         https://bugs.freedesktop.org/show_bug.cgi?id=93787
> >
> > I had hoped this patch might also fix that problem, but I guess further
> > investigation will be needed; it seems to be something very
> > ILK-specific, not seen on SNB, IVB, etc.
> >
> >
> >>  Test kms_force_connector_basic:
> >>          Subgroup force-edid:
> >>                  skip       -> PASS       (snb-x220t)
> >>                  pass       -> SKIP       (ivb-t430s)
> > Pre-existing:
> >         https://bugs.freedesktop.org/show_bug.cgi?id=93769
> >
> >>          Subgroup force-load-detect:
> >>                  dmesg-fail -> FAIL       (snb-x220t)
> >>                  dmesg-fail -> FAIL       (hsw-gt2)
> > The dmesg warning is gone now (a locking warning that I don't think my
> > patch should have affected); the remaining test failure is identical to
> > what it was before (temp->connection != DRM_MODE_UNKNOWNCONNECTION) and
> > unrelated.
> >
> > I don't see a bugzilla for this, which seems strange since it's a
> > pre-existing defect.  Am I overlooking it or do I need to file a new
> > one?
> >
> >>                  fail       -> SKIP       (ilk-hp8440p)
> > Not sure what is actually plugged into the CI machine, so the skip may
> > be legitimate/expected?  Doesn't seem related to anything in my patch
> > anyway.
> >
> >>  Test kms_pipe_crc_basic:
> >>          Subgroup suspend-read-crc-pipe-c:
> >>                  pass       -> DMESG-WARN (bsw-nuc-2)
> > This one was happening sporadically before my patch; I think it's the
> > same bug filed here:
> >
> >   https://bugs.freedesktop.org/show_bug.cgi?id=94164
> >
> > I'll update the bugzilla to indicate it also happens on this test.
> >
> >>  Test pm_rpm:
> >>          Subgroup basic-rte:
> >>                  pass       -> DMESG-WARN (bsw-nuc-2)
> >>                  fail       -> PASS       (hsw-brixbox)
> > Same bug mentioned above (94164)
> >
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Thanks for the review.  Pushed to dinq.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/i915: Add two-stage ILK-style watermark programming (v11)
  2016-02-25 16:48 ` Matt Roper
  2016-02-29  8:23   ` Maarten Lankhorst
@ 2016-02-29 19:52   ` Imre Deak
  1 sibling, 0 replies; 7+ messages in thread
From: Imre Deak @ 2016-02-29 19:52 UTC (permalink / raw)
  To: Matt Roper, intel-gfx; +Cc: Sarvela, Tomi P

On to, 2016-02-25 at 08:48 -0800, Matt Roper wrote:
> I never got a CI email for this (probably because fdo was down for a
> while),
> but I can see the results below in patchwork

We also have the problem, that if a patch prevents a machine from
booting that machine won't be included in the CI test result. This was
the case here on SKL.

--Imre

> 
> >  Test gem_cs_prefetch:
> >          Subgroup basic-default:
> >                  incomplete -> PASS       (ilk-hp8440p)
> >  Test kms_flip:
> >          Subgroup basic-flip-vs-dpms:
> >                  pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
> 
> Pre-existing watermark bug here:
>         https://bugs.freedesktop.org/show_bug.cgi?id=93787
> 
> I had hoped this patch might also fix that problem, but I guess
> further
> investigation will be needed; it seems to be something very
> ILK-specific, not seen on SNB, IVB, etc.
> 
> 
> >  Test kms_force_connector_basic:
> >          Subgroup force-edid:
> >                  skip       -> PASS       (snb-x220t)
> >                  pass       -> SKIP       (ivb-t430s)
> 
> Pre-existing:
>         https://bugs.freedesktop.org/show_bug.cgi?id=93769
> 
> >          Subgroup force-load-detect:
> >                  dmesg-fail -> FAIL       (snb-x220t)
> >                  dmesg-fail -> FAIL       (hsw-gt2)
> 
> The dmesg warning is gone now (a locking warning that I don't think
> my
> patch should have affected); the remaining test failure is identical
> to
> what it was before (temp->connection != DRM_MODE_UNKNOWNCONNECTION)
> and
> unrelated.
> 
> I don't see a bugzilla for this, which seems strange since it's a
> pre-existing defect.  Am I overlooking it or do I need to file a new
> one?
> 
> >                  fail       -> SKIP       (ilk-hp8440p)
> 
> Not sure what is actually plugged into the CI machine, so the skip
> may
> be legitimate/expected?  Doesn't seem related to anything in my patch
> anyway.
> 
> >  Test kms_pipe_crc_basic:
> >          Subgroup suspend-read-crc-pipe-c:
> >                  pass       -> DMESG-WARN (bsw-nuc-2)
> 
> This one was happening sporadically before my patch; I think it's the
> same bug filed here:
> 
>   https://bugs.freedesktop.org/show_bug.cgi?id=94164
> 
> I'll update the bugzilla to indicate it also happens on this test.
> 
> >  Test pm_rpm:
> >          Subgroup basic-rte:
> >                  pass       -> DMESG-WARN (bsw-nuc-2)
> >                  fail       -> PASS       (hsw-brixbox)
> 
> Same bug mentioned above (94164)
> 
> 
> 
> Matt
> 
> 
> On Tue, Feb 23, 2016 at 05:20:13PM -0800, 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.
> > 
> > v4:
> >  - Add a wm_mutex to cover updates to intel_crtc->active and the
> >    need_postvbl_update flag.  Since we don't have async yet it
> > isn't
> >    terribly important yet, but might as well add it now.
> >  - Change interface to program watermarks.  Platforms will now
> > expose
> >    .initial_watermarks() and .optimize_watermarks() functions to do
> >    watermark programming.  These should lock wm_mutex, copy the
> >    appropriate state values into intel_crtc->active, and then call
> >    the internal program watermarks function.
> > 
> > v5:
> >  - Skip intermediate watermark calculation/check during initial
> > hardware
> >    readout since we don't trust the existing HW values (and don't
> > have
> >    valid values of our own yet).
> >  - Don't try to call .optimize_watermarks() on platforms that don't
> > have
> >    atomic watermarks yet.  (Maarten)
> > 
> > v6:
> >  - Rebase
> > 
> > v7:
> >  - Further rebase
> > 
> > v8:
> >  - A few minor indentation and line length fixes
> > 
> > v9:
> >  - Yet another rebase since Maarten's patches reworked a bunch of
> > the
> >    code (wm_pre, wm_post, etc.) that this was previously based on.
> > 
> > v10:
> >  - Move wm_mutex to dev_priv to protect against racing commits
> > against
> >    disjoint CRTC sets. (Maarten)
> >  - Drop unnecessary clearing of cstate->wm.need_postvbl_update
> > (Maarten)
> > 
> > v11:
> >  - Now that we've moved to atomic watermark updates, make sure we
> > call
> >    the proper function to program watermarks in
> >    {ironlake,haswell}_crtc_enable(); the failure to do so on the
> >    previous patch iteration led to us not actually programming the
> >    watermarks before turning on the CRTC, which was the cause of
> > the
> >    underruns that the CI system was seeing.
> >  - Fix inverted logic for determining when to optimize
> > watermarks.  We
> >    were needlessly optimizing when the intermediate/optimal values
> > were
> >    the same (harmless), but not actually optimizing when they
> > differed
> >    (also harmless, but wasteful from a power/bandwidth
> > perspective).
> > 
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c      |   1 +
> >  drivers/gpu/drm/i915/i915_drv.h      |  13 ++-
> >  drivers/gpu/drm/i915/intel_atomic.c  |   1 +
> >  drivers/gpu/drm/i915/intel_display.c |  97 +++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_drv.h     |  28 +++++-
> >  drivers/gpu/drm/i915/intel_pm.c      | 162
> > ++++++++++++++++++++++++-----------
> >  6 files changed, 244 insertions(+), 58 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c
> > index 1c6d227..36c0cf1 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1010,6 +1010,7 @@ int i915_driver_load(struct drm_device *dev,
> > unsigned long flags)
> >  	mutex_init(&dev_priv->sb_lock);
> >  	mutex_init(&dev_priv->modeset_restore_lock);
> >  	mutex_init(&dev_priv->av_mutex);
> > +	mutex_init(&dev_priv->wm.wm_mutex);
> >  
> >  	ret = i915_workqueues_init(dev_priv);
> >  	if (ret < 0)
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 9e76bfc..cf17d62 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -631,7 +631,11 @@ struct drm_i915_display_funcs {
> >  			  struct dpll *best_clock);
> >  	int (*compute_pipe_wm)(struct intel_crtc *crtc,
> >  			       struct drm_atomic_state *state);
> > -	void (*program_watermarks)(struct intel_crtc_state
> > *cstate);
> > +	int (*compute_intermediate_wm)(struct drm_device *dev,
> > +				       struct intel_crtc
> > *intel_crtc,
> > +				       struct intel_crtc_state
> > *newstate);
> > +	void (*initial_watermarks)(struct intel_crtc_state
> > *cstate);
> > +	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);
> > @@ -1980,6 +1984,13 @@ struct drm_i915_private {
> >  		};
> >  
> >  		uint8_t max_level;
> > +
> > +		/*
> > +		 * Should be held around atomic WM register
> > writing; also
> > +		 * protects * intel_crtc->wm.active and
> > +		 * cstate->wm.need_postvbl_update.
> > +		 */
> > +		struct mutex wm_mutex;
> >  	} wm;
> >  
> >  	struct i915_runtime_pm pm;
> > diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> > b/drivers/gpu/drm/i915/intel_atomic.c
> > index 4625f8a..9682d94 100644
> > --- a/drivers/gpu/drm/i915/intel_atomic.c
> > +++ b/drivers/gpu/drm/i915/intel_atomic.c
> > @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> >  	crtc_state->disable_lp_wm = false;
> >  	crtc_state->disable_cxsr = false;
> >  	crtc_state->wm_changed = false;
> > +	crtc_state->wm.need_postvbl_update = false;
> >  
> >  	return &crtc_state->base;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index deee560..423b99e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4846,7 +4846,42 @@ static void intel_pre_plane_update(struct
> > intel_crtc_state *old_crtc_state)
> >  			intel_set_memory_cxsr(dev_priv, false);
> >  	}
> >  
> > -	if (!needs_modeset(&pipe_config->base) && pipe_config-
> > >wm_changed)
> > +	/*
> > +	 * 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 (pipe_config->disable_lp_wm) {
> > +		ilk_disable_lp_wm(dev);
> > +		intel_wait_for_vblank(dev, crtc->pipe);
> > +	}
> > +
> > +	/*
> > +	 * If we're doing a modeset, we're done.  No need to do
> > any pre-vblank
> > +	 * watermark programming here.
> > +	 */
> > +	if (needs_modeset(&pipe_config->base))
> > +		return;
> > +
> > +	/*
> > +	 * For platforms that support atomic watermarks, program
> > the
> > +	 * 'intermediate' 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.initial_watermarks != NULL)
> > +		dev_priv->display.initial_watermarks(pipe_config);
> > +	else if (pipe_config->wm_changed)
> >  		intel_update_watermarks(&crtc->base);
> >  }
> >  
> > @@ -4925,7 +4960,7 @@ static void ironlake_crtc_enable(struct
> > drm_crtc *crtc)
> >  	 */
> >  	intel_crtc_load_lut(crtc);
> >  
> > -	intel_update_watermarks(crtc);
> > +	dev_priv->display.initial_watermarks(intel_crtc->config);
> >  	intel_enable_pipe(intel_crtc);
> >  
> >  	if (intel_crtc->config->has_pch_encoder)
> > @@ -5024,7 +5059,7 @@ static void haswell_crtc_enable(struct
> > drm_crtc *crtc)
> >  	if (!intel_crtc->config->has_dsi_encoder)
> >  		intel_ddi_enable_transcoder_func(crtc);
> >  
> > -	intel_update_watermarks(crtc);
> > +	dev_priv->display.initial_watermarks(pipe_config);
> >  	intel_enable_pipe(intel_crtc);
> >  
> >  	if (intel_crtc->config->has_pch_encoder)
> > @@ -11800,6 +11835,7 @@ int intel_plane_atomic_calc_changes(struct
> > drm_crtc_state *crtc_state,
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	struct drm_plane *plane = plane_state->plane;
> >  	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_plane_state *old_plane_state =
> >  		to_intel_plane_state(plane->state);
> >  	int idx = intel_crtc->base.base.id, ret;
> > @@ -11859,6 +11895,11 @@ int intel_plane_atomic_calc_changes(struct
> > drm_crtc_state *crtc_state,
> >  		pipe_config->wm_changed = true;
> >  	}
> >  
> > +	/* Pre-gen9 platforms need two-step watermark updates */
> > +	if (pipe_config->wm_changed && INTEL_INFO(dev)->gen < 9 &&
> > +	    dev_priv->display.optimize_watermarks)
> > +		to_intel_crtc_state(crtc_state)-
> > >wm.need_postvbl_update = true;
> > +
> >  	if (visible || was_visible)
> >  		intel_crtc->atomic.fb_bits |=
> >  			to_intel_plane(plane)->frontbuffer_bit;
> > @@ -11983,8 +12024,29 @@ 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(intel_crtc, state);
> > -		if (ret)
> > +		if (ret) {
> > +			DRM_DEBUG_KMS("Target pipe watermarks are
> > invalid\n");
> >  			return ret;
> > +		}
> > +	}
> > +
> > +	if (dev_priv->display.compute_intermediate_wm &&
> > +	    !to_intel_atomic_state(state)->skip_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,
> > +								in
> > tel_crtc,
> > +								pi
> > pe_config);
> > +		if (ret) {
> > +			DRM_DEBUG_KMS("No valid intermediate pipe
> > watermarks are possible\n");
> > +			return ret;
> > +		}
> >  	}
> >  
> >  	if (INTEL_INFO(dev)->gen >= 9) {
> > @@ -13452,6 +13514,7 @@ static int intel_atomic_commit(struct
> > drm_device *dev,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc_state *crtc_state;
> >  	struct drm_crtc *crtc;
> > +	struct intel_crtc_state *intel_cstate;
> >  	int ret = 0, i;
> >  	bool hw_check = intel_state->modeset;
> >  
> > @@ -13555,6 +13618,20 @@ static int intel_atomic_commit(struct
> > drm_device *dev,
> >  
> >  	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 (dev_priv->display.optimize_watermarks)
> > +			dev_priv-
> > >display.optimize_watermarks(intel_cstate);
> > +	}
> > +
> >  	mutex_lock(&dev->struct_mutex);
> >  	drm_atomic_helper_cleanup_planes(dev, state);
> >  	mutex_unlock(&dev->struct_mutex);
> > @@ -15225,7 +15302,7 @@ static void sanitize_watermarks(struct
> > drm_device *dev)
> >  	int i;
> >  
> >  	/* Only supported on platforms that use atomic watermark
> > design */
> > -	if (!dev_priv->display.program_watermarks)
> > +	if (!dev_priv->display.optimize_watermarks)
> >  		return;
> >  
> >  	/*
> > @@ -15246,6 +15323,13 @@ retry:
> >  	if (WARN_ON(IS_ERR(state)))
> >  		goto fail;
> >  
> > +	/*
> > +	 * Hardware readout is the only time we don't want to
> > calculate
> > +	 * intermediate watermarks (since we don't trust the
> > current
> > +	 * watermarks).
> > +	 */
> > +	to_intel_atomic_state(state)->skip_intermediate_wm = true;
> > +
> >  	ret = intel_atomic_check(dev, state);
> >  	if (ret) {
> >  		/*
> > @@ -15268,7 +15352,8 @@ retry:
> >  	for_each_crtc_in_state(state, crtc, cstate, i) {
> >  		struct intel_crtc_state *cs =
> > to_intel_crtc_state(cstate);
> >  
> > -		dev_priv->display.program_watermarks(cs);
> > +		cs->wm.need_postvbl_update = true;
> > +		dev_priv->display.optimize_watermarks(cs);
> >  	}
> >  
> >  	drm_atomic_state_free(state);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 4852049..a9f7641 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -260,6 +260,12 @@ struct intel_atomic_state {
> >  
> >  	struct intel_shared_dpll_config
> > shared_dpll[I915_NUM_PLLS];
> >  	struct intel_wm_config wm_config;
> > +
> > +	/*
> > +	 * Current watermarks can't be trusted during hardware
> > readout, so
> > +	 * don't bother calculating intermediate watermarks.
> > +	 */
> > +	bool skip_intermediate_wm;
> >  };
> >  
> >  struct intel_plane_state {
> > @@ -509,13 +515,29 @@ struct intel_crtc_state {
> >  
> >  	struct {
> >  		/*
> > -		 * optimal watermarks, programmed post-vblank when
> > this state
> > -		 * is committed
> > +		 * Optimal watermarks, programmed post-vblank when
> > this state
> > +		 * is committed.
> >  		 */
> >  		union {
> >  			struct intel_pipe_wm ilk;
> >  			struct skl_pipe_wm skl;
> >  		} optimal;
> > +
> > +		/*
> > +		 * Intermediate watermarks; these can be
> > programmed immediately
> > +		 * since they satisfy both the current
> > configuration we're
> > +		 * switching away from and the new configuration
> > we're switching
> > +		 * to.
> > +		 */
> > +		struct intel_pipe_wm intermediate;
> > +
> > +		/*
> > +		 * 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;
> >  };
> >  
> > @@ -601,6 +623,7 @@ struct intel_crtc {
> >  			struct intel_pipe_wm ilk;
> >  			struct skl_pipe_wm skl;
> >  		} active;
> > +
> >  		/* allow CxSR on this pipe */
> >  		bool cxsr_allowed;
> >  	} wm;
> > @@ -1566,6 +1589,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);
> >  int sanitize_rc6_option(const struct drm_device *dev, int
> > enable_rc6);
> >  
> >  /* intel_sdvo.c */
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 347d4df..ccdb581 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2278,6 +2278,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 intel_crtc *intel_crtc,
> >  			       struct drm_atomic_state *state)
> > @@ -2292,10 +2315,6 @@ static int ilk_compute_pipe_wm(struct
> > intel_crtc *intel_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;
> >  
> >  	cstate = intel_atomic_get_crtc_state(state, intel_crtc);
> > @@ -2319,21 +2338,18 @@ static int ilk_compute_pipe_wm(struct
> > intel_crtc *intel_crtc,
> >  			curstate = to_intel_plane_state(ps);
> >  	}
> >  
> > -	config.sprites_enabled = sprstate->visible;
> > -	config.sprites_scaled = sprstate->visible &&
> > +	pipe_wm->pipe_enabled = cstate->base.active;
> > +	pipe_wm->sprites_enabled = sprstate->visible;
> > +	pipe_wm->sprites_scaled = sprstate->visible &&
> >  		(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,
> > @@ -2342,12 +2358,8 @@ static int ilk_compute_pipe_wm(struct
> > intel_crtc *intel_crtc,
> >  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  		pipe_wm->linetime = hsw_compute_linetime_wm(dev,
> > cstate);
> >  
> > -	/* 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);
> >  
> > @@ -2372,6 +2384,59 @@ static int ilk_compute_pipe_wm(struct
> > intel_crtc *intel_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
> > *intel_crtc,
> > +				       struct intel_crtc_state
> > *newstate)
> > +{
> > +	struct intel_pipe_wm *a = &newstate->wm.intermediate;
> > +	struct intel_pipe_wm *b = &intel_crtc->wm.active.ilk;
> > +	int level, max_level = ilk_wm_max_level(dev);
> > +
> > +	/*
> > +	 * Start with the final, target watermarks, then combine
> > with the
> > +	 * currently active watermarks to get values that are safe
> > both before
> > +	 * and after the vblank.
> > +	 */
> > +	*a = newstate->wm.optimal.ilk;
> > +	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; only update if it's
> > different.
> > +	 */
> > +	if (memcmp(a, &newstate->wm.optimal.ilk, sizeof(*a)) == 0)
> > +		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,
> > @@ -2383,9 +2448,7 @@ static void ilk_merge_wm_level(struct
> > drm_device *dev,
> >  	ret_wm->enable = true;
> >  
> >  	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.optimal.ilk;
> > +		const struct intel_pipe_wm *active = &intel_crtc-
> > >wm.active.ilk;
> >  		const struct intel_wm_level *wm = &active-
> > >wm[level];
> >  
> >  		if (!active->pipe_enabled)
> > @@ -2533,15 +2596,14 @@ static void ilk_compute_wm_results(struct
> > drm_device *dev,
> >  
> >  	/* LP0 register values */
> >  	for_each_intel_crtc(dev, intel_crtc) {
> > -		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.optimal.ilk.wm[0];
> > +		const struct intel_wm_level *r =
> > +			&intel_crtc->wm.active.ilk.wm[0];
> >  
> >  		if (WARN_ON(!r->enable))
> >  			continue;
> >  
> > -		results->wm_linetime[pipe] = cstate-
> > >wm.optimal.ilk.linetime;
> > +		results->wm_linetime[pipe] = intel_crtc-
> > >wm.active.ilk.linetime;
> >  
> >  		results->wm_pipe[pipe] =
> >  			(r->pri_val << WM0_PIPE_PLANE_SHIFT) |
> > @@ -2748,7 +2810,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;
> >  
> > @@ -3643,11 +3705,9 @@ static void ilk_compute_wm_config(struct
> > drm_device *dev,
> >  	}
> >  }
> >  
> > -static void ilk_program_watermarks(struct intel_crtc_state
> > *cstate)
> > +static void ilk_program_watermarks(struct drm_i915_private
> > *dev_priv)
> >  {
> > -	struct drm_crtc *crtc = cstate->base.crtc;
> > -	struct drm_device *dev = crtc->dev;
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_device *dev = dev_priv->dev;
> >  	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {},
> > *best_lp_wm;
> >  	struct ilk_wm_maximums max;
> >  	struct intel_wm_config config = {};
> > @@ -3678,28 +3738,28 @@ static void ilk_program_watermarks(struct
> > intel_crtc_state *cstate)
> >  	ilk_write_wm_values(dev_priv, &results);
> >  }
> >  
> > -static void ilk_update_wm(struct drm_crtc *crtc)
> > +static void ilk_initial_watermarks(struct intel_crtc_state
> > *cstate)
> >  {
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	struct intel_crtc_state *cstate =
> > to_intel_crtc_state(crtc->state);
> > -
> > -	WARN_ON(cstate->base.active != intel_crtc->active);
> > +	struct drm_i915_private *dev_priv = to_i915(cstate-
> > >base.crtc->dev);
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> > >base.crtc);
> >  
> > -	/*
> > -	 * 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);
> > -	}
> > +	mutex_lock(&dev_priv->wm.wm_mutex);
> > +	intel_crtc->wm.active.ilk = cstate->wm.intermediate;
> > +	ilk_program_watermarks(dev_priv);
> > +	mutex_unlock(&dev_priv->wm.wm_mutex);
> > +}
> >  
> > -	intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
> > +static void ilk_optimize_watermarks(struct intel_crtc_state
> > *cstate)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(cstate-
> > >base.crtc->dev);
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> > >base.crtc);
> >  
> > -	ilk_program_watermarks(cstate);
> > +	mutex_lock(&dev_priv->wm.wm_mutex);
> > +	if (cstate->wm.need_postvbl_update) {
> > +		intel_crtc->wm.active.ilk = cstate-
> > >wm.optimal.ilk;
> > +		ilk_program_watermarks(dev_priv);
> > +	}
> > +	mutex_unlock(&dev_priv->wm.wm_mutex);
> >  }
> >  
> >  static void skl_pipe_wm_active_state(uint32_t val,
> > @@ -7076,9 +7136,13 @@ 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.program_watermarks =
> > ilk_program_watermarks;
> > +			dev_priv->display.compute_intermediate_wm
> > =
> > +				ilk_compute_intermediate_wm;
> > +			dev_priv->display.initial_watermarks =
> > +				ilk_initial_watermarks;
> > +			dev_priv->display.optimize_watermarks =
> > +				ilk_optimize_watermarks;
> >  		} else {
> >  			DRM_DEBUG_KMS("Failed to read display
> > plane latency. "
> >  				      "Disable CxSR\n");
> > -- 
> > 2.1.4
> > 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2016-02-29 19:52 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-24  1:20 [PATCH] drm/i915: Add two-stage ILK-style watermark programming (v11) Matt Roper
2016-02-24  1:24 ` Matt Roper
2016-02-24  8:20   ` Maarten Lankhorst
2016-02-25 16:48 ` Matt Roper
2016-02-29  8:23   ` Maarten Lankhorst
2016-02-29 16:42     ` Matt Roper
2016-02-29 19:52   ` Imre Deak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).