Re: [PATCH 2/2] drm/i915/fbc: Resize CFB in non-full modeset paths

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

On Fri, 06 Apr 2018, José Roberto de Souza <jose.souza@intel.com> wrote:
> A simple page flip can cause the CFB required size to increase and
> if it is bigger than the currently allocated CFB it needs to be
> resized to activate FBC again.
>
> Until now this case was not being handled but CI is starting to
> get some of this errors.
>
> So here it will free the old CFB and a try to allocate the required
> CFB in the schedule activation work, it will happen after one vblank
> so is guarantee that FBC was completed disabled and is not using CFB.
>
> Also in case that there is no enough stolen memory to allocate the
> new CFB it will try 3 times per full modeset as the CFB requirement
> could be reduced in the next non-full modeset.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105683
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  3 +++
>  drivers/gpu/drm/i915/intel_fbc.c | 46 +++++++++++++++++++++-----------
>  2 files changed, 33 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5373b171bb96..4ce19b45f67d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -566,6 +566,9 @@ struct intel_fbc {
>  	} work;
>  
>  	const char *no_fbc_reason;
> +
> +	bool cfb_try_resize;
> +	u8 cfb_resize_tries_left;
>  };
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 573b034a02fd..7d77936db3ec 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -41,6 +41,9 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> +static void __intel_fbc_cleanup_cfb(struct drm_i915_private *dev_priv);
> +static int intel_fbc_alloc_cfb(struct intel_crtc *crtc);
> +
>  static inline bool fbc_supported(struct drm_i915_private *dev_priv)
>  {
>  	return HAS_FBC(dev_priv);
> @@ -446,6 +449,15 @@ static void intel_fbc_work_fn(struct work_struct *__work)
>  		goto retry;
>  	}
>  
> +	if (fbc->cfb_try_resize && fbc->cfb_resize_tries_left) {
> +		__intel_fbc_cleanup_cfb(dev_priv);
> +		if (intel_fbc_alloc_cfb(crtc)) {
> +			fbc->no_fbc_reason = "not enough stolen memory";
> +			fbc->cfb_resize_tries_left--;
> +			goto out;
> +		}
> +	}
> +
>  	intel_fbc_hw_activate(dev_priv);
>  
>  	work->scheduled = false;
> @@ -850,22 +862,6 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  		return false;
>  	}
>  
> -	/* It is possible for the required CFB size change without a
> -	 * crtc->disable + crtc->enable since it is possible to change the
> -	 * stride without triggering a full modeset. Since we try to
> -	 * over-allocate the CFB, there's a chance we may keep FBC enabled even
> -	 * if this happens, but if we exceed the current CFB size we'll have to
> -	 * disable FBC. Notice that it would be possible to disable FBC, wait
> -	 * for a frame, free the stolen node, then try to reenable FBC in case
> -	 * we didn't get any invalidate/deactivate calls, but this would require
> -	 * a lot of tracking just for a specific case. If we conclude it's an
> -	 * important case, we can implement it later. */
> -	if (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) >
> -	    fbc->compressed_fb.size * fbc->threshold) {
> -		fbc->no_fbc_reason = "CFB requirements changed";
> -		return false;
> -	}
> -
>  	/*
>  	 * Work around a problem on GEN9+ HW, where enabling FBC on a plane
>  	 * having a Y offset that isn't divisible by 4 causes FIFO underrun
> @@ -877,6 +873,23 @@ static bool intel_fbc_can_activate(struct intel_crtc *crtc)
>  		return false;
>  	}
>  
> +	/* It is possible for the required CFB size change without a
> +	 * crtc->disable + crtc->enable since it is possible to change the
> +	 * stride without triggering a full modeset. Since we try to
> +	 * over-allocate the CFB, there's a chance we may keep FBC enabled even
> +	 * if this happens, but if we exceed the current CFB size we'll have to
> +	 * resize CFB.
> +	 */
> +	if (!drm_mm_node_allocated(&fbc->compressed_fb) ||
> +	    (intel_fbc_calculate_cfb_size(dev_priv, &fbc->state_cache) >
> +	     fbc->compressed_fb.size * fbc->threshold)) {
> +		fbc->cfb_try_resize = true;
> +		DRM_DEBUG_KMS("CFB requirements have changed, activation  \
> +			      work will try to resize it");

Either the string entirely on one line, or the string split to two, but
please never line continuation within the string.

BR,
Jani.


> +	} else {
> +		fbc->cfb_try_resize = false;
> +	}
> +
>  	return true;
>  }
>  
> @@ -1208,6 +1221,7 @@ void intel_fbc_enable(struct intel_crtc *crtc,
>  
>  	fbc->enabled = true;
>  	fbc->crtc = crtc;
> +	fbc->cfb_resize_tries_left = 3;
>  out:
>  	mutex_unlock(&fbc->lock);
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx