[PATCH 0/6] Start of skl watermark cleanup

[PATCH 0/6] Start of skl watermark cleanup

[Lyude]

While it (mostly) works, the code for handling watermarks on Skylake has been
kind of ugly for a while. As well a lot of it isn't that friendly to atomic
transactions, Lots of copy paste, redundant wm values, etc. While this isn't a
full cleanup, it's a good start. As well, we add a couple of features for
making debugging watermarks a little easier.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>

Lyude (6):
  drm/i915/skl: Move per-pipe ddb allocations into crtc states
  drm/i915/skl: Remove linetime from skl_wm_values
  drm/i915: Add enable_sagv option
  drm/i915/gen9: Make skl_wm_level per-plane
  drm/i915/gen9: Get rid of redundant watermark values
  drm/i915/gen9: Add ddb changes to atomic debug output

 drivers/gpu/drm/i915/i915_drv.h      |  10 +-
 drivers/gpu/drm/i915/i915_params.c   |   5 +
 drivers/gpu/drm/i915/i915_params.h   |   1 +
 drivers/gpu/drm/i915/intel_display.c |  52 +++--
 drivers/gpu/drm/i915/intel_drv.h     |  20 +-
 drivers/gpu/drm/i915/intel_pm.c      | 437 ++++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
 7 files changed, 260 insertions(+), 273 deletions(-)

-- 
2.7.4

[PATCH 1/6] drm/i915/skl: Move per-pipe ddb allocations into crtc states

[Lyude]

First part of cleaning up all of the skl watermark code. This moves the
structures for storing the ddb allocations of each pipe into
intel_crtc_state, along with moving the structures for storing the
current ddb allocations active on hardware into intel_crtc.

Signed-off-by: Lyude <cpaul@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 -
 drivers/gpu/drm/i915/intel_display.c | 16 ++++++++-------
 drivers/gpu/drm/i915/intel_drv.h     |  8 +++++---
 drivers/gpu/drm/i915/intel_pm.c      | 40 +++++++++++++++---------------------
 4 files changed, 30 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f8c66ee..85e541c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1636,7 +1636,6 @@ static inline bool skl_ddb_entry_equal(const struct skl_ddb_entry *e1,
 }
 
 struct skl_ddb_allocation {
-	struct skl_ddb_entry pipe[I915_MAX_PIPES];
 	struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES]; /* packed/uv */
 	struct skl_ddb_entry y_plane[I915_MAX_PIPES][I915_MAX_PLANES];
 };
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a366656..17733af 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14235,12 +14235,11 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 			     unsigned int *crtc_vblank_mask)
 {
 	struct drm_device *dev = state->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_crtc *crtc;
+	struct intel_crtc *intel_crtc;
 	struct drm_crtc_state *old_crtc_state;
-	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
-	struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
+	struct intel_crtc_state *cstate;
 	unsigned int updated = 0;
 	bool progress;
 	enum pipe pipe;
@@ -14258,12 +14257,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 		for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
 			bool vbl_wait = false;
 			unsigned int cmask = drm_crtc_mask(crtc);
-			pipe = to_intel_crtc(crtc)->pipe;
+
+			intel_crtc = to_intel_crtc(crtc);
+			cstate = to_intel_crtc_state(crtc->state);
+			pipe = intel_crtc->pipe;
 
 			if (updated & cmask || !crtc->state->active)
 				continue;
-			if (skl_ddb_allocation_overlaps(state, cur_ddb, new_ddb,
-							pipe))
+			if (skl_ddb_allocation_overlaps(state, intel_crtc))
 				continue;
 
 			updated |= cmask;
@@ -14274,7 +14275,8 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
 			 * then we need to wait for a vblank to pass for the
 			 * new ddb allocation to take effect.
 			 */
-			if (!skl_ddb_allocation_equals(cur_ddb, new_ddb, pipe) &&
+			if (!skl_ddb_entry_equal(&cstate->wm.skl.ddb,
+						 &intel_crtc->hw_ddb) &&
 			    !crtc->state->active_changed &&
 			    intel_state->wm_results.dirty_pipes != updated)
 				vbl_wait = true;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f48e79a..35ba282 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -496,6 +496,7 @@ struct intel_crtc_wm_state {
 		struct {
 			/* gen9+ only needs 1-step wm programming */
 			struct skl_pipe_wm optimal;
+			struct skl_ddb_entry ddb;
 
 			/* cached plane data rate */
 			unsigned plane_data_rate[I915_MAX_PLANES];
@@ -733,6 +734,9 @@ struct intel_crtc {
 		bool cxsr_allowed;
 	} wm;
 
+	/* gen9+: ddb allocation currently being used */
+	struct skl_ddb_entry hw_ddb;
+
 	int scanline_offset;
 
 	struct {
@@ -1755,9 +1759,7 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
 			       const struct skl_ddb_allocation *new,
 			       enum pipe pipe);
 bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
-				 const struct skl_ddb_allocation *old,
-				 const struct skl_ddb_allocation *new,
-				 enum pipe pipe);
+				 struct intel_crtc *intel_crtc);
 void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
 			 const struct skl_wm_values *wm);
 void skl_write_plane_wm(struct intel_crtc *intel_crtc,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7f1748a..0383516 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3058,7 +3058,6 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	struct drm_crtc *for_crtc = cstate->base.crtc;
 	unsigned int pipe_size, ddb_size;
 	int nth_active_pipe;
-	int pipe = to_intel_crtc(for_crtc)->pipe;
 
 	if (WARN_ON(!state) || !cstate->base.active) {
 		alloc->start = 0;
@@ -3086,7 +3085,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	 * we currently hold.
 	 */
 	if (!intel_state->active_pipe_changes) {
-		*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
+		*alloc = to_intel_crtc(for_crtc)->hw_ddb;
 		return;
 	}
 
@@ -3354,7 +3353,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	struct drm_plane *plane;
 	struct drm_plane_state *pstate;
 	enum pipe pipe = intel_crtc->pipe;
-	struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
+	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
 	uint16_t alloc_size, start, cursor_blocks;
 	uint16_t *minimum = cstate->wm.skl.minimum_blocks;
 	uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
@@ -3366,7 +3365,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		return 0;
 
 	if (!cstate->base.active) {
-		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
+		memset(alloc, 0, sizeof(*alloc));
 		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
 		memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
 		return 0;
@@ -3895,14 +3894,6 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
 			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
 }
 
-bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
-			       const struct skl_ddb_allocation *new,
-			       enum pipe pipe)
-{
-	return new->pipe[pipe].start == old->pipe[pipe].start &&
-	       new->pipe[pipe].end == old->pipe[pipe].end;
-}
-
 static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
 					   const struct skl_ddb_entry *b)
 {
@@ -3910,22 +3901,22 @@ static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
 }
 
 bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
-				 const struct skl_ddb_allocation *old,
-				 const struct skl_ddb_allocation *new,
-				 enum pipe pipe)
+				 struct intel_crtc *intel_crtc)
 {
-	struct drm_device *dev = state->dev;
-	struct intel_crtc *intel_crtc;
-	enum pipe otherp;
+	struct drm_crtc *other_crtc;
+	struct drm_crtc_state *other_cstate;
+	struct intel_crtc *other_intel_crtc;
+	const struct skl_ddb_entry *ddb =
+		&to_intel_crtc_state(intel_crtc->base.state)->wm.skl.ddb;
+	int i;
 
-	for_each_intel_crtc(dev, intel_crtc) {
-		otherp = intel_crtc->pipe;
+	for_each_crtc_in_state(state, other_crtc, other_cstate, i) {
+		other_intel_crtc = to_intel_crtc(other_crtc);
 
-		if (otherp == pipe)
+		if (other_intel_crtc == intel_crtc)
 			continue;
 
-		if (skl_ddb_entries_overlap(&new->pipe[pipe],
-					    &old->pipe[otherp]))
+		if (skl_ddb_entries_overlap(ddb, &other_intel_crtc->hw_ddb))
 			return true;
 	}
 
@@ -4084,7 +4075,6 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst,
 	memcpy(dst->plane_trans[pipe], src->plane_trans[pipe],
 	       sizeof(dst->plane_trans[pipe]));
 
-	dst->ddb.pipe[pipe] = src->ddb.pipe[pipe];
 	memcpy(dst->ddb.y_plane[pipe], src->ddb.y_plane[pipe],
 	       sizeof(dst->ddb.y_plane[pipe]));
 	memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe],
@@ -4192,6 +4182,8 @@ static void skl_update_wm(struct drm_crtc *crtc)
 
 	skl_copy_wm_for_pipe(hw_vals, results, pipe);
 
+	intel_crtc->hw_ddb = cstate->wm.skl.ddb;
+
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
 
-- 
2.7.4

[PATCH 2/6] drm/i915/skl: Remove linetime from skl_wm_values

[Lyude]

Next part of cleaning up the watermark code for skl. This is easy, since
it seems that we never actually needed to keep track of the linetime in
the skl_wm_values struct anyway.

Signed-off-by: Lyude <cpaul@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 1 -
 drivers/gpu/drm/i915/intel_display.c | 6 ++++--
 drivers/gpu/drm/i915/intel_pm.c      | 7 +------
 3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 85e541c..d26e5999 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1643,7 +1643,6 @@ struct skl_ddb_allocation {
 struct skl_wm_values {
 	unsigned dirty_pipes;
 	struct skl_ddb_allocation ddb;
-	uint32_t wm_linetime[I915_MAX_PIPES];
 	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
 	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
 };
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 17733af..a71d05a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14832,6 +14832,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *intel_cstate =
+		to_intel_crtc_state(crtc->state);
 	struct intel_crtc_state *old_intel_state =
 		to_intel_crtc_state(old_crtc_state);
 	bool modeset = needs_modeset(crtc->state);
@@ -14848,13 +14850,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 		intel_color_load_luts(crtc->state);
 	}
 
-	if (to_intel_crtc_state(crtc->state)->update_pipe)
+	if (intel_cstate->update_pipe)
 		intel_update_pipe_config(intel_crtc, old_intel_state);
 	else if (INTEL_GEN(dev_priv) >= 9) {
 		skl_detach_scalers(intel_crtc);
 
 		I915_WRITE(PIPE_WM_LINETIME(pipe),
-			   dev_priv->wm.skl_hw.wm_linetime[pipe]);
+			   intel_cstate->wm.skl.optimal.linetime);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0383516..af96888 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3839,8 +3839,6 @@ static void skl_compute_wm_results(struct drm_device *dev,
 		temp |= PLANE_WM_EN;
 
 	r->plane_trans[pipe][PLANE_CURSOR] = temp;
-
-	r->wm_linetime[pipe] = p_wm->linetime;
 }
 
 static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
@@ -4069,7 +4067,6 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst,
 		     struct skl_wm_values *src,
 		     enum pipe pipe)
 {
-	dst->wm_linetime[pipe] = src->wm_linetime[pipe];
 	memcpy(dst->plane[pipe], src->plane[pipe],
 	       sizeof(dst->plane[pipe]));
 	memcpy(dst->plane_trans[pipe], src->plane_trans[pipe],
@@ -4320,8 +4317,6 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 
 	max_level = ilk_wm_max_level(dev);
 
-	hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
-
 	for (level = 0; level <= max_level; level++) {
 		for (i = 0; i < intel_num_planes(intel_crtc); i++)
 			hw->plane[pipe][i][level] =
@@ -4338,7 +4333,7 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 
 	hw->dirty_pipes |= drm_crtc_mask(crtc);
 
-	active->linetime = hw->wm_linetime[pipe];
+	active->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
 
 	for (level = 0; level <= max_level; level++) {
 		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
-- 
2.7.4

[PATCH 3/6] drm/i915: Add enable_sagv option

[Lyude]

This option allows us to manually control the SAGV at module load time.
This can be useful in situations such as trying to debug watermark
changes, since enabled SAGV + incorrect watermarks = total GPU
annihilation.

Signed-off-by: Lyude <cpaul@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c   |  5 +++++
 drivers/gpu/drm/i915/i915_params.h   |  1 +
 drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++++---
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 768ad89..f462cd4 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -62,6 +62,7 @@ struct i915_params i915 __read_mostly = {
 	.inject_load_failure = 0,
 	.enable_dpcd_backlight = false,
 	.enable_gvt = false,
+	.enable_sagv = -1,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -233,3 +234,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight,
 module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
 MODULE_PARM_DESC(enable_gvt,
 	"Enable support for Intel GVT-g graphics virtualization host support(default:false)");
+
+module_param_named_unsafe(enable_sagv, i915.enable_sagv, int, 0400);
+MODULE_PARM_DESC(enable_sagv,
+	"Enable the SAGV (gen9+ only)(1=enabled, 0=disabled, -1=driver discretion [default])");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 3a0dd78..a7db125 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -65,6 +65,7 @@ struct i915_params {
 	bool enable_dp_mst;
 	bool enable_dpcd_backlight;
 	bool enable_gvt;
+	int enable_sagv;
 };
 
 extern struct i915_params i915 __read_mostly;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a71d05a..dd15ae2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -16904,12 +16904,22 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
 		pll->on = false;
 	}
 
-	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
+	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
 		vlv_wm_get_hw_state(dev);
-	else if (IS_GEN9(dev))
+	} else if (IS_GEN9(dev)) {
 		skl_wm_get_hw_state(dev);
-	else if (HAS_PCH_SPLIT(dev))
+
+		if (i915.enable_sagv != -1) {
+			if (i915.enable_sagv)
+				intel_enable_sagv(dev_priv);
+			else
+				intel_disable_sagv(dev_priv);
+
+			dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
+		}
+	} else if (HAS_PCH_SPLIT(dev)) {
 		ilk_wm_get_hw_state(dev);
+	}
 
 	for_each_intel_crtc(dev, crtc) {
 		unsigned long put_domains;
-- 
2.7.4

[PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane

[Lyude]

Having skl_wm_level contain all of the watermarks for each plane is
annoying since it prevents us from having any sort of object to
represent a single watermark level, something we take advantage of in
the next commit to cut down on all of the copy paste code in here.

Signed-off-by: Lyude <cpaul@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |   6 +-
 drivers/gpu/drm/i915/intel_drv.h |   6 +-
 drivers/gpu/drm/i915/intel_pm.c  | 208 +++++++++++++++++----------------------
 3 files changed, 100 insertions(+), 120 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d26e5999..0f97d43 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1648,9 +1648,9 @@ struct skl_wm_values {
 };
 
 struct skl_wm_level {
-	bool plane_en[I915_MAX_PLANES];
-	uint16_t plane_res_b[I915_MAX_PLANES];
-	uint8_t plane_res_l[I915_MAX_PLANES];
+	bool plane_en;
+	uint16_t plane_res_b;
+	uint8_t plane_res_l;
 };
 
 /*
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 35ba282..d684f4f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -468,9 +468,13 @@ struct intel_pipe_wm {
 	bool sprites_scaled;
 };
 
-struct skl_pipe_wm {
+struct skl_plane_wm {
 	struct skl_wm_level wm[8];
 	struct skl_wm_level trans_wm;
+};
+
+struct skl_pipe_wm {
+	struct skl_plane_wm planes[I915_MAX_PLANES];
 	uint32_t linetime;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index af96888..250f12d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3668,67 +3668,52 @@ static int
 skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 		     struct skl_ddb_allocation *ddb,
 		     struct intel_crtc_state *cstate,
+		     struct intel_plane *intel_plane,
 		     int level,
 		     struct skl_wm_level *result)
 {
 	struct drm_atomic_state *state = cstate->base.state;
 	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
-	struct drm_plane *plane;
-	struct intel_plane *intel_plane;
-	struct intel_plane_state *intel_pstate;
+	struct drm_plane *plane = &intel_plane->base;
+	struct intel_plane_state *intel_pstate = NULL;
 	uint16_t ddb_blocks;
 	enum pipe pipe = intel_crtc->pipe;
 	int ret;
+	int i = skl_wm_plane_id(intel_plane);
+
+	if (state)
+		intel_pstate =
+			intel_atomic_get_existing_plane_state(state,
+							      intel_plane);
 
 	/*
-	 * We'll only calculate watermarks for planes that are actually
-	 * enabled, so make sure all other planes are set as disabled.
+	 * Note: If we start supporting multiple pending atomic commits against
+	 * the same planes/CRTC's in the future, plane->state will no longer be
+	 * the correct pre-state to use for the calculations here and we'll
+	 * need to change where we get the 'unchanged' plane data from.
+	 *
+	 * For now this is fine because we only allow one queued commit against
+	 * a CRTC.  Even if the plane isn't modified by this transaction and we
+	 * don't have a plane lock, we still have the CRTC's lock, so we know
+	 * that no other transactions are racing with us to update it.
 	 */
-	memset(result, 0, sizeof(*result));
-
-	for_each_intel_plane_mask(&dev_priv->drm,
-				  intel_plane,
-				  cstate->base.plane_mask) {
-		int i = skl_wm_plane_id(intel_plane);
-
-		plane = &intel_plane->base;
-		intel_pstate = NULL;
-		if (state)
-			intel_pstate =
-				intel_atomic_get_existing_plane_state(state,
-								      intel_plane);
+	if (!intel_pstate)
+		intel_pstate = to_intel_plane_state(plane->state);
 
-		/*
-		 * Note: If we start supporting multiple pending atomic commits
-		 * against the same planes/CRTC's in the future, plane->state
-		 * will no longer be the correct pre-state to use for the
-		 * calculations here and we'll need to change where we get the
-		 * 'unchanged' plane data from.
-		 *
-		 * For now this is fine because we only allow one queued commit
-		 * against a CRTC.  Even if the plane isn't modified by this
-		 * transaction and we don't have a plane lock, we still have
-		 * the CRTC's lock, so we know that no other transactions are
-		 * racing with us to update it.
-		 */
-		if (!intel_pstate)
-			intel_pstate = to_intel_plane_state(plane->state);
+	WARN_ON(!intel_pstate->base.fb);
 
-		WARN_ON(!intel_pstate->base.fb);
+	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
 
-		ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
-
-		ret = skl_compute_plane_wm(dev_priv,
-					   cstate,
-					   intel_pstate,
-					   ddb_blocks,
-					   level,
-					   &result->plane_res_b[i],
-					   &result->plane_res_l[i],
-					   &result->plane_en[i]);
-		if (ret)
-			return ret;
-	}
+	ret = skl_compute_plane_wm(dev_priv,
+				   cstate,
+				   intel_pstate,
+				   ddb_blocks,
+				   level,
+				   &result->plane_res_b,
+				   &result->plane_res_l,
+				   &result->plane_en);
+	if (ret)
+		return ret;
 
 	return 0;
 }
@@ -3749,19 +3734,11 @@ skl_compute_linetime_wm(struct intel_crtc_state *cstate)
 static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
 				      struct skl_wm_level *trans_wm /* out */)
 {
-	struct drm_crtc *crtc = cstate->base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_plane *intel_plane;
-
 	if (!cstate->base.active)
 		return;
 
 	/* Until we know more, just disable transition WMs */
-	for_each_intel_plane_on_crtc(crtc->dev, intel_crtc, intel_plane) {
-		int i = skl_wm_plane_id(intel_plane);
-
-		trans_wm->plane_en[i] = false;
-	}
+	trans_wm->plane_en = false;
 }
 
 static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
@@ -3770,19 +3747,33 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 {
 	struct drm_device *dev = cstate->base.crtc->dev;
 	const struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_plane *intel_plane;
+	struct skl_plane_wm *wm;
 	int level, max_level = ilk_wm_max_level(dev);
 	int ret;
 
-	for (level = 0; level <= max_level; level++) {
-		ret = skl_compute_wm_level(dev_priv, ddb, cstate,
-					   level, &pipe_wm->wm[level]);
-		if (ret)
-			return ret;
+	/*
+	 * We'll only calculate watermarks for planes that are actually
+	 * enabled, so make sure all other planes are set as disabled.
+	 */
+	memset(pipe_wm->planes, 0, sizeof(pipe_wm->planes));
+
+	for_each_intel_plane_mask(&dev_priv->drm,
+				  intel_plane,
+				  cstate->base.plane_mask) {
+		wm = &pipe_wm->planes[skl_wm_plane_id(intel_plane)];
+
+		for (level = 0; level <= max_level; level++) {
+			ret = skl_compute_wm_level(dev_priv, ddb, cstate,
+						   intel_plane, level,
+						   &wm->wm[level]);
+			if (ret)
+				return ret;
+		}
+		skl_compute_transition_wm(cstate, &wm->trans_wm);
 	}
 	pipe_wm->linetime = skl_compute_linetime_wm(cstate);
 
-	skl_compute_transition_wm(cstate, &pipe_wm->trans_wm);
-
 	return 0;
 }
 
@@ -3792,50 +3783,56 @@ static void skl_compute_wm_results(struct drm_device *dev,
 				   struct intel_crtc *intel_crtc)
 {
 	int level, max_level = ilk_wm_max_level(dev);
+	struct skl_plane_wm *plane_wm;
 	enum pipe pipe = intel_crtc->pipe;
 	uint32_t temp;
 	int i;
 
-	for (level = 0; level <= max_level; level++) {
-		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
+	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
+		plane_wm = &p_wm->planes[i];
+
+		for (level = 0; level <= max_level; level++) {
 			temp = 0;
 
-			temp |= p_wm->wm[level].plane_res_l[i] <<
+			temp |= plane_wm->wm[level].plane_res_l <<
 					PLANE_WM_LINES_SHIFT;
-			temp |= p_wm->wm[level].plane_res_b[i];
-			if (p_wm->wm[level].plane_en[i])
+			temp |= plane_wm->wm[level].plane_res_b;
+			if (plane_wm->wm[level].plane_en)
 				temp |= PLANE_WM_EN;
 
 			r->plane[pipe][i][level] = temp;
 		}
 
-		temp = 0;
-
-		temp |= p_wm->wm[level].plane_res_l[PLANE_CURSOR] << PLANE_WM_LINES_SHIFT;
-		temp |= p_wm->wm[level].plane_res_b[PLANE_CURSOR];
+	}
 
-		if (p_wm->wm[level].plane_en[PLANE_CURSOR])
+	for (level = 0; level <= max_level; level++) {
+		plane_wm = &p_wm->planes[PLANE_CURSOR];
+		temp = 0;
+		temp |= plane_wm->wm[level].plane_res_l << PLANE_WM_LINES_SHIFT;
+		temp |= plane_wm->wm[level].plane_res_b;
+		if (plane_wm->wm[level].plane_en)
 			temp |= PLANE_WM_EN;
 
 		r->plane[pipe][PLANE_CURSOR][level] = temp;
-
 	}
 
 	/* transition WMs */
 	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
+		plane_wm = &p_wm->planes[i];
 		temp = 0;
-		temp |= p_wm->trans_wm.plane_res_l[i] << PLANE_WM_LINES_SHIFT;
-		temp |= p_wm->trans_wm.plane_res_b[i];
-		if (p_wm->trans_wm.plane_en[i])
+		temp |= plane_wm->trans_wm.plane_res_l << PLANE_WM_LINES_SHIFT;
+		temp |= plane_wm->trans_wm.plane_res_b;
+		if (plane_wm->trans_wm.plane_en)
 			temp |= PLANE_WM_EN;
 
 		r->plane_trans[pipe][i] = temp;
 	}
 
+	plane_wm = &p_wm->planes[PLANE_CURSOR];
 	temp = 0;
-	temp |= p_wm->trans_wm.plane_res_l[PLANE_CURSOR] << PLANE_WM_LINES_SHIFT;
-	temp |= p_wm->trans_wm.plane_res_b[PLANE_CURSOR];
-	if (p_wm->trans_wm.plane_en[PLANE_CURSOR])
+	temp |= plane_wm->trans_wm.plane_res_l << PLANE_WM_LINES_SHIFT;
+	temp |= plane_wm->trans_wm.plane_res_b;
+	if (plane_wm->trans_wm.plane_en)
 		temp |= PLANE_WM_EN;
 
 	r->plane_trans[pipe][PLANE_CURSOR] = temp;
@@ -4262,44 +4259,24 @@ static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
 static void skl_pipe_wm_active_state(uint32_t val,
 				     struct skl_pipe_wm *active,
 				     bool is_transwm,
-				     bool is_cursor,
 				     int i,
 				     int level)
 {
+	struct skl_plane_wm *plane_wm = &active->planes[i];
 	bool is_enabled = (val & PLANE_WM_EN) != 0;
 
 	if (!is_transwm) {
-		if (!is_cursor) {
-			active->wm[level].plane_en[i] = is_enabled;
-			active->wm[level].plane_res_b[i] =
-					val & PLANE_WM_BLOCKS_MASK;
-			active->wm[level].plane_res_l[i] =
-					(val >> PLANE_WM_LINES_SHIFT) &
-						PLANE_WM_LINES_MASK;
-		} else {
-			active->wm[level].plane_en[PLANE_CURSOR] = is_enabled;
-			active->wm[level].plane_res_b[PLANE_CURSOR] =
-					val & PLANE_WM_BLOCKS_MASK;
-			active->wm[level].plane_res_l[PLANE_CURSOR] =
-					(val >> PLANE_WM_LINES_SHIFT) &
-						PLANE_WM_LINES_MASK;
-		}
+		plane_wm->wm[level].plane_en = is_enabled;
+		plane_wm->wm[level].plane_res_b = val & PLANE_WM_BLOCKS_MASK;
+		plane_wm->wm[level].plane_res_l =
+			(val >> PLANE_WM_LINES_SHIFT) &
+			PLANE_WM_LINES_MASK;
 	} else {
-		if (!is_cursor) {
-			active->trans_wm.plane_en[i] = is_enabled;
-			active->trans_wm.plane_res_b[i] =
-					val & PLANE_WM_BLOCKS_MASK;
-			active->trans_wm.plane_res_l[i] =
-					(val >> PLANE_WM_LINES_SHIFT) &
-						PLANE_WM_LINES_MASK;
-		} else {
-			active->trans_wm.plane_en[PLANE_CURSOR] = is_enabled;
-			active->trans_wm.plane_res_b[PLANE_CURSOR] =
-					val & PLANE_WM_BLOCKS_MASK;
-			active->trans_wm.plane_res_l[PLANE_CURSOR] =
-					(val >> PLANE_WM_LINES_SHIFT) &
-						PLANE_WM_LINES_MASK;
-		}
+		plane_wm->trans_wm.plane_en = is_enabled;
+		plane_wm->trans_wm.plane_res_b = val & PLANE_WM_BLOCKS_MASK;
+		plane_wm->trans_wm.plane_res_l =
+			(val >> PLANE_WM_LINES_SHIFT) &
+			PLANE_WM_LINES_MASK;
 	}
 }
 
@@ -4338,20 +4315,19 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	for (level = 0; level <= max_level; level++) {
 		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
 			temp = hw->plane[pipe][i][level];
-			skl_pipe_wm_active_state(temp, active, false,
-						false, i, level);
+			skl_pipe_wm_active_state(temp, active, false, i, level);
 		}
 		temp = hw->plane[pipe][PLANE_CURSOR][level];
-		skl_pipe_wm_active_state(temp, active, false, true, i, level);
+		skl_pipe_wm_active_state(temp, active, false, i, level);
 	}
 
 	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
 		temp = hw->plane_trans[pipe][i];
-		skl_pipe_wm_active_state(temp, active, true, false, i, 0);
+		skl_pipe_wm_active_state(temp, active, true, i, 0);
 	}
 
 	temp = hw->plane_trans[pipe][PLANE_CURSOR];
-	skl_pipe_wm_active_state(temp, active, true, true, i, 0);
+	skl_pipe_wm_active_state(temp, active, true, i, 0);
 
 	intel_crtc->wm.active.skl = *active;
 }
-- 
2.7.4

[PATCH 5/6] drm/i915/gen9: Get rid of redundant watermark values

[Lyude]

Now that we've make skl_wm_levels make a little more sense, we can
remove all of the redundant wm information. Up until now we'd been
storing two copies of all of the skl watermarks: one being the
skl_pipe_wm structs, the other being the global wm struct in
drm_i915_private containing the raw register values. This is confusing
and problematic, since it means we're prone to accidentally letting the
two copies go out of sync. So, get rid of all of the functions
responsible for computing the register values and just use a single
helper, skl_write_wm_level(), to convert and write the new watermarks on
the fly.

Signed-off-by: Lyude <cpaul@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   2 -
 drivers/gpu/drm/i915/intel_display.c |  14 ++-
 drivers/gpu/drm/i915/intel_drv.h     |   6 +-
 drivers/gpu/drm/i915/intel_pm.c      | 203 ++++++++++++-----------------------
 drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
 5 files changed, 88 insertions(+), 145 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0f97d43..63519ac 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1643,8 +1643,6 @@ struct skl_ddb_allocation {
 struct skl_wm_values {
 	unsigned dirty_pipes;
 	struct skl_ddb_allocation ddb;
-	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
-	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
 };
 
 struct skl_wm_level {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dd15ae2..c580d3d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3378,6 +3378,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_framebuffer *fb = plane_state->base.fb;
 	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	const struct skl_plane_wm *p_wm =
+		&crtc_state->wm.skl.optimal.planes[0];
 	int pipe = intel_crtc->pipe;
 	u32 plane_ctl;
 	unsigned int rotation = plane_state->base.rotation;
@@ -3414,7 +3416,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_y = src_y;
 
 	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
-		skl_write_plane_wm(intel_crtc, wm, 0);
+		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
 
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
@@ -3448,6 +3450,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
+	const struct skl_plane_wm *p_wm = &cstate->wm.skl.optimal.planes[0];
 	int pipe = intel_crtc->pipe;
 
 	/*
@@ -3455,7 +3459,8 @@ static void skylake_disable_primary_plane(struct drm_plane *primary,
 	 * plane's visiblity isn't actually changing neither is its watermarks.
 	 */
 	if (!crtc->primary->state->visible)
-		skl_write_plane_wm(intel_crtc, &dev_priv->wm.skl_results, 0);
+		skl_write_plane_wm(intel_crtc, p_wm,
+				   &dev_priv->wm.skl_results.ddb, 0);
 
 	I915_WRITE(PLANE_CTL(pipe, 0), 0);
 	I915_WRITE(PLANE_SURF(pipe, 0), 0);
@@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	const struct skl_plane_wm *p_wm =
+		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
 	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
-		skl_write_cursor_wm(intel_crtc, wm);
+		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
 
 	if (plane_state && plane_state->base.visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d684f4f..958dc72 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
 bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
 				 struct intel_crtc *intel_crtc);
 void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
-			 const struct skl_wm_values *wm);
+			 const struct skl_plane_wm *wm,
+			 const struct skl_ddb_allocation *ddb);
 void skl_write_plane_wm(struct intel_crtc *intel_crtc,
-			const struct skl_wm_values *wm,
+			const struct skl_plane_wm *wm,
+			const struct skl_ddb_allocation *ddb,
 			int plane);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 bool ilk_disable_lp_wm(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 250f12d..7708646 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3000,6 +3000,8 @@ bool intel_can_enable_sagv(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 drm_crtc *crtc;
+	struct intel_crtc_state *cstate;
+	struct skl_plane_wm *wm;
 	enum pipe pipe;
 	int level, plane;
 
@@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 	/* Since we're now guaranteed to only have one active CRTC... */
 	pipe = ffs(intel_state->active_crtcs) - 1;
 	crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+	cstate = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
 
 	if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE)
 		return false;
 
 	for_each_plane(dev_priv, pipe, plane) {
+		wm = &cstate->wm.skl.optimal.planes[plane];
+
 		/* Skip this plane if it's not enabled */
-		if (intel_state->wm_results.plane[pipe][plane][0] == 0)
+		if (!wm->wm[0].plane_en)
 			continue;
 
 		/* Find the highest enabled wm level for this plane */
-		for (level = ilk_wm_max_level(dev);
-		     intel_state->wm_results.plane[pipe][plane][level] == 0; --level)
+		for (level = ilk_wm_max_level(dev); !wm->wm[level].plane_en;
+		     --level)
 		     { }
 
 		/*
@@ -3777,67 +3782,6 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 	return 0;
 }
 
-static void skl_compute_wm_results(struct drm_device *dev,
-				   struct skl_pipe_wm *p_wm,
-				   struct skl_wm_values *r,
-				   struct intel_crtc *intel_crtc)
-{
-	int level, max_level = ilk_wm_max_level(dev);
-	struct skl_plane_wm *plane_wm;
-	enum pipe pipe = intel_crtc->pipe;
-	uint32_t temp;
-	int i;
-
-	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
-		plane_wm = &p_wm->planes[i];
-
-		for (level = 0; level <= max_level; level++) {
-			temp = 0;
-
-			temp |= plane_wm->wm[level].plane_res_l <<
-					PLANE_WM_LINES_SHIFT;
-			temp |= plane_wm->wm[level].plane_res_b;
-			if (plane_wm->wm[level].plane_en)
-				temp |= PLANE_WM_EN;
-
-			r->plane[pipe][i][level] = temp;
-		}
-
-	}
-
-	for (level = 0; level <= max_level; level++) {
-		plane_wm = &p_wm->planes[PLANE_CURSOR];
-		temp = 0;
-		temp |= plane_wm->wm[level].plane_res_l << PLANE_WM_LINES_SHIFT;
-		temp |= plane_wm->wm[level].plane_res_b;
-		if (plane_wm->wm[level].plane_en)
-			temp |= PLANE_WM_EN;
-
-		r->plane[pipe][PLANE_CURSOR][level] = temp;
-	}
-
-	/* transition WMs */
-	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
-		plane_wm = &p_wm->planes[i];
-		temp = 0;
-		temp |= plane_wm->trans_wm.plane_res_l << PLANE_WM_LINES_SHIFT;
-		temp |= plane_wm->trans_wm.plane_res_b;
-		if (plane_wm->trans_wm.plane_en)
-			temp |= PLANE_WM_EN;
-
-		r->plane_trans[pipe][i] = temp;
-	}
-
-	plane_wm = &p_wm->planes[PLANE_CURSOR];
-	temp = 0;
-	temp |= plane_wm->trans_wm.plane_res_l << PLANE_WM_LINES_SHIFT;
-	temp |= plane_wm->trans_wm.plane_res_b;
-	if (plane_wm->trans_wm.plane_en)
-		temp |= PLANE_WM_EN;
-
-	r->plane_trans[pipe][PLANE_CURSOR] = temp;
-}
-
 static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
 				i915_reg_t reg,
 				const struct skl_ddb_entry *entry)
@@ -3848,8 +3792,22 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
 		I915_WRITE(reg, 0);
 }
 
+static void skl_write_wm_level(struct drm_i915_private *dev_priv,
+			       i915_reg_t reg,
+			       const struct skl_wm_level *level)
+{
+	uint32_t val = 0;
+
+	val |= level->plane_res_b;
+	val |= level->plane_res_l << PLANE_WM_LINES_SHIFT;
+	val |= level->plane_en;
+
+	I915_WRITE(reg, val);
+}
+
 void skl_write_plane_wm(struct intel_crtc *intel_crtc,
-			const struct skl_wm_values *wm,
+			const struct skl_plane_wm *wm,
+			const struct skl_ddb_allocation *ddb,
 			int plane)
 {
 	struct drm_crtc *crtc = &intel_crtc->base;
@@ -3859,19 +3817,21 @@ void skl_write_plane_wm(struct intel_crtc *intel_crtc,
 	enum pipe pipe = intel_crtc->pipe;
 
 	for (level = 0; level <= max_level; level++) {
-		I915_WRITE(PLANE_WM(pipe, plane, level),
-			   wm->plane[pipe][plane][level]);
+		skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane, level),
+				   &wm->wm[level]);
 	}
-	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
+	skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane),
+			   &wm->trans_wm);
 
 	skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane),
-			    &wm->ddb.plane[pipe][plane]);
+			    &ddb->plane[pipe][plane]);
 	skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, plane),
-			    &wm->ddb.y_plane[pipe][plane]);
+			    &ddb->y_plane[pipe][plane]);
 }
 
 void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
-			 const struct skl_wm_values *wm)
+			 const struct skl_plane_wm *wm,
+			 const struct skl_ddb_allocation *ddb)
 {
 	struct drm_crtc *crtc = &intel_crtc->base;
 	struct drm_device *dev = crtc->dev;
@@ -3880,13 +3840,13 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
 	enum pipe pipe = intel_crtc->pipe;
 
 	for (level = 0; level <= max_level; level++) {
-		I915_WRITE(CUR_WM(pipe, level),
-			   wm->plane[pipe][PLANE_CURSOR][level]);
+		skl_write_wm_level(dev_priv, CUR_WM(pipe, level),
+				   &wm->wm[level]);
 	}
-	I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
+	skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm->trans_wm);
 
 	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
-			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
+			    &ddb->plane[pipe][PLANE_CURSOR]);
 }
 
 static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
@@ -4064,11 +4024,6 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst,
 		     struct skl_wm_values *src,
 		     enum pipe pipe)
 {
-	memcpy(dst->plane[pipe], src->plane[pipe],
-	       sizeof(dst->plane[pipe]));
-	memcpy(dst->plane_trans[pipe], src->plane_trans[pipe],
-	       sizeof(dst->plane_trans[pipe]));
-
 	memcpy(dst->ddb.y_plane[pipe], src->ddb.y_plane[pipe],
 	       sizeof(dst->ddb.y_plane[pipe]));
 	memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe],
@@ -4117,7 +4072,6 @@ skl_compute_wm(struct drm_atomic_state *state)
 	 * 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);
 
@@ -4135,7 +4089,6 @@ skl_compute_wm(struct drm_atomic_state *state)
 			continue;
 
 		intel_cstate->update_wm_pre = true;
-		skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
 	}
 
 	return 0;
@@ -4169,9 +4122,11 @@ static void skl_update_wm(struct drm_crtc *crtc)
 		int plane;
 
 		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
-			skl_write_plane_wm(intel_crtc, results, plane);
+			skl_write_plane_wm(intel_crtc, &pipe_wm->planes[plane],
+					   &results->ddb, plane);
 
-		skl_write_cursor_wm(intel_crtc, results);
+		skl_write_cursor_wm(intel_crtc, &pipe_wm->planes[PLANE_CURSOR],
+				    &results->ddb);
 	}
 
 	skl_copy_wm_for_pipe(hw_vals, results, pipe);
@@ -4256,28 +4211,13 @@ static void ilk_optimize_watermarks(struct intel_crtc_state *cstate)
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
 
-static void skl_pipe_wm_active_state(uint32_t val,
-				     struct skl_pipe_wm *active,
-				     bool is_transwm,
-				     int i,
-				     int level)
+static inline void skl_wm_level_from_reg_val(uint32_t val,
+					     struct skl_wm_level *level)
 {
-	struct skl_plane_wm *plane_wm = &active->planes[i];
-	bool is_enabled = (val & PLANE_WM_EN) != 0;
-
-	if (!is_transwm) {
-		plane_wm->wm[level].plane_en = is_enabled;
-		plane_wm->wm[level].plane_res_b = val & PLANE_WM_BLOCKS_MASK;
-		plane_wm->wm[level].plane_res_l =
-			(val >> PLANE_WM_LINES_SHIFT) &
-			PLANE_WM_LINES_MASK;
-	} else {
-		plane_wm->trans_wm.plane_en = is_enabled;
-		plane_wm->trans_wm.plane_res_b = val & PLANE_WM_BLOCKS_MASK;
-		plane_wm->trans_wm.plane_res_l =
-			(val >> PLANE_WM_LINES_SHIFT) &
-			PLANE_WM_LINES_MASK;
-	}
+	level->plane_en = val & PLANE_WM_EN;
+	level->plane_res_b = val & PLANE_WM_BLOCKS_MASK;
+	level->plane_res_l = (val & PLANE_WM_LINES_MASK) >>
+		PLANE_WM_LINES_SHIFT;
 }
 
 static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
@@ -4287,23 +4227,33 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	struct skl_wm_values *hw = &dev_priv->wm.skl_hw;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
+	struct intel_plane *intel_plane;
 	struct skl_pipe_wm *active = &cstate->wm.skl.optimal;
+	struct skl_plane_wm *wm;
 	enum pipe pipe = intel_crtc->pipe;
-	int level, i, max_level;
-	uint32_t temp;
+	int level, id, max_level = ilk_wm_max_level(dev);
+	uint32_t val;
 
-	max_level = ilk_wm_max_level(dev);
+	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
+		id = skl_wm_plane_id(intel_plane);
+		wm = &cstate->wm.skl.optimal.planes[id];
 
-	for (level = 0; level <= max_level; level++) {
-		for (i = 0; i < intel_num_planes(intel_crtc); i++)
-			hw->plane[pipe][i][level] =
-					I915_READ(PLANE_WM(pipe, i, level));
-		hw->plane[pipe][PLANE_CURSOR][level] = I915_READ(CUR_WM(pipe, level));
-	}
+		for (level = 0; level <= max_level; level++) {
+			if (id != PLANE_CURSOR)
+				val = I915_READ(PLANE_WM(pipe, id, level));
+			else
+				val = I915_READ(CUR_WM(pipe, level));
+
+			skl_wm_level_from_reg_val(val, &wm->wm[level]);
+		}
 
-	for (i = 0; i < intel_num_planes(intel_crtc); i++)
-		hw->plane_trans[pipe][i] = I915_READ(PLANE_WM_TRANS(pipe, i));
-	hw->plane_trans[pipe][PLANE_CURSOR] = I915_READ(CUR_WM_TRANS(pipe));
+		if (id != PLANE_CURSOR)
+			val = I915_READ(PLANE_WM_TRANS(pipe, id));
+		else
+			val = I915_READ(CUR_WM_TRANS(pipe));
+
+		skl_wm_level_from_reg_val(val, &wm->trans_wm);
+	}
 
 	if (!intel_crtc->active)
 		return;
@@ -4311,25 +4261,6 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	hw->dirty_pipes |= drm_crtc_mask(crtc);
 
 	active->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
-
-	for (level = 0; level <= max_level; level++) {
-		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
-			temp = hw->plane[pipe][i][level];
-			skl_pipe_wm_active_state(temp, active, false, i, level);
-		}
-		temp = hw->plane[pipe][PLANE_CURSOR][level];
-		skl_pipe_wm_active_state(temp, active, false, i, level);
-	}
-
-	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
-		temp = hw->plane_trans[pipe][i];
-		skl_pipe_wm_active_state(temp, active, true, i, 0);
-	}
-
-	temp = hw->plane_trans[pipe][PLANE_CURSOR];
-	skl_pipe_wm_active_state(temp, active, true, i, 0);
-
-	intel_crtc->wm.active.skl = *active;
 }
 
 void skl_wm_get_hw_state(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 73a521f..0fb775b 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -208,6 +208,8 @@ skl_update_plane(struct drm_plane *drm_plane,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
+	const struct skl_plane_wm *p_wm =
+		&crtc_state->wm.skl.optimal.planes[plane];
 	u32 plane_ctl;
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
 	u32 surf_addr = plane_state->main.offset;
@@ -232,7 +234,7 @@ skl_update_plane(struct drm_plane *drm_plane,
 	plane_ctl |= skl_plane_ctl_rotation(rotation);
 
 	if (wm->dirty_pipes & drm_crtc_mask(crtc))
-		skl_write_plane_wm(intel_crtc, wm, plane);
+		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, plane);
 
 	if (key->flags) {
 		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
@@ -289,6 +291,7 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
+	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 
@@ -298,7 +301,8 @@ skl_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	 */
 	if (!dplane->state->visible)
 		skl_write_plane_wm(to_intel_crtc(crtc),
-				   &dev_priv->wm.skl_results, plane);
+				   &cstate->wm.skl.optimal.planes[plane],
+				   &dev_priv->wm.skl_results.ddb, plane);
 
 	I915_WRITE(PLANE_CTL(pipe, plane), 0);
 
-- 
2.7.4

[PATCH 6/6] drm/i915/gen9: Add ddb changes to atomic debug output

[Lyude]

Finally, add some debugging output for ddb changes in the atomic debug
output. This makes it a lot easier to spot bugs from incorrect ddb
allocations.

Signed-off-by: Lyude <cpaul@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 57 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7708646..2691428 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4030,6 +4030,61 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst,
 	       sizeof(dst->ddb.plane[pipe]));
 }
 
+static void
+skl_print_wm_changes(const struct drm_atomic_state *state)
+{
+	const struct drm_device *dev = state->dev;
+	const struct drm_i915_private *dev_priv = to_i915(dev);
+	const struct intel_atomic_state *intel_state =
+		to_intel_atomic_state(state);
+	const struct drm_crtc *crtc;
+	const struct drm_crtc_state *cstate;
+	const struct drm_plane *plane;
+	const struct intel_plane *intel_plane;
+	const struct drm_plane_state *pstate;
+	const struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb;
+	const struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
+	enum pipe pipe;
+	int id;
+	int i, j;
+
+	for_each_crtc_in_state(state, crtc, cstate, i) {
+		if (!crtc->state)
+			continue;
+
+		pipe = to_intel_crtc(crtc)->pipe;
+
+		for_each_plane_in_state(state, plane, pstate, j) {
+			const struct skl_ddb_entry *old, *new;
+
+			intel_plane = to_intel_plane(plane);
+			id = skl_wm_plane_id(intel_plane);
+			old = &old_ddb->plane[pipe][id];
+			new = &new_ddb->plane[pipe][id];
+
+			if (intel_plane->pipe != pipe)
+				continue;
+
+			if (skl_ddb_entry_equal(old, new))
+				continue;
+
+			if (id != PLANE_CURSOR) {
+				DRM_DEBUG_ATOMIC("[PLANE:%d:plane %d%c] ddb (%d - %d) -> (%d - %d)\n",
+						 plane->base.id, id + 1,
+						 pipe_name(pipe),
+						 old->start, old->end,
+						 new->start, new->end);
+			} else {
+				DRM_DEBUG_ATOMIC("[PLANE:%d:cursor %c] ddb (%d - %d) -> (%d - %d)\n",
+						 plane->base.id,
+						 pipe_name(pipe),
+						 old->start, old->end,
+						 new->start, new->end);
+			}
+		}
+	}
+}
+
 static int
 skl_compute_wm(struct drm_atomic_state *state)
 {
@@ -4091,6 +4146,8 @@ skl_compute_wm(struct drm_atomic_state *state)
 		intel_cstate->update_wm_pre = true;
 	}
 
+	skl_print_wm_changes(state);
+
 	return 0;
 }
 
-- 
2.7.4

Re: [Intel-gfx] [PATCH 3/6] drm/i915: Add enable_sagv option

[Paulo Zanoni]

Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
> This option allows us to manually control the SAGV at module load
> time.
> This can be useful in situations such as trying to debug watermark
> changes, since enabled SAGV + incorrect watermarks = total GPU
> annihilation.

I'm not a huge fan of adding options that are only for very limited
debugging situations, especially simple ones that can always just be
re-implemented during debugging sessions such as this one. Anyway, I'm
not opposed to adding the option since it's marked as unsafe anyway,
I'm just stating my general opinion. See more below.


> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_params.c   |  5 +++++
>  drivers/gpu/drm/i915/i915_params.h   |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 16 +++++++++++++---
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index 768ad89..f462cd4 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -62,6 +62,7 @@ struct i915_params i915 __read_mostly = {
>  	.inject_load_failure = 0,
>  	.enable_dpcd_backlight = false,
>  	.enable_gvt = false,
> +	.enable_sagv = -1,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -233,3 +234,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight,
>  module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
>  MODULE_PARM_DESC(enable_gvt,
>  	"Enable support for Intel GVT-g graphics virtualization host
> support(default:false)");
> +
> +module_param_named_unsafe(enable_sagv, i915.enable_sagv, int, 0400);
> +MODULE_PARM_DESC(enable_sagv,
> +	"Enable the SAGV (gen9+ only)(1=enabled, 0=disabled,
> -1=driver discretion [default])");
> diff --git a/drivers/gpu/drm/i915/i915_params.h
> b/drivers/gpu/drm/i915/i915_params.h
> index 3a0dd78..a7db125 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -65,6 +65,7 @@ struct i915_params {
>  	bool enable_dp_mst;
>  	bool enable_dpcd_backlight;
>  	bool enable_gvt;
> +	int enable_sagv;
>  };
>  
>  extern struct i915_params i915 __read_mostly;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index a71d05a..dd15ae2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -16904,12 +16904,22 @@ intel_modeset_setup_hw_state(struct
> drm_device *dev)
>  		pll->on = false;
>  	}
>  
> -	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> +	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
>  		vlv_wm_get_hw_state(dev);
> -	else if (IS_GEN9(dev))
> +	} else if (IS_GEN9(dev)) {
>  		skl_wm_get_hw_state(dev);
> -	else if (HAS_PCH_SPLIT(dev))
> +
> +		if (i915.enable_sagv != -1) {
> +			if (i915.enable_sagv)
> +				intel_enable_sagv(dev_priv);
> +			else
> +				intel_disable_sagv(dev_priv);
> +
> +			dev_priv->sagv_status =
> I915_SAGV_NOT_CONTROLLED;

Adding this code to the middle of a get_hw_state() if-ladder doesn't
seem to be the best approach. My suggestion would be to create
intel_setup_sagv() (or intel_init_sagv) and then call it from somewhere
(maybe the end of this function?).

Also, I915_SAGV_NOT_CONTROLLED is only used on Skylake. By setting
sagv_status to to you're making i915.enable_sagv behave differently on
SKL compared to "all the other" (aka only KBL now) platforms. It would
probably be better to have unified behavior, maybe by reworking the
I915_SAGV_NOT_CONTROLLED handling or just adding a new flag or
something else.


> +		}
> +	} else if (HAS_PCH_SPLIT(dev)) {
>  		ilk_wm_get_hw_state(dev);
> +	}
>  
>  	for_each_intel_crtc(dev, crtc) {
>  		unsigned long put_domains;

Re: [Intel-gfx] [PATCH 1/6] drm/i915/skl: Move per-pipe ddb allocations into crtc states

[Paulo Zanoni]

Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
> First part of cleaning up all of the skl watermark code. This moves
> the
> structures for storing the ddb allocations of each pipe into
> intel_crtc_state, along with moving the structures for storing the
> current ddb allocations active on hardware into intel_crtc.
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  1 -
>  drivers/gpu/drm/i915/intel_display.c | 16 ++++++++-------
>  drivers/gpu/drm/i915/intel_drv.h     |  8 +++++---
>  drivers/gpu/drm/i915/intel_pm.c      | 40 +++++++++++++++-----------
> ----------
>  4 files changed, 30 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index f8c66ee..85e541c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1636,7 +1636,6 @@ static inline bool skl_ddb_entry_equal(const
> struct skl_ddb_entry *e1,
>  }
>  
>  struct skl_ddb_allocation {
> -	struct skl_ddb_entry pipe[I915_MAX_PIPES];
>  	struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES];
> /* packed/uv */
>  	struct skl_ddb_entry
> y_plane[I915_MAX_PIPES][I915_MAX_PLANES];
>  };
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index a366656..17733af 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14235,12 +14235,11 @@ static void skl_update_crtcs(struct
> drm_atomic_state *state,
>  			     unsigned int *crtc_vblank_mask)
>  {
>  	struct drm_device *dev = state->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
>  	struct drm_crtc *crtc;
> +	struct intel_crtc *intel_crtc;
>  	struct drm_crtc_state *old_crtc_state;
> -	struct skl_ddb_allocation *new_ddb = &intel_state-
> >wm_results.ddb;
> -	struct skl_ddb_allocation *cur_ddb = &dev_priv-
> >wm.skl_hw.ddb;
> +	struct intel_crtc_state *cstate;
>  	unsigned int updated = 0;
>  	bool progress;
>  	enum pipe pipe;
> @@ -14258,12 +14257,14 @@ static void skl_update_crtcs(struct
> drm_atomic_state *state,
>  		for_each_crtc_in_state(state, crtc, old_crtc_state,
> i) {
>  			bool vbl_wait = false;
>  			unsigned int cmask = drm_crtc_mask(crtc);
> -			pipe = to_intel_crtc(crtc)->pipe;
> +
> +			intel_crtc = to_intel_crtc(crtc);
> +			cstate = to_intel_crtc_state(crtc->state);
> +			pipe = intel_crtc->pipe;
>  
>  			if (updated & cmask || !crtc->state->active)
>  				continue;
> -			if (skl_ddb_allocation_overlaps(state,
> cur_ddb, new_ddb,
> -							pipe))
> +			if (skl_ddb_allocation_overlaps(state,
> intel_crtc))
>  				continue;
>  
>  			updated |= cmask;
> @@ -14274,7 +14275,8 @@ static void skl_update_crtcs(struct
> drm_atomic_state *state,
>  			 * then we need to wait for a vblank to pass
> for the
>  			 * new ddb allocation to take effect.
>  			 */
> -			if (!skl_ddb_allocation_equals(cur_ddb,
> new_ddb, pipe) &&
> +			if (!skl_ddb_entry_equal(&cstate-
> >wm.skl.ddb,
> +						 &intel_crtc-
> >hw_ddb) &&
>  			    !crtc->state->active_changed &&
>  			    intel_state->wm_results.dirty_pipes !=
> updated)
>  				vbl_wait = true;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index f48e79a..35ba282 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -496,6 +496,7 @@ struct intel_crtc_wm_state {
>  		struct {
>  			/* gen9+ only needs 1-step wm programming */
>  			struct skl_pipe_wm optimal;
> +			struct skl_ddb_entry ddb;
>  
>  			/* cached plane data rate */
>  			unsigned plane_data_rate[I915_MAX_PLANES];
> @@ -733,6 +734,9 @@ struct intel_crtc {
>  		bool cxsr_allowed;
>  	} wm;
>  
> +	/* gen9+: ddb allocation currently being used */
> +	struct skl_ddb_entry hw_ddb;
> +
>  	int scanline_offset;
>  
>  	struct {
> @@ -1755,9 +1759,7 @@ bool skl_ddb_allocation_equals(const struct
> skl_ddb_allocation *old,
>  			       const struct skl_ddb_allocation *new,
>  			       enum pipe pipe);
>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> -				 const struct skl_ddb_allocation
> *old,
> -				 const struct skl_ddb_allocation
> *new,
> -				 enum pipe pipe);
> +				 struct intel_crtc *intel_crtc);
>  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
>  			 const struct skl_wm_values *wm);
>  void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 7f1748a..0383516 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3058,7 +3058,6 @@ skl_ddb_get_pipe_allocation_limits(struct
> drm_device *dev,
>  	struct drm_crtc *for_crtc = cstate->base.crtc;
>  	unsigned int pipe_size, ddb_size;
>  	int nth_active_pipe;
> -	int pipe = to_intel_crtc(for_crtc)->pipe;
>  
>  	if (WARN_ON(!state) || !cstate->base.active) {
>  		alloc->start = 0;
> @@ -3086,7 +3085,7 @@ skl_ddb_get_pipe_allocation_limits(struct
> drm_device *dev,
>  	 * we currently hold.
>  	 */
>  	if (!intel_state->active_pipe_changes) {
> -		*alloc = dev_priv->wm.skl_hw.ddb.pipe[pipe];
> +		*alloc = to_intel_crtc(for_crtc)->hw_ddb;
>  		return;
>  	}
>  
> @@ -3354,7 +3353,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	struct drm_plane *plane;
>  	struct drm_plane_state *pstate;
>  	enum pipe pipe = intel_crtc->pipe;
> -	struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
> +	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
>  	uint16_t alloc_size, start, cursor_blocks;
>  	uint16_t *minimum = cstate->wm.skl.minimum_blocks;
>  	uint16_t *y_minimum = cstate->wm.skl.minimum_y_blocks;
> @@ -3366,7 +3365,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  		return 0;
>  
>  	if (!cstate->base.active) {
> -		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
> +		memset(alloc, 0, sizeof(*alloc));

Nitpick: I would have kept the double assignment here instead of
calling memset:
alloc->start = alloc->end = 0;

Anyway, this chunk will have a small conflict with my current multi-
pipe fix, and I'd like to see the multi-pipe fix get merged before this
since it will make it much easier to backport it to stable if we don't
have yet another round of code churn separating stable from dinq.

Anyway, this patch looks correct and and it seems the multi-pipe fix
won't require any change besides the small rebase above. I'm still
going to test the series later, but I suppose a rebased version of this
patch will be ready to get a reviewed-by tag from me.


>  		memset(ddb->plane[pipe], 0, sizeof(ddb-
> >plane[pipe]));
>  		memset(ddb->y_plane[pipe], 0, sizeof(ddb-
> >y_plane[pipe]));
>  		return 0;
> @@ -3895,14 +3894,6 @@ void skl_write_cursor_wm(struct intel_crtc
> *intel_crtc,
>  			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
>  }
>  
> -bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
> -			       const struct skl_ddb_allocation *new,
> -			       enum pipe pipe)
> -{
> -	return new->pipe[pipe].start == old->pipe[pipe].start &&
> -	       new->pipe[pipe].end == old->pipe[pipe].end;
> -}
> -
>  static inline bool skl_ddb_entries_overlap(const struct
> skl_ddb_entry *a,
>  					   const struct
> skl_ddb_entry *b)
>  {
> @@ -3910,22 +3901,22 @@ static inline bool
> skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
>  }
>  
>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> -				 const struct skl_ddb_allocation
> *old,
> -				 const struct skl_ddb_allocation
> *new,
> -				 enum pipe pipe)
> +				 struct intel_crtc *intel_crtc)
>  {
> -	struct drm_device *dev = state->dev;
> -	struct intel_crtc *intel_crtc;
> -	enum pipe otherp;
> +	struct drm_crtc *other_crtc;
> +	struct drm_crtc_state *other_cstate;
> +	struct intel_crtc *other_intel_crtc;
> +	const struct skl_ddb_entry *ddb =
> +		&to_intel_crtc_state(intel_crtc->base.state)-
> >wm.skl.ddb;
> +	int i;
>  
> -	for_each_intel_crtc(dev, intel_crtc) {
> -		otherp = intel_crtc->pipe;
> +	for_each_crtc_in_state(state, other_crtc, other_cstate, i) {
> +		other_intel_crtc = to_intel_crtc(other_crtc);
>  
> -		if (otherp == pipe)
> +		if (other_intel_crtc == intel_crtc)
>  			continue;
>  
> -		if (skl_ddb_entries_overlap(&new->pipe[pipe],
> -					    &old->pipe[otherp]))
> +		if (skl_ddb_entries_overlap(ddb, &other_intel_crtc-
> >hw_ddb))
>  			return true;
>  	}
>  
> @@ -4084,7 +4075,6 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst,
>  	memcpy(dst->plane_trans[pipe], src->plane_trans[pipe],
>  	       sizeof(dst->plane_trans[pipe]));
>  
> -	dst->ddb.pipe[pipe] = src->ddb.pipe[pipe];
>  	memcpy(dst->ddb.y_plane[pipe], src->ddb.y_plane[pipe],
>  	       sizeof(dst->ddb.y_plane[pipe]));
>  	memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe],
> @@ -4192,6 +4182,8 @@ static void skl_update_wm(struct drm_crtc
> *crtc)
>  
>  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
>  
> +	intel_crtc->hw_ddb = cstate->wm.skl.ddb;
> +
>  	mutex_unlock(&dev_priv->wm.wm_mutex);
>  }
>  

Re: [Intel-gfx] [PATCH 2/6] drm/i915/skl: Remove linetime from skl_wm_values

[Paulo Zanoni]

Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
> Next part of cleaning up the watermark code for skl. This is easy,
> since
> it seems that we never actually needed to keep track of the linetime
> in
> the skl_wm_values struct anyway.

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

> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      | 1 -
>  drivers/gpu/drm/i915/intel_display.c | 6 ++++--
>  drivers/gpu/drm/i915/intel_pm.c      | 7 +------
>  3 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 85e541c..d26e5999 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1643,7 +1643,6 @@ struct skl_ddb_allocation {
>  struct skl_wm_values {
>  	unsigned dirty_pipes;
>  	struct skl_ddb_allocation ddb;
> -	uint32_t wm_linetime[I915_MAX_PIPES];
>  	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
>  	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
>  };
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 17733af..a71d05a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14832,6 +14832,8 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc_state *intel_cstate =
> +		to_intel_crtc_state(crtc->state);
>  	struct intel_crtc_state *old_intel_state =
>  		to_intel_crtc_state(old_crtc_state);
>  	bool modeset = needs_modeset(crtc->state);
> @@ -14848,13 +14850,13 @@ static void intel_begin_crtc_commit(struct
> drm_crtc *crtc,
>  		intel_color_load_luts(crtc->state);
>  	}
>  
> -	if (to_intel_crtc_state(crtc->state)->update_pipe)
> +	if (intel_cstate->update_pipe)
>  		intel_update_pipe_config(intel_crtc,
> old_intel_state);
>  	else if (INTEL_GEN(dev_priv) >= 9) {
>  		skl_detach_scalers(intel_crtc);
>  
>  		I915_WRITE(PIPE_WM_LINETIME(pipe),
> -			   dev_priv->wm.skl_hw.wm_linetime[pipe]);
> +			   intel_cstate->wm.skl.optimal.linetime);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 0383516..af96888 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3839,8 +3839,6 @@ static void skl_compute_wm_results(struct
> drm_device *dev,
>  		temp |= PLANE_WM_EN;
>  
>  	r->plane_trans[pipe][PLANE_CURSOR] = temp;
> -
> -	r->wm_linetime[pipe] = p_wm->linetime;
>  }
>  
>  static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
> @@ -4069,7 +4067,6 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst,
>  		     struct skl_wm_values *src,
>  		     enum pipe pipe)
>  {
> -	dst->wm_linetime[pipe] = src->wm_linetime[pipe];
>  	memcpy(dst->plane[pipe], src->plane[pipe],
>  	       sizeof(dst->plane[pipe]));
>  	memcpy(dst->plane_trans[pipe], src->plane_trans[pipe],
> @@ -4320,8 +4317,6 @@ static void skl_pipe_wm_get_hw_state(struct
> drm_crtc *crtc)
>  
>  	max_level = ilk_wm_max_level(dev);
>  
> -	hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
> -
>  	for (level = 0; level <= max_level; level++) {
>  		for (i = 0; i < intel_num_planes(intel_crtc); i++)
>  			hw->plane[pipe][i][level] =
> @@ -4338,7 +4333,7 @@ static void skl_pipe_wm_get_hw_state(struct
> drm_crtc *crtc)
>  
>  	hw->dirty_pipes |= drm_crtc_mask(crtc);
>  
> -	active->linetime = hw->wm_linetime[pipe];
> +	active->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
>  
>  	for (level = 0; level <= max_level; level++) {
>  		for (i = 0; i < intel_num_planes(intel_crtc); i++) {

Re: [Intel-gfx] [PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane

[Paulo Zanoni]

Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
> Having skl_wm_level contain all of the watermarks for each plane is
> annoying since it prevents us from having any sort of object to
> represent a single watermark level, something we take advantage of in
> the next commit to cut down on all of the copy paste code in here.

I'd like to start my review pointing that I really like this patch. I
agree the current form is annoying.

See below for some details.


> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |   6 +-
>  drivers/gpu/drm/i915/intel_drv.h |   6 +-
>  drivers/gpu/drm/i915/intel_pm.c  | 208 +++++++++++++++++----------
> ------------
>  3 files changed, 100 insertions(+), 120 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index d26e5999..0f97d43 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1648,9 +1648,9 @@ struct skl_wm_values {
>  };
>  
>  struct skl_wm_level {
> -	bool plane_en[I915_MAX_PLANES];
> -	uint16_t plane_res_b[I915_MAX_PLANES];
> -	uint8_t plane_res_l[I915_MAX_PLANES];
> +	bool plane_en;
> +	uint16_t plane_res_b;
> +	uint8_t plane_res_l;
>  };
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 35ba282..d684f4f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -468,9 +468,13 @@ struct intel_pipe_wm {
>  	bool sprites_scaled;
>  };
>  
> -struct skl_pipe_wm {
> +struct skl_plane_wm {
>  	struct skl_wm_level wm[8];
>  	struct skl_wm_level trans_wm;
> +};
> +
> +struct skl_pipe_wm {
> +	struct skl_plane_wm planes[I915_MAX_PLANES];
>  	uint32_t linetime;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index af96888..250f12d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3668,67 +3668,52 @@ static int
>  skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>  		     struct skl_ddb_allocation *ddb,
>  		     struct intel_crtc_state *cstate,
> +		     struct intel_plane *intel_plane,
>  		     int level,
>  		     struct skl_wm_level *result)
>  {
>  	struct drm_atomic_state *state = cstate->base.state;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> >base.crtc);
> -	struct drm_plane *plane;
> -	struct intel_plane *intel_plane;
> -	struct intel_plane_state *intel_pstate;
> +	struct drm_plane *plane = &intel_plane->base;
> +	struct intel_plane_state *intel_pstate = NULL;
>  	uint16_t ddb_blocks;
>  	enum pipe pipe = intel_crtc->pipe;
>  	int ret;
> +	int i = skl_wm_plane_id(intel_plane);
> +
> +	if (state)
> +		intel_pstate =
> +			intel_atomic_get_existing_plane_state(state,
> +							      intel_
> plane);
>  
>  	/*
> -	 * We'll only calculate watermarks for planes that are
> actually
> -	 * enabled, so make sure all other planes are set as
> disabled.
> +	 * Note: If we start supporting multiple pending atomic
> commits against
> +	 * the same planes/CRTC's in the future, plane->state will
> no longer be
> +	 * the correct pre-state to use for the calculations here
> and we'll
> +	 * need to change where we get the 'unchanged' plane data
> from.
> +	 *
> +	 * For now this is fine because we only allow one queued
> commit against
> +	 * a CRTC.  Even if the plane isn't modified by this
> transaction and we
> +	 * don't have a plane lock, we still have the CRTC's lock,
> so we know
> +	 * that no other transactions are racing with us to update
> it.
>  	 */
> -	memset(result, 0, sizeof(*result));
> -
> -	for_each_intel_plane_mask(&dev_priv->drm,
> -				  intel_plane,
> -				  cstate->base.plane_mask) {
> -		int i = skl_wm_plane_id(intel_plane);
> -
> -		plane = &intel_plane->base;
> -		intel_pstate = NULL;
> -		if (state)
> -			intel_pstate =
> -				intel_atomic_get_existing_plane_stat
> e(state,
> -								    
>   intel_plane);
> +	if (!intel_pstate)
> +		intel_pstate = to_intel_plane_state(plane->state);
>  
> -		/*
> -		 * Note: If we start supporting multiple pending
> atomic commits
> -		 * against the same planes/CRTC's in the future,
> plane->state
> -		 * will no longer be the correct pre-state to use
> for the
> -		 * calculations here and we'll need to change where
> we get the
> -		 * 'unchanged' plane data from.
> -		 *
> -		 * For now this is fine because we only allow one
> queued commit
> -		 * against a CRTC.  Even if the plane isn't modified
> by this
> -		 * transaction and we don't have a plane lock, we
> still have
> -		 * the CRTC's lock, so we know that no other
> transactions are
> -		 * racing with us to update it.
> -		 */
> -		if (!intel_pstate)
> -			intel_pstate = to_intel_plane_state(plane-
> >state);
> +	WARN_ON(!intel_pstate->base.fb);
>  
> -		WARN_ON(!intel_pstate->base.fb);
> +	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
>  
> -		ddb_blocks = skl_ddb_entry_size(&ddb-
> >plane[pipe][i]);
> -
> -		ret = skl_compute_plane_wm(dev_priv,
> -					   cstate,
> -					   intel_pstate,
> -					   ddb_blocks,
> -					   level,
> -					   &result->plane_res_b[i],
> -					   &result->plane_res_l[i],
> -					   &result->plane_en[i]);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = skl_compute_plane_wm(dev_priv,
> +				   cstate,
> +				   intel_pstate,
> +				   ddb_blocks,
> +				   level,
> +				   &result->plane_res_b,
> +				   &result->plane_res_l,
> +				   &result->plane_en);
> +	if (ret)
> +		return ret;
>  
>  	return 0;
>  }
> @@ -3749,19 +3734,11 @@ skl_compute_linetime_wm(struct
> intel_crtc_state *cstate)
>  static void skl_compute_transition_wm(struct intel_crtc_state
> *cstate,
>  				      struct skl_wm_level *trans_wm
> /* out */)
>  {
> -	struct drm_crtc *crtc = cstate->base.crtc;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane;
> -
>  	if (!cstate->base.active)
>  		return;
>  
>  	/* Until we know more, just disable transition WMs */
> -	for_each_intel_plane_on_crtc(crtc->dev, intel_crtc,
> intel_plane) {
> -		int i = skl_wm_plane_id(intel_plane);
> -
> -		trans_wm->plane_en[i] = false;
> -	}
> +	trans_wm->plane_en = false;
>  }
>  
>  static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> @@ -3770,19 +3747,33 @@ static int skl_build_pipe_wm(struct
> intel_crtc_state *cstate,
>  {
>  	struct drm_device *dev = cstate->base.crtc->dev;
>  	const struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_plane *intel_plane;
> +	struct skl_plane_wm *wm;
>  	int level, max_level = ilk_wm_max_level(dev);
>  	int ret;
>  
> -	for (level = 0; level <= max_level; level++) {
> -		ret = skl_compute_wm_level(dev_priv, ddb, cstate,
> -					   level, &pipe_wm-
> >wm[level]);
> -		if (ret)
> -			return ret;
> +	/*
> +	 * We'll only calculate watermarks for planes that are
> actually
> +	 * enabled, so make sure all other planes are set as
> disabled.
> +	 */
> +	memset(pipe_wm->planes, 0, sizeof(pipe_wm->planes));
> +
> +	for_each_intel_plane_mask(&dev_priv->drm,
> +				  intel_plane,
> +				  cstate->base.plane_mask) {
> +		wm = &pipe_wm->planes[skl_wm_plane_id(intel_plane)];
> +
> +		for (level = 0; level <= max_level; level++) {
> +			ret = skl_compute_wm_level(dev_priv, ddb,
> cstate,
> +						   intel_plane,
> level,
> +						   &wm->wm[level]);
> +			if (ret)
> +				return ret;
> +		}
> +		skl_compute_transition_wm(cstate, &wm->trans_wm);
>  	}
>  	pipe_wm->linetime = skl_compute_linetime_wm(cstate);
>  
> -	skl_compute_transition_wm(cstate, &pipe_wm->trans_wm);
> -
>  	return 0;
>  }
>  
> @@ -3792,50 +3783,56 @@ static void skl_compute_wm_results(struct
> drm_device *dev,
>  				   struct intel_crtc *intel_crtc)
>  {
>  	int level, max_level = ilk_wm_max_level(dev);
> +	struct skl_plane_wm *plane_wm;
>  	enum pipe pipe = intel_crtc->pipe;
>  	uint32_t temp;
>  	int i;
>  
> -	for (level = 0; level <= max_level; level++) {
> -		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> +	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> +		plane_wm = &p_wm->planes[i];
> +
> +		for (level = 0; level <= max_level; level++) {
>  			temp = 0;
>  
> -			temp |= p_wm->wm[level].plane_res_l[i] <<
> +			temp |= plane_wm->wm[level].plane_res_l <<
>  					PLANE_WM_LINES_SHIFT;
> -			temp |= p_wm->wm[level].plane_res_b[i];
> -			if (p_wm->wm[level].plane_en[i])
> +			temp |= plane_wm->wm[level].plane_res_b;
> +			if (plane_wm->wm[level].plane_en)
>  				temp |= PLANE_WM_EN;
>  
>  			r->plane[pipe][i][level] = temp;
>  		}
>  
> -		temp = 0;
> -
> -		temp |= p_wm->wm[level].plane_res_l[PLANE_CURSOR] <<
> PLANE_WM_LINES_SHIFT;
> -		temp |= p_wm->wm[level].plane_res_b[PLANE_CURSOR];
> +	}
>  
> -		if (p_wm->wm[level].plane_en[PLANE_CURSOR])
> +	for (level = 0; level <= max_level; level++) {
> +		plane_wm = &p_wm->planes[PLANE_CURSOR];
> +		temp = 0;
> +		temp |= plane_wm->wm[level].plane_res_l <<
> PLANE_WM_LINES_SHIFT;
> +		temp |= plane_wm->wm[level].plane_res_b;
> +		if (plane_wm->wm[level].plane_en)
>  			temp |= PLANE_WM_EN;
>  
>  		r->plane[pipe][PLANE_CURSOR][level] = temp;
> -
>  	}
>  
>  	/* transition WMs */
>  	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> +		plane_wm = &p_wm->planes[i];
>  		temp = 0;
> -		temp |= p_wm->trans_wm.plane_res_l[i] <<
> PLANE_WM_LINES_SHIFT;
> -		temp |= p_wm->trans_wm.plane_res_b[i];
> -		if (p_wm->trans_wm.plane_en[i])
> +		temp |= plane_wm->trans_wm.plane_res_l <<
> PLANE_WM_LINES_SHIFT;
> +		temp |= plane_wm->trans_wm.plane_res_b;
> +		if (plane_wm->trans_wm.plane_en)
>  			temp |= PLANE_WM_EN;
>  
>  		r->plane_trans[pipe][i] = temp;
>  	}
>  
> +	plane_wm = &p_wm->planes[PLANE_CURSOR];
>  	temp = 0;
> -	temp |= p_wm->trans_wm.plane_res_l[PLANE_CURSOR] <<
> PLANE_WM_LINES_SHIFT;
> -	temp |= p_wm->trans_wm.plane_res_b[PLANE_CURSOR];
> -	if (p_wm->trans_wm.plane_en[PLANE_CURSOR])
> +	temp |= plane_wm->trans_wm.plane_res_l <<
> PLANE_WM_LINES_SHIFT;
> +	temp |= plane_wm->trans_wm.plane_res_b;
> +	if (plane_wm->trans_wm.plane_en)
>  		temp |= PLANE_WM_EN;
>  
>  	r->plane_trans[pipe][PLANE_CURSOR] = temp;
> @@ -4262,44 +4259,24 @@ static void ilk_optimize_watermarks(struct
> intel_crtc_state *cstate)
>  static void skl_pipe_wm_active_state(uint32_t val,
>  				     struct skl_pipe_wm *active,
>  				     bool is_transwm,
> -				     bool is_cursor,
>  				     int i,
>  				     int level)
>  {
> +	struct skl_plane_wm *plane_wm = &active->planes[i];
>  	bool is_enabled = (val & PLANE_WM_EN) != 0;
>  
>  	if (!is_transwm) {
> -		if (!is_cursor) {
> -			active->wm[level].plane_en[i] = is_enabled;
> -			active->wm[level].plane_res_b[i] =
> -					val & PLANE_WM_BLOCKS_MASK;
> -			active->wm[level].plane_res_l[i] =
> -					(val >>
> PLANE_WM_LINES_SHIFT) &
> -						PLANE_WM_LINES_MASK;
> -		} else {
> -			active->wm[level].plane_en[PLANE_CURSOR] =
> is_enabled;
> -			active->wm[level].plane_res_b[PLANE_CURSOR]
> =
> -					val & PLANE_WM_BLOCKS_MASK;
> -			active->wm[level].plane_res_l[PLANE_CURSOR]
> =
> -					(val >>
> PLANE_WM_LINES_SHIFT) &
> -						PLANE_WM_LINES_MASK;
> -		}
> +		plane_wm->wm[level].plane_en = is_enabled;
> +		plane_wm->wm[level].plane_res_b = val &
> PLANE_WM_BLOCKS_MASK;
> +		plane_wm->wm[level].plane_res_l =
> +			(val >> PLANE_WM_LINES_SHIFT) &
> +			PLANE_WM_LINES_MASK;

Nitpick: you can join the two lines above and still stay under 80
columns.


>  	} else {
> -		if (!is_cursor) {
> -			active->trans_wm.plane_en[i] = is_enabled;
> -			active->trans_wm.plane_res_b[i] =
> -					val & PLANE_WM_BLOCKS_MASK;
> -			active->trans_wm.plane_res_l[i] =
> -					(val >>
> PLANE_WM_LINES_SHIFT) &
> -						PLANE_WM_LINES_MASK;
> -		} else {
> -			active->trans_wm.plane_en[PLANE_CURSOR] =
> is_enabled;
> -			active->trans_wm.plane_res_b[PLANE_CURSOR] =
> -					val & PLANE_WM_BLOCKS_MASK;
> -			active->trans_wm.plane_res_l[PLANE_CURSOR] =
> -					(val >>
> PLANE_WM_LINES_SHIFT) &
> -						PLANE_WM_LINES_MASK;
> -		}
> +		plane_wm->trans_wm.plane_en = is_enabled;
> +		plane_wm->trans_wm.plane_res_b = val &
> PLANE_WM_BLOCKS_MASK;
> +		plane_wm->trans_wm.plane_res_l =
> +			(val >> PLANE_WM_LINES_SHIFT) &
> +			PLANE_WM_LINES_MASK;

Same here.


>  	}
>  }
>  
> @@ -4338,20 +4315,19 @@ static void skl_pipe_wm_get_hw_state(struct
> drm_crtc *crtc)
>  	for (level = 0; level <= max_level; level++) {
>  		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>  			temp = hw->plane[pipe][i][level];
> -			skl_pipe_wm_active_state(temp, active,
> false,
> -						false, i, level);
> +			skl_pipe_wm_active_state(temp, active,
> false, i, level);
>  		}
>  		temp = hw->plane[pipe][PLANE_CURSOR][level];
> -		skl_pipe_wm_active_state(temp, active, false, true,
> i, level);
> +		skl_pipe_wm_active_state(temp, active, false, i,
> level);

While this is not wrong today, history shows that the number of planes
increases over time, so we may at some point in the future add PLANE_D
and more, so the code will become wrong. Just pass PLANE_CURSOR instead
of "i" here and below. Also, this simplification could have been a
separate patch.

Everything else looks correct, so if you fix the detail above I'll
provide a r-b tag.


>  	}
>  
>  	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>  		temp = hw->plane_trans[pipe][i];
> -		skl_pipe_wm_active_state(temp, active, true, false,
> i, 0);
> +		skl_pipe_wm_active_state(temp, active, true, i, 0);
>  	}
>  
>  	temp = hw->plane_trans[pipe][PLANE_CURSOR];
> -	skl_pipe_wm_active_state(temp, active, true, true, i, 0);
> +	skl_pipe_wm_active_state(temp, active, true, i, 0);
>  
>  	intel_crtc->wm.active.skl = *active;
>  }

Re: [Intel-gfx] [PATCH 5/6] drm/i915/gen9: Get rid of redundant watermark values

[Paulo Zanoni]

Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
> Now that we've make skl_wm_levels make a little more sense, we can
> remove all of the redundant wm information. Up until now we'd been
> storing two copies of all of the skl watermarks: one being the
> skl_pipe_wm structs, the other being the global wm struct in
> drm_i915_private containing the raw register values. This is
> confusing
> and problematic, since it means we're prone to accidentally letting
> the
> two copies go out of sync. So, get rid of all of the functions
> responsible for computing the register values and just use a single
> helper, skl_write_wm_level(), to convert and write the new watermarks
> on
> the fly.

I like the direction of this patch too, but there are some small
possible problems. See below.


> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   2 -
>  drivers/gpu/drm/i915/intel_display.c |  14 ++-
>  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 203 ++++++++++++-------------
> ----------
>  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
>  5 files changed, 88 insertions(+), 145 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 0f97d43..63519ac 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1643,8 +1643,6 @@ struct skl_ddb_allocation {
>  struct skl_wm_values {
>  	unsigned dirty_pipes;
>  	struct skl_ddb_allocation ddb;
> -	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> -	uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
>  };
>  
>  struct skl_wm_level {
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index dd15ae2..c580d3d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3378,6 +3378,8 @@ static void skylake_update_primary_plane(struct
> drm_plane *plane,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> >base.crtc);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
>  	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> +	const struct skl_plane_wm *p_wm =
> +		&crtc_state->wm.skl.optimal.planes[0];

I wish someone would do a patch to convert all these hardcoded values
to PLANE_X, and convert "int"s  to "enum plane"s everywhere.


>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl;
>  	unsigned int rotation = plane_state->base.rotation;
> @@ -3414,7 +3416,7 @@ static void skylake_update_primary_plane(struct
> drm_plane *plane,
>  	intel_crtc->adjusted_y = src_y;
>  
>  	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> -		skl_write_plane_wm(intel_crtc, wm, 0);
> +		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0);
>  
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
> @@ -3448,6 +3450,8 @@ static void
> skylake_disable_primary_plane(struct drm_plane *primary,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> +	const struct skl_plane_wm *p_wm = &cstate-
> >wm.skl.optimal.planes[0];
>  	int pipe = intel_crtc->pipe;
>  
>  	/*
> @@ -3455,7 +3459,8 @@ static void
> skylake_disable_primary_plane(struct drm_plane *primary,
>  	 * plane's visiblity isn't actually changing neither is its
> watermarks.
>  	 */
>  	if (!crtc->primary->state->visible)
> -		skl_write_plane_wm(intel_crtc, &dev_priv-
> >wm.skl_results, 0);
> +		skl_write_plane_wm(intel_crtc, p_wm,
> +				   &dev_priv->wm.skl_results.ddb,
> 0);
>  
>  	I915_WRITE(PLANE_CTL(pipe, 0), 0);
>  	I915_WRITE(PLANE_SURF(pipe, 0), 0);
> @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct
> drm_crtc *crtc, u32 base,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
>  	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> +	const struct skl_plane_wm *p_wm =
> +		&cstate->wm.skl.optimal.planes[PLANE_CURSOR];
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
>  	if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> drm_crtc_mask(crtc))
> -		skl_write_cursor_wm(intel_crtc, wm);
> +		skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb);
>  
>  	if (plane_state && plane_state->base.visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index d684f4f..958dc72 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const struct
> skl_ddb_allocation *old,
>  bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
>  				 struct intel_crtc *intel_crtc);
>  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> -			 const struct skl_wm_values *wm);
> +			 const struct skl_plane_wm *wm,
> +			 const struct skl_ddb_allocation *ddb);
>  void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> -			const struct skl_wm_values *wm,
> +			const struct skl_plane_wm *wm,
> +			const struct skl_ddb_allocation *ddb,
>  			int plane);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 250f12d..7708646 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3000,6 +3000,8 @@ bool intel_can_enable_sagv(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 drm_crtc *crtc;
> +	struct intel_crtc_state *cstate;
> +	struct skl_plane_wm *wm;
>  	enum pipe pipe;
>  	int level, plane;
>  
> @@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct
> drm_atomic_state *state)
>  	/* Since we're now guaranteed to only have one active
> CRTC... */
>  	pipe = ffs(intel_state->active_crtcs) - 1;
>  	crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +	cstate = intel_atomic_get_crtc_state(state,
> to_intel_crtc(crtc));
>  
>  	if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE)
>  		return false;
>  
>  	for_each_plane(dev_priv, pipe, plane) {
> +		wm = &cstate->wm.skl.optimal.planes[plane];
> +
>  		/* Skip this plane if it's not enabled */
> -		if (intel_state->wm_results.plane[pipe][plane][0] ==
> 0)
> +		if (!wm->wm[0].plane_en)
>  			continue;
>  
>  		/* Find the highest enabled wm level for this plane
> */
> -		for (level = ilk_wm_max_level(dev);
> -		     intel_state-
> >wm_results.plane[pipe][plane][level] == 0; --level)
> +		for (level = ilk_wm_max_level(dev); !wm-
> >wm[level].plane_en;
> +		     --level)
>  		     { }
>  
>  		/*
> @@ -3777,67 +3782,6 @@ static int skl_build_pipe_wm(struct
> intel_crtc_state *cstate,
>  	return 0;
>  }
>  
> -static void skl_compute_wm_results(struct drm_device *dev,
> -				   struct skl_pipe_wm *p_wm,
> -				   struct skl_wm_values *r,
> -				   struct intel_crtc *intel_crtc)
> -{
> -	int level, max_level = ilk_wm_max_level(dev);
> -	struct skl_plane_wm *plane_wm;
> -	enum pipe pipe = intel_crtc->pipe;
> -	uint32_t temp;
> -	int i;
> -
> -	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> -		plane_wm = &p_wm->planes[i];
> -
> -		for (level = 0; level <= max_level; level++) {
> -			temp = 0;
> -
> -			temp |= plane_wm->wm[level].plane_res_l <<
> -					PLANE_WM_LINES_SHIFT;
> -			temp |= plane_wm->wm[level].plane_res_b;
> -			if (plane_wm->wm[level].plane_en)
> -				temp |= PLANE_WM_EN;
> -
> -			r->plane[pipe][i][level] = temp;
> -		}
> -
> -	}
> -
> -	for (level = 0; level <= max_level; level++) {
> -		plane_wm = &p_wm->planes[PLANE_CURSOR];
> -		temp = 0;
> -		temp |= plane_wm->wm[level].plane_res_l <<
> PLANE_WM_LINES_SHIFT;
> -		temp |= plane_wm->wm[level].plane_res_b;
> -		if (plane_wm->wm[level].plane_en)
> -			temp |= PLANE_WM_EN;
> -
> -		r->plane[pipe][PLANE_CURSOR][level] = temp;
> -	}
> -
> -	/* transition WMs */
> -	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> -		plane_wm = &p_wm->planes[i];
> -		temp = 0;
> -		temp |= plane_wm->trans_wm.plane_res_l <<
> PLANE_WM_LINES_SHIFT;
> -		temp |= plane_wm->trans_wm.plane_res_b;
> -		if (plane_wm->trans_wm.plane_en)
> -			temp |= PLANE_WM_EN;
> -
> -		r->plane_trans[pipe][i] = temp;
> -	}
> -
> -	plane_wm = &p_wm->planes[PLANE_CURSOR];
> -	temp = 0;
> -	temp |= plane_wm->trans_wm.plane_res_l <<
> PLANE_WM_LINES_SHIFT;
> -	temp |= plane_wm->trans_wm.plane_res_b;
> -	if (plane_wm->trans_wm.plane_en)
> -		temp |= PLANE_WM_EN;
> -
> -	r->plane_trans[pipe][PLANE_CURSOR] = temp;
> -}
> -
>  static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
>  				i915_reg_t reg,
>  				const struct skl_ddb_entry *entry)
> @@ -3848,8 +3792,22 @@ static void skl_ddb_entry_write(struct
> drm_i915_private *dev_priv,
>  		I915_WRITE(reg, 0);
>  }
>  
> +static void skl_write_wm_level(struct drm_i915_private *dev_priv,
> +			       i915_reg_t reg,
> +			       const struct skl_wm_level *level)
> +{
> +	uint32_t val = 0;
> +
> +	val |= level->plane_res_b;
> +	val |= level->plane_res_l << PLANE_WM_LINES_SHIFT;
> +	val |= level->plane_en;

The line above seems wrong, you should check for plane_en and then set
PLANE_WM_EN.

IMHO it would even better if we completely zeroed the register in case
plane_en is false, so we could have:

uint32_t val = 0;

if (level->plane_en) {
	val |= PLANE_WM_EN;
	val |= level->plane_res_b;
	val |= level->plane_res_l << PLANE_WM_LINES_SHIFT;
}


> +
> +	I915_WRITE(reg, val);
> +}
> +
>  void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> -			const struct skl_wm_values *wm,
> +			const struct skl_plane_wm *wm,
> +			const struct skl_ddb_allocation *ddb,
>  			int plane)
>  {
>  	struct drm_crtc *crtc = &intel_crtc->base;
> @@ -3859,19 +3817,21 @@ void skl_write_plane_wm(struct intel_crtc
> *intel_crtc,
>  	enum pipe pipe = intel_crtc->pipe;
>  
>  	for (level = 0; level <= max_level; level++) {
> -		I915_WRITE(PLANE_WM(pipe, plane, level),
> -			   wm->plane[pipe][plane][level]);
> +		skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane,
> level),
> +				   &wm->wm[level]);
>  	}
> -	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm-
> >plane_trans[pipe][plane]);
> +	skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane),
> +			   &wm->trans_wm);
>  
>  	skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane),
> -			    &wm->ddb.plane[pipe][plane]);
> +			    &ddb->plane[pipe][plane]);
>  	skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe,
> plane),
> -			    &wm->ddb.y_plane[pipe][plane]);
> +			    &ddb->y_plane[pipe][plane]);
>  }
>  
>  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> -			 const struct skl_wm_values *wm)
> +			 const struct skl_plane_wm *wm,
> +			 const struct skl_ddb_allocation *ddb)
>  {
>  	struct drm_crtc *crtc = &intel_crtc->base;
>  	struct drm_device *dev = crtc->dev;
> @@ -3880,13 +3840,13 @@ void skl_write_cursor_wm(struct intel_crtc
> *intel_crtc,
>  	enum pipe pipe = intel_crtc->pipe;
>  
>  	for (level = 0; level <= max_level; level++) {
> -		I915_WRITE(CUR_WM(pipe, level),
> -			   wm->plane[pipe][PLANE_CURSOR][level]);
> +		skl_write_wm_level(dev_priv, CUR_WM(pipe, level),
> +				   &wm->wm[level]);
>  	}
> -	I915_WRITE(CUR_WM_TRANS(pipe), wm-
> >plane_trans[pipe][PLANE_CURSOR]);
> +	skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm-
> >trans_wm);
>  
>  	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> -			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
> +			    &ddb->plane[pipe][PLANE_CURSOR]);
>  }
>  
>  static inline bool skl_ddb_entries_overlap(const struct
> skl_ddb_entry *a,
> @@ -4064,11 +4024,6 @@ skl_copy_wm_for_pipe(struct skl_wm_values
> *dst,
>  		     struct skl_wm_values *src,
>  		     enum pipe pipe)
>  {
> -	memcpy(dst->plane[pipe], src->plane[pipe],
> -	       sizeof(dst->plane[pipe]));
> -	memcpy(dst->plane_trans[pipe], src->plane_trans[pipe],
> -	       sizeof(dst->plane_trans[pipe]));
> -
>  	memcpy(dst->ddb.y_plane[pipe], src->ddb.y_plane[pipe],
>  	       sizeof(dst->ddb.y_plane[pipe]));
>  	memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe],
> @@ -4117,7 +4072,6 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	 * 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);
>  
> @@ -4135,7 +4089,6 @@ skl_compute_wm(struct drm_atomic_state *state)
>  			continue;
>  
>  		intel_cstate->update_wm_pre = true;
> -		skl_compute_wm_results(crtc->dev, pipe_wm, results,
> intel_crtc);
>  	}
>  
>  	return 0;
> @@ -4169,9 +4122,11 @@ static void skl_update_wm(struct drm_crtc
> *crtc)
>  		int plane;
>  
>  		for (plane = 0; plane <
> intel_num_planes(intel_crtc); plane++)
> -			skl_write_plane_wm(intel_crtc, results,
> plane);
> +			skl_write_plane_wm(intel_crtc, &pipe_wm-
> >planes[plane],
> +					   &results->ddb, plane);
>  
> -		skl_write_cursor_wm(intel_crtc, results);
> +		skl_write_cursor_wm(intel_crtc, &pipe_wm-
> >planes[PLANE_CURSOR],
> +				    &results->ddb);
>  	}
>  
>  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
> @@ -4256,28 +4211,13 @@ static void ilk_optimize_watermarks(struct
> intel_crtc_state *cstate)
>  	mutex_unlock(&dev_priv->wm.wm_mutex);
>  }
>  
> -static void skl_pipe_wm_active_state(uint32_t val,
> -				     struct skl_pipe_wm *active,
> -				     bool is_transwm,
> -				     int i,
> -				     int level)
> +static inline void skl_wm_level_from_reg_val(uint32_t val,
> +					     struct skl_wm_level
> *level)
>  {
> -	struct skl_plane_wm *plane_wm = &active->planes[i];
> -	bool is_enabled = (val & PLANE_WM_EN) != 0;
> -
> -	if (!is_transwm) {
> -		plane_wm->wm[level].plane_en = is_enabled;
> -		plane_wm->wm[level].plane_res_b = val &
> PLANE_WM_BLOCKS_MASK;
> -		plane_wm->wm[level].plane_res_l =
> -			(val >> PLANE_WM_LINES_SHIFT) &
> -			PLANE_WM_LINES_MASK;
> -	} else {
> -		plane_wm->trans_wm.plane_en = is_enabled;
> -		plane_wm->trans_wm.plane_res_b = val &
> PLANE_WM_BLOCKS_MASK;
> -		plane_wm->trans_wm.plane_res_l =
> -			(val >> PLANE_WM_LINES_SHIFT) &
> -			PLANE_WM_LINES_MASK;
> -	}
> +	level->plane_en = val & PLANE_WM_EN;
> +	level->plane_res_b = val & PLANE_WM_BLOCKS_MASK;
> +	level->plane_res_l = (val & PLANE_WM_LINES_MASK) >>
> +		PLANE_WM_LINES_SHIFT;

This also looks wrong, since PLANE_WM_LINES_MASK is 0x1f. We should do
like the original code did: shifting before masking.


>  }
>  
>  static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> @@ -4287,23 +4227,33 @@ static void skl_pipe_wm_get_hw_state(struct
> drm_crtc *crtc)
>  	struct skl_wm_values *hw = &dev_priv->wm.skl_hw;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
> +	struct intel_plane *intel_plane;
>  	struct skl_pipe_wm *active = &cstate->wm.skl.optimal;
> +	struct skl_plane_wm *wm;
>  	enum pipe pipe = intel_crtc->pipe;
> -	int level, i, max_level;
> -	uint32_t temp;
> +	int level, id, max_level = ilk_wm_max_level(dev);
> +	uint32_t val;
>  
> -	max_level = ilk_wm_max_level(dev);
> +	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> +		id = skl_wm_plane_id(intel_plane);
> +		wm = &cstate->wm.skl.optimal.planes[id];
>  
> -	for (level = 0; level <= max_level; level++) {
> -		for (i = 0; i < intel_num_planes(intel_crtc); i++)
> -			hw->plane[pipe][i][level] =
> -					I915_READ(PLANE_WM(pipe, i,
> level));
> -		hw->plane[pipe][PLANE_CURSOR][level] =
> I915_READ(CUR_WM(pipe, level));
> -	}
> +		for (level = 0; level <= max_level; level++) {
> +			if (id != PLANE_CURSOR)
> +				val = I915_READ(PLANE_WM(pipe, id,
> level));
> +			else
> +				val = I915_READ(CUR_WM(pipe,
> level));
> +
> +			skl_wm_level_from_reg_val(val, &wm-
> >wm[level]);
> +		}
>  
> -	for (i = 0; i < intel_num_planes(intel_crtc); i++)
> -		hw->plane_trans[pipe][i] =
> I915_READ(PLANE_WM_TRANS(pipe, i));
> -	hw->plane_trans[pipe][PLANE_CURSOR] =
> I915_READ(CUR_WM_TRANS(pipe));
> +		if (id != PLANE_CURSOR)
> +			val = I915_READ(PLANE_WM_TRANS(pipe, id));
> +		else
> +			val = I915_READ(CUR_WM_TRANS(pipe));
> +
> +		skl_wm_level_from_reg_val(val, &wm->trans_wm);
> +	}
>  
>  	if (!intel_crtc->active)
>  		return;
> @@ -4311,25 +4261,6 @@ static void skl_pipe_wm_get_hw_state(struct
> drm_crtc *crtc)
>  	hw->dirty_pipes |= drm_crtc_mask(crtc);
>  
>  	active->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
> -
> -	for (level = 0; level <= max_level; level++) {
> -		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> -			temp = hw->plane[pipe][i][level];
> -			skl_pipe_wm_active_state(temp, active,
> false, i, level);
> -		}
> -		temp = hw->plane[pipe][PLANE_CURSOR][level];
> -		skl_pipe_wm_active_state(temp, active, false, i,
> level);
> -	}
> -
> -	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
> -		temp = hw->plane_trans[pipe][i];
> -		skl_pipe_wm_active_state(temp, active, true, i, 0);
> -	}
> -
> -	temp = hw->plane_trans[pipe][PLANE_CURSOR];
> -	skl_pipe_wm_active_state(temp, active, true, i, 0);
> -
> -	intel_crtc->wm.active.skl = *active;

As far as I understood, we should still be setting intel_crtc-
>wm.active.skl. Shouldn't we? If not, why?


Now, due to the problems above, weren't you getting some WARNs about
the hw state not matching what it should? In case not, maybe you could
investigate why.


>  }
>  
>  void skl_wm_get_hw_state(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 73a521f..0fb775b 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -208,6 +208,8 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
> +	const struct skl_plane_wm *p_wm =
> +		&crtc_state->wm.skl.optimal.planes[plane];
>  	u32 plane_ctl;
>  	const struct drm_intel_sprite_colorkey *key = &plane_state-
> >ckey;
>  	u32 surf_addr = plane_state->main.offset;
> @@ -232,7 +234,7 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
>  	if (wm->dirty_pipes & drm_crtc_mask(crtc))
> -		skl_write_plane_wm(intel_crtc, wm, plane);
> +		skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb,
> plane);
>  
>  	if (key->flags) {
>  		I915_WRITE(PLANE_KEYVAL(pipe, plane), key-
> >min_value);
> @@ -289,6 +291,7 @@ skl_disable_plane(struct drm_plane *dplane,
> struct drm_crtc *crtc)
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> +	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-
> >state);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  
> @@ -298,7 +301,8 @@ skl_disable_plane(struct drm_plane *dplane,
> struct drm_crtc *crtc)
>  	 */
>  	if (!dplane->state->visible)
>  		skl_write_plane_wm(to_intel_crtc(crtc),
> -				   &dev_priv->wm.skl_results,
> plane);
> +				   &cstate-
> >wm.skl.optimal.planes[plane],
> +				   &dev_priv->wm.skl_results.ddb,
> plane);
>  
>  	I915_WRITE(PLANE_CTL(pipe, plane), 0);
>  

Re: [Intel-gfx] [PATCH 5/6] drm/i915/gen9: Get rid of redundant watermark values

[Chris Wilson]

On Wed, Oct 05, 2016 at 06:44:04PM -0300, Paulo Zanoni wrote:
> Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index dd15ae2..c580d3d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3378,6 +3378,8 @@ static void skylake_update_primary_plane(struct
> > drm_plane *plane,
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-
> > >base.crtc);
> >  	struct drm_framebuffer *fb = plane_state->base.fb;
> >  	const struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> > +	const struct skl_plane_wm *p_wm =
> > +		&crtc_state->wm.skl.optimal.planes[0];
> 
> I wish someone would do a patch to convert all these hardcoded values
> to PLANE_X, and convert "int"s  to "enum plane"s everywhere.

Note that this is not PLANE_A, but setting up a shorthand local for

	const struct skl_plane_wm *p_wm = crtc_state->wm.skl.optimal.planes;

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane

[Maarten Lankhorst]

Op 05-10-16 om 22:33 schreef Paulo Zanoni:
> Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu:
>> Having skl_wm_level contain all of the watermarks for each plane is
>> annoying since it prevents us from having any sort of object to
>> represent a single watermark level, something we take advantage of in
>> the next commit to cut down on all of the copy paste code in here.
> I'd like to start my review pointing that I really like this patch. I
> agree the current form is annoying.
>
> See below for some details.
>
>
>> Signed-off-by: Lyude <cpaul@redhat.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |   6 +-
>>  drivers/gpu/drm/i915/intel_drv.h |   6 +-
>>  drivers/gpu/drm/i915/intel_pm.c  | 208 +++++++++++++++++----------
>> ------------
>>  3 files changed, 100 insertions(+), 120 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index d26e5999..0f97d43 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1648,9 +1648,9 @@ struct skl_wm_values {
>>  };
>>  
>>  struct skl_wm_level {
>> -	bool plane_en[I915_MAX_PLANES];
>> -	uint16_t plane_res_b[I915_MAX_PLANES];
>> -	uint8_t plane_res_l[I915_MAX_PLANES];
>> +	bool plane_en;
>> +	uint16_t plane_res_b;
>> +	uint8_t plane_res_l;
>>  };
>>  
>>  /*
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 35ba282..d684f4f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -468,9 +468,13 @@ struct intel_pipe_wm {
>>  	bool sprites_scaled;
>>  };
>>  
>> -struct skl_pipe_wm {
>> +struct skl_plane_wm {
>>  	struct skl_wm_level wm[8];
>>  	struct skl_wm_level trans_wm;
>> +};
>> +
>> +struct skl_pipe_wm {
>> +	struct skl_plane_wm planes[I915_MAX_PLANES];
>>  	uint32_t linetime;
>>  };
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index af96888..250f12d 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3668,67 +3668,52 @@ static int
>>  skl_compute_wm_level(const struct drm_i915_private *dev_priv,
>>  		     struct skl_ddb_allocation *ddb,
>>  		     struct intel_crtc_state *cstate,
>> +		     struct intel_plane *intel_plane,
>>  		     int level,
>>  		     struct skl_wm_level *result)
>>  {
>>  	struct drm_atomic_state *state = cstate->base.state;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
>>> base.crtc);
>> -	struct drm_plane *plane;
>> -	struct intel_plane *intel_plane;
>> -	struct intel_plane_state *intel_pstate;
>> +	struct drm_plane *plane = &intel_plane->base;
>> +	struct intel_plane_state *intel_pstate = NULL;
>>  	uint16_t ddb_blocks;
>>  	enum pipe pipe = intel_crtc->pipe;
>>  	int ret;
>> +	int i = skl_wm_plane_id(intel_plane);
>> +
>> +	if (state)
>> +		intel_pstate =
>> +			intel_atomic_get_existing_plane_state(state,
>> +							      intel_
>> plane);
>>  
>>  	/*
>> -	 * We'll only calculate watermarks for planes that are
>> actually
>> -	 * enabled, so make sure all other planes are set as
>> disabled.
>> +	 * Note: If we start supporting multiple pending atomic
>> commits against
>> +	 * the same planes/CRTC's in the future, plane->state will
>> no longer be
>> +	 * the correct pre-state to use for the calculations here
>> and we'll
>> +	 * need to change where we get the 'unchanged' plane data
>> from.
>> +	 *
>> +	 * For now this is fine because we only allow one queued
>> commit against
>> +	 * a CRTC.  Even if the plane isn't modified by this
>> transaction and we
>> +	 * don't have a plane lock, we still have the CRTC's lock,
>> so we know
>> +	 * that no other transactions are racing with us to update
>> it.
>>  	 */
>> -	memset(result, 0, sizeof(*result));
>> -
>> -	for_each_intel_plane_mask(&dev_priv->drm,
>> -				  intel_plane,
>> -				  cstate->base.plane_mask) {
>> -		int i = skl_wm_plane_id(intel_plane);
>> -
>> -		plane = &intel_plane->base;
>> -		intel_pstate = NULL;
>> -		if (state)
>> -			intel_pstate =
>> -				intel_atomic_get_existing_plane_stat
>> e(state,
>> -								    
>>   intel_plane);
>> +	if (!intel_pstate)
>> +		intel_pstate = to_intel_plane_state(plane->state);
>>  
>> -		/*
>> -		 * Note: If we start supporting multiple pending
>> atomic commits
>> -		 * against the same planes/CRTC's in the future,
>> plane->state
>> -		 * will no longer be the correct pre-state to use
>> for the
>> -		 * calculations here and we'll need to change where
>> we get the
>> -		 * 'unchanged' plane data from.
>> -		 *
>> -		 * For now this is fine because we only allow one
>> queued commit
>> -		 * against a CRTC.  Even if the plane isn't modified
>> by this
>> -		 * transaction and we don't have a plane lock, we
>> still have
>> -		 * the CRTC's lock, so we know that no other
>> transactions are
>> -		 * racing with us to update it.
>> -		 */
>> -		if (!intel_pstate)
>> -			intel_pstate = to_intel_plane_state(plane-
>>> state);
>> +	WARN_ON(!intel_pstate->base.fb);
>>  
>> -		WARN_ON(!intel_pstate->base.fb);
>> +	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
>>  
>> -		ddb_blocks = skl_ddb_entry_size(&ddb-
>>> plane[pipe][i]);
>> -
>> -		ret = skl_compute_plane_wm(dev_priv,
>> -					   cstate,
>> -					   intel_pstate,
>> -					   ddb_blocks,
>> -					   level,
>> -					   &result->plane_res_b[i],
>> -					   &result->plane_res_l[i],
>> -					   &result->plane_en[i]);
>> -		if (ret)
>> -			return ret;
>> -	}
>> +	ret = skl_compute_plane_wm(dev_priv,
>> +				   cstate,
>> +				   intel_pstate,
>> +				   ddb_blocks,
>> +				   level,
>> +				   &result->plane_res_b,
>> +				   &result->plane_res_l,
>> +				   &result->plane_en);
>> +	if (ret)
>> +		return ret;
>>  
>>  	return 0;
>>  }
>> @@ -3749,19 +3734,11 @@ skl_compute_linetime_wm(struct
>> intel_crtc_state *cstate)
>>  static void skl_compute_transition_wm(struct intel_crtc_state
>> *cstate,
>>  				      struct skl_wm_level *trans_wm
>> /* out */)
>>  {
>> -	struct drm_crtc *crtc = cstate->base.crtc;
>> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	struct intel_plane *intel_plane;
>> -
>>  	if (!cstate->base.active)
>>  		return;
>>  
>>  	/* Until we know more, just disable transition WMs */
>> -	for_each_intel_plane_on_crtc(crtc->dev, intel_crtc,
>> intel_plane) {
>> -		int i = skl_wm_plane_id(intel_plane);
>> -
>> -		trans_wm->plane_en[i] = false;
>> -	}
>> +	trans_wm->plane_en = false;
>>  }
>>  
>>  static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
>> @@ -3770,19 +3747,33 @@ static int skl_build_pipe_wm(struct
>> intel_crtc_state *cstate,
>>  {
>>  	struct drm_device *dev = cstate->base.crtc->dev;
>>  	const struct drm_i915_private *dev_priv = to_i915(dev);
>> +	struct intel_plane *intel_plane;
>> +	struct skl_plane_wm *wm;
>>  	int level, max_level = ilk_wm_max_level(dev);
>>  	int ret;
>>  
>> -	for (level = 0; level <= max_level; level++) {
>> -		ret = skl_compute_wm_level(dev_priv, ddb, cstate,
>> -					   level, &pipe_wm-
>>> wm[level]);
>> -		if (ret)
>> -			return ret;
>> +	/*
>> +	 * We'll only calculate watermarks for planes that are
>> actually
>> +	 * enabled, so make sure all other planes are set as
>> disabled.
>> +	 */
>> +	memset(pipe_wm->planes, 0, sizeof(pipe_wm->planes));
>> +
>> +	for_each_intel_plane_mask(&dev_priv->drm,
>> +				  intel_plane,
>> +				  cstate->base.plane_mask) {
>> +		wm = &pipe_wm->planes[skl_wm_plane_id(intel_plane)];
>> +
>> +		for (level = 0; level <= max_level; level++) {
>> +			ret = skl_compute_wm_level(dev_priv, ddb,
>> cstate,
>> +						   intel_plane,
>> level,
>> +						   &wm->wm[level]);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +		skl_compute_transition_wm(cstate, &wm->trans_wm);
>>  	}
>>  	pipe_wm->linetime = skl_compute_linetime_wm(cstate);
>>  
>> -	skl_compute_transition_wm(cstate, &pipe_wm->trans_wm);
>> -
>>  	return 0;
>>  }
>>  
>> @@ -3792,50 +3783,56 @@ static void skl_compute_wm_results(struct
>> drm_device *dev,
>>  				   struct intel_crtc *intel_crtc)
>>  {
>>  	int level, max_level = ilk_wm_max_level(dev);
>> +	struct skl_plane_wm *plane_wm;
>>  	enum pipe pipe = intel_crtc->pipe;
>>  	uint32_t temp;
>>  	int i;
>>  
>> -	for (level = 0; level <= max_level; level++) {
>> -		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>> +	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>> +		plane_wm = &p_wm->planes[i];
>> +
>> +		for (level = 0; level <= max_level; level++) {
>>  			temp = 0;
>>  
>> -			temp |= p_wm->wm[level].plane_res_l[i] <<
>> +			temp |= plane_wm->wm[level].plane_res_l <<
>>  					PLANE_WM_LINES_SHIFT;
>> -			temp |= p_wm->wm[level].plane_res_b[i];
>> -			if (p_wm->wm[level].plane_en[i])
>> +			temp |= plane_wm->wm[level].plane_res_b;
>> +			if (plane_wm->wm[level].plane_en)
>>  				temp |= PLANE_WM_EN;
>>  
>>  			r->plane[pipe][i][level] = temp;
>>  		}
>>  
>> -		temp = 0;
>> -
>> -		temp |= p_wm->wm[level].plane_res_l[PLANE_CURSOR] <<
>> PLANE_WM_LINES_SHIFT;
>> -		temp |= p_wm->wm[level].plane_res_b[PLANE_CURSOR];
>> +	}
>>  
>> -		if (p_wm->wm[level].plane_en[PLANE_CURSOR])
>> +	for (level = 0; level <= max_level; level++) {
>> +		plane_wm = &p_wm->planes[PLANE_CURSOR];
>> +		temp = 0;
>> +		temp |= plane_wm->wm[level].plane_res_l <<
>> PLANE_WM_LINES_SHIFT;
>> +		temp |= plane_wm->wm[level].plane_res_b;
>> +		if (plane_wm->wm[level].plane_en)
>>  			temp |= PLANE_WM_EN;
>>  
>>  		r->plane[pipe][PLANE_CURSOR][level] = temp;
>> -
>>  	}
>>  
>>  	/* transition WMs */
>>  	for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>> +		plane_wm = &p_wm->planes[i];
>>  		temp = 0;
>> -		temp |= p_wm->trans_wm.plane_res_l[i] <<
>> PLANE_WM_LINES_SHIFT;
>> -		temp |= p_wm->trans_wm.plane_res_b[i];
>> -		if (p_wm->trans_wm.plane_en[i])
>> +		temp |= plane_wm->trans_wm.plane_res_l <<
>> PLANE_WM_LINES_SHIFT;
>> +		temp |= plane_wm->trans_wm.plane_res_b;
>> +		if (plane_wm->trans_wm.plane_en)
>>  			temp |= PLANE_WM_EN;
>>  
>>  		r->plane_trans[pipe][i] = temp;
>>  	}
>>  
>> +	plane_wm = &p_wm->planes[PLANE_CURSOR];
>>  	temp = 0;
>> -	temp |= p_wm->trans_wm.plane_res_l[PLANE_CURSOR] <<
>> PLANE_WM_LINES_SHIFT;
>> -	temp |= p_wm->trans_wm.plane_res_b[PLANE_CURSOR];
>> -	if (p_wm->trans_wm.plane_en[PLANE_CURSOR])
>> +	temp |= plane_wm->trans_wm.plane_res_l <<
>> PLANE_WM_LINES_SHIFT;
>> +	temp |= plane_wm->trans_wm.plane_res_b;
>> +	if (plane_wm->trans_wm.plane_en)
>>  		temp |= PLANE_WM_EN;
>>  
>>  	r->plane_trans[pipe][PLANE_CURSOR] = temp;
>> @@ -4262,44 +4259,24 @@ static void ilk_optimize_watermarks(struct
>> intel_crtc_state *cstate)
>>  static void skl_pipe_wm_active_state(uint32_t val,
>>  				     struct skl_pipe_wm *active,
>>  				     bool is_transwm,
>> -				     bool is_cursor,
>>  				     int i,
>>  				     int level)
>>  {
>> +	struct skl_plane_wm *plane_wm = &active->planes[i];
>>  	bool is_enabled = (val & PLANE_WM_EN) != 0;
>>  
>>  	if (!is_transwm) {
>> -		if (!is_cursor) {
>> -			active->wm[level].plane_en[i] = is_enabled;
>> -			active->wm[level].plane_res_b[i] =
>> -					val & PLANE_WM_BLOCKS_MASK;
>> -			active->wm[level].plane_res_l[i] =
>> -					(val >>
>> PLANE_WM_LINES_SHIFT) &
>> -						PLANE_WM_LINES_MASK;
>> -		} else {
>> -			active->wm[level].plane_en[PLANE_CURSOR] =
>> is_enabled;
>> -			active->wm[level].plane_res_b[PLANE_CURSOR]
>> =
>> -					val & PLANE_WM_BLOCKS_MASK;
>> -			active->wm[level].plane_res_l[PLANE_CURSOR]
>> =
>> -					(val >>
>> PLANE_WM_LINES_SHIFT) &
>> -						PLANE_WM_LINES_MASK;
>> -		}
>> +		plane_wm->wm[level].plane_en = is_enabled;
>> +		plane_wm->wm[level].plane_res_b = val &
>> PLANE_WM_BLOCKS_MASK;
>> +		plane_wm->wm[level].plane_res_l =
>> +			(val >> PLANE_WM_LINES_SHIFT) &
>> +			PLANE_WM_LINES_MASK;
> Nitpick: you can join the two lines above and still stay under 80
> columns.
>
>
>>  	} else {
>> -		if (!is_cursor) {
>> -			active->trans_wm.plane_en[i] = is_enabled;
>> -			active->trans_wm.plane_res_b[i] =
>> -					val & PLANE_WM_BLOCKS_MASK;
>> -			active->trans_wm.plane_res_l[i] =
>> -					(val >>
>> PLANE_WM_LINES_SHIFT) &
>> -						PLANE_WM_LINES_MASK;
>> -		} else {
>> -			active->trans_wm.plane_en[PLANE_CURSOR] =
>> is_enabled;
>> -			active->trans_wm.plane_res_b[PLANE_CURSOR] =
>> -					val & PLANE_WM_BLOCKS_MASK;
>> -			active->trans_wm.plane_res_l[PLANE_CURSOR] =
>> -					(val >>
>> PLANE_WM_LINES_SHIFT) &
>> -						PLANE_WM_LINES_MASK;
>> -		}
>> +		plane_wm->trans_wm.plane_en = is_enabled;
>> +		plane_wm->trans_wm.plane_res_b = val &
>> PLANE_WM_BLOCKS_MASK;
>> +		plane_wm->trans_wm.plane_res_l =
>> +			(val >> PLANE_WM_LINES_SHIFT) &
>> +			PLANE_WM_LINES_MASK;
> Same here.
>
>
>>  	}
>>  }
>>  
>> @@ -4338,20 +4315,19 @@ static void skl_pipe_wm_get_hw_state(struct
>> drm_crtc *crtc)
>>  	for (level = 0; level <= max_level; level++) {
>>  		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>>  			temp = hw->plane[pipe][i][level];
>> -			skl_pipe_wm_active_state(temp, active,
>> false,
>> -						false, i, level);
>> +			skl_pipe_wm_active_state(temp, active,
>> false, i, level);
>>  		}
>>  		temp = hw->plane[pipe][PLANE_CURSOR][level];
>> -		skl_pipe_wm_active_state(temp, active, false, true,
>> i, level);
>> +		skl_pipe_wm_active_state(temp, active, false, i,
>> level);
> While this is not wrong today, history shows that the number of planes
> increases over time, so we may at some point in the future add PLANE_D
> and more, so the code will become wrong. Just pass PLANE_CURSOR instead
> of "i" here and below. Also, this simplification could have been a
> separate patch.
Agreed, but I want to note that PLANE_CURSOR is always supposed to be the last member.
Unless you have sprite planes covering the cursor, which doesn't ever happen.

~Maarten

Re: [PATCH 0/6] Start of skl watermark cleanup

[Maarten Lankhorst]

Op 05-10-16 om 17:33 schreef Lyude:
> While it (mostly) works, the code for handling watermarks on Skylake has been
> kind of ugly for a while. As well a lot of it isn't that friendly to atomic
> transactions, Lots of copy paste, redundant wm values, etc. While this isn't a
> full cleanup, it's a good start. As well, we add a couple of features for
> making debugging watermarks a little easier.
It's a good start, with the review comments addressed and rebased I think it's good to go. :)

Feel free to add my r-b.

~Maarten