On Tue, Jul 19, 2016 at 12:30:55PM -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:
>
> - Calculate new watermark values in skl_compute_wm()
>
> - Update non-watermark settings for each plane in
> skylake_update_primary_plane()/i9xx_update_cursor()
>
> - Arm plane registers to update at the start of the next vblank in
> skylake_update_primary_plane()/i9xx_update_cursor()
>
> - *Possibly underrun here, since the plane may now have updated it's
> settings using the old and insufficient watermark values*
>
> - Update watermark settings in skl_write_wm_values()
>
> - *Possibly underrun here as well if we've passed a vblank,
> causing the hardware to get stuck running on intermediate wm values*
>
> - Finally arm plane registers so they update to the correct values at
> the start of the next vblank in skl_wm_flush_pipe()
I think you're on the right track with this patch, but I don't think the
sequence of events is quite what you describe in the commit message
here. The calls to skylake_update_primary_plane()/i9xx_update_cursor()
are while we're under vblank evasion, which happens during
finish_atomic_commit() -> drm_atomic_helper_commit_planes_on_crtc()
Then gen9 watermarks get written during intel_pre_plane_update() for
non-modesets or during crtc_enable for modesets. So I believe the
actual sequence of events is:
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
}
So you're right that fundamentally the problem arises from the
watermarks not being updated under vblank evasion when the corresponding
plane updates happen (so we might wind up with new watermarks before
we've actually switched our plane registers if a vblank sneaks in in the
meantime). The sequence is just slightly different from the way you'd
listed it out.
>
> Now we update watermarks atomically like this:
>
> - Calculate new watermark values in skl_compute_wm()
>
> - Update watermark values for each plane in
> skylake_write_plane_wm()/skylake_write_cursor_wm()
>
> - Update all the other values for the plane (position, address, etc.)
> in skylake_update_primary_plane()/i9xx_update_cursor()
Do we also need it in skl_update_plane() (from intel_sprite.c)?
>
> - Arm plane registers (including the watermark settings) to update at
> the start of the next vblank in
> skylake_update_primary_plane()/i9xx_update_cursor()
>
> Which is more of a step in the right direction to fixing all of the
> underrun issues we're currently seeing with skl
>
> Signed-off-by: Lyude Paul <cpaul@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@xxxxxxxxx>
> Cc: Hans de Goede <hdegoede@xxxxxxxxxx> <cpaul@xxxxxxxxxx>
> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 47 ++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_pm.c | 15 +-----------
> 2 files changed, 48 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fb7d8fc5..3d2c125 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2971,6 +2971,27 @@ u32 skl_plane_ctl_rotation(unsigned int rotation)
> return 0;
> }
>
> +static void skylake_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;
> +
> + mutex_lock(&dev_priv->wm.wm_mutex);
I don't think this mutex is actually protecting anything important here
is it? We already have the crtc & plane mutexes and we're not really
using/touching any global state. Actually, we're calling this while
under vblank evasion, so we're not allowed to sleep here anyway.
(same comment for the cursor version)
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]);
> +
> + mutex_unlock(&dev_priv->wm.wm_mutex);
> +}
> +
> static void skylake_update_primary_plane(struct drm_plane *plane,
> const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> @@ -3031,6 +3052,8 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
> intel_crtc->adjusted_x = x_offset;
> intel_crtc->adjusted_y = y_offset;
>
> + skylake_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);
> @@ -10233,6 +10256,27 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base,
> }
> }
>
> +static 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;
> +
> + mutex_lock(&dev_priv->wm.wm_mutex);
> +
> + 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]);
> +
> + mutex_unlock(&dev_priv->wm.wm_mutex);
> +}
> +
> static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base,
> const struct intel_plane_state *plane_state)
> {
> @@ -10242,6 +10286,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_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 213ad35..3a7709c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3788,7 +3788,7 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
> skl_disable_sagv(dev_priv);
>
> 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,19 +3798,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),
> --
> 2.7.4
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html