Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1464760 > unrolled thread

[PATCH v12 0/7] Finally fix watermarks

Started byLyude <cpaul@redhat.com>
First post2016-08-17 22:00 +0200
Last post2016-08-22 09:30 +0200
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v12 0/7] Finally fix watermarks Lyude <cpaul@redhat.com> - 2016-08-17 22:00 +0200
    [PATCH v12 7/7] drm/i915/skl: Update DDB values atomically with wms/plane attrs Lyude <cpaul@redhat.com> - 2016-08-17 22:00 +0200
    [PATCH v12 6/7] drm/i915: Move CRTC updating in atomic_commit into it's own hook Lyude <cpaul@redhat.com> - 2016-08-17 22:00 +0200
    [PATCH v12 5/7] drm/i915/skl: Ensure pipes with changed wms get added to the state Lyude <cpaul@redhat.com> - 2016-08-17 22:00 +0200
    [PATCH v12 2/7] drm/i915/skl: Add support for the SAGV, fix underrun hangs Lyude <cpaul@redhat.com> - 2016-08-17 22:00 +0200
      Re: [PATCH v12 2/7] drm/i915/skl: Add support for the SAGV, fix  underrun hangs Maarten Lankhorst <maarten.lankhorst@linux.intel.com> - 2016-08-18 09:50 +0200
        Re: [PATCH v12 2/7] drm/i915/skl: Add support for the SAGV, fix  underrun hangs Maarten Lankhorst <maarten.lankhorst@linux.intel.com> - 2016-08-22 09:30 +0200

#1464760 — [PATCH v12 0/7] Finally fix watermarks

FromLyude <cpaul@redhat.com>
Date2016-08-17 22:00 +0200
Subject[PATCH v12 0/7] Finally fix watermarks
Message-ID<s7fah-5wE-3@gated-at.bofh.it>
Patches that actually changed (please re-review:
  drm/i915/gen6+: Interpret mailbox error flags
  drm/i915/skl: Add support for the SAGV, fix underrun hangs
  drm/i915/skl: Update DDB values atomically with wms/plane attrs

Everything else is the same. Updated version of

https://patchwork.freedesktop.org/series/10276/

Lyude (6):
  drm/i915/gen6+: Interpret mailbox error flags
  drm/i915/skl: Add support for the SAGV, fix underrun hangs
  drm/i915/skl: Update plane watermarks atomically during plane updates
  drm/i915/skl: Ensure pipes with changed wms get added to the state
  drm/i915: Move CRTC updating in atomic_commit into it's own hook
  drm/i915/skl: Update DDB values atomically with wms/plane attrs

Matt Roper (1):
  drm/i915/gen9: Only copy WM results for changed pipes to skl_hw

 drivers/gpu/drm/i915/i915_drv.h      |   9 +
 drivers/gpu/drm/i915/i915_reg.h      |  13 +
 drivers/gpu/drm/i915/intel_display.c | 190 +++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  15 ++
 drivers/gpu/drm/i915/intel_pm.c      | 473 +++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_sprite.c  |   6 +
 6 files changed, 512 insertions(+), 194 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1464761 — [PATCH v12 7/7] drm/i915/skl: Update DDB values atomically with wms/plane attrs

FromLyude <cpaul@redhat.com>
Date2016-08-17 22:00 +0200
Subject[PATCH v12 7/7] drm/i915/skl: Update DDB values atomically with wms/plane attrs
Message-ID<s7fai-5wE-13@gated-at.bofh.it>
In reply to#1464760
Now that we can hook into update_crtcs and control the order in which we
update CRTCs at each modeset, we can finish the final step of fixing
Skylake's watermark handling by performing DDB updates at the same time
as plane updates and watermark updates.

The first major change in this patch is skl_update_crtcs(), which
handles ensuring that we order each CRTC update in our atomic commits
properly so that they honor the DDB flush order.

The second major change in this patch is the order in which we flush the
pipes. While the previous order may have worked, it can't be used in
this approach since it no longer will do the right thing. For example,
using the old ddb flush order:

We have pipes A, B, and C enabled, and we're disabling C. Initial ddb
allocation looks like this:

|   A   |   B   |xxxxxxx|

Since we're performing the ddb updates after performing any CRTC
disablements in intel_atomic_commit_tail(), the space to the right of
pipe B is unallocated.

1. Flush pipes with new allocation contained into old space. None
   apply, so we skip this
2. Flush pipes having their allocation reduced, but overlapping with a
   previous allocation. None apply, so we also skip this
3. Flush pipes that got more space allocated. This applies to A and B,
   giving us the following update order: A, B

This is wrong, since updating pipe A first will cause it to overlap with
B and potentially burst into flames. Our new order (see the code
comments for details) would update the pipes in the proper order: B, A.

As well, we calculate the order for each DDB update during the check
phase, and reference it later in the commit phase when we hit
skl_update_crtcs().

This long overdue patch fixes the rest of the underruns on Skylake.

Changes since v1:
 - Add skl_ddb_entry_write() for cursor into skl_write_cursor_wm()
Changes since v2:
 - Use the method for updating CRTCs that Ville suggested
 - In skl_update_wm(), only copy the watermarks for the crtc that was
   passed to us
Changes since v3:
 - Small comment fix in skl_ddb_allocation_overlaps()
Changes since v4:
 - Remove the second loop in intel_update_crtcs() and use Ville's
   suggestion for updating the ddb allocations in the right order
 - Get rid of the second loop and just use the ddb state as it updates
   to determine what order to update everything in (thanks for the
   suggestion Ville)
 - Simplify skl_ddb_allocation_overlaps()
 - Split actual overlap checking into it's own helper

Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration")
Fixes: 8211bd5bdf5e ("drm/i915/skl: Program the DDB allocation")
[omitting CC for stable, since this patch will need to be changed for
such backports first]

Testcase: kms_cursor_legacy
Signed-off-by: Lyude <cpaul@redhat.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  93 +++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |   7 ++
 drivers/gpu/drm/i915/intel_pm.c      | 200 ++++++++---------------------------
 3 files changed, 132 insertions(+), 168 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4b078e8..3aa9109 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13361,16 +13361,23 @@ static void verify_wm_state(struct drm_crtc *crtc,
 			  hw_entry->start, hw_entry->end);
 	}
 
-	/* cursor */
-	hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
-	sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
-
-	if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
-		DRM_ERROR("mismatch in DDB state pipe %c cursor "
-			  "(expected (%u,%u), found (%u,%u))\n",
-			  pipe_name(pipe),
-			  sw_entry->start, sw_entry->end,
-			  hw_entry->start, hw_entry->end);
+	/*
+	 * cursor
+	 * If the cursor plane isn't active, we may not have updated it's ddb
+	 * allocation. In that case since the ddb allocation will be updated
+	 * once the plane becomes visible, we can skip this check
+	 */
+	if (intel_crtc->cursor_addr) {
+		hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
+		sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
+
+		if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
+			DRM_ERROR("mismatch in DDB state pipe %c cursor "
+				  "(expected (%u,%u), found (%u,%u))\n",
+				  pipe_name(pipe),
+				  sw_entry->start, sw_entry->end,
+				  hw_entry->start, hw_entry->end);
+		}
 	}
 }
 
@@ -14122,6 +14129,65 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
 	}
 }
 
+static void skl_update_crtcs(struct drm_atomic_state *state,
+			     unsigned int *crtc_vblank_mask)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state;
+	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
+	struct skl_ddb_allocation *cur_ddb = &dev_priv->wm.skl_hw.ddb;
+	unsigned int updated = 0;
+	bool progress;
+	enum pipe pipe;
+
+	/*
+	 * Whenever the number of active pipes changes, we need to make sure we 
+	 * update the pipes in the right order so that their ddb allocations
+	 * never overlap with eachother inbetween CRTC updates. Otherwise we'll
+	 * cause pipe underruns and other bad stuff.
+	 */
+	do {
+		int i;
+		progress = false;
+
+		for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
+			bool vbl_wait = false;
+			unsigned int cmask = drm_crtc_mask(crtc);
+			pipe = to_intel_crtc(crtc)->pipe;
+
+			if (updated & cmask || !crtc->state->active)
+				continue;
+			if (skl_ddb_allocation_overlaps(state, cur_ddb, new_ddb,
+							pipe))
+				continue;
+
+			updated |= cmask;
+
+			/*
+			 * If this is an already active pipe, it's DDB changed,
+			 * and this isn't the last pipe that needs updating
+			 * then we need to wait for a vblank to pass for the
+			 * new ddb allocation to take effect.
+			 */
+			if (!skl_ddb_allocation_equals(cur_ddb, new_ddb, pipe) &&
+			    !crtc->state->active_changed &&
+			    intel_state->wm_results.dirty_pipes != updated)
+				vbl_wait = true;
+
+			intel_update_crtc(crtc, state, old_crtc_state,
+					  crtc_vblank_mask);
+
+			if (vbl_wait)
+				intel_wait_for_vblank(dev, pipe);
+
+			progress = true;
+		}
+	} while (progress);
+}
+
 static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
@@ -15761,8 +15827,6 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 	}
 
-	dev_priv->display.update_crtcs = intel_update_crtcs;
-
 	/* Returns the core display clock speed */
 	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
 		dev_priv->display.get_display_clock_speed =
@@ -15852,6 +15916,11 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 			skl_modeset_calc_cdclk;
 	}
 
+	if (dev_priv->info.gen >= 9)
+		dev_priv->display.update_crtcs = skl_update_crtcs;
+	else
+		dev_priv->display.update_crtcs = intel_update_crtcs;
+
 	switch (INTEL_INFO(dev_priv)->gen) {
 	case 2:
 		dev_priv->display.queue_flip = intel_gen2_queue_flip;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1d31cca..7bf8209 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1725,6 +1725,13 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 bool skl_can_enable_sagv(struct drm_atomic_state *state);
 int skl_enable_sagv(struct drm_i915_private *dev_priv);
 int skl_disable_sagv(struct drm_i915_private *dev_priv);
+bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
+			       const struct skl_ddb_allocation *new,
+			       enum pipe pipe);
+bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
+				 const struct skl_ddb_allocation *old,
+				 const struct skl_ddb_allocation *new,
+				 enum pipe pipe);
 void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
 			 const struct skl_wm_values *wm);
 void skl_write_plane_wm(struct intel_crtc *intel_crtc,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a3d24cb..54f014c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3853,6 +3853,11 @@ void skl_write_plane_wm(struct intel_crtc *intel_crtc,
 			   wm->plane[pipe][plane][level]);
 	}
 	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
+
+	skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane),
+			    &wm->ddb.plane[pipe][plane]);
+	skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, plane),
+			    &wm->ddb.y_plane[pipe][plane]);
 }
 
 void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
@@ -3869,170 +3874,46 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
 			   wm->plane[pipe][PLANE_CURSOR][level]);
 	}
 	I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
-}
-
-static void skl_write_wm_values(struct drm_i915_private *dev_priv,
-				const struct skl_wm_values *new)
-{
-	struct drm_device *dev = &dev_priv->drm;
-	struct intel_crtc *crtc;
-
-	for_each_intel_crtc(dev, crtc) {
-		int i;
-		enum pipe pipe = crtc->pipe;
-
-		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
-			continue;
-		if (!crtc->active)
-			continue;
-
-		for (i = 0; i < intel_num_planes(crtc); i++) {
-			skl_ddb_entry_write(dev_priv,
-					    PLANE_BUF_CFG(pipe, i),
-					    &new->ddb.plane[pipe][i]);
-			skl_ddb_entry_write(dev_priv,
-					    PLANE_NV12_BUF_CFG(pipe, i),
-					    &new->ddb.y_plane[pipe][i]);
-		}
 
-		skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
-				    &new->ddb.plane[pipe][PLANE_CURSOR]);
-	}
+	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
+			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
 }
 
-/*
- * When setting up a new DDB allocation arrangement, we need to correctly
- * sequence the times at which the new allocations for the pipes are taken into
- * account or we'll have pipes fetching from space previously allocated to
- * another pipe.
- *
- * Roughly the sequence looks like:
- *  1. re-allocate the pipe(s) with the allocation being reduced and not
- *     overlapping with a previous light-up pipe (another way to put it is:
- *     pipes with their new allocation strickly included into their old ones).
- *  2. re-allocate the other pipes that get their allocation reduced
- *  3. allocate the pipes having their allocation increased
- *
- * Steps 1. and 2. are here to take care of the following case:
- * - Initially DDB looks like this:
- *     |   B    |   C    |
- * - enable pipe A.
- * - pipe B has a reduced DDB allocation that overlaps with the old pipe C
- *   allocation
- *     |  A  |  B  |  C  |
- *
- * We need to sequence the re-allocation: C, B, A (and not B, C, A).
- */
-
-static void
-skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, int pass)
+bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
+			       const struct skl_ddb_allocation *new,
+			       enum pipe pipe)
 {
-	int plane;
-
-	DRM_DEBUG_KMS("flush pipe %c (pass %d)\n", pipe_name(pipe), pass);
-
-	for_each_plane(dev_priv, pipe, plane) {
-		I915_WRITE(PLANE_SURF(pipe, plane),
-			   I915_READ(PLANE_SURF(pipe, plane)));
-	}
-	I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
+	return new->pipe[pipe].start == old->pipe[pipe].start &&
+	       new->pipe[pipe].end == old->pipe[pipe].end;
 }
 
-static bool
-skl_ddb_allocation_included(const struct skl_ddb_allocation *old,
-			    const struct skl_ddb_allocation *new,
-			    enum pipe pipe)
+static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
+					   const struct skl_ddb_entry *b)
 {
-	uint16_t old_size, new_size;
-
-	old_size = skl_ddb_entry_size(&old->pipe[pipe]);
-	new_size = skl_ddb_entry_size(&new->pipe[pipe]);
-
-	return old_size != new_size &&
-	       new->pipe[pipe].start >= old->pipe[pipe].start &&
-	       new->pipe[pipe].end <= old->pipe[pipe].end;
+	return a->start < b->end && b->start < a->end;
 }
 
-static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
-				struct skl_wm_values *new_values)
+bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
+				 const struct skl_ddb_allocation *old,
+				 const struct skl_ddb_allocation *new,
+				 enum pipe pipe)
 {
-	struct drm_device *dev = &dev_priv->drm;
-	struct skl_ddb_allocation *cur_ddb, *new_ddb;
-	bool reallocated[I915_MAX_PIPES] = {};
-	struct intel_crtc *crtc;
-	enum pipe pipe;
-
-	new_ddb = &new_values->ddb;
-	cur_ddb = &dev_priv->wm.skl_hw.ddb;
-
-	/*
-	 * First pass: flush the pipes with the new allocation contained into
-	 * the old space.
-	 *
-	 * We'll wait for the vblank on those pipes to ensure we can safely
-	 * re-allocate the freed space without this pipe fetching from it.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		pipe = crtc->pipe;
-
-		if (!skl_ddb_allocation_included(cur_ddb, new_ddb, pipe))
-			continue;
-
-		skl_wm_flush_pipe(dev_priv, pipe, 1);
-		intel_wait_for_vblank(dev, pipe);
-
-		reallocated[pipe] = true;
-	}
-
-
-	/*
-	 * Second pass: flush the pipes that are having their allocation
-	 * reduced, but overlapping with a previous allocation.
-	 *
-	 * Here as well we need to wait for the vblank to make sure the freed
-	 * space is not used anymore.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
+	struct drm_device *dev = state->dev;
+	struct intel_crtc *intel_crtc;
+	enum pipe otherp;
 
-		pipe = crtc->pipe;
+	for_each_intel_crtc(dev, intel_crtc) {
+		otherp = intel_crtc->pipe;
 
-		if (reallocated[pipe])
+		if (otherp == pipe)
 			continue;
 
-		if (skl_ddb_entry_size(&new_ddb->pipe[pipe]) <
-		    skl_ddb_entry_size(&cur_ddb->pipe[pipe])) {
-			skl_wm_flush_pipe(dev_priv, pipe, 2);
-			intel_wait_for_vblank(dev, pipe);
-			reallocated[pipe] = true;
-		}
+		if (skl_ddb_entries_overlap(&new->pipe[pipe],
+					    &old->pipe[otherp]))
+			return true;
 	}
 
-	/*
-	 * Third pass: flush the pipes that got more space allocated.
-	 *
-	 * We don't need to actively wait for the update here, next vblank
-	 * will just get more DDB space with the correct WM values.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		pipe = crtc->pipe;
-
-		/*
-		 * At this point, only the pipes more space than before are
-		 * left to re-allocate.
-		 */
-		if (reallocated[pipe])
-			continue;
-
-		skl_wm_flush_pipe(dev_priv, pipe, 3);
-	}
+	return false;
 }
 
 static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
@@ -4219,7 +4100,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
-	int pipe;
+	enum pipe pipe = intel_crtc->pipe;
 
 	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
 		return;
@@ -4228,15 +4109,22 @@ static void skl_update_wm(struct drm_crtc *crtc)
 
 	mutex_lock(&dev_priv->wm.wm_mutex);
 
-	skl_write_wm_values(dev_priv, results);
-	skl_flush_wm_values(dev_priv, results);
-
 	/*
-	 * Store the new configuration (but only for the pipes that have
-	 * changed; the other values weren't recomputed).
+	 * If this pipe isn't active already, we're going to be enabling it
+	 * very soon. Since it's safe to update a pipe's ddb allocation while
+	 * the pipe's shut off, just do so here. Already active pipes will have
+	 * their watermarks updated once we update their planes.
 	 */
-	for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes)
-		skl_copy_wm_for_pipe(hw_vals, results, pipe);
+	if (crtc->state->active_changed) {
+		int plane;
+
+		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
+			skl_write_plane_wm(intel_crtc, results, plane);
+
+		skl_write_cursor_wm(intel_crtc, results);
+	}
+
+	skl_copy_wm_for_pipe(hw_vals, results, pipe);
 
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1464762 — [PATCH v12 6/7] drm/i915: Move CRTC updating in atomic_commit into it's own hook

FromLyude <cpaul@redhat.com>
Date2016-08-17 22:00 +0200
Subject[PATCH v12 6/7] drm/i915: Move CRTC updating in atomic_commit into it's own hook
Message-ID<s7fai-5wE-19@gated-at.bofh.it>
In reply to#1464760
Since we have to write ddb allocations at the same time as we do other
plane updates, we're going to need to be able to control the order in
which we execute modesets on each pipe. The easiest way to do this is to
just factor this section of intel_atomic_commit_tail()
(intel_atomic_commit() for stable branches) into it's own function, and
add an appropriate display function hook for it.

Based off of Matt Rope's suggestions

Changes since v1:
 - Drop pipe_config->base.active check in intel_update_crtcs() since we
   check that before calling the function

Signed-off-by: Lyude <cpaul@redhat.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
[omitting CC for stable, since this patch will need to be changed for
such backports first]
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +
 drivers/gpu/drm/i915/intel_display.c | 74 +++++++++++++++++++++++++-----------
 2 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f20530bb..2565624 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -506,6 +506,8 @@ struct drm_i915_display_funcs {
 				  struct intel_crtc_state *crtc_state);
 	void (*crtc_enable)(struct drm_crtc *crtc);
 	void (*crtc_disable)(struct drm_crtc *crtc);
+	void (*update_crtcs)(struct drm_atomic_state *state,
+			     unsigned int *crtc_vblank_mask);
 	void (*audio_codec_enable)(struct drm_connector *connector,
 				   struct intel_encoder *encoder,
 				   const struct drm_display_mode *adjusted_mode);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ca429b5..4b078e8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14076,6 +14076,52 @@ static bool needs_vblank_wait(struct intel_crtc_state *crtc_state)
 	return false;
 }
 
+static void intel_update_crtc(struct drm_crtc *crtc,
+			      struct drm_atomic_state *state,
+			      struct drm_crtc_state *old_crtc_state,
+			      unsigned int *crtc_vblank_mask)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc->state);
+	bool modeset = needs_modeset(crtc->state);
+
+	if (modeset) {
+		update_scanline_offset(intel_crtc);
+		dev_priv->display.crtc_enable(crtc);
+	} else {
+		intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
+	}
+
+	if (drm_atomic_get_existing_plane_state(state, crtc->primary)) {
+		intel_fbc_enable(
+		    intel_crtc, pipe_config,
+		    to_intel_plane_state(crtc->primary->state));
+	}
+
+	drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
+
+	if (needs_vblank_wait(pipe_config))
+		*crtc_vblank_mask |= drm_crtc_mask(crtc);
+}
+
+static void intel_update_crtcs(struct drm_atomic_state *state,
+			       unsigned int *crtc_vblank_mask)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state;
+	int i;
+
+	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
+		if (!crtc->state->active)
+			continue;
+
+		intel_update_crtc(crtc, state, old_crtc_state,
+				  crtc_vblank_mask);
+	}
+}
+
 static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
@@ -14174,17 +14220,9 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		intel_modeset_verify_disabled(dev);
 	}
 
-	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
+	/* Complete the events for pipes that have now been disabled */
 	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
-		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 		bool modeset = needs_modeset(crtc->state);
-		struct intel_crtc_state *pipe_config =
-			to_intel_crtc_state(crtc->state);
-
-		if (modeset && crtc->state->active) {
-			update_scanline_offset(to_intel_crtc(crtc));
-			dev_priv->display.crtc_enable(crtc);
-		}
 
 		/* Complete events for now disable pipes here. */
 		if (modeset && !crtc->state->active && crtc->state->event) {
@@ -14194,21 +14232,11 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 
 			crtc->state->event = NULL;
 		}
-
-		if (!modeset)
-			intel_pre_plane_update(to_intel_crtc_state(old_crtc_state));
-
-		if (crtc->state->active &&
-		    drm_atomic_get_existing_plane_state(state, crtc->primary))
-			intel_fbc_enable(intel_crtc, pipe_config, to_intel_plane_state(crtc->primary->state));
-
-		if (crtc->state->active)
-			drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
-
-		if (pipe_config->base.active && needs_vblank_wait(pipe_config))
-			crtc_vblank_mask |= 1 << i;
 	}
 
+	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
+	dev_priv->display.update_crtcs(state, &crtc_vblank_mask);
+
 	/* FIXME: We should call drm_atomic_helper_commit_hw_done() here
 	 * already, but still need the state for the delayed optimization. To
 	 * fix this:
@@ -15733,6 +15761,8 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 	}
 
+	dev_priv->display.update_crtcs = intel_update_crtcs;
+
 	/* Returns the core display clock speed */
 	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
 		dev_priv->display.get_display_clock_speed =
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1464764 — [PATCH v12 5/7] drm/i915/skl: Ensure pipes with changed wms get added to the state

FromLyude <cpaul@redhat.com>
Date2016-08-17 22:00 +0200
Subject[PATCH v12 5/7] drm/i915/skl: Ensure pipes with changed wms get added to the state
Message-ID<s7fai-5wE-25@gated-at.bofh.it>
In reply to#1464760
If we're enabling a pipe, we'll need to modify the watermarks on all
active planes. Since those planes won't be added to the state on
their own, we need to add them ourselves.

Signed-off-by: Lyude <cpaul@redhat.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 849f039..a3d24cb 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4117,6 +4117,10 @@ skl_compute_ddb(struct drm_atomic_state *state)
 		ret = skl_allocate_pipe_ddb(cstate, ddb);
 		if (ret)
 			return ret;
+
+		ret = drm_atomic_add_affected_planes(state, &intel_crtc->base);
+		if (ret)
+			return ret;
 	}
 
 	return 0;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1464769 — [PATCH v12 2/7] drm/i915/skl: Add support for the SAGV, fix underrun hangs

FromLyude <cpaul@redhat.com>
Date2016-08-17 22:00 +0200
Subject[PATCH v12 2/7] drm/i915/skl: Add support for the SAGV, fix underrun hangs
Message-ID<s7fai-5wE-33@gated-at.bofh.it>
In reply to#1464760
Since the watermark calculations for Skylake are still broken, we're apt
to hitting underruns very easily under multi-monitor configurations.
While it would be lovely if this was fixed, it's not. Another problem
that's been coming from this however, is the mysterious issue of
underruns causing full system hangs. An easy way to reproduce this with
a skylake system:

- Get a laptop with a skylake GPU, and hook up two external monitors to
  it
- Move the cursor from the built-in LCD to one of the external displays
  as quickly as you can
- You'll get a few pipe underruns, and eventually the entire system will
  just freeze.

After doing a lot of investigation and reading through the bspec, I
found the existence of the SAGV, which is responsible for adjusting the
system agent voltage and clock frequencies depending on how much power
we need. According to the bspec:

"The display engine access to system memory is blocked during the
 adjustment time. SAGV defaults to enabled. Software must use the
 GT-driver pcode mailbox to disable SAGV when the display engine is not
 able to tolerate the blocking time."

The rest of the bspec goes on to explain that software can simply leave
the SAGV enabled, and disable it when we use interlaced pipes/have more
then one pipe active.

Sure enough, with this patchset the system hangs resulting from pipe
underruns on Skylake have completely vanished on my T460s. Additionally,
the bspec mentions turning off the SAGV	with more then one pipe enabled
as a workaround for display underruns. While this patch doesn't entirely
fix that, it looks like it does improve the situation a little bit so
it's likely this is going to be required to make watermarks on Skylake
fully functional.

This will still need additional work in the future: we shouldn't be
enabling the SAGV if any of the currently enabled planes can't enable WM
levels that introduce latencies >= 30 µs.

Changes since v11:
 - Add skl_can_enable_sagv()
 - Make sure we don't enable SAGV when not all planes can enable
   watermarks >= the SAGV engine block time. I was originally going to
   save this for later, but I recently managed to run into a machine
   that was having problems with a single pipe configuration + SAGV.
 - Make comparisons to I915_SKL_SAGV_NOT_CONTROLLED explicit
 - Change I915_SAGV_DYNAMIC_FREQ to I915_SAGV_ENABLE
 - Move printks outside of mutexes
 - Don't print error messages twice
Changes since v10:
 - Apparently sandybridge_pcode_read actually writes values and reads
   them back, despite it's misleading function name. This means we've
   been doing this mostly wrong and have been writing garbage to the
   SAGV control. Because of this, we no longer attempt to read the SAGV
   status during initialization (since there are no helpers for this).
 - mlankhorst noticed that this patch was breaking on some very early
   pre-release Skylake machines, which apparently don't allow you to
   disable the SAGV. To prevent machines from failing tests due to SAGV
   errors, if the first time we try to control the SAGV results in the
   mailbox indicating an invalid command, we just disable future attempts
   to control the SAGV state by setting dev_priv->skl_sagv_status to
   I915_SKL_SAGV_NOT_CONTROLLED and make a note of it in dmesg.
 - Move mutex_unlock() a little higher in skl_enable_sagv(). This
   doesn't actually fix anything, but lets us release the lock a little
   sooner since we're finished with it.
Changes since v9:
 - Only enable/disable sagv on Skylake
Changes since v8:
 - Add intel_state->modeset guard to the conditional for
   skl_enable_sagv()
Changes since v7:
 - Remove GEN9_SAGV_LOW_FREQ, replace with GEN9_SAGV_IS_ENABLED (that's
   all we use it for anyway)
 - Use GEN9_SAGV_IS_ENABLED instead of 0x1 for clarification
 - Fix a styling error that snuck past me
Changes since v6:
 - Protect skl_enable_sagv() with intel_state->modeset conditional in
   intel_atomic_commit_tail()
Changes since v5:
 - Don't use is_power_of_2. Makes things confusing
 - Don't use the old state to figure out whether or not to
   enable/disable the sagv, use the new one
 - Split the loop in skl_disable_sagv into it's own function
 - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail()
Changes since v4:
 - Use is_power_of_2 against active_crtcs to check whether we have > 1
   pipe enabled
 - Fix skl_sagv_get_hw_state(): (temp & 0x1) indicates disabled, 0x0
   enabled
 - Call skl_sagv_enable/disable() from pre/post-plane updates
Changes since v3:
 - Use time_before() to compare timeout to jiffies
Changes since v2:
 - Really apply minor style nitpicks to patch this time
Changes since v1:
 - Added comments about this probably being one of the requirements to
   fixing Skylake's watermark issues
 - Minor style nitpicks from Matt Roper
 - Disable these functions on Broxton, since it doesn't have an SAGV

Signed-off-by: Lyude <cpaul@redhat.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_drv.h      |   7 ++
 drivers/gpu/drm/i915/i915_reg.h      |   4 +
 drivers/gpu/drm/i915/intel_display.c |  11 +++
 drivers/gpu/drm/i915/intel_drv.h     |   3 +
 drivers/gpu/drm/i915/intel_pm.c      | 148 +++++++++++++++++++++++++++++++++++
 5 files changed, 173 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 35caa9b..f20530bb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1949,6 +1949,13 @@ struct drm_i915_private {
 	struct i915_suspend_saved_registers regfile;
 	struct vlv_s0ix_state vlv_s0ix_state;
 
+	enum {
+		I915_SKL_SAGV_UNKNOWN = 0,
+		I915_SKL_SAGV_DISABLED,
+		I915_SKL_SAGV_ENABLED,
+		I915_SKL_SAGV_NOT_CONTROLLED
+	} skl_sagv_status;
+
 	struct {
 		/*
 		 * Raw watermark latency values:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7419fbf..be82c49 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7153,6 +7153,10 @@ enum {
 #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
 #define   DISPLAY_IPS_CONTROL			0x19
 #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
+#define   GEN9_PCODE_SAGV_CONTROL		0x21
+#define     GEN9_SAGV_DISABLE			0x0
+#define     GEN9_SAGV_IS_DISABLED		0x1
+#define     GEN9_SAGV_ENABLE 	             0x3
 #define GEN6_PCODE_DATA				_MMIO(0x138128)
 #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
 #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 781d22e..ca4b83f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14156,6 +14156,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		     intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco))
 			dev_priv->display.modeset_commit_cdclk(state);
 
+		/*
+		 * SKL workaround: bspec recommends we disable the SAGV when we
+		 * have more then one pipe enabled
+		 */
+		if (IS_SKYLAKE(dev_priv) && !skl_can_enable_sagv(state))
+			skl_disable_sagv(dev_priv);
+
 		intel_modeset_verify_disabled(dev);
 	}
 
@@ -14229,6 +14236,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
 	}
 
+	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
+	    skl_can_enable_sagv(state))
+		skl_enable_sagv(dev_priv);
+
 	drm_atomic_helper_commit_hw_done(state);
 
 	if (intel_state->modeset)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1c700b0..d203c77 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1722,6 +1722,9 @@ void ilk_wm_get_hw_state(struct drm_device *dev);
 void skl_wm_get_hw_state(struct drm_device *dev);
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */);
+bool skl_can_enable_sagv(struct drm_atomic_state *state);
+int skl_enable_sagv(struct drm_i915_private *dev_priv);
+int skl_disable_sagv(struct drm_i915_private *dev_priv);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b4cf988..fed2bae8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2860,6 +2860,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev)
 
 #define SKL_DDB_SIZE		896	/* in blocks */
 #define BXT_DDB_SIZE		512
+#define SKL_SAGV_BLOCK_TIME 30 /* µs */
 
 /*
  * Return the index of a plane in the SKL DDB and wm result arrays.  Primary
@@ -2883,6 +2884,153 @@ skl_wm_plane_id(const struct intel_plane *plane)
 	}
 }
 
+/*
+ * SAGV dynamically adjusts the system agent voltage and clock frequencies
+ * depending on power and performance requirements. The display engine access
+ * to system memory is blocked during the adjustment time. Because of the
+ * blocking time, having this enabled can cause full system hangs and/or pipe
+ * underruns if we don't meet all of the following requirements:
+ *
+ *  - <= 1 pipe enabled
+ *  - All planes can enable watermarks for latencies >= SAGV engine block time
+ *  - We're not using an interlaced display configuration
+ */
+int
+skl_enable_sagv(struct drm_i915_private *dev_priv)
+{
+	int ret;
+
+	if (dev_priv->skl_sagv_status == I915_SKL_SAGV_NOT_CONTROLLED ||
+	    dev_priv->skl_sagv_status == I915_SKL_SAGV_ENABLED)
+		return 0;
+
+	DRM_DEBUG_KMS("Enabling the SAGV\n");
+	mutex_lock(&dev_priv->rps.hw_lock);
+
+	ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+				      GEN9_SAGV_ENABLE);
+
+	/* We don't need to wait for the SAGV when enabling */
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	/*
+	 * Some skl systems, pre-release machines in particular,
+	 * don't actually have an SAGV.
+	 */
+	if (ret == -ENOSYS) {
+		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
+		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
+		return 0;
+	} else if (ret < 0) {
+		DRM_ERROR("Failed to enable the SAGV\n");
+		return ret;
+	}
+
+	dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED;
+	return 0;
+}
+
+static int
+skl_do_sagv_disable(struct drm_i915_private *dev_priv)
+{
+	int ret;
+	uint32_t temp = GEN9_SAGV_DISABLE;
+
+	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+				     &temp);
+	if (ret)
+		return ret;
+	else
+		return temp & GEN9_SAGV_IS_DISABLED;
+}
+
+int
+skl_disable_sagv(struct drm_i915_private *dev_priv)
+{
+	int ret, result;
+
+	if (dev_priv->skl_sagv_status == I915_SKL_SAGV_NOT_CONTROLLED ||
+	    dev_priv->skl_sagv_status == I915_SKL_SAGV_DISABLED)
+		return 0;
+
+	DRM_DEBUG_KMS("Disabling the SAGV\n");
+	mutex_lock(&dev_priv->rps.hw_lock);
+
+	/* bspec says to keep retrying for at least 1 ms */
+	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	if (ret == -ETIMEDOUT) {
+		DRM_ERROR("Request to disable SAGV timed out\n");
+		return -ETIMEDOUT;
+	}
+
+	/*
+	 * Some skl systems, pre-release machines in particular,
+	 * don't actually have an SAGV.
+	 */
+	if (result == -ENOSYS) {
+		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
+		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
+		return 0;
+	} else if (result < 0) {
+		DRM_ERROR("Failed to disable the SAGV\n");
+		return result;
+	}
+
+	dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED;
+	return 0;
+}
+
+bool skl_can_enable_sagv(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_crtc *crtc;
+	enum pipe pipe;
+	int level, plane;
+
+	/*
+	 * SKL workaround: bspec recommends we disable the SAGV when we have
+	 * more then one pipe enabled
+	 *
+	 * If there are no active CRTCs, no additional checks need be performed
+	 */
+	if (hweight32(intel_state->active_crtcs) == 0)
+		return true;
+	else if (hweight32(intel_state->active_crtcs) > 1)
+		return false;
+
+	/* Since we're now guaranteed to only have one active CRTC... */
+	pipe = ffs(intel_state->active_crtcs) - 1;
+	crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+
+	if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE)
+		return false;
+
+	for_each_plane(dev_priv, pipe, plane) {
+		/* Skip this plane if it's not enabled */
+		if (intel_state->wm_results.plane[pipe][plane][0] == 0)
+			continue;
+
+		/* Find the highest enabled wm level for this plane */
+		for (level = ilk_wm_max_level(dev);
+		     intel_state->wm_results.plane[pipe][plane][level] == 0;
+		     --level);
+
+		/*
+		 * If any of the planes on this pipe don't enable wm levels
+		 * that incur memory latencies higher then 30µs we can't enable
+		 * the SAGV
+		 */
+		if (dev_priv->wm.skl_latency[level] < SKL_SAGV_BLOCK_TIME)
+			return false;
+	}
+
+	return true;
+}
+
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				   const struct intel_crtc_state *cstate,
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1464981 — Re: [PATCH v12 2/7] drm/i915/skl: Add support for the SAGV, fix underrun hangs

FromMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date2016-08-18 09:50 +0200
SubjectRe: [PATCH v12 2/7] drm/i915/skl: Add support for the SAGV, fix underrun hangs
Message-ID<s7qfn-55N-5@gated-at.bofh.it>
In reply to#1464769
Hey,

Op 17-08-16 om 21:55 schreef Lyude:
> Since the watermark calculations for Skylake are still broken, we're apt
> to hitting underruns very easily under multi-monitor configurations.
> While it would be lovely if this was fixed, it's not. Another problem
> that's been coming from this however, is the mysterious issue of
> underruns causing full system hangs. An easy way to reproduce this with
> a skylake system:
>
> - Get a laptop with a skylake GPU, and hook up two external monitors to
>   it
> - Move the cursor from the built-in LCD to one of the external displays
>   as quickly as you can
> - You'll get a few pipe underruns, and eventually the entire system will
>   just freeze.
>
> After doing a lot of investigation and reading through the bspec, I
> found the existence of the SAGV, which is responsible for adjusting the
> system agent voltage and clock frequencies depending on how much power
> we need. According to the bspec:
>
> "The display engine access to system memory is blocked during the
>  adjustment time. SAGV defaults to enabled. Software must use the
>  GT-driver pcode mailbox to disable SAGV when the display engine is not
>  able to tolerate the blocking time."
>
> The rest of the bspec goes on to explain that software can simply leave
> the SAGV enabled, and disable it when we use interlaced pipes/have more
> then one pipe active.
>
> Sure enough, with this patchset the system hangs resulting from pipe
> underruns on Skylake have completely vanished on my T460s. Additionally,
> the bspec mentions turning off the SAGV	with more then one pipe enabled
> as a workaround for display underruns. While this patch doesn't entirely
> fix that, it looks like it does improve the situation a little bit so
> it's likely this is going to be required to make watermarks on Skylake
> fully functional.
>
> This will still need additional work in the future: we shouldn't be
> enabling the SAGV if any of the currently enabled planes can't enable WM
> levels that introduce latencies >= 30 µs.
>
> Changes since v11:
>  - Add skl_can_enable_sagv()
>  - Make sure we don't enable SAGV when not all planes can enable
>    watermarks >= the SAGV engine block time. I was originally going to
>    save this for later, but I recently managed to run into a machine
>    that was having problems with a single pipe configuration + SAGV.
>  - Make comparisons to I915_SKL_SAGV_NOT_CONTROLLED explicit
>  - Change I915_SAGV_DYNAMIC_FREQ to I915_SAGV_ENABLE
>  - Move printks outside of mutexes
>  - Don't print error messages twice
> Changes since v10:
>  - Apparently sandybridge_pcode_read actually writes values and reads
>    them back, despite it's misleading function name. This means we've
>    been doing this mostly wrong and have been writing garbage to the
>    SAGV control. Because of this, we no longer attempt to read the SAGV
>    status during initialization (since there are no helpers for this).
>  - mlankhorst noticed that this patch was breaking on some very early
>    pre-release Skylake machines, which apparently don't allow you to
>    disable the SAGV. To prevent machines from failing tests due to SAGV
>    errors, if the first time we try to control the SAGV results in the
>    mailbox indicating an invalid command, we just disable future attempts
>    to control the SAGV state by setting dev_priv->skl_sagv_status to
>    I915_SKL_SAGV_NOT_CONTROLLED and make a note of it in dmesg.
>  - Move mutex_unlock() a little higher in skl_enable_sagv(). This
>    doesn't actually fix anything, but lets us release the lock a little
>    sooner since we're finished with it.
> Changes since v9:
>  - Only enable/disable sagv on Skylake
> Changes since v8:
>  - Add intel_state->modeset guard to the conditional for
>    skl_enable_sagv()
> Changes since v7:
>  - Remove GEN9_SAGV_LOW_FREQ, replace with GEN9_SAGV_IS_ENABLED (that's
>    all we use it for anyway)
>  - Use GEN9_SAGV_IS_ENABLED instead of 0x1 for clarification
>  - Fix a styling error that snuck past me
> Changes since v6:
>  - Protect skl_enable_sagv() with intel_state->modeset conditional in
>    intel_atomic_commit_tail()
> Changes since v5:
>  - Don't use is_power_of_2. Makes things confusing
>  - Don't use the old state to figure out whether or not to
>    enable/disable the sagv, use the new one
>  - Split the loop in skl_disable_sagv into it's own function
>  - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail()
> Changes since v4:
>  - Use is_power_of_2 against active_crtcs to check whether we have > 1
>    pipe enabled
>  - Fix skl_sagv_get_hw_state(): (temp & 0x1) indicates disabled, 0x0
>    enabled
>  - Call skl_sagv_enable/disable() from pre/post-plane updates
> Changes since v3:
>  - Use time_before() to compare timeout to jiffies
> Changes since v2:
>  - Really apply minor style nitpicks to patch this time
> Changes since v1:
>  - Added comments about this probably being one of the requirements to
>    fixing Skylake's watermark issues
>  - Minor style nitpicks from Matt Roper
>  - Disable these functions on Broxton, since it doesn't have an SAGV
>
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   7 ++
>  drivers/gpu/drm/i915/i915_reg.h      |   4 +
>  drivers/gpu/drm/i915/intel_display.c |  11 +++
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +
>  drivers/gpu/drm/i915/intel_pm.c      | 148 +++++++++++++++++++++++++++++++++++
>  5 files changed, 173 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 35caa9b..f20530bb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1949,6 +1949,13 @@ struct drm_i915_private {
>  	struct i915_suspend_saved_registers regfile;
>  	struct vlv_s0ix_state vlv_s0ix_state;
>  
> +	enum {
> +		I915_SKL_SAGV_UNKNOWN = 0,
> +		I915_SKL_SAGV_DISABLED,
> +		I915_SKL_SAGV_ENABLED,
> +		I915_SKL_SAGV_NOT_CONTROLLED
> +	} skl_sagv_status;
> +
>  	struct {
>  		/*
>  		 * Raw watermark latency values:
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7419fbf..be82c49 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7153,6 +7153,10 @@ enum {
>  #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
>  #define   DISPLAY_IPS_CONTROL			0x19
>  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> +#define   GEN9_PCODE_SAGV_CONTROL		0x21
> +#define     GEN9_SAGV_DISABLE			0x0
> +#define     GEN9_SAGV_IS_DISABLED		0x1
> +#define     GEN9_SAGV_ENABLE 	             0x3
>  #define GEN6_PCODE_DATA				_MMIO(0x138128)
>  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
>  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 781d22e..ca4b83f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14156,6 +14156,13 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		     intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco))
>  			dev_priv->display.modeset_commit_cdclk(state);
>  
> +		/*
> +		 * SKL workaround: bspec recommends we disable the SAGV when we
> +		 * have more then one pipe enabled
> +		 */
> +		if (IS_SKYLAKE(dev_priv) && !skl_can_enable_sagv(state))
> +			skl_disable_sagv(dev_priv);
> +
>  		intel_modeset_verify_disabled(dev);
>  	}
>  
> @@ -14229,6 +14236,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
>  	}
>  
> +	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
> +	    skl_can_enable_sagv(state))
> +		skl_enable_sagv(dev_priv);
> +
>  	drm_atomic_helper_commit_hw_done(state);
>  
>  	if (intel_state->modeset)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1c700b0..d203c77 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1722,6 +1722,9 @@ void ilk_wm_get_hw_state(struct drm_device *dev);
>  void skl_wm_get_hw_state(struct drm_device *dev);
>  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */);
> +bool skl_can_enable_sagv(struct drm_atomic_state *state);
> +int skl_enable_sagv(struct drm_i915_private *dev_priv);
> +int skl_disable_sagv(struct drm_i915_private *dev_priv);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b4cf988..fed2bae8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2860,6 +2860,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev)
>  
>  #define SKL_DDB_SIZE		896	/* in blocks */
>  #define BXT_DDB_SIZE		512
> +#define SKL_SAGV_BLOCK_TIME 30 /* µs */
>  
>  /*
>   * Return the index of a plane in the SKL DDB and wm result arrays.  Primary
> @@ -2883,6 +2884,153 @@ skl_wm_plane_id(const struct intel_plane *plane)
>  	}
>  }
>  
> +/*
> + * SAGV dynamically adjusts the system agent voltage and clock frequencies
> + * depending on power and performance requirements. The display engine access
> + * to system memory is blocked during the adjustment time. Because of the
> + * blocking time, having this enabled can cause full system hangs and/or pipe
> + * underruns if we don't meet all of the following requirements:
> + *
> + *  - <= 1 pipe enabled
> + *  - All planes can enable watermarks for latencies >= SAGV engine block time
> + *  - We're not using an interlaced display configuration
> + */
> +int
> +skl_enable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +
> +	if (dev_priv->skl_sagv_status == I915_SKL_SAGV_NOT_CONTROLLED ||
> +	    dev_priv->skl_sagv_status == I915_SKL_SAGV_ENABLED)
> +		return 0;
> +
> +	DRM_DEBUG_KMS("Enabling the SAGV\n");
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +
> +	ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				      GEN9_SAGV_ENABLE);
> +
> +	/* We don't need to wait for the SAGV when enabling */
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	/*
> +	 * Some skl systems, pre-release machines in particular,
> +	 * don't actually have an SAGV.
> +	 */
> +	if (ret == -ENOSYS) {
> +		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
> +		return 0;
> +	} else if (ret < 0) {
> +		DRM_ERROR("Failed to enable the SAGV\n");
> +		return ret;
> +	}
> +
> +	dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED;
> +	return 0;
> +}
> +
> +static int
> +skl_do_sagv_disable(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +	uint32_t temp = GEN9_SAGV_DISABLE;
> +
> +	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				     &temp);
> +	if (ret)
> +		return ret;
> +	else
> +		return temp & GEN9_SAGV_IS_DISABLED;
> +}
> +
> +int
> +skl_disable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret, result;
> +
> +	if (dev_priv->skl_sagv_status == I915_SKL_SAGV_NOT_CONTROLLED ||
> +	    dev_priv->skl_sagv_status == I915_SKL_SAGV_DISABLED)
> +		return 0;
> +
> +	DRM_DEBUG_KMS("Disabling the SAGV\n");
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +
> +	/* bspec says to keep retrying for at least 1 ms */
> +	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (ret == -ETIMEDOUT) {
> +		DRM_ERROR("Request to disable SAGV timed out\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	/*
> +	 * Some skl systems, pre-release machines in particular,
> +	 * don't actually have an SAGV.
> +	 */
> +	if (result == -ENOSYS) {
> +		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
> +		return 0;
> +	} else if (result < 0) {
> +		DRM_ERROR("Failed to disable the SAGV\n");
> +		return result;
> +	}
> +
> +	dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED;
> +	return 0;
> +}
> +
> +bool skl_can_enable_sagv(struct drm_atomic_state *state)
> +{
> +	struct drm_device *dev = state->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct drm_crtc *crtc;
> +	enum pipe pipe;
> +	int level, plane;
> +
> +	/*
> +	 * SKL workaround: bspec recommends we disable the SAGV when we have
> +	 * more then one pipe enabled
> +	 *
> +	 * If there are no active CRTCs, no additional checks need be performed
> +	 */
> +	if (hweight32(intel_state->active_crtcs) == 0)
> +		return true;
> +	else if (hweight32(intel_state->active_crtcs) > 1)
> +		return false;
> +
> +	/* Since we're now guaranteed to only have one active CRTC... */
> +	pipe = ffs(intel_state->active_crtcs) - 1;
> +	crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> +
> +	if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE)
> +		return false;
> +
> +	for_each_plane(dev_priv, pipe, plane) {
> +		/* Skip this plane if it's not enabled */
> +		if (intel_state->wm_results.plane[pipe][plane][0] == 0)
> +			continue;
> +
> +		/* Find the highest enabled wm level for this plane */
> +		for (level = ilk_wm_max_level(dev);
> +		     intel_state->wm_results.plane[pipe][plane][level] == 0;
> +		     --level);
> +
> +		/*
> +		 * If any of the planes on this pipe don't enable wm levels
> +		 * that incur memory latencies higher then 30µs we can't enable
> +		 * the SAGV
> +		 */
> +		if (dev_priv->wm.skl_latency[level] < SKL_SAGV_BLOCK_TIME)
> +			return false;
Shouldn't this check be >= BLOCK_TIME?

~Maarten

[toc] | [prev] | [next] | [standalone]

#1467420 — Re: [PATCH v12 2/7] drm/i915/skl: Add support for the SAGV, fix underrun hangs

FromMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date2016-08-22 09:30 +0200
SubjectRe: [PATCH v12 2/7] drm/i915/skl: Add support for the SAGV, fix underrun hangs
Message-ID<s8RQd-3fx-19@gated-at.bofh.it>
In reply to#1464981
Op 18-08-16 om 16:05 schreef Lyude Paul:
> On Thu, 2016-08-18 at 09:39 +0200, Maarten Lankhorst wrote:
>> Hey,
>>
>> Op 17-08-16 om 21:55 schreef Lyude:
>>> Since the watermark calculations for Skylake are still broken, we're apt
>>> to hitting underruns very easily under multi-monitor configurations.
>>> While it would be lovely if this was fixed, it's not. Another problem
>>> that's been coming from this however, is the mysterious issue of
>>> underruns causing full system hangs. An easy way to reproduce this with
>>> a skylake system:
>>>
>>> - Get a laptop with a skylake GPU, and hook up two external monitors to
>>>   it
>>> - Move the cursor from the built-in LCD to one of the external displays
>>>   as quickly as you can
>>> - You'll get a few pipe underruns, and eventually the entire system will
>>>   just freeze.
>>>
>>> After doing a lot of investigation and reading through the bspec, I
>>> found the existence of the SAGV, which is responsible for adjusting the
>>> system agent voltage and clock frequencies depending on how much power
>>> we need. According to the bspec:
>>>
>>> "The display engine access to system memory is blocked during the
>>>  adjustment time. SAGV defaults to enabled. Software must use the
>>>  GT-driver pcode mailbox to disable SAGV when the display engine is not
>>>  able to tolerate the blocking time."
>>>
>>> The rest of the bspec goes on to explain that software can simply leave
>>> the SAGV enabled, and disable it when we use interlaced pipes/have more
>>> then one pipe active.
>>>
>>> Sure enough, with this patchset the system hangs resulting from pipe
>>> underruns on Skylake have completely vanished on my T460s. Additionally,
>>> the bspec mentions turning off the SAGV	with more then one pipe
>>> enabled
>>> as a workaround for display underruns. While this patch doesn't entirely
>>> fix that, it looks like it does improve the situation a little bit so
>>> it's likely this is going to be required to make watermarks on Skylake
>>> fully functional.
>>>
>>> This will still need additional work in the future: we shouldn't be
>>> enabling the SAGV if any of the currently enabled planes can't enable WM
>>> levels that introduce latencies >= 30 µs.
>>>
>>> Changes since v11:
>>>  - Add skl_can_enable_sagv()
>>>  - Make sure we don't enable SAGV when not all planes can enable
>>>    watermarks >= the SAGV engine block time. I was originally going to
>>>    save this for later, but I recently managed to run into a machine
>>>    that was having problems with a single pipe configuration + SAGV.
>>>  - Make comparisons to I915_SKL_SAGV_NOT_CONTROLLED explicit
>>>  - Change I915_SAGV_DYNAMIC_FREQ to I915_SAGV_ENABLE
>>>  - Move printks outside of mutexes
>>>  - Don't print error messages twice
>>> Changes since v10:
>>>  - Apparently sandybridge_pcode_read actually writes values and reads
>>>    them back, despite it's misleading function name. This means we've
>>>    been doing this mostly wrong and have been writing garbage to the
>>>    SAGV control. Because of this, we no longer attempt to read the SAGV
>>>    status during initialization (since there are no helpers for this).
>>>  - mlankhorst noticed that this patch was breaking on some very early
>>>    pre-release Skylake machines, which apparently don't allow you to
>>>    disable the SAGV. To prevent machines from failing tests due to SAGV
>>>    errors, if the first time we try to control the SAGV results in the
>>>    mailbox indicating an invalid command, we just disable future attempts
>>>    to control the SAGV state by setting dev_priv->skl_sagv_status to
>>>    I915_SKL_SAGV_NOT_CONTROLLED and make a note of it in dmesg.
>>>  - Move mutex_unlock() a little higher in skl_enable_sagv(). This
>>>    doesn't actually fix anything, but lets us release the lock a little
>>>    sooner since we're finished with it.
>>> Changes since v9:
>>>  - Only enable/disable sagv on Skylake
>>> Changes since v8:
>>>  - Add intel_state->modeset guard to the conditional for
>>>    skl_enable_sagv()
>>> Changes since v7:
>>>  - Remove GEN9_SAGV_LOW_FREQ, replace with GEN9_SAGV_IS_ENABLED (that's
>>>    all we use it for anyway)
>>>  - Use GEN9_SAGV_IS_ENABLED instead of 0x1 for clarification
>>>  - Fix a styling error that snuck past me
>>> Changes since v6:
>>>  - Protect skl_enable_sagv() with intel_state->modeset conditional in
>>>    intel_atomic_commit_tail()
>>> Changes since v5:
>>>  - Don't use is_power_of_2. Makes things confusing
>>>  - Don't use the old state to figure out whether or not to
>>>    enable/disable the sagv, use the new one
>>>  - Split the loop in skl_disable_sagv into it's own function
>>>  - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail()
>>> Changes since v4:
>>>  - Use is_power_of_2 against active_crtcs to check whether we have > 1
>>>    pipe enabled
>>>  - Fix skl_sagv_get_hw_state(): (temp & 0x1) indicates disabled, 0x0
>>>    enabled
>>>  - Call skl_sagv_enable/disable() from pre/post-plane updates
>>> Changes since v3:
>>>  - Use time_before() to compare timeout to jiffies
>>> Changes since v2:
>>>  - Really apply minor style nitpicks to patch this time
>>> Changes since v1:
>>>  - Added comments about this probably being one of the requirements to
>>>    fixing Skylake's watermark issues
>>>  - Minor style nitpicks from Matt Roper
>>>  - Disable these functions on Broxton, since it doesn't have an SAGV
>>>
>>> Signed-off-by: Lyude <cpaul@redhat.com>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.h      |   7 ++
>>>  drivers/gpu/drm/i915/i915_reg.h      |   4 +
>>>  drivers/gpu/drm/i915/intel_display.c |  11 +++
>>>  drivers/gpu/drm/i915/intel_drv.h     |   3 +
>>>  drivers/gpu/drm/i915/intel_pm.c      | 148
>>> +++++++++++++++++++++++++++++++++++
>>>  5 files changed, 173 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 35caa9b..f20530bb 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1949,6 +1949,13 @@ struct drm_i915_private {
>>>  	struct i915_suspend_saved_registers regfile;
>>>  	struct vlv_s0ix_state vlv_s0ix_state;
>>>  
>>> +	enum {
>>> +		I915_SKL_SAGV_UNKNOWN = 0,
>>> +		I915_SKL_SAGV_DISABLED,
>>> +		I915_SKL_SAGV_ENABLED,
>>> +		I915_SKL_SAGV_NOT_CONTROLLED
>>> +	} skl_sagv_status;
>>> +
>>>  	struct {
>>>  		/*
>>>  		 * Raw watermark latency values:
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>> b/drivers/gpu/drm/i915/i915_reg.h
>>> index 7419fbf..be82c49 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -7153,6 +7153,10 @@ enum {
>>>  #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
>>>  #define   DISPLAY_IPS_CONTROL			0x19
>>>  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
>>> +#define   GEN9_PCODE_SAGV_CONTROL		0x21
>>> +#define     GEN9_SAGV_DISABLE			0x0
>>> +#define     GEN9_SAGV_IS_DISABLED		0x1
>>> +#define     GEN9_SAGV_ENABLE 	             0x3
>>>  #define GEN6_PCODE_DATA				_MMIO(0x138128)
>>>  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
>>>  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>> b/drivers/gpu/drm/i915/intel_display.c
>>> index 781d22e..ca4b83f 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -14156,6 +14156,13 @@ static void intel_atomic_commit_tail(struct
>>> drm_atomic_state *state)
>>>  		     intel_state->cdclk_pll_vco != dev_priv-
>>>> cdclk_pll.vco))
>>>  			dev_priv->display.modeset_commit_cdclk(state);
>>>  
>>> +		/*
>>> +		 * SKL workaround: bspec recommends we disable the SAGV
>>> when we
>>> +		 * have more then one pipe enabled
>>> +		 */
>>> +		if (IS_SKYLAKE(dev_priv) && !skl_can_enable_sagv(state))
>>> +			skl_disable_sagv(dev_priv);
>>> +
>>>  		intel_modeset_verify_disabled(dev);
>>>  	}
>>>  
>>> @@ -14229,6 +14236,10 @@ static void intel_atomic_commit_tail(struct
>>> drm_atomic_state *state)
>>>  		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc-
>>>> state);
>>>  	}
>>>  
>>> +	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
>>> +	    skl_can_enable_sagv(state))
>>> +		skl_enable_sagv(dev_priv);
>>> +
>>>  	drm_atomic_helper_commit_hw_done(state);
>>>  
>>>  	if (intel_state->modeset)
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>> b/drivers/gpu/drm/i915/intel_drv.h
>>> index 1c700b0..d203c77 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1722,6 +1722,9 @@ void ilk_wm_get_hw_state(struct drm_device *dev);
>>>  void skl_wm_get_hw_state(struct drm_device *dev);
>>>  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>>>  			  struct skl_ddb_allocation *ddb /* out */);
>>> +bool skl_can_enable_sagv(struct drm_atomic_state *state);
>>> +int skl_enable_sagv(struct drm_i915_private *dev_priv);
>>> +int skl_disable_sagv(struct drm_i915_private *dev_priv);
>>>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>>>  bool ilk_disable_lp_wm(struct drm_device *dev);
>>>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>>> b/drivers/gpu/drm/i915/intel_pm.c
>>> index b4cf988..fed2bae8 100644
>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>> @@ -2860,6 +2860,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev)
>>>  
>>>  #define SKL_DDB_SIZE		896	/* in blocks */
>>>  #define BXT_DDB_SIZE		512
>>> +#define SKL_SAGV_BLOCK_TIME 30 /* µs */
>>>  
>>>  /*
>>>   * Return the index of a plane in the SKL DDB and wm result
>>> arrays.  Primary
>>> @@ -2883,6 +2884,153 @@ skl_wm_plane_id(const struct intel_plane *plane)
>>>  	}
>>>  }
>>>  
>>> +/*
>>> + * SAGV dynamically adjusts the system agent voltage and clock frequencies
>>> + * depending on power and performance requirements. The display engine
>>> access
>>> + * to system memory is blocked during the adjustment time. Because of the
>>> + * blocking time, having this enabled can cause full system hangs and/or
>>> pipe
>>> + * underruns if we don't meet all of the following requirements:
>>> + *
>>> + *  - <= 1 pipe enabled
>>> + *  - All planes can enable watermarks for latencies >= SAGV engine block
>>> time
>>> + *  - We're not using an interlaced display configuration
>>> + */
>>> +int
>>> +skl_enable_sagv(struct drm_i915_private *dev_priv)
>>> +{
>>> +	int ret;
>>> +
>>> +	if (dev_priv->skl_sagv_status == I915_SKL_SAGV_NOT_CONTROLLED ||
>>> +	    dev_priv->skl_sagv_status == I915_SKL_SAGV_ENABLED)
>>> +		return 0;
>>> +
>>> +	DRM_DEBUG_KMS("Enabling the SAGV\n");
>>> +	mutex_lock(&dev_priv->rps.hw_lock);
>>> +
>>> +	ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
>>> +				      GEN9_SAGV_ENABLE);
>>> +
>>> +	/* We don't need to wait for the SAGV when enabling */
>>> +	mutex_unlock(&dev_priv->rps.hw_lock);
>>> +
>>> +	/*
>>> +	 * Some skl systems, pre-release machines in particular,
>>> +	 * don't actually have an SAGV.
>>> +	 */
>>> +	if (ret == -ENOSYS) {
>>> +		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
>>> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
>>> +		return 0;
>>> +	} else if (ret < 0) {
>>> +		DRM_ERROR("Failed to enable the SAGV\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED;
>>> +	return 0;
>>> +}
>>> +
>>> +static int
>>> +skl_do_sagv_disable(struct drm_i915_private *dev_priv)
>>> +{
>>> +	int ret;
>>> +	uint32_t temp = GEN9_SAGV_DISABLE;
>>> +
>>> +	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
>>> +				     &temp);
>>> +	if (ret)
>>> +		return ret;
>>> +	else
>>> +		return temp & GEN9_SAGV_IS_DISABLED;
>>> +}
>>> +
>>> +int
>>> +skl_disable_sagv(struct drm_i915_private *dev_priv)
>>> +{
>>> +	int ret, result;
>>> +
>>> +	if (dev_priv->skl_sagv_status == I915_SKL_SAGV_NOT_CONTROLLED ||
>>> +	    dev_priv->skl_sagv_status == I915_SKL_SAGV_DISABLED)
>>> +		return 0;
>>> +
>>> +	DRM_DEBUG_KMS("Disabling the SAGV\n");
>>> +	mutex_lock(&dev_priv->rps.hw_lock);
>>> +
>>> +	/* bspec says to keep retrying for at least 1 ms */
>>> +	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
>>> +	mutex_unlock(&dev_priv->rps.hw_lock);
>>> +
>>> +	if (ret == -ETIMEDOUT) {
>>> +		DRM_ERROR("Request to disable SAGV timed out\n");
>>> +		return -ETIMEDOUT;
>>> +	}
>>> +
>>> +	/*
>>> +	 * Some skl systems, pre-release machines in particular,
>>> +	 * don't actually have an SAGV.
>>> +	 */
>>> +	if (result == -ENOSYS) {
>>> +		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
>>> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
>>> +		return 0;
>>> +	} else if (result < 0) {
>>> +		DRM_ERROR("Failed to disable the SAGV\n");
>>> +		return result;
>>> +	}
>>> +
>>> +	dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED;
>>> +	return 0;
>>> +}
>>> +
>>> +bool skl_can_enable_sagv(struct drm_atomic_state *state)
>>> +{
>>> +	struct drm_device *dev = state->dev;
>>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>>> +	struct intel_atomic_state *intel_state =
>>> to_intel_atomic_state(state);
>>> +	struct drm_crtc *crtc;
>>> +	enum pipe pipe;
>>> +	int level, plane;
>>> +
>>> +	/*
>>> +	 * SKL workaround: bspec recommends we disable the SAGV when we
>>> have
>>> +	 * more then one pipe enabled
>>> +	 *
>>> +	 * If there are no active CRTCs, no additional checks need be
>>> performed
>>> +	 */
>>> +	if (hweight32(intel_state->active_crtcs) == 0)
>>> +		return true;
>>> +	else if (hweight32(intel_state->active_crtcs) > 1)
>>> +		return false;
>>> +
>>> +	/* Since we're now guaranteed to only have one active CRTC... */
>>> +	pipe = ffs(intel_state->active_crtcs) - 1;
>>> +	crtc = dev_priv->pipe_to_crtc_mapping[pipe];
>>> +
>>> +	if (crtc->state->mode.flags & DRM_MODE_FLAG_INTERLACE)
>>> +		return false;
>>> +
>>> +	for_each_plane(dev_priv, pipe, plane) {
>>> +		/* Skip this plane if it's not enabled */
>>> +		if (intel_state->wm_results.plane[pipe][plane][0] == 0)
>>> +			continue;
>>> +
>>> +		/* Find the highest enabled wm level for this plane */
>>> +		for (level = ilk_wm_max_level(dev);
>>> +		     intel_state->wm_results.plane[pipe][plane][level] ==
>>> 0;
>>> +		     --level);
>>> +
>>> +		/*
>>> +		 * If any of the planes on this pipe don't enable wm levels
>>> +		 * that incur memory latencies higher then 30µs we can't
>>> enable
>>> +		 * the SAGV
>>> +		 */
>>> +		if (dev_priv->wm.skl_latency[level] < SKL_SAGV_BLOCK_TIME)
>>> +			return false;
>> Shouldn't this check be >= BLOCK_TIME?
>>
> That's the requirement for the sagv but the conditional here is still correct.
>
> WM0 - 2ms
> WM1 - 5ms
> WM2 - 10ms
> WM3 - 20ms
> WM4+- disabled
>
> (20ms < BLOCK_TIME) == true, which indicates we don't have any watermark levels
> with latency values >= 30ms. We can't enable so return false.
>
> WM0 - 2ms
> WM1 - 5ms
> WM2 - 10ms
> WM3 - 20ms
> WM4 - 33ms
> WM5 - 50ms
> WM6 - 70ms
> WM7 - 99ms
>
> (99ms < BLOCK_TIME) == false, and 99 >= BLOCK_TIME so we end up returning true
> to indicate it's safe to enable.
Ah right, thanks for explaining.

~Maarten

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web