On Mon, Feb 20, 2017 at 03:30:58PM +0100, Maarten Lankhorst wrote: > Op 20-02-17 om 14:38 schreef Ville Syrjälä: > > On Mon, Feb 20, 2017 at 10:04:06AM +0100, Maarten Lankhorst wrote: > >> Op 17-02-17 om 16:01 schreef [email protected]: > >>> From: Ville Syrjälä <[email protected]> > >>> > >>> In order to make cursor updates actually safe wrt. watermark programming > >>> we have to clear the legacy_cursor_update flag in the atomic state. That > >>> will cause the regular atomic update path to do the necessary vblank > >>> wait after the plane update if needed, otherwise the vblank wait would > >>> be skipped and we'd feed the optimal watermarks to the hardware before > >>> the plane update has actually happened. > >>> > >>> To make the slow vs. fast path determination in > >>> intel_legacy_cursor_update() a little simpler we can ignore the actual > >>> visibility of the plane (which can only get computed once we've already > >>> chosen out path) and instead we simply check whether the fb is being > >>> set or cleared by the user. This means a fully clipped but logically > >>> visible cursor will be considered visible as far as watermark > >>> programming is concerned. We can do that for the cursor since it's a > >>> fixed size plane and the clipped size doesn't play a role in the > >>> watermark computation. > >>> > >>> This should fix underruns that can occur when the cursor gets > >>> enable/disabled or the size gets changed. Hopefully it's good enough > >>> that only pure cursor movement and flips go through unthrottled. > >>> > >>> Cc: Maarten Lankhorst <[email protected]> > >>> Cc: Daniel Vetter <[email protected]> > >>> Cc: Uwe Kleine-König <[email protected]> > >>> Reported-by: Uwe Kleine-König <[email protected]> > >>> Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow converting > >>> legacy page flip to atomic, v3.") > >>> Signed-off-by: Ville Syrjälä <[email protected]> > >>> --- > >>> drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++----------- > >>> drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++-------- > >>> 2 files changed, 30 insertions(+), 19 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/intel_display.c > >>> b/drivers/gpu/drm/i915/intel_display.c > >>> index b05d9c85384b..356ac04093e8 100644 > >>> --- a/drivers/gpu/drm/i915/intel_display.c > >>> +++ b/drivers/gpu/drm/i915/intel_display.c > >>> @@ -13031,6 +13031,17 @@ static int intel_atomic_commit(struct drm_device > >>> *dev, > >>> struct drm_i915_private *dev_priv = to_i915(dev); > >>> int ret = 0; > >>> > >>> + /* > >>> + * The intel_legacy_cursor_update() fast path takes care > >>> + * of avoiding the vblank waits for simple cursor > >>> + * movement and flips. For cursor on/off and size changes, > >>> + * we want to perform the vblank waits so that watermark > >>> + * updates happen during the correct frames. Gen9+ have > >>> + * double buffered watermarks and so shouldn't need this. > >>> + */ > >>> + if (INTEL_GEN(dev_priv) < 9) > >>> + state->legacy_cursor_update = false; > >> Could we perhaps add a check in ilk_compute_pipe_wm which unsets the > >> legacy_cursor_update flag so we keep things unsynced as much as possible? > > I'd have to sprinkle that stuff everywhere but the SKL code > > eventually. Seems a little pointless when I can just plop it > > there. > Ah indeed. Lets hope it doesn't slow things down too much. > >>> ret = drm_atomic_helper_setup_commit(state, nonblock); > >>> if (ret) > >>> return ret; > >>> @@ -13455,8 +13466,7 @@ intel_legacy_cursor_update(struct drm_plane > >>> *plane, > >>> old_plane_state->src_h != src_h || > >>> old_plane_state->crtc_w != crtc_w || > >>> old_plane_state->crtc_h != crtc_h || > >>> - !old_plane_state->visible || > >>> - old_plane_state->fb->modifier != fb->modifier) > >>> + !old_plane_state->fb != !fb) > >>> goto slow; > >>> > >>> new_plane_state = intel_plane_duplicate_state(plane); > >>> @@ -13479,10 +13489,6 @@ intel_legacy_cursor_update(struct drm_plane > >>> *plane, > >>> if (ret) > >>> goto out_free; > >>> > >>> - /* Visibility changed, must take slowpath. */ > >>> - if (!new_plane_state->visible) > >>> - goto slow_free; > >>> - > >>> ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex); > >>> if (ret) > >>> goto out_free; > >> Those 2 hunks are needed. If you move the cursor out of the visible area > >> or back you need to update. > > No. I changed the wm code to consider a non-visible but logicall active > > cursor as needing proper watermarks. That avoids needing this fallback > > path here. > Ah indeed. But one thing you dropped is the fb modifier check. > I suppose there's no harm with no support for using prite planes as cursor > plane yet, but might be nice to keep it in.
We'd have bigger problems than the modifier if we want to use a sprite plane as the cursor because for sprite planes the watermarks are computed based on the clipped size. So the wm code would need some surgery as well. > > Cc'ing Ristovski for testing the patch. :) 17:57 < Ristovski> mlankhorst: So I tested the patch and it passed all my manual tests, including vigorous_pointer_movement and spastic_window_repositioning, good enough for me! -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
