[Intel-gfx] [PATCH] drm/i915: Don't stall too much for cursor vs. pageflip

[Chris Wilson]

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 at linux.intel.com>
> Cc: rafael.ristovski at gmail.com
> Signed-off-by: Daniel Vetter <daniel.vetter at 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

-- 
Chris Wilson, Intel Open Source Technology Centre