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

[PATCH v3] 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 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

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/i915_drv.h |   2 +
 drivers/gpu/drm/i915/i915_reg.h |   5 ++
 drivers/gpu/drm/i915/intel_pm.c | 110 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 03e1bfa..660d0a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1959,6 +1959,8 @@ struct drm_i915_private {
 	struct i915_suspend_saved_registers regfile;
 	struct vlv_s0ix_state vlv_s0ix_state;
 
+	bool skl_sagv_enabled;
+
 	struct {
 		/*
 		 * Raw watermark latency values:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8bfde75..9b2eb0b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7162,6 +7162,11 @@ 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_LOW_FREQ			0x1
+#define     GEN9_SAGV_HIGH_FREQ			0x2
+#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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5a8ee0c..948564a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2876,6 +2876,109 @@ skl_wm_plane_id(const struct intel_plane *plane)
 }
 
 static void
+skl_sagv_get_hw_state(struct drm_i915_private *dev_priv)
+{
+	u32 temp;
+	int ret;
+
+	if (IS_BROXTON(dev_priv))
+		return;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL, &temp);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	if (!ret) {
+		dev_priv->skl_sagv_enabled = !!(temp & GEN9_SAGV_DYNAMIC_FREQ);
+	} else {
+		/*
+		 * If for some reason we can't access the SAGV state, follow
+		 * the bspec and assume it's enabled
+		 */
+		DRM_ERROR("Failed to get SAGV state, assuming enabled\n");
+		dev_priv->skl_sagv_enabled = true;
+	}
+}
+
+/*
+ * 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.
+ */
+static int
+skl_enable_sagv(struct drm_i915_private *dev_priv)
+{
+	int ret;
+
+	if (IS_BROXTON(dev_priv))
+		return 0;
+	if (dev_priv->skl_sagv_enabled)
+		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);
+	if (!ret)
+		dev_priv->skl_sagv_enabled = true;
+	else
+		DRM_ERROR("Failed to enable the SAGV\n");
+
+	/* We don't need to wait for SAGV when enabling */
+	mutex_unlock(&dev_priv->rps.hw_lock);
+	return ret;
+}
+
+static int
+skl_disable_sagv(struct drm_i915_private *dev_priv)
+{
+	int ret = 0;
+	unsigned long timeout;
+	u32 temp;
+
+	if (IS_BROXTON(dev_priv))
+		return 0;
+	if (!dev_priv->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 */
+	timeout = jiffies + msecs_to_jiffies(1);
+	do {
+		ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+					      GEN9_SAGV_DISABLE);
+		if (ret) {
+			DRM_ERROR("Failed to disable the SAGV\n");
+			goto out;
+		}
+
+		ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+					     &temp);
+		if (ret) {
+			DRM_ERROR("Failed to check the status of the SAGV\n");
+			goto out;
+		}
+	} while (!(temp & 0x1) && jiffies < timeout);
+
+	if (temp & 0x1) {
+		dev_priv->skl_sagv_enabled = false;
+	} else {
+		ret = -1;
+		DRM_ERROR("Request to disable SAGV timed out\n");
+	}
+
+out:
+	mutex_unlock(&dev_priv->rps.hw_lock);
+	return ret;
+}
+
+static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				   const struct intel_crtc_state *cstate,
 				   struct skl_ddb_entry *alloc, /* out */
@@ -3686,6 +3789,11 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 	struct drm_device *dev = &dev_priv->drm;
 	struct intel_crtc *crtc;
 
+	if (dev_priv->active_crtcs == 1)
+		skl_enable_sagv(dev_priv);
+	else
+		skl_disable_sagv(dev_priv);
+
 	for_each_intel_crtc(dev, crtc) {
 		int i, level, max_level = ilk_wm_max_level(dev);
 		enum pipe pipe = crtc->pipe;
@@ -4228,6 +4336,8 @@ void skl_wm_get_hw_state(struct drm_device *dev)
 		/* Easy/common case; just sanitize DDB now if everything off */
 		memset(ddb, 0, sizeof(*ddb));
 	}
+
+	skl_sagv_get_hw_state(dev_priv);
 }
 
 static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
-- 
2.7.4

[PATCH v3] 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 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

Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/i915_drv.h |   2 +
 drivers/gpu/drm/i915/i915_reg.h |   5 ++
 drivers/gpu/drm/i915/intel_pm.c | 110 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 117 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 03e1bfa..660d0a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1959,6 +1959,8 @@ struct drm_i915_private {
 	struct i915_suspend_saved_registers regfile;
 	struct vlv_s0ix_state vlv_s0ix_state;
 
+	bool skl_sagv_enabled;
+
 	struct {
 		/*
 		 * Raw watermark latency values:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8bfde75..9b2eb0b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7162,6 +7162,11 @@ 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_LOW_FREQ			0x1
+#define     GEN9_SAGV_HIGH_FREQ			0x2
+#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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5a8ee0c..948564a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2876,6 +2876,109 @@ skl_wm_plane_id(const struct intel_plane *plane)
 }
 
 static void
+skl_sagv_get_hw_state(struct drm_i915_private *dev_priv)
+{
+	u32 temp;
+	int ret;
+
+	if (IS_BROXTON(dev_priv))
+		return;
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL, &temp);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	if (!ret) {
+		dev_priv->skl_sagv_enabled = !!(temp & GEN9_SAGV_DYNAMIC_FREQ);
+	} else {
+		/*
+		 * If for some reason we can't access the SAGV state, follow
+		 * the bspec and assume it's enabled
+		 */
+		DRM_ERROR("Failed to get SAGV state, assuming enabled\n");
+		dev_priv->skl_sagv_enabled = true;
+	}
+}
+
+/*
+ * 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.
+ */
+static int
+skl_enable_sagv(struct drm_i915_private *dev_priv)
+{
+	int ret;
+
+	if (IS_BROXTON(dev_priv))
+		return 0;
+	if (dev_priv->skl_sagv_enabled)
+		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);
+	if (!ret)
+		dev_priv->skl_sagv_enabled = true;
+	else
+		DRM_ERROR("Failed to enable the SAGV\n");
+
+	/* We don't need to wait for SAGV when enabling */
+	mutex_unlock(&dev_priv->rps.hw_lock);
+	return ret;
+}
+
+static int
+skl_disable_sagv(struct drm_i915_private *dev_priv)
+{
+	int ret = 0;
+	unsigned long timeout;
+	u32 temp;
+
+	if (IS_BROXTON(dev_priv))
+		return 0;
+	if (!dev_priv->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 */
+	timeout = jiffies + msecs_to_jiffies(1);
+	do {
+		ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+					      GEN9_SAGV_DISABLE);
+		if (ret) {
+			DRM_ERROR("Failed to disable the SAGV\n");
+			goto out;
+		}
+
+		ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+					     &temp);
+		if (ret) {
+			DRM_ERROR("Failed to check the status of the SAGV\n");
+			goto out;
+		}
+	} while (!(temp & 0x1) && jiffies < timeout);
+
+	if (temp & 0x1) {
+		dev_priv->skl_sagv_enabled = false;
+	} else {
+		ret = -1;
+		DRM_ERROR("Request to disable SAGV timed out\n");
+	}
+
+out:
+	mutex_unlock(&dev_priv->rps.hw_lock);
+	return ret;
+}
+
+static void
 skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
 				   const struct intel_crtc_state *cstate,
 				   struct skl_ddb_entry *alloc, /* out */
@@ -3686,6 +3789,11 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 	struct drm_device *dev = &dev_priv->drm;
 	struct intel_crtc *crtc;
 
+	if (dev_priv->active_crtcs == 1)
+		skl_enable_sagv(dev_priv);
+	else
+		skl_disable_sagv(dev_priv);
+
 	for_each_intel_crtc(dev, crtc) {
 		int i, level, max_level = ilk_wm_max_level(dev);
 		enum pipe pipe = crtc->pipe;
@@ -4228,6 +4336,8 @@ void skl_wm_get_hw_state(struct drm_device *dev)
 		/* Easy/common case; just sanitize DDB now if everything off */
 		memset(ddb, 0, sizeof(*ddb));
 	}
+
+	skl_sagv_get_hw_state(dev_priv);
 }
 
 static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
-- 
2.7.4

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

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

[Matt Roper]

On Tue, Jul 12, 2016 at 01:36:03PM -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 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
> 
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Lyude <cpaul@redhat.com>

I don't have a SKL to try this out on (only BXT here), but this matches
my interpretation of the current bspec text, so

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

I think this also applies to

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94625

And maybe a +cc stable?


Matt

> ---
>  drivers/gpu/drm/i915/i915_drv.h |   2 +
>  drivers/gpu/drm/i915/i915_reg.h |   5 ++
>  drivers/gpu/drm/i915/intel_pm.c | 110 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 117 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 03e1bfa..660d0a6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1959,6 +1959,8 @@ struct drm_i915_private {
>  	struct i915_suspend_saved_registers regfile;
>  	struct vlv_s0ix_state vlv_s0ix_state;
>  
> +	bool skl_sagv_enabled;
> +
>  	struct {
>  		/*
>  		 * Raw watermark latency values:
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8bfde75..9b2eb0b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7162,6 +7162,11 @@ 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_LOW_FREQ			0x1
> +#define     GEN9_SAGV_HIGH_FREQ			0x2
> +#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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 5a8ee0c..948564a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2876,6 +2876,109 @@ skl_wm_plane_id(const struct intel_plane *plane)
>  }
>  
>  static void
> +skl_sagv_get_hw_state(struct drm_i915_private *dev_priv)
> +{
> +	u32 temp;
> +	int ret;
> +
> +	if (IS_BROXTON(dev_priv))
> +		return;
> +
> +	mutex_lock(&dev_priv->rps.hw_lock);
> +	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL, &temp);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (!ret) {
> +		dev_priv->skl_sagv_enabled = !!(temp & GEN9_SAGV_DYNAMIC_FREQ);
> +	} else {
> +		/*
> +		 * If for some reason we can't access the SAGV state, follow
> +		 * the bspec and assume it's enabled
> +		 */
> +		DRM_ERROR("Failed to get SAGV state, assuming enabled\n");
> +		dev_priv->skl_sagv_enabled = true;
> +	}
> +}
> +
> +/*
> + * 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.
> + */
> +static int
> +skl_enable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +
> +	if (IS_BROXTON(dev_priv))
> +		return 0;
> +	if (dev_priv->skl_sagv_enabled)
> +		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);
> +	if (!ret)
> +		dev_priv->skl_sagv_enabled = true;
> +	else
> +		DRM_ERROR("Failed to enable the SAGV\n");
> +
> +	/* We don't need to wait for SAGV when enabling */
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +	return ret;
> +}
> +
> +static int
> +skl_disable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret = 0;
> +	unsigned long timeout;
> +	u32 temp;
> +
> +	if (IS_BROXTON(dev_priv))
> +		return 0;
> +	if (!dev_priv->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 */
> +	timeout = jiffies + msecs_to_jiffies(1);
> +	do {
> +		ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +					      GEN9_SAGV_DISABLE);
> +		if (ret) {
> +			DRM_ERROR("Failed to disable the SAGV\n");
> +			goto out;
> +		}
> +
> +		ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +					     &temp);
> +		if (ret) {
> +			DRM_ERROR("Failed to check the status of the SAGV\n");
> +			goto out;
> +		}
> +	} while (!(temp & 0x1) && jiffies < timeout);
> +
> +	if (temp & 0x1) {
> +		dev_priv->skl_sagv_enabled = false;
> +	} else {
> +		ret = -1;
> +		DRM_ERROR("Request to disable SAGV timed out\n");
> +	}
> +
> +out:
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +	return ret;
> +}
> +
> +static void
>  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>  				   const struct intel_crtc_state *cstate,
>  				   struct skl_ddb_entry *alloc, /* out */
> @@ -3686,6 +3789,11 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  	struct drm_device *dev = &dev_priv->drm;
>  	struct intel_crtc *crtc;
>  
> +	if (dev_priv->active_crtcs == 1)
> +		skl_enable_sagv(dev_priv);
> +	else
> +		skl_disable_sagv(dev_priv);
> +
>  	for_each_intel_crtc(dev, crtc) {
>  		int i, level, max_level = ilk_wm_max_level(dev);
>  		enum pipe pipe = crtc->pipe;
> @@ -4228,6 +4336,8 @@ void skl_wm_get_hw_state(struct drm_device *dev)
>  		/* Easy/common case; just sanitize DDB now if everything off */
>  		memset(ddb, 0, sizeof(*ddb));
>  	}
> +
> +	skl_sagv_get_hw_state(dev_priv);
>  }
>  
>  static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> -- 
> 2.7.4
> 

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

✗ Ro.CI.BAT: failure for drm/i915/skl: Add support for the SAGV, fix underrun hangs (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/skl: Add support for the SAGV, fix underrun hangs (rev3)
URL   : https://patchwork.freedesktop.org/series/9736/
State : failure

== Summary ==

Series 9736v3 drm/i915/skl: Add support for the SAGV, fix underrun hangs
http://patchwork.freedesktop.org/api/1.0/series/9736/revisions/3/mbox

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (ro-hsw-i7-4770r)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> SKIP       (ro-bdw-i7-5557U)

ro-bdw-i5-5250u  total:237  pass:213  dwarn:1   dfail:0   fail:7   skip:16 
ro-bdw-i7-5557U  total:237  pass:213  dwarn:1   dfail:0   fail:7   skip:16 
ro-bdw-i7-5600u  total:237  pass:199  dwarn:0   dfail:0   fail:7   skip:31 
ro-bsw-n3050     total:218  pass:172  dwarn:0   dfail:0   fail:3   skip:42 
ro-byt-n2820     total:237  pass:191  dwarn:0   dfail:0   fail:8   skip:38 
ro-hsw-i3-4010u  total:237  pass:206  dwarn:0   dfail:0   fail:7   skip:24 
ro-hsw-i7-4770r  total:237  pass:205  dwarn:0   dfail:0   fail:8   skip:24 
ro-ilk-i7-620lm  total:237  pass:166  dwarn:0   dfail:0   fail:8   skip:63 
ro-ilk1-i5-650   total:232  pass:166  dwarn:0   dfail:0   fail:8   skip:58 
ro-ivb-i7-3770   total:237  pass:197  dwarn:0   dfail:0   fail:7   skip:33 
ro-skl3-i5-6260u total:237  pass:217  dwarn:1   dfail:0   fail:7   skip:12 
ro-snb-i7-2620M  total:237  pass:188  dwarn:0   dfail:0   fail:8   skip:41 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1478/

9561f5c drm-intel-nightly: 2016y-07m-12d-15h-14m-43s UTC integration manifest
964a49a drm/i915/skl: Add support for the SAGV, fix underrun hangs

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

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

[Matt Roper]

On Tue, Jul 12, 2016 at 11:21:39AM -0700, Matt Roper wrote:
> On Tue, Jul 12, 2016 at 01:36:03PM -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 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
> > 
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Lyude <cpaul@redhat.com>
> 
> I don't have a SKL to try this out on (only BXT here), but this matches
> my interpretation of the current bspec text, so
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> I think this also applies to
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94625
> 
> And maybe a +cc stable?
> 

Oops, one more minor issue below that I missed the first time around.

...
> > +	/* bspec says to keep retrying for at least 1 ms */
> > +	timeout = jiffies + msecs_to_jiffies(1);
> > +	do {
> > +		ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> > +					      GEN9_SAGV_DISABLE);
> > +		if (ret) {
> > +			DRM_ERROR("Failed to disable the SAGV\n");
> > +			goto out;
> > +		}
> > +
> > +		ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> > +					     &temp);
> > +		if (ret) {
> > +			DRM_ERROR("Failed to check the status of the SAGV\n");
> > +			goto out;
> > +		}
> > +	} while (!(temp & 0x1) && jiffies < timeout);

We shouldn't compare jiffies directly here due to wraparound; you can
use time_before() to handle that safely.


Matt

> > +
> > +	if (temp & 0x1) {
> > +		dev_priv->skl_sagv_enabled = false;
> > +	} else {
> > +		ret = -1;
> > +		DRM_ERROR("Request to disable SAGV timed out\n");
> > +	}
> > +
> > +out:
> > +	mutex_unlock(&dev_priv->rps.hw_lock);
> > +	return ret;
> > +}
> > +
> > +static void
> >  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> >  				   const struct intel_crtc_state *cstate,
> >  				   struct skl_ddb_entry *alloc, /* out */
> > @@ -3686,6 +3789,11 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
> >  	struct drm_device *dev = &dev_priv->drm;
> >  	struct intel_crtc *crtc;
> >  
> > +	if (dev_priv->active_crtcs == 1)
> > +		skl_enable_sagv(dev_priv);
> > +	else
> > +		skl_disable_sagv(dev_priv);
> > +
> >  	for_each_intel_crtc(dev, crtc) {
> >  		int i, level, max_level = ilk_wm_max_level(dev);
> >  		enum pipe pipe = crtc->pipe;
> > @@ -4228,6 +4336,8 @@ void skl_wm_get_hw_state(struct drm_device *dev)
> >  		/* Easy/common case; just sanitize DDB now if everything off */
> >  		memset(ddb, 0, sizeof(*ddb));
> >  	}
> > +
> > +	skl_sagv_get_hw_state(dev_priv);
> >  }
> >  
> >  static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Ville Syrjälä]

On Wed, Jul 13, 2016 at 11:08:46AM -0700, Matt Roper wrote:
> On Tue, Jul 12, 2016 at 11:21:39AM -0700, Matt Roper wrote:
> > On Tue, Jul 12, 2016 at 01:36:03PM -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 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
> > > 
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Lyude <cpaul@redhat.com>
> > 
> > I don't have a SKL to try this out on (only BXT here), but this matches
> > my interpretation of the current bspec text, so
> > 
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > I think this also applies to
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94625
> > 
> > And maybe a +cc stable?
> > 
> 
> Oops, one more minor issue below that I missed the first time around.
> 
> ...
> > > +	/* bspec says to keep retrying for at least 1 ms */
> > > +	timeout = jiffies + msecs_to_jiffies(1);
> > > +	do {
> > > +		ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> > > +					      GEN9_SAGV_DISABLE);
> > > +		if (ret) {
> > > +			DRM_ERROR("Failed to disable the SAGV\n");
> > > +			goto out;
> > > +		}
> > > +
> > > +		ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> > > +					     &temp);
> > > +		if (ret) {
> > > +			DRM_ERROR("Failed to check the status of the SAGV\n");
> > > +			goto out;
> > > +		}
> > > +	} while (!(temp & 0x1) && jiffies < timeout);
> 
> We shouldn't compare jiffies directly here due to wraparound; you can
> use time_before() to handle that safely.

We have wait_for()/_wait_for() for polling stuff.

> 
> 
> Matt
> 
> > > +
> > > +	if (temp & 0x1) {
> > > +		dev_priv->skl_sagv_enabled = false;
> > > +	} else {
> > > +		ret = -1;
> > > +		DRM_ERROR("Request to disable SAGV timed out\n");
> > > +	}
> > > +
> > > +out:
> > > +	mutex_unlock(&dev_priv->rps.hw_lock);
> > > +	return ret;
> > > +}
> > > +
> > > +static void
> > >  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> > >  				   const struct intel_crtc_state *cstate,
> > >  				   struct skl_ddb_entry *alloc, /* out */
> > > @@ -3686,6 +3789,11 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
> > >  	struct drm_device *dev = &dev_priv->drm;
> > >  	struct intel_crtc *crtc;
> > >  
> > > +	if (dev_priv->active_crtcs == 1)
> > > +		skl_enable_sagv(dev_priv);
> > > +	else
> > > +		skl_disable_sagv(dev_priv);
> > > +
> > >  	for_each_intel_crtc(dev, crtc) {
> > >  		int i, level, max_level = ilk_wm_max_level(dev);
> > >  		enum pipe pipe = crtc->pipe;
> > > @@ -4228,6 +4336,8 @@ void skl_wm_get_hw_state(struct drm_device *dev)
> > >  		/* Easy/common case; just sanitize DDB now if everything off */
> > >  		memset(ddb, 0, sizeof(*ddb));
> > >  	}
> > > +
> > > +	skl_sagv_get_hw_state(dev_priv);
> > >  }
> > >  
> > >  static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Matt Roper]

On Wed, Jul 13, 2016 at 09:12:09PM +0300, Ville Syrjälä wrote:
> On Wed, Jul 13, 2016 at 11:08:46AM -0700, Matt Roper wrote:
> > On Tue, Jul 12, 2016 at 11:21:39AM -0700, Matt Roper wrote:
> > > On Tue, Jul 12, 2016 at 01:36:03PM -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 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
> > > > 
> > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Lyude <cpaul@redhat.com>
> > > 
> > > I don't have a SKL to try this out on (only BXT here), but this matches
> > > my interpretation of the current bspec text, so
> > > 
> > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > > 
> > > I think this also applies to
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94625
> > > 
> > > And maybe a +cc stable?
> > > 
> > 
> > Oops, one more minor issue below that I missed the first time around.
> > 
> > ...
> > > > +	/* bspec says to keep retrying for at least 1 ms */
> > > > +	timeout = jiffies + msecs_to_jiffies(1);
> > > > +	do {
> > > > +		ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> > > > +					      GEN9_SAGV_DISABLE);
> > > > +		if (ret) {
> > > > +			DRM_ERROR("Failed to disable the SAGV\n");
> > > > +			goto out;
> > > > +		}
> > > > +
> > > > +		ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> > > > +					     &temp);
> > > > +		if (ret) {
> > > > +			DRM_ERROR("Failed to check the status of the SAGV\n");
> > > > +			goto out;
> > > > +		}
> > > > +	} while (!(temp & 0x1) && jiffies < timeout);
> > 
> > We shouldn't compare jiffies directly here due to wraparound; you can
> > use time_before() to handle that safely.
> 
> We have wait_for()/_wait_for() for polling stuff.

Those just block until a condition becomes true, right?  In this case my
understanding from the bspec is that we need to keep re-writing the SAGV
disable until it sticks.


Matt

> 
> > 
> > 
> > Matt
> > 
> > > > +
> > > > +	if (temp & 0x1) {
> > > > +		dev_priv->skl_sagv_enabled = false;
> > > > +	} else {
> > > > +		ret = -1;
> > > > +		DRM_ERROR("Request to disable SAGV timed out\n");
> > > > +	}
> > > > +
> > > > +out:
> > > > +	mutex_unlock(&dev_priv->rps.hw_lock);
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static void
> > > >  skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> > > >  				   const struct intel_crtc_state *cstate,
> > > >  				   struct skl_ddb_entry *alloc, /* out */
> > > > @@ -3686,6 +3789,11 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
> > > >  	struct drm_device *dev = &dev_priv->drm;
> > > >  	struct intel_crtc *crtc;
> > > >  
> > > > +	if (dev_priv->active_crtcs == 1)
> > > > +		skl_enable_sagv(dev_priv);
> > > > +	else
> > > > +		skl_disable_sagv(dev_priv);
> > > > +
> > > >  	for_each_intel_crtc(dev, crtc) {
> > > >  		int i, level, max_level = ilk_wm_max_level(dev);
> > > >  		enum pipe pipe = crtc->pipe;
> > > > @@ -4228,6 +4336,8 @@ void skl_wm_get_hw_state(struct drm_device *dev)
> > > >  		/* Easy/common case; just sanitize DDB now if everything off */
> > > >  		memset(ddb, 0, sizeof(*ddb));
> > > >  	}
> > > > +
> > > > +	skl_sagv_get_hw_state(dev_priv);
> > > >  }
> > > >  
> > > >  static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc)
> > > > -- 
> > > > 2.7.4
> > > > 
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > IoTG Platform Enabling & Development
> > > Intel Corporation
> > > (916) 356-2795
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

Re: [Intel-gfx] [PATCH v3] drm/i915/skl: Add support for the SAGV, fix underrun hangs

[Chris Wilson]

On Wed, Jul 13, 2016 at 11:17:52AM -0700, Matt Roper wrote:
> On Wed, Jul 13, 2016 at 09:12:09PM +0300, Ville Syrjälä wrote:
> > We have wait_for()/_wait_for() for polling stuff.
> 
> Those just block until a condition becomes true, right?  In this case my
> understanding from the bspec is that we need to keep re-writing the SAGV
> disable until it sticks.

the condition is an arbitrary expression such as

static inline bool sagv_disabled(struct drm_i915_private (dev_priv)
{
	u32 tmp;

	if (sandybridge_pcode_write(dev_priv,
				    GEN9_PCODE_SAGV_CONTROL,
				    GEN9_SAGV_DISABLE))
		goto error;

	if (sandybridge_pcode_read(dev_priv,
				   GEN9_PCODE_SAGV_CONTROL,
				   &tmp))
		goto error;
	
	return tmp & 1;

error:
	DRM_ERROR("Failed to disable the SAGV\n");
	return true;
}


ret = wait_for(sagv_disabled(dev_priv), 1);
if (ret)
	DRM_ERROR("Timed out waiting for SAGV to be disabled\n");

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Oskar Berggren]

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

2016-07-12 19:21 GMT+01:00 Matt Roper <matthew.d.roper@intel.com>:

> On Tue, Jul 12, 2016 at 01:36:03PM -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 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
> >
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Lyude <cpaul@redhat.com>
>
> I don't have a SKL to try this out on (only BXT here), but this matches
> my interpretation of the current bspec text, so
>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>
> I think this also applies to
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94625
>
>
Yes, this does sound very similar to the problems I were having in that
report.
Vacation etc. so haven't found time to try the 4.6 kernel yet, and now I
will be
travelling until next week. If someone is easily able to prepare a kernel
for
Fedora 24 with this patch it's more likely I can find time to try it next
week (on
Dell XPS 9550 with 1 or 2 external monitors).

I should mention that there was a brief period in 4.4.4-4.4.9 or so that
seemed
to work fairly stable compared to both 4.3 and 4.5.

/Oskar

[-- Attachment #1.2: Type: text/html, Size: 4592 bytes --]

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

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