[PATCH v11 0/7] Finally fix watermarks

[PATCH v11 0/7] Finally fix watermarks

[Lyude]

Maarten Lankhorst unfortunately found that some early pre-release skl machines
were failing tests due to them apparently not having SAGVs. As a result, a
patch had to be added to check for invalid pcode commands, and the SAGV support
patch has been reworked to stop trying to play with the SAGV if it's not
controllable on the system. As well, this means the RBs for:

drm/i915/skl: Add support for the SAGV, fix underrun hangs

Had to be cleared, seeing as the patch has changed considerably now.

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

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

 drivers/gpu/drm/i915/i915_drv.h      |   9 +
 drivers/gpu/drm/i915/i915_reg.h      |   5 +
 drivers/gpu/drm/i915/intel_display.c | 198 +++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  14 ++
 drivers/gpu/drm/i915/intel_pm.c      | 356 +++++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_sprite.c  |   6 +
 6 files changed, 395 insertions(+), 193 deletions(-)

-- 
2.7.4

[PATCH v11 0/7] Finally fix watermarks

[Lyude]

Maarten Lankhorst unfortunately found that some early pre-release skl machines
were failing tests due to them apparently not having SAGVs. As a result, a
patch had to be added to check for invalid pcode commands, and the SAGV support
patch has been reworked to stop trying to play with the SAGV if it's not
controllable on the system. As well, this means the RBs for:

drm/i915/skl: Add support for the SAGV, fix underrun hangs

Had to be cleared, seeing as the patch has changed considerably now.

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

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

 drivers/gpu/drm/i915/i915_drv.h      |   9 +
 drivers/gpu/drm/i915/i915_reg.h      |   5 +
 drivers/gpu/drm/i915/intel_display.c | 198 +++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  14 ++
 drivers/gpu/drm/i915/intel_pm.c      | 356 +++++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_sprite.c  |   6 +
 6 files changed, 395 insertions(+), 193 deletions(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v11 1/7] drm/i915/gen6+: Return -EINVAL on invalid pcode commands

[Lyude]

In order to add proper support for the SAGV, we need to be able to know
what the cause of a failure to change the SAGV through the pcode mailbox
was. The reasoning for this is that some very early pre-release Skylake
machines don't actually allow you to control the SAGV on them, and
indicate an invalid mailbox command was sent.

This also might come in handy in the future for debugging.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index da82744..73b3d4d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7121,6 +7121,7 @@ enum {
 #define VLV_MEDIA_C0_COUNT			_MMIO(0x13811C)
 
 #define GEN6_PCODE_MAILBOX			_MMIO(0x138124)
+#define   GEN6_PCODE_INVALID_CMD		(1<<0)
 #define   GEN6_PCODE_READY			(1<<31)
 #define	  GEN6_PCODE_WRITE_RC6VIDS		0x4
 #define	  GEN6_PCODE_READ_RC6VIDS		0x5
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 99014d7..8752730 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7674,6 +7674,11 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
 	*val = I915_READ_FW(GEN6_PCODE_DATA);
 	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
 
+	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_INVALID_CMD) {
+		DRM_DEBUG_DRIVER("warning: pcode (read) mailbox access indicated invalid command\n");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -7704,6 +7709,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
 
 	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
 
+	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_INVALID_CMD) {
+		DRM_DEBUG_DRIVER("warning: pcode (write) mailbox access indicated invalid command\n");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH v11 1/7] drm/i915/gen6+: Return -EINVAL on invalid pcode commands

[Lyude]

In order to add proper support for the SAGV, we need to be able to know
what the cause of a failure to change the SAGV through the pcode mailbox
was. The reasoning for this is that some very early pre-release Skylake
machines don't actually allow you to control the SAGV on them, and
indicate an invalid mailbox command was sent.

This also might come in handy in the future for debugging.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index da82744..73b3d4d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7121,6 +7121,7 @@ enum {
 #define VLV_MEDIA_C0_COUNT			_MMIO(0x13811C)
 
 #define GEN6_PCODE_MAILBOX			_MMIO(0x138124)
+#define   GEN6_PCODE_INVALID_CMD		(1<<0)
 #define   GEN6_PCODE_READY			(1<<31)
 #define	  GEN6_PCODE_WRITE_RC6VIDS		0x4
 #define	  GEN6_PCODE_READ_RC6VIDS		0x5
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 99014d7..8752730 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7674,6 +7674,11 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
 	*val = I915_READ_FW(GEN6_PCODE_DATA);
 	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
 
+	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_INVALID_CMD) {
+		DRM_DEBUG_DRIVER("warning: pcode (read) mailbox access indicated invalid command\n");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -7704,6 +7709,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
 
 	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
 
+	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_INVALID_CMD) {
+		DRM_DEBUG_DRIVER("warning: pcode (write) mailbox access indicated invalid command\n");
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.7.4

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

[PATCH v11 2/7] drm/i915/skl: Add support for the SAGV, fix underrun hangs

[Lyude]

Since the watermark calculations for Skylake are still broken, we're apt
to hitting underruns very easily under multi-monitor configurations.
While it would be lovely if this was fixed, it's not. Another problem
that's been coming from this however, is the mysterious issue of
underruns causing full system hangs. An easy way to reproduce this with
a skylake system:

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

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

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

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

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7971c76..d74d166 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1945,6 +1945,13 @@ struct drm_i915_private {
 	struct i915_suspend_saved_registers regfile;
 	struct vlv_s0ix_state vlv_s0ix_state;
 
+	enum {
+		I915_SKL_SAGV_UNKNOWN = 0,
+		I915_SKL_SAGV_DISABLED,
+		I915_SKL_SAGV_ENABLED,
+		I915_SKL_SAGV_NOT_CONTROLLED
+	} skl_sagv_status;
+
 	struct {
 		/*
 		 * Raw watermark latency values:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 73b3d4d..4980cfe 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7144,6 +7144,10 @@ enum {
 #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
 #define   DISPLAY_IPS_CONTROL			0x19
 #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
+#define   GEN9_PCODE_SAGV_CONTROL		0x21
+#define     GEN9_SAGV_DISABLE			0x0
+#define     GEN9_SAGV_IS_DISABLED		0x1
+#define     GEN9_SAGV_DYNAMIC_FREQ              0x3
 #define GEN6_PCODE_DATA				_MMIO(0x138128)
 #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
 #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c5c0c35..35bdd67 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14142,6 +14142,14 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		     intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco))
 			dev_priv->display.modeset_commit_cdclk(state);
 
+		/*
+		 * SKL workaround: bspec recommends we disable the SAGV when we
+		 * have more then one pipe enabled
+		 */
+		if (IS_SKYLAKE(dev_priv) &&
+		    hweight32(intel_state->active_crtcs) > 1)
+			skl_disable_sagv(dev_priv);
+
 		intel_modeset_verify_disabled(dev);
 	}
 
@@ -14215,6 +14223,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
 	}
 
+	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
+	    hweight32(intel_state->active_crtcs) <= 1)
+		skl_enable_sagv(dev_priv);
+
 	drm_atomic_helper_commit_hw_done(state);
 
 	if (intel_state->modeset)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9539f0e..76e78b8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1721,6 +1721,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 */);
+int skl_enable_sagv(struct drm_i915_private *dev_priv);
+int skl_disable_sagv(struct drm_i915_private *dev_priv);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8752730..0a202264 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2883,6 +2883,95 @@ skl_wm_plane_id(const struct intel_plane *plane)
 	}
 }
 
+/*
+ * SAGV dynamically adjusts the system agent voltage and clock frequencies
+ * depending on power and performance requirements. The display engine access
+ * to system memory is blocked during the adjustment time. Having this enabled
+ * in multi-pipe configurations can cause issues (such as underruns causing
+ * full system hangs), and the bspec also suggests that software disable it
+ * when more then one pipe is enabled.
+ */
+int
+skl_enable_sagv(struct drm_i915_private *dev_priv)
+{
+	int ret;
+
+	if (dev_priv->skl_sagv_status &&
+	    dev_priv->skl_sagv_status != I915_SKL_SAGV_DISABLED)
+		return 0;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+	DRM_DEBUG_KMS("Enabling the SAGV\n");
+
+	ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+				      GEN9_SAGV_DYNAMIC_FREQ);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	if (!ret) {
+		dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED;
+	} else if (ret == -EINVAL) {
+		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
+		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
+		ret = 0;
+	} else {
+		DRM_ERROR("Failed to enable the SAGV\n");
+	}
+
+	/* We don't need to wait for SAGV when enabling */
+	return ret;
+}
+
+static int
+skl_do_sagv_disable(struct drm_i915_private *dev_priv)
+{
+	int ret;
+	uint32_t temp = GEN9_SAGV_DISABLE;
+
+	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+				     &temp);
+	if (ret) {
+		/*
+		 * Some very early Skylake systems don't actually let you
+		 * control the SAGV, which is normal.
+		 */
+		if (ret != -EINVAL)
+			DRM_ERROR("Failed to disable the SAGV\n");
+
+		return ret;
+	}
+
+	return temp & GEN9_SAGV_IS_DISABLED;
+}
+
+int
+skl_disable_sagv(struct drm_i915_private *dev_priv)
+{
+	int ret, result;
+
+	if (dev_priv->skl_sagv_status &&
+	    dev_priv->skl_sagv_status != I915_SKL_SAGV_ENABLED)
+		return 0;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+	DRM_DEBUG_KMS("Disabling the SAGV\n");
+
+	/* bspec says to keep retrying for at least 1 ms */
+	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	if (!ret) {
+		dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED;
+	} else if (ret == -ETIMEDOUT) {
+		DRM_ERROR("Request to disable SAGV timed out\n");
+	} else if (result == -EINVAL) {
+		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
+		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				   const struct intel_crtc_state *cstate,
-- 
2.7.4

[PATCH v11 2/7] drm/i915/skl: Add support for the SAGV, fix underrun hangs

[Lyude]

Since the watermark calculations for Skylake are still broken, we're apt
to hitting underruns very easily under multi-monitor configurations.
While it would be lovely if this was fixed, it's not. Another problem
that's been coming from this however, is the mysterious issue of
underruns causing full system hangs. An easy way to reproduce this with
a skylake system:

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

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

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

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

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

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

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7971c76..d74d166 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1945,6 +1945,13 @@ struct drm_i915_private {
 	struct i915_suspend_saved_registers regfile;
 	struct vlv_s0ix_state vlv_s0ix_state;
 
+	enum {
+		I915_SKL_SAGV_UNKNOWN = 0,
+		I915_SKL_SAGV_DISABLED,
+		I915_SKL_SAGV_ENABLED,
+		I915_SKL_SAGV_NOT_CONTROLLED
+	} skl_sagv_status;
+
 	struct {
 		/*
 		 * Raw watermark latency values:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 73b3d4d..4980cfe 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7144,6 +7144,10 @@ enum {
 #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
 #define   DISPLAY_IPS_CONTROL			0x19
 #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
+#define   GEN9_PCODE_SAGV_CONTROL		0x21
+#define     GEN9_SAGV_DISABLE			0x0
+#define     GEN9_SAGV_IS_DISABLED		0x1
+#define     GEN9_SAGV_DYNAMIC_FREQ              0x3
 #define GEN6_PCODE_DATA				_MMIO(0x138128)
 #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
 #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c5c0c35..35bdd67 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14142,6 +14142,14 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		     intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco))
 			dev_priv->display.modeset_commit_cdclk(state);
 
+		/*
+		 * SKL workaround: bspec recommends we disable the SAGV when we
+		 * have more then one pipe enabled
+		 */
+		if (IS_SKYLAKE(dev_priv) &&
+		    hweight32(intel_state->active_crtcs) > 1)
+			skl_disable_sagv(dev_priv);
+
 		intel_modeset_verify_disabled(dev);
 	}
 
@@ -14215,6 +14223,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
 	}
 
+	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
+	    hweight32(intel_state->active_crtcs) <= 1)
+		skl_enable_sagv(dev_priv);
+
 	drm_atomic_helper_commit_hw_done(state);
 
 	if (intel_state->modeset)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9539f0e..76e78b8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1721,6 +1721,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 */);
+int skl_enable_sagv(struct drm_i915_private *dev_priv);
+int skl_disable_sagv(struct drm_i915_private *dev_priv);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8752730..0a202264 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2883,6 +2883,95 @@ skl_wm_plane_id(const struct intel_plane *plane)
 	}
 }
 
+/*
+ * SAGV dynamically adjusts the system agent voltage and clock frequencies
+ * depending on power and performance requirements. The display engine access
+ * to system memory is blocked during the adjustment time. Having this enabled
+ * in multi-pipe configurations can cause issues (such as underruns causing
+ * full system hangs), and the bspec also suggests that software disable it
+ * when more then one pipe is enabled.
+ */
+int
+skl_enable_sagv(struct drm_i915_private *dev_priv)
+{
+	int ret;
+
+	if (dev_priv->skl_sagv_status &&
+	    dev_priv->skl_sagv_status != I915_SKL_SAGV_DISABLED)
+		return 0;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+	DRM_DEBUG_KMS("Enabling the SAGV\n");
+
+	ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+				      GEN9_SAGV_DYNAMIC_FREQ);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	if (!ret) {
+		dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED;
+	} else if (ret == -EINVAL) {
+		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
+		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
+		ret = 0;
+	} else {
+		DRM_ERROR("Failed to enable the SAGV\n");
+	}
+
+	/* We don't need to wait for SAGV when enabling */
+	return ret;
+}
+
+static int
+skl_do_sagv_disable(struct drm_i915_private *dev_priv)
+{
+	int ret;
+	uint32_t temp = GEN9_SAGV_DISABLE;
+
+	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+				     &temp);
+	if (ret) {
+		/*
+		 * Some very early Skylake systems don't actually let you
+		 * control the SAGV, which is normal.
+		 */
+		if (ret != -EINVAL)
+			DRM_ERROR("Failed to disable the SAGV\n");
+
+		return ret;
+	}
+
+	return temp & GEN9_SAGV_IS_DISABLED;
+}
+
+int
+skl_disable_sagv(struct drm_i915_private *dev_priv)
+{
+	int ret, result;
+
+	if (dev_priv->skl_sagv_status &&
+	    dev_priv->skl_sagv_status != I915_SKL_SAGV_ENABLED)
+		return 0;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+	DRM_DEBUG_KMS("Disabling the SAGV\n");
+
+	/* bspec says to keep retrying for at least 1 ms */
+	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	if (!ret) {
+		dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED;
+	} else if (ret == -ETIMEDOUT) {
+		DRM_ERROR("Request to disable SAGV timed out\n");
+	} else if (result == -EINVAL) {
+		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
+		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				   const struct intel_crtc_state *cstate,
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v11 3/7] 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

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>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Cc: stable@vger.kernel.org
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 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 0a202264..77166c6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4045,6 +4045,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)
 {
@@ -4117,8 +4135,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;
@@ -4130,8 +4150,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 v11 3/7] 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

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>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Cc: stable@vger.kernel.org
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 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 0a202264..77166c6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4045,6 +4045,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)
 {
@@ -4117,8 +4135,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;
@@ -4130,8 +4150,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

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

[PATCH v11 4/7] 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
  }

So this patch moves all of the watermark writes into the right place;
inside of the vblank evasion where we update all of the registers for
each plane. While this patch doesn't fix everything, it does allow us to
update the watermark values in the way the hardware expects us to.

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:

Changes since v1:
 - Use IS_GEN9() instead of IS_SKYLAKE() since these fixes apply to more
   then just Skylake
 - Update description to make it clear this patch doesn't fix everything
 - Check if pipes were actually changed before writing watermarks

Changes since v2:
 - Write PIPE_WM_LINETIME during vblank evasion

Changes since v3:
 - Rebase against new SAGV patch changes

Changes since v4:
 - Add a parameter to choose what skl_wm_values struct to use when
   writing new plane watermarks

Changes since v5:
 - Remove cursor ddb entry write in skl_write_cursor_wm(), defer until
   patch 6
 - Write WM_LINETIME in intel_begin_crtc_commit()

Changes since v6:
 - Remove redundant dirty_pipes check in skl_write_plane_wm (we check
   this in all places where we call this function, and it was supposed
   to have been removed earlier anyway)
 - In i9xx_update_cursor(), use dev_priv->info.gen >= 9 instead of
   IS_GEN9(dev_priv). We do this everywhere else and I'd imagine this
   needs to be done for gen10 as well

Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
Signed-off-by: Lyude <cpaul@redhat.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
 drivers/gpu/drm/i915/intel_pm.c      | 50 ++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_sprite.c  |  7 +++++
 4 files changed, 62 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 35bdd67..b2f8e24 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3386,6 +3386,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_framebuffer *fb = plane_state->base.fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
 	int pipe = intel_crtc->pipe;
 	u32 plane_ctl;
 	unsigned int rotation = plane_state->base.rotation;
@@ -3419,6 +3421,9 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = src_x;
 	intel_crtc->adjusted_y = src_y;
 
+	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
+	    skl_write_plane_wm(intel_crtc, wm, 0);
+
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
 	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
@@ -10699,9 +10704,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
+	if (dev_priv->info.gen >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
+		skl_write_cursor_wm(intel_crtc, wm);
+
 	if (plane_state && plane_state->base.visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (plane_state->base.crtc_w) {
@@ -14610,10 +14619,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 				    struct drm_crtc_state *old_crtc_state)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_crtc_state *old_intel_state =
 		to_intel_crtc_state(old_crtc_state);
 	bool modeset = needs_modeset(crtc->state);
+	enum pipe pipe = intel_crtc->pipe;
 
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(intel_crtc);
@@ -14628,8 +14639,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 
 	if (to_intel_crtc_state(crtc->state)->update_pipe)
 		intel_update_pipe_config(intel_crtc, old_intel_state);
-	else if (INTEL_INFO(dev)->gen >= 9)
+	else if (INTEL_INFO(dev)->gen >= 9) {
 		skl_detach_scalers(intel_crtc);
+
+		I915_WRITE(PIPE_WM_LINETIME(pipe),
+			   dev_priv->wm.skl_hw.wm_linetime[pipe]);
+	}
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 76e78b8..88088c3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1723,6 +1723,11 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */);
 int skl_enable_sagv(struct drm_i915_private *dev_priv);
 int skl_disable_sagv(struct drm_i915_private *dev_priv);
+void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
+			 const struct skl_wm_values *wm);
+void skl_write_plane_wm(struct intel_crtc *intel_crtc,
+			const struct skl_wm_values *wm,
+			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 77166c6..e8653d8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3779,6 +3779,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,
+			const struct skl_wm_values *wm,
+			int plane)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	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,
+			 const struct skl_wm_values *wm)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	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)
 {
@@ -3786,7 +3819,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)
@@ -3794,21 +3827,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 		if (!crtc->active)
 			continue;
 
-		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 366900d..3c560f9 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -203,6 +203,10 @@ skl_update_plane(struct drm_plane *drm_plane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
 	struct drm_framebuffer *fb = plane_state->base.fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 	u32 plane_ctl;
@@ -228,6 +232,9 @@ skl_update_plane(struct drm_plane *drm_plane,
 
 	plane_ctl |= skl_plane_ctl_rotation(rotation);
 
+	if (wm->dirty_pipes & drm_crtc_mask(crtc))
+	    skl_write_plane_wm(intel_crtc, wm, 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 v11 4/7] 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
  }

So this patch moves all of the watermark writes into the right place;
inside of the vblank evasion where we update all of the registers for
each plane. While this patch doesn't fix everything, it does allow us to
update the watermark values in the way the hardware expects us to.

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:

Changes since v1:
 - Use IS_GEN9() instead of IS_SKYLAKE() since these fixes apply to more
   then just Skylake
 - Update description to make it clear this patch doesn't fix everything
 - Check if pipes were actually changed before writing watermarks

Changes since v2:
 - Write PIPE_WM_LINETIME during vblank evasion

Changes since v3:
 - Rebase against new SAGV patch changes

Changes since v4:
 - Add a parameter to choose what skl_wm_values struct to use when
   writing new plane watermarks

Changes since v5:
 - Remove cursor ddb entry write in skl_write_cursor_wm(), defer until
   patch 6
 - Write WM_LINETIME in intel_begin_crtc_commit()

Changes since v6:
 - Remove redundant dirty_pipes check in skl_write_plane_wm (we check
   this in all places where we call this function, and it was supposed
   to have been removed earlier anyway)
 - In i9xx_update_cursor(), use dev_priv->info.gen >= 9 instead of
   IS_GEN9(dev_priv). We do this everywhere else and I'd imagine this
   needs to be done for gen10 as well

Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
Signed-off-by: Lyude <cpaul@redhat.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
 drivers/gpu/drm/i915/intel_pm.c      | 50 ++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_sprite.c  |  7 +++++
 4 files changed, 62 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 35bdd67..b2f8e24 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3386,6 +3386,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
 	struct drm_framebuffer *fb = plane_state->base.fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
 	int pipe = intel_crtc->pipe;
 	u32 plane_ctl;
 	unsigned int rotation = plane_state->base.rotation;
@@ -3419,6 +3421,9 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = src_x;
 	intel_crtc->adjusted_y = src_y;
 
+	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
+	    skl_write_plane_wm(intel_crtc, wm, 0);
+
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
 	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
@@ -10699,9 +10704,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
+	if (dev_priv->info.gen >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc))
+		skl_write_cursor_wm(intel_crtc, wm);
+
 	if (plane_state && plane_state->base.visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (plane_state->base.crtc_w) {
@@ -14610,10 +14619,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 				    struct drm_crtc_state *old_crtc_state)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_crtc_state *old_intel_state =
 		to_intel_crtc_state(old_crtc_state);
 	bool modeset = needs_modeset(crtc->state);
+	enum pipe pipe = intel_crtc->pipe;
 
 	/* Perform vblank evasion around commit operation */
 	intel_pipe_update_start(intel_crtc);
@@ -14628,8 +14639,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
 
 	if (to_intel_crtc_state(crtc->state)->update_pipe)
 		intel_update_pipe_config(intel_crtc, old_intel_state);
-	else if (INTEL_INFO(dev)->gen >= 9)
+	else if (INTEL_INFO(dev)->gen >= 9) {
 		skl_detach_scalers(intel_crtc);
+
+		I915_WRITE(PIPE_WM_LINETIME(pipe),
+			   dev_priv->wm.skl_hw.wm_linetime[pipe]);
+	}
 }
 
 static void intel_finish_crtc_commit(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 76e78b8..88088c3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1723,6 +1723,11 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */);
 int skl_enable_sagv(struct drm_i915_private *dev_priv);
 int skl_disable_sagv(struct drm_i915_private *dev_priv);
+void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
+			 const struct skl_wm_values *wm);
+void skl_write_plane_wm(struct intel_crtc *intel_crtc,
+			const struct skl_wm_values *wm,
+			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 77166c6..e8653d8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3779,6 +3779,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,
+			const struct skl_wm_values *wm,
+			int plane)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	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,
+			 const struct skl_wm_values *wm)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	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)
 {
@@ -3786,7 +3819,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)
@@ -3794,21 +3827,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 		if (!crtc->active)
 			continue;
 
-		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 366900d..3c560f9 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -203,6 +203,10 @@ skl_update_plane(struct drm_plane *drm_plane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
 	struct drm_framebuffer *fb = plane_state->base.fb;
+	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	struct drm_crtc *crtc = crtc_state->base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	const int pipe = intel_plane->pipe;
 	const int plane = intel_plane->plane + 1;
 	u32 plane_ctl;
@@ -228,6 +232,9 @@ skl_update_plane(struct drm_plane *drm_plane,
 
 	plane_ctl |= skl_plane_ctl_rotation(rotation);
 
+	if (wm->dirty_pipes & drm_crtc_mask(crtc))
+	    skl_write_plane_wm(intel_crtc, wm, 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lyude]

If we're enabling a pipe, we'll need to modify the watermarks on all
active planes. Since those planes won't be added to the state on
their own, we need to add them ourselves.

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

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

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

[Lyude]

If we're enabling a pipe, we'll need to modify the watermarks on all
active planes. Since those planes won't be added to the state on
their own, we need to add them ourselves.

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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Lyude]

Since we have to write ddb allocations at the same time as we do other
plane updates, we're going to need to be able to control the order in
which we execute modesets on each pipe. The easiest way to do this is to
just factor this section of intel_atomic_commit_tail()
(intel_atomic_commit() for stable branches) into it's own function, and
add an appropriate display function hook for it.

Based off of Matt Rope's suggestions

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

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

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

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

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

[Lyude]

Since we have to write ddb allocations at the same time as we do other
plane updates, we're going to need to be able to control the order in
which we execute modesets on each pipe. The easiest way to do this is to
just factor this section of intel_atomic_commit_tail()
(intel_atomic_commit() for stable branches) into it's own function, and
add an appropriate display function hook for it.

Based off of Matt Rope's suggestions

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

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

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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v11 7/7] drm/i915/skl: Update DDB values atomically with wms/plane attrs

[Lyude]

Now that we can hook into update_crtcs and control the order in which we
update CRTCs at each modeset, we can finish the final step of fixing
Skylake's watermark handling by performing DDB updates at the same time
as plane updates and watermark updates.

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

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

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

|   A   |   B   |xxxxxxx|

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

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

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

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

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

Changes since v1:
 - Add skl_ddb_entry_write() for cursor into skl_write_cursor_wm()
Changes since v2:
 - Use the method for updating CRTCs that Ville suggested
 - In skl_update_wm(), only copy the watermarks for the crtc that was
   passed to us
Changes since v3:
 - Small comment fix in skl_ddb_allocation_overlaps()

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

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 61a45f1..68fdbf0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13348,16 +13348,23 @@ static void verify_wm_state(struct drm_crtc *crtc,
 			  hw_entry->start, hw_entry->end);
 	}
 
-	/* cursor */
-	hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
-	sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
-
-	if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
-		DRM_ERROR("mismatch in DDB state pipe %c cursor "
-			  "(expected (%u,%u), found (%u,%u))\n",
-			  pipe_name(pipe),
-			  sw_entry->start, sw_entry->end,
-			  hw_entry->start, hw_entry->end);
+	/*
+	 * cursor
+	 * If the cursor plane isn't active, we may not have updated it's ddb
+	 * allocation. In that case since the ddb allocation will be updated
+	 * once the plane becomes visible, we can skip this check
+	 */
+	if (intel_crtc->cursor_addr) {
+		hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
+		sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
+
+		if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
+			DRM_ERROR("mismatch in DDB state pipe %c cursor "
+				  "(expected (%u,%u), found (%u,%u))\n",
+				  pipe_name(pipe),
+				  sw_entry->start, sw_entry->end,
+				  hw_entry->start, hw_entry->end);
+		}
 	}
 }
 
@@ -14109,6 +14116,72 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
 	}
 }
 
+static void skl_update_crtcs(struct drm_atomic_state *state,
+			     unsigned int *crtc_vblank_mask)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state;
+	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
+	struct skl_ddb_allocation cur_ddb;
+	bool progress;
+	bool reallocated[I915_MAX_PIPES] = {};
+	enum pipe pipe;
+	int wait_vbl_pipes, i;
+
+	/*
+	 * Whenever the number of active pipes change, so does the DDB
+	 * allocation. DDB allocations on pipes cannot ever overlap with
+	 * eachother at any point in time, so we need to change the order we
+	 * update the pipes so that we ensure they never overlap inbetween DDB
+	 * updates.
+	 */
+	do {
+		progress = false;
+		wait_vbl_pipes = 0;
+		cur_ddb = dev_priv->wm.skl_hw.ddb;
+
+		for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
+			struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+			pipe = intel_crtc->pipe;
+
+			if (!intel_crtc->active || needs_modeset(crtc->state))
+				continue;
+			if (skl_ddb_allocation_equals(&cur_ddb, new_ddb, pipe))
+				continue;
+			if (skl_ddb_allocation_overlaps(state, &cur_ddb,
+							new_ddb, pipe))
+				continue;
+
+			intel_update_crtc(crtc, state, old_crtc_state,
+					  crtc_vblank_mask);
+
+			wait_vbl_pipes |= drm_crtc_mask(crtc);
+			reallocated[pipe] = true;
+			progress = true;
+		}
+
+		/* Wait for each pipe's new allocation to take effect */
+		intel_atomic_wait_for_vblanks(dev, dev_priv, wait_vbl_pipes);
+	} while (progress);
+
+	/*
+	 * Now that we've handled any ddb reallocations, we can go ahead and
+	 * enable any new pipes.
+	 */
+	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
+		pipe = to_intel_crtc(crtc)->pipe;
+
+		if (reallocated[pipe] || !crtc->state->active)
+			continue;
+
+		intel_update_crtc(crtc, state, old_crtc_state,
+				  crtc_vblank_mask);
+	}
+}
+
 static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
@@ -15738,8 +15811,6 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 	}
 
-	dev_priv->display.update_crtcs = intel_update_crtcs;
-
 	/* Returns the core display clock speed */
 	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
 		dev_priv->display.get_display_clock_speed =
@@ -15829,6 +15900,11 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 			skl_modeset_calc_cdclk;
 	}
 
+	if (dev_priv->info.gen >= 9)
+		dev_priv->display.update_crtcs = skl_update_crtcs;
+	else
+		dev_priv->display.update_crtcs = intel_update_crtcs;
+
 	switch (INTEL_INFO(dev_priv)->gen) {
 	case 2:
 		dev_priv->display.queue_flip = intel_gen2_queue_flip;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 88088c3..9f9fe69 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1723,6 +1723,13 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */);
 int skl_enable_sagv(struct drm_i915_private *dev_priv);
 int skl_disable_sagv(struct drm_i915_private *dev_priv);
+bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
+			       const struct skl_ddb_allocation *new,
+			       enum pipe pipe);
+bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
+				 const struct skl_ddb_allocation *old,
+				 const struct skl_ddb_allocation *new,
+				 enum pipe pipe);
 void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
 			 const struct skl_wm_values *wm);
 void skl_write_plane_wm(struct intel_crtc *intel_crtc,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f2e0071..6fe9e57 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3794,6 +3794,11 @@ void skl_write_plane_wm(struct intel_crtc *intel_crtc,
 			   wm->plane[pipe][plane][level]);
 	}
 	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
+
+	skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane),
+			    &wm->ddb.plane[pipe][plane]);
+	skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, plane),
+			    &wm->ddb.y_plane[pipe][plane]);
 }
 
 void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
@@ -3810,170 +3815,49 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
 			   wm->plane[pipe][PLANE_CURSOR][level]);
 	}
 	I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
-}
-
-static void skl_write_wm_values(struct drm_i915_private *dev_priv,
-				const struct skl_wm_values *new)
-{
-	struct drm_device *dev = &dev_priv->drm;
-	struct intel_crtc *crtc;
 
-	for_each_intel_crtc(dev, crtc) {
-		int i;
-		enum pipe pipe = crtc->pipe;
-
-		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
-			continue;
-		if (!crtc->active)
-			continue;
-
-		for (i = 0; i < intel_num_planes(crtc); i++) {
-			skl_ddb_entry_write(dev_priv,
-					    PLANE_BUF_CFG(pipe, i),
-					    &new->ddb.plane[pipe][i]);
-			skl_ddb_entry_write(dev_priv,
-					    PLANE_NV12_BUF_CFG(pipe, i),
-					    &new->ddb.y_plane[pipe][i]);
-		}
-
-		skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
-				    &new->ddb.plane[pipe][PLANE_CURSOR]);
-	}
+	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
+			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
 }
 
-/*
- * When setting up a new DDB allocation arrangement, we need to correctly
- * sequence the times at which the new allocations for the pipes are taken into
- * account or we'll have pipes fetching from space previously allocated to
- * another pipe.
- *
- * Roughly the sequence looks like:
- *  1. re-allocate the pipe(s) with the allocation being reduced and not
- *     overlapping with a previous light-up pipe (another way to put it is:
- *     pipes with their new allocation strickly included into their old ones).
- *  2. re-allocate the other pipes that get their allocation reduced
- *  3. allocate the pipes having their allocation increased
- *
- * Steps 1. and 2. are here to take care of the following case:
- * - Initially DDB looks like this:
- *     |   B    |   C    |
- * - enable pipe A.
- * - pipe B has a reduced DDB allocation that overlaps with the old pipe C
- *   allocation
- *     |  A  |  B  |  C  |
- *
- * We need to sequence the re-allocation: C, B, A (and not B, C, A).
- */
-
-static void
-skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, int pass)
-{
-	int plane;
-
-	DRM_DEBUG_KMS("flush pipe %c (pass %d)\n", pipe_name(pipe), pass);
-
-	for_each_plane(dev_priv, pipe, plane) {
-		I915_WRITE(PLANE_SURF(pipe, plane),
-			   I915_READ(PLANE_SURF(pipe, plane)));
-	}
-	I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
-}
-
-static bool
-skl_ddb_allocation_included(const struct skl_ddb_allocation *old,
-			    const struct skl_ddb_allocation *new,
-			    enum pipe pipe)
+bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
+			       const struct skl_ddb_allocation *new,
+			       enum pipe pipe)
 {
-	uint16_t old_size, new_size;
-
-	old_size = skl_ddb_entry_size(&old->pipe[pipe]);
-	new_size = skl_ddb_entry_size(&new->pipe[pipe]);
-
-	return old_size != new_size &&
-	       new->pipe[pipe].start >= old->pipe[pipe].start &&
-	       new->pipe[pipe].end <= old->pipe[pipe].end;
+	return new->pipe[pipe].start == old->pipe[pipe].start &&
+	       new->pipe[pipe].end == old->pipe[pipe].end;
 }
 
-static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
-				struct skl_wm_values *new_values)
+bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
+				 const struct skl_ddb_allocation *old,
+				 const struct skl_ddb_allocation *new,
+				 enum pipe pipe)
 {
-	struct drm_device *dev = &dev_priv->drm;
-	struct skl_ddb_allocation *cur_ddb, *new_ddb;
-	bool reallocated[I915_MAX_PIPES] = {};
-	struct intel_crtc *crtc;
-	enum pipe pipe;
-
-	new_ddb = &new_values->ddb;
-	cur_ddb = &dev_priv->wm.skl_hw.ddb;
-
-	/*
-	 * First pass: flush the pipes with the new allocation contained into
-	 * the old space.
-	 *
-	 * We'll wait for the vblank on those pipes to ensure we can safely
-	 * re-allocate the freed space without this pipe fetching from it.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		pipe = crtc->pipe;
-
-		if (!skl_ddb_allocation_included(cur_ddb, new_ddb, pipe))
-			continue;
-
-		skl_wm_flush_pipe(dev_priv, pipe, 1);
-		intel_wait_for_vblank(dev, pipe);
-
-		reallocated[pipe] = true;
-	}
-
-
-	/*
-	 * Second pass: flush the pipes that are having their allocation
-	 * reduced, but overlapping with a previous allocation.
-	 *
-	 * Here as well we need to wait for the vblank to make sure the freed
-	 * space is not used anymore.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		pipe = crtc->pipe;
-
-		if (reallocated[pipe])
-			continue;
-
-		if (skl_ddb_entry_size(&new_ddb->pipe[pipe]) <
-		    skl_ddb_entry_size(&cur_ddb->pipe[pipe])) {
-			skl_wm_flush_pipe(dev_priv, pipe, 2);
-			intel_wait_for_vblank(dev, pipe);
-			reallocated[pipe] = true;
-		}
-	}
-
-	/*
-	 * Third pass: flush the pipes that got more space allocated.
-	 *
-	 * We don't need to actively wait for the update here, next vblank
-	 * will just get more DDB space with the correct WM values.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
+	struct drm_device *dev = state->dev;
+	struct intel_crtc *intel_crtc;
+	enum pipe otherp;
 
-		pipe = crtc->pipe;
+	for_each_intel_crtc(dev, intel_crtc) {
+		otherp = intel_crtc->pipe;
 
 		/*
-		 * At this point, only the pipes more space than before are
-		 * left to re-allocate.
+		 * When checking for overlaps, we don't want to:
+		 *  - Compare against ourselves
+		 *  - Compare against pipes that will be/are disabled
+		 *  - Compare against pipes that aren't enabled yet
 		 */
-		if (reallocated[pipe])
+		if (otherp == pipe || !new->pipe[otherp].end ||
+		    !old->pipe[otherp].end)
 			continue;
 
-		skl_wm_flush_pipe(dev_priv, pipe, 3);
+		if ((new->pipe[pipe].start >= old->pipe[otherp].start &&
+		     new->pipe[pipe].start < old->pipe[otherp].end) ||
+		    (old->pipe[otherp].start >= new->pipe[pipe].start &&
+		     old->pipe[otherp].start < new->pipe[pipe].end))
+			return true;
 	}
+
+	return false;
 }
 
 static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
@@ -4160,7 +4044,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
-	int pipe;
+	enum pipe pipe = intel_crtc->pipe;
 
 	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
 		return;
@@ -4169,15 +4053,22 @@ static void skl_update_wm(struct drm_crtc *crtc)
 
 	mutex_lock(&dev_priv->wm.wm_mutex);
 
-	skl_write_wm_values(dev_priv, results);
-	skl_flush_wm_values(dev_priv, results);
-
 	/*
-	 * Store the new configuration (but only for the pipes that have
-	 * changed; the other values weren't recomputed).
+	 * If this pipe isn't active already, we're going to be enabling it
+	 * very soon. Since it's safe to update a pipe's ddb allocation while
+	 * the pipe's shut off, just do so here. Already active pipes will have
+	 * their watermarks updated once we update their planes.
 	 */
-	for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes)
-		skl_copy_wm_for_pipe(hw_vals, results, pipe);
+	if (crtc->state->active_changed) {
+		int plane;
+
+		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
+			skl_write_plane_wm(intel_crtc, results, plane);
+
+		skl_write_cursor_wm(intel_crtc, results);
+	}
+
+	skl_copy_wm_for_pipe(hw_vals, results, pipe);
 
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
-- 
2.7.4

[PATCH v11 7/7] drm/i915/skl: Update DDB values atomically with wms/plane attrs

[Lyude]

Now that we can hook into update_crtcs and control the order in which we
update CRTCs at each modeset, we can finish the final step of fixing
Skylake's watermark handling by performing DDB updates at the same time
as plane updates and watermark updates.

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

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

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

|   A   |   B   |xxxxxxx|

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

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

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

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

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

Changes since v1:
 - Add skl_ddb_entry_write() for cursor into skl_write_cursor_wm()
Changes since v2:
 - Use the method for updating CRTCs that Ville suggested
 - In skl_update_wm(), only copy the watermarks for the crtc that was
   passed to us
Changes since v3:
 - Small comment fix in skl_ddb_allocation_overlaps()

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

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 61a45f1..68fdbf0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13348,16 +13348,23 @@ static void verify_wm_state(struct drm_crtc *crtc,
 			  hw_entry->start, hw_entry->end);
 	}
 
-	/* cursor */
-	hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
-	sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
-
-	if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
-		DRM_ERROR("mismatch in DDB state pipe %c cursor "
-			  "(expected (%u,%u), found (%u,%u))\n",
-			  pipe_name(pipe),
-			  sw_entry->start, sw_entry->end,
-			  hw_entry->start, hw_entry->end);
+	/*
+	 * cursor
+	 * If the cursor plane isn't active, we may not have updated it's ddb
+	 * allocation. In that case since the ddb allocation will be updated
+	 * once the plane becomes visible, we can skip this check
+	 */
+	if (intel_crtc->cursor_addr) {
+		hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
+		sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
+
+		if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
+			DRM_ERROR("mismatch in DDB state pipe %c cursor "
+				  "(expected (%u,%u), found (%u,%u))\n",
+				  pipe_name(pipe),
+				  sw_entry->start, sw_entry->end,
+				  hw_entry->start, hw_entry->end);
+		}
 	}
 }
 
@@ -14109,6 +14116,72 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
 	}
 }
 
+static void skl_update_crtcs(struct drm_atomic_state *state,
+			     unsigned int *crtc_vblank_mask)
+{
+	struct drm_device *dev = state->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state;
+	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
+	struct skl_ddb_allocation cur_ddb;
+	bool progress;
+	bool reallocated[I915_MAX_PIPES] = {};
+	enum pipe pipe;
+	int wait_vbl_pipes, i;
+
+	/*
+	 * Whenever the number of active pipes change, so does the DDB
+	 * allocation. DDB allocations on pipes cannot ever overlap with
+	 * eachother at any point in time, so we need to change the order we
+	 * update the pipes so that we ensure they never overlap inbetween DDB
+	 * updates.
+	 */
+	do {
+		progress = false;
+		wait_vbl_pipes = 0;
+		cur_ddb = dev_priv->wm.skl_hw.ddb;
+
+		for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
+			struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+			pipe = intel_crtc->pipe;
+
+			if (!intel_crtc->active || needs_modeset(crtc->state))
+				continue;
+			if (skl_ddb_allocation_equals(&cur_ddb, new_ddb, pipe))
+				continue;
+			if (skl_ddb_allocation_overlaps(state, &cur_ddb,
+							new_ddb, pipe))
+				continue;
+
+			intel_update_crtc(crtc, state, old_crtc_state,
+					  crtc_vblank_mask);
+
+			wait_vbl_pipes |= drm_crtc_mask(crtc);
+			reallocated[pipe] = true;
+			progress = true;
+		}
+
+		/* Wait for each pipe's new allocation to take effect */
+		intel_atomic_wait_for_vblanks(dev, dev_priv, wait_vbl_pipes);
+	} while (progress);
+
+	/*
+	 * Now that we've handled any ddb reallocations, we can go ahead and
+	 * enable any new pipes.
+	 */
+	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
+		pipe = to_intel_crtc(crtc)->pipe;
+
+		if (reallocated[pipe] || !crtc->state->active)
+			continue;
+
+		intel_update_crtc(crtc, state, old_crtc_state,
+				  crtc_vblank_mask);
+	}
+}
+
 static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
@@ -15738,8 +15811,6 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 	}
 
-	dev_priv->display.update_crtcs = intel_update_crtcs;
-
 	/* Returns the core display clock speed */
 	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
 		dev_priv->display.get_display_clock_speed =
@@ -15829,6 +15900,11 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
 			skl_modeset_calc_cdclk;
 	}
 
+	if (dev_priv->info.gen >= 9)
+		dev_priv->display.update_crtcs = skl_update_crtcs;
+	else
+		dev_priv->display.update_crtcs = intel_update_crtcs;
+
 	switch (INTEL_INFO(dev_priv)->gen) {
 	case 2:
 		dev_priv->display.queue_flip = intel_gen2_queue_flip;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 88088c3..9f9fe69 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1723,6 +1723,13 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */);
 int skl_enable_sagv(struct drm_i915_private *dev_priv);
 int skl_disable_sagv(struct drm_i915_private *dev_priv);
+bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
+			       const struct skl_ddb_allocation *new,
+			       enum pipe pipe);
+bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
+				 const struct skl_ddb_allocation *old,
+				 const struct skl_ddb_allocation *new,
+				 enum pipe pipe);
 void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
 			 const struct skl_wm_values *wm);
 void skl_write_plane_wm(struct intel_crtc *intel_crtc,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f2e0071..6fe9e57 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3794,6 +3794,11 @@ void skl_write_plane_wm(struct intel_crtc *intel_crtc,
 			   wm->plane[pipe][plane][level]);
 	}
 	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
+
+	skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane),
+			    &wm->ddb.plane[pipe][plane]);
+	skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, plane),
+			    &wm->ddb.y_plane[pipe][plane]);
 }
 
 void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
@@ -3810,170 +3815,49 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
 			   wm->plane[pipe][PLANE_CURSOR][level]);
 	}
 	I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
-}
-
-static void skl_write_wm_values(struct drm_i915_private *dev_priv,
-				const struct skl_wm_values *new)
-{
-	struct drm_device *dev = &dev_priv->drm;
-	struct intel_crtc *crtc;
 
-	for_each_intel_crtc(dev, crtc) {
-		int i;
-		enum pipe pipe = crtc->pipe;
-
-		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
-			continue;
-		if (!crtc->active)
-			continue;
-
-		for (i = 0; i < intel_num_planes(crtc); i++) {
-			skl_ddb_entry_write(dev_priv,
-					    PLANE_BUF_CFG(pipe, i),
-					    &new->ddb.plane[pipe][i]);
-			skl_ddb_entry_write(dev_priv,
-					    PLANE_NV12_BUF_CFG(pipe, i),
-					    &new->ddb.y_plane[pipe][i]);
-		}
-
-		skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
-				    &new->ddb.plane[pipe][PLANE_CURSOR]);
-	}
+	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
+			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
 }
 
-/*
- * When setting up a new DDB allocation arrangement, we need to correctly
- * sequence the times at which the new allocations for the pipes are taken into
- * account or we'll have pipes fetching from space previously allocated to
- * another pipe.
- *
- * Roughly the sequence looks like:
- *  1. re-allocate the pipe(s) with the allocation being reduced and not
- *     overlapping with a previous light-up pipe (another way to put it is:
- *     pipes with their new allocation strickly included into their old ones).
- *  2. re-allocate the other pipes that get their allocation reduced
- *  3. allocate the pipes having their allocation increased
- *
- * Steps 1. and 2. are here to take care of the following case:
- * - Initially DDB looks like this:
- *     |   B    |   C    |
- * - enable pipe A.
- * - pipe B has a reduced DDB allocation that overlaps with the old pipe C
- *   allocation
- *     |  A  |  B  |  C  |
- *
- * We need to sequence the re-allocation: C, B, A (and not B, C, A).
- */
-
-static void
-skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, int pass)
-{
-	int plane;
-
-	DRM_DEBUG_KMS("flush pipe %c (pass %d)\n", pipe_name(pipe), pass);
-
-	for_each_plane(dev_priv, pipe, plane) {
-		I915_WRITE(PLANE_SURF(pipe, plane),
-			   I915_READ(PLANE_SURF(pipe, plane)));
-	}
-	I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
-}
-
-static bool
-skl_ddb_allocation_included(const struct skl_ddb_allocation *old,
-			    const struct skl_ddb_allocation *new,
-			    enum pipe pipe)
+bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
+			       const struct skl_ddb_allocation *new,
+			       enum pipe pipe)
 {
-	uint16_t old_size, new_size;
-
-	old_size = skl_ddb_entry_size(&old->pipe[pipe]);
-	new_size = skl_ddb_entry_size(&new->pipe[pipe]);
-
-	return old_size != new_size &&
-	       new->pipe[pipe].start >= old->pipe[pipe].start &&
-	       new->pipe[pipe].end <= old->pipe[pipe].end;
+	return new->pipe[pipe].start == old->pipe[pipe].start &&
+	       new->pipe[pipe].end == old->pipe[pipe].end;
 }
 
-static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
-				struct skl_wm_values *new_values)
+bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
+				 const struct skl_ddb_allocation *old,
+				 const struct skl_ddb_allocation *new,
+				 enum pipe pipe)
 {
-	struct drm_device *dev = &dev_priv->drm;
-	struct skl_ddb_allocation *cur_ddb, *new_ddb;
-	bool reallocated[I915_MAX_PIPES] = {};
-	struct intel_crtc *crtc;
-	enum pipe pipe;
-
-	new_ddb = &new_values->ddb;
-	cur_ddb = &dev_priv->wm.skl_hw.ddb;
-
-	/*
-	 * First pass: flush the pipes with the new allocation contained into
-	 * the old space.
-	 *
-	 * We'll wait for the vblank on those pipes to ensure we can safely
-	 * re-allocate the freed space without this pipe fetching from it.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		pipe = crtc->pipe;
-
-		if (!skl_ddb_allocation_included(cur_ddb, new_ddb, pipe))
-			continue;
-
-		skl_wm_flush_pipe(dev_priv, pipe, 1);
-		intel_wait_for_vblank(dev, pipe);
-
-		reallocated[pipe] = true;
-	}
-
-
-	/*
-	 * Second pass: flush the pipes that are having their allocation
-	 * reduced, but overlapping with a previous allocation.
-	 *
-	 * Here as well we need to wait for the vblank to make sure the freed
-	 * space is not used anymore.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
-
-		pipe = crtc->pipe;
-
-		if (reallocated[pipe])
-			continue;
-
-		if (skl_ddb_entry_size(&new_ddb->pipe[pipe]) <
-		    skl_ddb_entry_size(&cur_ddb->pipe[pipe])) {
-			skl_wm_flush_pipe(dev_priv, pipe, 2);
-			intel_wait_for_vblank(dev, pipe);
-			reallocated[pipe] = true;
-		}
-	}
-
-	/*
-	 * Third pass: flush the pipes that got more space allocated.
-	 *
-	 * We don't need to actively wait for the update here, next vblank
-	 * will just get more DDB space with the correct WM values.
-	 */
-	for_each_intel_crtc(dev, crtc) {
-		if (!crtc->active)
-			continue;
+	struct drm_device *dev = state->dev;
+	struct intel_crtc *intel_crtc;
+	enum pipe otherp;
 
-		pipe = crtc->pipe;
+	for_each_intel_crtc(dev, intel_crtc) {
+		otherp = intel_crtc->pipe;
 
 		/*
-		 * At this point, only the pipes more space than before are
-		 * left to re-allocate.
+		 * When checking for overlaps, we don't want to:
+		 *  - Compare against ourselves
+		 *  - Compare against pipes that will be/are disabled
+		 *  - Compare against pipes that aren't enabled yet
 		 */
-		if (reallocated[pipe])
+		if (otherp == pipe || !new->pipe[otherp].end ||
+		    !old->pipe[otherp].end)
 			continue;
 
-		skl_wm_flush_pipe(dev_priv, pipe, 3);
+		if ((new->pipe[pipe].start >= old->pipe[otherp].start &&
+		     new->pipe[pipe].start < old->pipe[otherp].end) ||
+		    (old->pipe[otherp].start >= new->pipe[pipe].start &&
+		     old->pipe[otherp].start < new->pipe[pipe].end))
+			return true;
 	}
+
+	return false;
 }
 
 static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
@@ -4160,7 +4044,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
 	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
 	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
 	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
-	int pipe;
+	enum pipe pipe = intel_crtc->pipe;
 
 	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
 		return;
@@ -4169,15 +4053,22 @@ static void skl_update_wm(struct drm_crtc *crtc)
 
 	mutex_lock(&dev_priv->wm.wm_mutex);
 
-	skl_write_wm_values(dev_priv, results);
-	skl_flush_wm_values(dev_priv, results);
-
 	/*
-	 * Store the new configuration (but only for the pipes that have
-	 * changed; the other values weren't recomputed).
+	 * If this pipe isn't active already, we're going to be enabling it
+	 * very soon. Since it's safe to update a pipe's ddb allocation while
+	 * the pipe's shut off, just do so here. Already active pipes will have
+	 * their watermarks updated once we update their planes.
 	 */
-	for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes)
-		skl_copy_wm_for_pipe(hw_vals, results, pipe);
+	if (crtc->state->active_changed) {
+		int plane;
+
+		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
+			skl_write_plane_wm(intel_crtc, results, plane);
+
+		skl_write_cursor_wm(intel_crtc, results);
+	}
+
+	skl_copy_wm_for_pipe(hw_vals, results, pipe);
 
 	mutex_unlock(&dev_priv->wm.wm_mutex);
 }
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Ro.CI.BAT: failure for Finally fix watermarks (rev9)

[Patchwork]

== Series Details ==

Series: Finally fix watermarks (rev9)
URL   : https://patchwork.freedesktop.org/series/10276/
State : failure

== Summary ==

Series 10276v9 Finally fix watermarks
http://patchwork.freedesktop.org/api/1.0/series/10276/revisions/9/mbox

Test gem_exec_gttfill:
        Subgroup basic:
                pass       -> SKIP       (fi-snb-i7-2600)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (fi-hsw-i7-4770k)
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)
                dmesg-warn -> PASS       (fi-skl-i5-6260u)
                dmesg-warn -> PASS       (fi-skl-i7-6700k)
Test kms_cursor_legacy:
        Subgroup basic-cursor-vs-flip-varying-size:
                pass       -> FAIL       (ro-ilk1-i5-650)
        Subgroup basic-flip-vs-cursor-legacy:
                pass       -> FAIL       (fi-hsw-i7-4770k)
                dmesg-fail -> FAIL       (fi-skl-i7-6700k)
        Subgroup basic-flip-vs-cursor-varying-size:
                dmesg-fail -> PASS       (fi-skl-i7-6700k)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b-frame-sequence:
                fail       -> PASS       (ro-ivb2-i7-3770)
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-hsw-i7-4770k)
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
                dmesg-warn -> PASS       (fi-skl-i5-6260u)
                dmesg-warn -> PASS       (fi-skl-i7-6700k)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-hsw-i7-4770k)
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
                dmesg-warn -> PASS       (fi-skl-i5-6260u)
                dmesg-warn -> PASS       (fi-skl-i7-6700k)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (fi-skl-i5-6260u)
                dmesg-warn -> PASS       (fi-skl-i7-6700k)

fi-hsw-i7-4770k  total:207  pass:183  dwarn:2   dfail:0   fail:1   skip:20 
fi-kbl-qkkr      total:244  pass:186  dwarn:29  dfail:0   fail:2   skip:27 
fi-skl-i5-6260u  total:244  pass:228  dwarn:0   dfail:0   fail:2   skip:14 
fi-skl-i7-6700k  total:244  pass:213  dwarn:0   dfail:0   fail:3   skip:28 
fi-snb-i7-2600   total:244  pass:201  dwarn:0   dfail:0   fail:0   skip:43 
ro-bdw-i5-5250u  total:240  pass:218  dwarn:2   dfail:0   fail:2   skip:18 
ro-bdw-i7-5600u  total:240  pass:207  dwarn:0   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:194  dwarn:0   dfail:0   fail:4   skip:42 
ro-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:185  dwarn:0   dfail:0   fail:0   skip:55 
ro-ilk1-i5-650   total:235  pass:173  dwarn:0   dfail:0   fail:2   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:0   fail:4   skip:14 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1842/

4a26251 drm-intel-nightly: 2016y-08m-11d-16h-12m-42s UTC integration manifest
03a294b drm/i915/skl: Update DDB values atomically with wms/plane attrs
f269f34 drm/i915: Move CRTC updating in atomic_commit into it's own hook
aa1c495 drm/i915/skl: Ensure pipes with changed wms get added to the state
486bb63 drm/i915/skl: Update plane watermarks atomically during plane updates
bdb3379 drm/i915/gen9: Only copy WM results for changed pipes to skl_hw
2acd342 drm/i915/skl: Add support for the SAGV, fix underrun hangs
a324893 drm/i915/gen6+: Return -EINVAL on invalid pcode commands

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

Re: [PATCH v11 1/7] drm/i915/gen6+: Return -EINVAL on invalid pcode commands

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 03:54:30PM -0400, Lyude wrote:
> In order to add proper support for the SAGV, we need to be able to know
> what the cause of a failure to change the SAGV through the pcode mailbox
> was. The reasoning for this is that some very early pre-release Skylake
> machines don't actually allow you to control the SAGV on them, and
> indicate an invalid mailbox command was sent.
> 
> This also might come in handy in the future for debugging.
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index da82744..73b3d4d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7121,6 +7121,7 @@ enum {
>  #define VLV_MEDIA_C0_COUNT			_MMIO(0x13811C)
>  
>  #define GEN6_PCODE_MAILBOX			_MMIO(0x138124)
> +#define   GEN6_PCODE_INVALID_CMD		(1<<0)

It doesn't work quite like that. Instead the low 8 bits will contian
the error code, iff pcode has cleared the "READY" bit:

snb-ivb docs say:
00h MAILBOX_GTDRIVER_CC_SUCCESS
01h MAILBOX_GTDRIVER_CC_ILLEGAL_CMD
02h MAILBOX_GTDRIVER_CC_MIN_FREQUENCY_TABLE_GT_RATIO_OUT_OF_RANGE
03h MAILBOX_GTDRIVER_CC_TIMEOUT
FFh MAILBOX_GTDRIVER_CC_UNIMPLEMENTED_CMD

hsw-bdw docs say:
00h SUCCESS
01h ILLEGAL_CMD
02h TIMEOUT
03h ILLEGAL_DATA
10h MIN_FREQUENCY_TABLE_GT_RATIO_OUT_OF_RANGE

skl docs say:
00h SUCCESS
01h ILLEGAL_CMD
02h TIMEOUT
03h ILLEGAL_DATA

So looks like 02h and 03h will need to be interpreted in two different
ways depending on the platform. Assuming we want to decode the error
into a a human readable form for dmesg.

>  #define   GEN6_PCODE_READY			(1<<31)
>  #define	  GEN6_PCODE_WRITE_RC6VIDS		0x4
>  #define	  GEN6_PCODE_READ_RC6VIDS		0x5
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 99014d7..8752730 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7674,6 +7674,11 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
>  	*val = I915_READ_FW(GEN6_PCODE_DATA);
>  	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
>  
> +	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_INVALID_CMD) {
> +		DRM_DEBUG_DRIVER("warning: pcode (read) mailbox access indicated invalid command\n");
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -7704,6 +7709,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
>  
>  	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
>  
> +	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_INVALID_CMD) {
> +		DRM_DEBUG_DRIVER("warning: pcode (write) mailbox access indicated invalid command\n");
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v11 1/7] drm/i915/gen6+: Return -EINVAL on invalid pcode commands

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 03:54:30PM -0400, Lyude wrote:
> In order to add proper support for the SAGV, we need to be able to know
> what the cause of a failure to change the SAGV through the pcode mailbox
> was. The reasoning for this is that some very early pre-release Skylake
> machines don't actually allow you to control the SAGV on them, and
> indicate an invalid mailbox command was sent.
> 
> This also might come in handy in the future for debugging.
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index da82744..73b3d4d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7121,6 +7121,7 @@ enum {
>  #define VLV_MEDIA_C0_COUNT			_MMIO(0x13811C)
>  
>  #define GEN6_PCODE_MAILBOX			_MMIO(0x138124)
> +#define   GEN6_PCODE_INVALID_CMD		(1<<0)

It doesn't work quite like that. Instead the low 8 bits will contian
the error code, iff pcode has cleared the "READY" bit:

snb-ivb docs say:
00h MAILBOX_GTDRIVER_CC_SUCCESS
01h MAILBOX_GTDRIVER_CC_ILLEGAL_CMD
02h MAILBOX_GTDRIVER_CC_MIN_FREQUENCY_TABLE_GT_RATIO_OUT_OF_RANGE
03h MAILBOX_GTDRIVER_CC_TIMEOUT
FFh MAILBOX_GTDRIVER_CC_UNIMPLEMENTED_CMD

hsw-bdw docs say:
00h SUCCESS
01h ILLEGAL_CMD
02h TIMEOUT
03h ILLEGAL_DATA
10h MIN_FREQUENCY_TABLE_GT_RATIO_OUT_OF_RANGE

skl docs say:
00h SUCCESS
01h ILLEGAL_CMD
02h TIMEOUT
03h ILLEGAL_DATA

So looks like 02h and 03h will need to be interpreted in two different
ways depending on the platform. Assuming we want to decode the error
into a a human readable form for dmesg.

>  #define   GEN6_PCODE_READY			(1<<31)
>  #define	  GEN6_PCODE_WRITE_RC6VIDS		0x4
>  #define	  GEN6_PCODE_READ_RC6VIDS		0x5
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 99014d7..8752730 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7674,6 +7674,11 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
>  	*val = I915_READ_FW(GEN6_PCODE_DATA);
>  	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
>  
> +	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_INVALID_CMD) {
> +		DRM_DEBUG_DRIVER("warning: pcode (read) mailbox access indicated invalid command\n");
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -7704,6 +7709,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
>  
>  	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
>  
> +	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_INVALID_CMD) {
> +		DRM_DEBUG_DRIVER("warning: pcode (write) mailbox access indicated invalid command\n");
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.7.4

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH v11 1/7] drm/i915/gen6+: Return -EINVAL on invalid pcode commands

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 03:54:30PM -0400, Lyude wrote:
> In order to add proper support for the SAGV, we need to be able to know
> what the cause of a failure to change the SAGV through the pcode mailbox
> was. The reasoning for this is that some very early pre-release Skylake
> machines don't actually allow you to control the SAGV on them, and
> indicate an invalid mailbox command was sent.
> 
> This also might come in handy in the future for debugging.
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_reg.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 10 ++++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index da82744..73b3d4d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7121,6 +7121,7 @@ enum {
>  #define VLV_MEDIA_C0_COUNT			_MMIO(0x13811C)
>  
>  #define GEN6_PCODE_MAILBOX			_MMIO(0x138124)
> +#define   GEN6_PCODE_INVALID_CMD		(1<<0)

It doesn't work quite like that. Instead the low 8 bits will contian
the error code, iff pcode has cleared the "READY" bit:

snb-ivb docs say:
00h MAILBOX_GTDRIVER_CC_SUCCESS
01h MAILBOX_GTDRIVER_CC_ILLEGAL_CMD
02h MAILBOX_GTDRIVER_CC_MIN_FREQUENCY_TABLE_GT_RATIO_OUT_OF_RANGE
03h MAILBOX_GTDRIVER_CC_TIMEOUT
FFh MAILBOX_GTDRIVER_CC_UNIMPLEMENTED_CMD

hsw-bdw docs say:
00h SUCCESS
01h ILLEGAL_CMD
02h TIMEOUT
03h ILLEGAL_DATA
10h MIN_FREQUENCY_TABLE_GT_RATIO_OUT_OF_RANGE

skl docs say:
00h SUCCESS
01h ILLEGAL_CMD
02h TIMEOUT
03h ILLEGAL_DATA

So looks like 02h and 03h will need to be interpreted in two different
ways depending on the platform. Assuming we want to decode the error
into a a human readable form for dmesg.

>  #define   GEN6_PCODE_READY			(1<<31)
>  #define	  GEN6_PCODE_WRITE_RC6VIDS		0x4
>  #define	  GEN6_PCODE_READ_RC6VIDS		0x5
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 99014d7..8752730 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7674,6 +7674,11 @@ int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val
>  	*val = I915_READ_FW(GEN6_PCODE_DATA);
>  	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
>  
> +	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_INVALID_CMD) {
> +		DRM_DEBUG_DRIVER("warning: pcode (read) mailbox access indicated invalid command\n");
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -7704,6 +7709,11 @@ int sandybridge_pcode_write(struct drm_i915_private *dev_priv,
>  
>  	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
>  
> +	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_INVALID_CMD) {
> +		DRM_DEBUG_DRIVER("warning: pcode (write) mailbox access indicated invalid command\n");
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> -- 
> 2.7.4

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

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

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 03:54:31PM -0400, Lyude wrote:
> Since the watermark calculations for Skylake are still broken, we're apt
> to hitting underruns very easily under multi-monitor configurations.
> While it would be lovely if this was fixed, it's not. Another problem
> that's been coming from this however, is the mysterious issue of
> underruns causing full system hangs. An easy way to reproduce this with
> a skylake system:
> 
> - Get a laptop with a skylake GPU, and hook up two external monitors to
>   it
> - Move the cursor from the built-in LCD to one of the external displays
>   as quickly as you can
> - You'll get a few pipe underruns, and eventually the entire system will
>   just freeze.
> 
> After doing a lot of investigation and reading through the bspec, I
> found the existence of the SAGV, which is responsible for adjusting the
> system agent voltage and clock frequencies depending on how much power
> we need. According to the bspec:
> 
> "The display engine access to system memory is blocked during the
>  adjustment time. SAGV defaults to enabled. Software must use the
>  GT-driver pcode mailbox to disable SAGV when the display engine is not
>  able to tolerate the blocking time."
> 
> The rest of the bspec goes on to explain that software can simply leave
> the SAGV enabled, and disable it when we use interlaced pipes/have more
> then one pipe active.
> 
> Sure enough, with this patchset the system hangs resulting from pipe
> underruns on Skylake have completely vanished on my T460s. Additionally,
> the bspec mentions turning off the SAGV	with more then one pipe enabled
> as a workaround for display underruns. While this patch doesn't entirely
> fix that, it looks like it does improve the situation a little bit so
> it's likely this is going to be required to make watermarks on Skylake
> fully functional.
> 
> Changes since v10:
>  - Apparently sandybridge_pcode_read actually writes values and reads
>    them back, despite it's misleading function name. This means we've
>    been doing this mostly wrong and have been writing garbage to the
>    SAGV control. Because of this, we no longer attempt to read the SAGV
>    status during initialization (since there are no helpers for this).
>  - mlankhorst noticed that this patch was breaking on some very early
>    pre-release Skylake machines, which apparently don't allow you to
>    disable the SAGV. To prevent machines from failing tests due to SAGV
>    errors, if the first time we try to control the SAGV results in the
>    mailbox indicating an invalid command, we just disable future attempts
>    to control the SAGV state by setting dev_priv->skl_sagv_status to
>    I915_SKL_SAGV_NOT_CONTROLLED and make a note of it in dmesg.
>  - Move mutex_unlock() a little higher in skl_enable_sagv(). This
>    doesn't actually fix anything, but lets us release the lock a little
>    sooner since we're finished with it.
> Changes since v9:
>  - Only enable/disable sagv on Skylake
> Changes since v8:
>  - Add intel_state->modeset guard to the conditional for
>    skl_enable_sagv()
> Changes since v7:
>  - Remove GEN9_SAGV_LOW_FREQ, replace with GEN9_SAGV_IS_ENABLED (that's
>    all we use it for anyway)
>  - Use GEN9_SAGV_IS_ENABLED instead of 0x1 for clarification
>  - Fix a styling error that snuck past me
> Changes since v6:
>  - Protect skl_enable_sagv() with intel_state->modeset conditional in
>    intel_atomic_commit_tail()
> Changes since v5:
>  - Don't use is_power_of_2. Makes things confusing
>  - Don't use the old state to figure out whether or not to
>    enable/disable the sagv, use the new one
>  - Split the loop in skl_disable_sagv into it's own function
>  - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail()
> Changes since v4:
>  - Use is_power_of_2 against active_crtcs to check whether we have > 1
>    pipe enabled
>  - Fix skl_sagv_get_hw_state(): (temp & 0x1) indicates disabled, 0x0
>    enabled
>  - Call skl_sagv_enable/disable() from pre/post-plane updates
> Changes since v3:
>  - Use time_before() to compare timeout to jiffies
> Changes since v2:
>  - Really apply minor style nitpicks to patch this time
> Changes since v1:
>  - Added comments about this probably being one of the requirements to
>    fixing Skylake's watermark issues
>  - Minor style nitpicks from Matt Roper
>  - Disable these functions on Broxton, since it doesn't have an SAGV
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  7 +++
>  drivers/gpu/drm/i915/i915_reg.h      |  4 ++
>  drivers/gpu/drm/i915/intel_display.c | 12 +++++
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>  drivers/gpu/drm/i915/intel_pm.c      | 89 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 114 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7971c76..d74d166 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1945,6 +1945,13 @@ struct drm_i915_private {
>  	struct i915_suspend_saved_registers regfile;
>  	struct vlv_s0ix_state vlv_s0ix_state;
>  
> +	enum {
> +		I915_SKL_SAGV_UNKNOWN = 0,
> +		I915_SKL_SAGV_DISABLED,
> +		I915_SKL_SAGV_ENABLED,
> +		I915_SKL_SAGV_NOT_CONTROLLED
> +	} skl_sagv_status;
> +
>  	struct {
>  		/*
>  		 * Raw watermark latency values:
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 73b3d4d..4980cfe 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7144,6 +7144,10 @@ enum {
>  #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
>  #define   DISPLAY_IPS_CONTROL			0x19
>  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> +#define   GEN9_PCODE_SAGV_CONTROL		0x21
> +#define     GEN9_SAGV_DISABLE			0x0
> +#define     GEN9_SAGV_IS_DISABLED		0x1
> +#define     GEN9_SAGV_DYNAMIC_FREQ              0x3

Hmm. The definition of these bits is definitely peculiar. Unfortunately
the spec doesn't seem to explain them. First I though bit 0 might be
more of an ack bit, but then why would we set it for the "enable" request?

I'd maybe do s/DYNAMIC_FREQ/ENABLE/ to make it more clear what is the
counterpart to DISABLE.

>  #define GEN6_PCODE_DATA				_MMIO(0x138128)
>  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
>  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c5c0c35..35bdd67 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14142,6 +14142,14 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		     intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco))
>  			dev_priv->display.modeset_commit_cdclk(state);
>  
> +		/*
> +		 * SKL workaround: bspec recommends we disable the SAGV when we
> +		 * have more then one pipe enabled
> +		 */

It doesn't actually say that AFAICS. What it says is the *if* you
guarantee that in single pipe mode you never run afoul of the SAGV
block time (30us on SKL), then you can simply disable SAGV depending
on the number of active pipes. I guess that can sort of imply that you
shouldn't enable it with multiple pipes, but it's a very vague way of
saying that for sure.

Then it goes on to say something that if you can't enable watermark
levels with latency>=SAGV block time, then you shouldn't enable SAGV
either. I would interpret that so that you have to have to have
enabled at least the first watermark level with higher latency than
the SAGV block time, since that means the plane can tolerate also
the SAGV block time.

Eg. on one SKL I have the following WM latencies:
WM0 latency 2 (2.0 usec)
WM1 latency 19 (19.0 usec)
WM2 latency 28 (28.0 usec)
WM3 latency 32 (32.0 usec)
WM4 latency 63 (63.0 usec)
WM5 latency 77 (77.0 usec)
WM6 latency 83 (83.0 usec)
WM7 latency 99 (99.0 usec)

So if we can't enable WM3 for all of the active planes, then we should
not enable SAGV. But if all planes can enable WM3 or higher, we can
also enable SAGV.

Anyways, doing it all properly seems like a bit more work, so we can
definitely leave it for a future improvement.

> +		if (IS_SKYLAKE(dev_priv) &&
> +		    hweight32(intel_state->active_crtcs) > 1)
> +			skl_disable_sagv(dev_priv);
> +
>  		intel_modeset_verify_disabled(dev);
>  	}
>  
> @@ -14215,6 +14223,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
>  	}
>  
> +	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
> +	    hweight32(intel_state->active_crtcs) <= 1)
> +		skl_enable_sagv(dev_priv);
> +
>  	drm_atomic_helper_commit_hw_done(state);
>  
>  	if (intel_state->modeset)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9539f0e..76e78b8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1721,6 +1721,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 */);
> +int skl_enable_sagv(struct drm_i915_private *dev_priv);
> +int skl_disable_sagv(struct drm_i915_private *dev_priv);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8752730..0a202264 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2883,6 +2883,95 @@ skl_wm_plane_id(const struct intel_plane *plane)
>  	}
>  }
>  
> +/*
> + * SAGV dynamically adjusts the system agent voltage and clock frequencies
> + * depending on power and performance requirements. The display engine access
> + * to system memory is blocked during the adjustment time. Having this enabled
> + * in multi-pipe configurations can cause issues (such as underruns causing
> + * full system hangs), and the bspec also suggests that software disable it
> + * when more then one pipe is enabled.
> + */
> +int
> +skl_enable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +
> +	if (dev_priv->skl_sagv_status &&

Was that supposed to check for NOT_CONTROLLED?

> +	    dev_priv->skl_sagv_status != I915_SKL_SAGV_DISABLED)
> +		return 0;
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	DRM_DEBUG_KMS("Enabling the SAGV\n");
> +

printk can be outside the mutex.

> +	ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				      GEN9_SAGV_DYNAMIC_FREQ);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (!ret) {
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED;
> +	} else if (ret == -EINVAL) {
> +		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
> +		ret = 0;
> +	} else {
> +		DRM_ERROR("Failed to enable the SAGV\n");
> +	}
> +
> +	/* We don't need to wait for SAGV when enabling */
> +	return ret;
> +}
> +
> +static int
> +skl_do_sagv_disable(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +	uint32_t temp = GEN9_SAGV_DISABLE;
> +
> +	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				     &temp);
> +	if (ret) {
> +		/*
> +		 * Some very early Skylake systems don't actually let you
> +		 * control the SAGV, which is normal.
> +		 */
> +		if (ret != -EINVAL)
> +			DRM_ERROR("Failed to disable the SAGV\n");

Why are we printing errors both here and in the caller?

> +
> +		return ret;
> +	}
> +
> +	return temp & GEN9_SAGV_IS_DISABLED;
> +}
> +
> +int
> +skl_disable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret, result;
> +
> +	if (dev_priv->skl_sagv_status &&

same question about NOT_CONTROLLED

> +	    dev_priv->skl_sagv_status != I915_SKL_SAGV_ENABLED)
> +		return 0;
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	DRM_DEBUG_KMS("Disabling the SAGV\n");

this printk can move up as well

> +
> +	/* bspec says to keep retrying for at least 1 ms */
> +	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (!ret) {
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED;
> +	} else if (ret == -ETIMEDOUT) {
> +		DRM_ERROR("Request to disable SAGV timed out\n");
> +	} else if (result == -EINVAL) {
> +		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
> +		ret = 0;
> +	}

The ret/result thing here looks messy. Maybe deal with 'ret' first,
and the with 'result':

if (ret) {
	DRM_ERROR...
	return ret;
}

switch (result) {
case -EINVAL:
	...
case 0:
case 1:
	...
default:
	...
}

or whatever are all the different cases you want to distinguish. Or did
I miss some subtle logic here?

> +
> +	return ret;
> +}
> +
>  static void
>  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  				   const struct intel_crtc_state *cstate,
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC

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

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 03:54:31PM -0400, Lyude wrote:
> Since the watermark calculations for Skylake are still broken, we're apt
> to hitting underruns very easily under multi-monitor configurations.
> While it would be lovely if this was fixed, it's not. Another problem
> that's been coming from this however, is the mysterious issue of
> underruns causing full system hangs. An easy way to reproduce this with
> a skylake system:
> 
> - Get a laptop with a skylake GPU, and hook up two external monitors to
>   it
> - Move the cursor from the built-in LCD to one of the external displays
>   as quickly as you can
> - You'll get a few pipe underruns, and eventually the entire system will
>   just freeze.
> 
> After doing a lot of investigation and reading through the bspec, I
> found the existence of the SAGV, which is responsible for adjusting the
> system agent voltage and clock frequencies depending on how much power
> we need. According to the bspec:
> 
> "The display engine access to system memory is blocked during the
>  adjustment time. SAGV defaults to enabled. Software must use the
>  GT-driver pcode mailbox to disable SAGV when the display engine is not
>  able to tolerate the blocking time."
> 
> The rest of the bspec goes on to explain that software can simply leave
> the SAGV enabled, and disable it when we use interlaced pipes/have more
> then one pipe active.
> 
> Sure enough, with this patchset the system hangs resulting from pipe
> underruns on Skylake have completely vanished on my T460s. Additionally,
> the bspec mentions turning off the SAGV	with more then one pipe enabled
> as a workaround for display underruns. While this patch doesn't entirely
> fix that, it looks like it does improve the situation a little bit so
> it's likely this is going to be required to make watermarks on Skylake
> fully functional.
> 
> Changes since v10:
>  - Apparently sandybridge_pcode_read actually writes values and reads
>    them back, despite it's misleading function name. This means we've
>    been doing this mostly wrong and have been writing garbage to the
>    SAGV control. Because of this, we no longer attempt to read the SAGV
>    status during initialization (since there are no helpers for this).
>  - mlankhorst noticed that this patch was breaking on some very early
>    pre-release Skylake machines, which apparently don't allow you to
>    disable the SAGV. To prevent machines from failing tests due to SAGV
>    errors, if the first time we try to control the SAGV results in the
>    mailbox indicating an invalid command, we just disable future attempts
>    to control the SAGV state by setting dev_priv->skl_sagv_status to
>    I915_SKL_SAGV_NOT_CONTROLLED and make a note of it in dmesg.
>  - Move mutex_unlock() a little higher in skl_enable_sagv(). This
>    doesn't actually fix anything, but lets us release the lock a little
>    sooner since we're finished with it.
> Changes since v9:
>  - Only enable/disable sagv on Skylake
> Changes since v8:
>  - Add intel_state->modeset guard to the conditional for
>    skl_enable_sagv()
> Changes since v7:
>  - Remove GEN9_SAGV_LOW_FREQ, replace with GEN9_SAGV_IS_ENABLED (that's
>    all we use it for anyway)
>  - Use GEN9_SAGV_IS_ENABLED instead of 0x1 for clarification
>  - Fix a styling error that snuck past me
> Changes since v6:
>  - Protect skl_enable_sagv() with intel_state->modeset conditional in
>    intel_atomic_commit_tail()
> Changes since v5:
>  - Don't use is_power_of_2. Makes things confusing
>  - Don't use the old state to figure out whether or not to
>    enable/disable the sagv, use the new one
>  - Split the loop in skl_disable_sagv into it's own function
>  - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail()
> Changes since v4:
>  - Use is_power_of_2 against active_crtcs to check whether we have > 1
>    pipe enabled
>  - Fix skl_sagv_get_hw_state(): (temp & 0x1) indicates disabled, 0x0
>    enabled
>  - Call skl_sagv_enable/disable() from pre/post-plane updates
> Changes since v3:
>  - Use time_before() to compare timeout to jiffies
> Changes since v2:
>  - Really apply minor style nitpicks to patch this time
> Changes since v1:
>  - Added comments about this probably being one of the requirements to
>    fixing Skylake's watermark issues
>  - Minor style nitpicks from Matt Roper
>  - Disable these functions on Broxton, since it doesn't have an SAGV
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  7 +++
>  drivers/gpu/drm/i915/i915_reg.h      |  4 ++
>  drivers/gpu/drm/i915/intel_display.c | 12 +++++
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>  drivers/gpu/drm/i915/intel_pm.c      | 89 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 114 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7971c76..d74d166 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1945,6 +1945,13 @@ struct drm_i915_private {
>  	struct i915_suspend_saved_registers regfile;
>  	struct vlv_s0ix_state vlv_s0ix_state;
>  
> +	enum {
> +		I915_SKL_SAGV_UNKNOWN = 0,
> +		I915_SKL_SAGV_DISABLED,
> +		I915_SKL_SAGV_ENABLED,
> +		I915_SKL_SAGV_NOT_CONTROLLED
> +	} skl_sagv_status;
> +
>  	struct {
>  		/*
>  		 * Raw watermark latency values:
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 73b3d4d..4980cfe 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7144,6 +7144,10 @@ enum {
>  #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
>  #define   DISPLAY_IPS_CONTROL			0x19
>  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> +#define   GEN9_PCODE_SAGV_CONTROL		0x21
> +#define     GEN9_SAGV_DISABLE			0x0
> +#define     GEN9_SAGV_IS_DISABLED		0x1
> +#define     GEN9_SAGV_DYNAMIC_FREQ              0x3

Hmm. The definition of these bits is definitely peculiar. Unfortunately
the spec doesn't seem to explain them. First I though bit 0 might be
more of an ack bit, but then why would we set it for the "enable" request?

I'd maybe do s/DYNAMIC_FREQ/ENABLE/ to make it more clear what is the
counterpart to DISABLE.

>  #define GEN6_PCODE_DATA				_MMIO(0x138128)
>  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
>  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c5c0c35..35bdd67 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14142,6 +14142,14 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		     intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco))
>  			dev_priv->display.modeset_commit_cdclk(state);
>  
> +		/*
> +		 * SKL workaround: bspec recommends we disable the SAGV when we
> +		 * have more then one pipe enabled
> +		 */

It doesn't actually say that AFAICS. What it says is the *if* you
guarantee that in single pipe mode you never run afoul of the SAGV
block time (30us on SKL), then you can simply disable SAGV depending
on the number of active pipes. I guess that can sort of imply that you
shouldn't enable it with multiple pipes, but it's a very vague way of
saying that for sure.

Then it goes on to say something that if you can't enable watermark
levels with latency>=SAGV block time, then you shouldn't enable SAGV
either. I would interpret that so that you have to have to have
enabled at least the first watermark level with higher latency than
the SAGV block time, since that means the plane can tolerate also
the SAGV block time.

Eg. on one SKL I have the following WM latencies:
WM0 latency 2 (2.0 usec)
WM1 latency 19 (19.0 usec)
WM2 latency 28 (28.0 usec)
WM3 latency 32 (32.0 usec)
WM4 latency 63 (63.0 usec)
WM5 latency 77 (77.0 usec)
WM6 latency 83 (83.0 usec)
WM7 latency 99 (99.0 usec)

So if we can't enable WM3 for all of the active planes, then we should
not enable SAGV. But if all planes can enable WM3 or higher, we can
also enable SAGV.

Anyways, doing it all properly seems like a bit more work, so we can
definitely leave it for a future improvement.

> +		if (IS_SKYLAKE(dev_priv) &&
> +		    hweight32(intel_state->active_crtcs) > 1)
> +			skl_disable_sagv(dev_priv);
> +
>  		intel_modeset_verify_disabled(dev);
>  	}
>  
> @@ -14215,6 +14223,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
>  	}
>  
> +	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
> +	    hweight32(intel_state->active_crtcs) <= 1)
> +		skl_enable_sagv(dev_priv);
> +
>  	drm_atomic_helper_commit_hw_done(state);
>  
>  	if (intel_state->modeset)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9539f0e..76e78b8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1721,6 +1721,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 */);
> +int skl_enable_sagv(struct drm_i915_private *dev_priv);
> +int skl_disable_sagv(struct drm_i915_private *dev_priv);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8752730..0a202264 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2883,6 +2883,95 @@ skl_wm_plane_id(const struct intel_plane *plane)
>  	}
>  }
>  
> +/*
> + * SAGV dynamically adjusts the system agent voltage and clock frequencies
> + * depending on power and performance requirements. The display engine access
> + * to system memory is blocked during the adjustment time. Having this enabled
> + * in multi-pipe configurations can cause issues (such as underruns causing
> + * full system hangs), and the bspec also suggests that software disable it
> + * when more then one pipe is enabled.
> + */
> +int
> +skl_enable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +
> +	if (dev_priv->skl_sagv_status &&

Was that supposed to check for NOT_CONTROLLED?

> +	    dev_priv->skl_sagv_status != I915_SKL_SAGV_DISABLED)
> +		return 0;
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	DRM_DEBUG_KMS("Enabling the SAGV\n");
> +

printk can be outside the mutex.

> +	ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				      GEN9_SAGV_DYNAMIC_FREQ);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (!ret) {
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED;
> +	} else if (ret == -EINVAL) {
> +		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
> +		ret = 0;
> +	} else {
> +		DRM_ERROR("Failed to enable the SAGV\n");
> +	}
> +
> +	/* We don't need to wait for SAGV when enabling */
> +	return ret;
> +}
> +
> +static int
> +skl_do_sagv_disable(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +	uint32_t temp = GEN9_SAGV_DISABLE;
> +
> +	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				     &temp);
> +	if (ret) {
> +		/*
> +		 * Some very early Skylake systems don't actually let you
> +		 * control the SAGV, which is normal.
> +		 */
> +		if (ret != -EINVAL)
> +			DRM_ERROR("Failed to disable the SAGV\n");

Why are we printing errors both here and in the caller?

> +
> +		return ret;
> +	}
> +
> +	return temp & GEN9_SAGV_IS_DISABLED;
> +}
> +
> +int
> +skl_disable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret, result;
> +
> +	if (dev_priv->skl_sagv_status &&

same question about NOT_CONTROLLED

> +	    dev_priv->skl_sagv_status != I915_SKL_SAGV_ENABLED)
> +		return 0;
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	DRM_DEBUG_KMS("Disabling the SAGV\n");

this printk can move up as well

> +
> +	/* bspec says to keep retrying for at least 1 ms */
> +	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (!ret) {
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED;
> +	} else if (ret == -ETIMEDOUT) {
> +		DRM_ERROR("Request to disable SAGV timed out\n");
> +	} else if (result == -EINVAL) {
> +		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
> +		ret = 0;
> +	}

The ret/result thing here looks messy. Maybe deal with 'ret' first,
and the with 'result':

if (ret) {
	DRM_ERROR...
	return ret;
}

switch (result) {
case -EINVAL:
	...
case 0:
case 1:
	...
default:
	...
}

or whatever are all the different cases you want to distinguish. Or did
I miss some subtle logic here?

> +
> +	return ret;
> +}
> +
>  static void
>  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  				   const struct intel_crtc_state *cstate,
> -- 
> 2.7.4

-- 
Ville Syrj�l�
Intel OTC

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

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 03:54:31PM -0400, Lyude wrote:
> Since the watermark calculations for Skylake are still broken, we're apt
> to hitting underruns very easily under multi-monitor configurations.
> While it would be lovely if this was fixed, it's not. Another problem
> that's been coming from this however, is the mysterious issue of
> underruns causing full system hangs. An easy way to reproduce this with
> a skylake system:
> 
> - Get a laptop with a skylake GPU, and hook up two external monitors to
>   it
> - Move the cursor from the built-in LCD to one of the external displays
>   as quickly as you can
> - You'll get a few pipe underruns, and eventually the entire system will
>   just freeze.
> 
> After doing a lot of investigation and reading through the bspec, I
> found the existence of the SAGV, which is responsible for adjusting the
> system agent voltage and clock frequencies depending on how much power
> we need. According to the bspec:
> 
> "The display engine access to system memory is blocked during the
>  adjustment time. SAGV defaults to enabled. Software must use the
>  GT-driver pcode mailbox to disable SAGV when the display engine is not
>  able to tolerate the blocking time."
> 
> The rest of the bspec goes on to explain that software can simply leave
> the SAGV enabled, and disable it when we use interlaced pipes/have more
> then one pipe active.
> 
> Sure enough, with this patchset the system hangs resulting from pipe
> underruns on Skylake have completely vanished on my T460s. Additionally,
> the bspec mentions turning off the SAGV	with more then one pipe enabled
> as a workaround for display underruns. While this patch doesn't entirely
> fix that, it looks like it does improve the situation a little bit so
> it's likely this is going to be required to make watermarks on Skylake
> fully functional.
> 
> Changes since v10:
>  - Apparently sandybridge_pcode_read actually writes values and reads
>    them back, despite it's misleading function name. This means we've
>    been doing this mostly wrong and have been writing garbage to the
>    SAGV control. Because of this, we no longer attempt to read the SAGV
>    status during initialization (since there are no helpers for this).
>  - mlankhorst noticed that this patch was breaking on some very early
>    pre-release Skylake machines, which apparently don't allow you to
>    disable the SAGV. To prevent machines from failing tests due to SAGV
>    errors, if the first time we try to control the SAGV results in the
>    mailbox indicating an invalid command, we just disable future attempts
>    to control the SAGV state by setting dev_priv->skl_sagv_status to
>    I915_SKL_SAGV_NOT_CONTROLLED and make a note of it in dmesg.
>  - Move mutex_unlock() a little higher in skl_enable_sagv(). This
>    doesn't actually fix anything, but lets us release the lock a little
>    sooner since we're finished with it.
> Changes since v9:
>  - Only enable/disable sagv on Skylake
> Changes since v8:
>  - Add intel_state->modeset guard to the conditional for
>    skl_enable_sagv()
> Changes since v7:
>  - Remove GEN9_SAGV_LOW_FREQ, replace with GEN9_SAGV_IS_ENABLED (that's
>    all we use it for anyway)
>  - Use GEN9_SAGV_IS_ENABLED instead of 0x1 for clarification
>  - Fix a styling error that snuck past me
> Changes since v6:
>  - Protect skl_enable_sagv() with intel_state->modeset conditional in
>    intel_atomic_commit_tail()
> Changes since v5:
>  - Don't use is_power_of_2. Makes things confusing
>  - Don't use the old state to figure out whether or not to
>    enable/disable the sagv, use the new one
>  - Split the loop in skl_disable_sagv into it's own function
>  - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail()
> Changes since v4:
>  - Use is_power_of_2 against active_crtcs to check whether we have > 1
>    pipe enabled
>  - Fix skl_sagv_get_hw_state(): (temp & 0x1) indicates disabled, 0x0
>    enabled
>  - Call skl_sagv_enable/disable() from pre/post-plane updates
> Changes since v3:
>  - Use time_before() to compare timeout to jiffies
> Changes since v2:
>  - Really apply minor style nitpicks to patch this time
> Changes since v1:
>  - Added comments about this probably being one of the requirements to
>    fixing Skylake's watermark issues
>  - Minor style nitpicks from Matt Roper
>  - Disable these functions on Broxton, since it doesn't have an SAGV
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  7 +++
>  drivers/gpu/drm/i915/i915_reg.h      |  4 ++
>  drivers/gpu/drm/i915/intel_display.c | 12 +++++
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>  drivers/gpu/drm/i915/intel_pm.c      | 89 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 114 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7971c76..d74d166 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1945,6 +1945,13 @@ struct drm_i915_private {
>  	struct i915_suspend_saved_registers regfile;
>  	struct vlv_s0ix_state vlv_s0ix_state;
>  
> +	enum {
> +		I915_SKL_SAGV_UNKNOWN = 0,
> +		I915_SKL_SAGV_DISABLED,
> +		I915_SKL_SAGV_ENABLED,
> +		I915_SKL_SAGV_NOT_CONTROLLED
> +	} skl_sagv_status;
> +
>  	struct {
>  		/*
>  		 * Raw watermark latency values:
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 73b3d4d..4980cfe 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7144,6 +7144,10 @@ enum {
>  #define   HSW_PCODE_DE_WRITE_FREQ_REQ		0x17
>  #define   DISPLAY_IPS_CONTROL			0x19
>  #define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> +#define   GEN9_PCODE_SAGV_CONTROL		0x21
> +#define     GEN9_SAGV_DISABLE			0x0
> +#define     GEN9_SAGV_IS_DISABLED		0x1
> +#define     GEN9_SAGV_DYNAMIC_FREQ              0x3

Hmm. The definition of these bits is definitely peculiar. Unfortunately
the spec doesn't seem to explain them. First I though bit 0 might be
more of an ack bit, but then why would we set it for the "enable" request?

I'd maybe do s/DYNAMIC_FREQ/ENABLE/ to make it more clear what is the
counterpart to DISABLE.

>  #define GEN6_PCODE_DATA				_MMIO(0x138128)
>  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
>  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c5c0c35..35bdd67 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14142,6 +14142,14 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		     intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco))
>  			dev_priv->display.modeset_commit_cdclk(state);
>  
> +		/*
> +		 * SKL workaround: bspec recommends we disable the SAGV when we
> +		 * have more then one pipe enabled
> +		 */

It doesn't actually say that AFAICS. What it says is the *if* you
guarantee that in single pipe mode you never run afoul of the SAGV
block time (30us on SKL), then you can simply disable SAGV depending
on the number of active pipes. I guess that can sort of imply that you
shouldn't enable it with multiple pipes, but it's a very vague way of
saying that for sure.

Then it goes on to say something that if you can't enable watermark
levels with latency>=SAGV block time, then you shouldn't enable SAGV
either. I would interpret that so that you have to have to have
enabled at least the first watermark level with higher latency than
the SAGV block time, since that means the plane can tolerate also
the SAGV block time.

Eg. on one SKL I have the following WM latencies:
WM0 latency 2 (2.0 usec)
WM1 latency 19 (19.0 usec)
WM2 latency 28 (28.0 usec)
WM3 latency 32 (32.0 usec)
WM4 latency 63 (63.0 usec)
WM5 latency 77 (77.0 usec)
WM6 latency 83 (83.0 usec)
WM7 latency 99 (99.0 usec)

So if we can't enable WM3 for all of the active planes, then we should
not enable SAGV. But if all planes can enable WM3 or higher, we can
also enable SAGV.

Anyways, doing it all properly seems like a bit more work, so we can
definitely leave it for a future improvement.

> +		if (IS_SKYLAKE(dev_priv) &&
> +		    hweight32(intel_state->active_crtcs) > 1)
> +			skl_disable_sagv(dev_priv);
> +
>  		intel_modeset_verify_disabled(dev);
>  	}
>  
> @@ -14215,6 +14223,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
>  	}
>  
> +	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
> +	    hweight32(intel_state->active_crtcs) <= 1)
> +		skl_enable_sagv(dev_priv);
> +
>  	drm_atomic_helper_commit_hw_done(state);
>  
>  	if (intel_state->modeset)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9539f0e..76e78b8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1721,6 +1721,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 */);
> +int skl_enable_sagv(struct drm_i915_private *dev_priv);
> +int skl_disable_sagv(struct drm_i915_private *dev_priv);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8752730..0a202264 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2883,6 +2883,95 @@ skl_wm_plane_id(const struct intel_plane *plane)
>  	}
>  }
>  
> +/*
> + * SAGV dynamically adjusts the system agent voltage and clock frequencies
> + * depending on power and performance requirements. The display engine access
> + * to system memory is blocked during the adjustment time. Having this enabled
> + * in multi-pipe configurations can cause issues (such as underruns causing
> + * full system hangs), and the bspec also suggests that software disable it
> + * when more then one pipe is enabled.
> + */
> +int
> +skl_enable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +
> +	if (dev_priv->skl_sagv_status &&

Was that supposed to check for NOT_CONTROLLED?

> +	    dev_priv->skl_sagv_status != I915_SKL_SAGV_DISABLED)
> +		return 0;
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	DRM_DEBUG_KMS("Enabling the SAGV\n");
> +

printk can be outside the mutex.

> +	ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				      GEN9_SAGV_DYNAMIC_FREQ);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (!ret) {
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED;
> +	} else if (ret == -EINVAL) {
> +		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
> +		ret = 0;
> +	} else {
> +		DRM_ERROR("Failed to enable the SAGV\n");
> +	}
> +
> +	/* We don't need to wait for SAGV when enabling */
> +	return ret;
> +}
> +
> +static int
> +skl_do_sagv_disable(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +	uint32_t temp = GEN9_SAGV_DISABLE;
> +
> +	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				     &temp);
> +	if (ret) {
> +		/*
> +		 * Some very early Skylake systems don't actually let you
> +		 * control the SAGV, which is normal.
> +		 */
> +		if (ret != -EINVAL)
> +			DRM_ERROR("Failed to disable the SAGV\n");

Why are we printing errors both here and in the caller?

> +
> +		return ret;
> +	}
> +
> +	return temp & GEN9_SAGV_IS_DISABLED;
> +}
> +
> +int
> +skl_disable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret, result;
> +
> +	if (dev_priv->skl_sagv_status &&

same question about NOT_CONTROLLED

> +	    dev_priv->skl_sagv_status != I915_SKL_SAGV_ENABLED)
> +		return 0;
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	DRM_DEBUG_KMS("Disabling the SAGV\n");

this printk can move up as well

> +
> +	/* bspec says to keep retrying for at least 1 ms */
> +	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (!ret) {
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED;
> +	} else if (ret == -ETIMEDOUT) {
> +		DRM_ERROR("Request to disable SAGV timed out\n");
> +	} else if (result == -EINVAL) {
> +		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
> +		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
> +		ret = 0;
> +	}

The ret/result thing here looks messy. Maybe deal with 'ret' first,
and the with 'result':

if (ret) {
	DRM_ERROR...
	return ret;
}

switch (result) {
case -EINVAL:
	...
case 0:
case 1:
	...
default:
	...
}

or whatever are all the different cases you want to distinguish. Or did
I miss some subtle logic here?

> +
> +	return ret;
> +}
> +
>  static void
>  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  				   const struct intel_crtc_state *cstate,
> -- 
> 2.7.4

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

Re: [PATCH v11 4/7] drm/i915/skl: Update plane watermarks atomically during plane updates

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 03:54:33PM -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
>   }
> 
> So this patch moves all of the watermark writes into the right place;
> inside of the vblank evasion where we update all of the registers for
> each plane. While this patch doesn't fix everything, it does allow us to
> update the watermark values in the way the hardware expects us to.
> 
> 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:
> 
> Changes since v1:
>  - Use IS_GEN9() instead of IS_SKYLAKE() since these fixes apply to more
>    then just Skylake
>  - Update description to make it clear this patch doesn't fix everything
>  - Check if pipes were actually changed before writing watermarks
> 
> Changes since v2:
>  - Write PIPE_WM_LINETIME during vblank evasion
> 
> Changes since v3:
>  - Rebase against new SAGV patch changes
> 
> Changes since v4:
>  - Add a parameter to choose what skl_wm_values struct to use when
>    writing new plane watermarks
> 
> Changes since v5:
>  - Remove cursor ddb entry write in skl_write_cursor_wm(), defer until
>    patch 6
>  - Write WM_LINETIME in intel_begin_crtc_commit()
> 
> Changes since v6:
>  - Remove redundant dirty_pipes check in skl_write_plane_wm (we check
>    this in all places where we call this function, and it was supposed
>    to have been removed earlier anyway)
>  - In i9xx_update_cursor(), use dev_priv->info.gen >= 9 instead of
>    IS_GEN9(dev_priv). We do this everywhere else and I'd imagine this
>    needs to be done for gen10 as well
> 
> Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
> Signed-off-by: Lyude <cpaul@redhat.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
>  drivers/gpu/drm/i915/intel_pm.c      | 50 ++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_sprite.c  |  7 +++++
>  4 files changed, 62 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 35bdd67..b2f8e24 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3386,6 +3386,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);

rebase fail?

Please double check with gcc/sparse that new warnings are kept to a
minimum.

> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;

const?

>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl;
>  	unsigned int rotation = plane_state->base.rotation;
> @@ -3419,6 +3421,9 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	intel_crtc->adjusted_x = src_x;
>  	intel_crtc->adjusted_y = src_y;
>  
> +	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> +	    skl_write_plane_wm(intel_crtc, wm, 0);

Indent fail.

> +
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
>  	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> @@ -10699,9 +10704,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
> +	if (dev_priv->info.gen >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc)

INTEL_GEN()

> +		skl_write_cursor_wm(intel_crtc, wm);
> +
>  	if (plane_state && plane_state->base.visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
>  		switch (plane_state->base.crtc_w) {
> @@ -14610,10 +14619,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  				    struct drm_crtc_state *old_crtc_state)
>  {
>  	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_crtc_state *old_intel_state =
>  		to_intel_crtc_state(old_crtc_state);
>  	bool modeset = needs_modeset(crtc->state);
> +	enum pipe pipe = intel_crtc->pipe;
>  
>  	/* Perform vblank evasion around commit operation */
>  	intel_pipe_update_start(intel_crtc);
> @@ -14628,8 +14639,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  
>  	if (to_intel_crtc_state(crtc->state)->update_pipe)
>  		intel_update_pipe_config(intel_crtc, old_intel_state);
> -	else if (INTEL_INFO(dev)->gen >= 9)
> +	else if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_detach_scalers(intel_crtc);
> +
> +		I915_WRITE(PIPE_WM_LINETIME(pipe),
> +			   dev_priv->wm.skl_hw.wm_linetime[pipe]);
> +	}
>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 76e78b8..88088c3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1723,6 +1723,11 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */);
>  int skl_enable_sagv(struct drm_i915_private *dev_priv);
>  int skl_disable_sagv(struct drm_i915_private *dev_priv);
> +void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> +			 const struct skl_wm_values *wm);
> +void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> +			const struct skl_wm_values *wm,
> +			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 77166c6..e8653d8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3779,6 +3779,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,
> +			const struct skl_wm_values *wm,
> +			int plane)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	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,
> +			 const struct skl_wm_values *wm)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	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)
>  {
> @@ -3786,7 +3819,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)
> @@ -3794,21 +3827,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  		if (!crtc->active)
>  			continue;
>  
> -		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 366900d..3c560f9 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -203,6 +203,10 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> +	struct drm_crtc *crtc = crtc_state->base.crtc;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  	u32 plane_ctl;
> @@ -228,6 +232,9 @@ skl_update_plane(struct drm_plane *drm_plane,
>  
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
> +	if (wm->dirty_pipes & drm_crtc_mask(crtc))
> +	    skl_write_plane_wm(intel_crtc, wm, 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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v11 4/7] drm/i915/skl: Update plane watermarks atomically during plane updates

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 03:54:33PM -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
>   }
> 
> So this patch moves all of the watermark writes into the right place;
> inside of the vblank evasion where we update all of the registers for
> each plane. While this patch doesn't fix everything, it does allow us to
> update the watermark values in the way the hardware expects us to.
> 
> 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:
> 
> Changes since v1:
>  - Use IS_GEN9() instead of IS_SKYLAKE() since these fixes apply to more
>    then just Skylake
>  - Update description to make it clear this patch doesn't fix everything
>  - Check if pipes were actually changed before writing watermarks
> 
> Changes since v2:
>  - Write PIPE_WM_LINETIME during vblank evasion
> 
> Changes since v3:
>  - Rebase against new SAGV patch changes
> 
> Changes since v4:
>  - Add a parameter to choose what skl_wm_values struct to use when
>    writing new plane watermarks
> 
> Changes since v5:
>  - Remove cursor ddb entry write in skl_write_cursor_wm(), defer until
>    patch 6
>  - Write WM_LINETIME in intel_begin_crtc_commit()
> 
> Changes since v6:
>  - Remove redundant dirty_pipes check in skl_write_plane_wm (we check
>    this in all places where we call this function, and it was supposed
>    to have been removed earlier anyway)
>  - In i9xx_update_cursor(), use dev_priv->info.gen >= 9 instead of
>    IS_GEN9(dev_priv). We do this everywhere else and I'd imagine this
>    needs to be done for gen10 as well
> 
> Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
> Signed-off-by: Lyude <cpaul@redhat.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Cc: stable@vger.kernel.org
> Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
>  drivers/gpu/drm/i915/intel_pm.c      | 50 ++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_sprite.c  |  7 +++++
>  4 files changed, 62 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 35bdd67..b2f8e24 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3386,6 +3386,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);

rebase fail?

Please double check with gcc/sparse that new warnings are kept to a
minimum.

> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;

const?

>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl;
>  	unsigned int rotation = plane_state->base.rotation;
> @@ -3419,6 +3421,9 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	intel_crtc->adjusted_x = src_x;
>  	intel_crtc->adjusted_y = src_y;
>  
> +	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> +	    skl_write_plane_wm(intel_crtc, wm, 0);

Indent fail.

> +
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
>  	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> @@ -10699,9 +10704,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
> +	if (dev_priv->info.gen >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc)

INTEL_GEN()

> +		skl_write_cursor_wm(intel_crtc, wm);
> +
>  	if (plane_state && plane_state->base.visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
>  		switch (plane_state->base.crtc_w) {
> @@ -14610,10 +14619,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  				    struct drm_crtc_state *old_crtc_state)
>  {
>  	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_crtc_state *old_intel_state =
>  		to_intel_crtc_state(old_crtc_state);
>  	bool modeset = needs_modeset(crtc->state);
> +	enum pipe pipe = intel_crtc->pipe;
>  
>  	/* Perform vblank evasion around commit operation */
>  	intel_pipe_update_start(intel_crtc);
> @@ -14628,8 +14639,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  
>  	if (to_intel_crtc_state(crtc->state)->update_pipe)
>  		intel_update_pipe_config(intel_crtc, old_intel_state);
> -	else if (INTEL_INFO(dev)->gen >= 9)
> +	else if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_detach_scalers(intel_crtc);
> +
> +		I915_WRITE(PIPE_WM_LINETIME(pipe),
> +			   dev_priv->wm.skl_hw.wm_linetime[pipe]);
> +	}
>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 76e78b8..88088c3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1723,6 +1723,11 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */);
>  int skl_enable_sagv(struct drm_i915_private *dev_priv);
>  int skl_disable_sagv(struct drm_i915_private *dev_priv);
> +void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> +			 const struct skl_wm_values *wm);
> +void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> +			const struct skl_wm_values *wm,
> +			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 77166c6..e8653d8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3779,6 +3779,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,
> +			const struct skl_wm_values *wm,
> +			int plane)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	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,
> +			 const struct skl_wm_values *wm)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	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)
>  {
> @@ -3786,7 +3819,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)
> @@ -3794,21 +3827,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  		if (!crtc->active)
>  			continue;
>  
> -		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 366900d..3c560f9 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -203,6 +203,10 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> +	struct drm_crtc *crtc = crtc_state->base.crtc;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  	u32 plane_ctl;
> @@ -228,6 +232,9 @@ skl_update_plane(struct drm_plane *drm_plane,
>  
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
> +	if (wm->dirty_pipes & drm_crtc_mask(crtc))
> +	    skl_write_plane_wm(intel_crtc, wm, 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

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH v11 4/7] drm/i915/skl: Update plane watermarks atomically during plane updates

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 03:54:33PM -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
>   }
> 
> So this patch moves all of the watermark writes into the right place;
> inside of the vblank evasion where we update all of the registers for
> each plane. While this patch doesn't fix everything, it does allow us to
> update the watermark values in the way the hardware expects us to.
> 
> 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:
> 
> Changes since v1:
>  - Use IS_GEN9() instead of IS_SKYLAKE() since these fixes apply to more
>    then just Skylake
>  - Update description to make it clear this patch doesn't fix everything
>  - Check if pipes were actually changed before writing watermarks
> 
> Changes since v2:
>  - Write PIPE_WM_LINETIME during vblank evasion
> 
> Changes since v3:
>  - Rebase against new SAGV patch changes
> 
> Changes since v4:
>  - Add a parameter to choose what skl_wm_values struct to use when
>    writing new plane watermarks
> 
> Changes since v5:
>  - Remove cursor ddb entry write in skl_write_cursor_wm(), defer until
>    patch 6
>  - Write WM_LINETIME in intel_begin_crtc_commit()
> 
> Changes since v6:
>  - Remove redundant dirty_pipes check in skl_write_plane_wm (we check
>    this in all places where we call this function, and it was supposed
>    to have been removed earlier anyway)
>  - In i9xx_update_cursor(), use dev_priv->info.gen >= 9 instead of
>    IS_GEN9(dev_priv). We do this everywhere else and I'd imagine this
>    needs to be done for gen10 as well
> 
> Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
> Signed-off-by: Lyude <cpaul@redhat.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
>  drivers/gpu/drm/i915/intel_pm.c      | 50 ++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_sprite.c  |  7 +++++
>  4 files changed, 62 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 35bdd67..b2f8e24 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3386,6 +3386,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);

rebase fail?

Please double check with gcc/sparse that new warnings are kept to a
minimum.

> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;

const?

>  	int pipe = intel_crtc->pipe;
>  	u32 plane_ctl;
>  	unsigned int rotation = plane_state->base.rotation;
> @@ -3419,6 +3421,9 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	intel_crtc->adjusted_x = src_x;
>  	intel_crtc->adjusted_y = src_y;
>  
> +	if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base))
> +	    skl_write_plane_wm(intel_crtc, wm, 0);

Indent fail.

> +
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x);
>  	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> @@ -10699,9 +10704,13 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
> +	if (dev_priv->info.gen >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc)

INTEL_GEN()

> +		skl_write_cursor_wm(intel_crtc, wm);
> +
>  	if (plane_state && plane_state->base.visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
>  		switch (plane_state->base.crtc_w) {
> @@ -14610,10 +14619,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  				    struct drm_crtc_state *old_crtc_state)
>  {
>  	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_crtc_state *old_intel_state =
>  		to_intel_crtc_state(old_crtc_state);
>  	bool modeset = needs_modeset(crtc->state);
> +	enum pipe pipe = intel_crtc->pipe;
>  
>  	/* Perform vblank evasion around commit operation */
>  	intel_pipe_update_start(intel_crtc);
> @@ -14628,8 +14639,12 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc,
>  
>  	if (to_intel_crtc_state(crtc->state)->update_pipe)
>  		intel_update_pipe_config(intel_crtc, old_intel_state);
> -	else if (INTEL_INFO(dev)->gen >= 9)
> +	else if (INTEL_INFO(dev)->gen >= 9) {
>  		skl_detach_scalers(intel_crtc);
> +
> +		I915_WRITE(PIPE_WM_LINETIME(pipe),
> +			   dev_priv->wm.skl_hw.wm_linetime[pipe]);
> +	}
>  }
>  
>  static void intel_finish_crtc_commit(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 76e78b8..88088c3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1723,6 +1723,11 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */);
>  int skl_enable_sagv(struct drm_i915_private *dev_priv);
>  int skl_disable_sagv(struct drm_i915_private *dev_priv);
> +void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> +			 const struct skl_wm_values *wm);
> +void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> +			const struct skl_wm_values *wm,
> +			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 77166c6..e8653d8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3779,6 +3779,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,
> +			const struct skl_wm_values *wm,
> +			int plane)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	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,
> +			 const struct skl_wm_values *wm)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	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)
>  {
> @@ -3786,7 +3819,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)
> @@ -3794,21 +3827,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  		if (!crtc->active)
>  			continue;
>  
> -		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 366900d..3c560f9 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -203,6 +203,10 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> +	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> +	struct drm_crtc *crtc = crtc_state->base.crtc;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	const int pipe = intel_plane->pipe;
>  	const int plane = intel_plane->plane + 1;
>  	u32 plane_ctl;
> @@ -228,6 +232,9 @@ skl_update_plane(struct drm_plane *drm_plane,
>  
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
> +	if (wm->dirty_pipes & drm_crtc_mask(crtc))
> +	    skl_write_plane_wm(intel_crtc, wm, 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

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

Re: [PATCH v11 7/7] drm/i915/skl: Update DDB values atomically with wms/plane attrs

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 03:54:36PM -0400, Lyude wrote:
> Now that we can hook into update_crtcs and control the order in which we
> update CRTCs at each modeset, we can finish the final step of fixing
> Skylake's watermark handling by performing DDB updates at the same time
> as plane updates and watermark updates.
> 
> The first major change in this patch is skl_update_crtcs(), which
> handles ensuring that we order each CRTC update in our atomic commits
> properly so that they honor the DDB flush order.
> 
> The second major change in this patch is the order in which we flush the
> pipes. While the previous order may have worked, it can't be used in
> this approach since it no longer will do the right thing. For example,
> using the old ddb flush order:
> 
> We have pipes A, B, and C enabled, and we're disabling C. Initial ddb
> allocation looks like this:
> 
> |   A   |   B   |xxxxxxx|
> 
> Since we're performing the ddb updates after performing any CRTC
> disablements in intel_atomic_commit_tail(), the space to the right of
> pipe B is unallocated.
> 
> 1. Flush pipes with new allocation contained into old space. None
>    apply, so we skip this
> 2. Flush pipes having their allocation reduced, but overlapping with a
>    previous allocation. None apply, so we also skip this
> 3. Flush pipes that got more space allocated. This applies to A and B,
>    giving us the following update order: A, B
> 
> This is wrong, since updating pipe A first will cause it to overlap with
> B and potentially burst into flames. Our new order (see the code
> comments for details) would update the pipes in the proper order: B, A.
> 
> As well, we calculate the order for each DDB update during the check
> phase, and reference it later in the commit phase when we hit
> skl_update_crtcs().
> 
> This long overdue patch fixes the rest of the underruns on Skylake.
> 
> Changes since v1:
>  - Add skl_ddb_entry_write() for cursor into skl_write_cursor_wm()
> Changes since v2:
>  - Use the method for updating CRTCs that Ville suggested
>  - In skl_update_wm(), only copy the watermarks for the crtc that was
>    passed to us
> Changes since v3:
>  - Small comment fix in skl_ddb_allocation_overlaps()
> 
> Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration")
> Fixes: 8211bd5bdf5e ("drm/i915/skl: Program the DDB allocation")
> [omitting CC for stable, since this patch will need to be changed for
> such backports first]
> 
> Testcase: kms_cursor_legacy
> Signed-off-by: Lyude <cpaul@redhat.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 100 +++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h     |   7 ++
>  drivers/gpu/drm/i915/intel_pm.c      | 207 +++++++++--------------------------
>  3 files changed, 144 insertions(+), 170 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 61a45f1..68fdbf0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13348,16 +13348,23 @@ static void verify_wm_state(struct drm_crtc *crtc,
>  			  hw_entry->start, hw_entry->end);
>  	}
>  
> -	/* cursor */
> -	hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> -	sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
> -
> -	if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
> -		DRM_ERROR("mismatch in DDB state pipe %c cursor "
> -			  "(expected (%u,%u), found (%u,%u))\n",
> -			  pipe_name(pipe),
> -			  sw_entry->start, sw_entry->end,
> -			  hw_entry->start, hw_entry->end);
> +	/*
> +	 * cursor
> +	 * If the cursor plane isn't active, we may not have updated it's ddb
> +	 * allocation. In that case since the ddb allocation will be updated
> +	 * once the plane becomes visible, we can skip this check
> +	 */
> +	if (intel_crtc->cursor_addr) {
> +		hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> +		sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
> +
> +		if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
> +			DRM_ERROR("mismatch in DDB state pipe %c cursor "
> +				  "(expected (%u,%u), found (%u,%u))\n",
> +				  pipe_name(pipe),
> +				  sw_entry->start, sw_entry->end,
> +				  hw_entry->start, hw_entry->end);
> +		}
>  	}
>  }
>  
> @@ -14109,6 +14116,72 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
>  	}
>  }
>  
> +static void skl_update_crtcs(struct drm_atomic_state *state,
> +			     unsigned int *crtc_vblank_mask)
> +{
> +	struct drm_device *dev = state->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *old_crtc_state;
> +	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
> +	struct skl_ddb_allocation cur_ddb;
> +	bool progress;
> +	bool reallocated[I915_MAX_PIPES] = {};
> +	enum pipe pipe;
> +	int wait_vbl_pipes, i;
> +
> +	/*
> +	 * Whenever the number of active pipes change, so does the DDB
> +	 * allocation. DDB allocations on pipes cannot ever overlap with
> +	 * eachother at any point in time, so we need to change the order we
> +	 * update the pipes so that we ensure they never overlap inbetween DDB
> +	 * updates.
> +	 */
> +	do {
> +		progress = false;
> +		wait_vbl_pipes = 0;
> +		cur_ddb = dev_priv->wm.skl_hw.ddb;

I guess would could do the loop twice to avoid the copy. But not sure
that's really any more efficient.

> +
> +		for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> +			struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +			pipe = intel_crtc->pipe;
> +
> +			if (!intel_crtc->active || needs_modeset(crtc->state))
> +				continue;

crtc->state->active?

> +			if (skl_ddb_allocation_equals(&cur_ddb, new_ddb, pipe))
> +				continue;
> +			if (skl_ddb_allocation_overlaps(state, &cur_ddb,
> +							new_ddb, pipe))
> +				continue;
> +
> +			intel_update_crtc(crtc, state, old_crtc_state,
> +					  crtc_vblank_mask);

What did the caller want with the vblank_mask? Do more vblank waits?
Shouldn't be needed I think since we wait here, no?

> +
> +			wait_vbl_pipes |= drm_crtc_mask(crtc);
> +			reallocated[pipe] = true;
> +			progress = true;

I guess we could throw this out actully. wait_vbl_pipes!=0 should mean
the same thing.

> +		}
> +
> +		/* Wait for each pipe's new allocation to take effect */
> +		intel_atomic_wait_for_vblanks(dev, dev_priv, wait_vbl_pipes);
> +	} while (progress);
> +
> +	/*
> +	 * Now that we've handled any ddb reallocations, we can go ahead and
> +	 * enable any new pipes.
> +	 */
> +	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> +		pipe = to_intel_crtc(crtc)->pipe;
> +
> +		if (reallocated[pipe] || !crtc->state->active)
> +			continue;

Do we need this reallocated[] flag? Isn't this the same as
'active && needs_modeset' ?

> +
> +		intel_update_crtc(crtc, state, old_crtc_state,
> +				  crtc_vblank_mask);
> +	}
> +}
> +
>  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
> @@ -15738,8 +15811,6 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
>  	}
>  
> -	dev_priv->display.update_crtcs = intel_update_crtcs;
> -
>  	/* Returns the core display clock speed */
>  	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
>  		dev_priv->display.get_display_clock_speed =
> @@ -15829,6 +15900,11 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>  			skl_modeset_calc_cdclk;
>  	}
>  
> +	if (dev_priv->info.gen >= 9)
> +		dev_priv->display.update_crtcs = skl_update_crtcs;
> +	else
> +		dev_priv->display.update_crtcs = intel_update_crtcs;
> +
>  	switch (INTEL_INFO(dev_priv)->gen) {
>  	case 2:
>  		dev_priv->display.queue_flip = intel_gen2_queue_flip;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 88088c3..9f9fe69 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1723,6 +1723,13 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */);
>  int skl_enable_sagv(struct drm_i915_private *dev_priv);
>  int skl_disable_sagv(struct drm_i915_private *dev_priv);
> +bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
> +			       const struct skl_ddb_allocation *new,
> +			       enum pipe pipe);
> +bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> +				 const struct skl_ddb_allocation *old,
> +				 const struct skl_ddb_allocation *new,
> +				 enum pipe pipe);
>  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
>  			 const struct skl_wm_values *wm);
>  void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f2e0071..6fe9e57 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3794,6 +3794,11 @@ void skl_write_plane_wm(struct intel_crtc *intel_crtc,
>  			   wm->plane[pipe][plane][level]);
>  	}
>  	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
> +
> +	skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane),
> +			    &wm->ddb.plane[pipe][plane]);
> +	skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, plane),
> +			    &wm->ddb.y_plane[pipe][plane]);
>  }
>  
>  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> @@ -3810,170 +3815,49 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
>  			   wm->plane[pipe][PLANE_CURSOR][level]);
>  	}
>  	I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
> -}
> -
> -static void skl_write_wm_values(struct drm_i915_private *dev_priv,
> -				const struct skl_wm_values *new)
> -{
> -	struct drm_device *dev = &dev_priv->drm;
> -	struct intel_crtc *crtc;
>  
> -	for_each_intel_crtc(dev, crtc) {
> -		int i;
> -		enum pipe pipe = crtc->pipe;
> -
> -		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
> -			continue;
> -		if (!crtc->active)
> -			continue;
> -
> -		for (i = 0; i < intel_num_planes(crtc); i++) {
> -			skl_ddb_entry_write(dev_priv,
> -					    PLANE_BUF_CFG(pipe, i),
> -					    &new->ddb.plane[pipe][i]);
> -			skl_ddb_entry_write(dev_priv,
> -					    PLANE_NV12_BUF_CFG(pipe, i),
> -					    &new->ddb.y_plane[pipe][i]);
> -		}
> -
> -		skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> -				    &new->ddb.plane[pipe][PLANE_CURSOR]);
> -	}
> +	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> +			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
>  }
>  
> -/*
> - * When setting up a new DDB allocation arrangement, we need to correctly
> - * sequence the times at which the new allocations for the pipes are taken into
> - * account or we'll have pipes fetching from space previously allocated to
> - * another pipe.
> - *
> - * Roughly the sequence looks like:
> - *  1. re-allocate the pipe(s) with the allocation being reduced and not
> - *     overlapping with a previous light-up pipe (another way to put it is:
> - *     pipes with their new allocation strickly included into their old ones).
> - *  2. re-allocate the other pipes that get their allocation reduced
> - *  3. allocate the pipes having their allocation increased
> - *
> - * Steps 1. and 2. are here to take care of the following case:
> - * - Initially DDB looks like this:
> - *     |   B    |   C    |
> - * - enable pipe A.
> - * - pipe B has a reduced DDB allocation that overlaps with the old pipe C
> - *   allocation
> - *     |  A  |  B  |  C  |
> - *
> - * We need to sequence the re-allocation: C, B, A (and not B, C, A).
> - */
> -
> -static void
> -skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, int pass)
> -{
> -	int plane;
> -
> -	DRM_DEBUG_KMS("flush pipe %c (pass %d)\n", pipe_name(pipe), pass);
> -
> -	for_each_plane(dev_priv, pipe, plane) {
> -		I915_WRITE(PLANE_SURF(pipe, plane),
> -			   I915_READ(PLANE_SURF(pipe, plane)));
> -	}
> -	I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
> -}
> -
> -static bool
> -skl_ddb_allocation_included(const struct skl_ddb_allocation *old,
> -			    const struct skl_ddb_allocation *new,
> -			    enum pipe pipe)
> +bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
> +			       const struct skl_ddb_allocation *new,
> +			       enum pipe pipe)
>  {
> -	uint16_t old_size, new_size;
> -
> -	old_size = skl_ddb_entry_size(&old->pipe[pipe]);
> -	new_size = skl_ddb_entry_size(&new->pipe[pipe]);
> -
> -	return old_size != new_size &&
> -	       new->pipe[pipe].start >= old->pipe[pipe].start &&
> -	       new->pipe[pipe].end <= old->pipe[pipe].end;
> +	return new->pipe[pipe].start == old->pipe[pipe].start &&
> +	       new->pipe[pipe].end == old->pipe[pipe].end;
>  }
>  
> -static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
> -				struct skl_wm_values *new_values)
> +bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> +				 const struct skl_ddb_allocation *old,
> +				 const struct skl_ddb_allocation *new,
> +				 enum pipe pipe)
>  {
> -	struct drm_device *dev = &dev_priv->drm;
> -	struct skl_ddb_allocation *cur_ddb, *new_ddb;
> -	bool reallocated[I915_MAX_PIPES] = {};
> -	struct intel_crtc *crtc;
> -	enum pipe pipe;
> -
> -	new_ddb = &new_values->ddb;
> -	cur_ddb = &dev_priv->wm.skl_hw.ddb;
> -
> -	/*
> -	 * First pass: flush the pipes with the new allocation contained into
> -	 * the old space.
> -	 *
> -	 * We'll wait for the vblank on those pipes to ensure we can safely
> -	 * re-allocate the freed space without this pipe fetching from it.
> -	 */
> -	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> -			continue;
> -
> -		pipe = crtc->pipe;
> -
> -		if (!skl_ddb_allocation_included(cur_ddb, new_ddb, pipe))
> -			continue;
> -
> -		skl_wm_flush_pipe(dev_priv, pipe, 1);
> -		intel_wait_for_vblank(dev, pipe);
> -
> -		reallocated[pipe] = true;
> -	}
> -
> -
> -	/*
> -	 * Second pass: flush the pipes that are having their allocation
> -	 * reduced, but overlapping with a previous allocation.
> -	 *
> -	 * Here as well we need to wait for the vblank to make sure the freed
> -	 * space is not used anymore.
> -	 */
> -	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> -			continue;
> -
> -		pipe = crtc->pipe;
> -
> -		if (reallocated[pipe])
> -			continue;
> -
> -		if (skl_ddb_entry_size(&new_ddb->pipe[pipe]) <
> -		    skl_ddb_entry_size(&cur_ddb->pipe[pipe])) {
> -			skl_wm_flush_pipe(dev_priv, pipe, 2);
> -			intel_wait_for_vblank(dev, pipe);
> -			reallocated[pipe] = true;
> -		}
> -	}
> -
> -	/*
> -	 * Third pass: flush the pipes that got more space allocated.
> -	 *
> -	 * We don't need to actively wait for the update here, next vblank
> -	 * will just get more DDB space with the correct WM values.
> -	 */
> -	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> -			continue;
> +	struct drm_device *dev = state->dev;
> +	struct intel_crtc *intel_crtc;
> +	enum pipe otherp;
>  
> -		pipe = crtc->pipe;
> +	for_each_intel_crtc(dev, intel_crtc) {
> +		otherp = intel_crtc->pipe;
>  
>  		/*
> -		 * At this point, only the pipes more space than before are
> -		 * left to re-allocate.
> +		 * When checking for overlaps, we don't want to:
> +		 *  - Compare against ourselves
> +		 *  - Compare against pipes that will be/are disabled
> +		 *  - Compare against pipes that aren't enabled yet
>  		 */
> -		if (reallocated[pipe])
> +		if (otherp == pipe || !new->pipe[otherp].end ||
> +		    !old->pipe[otherp].end)
>  			continue;

Arent we setting start=end=0 while the ddb entry is deallocated (while
a pipe is disabled)? If so we shouldn't need to check for empty entry
explicitly. Hmm. maybe we're not. I think we should since that would
make things simpler.

>  
> -		skl_wm_flush_pipe(dev_priv, pipe, 3);
> +		if ((new->pipe[pipe].start >= old->pipe[otherp].start &&
> +		     new->pipe[pipe].start < old->pipe[otherp].end) ||
> +		    (old->pipe[otherp].start >= new->pipe[pipe].start &&
> +		     old->pipe[otherp].start < new->pipe[pipe].end))

I'd extract this to a small helper, and simplify a bit while at it.
So perhaps:

skl_ddb_entries_overlap()
{
	return a->start < b->end && b->start < a->end;
}

> +			return true;
>  	}
> +
> +	return false;
>  }
>  
>  static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
> @@ -4160,7 +4044,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
>  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> -	int pipe;
> +	enum pipe pipe = intel_crtc->pipe;
>  
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
>  		return;
> @@ -4169,15 +4053,22 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  
>  	mutex_lock(&dev_priv->wm.wm_mutex);
>  
> -	skl_write_wm_values(dev_priv, results);
> -	skl_flush_wm_values(dev_priv, results);
> -
>  	/*
> -	 * Store the new configuration (but only for the pipes that have
> -	 * changed; the other values weren't recomputed).
> +	 * If this pipe isn't active already, we're going to be enabling it
> +	 * very soon. Since it's safe to update a pipe's ddb allocation while
> +	 * the pipe's shut off, just do so here. Already active pipes will have
> +	 * their watermarks updated once we update their planes.
>  	 */
> -	for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes)
> -		skl_copy_wm_for_pipe(hw_vals, results, pipe);
> +	if (crtc->state->active_changed) {
> +		int plane;
> +
> +		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
> +			skl_write_plane_wm(intel_crtc, results, plane);
> +
> +		skl_write_cursor_wm(intel_crtc, results);
> +	}
> +
> +	skl_copy_wm_for_pipe(hw_vals, results, pipe);

Hmm. So I'm thinking that if we make the ddb allocation match the
actual pipe state at all time, the algorithm will be simpler. So if any
disabled pipe (even ones disabled temporarily for modeset) will have
their current ddb entry zeroed, the main update loop can handle
everything for us, even enabling new pipes. Well, it won't handle
disabling pipes, but that's fine.

>  
>  	mutex_unlock(&dev_priv->wm.wm_mutex);
>  }
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v11 7/7] drm/i915/skl: Update DDB values atomically with wms/plane attrs

[Ville Syrjälä]

On Thu, Aug 11, 2016 at 03:54:36PM -0400, Lyude wrote:
> Now that we can hook into update_crtcs and control the order in which we
> update CRTCs at each modeset, we can finish the final step of fixing
> Skylake's watermark handling by performing DDB updates at the same time
> as plane updates and watermark updates.
> 
> The first major change in this patch is skl_update_crtcs(), which
> handles ensuring that we order each CRTC update in our atomic commits
> properly so that they honor the DDB flush order.
> 
> The second major change in this patch is the order in which we flush the
> pipes. While the previous order may have worked, it can't be used in
> this approach since it no longer will do the right thing. For example,
> using the old ddb flush order:
> 
> We have pipes A, B, and C enabled, and we're disabling C. Initial ddb
> allocation looks like this:
> 
> |   A   |   B   |xxxxxxx|
> 
> Since we're performing the ddb updates after performing any CRTC
> disablements in intel_atomic_commit_tail(), the space to the right of
> pipe B is unallocated.
> 
> 1. Flush pipes with new allocation contained into old space. None
>    apply, so we skip this
> 2. Flush pipes having their allocation reduced, but overlapping with a
>    previous allocation. None apply, so we also skip this
> 3. Flush pipes that got more space allocated. This applies to A and B,
>    giving us the following update order: A, B
> 
> This is wrong, since updating pipe A first will cause it to overlap with
> B and potentially burst into flames. Our new order (see the code
> comments for details) would update the pipes in the proper order: B, A.
> 
> As well, we calculate the order for each DDB update during the check
> phase, and reference it later in the commit phase when we hit
> skl_update_crtcs().
> 
> This long overdue patch fixes the rest of the underruns on Skylake.
> 
> Changes since v1:
>  - Add skl_ddb_entry_write() for cursor into skl_write_cursor_wm()
> Changes since v2:
>  - Use the method for updating CRTCs that Ville suggested
>  - In skl_update_wm(), only copy the watermarks for the crtc that was
>    passed to us
> Changes since v3:
>  - Small comment fix in skl_ddb_allocation_overlaps()
> 
> Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration")
> Fixes: 8211bd5bdf5e ("drm/i915/skl: Program the DDB allocation")
> [omitting CC for stable, since this patch will need to be changed for
> such backports first]
> 
> Testcase: kms_cursor_legacy
> Signed-off-by: Lyude <cpaul@redhat.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 100 +++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h     |   7 ++
>  drivers/gpu/drm/i915/intel_pm.c      | 207 +++++++++--------------------------
>  3 files changed, 144 insertions(+), 170 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 61a45f1..68fdbf0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13348,16 +13348,23 @@ static void verify_wm_state(struct drm_crtc *crtc,
>  			  hw_entry->start, hw_entry->end);
>  	}
>  
> -	/* cursor */
> -	hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> -	sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
> -
> -	if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
> -		DRM_ERROR("mismatch in DDB state pipe %c cursor "
> -			  "(expected (%u,%u), found (%u,%u))\n",
> -			  pipe_name(pipe),
> -			  sw_entry->start, sw_entry->end,
> -			  hw_entry->start, hw_entry->end);
> +	/*
> +	 * cursor
> +	 * If the cursor plane isn't active, we may not have updated it's ddb
> +	 * allocation. In that case since the ddb allocation will be updated
> +	 * once the plane becomes visible, we can skip this check
> +	 */
> +	if (intel_crtc->cursor_addr) {
> +		hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR];
> +		sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR];
> +
> +		if (!skl_ddb_entry_equal(hw_entry, sw_entry)) {
> +			DRM_ERROR("mismatch in DDB state pipe %c cursor "
> +				  "(expected (%u,%u), found (%u,%u))\n",
> +				  pipe_name(pipe),
> +				  sw_entry->start, sw_entry->end,
> +				  hw_entry->start, hw_entry->end);
> +		}
>  	}
>  }
>  
> @@ -14109,6 +14116,72 @@ static void intel_update_crtcs(struct drm_atomic_state *state,
>  	}
>  }
>  
> +static void skl_update_crtcs(struct drm_atomic_state *state,
> +			     unsigned int *crtc_vblank_mask)
> +{
> +	struct drm_device *dev = state->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *old_crtc_state;
> +	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
> +	struct skl_ddb_allocation cur_ddb;
> +	bool progress;
> +	bool reallocated[I915_MAX_PIPES] = {};
> +	enum pipe pipe;
> +	int wait_vbl_pipes, i;
> +
> +	/*
> +	 * Whenever the number of active pipes change, so does the DDB
> +	 * allocation. DDB allocations on pipes cannot ever overlap with
> +	 * eachother at any point in time, so we need to change the order we
> +	 * update the pipes so that we ensure they never overlap inbetween DDB
> +	 * updates.
> +	 */
> +	do {
> +		progress = false;
> +		wait_vbl_pipes = 0;
> +		cur_ddb = dev_priv->wm.skl_hw.ddb;

I guess would could do the loop twice to avoid the copy. But not sure
that's really any more efficient.

> +
> +		for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> +			struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +			pipe = intel_crtc->pipe;
> +
> +			if (!intel_crtc->active || needs_modeset(crtc->state))
> +				continue;

crtc->state->active?

> +			if (skl_ddb_allocation_equals(&cur_ddb, new_ddb, pipe))
> +				continue;
> +			if (skl_ddb_allocation_overlaps(state, &cur_ddb,
> +							new_ddb, pipe))
> +				continue;
> +
> +			intel_update_crtc(crtc, state, old_crtc_state,
> +					  crtc_vblank_mask);

What did the caller want with the vblank_mask? Do more vblank waits?
Shouldn't be needed I think since we wait here, no?

> +
> +			wait_vbl_pipes |= drm_crtc_mask(crtc);
> +			reallocated[pipe] = true;
> +			progress = true;

I guess we could throw this out actully. wait_vbl_pipes!=0 should mean
the same thing.

> +		}
> +
> +		/* Wait for each pipe's new allocation to take effect */
> +		intel_atomic_wait_for_vblanks(dev, dev_priv, wait_vbl_pipes);
> +	} while (progress);
> +
> +	/*
> +	 * Now that we've handled any ddb reallocations, we can go ahead and
> +	 * enable any new pipes.
> +	 */
> +	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> +		pipe = to_intel_crtc(crtc)->pipe;
> +
> +		if (reallocated[pipe] || !crtc->state->active)
> +			continue;

Do we need this reallocated[] flag? Isn't this the same as
'active && needs_modeset' ?

> +
> +		intel_update_crtc(crtc, state, old_crtc_state,
> +				  crtc_vblank_mask);
> +	}
> +}
> +
>  static void intel_atomic_commit_tail(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
> @@ -15738,8 +15811,6 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>  		dev_priv->display.crtc_disable = i9xx_crtc_disable;
>  	}
>  
> -	dev_priv->display.update_crtcs = intel_update_crtcs;
> -
>  	/* Returns the core display clock speed */
>  	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
>  		dev_priv->display.get_display_clock_speed =
> @@ -15829,6 +15900,11 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv)
>  			skl_modeset_calc_cdclk;
>  	}
>  
> +	if (dev_priv->info.gen >= 9)
> +		dev_priv->display.update_crtcs = skl_update_crtcs;
> +	else
> +		dev_priv->display.update_crtcs = intel_update_crtcs;
> +
>  	switch (INTEL_INFO(dev_priv)->gen) {
>  	case 2:
>  		dev_priv->display.queue_flip = intel_gen2_queue_flip;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 88088c3..9f9fe69 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1723,6 +1723,13 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */);
>  int skl_enable_sagv(struct drm_i915_private *dev_priv);
>  int skl_disable_sagv(struct drm_i915_private *dev_priv);
> +bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
> +			       const struct skl_ddb_allocation *new,
> +			       enum pipe pipe);
> +bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> +				 const struct skl_ddb_allocation *old,
> +				 const struct skl_ddb_allocation *new,
> +				 enum pipe pipe);
>  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
>  			 const struct skl_wm_values *wm);
>  void skl_write_plane_wm(struct intel_crtc *intel_crtc,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f2e0071..6fe9e57 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3794,6 +3794,11 @@ void skl_write_plane_wm(struct intel_crtc *intel_crtc,
>  			   wm->plane[pipe][plane][level]);
>  	}
>  	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
> +
> +	skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane),
> +			    &wm->ddb.plane[pipe][plane]);
> +	skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, plane),
> +			    &wm->ddb.y_plane[pipe][plane]);
>  }
>  
>  void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> @@ -3810,170 +3815,49 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc,
>  			   wm->plane[pipe][PLANE_CURSOR][level]);
>  	}
>  	I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
> -}
> -
> -static void skl_write_wm_values(struct drm_i915_private *dev_priv,
> -				const struct skl_wm_values *new)
> -{
> -	struct drm_device *dev = &dev_priv->drm;
> -	struct intel_crtc *crtc;
>  
> -	for_each_intel_crtc(dev, crtc) {
> -		int i;
> -		enum pipe pipe = crtc->pipe;
> -
> -		if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
> -			continue;
> -		if (!crtc->active)
> -			continue;
> -
> -		for (i = 0; i < intel_num_planes(crtc); i++) {
> -			skl_ddb_entry_write(dev_priv,
> -					    PLANE_BUF_CFG(pipe, i),
> -					    &new->ddb.plane[pipe][i]);
> -			skl_ddb_entry_write(dev_priv,
> -					    PLANE_NV12_BUF_CFG(pipe, i),
> -					    &new->ddb.y_plane[pipe][i]);
> -		}
> -
> -		skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> -				    &new->ddb.plane[pipe][PLANE_CURSOR]);
> -	}
> +	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> +			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
>  }
>  
> -/*
> - * When setting up a new DDB allocation arrangement, we need to correctly
> - * sequence the times at which the new allocations for the pipes are taken into
> - * account or we'll have pipes fetching from space previously allocated to
> - * another pipe.
> - *
> - * Roughly the sequence looks like:
> - *  1. re-allocate the pipe(s) with the allocation being reduced and not
> - *     overlapping with a previous light-up pipe (another way to put it is:
> - *     pipes with their new allocation strickly included into their old ones).
> - *  2. re-allocate the other pipes that get their allocation reduced
> - *  3. allocate the pipes having their allocation increased
> - *
> - * Steps 1. and 2. are here to take care of the following case:
> - * - Initially DDB looks like this:
> - *     |   B    |   C    |
> - * - enable pipe A.
> - * - pipe B has a reduced DDB allocation that overlaps with the old pipe C
> - *   allocation
> - *     |  A  |  B  |  C  |
> - *
> - * We need to sequence the re-allocation: C, B, A (and not B, C, A).
> - */
> -
> -static void
> -skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, int pass)
> -{
> -	int plane;
> -
> -	DRM_DEBUG_KMS("flush pipe %c (pass %d)\n", pipe_name(pipe), pass);
> -
> -	for_each_plane(dev_priv, pipe, plane) {
> -		I915_WRITE(PLANE_SURF(pipe, plane),
> -			   I915_READ(PLANE_SURF(pipe, plane)));
> -	}
> -	I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe)));
> -}
> -
> -static bool
> -skl_ddb_allocation_included(const struct skl_ddb_allocation *old,
> -			    const struct skl_ddb_allocation *new,
> -			    enum pipe pipe)
> +bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
> +			       const struct skl_ddb_allocation *new,
> +			       enum pipe pipe)
>  {
> -	uint16_t old_size, new_size;
> -
> -	old_size = skl_ddb_entry_size(&old->pipe[pipe]);
> -	new_size = skl_ddb_entry_size(&new->pipe[pipe]);
> -
> -	return old_size != new_size &&
> -	       new->pipe[pipe].start >= old->pipe[pipe].start &&
> -	       new->pipe[pipe].end <= old->pipe[pipe].end;
> +	return new->pipe[pipe].start == old->pipe[pipe].start &&
> +	       new->pipe[pipe].end == old->pipe[pipe].end;
>  }
>  
> -static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
> -				struct skl_wm_values *new_values)
> +bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> +				 const struct skl_ddb_allocation *old,
> +				 const struct skl_ddb_allocation *new,
> +				 enum pipe pipe)
>  {
> -	struct drm_device *dev = &dev_priv->drm;
> -	struct skl_ddb_allocation *cur_ddb, *new_ddb;
> -	bool reallocated[I915_MAX_PIPES] = {};
> -	struct intel_crtc *crtc;
> -	enum pipe pipe;
> -
> -	new_ddb = &new_values->ddb;
> -	cur_ddb = &dev_priv->wm.skl_hw.ddb;
> -
> -	/*
> -	 * First pass: flush the pipes with the new allocation contained into
> -	 * the old space.
> -	 *
> -	 * We'll wait for the vblank on those pipes to ensure we can safely
> -	 * re-allocate the freed space without this pipe fetching from it.
> -	 */
> -	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> -			continue;
> -
> -		pipe = crtc->pipe;
> -
> -		if (!skl_ddb_allocation_included(cur_ddb, new_ddb, pipe))
> -			continue;
> -
> -		skl_wm_flush_pipe(dev_priv, pipe, 1);
> -		intel_wait_for_vblank(dev, pipe);
> -
> -		reallocated[pipe] = true;
> -	}
> -
> -
> -	/*
> -	 * Second pass: flush the pipes that are having their allocation
> -	 * reduced, but overlapping with a previous allocation.
> -	 *
> -	 * Here as well we need to wait for the vblank to make sure the freed
> -	 * space is not used anymore.
> -	 */
> -	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> -			continue;
> -
> -		pipe = crtc->pipe;
> -
> -		if (reallocated[pipe])
> -			continue;
> -
> -		if (skl_ddb_entry_size(&new_ddb->pipe[pipe]) <
> -		    skl_ddb_entry_size(&cur_ddb->pipe[pipe])) {
> -			skl_wm_flush_pipe(dev_priv, pipe, 2);
> -			intel_wait_for_vblank(dev, pipe);
> -			reallocated[pipe] = true;
> -		}
> -	}
> -
> -	/*
> -	 * Third pass: flush the pipes that got more space allocated.
> -	 *
> -	 * We don't need to actively wait for the update here, next vblank
> -	 * will just get more DDB space with the correct WM values.
> -	 */
> -	for_each_intel_crtc(dev, crtc) {
> -		if (!crtc->active)
> -			continue;
> +	struct drm_device *dev = state->dev;
> +	struct intel_crtc *intel_crtc;
> +	enum pipe otherp;
>  
> -		pipe = crtc->pipe;
> +	for_each_intel_crtc(dev, intel_crtc) {
> +		otherp = intel_crtc->pipe;
>  
>  		/*
> -		 * At this point, only the pipes more space than before are
> -		 * left to re-allocate.
> +		 * When checking for overlaps, we don't want to:
> +		 *  - Compare against ourselves
> +		 *  - Compare against pipes that will be/are disabled
> +		 *  - Compare against pipes that aren't enabled yet
>  		 */
> -		if (reallocated[pipe])
> +		if (otherp == pipe || !new->pipe[otherp].end ||
> +		    !old->pipe[otherp].end)
>  			continue;

Arent we setting start=end=0 while the ddb entry is deallocated (while
a pipe is disabled)? If so we shouldn't need to check for empty entry
explicitly. Hmm. maybe we're not. I think we should since that would
make things simpler.

>  
> -		skl_wm_flush_pipe(dev_priv, pipe, 3);
> +		if ((new->pipe[pipe].start >= old->pipe[otherp].start &&
> +		     new->pipe[pipe].start < old->pipe[otherp].end) ||
> +		    (old->pipe[otherp].start >= new->pipe[pipe].start &&
> +		     old->pipe[otherp].start < new->pipe[pipe].end))

I'd extract this to a small helper, and simplify a bit while at it.
So perhaps:

skl_ddb_entries_overlap()
{
	return a->start < b->end && b->start < a->end;
}

> +			return true;
>  	}
> +
> +	return false;
>  }
>  
>  static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
> @@ -4160,7 +4044,7 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw;
>  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>  	struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal;
> -	int pipe;
> +	enum pipe pipe = intel_crtc->pipe;
>  
>  	if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
>  		return;
> @@ -4169,15 +4053,22 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  
>  	mutex_lock(&dev_priv->wm.wm_mutex);
>  
> -	skl_write_wm_values(dev_priv, results);
> -	skl_flush_wm_values(dev_priv, results);
> -
>  	/*
> -	 * Store the new configuration (but only for the pipes that have
> -	 * changed; the other values weren't recomputed).
> +	 * If this pipe isn't active already, we're going to be enabling it
> +	 * very soon. Since it's safe to update a pipe's ddb allocation while
> +	 * the pipe's shut off, just do so here. Already active pipes will have
> +	 * their watermarks updated once we update their planes.
>  	 */
> -	for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes)
> -		skl_copy_wm_for_pipe(hw_vals, results, pipe);
> +	if (crtc->state->active_changed) {
> +		int plane;
> +
> +		for (plane = 0; plane < intel_num_planes(intel_crtc); plane++)
> +			skl_write_plane_wm(intel_crtc, results, plane);
> +
> +		skl_write_cursor_wm(intel_crtc, results);
> +	}
> +
> +	skl_copy_wm_for_pipe(hw_vals, results, pipe);

Hmm. So I'm thinking that if we make the ddb allocation match the
actual pipe state at all time, the algorithm will be simpler. So if any
disabled pipe (even ones disabled temporarily for modeset) will have
their current ddb entry zeroed, the main update loop can handle
everything for us, even enabling new pipes. Well, it won't handle
disabling pipes, but that's fine.

>  
>  	mutex_unlock(&dev_priv->wm.wm_mutex);
>  }
> -- 
> 2.7.4

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