[Intel-gfx] [RFC PATCH 150/162] drm/i915: need consider system BO snoop for dgfx

[Chris Wilson]

Quoting Matthew Auld (2020-11-27 12:07:06)
> From: CQ Tang <cq.tang at intel.com>
> 
> When cache_level is NONE, we check HAS_LLC(i915).
> But additionally for DGFX, we also need to check
> HAS_SNOOP(i915) on system memory object to use
> I915_BO_CACHE_COHERENT_FOR_READ. on dg1, has_llc=0, and
> has_snoop=1. Otherwise, we set obj->cache_choerent=0 and
> have performance impact.
> 
> Cc: Chris P Wilson <chris.p.wilson at intel.com>
> Cc: Ramalingam C <ramalingam.c at intel.com>
> Cc: Sudeep Dutt <sudeep.dutt at intel.com>
> Cc: Matthew Auld <matthew.auld at intel.com>
> Signed-off-by: CQ Tang <cq.tang at intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_object.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> index ddb448f275eb..be603171c444 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> @@ -95,6 +95,20 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
>         mutex_init(&obj->mm.get_dma_page.lock);
>  }
>  
> +static bool i915_gem_object_use_llc(struct drm_i915_gem_object *obj)
> +{
> +       struct drm_i915_private *i915 = to_i915(obj->base.dev);
> +
> +       if (HAS_LLC(i915))
> +               return true;
> +
> +       if (IS_DGFX(i915) && HAS_SNOOP(i915) &&
> +           !i915_gem_object_is_lmem(obj))
> +               return true;
> +
> +       return false;
> +}
> +
>  /**
>   * Mark up the object's coherency levels for a given cache_level
>   * @obj: #drm_i915_gem_object
> @@ -108,7 +122,7 @@ void i915_gem_object_set_cache_coherency(struct drm_i915_gem_object *obj,
>         if (cache_level != I915_CACHE_NONE)
>                 obj->cache_coherent = (I915_BO_CACHE_COHERENT_FOR_READ |
>                                        I915_BO_CACHE_COHERENT_FOR_WRITE);
> -       else if (HAS_LLC(to_i915(obj->base.dev)))
> +       else if (i915_gem_object_use_llc(obj))
>                 obj->cache_coherent = I915_BO_CACHE_COHERENT_FOR_READ;
>         else
>                 obj->cache_coherent = 0;

You must also define obj->cache_level correctly. You can not just assume
the object will be snooped.
-Chris
---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.