Re: [Intel-gfx] [PATCH] drm/i915: Do not set cache_dirty for DGFX

[Matthew Auld <matthew.auld@intel.com>]

On 02/11/2022 07:39, Das, Nirmoy wrote:
> 
> On 11/2/2022 6:14 AM, Niranjana Vishwanathapura wrote:
>> Currently on DG1, which do not have LLC, we hit the below
>> warning while rebinding an userptr invalidated object.
>>
>> WARNING: CPU: 4 PID: 13008 at drivers/gpu/drm/i915/gem/i915_gem_pages.c:34 __i915_gem_object_set_pages+0x296/0x2d0 [i915]
>> ...
>> RIP: 0010:__i915_gem_object_set_pages+0x296/0x2d0 [i915]
>> ...
>> Call Trace:
>>   <TASK>
>>   i915_gem_userptr_get_pages+0x175/0x1a0 [i915]
>>   ____i915_gem_object_get_pages+0x32/0xb0 [i915]
>>   i915_gem_object_userptr_submit_init+0x286/0x470 [i915]
>>   eb_lookup_vmas+0x2ff/0xcf0 [i915]
>>   ? __intel_wakeref_get_first+0x55/0xb0 [i915]
>>   i915_gem_do_execbuffer+0x785/0x21d0 [i915]
>>   i915_gem_execbuffer2_ioctl+0xe7/0x3d0 [i915]
>>
>> We shouldn't be setting the obj->cache_dirty for DGFX,
>> fix it.
> 
> With Fixes: |d70af57944 |("drm/i915/shmem: ensure flush during swap-in 
> on non-LLC")
> 
> Acked-by: Nirmoy Das <nirmoy.das@intel.com>

Any idea why this escaped our testing in CI? Perhaps something to improve.

Reviewed-by: Matthew Auld <matthew.auld@intel.com>

> 
>> Suggested-by: Matthew Auld<matthew.auld@intel.com>
>> Reported-by: Niranjana Vishwanathapura<niranjana.vishwanathapura@intel.com>
>> Signed-off-by: Niranjana Vishwanathapura<niranjana.vishwanathapura@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> index 11125c32dd35..2f7804492cd5 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
>> @@ -369,14 +369,14 @@ __i915_gem_object_release_shmem(struct drm_i915_gem_object *obj,
>>   
>>   	__start_cpu_write(obj);
>>   	/*
>> -	 * On non-LLC platforms, force the flush-on-acquire if this is ever
>> +	 * On non-LLC igfx platforms, force the flush-on-acquire if this is ever
>>   	 * swapped-in. Our async flush path is not trust worthy enough yet(and
>>   	 * happens in the wrong order), and with some tricks it's conceivable
>>   	 * for userspace to change the cache-level to I915_CACHE_NONE after the
>>   	 * pages are swapped-in, and since execbuf binds the object before doing
>>   	 * the async flush, we have a race window.
>>   	 */
>> -	if (!HAS_LLC(i915))
>> +	if (!HAS_LLC(i915) && !IS_DGFX(i915))
>>   		obj->cache_dirty = true;
>>   }
>>