Re: [Intel-gfx] [RFC 5/8] drm/i915: Improve the vm_fault_gtt user PAT index restriction

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 28/07/2023 01:04, Matt Roper wrote:
> On Thu, Jul 27, 2023 at 03:55:01PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Now that i915 understands the caching modes behind PAT indices, we can
>> refine the check in vm_fault_gtt() to not reject the uncached PAT if it
>> was set by userspace on a snoopable platform.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Fei Yang <fei.yang@intel.com>
>> Cc: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 14 +++-----------
>>   1 file changed, 3 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index cd7f8ded0d6f..9aa6ecf68432 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -382,17 +382,9 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf)
>>   		goto err_reset;
>>   	}
>>   
>> -	/*
>> -	 * For objects created by userspace through GEM_CREATE with pat_index
>> -	 * set by set_pat extension, coherency is managed by userspace, make
>> -	 * sure we don't fail handling the vm fault by calling
>> -	 * i915_gem_object_has_cache_level() which always return true for such
>> -	 * objects. Otherwise this helper function would fall back to checking
>> -	 * whether the object is un-cached.
>> -	 */
>> -	if (!((obj->pat_set_by_user ||
>> -	       i915_gem_object_has_cache_mode(obj, I915_CACHE_MODE_UC)) ||
>> -	      HAS_LLC(i915))) {
>> +	/* Access to snoopable pages through the GTT is incoherent. */
> 
> This comment was removed in the previous patch, but now it came back
> here.  Should we have just left it be in the previous patch?

Oops yes, fumble when splitting the single patch into this series.

> I'm not really clear on what it means either.  Are we using "GTT" as
> shorthand to refer to the aperture here?

It is about CPU mmap access so I think so.

Original code was:

         /* Access to snoopable pages through the GTT is incoherent. */
         if (obj->cache_level != I915_CACHE_NONE && !HAS_LLC(i915)) {
                 ret = -EFAULT;
                 goto err_unpin;
         }

Which was disallowing anything not uncached on snoopable platforms. So I 
made it equivalent to that:

	/* Access to snoopable pages through the GTT is incoherent. */
	if (!i915_gem_object_has_cache_mode(obj, I915_CACHE_MODE_UC) &&
	    !HAS_LLC(i915)) {
		ret = -EFAULT;
		goto err_unpin;
	}

Should be like-for-like assuming PAT-to-cache-mode tables are all good.

On Meteorlake it is no change in behaviour either way due !HAS_LLC.

Regards,

Tvrtko


> 
> Matt
> 
>> +	if (!i915_gem_object_has_cache_mode(obj, I915_CACHE_MODE_UC) &&
>> +	    !HAS_LLC(i915)) {
>>   		ret = -EFAULT;
>>   		goto err_unpin;
>>   	}
>> -- 
>> 2.39.2
>>
>