[PATCH 0/3] Remaining patches for WM cleanup series

[PATCH 0/3] Remaining patches for WM cleanup series

[Mahesh Kumar]

This series contains remaining two patches from wm cleanup series
https://patchwork.freedesktop.org/series/20152/

Initial 10 patches already got merged in tree so sending remaining 2
separately.

This series also includes a patch to solve underrun issue, when i915 is
build as a module.

Mahesh Kumar (3):
  drm/i915/skl+: Don't trust cached ddb values
  drm/i915/skl: New ddb allocation algorithm
  drm/i915/skl+: consider max supported plane pixel rate while scaling

 drivers/gpu/drm/i915/intel_display.c |   3 +
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      | 359 +++++++++++++++++++++++++----------
 3 files changed, 261 insertions(+), 103 deletions(-)

-- 
2.11.0

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

[PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values

[Mahesh Kumar]

Don't trust cached DDB values. Recalculate the ddb value if cached value
is zero.

If i915 is build as a module, there may be a race condition when
cursor_disable call comes even before intel_fbdev_initial_config.
Which may lead to cached value being 0. And further commit will fail
until a modeset.

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 936eef1634c7..b67be1355e39 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *for_crtc = cstate->base.crtc;
 	unsigned int pipe_size, ddb_size;
+	unsigned int active_crtcs;
 	int nth_active_pipe;
 
 	if (WARN_ON(!state) || !cstate->base.active) {
@@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 	}
 
 	if (intel_state->active_pipe_changes)
-		*num_active = hweight32(intel_state->active_crtcs);
+		active_crtcs = intel_state->active_crtcs;
 	else
-		*num_active = hweight32(dev_priv->active_crtcs);
+		active_crtcs = dev_priv->active_crtcs;
 
+	*num_active = hweight32(active_crtcs);
 	ddb_size = INTEL_INFO(dev_priv)->ddb_size;
 	WARN_ON(ddb_size == 0);
 
@@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 		 * copy from old state to be sure
 		 */
 		*alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
-		return;
+		if (!skl_ddb_entry_size(alloc))
+			DRM_DEBUG_KMS("Cached pipe DDB is 0 recalculate\n");
+		else
+			return;
 	}
 
-	nth_active_pipe = hweight32(intel_state->active_crtcs &
+	nth_active_pipe = hweight32(active_crtcs &
 				    (drm_crtc_mask(for_crtc) - 1));
-	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
+	pipe_size = ddb_size / hweight32(active_crtcs);
 	alloc->start = nth_active_pipe * ddb_size / *num_active;
 	alloc->end = alloc->start + pipe_size;
 }
-- 
2.11.0

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

[PATCH 2/3] drm/i915/skl: New ddb allocation algorithm

[Mahesh Kumar]

This patch implements new DDB allocation algorithm as per HW team
recommendation. This algo takecare of scenario where we allocate less DDB
for the planes with lower relative pixel rate, but they require more DDB
to work.
It also takes care of enabling same watermark level for each
plane in crtc, for efficient power saving.

Changes since v1:
 - Rebase on top of Paulo's patch series

Changes since v2:
 - Fix the for loop condition to enable WM

Changes since v3:
 - Fix crash in cursor i-g-t reported by Maarten
 - Rebase after addressing Paulo's comments
 - Few other ULT fixes
Changes since v4:
 - Rebase on drm-tip
 - Added separate function to enable WM levels
Changes since v5:
 - Fix a crash identified in skl-6770HQ system
Changes since v6:
 - Address review comments from Matt
Changes since v7:
 - Fix failure return in skl_compute_plane_wm (Matt)
 - fix typo
Changes since v8:
 - Always check cursor wm enable irrespective of total_data_rate

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 257 +++++++++++++++++++++++++---------------
 1 file changed, 159 insertions(+), 98 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b67be1355e39..46aca5cbc047 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4044,13 +4044,41 @@ skl_ddb_calc_min(const struct intel_crtc_state *cstate, int num_active,
 	minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
 }
 
+static void
+skl_enable_plane_wm_levels(const struct drm_i915_private *dev_priv,
+			   uint16_t plane_ddb,
+			   uint16_t max_level,
+			   struct skl_plane_wm *wm)
+{
+	int level;
+	/*
+	 * Now enable all levels in WM structure which can be enabled
+	 * using current DDB allocation
+	 */
+	for (level = ilk_wm_max_level(dev_priv); level >= 0; level--) {
+		struct skl_wm_level *level_wm = &wm->wm[level];
+
+		if (level > max_level || level_wm->plane_res_b == 0
+				      || level_wm->plane_res_l >= 31
+				      || level_wm->plane_res_b >= plane_ddb) {
+			level_wm->plane_en = false;
+			level_wm->plane_res_b = 0;
+			level_wm->plane_res_l = 0;
+		} else {
+			level_wm->plane_en = true;
+		}
+	}
+}
+
 static int
 skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
+		      struct skl_pipe_wm *pipe_wm,
 		      struct skl_ddb_allocation *ddb /* out */)
 {
 	struct drm_atomic_state *state = cstate->base.state;
 	struct drm_crtc *crtc = cstate->base.crtc;
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
@@ -4063,6 +4091,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	unsigned plane_data_rate[I915_MAX_PLANES] = {};
 	unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
 	uint16_t total_min_blocks = 0;
+	uint16_t total_level_ddb;
+	uint16_t plane_blocks = 0;
+	int max_level, level;
 
 	/* Clear the partitioning for disabled planes. */
 	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
@@ -4101,10 +4132,48 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		return -EINVAL;
 	}
 
-	alloc_size -= total_min_blocks;
-	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
+	alloc_size -= minimum[PLANE_CURSOR];
+	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
+							minimum[PLANE_CURSOR];
 	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
 
+	for (level = ilk_wm_max_level(dev_priv); level >= 0; level--) {
+		total_level_ddb = 0;
+		for_each_plane_id_on_crtc(intel_crtc, plane_id) {
+			/*
+			 * TODO: We should calculate watermark values for Y/UV
+			 * plane both in case of NV12 format and use both values
+			 * for ddb calculation. NV12 is disabled as of now, So
+			 * using only single/UV plane value here.
+			 */
+			struct skl_plane_wm *wm = &pipe_wm->planes[plane_id];
+			uint16_t plane_res_b = wm->wm[level].plane_res_b;
+			uint16_t min = minimum[plane_id] + y_minimum[plane_id];
+
+			if (plane_id == PLANE_CURSOR)
+				continue;
+
+			total_level_ddb += max(plane_res_b, min);
+		}
+
+		/*
+		 * If This level can successfully be enabled with the
+		 * pipe's current DDB allocation, then all lower levels are
+		 * guaranteed to succeed as well.
+		 */
+		if (total_level_ddb <= alloc_size)
+			break;
+	}
+
+	if ((level < 0) || (total_min_blocks > alloc_size)) {
+		DRM_DEBUG_KMS("Requested display configuration exceeds system DDB limitations");
+		DRM_DEBUG_KMS("minimum required %d/%d\n", (level < 0) ?
+				total_level_ddb : total_min_blocks, alloc_size);
+		return -EINVAL;
+	}
+	max_level = level;
+	alloc_size -= total_level_ddb;
+
 	/*
 	 * 2. Distribute the remaining space in proportion to the amount of
 	 * data each plane needs to fetch from memory.
@@ -4114,13 +4183,24 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	total_data_rate = skl_get_total_relative_data_rate(cstate,
 							   plane_data_rate,
 							   plane_y_data_rate);
+	/*
+	 * PLANE_CURSOR data rate is not included in total_data_rate.
+	 * If, only cursor plane is enabled we have to enable it's WM
+	 * levels explicitly before returning. Cursor has fixed ddb allocation,
+	 * So it's ok to always check cursor WM enabling before return.
+	 */
+	plane_blocks = skl_ddb_entry_size(&ddb->plane[pipe][PLANE_CURSOR]);
+	skl_enable_plane_wm_levels(dev_priv, plane_blocks, max_level,
+				   &pipe_wm->planes[PLANE_CURSOR]);
 	if (total_data_rate == 0)
 		return 0;
 
 	start = alloc->start;
 	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
 		unsigned int data_rate, y_data_rate;
-		uint16_t plane_blocks, y_plane_blocks = 0;
+		uint16_t plane_blocks = 0, y_plane_blocks = 0;
+		struct skl_plane_wm *wm = &pipe_wm->planes[plane_id];
+		uint16_t plane_res_b = wm->wm[max_level].plane_res_b;
 
 		if (plane_id == PLANE_CURSOR)
 			continue;
@@ -4132,33 +4212,36 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		 * promote the expression to 64 bits to avoid overflowing, the
 		 * result is < available as data_rate / total_data_rate < 1
 		 */
-		plane_blocks = minimum[plane_id];
-		plane_blocks += div_u64((uint64_t)alloc_size * data_rate,
-					total_data_rate);
 
 		/* Leave disabled planes at (0,0) */
 		if (data_rate) {
+			plane_blocks = max(minimum[plane_id], plane_res_b);
+			plane_blocks += div_u64((uint64_t)alloc_size *
+					data_rate, total_data_rate);
 			ddb->plane[pipe][plane_id].start = start;
 			ddb->plane[pipe][plane_id].end = start + plane_blocks;
+			start += plane_blocks;
 		}
 
-		start += plane_blocks;
-
 		/*
 		 * allocation for y_plane part of planar format:
+		 * TODO: Once we start calculating watermark values for Y/UV
+		 * plane both consider it for initial allowed wm blocks.
 		 */
 		y_data_rate = plane_y_data_rate[plane_id];
 
-		y_plane_blocks = y_minimum[plane_id];
-		y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate,
-					total_data_rate);
-
 		if (y_data_rate) {
+			y_plane_blocks = y_minimum[plane_id];
+			y_plane_blocks += div_u64((uint64_t)alloc_size *
+					y_data_rate, total_data_rate);
 			ddb->y_plane[pipe][plane_id].start = start;
 			ddb->y_plane[pipe][plane_id].end = start + y_plane_blocks;
+			start += y_plane_blocks;
 		}
-
-		start += y_plane_blocks;
+		skl_enable_plane_wm_levels(dev_priv,
+					   plane_blocks,
+					   max_level,
+					   wm);
 	}
 
 	return 0;
@@ -4248,11 +4331,9 @@ skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cstate,
 static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				struct intel_crtc_state *cstate,
 				const struct intel_plane_state *intel_pstate,
-				uint16_t ddb_allocation,
 				int level,
 				uint16_t *out_blocks, /* out */
-				uint8_t *out_lines, /* out */
-				bool *enabled /* out */)
+				uint8_t *out_lines /* out */)
 {
 	struct intel_plane *plane = to_intel_plane(intel_pstate->base.plane);
 	const struct drm_plane_state *pstate = &intel_pstate->base;
@@ -4275,10 +4356,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	bool y_tiled, x_tiled;
 
 	if (latency == 0 ||
-	    !intel_wm_plane_visible(cstate, intel_pstate)) {
-		*enabled = false;
+	    !intel_wm_plane_visible(cstate, intel_pstate))
 		return 0;
-	}
 
 	y_tiled = fb->modifier == I915_FORMAT_MOD_Y_TILED ||
 		  fb->modifier == I915_FORMAT_MOD_Yf_TILED;
@@ -4364,9 +4443,6 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		if ((cpp * cstate->base.adjusted_mode.crtc_htotal / 512 < 1) &&
 		    (plane_bytes_per_line / 512 < 1))
 			selected_result = method2;
-		else if ((ddb_allocation && ddb_allocation /
-			fixed_16_16_to_u32_round_up(plane_blocks_per_line)) >= 1)
-			selected_result = min_fixed_16_16(method1, method2);
 		else if (latency >= linetime_us)
 			selected_result = min_fixed_16_16(method1, method2);
 		else
@@ -4386,64 +4462,42 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		}
 	}
 
-	if (res_blocks >= ddb_allocation || res_lines > 31) {
-		*enabled = false;
-
-		/*
-		 * If there are no valid level 0 watermarks, then we can't
-		 * support this display configuration.
-		 */
-		if (level) {
-			return 0;
-		} else {
-			struct drm_plane *plane = pstate->plane;
+	if (res_lines >= 31 && level == 0) {
+		struct drm_plane *plane = pstate->plane;
 
-			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
-			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
-				      plane->base.id, plane->name,
-				      res_blocks, ddb_allocation, res_lines);
-			return -EINVAL;
-		}
+		DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
+		DRM_DEBUG_KMS("[PLANE:%d:%s] lines required = %u/31\n",
+				plane->base.id, plane->name, res_lines);
+		return -EINVAL;
 	}
 
 	*out_blocks = res_blocks;
 	*out_lines = res_lines;
-	*enabled = true;
 
 	return 0;
 }
 
 static int
 skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
-		      struct skl_ddb_allocation *ddb,
 		      struct intel_crtc_state *cstate,
 		      const struct intel_plane_state *intel_pstate,
 		      struct skl_plane_wm *wm)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
-	struct drm_plane *plane = intel_pstate->base.plane;
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	uint16_t ddb_blocks;
-	enum pipe pipe = intel_crtc->pipe;
 	int level, max_level = ilk_wm_max_level(dev_priv);
 	int ret;
 
 	if (WARN_ON(!intel_pstate->base.fb))
 		return -EINVAL;
 
-	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][intel_plane->id]);
-
 	for (level = 0; level <= max_level; level++) {
 		struct skl_wm_level *result = &wm->wm[level];
 
 		ret = skl_compute_plane_wm(dev_priv,
 					   cstate,
 					   intel_pstate,
-					   ddb_blocks,
 					   level,
 					   &result->plane_res_b,
-					   &result->plane_res_l,
-					   &result->plane_en);
+					   &result->plane_res_l);
 		if (ret)
 			return ret;
 	}
@@ -4509,8 +4563,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 
 		wm = &pipe_wm->planes[plane_id];
 
-		ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
-					    intel_pstate, wm);
+		ret = skl_compute_wm_levels(dev_priv, cstate, intel_pstate, wm);
 		if (ret)
 			return ret;
 		skl_compute_transition_wm(cstate, &wm->trans_wm);
@@ -4623,6 +4676,45 @@ bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
 	return false;
 }
 
+static int
+skl_ddb_add_affected_planes(struct intel_crtc_state *cstate,
+			    const struct skl_pipe_wm *old_pipe_wm,
+			    const struct skl_pipe_wm *pipe_wm)
+{
+	struct drm_atomic_state *state = cstate->base.state;
+	struct drm_device *dev = state->dev;
+	struct drm_crtc *crtc = cstate->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
+	struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
+	struct drm_plane_state *plane_state;
+	struct drm_plane *plane;
+	enum pipe pipe = intel_crtc->pipe;
+
+	WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
+
+	drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
+		enum plane_id plane_id = to_intel_plane(plane)->id;
+		const struct skl_plane_wm *wm = &pipe_wm->planes[plane_id];
+		const struct skl_plane_wm *old_wm = &old_pipe_wm->planes[plane_id];
+
+		if ((skl_ddb_entry_equal(&cur_ddb->plane[pipe][plane_id],
+					&new_ddb->plane[pipe][plane_id]) &&
+		    skl_ddb_entry_equal(&cur_ddb->y_plane[pipe][plane_id],
+					&new_ddb->y_plane[pipe][plane_id])) &&
+		    !memcmp(wm, old_wm, sizeof(struct skl_plane_wm)))
+			continue;
+
+		plane_state = drm_atomic_get_plane_state(state, plane);
+		if (IS_ERR(plane_state))
+			return PTR_ERR(plane_state);
+	}
+
+	return 0;
+}
+
 static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
 			      const struct skl_pipe_wm *old_pipe_wm,
 			      struct skl_pipe_wm *pipe_wm, /* out */
@@ -4636,6 +4728,17 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
 	if (ret)
 		return ret;
 
+	ret = skl_allocate_pipe_ddb(intel_cstate, pipe_wm, ddb);
+	if (ret)
+		return ret;
+	/*
+	 * TODO: Planes are included in state to arm WM registers.
+	 * Scope to optimize further, by just rewriting plane surf register.
+	 */
+	ret = skl_ddb_add_affected_planes(intel_cstate, old_pipe_wm, pipe_wm);
+	if (ret)
+		return ret;
+
 	if (!memcmp(old_pipe_wm, pipe_wm, sizeof(*pipe_wm)))
 		*changed = false;
 	else
@@ -4658,41 +4761,7 @@ pipes_modified(struct drm_atomic_state *state)
 }
 
 static int
-skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
-{
-	struct drm_atomic_state *state = cstate->base.state;
-	struct drm_device *dev = state->dev;
-	struct drm_crtc *crtc = cstate->base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
-	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
-	struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
-	struct drm_plane_state *plane_state;
-	struct drm_plane *plane;
-	enum pipe pipe = intel_crtc->pipe;
-
-	WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
-
-	drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
-		enum plane_id plane_id = to_intel_plane(plane)->id;
-
-		if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][plane_id],
-					&new_ddb->plane[pipe][plane_id]) &&
-		    skl_ddb_entry_equal(&cur_ddb->y_plane[pipe][plane_id],
-					&new_ddb->y_plane[pipe][plane_id]))
-			continue;
-
-		plane_state = drm_atomic_get_plane_state(state, plane);
-		if (IS_ERR(plane_state))
-			return PTR_ERR(plane_state);
-	}
-
-	return 0;
-}
-
-static int
-skl_compute_ddb(struct drm_atomic_state *state)
+skl_include_affected_crtcs(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -4756,14 +4825,6 @@ skl_compute_ddb(struct drm_atomic_state *state)
 		cstate = intel_atomic_get_crtc_state(state, intel_crtc);
 		if (IS_ERR(cstate))
 			return PTR_ERR(cstate);
-
-		ret = skl_allocate_pipe_ddb(cstate, ddb);
-		if (ret)
-			return ret;
-
-		ret = skl_ddb_add_affected_planes(cstate);
-		if (ret)
-			return ret;
 	}
 
 	return 0;
@@ -4844,7 +4905,7 @@ skl_compute_wm(struct drm_atomic_state *state)
 	/* Clear all dirty flags */
 	results->dirty_pipes = 0;
 
-	ret = skl_compute_ddb(state);
+	ret = skl_include_affected_crtcs(state);
 	if (ret)
 		return ret;
 
-- 
2.11.0

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

[PATCH 3/3] drm/i915/skl+: consider max supported plane pixel rate while scaling

[Mahesh Kumar]

A display resolution is only supported if it meets all the restrictions
below for Maximum Pipe Pixel Rate.

The display resolution must fit within the maximum pixel rate output
from the pipe. Make sure that the display pipe is able to feed pixels at
a rate required to support the desired resolution.
For each enabled plane on the pipe {
    If plane scaling enabled {
	Horizontal down scale amount = Maximum[1, plane horizontal size /
		    scaler horizontal window size]
	Vertical down scale amount = Maximum[1, plane vertical size /
		    scaler vertical window size]
	Plane down scale amount = Horizontal down scale amount *
		    Vertical down scale amount
	Plane Ratio = 1 / Plane down scale amount
    }
    Else {
	Plane Ratio = 1
    }
    If plane source pixel format is 64 bits per pixel {
	Plane Ratio = Plane Ratio * 8/9
    }
}

Pipe Ratio = Minimum Plane Ratio of all enabled planes on the pipe

If pipe scaling is enabled {
    Horizontal down scale amount = Maximum[1, pipe horizontal source size /
		scaler horizontal window size]
    Vertical down scale amount = Maximum[1, pipe vertical source size /
		scaler vertical window size]
    Note: The progressive fetch - interlace display mode is equivalent to a
		2.0 vertical down scale
    Pipe down scale amount = Horizontal down scale amount *
		Vertical down scale amount
    Pipe Ratio = Pipe Ratio / Pipe down scale amount
}

Pipe maximum pixel rate = CDCLK frequency * Pipe Ratio

In this patch our calculation is based on pipe downscale amount
(plane max downscale amount * pipe downscale amount) instead of Pipe
Ratio. So,
max supported crtc clock with given scaling = CDCLK / pipe downscale.
Flip will fail if,
current crtc clock > max supported crct clock with given scaling.

Changes since V1:
 - separate out fixed_16_16 wrapper API definition
Changes since V2:
 - Fix buggy crtc !active condition (Maarten)
 - use intel_wm_plane_visible wrapper as per Maarten's suggestion
Changes since V3:
 - Change failure return from ERANGE to EINVAL
Changes since V4:
 - Rebase based on previous patch changes
Changes since V5:
 - return EINVAL instead of continue (Maarten)
Changes since V6:
 - Improve commit message
 - Address review comment
Changes since V7:
 - use !enable instead of !active
 - rename config variable for consistency (Maarten)

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  3 ++
 drivers/gpu/drm/i915/intel_drv.h     |  2 +
 drivers/gpu/drm/i915/intel_pm.c      | 87 ++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7fa21df5bcd7..fb2e43d3eb5b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11177,6 +11177,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
 			ret = skl_update_scaler_crtc(pipe_config);
 
 		if (!ret)
+			ret = skl_check_pipe_max_pixel_rate(intel_crtc,
+							    pipe_config);
+		if (!ret)
 			ret = intel_atomic_setup_scalers(dev_priv, intel_crtc,
 							 pipe_config);
 	}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bd500977b3fc..93afac4a83fa 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1885,6 +1885,8 @@ bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
 				 int ignore);
 bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
+int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
+				  struct intel_crtc_state *cstate);
 static inline int intel_enable_rc6(void)
 {
 	return i915.enable_rc6;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 46aca5cbc047..ead346f1deb2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3868,6 +3868,93 @@ skl_plane_downscale_amount(const struct intel_crtc_state *cstate,
 	return mul_fixed16(downscale_w, downscale_h);
 }
 
+static uint_fixed_16_16_t
+skl_pipe_downscale_amount(const struct intel_crtc_state *crtc_state)
+{
+	uint_fixed_16_16_t pipe_downscale = u32_to_fixed_16_16(1);
+
+	if (!crtc_state->base.enable)
+		return pipe_downscale;
+
+	if (crtc_state->pch_pfit.enabled) {
+		uint32_t src_w, src_h, dst_w, dst_h;
+		uint32_t pfit_size = crtc_state->pch_pfit.size;
+		uint_fixed_16_16_t fp_w_ratio, fp_h_ratio;
+		uint_fixed_16_16_t downscale_h, downscale_w;
+
+		src_w = crtc_state->pipe_src_w;
+		src_h = crtc_state->pipe_src_h;
+		dst_w = pfit_size >> 16;
+		dst_h = pfit_size & 0xffff;
+
+		if (!dst_w || !dst_h)
+			return pipe_downscale;
+
+		fp_w_ratio = fixed_16_16_div(src_w, dst_w);
+		fp_h_ratio = fixed_16_16_div(src_h, dst_h);
+		downscale_w = max_fixed_16_16(fp_w_ratio, u32_to_fixed_16_16(1));
+		downscale_h = max_fixed_16_16(fp_h_ratio, u32_to_fixed_16_16(1));
+
+		pipe_downscale = mul_fixed16(downscale_w, downscale_h);
+	}
+
+	return pipe_downscale;
+}
+
+int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
+				  struct intel_crtc_state *cstate)
+{
+	struct drm_crtc_state *crtc_state = &cstate->base;
+	struct drm_atomic_state *state = crtc_state->state;
+	struct drm_plane *plane;
+	const struct drm_plane_state *pstate;
+	struct intel_plane_state *intel_pstate;
+	int crtc_clock, cdclk;
+	uint32_t pipe_max_pixel_rate;
+	uint_fixed_16_16_t pipe_downscale;
+	uint_fixed_16_16_t max_downscale = u32_to_fixed_16_16(1);
+
+	if (!cstate->base.enable)
+		return 0;
+
+	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) {
+		uint_fixed_16_16_t plane_downscale;
+		uint_fixed_16_16_t fp_9_div_8 = fixed_16_16_div(9, 8);
+		int bpp;
+
+		if (!intel_wm_plane_visible(cstate,
+					    to_intel_plane_state(pstate)))
+			continue;
+
+		if (WARN_ON(!pstate->fb))
+			return -EINVAL;
+
+		intel_pstate = to_intel_plane_state(pstate);
+		plane_downscale = skl_plane_downscale_amount(cstate,
+							     intel_pstate);
+		bpp = pstate->fb->format->cpp[0] * 8;
+		if (bpp == 64)
+			plane_downscale = mul_fixed16(plane_downscale,
+						      fp_9_div_8);
+
+		max_downscale = max_fixed_16_16(plane_downscale, max_downscale);
+	}
+	pipe_downscale = skl_pipe_downscale_amount(cstate);
+
+	pipe_downscale = mul_fixed16(pipe_downscale, max_downscale);
+
+	crtc_clock = crtc_state->adjusted_mode.crtc_clock;
+	cdclk = to_intel_atomic_state(state)->cdclk.logical.cdclk;
+	pipe_max_pixel_rate = div_round_up_u32_fixed16(cdclk, pipe_downscale);
+
+	if (pipe_max_pixel_rate < crtc_clock) {
+		DRM_ERROR("Max supported pixel clock with scaling exceeded\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static unsigned int
 skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 			     const struct drm_plane_state *pstate,
-- 
2.11.0

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

✗ Fi.CI.BAT: warning for Remaining patches for WM cleanup series (rev3)

[Patchwork]

== Series Details ==

Series: Remaining patches for WM cleanup series (rev3)
URL   : https://patchwork.freedesktop.org/series/24615/
State : warning

== Summary ==

Series 24615v3 Remaining patches for WM cleanup series
https://patchwork.freedesktop.org/api/1.0/series/24615/revisions/3/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> DMESG-FAIL (fi-skl-6700hq) fdo#101144
        Subgroup basic-flip-vs-modeset:
                pass       -> SKIP       (fi-skl-6700hq) fdo#100867
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> SKIP       (fi-skl-6700hq) fdo#99739
        Subgroup basic-plain-flip:
                pass       -> SKIP       (fi-skl-6700hq)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> SKIP       (fi-skl-6700hq)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> FAIL       (fi-skl-6700hq) fdo#101154 +16

fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fdo#100867 https://bugs.freedesktop.org/show_bug.cgi?id=100867
fdo#99739 https://bugs.freedesktop.org/show_bug.cgi?id=99739
fdo#101154 https://bugs.freedesktop.org/show_bug.cgi?id=101154

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:447s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:430s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:576s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:510s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:483s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:486s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:434s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:416s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:420s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:495s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:467s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:461s
fi-kbl-7560u     total:278  pass:263  dwarn:5   dfail:0   fail:0   skip:10  time:568s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:461s
fi-skl-6700hq    total:278  pass:239  dwarn:0   dfail:1   fail:17  skip:21  time:439s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:463s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:493s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:439s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:529s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:405s

235defd9a6d61596c283002e2b55694cec4806fc drm-tip: 2017y-05m-26d-14h-59m-56s UTC integration manifest
884c1d0 drm/i915/skl+: consider max supported plane pixel rate while scaling
8d9989d drm/i915/skl: New ddb allocation algorithm
bf734bd drm/i915/skl+: Don't trust cached ddb values

== Logs ==

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

Re: [PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values

[Rodrigo Vivi]

On Fri, May 26, 2017 at 8:15 AM, Mahesh Kumar <mahesh1.kumar@intel.com> wrote:
> Don't trust cached DDB values. Recalculate the ddb value if cached value
> is zero.
>
> If i915 is build as a module, there may be a race condition when
> cursor_disable call comes even before intel_fbdev_initial_config.
> Which may lead to cached value being 0. And further commit will fail
> until a modeset.
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 936eef1634c7..b67be1355e39 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>         struct drm_i915_private *dev_priv = to_i915(dev);
>         struct drm_crtc *for_crtc = cstate->base.crtc;
>         unsigned int pipe_size, ddb_size;
> +       unsigned int active_crtcs;
>         int nth_active_pipe;
>
>         if (WARN_ON(!state) || !cstate->base.active) {
> @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>         }
>
>         if (intel_state->active_pipe_changes)
> -               *num_active = hweight32(intel_state->active_crtcs);
> +               active_crtcs = intel_state->active_crtcs;
>         else
> -               *num_active = hweight32(dev_priv->active_crtcs);
> +               active_crtcs = dev_priv->active_crtcs;
>
> +       *num_active = hweight32(active_crtcs);
>         ddb_size = INTEL_INFO(dev_priv)->ddb_size;
>         WARN_ON(ddb_size == 0);
>
> @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>                  * copy from old state to be sure
>                  */
>                 *alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
> -               return;
> +               if (!skl_ddb_entry_size(alloc))
> +                       DRM_DEBUG_KMS("Cached pipe DDB is 0 recalculate\n");
> +               else
> +                       return;

I see 2 different patches inside this patch here. One is this chunk
here that does what commit message tells.

>         }
>
> -       nth_active_pipe = hweight32(intel_state->active_crtcs &
> +       nth_active_pipe = hweight32(active_crtcs &
>                                     (drm_crtc_mask(for_crtc) - 1));
> -       pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
> +       pipe_size = ddb_size / hweight32(active_crtcs);

While this is fixing another bug where we were still using
intel_state->active_crtcs here even when we use dev_priv->active_crtcs
for num_active.

Am I missing or misunderstanding something?

>         alloc->start = nth_active_pipe * ddb_size / *num_active;
>         alloc->end = alloc->start + pipe_size;
>  }
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values

[Mahesh Kumar]

Hi,


On Saturday 27 May 2017 02:53 AM, Rodrigo Vivi wrote:
> On Fri, May 26, 2017 at 8:15 AM, Mahesh Kumar <mahesh1.kumar@intel.com> wrote:
>> Don't trust cached DDB values. Recalculate the ddb value if cached value
>> is zero.
>>
>> If i915 is build as a module, there may be a race condition when
>> cursor_disable call comes even before intel_fbdev_initial_config.
>> Which may lead to cached value being 0. And further commit will fail
>> until a modeset.
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 936eef1634c7..b67be1355e39 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>          struct drm_i915_private *dev_priv = to_i915(dev);
>>          struct drm_crtc *for_crtc = cstate->base.crtc;
>>          unsigned int pipe_size, ddb_size;
>> +       unsigned int active_crtcs;
>>          int nth_active_pipe;
>>
>>          if (WARN_ON(!state) || !cstate->base.active) {
>> @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>          }
>>
>>          if (intel_state->active_pipe_changes)
>> -               *num_active = hweight32(intel_state->active_crtcs);
>> +               active_crtcs = intel_state->active_crtcs;
>>          else
>> -               *num_active = hweight32(dev_priv->active_crtcs);
>> +               active_crtcs = dev_priv->active_crtcs;
>>
>> +       *num_active = hweight32(active_crtcs);
>>          ddb_size = INTEL_INFO(dev_priv)->ddb_size;
>>          WARN_ON(ddb_size == 0);
>>
>> @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>                   * copy from old state to be sure
>>                   */
>>                  *alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
>> -               return;
>> +               if (!skl_ddb_entry_size(alloc))
>> +                       DRM_DEBUG_KMS("Cached pipe DDB is 0 recalculate\n");
>> +               else
>> +                       return;
> I see 2 different patches inside this patch here. One is this chunk
> here that does what commit message tells.
>
>>          }
>>
>> -       nth_active_pipe = hweight32(intel_state->active_crtcs &
>> +       nth_active_pipe = hweight32(active_crtcs &
>>                                      (drm_crtc_mask(for_crtc) - 1));
>> -       pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
>> +       pipe_size = ddb_size / hweight32(active_crtcs);
> While this is fixing another bug where we were still using
> intel_state->active_crtcs here even when we use dev_priv->active_crtcs
> for num_active.
Thanks for review :)

This one is prerequisite for the first part. In earlier scenario, we 
will reach to this point only if (intel_state->active_pipe_changes) is true.
So it was ok to get num of active crtcs from 
"intel_state->active_crtcs". But after above change we may reach to this 
point even if
"intel_state->active_pipe_changes" is not true. So we should get the 
active_crtcs from correct struct.

Thats why I think this should be part of same patch. Please let me know 
if you still have different opinion.

thanks,
-Mahesh
>
> Am I missing or misunderstanding something?
>
>>          alloc->start = nth_active_pipe * ddb_size / *num_active;
>>          alloc->end = alloc->start + pipe_size;
>>   }
>> --
>> 2.11.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

Re: [PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values

[Maarten Lankhorst]

Op 26-05-17 om 17:15 schreef Mahesh Kumar:
> Don't trust cached DDB values. Recalculate the ddb value if cached value
> is zero.
>
> If i915 is build as a module, there may be a race condition when
> cursor_disable call comes even before intel_fbdev_initial_config.
> Which may lead to cached value being 0. And further commit will fail
> until a modeset.
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 936eef1634c7..b67be1355e39 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc *for_crtc = cstate->base.crtc;
>  	unsigned int pipe_size, ddb_size;
> +	unsigned int active_crtcs;
>  	int nth_active_pipe;
>  
>  	if (WARN_ON(!state) || !cstate->base.active) {
> @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  	}
>  
>  	if (intel_state->active_pipe_changes)
> -		*num_active = hweight32(intel_state->active_crtcs);
> +		active_crtcs = intel_state->active_crtcs;
>  	else
> -		*num_active = hweight32(dev_priv->active_crtcs);
> +		active_crtcs = dev_priv->active_crtcs;
>  
> +	*num_active = hweight32(active_crtcs);
>  	ddb_size = INTEL_INFO(dev_priv)->ddb_size;
>  	WARN_ON(ddb_size == 0);
>  
> @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  		 * copy from old state to be sure
>  		 */
>  		*alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
> -		return;
> +		if (!skl_ddb_entry_size(alloc))
> +			DRM_DEBUG_KMS("Cached pipe DDB is 0 recalculate\n");
> +		else
> +			return;
>  	}
>  
> -	nth_active_pipe = hweight32(intel_state->active_crtcs &
> +	nth_active_pipe = hweight32(active_crtcs &
>  				    (drm_crtc_mask(for_crtc) - 1));
> -	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
> +	pipe_size = ddb_size / hweight32(active_crtcs);
>  	alloc->start = nth_active_pipe * ddb_size / *num_active;
>  	alloc->end = alloc->start + pipe_size;
>  }

I'd love it if you also add a warning somewhere for active pipe with no ddb allocation to prevent this from reoccuring in the future. :)
But with the root cause being a invalid ddb allocation at boot, won't the below one liner should fix it too, but better?

I'l ltest it when f3-kbl becomes available again.

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a488e068c3d6..63a47909f618 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5051,7 +5051,7 @@ skl_compute_wm(struct drm_atomic_state *state)
 	 */
 	for_each_new_crtc_in_state(state, crtc, cstate, i)
 		changed = true;
-	if (!changed)
+	if (!changed && !to_i915(state->dev)->wm.distrust_bios_wm)
 		return 0;
 
 	/* Clear all dirty flags */

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

Re: [PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values

[Mahesh Kumar]

Hi Maarten,

Thanks for review :)


On Tuesday 30 May 2017 03:30 PM, Maarten Lankhorst wrote:
> Op 26-05-17 om 17:15 schreef Mahesh Kumar:
>> Don't trust cached DDB values. Recalculate the ddb value if cached value
>> is zero.
>>
>> If i915 is build as a module, there may be a race condition when
>> cursor_disable call comes even before intel_fbdev_initial_config.
>> Which may lead to cached value being 0. And further commit will fail
>> until a modeset.
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 936eef1634c7..b67be1355e39 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>   	struct drm_i915_private *dev_priv = to_i915(dev);
>>   	struct drm_crtc *for_crtc = cstate->base.crtc;
>>   	unsigned int pipe_size, ddb_size;
>> +	unsigned int active_crtcs;
>>   	int nth_active_pipe;
>>   
>>   	if (WARN_ON(!state) || !cstate->base.active) {
>> @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>   	}
>>   
>>   	if (intel_state->active_pipe_changes)
>> -		*num_active = hweight32(intel_state->active_crtcs);
>> +		active_crtcs = intel_state->active_crtcs;
>>   	else
>> -		*num_active = hweight32(dev_priv->active_crtcs);
>> +		active_crtcs = dev_priv->active_crtcs;
>>   
>> +	*num_active = hweight32(active_crtcs);
>>   	ddb_size = INTEL_INFO(dev_priv)->ddb_size;
>>   	WARN_ON(ddb_size == 0);
>>   
>> @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>   		 * copy from old state to be sure
>>   		 */
>>   		*alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
>> -		return;
>> +		if (!skl_ddb_entry_size(alloc))
>> +			DRM_DEBUG_KMS("Cached pipe DDB is 0 recalculate\n");
>> +		else
>> +			return;
>>   	}
>>   
>> -	nth_active_pipe = hweight32(intel_state->active_crtcs &
>> +	nth_active_pipe = hweight32(active_crtcs &
>>   				    (drm_crtc_mask(for_crtc) - 1));
>> -	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
>> +	pipe_size = ddb_size / hweight32(active_crtcs);
>>   	alloc->start = nth_active_pipe * ddb_size / *num_active;
>>   	alloc->end = alloc->start + pipe_size;
>>   }
> I'd love it if you also add a warning somewhere for active pipe with no ddb allocation to prevent this from reoccuring in the future. :)
> But with the root cause being a invalid ddb allocation at boot, won't the below one liner should fix it too, but better?
Above DEBUG message is actually warning. That will hit only if pipe is 
active & DDB allocated for that pipe is "0" (we do return from caller if 
!cstate->active).
I can reword the msg to be WARN & make it something like "Pipe is active 
with No DDB allocated, recompute ddb now". This will fix the issue as 
well as add warning msg.

>
> I'l ltest it when f3-kbl becomes available again.
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a488e068c3d6..63a47909f618 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5051,7 +5051,7 @@ skl_compute_wm(struct drm_atomic_state *state)
>   	 */
>   	for_each_new_crtc_in_state(state, crtc, cstate, i)
>   		changed = true;
> -	if (!changed)
> +	if (!changed && !to_i915(state->dev)->wm.distrust_bios_wm)
>   		return 0;
>   
This change should also solve the issue during boot.

-Mahesh
>   	/* Clear all dirty flags */
>

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

Re: [PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values

[Lankhorst, Maarten]

[-- Attachment #1.1: Type: text/plain, Size: 4539 bytes --]

Mahesh Kumar schreef op di 30-05-2017 om 18:26 [+0530]:
> Hi Maarten,
> 
> Thanks for review :)
> 
> 
> On Tuesday 30 May 2017 03:30 PM, Maarten Lankhorst wrote:
> > Op 26-05-17 om 17:15 schreef Mahesh Kumar:
> > > Don't trust cached DDB values. Recalculate the ddb value if
> > > cached value
> > > is zero.
> > > 
> > > If i915 is build as a module, there may be a race condition when
> > > cursor_disable call comes even before intel_fbdev_initial_config.
> > > Which may lead to cached value being 0. And further commit will
> > > fail
> > > until a modeset.
> > > 
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
> > >   1 file changed, 10 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 936eef1634c7..b67be1355e39 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct
> > > drm_device *dev,
> > >   	struct drm_i915_private *dev_priv = to_i915(dev);
> > >   	struct drm_crtc *for_crtc = cstate->base.crtc;
> > >   	unsigned int pipe_size, ddb_size;
> > > +	unsigned int active_crtcs;
> > >   	int nth_active_pipe;
> > >   
> > >   	if (WARN_ON(!state) || !cstate->base.active) {
> > > @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct
> > > drm_device *dev,
> > >   	}
> > >   
> > >   	if (intel_state->active_pipe_changes)
> > > -		*num_active = hweight32(intel_state-
> > > >active_crtcs);
> > > +		active_crtcs = intel_state->active_crtcs;
> > >   	else
> > > -		*num_active = hweight32(dev_priv->active_crtcs);
> > > +		active_crtcs = dev_priv->active_crtcs;
> > >   
> > > +	*num_active = hweight32(active_crtcs);
> > >   	ddb_size = INTEL_INFO(dev_priv)->ddb_size;
> > >   	WARN_ON(ddb_size == 0);
> > >   
> > > @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct
> > > drm_device *dev,
> > >   		 * copy from old state to be sure
> > >   		 */
> > >   		*alloc = to_intel_crtc_state(for_crtc->state)-
> > > >wm.skl.ddb;
> > > -		return;
> > > +		if (!skl_ddb_entry_size(alloc))
> > > +			DRM_DEBUG_KMS("Cached pipe DDB is 0
> > > recalculate\n");
> > > +		else
> > > +			return;
> > >   	}
> > >   
> > > -	nth_active_pipe = hweight32(intel_state->active_crtcs &
> > > +	nth_active_pipe = hweight32(active_crtcs &
> > >   				    (drm_crtc_mask(for_crtc) -
> > > 1));
> > > -	pipe_size = ddb_size / hweight32(intel_state-
> > > >active_crtcs);
> > > +	pipe_size = ddb_size / hweight32(active_crtcs);
> > >   	alloc->start = nth_active_pipe * ddb_size /
> > > *num_active;
> > >   	alloc->end = alloc->start + pipe_size;
> > >   }
> > 
> > I'd love it if you also add a warning somewhere for active pipe
> > with no ddb allocation to prevent this from reoccuring in the
> > future. :)
> > But with the root cause being a invalid ddb allocation at boot,
> > won't the below one liner should fix it too, but better?
> 
> Above DEBUG message is actually warning. That will hit only if pipe
> is 
> active & DDB allocated for that pipe is "0" (we do return from caller
> if 
> !cstate->active).
> I can reword the msg to be WARN & make it something like "Pipe is
> active 
> with No DDB allocated, recompute ddb now". This will fix the issue
> as 
> well as add warning msg.
> 
> > 
> > I'l ltest it when f3-kbl becomes available again.
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index a488e068c3d6..63a47909f618 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5051,7 +5051,7 @@ skl_compute_wm(struct drm_atomic_state
> > *state)
> >   	 */
> >   	for_each_new_crtc_in_state(state, crtc, cstate, i)
> >   		changed = true;
> > -	if (!changed)
> > +	if (!changed && !to_i915(state->dev)->wm.distrust_bios_wm)
> >   		return 0;
> >   
> 
> This change should also solve the issue during boot.

Yeah I did some testing and this fixes it.
Looks like a bug in the original implementation of skl_compute_wm that
the KBL just happened to hit.

So I guess if others are hitting it too, then this needs
Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic
check time (v4)")
Cc: <stable@vger.kernel.org> # v4.8+

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3282 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH] drm/i915: Always recompute watermarks when distrust_bios_wm is set

[Maarten Lankhorst]

On some systems there can be a race condition in which no crtc state is
added to the first atomic commit. This results in all crtc's having a
null DDB allocation, causing a FIFO underrun on any update until the
first modeset.

Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Inspired-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic
check time (v4)")
Cc: <stable@vger.kernel.org> # v4.8+
Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a488e068c3d6..8daa382b47f3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5037,10 +5037,27 @@ skl_compute_wm(struct drm_atomic_state *state)
 	struct drm_crtc_state *cstate;
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct skl_wm_values *results = &intel_state->wm_results;
+	struct drm_device *dev = state->dev;
 	struct skl_pipe_wm *pipe_wm;
 	bool changed = false;
 	int ret, i;
 
+	if (to_i915(dev)->wm.distrust_bios_wm) {
+		changed = true;
+
+		/*
+		 * The first commit after driver load must update the ddb
+		 * allocations, so make sure we at least hold a lock to
+		 * inspect the state required for watermark updates.
+		 *
+		 * Below we'll grab the required crtc state.
+		 */
+		ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
+				       state->acquire_ctx);
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * If this transaction isn't actually touching any CRTC's, don't
 	 * bother with watermark calculation.  Note that if we pass this
@@ -5051,6 +5068,7 @@ skl_compute_wm(struct drm_atomic_state *state)
 	 */
 	for_each_new_crtc_in_state(state, crtc, cstate, i)
 		changed = true;
+
 	if (!changed)
 		return 0;
 
-- 
2.11.0

[PATCH] drm/i915: Always recompute watermarks when distrust_bios_wm is set

[Maarten Lankhorst]

On some systems there can be a race condition in which no crtc state is
added to the first atomic commit. This results in all crtc's having a
null DDB allocation, causing a FIFO underrun on any update until the
first modeset.

Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Inspired-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic
check time (v4)")
Cc: <stable@vger.kernel.org> # v4.8+
Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a488e068c3d6..8daa382b47f3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5037,10 +5037,27 @@ skl_compute_wm(struct drm_atomic_state *state)
 	struct drm_crtc_state *cstate;
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct skl_wm_values *results = &intel_state->wm_results;
+	struct drm_device *dev = state->dev;
 	struct skl_pipe_wm *pipe_wm;
 	bool changed = false;
 	int ret, i;
 
+	if (to_i915(dev)->wm.distrust_bios_wm) {
+		changed = true;
+
+		/*
+		 * The first commit after driver load must update the ddb
+		 * allocations, so make sure we at least hold a lock to
+		 * inspect the state required for watermark updates.
+		 *
+		 * Below we'll grab the required crtc state.
+		 */
+		ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
+				       state->acquire_ctx);
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * If this transaction isn't actually touching any CRTC's, don't
 	 * bother with watermark calculation.  Note that if we pass this
@@ -5051,6 +5068,7 @@ skl_compute_wm(struct drm_atomic_state *state)
 	 */
 	for_each_new_crtc_in_state(state, crtc, cstate, i)
 		changed = true;
+
 	if (!changed)
 		return 0;
 
-- 
2.11.0

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

Re: [PATCH] drm/i915: Always recompute watermarks when distrust_bios_wm is set

[Mahesh Kumar]

Hi,


On Wednesday 31 May 2017 01:26 PM, Maarten Lankhorst wrote:
> On some systems there can be a race condition in which no crtc state is
> added to the first atomic commit. This results in all crtc's having a
> null DDB allocation, causing a FIFO underrun on any update until the
> first modeset.
>
> Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Inspired-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic
> check time (v4)")
> Cc: <stable@vger.kernel.org> # v4.8+
> Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a488e068c3d6..8daa382b47f3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5037,10 +5037,27 @@ skl_compute_wm(struct drm_atomic_state *state)
>   	struct drm_crtc_state *cstate;
>   	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>   	struct skl_wm_values *results = &intel_state->wm_results;
> +	struct drm_device *dev = state->dev;
>   	struct skl_pipe_wm *pipe_wm;
>   	bool changed = false;
>   	int ret, i;
>   
> +	if (to_i915(dev)->wm.distrust_bios_wm) {
> +		changed = true;
> +
> +		/*
> +		 * The first commit after driver load must update the ddb
> +		 * allocations, so make sure we at least hold a lock to
> +		 * inspect the state required for watermark updates.
> +		 *
> +		 * Below we'll grab the required crtc state.
> +		 */
> +		ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
> +				       state->acquire_ctx);
Do we need to get lock here?
We will anyway go ahead & get the connection_mutex lock during 
skl_compute_ddb based on distruct_bios_wm flag.

-Mahesh
> +		if (ret)
> +			return ret;
> +	}
> +
>   	/*
>   	 * If this transaction isn't actually touching any CRTC's, don't
>   	 * bother with watermark calculation.  Note that if we pass this
> @@ -5051,6 +5068,7 @@ skl_compute_wm(struct drm_atomic_state *state)
>   	 */
>   	for_each_new_crtc_in_state(state, crtc, cstate, i)
>   		changed = true;
> +
>   	if (!changed)
>   		return 0;
>   

Re: [PATCH] drm/i915: Always recompute watermarks when distrust_bios_wm is set

[Maarten Lankhorst]

Op 31-05-17 om 12:42 schreef Mahesh Kumar:
> Hi,
>
>
> On Wednesday 31 May 2017 01:26 PM, Maarten Lankhorst wrote:
>> On some systems there can be a race condition in which no crtc state is
>> added to the first atomic commit. This results in all crtc's having a
>> null DDB allocation, causing a FIFO underrun on any update until the
>> first modeset.
>>
>> Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Inspired-by: Mahesh Kumar <mahesh1.kumar@intel.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic
>> check time (v4)")
>> Cc: <stable@vger.kernel.org> # v4.8+
>> Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index a488e068c3d6..8daa382b47f3 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -5037,10 +5037,27 @@ skl_compute_wm(struct drm_atomic_state *state)
>>       struct drm_crtc_state *cstate;
>>       struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>>       struct skl_wm_values *results = &intel_state->wm_results;
>> +    struct drm_device *dev = state->dev;
>>       struct skl_pipe_wm *pipe_wm;
>>       bool changed = false;
>>       int ret, i;
>>   +    if (to_i915(dev)->wm.distrust_bios_wm) {
>> +        changed = true;
>> +
>> +        /*
>> +         * The first commit after driver load must update the ddb
>> +         * allocations, so make sure we at least hold a lock to
>> +         * inspect the state required for watermark updates.
>> +         *
>> +         * Below we'll grab the required crtc state.
>> +         */
>> +        ret = drm_modeset_lock(&dev->mode_config.connection_mutex,
>> +                       state->acquire_ctx);
> Do we need to get lock here?
> We will anyway go ahead & get the connection_mutex lock during skl_compute_ddb based on distruct_bios_wm flag. 
Oh right, missed that one, yeah ignore it. :)

I'll send the original patch again, thanks for catching.

~Maarten

[PATCH] drm/i915: Always recompute watermarks when distrust_bios_wm is set, v2.

[Maarten Lankhorst]

On some systems there can be a race condition in which no crtc state is
added to the first atomic commit. This results in all crtc's having a
null DDB allocation, causing a FIFO underrun on any update until the
first modeset.

Changes since v1:
- Do not take the connection_mutex, this is already done below.

Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Inspired-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic
check time (v4)")
Cc: <stable@vger.kernel.org> # v4.8+
Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 936eef1634c7..fce4bc5ccc99 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4819,11 +4819,19 @@ skl_compute_wm(struct drm_atomic_state *state)
 	struct drm_crtc_state *cstate;
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct skl_wm_values *results = &intel_state->wm_results;
+	struct drm_device *dev = state->dev;
 	struct skl_pipe_wm *pipe_wm;
 	bool changed = false;
 	int ret, i;
 
 	/*
+	 * When we distrust bios wm we always need to recompute to set the
+	 * expected DDB allocations for each CRTC.
+	 */
+	if (to_i915(dev)->wm.distrust_bios_wm)
+		changed = true;
+
+	/*
 	 * If this transaction isn't actually touching any CRTC's, don't
 	 * bother with watermark calculation.  Note that if we pass this
 	 * test, we're guaranteed to hold at least one CRTC state mutex,
@@ -4833,6 +4841,7 @@ skl_compute_wm(struct drm_atomic_state *state)
 	 */
 	for_each_new_crtc_in_state(state, crtc, cstate, i)
 		changed = true;
+
 	if (!changed)
 		return 0;
 
-- 
2.11.0

Re: [PATCH] drm/i915: Always recompute watermarks when distrust_bios_wm is set, v2.

[Mahesh Kumar]

Reviewed-by: Mahesh Kumar <mahesh1.kumar@intel.com>


On Wednesday 31 May 2017 09:12 PM, Maarten Lankhorst wrote:
> On some systems there can be a race condition in which no crtc state is
> added to the first atomic commit. This results in all crtc's having a
> null DDB allocation, causing a FIFO underrun on any update until the
> first modeset.
>
> Changes since v1:
> - Do not take the connection_mutex, this is already done below.
>
> Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Inspired-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic
> check time (v4)")
> Cc: <stable@vger.kernel.org> # v4.8+
> Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 936eef1634c7..fce4bc5ccc99 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4819,11 +4819,19 @@ skl_compute_wm(struct drm_atomic_state *state)
>   	struct drm_crtc_state *cstate;
>   	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>   	struct skl_wm_values *results = &intel_state->wm_results;
> +	struct drm_device *dev = state->dev;
>   	struct skl_pipe_wm *pipe_wm;
>   	bool changed = false;
>   	int ret, i;
>   
>   	/*
> +	 * When we distrust bios wm we always need to recompute to set the
> +	 * expected DDB allocations for each CRTC.
> +	 */
> +	if (to_i915(dev)->wm.distrust_bios_wm)
> +		changed = true;
> +
> +	/*
>   	 * If this transaction isn't actually touching any CRTC's, don't
>   	 * bother with watermark calculation.  Note that if we pass this
>   	 * test, we're guaranteed to hold at least one CRTC state mutex,
> @@ -4833,6 +4841,7 @@ skl_compute_wm(struct drm_atomic_state *state)
>   	 */
>   	for_each_new_crtc_in_state(state, crtc, cstate, i)
>   		changed = true;
> +
>   	if (!changed)
>   		return 0;
>   

Re: [PATCH] drm/i915: Always recompute watermarks when distrust_bios_wm is set, v2.

[Mahesh Kumar]

Reviewed-by: Mahesh Kumar <mahesh1.kumar@intel.com>


On Wednesday 31 May 2017 09:12 PM, Maarten Lankhorst wrote:
> On some systems there can be a race condition in which no crtc state is
> added to the first atomic commit. This results in all crtc's having a
> null DDB allocation, causing a FIFO underrun on any update until the
> first modeset.
>
> Changes since v1:
> - Do not take the connection_mutex, this is already done below.
>
> Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Inspired-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic
> check time (v4)")
> Cc: <stable@vger.kernel.org> # v4.8+
> Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_pm.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 936eef1634c7..fce4bc5ccc99 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4819,11 +4819,19 @@ skl_compute_wm(struct drm_atomic_state *state)
>   	struct drm_crtc_state *cstate;
>   	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>   	struct skl_wm_values *results = &intel_state->wm_results;
> +	struct drm_device *dev = state->dev;
>   	struct skl_pipe_wm *pipe_wm;
>   	bool changed = false;
>   	int ret, i;
>   
>   	/*
> +	 * When we distrust bios wm we always need to recompute to set the
> +	 * expected DDB allocations for each CRTC.
> +	 */
> +	if (to_i915(dev)->wm.distrust_bios_wm)
> +		changed = true;
> +
> +	/*
>   	 * If this transaction isn't actually touching any CRTC's, don't
>   	 * bother with watermark calculation.  Note that if we pass this
>   	 * test, we're guaranteed to hold at least one CRTC state mutex,
> @@ -4833,6 +4841,7 @@ skl_compute_wm(struct drm_atomic_state *state)
>   	 */
>   	for_each_new_crtc_in_state(state, crtc, cstate, i)
>   		changed = true;
> +
>   	if (!changed)
>   		return 0;
>   

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

Re: [PATCH] drm/i915: Always recompute watermarks when distrust_bios_wm is set, v2.

[Matt Roper]

On Wed, May 31, 2017 at 05:42:36PM +0200, Maarten Lankhorst wrote:
> On some systems there can be a race condition in which no crtc state is
> added to the first atomic commit. This results in all crtc's having a
> null DDB allocation, causing a FIFO underrun on any update until the
> first modeset.
> 
> Changes since v1:
> - Do not take the connection_mutex, this is already done below.
> 
> Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Inspired-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic
> check time (v4)")
> Cc: <stable@vger.kernel.org> # v4.8+
> Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_pm.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 936eef1634c7..fce4bc5ccc99 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4819,11 +4819,19 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	struct drm_crtc_state *cstate;
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct skl_wm_values *results = &intel_state->wm_results;
> +	struct drm_device *dev = state->dev;
>  	struct skl_pipe_wm *pipe_wm;
>  	bool changed = false;
>  	int ret, i;
>  
>  	/*
> +	 * When we distrust bios wm we always need to recompute to set the
> +	 * expected DDB allocations for each CRTC.
> +	 */
> +	if (to_i915(dev)->wm.distrust_bios_wm)
> +		changed = true;
> +
> +	/*
>  	 * If this transaction isn't actually touching any CRTC's, don't
>  	 * bother with watermark calculation.  Note that if we pass this
>  	 * test, we're guaranteed to hold at least one CRTC state mutex,
> @@ -4833,6 +4841,7 @@ skl_compute_wm(struct drm_atomic_state *state)
>  	 */
>  	for_each_new_crtc_in_state(state, crtc, cstate, i)
>  		changed = true;
> +
>  	if (!changed)
>  		return 0;
>  
> -- 
> 2.11.0
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 2/3] drm/i915/skl: New ddb allocation algorithm

[Matt Roper]

On Fri, May 26, 2017 at 08:45:45PM +0530, Mahesh Kumar wrote:
> This patch implements new DDB allocation algorithm as per HW team
> recommendation. This algo takecare of scenario where we allocate less DDB
> for the planes with lower relative pixel rate, but they require more DDB
> to work.
> It also takes care of enabling same watermark level for each
> plane in crtc, for efficient power saving.
> 
> Changes since v1:
>  - Rebase on top of Paulo's patch series
> 
> Changes since v2:
>  - Fix the for loop condition to enable WM
> 
> Changes since v3:
>  - Fix crash in cursor i-g-t reported by Maarten
>  - Rebase after addressing Paulo's comments
>  - Few other ULT fixes
> Changes since v4:
>  - Rebase on drm-tip
>  - Added separate function to enable WM levels
> Changes since v5:
>  - Fix a crash identified in skl-6770HQ system
> Changes since v6:
>  - Address review comments from Matt
> Changes since v7:
>  - Fix failure return in skl_compute_plane_wm (Matt)
>  - fix typo
> Changes since v8:
>  - Always check cursor wm enable irrespective of total_data_rate
> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
...
> @@ -4114,13 +4183,24 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	total_data_rate = skl_get_total_relative_data_rate(cstate,
>  							   plane_data_rate,
>  							   plane_y_data_rate);
> +	/*
> +	 * PLANE_CURSOR data rate is not included in total_data_rate.
> +	 * If, only cursor plane is enabled we have to enable it's WM

A couple minor typo's here.  You probably don't want the comma after
"If," and the "it's" at the end of the line should be "its."

With those fixed,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


> +	 * levels explicitly before returning. Cursor has fixed ddb allocation,
> +	 * So it's ok to always check cursor WM enabling before return.
> +	 */
> +	plane_blocks = skl_ddb_entry_size(&ddb->plane[pipe][PLANE_CURSOR]);
> +	skl_enable_plane_wm_levels(dev_priv, plane_blocks, max_level,
> +				   &pipe_wm->planes[PLANE_CURSOR]);
>  	if (total_data_rate == 0)
>  		return 0;
>  
>  	start = alloc->start;
>  	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
>  		unsigned int data_rate, y_data_rate;
> -		uint16_t plane_blocks, y_plane_blocks = 0;
> +		uint16_t plane_blocks = 0, y_plane_blocks = 0;
> +		struct skl_plane_wm *wm = &pipe_wm->planes[plane_id];
> +		uint16_t plane_res_b = wm->wm[max_level].plane_res_b;
>  
>  		if (plane_id == PLANE_CURSOR)
>  			continue;
...

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915/skl: New ddb allocation algorithm

[Mahesh Kumar]

From: "Kumar, Mahesh" <mahesh1.kumar@intel.com>

This patch implements new DDB allocation algorithm as per HW team
recommendation. This algo takecare of scenario where we allocate less DDB
for the planes with lower relative pixel rate, but they require more DDB
to work.
It also takes care of enabling same watermark level for each
plane in crtc, for efficient power saving.

Changes since v1:
 - Rebase on top of Paulo's patch series

Changes since v2:
 - Fix the for loop condition to enable WM

Changes since v3:
 - Fix crash in cursor i-g-t reported by Maarten
 - Rebase after addressing Paulo's comments
 - Few other ULT fixes
Changes since v4:
 - Rebase on drm-tip
 - Added separate function to enable WM levels
Changes since v5:
 - Fix a crash identified in skl-6770HQ system
Changes since v6:
 - Address review comments from Matt
Changes since v7:
 - Fix failure return in skl_compute_plane_wm (Matt)
 - fix typo
Changes since v8:
 - Always check cursor wm enable irrespective of total_data_rate
Changes since v9:
 - fix typo

Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 257 +++++++++++++++++++++++++---------------
 1 file changed, 159 insertions(+), 98 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fce4bc5ccc99..1f9fee30cb4c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4039,13 +4039,41 @@ skl_ddb_calc_min(const struct intel_crtc_state *cstate, int num_active,
 	minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
 }
 
+static void
+skl_enable_plane_wm_levels(const struct drm_i915_private *dev_priv,
+			   uint16_t plane_ddb,
+			   uint16_t max_level,
+			   struct skl_plane_wm *wm)
+{
+	int level;
+	/*
+	 * Now enable all levels in WM structure which can be enabled
+	 * using current DDB allocation
+	 */
+	for (level = ilk_wm_max_level(dev_priv); level >= 0; level--) {
+		struct skl_wm_level *level_wm = &wm->wm[level];
+
+		if (level > max_level || level_wm->plane_res_b == 0
+				      || level_wm->plane_res_l >= 31
+				      || level_wm->plane_res_b >= plane_ddb) {
+			level_wm->plane_en = false;
+			level_wm->plane_res_b = 0;
+			level_wm->plane_res_l = 0;
+		} else {
+			level_wm->plane_en = true;
+		}
+	}
+}
+
 static int
 skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
+		      struct skl_pipe_wm *pipe_wm,
 		      struct skl_ddb_allocation *ddb /* out */)
 {
 	struct drm_atomic_state *state = cstate->base.state;
 	struct drm_crtc *crtc = cstate->base.crtc;
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
@@ -4058,6 +4086,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	unsigned plane_data_rate[I915_MAX_PLANES] = {};
 	unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
 	uint16_t total_min_blocks = 0;
+	uint16_t total_level_ddb;
+	uint16_t plane_blocks = 0;
+	int max_level, level;
 
 	/* Clear the partitioning for disabled planes. */
 	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
@@ -4096,10 +4127,48 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		return -EINVAL;
 	}
 
-	alloc_size -= total_min_blocks;
-	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
+	alloc_size -= minimum[PLANE_CURSOR];
+	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
+							minimum[PLANE_CURSOR];
 	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
 
+	for (level = ilk_wm_max_level(dev_priv); level >= 0; level--) {
+		total_level_ddb = 0;
+		for_each_plane_id_on_crtc(intel_crtc, plane_id) {
+			/*
+			 * TODO: We should calculate watermark values for Y/UV
+			 * plane both in case of NV12 format and use both values
+			 * for ddb calculation. NV12 is disabled as of now, So
+			 * using only single/UV plane value here.
+			 */
+			struct skl_plane_wm *wm = &pipe_wm->planes[plane_id];
+			uint16_t plane_res_b = wm->wm[level].plane_res_b;
+			uint16_t min = minimum[plane_id] + y_minimum[plane_id];
+
+			if (plane_id == PLANE_CURSOR)
+				continue;
+
+			total_level_ddb += max(plane_res_b, min);
+		}
+
+		/*
+		 * If This level can successfully be enabled with the
+		 * pipe's current DDB allocation, then all lower levels are
+		 * guaranteed to succeed as well.
+		 */
+		if (total_level_ddb <= alloc_size)
+			break;
+	}
+
+	if ((level < 0) || (total_min_blocks > alloc_size)) {
+		DRM_DEBUG_KMS("Requested display configuration exceeds system DDB limitations");
+		DRM_DEBUG_KMS("minimum required %d/%d\n", (level < 0) ?
+				total_level_ddb : total_min_blocks, alloc_size);
+		return -EINVAL;
+	}
+	max_level = level;
+	alloc_size -= total_level_ddb;
+
 	/*
 	 * 2. Distribute the remaining space in proportion to the amount of
 	 * data each plane needs to fetch from memory.
@@ -4109,13 +4178,24 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	total_data_rate = skl_get_total_relative_data_rate(cstate,
 							   plane_data_rate,
 							   plane_y_data_rate);
+	/*
+	 * PLANE_CURSOR data rate is not included in total_data_rate.
+	 * If only cursor plane is enabled we have to enable its WM levels
+	 * explicitly before returning. Cursor has fixed ddb allocation,
+	 * So it's ok to always check cursor WM enabling before return.
+	 */
+	plane_blocks = skl_ddb_entry_size(&ddb->plane[pipe][PLANE_CURSOR]);
+	skl_enable_plane_wm_levels(dev_priv, plane_blocks, max_level,
+				   &pipe_wm->planes[PLANE_CURSOR]);
 	if (total_data_rate == 0)
 		return 0;
 
 	start = alloc->start;
 	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
 		unsigned int data_rate, y_data_rate;
-		uint16_t plane_blocks, y_plane_blocks = 0;
+		uint16_t plane_blocks = 0, y_plane_blocks = 0;
+		struct skl_plane_wm *wm = &pipe_wm->planes[plane_id];
+		uint16_t plane_res_b = wm->wm[max_level].plane_res_b;
 
 		if (plane_id == PLANE_CURSOR)
 			continue;
@@ -4127,33 +4207,36 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 		 * promote the expression to 64 bits to avoid overflowing, the
 		 * result is < available as data_rate / total_data_rate < 1
 		 */
-		plane_blocks = minimum[plane_id];
-		plane_blocks += div_u64((uint64_t)alloc_size * data_rate,
-					total_data_rate);
 
 		/* Leave disabled planes at (0,0) */
 		if (data_rate) {
+			plane_blocks = max(minimum[plane_id], plane_res_b);
+			plane_blocks += div_u64((uint64_t)alloc_size *
+					data_rate, total_data_rate);
 			ddb->plane[pipe][plane_id].start = start;
 			ddb->plane[pipe][plane_id].end = start + plane_blocks;
+			start += plane_blocks;
 		}
 
-		start += plane_blocks;
-
 		/*
 		 * allocation for y_plane part of planar format:
+		 * TODO: Once we start calculating watermark values for Y/UV
+		 * plane both consider it for initial allowed wm blocks.
 		 */
 		y_data_rate = plane_y_data_rate[plane_id];
 
-		y_plane_blocks = y_minimum[plane_id];
-		y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate,
-					total_data_rate);
-
 		if (y_data_rate) {
+			y_plane_blocks = y_minimum[plane_id];
+			y_plane_blocks += div_u64((uint64_t)alloc_size *
+					y_data_rate, total_data_rate);
 			ddb->y_plane[pipe][plane_id].start = start;
 			ddb->y_plane[pipe][plane_id].end = start + y_plane_blocks;
+			start += y_plane_blocks;
 		}
-
-		start += y_plane_blocks;
+		skl_enable_plane_wm_levels(dev_priv,
+					   plane_blocks,
+					   max_level,
+					   wm);
 	}
 
 	return 0;
@@ -4243,11 +4326,9 @@ skl_adjusted_plane_pixel_rate(const struct intel_crtc_state *cstate,
 static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				struct intel_crtc_state *cstate,
 				const struct intel_plane_state *intel_pstate,
-				uint16_t ddb_allocation,
 				int level,
 				uint16_t *out_blocks, /* out */
-				uint8_t *out_lines, /* out */
-				bool *enabled /* out */)
+				uint8_t *out_lines /* out */)
 {
 	struct intel_plane *plane = to_intel_plane(intel_pstate->base.plane);
 	const struct drm_plane_state *pstate = &intel_pstate->base;
@@ -4270,10 +4351,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	bool y_tiled, x_tiled;
 
 	if (latency == 0 ||
-	    !intel_wm_plane_visible(cstate, intel_pstate)) {
-		*enabled = false;
+	    !intel_wm_plane_visible(cstate, intel_pstate))
 		return 0;
-	}
 
 	y_tiled = fb->modifier == I915_FORMAT_MOD_Y_TILED ||
 		  fb->modifier == I915_FORMAT_MOD_Yf_TILED;
@@ -4359,9 +4438,6 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		if ((cpp * cstate->base.adjusted_mode.crtc_htotal / 512 < 1) &&
 		    (plane_bytes_per_line / 512 < 1))
 			selected_result = method2;
-		else if ((ddb_allocation && ddb_allocation /
-			fixed_16_16_to_u32_round_up(plane_blocks_per_line)) >= 1)
-			selected_result = min_fixed_16_16(method1, method2);
 		else if (latency >= linetime_us)
 			selected_result = min_fixed_16_16(method1, method2);
 		else
@@ -4381,64 +4457,42 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		}
 	}
 
-	if (res_blocks >= ddb_allocation || res_lines > 31) {
-		*enabled = false;
-
-		/*
-		 * If there are no valid level 0 watermarks, then we can't
-		 * support this display configuration.
-		 */
-		if (level) {
-			return 0;
-		} else {
-			struct drm_plane *plane = pstate->plane;
+	if (res_lines >= 31 && level == 0) {
+		struct drm_plane *plane = pstate->plane;
 
-			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
-			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
-				      plane->base.id, plane->name,
-				      res_blocks, ddb_allocation, res_lines);
-			return -EINVAL;
-		}
+		DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
+		DRM_DEBUG_KMS("[PLANE:%d:%s] lines required = %u/31\n",
+				plane->base.id, plane->name, res_lines);
+		return -EINVAL;
 	}
 
 	*out_blocks = res_blocks;
 	*out_lines = res_lines;
-	*enabled = true;
 
 	return 0;
 }
 
 static int
 skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
-		      struct skl_ddb_allocation *ddb,
 		      struct intel_crtc_state *cstate,
 		      const struct intel_plane_state *intel_pstate,
 		      struct skl_plane_wm *wm)
 {
-	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
-	struct drm_plane *plane = intel_pstate->base.plane;
-	struct intel_plane *intel_plane = to_intel_plane(plane);
-	uint16_t ddb_blocks;
-	enum pipe pipe = intel_crtc->pipe;
 	int level, max_level = ilk_wm_max_level(dev_priv);
 	int ret;
 
 	if (WARN_ON(!intel_pstate->base.fb))
 		return -EINVAL;
 
-	ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][intel_plane->id]);
-
 	for (level = 0; level <= max_level; level++) {
 		struct skl_wm_level *result = &wm->wm[level];
 
 		ret = skl_compute_plane_wm(dev_priv,
 					   cstate,
 					   intel_pstate,
-					   ddb_blocks,
 					   level,
 					   &result->plane_res_b,
-					   &result->plane_res_l,
-					   &result->plane_en);
+					   &result->plane_res_l);
 		if (ret)
 			return ret;
 	}
@@ -4504,8 +4558,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
 
 		wm = &pipe_wm->planes[plane_id];
 
-		ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
-					    intel_pstate, wm);
+		ret = skl_compute_wm_levels(dev_priv, cstate, intel_pstate, wm);
 		if (ret)
 			return ret;
 		skl_compute_transition_wm(cstate, &wm->trans_wm);
@@ -4618,6 +4671,45 @@ bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
 	return false;
 }
 
+static int
+skl_ddb_add_affected_planes(struct intel_crtc_state *cstate,
+			    const struct skl_pipe_wm *old_pipe_wm,
+			    const struct skl_pipe_wm *pipe_wm)
+{
+	struct drm_atomic_state *state = cstate->base.state;
+	struct drm_device *dev = state->dev;
+	struct drm_crtc *crtc = cstate->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
+	struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
+	struct drm_plane_state *plane_state;
+	struct drm_plane *plane;
+	enum pipe pipe = intel_crtc->pipe;
+
+	WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
+
+	drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
+		enum plane_id plane_id = to_intel_plane(plane)->id;
+		const struct skl_plane_wm *wm = &pipe_wm->planes[plane_id];
+		const struct skl_plane_wm *old_wm = &old_pipe_wm->planes[plane_id];
+
+		if ((skl_ddb_entry_equal(&cur_ddb->plane[pipe][plane_id],
+					&new_ddb->plane[pipe][plane_id]) &&
+		    skl_ddb_entry_equal(&cur_ddb->y_plane[pipe][plane_id],
+					&new_ddb->y_plane[pipe][plane_id])) &&
+		    !memcmp(wm, old_wm, sizeof(struct skl_plane_wm)))
+			continue;
+
+		plane_state = drm_atomic_get_plane_state(state, plane);
+		if (IS_ERR(plane_state))
+			return PTR_ERR(plane_state);
+	}
+
+	return 0;
+}
+
 static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
 			      const struct skl_pipe_wm *old_pipe_wm,
 			      struct skl_pipe_wm *pipe_wm, /* out */
@@ -4631,6 +4723,17 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
 	if (ret)
 		return ret;
 
+	ret = skl_allocate_pipe_ddb(intel_cstate, pipe_wm, ddb);
+	if (ret)
+		return ret;
+	/*
+	 * TODO: Planes are included in state to arm WM registers.
+	 * Scope to optimize further, by just rewriting plane surf register.
+	 */
+	ret = skl_ddb_add_affected_planes(intel_cstate, old_pipe_wm, pipe_wm);
+	if (ret)
+		return ret;
+
 	if (!memcmp(old_pipe_wm, pipe_wm, sizeof(*pipe_wm)))
 		*changed = false;
 	else
@@ -4653,41 +4756,7 @@ pipes_modified(struct drm_atomic_state *state)
 }
 
 static int
-skl_ddb_add_affected_planes(struct intel_crtc_state *cstate)
-{
-	struct drm_atomic_state *state = cstate->base.state;
-	struct drm_device *dev = state->dev;
-	struct drm_crtc *crtc = cstate->base.crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
-	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
-	struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
-	struct drm_plane_state *plane_state;
-	struct drm_plane *plane;
-	enum pipe pipe = intel_crtc->pipe;
-
-	WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc));
-
-	drm_for_each_plane_mask(plane, dev, cstate->base.plane_mask) {
-		enum plane_id plane_id = to_intel_plane(plane)->id;
-
-		if (skl_ddb_entry_equal(&cur_ddb->plane[pipe][plane_id],
-					&new_ddb->plane[pipe][plane_id]) &&
-		    skl_ddb_entry_equal(&cur_ddb->y_plane[pipe][plane_id],
-					&new_ddb->y_plane[pipe][plane_id]))
-			continue;
-
-		plane_state = drm_atomic_get_plane_state(state, plane);
-		if (IS_ERR(plane_state))
-			return PTR_ERR(plane_state);
-	}
-
-	return 0;
-}
-
-static int
-skl_compute_ddb(struct drm_atomic_state *state)
+skl_include_affected_crtcs(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -4751,14 +4820,6 @@ skl_compute_ddb(struct drm_atomic_state *state)
 		cstate = intel_atomic_get_crtc_state(state, intel_crtc);
 		if (IS_ERR(cstate))
 			return PTR_ERR(cstate);
-
-		ret = skl_allocate_pipe_ddb(cstate, ddb);
-		if (ret)
-			return ret;
-
-		ret = skl_ddb_add_affected_planes(cstate);
-		if (ret)
-			return ret;
 	}
 
 	return 0;
@@ -4848,7 +4909,7 @@ skl_compute_wm(struct drm_atomic_state *state)
 	/* Clear all dirty flags */
 	results->dirty_pipes = 0;
 
-	ret = skl_compute_ddb(state);
+	ret = skl_include_affected_crtcs(state);
 	if (ret)
 		return ret;
 
-- 
2.11.0

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

Re: [PATCH 3/3] drm/i915/skl+: consider max supported plane pixel rate while scaling

[Maarten Lankhorst]

Op 26-05-17 om 17:15 schreef Mahesh Kumar:
> A display resolution is only supported if it meets all the restrictions
> below for Maximum Pipe Pixel Rate.
>
> The display resolution must fit within the maximum pixel rate output
> from the pipe. Make sure that the display pipe is able to feed pixels at
> a rate required to support the desired resolution.
> For each enabled plane on the pipe {
>     If plane scaling enabled {
> 	Horizontal down scale amount = Maximum[1, plane horizontal size /
> 		    scaler horizontal window size]
> 	Vertical down scale amount = Maximum[1, plane vertical size /
> 		    scaler vertical window size]
> 	Plane down scale amount = Horizontal down scale amount *
> 		    Vertical down scale amount
> 	Plane Ratio = 1 / Plane down scale amount
>     }
>     Else {
> 	Plane Ratio = 1
>     }
>     If plane source pixel format is 64 bits per pixel {
> 	Plane Ratio = Plane Ratio * 8/9
>     }
> }
>
> Pipe Ratio = Minimum Plane Ratio of all enabled planes on the pipe
>
> If pipe scaling is enabled {
>     Horizontal down scale amount = Maximum[1, pipe horizontal source size /
> 		scaler horizontal window size]
>     Vertical down scale amount = Maximum[1, pipe vertical source size /
> 		scaler vertical window size]
>     Note: The progressive fetch - interlace display mode is equivalent to a
> 		2.0 vertical down scale
>     Pipe down scale amount = Horizontal down scale amount *
> 		Vertical down scale amount
>     Pipe Ratio = Pipe Ratio / Pipe down scale amount
> }
>
> Pipe maximum pixel rate = CDCLK frequency * Pipe Ratio
>
> In this patch our calculation is based on pipe downscale amount
> (plane max downscale amount * pipe downscale amount) instead of Pipe
> Ratio. So,
> max supported crtc clock with given scaling = CDCLK / pipe downscale.
> Flip will fail if,
> current crtc clock > max supported crct clock with given scaling.
>
> Changes since V1:
>  - separate out fixed_16_16 wrapper API definition
> Changes since V2:
>  - Fix buggy crtc !active condition (Maarten)
>  - use intel_wm_plane_visible wrapper as per Maarten's suggestion
> Changes since V3:
>  - Change failure return from ERANGE to EINVAL
> Changes since V4:
>  - Rebase based on previous patch changes
> Changes since V5:
>  - return EINVAL instead of continue (Maarten)
> Changes since V6:
>  - Improve commit message
>  - Address review comment
> Changes since V7:
>  - use !enable instead of !active
>  - rename config variable for consistency (Maarten)
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  3 ++
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>  drivers/gpu/drm/i915/intel_pm.c      | 87 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7fa21df5bcd7..fb2e43d3eb5b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11177,6 +11177,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  			ret = skl_update_scaler_crtc(pipe_config);
>  
>  		if (!ret)
> +			ret = skl_check_pipe_max_pixel_rate(intel_crtc,
> +							    pipe_config);
> +		if (!ret)
>  			ret = intel_atomic_setup_scalers(dev_priv, intel_crtc,
>  							 pipe_config);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bd500977b3fc..93afac4a83fa 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1885,6 +1885,8 @@ bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
>  				 int ignore);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> +int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
> +				  struct intel_crtc_state *cstate);
>  static inline int intel_enable_rc6(void)
>  {
>  	return i915.enable_rc6;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 46aca5cbc047..ead346f1deb2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3868,6 +3868,93 @@ skl_plane_downscale_amount(const struct intel_crtc_state *cstate,
>  	return mul_fixed16(downscale_w, downscale_h);
>  }
>  
> +static uint_fixed_16_16_t
> +skl_pipe_downscale_amount(const struct intel_crtc_state *crtc_state)
> +{
> +	uint_fixed_16_16_t pipe_downscale = u32_to_fixed_16_16(1);
> +
> +	if (!crtc_state->base.enable)
> +		return pipe_downscale;
> +
> +	if (crtc_state->pch_pfit.enabled) {
> +		uint32_t src_w, src_h, dst_w, dst_h;
> +		uint32_t pfit_size = crtc_state->pch_pfit.size;
> +		uint_fixed_16_16_t fp_w_ratio, fp_h_ratio;
> +		uint_fixed_16_16_t downscale_h, downscale_w;
> +
> +		src_w = crtc_state->pipe_src_w;
> +		src_h = crtc_state->pipe_src_h;
> +		dst_w = pfit_size >> 16;
> +		dst_h = pfit_size & 0xffff;
> +
> +		if (!dst_w || !dst_h)
> +			return pipe_downscale;
> +
> +		fp_w_ratio = fixed_16_16_div(src_w, dst_w);
> +		fp_h_ratio = fixed_16_16_div(src_h, dst_h);
> +		downscale_w = max_fixed_16_16(fp_w_ratio, u32_to_fixed_16_16(1));
> +		downscale_h = max_fixed_16_16(fp_h_ratio, u32_to_fixed_16_16(1));
> +
> +		pipe_downscale = mul_fixed16(downscale_w, downscale_h);
> +	}
> +
> +	return pipe_downscale;
> +}
> +
> +int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
> +				  struct intel_crtc_state *cstate)
> +{
> +	struct drm_crtc_state *crtc_state = &cstate->base;
> +	struct drm_atomic_state *state = crtc_state->state;
> +	struct drm_plane *plane;
> +	const struct drm_plane_state *pstate;
> +	struct intel_plane_state *intel_pstate;
> +	int crtc_clock, cdclk;
> +	uint32_t pipe_max_pixel_rate;
> +	uint_fixed_16_16_t pipe_downscale;
> +	uint_fixed_16_16_t max_downscale = u32_to_fixed_16_16(1);
> +
> +	if (!cstate->base.enable)
> +		return 0;
> +
> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate, crtc_state) {
> +		uint_fixed_16_16_t plane_downscale;
> +		uint_fixed_16_16_t fp_9_div_8 = fixed_16_16_div(9, 8);
> +		int bpp;
> +
> +		if (!intel_wm_plane_visible(cstate,
> +					    to_intel_plane_state(pstate)))
> +			continue;
> +
> +		if (WARN_ON(!pstate->fb))
> +			return -EINVAL;
> +
> +		intel_pstate = to_intel_plane_state(pstate);
> +		plane_downscale = skl_plane_downscale_amount(cstate,
> +							     intel_pstate);
> +		bpp = pstate->fb->format->cpp[0] * 8;
> +		if (bpp == 64)
> +			plane_downscale = mul_fixed16(plane_downscale,
> +						      fp_9_div_8);
> +
> +		max_downscale = max_fixed_16_16(plane_downscale, max_downscale);
> +	}
> +	pipe_downscale = skl_pipe_downscale_amount(cstate);
> +
> +	pipe_downscale = mul_fixed16(pipe_downscale, max_downscale);
> +
> +	crtc_clock = crtc_state->adjusted_mode.crtc_clock;
> +	cdclk = to_intel_atomic_state(state)->cdclk.logical.cdclk;
> +	pipe_max_pixel_rate = div_round_up_u32_fixed16(cdclk, pipe_downscale);
> +
> +	if (pipe_max_pixel_rate < crtc_clock) {
> +		DRM_ERROR("Max supported pixel clock with scaling exceeded\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static unsigned int
>  skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
>  			     const struct drm_plane_state *pstate,

Applied the whole series, thanks.

~Maarten

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