Re: [PATCH] drm/i915: Unset legacy_cursor_update early in intel_atomic_commit, v2.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com>
> 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@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Reported-by: Marta Löfstedt <marta.lofstedt@intel.com>
> Cc: Marta Löfstedt <marta.lofstedt@intel.com>
> Tested-by: Marta Löfstedt <marta.lofstedt@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  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;

Hmm. I guess that's better. But I still don't see why you want to change
this bit of code in this patch. AFAICS it's got nothing to do with the fix
itself, and instead it's just trying to optimize some cursor updates
that were kicked over to the slow path. Or am I missing something?

> +	}
> +
> +	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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx