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

[PATCH v5 1/6] 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 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
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

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Lyude <cpaul@redhat.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      |   2 +
 drivers/gpu/drm/i915/i915_reg.h      |   5 ++
 drivers/gpu/drm/i915/intel_display.c |  12 ++++
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      | 112 +++++++++++++++++++++++++++++++++++
 5 files changed, 133 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 65ada5d..87018d3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1962,6 +1962,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 2f93d4a..5fb1c63 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7170,6 +7170,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_display.c b/drivers/gpu/drm/i915/intel_display.c
index a8e8cc8..76ba79f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4565,6 +4565,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_plane *primary = crtc->base.primary;
 	struct drm_plane_state *old_pri_state =
 		drm_atomic_get_existing_plane_state(old_state, primary);
@@ -4589,6 +4590,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 		     !old_primary_state->visible))
 			intel_post_enable_primary(&crtc->base);
 	}
+
+	if (hweight32(dev_priv->active_crtcs) <= 1)
+		skl_enable_sagv(dev_priv);
 }
 
 static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
@@ -4649,6 +4653,14 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 	}
 
 	/*
+	 * SKL workaround: bspec recommends we disable the SAGV when we have
+	 * more then one pipe enabled
+	 */
+	if (pipe_config->base.active &&
+	    hweight32(dev_priv->active_crtcs | drm_crtc_mask(&crtc->base)) > 1)
+		skl_disable_sagv(dev_priv);
+
+	/*
 	 * If we're doing a modeset, we're done.  No need to do any pre-vblank
 	 * watermark programming here.
 	 */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 50cdc89..6b0532a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1709,6 +1709,8 @@ void ilk_wm_get_hw_state(struct drm_device *dev);
 void skl_wm_get_hw_state(struct drm_device *dev);
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */);
+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 f610b71..68721a5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2884,6 +2884,116 @@ 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 & 0x1);
+	} 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.
+ */
+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_do_sagv_disable(struct drm_i915_private *dev_priv)
+{
+	int ret;
+	uint32_t temp;
+
+	ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+				      GEN9_SAGV_DISABLE);
+	if (ret) {
+		DRM_ERROR("Failed to disable the SAGV\n");
+		return ret;
+	}
+
+	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
+				     &temp);
+	if (ret) {
+		DRM_ERROR("Failed to check the status of the SAGV\n");
+		return ret;
+	}
+
+	return temp & 0x1;
+}
+
+int
+skl_disable_sagv(struct drm_i915_private *dev_priv)
+{
+	int ret, result;
+
+	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 */
+	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
+	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	if (ret == -ETIMEDOUT)
+		DRM_ERROR("Request to disable SAGV timed out\n");
+	else {
+		if (result == 1)
+			dev_priv->skl_sagv_enabled = false;
+
+		ret = result;
+	}
+
+	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 */
@@ -4236,6 +4346,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

Re: [v5,1/6] drm/i915/skl: Add support for the SAGV, fix underrun hangs

[Hans de Goede]

Hi,

On 08/02/2016 08:52 PM, cpaul@redhat.com 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 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
> 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
>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Lyude <cpaul@redhat.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      |   2 +
>  drivers/gpu/drm/i915/i915_reg.h      |   5 ++
>  drivers/gpu/drm/i915/intel_display.c |  12 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  drivers/gpu/drm/i915/intel_pm.c      | 112 +++++++++++++++++++++++++++++++++++
>  5 files changed, 133 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 65ada5d..87018d3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1962,6 +1962,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 2f93d4a..5fb1c63 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7170,6 +7170,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_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a8e8cc8..76ba79f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4565,6 +4565,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->base.state);
>  	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_plane *primary = crtc->base.primary;
>  	struct drm_plane_state *old_pri_state =
>  		drm_atomic_get_existing_plane_state(old_state, primary);
> @@ -4589,6 +4590,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  		     !old_primary_state->visible))
>  			intel_post_enable_primary(&crtc->base);
>  	}
> +
> +	if (hweight32(dev_priv->active_crtcs) <= 1)
> +		skl_enable_sagv(dev_priv);
>  }
>
>  static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
> @@ -4649,6 +4653,14 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
>  	}
>
>  	/*
> +	 * SKL workaround: bspec recommends we disable the SAGV when we have
> +	 * more then one pipe enabled
> +	 */
> +	if (pipe_config->base.active &&
> +	    hweight32(dev_priv->active_crtcs | drm_crtc_mask(&crtc->base)) > 1)
> +		skl_disable_sagv(dev_priv);
> +
> +	/*

Why | drm_crtc_mask? Also if it is a mask I would expect & not | >
Last why are you using drm_crtc_mask here and not in post_plane_update ?

Regards,

Hans




>  	 * If we're doing a modeset, we're done.  No need to do any pre-vblank
>  	 * watermark programming here.
>  	 */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 50cdc89..6b0532a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1709,6 +1709,8 @@ void ilk_wm_get_hw_state(struct drm_device *dev);
>  void skl_wm_get_hw_state(struct drm_device *dev);
>  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */);
> +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 f610b71..68721a5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2884,6 +2884,116 @@ 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 & 0x1);
> +	} 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.
> + */
> +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_do_sagv_disable(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +	uint32_t temp;
> +
> +	ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				      GEN9_SAGV_DISABLE);
> +	if (ret) {
> +		DRM_ERROR("Failed to disable the SAGV\n");
> +		return ret;
> +	}
> +
> +	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				     &temp);
> +	if (ret) {
> +		DRM_ERROR("Failed to check the status of the SAGV\n");
> +		return ret;
> +	}
> +
> +	return temp & 0x1;
> +}
> +
> +int
> +skl_disable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret, result;
> +
> +	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 */
> +	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (ret == -ETIMEDOUT)
> +		DRM_ERROR("Request to disable SAGV timed out\n");
> +	else {
> +		if (result == 1)
> +			dev_priv->skl_sagv_enabled = false;
> +
> +		ret = result;
> +	}
> +
> +	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 */
> @@ -4236,6 +4346,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)
>

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

[Matt Roper]

On Tue, Aug 02, 2016 at 02:52:49PM -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 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
> 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
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Lyude <cpaul@redhat.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      |   2 +
>  drivers/gpu/drm/i915/i915_reg.h      |   5 ++
>  drivers/gpu/drm/i915/intel_display.c |  12 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  drivers/gpu/drm/i915/intel_pm.c      | 112 +++++++++++++++++++++++++++++++++++
>  5 files changed, 133 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 65ada5d..87018d3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1962,6 +1962,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 2f93d4a..5fb1c63 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7170,6 +7170,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_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a8e8cc8..76ba79f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4565,6 +4565,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->base.state);
>  	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_plane *primary = crtc->base.primary;
>  	struct drm_plane_state *old_pri_state =
>  		drm_atomic_get_existing_plane_state(old_state, primary);
> @@ -4589,6 +4590,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  		     !old_primary_state->visible))
>  			intel_post_enable_primary(&crtc->base);
>  	}
> +
> +	if (hweight32(dev_priv->active_crtcs) <= 1)
> +		skl_enable_sagv(dev_priv);

There might be a slightly better place to handle this; see comment
below.

>  }
>  
>  static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
> @@ -4649,6 +4653,14 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
>  	}
>  
>  	/*
> +	 * SKL workaround: bspec recommends we disable the SAGV when we have
> +	 * more then one pipe enabled
> +	 */
> +	if (pipe_config->base.active &&
> +	    hweight32(dev_priv->active_crtcs | drm_crtc_mask(&crtc->base)) > 1)
> +		skl_disable_sagv(dev_priv);

As Hans pointed out, this doesn't look right.  I'm guessing you're
trying to guard against the case where there's no intersection between
the starting pipe usage and the final pipe usage?  E.g., only pipe A
active before commit, only pipe B active after commit, and you're
worried there might be a brief point where both A and B are on together?
I don't think this should really matter since we power everything
old-changing down before powering anything new-changing up.  So I think
all we really need to care about for enabling/disabling is how many
CRTC's there are in the final state.

It seems like a more natural place to place to handle enable/disable
would be in the 'if (intel_state->modeset)' blocks of
intel_atomic_commit_tail() (since only a modeset operation could change
the number of CRTC's in use to trigger a SAGV toggle).  That also has
the slight benefit of only getting run once for the atomic transaction
rather than once for each CRTC (and you can just do a simple test of
hweight(intel_state->active_crtcs) to figure out how many CRTC's are
going to be on after the transaction completes.


> +
> +	/*
>  	 * If we're doing a modeset, we're done.  No need to do any pre-vblank
>  	 * watermark programming here.
>  	 */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 50cdc89..6b0532a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1709,6 +1709,8 @@ void ilk_wm_get_hw_state(struct drm_device *dev);
>  void skl_wm_get_hw_state(struct drm_device *dev);
>  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */);
> +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 f610b71..68721a5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2884,6 +2884,116 @@ 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 & 0x1);
> +	} 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.
> + */
> +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_do_sagv_disable(struct drm_i915_private *dev_priv)
> +{
> +	int ret;
> +	uint32_t temp;
> +
> +	ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				      GEN9_SAGV_DISABLE);
> +	if (ret) {
> +		DRM_ERROR("Failed to disable the SAGV\n");
> +		return ret;
> +	}
> +
> +	ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL,
> +				     &temp);
> +	if (ret) {
> +		DRM_ERROR("Failed to check the status of the SAGV\n");
> +		return ret;
> +	}
> +
> +	return temp & 0x1;
> +}
> +
> +int
> +skl_disable_sagv(struct drm_i915_private *dev_priv)
> +{
> +	int ret, result;
> +
> +	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 */
> +	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
> +	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	if (ret == -ETIMEDOUT)
> +		DRM_ERROR("Request to disable SAGV timed out\n");
> +	else {

Minor style nitpick; if either branch of an if/else needs braces, they
both need braces.

> +		if (result == 1)

You've got a few uses of 0x1 as a magic number that are correct, but
seem counterintuitive to someone not looking at the bspec (1=off).
Maybe replacing these with a #define might help clarify slightly?


Matt

> +			dev_priv->skl_sagv_enabled = false;
> +
> +		ret = result;
> +	}
> +
> +	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 */
> @@ -4236,6 +4346,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

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

[Lyude]

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

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

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

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

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 68721a5..7fd299e 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4064,6 +4064,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)
 {
@@ -4136,8 +4154,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;
@@ -4149,8 +4169,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 v5 3/6] drm/i915/skl: Update plane watermarks atomically during plane updates

[Lyude]

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

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

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

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

  or

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

Now we update watermarks atomically like this:

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

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

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

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 76ba79f..79d146c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2980,6 +2980,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	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, stride_div, stride;
 	u32 tile_height, plane_offset, plane_size;
@@ -3031,6 +3032,9 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = x_offset;
 	intel_crtc->adjusted_y = y_offset;
 
+	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), plane_offset);
 	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
@@ -10243,9 +10247,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 (IS_GEN9(dev_priv) && wm->dirty_pipes & drm_crtc_mask(crtc))
+		skl_write_cursor_wm(intel_crtc, wm);
+
 	if (plane_state && plane_state->visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (plane_state->base.crtc_w) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6b0532a..1b444d3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1711,6 +1711,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 7fd299e..53adcbf 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3798,6 +3798,47 @@ 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;
+
+	if (!(wm->dirty_pipes & drm_crtc_mask(crtc)))
+		return;
+
+	I915_WRITE(PIPE_WM_LINETIME(pipe), wm->wm_linetime[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]);
+
+	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
+			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
+}
+
 static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 				const struct skl_wm_values *new)
 {
@@ -3805,7 +3846,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)
@@ -3813,21 +3854,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 0de935a..55d173f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -203,6 +203,9 @@ skl_update_plane(struct drm_plane *drm_plane,
 	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, stride_div, stride;
@@ -238,6 +241,9 @@ skl_update_plane(struct drm_plane *drm_plane,
 	crtc_w--;
 	crtc_h--;
 
+	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

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

[Matt Roper]

On Tue, Aug 02, 2016 at 02:52:51PM -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
> 
> Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: stable@vger.kernel.org
> Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  8 +++++
>  drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
>  drivers/gpu/drm/i915/intel_pm.c      | 58 ++++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++
>  4 files changed, 61 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 76ba79f..79d146c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2980,6 +2980,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	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, stride_div, stride;
>  	u32 tile_height, plane_offset, plane_size;
> @@ -3031,6 +3032,9 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	intel_crtc->adjusted_x = x_offset;
>  	intel_crtc->adjusted_y = y_offset;
>  
> +	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), plane_offset);
>  	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
> @@ -10243,9 +10247,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 (IS_GEN9(dev_priv) && wm->dirty_pipes & drm_crtc_mask(crtc))
> +		skl_write_cursor_wm(intel_crtc, wm);
> +
>  	if (plane_state && plane_state->visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
>  		switch (plane_state->base.crtc_w) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6b0532a..1b444d3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1711,6 +1711,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 7fd299e..53adcbf 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3798,6 +3798,47 @@ 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;
> +
> +	if (!(wm->dirty_pipes & drm_crtc_mask(crtc)))
> +		return;
> +
> +	I915_WRITE(PIPE_WM_LINETIME(pipe), wm->wm_linetime[pipe]);

I think I mentioned this on the previous version, but if we program this
here then it only gets programmed when a plane is being updated.  It's
possible that we could have the linetime value change due to a scaler
update or a modeset, but without corresponding plane updates (e.g.,
maybe all planes are off).  So we probably need to make sure this value
gets written outside the plane updates (but still under vblank evasion).

> +
> +	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]);

I notice that you moved the DDB update into skl_write_cursor_wm() down
below, but didn't move the plane DDB update in here.  I realize you
remedy that in patch #6 where you finally fix up the flushing order, but
the inconsistency between how cursor vs !cursor planes are handled seems
confusing.  Can we either move the plane DDB update here in this patch
(we don't flush properly yet, but we're really no worse off than we
already are), or can we defer the move of the cursor DDB to patch 6 to
keep them consistent?


Matt

> +}
> +
> +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]);
> +
> +	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> +			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
> +}
> +
>  static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  				const struct skl_wm_values *new)
>  {
> @@ -3805,7 +3846,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)
> @@ -3813,21 +3854,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 0de935a..55d173f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -203,6 +203,9 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	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, stride_div, stride;
> @@ -238,6 +241,9 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	crtc_w--;
>  	crtc_h--;
>  
> +	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
> 

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

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

[Matt Roper]

On Tue, Aug 02, 2016 at 02:20:33PM -0700, Matt Roper wrote:
> On Tue, Aug 02, 2016 at 02:52:51PM -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
> > 
> > Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
> > Signed-off-by: Lyude <cpaul@redhat.com>
> > Cc: stable@vger.kernel.org
> > Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>

I imagine we'll eventually probably want to create a new display vfunc
to handle platform-specific pipe-level stuff that needs to happen under
vblank evasion (like the scalers and linetime WM we have today) to keep
the code clean; maybe add a TODO comment to that effect?

Anyway, this looks good enough for now, so

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

> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  8 +++++
> >  drivers/gpu/drm/i915/intel_drv.h     |  5 ++++
> >  drivers/gpu/drm/i915/intel_pm.c      | 58 ++++++++++++++++++++++++++----------
> >  drivers/gpu/drm/i915/intel_sprite.c  |  6 ++++
> >  4 files changed, 61 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 76ba79f..79d146c 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2980,6 +2980,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
> >  	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, stride_div, stride;
> >  	u32 tile_height, plane_offset, plane_size;
> > @@ -3031,6 +3032,9 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
> >  	intel_crtc->adjusted_x = x_offset;
> >  	intel_crtc->adjusted_y = y_offset;
> >  
> > +	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), plane_offset);
> >  	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
> > @@ -10243,9 +10247,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 (IS_GEN9(dev_priv) && wm->dirty_pipes & drm_crtc_mask(crtc))
> > +		skl_write_cursor_wm(intel_crtc, wm);
> > +
> >  	if (plane_state && plane_state->visible) {
> >  		cntl = MCURSOR_GAMMA_ENABLE;
> >  		switch (plane_state->base.crtc_w) {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 6b0532a..1b444d3 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1711,6 +1711,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 7fd299e..53adcbf 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3798,6 +3798,47 @@ 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;
> > +
> > +	if (!(wm->dirty_pipes & drm_crtc_mask(crtc)))
> > +		return;
> > +
> > +	I915_WRITE(PIPE_WM_LINETIME(pipe), wm->wm_linetime[pipe]);
> 
> I think I mentioned this on the previous version, but if we program this
> here then it only gets programmed when a plane is being updated.  It's
> possible that we could have the linetime value change due to a scaler
> update or a modeset, but without corresponding plane updates (e.g.,
> maybe all planes are off).  So we probably need to make sure this value
> gets written outside the plane updates (but still under vblank evasion).
> 
> > +
> > +	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]);
> 
> I notice that you moved the DDB update into skl_write_cursor_wm() down
> below, but didn't move the plane DDB update in here.  I realize you
> remedy that in patch #6 where you finally fix up the flushing order, but
> the inconsistency between how cursor vs !cursor planes are handled seems
> confusing.  Can we either move the plane DDB update here in this patch
> (we don't flush properly yet, but we're really no worse off than we
> already are), or can we defer the move of the cursor DDB to patch 6 to
> keep them consistent?
> 
> 
> Matt
> 
> > +}
> > +
> > +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]);
> > +
> > +	skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
> > +			    &wm->ddb.plane[pipe][PLANE_CURSOR]);
> > +}
> > +
> >  static void skl_write_wm_values(struct drm_i915_private *dev_priv,
> >  				const struct skl_wm_values *new)
> >  {
> > @@ -3805,7 +3846,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)
> > @@ -3813,21 +3854,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 0de935a..55d173f 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -203,6 +203,9 @@ skl_update_plane(struct drm_plane *drm_plane,
> >  	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, stride_div, stride;
> > @@ -238,6 +241,9 @@ skl_update_plane(struct drm_plane *drm_plane,
> >  	crtc_w--;
> >  	crtc_h--;
> >  
> > +	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
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

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

[Matt Roper]

On Wed, Aug 03, 2016 at 02:14:53PM -0700, Matt Roper wrote:
...
> 
> I imagine we'll eventually probably want to create a new display vfunc
> to handle platform-specific pipe-level stuff that needs to happen under
> vblank evasion (like the scalers and linetime WM we have today) to keep
> the code clean; maybe add a TODO comment to that effect?
> 
> Anyway, this looks good enough for now, so
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

Woops, meant to add this reply to your v6 version of the patch
obviously...


Matt

[PATCH v5 4/6] 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
other active pipes. Since those pipes won't be added to the state on
their own, we need to add them ourselves.

Signed-off-by: Lyude <cpaul@redhat.com>
Cc: stable@vger.kernel.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 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 53adcbf..6b2452b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4085,6 +4085,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

Re: [PATCH v5 4/6] drm/i915/skl: Ensure pipes with changed wms get added to the state

[Matt Roper]

On Tue, Aug 02, 2016 at 02:52:52PM -0400, Lyude wrote:
> If we're enabling a pipe, we'll need to modify the watermarks on all
> other active pipes. Since those pipes won't be added to the state on
> their own, we need to add them ourselves.

All pipes (crtc's) are already added to the state if we have a change in
active pipes.  I think what you meant to write was "...we'll need to
modify the watermarks on all active *planes*.  Since those *planes*
won't..."

Aside from the commit message, I believe the logic is correct, so you
can consider this
        Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
once you reword it.


Matt

> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: stable@vger.kernel.org
> Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  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 53adcbf..6b2452b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4085,6 +4085,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
> 

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