[PATCH 0/6] drm/i915/skl: Finally fix watermarks

[PATCH 0/6] drm/i915/skl: Finally fix watermarks

[Lyude]

To Sebastian Reichel:
	If this e-mail has the bizarre email address formatting issue you
	noticed in the last patch series I sent please let me know. I haven't
	gotten a chance to properly look at the e-mail you forwarded to me to
	see what's causing the problem, but I double checked the Cc: line for
	this e-mail manually before sending it out so hopefully I should be
	good for now…

Anyway, onto the actual patch series:

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 all of the pipe underruns on Skylake I was
able to reproduce.

Of course, one of the prerequisites for this patch series to actually fix all
of the pipe underruns is the patch I previously submitted that added support
for Skylake's SAGV.

Originally this patch series left behind the issue of running into pipe
underruns when we disabled pipes, however I've now managed to fix that behavior
as well. As such I've renamed the patch series appropriately instead of just
incrementing the version.

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

Lyude (5):
  drm/i915/skl: Update plane watermarks atomically during plane updates
  drm/i915/skl: Actually reuse wm values when pipes don't change
  drm/i915/skl: Always wait for pipes to update after a flush
  drm/i915/skl: Only flush pipes when we change the ddb allocation
  drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()

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

 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c |   5 ++
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      | 133 +++++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_sprite.c  |   2 +
 5 files changed, 120 insertions(+), 23 deletions(-)

-- 
2.7.4

[PATCH 0/6] drm/i915/skl: Finally fix watermarks

[Lyude]

To Sebastian Reichel:
	If this e-mail has the bizarre email address formatting issue you
	noticed in the last patch series I sent please let me know. I haven't
	gotten a chance to properly look at the e-mail you forwarded to me to
	see what's causing the problem, but I double checked the Cc: line for
	this e-mail manually before sending it out so hopefully I should be
	good for now…

Anyway, onto the actual patch series:

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 all of the pipe underruns on Skylake I was
able to reproduce.

Of course, one of the prerequisites for this patch series to actually fix all
of the pipe underruns is the patch I previously submitted that added support
for Skylake's SAGV.

Originally this patch series left behind the issue of running into pipe
underruns when we disabled pipes, however I've now managed to fix that behavior
as well. As such I've renamed the patch series appropriately instead of just
incrementing the version.

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

Lyude (5):
  drm/i915/skl: Update plane watermarks atomically during plane updates
  drm/i915/skl: Actually reuse wm values when pipes don't change
  drm/i915/skl: Always wait for pipes to update after a flush
  drm/i915/skl: Only flush pipes when we change the ddb allocation
  drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()

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

 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/intel_display.c |   5 ++
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      | 133 +++++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_sprite.c  |   2 +
 5 files changed, 120 insertions(+), 23 deletions(-)

-- 
2.7.4

[PATCH 1/6] drm/i915/skl: Update plane watermarks atomically during plane updates

[Lyude]

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:

  non-modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - intel_pre_plane_update:
        - intel_update_watermarks()
     - {vblank happens; new watermarks + old plane values => underrun }
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
        - end vblank evasion
  }

  or

  modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - crtc_enable:
        - intel_update_watermarks()
     - {vblank happens; new watermarks + old plane values => underrun }
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
        - end vblank evasion
  }

Now we update watermarks atomically like this:

  non-modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - intel_pre_plane_update:
        - intel_update_watermarks() (wm values aren't written yet)
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
        - write new wm values
        - end vblank evasion
  }

  modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - crtc_enable:
        - intel_update_watermarks() (actual wm values aren't written
          yet)
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
	- write new wm values
        - end vblank evasion
  }

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

Changes since original patch series:
- Remove mutex_lock/mutex_unlock since they don't do anything and we're
  not touching global state
- Move skl_write_cursor_wm/skl_write_plane_wm functions into intel_pm.c, make
  externally visible
- Add skl_write_plane_wm calls to skl_update_plane
- Fix conditional for for loop in skl_write_plane_wm (level < max_level
  should be level <= max_level)
- Make diagram in commit more accurate to what's actually happening
- Add Fixes:

Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
Signed-off-by: Lyude <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 |  5 ++++
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_pm.c      | 48 +++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_sprite.c  |  2 ++
 4 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 78beb7e..c0d2074 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3031,6 +3031,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = x_offset;
 	intel_crtc->adjusted_y = y_offset;
 
+	skl_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);
@@ -10242,6 +10244,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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e74d851..f1f54d9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1709,6 +1709,8 @@ 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 */);
+void skl_write_cursor_wm(struct intel_crtc *intel_crtc);
+void skl_write_plane_wm(struct intel_crtc *intel_crtc, int plane);
 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 64d628c..fa86bea 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3680,6 +3680,39 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
 		I915_WRITE(reg, 0);
 }
 
+void skl_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;
+
+	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]);
+}
+
+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;
+
+	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]);
+}
+
 static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 				const struct skl_wm_values *new)
 {
@@ -3687,7 +3720,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 	struct intel_crtc *crtc;
 
 	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)
@@ -3697,19 +3730,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),
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0de935a..50026f1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -238,6 +238,8 @@ skl_update_plane(struct drm_plane *drm_plane,
 	crtc_w--;
 	crtc_h--;
 
+	skl_write_plane_wm(to_intel_crtc(crtc_state->base.crtc), plane);
+
 	if (key->flags) {
 		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
 		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
-- 
2.7.4

[PATCH 1/6] drm/i915/skl: Update plane watermarks atomically during plane updates

[Lyude]

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:

  non-modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - intel_pre_plane_update:
        - intel_update_watermarks()
     - {vblank happens; new watermarks + old plane values => underrun }
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
        - end vblank evasion
  }

  or

  modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - crtc_enable:
        - intel_update_watermarks()
     - {vblank happens; new watermarks + old plane values => underrun }
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
        - end vblank evasion
  }

Now we update watermarks atomically like this:

  non-modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - intel_pre_plane_update:
        - intel_update_watermarks() (wm values aren't written yet)
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
        - write new wm values
        - end vblank evasion
  }

  modeset {
   - calculate (during atomic check phase)
   - finish_atomic_commit:
     - crtc_enable:
        - intel_update_watermarks() (actual wm values aren't written
          yet)
     - drm_atomic_helper_commit_planes_on_crtc:
        - start vblank evasion
        - write new plane registers
	- write new wm values
        - end vblank evasion
  }

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

Changes since original patch series:
- Remove mutex_lock/mutex_unlock since they don't do anything and we're
  not touching global state
- Move skl_write_cursor_wm/skl_write_plane_wm functions into intel_pm.c, make
  externally visible
- Add skl_write_plane_wm calls to skl_update_plane
- Fix conditional for for loop in skl_write_plane_wm (level < max_level
  should be level <= max_level)
- Make diagram in commit more accurate to what's actually happening
- Add Fixes:

Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
Signed-off-by: Lyude <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 |  5 ++++
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_pm.c      | 48 +++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_sprite.c  |  2 ++
 4 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 78beb7e..c0d2074 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3031,6 +3031,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = x_offset;
 	intel_crtc->adjusted_y = y_offset;
 
+	skl_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);
@@ -10242,6 +10244,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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e74d851..f1f54d9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1709,6 +1709,8 @@ 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 */);
+void skl_write_cursor_wm(struct intel_crtc *intel_crtc);
+void skl_write_plane_wm(struct intel_crtc *intel_crtc, int plane);
 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 64d628c..fa86bea 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3680,6 +3680,39 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
 		I915_WRITE(reg, 0);
 }
 
+void skl_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;
+
+	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]);
+}
+
+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;
+
+	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]);
+}
+
 static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 				const struct skl_wm_values *new)
 {
@@ -3687,7 +3720,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 	struct intel_crtc *crtc;
 
 	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)
@@ -3697,19 +3730,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),
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0de935a..50026f1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -238,6 +238,8 @@ skl_update_plane(struct drm_plane *drm_plane,
 	crtc_w--;
 	crtc_h--;
 
+	skl_write_plane_wm(to_intel_crtc(crtc_state->base.crtc), plane);
+
 	if (key->flags) {
 		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
 		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
-- 
2.7.4

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

[PATCH 2/6] drm/i915/gen9: Only copy WM results for changed pipes to skl_hw

[Lyude]

From: Matt Roper <matthew.d.roper@intel.com>

When we write watermark values to the hardware, those values are stored
in dev_priv->wm.skl_hw.  However with recent watermark changes, the
results structure we're copying from only contains valid watermark and
DDB values for the pipes that are actually changing; the values for
other pipes remain 0.  Thus a blind copy of the entire skl_wm_values
structure will clobber the values for unchanged pipes...we need to be
more selective and only copy over the values for the changing pipes.

This mistake was hidden until recently due to another bug that caused us
to erroneously re-calculate watermarks for all active pipes rather than
changing pipes.  Only when that bug was fixed was the impact of this bug
discovered (e.g., modesets failing with "Requested display configuration
exceeds system watermark limitations" messages and leaving watermarks
non-functional, even ones initiated by intel_fbdev_restore_mode).

Changes since v1:
 - Add a function for copying a pipe's wm values
   (skl_copy_wm_for_pipe()) so we can reuse this later

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 734fa01f3a17 ("drm/i915/gen9: Calculate watermarks during atomic 'check' (v2)")
Fixes: 9b6130227495 ("drm/i915/gen9: Re-allocate DDB only for changed pipes")
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fa86bea..b7d4af1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3966,6 +3966,24 @@ skl_compute_ddb(struct drm_atomic_state *state)
 	return 0;
 }
 
+static void
+skl_copy_wm_for_pipe(struct skl_wm_values *dst,
+		     struct skl_wm_values *src,
+		     enum pipe pipe)
+{
+	dst->wm_linetime[pipe] = src->wm_linetime[pipe];
+	memcpy(dst->plane[pipe], src->plane[pipe],
+	       sizeof(dst->plane[pipe]));
+	memcpy(dst->plane_trans[pipe], src->plane_trans[pipe],
+	       sizeof(dst->plane_trans[pipe]));
+
+	dst->ddb.pipe[pipe] = src->ddb.pipe[pipe];
+	memcpy(dst->ddb.y_plane[pipe], src->ddb.y_plane[pipe],
+	       sizeof(dst->ddb.y_plane[pipe]));
+	memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe],
+	       sizeof(dst->ddb.plane[pipe]));
+}
+
 static int
 skl_compute_wm(struct drm_atomic_state *state)
 {
@@ -4038,8 +4056,10 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct skl_wm_values *results = &dev_priv->wm.skl_results;
+	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;
 
 	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
 		return;
@@ -4051,8 +4071,12 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	skl_write_wm_values(dev_priv, results);
 	skl_flush_wm_values(dev_priv, results);
 
-	/* store the new configuration */
-	dev_priv->wm.skl_hw = *results;
+	/*
+	 * Store the new configuration (but only for the pipes that have
+	 * changed; the other values weren't recomputed).
+	 */
+	for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes)
+		skl_copy_wm_for_pipe(hw_vals, results, pipe);
 
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
-- 
2.7.4

[PATCH 2/6] drm/i915/gen9: Only copy WM results for changed pipes to skl_hw

[Lyude]

From: Matt Roper <matthew.d.roper@intel.com>

When we write watermark values to the hardware, those values are stored
in dev_priv->wm.skl_hw.  However with recent watermark changes, the
results structure we're copying from only contains valid watermark and
DDB values for the pipes that are actually changing; the values for
other pipes remain 0.  Thus a blind copy of the entire skl_wm_values
structure will clobber the values for unchanged pipes...we need to be
more selective and only copy over the values for the changing pipes.

This mistake was hidden until recently due to another bug that caused us
to erroneously re-calculate watermarks for all active pipes rather than
changing pipes.  Only when that bug was fixed was the impact of this bug
discovered (e.g., modesets failing with "Requested display configuration
exceeds system watermark limitations" messages and leaving watermarks
non-functional, even ones initiated by intel_fbdev_restore_mode).

Changes since v1:
 - Add a function for copying a pipe's wm values
   (skl_copy_wm_for_pipe()) so we can reuse this later

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Fixes: 734fa01f3a17 ("drm/i915/gen9: Calculate watermarks during atomic 'check' (v2)")
Fixes: 9b6130227495 ("drm/i915/gen9: Re-allocate DDB only for changed pipes")
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fa86bea..b7d4af1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3966,6 +3966,24 @@ skl_compute_ddb(struct drm_atomic_state *state)
 	return 0;
 }
 
+static void
+skl_copy_wm_for_pipe(struct skl_wm_values *dst,
+		     struct skl_wm_values *src,
+		     enum pipe pipe)
+{
+	dst->wm_linetime[pipe] = src->wm_linetime[pipe];
+	memcpy(dst->plane[pipe], src->plane[pipe],
+	       sizeof(dst->plane[pipe]));
+	memcpy(dst->plane_trans[pipe], src->plane_trans[pipe],
+	       sizeof(dst->plane_trans[pipe]));
+
+	dst->ddb.pipe[pipe] = src->ddb.pipe[pipe];
+	memcpy(dst->ddb.y_plane[pipe], src->ddb.y_plane[pipe],
+	       sizeof(dst->ddb.y_plane[pipe]));
+	memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe],
+	       sizeof(dst->ddb.plane[pipe]));
+}
+
 static int
 skl_compute_wm(struct drm_atomic_state *state)
 {
@@ -4038,8 +4056,10 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct skl_wm_values *results = &dev_priv->wm.skl_results;
+	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;
 
 	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
 		return;
@@ -4051,8 +4071,12 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	skl_write_wm_values(dev_priv, results);
 	skl_flush_wm_values(dev_priv, results);
 
-	/* store the new configuration */
-	dev_priv->wm.skl_hw = *results;
+	/*
+	 * Store the new configuration (but only for the pipes that have
+	 * changed; the other values weren't recomputed).
+	 */
+	for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes)
+		skl_copy_wm_for_pipe(hw_vals, results, pipe);
 
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
-- 
2.7.4

[PATCH 3/6] drm/i915/skl: Actually reuse wm values when pipes don't change

[Lyude]

Up until now we've actually been making the mistake of leaving the
watermark results for each pipe completely blank in skl_compute_wm()
when they haven't changed, fix this.

Fixes: 734fa01f3a17 ("drm/i915/gen9: Calculate watermarks during atomic 'check' (v2)")
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>
Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b7d4af1..788db86 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3987,10 +3987,13 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst,
 static int
 skl_compute_wm(struct drm_atomic_state *state)
 {
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *crtc;
 	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 skl_wm_values *hw_wm = &dev_priv->wm.skl_hw;
 	struct skl_pipe_wm *pipe_wm;
 	bool changed = false;
 	int ret, i;
@@ -4039,12 +4042,14 @@ skl_compute_wm(struct drm_atomic_state *state)
 		if (changed)
 			results->dirty_pipes |= drm_crtc_mask(crtc);
 
-		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
+		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) {
 			/* This pipe's WM's did not change */
+			skl_copy_wm_for_pipe(results, hw_wm, intel_crtc->pipe);
 			continue;
+		}
 
 		intel_cstate->update_wm_pre = true;
-		skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
+		skl_compute_wm_results(dev, pipe_wm, results, intel_crtc);
 	}
 
 	return 0;
-- 
2.7.4

[PATCH 3/6] drm/i915/skl: Actually reuse wm values when pipes don't change

[Lyude]

Up until now we've actually been making the mistake of leaving the
watermark results for each pipe completely blank in skl_compute_wm()
when they haven't changed, fix this.

Fixes: 734fa01f3a17 ("drm/i915/gen9: Calculate watermarks during atomic 'check' (v2)")
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>
Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b7d4af1..788db86 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3987,10 +3987,13 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst,
 static int
 skl_compute_wm(struct drm_atomic_state *state)
 {
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_crtc *crtc;
 	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 skl_wm_values *hw_wm = &dev_priv->wm.skl_hw;
 	struct skl_pipe_wm *pipe_wm;
 	bool changed = false;
 	int ret, i;
@@ -4039,12 +4042,14 @@ skl_compute_wm(struct drm_atomic_state *state)
 		if (changed)
 			results->dirty_pipes |= drm_crtc_mask(crtc);
 
-		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
+		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) {
 			/* This pipe's WM's did not change */
+			skl_copy_wm_for_pipe(results, hw_wm, intel_crtc->pipe);
 			continue;
+		}
 
 		intel_cstate->update_wm_pre = true;
-		skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
+		skl_compute_wm_results(dev, pipe_wm, results, intel_crtc);
 	}
 
 	return 0;
-- 
2.7.4

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

[PATCH 4/6] drm/i915/skl: Always wait for pipes to update after a flush

[Lyude]

As we've learned, all watermark updates on Skylake have to be strictly
atomic or things fail. While the bspec doesn't mandate that we need to
wait for pipes to finish after the third iteration of flushes, not doing
so gives us the opportunity to break this atomicity later. This example
assumes that we're lucky enough not to be interrupted by the scheduler
at any point during this:

 - Start with pipe A and pipe B enabled
 - Enable pipe C
 - Flush pipe A in pass 1, wait until update finishes
 - Flush pipe B in pass 3, continue without waiting for next vblank
 - Start another wm update
 - We enter the next vblank for pipe B before we finish writing all the
   vm values
 - *Underrun*

As such, we always need to wait for each pipe we flush to update so as
to never break this atomicity.

Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration")
Signed-off-by: Lyude <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_pm.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 788db86..2e31df4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3859,8 +3859,11 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 	/*
 	 * 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.
+	 * While the hardware doesn't require to wait for the next vblank here,
+	 * continuing before the pipe finishes updating could result in us
+	 * trying to update the wm values again before the pipe finishes
+	 * updating, which results in the hardware using intermediate wm values
+	 * and subsequently underrunning pipes.
 	 */
 	for_each_intel_crtc(dev, crtc) {
 		if (!crtc->active)
@@ -3876,6 +3879,16 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 			continue;
 
 		skl_wm_flush_pipe(dev_priv, pipe, 3);
+
+		/*
+		 * The only time we can get away with not waiting for an update
+		 * is when we just enabled the pipe, e.g. when it doesn't have
+		 * vblanks enabled anyway.
+		 */
+		if (drm_crtc_vblank_get(&crtc->base) == 0) {
+			intel_wait_for_vblank(dev, pipe);
+			drm_crtc_vblank_put(&crtc->base);
+		}
 	}
 }
 
-- 
2.7.4

[PATCH 4/6] drm/i915/skl: Always wait for pipes to update after a flush

[Lyude]

As we've learned, all watermark updates on Skylake have to be strictly
atomic or things fail. While the bspec doesn't mandate that we need to
wait for pipes to finish after the third iteration of flushes, not doing
so gives us the opportunity to break this atomicity later. This example
assumes that we're lucky enough not to be interrupted by the scheduler
at any point during this:

 - Start with pipe A and pipe B enabled
 - Enable pipe C
 - Flush pipe A in pass 1, wait until update finishes
 - Flush pipe B in pass 3, continue without waiting for next vblank
 - Start another wm update
 - We enter the next vblank for pipe B before we finish writing all the
   vm values
 - *Underrun*

As such, we always need to wait for each pipe we flush to update so as
to never break this atomicity.

Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration")
Signed-off-by: Lyude <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_pm.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 788db86..2e31df4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3859,8 +3859,11 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 	/*
 	 * 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.
+	 * While the hardware doesn't require to wait for the next vblank here,
+	 * continuing before the pipe finishes updating could result in us
+	 * trying to update the wm values again before the pipe finishes
+	 * updating, which results in the hardware using intermediate wm values
+	 * and subsequently underrunning pipes.
 	 */
 	for_each_intel_crtc(dev, crtc) {
 		if (!crtc->active)
@@ -3876,6 +3879,16 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 			continue;
 
 		skl_wm_flush_pipe(dev_priv, pipe, 3);
+
+		/*
+		 * The only time we can get away with not waiting for an update
+		 * is when we just enabled the pipe, e.g. when it doesn't have
+		 * vblanks enabled anyway.
+		 */
+		if (drm_crtc_vblank_get(&crtc->base) == 0) {
+			intel_wait_for_vblank(dev, pipe);
+			drm_crtc_vblank_put(&crtc->base);
+		}
 	}
 }
 
-- 
2.7.4

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

[PATCH 5/6] drm/i915/skl: Only flush pipes when we change the ddb allocation

[Lyude]

Manual pipe flushes are only necessary in order to make sure that we prevent
pipes with changed ddb allocations from overlapping from one another at
any point in time. Additionally, forcing us to wait for the next vblank
every time we have to update the watermark values because the cursor was
moving between screens will introduce a rather noticable lag for users.

Signed-off-by: Lyude <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/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 31 +++++++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c97724d..9e1e045 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1597,6 +1597,7 @@ struct skl_ddb_allocation {
 
 struct skl_wm_values {
 	unsigned dirty_pipes;
+	bool ddb_changed;
 	struct skl_ddb_allocation ddb;
 	uint32_t wm_linetime[I915_MAX_PIPES];
 	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2e31df4..4178bdd 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3809,6 +3809,12 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 	new_ddb = &new_values->ddb;
 	cur_ddb = &dev_priv->wm.skl_hw.ddb;
 
+	/* We only ever need to flush when the ddb allocations change */
+	if (!new_values->ddb_changed)
+		return;
+
+	new_values->ddb_changed = false;
+
 	/*
 	 * First pass: flush the pipes with the new allocation contained into
 	 * the old space.
@@ -3926,6 +3932,22 @@ pipes_modified(struct drm_atomic_state *state)
 	return ret;
 }
 
+static bool
+skl_pipe_ddb_changed(struct skl_ddb_allocation *old,
+		     struct skl_ddb_allocation *new,
+		     enum pipe pipe)
+{
+	if (memcmp(&old->pipe[pipe], &new->pipe[pipe],
+		   sizeof(old->pipe[pipe])) != 0 ||
+	    memcmp(&old->plane[pipe], &new->plane[pipe],
+		   sizeof(old->plane[pipe])) != 0 ||
+	    memcmp(&old->y_plane[pipe], &new->y_plane[pipe],
+		   sizeof(old->y_plane[pipe])) != 0)
+		return true;
+
+	return false;
+}
+
 static int
 skl_compute_ddb(struct drm_atomic_state *state)
 {
@@ -3933,7 +3955,8 @@ skl_compute_ddb(struct drm_atomic_state *state)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct intel_crtc *intel_crtc;
-	struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb;
+	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
+	struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb;
 	uint32_t realloc_pipes = pipes_modified(state);
 	int ret;
 
@@ -3971,9 +3994,13 @@ skl_compute_ddb(struct drm_atomic_state *state)
 		if (IS_ERR(cstate))
 			return PTR_ERR(cstate);
 
-		ret = skl_allocate_pipe_ddb(cstate, ddb);
+		ret = skl_allocate_pipe_ddb(cstate, new_ddb);
 		if (ret)
 			return ret;
+
+		if (!intel_state->wm_results.ddb_changed &&
+		    skl_pipe_ddb_changed(old_ddb, new_ddb, intel_crtc->pipe))
+			intel_state->wm_results.ddb_changed = true;
 	}
 
 	return 0;
-- 
2.7.4

[PATCH 5/6] drm/i915/skl: Only flush pipes when we change the ddb allocation

[Lyude]

Manual pipe flushes are only necessary in order to make sure that we prevent
pipes with changed ddb allocations from overlapping from one another at
any point in time. Additionally, forcing us to wait for the next vblank
every time we have to update the watermark values because the cursor was
moving between screens will introduce a rather noticable lag for users.

Signed-off-by: Lyude <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/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 31 +++++++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c97724d..9e1e045 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1597,6 +1597,7 @@ struct skl_ddb_allocation {
 
 struct skl_wm_values {
 	unsigned dirty_pipes;
+	bool ddb_changed;
 	struct skl_ddb_allocation ddb;
 	uint32_t wm_linetime[I915_MAX_PIPES];
 	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2e31df4..4178bdd 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3809,6 +3809,12 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 	new_ddb = &new_values->ddb;
 	cur_ddb = &dev_priv->wm.skl_hw.ddb;
 
+	/* We only ever need to flush when the ddb allocations change */
+	if (!new_values->ddb_changed)
+		return;
+
+	new_values->ddb_changed = false;
+
 	/*
 	 * First pass: flush the pipes with the new allocation contained into
 	 * the old space.
@@ -3926,6 +3932,22 @@ pipes_modified(struct drm_atomic_state *state)
 	return ret;
 }
 
+static bool
+skl_pipe_ddb_changed(struct skl_ddb_allocation *old,
+		     struct skl_ddb_allocation *new,
+		     enum pipe pipe)
+{
+	if (memcmp(&old->pipe[pipe], &new->pipe[pipe],
+		   sizeof(old->pipe[pipe])) != 0 ||
+	    memcmp(&old->plane[pipe], &new->plane[pipe],
+		   sizeof(old->plane[pipe])) != 0 ||
+	    memcmp(&old->y_plane[pipe], &new->y_plane[pipe],
+		   sizeof(old->y_plane[pipe])) != 0)
+		return true;
+
+	return false;
+}
+
 static int
 skl_compute_ddb(struct drm_atomic_state *state)
 {
@@ -3933,7 +3955,8 @@ skl_compute_ddb(struct drm_atomic_state *state)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct intel_crtc *intel_crtc;
-	struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb;
+	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
+	struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb;
 	uint32_t realloc_pipes = pipes_modified(state);
 	int ret;
 
@@ -3971,9 +3994,13 @@ skl_compute_ddb(struct drm_atomic_state *state)
 		if (IS_ERR(cstate))
 			return PTR_ERR(cstate);
 
-		ret = skl_allocate_pipe_ddb(cstate, ddb);
+		ret = skl_allocate_pipe_ddb(cstate, new_ddb);
 		if (ret)
 			return ret;
+
+		if (!intel_state->wm_results.ddb_changed &&
+		    skl_pipe_ddb_changed(old_ddb, new_ddb, intel_crtc->pipe))
+			intel_state->wm_results.ddb_changed = true;
 	}
 
 	return 0;
-- 
2.7.4

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

[PATCH 6/6] drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()

[Lyude]

Similar to how a vehicle will travel faster if you paint flames on it,
cleaning up this extra whitespace is guaranteed to provide additional
stability while updating watermark values.

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4178bdd..a08a638 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3837,7 +3837,6 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 		reallocated[pipe] = true;
 	}
 
-
 	/*
 	 * Second pass: flush the pipes that are having their allocation
 	 * reduced, but overlapping with a previous allocation.
-- 
2.7.4

[PATCH 6/6] drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()

[Lyude]

Similar to how a vehicle will travel faster if you paint flames on it,
cleaning up this extra whitespace is guaranteed to provide additional
stability while updating watermark values.

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4178bdd..a08a638 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3837,7 +3837,6 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 		reallocated[pipe] = true;
 	}
 
-
 	/*
 	 * Second pass: flush the pipes that are having their allocation
 	 * reduced, but overlapping with a previous allocation.
-- 
2.7.4

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

Re: [PATCH 1/6] drm/i915/skl: Update plane watermarks atomically during plane updates

[Matt Roper]

On Wed, Jul 20, 2016 at 04:59:57PM -0400, Lyude wrote:
> 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:
> 
>   non-modeset {
>    - calculate (during atomic check phase)
>    - finish_atomic_commit:
>      - intel_pre_plane_update:
>         - intel_update_watermarks()
>      - {vblank happens; new watermarks + old plane values => underrun }
>      - drm_atomic_helper_commit_planes_on_crtc:
>         - start vblank evasion
>         - write new plane registers
>         - end vblank evasion
>   }
> 
>   or
> 
>   modeset {
>    - calculate (during atomic check phase)
>    - finish_atomic_commit:
>      - crtc_enable:
>         - intel_update_watermarks()
>      - {vblank happens; new watermarks + old plane values => underrun }
>      - drm_atomic_helper_commit_planes_on_crtc:
>         - start vblank evasion
>         - write new plane registers
>         - end vblank evasion
>   }
> 
> Now we update watermarks atomically like this:
> 
>   non-modeset {
>    - calculate (during atomic check phase)
>    - finish_atomic_commit:
>      - intel_pre_plane_update:
>         - intel_update_watermarks() (wm values aren't written yet)
>      - drm_atomic_helper_commit_planes_on_crtc:
>         - start vblank evasion
>         - write new plane registers
>         - write new wm values
>         - end vblank evasion
>   }
> 
>   modeset {
>    - calculate (during atomic check phase)
>    - finish_atomic_commit:
>      - crtc_enable:
>         - intel_update_watermarks() (actual wm values aren't written
>           yet)
>      - drm_atomic_helper_commit_planes_on_crtc:
>         - start vblank evasion
>         - write new plane registers
> 	- write new wm values
>         - end vblank evasion
>   }
> 
> Which is more of a step in the right direction to fixing all of the
> underrun issues we're currently seeing with skl

So my understanding of this patch is that it moves watermark value
writes to the right place (inside the vblank evasion where we write the
rest of the plane registers), but we aren't tackling the problems with
DDB writes & flushing order yet, so we don't expect everything to be
fixed yet, right?  You might want to clarify that slightly in the commit
message here. 

One other comment inline below.

> 
> Changes since original patch series:
> - Remove mutex_lock/mutex_unlock since they don't do anything and we're
>   not touching global state
> - Move skl_write_cursor_wm/skl_write_plane_wm functions into intel_pm.c, make
>   externally visible
> - Add skl_write_plane_wm calls to skl_update_plane
> - Fix conditional for for loop in skl_write_plane_wm (level < max_level
>   should be level <= max_level)
> - Make diagram in commit more accurate to what's actually happening
> - Add Fixes:
> 
> Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
> Signed-off-by: Lyude <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 |  5 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c      | 48 +++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_sprite.c  |  2 ++
>  4 files changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 78beb7e..c0d2074 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3031,6 +3031,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	intel_crtc->adjusted_x = x_offset;
>  	intel_crtc->adjusted_y = y_offset;
>  
> +	skl_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);
> @@ -10242,6 +10244,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))

I believe this should be IS_GEN9 so that it applies to BXT and KBL too.


Matt

> +		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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e74d851..f1f54d9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1709,6 +1709,8 @@ 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 */);
> +void skl_write_cursor_wm(struct intel_crtc *intel_crtc);
> +void skl_write_plane_wm(struct intel_crtc *intel_crtc, int plane);
>  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 64d628c..fa86bea 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3680,6 +3680,39 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
>  		I915_WRITE(reg, 0);
>  }
>  
> +void skl_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;
> +
> +	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]);
> +}
> +
> +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;
> +
> +	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]);
> +}
> +
>  static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  				const struct skl_wm_values *new)
>  {
> @@ -3687,7 +3720,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  	struct intel_crtc *crtc;
>  
>  	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)
> @@ -3697,19 +3730,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),
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0de935a..50026f1 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -238,6 +238,8 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	crtc_w--;
>  	crtc_h--;
>  
> +	skl_write_plane_wm(to_intel_crtc(crtc_state->base.crtc), plane);
> +
>  	if (key->flags) {
>  		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
>  		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
> -- 
> 2.7.4
> 

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

Re: [PATCH 1/6] drm/i915/skl: Update plane watermarks atomically during plane updates

[Matt Roper]

On Wed, Jul 20, 2016 at 04:59:57PM -0400, Lyude wrote:
> 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:
> 
>   non-modeset {
>    - calculate (during atomic check phase)
>    - finish_atomic_commit:
>      - intel_pre_plane_update:
>         - intel_update_watermarks()
>      - {vblank happens; new watermarks + old plane values => underrun }
>      - drm_atomic_helper_commit_planes_on_crtc:
>         - start vblank evasion
>         - write new plane registers
>         - end vblank evasion
>   }
> 
>   or
> 
>   modeset {
>    - calculate (during atomic check phase)
>    - finish_atomic_commit:
>      - crtc_enable:
>         - intel_update_watermarks()
>      - {vblank happens; new watermarks + old plane values => underrun }
>      - drm_atomic_helper_commit_planes_on_crtc:
>         - start vblank evasion
>         - write new plane registers
>         - end vblank evasion
>   }
> 
> Now we update watermarks atomically like this:
> 
>   non-modeset {
>    - calculate (during atomic check phase)
>    - finish_atomic_commit:
>      - intel_pre_plane_update:
>         - intel_update_watermarks() (wm values aren't written yet)
>      - drm_atomic_helper_commit_planes_on_crtc:
>         - start vblank evasion
>         - write new plane registers
>         - write new wm values
>         - end vblank evasion
>   }
> 
>   modeset {
>    - calculate (during atomic check phase)
>    - finish_atomic_commit:
>      - crtc_enable:
>         - intel_update_watermarks() (actual wm values aren't written
>           yet)
>      - drm_atomic_helper_commit_planes_on_crtc:
>         - start vblank evasion
>         - write new plane registers
> 	- write new wm values
>         - end vblank evasion
>   }
> 
> Which is more of a step in the right direction to fixing all of the
> underrun issues we're currently seeing with skl

So my understanding of this patch is that it moves watermark value
writes to the right place (inside the vblank evasion where we write the
rest of the plane registers), but we aren't tackling the problems with
DDB writes & flushing order yet, so we don't expect everything to be
fixed yet, right?  You might want to clarify that slightly in the commit
message here. 

One other comment inline below.

> 
> Changes since original patch series:
> - Remove mutex_lock/mutex_unlock since they don't do anything and we're
>   not touching global state
> - Move skl_write_cursor_wm/skl_write_plane_wm functions into intel_pm.c, make
>   externally visible
> - Add skl_write_plane_wm calls to skl_update_plane
> - Fix conditional for for loop in skl_write_plane_wm (level < max_level
>   should be level <= max_level)
> - Make diagram in commit more accurate to what's actually happening
> - Add Fixes:
> 
> Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
> Signed-off-by: Lyude <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 |  5 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c      | 48 +++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_sprite.c  |  2 ++
>  4 files changed, 43 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 78beb7e..c0d2074 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3031,6 +3031,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	intel_crtc->adjusted_x = x_offset;
>  	intel_crtc->adjusted_y = y_offset;
>  
> +	skl_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);
> @@ -10242,6 +10244,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))

I believe this should be IS_GEN9 so that it applies to BXT and KBL too.


Matt

> +		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_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e74d851..f1f54d9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1709,6 +1709,8 @@ 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 */);
> +void skl_write_cursor_wm(struct intel_crtc *intel_crtc);
> +void skl_write_plane_wm(struct intel_crtc *intel_crtc, int plane);
>  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 64d628c..fa86bea 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3680,6 +3680,39 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv,
>  		I915_WRITE(reg, 0);
>  }
>  
> +void skl_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;
> +
> +	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]);
> +}
> +
> +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;
> +
> +	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]);
> +}
> +
>  static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  				const struct skl_wm_values *new)
>  {
> @@ -3687,7 +3720,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  	struct intel_crtc *crtc;
>  
>  	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)
> @@ -3697,19 +3730,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),
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0de935a..50026f1 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -238,6 +238,8 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	crtc_w--;
>  	crtc_h--;
>  
> +	skl_write_plane_wm(to_intel_crtc(crtc_state->base.crtc), plane);
> +
>  	if (key->flags) {
>  		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
>  		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
> -- 
> 2.7.4
> 

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

Re: [PATCH 2/6] drm/i915/gen9: Only copy WM results for changed pipes to skl_hw

[Matt Roper]

On Wed, Jul 20, 2016 at 04:59:58PM -0400, Lyude wrote:
> From: Matt Roper <matthew.d.roper@intel.com>
> 
> When we write watermark values to the hardware, those values are stored
> in dev_priv->wm.skl_hw.  However with recent watermark changes, the
> results structure we're copying from only contains valid watermark and
> DDB values for the pipes that are actually changing; the values for
> other pipes remain 0.  Thus a blind copy of the entire skl_wm_values
> structure will clobber the values for unchanged pipes...we need to be
> more selective and only copy over the values for the changing pipes.
> 
> This mistake was hidden until recently due to another bug that caused us
> to erroneously re-calculate watermarks for all active pipes rather than
> changing pipes.  Only when that bug was fixed was the impact of this bug
> discovered (e.g., modesets failing with "Requested display configuration
> exceeds system watermark limitations" messages and leaving watermarks
> non-functional, even ones initiated by intel_fbdev_restore_mode).
> 
> Changes since v1:
>  - Add a function for copying a pipe's wm values
>    (skl_copy_wm_for_pipe()) so we can reuse this later

Your changes look good to me.

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

> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Fixes: 734fa01f3a17 ("drm/i915/gen9: Calculate watermarks during atomic 'check' (v2)")
> Fixes: 9b6130227495 ("drm/i915/gen9: Re-allocate DDB only for changed pipes")
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fa86bea..b7d4af1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3966,6 +3966,24 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static void
> +skl_copy_wm_for_pipe(struct skl_wm_values *dst,
> +		     struct skl_wm_values *src,
> +		     enum pipe pipe)
> +{
> +	dst->wm_linetime[pipe] = src->wm_linetime[pipe];
> +	memcpy(dst->plane[pipe], src->plane[pipe],
> +	       sizeof(dst->plane[pipe]));
> +	memcpy(dst->plane_trans[pipe], src->plane_trans[pipe],
> +	       sizeof(dst->plane_trans[pipe]));
> +
> +	dst->ddb.pipe[pipe] = src->ddb.pipe[pipe];
> +	memcpy(dst->ddb.y_plane[pipe], src->ddb.y_plane[pipe],
> +	       sizeof(dst->ddb.y_plane[pipe]));
> +	memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe],
> +	       sizeof(dst->ddb.plane[pipe]));
> +}
> +
>  static int
>  skl_compute_wm(struct drm_atomic_state *state)
>  {
> @@ -4038,8 +4056,10 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct skl_wm_values *results = &dev_priv->wm.skl_results;
> +	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;
>  
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
>  		return;
> @@ -4051,8 +4071,12 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	skl_write_wm_values(dev_priv, results);
>  	skl_flush_wm_values(dev_priv, results);
>  
> -	/* store the new configuration */
> -	dev_priv->wm.skl_hw = *results;
> +	/*
> +	 * Store the new configuration (but only for the pipes that have
> +	 * changed; the other values weren't recomputed).
> +	 */
> +	for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes)
> +		skl_copy_wm_for_pipe(hw_vals, results, pipe);
>  
>  	mutex_unlock(&dev_priv->wm.wm_mutex);
>  }
> -- 
> 2.7.4
> 

-- 
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

Re: [PATCH 3/6] drm/i915/skl: Actually reuse wm values when pipes don't change

[Matt Roper]

On Wed, Jul 20, 2016 at 04:59:59PM -0400, Lyude wrote:
> Up until now we've actually been making the mistake of leaving the
> watermark results for each pipe completely blank in skl_compute_wm()
> when they haven't changed, fix this.

Should this be moved before patch #1?  With the existing code we don't
try to re-write watermark registers if they aren't changing, so leaving
them at zero should be safe.  I think we want to make this change before
we start re-writing non-dirty watermarks.  Alternatively, we could just
add the dirty bit test to the appropriate places in patch #1 and not
worry about copying over the unchanged values.


Matt

> 
> Fixes: 734fa01f3a17 ("drm/i915/gen9: Calculate watermarks during atomic 'check' (v2)")
> 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>
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b7d4af1..788db86 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3987,10 +3987,13 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst,
>  static int
>  skl_compute_wm(struct drm_atomic_state *state)
>  {
> +	struct drm_device *dev = state->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc *crtc;
>  	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 skl_wm_values *hw_wm = &dev_priv->wm.skl_hw;
>  	struct skl_pipe_wm *pipe_wm;
>  	bool changed = false;
>  	int ret, i;
> @@ -4039,12 +4042,14 @@ skl_compute_wm(struct drm_atomic_state *state)
>  		if (changed)
>  			results->dirty_pipes |= drm_crtc_mask(crtc);
>  
> -		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> +		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) {
>  			/* This pipe's WM's did not change */
> +			skl_copy_wm_for_pipe(results, hw_wm, intel_crtc->pipe);
>  			continue;
> +		}
>  
>  		intel_cstate->update_wm_pre = true;
> -		skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
> +		skl_compute_wm_results(dev, pipe_wm, results, intel_crtc);
>  	}
>  
>  	return 0;
> -- 
> 2.7.4
> 

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

Re: [PATCH 3/6] drm/i915/skl: Actually reuse wm values when pipes don't change

[Matt Roper]

On Wed, Jul 20, 2016 at 04:59:59PM -0400, Lyude wrote:
> Up until now we've actually been making the mistake of leaving the
> watermark results for each pipe completely blank in skl_compute_wm()
> when they haven't changed, fix this.

Should this be moved before patch #1?  With the existing code we don't
try to re-write watermark registers if they aren't changing, so leaving
them at zero should be safe.  I think we want to make this change before
we start re-writing non-dirty watermarks.  Alternatively, we could just
add the dirty bit test to the appropriate places in patch #1 and not
worry about copying over the unchanged values.


Matt

> 
> Fixes: 734fa01f3a17 ("drm/i915/gen9: Calculate watermarks during atomic 'check' (v2)")
> 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>
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b7d4af1..788db86 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3987,10 +3987,13 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst,
>  static int
>  skl_compute_wm(struct drm_atomic_state *state)
>  {
> +	struct drm_device *dev = state->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_crtc *crtc;
>  	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 skl_wm_values *hw_wm = &dev_priv->wm.skl_hw;
>  	struct skl_pipe_wm *pipe_wm;
>  	bool changed = false;
>  	int ret, i;
> @@ -4039,12 +4042,14 @@ skl_compute_wm(struct drm_atomic_state *state)
>  		if (changed)
>  			results->dirty_pipes |= drm_crtc_mask(crtc);
>  
> -		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
> +		if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) {
>  			/* This pipe's WM's did not change */
> +			skl_copy_wm_for_pipe(results, hw_wm, intel_crtc->pipe);
>  			continue;
> +		}
>  
>  		intel_cstate->update_wm_pre = true;
> -		skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
> +		skl_compute_wm_results(dev, pipe_wm, results, intel_crtc);
>  	}
>  
>  	return 0;
> -- 
> 2.7.4
> 

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

Re: [PATCH 4/6] drm/i915/skl: Always wait for pipes to update after a flush

[Matt Roper]

On Wed, Jul 20, 2016 at 05:00:00PM -0400, Lyude wrote:
> As we've learned, all watermark updates on Skylake have to be strictly
> atomic or things fail. While the bspec doesn't mandate that we need to
> wait for pipes to finish after the third iteration of flushes, not doing
> so gives us the opportunity to break this atomicity later. This example
> assumes that we're lucky enough not to be interrupted by the scheduler
> at any point during this:
> 
>  - Start with pipe A and pipe B enabled
>  - Enable pipe C
>  - Flush pipe A in pass 1, wait until update finishes
>  - Flush pipe B in pass 3, continue without waiting for next vblank
>  - Start another wm update
>  - We enter the next vblank for pipe B before we finish writing all the
>    vm values
>  - *Underrun*
> 
> As such, we always need to wait for each pipe we flush to update so as
> to never break this atomicity.

I'm not sure I follow this commit.  If we're enabling a new pipe, the
the allocation for A and B are generally going to shrink, so they'll
usually be flushed in passes 1 and 2, not 3.

My understanding is that the problem that still remains (now that your
first three patches have made progress towards fixing underruns) is that
our DDB updates and flushes (which come from update_watermarks) happen
pre-evasion, whereas the watermarks themselves now happen under evasion.
We really want both the new DDB value and the new watermark value to be
written together and take effect on the same vblank.  I think the
problem is that you might have a shrinking DDB allocation (e.g., because
a new pipe was added or you changed a mode that changed the DDB balance)
which some of the existing WM values exceed.  You can have a sequence
like this:

  - update_wm:
     - write new (smaller) DDB
     - flush DDB
  - vblank happens, old (big) wm + new (small) ddb = underrun
  - vblank evasion:
     - write new plane regs and WM's
     - flush
  - post-evasion vblank happens, underrun is corrected

I think ultimately we want to move the DDB register writes into the
update functions that happen under evasion, just like you did for the WM
registers.  However just doing this the straightforward way won't
satisfy our requirements about pipe update ordering (the three passes
you see today in skl_flush_wm_values).  To make that work, I think the
general approach is that we need to basically replace the
for_each_crtc_in_state() loop in intel_atomic_commit_tail() with some
new CRTC iterator that processes CRTC's in a more intelligent ordering.
We've computed our DDB changes during the atomic check phase, so we
already know which allocations are shrinking, growing, etc. and we
should be able to calculate an appropriate CRTC ordering at the same
time.

With an intelligent CRTC iterator that follows the pre-computed pipe
ordering rules (and adds the necessary vblank waits between each
"phase"), I think we should be able to just write both DDB and WM values
in the skl_update_primary_plane() and similar functions and let the
existing flushes that happen take care of flushing them out at the
appropriate time.  Of course I've kicked that idea around in my head for
a while, but haven't had time to actually write any code for it, so I
may be completely overlooking some stumbling block that makes it much
more complicated than I'm envisioning.


Matt

> 
> Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration")
> Signed-off-by: Lyude <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_pm.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 788db86..2e31df4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3859,8 +3859,11 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  	/*
>  	 * 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.
> +	 * While the hardware doesn't require to wait for the next vblank here,
> +	 * continuing before the pipe finishes updating could result in us
> +	 * trying to update the wm values again before the pipe finishes
> +	 * updating, which results in the hardware using intermediate wm values
> +	 * and subsequently underrunning pipes.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
>  		if (!crtc->active)
> @@ -3876,6 +3879,16 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  			continue;
>  
>  		skl_wm_flush_pipe(dev_priv, pipe, 3);
> +
> +		/*
> +		 * The only time we can get away with not waiting for an update
> +		 * is when we just enabled the pipe, e.g. when it doesn't have
> +		 * vblanks enabled anyway.
> +		 */
> +		if (drm_crtc_vblank_get(&crtc->base) == 0) {
> +			intel_wait_for_vblank(dev, pipe);
> +			drm_crtc_vblank_put(&crtc->base);
> +		}
>  	}
>  }
>  
> -- 
> 2.7.4
> 

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

Re: [PATCH 4/6] drm/i915/skl: Always wait for pipes to update after a flush

[Matt Roper]

On Wed, Jul 20, 2016 at 05:00:00PM -0400, Lyude wrote:
> As we've learned, all watermark updates on Skylake have to be strictly
> atomic or things fail. While the bspec doesn't mandate that we need to
> wait for pipes to finish after the third iteration of flushes, not doing
> so gives us the opportunity to break this atomicity later. This example
> assumes that we're lucky enough not to be interrupted by the scheduler
> at any point during this:
> 
>  - Start with pipe A and pipe B enabled
>  - Enable pipe C
>  - Flush pipe A in pass 1, wait until update finishes
>  - Flush pipe B in pass 3, continue without waiting for next vblank
>  - Start another wm update
>  - We enter the next vblank for pipe B before we finish writing all the
>    vm values
>  - *Underrun*
> 
> As such, we always need to wait for each pipe we flush to update so as
> to never break this atomicity.

I'm not sure I follow this commit.  If we're enabling a new pipe, the
the allocation for A and B are generally going to shrink, so they'll
usually be flushed in passes 1 and 2, not 3.

My understanding is that the problem that still remains (now that your
first three patches have made progress towards fixing underruns) is that
our DDB updates and flushes (which come from update_watermarks) happen
pre-evasion, whereas the watermarks themselves now happen under evasion.
We really want both the new DDB value and the new watermark value to be
written together and take effect on the same vblank.  I think the
problem is that you might have a shrinking DDB allocation (e.g., because
a new pipe was added or you changed a mode that changed the DDB balance)
which some of the existing WM values exceed.  You can have a sequence
like this:

  - update_wm:
     - write new (smaller) DDB
     - flush DDB
  - vblank happens, old (big) wm + new (small) ddb = underrun
  - vblank evasion:
     - write new plane regs and WM's
     - flush
  - post-evasion vblank happens, underrun is corrected

I think ultimately we want to move the DDB register writes into the
update functions that happen under evasion, just like you did for the WM
registers.  However just doing this the straightforward way won't
satisfy our requirements about pipe update ordering (the three passes
you see today in skl_flush_wm_values).  To make that work, I think the
general approach is that we need to basically replace the
for_each_crtc_in_state() loop in intel_atomic_commit_tail() with some
new CRTC iterator that processes CRTC's in a more intelligent ordering.
We've computed our DDB changes during the atomic check phase, so we
already know which allocations are shrinking, growing, etc. and we
should be able to calculate an appropriate CRTC ordering at the same
time.

With an intelligent CRTC iterator that follows the pre-computed pipe
ordering rules (and adds the necessary vblank waits between each
"phase"), I think we should be able to just write both DDB and WM values
in the skl_update_primary_plane() and similar functions and let the
existing flushes that happen take care of flushing them out at the
appropriate time.  Of course I've kicked that idea around in my head for
a while, but haven't had time to actually write any code for it, so I
may be completely overlooking some stumbling block that makes it much
more complicated than I'm envisioning.


Matt

> 
> Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration")
> Signed-off-by: Lyude <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_pm.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 788db86..2e31df4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3859,8 +3859,11 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  	/*
>  	 * 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.
> +	 * While the hardware doesn't require to wait for the next vblank here,
> +	 * continuing before the pipe finishes updating could result in us
> +	 * trying to update the wm values again before the pipe finishes
> +	 * updating, which results in the hardware using intermediate wm values
> +	 * and subsequently underrunning pipes.
>  	 */
>  	for_each_intel_crtc(dev, crtc) {
>  		if (!crtc->active)
> @@ -3876,6 +3879,16 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  			continue;
>  
>  		skl_wm_flush_pipe(dev_priv, pipe, 3);
> +
> +		/*
> +		 * The only time we can get away with not waiting for an update
> +		 * is when we just enabled the pipe, e.g. when it doesn't have
> +		 * vblanks enabled anyway.
> +		 */
> +		if (drm_crtc_vblank_get(&crtc->base) == 0) {
> +			intel_wait_for_vblank(dev, pipe);
> +			drm_crtc_vblank_put(&crtc->base);
> +		}
>  	}
>  }
>  
> -- 
> 2.7.4
> 

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

✗ Ro.CI.BAT: failure for drm/i915/skl: Finally fix watermarks

[Patchwork]

== Series Details ==

Series: drm/i915/skl: Finally fix watermarks
URL   : https://patchwork.freedesktop.org/series/10108/
State : failure

== Summary ==

Series 10108v1 drm/i915/skl: Finally fix watermarks
http://patchwork.freedesktop.org/api/1.0/series/10108/revisions/1/mbox

Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> INCOMPLETE (fi-hsw-i7-4770k)

fi-hsw-i7-4770k  total:107  pass:91   dwarn:0   dfail:0   fail:0   skip:15 
fi-kbl-qkkr      total:244  pass:179  dwarn:29  dfail:0   fail:8   skip:28 
fi-skl-i5-6260u  total:244  pass:224  dwarn:0   dfail:0   fail:8   skip:12 
fi-skl-i7-6700k  total:244  pass:210  dwarn:0   dfail:0   fail:8   skip:26 
fi-snb-i7-2600   total:244  pass:196  dwarn:0   dfail:0   fail:8   skip:40 
ro-bdw-i5-5250u  total:244  pass:219  dwarn:4   dfail:0   fail:8   skip:13 
ro-bdw-i7-5600u  total:244  pass:204  dwarn:0   dfail:0   fail:8   skip:32 
ro-bsw-n3050     total:218  pass:173  dwarn:0   dfail:0   fail:2   skip:42 
ro-byt-n2820     total:244  pass:197  dwarn:0   dfail:0   fail:9   skip:38 
ro-hsw-i3-4010u  total:244  pass:212  dwarn:0   dfail:0   fail:8   skip:24 
ro-hsw-i7-4770r  total:244  pass:212  dwarn:0   dfail:0   fail:8   skip:24 
ro-ilk-i7-620lm  total:244  pass:172  dwarn:0   dfail:0   fail:9   skip:63 
ro-ilk1-i5-650   total:239  pass:172  dwarn:0   dfail:0   fail:9   skip:58 
ro-ivb-i7-3770   total:244  pass:203  dwarn:0   dfail:0   fail:8   skip:33 
ro-skl3-i5-6260u total:244  pass:224  dwarn:0   dfail:0   fail:8   skip:12 
ro-snb-i7-2620M  total:244  pass:193  dwarn:0   dfail:0   fail:9   skip:42 
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1554/

621e9dc drm-intel-nightly: 2016y-07m-20d-19h-19m-37s UTC integration manifest
9bf0f61 drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()
84afdf6 drm/i915/skl: Only flush pipes when we change the ddb allocation
4b2399f drm/i915/skl: Always wait for pipes to update after a flush
4762e9a drm/i915/skl: Actually reuse wm values when pipes don't change
e6daaf7 drm/i915/gen9: Only copy WM results for changed pipes to skl_hw
dae55b1 drm/i915/skl: Update plane watermarks atomically during plane updates

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

Re: [PATCH 4/6] drm/i915/skl: Always wait for pipes to update after a flush

[Lyude Paul]

Two things for this one:
1. I completely messed up the description on this patchset, this was for fixing
underruns on pipe disablement. I'm impressed I wrote up that whole description
without realizing that at all, lol.
2. This patch may not actually be necessary. On the original git branch I was
testing this on it was required to prevent underruns on pipe disables, but
trying this on nightly I don't seem to reproduce those underruns even when I
remove this patch, so I guess we can drop this from the series

On Wed, 2016-07-20 at 17:27 -0700, Matt Roper wrote:
> On Wed, Jul 20, 2016 at 05:00:00PM -0400, Lyude wrote:
> > 
> > As we've learned, all watermark updates on Skylake have to be strictly
> > atomic or things fail. While the bspec doesn't mandate that we need to
> > wait for pipes to finish after the third iteration of flushes, not doing
> > so gives us the opportunity to break this atomicity later. This example
> > assumes that we're lucky enough not to be interrupted by the scheduler
> > at any point during this:
> > 
> >  - Start with pipe A and pipe B enabled
> >  - Enable pipe C
> >  - Flush pipe A in pass 1, wait until update finishes
> >  - Flush pipe B in pass 3, continue without waiting for next vblank
> >  - Start another wm update
> >  - We enter the next vblank for pipe B before we finish writing all the
> >    vm values
> >  - *Underrun*
> > 
> > As such, we always need to wait for each pipe we flush to update so as
> > to never break this atomicity.
> 
> I'm not sure I follow this commit.  If we're enabling a new pipe, the
> the allocation for A and B are generally going to shrink, so they'll
> usually be flushed in passes 1 and 2, not 3.
> 
> My understanding is that the problem that still remains (now that your
> first three patches have made progress towards fixing underruns) is that
> our DDB updates and flushes (which come from update_watermarks) happen
> pre-evasion, whereas the watermarks themselves now happen under evasion.
> We really want both the new DDB value and the new watermark value to be
> written together and take effect on the same vblank.  I think the
> problem is that you might have a shrinking DDB allocation (e.g., because
> a new pipe was added or you changed a mode that changed the DDB balance)
> which some of the existing WM values exceed.  You can have a sequence
> like this:
> 
>   - update_wm:
>      - write new (smaller) DDB
>      - flush DDB
>   - vblank happens, old (big) wm + new (small) ddb = underrun
>   - vblank evasion:
>      - write new plane regs and WM's
>      - flush
>   - post-evasion vblank happens, underrun is corrected
> 
> I think ultimately we want to move the DDB register writes into the
> update functions that happen under evasion, just like you did for the WM
> registers.  However just doing this the straightforward way won't
> satisfy our requirements about pipe update ordering (the three passes
> you see today in skl_flush_wm_values).  To make that work, I think the
> general approach is that we need to basically replace the
> for_each_crtc_in_state() loop in intel_atomic_commit_tail() with some
> new CRTC iterator that processes CRTC's in a more intelligent ordering.
> We've computed our DDB changes during the atomic check phase, so we
> already know which allocations are shrinking, growing, etc. and we
> should be able to calculate an appropriate CRTC ordering at the same
> time.
> 
> With an intelligent CRTC iterator that follows the pre-computed pipe
> ordering rules (and adds the necessary vblank waits between each
> "phase"), I think we should be able to just write both DDB and WM values
> in the skl_update_primary_plane() and similar functions and let the
> existing flushes that happen take care of flushing them out at the
> appropriate time.  Of course I've kicked that idea around in my head for
> a while, but haven't had time to actually write any code for it, so I
> may be completely overlooking some stumbling block that makes it much
> more complicated than I'm envisioning.
> 
> 
> Matt
> 
> > 
> > 
> > Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration")
> > Signed-off-by: Lyude <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_pm.c | 17 +++++++++++++++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 788db86..2e31df4 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3859,8 +3859,11 @@ static void skl_flush_wm_values(struct
> > drm_i915_private *dev_priv,
> >  	/*
> >  	 * 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.
> > +	 * While the hardware doesn't require to wait for the next vblank
> > here,
> > +	 * continuing before the pipe finishes updating could result in us
> > +	 * trying to update the wm values again before the pipe finishes
> > +	 * updating, which results in the hardware using intermediate wm
> > values
> > +	 * and subsequently underrunning pipes.
> >  	 */
> >  	for_each_intel_crtc(dev, crtc) {
> >  		if (!crtc->active)
> > @@ -3876,6 +3879,16 @@ static void skl_flush_wm_values(struct
> > drm_i915_private *dev_priv,
> >  			continue;
> >  
> >  		skl_wm_flush_pipe(dev_priv, pipe, 3);
> > +
> > +		/*
> > +		 * The only time we can get away with not waiting for an
> > update
> > +		 * is when we just enabled the pipe, e.g. when it doesn't
> > have
> > +		 * vblanks enabled anyway.
> > +		 */
> > +		if (drm_crtc_vblank_get(&crtc->base) == 0) {
> > +			intel_wait_for_vblank(dev, pipe);
> > +			drm_crtc_vblank_put(&crtc->base);
> > +		}
> >  	}
> >  }
> >  
> > -- 
> > 2.7.4
> > 
>