On Tue, Mar 21, 2017 at 03:42:53PM +0200, Ander Conselvan De Oliveira wrote: > On Thu, 2017-03-02 at 16:58 +0200, Ville Syrjälä wrote: > > On Sat, Feb 25, 2017 at 04:31:04PM +0100, Maarten Lankhorst wrote: > > > Op 24-02-17 om 14:11 schreef Ville Syrjälä: > > > > 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 ville.syrj...@linux.intel.com: > > > > > > > > From: Ville Syrjälä <ville.syrj...@linux.intel.com> > > > > > > > > > > > > > > > > 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 <maarten.lankho...@linux.intel.com> > > > > > > > > Cc: Daniel Vetter <daniel.vet...@ffwll.ch> > > > > > > > > Cc: Uwe Kleine-König <u...@kleine-koenig.org> > > > > > > > > Reported-by: Uwe Kleine-König <u...@kleine-koenig.org> > > > > > > > > Fixes: f79f26921ee1 ("drm/i915: Add a cursor hack to allow > > > > > > > > converting legacy page flip to atomic, v3.") > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com> > > > > > > > > --- > > > > > > > > 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! > > > > > > > > > > Fair enough. > > > > > > Reviewed-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com> > > > > Pushed to dinq. Thanks for the review and testing. > > This patch causes FIFO underruns on gen9 devices with kms_cursor_crc *-random > tests. The problem seems to happen when the cursor plane gets enabled after > being disabled because the cursor was completely offscreen. With the patch > reverted the enable goes through the slow path, which doesn't cause an > underrun.
https://patchwork.freedesktop.org/series/21226/#rev1 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx