[PATCH v4 0/6] Finally fix watermarks

[PATCH v4 0/6] Finally fix watermarks

[Lyude]

Latest version of https://lkml.org/lkml/2016/7/26/290 . Resending the whole
thing to keep it in one place.

Lyude (5):
  drm/i915/skl: Add support for the SAGV, fix underrun hangs
  drm/i915/skl: Only flush pipes when we change the ddb allocation
  drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()
  drm/i915/skl: Update plane watermarks atomically during plane updates
  drm/i915/skl: Always wait for pipes to update after a flush

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

 drivers/gpu/drm/i915/i915_drv.h      |   3 +
 drivers/gpu/drm/i915/i915_reg.h      |   5 +
 drivers/gpu/drm/i915/intel_display.c |  24 ++++
 drivers/gpu/drm/i915/intel_drv.h     |   4 +
 drivers/gpu/drm/i915/intel_pm.c      | 240 +++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_sprite.c  |   2 +
 6 files changed, 255 insertions(+), 23 deletions(-)

-- 
2.7.4

[PATCH v4 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 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 |  19 +++++++
 drivers/gpu/drm/i915/intel_drv.h     |   2 +
 drivers/gpu/drm/i915/intel_pm.c      | 105 +++++++++++++++++++++++++++++++++++
 5 files changed, 133 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9f655e2..1f6fe8c 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 9397dde..89d146f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7166,6 +7166,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 78beb7e..b80c051 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4562,9 +4562,12 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;
+	struct intel_atomic_state *old_intel_state =
+		to_intel_atomic_state(old_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 +4592,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
 		     !old_primary_state->visible))
 			intel_post_enable_primary(&crtc->base);
 	}
+
+	if (old_intel_state->modeset &&
+	    (old_intel_state->active_crtcs == 0 ||
+	     is_power_of_2(old_intel_state->active_crtcs)))
+		skl_enable_sagv(dev_priv);
 }
 
 static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
@@ -4599,6 +4607,8 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
 	struct intel_crtc_state *pipe_config =
 		to_intel_crtc_state(crtc->base.state);
 	struct drm_atomic_state *old_state = old_crtc_state->base.state;
+	struct intel_atomic_state *old_intel_state =
+		to_intel_atomic_state(old_state);
 	struct drm_plane *primary = crtc->base.primary;
 	struct drm_plane_state *old_pri_state =
 		drm_atomic_get_existing_plane_state(old_state, primary);
@@ -4649,6 +4659,15 @@ 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 (old_intel_state->modeset &&
+	    !is_power_of_2(old_intel_state->active_crtcs) &&
+	    old_intel_state->active_crtcs != 0)
+		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 e74d851..113bf48 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 64d628c..55a9694 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 & 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;
+}
+
+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) && time_before(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 */
@@ -4228,6 +4331,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: [PATCH v4 1/6] drm/i915/skl: Add support for the SAGV, fix underrun hangs

[Matt Roper]

On Tue, Jul 26, 2016 at 01:34:37PM -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 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 |  19 +++++++
>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>  drivers/gpu/drm/i915/intel_pm.c      | 105 +++++++++++++++++++++++++++++++++++
>  5 files changed, 133 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9f655e2..1f6fe8c 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 9397dde..89d146f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7166,6 +7166,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 78beb7e..b80c051 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4562,9 +4562,12 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
>  	struct drm_atomic_state *old_state = old_crtc_state->base.state;
> +	struct intel_atomic_state *old_intel_state =
> +		to_intel_atomic_state(old_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 +4592,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  		     !old_primary_state->visible))
>  			intel_post_enable_primary(&crtc->base);
>  	}
> +
> +	if (old_intel_state->modeset &&
> +	    (old_intel_state->active_crtcs == 0 ||
> +	     is_power_of_2(old_intel_state->active_crtcs)))

We use hweight32 for counting bits elsewhere in the driver, which is a
little bit more self-explanatory, but I guess is_power_of_2 works too.


> +		skl_enable_sagv(dev_priv);
>  }
>  
>  static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
> @@ -4599,6 +4607,8 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state)
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->base.state);
>  	struct drm_atomic_state *old_state = old_crtc_state->base.state;
> +	struct intel_atomic_state *old_intel_state =
> +		to_intel_atomic_state(old_state);
>  	struct drm_plane *primary = crtc->base.primary;
>  	struct drm_plane_state *old_pri_state =
>  		drm_atomic_get_existing_plane_state(old_state, primary);
> @@ -4649,6 +4659,15 @@ 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 (old_intel_state->modeset &&
> +	    !is_power_of_2(old_intel_state->active_crtcs) &&
> +	    old_intel_state->active_crtcs != 0)
> +		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 e74d851..113bf48 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 64d628c..55a9694 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 & 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;
> +}
> +
> +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) && time_before(jiffies, timeout));

I think Chris pointed out on a previous iteration that it would be
easier / cleaner to move the contents of this loop out to a new function
that returns the value of the read and then just call

        ret = wait_for(do_sagv_disable(...), 1);
        if (ret)
                DRM_ERROR("Timeout");


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 */
> @@ -4228,6 +4331,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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

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

[Hans de Goede]

Hi,

On 26-07-16 19:34, 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 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

This seems to not do what you want it to do, if I'm reading your changes
and the original code correct:

<snip>

> @@ -4589,6 +4592,11 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>  		     !old_primary_state->visible))
>  			intel_post_enable_primary(&crtc->base);
>  	}
> +
> +	if (old_intel_state->modeset &&
> +	    (old_intel_state->active_crtcs == 0 ||
> +	     is_power_of_2(old_intel_state->active_crtcs)))
> +		skl_enable_sagv(dev_priv);
>  }
>

Here you are enabling the sagv if the *old* state allows it
(0 or 1 pipes active).

But judging from previous patches / the commit msg the
intent is to enable the sag if the *new* state allows
it, not the old one.

See e.g. the checks for calling intel_post_enable_primary()
which use both primary_state and old_primary_state

Also if you're going to respin you may want to switch
to using hweight as mentioned before, then you can simply
do something like (hamming_weight(active_crtcs) <= 1) as
condition to check for 0 or 1 active crtcs instead of
having 2 checks. Note please double check my logic here.

<snip>

> @@ -4649,6 +4659,15 @@ 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 (old_intel_state->modeset &&
> +	    !is_power_of_2(old_intel_state->active_crtcs) &&
> +	    old_intel_state->active_crtcs != 0)
> +		skl_disable_sagv(dev_priv);
> +
> +	/*

Same thing, you're disabling the sagv if the old state
disallows it, but I believe you should be looking at the new
state instead.

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 e74d851..113bf48 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 64d628c..55a9694 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 & 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;
> +}
> +
> +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) && time_before(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 */
> @@ -4228,6 +4331,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)
>

[PATCH v4 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 55a9694..8ae3f2b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4049,6 +4049,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)
 {
@@ -4121,8 +4139,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;
@@ -4134,8 +4154,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 v4 3/6] drm/i915/skl: Only flush pipes when we change the ddb allocation

[Lyude]

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

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

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

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

[Matt Roper]

On Tue, Jul 26, 2016 at 01:34:39PM -0400, Lyude wrote:
> Manual pipe flushes are only necessary in order to make sure that we prevent
> pipes with changed ddb allocations from overlapping from one another at
> any point in time. Additionally, forcing us to wait for the next vblank
> every time we have to update the watermark values because the cursor was
> moving between screens will introduce a rather noticable lag for users.
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1f6fe8c..d65e897 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1597,6 +1597,7 @@ struct skl_ddb_allocation {
>  
>  struct skl_wm_values {
>  	unsigned dirty_pipes;
> +	bool ddb_changed;
>  	struct skl_ddb_allocation ddb;
>  	uint32_t wm_linetime[I915_MAX_PIPES];
>  	uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8ae3f2b..cda4196 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3892,6 +3892,12 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  	new_ddb = &new_values->ddb;
>  	cur_ddb = &dev_priv->wm.skl_hw.ddb;
>  
> +	/* We only ever need to flush when the ddb allocations change */
> +	if (!new_values->ddb_changed)
> +		return;
> +
> +	new_values->ddb_changed = false;
> +
>  	/*
>  	 * First pass: flush the pipes with the new allocation contained into
>  	 * the old space.
> @@ -3996,6 +4002,22 @@ pipes_modified(struct drm_atomic_state *state)
>  	return ret;
>  }
>  
> +static bool
> +skl_pipe_ddb_changed(struct skl_ddb_allocation *old,
> +		     struct skl_ddb_allocation *new,
> +		     enum pipe pipe)
> +{
> +	if (memcmp(&old->pipe[pipe], &new->pipe[pipe],
> +		   sizeof(old->pipe[pipe])) != 0 ||
> +	    memcmp(&old->plane[pipe], &new->plane[pipe],
> +		   sizeof(old->plane[pipe])) != 0 ||
> +	    memcmp(&old->y_plane[pipe], &new->y_plane[pipe],
> +		   sizeof(old->y_plane[pipe])) != 0)
> +		return true;
> +
> +	return false;
> +}
> +
>  static int
>  skl_compute_ddb(struct drm_atomic_state *state)
>  {
> @@ -4003,7 +4025,8 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct intel_crtc *intel_crtc;
> -	struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb;
> +	struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
> +	struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb;
>  	uint32_t realloc_pipes = pipes_modified(state);
>  	int ret;
>  
> @@ -4041,9 +4064,13 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  		if (IS_ERR(cstate))
>  			return PTR_ERR(cstate);
>  
> -		ret = skl_allocate_pipe_ddb(cstate, ddb);
> +		ret = skl_allocate_pipe_ddb(cstate, new_ddb);
>  		if (ret)
>  			return ret;
> +
> +		if (!intel_state->wm_results.ddb_changed &&

I think you can simplify and drop the first part of this condition here,
but in general,

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


> +		    skl_pipe_ddb_changed(old_ddb, new_ddb, intel_crtc->pipe))
> +			intel_state->wm_results.ddb_changed = true;
>  	}
>  
>  	return 0;
> -- 
> 2.7.4
> 

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

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

[Lyude]

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

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

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

[PATCH v4 5/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

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 |  5 ++++
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_pm.c      | 58 ++++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_sprite.c  |  2 ++
 4 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b80c051..cd67945 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3031,6 +3031,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	intel_crtc->adjusted_x = x_offset;
 	intel_crtc->adjusted_y = y_offset;
 
+	skl_write_plane_wm(intel_crtc, 0);
+
 	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
 	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
 	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
@@ -10261,6 +10263,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
 	int pipe = intel_crtc->pipe;
 	uint32_t cntl = 0;
 
+	if (IS_GEN9(dev_priv))
+		skl_write_cursor_wm(intel_crtc);
+
 	if (plane_state && plane_state->visible) {
 		cntl = MCURSOR_GAMMA_ENABLE;
 		switch (plane_state->base.crtc_w) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 113bf48..e212ed9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1711,6 +1711,8 @@ 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);
+void skl_write_plane_wm(struct intel_crtc *intel_crtc, int plane);
 uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
 bool ilk_disable_lp_wm(struct drm_device *dev);
 int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fb5d2eb..d469ad2 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3783,6 +3783,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,
+			int plane)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	int level, max_level = ilk_wm_max_level(dev);
+	enum pipe pipe = intel_crtc->pipe;
+
+	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)
+{
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
+	int level, max_level = ilk_wm_max_level(dev);
+	enum pipe pipe = intel_crtc->pipe;
+
+	if (!(wm->dirty_pipes & drm_crtc_mask(crtc)))
+		return;
+
+	for (level = 0; level <= max_level; level++) {
+		I915_WRITE(CUR_WM(pipe, level),
+			   wm->plane[pipe][PLANE_CURSOR][level]);
+	}
+	I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
+}
+
 static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 				const struct skl_wm_values *new)
 {
@@ -3790,7 +3831,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)
@@ -3798,21 +3839,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..50026f1 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -238,6 +238,8 @@ skl_update_plane(struct drm_plane *drm_plane,
 	crtc_w--;
 	crtc_h--;
 
+	skl_write_plane_wm(to_intel_crtc(crtc_state->base.crtc), plane);
+
 	if (key->flags) {
 		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
 		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
-- 
2.7.4

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

[Matt Roper]

On Tue, Jul 26, 2016 at 01:34:41PM -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
> 
> 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 |  5 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c      | 58 ++++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_sprite.c  |  2 ++
>  4 files changed, 51 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b80c051..cd67945 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3031,6 +3031,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	intel_crtc->adjusted_x = x_offset;
>  	intel_crtc->adjusted_y = y_offset;
>  
> +	skl_write_plane_wm(intel_crtc, 0);
> +
>  	I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
>  	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
> @@ -10261,6 +10263,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
>  	int pipe = intel_crtc->pipe;
>  	uint32_t cntl = 0;
>  
> +	if (IS_GEN9(dev_priv))
> +		skl_write_cursor_wm(intel_crtc);
> +
>  	if (plane_state && plane_state->visible) {
>  		cntl = MCURSOR_GAMMA_ENABLE;
>  		switch (plane_state->base.crtc_w) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 113bf48..e212ed9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1711,6 +1711,8 @@ 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);
> +void skl_write_plane_wm(struct intel_crtc *intel_crtc, int plane);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fb5d2eb..d469ad2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3783,6 +3783,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,
> +			int plane)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> +	int level, max_level = ilk_wm_max_level(dev);
> +	enum pipe pipe = intel_crtc->pipe;
> +
> +	if (!(wm->dirty_pipes & drm_crtc_mask(crtc)))
> +		return;
> +
> +	I915_WRITE(PIPE_WM_LINETIME(pipe), wm->wm_linetime[pipe]);

Putting this here may lead to it being programmed multiple times or not
at all, depending on what pipes are in use.  Worst case is you have the
CRTC on with no planes except the cursor (and possibly the canvas), but
do a modeset or pipe scaling change that needs to change the linetime
watermark.

I guess we probably need a new device-specific callback from
intel_update_pipe_config() (which gets called by
intel_begin_crtc_commit()) where we can do this once per CRTC.


Matt

> +
> +	for (level = 0; level <= max_level; level++) {
> +		I915_WRITE(PLANE_WM(pipe, plane, level),
> +			   wm->plane[pipe][plane][level]);
> +	}
> +	I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
> +}
> +
> +void skl_write_cursor_wm(struct intel_crtc *intel_crtc)
> +{
> +	struct drm_crtc *crtc = &intel_crtc->base;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct skl_wm_values *wm = &dev_priv->wm.skl_results;
> +	int level, max_level = ilk_wm_max_level(dev);
> +	enum pipe pipe = intel_crtc->pipe;
> +
> +	if (!(wm->dirty_pipes & drm_crtc_mask(crtc)))
> +		return;
> +
> +	for (level = 0; level <= max_level; level++) {
> +		I915_WRITE(CUR_WM(pipe, level),
> +			   wm->plane[pipe][PLANE_CURSOR][level]);
> +	}
> +	I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
> +}
> +
>  static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  				const struct skl_wm_values *new)
>  {
> @@ -3790,7 +3831,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)
> @@ -3798,21 +3839,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..50026f1 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -238,6 +238,8 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	crtc_w--;
>  	crtc_h--;
>  
> +	skl_write_plane_wm(to_intel_crtc(crtc_state->base.crtc), plane);
> +
>  	if (key->flags) {
>  		I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value);
>  		I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
> -- 
> 2.7.4
> 

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

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

[Lyude]

Unfortunately right now we don't really update watermarks on Skylake
properly, since ideally we'd be updating both the ddb allocations, plane
properties, and watermarks all in a single go. Until this is fixed
however, we can improve things somewhat by adding a vblank wait after
the third iteration of pipe flushes, since this forces us to always wait
for new ddb allocations to take affect before trying to change them
again.

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 | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d469ad2..a26dbd9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3973,8 +3973,11 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 	/*
 	 * Third pass: flush the pipes that got more space allocated.
 	 *
-	 * We don't need to actively wait for the update here, next vblank
-	 * will just get more DDB space with the correct WM values.
+	 * While the hardware doesn't require to wait for the next vblank here,
+	 * continuing before the pipe finishes updating could result in us
+	 * trying to update the wm values again before the pipe finishes
+	 * updating, which results in the hardware using intermediate wm values
+	 * and subsequently underrunning pipes.
 	 */
 	for_each_intel_crtc(dev, crtc) {
 		if (!crtc->active)
@@ -3990,6 +3993,16 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
 			continue;
 
 		skl_wm_flush_pipe(dev_priv, pipe, 3);
+
+		/*
+		 * The only time we can get away with not waiting for an update
+		 * is when we just enabled the pipe, e.g. when it doesn't have
+		 * vblanks enabled anyway.
+		 */
+		if (drm_crtc_vblank_get(&crtc->base) == 0) {
+			intel_wait_for_vblank(dev, pipe);
+			drm_crtc_vblank_put(&crtc->base);
+		}
 	}
 }
 
-- 
2.7.4

Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Matt Roper]

This is completely untested (and probably horribly broken/buggy), but
here's a quick mockup of the general approach I was thinking for
ensuring DDB & WM's can be updated together while ensuring the
three-step pipe flushing process is honored:

        https://github.com/mattrope/kernel/commits/experimental/lyude_ddb

Basically the idea is to take note of what's happening to the pipe's DDB
allocation (shrinking, growing, unchanged, etc.) during the atomic check
phase; then during the commit phase, we loop over the CRTC's three times
instead of just once, but only operate on a subset of the CRTC's in each
loop.  While operating on each CRTC, the plane, WM, and DDB all get
programmed together and have a single flush for all three.



Matt

On Tue, Jul 26, 2016 at 01:34:36PM -0400, Lyude wrote:
> Latest version of https://lkml.org/lkml/2016/7/26/290 . Resending the whole
> thing to keep it in one place.
> 
> Lyude (5):
>   drm/i915/skl: Add support for the SAGV, fix underrun hangs
>   drm/i915/skl: Only flush pipes when we change the ddb allocation
>   drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()
>   drm/i915/skl: Update plane watermarks atomically during plane updates
>   drm/i915/skl: Always wait for pipes to update after a flush
> 
> Matt Roper (1):
>   drm/i915/gen9: Only copy WM results for changed pipes to skl_hw
> 
>  drivers/gpu/drm/i915/i915_drv.h      |   3 +
>  drivers/gpu/drm/i915/i915_reg.h      |   5 +
>  drivers/gpu/drm/i915/intel_display.c |  24 ++++
>  drivers/gpu/drm/i915/intel_drv.h     |   4 +
>  drivers/gpu/drm/i915/intel_pm.c      | 240 +++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_sprite.c  |   2 +
>  6 files changed, 255 insertions(+), 23 deletions(-)
> 
> -- 
> 2.7.4
> 
> _______________________________________________
> 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

Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Ville Syrjälä]

On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
> This is completely untested (and probably horribly broken/buggy), but
> here's a quick mockup of the general approach I was thinking for
> ensuring DDB & WM's can be updated together while ensuring the
> three-step pipe flushing process is honored:
> 
>         https://github.com/mattrope/kernel/commits/experimental/lyude_ddb
> 
> Basically the idea is to take note of what's happening to the pipe's DDB
> allocation (shrinking, growing, unchanged, etc.) during the atomic check
> phase;

Didn't look too closely, but I think you can't actually do that unless
you lock all the crtcs whenever the number of active pipes is goind to
change. Meaning we'd essentially be back to the one-big-modeset-lock
apporach, which will cause missed flips and whanot on the other pipes.

The alternative I think would consist of:
- make sure level 0 watermark never exceeds total_ddb_size/max_pipes,
  so that a modeset doesn't have to care about the wms for the other
  pipes not fitting in
- level 1+ watermarks would be checked against total_ddb_size
- protect the plane/pipe commit with the wm mutex whenever the wms
  need to be reprogrammed
- keep the flush_wm thing around for the case when ddb size does get
  changed, protect it with the wm lock
- when programming wms, we will first filter out any level that
  doesn't fit in with the current ddb size, and then program the
  rest in
- potentially introduce per-pipe wm locks if the one big lock looks
  like an issue, which it might if the flush_wm holds it all the way
  through

> then during the commit phase, we loop over the CRTC's three times
> instead of just once, but only operate on a subset of the CRTC's in each
> loop.  While operating on each CRTC, the plane, WM, and DDB all get
> programmed together and have a single flush for all three.
>
> 
> 
> 
> Matt
> 
> On Tue, Jul 26, 2016 at 01:34:36PM -0400, Lyude wrote:
> > Latest version of https://lkml.org/lkml/2016/7/26/290 . Resending the whole
> > thing to keep it in one place.
> > 
> > Lyude (5):
> >   drm/i915/skl: Add support for the SAGV, fix underrun hangs
> >   drm/i915/skl: Only flush pipes when we change the ddb allocation
> >   drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()
> >   drm/i915/skl: Update plane watermarks atomically during plane updates
> >   drm/i915/skl: Always wait for pipes to update after a flush
> > 
> > Matt Roper (1):
> >   drm/i915/gen9: Only copy WM results for changed pipes to skl_hw
> > 
> >  drivers/gpu/drm/i915/i915_drv.h      |   3 +
> >  drivers/gpu/drm/i915/i915_reg.h      |   5 +
> >  drivers/gpu/drm/i915/intel_display.c |  24 ++++
> >  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> >  drivers/gpu/drm/i915/intel_pm.c      | 240 +++++++++++++++++++++++++++++++----
> >  drivers/gpu/drm/i915/intel_sprite.c  |   2 +
> >  6 files changed, 255 insertions(+), 23 deletions(-)
> > 
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > 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

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Lyude]

So I've been working on trying to fix this entirely again (e.g. writing
the ddb properly), since from bug reports it still doesn't sound like
we've got enough workarounds to make this tolerable. I've shown this to
matt roper, but I should probably post what I've been trying to do for
you as well.

So the approach I came up with is here

https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2

My approach differs a little bit from what the bspec recommends, but I
think it's going to be a bit easier to implement. At the end of all the
changes I'm attempting it should look like this:

 * We no longer have a global watermark update for skl
 * A new hook called "update_ddbs" is added to i915_display_funcs. This
   gets called in intel_atomic_commit_tail() after we've disabled any
   CRTCs that needed disabling, and before we begin enabling/updating
   CRTCs
 * Because pipe ddb allocations (not the inner-pipe ddb allocations
   that apply to each pipe's planes) only change while
   enabling/disabling crtcs:
    * Pass 1: Find which pipe's new ddb allocation won't overlap with
      another pipe's previous allocation, and update that pipe first
    * Pass 2: Update the allocation of the remaining pipe

Here's an illustration of what this looks like. Parts of the ddb not
being used by any CRTCs are marked out with 'x's:

With pipe A enabled, we enable pipe B:
Initial DDB:    |           A           |
update_ddbs:    |     A     |xxxxxxxxxxx| Pass 1
Enable pipe B:  |     A     |     B     |

With pipes B and A active, we enable pipe C:

Initial DDB:    |     B     |     A     |
update_ddbs:    |   B   |xxx|     A     | Pass 1
update_ddbs:    |   B   |   A   |xxxxxxx| Pass 2
Enable pipe C:  |   B   |   A   |   C   |

With pipes A, B, and C active, we disable B:
Initial DDB:    |   A   |   B   |   C   |
Disable pipe B: |   A   |xxxxxxx|   C   |
update_ddbs:    |     A     |     C     | Pass 1
Since neither pipe's new allocation overlapped, we skip pass 2

Another allocation with A, B, and C active, disabling A:
Initial DDB:    |   A   |   B   |   C   |
Disable pipe A: |xxxxxxx|   B   |   C   |
update_ddbs:    |     B     |xxx|   C   | Pass 1
update_ddbs:    |     B     |     C     | Pass 2

This should ensure we can always move around the allocations of pipes
without them ever overlapping and exploding.

This branch doesn't entirely fix underrun issues, but I'm mostly sure
that's the fault of still not having removed the global wm update hook
yet (which is leading to additional pipe flushes in places they
shouldn't be):

https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2

As for updating inner-pipe ddb allocations for each plane on a pipe, we
should be able to just do that at the same time we update each pipe's
watermarks

Let me know what you think
	Lyude

On Fri, 2016-07-29 at 12:39 +0300, Ville Syrjälä wrote:
> On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
> > 
> > This is completely untested (and probably horribly broken/buggy),
> > but
> > here's a quick mockup of the general approach I was thinking for
> > ensuring DDB & WM's can be updated together while ensuring the
> > three-step pipe flushing process is honored:
> > 
> >         https://github.com/mattrope/kernel/commits/experimental/lyu
> > de_ddb
> > 
> > Basically the idea is to take note of what's happening to the
> > pipe's DDB
> > allocation (shrinking, growing, unchanged, etc.) during the atomic
> > check
> > phase;
> 
> Didn't look too closely, but I think you can't actually do that
> unless
> you lock all the crtcs whenever the number of active pipes is goind
> to
> change. Meaning we'd essentially be back to the one-big-modeset-lock
> apporach, which will cause missed flips and whanot on the other
> pipes.
> 
> The alternative I think would consist of:
> - make sure level 0 watermark never exceeds total_ddb_size/max_pipes,
>   so that a modeset doesn't have to care about the wms for the other
>   pipes not fitting in
> - level 1+ watermarks would be checked against total_ddb_size
> - protect the plane/pipe commit with the wm mutex whenever the wms
>   need to be reprogrammed
> - keep the flush_wm thing around for the case when ddb size does get
>   changed, protect it with the wm lock
> - when programming wms, we will first filter out any level that
>   doesn't fit in with the current ddb size, and then program the
>   rest in
> - potentially introduce per-pipe wm locks if the one big lock looks
>   like an issue, which it might if the flush_wm holds it all the way
>   through
> 
> > 
> > then during the commit phase, we loop over the CRTC's three times
> > instead of just once, but only operate on a subset of the CRTC's in
> > each
> > loop.  While operating on each CRTC, the plane, WM, and DDB all get
> > programmed together and have a single flush for all three.
> > 
> > 
> > 
> > 
> > Matt
> > 
> > On Tue, Jul 26, 2016 at 01:34:36PM -0400, Lyude wrote:
> > > 
> > > Latest version of https://lkml.org/lkml/2016/7/26/290 . Resending
> > > the whole
> > > thing to keep it in one place.
> > > 
> > > Lyude (5):
> > >   drm/i915/skl: Add support for the SAGV, fix underrun hangs
> > >   drm/i915/skl: Only flush pipes when we change the ddb
> > > allocation
> > >   drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()
> > >   drm/i915/skl: Update plane watermarks atomically during plane
> > > updates
> > >   drm/i915/skl: Always wait for pipes to update after a flush
> > > 
> > > Matt Roper (1):
> > >   drm/i915/gen9: Only copy WM results for changed pipes to skl_hw
> > > 
> > >  drivers/gpu/drm/i915/i915_drv.h      |   3 +
> > >  drivers/gpu/drm/i915/i915_reg.h      |   5 +
> > >  drivers/gpu/drm/i915/intel_display.c |  24 ++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> > >  drivers/gpu/drm/i915/intel_pm.c      | 240
> > > +++++++++++++++++++++++++++++++----
> > >  drivers/gpu/drm/i915/intel_sprite.c  |   2 +
> > >  6 files changed, 255 insertions(+), 23 deletions(-)
> > > 
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > 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
> 
-- 
Cheers,
	Lyude

Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Ville Syrjälä]

On Fri, Jul 29, 2016 at 02:48:09PM -0400, Lyude wrote:
> So I've been working on trying to fix this entirely again (e.g. writing
> the ddb properly), since from bug reports it still doesn't sound like
> we've got enough workarounds to make this tolerable. I've shown this to
> matt roper, but I should probably post what I've been trying to do for
> you as well.
> 
> So the approach I came up with is here
> 
> https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2
> 
> My approach differs a little bit from what the bspec recommends, but I
> think it's going to be a bit easier to implement. At the end of all the
> changes I'm attempting it should look like this:
> 
>  * We no longer have a global watermark update for skl
>  * A new hook called "update_ddbs" is added to i915_display_funcs. This
>    gets called in intel_atomic_commit_tail() after we've disabled any
>    CRTCs that needed disabling, and before we begin enabling/updating
>    CRTCs
>  * Because pipe ddb allocations (not the inner-pipe ddb allocations
>    that apply to each pipe's planes) only change while
>    enabling/disabling crtcs:
>     * Pass 1: Find which pipe's new ddb allocation won't overlap with
>       another pipe's previous allocation, and update that pipe first
>     * Pass 2: Update the allocation of the remaining pipe
> 
> Here's an illustration of what this looks like. Parts of the ddb not
> being used by any CRTCs are marked out with 'x's:
> 
> With pipe A enabled, we enable pipe B:
> Initial DDB:    |           A           |
> update_ddbs:    |     A     |xxxxxxxxxxx| Pass 1
> Enable pipe B:  |     A     |     B     |
> 
> With pipes B and A active, we enable pipe C:
> 
> Initial DDB:    |     B     |     A     |
> update_ddbs:    |   B   |xxx|     A     | Pass 1
> update_ddbs:    |   B   |   A   |xxxxxxx| Pass 2
> Enable pipe C:  |   B   |   A   |   C   |
> 
> With pipes A, B, and C active, we disable B:
> Initial DDB:    |   A   |   B   |   C   |
> Disable pipe B: |   A   |xxxxxxx|   C   |
> update_ddbs:    |     A     |     C     | Pass 1
> Since neither pipe's new allocation overlapped, we skip pass 2
> 
> Another allocation with A, B, and C active, disabling A:
> Initial DDB:    |   A   |   B   |   C   |
> Disable pipe A: |xxxxxxx|   B   |   C   |
> update_ddbs:    |     B     |xxx|   C   | Pass 1
> update_ddbs:    |     B     |     C     | Pass 2
> 
> This should ensure we can always move around the allocations of pipes
> without them ever overlapping and exploding.

That's what the current flush thing does, or at least that what it used
to do at least. Not sure it's really doing it anymore, but I'm pretty
sure the order in which it did things was sound at some point.

> 
> This branch doesn't entirely fix underrun issues, but I'm mostly sure
> that's the fault of still not having removed the global wm update hook
> yet (which is leading to additional pipe flushes in places they
> shouldn't be):

Well it should basically boil down to s/update_wm/update_ddb/
Only DDB reallocation really warrants a global hook. Everything else
should be handled via per-crtc hooks, on all platforms.

> 
> https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2
> 
> As for updating inner-pipe ddb allocations for each plane on a pipe, we
> should be able to just do that at the same time we update each pipe's
> watermarks

Yes.

None of that changes what I said before though. Either you need to
lock down everything when the DDB needs to be repartitioned, or you
do what I outlined in the previous mail. Otherwise a plane update
etc. happening in parallel will still blow up on account of the DDB
state changing underneath the plane update somewhere between compute
and commit. I can't really think of third option that would work.

> 
> Let me know what you think
> 	Lyude
> 
> On Fri, 2016-07-29 at 12:39 +0300, Ville Syrjälä wrote:
> > On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
> > > 
> > > This is completely untested (and probably horribly broken/buggy),
> > > but
> > > here's a quick mockup of the general approach I was thinking for
> > > ensuring DDB & WM's can be updated together while ensuring the
> > > three-step pipe flushing process is honored:
> > > 
> > >         https://github.com/mattrope/kernel/commits/experimental/lyu
> > > de_ddb
> > > 
> > > Basically the idea is to take note of what's happening to the
> > > pipe's DDB
> > > allocation (shrinking, growing, unchanged, etc.) during the atomic
> > > check
> > > phase;
> > 
> > Didn't look too closely, but I think you can't actually do that
> > unless
> > you lock all the crtcs whenever the number of active pipes is goind
> > to
> > change. Meaning we'd essentially be back to the one-big-modeset-lock
> > apporach, which will cause missed flips and whanot on the other
> > pipes.
> > 
> > The alternative I think would consist of:
> > - make sure level 0 watermark never exceeds total_ddb_size/max_pipes,
> >   so that a modeset doesn't have to care about the wms for the other
> >   pipes not fitting in
> > - level 1+ watermarks would be checked against total_ddb_size
> > - protect the plane/pipe commit with the wm mutex whenever the wms
> >   need to be reprogrammed
> > - keep the flush_wm thing around for the case when ddb size does get
> >   changed, protect it with the wm lock
> > - when programming wms, we will first filter out any level that
> >   doesn't fit in with the current ddb size, and then program the
> >   rest in
> > - potentially introduce per-pipe wm locks if the one big lock looks
> >   like an issue, which it might if the flush_wm holds it all the way
> >   through
> > 
> > > 
> > > then during the commit phase, we loop over the CRTC's three times
> > > instead of just once, but only operate on a subset of the CRTC's in
> > > each
> > > loop.  While operating on each CRTC, the plane, WM, and DDB all get
> > > programmed together and have a single flush for all three.
> > > 
> > > 
> > > 
> > > 
> > > Matt
> > > 
> > > On Tue, Jul 26, 2016 at 01:34:36PM -0400, Lyude wrote:
> > > > 
> > > > Latest version of https://lkml.org/lkml/2016/7/26/290 . Resending
> > > > the whole
> > > > thing to keep it in one place.
> > > > 
> > > > Lyude (5):
> > > >   drm/i915/skl: Add support for the SAGV, fix underrun hangs
> > > >   drm/i915/skl: Only flush pipes when we change the ddb
> > > > allocation
> > > >   drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()
> > > >   drm/i915/skl: Update plane watermarks atomically during plane
> > > > updates
> > > >   drm/i915/skl: Always wait for pipes to update after a flush
> > > > 
> > > > Matt Roper (1):
> > > >   drm/i915/gen9: Only copy WM results for changed pipes to skl_hw
> > > > 
> > > >  drivers/gpu/drm/i915/i915_drv.h      |   3 +
> > > >  drivers/gpu/drm/i915/i915_reg.h      |   5 +
> > > >  drivers/gpu/drm/i915/intel_display.c |  24 ++++
> > > >  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> > > >  drivers/gpu/drm/i915/intel_pm.c      | 240
> > > > +++++++++++++++++++++++++++++++----
> > > >  drivers/gpu/drm/i915/intel_sprite.c  |   2 +
> > > >  6 files changed, 255 insertions(+), 23 deletions(-)
> > > > 
> > > > -- 
> > > > 2.7.4
> > > > 
> > > > _______________________________________________
> > > > 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
> > 
> -- 
> Cheers,
> 	Lyude

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Lyude]

On Fri, 2016-07-29 at 22:26 +0300, Ville Syrjälä wrote:
> On Fri, Jul 29, 2016 at 02:48:09PM -0400, Lyude wrote:
> > 
> > So I've been working on trying to fix this entirely again (e.g.
> > writing
> > the ddb properly), since from bug reports it still doesn't sound
> > like
> > we've got enough workarounds to make this tolerable. I've shown
> > this to
> > matt roper, but I should probably post what I've been trying to do
> > for
> > you as well.
> > 
> > So the approach I came up with is here
> > 
> > https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2
> > 
> > My approach differs a little bit from what the bspec recommends,
> > but I
> > think it's going to be a bit easier to implement. At the end of all
> > the
> > changes I'm attempting it should look like this:
> > 
> >  * We no longer have a global watermark update for skl
> >  * A new hook called "update_ddbs" is added to i915_display_funcs.
> > This
> >    gets called in intel_atomic_commit_tail() after we've disabled
> > any
> >    CRTCs that needed disabling, and before we begin
> > enabling/updating
> >    CRTCs
> >  * Because pipe ddb allocations (not the inner-pipe ddb allocations
> >    that apply to each pipe's planes) only change while
> >    enabling/disabling crtcs:
> >     * Pass 1: Find which pipe's new ddb allocation won't overlap
> > with
> >       another pipe's previous allocation, and update that pipe
> > first
> >     * Pass 2: Update the allocation of the remaining pipe
> > 
> > Here's an illustration of what this looks like. Parts of the ddb
> > not
> > being used by any CRTCs are marked out with 'x's:
> > 
> > With pipe A enabled, we enable pipe B:
> > Initial DDB:    |           A           |
> > update_ddbs:    |     A     |xxxxxxxxxxx| Pass 1
> > Enable pipe B:  |     A     |     B     |
> > 
> > With pipes B and A active, we enable pipe C:
> > 
> > Initial DDB:    |     B     |     A     |
> > update_ddbs:    |   B   |xxx|     A     | Pass 1
> > update_ddbs:    |   B   |   A   |xxxxxxx| Pass 2
> > Enable pipe C:  |   B   |   A   |   C   |
> > 
> > With pipes A, B, and C active, we disable B:
> > Initial DDB:    |   A   |   B   |   C   |
> > Disable pipe B: |   A   |xxxxxxx|   C   |
> > update_ddbs:    |     A     |     C     | Pass 1
> > Since neither pipe's new allocation overlapped, we skip pass 2
> > 
> > Another allocation with A, B, and C active, disabling A:
> > Initial DDB:    |   A   |   B   |   C   |
> > Disable pipe A: |xxxxxxx|   B   |   C   |
> > update_ddbs:    |     B     |xxx|   C   | Pass 1
> > update_ddbs:    |     B     |     C     | Pass 2
> > 
> > This should ensure we can always move around the allocations of
> > pipes
> > without them ever overlapping and exploding.
> 
> That's what the current flush thing does, or at least that what it
> used
> to do at least. Not sure it's really doing it anymore, but I'm pretty
> sure the order in which it did things was sound at some point.
> 
> > 
> > 
> > This branch doesn't entirely fix underrun issues, but I'm mostly
> > sure
> > that's the fault of still not having removed the global wm update
> > hook
> > yet (which is leading to additional pipe flushes in places they
> > shouldn't be):
> 
> Well it should basically boil down to s/update_wm/update_ddb/
> Only DDB reallocation really warrants a global hook. Everything else
> should be handled via per-crtc hooks, on all platforms.
> 
> > 
> > 
> > https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2
> > 
> > As for updating inner-pipe ddb allocations for each plane on a
> > pipe, we
> > should be able to just do that at the same time we update each
> > pipe's
> > watermarks
> 
> Yes.
> 
> None of that changes what I said before though. Either you need to
> lock down everything when the DDB needs to be repartitioned, or you
> do what I outlined in the previous mail. Otherwise a plane update
> etc. happening in parallel will still blow up on account of the DDB
> state changing underneath the plane update somewhere between compute
> and commit. I can't really think of third option that would work.
> 
Bleh! I didn't even realize plane updates could happen in parallel like
that. Suddenly your proposal makes a lot more sense...

Anyway, your method definitely sounds like the right one. Unless Matt
thinks there's something that could go wrong there, I'm going to start
working that into the driver and repost the patchset once I've added
that into the driver.

Cheers,
	Lyude 
> > 
> > 
> > Let me know what you think
> > 	Lyude
> > 
> > On Fri, 2016-07-29 at 12:39 +0300, Ville Syrjälä wrote:
> > > 
> > > On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
> > > > 
> > > > 
> > > > This is completely untested (and probably horribly
> > > > broken/buggy),
> > > > but
> > > > here's a quick mockup of the general approach I was thinking
> > > > for
> > > > ensuring DDB & WM's can be updated together while ensuring the
> > > > three-step pipe flushing process is honored:
> > > > 
> > > >         https://github.com/mattrope/kernel/commits/experimental
> > > > /lyu
> > > > de_ddb
> > > > 
> > > > Basically the idea is to take note of what's happening to the
> > > > pipe's DDB
> > > > allocation (shrinking, growing, unchanged, etc.) during the
> > > > atomic
> > > > check
> > > > phase;
> > > 
> > > Didn't look too closely, but I think you can't actually do that
> > > unless
> > > you lock all the crtcs whenever the number of active pipes is
> > > goind
> > > to
> > > change. Meaning we'd essentially be back to the one-big-modeset-
> > > lock
> > > apporach, which will cause missed flips and whanot on the other
> > > pipes.
> > > 
> > > The alternative I think would consist of:
> > > - make sure level 0 watermark never exceeds
> > > total_ddb_size/max_pipes,
> > >   so that a modeset doesn't have to care about the wms for the
> > > other
> > >   pipes not fitting in
> > > - level 1+ watermarks would be checked against total_ddb_size
> > > - protect the plane/pipe commit with the wm mutex whenever the
> > > wms
> > >   need to be reprogrammed
> > > - keep the flush_wm thing around for the case when ddb size does
> > > get
> > >   changed, protect it with the wm lock
> > > - when programming wms, we will first filter out any level that
> > >   doesn't fit in with the current ddb size, and then program the
> > >   rest in
> > > - potentially introduce per-pipe wm locks if the one big lock
> > > looks
> > >   like an issue, which it might if the flush_wm holds it all the
> > > way
> > >   through
> > > 
> > > > 
> > > > 
> > > > then during the commit phase, we loop over the CRTC's three
> > > > times
> > > > instead of just once, but only operate on a subset of the
> > > > CRTC's in
> > > > each
> > > > loop.  While operating on each CRTC, the plane, WM, and DDB all
> > > > get
> > > > programmed together and have a single flush for all three.
> > > > 
> > > > 
> > > > 
> > > > 
> > > > Matt
> > > > 
> > > > On Tue, Jul 26, 2016 at 01:34:36PM -0400, Lyude wrote:
> > > > > 
> > > > > 
> > > > > Latest version of https://lkml.org/lkml/2016/7/26/290 .
> > > > > Resending
> > > > > the whole
> > > > > thing to keep it in one place.
> > > > > 
> > > > > Lyude (5):
> > > > >   drm/i915/skl: Add support for the SAGV, fix underrun hangs
> > > > >   drm/i915/skl: Only flush pipes when we change the ddb
> > > > > allocation
> > > > >   drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()
> > > > >   drm/i915/skl: Update plane watermarks atomically during
> > > > > plane
> > > > > updates
> > > > >   drm/i915/skl: Always wait for pipes to update after a flush
> > > > > 
> > > > > Matt Roper (1):
> > > > >   drm/i915/gen9: Only copy WM results for changed pipes to
> > > > > skl_hw
> > > > > 
> > > > >  drivers/gpu/drm/i915/i915_drv.h      |   3 +
> > > > >  drivers/gpu/drm/i915/i915_reg.h      |   5 +
> > > > >  drivers/gpu/drm/i915/intel_display.c |  24 ++++
> > > > >  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> > > > >  drivers/gpu/drm/i915/intel_pm.c      | 240
> > > > > +++++++++++++++++++++++++++++++----
> > > > >  drivers/gpu/drm/i915/intel_sprite.c  |   2 +
> > > > >  6 files changed, 255 insertions(+), 23 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > > > _______________________________________________
> > > > > 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
> > > 
> > -- 
> > Cheers,
> > 	Lyude
> 
-- 
Cheers,
	Lyude

Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Matt Roper]

On Fri, Jul 29, 2016 at 10:26:20PM +0300, Ville Syrjälä wrote:
> On Fri, Jul 29, 2016 at 02:48:09PM -0400, Lyude wrote:
> > So I've been working on trying to fix this entirely again (e.g. writing
> > the ddb properly), since from bug reports it still doesn't sound like
> > we've got enough workarounds to make this tolerable. I've shown this to
> > matt roper, but I should probably post what I've been trying to do for
> > you as well.
> > 
> > So the approach I came up with is here
> > 
> > https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2
> > 
> > My approach differs a little bit from what the bspec recommends, but I
> > think it's going to be a bit easier to implement. At the end of all the
> > changes I'm attempting it should look like this:
> > 
> >  * We no longer have a global watermark update for skl
> >  * A new hook called "update_ddbs" is added to i915_display_funcs. This
> >    gets called in intel_atomic_commit_tail() after we've disabled any
> >    CRTCs that needed disabling, and before we begin enabling/updating
> >    CRTCs
> >  * Because pipe ddb allocations (not the inner-pipe ddb allocations
> >    that apply to each pipe's planes) only change while
> >    enabling/disabling crtcs:
> >     * Pass 1: Find which pipe's new ddb allocation won't overlap with
> >       another pipe's previous allocation, and update that pipe first
> >     * Pass 2: Update the allocation of the remaining pipe
> > 
> > Here's an illustration of what this looks like. Parts of the ddb not
> > being used by any CRTCs are marked out with 'x's:
> > 
> > With pipe A enabled, we enable pipe B:
> > Initial DDB:    |           A           |
> > update_ddbs:    |     A     |xxxxxxxxxxx| Pass 1
> > Enable pipe B:  |     A     |     B     |
> > 
> > With pipes B and A active, we enable pipe C:
> > 
> > Initial DDB:    |     B     |     A     |
> > update_ddbs:    |   B   |xxx|     A     | Pass 1
> > update_ddbs:    |   B   |   A   |xxxxxxx| Pass 2
> > Enable pipe C:  |   B   |   A   |   C   |
> > 
> > With pipes A, B, and C active, we disable B:
> > Initial DDB:    |   A   |   B   |   C   |
> > Disable pipe B: |   A   |xxxxxxx|   C   |
> > update_ddbs:    |     A     |     C     | Pass 1
> > Since neither pipe's new allocation overlapped, we skip pass 2
> > 
> > Another allocation with A, B, and C active, disabling A:
> > Initial DDB:    |   A   |   B   |   C   |
> > Disable pipe A: |xxxxxxx|   B   |   C   |
> > update_ddbs:    |     B     |xxx|   C   | Pass 1
> > update_ddbs:    |     B     |     C     | Pass 2
> > 
> > This should ensure we can always move around the allocations of pipes
> > without them ever overlapping and exploding.
> 
> That's what the current flush thing does, or at least that what it used
> to do at least. Not sure it's really doing it anymore, but I'm pretty
> sure the order in which it did things was sound at some point.
> 
> > 
> > This branch doesn't entirely fix underrun issues, but I'm mostly sure
> > that's the fault of still not having removed the global wm update hook
> > yet (which is leading to additional pipe flushes in places they
> > shouldn't be):
> 
> Well it should basically boil down to s/update_wm/update_ddb/
> Only DDB reallocation really warrants a global hook. Everything else
> should be handled via per-crtc hooks, on all platforms.

I don't think we want even update_ddb.  We want *calculate_ddb* to be a
global hook during the atomic check phase (and we do have that today),
but when we get around to the commit phase and start writing stuff to
hardware, we want to program the per-pipe DDB entries at the same time
we program the WM's and regular plane registers (since they should be
double buffered and flushed out the same way).  We just need to make
sure that the order we process pipes in follows the special flushing
rules noted above, which is what my pseudo-patches were trying to
describe.

> 
> > 
> > https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2
> > 
> > As for updating inner-pipe ddb allocations for each plane on a pipe, we
> > should be able to just do that at the same time we update each pipe's
> > watermarks
> 
> Yes.
> 
> None of that changes what I said before though. Either you need to
> lock down everything when the DDB needs to be repartitioned, or you
> do what I outlined in the previous mail. Otherwise a plane update
> etc. happening in parallel will still blow up on account of the DDB
> state changing underneath the plane update somewhere between compute
> and commit. I can't really think of third option that would work.

Yep, I agree with all of this as well (and we do lock everything down
today for exactly this reason).  It's unfortunate that we need a
BKL-style mechanism for the DDB, but as I noted in the other message,
using fixed pre-partitioning for level 0 breaks too many additional use
cases.  :-(


Matt

> 
> > 
> > Let me know what you think
> > 	Lyude
> > 
> > On Fri, 2016-07-29 at 12:39 +0300, Ville Syrjälä wrote:
> > > On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
> > > > 
> > > > This is completely untested (and probably horribly broken/buggy),
> > > > but
> > > > here's a quick mockup of the general approach I was thinking for
> > > > ensuring DDB & WM's can be updated together while ensuring the
> > > > three-step pipe flushing process is honored:
> > > > 
> > > >         https://github.com/mattrope/kernel/commits/experimental/lyu
> > > > de_ddb
> > > > 
> > > > Basically the idea is to take note of what's happening to the
> > > > pipe's DDB
> > > > allocation (shrinking, growing, unchanged, etc.) during the atomic
> > > > check
> > > > phase;
> > > 
> > > Didn't look too closely, but I think you can't actually do that
> > > unless
> > > you lock all the crtcs whenever the number of active pipes is goind
> > > to
> > > change. Meaning we'd essentially be back to the one-big-modeset-lock
> > > apporach, which will cause missed flips and whanot on the other
> > > pipes.
> > > 
> > > The alternative I think would consist of:
> > > - make sure level 0 watermark never exceeds total_ddb_size/max_pipes,
> > >   so that a modeset doesn't have to care about the wms for the other
> > >   pipes not fitting in
> > > - level 1+ watermarks would be checked against total_ddb_size
> > > - protect the plane/pipe commit with the wm mutex whenever the wms
> > >   need to be reprogrammed
> > > - keep the flush_wm thing around for the case when ddb size does get
> > >   changed, protect it with the wm lock
> > > - when programming wms, we will first filter out any level that
> > >   doesn't fit in with the current ddb size, and then program the
> > >   rest in
> > > - potentially introduce per-pipe wm locks if the one big lock looks
> > >   like an issue, which it might if the flush_wm holds it all the way
> > >   through
> > > 
> > > > 
> > > > then during the commit phase, we loop over the CRTC's three times
> > > > instead of just once, but only operate on a subset of the CRTC's in
> > > > each
> > > > loop.  While operating on each CRTC, the plane, WM, and DDB all get
> > > > programmed together and have a single flush for all three.
> > > > 
> > > > 
> > > > 
> > > > 
> > > > Matt
> > > > 
> > > > On Tue, Jul 26, 2016 at 01:34:36PM -0400, Lyude wrote:
> > > > > 
> > > > > Latest version of https://lkml.org/lkml/2016/7/26/290 . Resending
> > > > > the whole
> > > > > thing to keep it in one place.
> > > > > 
> > > > > Lyude (5):
> > > > >   drm/i915/skl: Add support for the SAGV, fix underrun hangs
> > > > >   drm/i915/skl: Only flush pipes when we change the ddb
> > > > > allocation
> > > > >   drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()
> > > > >   drm/i915/skl: Update plane watermarks atomically during plane
> > > > > updates
> > > > >   drm/i915/skl: Always wait for pipes to update after a flush
> > > > > 
> > > > > Matt Roper (1):
> > > > >   drm/i915/gen9: Only copy WM results for changed pipes to skl_hw
> > > > > 
> > > > >  drivers/gpu/drm/i915/i915_drv.h      |   3 +
> > > > >  drivers/gpu/drm/i915/i915_reg.h      |   5 +
> > > > >  drivers/gpu/drm/i915/intel_display.c |  24 ++++
> > > > >  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> > > > >  drivers/gpu/drm/i915/intel_pm.c      | 240
> > > > > +++++++++++++++++++++++++++++++----
> > > > >  drivers/gpu/drm/i915/intel_sprite.c  |   2 +
> > > > >  6 files changed, 255 insertions(+), 23 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.7.4
> > > > > 
> > > > > _______________________________________________
> > > > > 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
> > > 
> > -- 
> > Cheers,
> > 	Lyude
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Ville Syrjälä]

On Fri, Jul 29, 2016 at 01:41:26PM -0700, Matt Roper wrote:
> On Fri, Jul 29, 2016 at 10:26:20PM +0300, Ville Syrjälä wrote:
> > On Fri, Jul 29, 2016 at 02:48:09PM -0400, Lyude wrote:
> > > So I've been working on trying to fix this entirely again (e.g. writing
> > > the ddb properly), since from bug reports it still doesn't sound like
> > > we've got enough workarounds to make this tolerable. I've shown this to
> > > matt roper, but I should probably post what I've been trying to do for
> > > you as well.
> > > 
> > > So the approach I came up with is here
> > > 
> > > https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2
> > > 
> > > My approach differs a little bit from what the bspec recommends, but I
> > > think it's going to be a bit easier to implement. At the end of all the
> > > changes I'm attempting it should look like this:
> > > 
> > >  * We no longer have a global watermark update for skl
> > >  * A new hook called "update_ddbs" is added to i915_display_funcs. This
> > >    gets called in intel_atomic_commit_tail() after we've disabled any
> > >    CRTCs that needed disabling, and before we begin enabling/updating
> > >    CRTCs
> > >  * Because pipe ddb allocations (not the inner-pipe ddb allocations
> > >    that apply to each pipe's planes) only change while
> > >    enabling/disabling crtcs:
> > >     * Pass 1: Find which pipe's new ddb allocation won't overlap with
> > >       another pipe's previous allocation, and update that pipe first
> > >     * Pass 2: Update the allocation of the remaining pipe
> > > 
> > > Here's an illustration of what this looks like. Parts of the ddb not
> > > being used by any CRTCs are marked out with 'x's:
> > > 
> > > With pipe A enabled, we enable pipe B:
> > > Initial DDB:    |           A           |
> > > update_ddbs:    |     A     |xxxxxxxxxxx| Pass 1
> > > Enable pipe B:  |     A     |     B     |
> > > 
> > > With pipes B and A active, we enable pipe C:
> > > 
> > > Initial DDB:    |     B     |     A     |
> > > update_ddbs:    |   B   |xxx|     A     | Pass 1
> > > update_ddbs:    |   B   |   A   |xxxxxxx| Pass 2
> > > Enable pipe C:  |   B   |   A   |   C   |
> > > 
> > > With pipes A, B, and C active, we disable B:
> > > Initial DDB:    |   A   |   B   |   C   |
> > > Disable pipe B: |   A   |xxxxxxx|   C   |
> > > update_ddbs:    |     A     |     C     | Pass 1
> > > Since neither pipe's new allocation overlapped, we skip pass 2
> > > 
> > > Another allocation with A, B, and C active, disabling A:
> > > Initial DDB:    |   A   |   B   |   C   |
> > > Disable pipe A: |xxxxxxx|   B   |   C   |
> > > update_ddbs:    |     B     |xxx|   C   | Pass 1
> > > update_ddbs:    |     B     |     C     | Pass 2
> > > 
> > > This should ensure we can always move around the allocations of pipes
> > > without them ever overlapping and exploding.
> > 
> > That's what the current flush thing does, or at least that what it used
> > to do at least. Not sure it's really doing it anymore, but I'm pretty
> > sure the order in which it did things was sound at some point.
> > 
> > > 
> > > This branch doesn't entirely fix underrun issues, but I'm mostly sure
> > > that's the fault of still not having removed the global wm update hook
> > > yet (which is leading to additional pipe flushes in places they
> > > shouldn't be):
> > 
> > Well it should basically boil down to s/update_wm/update_ddb/
> > Only DDB reallocation really warrants a global hook. Everything else
> > should be handled via per-crtc hooks, on all platforms.
> 
> I don't think we want even update_ddb.  We want *calculate_ddb* to be a
> global hook during the atomic check phase (and we do have that today),
> but when we get around to the commit phase and start writing stuff to
> hardware, we want to program the per-pipe DDB entries at the same time
> we program the WM's and regular plane registers (since they should be
> double buffered and flushed out the same way).  We just need to make
> sure that the order we process pipes in follows the special flushing
> rules noted above, which is what my pseudo-patches were trying to
> describe.

My comments are w.r.t. the scheme where we don't lock down all the crtcs
for every DDB update.

> 
> > 
> > > 
> > > https://github.com/lyude/linux/tree/wip/skl-fix-wms-v5r2
> > > 
> > > As for updating inner-pipe ddb allocations for each plane on a pipe, we
> > > should be able to just do that at the same time we update each pipe's
> > > watermarks
> > 
> > Yes.
> > 
> > None of that changes what I said before though. Either you need to
> > lock down everything when the DDB needs to be repartitioned, or you
> > do what I outlined in the previous mail. Otherwise a plane update
> > etc. happening in parallel will still blow up on account of the DDB
> > state changing underneath the plane update somewhere between compute
> > and commit. I can't really think of third option that would work.
> 
> Yep, I agree with all of this as well (and we do lock everything down
> today for exactly this reason).  It's unfortunate that we need a
> BKL-style mechanism for the DDB, but as I noted in the other message,
> using fixed pre-partitioning for level 0 breaks too many additional use
> cases.  :-(

Hmm. Well that sucks a bit.

> 
> 
> Matt
> 
> > 
> > > 
> > > Let me know what you think
> > > 	Lyude
> > > 
> > > On Fri, 2016-07-29 at 12:39 +0300, Ville Syrjälä wrote:
> > > > On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
> > > > > 
> > > > > This is completely untested (and probably horribly broken/buggy),
> > > > > but
> > > > > here's a quick mockup of the general approach I was thinking for
> > > > > ensuring DDB & WM's can be updated together while ensuring the
> > > > > three-step pipe flushing process is honored:
> > > > > 
> > > > >         https://github.com/mattrope/kernel/commits/experimental/lyu
> > > > > de_ddb
> > > > > 
> > > > > Basically the idea is to take note of what's happening to the
> > > > > pipe's DDB
> > > > > allocation (shrinking, growing, unchanged, etc.) during the atomic
> > > > > check
> > > > > phase;
> > > > 
> > > > Didn't look too closely, but I think you can't actually do that
> > > > unless
> > > > you lock all the crtcs whenever the number of active pipes is goind
> > > > to
> > > > change. Meaning we'd essentially be back to the one-big-modeset-lock
> > > > apporach, which will cause missed flips and whanot on the other
> > > > pipes.
> > > > 
> > > > The alternative I think would consist of:
> > > > - make sure level 0 watermark never exceeds total_ddb_size/max_pipes,
> > > >   so that a modeset doesn't have to care about the wms for the other
> > > >   pipes not fitting in
> > > > - level 1+ watermarks would be checked against total_ddb_size
> > > > - protect the plane/pipe commit with the wm mutex whenever the wms
> > > >   need to be reprogrammed
> > > > - keep the flush_wm thing around for the case when ddb size does get
> > > >   changed, protect it with the wm lock
> > > > - when programming wms, we will first filter out any level that
> > > >   doesn't fit in with the current ddb size, and then program the
> > > >   rest in
> > > > - potentially introduce per-pipe wm locks if the one big lock looks
> > > >   like an issue, which it might if the flush_wm holds it all the way
> > > >   through
> > > > 
> > > > > 
> > > > > then during the commit phase, we loop over the CRTC's three times
> > > > > instead of just once, but only operate on a subset of the CRTC's in
> > > > > each
> > > > > loop.  While operating on each CRTC, the plane, WM, and DDB all get
> > > > > programmed together and have a single flush for all three.
> > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > Matt
> > > > > 
> > > > > On Tue, Jul 26, 2016 at 01:34:36PM -0400, Lyude wrote:
> > > > > > 
> > > > > > Latest version of https://lkml.org/lkml/2016/7/26/290 . Resending
> > > > > > the whole
> > > > > > thing to keep it in one place.
> > > > > > 
> > > > > > Lyude (5):
> > > > > >   drm/i915/skl: Add support for the SAGV, fix underrun hangs
> > > > > >   drm/i915/skl: Only flush pipes when we change the ddb
> > > > > > allocation
> > > > > >   drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()
> > > > > >   drm/i915/skl: Update plane watermarks atomically during plane
> > > > > > updates
> > > > > >   drm/i915/skl: Always wait for pipes to update after a flush
> > > > > > 
> > > > > > Matt Roper (1):
> > > > > >   drm/i915/gen9: Only copy WM results for changed pipes to skl_hw
> > > > > > 
> > > > > >  drivers/gpu/drm/i915/i915_drv.h      |   3 +
> > > > > >  drivers/gpu/drm/i915/i915_reg.h      |   5 +
> > > > > >  drivers/gpu/drm/i915/intel_display.c |  24 ++++
> > > > > >  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> > > > > >  drivers/gpu/drm/i915/intel_pm.c      | 240
> > > > > > +++++++++++++++++++++++++++++++----
> > > > > >  drivers/gpu/drm/i915/intel_sprite.c  |   2 +
> > > > > >  6 files changed, 255 insertions(+), 23 deletions(-)
> > > > > > 
> > > > > > -- 
> > > > > > 2.7.4
> > > > > > 
> > > > > > _______________________________________________
> > > > > > 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
> > > > 
> > > -- 
> > > Cheers,
> > > 	Lyude
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Matt Roper]

On Fri, Jul 29, 2016 at 12:39:05PM +0300, Ville Syrjälä wrote:
> On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
> > This is completely untested (and probably horribly broken/buggy), but
> > here's a quick mockup of the general approach I was thinking for
> > ensuring DDB & WM's can be updated together while ensuring the
> > three-step pipe flushing process is honored:
> > 
> >         https://github.com/mattrope/kernel/commits/experimental/lyude_ddb
> > 
> > Basically the idea is to take note of what's happening to the pipe's DDB
> > allocation (shrinking, growing, unchanged, etc.) during the atomic check
> > phase;
> 
> Didn't look too closely, but I think you can't actually do that unless
> you lock all the crtcs whenever the number of active pipes is goind to
> change. Meaning we'd essentially be back to the one-big-modeset-lock
> apporach, which will cause missed flips and whanot on the other pipes.
> 
> The alternative I think would consist of:
> - make sure level 0 watermark never exceeds total_ddb_size/max_pipes,
>   so that a modeset doesn't have to care about the wms for the other
>   pipes not fitting in

Unfortunately this part is the problem.  You might get away with doing
this on SKL/KBL which only have three planes max per pipe and a large
(896 block) DDB, but on BXT you have up to four planes (we don't
actually enable the topmost plane in a full-featured manner just yet,
but need to soon), yet the total DDB is only 512 blocks.  Sadly, the
platform with more planes was given a smaller DDB...   :-(

We're already in trouble because users that are running setups like 3x
4K with most/all planes in use at large sizes can't find level 0
watermarks that satisfy their needs and we have to reject the whole
configuration.  If we further limit each pipe's usage to total/maxpipes
(i.e., 170 blocks per pipe on BXT), then we're going to hit similar
issues when only driving one or two displays with with all of the planes
in use, even though we should have had more DDB space to work with.

I guess serious plane usage isn't too common in desktop setups today,
but it's a very critical feature in the embedded world; we can't really
afford to cripple plane usage further.  Unfortunately, as you point out
above, this means that we have to follow the bspec's DDB allocation
method, which in turn means we need to grab _all_ CRTC locks any time
_any_ CRTC is being turned on or turned off which is a BKL-style way of
doing things.


Matt

> - level 1+ watermarks would be checked against total_ddb_size
> - protect the plane/pipe commit with the wm mutex whenever the wms
>   need to be reprogrammed
> - keep the flush_wm thing around for the case when ddb size does get
>   changed, protect it with the wm lock
> - when programming wms, we will first filter out any level that
>   doesn't fit in with the current ddb size, and then program the
>   rest in
> - potentially introduce per-pipe wm locks if the one big lock looks
>   like an issue, which it might if the flush_wm holds it all the way
>   through
> 
> > then during the commit phase, we loop over the CRTC's three times
> > instead of just once, but only operate on a subset of the CRTC's in each
> > loop.  While operating on each CRTC, the plane, WM, and DDB all get
> > programmed together and have a single flush for all three.
> >
> > 
> > 
> > 
> > Matt
> > 
> > On Tue, Jul 26, 2016 at 01:34:36PM -0400, Lyude wrote:
> > > Latest version of https://lkml.org/lkml/2016/7/26/290 . Resending the whole
> > > thing to keep it in one place.
> > > 
> > > Lyude (5):
> > >   drm/i915/skl: Add support for the SAGV, fix underrun hangs
> > >   drm/i915/skl: Only flush pipes when we change the ddb allocation
> > >   drm/i915/skl: Fix extra whitespace in skl_flush_wm_values()
> > >   drm/i915/skl: Update plane watermarks atomically during plane updates
> > >   drm/i915/skl: Always wait for pipes to update after a flush
> > > 
> > > Matt Roper (1):
> > >   drm/i915/gen9: Only copy WM results for changed pipes to skl_hw
> > > 
> > >  drivers/gpu/drm/i915/i915_drv.h      |   3 +
> > >  drivers/gpu/drm/i915/i915_reg.h      |   5 +
> > >  drivers/gpu/drm/i915/intel_display.c |  24 ++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |   4 +
> > >  drivers/gpu/drm/i915/intel_pm.c      | 240 +++++++++++++++++++++++++++++++----
> > >  drivers/gpu/drm/i915/intel_sprite.c  |   2 +
> > >  6 files changed, 255 insertions(+), 23 deletions(-)
> > > 
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > 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
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Maarten Lankhorst]

Op 29-07-16 om 22:33 schreef Matt Roper:
> On Fri, Jul 29, 2016 at 12:39:05PM +0300, Ville Syrjälä wrote:
>> On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
>>> This is completely untested (and probably horribly broken/buggy), but
>>> here's a quick mockup of the general approach I was thinking for
>>> ensuring DDB & WM's can be updated together while ensuring the
>>> three-step pipe flushing process is honored:
>>>
>>>         https://github.com/mattrope/kernel/commits/experimental/lyude_ddb
>>>
>>> Basically the idea is to take note of what's happening to the pipe's DDB
>>> allocation (shrinking, growing, unchanged, etc.) during the atomic check
>>> phase;
>> Didn't look too closely, but I think you can't actually do that unless
>> you lock all the crtcs whenever the number of active pipes is goind to
>> change. Meaning we'd essentially be back to the one-big-modeset-lock
>> apporach, which will cause missed flips and whanot on the other pipes.
>>
>> The alternative I think would consist of:
>> - make sure level 0 watermark never exceeds total_ddb_size/max_pipes,
>>   so that a modeset doesn't have to care about the wms for the other
>>   pipes not fitting in
> Unfortunately this part is the problem.  You might get away with doing
> this on SKL/KBL which only have three planes max per pipe and a large
> (896 block) DDB, but on BXT you have up to four planes (we don't
> actually enable the topmost plane in a full-featured manner just yet,
> but need to soon), yet the total DDB is only 512 blocks.  Sadly, the
> platform with more planes was given a smaller DDB...   :-(
> We're already in trouble because users that are running setups like 3x
> 4K with most/all planes in use at large sizes can't find level 0
> watermarks that satisfy their needs and we have to reject the whole
> configuration.  If we further limit each pipe's usage to total/maxpipes
> (i.e., 170 blocks per pipe on BXT), then we're going to hit similar
> issues when only driving one or two displays with with all of the planes
> in use, even though we should have had more DDB space to work with.
>
> I guess serious plane usage isn't too common in desktop setups today,
> but it's a very critical feature in the embedded world; we can't really
> afford to cripple plane usage further.  Unfortunately, as you point out
> above, this means that we have to follow the bspec's DDB allocation
> method, which in turn means we need to grab _all_ CRTC locks any time
> _any_ CRTC is being turned on or turned off which is a BKL-style way of
> doing things.
Meh, I'm running into a similar issue on vlv/chv. I don't see a way around it. :(
Only thing we could do is split up the state to make the non-modeset
crtc's complete early.

Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Ville Syrjälä]

On Mon, Aug 01, 2016 at 10:48:37AM +0200, Maarten Lankhorst wrote:
> Op 29-07-16 om 22:33 schreef Matt Roper:
> > On Fri, Jul 29, 2016 at 12:39:05PM +0300, Ville Syrjälä wrote:
> >> On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
> >>> This is completely untested (and probably horribly broken/buggy), but
> >>> here's a quick mockup of the general approach I was thinking for
> >>> ensuring DDB & WM's can be updated together while ensuring the
> >>> three-step pipe flushing process is honored:
> >>>
> >>>         https://github.com/mattrope/kernel/commits/experimental/lyude_ddb
> >>>
> >>> Basically the idea is to take note of what's happening to the pipe's DDB
> >>> allocation (shrinking, growing, unchanged, etc.) during the atomic check
> >>> phase;
> >> Didn't look too closely, but I think you can't actually do that unless
> >> you lock all the crtcs whenever the number of active pipes is goind to
> >> change. Meaning we'd essentially be back to the one-big-modeset-lock
> >> apporach, which will cause missed flips and whanot on the other pipes.
> >>
> >> The alternative I think would consist of:
> >> - make sure level 0 watermark never exceeds total_ddb_size/max_pipes,
> >>   so that a modeset doesn't have to care about the wms for the other
> >>   pipes not fitting in
> > Unfortunately this part is the problem.  You might get away with doing
> > this on SKL/KBL which only have three planes max per pipe and a large
> > (896 block) DDB, but on BXT you have up to four planes (we don't
> > actually enable the topmost plane in a full-featured manner just yet,
> > but need to soon), yet the total DDB is only 512 blocks.  Sadly, the
> > platform with more planes was given a smaller DDB...   :-(
> > We're already in trouble because users that are running setups like 3x
> > 4K with most/all planes in use at large sizes can't find level 0
> > watermarks that satisfy their needs and we have to reject the whole
> > configuration.  If we further limit each pipe's usage to total/maxpipes
> > (i.e., 170 blocks per pipe on BXT), then we're going to hit similar
> > issues when only driving one or two displays with with all of the planes
> > in use, even though we should have had more DDB space to work with.
> >
> > I guess serious plane usage isn't too common in desktop setups today,
> > but it's a very critical feature in the embedded world; we can't really
> > afford to cripple plane usage further.  Unfortunately, as you point out
> > above, this means that we have to follow the bspec's DDB allocation
> > method, which in turn means we need to grab _all_ CRTC locks any time
> > _any_ CRTC is being turned on or turned off which is a BKL-style way of
> > doing things.
> Meh, I'm running into a similar issue on vlv/chv. I don't see a way around it. :(

Now are you hitting it w/ vlv/chv? They don't even have a shared DDB.

> Only thing we could do is split up the state to make the non-modeset
> crtc's complete early.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Maarten Lankhorst]

Op 01-08-16 om 13:48 schreef Ville Syrjälä:
> On Mon, Aug 01, 2016 at 10:48:37AM +0200, Maarten Lankhorst wrote:
>> Op 29-07-16 om 22:33 schreef Matt Roper:
>>> On Fri, Jul 29, 2016 at 12:39:05PM +0300, Ville Syrjälä wrote:
>>>> On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
>>>>> This is completely untested (and probably horribly broken/buggy), but
>>>>> here's a quick mockup of the general approach I was thinking for
>>>>> ensuring DDB & WM's can be updated together while ensuring the
>>>>> three-step pipe flushing process is honored:
>>>>>
>>>>>         https://github.com/mattrope/kernel/commits/experimental/lyude_ddb
>>>>>
>>>>> Basically the idea is to take note of what's happening to the pipe's DDB
>>>>> allocation (shrinking, growing, unchanged, etc.) during the atomic check
>>>>> phase;
>>>> Didn't look too closely, but I think you can't actually do that unless
>>>> you lock all the crtcs whenever the number of active pipes is goind to
>>>> change. Meaning we'd essentially be back to the one-big-modeset-lock
>>>> apporach, which will cause missed flips and whanot on the other pipes.
>>>>
>>>> The alternative I think would consist of:
>>>> - make sure level 0 watermark never exceeds total_ddb_size/max_pipes,
>>>>   so that a modeset doesn't have to care about the wms for the other
>>>>   pipes not fitting in
>>> Unfortunately this part is the problem.  You might get away with doing
>>> this on SKL/KBL which only have three planes max per pipe and a large
>>> (896 block) DDB, but on BXT you have up to four planes (we don't
>>> actually enable the topmost plane in a full-featured manner just yet,
>>> but need to soon), yet the total DDB is only 512 blocks.  Sadly, the
>>> platform with more planes was given a smaller DDB...   :-(
>>> We're already in trouble because users that are running setups like 3x
>>> 4K with most/all planes in use at large sizes can't find level 0
>>> watermarks that satisfy their needs and we have to reject the whole
>>> configuration.  If we further limit each pipe's usage to total/maxpipes
>>> (i.e., 170 blocks per pipe on BXT), then we're going to hit similar
>>> issues when only driving one or two displays with with all of the planes
>>> in use, even though we should have had more DDB space to work with.
>>>
>>> I guess serious plane usage isn't too common in desktop setups today,
>>> but it's a very critical feature in the embedded world; we can't really
>>> afford to cripple plane usage further.  Unfortunately, as you point out
>>> above, this means that we have to follow the bspec's DDB allocation
>>> method, which in turn means we need to grab _all_ CRTC locks any time
>>> _any_ CRTC is being turned on or turned off which is a BKL-style way of
>>> doing things.
>> Meh, I'm running into a similar issue on vlv/chv. I don't see a way around it. :(
> Now are you hitting it w/ vlv/chv? They don't even have a shared DDB.
Not really the same, but determining what power saving levels + values to set with possibly parallel updates.

~Maarten

Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks

[Ville Syrjälä]

On Tue, Aug 02, 2016 at 05:41:50PM +0200, Maarten Lankhorst wrote:
> Op 01-08-16 om 13:48 schreef Ville Syrjälä:
> > On Mon, Aug 01, 2016 at 10:48:37AM +0200, Maarten Lankhorst wrote:
> >> Op 29-07-16 om 22:33 schreef Matt Roper:
> >>> On Fri, Jul 29, 2016 at 12:39:05PM +0300, Ville Syrjälä wrote:
> >>>> On Thu, Jul 28, 2016 at 05:03:52PM -0700, Matt Roper wrote:
> >>>>> This is completely untested (and probably horribly broken/buggy), but
> >>>>> here's a quick mockup of the general approach I was thinking for
> >>>>> ensuring DDB & WM's can be updated together while ensuring the
> >>>>> three-step pipe flushing process is honored:
> >>>>>
> >>>>>         https://github.com/mattrope/kernel/commits/experimental/lyude_ddb
> >>>>>
> >>>>> Basically the idea is to take note of what's happening to the pipe's DDB
> >>>>> allocation (shrinking, growing, unchanged, etc.) during the atomic check
> >>>>> phase;
> >>>> Didn't look too closely, but I think you can't actually do that unless
> >>>> you lock all the crtcs whenever the number of active pipes is goind to
> >>>> change. Meaning we'd essentially be back to the one-big-modeset-lock
> >>>> apporach, which will cause missed flips and whanot on the other pipes.
> >>>>
> >>>> The alternative I think would consist of:
> >>>> - make sure level 0 watermark never exceeds total_ddb_size/max_pipes,
> >>>>   so that a modeset doesn't have to care about the wms for the other
> >>>>   pipes not fitting in
> >>> Unfortunately this part is the problem.  You might get away with doing
> >>> this on SKL/KBL which only have three planes max per pipe and a large
> >>> (896 block) DDB, but on BXT you have up to four planes (we don't
> >>> actually enable the topmost plane in a full-featured manner just yet,
> >>> but need to soon), yet the total DDB is only 512 blocks.  Sadly, the
> >>> platform with more planes was given a smaller DDB...   :-(
> >>> We're already in trouble because users that are running setups like 3x
> >>> 4K with most/all planes in use at large sizes can't find level 0
> >>> watermarks that satisfy their needs and we have to reject the whole
> >>> configuration.  If we further limit each pipe's usage to total/maxpipes
> >>> (i.e., 170 blocks per pipe on BXT), then we're going to hit similar
> >>> issues when only driving one or two displays with with all of the planes
> >>> in use, even though we should have had more DDB space to work with.
> >>>
> >>> I guess serious plane usage isn't too common in desktop setups today,
> >>> but it's a very critical feature in the embedded world; we can't really
> >>> afford to cripple plane usage further.  Unfortunately, as you point out
> >>> above, this means that we have to follow the bspec's DDB allocation
> >>> method, which in turn means we need to grab _all_ CRTC locks any time
> >>> _any_ CRTC is being turned on or turned off which is a BKL-style way of
> >>> doing things.
> >> Meh, I'm running into a similar issue on vlv/chv. I don't see a way around it. :(
> > Now are you hitting it w/ vlv/chv? They don't even have a shared DDB.
> Not really the same, but determining what power saving levels + values to set with possibly parallel updates.

Just like ILK we just need to track reasonably closely how many pipes
are active, and redo the wm level selection when that changes. Just
make sure the code never thinks there are less pipes active than in
reality, and it should all be safe.

In theory we wouldn't even need to do that since the PM5/DDR DVFS don't
actually depend on the number of active pipes. It's just that they never
validated that combination, so it's not something that's recommended to
be used. And since the whole DDR DVFS extra latency vs. DDL deadlines
is such a mess, it probably would end up in more underruns.

-- 
Ville Syrjälä
Intel OTC