* [PATCH v2 0/2] drm/i915: Fix timeout handling when retiring requests
@ 2022-11-18 10:42 ` Janusz Krzysztofik
0 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-18 10:42 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.
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 | 11 +++++++++--
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
2 files changed, 12 insertions(+), 2 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Intel-gfx] [PATCH v2 0/2] drm/i915: Fix timeout handling when retiring requests
@ 2022-11-18 10:42 ` Janusz Krzysztofik
0 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-18 10:42 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.
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 | 11 +++++++++--
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
2 files changed, 12 insertions(+), 2 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/2] drm/i915: Fix negative value passed as remaining time
2022-11-18 10:42 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-18 10:42 ` Janusz Krzysztofik
-1 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-18 10:42 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 other than -ETIME is
passed back via remaininig_timeout, we have no clue on how much of
the initial timeout might have been left for spending it on waiting for
GuC to become idle. Then, we have no choice other than fail in that case
-- do it. However, if -ETIME is returned via remaining_timeout then we
know that no more time has been left. Then, pass 0 timeout value to
intel_uc_wait_for_idle() to give it a chance to return success if GuC is
already idle.
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 | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 0325f071046ca..5d612ba547d23 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -677,8 +677,15 @@ 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 == -ETIME)
+ remaining_timeout = 0;
+ else if (remaining_timeout < 0)
+ return remaining_timeout;
+
+ 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] 24+ messages in thread
* [Intel-gfx] [PATCH v2 1/2] drm/i915: Fix negative value passed as remaining time
@ 2022-11-18 10:42 ` Janusz Krzysztofik
0 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-18 10:42 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 other than -ETIME is
passed back via remaininig_timeout, we have no clue on how much of
the initial timeout might have been left for spending it on waiting for
GuC to become idle. Then, we have no choice other than fail in that case
-- do it. However, if -ETIME is returned via remaining_timeout then we
know that no more time has been left. Then, pass 0 timeout value to
intel_uc_wait_for_idle() to give it a chance to return success if GuC is
already idle.
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 | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index 0325f071046ca..5d612ba547d23 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -677,8 +677,15 @@ 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 == -ETIME)
+ remaining_timeout = 0;
+ else if (remaining_timeout < 0)
+ return remaining_timeout;
+
+ 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] 24+ messages in thread
* [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-18 10:42 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-18 10:42 ` Janusz Krzysztofik
-1 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-18 10:42 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.
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>
Cc: stable@vger.kernel.org # v5.5+
---
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index edb881d756309..3ac4603eeb4ee 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
if (remaining_timeout)
*remaining_timeout = timeout;
+ if (!timeout)
+ timeout = -ETIME;
+
return active_count ? timeout : 0;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Intel-gfx] [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-18 10:42 ` Janusz Krzysztofik
0 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-18 10:42 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.
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>
Cc: stable@vger.kernel.org # v5.5+
---
drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index edb881d756309..3ac4603eeb4ee 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
if (remaining_timeout)
*remaining_timeout = timeout;
+ if (!timeout)
+ timeout = -ETIME;
+
return active_count ? timeout : 0;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Fix timeout handling when retiring requests (rev2)
2022-11-18 10:42 ` [Intel-gfx] " Janusz Krzysztofik
` (2 preceding siblings ...)
(?)
@ 2022-11-18 11:34 ` Patchwork
-1 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2022-11-18 11:34 UTC (permalink / raw)
To: Janusz Krzysztofik; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 8651 bytes --]
== Series Details ==
Series: drm/i915: Fix timeout handling when retiring requests (rev2)
URL : https://patchwork.freedesktop.org/series/110964/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_12398 -> Patchwork_110964v2
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_110964v2 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_110964v2, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/index.html
Participating hosts (41 -> 39)
------------------------------
Additional (1): fi-rkl-11600
Missing (3): fi-ctg-p8600 fi-ilk-m540 fi-bdw-samus
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_110964v2:
### IGT changes ###
#### Possible regressions ####
* igt@i915_pm_backlight@basic-brightness:
- fi-rkl-11600: NOTRUN -> [SKIP][1]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@i915_pm_backlight@basic-brightness.html
Known issues
------------
Here are the changes found in Patchwork_110964v2 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@debugfs_test@basic-hwmon:
- fi-rkl-11600: NOTRUN -> [SKIP][2] ([i915#7456])
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@debugfs_test@basic-hwmon.html
* igt@gem_huc_copy@huc-copy:
- fi-rkl-11600: NOTRUN -> [SKIP][3] ([i915#2190])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@gem_huc_copy@huc-copy.html
* igt@gem_lmem_swapping@basic:
- fi-apl-guc: NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#4613]) +3 similar issues
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-apl-guc/igt@gem_lmem_swapping@basic.html
- fi-rkl-11600: NOTRUN -> [SKIP][5] ([i915#4613]) +3 similar issues
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@gem_lmem_swapping@basic.html
* igt@gem_tiled_pread_basic:
- fi-rkl-11600: NOTRUN -> [SKIP][6] ([i915#3282])
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@gem_tiled_pread_basic.html
* igt@i915_selftest@live@gt_heartbeat:
- fi-bxt-dsi: [PASS][7] -> [DMESG-FAIL][8] ([i915#5334])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/fi-bxt-dsi/igt@i915_selftest@live@gt_heartbeat.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-bxt-dsi/igt@i915_selftest@live@gt_heartbeat.html
* igt@i915_suspend@basic-s3-without-i915:
- fi-rkl-11600: NOTRUN -> [INCOMPLETE][9] ([i915#4817])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html
* igt@kms_chamelium@common-hpd-after-suspend:
- fi-apl-guc: NOTRUN -> [SKIP][10] ([fdo#109271] / [fdo#111827])
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-apl-guc/igt@kms_chamelium@common-hpd-after-suspend.html
* igt@kms_chamelium@hdmi-edid-read:
- fi-rkl-11600: NOTRUN -> [SKIP][11] ([fdo#111827]) +7 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@kms_chamelium@hdmi-edid-read.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
- fi-rkl-11600: NOTRUN -> [SKIP][12] ([i915#4103])
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html
* igt@kms_force_connector_basic@force-load-detect:
- fi-rkl-11600: NOTRUN -> [SKIP][13] ([fdo#109285] / [i915#4098])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@kms_force_connector_basic@force-load-detect.html
* igt@kms_psr@primary_page_flip:
- fi-rkl-11600: NOTRUN -> [SKIP][14] ([i915#1072]) +3 similar issues
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@kms_psr@primary_page_flip.html
* igt@kms_setmode@basic-clone-single-crtc:
- fi-rkl-11600: NOTRUN -> [SKIP][15] ([i915#3555] / [i915#4098])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@kms_setmode@basic-clone-single-crtc.html
* igt@prime_vgem@basic-read:
- fi-rkl-11600: NOTRUN -> [SKIP][16] ([fdo#109295] / [i915#3291] / [i915#3708]) +2 similar issues
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@prime_vgem@basic-read.html
* igt@prime_vgem@basic-userptr:
- fi-rkl-11600: NOTRUN -> [SKIP][17] ([fdo#109295] / [i915#3301] / [i915#3708])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@prime_vgem@basic-userptr.html
#### Possible fixes ####
* igt@core_hotunplug@unbind-rebind:
- fi-apl-guc: [INCOMPLETE][18] ([i915#7073]) -> [PASS][19]
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/fi-apl-guc/igt@core_hotunplug@unbind-rebind.html
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-apl-guc/igt@core_hotunplug@unbind-rebind.html
* igt@i915_pm_rpm@module-reload:
- {bat-rpls-2}: [WARN][20] ([i915#7346]) -> [PASS][21]
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/bat-rpls-2/igt@i915_pm_rpm@module-reload.html
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/bat-rpls-2/igt@i915_pm_rpm@module-reload.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size:
- fi-bsw-kefka: [FAIL][22] ([i915#6298]) -> [PASS][23]
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.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#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
[i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
[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#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
[i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
[i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
[i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
[i915#7073]: https://gitlab.freedesktop.org/drm/intel/issues/7073
[i915#7346]: https://gitlab.freedesktop.org/drm/intel/issues/7346
[i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
Build changes
-------------
* Linux: CI_DRM_12398 -> Patchwork_110964v2
CI-20190529: 20190529
CI_DRM_12398: 6ff9396457d55a1915566b11121e8fe6f9068b1c @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7068: 5c0ec905b6bbecfb8df8b8f3315d0470539e6ae3 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_110964v2: 6ff9396457d55a1915566b11121e8fe6f9068b1c @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
65510db8f33c drm/i915: Never return 0 if not all requests retired
d4f26d1d216d drm/i915: Fix negative value passed as remaining time
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/index.html
[-- Attachment #2: Type: text/html, Size: 9941 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Fix timeout handling when retiring requests (rev2)
2022-11-18 10:42 ` [Intel-gfx] " Janusz Krzysztofik
` (3 preceding siblings ...)
(?)
@ 2022-11-18 14:15 ` Patchwork
-1 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2022-11-18 14:15 UTC (permalink / raw)
To: Janusz Krzysztofik; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 8244 bytes --]
== Series Details ==
Series: drm/i915: Fix timeout handling when retiring requests (rev2)
URL : https://patchwork.freedesktop.org/series/110964/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_12398 -> Patchwork_110964v2
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/index.html
Participating hosts (41 -> 39)
------------------------------
Additional (1): fi-rkl-11600
Missing (3): fi-ctg-p8600 fi-ilk-m540 fi-bdw-samus
Known issues
------------
Here are the changes found in Patchwork_110964v2 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@debugfs_test@basic-hwmon:
- fi-rkl-11600: NOTRUN -> [SKIP][1] ([i915#7456])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@debugfs_test@basic-hwmon.html
* igt@gem_huc_copy@huc-copy:
- fi-rkl-11600: NOTRUN -> [SKIP][2] ([i915#2190])
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@gem_huc_copy@huc-copy.html
* igt@gem_lmem_swapping@basic:
- fi-apl-guc: NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#4613]) +3 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-apl-guc/igt@gem_lmem_swapping@basic.html
- fi-rkl-11600: NOTRUN -> [SKIP][4] ([i915#4613]) +3 similar issues
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@gem_lmem_swapping@basic.html
* igt@gem_tiled_pread_basic:
- fi-rkl-11600: NOTRUN -> [SKIP][5] ([i915#3282])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@gem_tiled_pread_basic.html
* igt@i915_pm_backlight@basic-brightness:
- fi-rkl-11600: NOTRUN -> [SKIP][6] ([i915#7561])
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@i915_pm_backlight@basic-brightness.html
* igt@i915_selftest@live@gt_heartbeat:
- fi-bxt-dsi: [PASS][7] -> [DMESG-FAIL][8] ([i915#5334])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/fi-bxt-dsi/igt@i915_selftest@live@gt_heartbeat.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-bxt-dsi/igt@i915_selftest@live@gt_heartbeat.html
* igt@i915_suspend@basic-s3-without-i915:
- fi-rkl-11600: NOTRUN -> [INCOMPLETE][9] ([i915#4817])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html
* igt@kms_chamelium@common-hpd-after-suspend:
- fi-apl-guc: NOTRUN -> [SKIP][10] ([fdo#109271] / [fdo#111827])
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-apl-guc/igt@kms_chamelium@common-hpd-after-suspend.html
* igt@kms_chamelium@hdmi-edid-read:
- fi-rkl-11600: NOTRUN -> [SKIP][11] ([fdo#111827]) +7 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@kms_chamelium@hdmi-edid-read.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
- fi-rkl-11600: NOTRUN -> [SKIP][12] ([i915#4103])
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html
* igt@kms_force_connector_basic@force-load-detect:
- fi-rkl-11600: NOTRUN -> [SKIP][13] ([fdo#109285] / [i915#4098])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@kms_force_connector_basic@force-load-detect.html
* igt@kms_psr@primary_page_flip:
- fi-rkl-11600: NOTRUN -> [SKIP][14] ([i915#1072]) +3 similar issues
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@kms_psr@primary_page_flip.html
* igt@kms_setmode@basic-clone-single-crtc:
- fi-rkl-11600: NOTRUN -> [SKIP][15] ([i915#3555] / [i915#4098])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@kms_setmode@basic-clone-single-crtc.html
* igt@prime_vgem@basic-read:
- fi-rkl-11600: NOTRUN -> [SKIP][16] ([fdo#109295] / [i915#3291] / [i915#3708]) +2 similar issues
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@prime_vgem@basic-read.html
* igt@prime_vgem@basic-userptr:
- fi-rkl-11600: NOTRUN -> [SKIP][17] ([fdo#109295] / [i915#3301] / [i915#3708])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-rkl-11600/igt@prime_vgem@basic-userptr.html
#### Possible fixes ####
* igt@core_hotunplug@unbind-rebind:
- fi-apl-guc: [INCOMPLETE][18] ([i915#7073]) -> [PASS][19]
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/fi-apl-guc/igt@core_hotunplug@unbind-rebind.html
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-apl-guc/igt@core_hotunplug@unbind-rebind.html
* igt@i915_pm_rpm@module-reload:
- {bat-rpls-2}: [WARN][20] ([i915#7346]) -> [PASS][21]
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/bat-rpls-2/igt@i915_pm_rpm@module-reload.html
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/bat-rpls-2/igt@i915_pm_rpm@module-reload.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size:
- fi-bsw-kefka: [FAIL][22] ([i915#6298]) -> [PASS][23]
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.html
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size.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#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
[i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
[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#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
[i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
[i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
[i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
[i915#7073]: https://gitlab.freedesktop.org/drm/intel/issues/7073
[i915#7346]: https://gitlab.freedesktop.org/drm/intel/issues/7346
[i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
[i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
Build changes
-------------
* Linux: CI_DRM_12398 -> Patchwork_110964v2
CI-20190529: 20190529
CI_DRM_12398: 6ff9396457d55a1915566b11121e8fe6f9068b1c @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7068: 5c0ec905b6bbecfb8df8b8f3315d0470539e6ae3 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_110964v2: 6ff9396457d55a1915566b11121e8fe6f9068b1c @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
65510db8f33c drm/i915: Never return 0 if not all requests retired
d4f26d1d216d drm/i915: Fix negative value passed as remaining time
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/index.html
[-- Attachment #2: Type: text/html, Size: 9522 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-18 10:42 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-18 19:56 ` Das, Nirmoy
-1 siblings, 0 replies; 24+ messages in thread
From: Das, Nirmoy @ 2022-11-18 19:56 UTC (permalink / raw)
To: Janusz Krzysztofik, Tvrtko Ursulin, Joonas Lahtinen
Cc: Matthew Brost, intel-gfx, dri-devel, Chris Wilson, Andrzej Hajda,
John Harrison, Nirmoy Das
On 11/18/2022 11:42 AM, 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.
>
> 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>
> Cc: stable@vger.kernel.org # v5.5+
> ---
> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> index edb881d756309..3ac4603eeb4ee 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
> if (remaining_timeout)
> *remaining_timeout = timeout;
>
> + if (!timeout)
> + timeout = -ETIME;
This will return error, -ETIME when 0 timeout is passed,
intel_gt_retire_requests().
We don't want that. I think you can use a separate variable to store
return val from the dma_fence_wait_timeout()
Regards,
Nirmoy
> +
> return active_count ? timeout : 0;
> }
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-18 19:56 ` Das, Nirmoy
0 siblings, 0 replies; 24+ messages in thread
From: Das, Nirmoy @ 2022-11-18 19:56 UTC (permalink / raw)
To: Janusz Krzysztofik, Tvrtko Ursulin, Joonas Lahtinen
Cc: intel-gfx, dri-devel, Chris Wilson, Andrzej Hajda, Nirmoy Das
On 11/18/2022 11:42 AM, 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.
>
> 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>
> Cc: stable@vger.kernel.org # v5.5+
> ---
> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> index edb881d756309..3ac4603eeb4ee 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
> if (remaining_timeout)
> *remaining_timeout = timeout;
>
> + if (!timeout)
> + timeout = -ETIME;
This will return error, -ETIME when 0 timeout is passed,
intel_gt_retire_requests().
We don't want that. I think you can use a separate variable to store
return val from the dma_fence_wait_timeout()
Regards,
Nirmoy
> +
> return active_count ? timeout : 0;
> }
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915: Fix timeout handling when retiring requests (rev2)
2022-11-18 10:42 ` [Intel-gfx] " Janusz Krzysztofik
` (4 preceding siblings ...)
(?)
@ 2022-11-19 1:20 ` Patchwork
-1 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2022-11-19 1:20 UTC (permalink / raw)
To: Janusz Krzysztofik; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 31804 bytes --]
== Series Details ==
Series: drm/i915: Fix timeout handling when retiring requests (rev2)
URL : https://patchwork.freedesktop.org/series/110964/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_12398_full -> Patchwork_110964v2_full
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_110964v2_full absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_110964v2_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
Participating hosts (9 -> 11)
------------------------------
Additional (2): shard-rkl shard-dg1
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_110964v2_full:
### IGT changes ###
#### Possible regressions ####
* igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilegen12rcccs-upscaling@pipe-a-valid-mode:
- shard-tglb: [PASS][1] -> [INCOMPLETE][2]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-tglb7/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilegen12rcccs-upscaling@pipe-a-valid-mode.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-tglb6/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytilegen12rcccs-upscaling@pipe-a-valid-mode.html
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* {igt@i915_pm_backlight@fade-with-suspend}:
- {shard-rkl}: NOTRUN -> [SKIP][3] +2 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-rkl-5/igt@i915_pm_backlight@fade-with-suspend.html
- {shard-dg1}: NOTRUN -> [SKIP][4] +2 similar issues
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-dg1-18/igt@i915_pm_backlight@fade-with-suspend.html
Known issues
------------
Here are the changes found in Patchwork_110964v2_full that come from known issues:
### CI changes ###
#### Issues hit ####
* boot:
- shard-glk: ([PASS][5], [PASS][6], [PASS][7], [PASS][8], [PASS][9], [PASS][10], [PASS][11], [PASS][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], [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], [FAIL][51], [PASS][52], [PASS][53], [PASS][54]) ([i915#4392])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk5/boot.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk5/boot.html
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk5/boot.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk6/boot.html
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk6/boot.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk6/boot.html
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk7/boot.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk7/boot.html
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk7/boot.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk8/boot.html
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk8/boot.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk8/boot.html
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk9/boot.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk9/boot.html
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk9/boot.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk1/boot.html
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk1/boot.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk1/boot.html
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk1/boot.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk2/boot.html
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk2/boot.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk2/boot.html
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk3/boot.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk3/boot.html
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk3/boot.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk5/boot.html
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk3/boot.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk9/boot.html
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk9/boot.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk8/boot.html
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk8/boot.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk1/boot.html
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk1/boot.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk8/boot.html
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk1/boot.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk1/boot.html
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk7/boot.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk2/boot.html
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk2/boot.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk7/boot.html
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk2/boot.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk3/boot.html
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk7/boot.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk6/boot.html
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk6/boot.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk6/boot.html
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk3/boot.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk5/boot.html
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk5/boot.html
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk3/boot.html
### IGT changes ###
#### Issues hit ####
* igt@gem_ctx_exec@basic-nohangcheck:
- shard-tglb: [PASS][55] -> [FAIL][56] ([i915#6268])
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-tglb8/igt@gem_ctx_exec@basic-nohangcheck.html
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-tglb1/igt@gem_ctx_exec@basic-nohangcheck.html
* igt@gem_exec_balancer@parallel-balancer:
- shard-iclb: [PASS][57] -> [SKIP][58] ([i915#4525])
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb1/igt@gem_exec_balancer@parallel-balancer.html
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb5/igt@gem_exec_balancer@parallel-balancer.html
* igt@gem_exec_fair@basic-none-share@rcs0:
- shard-tglb: [PASS][59] -> [FAIL][60] ([i915#2842])
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-tglb6/igt@gem_exec_fair@basic-none-share@rcs0.html
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-tglb5/igt@gem_exec_fair@basic-none-share@rcs0.html
* igt@gem_exec_fair@basic-none@vecs0:
- shard-glk: [PASS][61] -> [FAIL][62] ([i915#2842])
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-glk3/igt@gem_exec_fair@basic-none@vecs0.html
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk1/igt@gem_exec_fair@basic-none@vecs0.html
* igt@gem_exec_suspend@basic-s4-devices@smem:
- shard-skl: NOTRUN -> [SKIP][63] ([fdo#109271]) +1 similar issue
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-skl10/igt@gem_exec_suspend@basic-s4-devices@smem.html
* igt@i915_pm_rc6_residency@rc6-idle@vcs0:
- shard-skl: [PASS][64] -> [WARN][65] ([i915#1804])
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-skl10/igt@i915_pm_rc6_residency@rc6-idle@vcs0.html
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-skl7/igt@i915_pm_rc6_residency@rc6-idle@vcs0.html
* igt@kms_dither@fb-8bpc-vs-panel-6bpc@pipe-a-hdmi-a-1:
- shard-glk: NOTRUN -> [SKIP][66] ([fdo#109271])
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-glk6/igt@kms_dither@fb-8bpc-vs-panel-6bpc@pipe-a-hdmi-a-1.html
* igt@kms_flip@flip-vs-blocking-wf-vblank@a-edp1:
- shard-skl: [PASS][67] -> [FAIL][68] ([i915#2122]) +1 similar issue
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-skl9/igt@kms_flip@flip-vs-blocking-wf-vblank@a-edp1.html
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-skl4/igt@kms_flip@flip-vs-blocking-wf-vblank@a-edp1.html
* igt@kms_flip@flip-vs-expired-vblank@a-edp1:
- shard-skl: [PASS][69] -> [FAIL][70] ([i915#79])
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-skl7/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-skl10/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html
- shard-iclb: [PASS][71] -> [FAIL][72] ([i915#79])
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb1/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb5/igt@kms_flip@flip-vs-expired-vblank@a-edp1.html
* igt@kms_flip_scaled_crc@flip-32bpp-4tile-to-32bpp-4tiledg2rcccs-upscaling@pipe-a-valid-mode:
- shard-iclb: NOTRUN -> [SKIP][73] ([i915#2587] / [i915#2672])
[73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb1/igt@kms_flip_scaled_crc@flip-32bpp-4tile-to-32bpp-4tiledg2rcccs-upscaling@pipe-a-valid-mode.html
* igt@kms_flip_scaled_crc@flip-64bpp-4tile-to-32bpp-4tiledg2rcccs-downscaling@pipe-a-default-mode:
- shard-iclb: NOTRUN -> [SKIP][74] ([i915#2672])
[74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb2/igt@kms_flip_scaled_crc@flip-64bpp-4tile-to-32bpp-4tiledg2rcccs-downscaling@pipe-a-default-mode.html
* igt@kms_flip_scaled_crc@flip-64bpp-linear-to-16bpp-linear-downscaling@pipe-a-default-mode:
- shard-iclb: [PASS][75] -> [SKIP][76] ([i915#3555])
[75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb3/igt@kms_flip_scaled_crc@flip-64bpp-linear-to-16bpp-linear-downscaling@pipe-a-default-mode.html
[76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb2/igt@kms_flip_scaled_crc@flip-64bpp-linear-to-16bpp-linear-downscaling@pipe-a-default-mode.html
* igt@kms_frontbuffer_tracking@fbc-shrfb-scaledprimary:
- shard-iclb: [PASS][77] -> [FAIL][78] ([i915#2546])
[77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-shrfb-scaledprimary.html
[78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb2/igt@kms_frontbuffer_tracking@fbc-shrfb-scaledprimary.html
* igt@kms_plane_scaling@plane-downscale-with-pixel-format-factor-0-75:
- shard-snb: NOTRUN -> [SKIP][79] ([fdo#109271])
[79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-snb6/igt@kms_plane_scaling@plane-downscale-with-pixel-format-factor-0-75.html
* igt@kms_psr2_su@page_flip-xrgb8888:
- shard-iclb: NOTRUN -> [SKIP][80] ([fdo#109642] / [fdo#111068] / [i915#658])
[80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb3/igt@kms_psr2_su@page_flip-xrgb8888.html
* igt@kms_psr@psr2_sprite_plane_onoff:
- shard-iclb: [PASS][81] -> [SKIP][82] ([fdo#109441]) +1 similar issue
[81]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb2/igt@kms_psr@psr2_sprite_plane_onoff.html
[82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb3/igt@kms_psr@psr2_sprite_plane_onoff.html
* igt@kms_psr_stress_test@flip-primary-invalidate-overlay:
- shard-tglb: [PASS][83] -> [SKIP][84] ([i915#5519])
[83]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-tglb6/igt@kms_psr_stress_test@flip-primary-invalidate-overlay.html
[84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-tglb8/igt@kms_psr_stress_test@flip-primary-invalidate-overlay.html
* igt@perf_pmu@module-unload:
- shard-skl: [PASS][85] -> [DMESG-WARN][86] ([i915#1982]) +1 similar issue
[85]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-skl6/igt@perf_pmu@module-unload.html
[86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-skl9/igt@perf_pmu@module-unload.html
#### Possible fixes ####
* igt@gem_exec_balancer@parallel-bb-first:
- shard-iclb: [SKIP][87] ([i915#4525]) -> [PASS][88] +2 similar issues
[87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb6/igt@gem_exec_balancer@parallel-bb-first.html
[88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb1/igt@gem_exec_balancer@parallel-bb-first.html
* igt@gem_exec_suspend@basic-s3@smem:
- shard-skl: [INCOMPLETE][89] ([i915#6179]) -> [PASS][90]
[89]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-skl9/igt@gem_exec_suspend@basic-s3@smem.html
[90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-skl1/igt@gem_exec_suspend@basic-s3@smem.html
* igt@i915_pm_dc@dc6-dpms:
- shard-iclb: [FAIL][91] ([i915#3989] / [i915#454]) -> [PASS][92]
[91]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb3/igt@i915_pm_dc@dc6-dpms.html
[92]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb2/igt@i915_pm_dc@dc6-dpms.html
* igt@i915_pm_sseu@full-enable:
- shard-skl: [FAIL][93] ([i915#7084]) -> [PASS][94]
[93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-skl7/igt@i915_pm_sseu@full-enable.html
[94]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-skl10/igt@i915_pm_sseu@full-enable.html
* igt@kms_flip@plain-flip-fb-recreate-interruptible@b-edp1:
- shard-skl: [FAIL][95] ([i915#2122]) -> [PASS][96] +2 similar issues
[95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-skl1/igt@kms_flip@plain-flip-fb-recreate-interruptible@b-edp1.html
[96]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-skl6/igt@kms_flip@plain-flip-fb-recreate-interruptible@b-edp1.html
* igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-pixel-formats@pipe-b-edp-1:
- shard-iclb: [SKIP][97] ([i915#5176]) -> [PASS][98] +1 similar issue
[97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb3/igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-pixel-formats@pipe-b-edp-1.html
[98]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb1/igt@kms_plane_scaling@plane-scaler-with-clipping-clamping-pixel-formats@pipe-b-edp-1.html
* igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-5@pipe-a-edp-1:
- shard-iclb: [SKIP][99] ([i915#5235]) -> [PASS][100] +2 similar issues
[99]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb2/igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-5@pipe-a-edp-1.html
[100]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb3/igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-5@pipe-a-edp-1.html
* igt@kms_psr@psr2_sprite_mmap_gtt:
- shard-iclb: [SKIP][101] ([fdo#109441]) -> [PASS][102]
[101]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb3/igt@kms_psr@psr2_sprite_mmap_gtt.html
[102]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb2/igt@kms_psr@psr2_sprite_mmap_gtt.html
* igt@kms_psr_stress_test@invalidate-primary-flip-overlay:
- shard-tglb: [SKIP][103] ([i915#5519]) -> [PASS][104]
[103]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-tglb7/igt@kms_psr_stress_test@invalidate-primary-flip-overlay.html
[104]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-tglb3/igt@kms_psr_stress_test@invalidate-primary-flip-overlay.html
* igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
- shard-snb: [SKIP][105] ([fdo#109271]) -> [PASS][106]
[105]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-snb5/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
[106]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-snb7/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
#### Warnings ####
* igt@gem_pread@exhaustion:
- shard-apl: [WARN][107] ([i915#2658]) -> [INCOMPLETE][108] ([i915#7248])
[107]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-apl7/igt@gem_pread@exhaustion.html
[108]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-apl7/igt@gem_pread@exhaustion.html
* igt@kms_plane_alpha_blend@alpha-basic@pipe-c-dp-1:
- shard-apl: [DMESG-FAIL][109] ([IGT#6]) -> [FAIL][110] ([i915#4573]) +1 similar issue
[109]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-apl3/igt@kms_plane_alpha_blend@alpha-basic@pipe-c-dp-1.html
[110]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-apl6/igt@kms_plane_alpha_blend@alpha-basic@pipe-c-dp-1.html
* igt@kms_psr2_sf@cursor-plane-update-sf:
- shard-iclb: [SKIP][111] ([fdo#111068] / [i915#658]) -> [SKIP][112] ([i915#2920])
[111]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb8/igt@kms_psr2_sf@cursor-plane-update-sf.html
[112]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/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][113] ([i915#658]) -> [SKIP][114] ([i915#2920])
[113]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb3/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-fully-sf.html
[114]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb2/igt@kms_psr2_sf@overlay-plane-move-continuous-exceed-fully-sf.html
* igt@kms_psr2_sf@primary-plane-update-sf-dmg-area:
- shard-iclb: [SKIP][115] ([i915#2920]) -> [SKIP][116] ([fdo#111068] / [i915#658])
[115]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12398/shard-iclb2/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area.html
[116]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_110964v2/shard-iclb3/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area.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#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279
[fdo#109283]: https://bugs.freedesktop.org/show_bug.cgi?id=109283
[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#109312]: https://bugs.freedesktop.org/show_bug.cgi?id=109312
[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#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
[fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
[fdo#110542]: https://bugs.freedesktop.org/show_bug.cgi?id=110542
[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#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#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#1722]: https://gitlab.freedesktop.org/drm/intel/issues/1722
[i915#1769]: https://gitlab.freedesktop.org/drm/intel/issues/1769
[i915#1804]: https://gitlab.freedesktop.org/drm/intel/issues/1804
[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#1850]: https://gitlab.freedesktop.org/drm/intel/issues/1850
[i915#1902]: https://gitlab.freedesktop.org/drm/intel/issues/1902
[i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
[i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
[i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
[i915#2433]: https://gitlab.freedesktop.org/drm/intel/issues/2433
[i915#2436]: https://gitlab.freedesktop.org/drm/intel/issues/2436
[i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
[i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
[i915#2546]: https://gitlab.freedesktop.org/drm/intel/issues/2546
[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#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#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#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#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
[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#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
[i915#3469]: https://gitlab.freedesktop.org/drm/intel/issues/3469
[i915#3528]: https://gitlab.freedesktop.org/drm/intel/issues/3528
[i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
[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#3826]: https://gitlab.freedesktop.org/drm/intel/issues/3826
[i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
[i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
[i915#3936]: https://gitlab.freedesktop.org/drm/intel/issues/3936
[i915#3952]: https://gitlab.freedesktop.org/drm/intel/issues/3952
[i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
[i915#3989]: https://gitlab.freedesktop.org/drm/intel/issues/3989
[i915#4036]: https://gitlab.freedesktop.org/drm/intel/issues/4036
[i915#404]: https://gitlab.freedesktop.org/drm/intel/issues/404
[i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
[i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
[i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
[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#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#4349]: https://gitlab.freedesktop.org/drm/intel/issues/4349
[i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
[i915#4392]: https://gitlab.freedesktop.org/drm/intel/issues/4392
[i915#4525]: https://gitlab.freedesktop.org/drm/intel/issues/4525
[i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
[i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
[i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565
[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#4771]: https://gitlab.freedesktop.org/drm/intel/issues/4771
[i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
[i915#4818]: https://gitlab.freedesktop.org/drm/intel/issues/4818
[i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
[i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
[i915#4854]: https://gitlab.freedesktop.org/drm/intel/issues/4854
[i915#4855]: https://gitlab.freedesktop.org/drm/intel/issues/4855
[i915#4859]: https://gitlab.freedesktop.org/drm/intel/issues/4859
[i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
[i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
[i915#4877]: https://gitlab.freedesktop.org/drm/intel/issues/4877
[i915#4881]: https://gitlab.freedesktop.org/drm/intel/issues/4881
[i915#4958]: https://gitlab.freedesktop.org/drm/intel/issues/4958
[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#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
[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#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
[i915#5461]: https://gitlab.freedesktop.org/drm/intel/issues/5461
[i915#5519]: https://gitlab.freedesktop.org/drm/intel/issues/5519
[i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
[i915#5723]: https://gitlab.freedesktop.org/drm/intel/issues/5723
[i915#5784]: https://gitlab.freedesktop.org/drm/intel/issues/5784
[i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
[i915#6179]: https://gitlab.freedesktop.org/drm/intel/issues/6179
[i915#6227]: https://gitlab.freedesktop.org/drm/intel/issues/6227
[i915#6230]: https://gitlab.freedesktop.org/drm/intel/issues/6230
[i915#6245]: https://gitlab.freedesktop.org/drm/intel/issues/6245
[i915#6248]: https://gitlab.freedesktop.org/drm/intel/issues/6248
[i915#6252]: https://gitlab.freedesktop.org/drm/intel/issues/6252
[i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268
[i915#6301]: https://gitlab.freedesktop.org/drm/intel/issues/6301
[i915#6335]: https://gitlab.freedesktop.org/drm/intel/issues/6335
[i915#6344]: https://gitlab.freedesktop.org/drm/intel/issues/6344
[i915#6412]: https://gitlab.freedesktop.org/drm/intel/issues/6412
[i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433
[i915#6463]: https://gitlab.freedesktop.org/drm/intel/issues/6463
[i915#6493]: https://gitlab.freedesktop.org/drm/intel/issues/6493
[i915#6497]: https://gitlab.freedesktop.org/drm/intel/issues/6497
[i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
[i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
[i915#6590]: https://gitlab.freedesktop.org/drm/intel/issues/6590
[i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
[i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768
[i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
[i915#7084]: https://gitlab.freedesktop.org/drm/intel/issues/7084
[i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
[i915#7178]: https://gitlab.freedesktop.org/drm/intel/issues/7178
[i915#7248]: https://gitlab.freedesktop.org/drm/intel/issues/7248
[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
[i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
Build changes
-------------
* Linux: CI_DRM_12398 -> Patchwork_110964v2
CI-20190529: 20190529
CI_DRM_12398: 6ff9396457d55a1915566b11121e8fe6f9068b1c @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7068: 5c0ec905b6bbecfb8df8b8f3315d0470539e6ae3 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_110964v2: 6ff9396457d55a1915566b11121e8fe6f9068b1c @ 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_110964v2/index.html
[-- Attachment #2: Type: text/html, Size: 25479 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-18 19:56 ` [Intel-gfx] " Das, Nirmoy
@ 2022-11-21 8:30 ` Janusz Krzysztofik
-1 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 8:30 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen, Das, Nirmoy
Cc: Matthew Brost, intel-gfx, dri-devel, Chris Wilson, Andrzej Hajda,
Janusz Krzysztofik, John Harrison, Nirmoy Das
Hi Nimroy,
Thanks for looking at this.
On Friday, 18 November 2022 20:56:50 CET Das, Nirmoy wrote:
>
> On 11/18/2022 11:42 AM, 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.
> >
> > 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>
> > Cc: stable@vger.kernel.org # v5.5+
> > ---
> > drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/
drm/i915/gt/intel_gt_requests.c
> > index edb881d756309..3ac4603eeb4ee 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
> > if (remaining_timeout)
> > *remaining_timeout = timeout;
> >
> > + if (!timeout)
> > + timeout = -ETIME;
>
> This will return error, -ETIME when 0 timeout is passed,
> intel_gt_retire_requests().
Yes, but only when active_count is not 0 after we loop through
timelines->active_list calling retire_requests() on each and counting up
failures in active_count.
> We don't want that.
When 0 timeout is passed to intel_gt_retire_requests(), do we really want it
to return 0 unconditionally, or are we rather interested if those calls to
retire_requests() succeeded?
> I think you can use a separate variable to store
> return val from the dma_fence_wait_timeout()
>
>
> Regards,
>
> Nirmoy
>
> > +
> > return active_count ? timeout : 0;
If active count is 0, we return 0 regardless of timeout value, and that's OK.
However, if active_count is not 0, we shouldn't return 0, I believe, we should
return either remaining time if some left, or error (-ETIME) if not. If you
think I'm wrong, please explain why.
Thanks,
Janusz
> > }
> >
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-21 8:30 ` Janusz Krzysztofik
0 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 8:30 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen, Das, Nirmoy
Cc: intel-gfx, dri-devel, Chris Wilson, Andrzej Hajda, Nirmoy Das
Hi Nimroy,
Thanks for looking at this.
On Friday, 18 November 2022 20:56:50 CET Das, Nirmoy wrote:
>
> On 11/18/2022 11:42 AM, 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.
> >
> > 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>
> > Cc: stable@vger.kernel.org # v5.5+
> > ---
> > drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/
drm/i915/gt/intel_gt_requests.c
> > index edb881d756309..3ac4603eeb4ee 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
> > if (remaining_timeout)
> > *remaining_timeout = timeout;
> >
> > + if (!timeout)
> > + timeout = -ETIME;
>
> This will return error, -ETIME when 0 timeout is passed,
> intel_gt_retire_requests().
Yes, but only when active_count is not 0 after we loop through
timelines->active_list calling retire_requests() on each and counting up
failures in active_count.
> We don't want that.
When 0 timeout is passed to intel_gt_retire_requests(), do we really want it
to return 0 unconditionally, or are we rather interested if those calls to
retire_requests() succeeded?
> I think you can use a separate variable to store
> return val from the dma_fence_wait_timeout()
>
>
> Regards,
>
> Nirmoy
>
> > +
> > return active_count ? timeout : 0;
If active count is 0, we return 0 regardless of timeout value, and that's OK.
However, if active_count is not 0, we shouldn't return 0, I believe, we should
return either remaining time if some left, or error (-ETIME) if not. If you
think I'm wrong, please explain why.
Thanks,
Janusz
> > }
> >
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 1/2] drm/i915: Fix negative value passed as remaining time
2022-11-18 10:42 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-21 8:36 ` Janusz Krzysztofik
-1 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 8:36 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
On Friday, 18 November 2022 11:42:21 CET 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 other than -ETIME is
> passed back via remaininig_timeout, we have no clue on how much of
> the initial timeout might have been left for spending it on waiting for
> GuC to become idle. Then, we have no choice other than fail in that case
> -- do it.
Looking at this again, I think we should ignore those errors, like they have
been already ignored by intel_gt_retire_requests_timeout() returning 0, and
call intel_uc_wait_for_idle() with 0 timeout.
I'll submit new version if you agree.
Thanks,
Janusz
> However, if -ETIME is returned via remaining_timeout then we
> know that no more time has been left. Then, pass 0 timeout value to
> intel_uc_wait_for_idle() to give it a chance to return success if GuC is
> already idle.
>
> 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 | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/
intel_gt.c
> index 0325f071046ca..5d612ba547d23 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -677,8 +677,15 @@ 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 == -ETIME)
> + remaining_timeout = 0;
> + else if (remaining_timeout < 0)
> + return remaining_timeout;
> +
> + return intel_uc_wait_for_idle(>->uc, remaining_timeout);
> }
>
> int intel_gt_init(struct intel_gt *gt)
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: Fix negative value passed as remaining time
@ 2022-11-21 8:36 ` Janusz Krzysztofik
0 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 8:36 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen
Cc: Daniel Vetter, intel-gfx, Chris Wilson, dri-devel, Andrzej Hajda,
Nirmoy Das
On Friday, 18 November 2022 11:42:21 CET 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 other than -ETIME is
> passed back via remaininig_timeout, we have no clue on how much of
> the initial timeout might have been left for spending it on waiting for
> GuC to become idle. Then, we have no choice other than fail in that case
> -- do it.
Looking at this again, I think we should ignore those errors, like they have
been already ignored by intel_gt_retire_requests_timeout() returning 0, and
call intel_uc_wait_for_idle() with 0 timeout.
I'll submit new version if you agree.
Thanks,
Janusz
> However, if -ETIME is returned via remaining_timeout then we
> know that no more time has been left. Then, pass 0 timeout value to
> intel_uc_wait_for_idle() to give it a chance to return success if GuC is
> already idle.
>
> 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 | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/
intel_gt.c
> index 0325f071046ca..5d612ba547d23 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -677,8 +677,15 @@ 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 == -ETIME)
> + remaining_timeout = 0;
> + else if (remaining_timeout < 0)
> + return remaining_timeout;
> +
> + return intel_uc_wait_for_idle(>->uc, remaining_timeout);
> }
>
> int intel_gt_init(struct intel_gt *gt)
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-21 8:30 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-21 10:17 ` Andrzej Hajda
-1 siblings, 0 replies; 24+ messages in thread
From: Andrzej Hajda @ 2022-11-21 10:17 UTC (permalink / raw)
To: Janusz Krzysztofik, Tvrtko Ursulin, Joonas Lahtinen, Das, Nirmoy
Cc: Matthew Brost, intel-gfx, dri-devel, Chris Wilson, John Harrison,
Nirmoy Das
On 21.11.2022 09:30, Janusz Krzysztofik wrote:
> Hi Nimroy,
>
> Thanks for looking at this.
>
> On Friday, 18 November 2022 20:56:50 CET Das, Nirmoy wrote:
>> On 11/18/2022 11:42 AM, 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.
>>>
>>> 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>
>>> Cc: stable@vger.kernel.org # v5.5+
>>> ---
>>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/
> drm/i915/gt/intel_gt_requests.c
>>> index edb881d756309..3ac4603eeb4ee 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>> @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
>>> if (remaining_timeout)
>>> *remaining_timeout = timeout;
>>>
>>> + if (!timeout)
>>> + timeout = -ETIME;
>> This will return error, -ETIME when 0 timeout is passed,
>> intel_gt_retire_requests().
> Yes, but only when active_count is not 0 after we loop through
> timelines->active_list calling retire_requests() on each and counting up
> failures in active_count.
Moving this line just after the call to dma_fence_wait_timeout should
solve the controversy.
Regards
Andrzej
>
>> We don't want that.
> When 0 timeout is passed to intel_gt_retire_requests(), do we really want it
> to return 0 unconditionally, or are we rather interested if those calls to
> retire_requests() succeeded?
>
>> I think you can use a separate variable to store
>> return val from the dma_fence_wait_timeout()
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>>> +
>>> return active_count ? timeout : 0;
> If active count is 0, we return 0 regardless of timeout value, and that's OK.
> However, if active_count is not 0, we shouldn't return 0, I believe, we should
> return either remaining time if some left, or error (-ETIME) if not. If you
> think I'm wrong, please explain why.
>
> Thanks,
> Janusz
>
>>> }
>>>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-21 10:17 ` Andrzej Hajda
0 siblings, 0 replies; 24+ messages in thread
From: Andrzej Hajda @ 2022-11-21 10:17 UTC (permalink / raw)
To: Janusz Krzysztofik, Tvrtko Ursulin, Joonas Lahtinen, Das, Nirmoy
Cc: intel-gfx, dri-devel, Chris Wilson, Nirmoy Das
On 21.11.2022 09:30, Janusz Krzysztofik wrote:
> Hi Nimroy,
>
> Thanks for looking at this.
>
> On Friday, 18 November 2022 20:56:50 CET Das, Nirmoy wrote:
>> On 11/18/2022 11:42 AM, 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.
>>>
>>> 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>
>>> Cc: stable@vger.kernel.org # v5.5+
>>> ---
>>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/
> drm/i915/gt/intel_gt_requests.c
>>> index edb881d756309..3ac4603eeb4ee 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>> @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
>>> if (remaining_timeout)
>>> *remaining_timeout = timeout;
>>>
>>> + if (!timeout)
>>> + timeout = -ETIME;
>> This will return error, -ETIME when 0 timeout is passed,
>> intel_gt_retire_requests().
> Yes, but only when active_count is not 0 after we loop through
> timelines->active_list calling retire_requests() on each and counting up
> failures in active_count.
Moving this line just after the call to dma_fence_wait_timeout should
solve the controversy.
Regards
Andrzej
>
>> We don't want that.
> When 0 timeout is passed to intel_gt_retire_requests(), do we really want it
> to return 0 unconditionally, or are we rather interested if those calls to
> retire_requests() succeeded?
>
>> I think you can use a separate variable to store
>> return val from the dma_fence_wait_timeout()
>>
>>
>> Regards,
>>
>> Nirmoy
>>
>>> +
>>> return active_count ? timeout : 0;
> If active count is 0, we return 0 regardless of timeout value, and that's OK.
> However, if active_count is not 0, we shouldn't return 0, I believe, we should
> return either remaining time if some left, or error (-ETIME) if not. If you
> think I'm wrong, please explain why.
>
> Thanks,
> Janusz
>
>>> }
>>>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-21 10:17 ` [Intel-gfx] " Andrzej Hajda
@ 2022-11-21 10:51 ` Janusz Krzysztofik
-1 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 10:51 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen, Das, Nirmoy, Andrzej Hajda
Cc: Matthew Brost, intel-gfx, dri-devel, Chris Wilson, John Harrison,
Nirmoy Das
Hi Andrzej,
Thanks for your comment.
On Monday, 21 November 2022 11:17:42 CET Andrzej Hajda wrote:
>
> On 21.11.2022 09:30, Janusz Krzysztofik wrote:
> > Hi Nimroy,
> >
> > Thanks for looking at this.
> >
> > On Friday, 18 November 2022 20:56:50 CET Das, Nirmoy wrote:
> >> On 11/18/2022 11:42 AM, 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.
> >>>
> >>> 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>
> >>> Cc: stable@vger.kernel.org # v5.5+
> >>> ---
> >>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
> >>> 1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/
> > drm/i915/gt/intel_gt_requests.c
> >>> index edb881d756309..3ac4603eeb4ee 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>> @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
> >>> if (remaining_timeout)
> >>> *remaining_timeout = timeout;
> >>>
> >>> + if (!timeout)
> >>> + timeout = -ETIME;
> >> This will return error, -ETIME when 0 timeout is passed,
> >> intel_gt_retire_requests().
> > Yes, but only when active_count is not 0 after we loop through
> > timelines->active_list calling retire_requests() on each and counting up
> > failures in active_count.
>
> Moving this line just after the call to dma_fence_wait_timeout should
> solve the controversy.
But that would break our need to pass 0, not -ETIME, to flush_submission() in
case the initial value of timeout was 0, as pointed out by Chris during our
discussion on v2.
Maybe an inline comment above the added lines that explains why we are doing
this could help?
Thanks,
Janusz
>
> Regards
> Andrzej
>
> >
> >> We don't want that.
> > When 0 timeout is passed to intel_gt_retire_requests(), do we really want it
> > to return 0 unconditionally, or are we rather interested if those calls to
> > retire_requests() succeeded?
> >
> >> I think you can use a separate variable to store
> >> return val from the dma_fence_wait_timeout()
> >>
> >>
> >> Regards,
> >>
> >> Nirmoy
> >>
> >>> +
> >>> return active_count ? timeout : 0;
> > If active count is 0, we return 0 regardless of timeout value, and that's OK.
> > However, if active_count is not 0, we shouldn't return 0, I believe, we should
> > return either remaining time if some left, or error (-ETIME) if not. If you
> > think I'm wrong, please explain why.
> >
> > Thanks,
> > Janusz
> >
> >>> }
> >>>
> >
> >
> >
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-21 10:51 ` Janusz Krzysztofik
0 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 10:51 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen, Das, Nirmoy, Andrzej Hajda
Cc: intel-gfx, dri-devel, Chris Wilson, Nirmoy Das
Hi Andrzej,
Thanks for your comment.
On Monday, 21 November 2022 11:17:42 CET Andrzej Hajda wrote:
>
> On 21.11.2022 09:30, Janusz Krzysztofik wrote:
> > Hi Nimroy,
> >
> > Thanks for looking at this.
> >
> > On Friday, 18 November 2022 20:56:50 CET Das, Nirmoy wrote:
> >> On 11/18/2022 11:42 AM, 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.
> >>>
> >>> 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>
> >>> Cc: stable@vger.kernel.org # v5.5+
> >>> ---
> >>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
> >>> 1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/
> > drm/i915/gt/intel_gt_requests.c
> >>> index edb881d756309..3ac4603eeb4ee 100644
> >>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>> @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
> >>> if (remaining_timeout)
> >>> *remaining_timeout = timeout;
> >>>
> >>> + if (!timeout)
> >>> + timeout = -ETIME;
> >> This will return error, -ETIME when 0 timeout is passed,
> >> intel_gt_retire_requests().
> > Yes, but only when active_count is not 0 after we loop through
> > timelines->active_list calling retire_requests() on each and counting up
> > failures in active_count.
>
> Moving this line just after the call to dma_fence_wait_timeout should
> solve the controversy.
But that would break our need to pass 0, not -ETIME, to flush_submission() in
case the initial value of timeout was 0, as pointed out by Chris during our
discussion on v2.
Maybe an inline comment above the added lines that explains why we are doing
this could help?
Thanks,
Janusz
>
> Regards
> Andrzej
>
> >
> >> We don't want that.
> > When 0 timeout is passed to intel_gt_retire_requests(), do we really want it
> > to return 0 unconditionally, or are we rather interested if those calls to
> > retire_requests() succeeded?
> >
> >> I think you can use a separate variable to store
> >> return val from the dma_fence_wait_timeout()
> >>
> >>
> >> Regards,
> >>
> >> Nirmoy
> >>
> >>> +
> >>> return active_count ? timeout : 0;
> > If active count is 0, we return 0 regardless of timeout value, and that's OK.
> > However, if active_count is not 0, we shouldn't return 0, I believe, we should
> > return either remaining time if some left, or error (-ETIME) if not. If you
> > think I'm wrong, please explain why.
> >
> > Thanks,
> > Janusz
> >
> >>> }
> >>>
> >
> >
> >
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-21 10:51 ` [Intel-gfx] " Janusz Krzysztofik
@ 2022-11-21 10:59 ` Janusz Krzysztofik
-1 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 10:59 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen, Das, Nirmoy, Andrzej Hajda
Cc: Matthew Brost, intel-gfx, dri-devel, Chris Wilson,
Janusz Krzysztofik, John Harrison, Nirmoy Das
On Monday, 21 November 2022 11:51:15 CET Janusz Krzysztofik wrote:
> Hi Andrzej,
>
> Thanks for your comment.
>
> On Monday, 21 November 2022 11:17:42 CET Andrzej Hajda wrote:
> >
> > On 21.11.2022 09:30, Janusz Krzysztofik wrote:
> > > Hi Nimroy,
> > >
> > > Thanks for looking at this.
> > >
> > > On Friday, 18 November 2022 20:56:50 CET Das, Nirmoy wrote:
> > >> On 11/18/2022 11:42 AM, 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.
> > >>>
> > >>> 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>
> > >>> Cc: stable@vger.kernel.org # v5.5+
> > >>> ---
> > >>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
> > >>> 1 file changed, 3 insertions(+)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/
> > > drm/i915/gt/intel_gt_requests.c
> > >>> index edb881d756309..3ac4603eeb4ee 100644
> > >>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > >>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > >>> @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
> > >>> if (remaining_timeout)
> > >>> *remaining_timeout = timeout;
> > >>>
> > >>> + if (!timeout)
> > >>> + timeout = -ETIME;
> > >> This will return error, -ETIME when 0 timeout is passed,
> > >> intel_gt_retire_requests().
> > > Yes, but only when active_count is not 0 after we loop through
> > > timelines->active_list calling retire_requests() on each and counting up
> > > failures in active_count.
> >
> > Moving this line just after the call to dma_fence_wait_timeout should
> > solve the controversy.
>
> But that would break our need to pass 0, not -ETIME, to flush_submission() in
> case the initial value of timeout was 0, as pointed out by Chris during our
> discussion on v2.
>
> Maybe an inline comment above the added lines that explains why we are doing
> this could help?
How about not adding those two lines but modifying the return line instead?
- return active_count ? timeout : 0;
+ return active_count ? timeout ?: -ETIME : 0;
Would that be self explanatory?
Thanks,
Janusz
>
> Thanks,
> Janusz
>
> >
> > Regards
> > Andrzej
> >
> > >
> > >> We don't want that.
> > > When 0 timeout is passed to intel_gt_retire_requests(), do we really want it
> > > to return 0 unconditionally, or are we rather interested if those calls to
> > > retire_requests() succeeded?
> > >
> > >> I think you can use a separate variable to store
> > >> return val from the dma_fence_wait_timeout()
> > >>
> > >>
> > >> Regards,
> > >>
> > >> Nirmoy
> > >>
> > >>> +
> > >>> return active_count ? timeout : 0;
> > > If active count is 0, we return 0 regardless of timeout value, and that's OK.
> > > However, if active_count is not 0, we shouldn't return 0, I believe, we should
> > > return either remaining time if some left, or error (-ETIME) if not. If you
> > > think I'm wrong, please explain why.
> > >
> > > Thanks,
> > > Janusz
> > >
> > >>> }
> > >>>
> > >
> > >
> > >
> >
> >
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-21 10:59 ` Janusz Krzysztofik
0 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 10:59 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen, Das, Nirmoy, Andrzej Hajda
Cc: intel-gfx, dri-devel, Chris Wilson, Nirmoy Das
On Monday, 21 November 2022 11:51:15 CET Janusz Krzysztofik wrote:
> Hi Andrzej,
>
> Thanks for your comment.
>
> On Monday, 21 November 2022 11:17:42 CET Andrzej Hajda wrote:
> >
> > On 21.11.2022 09:30, Janusz Krzysztofik wrote:
> > > Hi Nimroy,
> > >
> > > Thanks for looking at this.
> > >
> > > On Friday, 18 November 2022 20:56:50 CET Das, Nirmoy wrote:
> > >> On 11/18/2022 11:42 AM, 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.
> > >>>
> > >>> 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>
> > >>> Cc: stable@vger.kernel.org # v5.5+
> > >>> ---
> > >>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
> > >>> 1 file changed, 3 insertions(+)
> > >>>
> > >>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/
> > > drm/i915/gt/intel_gt_requests.c
> > >>> index edb881d756309..3ac4603eeb4ee 100644
> > >>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > >>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> > >>> @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
> > >>> if (remaining_timeout)
> > >>> *remaining_timeout = timeout;
> > >>>
> > >>> + if (!timeout)
> > >>> + timeout = -ETIME;
> > >> This will return error, -ETIME when 0 timeout is passed,
> > >> intel_gt_retire_requests().
> > > Yes, but only when active_count is not 0 after we loop through
> > > timelines->active_list calling retire_requests() on each and counting up
> > > failures in active_count.
> >
> > Moving this line just after the call to dma_fence_wait_timeout should
> > solve the controversy.
>
> But that would break our need to pass 0, not -ETIME, to flush_submission() in
> case the initial value of timeout was 0, as pointed out by Chris during our
> discussion on v2.
>
> Maybe an inline comment above the added lines that explains why we are doing
> this could help?
How about not adding those two lines but modifying the return line instead?
- return active_count ? timeout : 0;
+ return active_count ? timeout ?: -ETIME : 0;
Would that be self explanatory?
Thanks,
Janusz
>
> Thanks,
> Janusz
>
> >
> > Regards
> > Andrzej
> >
> > >
> > >> We don't want that.
> > > When 0 timeout is passed to intel_gt_retire_requests(), do we really want it
> > > to return 0 unconditionally, or are we rather interested if those calls to
> > > retire_requests() succeeded?
> > >
> > >> I think you can use a separate variable to store
> > >> return val from the dma_fence_wait_timeout()
> > >>
> > >>
> > >> Regards,
> > >>
> > >> Nirmoy
> > >>
> > >>> +
> > >>> return active_count ? timeout : 0;
> > > If active count is 0, we return 0 regardless of timeout value, and that's OK.
> > > However, if active_count is not 0, we shouldn't return 0, I believe, we should
> > > return either remaining time if some left, or error (-ETIME) if not. If you
> > > think I'm wrong, please explain why.
> > >
> > > Thanks,
> > > Janusz
> > >
> > >>> }
> > >>>
> > >
> > >
> > >
> >
> >
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-21 10:59 ` [Intel-gfx] " Janusz Krzysztofik
(?)
@ 2022-11-21 12:12 ` Andrzej Hajda
2022-11-21 23:13 ` Janusz Krzysztofik
-1 siblings, 1 reply; 24+ messages in thread
From: Andrzej Hajda @ 2022-11-21 12:12 UTC (permalink / raw)
To: Janusz Krzysztofik, Tvrtko Ursulin, Joonas Lahtinen, Das, Nirmoy
Cc: intel-gfx, Chris Wilson, dri-devel, Nirmoy Das
On 21.11.2022 11:59, Janusz Krzysztofik wrote:
> On Monday, 21 November 2022 11:51:15 CET Janusz Krzysztofik wrote:
>> Hi Andrzej,
>>
>> Thanks for your comment.
>>
>> On Monday, 21 November 2022 11:17:42 CET Andrzej Hajda wrote:
>>>
>>> On 21.11.2022 09:30, Janusz Krzysztofik wrote:
>>>> Hi Nimroy,
>>>>
>>>> Thanks for looking at this.
>>>>
>>>> On Friday, 18 November 2022 20:56:50 CET Das, Nirmoy wrote:
>>>>> On 11/18/2022 11:42 AM, 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.
>>>>>>
>>>>>> 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>
>>>>>> Cc: stable@vger.kernel.org # v5.5+
>>>>>> ---
>>>>>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
>>>>>> 1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/
>>>> drm/i915/gt/intel_gt_requests.c
>>>>>> index edb881d756309..3ac4603eeb4ee 100644
>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>>>>>> @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
>>>>>> if (remaining_timeout)
>>>>>> *remaining_timeout = timeout;
>>>>>>
>>>>>> + if (!timeout)
>>>>>> + timeout = -ETIME;
>>>>> This will return error, -ETIME when 0 timeout is passed,
>>>>> intel_gt_retire_requests().
>>>> Yes, but only when active_count is not 0 after we loop through
>>>> timelines->active_list calling retire_requests() on each and counting up
>>>> failures in active_count.
>>>
>>> Moving this line just after the call to dma_fence_wait_timeout should
>>> solve the controversy.
>>
>> But that would break our need to pass 0, not -ETIME, to flush_submission() in
>> case the initial value of timeout was 0, as pointed out by Chris during our
>> discussion on v2.
>>
>> Maybe an inline comment above the added lines that explains why we are doing
>> this could help?
>
> How about not adding those two lines but modifying the return line instead?
>
> - return active_count ? timeout : 0;
> + return active_count ? timeout ?: -ETIME : 0;
Personally I would translate ret value from dma_fence* API ASAP, and
call flush_submission conditionally - to limit coexistence of both APIs.
But this looks correct to me, as well.
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
Regards
Andrzej
>
> Would that be self explanatory?
>
> Thanks,
> Janusz
>
>>
>> Thanks,
>> Janusz
>>
>>>
>>> Regards
>>> Andrzej
>>>
>>>>
>>>>> We don't want that.
>>>> When 0 timeout is passed to intel_gt_retire_requests(), do we really want it
>>>> to return 0 unconditionally, or are we rather interested if those calls to
>>>> retire_requests() succeeded?
>>>>
>>>>> I think you can use a separate variable to store
>>>>> return val from the dma_fence_wait_timeout()
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Nirmoy
>>>>>
>>>>>> +
>>>>>> return active_count ? timeout : 0;
>>>> If active count is 0, we return 0 regardless of timeout value, and that's OK.
>>>> However, if active_count is not 0, we shouldn't return 0, I believe, we should
>>>> return either remaining time if some left, or error (-ETIME) if not. If you
>>>> think I'm wrong, please explain why.
>>>>
>>>> Thanks,
>>>> Janusz
>>>>
>>>>>> }
>>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
2022-11-21 12:12 ` Andrzej Hajda
@ 2022-11-21 23:13 ` Janusz Krzysztofik
0 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 23:13 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen, Das, Nirmoy, Andrzej Hajda
Cc: Janusz Krzysztofik, intel-gfx, Chris Wilson, dri-devel, Nirmoy Das
Hi Andrzej,
Thanks for providing your R-b, however, I'd still like to convince you that my
approach, which you accepted anyway, is better justified than if we updated 0
timeout with -ETIME immediately after returned by dma_fence_wait_timeout().
On Monday, 21 November 2022 13:12:00 CET Andrzej Hajda wrote:
> On 21.11.2022 11:59, Janusz Krzysztofik wrote:
> > On Monday, 21 November 2022 11:51:15 CET Janusz Krzysztofik wrote:
> >> Hi Andrzej,
> >>
> >> Thanks for your comment.
> >>
> >> On Monday, 21 November 2022 11:17:42 CET Andrzej Hajda wrote:
> >>>
> >>> On 21.11.2022 09:30, Janusz Krzysztofik wrote:
> >>>> Hi Nimroy,
> >>>>
> >>>> Thanks for looking at this.
> >>>>
> >>>> On Friday, 18 November 2022 20:56:50 CET Das, Nirmoy wrote:
> >>>>> On 11/18/2022 11:42 AM, 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.
> >>>>>>
> >>>>>> 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>
> >>>>>> Cc: stable@vger.kernel.org # v5.5+
> >>>>>> ---
> >>>>>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
> >>>>>> 1 file changed, 3 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/
> >>>> drm/i915/gt/intel_gt_requests.c
> >>>>>> index edb881d756309..3ac4603eeb4ee 100644
> >>>>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>>>>> @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
> >>>>>> if (remaining_timeout)
> >>>>>> *remaining_timeout = timeout;
> >>>>>>
> >>>>>> + if (!timeout)
> >>>>>> + timeout = -ETIME;
> >>>>> This will return error, -ETIME when 0 timeout is passed,
> >>>>> intel_gt_retire_requests().
> >>>> Yes, but only when active_count is not 0 after we loop through
> >>>> timelines->active_list calling retire_requests() on each and counting up
> >>>> failures in active_count.
> >>>
> >>> Moving this line just after the call to dma_fence_wait_timeout should
> >>> solve the controversy.
> >>
> >> But that would break our need to pass 0, not -ETIME, to flush_submission() in
> >> case the initial value of timeout was 0, as pointed out by Chris during our
> >> discussion on v2.
> >>
> >> Maybe an inline comment above the added lines that explains why we are doing
> >> this could help?
> >
> > How about not adding those two lines but modifying the return line instead?
> >
> > - return active_count ? timeout : 0;
> > + return active_count ? timeout ?: -ETIME : 0;
>
> Personally I would translate ret value from dma_fence* API ASAP,
I think that would suggest we are trying to fix a problematic 0 response from
dma_fence_wait_timeout() on success, while we already agreed with Chris'
opinion that 0 is perfectl OK in that case, and returning 1 should be rather
considered as problematic, since 0 just means success but no time left, and
-ETIME means no success within timeout. That's what had been implemented one
time in our i915_request_wait_timeuout() backend, regardless of any breakage
potentially introduced by later patches.
Then, fixing 0 return value from dma_fence_wait_timeout(), which is OK, is not
what this patch is about. The real problem is inconsistency between our
declared API of i915_retire_reqiests_wait_timeout(), which promises to return
0 on success, and that 0 remaining timeout value from dma_fence_wait_timeout()
that we can potentially return when not all requests have been retired.
That's what the patch is trying to fix, regardless of what that 0 timeout
value can tell us about success or failure of a single call to
dma_fence_wait_timeout(), not even speaking of a case when the function is
called with timeout already equal 0. Focused on success of retire_requests()
rather than dma_fence_wait_timeout(), we generally ignore error codes from the
latter, using them only for skipping next calls to that function, based on an
assumption that no more time has been left.
Then, clearly fixing just our return value in the problematic case of 0 time
left while not all requests have been retired seems the best option to me.
I've added your R-b to my v3 which implements just what you've accepted -- I
hope you don't mind.
Thanks,
Janusz
> and
> call flush_submission conditionally - to limit coexistence of both APIs.
> But this looks correct to me, as well.
>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>
> Regards
> Andrzej
>
> >
> > Would that be self explanatory?
> >
> > Thanks,
> > Janusz
> >
> >>
> >> Thanks,
> >> Janusz
> >>
> >>>
> >>> Regards
> >>> Andrzej
> >>>
> >>>>
> >>>>> We don't want that.
> >>>> When 0 timeout is passed to intel_gt_retire_requests(), do we really want it
> >>>> to return 0 unconditionally, or are we rather interested if those calls to
> >>>> retire_requests() succeeded?
> >>>>
> >>>>> I think you can use a separate variable to store
> >>>>> return val from the dma_fence_wait_timeout()
> >>>>>
> >>>>>
> >>>>> Regards,
> >>>>>
> >>>>> Nirmoy
> >>>>>
> >>>>>> +
> >>>>>> return active_count ? timeout : 0;
> >>>> If active count is 0, we return 0 regardless of timeout value, and that's OK.
> >>>> However, if active_count is not 0, we shouldn't return 0, I believe, we should
> >>>> return either remaining time if some left, or error (-ETIME) if not. If you
> >>>> think I'm wrong, please explain why.
> >>>>
> >>>> Thanks,
> >>>> Janusz
> >>>>
> >>>>>> }
> >>>>>>
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>
> >>
> >
> >
> >
> >
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired
@ 2022-11-21 23:13 ` Janusz Krzysztofik
0 siblings, 0 replies; 24+ messages in thread
From: Janusz Krzysztofik @ 2022-11-21 23:13 UTC (permalink / raw)
To: Tvrtko Ursulin, Joonas Lahtinen, Das, Nirmoy, Andrzej Hajda
Cc: intel-gfx, Chris Wilson, dri-devel, Nirmoy Das
Hi Andrzej,
Thanks for providing your R-b, however, I'd still like to convince you that my
approach, which you accepted anyway, is better justified than if we updated 0
timeout with -ETIME immediately after returned by dma_fence_wait_timeout().
On Monday, 21 November 2022 13:12:00 CET Andrzej Hajda wrote:
> On 21.11.2022 11:59, Janusz Krzysztofik wrote:
> > On Monday, 21 November 2022 11:51:15 CET Janusz Krzysztofik wrote:
> >> Hi Andrzej,
> >>
> >> Thanks for your comment.
> >>
> >> On Monday, 21 November 2022 11:17:42 CET Andrzej Hajda wrote:
> >>>
> >>> On 21.11.2022 09:30, Janusz Krzysztofik wrote:
> >>>> Hi Nimroy,
> >>>>
> >>>> Thanks for looking at this.
> >>>>
> >>>> On Friday, 18 November 2022 20:56:50 CET Das, Nirmoy wrote:
> >>>>> On 11/18/2022 11:42 AM, 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.
> >>>>>>
> >>>>>> 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>
> >>>>>> Cc: stable@vger.kernel.org # v5.5+
> >>>>>> ---
> >>>>>> drivers/gpu/drm/i915/gt/intel_gt_requests.c | 3 +++
> >>>>>> 1 file changed, 3 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/
> >>>> drm/i915/gt/intel_gt_requests.c
> >>>>>> index edb881d756309..3ac4603eeb4ee 100644
> >>>>>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> >>>>>> @@ -199,6 +199,9 @@ out_active: spin_lock(&timelines->lock);
> >>>>>> if (remaining_timeout)
> >>>>>> *remaining_timeout = timeout;
> >>>>>>
> >>>>>> + if (!timeout)
> >>>>>> + timeout = -ETIME;
> >>>>> This will return error, -ETIME when 0 timeout is passed,
> >>>>> intel_gt_retire_requests().
> >>>> Yes, but only when active_count is not 0 after we loop through
> >>>> timelines->active_list calling retire_requests() on each and counting up
> >>>> failures in active_count.
> >>>
> >>> Moving this line just after the call to dma_fence_wait_timeout should
> >>> solve the controversy.
> >>
> >> But that would break our need to pass 0, not -ETIME, to flush_submission() in
> >> case the initial value of timeout was 0, as pointed out by Chris during our
> >> discussion on v2.
> >>
> >> Maybe an inline comment above the added lines that explains why we are doing
> >> this could help?
> >
> > How about not adding those two lines but modifying the return line instead?
> >
> > - return active_count ? timeout : 0;
> > + return active_count ? timeout ?: -ETIME : 0;
>
> Personally I would translate ret value from dma_fence* API ASAP,
I think that would suggest we are trying to fix a problematic 0 response from
dma_fence_wait_timeout() on success, while we already agreed with Chris'
opinion that 0 is perfectl OK in that case, and returning 1 should be rather
considered as problematic, since 0 just means success but no time left, and
-ETIME means no success within timeout. That's what had been implemented one
time in our i915_request_wait_timeuout() backend, regardless of any breakage
potentially introduced by later patches.
Then, fixing 0 return value from dma_fence_wait_timeout(), which is OK, is not
what this patch is about. The real problem is inconsistency between our
declared API of i915_retire_reqiests_wait_timeout(), which promises to return
0 on success, and that 0 remaining timeout value from dma_fence_wait_timeout()
that we can potentially return when not all requests have been retired.
That's what the patch is trying to fix, regardless of what that 0 timeout
value can tell us about success or failure of a single call to
dma_fence_wait_timeout(), not even speaking of a case when the function is
called with timeout already equal 0. Focused on success of retire_requests()
rather than dma_fence_wait_timeout(), we generally ignore error codes from the
latter, using them only for skipping next calls to that function, based on an
assumption that no more time has been left.
Then, clearly fixing just our return value in the problematic case of 0 time
left while not all requests have been retired seems the best option to me.
I've added your R-b to my v3 which implements just what you've accepted -- I
hope you don't mind.
Thanks,
Janusz
> and
> call flush_submission conditionally - to limit coexistence of both APIs.
> But this looks correct to me, as well.
>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>
> Regards
> Andrzej
>
> >
> > Would that be self explanatory?
> >
> > Thanks,
> > Janusz
> >
> >>
> >> Thanks,
> >> Janusz
> >>
> >>>
> >>> Regards
> >>> Andrzej
> >>>
> >>>>
> >>>>> We don't want that.
> >>>> When 0 timeout is passed to intel_gt_retire_requests(), do we really want it
> >>>> to return 0 unconditionally, or are we rather interested if those calls to
> >>>> retire_requests() succeeded?
> >>>>
> >>>>> I think you can use a separate variable to store
> >>>>> return val from the dma_fence_wait_timeout()
> >>>>>
> >>>>>
> >>>>> Regards,
> >>>>>
> >>>>> Nirmoy
> >>>>>
> >>>>>> +
> >>>>>> return active_count ? timeout : 0;
> >>>> If active count is 0, we return 0 regardless of timeout value, and that's OK.
> >>>> However, if active_count is not 0, we shouldn't return 0, I believe, we should
> >>>> return either remaining time if some left, or error (-ETIME) if not. If you
> >>>> think I'm wrong, please explain why.
> >>>>
> >>>> Thanks,
> >>>> Janusz
> >>>>
> >>>>>> }
> >>>>>>
> >>>>
> >>>>
> >>>>
> >>>
> >>>
> >>
> >>
> >
> >
> >
> >
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2022-11-21 23:14 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-18 10:42 [PATCH v2 0/2] drm/i915: Fix timeout handling when retiring requests Janusz Krzysztofik
2022-11-18 10:42 ` [Intel-gfx] " Janusz Krzysztofik
2022-11-18 10:42 ` [PATCH v2 1/2] drm/i915: Fix negative value passed as remaining time Janusz Krzysztofik
2022-11-18 10:42 ` [Intel-gfx] " Janusz Krzysztofik
2022-11-21 8:36 ` Janusz Krzysztofik
2022-11-21 8:36 ` [Intel-gfx] " Janusz Krzysztofik
2022-11-18 10:42 ` [PATCH v2 2/2] drm/i915: Never return 0 if not all requests retired Janusz Krzysztofik
2022-11-18 10:42 ` [Intel-gfx] " Janusz Krzysztofik
2022-11-18 19:56 ` Das, Nirmoy
2022-11-18 19:56 ` [Intel-gfx] " Das, Nirmoy
2022-11-21 8:30 ` Janusz Krzysztofik
2022-11-21 8:30 ` [Intel-gfx] " Janusz Krzysztofik
2022-11-21 10:17 ` Andrzej Hajda
2022-11-21 10:17 ` [Intel-gfx] " Andrzej Hajda
2022-11-21 10:51 ` Janusz Krzysztofik
2022-11-21 10:51 ` [Intel-gfx] " Janusz Krzysztofik
2022-11-21 10:59 ` Janusz Krzysztofik
2022-11-21 10:59 ` [Intel-gfx] " Janusz Krzysztofik
2022-11-21 12:12 ` Andrzej Hajda
2022-11-21 23:13 ` Janusz Krzysztofik
2022-11-21 23:13 ` Janusz Krzysztofik
2022-11-18 11:34 ` [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Fix timeout handling when retiring requests (rev2) Patchwork
2022-11-18 14:15 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2022-11-19 1:20 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " 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.