On Wed, Jul 20, 2016 at 04:59:57PM -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

}

Which is more of a step in the right direction to fixing all of the underrun issues we're currently seeing with skl

So my understanding of this patch is that it moves watermark value writes to the right place (inside the vblank evasion where we write the rest of the plane registers), but we aren't tackling the problems with DDB writes & flushing order yet, so we don't expect everything to be fixed yet, right? You might want to clarify that slightly in the commit message here.

One other comment inline below.

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:

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 cpaul@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 | 48 +++++++++++++++++++++++++----------- drivers/gpu/drm/i915/intel_sprite.c | 2 ++ 4 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 78beb7e..c0d2074 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);

@@ -10242,6 +10244,9 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base, int pipe = intel_crtc->pipe; uint32_t cntl = 0;

• if (IS_SKYLAKE(dev_priv))

I believe this should be IS_GEN9 so that it applies to BXT and KBL too.

Matt

• 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 e74d851..f1f54d9 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 */); +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 64d628c..fa86bea 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3680,6 +3680,39 @@ 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;
• 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;
• 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) { @@ -3687,7 +3720,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)

@@ -3697,19 +3730,6 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,

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

On Wed, Jul 20, 2016 at 04:59:58PM -0400, Lyude wrote:

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

Your changes look good to me.

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

Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com 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

---

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 fa86bea..b7d4af1 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3966,6 +3966,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) { @@ -4038,8 +4056,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;

@@ -4051,8 +4071,12 @@ static void skl_update_wm(struct drm_crtc *crtc) skl_write_wm_values(dev_priv, results); skl_flush_wm_values(dev_priv, results);

• /* store the new configuration */
• dev_priv->wm.skl_hw = *results;

• /*

• * Store the new configuration (but only for the pipes that have
  

• * changed; the other values weren't recomputed).
  

• */
  

• for_each_pipe_masked(dev_priv, pipe, results->dirty_pipes)

• skl_copy_wm_for_pipe(hw_vals, results, pipe);
  
  

  mutex_unlock(&dev_priv->wm.wm_mutex);

}

2.7.4

On Wed, Jul 20, 2016 at 04:59:59PM -0400, Lyude wrote:

Up until now we've actually been making the mistake of leaving the watermark results for each pipe completely blank in skl_compute_wm() when they haven't changed, fix this.

Should this be moved before patch #1? With the existing code we don't try to re-write watermark registers if they aren't changing, so leaving them at zero should be safe. I think we want to make this change before we start re-writing non-dirty watermarks. Alternatively, we could just add the dirty bit test to the appropriate places in patch #1 and not worry about copying over the unchanged values.

Matt

Fixes: 734fa01f3a17 ("drm/i915/gen9: Calculate watermarks during atomic 'check' (v2)") 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 cpaul@redhat.com Cc: Matt Roper matthew.d.roper@intel.com Signed-off-by: Lyude cpaul@redhat.com

---

drivers/gpu/drm/i915/intel_pm.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index b7d4af1..788db86 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3987,10 +3987,13 @@ skl_copy_wm_for_pipe(struct skl_wm_values *dst, static int skl_compute_wm(struct drm_atomic_state *state) {

• struct drm_device *dev = state->dev;
• struct drm_i915_private *dev_priv = to_i915(dev); struct drm_crtc *crtc; struct drm_crtc_state *cstate; struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct skl_wm_values *results = &intel_state->wm_results;
• struct skl_wm_values *hw_wm = &dev_priv->wm.skl_hw; struct skl_pipe_wm *pipe_wm; bool changed = false; int ret, i;

@@ -4039,12 +4042,14 @@ skl_compute_wm(struct drm_atomic_state *state) if (changed) results->dirty_pipes |= drm_crtc_mask(crtc);

• if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0)
  

• if ((results->dirty_pipes & drm_crtc_mask(crtc)) == 0) {
  /* This pipe's WM's did not change */
  

• 	skl_copy_wm_for_pipe(results, hw_wm, intel_crtc->pipe);
  continue;
  

• }
  
  

  intel_cstate->update_wm_pre = true;

• skl_compute_wm_results(crtc->dev, pipe_wm, results, intel_crtc);
  

• skl_compute_wm_results(dev, pipe_wm, results, intel_crtc);
  

  }

  return 0;

-- 2.7.4

On Wed, Jul 20, 2016 at 05:00:00PM -0400, Lyude wrote:

As we've learned, all watermark updates on Skylake have to be strictly atomic or things fail. While the bspec doesn't mandate that we need to wait for pipes to finish after the third iteration of flushes, not doing so gives us the opportunity to break this atomicity later. This example assumes that we're lucky enough not to be interrupted by the scheduler at any point during this:

• Start with pipe A and pipe B enabled
• Enable pipe C
• Flush pipe A in pass 1, wait until update finishes
• Flush pipe B in pass 3, continue without waiting for next vblank
• Start another wm update
• We enter the next vblank for pipe B before we finish writing all the vm values
• *Underrun*

As such, we always need to wait for each pipe we flush to update so as to never break this atomicity.

I'm not sure I follow this commit. If we're enabling a new pipe, the the allocation for A and B are generally going to shrink, so they'll usually be flushed in passes 1 and 2, not 3.

My understanding is that the problem that still remains (now that your first three patches have made progress towards fixing underruns) is that our DDB updates and flushes (which come from update_watermarks) happen pre-evasion, whereas the watermarks themselves now happen under evasion. We really want both the new DDB value and the new watermark value to be written together and take effect on the same vblank. I think the problem is that you might have a shrinking DDB allocation (e.g., because a new pipe was added or you changed a mode that changed the DDB balance) which some of the existing WM values exceed. You can have a sequence like this:

- update_wm: - write new (smaller) DDB - flush DDB - vblank happens, old (big) wm + new (small) ddb = underrun - vblank evasion: - write new plane regs and WM's - flush - post-evasion vblank happens, underrun is corrected

I think ultimately we want to move the DDB register writes into the update functions that happen under evasion, just like you did for the WM registers. However just doing this the straightforward way won't satisfy our requirements about pipe update ordering (the three passes you see today in skl_flush_wm_values). To make that work, I think the general approach is that we need to basically replace the for_each_crtc_in_state() loop in intel_atomic_commit_tail() with some new CRTC iterator that processes CRTC's in a more intelligent ordering. We've computed our DDB changes during the atomic check phase, so we already know which allocations are shrinking, growing, etc. and we should be able to calculate an appropriate CRTC ordering at the same time.

With an intelligent CRTC iterator that follows the pre-computed pipe ordering rules (and adds the necessary vblank waits between each "phase"), I think we should be able to just write both DDB and WM values in the skl_update_primary_plane() and similar functions and let the existing flushes that happen take care of flushing them out at the appropriate time. Of course I've kicked that idea around in my head for a while, but haven't had time to actually write any code for it, so I may be completely overlooking some stumbling block that makes it much more complicated than I'm envisioning.

Matt

Fixes: 0e8fb7ba7ca5 ("drm/i915/skl: Flush the WM configuration") 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 cpaul@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 788db86..2e31df4 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3859,8 +3859,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)

@@ -3876,6 +3879,16 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv, continue;

skl_wm_flush_pipe(dev_priv, pipe, 3);

• /*
  

•  * The only time we can get away with not waiting for an update
  

•  * is when we just enabled the pipe, e.g. when it doesn't have
  

•  * vblanks enabled anyway.
  

•  */
  

• if (drm_crtc_vblank_get(&crtc->base) == 0) {
  

• 	intel_wait_for_vblank(dev, pipe);
  

• 	drm_crtc_vblank_put(&crtc->base);
  

• }
  

  }

}

-- 2.7.4