[PATCH v2 00/10] Start of skl watermark cleanup

[PATCH v2 00/10] 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.

Lyude (10):
  drm/i915/skl: Move per-pipe ddb allocations into crtc states
  drm/i915/skl: Remove linetime from skl_wm_values
  drm/i915/gen9: Make skl_wm_level per-plane
  drm/i915/gen9: Cleanup skl_pipe_wm_active_state
  drm/i915/gen9: Get rid of redundant watermark values
  drm/i915/gen9: Add ddb changes to atomic debug output
  drm/i915/gen9: Make skl_pipe_wm_get_hw_state() reusable
  drm/i915/gen9: Add skl_wm_level_equals()
  drm/i915/gen9: Actually verify WM levels in verify_wm_state()
  drm/i915/gen9: Don't wrap strings in verify_wm_state()

 drivers/gpu/drm/i915/i915_drv.h      |  10 +-
 drivers/gpu/drm/i915/intel_display.c | 138 +++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  24 +-
 drivers/gpu/drm/i915/intel_pm.c      | 470 +++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
 5 files changed, 357 insertions(+), 293 deletions(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 01/10] 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.

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

Signed-off-by: Lyude <cpaul@redhat.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@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 a219a35..bb2de8d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1637,7 +1637,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 fe6c1c6..68355b0 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;
@@ -3370,7 +3369,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;
+		alloc->start = alloc->end = 0;
 		return 0;
 	}
 
@@ -3897,14 +3896,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)
 {
@@ -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,
-				 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;
 	}
 
@@ -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]));
 
-	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],
@@ -4200,6 +4190,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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 01/10] drm/i915/skl: Move per-pipe ddb allocations into crtc states

[Paulo Zanoni]

Em Sex, 2016-10-07 às 20:11 -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.
> 
> Changes since v1:
> - Don't replace alloc->start = alloc->end = 0;
> 
> Signed-off-by: Lyude <cpaul@redhat.com>

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

> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@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 a219a35..bb2de8d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1637,7 +1637,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 fe6c1c6..68355b0 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;
> @@ -3370,7 +3369,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;
> +		alloc->start = alloc->end = 0;
>  		return 0;
>  	}
>  
> @@ -3897,14 +3896,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)
>  {
> @@ -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,
> -				 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;
>  	}
>  
> @@ -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]));
>  
> -	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],
> @@ -4200,6 +4190,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);
>  }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 02/10] 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>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@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 bb2de8d..e9d035ea 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1644,7 +1644,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 68355b0..cc5d5e9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3841,8 +3841,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,
@@ -4077,7 +4075,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],
@@ -4328,8 +4325,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] =
@@ -4346,7 +4341,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

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

[PATCH v2 03/10] 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.

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 <cpaul@redhat.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@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  | 207 +++++++++++++++++++--------------------
 3 files changed, 111 insertions(+), 108 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e9d035ea..0287c93 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1649,9 +1649,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 cc5d5e9..4c2ebcd 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -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->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));
+	if (!intel_pstate)
+		intel_pstate = to_intel_plane_state(plane->state);
 
-	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);
+	WARN_ON(!intel_pstate->base.fb);
 
-		/*
-		 * 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);
+	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
 
-		WARN_ON(!intel_pstate->base.fb);
-
-		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;
 }
@@ -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 */)
 {
-	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,
@@ -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;
 
-	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;
 }
 
@@ -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;
 
-	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;
@@ -4278,35 +4275,37 @@ static void skl_pipe_wm_active_state(uint32_t val,
 
 	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;
+			active->planes[i].wm[level].plane_en = is_enabled;
+			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->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;
+			active->planes[PLANE_CURSOR].wm[level].plane_en =
+				is_enabled;
+			active->planes[PLANE_CURSOR].wm[level].plane_res_b =
+				val & PLANE_WM_BLOCKS_MASK;
+			active->planes[PLANE_CURSOR].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;
+			active->planes[i].trans_wm.plane_en = is_enabled;
+			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->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;
+			active->planes[PLANE_CURSOR].trans_wm.plane_en =
+				is_enabled;
+			active->planes[PLANE_CURSOR].trans_wm.plane_res_b =
+				val & PLANE_WM_BLOCKS_MASK;
+			active->planes[PLANE_CURSOR].trans_wm.plane_res_l =
+				(val >> PLANE_WM_LINES_SHIFT) &
+				PLANE_WM_LINES_MASK;
 		}
 	}
 }
-- 
2.7.4

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

Re: [PATCH v2 03/10] drm/i915/gen9: Make skl_wm_level per-plane

[Paulo Zanoni]

Em Sex, 2016-10-07 às 20:11 -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.
> 
> 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 <cpaul@redhat.com>

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

> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@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  | 207 +++++++++++++++++++--------
> ------------
>  3 files changed, 111 insertions(+), 108 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index e9d035ea..0287c93 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1649,9 +1649,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 cc5d5e9..4c2ebcd 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -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-
> >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));
> +	if (!intel_pstate)
> +		intel_pstate = to_intel_plane_state(plane->state);
>  
> -	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);
> +	WARN_ON(!intel_pstate->base.fb);
>  
> -		/*
> -		 * 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);
> +	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
>  
> -		WARN_ON(!intel_pstate->base.fb);
> -
> -		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;
>  }
> @@ -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 */)
>  {
> -	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,
> @@ -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;
>  
> -	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;
>  }
>  
> @@ -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;
>  
> -	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;
> @@ -4278,35 +4275,37 @@ static void skl_pipe_wm_active_state(uint32_t
> val,
>  
>  	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;
> +			active->planes[i].wm[level].plane_en =
> is_enabled;
> +			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->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;
> +			active-
> >planes[PLANE_CURSOR].wm[level].plane_en =
> +				is_enabled;
> +			active-
> >planes[PLANE_CURSOR].wm[level].plane_res_b =
> +				val & PLANE_WM_BLOCKS_MASK;
> +			active-
> >planes[PLANE_CURSOR].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;
> +			active->planes[i].trans_wm.plane_en =
> is_enabled;
> +			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->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;
> +			active-
> >planes[PLANE_CURSOR].trans_wm.plane_en =
> +				is_enabled;
> +			active-
> >planes[PLANE_CURSOR].trans_wm.plane_res_b =
> +				val & PLANE_WM_BLOCKS_MASK;
> +			active-
> >planes[PLANE_CURSOR].trans_wm.plane_res_l =
> +				(val >> PLANE_WM_LINES_SHIFT) &
> +				PLANE_WM_LINES_MASK;
>  		}
>  	}
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/10] drm/i915/gen9: Cleanup skl_pipe_wm_active_state

[Lyude]

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 <cpaul@redhat.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 52 +++++++++++------------------------------
 1 file changed, 14 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4c2ebcd..5dbaf12 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -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) {
-		if (!is_cursor) {
-			active->planes[i].wm[level].plane_en = is_enabled;
-			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->planes[PLANE_CURSOR].wm[level].plane_en =
-				is_enabled;
-			active->planes[PLANE_CURSOR].wm[level].plane_res_b =
-				val & PLANE_WM_BLOCKS_MASK;
-			active->planes[PLANE_CURSOR].wm[level].plane_res_l =
-				(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->planes[i].trans_wm.plane_en = is_enabled;
-			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->planes[PLANE_CURSOR].trans_wm.plane_en =
-				is_enabled;
-			active->planes[PLANE_CURSOR].trans_wm.plane_res_b =
-				val & PLANE_WM_BLOCKS_MASK;
-			active->planes[PLANE_CURSOR].trans_wm.plane_res_l =
-				(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;
 	}
 }
 
@@ -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];
-		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, PLANE_CURSOR, 0);
 
 	intel_crtc->wm.active.skl = *active;
 }
-- 
2.7.4

Re: [PATCH 04/10] drm/i915/gen9: Cleanup skl_pipe_wm_active_state

[Paulo Zanoni]

Em Sex, 2016-10-07 às 20:11 -0400, Lyude escreveu:
> This function is a wreck, let's help it get it's life back together
> and
> cleanup all of the copy pasta here.

s/it's/its/

Idea for your next patch series: rename skl_pipe_wm_active_state()'s
"i" parameter to something more meaningful.

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

> 
> (adding Maarten's reviewed-by since this is just a split-up version
> of one
> of the previous patches)
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 52 +++++++++++------------------
> ------------
>  1 file changed, 14 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 4c2ebcd..5dbaf12 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -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) {
> -		if (!is_cursor) {
> -			active->planes[i].wm[level].plane_en =
> is_enabled;
> -			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-
> >planes[PLANE_CURSOR].wm[level].plane_en =
> -				is_enabled;
> -			active-
> >planes[PLANE_CURSOR].wm[level].plane_res_b =
> -				val & PLANE_WM_BLOCKS_MASK;
> -			active-
> >planes[PLANE_CURSOR].wm[level].plane_res_l =
> -				(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->planes[i].trans_wm.plane_en =
> is_enabled;
> -			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-
> >planes[PLANE_CURSOR].trans_wm.plane_en =
> -				is_enabled;
> -			active-
> >planes[PLANE_CURSOR].trans_wm.plane_res_b =
> -				val & PLANE_WM_BLOCKS_MASK;
> -			active-
> >planes[PLANE_CURSOR].trans_wm.plane_res_l =
> -				(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;
>  	}
>  }
>  
> @@ -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];
> -		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, PLANE_CURSOR,
> 0);
>  
>  	intel_crtc->wm.active.skl = *active;
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 05/10] 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.

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 <cpaul@redhat.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@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      | 204 ++++++++++++-----------------------
 drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
 5 files changed, 90 insertions(+), 144 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0287c93..76583b2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -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 {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a71d05a..39400a0 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 5dbaf12..5cb537c 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)
 		     { }
 
 		/*
@@ -3779,67 +3784,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)
@@ -3850,8 +3794,24 @@ 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;
+
+	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;
@@ -3861,19 +3821,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;
@@ -3882,13 +3844,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,
@@ -4072,11 +4034,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],
@@ -4125,7 +4082,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);
 
@@ -4143,7 +4099,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;
@@ -4177,9 +4132,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);
@@ -4264,26 +4221,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_SHIFT) &
+		PLANE_WM_LINES_MASK;
 }
 
 static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
@@ -4293,49 +4237,39 @@ 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));
 
-	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));
+			skl_wm_level_from_reg_val(val, &wm->wm[level]);
+		}
+
+		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;
 
 	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, PLANE_CURSOR,
-					 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, PLANE_CURSOR, 0);
-
 	intel_crtc->wm.active.skl = *active;
 }
 
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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values

[Maarten Lankhorst]

Op 08-10-16 om 02:11 schreef 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.
>
> 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 <cpaul@redhat.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@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      | 204 ++++++++++++-----------------------
>  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
>  5 files changed, 90 insertions(+), 144 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0287c93..76583b2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -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 {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a71d05a..39400a0 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 5dbaf12..5cb537c 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));
Wrong cstate here, should be crtc->state since it's committed and crtc_state is the old state.

~Maarten

Re: [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values

[Paulo Zanoni]

Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:
> Op 08-10-16 om 02:11 schreef 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.
> > 
> > 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 <cpaul@redhat.com>
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@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      | 204 ++++++++++++-----------
> > ------------
> >  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
> >  5 files changed, 90 insertions(+), 144 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 0287c93..76583b2 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -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 {
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index a71d05a..39400a0 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 5dbaf12..5cb537c 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));
> 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 <paulo.r.zanoni@intel.com>

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

Re: [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values

[Paulo Zanoni]

Em Qui, 2016-10-13 às 17:04 -0300, Paulo Zanoni escreveu:
> Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:
> > 
> > Op 08-10-16 om 02:11 schreef 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.
> > > 
> > > 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 <cpaul@redhat.com>
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com
> > > >
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@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      | 204 ++++++++++++---------
> > > --
> > > ------------
> > >  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
> > >  5 files changed, 90 insertions(+), 144 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 0287c93..76583b2 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -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 {
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index a71d05a..39400a0 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 5dbaf12..5cb537c 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));
> > 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 <paulo.r.zanoni@intel.com>

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.

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

Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values

[Lyude Paul]

Your is SAGV related, and when we don't make the SAGV happy people's
machines usually hang. So I'm definitely for your patches getting
merged first


On Thu, 2016-10-13 at 17:07 -0300, Paulo Zanoni wrote:
> Em Qui, 2016-10-13 às 17:04 -0300, Paulo Zanoni escreveu:
> > 
> > Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:
> > > 
> > > 
> > > Op 08-10-16 om 02:11 schreef 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.
> > > > 
> > > > 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 <cpaul@redhat.com>
> > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c
> > > > om
> > > > > 
> > > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@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      | 204 ++++++++++++-------
> > > > --
> > > > --
> > > > ------------
> > > >  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
> > > >  5 files changed, 90 insertions(+), 144 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 0287c93..76583b2 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -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 {
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index a71d05a..39400a0 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 5dbaf12..5cb537c 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));
> > > 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 <paulo.r.zanoni@intel.com>
> 
> 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.
> 
> > 
> > 
> > > 
> > > 
> > > 
> > > ~Maarten
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values

[Daniel Vetter]

On Thu, Oct 13, 2016 at 05:04:03PM -0300, Paulo Zanoni wrote:
> Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:
> > Op 08-10-16 om 02:11 schreef 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.
> > > 
> > > 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 <cpaul@redhat.com>
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@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      | 204 ++++++++++++-----------
> > > ------------
> > >  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
> > >  5 files changed, 90 insertions(+), 144 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 0287c93..76583b2 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -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 {
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index a71d05a..39400a0 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 5dbaf12..5cb537c 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));
> > 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?

Maarten has patches somewhere to create explicit old/new state pointers in
drm_atomic_state. Not sure where exactly they're stuck.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values

[Maarten Lankhorst]

Op 17-10-16 om 08:05 schreef Daniel Vetter:
> On Thu, Oct 13, 2016 at 05:04:03PM -0300, Paulo Zanoni wrote:
>> Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu:
>>> Op 08-10-16 om 02:11 schreef 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.
>>>>
>>>> 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 <cpaul@redhat.com>
>>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Cc: Paulo Zanoni <paulo.r.zanoni@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      | 204 ++++++++++++-----------
>>>> ------------
>>>>  drivers/gpu/drm/i915/intel_sprite.c  |   8 +-
>>>>  5 files changed, 90 insertions(+), 144 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>>> b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 0287c93..76583b2 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -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 {
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>>> b/drivers/gpu/drm/i915/intel_display.c
>>>> index a71d05a..39400a0 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 5dbaf12..5cb537c 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));
>>> 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?
> Maarten has patches somewhere to create explicit old/new state pointers in
> drm_atomic_state. Not sure where exactly they're stuck.
> -Daniel

Waiting for drm-misc merge window to reopen, but making things more explicit will help a lot.

~Maarten

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

[PATCH v2 06/10] 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>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@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 5cb537c..9e53ff7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4040,6 +4040,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)
 {
@@ -4101,6 +4156,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

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

Re: [PATCH v2 06/10] drm/i915/gen9: Add ddb changes to atomic debug output

[Paulo Zanoni]

Em Sex, 2016-10-07 às 20:11 -0400, Lyude escreveu:
> 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>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@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 5cb537c..9e53ff7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4040,6 +4040,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;

Why exactly do we have this check?

Everything else looks good. So with either an explanation or the check
removed in case it's not needed:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> +
> +		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)
>  {
> @@ -4101,6 +4156,8 @@ skl_compute_wm(struct drm_atomic_state *state)
>  		intel_cstate->update_wm_pre = true;
>  	}
>  
> +	skl_print_wm_changes(state);
> +
>  	return 0;
>  }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/10] drm/i915/gen9: Make skl_pipe_wm_get_hw_state() reusable

[Lyude]

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 <cpaul@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 drivers/gpu/drm/i915/intel_pm.c  | 27 +++++++++++++++++----------
 2 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 958dc72..73a2d16d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9e53ff7..27a520ce 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4287,15 +4287,13 @@ static inline void skl_wm_level_from_reg_val(uint32_t val,
 		PLANE_WM_LINES_MASK;
 }
 
-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);
-	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, id, max_level = ilk_wm_max_level(dev);
@@ -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);
-		wm = &cstate->wm.skl.optimal.planes[id];
+		wm = &out->planes[id];
 
 		for (level = 0; level <= max_level; level++) {
 			if (id != PLANE_CURSOR)
@@ -4325,20 +4323,29 @@ static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc)
 	if (!intel_crtc->active)
 		return;
 
-	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);
+	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;
+	struct intel_crtc *intel_crtc;
+	struct intel_crtc_state *cstate;
 
 	skl_ddb_get_hw_state(dev_priv, ddb);
-	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) {
+		intel_crtc = to_intel_crtc(crtc);
+		cstate = to_intel_crtc_state(crtc->state);
+
+		skl_pipe_wm_get_hw_state(crtc, &cstate->wm.skl.optimal);
+		intel_crtc->wm.active.skl = cstate->wm.skl.optimal;
+
+		if (!intel_crtc->active)
+			hw->dirty_pipes |= drm_crtc_mask(crtc);
+	}
 
 	if (dev_priv->active_crtcs) {
 		/* Fully recompute DDB on first atomic commit */
-- 
2.7.4

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

Re: [PATCH 07/10] drm/i915/gen9: Make skl_pipe_wm_get_hw_state() reusable

[Paulo Zanoni]

Em Sex, 2016-10-07 às 20:11 -0400, Lyude escreveu:
> 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 <cpaul@redhat.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c  | 27 +++++++++++++++++----------
>  2 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 958dc72..73a2d16d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 9e53ff7..27a520ce 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4287,15 +4287,13 @@ static inline void
> skl_wm_level_from_reg_val(uint32_t val,
>  		PLANE_WM_LINES_MASK;
>  }
>  
> -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);
> -	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, id, max_level = ilk_wm_max_level(dev);
> @@ -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);
> -		wm = &cstate->wm.skl.optimal.planes[id];
> +		wm = &out->planes[id];
>  
>  		for (level = 0; level <= max_level; level++) {
>  			if (id != PLANE_CURSOR)
> @@ -4325,20 +4323,29 @@ static void skl_pipe_wm_get_hw_state(struct
> drm_crtc *crtc)
>  	if (!intel_crtc->active)
>  		return;
>  
> -	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);
> +	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;
> +	struct intel_crtc *intel_crtc;
> +	struct intel_crtc_state *cstate;
>  
>  	skl_ddb_get_hw_state(dev_priv, ddb);
> -	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)
> {
> +		intel_crtc = to_intel_crtc(crtc);
> +		cstate = to_intel_crtc_state(crtc->state);
> +
> +		skl_pipe_wm_get_hw_state(crtc, &cstate-
> >wm.skl.optimal);
> +		intel_crtc->wm.active.skl = cstate->wm.skl.optimal;

We're changing how the code behaves regarding intel_crtc-
>wm.active.skl. Previously we would only set it if intel_crtc->active
is true due to that return in skl_pipe_wm_get_hw_state(). Now we're
always setting it.

If this is some sort of fix it probably deserves to be in a separate
commit with a nice commit message.

> +
> +		if (!intel_crtc->active)
> +			hw->dirty_pipes |= drm_crtc_mask(crtc);

Same here: previously we would not set dirty_pipes in case !intel_crtc-
>active. Now we're doing the opposite. Didn't you mean "if (intel_crtc-
>active)" here?

> +	}
>  
>  	if (dev_priv->active_crtcs) {
>  		/* Fully recompute DDB on first atomic commit */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/10] drm/i915/gen9: Add skl_wm_level_equals()

[Lyude]

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

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: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 drivers/gpu/drm/i915/intel_pm.c  | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 73a2d16d..3e6e9af 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 27a520ce..6af1587 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3853,6 +3853,20 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
 			    &ddb->plane[pipe][PLANE_CURSOR]);
 }
 
+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 */
+	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)
 {
-- 
2.7.4

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

Re: [PATCH 08/10] drm/i915/gen9: Add skl_wm_level_equals()

[Paulo Zanoni]

Em Sex, 2016-10-07 às 20:11 -0400, Lyude escreveu:
> Helper we're going to be using for implementing verification of the
> wm
> levels in skl_verify_wm_level().
> 
> Signed-off-by: Lyude <cpaul@redhat.com>

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

> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c  | 14 ++++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 73a2d16d..3e6e9af 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 27a520ce..6af1587 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3853,6 +3853,20 @@ void skl_write_cursor_wm(struct intel_crtc
> *intel_crtc,
>  			    &ddb->plane[pipe][PLANE_CURSOR]);
>  }
>  
> +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
> */
> +	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)
>  {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/10] drm/i915/gen9: Actually verify WM levels in verify_wm_state()

[Lyude]

Thanks to Paulo Zanoni for indirectly pointing this out.

Looks like we never actually added any code for checking whether or not
we actually wrote watermark levels properly. Let's fix that.

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: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 100 +++++++++++++++++++++++++++++------
 1 file changed, 84 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 39400a0..2c682bc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13444,30 +13444,66 @@ static void verify_wm_state(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct skl_ddb_allocation hw_ddb, *sw_ddb;
-	struct skl_ddb_entry *hw_entry, *sw_entry;
+	struct skl_pipe_wm hw_wm, *sw_wm;
+	struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
+	struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	const enum pipe pipe = intel_crtc->pipe;
-	int plane;
+	int plane, level, max_level = ilk_wm_max_level(dev);
 
 	if (INTEL_INFO(dev)->gen < 9 || !new_state->active)
 		return;
 
+	skl_pipe_wm_get_hw_state(crtc, &hw_wm);
+	sw_wm = &intel_crtc->wm.active.skl;
+
 	skl_ddb_get_hw_state(dev_priv, &hw_ddb);
 	sw_ddb = &dev_priv->wm.skl_hw.ddb;
 
 	/* planes */
 	for_each_plane(dev_priv, pipe, plane) {
-		hw_entry = &hw_ddb.plane[pipe][plane];
-		sw_entry = &sw_ddb->plane[pipe][plane];
+		hw_plane_wm = &hw_wm.planes[plane];
+		sw_plane_wm = &sw_wm->planes[plane];
 
-		if (skl_ddb_entry_equal(hw_entry, sw_entry))
-			continue;
+		/* Watermarks */
+		for (level = 0; level <= max_level; level++) {
+			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
+						&sw_plane_wm->wm[level]))
+				continue;
+
+			DRM_ERROR("mismatch in WM pipe %c plane %d level %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n",
+				  pipe_name(pipe), plane + 1, level,
+				  sw_plane_wm->wm[level].plane_en,
+				  sw_plane_wm->wm[level].plane_res_b,
+				  sw_plane_wm->wm[level].plane_res_l,
+				  hw_plane_wm->wm[level].plane_en,
+				  hw_plane_wm->wm[level].plane_res_b,
+				  hw_plane_wm->wm[level].plane_res_l);
+		}
 
-		DRM_ERROR("mismatch in DDB state pipe %c plane %d "
-			  "(expected (%u,%u), found (%u,%u))\n",
-			  pipe_name(pipe), plane + 1,
-			  sw_entry->start, sw_entry->end,
-			  hw_entry->start, hw_entry->end);
+		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
+					 &sw_plane_wm->trans_wm)) {
+			DRM_ERROR("mismatch in trans WM pipe %c plane %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n",
+				  pipe_name(pipe), plane + 1,
+				  sw_plane_wm->trans_wm.plane_en,
+				  sw_plane_wm->trans_wm.plane_res_b,
+				  sw_plane_wm->trans_wm.plane_res_l,
+				  hw_plane_wm->trans_wm.plane_en,
+				  hw_plane_wm->trans_wm.plane_res_b,
+				  hw_plane_wm->trans_wm.plane_res_l);
+		}
+
+		/* DDB */
+		hw_ddb_entry = &hw_ddb.plane[pipe][plane];
+		sw_ddb_entry = &sw_ddb->plane[pipe][plane];
+
+		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
+			DRM_ERROR("mismatch in DDB state pipe %c plane %d "
+				  "(expected (%u,%u), found (%u,%u))\n",
+				  pipe_name(pipe), plane + 1,
+				  sw_ddb_entry->start, sw_ddb_entry->end,
+				  hw_ddb_entry->start, hw_ddb_entry->end);
+		}
 	}
 
 	/*
@@ -13477,15 +13513,47 @@ static void verify_wm_state(struct drm_crtc *crtc,
 	 * once the plane becomes visible, we can skip this check
 	 */
 	if (intel_crtc->cursor_addr) {
-		hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
-		sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
+		hw_plane_wm = &hw_wm.planes[PLANE_CURSOR];
+		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
+
+		/* Watermarks */
+		for (level = 0; level <= max_level; level++) {
+			if (skl_wm_level_equals(&hw_plane_wm->wm[level],
+						&sw_plane_wm->wm[level]))
+				continue;
+
+			DRM_ERROR("mismatch in WM pipe %c cursor level %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n",
+				  pipe_name(pipe), level,
+				  sw_plane_wm->wm[level].plane_en,
+				  sw_plane_wm->wm[level].plane_res_b,
+				  sw_plane_wm->wm[level].plane_res_l,
+				  hw_plane_wm->wm[level].plane_en,
+				  hw_plane_wm->wm[level].plane_res_b,
+				  hw_plane_wm->wm[level].plane_res_l);
+		}
+
+		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
+					 &sw_plane_wm->trans_wm)) {
+			DRM_ERROR("mismatch in trans WM pipe %c cursor (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n",
+				  pipe_name(pipe),
+				  sw_plane_wm->trans_wm.plane_en,
+				  sw_plane_wm->trans_wm.plane_res_b,
+				  sw_plane_wm->trans_wm.plane_res_l,
+				  hw_plane_wm->trans_wm.plane_en,
+				  hw_plane_wm->trans_wm.plane_res_b,
+				  hw_plane_wm->trans_wm.plane_res_l);
+		}
+
+		/* DDB */
+		hw_ddb_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
+		sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
 
-		if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
+		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
 			DRM_ERROR("mismatch in DDB state pipe %c cursor "
 				  "(expected (%u,%u), found (%u,%u))\n",
 				  pipe_name(pipe),
-				  sw_entry->start, sw_entry->end,
-				  hw_entry->start, hw_entry->end);
+				  sw_ddb_entry->start, sw_ddb_entry->end,
+				  hw_ddb_entry->start, hw_ddb_entry->end);
 		}
 	}
 }
-- 
2.7.4

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

Re: [PATCH 09/10] drm/i915/gen9: Actually verify WM levels in verify_wm_state()

[Paulo Zanoni]

Em Sex, 2016-10-07 às 20:11 -0400, Lyude escreveu:
> Thanks to Paulo Zanoni for indirectly pointing this out.
> 
> Looks like we never actually added any code for checking whether or
> not
> we actually wrote watermark levels properly. Let's fix that.

Thanks for doing this!

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

A check that would have prevented one of the bugs I solved would be "if
plane is active, then level 0 must be enabled, and DDB partitioning
size must be non-zero". I'll put this in my TODO list, but I won't
complain if somebody does it first :)

> 
> 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: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 100
> +++++++++++++++++++++++++++++------
>  1 file changed, 84 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 39400a0..2c682bc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13444,30 +13444,66 @@ static void verify_wm_state(struct drm_crtc
> *crtc,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct skl_ddb_allocation hw_ddb, *sw_ddb;
> -	struct skl_ddb_entry *hw_entry, *sw_entry;
> +	struct skl_pipe_wm hw_wm, *sw_wm;
> +	struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
> +	struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	const enum pipe pipe = intel_crtc->pipe;
> -	int plane;
> +	int plane, level, max_level = ilk_wm_max_level(dev);
>  
>  	if (INTEL_INFO(dev)->gen < 9 || !new_state->active)
>  		return;
>  
> +	skl_pipe_wm_get_hw_state(crtc, &hw_wm);
> +	sw_wm = &intel_crtc->wm.active.skl;
> +
>  	skl_ddb_get_hw_state(dev_priv, &hw_ddb);
>  	sw_ddb = &dev_priv->wm.skl_hw.ddb;
>  
>  	/* planes */
>  	for_each_plane(dev_priv, pipe, plane) {
> -		hw_entry = &hw_ddb.plane[pipe][plane];
> -		sw_entry = &sw_ddb->plane[pipe][plane];
> +		hw_plane_wm = &hw_wm.planes[plane];
> +		sw_plane_wm = &sw_wm->planes[plane];
>  
> -		if (skl_ddb_entry_equal(hw_entry, sw_entry))
> -			continue;
> +		/* Watermarks */
> +		for (level = 0; level <= max_level; level++) {
> +			if (skl_wm_level_equals(&hw_plane_wm-
> >wm[level],
> +						&sw_plane_wm-
> >wm[level]))
> +				continue;
> +
> +			DRM_ERROR("mismatch in WM pipe %c plane %d
> level %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n",
> +				  pipe_name(pipe), plane + 1, level,
> +				  sw_plane_wm->wm[level].plane_en,
> +				  sw_plane_wm-
> >wm[level].plane_res_b,
> +				  sw_plane_wm-
> >wm[level].plane_res_l,
> +				  hw_plane_wm->wm[level].plane_en,
> +				  hw_plane_wm-
> >wm[level].plane_res_b,
> +				  hw_plane_wm-
> >wm[level].plane_res_l);
> +		}
>  
> -		DRM_ERROR("mismatch in DDB state pipe %c plane %d "
> -			  "(expected (%u,%u), found (%u,%u))\n",
> -			  pipe_name(pipe), plane + 1,
> -			  sw_entry->start, sw_entry->end,
> -			  hw_entry->start, hw_entry->end);
> +		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> +					 &sw_plane_wm->trans_wm)) {
> +			DRM_ERROR("mismatch in trans WM pipe %c
> plane %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n",
> +				  pipe_name(pipe), plane + 1,
> +				  sw_plane_wm->trans_wm.plane_en,
> +				  sw_plane_wm->trans_wm.plane_res_b,
> +				  sw_plane_wm->trans_wm.plane_res_l,
> +				  hw_plane_wm->trans_wm.plane_en,
> +				  hw_plane_wm->trans_wm.plane_res_b,
> +				  hw_plane_wm-
> >trans_wm.plane_res_l);
> +		}
> +
> +		/* DDB */
> +		hw_ddb_entry = &hw_ddb.plane[pipe][plane];
> +		sw_ddb_entry = &sw_ddb->plane[pipe][plane];
> +
> +		if (!skl_ddb_entry_equal(hw_ddb_entry,
> sw_ddb_entry)) {
> +			DRM_ERROR("mismatch in DDB state pipe %c
> plane %d "
> +				  "(expected (%u,%u), found
> (%u,%u))\n",
> +				  pipe_name(pipe), plane + 1,
> +				  sw_ddb_entry->start, sw_ddb_entry-
> >end,
> +				  hw_ddb_entry->start, hw_ddb_entry-
> >end);
> +		}
>  	}
>  
>  	/*
> @@ -13477,15 +13513,47 @@ static void verify_wm_state(struct drm_crtc
> *crtc,
>  	 * once the plane becomes visible, we can skip this check
>  	 */
>  	if (intel_crtc->cursor_addr) {
> -		hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> -		sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
> +		hw_plane_wm = &hw_wm.planes[PLANE_CURSOR];
> +		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
> +
> +		/* Watermarks */
> +		for (level = 0; level <= max_level; level++) {
> +			if (skl_wm_level_equals(&hw_plane_wm-
> >wm[level],
> +						&sw_plane_wm-
> >wm[level]))
> +				continue;
> +
> +			DRM_ERROR("mismatch in WM pipe %c cursor
> level %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n",
> +				  pipe_name(pipe), level,
> +				  sw_plane_wm->wm[level].plane_en,
> +				  sw_plane_wm-
> >wm[level].plane_res_b,
> +				  sw_plane_wm-
> >wm[level].plane_res_l,
> +				  hw_plane_wm->wm[level].plane_en,
> +				  hw_plane_wm-
> >wm[level].plane_res_b,
> +				  hw_plane_wm-
> >wm[level].plane_res_l);
> +		}
> +
> +		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> +					 &sw_plane_wm->trans_wm)) {
> +			DRM_ERROR("mismatch in trans WM pipe %c
> cursor (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n",
> +				  pipe_name(pipe),
> +				  sw_plane_wm->trans_wm.plane_en,
> +				  sw_plane_wm->trans_wm.plane_res_b,
> +				  sw_plane_wm->trans_wm.plane_res_l,
> +				  hw_plane_wm->trans_wm.plane_en,
> +				  hw_plane_wm->trans_wm.plane_res_b,
> +				  hw_plane_wm-
> >trans_wm.plane_res_l);
> +		}
> +
> +		/* DDB */
> +		hw_ddb_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> +		sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
>  
> -		if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
> +		if (!skl_ddb_entry_equal(hw_ddb_entry,
> sw_ddb_entry)) {
>  			DRM_ERROR("mismatch in DDB state pipe %c
> cursor "
>  				  "(expected (%u,%u), found
> (%u,%u))\n",
>  				  pipe_name(pipe),
> -				  sw_entry->start, sw_entry->end,
> -				  hw_entry->start, hw_entry->end);
> +				  sw_ddb_entry->start, sw_ddb_entry-
> >end,
> +				  hw_ddb_entry->start, hw_ddb_entry-
> >end);
>  		}
>  	}
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/10] drm/i915/gen9: Actually verify WM levels in verify_wm_state()

[Paulo Zanoni]

Em Qui, 2016-10-13 às 18:15 -0300, Paulo Zanoni escreveu:
> Em Sex, 2016-10-07 às 20:11 -0400, Lyude escreveu:
> > 
> > Thanks to Paulo Zanoni for indirectly pointing this out.
> > 
> > Looks like we never actually added any code for checking whether or
> > not
> > we actually wrote watermark levels properly. Let's fix that.
> 
> Thanks for doing this!
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> A check that would have prevented one of the bugs I solved would be
> "if
> plane is active, then level 0 must be enabled, and DDB partitioning
> size must be non-zero". I'll put this in my TODO list, but I won't
> complain if somebody does it first :)

Also, bikeshed: use %u instead of %d for the unsigned types
(plane_res_b, plane_res_l).

> 
> > 
> > 
> > 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: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 100
> > +++++++++++++++++++++++++++++------
> >  1 file changed, 84 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 39400a0..2c682bc 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13444,30 +13444,66 @@ static void verify_wm_state(struct
> > drm_crtc
> > *crtc,
> >  	struct drm_device *dev = crtc->dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct skl_ddb_allocation hw_ddb, *sw_ddb;
> > -	struct skl_ddb_entry *hw_entry, *sw_entry;
> > +	struct skl_pipe_wm hw_wm, *sw_wm;
> > +	struct skl_plane_wm *hw_plane_wm, *sw_plane_wm;
> > +	struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	const enum pipe pipe = intel_crtc->pipe;
> > -	int plane;
> > +	int plane, level, max_level = ilk_wm_max_level(dev);
> >  
> >  	if (INTEL_INFO(dev)->gen < 9 || !new_state->active)
> >  		return;
> >  
> > +	skl_pipe_wm_get_hw_state(crtc, &hw_wm);
> > +	sw_wm = &intel_crtc->wm.active.skl;
> > +
> >  	skl_ddb_get_hw_state(dev_priv, &hw_ddb);
> >  	sw_ddb = &dev_priv->wm.skl_hw.ddb;
> >  
> >  	/* planes */
> >  	for_each_plane(dev_priv, pipe, plane) {
> > -		hw_entry = &hw_ddb.plane[pipe][plane];
> > -		sw_entry = &sw_ddb->plane[pipe][plane];
> > +		hw_plane_wm = &hw_wm.planes[plane];
> > +		sw_plane_wm = &sw_wm->planes[plane];
> >  
> > -		if (skl_ddb_entry_equal(hw_entry, sw_entry))
> > -			continue;
> > +		/* Watermarks */
> > +		for (level = 0; level <= max_level; level++) {
> > +			if (skl_wm_level_equals(&hw_plane_wm-
> > > 
> > > wm[level],
> > +						&sw_plane_wm-
> > > 
> > > wm[level]))
> > +				continue;
> > +
> > +			DRM_ERROR("mismatch in WM pipe %c plane %d
> > level %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n",
> > +				  pipe_name(pipe), plane + 1,
> > level,
> > +				  sw_plane_wm->wm[level].plane_en,
> > +				  sw_plane_wm-
> > > 
> > > wm[level].plane_res_b,
> > +				  sw_plane_wm-
> > > 
> > > wm[level].plane_res_l,
> > +				  hw_plane_wm->wm[level].plane_en,
> > +				  hw_plane_wm-
> > > 
> > > wm[level].plane_res_b,
> > +				  hw_plane_wm-
> > > 
> > > wm[level].plane_res_l);
> > +		}
> >  
> > -		DRM_ERROR("mismatch in DDB state pipe %c plane %d
> > "
> > -			  "(expected (%u,%u), found (%u,%u))\n",
> > -			  pipe_name(pipe), plane + 1,
> > -			  sw_entry->start, sw_entry->end,
> > -			  hw_entry->start, hw_entry->end);
> > +		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> > +					 &sw_plane_wm->trans_wm))
> > {
> > +			DRM_ERROR("mismatch in trans WM pipe %c
> > plane %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n",
> > +				  pipe_name(pipe), plane + 1,
> > +				  sw_plane_wm->trans_wm.plane_en,
> > +				  sw_plane_wm-
> > >trans_wm.plane_res_b,
> > +				  sw_plane_wm-
> > >trans_wm.plane_res_l,
> > +				  hw_plane_wm->trans_wm.plane_en,
> > +				  hw_plane_wm-
> > >trans_wm.plane_res_b,
> > +				  hw_plane_wm-
> > > 
> > > trans_wm.plane_res_l);
> > +		}
> > +
> > +		/* DDB */
> > +		hw_ddb_entry = &hw_ddb.plane[pipe][plane];
> > +		sw_ddb_entry = &sw_ddb->plane[pipe][plane];
> > +
> > +		if (!skl_ddb_entry_equal(hw_ddb_entry,
> > sw_ddb_entry)) {
> > +			DRM_ERROR("mismatch in DDB state pipe %c
> > plane %d "
> > +				  "(expected (%u,%u), found
> > (%u,%u))\n",
> > +				  pipe_name(pipe), plane + 1,
> > +				  sw_ddb_entry->start,
> > sw_ddb_entry-
> > > 
> > > end,
> > +				  hw_ddb_entry->start,
> > hw_ddb_entry-
> > > 
> > > end);
> > +		}
> >  	}
> >  
> >  	/*
> > @@ -13477,15 +13513,47 @@ static void verify_wm_state(struct
> > drm_crtc
> > *crtc,
> >  	 * once the plane becomes visible, we can skip this check
> >  	 */
> >  	if (intel_crtc->cursor_addr) {
> > -		hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> > -		sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
> > +		hw_plane_wm = &hw_wm.planes[PLANE_CURSOR];
> > +		sw_plane_wm = &sw_wm->planes[PLANE_CURSOR];
> > +
> > +		/* Watermarks */
> > +		for (level = 0; level <= max_level; level++) {
> > +			if (skl_wm_level_equals(&hw_plane_wm-
> > > 
> > > wm[level],
> > +						&sw_plane_wm-
> > > 
> > > wm[level]))
> > +				continue;
> > +
> > +			DRM_ERROR("mismatch in WM pipe %c cursor
> > level %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n",
> > +				  pipe_name(pipe), level,
> > +				  sw_plane_wm->wm[level].plane_en,
> > +				  sw_plane_wm-
> > > 
> > > wm[level].plane_res_b,
> > +				  sw_plane_wm-
> > > 
> > > wm[level].plane_res_l,
> > +				  hw_plane_wm->wm[level].plane_en,
> > +				  hw_plane_wm-
> > > 
> > > wm[level].plane_res_b,
> > +				  hw_plane_wm-
> > > 
> > > wm[level].plane_res_l);
> > +		}
> > +
> > +		if (!skl_wm_level_equals(&hw_plane_wm->trans_wm,
> > +					 &sw_plane_wm->trans_wm))
> > {
> > +			DRM_ERROR("mismatch in trans WM pipe %c
> > cursor (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n",
> > +				  pipe_name(pipe),
> > +				  sw_plane_wm->trans_wm.plane_en,
> > +				  sw_plane_wm-
> > >trans_wm.plane_res_b,
> > +				  sw_plane_wm-
> > >trans_wm.plane_res_l,
> > +				  hw_plane_wm->trans_wm.plane_en,
> > +				  hw_plane_wm-
> > >trans_wm.plane_res_b,
> > +				  hw_plane_wm-
> > > 
> > > trans_wm.plane_res_l);
> > +		}
> > +
> > +		/* DDB */
> > +		hw_ddb_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> > +		sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
> >  
> > -		if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
> > +		if (!skl_ddb_entry_equal(hw_ddb_entry,
> > sw_ddb_entry)) {
> >  			DRM_ERROR("mismatch in DDB state pipe %c
> > cursor "
> >  				  "(expected (%u,%u), found
> > (%u,%u))\n",
> >  				  pipe_name(pipe),
> > -				  sw_entry->start, sw_entry->end,
> > -				  hw_entry->start, hw_entry->end);
> > +				  sw_ddb_entry->start,
> > sw_ddb_entry-
> > > 
> > > end,
> > +				  hw_ddb_entry->start,
> > hw_ddb_entry-
> > > 
> > > end);
> >  		}
> >  	}
> >  }
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/10] drm/i915/gen9: Don't wrap strings in verify_wm_state()

[Lyude]

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: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2c682bc..6191baf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13498,8 +13498,7 @@ static void verify_wm_state(struct drm_crtc *crtc,
 		sw_ddb_entry = &sw_ddb->plane[pipe][plane];
 
 		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
-			DRM_ERROR("mismatch in DDB state pipe %c plane %d "
-				  "(expected (%u,%u), found (%u,%u))\n",
+			DRM_ERROR("mismatch in DDB state pipe %c plane %d (expected (%u,%u), found (%u,%u))\n",
 				  pipe_name(pipe), plane + 1,
 				  sw_ddb_entry->start, sw_ddb_entry->end,
 				  hw_ddb_entry->start, hw_ddb_entry->end);
@@ -13549,8 +13548,7 @@ static void verify_wm_state(struct drm_crtc *crtc,
 		sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
 
 		if (!skl_ddb_entry_equal(hw_ddb_entry, sw_ddb_entry)) {
-			DRM_ERROR("mismatch in DDB state pipe %c cursor "
-				  "(expected (%u,%u), found (%u,%u))\n",
+			DRM_ERROR("mismatch in DDB state pipe %c cursor (expected (%u,%u), found (%u,%u))\n",
 				  pipe_name(pipe),
 				  sw_ddb_entry->start, sw_ddb_entry->end,
 				  hw_ddb_entry->start, hw_ddb_entry->end);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/i915/gen9: Don't wrap strings in verify_wm_state()

[Paulo Zanoni]

Em Sex, 2016-10-07 às 20:11 -0400, Lyude escreveu:

Bikesheding: it would be nice to write a commit message explaining why,
even if the message just tells the user to read
Documentation/CodingStyle.

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: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 2c682bc..6191baf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13498,8 +13498,7 @@ static void verify_wm_state(struct drm_crtc
> *crtc,
>  		sw_ddb_entry = &sw_ddb->plane[pipe][plane];
>  
>  		if (!skl_ddb_entry_equal(hw_ddb_entry,
> sw_ddb_entry)) {
> -			DRM_ERROR("mismatch in DDB state pipe %c
> plane %d "
> -				  "(expected (%u,%u), found
> (%u,%u))\n",
> +			DRM_ERROR("mismatch in DDB state pipe %c
> plane %d (expected (%u,%u), found (%u,%u))\n",
>  				  pipe_name(pipe), plane + 1,
>  				  sw_ddb_entry->start, sw_ddb_entry-
> >end,
>  				  hw_ddb_entry->start, hw_ddb_entry-
> >end);
> @@ -13549,8 +13548,7 @@ static void verify_wm_state(struct drm_crtc
> *crtc,
>  		sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
>  
>  		if (!skl_ddb_entry_equal(hw_ddb_entry,
> sw_ddb_entry)) {
> -			DRM_ERROR("mismatch in DDB state pipe %c
> cursor "
> -				  "(expected (%u,%u), found
> (%u,%u))\n",
> +			DRM_ERROR("mismatch in DDB state pipe %c
> cursor (expected (%u,%u), found (%u,%u))\n",
>  				  pipe_name(pipe),
>  				  sw_ddb_entry->start, sw_ddb_entry-
> >end,
>  				  hw_ddb_entry->start, hw_ddb_entry-
> >end);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx