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

[Paulo Zanoni]

2014-09-18 12:29 GMT-03:00 Rodrigo Vivi <rodrigo.vivi at intel.com>:
> 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.

This is the first time that I try to understand the buffer tracking
code, but as far as I could understood - and as far as you explained
stuff to me on IRC - the code looks good. But we still could use the
review of either Ville or Daniel. With that said:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>

And the s/IS_GEN8/IS_BROADWELL/ thing ideally would be a separate
patch, but it's fine this way too.

>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         | 10 +++++++++-
>  drivers/gpu/drm/i915/intel_display.c    |  6 ++++--
>  drivers/gpu/drm/i915/intel_pm.c         |  4 ++--
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  9 +++++++--
>  4 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e3ca8df..a8636e5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -659,6 +659,14 @@ struct i915_fbc {
>
>         bool false_color;
>
> +       /* 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;
> @@ -2820,7 +2828,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_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b78f00a..20ba1eb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9225,8 +9225,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 6f3b94b..40377c7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -345,11 +345,11 @@ bool intel_fbc_enabled(struct drm_device *dev)
>         return dev_priv->display.fbc_enabled(dev);
>  }
>
> -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;
>
> -       if (!IS_GEN8(dev))
> +       if (!IS_BROADWELL(dev))
>                 return;
>
>         if (!intel_fbc_enabled(dev))
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 25795f2..c2ddf5f 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2240,6 +2240,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;
>
> @@ -2270,8 +2271,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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni