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

Groups > linux.kernel > #1446596 > unrolled thread

[PATCH 0/2] drm/i915/skl: Fix (most) pipe underruns, properly this time

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] drm/i915/skl: Fix (most) pipe underruns, properly this time Lyude <cpaul@redhat.com> - 2016-07-19 18:40 +0200
    [PATCH 1/2] drm/i915/skl: Update plane watermarks atomically during plane updates Lyude <cpaul@redhat.com> - 2016-07-19 18:40 +0200

#1446596 — [PATCH 0/2] drm/i915/skl: Fix (most) pipe underruns, properly this time

Unfortunately as a few of you are aware, Skylake is still very prone to pipe
underruns. Most of this comes from not doing things atomically enough (e.g.
needing to ensure that we update watermarks with other plane attributes, not
forcefully flushing pipes until we need to, etc.). Now that I've finally got a
grasp on how double buffered registers, arming registers, etc. works on skl,
I've written up patches that fix most of the pipe underruns on Skylake.

When I say "most", I'm referring to the fact that I still seem to be able to
reproduce pipe underruns, but this seems to be strictly exclusive to when pipes
are being disabled. This means things *still* need to be made more atomic
(sigh), but at least this is a start.

Testing this series with a chamelium[1], mainly so I could heavily stress test
the hotplugging of displays, I'm no longer able to reproduce pipe underruns
when enabling another pipe. I've also tried reproducing underruns using xdotool
to move the cursor in X from one monitor to another as quickly as possible
(e.g. > 60 fps), and am no longer able to reproduce underruns there either with
this patch series.

Signed-off-by: Lyude Paul <cpaul@redhat.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> <cpaul@redhat.com>
Cc: Matt Roper <matthew.d.roper@intel.com>

[1]: https://www.chromium.org/chromium-os/testing/chamelium

Lyude (2):
  drm/i915/skl: Update plane watermarks atomically during plane updates
  drm/i915/skl: Don't mark pipes as dirty unless we've added/removed
    pipes

 drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c      | 31 +++++++++---------------
 2 files changed, 59 insertions(+), 19 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1446598 — [PATCH 1/2] drm/i915/skl: Update plane watermarks atomically during plane updates

Thanks to Ville for suggesting this as a potential solution to pipe
underruns on Skylake.

On Skylake all of the registers for configuring planes, including the
registers for configuring their watermarks, are double buffered. New
values written to them won't take effect until said registers are
"armed", which is done by writing to the PLANE_SURF (or in the case of
cursor planes, the CURBASE register) register.

With this in mind, up until now we've been updating watermarks on skl
like this:

 - Calculate new watermark values in skl_compute_wm()

 - Update non-watermark settings for each plane in
   skylake_update_primary_plane()/i9xx_update_cursor()

 - Arm plane registers to update at the start of the next vblank in
   skylake_update_primary_plane()/i9xx_update_cursor()

 - *Possibly underrun here, since the plane may now have updated it's
   settings using the old and insufficient watermark values*

 - Update watermark settings in skl_write_wm_values()

 - *Possibly underrun here as well if we've passed a vblank,
   causing the hardware to get stuck running on intermediate wm values*

 - Finally arm plane registers so they update to the correct values at
   the start of the next vblank in skl_wm_flush_pipe()

Now we update watermarks atomically like this:

 - Calculate new watermark values in skl_compute_wm()

 - Update watermark values for each plane in
   skylake_write_plane_wm()/skylake_write_cursor_wm()

 - Update all the other values for the plane (position, address, etc.)
   in skylake_update_primary_plane()/i9xx_update_cursor()

 - Arm plane registers (including the watermark settings) to update at
   the start of the next vblank in
   skylake_update_primary_plane()/i9xx_update_cursor()

Which is more of a step in the right direction to fixing all of the
underrun issues we're currently seeing with skl

Signed-off-by: Lyude Paul <cpaul@redhat.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> <cpaul@redhat.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_pm.c      | 15 +-----------
 2 files changed, 48 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fb7d8fc5..3d2c125 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2971,6 +2971,27 @@ u32 skl_plane_ctl_rotation(unsigned int rotation)
 	return 0;
 }
 
+static void skylake_write_plane_wm(struct intel_crtc *intel_crtc,
+				   int plane)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	int level, max_level = ilk_wm_max_level(dev);
+	enum pipe pipe = intel_crtc->pipe;
+
+	mutex_lock(&dev_priv->wm.wm_mutex);
+
+	for (level = 0; level < max_level; level++) {
+		I915_WRITE(PLANE_WM(pipe, plane, level),
+			   wm->plane[pipe][plane][level]);
+	}
+	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
+
+	mutex_unlock(&dev_priv->wm.wm_mutex);
+}
+
 static void skylake_update_primary_plane(struct drm_plane *plane,
 					 const struct intel_crtc_state *crtc_state,
 					 const struct intel_plane_state *plane_state)
@@ -3031,6 +3052,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = x_offset;
 	intel_crtc->adjusted_y = y_offset;
 
+	skylake_write_plane_wm(intel_crtc, 0);
+
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
 	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
@@ -10233,6 +10256,27 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base,
 	}
 }
 
+static void skl_write_cursor_wm(struct intel_crtc *intel_crtc)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	int level, max_level = ilk_wm_max_level(dev);
+	enum pipe pipe = intel_crtc->pipe;
+
+	mutex_lock(&dev_priv->wm.wm_mutex);
+
+	for (level = 0; level <= max_level; level++) {
+		I915_WRITE(CUR_WM(pipe, level),
+			   wm->plane[pipe][PLANE_CURSOR][level]);
+	}
+	I915_WRITE(CUR_WM_TRANS(pipe),
+		   wm->plane_trans[pipe][PLANE_CURSOR]);
+
+	mutex_unlock(&dev_priv->wm.wm_mutex);
+}
+
 static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 			       const struct intel_plane_state *plane_state)
 {
@@ -10242,6 +10286,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
+	if (IS_SKYLAKE(dev_priv))
+		skl_write_cursor_wm(intel_crtc);
+
 	if (plane_state && plane_state->visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (plane_state->base.crtc_w) {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 213ad35..3a7709c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3788,7 +3788,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 		skl_disable_sagv(dev_priv);
 
 	for_each_intel_crtc(dev, crtc) {
-		int i, level, max_level = ilk_wm_max_level(dev);
+		int i;
 		enum pipe pipe = crtc->pipe;
 
 		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
@@ -3798,19 +3798,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 
 		I915_WRITE(PIPE_WM_LINETIME(pipe), new->wm_linetime[pipe]);
 
-		for (level = 0; level <= max_level; level++) {
-			for (i = 0; i < intel_num_planes(crtc); i++)
-				I915_WRITE(PLANE_WM(pipe, i, level),
-					   new->plane[pipe][i][level]);
-			I915_WRITE(CUR_WM(pipe, level),
-				   new->plane[pipe][PLANE_CURSOR][level]);
-		}
-		for (i = 0; i < intel_num_planes(crtc); i++)
-			I915_WRITE(PLANE_WM_TRANS(pipe, i),
-				   new->plane_trans[pipe][i]);
-		I915_WRITE(CUR_WM_TRANS(pipe),
-			   new->plane_trans[pipe][PLANE_CURSOR]);
-
 		for (i = 0; i < intel_num_planes(crtc); i++) {
 			skl_ddb_entry_write(dev_priv,
 					    PLANE_BUF_CFG(pipe, i),
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web