Re: [PATCH] drm/i915: Make legacy cursor updates more unsynced

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: ville.syrjala@linux.intel.com, intel-gfx@lists.freedesktop.org
Cc: "Daniel Vetter" <daniel.vetter@ffwll.ch>,
	"Rafael Ristovski" <rafael.ristovski@gmail.com>,
	"Uwe Kleine-König" <uwe@kleine-koenig.org>
Subject: Re: [PATCH] drm/i915: Make legacy cursor updates more unsynced
Date: Wed, 29 Mar 2017 15:56:49 +0200	[thread overview]
Message-ID: <af49c8bc-ce69-ca31-d1b9-ae661940ce75@linux.intel.com> (raw)
In-Reply-To: <20170328193538.8687-1-ville.syrjala@linux.intel.com>

Op 28-03-17 om 21:35 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We're clearing the legacy_cursor_update flag before calling
> drm_atomic_helper_setup_commit() which means the helper will
> wait for the flip to complete before cleaning up the framebuffers.
> That's not what we want for the legacy cursor, so let's clear
> the flag after setting up the commit.
>
> Also toss in a FIXME about solving these problems in a nicer
> way using the fabled vblank workers.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Uwe Kleine-König <uwe@kleine-koenig.org>
> Cc: Rafael Ristovski <rafael.ristovski@gmail.com>
> Fixes: a5509abda48e ("drm/i915: Fix legacy cursor vs. watermarks for ILK-BDW")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 654b8a0c28ee..05ff193f2dd8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13024,6 +13024,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;
> +
>  	/*
>  	 * The intel_legacy_cursor_update() fast path takes care
>  	 * of avoiding the vblank waits for simple cursor
> @@ -13031,14 +13035,18 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	 * 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.
> +	 *
> +	 * Do this after drm_atomic_helper_setup_commit() because
> +	 * we still want to skip the fb cleanup waits from the
> +	 * atomic helper. Although that does risk yanking the
> +	 * mapping from under the display engine.
> +	 *
> +	 * 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;
>  
> -	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);

From irc:
<vsyrjala> hmm. actually should probably move that thing to happen even later. we should skip the flip waits from intel_atomic_prepare_commit() as well i recon

Makes sense. Not sure how much we care about legacy page flips, since they're on their way out.

If this is fixed and trybot is happy with v2 then I will be too. :)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx