[PATCH 00/15] drm/i915: VLV/CHV atomic wm prep work

[PATCH 00/15] drm/i915: VLV/CHV atomic wm prep work

[ville.syrjala]

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

A decent pile of prep work split off from my VLV/CHV atomic watermark
work. Mostly VLV/CHV specific stuff, but there are a few small things
in there that touch other platforms as well.

Entire series available here:
git://github.com/vsyrjala/linux.git vlv_atomic_wm_prep

Ville Syrjälä (15):
  drm/i915: Drop the nop intel_update_watermarks() call from
    haswell_crtc_enable()
  drm/i915: Use the ilk_disable_lp_wm() return value
  drm/i915: Fix the level 0 max_wm hack on VLV/CHV
  drm/i915: Clean up VLV/CHV maxfifo watermark setup
  drm/i915: Remove duplicated wm setup for vlv and chv
  drm/i915: Organize vlv/chv watermarks by plane_id
  drm/i915: Introduce vlv_invert_wm_value()
  drm/i915: Pass around dev_priv in vlv wm functions
  drm/i915: Protect cxsr state with wm_mutex
  drm/i915: Skip vblank wait if cxsr was already off
  drm/i915: Protect DSPARB registers with a spinlock
  drm/i915: Zero out HOWM registers before writing new WM/HOWM register
    values
  drm/i915: Write all DDL registers in one go
  drm/i915: Clean up vlv_program_watermarks()
  drm/i915: Pass crtc state to vlv_compute_wm_level()

 drivers/gpu/drm/i915/i915_drv.c      |   1 +
 drivers/gpu/drm/i915/i915_drv.h      |  21 +-
 drivers/gpu/drm/i915/intel_display.c |  21 +-
 drivers/gpu/drm/i915/intel_pm.c      | 394 ++++++++++++++++++-----------------
 4 files changed, 218 insertions(+), 219 deletions(-)

-- 
2.7.4

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

[PATCH 01/15] drm/i915: Drop the nop intel_update_watermarks() call from haswell_crtc_enable()

[ville.syrjala]

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

HSW+ all use the .initial_watermarks() hook, so there's no point in
calling intel_update_watermarks() from HSW+ specific code. We'll still
hang on to the .initial_watermarks NULL check since theoretically if the
memory latencies are not populated we would not populate the function
pointer either.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 77c4ff9efbe3..5fef72b8ca61 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5465,10 +5465,7 @@ static void haswell_crtc_enable(struct intel_crtc_state *pipe_config,
 		intel_ddi_enable_transcoder_func(crtc);
 
 	if (dev_priv->display.initial_watermarks != NULL)
-		dev_priv->display.initial_watermarks(old_intel_state,
-						     pipe_config);
-	else
-		intel_update_watermarks(intel_crtc);
+		dev_priv->display.initial_watermarks(old_intel_state, pipe_config);
 
 	/* XXX: Do the pipe assertions at the right place for BXT DSI. */
 	if (!transcoder_is_dsi(cpu_transcoder))
-- 
2.7.4

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

[PATCH 02/15] drm/i915: Use the ilk_disable_lp_wm() return value

[ville.syrjala]

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

ilk_disable_lp_wm() will tell us whether the LP1+ watermarks were
disabled or not, and hence whether we need to for the vblank wait or
not. Let's use that information to eliminate some useless vblank
waits.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5fef72b8ca61..ce9e7f2f395e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5114,10 +5114,8 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 	 *
 	 * WaCxSRDisabledForSpriteScaling:ivb
 	 */
-	if (pipe_config->disable_lp_wm) {
-		ilk_disable_lp_wm(dev);
+	if (pipe_config->disable_lp_wm && ilk_disable_lp_wm(dev))
 		intel_wait_for_vblank(dev_priv, crtc->pipe);
-	}
 
 	/*
 	 * If we're doing a modeset, we're done.  No need to do any pre-vblank
-- 
2.7.4

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

[PATCH 03/15] drm/i915: Fix the level 0 max_wm hack on VLV/CHV

[ville.syrjala]

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

The watermark should never exceed the FIFO size, so we need to
check against the current FIFO size instead of the theoretical
maximum when we clamp the level 0 watermark.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 29b6653661cd..8aa11e1b84ed 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1132,13 +1132,13 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 		/* normal watermarks */
 		for (level = 0; level < wm_state->num_levels; level++) {
 			int wm = vlv_compute_wm_level(plane, crtc, state, level);
-			int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
+			int max_wm = plane->wm.fifo_size;
 
 			/* hack */
 			if (WARN_ON(level == 0 && wm > max_wm))
 				wm = max_wm;
 
-			if (wm > plane->wm.fifo_size)
+			if (wm > max_wm)
 				break;
 
 			switch (plane->base.type) {
-- 
2.7.4

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

[PATCH 04/15] drm/i915: Clean up VLV/CHV maxfifo watermark setup

[ville.syrjala]

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

Let's compute the maxfifo watermarks using max() instead of min().
Can't even recall why I did it the other way originally. Anyways
using max() avoids having to initialize the watermarks to the max
value first.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8aa11e1b84ed..a31f55a7af38 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1100,7 +1100,6 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct vlv_wm_state *wm_state = &crtc->wm_state;
 	struct intel_plane *plane;
-	int sr_fifo_size = INTEL_INFO(dev_priv)->num_pipes * 512 - 1;
 	int level;
 
 	memset(wm_state, 0, sizeof(*wm_state));
@@ -1115,13 +1114,6 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 	if (wm_state->num_active_planes != 1)
 		wm_state->cxsr = false;
 
-	if (wm_state->cxsr) {
-		for (level = 0; level < wm_state->num_levels; level++) {
-			wm_state->sr[level].plane = sr_fifo_size;
-			wm_state->sr[level].cursor = 63;
-		}
-	}
-
 	for_each_intel_plane_on_crtc(dev, crtc, plane) {
 		struct intel_plane_state *state =
 			to_intel_plane_state(plane->base.state);
@@ -1172,14 +1164,14 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 		case DRM_PLANE_TYPE_PRIMARY:
 			for (level = 0; level < wm_state->num_levels; level++)
 				wm_state->sr[level].plane =
-					min(wm_state->sr[level].plane,
+					max(wm_state->sr[level].plane,
 					    wm_state->wm[level].primary);
 			break;
 		case DRM_PLANE_TYPE_OVERLAY:
 			sprite = vlv_sprite_id(plane->id);
 			for (level = 0; level < wm_state->num_levels; level++)
 				wm_state->sr[level].plane =
-					min(wm_state->sr[level].plane,
+					max(wm_state->sr[level].plane,
 					    wm_state->wm[level].sprite[sprite]);
 			break;
 		}
-- 
2.7.4

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

[PATCH 05/15] drm/i915: Remove duplicated wm setup for vlv and chv

[ville.syrjala]

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

The code for vlv and chv wm latency/function pointer setup is
identical. Drop one of the copies.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a31f55a7af38..ae664eb37d5c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7683,10 +7683,7 @@ void intel_init_pm(struct drm_i915_private *dev_priv)
 			DRM_DEBUG_KMS("Failed to read display plane latency. "
 				      "Disable CxSR\n");
 		}
-	} else if (IS_CHERRYVIEW(dev_priv)) {
-		vlv_setup_wm_latency(dev_priv);
-		dev_priv->display.update_wm = vlv_update_wm;
-	} else if (IS_VALLEYVIEW(dev_priv)) {
+	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		vlv_setup_wm_latency(dev_priv);
 		dev_priv->display.update_wm = vlv_update_wm;
 	} else if (IS_PINEVIEW(dev_priv)) {
-- 
2.7.4

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

[PATCH 06/15] drm/i915: Organize vlv/chv watermarks by plane_id

[ville.syrjala]

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

Store the vlv/chv watermark values in straight up arrays indexed by
enum plane_id. Avoids a lot of useless checks for the plane type when
we don't have to think which structure member we need to access.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c5349aaaf874..be89267474a1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1657,24 +1657,22 @@ struct ilk_wm_values {
 };
 
 struct vlv_pipe_wm {
-	uint16_t primary;
-	uint16_t sprite[2];
-	uint8_t cursor;
+	uint16_t plane[I915_MAX_PLANES];
 };
 
 struct vlv_sr_wm {
 	uint16_t plane;
-	uint8_t cursor;
+	uint16_t cursor;
+};
+
+struct vlv_wm_ddl_values {
+	uint8_t plane[I915_MAX_PLANES];
 };
 
 struct vlv_wm_values {
 	struct vlv_pipe_wm pipe[3];
 	struct vlv_sr_wm sr;
-	struct {
-		uint8_t cursor;
-		uint8_t sprite[2];
-		uint8_t primary;
-	} ddl[3];
+	struct vlv_wm_ddl_values ddl[3];
 	uint8_t level;
 	bool cxsr;
 };
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ae664eb37d5c..0ca8f0941d00 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -849,56 +849,56 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
 	enum pipe pipe = crtc->pipe;
 
 	I915_WRITE(VLV_DDL(pipe),
-		   (wm->ddl[pipe].cursor << DDL_CURSOR_SHIFT) |
-		   (wm->ddl[pipe].sprite[1] << DDL_SPRITE_SHIFT(1)) |
-		   (wm->ddl[pipe].sprite[0] << DDL_SPRITE_SHIFT(0)) |
-		   (wm->ddl[pipe].primary << DDL_PLANE_SHIFT));
+		   (wm->ddl[pipe].plane[PLANE_CURSOR] << DDL_CURSOR_SHIFT) |
+		   (wm->ddl[pipe].plane[PLANE_SPRITE1] << DDL_SPRITE_SHIFT(1)) |
+		   (wm->ddl[pipe].plane[PLANE_SPRITE0] << DDL_SPRITE_SHIFT(0)) |
+		   (wm->ddl[pipe].plane[PLANE_PRIMARY] << DDL_PLANE_SHIFT));
 
 	I915_WRITE(DSPFW1,
 		   FW_WM(wm->sr.plane, SR) |
-		   FW_WM(wm->pipe[PIPE_B].cursor, CURSORB) |
-		   FW_WM_VLV(wm->pipe[PIPE_B].primary, PLANEB) |
-		   FW_WM_VLV(wm->pipe[PIPE_A].primary, PLANEA));
+		   FW_WM(wm->pipe[PIPE_B].plane[PLANE_CURSOR], CURSORB) |
+		   FW_WM_VLV(wm->pipe[PIPE_B].plane[PLANE_PRIMARY], PLANEB) |
+		   FW_WM_VLV(wm->pipe[PIPE_A].plane[PLANE_PRIMARY], PLANEA));
 	I915_WRITE(DSPFW2,
-		   FW_WM_VLV(wm->pipe[PIPE_A].sprite[1], SPRITEB) |
-		   FW_WM(wm->pipe[PIPE_A].cursor, CURSORA) |
-		   FW_WM_VLV(wm->pipe[PIPE_A].sprite[0], SPRITEA));
+		   FW_WM_VLV(wm->pipe[PIPE_A].plane[PLANE_SPRITE1], SPRITEB) |
+		   FW_WM(wm->pipe[PIPE_A].plane[PLANE_CURSOR], CURSORA) |
+		   FW_WM_VLV(wm->pipe[PIPE_A].plane[PLANE_SPRITE0], SPRITEA));
 	I915_WRITE(DSPFW3,
 		   FW_WM(wm->sr.cursor, CURSOR_SR));
 
 	if (IS_CHERRYVIEW(dev_priv)) {
 		I915_WRITE(DSPFW7_CHV,
-			   FW_WM_VLV(wm->pipe[PIPE_B].sprite[1], SPRITED) |
-			   FW_WM_VLV(wm->pipe[PIPE_B].sprite[0], SPRITEC));
+			   FW_WM_VLV(wm->pipe[PIPE_B].plane[PLANE_SPRITE1], SPRITED) |
+			   FW_WM_VLV(wm->pipe[PIPE_B].plane[PLANE_SPRITE0], SPRITEC));
 		I915_WRITE(DSPFW8_CHV,
-			   FW_WM_VLV(wm->pipe[PIPE_C].sprite[1], SPRITEF) |
-			   FW_WM_VLV(wm->pipe[PIPE_C].sprite[0], SPRITEE));
+			   FW_WM_VLV(wm->pipe[PIPE_C].plane[PLANE_SPRITE1], SPRITEF) |
+			   FW_WM_VLV(wm->pipe[PIPE_C].plane[PLANE_SPRITE0], SPRITEE));
 		I915_WRITE(DSPFW9_CHV,
-			   FW_WM_VLV(wm->pipe[PIPE_C].primary, PLANEC) |
-			   FW_WM(wm->pipe[PIPE_C].cursor, CURSORC));
+			   FW_WM_VLV(wm->pipe[PIPE_C].plane[PLANE_PRIMARY], PLANEC) |
+			   FW_WM(wm->pipe[PIPE_C].plane[PLANE_CURSOR], CURSORC));
 		I915_WRITE(DSPHOWM,
 			   FW_WM(wm->sr.plane >> 9, SR_HI) |
-			   FW_WM(wm->pipe[PIPE_C].sprite[1] >> 8, SPRITEF_HI) |
-			   FW_WM(wm->pipe[PIPE_C].sprite[0] >> 8, SPRITEE_HI) |
-			   FW_WM(wm->pipe[PIPE_C].primary >> 8, PLANEC_HI) |
-			   FW_WM(wm->pipe[PIPE_B].sprite[1] >> 8, SPRITED_HI) |
-			   FW_WM(wm->pipe[PIPE_B].sprite[0] >> 8, SPRITEC_HI) |
-			   FW_WM(wm->pipe[PIPE_B].primary >> 8, PLANEB_HI) |
-			   FW_WM(wm->pipe[PIPE_A].sprite[1] >> 8, SPRITEB_HI) |
-			   FW_WM(wm->pipe[PIPE_A].sprite[0] >> 8, SPRITEA_HI) |
-			   FW_WM(wm->pipe[PIPE_A].primary >> 8, PLANEA_HI));
+			   FW_WM(wm->pipe[PIPE_C].plane[PLANE_SPRITE1] >> 8, SPRITEF_HI) |
+			   FW_WM(wm->pipe[PIPE_C].plane[PLANE_SPRITE0] >> 8, SPRITEE_HI) |
+			   FW_WM(wm->pipe[PIPE_C].plane[PLANE_PRIMARY] >> 8, PLANEC_HI) |
+			   FW_WM(wm->pipe[PIPE_B].plane[PLANE_SPRITE1] >> 8, SPRITED_HI) |
+			   FW_WM(wm->pipe[PIPE_B].plane[PLANE_SPRITE0] >> 8, SPRITEC_HI) |
+			   FW_WM(wm->pipe[PIPE_B].plane[PLANE_PRIMARY] >> 8, PLANEB_HI) |
+			   FW_WM(wm->pipe[PIPE_A].plane[PLANE_SPRITE1] >> 8, SPRITEB_HI) |
+			   FW_WM(wm->pipe[PIPE_A].plane[PLANE_SPRITE0] >> 8, SPRITEA_HI) |
+			   FW_WM(wm->pipe[PIPE_A].plane[PLANE_PRIMARY] >> 8, PLANEA_HI));
 	} else {
 		I915_WRITE(DSPFW7,
-			   FW_WM_VLV(wm->pipe[PIPE_B].sprite[1], SPRITED) |
-			   FW_WM_VLV(wm->pipe[PIPE_B].sprite[0], SPRITEC));
+			   FW_WM_VLV(wm->pipe[PIPE_B].plane[PLANE_SPRITE1], SPRITED) |
+			   FW_WM_VLV(wm->pipe[PIPE_B].plane[PLANE_SPRITE0], SPRITEC));
 		I915_WRITE(DSPHOWM,
 			   FW_WM(wm->sr.plane >> 9, SR_HI) |
-			   FW_WM(wm->pipe[PIPE_B].sprite[1] >> 8, SPRITED_HI) |
-			   FW_WM(wm->pipe[PIPE_B].sprite[0] >> 8, SPRITEC_HI) |
-			   FW_WM(wm->pipe[PIPE_B].primary >> 8, PLANEB_HI) |
-			   FW_WM(wm->pipe[PIPE_A].sprite[1] >> 8, SPRITEB_HI) |
-			   FW_WM(wm->pipe[PIPE_A].sprite[0] >> 8, SPRITEA_HI) |
-			   FW_WM(wm->pipe[PIPE_A].primary >> 8, PLANEA_HI));
+			   FW_WM(wm->pipe[PIPE_B].plane[PLANE_SPRITE1] >> 8, SPRITED_HI) |
+			   FW_WM(wm->pipe[PIPE_B].plane[PLANE_SPRITE0] >> 8, SPRITEC_HI) |
+			   FW_WM(wm->pipe[PIPE_B].plane[PLANE_PRIMARY] >> 8, PLANEB_HI) |
+			   FW_WM(wm->pipe[PIPE_A].plane[PLANE_SPRITE1] >> 8, SPRITEB_HI) |
+			   FW_WM(wm->pipe[PIPE_A].plane[PLANE_SPRITE0] >> 8, SPRITEA_HI) |
+			   FW_WM(wm->pipe[PIPE_A].plane[PLANE_PRIMARY] >> 8, PLANEA_HI));
 	}
 
 	/* zero (unused) WM1 watermarks */
@@ -1053,12 +1053,6 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
 	WARN_ON(fifo_left != 0);
 }
 
-/* FIXME kill me */
-static inline int vlv_sprite_id(enum plane_id plane_id)
-{
-	return plane_id - PLANE_SPRITE0;
-}
-
 static void vlv_invert_wms(struct intel_crtc *crtc)
 {
 	struct vlv_wm_state *wm_state = &crtc->wm_state;
@@ -1074,22 +1068,8 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
 		wm_state->sr[level].cursor = 63 - wm_state->sr[level].cursor;
 
 		for_each_intel_plane_on_crtc(dev, crtc, plane) {
-			switch (plane->base.type) {
-				int sprite;
-			case DRM_PLANE_TYPE_CURSOR:
-				wm_state->wm[level].cursor = plane->wm.fifo_size -
-					wm_state->wm[level].cursor;
-				break;
-			case DRM_PLANE_TYPE_PRIMARY:
-				wm_state->wm[level].primary = plane->wm.fifo_size -
-					wm_state->wm[level].primary;
-				break;
-			case DRM_PLANE_TYPE_OVERLAY:
-				sprite = vlv_sprite_id(plane->id);
-				wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
-					wm_state->wm[level].sprite[sprite];
-				break;
-			}
+			wm_state->wm[level].plane[plane->id] = plane->wm.fifo_size -
+				wm_state->wm[level].plane[plane->id];
 		}
 	}
 }
@@ -1117,6 +1097,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 	for_each_intel_plane_on_crtc(dev, crtc, plane) {
 		struct intel_plane_state *state =
 			to_intel_plane_state(plane->base.state);
+		int level;
 
 		if (!state->base.visible)
 			continue;
@@ -1133,19 +1114,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 			if (wm > max_wm)
 				break;
 
-			switch (plane->base.type) {
-				int sprite;
-			case DRM_PLANE_TYPE_CURSOR:
-				wm_state->wm[level].cursor = wm;
-				break;
-			case DRM_PLANE_TYPE_PRIMARY:
-				wm_state->wm[level].primary = wm;
-				break;
-			case DRM_PLANE_TYPE_OVERLAY:
-				sprite = vlv_sprite_id(plane->id);
-				wm_state->wm[level].sprite[sprite] = wm;
-				break;
-			}
+			wm_state->wm[level].plane[plane->id] = wm;
 		}
 
 		wm_state->num_levels = level;
@@ -1154,26 +1123,15 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 			continue;
 
 		/* maxfifo watermarks */
-		switch (plane->base.type) {
-			int sprite, level;
-		case DRM_PLANE_TYPE_CURSOR:
+		if (plane->id == PLANE_CURSOR) {
 			for (level = 0; level < wm_state->num_levels; level++)
 				wm_state->sr[level].cursor =
-					wm_state->wm[level].cursor;
-			break;
-		case DRM_PLANE_TYPE_PRIMARY:
-			for (level = 0; level < wm_state->num_levels; level++)
-				wm_state->sr[level].plane =
-					max(wm_state->sr[level].plane,
-					    wm_state->wm[level].primary);
-			break;
-		case DRM_PLANE_TYPE_OVERLAY:
-			sprite = vlv_sprite_id(plane->id);
+					wm_state->wm[level].plane[PLANE_CURSOR];
+		} else {
 			for (level = 0; level < wm_state->num_levels; level++)
 				wm_state->sr[level].plane =
 					max(wm_state->sr[level].plane,
-					    wm_state->wm[level].sprite[sprite]);
-			break;
+					    wm_state->wm[level].plane[plane->id]);
 		}
 	}
 
@@ -1321,10 +1279,10 @@ static void vlv_merge_wm(struct drm_device *dev,
 		if (wm->cxsr)
 			wm->sr = wm_state->sr[wm->level];
 
-		wm->ddl[pipe].primary = DDL_PRECISION_HIGH | 2;
-		wm->ddl[pipe].sprite[0] = DDL_PRECISION_HIGH | 2;
-		wm->ddl[pipe].sprite[1] = DDL_PRECISION_HIGH | 2;
-		wm->ddl[pipe].cursor = DDL_PRECISION_HIGH | 2;
+		wm->ddl[pipe].plane[PLANE_PRIMARY] = DDL_PRECISION_HIGH | 2;
+		wm->ddl[pipe].plane[PLANE_SPRITE0] = DDL_PRECISION_HIGH | 2;
+		wm->ddl[pipe].plane[PLANE_SPRITE1] = DDL_PRECISION_HIGH | 2;
+		wm->ddl[pipe].plane[PLANE_CURSOR] = DDL_PRECISION_HIGH | 2;
 	}
 }
 
@@ -1362,8 +1320,8 @@ static void vlv_update_wm(struct intel_crtc *crtc)
 
 	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
 		      "sprite0=%d, sprite1=%d, SR: plane=%d, cursor=%d level=%d cxsr=%d\n",
-		      pipe_name(pipe), wm.pipe[pipe].primary, wm.pipe[pipe].cursor,
-		      wm.pipe[pipe].sprite[0], wm.pipe[pipe].sprite[1],
+		      pipe_name(pipe), wm.pipe[pipe].plane[PLANE_PRIMARY], wm.pipe[pipe].plane[PLANE_CURSOR],
+		      wm.pipe[pipe].plane[PLANE_SPRITE0], wm.pipe[pipe].plane[PLANE_SPRITE1],
 		      wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
 
 	if (wm.cxsr && !dev_priv->wm.vlv.cxsr)
@@ -4437,67 +4395,67 @@ static void vlv_read_wm_values(struct drm_i915_private *dev_priv,
 	for_each_pipe(dev_priv, pipe) {
 		tmp = I915_READ(VLV_DDL(pipe));
 
-		wm->ddl[pipe].primary =
+		wm->ddl[pipe].plane[PLANE_PRIMARY] =
 			(tmp >> DDL_PLANE_SHIFT) & (DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK);
-		wm->ddl[pipe].cursor =
+		wm->ddl[pipe].plane[PLANE_CURSOR] =
 			(tmp >> DDL_CURSOR_SHIFT) & (DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK);
-		wm->ddl[pipe].sprite[0] =
+		wm->ddl[pipe].plane[PLANE_SPRITE0] =
 			(tmp >> DDL_SPRITE_SHIFT(0)) & (DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK);
-		wm->ddl[pipe].sprite[1] =
+		wm->ddl[pipe].plane[PLANE_SPRITE1] =
 			(tmp >> DDL_SPRITE_SHIFT(1)) & (DDL_PRECISION_HIGH | DRAIN_LATENCY_MASK);
 	}
 
 	tmp = I915_READ(DSPFW1);
 	wm->sr.plane = _FW_WM(tmp, SR);
-	wm->pipe[PIPE_B].cursor = _FW_WM(tmp, CURSORB);
-	wm->pipe[PIPE_B].primary = _FW_WM_VLV(tmp, PLANEB);
-	wm->pipe[PIPE_A].primary = _FW_WM_VLV(tmp, PLANEA);
+	wm->pipe[PIPE_B].plane[PLANE_CURSOR] = _FW_WM(tmp, CURSORB);
+	wm->pipe[PIPE_B].plane[PLANE_PRIMARY] = _FW_WM_VLV(tmp, PLANEB);
+	wm->pipe[PIPE_A].plane[PLANE_PRIMARY] = _FW_WM_VLV(tmp, PLANEA);
 
 	tmp = I915_READ(DSPFW2);
-	wm->pipe[PIPE_A].sprite[1] = _FW_WM_VLV(tmp, SPRITEB);
-	wm->pipe[PIPE_A].cursor = _FW_WM(tmp, CURSORA);
-	wm->pipe[PIPE_A].sprite[0] = _FW_WM_VLV(tmp, SPRITEA);
+	wm->pipe[PIPE_A].plane[PLANE_SPRITE1] = _FW_WM_VLV(tmp, SPRITEB);
+	wm->pipe[PIPE_A].plane[PLANE_CURSOR] = _FW_WM(tmp, CURSORA);
+	wm->pipe[PIPE_A].plane[PLANE_SPRITE0] = _FW_WM_VLV(tmp, SPRITEA);
 
 	tmp = I915_READ(DSPFW3);
 	wm->sr.cursor = _FW_WM(tmp, CURSOR_SR);
 
 	if (IS_CHERRYVIEW(dev_priv)) {
 		tmp = I915_READ(DSPFW7_CHV);
-		wm->pipe[PIPE_B].sprite[1] = _FW_WM_VLV(tmp, SPRITED);
-		wm->pipe[PIPE_B].sprite[0] = _FW_WM_VLV(tmp, SPRITEC);
+		wm->pipe[PIPE_B].plane[PLANE_SPRITE1] = _FW_WM_VLV(tmp, SPRITED);
+		wm->pipe[PIPE_B].plane[PLANE_SPRITE0] = _FW_WM_VLV(tmp, SPRITEC);
 
 		tmp = I915_READ(DSPFW8_CHV);
-		wm->pipe[PIPE_C].sprite[1] = _FW_WM_VLV(tmp, SPRITEF);
-		wm->pipe[PIPE_C].sprite[0] = _FW_WM_VLV(tmp, SPRITEE);
+		wm->pipe[PIPE_C].plane[PLANE_SPRITE1] = _FW_WM_VLV(tmp, SPRITEF);
+		wm->pipe[PIPE_C].plane[PLANE_SPRITE0] = _FW_WM_VLV(tmp, SPRITEE);
 
 		tmp = I915_READ(DSPFW9_CHV);
-		wm->pipe[PIPE_C].primary = _FW_WM_VLV(tmp, PLANEC);
-		wm->pipe[PIPE_C].cursor = _FW_WM(tmp, CURSORC);
+		wm->pipe[PIPE_C].plane[PLANE_PRIMARY] = _FW_WM_VLV(tmp, PLANEC);
+		wm->pipe[PIPE_C].plane[PLANE_CURSOR] = _FW_WM(tmp, CURSORC);
 
 		tmp = I915_READ(DSPHOWM);
 		wm->sr.plane |= _FW_WM(tmp, SR_HI) << 9;
-		wm->pipe[PIPE_C].sprite[1] |= _FW_WM(tmp, SPRITEF_HI) << 8;
-		wm->pipe[PIPE_C].sprite[0] |= _FW_WM(tmp, SPRITEE_HI) << 8;
-		wm->pipe[PIPE_C].primary |= _FW_WM(tmp, PLANEC_HI) << 8;
-		wm->pipe[PIPE_B].sprite[1] |= _FW_WM(tmp, SPRITED_HI) << 8;
-		wm->pipe[PIPE_B].sprite[0] |= _FW_WM(tmp, SPRITEC_HI) << 8;
-		wm->pipe[PIPE_B].primary |= _FW_WM(tmp, PLANEB_HI) << 8;
-		wm->pipe[PIPE_A].sprite[1] |= _FW_WM(tmp, SPRITEB_HI) << 8;
-		wm->pipe[PIPE_A].sprite[0] |= _FW_WM(tmp, SPRITEA_HI) << 8;
-		wm->pipe[PIPE_A].primary |= _FW_WM(tmp, PLANEA_HI) << 8;
+		wm->pipe[PIPE_C].plane[PLANE_SPRITE1] |= _FW_WM(tmp, SPRITEF_HI) << 8;
+		wm->pipe[PIPE_C].plane[PLANE_SPRITE0] |= _FW_WM(tmp, SPRITEE_HI) << 8;
+		wm->pipe[PIPE_C].plane[PLANE_PRIMARY] |= _FW_WM(tmp, PLANEC_HI) << 8;
+		wm->pipe[PIPE_B].plane[PLANE_SPRITE1] |= _FW_WM(tmp, SPRITED_HI) << 8;
+		wm->pipe[PIPE_B].plane[PLANE_SPRITE0] |= _FW_WM(tmp, SPRITEC_HI) << 8;
+		wm->pipe[PIPE_B].plane[PLANE_PRIMARY] |= _FW_WM(tmp, PLANEB_HI) << 8;
+		wm->pipe[PIPE_A].plane[PLANE_SPRITE1] |= _FW_WM(tmp, SPRITEB_HI) << 8;
+		wm->pipe[PIPE_A].plane[PLANE_SPRITE0] |= _FW_WM(tmp, SPRITEA_HI) << 8;
+		wm->pipe[PIPE_A].plane[PLANE_PRIMARY] |= _FW_WM(tmp, PLANEA_HI) << 8;
 	} else {
 		tmp = I915_READ(DSPFW7);
-		wm->pipe[PIPE_B].sprite[1] = _FW_WM_VLV(tmp, SPRITED);
-		wm->pipe[PIPE_B].sprite[0] = _FW_WM_VLV(tmp, SPRITEC);
+		wm->pipe[PIPE_B].plane[PLANE_SPRITE1] = _FW_WM_VLV(tmp, SPRITED);
+		wm->pipe[PIPE_B].plane[PLANE_SPRITE0] = _FW_WM_VLV(tmp, SPRITEC);
 
 		tmp = I915_READ(DSPHOWM);
 		wm->sr.plane |= _FW_WM(tmp, SR_HI) << 9;
-		wm->pipe[PIPE_B].sprite[1] |= _FW_WM(tmp, SPRITED_HI) << 8;
-		wm->pipe[PIPE_B].sprite[0] |= _FW_WM(tmp, SPRITEC_HI) << 8;
-		wm->pipe[PIPE_B].primary |= _FW_WM(tmp, PLANEB_HI) << 8;
-		wm->pipe[PIPE_A].sprite[1] |= _FW_WM(tmp, SPRITEB_HI) << 8;
-		wm->pipe[PIPE_A].sprite[0] |= _FW_WM(tmp, SPRITEA_HI) << 8;
-		wm->pipe[PIPE_A].primary |= _FW_WM(tmp, PLANEA_HI) << 8;
+		wm->pipe[PIPE_B].plane[PLANE_SPRITE1] |= _FW_WM(tmp, SPRITED_HI) << 8;
+		wm->pipe[PIPE_B].plane[PLANE_SPRITE0] |= _FW_WM(tmp, SPRITEC_HI) << 8;
+		wm->pipe[PIPE_B].plane[PLANE_PRIMARY] |= _FW_WM(tmp, PLANEB_HI) << 8;
+		wm->pipe[PIPE_A].plane[PLANE_SPRITE1] |= _FW_WM(tmp, SPRITEB_HI) << 8;
+		wm->pipe[PIPE_A].plane[PLANE_SPRITE0] |= _FW_WM(tmp, SPRITEA_HI) << 8;
+		wm->pipe[PIPE_A].plane[PLANE_PRIMARY] |= _FW_WM(tmp, PLANEA_HI) << 8;
 	}
 }
 
@@ -4556,8 +4514,11 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
 
 	for_each_pipe(dev_priv, pipe)
 		DRM_DEBUG_KMS("Initial watermarks: pipe %c, plane=%d, cursor=%d, sprite0=%d, sprite1=%d\n",
-			      pipe_name(pipe), wm->pipe[pipe].primary, wm->pipe[pipe].cursor,
-			      wm->pipe[pipe].sprite[0], wm->pipe[pipe].sprite[1]);
+			      pipe_name(pipe),
+			      wm->pipe[pipe].plane[PLANE_PRIMARY],
+			      wm->pipe[pipe].plane[PLANE_CURSOR],
+			      wm->pipe[pipe].plane[PLANE_SPRITE0],
+			      wm->pipe[pipe].plane[PLANE_SPRITE1]);
 
 	DRM_DEBUG_KMS("Initial watermarks: SR plane=%d, SR cursor=%d level=%d cxsr=%d\n",
 		      wm->sr.plane, wm->sr.cursor, wm->level, wm->cxsr);
-- 
2.7.4

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

[PATCH 07/15] drm/i915: Introduce vlv_invert_wm_value()

[ville.syrjala]

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

Add a small helper to do invert the vlv/chv values. Less fragile
perhaps, and let's us clearly mark all overlarge wateramarks as
disabled (by just making them all USHRT_MAX).

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0ca8f0941d00..c955ee6341a1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1053,6 +1053,14 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
 	WARN_ON(fifo_left != 0);
 }
 
+static u16 vlv_invert_wm_value(u16 wm, u16 fifo_size)
+{
+	if (wm > fifo_size)
+		return USHRT_MAX;
+	else
+		return fifo_size - wm;
+}
+
 static void vlv_invert_wms(struct intel_crtc *crtc)
 {
 	struct vlv_wm_state *wm_state = &crtc->wm_state;
@@ -1064,12 +1072,17 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
 			INTEL_INFO(to_i915(dev))->num_pipes * 512 - 1;
 		struct intel_plane *plane;
 
-		wm_state->sr[level].plane = sr_fifo_size - wm_state->sr[level].plane;
-		wm_state->sr[level].cursor = 63 - wm_state->sr[level].cursor;
+		wm_state->sr[level].plane =
+			vlv_invert_wm_value(wm_state->sr[level].plane,
+					    sr_fifo_size);
+		wm_state->sr[level].cursor =
+			vlv_invert_wm_value(wm_state->sr[level].cursor,
+					    63);
 
 		for_each_intel_plane_on_crtc(dev, crtc, plane) {
-			wm_state->wm[level].plane[plane->id] = plane->wm.fifo_size -
-				wm_state->wm[level].plane[plane->id];
+			wm_state->wm[level].plane[plane->id] =
+				vlv_invert_wm_value(wm_state->wm[level].plane[plane->id],
+						    plane->wm.fifo_size);
 		}
 	}
 }
-- 
2.7.4

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

[PATCH 08/15] drm/i915: Pass around dev_priv in vlv wm functions

[ville.syrjala]

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

Pasing dev_priv instead of dev is the future. Let's make the vlv/chv wm
functions respect that idea.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c955ee6341a1..e3ee07007fa2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1067,9 +1067,9 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
 	int level;
 
 	for (level = 0; level < wm_state->num_levels; level++) {
-		struct drm_device *dev = crtc->base.dev;
+		struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 		const int sr_fifo_size =
-			INTEL_INFO(to_i915(dev))->num_pipes * 512 - 1;
+			INTEL_INFO(dev_priv)->num_pipes * 512 - 1;
 		struct intel_plane *plane;
 
 		wm_state->sr[level].plane =
@@ -1079,7 +1079,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
 			vlv_invert_wm_value(wm_state->sr[level].cursor,
 					    63);
 
-		for_each_intel_plane_on_crtc(dev, crtc, plane) {
+		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
 			wm_state->wm[level].plane[plane->id] =
 				vlv_invert_wm_value(wm_state->wm[level].plane[plane->id],
 						    plane->wm.fifo_size);
@@ -1089,8 +1089,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
 
 static void vlv_compute_wm(struct intel_crtc *crtc)
 {
-	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	struct vlv_wm_state *wm_state = &crtc->wm_state;
 	struct intel_plane *plane;
 	int level;
@@ -1107,7 +1106,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 	if (wm_state->num_active_planes != 1)
 		wm_state->cxsr = false;
 
-	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+	for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
 		struct intel_plane_state *state =
 			to_intel_plane_state(plane->base.state);
 		int level;
@@ -1253,16 +1252,16 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
 
 #undef VLV_FIFO
 
-static void vlv_merge_wm(struct drm_device *dev,
+static void vlv_merge_wm(struct drm_i915_private *dev_priv,
 			 struct vlv_wm_values *wm)
 {
 	struct intel_crtc *crtc;
 	int num_active_crtcs = 0;
 
-	wm->level = to_i915(dev)->wm.max_level;
+	wm->level = dev_priv->wm.max_level;
 	wm->cxsr = true;
 
-	for_each_intel_crtc(dev, crtc) {
+	for_each_intel_crtc(&dev_priv->drm, crtc) {
 		const struct vlv_wm_state *wm_state = &crtc->wm_state;
 
 		if (!crtc->active)
@@ -1281,7 +1280,7 @@ static void vlv_merge_wm(struct drm_device *dev,
 	if (num_active_crtcs > 1)
 		wm->level = VLV_WM_LEVEL_PM2;
 
-	for_each_intel_crtc(dev, crtc) {
+	for_each_intel_crtc(&dev_priv->drm, crtc) {
 		struct vlv_wm_state *wm_state = &crtc->wm_state;
 		enum pipe pipe = crtc->pipe;
 
@@ -1301,13 +1300,12 @@ static void vlv_merge_wm(struct drm_device *dev,
 
 static void vlv_update_wm(struct intel_crtc *crtc)
 {
-	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
 	struct vlv_wm_values wm = {};
 
 	vlv_compute_wm(crtc);
-	vlv_merge_wm(dev, &wm);
+	vlv_merge_wm(dev_priv, &wm);
 
 	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
 		/* FIXME should be part of crtc atomic commit */
-- 
2.7.4

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

Re: [PATCH 08/15] drm/i915: Pass around dev_priv in vlv wm functions

[Maarten Lankhorst]

Op 28-11-16 om 18:37 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Pasing dev_priv instead of dev is the future. Let's make the vlv/chv wm
> functions respect that idea.
^Passing

With that fixed, patch 1-10

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

When I was looking at converting VLV/CHV watermarks to atomic, I came up with a lot of these fixes too, so they just make sense. :)
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c955ee6341a1..e3ee07007fa2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1067,9 +1067,9 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
>  	int level;
>  
>  	for (level = 0; level < wm_state->num_levels; level++) {
> -		struct drm_device *dev = crtc->base.dev;
> +		struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  		const int sr_fifo_size =
> -			INTEL_INFO(to_i915(dev))->num_pipes * 512 - 1;
> +			INTEL_INFO(dev_priv)->num_pipes * 512 - 1;
>  		struct intel_plane *plane;
>  
>  		wm_state->sr[level].plane =
> @@ -1079,7 +1079,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
>  			vlv_invert_wm_value(wm_state->sr[level].cursor,
>  					    63);
>  
> -		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +		for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
>  			wm_state->wm[level].plane[plane->id] =
>  				vlv_invert_wm_value(wm_state->wm[level].plane[plane->id],
>  						    plane->wm.fifo_size);
> @@ -1089,8 +1089,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
>  
>  static void vlv_compute_wm(struct intel_crtc *crtc)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	struct vlv_wm_state *wm_state = &crtc->wm_state;
>  	struct intel_plane *plane;
>  	int level;
> @@ -1107,7 +1106,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>  	if (wm_state->num_active_planes != 1)
>  		wm_state->cxsr = false;
>  
> -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +	for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
>  		struct intel_plane_state *state =
>  			to_intel_plane_state(plane->base.state);
>  		int level;
> @@ -1253,16 +1252,16 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
>  
>  #undef VLV_FIFO
>  
> -static void vlv_merge_wm(struct drm_device *dev,
> +static void vlv_merge_wm(struct drm_i915_private *dev_priv,
>  			 struct vlv_wm_values *wm)
>  {
>  	struct intel_crtc *crtc;
>  	int num_active_crtcs = 0;
>  
> -	wm->level = to_i915(dev)->wm.max_level;
> +	wm->level = dev_priv->wm.max_level;
>  	wm->cxsr = true;
>  
> -	for_each_intel_crtc(dev, crtc) {
> +	for_each_intel_crtc(&dev_priv->drm, crtc) {
>  		const struct vlv_wm_state *wm_state = &crtc->wm_state;
>  
>  		if (!crtc->active)
> @@ -1281,7 +1280,7 @@ static void vlv_merge_wm(struct drm_device *dev,
>  	if (num_active_crtcs > 1)
>  		wm->level = VLV_WM_LEVEL_PM2;
>  
> -	for_each_intel_crtc(dev, crtc) {
> +	for_each_intel_crtc(&dev_priv->drm, crtc) {
>  		struct vlv_wm_state *wm_state = &crtc->wm_state;
>  		enum pipe pipe = crtc->pipe;
>  
> @@ -1301,13 +1300,12 @@ static void vlv_merge_wm(struct drm_device *dev,
>  
>  static void vlv_update_wm(struct intel_crtc *crtc)
>  {
> -	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum pipe pipe = crtc->pipe;
>  	struct vlv_wm_values wm = {};
>  
>  	vlv_compute_wm(crtc);
> -	vlv_merge_wm(dev, &wm);
> +	vlv_merge_wm(dev_priv, &wm);
>  
>  	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
>  		/* FIXME should be part of crtc atomic commit */


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

[PATCH 09/15] drm/i915: Protect cxsr state with wm_mutex

[ville.syrjala]

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

Let's protect the cxsr state with the wm_mutex, since it might
get poked from multiple places if there's a parallel plane update
happening with a pipe getting enable/disabled.

It's still pretty racy for the old platforms, but for vlv/chv it
should work, I think. If not, we'll improve it later anyway.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ce9e7f2f395e..8867bf61ed9c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5023,7 +5023,6 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
 	 */
 	if (HAS_GMCH_DISPLAY(dev_priv)) {
 		intel_set_memory_cxsr(dev_priv, false);
-		dev_priv->wm.vlv.cxsr = false;
 		intel_wait_for_vblank(dev_priv, pipe);
 	}
 }
@@ -5102,7 +5101,6 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 		 */
 		if (old_crtc_state->base.active) {
 			intel_set_memory_cxsr(dev_priv, false);
-			dev_priv->wm.vlv.cxsr = false;
 			intel_wait_for_vblank(dev_priv, crtc->pipe);
 		}
 	}
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e3ee07007fa2..b25a8f0a08ea 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -312,14 +312,13 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
 #define FW_WM(value, plane) \
 	(((value) << DSPFW_ ## plane ## _SHIFT) & DSPFW_ ## plane ## _MASK)
 
-void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
+static void _intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
 {
 	u32 val;
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		I915_WRITE(FW_BLC_SELF_VLV, enable ? FW_CSPWRDWNEN : 0);
 		POSTING_READ(FW_BLC_SELF_VLV);
-		dev_priv->wm.vlv.cxsr = enable;
 	} else if (IS_G4X(dev_priv) || IS_CRESTLINE(dev_priv)) {
 		I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
 		POSTING_READ(FW_BLC_SELF);
@@ -350,6 +349,13 @@ void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
 	DRM_DEBUG_KMS("memory self-refresh is %s\n", enableddisabled(enable));
 }
 
+void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
+{
+	mutex_lock(&dev_priv->wm.wm_mutex);
+	_intel_set_memory_cxsr(dev_priv, enable);
+	dev_priv->wm.vlv.cxsr = enable;
+	mutex_unlock(&dev_priv->wm.wm_mutex);
+}
 
 /*
  * Latency for FIFO fetches is dependent on several factors:
@@ -1322,7 +1328,7 @@ static void vlv_update_wm(struct intel_crtc *crtc)
 		chv_set_memory_pm5(dev_priv, false);
 
 	if (!wm.cxsr && dev_priv->wm.vlv.cxsr)
-		intel_set_memory_cxsr(dev_priv, false);
+		_intel_set_memory_cxsr(dev_priv, false);
 
 	/* FIXME should be part of crtc atomic commit */
 	vlv_pipe_set_fifo_size(crtc);
@@ -1336,7 +1342,7 @@ static void vlv_update_wm(struct intel_crtc *crtc)
 		      wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
 
 	if (wm.cxsr && !dev_priv->wm.vlv.cxsr)
-		intel_set_memory_cxsr(dev_priv, true);
+		_intel_set_memory_cxsr(dev_priv, true);
 
 	if (wm.level >= VLV_WM_LEVEL_PM5 &&
 	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_PM5)
-- 
2.7.4

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

[PATCH 10/15] drm/i915: Skip vblank wait if cxsr was already off

[ville.syrjala]

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

Before we attempt to turn any planes on or off we must first exit
csxr. That's due to cxsr effectively making the plane enable bits
read-only. Currently we achieve that with a vblank wait right after
toggling the cxsr enable bit. We do the vblank wait even if cxsr was
already off, which seems wasteful, so let's try to only do it when
absolutely necessary.

We could start tracking the cxsr state fully somewhere, but for now
it seems easiest to just have intel_set_memory_cxsr() return the
previous cxsr state.

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 | 10 ++++------
 drivers/gpu/drm/i915/intel_pm.c      | 31 ++++++++++++++++++++++++-------
 3 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index be89267474a1..9d808b706824 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3571,7 +3571,7 @@ extern void i915_redisable_vga_power_on(struct drm_i915_private *dev_priv);
 extern bool ironlake_set_drps(struct drm_i915_private *dev_priv, u8 val);
 extern void intel_init_pch_refclk(struct drm_i915_private *dev_priv);
 extern void intel_set_rps(struct drm_i915_private *dev_priv, u8 val);
-extern void intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
+extern bool intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
 				  bool enable);
 
 int i915_reg_read_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8867bf61ed9c..d5392d3cba06 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5021,10 +5021,9 @@ intel_pre_disable_primary_noatomic(struct drm_crtc *crtc)
 	 * event which is after the vblank start event, so we need to have a
 	 * wait-for-vblank between disabling the plane and the pipe.
 	 */
-	if (HAS_GMCH_DISPLAY(dev_priv)) {
-		intel_set_memory_cxsr(dev_priv, false);
+	if (HAS_GMCH_DISPLAY(dev_priv) &&
+	    intel_set_memory_cxsr(dev_priv, false))
 		intel_wait_for_vblank(dev_priv, pipe);
-	}
 }
 
 static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
@@ -5099,10 +5098,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 		 * event which is after the vblank start event, so we need to have a
 		 * wait-for-vblank between disabling the plane and the pipe.
 		 */
-		if (old_crtc_state->base.active) {
-			intel_set_memory_cxsr(dev_priv, false);
+		if (old_crtc_state->base.active &&
+		    intel_set_memory_cxsr(dev_priv, false))
 			intel_wait_for_vblank(dev_priv, crtc->pipe);
-		}
 	}
 
 	/*
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b25a8f0a08ea..24d85a4e4ed3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -312,22 +312,30 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
 #define FW_WM(value, plane) \
 	(((value) << DSPFW_ ## plane ## _SHIFT) & DSPFW_ ## plane ## _MASK)
 
-static void _intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
+static bool _intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
 {
+	bool was_enabled;
 	u32 val;
 
 	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+		was_enabled = I915_READ(FW_BLC_SELF_VLV) & FW_CSPWRDWNEN;
 		I915_WRITE(FW_BLC_SELF_VLV, enable ? FW_CSPWRDWNEN : 0);
 		POSTING_READ(FW_BLC_SELF_VLV);
 	} else if (IS_G4X(dev_priv) || IS_CRESTLINE(dev_priv)) {
+		was_enabled = I915_READ(FW_BLC_SELF) & FW_BLC_SELF_EN;
 		I915_WRITE(FW_BLC_SELF, enable ? FW_BLC_SELF_EN : 0);
 		POSTING_READ(FW_BLC_SELF);
 	} else if (IS_PINEVIEW(dev_priv)) {
-		val = I915_READ(DSPFW3) & ~PINEVIEW_SELF_REFRESH_EN;
-		val |= enable ? PINEVIEW_SELF_REFRESH_EN : 0;
+		val = I915_READ(DSPFW3);
+		was_enabled = val & PINEVIEW_SELF_REFRESH_EN;
+		if (enable)
+			val |= PINEVIEW_SELF_REFRESH_EN;
+		else
+			val &= ~PINEVIEW_SELF_REFRESH_EN;
 		I915_WRITE(DSPFW3, val);
 		POSTING_READ(DSPFW3);
 	} else if (IS_I945G(dev_priv) || IS_I945GM(dev_priv)) {
+		was_enabled = I915_READ(FW_BLC_SELF) & FW_BLC_SELF_EN;
 		val = enable ? _MASKED_BIT_ENABLE(FW_BLC_SELF_EN) :
 			       _MASKED_BIT_DISABLE(FW_BLC_SELF_EN);
 		I915_WRITE(FW_BLC_SELF, val);
@@ -338,23 +346,32 @@ static void _intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enabl
 		 * and yet it does have the related watermark in
 		 * FW_BLC_SELF. What's going on?
 		 */
+		was_enabled = I915_READ(INSTPM) & INSTPM_SELF_EN;
 		val = enable ? _MASKED_BIT_ENABLE(INSTPM_SELF_EN) :
 			       _MASKED_BIT_DISABLE(INSTPM_SELF_EN);
 		I915_WRITE(INSTPM, val);
 		POSTING_READ(INSTPM);
 	} else {
-		return;
+		return false;
 	}
 
-	DRM_DEBUG_KMS("memory self-refresh is %s\n", enableddisabled(enable));
+	DRM_DEBUG_KMS("memory self-refresh is %s (was %s)\n",
+		      enableddisabled(enable),
+		      enableddisabled(was_enabled));
+
+	return was_enabled;
 }
 
-void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
+bool intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
 {
+	bool ret;
+
 	mutex_lock(&dev_priv->wm.wm_mutex);
-	_intel_set_memory_cxsr(dev_priv, enable);
+	ret = _intel_set_memory_cxsr(dev_priv, enable);
 	dev_priv->wm.vlv.cxsr = enable;
 	mutex_unlock(&dev_priv->wm.wm_mutex);
+
+	return ret;
 }
 
 /*
-- 
2.7.4

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

[PATCH 11/15] drm/i915: Protect DSPARB registers with a spinlock

[ville.syrjala]

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

Each DSPARB register can house bits for two separate pipes, hence
we must protect the registers during reprogramming so that parallel
FIFO reconfigurations happening simultaneosly on multiple pipes won't
corrupt each others values.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0fba4bb5655e..c98032e9112b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -811,6 +811,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
 	spin_lock_init(&dev_priv->uncore.lock);
 	spin_lock_init(&dev_priv->mm.object_stat_lock);
 	spin_lock_init(&dev_priv->mmio_flip_lock);
+	spin_lock_init(&dev_priv->wm.dsparb_lock);
 	mutex_init(&dev_priv->sb_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
 	mutex_init(&dev_priv->av_mutex);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9d808b706824..75439e9a67f7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2169,6 +2169,9 @@ struct drm_i915_private {
 	} sagv_status;
 
 	struct {
+		/* protects DSPARB registers on pre-g4x/vlv/chv */
+		spinlock_t dsparb_lock;
+
 		/*
 		 * Raw watermark latency values:
 		 * in 0.1us units for WM0,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 24d85a4e4ed3..9f25f2195a6a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1215,6 +1215,8 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
 		      pipe_name(crtc->pipe), sprite0_start,
 		      sprite1_start, fifo_size);
 
+	spin_lock(&dev_priv->wm.dsparb_lock);
+
 	switch (crtc->pipe) {
 		uint32_t dsparb, dsparb2, dsparb3;
 	case PIPE_A:
@@ -1271,6 +1273,10 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
 	default:
 		break;
 	}
+
+	POSTING_READ(DSPARB);
+
+	spin_unlock(&dev_priv->wm.dsparb_lock);
 }
 
 #undef VLV_FIFO
-- 
2.7.4

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

Re: [PATCH 11/15] drm/i915: Protect DSPARB registers with a spinlock

[Maarten Lankhorst]

Op 28-11-16 om 18:37 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Each DSPARB register can house bits for two separate pipes, hence
> we must protect the registers during reprogramming so that parallel
> FIFO reconfigurations happening simultaneosly on multiple pipes won't
> corrupt each others values.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 1 +
>  drivers/gpu/drm/i915/i915_drv.h | 3 +++
>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
>  3 files changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0fba4bb5655e..c98032e9112b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -811,6 +811,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>  	spin_lock_init(&dev_priv->uncore.lock);
>  	spin_lock_init(&dev_priv->mm.object_stat_lock);
>  	spin_lock_init(&dev_priv->mmio_flip_lock);
> +	spin_lock_init(&dev_priv->wm.dsparb_lock);
Can we make sure all wm updates are done with dev_priv->wm.wm_mutex held instead?

gen9 wm's and ilk wm's use that mutex, for similar reasons.
>  	mutex_init(&dev_priv->sb_lock);
>  	mutex_init(&dev_priv->modeset_restore_lock);
>  	mutex_init(&dev_priv->av_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9d808b706824..75439e9a67f7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2169,6 +2169,9 @@ struct drm_i915_private {
>  	} sagv_status;
>  
>  	struct {
> +		/* protects DSPARB registers on pre-g4x/vlv/chv */
> +		spinlock_t dsparb_lock;
> +
>  		/*
>  		 * Raw watermark latency values:
>  		 * in 0.1us units for WM0,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 24d85a4e4ed3..9f25f2195a6a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1215,6 +1215,8 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
>  		      pipe_name(crtc->pipe), sprite0_start,
>  		      sprite1_start, fifo_size);
>  
> +	spin_lock(&dev_priv->wm.dsparb_lock);
> +
>  	switch (crtc->pipe) {
>  		uint32_t dsparb, dsparb2, dsparb3;
>  	case PIPE_A:
> @@ -1271,6 +1273,10 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
>  	default:
>  		break;
>  	}
> +
> +	POSTING_READ(DSPARB);
> +
> +	spin_unlock(&dev_priv->wm.dsparb_lock);
>  }
>  
>  #undef VLV_FIFO


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

Re: [PATCH 11/15] drm/i915: Protect DSPARB registers with a spinlock

[Ville Syrjälä]

On Thu, Dec 01, 2016 at 12:56:16PM +0100, Maarten Lankhorst wrote:
> Op 28-11-16 om 18:37 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Each DSPARB register can house bits for two separate pipes, hence
> > we must protect the registers during reprogramming so that parallel
> > FIFO reconfigurations happening simultaneosly on multiple pipes won't
> > corrupt each others values.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 1 +
> >  drivers/gpu/drm/i915/i915_drv.h | 3 +++
> >  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> >  3 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 0fba4bb5655e..c98032e9112b 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -811,6 +811,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> >  	spin_lock_init(&dev_priv->uncore.lock);
> >  	spin_lock_init(&dev_priv->mm.object_stat_lock);
> >  	spin_lock_init(&dev_priv->mmio_flip_lock);
> > +	spin_lock_init(&dev_priv->wm.dsparb_lock);
> Can we make sure all wm updates are done with dev_priv->wm.wm_mutex held instead?

We can't grab a mutex from the vblank evade critical section. We'd have
to hold the mutex across the whole thing then. Not sure if that would be
a good or a bad thing.

> 
> gen9 wm's and ilk wm's use that mutex, for similar reasons.
> >  	mutex_init(&dev_priv->sb_lock);
> >  	mutex_init(&dev_priv->modeset_restore_lock);
> >  	mutex_init(&dev_priv->av_mutex);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 9d808b706824..75439e9a67f7 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2169,6 +2169,9 @@ struct drm_i915_private {
> >  	} sagv_status;
> >  
> >  	struct {
> > +		/* protects DSPARB registers on pre-g4x/vlv/chv */
> > +		spinlock_t dsparb_lock;
> > +
> >  		/*
> >  		 * Raw watermark latency values:
> >  		 * in 0.1us units for WM0,
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 24d85a4e4ed3..9f25f2195a6a 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -1215,6 +1215,8 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
> >  		      pipe_name(crtc->pipe), sprite0_start,
> >  		      sprite1_start, fifo_size);
> >  
> > +	spin_lock(&dev_priv->wm.dsparb_lock);
> > +
> >  	switch (crtc->pipe) {
> >  		uint32_t dsparb, dsparb2, dsparb3;
> >  	case PIPE_A:
> > @@ -1271,6 +1273,10 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
> >  	default:
> >  		break;
> >  	}
> > +
> > +	POSTING_READ(DSPARB);
> > +
> > +	spin_unlock(&dev_priv->wm.dsparb_lock);
> >  }
> >  
> >  #undef VLV_FIFO
> 

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

Re: [PATCH 11/15] drm/i915: Protect DSPARB registers with a spinlock

[Maarten Lankhorst]

Op 01-12-16 om 14:13 schreef Ville Syrjälä:
> On Thu, Dec 01, 2016 at 12:56:16PM +0100, Maarten Lankhorst wrote:
>> Op 28-11-16 om 18:37 schreef ville.syrjala@linux.intel.com:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Each DSPARB register can house bits for two separate pipes, hence
>>> we must protect the registers during reprogramming so that parallel
>>> FIFO reconfigurations happening simultaneosly on multiple pipes won't
>>> corrupt each others values.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.c | 1 +
>>>  drivers/gpu/drm/i915/i915_drv.h | 3 +++
>>>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
>>>  3 files changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>> index 0fba4bb5655e..c98032e9112b 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>> @@ -811,6 +811,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>>>  	spin_lock_init(&dev_priv->uncore.lock);
>>>  	spin_lock_init(&dev_priv->mm.object_stat_lock);
>>>  	spin_lock_init(&dev_priv->mmio_flip_lock);
>>> +	spin_lock_init(&dev_priv->wm.dsparb_lock);
>> Can we make sure all wm updates are done with dev_priv->wm.wm_mutex held instead?
> We can't grab a mutex from the vblank evade critical section. We'd have
> to hold the mutex across the whole thing then. Not sure if that would be
> a good or a bad thing.
Ah right, I missed that part since it didn't happen in the patches yet, might be worth pointing out in the commit message.

Will vlv_wm_values also be updated with that lock in the future? Looks like it could be a fun race otherwise.
>> gen9 wm's and ilk wm's use that mutex, for similar reasons.
>>>  	mutex_init(&dev_priv->sb_lock);
>>>  	mutex_init(&dev_priv->modeset_restore_lock);
>>>  	mutex_init(&dev_priv->av_mutex);
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 9d808b706824..75439e9a67f7 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2169,6 +2169,9 @@ struct drm_i915_private {
>>>  	} sagv_status;
>>>  
>>>  	struct {
>>> +		/* protects DSPARB registers on pre-g4x/vlv/chv */
>>> +		spinlock_t dsparb_lock;
>>> +
>>>  		/*
>>>  		 * Raw watermark latency values:
>>>  		 * in 0.1us units for WM0,
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>> index 24d85a4e4ed3..9f25f2195a6a 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -1215,6 +1215,8 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
>>>  		      pipe_name(crtc->pipe), sprite0_start,
>>>  		      sprite1_start, fifo_size);
>>>  
>>> +	spin_lock(&dev_priv->wm.dsparb_lock);
>>> +
>>>  	switch (crtc->pipe) {
>>>  		uint32_t dsparb, dsparb2, dsparb3;
>>>  	case PIPE_A:
>>> @@ -1271,6 +1273,10 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
>>>  	default:
>>>  		break;
>>>  	}
>>> +
>>> +	POSTING_READ(DSPARB);
>>> +
>>> +	spin_unlock(&dev_priv->wm.dsparb_lock);
>>>  }
>>>  
>>>  #undef VLV_FIFO


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

Re: [PATCH 11/15] drm/i915: Protect DSPARB registers with a spinlock

[Ville Syrjälä]

On Thu, Dec 01, 2016 at 03:45:35PM +0100, Maarten Lankhorst wrote:
> Op 01-12-16 om 14:13 schreef Ville Syrjälä:
> > On Thu, Dec 01, 2016 at 12:56:16PM +0100, Maarten Lankhorst wrote:
> >> Op 28-11-16 om 18:37 schreef ville.syrjala@linux.intel.com:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> Each DSPARB register can house bits for two separate pipes, hence
> >>> we must protect the registers during reprogramming so that parallel
> >>> FIFO reconfigurations happening simultaneosly on multiple pipes won't
> >>> corrupt each others values.
> >>>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/i915_drv.c | 1 +
> >>>  drivers/gpu/drm/i915/i915_drv.h | 3 +++
> >>>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> >>>  3 files changed, 10 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >>> index 0fba4bb5655e..c98032e9112b 100644
> >>> --- a/drivers/gpu/drm/i915/i915_drv.c
> >>> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >>> @@ -811,6 +811,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> >>>  	spin_lock_init(&dev_priv->uncore.lock);
> >>>  	spin_lock_init(&dev_priv->mm.object_stat_lock);
> >>>  	spin_lock_init(&dev_priv->mmio_flip_lock);
> >>> +	spin_lock_init(&dev_priv->wm.dsparb_lock);
> >> Can we make sure all wm updates are done with dev_priv->wm.wm_mutex held instead?
> > We can't grab a mutex from the vblank evade critical section. We'd have
> > to hold the mutex across the whole thing then. Not sure if that would be
> > a good or a bad thing.
> Ah right, I missed that part since it didn't happen in the patches yet, might be worth pointing out in the commit message.

Right. A bit of tunnel vision on my part here. I'll plop in a note about it.

> 
> Will vlv_wm_values also be updated with that lock in the future? Looks like it could be a fun race otherwise.

Yes, wm_mutex will be protecting all the global wm state.

> >> gen9 wm's and ilk wm's use that mutex, for similar reasons.
> >>>  	mutex_init(&dev_priv->sb_lock);
> >>>  	mutex_init(&dev_priv->modeset_restore_lock);
> >>>  	mutex_init(&dev_priv->av_mutex);
> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>> index 9d808b706824..75439e9a67f7 100644
> >>> --- a/drivers/gpu/drm/i915/i915_drv.h
> >>> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >>> @@ -2169,6 +2169,9 @@ struct drm_i915_private {
> >>>  	} sagv_status;
> >>>  
> >>>  	struct {
> >>> +		/* protects DSPARB registers on pre-g4x/vlv/chv */
> >>> +		spinlock_t dsparb_lock;
> >>> +
> >>>  		/*
> >>>  		 * Raw watermark latency values:
> >>>  		 * in 0.1us units for WM0,
> >>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>> index 24d85a4e4ed3..9f25f2195a6a 100644
> >>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>> @@ -1215,6 +1215,8 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
> >>>  		      pipe_name(crtc->pipe), sprite0_start,
> >>>  		      sprite1_start, fifo_size);
> >>>  
> >>> +	spin_lock(&dev_priv->wm.dsparb_lock);
> >>> +
> >>>  	switch (crtc->pipe) {
> >>>  		uint32_t dsparb, dsparb2, dsparb3;
> >>>  	case PIPE_A:
> >>> @@ -1271,6 +1273,10 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
> >>>  	default:
> >>>  		break;
> >>>  	}
> >>> +
> >>> +	POSTING_READ(DSPARB);
> >>> +
> >>> +	spin_unlock(&dev_priv->wm.dsparb_lock);
> >>>  }
> >>>  
> >>>  #undef VLV_FIFO
> 

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

[PATCH v2 11/15] drm/i915: Protect DSPARB registers with a spinlock

[ville.syrjala]

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

Each DSPARB register can house bits for two separate pipes, hence
we must protect the registers during reprogramming so that parallel
FIFO reconfigurations happening simultaneosly on multiple pipes won't
corrupt each others values.

We'll use a new spinlock for this instead of the wm_mutex since we'll
have to move the DSPARB programming to happen from the vblank evade
critical section, and we can't use mutexes in there.

v2: Document why we use a spinlock instead of a mutex (Maarten)

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 1 +
 drivers/gpu/drm/i915/i915_drv.h | 3 +++
 drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
 3 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0fba4bb5655e..c98032e9112b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -811,6 +811,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
 	spin_lock_init(&dev_priv->uncore.lock);
 	spin_lock_init(&dev_priv->mm.object_stat_lock);
 	spin_lock_init(&dev_priv->mmio_flip_lock);
+	spin_lock_init(&dev_priv->wm.dsparb_lock);
 	mutex_init(&dev_priv->sb_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
 	mutex_init(&dev_priv->av_mutex);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9d808b706824..75439e9a67f7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2169,6 +2169,9 @@ struct drm_i915_private {
 	} sagv_status;
 
 	struct {
+		/* protects DSPARB registers on pre-g4x/vlv/chv */
+		spinlock_t dsparb_lock;
+
 		/*
 		 * Raw watermark latency values:
 		 * in 0.1us units for WM0,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 24d85a4e4ed3..9f25f2195a6a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1215,6 +1215,8 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
 		      pipe_name(crtc->pipe), sprite0_start,
 		      sprite1_start, fifo_size);
 
+	spin_lock(&dev_priv->wm.dsparb_lock);
+
 	switch (crtc->pipe) {
 		uint32_t dsparb, dsparb2, dsparb3;
 	case PIPE_A:
@@ -1271,6 +1273,10 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
 	default:
 		break;
 	}
+
+	POSTING_READ(DSPARB);
+
+	spin_unlock(&dev_priv->wm.dsparb_lock);
 }
 
 #undef VLV_FIFO
-- 
2.7.4

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

Re: [PATCH v2 11/15] drm/i915: Protect DSPARB registers with a spinlock

[Maarten Lankhorst]

Op 05-12-16 om 15:13 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Each DSPARB register can house bits for two separate pipes, hence
> we must protect the registers during reprogramming so that parallel
> FIFO reconfigurations happening simultaneosly on multiple pipes won't
> corrupt each others values.
>
> We'll use a new spinlock for this instead of the wm_mutex since we'll
> have to move the DSPARB programming to happen from the vblank evade
> critical section, and we can't use mutexes in there.
>
> v2: Document why we use a spinlock instead of a mutex (Maarten)
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 11/15] drm/i915: Protect DSPARB registers with a spinlock

[Ville Syrjälä]

On Tue, Dec 06, 2016 at 09:26:22AM +0100, Maarten Lankhorst wrote:
> Op 05-12-16 om 15:13 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Each DSPARB register can house bits for two separate pipes, hence
> > we must protect the registers during reprogramming so that parallel
> > FIFO reconfigurations happening simultaneosly on multiple pipes won't
> > corrupt each others values.
> >
> > We'll use a new spinlock for this instead of the wm_mutex since we'll
> > have to move the DSPARB programming to happen from the vblank evade
> > critical section, and we can't use mutexes in there.
> >
> > v2: Document why we use a spinlock instead of a mutex (Maarten)
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Thanks. Pushed to dinq.

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

[PATCH 12/15] drm/i915: Zero out HOWM registers before writing new WM/HOWM register values

[ville.syrjala]

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

On VLV/CHV some of the watermark values are split across two registers:
low order bits in one, and high order bits in another. So we may not be
able to update a single watermark value atomically, and thus we must be
careful that we don't temporarily introduce out of bounds values during
the reprogramming. To prevent this we can simply zero out all the high
order bits initially, then we update the low order bits, and finally
we update the high order bits with the final value.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 9f25f2195a6a..8a3441bbff30 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -877,6 +877,17 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
 		   (wm->ddl[pipe].plane[PLANE_SPRITE0] << DDL_SPRITE_SHIFT(0)) |
 		   (wm->ddl[pipe].plane[PLANE_PRIMARY] << DDL_PLANE_SHIFT));
 
+	/*
+	 * Zero the (unused) WM1 watermarks, and also clear all the
+	 * high order bits so that there are no out of bounds values
+	 * present in the registers during the reprogramming.
+	 */
+	I915_WRITE(DSPHOWM, 0);
+	I915_WRITE(DSPHOWM1, 0);
+	I915_WRITE(DSPFW4, 0);
+	I915_WRITE(DSPFW5, 0);
+	I915_WRITE(DSPFW6, 0);
+
 	I915_WRITE(DSPFW1,
 		   FW_WM(wm->sr.plane, SR) |
 		   FW_WM(wm->pipe[PIPE_B].plane[PLANE_CURSOR], CURSORB) |
@@ -924,12 +935,6 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
 			   FW_WM(wm->pipe[PIPE_A].plane[PLANE_PRIMARY] >> 8, PLANEA_HI));
 	}
 
-	/* zero (unused) WM1 watermarks */
-	I915_WRITE(DSPFW4, 0);
-	I915_WRITE(DSPFW5, 0);
-	I915_WRITE(DSPFW6, 0);
-	I915_WRITE(DSPHOWM1, 0);
-
 	POSTING_READ(DSPFW1);
 }
 
-- 
2.7.4

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

Re: [PATCH 12/15] drm/i915: Zero out HOWM registers before writing new WM/HOWM register values

[Maarten Lankhorst]

Op 28-11-16 om 18:37 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> On VLV/CHV some of the watermark values are split across two registers:
> low order bits in one, and high order bits in another. So we may not be
> able to update a single watermark value atomically, and thus we must be
> careful that we don't temporarily introduce out of bounds values during
> the reprogramming. To prevent this we can simply zero out all the high
> order bits initially, then we update the low order bits, and finally
> we update the high order bits with the final value.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 9f25f2195a6a..8a3441bbff30 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -877,6 +877,17 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
>  		   (wm->ddl[pipe].plane[PLANE_SPRITE0] << DDL_SPRITE_SHIFT(0)) |
>  		   (wm->ddl[pipe].plane[PLANE_PRIMARY] << DDL_PLANE_SHIFT));
>  
> +	/*
> +	 * Zero the (unused) WM1 watermarks, and also clear all the
> +	 * high order bits so that there are no out of bounds values
> +	 * present in the registers during the reprogramming.
> +	 */
> +	I915_WRITE(DSPHOWM, 0);
> +	I915_WRITE(DSPHOWM1, 0);
> +	I915_WRITE(DSPFW4, 0);
> +	I915_WRITE(DSPFW5, 0);
> +	I915_WRITE(DSPFW6, 0);
Watermarks for DSPHOWM are inverted right? And lower values just mean more wakeups?
Should be harmless then.

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>  	I915_WRITE(DSPFW1,
>  		   FW_WM(wm->sr.plane, SR) |
>  		   FW_WM(wm->pipe[PIPE_B].plane[PLANE_CURSOR], CURSORB) |
> @@ -924,12 +935,6 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
>  			   FW_WM(wm->pipe[PIPE_A].plane[PLANE_PRIMARY] >> 8, PLANEA_HI));
>  	}
>  
> -	/* zero (unused) WM1 watermarks */
> -	I915_WRITE(DSPFW4, 0);
> -	I915_WRITE(DSPFW5, 0);
> -	I915_WRITE(DSPFW6, 0);
> -	I915_WRITE(DSPHOWM1, 0);
> -
>  	POSTING_READ(DSPFW1);
>  }
>  


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

Re: [PATCH 12/15] drm/i915: Zero out HOWM registers before writing new WM/HOWM register values

[Ville Syrjälä]

On Thu, Dec 01, 2016 at 03:43:07PM +0100, Maarten Lankhorst wrote:
> Op 28-11-16 om 18:37 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > On VLV/CHV some of the watermark values are split across two registers:
> > low order bits in one, and high order bits in another. So we may not be
> > able to update a single watermark value atomically, and thus we must be
> > careful that we don't temporarily introduce out of bounds values during
> > the reprogramming. To prevent this we can simply zero out all the high
> > order bits initially, then we update the low order bits, and finally
> > we update the high order bits with the final value.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 9f25f2195a6a..8a3441bbff30 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -877,6 +877,17 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
> >  		   (wm->ddl[pipe].plane[PLANE_SPRITE0] << DDL_SPRITE_SHIFT(0)) |
> >  		   (wm->ddl[pipe].plane[PLANE_PRIMARY] << DDL_PLANE_SHIFT));
> >  
> > +	/*
> > +	 * Zero the (unused) WM1 watermarks, and also clear all the
> > +	 * high order bits so that there are no out of bounds values
> > +	 * present in the registers during the reprogramming.
> > +	 */
> > +	I915_WRITE(DSPHOWM, 0);
> > +	I915_WRITE(DSPHOWM1, 0);
> > +	I915_WRITE(DSPFW4, 0);
> > +	I915_WRITE(DSPFW5, 0);
> > +	I915_WRITE(DSPFW6, 0);
> Watermarks for DSPHOWM are inverted right? And lower values just mean more wakeups?

Yes, it's all inverted.

> Should be harmless then.
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >  	I915_WRITE(DSPFW1,
> >  		   FW_WM(wm->sr.plane, SR) |
> >  		   FW_WM(wm->pipe[PIPE_B].plane[PLANE_CURSOR], CURSORB) |
> > @@ -924,12 +935,6 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
> >  			   FW_WM(wm->pipe[PIPE_A].plane[PLANE_PRIMARY] >> 8, PLANEA_HI));
> >  	}
> >  
> > -	/* zero (unused) WM1 watermarks */
> > -	I915_WRITE(DSPFW4, 0);
> > -	I915_WRITE(DSPFW5, 0);
> > -	I915_WRITE(DSPFW6, 0);
> > -	I915_WRITE(DSPHOWM1, 0);
> > -
> >  	POSTING_READ(DSPFW1);
> >  }
> >  
> 

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

[PATCH 13/15] drm/i915: Write all DDL registers in one go

[ville.syrjala]

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

We'll want to decouple the vlv/chv wm register reprogramming from any
single pipe. So let's just write all the DDL registers in one go. We
already write all the wm registers anyway since the bits are sprinkled
all over the place and so writing them for just a single pipe would have
been too messy anyway.

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 8a3441bbff30..fee536aff2ee 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -865,17 +865,18 @@ static bool g4x_compute_srwm(struct drm_i915_private *dev_priv,
 #define FW_WM_VLV(value, plane) \
 	(((value) << DSPFW_ ## plane ## _SHIFT) & DSPFW_ ## plane ## _MASK_VLV)
 
-static void vlv_write_wm_values(struct intel_crtc *crtc,
+static void vlv_write_wm_values(struct drm_i915_private *dev_priv,
 				const struct vlv_wm_values *wm)
 {
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	enum pipe pipe = crtc->pipe;
+	enum pipe pipe;
 
-	I915_WRITE(VLV_DDL(pipe),
-		   (wm->ddl[pipe].plane[PLANE_CURSOR] << DDL_CURSOR_SHIFT) |
-		   (wm->ddl[pipe].plane[PLANE_SPRITE1] << DDL_SPRITE_SHIFT(1)) |
-		   (wm->ddl[pipe].plane[PLANE_SPRITE0] << DDL_SPRITE_SHIFT(0)) |
-		   (wm->ddl[pipe].plane[PLANE_PRIMARY] << DDL_PLANE_SHIFT));
+	for_each_pipe(dev_priv, pipe) {
+		I915_WRITE(VLV_DDL(pipe),
+			   (wm->ddl[pipe].plane[PLANE_CURSOR] << DDL_CURSOR_SHIFT) |
+			   (wm->ddl[pipe].plane[PLANE_SPRITE1] << DDL_SPRITE_SHIFT(1)) |
+			   (wm->ddl[pipe].plane[PLANE_SPRITE0] << DDL_SPRITE_SHIFT(0)) |
+			   (wm->ddl[pipe].plane[PLANE_PRIMARY] << DDL_PLANE_SHIFT));
+	}
 
 	/*
 	 * Zero the (unused) WM1 watermarks, and also clear all the
@@ -1361,7 +1362,7 @@ static void vlv_update_wm(struct intel_crtc *crtc)
 	/* FIXME should be part of crtc atomic commit */
 	vlv_pipe_set_fifo_size(crtc);
 
-	vlv_write_wm_values(crtc, &wm);
+	vlv_write_wm_values(dev_priv, &wm);
 
 	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
 		      "sprite0=%d, sprite1=%d, SR: plane=%d, cursor=%d level=%d cxsr=%d\n",
-- 
2.7.4

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

[PATCH 14/15] drm/i915: Clean up vlv_program_watermarks()

[ville.syrjala]

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

Add small helpers to make the intent of the staggered enable/disable
sequence in vlv_program_watermarks() easier on the eyes.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fee536aff2ee..291843c2b61b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1333,55 +1333,63 @@ static void vlv_merge_wm(struct drm_i915_private *dev_priv,
 	}
 }
 
+static bool is_disabling(int old, int new, int threshold)
+{
+	return old >= threshold && new < threshold;
+}
+
+static bool is_enabling(int old, int new, int threshold)
+{
+	return old < threshold && new >= threshold;
+}
+
 static void vlv_update_wm(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
-	struct vlv_wm_values wm = {};
+	struct vlv_wm_values *old_wm = &dev_priv->wm.vlv;
+	struct vlv_wm_values new_wm = {};
 
 	vlv_compute_wm(crtc);
-	vlv_merge_wm(dev_priv, &wm);
+	vlv_merge_wm(dev_priv, &new_wm);
 
-	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
+	if (memcmp(old_wm, &new_wm, sizeof(new_wm)) == 0) {
 		/* FIXME should be part of crtc atomic commit */
 		vlv_pipe_set_fifo_size(crtc);
+
 		return;
 	}
 
-	if (wm.level < VLV_WM_LEVEL_DDR_DVFS &&
-	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_DDR_DVFS)
+	if (is_disabling(old_wm->level, new_wm.level, VLV_WM_LEVEL_DDR_DVFS))
 		chv_set_memory_dvfs(dev_priv, false);
 
-	if (wm.level < VLV_WM_LEVEL_PM5 &&
-	    dev_priv->wm.vlv.level >= VLV_WM_LEVEL_PM5)
+	if (is_disabling(old_wm->level, new_wm.level, VLV_WM_LEVEL_PM5))
 		chv_set_memory_pm5(dev_priv, false);
 
-	if (!wm.cxsr && dev_priv->wm.vlv.cxsr)
+	if (is_disabling(old_wm->cxsr, new_wm.cxsr, true))
 		_intel_set_memory_cxsr(dev_priv, false);
 
 	/* FIXME should be part of crtc atomic commit */
 	vlv_pipe_set_fifo_size(crtc);
 
-	vlv_write_wm_values(dev_priv, &wm);
+	vlv_write_wm_values(dev_priv, &new_wm);
 
 	DRM_DEBUG_KMS("Setting FIFO watermarks - %c: plane=%d, cursor=%d, "
 		      "sprite0=%d, sprite1=%d, SR: plane=%d, cursor=%d level=%d cxsr=%d\n",
-		      pipe_name(pipe), wm.pipe[pipe].plane[PLANE_PRIMARY], wm.pipe[pipe].plane[PLANE_CURSOR],
-		      wm.pipe[pipe].plane[PLANE_SPRITE0], wm.pipe[pipe].plane[PLANE_SPRITE1],
-		      wm.sr.plane, wm.sr.cursor, wm.level, wm.cxsr);
+		      pipe_name(pipe), new_wm.pipe[pipe].plane[PLANE_PRIMARY], new_wm.pipe[pipe].plane[PLANE_CURSOR],
+		      new_wm.pipe[pipe].plane[PLANE_SPRITE0], new_wm.pipe[pipe].plane[PLANE_SPRITE1],
+		      new_wm.sr.plane, new_wm.sr.cursor, new_wm.level, new_wm.cxsr);
 
-	if (wm.cxsr && !dev_priv->wm.vlv.cxsr)
+	if (is_enabling(old_wm->cxsr, new_wm.cxsr, true))
 		_intel_set_memory_cxsr(dev_priv, true);
 
-	if (wm.level >= VLV_WM_LEVEL_PM5 &&
-	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_PM5)
+	if (is_enabling(old_wm->level, new_wm.level, VLV_WM_LEVEL_PM5))
 		chv_set_memory_pm5(dev_priv, true);
 
-	if (wm.level >= VLV_WM_LEVEL_DDR_DVFS &&
-	    dev_priv->wm.vlv.level < VLV_WM_LEVEL_DDR_DVFS)
+	if (is_enabling(old_wm->level, new_wm.level, VLV_WM_LEVEL_DDR_DVFS))
 		chv_set_memory_dvfs(dev_priv, true);
 
-	dev_priv->wm.vlv = wm;
+	*old_wm = new_wm;
 }
 
 #define single_plane_enabled(mask) is_power_of_2(mask)
-- 
2.7.4

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

[PATCH 15/15] drm/i915: Pass crtc state to vlv_compute_wm_level()

[ville.syrjala]

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

Rather than accessing crtc->config in vlv_compute_wm_level() let's
pass in the crtc state explicitly. One step closer to atomic.

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

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 291843c2b61b..2a2aa8968b93 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -978,24 +978,26 @@ static void vlv_setup_wm_latency(struct drm_i915_private *dev_priv)
 	}
 }
 
-static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
-				     struct intel_crtc *crtc,
-				     const struct intel_plane_state *state,
+static uint16_t vlv_compute_wm_level(const struct intel_crtc_state *crtc_state,
+				     const struct intel_plane_state *plane_state,
 				     int level)
 {
+	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	const struct drm_display_mode *adjusted_mode =
+		&crtc_state->base.adjusted_mode;
 	int clock, htotal, cpp, width, wm;
 
 	if (dev_priv->wm.pri_latency[level] == 0)
 		return USHRT_MAX;
 
-	if (!state->base.visible)
+	if (!plane_state->base.visible)
 		return 0;
 
-	cpp = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
-	clock = crtc->config->base.adjusted_mode.crtc_clock;
-	htotal = crtc->config->base.adjusted_mode.crtc_htotal;
-	width = crtc->config->pipe_src_w;
+	cpp = drm_format_plane_cpp(plane_state->base.fb->pixel_format, 0);
+	clock = adjusted_mode->crtc_clock;
+	htotal = adjusted_mode->crtc_htotal;
+	width = crtc_state->pipe_src_w;
 	if (WARN_ON(htotal == 0))
 		htotal = 1;
 
@@ -1145,7 +1147,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 
 		/* normal watermarks */
 		for (level = 0; level < wm_state->num_levels; level++) {
-			int wm = vlv_compute_wm_level(plane, crtc, state, level);
+			int wm = vlv_compute_wm_level(crtc->config, state, level);
 			int max_wm = plane->wm.fifo_size;
 
 			/* hack */
-- 
2.7.4

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

Re: [PATCH 15/15] drm/i915: Pass crtc state to vlv_compute_wm_level()

[Maarten Lankhorst]

Op 28-11-16 om 18:37 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Rather than accessing crtc->config in vlv_compute_wm_level() let's
> pass in the crtc state explicitly. One step closer to atomic.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Yay. All users of intel_crtc->config have to die in the future, but this is temporarily a legit use for it.

For patch 13, 14 and 15

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>  drivers/gpu/drm/i915/intel_pm.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 291843c2b61b..2a2aa8968b93 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -978,24 +978,26 @@ static void vlv_setup_wm_latency(struct drm_i915_private *dev_priv)
>  	}
>  }
>  
> -static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
> -				     struct intel_crtc *crtc,
> -				     const struct intel_plane_state *state,
> +static uint16_t vlv_compute_wm_level(const struct intel_crtc_state *crtc_state,
> +				     const struct intel_plane_state *plane_state,
>  				     int level)
>  {
> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	const struct drm_display_mode *adjusted_mode =
> +		&crtc_state->base.adjusted_mode;
>  	int clock, htotal, cpp, width, wm;
>  
>  	if (dev_priv->wm.pri_latency[level] == 0)
>  		return USHRT_MAX;
>  
> -	if (!state->base.visible)
> +	if (!plane_state->base.visible)
>  		return 0;
>  
> -	cpp = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> -	clock = crtc->config->base.adjusted_mode.crtc_clock;
> -	htotal = crtc->config->base.adjusted_mode.crtc_htotal;
> -	width = crtc->config->pipe_src_w;
> +	cpp = drm_format_plane_cpp(plane_state->base.fb->pixel_format, 0);
> +	clock = adjusted_mode->crtc_clock;
> +	htotal = adjusted_mode->crtc_htotal;
> +	width = crtc_state->pipe_src_w;
>  	if (WARN_ON(htotal == 0))
>  		htotal = 1;
>  
> @@ -1145,7 +1147,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>  
>  		/* normal watermarks */
>  		for (level = 0; level < wm_state->num_levels; level++) {
> -			int wm = vlv_compute_wm_level(plane, crtc, state, level);
> +			int wm = vlv_compute_wm_level(crtc->config, state, level);
>  			int max_wm = plane->wm.fifo_size;
>  
>  			/* hack */


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

Re: [PATCH 15/15] drm/i915: Pass crtc state to vlv_compute_wm_level()

[Ville Syrjälä]

On Thu, Dec 01, 2016 at 03:47:55PM +0100, Maarten Lankhorst wrote:
> Op 28-11-16 om 18:37 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Rather than accessing crtc->config in vlv_compute_wm_level() let's
> > pass in the crtc state explicitly. One step closer to atomic.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Yay. All users of intel_crtc->config have to die in the future, but this is temporarily a legit use for it.
> 
> For patch 13, 14 and 15
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >  drivers/gpu/drm/i915/intel_pm.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 291843c2b61b..2a2aa8968b93 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -978,24 +978,26 @@ static void vlv_setup_wm_latency(struct drm_i915_private *dev_priv)
> >  	}
> >  }
> >  
> > -static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
> > -				     struct intel_crtc *crtc,
> > -				     const struct intel_plane_state *state,
> > +static uint16_t vlv_compute_wm_level(const struct intel_crtc_state *crtc_state,
> > +				     const struct intel_plane_state *plane_state,
> >  				     int level)
> >  {
> > +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +	const struct drm_display_mode *adjusted_mode =
> > +		&crtc_state->base.adjusted_mode;
> >  	int clock, htotal, cpp, width, wm;
> >  
> >  	if (dev_priv->wm.pri_latency[level] == 0)
> >  		return USHRT_MAX;
> >  
> > -	if (!state->base.visible)
> > +	if (!plane_state->base.visible)
> >  		return 0;
> >  
> > -	cpp = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> > -	clock = crtc->config->base.adjusted_mode.crtc_clock;
> > -	htotal = crtc->config->base.adjusted_mode.crtc_htotal;
> > -	width = crtc->config->pipe_src_w;
> > +	cpp = drm_format_plane_cpp(plane_state->base.fb->pixel_format, 0);
> > +	clock = adjusted_mode->crtc_clock;
> > +	htotal = adjusted_mode->crtc_htotal;
> > +	width = crtc_state->pipe_src_w;
> >  	if (WARN_ON(htotal == 0))
> >  		htotal = 1;
> >  
> > @@ -1145,7 +1147,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> >  
> >  		/* normal watermarks */
> >  		for (level = 0; level < wm_state->num_levels; level++) {
> > -			int wm = vlv_compute_wm_level(plane, crtc, state, level);
> > +			int wm = vlv_compute_wm_level(crtc->config, state, level);
> >  			int max_wm = plane->wm.fifo_size;
> >  
> >  			/* hack */
> 

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

Re: [PATCH 15/15] drm/i915: Pass crtc state to vlv_compute_wm_level()

[Ville Syrjälä]

On Thu, Dec 01, 2016 at 03:47:55PM +0100, Maarten Lankhorst wrote:
> Op 28-11-16 om 18:37 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Rather than accessing crtc->config in vlv_compute_wm_level() let's
> > pass in the crtc state explicitly. One step closer to atomic.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Yay. All users of intel_crtc->config have to die in the future, but this is temporarily a legit use for it.

Pardon the stray reply earlier. What I wanted to say was that I was
playing around with coccinelle a bit the other night, and managed to
trick it into a little bit of crtc->config killing. I think I'll try
to expand my efforts there a bit more before posting the stuff though.

> 
> For patch 13, 14 and 15
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >  drivers/gpu/drm/i915/intel_pm.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 291843c2b61b..2a2aa8968b93 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -978,24 +978,26 @@ static void vlv_setup_wm_latency(struct drm_i915_private *dev_priv)
> >  	}
> >  }
> >  
> > -static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
> > -				     struct intel_crtc *crtc,
> > -				     const struct intel_plane_state *state,
> > +static uint16_t vlv_compute_wm_level(const struct intel_crtc_state *crtc_state,
> > +				     const struct intel_plane_state *plane_state,
> >  				     int level)
> >  {
> > +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +	const struct drm_display_mode *adjusted_mode =
> > +		&crtc_state->base.adjusted_mode;
> >  	int clock, htotal, cpp, width, wm;
> >  
> >  	if (dev_priv->wm.pri_latency[level] == 0)
> >  		return USHRT_MAX;
> >  
> > -	if (!state->base.visible)
> > +	if (!plane_state->base.visible)
> >  		return 0;
> >  
> > -	cpp = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> > -	clock = crtc->config->base.adjusted_mode.crtc_clock;
> > -	htotal = crtc->config->base.adjusted_mode.crtc_htotal;
> > -	width = crtc->config->pipe_src_w;
> > +	cpp = drm_format_plane_cpp(plane_state->base.fb->pixel_format, 0);
> > +	clock = adjusted_mode->crtc_clock;
> > +	htotal = adjusted_mode->crtc_htotal;
> > +	width = crtc_state->pipe_src_w;
> >  	if (WARN_ON(htotal == 0))
> >  		htotal = 1;
> >  
> > @@ -1145,7 +1147,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> >  
> >  		/* normal watermarks */
> >  		for (level = 0; level < wm_state->num_levels; level++) {
> > -			int wm = vlv_compute_wm_level(plane, crtc, state, level);
> > +			int wm = vlv_compute_wm_level(crtc->config, state, level);
> >  			int max_wm = plane->wm.fifo_size;
> >  
> >  			/* hack */
> 

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

Re: [PATCH 15/15] drm/i915: Pass crtc state to vlv_compute_wm_level()

[Maarten Lankhorst]

Hey,

Op 02-12-16 om 14:07 schreef Ville Syrjälä:
> On Thu, Dec 01, 2016 at 03:47:55PM +0100, Maarten Lankhorst wrote:
>> Op 28-11-16 om 18:37 schreef ville.syrjala@linux.intel.com:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> Rather than accessing crtc->config in vlv_compute_wm_level() let's
>>> pass in the crtc state explicitly. One step closer to atomic.
>>>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Yay. All users of intel_crtc->config have to die in the future, but this is temporarily a legit use for it.
> Pardon the stray reply earlier. What I wanted to say was that I was
> playing around with coccinelle a bit the other night, and managed to
> trick it into a little bit of crtc->config killing. I think I'll try
> to expand my efforts there a bit more before posting the stuff though.
>
Don't worry about it, most of them have to be audited to make sure that the right crtc_state passed in is used. It shouldn't blindly be used to convert to crtc->state.
I did some of the more tricky conversions in my nightly branch. https://cgit.freedesktop.org/~mlankhorst/linux/log/?h=nightly
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: VLV/CHV atomic wm prep work

[Patchwork]

== Series Details ==

Series: drm/i915: VLV/CHV atomic wm prep work
URL   : https://patchwork.freedesktop.org/series/16049/
State : success

== Summary ==

Series 16049v1 drm/i915: VLV/CHV atomic wm prep work
https://patchwork.freedesktop.org/api/1.0/series/16049/revisions/1/mbox/


fi-bdw-5557u     total:245  pass:230  dwarn:0   dfail:0   fail:0   skip:15 
fi-bxt-t5700     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
fi-ilk-650       total:245  pass:192  dwarn:0   dfail:0   fail:0   skip:53 
fi-skl-6260u     total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:245  pass:224  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:245  pass:223  dwarn:1   dfail:0   fail:0   skip:21 

477cc06f6ef4842fd63e28ff30d721462af199a6 drm-tip: 2016y-11m-28d-16h-17m-39s UTC integration manifest
88f6419 drm/i915: Pass crtc state to vlv_compute_wm_level()
1eefb43 drm/i915: Clean up vlv_program_watermarks()
64e5d5f drm/i915: Write all DDL registers in one go
eb21519 drm/i915: Zero out HOWM registers before writing new WM/HOWM register values
504910a drm/i915: Protect DSPARB registers with a spinlock
bcd7ce2b drm/i915: Skip vblank wait if cxsr was already off
1f7b10b drm/i915: Protect cxsr state with wm_mutex
4a4a8e8 drm/i915: Pass around dev_priv in vlv wm functions
929c00f drm/i915: Introduce vlv_invert_wm_value()
7c0bdea drm/i915: Organize vlv/chv watermarks by plane_id
4c71fb0 drm/i915: Remove duplicated wm setup for vlv and chv
132b000 drm/i915: Clean up VLV/CHV maxfifo watermark setup
d39eaac drm/i915: Fix the level 0 max_wm hack on VLV/CHV
a96b9e0 drm/i915: Use the ilk_disable_lp_wm() return value
1116ef3 drm/i915: Drop the nop intel_update_watermarks() call from haswell_crtc_enable()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3129/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: VLV/CHV atomic wm prep work

[Patchwork]

== Series Details ==

Series: drm/i915: VLV/CHV atomic wm prep work
URL   : https://patchwork.freedesktop.org/series/16049/
State : success

== Summary ==

Series 16049v1 drm/i915: VLV/CHV atomic wm prep work
https://patchwork.freedesktop.org/api/1.0/series/16049/revisions/1/mbox/


fi-bdw-5557u     total:245  pass:230  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:245  pass:205  dwarn:0   dfail:0   fail:0   skip:40 
fi-byt-j1900     total:245  pass:217  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:245  pass:225  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:245  pass:192  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7500u     total:245  pass:223  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:245  pass:224  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:245  pass:223  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:245  pass:231  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:245  pass:213  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:245  pass:212  dwarn:0   dfail:0   fail:0   skip:33 

bf8dfcdb45998f470c39ea7b89412a2edbd069db drm-tip: 2016y-11m-29d-09h-31m-06s UTC integration manifest
c43a650 drm/i915: Pass crtc state to vlv_compute_wm_level()
cb4a7f1 drm/i915: Clean up vlv_program_watermarks()
e08c64e drm/i915: Write all DDL registers in one go
416759c drm/i915: Zero out HOWM registers before writing new WM/HOWM register values
a5970d8 drm/i915: Protect DSPARB registers with a spinlock
cec33e4 drm/i915: Skip vblank wait if cxsr was already off
e8ab9b6 drm/i915: Protect cxsr state with wm_mutex
25e6b89 drm/i915: Pass around dev_priv in vlv wm functions
1371815 drm/i915: Introduce vlv_invert_wm_value()
908d4aa drm/i915: Organize vlv/chv watermarks by plane_id
4b3c888 drm/i915: Remove duplicated wm setup for vlv and chv
3560754 drm/i915: Clean up VLV/CHV maxfifo watermark setup
00f47ab drm/i915: Fix the level 0 max_wm hack on VLV/CHV
2cfcc52 drm/i915: Use the ilk_disable_lp_wm() return value
a48708a drm/i915: Drop the nop intel_update_watermarks() call from haswell_crtc_enable()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3136/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/15] drm/i915: VLV/CHV atomic wm prep work

[Ville Syrjälä]

On Mon, Nov 28, 2016 at 07:37:02PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> A decent pile of prep work split off from my VLV/CHV atomic watermark
> work. Mostly VLV/CHV specific stuff, but there are a few small things
> in there that touch other platforms as well.
> 
> Entire series available here:
> git://github.com/vsyrjala/linux.git vlv_atomic_wm_prep
> 
> Ville Syrjälä (15):
>   drm/i915: Drop the nop intel_update_watermarks() call from
>     haswell_crtc_enable()
>   drm/i915: Use the ilk_disable_lp_wm() return value
>   drm/i915: Fix the level 0 max_wm hack on VLV/CHV
>   drm/i915: Clean up VLV/CHV maxfifo watermark setup
>   drm/i915: Remove duplicated wm setup for vlv and chv
>   drm/i915: Organize vlv/chv watermarks by plane_id
>   drm/i915: Introduce vlv_invert_wm_value()
>   drm/i915: Pass around dev_priv in vlv wm functions
>   drm/i915: Protect cxsr state with wm_mutex
>   drm/i915: Skip vblank wait if cxsr was already off
>   drm/i915: Zero out HOWM registers before writing new WM/HOWM register
>     values
>   drm/i915: Write all DDL registers in one go
>   drm/i915: Clean up vlv_program_watermarks()
>   drm/i915: Pass crtc state to vlv_compute_wm_level()

Pushed all of the above to dinq. Thanks for the reviews.

>   drm/i915: Protect DSPARB registers with a spinlock

Sent out a v2 of this one, with an amended commit msg.

> 
>  drivers/gpu/drm/i915/i915_drv.c      |   1 +
>  drivers/gpu/drm/i915/i915_drv.h      |  21 +-
>  drivers/gpu/drm/i915/intel_display.c |  21 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 394 ++++++++++++++++++-----------------
>  4 files changed, 218 insertions(+), 219 deletions(-)
> 
> -- 
> 2.7.4

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