* [PATCH v3 0/2] drm/i915: Fix timeout handling when retiring requests
@ 2022-11-21 14:56 ` Janusz Krzysztofik
0 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 14:56 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen
Cc: Matthew Brost, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Janusz Krzysztofik, John Harrison, Nirmoy Das
Fixes for issues discovered via code review while working on
https://gitlab.freedesktop.org/drm/intel/issues/7349.
v3:
PATCH 1: don't fail on any error passed back via remaining_timeout,
PATCH 2: use conditional expression, more compact but also better
reflecting intention standing behind the change.
v2:
PATCH 1: fix the issue on the caller side, not the provider,
reword commit message and description.
PATCH 2: move the added lines down so flush_submission() is not affected,
reword commit message and description.
PATCH 3: drop -- controversial, not needed.
Janusz Krzysztofik (2):
drm/i915: Fix negative value passed as remaining time
drm/i915: Never return 0 if not all requests retired
drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
2 files changed, 8 insertions(+), 3 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Intel-gfx] [PATCH v3 0/2] drm/i915: Fix timeout handling when retiring requests
@ 2022-11-21 14:56 ` Janusz Krzysztofik
0 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 14:56 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen
Cc: Daniel Vetter, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Nirmoy Das
Fixes for issues discovered via code review while working on
https://gitlab.freedesktop.org/drm/intel/issues/7349.
v3:
PATCH 1: don't fail on any error passed back via remaining_timeout,
PATCH 2: use conditional expression, more compact but also better
reflecting intention standing behind the change.
v2:
PATCH 1: fix the issue on the caller side, not the provider,
reword commit message and description.
PATCH 2: move the added lines down so flush_submission() is not affected,
reword commit message and description.
PATCH 3: drop -- controversial, not needed.
Janusz Krzysztofik (2):
drm/i915: Fix negative value passed as remaining time
drm/i915: Never return 0 if not all requests retired
drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
2 files changed, 8 insertions(+), 3 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 1/2] drm/i915: Fix negative value passed as remaining time
2022-11-21 14:56 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-21 14:56 ` Janusz Krzysztofik
-1 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 14:56 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen
Cc: Matthew Brost, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Janusz Krzysztofik, John Harrison, Nirmoy Das
Commit b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
with GuC") extended the API of intel_gt_retire_requests_timeout() with an
extra argument 'remaining_timeout', intended for passing back unconsumed
portion of requested timeout when 0 (success) is returned. However, when
request retirement happens to succeed despite an error returned by a call
to dma_fence_wait_timeout(), that error code (a negative value) is passed
back instead of remaining time. If we then pass that negative value
forward as requested timeout to intel_uc_wait_for_idle(), an explicit BUG
will be triggered.
If request retirement succeeds but an error code is passed back via
remaininig_timeout, we may have no clue on how much of the initial timeout
might have been left for spending it on waiting for GuC to become idle.
OTOH, since all pending requests have been successfully retired, that
error code has been already ignored by intel_gt_retire_requests_timeout(),
then we shouldn't fail.
Assume no more time has been left on error and pass 0 timeout value to
intel_uc_wait_for_idle() to give it a chance to return success if GuC is
already idle.
v3: Don't fail on any error passed back via remaining_timeout.
v2: Fix the issue on the caller side, not the provider.
Fixes: b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC")
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: stable@vger.kernel.org # v5.15+
---
drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index b5ad9caa55372..7ef0edb2e37cd 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -677,8 +677,13 @@ int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
return -EINTR;
}
- return timeout ? timeout : intel_uc_wait_for_idle(>->uc,
- remaining_timeout);
+ if (timeout)
+ return timeout;
+
+ if (remaining_timeout < 0)
+ remaining_timeout = 0;
+
+ return intel_uc_wait_for_idle(>->uc, remaining_timeout);
}
int intel_gt_init(struct intel_gt *gt)
--
2.25.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Intel-gfx] [PATCH v3 1/2] drm/i915: Fix negative value passed as remaining time
@ 2022-11-21 14:56 ` Janusz Krzysztofik
0 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 14:56 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen
Cc: Daniel Vetter, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Nirmoy Das
Commit b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
with GuC") extended the API of intel_gt_retire_requests_timeout() with an
extra argument 'remaining_timeout', intended for passing back unconsumed
portion of requested timeout when 0 (success) is returned. However, when
request retirement happens to succeed despite an error returned by a call
to dma_fence_wait_timeout(), that error code (a negative value) is passed
back instead of remaining time. If we then pass that negative value
forward as requested timeout to intel_uc_wait_for_idle(), an explicit BUG
will be triggered.
If request retirement succeeds but an error code is passed back via
remaininig_timeout, we may have no clue on how much of the initial timeout
might have been left for spending it on waiting for GuC to become idle.
OTOH, since all pending requests have been successfully retired, that
error code has been already ignored by intel_gt_retire_requests_timeout(),
then we shouldn't fail.
Assume no more time has been left on error and pass 0 timeout value to
intel_uc_wait_for_idle() to give it a chance to return success if GuC is
already idle.
v3: Don't fail on any error passed back via remaining_timeout.
v2: Fix the issue on the caller side, not the provider.
Fixes: b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC")
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: stable@vger.kernel.org # v5.15+
---
drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index b5ad9caa55372..7ef0edb2e37cd 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -677,8 +677,13 @@ int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
return -EINTR;
}
- return timeout ? timeout : intel_uc_wait_for_idle(>->uc,
- remaining_timeout);
+ if (timeout)
+ return timeout;
+
+ if (remaining_timeout < 0)
+ remaining_timeout = 0;
+
+ return intel_uc_wait_for_idle(>->uc, remaining_timeout);
}
int intel_gt_init(struct intel_gt *gt)
--
2.25.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-21 14:56 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-21 14:56 ` Janusz Krzysztofik
-1 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 14:56 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen
Cc: Matthew Brost, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Janusz Krzysztofik, John Harrison, Nirmoy Das
Users of intel_gt_retire_requests_timeout() expect 0 return value on
success. However, we have no protection from passing back 0 potentially
returned by a call to dma_fence_wait_timeout() when it succedes right
after its timeout has expired.
Replace 0 with -ETIME before potentially using the timeout value as return
code, so -ETIME is returned if there are still some requests not retired
after timeout, 0 otherwise.
v3: Use conditional expression, more compact but also better reflecting
intention standing behind the change.
v2: Move the added lines down so flush_submission() is not affected.
Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: stable@vger.kernel.org # v5.5+
---
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index edb881d756309..1dfd01668c79c 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
if (remaining_timeout)
*remaining_timeout = timeout;
- return active_count ? timeout : 0;
+ return active_count ? timeout ?: -ETIME : 0;
}
static void retire_work_handler(struct work_struct *work)
--
2.25.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Intel-gfx] [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-21 14:56 ` Janusz Krzysztofik
0 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 14:56 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen
Cc: Daniel Vetter, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Nirmoy Das
Users of intel_gt_retire_requests_timeout() expect 0 return value on
success. However, we have no protection from passing back 0 potentially
returned by a call to dma_fence_wait_timeout() when it succedes right
after its timeout has expired.
Replace 0 with -ETIME before potentially using the timeout value as return
code, so -ETIME is returned if there are still some requests not retired
after timeout, 0 otherwise.
v3: Use conditional expression, more compact but also better reflecting
intention standing behind the change.
v2: Move the added lines down so flush_submission() is not affected.
Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: stable@vger.kernel.org # v5.5+
---
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index edb881d756309..1dfd01668c79c 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
if (remaining_timeout)
*remaining_timeout = timeout;
- return active_count ? timeout : 0;
+ return active_count ? timeout ?: -ETIME : 0;
}
static void retire_work_handler(struct work_struct *work)
--
2.25.1
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Intel-gfx] ✗ Fi.CI.DOCS: warning for drm/i915: Fix timeout handling when retiring requests (rev3)
2022-11-21 14:56 ` [Intel-gfx] " Janusz Krzysztofik
` (2 preceding siblings ...)
(?)
@ 2022-11-21 15:37 ` Patchwork
-1 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2022-11-21 15:37 UTC (permalink / raw)
To: Janusz Krzysztofik; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Fix timeout handling when retiring requests (rev3)
URL : https://patchwork.freedesktop.org/series/110964/
State : warning
== Summary ==
Error: make htmldocs had i915 warnings
./drivers/gpu/drm/i915/gt/intel_gt_mcr.c:739: warning: expecting prototype for intel_gt_mcr_wait_for_reg_fw(). Prototype was for intel_gt_mcr_wait_for_reg() instead
./drivers/gpu/drm/i915/gt/intel_gt_mcr.c:739: warning: expecting prototype for intel_gt_mcr_wait_for_reg_fw(). Prototype was for intel_gt_mcr_wait_for_reg() instead
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Fix timeout handling when retiring requests (rev3)
2022-11-21 14:56 ` [Intel-gfx] " Janusz Krzysztofik
` (3 preceding siblings ...)
(?)
@ 2022-11-21 15:56 ` Patchwork
-1 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2022-11-21 15:56 UTC (permalink / raw)
To: Janusz Krzysztofik; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 9405 bytes --]
== Series Details ==
Series: drm/i915: Fix timeout handling when retiring requests (rev3)
URL : https://patchwork.freedesktop.org/series/110964/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_12407 -> Patchwork_110964v3
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/index.html
Participating hosts (23 -> 34)
------------------------------
Additional (13): bat-dg1-7 bat-kbl-2 bat-dg2-8 bat-adlm-1 bat-dg2-9 bat-adlp-6 bat-adlp-4 fi-hsw-4770 bat-adln-1 bat-rplp-1 bat-rpls-2 bat-dg2-11 bat-jsl-1
Missing (2): fi-ilk-m540 fi-bsw-nick
Known issues
------------
Here are the changes found in Patchwork_110964v3 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@debugfs_test@basic-hwmon:
- bat-adlp-4: NOTRUN -> [SKIP][1] ([i915#7456])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/bat-adlp-4/igt@debugfs_test@basic-hwmon.html
* igt@gem_lmem_swapping@verify-random:
- bat-adlp-4: NOTRUN -> [SKIP][2] ([i915#4613]) +3 similar issues
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/bat-adlp-4/igt@gem_lmem_swapping@verify-random.html
* igt@gem_tiled_pread_basic:
- bat-adlp-4: NOTRUN -> [SKIP][3] ([i915#3282])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/bat-adlp-4/igt@gem_tiled_pread_basic.html
* igt@i915_pm_rps@basic-api:
- bat-adlp-4: NOTRUN -> [SKIP][4] ([i915#6621])
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/bat-adlp-4/igt@i915_pm_rps@basic-api.html
* igt@i915_selftest@live@gt_heartbeat:
- fi-apl-guc: [PASS][5] -> [DMESG-FAIL][6] ([i915#5334])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
* igt@i915_selftest@live@hangcheck:
- fi-hsw-4770: NOTRUN -> [INCOMPLETE][7] ([i915#4785])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
* igt@i915_suspend@basic-s3-without-i915:
- fi-rkl-11600: [PASS][8] -> [INCOMPLETE][9] ([i915#4817])
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html
* igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
- fi-hsw-4770: NOTRUN -> [SKIP][10] ([fdo#109271]) +11 similar issues
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/fi-hsw-4770/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html
* igt@kms_chamelium@dp-crc-fast:
- fi-hsw-4770: NOTRUN -> [SKIP][11] ([fdo#109271] / [fdo#111827]) +7 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/fi-hsw-4770/igt@kms_chamelium@dp-crc-fast.html
- bat-adlp-4: NOTRUN -> [SKIP][12] ([fdo#111827]) +8 similar issues
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/bat-adlp-4/igt@kms_chamelium@dp-crc-fast.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
- bat-adlp-4: NOTRUN -> [SKIP][13] ([i915#4103])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/bat-adlp-4/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html
* igt@kms_force_connector_basic@prune-stale-modes:
- bat-adlp-4: NOTRUN -> [SKIP][14] ([i915#4093]) +3 similar issues
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/bat-adlp-4/igt@kms_force_connector_basic@prune-stale-modes.html
* igt@kms_pipe_crc_basic@suspend-read-crc:
- bat-adlp-4: NOTRUN -> [SKIP][15] ([i915#3546])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/bat-adlp-4/igt@kms_pipe_crc_basic@suspend-read-crc.html
* igt@kms_psr@sprite_plane_onoff:
- fi-hsw-4770: NOTRUN -> [SKIP][16] ([fdo#109271] / [i915#1072]) +3 similar issues
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/fi-hsw-4770/igt@kms_psr@sprite_plane_onoff.html
* igt@kms_setmode@basic-clone-single-crtc:
- bat-adlp-4: NOTRUN -> [SKIP][17] ([i915#3555] / [i915#4579])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/bat-adlp-4/igt@kms_setmode@basic-clone-single-crtc.html
* igt@prime_vgem@basic-userptr:
- bat-adlp-4: NOTRUN -> [SKIP][18] ([fdo#109295] / [i915#3301] / [i915#3708])
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/bat-adlp-4/igt@prime_vgem@basic-userptr.html
* igt@prime_vgem@basic-write:
- bat-adlp-4: NOTRUN -> [SKIP][19] ([fdo#109295] / [i915#3291] / [i915#3708]) +2 similar issues
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/bat-adlp-4/igt@prime_vgem@basic-write.html
* igt@runner@aborted:
- fi-hsw-4770: NOTRUN -> [FAIL][20] ([fdo#109271] / [i915#4312] / [i915#5594])
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/fi-hsw-4770/igt@runner@aborted.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#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
[fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
[i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
[i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
[i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
[i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
[i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
[i915#3003]: https://gitlab.freedesktop.org/drm/intel/issues/3003
[i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
[i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
[i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
[i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
[i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
[i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
[i915#4093]: https://gitlab.freedesktop.org/drm/intel/issues/4093
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
[i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
[i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
[i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258
[i915#4303]: https://gitlab.freedesktop.org/drm/intel/issues/4303
[i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
[i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#4785]: https://gitlab.freedesktop.org/drm/intel/issues/4785
[i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
[i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
[i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
[i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
[i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
[i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
[i915#5594]: https://gitlab.freedesktop.org/drm/intel/issues/5594
[i915#6559]: https://gitlab.freedesktop.org/drm/intel/issues/6559
[i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
[i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
[i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
[i915#7346]: https://gitlab.freedesktop.org/drm/intel/issues/7346
[i915#7359]: https://gitlab.freedesktop.org/drm/intel/issues/7359
[i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
[i915#7498]: https://gitlab.freedesktop.org/drm/intel/issues/7498
[i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
Build changes
-------------
* Linux: CI_DRM_12407 -> Patchwork_110964v3
CI-20190529: 20190529
CI_DRM_12407: acd6b3e8e35f7b7b5ce9d16d85f5cdc1e5a94bdf @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7069: 40a2de5cc6a6b43af7da7905bfe1ede9d9a3200c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_110964v3: acd6b3e8e35f7b7b5ce9d16d85f5cdc1e5a94bdf @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
ff2c74bb4268 drm/i915: Never return 0 if not all requests retired
c56ba5ca18a1 drm/i915: Fix negative value passed as remaining time
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/index.html
[-- Attachment #2: Type: text/html, Size: 9017 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Fix negative value passed as remaining time
2022-11-21 14:56 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-21 17:40 ` Andrzej Hajda
-1 siblings, 0 replies; 29+ messages in thread
From: Andrzej Hajda @ 2022-11-21 17:40 UTC (permalink / raw)
To: Janusz Krzysztofik, Tvrtko Ursulin, Joonas Lahtinen
Cc: dri-devel, intel-gfx, Chris Wilson, Nirmoy Das
On 21.11.2022 15:56, Janusz Krzysztofik wrote:
> Commit b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
> with GuC") extended the API of intel_gt_retire_requests_timeout() with an
> extra argument 'remaining_timeout', intended for passing back unconsumed
> portion of requested timeout when 0 (success) is returned. However, when
> request retirement happens to succeed despite an error returned by a call
> to dma_fence_wait_timeout(), that error code (a negative value) is passed
> back instead of remaining time. If we then pass that negative value
> forward as requested timeout to intel_uc_wait_for_idle(), an explicit BUG
> will be triggered.
>
> If request retirement succeeds but an error code is passed back via
> remaininig_timeout, we may have no clue on how much of the initial timeout
> might have been left for spending it on waiting for GuC to become idle.
> OTOH, since all pending requests have been successfully retired, that
> error code has been already ignored by intel_gt_retire_requests_timeout(),
> then we shouldn't fail.
>
> Assume no more time has been left on error and pass 0 timeout value to
> intel_uc_wait_for_idle() to give it a chance to return success if GuC is
> already idle.
>
> v3: Don't fail on any error passed back via remaining_timeout.
>
> v2: Fix the issue on the caller side, not the provider.
>
> Fixes: b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC")
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: stable@vger.kernel.org # v5.15+
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
Regards
Andrzej
> ---
> drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index b5ad9caa55372..7ef0edb2e37cd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -677,8 +677,13 @@ int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
> return -EINTR;
> }
>
> - return timeout ? timeout : intel_uc_wait_for_idle(>->uc,
> - remaining_timeout);
> + if (timeout)
> + return timeout;
> +
> + if (remaining_timeout < 0)
> + remaining_timeout = 0;
> +
> + return intel_uc_wait_for_idle(>->uc, remaining_timeout);
> }
>
> int intel_gt_init(struct intel_gt *gt)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Fix negative value passed as remaining time
@ 2022-11-21 17:40 ` Andrzej Hajda
0 siblings, 0 replies; 29+ messages in thread
From: Andrzej Hajda @ 2022-11-21 17:40 UTC (permalink / raw)
To: Janusz Krzysztofik, Tvrtko Ursulin, Joonas Lahtinen
Cc: dri-devel, intel-gfx, Chris Wilson, Daniel Vetter, Nirmoy Das
On 21.11.2022 15:56, Janusz Krzysztofik wrote:
> Commit b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
> with GuC") extended the API of intel_gt_retire_requests_timeout() with an
> extra argument 'remaining_timeout', intended for passing back unconsumed
> portion of requested timeout when 0 (success) is returned. However, when
> request retirement happens to succeed despite an error returned by a call
> to dma_fence_wait_timeout(), that error code (a negative value) is passed
> back instead of remaining time. If we then pass that negative value
> forward as requested timeout to intel_uc_wait_for_idle(), an explicit BUG
> will be triggered.
>
> If request retirement succeeds but an error code is passed back via
> remaininig_timeout, we may have no clue on how much of the initial timeout
> might have been left for spending it on waiting for GuC to become idle.
> OTOH, since all pending requests have been successfully retired, that
> error code has been already ignored by intel_gt_retire_requests_timeout(),
> then we shouldn't fail.
>
> Assume no more time has been left on error and pass 0 timeout value to
> intel_uc_wait_for_idle() to give it a chance to return success if GuC is
> already idle.
>
> v3: Don't fail on any error passed back via remaining_timeout.
>
> v2: Fix the issue on the caller side, not the provider.
>
> Fixes: b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC")
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: stable@vger.kernel.org # v5.15+
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
Regards
Andrzej
> ---
> drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index b5ad9caa55372..7ef0edb2e37cd 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -677,8 +677,13 @@ int intel_gt_wait_for_idle(struct intel_gt *gt, long timeout)
> return -EINTR;
> }
>
> - return timeout ? timeout : intel_uc_wait_for_idle(>->uc,
> - remaining_timeout);
> + if (timeout)
> + return timeout;
> +
> + if (remaining_timeout < 0)
> + remaining_timeout = 0;
> +
> + return intel_uc_wait_for_idle(>->uc, remaining_timeout);
> }
>
> int intel_gt_init(struct intel_gt *gt)
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915: Fix timeout handling when retiring requests (rev3)
2022-11-21 14:56 ` [Intel-gfx] " Janusz Krzysztofik
` (4 preceding siblings ...)
(?)
@ 2022-11-21 18:03 ` Patchwork
-1 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2022-11-21 18:03 UTC (permalink / raw)
To: Janusz Krzysztofik; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 46450 bytes --]
== Series Details ==
Series: drm/i915: Fix timeout handling when retiring requests (rev3)
URL : https://patchwork.freedesktop.org/series/110964/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_12407_full -> Patchwork_110964v3_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Participating hosts (9 -> 10)
------------------------------
Additional (1): shard-rkl
Known issues
------------
Here are the changes found in Patchwork_110964v3_full that come from known issues:
### CI changes ###
#### Possible fixes ####
* boot:
- shard-glk: ([PASS][1], [PASS][2], [PASS][3], [PASS][4], [PASS][5], [PASS][6], [PASS][7], [PASS][8], [PASS][9], [PASS][10], [PASS][11], [FAIL][12], [PASS][13], [PASS][14], [PASS][15], [PASS][16], [PASS][17], [PASS][18], [PASS][19], [PASS][20], [PASS][21], [PASS][22], [PASS][23], [PASS][24], [PASS][25]) ([i915#4392]) -> ([PASS][26], [PASS][27], [PASS][28], [PASS][29], [PASS][30], [PASS][31], [PASS][32], [PASS][33], [PASS][34], [PASS][35], [PASS][36], [PASS][37], [PASS][38], [PASS][39], [PASS][40], [PASS][41], [PASS][42], [PASS][43], [PASS][44], [PASS][45], [PASS][46], [PASS][47], [PASS][48], [PASS][49], [PASS][50])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk3/boot.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk3/boot.html
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk3/boot.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk2/boot.html
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk2/boot.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk2/boot.html
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk1/boot.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk1/boot.html
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk9/boot.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk9/boot.html
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk9/boot.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk8/boot.html
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk8/boot.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk8/boot.html
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk8/boot.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk7/boot.html
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk7/boot.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk7/boot.html
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk6/boot.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk6/boot.html
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk6/boot.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk5/boot.html
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk5/boot.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk5/boot.html
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk5/boot.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk7/boot.html
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk1/boot.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk1/boot.html
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk1/boot.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk2/boot.html
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk2/boot.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk2/boot.html
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk3/boot.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk3/boot.html
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk3/boot.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk5/boot.html
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk5/boot.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk5/boot.html
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk6/boot.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk6/boot.html
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk6/boot.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk6/boot.html
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk7/boot.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk9/boot.html
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk9/boot.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk9/boot.html
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk8/boot.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk8/boot.html
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk7/boot.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk8/boot.html
### IGT changes ###
#### Issues hit ####
* igt@gem_create@create-massive:
- shard-skl: NOTRUN -> [DMESG-WARN][51] ([i915#4991]) +1 similar issue
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl7/igt@gem_create@create-massive.html
* igt@gem_eio@reset-stress:
- shard-snb: [PASS][52] -> [TIMEOUT][53] ([i915#3063])
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-snb5/igt@gem_eio@reset-stress.html
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-snb2/igt@gem_eio@reset-stress.html
* igt@gem_exec_balancer@parallel-keep-in-fence:
- shard-iclb: [PASS][54] -> [SKIP][55] ([i915#4525])
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb2/igt@gem_exec_balancer@parallel-keep-in-fence.html
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb3/igt@gem_exec_balancer@parallel-keep-in-fence.html
* igt@gem_exec_fair@basic-none-share@rcs0:
- shard-tglb: NOTRUN -> [FAIL][56] ([i915#2842])
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@gem_exec_fair@basic-none-share@rcs0.html
* igt@gem_exec_fair@basic-none-solo@rcs0:
- shard-apl: [PASS][57] -> [FAIL][58] ([i915#2842]) +1 similar issue
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-apl8/igt@gem_exec_fair@basic-none-solo@rcs0.html
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl1/igt@gem_exec_fair@basic-none-solo@rcs0.html
* igt@gem_exec_fair@basic-pace-share@rcs0:
- shard-glk: [PASS][59] -> [FAIL][60] ([i915#2842])
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk3/igt@gem_exec_fair@basic-pace-share@rcs0.html
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk5/igt@gem_exec_fair@basic-pace-share@rcs0.html
* igt@gem_exec_fair@basic-pace@vecs0:
- shard-skl: NOTRUN -> [SKIP][61] ([fdo#109271]) +315 similar issues
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl7/igt@gem_exec_fair@basic-pace@vecs0.html
* igt@gem_exec_params@no-blt:
- shard-tglb: NOTRUN -> [SKIP][62] ([fdo#109283])
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@gem_exec_params@no-blt.html
* igt@gem_lmem_swapping@basic:
- shard-skl: NOTRUN -> [SKIP][63] ([fdo#109271] / [i915#4613]) +5 similar issues
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl6/igt@gem_lmem_swapping@basic.html
* igt@gem_lmem_swapping@parallel-random:
- shard-tglb: NOTRUN -> [SKIP][64] ([i915#4613])
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@gem_lmem_swapping@parallel-random.html
* igt@gem_lmem_swapping@random:
- shard-glk: NOTRUN -> [SKIP][65] ([fdo#109271] / [i915#4613])
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk9/igt@gem_lmem_swapping@random.html
* igt@gem_mmap_gtt@coherency:
- shard-tglb: NOTRUN -> [SKIP][66] ([fdo#111656])
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@gem_mmap_gtt@coherency.html
* igt@gem_pread@exhaustion:
- shard-apl: NOTRUN -> [WARN][67] ([i915#2658])
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl1/igt@gem_pread@exhaustion.html
* igt@gem_pwrite@basic-exhaustion:
- shard-tglb: NOTRUN -> [WARN][68] ([i915#2658])
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@gem_pwrite@basic-exhaustion.html
* igt@gem_pxp@verify-pxp-stale-buf-optout-execution:
- shard-tglb: NOTRUN -> [SKIP][69] ([i915#4270])
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@gem_pxp@verify-pxp-stale-buf-optout-execution.html
* igt@gem_workarounds@suspend-resume-context:
- shard-snb: [PASS][70] -> [SKIP][71] ([fdo#109271])
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-snb5/igt@gem_workarounds@suspend-resume-context.html
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-snb5/igt@gem_workarounds@suspend-resume-context.html
* igt@gen7_exec_parse@cmd-crossing-page:
- shard-tglb: NOTRUN -> [SKIP][72] ([fdo#109289])
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@gen7_exec_parse@cmd-crossing-page.html
* igt@gen9_exec_parse@bb-start-param:
- shard-tglb: NOTRUN -> [SKIP][73] ([i915#2527] / [i915#2856])
[73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@gen9_exec_parse@bb-start-param.html
* igt@i915_pipe_stress@stress-xrgb8888-ytiled:
- shard-skl: NOTRUN -> [FAIL][74] ([i915#7036])
[74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl4/igt@i915_pipe_stress@stress-xrgb8888-ytiled.html
* igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a:
- shard-glk: NOTRUN -> [SKIP][75] ([fdo#109271] / [i915#1937])
[75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk9/igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a.html
* igt@i915_selftest@live@gt_pm:
- shard-skl: NOTRUN -> [DMESG-FAIL][76] ([i915#1886])
[76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl6/igt@i915_selftest@live@gt_pm.html
* igt@kms_big_fb@4-tiled-32bpp-rotate-0:
- shard-tglb: NOTRUN -> [SKIP][77] ([i915#5286]) +1 similar issue
[77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_big_fb@4-tiled-32bpp-rotate-0.html
* igt@kms_big_fb@linear-8bpp-rotate-90:
- shard-tglb: NOTRUN -> [SKIP][78] ([fdo#111614])
[78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_big_fb@linear-8bpp-rotate-90.html
* igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-0-async-flip:
- shard-skl: NOTRUN -> [FAIL][79] ([i915#3763])
[79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl9/igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-0-async-flip.html
* igt@kms_big_fb@yf-tiled-max-hw-stride-64bpp-rotate-180-hflip:
- shard-tglb: NOTRUN -> [SKIP][80] ([fdo#111615]) +1 similar issue
[80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_big_fb@yf-tiled-max-hw-stride-64bpp-rotate-180-hflip.html
* igt@kms_big_joiner@basic:
- shard-tglb: NOTRUN -> [SKIP][81] ([i915#2705])
[81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_big_joiner@basic.html
* igt@kms_ccs@pipe-a-bad-rotation-90-4_tiled_dg2_rc_ccs_cc:
- shard-glk: NOTRUN -> [SKIP][82] ([fdo#109271]) +39 similar issues
[82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk9/igt@kms_ccs@pipe-a-bad-rotation-90-4_tiled_dg2_rc_ccs_cc.html
* igt@kms_ccs@pipe-a-random-ccs-data-4_tiled_dg2_mc_ccs:
- shard-tglb: NOTRUN -> [SKIP][83] ([i915#6095]) +3 similar issues
[83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_ccs@pipe-a-random-ccs-data-4_tiled_dg2_mc_ccs.html
* igt@kms_ccs@pipe-a-random-ccs-data-y_tiled_gen12_rc_ccs_cc:
- shard-glk: NOTRUN -> [SKIP][84] ([fdo#109271] / [i915#3886])
[84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk9/igt@kms_ccs@pipe-a-random-ccs-data-y_tiled_gen12_rc_ccs_cc.html
* igt@kms_ccs@pipe-b-crc-primary-basic-y_tiled_gen12_mc_ccs:
- shard-tglb: NOTRUN -> [SKIP][85] ([i915#3689] / [i915#3886]) +1 similar issue
[85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_ccs@pipe-b-crc-primary-basic-y_tiled_gen12_mc_ccs.html
* igt@kms_ccs@pipe-c-bad-rotation-90-y_tiled_gen12_rc_ccs_cc:
- shard-apl: NOTRUN -> [SKIP][86] ([fdo#109271] / [i915#3886]) +2 similar issues
[86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl1/igt@kms_ccs@pipe-c-bad-rotation-90-y_tiled_gen12_rc_ccs_cc.html
* igt@kms_ccs@pipe-c-crc-primary-basic-yf_tiled_ccs:
- shard-tglb: NOTRUN -> [SKIP][87] ([fdo#111615] / [i915#3689])
[87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_ccs@pipe-c-crc-primary-basic-yf_tiled_ccs.html
* igt@kms_ccs@pipe-c-random-ccs-data-y_tiled_gen12_mc_ccs:
- shard-skl: NOTRUN -> [SKIP][88] ([fdo#109271] / [i915#3886]) +9 similar issues
[88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl9/igt@kms_ccs@pipe-c-random-ccs-data-y_tiled_gen12_mc_ccs.html
* igt@kms_chamelium@dp-mode-timings:
- shard-apl: NOTRUN -> [SKIP][89] ([fdo#109271] / [fdo#111827]) +1 similar issue
[89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl1/igt@kms_chamelium@dp-mode-timings.html
* igt@kms_chamelium@hdmi-crc-multiple:
- shard-glk: NOTRUN -> [SKIP][90] ([fdo#109271] / [fdo#111827]) +1 similar issue
[90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk9/igt@kms_chamelium@hdmi-crc-multiple.html
* igt@kms_chamelium@hdmi-edid-read:
- shard-tglb: NOTRUN -> [SKIP][91] ([fdo#109284] / [fdo#111827]) +2 similar issues
[91]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_chamelium@hdmi-edid-read.html
* igt@kms_chamelium@hdmi-hpd:
- shard-skl: NOTRUN -> [SKIP][92] ([fdo#109271] / [fdo#111827]) +12 similar issues
[92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl6/igt@kms_chamelium@hdmi-hpd.html
* igt@kms_content_protection@atomic:
- shard-tglb: NOTRUN -> [SKIP][93] ([i915#7118])
[93]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_content_protection@atomic.html
* igt@kms_cursor_crc@cursor-random-512x170:
- shard-tglb: NOTRUN -> [SKIP][94] ([i915#3359])
[94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_cursor_crc@cursor-random-512x170.html
* igt@kms_cursor_crc@cursor-suspend@pipe-a-dp-1:
- shard-apl: [PASS][95] -> [DMESG-WARN][96] ([i915#180])
[95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-apl6/igt@kms_cursor_crc@cursor-suspend@pipe-a-dp-1.html
[96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl3/igt@kms_cursor_crc@cursor-suspend@pipe-a-dp-1.html
* igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size:
- shard-glk: [PASS][97] -> [FAIL][98] ([i915#2346])
[97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk7/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size.html
[98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk7/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions-varying-size.html
* igt@kms_cursor_legacy@flip-vs-cursor@legacy:
- shard-skl: NOTRUN -> [FAIL][99] ([i915#2346]) +1 similar issue
[99]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl10/igt@kms_cursor_legacy@flip-vs-cursor@legacy.html
* igt@kms_fbcon_fbt@fbc:
- shard-glk: NOTRUN -> [FAIL][100] ([i915#4767])
[100]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk9/igt@kms_fbcon_fbt@fbc.html
* igt@kms_flip@2x-modeset-vs-vblank-race-interruptible:
- shard-tglb: NOTRUN -> [SKIP][101] ([fdo#109274] / [fdo#111825] / [i915#3637])
[101]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_flip@2x-modeset-vs-vblank-race-interruptible.html
* igt@kms_flip@flip-vs-suspend@a-edp1:
- shard-skl: NOTRUN -> [INCOMPLETE][102] ([i915#4839])
[102]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl4/igt@kms_flip@flip-vs-suspend@a-edp1.html
* igt@kms_flip@plain-flip-ts-check@c-edp1:
- shard-skl: [PASS][103] -> [FAIL][104] ([i915#2122]) +2 similar issues
[103]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-skl6/igt@kms_flip@plain-flip-ts-check@c-edp1.html
[104]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl6/igt@kms_flip@plain-flip-ts-check@c-edp1.html
* igt@kms_flip_scaled_crc@flip-32bpp-4tile-to-32bpp-4tiledg2rcccs-downscaling@pipe-a-valid-mode:
- shard-iclb: NOTRUN -> [SKIP][105] ([i915#2587] / [i915#2672]) +2 similar issues
[105]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb8/igt@kms_flip_scaled_crc@flip-32bpp-4tile-to-32bpp-4tiledg2rcccs-downscaling@pipe-a-valid-mode.html
* igt@kms_flip_scaled_crc@flip-32bpp-yftile-to-64bpp-yftile-upscaling@pipe-a-default-mode:
- shard-iclb: NOTRUN -> [SKIP][106] ([i915#2672])
[106]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb2/igt@kms_flip_scaled_crc@flip-32bpp-yftile-to-64bpp-yftile-upscaling@pipe-a-default-mode.html
* igt@kms_flip_scaled_crc@flip-32bpp-yftile-to-64bpp-yftile-upscaling@pipe-a-valid-mode:
- shard-tglb: NOTRUN -> [SKIP][107] ([i915#2587] / [i915#2672])
[107]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_flip_scaled_crc@flip-32bpp-yftile-to-64bpp-yftile-upscaling@pipe-a-valid-mode.html
* igt@kms_flip_scaled_crc@flip-64bpp-4tile-to-16bpp-4tile-upscaling@pipe-a-valid-mode:
- shard-apl: NOTRUN -> [SKIP][108] ([fdo#109271]) +21 similar issues
[108]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl1/igt@kms_flip_scaled_crc@flip-64bpp-4tile-to-16bpp-4tile-upscaling@pipe-a-valid-mode.html
* igt@kms_flip_scaled_crc@flip-64bpp-xtile-to-16bpp-xtile-downscaling@pipe-a-default-mode:
- shard-iclb: NOTRUN -> [SKIP][109] ([i915#3555]) +3 similar issues
[109]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb2/igt@kms_flip_scaled_crc@flip-64bpp-xtile-to-16bpp-xtile-downscaling@pipe-a-default-mode.html
* igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilegen12rcccs-upscaling@pipe-a-valid-mode:
- shard-iclb: NOTRUN -> [SKIP][110] ([i915#2672] / [i915#3555])
[110]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb8/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilegen12rcccs-upscaling@pipe-a-valid-mode.html
* igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-shrfb-draw-blt:
- shard-tglb: NOTRUN -> [SKIP][111] ([fdo#109280] / [fdo#111825]) +8 similar issues
[111]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_frontbuffer_tracking@fbc-2p-primscrn-pri-shrfb-draw-blt.html
* igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw:
- shard-tglb: NOTRUN -> [SKIP][112] ([i915#6497]) +1 similar issue
[112]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
* igt@kms_invalid_mode@clock-too-high@edp-1-pipe-d:
- shard-tglb: NOTRUN -> [SKIP][113] ([i915#6403]) +3 similar issues
[113]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_invalid_mode@clock-too-high@edp-1-pipe-d.html
* igt@kms_plane_alpha_blend@constant-alpha-min@pipe-c-edp-1:
- shard-skl: NOTRUN -> [FAIL][114] ([i915#4573]) +5 similar issues
[114]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl6/igt@kms_plane_alpha_blend@constant-alpha-min@pipe-c-edp-1.html
* igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-pixel-formats@pipe-b-edp-1:
- shard-iclb: [PASS][115] -> [SKIP][116] ([i915#5176]) +1 similar issue
[115]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb2/igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-pixel-formats@pipe-b-edp-1.html
[116]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb3/igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-pixel-formats@pipe-b-edp-1.html
* igt@kms_plane_scaling@planes-unity-scaling-downscale-factor-0-5@pipe-c-edp-1:
- shard-iclb: [PASS][117] -> [SKIP][118] ([i915#5235]) +2 similar issues
[117]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb8/igt@kms_plane_scaling@planes-unity-scaling-downscale-factor-0-5@pipe-c-edp-1.html
[118]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb2/igt@kms_plane_scaling@planes-unity-scaling-downscale-factor-0-5@pipe-c-edp-1.html
* igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-25@pipe-c-edp-1:
- shard-tglb: NOTRUN -> [SKIP][119] ([i915#5235]) +3 similar issues
[119]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-25@pipe-c-edp-1.html
* igt@kms_psr2_sf@cursor-plane-update-sf:
- shard-tglb: NOTRUN -> [SKIP][120] ([i915#2920])
[120]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_psr2_sf@cursor-plane-update-sf.html
* igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-fully-sf:
- shard-glk: NOTRUN -> [SKIP][121] ([fdo#109271] / [i915#658])
[121]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk9/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-fully-sf.html
* igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area:
- shard-skl: NOTRUN -> [SKIP][122] ([fdo#109271] / [i915#658]) +4 similar issues
[122]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl4/igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area.html
* igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area:
- shard-apl: NOTRUN -> [SKIP][123] ([fdo#109271] / [i915#658])
[123]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl1/igt@kms_psr2_sf@overlay-primary-update-sf-dmg-area.html
* igt@kms_psr2_su@page_flip-nv12@pipe-b-edp-1:
- shard-iclb: NOTRUN -> [FAIL][124] ([i915#5939]) +2 similar issues
[124]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb2/igt@kms_psr2_su@page_flip-nv12@pipe-b-edp-1.html
* igt@kms_psr@psr2_no_drrs:
- shard-iclb: [PASS][125] -> [SKIP][126] ([fdo#109441]) +1 similar issue
[125]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb2/igt@kms_psr@psr2_no_drrs.html
[126]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb3/igt@kms_psr@psr2_no_drrs.html
* igt@kms_rotation_crc@multiplane-rotation-cropping-top:
- shard-glk: [PASS][127] -> [DMESG-FAIL][128] ([i915#118])
[127]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk6/igt@kms_rotation_crc@multiplane-rotation-cropping-top.html
[128]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk3/igt@kms_rotation_crc@multiplane-rotation-cropping-top.html
* igt@kms_writeback@writeback-fb-id:
- shard-tglb: NOTRUN -> [SKIP][129] ([i915#2437])
[129]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@kms_writeback@writeback-fb-id.html
* igt@kms_writeback@writeback-invalid-parameters:
- shard-skl: NOTRUN -> [SKIP][130] ([fdo#109271] / [i915#2437])
[130]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl5/igt@kms_writeback@writeback-invalid-parameters.html
* igt@kms_writeback@writeback-pixel-formats:
- shard-glk: NOTRUN -> [SKIP][131] ([fdo#109271] / [i915#2437])
[131]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk9/igt@kms_writeback@writeback-pixel-formats.html
* igt@runner@aborted:
- shard-skl: NOTRUN -> ([FAIL][132], [FAIL][133]) ([i915#3002] / [i915#4312])
[132]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl7/igt@runner@aborted.html
[133]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl9/igt@runner@aborted.html
* igt@sysfs_clients@fair-7:
- shard-tglb: NOTRUN -> [SKIP][134] ([i915#2994])
[134]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb7/igt@sysfs_clients@fair-7.html
* igt@sysfs_clients@split-25:
- shard-skl: NOTRUN -> [SKIP][135] ([fdo#109271] / [i915#2994]) +3 similar issues
[135]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl5/igt@sysfs_clients@split-25.html
#### Possible fixes ####
* igt@gem_exec_fair@basic-deadline:
- shard-tglb: [FAIL][136] ([i915#2846]) -> [PASS][137]
[136]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-tglb6/igt@gem_exec_fair@basic-deadline.html
[137]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb2/igt@gem_exec_fair@basic-deadline.html
* igt@gem_exec_fair@basic-pace@vcs0:
- shard-iclb: [FAIL][138] ([i915#2842]) -> [PASS][139]
[138]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb3/igt@gem_exec_fair@basic-pace@vcs0.html
[139]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb8/igt@gem_exec_fair@basic-pace@vcs0.html
* igt@gem_huc_copy@huc-copy:
- shard-tglb: [SKIP][140] ([i915#2190]) -> [PASS][141]
[140]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-tglb6/igt@gem_huc_copy@huc-copy.html
[141]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb5/igt@gem_huc_copy@huc-copy.html
* igt@i915_pm_rps@engine-order:
- shard-apl: [FAIL][142] ([i915#6537]) -> [PASS][143]
[142]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-apl3/igt@i915_pm_rps@engine-order.html
[143]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl2/igt@i915_pm_rps@engine-order.html
* igt@i915_pm_sseu@full-enable:
- shard-skl: [FAIL][144] ([i915#6991]) -> [PASS][145]
[144]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-skl5/igt@i915_pm_sseu@full-enable.html
[145]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl2/igt@i915_pm_sseu@full-enable.html
* igt@kms_async_flips@alternate-sync-async-flip@pipe-b-hdmi-a-1:
- shard-glk: [FAIL][146] ([i915#2521]) -> [PASS][147]
[146]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk2/igt@kms_async_flips@alternate-sync-async-flip@pipe-b-hdmi-a-1.html
[147]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk8/igt@kms_async_flips@alternate-sync-async-flip@pipe-b-hdmi-a-1.html
* igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions:
- shard-glk: [FAIL][148] ([i915#2346]) -> [PASS][149]
[148]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk7/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions.html
[149]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk7/igt@kms_cursor_legacy@flip-vs-cursor@atomic-transitions.html
* igt@kms_flip@flip-vs-suspend@b-dp1:
- shard-apl: [DMESG-WARN][150] ([i915#180]) -> [PASS][151] +1 similar issue
[150]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-apl8/igt@kms_flip@flip-vs-suspend@b-dp1.html
[151]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl1/igt@kms_flip@flip-vs-suspend@b-dp1.html
* igt@kms_flip@plain-flip-fb-recreate@a-edp1:
- shard-skl: [FAIL][152] ([i915#2122]) -> [PASS][153]
[152]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-skl9/igt@kms_flip@plain-flip-fb-recreate@a-edp1.html
[153]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl9/igt@kms_flip@plain-flip-fb-recreate@a-edp1.html
* igt@kms_flip_scaled_crc@flip-64bpp-xtile-to-32bpp-xtile-downscaling@pipe-a-default-mode:
- shard-iclb: [SKIP][154] ([i915#3555]) -> [PASS][155]
[154]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb2/igt@kms_flip_scaled_crc@flip-64bpp-xtile-to-32bpp-xtile-downscaling@pipe-a-default-mode.html
[155]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb3/igt@kms_flip_scaled_crc@flip-64bpp-xtile-to-32bpp-xtile-downscaling@pipe-a-default-mode.html
* igt@kms_plane_scaling@planes-downscale-factor-0-5@pipe-a-edp-1:
- shard-iclb: [SKIP][156] ([i915#5235]) -> [PASS][157] +2 similar issues
[156]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb2/igt@kms_plane_scaling@planes-downscale-factor-0-5@pipe-a-edp-1.html
[157]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb3/igt@kms_plane_scaling@planes-downscale-factor-0-5@pipe-a-edp-1.html
* igt@kms_psr@psr2_primary_blt:
- shard-iclb: [SKIP][158] ([fdo#109441]) -> [PASS][159]
[158]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb1/igt@kms_psr@psr2_primary_blt.html
[159]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb2/igt@kms_psr@psr2_primary_blt.html
* igt@kms_psr_stress_test@flip-primary-invalidate-overlay:
- shard-tglb: [SKIP][160] ([i915#5519]) -> [PASS][161]
[160]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-tglb7/igt@kms_psr_stress_test@flip-primary-invalidate-overlay.html
[161]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-tglb8/igt@kms_psr_stress_test@flip-primary-invalidate-overlay.html
* igt@kms_psr_stress_test@invalidate-primary-flip-overlay:
- shard-iclb: [SKIP][162] ([i915#5519]) -> [PASS][163]
[162]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb5/igt@kms_psr_stress_test@invalidate-primary-flip-overlay.html
[163]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb5/igt@kms_psr_stress_test@invalidate-primary-flip-overlay.html
#### Warnings ####
* igt@gem_exec_balancer@parallel-ordering:
- shard-iclb: [SKIP][164] ([i915#4525]) -> [FAIL][165] ([i915#6117])
[164]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb6/igt@gem_exec_balancer@parallel-ordering.html
[165]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb1/igt@gem_exec_balancer@parallel-ordering.html
* igt@gem_pread@exhaustion:
- shard-glk: [INCOMPLETE][166] ([i915#7248]) -> [WARN][167] ([i915#2658])
[166]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-glk1/igt@gem_pread@exhaustion.html
[167]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-glk2/igt@gem_pread@exhaustion.html
* igt@i915_pm_dc@dc3co-vpb-simulation:
- shard-iclb: [SKIP][168] ([i915#658]) -> [SKIP][169] ([i915#588])
[168]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb8/igt@i915_pm_dc@dc3co-vpb-simulation.html
[169]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb2/igt@i915_pm_dc@dc3co-vpb-simulation.html
* igt@kms_ccs@pipe-b-crc-primary-basic-y_tiled_gen12_rc_ccs:
- shard-skl: [SKIP][170] ([fdo#109271]) -> [INCOMPLETE][171] ([i915#2295])
[170]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-skl10/igt@kms_ccs@pipe-b-crc-primary-basic-y_tiled_gen12_rc_ccs.html
[171]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-skl7/igt@kms_ccs@pipe-b-crc-primary-basic-y_tiled_gen12_rc_ccs.html
* igt@kms_plane_alpha_blend@alpha-basic@pipe-c-dp-1:
- shard-apl: [DMESG-FAIL][172] ([IGT#6]) -> [FAIL][173] ([i915#4573]) +1 similar issue
[172]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-apl2/igt@kms_plane_alpha_blend@alpha-basic@pipe-c-dp-1.html
[173]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl6/igt@kms_plane_alpha_blend@alpha-basic@pipe-c-dp-1.html
* igt@kms_psr2_sf@cursor-plane-move-continuous-sf:
- shard-iclb: [SKIP][174] ([i915#658]) -> [SKIP][175] ([i915#2920])
[174]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb1/igt@kms_psr2_sf@cursor-plane-move-continuous-sf.html
[175]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb2/igt@kms_psr2_sf@cursor-plane-move-continuous-sf.html
* igt@kms_psr2_sf@cursor-plane-update-sf:
- shard-iclb: [SKIP][176] ([fdo#111068] / [i915#658]) -> [SKIP][177] ([i915#2920])
[176]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb1/igt@kms_psr2_sf@cursor-plane-update-sf.html
[177]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb2/igt@kms_psr2_sf@cursor-plane-update-sf.html
* igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-fully-sf:
- shard-iclb: [SKIP][178] ([i915#2920]) -> [SKIP][179] ([i915#658]) +1 similar issue
[178]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb2/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-fully-sf.html
[179]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb3/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-fully-sf.html
* igt@kms_psr2_sf@overlay-plane-update-continuous-sf:
- shard-iclb: [SKIP][180] ([i915#2920]) -> [SKIP][181] ([fdo#111068] / [i915#658])
[180]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-iclb2/igt@kms_psr2_sf@overlay-plane-update-continuous-sf.html
[181]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-iclb3/igt@kms_psr2_sf@overlay-plane-update-continuous-sf.html
* igt@runner@aborted:
- shard-apl: ([FAIL][182], [FAIL][183], [FAIL][184]) ([i915#180] / [i915#3002] / [i915#4312]) -> ([FAIL][185], [FAIL][186], [FAIL][187]) ([i915#3002] / [i915#4312])
[182]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-apl7/igt@runner@aborted.html
[183]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-apl7/igt@runner@aborted.html
[184]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12407/shard-apl8/igt@runner@aborted.html
[185]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl2/igt@runner@aborted.html
[186]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl3/igt@runner@aborted.html
[187]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/shard-apl1/igt@runner@aborted.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[IGT#6]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/6
[fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
[fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
[fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
[fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
[fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
[fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
[fdo#109303]: https://bugs.freedesktop.org/show_bug.cgi?id=109303
[fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
[fdo#109308]: https://bugs.freedesktop.org/show_bug.cgi?id=109308
[fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
[fdo#109312]: https://bugs.freedesktop.org/show_bug.cgi?id=109312
[fdo#109313]: https://bugs.freedesktop.org/show_bug.cgi?id=109313
[fdo#109314]: https://bugs.freedesktop.org/show_bug.cgi?id=109314
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506
[fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
[fdo#110723]: https://bugs.freedesktop.org/show_bug.cgi?id=110723
[fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
[fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
[fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
[fdo#111656]: https://bugs.freedesktop.org/show_bug.cgi?id=111656
[fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
[fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
[fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283
[i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
[i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
[i915#1257]: https://gitlab.freedesktop.org/drm/intel/issues/1257
[i915#132]: https://gitlab.freedesktop.org/drm/intel/issues/132
[i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
[i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
[i915#1825]: https://gitlab.freedesktop.org/drm/intel/issues/1825
[i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
[i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
[i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
[i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
[i915#1902]: https://gitlab.freedesktop.org/drm/intel/issues/1902
[i915#1937]: https://gitlab.freedesktop.org/drm/intel/issues/1937
[i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
[i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
[i915#2295]: https://gitlab.freedesktop.org/drm/intel/issues/2295
[i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
[i915#2410]: https://gitlab.freedesktop.org/drm/intel/issues/2410
[i915#2433]: https://gitlab.freedesktop.org/drm/intel/issues/2433
[i915#2434]: https://gitlab.freedesktop.org/drm/intel/issues/2434
[i915#2435]: https://gitlab.freedesktop.org/drm/intel/issues/2435
[i915#2436]: https://gitlab.freedesktop.org/drm/intel/issues/2436
[i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
[i915#2521]: https://gitlab.freedesktop.org/drm/intel/issues/2521
[i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
[i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
[i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
[i915#2658]: https://gitlab.freedesktop.org/drm/intel/issues/2658
[i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
[i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681
[i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
[i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280
[i915#284]: https://gitlab.freedesktop.org/drm/intel/issues/284
[i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
[i915#2846]: https://gitlab.freedesktop.org/drm/intel/issues/2846
[i915#2856]: https://gitlab.freedesktop.org/drm/intel/issues/2856
[i915#2920]: https://gitlab.freedesktop.org/drm/intel/issues/2920
[i915#2994]: https://gitlab.freedesktop.org/drm/intel/issues/2994
[i915#3002]: https://gitlab.freedesktop.org/drm/intel/issues/3002
[i915#3063]: https://gitlab.freedesktop.org/drm/intel/issues/3063
[i915#3116]: https://gitlab.freedesktop.org/drm/intel/issues/3116
[i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
[i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
[i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
[i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
[i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
[i915#3323]: https://gitlab.freedesktop.org/drm/intel/issues/3323
[i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
[i915#3361]: https://gitlab.freedesktop.org/drm/intel/issues/3361
[i915#3528]: https://gitlab.freedesktop.org/drm/intel/issues/3528
[i915#3536]: https://gitlab.freedesktop.org/drm/intel/issues/3536
[i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3558]: https://gitlab.freedesktop.org/drm/intel/issues/3558
[i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
[i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
[i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#3734]: https://gitlab.freedesktop.org/drm/intel/issues/3734
[i915#3742]: https://gitlab.freedesktop.org/drm/intel/issues/3742
[i915#3763]: https://gitlab.freedesktop.org/drm/intel/issues/3763
[i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
[i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
[i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
[i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
[i915#426]: https://gitlab.freedesktop.org/drm/intel/issues/426
[i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
[i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
[i915#4392]: https://gitlab.freedesktop.org/drm/intel/issues/4392
[i915#4525]: https://gitlab.freedesktop.org/drm/intel/issues/4525
[i915#4573]: https://gitlab.freedesktop.org/drm/intel/issues/4573
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#4767]: https://gitlab.freedesktop.org/drm/intel/issues/4767
[i915#4839]: https://gitlab.freedesktop.org/drm/intel/issues/4839
[i915#4991]: https://gitlab.freedesktop.org/drm/intel/issues/4991
[i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
[i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
[i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
[i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
[i915#5327]: https://gitlab.freedesktop.org/drm/intel/issues/5327
[i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
[i915#5461]: https://gitlab.freedesktop.org/drm/intel/issues/5461
[i915#5519]: https://gitlab.freedesktop.org/drm/intel/issues/5519
[i915#5723]: https://gitlab.freedesktop.org/drm/intel/issues/5723
[i915#588]: https://gitlab.freedesktop.org/drm/intel/issues/588
[i915#5939]: https://gitlab.freedesktop.org/drm/intel/issues/5939
[i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
[i915#6117]: https://gitlab.freedesktop.org/drm/intel/issues/6117
[i915#6227]: https://gitlab.freedesktop.org/drm/intel/issues/6227
[i915#6230]: https://gitlab.freedesktop.org/drm/intel/issues/6230
[i915#6248]: https://gitlab.freedesktop.org/drm/intel/issues/6248
[i915#6259]: https://gitlab.freedesktop.org/drm/intel/issues/6259
[i915#6334]: https://gitlab.freedesktop.org/drm/intel/issues/6334
[i915#6335]: https://gitlab.freedesktop.org/drm/intel/issues/6335
[i915#6344]: https://gitlab.freedesktop.org/drm/intel/issues/6344
[i915#6403]: https://gitlab.freedesktop.org/drm/intel/issues/6403
[i915#6412]: https://gitlab.freedesktop.org/drm/intel/issues/6412
[i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433
[i915#6497]: https://gitlab.freedesktop.org/drm/intel/issues/6497
[i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
[i915#6537]: https://gitlab.freedesktop.org/drm/intel/issues/6537
[i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
[i915#6590]: https://gitlab.freedesktop.org/drm/intel/issues/6590
[i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768
[i915#6991]: https://gitlab.freedesktop.org/drm/intel/issues/6991
[i915#7036]: https://gitlab.freedesktop.org/drm/intel/issues/7036
[i915#7052]: https://gitlab.freedesktop.org/drm/intel/issues/7052
[i915#7118]: https://gitlab.freedesktop.org/drm/intel/issues/7118
[i915#7248]: https://gitlab.freedesktop.org/drm/intel/issues/7248
[i915#7276]: https://gitlab.freedesktop.org/drm/intel/issues/7276
[i915#7397]: https://gitlab.freedesktop.org/drm/intel/issues/7397
[i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
[i915#7468]: https://gitlab.freedesktop.org/drm/intel/issues/7468
[i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
Build changes
-------------
* Linux: CI_DRM_12407 -> Patchwork_110964v3
CI-20190529: 20190529
CI_DRM_12407: acd6b3e8e35f7b7b5ce9d16d85f5cdc1e5a94bdf @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7069: 40a2de5cc6a6b43af7da7905bfe1ede9d9a3200c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_110964v3: acd6b3e8e35f7b7b5ce9d16d85f5cdc1e5a94bdf @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v3/index.html
[-- Attachment #2: Type: text/html, Size: 49453 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Fix negative value passed as remaining time
2022-11-21 17:40 ` Andrzej Hajda
@ 2022-11-21 23:19 ` Janusz Krzysztofik
-1 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 23:19 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen, Andrzej Hajda
Cc: intel-gfx, dri-devel, Chris Wilson, Janusz Krzysztofik, Nirmoy Das
Hi Andrzej,
Thanks for providing your R-b.
On Monday, 21 November 2022 18:40:51 CET Andrzej Hajda wrote:
> On 21.11.2022 15:56, Janusz Krzysztofik wrote:
> > Commit b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
> > with GuC") extended the API of intel_gt_retire_requests_timeout() with an
> > extra argument 'remaining_timeout', intended for passing back unconsumed
> > portion of requested timeout when 0 (success) is returned. However, when
> > request retirement happens to succeed despite an error returned by a call
> > to dma_fence_wait_timeout(), that error code (a negative value) is passed
> > back instead of remaining time. If we then pass that negative value
> > forward as requested timeout to intel_uc_wait_for_idle(), an explicit BUG
> > will be triggered.
> >
> > If request retirement succeeds but an error code is passed back via
> > remaininig_timeout, we may have no clue on how much of the initial timeout
> > might have been left for spending it on waiting for GuC to become idle.
> > OTOH, since all pending requests have been successfully retired, that
> > error code has been already ignored by intel_gt_retire_requests_timeout(),
> > then we shouldn't fail.
> >
> > Assume no more time has been left on error and pass 0 timeout value to
> > intel_uc_wait_for_idle() to give it a chance to return success if GuC is
> > already idle.
> >
> > v3: Don't fail on any error passed back via remaining_timeout.
> >
> > v2: Fix the issue on the caller side, not the provider.
> >
> > Fixes: b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
with GuC")
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: stable@vger.kernel.org # v5.15+
>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
While still open for comments from others, I'm now looking for potential
committer.
Thanks,
Janusz
>
> Regards
> Andrzej
>
> > ---
> > drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/
intel_gt.c
> > index b5ad9caa55372..7ef0edb2e37cd 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -677,8 +677,13 @@ int intel_gt_wait_for_idle(struct intel_gt *gt, long
timeout)
> > return -EINTR;
> > }
> >
> > - return timeout ? timeout : intel_uc_wait_for_idle(>->uc,
> > -
remaining_timeout);
> > + if (timeout)
> > + return timeout;
> > +
> > + if (remaining_timeout < 0)
> > + remaining_timeout = 0;
> > +
> > + return intel_uc_wait_for_idle(>->uc, remaining_timeout);
> > }
> >
> > int intel_gt_init(struct intel_gt *gt)
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Fix negative value passed as remaining time
@ 2022-11-21 23:19 ` Janusz Krzysztofik
0 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 23:19 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen, Andrzej Hajda
Cc: intel-gfx, dri-devel, Chris Wilson, Daniel Vetter, Nirmoy Das
Hi Andrzej,
Thanks for providing your R-b.
On Monday, 21 November 2022 18:40:51 CET Andrzej Hajda wrote:
> On 21.11.2022 15:56, Janusz Krzysztofik wrote:
> > Commit b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
> > with GuC") extended the API of intel_gt_retire_requests_timeout() with an
> > extra argument 'remaining_timeout', intended for passing back unconsumed
> > portion of requested timeout when 0 (success) is returned. However, when
> > request retirement happens to succeed despite an error returned by a call
> > to dma_fence_wait_timeout(), that error code (a negative value) is passed
> > back instead of remaining time. If we then pass that negative value
> > forward as requested timeout to intel_uc_wait_for_idle(), an explicit BUG
> > will be triggered.
> >
> > If request retirement succeeds but an error code is passed back via
> > remaininig_timeout, we may have no clue on how much of the initial timeout
> > might have been left for spending it on waiting for GuC to become idle.
> > OTOH, since all pending requests have been successfully retired, that
> > error code has been already ignored by intel_gt_retire_requests_timeout(),
> > then we shouldn't fail.
> >
> > Assume no more time has been left on error and pass 0 timeout value to
> > intel_uc_wait_for_idle() to give it a chance to return success if GuC is
> > already idle.
> >
> > v3: Don't fail on any error passed back via remaining_timeout.
> >
> > v2: Fix the issue on the caller side, not the provider.
> >
> > Fixes: b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
with GuC")
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: stable@vger.kernel.org # v5.15+
>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
While still open for comments from others, I'm now looking for potential
committer.
Thanks,
Janusz
>
> Regards
> Andrzej
>
> > ---
> > drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/
intel_gt.c
> > index b5ad9caa55372..7ef0edb2e37cd 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -677,8 +677,13 @@ int intel_gt_wait_for_idle(struct intel_gt *gt, long
timeout)
> > return -EINTR;
> > }
> >
> > - return timeout ? timeout : intel_uc_wait_for_idle(>->uc,
> > -
remaining_timeout);
> > + if (timeout)
> > + return timeout;
> > +
> > + if (remaining_timeout < 0)
> > + remaining_timeout = 0;
> > +
> > + return intel_uc_wait_for_idle(>->uc, remaining_timeout);
> > }
> >
> > int intel_gt_init(struct intel_gt *gt)
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Fix negative value passed as remaining time
2022-11-21 23:19 ` Janusz Krzysztofik
@ 2022-11-22 10:41 ` Tvrtko Ursulin
-1 siblings, 0 replies; 29+ messages in thread
From: Tvrtko Ursulin @ 2022-11-22 10:41 UTC (permalink / raw)
To: Janusz Krzysztofik, Joonas Lahtinen, Andrzej Hajda
Cc: dri-devel, intel-gfx, Chris Wilson, Nirmoy Das
On 21/11/2022 23:19, Janusz Krzysztofik wrote:
> Hi Andrzej,
>
> Thanks for providing your R-b.
>
> On Monday, 21 November 2022 18:40:51 CET Andrzej Hajda wrote:
>> On 21.11.2022 15:56, Janusz Krzysztofik wrote:
>>> Commit b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
>>> with GuC") extended the API of intel_gt_retire_requests_timeout() with an
>>> extra argument 'remaining_timeout', intended for passing back unconsumed
>>> portion of requested timeout when 0 (success) is returned. However, when
>>> request retirement happens to succeed despite an error returned by a call
>>> to dma_fence_wait_timeout(), that error code (a negative value) is passed
>>> back instead of remaining time. If we then pass that negative value
>>> forward as requested timeout to intel_uc_wait_for_idle(), an explicit BUG
>>> will be triggered.
Right, AFAICT a GEM_BUG_ON in debug builds, but in production builds
negative timeout will get passed along all the way to schedule_timeout
where error and call trace will be dumped. So fix appears warranted indeed.
>>> If request retirement succeeds but an error code is passed back via
>>> remaininig_timeout, we may have no clue on how much of the initial timeout
>>> might have been left for spending it on waiting for GuC to become idle.
>>> OTOH, since all pending requests have been successfully retired, that
>>> error code has been already ignored by intel_gt_retire_requests_timeout(),
>>> then we shouldn't fail.
>>>
>>> Assume no more time has been left on error and pass 0 timeout value to
>>> intel_uc_wait_for_idle() to give it a chance to return success if GuC is
>>> already idle.
>>>
>>> v3: Don't fail on any error passed back via remaining_timeout.
>>>
>>> v2: Fix the issue on the caller side, not the provider.
>>>
>>> Fixes: b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
> with GuC")
>>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>>> Cc: stable@vger.kernel.org # v5.15+
>>
>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>
> While still open for comments from others, I'm now looking for potential
> committer.
Both patches are considered good to go?
Regards,
Tvrtko
>
> Thanks,
> Janusz
>
>
>>
>> Regards
>> Andrzej
>>
>>> ---
>>> drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/
> intel_gt.c
>>> index b5ad9caa55372..7ef0edb2e37cd 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>>> @@ -677,8 +677,13 @@ int intel_gt_wait_for_idle(struct intel_gt *gt, long
> timeout)
>>> return -EINTR;
>>> }
>>>
>>> - return timeout ? timeout : intel_uc_wait_for_idle(>->uc,
>>> -
> remaining_timeout);
>>> + if (timeout)
>>> + return timeout;
>>> +
>>> + if (remaining_timeout < 0)
>>> + remaining_timeout = 0;
>>> +
>>> + return intel_uc_wait_for_idle(>->uc, remaining_timeout);
>>> }
>>>
>>> int intel_gt_init(struct intel_gt *gt)
>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Fix negative value passed as remaining time
@ 2022-11-22 10:41 ` Tvrtko Ursulin
0 siblings, 0 replies; 29+ messages in thread
From: Tvrtko Ursulin @ 2022-11-22 10:41 UTC (permalink / raw)
To: Janusz Krzysztofik, Joonas Lahtinen, Andrzej Hajda
Cc: dri-devel, intel-gfx, Chris Wilson, Daniel Vetter, Nirmoy Das
On 21/11/2022 23:19, Janusz Krzysztofik wrote:
> Hi Andrzej,
>
> Thanks for providing your R-b.
>
> On Monday, 21 November 2022 18:40:51 CET Andrzej Hajda wrote:
>> On 21.11.2022 15:56, Janusz Krzysztofik wrote:
>>> Commit b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
>>> with GuC") extended the API of intel_gt_retire_requests_timeout() with an
>>> extra argument 'remaining_timeout', intended for passing back unconsumed
>>> portion of requested timeout when 0 (success) is returned. However, when
>>> request retirement happens to succeed despite an error returned by a call
>>> to dma_fence_wait_timeout(), that error code (a negative value) is passed
>>> back instead of remaining time. If we then pass that negative value
>>> forward as requested timeout to intel_uc_wait_for_idle(), an explicit BUG
>>> will be triggered.
Right, AFAICT a GEM_BUG_ON in debug builds, but in production builds
negative timeout will get passed along all the way to schedule_timeout
where error and call trace will be dumped. So fix appears warranted indeed.
>>> If request retirement succeeds but an error code is passed back via
>>> remaininig_timeout, we may have no clue on how much of the initial timeout
>>> might have been left for spending it on waiting for GuC to become idle.
>>> OTOH, since all pending requests have been successfully retired, that
>>> error code has been already ignored by intel_gt_retire_requests_timeout(),
>>> then we shouldn't fail.
>>>
>>> Assume no more time has been left on error and pass 0 timeout value to
>>> intel_uc_wait_for_idle() to give it a chance to return success if GuC is
>>> already idle.
>>>
>>> v3: Don't fail on any error passed back via remaining_timeout.
>>>
>>> v2: Fix the issue on the caller side, not the provider.
>>>
>>> Fixes: b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
> with GuC")
>>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>>> Cc: stable@vger.kernel.org # v5.15+
>>
>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>
> While still open for comments from others, I'm now looking for potential
> committer.
Both patches are considered good to go?
Regards,
Tvrtko
>
> Thanks,
> Janusz
>
>
>>
>> Regards
>> Andrzej
>>
>>> ---
>>> drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/
> intel_gt.c
>>> index b5ad9caa55372..7ef0edb2e37cd 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>>> @@ -677,8 +677,13 @@ int intel_gt_wait_for_idle(struct intel_gt *gt, long
> timeout)
>>> return -EINTR;
>>> }
>>>
>>> - return timeout ? timeout : intel_uc_wait_for_idle(>->uc,
>>> -
> remaining_timeout);
>>> + if (timeout)
>>> + return timeout;
>>> +
>>> + if (remaining_timeout < 0)
>>> + remaining_timeout = 0;
>>> +
>>> + return intel_uc_wait_for_idle(>->uc, remaining_timeout);
>>> }
>>>
>>> int intel_gt_init(struct intel_gt *gt)
>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-21 14:56 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-22 10:50 ` Tvrtko Ursulin
-1 siblings, 0 replies; 29+ messages in thread
From: Tvrtko Ursulin @ 2022-11-22 10:50 UTC (permalink / raw)
To: Janusz Krzysztofik, Joonas Lahtinen
Cc: Matthew Brost, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
John Harrison, Nirmoy Das
On 21/11/2022 14:56, Janusz Krzysztofik wrote:
> Users of intel_gt_retire_requests_timeout() expect 0 return value on
> success. However, we have no protection from passing back 0 potentially
> returned by a call to dma_fence_wait_timeout() when it succedes right
> after its timeout has expired.
Is this talking about a potential weakness, or ambiguous kerneldoc, of
dma_fence_wait_timeout, dma_fence_default_wait and
i915_request_wait_timeout? They appear to say 0 return means timeout,
implying unsignaled fence. In other words signaled must return positive
remaining timeout. Implementations seems to allow a race which indeed
appears that return 0 and signaled fence is possible.
If dma_fence_wait can indeed return 0 even when a request is signaled,
then how is timeout ?: -ETIME below correct? It isn't a chance for false
negative in its' callers?
Regards,
Tvrtko
> Replace 0 with -ETIME before potentially using the timeout value as return
> code, so -ETIME is returned if there are still some requests not retired
> after timeout, 0 otherwise.
>
> v3: Use conditional expression, more compact but also better reflecting
> intention standing behind the change.
>
> v2: Move the added lines down so flush_submission() is not affected.
>
> Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: stable@vger.kernel.org # v5.5+
> ---
> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> index edb881d756309..1dfd01668c79c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> @@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
> if (remaining_timeout)
> *remaining_timeout = timeout;
>
> - return active_count ? timeout : 0;
> + return active_count ? timeout ?: -ETIME : 0;
> }
>
> static void retire_work_handler(struct work_struct *work)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-22 10:50 ` Tvrtko Ursulin
0 siblings, 0 replies; 29+ messages in thread
From: Tvrtko Ursulin @ 2022-11-22 10:50 UTC (permalink / raw)
To: Janusz Krzysztofik, Joonas Lahtinen
Cc: Daniel Vetter, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Nirmoy Das
On 21/11/2022 14:56, Janusz Krzysztofik wrote:
> Users of intel_gt_retire_requests_timeout() expect 0 return value on
> success. However, we have no protection from passing back 0 potentially
> returned by a call to dma_fence_wait_timeout() when it succedes right
> after its timeout has expired.
Is this talking about a potential weakness, or ambiguous kerneldoc, of
dma_fence_wait_timeout, dma_fence_default_wait and
i915_request_wait_timeout? They appear to say 0 return means timeout,
implying unsignaled fence. In other words signaled must return positive
remaining timeout. Implementations seems to allow a race which indeed
appears that return 0 and signaled fence is possible.
If dma_fence_wait can indeed return 0 even when a request is signaled,
then how is timeout ?: -ETIME below correct? It isn't a chance for false
negative in its' callers?
Regards,
Tvrtko
> Replace 0 with -ETIME before potentially using the timeout value as return
> code, so -ETIME is returned if there are still some requests not retired
> after timeout, 0 otherwise.
>
> v3: Use conditional expression, more compact but also better reflecting
> intention standing behind the change.
>
> v2: Move the added lines down so flush_submission() is not affected.
>
> Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: stable@vger.kernel.org # v5.5+
> ---
> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> index edb881d756309..1dfd01668c79c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> @@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
> if (remaining_timeout)
> *remaining_timeout = timeout;
>
> - return active_count ? timeout : 0;
> + return active_count ? timeout ?: -ETIME : 0;
> }
>
> static void retire_work_handler(struct work_struct *work)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-22 10:50 ` [Intel-gfx] " Tvrtko Ursulin
@ 2022-11-23 9:28 ` Janusz Krzysztofik
-1 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-23 9:28 UTC (permalink / raw)
To: Joonas Lahtinen, Tvrtko Ursulin
Cc: Matthew Brost, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Janusz Krzysztofik, John Harrison, Nirmoy Das
Hi Tvrtko,
Thanks for your comments.
On Tuesday, 22 November 2022 11:50:38 CET Tvrtko Ursulin wrote:
>
> On 21/11/2022 14:56, Janusz Krzysztofik wrote:
> > Users of intel_gt_retire_requests_timeout() expect 0 return value on
> > success. However, we have no protection from passing back 0 potentially
> > returned by a call to dma_fence_wait_timeout() when it succedes right
> > after its timeout has expired.
>
> Is this talking about a potential weakness, or ambiguous kerneldoc, of
> dma_fence_wait_timeout, dma_fence_default_wait and
> i915_request_wait_timeout? They appear to say 0 return means timeout,
> implying unsignaled fence. In other words signaled must return positive
> remaining timeout. Implementations seems to allow a race which indeed
> appears that return 0 and signaled fence is possible.
While my initial analysis was indeed focused on inconsistent semantics of 0
return values from different dma_fence_default_wait() backends, I should have
also mentioned in this commit description that users may perfectly
call intel_gt_retire_requests_timeout() with 0 timeout, in which case the
false positive 0 value can be returned regardless of dma_fence_wait_timeout()
potential issues. Would you like me to reword and resubmit?
> If dma_fence_wait can indeed return 0 even when a request is signaled,
> then how is timeout ?: -ETIME below correct? It isn't a chance for false
> negative in its' callers?
The goal of intel_gt_retire_requests_timeout() is to retire requests. When
that goal has been reached, i.e., all requests have been retired, active count
is 0, and 0 is correctly returned, regardless of timeout value.
The value of timeout is used only when there are still pending requests, which
means that the goal hasn't been reached and the function hasn't succeeded.
Then, no false negative is possible, unlike the false positive that we now
have when we return 0 while some requests are still pending.
Thanks,
Janusz
>
> Regards,
>
> Tvrtko
>
> > Replace 0 with -ETIME before potentially using the timeout value as return
> > code, so -ETIME is returned if there are still some requests not retired
> > after timeout, 0 otherwise.
> >
> > v3: Use conditional expression, more compact but also better reflecting
> > intention standing behind the change.
> >
> > v2: Move the added lines down so flush_submission() is not affected.
> >
> > Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > Cc: stable@vger.kernel.org # v5.5+
> > ---
> > drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > index edb881d756309..1dfd01668c79c 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > @@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
> > if (remaining_timeout)
> > *remaining_timeout = timeout;
> >
> > - return active_count ? timeout : 0;
> > + return active_count ? timeout ?: -ETIME : 0;
> > }
> >
> > static void retire_work_handler(struct work_struct *work)
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-23 9:28 ` Janusz Krzysztofik
0 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-23 9:28 UTC (permalink / raw)
To: Joonas Lahtinen, Tvrtko Ursulin
Cc: Daniel Vetter, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Nirmoy Das
Hi Tvrtko,
Thanks for your comments.
On Tuesday, 22 November 2022 11:50:38 CET Tvrtko Ursulin wrote:
>
> On 21/11/2022 14:56, Janusz Krzysztofik wrote:
> > Users of intel_gt_retire_requests_timeout() expect 0 return value on
> > success. However, we have no protection from passing back 0 potentially
> > returned by a call to dma_fence_wait_timeout() when it succedes right
> > after its timeout has expired.
>
> Is this talking about a potential weakness, or ambiguous kerneldoc, of
> dma_fence_wait_timeout, dma_fence_default_wait and
> i915_request_wait_timeout? They appear to say 0 return means timeout,
> implying unsignaled fence. In other words signaled must return positive
> remaining timeout. Implementations seems to allow a race which indeed
> appears that return 0 and signaled fence is possible.
While my initial analysis was indeed focused on inconsistent semantics of 0
return values from different dma_fence_default_wait() backends, I should have
also mentioned in this commit description that users may perfectly
call intel_gt_retire_requests_timeout() with 0 timeout, in which case the
false positive 0 value can be returned regardless of dma_fence_wait_timeout()
potential issues. Would you like me to reword and resubmit?
> If dma_fence_wait can indeed return 0 even when a request is signaled,
> then how is timeout ?: -ETIME below correct? It isn't a chance for false
> negative in its' callers?
The goal of intel_gt_retire_requests_timeout() is to retire requests. When
that goal has been reached, i.e., all requests have been retired, active count
is 0, and 0 is correctly returned, regardless of timeout value.
The value of timeout is used only when there are still pending requests, which
means that the goal hasn't been reached and the function hasn't succeeded.
Then, no false negative is possible, unlike the false positive that we now
have when we return 0 while some requests are still pending.
Thanks,
Janusz
>
> Regards,
>
> Tvrtko
>
> > Replace 0 with -ETIME before potentially using the timeout value as return
> > code, so -ETIME is returned if there are still some requests not retired
> > after timeout, 0 otherwise.
> >
> > v3: Use conditional expression, more compact but also better reflecting
> > intention standing behind the change.
> >
> > v2: Move the added lines down so flush_submission() is not affected.
> >
> > Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> > Cc: stable@vger.kernel.org # v5.5+
> > ---
> > drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > index edb881d756309..1dfd01668c79c 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > @@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
> > if (remaining_timeout)
> > *remaining_timeout = timeout;
> >
> > - return active_count ? timeout : 0;
> > + return active_count ? timeout ?: -ETIME : 0;
> > }
> >
> > static void retire_work_handler(struct work_struct *work)
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Fix negative value passed as remaining time
2022-11-22 10:41 ` Tvrtko Ursulin
@ 2022-11-23 11:29 ` Janusz Krzysztofik
-1 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-23 11:29 UTC (permalink / raw)
To: Joonas Lahtinen, Andrzej Hajda, Tvrtko Ursulin
Cc: intel-gfx, dri-devel, Chris Wilson, Daniel Vetter, Nirmoy Das
On Tuesday, 22 November 2022 11:41:29 CET Tvrtko Ursulin wrote:
>
> On 21/11/2022 23:19, Janusz Krzysztofik wrote:
> > Hi Andrzej,
> >
> > Thanks for providing your R-b.
> >
> > On Monday, 21 November 2022 18:40:51 CET Andrzej Hajda wrote:
> >> On 21.11.2022 15:56, Janusz Krzysztofik wrote:
> >>> Commit b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
> >>> with GuC") extended the API of intel_gt_retire_requests_timeout() with an
> >>> extra argument 'remaining_timeout', intended for passing back unconsumed
> >>> portion of requested timeout when 0 (success) is returned. However, when
> >>> request retirement happens to succeed despite an error returned by a call
> >>> to dma_fence_wait_timeout(), that error code (a negative value) is passed
> >>> back instead of remaining time. If we then pass that negative value
> >>> forward as requested timeout to intel_uc_wait_for_idle(), an explicit BUG
> >>> will be triggered.
>
> Right, AFAICT a GEM_BUG_ON in debug builds, but in production builds
> negative timeout will get passed along all the way to schedule_timeout
> where error and call trace will be dumped. So fix appears warranted indeed.
>
> >>> If request retirement succeeds but an error code is passed back via
> >>> remaininig_timeout, we may have no clue on how much of the initial timeout
> >>> might have been left for spending it on waiting for GuC to become idle.
> >>> OTOH, since all pending requests have been successfully retired, that
> >>> error code has been already ignored by intel_gt_retire_requests_timeout(),
> >>> then we shouldn't fail.
> >>>
> >>> Assume no more time has been left on error and pass 0 timeout value to
> >>> intel_uc_wait_for_idle() to give it a chance to return success if GuC is
> >>> already idle.
> >>>
> >>> v3: Don't fail on any error passed back via remaining_timeout.
> >>>
> >>> v2: Fix the issue on the caller side, not the provider.
> >>>
> >>> Fixes: b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
> > with GuC")
> >>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >>> Cc: stable@vger.kernel.org # v5.15+
> >>
> >> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> >
> > While still open for comments from others, I'm now looking for potential
> > committer.
>
> Both patches are considered good to go?
Yes, I hope so. The actual diff of 2/2 v3 has received R-b from Andrzej still
under discussion of v2, then I decided to add that tag to v3.
Andrzej, can you please respond to 2/2 v3 and confirm your R-b applies?
Thanks,
Janusz
>
> Regards,
>
> Tvrtko
>
> >
> > Thanks,
> > Janusz
> >
> >
> >>
> >> Regards
> >> Andrzej
> >>
> >>> ---
> >>> drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
> >>> 1 file changed, 7 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/
> > intel_gt.c
> >>> index b5ad9caa55372..7ef0edb2e37cd 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> >>> @@ -677,8 +677,13 @@ int intel_gt_wait_for_idle(struct intel_gt *gt, long
> > timeout)
> >>> return -EINTR;
> >>> }
> >>>
> >>> - return timeout ? timeout : intel_uc_wait_for_idle(>->uc,
> >>> -
> > remaining_timeout);
> >>> + if (timeout)
> >>> + return timeout;
> >>> +
> >>> + if (remaining_timeout < 0)
> >>> + remaining_timeout = 0;
> >>> +
> >>> + return intel_uc_wait_for_idle(>->uc, remaining_timeout);
> >>> }
> >>>
> >>> int intel_gt_init(struct intel_gt *gt)
> >>
> >>
> >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 1/2] drm/i915: Fix negative value passed as remaining time
@ 2022-11-23 11:29 ` Janusz Krzysztofik
0 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-23 11:29 UTC (permalink / raw)
To: Joonas Lahtinen, Andrzej Hajda, Tvrtko Ursulin
Cc: intel-gfx, dri-devel, Chris Wilson, Janusz Krzysztofik, Nirmoy Das
On Tuesday, 22 November 2022 11:41:29 CET Tvrtko Ursulin wrote:
>
> On 21/11/2022 23:19, Janusz Krzysztofik wrote:
> > Hi Andrzej,
> >
> > Thanks for providing your R-b.
> >
> > On Monday, 21 November 2022 18:40:51 CET Andrzej Hajda wrote:
> >> On 21.11.2022 15:56, Janusz Krzysztofik wrote:
> >>> Commit b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
> >>> with GuC") extended the API of intel_gt_retire_requests_timeout() with an
> >>> extra argument 'remaining_timeout', intended for passing back unconsumed
> >>> portion of requested timeout when 0 (success) is returned. However, when
> >>> request retirement happens to succeed despite an error returned by a call
> >>> to dma_fence_wait_timeout(), that error code (a negative value) is passed
> >>> back instead of remaining time. If we then pass that negative value
> >>> forward as requested timeout to intel_uc_wait_for_idle(), an explicit BUG
> >>> will be triggered.
>
> Right, AFAICT a GEM_BUG_ON in debug builds, but in production builds
> negative timeout will get passed along all the way to schedule_timeout
> where error and call trace will be dumped. So fix appears warranted indeed.
>
> >>> If request retirement succeeds but an error code is passed back via
> >>> remaininig_timeout, we may have no clue on how much of the initial timeout
> >>> might have been left for spending it on waiting for GuC to become idle.
> >>> OTOH, since all pending requests have been successfully retired, that
> >>> error code has been already ignored by intel_gt_retire_requests_timeout(),
> >>> then we shouldn't fail.
> >>>
> >>> Assume no more time has been left on error and pass 0 timeout value to
> >>> intel_uc_wait_for_idle() to give it a chance to return success if GuC is
> >>> already idle.
> >>>
> >>> v3: Don't fail on any error passed back via remaining_timeout.
> >>>
> >>> v2: Fix the issue on the caller side, not the provider.
> >>>
> >>> Fixes: b97060a99b01 ("drm/i915/guc: Update intel_gt_wait_for_idle to work
> > with GuC")
> >>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >>> Cc: stable@vger.kernel.org # v5.15+
> >>
> >> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> >
> > While still open for comments from others, I'm now looking for potential
> > committer.
>
> Both patches are considered good to go?
Yes, I hope so. The actual diff of 2/2 v3 has received R-b from Andrzej still
under discussion of v2, then I decided to add that tag to v3.
Andrzej, can you please respond to 2/2 v3 and confirm your R-b applies?
Thanks,
Janusz
>
> Regards,
>
> Tvrtko
>
> >
> > Thanks,
> > Janusz
> >
> >
> >>
> >> Regards
> >> Andrzej
> >>
> >>> ---
> >>> drivers/gpu/drm/i915/gt/intel_gt.c | 9 +++++++--
> >>> 1 file changed, 7 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/
> > intel_gt.c
> >>> index b5ad9caa55372..7ef0edb2e37cd 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> >>> @@ -677,8 +677,13 @@ int intel_gt_wait_for_idle(struct intel_gt *gt, long
> > timeout)
> >>> return -EINTR;
> >>> }
> >>>
> >>> - return timeout ? timeout : intel_uc_wait_for_idle(>->uc,
> >>> -
> > remaining_timeout);
> >>> + if (timeout)
> >>> + return timeout;
> >>> +
> >>> + if (remaining_timeout < 0)
> >>> + remaining_timeout = 0;
> >>> +
> >>> + return intel_uc_wait_for_idle(>->uc, remaining_timeout);
> >>> }
> >>>
> >>> int intel_gt_init(struct intel_gt *gt)
> >>
> >>
> >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-23 9:28 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-23 12:57 ` Tvrtko Ursulin
-1 siblings, 0 replies; 29+ messages in thread
From: Tvrtko Ursulin @ 2022-11-23 12:57 UTC (permalink / raw)
To: Janusz Krzysztofik, Joonas Lahtinen
Cc: Matthew Brost, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
John Harrison, Nirmoy Das
On 23/11/2022 09:28, Janusz Krzysztofik wrote:
> Hi Tvrtko,
>
> Thanks for your comments.
>
> On Tuesday, 22 November 2022 11:50:38 CET Tvrtko Ursulin wrote:
>>
>> On 21/11/2022 14:56, Janusz Krzysztofik wrote:
>>> Users of intel_gt_retire_requests_timeout() expect 0 return value on
>>> success. However, we have no protection from passing back 0 potentially
>>> returned by a call to dma_fence_wait_timeout() when it succedes right
>>> after its timeout has expired.
>>
>> Is this talking about a potential weakness, or ambiguous kerneldoc, of
>> dma_fence_wait_timeout, dma_fence_default_wait and
>> i915_request_wait_timeout? They appear to say 0 return means timeout,
>> implying unsignaled fence. In other words signaled must return positive
>> remaining timeout. Implementations seems to allow a race which indeed
>> appears that return 0 and signaled fence is possible.
>
> While my initial analysis was indeed focused on inconsistent semantics of 0
> return values from different dma_fence_default_wait() backends, I should have
> also mentioned in this commit description that users may perfectly
> call intel_gt_retire_requests_timeout() with 0 timeout, in which case the
> false positive 0 value can be returned regardless of dma_fence_wait_timeout()
> potential issues. Would you like me to reword and resubmit?
Not sure yet.
So the only caller which passes in zero to
intel_gt_retire_requests_timeout appears to be intel_gt_retire_requests
and it eats the return value anyway so this patch is immaterial for that
one.
I guess it can change how intel_gt_wait_for_idle behaves with short-ish
timeouts. In case this race is hit. But then wouldn't it make sense to
follow up with a patch which addresses this race by re-checking the "is
signaled" when timeout expires, either in dma_fence_wait_timeout, or to
intel_gt_retire_requests_timeout. Or if not that at least better
document the dma_fence_wait_timeout and/or
intel_gt_retire_requests_timeout. Makes sense?
Regards,
Tvrtko
>
>> If dma_fence_wait can indeed return 0 even when a request is signaled,
>> then how is timeout ?: -ETIME below correct? It isn't a chance for false
>> negative in its' callers?
>
> The goal of intel_gt_retire_requests_timeout() is to retire requests. When
> that goal has been reached, i.e., all requests have been retired, active count
> is 0, and 0 is correctly returned, regardless of timeout value.
>
> The value of timeout is used only when there are still pending requests, which
> means that the goal hasn't been reached and the function hasn't succeeded.
> Then, no false negative is possible, unlike the false positive that we now
> have when we return 0 while some requests are still pending.
>
> Thanks,
> Janusz
>
>>
>> Regards,
>>
>> Tvrtko
>>
>>> Replace 0 with -ETIME before potentially using the timeout value as return
>>> code, so -ETIME is returned if there are still some requests not retired
>>> after timeout, 0 otherwise.
>>>
>>> v3: Use conditional expression, more compact but also better reflecting
>>> intention standing behind the change.
>>>
>>> v2: Move the added lines down so flush_submission() is not affected.
>>>
>>> Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
>>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>>> Cc: stable@vger.kernel.org # v5.5+
>>> ---
>>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>> index edb881d756309..1dfd01668c79c 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>> @@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
>>> if (remaining_timeout)
>>> *remaining_timeout = timeout;
>>>
>>> - return active_count ? timeout : 0;
>>> + return active_count ? timeout ?: -ETIME : 0;
>>> }
>>>
>>> static void retire_work_handler(struct work_struct *work)
>>
>
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-23 12:57 ` Tvrtko Ursulin
0 siblings, 0 replies; 29+ messages in thread
From: Tvrtko Ursulin @ 2022-11-23 12:57 UTC (permalink / raw)
To: Janusz Krzysztofik, Joonas Lahtinen
Cc: Daniel Vetter, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Nirmoy Das
On 23/11/2022 09:28, Janusz Krzysztofik wrote:
> Hi Tvrtko,
>
> Thanks for your comments.
>
> On Tuesday, 22 November 2022 11:50:38 CET Tvrtko Ursulin wrote:
>>
>> On 21/11/2022 14:56, Janusz Krzysztofik wrote:
>>> Users of intel_gt_retire_requests_timeout() expect 0 return value on
>>> success. However, we have no protection from passing back 0 potentially
>>> returned by a call to dma_fence_wait_timeout() when it succedes right
>>> after its timeout has expired.
>>
>> Is this talking about a potential weakness, or ambiguous kerneldoc, of
>> dma_fence_wait_timeout, dma_fence_default_wait and
>> i915_request_wait_timeout? They appear to say 0 return means timeout,
>> implying unsignaled fence. In other words signaled must return positive
>> remaining timeout. Implementations seems to allow a race which indeed
>> appears that return 0 and signaled fence is possible.
>
> While my initial analysis was indeed focused on inconsistent semantics of 0
> return values from different dma_fence_default_wait() backends, I should have
> also mentioned in this commit description that users may perfectly
> call intel_gt_retire_requests_timeout() with 0 timeout, in which case the
> false positive 0 value can be returned regardless of dma_fence_wait_timeout()
> potential issues. Would you like me to reword and resubmit?
Not sure yet.
So the only caller which passes in zero to
intel_gt_retire_requests_timeout appears to be intel_gt_retire_requests
and it eats the return value anyway so this patch is immaterial for that
one.
I guess it can change how intel_gt_wait_for_idle behaves with short-ish
timeouts. In case this race is hit. But then wouldn't it make sense to
follow up with a patch which addresses this race by re-checking the "is
signaled" when timeout expires, either in dma_fence_wait_timeout, or to
intel_gt_retire_requests_timeout. Or if not that at least better
document the dma_fence_wait_timeout and/or
intel_gt_retire_requests_timeout. Makes sense?
Regards,
Tvrtko
>
>> If dma_fence_wait can indeed return 0 even when a request is signaled,
>> then how is timeout ?: -ETIME below correct? It isn't a chance for false
>> negative in its' callers?
>
> The goal of intel_gt_retire_requests_timeout() is to retire requests. When
> that goal has been reached, i.e., all requests have been retired, active count
> is 0, and 0 is correctly returned, regardless of timeout value.
>
> The value of timeout is used only when there are still pending requests, which
> means that the goal hasn't been reached and the function hasn't succeeded.
> Then, no false negative is possible, unlike the false positive that we now
> have when we return 0 while some requests are still pending.
>
> Thanks,
> Janusz
>
>>
>> Regards,
>>
>> Tvrtko
>>
>>> Replace 0 with -ETIME before potentially using the timeout value as return
>>> code, so -ETIME is returned if there are still some requests not retired
>>> after timeout, 0 otherwise.
>>>
>>> v3: Use conditional expression, more compact but also better reflecting
>>> intention standing behind the change.
>>>
>>> v2: Move the added lines down so flush_submission() is not affected.
>>>
>>> Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
>>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>>> Cc: stable@vger.kernel.org # v5.5+
>>> ---
>>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>> index edb881d756309..1dfd01668c79c 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>> @@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
>>> if (remaining_timeout)
>>> *remaining_timeout = timeout;
>>>
>>> - return active_count ? timeout : 0;
>>> + return active_count ? timeout ?: -ETIME : 0;
>>> }
>>>
>>> static void retire_work_handler(struct work_struct *work)
>>
>
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-21 14:56 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-23 15:42 ` Andrzej Hajda
-1 siblings, 0 replies; 29+ messages in thread
From: Andrzej Hajda @ 2022-11-23 15:42 UTC (permalink / raw)
To: Janusz Krzysztofik, Tvrtko Ursulin, Joonas Lahtinen
Cc: Matthew Brost, intel-gfx, Chris Wilson, dri-devel, John Harrison,
Nirmoy Das
On 21.11.2022 15:56, Janusz Krzysztofik wrote:
> Users of intel_gt_retire_requests_timeout() expect 0 return value on
> success. However, we have no protection from passing back 0 potentially
> returned by a call to dma_fence_wait_timeout() when it succedes right
> after its timeout has expired.
>
> Replace 0 with -ETIME before potentially using the timeout value as return
> code, so -ETIME is returned if there are still some requests not retired
> after timeout, 0 otherwise.
>
> v3: Use conditional expression, more compact but also better reflecting
> intention standing behind the change.
>
> v2: Move the added lines down so flush_submission() is not affected.
>
> Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
I confirm my r-b.
Regards
Andrzej
> Cc: stable@vger.kernel.org # v5.5+
> ---
> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> index edb881d756309..1dfd01668c79c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> @@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
> if (remaining_timeout)
> *remaining_timeout = timeout;
>
> - return active_count ? timeout : 0;
> + return active_count ? timeout ?: -ETIME : 0;
> }
>
> static void retire_work_handler(struct work_struct *work)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-23 15:42 ` Andrzej Hajda
0 siblings, 0 replies; 29+ messages in thread
From: Andrzej Hajda @ 2022-11-23 15:42 UTC (permalink / raw)
To: Janusz Krzysztofik, Tvrtko Ursulin, Joonas Lahtinen
Cc: intel-gfx, Chris Wilson, dri-devel, Daniel Vetter, Nirmoy Das
On 21.11.2022 15:56, Janusz Krzysztofik wrote:
> Users of intel_gt_retire_requests_timeout() expect 0 return value on
> success. However, we have no protection from passing back 0 potentially
> returned by a call to dma_fence_wait_timeout() when it succedes right
> after its timeout has expired.
>
> Replace 0 with -ETIME before potentially using the timeout value as return
> code, so -ETIME is returned if there are still some requests not retired
> after timeout, 0 otherwise.
>
> v3: Use conditional expression, more compact but also better reflecting
> intention standing behind the change.
>
> v2: Move the added lines down so flush_submission() is not affected.
>
> Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
I confirm my r-b.
Regards
Andrzej
> Cc: stable@vger.kernel.org # v5.5+
> ---
> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> index edb881d756309..1dfd01668c79c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> @@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
> if (remaining_timeout)
> *remaining_timeout = timeout;
>
> - return active_count ? timeout : 0;
> + return active_count ? timeout ?: -ETIME : 0;
> }
>
> static void retire_work_handler(struct work_struct *work)
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-23 12:57 ` [Intel-gfx] " Tvrtko Ursulin
@ 2022-11-23 16:21 ` Janusz Krzysztofik
-1 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-23 16:21 UTC (permalink / raw)
To: Joonas Lahtinen, Tvrtko Ursulin
Cc: Matthew Brost, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Janusz Krzysztofik, John Harrison, Nirmoy Das
On Wednesday, 23 November 2022 13:57:26 CET Tvrtko Ursulin wrote:
>
> On 23/11/2022 09:28, Janusz Krzysztofik wrote:
> > Hi Tvrtko,
> >
> > Thanks for your comments.
> >
> > On Tuesday, 22 November 2022 11:50:38 CET Tvrtko Ursulin wrote:
> >>
> >> On 21/11/2022 14:56, Janusz Krzysztofik wrote:
> >>> Users of intel_gt_retire_requests_timeout() expect 0 return value on
> >>> success. However, we have no protection from passing back 0 potentially
> >>> returned by a call to dma_fence_wait_timeout() when it succedes right
> >>> after its timeout has expired.
> >>
> >> Is this talking about a potential weakness, or ambiguous kerneldoc, of
> >> dma_fence_wait_timeout, dma_fence_default_wait and
> >> i915_request_wait_timeout? They appear to say 0 return means timeout,
> >> implying unsignaled fence. In other words signaled must return positive
> >> remaining timeout. Implementations seems to allow a race which indeed
> >> appears that return 0 and signaled fence is possible.
> >
> > While my initial analysis was indeed focused on inconsistent semantics of 0
> > return values from different dma_fence_default_wait() backends, I should have
> > also mentioned in this commit description that users may perfectly
> > call intel_gt_retire_requests_timeout() with 0 timeout, in which case the
> > false positive 0 value can be returned regardless of dma_fence_wait_timeout()
> > potential issues. Would you like me to reword and resubmit?
>
> Not sure yet.
>
> So the only caller which passes in zero to
> intel_gt_retire_requests_timeout appears to be intel_gt_retire_requests
> and it eats the return value anyway so this patch is immaterial for that
> one.
Right.
> I guess it can change how intel_gt_wait_for_idle behaves with short-ish
> timeouts. In case this race is hit. But then wouldn't it make sense to
> follow up with a patch which addresses this race by re-checking the "is
> signaled" when timeout expires,
But inside intel_gt_retire_requests_timeout() we generally don't care if
fences have been signaled. As long as user requested timeout hasn't expired,
we use dma_fence_wait_timeout() as an aid, otherwise we keep trying to retire
requests without waiting on fences. If the retirement succeeds then we return
0 (success) regardless of what the return value from the last called
dma_fence_wait_timeout() was. If it was 0 then the only useful information is
that no more time has been left, and no matter if that 0 meant signaled or not
signaled, we must return an error if there are still some requests not
retired, I believe.
> either in dma_fence_wait_timeout, or to
> intel_gt_retire_requests_timeout. Or if not that at least better
> document the dma_fence_wait_timeout and/or
> intel_gt_retire_requests_timeout. Makes sense?
Documenting -- yes, as soon as we get into an agreement on what's the core of
this issue -- whether that potential weakness, or ambiguous kerneldoc, of
dma_fence_wait_timeout, dma_fence_default_wait and i915_request_wait_timeout,
as you've stated, that we have to address somehow, or potentially incorrect
direct use of the timeout variable, intended for storing time left to spend on
fence waits, as our return value when timeout has expired. And if the former
then maybe we should also try to finally resolve that over a year old conflict
(whether 0 means signaled on not signaled) inside our implementation of
dma_fence_ops.wait, and simply use a wrapper around it for either our internal
use if we decide to follow the reference implementation, or for dma_fence_ops
use otherwise. Or maybe the reference implementation should be fixed if
problematic. I don't feel competent enough to decide.
Thanks,
Janusz
>
> Regards,
>
> Tvrtko
>
> >
> >> If dma_fence_wait can indeed return 0 even when a request is signaled,
> >> then how is timeout ?: -ETIME below correct? It isn't a chance for false
> >> negative in its' callers?
> >
> > The goal of intel_gt_retire_requests_timeout() is to retire requests. When
> > that goal has been reached, i.e., all requests have been retired, active count
> > is 0, and 0 is correctly returned, regardless of timeout value.
> >
> > The value of timeout is used only when there are still pending requests, which
> > means that the goal hasn't been reached and the function hasn't succeeded.
> > Then, no false negative is possible, unlike the false positive that we now
> > have when we return 0 while some requests are still pending.
> >
> > Thanks,
> > Janusz
> >
> >>
> >> Regards,
> >>
> >> Tvrtko
> >>
> >>> Replace 0 with -ETIME before potentially using the timeout value as return
> >>> code, so -ETIME is returned if there are still some requests not retired
> >>> after timeout, 0 otherwise.
> >>>
> >>> v3: Use conditional expression, more compact but also better reflecting
> >>> intention standing behind the change.
> >>>
> >>> v2: Move the added lines down so flush_submission() is not affected.
> >>>
> >>> Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
> >>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> >>> Cc: stable@vger.kernel.org # v5.5+
> >>> ---
> >>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>> index edb881d756309..1dfd01668c79c 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>> @@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
> >>> if (remaining_timeout)
> >>> *remaining_timeout = timeout;
> >>>
> >>> - return active_count ? timeout : 0;
> >>> + return active_count ? timeout ?: -ETIME : 0;
> >>> }
> >>>
> >>> static void retire_work_handler(struct work_struct *work)
> >>
> >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-23 16:21 ` Janusz Krzysztofik
0 siblings, 0 replies; 29+ messages in thread
From: Janusz Krzysztofik @ 2022-11-23 16:21 UTC (permalink / raw)
To: Joonas Lahtinen, Tvrtko Ursulin
Cc: Daniel Vetter, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Nirmoy Das
On Wednesday, 23 November 2022 13:57:26 CET Tvrtko Ursulin wrote:
>
> On 23/11/2022 09:28, Janusz Krzysztofik wrote:
> > Hi Tvrtko,
> >
> > Thanks for your comments.
> >
> > On Tuesday, 22 November 2022 11:50:38 CET Tvrtko Ursulin wrote:
> >>
> >> On 21/11/2022 14:56, Janusz Krzysztofik wrote:
> >>> Users of intel_gt_retire_requests_timeout() expect 0 return value on
> >>> success. However, we have no protection from passing back 0 potentially
> >>> returned by a call to dma_fence_wait_timeout() when it succedes right
> >>> after its timeout has expired.
> >>
> >> Is this talking about a potential weakness, or ambiguous kerneldoc, of
> >> dma_fence_wait_timeout, dma_fence_default_wait and
> >> i915_request_wait_timeout? They appear to say 0 return means timeout,
> >> implying unsignaled fence. In other words signaled must return positive
> >> remaining timeout. Implementations seems to allow a race which indeed
> >> appears that return 0 and signaled fence is possible.
> >
> > While my initial analysis was indeed focused on inconsistent semantics of 0
> > return values from different dma_fence_default_wait() backends, I should have
> > also mentioned in this commit description that users may perfectly
> > call intel_gt_retire_requests_timeout() with 0 timeout, in which case the
> > false positive 0 value can be returned regardless of dma_fence_wait_timeout()
> > potential issues. Would you like me to reword and resubmit?
>
> Not sure yet.
>
> So the only caller which passes in zero to
> intel_gt_retire_requests_timeout appears to be intel_gt_retire_requests
> and it eats the return value anyway so this patch is immaterial for that
> one.
Right.
> I guess it can change how intel_gt_wait_for_idle behaves with short-ish
> timeouts. In case this race is hit. But then wouldn't it make sense to
> follow up with a patch which addresses this race by re-checking the "is
> signaled" when timeout expires,
But inside intel_gt_retire_requests_timeout() we generally don't care if
fences have been signaled. As long as user requested timeout hasn't expired,
we use dma_fence_wait_timeout() as an aid, otherwise we keep trying to retire
requests without waiting on fences. If the retirement succeeds then we return
0 (success) regardless of what the return value from the last called
dma_fence_wait_timeout() was. If it was 0 then the only useful information is
that no more time has been left, and no matter if that 0 meant signaled or not
signaled, we must return an error if there are still some requests not
retired, I believe.
> either in dma_fence_wait_timeout, or to
> intel_gt_retire_requests_timeout. Or if not that at least better
> document the dma_fence_wait_timeout and/or
> intel_gt_retire_requests_timeout. Makes sense?
Documenting -- yes, as soon as we get into an agreement on what's the core of
this issue -- whether that potential weakness, or ambiguous kerneldoc, of
dma_fence_wait_timeout, dma_fence_default_wait and i915_request_wait_timeout,
as you've stated, that we have to address somehow, or potentially incorrect
direct use of the timeout variable, intended for storing time left to spend on
fence waits, as our return value when timeout has expired. And if the former
then maybe we should also try to finally resolve that over a year old conflict
(whether 0 means signaled on not signaled) inside our implementation of
dma_fence_ops.wait, and simply use a wrapper around it for either our internal
use if we decide to follow the reference implementation, or for dma_fence_ops
use otherwise. Or maybe the reference implementation should be fixed if
problematic. I don't feel competent enough to decide.
Thanks,
Janusz
>
> Regards,
>
> Tvrtko
>
> >
> >> If dma_fence_wait can indeed return 0 even when a request is signaled,
> >> then how is timeout ?: -ETIME below correct? It isn't a chance for false
> >> negative in its' callers?
> >
> > The goal of intel_gt_retire_requests_timeout() is to retire requests. When
> > that goal has been reached, i.e., all requests have been retired, active count
> > is 0, and 0 is correctly returned, regardless of timeout value.
> >
> > The value of timeout is used only when there are still pending requests, which
> > means that the goal hasn't been reached and the function hasn't succeeded.
> > Then, no false negative is possible, unlike the false positive that we now
> > have when we return 0 while some requests are still pending.
> >
> > Thanks,
> > Janusz
> >
> >>
> >> Regards,
> >>
> >> Tvrtko
> >>
> >>> Replace 0 with -ETIME before potentially using the timeout value as return
> >>> code, so -ETIME is returned if there are still some requests not retired
> >>> after timeout, 0 otherwise.
> >>>
> >>> v3: Use conditional expression, more compact but also better reflecting
> >>> intention standing behind the change.
> >>>
> >>> v2: Move the added lines down so flush_submission() is not affected.
> >>>
> >>> Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
> >>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
> >>> Cc: stable@vger.kernel.org # v5.5+
> >>> ---
> >>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>> index edb881d756309..1dfd01668c79c 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>> @@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
> >>> if (remaining_timeout)
> >>> *remaining_timeout = timeout;
> >>>
> >>> - return active_count ? timeout : 0;
> >>> + return active_count ? timeout ?: -ETIME : 0;
> >>> }
> >>>
> >>> static void retire_work_handler(struct work_struct *work)
> >>
> >
> >
> >
> >
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-23 16:21 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-24 12:27 ` Tvrtko Ursulin
-1 siblings, 0 replies; 29+ messages in thread
From: Tvrtko Ursulin @ 2022-11-24 12:27 UTC (permalink / raw)
To: Janusz Krzysztofik, Joonas Lahtinen
Cc: Matthew Brost, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
John Harrison, Nirmoy Das
On 23/11/2022 16:21, Janusz Krzysztofik wrote:
> On Wednesday, 23 November 2022 13:57:26 CET Tvrtko Ursulin wrote:
>>
>> On 23/11/2022 09:28, Janusz Krzysztofik wrote:
>>> Hi Tvrtko,
>>>
>>> Thanks for your comments.
>>>
>>> On Tuesday, 22 November 2022 11:50:38 CET Tvrtko Ursulin wrote:
>>>>
>>>> On 21/11/2022 14:56, Janusz Krzysztofik wrote:
>>>>> Users of intel_gt_retire_requests_timeout() expect 0 return value on
>>>>> success. However, we have no protection from passing back 0 potentially
>>>>> returned by a call to dma_fence_wait_timeout() when it succedes right
>>>>> after its timeout has expired.
>>>>
>>>> Is this talking about a potential weakness, or ambiguous kerneldoc, of
>>>> dma_fence_wait_timeout, dma_fence_default_wait and
>>>> i915_request_wait_timeout? They appear to say 0 return means timeout,
>>>> implying unsignaled fence. In other words signaled must return positive
>>>> remaining timeout. Implementations seems to allow a race which indeed
>>>> appears that return 0 and signaled fence is possible.
>>>
>>> While my initial analysis was indeed focused on inconsistent semantics of 0
>>> return values from different dma_fence_default_wait() backends, I should have
>>> also mentioned in this commit description that users may perfectly
>>> call intel_gt_retire_requests_timeout() with 0 timeout, in which case the
>>> false positive 0 value can be returned regardless of dma_fence_wait_timeout()
>>> potential issues. Would you like me to reword and resubmit?
>>
>> Not sure yet.
>>
>> So the only caller which passes in zero to
>> intel_gt_retire_requests_timeout appears to be intel_gt_retire_requests
>> and it eats the return value anyway so this patch is immaterial for that
>> one.
>
> Right.
>
>> I guess it can change how intel_gt_wait_for_idle behaves with short-ish
>> timeouts. In case this race is hit. But then wouldn't it make sense to
>> follow up with a patch which addresses this race by re-checking the "is
>> signaled" when timeout expires,
>
> But inside intel_gt_retire_requests_timeout() we generally don't care if
> fences have been signaled. As long as user requested timeout hasn't expired,
> we use dma_fence_wait_timeout() as an aid, otherwise we keep trying to retire
> requests without waiting on fences. If the retirement succeeds then we return
> 0 (success) regardless of what the return value from the last called
> dma_fence_wait_timeout() was. If it was 0 then the only useful information is
> that no more time has been left, and no matter if that 0 meant signaled or not
> signaled, we must return an error if there are still some requests not
> retired, I believe.
Yes I agree with all that. Sorry if my reply was misleading, I mentioned
a follow-up, didn't mean that these two are not ready to go.
>> either in dma_fence_wait_timeout, or to
>> intel_gt_retire_requests_timeout. Or if not that at least better
>> document the dma_fence_wait_timeout and/or
>> intel_gt_retire_requests_timeout. Makes sense?
>
> Documenting -- yes, as soon as we get into an agreement on what's the core of
> this issue -- whether that potential weakness, or ambiguous kerneldoc, of
> dma_fence_wait_timeout, dma_fence_default_wait and i915_request_wait_timeout,
> as you've stated, that we have to address somehow, or potentially incorrect
> direct use of the timeout variable, intended for storing time left to spend on
> fence waits, as our return value when timeout has expired. And if the former
> then maybe we should also try to finally resolve that over a year old conflict
> (whether 0 means signaled on not signaled) inside our implementation of
> dma_fence_ops.wait, and simply use a wrapper around it for either our internal
> use if we decide to follow the reference implementation, or for dma_fence_ops
> use otherwise. Or maybe the reference implementation should be fixed if
> problematic. I don't feel competent enough to decide.
Right, we can leave that for later. I'll pull these two in if someone
hasn't already.
Regards,
Tvrtko
>
> Thanks,
> Janusz
>
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>>> If dma_fence_wait can indeed return 0 even when a request is signaled,
>>>> then how is timeout ?: -ETIME below correct? It isn't a chance for false
>>>> negative in its' callers?
>>>
>>> The goal of intel_gt_retire_requests_timeout() is to retire requests. When
>>> that goal has been reached, i.e., all requests have been retired, active count
>>> is 0, and 0 is correctly returned, regardless of timeout value.
>>>
>>> The value of timeout is used only when there are still pending requests, which
>>> means that the goal hasn't been reached and the function hasn't succeeded.
>>> Then, no false negative is possible, unlike the false positive that we now
>>> have when we return 0 while some requests are still pending.
>>>
>>> Thanks,
>>> Janusz
>>>
>>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>> Replace 0 with -ETIME before potentially using the timeout value as return
>>>>> code, so -ETIME is returned if there are still some requests not retired
>>>>> after timeout, 0 otherwise.
>>>>>
>>>>> v3: Use conditional expression, more compact but also better reflecting
>>>>> intention standing behind the change.
>>>>>
>>>>> v2: Move the added lines down so flush_submission() is not affected.
>>>>>
>>>>> Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
>>>>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>>>>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>>>>> Cc: stable@vger.kernel.org # v5.5+
>>>>> ---
>>>>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>>>> index edb881d756309..1dfd01668c79c 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>>>> @@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
>>>>> if (remaining_timeout)
>>>>> *remaining_timeout = timeout;
>>>>>
>>>>> - return active_count ? timeout : 0;
>>>>> + return active_count ? timeout ?: -ETIME : 0;
>>>>> }
>>>>>
>>>>> static void retire_work_handler(struct work_struct *work)
>>>>
>>>
>>>
>>>
>>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Intel-gfx] [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-24 12:27 ` Tvrtko Ursulin
0 siblings, 0 replies; 29+ messages in thread
From: Tvrtko Ursulin @ 2022-11-24 12:27 UTC (permalink / raw)
To: Janusz Krzysztofik, Joonas Lahtinen
Cc: Daniel Vetter, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Nirmoy Das
On 23/11/2022 16:21, Janusz Krzysztofik wrote:
> On Wednesday, 23 November 2022 13:57:26 CET Tvrtko Ursulin wrote:
>>
>> On 23/11/2022 09:28, Janusz Krzysztofik wrote:
>>> Hi Tvrtko,
>>>
>>> Thanks for your comments.
>>>
>>> On Tuesday, 22 November 2022 11:50:38 CET Tvrtko Ursulin wrote:
>>>>
>>>> On 21/11/2022 14:56, Janusz Krzysztofik wrote:
>>>>> Users of intel_gt_retire_requests_timeout() expect 0 return value on
>>>>> success. However, we have no protection from passing back 0 potentially
>>>>> returned by a call to dma_fence_wait_timeout() when it succedes right
>>>>> after its timeout has expired.
>>>>
>>>> Is this talking about a potential weakness, or ambiguous kerneldoc, of
>>>> dma_fence_wait_timeout, dma_fence_default_wait and
>>>> i915_request_wait_timeout? They appear to say 0 return means timeout,
>>>> implying unsignaled fence. In other words signaled must return positive
>>>> remaining timeout. Implementations seems to allow a race which indeed
>>>> appears that return 0 and signaled fence is possible.
>>>
>>> While my initial analysis was indeed focused on inconsistent semantics of 0
>>> return values from different dma_fence_default_wait() backends, I should have
>>> also mentioned in this commit description that users may perfectly
>>> call intel_gt_retire_requests_timeout() with 0 timeout, in which case the
>>> false positive 0 value can be returned regardless of dma_fence_wait_timeout()
>>> potential issues. Would you like me to reword and resubmit?
>>
>> Not sure yet.
>>
>> So the only caller which passes in zero to
>> intel_gt_retire_requests_timeout appears to be intel_gt_retire_requests
>> and it eats the return value anyway so this patch is immaterial for that
>> one.
>
> Right.
>
>> I guess it can change how intel_gt_wait_for_idle behaves with short-ish
>> timeouts. In case this race is hit. But then wouldn't it make sense to
>> follow up with a patch which addresses this race by re-checking the "is
>> signaled" when timeout expires,
>
> But inside intel_gt_retire_requests_timeout() we generally don't care if
> fences have been signaled. As long as user requested timeout hasn't expired,
> we use dma_fence_wait_timeout() as an aid, otherwise we keep trying to retire
> requests without waiting on fences. If the retirement succeeds then we return
> 0 (success) regardless of what the return value from the last called
> dma_fence_wait_timeout() was. If it was 0 then the only useful information is
> that no more time has been left, and no matter if that 0 meant signaled or not
> signaled, we must return an error if there are still some requests not
> retired, I believe.
Yes I agree with all that. Sorry if my reply was misleading, I mentioned
a follow-up, didn't mean that these two are not ready to go.
>> either in dma_fence_wait_timeout, or to
>> intel_gt_retire_requests_timeout. Or if not that at least better
>> document the dma_fence_wait_timeout and/or
>> intel_gt_retire_requests_timeout. Makes sense?
>
> Documenting -- yes, as soon as we get into an agreement on what's the core of
> this issue -- whether that potential weakness, or ambiguous kerneldoc, of
> dma_fence_wait_timeout, dma_fence_default_wait and i915_request_wait_timeout,
> as you've stated, that we have to address somehow, or potentially incorrect
> direct use of the timeout variable, intended for storing time left to spend on
> fence waits, as our return value when timeout has expired. And if the former
> then maybe we should also try to finally resolve that over a year old conflict
> (whether 0 means signaled on not signaled) inside our implementation of
> dma_fence_ops.wait, and simply use a wrapper around it for either our internal
> use if we decide to follow the reference implementation, or for dma_fence_ops
> use otherwise. Or maybe the reference implementation should be fixed if
> problematic. I don't feel competent enough to decide.
Right, we can leave that for later. I'll pull these two in if someone
hasn't already.
Regards,
Tvrtko
>
> Thanks,
> Janusz
>
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>>> If dma_fence_wait can indeed return 0 even when a request is signaled,
>>>> then how is timeout ?: -ETIME below correct? It isn't a chance for false
>>>> negative in its' callers?
>>>
>>> The goal of intel_gt_retire_requests_timeout() is to retire requests. When
>>> that goal has been reached, i.e., all requests have been retired, active count
>>> is 0, and 0 is correctly returned, regardless of timeout value.
>>>
>>> The value of timeout is used only when there are still pending requests, which
>>> means that the goal hasn't been reached and the function hasn't succeeded.
>>> Then, no false negative is possible, unlike the false positive that we now
>>> have when we return 0 while some requests are still pending.
>>>
>>> Thanks,
>>> Janusz
>>>
>>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>> Replace 0 with -ETIME before potentially using the timeout value as return
>>>>> code, so -ETIME is returned if there are still some requests not retired
>>>>> after timeout, 0 otherwise.
>>>>>
>>>>> v3: Use conditional expression, more compact but also better reflecting
>>>>> intention standing behind the change.
>>>>>
>>>>> v2: Move the added lines down so flush_submission() is not affected.
>>>>>
>>>>> Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
>>>>> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>>>>> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>>>>> Cc: stable@vger.kernel.org # v5.5+
>>>>> ---
>>>>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 2 +-
>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>>>> index edb881d756309..1dfd01668c79c 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>>>> @@ -199,7 +199,7 @@ out_active: spin_lock(&timelines->lock);
>>>>> if (remaining_timeout)
>>>>> *remaining_timeout = timeout;
>>>>>
>>>>> - return active_count ? timeout : 0;
>>>>> + return active_count ? timeout ?: -ETIME : 0;
>>>>> }
>>>>>
>>>>> static void retire_work_handler(struct work_struct *work)
>>>>
>>>
>>>
>>>
>>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2022-11-24 12:27 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-21 14:56 [PATCH v3 0/2] drm/i915: Fix timeout handling when retiring requests Janusz Krzysztofik
2022-11-21 14:56 ` [Intel-gfx] " Janusz Krzysztofik
2022-11-21 14:56 ` [PATCH v3 1/2] drm/i915: Fix negative value passed as remaining time Janusz Krzysztofik
2022-11-21 14:56 ` [Intel-gfx] " Janusz Krzysztofik
2022-11-21 17:40 ` Andrzej Hajda
2022-11-21 17:40 ` Andrzej Hajda
2022-11-21 23:19 ` Janusz Krzysztofik
2022-11-21 23:19 ` Janusz Krzysztofik
2022-11-22 10:41 ` Tvrtko Ursulin
2022-11-22 10:41 ` Tvrtko Ursulin
2022-11-23 11:29 ` Janusz Krzysztofik
2022-11-23 11:29 ` Janusz Krzysztofik
2022-11-21 14:56 ` [PATCH v3 2/2] drm/i915: Never return 0 if not all requests retired Janusz Krzysztofik
2022-11-21 14:56 ` [Intel-gfx] " Janusz Krzysztofik
2022-11-22 10:50 ` Tvrtko Ursulin
2022-11-22 10:50 ` [Intel-gfx] " Tvrtko Ursulin
2022-11-23 9:28 ` Janusz Krzysztofik
2022-11-23 9:28 ` [Intel-gfx] " Janusz Krzysztofik
2022-11-23 12:57 ` Tvrtko Ursulin
2022-11-23 12:57 ` [Intel-gfx] " Tvrtko Ursulin
2022-11-23 16:21 ` Janusz Krzysztofik
2022-11-23 16:21 ` [Intel-gfx] " Janusz Krzysztofik
2022-11-24 12:27 ` Tvrtko Ursulin
2022-11-24 12:27 ` [Intel-gfx] " Tvrtko Ursulin
2022-11-23 15:42 ` Andrzej Hajda
2022-11-23 15:42 ` [Intel-gfx] " Andrzej Hajda
2022-11-21 15:37 ` [Intel-gfx] ✗ Fi.CI.DOCS: warning for drm/i915: Fix timeout handling when retiring requests (rev3) Patchwork
2022-11-21 15:56 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2022-11-21 18:03 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.