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

[Paulo Zanoni]

Em Qua, 2018-04-04 às 18:21 -0700, José Roberto de Souza escreveu:
> A simple page flip can cause the CFB required sized to increase and
> if it is bigger than the currently allocated CFB it needs to be
> resize 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.

For CI specifically, have they tried to increase the stolen memory size
in the BIOS?


> 
> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_fbc.c | 49 ++++++++++++++++++++++++++--
> ------------
>  2 files changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 5373b171bb96..5405b0b0f1c0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -566,6 +566,7 @@ struct intel_fbc {
>  	} work;
>  
>  	const char *no_fbc_reason;
> +	bool try_resize_cfb;
>  };
>  
>  /*
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> b/drivers/gpu/drm/i915/intel_fbc.c
> index 707d49c12638..ced11dd5747a 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -41,6 +41,11 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> +#define NO_FBC_REASON_NOT_ENOUGH_STOLEN_MEM "not enough stolen
> memory"

[przanoni at przanoni-mobl ~]$ echo "NO_FBC_REASON_NOT_ENOUGH_STOLEN_MEM"
| wc -c
36
[przanoni at przanoni-mobl ~]$ echo "not enough stolen memory" | wc -c
25


> +
> +static void __intel_fbc_cleanup_cfb(struct drm_i915_private
> *dev_priv);
> +static int intel_fbc_alloc_cfb(struct intel_crtc *crtc);

Please just move the functions up/down so we can avoid these.


> +
>  static inline bool fbc_supported(struct drm_i915_private *dev_priv)
>  {
>  	return HAS_FBC(dev_priv);
> @@ -446,6 +451,14 @@ static void intel_fbc_work_fn(struct work_struct
> *__work)
>  		goto retry;
>  	}
>  
> +	if (fbc->try_resize_cfb) {
> +		__intel_fbc_cleanup_cfb(dev_priv);
> +		if (intel_fbc_alloc_cfb(crtc)) {
> +			fbc->no_fbc_reason =
> NO_FBC_REASON_NOT_ENOUGH_STOLEN_MEM;
> +			goto out;
> +		}
> +	}
> +
>  	intel_fbc_hw_activate(dev_priv);
>  
>  	work->scheduled = false;
> @@ -843,22 +856,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
> @@ -870,6 +867,24 @@ 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)) {
> +		fbc->try_resize_cfb = true;
> +		DRM_DEBUG_KMS("Previous try to resize failed, trying
> again in the next schedule activation");
> +	} else if (intel_fbc_calculate_cfb_size(dev_priv, &fbc-
> >state_cache) >
> +		   fbc->compressed_fb.size * fbc->threshold) {
> +		fbc->try_resize_cfb = true;
> +		DRM_DEBUG_KMS("CFB requirements changed, next
> schedule activation will try to resize it");
> +	} else {
> +		fbc->try_resize_cfb = false;
> +	}

It's been a while since I worked with FBC so please help me with this
one: what is preventing our code from forever trying to enable FBC in
every single frame in case there's just not enough stolen memory?


> +
>  	return true;
>  }
>  
> @@ -1192,7 +1207,7 @@ void intel_fbc_enable(struct intel_crtc *crtc,
>  
>  	intel_fbc_update_state_cache(crtc, crtc_state, plane_state);
>  	if (intel_fbc_alloc_cfb(crtc)) {
> -		fbc->no_fbc_reason = "not enough stolen memory";
> +		fbc->no_fbc_reason =
> NO_FBC_REASON_NOT_ENOUGH_STOLEN_MEM;
>  		goto out;
>  	}
>