[CI 15/17] drm/i915/gen9: Calculate watermarks during atomic 'check' (v2)

[Matt Roper <matthew.d.roper@intel.com>]

Moving watermark calculation into the check phase will allow us to to
reject display configurations for which there are no valid watermark
values before we start trying to program the hardware (although those
tests will come in a subsequent patch).

Another advantage of moving this calculation to the check phase is that
we can calculate the watermarks in a single shot as part of the atomic
transaction.  The watermark interfaces we inherited from our legacy
modesetting days are a bit broken in the atomic design because they use
per-crtc entry points but actually re-calculate and re-program something
that is really more of a global state.  That worked okay in the legacy
modesetting world because operations only ever updated a single CRTC at
a time.  However in the atomic world, a transaction can involve multiple
CRTC's, which means we wind up computing and programming the watermarks
NxN times (where N is the number of CRTC's involved).  With this patch
we eliminate the redundant re-calculation of watermark data for atomic
states (which was the cause of the WARN_ON(!wm_changed) problems that
have plagued us for a while).

We still need to work on the 'commit' side of watermark handling so that
we aren't doing redundant NxN programming of watermarks, but that's
content for future patches.

v2:
 - Bail out of skl_write_wm_values() if the CRTC isn't active.  Now that
   we set dirty_pipes to ~0 if the active pipes change (because
   we need to deal with DDB changes), we can now wind up here for
   disabled pipes, whereas we couldn't before.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89055
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92181
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Tested-by: Daniel Stone <daniels@collabora.com>
---
 drivers/gpu/drm/i915/intel_display.c |   2 +-
 drivers/gpu/drm/i915/intel_drv.h     |   2 +-
 drivers/gpu/drm/i915/intel_pm.c      | 140 +++++++++++++----------------------
 3 files changed, 54 insertions(+), 90 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 33f6c4b..790ae7a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13601,7 +13601,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	drm_atomic_helper_swap_state(dev, state);
 	dev_priv->wm.config = intel_state->wm_config;
 	dev_priv->wm.distrust_bios_wm = false;
-	dev_priv->wm.skl_results.ddb = intel_state->ddb;
+	dev_priv->wm.skl_results = intel_state->wm_results;
 	intel_shared_dpll_commit(state);
 
 	if (intel_state->modeset) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c9a4c42..b477978 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -314,7 +314,7 @@ struct intel_atomic_state {
 	bool skip_intermediate_wm;
 
 	/* Gen9+ only */
-	struct skl_ddb_allocation ddb;
+	struct skl_wm_values wm_results;
 };
 
 struct intel_plane_state {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1d8407a..f9dff5e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3221,23 +3221,6 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
 	return ret;
 }
 
-static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
-				       const struct intel_crtc *intel_crtc)
-{
-	struct drm_device *dev = intel_crtc->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	const struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
-
-	/*
-	 * If ddb allocation of pipes changed, it may require recalculation of
-	 * watermarks
-	 */
-	if (memcmp(new_ddb->pipe, cur_ddb->pipe, sizeof(new_ddb->pipe)))
-		return true;
-
-	return false;
-}
-
 static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				struct intel_crtc_state *cstate,
 				struct intel_plane_state *intel_pstate,
@@ -3533,6 +3516,8 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 
 		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
 			continue;
+		if (!crtc->active)
+			continue;
 
 		I915_WRITE(PIPE_WM_LINETIME(pipe), new->wm_linetime[pipe]);
 
@@ -3716,66 +3701,9 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
 	else
 		*changed = true;
 
-	intel_crtc->wm.active.skl = *pipe_wm;
-
 	return 0;
 }
 
-static void skl_update_other_pipe_wm(struct drm_device *dev,
-				     struct drm_crtc *crtc,
-				     struct skl_wm_values *r)
-{
-	struct intel_crtc *intel_crtc;
-	struct intel_crtc *this_crtc = to_intel_crtc(crtc);
-
-	/*
-	 * If the WM update hasn't changed the allocation for this_crtc (the
-	 * crtc we are currently computing the new WM values for), other
-	 * enabled crtcs will keep the same allocation and we don't need to
-	 * recompute anything for them.
-	 */
-	if (!skl_ddb_allocation_changed(&r->ddb, this_crtc))
-		return;
-
-	/*
-	 * Otherwise, because of this_crtc being freshly enabled/disabled, the
-	 * other active pipes need new DDB allocation and WM values.
-	 */
-	for_each_intel_crtc(dev, intel_crtc) {
-		struct skl_pipe_wm pipe_wm = {};
-		bool wm_changed;
-
-		if (this_crtc->pipe == intel_crtc->pipe)
-			continue;
-
-		if (!intel_crtc->active)
-			continue;
-
-		skl_update_pipe_wm(intel_crtc->base.state,
-				   &r->ddb, &pipe_wm, &wm_changed);
-
-		/*
-		 * If we end up re-computing the other pipe WM values, it's
-		 * because it was really needed, so we expect the WM values to
-		 * be different.
-		 */
-		WARN_ON(!wm_changed);
-
-		skl_compute_wm_results(dev, &pipe_wm, r, intel_crtc);
-		r->dirty_pipes |= drm_crtc_mask(&intel_crtc->base);
-	}
-}
-
-static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe)
-{
-	watermarks->wm_linetime[pipe] = 0;
-	memset(watermarks->plane[pipe], 0,
-	       sizeof(uint32_t) * 8 * I915_MAX_PLANES);
-	memset(watermarks->plane_trans[pipe],
-	       0, sizeof(uint32_t) * I915_MAX_PLANES);
-	watermarks->plane_trans[pipe][PLANE_CURSOR] = 0;
-}
-
 static int
 skl_compute_ddb(struct drm_atomic_state *state)
 {
@@ -3783,6 +3711,7 @@ skl_compute_ddb(struct drm_atomic_state *state)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct intel_crtc *intel_crtc;
+	struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb;
 	unsigned realloc_pipes = dev_priv->active_crtcs;
 	int ret;
 
@@ -3808,8 +3737,10 @@ skl_compute_ddb(struct drm_atomic_state *state)
 	 * any other display updates race with this transaction, so we need
 	 * to grab the lock on *all* CRTC's.
 	 */
-	if (intel_state->active_pipe_changes)
+	if (intel_state->active_pipe_changes) {
 		realloc_pipes = ~0;
+		intel_state->wm_results.dirty_pipes = ~0;
+	}
 
 	for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) {
 		struct intel_crtc_state *cstate;
@@ -3818,7 +3749,7 @@ skl_compute_ddb(struct drm_atomic_state *state)
 		if (IS_ERR(cstate))
 			return PTR_ERR(cstate);
 
-		ret = skl_allocate_pipe_ddb(cstate, &intel_state->ddb);
+		ret = skl_allocate_pipe_ddb(cstate, ddb);
 		if (ret)
 			return ret;
 	}
@@ -3831,8 +3762,11 @@ skl_compute_wm(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *cstate;
-	int ret, i;
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct skl_wm_values *results = &intel_state->wm_results;
+	struct skl_pipe_wm *pipe_wm;
 	bool changed = false;
+	int ret, i;
 
 	/*
 	 * If this transaction isn't actually touching any CRTC's, don't
@@ -3847,10 +3781,45 @@ skl_compute_wm(struct drm_atomic_state *state)
 	if (!changed)
 		return 0;
 
+	/* Clear all dirty flags */
+	results->dirty_pipes = 0;
+
 	ret = skl_compute_ddb(state);
 	if (ret)
 		return ret;
 
+	/*
+	 * Calculate WM's for all pipes that are part of this transaction.
+	 * Note that the DDB allocation above may have added more CRTC's that
+	 * weren't otherwise being modified (and set bits in dirty_pipes) if
+	 * pipe allocations had to change.
+	 *
+	 * FIXME:  Now that we're doing this in the atomic check phase, we
+	 * should allow skl_update_pipe_wm() to return failure in cases where
+	 * no suitable watermark values can be found.
+	 */
+	for_each_crtc_in_state(state, crtc, cstate, i) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+		struct intel_crtc_state *intel_cstate =
+			to_intel_crtc_state(cstate);
+
+		pipe_wm = &intel_cstate->wm.skl.optimal;
+		ret = skl_update_pipe_wm(cstate, &results->ddb, pipe_wm,
+					 &changed);
+		if (ret)
+			return ret;
+
+		if (changed)
+			results->dirty_pipes |= drm_crtc_mask(crtc);
+
+		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
+			/* This pipe's WM's did not change */
+			continue;
+
+		intel_cstate->update_wm_pre = true;
+		skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
+	}
+
 	return 0;
 }
 
@@ -3862,26 +3831,21 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	struct skl_wm_values *results = &dev_priv->wm.skl_results;
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
-	bool wm_changed;
-
-	/* Clear all dirty flags */
-	results->dirty_pipes = 0;
 
-	skl_clear_wm(results, intel_crtc->pipe);
-
-	skl_update_pipe_wm(crtc->state, &results->ddb, pipe_wm, &wm_changed);
-	if (!wm_changed)
+	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
 		return;
 
-	skl_compute_wm_results(dev, pipe_wm, results, intel_crtc);
-	results->dirty_pipes |= drm_crtc_mask(&intel_crtc->base);
+	intel_crtc->wm.active.skl = *pipe_wm;
+
+	mutex_lock(&dev_priv->wm.wm_mutex);
 
-	skl_update_other_pipe_wm(dev, crtc, results);
 	skl_write_wm_values(dev_priv, results);
 	skl_flush_wm_values(dev_priv, results);
 
 	/* store the new configuration */
 	dev_priv->wm.skl_hw = *results;
+
+	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
 
 static void ilk_compute_wm_config(struct drm_device *dev,
-- 
2.1.4

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