Start of skl watermark cleanup

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.

Changes since v1:
- Don't replace alloc->start = alloc->end = 0;

Signed-off-by Lyude <[email protected]>

Reviewed-by Maarten Lankhorst <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Paulo Zanoni <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_drv.h +0 -0

index a219a35..bb2de8d 100644
@@ -1637,7 +1637,6 @@ static inline bool skl_ddb_entry_equal(const
 struct skl_ddb_entry *e1,
  }

 struct skl_ddb_allocation {

drivers/gpu/drm/i915/i915_drv.h +0 -1

-	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];
 };

drivers/gpu/drm/i915/intel_display.c +0 -1

index a366656..17733af 100644
@@ -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;

drivers/gpu/drm/i915/intel_display.c +1 -0

+	struct intel_crtc *intel_crtc;

  struct drm_crtc_state *old_crtc_state;

drivers/gpu/drm/i915/intel_display.c +0 -1

-	struct skl_ddb_allocation *new_ddb = &intel_state-

Quoted 1 line

wm_results.ddb;

drivers/gpu/drm/i915/intel_display.c +0 -1

-	struct skl_ddb_allocation *cur_ddb = &dev_priv-

Quoted 1 line

wm.skl_hw.ddb;

drivers/gpu/drm/i915/intel_display.c +1 -0

+	struct intel_crtc_state *cstate;

  unsigned int updated = 0;
  bool progress;
  enum pipe pipe;

drivers/gpu/drm/i915/intel_display.c +4 -1

@@ -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;

drivers/gpu/drm/i915/intel_display.c +0 -1

-			if (skl_ddb_allocation_overlaps(state,

cur_ddb, new_ddb,

drivers/gpu/drm/i915/intel_display.c +1 -1

-							pipe))
+			if (skl_ddb_allocation_overlaps(state,

intel_crtc))
  continue;

  updated |= cmask;

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -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. */

drivers/gpu/drm/i915/intel_display.c +0 -1

-			if (!skl_ddb_allocation_equals(cur_ddb,

new_ddb, pipe) &&

drivers/gpu/drm/i915/intel_display.c +1 -0

+			if (!skl_ddb_entry_equal(&cstate-

Quoted 1 line

wm.skl.ddb,

drivers/gpu/drm/i915/intel_display.c +1 -0

+						 &intel_crtc-

Quoted 1 line

hw_ddb) &&

      !crtc->state->active_changed && intel_state->wm_results.dirty_pipes != updated)
  vbl_wait = true;

drivers/gpu/drm/i915/intel_drv.h +1 -0

index f48e79a..35ba282 100644
@@ -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];

drivers/gpu/drm/i915/intel_drv.h +0 -0

@@ -733,6 +734,9 @@ struct intel_crtc {
  		bool cxsr_allowed;
  	} wm;

drivers/gpu/drm/i915/intel_drv.h +3 -0

+	/* gen9+: ddb allocation currently being used */
+	struct skl_ddb_entry hw_ddb;
+

  int scanline_offset;

  struct {

drivers/gpu/drm/i915/intel_drv.h +1 -3

@@ -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,

drivers/gpu/drm/i915/intel_pm.c +0 -1

index fe6c1c6..68355b0 100644
@@ -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;

drivers/gpu/drm/i915/intel_pm.c +1 -1

@@ -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;
  	}

drivers/gpu/drm/i915/intel_pm.c +1 -1

@@ -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;

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -3370,7 +3369,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
 *cstate,
  		return 0;

  if (!cstate->base.active) {

drivers/gpu/drm/i915/intel_pm.c +1 -1

-		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
+		alloc->start = alloc->end = 0;

  return 0;
  }

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -3897,14 +3896,6 @@ void skl_write_cursor_wm(struct intel_crtc
 *intel_crtc,
  			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
  }

drivers/gpu/drm/i915/intel_pm.c +0 -8

-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)
 {

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -3912,22 +3903,22 @@ static inline bool
 skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
  }

 bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,

drivers/gpu/drm/i915/intel_pm.c +0 -1

-				 const struct skl_ddb_allocation

*old,

drivers/gpu/drm/i915/intel_pm.c +0 -1

-				 const struct skl_ddb_allocation

*new,

drivers/gpu/drm/i915/intel_pm.c +1 -1

-				 enum pipe pipe)
+				 struct intel_crtc *intel_crtc)

 {

drivers/gpu/drm/i915/intel_pm.c +5 -3

-	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)-

Quoted 1 line

wm.skl.ddb;

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	int i;

drivers/gpu/drm/i915/intel_pm.c +2 -2

-	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);

drivers/gpu/drm/i915/intel_pm.c +1 -1

-		if (otherp == pipe)
+		if (other_intel_crtc == intel_crtc)

  continue;

drivers/gpu/drm/i915/intel_pm.c +1 -2

-		if (skl_ddb_entries_overlap(&new->pipe[pipe],
-					    &old->pipe[otherp]))
+		if (skl_ddb_entries_overlap(ddb, &other_intel_crtc-

Quoted 1 line

hw_ddb))

  return true;
  }

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -4092,7 +4083,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]));

drivers/gpu/drm/i915/intel_pm.c +0 -1

-	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],

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -4200,6 +4190,8 @@ static void skl_update_wm(struct drm_crtc
 *crtc)

  skl_copy_wm_for_pipe(hw_vals, results, pipe);

drivers/gpu/drm/i915/intel_pm.c +2 -0

+	intel_crtc->hw_ddb = cstate->wm.skl.ddb;
+

  mutex_unlock(&dev_priv->wm.wm_mutex);
 }

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.

Changes since v1:
- Style nitpicks
- Fix accidental usage of i vs. PLANE_CURSOR
- Split out skl_pipe_wm_active_state simplification into separate patch

Signed-off-by Lyude <[email protected]>

Reviewed-by Maarten Lankhorst <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.h  |   6 +- drivers/gpu/drm/i915/intel_drv.h |   6 +- drivers/gpu/drm/i915/intel_pm.c  | 207 +++++++++++++++++++--------
------------

 3 files changed, 111 insertions(+), 108 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -0

index e9d035ea..0287c93 100644
@@ -1649,9 +1649,9 @@ struct skl_wm_values {
  };

 struct skl_wm_level {

drivers/gpu/drm/i915/i915_drv.h +3 -3

-	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;

 };

 /*

drivers/gpu/drm/i915/intel_drv.h +0 -0

index 35ba282..d684f4f 100644
@@ -468,9 +468,13 @@ struct intel_pipe_wm {
  	bool sprites_scaled;
  };

drivers/gpu/drm/i915/intel_drv.h +1 -1

-struct skl_pipe_wm {
+struct skl_plane_wm {

  struct skl_wm_level wm[8];
  struct skl_wm_level trans_wm;

drivers/gpu/drm/i915/intel_drv.h +4 -0

+};
+
+struct skl_pipe_wm {
+	struct skl_plane_wm planes[I915_MAX_PLANES];

  uint32_t linetime;
 };

drivers/gpu/drm/i915/intel_pm.c +1 -0

index cc5d5e9..4c2ebcd 100644
@@ -3670,67 +3670,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-

Quoted 1 line

base.crtc);

drivers/gpu/drm/i915/intel_pm.c +2 -3

-	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;

drivers/gpu/drm/i915/intel_pm.c +6 -0

+	int i = skl_wm_plane_id(intel_plane);
+
+	if (state)
+		intel_pstate =
+			intel_atomic_get_existing_plane_state(state,
+							      intel_

plane);

  /*

drivers/gpu/drm/i915/intel_pm.c +0 -1

-	 * We'll only calculate watermarks for planes that are

actually

drivers/gpu/drm/i915/intel_pm.c +0 -1

-	 * enabled, so make sure all other planes are set as

disabled.

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	 * Note: If we start supporting multiple pending atomic

commits against

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	 * the same planes/CRTC's in the future, plane->state will

no longer be

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	 * the correct pre-state to use for the calculations here

and we'll

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	 * need to change where we get the 'unchanged' plane data

from.

drivers/gpu/drm/i915/intel_pm.c +2 -0

+	 *
+	 * For now this is fine because we only allow one queued

commit against

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	 * a CRTC.  Even if the plane isn't modified by this

transaction and we

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	 * don't have a plane lock, we still have the CRTC's lock,

so we know

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	 * that no other transactions are racing with us to update

it.
   */

drivers/gpu/drm/i915/intel_pm.c +2 -1

-	memset(result, 0, sizeof(*result));
+	if (!intel_pstate)
+		intel_pstate = to_intel_plane_state(plane->state);

drivers/gpu/drm/i915/intel_pm.c +0 -10

-	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,

drivers/gpu/drm/i915/intel_pm.c +0 -1

-								    

  intel_plane);

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	WARN_ON(!intel_pstate->base.fb);

drivers/gpu/drm/i915/intel_pm.c +0 -2

-		/*
-		 * Note: If we start supporting multiple pending

atomic commits

drivers/gpu/drm/i915/intel_pm.c +0 -1

-		 * against the same planes/CRTC's in the future,

plane->state

drivers/gpu/drm/i915/intel_pm.c +0 -1

-		 * will no longer be the correct pre-state to use

for the

drivers/gpu/drm/i915/intel_pm.c +0 -1

-		 * calculations here and we'll need to change where

we get the

drivers/gpu/drm/i915/intel_pm.c +0 -3

-		 * 'unchanged' plane data from.
-		 *
-		 * For now this is fine because we only allow one

queued commit

drivers/gpu/drm/i915/intel_pm.c +0 -1

-		 * against a CRTC.  Even if the plane isn't modified

by this

drivers/gpu/drm/i915/intel_pm.c +0 -1

-		 * transaction and we don't have a plane lock, we

still have

drivers/gpu/drm/i915/intel_pm.c +0 -1

-		 * the CRTC's lock, so we know that no other

transactions are

drivers/gpu/drm/i915/intel_pm.c +0 -4

-		 * racing with us to update it.
-		 */
-		if (!intel_pstate)
-			intel_pstate = to_intel_plane_state(plane-

Quoted 1 line

state);

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);

drivers/gpu/drm/i915/intel_pm.c +0 -3

-		WARN_ON(!intel_pstate->base.fb);
-
-		ddb_blocks = skl_ddb_entry_size(&ddb-

Quoted 1 line

plane[pipe][i]);

drivers/gpu/drm/i915/intel_pm.c +10 -12

-
-		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;
 }

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -3751,19 +3736,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 */)
 {

drivers/gpu/drm/i915/intel_pm.c +0 -4

-	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 */

drivers/gpu/drm/i915/intel_pm.c +0 -1

-	for_each_intel_plane_on_crtc(crtc->dev, intel_crtc,

intel_plane) {

drivers/gpu/drm/i915/intel_pm.c +1 -4

-		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,

drivers/gpu/drm/i915/intel_pm.c +2 -0

@@ -3772,19 +3749,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;

drivers/gpu/drm/i915/intel_pm.c +0 -3

-	for (level = 0; level <= max_level; level++) {
-		ret = skl_compute_wm_level(dev_priv, ddb, cstate,
-					   level, &pipe_wm-

Quoted 1 line

wm[level]);

drivers/gpu/drm/i915/intel_pm.c +2 -2

-		if (ret)
-			return ret;
+	/*
+	 * We'll only calculate watermarks for planes that are

actually

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	 * enabled, so make sure all other planes are set as

disabled.

drivers/gpu/drm/i915/intel_pm.c +10 -0

+	 */
+	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,

drivers/gpu/drm/i915/intel_pm.c +1 -0

+						   intel_plane,

level,

drivers/gpu/drm/i915/intel_pm.c +5 -0

+						   &wm->wm[level]);
+			if (ret)
+				return ret;
+		}
+		skl_compute_transition_wm(cstate, &wm->trans_wm);

  }
  pipe_wm->linetime = skl_compute_linetime_wm(cstate);

drivers/gpu/drm/i915/intel_pm.c +0 -2

-	skl_compute_transition_wm(cstate, &pipe_wm->trans_wm);
-

  return 0;
 }

drivers/gpu/drm/i915/intel_pm.c +1 -0

@@ -3794,50 +3785,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;

drivers/gpu/drm/i915/intel_pm.c +4 -2

-	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;

drivers/gpu/drm/i915/intel_pm.c +1 -1

-			temp |= p_wm->wm[level].plane_res_l[i] <<
+			temp |= plane_wm->wm[level].plane_res_l <<

  PLANE_WM_LINES_SHIFT;

drivers/gpu/drm/i915/intel_pm.c +2 -2

-			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; }

drivers/gpu/drm/i915/intel_pm.c +0 -3

-		temp = 0;
-
-		temp |= p_wm->wm[level].plane_res_l[PLANE_CURSOR] <<

PLANE_WM_LINES_SHIFT;

drivers/gpu/drm/i915/intel_pm.c +1 -1

-		temp |= p_wm->wm[level].plane_res_b[PLANE_CURSOR];
+	}

drivers/gpu/drm/i915/intel_pm.c +4 -1

-		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;

drivers/gpu/drm/i915/intel_pm.c +2 -0

+		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++) {

drivers/gpu/drm/i915/intel_pm.c +1 -0

+		plane_wm = &p_wm->planes[i];

  temp = 0;

drivers/gpu/drm/i915/intel_pm.c +0 -1

-		temp |= p_wm->trans_wm.plane_res_l[i] <<

PLANE_WM_LINES_SHIFT;

drivers/gpu/drm/i915/intel_pm.c +1 -2

-		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;

drivers/gpu/drm/i915/intel_pm.c +2 -0

+		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;
  }

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	plane_wm = &p_wm->planes[PLANE_CURSOR];

  temp = 0;

drivers/gpu/drm/i915/intel_pm.c +0 -1

-	temp |= p_wm->trans_wm.plane_res_l[PLANE_CURSOR] <<

PLANE_WM_LINES_SHIFT;

drivers/gpu/drm/i915/intel_pm.c +1 -2

-	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;

drivers/gpu/drm/i915/intel_pm.c +2 -0

+	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;

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -4278,35 +4275,37 @@ static void skl_pipe_wm_active_state(uint32_t
 val,

  if (!is_transwm) {
  if (!is_cursor) {

drivers/gpu/drm/i915/intel_pm.c +0 -5

-			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) &

drivers/gpu/drm/i915/intel_pm.c +1 -1

-						PLANE_WM_LINES_MASK;
+			active->planes[i].wm[level].plane_en =

is_enabled;

drivers/gpu/drm/i915/intel_pm.c +5 -0

+			active->planes[i].wm[level].plane_res_b =
+				val & PLANE_WM_BLOCKS_MASK;
+			active->planes[i].wm[level].plane_res_l =
+				(val >> PLANE_WM_LINES_SHIFT) &
+				PLANE_WM_LINES_MASK;

  } else {

drivers/gpu/drm/i915/intel_pm.c +0 -1

-			active->wm[level].plane_en[PLANE_CURSOR] =

is_enabled;

drivers/gpu/drm/i915/intel_pm.c +0 -1

-			active->wm[level].plane_res_b[PLANE_CURSOR]

=

drivers/gpu/drm/i915/intel_pm.c +0 -2

-					val & PLANE_WM_BLOCKS_MASK;
-			active->wm[level].plane_res_l[PLANE_CURSOR]

=

drivers/gpu/drm/i915/intel_pm.c +0 -1

-					(val >>

PLANE_WM_LINES_SHIFT) &

drivers/gpu/drm/i915/intel_pm.c +1 -1

-						PLANE_WM_LINES_MASK;
+			active-

Quoted 1 line

planes[PLANE_CURSOR].wm[level].plane_en =

drivers/gpu/drm/i915/intel_pm.c +2 -0

+				is_enabled;
+			active-

Quoted 1 line

planes[PLANE_CURSOR].wm[level].plane_res_b =

drivers/gpu/drm/i915/intel_pm.c +2 -0

+				val & PLANE_WM_BLOCKS_MASK;
+			active-

Quoted 1 line

planes[PLANE_CURSOR].wm[level].plane_res_l =

drivers/gpu/drm/i915/intel_pm.c +2 -0

+				(val >> PLANE_WM_LINES_SHIFT) &
+				PLANE_WM_LINES_MASK;

  }
  } else {
  if (!is_cursor) {

drivers/gpu/drm/i915/intel_pm.c +0 -5

-			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) &

drivers/gpu/drm/i915/intel_pm.c +1 -1

-						PLANE_WM_LINES_MASK;
+			active->planes[i].trans_wm.plane_en =

is_enabled;

drivers/gpu/drm/i915/intel_pm.c +5 -0

+			active->planes[i].trans_wm.plane_res_b =
+				val & PLANE_WM_BLOCKS_MASK;
+			active->planes[i].trans_wm.plane_res_l =
+				(val >> PLANE_WM_LINES_SHIFT) &
+				PLANE_WM_LINES_MASK;

  } else {

drivers/gpu/drm/i915/intel_pm.c +0 -1

-			active->trans_wm.plane_en[PLANE_CURSOR] =

is_enabled;

drivers/gpu/drm/i915/intel_pm.c +0 -4

-			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) &

drivers/gpu/drm/i915/intel_pm.c +1 -1

-						PLANE_WM_LINES_MASK;
+			active-

Quoted 1 line

planes[PLANE_CURSOR].trans_wm.plane_en =

drivers/gpu/drm/i915/intel_pm.c +2 -0

+				is_enabled;
+			active-

Quoted 1 line

planes[PLANE_CURSOR].trans_wm.plane_res_b =

drivers/gpu/drm/i915/intel_pm.c +2 -0

+				val & PLANE_WM_BLOCKS_MASK;
+			active-

Quoted 1 line

planes[PLANE_CURSOR].trans_wm.plane_res_l =

drivers/gpu/drm/i915/intel_pm.c +2 -0

+				(val >> PLANE_WM_LINES_SHIFT) &
+				PLANE_WM_LINES_MASK;

  }
  }
 }

This function is a wreck, let's help it get it's life back together and
cleanup all of the copy pasta here.

(adding Maarten's reviewed-by since this is just a split-up version of one
of the previous patches)

Signed-off-by Lyude <[email protected]>

Reviewed-by Maarten Lankhorst <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_pm.c | 52 +++++++++++------------------
------------

 1 file changed, 14 insertions(+), 38 deletions(-)

drivers/gpu/drm/i915/intel_pm.c +1 -1

index 4c2ebcd..5dbaf12 100644
@@ -4267,46 +4267,22 @@ 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) {

drivers/gpu/drm/i915/intel_pm.c +0 -2

-		if (!is_cursor) {
-			active->planes[i].wm[level].plane_en =

is_enabled;

drivers/gpu/drm/i915/intel_pm.c +0 -7

-			active->planes[i].wm[level].plane_res_b =
-				val & PLANE_WM_BLOCKS_MASK;
-			active->planes[i].wm[level].plane_res_l =
-				(val >> PLANE_WM_LINES_SHIFT) &
-				PLANE_WM_LINES_MASK;
-		} else {
-			active-

Quoted 1 line

planes[PLANE_CURSOR].wm[level].plane_en =

drivers/gpu/drm/i915/intel_pm.c +0 -2

-				is_enabled;
-			active-

Quoted 1 line

planes[PLANE_CURSOR].wm[level].plane_res_b =

drivers/gpu/drm/i915/intel_pm.c +0 -2

-				val & PLANE_WM_BLOCKS_MASK;
-			active-

Quoted 1 line

planes[PLANE_CURSOR].wm[level].plane_res_l =

drivers/gpu/drm/i915/intel_pm.c +2 -3

-				(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;

drivers/gpu/drm/i915/intel_pm.c +2 -0

+		plane_wm->wm[level].plane_res_l =
+			(val >> PLANE_WM_LINES_SHIFT) &

PLANE_WM_LINES_MASK;
  } else {

drivers/gpu/drm/i915/intel_pm.c +0 -2

-		if (!is_cursor) {
-			active->planes[i].trans_wm.plane_en =

is_enabled;

drivers/gpu/drm/i915/intel_pm.c +0 -7

-			active->planes[i].trans_wm.plane_res_b =
-				val & PLANE_WM_BLOCKS_MASK;
-			active->planes[i].trans_wm.plane_res_l =
-				(val >> PLANE_WM_LINES_SHIFT) &
-				PLANE_WM_LINES_MASK;
-		} else {
-			active-

Quoted 1 line

planes[PLANE_CURSOR].trans_wm.plane_en =

drivers/gpu/drm/i915/intel_pm.c +0 -2

-				is_enabled;
-			active-

Quoted 1 line

planes[PLANE_CURSOR].trans_wm.plane_res_b =

drivers/gpu/drm/i915/intel_pm.c +0 -2

-				val & PLANE_WM_BLOCKS_MASK;
-			active-

Quoted 1 line

planes[PLANE_CURSOR].trans_wm.plane_res_l =

drivers/gpu/drm/i915/intel_pm.c +2 -3

-				(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;

drivers/gpu/drm/i915/intel_pm.c +2 -0

+		plane_wm->trans_wm.plane_res_l =
+			(val >> PLANE_WM_LINES_SHIFT) &

PLANE_WM_LINES_MASK;
  }
 }

drivers/gpu/drm/i915/intel_pm.c +3 -3

@@ -4345,20 +4321,20 @@ 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,
 PLANE_CURSOR,
+					 level);
  	}

  for (i = 0; i < intel_num_planes(intel_crtc); i++) { temp = hw->plane_trans[pipe][i];

drivers/gpu/drm/i915/intel_pm.c +0 -1

-		skl_pipe_wm_active_state(temp, active, true, false,

i, 0);

drivers/gpu/drm/i915/intel_pm.c +1 -0

+		skl_pipe_wm_active_state(temp, active, true, i, 0);

  }

  temp = hw->plane_trans[pipe][PLANE_CURSOR];

drivers/gpu/drm/i915/intel_pm.c +1 -1

-	skl_pipe_wm_active_state(temp, active, true, true, i, 0);
+	skl_pipe_wm_active_state(temp, active, true, PLANE_CURSOR,

0);

  intel_crtc->wm.active.skl = *active;
 }

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.

Changes since v1:
- Fixup skl_write_wm_level()
- Fixup skl_wm_level_from_reg_val()
- Don't forget to copy *active to intel_crtc->wm.active.skl

Signed-off-by Lyude <[email protected]>

Reviewed-by Maarten Lankhorst <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Paulo Zanoni <[email protected]>

---
 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      | 204 ++++++++++++-----------------------
 drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
 5 files changed, 90 insertions(+), 144 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -2

index 0287c93..76583b2 100644
@@ -1644,8 +1644,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 {

drivers/gpu/drm/i915/intel_display.c +11 -3

index a71d05a..39400a0 100644
@@ -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;

drivers/gpu/drm/i915/intel_drv.h +4 -2

index d684f4f..958dc72 100644
@@ -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);

drivers/gpu/drm/i915/intel_pm.c +2 -0

index 5dbaf12..5cb537c 100644
@@ -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;
 

Op 08-10-16 om 02:11 schreef Lyude:

Quoted 160 lines

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.

Changes since v1:
- Fixup skl_write_wm_level()
- Fixup skl_wm_level_from_reg_val()
- Don't forget to copy *active to intel_crtc->wm.active.skl

Signed-off-by Lyude <[email protected]>

Reviewed-by Maarten Lankhorst <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Paulo Zanoni <[email protected]>

---
 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      | 204 ++++++++++++-----------
------------
 drivers/gpu/drm/i915/intel_sprite.c  |   8 +-

 5 files changed, 90 insertions(+), 144 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -2

index 0287c93..76583b2 100644
@@ -1644,8 +1644,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 {

drivers/gpu/drm/i915/intel_display.c +0 -0

index a71d05a..39400a0 100644
@@ -3378,6 +3378,8 @@ static void
 skylake_update_primary_plane(struct drm_plane *plane,

  struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-

Quoted 1 line

base.crtc);

  struct drm_framebuffer *fb = plane_state->base.fb; const struct skl_wm_values *wm = &dev_priv-

Quoted 1 line

wm.skl_results;

drivers/gpu/drm/i915/intel_display.c +2 -0

+	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;

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -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))

drivers/gpu/drm/i915/intel_display.c +1 -1

-		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);

drivers/gpu/drm/i915/intel_display.c +2 -0

@@ -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;

  /*

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -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)

drivers/gpu/drm/i915/intel_display.c +0 -1

-		skl_write_plane_wm(intel_crtc, &dev_priv-

Quoted 1 line

wm.skl_results, 0);

drivers/gpu/drm/i915/intel_display.c +2 -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);

drivers/gpu/drm/i915/intel_display.c +1 -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-

Quoted 1 line

wm.skl_results;

drivers/gpu/drm/i915/intel_display.c +2 -0

+	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))

drivers/gpu/drm/i915/intel_display.c +1 -1

-		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;

drivers/gpu/drm/i915/intel_drv.h +4 -2

index d684f4f..958dc72 100644
@@ -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);

drivers/gpu/drm/i915/intel_pm.c +0 -0

index 5dbaf12..5cb537c 100644
@@ -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;

drivers/gpu/drm/i915/intel_pm.c +2 -0

+	struct intel_crtc_state *cstate;
+	struct skl_plane_wm *wm;

  enum pipe pipe;
  int level, plane;

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct
 drm_atomic_state *state)

Quoted 6 lines

drivers/gpu/drm/i915/intel_pm.c +0 -0

  	/* 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];

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	cstate = intel_atomic_get_crtc_state(state,

to_intel_crtc(crtc));

Wrong cstate here, should be crtc->state since it's committed and crtc_state is the old state.

~Maarten

Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:

Quoted 179 lines

Op 08-10-16 om 02:11 schreef Lyude:

Quoted 177 lines

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.

Changes since v1:
- Fixup skl_write_wm_level()
- Fixup skl_wm_level_from_reg_val()
- Don't forget to copy *active to intel_crtc->wm.active.skl

Signed-off-by Lyude <[email protected]>

Reviewed-by Maarten Lankhorst <[email protected]

Quoted 1 line

Cc Ville Syrjälä <[email protected]>

Cc Paulo Zanoni <[email protected]>

---
 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      | 204 ++++++++++++--------- --
------------
 drivers/gpu/drm/i915/intel_sprite.c  |   8 +-

 5 files changed, 90 insertions(+), 144 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -2

index 0287c93..76583b2 100644
@@ -1644,8 +1644,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 {

drivers/gpu/drm/i915/intel_display.c +0 -0

index a71d05a..39400a0 100644
@@ -3378,6 +3378,8 @@ static void
 skylake_update_primary_plane(struct drm_plane *plane,

  struct intel_crtc *intel_crtc =
to_intel_crtc(crtc_state-

Quoted 2 lines

base.crtc);

  struct drm_framebuffer *fb = plane_state->base.fb; const struct skl_wm_values *wm = &dev_priv-

Quoted 2 lines

wm.skl_results;

drivers/gpu/drm/i915/intel_display.c +2 -0

+	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;

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -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))

drivers/gpu/drm/i915/intel_display.c +1 -1

-		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);

drivers/gpu/drm/i915/intel_display.c +2 -0

@@ -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-

Quoted 2 lines

wm.skl.optimal.planes[0];

  int pipe = intel_crtc->pipe;

  /*

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -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)

drivers/gpu/drm/i915/intel_display.c +0 -1

-		skl_write_plane_wm(intel_crtc, &dev_priv-

Quoted 2 lines

wm.skl_results, 0);

drivers/gpu/drm/i915/intel_display.c +2 -0

+		skl_write_plane_wm(intel_crtc, p_wm,
+				   &dev_priv-

Quoted 1 line

wm.skl_results.ddb,

0);

  I915_WRITE(PLANE_CTL(pipe, 0), 0);
  I915_WRITE(PLANE_SURF(pipe, 0), 0);

drivers/gpu/drm/i915/intel_display.c +1 -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-

Quoted 2 lines

wm.skl_results;

drivers/gpu/drm/i915/intel_display.c +2 -0

+	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))

drivers/gpu/drm/i915/intel_display.c +1 -1

-		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;

drivers/gpu/drm/i915/intel_drv.h +0 -0

index d684f4f..958dc72 100644
@@ -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,

drivers/gpu/drm/i915/intel_drv.h +2 -1

-			 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,

drivers/gpu/drm/i915/intel_drv.h +2 -1

-			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);

drivers/gpu/drm/i915/intel_pm.c +0 -0

index 5dbaf12..5cb537c 100644
@@ -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;

drivers/gpu/drm/i915/intel_pm.c +2 -0

+	struct intel_crtc_state *cstate;
+	struct skl_plane_wm *wm;

  enum pipe pipe;
  int level, plane;

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct
 drm_atomic_state *state)

Quoted 8 lines

Quoted 6 lines

drivers/gpu/drm/i915/intel_pm.c +0 -0

  	/* 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];

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	cstate = intel_atomic_get_crtc_state(state,

to_intel_crtc(crtc));

Wrong cstate here, should be crtc->state since it's committed and crtc_state is the old state.

Ah... How many instances of the "you're looking at the wrong state struct" have we spotted/fixed ever since the introduction of the atomic
code? Can't we try to do something more human-proof in order to avoid these problems from happening again and again?

Like for example copying the old states to a specific structure like drm_atomic_state->old_states_which_you_probably_shouldn_be_using, and then setting the CRTC/connector/plane states in the drm_atomic_state struct to NULL so bugs will be much easier to spot?

Anyway, with this problem fixed, feel free to add:

Reviewed-by Paulo Zanoni <[email protected]>

Quoted 4 lines

~Maarten

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Em Qui, 2016-10-13 às 17:04 -0300, Paulo Zanoni escreveu:

Quoted 222 lines

Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:

Quoted 196 lines

Op 08-10-16 om 02:11 schreef Lyude:

Quoted 193 lines

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.

Changes since v1:
- Fixup skl_write_wm_level()
- Fixup skl_wm_level_from_reg_val()
- Don't forget to copy *active to intel_crtc->wm.active.skl

Signed-off-by Lyude <[email protected]>

Reviewed-by Maarten Lankhorst <[email protected]

om

Quoted 2 lines

Cc Ville Syrjälä <[email protected]>

Cc Paulo Zanoni <[email protected]>

---
 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      | 204 ++++++++++++------- --
--
------------
 drivers/gpu/drm/i915/intel_sprite.c  |   8 +-

 5 files changed, 90 insertions(+), 144 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -2

index 0287c93..76583b2 100644
@@ -1644,8 +1644,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 {

drivers/gpu/drm/i915/intel_display.c +0 -0

index a71d05a..39400a0 100644
@@ -3378,6 +3378,8 @@ static void
 skylake_update_primary_plane(struct drm_plane *plane,

  struct intel_crtc *intel_crtc =
to_intel_crtc(crtc_state-

Quoted 3 lines

base.crtc);

  struct drm_framebuffer *fb = plane_state->base.fb; const struct skl_wm_values *wm = &dev_priv-

Quoted 3 lines

wm.skl_results;

drivers/gpu/drm/i915/intel_display.c +2 -0

+	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;

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -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-

Quoted 1 line

base))

drivers/gpu/drm/i915/intel_display.c +1 -1

-		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);

drivers/gpu/drm/i915/intel_display.c +2 -0

@@ -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-

Quoted 3 lines

wm.skl.optimal.planes[0];

  int pipe = intel_crtc->pipe;

  /*

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -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)

drivers/gpu/drm/i915/intel_display.c +0 -1

-		skl_write_plane_wm(intel_crtc, &dev_priv-

Quoted 3 lines

wm.skl_results, 0);

drivers/gpu/drm/i915/intel_display.c +2 -0

+		skl_write_plane_wm(intel_crtc, p_wm,
+				   &dev_priv-

Quoted 2 lines

wm.skl_results.ddb,

0);

  I915_WRITE(PLANE_CTL(pipe, 0), 0);
  I915_WRITE(PLANE_SURF(pipe, 0), 0);

drivers/gpu/drm/i915/intel_display.c +1 -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-

Quoted 3 lines

wm.skl_results;

drivers/gpu/drm/i915/intel_display.c +2 -0

+	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))

drivers/gpu/drm/i915/intel_display.c +1 -1

-		skl_write_cursor_wm(intel_crtc, wm);
+		skl_write_cursor_wm(intel_crtc, p_wm, &wm-

Quoted 1 line

ddb);

  if (plane_state && plane_state->base.visible) { cntl = MCURSOR_GAMMA_ENABLE;

drivers/gpu/drm/i915/intel_drv.h +0 -0

index d684f4f..958dc72 100644
@@ -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,

drivers/gpu/drm/i915/intel_drv.h +2 -1

-			 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,

drivers/gpu/drm/i915/intel_drv.h +2 -1

-			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);

drivers/gpu/drm/i915/intel_pm.c +0 -0

index 5dbaf12..5cb537c 100644
@@ -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;

drivers/gpu/drm/i915/intel_pm.c +2 -0

+	struct intel_crtc_state *cstate;
+	struct skl_plane_wm *wm;

  enum pipe pipe;
  int level, plane;

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct
 drm_atomic_state *state)

Quoted 8 lines

Quoted 6 lines

drivers/gpu/drm/i915/intel_pm.c +0 -0

  	/* 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];

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	cstate = intel_atomic_get_crtc_state(state,

to_intel_crtc(crtc));

Wrong cstate here, should be crtc->state since it's committed and crtc_state is the old state.

Ah... How many instances of the "you're looking at the wrong state struct" have we spotted/fixed ever since the introduction of the atomic
code? Can't we try to do something more human-proof in order to avoid
these problems from happening again and again?

Like for example copying the old states to a specific structure like drm_atomic_state->old_states_which_you_probably_shouldn_be_using, and
then setting the CRTC/connector/plane states in the drm_atomic_state
struct to NULL so bugs will be much easier to spot?

Quoted 2 lines

Anyway, with this problem fixed, feel free to add:

Reviewed-by Paulo Zanoni <[email protected]>

Oh, I forgot to mention: this patch has a conflict with my "unconditionally apply the memory workaround" patch, so we may need to
decide which one gets merged first. Of course I'd suggest my patch to be merged first since I want it to land in stable, but I'm always biased towards my patches. If you agree, I can even volunteer myself to
solve the conflicts of this patch later when applying.

Quoted 10 lines

Quoted 6 lines

~Maarten

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:

Quoted 161 lines

Op 08-10-16 om 02:11 schreef Lyude:

Quoted 160 lines

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.

Changes since v1:
- Fixup skl_write_wm_level()
- Fixup skl_wm_level_from_reg_val()
- Don't forget to copy *active to intel_crtc->wm.active.skl

Signed-off-by Lyude <[email protected]>

Reviewed-by Maarten Lankhorst <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Paulo Zanoni <[email protected]>

---
 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      | 204 ++++++++++++-----------
------------
 drivers/gpu/drm/i915/intel_sprite.c  |   8 +-

 5 files changed, 90 insertions(+), 144 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -2

index 0287c93..76583b2 100644
@@ -1644,8 +1644,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 {

drivers/gpu/drm/i915/intel_display.c +0 -0

index a71d05a..39400a0 100644
@@ -3378,6 +3378,8 @@ static void
 skylake_update_primary_plane(struct drm_plane *plane,

  struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state-

Quoted 1 line

base.crtc);

  struct drm_framebuffer *fb = plane_state->base.fb; const struct skl_wm_values *wm = &dev_priv-

Quoted 1 line

wm.skl_results;

drivers/gpu/drm/i915/intel_display.c +2 -0

+	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;

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -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))

drivers/gpu/drm/i915/intel_display.c +1 -1

-		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);

drivers/gpu/drm/i915/intel_display.c +2 -0

@@ -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;

  /*

drivers/gpu/drm/i915/intel_display.c +0 -0

@@ -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)

drivers/gpu/drm/i915/intel_display.c +0 -1

-		skl_write_plane_wm(intel_crtc, &dev_priv-

Quoted 1 line

wm.skl_results, 0);

drivers/gpu/drm/i915/intel_display.c +2 -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);

drivers/gpu/drm/i915/intel_display.c +1 -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-

Quoted 1 line

wm.skl_results;

drivers/gpu/drm/i915/intel_display.c +2 -0

+	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))

drivers/gpu/drm/i915/intel_display.c +1 -1

-		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;

drivers/gpu/drm/i915/intel_drv.h +4 -2

index d684f4f..958dc72 100644
@@ -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);

drivers/gpu/drm/i915/intel_pm.c +0 -0

index 5dbaf12..5cb537c 100644
@@ -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;

drivers/gpu/drm/i915/intel_pm.c +2 -0

+	struct intel_crtc_state *cstate;
+	struct skl_plane_wm *wm;

  enum pipe pipe;
  int level, plane;

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct
 drm_atomic_state *state)

Quoted 8 lines

Quoted 6 lines

drivers/gpu/drm/i915/intel_pm.c +0 -0

  	/* 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];

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	cstate = intel_atomic_get_crtc_state(state,

to_intel_crtc(crtc));

Wrong cstate here, should be crtc->state since it's committed and crtc_state is the old state.

Ah... How many instances of the "you're looking at the wrong state struct" have we spotted/fixed ever since the introduction of the atomic code? Can't we try to do something more human-proof in order to avoid these problems from happening again and again?

Like for example copying the old states to a specific structure like drm_atomic_state->old_states_which_you_probably_shouldn_be_using, and then setting the CRTC/connector/plane states in the drm_atomic_state struct to NULL so bugs will be much easier to spot?

Quoted 174 lines

Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:

Quoted 158 lines

Op 08-10-16 om 02:11 schreef Lyude:

Quoted 157 lines

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.

Changes since v1:
- Fixup skl_write_wm_level()
- Fixup skl_wm_level_from_reg_val()
- Don't forget to copy *active to intel_crtc->wm.active.skl

Signed-off-by Lyude <[email protected]>

Reviewed-by Maarten Lankhorst <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Paulo Zanoni <[email protected]>

---
 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      | 204 ++++++++++++-----------
------------
 drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
 5 files changed, 90 insertions(+), 144 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +0 -2

index 0287c93..76583b2 100644
@@ -1644,8 +1644,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 {

drivers/gpu/drm/i915/intel_display.c +2 -0

index a71d05a..39400a0 100644
@@ -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];

drivers/gpu/drm/i915/intel_display.c +1 -1

 	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);

drivers/gpu/drm/i915/intel_display.c +2 -0

 	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];

drivers/gpu/drm/i915/intel_display.c +0 -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.

drivers/gpu/drm/i915/intel_display.c +0 -1

 	 */
 	if (!crtc->primary->state->visible)
-		skl_write_plane_wm(intel_crtc, &dev_priv-

Quoted 1 line

wm.skl_results, 0);

drivers/gpu/drm/i915/intel_display.c +2 -0

+		skl_write_plane_wm(intel_crtc, p_wm,
+				   &dev_priv->wm.skl_results.ddb,

0);

drivers/gpu/drm/i915/intel_display.c +4 -1

 	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;

drivers/gpu/drm/i915/intel_drv.h +4 -2

index d684f4f..958dc72 100644
@@ -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);

drivers/gpu/drm/i915/intel_pm.c +2 -0

index 5dbaf12..5cb537c 100644
@@ -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;

Quoted 10 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -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... */

drivers/gpu/drm/i915/intel_pm.c +1 -0

 	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));

Wrong cstate here, should be crtc->state since it's committed and crtc_state is the old state.

Ah... How many instances of the "you're looking at the wrong state struct" have we spotted/fixed ever since the introduction of the atomic code? Can't we try to do something more human-proof in order to avoid these problems from happening again and again?

Quoted 4 lines

Like for example copying the old states to a specific structure like drm_atomic_state->old_states_which_you_probably_shouldn_be_using, and then setting the CRTC/connector/plane states in the drm_atomic_state struct to NULL so bugs will be much easier to spot?

Maarten has patches somewhere to create explicit old/new state pointers in drm_atomic_state. Not sure where exactly they're stuck. -Daniel

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 <[email protected]>

Reviewed-by Maarten Lankhorst <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_pm.c | 57
+++++++++++++++++++++++++++++++++++++++++

 1 file changed, 57 insertions(+)

drivers/gpu/drm/i915/intel_pm.c +0 -0

index 5cb537c..9e53ff7 100644
@@ -4040,6 +4040,61 @@ skl_copy_wm_for_pipe(struct skl_wm_values
 *dst,
  	       sizeof(dst->ddb.plane[pipe]));
  }

drivers/gpu/drm/i915/intel_pm.c +13 -0

+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-

Quoted 1 line

wm.skl_hw.ddb;

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	const struct skl_ddb_allocation *new_ddb = &intel_state-

Quoted 1 line

wm_results.ddb;

drivers/gpu/drm/i915/intel_pm.c +19 -0

+
+		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",

drivers/gpu/drm/i915/intel_pm.c +4 -0

+						 plane->base.id, id
+ 1,
+						 pipe_name(pipe),
+						 old->start, old-

Quoted 1 line

end,

drivers/gpu/drm/i915/intel_pm.c +1 -0

+						 new->start, new-

Quoted 1 line

end);

drivers/gpu/drm/i915/intel_pm.c +2 -0

+			} else {
+				DRM_DEBUG_ATOMIC("[PLANE:%d:cursor

%c] ddb (%d - %d) -> (%d - %d)\n",

drivers/gpu/drm/i915/intel_pm.c +3 -0

+						 plane->base.id,
+						 pipe_name(pipe),
+						 old->start, old-

Quoted 1 line

end,

drivers/gpu/drm/i915/intel_pm.c +1 -0

+						 new->start, new-

Quoted 1 line

end);

drivers/gpu/drm/i915/intel_pm.c +5 -0

+			}
+		}
+	}
+}
+

 static int
 skl_compute_wm(struct drm_atomic_state *state)
 {

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -4101,6 +4156,8 @@ skl_compute_wm(struct drm_atomic_state *state)
  		intel_cstate->update_wm_pre = true;
  	}

drivers/gpu/drm/i915/intel_pm.c +2 -0

+	skl_print_wm_changes(state);
+

  return 0;
 }

There's not much of a reason this should have the locations to read out
the hardware state hardcoded, so allow the caller to specify the location and add this function to intel_drv.h. As well, we're going to
need this function to be reusable for the next patch.

Signed-off-by Lyude <[email protected]>

Cc Maarten Lankhorst <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_drv.h |  2 ++ drivers/gpu/drm/i915/intel_pm.c  | 27 +++++++++++++++++----------

 2 files changed, 19 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_drv.h +2 -0

index 958dc72..73a2d16d 100644
@@ -1756,6 +1756,8 @@ void ilk_wm_get_hw_state(struct drm_device
 *dev);
  void skl_wm_get_hw_state(struct drm_device *dev);
  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
  			  struct skl_ddb_allocation *ddb /* out */);
+void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
+			      struct skl_pipe_wm *out);
  bool intel_can_enable_sagv(struct drm_atomic_state *state);
  int intel_enable_sagv(struct drm_i915_private *dev_priv);

 int intel_disable_sagv(struct drm_i915_private *dev_priv);

drivers/gpu/drm/i915/intel_pm.c +0 -0

index 9e53ff7..27a520ce 100644
@@ -4287,15 +4287,13 @@ static inline void
 skl_wm_level_from_reg_val(uint32_t val,
  		PLANE_WM_LINES_MASK;
  }

drivers/gpu/drm/i915/intel_pm.c +2 -1

-static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
+void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
+			      struct skl_pipe_wm *out)

 {
  struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = to_i915(dev);

drivers/gpu/drm/i915/intel_pm.c +0 -1

-	struct skl_wm_values *hw = &dev_priv->wm.skl_hw;

  struct intel_crtc *intel_crtc = to_intel_crtc(crtc);

drivers/gpu/drm/i915/intel_pm.c +0 -1

-	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc-

Quoted 1 line

state);

  struct intel_plane *intel_plane;

drivers/gpu/drm/i915/intel_pm.c +0 -1

-	struct skl_pipe_wm *active = &cstate->wm.skl.optimal;

  struct skl_plane_wm *wm;
  enum pipe pipe = intel_crtc->pipe;
  int level, id, max_level = ilk_wm_max_level(dev);

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -4303,7 +4301,7 @@ static void skl_pipe_wm_get_hw_state(struct
 drm_crtc *crtc)

  for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { id = skl_wm_plane_id(intel_plane);

drivers/gpu/drm/i915/intel_pm.c +1 -1

-		wm = &cstate->wm.skl.optimal.planes[id];
+		wm = &out->planes[id];

  for (level = 0; level <= max_level; level++) { if (id != PLANE_CURSOR)

drivers/gpu/drm/i915/intel_pm.c +0 -0

@@ -4325,20 +4323,29 @@ static void skl_pipe_wm_get_hw_state(struct
 drm_crtc *crtc)
  	if (!intel_crtc->active)
  		return;

drivers/gpu/drm/i915/intel_pm.c +1 -3

-	hw->dirty_pipes |= drm_crtc_mask(crtc);
-	active->linetime = I915_READ(PIPE_WM_LINETIME(pipe));
-	intel_crtc->wm.active.skl = *active;
+	out->linetime = I915_READ(PIPE_WM_LINETIME(pipe));

 }

 void skl_wm_get_hw_state(struct drm_device *dev) {
  struct drm_i915_private *dev_priv = to_i915(dev);

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	struct skl_wm_values *hw = &dev_priv->wm.skl_hw;

  struct skl_ddb_allocation *ddb = &dev_priv->wm.skl_hw.ddb; struct drm_crtc *crtc;

drivers/gpu/drm/i915/intel_pm.c +2 -0

+	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *cstate;

  skl_ddb_get_hw_state(dev_priv, ddb);

drivers/gpu/drm/i915/intel_pm.c +1 -2

-	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
-		skl_pipe_wm_get_hw_state(crtc);
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)

{

drivers/gpu/drm/i915/intel_pm.c +4 -0

+		intel_crtc = to_intel_crtc(crtc);
+		cstate = to_intel_crtc_state(crtc->state);
+
+		skl_pipe_wm_get_hw_state(crtc, &cstate-

Quoted 1 line

wm.skl.optimal);

wm.active.skl. Previously we would only set it if intel_crtc->active

active. Now we're doing the opposite. Didn't you mean "if (intel_crtc- active)" here?

drivers/gpu/drm/i915/intel_pm.c +1 -0

+	}

  if (dev_priv->active_crtcs) {
  /* Fully recompute DDB on first atomic commit */

Helper we're going to be using for implementing verification of the wm
levels in skl_verify_wm_level().

Signed-off-by Lyude <[email protected]>

Cc Maarten Lankhorst <[email protected]>

Cc Ville Syrjälä <[email protected]>

Cc Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_drv.h |  2 ++ drivers/gpu/drm/i915/intel_pm.c  | 14 ++++++++++++++

 2 files changed, 16 insertions(+)

drivers/gpu/drm/i915/intel_drv.h +2 -0

index 73a2d16d..3e6e9af 100644
@@ -1761,6 +1761,8 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc
 *crtc,
  bool intel_can_enable_sagv(struct drm_atomic_state *state);
  int intel_enable_sagv(struct drm_i915_private *dev_priv);
  int intel_disable_sagv(struct drm_i915_private *dev_priv);
+bool skl_wm_level_equals(const struct skl_wm_level *l1,
+			 const struct skl_wm_level *l2);
  bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
  			       const struct skl_ddb_allocation *new,

         enum pipe pipe);

drivers/gpu/drm/i915/intel_pm.c +0 -0

index 27a520ce..6af1587 100644
@@ -3853,6 +3853,20 @@ void skl_write_cursor_wm(struct intel_crtc
 *intel_crtc,
  			    &ddb->plane[pipe][PLANE_CURSOR]);
  }

drivers/gpu/drm/i915/intel_pm.c +7 -0

+bool skl_wm_level_equals(const struct skl_wm_level *l1,
+			 const struct skl_wm_level *l2)
+{
+	if (l1->plane_en != l2->plane_en)
+		return false;
+
+	/* If both planes aren't enabled, the rest shouldn't matter

*/

drivers/gpu/drm/i915/intel_pm.c +7 -0

+	if (!l1->plane_en)
+		return true;
+
+	return (l1->plane_res_l == l2->plane_res_l &&
+		l1->plane_res_b == l2->plane_res_b);
+}
+

 static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
     const struct skl_ddb_entry *b)
 {