Re: [PATCH] drm/i915: Block enabling FBC until flips have been completed

["Souza, Jose" <jose.souza@intel.com>]

On Thu, 2018-04-12 at 18:07 +0200, Maarten Lankhorst wrote:
> There is a small race window in which FBC can be enabled after
> pre_plane_update is called, but before the page flip has been
> queued or completed.

I don't think there is such window, intel_fbc_deactivate() that is
called from intel_fbc_pre_update() will set fbc->work.scheduled =
false; so the FBC will not be enabled in intel_fbc_work_fn()

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_fbc.c | 35 +++++++++++-------------------
> -----
>  2 files changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index a0b8db3db141..2e2f24c2db9e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -492,6 +492,7 @@ struct intel_fbc {
>  
>  	bool enabled;
>  	bool active;
> +	bool flip_pending;
>  
>  	bool underrun_detected;
>  	struct work_struct underrun_work;
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> b/drivers/gpu/drm/i915/intel_fbc.c
> index b431b6733cc1..4770dd7dad5c 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -924,13 +924,6 @@ static void intel_fbc_get_reg_params(struct
> intel_crtc *crtc,
>  						32 * fbc->threshold) 
> * 8;
>  }
>  
> -static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params
> *params1,
> -				       struct intel_fbc_reg_params
> *params2)
> -{
> -	/* We can use this since intel_fbc_get_reg_params() does a
> memset. */
> -	return memcmp(params1, params2, sizeof(*params1)) == 0;
> -}
> -
>  void intel_fbc_pre_update(struct intel_crtc *crtc,
>  			  struct intel_crtc_state *crtc_state,
>  			  struct intel_plane_state *plane_state)
> @@ -952,6 +945,7 @@ void intel_fbc_pre_update(struct intel_crtc
> *crtc,
>  	if (!fbc->enabled || fbc->crtc != crtc)
>  		goto unlock;
>  
> +	fbc->flip_pending = true;

Also this is not a good name, other actions can cause this function to
be executed other than a flip.

>  	intel_fbc_update_state_cache(crtc, crtc_state, plane_state);
>  
>  deactivate:
> @@ -988,13 +982,15 @@ static void __intel_fbc_post_update(struct
> intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	struct intel_fbc *fbc = &dev_priv->fbc;
> -	struct intel_fbc_reg_params old_params;
>  
>  	WARN_ON(!mutex_is_locked(&fbc->lock));
>  
>  	if (!fbc->enabled || fbc->crtc != crtc)
>  		return;
>  
> +	fbc->flip_pending = false;
> +	WARN_ON(fbc->active);
> +
>  	if (!i915_modparams.enable_fbc) {
>  		intel_fbc_deactivate(dev_priv, "disabled at runtime
> per module param");
>  		__intel_fbc_disable(dev_priv);
> @@ -1002,25 +998,16 @@ static void __intel_fbc_post_update(struct
> intel_crtc *crtc)
>  		return;
>  	}
>  
> -	if (!intel_fbc_can_activate(crtc)) {
> -		WARN_ON(fbc->active);
> +	if (!intel_fbc_can_activate(crtc))
>  		return;
> -	}
>  
> -	old_params = fbc->params;
>  	intel_fbc_get_reg_params(crtc, &fbc->params);
>  
> -	/* If the scanout has not changed, don't modify the FBC
> settings.
> -	 * Note that we make the fundamental assumption that the fb-
> >obj
> -	 * cannot be unpinned (and have its GTT offset and fence
> revoked)
> -	 * without first being decoupled from the scanout and FBC
> disabled.
> -	 */
> -	if (fbc->active &&
> -	    intel_fbc_reg_params_equal(&old_params, &fbc->params))
> -		return;
> -
> -	intel_fbc_deactivate(dev_priv, "FBC enabled (active or
> scheduled)");
> -	intel_fbc_schedule_activation(crtc);
> +	if (!fbc->busy_bits) {

I guess this 'if' the line that is fixing the issue.

> +		intel_fbc_deactivate(dev_priv, "FBC enabled (active
> or scheduled)");
> +		intel_fbc_schedule_activation(crtc);
> +	} else
> +		intel_fbc_deactivate(dev_priv, "frontbuffer write");
>  }
>  
>  void intel_fbc_post_update(struct intel_crtc *crtc)
> @@ -1085,7 +1072,7 @@ void intel_fbc_flush(struct drm_i915_private
> *dev_priv,
>  	    (frontbuffer_bits & intel_fbc_get_frontbuffer_bit(fbc)))
> {
>  		if (fbc->active)
>  			intel_fbc_recompress(dev_priv);
> -		else
> +		else if (!fbc->flip_pending)
>  			__intel_fbc_post_update(fbc->crtc);
>  	}
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx