Re: [Intel-gfx] [PATCH 2/3] drm/i915: Make i915_coherent_map_type GT-centric

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Jonathan,

On Wed, Jul 26, 2023 at 08:53:55AM -0700, Jonathan Cavitt wrote:
> Refactor i915_coherent_map_type to be GT-centric rather than
> device-centric.  Each GT may require different coherency
> handling due to hardware workarounds.
> 
> Since the function now takes a GT instead of the i915, the function is
> renamed and moved to the gt folder.

[...]

> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -465,21 +465,6 @@ void *i915_gem_object_pin_map_unlocked(struct drm_i915_gem_object *obj,
>  	return ret;
>  }
>  
> -enum i915_map_type i915_coherent_map_type(struct drm_i915_private *i915,
> -					  struct drm_i915_gem_object *obj,
> -					  bool always_coherent)
> -{
> -	/*
> -	 * Wa_22016122933: always return I915_MAP_WC for MTL
> -	 */
> -	if (i915_gem_object_is_lmem(obj) || IS_METEORLAKE(i915))
> -		return I915_MAP_WC;
> -	if (HAS_LLC(i915) || always_coherent)
> -		return I915_MAP_WB;
> -	else
> -		return I915_MAP_WC;
> -}
> -

[...]

> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -1134,6 +1134,21 @@ void intel_gt_invalidate_tlb(struct intel_gt *gt, u32 seqno)
>  	}
>  }
>  
> +enum i915_map_type intel_gt_coherent_map_type(struct intel_gt *gt,
> +					      struct drm_i915_gem_object *obj,
> +					      bool always_coherent)
> +{
> +	/*
> +	 * Wa_22016122933: always return I915_MAP_WC for MTL
> +	 */
> +	if (i915_gem_object_is_lmem(obj) || IS_METEORLAKE(gt->i915))
> +		return I915_MAP_WC;
> +	if (HAS_LLC(gt->i915) || always_coherent)
> +		return I915_MAP_WB;
> +	else
> +		return I915_MAP_WC;
> +}
> +

yep! That's how it should be! Thanks for moving it.

I'm just thinking whether intel_gt.c is the right place to put
it... I'm anyway OK with it.


> --- a/drivers/gpu/drm/i915/gt/intel_ring.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring.c
> @@ -13,6 +13,7 @@
>  #include "intel_engine_regs.h"
>  #include "intel_gpu_commands.h"
>  #include "intel_ring.h"
> +#include "intel_gt.h"

mmhhh... pity... this required this extra include because it
needs the prototype of intel_gt_coherent_map_type();

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>  

Andi