Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450714 > unrolled thread

[PATCH v4 0/6] Finally fix watermarks

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 0/6] Finally fix watermarks Lyude <cpaul@redhat.com> - 2016-07-26 19:40 +0200
    [PATCH v4 1/6] drm/i915/skl: Add support for the SAGV, fix underrun hangs Lyude <cpaul@redhat.com> - 2016-07-26 19:40 +0200
      Re: [PATCH v4 1/6] drm/i915/skl: Add support for the SAGV, fix  underrun hangs Matt Roper <matthew.d.roper@intel.com> - 2016-07-28 15:20 +0200
      Re: [v4,1/6] drm/i915/skl: Add support for the SAGV, fix underrun  hangs Hans de Goede <hdegoede@redhat.com> - 2016-08-02 18:30 +0200
    Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks Matt Roper <matthew.d.roper@intel.com> - 2016-07-29 02:10 +0200
      Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-07-29 11:40 +0200
        Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks Lyude <cpaul@redhat.com> - 2016-07-29 20:50 +0200
          Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-07-29 21:30 +0200
            Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks Lyude <cpaul@redhat.com> - 2016-07-29 21:50 +0200
            Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks Matt Roper <matthew.d.roper@intel.com> - 2016-07-29 22:50 +0200
              Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-08-01 14:00 +0200
        Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks Matt Roper <matthew.d.roper@intel.com> - 2016-07-29 22:40 +0200
          Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks Maarten Lankhorst <maarten.lankhorst@linux.intel.com> - 2016-08-01 11:00 +0200
            Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-08-01 13:50 +0200
              Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks Maarten Lankhorst <maarten.lankhorst@linux.intel.com> - 2016-08-02 18:00 +0200
                Re: [Intel-gfx] [PATCH v4 0/6] Finally fix watermarks Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-08-02 18:10 +0200

#1450714 — [PATCH v4 0/6] Finally fix watermarks

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

[toc] | [next] | [standalone]

#1450715 — [PATCH v4 1/6] drm/i915/skl: Add support for the SAGV, fix underrun hangs

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web