Re: [PATCH] drm/i915: Minimize the huge amount of unecessary fbc sw cache clean.

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 24, 2014 at 07:50:59PM -0400, Rodrigo Vivi wrote:
> The sw cache clean on BDW is a tempoorary workaround because we cannot
> set cache clean on blt ring with risk of hungs. So we are doing the cache clean on sw.
> However we are doing much more than needed. Not only when using blt ring.
> So, with this extra w/a we minimize the ammount of cache cleans and call it only
> on same cases that it was being called on gen7.
> 
> The traditional FBC Cache clean happens over LRI on BLT ring when there is a
> frontbuffer touch happening. frontbuffer tracking set fbc_dirty variable
> to let BLT flush that it must clean FBC cache.
> 
> fbc.need_sw_cache_clean works in the opposite information direction
> of ring->fbc_dirty telling software on frontbuffer tracking to perform
> the cache clean on sw side.
> 
> v2: Clean it a little bit and fully check for Broadwell instead of gen8.
> 
> v3: Rebase after frontbuffer organization.
> 
> v4: Wiggle confused me. So fixing v3!
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h          | 10 +++++++++-
>  drivers/gpu/drm/i915/intel_frontbuffer.c |  6 ++++--
>  drivers/gpu/drm/i915/intel_pm.c          |  2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c  |  9 +++++++--
>  4 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index da607ab..4cd2aa3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -676,6 +676,14 @@ struct i915_fbc {
>  	 * possible. */
>  	bool enabled;
>  
> +	/* On gen8 some rings cannont perform fbc clean operation so for now
> +	 * we are doing this on SW with mmio.
> +	 * This variable works in the opposite information direction
> +	 * of ring->fbc_dirty telling software on frontbuffer tracking
> +	 * to perform the cache clean on sw side.
> +	 */
> +	bool need_sw_cache_clean;
> +
>  	struct intel_fbc_work {
>  		struct delayed_work work;
>  		struct drm_crtc *crtc;
> @@ -2843,7 +2851,7 @@ extern void intel_modeset_setup_hw_state(struct drm_device *dev,
>  extern void i915_redisable_vga(struct drm_device *dev);
>  extern void i915_redisable_vga_power_on(struct drm_device *dev);
>  extern bool intel_fbc_enabled(struct drm_device *dev);
> -extern void gen8_fbc_sw_flush(struct drm_device *dev, u32 value);
> +extern void bdw_fbc_sw_flush(struct drm_device *dev, u32 value);
>  extern void intel_disable_fbc(struct drm_device *dev);
>  extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
>  extern void intel_init_pch_refclk(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index f74744c..7eb74a6 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -189,8 +189,10 @@ void intel_frontbuffer_flush(struct drm_device *dev,
>  	 * needs to be reworked into a proper frontbuffer tracking scheme like
>  	 * psr employs.
>  	 */
> -	if (IS_BROADWELL(dev))
> -		gen8_fbc_sw_flush(dev, FBC_REND_CACHE_CLEAN);
> +	if (dev_priv->fbc.need_sw_cache_clean) {
> +		dev_priv->fbc.need_sw_cache_clean = false;
> +		bdw_fbc_sw_flush(dev, FBC_REND_CACHE_CLEAN);
> +	}
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index aaab056..36842c4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -380,7 +380,7 @@ bool intel_fbc_enabled(struct drm_device *dev)
>  	return dev_priv->fbc.enabled;
>  }
>  
> -void gen8_fbc_sw_flush(struct drm_device *dev, u32 value)
> +void bdw_fbc_sw_flush(struct drm_device *dev, u32 value)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 896f564..b56e031 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2237,6 +2237,7 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
>  			   u32 invalidate, u32 flush)
>  {
>  	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t cmd;
>  	int ret;
>  
> @@ -2267,8 +2268,12 @@ static int gen6_ring_flush(struct intel_engine_cs *ring,
>  	}
>  	intel_ring_advance(ring);
>  
> -	if (IS_GEN7(dev) && !invalidate && flush)
> -		return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
> +	if (!invalidate && flush) {
> +		if (IS_GEN7(dev))
> +			return gen7_ring_fbc_flush(ring, FBC_REND_CACHE_CLEAN);
> +		else if (IS_BROADWELL(dev))
> +			dev_priv->fbc.need_sw_cache_clean = true;
> +	}
>  
>  	return 0;
>  }
> -- 
> 1.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch