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 v8: - Add intel_state->modeset guard to the conditional for skl_enable_sagv() Changes since v7: - Remove GEN9_SAGV_LOW_FREQ, replace with GEN9_SAGV_IS_ENABLED (that's all we use it for anyway) - Use GEN9_SAGV_IS_ENABLED instead of 0x1 for clarification - Fix a styling error that snuck past me Changes since v6: - Protect skl_enable_sagv() with intel_state->modeset conditional in intel_atomic_commit_tail() Changes since v5: - Don't use is_power_of_2. Makes things confusing - Don't use the old state to figure out whether or not to enable/disable the sagv, use the new one - Split the loop in skl_disable_sagv into it's own function - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail() 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 Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.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 | 4 ++ drivers/gpu/drm/i915/intel_display.c | 11 ++++ drivers/gpu/drm/i915/intel_drv.h | 2 + drivers/gpu/drm/i915/intel_pm.c | 112 +++++++++++++++++++++++++++++++++++ 5 files changed, 131 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index feec00f..eb449f6 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1948,6 +1948,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 f38a5e2..f7e0bc2 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7170,6 +7170,10 @@ 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_IS_DISABLED 0x1 +#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 9cbf543..814d07d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13698,6 +13698,14 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)) dev_priv->display.modeset_commit_cdclk(state);

+ /* + * SKL workaround: bspec recommends we disable the SAGV when we + * have more then one pipe enabled + */ + if (IS_SKYLAKE(dev_priv) && + hweight32(intel_state->active_crtcs) > 1) + skl_disable_sagv(dev_priv); + intel_modeset_verify_disabled(dev); }

@@ -13771,6 +13779,9 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state); }

+ if (intel_state->modeset && hweight32(intel_state->active_crtcs) <= 1) + skl_enable_sagv(dev_priv); + drm_atomic_helper_commit_hw_done(state);

if (intel_state->modeset) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 1ad2e2c5..221cbe0 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1700,6 +1700,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 aef0b10..0c52c50 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2884,6 +2884,116 @@ skl_wm_plane_id(const struct intel_plane *plane) }

static void +skl_sagv_get_hw_state(struct drm_i915_private *dev_priv) +{ + u32 temp; + int ret; + + if (IS_BROXTON(dev_priv)) + return; + + mutex_lock(&dev_priv->rps.hw_lock); + ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL, &temp); + mutex_unlock(&dev_priv->rps.hw_lock); + + if (!ret) { + dev_priv->skl_sagv_enabled = !(temp & GEN9_SAGV_IS_DISABLED); + } else { + /* + * If for some reason we can't access the SAGV state, follow + * the bspec and assume it's enabled + */ + DRM_ERROR("Failed to get SAGV state, assuming enabled\n"); + dev_priv->skl_sagv_enabled = true; + } +} + +/* + * SAGV dynamically adjusts the system agent voltage and clock frequencies + * depending on power and performance requirements. The display engine access + * to system memory is blocked during the adjustment time. Having this enabled + * in multi-pipe configurations can cause issues (such as underruns causing + * full system hangs), and the bspec also suggests that software disable it + * when more then one pipe is enabled. + */ +int +skl_enable_sagv(struct drm_i915_private *dev_priv) +{ + int ret; + + if (IS_BROXTON(dev_priv)) + return 0; + if (dev_priv->skl_sagv_enabled) + return 0; + + mutex_lock(&dev_priv->rps.hw_lock); + DRM_DEBUG_KMS("Enabling the SAGV\n"); + + ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL, + GEN9_SAGV_DYNAMIC_FREQ); + if (!ret) + dev_priv->skl_sagv_enabled = true; + else + DRM_ERROR("Failed to enable the SAGV\n"); + + /* We don't need to wait for SAGV when enabling */ + mutex_unlock(&dev_priv->rps.hw_lock); + return ret; +} + +static int +skl_do_sagv_disable(struct drm_i915_private *dev_priv) +{ + int ret; + uint32_t temp; + + ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL, + GEN9_SAGV_DISABLE); + if (ret) { + DRM_ERROR("Failed to disable the SAGV\n"); + return ret; + } + + ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL, + &temp); + if (ret) { + DRM_ERROR("Failed to check the status of the SAGV\n"); + return ret; + } + + return temp & GEN9_SAGV_IS_DISABLED; +} + +int +skl_disable_sagv(struct drm_i915_private *dev_priv) +{ + int ret, result; + + if (IS_BROXTON(dev_priv)) + return 0; + if (!dev_priv->skl_sagv_enabled) + return 0; + + mutex_lock(&dev_priv->rps.hw_lock); + DRM_DEBUG_KMS("Disabling the SAGV\n"); + + /* bspec says to keep retrying for at least 1 ms */ + ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1); + mutex_unlock(&dev_priv->rps.hw_lock); + + if (ret == -ETIMEDOUT) { + DRM_ERROR("Request to disable SAGV timed out\n"); + } else { + if (result == GEN9_SAGV_IS_DISABLED) + dev_priv->skl_sagv_enabled = false; + + ret = result; + } + + return ret; +} + +static void skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, const struct intel_crtc_state *cstate, struct skl_ddb_entry *alloc, /* out */ @@ -4238,6 +4348,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)

If we're enabling a pipe, we'll need to modify the watermarks on all active planes. Since those planes won't be added to the state on their own, we need to add them ourselves.

Signed-off-by: Lyude cpaul@redhat.com Reviewed-by: Matt Roper matthew.d.roper@intel.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 --- drivers/gpu/drm/i915/intel_pm.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index e539a41..3f23e5e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4079,6 +4079,10 @@ skl_compute_ddb(struct drm_atomic_state *state) ret = skl_allocate_pipe_ddb(cstate, ddb); if (ret) return ret; + + ret = drm_atomic_add_affected_planes(state, &intel_crtc->base); + if (ret) + return ret; }

return 0;

Now that we can hook into update_crtcs and control the order in which we update CRTCs at each modeset, we can finish the final step of fixing Skylake's watermark handling by performing DDB updates at the same time as plane updates and watermark updates.

The first major change in this patch is skl_update_crtcs(), which handles ensuring that we order each CRTC update in our atomic commits properly so that they honor the DDB flush order.

The second major change in this patch is the order in which we flush the pipes. While the previous order may have worked, it can't be used in this approach since it no longer will do the right thing. For example, using the old ddb flush order:

We have pipes A, B, and C enabled, and we're disabling C. Initial ddb allocation looks like this:

| A | B |xxxxxxx|

Since we're performing the ddb updates after performing any CRTC disablements in intel_atomic_commit_tail(), the space to the right of pipe B is unallocated.

1. Flush pipes with new allocation contained into old space. None apply, so we skip this 2. Flush pipes having their allocation reduced, but overlapping with a previous allocation. None apply, so we also skip this 3. Flush pipes that got more space allocated. This applies to A and B, giving us the following update order: A, B

This is wrong, since updating pipe A first will cause it to overlap with B and potentially burst into flames. Our new order (see the code comments for details) would update the pipes in the proper order: B, A.

As well, we calculate the order for each DDB update during the check phase, and reference it later in the commit phase when we hit skl_update_crtcs().

This long overdue patch fixes the rest of the underruns on Skylake.

Changes since v1: - Add skl_ddb_entry_write() for cursor into skl_write_cursor_wm() Changes since v2: - Use the method for updating CRTCs that Ville suggested - In skl_update_wm(), only copy the watermarks for the crtc that was passed to us

Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration") Fixes: 8211bd5bdf5e ("drm/i915/skl: Program the DDB allocation") Signed-off-by: Lyude cpaul@redhat.com [omitting CC for stable, since this patch will need to be changed for such backports first] 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 | 100 +++++++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 7 ++ drivers/gpu/drm/i915/intel_pm.c | 207 +++++++++-------------------------- 3 files changed, 144 insertions(+), 170 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 980b6fd..ad5f6e5 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12903,16 +12903,23 @@ static void verify_wm_state(struct drm_crtc *crtc, hw_entry->start, hw_entry->end); }

- /* cursor */ - hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR]; - sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR]; - - if (!skl_ddb_entry_equal(hw_entry, sw_entry)) { - DRM_ERROR("mismatch in DDB state pipe %c cursor " - "(expected (%u,%u), found (%u,%u))\n", - pipe_name(pipe), - sw_entry->start, sw_entry->end, - hw_entry->start, hw_entry->end); + /* + * cursor + * If the cursor plane isn't active, we may not have updated it's ddb + * allocation. In that case since the ddb allocation will be updated + * once the plane becomes visible, we can skip this check + */ + if (intel_crtc->cursor_addr) { + hw_entry = &hw_ddb.plane[pipe][PLANE_CURSOR]; + sw_entry = &sw_ddb->plane[pipe][PLANE_CURSOR]; + + if (!skl_ddb_entry_equal(hw_entry, sw_entry)) { + DRM_ERROR("mismatch in DDB state pipe %c cursor " + "(expected (%u,%u), found (%u,%u))\n", + pipe_name(pipe), + sw_entry->start, sw_entry->end, + hw_entry->start, hw_entry->end); + } } }

@@ -13664,6 +13671,72 @@ static void intel_update_crtcs(struct drm_atomic_state *state, } }

+static void skl_update_crtcs(struct drm_atomic_state *state, + unsigned int *crtc_vblank_mask) +{ + struct drm_device *dev = state->dev; + struct drm_i915_private *dev_priv = to_i915(dev); + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); + struct drm_crtc *crtc; + struct drm_crtc_state *old_crtc_state; + struct skl_ddb_allocation *new_ddb = &intel_state->wm_results.ddb; + struct skl_ddb_allocation cur_ddb; + bool progress; + bool reallocated[I915_MAX_PIPES] = {}; + enum pipe pipe; + int wait_vbl_pipes, i; + + /* + * Whenever the number of active pipes change, so does the DDB + * allocation. DDB allocations on pipes cannot ever overlap with + * eachother at any point in time, so we need to change the order we + * update the pipes so that we ensure they never overlap inbetween DDB + * updates. + */ + do { + progress = false; + wait_vbl_pipes = 0; + cur_ddb = dev_priv->wm.skl_hw.ddb; + + for_each_crtc_in_state(state, crtc, old_crtc_state, i) { + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + pipe = intel_crtc->pipe; + + if (!intel_crtc->active || needs_modeset(crtc->state)) + continue; + if (skl_ddb_allocation_equals(&cur_ddb, new_ddb, pipe)) + continue; + if (skl_ddb_allocation_overlaps(state, &cur_ddb, + new_ddb, pipe)) + continue; + + intel_update_crtc(crtc, state, old_crtc_state, + crtc_vblank_mask); + + wait_vbl_pipes |= drm_crtc_mask(crtc); + reallocated[pipe] = true; + progress = true; + } + + /* Wait for each pipe's new allocation to take effect */ + intel_atomic_wait_for_vblanks(dev, dev_priv, wait_vbl_pipes); + } while (progress); + + /* + * Now that we've handled any ddb reallocations, we can go ahead and + * enable any new pipes. + */ + for_each_crtc_in_state(state, crtc, old_crtc_state, i) { + pipe = to_intel_crtc(crtc)->pipe; + + if (reallocated[pipe] || !crtc->state->active) + continue; + + intel_update_crtc(crtc, state, old_crtc_state, + crtc_vblank_mask); + } +} + static void intel_atomic_commit_tail(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; @@ -15233,8 +15306,6 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv) dev_priv->display.crtc_disable = i9xx_crtc_disable; }

- dev_priv->display.update_crtcs = intel_update_crtcs; - /* Returns the core display clock speed */ if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) dev_priv->display.get_display_clock_speed = @@ -15324,6 +15395,11 @@ void intel_init_display_hooks(struct drm_i915_private *dev_priv) skl_modeset_calc_cdclk; }

+ if (dev_priv->info.gen >= 9) + dev_priv->display.update_crtcs = skl_update_crtcs; + else + dev_priv->display.update_crtcs = intel_update_crtcs; + switch (INTEL_INFO(dev_priv)->gen) { case 2: dev_priv->display.queue_flip = intel_gen2_queue_flip; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 1aedd9f..4b149dc 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1702,6 +1702,13 @@ 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); +bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old, + const struct skl_ddb_allocation *new, + enum pipe pipe); +bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state, + const struct skl_ddb_allocation *old, + const struct skl_ddb_allocation *new, + enum pipe pipe); void skl_write_cursor_wm(struct intel_crtc *intel_crtc, const struct skl_wm_values *wm); void skl_write_plane_wm(struct intel_crtc *intel_crtc, diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 3f23e5e..a804908 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3815,6 +3815,11 @@ void skl_write_plane_wm(struct intel_crtc *intel_crtc, wm->plane[pipe][plane][level]); } I915_WRITE(PLANE_WM_TRANS(pipe, plane), wm->plane_trans[pipe][plane]); + + skl_ddb_entry_write(dev_priv, PLANE_BUF_CFG(pipe, plane), + &wm->ddb.plane[pipe][plane]); + skl_ddb_entry_write(dev_priv, PLANE_NV12_BUF_CFG(pipe, plane), + &wm->ddb.y_plane[pipe][plane]); }

void skl_write_cursor_wm(struct intel_crtc *intel_crtc, @@ -3831,170 +3836,49 @@ void skl_write_cursor_wm(struct intel_crtc *intel_crtc, 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) -{ - struct drm_device *dev = &dev_priv->drm; - struct intel_crtc *crtc;

- for_each_intel_crtc(dev, crtc) { - int i; - enum pipe pipe = crtc->pipe; - - if ((new->dirty_pipes & drm_crtc_mask(&crtc->base)) == 0) - continue; - if (!crtc->active) - continue; - - for (i = 0; i < intel_num_planes(crtc); i++) { - skl_ddb_entry_write(dev_priv, - PLANE_BUF_CFG(pipe, i), - &new->ddb.plane[pipe][i]); - skl_ddb_entry_write(dev_priv, - PLANE_NV12_BUF_CFG(pipe, i), - &new->ddb.y_plane[pipe][i]); - } - - skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe), - &new->ddb.plane[pipe][PLANE_CURSOR]); - } + skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe), + &wm->ddb.plane[pipe][PLANE_CURSOR]); }

-/* - * When setting up a new DDB allocation arrangement, we need to correctly - * sequence the times at which the new allocations for the pipes are taken into - * account or we'll have pipes fetching from space previously allocated to - * another pipe. - * - * Roughly the sequence looks like: - * 1. re-allocate the pipe(s) with the allocation being reduced and not - * overlapping with a previous light-up pipe (another way to put it is: - * pipes with their new allocation strickly included into their old ones). - * 2. re-allocate the other pipes that get their allocation reduced - * 3. allocate the pipes having their allocation increased - * - * Steps 1. and 2. are here to take care of the following case: - * - Initially DDB looks like this: - * | B | C | - * - enable pipe A. - * - pipe B has a reduced DDB allocation that overlaps with the old pipe C - * allocation - * | A | B | C | - * - * We need to sequence the re-allocation: C, B, A (and not B, C, A). - */ - -static void -skl_wm_flush_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, int pass) -{ - int plane; - - DRM_DEBUG_KMS("flush pipe %c (pass %d)\n", pipe_name(pipe), pass); - - for_each_plane(dev_priv, pipe, plane) { - I915_WRITE(PLANE_SURF(pipe, plane), - I915_READ(PLANE_SURF(pipe, plane))); - } - I915_WRITE(CURBASE(pipe), I915_READ(CURBASE(pipe))); -} - -static bool -skl_ddb_allocation_included(const struct skl_ddb_allocation *old, - const struct skl_ddb_allocation *new, - enum pipe pipe) +bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old, + const struct skl_ddb_allocation *new, + enum pipe pipe) { - uint16_t old_size, new_size; - - old_size = skl_ddb_entry_size(&old->pipe[pipe]); - new_size = skl_ddb_entry_size(&new->pipe[pipe]); - - return old_size != new_size && - new->pipe[pipe].start >= old->pipe[pipe].start && - new->pipe[pipe].end <= old->pipe[pipe].end; + return new->pipe[pipe].start == old->pipe[pipe].start && + new->pipe[pipe].end == old->pipe[pipe].end; }

-static void skl_flush_wm_values(struct drm_i915_private *dev_priv, - struct skl_wm_values *new_values) +bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state, + const struct skl_ddb_allocation *old, + const struct skl_ddb_allocation *new, + enum pipe pipe) { - struct drm_device *dev = &dev_priv->drm; - struct skl_ddb_allocation *cur_ddb, *new_ddb; - bool reallocated[I915_MAX_PIPES] = {}; - struct intel_crtc *crtc; - enum pipe pipe; - - new_ddb = &new_values->ddb; - cur_ddb = &dev_priv->wm.skl_hw.ddb; - - /* - * First pass: flush the pipes with the new allocation contained into - * the old space. - * - * We'll wait for the vblank on those pipes to ensure we can safely - * re-allocate the freed space without this pipe fetching from it. - */ - for_each_intel_crtc(dev, crtc) { - if (!crtc->active) - continue; - - pipe = crtc->pipe; - - if (!skl_ddb_allocation_included(cur_ddb, new_ddb, pipe)) - continue; - - skl_wm_flush_pipe(dev_priv, pipe, 1); - intel_wait_for_vblank(dev, pipe); - - reallocated[pipe] = true; - } - - - /* - * Second pass: flush the pipes that are having their allocation - * reduced, but overlapping with a previous allocation. - * - * Here as well we need to wait for the vblank to make sure the freed - * space is not used anymore. - */ - for_each_intel_crtc(dev, crtc) { - if (!crtc->active) - continue; - - pipe = crtc->pipe; - - if (reallocated[pipe]) - continue; - - if (skl_ddb_entry_size(&new_ddb->pipe[pipe]) < - skl_ddb_entry_size(&cur_ddb->pipe[pipe])) { - skl_wm_flush_pipe(dev_priv, pipe, 2); - intel_wait_for_vblank(dev, pipe); - reallocated[pipe] = true; - } - } - - /* - * 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. - */ - for_each_intel_crtc(dev, crtc) { - if (!crtc->active) - continue; + struct drm_device *dev = state->dev; + struct intel_crtc *intel_crtc; + enum pipe otherp;

- pipe = crtc->pipe; + for_each_intel_crtc(dev, intel_crtc) { + otherp = intel_crtc->pipe;

/* - * At this point, only the pipes more space than before are - * left to re-allocate. + * When checking for overlaps, we don't want to: + * - Compare against ourselves + * - Compare against pipes that will be disabled in step 0 + * - Compare against pipes that won't be enabled until step 3 */ - if (reallocated[pipe]) + if (otherp == pipe || !new->pipe[otherp].end || + !old->pipe[otherp].end) continue;

- skl_wm_flush_pipe(dev_priv, pipe, 3); + if ((new->pipe[pipe].start >= old->pipe[otherp].start && + new->pipe[pipe].start < old->pipe[otherp].end) || + (old->pipe[otherp].start >= new->pipe[pipe].start && + old->pipe[otherp].start < new->pipe[pipe].end)) + return true; } + + return false; }

static int skl_update_pipe_wm(struct drm_crtc_state *cstate, @@ -4181,7 +4065,7 @@ static void skl_update_wm(struct drm_crtc *crtc) 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; + enum pipe pipe = intel_crtc->pipe;

if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) return; @@ -4190,15 +4074,22 @@ static void skl_update_wm(struct drm_crtc *crtc)

mutex_lock(&dev_priv->wm.wm_mutex);

- skl_write_wm_values(dev_priv, results); - skl_flush_wm_values(dev_priv, results); - /* - * Store the new configuration (but only for the pipes that have - * changed; the other values weren't recomputed). + * If this pipe isn't active already, we're going to be enabling it + * very soon. Since it's safe to update a pipe's ddb allocation while + * the pipe's shut off, just do so here. Already active pipes will have + * their watermarks updated once we update their planes. */ - for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes) - skl_copy_wm_for_pipe(hw_vals, results, pipe); + if (crtc->state->active_changed) { + int plane; + + for (plane = 0; plane < intel_num_planes(intel_crtc); plane++) + skl_write_plane_wm(intel_crtc, results, plane); + + skl_write_cursor_wm(intel_crtc, results); + } + + skl_copy_wm_for_pipe(hw_vals, results, pipe);

mutex_unlock(&dev_priv->wm.wm_mutex); }