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(-)
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)
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 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);@@ -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
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)
From: Matt Roper matthew.d.roper@intel.com
When we write watermark values to the hardware, those values are stored in dev_priv->wm.skl_hw. However with recent watermark changes, the results structure we're copying from only contains valid watermark and DDB values for the pipes that are actually changing; the values for other pipes remain 0. Thus a blind copy of the entire skl_wm_values structure will clobber the values for unchanged pipes...we need to be more selective and only copy over the values for the changing pipes.
This mistake was hidden until recently due to another bug that caused us to erroneously re-calculate watermarks for all active pipes rather than changing pipes. Only when that bug was fixed was the impact of this bug discovered (e.g., modesets failing with "Requested display configuration exceeds system watermark limitations" messages and leaving watermarks non-functional, even ones initiated by intel_fbdev_restore_mode).
Changes since v1: - Add a function for copying a pipe's wm values (skl_copy_wm_for_pipe()) so we can reuse this later
Fixes: 734fa01f3a17 ("drm/i915/gen9: Calculate watermarks during atomic 'check' (v2)") Fixes: 9b6130227495 ("drm/i915/gen9: Re-allocate DDB only for changed pipes") Signed-off-by: Matt Roper matthew.d.roper@intel.com Signed-off-by: Lyude cpaul@redhat.com Reviewed-by: Matt Roper matthew.d.roper@intel.com Cc: stable@vger.kernel.org Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Radhakrishna Sripada radhakrishna.sripada@intel.com Cc: Hans de Goede hdegoede@redhat.com --- drivers/gpu/drm/i915/intel_pm.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 55a9694..8ae3f2b 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4049,6 +4049,24 @@ skl_compute_ddb(struct drm_atomic_state *state) return 0; }
+static void +skl_copy_wm_for_pipe(struct skl_wm_values *dst, + struct skl_wm_values *src, + enum pipe pipe) +{ + dst->wm_linetime[pipe] = src->wm_linetime[pipe]; + memcpy(dst->plane[pipe], src->plane[pipe], + sizeof(dst->plane[pipe])); + memcpy(dst->plane_trans[pipe], src->plane_trans[pipe], + sizeof(dst->plane_trans[pipe])); + + dst->ddb.pipe[pipe] = src->ddb.pipe[pipe]; + memcpy(dst->ddb.y_plane[pipe], src->ddb.y_plane[pipe], + sizeof(dst->ddb.y_plane[pipe])); + memcpy(dst->ddb.plane[pipe], src->ddb.plane[pipe], + sizeof(dst->ddb.plane[pipe])); +} + static int skl_compute_wm(struct drm_atomic_state *state) { @@ -4121,8 +4139,10 @@ static void skl_update_wm(struct drm_crtc *crtc) struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = to_i915(dev); struct skl_wm_values *results = &dev_priv->wm.skl_results; + struct skl_wm_values *hw_vals = &dev_priv->wm.skl_hw; struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state); struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal; + int pipe;
if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) return; @@ -4134,8 +4154,12 @@ static void skl_update_wm(struct drm_crtc *crtc) skl_write_wm_values(dev_priv, results); skl_flush_wm_values(dev_priv, results);
- /* store the new configuration */ - dev_priv->wm.skl_hw = *results; + /* + * Store the new configuration (but only for the pipes that have + * changed; the other values weren't recomputed). + */ + for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes) + skl_copy_wm_for_pipe(hw_vals, results, pipe);
mutex_unlock(&dev_priv->wm.wm_mutex); }
Manual pipe flushes are only necessary in order to make sure that we prevent pipes with changed ddb allocations from overlapping from one another at any point in time. Additionally, forcing us to wait for the next vblank every time we have to update the watermark values because the cursor was moving between screens will introduce a rather noticable lag for users.
Signed-off-by: Lyude cpaul@redhat.com Cc: stable@vger.kernel.org Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Radhakrishna Sripada radhakrishna.sripada@intel.com Cc: Hans de Goede hdegoede@redhat.com Cc: Matt Roper matthew.d.roper@intel.com --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1f6fe8c..d65e897 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1597,6 +1597,7 @@ struct skl_ddb_allocation {
struct skl_wm_values { unsigned dirty_pipes; + bool ddb_changed; struct skl_ddb_allocation ddb; uint32_t wm_linetime[I915_MAX_PIPES]; uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8]; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 8ae3f2b..cda4196 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3892,6 +3892,12 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv, new_ddb = &new_values->ddb; cur_ddb = &dev_priv->wm.skl_hw.ddb;
+ /* We only ever need to flush when the ddb allocations change */ + if (!new_values->ddb_changed) + return; + + new_values->ddb_changed = false; + /* * First pass: flush the pipes with the new allocation contained into * the old space. @@ -3996,6 +4002,22 @@ pipes_modified(struct drm_atomic_state *state) return ret; }
+static bool +skl_pipe_ddb_changed(struct skl_ddb_allocation *old, + struct skl_ddb_allocation *new, + enum pipe pipe) +{ + if (memcmp(&old->pipe[pipe], &new->pipe[pipe], + sizeof(old->pipe[pipe])) != 0 || + memcmp(&old->plane[pipe], &new->plane[pipe], + sizeof(old->plane[pipe])) != 0 || + memcmp(&old->y_plane[pipe], &new->y_plane[pipe], + sizeof(old->y_plane[pipe])) != 0) + return true; + + return false; +} + static int skl_compute_ddb(struct drm_atomic_state *state) { @@ -4003,7 +4025,8 @@ skl_compute_ddb(struct drm_atomic_state *state) struct drm_i915_private *dev_priv = to_i915(dev); struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct intel_crtc *intel_crtc; - struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb; + struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb; + struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb; uint32_t realloc_pipes = pipes_modified(state); int ret;
@@ -4041,9 +4064,13 @@ skl_compute_ddb(struct drm_atomic_state *state) if (IS_ERR(cstate)) return PTR_ERR(cstate);
- ret = skl_allocate_pipe_ddb(cstate, ddb); + ret = skl_allocate_pipe_ddb(cstate, new_ddb); if (ret) return ret; + + if (!intel_state->wm_results.ddb_changed && + skl_pipe_ddb_changed(old_ddb, new_ddb, intel_crtc->pipe)) + intel_state->wm_results.ddb_changed = true; }
return 0;
On Tue, Jul 26, 2016 at 01:34:39PM -0400, Lyude wrote:
Manual pipe flushes are only necessary in order to make sure that we prevent pipes with changed ddb allocations from overlapping from one another at any point in time. Additionally, forcing us to wait for the next vblank every time we have to update the watermark values because the cursor was moving between screens will introduce a rather noticable lag for users.
Signed-off-by: Lyude cpaul@redhat.com Cc: stable@vger.kernel.org Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Radhakrishna Sripada radhakrishna.sripada@intel.com Cc: Hans de Goede hdegoede@redhat.com Cc: Matt Roper matthew.d.roper@intel.com
drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1f6fe8c..d65e897 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1597,6 +1597,7 @@ struct skl_ddb_allocation {
struct skl_wm_values { unsigned dirty_pipes;
- bool ddb_changed; struct skl_ddb_allocation ddb; uint32_t wm_linetime[I915_MAX_PIPES]; uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 8ae3f2b..cda4196 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3892,6 +3892,12 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv, new_ddb = &new_values->ddb; cur_ddb = &dev_priv->wm.skl_hw.ddb;
- /* We only ever need to flush when the ddb allocations change */
- if (!new_values->ddb_changed)
return;- new_values->ddb_changed = false;
- /*
- First pass: flush the pipes with the new allocation contained into
- the old space.
@@ -3996,6 +4002,22 @@ pipes_modified(struct drm_atomic_state *state) return ret; }
+static bool +skl_pipe_ddb_changed(struct skl_ddb_allocation *old,
struct skl_ddb_allocation *new,enum pipe pipe)+{
- if (memcmp(&old->pipe[pipe], &new->pipe[pipe],
sizeof(old->pipe[pipe])) != 0 ||memcmp(&old->plane[pipe], &new->plane[pipe],sizeof(old->plane[pipe])) != 0 ||memcmp(&old->y_plane[pipe], &new->y_plane[pipe],sizeof(old->y_plane[pipe])) != 0)return true;- return false;
+}
static int skl_compute_ddb(struct drm_atomic_state *state) { @@ -4003,7 +4025,8 @@ skl_compute_ddb(struct drm_atomic_state *state) struct drm_i915_private *dev_priv = to_i915(dev); struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct intel_crtc *intel_crtc;
- struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb;
- struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb;
- struct skl_ddb_allocation *old_ddb = &dev_priv->wm.skl_hw.ddb; uint32_t realloc_pipes = pipes_modified(state); int ret;
@@ -4041,9 +4064,13 @@ skl_compute_ddb(struct drm_atomic_state *state) if (IS_ERR(cstate)) return PTR_ERR(cstate);
ret = skl_allocate_pipe_ddb(cstate, ddb);
ret = skl_allocate_pipe_ddb(cstate, new_ddb);if (!intel_state->wm_results.ddb_changed &&
I think you can simplify and drop the first part of this condition here, but in general,
Reviewed-by: Matt Roper matthew.d.roper@intel.com
skl_pipe_ddb_changed(old_ddb, new_ddb, intel_crtc->pipe))intel_state->wm_results.ddb_changed = true;}
return 0;
-- 2.7.4
Similar to how a vehicle will travel faster if you paint flames on it, cleaning up this extra whitespace is guaranteed to provide additional stability while updating watermark values.
Signed-off-by: Lyude cpaul@redhat.com --- drivers/gpu/drm/i915/intel_pm.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index cda4196..fb5d2eb 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3920,7 +3920,6 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv, reallocated[pipe] = true; }
- /* * Second pass: flush the pipes that are having their allocation * reduced, but overlapping with a previous allocation.
Thanks to Ville for suggesting this as a potential solution to pipe underruns on Skylake.
On Skylake all of the registers for configuring planes, including the registers for configuring their watermarks, are double buffered. New values written to them won't take effect until said registers are "armed", which is done by writing to the PLANE_SURF (or in the case of cursor planes, the CURBASE register) register.
With this in mind, up until now we've been updating watermarks on skl like this:
non-modeset { - calculate (during atomic check phase) - finish_atomic_commit: - intel_pre_plane_update: - intel_update_watermarks() - {vblank happens; new watermarks + old plane values => underrun } - drm_atomic_helper_commit_planes_on_crtc: - start vblank evasion - write new plane registers - end vblank evasion }
or
modeset { - calculate (during atomic check phase) - finish_atomic_commit: - crtc_enable: - intel_update_watermarks() - {vblank happens; new watermarks + old plane values => underrun } - drm_atomic_helper_commit_planes_on_crtc: - start vblank evasion - write new plane registers - end vblank evasion }
Now we update watermarks atomically like this:
non-modeset { - calculate (during atomic check phase) - finish_atomic_commit: - intel_pre_plane_update: - intel_update_watermarks() (wm values aren't written yet) - drm_atomic_helper_commit_planes_on_crtc: - start vblank evasion - write new plane registers - write new wm values - end vblank evasion }
modeset { - calculate (during atomic check phase) - finish_atomic_commit: - crtc_enable: - intel_update_watermarks() (actual wm values aren't written yet) - drm_atomic_helper_commit_planes_on_crtc: - start vblank evasion - write new plane registers - write new wm values - end vblank evasion }
So this patch moves all of the watermark writes into the right place; inside of the vblank evasion where we update all of the registers for each plane. While this patch doesn't fix everything, it does allow us to update the watermark values in the way the hardware expects us to.
Changes since original patch series: - Remove mutex_lock/mutex_unlock since they don't do anything and we're not touching global state - Move skl_write_cursor_wm/skl_write_plane_wm functions into intel_pm.c, make externally visible - Add skl_write_plane_wm calls to skl_update_plane - Fix conditional for for loop in skl_write_plane_wm (level < max_level should be level <= max_level) - Make diagram in commit more accurate to what's actually happening - Add Fixes:
Changes since v1: - Use IS_GEN9() instead of IS_SKYLAKE() since these fixes apply to more then just Skylake - Update description to make it clear this patch doesn't fix everything - Check if pipes were actually changed before writing watermarks
Changes since v2: - Write PIPE_WM_LINETIME during vblank evasion
Changes since v3: - Rebase against new SAGV patch changes
Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation") Signed-off-by: Lyude cpaul@redhat.com Cc: stable@vger.kernel.org Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Radhakrishna Sripada radhakrishna.sripada@intel.com Cc: Hans de Goede hdegoede@redhat.com Cc: Matt Roper matthew.d.roper@intel.com --- drivers/gpu/drm/i915/intel_display.c | 5 ++++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_pm.c | 58 ++++++++++++++++++++++++++---------- drivers/gpu/drm/i915/intel_sprite.c | 2 ++ 4 files changed, 51 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b80c051..cd67945 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3031,6 +3031,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane, intel_crtc->adjusted_x = x_offset; intel_crtc->adjusted_y = y_offset;
+ skl_write_plane_wm(intel_crtc, 0); + I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset); I915_WRITE(PLANE_SIZE(pipe, 0), plane_size); @@ -10261,6 +10263,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base, int pipe = intel_crtc->pipe; uint32_t cntl = 0;
+ if (IS_GEN9(dev_priv)) + skl_write_cursor_wm(intel_crtc); + if (plane_state && plane_state->visible) { cntl = MCURSOR_GAMMA_ENABLE; switch (plane_state->base.crtc_w) { diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 113bf48..e212ed9 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1711,6 +1711,8 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv, struct skl_ddb_allocation *ddb /* out */); int skl_enable_sagv(struct drm_i915_private *dev_priv); int skl_disable_sagv(struct drm_i915_private *dev_priv); +void skl_write_cursor_wm(struct intel_crtc *intel_crtc); +void skl_write_plane_wm(struct intel_crtc *intel_crtc, int plane); uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config); bool ilk_disable_lp_wm(struct drm_device *dev); int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fb5d2eb..d469ad2 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3783,6 +3783,47 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv, I915_WRITE(reg, 0); }
+void skl_write_plane_wm(struct intel_crtc *intel_crtc, + int plane) +{ + struct drm_crtc *crtc = &intel_crtc->base; + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = to_i915(dev); + struct skl_wm_values *wm = &dev_priv->wm.skl_results; + int level, max_level = ilk_wm_max_level(dev); + enum pipe pipe = intel_crtc->pipe; + + if (!(wm->dirty_pipes & drm_crtc_mask(crtc))) + return; + + I915_WRITE(PIPE_WM_LINETIME(pipe), wm->wm_linetime[pipe]); + + for (level = 0; level <= max_level; level++) { + I915_WRITE(PLANE_WM(pipe, plane, level), + wm->plane[pipe][plane][level]); + } + I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]); +} + +void skl_write_cursor_wm(struct intel_crtc *intel_crtc) +{ + struct drm_crtc *crtc = &intel_crtc->base; + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = to_i915(dev); + struct skl_wm_values *wm = &dev_priv->wm.skl_results; + int level, max_level = ilk_wm_max_level(dev); + enum pipe pipe = intel_crtc->pipe; + + if (!(wm->dirty_pipes & drm_crtc_mask(crtc))) + return; + + for (level = 0; level <= max_level; level++) { + I915_WRITE(CUR_WM(pipe, level), + wm->plane[pipe][PLANE_CURSOR][level]); + } + I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]); +} + static void skl_write_wm_values(struct drm_i915_private *dev_priv, const struct skl_wm_values *new) { @@ -3790,7 +3831,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv, struct intel_crtc *crtc;
for_each_intel_crtc(dev, crtc) { - int i, level, max_level = ilk_wm_max_level(dev); + int i; enum pipe pipe = crtc->pipe;
if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0) @@ -3798,21 +3839,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv, if (!crtc->active) continue;
- I915_WRITE(PIPE_WM_LINETIME(pipe), new->wm_linetime[pipe]); - - for (level = 0; level <= max_level; level++) { - for (i = 0; i < intel_num_planes(crtc); i++) - I915_WRITE(PLANE_WM(pipe, i, level), - new->plane[pipe][i][level]); - I915_WRITE(CUR_WM(pipe, level), - new->plane[pipe][PLANE_CURSOR][level]); - } - for (i = 0; i < intel_num_planes(crtc); i++) - I915_WRITE(PLANE_WM_TRANS(pipe, i), - new->plane_trans[pipe][i]); - I915_WRITE(CUR_WM_TRANS(pipe), - new->plane_trans[pipe][PLANE_CURSOR]); - for (i = 0; i < intel_num_planes(crtc); i++) { skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, i), diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 0de935a..50026f1 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -238,6 +238,8 @@ skl_update_plane(struct drm_plane *drm_plane, crtc_w--; crtc_h--;
+ skl_write_plane_wm(to_intel_crtc(crtc_state->base.crtc), plane); + if (key->flags) { I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value); I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
On Tue, Jul 26, 2016 at 01:34:41PM -0400, Lyude wrote:
Thanks to Ville for suggesting this as a potential solution to pipe underruns on Skylake.
On Skylake all of the registers for configuring planes, including the registers for configuring their watermarks, are double buffered. New values written to them won't take effect until said registers are "armed", which is done by writing to the PLANE_SURF (or in the case of cursor planes, the CURBASE register) register.
With this in mind, up until now we've been updating watermarks on skl like this:
non-modeset {
- calculate (during atomic check phase)
- finish_atomic_commit:
- intel_pre_plane_update:
- intel_update_watermarks()
- {vblank happens; new watermarks + old plane values => underrun }
- drm_atomic_helper_commit_planes_on_crtc:
- start vblank evasion
- write new plane registers
- end vblank evasion
}
or
modeset {
- calculate (during atomic check phase)
- finish_atomic_commit:
- crtc_enable:
- intel_update_watermarks()
- {vblank happens; new watermarks + old plane values => underrun }
- drm_atomic_helper_commit_planes_on_crtc:
- start vblank evasion
- write new plane registers
- end vblank evasion
}
Now we update watermarks atomically like this:
non-modeset {
- calculate (during atomic check phase)
- finish_atomic_commit:
- intel_pre_plane_update:
- intel_update_watermarks() (wm values aren't written yet)
- drm_atomic_helper_commit_planes_on_crtc:
- start vblank evasion
- write new plane registers
- write new wm values
- end vblank evasion
}
modeset {
- calculate (during atomic check phase)
- finish_atomic_commit:
- crtc_enable:
- intel_update_watermarks() (actual wm values aren't written yet)
- drm_atomic_helper_commit_planes_on_crtc:
- start vblank evasion
- write new plane registers
- write new wm values
- end vblank evasion
}
So this patch moves all of the watermark writes into the right place; inside of the vblank evasion where we update all of the registers for each plane. While this patch doesn't fix everything, it does allow us to update the watermark values in the way the hardware expects us to.
Changes since original patch series:
- Remove mutex_lock/mutex_unlock since they don't do anything and we're not touching global state
- Move skl_write_cursor_wm/skl_write_plane_wm functions into intel_pm.c, make externally visible
- Add skl_write_plane_wm calls to skl_update_plane
- Fix conditional for for loop in skl_write_plane_wm (level < max_level should be level <= max_level)
- Make diagram in commit more accurate to what's actually happening
- Add Fixes:
Changes since v1:
- Use IS_GEN9() instead of IS_SKYLAKE() since these fixes apply to more then just Skylake
- Update description to make it clear this patch doesn't fix everything
- Check if pipes were actually changed before writing watermarks
Changes since v2:
- Write PIPE_WM_LINETIME during vblank evasion
Changes since v3:
- Rebase against new SAGV patch changes
Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation") Signed-off-by: Lyude cpaul@redhat.com Cc: stable@vger.kernel.org Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Radhakrishna Sripada radhakrishna.sripada@intel.com Cc: Hans de Goede hdegoede@redhat.com Cc: Matt Roper matthew.d.roper@intel.com
drivers/gpu/drm/i915/intel_display.c | 5 ++++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_pm.c | 58 ++++++++++++++++++++++++++---------- drivers/gpu/drm/i915/intel_sprite.c | 2 ++ 4 files changed, 51 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index b80c051..cd67945 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3031,6 +3031,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane, intel_crtc->adjusted_x = x_offset; intel_crtc->adjusted_y = y_offset;
- skl_write_plane_wm(intel_crtc, 0);
- I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset); I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
@@ -10261,6 +10263,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base, int pipe = intel_crtc->pipe; uint32_t cntl = 0;
- if (IS_GEN9(dev_priv))
skl_write_cursor_wm(intel_crtc);- if (plane_state && plane_state->visible) { cntl = MCURSOR_GAMMA_ENABLE; switch (plane_state->base.crtc_w) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 113bf48..e212ed9 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1711,6 +1711,8 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv, struct skl_ddb_allocation *ddb /* out */); int skl_enable_sagv(struct drm_i915_private *dev_priv); int skl_disable_sagv(struct drm_i915_private *dev_priv); +void skl_write_cursor_wm(struct intel_crtc *intel_crtc); +void skl_write_plane_wm(struct intel_crtc *intel_crtc, int plane); uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config); bool ilk_disable_lp_wm(struct drm_device *dev); int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index fb5d2eb..d469ad2 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3783,6 +3783,47 @@ static void skl_ddb_entry_write(struct drm_i915_private *dev_priv, I915_WRITE(reg, 0); }
+void skl_write_plane_wm(struct intel_crtc *intel_crtc,
int plane)+{
- struct drm_crtc *crtc = &intel_crtc->base;
- struct drm_device *dev = crtc->dev;
- struct drm_i915_private *dev_priv = to_i915(dev);
- struct skl_wm_values *wm = &dev_priv->wm.skl_results;
- int level, max_level = ilk_wm_max_level(dev);
- enum pipe pipe = intel_crtc->pipe;
- if (!(wm->dirty_pipes & drm_crtc_mask(crtc)))
return;- I915_WRITE(PIPE_WM_LINETIME(pipe), wm->wm_linetime[pipe]);
Putting this here may lead to it being programmed multiple times or not at all, depending on what pipes are in use. Worst case is you have the CRTC on with no planes except the cursor (and possibly the canvas), but do a modeset or pipe scaling change that needs to change the linetime watermark.
I guess we probably need a new device-specific callback from intel_update_pipe_config() (which gets called by intel_begin_crtc_commit()) where we can do this once per CRTC.
Matt
- for (level = 0; level <= max_level; level++) {
I915_WRITE(PLANE_WM(pipe, plane, level),wm->plane[pipe][plane][level]);- }
- I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]);
+}
+void skl_write_cursor_wm(struct intel_crtc *intel_crtc) +{
- struct drm_crtc *crtc = &intel_crtc->base;
- struct drm_device *dev = crtc->dev;
- struct drm_i915_private *dev_priv = to_i915(dev);
- struct skl_wm_values *wm = &dev_priv->wm.skl_results;
- int level, max_level = ilk_wm_max_level(dev);
- enum pipe pipe = intel_crtc->pipe;
- if (!(wm->dirty_pipes & drm_crtc_mask(crtc)))
return;- for (level = 0; level <= max_level; level++) {
I915_WRITE(CUR_WM(pipe, level),wm->plane[pipe][PLANE_CURSOR][level]);- }
- I915_WRITE(CUR_WM_TRANS(pipe), wm->plane_trans[pipe][PLANE_CURSOR]);
+}
static void skl_write_wm_values(struct drm_i915_private *dev_priv, const struct skl_wm_values *new) { @@ -3790,7 +3831,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv, struct intel_crtc *crtc;
for_each_intel_crtc(dev, crtc) {
int i, level, max_level = ilk_wm_max_level(dev);
int i;enum pipe pipe = crtc->pipe;
if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0)
@@ -3798,21 +3839,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv, if (!crtc->active) continue;
I915_WRITE(PIPE_WM_LINETIME(pipe), new->wm_linetime[pipe]);for (level = 0; level <= max_level; level++) {for (i = 0; i < intel_num_planes(crtc); i++)I915_WRITE(PLANE_WM(pipe, i, level),new->plane[pipe][i][level]);I915_WRITE(CUR_WM(pipe, level),new->plane[pipe][PLANE_CURSOR][level]);}for (i = 0; i < intel_num_planes(crtc); i++)I915_WRITE(PLANE_WM_TRANS(pipe, i),new->plane_trans[pipe][i]);I915_WRITE(CUR_WM_TRANS(pipe),new->plane_trans[pipe][PLANE_CURSOR]);- for (i = 0; i < intel_num_planes(crtc); i++) { skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, i),
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 0de935a..50026f1 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -238,6 +238,8 @@ skl_update_plane(struct drm_plane *drm_plane, crtc_w--; crtc_h--;
- skl_write_plane_wm(to_intel_crtc(crtc_state->base.crtc), plane);
- if (key->flags) { I915_WRITE(PLANE_KEYVAL(pipe, plane), key->min_value); I915_WRITE(PLANE_KEYMAX(pipe, plane), key->max_value);
-- 2.7.4
Unfortunately right now we don't really update watermarks on Skylake properly, since ideally we'd be updating both the ddb allocations, plane properties, and watermarks all in a single go. Until this is fixed however, we can improve things somewhat by adding a vblank wait after the third iteration of pipe flushes, since this forces us to always wait for new ddb allocations to take affect before trying to change them again.
Signed-off-by: Lyude cpaul@redhat.com Cc: stable@vger.kernel.org Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Daniel Vetter daniel.vetter@intel.com Cc: Radhakrishna Sripada radhakrishna.sripada@intel.com Cc: Hans de Goede hdegoede@redhat.com Cc: Matt Roper matthew.d.roper@intel.com --- drivers/gpu/drm/i915/intel_pm.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index d469ad2..a26dbd9 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3973,8 +3973,11 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv, /* * Third pass: flush the pipes that got more space allocated. * - * We don't need to actively wait for the update here, next vblank - * will just get more DDB space with the correct WM values. + * While the hardware doesn't require to wait for the next vblank here, + * continuing before the pipe finishes updating could result in us + * trying to update the wm values again before the pipe finishes + * updating, which results in the hardware using intermediate wm values + * and subsequently underrunning pipes. */ for_each_intel_crtc(dev, crtc) { if (!crtc->active) @@ -3990,6 +3993,16 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv, continue;
skl_wm_flush_pipe(dev_priv, pipe, 3); + + /* + * The only time we can get away with not waiting for an update + * is when we just enabled the pipe, e.g. when it doesn't have + * vblanks enabled anyway. + */ + if (drm_crtc_vblank_get(&crtc->base) == 0) { + intel_wait_for_vblank(dev, pipe); + drm_crtc_vblank_put(&crtc->base); + } } }
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
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_ddbBasically 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
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
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
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
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
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
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_ddbBasically 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
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_ddbBasically 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.
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_ddbBasically 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.
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_ddbBasically 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
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_ddbBasically 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.
dri-devel@lists.freedesktop.org
-
Hans de Goede -
Lyude -
Maarten Lankhorst -
Matt Roper -
Ville Syrjälä