[PATCH 00/19] drm/i915: More HSW watermark prep work

[PATCH 00/19] drm/i915: More HSW watermark prep work

[ville.syrjala]

Another big set of prep work to get the watermarks ready for the atomic
age.

The main themes here are some new structures to allow us to eventually
pre-compute the watermarks and trying to reduce the overhead of the
watermark update itself.

My plan has been to attempt to update the watermarks from the vblank irq
handler, but I'm not 100% sure that I can do that. I have a big patch set
at [1] that does it (and more), and for the most part it is working. But
I am seeing an occasional underrun (very rarely though). I'm not sure
that's caused by missing the dealine in the vblank handler or something
else.

In my big patch set, the next step is adding the vblank update mechanism,
and after that convert ILK/SNB/IVB over to the HSW watermark code. As I'm
not yet 100% convinced that the vblank thing will work out, I think I will
reverse those steps, so that we can get the ILK+ stuff in while I keep
looking into the underrun issue a bit more.

The big patchset also has some other random bits and pieces (mainly
sprite related), and I'll probably be sending some of that out as well.

[1] git://gitorious.org/vsyrjala/linux.git watermarks_full

[PATCH 01/19] drm/i915: Pass crtc to intel_update_watermarks()

[ville.syrjala]

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

Passing the appropriate crtc to intel_update_watermarks() should help
in avoiding needless work in the future.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 20 ++++++++---------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_pm.c      | 43 +++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_sprite.c  |  6 ++---
 5 files changed, 40 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7594356..ab46eb3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -357,7 +357,7 @@ struct drm_i915_display_funcs {
 			  int target, int refclk,
 			  struct dpll *match_clock,
 			  struct dpll *best_clock);
-	void (*update_wm)(struct drm_device *dev);
+	void (*update_wm)(struct drm_crtc *crtc);
 	void (*update_sprite_wm)(struct drm_plane *plane,
 				 struct drm_crtc *crtc,
 				 uint32_t sprite_width, int pixel_size,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f526ea9..13856ec 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3278,7 +3278,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
 	intel_set_pch_fifo_underrun_reporting(dev, pipe, true);
 
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_enable)
@@ -3388,7 +3388,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	if (intel_crtc->config.has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, true);
 
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	if (intel_crtc->config.has_pch_encoder)
 		dev_priv->display.fdi_link_train(crtc);
@@ -3520,7 +3520,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	}
 
 	intel_crtc->active = false;
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
@@ -3577,7 +3577,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	}
 
 	intel_crtc->active = false;
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
@@ -3677,7 +3677,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	intel_crtc->active = true;
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_pll_enable)
@@ -3722,7 +3722,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	intel_crtc->active = true;
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_enable)
@@ -3806,7 +3806,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc->active = false;
 	intel_update_fbc(dev);
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 }
 
 static void i9xx_crtc_off(struct drm_crtc *crtc)
@@ -4972,7 +4972,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	return ret;
 }
@@ -5860,7 +5860,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	return ret;
 }
@@ -6316,7 +6316,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a38f5b2..8b132e2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -714,7 +714,7 @@ extern void hsw_fdi_link_train(struct drm_crtc *crtc);
 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_watermarks(struct drm_crtc *crtc);
 extern void intel_update_sprite_watermarks(struct drm_plane *plane,
 					   struct drm_crtc *crtc,
 					   uint32_t sprite_width, int pixel_size,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6b1d003..774c57f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1087,10 +1087,10 @@ static struct drm_crtc *single_enabled_crtc(struct drm_device *dev)
 	return enabled;
 }
 
-static void pineview_update_wm(struct drm_device *dev)
+static void pineview_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc;
 	const struct cxsr_latency *latency;
 	u32 reg;
 	unsigned long wm;
@@ -1365,8 +1365,9 @@ static void vlv_update_drain_latency(struct drm_device *dev)
 
 #define single_plane_enabled(mask) is_power_of_2(mask)
 
-static void valleyview_update_wm(struct drm_device *dev)
+static void valleyview_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	static const int sr_latency_ns = 12000;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
@@ -1424,8 +1425,9 @@ static void valleyview_update_wm(struct drm_device *dev)
 		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
 }
 
-static void g4x_update_wm(struct drm_device *dev)
+static void g4x_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	static const int sr_latency_ns = 12000;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
@@ -1476,10 +1478,10 @@ static void g4x_update_wm(struct drm_device *dev)
 		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
 }
 
-static void i965_update_wm(struct drm_device *dev)
+static void i965_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc;
 	int srwm = 1;
 	int cursor_sr = 16;
 
@@ -1541,8 +1543,9 @@ static void i965_update_wm(struct drm_device *dev)
 	I915_WRITE(DSPFW3, (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
 }
 
-static void i9xx_update_wm(struct drm_device *dev)
+static void i9xx_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	const struct intel_watermark_params *wm_info;
 	uint32_t fwater_lo;
@@ -1550,7 +1553,7 @@ static void i9xx_update_wm(struct drm_device *dev)
 	int cwm, srwm = 1;
 	int fifo_size;
 	int planea_wm, planeb_wm;
-	struct drm_crtc *crtc, *enabled = NULL;
+	struct drm_crtc *enabled = NULL;
 
 	if (IS_I945GM(dev))
 		wm_info = &i945_wm_info;
@@ -1658,10 +1661,10 @@ static void i9xx_update_wm(struct drm_device *dev)
 	}
 }
 
-static void i830_update_wm(struct drm_device *dev)
+static void i830_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc;
 	uint32_t fwater_lo;
 	int planea_wm;
 
@@ -1785,8 +1788,9 @@ static bool ironlake_compute_srwm(struct drm_device *dev, int level, int plane,
 				   display, cursor);
 }
 
-static void ironlake_update_wm(struct drm_device *dev)
+static void ironlake_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int fbc_wm, plane_wm, cursor_wm;
 	unsigned int enabled;
@@ -1868,8 +1872,9 @@ static void ironlake_update_wm(struct drm_device *dev)
 	 */
 }
 
-static void sandybridge_update_wm(struct drm_device *dev)
+static void sandybridge_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int latency = dev_priv->wm.pri_latency[0] * 100;	/* In unit 0.1us */
 	u32 val;
@@ -1970,8 +1975,9 @@ static void sandybridge_update_wm(struct drm_device *dev)
 		   cursor_wm);
 }
 
-static void ivybridge_update_wm(struct drm_device *dev)
+static void ivybridge_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int latency = dev_priv->wm.pri_latency[0] * 100;	/* In unit 0.1us */
 	u32 val;
@@ -2841,8 +2847,9 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 		I915_WRITE(WM3_LP_ILK, results->wm_lp[2]);
 }
 
-static void haswell_update_wm(struct drm_device *dev)
+static void haswell_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct hsw_wm_maximums lp_max_1_2, lp_max_5_6;
 	struct hsw_pipe_wm_parameters params[3];
@@ -2879,7 +2886,7 @@ static void haswell_update_sprite_wm(struct drm_plane *plane,
 	intel_plane->wm.horiz_pixels = sprite_width;
 	intel_plane->wm.bytes_per_pixel = pixel_size;
 
-	haswell_update_wm(plane->dev);
+	haswell_update_wm(crtc);
 }
 
 static bool
@@ -3076,12 +3083,12 @@ static void sandybridge_update_sprite_wm(struct drm_plane *plane,
  * We don't use the sprite, so we can ignore that.  And on Crestline we have
  * to set the non-SR watermarks to 8.
  */
-void intel_update_watermarks(struct drm_device *dev)
+void intel_update_watermarks(struct drm_crtc *crtc)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 
 	if (dev_priv->display.update_wm)
-		dev_priv->display.update_wm(dev);
+		dev_priv->display.update_wm(crtc);
 }
 
 void intel_update_sprite_watermarks(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 78b621c..a8a72337 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -285,7 +285,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		dev_priv->sprite_scaling_enabled |= 1 << pipe;
 
 		if (!scaling_was_enabled) {
-			intel_update_watermarks(dev);
+			intel_update_watermarks(crtc);
 			intel_wait_for_vblank(dev, pipe);
 		}
 		sprscale = SPRITE_SCALE_ENABLE | (src_w << 16) | src_h;
@@ -320,7 +320,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	/* potentially re-enable LP watermarks */
 	if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled)
-		intel_update_watermarks(dev);
+		intel_update_watermarks(crtc);
 }
 
 static void
@@ -346,7 +346,7 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	/* potentially re-enable LP watermarks */
 	if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled)
-		intel_update_watermarks(dev);
+		intel_update_watermarks(crtc);
 }
 
 static int
-- 
1.8.1.5

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

[PATCH 02/19] drm/i915: Call intel_update_watermarks() in specific place during modeset

[ville.syrjala]

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

Make the call to intel_update_watermarks() just once or twice during
modeset. Ideally it should happen independently when each plane gets
enabled/disabled, but for now it seems better to keep it in central
place. We can improve things when we get all the planes sorted out
in a better way.

When enabling set up the watermarks just before the pipe is enabled.
And when disabling we need to wait until we've marked the crtc as
inactive.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 13856ec..a5181fe 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3278,8 +3278,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
 	intel_set_pch_fifo_underrun_reporting(dev, pipe, true);
 
-	intel_update_watermarks(crtc);
-
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_enable)
 			encoder->pre_enable(encoder);
@@ -3302,6 +3300,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	 */
 	intel_crtc_load_lut(crtc);
 
+	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe,
 			  intel_crtc->config.has_pch_encoder, false);
 	intel_enable_plane(dev_priv, plane, pipe);
@@ -3388,8 +3387,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	if (intel_crtc->config.has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, true);
 
-	intel_update_watermarks(crtc);
-
 	if (intel_crtc->config.has_pch_encoder)
 		dev_priv->display.fdi_link_train(crtc);
 
@@ -3410,6 +3407,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_ddi_set_pipe_settings(crtc);
 	intel_ddi_enable_transcoder_func(crtc);
 
+	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe,
 			  intel_crtc->config.has_pch_encoder, false);
 	intel_enable_plane(dev_priv, plane, pipe);
@@ -3677,7 +3675,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	intel_crtc->active = true;
-	intel_update_watermarks(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_pll_enable)
@@ -3696,6 +3693,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	intel_crtc_load_lut(crtc);
 
+	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe, false, is_dsi);
 	intel_enable_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
@@ -3722,7 +3720,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	intel_crtc->active = true;
-	intel_update_watermarks(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_enable)
@@ -3734,6 +3731,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	intel_crtc_load_lut(crtc);
 
+	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe, false, false);
 	intel_enable_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
@@ -3805,8 +3803,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 		i9xx_disable_pll(dev_priv, pipe);
 
 	intel_crtc->active = false;
-	intel_update_fbc(dev);
 	intel_update_watermarks(crtc);
+
+	intel_update_fbc(dev);
 }
 
 static void i9xx_crtc_off(struct drm_crtc *crtc)
@@ -4972,8 +4971,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(crtc);
-
 	return ret;
 }
 
@@ -5860,8 +5857,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(crtc);
-
 	return ret;
 }
 
@@ -6316,8 +6311,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(crtc);
-
 	return ret;
 }
 
-- 
1.8.1.5

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

[PATCH 03/19] drm/i915: Constify some watermark data

[ville.syrjala]

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

hsw_pipe_wm_parameters and hsw_wm_maximums typically are read only. Make
them const.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 774c57f..96493dc 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2202,7 +2202,7 @@ struct intel_wm_config {
  * For both WM_PIPE and WM_LP.
  * mem_value must be in 0.1us units.
  */
-static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
+static uint32_t ilk_compute_pri_wm(const struct hsw_pipe_wm_parameters *params,
 				   uint32_t mem_value,
 				   bool is_lp)
 {
@@ -2231,7 +2231,7 @@ static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
  * For both WM_PIPE and WM_LP.
  * mem_value must be in 0.1us units.
  */
-static uint32_t ilk_compute_spr_wm(struct hsw_pipe_wm_parameters *params,
+static uint32_t ilk_compute_spr_wm(const struct hsw_pipe_wm_parameters *params,
 				   uint32_t mem_value)
 {
 	uint32_t method1, method2;
@@ -2254,7 +2254,7 @@ static uint32_t ilk_compute_spr_wm(struct hsw_pipe_wm_parameters *params,
  * For both WM_PIPE and WM_LP.
  * mem_value must be in 0.1us units.
  */
-static uint32_t ilk_compute_cur_wm(struct hsw_pipe_wm_parameters *params,
+static uint32_t ilk_compute_cur_wm(const struct hsw_pipe_wm_parameters *params,
 				   uint32_t mem_value)
 {
 	if (!params->active || !params->cur.enabled)
@@ -2268,7 +2268,7 @@ static uint32_t ilk_compute_cur_wm(struct hsw_pipe_wm_parameters *params,
 }
 
 /* Only for WM_LP. */
-static uint32_t ilk_compute_fbc_wm(struct hsw_pipe_wm_parameters *params,
+static uint32_t ilk_compute_fbc_wm(const struct hsw_pipe_wm_parameters *params,
 				   uint32_t pri_val)
 {
 	if (!params->active || !params->pri.enabled)
@@ -2419,7 +2419,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,
+				 const struct hsw_pipe_wm_parameters *p,
 				 struct intel_wm_level *result)
 {
 	uint16_t pri_latency = dev_priv->wm.pri_latency[level];
@@ -2441,8 +2441,8 @@ 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,
+			      int level, const struct hsw_wm_maximums *max,
+			      const struct hsw_pipe_wm_parameters *params,
 			      struct intel_wm_level *result)
 {
 	enum pipe pipe;
@@ -2462,7 +2462,7 @@ static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
 
 static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv,
 				    enum pipe pipe,
-				    struct hsw_pipe_wm_parameters *params)
+				    const struct hsw_pipe_wm_parameters *params)
 {
 	uint32_t pri_val, cur_val, spr_val;
 	/* WM0 latency values stored in 0.1us units */
@@ -2670,8 +2670,8 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 }
 
 static void hsw_compute_wm_results(struct drm_device *dev,
-				   struct hsw_pipe_wm_parameters *params,
-				   struct hsw_wm_maximums *lp_maximums,
+				   const struct hsw_pipe_wm_parameters *params,
+				   const struct hsw_wm_maximums *lp_maximums,
 				   struct hsw_wm_values *results)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.8.1.5

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

[PATCH 04/19] drm/i915: Use ilk_compute_wm_level to compute WM_PIPE values

[ville.syrjala]

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

Unify the code a bit to use ilk_compute_wm_level for all watermark
levels.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 96493dc..d118ee1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2460,33 +2460,31 @@ static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
 	return ilk_check_wm(level, max, result);
 }
 
-static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv,
-				    enum pipe pipe,
+
+static uint32_t hsw_compute_wm_pipe(struct drm_device *dev,
 				    const struct hsw_pipe_wm_parameters *params)
 {
-	uint32_t pri_val, cur_val, spr_val;
-	/* WM0 latency values stored in 0.1us units */
-	uint16_t pri_latency = dev_priv->wm.pri_latency[0];
-	uint16_t spr_latency = dev_priv->wm.spr_latency[0];
-	uint16_t cur_latency = dev_priv->wm.cur_latency[0];
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_wm_config config = {
+		.num_pipes_active = 1,
+		.sprites_enabled = params->spr.enabled,
+		.sprites_scaled = params->spr.scaled,
+	};
+	struct hsw_wm_maximums max;
+	struct intel_wm_level res;
+
+	if (!params->active)
+		return 0;
+
+	ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
 
-	pri_val = ilk_compute_pri_wm(params, pri_latency, false);
-	spr_val = ilk_compute_spr_wm(params, spr_latency);
-	cur_val = ilk_compute_cur_wm(params, cur_latency);
+	ilk_compute_wm_level(dev_priv, 0, params, &res);
 
-	WARN(pri_val > 127,
-	     "Primary WM error, mode not supported for pipe %c\n",
-	     pipe_name(pipe));
-	WARN(spr_val > 127,
-	     "Sprite WM error, mode not supported for pipe %c\n",
-	     pipe_name(pipe));
-	WARN(cur_val > 63,
-	     "Cursor WM error, mode not supported for pipe %c\n",
-	     pipe_name(pipe));
+	ilk_check_wm(0, &max, &res);
 
-	return (pri_val << WM0_PIPE_PLANE_SHIFT) |
-	       (spr_val << WM0_PIPE_SPRITE_SHIFT) |
-	       cur_val;
+	return (res.pri_val << WM0_PIPE_PLANE_SHIFT) |
+	       (res.spr_val << WM0_PIPE_SPRITE_SHIFT) |
+	       res.cur_val;
 }
 
 static uint32_t
@@ -2715,7 +2713,7 @@ static void hsw_compute_wm_results(struct drm_device *dev,
 	}
 
 	for_each_pipe(pipe)
-		results->wm_pipe[pipe] = hsw_compute_wm_pipe(dev_priv, pipe,
+		results->wm_pipe[pipe] = hsw_compute_wm_pipe(dev,
 							     &params[pipe]);
 
 	for_each_pipe(pipe) {
-- 
1.8.1.5

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

[PATCH 05/19] drm/i915: Refactor max WM level

[ville.syrjala]

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

Pull the expected max WM level determinations out to a separate
function. Will have another user soon.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d118ee1..163ba74 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2558,19 +2558,22 @@ static void intel_fixup_cur_wm_latency(struct drm_device *dev, uint16_t wm[5])
 		wm[3] *= 2;
 }
 
-static void intel_print_wm_latency(struct drm_device *dev,
-				   const char *name,
-				   const uint16_t wm[5])
+static int ilk_wm_max_level(const struct drm_device *dev)
 {
-	int level, max_level;
-
 	/* how many WM levels are we expecting */
 	if (IS_HASWELL(dev))
-		max_level = 4;
+		return 4;
 	else if (INTEL_INFO(dev)->gen >= 6)
-		max_level = 3;
+		return 3;
 	else
-		max_level = 2;
+		return 2;
+}
+
+static void intel_print_wm_latency(struct drm_device *dev,
+				   const char *name,
+				   const uint16_t wm[5])
+{
+	int level, max_level = ilk_wm_max_level(dev);
 
 	for (level = 0; level <= max_level; level++) {
 		unsigned int latency = wm[level];
-- 
1.8.1.5

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

[PATCH 06/19] drm/i915: Add intel_pipe_wm and prepare for watermark pre-compute

[ville.syrjala]

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

Introduce a new struct intel_pipe_wm which contains all the
watermarks for a single pipe. Use it to unify the LP0 and LP1+
watermark computations so that we can just iterate through the
watermark levels neatly and call ilk_compute_wm_level() for each.

Also add another tool ilk_wm_merge() that merges the LP1+ watermarks
from all pipes. For that, embed one intel_pipe_wm inside intel_crtc that
contains the currently valid watermarks for each pipe.

This is mainly preparatory work for pre-computing the watermarks for
each pipe and merging them at a later time. For now the merging still
happens immediately.

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

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8b132e2..664df77 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -293,6 +293,12 @@ struct intel_crtc_config {
 	bool ips_enabled;
 };
 
+struct intel_pipe_wm {
+	struct intel_wm_level wm[5];
+	uint32_t linetime;
+	bool fbc_wm_enabled;
+};
+
 struct intel_crtc {
 	struct drm_crtc base;
 	enum pipe pipe;
@@ -333,6 +339,12 @@ struct intel_crtc {
 	/* Access to these should be protected by dev_priv->irq_lock. */
 	bool cpu_fifo_underrun_disabled;
 	bool pch_fifo_underrun_disabled;
+
+	/* per-pipe watermark state */
+	struct {
+		/* watermarks currently being used  */
+		struct intel_pipe_wm active;
+	} wm;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 163ba74..c6f105f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2440,53 +2440,6 @@ static void ilk_compute_wm_level(struct drm_i915_private *dev_priv,
 	result->enable = true;
 }
 
-static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
-			      int level, const struct hsw_wm_maximums *max,
-			      const struct hsw_pipe_wm_parameters *params,
-			      struct intel_wm_level *result)
-{
-	enum pipe pipe;
-	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]);
-
-	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;
-
-	return ilk_check_wm(level, max, result);
-}
-
-
-static uint32_t hsw_compute_wm_pipe(struct drm_device *dev,
-				    const struct hsw_pipe_wm_parameters *params)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_wm_config config = {
-		.num_pipes_active = 1,
-		.sprites_enabled = params->spr.enabled,
-		.sprites_scaled = params->spr.scaled,
-	};
-	struct hsw_wm_maximums max;
-	struct intel_wm_level res;
-
-	if (!params->active)
-		return 0;
-
-	ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
-
-	ilk_compute_wm_level(dev_priv, 0, params, &res);
-
-	ilk_check_wm(0, &max, &res);
-
-	return (res.pri_val << WM0_PIPE_PLANE_SHIFT) |
-	       (res.spr_val << WM0_PIPE_SPRITE_SHIFT) |
-	       res.cur_val;
-}
-
 static uint32_t
 hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
 {
@@ -2670,44 +2623,121 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 		*lp_max_5_6 = *lp_max_1_2;
 }
 
+/* Compute new watermarks for the pipe */
+static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
+				  const struct hsw_pipe_wm_parameters *params,
+				  struct intel_pipe_wm *pipe_wm)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int level, max_level = ilk_wm_max_level(dev);
+	struct intel_wm_config config = {
+		.num_pipes_active = 1,
+		.sprites_enabled = params->spr.enabled,
+		.sprites_scaled = params->spr.scaled,
+	};
+	struct hsw_wm_maximums max;
+
+	memset(pipe_wm, 0, sizeof(*pipe_wm));
+
+	ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
+
+	for (level = 0; level <= max_level; level++)
+		ilk_compute_wm_level(dev_priv, level, params,
+				     &pipe_wm->wm[level]);
+
+	pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
+
+	/* At least LP0 must be valid */
+	return ilk_check_wm(0, &max, &pipe_wm->wm[0]);
+}
+
+/*
+ * Merge the watermarks from all active pipes for a specific level.
+ */
+static void ilk_merge_wm_level(struct drm_device *dev,
+			       int level,
+			       struct intel_wm_level *ret_wm)
+{
+	const struct intel_crtc *intel_crtc;
+
+	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
+		const struct intel_wm_level *wm =
+			&intel_crtc->wm.active.wm[level];
+
+		if (!wm->enable)
+			return;
+
+		ret_wm->pri_val = max(ret_wm->pri_val, wm->pri_val);
+		ret_wm->spr_val = max(ret_wm->spr_val, wm->spr_val);
+		ret_wm->cur_val = max(ret_wm->cur_val, wm->cur_val);
+		ret_wm->fbc_val = max(ret_wm->fbc_val, wm->fbc_val);
+	}
+
+	ret_wm->enable = true;
+}
+
+/*
+ * Merge all low power watermarks for all active pipes.
+ */
+static void ilk_wm_merge(struct drm_device *dev,
+			 const struct hsw_wm_maximums *max,
+			 struct intel_pipe_wm *merged)
+{
+	int level, max_level = ilk_wm_max_level(dev);
+
+	merged->fbc_wm_enabled = true;
+
+	/* merge each WM1+ level */
+	for (level = 1; level <= max_level; level++) {
+		struct intel_wm_level *wm = &merged->wm[level];
+
+		ilk_merge_wm_level(dev, level, wm);
+
+		if (!ilk_check_wm(level, max, wm))
+			break;
+
+		/*
+		 * The spec says it is preferred to disable
+		 * FBC WMs instead of disabling a WM level.
+		 */
+		if (wm->fbc_val > max->fbc) {
+			merged->fbc_wm_enabled = false;
+			wm->fbc_val = 0;
+		}
+	}
+}
+
 static void hsw_compute_wm_results(struct drm_device *dev,
 				   const struct hsw_pipe_wm_parameters *params,
 				   const struct hsw_wm_maximums *lp_maximums,
 				   struct hsw_wm_values *results)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc;
-	struct intel_wm_level lp_results[4] = {};
-	enum pipe pipe;
-	int level, max_level, wm_lp;
+	struct intel_crtc *intel_crtc;
+	int level, wm_lp;
+	struct intel_pipe_wm lp_wm = {};
 
-	for (level = 1; level <= 4; level++)
-		if (!hsw_compute_lp_wm(dev_priv, level,
-				       lp_maximums, params,
-				       &lp_results[level - 1]))
-			break;
-	max_level = level - 1;
+	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
+		intel_compute_pipe_wm(&intel_crtc->base,
+				      &params[intel_crtc->pipe],
+				      &intel_crtc->wm.active);
+
+	ilk_wm_merge(dev, lp_maximums, &lp_wm);
 
 	memset(results, 0, sizeof(*results));
 
-	/* The spec says it is preferred to disable FBC WMs instead of disabling
-	 * a WM level. */
-	results->enable_fbc_wm = true;
-	for (level = 1; level <= max_level; level++) {
-		if (lp_results[level - 1].fbc_val > lp_maximums->fbc) {
-			results->enable_fbc_wm = false;
-			lp_results[level - 1].fbc_val = 0;
-		}
-	}
+	results->enable_fbc_wm = lp_wm.fbc_wm_enabled;
 
+	/* LP1+ register values */
 	for (wm_lp = 1; wm_lp <= 3; wm_lp++) {
 		const struct intel_wm_level *r;
 
-		level = (max_level == 4 && wm_lp > 1) ? wm_lp + 1 : wm_lp;
-		if (level > max_level)
+		level = wm_lp + (wm_lp >= 2 && lp_wm.wm[4].enable);
+
+		r = &lp_wm.wm[level];
+		if (!r->enable)
 			break;
 
-		r = &lp_results[level - 1];
 		results->wm_lp[wm_lp - 1] = HSW_WM_LP_VAL(level * 2,
 							  r->fbc_val,
 							  r->pri_val,
@@ -2715,13 +2745,21 @@ static void hsw_compute_wm_results(struct drm_device *dev,
 		results->wm_lp_spr[wm_lp - 1] = r->spr_val;
 	}
 
-	for_each_pipe(pipe)
-		results->wm_pipe[pipe] = hsw_compute_wm_pipe(dev,
-							     &params[pipe]);
+	/* LP0 register values */
+	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
+		enum pipe pipe = intel_crtc->pipe;
+		const struct intel_wm_level *r =
+			&intel_crtc->wm.active.wm[0];
 
-	for_each_pipe(pipe) {
-		crtc = dev_priv->pipe_to_crtc_mapping[pipe];
-		results->wm_linetime[pipe] = hsw_compute_linetime_wm(dev, crtc);
+		if (!r->enable)
+			continue;
+
+		results->wm_linetime[pipe] = intel_crtc->wm.active.linetime;
+
+		results->wm_pipe[pipe] =
+			(r->pri_val << WM0_PIPE_PLANE_SHIFT) |
+			(r->spr_val << WM0_PIPE_SPRITE_SHIFT) |
+			r->cur_val;
 	}
 }
 
-- 
1.8.1.5

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

[PATCH 07/19] drm/i915: Don't re-compute pipe watermarks except for the affected pipe

[ville.syrjala]

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

No point in re-computing the watermarks for all pipes, when only one
pipe has changed. The watermarks stored under intel_crtc.wm.active are
still valid for the other pipes. We just need to redo the merging.

We can also skip the merge/update procedure completely if the new
watermarks for the affected pipe come out unchanged.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c6f105f..1fb904a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2566,29 +2566,19 @@ static void intel_setup_wm_latency(struct drm_device *dev)
 	intel_print_wm_latency(dev, "Cursor", dev_priv->wm.cur_latency);
 }
 
-static void hsw_compute_wm_parameters(struct drm_device *dev,
-				      struct hsw_pipe_wm_parameters *params,
+static void hsw_compute_wm_parameters(struct drm_crtc *crtc,
+				      struct hsw_pipe_wm_parameters *p,
 				      struct hsw_wm_maximums *lp_max_1_2,
 				      struct hsw_wm_maximums *lp_max_5_6)
 {
-	struct drm_crtc *crtc;
-	struct drm_plane *plane;
-	enum pipe pipe;
+	struct drm_device *dev = crtc->dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
 	struct intel_wm_config config = {};
+	struct drm_plane *plane;
 
-	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-		struct hsw_pipe_wm_parameters *p;
-
-		pipe = intel_crtc->pipe;
-		p = &params[pipe];
-
-		p->active = intel_crtc_active(crtc);
-		if (!p->active)
-			continue;
-
-		config.num_pipes_active++;
-
+	p->active = intel_crtc_active(crtc);
+	if (p->active) {
 		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;
@@ -2601,17 +2591,17 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
 		p->cur.enabled = true;
 	}
 
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+		config.num_pipes_active += intel_crtc_active(crtc);
+
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
 		struct intel_plane *intel_plane = to_intel_plane(plane);
-		struct hsw_pipe_wm_parameters *p;
-
-		pipe = intel_plane->pipe;
-		p = &params[pipe];
 
-		p->spr = intel_plane->wm;
+		if (intel_plane->pipe == pipe)
+			p->spr = intel_plane->wm;
 
-		config.sprites_enabled |= p->spr.enabled;
-		config.sprites_scaled |= p->spr.scaled;
+		config.sprites_enabled |= intel_plane->wm.enabled;
+		config.sprites_scaled |= intel_plane->wm.scaled;
 	}
 
 	ilk_wm_max(dev, 1, &config, INTEL_DDB_PART_1_2, lp_max_1_2);
@@ -2638,8 +2628,6 @@ static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
 	};
 	struct hsw_wm_maximums max;
 
-	memset(pipe_wm, 0, sizeof(*pipe_wm));
-
 	ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
 
 	for (level = 0; level <= max_level; level++)
@@ -2709,7 +2697,6 @@ static void ilk_wm_merge(struct drm_device *dev,
 }
 
 static void hsw_compute_wm_results(struct drm_device *dev,
-				   const struct hsw_pipe_wm_parameters *params,
 				   const struct hsw_wm_maximums *lp_maximums,
 				   struct hsw_wm_values *results)
 {
@@ -2717,11 +2704,6 @@ static void hsw_compute_wm_results(struct drm_device *dev,
 	int level, wm_lp;
 	struct intel_pipe_wm lp_wm = {};
 
-	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
-		intel_compute_pipe_wm(&intel_crtc->base,
-				      &params[intel_crtc->pipe],
-				      &intel_crtc->wm.active);
-
 	ilk_wm_merge(dev, lp_maximums, &lp_wm);
 
 	memset(results, 0, sizeof(*results));
@@ -2888,20 +2870,27 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 
 static void haswell_update_wm(struct drm_crtc *crtc)
 {
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct hsw_wm_maximums lp_max_1_2, lp_max_5_6;
-	struct hsw_pipe_wm_parameters params[3];
+	struct hsw_pipe_wm_parameters params = {};
 	struct hsw_wm_values results_1_2, results_5_6, *best_results;
 	enum intel_ddb_partitioning partitioning;
+	struct intel_pipe_wm pipe_wm = {};
+
+	hsw_compute_wm_parameters(crtc, &params, &lp_max_1_2, &lp_max_5_6);
+
+	intel_compute_pipe_wm(crtc, &params, &pipe_wm);
+
+	if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm)))
+		return;
 
-	hsw_compute_wm_parameters(dev, params, &lp_max_1_2, &lp_max_5_6);
+	intel_crtc->wm.active = pipe_wm;
 
-	hsw_compute_wm_results(dev, params,
-			       &lp_max_1_2, &results_1_2);
+	hsw_compute_wm_results(dev, &lp_max_1_2, &results_1_2);
 	if (lp_max_1_2.pri != lp_max_5_6.pri) {
-		hsw_compute_wm_results(dev, params,
-				       &lp_max_5_6, &results_5_6);
+		hsw_compute_wm_results(dev, &lp_max_5_6, &results_5_6);
 		best_results = hsw_find_best_result(&results_1_2, &results_5_6);
 	} else {
 		best_results = &results_1_2;
-- 
1.8.1.5

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

[PATCH 08/19] drm/i915: Move LP1+ watermark merging out from hsw_compute_wm_results()

[ville.syrjala]

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

I want to convert hsw_find_best_result() to use intel_pipe_wm, so we
need to move the merging to happen outside hsw_compute_wm_results().

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1fb904a..b538d09 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2697,26 +2697,23 @@ static void ilk_wm_merge(struct drm_device *dev,
 }
 
 static void hsw_compute_wm_results(struct drm_device *dev,
-				   const struct hsw_wm_maximums *lp_maximums,
+				   const struct intel_pipe_wm *lp_wm,
 				   struct hsw_wm_values *results)
 {
 	struct intel_crtc *intel_crtc;
 	int level, wm_lp;
-	struct intel_pipe_wm lp_wm = {};
-
-	ilk_wm_merge(dev, lp_maximums, &lp_wm);
 
 	memset(results, 0, sizeof(*results));
 
-	results->enable_fbc_wm = lp_wm.fbc_wm_enabled;
+	results->enable_fbc_wm = lp_wm->fbc_wm_enabled;
 
 	/* LP1+ register values */
 	for (wm_lp = 1; wm_lp <= 3; wm_lp++) {
 		const struct intel_wm_level *r;
 
-		level = wm_lp + (wm_lp >= 2 && lp_wm.wm[4].enable);
+		level = wm_lp + (wm_lp >= 2 && lp_wm->wm[4].enable);
 
-		r = &lp_wm.wm[level];
+		r = &lp_wm->wm[level];
 		if (!r->enable)
 			break;
 
@@ -2878,6 +2875,7 @@ static void haswell_update_wm(struct drm_crtc *crtc)
 	struct hsw_wm_values results_1_2, results_5_6, *best_results;
 	enum intel_ddb_partitioning partitioning;
 	struct intel_pipe_wm pipe_wm = {};
+	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {};
 
 	hsw_compute_wm_parameters(crtc, &params, &lp_max_1_2, &lp_max_5_6);
 
@@ -2888,9 +2886,12 @@ static void haswell_update_wm(struct drm_crtc *crtc)
 
 	intel_crtc->wm.active = pipe_wm;
 
-	hsw_compute_wm_results(dev, &lp_max_1_2, &results_1_2);
+	ilk_wm_merge(dev, &lp_max_1_2, &lp_wm_1_2);
+	ilk_wm_merge(dev, &lp_max_5_6, &lp_wm_5_6);
+
+	hsw_compute_wm_results(dev, &lp_wm_1_2, &results_1_2);
 	if (lp_max_1_2.pri != lp_max_5_6.pri) {
-		hsw_compute_wm_results(dev, &lp_max_5_6, &results_5_6);
+		hsw_compute_wm_results(dev, &lp_wm_5_6, &results_5_6);
 		best_results = hsw_find_best_result(&results_1_2, &results_5_6);
 	} else {
 		best_results = &results_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/19] drm/i915: Use intel_pipe_wm in hsw_find_best_results

[ville.syrjala]

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

Let's try to keep using the intermediate intel_pipe_wm representation
for as long as possible. It avoids subtle knowledge about the
internals of the hardware registers when trying to choose the
best watermark configuration.

While at it replace the memset() w/ zero initialization.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.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 b538d09..d1184f9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2703,8 +2703,6 @@ static void hsw_compute_wm_results(struct drm_device *dev,
 	struct intel_crtc *intel_crtc;
 	int level, wm_lp;
 
-	memset(results, 0, sizeof(*results));
-
 	results->enable_fbc_wm = lp_wm->fbc_wm_enabled;
 
 	/* LP1+ register values */
@@ -2744,24 +2742,26 @@ static void hsw_compute_wm_results(struct drm_device *dev,
 
 /* Find the result with the highest level enabled. Check for enable_fbc_wm in
  * case both are at the same level. Prefer r1 in case they're the same. */
-static struct hsw_wm_values *hsw_find_best_result(struct hsw_wm_values *r1,
-						  struct hsw_wm_values *r2)
+static struct intel_pipe_wm *hsw_find_best_result(struct drm_device *dev,
+						  struct intel_pipe_wm *r1,
+						  struct intel_pipe_wm *r2)
 {
-	int i, val_r1 = 0, val_r2 = 0;
+	int level, max_level = ilk_wm_max_level(dev);
+	int level1 = 0, level2 = 0;
 
-	for (i = 0; i < 3; i++) {
-		if (r1->wm_lp[i] & WM3_LP_EN)
-			val_r1 = r1->wm_lp[i] & WM1_LP_LATENCY_MASK;
-		if (r2->wm_lp[i] & WM3_LP_EN)
-			val_r2 = r2->wm_lp[i] & WM1_LP_LATENCY_MASK;
+	for (level = 1; level <= max_level; level++) {
+		if (r1->wm[level].enable)
+			level1 = level;
+		if (r2->wm[level].enable)
+			level2 = level;
 	}
 
-	if (val_r1 == val_r2) {
-		if (r2->enable_fbc_wm && !r1->enable_fbc_wm)
+	if (level1 == level2) {
+		if (r2->fbc_wm_enabled && !r1->fbc_wm_enabled)
 			return r2;
 		else
 			return r1;
-	} else if (val_r1 > val_r2) {
+	} else if (level1 > level2) {
 		return r1;
 	} else {
 		return r2;
@@ -2872,10 +2872,10 @@ static void haswell_update_wm(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct hsw_wm_maximums lp_max_1_2, lp_max_5_6;
 	struct hsw_pipe_wm_parameters params = {};
-	struct hsw_wm_values results_1_2, results_5_6, *best_results;
+	struct hsw_wm_values results = {};
 	enum intel_ddb_partitioning partitioning;
 	struct intel_pipe_wm pipe_wm = {};
-	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {};
+	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
 
 	hsw_compute_wm_parameters(crtc, &params, &lp_max_1_2, &lp_max_5_6);
 
@@ -2889,18 +2889,18 @@ static void haswell_update_wm(struct drm_crtc *crtc)
 	ilk_wm_merge(dev, &lp_max_1_2, &lp_wm_1_2);
 	ilk_wm_merge(dev, &lp_max_5_6, &lp_wm_5_6);
 
-	hsw_compute_wm_results(dev, &lp_wm_1_2, &results_1_2);
 	if (lp_max_1_2.pri != lp_max_5_6.pri) {
-		hsw_compute_wm_results(dev, &lp_wm_5_6, &results_5_6);
-		best_results = hsw_find_best_result(&results_1_2, &results_5_6);
+		best_lp_wm = hsw_find_best_result(dev, &lp_wm_1_2, &lp_wm_5_6);
 	} else {
-		best_results = &results_1_2;
+		best_lp_wm = &lp_wm_1_2;
 	}
 
-	partitioning = (best_results == &results_1_2) ?
+	hsw_compute_wm_results(dev, best_lp_wm, &results);
+
+	partitioning = (best_lp_wm == &lp_wm_1_2) ?
 		       INTEL_DDB_PART_1_2 : INTEL_DDB_PART_5_6;
 
-	hsw_write_wm_values(dev_priv, best_results, partitioning);
+	hsw_write_wm_values(dev_priv, &results, partitioning);
 }
 
 static void haswell_update_sprite_wm(struct drm_plane *plane,
-- 
1.8.1.5

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

[PATCH 10/19] drm/i915: Move some computations out from hsw_compute_wm_parameters()

[ville.syrjala]

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

Move the watermark max computations into haswell_update_wm(). This
allows keeping the 1/2 vs. 5/6 split code in one place, and avoid having
to pass around so many things. We also save a bit of stack space by only
requiring one copy of struct hsw_wm_maximums.

Also move the intel_wm_config out from hsw_compute_wm_parameters() and
pass it it. We'll have some need for it in haswell_update_wm() later.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d1184f9..5c5fba4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2568,13 +2568,11 @@ static void intel_setup_wm_latency(struct drm_device *dev)
 
 static void hsw_compute_wm_parameters(struct drm_crtc *crtc,
 				      struct hsw_pipe_wm_parameters *p,
-				      struct hsw_wm_maximums *lp_max_1_2,
-				      struct hsw_wm_maximums *lp_max_5_6)
+				      struct intel_wm_config *config)
 {
 	struct drm_device *dev = crtc->dev;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
-	struct intel_wm_config config = {};
 	struct drm_plane *plane;
 
 	p->active = intel_crtc_active(crtc);
@@ -2592,7 +2590,7 @@ static void hsw_compute_wm_parameters(struct drm_crtc *crtc,
 	}
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
-		config.num_pipes_active += intel_crtc_active(crtc);
+		config->num_pipes_active += intel_crtc_active(crtc);
 
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
 		struct intel_plane *intel_plane = to_intel_plane(plane);
@@ -2600,17 +2598,9 @@ static void hsw_compute_wm_parameters(struct drm_crtc *crtc,
 		if (intel_plane->pipe == pipe)
 			p->spr = intel_plane->wm;
 
-		config.sprites_enabled |= intel_plane->wm.enabled;
-		config.sprites_scaled |= intel_plane->wm.scaled;
+		config->sprites_enabled |= intel_plane->wm.enabled;
+		config->sprites_scaled |= intel_plane->wm.scaled;
 	}
-
-	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 && 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;
 }
 
 /* Compute new watermarks for the pipe */
@@ -2870,14 +2860,15 @@ static void haswell_update_wm(struct drm_crtc *crtc)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct hsw_wm_maximums lp_max_1_2, lp_max_5_6;
+	struct hsw_wm_maximums max;
 	struct hsw_pipe_wm_parameters params = {};
 	struct hsw_wm_values results = {};
 	enum intel_ddb_partitioning partitioning;
 	struct intel_pipe_wm pipe_wm = {};
 	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
+	struct intel_wm_config config = {};
 
-	hsw_compute_wm_parameters(crtc, &params, &lp_max_1_2, &lp_max_5_6);
+	hsw_compute_wm_parameters(crtc, &params, &config);
 
 	intel_compute_pipe_wm(crtc, &params, &pipe_wm);
 
@@ -2886,10 +2877,14 @@ static void haswell_update_wm(struct drm_crtc *crtc)
 
 	intel_crtc->wm.active = pipe_wm;
 
-	ilk_wm_merge(dev, &lp_max_1_2, &lp_wm_1_2);
-	ilk_wm_merge(dev, &lp_max_5_6, &lp_wm_5_6);
+	ilk_wm_max(dev, 1, &config, INTEL_DDB_PART_1_2, &max);
+	ilk_wm_merge(dev, &max, &lp_wm_1_2);
+
+	/* 5/6 split only in single pipe config on IVB+ */
+	if (INTEL_INFO(dev)->gen >= 7 && config.num_pipes_active <= 1) {
+		ilk_wm_max(dev, 1, &config, INTEL_DDB_PART_5_6, &max);
+		ilk_wm_merge(dev, &max, &lp_wm_5_6);
 
-	if (lp_max_1_2.pri != lp_max_5_6.pri) {
 		best_lp_wm = hsw_find_best_result(dev, &lp_wm_1_2, &lp_wm_5_6);
 	} else {
 		best_lp_wm = &lp_wm_1_2;
-- 
1.8.1.5

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

[PATCH 11/19] drm/i915: Don't compute 5/6 DDB split w/ zero active pipes

[ville.syrjala]

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

When there are zero active pipes, all the watermarks should be zero
also. No point in wasting time w/ computing the 5/6 split watermark
config.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5c5fba4..7b4f7d9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2881,7 +2881,7 @@ static void haswell_update_wm(struct drm_crtc *crtc)
 	ilk_wm_merge(dev, &max, &lp_wm_1_2);
 
 	/* 5/6 split only in single pipe config on IVB+ */
-	if (INTEL_INFO(dev)->gen >= 7 && config.num_pipes_active <= 1) {
+	if (INTEL_INFO(dev)->gen >= 7 && config.num_pipes_active == 1) {
 		ilk_wm_max(dev, 1, &config, INTEL_DDB_PART_5_6, &max);
 		ilk_wm_merge(dev, &max, &lp_wm_5_6);
 
-- 
1.8.1.5

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

[PATCH 12/19] drm/i915: Refactor wm_lp to level calculation

[ville.syrjala]

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

On HSW the LP1,LP2,LP3 levels are either 1,2,3 or 1,3,4. We make the
conversion from LPn to to the level at one point current. Later we're
going to do it in a few places, so move it to a separate function.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7b4f7d9..ee74352 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2686,6 +2686,12 @@ static void ilk_wm_merge(struct drm_device *dev,
 	}
 }
 
+static int ilk_wm_lp_to_level(int wm_lp, const struct intel_pipe_wm *pipe_wm)
+{
+	/* LP1,LP2,LP3 levels are either 1,2,3 or 1,3,4 */
+	return wm_lp + (wm_lp >= 2 && pipe_wm->wm[4].enable);
+}
+
 static void hsw_compute_wm_results(struct drm_device *dev,
 				   const struct intel_pipe_wm *lp_wm,
 				   struct hsw_wm_values *results)
@@ -2699,7 +2705,7 @@ static void hsw_compute_wm_results(struct drm_device *dev,
 	for (wm_lp = 1; wm_lp <= 3; wm_lp++) {
 		const struct intel_wm_level *r;
 
-		level = wm_lp + (wm_lp >= 2 && lp_wm->wm[4].enable);
+		level = ilk_wm_lp_to_level(wm_lp, lp_wm);
 
 		r = &lp_wm->wm[level];
 		if (!r->enable)
-- 
1.8.1.5

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

[PATCH 13/19] drm/i915: Kill fbc_wm_enabled from intel_wm_config

[ville.syrjala]

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

The fbc_wm_enabled member in intel_wm_config is useless for the time
being. The original idea for it was that we'd pre-compute it and so
that the WM merging process could know whether it needs to worry
about FBC watermarks at all.

But we don't have a convenient way to pre-check for the possibility
of FBC being used. intel_update_fbc() should be split up for that.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ee74352..c0f2837 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2195,7 +2195,6 @@ struct intel_wm_config {
 	unsigned int num_pipes_active;
 	bool sprites_enabled;
 	bool sprites_scaled;
-	bool fbc_wm_enabled;
 };
 
 /*
-- 
1.8.1.5

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

[PATCH 14/19] drm/i915: Store current watermark state in dev_priv->wm

[ville.syrjala]

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

To make it easier to check what watermark updates are actually
necessary, keep copies of the relevant bits that match the current
hardware state.

Also add DDB partitioning into hsw_wm_values as that's another piece
of state we want to track.

We don't read out the hardware state on init yet, so we can't really
start using this yet, but it will be used later.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ab46eb3..35db44b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1088,6 +1088,15 @@ struct intel_wm_level {
 	uint32_t fbc_val;
 };
 
+struct hsw_wm_values {
+	uint32_t wm_pipe[3];
+	uint32_t wm_lp[3];
+	uint32_t wm_lp_spr[3];
+	uint32_t wm_linetime[3];
+	bool enable_fbc_wm;
+	enum intel_ddb_partitioning partitioning;
+};
+
 /*
  * This struct tracks the state needed for the Package C8+ feature.
  *
@@ -1339,6 +1348,9 @@ typedef struct drm_i915_private {
 		uint16_t spr_latency[5];
 		/* cursor */
 		uint16_t cur_latency[5];
+
+		/* current hardware state */
+		struct hsw_wm_values hw;
 	} wm;
 
 	struct i915_package_c8 pc8;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c0f2837..27cc69d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2182,14 +2182,6 @@ struct hsw_wm_maximums {
 	uint16_t fbc;
 };
 
-struct hsw_wm_values {
-	uint32_t wm_pipe[3];
-	uint32_t wm_lp[3];
-	uint32_t wm_lp_spr[3];
-	uint32_t wm_linetime[3];
-	bool enable_fbc_wm;
-};
-
 /* used in computing the new watermarks state */
 struct intel_wm_config {
 	unsigned int num_pipes_active;
@@ -2693,12 +2685,14 @@ static int ilk_wm_lp_to_level(int wm_lp, const struct intel_pipe_wm *pipe_wm)
 
 static void hsw_compute_wm_results(struct drm_device *dev,
 				   const struct intel_pipe_wm *lp_wm,
+				   enum intel_ddb_partitioning partitioning,
 				   struct hsw_wm_values *results)
 {
 	struct intel_crtc *intel_crtc;
 	int level, wm_lp;
 
 	results->enable_fbc_wm = lp_wm->fbc_wm_enabled;
+	results->partitioning = partitioning;
 
 	/* LP1+ register values */
 	for (wm_lp = 1; wm_lp <= 3; wm_lp++) {
@@ -2768,13 +2762,10 @@ static struct intel_pipe_wm *hsw_find_best_result(struct drm_device *dev,
  * causes WMs to be re-evaluated, expending some power.
  */
 static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
-				struct hsw_wm_values *results,
-				enum intel_ddb_partitioning partitioning)
+				struct hsw_wm_values *results)
 {
 	struct hsw_wm_values previous;
 	uint32_t val;
-	enum intel_ddb_partitioning prev_partitioning;
-	bool prev_enable_fbc_wm;
 
 	previous.wm_pipe[0] = I915_READ(WM0_PIPEA_ILK);
 	previous.wm_pipe[1] = I915_READ(WM0_PIPEB_ILK);
@@ -2789,21 +2780,12 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 	previous.wm_linetime[1] = I915_READ(PIPE_WM_LINETIME(PIPE_B));
 	previous.wm_linetime[2] = I915_READ(PIPE_WM_LINETIME(PIPE_C));
 
-	prev_partitioning = (I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6) ?
+	previous.partitioning = (I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6) ?
 				INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2;
 
-	prev_enable_fbc_wm = !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
-
-	if (memcmp(results->wm_pipe, previous.wm_pipe,
-		   sizeof(results->wm_pipe)) == 0 &&
-	    memcmp(results->wm_lp, previous.wm_lp,
-		   sizeof(results->wm_lp)) == 0 &&
-	    memcmp(results->wm_lp_spr, previous.wm_lp_spr,
-		   sizeof(results->wm_lp_spr)) == 0 &&
-	    memcmp(results->wm_linetime, previous.wm_linetime,
-		   sizeof(results->wm_linetime)) == 0 &&
-	    partitioning == prev_partitioning &&
-	    results->enable_fbc_wm == prev_enable_fbc_wm)
+	previous.enable_fbc_wm = !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
+
+	if (memcmp(results, &previous, sizeof(*results)) == 0)
 		return;
 
 	if (previous.wm_lp[2] != 0)
@@ -2827,16 +2809,16 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 	if (previous.wm_linetime[2] != results->wm_linetime[2])
 		I915_WRITE(PIPE_WM_LINETIME(PIPE_C), results->wm_linetime[2]);
 
-	if (prev_partitioning != partitioning) {
+	if (previous.partitioning != results->partitioning) {
 		val = I915_READ(WM_MISC);
-		if (partitioning == INTEL_DDB_PART_1_2)
+		if (results->partitioning == INTEL_DDB_PART_1_2)
 			val &= ~WM_MISC_DATA_PARTITION_5_6;
 		else
 			val |= WM_MISC_DATA_PARTITION_5_6;
 		I915_WRITE(WM_MISC, val);
 	}
 
-	if (prev_enable_fbc_wm != results->enable_fbc_wm) {
+	if (previous.enable_fbc_wm != results->enable_fbc_wm) {
 		val = I915_READ(DISP_ARB_CTL);
 		if (results->enable_fbc_wm)
 			val &= ~DISP_FBC_WM_DIS;
@@ -2858,6 +2840,8 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 		I915_WRITE(WM2_LP_ILK, results->wm_lp[1]);
 	if (results->wm_lp[2] != 0)
 		I915_WRITE(WM3_LP_ILK, results->wm_lp[2]);
+
+	dev_priv->wm.hw = *results;
 }
 
 static void haswell_update_wm(struct drm_crtc *crtc)
@@ -2895,12 +2879,12 @@ static void haswell_update_wm(struct drm_crtc *crtc)
 		best_lp_wm = &lp_wm_1_2;
 	}
 
-	hsw_compute_wm_results(dev, best_lp_wm, &results);
-
 	partitioning = (best_lp_wm == &lp_wm_1_2) ?
 		       INTEL_DDB_PART_1_2 : INTEL_DDB_PART_5_6;
 
-	hsw_write_wm_values(dev_priv, &results, partitioning);
+	hsw_compute_wm_results(dev, best_lp_wm, partitioning, &results);
+
+	hsw_write_wm_values(dev_priv, &results);
 }
 
 static void haswell_update_sprite_wm(struct drm_plane *plane,
-- 
1.8.1.5

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

[PATCH 15/19] drm/i915: Improve watermark dirtyness checks

[ville.syrjala]

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

Currently hsw_write_vm_values() may write to certain watermark
registers needlessly. For instance it will disable LP1+ watermarks
around WM_PIPE changes even though that's not needed. Also if only,
say, LP3 changes, the current code will again disable all LP1+
watermarks even though only LP3 needs to be reconfigured.

Add an easy to read function that will compute the dirtyness of the
watermarks, and use that information to further optimize the watermark
programming.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 27cc69d..82e1af6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2757,6 +2757,63 @@ static struct intel_pipe_wm *hsw_find_best_result(struct drm_device *dev,
 	}
 }
 
+/* dirty bits used to track which watermarks need changes */
+#define WM_DIRTY_PIPE(pipe) (1 << (pipe))
+#define WM_DIRTY_LINETIME(pipe) (1 << (8 + (pipe)))
+#define WM_DIRTY_LP(wm_lp) (1 << (15 + (wm_lp)))
+#define WM_DIRTY_LP_ALL (WM_DIRTY_LP(1) | WM_DIRTY_LP(2) | WM_DIRTY_LP(3))
+#define WM_DIRTY_FBC (1 << 24)
+#define WM_DIRTY_DDB (1 << 25)
+
+static unsigned int ilk_compute_wm_dirty(struct drm_device *dev,
+					 const struct hsw_wm_values *old,
+					 const struct hsw_wm_values *new)
+{
+	unsigned int dirty = 0;
+	enum pipe pipe;
+	int wm_lp;
+
+	for_each_pipe(pipe) {
+		if (old->wm_linetime[pipe] != new->wm_linetime[pipe]) {
+			dirty |= WM_DIRTY_LINETIME(pipe);
+			/* Must disable LP1+ watermarks too */
+			dirty |= WM_DIRTY_LP_ALL;
+		}
+
+		if (old->wm_pipe[pipe] != new->wm_pipe[pipe])
+			dirty |= WM_DIRTY_PIPE(pipe);
+	}
+
+	if (old->enable_fbc_wm != new->enable_fbc_wm) {
+		dirty |= WM_DIRTY_FBC;
+		/* Must disable LP1+ watermarks too */
+		dirty |= WM_DIRTY_LP_ALL;
+	}
+
+	if (old->partitioning != new->partitioning) {
+		dirty |= WM_DIRTY_DDB;
+		/* Must disable LP1+ watermarks too */
+		dirty |= WM_DIRTY_LP_ALL;
+	}
+
+	/* LP1+ watermarks already deemed dirty, no need to continue */
+	if (dirty & WM_DIRTY_LP_ALL)
+		return dirty;
+
+	/* Find the lowest numbered LP1+ watermark in need of an update... */
+	for (wm_lp = 1; wm_lp <= 3; wm_lp++) {
+		if (old->wm_lp[wm_lp - 1] != new->wm_lp[wm_lp - 1] ||
+		    old->wm_lp_spr[wm_lp - 1] != new->wm_lp_spr[wm_lp - 1])
+			break;
+	}
+
+	/* ...and mark it and all higher numbered LP1+ watermarks as dirty */
+	for (; wm_lp <= 3; wm_lp++)
+		dirty |= WM_DIRTY_LP(wm_lp);
+
+	return dirty;
+}
+
 /*
  * The spec says we shouldn't write when we don't need, because every write
  * causes WMs to be re-evaluated, expending some power.
@@ -2765,6 +2822,7 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 				struct hsw_wm_values *results)
 {
 	struct hsw_wm_values previous;
+	unsigned int dirty;
 	uint32_t val;
 
 	previous.wm_pipe[0] = I915_READ(WM0_PIPEA_ILK);
@@ -2785,31 +2843,32 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 
 	previous.enable_fbc_wm = !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
 
-	if (memcmp(results, &previous, sizeof(*results)) == 0)
+	dirty = ilk_compute_wm_dirty(dev_priv->dev, &previous, results);
+	if (!dirty)
 		return;
 
-	if (previous.wm_lp[2] != 0)
+	if (dirty & WM_DIRTY_LP(3) && previous.wm_lp[2] != 0)
 		I915_WRITE(WM3_LP_ILK, 0);
-	if (previous.wm_lp[1] != 0)
+	if (dirty & WM_DIRTY_LP(2) && previous.wm_lp[1] != 0)
 		I915_WRITE(WM2_LP_ILK, 0);
-	if (previous.wm_lp[0] != 0)
+	if (dirty & WM_DIRTY_LP(1) && previous.wm_lp[0] != 0)
 		I915_WRITE(WM1_LP_ILK, 0);
 
-	if (previous.wm_pipe[0] != results->wm_pipe[0])
+	if (dirty & WM_DIRTY_PIPE(PIPE_A))
 		I915_WRITE(WM0_PIPEA_ILK, results->wm_pipe[0]);
-	if (previous.wm_pipe[1] != results->wm_pipe[1])
+	if (dirty & WM_DIRTY_PIPE(PIPE_B))
 		I915_WRITE(WM0_PIPEB_ILK, results->wm_pipe[1]);
-	if (previous.wm_pipe[2] != results->wm_pipe[2])
+	if (dirty & WM_DIRTY_PIPE(PIPE_C))
 		I915_WRITE(WM0_PIPEC_IVB, results->wm_pipe[2]);
 
-	if (previous.wm_linetime[0] != results->wm_linetime[0])
+	if (dirty & WM_DIRTY_LINETIME(PIPE_A))
 		I915_WRITE(PIPE_WM_LINETIME(PIPE_A), results->wm_linetime[0]);
-	if (previous.wm_linetime[1] != results->wm_linetime[1])
+	if (dirty & WM_DIRTY_LINETIME(PIPE_B))
 		I915_WRITE(PIPE_WM_LINETIME(PIPE_B), results->wm_linetime[1]);
-	if (previous.wm_linetime[2] != results->wm_linetime[2])
+	if (dirty & WM_DIRTY_LINETIME(PIPE_C))
 		I915_WRITE(PIPE_WM_LINETIME(PIPE_C), results->wm_linetime[2]);
 
-	if (previous.partitioning != results->partitioning) {
+	if (dirty & WM_DIRTY_DDB) {
 		val = I915_READ(WM_MISC);
 		if (results->partitioning == INTEL_DDB_PART_1_2)
 			val &= ~WM_MISC_DATA_PARTITION_5_6;
@@ -2818,7 +2877,7 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 		I915_WRITE(WM_MISC, val);
 	}
 
-	if (previous.enable_fbc_wm != results->enable_fbc_wm) {
+	if (dirty & WM_DIRTY_FBC) {
 		val = I915_READ(DISP_ARB_CTL);
 		if (results->enable_fbc_wm)
 			val &= ~DISP_FBC_WM_DIS;
@@ -2827,18 +2886,18 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 		I915_WRITE(DISP_ARB_CTL, val);
 	}
 
-	if (previous.wm_lp_spr[0] != results->wm_lp_spr[0])
+	if (dirty & WM_DIRTY_LP(1) && previous.wm_lp_spr[0] != results->wm_lp_spr[0])
 		I915_WRITE(WM1S_LP_ILK, results->wm_lp_spr[0]);
-	if (previous.wm_lp_spr[1] != results->wm_lp_spr[1])
+	if (dirty & WM_DIRTY_LP(2) && previous.wm_lp_spr[1] != results->wm_lp_spr[1])
 		I915_WRITE(WM2S_LP_IVB, results->wm_lp_spr[1]);
-	if (previous.wm_lp_spr[2] != results->wm_lp_spr[2])
+	if (dirty & WM_DIRTY_LP(3) && previous.wm_lp_spr[2] != results->wm_lp_spr[2])
 		I915_WRITE(WM3S_LP_IVB, results->wm_lp_spr[2]);
 
-	if (results->wm_lp[0] != 0)
+	if (dirty & WM_DIRTY_LP(1) && results->wm_lp[0] != 0)
 		I915_WRITE(WM1_LP_ILK, results->wm_lp[0]);
-	if (results->wm_lp[1] != 0)
+	if (dirty & WM_DIRTY_LP(2) && results->wm_lp[1] != 0)
 		I915_WRITE(WM2_LP_ILK, results->wm_lp[1]);
-	if (results->wm_lp[2] != 0)
+	if (dirty & WM_DIRTY_LP(3) && results->wm_lp[2] != 0)
 		I915_WRITE(WM3_LP_ILK, results->wm_lp[2]);
 
 	dev_priv->wm.hw = *results;
-- 
1.8.1.5

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

[PATCH 16/19] drm/i915: Init HSW watermark tracking in intel_modeset_setup_hw_state()

[ville.syrjala]

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

Fill out the HSW watermark s/w tracking structures with the current
hardware state in intel_modeset_setup_hw_state(). This allows us to skip
the HW state readback during watermark programming and just use the values
we keep around in dev_priv->wm. Reduces the overhead of the watermark
programming quite a bit.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   3 +
 drivers/gpu/drm/i915/intel_drv.h     |   1 +
 drivers/gpu/drm/i915/intel_pm.c      | 104 ++++++++++++++++++++++++++---------
 3 files changed, 82 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a5181fe..738e285 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10473,6 +10473,9 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 		pll->on = false;
 	}
 
+	if (IS_HASWELL(dev))
+		ilk_init_wm(dev);
+
 	if (force_restore) {
 		/*
 		 * We need to use raw interfaces for restoring state to avoid
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 664df77..a684d5e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -809,5 +809,6 @@ extern void hsw_pc8_disable_interrupts(struct drm_device *dev);
 extern void hsw_pc8_restore_interrupts(struct drm_device *dev);
 extern void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
 extern void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
+extern void ilk_init_wm(struct drm_device *dev);
 
 #endif /* __INTEL_DRV_H__ */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 82e1af6..fe44e86 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2821,37 +2821,19 @@ static unsigned int ilk_compute_wm_dirty(struct drm_device *dev,
 static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 				struct hsw_wm_values *results)
 {
-	struct hsw_wm_values previous;
+	struct hsw_wm_values *previous = &dev_priv->wm.hw;
 	unsigned int dirty;
 	uint32_t val;
 
-	previous.wm_pipe[0] = I915_READ(WM0_PIPEA_ILK);
-	previous.wm_pipe[1] = I915_READ(WM0_PIPEB_ILK);
-	previous.wm_pipe[2] = I915_READ(WM0_PIPEC_IVB);
-	previous.wm_lp[0] = I915_READ(WM1_LP_ILK);
-	previous.wm_lp[1] = I915_READ(WM2_LP_ILK);
-	previous.wm_lp[2] = I915_READ(WM3_LP_ILK);
-	previous.wm_lp_spr[0] = I915_READ(WM1S_LP_ILK);
-	previous.wm_lp_spr[1] = I915_READ(WM2S_LP_IVB);
-	previous.wm_lp_spr[2] = I915_READ(WM3S_LP_IVB);
-	previous.wm_linetime[0] = I915_READ(PIPE_WM_LINETIME(PIPE_A));
-	previous.wm_linetime[1] = I915_READ(PIPE_WM_LINETIME(PIPE_B));
-	previous.wm_linetime[2] = I915_READ(PIPE_WM_LINETIME(PIPE_C));
-
-	previous.partitioning = (I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6) ?
-				INTEL_DDB_PART_5_6 : INTEL_DDB_PART_1_2;
-
-	previous.enable_fbc_wm = !(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
-
-	dirty = ilk_compute_wm_dirty(dev_priv->dev, &previous, results);
+	dirty = ilk_compute_wm_dirty(dev_priv->dev, previous, results);
 	if (!dirty)
 		return;
 
-	if (dirty & WM_DIRTY_LP(3) && previous.wm_lp[2] != 0)
+	if (dirty & WM_DIRTY_LP(3) && previous->wm_lp[2] != 0)
 		I915_WRITE(WM3_LP_ILK, 0);
-	if (dirty & WM_DIRTY_LP(2) && previous.wm_lp[1] != 0)
+	if (dirty & WM_DIRTY_LP(2) && previous->wm_lp[1] != 0)
 		I915_WRITE(WM2_LP_ILK, 0);
-	if (dirty & WM_DIRTY_LP(1) && previous.wm_lp[0] != 0)
+	if (dirty & WM_DIRTY_LP(1) && previous->wm_lp[0] != 0)
 		I915_WRITE(WM1_LP_ILK, 0);
 
 	if (dirty & WM_DIRTY_PIPE(PIPE_A))
@@ -2886,11 +2868,11 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 		I915_WRITE(DISP_ARB_CTL, val);
 	}
 
-	if (dirty & WM_DIRTY_LP(1) && previous.wm_lp_spr[0] != results->wm_lp_spr[0])
+	if (dirty & WM_DIRTY_LP(1) && previous->wm_lp_spr[0] != results->wm_lp_spr[0])
 		I915_WRITE(WM1S_LP_ILK, results->wm_lp_spr[0]);
-	if (dirty & WM_DIRTY_LP(2) && previous.wm_lp_spr[1] != results->wm_lp_spr[1])
+	if (dirty & WM_DIRTY_LP(2) && previous->wm_lp_spr[1] != results->wm_lp_spr[1])
 		I915_WRITE(WM2S_LP_IVB, results->wm_lp_spr[1]);
-	if (dirty & WM_DIRTY_LP(3) && previous.wm_lp_spr[2] != results->wm_lp_spr[2])
+	if (dirty & WM_DIRTY_LP(3) && previous->wm_lp_spr[2] != results->wm_lp_spr[2])
 		I915_WRITE(WM3S_LP_IVB, results->wm_lp_spr[2]);
 
 	if (dirty & WM_DIRTY_LP(1) && results->wm_lp[0] != 0)
@@ -3123,6 +3105,76 @@ static void sandybridge_update_sprite_wm(struct drm_plane *plane,
 	I915_WRITE(WM3S_LP_IVB, sprite_wm);
 }
 
+static void ilk_init_pipe_wm(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct hsw_wm_values *hw = &dev_priv->wm.hw;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_pipe_wm *active = &intel_crtc->wm.active;
+	enum pipe pipe = intel_crtc->pipe;
+	static const unsigned int wm0_pipe_reg[] = {
+		[PIPE_A] = WM0_PIPEA_ILK,
+		[PIPE_B] = WM0_PIPEB_ILK,
+		[PIPE_C] = WM0_PIPEC_IVB,
+	};
+
+	hw->wm_pipe[pipe] = I915_READ(wm0_pipe_reg[pipe]);
+	hw->wm_linetime[pipe] = I915_READ(PIPE_WM_LINETIME(pipe));
+
+	/* Assume sprites are disabled */
+
+	if (intel_crtc_active(crtc)) {
+		u32 tmp = hw->wm_pipe[pipe];
+
+		/*
+		 * For active pipes LP0 watermark is marked as
+		 * enabled, and LP1+ watermaks as disabled since
+		 * we can't really reverse compute them in case
+		 * multiple pipes are active.
+		 */
+		active->wm[0].enable = true;
+		active->wm[0].pri_val = (tmp & WM0_PIPE_PLANE_MASK) >> WM0_PIPE_PLANE_SHIFT;
+		active->wm[0].spr_val = (tmp & WM0_PIPE_SPRITE_MASK) >> WM0_PIPE_SPRITE_SHIFT;
+		active->wm[0].cur_val = tmp & WM0_PIPE_CURSOR_MASK;
+		active->linetime = hw->wm_linetime[pipe];
+	} else {
+		int level, max_level = ilk_wm_max_level(dev);
+
+		/*
+		 * For inactive pipes, all watermark levels
+		 * should be marked as enabled but zeroed,
+		 * which is what we'd comoute them to.
+		 */
+		for (level = 0; level <= max_level; level++)
+			active->wm[level].enable = true;
+	}
+}
+
+void ilk_init_wm(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct hsw_wm_values *hw = &dev_priv->wm.hw;
+	struct drm_crtc *crtc;
+
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
+		ilk_init_pipe_wm(crtc);
+
+	hw->wm_lp[0] = I915_READ(WM1_LP_ILK);
+	hw->wm_lp[1] = I915_READ(WM2_LP_ILK);
+	hw->wm_lp[2] = I915_READ(WM3_LP_ILK);
+
+	hw->wm_lp_spr[0] = I915_READ(WM1S_LP_ILK);
+	hw->wm_lp_spr[1] = I915_READ(WM2S_LP_IVB);
+	hw->wm_lp_spr[2] = I915_READ(WM3S_LP_IVB);
+
+	hw->partitioning =
+		!!(I915_READ(WM_MISC) & WM_MISC_DATA_PARTITION_5_6);
+
+	hw->enable_fbc_wm =
+		!(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
+}
+
 /**
  * intel_update_watermarks - update FIFO watermark values based on current modes
  *
-- 
1.8.1.5

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

[PATCH 17/19] drm/i915: Remove a somewhat silly debug print from watermark code

[ville.syrjala]

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

This debug print just adds overhead to the watermark merging process,
and doesn't really give enough information to be useful. Just kill
and let's add something much better a bit later.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fe44e86..62c85d9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2403,8 +2403,6 @@ static bool ilk_check_wm(int level,
 		result->enable = true;
 	}
 
-	DRM_DEBUG_KMS("WM%d: %sabled\n", level, result->enable ? "en" : "dis");
-
 	return ret;
 }
 
-- 
1.8.1.5

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

[PATCH 18/19] drm/i915: Adjust watermark register masks

[ville.syrjala]

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

We want to be able to use the masks to decode the register contents
regardless of the hardware generation. So just expand the masks to
cover all available bits, even if those are reserved on some
generations.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 35a6c74..ff23680 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3219,11 +3219,11 @@
 
 /* define the Watermark register on Ironlake */
 #define WM0_PIPEA_ILK		0x45100
-#define  WM0_PIPE_PLANE_MASK	(0x7f<<16)
+#define  WM0_PIPE_PLANE_MASK	(0xffff<<16)
 #define  WM0_PIPE_PLANE_SHIFT	16
-#define  WM0_PIPE_SPRITE_MASK	(0x3f<<8)
+#define  WM0_PIPE_SPRITE_MASK	(0xff<<8)
 #define  WM0_PIPE_SPRITE_SHIFT	8
-#define  WM0_PIPE_CURSOR_MASK	(0x1f)
+#define  WM0_PIPE_CURSOR_MASK	(0xff)
 
 #define WM0_PIPEB_ILK		0x45104
 #define WM0_PIPEC_IVB		0x45200
@@ -3233,9 +3233,9 @@
 #define  WM1_LP_LATENCY_MASK	(0x7f<<24)
 #define  WM1_LP_FBC_MASK	(0xf<<20)
 #define  WM1_LP_FBC_SHIFT	20
-#define  WM1_LP_SR_MASK		(0x1ff<<8)
+#define  WM1_LP_SR_MASK		(0xfff<<8)
 #define  WM1_LP_SR_SHIFT	8
-#define  WM1_LP_CURSOR_MASK	(0x3f)
+#define  WM1_LP_CURSOR_MASK	(0xff)
 #define WM2_LP_ILK		0x4510c
 #define  WM2_LP_EN		(1<<31)
 #define WM3_LP_ILK		0x45110
-- 
1.8.1.5

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

[PATCH 19/19] drm/i915: Add watermark tracepoints

[ville.syrjala]

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

We may want to know what kind of watermarks got computed and programmed
into the hardware. Using tracepoints is much leaner than debug prints.

Also add trace call for the watermark state we read out of the
hardware during init, though I;m not sure there's any way to see that
trace as the events aren't available until the module is loaded.

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

diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index e2c5ee6..6dc9a91 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -14,6 +14,187 @@
 #define TRACE_SYSTEM_STRING __stringify(TRACE_SYSTEM)
 #define TRACE_INCLUDE_FILE i915_trace
 
+/* watermark */
+
+TRACE_EVENT(i915_wm_update_start,
+	    TP_PROTO(enum pipe pipe),
+	    TP_ARGS(pipe),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = pipe;
+			   ),
+
+	    TP_printk("pipe %c", pipe_name(__entry->pipe))
+);
+
+TRACE_EVENT(i915_wm_update_end,
+	    TP_PROTO(enum pipe pipe, bool changed),
+	    TP_ARGS(pipe, changed),
+
+	    TP_STRUCT__entry(
+			     __field(enum pipe, pipe)
+			     __field(bool, changed)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pipe = pipe;
+			   __entry->changed = changed;
+			   ),
+
+	    TP_printk("pipe %c, changed=%s",
+		      pipe_name(__entry->pipe), __entry->changed ? "yes" : "no")
+);
+
+TRACE_EVENT_CONDITION(i915_wm_misc,
+	TP_PROTO(const struct hsw_wm_values *hw, bool trace),
+	TP_ARGS(hw, trace),
+
+	TP_CONDITION(trace),
+
+	TP_STRUCT__entry(
+		__field(bool, enable_fbc_wm)
+		__field(enum intel_ddb_partitioning, partitioning)
+		),
+
+	TP_fast_assign(
+		__entry->enable_fbc_wm = hw->enable_fbc_wm;
+		__entry->partitioning = hw->partitioning;
+		),
+
+	TP_printk("fbc=%s, ddb partitioning=%s",
+		__entry->enable_fbc_wm ? "yes" : "no",
+		__entry->partitioning == INTEL_DDB_PART_5_6 ? "5/6" : "1/2")
+);
+
+TRACE_EVENT_CONDITION(i915_wm_pipe,
+	TP_PROTO(struct drm_device *dev, enum pipe pipe, const struct hsw_wm_values *hw, bool trace),
+	TP_ARGS(dev, pipe, hw, trace),
+
+	TP_CONDITION(pipe < INTEL_INFO(dev)->num_pipes && trace),
+
+	TP_STRUCT__entry(
+		__field(enum pipe, pipe)
+		__field(uint32_t, wm_pipe)
+		),
+
+	TP_fast_assign(
+		__entry->pipe = pipe;
+		__entry->wm_pipe = hw->wm_pipe[pipe];
+		),
+
+	TP_printk("pipe %c: pri=%u, spr=%u, cur=%u",
+		pipe_name(__entry->pipe),
+		(__entry->wm_pipe & WM0_PIPE_PLANE_MASK) >> WM0_PIPE_PLANE_SHIFT,
+		(__entry->wm_pipe & WM0_PIPE_SPRITE_MASK) >> WM0_PIPE_SPRITE_SHIFT,
+		__entry->wm_pipe & WM0_PIPE_CURSOR_MASK)
+);
+
+TRACE_EVENT_CONDITION(i915_wm_linetime,
+	TP_PROTO(struct drm_device *dev, enum pipe pipe, const struct hsw_wm_values *hw, bool trace),
+	TP_ARGS(dev, pipe, hw, trace),
+
+	TP_CONDITION(IS_HASWELL(dev) && pipe < INTEL_INFO(dev)->num_pipes && trace),
+
+	TP_STRUCT__entry(
+		__field(enum pipe, pipe)
+		__field(uint32_t, wm_linetime)
+		),
+
+	TP_fast_assign(
+		__entry->pipe = pipe;
+		__entry->wm_linetime = hw->wm_linetime[pipe];
+		),
+
+	TP_printk("pipe %c: linetime=%u, ips linetime=%u",
+		pipe_name(__entry->pipe),
+		__entry->wm_linetime & PIPE_WM_LINETIME_MASK,
+		(__entry->wm_linetime & PIPE_WM_LINETIME_IPS_LINETIME_MASK) >> 16)
+);
+
+
+TRACE_EVENT_CONDITION(i915_wm_lp1_ilk,
+	TP_PROTO(struct drm_device *dev, const struct hsw_wm_values *hw, bool trace),
+	TP_ARGS(dev, hw, trace),
+
+	TP_CONDITION(INTEL_INFO(dev)->gen <= 6 && trace),
+
+	TP_STRUCT__entry(
+		__field(uint32_t, wm_lp)
+		__field(uint32_t, wm_lp_spr)
+		),
+
+	TP_fast_assign(
+		__entry->wm_lp = hw->wm_lp[0];
+		__entry->wm_lp_spr = hw->wm_lp_spr[0];
+		),
+
+	TP_printk("LP1: en=%s, lat=%u, fbc=%u, pri=%u, cur=%u, spr=%u, spr en=%s",
+		__entry->wm_lp & WM1_LP_SR_EN ? "yes" : "no",
+		(__entry->wm_lp & WM1_LP_LATENCY_MASK) >> WM1_LP_LATENCY_SHIFT,
+		(__entry->wm_lp & WM1_LP_FBC_MASK) >> WM1_LP_FBC_SHIFT,
+		(__entry->wm_lp & WM1_LP_SR_MASK) >> WM1_LP_SR_SHIFT,
+		__entry->wm_lp & WM1_LP_CURSOR_MASK,
+		__entry->wm_lp_spr & ~WM1S_LP_EN,
+		__entry->wm_lp_spr & WM1S_LP_EN ? "yes" : "no")
+);
+
+TRACE_EVENT_CONDITION(i915_wm_lp_ilk,
+	TP_PROTO(struct drm_device *dev, int lp, const struct hsw_wm_values *hw, bool trace),
+	TP_ARGS(dev, lp, hw, trace),
+
+	TP_CONDITION(INTEL_INFO(dev)->gen <= 6 && trace),
+
+	TP_STRUCT__entry(
+		__field(int, lp)
+		__field(uint32_t, wm_lp)
+		),
+
+	TP_fast_assign(
+		__entry->lp = lp;
+		__entry->wm_lp = hw->wm_lp[lp - 1];
+		),
+
+	TP_printk("LP%d: en=%s, lat=%u, fbc=%u, pri=%u, cur=%u",
+		__entry->lp,
+		__entry->wm_lp & WM1_LP_SR_EN ? "yes" : "no",
+		(__entry->wm_lp & WM1_LP_LATENCY_MASK) >> WM1_LP_LATENCY_SHIFT,
+		(__entry->wm_lp & WM1_LP_FBC_MASK) >> WM1_LP_FBC_SHIFT,
+		(__entry->wm_lp & WM1_LP_SR_MASK) >> WM1_LP_SR_SHIFT,
+		__entry->wm_lp & WM1_LP_CURSOR_MASK)
+);
+
+TRACE_EVENT_CONDITION(i915_wm_lp_ivb,
+	TP_PROTO(struct drm_device *dev, int lp, const struct hsw_wm_values *hw, bool trace),
+	TP_ARGS(dev, lp, hw, trace),
+
+	TP_CONDITION(INTEL_INFO(dev)->gen >= 7 && trace),
+
+	TP_STRUCT__entry(
+		__field(int, lp)
+		__field(uint32_t, wm_lp)
+		__field(uint32_t, wm_lp_spr)
+		),
+
+	TP_fast_assign(
+		__entry->lp = lp;
+		__entry->wm_lp = hw->wm_lp[lp - 1];
+		__entry->wm_lp_spr = hw->wm_lp_spr[lp - 1];
+		),
+
+	TP_printk("LP%d: en=%s, lat=%u, fbc=%u, pri=%u, cur=%u, spr=%u",
+		__entry->lp,
+		__entry->wm_lp & WM1_LP_SR_EN ? "yes" : "no",
+		(__entry->wm_lp & WM1_LP_LATENCY_MASK) >> WM1_LP_LATENCY_SHIFT,
+		(__entry->wm_lp & WM1_LP_FBC_MASK) >> WM1_LP_FBC_SHIFT,
+		(__entry->wm_lp & WM1_LP_SR_MASK) >> WM1_LP_SR_SHIFT,
+		__entry->wm_lp & WM1_LP_CURSOR_MASK,
+		__entry->wm_lp_spr)
+);
+
 /* object tracking */
 
 TRACE_EVENT(i915_gem_object_create,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 62c85d9..d907326 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2815,8 +2815,9 @@ static unsigned int ilk_compute_wm_dirty(struct drm_device *dev,
 /*
  * The spec says we shouldn't write when we don't need, because every write
  * causes WMs to be re-evaluated, expending some power.
+ * Returns true if some watermarks were changed.
  */
-static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
+static bool hsw_write_wm_values(struct drm_i915_private *dev_priv,
 				struct hsw_wm_values *results)
 {
 	struct hsw_wm_values *previous = &dev_priv->wm.hw;
@@ -2825,7 +2826,7 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 
 	dirty = ilk_compute_wm_dirty(dev_priv->dev, previous, results);
 	if (!dirty)
-		return;
+		return false;
 
 	if (dirty & WM_DIRTY_LP(3) && previous->wm_lp[2] != 0)
 		I915_WRITE(WM3_LP_ILK, 0);
@@ -2881,6 +2882,32 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 		I915_WRITE(WM3_LP_ILK, results->wm_lp[2]);
 
 	dev_priv->wm.hw = *results;
+
+	return true;
+}
+
+static void ilk_wm_trace(struct drm_device *dev, bool trace)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	const struct hsw_wm_values *hw = &dev_priv->wm.hw;
+
+	trace_i915_wm_misc(hw, trace);
+
+	trace_i915_wm_pipe(dev, PIPE_A, hw, trace);
+	trace_i915_wm_pipe(dev, PIPE_B, hw, trace);
+	trace_i915_wm_pipe(dev, PIPE_C, hw, trace);
+
+	trace_i915_wm_linetime(dev, PIPE_A, hw, trace);
+	trace_i915_wm_linetime(dev, PIPE_B, hw, trace);
+	trace_i915_wm_linetime(dev, PIPE_C, hw, trace);
+
+	trace_i915_wm_lp1_ilk(dev, hw, trace);
+	trace_i915_wm_lp_ilk(dev, 2, hw, trace);
+	trace_i915_wm_lp_ilk(dev, 3, hw, trace);
+
+	trace_i915_wm_lp_ivb(dev, 1, hw, trace);
+	trace_i915_wm_lp_ivb(dev, 2, hw, trace);
+	trace_i915_wm_lp_ivb(dev, 3, hw, trace);
 }
 
 static void haswell_update_wm(struct drm_crtc *crtc)
@@ -2895,6 +2922,7 @@ static void haswell_update_wm(struct drm_crtc *crtc)
 	struct intel_pipe_wm pipe_wm = {};
 	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
 	struct intel_wm_config config = {};
+	bool changed;
 
 	hsw_compute_wm_parameters(crtc, &params, &config);
 
@@ -2903,6 +2931,8 @@ static void haswell_update_wm(struct drm_crtc *crtc)
 	if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm)))
 		return;
 
+	trace_i915_wm_update_start(intel_crtc->pipe);
+
 	intel_crtc->wm.active = pipe_wm;
 
 	ilk_wm_max(dev, 1, &config, INTEL_DDB_PART_1_2, &max);
@@ -2923,7 +2953,11 @@ static void haswell_update_wm(struct drm_crtc *crtc)
 
 	hsw_compute_wm_results(dev, best_lp_wm, partitioning, &results);
 
-	hsw_write_wm_values(dev_priv, &results);
+	changed = hsw_write_wm_values(dev_priv, &results);
+
+	trace_i915_wm_update_end(intel_crtc->pipe, changed);
+
+	ilk_wm_trace(dev, changed);
 }
 
 static void haswell_update_sprite_wm(struct drm_plane *plane,
@@ -3171,6 +3205,8 @@ void ilk_init_wm(struct drm_device *dev)
 
 	hw->enable_fbc_wm =
 		!(I915_READ(DISP_ARB_CTL) & DISP_FBC_WM_DIS);
+
+	ilk_wm_trace(dev, true);
 }
 
 /**
-- 
1.8.1.5

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

Re: [PATCH 01/19] drm/i915: Pass crtc to intel_update_watermarks()

[Paulo Zanoni]

2013/8/30  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Passing the appropriate crtc to intel_update_watermarks() should help
> in avoiding needless work in the future.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I like the fact that now we're passing the CRTC, but <bikeshed> my
only worry is that now some functions overwrite the "crtc" we pass as
argument, so this might be confusing and maybe lead to bugs in the
future: perhaps the argument could be called unused_crtc or
ignored_crtc, then we'd keep the "crtc" variables they already have
</bikeshed>. But it's just a bikeshed, so with or without changes:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>.


> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
>  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++---------
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 43 +++++++++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_sprite.c  |  6 ++---
>  5 files changed, 40 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7594356..ab46eb3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -357,7 +357,7 @@ struct drm_i915_display_funcs {
>                           int target, int refclk,
>                           struct dpll *match_clock,
>                           struct dpll *best_clock);
> -       void (*update_wm)(struct drm_device *dev);
> +       void (*update_wm)(struct drm_crtc *crtc);
>         void (*update_sprite_wm)(struct drm_plane *plane,
>                                  struct drm_crtc *crtc,
>                                  uint32_t sprite_width, int pixel_size,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f526ea9..13856ec 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3278,7 +3278,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>         intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
>         intel_set_pch_fifo_underrun_reporting(dev, pipe, true);
>
> -       intel_update_watermarks(dev);
> +       intel_update_watermarks(crtc);
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 if (encoder->pre_enable)
> @@ -3388,7 +3388,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>         if (intel_crtc->config.has_pch_encoder)
>                 intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, true);
>
> -       intel_update_watermarks(dev);
> +       intel_update_watermarks(crtc);
>
>         if (intel_crtc->config.has_pch_encoder)
>                 dev_priv->display.fdi_link_train(crtc);
> @@ -3520,7 +3520,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>         }
>
>         intel_crtc->active = false;
> -       intel_update_watermarks(dev);
> +       intel_update_watermarks(crtc);
>
>         mutex_lock(&dev->struct_mutex);
>         intel_update_fbc(dev);
> @@ -3577,7 +3577,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>         }
>
>         intel_crtc->active = false;
> -       intel_update_watermarks(dev);
> +       intel_update_watermarks(crtc);
>
>         mutex_lock(&dev->struct_mutex);
>         intel_update_fbc(dev);
> @@ -3677,7 +3677,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>                 return;
>
>         intel_crtc->active = true;
> -       intel_update_watermarks(dev);
> +       intel_update_watermarks(crtc);
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 if (encoder->pre_pll_enable)
> @@ -3722,7 +3722,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>                 return;
>
>         intel_crtc->active = true;
> -       intel_update_watermarks(dev);
> +       intel_update_watermarks(crtc);
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 if (encoder->pre_enable)
> @@ -3806,7 +3806,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>
>         intel_crtc->active = false;
>         intel_update_fbc(dev);
> -       intel_update_watermarks(dev);
> +       intel_update_watermarks(crtc);
>  }
>
>  static void i9xx_crtc_off(struct drm_crtc *crtc)
> @@ -4972,7 +4972,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>
>         ret = intel_pipe_set_base(crtc, x, y, fb);
>
> -       intel_update_watermarks(dev);
> +       intel_update_watermarks(crtc);
>
>         return ret;
>  }
> @@ -5860,7 +5860,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>
>         ret = intel_pipe_set_base(crtc, x, y, fb);
>
> -       intel_update_watermarks(dev);
> +       intel_update_watermarks(crtc);
>
>         return ret;
>  }
> @@ -6316,7 +6316,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>
>         ret = intel_pipe_set_base(crtc, x, y, fb);
>
> -       intel_update_watermarks(dev);
> +       intel_update_watermarks(crtc);
>
>         return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a38f5b2..8b132e2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -714,7 +714,7 @@ extern void hsw_fdi_link_train(struct drm_crtc *crtc);
>  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_watermarks(struct drm_crtc *crtc);
>  extern void intel_update_sprite_watermarks(struct drm_plane *plane,
>                                            struct drm_crtc *crtc,
>                                            uint32_t sprite_width, int pixel_size,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 6b1d003..774c57f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1087,10 +1087,10 @@ static struct drm_crtc *single_enabled_crtc(struct drm_device *dev)
>         return enabled;
>  }
>
> -static void pineview_update_wm(struct drm_device *dev)
> +static void pineview_update_wm(struct drm_crtc *crtc)
>  {
> +       struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct drm_crtc *crtc;
>         const struct cxsr_latency *latency;
>         u32 reg;
>         unsigned long wm;
> @@ -1365,8 +1365,9 @@ static void vlv_update_drain_latency(struct drm_device *dev)
>
>  #define single_plane_enabled(mask) is_power_of_2(mask)
>
> -static void valleyview_update_wm(struct drm_device *dev)
> +static void valleyview_update_wm(struct drm_crtc *crtc)
>  {
> +       struct drm_device *dev = crtc->dev;
>         static const int sr_latency_ns = 12000;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
> @@ -1424,8 +1425,9 @@ static void valleyview_update_wm(struct drm_device *dev)
>                    (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
>  }
>
> -static void g4x_update_wm(struct drm_device *dev)
> +static void g4x_update_wm(struct drm_crtc *crtc)
>  {
> +       struct drm_device *dev = crtc->dev;
>         static const int sr_latency_ns = 12000;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
> @@ -1476,10 +1478,10 @@ static void g4x_update_wm(struct drm_device *dev)
>                    (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
>  }
>
> -static void i965_update_wm(struct drm_device *dev)
> +static void i965_update_wm(struct drm_crtc *crtc)
>  {
> +       struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct drm_crtc *crtc;
>         int srwm = 1;
>         int cursor_sr = 16;
>
> @@ -1541,8 +1543,9 @@ static void i965_update_wm(struct drm_device *dev)
>         I915_WRITE(DSPFW3, (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
>  }
>
> -static void i9xx_update_wm(struct drm_device *dev)
> +static void i9xx_update_wm(struct drm_crtc *crtc)
>  {
> +       struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         const struct intel_watermark_params *wm_info;
>         uint32_t fwater_lo;
> @@ -1550,7 +1553,7 @@ static void i9xx_update_wm(struct drm_device *dev)
>         int cwm, srwm = 1;
>         int fifo_size;
>         int planea_wm, planeb_wm;
> -       struct drm_crtc *crtc, *enabled = NULL;
> +       struct drm_crtc *enabled = NULL;
>
>         if (IS_I945GM(dev))
>                 wm_info = &i945_wm_info;
> @@ -1658,10 +1661,10 @@ static void i9xx_update_wm(struct drm_device *dev)
>         }
>  }
>
> -static void i830_update_wm(struct drm_device *dev)
> +static void i830_update_wm(struct drm_crtc *crtc)
>  {
> +       struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct drm_crtc *crtc;
>         uint32_t fwater_lo;
>         int planea_wm;
>
> @@ -1785,8 +1788,9 @@ static bool ironlake_compute_srwm(struct drm_device *dev, int level, int plane,
>                                    display, cursor);
>  }
>
> -static void ironlake_update_wm(struct drm_device *dev)
> +static void ironlake_update_wm(struct drm_crtc *crtc)
>  {
> +       struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         int fbc_wm, plane_wm, cursor_wm;
>         unsigned int enabled;
> @@ -1868,8 +1872,9 @@ static void ironlake_update_wm(struct drm_device *dev)
>          */
>  }
>
> -static void sandybridge_update_wm(struct drm_device *dev)
> +static void sandybridge_update_wm(struct drm_crtc *crtc)
>  {
> +       struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         int latency = dev_priv->wm.pri_latency[0] * 100;        /* In unit 0.1us */
>         u32 val;
> @@ -1970,8 +1975,9 @@ static void sandybridge_update_wm(struct drm_device *dev)
>                    cursor_wm);
>  }
>
> -static void ivybridge_update_wm(struct drm_device *dev)
> +static void ivybridge_update_wm(struct drm_crtc *crtc)
>  {
> +       struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         int latency = dev_priv->wm.pri_latency[0] * 100;        /* In unit 0.1us */
>         u32 val;
> @@ -2841,8 +2847,9 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
>                 I915_WRITE(WM3_LP_ILK, results->wm_lp[2]);
>  }
>
> -static void haswell_update_wm(struct drm_device *dev)
> +static void haswell_update_wm(struct drm_crtc *crtc)
>  {
> +       struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct hsw_wm_maximums lp_max_1_2, lp_max_5_6;
>         struct hsw_pipe_wm_parameters params[3];
> @@ -2879,7 +2886,7 @@ static void haswell_update_sprite_wm(struct drm_plane *plane,
>         intel_plane->wm.horiz_pixels = sprite_width;
>         intel_plane->wm.bytes_per_pixel = pixel_size;
>
> -       haswell_update_wm(plane->dev);
> +       haswell_update_wm(crtc);
>  }
>
>  static bool
> @@ -3076,12 +3083,12 @@ static void sandybridge_update_sprite_wm(struct drm_plane *plane,
>   * We don't use the sprite, so we can ignore that.  And on Crestline we have
>   * to set the non-SR watermarks to 8.
>   */
> -void intel_update_watermarks(struct drm_device *dev)
> +void intel_update_watermarks(struct drm_crtc *crtc)
>  {
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>
>         if (dev_priv->display.update_wm)
> -               dev_priv->display.update_wm(dev);
> +               dev_priv->display.update_wm(crtc);
>  }
>
>  void intel_update_sprite_watermarks(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 78b621c..a8a72337 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -285,7 +285,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>                 dev_priv->sprite_scaling_enabled |= 1 << pipe;
>
>                 if (!scaling_was_enabled) {
> -                       intel_update_watermarks(dev);
> +                       intel_update_watermarks(crtc);
>                         intel_wait_for_vblank(dev, pipe);
>                 }
>                 sprscale = SPRITE_SCALE_ENABLE | (src_w << 16) | src_h;
> @@ -320,7 +320,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>
>         /* potentially re-enable LP watermarks */
>         if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled)
> -               intel_update_watermarks(dev);
> +               intel_update_watermarks(crtc);
>  }
>
>  static void
> @@ -346,7 +346,7 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
>
>         /* potentially re-enable LP watermarks */
>         if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled)
> -               intel_update_watermarks(dev);
> +               intel_update_watermarks(crtc);
>  }
>
>  static int
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 02/19] drm/i915: Call intel_update_watermarks() in specific place during modeset

[Paulo Zanoni]

2013/8/30  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Make the call to intel_update_watermarks() just once or twice during
> modeset. Ideally it should happen independently when each plane gets
> enabled/disabled, but for now it seems better to keep it in central
> place. We can improve things when we get all the planes sorted out
> in a better way.
>
> When enabling set up the watermarks just before the pipe is enabled.
> And when disabling we need to wait until we've marked the crtc as
> inactive.

Why do we need to wait until we've marked the CRTC as inactive?
(Daniel/Ville should put the answer in the commit message)

The patch looks correct, but <bikeshed> I'd separate the "don't update
watermarks at crtc->mode_set" part from the current patch, since if we
start getting regressions we won't know if they're caused by the fact
that now we don't adjust the watermarks before enabling the PCH and
doing link training, or if they're caused by the fact that we don't
update them anymore at mode_set </bikeshed>. But since it looks
correct, we shouldn't really be getting regressions...

With or without changes: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>



>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 13856ec..a5181fe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3278,8 +3278,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>         intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
>         intel_set_pch_fifo_underrun_reporting(dev, pipe, true);
>
> -       intel_update_watermarks(crtc);
> -
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 if (encoder->pre_enable)
>                         encoder->pre_enable(encoder);
> @@ -3302,6 +3300,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>          */
>         intel_crtc_load_lut(crtc);
>
> +       intel_update_watermarks(crtc);
>         intel_enable_pipe(dev_priv, pipe,
>                           intel_crtc->config.has_pch_encoder, false);
>         intel_enable_plane(dev_priv, plane, pipe);
> @@ -3388,8 +3387,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>         if (intel_crtc->config.has_pch_encoder)
>                 intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, true);
>
> -       intel_update_watermarks(crtc);
> -
>         if (intel_crtc->config.has_pch_encoder)
>                 dev_priv->display.fdi_link_train(crtc);
>
> @@ -3410,6 +3407,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>         intel_ddi_set_pipe_settings(crtc);
>         intel_ddi_enable_transcoder_func(crtc);
>
> +       intel_update_watermarks(crtc);
>         intel_enable_pipe(dev_priv, pipe,
>                           intel_crtc->config.has_pch_encoder, false);
>         intel_enable_plane(dev_priv, plane, pipe);
> @@ -3677,7 +3675,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>                 return;
>
>         intel_crtc->active = true;
> -       intel_update_watermarks(crtc);
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 if (encoder->pre_pll_enable)
> @@ -3696,6 +3693,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>
>         intel_crtc_load_lut(crtc);
>
> +       intel_update_watermarks(crtc);
>         intel_enable_pipe(dev_priv, pipe, false, is_dsi);
>         intel_enable_plane(dev_priv, plane, pipe);
>         intel_enable_planes(crtc);
> @@ -3722,7 +3720,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>                 return;
>
>         intel_crtc->active = true;
> -       intel_update_watermarks(crtc);
>
>         for_each_encoder_on_crtc(dev, crtc, encoder)
>                 if (encoder->pre_enable)
> @@ -3734,6 +3731,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>
>         intel_crtc_load_lut(crtc);
>
> +       intel_update_watermarks(crtc);
>         intel_enable_pipe(dev_priv, pipe, false, false);
>         intel_enable_plane(dev_priv, plane, pipe);
>         intel_enable_planes(crtc);
> @@ -3805,8 +3803,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>                 i9xx_disable_pll(dev_priv, pipe);
>
>         intel_crtc->active = false;
> -       intel_update_fbc(dev);
>         intel_update_watermarks(crtc);
> +
> +       intel_update_fbc(dev);
>  }
>
>  static void i9xx_crtc_off(struct drm_crtc *crtc)
> @@ -4972,8 +4971,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>
>         ret = intel_pipe_set_base(crtc, x, y, fb);
>
> -       intel_update_watermarks(crtc);
> -
>         return ret;
>  }
>
> @@ -5860,8 +5857,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>
>         ret = intel_pipe_set_base(crtc, x, y, fb);
>
> -       intel_update_watermarks(crtc);
> -
>         return ret;
>  }
>
> @@ -6316,8 +6311,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>
>         ret = intel_pipe_set_base(crtc, x, y, fb);
>
> -       intel_update_watermarks(crtc);
> -
>         return ret;
>  }
>
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 01/19] drm/i915: Pass crtc to intel_update_watermarks()

[Ville Syrjälä]

On Fri, Aug 30, 2013 at 05:09:49PM -0300, Paulo Zanoni wrote:
> 2013/8/30  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Passing the appropriate crtc to intel_update_watermarks() should help
> > in avoiding needless work in the future.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I like the fact that now we're passing the CRTC, but <bikeshed> my
> only worry is that now some functions overwrite the "crtc" we pass as
> argument, so this might be confusing and maybe lead to bugs in the
> future: perhaps the argument could be called unused_crtc or
> ignored_crtc, then we'd keep the "crtc" variables they already have
> </bikeshed>. But it's just a bikeshed, so with or without changes:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>.

Right. It could be a bit confusing. I'll change it.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 03/19] drm/i915: Constify some watermark data

[Paulo Zanoni]

2013/8/30  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> hsw_pipe_wm_parameters and hsw_wm_maximums typically are read only. Make
> them const.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 774c57f..96493dc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2202,7 +2202,7 @@ struct intel_wm_config {
>   * For both WM_PIPE and WM_LP.
>   * mem_value must be in 0.1us units.
>   */
> -static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
> +static uint32_t ilk_compute_pri_wm(const struct hsw_pipe_wm_parameters *params,
>                                    uint32_t mem_value,
>                                    bool is_lp)
>  {
> @@ -2231,7 +2231,7 @@ static uint32_t ilk_compute_pri_wm(struct hsw_pipe_wm_parameters *params,
>   * For both WM_PIPE and WM_LP.
>   * mem_value must be in 0.1us units.
>   */
> -static uint32_t ilk_compute_spr_wm(struct hsw_pipe_wm_parameters *params,
> +static uint32_t ilk_compute_spr_wm(const struct hsw_pipe_wm_parameters *params,
>                                    uint32_t mem_value)
>  {
>         uint32_t method1, method2;
> @@ -2254,7 +2254,7 @@ static uint32_t ilk_compute_spr_wm(struct hsw_pipe_wm_parameters *params,
>   * For both WM_PIPE and WM_LP.
>   * mem_value must be in 0.1us units.
>   */
> -static uint32_t ilk_compute_cur_wm(struct hsw_pipe_wm_parameters *params,
> +static uint32_t ilk_compute_cur_wm(const struct hsw_pipe_wm_parameters *params,
>                                    uint32_t mem_value)
>  {
>         if (!params->active || !params->cur.enabled)
> @@ -2268,7 +2268,7 @@ static uint32_t ilk_compute_cur_wm(struct hsw_pipe_wm_parameters *params,
>  }
>
>  /* Only for WM_LP. */
> -static uint32_t ilk_compute_fbc_wm(struct hsw_pipe_wm_parameters *params,
> +static uint32_t ilk_compute_fbc_wm(const struct hsw_pipe_wm_parameters *params,
>                                    uint32_t pri_val)
>  {
>         if (!params->active || !params->pri.enabled)
> @@ -2419,7 +2419,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,
> +                                const struct hsw_pipe_wm_parameters *p,
>                                  struct intel_wm_level *result)
>  {
>         uint16_t pri_latency = dev_priv->wm.pri_latency[level];
> @@ -2441,8 +2441,8 @@ 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,
> +                             int level, const struct hsw_wm_maximums *max,
> +                             const struct hsw_pipe_wm_parameters *params,
>                               struct intel_wm_level *result)
>  {
>         enum pipe pipe;
> @@ -2462,7 +2462,7 @@ static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
>
>  static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv,
>                                     enum pipe pipe,
> -                                   struct hsw_pipe_wm_parameters *params)
> +                                   const struct hsw_pipe_wm_parameters *params)
>  {
>         uint32_t pri_val, cur_val, spr_val;
>         /* WM0 latency values stored in 0.1us units */
> @@ -2670,8 +2670,8 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
>  }
>
>  static void hsw_compute_wm_results(struct drm_device *dev,
> -                                  struct hsw_pipe_wm_parameters *params,
> -                                  struct hsw_wm_maximums *lp_maximums,
> +                                  const struct hsw_pipe_wm_parameters *params,
> +                                  const struct hsw_wm_maximums *lp_maximums,
>                                    struct hsw_wm_values *results)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 02/19] drm/i915: Call intel_update_watermarks() in specific place during modeset

[Ville Syrjälä]

On Fri, Aug 30, 2013 at 05:26:29PM -0300, Paulo Zanoni wrote:
> 2013/8/30  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Make the call to intel_update_watermarks() just once or twice during
> > modeset. Ideally it should happen independently when each plane gets
> > enabled/disabled, but for now it seems better to keep it in central
> > place. We can improve things when we get all the planes sorted out
> > in a better way.
> >
> > When enabling set up the watermarks just before the pipe is enabled.
> > And when disabling we need to wait until we've marked the crtc as
> > inactive.
> 
> Why do we need to wait until we've marked the CRTC as inactive?
> (Daniel/Ville should put the answer in the commit message)

Because the watermark compute code looks at intel_crtc->active. If we
compute the watermarks before, the code thinks the pipe is active.

Hmm. BTW now that I look at intel_crtc_active() I start to wonder why it
looks at the clock in the user specified mode. In fact most (maybe all?)
of the pre-hsw watermark code is fscked up and it looks at the wrong
mode. Sigh. Suppose I need to make a quick for all that as well...

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 04/19] drm/i915: Use ilk_compute_wm_level to compute WM_PIPE values

[Paulo Zanoni]

2013/8/30  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Unify the code a bit to use ilk_compute_wm_level for all watermark
> levels.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Looks correct and doesn't change the WM values set on my specific
eDP+HDMI config here...

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

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 44 ++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 96493dc..d118ee1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2460,33 +2460,31 @@ static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
>         return ilk_check_wm(level, max, result);
>  }
>
> -static uint32_t hsw_compute_wm_pipe(struct drm_i915_private *dev_priv,
> -                                   enum pipe pipe,
> +
> +static uint32_t hsw_compute_wm_pipe(struct drm_device *dev,
>                                     const struct hsw_pipe_wm_parameters *params)
>  {
> -       uint32_t pri_val, cur_val, spr_val;
> -       /* WM0 latency values stored in 0.1us units */
> -       uint16_t pri_latency = dev_priv->wm.pri_latency[0];
> -       uint16_t spr_latency = dev_priv->wm.spr_latency[0];
> -       uint16_t cur_latency = dev_priv->wm.cur_latency[0];
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct intel_wm_config config = {
> +               .num_pipes_active = 1,
> +               .sprites_enabled = params->spr.enabled,
> +               .sprites_scaled = params->spr.scaled,
> +       };
> +       struct hsw_wm_maximums max;
> +       struct intel_wm_level res;
> +
> +       if (!params->active)
> +               return 0;
> +
> +       ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
>
> -       pri_val = ilk_compute_pri_wm(params, pri_latency, false);
> -       spr_val = ilk_compute_spr_wm(params, spr_latency);
> -       cur_val = ilk_compute_cur_wm(params, cur_latency);
> +       ilk_compute_wm_level(dev_priv, 0, params, &res);
>
> -       WARN(pri_val > 127,
> -            "Primary WM error, mode not supported for pipe %c\n",
> -            pipe_name(pipe));
> -       WARN(spr_val > 127,
> -            "Sprite WM error, mode not supported for pipe %c\n",
> -            pipe_name(pipe));
> -       WARN(cur_val > 63,
> -            "Cursor WM error, mode not supported for pipe %c\n",
> -            pipe_name(pipe));
> +       ilk_check_wm(0, &max, &res);
>
> -       return (pri_val << WM0_PIPE_PLANE_SHIFT) |
> -              (spr_val << WM0_PIPE_SPRITE_SHIFT) |
> -              cur_val;
> +       return (res.pri_val << WM0_PIPE_PLANE_SHIFT) |
> +              (res.spr_val << WM0_PIPE_SPRITE_SHIFT) |
> +              res.cur_val;
>  }
>
>  static uint32_t
> @@ -2715,7 +2713,7 @@ static void hsw_compute_wm_results(struct drm_device *dev,
>         }
>
>         for_each_pipe(pipe)
> -               results->wm_pipe[pipe] = hsw_compute_wm_pipe(dev_priv, pipe,
> +               results->wm_pipe[pipe] = hsw_compute_wm_pipe(dev,
>                                                              &params[pipe]);
>
>         for_each_pipe(pipe) {
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 05/19] drm/i915: Refactor max WM level

[Paulo Zanoni]

2013/8/30  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Pull the expected max WM level determinations out to a separate
> function. Will have another user soon.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d118ee1..163ba74 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2558,19 +2558,22 @@ static void intel_fixup_cur_wm_latency(struct drm_device *dev, uint16_t wm[5])
>                 wm[3] *= 2;
>  }
>
> -static void intel_print_wm_latency(struct drm_device *dev,
> -                                  const char *name,
> -                                  const uint16_t wm[5])
> +static int ilk_wm_max_level(const struct drm_device *dev)
>  {
> -       int level, max_level;
> -
>         /* how many WM levels are we expecting */
>         if (IS_HASWELL(dev))
> -               max_level = 4;
> +               return 4;
>         else if (INTEL_INFO(dev)->gen >= 6)
> -               max_level = 3;
> +               return 3;
>         else
> -               max_level = 2;
> +               return 2;
> +}
> +
> +static void intel_print_wm_latency(struct drm_device *dev,
> +                                  const char *name,
> +                                  const uint16_t wm[5])
> +{
> +       int level, max_level = ilk_wm_max_level(dev);
>
>         for (level = 0; level <= max_level; level++) {
>                 unsigned int latency = wm[level];
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 02/19] drm/i915: Call intel_update_watermarks() in specific place during modeset

[Daniel Vetter]

On Fri, Aug 30, 2013 at 11:49:52PM +0300, Ville Syrjälä wrote:
> On Fri, Aug 30, 2013 at 05:26:29PM -0300, Paulo Zanoni wrote:
> > 2013/8/30  <ville.syrjala@linux.intel.com>:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Make the call to intel_update_watermarks() just once or twice during
> > > modeset. Ideally it should happen independently when each plane gets
> > > enabled/disabled, but for now it seems better to keep it in central
> > > place. We can improve things when we get all the planes sorted out
> > > in a better way.
> > >
> > > When enabling set up the watermarks just before the pipe is enabled.
> > > And when disabling we need to wait until we've marked the crtc as
> > > inactive.
> > 
> > Why do we need to wait until we've marked the CRTC as inactive?
> > (Daniel/Ville should put the answer in the commit message)
> 
> Because the watermark compute code looks at intel_crtc->active. If we
> compute the watermarks before, the code thinks the pipe is active.
> 
> Hmm. BTW now that I look at intel_crtc_active() I start to wonder why it
> looks at the clock in the user specified mode. In fact most (maybe all?)
> of the pre-hsw watermark code is fscked up and it looks at the wrong
> mode. Sigh. Suppose I need to make a quick for all that as well...

That was a fallout from the partial modeset state recovery on boot-up -
we've ended up doing divide-by-zeros since the pipe was marked as active
but we've lacked the dotclock required to compute the new watermarks. So
for those we've just skipped this.

Since hsw still has no dotclock readout support we need to keep onto this
hack for a while longer. But adding an XXX comment here would be useful.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v2] drm/i915: Call intel_update_watermarks() in specific place during modeset

[ville.syrjala]

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

Make the call to intel_update_watermarks() just once or twice during
modeset. Ideally it should happen independently when each plane gets
enabled/disabled, but for now it seems better to keep it in central
place. We can improve things when we get all the planes sorted out
in a better way.

When enabling set up the watermarks just before the pipe is enabled.
And when disabling we need to wait until we've marked the crtc as
inactive, as otherwise intel_crtc_active() would still think the pipe
is enabled and the computed watermarks would reflect that.

v2: Pimp up the commit message a bit

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index adbd54f..80fea0c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3280,8 +3280,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
 	intel_set_pch_fifo_underrun_reporting(dev, pipe, true);
 
-	intel_update_watermarks(crtc);
-
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_enable)
 			encoder->pre_enable(encoder);
@@ -3304,6 +3302,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	 */
 	intel_crtc_load_lut(crtc);
 
+	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe,
 			  intel_crtc->config.has_pch_encoder, false);
 	intel_enable_plane(dev_priv, plane, pipe);
@@ -3390,8 +3389,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	if (intel_crtc->config.has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, true);
 
-	intel_update_watermarks(crtc);
-
 	if (intel_crtc->config.has_pch_encoder)
 		dev_priv->display.fdi_link_train(crtc);
 
@@ -3412,6 +3409,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	intel_ddi_set_pipe_settings(crtc);
 	intel_ddi_enable_transcoder_func(crtc);
 
+	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe,
 			  intel_crtc->config.has_pch_encoder, false);
 	intel_enable_plane(dev_priv, plane, pipe);
@@ -3683,7 +3681,6 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	intel_crtc->active = true;
-	intel_update_watermarks(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_pll_enable)
@@ -3702,6 +3699,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	intel_crtc_load_lut(crtc);
 
+	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe, false, is_dsi);
 	intel_enable_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
@@ -3728,7 +3726,6 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	intel_crtc->active = true;
-	intel_update_watermarks(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_enable)
@@ -3740,6 +3737,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 
 	intel_crtc_load_lut(crtc);
 
+	intel_update_watermarks(crtc);
 	intel_enable_pipe(dev_priv, pipe, false, false);
 	intel_enable_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
@@ -3811,8 +3809,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 		i9xx_disable_pll(dev_priv, pipe);
 
 	intel_crtc->active = false;
-	intel_update_fbc(dev);
 	intel_update_watermarks(crtc);
+
+	intel_update_fbc(dev);
 }
 
 static void i9xx_crtc_off(struct drm_crtc *crtc)
@@ -4998,8 +4997,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(crtc);
-
 	return ret;
 }
 
@@ -5907,8 +5904,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(crtc);
-
 	return ret;
 }
 
@@ -6427,8 +6422,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(crtc);
-
 	return ret;
 }
 
-- 
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: Pass crtc to intel_update_watermarks()

[ville.syrjala]

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

Passing the appropriate crtc to intel_update_watermarks() should help
in avoiding needless work in the future.

v2: Avoid clash with internal 'crtc' variable in some wm functions

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 20 +++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |  2 +-
 drivers/gpu/drm/i915/intel_pm.c      | 38 +++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_sprite.c  |  6 +++---
 5 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 152291c..32b947b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -361,7 +361,7 @@ struct drm_i915_display_funcs {
 			  int target, int refclk,
 			  struct dpll *match_clock,
 			  struct dpll *best_clock);
-	void (*update_wm)(struct drm_device *dev);
+	void (*update_wm)(struct drm_crtc *crtc);
 	void (*update_sprite_wm)(struct drm_plane *plane,
 				 struct drm_crtc *crtc,
 				 uint32_t sprite_width, int pixel_size,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 60284fc..adbd54f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3280,7 +3280,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, true);
 	intel_set_pch_fifo_underrun_reporting(dev, pipe, true);
 
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_enable)
@@ -3390,7 +3390,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 	if (intel_crtc->config.has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev, TRANSCODER_A, true);
 
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	if (intel_crtc->config.has_pch_encoder)
 		dev_priv->display.fdi_link_train(crtc);
@@ -3524,7 +3524,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	}
 
 	intel_crtc->active = false;
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
@@ -3583,7 +3583,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	}
 
 	intel_crtc->active = false;
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
@@ -3683,7 +3683,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	intel_crtc->active = true;
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_pll_enable)
@@ -3728,7 +3728,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	intel_crtc->active = true;
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_enable)
@@ -3812,7 +3812,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc->active = false;
 	intel_update_fbc(dev);
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 }
 
 static void i9xx_crtc_off(struct drm_crtc *crtc)
@@ -4998,7 +4998,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	return ret;
 }
@@ -5907,7 +5907,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	return ret;
 }
@@ -6427,7 +6427,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(dev);
+	intel_update_watermarks(crtc);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 774ebb6..2bd3fb9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -732,7 +732,7 @@ extern void intel_ddi_init(struct drm_device *dev, enum port port);
 extern enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder);
 
 /* For use by IVB LP watermark workaround in intel_sprite.c */
-extern void intel_update_watermarks(struct drm_device *dev);
+extern void intel_update_watermarks(struct drm_crtc *crtc);
 extern void intel_update_sprite_watermarks(struct drm_plane *plane,
 					   struct drm_crtc *crtc,
 					   uint32_t sprite_width, int pixel_size,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8cffef4..a841d7e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1081,8 +1081,9 @@ static struct drm_crtc *single_enabled_crtc(struct drm_device *dev)
 	return enabled;
 }
 
-static void pineview_update_wm(struct drm_device *dev)
+static void pineview_update_wm(struct drm_crtc *unused_crtc)
 {
+	struct drm_device *dev = unused_crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	const struct cxsr_latency *latency;
@@ -1363,8 +1364,9 @@ static void vlv_update_drain_latency(struct drm_device *dev)
 
 #define single_plane_enabled(mask) is_power_of_2(mask)
 
-static void valleyview_update_wm(struct drm_device *dev)
+static void valleyview_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	static const int sr_latency_ns = 12000;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
@@ -1422,8 +1424,9 @@ static void valleyview_update_wm(struct drm_device *dev)
 		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
 }
 
-static void g4x_update_wm(struct drm_device *dev)
+static void g4x_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	static const int sr_latency_ns = 12000;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int planea_wm, planeb_wm, cursora_wm, cursorb_wm;
@@ -1474,8 +1477,9 @@ static void g4x_update_wm(struct drm_device *dev)
 		   (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
 }
 
-static void i965_update_wm(struct drm_device *dev)
+static void i965_update_wm(struct drm_crtc *unused_crtc)
 {
+	struct drm_device *dev = unused_crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	int srwm = 1;
@@ -1541,8 +1545,9 @@ static void i965_update_wm(struct drm_device *dev)
 	I915_WRITE(DSPFW3, (cursor_sr << DSPFW_CURSOR_SR_SHIFT));
 }
 
-static void i9xx_update_wm(struct drm_device *dev)
+static void i9xx_update_wm(struct drm_crtc *unused_crtc)
 {
+	struct drm_device *dev = unused_crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	const struct intel_watermark_params *wm_info;
 	uint32_t fwater_lo;
@@ -1660,8 +1665,9 @@ static void i9xx_update_wm(struct drm_device *dev)
 	}
 }
 
-static void i830_update_wm(struct drm_device *dev)
+static void i830_update_wm(struct drm_crtc *unused_crtc)
 {
+	struct drm_device *dev = unused_crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
 	uint32_t fwater_lo;
@@ -1790,8 +1796,9 @@ static bool ironlake_compute_srwm(struct drm_device *dev, int level, int plane,
 				   display, cursor);
 }
 
-static void ironlake_update_wm(struct drm_device *dev)
+static void ironlake_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int fbc_wm, plane_wm, cursor_wm;
 	unsigned int enabled;
@@ -1873,8 +1880,9 @@ static void ironlake_update_wm(struct drm_device *dev)
 	 */
 }
 
-static void sandybridge_update_wm(struct drm_device *dev)
+static void sandybridge_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int latency = dev_priv->wm.pri_latency[0] * 100;	/* In unit 0.1us */
 	u32 val;
@@ -1975,8 +1983,9 @@ static void sandybridge_update_wm(struct drm_device *dev)
 		   cursor_wm);
 }
 
-static void ivybridge_update_wm(struct drm_device *dev)
+static void ivybridge_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int latency = dev_priv->wm.pri_latency[0] * 100;	/* In unit 0.1us */
 	u32 val;
@@ -2845,8 +2854,9 @@ static void hsw_write_wm_values(struct drm_i915_private *dev_priv,
 		I915_WRITE(WM3_LP_ILK, results->wm_lp[2]);
 }
 
-static void haswell_update_wm(struct drm_device *dev)
+static void haswell_update_wm(struct drm_crtc *crtc)
 {
+	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct hsw_wm_maximums lp_max_1_2, lp_max_5_6;
 	struct hsw_pipe_wm_parameters params[3];
@@ -2883,7 +2893,7 @@ static void haswell_update_sprite_wm(struct drm_plane *plane,
 	intel_plane->wm.horiz_pixels = sprite_width;
 	intel_plane->wm.bytes_per_pixel = pixel_size;
 
-	haswell_update_wm(plane->dev);
+	haswell_update_wm(crtc);
 }
 
 static bool
@@ -3080,12 +3090,12 @@ static void sandybridge_update_sprite_wm(struct drm_plane *plane,
  * We don't use the sprite, so we can ignore that.  And on Crestline we have
  * to set the non-SR watermarks to 8.
  */
-void intel_update_watermarks(struct drm_device *dev)
+void intel_update_watermarks(struct drm_crtc *crtc)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 
 	if (dev_priv->display.update_wm)
-		dev_priv->display.update_wm(dev);
+		dev_priv->display.update_wm(crtc);
 }
 
 void intel_update_sprite_watermarks(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 71717e2..231b289 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -288,7 +288,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		dev_priv->sprite_scaling_enabled |= 1 << pipe;
 
 		if (!scaling_was_enabled) {
-			intel_update_watermarks(dev);
+			intel_update_watermarks(crtc);
 			intel_wait_for_vblank(dev, pipe);
 		}
 		sprscale = SPRITE_SCALE_ENABLE | (src_w << 16) | src_h;
@@ -323,7 +323,7 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	/* potentially re-enable LP watermarks */
 	if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled)
-		intel_update_watermarks(dev);
+		intel_update_watermarks(crtc);
 }
 
 static void
@@ -349,7 +349,7 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc)
 
 	/* potentially re-enable LP watermarks */
 	if (scaling_was_enabled && !dev_priv->sprite_scaling_enabled)
-		intel_update_watermarks(dev);
+		intel_update_watermarks(crtc);
 }
 
 static int
-- 
1.8.1.5

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

Re: [PATCH 05/19] drm/i915: Refactor max WM level

[Daniel Vetter]

On Fri, Aug 30, 2013 at 06:40:55PM -0300, Paulo Zanoni wrote:
> 2013/8/30  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Pull the expected max WM level determinations out to a separate
> > function. Will have another user soon.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

Re: [PATCH 06/19] drm/i915: Add intel_pipe_wm and prepare for watermark pre-compute

[Paulo Zanoni]

2013/8/30  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Introduce a new struct intel_pipe_wm which contains all the
> watermarks for a single pipe. Use it to unify the LP0 and LP1+
> watermark computations so that we can just iterate through the
> watermark levels neatly and call ilk_compute_wm_level() for each.
>
> Also add another tool ilk_wm_merge() that merges the LP1+ watermarks
> from all pipes. For that, embed one intel_pipe_wm inside intel_crtc that
> contains the currently valid watermarks for each pipe.
>
> This is mainly preparatory work for pre-computing the watermarks for
> each pipe and merging them at a later time. For now the merging still
> happens immediately.

>From the commit message, it seems this patch could be split in 4 or 5
sub-patches.


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  12 +++
>  drivers/gpu/drm/i915/intel_pm.c  | 190 +++++++++++++++++++++++----------------
>  2 files changed, 126 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8b132e2..664df77 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -293,6 +293,12 @@ struct intel_crtc_config {
>         bool ips_enabled;
>  };
>
> +struct intel_pipe_wm {
> +       struct intel_wm_level wm[5];
> +       uint32_t linetime;
> +       bool fbc_wm_enabled;
> +};
> +
>  struct intel_crtc {
>         struct drm_crtc base;
>         enum pipe pipe;
> @@ -333,6 +339,12 @@ struct intel_crtc {
>         /* Access to these should be protected by dev_priv->irq_lock. */
>         bool cpu_fifo_underrun_disabled;
>         bool pch_fifo_underrun_disabled;
> +
> +       /* per-pipe watermark state */
> +       struct {
> +               /* watermarks currently being used  */
> +               struct intel_pipe_wm active;

The fact that we call "active" may imply that we have "inactive" WMs.


> +       } wm;
>  };
>
>  struct intel_plane_wm_parameters {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 163ba74..c6f105f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2440,53 +2440,6 @@ static void ilk_compute_wm_level(struct drm_i915_private *dev_priv,
>         result->enable = true;
>  }
>
> -static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
> -                             int level, const struct hsw_wm_maximums *max,
> -                             const struct hsw_pipe_wm_parameters *params,
> -                             struct intel_wm_level *result)
> -{
> -       enum pipe pipe;
> -       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]);
> -
> -       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;
> -
> -       return ilk_check_wm(level, max, result);
> -}
> -
> -
> -static uint32_t hsw_compute_wm_pipe(struct drm_device *dev,
> -                                   const struct hsw_pipe_wm_parameters *params)
> -{
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct intel_wm_config config = {
> -               .num_pipes_active = 1,
> -               .sprites_enabled = params->spr.enabled,
> -               .sprites_scaled = params->spr.scaled,
> -       };
> -       struct hsw_wm_maximums max;
> -       struct intel_wm_level res;
> -
> -       if (!params->active)
> -               return 0;
> -
> -       ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> -
> -       ilk_compute_wm_level(dev_priv, 0, params, &res);
> -
> -       ilk_check_wm(0, &max, &res);
> -
> -       return (res.pri_val << WM0_PIPE_PLANE_SHIFT) |
> -              (res.spr_val << WM0_PIPE_SPRITE_SHIFT) |
> -              res.cur_val;
> -}
> -
>  static uint32_t
>  hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
>  {
> @@ -2670,44 +2623,121 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
>                 *lp_max_5_6 = *lp_max_1_2;
>  }
>
> +/* Compute new watermarks for the pipe */
> +static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> +                                 const struct hsw_pipe_wm_parameters *params,
> +                                 struct intel_pipe_wm *pipe_wm)
> +{
> +       struct drm_device *dev = crtc->dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       int level, max_level = ilk_wm_max_level(dev);
> +       struct intel_wm_config config = {
> +               .num_pipes_active = 1,
> +               .sprites_enabled = params->spr.enabled,
> +               .sprites_scaled = params->spr.scaled,
> +       };
> +       struct hsw_wm_maximums max;
> +
> +       memset(pipe_wm, 0, sizeof(*pipe_wm));
> +
> +       ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);

Can you please add a nice comment explaining why we're using
INTEL_DDB_PART_1_2 and ignoring the 5_6 partitioning model? I know the
old code doesn't have it, but I had to look at the code again to try
to understand it.


> +
> +       for (level = 0; level <= max_level; level++)
> +               ilk_compute_wm_level(dev_priv, level, params,
> +                                    &pipe_wm->wm[level]);
> +
> +       pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
> +
> +       /* At least LP0 must be valid */
> +       return ilk_check_wm(0, &max, &pipe_wm->wm[0]);

I think functions like "ilk_check_wm" and "ilk_wm_max" could be
renamed to longer names that really explain what they do. Can you
please do this in a separate patch? Maybe ilk_check_wm could be
ilk_can_enable_wm or something similar, and maybe ilk_wm_max could be
ilk_compute_wm_max_values or ilk_compute_wm_maximums or something else
(the word "max" is used for both "max level" and "maximum values per
level").


> +}
> +
> +/*
> + * Merge the watermarks from all active pipes for a specific level.
> + */
> +static void ilk_merge_wm_level(struct drm_device *dev,
> +                              int level,
> +                              struct intel_wm_level *ret_wm)
> +{
> +       const struct intel_crtc *intel_crtc;
> +

Don't we need to memset() ret_wm to zero here? Even if it's not needed
now because of some other previous memset, I fear future code changes
may introduce bugs due to non-zero values reaching this point.


> +       list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
> +               const struct intel_wm_level *wm =
> +                       &intel_crtc->wm.active.wm[level];
> +
> +               if (!wm->enable)
> +                       return;
> +
> +               ret_wm->pri_val = max(ret_wm->pri_val, wm->pri_val);
> +               ret_wm->spr_val = max(ret_wm->spr_val, wm->spr_val);
> +               ret_wm->cur_val = max(ret_wm->cur_val, wm->cur_val);
> +               ret_wm->fbc_val = max(ret_wm->fbc_val, wm->fbc_val);
> +       }
> +
> +       ret_wm->enable = true;

Maybe it would be nice to add a comment to explain why we set this to
true, and who would set this to untrue in case it can't be true.


> +}
> +
> +/*
> + * Merge all low power watermarks for all active pipes.
> + */
> +static void ilk_wm_merge(struct drm_device *dev,
> +                        const struct hsw_wm_maximums *max,
> +                        struct intel_pipe_wm *merged)
> +{
> +       int level, max_level = ilk_wm_max_level(dev);
> +
> +       merged->fbc_wm_enabled = true;
> +
> +       /* merge each WM1+ level */
> +       for (level = 1; level <= max_level; level++) {
> +               struct intel_wm_level *wm = &merged->wm[level];
> +
> +               ilk_merge_wm_level(dev, level, wm);
> +
> +               if (!ilk_check_wm(level, max, wm))
> +                       break;
> +
> +               /*
> +                * The spec says it is preferred to disable
> +                * FBC WMs instead of disabling a WM level.
> +                */
> +               if (wm->fbc_val > max->fbc) {
> +                       merged->fbc_wm_enabled = false;
> +                       wm->fbc_val = 0;
> +               }
> +       }
> +}
> +
>  static void hsw_compute_wm_results(struct drm_device *dev,
>                                    const struct hsw_pipe_wm_parameters *params,
>                                    const struct hsw_wm_maximums *lp_maximums,
>                                    struct hsw_wm_values *results)
>  {
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> -       struct drm_crtc *crtc;
> -       struct intel_wm_level lp_results[4] = {};
> -       enum pipe pipe;
> -       int level, max_level, wm_lp;
> +       struct intel_crtc *intel_crtc;
> +       int level, wm_lp;
> +       struct intel_pipe_wm lp_wm = {};

Took me a while to realize that intel_pipe_wm isn't used only as the
WM values for a single pipe (as the name suggests), but also for the
merged WMs. Maybe we should rename the struct. And here maybe rename
from lp_wm to merged_wms?


>
> -       for (level = 1; level <= 4; level++)
> -               if (!hsw_compute_lp_wm(dev_priv, level,
> -                                      lp_maximums, params,
> -                                      &lp_results[level - 1]))
> -                       break;
> -       max_level = level - 1;
> +       list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
> +               intel_compute_pipe_wm(&intel_crtc->base,
> +                                     &params[intel_crtc->pipe],
> +                                     &intel_crtc->wm.active);
> +
> +       ilk_wm_merge(dev, lp_maximums, &lp_wm);
>
>         memset(results, 0, sizeof(*results));
>
> -       /* The spec says it is preferred to disable FBC WMs instead of disabling
> -        * a WM level. */
> -       results->enable_fbc_wm = true;
> -       for (level = 1; level <= max_level; level++) {
> -               if (lp_results[level - 1].fbc_val > lp_maximums->fbc) {
> -                       results->enable_fbc_wm = false;
> -                       lp_results[level - 1].fbc_val = 0;
> -               }
> -       }
> +       results->enable_fbc_wm = lp_wm.fbc_wm_enabled;
>
> +       /* LP1+ register values */
>         for (wm_lp = 1; wm_lp <= 3; wm_lp++) {
>                 const struct intel_wm_level *r;
>
> -               level = (max_level == 4 && wm_lp > 1) ? wm_lp + 1 : wm_lp;
> -               if (level > max_level)
> +               level = wm_lp + (wm_lp >= 2 && lp_wm.wm[4].enable);

I find the older version easier to understand since it matches the
spec and doesn't rely on adding a boolean value. Also maybe we should
add a comment explaining that we do this since on HSW we have 4 levels
but only 3 slots (yes, I know, I should have added this comment when I
wrote the original code).


> +
> +               r = &lp_wm.wm[level];
> +               if (!r->enable)
>                         break;
>
> -               r = &lp_results[level - 1];
>                 results->wm_lp[wm_lp - 1] = HSW_WM_LP_VAL(level * 2,
>                                                           r->fbc_val,
>                                                           r->pri_val,
> @@ -2715,13 +2745,21 @@ static void hsw_compute_wm_results(struct drm_device *dev,
>                 results->wm_lp_spr[wm_lp - 1] = r->spr_val;
>         }
>
> -       for_each_pipe(pipe)
> -               results->wm_pipe[pipe] = hsw_compute_wm_pipe(dev,
> -                                                            &params[pipe]);
> +       /* LP0 register values */
> +       list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
> +               enum pipe pipe = intel_crtc->pipe;
> +               const struct intel_wm_level *r =
> +                       &intel_crtc->wm.active.wm[0];
>
> -       for_each_pipe(pipe) {
> -               crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> -               results->wm_linetime[pipe] = hsw_compute_linetime_wm(dev, crtc);
> +               if (!r->enable)
> +                       continue;

Does this mean "pipe not active"? Maybe add a comment explaining. This
is confusing since we're talking about LP0 levels and they shouldn't
really be disabled.


Our WM code is getting quite complex, it's not very easy to follow.
The comments/renames I asked are for stuff that took me a while to
understand. I also wouldn't complain if "v2" comes as a series of
smaller patches.


I did test this patch on my Haswell machine and the values still seem
to be correct.


> +
> +               results->wm_linetime[pipe] = intel_crtc->wm.active.linetime;
> +
> +               results->wm_pipe[pipe] =
> +                       (r->pri_val << WM0_PIPE_PLANE_SHIFT) |
> +                       (r->spr_val << WM0_PIPE_SPRITE_SHIFT) |
> +                       r->cur_val;
>         }
>  }
>
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 06/19] drm/i915: Add intel_pipe_wm and prepare for watermark pre-compute

[Ville Syrjälä]

On Mon, Sep 16, 2013 at 12:07:32PM -0300, Paulo Zanoni wrote:
> 2013/8/30  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Introduce a new struct intel_pipe_wm which contains all the
> > watermarks for a single pipe. Use it to unify the LP0 and LP1+
> > watermark computations so that we can just iterate through the
> > watermark levels neatly and call ilk_compute_wm_level() for each.
> >
> > Also add another tool ilk_wm_merge() that merges the LP1+ watermarks
> > from all pipes. For that, embed one intel_pipe_wm inside intel_crtc that
> > contains the currently valid watermarks for each pipe.
> >
> > This is mainly preparatory work for pre-computing the watermarks for
> > each pipe and merging them at a later time. For now the merging still
> > happens immediately.
> 
> >From the commit message, it seems this patch could be split in 4 or 5
> sub-patches.

I think if I split it up any more then then the indiviual patches might
stop making sense. Then you're going to be asking "why are you doing this
change here?" and I have to answer "see patch n+2" or something.

> 
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  12 +++
> >  drivers/gpu/drm/i915/intel_pm.c  | 190 +++++++++++++++++++++++----------------
> >  2 files changed, 126 insertions(+), 76 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 8b132e2..664df77 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -293,6 +293,12 @@ struct intel_crtc_config {
> >         bool ips_enabled;
> >  };
> >
> > +struct intel_pipe_wm {
> > +       struct intel_wm_level wm[5];
> > +       uint32_t linetime;
> > +       bool fbc_wm_enabled;
> > +};
> > +
> >  struct intel_crtc {
> >         struct drm_crtc base;
> >         enum pipe pipe;
> > @@ -333,6 +339,12 @@ struct intel_crtc {
> >         /* Access to these should be protected by dev_priv->irq_lock. */
> >         bool cpu_fifo_underrun_disabled;
> >         bool pch_fifo_underrun_disabled;
> > +
> > +       /* per-pipe watermark state */
> > +       struct {
> > +               /* watermarks currently being used  */
> > +               struct intel_pipe_wm active;
> 
> The fact that we call "active" may imply that we have "inactive" WMs.

Soon enough. Well I call the other guy 'pending' and I wanted to call
this guy 'current', but you can't call anyone by that name in the
kernel, so I had to choose something else.

> 
> 
> > +       } wm;
> >  };
> >
> >  struct intel_plane_wm_parameters {
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 163ba74..c6f105f 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2440,53 +2440,6 @@ static void ilk_compute_wm_level(struct drm_i915_private *dev_priv,
> >         result->enable = true;
> >  }
> >
> > -static bool hsw_compute_lp_wm(struct drm_i915_private *dev_priv,
> > -                             int level, const struct hsw_wm_maximums *max,
> > -                             const struct hsw_pipe_wm_parameters *params,
> > -                             struct intel_wm_level *result)
> > -{
> > -       enum pipe pipe;
> > -       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]);
> > -
> > -       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;
> > -
> > -       return ilk_check_wm(level, max, result);
> > -}
> > -
> > -
> > -static uint32_t hsw_compute_wm_pipe(struct drm_device *dev,
> > -                                   const struct hsw_pipe_wm_parameters *params)
> > -{
> > -       struct drm_i915_private *dev_priv = dev->dev_private;
> > -       struct intel_wm_config config = {
> > -               .num_pipes_active = 1,
> > -               .sprites_enabled = params->spr.enabled,
> > -               .sprites_scaled = params->spr.scaled,
> > -       };
> > -       struct hsw_wm_maximums max;
> > -       struct intel_wm_level res;
> > -
> > -       if (!params->active)
> > -               return 0;
> > -
> > -       ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> > -
> > -       ilk_compute_wm_level(dev_priv, 0, params, &res);
> > -
> > -       ilk_check_wm(0, &max, &res);
> > -
> > -       return (res.pri_val << WM0_PIPE_PLANE_SHIFT) |
> > -              (res.spr_val << WM0_PIPE_SPRITE_SHIFT) |
> > -              res.cur_val;
> > -}
> > -
> >  static uint32_t
> >  hsw_compute_linetime_wm(struct drm_device *dev, struct drm_crtc *crtc)
> >  {
> > @@ -2670,44 +2623,121 @@ static void hsw_compute_wm_parameters(struct drm_device *dev,
> >                 *lp_max_5_6 = *lp_max_1_2;
> >  }
> >
> > +/* Compute new watermarks for the pipe */
> > +static bool intel_compute_pipe_wm(struct drm_crtc *crtc,
> > +                                 const struct hsw_pipe_wm_parameters *params,
> > +                                 struct intel_pipe_wm *pipe_wm)
> > +{
> > +       struct drm_device *dev = crtc->dev;
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +       int level, max_level = ilk_wm_max_level(dev);
> > +       struct intel_wm_config config = {
> > +               .num_pipes_active = 1,
> > +               .sprites_enabled = params->spr.enabled,
> > +               .sprites_scaled = params->spr.scaled,
> > +       };
> > +       struct hsw_wm_maximums max;
> > +
> > +       memset(pipe_wm, 0, sizeof(*pipe_wm));
> > +
> > +       ilk_wm_max(dev, 0, &config, INTEL_DDB_PART_1_2, &max);
> 
> Can you please add a nice comment explaining why we're using
> INTEL_DDB_PART_1_2 and ignoring the 5_6 partitioning model? I know the
> old code doesn't have it, but I had to look at the code again to try
> to understand it.

Can do.

> 
> 
> > +
> > +       for (level = 0; level <= max_level; level++)
> > +               ilk_compute_wm_level(dev_priv, level, params,
> > +                                    &pipe_wm->wm[level]);
> > +
> > +       pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
> > +
> > +       /* At least LP0 must be valid */
> > +       return ilk_check_wm(0, &max, &pipe_wm->wm[0]);
> 
> I think functions like "ilk_check_wm" and "ilk_wm_max" could be
> renamed to longer names that really explain what they do. Can you
> please do this in a separate patch? Maybe ilk_check_wm could be
> ilk_can_enable_wm or something similar,

I don't like that name very much. Not that I like ilk_check_wm() either.
Neither conveys the fact that we actually modify the structure. Maybe
ilk_validate_wm_level() or something?

> and maybe ilk_wm_max could be
> ilk_compute_wm_max_values or ilk_compute_wm_maximums or something else
> (the word "max" is used for both "max level" and "maximum values per
> level").

ilk_compute_wm_maximums() sounds reasonably clear to me. I'll make that
change.

> 
> 
> > +}
> > +
> > +/*
> > + * Merge the watermarks from all active pipes for a specific level.
> > + */
> > +static void ilk_merge_wm_level(struct drm_device *dev,
> > +                              int level,
> > +                              struct intel_wm_level *ret_wm)
> > +{
> > +       const struct intel_crtc *intel_crtc;
> > +
> 
> Don't we need to memset() ret_wm to zero here? Even if it's not needed
> now because of some other previous memset, I fear future code changes
> may introduce bugs due to non-zero values reaching this point.

I don't much like sprinkling memsets() around. Makes it look like
no-one knows when they should call the function. Also you might memset()
something by accident that you weren't supposed to, and possibly makes
the code slower. So zero initialization is nicer IMO.

> 
> 
> > +       list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
> > +               const struct intel_wm_level *wm =
> > +                       &intel_crtc->wm.active.wm[level];
> > +
> > +               if (!wm->enable)
> > +                       return;
> > +
> > +               ret_wm->pri_val = max(ret_wm->pri_val, wm->pri_val);
> > +               ret_wm->spr_val = max(ret_wm->spr_val, wm->spr_val);
> > +               ret_wm->cur_val = max(ret_wm->cur_val, wm->cur_val);
> > +               ret_wm->fbc_val = max(ret_wm->fbc_val, wm->fbc_val);
> > +       }
> > +
> > +       ret_wm->enable = true;
> 
> Maybe it would be nice to add a comment to explain why we set this to
> true, and who would set this to untrue in case it can't be true.

I was almost considering killing 'enable' from intel_wm_level. It should
really be called 'potentially_valid' or something. But I didn't really
check whether killing it completely would cause a lot of needless work to
be done.

> > +}
> > +
> > +/*
> > + * Merge all low power watermarks for all active pipes.
> > + */
> > +static void ilk_wm_merge(struct drm_device *dev,
> > +                        const struct hsw_wm_maximums *max,
> > +                        struct intel_pipe_wm *merged)
> > +{
> > +       int level, max_level = ilk_wm_max_level(dev);
> > +
> > +       merged->fbc_wm_enabled = true;
> > +
> > +       /* merge each WM1+ level */
> > +       for (level = 1; level <= max_level; level++) {
> > +               struct intel_wm_level *wm = &merged->wm[level];
> > +
> > +               ilk_merge_wm_level(dev, level, wm);
> > +
> > +               if (!ilk_check_wm(level, max, wm))
> > +                       break;
> > +
> > +               /*
> > +                * The spec says it is preferred to disable
> > +                * FBC WMs instead of disabling a WM level.
> > +                */
> > +               if (wm->fbc_val > max->fbc) {
> > +                       merged->fbc_wm_enabled = false;
> > +                       wm->fbc_val = 0;
> > +               }
> > +       }
> > +}
> > +
> >  static void hsw_compute_wm_results(struct drm_device *dev,
> >                                    const struct hsw_pipe_wm_parameters *params,
> >                                    const struct hsw_wm_maximums *lp_maximums,
> >                                    struct hsw_wm_values *results)
> >  {
> > -       struct drm_i915_private *dev_priv = dev->dev_private;
> > -       struct drm_crtc *crtc;
> > -       struct intel_wm_level lp_results[4] = {};
> > -       enum pipe pipe;
> > -       int level, max_level, wm_lp;
> > +       struct intel_crtc *intel_crtc;
> > +       int level, wm_lp;
> > +       struct intel_pipe_wm lp_wm = {};
> 
> Took me a while to realize that intel_pipe_wm isn't used only as the
> WM values for a single pipe (as the name suggests), but also for the
> merged WMs. Maybe we should rename the struct. And here maybe rename
> from lp_wm to merged_wms?

I think I originally called it 'merged' or something in most places,
but changed it to lp_wm to avoid renaming stuff too much that was
already present in the code.

I'm not sure 'merged' is the best name either. The spec talks about
consolidating watermarks from multiple pipes. Maybe we should use the
same terminology? But 'consolidated' is bit too long for my taste.

> 
> 
> >
> > -       for (level = 1; level <= 4; level++)
> > -               if (!hsw_compute_lp_wm(dev_priv, level,
> > -                                      lp_maximums, params,
> > -                                      &lp_results[level - 1]))
> > -                       break;
> > -       max_level = level - 1;
> > +       list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
> > +               intel_compute_pipe_wm(&intel_crtc->base,
> > +                                     &params[intel_crtc->pipe],
> > +                                     &intel_crtc->wm.active);
> > +
> > +       ilk_wm_merge(dev, lp_maximums, &lp_wm);
> >
> >         memset(results, 0, sizeof(*results));
> >
> > -       /* The spec says it is preferred to disable FBC WMs instead of disabling
> > -        * a WM level. */
> > -       results->enable_fbc_wm = true;
> > -       for (level = 1; level <= max_level; level++) {
> > -               if (lp_results[level - 1].fbc_val > lp_maximums->fbc) {
> > -                       results->enable_fbc_wm = false;
> > -                       lp_results[level - 1].fbc_val = 0;
> > -               }
> > -       }
> > +       results->enable_fbc_wm = lp_wm.fbc_wm_enabled;
> >
> > +       /* LP1+ register values */
> >         for (wm_lp = 1; wm_lp <= 3; wm_lp++) {
> >                 const struct intel_wm_level *r;
> >
> > -               level = (max_level == 4 && wm_lp > 1) ? wm_lp + 1 : wm_lp;
> > -               if (level > max_level)
> > +               level = wm_lp + (wm_lp >= 2 && lp_wm.wm[4].enable);
> 
> I find the older version easier to understand since it matches the
> spec and doesn't rely on adding a boolean value.

What's wrong with adding boolean values?

I could change it to 

level = wm_lp + (wm_lp >= 2 && lp_wm.wm[4].enable) ? 1 : 0;

but I think that just looks a bit silly.


> Also maybe we should
> add a comment explaining that we do this since on HSW we have 4 levels
> but only 3 slots (yes, I know, I should have added this comment when I
> wrote the original code).

I'm adding a comment in patch 12/19 when I refactor the computation
to a separate function.

> 
> 
> > +
> > +               r = &lp_wm.wm[level];
> > +               if (!r->enable)
> >                         break;
> >
> > -               r = &lp_results[level - 1];
> >                 results->wm_lp[wm_lp - 1] = HSW_WM_LP_VAL(level * 2,
> >                                                           r->fbc_val,
> >                                                           r->pri_val,
> > @@ -2715,13 +2745,21 @@ static void hsw_compute_wm_results(struct drm_device *dev,
> >                 results->wm_lp_spr[wm_lp - 1] = r->spr_val;
> >         }
> >
> > -       for_each_pipe(pipe)
> > -               results->wm_pipe[pipe] = hsw_compute_wm_pipe(dev,
> > -                                                            &params[pipe]);
> > +       /* LP0 register values */
> > +       list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) {
> > +               enum pipe pipe = intel_crtc->pipe;
> > +               const struct intel_wm_level *r =
> > +                       &intel_crtc->wm.active.wm[0];
> >
> > -       for_each_pipe(pipe) {
> > -               crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > -               results->wm_linetime[pipe] = hsw_compute_linetime_wm(dev, crtc);
> > +               if (!r->enable)
> > +                       continue;
> 
> Does this mean "pipe not active"? Maybe add a comment explaining. This
> is confusing since we're talking about LP0 levels and they shouldn't
> really be disabled.

Yeah. That should really be a WARN_ON() or something to indiate that it
should never happen.

> 
> 
> Our WM code is getting quite complex, it's not very easy to follow.
> The comments/renames I asked are for stuff that took me a while to
> understand. I also wouldn't complain if "v2" comes as a series of
> smaller patches.
> 
> 
> I did test this patch on my Haswell machine and the values still seem
> to be correct.
> 
> 
> > +
> > +               results->wm_linetime[pipe] = intel_crtc->wm.active.linetime;
> > +
> > +               results->wm_pipe[pipe] =
> > +                       (r->pri_val << WM0_PIPE_PLANE_SHIFT) |
> > +                       (r->spr_val << WM0_PIPE_SPRITE_SHIFT) |
> > +                       r->cur_val;
> >         }
> >  }
> >
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 06/19] drm/i915: Add intel_pipe_wm and prepare for watermark pre-compute

[Daniel Vetter]

On Mon, Sep 16, 2013 at 07:29:14PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 16, 2013 at 12:07:32PM -0300, Paulo Zanoni wrote:
> > 2013/8/30  <ville.syrjala@linux.intel.com>:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Introduce a new struct intel_pipe_wm which contains all the
> > > watermarks for a single pipe. Use it to unify the LP0 and LP1+
> > > watermark computations so that we can just iterate through the
> > > watermark levels neatly and call ilk_compute_wm_level() for each.
> > >
> > > Also add another tool ilk_wm_merge() that merges the LP1+ watermarks
> > > from all pipes. For that, embed one intel_pipe_wm inside intel_crtc that
> > > contains the currently valid watermarks for each pipe.
> > >
> > > This is mainly preparatory work for pre-computing the watermarks for
> > > each pipe and merging them at a later time. For now the merging still
> > > happens immediately.
> > 
> > >From the commit message, it seems this patch could be split in 4 or 5
> > sub-patches.
> 
> I think if I split it up any more then then the indiviual patches might
> stop making sense. Then you're going to be asking "why are you doing this
> change here?" and I have to answer "see patch n+2" or something.

Yeah, I think the patch is ok. It mixes in a few steps, but when
introducing new concepts I think smashing a few really simple things into
the first patch (if the patch is still pretty small like this one here)
helps the reader to understand how it fits together. Then with that
baseline more complex stuff can be converted over ...

Just my 2 uninformed cents ;-)

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