Re: [Intel-gfx] [PATCH 7/7] drm/i915: Consolidate all pre plane update vblank waits

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 09 Feb 2022, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> There are several reasons why we might have to do a vblank wait
> between some of the pre_plane_update() steps and the actual
> plane update. Currently we do a vblank wait for each of those
> individually. Let's consolidate things so that we just do a
> single vblank wait at the end of the pre_plane_update() step.
>
> Note that I don't think we should be hitting multiple vblank
> waits here currently, at least in most cases. But no real
> reason that couldn't happen in the future when some new
> features/workarounds are introduced.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 35 ++++++++++++--------
>  1 file changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 401a339973bf..7c37c4355606 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -742,6 +742,7 @@ void intel_plane_disable_noatomic(struct intel_crtc *crtc,
>  		to_intel_crtc_state(crtc->base.state);
>  	struct intel_plane_state *plane_state =
>  		to_intel_plane_state(plane->base.state);
> +	bool need_vblank_wait = false;
>  
>  	drm_dbg_kms(&dev_priv->drm,
>  		    "Disabling [PLANE:%d:%s] on [CRTC:%d:%s]\n",
> @@ -756,7 +757,7 @@ void intel_plane_disable_noatomic(struct intel_crtc *crtc,
>  	if ((crtc_state->active_planes & ~BIT(PLANE_CURSOR)) == 0 &&
>  	    hsw_ips_disable(crtc_state)) {
>  		crtc_state->ips_enabled = false;
> -		intel_crtc_wait_for_next_vblank(crtc);
> +		need_vblank_wait = true;
>  	}
>  
>  	/*
> @@ -770,7 +771,7 @@ void intel_plane_disable_noatomic(struct intel_crtc *crtc,
>  	 */
>  	if (HAS_GMCH(dev_priv) &&
>  	    intel_set_memory_cxsr(dev_priv, false))
> -		intel_crtc_wait_for_next_vblank(crtc);
> +		need_vblank_wait = true;
>  
>  	/*
>  	 * Gen2 reports pipe underruns whenever all planes are disabled.
> @@ -779,6 +780,9 @@ void intel_plane_disable_noatomic(struct intel_crtc *crtc,
>  	if (DISPLAY_VER(dev_priv) == 2 && !crtc_state->active_planes)
>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, crtc->pipe, false);
>  
> +	if (need_vblank_wait)
> +		intel_crtc_wait_for_next_vblank(crtc);
> +
>  	intel_plane_disable_arm(plane, crtc_state);
>  	intel_crtc_wait_for_next_vblank(crtc);
>  }
> @@ -1258,7 +1262,7 @@ static void intel_crtc_disable_flip_done(struct intel_atomic_state *state,
>  	}
>  }
>  
> -static void intel_crtc_async_flip_disable_wa(struct intel_atomic_state *state,
> +static bool intel_crtc_async_flip_disable_wa(struct intel_atomic_state *state,
>  					     struct intel_crtc *crtc)
>  {
>  	const struct intel_crtc_state *old_crtc_state =
> @@ -1268,7 +1272,7 @@ static void intel_crtc_async_flip_disable_wa(struct intel_atomic_state *state,
>  	u8 update_planes = new_crtc_state->update_planes;
>  	const struct intel_plane_state *old_plane_state;
>  	struct intel_plane *plane;
> -	bool need_vbl_wait = false;
> +	bool need_vblank_wait = false;
>  	int i;
>  
>  	for_each_old_intel_plane_in_state(state, plane, old_plane_state, i) {
> @@ -1281,12 +1285,11 @@ static void intel_crtc_async_flip_disable_wa(struct intel_atomic_state *state,
>  			 */
>  			plane->async_flip(plane, old_crtc_state,
>  					  old_plane_state, false);
> -			need_vbl_wait = true;
> +			need_vblank_wait = true;
>  		}
>  	}
>  
> -	if (need_vbl_wait)
> -		intel_crtc_wait_for_next_vblank(crtc);
> +	return need_vblank_wait;
>  }
>  
>  static void intel_pre_plane_update(struct intel_atomic_state *state,
> @@ -1298,14 +1301,15 @@ static void intel_pre_plane_update(struct intel_atomic_state *state,
>  	const struct intel_crtc_state *new_crtc_state =
>  		intel_atomic_get_new_crtc_state(state, crtc);
>  	enum pipe pipe = crtc->pipe;
> +	bool need_vblank_wait = false;
>  
>  	intel_psr_pre_plane_update(state, crtc);
>  
>  	if (hsw_ips_pre_update(state, crtc))
> -		intel_crtc_wait_for_next_vblank(crtc);
> +		need_vblank_wait = true;
>  
>  	if (intel_fbc_pre_update(state, crtc))
> -		intel_crtc_wait_for_next_vblank(crtc);
> +		need_vblank_wait = true;
>  
>  	if (!needs_async_flip_vtd_wa(old_crtc_state) &&
>  	    needs_async_flip_vtd_wa(new_crtc_state))
> @@ -1337,7 +1341,7 @@ static void intel_pre_plane_update(struct intel_atomic_state *state,
>  	 */
>  	if (HAS_GMCH(dev_priv) && old_crtc_state->hw.active &&
>  	    new_crtc_state->disable_cxsr && intel_set_memory_cxsr(dev_priv, false))
> -		intel_crtc_wait_for_next_vblank(crtc);
> +		need_vblank_wait = true;
>  
>  	/*
>  	 * IVB workaround: must disable low power watermarks for at least
> @@ -1348,7 +1352,7 @@ static void intel_pre_plane_update(struct intel_atomic_state *state,
>  	 */
>  	if (old_crtc_state->hw.active &&
>  	    new_crtc_state->disable_lp_wm && ilk_disable_lp_wm(dev_priv))
> -		intel_crtc_wait_for_next_vblank(crtc);
> +		need_vblank_wait = true;
>  
>  	/*
>  	 * If we're doing a modeset we don't need to do any
> @@ -1389,8 +1393,13 @@ static void intel_pre_plane_update(struct intel_atomic_state *state,
>  	 * WA for platforms where async address update enable bit
>  	 * is double buffered and only latched at start of vblank.
>  	 */
> -	if (old_crtc_state->uapi.async_flip && !new_crtc_state->uapi.async_flip)
> -		intel_crtc_async_flip_disable_wa(state, crtc);
> +	if (old_crtc_state->uapi.async_flip &&
> +	    !new_crtc_state->uapi.async_flip &&
> +	    intel_crtc_async_flip_disable_wa(state, crtc))
> +		need_vblank_wait = true;
> +
> +	if (need_vblank_wait)
> +		intel_crtc_wait_for_next_vblank(crtc);
>  }
>  
>  static void intel_crtc_disable_planes(struct intel_atomic_state *state,

-- 
Jani Nikula, Intel Open Source Graphics Center