| Message ID | 1466603235-18610-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 22, 2016 at 03:47:15PM +0200, Daniel Vetter wrote: > - We don't need to wait for the previous commit to have fully > completed, the waiting for hw_done in swap_state is sufficient. > > - We need to make sure that the pure page_flip signals hw_done early > enough. This is done through a gross hack by recomputing stuff. I > think it'd be better to fix this by moving things around a bit, > keeping separate state pointers where needed (e.g. hw verifier) and > for the wm optimization, by extracting that into a cancellable work > item. > > But legacy cursor vs. legacy page-flip is a very restricted use case, > and we need to make it work meanwhile. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96593 Testcase: igt/kms_cursor_legacy/basic-cursor-vs-flip > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: rafael.ristovski@gmail.com > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index dc33bf278cc2..ff66b73fa412 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13706,6 +13706,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) > struct drm_plane *plane; > struct drm_plane_state *plane_state; > bool hw_check = intel_state->modeset; > + bool need_post_state; > unsigned long put_domains[I915_MAX_PIPES] = {}; > unsigned crtc_vblank_mask = 0; > int i, ret; > @@ -13724,7 +13725,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) > WARN_ON(ret); > } > > - drm_atomic_helper_wait_for_dependencies(state); > + if (!state->legacy_cursor_update) > + drm_atomic_helper_wait_for_dependencies(state); This looks like a hack. Ideally the computed dependencies for the cursor update would be nil, right? > if (intel_state->modeset) { > memcpy(dev_priv->min_pixclk, intel_state->min_pixclk, > @@ -13821,6 +13823,17 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) > crtc_vblank_mask |= 1 << i; > } > > + need_post_state = false; > + for_each_crtc_in_state(state, crtc, old_crtc_state, i) { > + intel_cstate = to_intel_crtc_state(crtc->state); > + > + if (intel_cstate->wm.need_postvbl_update) > + need_post_state = true; > + > + if (needs_modeset(crtc->state) || intel_cstate->update_pipe) > + need_post_state = true; > + } This + the bifurcation of hw_done makes sense, but again shouldn't it fall out that !need_post_state is just a series of noops? I tried it on my workstation in the hope that the flicker was somehow a result of the cursor stalls. Sadly not. :( -Chris
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index dc33bf278cc2..ff66b73fa412 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13706,6 +13706,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) struct drm_plane *plane; struct drm_plane_state *plane_state; bool hw_check = intel_state->modeset; + bool need_post_state; unsigned long put_domains[I915_MAX_PIPES] = {}; unsigned crtc_vblank_mask = 0; int i, ret; @@ -13724,7 +13725,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) WARN_ON(ret); } - drm_atomic_helper_wait_for_dependencies(state); + if (!state->legacy_cursor_update) + drm_atomic_helper_wait_for_dependencies(state); if (intel_state->modeset) { memcpy(dev_priv->min_pixclk, intel_state->min_pixclk, @@ -13821,6 +13823,17 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) crtc_vblank_mask |= 1 << i; } + need_post_state = false; + for_each_crtc_in_state(state, crtc, old_crtc_state, i) { + intel_cstate = to_intel_crtc_state(crtc->state); + + if (intel_cstate->wm.need_postvbl_update) + need_post_state = true; + + if (needs_modeset(crtc->state) || intel_cstate->update_pipe) + need_post_state = true; + } + /* FIXME: We should call drm_atomic_helper_commit_hw_done() here * already, but still need the state for the delayed optimization. To * fix this: @@ -13830,6 +13843,9 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) * - switch over to the vblank wait helper in the core after that since * we don't need out special handling any more. */ + if (!need_post_state) + drm_atomic_helper_commit_hw_done(state); + if (!state->legacy_cursor_update) intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask); @@ -13856,7 +13872,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state); } - drm_atomic_helper_commit_hw_done(state); + if (need_post_state) + drm_atomic_helper_commit_hw_done(state); if (intel_state->modeset) intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
- We don't need to wait for the previous commit to have fully completed, the waiting for hw_done in swap_state is sufficient. - We need to make sure that the pure page_flip signals hw_done early enough. This is done through a gross hack by recomputing stuff. I think it'd be better to fix this by moving things around a bit, keeping separate state pointers where needed (e.g. hw verifier) and for the wm optimization, by extracting that into a cancellable work item. But legacy cursor vs. legacy page-flip is a very restricted use case, and we need to make it work meanwhile. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96593 Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: rafael.ristovski@gmail.com Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- Totally untested. I think we want at least an igt that runs legacy page flips against a storm of cursor movements and checks that the curosr movements never stalls. I think we can be slightly more lenient with cursor bo updates. Storms with those tend to happen when X starts up, or changes configurations. Not when moving it around. But I might be mistaken on that one, iirc at least some older versions of modesetting had a silly flag set which resulted in the cursor getting disabled, changed and then re-enabled on each update "to avoid tearing" or something like that. But I think that was fixed meanwhile. Need to hand this off to Maarten since I'm going on vacation this Friday for 2 weeks, and just a bit of time left to wind down various things. Thanks, Daniel --- drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)