Re: [Intel-gfx] [PATCH 5/8] drm/i915/mtl: end support for set caching ioctl

[Andrzej Hajda <andrzej.hajda@intel.com>]

On 17.04.2023 08:25, fei.yang@intel.com wrote:
> From: Fei Yang <fei.yang@intel.com>
> 
> The design is to keep Buffer Object's caching policy immutable through
> out its life cycle. This patch ends the support for set caching ioctl
> from MTL onward. While doing that we also set BO's to be 1-way coherent
> at creation time because GPU is no longer automatically snooping CPU
> cache.
> 
> Signed-off-by: Fei Yang <fei.yang@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_domain.c | 3 +++
>   drivers/gpu/drm/i915/gem/i915_gem_shmem.c  | 9 ++++++++-
>   2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> index d2d5a24301b2..bb3575b1479f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> @@ -337,6 +337,9 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
>   	if (IS_DGFX(i915))
>   		return -ENODEV;
>   
> +	if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70))
> +		return -EOPNOTSUPP;
> +
>   	switch (args->caching) {
>   	case I915_CACHING_NONE:
>   		level = I915_CACHE_NONE;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 37d1efcd3ca6..e602c323896b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -601,7 +601,14 @@ static int shmem_object_init(struct intel_memory_region *mem,
>   	obj->write_domain = I915_GEM_DOMAIN_CPU;
>   	obj->read_domains = I915_GEM_DOMAIN_CPU;
>   
> -	if (HAS_LLC(i915))
> +	/*
> +	 * MTL doesn't snooping CPU cache by default for GPU access (namely

Sounds strange, "doesn't snoop" ?


> +	 * 1-way coherency). However some UMD's are currently depending on
> +	 * that. Make 1-way coherent the default setting for MTL. A follow
> +	 * up patch will extend the GEM_CREATE uAPI to allow UMD's specify
> +	 * caching mode at BO creation time

Shouldn't such comment be a part of patch description? or at least 
removed by follow-up patch.

Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej


> +	 */
> +	if (HAS_LLC(i915) || (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)))
>   		/* On some devices, we can have the GPU use the LLC (the CPU
>   		 * cache) for about a 10% performance improvement
>   		 * compared to uncached.  Graphics requests other than