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

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

[Niranjana Vishwanathapura]

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.

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;
 }
 
-- 
2.21.0.rc0.32.g243a4c7e27

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: Do not set cache_dirty for DGFX

[Patchwork]

== Series Details ==

Series: drm/i915: Do not set cache_dirty for DGFX
URL   : https://patchwork.freedesktop.org/series/110399/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Do not set cache_dirty for DGFX

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 7690 bytes --]

== Series Details ==

Series: drm/i915: Do not set cache_dirty for DGFX
URL   : https://patchwork.freedesktop.org/series/110399/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12330 -> Patchwork_110399v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_110399v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_110399v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/index.html

Participating hosts (38 -> 29)
------------------------------

  Additional (3): fi-icl-u2 fi-tgl-dsi fi-pnv-d510 
  Missing    (12): fi-adl-ddr5 bat-dg2-8 bat-adlm-1 bat-dg2-9 bat-adlp-6 bat-adlp-4 bat-adln-1 bat-rplp-1 bat-rpls-1 bat-rpls-2 bat-dg2-11 bat-jsl-1 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_110399v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@dmabuf:
    - fi-bsw-kefka:       [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12330/fi-bsw-kefka/igt@i915_selftest@live@dmabuf.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-bsw-kefka/igt@i915_selftest@live@dmabuf.html

  
Known issues
------------

  Here are the changes found in Patchwork_110399v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-icl-u2:          NOTRUN -> [SKIP][3] ([i915#2190])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-icl-u2/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@random-engines:
    - fi-icl-u2:          NOTRUN -> [SKIP][4] ([i915#4613]) +3 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-icl-u2/igt@gem_lmem_swapping@random-engines.html

  * igt@i915_pm_rpm@module-reload:
    - fi-icl-u2:          NOTRUN -> [INCOMPLETE][5] ([i915#4890])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-icl-u2/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-cfl-guc:         [PASS][6] -> [DMESG-FAIL][7] ([i915#5334])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12330/fi-cfl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-cfl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-bdw-5557u:       [PASS][8] -> [INCOMPLETE][9] ([i915#146])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12330/fi-bdw-5557u/igt@i915_suspend@basic-s3-without-i915.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-bdw-5557u/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-hsw-4770:        NOTRUN -> [SKIP][10] ([fdo#109271] / [fdo#111827])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-hsw-4770/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          NOTRUN -> [SKIP][11] ([fdo#111827]) +7 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
    - fi-icl-u2:          NOTRUN -> [SKIP][12] ([i915#4103])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-icl-u2:          NOTRUN -> [SKIP][13] ([fdo#109285])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-icl-u2/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_psr@primary_page_flip:
    - fi-pnv-d510:        NOTRUN -> [SKIP][14] ([fdo#109271]) +43 similar issues
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-pnv-d510/igt@kms_psr@primary_page_flip.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-icl-u2:          NOTRUN -> [SKIP][15] ([i915#3555])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-icl-u2/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-userptr:
    - fi-icl-u2:          NOTRUN -> [SKIP][16] ([fdo#109295] / [i915#3301])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-icl-u2/igt@prime_vgem@basic-userptr.html

  * igt@runner@aborted:
    - fi-icl-u2:          NOTRUN -> [FAIL][17] ([i915#4312])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-icl-u2/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@gem_ctx_create@basic-files:
    - {fi-tgl-mst}:       [DMESG-WARN][18] ([i915#6434]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12330/fi-tgl-mst/igt@gem_ctx_create@basic-files.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-tgl-mst/igt@gem_ctx_create@basic-files.html

  * igt@i915_selftest@live@hangcheck:
    - fi-hsw-4770:        [INCOMPLETE][20] ([i915#4785]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12330/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#146]: https://gitlab.freedesktop.org/drm/intel/issues/146
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2411]: https://gitlab.freedesktop.org/drm/intel/issues/2411
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4785]: https://gitlab.freedesktop.org/drm/intel/issues/4785
  [i915#4890]: https://gitlab.freedesktop.org/drm/intel/issues/4890
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6434]: https://gitlab.freedesktop.org/drm/intel/issues/6434
  [i915#6856]: https://gitlab.freedesktop.org/drm/intel/issues/6856
  [i915#7125]: https://gitlab.freedesktop.org/drm/intel/issues/7125


Build changes
-------------

  * Linux: CI_DRM_12330 -> Patchwork_110399v1

  CI-20190529: 20190529
  CI_DRM_12330: b4169c84bf30172f83c6d365a927dcea0cd7a2ab @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7037: 6a25c53624502fc85cec3cf0a0bf244a2346e30f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_110399v1: b4169c84bf30172f83c6d365a927dcea0cd7a2ab @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

6be90ff6eb2b drm/i915: Do not set cache_dirty for DGFX

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110399v1/index.html

[-- Attachment #2: Type: text/html, Size: 8513 bytes --]

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

[Das, Nirmoy]

[-- Attachment #1: Type: text/plain, Size: 2258 bytes --]


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>

> 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;
>   }
>   

[-- Attachment #2: Type: text/html, Size: 3108 bytes --]

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

[Matthew Auld]

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;
>>   }
>>   

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

[Das, Nirmoy]

On 11/2/2022 11:36 AM, Matthew Auld wrote:
> 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.


I ran some userptr related igt tests none hit 
__i915_gem_object_release_shmem . So I think we are missing

coverage here or I/CI isn't running such test.

Niranjana, what test did you ran to hit this case WARN ?


Regards,

Nirmoy


>
> 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;
>>>   }

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

[Niranjana Vishwanathapura]

On Wed, Nov 02, 2022 at 05:09:27PM +0100, Das, Nirmoy wrote:
>
>On 11/2/2022 11:36 AM, Matthew Auld wrote:
>>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")
>>>

Ok, will add.

>>>Acked-by: Nirmoy Das <nirmoy.das@intel.com>
>>
>>Any idea why this escaped our testing in CI? Perhaps something to 
>>improve.
>
>
>I ran some userptr related igt tests none hit 
>__i915_gem_object_release_shmem . So I think we are missing
>
>coverage here or I/CI isn't running such test.
>
>Niranjana, what test did you ran to hit this case WARN ?
>

I hit this issue with modified gem_userptr_blits@vma-merge where
I added additional execbuf call after userptr invalidation as below
to test rebind happens properly after an userptr invalidation.

         igt_spin_end(spin);
+       igt_spin_reset(spin);
+
+       gem_execbuf_wr(i915, &spin->execbuf);
+       igt_spin_end(spin);
+
         gem_close(i915, handle);

         munmap(addr, sz);

Note that vma-merge subtest fails due to some other issue, but still
is good enough to reproduce this issue and test the fix.

Niranjana

>
>Regards,
>
>Nirmoy
>
>
>>
>>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;
>>>>  }

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

[Andi Shyti]

Hi Niranjana,

On Tue, Nov 01, 2022 at 10:14:16PM -0700, 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.
> 
> 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>

The BAT failure doesn't look related to this patch, to me.

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

Thanks,
Andi

> ---
>  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;
>  }
>  
> -- 
> 2.21.0.rc0.32.g243a4c7e27

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

[Andi Shyti]

Hi Niranjana,

On Tue, Nov 01, 2022 at 10:14:16PM -0700, 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.
> 
> 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>

Pushed in drm-intel-gt-next.

Thanks,
Andi