[PATCH 0/8] SKL WM fixups

[PATCH 0/8] SKL WM fixups

[Damien Lespiau]

Here's a few patches on top of the original WM series to both address some
review comments from Ville and disable the transition WMs (because we noticed
some underruns with them and the code is not quit ready).

With those, I'm hopeful a few more r-b tags can be given.

-- 
Damien

Damien Lespiau (8):
  drm/i915/skl: Make 'end' of the DDB allocation entry exclusive
  drm/i915/skl: Use a more descriptive parameter name in
    skl_compute_plane_wm()
  drm/i915/skl: Move the per-latency maximum test earlier
  drm/i915/skl: Reduce the number of holes in struct skl_wm_level
  drm/i915/skl: Move all the WM compute functions in one place
  drm/i915/skl: Rework when the transition WMs are computed
  drm/i915/skl: Correctly align skl_compute_plane_wm() arguments
  drm/i915/skl: Reduce the indentation level in skl_write_wm_values()

 drivers/gpu/drm/i915/i915_drv.h |   6 +-
 drivers/gpu/drm/i915/intel_pm.c | 164 ++++++++++++++++++++++------------------
 2 files changed, 94 insertions(+), 76 deletions(-)

-- 
1.8.3.1

[PATCH 1/8] drm/i915/skl: Make 'end' of the DDB allocation entry exclusive

[Damien Lespiau]

Ville suggested that we should use the same semantics as C arrays to
reduce the number of those pesky +1/-1 in the allocation code.

This patch leaves the debugfs file as is, showing the internal DDB
allocation structure, not the values written in the registers.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  4 ++--
 drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++++++++++++---------
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 14be1dc..0320698 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1414,7 +1414,7 @@ struct ilk_wm_values {
 };
 
 struct skl_ddb_entry {
-	uint16_t start, end;	/* in number of blocks */
+	uint16_t start, end;	/* in number of blocks, 'end' is exclusive */
 };
 
 static inline uint16_t skl_ddb_entry_size(const struct skl_ddb_entry *entry)
@@ -1423,7 +1423,7 @@ static inline uint16_t skl_ddb_entry_size(const struct skl_ddb_entry *entry)
 	if (entry->end == 0)
 		return 0;
 
-	return entry->end - entry->start + 1;
+	return entry->end - entry->start;
 }
 
 static inline bool skl_ddb_entry_equal(const struct skl_ddb_entry *e1,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2a41ae8..016b54e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3038,7 +3038,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 
 	pipe_size = ddb_size / config->num_pipes_active;
 	alloc->start = nth_active_pipe * ddb_size / config->num_pipes_active;
-	alloc->end = alloc->start + pipe_size - 1;
+	alloc->end = alloc->start + pipe_size;
 }
 
 static unsigned int skl_cursor_allocation(const struct intel_wm_config *config)
@@ -3053,6 +3053,8 @@ static void skl_ddb_entry_init_from_hw(struct skl_ddb_entry *entry, u32 reg)
 {
 	entry->start = reg & 0x3ff;
 	entry->end = (reg >> 16) & 0x3ff;
+	if (entry->end)
+		entry->end += 1;
 }
 
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
@@ -3129,7 +3131,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 	}
 
 	cursor_blocks = skl_cursor_allocation(config);
-	ddb->cursor[pipe].start = alloc.end - cursor_blocks + 1;
+	ddb->cursor[pipe].start = alloc.end - cursor_blocks;
 	ddb->cursor[pipe].end = alloc.end;
 
 	alloc_size -= cursor_blocks;
@@ -3163,7 +3165,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 				       total_data_rate);
 
 		ddb->plane[pipe][plane].start = start;
-		ddb->plane[pipe][plane].end = start + plane_blocks - 1;
+		ddb->plane[pipe][plane].end = start + plane_blocks;
 
 		start += plane_blocks;
 	}
@@ -3467,6 +3469,15 @@ static void skl_compute_wm_results(struct drm_device *dev,
 	r->wm_linetime[pipe] = p_wm->linetime;
 }
 
+static void skl_ddb_entry_write(struct drm_i915_private *dev_priv, uint32_t reg,
+				const struct skl_ddb_entry *entry)
+{
+	if (entry->end)
+		I915_WRITE(reg, (entry->end - 1) << 16 | entry->start);
+	else
+		I915_WRITE(reg, 0);
+}
+
 static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 				const struct skl_wm_values *new)
 {
@@ -3494,13 +3505,12 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 			I915_WRITE(CUR_WM_TRANS(pipe), new->cursor_trans[pipe]);
 
 			for (i = 0; i < intel_num_planes(crtc); i++)
-				I915_WRITE(PLANE_BUF_CFG(pipe, i),
-					   new->ddb.plane[pipe][i].end << 16 |
-					   new->ddb.plane[pipe][i].start);
+				skl_ddb_entry_write(dev_priv,
+						    PLANE_BUF_CFG(pipe, i),
+						    &new->ddb.plane[pipe][i]);
 
-			I915_WRITE(CUR_BUF_CFG(pipe),
-				   new->ddb.cursor[pipe].end << 16 |
-				   new->ddb.cursor[pipe].start);
+			skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
+					    &new->ddb.cursor[pipe]);
 		}
 	}
 }
-- 
1.8.3.1

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

Re: [PATCH 1/8] drm/i915/skl: Make 'end' of the DDB allocation entry exclusive

[Ville Syrjälä]

On Tue, Oct 14, 2014 at 05:30:59PM +0100, Damien Lespiau wrote:
> Ville suggested that we should use the same semantics as C arrays to
> reduce the number of those pesky +1/-1 in the allocation code.
> 
> This patch leaves the debugfs file as is, showing the internal DDB
> allocation structure, not the values written in the registers.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  4 ++--
>  drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++++++++++++---------
>  2 files changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 14be1dc..0320698 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1414,7 +1414,7 @@ struct ilk_wm_values {
>  };
>  
>  struct skl_ddb_entry {
> -	uint16_t start, end;	/* in number of blocks */
> +	uint16_t start, end;	/* in number of blocks, 'end' is exclusive */
>  };
>  
>  static inline uint16_t skl_ddb_entry_size(const struct skl_ddb_entry *entry)
> @@ -1423,7 +1423,7 @@ static inline uint16_t skl_ddb_entry_size(const struct skl_ddb_entry *entry)
>  	if (entry->end == 0)
>  		return 0;

We can now drop the special case for end==0. The rest looks good so

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
> -	return entry->end - entry->start + 1;
> +	return entry->end - entry->start;
>  }
>  
>  static inline bool skl_ddb_entry_equal(const struct skl_ddb_entry *e1,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2a41ae8..016b54e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3038,7 +3038,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  
>  	pipe_size = ddb_size / config->num_pipes_active;
>  	alloc->start = nth_active_pipe * ddb_size / config->num_pipes_active;
> -	alloc->end = alloc->start + pipe_size - 1;
> +	alloc->end = alloc->start + pipe_size;
>  }
>  
>  static unsigned int skl_cursor_allocation(const struct intel_wm_config *config)
> @@ -3053,6 +3053,8 @@ static void skl_ddb_entry_init_from_hw(struct skl_ddb_entry *entry, u32 reg)
>  {
>  	entry->start = reg & 0x3ff;
>  	entry->end = (reg >> 16) & 0x3ff;
> +	if (entry->end)
> +		entry->end += 1;
>  }
>  
>  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
> @@ -3129,7 +3131,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  	}
>  
>  	cursor_blocks = skl_cursor_allocation(config);
> -	ddb->cursor[pipe].start = alloc.end - cursor_blocks + 1;
> +	ddb->cursor[pipe].start = alloc.end - cursor_blocks;
>  	ddb->cursor[pipe].end = alloc.end;
>  
>  	alloc_size -= cursor_blocks;
> @@ -3163,7 +3165,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  				       total_data_rate);
>  
>  		ddb->plane[pipe][plane].start = start;
> -		ddb->plane[pipe][plane].end = start + plane_blocks - 1;
> +		ddb->plane[pipe][plane].end = start + plane_blocks;
>  
>  		start += plane_blocks;
>  	}
> @@ -3467,6 +3469,15 @@ static void skl_compute_wm_results(struct drm_device *dev,
>  	r->wm_linetime[pipe] = p_wm->linetime;
>  }
>  
> +static void skl_ddb_entry_write(struct drm_i915_private *dev_priv, uint32_t reg,
> +				const struct skl_ddb_entry *entry)
> +{
> +	if (entry->end)
> +		I915_WRITE(reg, (entry->end - 1) << 16 | entry->start);
> +	else
> +		I915_WRITE(reg, 0);
> +}
> +
>  static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  				const struct skl_wm_values *new)
>  {
> @@ -3494,13 +3505,12 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  			I915_WRITE(CUR_WM_TRANS(pipe), new->cursor_trans[pipe]);
>  
>  			for (i = 0; i < intel_num_planes(crtc); i++)
> -				I915_WRITE(PLANE_BUF_CFG(pipe, i),
> -					   new->ddb.plane[pipe][i].end << 16 |
> -					   new->ddb.plane[pipe][i].start);
> +				skl_ddb_entry_write(dev_priv,
> +						    PLANE_BUF_CFG(pipe, i),
> +						    &new->ddb.plane[pipe][i]);
>  
> -			I915_WRITE(CUR_BUF_CFG(pipe),
> -				   new->ddb.cursor[pipe].end << 16 |
> -				   new->ddb.cursor[pipe].start);
> +			skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> +					    &new->ddb.cursor[pipe]);
>  		}
>  	}
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/8] drm/i915/skl: Use a more descriptive parameter name in skl_compute_plane_wm()

[Damien Lespiau]

What we're talking about here is the DDB allocation (in blocks). That's
more descriptive than 'max_page_buff_alloc'.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 016b54e..475a3d4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3329,7 +3329,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 
 static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
 				   struct intel_plane_wm_parameters *p_params,
-				   uint16_t max_page_buff_alloc,
+				   uint16_t ddb_allocation,
 				   uint32_t mem_value,
 				   uint16_t *res_blocks, /* out */
 				   uint8_t *res_lines /* out */)
@@ -3353,7 +3353,7 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
 					p_params->bytes_per_pixel;
 
 	/* For now xtile and linear */
-	if (((max_page_buff_alloc * 512) / plane_bytes_per_line) >= 1)
+	if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
 		result_bytes = min(method1, method2);
 	else
 		result_bytes = method1;
-- 
1.8.3.1

[PATCH 3/8] drm/i915/skl: Move the per-latency maximum test earlier

[Damien Lespiau]

To align with the ilk WM code and because it makes sense to test against
the upper bounds as soon as possible, let's move the maximum checks from
skl_compute_wm_results() to skl_compute_plane_wm().

This has the nice benefit to be done in come common plane code and so
remove the duplication we had between the regular planes and the cursor.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 475a3d4..65df074 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3361,6 +3361,9 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
 	*res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
 	*res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
 
+	if (*res_blocks > ddb_allocation || *res_lines > 31)
+		return false;
+
 	return true;
 }
 
@@ -3422,17 +3425,11 @@ static void skl_compute_wm_results(struct drm_device *dev,
 	enum pipe pipe = intel_crtc->pipe;
 
 	for (level = 0; level <= max_level; level++) {
-		uint16_t ddb_blocks;
 		uint32_t temp;
 		int i;
 
 		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
 			temp = 0;
-			ddb_blocks = skl_ddb_entry_size(&r->ddb.plane[pipe][i]);
-
-			if ((p_wm->wm[level].plane_res_b[i] > ddb_blocks) ||
-				(p_wm->wm[level].plane_res_l[i] > 31))
-				p_wm->wm[level].plane_en[i] = false;
 
 			temp |= p_wm->wm[level].plane_res_l[i] <<
 					PLANE_WM_LINES_SHIFT;
@@ -3447,11 +3444,6 @@ static void skl_compute_wm_results(struct drm_device *dev,
 		}
 
 		temp = 0;
-		ddb_blocks = skl_ddb_entry_size(&r->ddb.cursor[pipe]);
-
-		if ((p_wm->wm[level].cursor_res_b > ddb_blocks) ||
-			(p_wm->wm[level].cursor_res_l > 31))
-			p_wm->wm[level].cursor_en = false;
 
 		temp |= p_wm->wm[level].cursor_res_l << PLANE_WM_LINES_SHIFT;
 		temp |= p_wm->wm[level].cursor_res_b;
-- 
1.8.3.1

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

Re: [PATCH 3/8] drm/i915/skl: Move the per-latency maximum test earlier

[Ville Syrjälä]

On Tue, Oct 14, 2014 at 05:31:01PM +0100, Damien Lespiau wrote:
> To align with the ilk WM code and because it makes sense to test against
> the upper bounds as soon as possible, let's move the maximum checks from
> skl_compute_wm_results() to skl_compute_plane_wm().
> 
> This has the nice benefit to be done in come common plane code and so
> remove the duplication we had between the regular planes and the cursor.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 475a3d4..65df074 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3361,6 +3361,9 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
>  	*res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
>  	*res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
>  
> +	if (*res_blocks > ddb_allocation || *res_lines > 31)
> +		return false;
> +

Unless I completely misread things we would still stick the computed *res
values into the register which still risks an overflow there. The ILK
code leaves the enrtire wm struct zeroed if any value can't fit in the
register (well apart from fbc_wm which is handled in a special way).

My impression is that we could just zero the values here any time we see
that they might exceed the limits. And I think we don't especially have
to care about the register max vs. current ddb allocation max
distinction on SKL like we do on ILK. Although if we did make that
distinction, and I've not thought it really though yet, maybe we can
skip recomputing some plane watermarks when the only thing changing for
the plane is the DDB allocation. We'd just have to re-evaluate the state
of the wm level enable bit when we're about to blast the watermarks into
the register. But maybe that would just complicate the code too much. I
guess it's better left as a potential future optimization.

>  	return true;
>  }
>  
> @@ -3422,17 +3425,11 @@ static void skl_compute_wm_results(struct drm_device *dev,
>  	enum pipe pipe = intel_crtc->pipe;
>  
>  	for (level = 0; level <= max_level; level++) {
> -		uint16_t ddb_blocks;
>  		uint32_t temp;
>  		int i;
>  
>  		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
>  			temp = 0;
> -			ddb_blocks = skl_ddb_entry_size(&r->ddb.plane[pipe][i]);
> -
> -			if ((p_wm->wm[level].plane_res_b[i] > ddb_blocks) ||
> -				(p_wm->wm[level].plane_res_l[i] > 31))
> -				p_wm->wm[level].plane_en[i] = false;
>  
>  			temp |= p_wm->wm[level].plane_res_l[i] <<
>  					PLANE_WM_LINES_SHIFT;
> @@ -3447,11 +3444,6 @@ static void skl_compute_wm_results(struct drm_device *dev,
>  		}
>  
>  		temp = 0;
> -		ddb_blocks = skl_ddb_entry_size(&r->ddb.cursor[pipe]);
> -
> -		if ((p_wm->wm[level].cursor_res_b > ddb_blocks) ||
> -			(p_wm->wm[level].cursor_res_l > 31))
> -			p_wm->wm[level].cursor_en = false;
>  
>  		temp |= p_wm->wm[level].cursor_res_l << PLANE_WM_LINES_SHIFT;
>  		temp |= p_wm->wm[level].cursor_res_b;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/8 v2] drm/i915/skl: Make 'end' of the DDB allocation entry exclusive

[Damien Lespiau]

Ville suggested that we should use the same semantics as C arrays to
reduce the number of those pesky +1/-1 in the allocation code.

This patch leaves the debugfs file as is, showing the internal DDB
allocation structure, not the values written in the registers.

v2: Remove the test on ->end in skl_ddb_entry_size() (Ville)

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  8 ++------
 drivers/gpu/drm/i915/intel_pm.c | 28 +++++++++++++++++++---------
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b4e713..3b914d0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1386,16 +1386,12 @@ struct ilk_wm_values {
 };
 
 struct skl_ddb_entry {
-	uint16_t start, end;	/* in number of blocks */
+	uint16_t start, end;	/* in number of blocks, 'end' is exclusive */
 };
 
 static inline uint16_t skl_ddb_entry_size(const struct skl_ddb_entry *entry)
 {
-	/* end not set, clearly no allocation here. start can be 0 though */
-	if (entry->end == 0)
-		return 0;
-
-	return entry->end - entry->start + 1;
+	return entry->end - entry->start;
 }
 
 static inline bool skl_ddb_entry_equal(const struct skl_ddb_entry *e1,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 159c2e0..e417a6a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3040,7 +3040,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 
 	pipe_size = ddb_size / config->num_pipes_active;
 	alloc->start = nth_active_pipe * ddb_size / config->num_pipes_active;
-	alloc->end = alloc->start + pipe_size - 1;
+	alloc->end = alloc->start + pipe_size;
 }
 
 static unsigned int skl_cursor_allocation(const struct intel_wm_config *config)
@@ -3055,6 +3055,8 @@ static void skl_ddb_entry_init_from_hw(struct skl_ddb_entry *entry, u32 reg)
 {
 	entry->start = reg & 0x3ff;
 	entry->end = (reg >> 16) & 0x3ff;
+	if (entry->end)
+		entry->end += 1;
 }
 
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
@@ -3131,7 +3133,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 	}
 
 	cursor_blocks = skl_cursor_allocation(config);
-	ddb->cursor[pipe].start = alloc.end - cursor_blocks + 1;
+	ddb->cursor[pipe].start = alloc.end - cursor_blocks;
 	ddb->cursor[pipe].end = alloc.end;
 
 	alloc_size -= cursor_blocks;
@@ -3165,7 +3167,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 				       total_data_rate);
 
 		ddb->plane[pipe][plane].start = start;
-		ddb->plane[pipe][plane].end = start + plane_blocks - 1;
+		ddb->plane[pipe][plane].end = start + plane_blocks;
 
 		start += plane_blocks;
 	}
@@ -3453,6 +3455,15 @@ static void skl_compute_wm_results(struct drm_device *dev,
 	r->wm_linetime[pipe] = p_wm->linetime;
 }
 
+static void skl_ddb_entry_write(struct drm_i915_private *dev_priv, uint32_t reg,
+				const struct skl_ddb_entry *entry)
+{
+	if (entry->end)
+		I915_WRITE(reg, (entry->end - 1) << 16 | entry->start);
+	else
+		I915_WRITE(reg, 0);
+}
+
 static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 				const struct skl_wm_values *new)
 {
@@ -3480,13 +3491,12 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 			I915_WRITE(CUR_WM_TRANS(pipe), new->cursor_trans[pipe]);
 
 			for (i = 0; i < intel_num_planes(crtc); i++)
-				I915_WRITE(PLANE_BUF_CFG(pipe, i),
-					   new->ddb.plane[pipe][i].end << 16 |
-					   new->ddb.plane[pipe][i].start);
+				skl_ddb_entry_write(dev_priv,
+						    PLANE_BUF_CFG(pipe, i),
+						    &new->ddb.plane[pipe][i]);
 
-			I915_WRITE(CUR_BUF_CFG(pipe),
-				   new->ddb.cursor[pipe].end << 16 |
-				   new->ddb.cursor[pipe].start);
+			skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
+					    &new->ddb.cursor[pipe]);
 		}
 	}
 }
-- 
1.8.3.1

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

[PATCH 3/8 v2] drm/i915/skl: Make res_blocks/lines intermediate values 32 bits

[Damien Lespiau]

To align with the ilk WM code and because it makes sense to test against
the upper bounds as soon as possible on variables that are bigger than
the number of bits in the register, let's move the maximum checks from
skl_compute_wm_results() to skl_compute_plane_wm().

v2: Leave the result values to 0 when overflowing the limits (Ville)
    Use 32 bits intermediate variables (Damien)

Instead of using the 16 and 8 bits space we have in the result
structure, use 32 bits local variables until we're sure they fit into
the constraints.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0142ad1..8580298 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3317,10 +3317,10 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
 				   struct intel_plane_wm_parameters *p_params,
 				   uint16_t ddb_allocation,
 				   uint32_t mem_value,
-				   uint16_t *res_blocks, /* out */
-				   uint8_t *res_lines /* out */)
+				   uint16_t *out_blocks, /* out */
+				   uint8_t *out_lines /* out */)
 {
-	uint32_t method1, method2, plane_bytes_per_line;
+	uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
 	uint32_t result_bytes;
 
 	if (mem_value == 0 || !p->active || !p_params->enabled)
@@ -3344,8 +3344,14 @@ static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
 	else
 		result_bytes = method1;
 
-	*res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
-	*res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
+	res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
+	res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
+
+	if (res_blocks > ddb_allocation || res_lines > 31)
+		return false;
+
+	*out_blocks = res_blocks;
+	*out_lines = res_lines;
 
 	return true;
 }
@@ -3408,17 +3414,11 @@ static void skl_compute_wm_results(struct drm_device *dev,
 	enum pipe pipe = intel_crtc->pipe;
 
 	for (level = 0; level <= max_level; level++) {
-		uint16_t ddb_blocks;
 		uint32_t temp;
 		int i;
 
 		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
 			temp = 0;
-			ddb_blocks = skl_ddb_entry_size(&r->ddb.plane[pipe][i]);
-
-			if ((p_wm->wm[level].plane_res_b[i] > ddb_blocks) ||
-				(p_wm->wm[level].plane_res_l[i] > 31))
-				p_wm->wm[level].plane_en[i] = false;
 
 			temp |= p_wm->wm[level].plane_res_l[i] <<
 					PLANE_WM_LINES_SHIFT;
@@ -3433,11 +3433,6 @@ static void skl_compute_wm_results(struct drm_device *dev,
 		}
 
 		temp = 0;
-		ddb_blocks = skl_ddb_entry_size(&r->ddb.cursor[pipe]);
-
-		if ((p_wm->wm[level].cursor_res_b > ddb_blocks) ||
-			(p_wm->wm[level].cursor_res_l > 31))
-			p_wm->wm[level].cursor_en = false;
 
 		temp |= p_wm->wm[level].cursor_res_l << PLANE_WM_LINES_SHIFT;
 		temp |= p_wm->wm[level].cursor_res_b;
-- 
1.8.3.1

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

[PATCH 4/8] drm/i915/skl: Reduce the number of holes in struct skl_wm_level

[Damien Lespiau]

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0320698..2e800db 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1452,9 +1452,9 @@ struct skl_wm_values {
 
 struct skl_wm_level {
 	bool plane_en[I915_MAX_PLANES];
+	bool cursor_en;
 	uint16_t plane_res_b[I915_MAX_PLANES];
 	uint8_t plane_res_l[I915_MAX_PLANES];
-	bool cursor_en;
 	uint16_t cursor_res_b;
 	uint8_t cursor_res_l;
 };
-- 
1.8.3.1

[PATCH 5/8] drm/i915/skl: Move all the WM compute functions in one place

[Damien Lespiau]

The DDB allocation code managed to split in two the compute functions.

Bring back skl_compute_transition_wm() and skl_compute_linetime_wm()
with their little friends.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 44 ++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 65df074..ec89a82 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3215,28 +3215,6 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
 	return ret;
 }
 
-static void skl_compute_transition_wm(struct drm_crtc *crtc,
-				  struct skl_pipe_wm_parameters *params,
-				  struct skl_pipe_wm *pipe_wm)
-{
-	/*
-	 * For now it is suggested to use the LP0 wm val of corresponding
-	 * plane as transition wm val. This is done while computing results.
-	 */
-	if (!params->active)
-		return;
-}
-
-static uint32_t
-skl_compute_linetime_wm(struct drm_crtc *crtc, struct skl_pipe_wm_parameters *p)
-{
-	if (!intel_crtc_active(crtc))
-		return 0;
-
-	return DIV_ROUND_UP(8 * p->pipe_htotal * 1000, p->pixel_rate);
-
-}
-
 static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb,
 				       const struct intel_crtc *intel_crtc)
 {
@@ -3395,6 +3373,28 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 						 &result->cursor_res_l);
 }
 
+static uint32_t
+skl_compute_linetime_wm(struct drm_crtc *crtc, struct skl_pipe_wm_parameters *p)
+{
+	if (!intel_crtc_active(crtc))
+		return 0;
+
+	return DIV_ROUND_UP(8 * p->pipe_htotal * 1000, p->pixel_rate);
+
+}
+
+static void skl_compute_transition_wm(struct drm_crtc *crtc,
+				      struct skl_pipe_wm_parameters *params,
+				      struct skl_pipe_wm *pipe_wm)
+{
+	/*
+	 * For now it is suggested to use the LP0 wm val of corresponding
+	 * plane as transition wm val.
+	 */
+	if (!params->active)
+		return;
+}
+
 static void skl_compute_pipe_wm(struct drm_crtc *crtc,
 				struct skl_ddb_allocation *ddb,
 				struct skl_pipe_wm_parameters *params,
-- 
1.8.3.1

[PATCH 6/8] drm/i915/skl: Rework when the transition WMs are computed

[Damien Lespiau]

The transition WMs code was doing a shortcut and the values were copied
from the WM0 ones at compute_wm_results() time. Going forward, we want
to compute them like the other WMs and resolve their final register
values in the same way as well.

This patch does just that and isolate the transtion WM compute code in
skl_compute_transition_wm() while skl_compute_wm_results() takes care of
the register values.

We also take the opportunity to disable the transition WMs for now.
We've noticed underruns and they seem to be the culprit.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 46 +++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ec89a82..7217fc6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3385,14 +3385,18 @@ skl_compute_linetime_wm(struct drm_crtc *crtc, struct skl_pipe_wm_parameters *p)
 
 static void skl_compute_transition_wm(struct drm_crtc *crtc,
 				      struct skl_pipe_wm_parameters *params,
-				      struct skl_pipe_wm *pipe_wm)
+				      struct skl_wm_level *trans_wm /* out */)
 {
-	/*
-	 * For now it is suggested to use the LP0 wm val of corresponding
-	 * plane as transition wm val.
-	 */
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int i;
+
 	if (!params->active)
 		return;
+
+	/* Until we know more, just disable transition WMs */
+	for (i = 0; i < intel_num_planes(intel_crtc); i++)
+		trans_wm->plane_en[i] = false;
+	trans_wm->cursor_en = false;
 }
 
 static void skl_compute_pipe_wm(struct drm_crtc *crtc,
@@ -3412,7 +3416,7 @@ static void skl_compute_pipe_wm(struct drm_crtc *crtc,
 	}
 	pipe_wm->linetime = skl_compute_linetime_wm(crtc, params);
 
-	skl_compute_transition_wm(crtc, params, pipe_wm);
+	skl_compute_transition_wm(crtc, params, &pipe_wm->trans_wm);
 }
 
 static void skl_compute_wm_results(struct drm_device *dev,
@@ -3423,11 +3427,10 @@ static void skl_compute_wm_results(struct drm_device *dev,
 {
 	int level, max_level = ilk_wm_max_level(dev);
 	enum pipe pipe = intel_crtc->pipe;
+	uint32_t temp;
+	int i;
 
 	for (level = 0; level <= max_level; level++) {
-		uint32_t temp;
-		int i;
-
 		for (i = 0; i < intel_num_planes(intel_crtc); i++) {
 			temp = 0;
 
@@ -3438,9 +3441,6 @@ static void skl_compute_wm_results(struct drm_device *dev,
 				temp |= PLANE_WM_EN;
 
 			r->plane[pipe][i][level] = temp;
-			/* Use the LP0 WM value for transition WM for now. */
-			if (level == 0)
-				r->plane_trans[pipe][i] = temp;
 		}
 
 		temp = 0;
@@ -3452,12 +3452,28 @@ static void skl_compute_wm_results(struct drm_device *dev,
 			temp |= PLANE_WM_EN;
 
 		r->cursor[pipe][level] = temp;
-		/* Use the LP0 WM value for transition WM for now. */
-		if (level == 0)
-			r->cursor_trans[pipe] = temp;
 
 	}
 
+	/* transition WMs */
+	for (i = 0; i < intel_num_planes(intel_crtc); 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_EN;
+
+		r->plane_trans[pipe][i] = temp;
+	}
+
+	temp = 0;
+	temp |= p_wm->trans_wm.cursor_res_l << PLANE_WM_LINES_SHIFT;
+	temp |= p_wm->trans_wm.cursor_res_b;
+	if (p_wm->trans_wm.cursor_en)
+		temp |= PLANE_WM_EN;
+
+	r->cursor_trans[pipe] = temp;
+
 	r->wm_linetime[pipe] = p_wm->linetime;
 }
 
-- 
1.8.3.1

[PATCH 7/8] drm/i915/skl: Correctly align skl_compute_plane_wm() arguments

[Damien Lespiau]

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7217fc6..d0de78c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3306,11 +3306,11 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 }
 
 static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
-				   struct intel_plane_wm_parameters *p_params,
-				   uint16_t ddb_allocation,
-				   uint32_t mem_value,
-				   uint16_t *res_blocks, /* out */
-				   uint8_t *res_lines /* out */)
+				 struct intel_plane_wm_parameters *p_params,
+				 uint16_t ddb_allocation,
+				 uint32_t mem_value,
+				 uint16_t *res_blocks, /* out */
+				 uint8_t *res_lines /* out */)
 {
 	uint32_t method1, method2, plane_bytes_per_line;
 	uint32_t result_bytes;
-- 
1.8.3.1

[PATCH 8/8] drm/i915/skl: Reduce the indentation level in skl_write_wm_values()

[Damien Lespiau]

We can reduce the indentation level by continuing early.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 42 ++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d0de78c..1795484 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3496,30 +3496,30 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 		int i, level, max_level = ilk_wm_max_level(dev);
 		enum pipe pipe = crtc->pipe;
 
-		if (new->dirty[pipe]) {
-			I915_WRITE(PIPE_WM_LINETIME(pipe),
-					new->wm_linetime[pipe]);
-
-			for (level = 0; level <= max_level; level++) {
-				for (i = 0; i < intel_num_planes(crtc); i++)
-					I915_WRITE(PLANE_WM(pipe, i, level),
-						new->plane[pipe][i][level]);
-				I915_WRITE(CUR_WM(pipe, level),
-					new->cursor[pipe][level]);
-			}
-			for (i = 0; i < intel_num_planes(crtc); i++)
-				I915_WRITE(PLANE_WM_TRANS(pipe, i),
-						new->plane_trans[pipe][i]);
-			I915_WRITE(CUR_WM_TRANS(pipe), new->cursor_trans[pipe]);
+		if (!new->dirty[pipe])
+			continue;
 
-			for (i = 0; i < intel_num_planes(crtc); i++)
-				skl_ddb_entry_write(dev_priv,
-						    PLANE_BUF_CFG(pipe, i),
-						    &new->ddb.plane[pipe][i]);
+		I915_WRITE(PIPE_WM_LINETIME(pipe), new->wm_linetime[pipe]);
 
-			skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
-					    &new->ddb.cursor[pipe]);
+		for (level = 0; level <= max_level; level++) {
+			for (i = 0; i < intel_num_planes(crtc); i++)
+				I915_WRITE(PLANE_WM(pipe, i, level),
+					   new->plane[pipe][i][level]);
+			I915_WRITE(CUR_WM(pipe, level),
+				   new->cursor[pipe][level]);
 		}
+		for (i = 0; i < intel_num_planes(crtc); i++)
+			I915_WRITE(PLANE_WM_TRANS(pipe, i),
+				   new->plane_trans[pipe][i]);
+		I915_WRITE(CUR_WM_TRANS(pipe), new->cursor_trans[pipe]);
+
+		for (i = 0; i < intel_num_planes(crtc); i++)
+			skl_ddb_entry_write(dev_priv,
+					    PLANE_BUF_CFG(pipe, i),
+					    &new->ddb.plane[pipe][i]);
+
+		skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
+				    &new->ddb.cursor[pipe]);
 	}
 }
 
-- 
1.8.3.1

Re: [PATCH 0/8] SKL WM fixups

[Ville Syrjälä]

On Tue, Oct 14, 2014 at 05:30:58PM +0100, Damien Lespiau wrote:
> Here's a few patches on top of the original WM series to both address some
> review comments from Ville and disable the transition WMs (because we noticed
> some underruns with them and the code is not quit ready).
> 
> With those, I'm hopeful a few more r-b tags can be given.

> 
> -- 
> Damien
> 
> Damien Lespiau (8):
>   drm/i915/skl: Make 'end' of the DDB allocation entry exclusive

Replied with a comment to this patch

>   drm/i915/skl: Use a more descriptive parameter name in
>     skl_compute_plane_wm()
>   drm/i915/skl: Move the per-latency maximum test earlier

and here too

>   drm/i915/skl: Reduce the number of holes in struct skl_wm_level
>   drm/i915/skl: Move all the WM compute functions in one place
>   drm/i915/skl: Rework when the transition WMs are computed
>   drm/i915/skl: Correctly align skl_compute_plane_wm() arguments
>   drm/i915/skl: Reduce the indentation level in skl_write_wm_values()

The rest seem good to me, so:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
>  drivers/gpu/drm/i915/i915_drv.h |   6 +-
>  drivers/gpu/drm/i915/intel_pm.c | 164 ++++++++++++++++++++++------------------
>  2 files changed, 94 insertions(+), 76 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx