On Mon, Sep 18, 2017 at 12:12:50PM +0200, Maarten Lankhorst wrote:
> Commit b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.") removed
> the call to wait_for_vblanks and replaced it with flip_done.
>
> Unfortunately legacy_cursor_update was unset too late, and the
> replacement call drm_atomic_helper_wait_for_flip_done() was
> a noop. Make sure that its unset before setup_commit() is
> called to fix this issue.
>
> Changes since v1:
> - Force vblank wait for watermarks not yet converted to atomic too. (Ville)
> - Use for_each_new_intel_crtc_in_state. (Ville)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Fixes: b44d5c0c105a ("drm/i915: Always wait for flip_done, v2.")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102675
> Testcase: kms_cursor_crc
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Reported-by: Marta Löfstedt <marta.lofstedt@xxxxxxxxx>
> Cc: Marta Löfstedt <marta.lofstedt@xxxxxxxxx>
> Tested-by: Marta Löfstedt <marta.lofstedt@xxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
I think the proper fix is switching over to the async plane helpers and
unconditionally always clearing the legacy cursor hack. Maybe even
outright removing it, since it doesn't really work all that well with most
drivers.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++---------------
> 1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8599e425abb1..8d051256da1e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12517,21 +12517,10 @@ static int intel_atomic_commit(struct drm_device *dev,
> struct drm_i915_private *dev_priv = to_i915(dev);
> int ret = 0;
>
> - ret = drm_atomic_helper_setup_commit(state, nonblock);
> - if (ret)
> - return ret;
> -
> drm_atomic_state_get(state);
> i915_sw_fence_init(&intel_state->commit_ready,
> intel_atomic_commit_ready);
>
> - ret = intel_atomic_prepare_commit(dev, state);
> - if (ret) {
> - DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
> - i915_sw_fence_commit(&intel_state->commit_ready);
> - return ret;
> - }
> -
> /*
> * The intel_legacy_cursor_update() fast path takes care
> * of avoiding the vblank waits for simple cursor
> @@ -12540,19 +12529,37 @@ static int intel_atomic_commit(struct drm_device *dev,
> * updates happen during the correct frames. Gen9+ have
> * double buffered watermarks and so shouldn't need this.
> *
> - * Do this after drm_atomic_helper_setup_commit() and
> - * intel_atomic_prepare_commit() because we still want
> - * to skip the flip and fb cleanup waits. Although that
> - * does risk yanking the mapping from under the display
> - * engine.
> + * Unset state->legacy_cursor_update before the call to
> + * drm_atomic_helper_setup_commit() because otherwise
> + * drm_atomic_helper_wait_for_flip_done() is a noop and
> + * we get FIFO underruns because we didn't wait
> + * for vblank.
> *
> * FIXME doing watermarks and fb cleanup from a vblank worker
> * (assuming we had any) would solve these problems.
> */
> - if (INTEL_GEN(dev_priv) < 9)
> - state->legacy_cursor_update = false;
> + if (INTEL_GEN(dev_priv) < 9 && state->legacy_cursor_update) {
> + struct intel_crtc_state *new_crtc_state;
> + struct intel_crtc *crtc;
> + int i;
> +
> + for_each_new_intel_crtc_in_state(intel_state, crtc, new_crtc_state, i)
> + if (new_crtc_state->wm.need_postvbl_update ||
> + new_crtc_state->update_wm_post)
> + state->legacy_cursor_update = false;
> + }
> +
> + ret = intel_atomic_prepare_commit(dev, state);
> + if (ret) {
> + DRM_DEBUG_ATOMIC("Preparing state failed with %i\n", ret);
> + i915_sw_fence_commit(&intel_state->commit_ready);
> + return ret;
> + }
> +
> + ret = drm_atomic_helper_setup_commit(state, nonblock);
> + if (!ret)
> + ret = drm_atomic_helper_swap_state(state, true);
>
> - ret = drm_atomic_helper_swap_state(state, true);
> if (ret) {
> i915_sw_fence_commit(&intel_state->commit_ready);
>
> --
> 2.14.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
