[PATCH 00/13] drm/i915: More ILK+ watermark prep patches

[PATCH 00/13] drm/i915: More ILK+ watermark prep patches

[ville.syrjala]

Next installment of ILK+ watermark prep patches.

This set doesn't really achieve much of anything, but the patches are
quite small and should not be very controversial. It gets us almost all
the way to the big watermark code replacement patch of the original series,
and so if we get these in I can focus on chopping up that last monster.

I did leave out the "drm/i915: Pass crtc to intel_update_watermarks() and
call it in one place during modeset" patch, since Chris rightfully complained
that it had two changes in the same patch. I'll revisit that one later as I
may need to think a bit more about where to call the WM funcs during modeset.

[PATCH 01/13] drm/i915: Use 'enabled' instead of 'enable' consistentnly in sprite WM code

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Let's be consistent and always call our variables 'enabled' insted of
the occasional 'enable'.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9b8c90ea..fe0c2af 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2838,7 +2838,7 @@ sandybridge_compute_sprite_srwm(struct drm_device *dev, int plane,
 
 static void sandybridge_update_sprite_wm(struct drm_device *dev, int pipe,
 					 uint32_t sprite_width, int pixel_size,
-					 bool enable, bool scaled)
+					 bool enabled, bool scaled)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int latency = dev_priv->wm.spr_latency[0] * 100;	/* In unit 0.1us */
@@ -2846,7 +2846,7 @@ static void sandybridge_update_sprite_wm(struct drm_device *dev, int pipe,
 	int sprite_wm, reg;
 	int ret;
 
-	if (!enable)
+	if (!enabled)
 		return;
 
 	switch (pipe) {
@@ -2961,13 +2961,13 @@ void intel_update_watermarks(struct drm_device *dev)
 
 void intel_update_sprite_watermarks(struct drm_device *dev, int pipe,
 				    uint32_t sprite_width, int pixel_size,
-				    bool enable, bool scaled)
+				    bool enabled, bool scaled)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (dev_priv->display.update_sprite_wm)
 		dev_priv->display.update_sprite_wm(dev, pipe, sprite_width,
-						   pixel_size, enable, scaled);
+						   pixel_size, enabled, scaled);
 }
 
 static struct drm_i915_gem_object *
-- 
1.8.1.5

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

[PATCH 02/13] drm/i915: Pull watermark level validity check out

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Refactor the code a bit to split the watermark level validity check into
a separate function.

Also add hack there that allows us to use it even for LP0 watermarks.
ATM we don't pre-compute/check the LP0 watermarks, so we just have to
clamp them to the maximum and hope things work out.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fe0c2af..f2374e0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2284,6 +2284,36 @@ static uint32_t ilk_compute_fbc_wm(struct hsw_pipe_wm_parameters *params,
 			  params->pri_bytes_per_pixel);
 }
 
+static bool ilk_check_wm(int level,
+			 const struct hsw_wm_maximums *max,
+			 struct hsw_lp_wm_result *result)
+{
+	bool ret;
+
+	result->enable = result->pri_val <= max->pri &&
+			 result->spr_val <= max->spr &&
+			 result->cur_val <= max->cur;
+
+	ret = result->enable;
+
+	/*
+	 * HACK until we can pre-compute everything,
+	 * and thus fail gracefully if LP0 watermarks
+	 * are exceeded...
+	 */
+	if (level == 0 && !result->enable) {
+		result->pri_val = min_t(uint32_t, result->pri_val, max->pri);
+		result->spr_val = min_t(uint32_t, result->spr_val, max->spr);
+		result->cur_val = min_t(uint32_t, result->cur_val, max->cur);
+		result->enable = true;
+		ret = false;
+	}
+
+	DRM_DEBUG_KMS("WM%d: %sabled\n", level, result->enable ? "en" : "dis");
+
+	return ret;
+}
+
 static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
 			      int level, struct hsw_wm_maximums *max,
 			      struct hsw_pipe_wm_parameters *params,
@@ -2317,10 +2347,7 @@ static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
 		result->fbc_enable = true;
 	}
 
-	result->enable = result->pri_val <= max->pri &&
-			 result->spr_val <= max->spr &&
-			 result->cur_val <= max->cur;
-	return result->enable;
+	return ilk_check_wm(level, max, result);
 }
 
 static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv,
-- 
1.8.1.5

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

[PATCH 03/13] drm/i915: Split watermark level computation from the code

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Refactor the watermarks computation for one level to a separate
function. This function will now set the ->enable flat to true,
even if the watermark level wasn't actually checked yet. In the
future we will delay the checking so we must consider all unchecked
watermarks as possibly valid.

v2: Preserve comment about latency units

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 51 +++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f2374e0..8a3cde3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2290,6 +2290,9 @@ static bool ilk_check_wm(int level,
 {
 	bool ret;
 
+	if (!result->enable)
+		return false;
+
 	result->enable = result->pri_val <= max->pri &&
 			 result->spr_val <= max->spr &&
 			 result->cur_val <= max->cur;
@@ -2314,31 +2317,45 @@ static bool ilk_check_wm(int level,
 	return ret;
 }
 
+static void ilk_compute_wm_level(struct drm_i915_private *dev_priv,
+				 int level,
+				 struct hsw_pipe_wm_parameters *p,
+				 struct hsw_lp_wm_result *result)
+{
+	uint16_t pri_latency = dev_priv->wm.pri_latency[level];
+	uint16_t spr_latency = dev_priv->wm.spr_latency[level];
+	uint16_t cur_latency = dev_priv->wm.cur_latency[level];
+
+	/* WM1+ latency values stored in 0.5us units */
+	if (level > 0) {
+		pri_latency *= 5;
+		spr_latency *= 5;
+		cur_latency *= 5;
+	}
+
+	result->pri_val = ilk_compute_pri_wm(p, pri_latency, level);
+	result->spr_val = ilk_compute_spr_wm(p, spr_latency);
+	result->cur_val = ilk_compute_cur_wm(p, cur_latency);
+	result->fbc_val = ilk_compute_fbc_wm(p, result->pri_val);
+	result->enable = true;
+}
+
 static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
 			      int level, struct hsw_wm_maximums *max,
 			      struct hsw_pipe_wm_parameters *params,
 			      struct hsw_lp_wm_result *result)
 {
 	enum pipe pipe;
-	uint32_t pri_val[3], spr_val[3], cur_val[3], fbc_val[3];
+	struct hsw_lp_wm_result res[3];
 
-	for (pipe = PIPE_A; pipe <= PIPE_C; pipe++) {
-		struct hsw_pipe_wm_parameters *p = &params[pipe];
-		/* WM1+ latency values stored in 0.5us units */
-		uint16_t pri_latency = dev_priv->wm.pri_latency[level] * 5;
-		uint16_t spr_latency = dev_priv->wm.spr_latency[level] * 5;
-		uint16_t cur_latency = dev_priv->wm.cur_latency[level] * 5;
-
-		pri_val[pipe] = ilk_compute_pri_wm(p, pri_latency, true);
-		spr_val[pipe] = ilk_compute_spr_wm(p, spr_latency);
-		cur_val[pipe] = ilk_compute_cur_wm(p, cur_latency);
-		fbc_val[pipe] = ilk_compute_fbc_wm(p, pri_val[pipe]);
-	}
+	for (pipe = PIPE_A; pipe <= PIPE_C; pipe++)
+		ilk_compute_wm_level(dev_priv, level, &params[pipe], &res[pipe]);
 
-	result->pri_val = max3(pri_val[0], pri_val[1], pri_val[2]);
-	result->spr_val = max3(spr_val[0], spr_val[1], spr_val[2]);
-	result->cur_val = max3(cur_val[0], cur_val[1], cur_val[2]);
-	result->fbc_val = max3(fbc_val[0], fbc_val[1], fbc_val[2]);
+	result->pri_val = max3(res[0].pri_val, res[1].pri_val, res[2].pri_val);
+	result->spr_val = max3(res[0].spr_val, res[1].spr_val, res[2].spr_val);
+	result->cur_val = max3(res[0].cur_val, res[1].cur_val, res[2].cur_val);
+	result->fbc_val = max3(res[0].fbc_val, res[1].fbc_val, res[2].fbc_val);
+	result->enable = true;
 
 	if (result->fbc_val > max->fbc) {
 		result->fbc_enable = false;
-- 
1.8.1.5

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

[PATCH 04/13] drm/i915: Kill fbc_enable from hsw_lp_wm_results

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We don't need to store the FBC WM enabled status in each watermark
level. We anyway have to reduce it down to a single boolean, so just
delay checking the FBC WM limit until we're computing the final
value.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8a3cde3..e789ae4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2182,7 +2182,6 @@ struct hsw_wm_maximums {
 
 struct hsw_lp_wm_result {
 	bool enable;
-	bool fbc_enable;
 	uint32_t pri_val;
 	uint32_t spr_val;
 	uint32_t cur_val;
@@ -2357,13 +2356,6 @@ static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
 	result->fbc_val = max3(res[0].fbc_val, res[1].fbc_val, res[2].fbc_val);
 	result->enable = true;
 
-	if (result->fbc_val > max->fbc) {
-		result->fbc_enable = false;
-		result->fbc_val = 0;
-	} else {
-		result->fbc_enable = true;
-	}
-
 	return ilk_check_wm(level, max, result);
 }
 
@@ -2602,9 +2594,9 @@ static void hsw_compute_wm_results(struct drm_device *dev,
 	 * a WM level. */
 	results->enable_fbc_wm = true;
 	for (level = 1; level <= max_level; level++) {
-		if (!lp_results[level - 1].fbc_enable) {
+		if (!lp_results[level - 1].fbc_val > lp_maximums->fbc) {
 			results->enable_fbc_wm = false;
-			break;
+			lp_results[level - 1].fbc_val = 0;
 		}
 	}
 
-- 
1.8.1.5

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

[PATCH 05/13] drm/i915: Rename hsw_data_buf_partitioning to intel_ddb_partitioning

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We're going to use the 1/2 vs. 5/6 split option already on IVB so the
HSW name is not proper. Just give it an intel_ prefix and move it to
i915_drv.h so that we can use it there later.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  5 +++++
 drivers/gpu/drm/i915/intel_pm.c | 17 ++++++-----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9ff09a2..8cc7011 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1050,6 +1050,11 @@ struct intel_vbt_data {
 	struct child_device_config *child_dev;
 };
 
+enum intel_ddb_partitioning {
+	INTEL_DDB_PART_1_2,
+	INTEL_DDB_PART_5_6, /* IVB+ */
+};
+
 typedef struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e789ae4..d5ab9f0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2196,11 +2196,6 @@ struct hsw_wm_values {
 	bool enable_fbc_wm;
 };
 
-enum hsw_data_buf_partitioning {
-	HSW_DATA_BUF_PART_1_2,
-	HSW_DATA_BUF_PART_5_6,
-};
-
 /*
  * For both WM_PIPE and WM_LP.
  * mem_value must be in 0.1us units.
@@ -2658,11 +2653,11 @@ static struct hsw_wm_values *hsw_find_best_result(struct hsw_wm_values *r1,
  */
 static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 				struct hsw_wm_values *results,
-				enum hsw_data_buf_partitioning partitioning)
+				enum intel_ddb_partitioning partitioning)
 {
 	struct hsw_wm_values previous;
 	uint32_t val;
-	enum hsw_data_buf_partitioning prev_partitioning;
+	enum intel_ddb_partitioning prev_partitioning;
 	bool prev_enable_fbc_wm;
 
 	previous.wm_pipe[0] = I915_READ(WM0_PIPEA_ILK);
@@ -2679,7 +2674,7 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 	previous.wm_linetime[2] = I915_READ(PIPE_WM_LINETIME(PIPE_C));
 
 	prev_partitioning = (I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6) ?
-			    HSW_DATA_BUF_PART_5_6 : HSW_DATA_BUF_PART_1_2;
+				INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2;
 
 	prev_enable_fbc_wm = !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
 
@@ -2718,7 +2713,7 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 
 	if (prev_partitioning != partitioning) {
 		val = I915_READ(WM_MISC);
-		if (partitioning == HSW_DATA_BUF_PART_1_2)
+		if (partitioning == INTEL_DDB_PART_1_2)
 			val &= ~WM_MISC_DATA_PARTITION_5_6;
 		else
 			val |= WM_MISC_DATA_PARTITION_5_6;
@@ -2755,7 +2750,7 @@ static void haswell_update_wm(struct drm_device *dev)
 	struct hsw_wm_maximums lp_max_1_2, lp_max_5_6;
 	struct hsw_pipe_wm_parameters params[3];
 	struct hsw_wm_values results_1_2, results_5_6, *best_results;
-	enum hsw_data_buf_partitioning partitioning;
+	enum intel_ddb_partitioning partitioning;
 
 	hsw_compute_wm_parameters(dev, params, &lp_max_1_2, &lp_max_5_6);
 
@@ -2770,7 +2765,7 @@ static void haswell_update_wm(struct drm_device *dev)
 	}
 
 	partitioning = (best_results == &results_1_2) ?
-		       HSW_DATA_BUF_PART_1_2 : HSW_DATA_BUF_PART_5_6;
+		       INTEL_DDB_PART_1_2 : INTEL_DDB_PART_5_6;
 
 	hsw_write_wm_values(dev_priv, best_results, partitioning);
 }
-- 
1.8.1.5

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

[PATCH 06/13] drm/i915: Rename hsw_lp_wm_result to intel_wm_level

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Let's call hsw_lp_wm_result intel_wm_level from now on and move it to
i915_drv.h for later use.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  8 ++++++++
 drivers/gpu/drm/i915/intel_pm.c | 20 ++++++--------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8cc7011..53c38ec 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1055,6 +1055,14 @@ enum intel_ddb_partitioning {
 	INTEL_DDB_PART_5_6, /* IVB+ */
 };
 
+struct intel_wm_level {
+	bool enable;
+	uint32_t pri_val;
+	uint32_t spr_val;
+	uint32_t cur_val;
+	uint32_t fbc_val;
+};
+
 typedef struct drm_i915_private {
 	struct drm_device *dev;
 	struct kmem_cache *slab;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d5ab9f0..797e777 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2180,14 +2180,6 @@ struct hsw_wm_maximums {
 	uint16_t fbc;
 };
 
-struct hsw_lp_wm_result {
-	bool enable;
-	uint32_t pri_val;
-	uint32_t spr_val;
-	uint32_t cur_val;
-	uint32_t fbc_val;
-};
-
 struct hsw_wm_values {
 	uint32_t wm_pipe[3];
 	uint32_t wm_lp[3];
@@ -2280,7 +2272,7 @@ static uint32_t ilk_compute_fbc_wm(struct hsw_pipe_wm_parameters *params,
 
 static bool ilk_check_wm(int level,
 			 const struct hsw_wm_maximums *max,
-			 struct hsw_lp_wm_result *result)
+			 struct intel_wm_level *result)
 {
 	bool ret;
 
@@ -2314,7 +2306,7 @@ static bool ilk_check_wm(int level,
 static void ilk_compute_wm_level(struct drm_i915_private *dev_priv,
 				 int level,
 				 struct hsw_pipe_wm_parameters *p,
-				 struct hsw_lp_wm_result *result)
+				 struct intel_wm_level *result)
 {
 	uint16_t pri_latency = dev_priv->wm.pri_latency[level];
 	uint16_t spr_latency = dev_priv->wm.spr_latency[level];
@@ -2337,10 +2329,10 @@ static void ilk_compute_wm_level(struct drm_i915_private *dev_priv,
 static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
 			      int level, struct hsw_wm_maximums *max,
 			      struct hsw_pipe_wm_parameters *params,
-			      struct hsw_lp_wm_result *result)
+			      struct intel_wm_level *result)
 {
 	enum pipe pipe;
-	struct hsw_lp_wm_result res[3];
+	struct intel_wm_level res[3];
 
 	for (pipe = PIPE_A; pipe <= PIPE_C; pipe++)
 		ilk_compute_wm_level(dev_priv, level, &params[pipe], &res[pipe]);
@@ -2574,7 +2566,7 @@ static void hsw_compute_wm_results(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
-	struct hsw_lp_wm_result lp_results[4] = {};
+	struct intel_wm_level lp_results[4] = {};
 	enum pipe pipe;
 	int level, max_level, wm_lp;
 
@@ -2597,7 +2589,7 @@ static void hsw_compute_wm_results(struct drm_device *dev,
 
 	memset(results, 0, sizeof(*results));
 	for (wm_lp = 1; wm_lp <= 3; wm_lp++) {
-		const struct hsw_lp_wm_result *r;
+		const struct intel_wm_level *r;
 
 		level = (max_level == 4 && wm_lp > 1) ? wm_lp + 1 : wm_lp;
 		if (level > max_level)
-- 
1.8.1.5

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

[PATCH 07/13] drm/i915: Calculate max watermark levels for ILK+

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There are quite a few variables we need to take into account to
determine the maximum watermark levels, so it feels a bit cleaner
to calculate those rather than just have a bunch of what look like
magic numbers.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 119 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 107 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 797e777..f51f89d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2270,6 +2270,104 @@ static uint32_t ilk_compute_fbc_wm(struct hsw_pipe_wm_parameters *params,
 			  params->pri_bytes_per_pixel);
 }
 
+static unsigned int ilk_display_fifo_size(const struct drm_device *dev)
+{
+	if (INTEL_INFO(dev)->gen >= 7)
+		return 768;
+	else
+		return 512;
+}
+
+/* Calculate the maximum primary/sprite plane watermark */
+static unsigned int ilk_plane_wm_max(const struct drm_device *dev,
+				     int level,
+				     unsigned int pipes_active,
+				     bool sprite_enabled,
+				     enum intel_ddb_partitioning ddb_partitioning,
+				     bool is_sprite)
+{
+	unsigned int fifo_size = ilk_display_fifo_size(dev);
+	unsigned int max;
+
+	/* if sprites aren't enabled, sprites get nothing */
+	if (is_sprite && !sprite_enabled)
+		return 0;
+
+	/* HSW allows LP1+ watermarks even with multiple pipes */
+	if (level == 0 || pipes_active > 1) {
+		fifo_size /= INTEL_INFO(dev)->num_pipes;
+
+		/*
+		 * For some reason the non self refresh
+		 * FIFO size is only half of the self
+		 * refresh FIFO size on ILK/SNB.
+		 */
+		if (INTEL_INFO(dev)->gen <= 6)
+			fifo_size /= 2;
+	}
+
+	if (sprite_enabled) {
+		/* level 0 is always calculated with 1:1 split */
+		if (level > 0 && ddb_partitioning == INTEL_DDB_PART_5_6) {
+			if (is_sprite)
+				fifo_size *= 5;
+			fifo_size /= 6;
+		} else {
+			fifo_size /= 2;
+		}
+	}
+
+	/* clamp to max that the registers can hold */
+	if (INTEL_INFO(dev)->gen >= 7)
+		/* IVB/HSW primary/sprite plane watermarks */
+		max = level == 0 ? 127 : 1023;
+	else if (!is_sprite)
+		/* ILK/SNB primary plane watermarks */
+		max = level == 0 ? 127 : 511;
+	else
+		/* ILK/SNB sprite plane watermarks */
+		max = level == 0 ? 63 : 255;
+
+	return min(fifo_size, max);
+}
+
+/* Calculate the maximum cursor plane watermark */
+static unsigned int ilk_cursor_wm_max(const struct drm_device *dev,
+				      int level, unsigned int pipes_active)
+{
+	/* HSW LP1+ watermarks w/ multiple pipes */
+	if (level > 0 && pipes_active > 1)
+		return 64;
+
+	/* othwewise just report max that registers can hold */
+	if (INTEL_INFO(dev)->gen >= 7)
+		return level == 0 ? 63 : 255;
+	else
+		return level == 0 ? 31 : 63;
+}
+
+/* Calculate the maximum FBC watermark */
+static unsigned int ilk_fbc_wm_max(void)
+{
+	/* max that registers can hold */
+	return 15;
+}
+
+static void ilk_wm_max(struct drm_device *dev,
+		       int level,
+		       unsigned int pipes_active,
+		       bool sprite_enabled,
+		       enum intel_ddb_partitioning ddb_partitioning,
+		       struct hsw_wm_maximums *max)
+{
+	max->pri = ilk_plane_wm_max(dev, level, pipes_active,
+				    sprite_enabled, ddb_partitioning, false);
+	max->spr = ilk_plane_wm_max(dev, level, pipes_active,
+				    sprite_enabled, ddb_partitioning, true);
+	max->cur = ilk_cursor_wm_max(dev, level, pipes_active);
+	max->fbc = ilk_fbc_wm_max();
+}
+
 static bool ilk_check_wm(int level,
 			 const struct hsw_wm_maximums *max,
 			 struct intel_wm_level *result)
@@ -2545,18 +2643,15 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 			sprites_enabled++;
 	}
 
-	if (pipes_active > 1) {
-		lp_max_1_2->pri = lp_max_5_6->pri = sprites_enabled ? 128 : 256;
-		lp_max_1_2->spr = lp_max_5_6->spr = 128;
-		lp_max_1_2->cur = lp_max_5_6->cur = 64;
-	} else {
-		lp_max_1_2->pri = sprites_enabled ? 384 : 768;
-		lp_max_5_6->pri = sprites_enabled ? 128 : 768;
-		lp_max_1_2->spr = 384;
-		lp_max_5_6->spr = 640;
-		lp_max_1_2->cur = lp_max_5_6->cur = 255;
-	}
-	lp_max_1_2->fbc = lp_max_5_6->fbc = 15;
+	ilk_wm_max(dev, 1, pipes_active, sprites_enabled,
+		   INTEL_DDB_PART_1_2, lp_max_1_2);
+
+	/* 5/6 split only in single pipe config on IVB+ */
+	if (INTEL_INFO(dev)->gen >= 7 && pipes_active <= 1)
+		ilk_wm_max(dev, 1, pipes_active, sprites_enabled,
+			   INTEL_DDB_PART_5_6, lp_max_5_6);
+	else
+		*lp_max_5_6 = *lp_max_1_2;
 }
 
 static void hsw_compute_wm_results(struct drm_device *dev,
-- 
1.8.1.5

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

[PATCH 08/13] drm/i915; Pull some watermarks state into a separate structure

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There is a bunch of global state that needs to be considered when
checking watermarks for validity. Move most of that to a new
structure intel_wm_config, to avoid having to pass around so
many variables.

One notable thing left out is the DDB partitioning information,
since we often anyway need to check the same watermarks against
both 1/2 and 5/6 DDB partitioning layouts.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 48 +++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f51f89d..014a8ba 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2188,6 +2188,14 @@ struct hsw_wm_values {
 	bool enable_fbc_wm;
 };
 
+/* used in computing the new watermarks state */
+struct intel_wm_config {
+	unsigned int pipes_active;
+	bool sprites_enabled;
+	bool sprites_scaled;
+	bool fbc_wm_enabled;
+};
+
 /*
  * For both WM_PIPE and WM_LP.
  * mem_value must be in 0.1us units.
@@ -2281,8 +2289,7 @@ static unsigned int ilk_display_fifo_size(const struct drm_device *dev)
 /* Calculate the maximum primary/sprite plane watermark */
 static unsigned int ilk_plane_wm_max(const struct drm_device *dev,
 				     int level,
-				     unsigned int pipes_active,
-				     bool sprite_enabled,
+				     const struct intel_wm_config *config,
 				     enum intel_ddb_partitioning ddb_partitioning,
 				     bool is_sprite)
 {
@@ -2290,11 +2297,11 @@ static unsigned int ilk_plane_wm_max(const struct drm_device *dev,
 	unsigned int max;
 
 	/* if sprites aren't enabled, sprites get nothing */
-	if (is_sprite && !sprite_enabled)
+	if (is_sprite && !config->sprites_enabled)
 		return 0;
 
 	/* HSW allows LP1+ watermarks even with multiple pipes */
-	if (level == 0 || pipes_active > 1) {
+	if (level == 0 || config->pipes_active > 1) {
 		fifo_size /= INTEL_INFO(dev)->num_pipes;
 
 		/*
@@ -2306,7 +2313,7 @@ static unsigned int ilk_plane_wm_max(const struct drm_device *dev,
 			fifo_size /= 2;
 	}
 
-	if (sprite_enabled) {
+	if (config->sprites_enabled) {
 		/* level 0 is always calculated with 1:1 split */
 		if (level > 0 && ddb_partitioning == INTEL_DDB_PART_5_6) {
 			if (is_sprite)
@@ -2333,10 +2340,11 @@ static unsigned int ilk_plane_wm_max(const struct drm_device *dev,
 
 /* Calculate the maximum cursor plane watermark */
 static unsigned int ilk_cursor_wm_max(const struct drm_device *dev,
-				      int level, unsigned int pipes_active)
+				      int level,
+				      const struct intel_wm_config *config)
 {
 	/* HSW LP1+ watermarks w/ multiple pipes */
-	if (level > 0 && pipes_active > 1)
+	if (level > 0 && config->pipes_active > 1)
 		return 64;
 
 	/* othwewise just report max that registers can hold */
@@ -2355,16 +2363,13 @@ static unsigned int ilk_fbc_wm_max(void)
 
 static void ilk_wm_max(struct drm_device *dev,
 		       int level,
-		       unsigned int pipes_active,
-		       bool sprite_enabled,
+		       const struct intel_wm_config *config,
 		       enum intel_ddb_partitioning ddb_partitioning,
 		       struct hsw_wm_maximums *max)
 {
-	max->pri = ilk_plane_wm_max(dev, level, pipes_active,
-				    sprite_enabled, ddb_partitioning, false);
-	max->spr = ilk_plane_wm_max(dev, level, pipes_active,
-				    sprite_enabled, ddb_partitioning, true);
-	max->cur = ilk_cursor_wm_max(dev, level, pipes_active);
+	max->pri = ilk_plane_wm_max(dev, level, config, ddb_partitioning, false);
+	max->spr = ilk_plane_wm_max(dev, level, config, ddb_partitioning, true);
+	max->cur = ilk_cursor_wm_max(dev, level, config);
 	max->fbc = ilk_fbc_wm_max();
 }
 
@@ -2604,7 +2609,7 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
 	enum pipe pipe;
-	int pipes_active = 0, sprites_enabled = 0;
+	struct intel_wm_config config = {};
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -2617,7 +2622,7 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 		if (!p->active)
 			continue;
 
-		pipes_active++;
+		config.pipes_active++;
 
 		p->pipe_htotal = intel_crtc->config.adjusted_mode.htotal;
 		p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
@@ -2639,17 +2644,14 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 		p->spr_bytes_per_pixel = intel_plane->wm.bytes_per_pixel;
 		p->spr_horiz_pixels = intel_plane->wm.horiz_pixels;
 
-		if (p->sprite_enabled)
-			sprites_enabled++;
+		config.sprites_enabled |= p->sprite_enabled;
 	}
 
-	ilk_wm_max(dev, 1, pipes_active, sprites_enabled,
-		   INTEL_DDB_PART_1_2, lp_max_1_2);
+	ilk_wm_max(dev, 1, &config, INTEL_DDB_PART_1_2, lp_max_1_2);
 
 	/* 5/6 split only in single pipe config on IVB+ */
-	if (INTEL_INFO(dev)->gen >= 7 && pipes_active <= 1)
-		ilk_wm_max(dev, 1, pipes_active, sprites_enabled,
-			   INTEL_DDB_PART_5_6, lp_max_5_6);
+	if (INTEL_INFO(dev)->gen >= 7 && config.pipes_active <= 1)
+		ilk_wm_max(dev, 1, &config, INTEL_DDB_PART_5_6, lp_max_5_6);
 	else
 		*lp_max_5_6 = *lp_max_1_2;
 }
-- 
1.8.1.5

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

[PATCH 09/13] drm/i915: Split plane watermark parameters into a separate struct

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Give a name to the plane watermark related data we have currently
stored under intel_plane->wm.

We also observe that this data is more or less the same that we have
in the hsw_pipe_wm_parameters structure, so use it there as well.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 14 +++++-----
 drivers/gpu/drm/i915/intel_pm.c  | 57 +++++++++++++++++++---------------------
 2 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ed33976..0366bca 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -330,6 +330,13 @@ struct intel_crtc {
 	bool pch_fifo_underrun_disabled;
 };
 
+struct intel_plane_wm_parameters {
+	bool enabled;
+	bool scaled;
+	uint8_t bytes_per_pixel;
+	uint32_t horiz_pixels;
+};
+
 struct intel_plane {
 	struct drm_plane base;
 	int plane;
@@ -348,12 +355,7 @@ struct intel_plane {
 	 * as the other pieces of the struct may not reflect the values we want
 	 * for the watermark calculations. Currently only Haswell uses this.
 	 */
-	struct {
-		bool enabled;
-		bool scaled;
-		uint8_t bytes_per_pixel;
-		uint32_t horiz_pixels;
-	} wm;
+	struct intel_plane_wm_parameters wm;
 
 	void (*update_plane)(struct drm_plane *plane,
 			     struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 014a8ba..9d6f4bf 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2162,15 +2162,11 @@ static uint32_t ilk_wm_fbc(uint32_t pri_val, uint32_t horiz_pixels,
 
 struct hsw_pipe_wm_parameters {
 	bool active;
-	bool sprite_enabled;
-	uint8_t pri_bytes_per_pixel;
-	uint8_t spr_bytes_per_pixel;
-	uint8_t cur_bytes_per_pixel;
-	uint32_t pri_horiz_pixels;
-	uint32_t spr_horiz_pixels;
-	uint32_t cur_horiz_pixels;
 	uint32_t pipe_htotal;
 	uint32_t pixel_rate;
+	struct intel_plane_wm_parameters pri;
+	struct intel_plane_wm_parameters spr;
+	struct intel_plane_wm_parameters cur;
 };
 
 struct hsw_wm_maximums {
@@ -2206,12 +2202,11 @@ static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
 {
 	uint32_t method1, method2;
 
-	/* TODO: for now, assume the primary plane is always enabled. */
-	if (!params->active)
+	if (!params->active || !params->pri.enabled)
 		return 0;
 
 	method1 = ilk_wm_method1(params->pixel_rate,
-				 params->pri_bytes_per_pixel,
+				 params->pri.bytes_per_pixel,
 				 mem_value);
 
 	if (!is_lp)
@@ -2219,8 +2214,8 @@ static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
 
 	method2 = ilk_wm_method2(params->pixel_rate,
 				 params->pipe_htotal,
-				 params->pri_horiz_pixels,
-				 params->pri_bytes_per_pixel,
+				 params->pri.horiz_pixels,
+				 params->pri.bytes_per_pixel,
 				 mem_value);
 
 	return min(method1, method2);
@@ -2235,16 +2230,16 @@ static uint32_t ilk_compute_spr_wm(struct hsw_pipe_wm_parameters *params,
 {
 	uint32_t method1, method2;
 
-	if (!params->active || !params->sprite_enabled)
+	if (!params->active || !params->spr.enabled)
 		return 0;
 
 	method1 = ilk_wm_method1(params->pixel_rate,
-				 params->spr_bytes_per_pixel,
+				 params->spr.bytes_per_pixel,
 				 mem_value);
 	method2 = ilk_wm_method2(params->pixel_rate,
 				 params->pipe_htotal,
-				 params->spr_horiz_pixels,
-				 params->spr_bytes_per_pixel,
+				 params->spr.horiz_pixels,
+				 params->spr.bytes_per_pixel,
 				 mem_value);
 	return min(method1, method2);
 }
@@ -2256,13 +2251,13 @@ static uint32_t ilk_compute_spr_wm(struct hsw_pipe_wm_parameters *params,
 static uint32_t ilk_compute_cur_wm(struct hsw_pipe_wm_parameters *params,
 				   uint32_t mem_value)
 {
-	if (!params->active)
+	if (!params->active || !params->cur.enabled)
 		return 0;
 
 	return ilk_wm_method2(params->pixel_rate,
 			      params->pipe_htotal,
-			      params->cur_horiz_pixels,
-			      params->cur_bytes_per_pixel,
+			      params->cur.horiz_pixels,
+			      params->cur.bytes_per_pixel,
 			      mem_value);
 }
 
@@ -2270,12 +2265,12 @@ static uint32_t ilk_compute_cur_wm(struct hsw_pipe_wm_parameters *params,
 static uint32_t ilk_compute_fbc_wm(struct hsw_pipe_wm_parameters *params,
 				   uint32_t pri_val)
 {
-	if (!params->active)
+	if (!params->active || !params->pri.enabled)
 		return 0;
 
 	return ilk_wm_fbc(pri_val,
-			  params->pri_horiz_pixels,
-			  params->pri_bytes_per_pixel);
+			  params->pri.horiz_pixels,
+			  params->pri.bytes_per_pixel);
 }
 
 static unsigned int ilk_display_fifo_size(const struct drm_device *dev)
@@ -2626,11 +2621,14 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 
 		p->pipe_htotal = intel_crtc->config.adjusted_mode.htotal;
 		p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
-		p->pri_bytes_per_pixel = crtc->fb->bits_per_pixel / 8;
-		p->cur_bytes_per_pixel = 4;
-		p->pri_horiz_pixels =
+		p->pri.bytes_per_pixel = crtc->fb->bits_per_pixel / 8;
+		p->cur.bytes_per_pixel = 4;
+		p->pri.horiz_pixels =
 			intel_crtc->config.requested_mode.hdisplay;
-		p->cur_horiz_pixels = 64;
+		p->cur.horiz_pixels = 64;
+		/* TODO: for now, assume primary and cursor planes are always enabled. */
+		p->pri.enabled = true;
+		p->cur.enabled = true;
 	}
 
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
@@ -2640,11 +2638,10 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 		pipe = intel_plane->pipe;
 		p = &params[pipe];
 
-		p->sprite_enabled = intel_plane->wm.enabled;
-		p->spr_bytes_per_pixel = intel_plane->wm.bytes_per_pixel;
-		p->spr_horiz_pixels = intel_plane->wm.horiz_pixels;
+		p->spr = intel_plane->wm;
 
-		config.sprites_enabled |= p->sprite_enabled;
+		config.sprites_enabled |= p->spr.enabled;
+		config.sprites_scaled |= p->spr.scaled;
 	}
 
 	ilk_wm_max(dev, 1, &config, INTEL_DDB_PART_1_2, lp_max_1_2);
-- 
1.8.1.5

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

[PATCH 10/13] drm/i915: Pass crtc to our update/disable_plane hooks

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We're going to want to know which CRTC we're dealing with, so pass it
down to the update/disable_plane hooks.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h    |  4 +++-
 drivers/gpu/drm/i915/intel_sprite.c | 21 ++++++++++++---------
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0366bca..2e61665 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -358,13 +358,15 @@ struct intel_plane {
 	struct intel_plane_wm_parameters wm;
 
 	void (*update_plane)(struct drm_plane *plane,
+			     struct drm_crtc *crtc,
 			     struct drm_framebuffer *fb,
 			     struct drm_i915_gem_object *obj,
 			     int crtc_x, int crtc_y,
 			     unsigned int crtc_w, unsigned int crtc_h,
 			     uint32_t x, uint32_t y,
 			     uint32_t src_w, uint32_t src_h);
-	void (*disable_plane)(struct drm_plane *plane);
+	void (*disable_plane)(struct drm_plane *plane,
+			      struct drm_crtc *crtc);
 	int (*update_colorkey)(struct drm_plane *plane,
 			       struct drm_intel_sprite_colorkey *key);
 	void (*get_colorkey)(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 5a36afb..d4e0592 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -38,7 +38,8 @@
 #include "i915_drv.h"
 
 static void
-vlv_update_plane(struct drm_plane *dplane, struct drm_framebuffer *fb,
+vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
+		 struct drm_framebuffer *fb,
 		 struct drm_i915_gem_object *obj, int crtc_x, int crtc_y,
 		 unsigned int crtc_w, unsigned int crtc_h,
 		 uint32_t x, uint32_t y,
@@ -140,7 +141,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_framebuffer *fb,
 }
 
 static void
-vlv_disable_plane(struct drm_plane *dplane)
+vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 {
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -207,7 +208,8 @@ vlv_get_colorkey(struct drm_plane *dplane,
 }
 
 static void
-ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
+ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
+		 struct drm_framebuffer *fb,
 		 struct drm_i915_gem_object *obj, int crtc_x, int crtc_y,
 		 unsigned int crtc_w, unsigned int crtc_h,
 		 uint32_t x, uint32_t y,
@@ -320,7 +322,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
 }
 
 static void
-ivb_disable_plane(struct drm_plane *plane)
+ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -400,7 +402,8 @@ ivb_get_colorkey(struct drm_plane *plane, struct drm_intel_sprite_colorkey *key)
 }
 
 static void
-ilk_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
+ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
+		 struct drm_framebuffer *fb,
 		 struct drm_i915_gem_object *obj, int crtc_x, int crtc_y,
 		 unsigned int crtc_w, unsigned int crtc_h,
 		 uint32_t x, uint32_t y,
@@ -488,7 +491,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
 }
 
 static void
-ilk_disable_plane(struct drm_plane *plane)
+ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 {
 	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -823,11 +826,11 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		intel_enable_primary(crtc);
 
 	if (visible)
-		intel_plane->update_plane(plane, fb, obj,
+		intel_plane->update_plane(plane, crtc, fb, obj,
 					  crtc_x, crtc_y, crtc_w, crtc_h,
 					  src_x, src_y, src_w, src_h);
 	else
-		intel_plane->disable_plane(plane);
+		intel_plane->disable_plane(plane, crtc);
 
 	if (disable_primary)
 		intel_disable_primary(crtc);
@@ -862,7 +865,7 @@ intel_disable_plane(struct drm_plane *plane)
 
 	if (plane->crtc)
 		intel_enable_primary(plane->crtc);
-	intel_plane->disable_plane(plane);
+	intel_plane->disable_plane(plane, plane->crtc);
 
 	if (!intel_plane->obj)
 		goto out;
-- 
1.8.1.5

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

[PATCH 11/13] drm/i915: Don't try to disable plane if it's already disabled

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Check plane->fb in intel_disable_plane() to determine if the plane
is already disabled.

If the plane has an fb, then it must also have a crtc, so we can drop
the plane->crtc check and just call intel_enable_primary() directly.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index d4e0592..35acbde 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -863,8 +863,10 @@ intel_disable_plane(struct drm_plane *plane)
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	int ret = 0;
 
-	if (plane->crtc)
-		intel_enable_primary(plane->crtc);
+	if (!plane->fb)
+		return 0;
+
+	intel_enable_primary(plane->crtc);
 	intel_plane->disable_plane(plane, plane->crtc);
 
 	if (!intel_plane->obj)
-- 
1.8.1.5

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

[PATCH 12/13] drm/i915: Pass plane and crtc to intel_update_sprite_watermarks

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We're going to want to know the crtc in the watermark code to avoid
doing more work than we have to. We should also pass the plane we're
disabling so that we know where to stick our watermark parameters
without having to go look the plane up.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_drv.h    |  3 ++-
 drivers/gpu/drm/i915/intel_pm.c     | 34 ++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_sprite.c |  8 ++++----
 4 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 53c38ec..6aec133 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -359,7 +359,8 @@ struct drm_i915_display_funcs {
 			  struct dpll *match_clock,
 			  struct dpll *best_clock);
 	void (*update_wm)(struct drm_device *dev);
-	void (*update_sprite_wm)(struct drm_device *dev, int pipe,
+	void (*update_sprite_wm)(struct drm_plane *plane,
+				 struct drm_crtc *crtc,
 				 uint32_t sprite_width, int pixel_size,
 				 bool enable, bool scaled);
 	void (*modeset_global_resources)(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2e61665..c3ad388 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -774,7 +774,8 @@ extern void intel_ddi_init(struct drm_device *dev, enum port port);
 
 /* For use by IVB LP watermark workaround in intel_sprite.c */
 extern void intel_update_watermarks(struct drm_device *dev);
-extern void intel_update_sprite_watermarks(struct drm_device *dev, int pipe,
+extern void intel_update_sprite_watermarks(struct drm_plane *plane,
+					   struct drm_crtc *crtc,
 					   uint32_t sprite_width, int pixel_size,
 					   bool enabled, bool scaled);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9d6f4bf..50877c2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2856,25 +2856,19 @@ static void haswell_update_wm(struct drm_device *dev)
 	hsw_write_wm_values(dev_priv, best_results, partitioning);
 }
 
-static void haswell_update_sprite_wm(struct drm_device *dev, int pipe,
+static void haswell_update_sprite_wm(struct drm_plane *plane,
+				     struct drm_crtc *crtc,
 				     uint32_t sprite_width, int pixel_size,
 				     bool enabled, bool scaled)
 {
-	struct drm_plane *plane;
-
-	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
-		struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_plane *intel_plane = to_intel_plane(plane);
 
-		if (intel_plane->pipe == pipe) {
-			intel_plane->wm.enabled = enabled;
-			intel_plane->wm.scaled = scaled;
-			intel_plane->wm.horiz_pixels = sprite_width;
-			intel_plane->wm.bytes_per_pixel = pixel_size;
-			break;
-		}
-	}
+	intel_plane->wm.enabled = enabled;
+	intel_plane->wm.scaled = scaled;
+	intel_plane->wm.horiz_pixels = sprite_width;
+	intel_plane->wm.bytes_per_pixel = pixel_size;
 
-	haswell_update_wm(dev);
+	haswell_update_wm(plane->dev);
 }
 
 static bool
@@ -2953,11 +2947,14 @@ sandybridge_compute_sprite_srwm(struct drm_device *dev, int plane,
 	return *sprite_wm > 0x3ff ? false : true;
 }
 
-static void sandybridge_update_sprite_wm(struct drm_device *dev, int pipe,
+static void sandybridge_update_sprite_wm(struct drm_plane *plane,
+					 struct drm_crtc *crtc,
 					 uint32_t sprite_width, int pixel_size,
 					 bool enabled, bool scaled)
 {
+	struct drm_device *dev = plane->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int pipe = to_intel_plane(plane)->pipe;
 	int latency = dev_priv->wm.spr_latency[0] * 100;	/* In unit 0.1us */
 	u32 val;
 	int sprite_wm, reg;
@@ -3076,14 +3073,15 @@ void intel_update_watermarks(struct drm_device *dev)
 		dev_priv->display.update_wm(dev);
 }
 
-void intel_update_sprite_watermarks(struct drm_device *dev, int pipe,
+void intel_update_sprite_watermarks(struct drm_plane *plane,
+				    struct drm_crtc *crtc,
 				    uint32_t sprite_width, int pixel_size,
 				    bool enabled, bool scaled)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = plane->dev->dev_private;
 
 	if (dev_priv->display.update_sprite_wm)
-		dev_priv->display.update_sprite_wm(dev, pipe, sprite_width,
+		dev_priv->display.update_sprite_wm(plane, crtc, sprite_width,
 						   pixel_size, enabled, scaled);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 35acbde..418bf8d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -109,7 +109,7 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 
 	sprctl |= SP_ENABLE;
 
-	intel_update_sprite_watermarks(dev, pipe, src_w, pixel_size, true,
+	intel_update_sprite_watermarks(dplane, crtc, src_w, pixel_size, true,
 				       src_w != crtc_w || src_h != crtc_h);
 
 	/* Sizes are 0 based */
@@ -265,7 +265,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	if (IS_HASWELL(dev))
 		sprctl |= SPRITE_PIPE_CSC_ENABLE;
 
-	intel_update_sprite_watermarks(dev, pipe, src_w, pixel_size, true,
+	intel_update_sprite_watermarks(plane, crtc, src_w, pixel_size, true,
 				       src_w != crtc_w || src_h != crtc_h);
 
 	/* Sizes are 0 based */
@@ -340,7 +340,7 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	dev_priv->sprite_scaling_enabled &= ~(1 << pipe);
 
-	intel_update_sprite_watermarks(dev, pipe, 0, 0, false, false);
+	intel_update_sprite_watermarks(plane, crtc, 0, 0, false, false);
 
 	/* potentially re-enable LP watermarks */
 	if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled)
@@ -455,7 +455,7 @@ ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		dvscntr |= DVS_TRICKLE_FEED_DISABLE; /* must disable */
 	dvscntr |= DVS_ENABLE;
 
-	intel_update_sprite_watermarks(dev, pipe, src_w, pixel_size, true,
+	intel_update_sprite_watermarks(plane, crtc, src_w, pixel_size, true,
 				       src_w != crtc_w || src_h != crtc_h);
 
 	/* Sizes are 0 based */
-- 
1.8.1.5

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

[PATCH 13/13] drm/i915: Always call intel_update_sprite_watermarks() when disabling a plane

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

ILK and VLV codepaths didn't update sprite watermarks when disabling a
sprite. Make them do that.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 418bf8d..7eb1597 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -154,6 +154,8 @@ vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	/* Activate double buffered register update */
 	I915_MODIFY_DISPBASE(SPSURF(pipe, plane), 0);
 	POSTING_READ(SPSURF(pipe, plane));
+
+	intel_update_sprite_watermarks(dplane, crtc, 0, 0, false, false);
 }
 
 static int
@@ -504,6 +506,8 @@ ilk_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 	/* Flush double buffered register updates */
 	I915_MODIFY_DISPBASE(DVSSURF(pipe), 0);
 	POSTING_READ(DVSSURF(pipe));
+
+	intel_update_sprite_watermarks(plane, crtc, 0, 0, false, false);
 }
 
 static void
-- 
1.8.1.5

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

Re: [PATCH 01/13] drm/i915: Use 'enabled' instead of 'enable' consistentnly in sprite WM code

[Chris Wilson]

s/consistentnly/consistently/ in subject.
-Chris, who is always ashamed of his sentence construction and spelling
in changelogs.

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 02/13] drm/i915: Pull watermark level validity check out

[Chris Wilson]

On Tue, Aug 06, 2013 at 10:24:01PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Refactor the code a bit to split the watermark level validity check into
> a separate function.
> 
> Also add hack there that allows us to use it even for LP0 watermarks.
> ATM we don't pre-compute/check the LP0 watermarks, so we just have to
> clamp them to the maximum and hope things work out.

I don't see where we log the unclamped or post-clamped values. This
would be a good opportunity to improve the logging in a common spot.

Comment inline.

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fe0c2af..f2374e0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2284,6 +2284,36 @@ static uint32_t ilk_compute_fbc_wm(struct hsw_pipe_wm_parameters *params,
>  			  params->pri_bytes_per_pixel);
>  }
>  
> +static bool ilk_check_wm(int level,
> +			 const struct hsw_wm_maximums *max,
> +			 struct hsw_lp_wm_result *result)
> +{
> +	bool ret;
> +
> +	result->enable = result->pri_val <= max->pri &&
> +			 result->spr_val <= max->spr &&
> +			 result->cur_val <= max->cur;
> +
> +	ret = result->enable;
> +
> +	/*
> +	 * HACK until we can pre-compute everything,
> +	 * and thus fail gracefully if LP0 watermarks
> +	 * are exceeded...
> +	 */
> +	if (level == 0 && !result->enable) {
> +		result->pri_val = min_t(uint32_t, result->pri_val, max->pri);
> +		result->spr_val = min_t(uint32_t, result->spr_val, max->spr);
> +		result->cur_val = min_t(uint32_t, result->cur_val, max->cur);
> +		result->enable = true;
> +		ret = false;

ret is already false.

> +	}
> +
> +	DRM_DEBUG_KMS("WM%d: %sabled\n", level, result->enable ? "en" : "dis");
> +
> +	return ret;
> +}
> +

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 08/13] drm/i915; Pull some watermarks state into a separate structure

[Chris Wilson]

On Tue, Aug 06, 2013 at 10:24:07PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> There is a bunch of global state that needs to be considered when
> checking watermarks for validity. Move most of that to a new
> structure intel_wm_config, to avoid having to pass around so
> many variables.
> 
> One notable thing left out is the DDB partitioning information,
> since we often anyway need to check the same watermarks against
> both 1/2 and 5/6 DDB partitioning layouts.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Switching sprites_enabled from a count threw me for a moment there, but
the conversion looks sane.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 13/13] drm/i915: Always call intel_update_sprite_watermarks() when disabling a plane

[Chris Wilson]

On Tue, Aug 06, 2013 at 10:24:12PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> ILK and VLV codepaths didn't update sprite watermarks when disabling a
> sprite. Make them do that.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I am totally loving the naming confusion between intel_plane and
intel_sprite in intel_display.c. There when we refer to a plane, we may
mean a slice of the CRTC pipeline or what we refer elsewhere as a sprite.

e.g. intel_disable_plane and intel_plane_disable.

Having got past that to realise this is intel_sprite.c not
intel_display.c,
Reviewed->by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/13] drm/i915: Split plane watermark parameters into a separate struct

[Chris Wilson]

On Tue, Aug 06, 2013 at 10:24:08PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Give a name to the plane watermark related data we have currently
> stored under intel_plane->wm.
> 
> We also observe that this data is more or less the same that we have
> in the hsw_pipe_wm_parameters structure, so use it there as well.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> ---
>  drivers/gpu/drm/i915/intel_drv.h | 14 +++++-----
>  drivers/gpu/drm/i915/intel_pm.c  | 57 +++++++++++++++++++---------------------
>  2 files changed, 35 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ed33976..0366bca 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -330,6 +330,13 @@ struct intel_crtc {
>  	bool pch_fifo_underrun_disabled;
>  };
>  
> +struct intel_plane_wm_parameters {
> +	bool enabled;
> +	bool scaled;
> +	uint8_t bytes_per_pixel;
> +	uint32_t horiz_pixels;

You are no friend to pa-hole.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 06/13] drm/i915: Rename hsw_lp_wm_result to intel_wm_level

[Chris Wilson]

On Tue, Aug 06, 2013 at 10:24:05PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Let's call hsw_lp_wm_result intel_wm_level from now on and move it to
> i915_drv.h for later use.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 12/13] drm/i915: Pass plane and crtc to intel_update_sprite_watermarks

[Chris Wilson]

On Tue, Aug 06, 2013 at 10:24:11PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We're going to want to know the crtc in the watermark code to avoid
> doing more work than we have to. We should also pass the plane we're
> disabling so that we know where to stick our watermark parameters
> without having to go look the plane up.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Looks a reasonable cleanup.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 11/13] drm/i915: Don't try to disable plane if it's already disabled

[Chris Wilson]

On Tue, Aug 06, 2013 at 10:24:10PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Check plane->fb in intel_disable_plane() to determine if the plane
> is already disabled.
> 
> If the plane has an fb, then it must also have a crtc, so we can drop
> the plane->crtc check and just call intel_enable_primary() directly.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Leave it in as a if (WARN(plane->crtc)) return -EINVAL;
I don't think this is performance critical and it serves as a good
reminder.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 05/13] drm/i915: Rename hsw_data_buf_partitioning to intel_ddb_partitioning

[Chris Wilson]

On Tue, Aug 06, 2013 at 10:24:04PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We're going to use the 1/2 vs. 5/6 split option already on IVB so the
> HSW name is not proper. Just give it an intel_ prefix and move it to
> i915_drv.h so that we can use it there later.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I do prefer DDB here, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 07/13] drm/i915: Calculate max watermark levels for ILK+

[Chris Wilson]

On Tue, Aug 06, 2013 at 10:24:06PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> There are quite a few variables we need to take into account to
> determine the maximum watermark levels, so it feels a bit cleaner
> to calculate those rather than just have a bunch of what look like
> magic numbers.

Any chance I can have num_pipes_active? I'm weird, but I keep expecting
pipes_active to a bitmask. (This comment applies to everywhere we have a
x_active counter :)

s/othwerwise/otherwise/
 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reads well, very well. Checked through a couple of the values, and it
seems consistent,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 10/13] drm/i915: Pass crtc to our update/disable_plane hooks

[Chris Wilson]

On Tue, Aug 06, 2013 at 10:24:09PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We're going to want to know which CRTC we're dealing with, so pass it
> down to the update/disable_plane hooks.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 04/13] drm/i915: Kill fbc_enable from hsw_lp_wm_results

[Chris Wilson]

On Tue, Aug 06, 2013 at 10:24:03PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We don't need to store the FBC WM enabled status in each watermark
> level. We anyway have to reduce it down to a single boolean, so just
> delay checking the FBC WM limit until we're computing the final
> value.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

The pay-off simply being the reduction of one bool in a temporary
struct [x3]?

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 03/13] drm/i915: Split watermark level computation from the code

[Chris Wilson]

On Tue, Aug 06, 2013 at 10:24:02PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Refactor the watermarks computation for one level to a separate
> function. This function will now set the ->enable flat to true,

s/flat/flag/

Though I did think you meant a flatten value at one point.

> even if the watermark level wasn't actually checked yet. In the
> future we will delay the checking so we must consider all unchecked
> watermarks as possibly valid.
> 
> v2: Preserve comment about latency units
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I don't have the context here to confirm passing level to
ilk_compute_pri_wm is exactly what you want, and it seems that level is
implicitly greater than 0 in the original code.

The transformation looks fine by itself. If you can calm my quirms over
the value of level (in the changelog is fine),
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 05/13] drm/i915: Rename hsw_data_buf_partitioning to intel_ddb_partitioning

[Daniel Vetter]

On Tue, Aug 06, 2013 at 09:31:33PM +0100, Chris Wilson wrote:
> On Tue, Aug 06, 2013 at 10:24:04PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We're going to use the 1/2 vs. 5/6 split option already on IVB so the
> > HSW name is not proper. Just give it an intel_ prefix and move it to
> > i915_drv.h so that we can use it there later.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I do prefer DDB here, so
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Merged up to this patch with the exception of patch 2. Since I didn't yet
take that one follow-up patches started to rain conflicts, so I think I'll
wait for the new version.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 03/13] drm/i915: Split watermark level computation from the code

[Ville Syrjälä]

On Tue, Aug 06, 2013 at 09:56:31PM +0100, Chris Wilson wrote:
> On Tue, Aug 06, 2013 at 10:24:02PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Refactor the watermarks computation for one level to a separate
> > function. This function will now set the ->enable flat to true,
> 
> s/flat/flag/
> 
> Though I did think you meant a flatten value at one point.
> 
> > even if the watermark level wasn't actually checked yet. In the
> > future we will delay the checking so we must consider all unchecked
> > watermarks as possibly valid.
> > 
> > v2: Preserve comment about latency units
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I don't have the context here to confirm passing level to
> ilk_compute_pri_wm is exactly what you want, and it seems that level is
> implicitly greater than 0 in the original code.
> 
> The transformation looks fine by itself. If you can calm my quirms over
> the value of level (in the changelog is fine),

Looks like Daniel already took this one. But yeah, I guess we should really
pass 'level > 0' to make it clearer that it's a boolean. If you recall I
had a patch in the original series to pass level all the way down, but I
dropped it since it didn't actually do any good.

As far as level=0, I didn't convert the hsw_compute_wm_pipe() to use the
new function. I could do that as a followup patch.

> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 04/13] drm/i915: Kill fbc_enable from hsw_lp_wm_results

[Ville Syrjälä]

On Tue, Aug 06, 2013 at 09:45:11PM +0100, Chris Wilson wrote:
> On Tue, Aug 06, 2013 at 10:24:03PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We don't need to store the FBC WM enabled status in each watermark
> > level. We anyway have to reduce it down to a single boolean, so just
> > delay checking the FBC WM limit until we're computing the final
> > value.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The pay-off simply being the reduction of one bool in a temporary
> struct [x3]?

I don't remember anymore if I had a better reason originally.

> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC

[PATCH v2] drm/i915: Pull watermark level validity check out

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Refactor the code a bit to split the watermark level validity check into
a separate function.

Also add hack there that allows us to use it even for LP0 watermarks.
ATM we don't pre-compute/check the LP0 watermarks, so we just have to
clamp them to the maximum and hope things work out.

v2: Add some debug prints when we exceed max WM0
    Kill pointless ret = false' assignment.
    Include the check for the already disabled 'result' which
    got shuffled around when the patchs got reorderd

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 51 +++++++++++++++++++++++++++++++++++------
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a5a9959..1dd8f30 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2278,6 +2278,49 @@ static uint32_t ilk_compute_fbc_wm(struct hsw_pipe_wm_parameters *params,
 			  params->pri_bytes_per_pixel);
 }
 
+static bool ilk_check_wm(int level,
+			 const struct hsw_wm_maximums *max,
+			 struct hsw_lp_wm_result *result)
+{
+	bool ret;
+
+	/* already determined to be invalid? */
+	if (!result->enable)
+		return false;
+
+	result->enable = result->pri_val <= max->pri &&
+			 result->spr_val <= max->spr &&
+			 result->cur_val <= max->cur;
+
+	ret = result->enable;
+
+	/*
+	 * HACK until we can pre-compute everything,
+	 * and thus fail gracefully if LP0 watermarks
+	 * are exceeded...
+	 */
+	if (level == 0 && !result->enable) {
+		if (result->pri_val > max->pri)
+			DRM_DEBUG_KMS("Primary WM%d too large %u (max %u)\n",
+				      level, result->pri_val, max->pri);
+		if (result->spr_val > max->spr)
+			DRM_DEBUG_KMS("Sprite WM%d too large %u (max %u)\n",
+				      level, result->spr_val, max->spr);
+		if (result->cur_val > max->cur)
+			DRM_DEBUG_KMS("Cursor WM%d too large %u (max %u)\n",
+				      level, result->cur_val, max->cur);
+
+		result->pri_val = min_t(uint32_t, result->pri_val, max->pri);
+		result->spr_val = min_t(uint32_t, result->spr_val, max->spr);
+		result->cur_val = min_t(uint32_t, result->cur_val, max->cur);
+		result->enable = true;
+	}
+
+	DRM_DEBUG_KMS("WM%d: %sabled\n", level, result->enable ? "en" : "dis");
+
+	return ret;
+}
+
 static void ilk_compute_wm_level(struct drm_i915_private *dev_priv,
 				 int level,
 				 struct hsw_pipe_wm_parameters *p,
@@ -2318,13 +2361,7 @@ static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
 	result->fbc_val = max3(res[0].fbc_val, res[1].fbc_val, res[2].fbc_val);
 	result->enable = true;
 
-	if (!result->enable)
-		return false;
-
-	result->enable = result->pri_val <= max->pri &&
-			 result->spr_val <= max->spr &&
-			 result->cur_val <= max->cur;
-	return result->enable;
+	return ilk_check_wm(level, max, result);
 }
 
 static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv,
-- 
1.8.1.5

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

[PATCH v2] drm/i915: Calculate max watermark levels for ILK+

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There are quite a few variables we need to take into account to
determine the maximum watermark levels, so it feels a bit cleaner
to calculate those rather than just have a bunch of what look like
magic numbers.

v2: s/pipes_active/num_pipes_active
    s/othwewise/otherwise

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 119 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 107 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d10d4c7..5daa32a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2270,6 +2270,104 @@ static uint32_t ilk_compute_fbc_wm(struct hsw_pipe_wm_parameters *params,
 			  params->pri_bytes_per_pixel);
 }
 
+static unsigned int ilk_display_fifo_size(const struct drm_device *dev)
+{
+	if (INTEL_INFO(dev)->gen >= 7)
+		return 768;
+	else
+		return 512;
+}
+
+/* Calculate the maximum primary/sprite plane watermark */
+static unsigned int ilk_plane_wm_max(const struct drm_device *dev,
+				     int level,
+				     unsigned int num_pipes_active,
+				     bool sprite_enabled,
+				     enum intel_ddb_partitioning ddb_partitioning,
+				     bool is_sprite)
+{
+	unsigned int fifo_size = ilk_display_fifo_size(dev);
+	unsigned int max;
+
+	/* if sprites aren't enabled, sprites get nothing */
+	if (is_sprite && !sprite_enabled)
+		return 0;
+
+	/* HSW allows LP1+ watermarks even with multiple pipes */
+	if (level == 0 || num_pipes_active > 1) {
+		fifo_size /= INTEL_INFO(dev)->num_pipes;
+
+		/*
+		 * For some reason the non self refresh
+		 * FIFO size is only half of the self
+		 * refresh FIFO size on ILK/SNB.
+		 */
+		if (INTEL_INFO(dev)->gen <= 6)
+			fifo_size /= 2;
+	}
+
+	if (sprite_enabled) {
+		/* level 0 is always calculated with 1:1 split */
+		if (level > 0 && ddb_partitioning == INTEL_DDB_PART_5_6) {
+			if (is_sprite)
+				fifo_size *= 5;
+			fifo_size /= 6;
+		} else {
+			fifo_size /= 2;
+		}
+	}
+
+	/* clamp to max that the registers can hold */
+	if (INTEL_INFO(dev)->gen >= 7)
+		/* IVB/HSW primary/sprite plane watermarks */
+		max = level == 0 ? 127 : 1023;
+	else if (!is_sprite)
+		/* ILK/SNB primary plane watermarks */
+		max = level == 0 ? 127 : 511;
+	else
+		/* ILK/SNB sprite plane watermarks */
+		max = level == 0 ? 63 : 255;
+
+	return min(fifo_size, max);
+}
+
+/* Calculate the maximum cursor plane watermark */
+static unsigned int ilk_cursor_wm_max(const struct drm_device *dev,
+				      int level, unsigned int num_pipes_active)
+{
+	/* HSW LP1+ watermarks w/ multiple pipes */
+	if (level > 0 && num_pipes_active > 1)
+		return 64;
+
+	/* otherwise just report max that registers can hold */
+	if (INTEL_INFO(dev)->gen >= 7)
+		return level == 0 ? 63 : 255;
+	else
+		return level == 0 ? 31 : 63;
+}
+
+/* Calculate the maximum FBC watermark */
+static unsigned int ilk_fbc_wm_max(void)
+{
+	/* max that registers can hold */
+	return 15;
+}
+
+static void ilk_wm_max(struct drm_device *dev,
+		       int level,
+		       unsigned int num_pipes_active,
+		       bool sprite_enabled,
+		       enum intel_ddb_partitioning ddb_partitioning,
+		       struct hsw_wm_maximums *max)
+{
+	max->pri = ilk_plane_wm_max(dev, level, num_pipes_active,
+				    sprite_enabled, ddb_partitioning, false);
+	max->spr = ilk_plane_wm_max(dev, level, num_pipes_active,
+				    sprite_enabled, ddb_partitioning, true);
+	max->cur = ilk_cursor_wm_max(dev, level, num_pipes_active);
+	max->fbc = ilk_fbc_wm_max();
+}
+
 static bool ilk_check_wm(int level,
 			 const struct hsw_wm_maximums *max,
 			 struct intel_wm_level *result)
@@ -2555,18 +2653,15 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 			sprites_enabled++;
 	}
 
-	if (pipes_active > 1) {
-		lp_max_1_2->pri = lp_max_5_6->pri = sprites_enabled ? 128 : 256;
-		lp_max_1_2->spr = lp_max_5_6->spr = 128;
-		lp_max_1_2->cur = lp_max_5_6->cur = 64;
-	} else {
-		lp_max_1_2->pri = sprites_enabled ? 384 : 768;
-		lp_max_5_6->pri = sprites_enabled ? 128 : 768;
-		lp_max_1_2->spr = 384;
-		lp_max_5_6->spr = 640;
-		lp_max_1_2->cur = lp_max_5_6->cur = 255;
-	}
-	lp_max_1_2->fbc = lp_max_5_6->fbc = 15;
+	ilk_wm_max(dev, 1, pipes_active, sprites_enabled,
+		   INTEL_DDB_PART_1_2, lp_max_1_2);
+
+	/* 5/6 split only in single pipe config on IVB+ */
+	if (INTEL_INFO(dev)->gen >= 7 && pipes_active <= 1)
+		ilk_wm_max(dev, 1, pipes_active, sprites_enabled,
+			   INTEL_DDB_PART_5_6, lp_max_5_6);
+	else
+		*lp_max_5_6 = *lp_max_1_2;
 }
 
 static void hsw_compute_wm_results(struct drm_device *dev,
-- 
1.8.1.5

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

[PATCH v2] drm/i915: Pull some watermarks state into a separate structure

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There is a bunch of global state that needs to be considered when
checking watermarks for validity. Move most of that to a new
structure intel_wm_config, to avoid having to pass around so
many variables.

One notable thing left out is the DDB partitioning information,
since we often anyway need to check the same watermarks against
both 1/2 and 5/6 DDB partitioning layouts.

v2: s/pipes_active/num_pipes_active

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 48 +++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5daa32a..9d087be 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2188,6 +2188,14 @@ struct hsw_wm_values {
 	bool enable_fbc_wm;
 };
 
+/* used in computing the new watermarks state */
+struct intel_wm_config {
+	unsigned int num_pipes_active;
+	bool sprites_enabled;
+	bool sprites_scaled;
+	bool fbc_wm_enabled;
+};
+
 /*
  * For both WM_PIPE and WM_LP.
  * mem_value must be in 0.1us units.
@@ -2281,8 +2289,7 @@ static unsigned int ilk_display_fifo_size(const struct drm_device *dev)
 /* Calculate the maximum primary/sprite plane watermark */
 static unsigned int ilk_plane_wm_max(const struct drm_device *dev,
 				     int level,
-				     unsigned int num_pipes_active,
-				     bool sprite_enabled,
+				     const struct intel_wm_config *config,
 				     enum intel_ddb_partitioning ddb_partitioning,
 				     bool is_sprite)
 {
@@ -2290,11 +2297,11 @@ static unsigned int ilk_plane_wm_max(const struct drm_device *dev,
 	unsigned int max;
 
 	/* if sprites aren't enabled, sprites get nothing */
-	if (is_sprite && !sprite_enabled)
+	if (is_sprite && !config->sprites_enabled)
 		return 0;
 
 	/* HSW allows LP1+ watermarks even with multiple pipes */
-	if (level == 0 || num_pipes_active > 1) {
+	if (level == 0 || config->num_pipes_active > 1) {
 		fifo_size /= INTEL_INFO(dev)->num_pipes;
 
 		/*
@@ -2306,7 +2313,7 @@ static unsigned int ilk_plane_wm_max(const struct drm_device *dev,
 			fifo_size /= 2;
 	}
 
-	if (sprite_enabled) {
+	if (config->sprites_enabled) {
 		/* level 0 is always calculated with 1:1 split */
 		if (level > 0 && ddb_partitioning == INTEL_DDB_PART_5_6) {
 			if (is_sprite)
@@ -2333,10 +2340,11 @@ static unsigned int ilk_plane_wm_max(const struct drm_device *dev,
 
 /* Calculate the maximum cursor plane watermark */
 static unsigned int ilk_cursor_wm_max(const struct drm_device *dev,
-				      int level, unsigned int num_pipes_active)
+				      int level,
+				      const struct intel_wm_config *config)
 {
 	/* HSW LP1+ watermarks w/ multiple pipes */
-	if (level > 0 && num_pipes_active > 1)
+	if (level > 0 && config->num_pipes_active > 1)
 		return 64;
 
 	/* otherwise just report max that registers can hold */
@@ -2355,16 +2363,13 @@ static unsigned int ilk_fbc_wm_max(void)
 
 static void ilk_wm_max(struct drm_device *dev,
 		       int level,
-		       unsigned int num_pipes_active,
-		       bool sprite_enabled,
+		       const struct intel_wm_config *config,
 		       enum intel_ddb_partitioning ddb_partitioning,
 		       struct hsw_wm_maximums *max)
 {
-	max->pri = ilk_plane_wm_max(dev, level, num_pipes_active,
-				    sprite_enabled, ddb_partitioning, false);
-	max->spr = ilk_plane_wm_max(dev, level, num_pipes_active,
-				    sprite_enabled, ddb_partitioning, true);
-	max->cur = ilk_cursor_wm_max(dev, level, num_pipes_active);
+	max->pri = ilk_plane_wm_max(dev, level, config, ddb_partitioning, false);
+	max->spr = ilk_plane_wm_max(dev, level, config, ddb_partitioning, true);
+	max->cur = ilk_cursor_wm_max(dev, level, config);
 	max->fbc = ilk_fbc_wm_max();
 }
 
@@ -2614,7 +2619,7 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
 	enum pipe pipe;
-	int pipes_active = 0, sprites_enabled = 0;
+	struct intel_wm_config config = {};
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -2627,7 +2632,7 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 		if (!p->active)
 			continue;
 
-		pipes_active++;
+		config.num_pipes_active++;
 
 		p->pipe_htotal = intel_crtc->config.adjusted_mode.htotal;
 		p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
@@ -2649,17 +2654,14 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 		p->spr_bytes_per_pixel = intel_plane->wm.bytes_per_pixel;
 		p->spr_horiz_pixels = intel_plane->wm.horiz_pixels;
 
-		if (p->sprite_enabled)
-			sprites_enabled++;
+		config.sprites_enabled |= p->sprite_enabled;
 	}
 
-	ilk_wm_max(dev, 1, pipes_active, sprites_enabled,
-		   INTEL_DDB_PART_1_2, lp_max_1_2);
+	ilk_wm_max(dev, 1, &config, INTEL_DDB_PART_1_2, lp_max_1_2);
 
 	/* 5/6 split only in single pipe config on IVB+ */
-	if (INTEL_INFO(dev)->gen >= 7 && pipes_active <= 1)
-		ilk_wm_max(dev, 1, pipes_active, sprites_enabled,
-			   INTEL_DDB_PART_5_6, lp_max_5_6);
+	if (INTEL_INFO(dev)->gen >= 7 && config.num_pipes_active <= 1)
+		ilk_wm_max(dev, 1, &config, INTEL_DDB_PART_5_6, lp_max_5_6);
 	else
 		*lp_max_5_6 = *lp_max_1_2;
 }
-- 
1.8.1.5

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

[PATCH v2] drm/i915: Split plane watermark parameters into a separate struct

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Give a name to the plane watermark related data we have currently
stored under intel_plane->wm.

We also observe that this data is more or less the same that we have
in the hsw_pipe_wm_parameters structure, so use it there as well.

v2: Make pahole happier

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 14 +++++-----
 drivers/gpu/drm/i915/intel_pm.c  | 57 +++++++++++++++++++---------------------
 2 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7df662b..3ea8e5f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -331,6 +331,13 @@ struct intel_crtc {
 	bool pch_fifo_underrun_disabled;
 };
 
+struct intel_plane_wm_parameters {
+	uint32_t horiz_pixels;
+	uint8_t bytes_per_pixel;
+	bool enabled;
+	bool scaled;
+};
+
 struct intel_plane {
 	struct drm_plane base;
 	int plane;
@@ -349,12 +356,7 @@ struct intel_plane {
 	 * as the other pieces of the struct may not reflect the values we want
 	 * for the watermark calculations. Currently only Haswell uses this.
 	 */
-	struct {
-		bool enabled;
-		bool scaled;
-		uint8_t bytes_per_pixel;
-		uint32_t horiz_pixels;
-	} wm;
+	struct intel_plane_wm_parameters wm;
 
 	void (*update_plane)(struct drm_plane *plane,
 			     struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9d087be..af030e8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2162,15 +2162,11 @@ static uint32_t ilk_wm_fbc(uint32_t pri_val, uint32_t horiz_pixels,
 
 struct hsw_pipe_wm_parameters {
 	bool active;
-	bool sprite_enabled;
-	uint8_t pri_bytes_per_pixel;
-	uint8_t spr_bytes_per_pixel;
-	uint8_t cur_bytes_per_pixel;
-	uint32_t pri_horiz_pixels;
-	uint32_t spr_horiz_pixels;
-	uint32_t cur_horiz_pixels;
 	uint32_t pipe_htotal;
 	uint32_t pixel_rate;
+	struct intel_plane_wm_parameters pri;
+	struct intel_plane_wm_parameters spr;
+	struct intel_plane_wm_parameters cur;
 };
 
 struct hsw_wm_maximums {
@@ -2206,12 +2202,11 @@ static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
 {
 	uint32_t method1, method2;
 
-	/* TODO: for now, assume the primary plane is always enabled. */
-	if (!params->active)
+	if (!params->active || !params->pri.enabled)
 		return 0;
 
 	method1 = ilk_wm_method1(params->pixel_rate,
-				 params->pri_bytes_per_pixel,
+				 params->pri.bytes_per_pixel,
 				 mem_value);
 
 	if (!is_lp)
@@ -2219,8 +2214,8 @@ static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
 
 	method2 = ilk_wm_method2(params->pixel_rate,
 				 params->pipe_htotal,
-				 params->pri_horiz_pixels,
-				 params->pri_bytes_per_pixel,
+				 params->pri.horiz_pixels,
+				 params->pri.bytes_per_pixel,
 				 mem_value);
 
 	return min(method1, method2);
@@ -2235,16 +2230,16 @@ static uint32_t ilk_compute_spr_wm(struct hsw_pipe_wm_parameters *params,
 {
 	uint32_t method1, method2;
 
-	if (!params->active || !params->sprite_enabled)
+	if (!params->active || !params->spr.enabled)
 		return 0;
 
 	method1 = ilk_wm_method1(params->pixel_rate,
-				 params->spr_bytes_per_pixel,
+				 params->spr.bytes_per_pixel,
 				 mem_value);
 	method2 = ilk_wm_method2(params->pixel_rate,
 				 params->pipe_htotal,
-				 params->spr_horiz_pixels,
-				 params->spr_bytes_per_pixel,
+				 params->spr.horiz_pixels,
+				 params->spr.bytes_per_pixel,
 				 mem_value);
 	return min(method1, method2);
 }
@@ -2256,13 +2251,13 @@ static uint32_t ilk_compute_spr_wm(struct hsw_pipe_wm_parameters *params,
 static uint32_t ilk_compute_cur_wm(struct hsw_pipe_wm_parameters *params,
 				   uint32_t mem_value)
 {
-	if (!params->active)
+	if (!params->active || !params->cur.enabled)
 		return 0;
 
 	return ilk_wm_method2(params->pixel_rate,
 			      params->pipe_htotal,
-			      params->cur_horiz_pixels,
-			      params->cur_bytes_per_pixel,
+			      params->cur.horiz_pixels,
+			      params->cur.bytes_per_pixel,
 			      mem_value);
 }
 
@@ -2270,12 +2265,12 @@ static uint32_t ilk_compute_cur_wm(struct hsw_pipe_wm_parameters *params,
 static uint32_t ilk_compute_fbc_wm(struct hsw_pipe_wm_parameters *params,
 				   uint32_t pri_val)
 {
-	if (!params->active)
+	if (!params->active || !params->pri.enabled)
 		return 0;
 
 	return ilk_wm_fbc(pri_val,
-			  params->pri_horiz_pixels,
-			  params->pri_bytes_per_pixel);
+			  params->pri.horiz_pixels,
+			  params->pri.bytes_per_pixel);
 }
 
 static unsigned int ilk_display_fifo_size(const struct drm_device *dev)
@@ -2636,11 +2631,14 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 
 		p->pipe_htotal = intel_crtc->config.adjusted_mode.htotal;
 		p->pixel_rate = ilk_pipe_pixel_rate(dev, crtc);
-		p->pri_bytes_per_pixel = crtc->fb->bits_per_pixel / 8;
-		p->cur_bytes_per_pixel = 4;
-		p->pri_horiz_pixels =
+		p->pri.bytes_per_pixel = crtc->fb->bits_per_pixel / 8;
+		p->cur.bytes_per_pixel = 4;
+		p->pri.horiz_pixels =
 			intel_crtc->config.requested_mode.hdisplay;
-		p->cur_horiz_pixels = 64;
+		p->cur.horiz_pixels = 64;
+		/* TODO: for now, assume primary and cursor planes are always enabled. */
+		p->pri.enabled = true;
+		p->cur.enabled = true;
 	}
 
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
@@ -2650,11 +2648,10 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 		pipe = intel_plane->pipe;
 		p = &params[pipe];
 
-		p->sprite_enabled = intel_plane->wm.enabled;
-		p->spr_bytes_per_pixel = intel_plane->wm.bytes_per_pixel;
-		p->spr_horiz_pixels = intel_plane->wm.horiz_pixels;
+		p->spr = intel_plane->wm;
 
-		config.sprites_enabled |= p->sprite_enabled;
+		config.sprites_enabled |= p->spr.enabled;
+		config.sprites_scaled |= p->spr.scaled;
 	}
 
 	ilk_wm_max(dev, 1, &config, INTEL_DDB_PART_1_2, lp_max_1_2);
-- 
1.8.1.5

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

[PATCH v2] drm/i915: Don't try to disable plane if it's already disabled

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Check plane->fb in intel_disable_plane() to determine if the plane
is already disabled.

If the plane has an fb, then it must also have a crtc, so we can drop
the plane->crtc check and just call intel_enable_primary() directly.

v2: WARN and bail if the plane doesn't have a crtc when it should

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index d4e0592..0a174d7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -863,8 +863,13 @@ intel_disable_plane(struct drm_plane *plane)
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	int ret = 0;
 
-	if (plane->crtc)
-		intel_enable_primary(plane->crtc);
+	if (!plane->fb)
+		return 0;
+
+	if (WARN_ON(!plane->crtc))
+		return -EINVAL;
+
+	intel_enable_primary(plane->crtc);
 	intel_plane->disable_plane(plane, plane->crtc);
 
 	if (!intel_plane->obj)
-- 
1.8.1.5

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

Re: [PATCH v2] drm/i915: Pull watermark level validity check out

[Chris Wilson]

On Wed, Aug 07, 2013 at 01:24:47PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Refactor the code a bit to split the watermark level validity check into
> a separate function.
> 
> Also add hack there that allows us to use it even for LP0 watermarks.
> ATM we don't pre-compute/check the LP0 watermarks, so we just have to
> clamp them to the maximum and hope things work out.
> 
> v2: Add some debug prints when we exceed max WM0
>     Kill pointless ret = false' assignment.
>     Include the check for the already disabled 'result' which
>     got shuffled around when the patchs got reorderd
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I still think we are missing a log entry of what watermark values we
pick, but the most important issue of having to clamp the values is
logged.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 13/13] drm/i915: Always call intel_update_sprite_watermarks() when disabling a plane

[Daniel Vetter]

On Tue, Aug 06, 2013 at 09:06:16PM +0100, Chris Wilson wrote:
> On Tue, Aug 06, 2013 at 10:24:12PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > ILK and VLV codepaths didn't update sprite watermarks when disabling a
> > sprite. Make them do that.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I am totally loving the naming confusion between intel_plane and
> intel_sprite in intel_display.c. There when we refer to a plane, we may
> mean a slice of the CRTC pipeline or what we refer elsewhere as a sprite.
> 
> e.g. intel_disable_plane and intel_plane_disable.
> 
> Having got past that to realise this is intel_sprite.c not
> intel_display.c,
> Reviewed->by: Chris Wilson <chris@chris-wilson.co.uk>

All merged to dinq, thanks for patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch