[RFC 0/8] Add two-stage watermark programming for VLV/CHV (v5)

[RFC 0/8] Add two-stage watermark programming for VLV/CHV (v5)

[Chi Ding]

From: root <root@localhost.localdomain>

In addition to calculating final watermarks, we also pre-calculate 
a set of intermediate watermark values at atomic check time. These 
intermediate watermarks are a combination of the watermarks for the 
old state and the new state; they should satisfy the requirements of 
both states which means they can be programmed immediately when we 
commit the atomic state (without waiting for a vblank).  Once the 
vblank does happen, we can then re-program watermarks to the more 
optimal final value.

The DSPARB Display Arbitration Control register is double buffered. 
The FIFO repartitioning happens atomically with plane updates but 
how the register double buffering works isn't clear at the moment.
It needs to be figured out to fix the watermark updates.

v2: Split the first patch of v1 into the following patches
- Remove unused parameters from intel_plane_wm_parameters.
- Rename skl_plane_id to wm_plane_id.
- Move fifo_size from intel_plane_wm_parameters to vlv_wm_state.

v3: Split the 3rd patch of v2 into the following two patches
- Move fifo_size from intel_plane_wm_parameters to vlv_wm_state
- Change to use intel_crtc_state instead of base CRTC object
- The patch headlines are also changed to fit the requirement.

v4: 
- Split the 3rd patch of v3 "Move fifo_size from intel_plane_wm_parameters 
to vlv_wm_state" to add a new one "return EINVAL when computed 
watermark exceeds system limitation"
- Add a new patch "Move active watermarks into intel_crtc->wm.active.vlv"
to be consistent with what we do on other platforms
- Change the patch "Add intermediate field in intel_crtc_wm_state and handlers
for two-level watermark" to use macro drm_atomic_crtc_state_for_each_plane_state
to simplify the code
- Change the patch "Add optimal field in intel_crtc_wm_state" to 
use mutex in vlv_update_wm to make assigning currently active wm_state
and merging multiple wm_state become one atomic operation

v5: recreated the patches because v4 patches were created with drm-intel 
kernel 4.7.0-rc1+ and can't be applied on the current version 4.7.0-rc4+
  

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Chi Ding <chix.ding@intel.com>

Maarten Lankhorst (8):
  drm/i915: Remove unused parameters from intel_plane_wm_parameters
  drm/i915: Rename skl_wm_plane_id to wm_plane_id
  drm/i915/vlv: Move fifo_size from intel_plane_wm_parameters to
    vlv_wm_state
  drm/i915/vlv: return EINVAL when computed watermark exceeds system
    limitation
  drm/i915/vlv: Change to use intel_crtc_state instead of base CRTC
    object
  drm/i915/vlv: Add optimal field in intel_crtc_wm_state
  drm/i915/vlv: Move active watermarks into intel_crtc->wm.active.vlv
  drm/i915/vlv: Add intermediate field in intel_crtc_wm_state and
    handlers     for two-level watermark

 drivers/gpu/drm/i915/intel_display.c |   8 +-
 drivers/gpu/drm/i915/intel_drv.h     |  52 ++----
 drivers/gpu/drm/i915/intel_pm.c      | 326 +++++++++++++++++++++++------------
 3 files changed, 232 insertions(+), 154 deletions(-)

-- 
1.8.0.1

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

[RFC 1/8] drm/i915: Remove unused parameters from intel_plane_wm_parameters

[Chi Ding]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Everything except fifo_size is unused and therefore removed

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Chi Ding <chix.ding@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1f82dcc..e6dea69 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -696,21 +696,6 @@ struct intel_crtc {
 };
 
 struct intel_plane_wm_parameters {
-	uint32_t horiz_pixels;
-	uint32_t vert_pixels;
-	/*
-	 *   For packed pixel formats:
-	 *     bytes_per_pixel - holds bytes per pixel
-	 *   For planar pixel formats:
-	 *     bytes_per_pixel - holds bytes per pixel for uv-plane
-	 *     y_bytes_per_pixel - holds bytes per pixel for y-plane
-	 */
-	uint8_t bytes_per_pixel;
-	uint8_t y_bytes_per_pixel;
-	bool enabled;
-	bool scaled;
-	u64 tiling;
-	unsigned int rotation;
 	uint16_t fifo_size;
 };
 
-- 
1.8.0.1

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

[RFC 2/8] drm/i915: Rename skl_wm_plane_id to wm_plane_id

[Chi Ding]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This function will be used not only by SKL but also VLV/CHV.
Therefore it's renamed

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Chi Ding <chix.ding@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 57 +++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c94521cc..29820e2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -55,6 +55,28 @@
 #define INTEL_RC6p_ENABLE			(1<<1)
 #define INTEL_RC6pp_ENABLE			(1<<2)
 
+/*
+ * Return the index of a plane in the DDB and wm result arrays.  Primary
+ * plane is always in slot 0, cursor is always in slot I915_MAX_PLANES-1, and
+ * other universal planes are in indices 1..n.  Note that this may leave unused
+ * indices between the top "sprite" plane and the cursor.
+ */
+static int
+wm_plane_id(const struct intel_plane *plane)
+{
+	switch (plane->base.type) {
+	case DRM_PLANE_TYPE_PRIMARY:
+		return 0;
+	case DRM_PLANE_TYPE_CURSOR:
+		return PLANE_CURSOR;
+	case DRM_PLANE_TYPE_OVERLAY:
+		return plane->plane + 1;
+	default:
+		MISSING_CASE(plane->base.type);
+		return plane->plane;
+	}
+}
+
 static void gen9_init_clock_gating(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -2853,27 +2875,6 @@ bool ilk_disable_lp_wm(struct drm_device *dev)
 #define SKL_DDB_SIZE		896	/* in blocks */
 #define BXT_DDB_SIZE		512
 
-/*
- * Return the index of a plane in the SKL DDB and wm result arrays.  Primary
- * plane is always in slot 0, cursor is always in slot I915_MAX_PLANES-1, and
- * other universal planes are in indices 1..n.  Note that this may leave unused
- * indices between the top "sprite" plane and the cursor.
- */
-static int
-skl_wm_plane_id(const struct intel_plane *plane)
-{
-	switch (plane->base.type) {
-	case DRM_PLANE_TYPE_PRIMARY:
-		return 0;
-	case DRM_PLANE_TYPE_CURSOR:
-		return PLANE_CURSOR;
-	case DRM_PLANE_TYPE_OVERLAY:
-		return plane->plane + 1;
-	default:
-		MISSING_CASE(plane->base.type);
-		return plane->plane;
-	}
-}
 
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
@@ -3081,7 +3082,7 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
 
 	/* Calculate and cache data rate for each plane */
 	for_each_plane_in_state(state, plane, pstate, i) {
-		id = skl_wm_plane_id(to_intel_plane(plane));
+		id = wm_plane_id(to_intel_plane(plane));
 		intel_plane = to_intel_plane(plane);
 
 		if (intel_plane->pipe != intel_crtc->pipe)
@@ -3100,7 +3101,7 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate)
 
 	/* Calculate CRTC's total data rate from cached values */
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
-		int id = skl_wm_plane_id(intel_plane);
+		int id = wm_plane_id(intel_plane);
 
 		/* packed/uv */
 		total_data_rate += intel_cstate->wm.skl.plane_data_rate[id];
@@ -3221,7 +3222,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	/* 1. Allocate the mininum required blocks for each active plane */
 	for_each_plane_in_state(state, plane, pstate, i) {
 		intel_plane = to_intel_plane(plane);
-		id = skl_wm_plane_id(intel_plane);
+		id = wm_plane_id(intel_plane);
 
 		if (intel_plane->pipe != pipe)
 			continue;
@@ -3260,7 +3261,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
 		unsigned int data_rate, y_data_rate;
 		uint16_t plane_blocks, y_plane_blocks = 0;
-		int id = skl_wm_plane_id(intel_plane);
+		int id = wm_plane_id(intel_plane);
 
 		data_rate = cstate->wm.skl.plane_data_rate[id];
 
@@ -3477,7 +3478,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
 			DRM_DEBUG_KMS("Plane %d.%d: blocks required = %u/%u, lines required = %u/31\n",
 				      to_intel_crtc(cstate->base.crtc)->pipe,
-				      skl_wm_plane_id(to_intel_plane(pstate->plane)),
+				      wm_plane_id(to_intel_plane(pstate->plane)),
 				      res_blocks, ddb_allocation, res_lines);
 
 			return -EINVAL;
@@ -3515,7 +3516,7 @@ skl_compute_wm_level(const struct drm_i915_private *dev_priv,
 	memset(result, 0, sizeof(*result));
 
 	for_each_intel_plane_mask(dev, intel_plane, cstate->base.plane_mask) {
-		int i = skl_wm_plane_id(intel_plane);
+		int i = wm_plane_id(intel_plane);
 
 		plane = &intel_plane->base;
 		intel_pstate = NULL;
@@ -3584,7 +3585,7 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
 
 	/* Until we know more, just disable transition WMs */
 	for_each_intel_plane_on_crtc(crtc->dev, intel_crtc, intel_plane) {
-		int i = skl_wm_plane_id(intel_plane);
+		int i = wm_plane_id(intel_plane);
 
 		trans_wm->plane_en[i] = false;
 	}
-- 
1.8.0.1

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

[RFC 3/8] drm/i915/vlv: Move fifo_size from intel_plane_wm_parameters to vlv_wm_state

[Chi Ding]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This commit saves watermark for each plane in vlv_wm_state to prepare
for two-level watermark because we'll compute and save intermediate and
optimal watermark and fifo size for each plane.

v2:
- remove redundant debug statements in vlv_pipe_set_fifo_size()
- reverse passing vlv_wm_state by parameter because it's unrelated to the
patch
- reverse the change of returning EINVAL when wm > max_wm in vlv_compute_wm
because it's unrelated to the patch

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Chi Ding <chix.ding@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 12 +-----
 drivers/gpu/drm/i915/intel_pm.c  | 90 ++++++++++++++++++++--------------------
 2 files changed, 46 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e6dea69..ae3e73e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -623,6 +623,7 @@ struct intel_crtc_state {
 struct vlv_wm_state {
 	struct vlv_pipe_wm wm[3];
 	struct vlv_sr_wm sr[3];
+	uint16_t fifo_size[I915_MAX_PLANES];
 	uint8_t num_active_planes;
 	uint8_t num_levels;
 	uint8_t level;
@@ -695,10 +696,6 @@ struct intel_crtc {
 	struct vlv_wm_state wm_state;
 };
 
-struct intel_plane_wm_parameters {
-	uint16_t fifo_size;
-};
-
 struct intel_plane {
 	struct drm_plane base;
 	int plane;
@@ -707,13 +704,6 @@ struct intel_plane {
 	int max_downscale;
 	uint32_t frontbuffer_bit;
 
-	/* Since we need to change the watermarks before/after
-	 * enabling/disabling the planes, we need to store the parameters here
-	 * as the other pieces of the struct may not reflect the values we want
-	 * for the watermark calculations. Currently only Haswell uses this.
-	 */
-	struct intel_plane_wm_parameters wm;
-
 	/*
 	 * NOTE: Do not place new plane state fields here (e.g., when adding
 	 * new plane properties).  New runtime state should now be placed in
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 29820e2..3e239af 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1010,12 +1010,14 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
 
 static void vlv_compute_fifo(struct intel_crtc *crtc)
 {
-	struct drm_device *dev = crtc->base.dev;
 	struct vlv_wm_state *wm_state = &crtc->wm_state;
+	struct drm_device *dev = crtc->base.dev;
 	struct intel_plane *plane;
 	unsigned int total_rate = 0;
 	const int fifo_size = 512 - 1;
 	int fifo_extra, fifo_left = fifo_size;
+	int rate[I915_MAX_PLANES] = {};
+	int i;
 
 	for_each_intel_plane_on_crtc(dev, crtc, plane) {
 		struct intel_plane_state *state =
@@ -1026,49 +1028,46 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
 
 		if (state->visible) {
 			wm_state->num_active_planes++;
-			total_rate += drm_format_plane_cpp(state->base.fb->pixel_format, 0);
+			rate[wm_plane_id(plane)] =
+			drm_format_plane_cpp(state->base.fb->pixel_format, 0);
+			total_rate += rate[wm_plane_id(plane)];
 		}
 	}
 
-	for_each_intel_plane_on_crtc(dev, crtc, plane) {
-		struct intel_plane_state *state =
-			to_intel_plane_state(plane->base.state);
-		unsigned int rate;
-
-		if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
-			plane->wm.fifo_size = 63;
+	for (i = 0; i < I915_MAX_PLANES; i++) {
+		if (i == PLANE_CURSOR) {
+			wm_state->fifo_size[i] = 63;
 			continue;
 		}
 
-		if (!state->visible) {
-			plane->wm.fifo_size = 0;
+		if (!rate[i]) {
+			wm_state->fifo_size[i] = 0;
 			continue;
 		}
 
-		rate = drm_format_plane_cpp(state->base.fb->pixel_format, 0);
-		plane->wm.fifo_size = fifo_size * rate / total_rate;
-		fifo_left -= plane->wm.fifo_size;
+		wm_state->fifo_size[i] = fifo_size * rate[i] / total_rate;
+		fifo_left -= wm_state->fifo_size[i];
 	}
 
 	fifo_extra = DIV_ROUND_UP(fifo_left, wm_state->num_active_planes ?: 1);
 
 	/* spread the remainder evenly */
-	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+	for (i = 0; i < I915_MAX_PLANES; i++) {
 		int plane_extra;
 
 		if (fifo_left == 0)
 			break;
 
-		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
+		if (i == PLANE_CURSOR)
 			continue;
 
 		/* give it all to the first plane if none are active */
-		if (plane->wm.fifo_size == 0 &&
+		if (!wm_state->fifo_size[i] &&
 		    wm_state->num_active_planes)
 			continue;
 
 		plane_extra = min(fifo_extra, fifo_left);
-		plane->wm.fifo_size += plane_extra;
+		wm_state->fifo_size[i] += plane_extra;
 		fifo_left -= plane_extra;
 	}
 
@@ -1089,19 +1088,24 @@ 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) {
+			int i = wm_plane_id(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 =
+					wm_state->fifo_size[i] -
 					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 =
+					wm_state->fifo_size[i] -
 					wm_state->wm[level].primary;
 				break;
 			case DRM_PLANE_TYPE_OVERLAY:
 				sprite = plane->plane;
-				wm_state->wm[level].sprite[sprite] = plane->wm.fifo_size -
+				wm_state->wm[level].sprite[sprite] =
+					wm_state->fifo_size[i] -
 					wm_state->wm[level].sprite[sprite];
 				break;
 			}
@@ -1109,7 +1113,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
 	}
 }
 
-static void vlv_compute_wm(struct intel_crtc *crtc)
+static int vlv_compute_wm(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct vlv_wm_state *wm_state = &crtc->wm_state;
@@ -1152,7 +1156,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 			if (WARN_ON(level == 0 && wm > max_wm))
 				wm = max_wm;
 
-			if (wm > plane->wm.fifo_size)
+			if (wm > wm_state->fifo_size[wm_plane_id(plane)])
 				break;
 
 			switch (plane->base.type) {
@@ -1206,6 +1210,8 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 	}
 
 	vlv_invert_wms(crtc);
+
+	return 0;
 }
 
 #define VLV_FIFO(plane, value) \
@@ -1215,26 +1221,16 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_plane *plane;
 	int sprite0_start = 0, sprite1_start = 0, fifo_size = 0;
+	const struct vlv_wm_state *wm_state = &crtc->wm_state;
 
-	for_each_intel_plane_on_crtc(dev, crtc, plane) {
-		if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
-			WARN_ON(plane->wm.fifo_size != 63);
-			continue;
-		}
-
-		if (plane->base.type == DRM_PLANE_TYPE_PRIMARY)
-			sprite0_start = plane->wm.fifo_size;
-		else if (plane->plane == 0)
-			sprite1_start = sprite0_start + plane->wm.fifo_size;
-		else
-			fifo_size = sprite1_start + plane->wm.fifo_size;
-	}
 
-	WARN_ON(fifo_size != 512 - 1);
+	WARN_ON(wm_state->fifo_size[PLANE_CURSOR] != 63);
+	sprite0_start = wm_state->fifo_size[0];
+	sprite1_start = sprite0_start + wm_state->fifo_size[1];
+	fifo_size = sprite1_start + wm_state->fifo_size[2];
 
-	DRM_DEBUG_KMS("Pipe %c FIFO split %d / %d / %d\n",
+	WARN(fifo_size != 512 - 1, "Pipe %c FIFO split %d / %d / %d\n",
 		      pipe_name(crtc->pipe), sprite0_start,
 		      sprite1_start, fifo_size);
 
@@ -4354,6 +4350,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct vlv_wm_values *wm = &dev_priv->wm.vlv;
+	struct intel_crtc *crtc;
 	struct intel_plane *plane;
 	enum pipe pipe;
 	u32 val;
@@ -4361,17 +4358,20 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
 	vlv_read_wm_values(dev_priv, wm);
 
 	for_each_intel_plane(dev, plane) {
+		struct vlv_wm_state *wm_state;
+		int i = wm_plane_id(plane);
+
+		crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, plane->pipe));
+		wm_state = &crtc->wm_state;
+
 		switch (plane->base.type) {
-			int sprite;
 		case DRM_PLANE_TYPE_CURSOR:
-			plane->wm.fifo_size = 63;
+			wm_state->fifo_size[i] = 63;
 			break;
 		case DRM_PLANE_TYPE_PRIMARY:
-			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, 0);
-			break;
 		case DRM_PLANE_TYPE_OVERLAY:
-			sprite = plane->plane;
-			plane->wm.fifo_size = vlv_get_fifo_size(dev, plane->pipe, sprite + 1);
+			wm_state->fifo_size[i] =
+				vlv_get_fifo_size(dev, plane->pipe, i);
 			break;
 		}
 	}
-- 
1.8.0.1

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

[RFC 4/8] drm/i915/vlv: return EINVAL when computed watermark exceeds system limitation

[Chi Ding]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

When computing normal watermarks in vlv_compute_wm(), if the value
is bigger than system limitation, return EINVAL

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Chi Ding <chix.ding@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3e239af..3fb896a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1152,9 +1152,14 @@ static int vlv_compute_wm(struct intel_crtc *crtc)
 			int wm = vlv_compute_wm_level(plane, crtc, state, level);
 			int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
 
-			/* hack */
-			if (WARN_ON(level == 0 && wm > max_wm))
-				wm = max_wm;
+			if (level == 0 && wm > max_wm) {
+				DRM_DEBUG_KMS("Requested display configuration "
+				"exceeds system watermark limitations\n");
+				DRM_DEBUG_KMS("Plane %d.%d: blocks required = %u/%u\n",
+					crtc->pipe,
+					drm_plane_index(&plane->base), wm, max_wm);
+				return -EINVAL;
+			}
 
 			if (wm > wm_state->fifo_size[wm_plane_id(plane)])
 				break;
-- 
1.8.0.1

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

[RFC 5/8] drm/i915/vlv: Change to use intel_crtc_state instead of base CRTC object

[Chi Ding]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

This commit changs some functions to operate on intel_crtc_state rather
than the base CRTC objects in order to transit to atomic. The reason we
want to do this is to allow future patches to move the computation steps
into the atomic 'check' phase where they'll be operating on in-flight CRTC
states rather than already-committed states.

v2: make the change for vlv_compute_wm_level() which is forgotten in v1

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Chi Ding <chix.ding@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 3fb896a..963b919 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -972,7 +972,7 @@ static void vlv_setup_wm_latency(struct drm_device *dev)
 }
 
 static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
-				     struct intel_crtc *crtc,
+					const struct intel_crtc_state *cstate,
 				     const struct intel_plane_state *state,
 				     int level)
 {
@@ -986,9 +986,9 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
 		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;
+	clock = cstate->base.adjusted_mode.crtc_clock;
+	htotal = cstate->base.adjusted_mode.crtc_htotal;
+	width = cstate->pipe_src_w;
 	if (WARN_ON(htotal == 0))
 		htotal = 1;
 
@@ -1008,8 +1008,9 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
 	return min_t(int, wm, USHRT_MAX);
 }
 
-static void vlv_compute_fifo(struct intel_crtc *crtc)
+static void vlv_compute_fifo(struct intel_crtc_state *cstate)
 {
+	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
 	struct vlv_wm_state *wm_state = &crtc->wm_state;
 	struct drm_device *dev = crtc->base.dev;
 	struct intel_plane *plane;
@@ -1113,8 +1114,9 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
 	}
 }
 
-static int vlv_compute_wm(struct intel_crtc *crtc)
+static int vlv_compute_wm(struct intel_crtc_state *cstate)
 {
+	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	struct vlv_wm_state *wm_state = &crtc->wm_state;
 	struct intel_plane *plane;
@@ -1128,7 +1130,7 @@ static int vlv_compute_wm(struct intel_crtc *crtc)
 
 	wm_state->num_active_planes = 0;
 
-	vlv_compute_fifo(crtc);
+	vlv_compute_fifo(cstate);
 
 	if (wm_state->num_active_planes != 1)
 		wm_state->cxsr = false;
@@ -1149,7 +1151,7 @@ static int 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(plane, cstate, state, level);
 			int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
 
 			if (level == 0 && wm > max_wm) {
@@ -1353,7 +1355,7 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 	enum pipe pipe = intel_crtc->pipe;
 	struct vlv_wm_values wm = {};
 
-	vlv_compute_wm(intel_crtc);
+	vlv_compute_wm(intel_crtc->config);
 	vlv_merge_wm(dev, &wm);
 
 	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
-- 
1.8.0.1

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

[RFC 6/8] drm/i915/vlv: Add optimal field in intel_crtc_wm_state

[Chi Ding]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

For two-stage watermark programming, we need to calculate optimal
watermark which is set after vblank and intermediate watermark which
can be set without waiting for vblank.

This commit adds optimal watermark field and changes the code to use it
in vlv_compute_wm(), vlv_update_wm() and vlv_wm_get_hw_state()

v2:
- use mutex in vlv_update_wm to make assigning currently active wm_state
and merging multiple wm_state become one atomic operation
- change vlv_compute_fifo and vlv_invert_wms to pass wm_state as parameter

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Chi Ding <chix.ding@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 23 ++++++++++++++---------
 drivers/gpu/drm/i915/intel_pm.c  | 24 ++++++++++++++++--------
 2 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ae3e73e..ac0124c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -413,6 +413,16 @@ struct skl_pipe_wm {
 	uint32_t linetime;
 };
 
+struct vlv_wm_state {
+	struct vlv_pipe_wm wm[3];
+	struct vlv_sr_wm sr[3];
+	uint16_t fifo_size[I915_MAX_PLANES];
+	uint8_t num_active_planes;
+	uint8_t num_levels;
+	uint8_t level;
+	bool cxsr;
+};
+
 struct intel_crtc_wm_state {
 	union {
 		struct {
@@ -433,6 +443,10 @@ struct intel_crtc_wm_state {
 		} ilk;
 
 		struct {
+			struct vlv_wm_state optimal;
+		} vlv;
+
+		struct {
 			/* gen9+ only needs 1-step wm programming */
 			struct skl_pipe_wm optimal;
 
@@ -620,15 +634,6 @@ struct intel_crtc_state {
 	uint32_t gamma_mode;
 };
 
-struct vlv_wm_state {
-	struct vlv_pipe_wm wm[3];
-	struct vlv_sr_wm sr[3];
-	uint16_t fifo_size[I915_MAX_PLANES];
-	uint8_t num_active_planes;
-	uint8_t num_levels;
-	uint8_t level;
-	bool cxsr;
-};
 
 struct intel_crtc {
 	struct drm_crtc base;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 963b919..04265ea 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1008,10 +1008,10 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
 	return min_t(int, wm, USHRT_MAX);
 }
 
-static void vlv_compute_fifo(struct intel_crtc_state *cstate)
+static void vlv_compute_fifo(struct intel_crtc_state *cstate,
+				struct vlv_wm_state *wm_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
-	struct vlv_wm_state *wm_state = &crtc->wm_state;
 	struct drm_device *dev = crtc->base.dev;
 	struct intel_plane *plane;
 	unsigned int total_rate = 0;
@@ -1075,9 +1075,9 @@ static void vlv_compute_fifo(struct intel_crtc_state *cstate)
 	WARN_ON(fifo_left != 0);
 }
 
-static void vlv_invert_wms(struct intel_crtc *crtc)
+static void vlv_invert_wms(struct intel_crtc *crtc,
+			struct vlv_wm_state *wm_state)
 {
-	struct vlv_wm_state *wm_state = &crtc->wm_state;
 	int level;
 
 	for (level = 0; level < wm_state->num_levels; level++) {
@@ -1118,7 +1118,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
 {
 	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
-	struct vlv_wm_state *wm_state = &crtc->wm_state;
+	struct vlv_wm_state *wm_state = &cstate->wm.vlv.optimal;
 	struct intel_plane *plane;
 	int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
 	int level;
@@ -1130,7 +1130,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
 
 	wm_state->num_active_planes = 0;
 
-	vlv_compute_fifo(cstate);
+	vlv_compute_fifo(cstate, wm_state);
 
 	if (wm_state->num_active_planes != 1)
 		wm_state->cxsr = false;
@@ -1216,7 +1216,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
 		memset(&wm_state->sr[level], 0, sizeof(wm_state->sr[level]));
 	}
 
-	vlv_invert_wms(crtc);
+	vlv_invert_wms(crtc, wm_state);
 
 	return 0;
 }
@@ -1356,7 +1356,10 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 	struct vlv_wm_values wm = {};
 
 	vlv_compute_wm(intel_crtc->config);
+	mutex_lock(&dev_priv->wm.wm_mutex);
+	intel_crtc->wm_state = intel_crtc->config->wm.vlv.optimal;
 	vlv_merge_wm(dev, &wm);
+	mutex_unlock(&dev_priv->wm.wm_mutex);
 
 	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
 		/* FIXME should be part of crtc atomic commit */
@@ -4420,10 +4423,15 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
 		mutex_unlock(&dev_priv->rps.hw_lock);
 	}
 
-	for_each_pipe(dev_priv, pipe)
+	for_each_intel_crtc(dev, crtc) {
+		pipe = crtc->pipe;
+		to_intel_crtc_state(crtc->base.state)->wm.vlv.optimal
+			= crtc->wm_state;
+
 		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]);
+	}
 
 	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);
-- 
1.8.0.1

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

[RFC 7/8] drm/i915/vlv: Move active watermarks into intel_crtc->wm.active.vlv

[Chi Ding]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Previously the active watermarks is saved in intel_crtc->wm_state
This commit adds a new field "vlv" into intel_crtc->wm.active and save
the active watermarks in it to be consistent with what we do on other
platforms.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Chi Ding <chix.ding@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 12 ++++++------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ac0124c..8e77adb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -680,6 +680,7 @@ struct intel_crtc {
 		union {
 			struct intel_pipe_wm ilk;
 			struct skl_pipe_wm skl;
+			struct vlv_wm_state vlv;
 		} active;
 
 		/* allow CxSR on this pipe */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 04265ea..cc43c1e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1229,7 +1229,7 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	int sprite0_start = 0, sprite1_start = 0, fifo_size = 0;
-	const struct vlv_wm_state *wm_state = &crtc->wm_state;
+	const struct vlv_wm_state *wm_state = &crtc->wm.active.vlv;
 
 
 	WARN_ON(wm_state->fifo_size[PLANE_CURSOR] != 63);
@@ -1311,7 +1311,7 @@ static void vlv_merge_wm(struct drm_device *dev,
 	wm->cxsr = true;
 
 	for_each_intel_crtc(dev, crtc) {
-		const struct vlv_wm_state *wm_state = &crtc->wm_state;
+		const struct vlv_wm_state *wm_state = &crtc->wm.active.vlv;
 
 		if (!crtc->active)
 			continue;
@@ -1330,7 +1330,7 @@ static void vlv_merge_wm(struct drm_device *dev,
 		wm->level = VLV_WM_LEVEL_PM2;
 
 	for_each_intel_crtc(dev, crtc) {
-		struct vlv_wm_state *wm_state = &crtc->wm_state;
+		struct vlv_wm_state *wm_state = &crtc->wm.active.vlv;
 		enum pipe pipe = crtc->pipe;
 
 		if (!crtc->active)
@@ -1357,7 +1357,7 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 
 	vlv_compute_wm(intel_crtc->config);
 	mutex_lock(&dev_priv->wm.wm_mutex);
-	intel_crtc->wm_state = intel_crtc->config->wm.vlv.optimal;
+	intel_crtc->wm.active.vlv = intel_crtc->config->wm.vlv.optimal;
 	vlv_merge_wm(dev, &wm);
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 
@@ -4372,7 +4372,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
 		int i = wm_plane_id(plane);
 
 		crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, plane->pipe));
-		wm_state = &crtc->wm_state;
+		wm_state = &crtc->wm.active.vlv;
 
 		switch (plane->base.type) {
 		case DRM_PLANE_TYPE_CURSOR:
@@ -4426,7 +4426,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
 	for_each_intel_crtc(dev, crtc) {
 		pipe = crtc->pipe;
 		to_intel_crtc_state(crtc->base.state)->wm.vlv.optimal
-			= crtc->wm_state;
+			= crtc->wm.active.vlv;
 
 		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,
-- 
1.8.0.1

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

[RFC 8/8] drm/i915/vlv: Add intermediate field in intel_crtc_wm_state and handlers for two-level watermark

[Chi Ding]

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Rename vlv_compute_wm to vlv_compute_pipe_wm to compute optimal watermark
Add vlv_compute_intermediate_wm to computer intermediate watermark
Add vlv_initial_watermarks to write intermediate watermark into hardware
Add vlv_optimize_watermarks to write optimal watermark into hardware
Change valleyview_crtc_enable to call .initial_watermarks handler

This patch adds the handlers for two-level atomic watermark for VLV/CHV.
It makes use of the optimal and intermediate watermark fields added in
the previous commits to calculate the optimal and intermediate state.
It sets the intermediate watermark which is the safer value of the
currently active and the optimal watermark pre-vblank. Then it sets the
optimal watermark after-vblank.

v2:
- use macro drm_atomic_crtc_state_for_each_plane_state
- remove redundant debug statements in vlv_pipe_set_fifo_size

v3:
- use macro drm_atomic_crtc_state_for_each_plane_state to simplify the code
- check !new_state->active || modeset in computing intermediate watermark

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Chi Ding <chix.ding@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   8 +-
 drivers/gpu/drm/i915/intel_drv.h     |   3 +-
 drivers/gpu/drm/i915/intel_pm.c      | 150 +++++++++++++++++++++++++++--------
 3 files changed, 121 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1141b86..1ceda24 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4602,8 +4602,6 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 
 	intel_frontbuffer_flip(dev, pipe_config->fb_bits);
 
-	crtc->wm.cxsr_allowed = true;
-
 	if (pipe_config->update_wm_post && pipe_config->base.active)
 		intel_update_watermarks(&crtc->base);
 
@@ -4649,8 +4647,6 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 	}
 
 	if (pipe_config->disable_cxsr && HAS_GMCH_DISPLAY(dev)) {
-		crtc->wm.cxsr_allowed = false;
-
 		/*
 		 * Vblank time updates from the shadow to live plane control register
 		 * are blocked if the memory self-refresh mode is active at that
@@ -6180,7 +6176,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 
 	intel_color_load_luts(&pipe_config->base);
 
-	intel_update_watermarks(crtc);
+	dev_priv->display.initial_watermarks(pipe_config);
 	intel_enable_pipe(intel_crtc);
 
 	assert_vblank_disabled(crtc);
@@ -14591,8 +14587,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	intel_crtc->cursor_cntl = ~0;
 	intel_crtc->cursor_size = ~0;
 
-	intel_crtc->wm.cxsr_allowed = true;
-
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8e77adb..30a6304 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -443,6 +443,7 @@ struct intel_crtc_wm_state {
 		} ilk;
 
 		struct {
+			struct vlv_wm_state intermediate;
 			struct vlv_wm_state optimal;
 		} vlv;
 
@@ -683,8 +684,6 @@ struct intel_crtc {
 			struct vlv_wm_state vlv;
 		} active;
 
-		/* allow CxSR on this pipe */
-		bool cxsr_allowed;
 	} wm;
 
 	int scanline_offset;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index cc43c1e..9a5e6e9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -31,6 +31,7 @@
 #include "intel_drv.h"
 #include "../../../platform/x86/intel_ips.h"
 #include <linux/module.h>
+#include <drm/drm_atomic_helper.h>
 
 /**
  * DOC: RC6
@@ -989,6 +990,7 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
 	clock = cstate->base.adjusted_mode.crtc_clock;
 	htotal = cstate->base.adjusted_mode.crtc_htotal;
 	width = cstate->pipe_src_w;
+
 	if (WARN_ON(htotal == 0))
 		htotal = 1;
 
@@ -1011,27 +1013,28 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
 static void vlv_compute_fifo(struct intel_crtc_state *cstate,
 				struct vlv_wm_state *wm_state)
 {
-	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
-	struct drm_device *dev = crtc->base.dev;
-	struct intel_plane *plane;
 	unsigned int total_rate = 0;
 	const int fifo_size = 512 - 1;
 	int fifo_extra, fifo_left = fifo_size;
 	int rate[I915_MAX_PLANES] = {};
 	int i;
+	const struct drm_plane_state *pstate;
+	struct drm_plane *plane;
 
-	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
 		struct intel_plane_state *state =
-			to_intel_plane_state(plane->base.state);
+			to_intel_plane_state(pstate);
+		struct intel_plane *intel_plane =
+			to_intel_plane(plane);
 
-		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
+		if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
 			continue;
 
 		if (state->visible) {
 			wm_state->num_active_planes++;
-			rate[wm_plane_id(plane)] =
+			rate[wm_plane_id(intel_plane)] =
 			drm_format_plane_cpp(state->base.fb->pixel_format, 0);
-			total_rate += rate[wm_plane_id(plane)];
+			total_rate += rate[wm_plane_id(intel_plane)];
 		}
 	}
 
@@ -1114,18 +1117,19 @@ static void vlv_invert_wms(struct intel_crtc *crtc,
 	}
 }
 
-static int vlv_compute_wm(struct intel_crtc_state *cstate)
+static int vlv_compute_pipe_wm(struct intel_crtc_state *cstate)
 {
 	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
 	struct drm_device *dev = crtc->base.dev;
 	struct vlv_wm_state *wm_state = &cstate->wm.vlv.optimal;
-	struct intel_plane *plane;
+	struct drm_plane *plane;
 	int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
 	int level;
+	const struct drm_plane_state *pstate;
 
 	memset(wm_state, 0, sizeof(*wm_state));
 
-	wm_state->cxsr = crtc->pipe != PIPE_C && crtc->wm.cxsr_allowed;
+	wm_state->cxsr = crtc->pipe != PIPE_C;
 	wm_state->num_levels = to_i915(dev)->wm.max_level + 1;
 
 	wm_state->num_active_planes = 0;
@@ -1142,31 +1146,33 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
 		}
 	}
 
-	for_each_intel_plane_on_crtc(dev, crtc, plane) {
+	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
 		struct intel_plane_state *state =
-			to_intel_plane_state(plane->base.state);
+			to_intel_plane_state(pstate);
+		struct intel_plane *intel_plane =
+			to_intel_plane(plane);
 
 		if (!state->visible)
 			continue;
 
 		/* normal watermarks */
 		for (level = 0; level < wm_state->num_levels; level++) {
-			int wm = vlv_compute_wm_level(plane, cstate, state, level);
-			int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
+			int wm = vlv_compute_wm_level(intel_plane, cstate, state, level);
+			int max_wm = intel_plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
 
 			if (level == 0 && wm > max_wm) {
 				DRM_DEBUG_KMS("Requested display configuration "
 				"exceeds system watermark limitations\n");
 				DRM_DEBUG_KMS("Plane %d.%d: blocks required = %u/%u\n",
-					crtc->pipe,
-					drm_plane_index(&plane->base), wm, max_wm);
+				      crtc->pipe,
+				      drm_plane_index(&intel_plane->base), wm, max_wm);
 				return -EINVAL;
 			}
 
-			if (wm > wm_state->fifo_size[wm_plane_id(plane)])
+			if (wm > wm_state->fifo_size[wm_plane_id(intel_plane)])
 				break;
 
-			switch (plane->base.type) {
+			switch (intel_plane->base.type) {
 				int sprite;
 			case DRM_PLANE_TYPE_CURSOR:
 				wm_state->wm[level].cursor = wm;
@@ -1175,7 +1181,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
 				wm_state->wm[level].primary = wm;
 				break;
 			case DRM_PLANE_TYPE_OVERLAY:
-				sprite = plane->plane;
+				sprite = intel_plane->plane;
 				wm_state->wm[level].sprite[sprite] = wm;
 				break;
 			}
@@ -1187,7 +1193,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
 			continue;
 
 		/* maxfifo watermarks */
-		switch (plane->base.type) {
+		switch (intel_plane->base.type) {
 			int sprite, level;
 		case DRM_PLANE_TYPE_CURSOR:
 			for (level = 0; level < wm_state->num_levels; level++)
@@ -1201,7 +1207,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
 					    wm_state->wm[level].primary);
 			break;
 		case DRM_PLANE_TYPE_OVERLAY:
-			sprite = plane->plane;
+			sprite = intel_plane->plane;
 			for (level = 0; level < wm_state->num_levels; level++)
 				wm_state->sr[level].plane =
 					min(wm_state->sr[level].plane,
@@ -1221,6 +1227,65 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
 	return 0;
 }
 
+
+static int vlv_compute_intermediate_wm(struct drm_device *dev,
+				       struct intel_crtc *intel_crtc,
+				       struct intel_crtc_state *newstate)
+{
+	struct vlv_wm_state *a = &newstate->wm.vlv.intermediate;
+	struct vlv_wm_state *b = &intel_crtc->wm.active.vlv;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int level, max_level = dev_priv->wm.max_level;
+
+	/*
+	 * Start with the final, target watermarks, then combine with the
+	 * currently active watermarks to get values that are safe both before
+	 * and after the vblank.
+	 */
+	*a = newstate->wm.vlv.optimal;
+
+	if (!newstate->base.active || drm_atomic_crtc_needs_modeset(&newstate->base))
+		return 0;
+
+	a->num_levels = min(a->num_levels, b->num_levels);
+	a->cxsr &= b->cxsr;
+
+	for (level = 0; level < a->num_levels; level++) {
+		struct vlv_pipe_wm *a_wm = &a->wm[level];
+		const struct vlv_pipe_wm *b_wm = &b->wm[level];
+		struct vlv_sr_wm *a_sr = &a->sr[level];
+		const struct vlv_sr_wm *b_sr = &b->sr[level];
+
+		a_wm->primary = min(a_wm->primary, b_wm->primary);
+		a_wm->sprite[0] = min(a_wm->sprite[0], b_wm->sprite[0]);
+		a_wm->sprite[1] = min(a_wm->sprite[1], b_wm->sprite[1]);
+		a_wm->cursor = min(a_wm->cursor, b_wm->cursor);
+
+		if (a->cxsr) {
+			a_sr->plane = min(a_sr->plane, b_sr->plane);
+			a_sr->cursor = min(a_sr->cursor, b_sr->cursor);
+		} else {
+			a_sr->plane = b_sr->plane;
+			a_sr->cursor = b_sr->cursor;
+		}
+	}
+
+	/* reset the invalid levels */
+	for (level = a->num_levels; level <= max_level; level++) {
+		memset(&a->wm[level], 0, sizeof(struct vlv_pipe_wm));
+		memset(&a->sr[level], 0, sizeof(struct vlv_sr_wm));
+	}
+
+	/*
+	 * If our intermediate WM are identical to the final WM, then we can
+	 * omit the post-vblank programming; only update if it's different.
+	 */
+	if (memcmp(a, &newstate->wm.ilk.optimal, sizeof(*a)) == 0)
+		newstate->wm.need_postvbl_update = false;
+
+	return 0;
+}
+
 #define VLV_FIFO(plane, value) \
 	(((value) << DSPARB_ ## plane ## _SHIFT_VLV) & DSPARB_ ## plane ## _MASK_VLV)
 
@@ -1355,11 +1420,7 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 	enum pipe pipe = intel_crtc->pipe;
 	struct vlv_wm_values wm = {};
 
-	vlv_compute_wm(intel_crtc->config);
-	mutex_lock(&dev_priv->wm.wm_mutex);
-	intel_crtc->wm.active.vlv = intel_crtc->config->wm.vlv.optimal;
 	vlv_merge_wm(dev, &wm);
-	mutex_unlock(&dev_priv->wm.wm_mutex);
 
 	if (memcmp(&dev_priv->wm.vlv, &wm, sizeof(wm)) == 0) {
 		/* FIXME should be part of crtc atomic commit */
@@ -1403,6 +1464,29 @@ static void vlv_update_wm(struct drm_crtc *crtc)
 	dev_priv->wm.vlv = wm;
 }
 
+
+static void vlv_initial_watermarks(struct intel_crtc_state *cstate)
+{
+	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
+
+	mutex_lock(&dev_priv->wm.wm_mutex);
+	intel_crtc->wm.active.vlv = cstate->wm.vlv.intermediate;
+	vlv_update_wm(cstate->base.crtc);
+	mutex_unlock(&dev_priv->wm.wm_mutex);
+}
+
+static void vlv_optimize_watermarks(struct intel_crtc_state *cstate)
+{
+	struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
+
+	mutex_lock(&dev_priv->wm.wm_mutex);
+	intel_crtc->wm.active.vlv = cstate->wm.vlv.optimal;
+	vlv_update_wm(cstate->base.crtc);
+	mutex_unlock(&dev_priv->wm.wm_mutex);
+}
+
 #define single_plane_enabled(mask) is_power_of_2(mask)
 
 static void g4x_update_wm(struct drm_crtc *crtc)
@@ -7581,12 +7665,16 @@ void intel_init_pm(struct drm_device *dev)
 			DRM_DEBUG_KMS("Failed to read display plane latency. "
 				      "Disable CxSR\n");
 		}
-	} else if (IS_CHERRYVIEW(dev)) {
-		vlv_setup_wm_latency(dev);
-		dev_priv->display.update_wm = vlv_update_wm;
-	} else if (IS_VALLEYVIEW(dev)) {
+	} else if (IS_CHERRYVIEW(dev) || IS_VALLEYVIEW(dev)) {
 		vlv_setup_wm_latency(dev);
-		dev_priv->display.update_wm = vlv_update_wm;
+		dev_priv->display.compute_pipe_wm =
+			vlv_compute_pipe_wm;
+		dev_priv->display.compute_intermediate_wm =
+			vlv_compute_intermediate_wm;
+		dev_priv->display.initial_watermarks =
+			vlv_initial_watermarks;
+		dev_priv->display.optimize_watermarks =
+			vlv_optimize_watermarks;
 	} else if (IS_PINEVIEW(dev)) {
 		if (!intel_get_cxsr_latency(IS_PINEVIEW_G(dev),
 					    dev_priv->is_ddr3,
-- 
1.8.0.1

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

✗ Ro.CI.BAT: failure for Add two-stage watermark programming for VLV/CHV (v5)

[Patchwork]

== Series Details ==

Series: Add two-stage watermark programming for VLV/CHV (v5)
URL   : https://patchwork.freedesktop.org/series/9079/
State : failure

== Summary ==

Series 9079v1 Add two-stage watermark programming for VLV/CHV (v5)
http://patchwork.freedesktop.org/api/1.0/series/9079/revisions/1/mbox

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i7-5557U)
        Subgroup suspend-read-crc-pipe-c:
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
Test vgem_basic:
        Subgroup create:
                skip       -> PASS       (ro-bdw-i7-5557U)
                skip       -> PASS       (ro-ilk1-i5-650)
                skip       -> PASS       (ro-hsw-i3-4010u)
                skip       -> PASS       (ro-bdw-i7-5600u)
                skip       -> PASS       (ro-bdw-i5-5250u)
                skip       -> PASS       (ro-hsw-i7-4770r)
                skip       -> PASS       (ro-ivb-i7-3770)
                skip       -> PASS       (ro-ivb2-i7-3770)
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup debugfs:
                skip       -> PASS       (ro-bdw-i7-5557U)
                skip       -> PASS       (ro-ilk1-i5-650)
                skip       -> PASS       (ro-hsw-i3-4010u)
                skip       -> PASS       (ro-bdw-i7-5600u)
                skip       -> PASS       (ro-bdw-i5-5250u)
                skip       -> PASS       (ro-hsw-i7-4770r)
                skip       -> PASS       (ro-ivb-i7-3770)
                skip       -> PASS       (ro-ivb2-i7-3770)
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup mmap:
                skip       -> DMESG-FAIL (ro-bdw-i7-5557U)
                skip       -> DMESG-FAIL (ro-ilk1-i5-650)
                skip       -> DMESG-FAIL (ro-hsw-i3-4010u)
                skip       -> DMESG-FAIL (ro-bdw-i7-5600u)
                skip       -> DMESG-FAIL (ro-bdw-i5-5250u)
                skip       -> DMESG-FAIL (ro-hsw-i7-4770r)
                skip       -> DMESG-FAIL (ro-ivb-i7-3770)
                skip       -> DMESG-FAIL (ro-ivb2-i7-3770)
                skip       -> DMESG-FAIL (ro-skl3-i5-6260u)
        Subgroup sysfs:
                skip       -> PASS       (ro-bdw-i7-5557U)
                skip       -> PASS       (ro-ilk1-i5-650)
                skip       -> PASS       (ro-hsw-i3-4010u)
                skip       -> PASS       (ro-bdw-i7-5600u)
                skip       -> PASS       (ro-bdw-i5-5250u)
                skip       -> PASS       (ro-hsw-i7-4770r)
                skip       -> PASS       (ro-ivb-i7-3770)
                skip       -> PASS       (ro-ivb2-i7-3770)
                skip       -> PASS       (ro-skl3-i5-6260u)
Test vgem_reload_basic:
                skip       -> PASS       (ro-bdw-i7-5557U)
                skip       -> PASS       (ro-ilk1-i5-650)
                skip       -> PASS       (ro-hsw-i3-4010u)
                skip       -> PASS       (ro-bdw-i7-5600u)
                skip       -> PASS       (ro-bdw-i5-5250u)
                skip       -> PASS       (ro-hsw-i7-4770r)
                skip       -> PASS       (ro-ivb-i7-3770)
                skip       -> PASS       (ro-ivb2-i7-3770)
                skip       -> PASS       (ro-skl3-i5-6260u)

fi-kbl-qkkr      total:227  pass:161  dwarn:28  dfail:0   fail:0   skip:38 
fi-skl-i5-6260u  total:227  pass:202  dwarn:0   dfail:0   fail:1   skip:24 
fi-skl-i7-6700k  total:227  pass:188  dwarn:0   dfail:0   fail:1   skip:38 
fi-snb-i7-2600   total:227  pass:174  dwarn:0   dfail:0   fail:1   skip:52 
ro-bdw-i5-5250u  total:227  pass:201  dwarn:3   dfail:1   fail:1   skip:21 
ro-bdw-i7-5557U  total:227  pass:202  dwarn:0   dfail:1   fail:1   skip:23 
ro-bdw-i7-5600u  total:227  pass:189  dwarn:0   dfail:1   fail:0   skip:37 
ro-bsw-n3050     total:227  pass:172  dwarn:4   dfail:1   fail:2   skip:48 
ro-hsw-i3-4010u  total:227  pass:194  dwarn:0   dfail:1   fail:1   skip:31 
ro-hsw-i7-4770r  total:227  pass:194  dwarn:0   dfail:1   fail:1   skip:31 
ro-ilk1-i5-650   total:222  pass:154  dwarn:0   dfail:1   fail:2   skip:65 
ro-ivb-i7-3770   total:227  pass:185  dwarn:0   dfail:1   fail:1   skip:40 
ro-ivb2-i7-3770  total:227  pass:189  dwarn:0   dfail:1   fail:1   skip:36 
ro-skl3-i5-6260u total:227  pass:205  dwarn:1   dfail:1   fail:1   skip:19 
ro-byt-n2820 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1286/

8e5ac92 drm-intel-nightly: 2016y-06m-22d-18h-10m-30s UTC integration manifest
fb5a3c7 drm/i915/vlv: Add intermediate field in intel_crtc_wm_state and handlers for two-level watermark
8ca5cba drm/i915/vlv: Move active watermarks into intel_crtc->wm.active.vlv
b7c4ad9 drm/i915/vlv: Add optimal field in intel_crtc_wm_state
f433618 drm/i915/vlv: Change to use intel_crtc_state instead of base CRTC object
67b6880 drm/i915/vlv: return EINVAL when computed watermark exceeds system limitation
5bd0371 drm/i915/vlv: Move fifo_size from intel_plane_wm_parameters to vlv_wm_state
b671d3c drm/i915: Rename skl_wm_plane_id to wm_plane_id
a5233ca drm/i915: Remove unused parameters from intel_plane_wm_parameters

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

Re: [RFC 8/8] drm/i915/vlv: Add intermediate field in intel_crtc_wm_state and handlers for two-level watermark

[Maarten Lankhorst]

Op 23-06-16 om 14:36 schreef Chi Ding:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> Rename vlv_compute_wm to vlv_compute_pipe_wm to compute optimal watermark
> Add vlv_compute_intermediate_wm to computer intermediate watermark
> Add vlv_initial_watermarks to write intermediate watermark into hardware
> Add vlv_optimize_watermarks to write optimal watermark into hardware
> Change valleyview_crtc_enable to call .initial_watermarks handler
>
> This patch adds the handlers for two-level atomic watermark for VLV/CHV.
> It makes use of the optimal and intermediate watermark fields added in
> the previous commits to calculate the optimal and intermediate state.
> It sets the intermediate watermark which is the safer value of the
> currently active and the optimal watermark pre-vblank. Then it sets the
> optimal watermark after-vblank.
>
> v2:
> - use macro drm_atomic_crtc_state_for_each_plane_state
> - remove redundant debug statements in vlv_pipe_set_fifo_size
>
> v3:
> - use macro drm_atomic_crtc_state_for_each_plane_state to simplify the code
> - check !new_state->active || modeset in computing intermediate watermark
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Chi Ding <chix.ding@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   8 +-
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 150 +++++++++++++++++++++++++++--------
>  3 files changed, 121 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1141b86..1ceda24 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4602,8 +4602,6 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  
>  	intel_frontbuffer_flip(dev, pipe_config->fb_bits);
>  
> -	crtc->wm.cxsr_allowed = true;
> -
>  	if (pipe_config->update_wm_post && pipe_config->base.active)
>  		intel_update_watermarks(&crtc->base);
>  
> @@ -4649,8 +4647,6 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
>  	}
>  
>  	if (pipe_config->disable_cxsr && HAS_GMCH_DISPLAY(dev)) {
> -		crtc->wm.cxsr_allowed = false;
> -
>  		/*
>  		 * Vblank time updates from the shadow to live plane control register
>  		 * are blocked if the memory self-refresh mode is active at that
> @@ -6180,7 +6176,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  
>  	intel_color_load_luts(&pipe_config->base);
>  
> -	intel_update_watermarks(crtc);
> +	dev_priv->display.initial_watermarks(pipe_config);
>  	intel_enable_pipe(intel_crtc);
>  
>  	assert_vblank_disabled(crtc);
> @@ -14591,8 +14587,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	intel_crtc->cursor_cntl = ~0;
>  	intel_crtc->cursor_size = ~0;
>  
> -	intel_crtc->wm.cxsr_allowed = true;
> -
>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 8e77adb..30a6304 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -443,6 +443,7 @@ struct intel_crtc_wm_state {
>  		} ilk;
>  
>  		struct {
> +			struct vlv_wm_state intermediate;
>  			struct vlv_wm_state optimal;
>  		} vlv;
>  
> @@ -683,8 +684,6 @@ struct intel_crtc {
>  			struct vlv_wm_state vlv;
>  		} active;
>  
> -		/* allow CxSR on this pipe */
> -		bool cxsr_allowed;
>  	} wm;
>  
>  	int scanline_offset;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index cc43c1e..9a5e6e9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -31,6 +31,7 @@
>  #include "intel_drv.h"
>  #include "../../../platform/x86/intel_ips.h"
>  #include <linux/module.h>
> +#include <drm/drm_atomic_helper.h>
>  
>  /**
>   * DOC: RC6
> @@ -989,6 +990,7 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
>  	clock = cstate->base.adjusted_mode.crtc_clock;
>  	htotal = cstate->base.adjusted_mode.crtc_htotal;
>  	width = cstate->pipe_src_w;
> +
>  	if (WARN_ON(htotal == 0))
>  		htotal = 1;
>  
> @@ -1011,27 +1013,28 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
>  static void vlv_compute_fifo(struct intel_crtc_state *cstate,
>  				struct vlv_wm_state *wm_state)
>  {
> -	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
> -	struct drm_device *dev = crtc->base.dev;
> -	struct intel_plane *plane;
>  	unsigned int total_rate = 0;
>  	const int fifo_size = 512 - 1;
>  	int fifo_extra, fifo_left = fifo_size;
>  	int rate[I915_MAX_PLANES] = {};
>  	int i;
> +	const struct drm_plane_state *pstate;
> +	struct drm_plane *plane;
>  
> -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
>  		struct intel_plane_state *state =
> -			to_intel_plane_state(plane->base.state);
> +			to_intel_plane_state(pstate);
> +		struct intel_plane *intel_plane =
> +			to_intel_plane(plane);
>  
> -		if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> +		if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
>  			continue;
>  
>  		if (state->visible) {
>  			wm_state->num_active_planes++;
> -			rate[wm_plane_id(plane)] =
> +			rate[wm_plane_id(intel_plane)] =
>  			drm_format_plane_cpp(state->base.fb->pixel_format, 0);
> -			total_rate += rate[wm_plane_id(plane)];
> +			total_rate += rate[wm_plane_id(intel_plane)];
>  		}
>  	}
>  
> @@ -1114,18 +1117,19 @@ static void vlv_invert_wms(struct intel_crtc *crtc,
>  	}
>  }
>  
> -static int vlv_compute_wm(struct intel_crtc_state *cstate)
> +static int vlv_compute_pipe_wm(struct intel_crtc_state *cstate)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(cstate->base.crtc);
>  	struct drm_device *dev = crtc->base.dev;
>  	struct vlv_wm_state *wm_state = &cstate->wm.vlv.optimal;
> -	struct intel_plane *plane;
> +	struct drm_plane *plane;
>  	int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
>  	int level;
> +	const struct drm_plane_state *pstate;
>  
>  	memset(wm_state, 0, sizeof(*wm_state));
>  
> -	wm_state->cxsr = crtc->pipe != PIPE_C && crtc->wm.cxsr_allowed;
> +	wm_state->cxsr = crtc->pipe != PIPE_C;
>  	wm_state->num_levels = to_i915(dev)->wm.max_level + 1;
>  
>  	wm_state->num_active_planes = 0;
> @@ -1142,31 +1146,33 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
>  		}
>  	}
>  
> -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, &cstate->base) {
>  		struct intel_plane_state *state =
> -			to_intel_plane_state(plane->base.state);
> +			to_intel_plane_state(pstate);
> +		struct intel_plane *intel_plane =
> +			to_intel_plane(plane);
>  
>  		if (!state->visible)
>  			continue;
>  
>  		/* normal watermarks */
>  		for (level = 0; level < wm_state->num_levels; level++) {
> -			int wm = vlv_compute_wm_level(plane, cstate, state, level);
> -			int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
> +			int wm = vlv_compute_wm_level(intel_plane, cstate, state, level);
> +			int max_wm = intel_plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
>  
>  			if (level == 0 && wm > max_wm) {
>  				DRM_DEBUG_KMS("Requested display configuration "
>  				"exceeds system watermark limitations\n");
>  				DRM_DEBUG_KMS("Plane %d.%d: blocks required = %u/%u\n",
> -					crtc->pipe,
> -					drm_plane_index(&plane->base), wm, max_wm);
> +				      crtc->pipe,
> +				      drm_plane_index(&intel_plane->base), wm, max_wm);
>  				return -EINVAL;
>  			}
>  
> -			if (wm > wm_state->fifo_size[wm_plane_id(plane)])
> +			if (wm > wm_state->fifo_size[wm_plane_id(intel_plane)])
>  				break;
>  
> -			switch (plane->base.type) {
> +			switch (intel_plane->base.type) {
>  				int sprite;
>  			case DRM_PLANE_TYPE_CURSOR:
>  				wm_state->wm[level].cursor = wm;
> @@ -1175,7 +1181,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
>  				wm_state->wm[level].primary = wm;
>  				break;
>  			case DRM_PLANE_TYPE_OVERLAY:
> -				sprite = plane->plane;
> +				sprite = intel_plane->plane;
>  				wm_state->wm[level].sprite[sprite] = wm;
>  				break;
>  			}
> @@ -1187,7 +1193,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
>  			continue;
>  
>  		/* maxfifo watermarks */
> -		switch (plane->base.type) {
> +		switch (intel_plane->base.type) {
>  			int sprite, level;
>  		case DRM_PLANE_TYPE_CURSOR:
>  			for (level = 0; level < wm_state->num_levels; level++)
> @@ -1201,7 +1207,7 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
>  					    wm_state->wm[level].primary);
>  			break;
>  		case DRM_PLANE_TYPE_OVERLAY:
> -			sprite = plane->plane;
> +			sprite = intel_plane->plane;
>  			for (level = 0; level < wm_state->num_levels; level++)
>  				wm_state->sr[level].plane =
>  					min(wm_state->sr[level].plane,
> @@ -1221,6 +1227,65 @@ static int vlv_compute_wm(struct intel_crtc_state *cstate)
>  	return 0;
>  }
>  
> +
> +static int vlv_compute_intermediate_wm(struct drm_device *dev,
> +				       struct intel_crtc *intel_crtc,
> +				       struct intel_crtc_state *newstate)
> +{
> +	struct vlv_wm_state *a = &newstate->wm.vlv.intermediate;
> +	struct vlv_wm_state *b = &intel_crtc->wm.active.vlv;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int level, max_level = dev_priv->wm.max_level;
> +
> +	/*
> +	 * Start with the final, target watermarks, then combine with the
> +	 * currently active watermarks to get values that are safe both before
> +	 * and after the vblank.
> +	 */
> +	*a = newstate->wm.vlv.optimal;
> +
> +	if (!newstate->base.active || drm_atomic_crtc_needs_modeset(&newstate->base))
> +		return 0;
> +
> +	a->num_levels = min(a->num_levels, b->num_levels);
> +	a->cxsr &= b->cxsr;
I think we lost a check for disable_cxsr here. This might also be required during modeset
since we don't enable planes until after the modeset, initial watermarks during modeset
probably requires that cxsr is disabled until after optimal watermarks.
> +	for (level = 0; level < a->num_levels; level++) {
> +		struct vlv_pipe_wm *a_wm = &a->wm[level];
> +		const struct vlv_pipe_wm *b_wm = &b->wm[level];
> +		struct vlv_sr_wm *a_sr = &a->sr[level];
> +		const struct vlv_sr_wm *b_sr = &b->sr[level];
> +
> +		a_wm->primary = min(a_wm->primary, b_wm->primary);
> +		a_wm->sprite[0] = min(a_wm->sprite[0], b_wm->sprite[0]);
> +		a_wm->sprite[1] = min(a_wm->sprite[1], b_wm->sprite[1]);
> +		a_wm->cursor = min(a_wm->cursor, b_wm->cursor);
Again, why min?

Don't make me write an atomic testcase that does this:

primary: set fb1,
sprite0: set null fb
commit

primary: set null fb
sprite0: set fb1
commit

In fact I will do so, it should be easy to catch FIFO underruns like that..
> +		if (a->cxsr) {
> +			a_sr->plane = min(a_sr->plane, b_sr->plane);
> +			a_sr->cursor = min(a_sr->cursor, b_sr->cursor);
> +		} else {
> +			a_sr->plane = b_sr->plane;
> +			a_sr->cursor = b_sr->cursor;
> +		}
^The else seems redundant. The values are unused when cxsr is disabled.

Rest seems ok.

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

[PATCH] drm/i915/vlv: Fix off-by-1 error in calculating num_levels.

[Maarten Lankhorst]

num_levels should be level+1, not level, else num_levels - 1 becomes
negative. This resulted in bogus watermarks being written to the first
255 levels like below:

[drm] Setting FIFO watermarks - C: plane=0, cursor=0, sprite0=0, sprite1=0, SR: plane=0, cursor=0 level=255 cxsr=0
[drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
[drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe C FIFO underrun
[drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request

Testcase: kms_atomic_transition
Fixes: 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark rewrite")
Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
Urgent fix for watermark support. This is definitely a pre-requisite for this series.
With this I've noticed that patch "[RFC 3/8] drm/i915/vlv: Move fifo_size from
intel_plane_wm_parameters to vlv_wm_state" introduces a regression with invalid FIFO split.

I need to find out what's going wrong in that patch before this series can be applied.

 drivers/gpu/drm/i915/intel_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 376c60b98515..8defdcc54529 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1148,7 +1148,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
 			}
 		}
 
-		wm_state->num_levels = level;
+		wm_state->num_levels = level + 1;
 
 		if (!wm_state->cxsr)
 			continue;
-- 
2.7.4


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

Re: [PATCH] drm/i915/vlv: Fix off-by-1 error in calculating num_levels.

[Ville Syrjälä]

On Tue, Jul 19, 2016 at 05:14:23PM +0200, Maarten Lankhorst wrote:
> num_levels should be level+1, not level, else num_levels - 1 becomes
> negative. This resulted in bogus watermarks being written to the first
> 255 levels like below:
> 
> [drm] Setting FIFO watermarks - C: plane=0, cursor=0, sprite0=0, sprite1=0, SR: plane=0, cursor=0 level=255 cxsr=0
> [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
> [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe C FIFO underrun
> [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
> 
> Testcase: kms_atomic_transition
> Fixes: 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark rewrite")
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> Urgent fix for watermark support. This is definitely a pre-requisite for this series.
> With this I've noticed that patch "[RFC 3/8] drm/i915/vlv: Move fifo_size from
> intel_plane_wm_parameters to vlv_wm_state" introduces a regression with invalid FIFO split.
> 
> I need to find out what's going wrong in that patch before this series can be applied.
> 
>  drivers/gpu/drm/i915/intel_pm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 376c60b98515..8defdcc54529 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1148,7 +1148,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>  			}
>  		}
>  
> -		wm_state->num_levels = level;
> +		wm_state->num_levels = level + 1;

Nope. The loop above breaks when the current level is bad, hence level-1
is actually the higher usable level.

>  
>  		if (!wm_state->cxsr)
>  			continue;
> -- 
> 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

Re: [PATCH] drm/i915/vlv: Fix off-by-1 error in calculating num_levels.

[Chris Wilson]

On Tue, Jul 19, 2016 at 06:25:42PM +0300, Ville Syrjälä wrote:
> On Tue, Jul 19, 2016 at 05:14:23PM +0200, Maarten Lankhorst wrote:
> > num_levels should be level+1, not level, else num_levels - 1 becomes
> > negative. This resulted in bogus watermarks being written to the first
> > 255 levels like below:
> > 
> > [drm] Setting FIFO watermarks - C: plane=0, cursor=0, sprite0=0, sprite1=0, SR: plane=0, cursor=0 level=255 cxsr=0
> > [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
> > [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe C FIFO underrun
> > [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
> > 
> > Testcase: kms_atomic_transition
> > Fixes: 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark rewrite")
> > Cc: stable@vger.kernel.org
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> > Urgent fix for watermark support. This is definitely a pre-requisite for this series.
> > With this I've noticed that patch "[RFC 3/8] drm/i915/vlv: Move fifo_size from
> > intel_plane_wm_parameters to vlv_wm_state" introduces a regression with invalid FIFO split.
> > 
> > I need to find out what's going wrong in that patch before this series can be applied.
> > 
> >  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 376c60b98515..8defdcc54529 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -1148,7 +1148,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> >  			}
> >  		}
> >  
> > -		wm_state->num_levels = level;
> > +		wm_state->num_levels = level + 1;
> 
> Nope. The loop above breaks when the current level is bad, hence level-1
> is actually the higher usable level.

Without knowing the limits of plane->wm.fifo_size, it looks like it can
break on level == 0 though. Might as well set that hack to paranoid levels:

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 630b116988f6..e8c2874b8629 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1124,11 +1124,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;
-
                        /* hack */
-                       if (WARN_ON(level == 0 && wm > max_wm))
-                               wm = max_wm;
+                       if (level == 0) {
+                               int plane_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
+                               int max_wm = min(plane->wm.fifo_size, plane_wm);
+                               if (WARN_ON(wm > max_wm))
+                                       wm = max_wm;
+                       }
 
                        if (wm > plane->wm.fifo_size)
                                break;

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/vlv: Fix off-by-1 error in calculating num_levels.

[Ville Syrjälä]

On Tue, Jul 19, 2016 at 04:50:49PM +0100, Chris Wilson wrote:
> On Tue, Jul 19, 2016 at 06:25:42PM +0300, Ville Syrjälä wrote:
> > On Tue, Jul 19, 2016 at 05:14:23PM +0200, Maarten Lankhorst wrote:
> > > num_levels should be level+1, not level, else num_levels - 1 becomes
> > > negative. This resulted in bogus watermarks being written to the first
> > > 255 levels like below:
> > > 
> > > [drm] Setting FIFO watermarks - C: plane=0, cursor=0, sprite0=0, sprite1=0, SR: plane=0, cursor=0 level=255 cxsr=0
> > > [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
> > > [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe C FIFO underrun
> > > [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
> > > 
> > > Testcase: kms_atomic_transition
> > > Fixes: 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark rewrite")
> > > Cc: stable@vger.kernel.org
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > ---
> > > Urgent fix for watermark support. This is definitely a pre-requisite for this series.
> > > With this I've noticed that patch "[RFC 3/8] drm/i915/vlv: Move fifo_size from
> > > intel_plane_wm_parameters to vlv_wm_state" introduces a regression with invalid FIFO split.
> > > 
> > > I need to find out what's going wrong in that patch before this series can be applied.
> > > 
> > >  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 376c60b98515..8defdcc54529 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -1148,7 +1148,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> > >  			}
> > >  		}
> > >  
> > > -		wm_state->num_levels = level;
> > > +		wm_state->num_levels = level + 1;
> > 
> > Nope. The loop above breaks when the current level is bad, hence level-1
> > is actually the higher usable level.
> 
> Without knowing the limits of plane->wm.fifo_size, it looks like it can
> break on level == 0 though.

Hmm. That shouldn't be possible. So looks like a bug snuck in.

> Might as well set that hack to paranoid levels:
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 630b116988f6..e8c2874b8629 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1124,11 +1124,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;
> -
>                         /* hack */
> -                       if (WARN_ON(level == 0 && wm > max_wm))
> -                               wm = max_wm;

Actually this should just be 

if (WARN_ON(level == 0 && wm > fifo_size))
	wm = fifo_size;

assuming we want to keep the hack around for now.

Eventually we'll want to make it just return an error though.

> +                       if (level == 0) {
> +                               int plane_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
> +                               int max_wm = min(plane->wm.fifo_size, plane_wm);
> +                               if (WARN_ON(wm > max_wm))
> +                                       wm = max_wm;
> +                       }
>  
>                         if (wm > plane->wm.fifo_size)
>                                 break;
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

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

Re: [PATCH] drm/i915/vlv: Fix off-by-1 error in calculating num_levels.

[Maarten Lankhorst]

Op 19-07-16 om 17:14 schreef Maarten Lankhorst:
> num_levels should be level+1, not level, else num_levels - 1 becomes
> negative. This resulted in bogus watermarks being written to the first
> 255 levels like below:
>
> [drm] Setting FIFO watermarks - C: plane=0, cursor=0, sprite0=0, sprite1=0, SR: plane=0, cursor=0 level=255 cxsr=0
> [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
> [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe C FIFO underrun
> [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
>
> Testcase: kms_atomic_transition
> Fixes: 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark rewrite")
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> Urgent fix for watermark support. This is definitely a pre-requisite for this series.
> With this I've noticed that patch "[RFC 3/8] drm/i915/vlv: Move fifo_size from
> intel_plane_wm_parameters to vlv_wm_state" introduces a regression with invalid FIFO split.
>
> I need to find out what's going wrong in that patch before this series can be applied.
>
>  drivers/gpu/drm/i915/intel_pm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 376c60b98515..8defdcc54529 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1148,7 +1148,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>  			}
>  		}
>  
> -		wm_state->num_levels = level;
> +		wm_state->num_levels = level + 1;
>  
>  		if (!wm_state->cxsr)
>  			continue;

Never mind, this patch is broken. level can still be 0, but it needs another solution. :(

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

Re: [PATCH] drm/i915/vlv: Fix off-by-1 error in calculating num_levels.

[Maarten Lankhorst]

Hey,

Op 19-07-16 om 18:21 schreef Ville Syrjälä:
> On Tue, Jul 19, 2016 at 04:50:49PM +0100, Chris Wilson wrote:
>> On Tue, Jul 19, 2016 at 06:25:42PM +0300, Ville Syrjälä wrote:
>>> On Tue, Jul 19, 2016 at 05:14:23PM +0200, Maarten Lankhorst wrote:
>>>> num_levels should be level+1, not level, else num_levels - 1 becomes
>>>> negative. This resulted in bogus watermarks being written to the first
>>>> 255 levels like below:
>>>>
>>>> [drm] Setting FIFO watermarks - C: plane=0, cursor=0, sprite0=0, sprite1=0, SR: plane=0, cursor=0 level=255 cxsr=0
>>>> [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
>>>> [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe C FIFO underrun
>>>> [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
>>>>
>>>> Testcase: kms_atomic_transition
>>>> Fixes: 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark rewrite")
>>>> Cc: stable@vger.kernel.org
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>> Urgent fix for watermark support. This is definitely a pre-requisite for this series.
>>>> With this I've noticed that patch "[RFC 3/8] drm/i915/vlv: Move fifo_size from
>>>> intel_plane_wm_parameters to vlv_wm_state" introduces a regression with invalid FIFO split.
>>>>
>>>> I need to find out what's going wrong in that patch before this series can be applied.
>>>>
>>>>  drivers/gpu/drm/i915/intel_pm.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>> index 376c60b98515..8defdcc54529 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -1148,7 +1148,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>>>>  			}
>>>>  		}
>>>>  
>>>> -		wm_state->num_levels = level;
>>>> +		wm_state->num_levels = level + 1;
>>> Nope. The loop above breaks when the current level is bad, hence level-1
>>> is actually the higher usable level.
>> Without knowing the limits of plane->wm.fifo_size, it looks like it can
>> break on level == 0 though.
> Hmm. That shouldn't be possible. So looks like a bug snuck in.
>
>> Might as well set that hack to paranoid levels:
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 630b116988f6..e8c2874b8629 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -1124,11 +1124,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;
>> -
>>                         /* hack */
>> -                       if (WARN_ON(level == 0 && wm > max_wm))
>> -                               wm = max_wm;
> Actually this should just be 
>
> if (WARN_ON(level == 0 && wm > fifo_size))
> 	wm = fifo_size;
>
> assuming we want to keep the hack around for now.
>
> Eventually we'll want to make it just return an error though.
Yes, that's what I came up with.

But we still need to clamp further, probably to plane->wm.fifo_size.
However sr_fifo_size is also clamped to 511 because of the level calculations here.

Below it sets sr[level].plane = min(sr_fifo_size, wm[level].plane),
which seems weird. How can this ever end up being something other than wm[level].plane?
Because sr_fifo_size >= max_wm is always true.

I guess vlv_invert_wms will invert it, but we could simply only set it there then, or remove the min()..

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

Re: [PATCH] drm/i915/vlv: Fix off-by-1 error in calculating num_levels.

[Ville Syrjälä]

On Mon, Jul 25, 2016 at 01:32:45PM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Op 19-07-16 om 18:21 schreef Ville Syrjälä:
> > On Tue, Jul 19, 2016 at 04:50:49PM +0100, Chris Wilson wrote:
> >> On Tue, Jul 19, 2016 at 06:25:42PM +0300, Ville Syrjälä wrote:
> >>> On Tue, Jul 19, 2016 at 05:14:23PM +0200, Maarten Lankhorst wrote:
> >>>> num_levels should be level+1, not level, else num_levels - 1 becomes
> >>>> negative. This resulted in bogus watermarks being written to the first
> >>>> 255 levels like below:
> >>>>
> >>>> [drm] Setting FIFO watermarks - C: plane=0, cursor=0, sprite0=0, sprite1=0, SR: plane=0, cursor=0 level=255 cxsr=0
> >>>> [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
> >>>> [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe C FIFO underrun
> >>>> [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
> >>>>
> >>>> Testcase: kms_atomic_transition
> >>>> Fixes: 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark rewrite")
> >>>> Cc: stable@vger.kernel.org
> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>> Urgent fix for watermark support. This is definitely a pre-requisite for this series.
> >>>> With this I've noticed that patch "[RFC 3/8] drm/i915/vlv: Move fifo_size from
> >>>> intel_plane_wm_parameters to vlv_wm_state" introduces a regression with invalid FIFO split.
> >>>>
> >>>> I need to find out what's going wrong in that patch before this series can be applied.
> >>>>
> >>>>  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>> index 376c60b98515..8defdcc54529 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>> @@ -1148,7 +1148,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> >>>>  			}
> >>>>  		}
> >>>>  
> >>>> -		wm_state->num_levels = level;
> >>>> +		wm_state->num_levels = level + 1;
> >>> Nope. The loop above breaks when the current level is bad, hence level-1
> >>> is actually the higher usable level.
> >> Without knowing the limits of plane->wm.fifo_size, it looks like it can
> >> break on level == 0 though.
> > Hmm. That shouldn't be possible. So looks like a bug snuck in.
> >
> >> Might as well set that hack to paranoid levels:
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index 630b116988f6..e8c2874b8629 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -1124,11 +1124,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;
> >> -
> >>                         /* hack */
> >> -                       if (WARN_ON(level == 0 && wm > max_wm))
> >> -                               wm = max_wm;
> > Actually this should just be 
> >
> > if (WARN_ON(level == 0 && wm > fifo_size))
> > 	wm = fifo_size;
> >
> > assuming we want to keep the hack around for now.
> >
> > Eventually we'll want to make it just return an error though.
> Yes, that's what I came up with.
> 
> But we still need to clamp further, probably to plane->wm.fifo_size.
> However sr_fifo_size is also clamped to 511 because of the level calculations here.
> 
> Below it sets sr[level].plane = min(sr_fifo_size, wm[level].plane),
> which seems weird. How can this ever end up being something other than wm[level].plane?

There are three planes. CHV was supposed to have maxfifo with up to 2 active
planes. But I'm not sure that feature made it in. I never got around
testing it.

> Because sr_fifo_size >= max_wm is always true.
> 
> I guess vlv_invert_wms will invert it, but we could simply only set it there then, or remove the min()..
> 
> ~Maarten

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

Re: [PATCH] drm/i915/vlv: Fix off-by-1 error in calculating num_levels.

[Maarten Lankhorst]

Op 25-07-16 om 13:51 schreef Ville Syrjälä:
> On Mon, Jul 25, 2016 at 01:32:45PM +0200, Maarten Lankhorst wrote:
>> Hey,
>>
>> Op 19-07-16 om 18:21 schreef Ville Syrjälä:
>>> On Tue, Jul 19, 2016 at 04:50:49PM +0100, Chris Wilson wrote:
>>>> On Tue, Jul 19, 2016 at 06:25:42PM +0300, Ville Syrjälä wrote:
>>>>> On Tue, Jul 19, 2016 at 05:14:23PM +0200, Maarten Lankhorst wrote:
>>>>>> num_levels should be level+1, not level, else num_levels - 1 becomes
>>>>>> negative. This resulted in bogus watermarks being written to the first
>>>>>> 255 levels like below:
>>>>>>
>>>>>> [drm] Setting FIFO watermarks - C: plane=0, cursor=0, sprite0=0, sprite1=0, SR: plane=0, cursor=0 level=255 cxsr=0
>>>>>> [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
>>>>>> [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe C FIFO underrun
>>>>>> [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
>>>>>>
>>>>>> Testcase: kms_atomic_transition
>>>>>> Fixes: 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark rewrite")
>>>>>> Cc: stable@vger.kernel.org
>>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> ---
>>>>>> Urgent fix for watermark support. This is definitely a pre-requisite for this series.
>>>>>> With this I've noticed that patch "[RFC 3/8] drm/i915/vlv: Move fifo_size from
>>>>>> intel_plane_wm_parameters to vlv_wm_state" introduces a regression with invalid FIFO split.
>>>>>>
>>>>>> I need to find out what's going wrong in that patch before this series can be applied.
>>>>>>
>>>>>>  drivers/gpu/drm/i915/intel_pm.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>>>> index 376c60b98515..8defdcc54529 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>>> @@ -1148,7 +1148,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
>>>>>>  			}
>>>>>>  		}
>>>>>>  
>>>>>> -		wm_state->num_levels = level;
>>>>>> +		wm_state->num_levels = level + 1;
>>>>> Nope. The loop above breaks when the current level is bad, hence level-1
>>>>> is actually the higher usable level.
>>>> Without knowing the limits of plane->wm.fifo_size, it looks like it can
>>>> break on level == 0 though.
>>> Hmm. That shouldn't be possible. So looks like a bug snuck in.
>>>
>>>> Might as well set that hack to paranoid levels:
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>> index 630b116988f6..e8c2874b8629 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -1124,11 +1124,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;
>>>> -
>>>>                         /* hack */
>>>> -                       if (WARN_ON(level == 0 && wm > max_wm))
>>>> -                               wm = max_wm;
>>> Actually this should just be 
>>>
>>> if (WARN_ON(level == 0 && wm > fifo_size))
>>> 	wm = fifo_size;
>>>
>>> assuming we want to keep the hack around for now.
>>>
>>> Eventually we'll want to make it just return an error though.
>> Yes, that's what I came up with.
>>
>> But we still need to clamp further, probably to plane->wm.fifo_size.
>> However sr_fifo_size is also clamped to 511 because of the level calculations here.
>>
>> Below it sets sr[level].plane = min(sr_fifo_size, wm[level].plane),
>> which seems weird. How can this ever end up being something other than wm[level].plane?
> There are three planes. CHV was supposed to have maxfifo with up to 2 active
> planes. But I'm not sure that feature made it in. I never got around
> testing it.
I don't know, we only enable it with a single plane, plus cursor. I don't see where the other
plane wm's would fit in?

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

Re: [PATCH] drm/i915/vlv: Fix off-by-1 error in calculating num_levels.

[Ville Syrjälä]

On Mon, Jul 25, 2016 at 02:46:01PM +0200, Maarten Lankhorst wrote:
> Op 25-07-16 om 13:51 schreef Ville Syrjälä:
> > On Mon, Jul 25, 2016 at 01:32:45PM +0200, Maarten Lankhorst wrote:
> >> Hey,
> >>
> >> Op 19-07-16 om 18:21 schreef Ville Syrjälä:
> >>> On Tue, Jul 19, 2016 at 04:50:49PM +0100, Chris Wilson wrote:
> >>>> On Tue, Jul 19, 2016 at 06:25:42PM +0300, Ville Syrjälä wrote:
> >>>>> On Tue, Jul 19, 2016 at 05:14:23PM +0200, Maarten Lankhorst wrote:
> >>>>>> num_levels should be level+1, not level, else num_levels - 1 becomes
> >>>>>> negative. This resulted in bogus watermarks being written to the first
> >>>>>> 255 levels like below:
> >>>>>>
> >>>>>> [drm] Setting FIFO watermarks - C: plane=0, cursor=0, sprite0=0, sprite1=0, SR: plane=0, cursor=0 level=255 cxsr=0
> >>>>>> [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
> >>>>>> [drm:intel_cpu_fifo_underrun_irq_handler [i915]] *ERROR* CPU pipe C FIFO underrun
> >>>>>> [drm:chv_set_memory_dvfs [i915]] *ERROR* timed out waiting for Punit DDR DVFS request
> >>>>>>
> >>>>>> Testcase: kms_atomic_transition
> >>>>>> Fixes: 262cd2e154c2 ("drm/i915: CHV DDR DVFS support and another watermark rewrite")
> >>>>>> Cc: stable@vger.kernel.org
> >>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>> ---
> >>>>>> Urgent fix for watermark support. This is definitely a pre-requisite for this series.
> >>>>>> With this I've noticed that patch "[RFC 3/8] drm/i915/vlv: Move fifo_size from
> >>>>>> intel_plane_wm_parameters to vlv_wm_state" introduces a regression with invalid FIFO split.
> >>>>>>
> >>>>>> I need to find out what's going wrong in that patch before this series can be applied.
> >>>>>>
> >>>>>>  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> >>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>>>> index 376c60b98515..8defdcc54529 100644
> >>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>>>> @@ -1148,7 +1148,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> >>>>>>  			}
> >>>>>>  		}
> >>>>>>  
> >>>>>> -		wm_state->num_levels = level;
> >>>>>> +		wm_state->num_levels = level + 1;
> >>>>> Nope. The loop above breaks when the current level is bad, hence level-1
> >>>>> is actually the higher usable level.
> >>>> Without knowing the limits of plane->wm.fifo_size, it looks like it can
> >>>> break on level == 0 though.
> >>> Hmm. That shouldn't be possible. So looks like a bug snuck in.
> >>>
> >>>> Might as well set that hack to paranoid levels:
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>>> index 630b116988f6..e8c2874b8629 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_pm.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >>>> @@ -1124,11 +1124,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;
> >>>> -
> >>>>                         /* hack */
> >>>> -                       if (WARN_ON(level == 0 && wm > max_wm))
> >>>> -                               wm = max_wm;
> >>> Actually this should just be 
> >>>
> >>> if (WARN_ON(level == 0 && wm > fifo_size))
> >>> 	wm = fifo_size;
> >>>
> >>> assuming we want to keep the hack around for now.
> >>>
> >>> Eventually we'll want to make it just return an error though.
> >> Yes, that's what I came up with.
> >>
> >> But we still need to clamp further, probably to plane->wm.fifo_size.
> >> However sr_fifo_size is also clamped to 511 because of the level calculations here.
> >>
> >> Below it sets sr[level].plane = min(sr_fifo_size, wm[level].plane),
> >> which seems weird. How can this ever end up being something other than wm[level].plane?
> > There are three planes. CHV was supposed to have maxfifo with up to 2 active
> > planes. But I'm not sure that feature made it in. I never got around
> > testing it.
> I don't know, we only enable it with a single plane, plus cursor. I don't see where the other
> plane wm's would fit in?

The FIFO would be split 1:1 when two planes are enabled. But as stated, I
never actually tried it, so I never enabled it.

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