Re: [Intel-gfx] [PATCH v2 4/4] drm/i915/skl: Update plane watermarks atomically during plane updates
On Thu, Jul 21, 2016 at 03:23:40PM -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
>
> Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
> Signed-off-by: Lyude
> Cc: sta...@vger.kernel.org
> Cc: Ville Syrjälä
> Cc: Daniel Vetter
> Cc: Radhakrishna Sripada
> Cc: Hans de Goede
> Cc: Matt Roper
> ---
> drivers/gpu/drm/i915/intel_display.c | 5
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_pm.c | 54
> ++--
> drivers/gpu/drm/i915/intel_sprite.c | 2 ++
> 4 files changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 78beb7e..d93d014 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_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 e74d851..f1f54d9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1709,6 +1709,8 @@
[Intel-gfx] [PATCH v2 4/4] drm/i915/skl: Update plane watermarks atomically during plane updates
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
Fixes: 2d41c0b59afc ("drm/i915/skl: SKL Watermark Computation")
Signed-off-by: Lyude
Cc: sta...@vger.kernel.org
Cc: Ville Syrjälä
Cc: Daniel Vetter
Cc: Radhakrishna Sripada
Cc: Hans de Goede
Cc: Matt Roper
---
drivers/gpu/drm/i915/intel_display.c | 5
drivers/gpu/drm/i915/intel_drv.h | 2 ++
drivers/gpu/drm/i915/intel_pm.c | 54 ++--
drivers/gpu/drm/i915/intel_sprite.c | 2 ++
4 files changed, 49 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 78beb7e..d93d014 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_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 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 sk
