| Message ID | 1469048403-32016-2-git-send-email-cpaul@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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)) + 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);
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 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(-)