Re: [PATCH] drm/i915: Handle changing enable_fbc parameter at runtime better.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Mar 05, 2018 at 01:36:08PM +0100, Maarten Lankhorst wrote:
> If i915.enable_fbc is cleared at runtime, but FBC was previously enabled
> then we don't disable FBC until the next time the crtc is disabled.
> 
> Make sure that if the module param is changed, we disable FBC in
> intel_fbc_post_update so we never have to worry about disabling.

What about switching this from a parameter to debugfs toggle like drrs?

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 62 +++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index eaaa59b45707..5325b59c2f9c 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -949,6 +949,30 @@ void intel_fbc_pre_update(struct intel_crtc *crtc,
>  	mutex_unlock(&fbc->lock);
>  }
>  
> +/**
> + * __intel_fbc_disable - disable FBC
> + * @dev_priv: i915 device instance
> + *
> + * This is the low level function that actually disables FBC. Callers should
> + * grab the FBC lock.
> + */
> +static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_fbc *fbc = &dev_priv->fbc;
> +	struct intel_crtc *crtc = fbc->crtc;
> +
> +	WARN_ON(!mutex_is_locked(&fbc->lock));
> +	WARN_ON(!fbc->enabled);
> +	WARN_ON(fbc->active);
> +
> +	DRM_DEBUG_KMS("Disabling FBC on pipe %c\n", pipe_name(crtc->pipe));
> +
> +	__intel_fbc_cleanup_cfb(dev_priv);
> +
> +	fbc->enabled = false;
> +	fbc->crtc = NULL;
> +}
> +
>  static void __intel_fbc_post_update(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -960,6 +984,13 @@ static void __intel_fbc_post_update(struct intel_crtc *crtc)
>  	if (!fbc->enabled || fbc->crtc != crtc)
>  		return;
>  
> +	if (!i915_modparams.enable_fbc) {
> +		intel_fbc_deactivate(dev_priv, "disabled at runtime per module param");
> +		__intel_fbc_disable(dev_priv);
> +
> +		return;
> +	}
> +
>  	if (!intel_fbc_can_activate(crtc)) {
>  		WARN_ON(fbc->active);
>  		return;
> @@ -1163,31 +1194,6 @@ void intel_fbc_enable(struct intel_crtc *crtc,
>  	mutex_unlock(&fbc->lock);
>  }
>  
> -/**
> - * __intel_fbc_disable - disable FBC
> - * @dev_priv: i915 device instance
> - *
> - * This is the low level function that actually disables FBC. Callers should
> - * grab the FBC lock.
> - */
> -static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
> -{
> -	struct intel_fbc *fbc = &dev_priv->fbc;
> -	struct intel_crtc *crtc = fbc->crtc;
> -
> -	WARN_ON(!mutex_is_locked(&fbc->lock));
> -	WARN_ON(!fbc->enabled);
> -	WARN_ON(fbc->active);
> -	WARN_ON(crtc->active);
> -
> -	DRM_DEBUG_KMS("Disabling FBC on pipe %c\n", pipe_name(crtc->pipe));
> -
> -	__intel_fbc_cleanup_cfb(dev_priv);
> -
> -	fbc->enabled = false;
> -	fbc->crtc = NULL;
> -}
> -
>  /**
>   * intel_fbc_disable - disable FBC if it's associated with crtc
>   * @crtc: the CRTC
> @@ -1202,6 +1208,8 @@ void intel_fbc_disable(struct intel_crtc *crtc)
>  	if (!fbc_supported(dev_priv))
>  		return;
>  
> +	WARN_ON(crtc->active);
> +
>  	mutex_lock(&fbc->lock);
>  	if (fbc->crtc == crtc)
>  		__intel_fbc_disable(dev_priv);
> @@ -1224,8 +1232,10 @@ void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
>  		return;
>  
>  	mutex_lock(&fbc->lock);
> -	if (fbc->enabled)
> +	if (fbc->enabled) {
> +		WARN_ON(fbc->crtc->active);
>  		__intel_fbc_disable(dev_priv);
> +	}
>  	mutex_unlock(&fbc->lock);
>  
>  	cancel_work_sync(&fbc->work.work);
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx