* [igt-dev] [PATCH i-g-t 0/3] IGT fixes for priority management + preempt timeout with GuC submission
@ 2021-09-16 18:02 Matthew Brost
2021-09-16 18:02 ` [igt-dev] [PATCH i-g-t 1/3] i915/gem_exec_schedule: Make gem_exec_schedule understand static priority mapping Matthew Brost
` (4 more replies)
0 siblings, 5 replies; 24+ messages in thread
From: Matthew Brost @ 2021-09-16 18:02 UTC (permalink / raw)
To: igt-dev; +Cc: john.c.harrison, daniele.ceraolospurio
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Matthew Brost (3):
i915/gem_exec_schedule: Make gem_exec_schedule understand static
priority mapping
i915/gem_ctx_shared: Make gem_ctx_shared understand static priority
mapping
i915/sysfs_preempt_timeout: Update test to work with GuC submission
lib/i915/gem_scheduler.c | 14 ++++++++++
lib/i915/gem_scheduler.h | 1 +
lib/i915/i915_drm_local.h | 10 +++++++
tests/i915/gem_ctx_shared.c | 5 ++--
tests/i915/gem_exec_schedule.c | 43 +++++++++++++++++++-----------
tests/i915/sysfs_preempt_timeout.c | 6 ++++-
6 files changed, 61 insertions(+), 18 deletions(-)
--
2.32.0
^ permalink raw reply [flat|nested] 24+ messages in thread
* [igt-dev] [PATCH i-g-t 1/3] i915/gem_exec_schedule: Make gem_exec_schedule understand static priority mapping
2021-09-16 18:02 [igt-dev] [PATCH i-g-t 0/3] IGT fixes for priority management + preempt timeout with GuC submission Matthew Brost
@ 2021-09-16 18:02 ` Matthew Brost
2021-10-04 23:24 ` Daniele Ceraolo Spurio
2021-09-16 18:03 ` [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared " Matthew Brost
` (3 subsequent siblings)
4 siblings, 1 reply; 24+ messages in thread
From: Matthew Brost @ 2021-09-16 18:02 UTC (permalink / raw)
To: igt-dev; +Cc: john.c.harrison, daniele.ceraolospurio
The i915 currently has 2k visible priority levels which are currently
unique. This is changing to statically map these 2k levels into 3
buckets:
low: < 0
mid: 0
high: > 0
Update gem_exec_schedule to understand this. This entails updating
promotion test to use 3 levels that will map into different buckets and
also add bit of delay after releasing a cork beforing completing the
spinners to give time to the i915 schedule to process the fence and
release and queue the requests.
Also skip any tests that rely on having more than 3 priority levels.
v2: Add a delay between starting releasing spinner and cork in
promotion, add local define for static mapping engine info
v3:
(Daniele)
- Update commit message explaining why delay is needed,
unconditionally add delay
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
lib/i915/gem_scheduler.c | 14 +++++++++++
lib/i915/gem_scheduler.h | 1 +
lib/i915/i915_drm_local.h | 10 ++++++++
tests/i915/gem_exec_schedule.c | 43 ++++++++++++++++++++++------------
4 files changed, 53 insertions(+), 15 deletions(-)
diff --git a/lib/i915/gem_scheduler.c b/lib/i915/gem_scheduler.c
index cdddf42ad..d006b8676 100644
--- a/lib/i915/gem_scheduler.c
+++ b/lib/i915/gem_scheduler.c
@@ -28,6 +28,7 @@
#include "igt_core.h"
#include "ioctl_wrappers.h"
+#include "i915/i915_drm_local.h"
#include "i915/gem_scheduler.h"
#include "i915/gem_submission.h"
@@ -90,6 +91,19 @@ bool gem_scheduler_has_ctx_priority(int fd)
I915_SCHEDULER_CAP_PRIORITY;
}
+/**
+ * gem_scheduler_has_static_priority:
+ * @fd: open i915 drm file descriptor
+ *
+ * Feature test macro to query whether the driver supports priority assigned
+ * from user space are statically mapping into 3 buckets.
+ */
+bool gem_scheduler_has_static_priority(int fd)
+{
+ return gem_scheduler_capability(fd) &
+ I915_SCHEDULER_CAP_STATIC_PRIORITY_MAP;
+}
+
/**
* gem_scheduler_has_preemption:
* @fd: open i915 drm file descriptor
diff --git a/lib/i915/gem_scheduler.h b/lib/i915/gem_scheduler.h
index d43e84bd2..b00804f70 100644
--- a/lib/i915/gem_scheduler.h
+++ b/lib/i915/gem_scheduler.h
@@ -29,6 +29,7 @@
unsigned gem_scheduler_capability(int fd);
bool gem_scheduler_enabled(int fd);
bool gem_scheduler_has_ctx_priority(int fd);
+bool gem_scheduler_has_static_priority(int fd);
bool gem_scheduler_has_preemption(int fd);
bool gem_scheduler_has_semaphores(int fd);
bool gem_scheduler_has_engine_busy_stats(int fd);
diff --git a/lib/i915/i915_drm_local.h b/lib/i915/i915_drm_local.h
index dd646aedf..a1527ff21 100644
--- a/lib/i915/i915_drm_local.h
+++ b/lib/i915/i915_drm_local.h
@@ -20,6 +20,16 @@ extern "C" {
* clean these up when kernel uapi headers are sync'd.
*/
+/*
+ * Indicates the 2k user priority levels are statically mapped into 3 buckets as
+ * follows:
+ *
+ * -1k to -1 Low priority
+ * 0 Normal priority
+ * 1 to 1k Highest priority
+ */
+#define I915_SCHEDULER_CAP_STATIC_PRIORITY_MAP (1ul << 5)
+
#if defined(__cplusplus)
}
#endif
diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
index 1f6f71db0..191eb558f 100644
--- a/tests/i915/gem_exec_schedule.c
+++ b/tests/i915/gem_exec_schedule.c
@@ -247,6 +247,7 @@ static void unplug_show_queue(int fd, struct igt_cork *c,
igt_cork_unplug(c); /* batches will now be queued on the engine */
igt_debugfs_dump(fd, "i915_engine_info");
+ usleep(250000);
for (int n = 0; n < max; n++) {
uint64_t ahnd = spin[n]->ahnd;
@@ -1469,10 +1470,10 @@ static void promotion(int fd, const intel_ctx_cfg_t *cfg, unsigned ring)
gem_context_set_priority(fd, ctx[LO]->id, MIN_PRIO);
ctx[HI] = intel_ctx_create(fd, cfg);
- gem_context_set_priority(fd, ctx[HI]->id, 0);
+ gem_context_set_priority(fd, ctx[HI]->id, MAX_PRIO);
ctx[NOISE] = intel_ctx_create(fd, cfg);
- gem_context_set_priority(fd, ctx[NOISE]->id, MIN_PRIO/2);
+ gem_context_set_priority(fd, ctx[NOISE]->id, 0);
result = gem_create(fd, 4096);
result_offset = get_offset(ahnd, result, 4096, 0);
@@ -3246,19 +3247,25 @@ igt_main
test_each_engine_store("preempt-other-chain", fd, ctx, e)
preempt_other(fd, &ctx->cfg, e->flags, CHAIN);
- test_each_engine_store("preempt-queue", fd, ctx, e)
- preempt_queue(fd, &ctx->cfg, e->flags, 0);
+ test_each_engine_store("preempt-engines", fd, ctx, e)
+ preempt_engines(fd, e, 0);
- test_each_engine_store("preempt-queue-chain", fd, ctx, e)
- preempt_queue(fd, &ctx->cfg, e->flags, CHAIN);
- test_each_engine_store("preempt-queue-contexts", fd, ctx, e)
- preempt_queue(fd, &ctx->cfg, e->flags, CONTEXTS);
+ igt_subtest_group {
+ igt_fixture {
+ igt_require(!gem_scheduler_has_static_priority(fd));
+ }
- test_each_engine_store("preempt-queue-contexts-chain", fd, ctx, e)
- preempt_queue(fd, &ctx->cfg, e->flags, CONTEXTS | CHAIN);
+ test_each_engine_store("preempt-queue", fd, ctx, e)
+ preempt_queue(fd, &ctx->cfg, e->flags, 0);
- test_each_engine_store("preempt-engines", fd, ctx, e)
- preempt_engines(fd, e, 0);
+ test_each_engine_store("preempt-queue-chain", fd, ctx, e)
+ preempt_queue(fd, &ctx->cfg, e->flags, CHAIN);
+ test_each_engine_store("preempt-queue-contexts", fd, ctx, e)
+ preempt_queue(fd, &ctx->cfg, e->flags, CONTEXTS);
+
+ test_each_engine_store("preempt-queue-contexts-chain", fd, ctx, e)
+ preempt_queue(fd, &ctx->cfg, e->flags, CONTEXTS | CHAIN);
+ }
igt_subtest_group {
igt_hang_t hang;
@@ -3300,11 +3307,17 @@ igt_main
test_each_engine_store("wide", fd, ctx, e)
wide(fd, &ctx->cfg, e->flags);
- test_each_engine_store("reorder-wide", fd, ctx, e)
- reorder_wide(fd, &ctx->cfg, e->flags);
-
test_each_engine_store("smoketest", fd, ctx, e)
smoketest(fd, &ctx->cfg, e->flags, 5);
+
+ igt_subtest_group {
+ igt_fixture {
+ igt_require(!gem_scheduler_has_static_priority(fd));
+ }
+
+ test_each_engine_store("reorder-wide", fd, ctx, e)
+ reorder_wide(fd, &ctx->cfg, e->flags);
+ }
}
igt_subtest_group {
--
2.32.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-09-16 18:02 [igt-dev] [PATCH i-g-t 0/3] IGT fixes for priority management + preempt timeout with GuC submission Matthew Brost
2021-09-16 18:02 ` [igt-dev] [PATCH i-g-t 1/3] i915/gem_exec_schedule: Make gem_exec_schedule understand static priority mapping Matthew Brost
@ 2021-09-16 18:03 ` Matthew Brost
2021-10-04 23:26 ` Daniele Ceraolo Spurio
2021-09-16 18:03 ` [igt-dev] [PATCH i-g-t 3/3] i915/sysfs_preempt_timeout: Update test to work with GuC submission Matthew Brost
` (2 subsequent siblings)
4 siblings, 1 reply; 24+ messages in thread
From: Matthew Brost @ 2021-09-16 18:03 UTC (permalink / raw)
To: igt-dev; +Cc: john.c.harrison, daniele.ceraolospurio
The i915 currently has 2k visible priority levels which are currently
unique. This is changing to statically map these 2k levels into 3
buckets:
low: < 0
mid: 0
high: > 0
Update gem_ctx_shared to understand this. This entails updating
promotion test to use 3 levels that will map into different buckets and
also add bit of delay after releasing a cork beforing completing the
spinners to give time to the i915 schedule to process the fence and
release and queue the requests.
v2: Add a delay between starting releasing spinner and cork in
promotion
v3:
(Daniele)
- Always add delay, update commit message
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
tests/i915/gem_ctx_shared.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
index ea1b5dd1b..7f88871b8 100644
--- a/tests/i915/gem_ctx_shared.c
+++ b/tests/i915/gem_ctx_shared.c
@@ -622,6 +622,7 @@ static void unplug_show_queue(int i915, struct igt_cork *c, uint64_t ahnd,
igt_cork_unplug(c); /* batches will now be queued on the engine */
igt_debugfs_dump(i915, "i915_engine_info");
+ usleep(250000);
for (int n = 0; n < ARRAY_SIZE(spin); n++) {
ahnd = spin[n]->ahnd;
@@ -831,10 +832,10 @@ static void promotion(int i915, const intel_ctx_cfg_t *cfg, unsigned ring)
gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
ctx[HI] = intel_ctx_create(i915, &q_cfg);
- gem_context_set_priority(i915, ctx[HI]->id, 0);
+ gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
- gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
+ gem_context_set_priority(i915, ctx[NOISE]->id, 0);
result = gem_create(i915, 4096);
result_offset = get_offset(ahnd, result, 4096, 0);
--
2.32.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [igt-dev] [PATCH i-g-t 3/3] i915/sysfs_preempt_timeout: Update test to work with GuC submission
2021-09-16 18:02 [igt-dev] [PATCH i-g-t 0/3] IGT fixes for priority management + preempt timeout with GuC submission Matthew Brost
2021-09-16 18:02 ` [igt-dev] [PATCH i-g-t 1/3] i915/gem_exec_schedule: Make gem_exec_schedule understand static priority mapping Matthew Brost
2021-09-16 18:03 ` [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared " Matthew Brost
@ 2021-09-16 18:03 ` Matthew Brost
2021-09-16 20:49 ` John Harrison
2021-09-16 19:33 ` [igt-dev] ✓ Fi.CI.BAT: success for IGT fixes for priority management + preempt timeout " Patchwork
2021-09-16 22:07 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
4 siblings, 1 reply; 24+ messages in thread
From: Matthew Brost @ 2021-09-16 18:03 UTC (permalink / raw)
To: igt-dev; +Cc: john.c.harrison, daniele.ceraolospurio
Increase reset timeout as resets can take a bit longer with GuC
submission because an error capture is done and with a large GuC log (16
MB) these take a while.
Don't run 'off' section as with GuC submission we don't handle
dynamically changing the preemption timeout from 'off' to 'on' on a
currently running context. This is not bug in GuC submission rather an
architectural decision to not implement this as there is no user aside
from IGTs. We don't run this section on any gen12+ platforms as we
assume GuC submission on these platforms.
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
tests/i915/sysfs_preempt_timeout.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tests/i915/sysfs_preempt_timeout.c b/tests/i915/sysfs_preempt_timeout.c
index d176ae72e..fe6b30236 100644
--- a/tests/i915/sysfs_preempt_timeout.c
+++ b/tests/i915/sysfs_preempt_timeout.c
@@ -29,6 +29,7 @@
#include <sys/types.h>
#include <unistd.h>
+#include "igt.h"
#include "igt_params.h"
#include "drmtest.h"
#include "i915/gem.h"
@@ -41,7 +42,7 @@
#include "sw_sync.h"
#define ATTR "preempt_timeout_ms"
-#define RESET_TIMEOUT 50 /* milliseconds, at least one jiffie for kworker */
+#define RESET_TIMEOUT 1000 /* milliseconds, at long enough for an error capture */
static bool __enable_hangcheck(int dir, bool state)
{
@@ -254,6 +255,9 @@ static void test_off(int i915, int engine)
gem_quiescent_gpu(i915);
igt_require(enable_hangcheck(i915, false));
+ /* Not a supported behavior for GuC enabled platforms */
+ igt_require(intel_gen(intel_get_drm_devid(i915)) < 12);
+
igt_assert(igt_sysfs_scanf(engine, "class", "%u", &class) == 1);
igt_assert(igt_sysfs_scanf(engine, "instance", "%u", &inst) == 1);
--
2.32.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for IGT fixes for priority management + preempt timeout with GuC submission
2021-09-16 18:02 [igt-dev] [PATCH i-g-t 0/3] IGT fixes for priority management + preempt timeout with GuC submission Matthew Brost
` (2 preceding siblings ...)
2021-09-16 18:03 ` [igt-dev] [PATCH i-g-t 3/3] i915/sysfs_preempt_timeout: Update test to work with GuC submission Matthew Brost
@ 2021-09-16 19:33 ` Patchwork
2021-09-16 22:07 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
4 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2021-09-16 19:33 UTC (permalink / raw)
To: Matthew Brost; +Cc: igt-dev
[-- Attachment #1: Type: text/plain, Size: 8918 bytes --]
== Series Details ==
Series: IGT fixes for priority management + preempt timeout with GuC submission
URL : https://patchwork.freedesktop.org/series/94777/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_10598 -> IGTPW_6234
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/index.html
Known issues
------------
Here are the changes found in IGTPW_6234 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@amdgpu/amd_basic@cs-compute:
- fi-cfl-guc: NOTRUN -> [SKIP][1] ([fdo#109271]) +17 similar issues
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-cfl-guc/igt@amdgpu/amd_basic@cs-compute.html
* igt@amdgpu/amd_basic@cs-gfx:
- fi-rkl-guc: NOTRUN -> [SKIP][2] ([fdo#109315]) +17 similar issues
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-rkl-guc/igt@amdgpu/amd_basic@cs-gfx.html
* igt@amdgpu/amd_basic@semaphore:
- fi-icl-y: NOTRUN -> [SKIP][3] ([fdo#109315]) +17 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-icl-y/igt@amdgpu/amd_basic@semaphore.html
* igt@core_hotunplug@unbind-rebind:
- fi-skl-6700k2: [PASS][4] -> [INCOMPLETE][5] ([i915#4130])
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/fi-skl-6700k2/igt@core_hotunplug@unbind-rebind.html
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-skl-6700k2/igt@core_hotunplug@unbind-rebind.html
- fi-tgl-1115g4: NOTRUN -> [INCOMPLETE][6] ([i915#4130])
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-tgl-1115g4/igt@core_hotunplug@unbind-rebind.html
- fi-rkl-11600: [PASS][7] -> [INCOMPLETE][8] ([i915#4130])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/fi-rkl-11600/igt@core_hotunplug@unbind-rebind.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-rkl-11600/igt@core_hotunplug@unbind-rebind.html
* igt@gem_huc_copy@huc-copy:
- fi-tgl-1115g4: NOTRUN -> [SKIP][9] ([i915#2190])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-tgl-1115g4/igt@gem_huc_copy@huc-copy.html
* igt@i915_module_load@reload:
- fi-icl-u2: NOTRUN -> [INCOMPLETE][10] ([i915#4130] / [i915#4136])
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-icl-u2/igt@i915_module_load@reload.html
- fi-kbl-guc: [PASS][11] -> [INCOMPLETE][12] ([i915#4130] / [i915#4139])
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/fi-kbl-guc/igt@i915_module_load@reload.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-kbl-guc/igt@i915_module_load@reload.html
* igt@i915_pm_backlight@basic-brightness:
- fi-tgl-1115g4: NOTRUN -> [SKIP][13] ([i915#1155])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-tgl-1115g4/igt@i915_pm_backlight@basic-brightness.html
* igt@kms_chamelium@common-hpd-after-suspend:
- fi-tgl-1115g4: NOTRUN -> [SKIP][14] ([fdo#111827]) +8 similar issues
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-tgl-1115g4/igt@kms_chamelium@common-hpd-after-suspend.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- fi-tgl-1115g4: NOTRUN -> [SKIP][15] ([i915#4103]) +1 similar issue
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-tgl-1115g4/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
* igt@kms_force_connector_basic@force-load-detect:
- fi-tgl-1115g4: NOTRUN -> [SKIP][16] ([fdo#109285])
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-tgl-1115g4/igt@kms_force_connector_basic@force-load-detect.html
* igt@kms_psr@primary_mmap_gtt:
- fi-tgl-1115g4: NOTRUN -> [SKIP][17] ([i915#1072]) +3 similar issues
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-tgl-1115g4/igt@kms_psr@primary_mmap_gtt.html
* igt@prime_vgem@basic-userptr:
- fi-tgl-1115g4: NOTRUN -> [SKIP][18] ([i915#3301])
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-tgl-1115g4/igt@prime_vgem@basic-userptr.html
* igt@runner@aborted:
- fi-icl-u2: NOTRUN -> [FAIL][19] ([i915#2426] / [i915#3363] / [i915#3690])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-icl-u2/igt@runner@aborted.html
- fi-cml-u2: NOTRUN -> [FAIL][20] ([i915#2082] / [i915#2426] / [i915#3363])
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-cml-u2/igt@runner@aborted.html
- fi-tgl-1115g4: NOTRUN -> [FAIL][21] ([i915#1602] / [i915#2722])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-tgl-1115g4/igt@runner@aborted.html
#### Possible fixes ####
* igt@core_hotunplug@unbind-rebind:
- fi-rkl-guc: [INCOMPLETE][22] ([i915#4130]) -> [PASS][23]
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/fi-rkl-guc/igt@core_hotunplug@unbind-rebind.html
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-rkl-guc/igt@core_hotunplug@unbind-rebind.html
- fi-cfl-guc: [INCOMPLETE][24] ([i915#4130]) -> [PASS][25]
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/fi-cfl-guc/igt@core_hotunplug@unbind-rebind.html
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-cfl-guc/igt@core_hotunplug@unbind-rebind.html
- fi-icl-u2: [INCOMPLETE][26] ([i915#4130]) -> [PASS][27]
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/fi-icl-u2/igt@core_hotunplug@unbind-rebind.html
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-icl-u2/igt@core_hotunplug@unbind-rebind.html
* igt@i915_module_load@reload:
- fi-icl-y: [INCOMPLETE][28] ([i915#4130]) -> [PASS][29]
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/fi-icl-y/igt@i915_module_load@reload.html
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-icl-y/igt@i915_module_load@reload.html
* igt@kms_chamelium@common-hpd-after-suspend:
- fi-icl-u2: [DMESG-WARN][30] ([i915#2868]) -> [PASS][31]
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/fi-icl-u2/igt@kms_chamelium@common-hpd-after-suspend.html
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-icl-u2/igt@kms_chamelium@common-hpd-after-suspend.html
#### Warnings ####
* igt@i915_module_load@reload:
- fi-cml-u2: [INCOMPLETE][32] ([i915#4136]) -> [INCOMPLETE][33] ([i915#4130] / [i915#4136])
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/fi-cml-u2/igt@i915_module_load@reload.html
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/fi-cml-u2/igt@i915_module_load@reload.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#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
[fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
[i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
[i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
[i915#1602]: https://gitlab.freedesktop.org/drm/intel/issues/1602
[i915#2082]: https://gitlab.freedesktop.org/drm/intel/issues/2082
[i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
[i915#2426]: https://gitlab.freedesktop.org/drm/intel/issues/2426
[i915#2722]: https://gitlab.freedesktop.org/drm/intel/issues/2722
[i915#2868]: https://gitlab.freedesktop.org/drm/intel/issues/2868
[i915#2932]: https://gitlab.freedesktop.org/drm/intel/issues/2932
[i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
[i915#3363]: https://gitlab.freedesktop.org/drm/intel/issues/3363
[i915#3690]: https://gitlab.freedesktop.org/drm/intel/issues/3690
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4130]: https://gitlab.freedesktop.org/drm/intel/issues/4130
[i915#4136]: https://gitlab.freedesktop.org/drm/intel/issues/4136
[i915#4139]: https://gitlab.freedesktop.org/drm/intel/issues/4139
Participating hosts (37 -> 35)
------------------------------
Additional (1): fi-tgl-1115g4
Missing (3): fi-bsw-cyan fi-cfl-8109u fi-bdw-samus
Build changes
-------------
* CI: CI-20190529 -> None
* IGT: IGT_6211 -> IGTPW_6234
CI-20190529: 20190529
CI_DRM_10598: 5b7e720d97fd94cf081daec8f9d09753cfbe1c31 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_6234: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/index.html
IGT_6211: 7b275b3eb17ddf6e7c5b7b9ba359b7f5345a5311 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/index.html
[-- Attachment #2: Type: text/html, Size: 10917 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 3/3] i915/sysfs_preempt_timeout: Update test to work with GuC submission
2021-09-16 18:03 ` [igt-dev] [PATCH i-g-t 3/3] i915/sysfs_preempt_timeout: Update test to work with GuC submission Matthew Brost
@ 2021-09-16 20:49 ` John Harrison
2021-09-17 7:36 ` Tvrtko Ursulin
0 siblings, 1 reply; 24+ messages in thread
From: John Harrison @ 2021-09-16 20:49 UTC (permalink / raw)
To: Matthew Brost, igt-dev; +Cc: daniele.ceraolospurio
On 9/16/2021 11:03, Matthew Brost wrote:
> Increase reset timeout as resets can take a bit longer with GuC
> submission because an error capture is done and with a large GuC log (16
> MB) these take a while.
>
> Don't run 'off' section as with GuC submission we don't handle
> dynamically changing the preemption timeout from 'off' to 'on' on a
> currently running context. This is not bug in GuC submission rather an
> architectural decision to not implement this as there is no user aside
> from IGTs. We don't run this section on any gen12+ platforms as we
> assume GuC submission on these platforms.
>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
> ---
> tests/i915/sysfs_preempt_timeout.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tests/i915/sysfs_preempt_timeout.c b/tests/i915/sysfs_preempt_timeout.c
> index d176ae72e..fe6b30236 100644
> --- a/tests/i915/sysfs_preempt_timeout.c
> +++ b/tests/i915/sysfs_preempt_timeout.c
> @@ -29,6 +29,7 @@
> #include <sys/types.h>
> #include <unistd.h>
>
> +#include "igt.h"
> #include "igt_params.h"
> #include "drmtest.h"
> #include "i915/gem.h"
> @@ -41,7 +42,7 @@
> #include "sw_sync.h"
>
> #define ATTR "preempt_timeout_ms"
> -#define RESET_TIMEOUT 50 /* milliseconds, at least one jiffie for kworker */
> +#define RESET_TIMEOUT 1000 /* milliseconds, at long enough for an error capture */
>
> static bool __enable_hangcheck(int dir, bool state)
> {
> @@ -254,6 +255,9 @@ static void test_off(int i915, int engine)
> gem_quiescent_gpu(i915);
> igt_require(enable_hangcheck(i915, false));
>
> + /* Not a supported behavior for GuC enabled platforms */
> + igt_require(intel_gen(intel_get_drm_devid(i915)) < 12);
> +
> igt_assert(igt_sysfs_scanf(engine, "class", "%u", &class) == 1);
> igt_assert(igt_sysfs_scanf(engine, "instance", "%u", &inst) == 1);
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [igt-dev] ✗ Fi.CI.IGT: failure for IGT fixes for priority management + preempt timeout with GuC submission
2021-09-16 18:02 [igt-dev] [PATCH i-g-t 0/3] IGT fixes for priority management + preempt timeout with GuC submission Matthew Brost
` (3 preceding siblings ...)
2021-09-16 19:33 ` [igt-dev] ✓ Fi.CI.BAT: success for IGT fixes for priority management + preempt timeout " Patchwork
@ 2021-09-16 22:07 ` Patchwork
4 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2021-09-16 22:07 UTC (permalink / raw)
To: Matthew Brost; +Cc: igt-dev
[-- Attachment #1: Type: text/plain, Size: 30289 bytes --]
== Series Details ==
Series: IGT fixes for priority management + preempt timeout with GuC submission
URL : https://patchwork.freedesktop.org/series/94777/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_10598_full -> IGTPW_6234_full
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with IGTPW_6234_full absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in IGTPW_6234_full, 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/IGTPW_6234/index.html
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in IGTPW_6234_full:
### IGT changes ###
#### Possible regressions ####
* igt@i915_selftest@perf:
- shard-tglb: NOTRUN -> [FAIL][1]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb2/igt@i915_selftest@perf.html
* igt@kms_frontbuffer_tracking@fbc-suspend:
- shard-iclb: [PASS][2] -> [INCOMPLETE][3]
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb5/igt@kms_frontbuffer_tracking@fbc-suspend.html
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-suspend.html
Known issues
------------
Here are the changes found in IGTPW_6234_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@feature_discovery@psr2:
- shard-iclb: [PASS][4] -> [SKIP][5] ([i915#658])
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb2/igt@feature_discovery@psr2.html
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb7/igt@feature_discovery@psr2.html
* igt@gem_ctx_sseu@engines:
- shard-tglb: NOTRUN -> [SKIP][6] ([i915#280])
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb1/igt@gem_ctx_sseu@engines.html
* igt@gem_eio@unwedge-stress:
- shard-tglb: [PASS][7] -> [TIMEOUT][8] ([i915#2369] / [i915#3063] / [i915#3648])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-tglb3/igt@gem_eio@unwedge-stress.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb2/igt@gem_eio@unwedge-stress.html
- shard-iclb: [PASS][9] -> [TIMEOUT][10] ([i915#2369] / [i915#2481] / [i915#3070])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb3/igt@gem_eio@unwedge-stress.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb3/igt@gem_eio@unwedge-stress.html
* igt@gem_exec_fair@basic-deadline:
- shard-kbl: NOTRUN -> [FAIL][11] ([i915#2846])
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl3/igt@gem_exec_fair@basic-deadline.html
* igt@gem_exec_fair@basic-none@vcs0:
- shard-kbl: NOTRUN -> [FAIL][12] ([i915#2842]) +3 similar issues
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl1/igt@gem_exec_fair@basic-none@vcs0.html
* igt@gem_exec_fair@basic-pace-solo@rcs0:
- shard-iclb: NOTRUN -> [FAIL][13] ([i915#2842]) +1 similar issue
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb1/igt@gem_exec_fair@basic-pace-solo@rcs0.html
- shard-tglb: NOTRUN -> [FAIL][14] ([i915#2842])
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb6/igt@gem_exec_fair@basic-pace-solo@rcs0.html
* igt@gem_exec_fair@basic-pace@rcs0:
- shard-tglb: [PASS][15] -> [FAIL][16] ([i915#2842]) +1 similar issue
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-tglb3/igt@gem_exec_fair@basic-pace@rcs0.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb6/igt@gem_exec_fair@basic-pace@rcs0.html
* igt@gem_exec_fair@basic-pace@vcs0:
- shard-iclb: [PASS][17] -> [FAIL][18] ([i915#2842])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb8/igt@gem_exec_fair@basic-pace@vcs0.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb6/igt@gem_exec_fair@basic-pace@vcs0.html
* igt@gem_huc_copy@huc-copy:
- shard-kbl: NOTRUN -> [SKIP][19] ([fdo#109271] / [i915#2190])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl7/igt@gem_huc_copy@huc-copy.html
* igt@gem_render_copy@y-tiled-ccs-to-y-tiled-mc-ccs:
- shard-iclb: NOTRUN -> [SKIP][20] ([i915#768]) +2 similar issues
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb8/igt@gem_render_copy@y-tiled-ccs-to-y-tiled-mc-ccs.html
* igt@gem_userptr_blits@create-destroy-unsync:
- shard-tglb: NOTRUN -> [SKIP][21] ([i915#3297])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb2/igt@gem_userptr_blits@create-destroy-unsync.html
- shard-iclb: NOTRUN -> [SKIP][22] ([i915#3297])
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb2/igt@gem_userptr_blits@create-destroy-unsync.html
* igt@gem_workarounds@suspend-resume:
- shard-tglb: [PASS][23] -> [INCOMPLETE][24] ([i915#456])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-tglb3/igt@gem_workarounds@suspend-resume.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb7/igt@gem_workarounds@suspend-resume.html
* igt@gen3_render_mixed_blits:
- shard-tglb: NOTRUN -> [SKIP][25] ([fdo#109289]) +3 similar issues
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb5/igt@gen3_render_mixed_blits.html
* igt@gen3_render_tiledx_blits:
- shard-iclb: NOTRUN -> [SKIP][26] ([fdo#109289]) +5 similar issues
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb2/igt@gen3_render_tiledx_blits.html
* igt@gen9_exec_parse@bb-secure:
- shard-tglb: NOTRUN -> [SKIP][27] ([i915#2856]) +4 similar issues
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb2/igt@gen9_exec_parse@bb-secure.html
* igt@gen9_exec_parse@unaligned-access:
- shard-iclb: NOTRUN -> [SKIP][28] ([i915#2856]) +2 similar issues
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb2/igt@gen9_exec_parse@unaligned-access.html
* igt@i915_pm_dc@dc6-dpms:
- shard-iclb: [PASS][29] -> [FAIL][30] ([i915#454])
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb8/igt@i915_pm_dc@dc6-dpms.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb3/igt@i915_pm_dc@dc6-dpms.html
* igt@i915_pm_rpm@gem-execbuf-stress-pc8:
- shard-iclb: NOTRUN -> [SKIP][31] ([fdo#109293] / [fdo#109506])
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb4/igt@i915_pm_rpm@gem-execbuf-stress-pc8.html
- shard-tglb: NOTRUN -> [SKIP][32] ([fdo#109506] / [i915#2411])
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb8/igt@i915_pm_rpm@gem-execbuf-stress-pc8.html
* igt@i915_pm_rpm@modeset-non-lpsp-stress-no-wait:
- shard-tglb: NOTRUN -> [SKIP][33] ([fdo#111644] / [i915#1397] / [i915#2411])
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb2/igt@i915_pm_rpm@modeset-non-lpsp-stress-no-wait.html
- shard-iclb: NOTRUN -> [SKIP][34] ([fdo#110892])
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb7/igt@i915_pm_rpm@modeset-non-lpsp-stress-no-wait.html
* igt@kms_big_fb@x-tiled-64bpp-rotate-90:
- shard-iclb: NOTRUN -> [SKIP][35] ([fdo#110725] / [fdo#111614])
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb1/igt@kms_big_fb@x-tiled-64bpp-rotate-90.html
* igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip:
- shard-kbl: NOTRUN -> [SKIP][36] ([fdo#109271] / [i915#3777]) +2 similar issues
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl4/igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-0-hflip.html
* igt@kms_big_fb@yf-tiled-64bpp-rotate-180:
- shard-iclb: NOTRUN -> [SKIP][37] ([fdo#110723]) +1 similar issue
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb1/igt@kms_big_fb@yf-tiled-64bpp-rotate-180.html
* igt@kms_big_fb@yf-tiled-max-hw-stride-64bpp-rotate-180-hflip:
- shard-tglb: NOTRUN -> [SKIP][38] ([fdo#111615]) +2 similar issues
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb6/igt@kms_big_fb@yf-tiled-max-hw-stride-64bpp-rotate-180-hflip.html
* igt@kms_ccs@pipe-a-bad-pixel-format-y_tiled_gen12_mc_ccs:
- shard-tglb: NOTRUN -> [SKIP][39] ([i915#3689] / [i915#3886]) +1 similar issue
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb6/igt@kms_ccs@pipe-a-bad-pixel-format-y_tiled_gen12_mc_ccs.html
* igt@kms_ccs@pipe-a-crc-sprite-planes-basic-y_tiled_ccs:
- shard-tglb: NOTRUN -> [SKIP][40] ([i915#3689]) +5 similar issues
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb7/igt@kms_ccs@pipe-a-crc-sprite-planes-basic-y_tiled_ccs.html
* igt@kms_ccs@pipe-a-missing-ccs-buffer-y_tiled_gen12_mc_ccs:
- shard-kbl: NOTRUN -> [SKIP][41] ([fdo#109271] / [i915#3886]) +13 similar issues
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl2/igt@kms_ccs@pipe-a-missing-ccs-buffer-y_tiled_gen12_mc_ccs.html
- shard-iclb: NOTRUN -> [SKIP][42] ([fdo#109278] / [i915#3886]) +4 similar issues
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb8/igt@kms_ccs@pipe-a-missing-ccs-buffer-y_tiled_gen12_mc_ccs.html
* igt@kms_color@pipe-d-ctm-max:
- shard-iclb: NOTRUN -> [SKIP][43] ([fdo#109278] / [i915#1149]) +1 similar issue
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb1/igt@kms_color@pipe-d-ctm-max.html
* igt@kms_color_chamelium@pipe-a-ctm-blue-to-red:
- shard-kbl: NOTRUN -> [SKIP][44] ([fdo#109271] / [fdo#111827]) +31 similar issues
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl7/igt@kms_color_chamelium@pipe-a-ctm-blue-to-red.html
* igt@kms_color_chamelium@pipe-c-ctm-green-to-red:
- shard-iclb: NOTRUN -> [SKIP][45] ([fdo#109284] / [fdo#111827]) +3 similar issues
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb5/igt@kms_color_chamelium@pipe-c-ctm-green-to-red.html
* igt@kms_color_chamelium@pipe-d-ctm-max:
- shard-tglb: NOTRUN -> [SKIP][46] ([fdo#109284] / [fdo#111827]) +2 similar issues
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb3/igt@kms_color_chamelium@pipe-d-ctm-max.html
* igt@kms_color_chamelium@pipe-d-ctm-negative:
- shard-iclb: NOTRUN -> [SKIP][47] ([fdo#109278] / [fdo#109284] / [fdo#111827]) +1 similar issue
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb6/igt@kms_color_chamelium@pipe-d-ctm-negative.html
* igt@kms_content_protection@type1:
- shard-iclb: NOTRUN -> [SKIP][48] ([fdo#109300] / [fdo#111066])
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb1/igt@kms_content_protection@type1.html
* igt@kms_content_protection@uevent:
- shard-kbl: NOTRUN -> [FAIL][49] ([i915#2105])
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl7/igt@kms_content_protection@uevent.html
* igt@kms_cursor_crc@pipe-b-cursor-32x10-random:
- shard-tglb: NOTRUN -> [SKIP][50] ([i915#3359]) +3 similar issues
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb1/igt@kms_cursor_crc@pipe-b-cursor-32x10-random.html
* igt@kms_cursor_crc@pipe-b-cursor-size-change:
- shard-kbl: NOTRUN -> [FAIL][51] ([i915#3444])
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl1/igt@kms_cursor_crc@pipe-b-cursor-size-change.html
- shard-tglb: [PASS][52] -> [FAIL][53] ([i915#2124] / [i915#4024])
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-tglb7/igt@kms_cursor_crc@pipe-b-cursor-size-change.html
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb3/igt@kms_cursor_crc@pipe-b-cursor-size-change.html
- shard-iclb: [PASS][54] -> [FAIL][55] ([i915#3444])
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb7/igt@kms_cursor_crc@pipe-b-cursor-size-change.html
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb5/igt@kms_cursor_crc@pipe-b-cursor-size-change.html
* igt@kms_cursor_crc@pipe-c-cursor-32x32-rapid-movement:
- shard-tglb: NOTRUN -> [SKIP][56] ([i915#3319])
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb5/igt@kms_cursor_crc@pipe-c-cursor-32x32-rapid-movement.html
* igt@kms_cursor_crc@pipe-c-cursor-512x512-random:
- shard-iclb: NOTRUN -> [SKIP][57] ([fdo#109278] / [fdo#109279]) +3 similar issues
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb3/igt@kms_cursor_crc@pipe-c-cursor-512x512-random.html
* igt@kms_cursor_crc@pipe-d-cursor-512x170-random:
- shard-tglb: NOTRUN -> [SKIP][58] ([fdo#109279] / [i915#3359]) +1 similar issue
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb7/igt@kms_cursor_crc@pipe-d-cursor-512x170-random.html
* igt@kms_cursor_crc@pipe-d-cursor-64x21-sliding:
- shard-iclb: NOTRUN -> [SKIP][59] ([fdo#109278]) +12 similar issues
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb5/igt@kms_cursor_crc@pipe-d-cursor-64x21-sliding.html
* igt@kms_cursor_crc@pipe-d-cursor-suspend:
- shard-kbl: NOTRUN -> [SKIP][60] ([fdo#109271]) +306 similar issues
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl2/igt@kms_cursor_crc@pipe-d-cursor-suspend.html
* igt@kms_cursor_legacy@cursora-vs-flipb-atomic:
- shard-tglb: NOTRUN -> [SKIP][61] ([fdo#111825]) +18 similar issues
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb7/igt@kms_cursor_legacy@cursora-vs-flipb-atomic.html
- shard-iclb: NOTRUN -> [SKIP][62] ([fdo#109274] / [fdo#109278]) +1 similar issue
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb6/igt@kms_cursor_legacy@cursora-vs-flipb-atomic.html
* igt@kms_fbcon_fbt@fbc-suspend:
- shard-kbl: NOTRUN -> [INCOMPLETE][63] ([i915#155] / [i915#180] / [i915#636])
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl1/igt@kms_fbcon_fbt@fbc-suspend.html
* igt@kms_flip@2x-flip-vs-rmfb-interruptible:
- shard-iclb: NOTRUN -> [SKIP][64] ([fdo#109274]) +2 similar issues
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb3/igt@kms_flip@2x-flip-vs-rmfb-interruptible.html
* igt@kms_flip@flip-vs-suspend@a-edp1:
- shard-tglb: [PASS][65] -> [INCOMPLETE][66] ([i915#2411] / [i915#456]) +1 similar issue
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-tglb6/igt@kms_flip@flip-vs-suspend@a-edp1.html
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb7/igt@kms_flip@flip-vs-suspend@a-edp1.html
* igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytile:
- shard-iclb: [PASS][67] -> [SKIP][68] ([i915#3701])
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb4/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytile.html
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb2/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-32bpp-ytile.html
* igt@kms_frontbuffer_tracking@fbc-suspend:
- shard-kbl: NOTRUN -> [INCOMPLETE][69] ([i915#155])
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl4/igt@kms_frontbuffer_tracking@fbc-suspend.html
* igt@kms_frontbuffer_tracking@fbcpsr-2p-scndscrn-spr-indfb-draw-pwrite:
- shard-iclb: NOTRUN -> [SKIP][70] ([fdo#109280]) +13 similar issues
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-2p-scndscrn-spr-indfb-draw-pwrite.html
* igt@kms_hdr@bpc-switch-suspend:
- shard-kbl: NOTRUN -> [DMESG-WARN][71] ([i915#180]) +1 similar issue
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl1/igt@kms_hdr@bpc-switch-suspend.html
* igt@kms_pipe_crc_basic@nonblocking-crc-pipe-d-frame-sequence:
- shard-kbl: NOTRUN -> [SKIP][72] ([fdo#109271] / [i915#533]) +2 similar issues
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl6/igt@kms_pipe_crc_basic@nonblocking-crc-pipe-d-frame-sequence.html
* igt@kms_plane@plane-panning-bottom-right-suspend@pipe-b-planes:
- shard-kbl: [PASS][73] -> [DMESG-WARN][74] ([i915#180]) +1 similar issue
[73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl4/igt@kms_plane@plane-panning-bottom-right-suspend@pipe-b-planes.html
[74]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl7/igt@kms_plane@plane-panning-bottom-right-suspend@pipe-b-planes.html
* igt@kms_plane_alpha_blend@pipe-a-alpha-transparent-fb:
- shard-kbl: NOTRUN -> [FAIL][75] ([i915#265])
[75]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl6/igt@kms_plane_alpha_blend@pipe-a-alpha-transparent-fb.html
* igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
- shard-kbl: NOTRUN -> [FAIL][76] ([fdo#108145] / [i915#265]) +1 similar issue
[76]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl6/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max.html
* igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-3:
- shard-kbl: NOTRUN -> [SKIP][77] ([fdo#109271] / [i915#658]) +6 similar issues
[77]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl6/igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-3.html
* igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-4:
- shard-iclb: NOTRUN -> [SKIP][78] ([i915#658])
[78]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb7/igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-4.html
- shard-tglb: NOTRUN -> [SKIP][79] ([i915#2920])
[79]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb2/igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-4.html
* igt@kms_psr2_su@page_flip:
- shard-tglb: NOTRUN -> [SKIP][80] ([i915#1911])
[80]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb6/igt@kms_psr2_su@page_flip.html
- shard-iclb: NOTRUN -> [SKIP][81] ([fdo#109642] / [fdo#111068] / [i915#658])
[81]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb1/igt@kms_psr2_su@page_flip.html
* igt@kms_psr@psr2_basic:
- shard-iclb: [PASS][82] -> [SKIP][83] ([fdo#109441])
[82]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb2/igt@kms_psr@psr2_basic.html
[83]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb1/igt@kms_psr@psr2_basic.html
* igt@kms_psr@psr2_primary_render:
- shard-tglb: NOTRUN -> [FAIL][84] ([i915#132] / [i915#3467])
[84]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb2/igt@kms_psr@psr2_primary_render.html
* igt@kms_psr@psr2_sprite_blt:
- shard-iclb: NOTRUN -> [SKIP][85] ([fdo#109441]) +1 similar issue
[85]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb5/igt@kms_psr@psr2_sprite_blt.html
* igt@kms_vrr@flip-suspend:
- shard-iclb: NOTRUN -> [SKIP][86] ([fdo#109502])
[86]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb4/igt@kms_vrr@flip-suspend.html
* igt@kms_writeback@writeback-fb-id:
- shard-kbl: NOTRUN -> [SKIP][87] ([fdo#109271] / [i915#2437])
[87]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl4/igt@kms_writeback@writeback-fb-id.html
- shard-tglb: NOTRUN -> [SKIP][88] ([i915#2437])
[88]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb2/igt@kms_writeback@writeback-fb-id.html
* igt@nouveau_crc@pipe-a-source-rg:
- shard-iclb: NOTRUN -> [SKIP][89] ([i915#2530])
[89]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb7/igt@nouveau_crc@pipe-a-source-rg.html
* igt@sysfs_clients@sema-25:
- shard-iclb: NOTRUN -> [SKIP][90] ([i915#2994]) +1 similar issue
[90]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb5/igt@sysfs_clients@sema-25.html
- shard-tglb: NOTRUN -> [SKIP][91] ([i915#2994]) +2 similar issues
[91]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb3/igt@sysfs_clients@sema-25.html
* igt@sysfs_clients@sema-50:
- shard-kbl: NOTRUN -> [SKIP][92] ([fdo#109271] / [i915#2994]) +4 similar issues
[92]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl3/igt@sysfs_clients@sema-50.html
#### Possible fixes ####
* igt@device_reset@unbind-reset-rebind:
- shard-kbl: [DMESG-WARN][93] ([i915#4130]) -> [PASS][94]
[93]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl2/igt@device_reset@unbind-reset-rebind.html
[94]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl4/igt@device_reset@unbind-reset-rebind.html
* igt@gem_exec_fair@basic-flow@rcs0:
- shard-tglb: [FAIL][95] ([i915#2842]) -> [PASS][96] +1 similar issue
[95]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-tglb6/igt@gem_exec_fair@basic-flow@rcs0.html
[96]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb6/igt@gem_exec_fair@basic-flow@rcs0.html
* igt@gem_exec_fair@basic-none-share@rcs0:
- shard-iclb: [FAIL][97] ([i915#2842]) -> [PASS][98]
[97]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb2/igt@gem_exec_fair@basic-none-share@rcs0.html
[98]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb5/igt@gem_exec_fair@basic-none-share@rcs0.html
* igt@gem_exec_fair@basic-pace@vcs1:
- shard-kbl: [SKIP][99] ([fdo#109271]) -> [PASS][100]
[99]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl6/igt@gem_exec_fair@basic-pace@vcs1.html
[100]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl6/igt@gem_exec_fair@basic-pace@vcs1.html
* igt@gem_exec_fair@basic-pace@vecs0:
- shard-kbl: [FAIL][101] ([i915#2842]) -> [PASS][102]
[101]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl6/igt@gem_exec_fair@basic-pace@vecs0.html
[102]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl6/igt@gem_exec_fair@basic-pace@vecs0.html
* igt@i915_suspend@debugfs-reader:
- shard-tglb: [INCOMPLETE][103] ([i915#456]) -> [PASS][104] +1 similar issue
[103]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-tglb7/igt@i915_suspend@debugfs-reader.html
[104]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb2/igt@i915_suspend@debugfs-reader.html
* igt@kms_flip@flip-vs-suspend@a-dp1:
- shard-kbl: [DMESG-WARN][105] ([i915#180]) -> [PASS][106] +6 similar issues
[105]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl3/igt@kms_flip@flip-vs-suspend@a-dp1.html
[106]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl3/igt@kms_flip@flip-vs-suspend@a-dp1.html
* igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-16bpp-ytile:
- shard-iclb: [SKIP][107] ([i915#3701]) -> [PASS][108]
[107]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb2/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-16bpp-ytile.html
[108]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb7/igt@kms_flip_scaled_crc@flip-64bpp-ytile-to-16bpp-ytile.html
* igt@kms_psr@psr2_cursor_plane_onoff:
- shard-iclb: [SKIP][109] ([fdo#109441]) -> [PASS][110] +1 similar issue
[109]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb5/igt@kms_psr@psr2_cursor_plane_onoff.html
[110]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb2/igt@kms_psr@psr2_cursor_plane_onoff.html
* igt@perf@polling-parameterized:
- shard-iclb: [FAIL][111] ([i915#1542]) -> [PASS][112]
[111]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb4/igt@perf@polling-parameterized.html
[112]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb5/igt@perf@polling-parameterized.html
- shard-tglb: [FAIL][113] ([i915#1542]) -> [PASS][114]
[113]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-tglb8/igt@perf@polling-parameterized.html
[114]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb3/igt@perf@polling-parameterized.html
* igt@perf@polling-small-buf:
- shard-tglb: [FAIL][115] ([i915#1722]) -> [PASS][116]
[115]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-tglb6/igt@perf@polling-small-buf.html
[116]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb7/igt@perf@polling-small-buf.html
#### Warnings ####
* igt@gem_exec_fair@basic-none-rrul@rcs0:
- shard-iclb: [FAIL][117] ([i915#2852]) -> [FAIL][118] ([i915#2842])
[117]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb3/igt@gem_exec_fair@basic-none-rrul@rcs0.html
[118]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb5/igt@gem_exec_fair@basic-none-rrul@rcs0.html
* igt@i915_pm_dc@dc3co-vpb-simulation:
- shard-iclb: [SKIP][119] ([i915#588]) -> [SKIP][120] ([i915#658])
[119]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb2/igt@i915_pm_dc@dc3co-vpb-simulation.html
[120]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb4/igt@i915_pm_dc@dc3co-vpb-simulation.html
* igt@i915_pm_rc6_residency@rc6-fence:
- shard-iclb: [WARN][121] ([i915#2684]) -> [WARN][122] ([i915#1804] / [i915#2684])
[121]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb2/igt@i915_pm_rc6_residency@rc6-fence.html
[122]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb6/igt@i915_pm_rc6_residency@rc6-fence.html
* igt@i915_pm_rc6_residency@rc6-idle:
- shard-tglb: [FAIL][123] ([i915#2681] / [i915#3591]) -> [WARN][124] ([i915#2681] / [i915#2684])
[123]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-tglb5/igt@i915_pm_rc6_residency@rc6-idle.html
[124]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-tglb3/igt@i915_pm_rc6_residency@rc6-idle.html
- shard-iclb: [FAIL][125] ([i915#2680]) -> [WARN][126] ([i915#2684])
[125]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb7/igt@i915_pm_rc6_residency@rc6-idle.html
[126]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb5/igt@i915_pm_rc6_residency@rc6-idle.html
* igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-2:
- shard-iclb: [SKIP][127] ([i915#658]) -> [SKIP][128] ([i915#2920]) +2 similar issues
[127]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb6/igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-2.html
[128]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb2/igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area-2.html
* igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-2:
- shard-iclb: [SKIP][129] ([i915#2920]) -> [SKIP][130] ([i915#658]) +1 similar issue
[129]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-iclb2/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-2.html
[130]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-iclb3/igt@kms_psr2_sf@primary-plane-update-sf-dmg-area-2.html
* igt@runner@aborted:
- shard-kbl: ([FAIL][131], [FAIL][132], [FAIL][133], [FAIL][134], [FAIL][135], [FAIL][136], [FAIL][137], [FAIL][138], [FAIL][139], [FAIL][140], [FAIL][141], [FAIL][142], [FAIL][143], [FAIL][144], [FAIL][145]) ([i915#1436] / [i915#180] / [i915#1814] / [i915#3002] / [i915#3363] / [i915#602]) -> ([FAIL][146], [FAIL][147], [FAIL][148], [FAIL][149], [FAIL][150], [FAIL][151], [FAIL][152], [FAIL][153], [FAIL][154], [FAIL][155], [FAIL][156]) ([i915#1436] / [i915#180] / [i915#1814] / [i915#3002] / [i915#3363] / [i915#602] / [i915#92])
[131]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl4/igt@runner@aborted.html
[132]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl3/igt@runner@aborted.html
[133]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl3/igt@runner@aborted.html
[134]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl6/igt@runner@aborted.html
[135]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl4/igt@runner@aborted.html
[136]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl1/igt@runner@aborted.html
[137]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl6/igt@runner@aborted.html
[138]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl4/igt@runner@aborted.html
[139]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl6/igt@runner@aborted.html
[140]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl3/igt@runner@aborted.html
[141]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl4/igt@runner@aborted.html
[142]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl7/igt@runner@aborted.html
[143]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl3/igt@runner@aborted.html
[144]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl1/igt@runner@aborted.html
[145]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10598/shard-kbl1/igt@runner@aborted.html
[146]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl7/igt@runner@aborted.html
[147]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl6/igt@runner@aborted.html
[148]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl7/igt@runner@aborted.html
[149]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/shard-kbl7/igt@runner@aborted.html
[150
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6234/index.html
[-- Attachment #2: Type: text/html, Size: 34103 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 3/3] i915/sysfs_preempt_timeout: Update test to work with GuC submission
2021-09-16 20:49 ` John Harrison
@ 2021-09-17 7:36 ` Tvrtko Ursulin
2021-09-17 16:14 ` Matthew Brost
0 siblings, 1 reply; 24+ messages in thread
From: Tvrtko Ursulin @ 2021-09-17 7:36 UTC (permalink / raw)
To: John Harrison, Matthew Brost, igt-dev; +Cc: daniele.ceraolospurio
On 16/09/2021 21:49, John Harrison wrote:
> On 9/16/2021 11:03, Matthew Brost wrote:
>> Increase reset timeout as resets can take a bit longer with GuC
>> submission because an error capture is done and with a large GuC log (16
>> MB) these take a while.
>>
>> Don't run 'off' section as with GuC submission we don't handle
>> dynamically changing the preemption timeout from 'off' to 'on' on a
>> currently running context. This is not bug in GuC submission rather an
>> architectural decision to not implement this as there is no user aside
>> from IGTs. We don't run this section on any gen12+ platforms as we
>> assume GuC submission on these platforms.
Tigerlake as well or not care about removing the coverage from it?
Regards,
Tvrtko
>>
>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
>
>> ---
>> tests/i915/sysfs_preempt_timeout.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/i915/sysfs_preempt_timeout.c
>> b/tests/i915/sysfs_preempt_timeout.c
>> index d176ae72e..fe6b30236 100644
>> --- a/tests/i915/sysfs_preempt_timeout.c
>> +++ b/tests/i915/sysfs_preempt_timeout.c
>> @@ -29,6 +29,7 @@
>> #include <sys/types.h>
>> #include <unistd.h>
>> +#include "igt.h"
>> #include "igt_params.h"
>> #include "drmtest.h"
>> #include "i915/gem.h"
>> @@ -41,7 +42,7 @@
>> #include "sw_sync.h"
>> #define ATTR "preempt_timeout_ms"
>> -#define RESET_TIMEOUT 50 /* milliseconds, at least one jiffie for
>> kworker */
>> +#define RESET_TIMEOUT 1000 /* milliseconds, at long enough for an
>> error capture */
>> static bool __enable_hangcheck(int dir, bool state)
>> {
>> @@ -254,6 +255,9 @@ static void test_off(int i915, int engine)
>> gem_quiescent_gpu(i915);
>> igt_require(enable_hangcheck(i915, false));
>> + /* Not a supported behavior for GuC enabled platforms */
>> + igt_require(intel_gen(intel_get_drm_devid(i915)) < 12);
>> +
>> igt_assert(igt_sysfs_scanf(engine, "class", "%u", &class) == 1);
>> igt_assert(igt_sysfs_scanf(engine, "instance", "%u", &inst) == 1);
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 3/3] i915/sysfs_preempt_timeout: Update test to work with GuC submission
2021-09-17 7:36 ` Tvrtko Ursulin
@ 2021-09-17 16:14 ` Matthew Brost
2021-09-20 9:22 ` Tvrtko Ursulin
0 siblings, 1 reply; 24+ messages in thread
From: Matthew Brost @ 2021-09-17 16:14 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: John Harrison, igt-dev, daniele.ceraolospurio
On Fri, Sep 17, 2021 at 08:36:19AM +0100, Tvrtko Ursulin wrote:
>
> On 16/09/2021 21:49, John Harrison wrote:
> > On 9/16/2021 11:03, Matthew Brost wrote:
> > > Increase reset timeout as resets can take a bit longer with GuC
> > > submission because an error capture is done and with a large GuC log (16
> > > MB) these take a while.
> > >
> > > Don't run 'off' section as with GuC submission we don't handle
> > > dynamically changing the preemption timeout from 'off' to 'on' on a
> > > currently running context. This is not bug in GuC submission rather an
> > > architectural decision to not implement this as there is no user aside
> > > from IGTs. We don't run this section on any gen12+ platforms as we
> > > assume GuC submission on these platforms.
>
> Tigerlake as well or not care about removing the coverage from it?
>
Tigerlake is skipped as well. I don't think we care about removing
coverage as this section isn't really a real world use case.
e.g. I don't a UMD will ever:
Turn off preemption
Create and submit a contet
Turn on preemption and expect an existing context to have the new preemption value
Matt
> Regards,
>
> Tvrtko
>
> > >
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
> >
> > > ---
> > > tests/i915/sysfs_preempt_timeout.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tests/i915/sysfs_preempt_timeout.c
> > > b/tests/i915/sysfs_preempt_timeout.c
> > > index d176ae72e..fe6b30236 100644
> > > --- a/tests/i915/sysfs_preempt_timeout.c
> > > +++ b/tests/i915/sysfs_preempt_timeout.c
> > > @@ -29,6 +29,7 @@
> > > #include <sys/types.h>
> > > #include <unistd.h>
> > > +#include "igt.h"
> > > #include "igt_params.h"
> > > #include "drmtest.h"
> > > #include "i915/gem.h"
> > > @@ -41,7 +42,7 @@
> > > #include "sw_sync.h"
> > > #define ATTR "preempt_timeout_ms"
> > > -#define RESET_TIMEOUT 50 /* milliseconds, at least one jiffie for
> > > kworker */
> > > +#define RESET_TIMEOUT 1000 /* milliseconds, at long enough for an
> > > error capture */
> > > static bool __enable_hangcheck(int dir, bool state)
> > > {
> > > @@ -254,6 +255,9 @@ static void test_off(int i915, int engine)
> > > gem_quiescent_gpu(i915);
> > > igt_require(enable_hangcheck(i915, false));
> > > + /* Not a supported behavior for GuC enabled platforms */
> > > + igt_require(intel_gen(intel_get_drm_devid(i915)) < 12);
> > > +
> > > igt_assert(igt_sysfs_scanf(engine, "class", "%u", &class) == 1);
> > > igt_assert(igt_sysfs_scanf(engine, "instance", "%u", &inst) == 1);
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 3/3] i915/sysfs_preempt_timeout: Update test to work with GuC submission
2021-09-17 16:14 ` Matthew Brost
@ 2021-09-20 9:22 ` Tvrtko Ursulin
0 siblings, 0 replies; 24+ messages in thread
From: Tvrtko Ursulin @ 2021-09-20 9:22 UTC (permalink / raw)
To: Matthew Brost; +Cc: John Harrison, igt-dev, daniele.ceraolospurio
On 17/09/2021 17:14, Matthew Brost wrote:
> On Fri, Sep 17, 2021 at 08:36:19AM +0100, Tvrtko Ursulin wrote:
>>
>> On 16/09/2021 21:49, John Harrison wrote:
>>> On 9/16/2021 11:03, Matthew Brost wrote:
>>>> Increase reset timeout as resets can take a bit longer with GuC
>>>> submission because an error capture is done and with a large GuC log (16
>>>> MB) these take a while.
>>>>
>>>> Don't run 'off' section as with GuC submission we don't handle
>>>> dynamically changing the preemption timeout from 'off' to 'on' on a
>>>> currently running context. This is not bug in GuC submission rather an
>>>> architectural decision to not implement this as there is no user aside
>>>> from IGTs. We don't run this section on any gen12+ platforms as we
>>>> assume GuC submission on these platforms.
>>
>> Tigerlake as well or not care about removing the coverage from it?
>>
>
> Tigerlake is skipped as well. I don't think we care about removing
> coverage as this section isn't really a real world use case.
>
> e.g. I don't a UMD will ever:
>
> Turn off preemption
> Create and submit a contet
> Turn on preemption and expect an existing context to have the new preemption value
I am pedantic against both commit text and code with comment, emphasis mine:
1)
"..._any gen12+_ platforms as we assume _GuC_ submission on these
platforms."
2)
/* Not a supported behavior for _GuC enabled platforms_ */
igt_require(intel_gen(intel_get_drm_devid(i915)) _< 12_);
Regards,
Tvrtko
> Matt
>
>> Regards,
>>
>> Tvrtko
>>
>>>>
>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
>>>
>>>> ---
>>>> tests/i915/sysfs_preempt_timeout.c | 6 +++++-
>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tests/i915/sysfs_preempt_timeout.c
>>>> b/tests/i915/sysfs_preempt_timeout.c
>>>> index d176ae72e..fe6b30236 100644
>>>> --- a/tests/i915/sysfs_preempt_timeout.c
>>>> +++ b/tests/i915/sysfs_preempt_timeout.c
>>>> @@ -29,6 +29,7 @@
>>>> #include <sys/types.h>
>>>> #include <unistd.h>
>>>> +#include "igt.h"
>>>> #include "igt_params.h"
>>>> #include "drmtest.h"
>>>> #include "i915/gem.h"
>>>> @@ -41,7 +42,7 @@
>>>> #include "sw_sync.h"
>>>> #define ATTR "preempt_timeout_ms"
>>>> -#define RESET_TIMEOUT 50 /* milliseconds, at least one jiffie for
>>>> kworker */
>>>> +#define RESET_TIMEOUT 1000 /* milliseconds, at long enough for an
>>>> error capture */
>>>> static bool __enable_hangcheck(int dir, bool state)
>>>> {
>>>> @@ -254,6 +255,9 @@ static void test_off(int i915, int engine)
>>>> gem_quiescent_gpu(i915);
>>>> igt_require(enable_hangcheck(i915, false));
>>>> + /* Not a supported behavior for GuC enabled platforms */
>>>> + igt_require(intel_gen(intel_get_drm_devid(i915)) < 12);
>>>> +
>>>> igt_assert(igt_sysfs_scanf(engine, "class", "%u", &class) == 1);
>>>> igt_assert(igt_sysfs_scanf(engine, "instance", "%u", &inst) == 1);
>>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 1/3] i915/gem_exec_schedule: Make gem_exec_schedule understand static priority mapping
2021-09-16 18:02 ` [igt-dev] [PATCH i-g-t 1/3] i915/gem_exec_schedule: Make gem_exec_schedule understand static priority mapping Matthew Brost
@ 2021-10-04 23:24 ` Daniele Ceraolo Spurio
0 siblings, 0 replies; 24+ messages in thread
From: Daniele Ceraolo Spurio @ 2021-10-04 23:24 UTC (permalink / raw)
To: Matthew Brost, igt-dev; +Cc: john.c.harrison
On 9/16/2021 11:02 AM, Matthew Brost wrote:
> The i915 currently has 2k visible priority levels which are currently
> unique. This is changing to statically map these 2k levels into 3
> buckets:
>
> low: < 0
> mid: 0
> high: > 0
>
> Update gem_exec_schedule to understand this. This entails updating
> promotion test to use 3 levels that will map into different buckets and
> also add bit of delay after releasing a cork beforing completing the
> spinners to give time to the i915 schedule to process the fence and
> release and queue the requests.
>
> Also skip any tests that rely on having more than 3 priority levels.
>
> v2: Add a delay between starting releasing spinner and cork in
> promotion, add local define for static mapping engine info
> v3:
> (Daniele)
> - Update commit message explaining why delay is needed,
> unconditionally add delay
>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
> lib/i915/gem_scheduler.c | 14 +++++++++++
> lib/i915/gem_scheduler.h | 1 +
> lib/i915/i915_drm_local.h | 10 ++++++++
> tests/i915/gem_exec_schedule.c | 43 ++++++++++++++++++++++------------
> 4 files changed, 53 insertions(+), 15 deletions(-)
>
> diff --git a/lib/i915/gem_scheduler.c b/lib/i915/gem_scheduler.c
> index cdddf42ad..d006b8676 100644
> --- a/lib/i915/gem_scheduler.c
> +++ b/lib/i915/gem_scheduler.c
> @@ -28,6 +28,7 @@
> #include "igt_core.h"
> #include "ioctl_wrappers.h"
>
> +#include "i915/i915_drm_local.h"
> #include "i915/gem_scheduler.h"
> #include "i915/gem_submission.h"
>
> @@ -90,6 +91,19 @@ bool gem_scheduler_has_ctx_priority(int fd)
> I915_SCHEDULER_CAP_PRIORITY;
> }
>
> +/**
> + * gem_scheduler_has_static_priority:
> + * @fd: open i915 drm file descriptor
> + *
> + * Feature test macro to query whether the driver supports priority assigned
> + * from user space are statically mapping into 3 buckets.
> + */
> +bool gem_scheduler_has_static_priority(int fd)
> +{
> + return gem_scheduler_capability(fd) &
> + I915_SCHEDULER_CAP_STATIC_PRIORITY_MAP;
> +}
> +
> /**
> * gem_scheduler_has_preemption:
> * @fd: open i915 drm file descriptor
> diff --git a/lib/i915/gem_scheduler.h b/lib/i915/gem_scheduler.h
> index d43e84bd2..b00804f70 100644
> --- a/lib/i915/gem_scheduler.h
> +++ b/lib/i915/gem_scheduler.h
> @@ -29,6 +29,7 @@
> unsigned gem_scheduler_capability(int fd);
> bool gem_scheduler_enabled(int fd);
> bool gem_scheduler_has_ctx_priority(int fd);
> +bool gem_scheduler_has_static_priority(int fd);
> bool gem_scheduler_has_preemption(int fd);
> bool gem_scheduler_has_semaphores(int fd);
> bool gem_scheduler_has_engine_busy_stats(int fd);
> diff --git a/lib/i915/i915_drm_local.h b/lib/i915/i915_drm_local.h
> index dd646aedf..a1527ff21 100644
> --- a/lib/i915/i915_drm_local.h
> +++ b/lib/i915/i915_drm_local.h
> @@ -20,6 +20,16 @@ extern "C" {
> * clean these up when kernel uapi headers are sync'd.
> */
>
> +/*
> + * Indicates the 2k user priority levels are statically mapped into 3 buckets as
> + * follows:
> + *
> + * -1k to -1 Low priority
> + * 0 Normal priority
> + * 1 to 1k Highest priority
> + */
> +#define I915_SCHEDULER_CAP_STATIC_PRIORITY_MAP (1ul << 5)
> +
> #if defined(__cplusplus)
> }
> #endif
> diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
> index 1f6f71db0..191eb558f 100644
> --- a/tests/i915/gem_exec_schedule.c
> +++ b/tests/i915/gem_exec_schedule.c
> @@ -247,6 +247,7 @@ static void unplug_show_queue(int fd, struct igt_cork *c,
>
> igt_cork_unplug(c); /* batches will now be queued on the engine */
> igt_debugfs_dump(fd, "i915_engine_info");
> + usleep(250000);
I'd add a comment here to track why the sleep is needed, something like:
/* give time to the kernel to complete the queueing */
with that:
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Daniele
>
> for (int n = 0; n < max; n++) {
> uint64_t ahnd = spin[n]->ahnd;
> @@ -1469,10 +1470,10 @@ static void promotion(int fd, const intel_ctx_cfg_t *cfg, unsigned ring)
> gem_context_set_priority(fd, ctx[LO]->id, MIN_PRIO);
>
> ctx[HI] = intel_ctx_create(fd, cfg);
> - gem_context_set_priority(fd, ctx[HI]->id, 0);
> + gem_context_set_priority(fd, ctx[HI]->id, MAX_PRIO);
>
> ctx[NOISE] = intel_ctx_create(fd, cfg);
> - gem_context_set_priority(fd, ctx[NOISE]->id, MIN_PRIO/2);
> + gem_context_set_priority(fd, ctx[NOISE]->id, 0);
>
> result = gem_create(fd, 4096);
> result_offset = get_offset(ahnd, result, 4096, 0);
> @@ -3246,19 +3247,25 @@ igt_main
> test_each_engine_store("preempt-other-chain", fd, ctx, e)
> preempt_other(fd, &ctx->cfg, e->flags, CHAIN);
>
> - test_each_engine_store("preempt-queue", fd, ctx, e)
> - preempt_queue(fd, &ctx->cfg, e->flags, 0);
> + test_each_engine_store("preempt-engines", fd, ctx, e)
> + preempt_engines(fd, e, 0);
>
> - test_each_engine_store("preempt-queue-chain", fd, ctx, e)
> - preempt_queue(fd, &ctx->cfg, e->flags, CHAIN);
> - test_each_engine_store("preempt-queue-contexts", fd, ctx, e)
> - preempt_queue(fd, &ctx->cfg, e->flags, CONTEXTS);
> + igt_subtest_group {
> + igt_fixture {
> + igt_require(!gem_scheduler_has_static_priority(fd));
> + }
>
> - test_each_engine_store("preempt-queue-contexts-chain", fd, ctx, e)
> - preempt_queue(fd, &ctx->cfg, e->flags, CONTEXTS | CHAIN);
> + test_each_engine_store("preempt-queue", fd, ctx, e)
> + preempt_queue(fd, &ctx->cfg, e->flags, 0);
>
> - test_each_engine_store("preempt-engines", fd, ctx, e)
> - preempt_engines(fd, e, 0);
> + test_each_engine_store("preempt-queue-chain", fd, ctx, e)
> + preempt_queue(fd, &ctx->cfg, e->flags, CHAIN);
> + test_each_engine_store("preempt-queue-contexts", fd, ctx, e)
> + preempt_queue(fd, &ctx->cfg, e->flags, CONTEXTS);
> +
> + test_each_engine_store("preempt-queue-contexts-chain", fd, ctx, e)
> + preempt_queue(fd, &ctx->cfg, e->flags, CONTEXTS | CHAIN);
> + }
>
> igt_subtest_group {
> igt_hang_t hang;
> @@ -3300,11 +3307,17 @@ igt_main
> test_each_engine_store("wide", fd, ctx, e)
> wide(fd, &ctx->cfg, e->flags);
>
> - test_each_engine_store("reorder-wide", fd, ctx, e)
> - reorder_wide(fd, &ctx->cfg, e->flags);
> -
> test_each_engine_store("smoketest", fd, ctx, e)
> smoketest(fd, &ctx->cfg, e->flags, 5);
> +
> + igt_subtest_group {
> + igt_fixture {
> + igt_require(!gem_scheduler_has_static_priority(fd));
> + }
> +
> + test_each_engine_store("reorder-wide", fd, ctx, e)
> + reorder_wide(fd, &ctx->cfg, e->flags);
> + }
> }
>
> igt_subtest_group {
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-09-16 18:03 ` [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared " Matthew Brost
@ 2021-10-04 23:26 ` Daniele Ceraolo Spurio
2021-10-05 10:44 ` Tvrtko Ursulin
0 siblings, 1 reply; 24+ messages in thread
From: Daniele Ceraolo Spurio @ 2021-10-04 23:26 UTC (permalink / raw)
To: Matthew Brost, igt-dev; +Cc: john.c.harrison
On 9/16/2021 11:03 AM, Matthew Brost wrote:
> The i915 currently has 2k visible priority levels which are currently
> unique. This is changing to statically map these 2k levels into 3
> buckets:
>
> low: < 0
> mid: 0
> high: > 0
>
> Update gem_ctx_shared to understand this. This entails updating
> promotion test to use 3 levels that will map into different buckets and
> also add bit of delay after releasing a cork beforing completing the
> spinners to give time to the i915 schedule to process the fence and
> release and queue the requests.
>
> v2: Add a delay between starting releasing spinner and cork in
> promotion
> v3:
> (Daniele)
> - Always add delay, update commit message
>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
> tests/i915/gem_ctx_shared.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> index ea1b5dd1b..7f88871b8 100644
> --- a/tests/i915/gem_ctx_shared.c
> +++ b/tests/i915/gem_ctx_shared.c
> @@ -622,6 +622,7 @@ static void unplug_show_queue(int i915, struct igt_cork *c, uint64_t ahnd,
>
> igt_cork_unplug(c); /* batches will now be queued on the engine */
> igt_debugfs_dump(i915, "i915_engine_info");
> + usleep(250000);
Same as previous patch, with a comment added:
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Daniele
>
> for (int n = 0; n < ARRAY_SIZE(spin); n++) {
> ahnd = spin[n]->ahnd;
> @@ -831,10 +832,10 @@ static void promotion(int i915, const intel_ctx_cfg_t *cfg, unsigned ring)
> gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
>
> ctx[HI] = intel_ctx_create(i915, &q_cfg);
> - gem_context_set_priority(i915, ctx[HI]->id, 0);
> + gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
>
> ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
> - gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
> + gem_context_set_priority(i915, ctx[NOISE]->id, 0);
>
> result = gem_create(i915, 4096);
> result_offset = get_offset(ahnd, result, 4096, 0);
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-10-04 23:26 ` Daniele Ceraolo Spurio
@ 2021-10-05 10:44 ` Tvrtko Ursulin
2021-10-05 16:44 ` Matthew Brost
0 siblings, 1 reply; 24+ messages in thread
From: Tvrtko Ursulin @ 2021-10-05 10:44 UTC (permalink / raw)
To: Daniele Ceraolo Spurio, Matthew Brost, igt-dev; +Cc: john.c.harrison
On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
>
>
> On 9/16/2021 11:03 AM, Matthew Brost wrote:
>> The i915 currently has 2k visible priority levels which are currently
>> unique. This is changing to statically map these 2k levels into 3
>> buckets:
>>
>> low: < 0
>> mid: 0
>> high: > 0
>>
>> Update gem_ctx_shared to understand this. This entails updating
>> promotion test to use 3 levels that will map into different buckets and
>> also add bit of delay after releasing a cork beforing completing the
>> spinners to give time to the i915 schedule to process the fence and
>> release and queue the requests.
>>
>> v2: Add a delay between starting releasing spinner and cork in
>> promotion
>> v3:
>> (Daniele)
>> - Always add delay, update commit message
>>
>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>> ---
>> tests/i915/gem_ctx_shared.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
>> index ea1b5dd1b..7f88871b8 100644
>> --- a/tests/i915/gem_ctx_shared.c
>> +++ b/tests/i915/gem_ctx_shared.c
>> @@ -622,6 +622,7 @@ static void unplug_show_queue(int i915, struct
>> igt_cork *c, uint64_t ahnd,
>> igt_cork_unplug(c); /* batches will now be queued on the engine */
>> igt_debugfs_dump(i915, "i915_engine_info");
>> + usleep(250000);
>
> Same as previous patch, with a comment added:
>
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Come on guys, it is a bit bad and lazy for good taste no? 250ms more
test runtime, multiplied by number of tests and subtests (engines) will
add up and over shadows the current test time. For instance current
state is:
# tests/gem_ctx_shared --r *promotion*
IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
Starting subtest: Q-promotion
Starting dynamic subtest: rcs0
Dynamic subtest rcs0: SUCCESS (0.174s)
Starting dynamic subtest: bcs0
Dynamic subtest bcs0: SUCCESS (0.224s)
Starting dynamic subtest: vcs0
Dynamic subtest vcs0: SUCCESS (0.153s)
Starting dynamic subtest: vecs0
Dynamic subtest vecs0: SUCCESS (0.153s)
Subtest Q-promotion: SUCCESS (0.708s)
This patch instantly makes this 1.7 seconds instead. Add the in/out
order subtests, other tests, platforms with more engines, in a world
where CI time budget is scarce - can't we do better than this and not
settle on sprinkling of usleep all over the place?
Like maybe add out fences to the two requests, merge them, and wait on
that (since there is no implicit write declared so set domain does not
suffice)?
Unless I am missing something it would be strictly correct for execlists
backend as well since it now works just because it is fast enough.
Regards,
Tvrtko
>
> Daniele
>
>> for (int n = 0; n < ARRAY_SIZE(spin); n++) {
>> ahnd = spin[n]->ahnd;
>> @@ -831,10 +832,10 @@ static void promotion(int i915, const
>> intel_ctx_cfg_t *cfg, unsigned ring)
>> gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
>> ctx[HI] = intel_ctx_create(i915, &q_cfg);
>> - gem_context_set_priority(i915, ctx[HI]->id, 0);
>> + gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
>> ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
>> - gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
>> + gem_context_set_priority(i915, ctx[NOISE]->id, 0);
>> result = gem_create(i915, 4096);
>> result_offset = get_offset(ahnd, result, 4096, 0);
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-10-05 10:44 ` Tvrtko Ursulin
@ 2021-10-05 16:44 ` Matthew Brost
2021-10-06 8:12 ` Tvrtko Ursulin
0 siblings, 1 reply; 24+ messages in thread
From: Matthew Brost @ 2021-10-05 16:44 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Daniele Ceraolo Spurio, igt-dev, john.c.harrison
On Tue, Oct 05, 2021 at 11:44:02AM +0100, Tvrtko Ursulin wrote:
>
> On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
> >
> >
> > On 9/16/2021 11:03 AM, Matthew Brost wrote:
> > > The i915 currently has 2k visible priority levels which are currently
> > > unique. This is changing to statically map these 2k levels into 3
> > > buckets:
> > >
> > > low: < 0
> > > mid: 0
> > > high: > 0
> > >
> > > Update gem_ctx_shared to understand this. This entails updating
> > > promotion test to use 3 levels that will map into different buckets and
> > > also add bit of delay after releasing a cork beforing completing the
> > > spinners to give time to the i915 schedule to process the fence and
> > > release and queue the requests.
> > >
> > > v2: Add a delay between starting releasing spinner and cork in
> > > promotion
> > > v3:
> > > (Daniele)
> > > - Always add delay, update commit message
> > >
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > > tests/i915/gem_ctx_shared.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > > index ea1b5dd1b..7f88871b8 100644
> > > --- a/tests/i915/gem_ctx_shared.c
> > > +++ b/tests/i915/gem_ctx_shared.c
> > > @@ -622,6 +622,7 @@ static void unplug_show_queue(int i915, struct
> > > igt_cork *c, uint64_t ahnd,
> > > igt_cork_unplug(c); /* batches will now be queued on the engine */
> > > igt_debugfs_dump(i915, "i915_engine_info");
> > > + usleep(250000);
> >
> > Same as previous patch, with a comment added:
> >
> > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>
> Come on guys, it is a bit bad and lazy for good taste no? 250ms more test
Yea, this is way too long of a sleep. 25ms seems just fine.
On TGL /w the updated sleep:
gem_ctx_shared --r *promotion*
IGT-Version: 1.26-g7e489b053 (x86_64) (Linux: 5.15.0-rc3-guc+ x86_64)
Starting subtest: Q-promotion
Starting dynamic subtest: rcs0
Dynamic subtest rcs0: SUCCESS (0.059s)
Starting dynamic subtest: bcs0
Dynamic subtest bcs0: SUCCESS (0.059s)
Starting dynamic subtest: vcs0
Dynamic subtest vcs0: SUCCESS (0.060s)
Starting dynamic subtest: vecs0
Dynamic subtest vecs0: SUCCESS (0.061s)
Subtest Q-promotion: SUCCESS (0.239s)
> runtime, multiplied by number of tests and subtests (engines) will add up
> and over shadows the current test time. For instance current state is:
>
> # tests/gem_ctx_shared --r *promotion*
> IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
> Starting subtest: Q-promotion
> Starting dynamic subtest: rcs0
> Dynamic subtest rcs0: SUCCESS (0.174s)
> Starting dynamic subtest: bcs0
> Dynamic subtest bcs0: SUCCESS (0.224s)
> Starting dynamic subtest: vcs0
> Dynamic subtest vcs0: SUCCESS (0.153s)
> Starting dynamic subtest: vecs0
> Dynamic subtest vecs0: SUCCESS (0.153s)
> Subtest Q-promotion: SUCCESS (0.708s)
>
> This patch instantly makes this 1.7 seconds instead. Add the in/out order
> subtests, other tests, platforms with more engines, in a world where CI time
> budget is scarce - can't we do better than this and not settle on sprinkling
> of usleep all over the place?
>
> Like maybe add out fences to the two requests, merge them, and wait on that
> (since there is no implicit write declared so set domain does not suffice)?
>
Not sure I follow. Anyways reposting now with a smaller timeout value.
Unless we hear something we plan on merging this tomorrow.
Matt
> Unless I am missing something it would be strictly correct for execlists
> backend as well since it now works just because it is fast enough.
>
> Regards,
>
> Tvrtko
>
> >
> > Daniele
> >
> > > for (int n = 0; n < ARRAY_SIZE(spin); n++) {
> > > ahnd = spin[n]->ahnd;
> > > @@ -831,10 +832,10 @@ static void promotion(int i915, const
> > > intel_ctx_cfg_t *cfg, unsigned ring)
> > > gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
> > > ctx[HI] = intel_ctx_create(i915, &q_cfg);
> > > - gem_context_set_priority(i915, ctx[HI]->id, 0);
> > > + gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
> > > ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
> > > - gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
> > > + gem_context_set_priority(i915, ctx[NOISE]->id, 0);
> > > result = gem_create(i915, 4096);
> > > result_offset = get_offset(ahnd, result, 4096, 0);
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-10-05 16:44 ` Matthew Brost
@ 2021-10-06 8:12 ` Tvrtko Ursulin
2021-10-06 16:41 ` Matthew Brost
0 siblings, 1 reply; 24+ messages in thread
From: Tvrtko Ursulin @ 2021-10-06 8:12 UTC (permalink / raw)
To: Matthew Brost; +Cc: Daniele Ceraolo Spurio, igt-dev, john.c.harrison
On 05/10/2021 17:44, Matthew Brost wrote:
> On Tue, Oct 05, 2021 at 11:44:02AM +0100, Tvrtko Ursulin wrote:
>>
>> On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
>>>
>>>
>>> On 9/16/2021 11:03 AM, Matthew Brost wrote:
>>>> The i915 currently has 2k visible priority levels which are currently
>>>> unique. This is changing to statically map these 2k levels into 3
>>>> buckets:
>>>>
>>>> low: < 0
>>>> mid: 0
>>>> high: > 0
>>>>
>>>> Update gem_ctx_shared to understand this. This entails updating
>>>> promotion test to use 3 levels that will map into different buckets and
>>>> also add bit of delay after releasing a cork beforing completing the
>>>> spinners to give time to the i915 schedule to process the fence and
>>>> release and queue the requests.
>>>>
>>>> v2: Add a delay between starting releasing spinner and cork in
>>>> promotion
>>>> v3:
>>>> (Daniele)
>>>> - Always add delay, update commit message
>>>>
>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>>> ---
>>>> tests/i915/gem_ctx_shared.c | 5 +++--
>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
>>>> index ea1b5dd1b..7f88871b8 100644
>>>> --- a/tests/i915/gem_ctx_shared.c
>>>> +++ b/tests/i915/gem_ctx_shared.c
>>>> @@ -622,6 +622,7 @@ static void unplug_show_queue(int i915, struct
>>>> igt_cork *c, uint64_t ahnd,
>>>> igt_cork_unplug(c); /* batches will now be queued on the engine */
>>>> igt_debugfs_dump(i915, "i915_engine_info");
>>>> + usleep(250000);
>>>
>>> Same as previous patch, with a comment added:
>>>
>>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>> Come on guys, it is a bit bad and lazy for good taste no? 250ms more test
>
> Yea, this is way too long of a sleep. 25ms seems just fine.
Until you get 1/1000 failures in CI on some platforms due phase of the
moon. Adding sleeps should be avoided where possible.
>
> On TGL /w the updated sleep:
> gem_ctx_shared --r *promotion*
> IGT-Version: 1.26-g7e489b053 (x86_64) (Linux: 5.15.0-rc3-guc+ x86_64)
> Starting subtest: Q-promotion
> Starting dynamic subtest: rcs0
> Dynamic subtest rcs0: SUCCESS (0.059s)
> Starting dynamic subtest: bcs0
> Dynamic subtest bcs0: SUCCESS (0.059s)
> Starting dynamic subtest: vcs0
> Dynamic subtest vcs0: SUCCESS (0.060s)
> Starting dynamic subtest: vecs0
> Dynamic subtest vecs0: SUCCESS (0.061s)
> Subtest Q-promotion: SUCCESS (0.239s)
>
>> runtime, multiplied by number of tests and subtests (engines) will add up
>> and over shadows the current test time. For instance current state is:
>>
>> # tests/gem_ctx_shared --r *promotion*
>> IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
>> Starting subtest: Q-promotion
>> Starting dynamic subtest: rcs0
>> Dynamic subtest rcs0: SUCCESS (0.174s)
>> Starting dynamic subtest: bcs0
>> Dynamic subtest bcs0: SUCCESS (0.224s)
>> Starting dynamic subtest: vcs0
>> Dynamic subtest vcs0: SUCCESS (0.153s)
>> Starting dynamic subtest: vecs0
>> Dynamic subtest vecs0: SUCCESS (0.153s)
>> Subtest Q-promotion: SUCCESS (0.708s)
>>
>> This patch instantly makes this 1.7 seconds instead. Add the in/out order
>> subtests, other tests, platforms with more engines, in a world where CI time
>> budget is scarce - can't we do better than this and not settle on sprinkling
>> of usleep all over the place?
>>
>> Like maybe add out fences to the two requests, merge them, and wait on that
>> (since there is no implicit write declared so set domain does not suffice)?
>>
>
> Not sure I follow. Anyways reposting now with a smaller timeout value.
> Unless we hear something we plan on merging this tomorrow.
The in/out-order test as example. It needs to wait until two dword
writes have completed, right? I am saying pass output fences out from
spinners and wait on them before checking content of the shared buffer.
I also think sleep after uncorking is also conceptually misleading
because that's not where the problem lies. Problem is set domain does
not actually wait for sdw completion (existing comment even hints so "no
write hazard lies!"). If I am right sporadic fail can in *theory* happen
with execlists as well.
Regards,
Tvrtko
>
> Matt
>
>> Unless I am missing something it would be strictly correct for execlists
>> backend as well since it now works just because it is fast enough.
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Daniele
>>>
>>>> for (int n = 0; n < ARRAY_SIZE(spin); n++) {
>>>> ahnd = spin[n]->ahnd;
>>>> @@ -831,10 +832,10 @@ static void promotion(int i915, const
>>>> intel_ctx_cfg_t *cfg, unsigned ring)
>>>> gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
>>>> ctx[HI] = intel_ctx_create(i915, &q_cfg);
>>>> - gem_context_set_priority(i915, ctx[HI]->id, 0);
>>>> + gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
>>>> ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
>>>> - gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
>>>> + gem_context_set_priority(i915, ctx[NOISE]->id, 0);
>>>> result = gem_create(i915, 4096);
>>>> result_offset = get_offset(ahnd, result, 4096, 0);
>>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-10-06 8:12 ` Tvrtko Ursulin
@ 2021-10-06 16:41 ` Matthew Brost
2021-10-06 18:34 ` Tvrtko Ursulin
0 siblings, 1 reply; 24+ messages in thread
From: Matthew Brost @ 2021-10-06 16:41 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Daniele Ceraolo Spurio, igt-dev, john.c.harrison
On Wed, Oct 06, 2021 at 09:12:41AM +0100, Tvrtko Ursulin wrote:
>
> On 05/10/2021 17:44, Matthew Brost wrote:
> > On Tue, Oct 05, 2021 at 11:44:02AM +0100, Tvrtko Ursulin wrote:
> > >
> > > On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
> > > >
> > > >
> > > > On 9/16/2021 11:03 AM, Matthew Brost wrote:
> > > > > The i915 currently has 2k visible priority levels which are currently
> > > > > unique. This is changing to statically map these 2k levels into 3
> > > > > buckets:
> > > > >
> > > > > low: < 0
> > > > > mid: 0
> > > > > high: > 0
> > > > >
> > > > > Update gem_ctx_shared to understand this. This entails updating
> > > > > promotion test to use 3 levels that will map into different buckets and
> > > > > also add bit of delay after releasing a cork beforing completing the
> > > > > spinners to give time to the i915 schedule to process the fence and
> > > > > release and queue the requests.
> > > > >
> > > > > v2: Add a delay between starting releasing spinner and cork in
> > > > > promotion
> > > > > v3:
> > > > > (Daniele)
> > > > > - Always add delay, update commit message
> > > > >
> > > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > > ---
> > > > > tests/i915/gem_ctx_shared.c | 5 +++--
> > > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > > > > index ea1b5dd1b..7f88871b8 100644
> > > > > --- a/tests/i915/gem_ctx_shared.c
> > > > > +++ b/tests/i915/gem_ctx_shared.c
> > > > > @@ -622,6 +622,7 @@ static void unplug_show_queue(int i915, struct
> > > > > igt_cork *c, uint64_t ahnd,
> > > > > igt_cork_unplug(c); /* batches will now be queued on the engine */
> > > > > igt_debugfs_dump(i915, "i915_engine_info");
> > > > > + usleep(250000);
> > > >
> > > > Same as previous patch, with a comment added:
> > > >
> > > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > >
> > > Come on guys, it is a bit bad and lazy for good taste no? 250ms more test
> >
> > Yea, this is way too long of a sleep. 25ms seems just fine.
>
> Until you get 1/1000 failures in CI on some platforms due phase of the moon.
> Adding sleeps should be avoided where possible.
>
Yea, that is always a risk. Looking at this test there is already 1
other sleep in the test and in gem_exec_schedule there are 5 other
sleeps. I'm not breaking precedent here.
> >
> > On TGL /w the updated sleep:
> > gem_ctx_shared --r *promotion*
> > IGT-Version: 1.26-g7e489b053 (x86_64) (Linux: 5.15.0-rc3-guc+ x86_64)
> > Starting subtest: Q-promotion
> > Starting dynamic subtest: rcs0
> > Dynamic subtest rcs0: SUCCESS (0.059s)
> > Starting dynamic subtest: bcs0
> > Dynamic subtest bcs0: SUCCESS (0.059s)
> > Starting dynamic subtest: vcs0
> > Dynamic subtest vcs0: SUCCESS (0.060s)
> > Starting dynamic subtest: vecs0
> > Dynamic subtest vecs0: SUCCESS (0.061s)
> > Subtest Q-promotion: SUCCESS (0.239s)
> >
> > > runtime, multiplied by number of tests and subtests (engines) will add up
> > > and over shadows the current test time. For instance current state is:
> > >
> > > # tests/gem_ctx_shared --r *promotion*
> > > IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
> > > Starting subtest: Q-promotion
> > > Starting dynamic subtest: rcs0
> > > Dynamic subtest rcs0: SUCCESS (0.174s)
> > > Starting dynamic subtest: bcs0
> > > Dynamic subtest bcs0: SUCCESS (0.224s)
> > > Starting dynamic subtest: vcs0
> > > Dynamic subtest vcs0: SUCCESS (0.153s)
> > > Starting dynamic subtest: vecs0
> > > Dynamic subtest vecs0: SUCCESS (0.153s)
> > > Subtest Q-promotion: SUCCESS (0.708s)
> > >
> > > This patch instantly makes this 1.7 seconds instead. Add the in/out order
> > > subtests, other tests, platforms with more engines, in a world where CI time
> > > budget is scarce - can't we do better than this and not settle on sprinkling
> > > of usleep all over the place?
> > >
> > > Like maybe add out fences to the two requests, merge them, and wait on that
> > > (since there is no implicit write declared so set domain does not suffice)?
> > >
> >
> > Not sure I follow. Anyways reposting now with a smaller timeout value.
> > Unless we hear something we plan on merging this tomorrow.
>
> The in/out-order test as example. It needs to wait until two dword writes
> have completed, right? I am saying pass output fences out from spinners and
> wait on them before checking content of the shared buffer.
>
> I also think sleep after uncorking is also conceptually misleading because
> that's not where the problem lies. Problem is set domain does not actually
> wait for sdw completion (existing comment even hints so "no write hazard
> lies!"). If I am right sporadic fail can in *theory* happen with execlists
> as well.
>
Still not following what you are saying. The sleep just adds period of
time for all the uncorked requests to make it into the GuC scheduler,
without it is possible for the lower priority request (submitted first)
completes before the higher priority request (submitted after) makes it
into the GuC. The sleep ensures alls the requests are in the GuC
scheduler still stuck behind spinning requests on the hardware, then
after the spinners complete the requests are scheduled in order of
priority. Yes, this possible with execlists too but I think the timing
is different enough it doesn't happen.
Again adding a sleep here seems legit to me, the same problem would
exist if we used fences as you suggest (e.g. the lower priority request
would be submitted first + could complete before the higher priority
request is submitted). Let's not over think this one.
Matt
> Regards,
>
> Tvrtko
>
> >
> > Matt
> >
> > > Unless I am missing something it would be strictly correct for execlists
> > > backend as well since it now works just because it is fast enough.
> > >
> > > Regards,
> > >
> > > Tvrtko
> > >
> > > >
> > > > Daniele
> > > >
> > > > > for (int n = 0; n < ARRAY_SIZE(spin); n++) {
> > > > > ahnd = spin[n]->ahnd;
> > > > > @@ -831,10 +832,10 @@ static void promotion(int i915, const
> > > > > intel_ctx_cfg_t *cfg, unsigned ring)
> > > > > gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
> > > > > ctx[HI] = intel_ctx_create(i915, &q_cfg);
> > > > > - gem_context_set_priority(i915, ctx[HI]->id, 0);
> > > > > + gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
> > > > > ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
> > > > > - gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
> > > > > + gem_context_set_priority(i915, ctx[NOISE]->id, 0);
> > > > > result = gem_create(i915, 4096);
> > > > > result_offset = get_offset(ahnd, result, 4096, 0);
> > > >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-10-06 16:41 ` Matthew Brost
@ 2021-10-06 18:34 ` Tvrtko Ursulin
2021-10-08 17:49 ` Matthew Brost
0 siblings, 1 reply; 24+ messages in thread
From: Tvrtko Ursulin @ 2021-10-06 18:34 UTC (permalink / raw)
To: Matthew Brost; +Cc: Daniele Ceraolo Spurio, igt-dev, john.c.harrison
On 06/10/2021 17:41, Matthew Brost wrote:
> On Wed, Oct 06, 2021 at 09:12:41AM +0100, Tvrtko Ursulin wrote:
>>
>> On 05/10/2021 17:44, Matthew Brost wrote:
>>> On Tue, Oct 05, 2021 at 11:44:02AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
>>>>>
>>>>>
>>>>> On 9/16/2021 11:03 AM, Matthew Brost wrote:
>>>>>> The i915 currently has 2k visible priority levels which are currently
>>>>>> unique. This is changing to statically map these 2k levels into 3
>>>>>> buckets:
>>>>>>
>>>>>> low: < 0
>>>>>> mid: 0
>>>>>> high: > 0
>>>>>>
>>>>>> Update gem_ctx_shared to understand this. This entails updating
>>>>>> promotion test to use 3 levels that will map into different buckets and
>>>>>> also add bit of delay after releasing a cork beforing completing the
>>>>>> spinners to give time to the i915 schedule to process the fence and
>>>>>> release and queue the requests.
>>>>>>
>>>>>> v2: Add a delay between starting releasing spinner and cork in
>>>>>> promotion
>>>>>> v3:
>>>>>> (Daniele)
>>>>>> - Always add delay, update commit message
>>>>>>
>>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>>>>> ---
>>>>>> tests/i915/gem_ctx_shared.c | 5 +++--
>>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
>>>>>> index ea1b5dd1b..7f88871b8 100644
>>>>>> --- a/tests/i915/gem_ctx_shared.c
>>>>>> +++ b/tests/i915/gem_ctx_shared.c
>>>>>> @@ -622,6 +622,7 @@ static void unplug_show_queue(int i915, struct
>>>>>> igt_cork *c, uint64_t ahnd,
>>>>>> igt_cork_unplug(c); /* batches will now be queued on the engine */
>>>>>> igt_debugfs_dump(i915, "i915_engine_info");
>>>>>> + usleep(250000);
>>>>>
>>>>> Same as previous patch, with a comment added:
>>>>>
>>>>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>>
>>>> Come on guys, it is a bit bad and lazy for good taste no? 250ms more test
>>>
>>> Yea, this is way too long of a sleep. 25ms seems just fine.
>>
>> Until you get 1/1000 failures in CI on some platforms due phase of the moon.
>> Adding sleeps should be avoided where possible.
>>
>
> Yea, that is always a risk. Looking at this test there is already 1
> other sleep in the test and in gem_exec_schedule there are 5 other
> sleeps. I'm not breaking precedent here.
>
>>>
>>> On TGL /w the updated sleep:
>>> gem_ctx_shared --r *promotion*
>>> IGT-Version: 1.26-g7e489b053 (x86_64) (Linux: 5.15.0-rc3-guc+ x86_64)
>>> Starting subtest: Q-promotion
>>> Starting dynamic subtest: rcs0
>>> Dynamic subtest rcs0: SUCCESS (0.059s)
>>> Starting dynamic subtest: bcs0
>>> Dynamic subtest bcs0: SUCCESS (0.059s)
>>> Starting dynamic subtest: vcs0
>>> Dynamic subtest vcs0: SUCCESS (0.060s)
>>> Starting dynamic subtest: vecs0
>>> Dynamic subtest vecs0: SUCCESS (0.061s)
>>> Subtest Q-promotion: SUCCESS (0.239s)
>>>
>>>> runtime, multiplied by number of tests and subtests (engines) will add up
>>>> and over shadows the current test time. For instance current state is:
>>>>
>>>> # tests/gem_ctx_shared --r *promotion*
>>>> IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
>>>> Starting subtest: Q-promotion
>>>> Starting dynamic subtest: rcs0
>>>> Dynamic subtest rcs0: SUCCESS (0.174s)
>>>> Starting dynamic subtest: bcs0
>>>> Dynamic subtest bcs0: SUCCESS (0.224s)
>>>> Starting dynamic subtest: vcs0
>>>> Dynamic subtest vcs0: SUCCESS (0.153s)
>>>> Starting dynamic subtest: vecs0
>>>> Dynamic subtest vecs0: SUCCESS (0.153s)
>>>> Subtest Q-promotion: SUCCESS (0.708s)
>>>>
>>>> This patch instantly makes this 1.7 seconds instead. Add the in/out order
>>>> subtests, other tests, platforms with more engines, in a world where CI time
>>>> budget is scarce - can't we do better than this and not settle on sprinkling
>>>> of usleep all over the place?
>>>>
>>>> Like maybe add out fences to the two requests, merge them, and wait on that
>>>> (since there is no implicit write declared so set domain does not suffice)?
>>>>
>>>
>>> Not sure I follow. Anyways reposting now with a smaller timeout value.
>>> Unless we hear something we plan on merging this tomorrow.
>>
>> The in/out-order test as example. It needs to wait until two dword writes
>> have completed, right? I am saying pass output fences out from spinners and
>> wait on them before checking content of the shared buffer.
>>
>> I also think sleep after uncorking is also conceptually misleading because
>> that's not where the problem lies. Problem is set domain does not actually
>> wait for sdw completion (existing comment even hints so "no write hazard
>> lies!"). If I am right sporadic fail can in *theory* happen with execlists
>> as well.
>>
>
> Still not following what you are saying. The sleep just adds period of
> time for all the uncorked requests to make it into the GuC scheduler,
> without it is possible for the lower priority request (submitted first)
> completes before the higher priority request (submitted after) makes it
> into the GuC. The sleep ensures alls the requests are in the GuC
> scheduler still stuck behind spinning requests on the hardware, then
> after the spinners complete the requests are scheduled in order of
> priority. Yes, this possible with execlists too but I think the timing
> is different enough it doesn't happen.
>
> Again adding a sleep here seems legit to me, the same problem would
> exist if we used fences as you suggest (e.g. the lower priority request
> would be submitted first + could complete before the higher priority
> request is submitted). Let's not over think this one.
I don't understand how sleep _after_ uncork can have an effect you
describe. Both HI and LO have been submitted and the following operation
will just check the writes in memory.
Unless the problem really is that the spinner does not start executing
before the unplug? Does the sleep before unplug work as well?
Regards,
Tvrtko
>
> Matt
>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Matt
>>>
>>>> Unless I am missing something it would be strictly correct for execlists
>>>> backend as well since it now works just because it is fast enough.
>>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>>
>>>>> Daniele
>>>>>
>>>>>> for (int n = 0; n < ARRAY_SIZE(spin); n++) {
>>>>>> ahnd = spin[n]->ahnd;
>>>>>> @@ -831,10 +832,10 @@ static void promotion(int i915, const
>>>>>> intel_ctx_cfg_t *cfg, unsigned ring)
>>>>>> gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
>>>>>> ctx[HI] = intel_ctx_create(i915, &q_cfg);
>>>>>> - gem_context_set_priority(i915, ctx[HI]->id, 0);
>>>>>> + gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
>>>>>> ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
>>>>>> - gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
>>>>>> + gem_context_set_priority(i915, ctx[NOISE]->id, 0);
>>>>>> result = gem_create(i915, 4096);
>>>>>> result_offset = get_offset(ahnd, result, 4096, 0);
>>>>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-10-06 18:34 ` Tvrtko Ursulin
@ 2021-10-08 17:49 ` Matthew Brost
2021-10-11 8:04 ` Tvrtko Ursulin
0 siblings, 1 reply; 24+ messages in thread
From: Matthew Brost @ 2021-10-08 17:49 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Daniele Ceraolo Spurio, igt-dev, john.c.harrison
On Wed, Oct 06, 2021 at 07:34:45PM +0100, Tvrtko Ursulin wrote:
>
> On 06/10/2021 17:41, Matthew Brost wrote:
> > On Wed, Oct 06, 2021 at 09:12:41AM +0100, Tvrtko Ursulin wrote:
> > >
> > > On 05/10/2021 17:44, Matthew Brost wrote:
> > > > On Tue, Oct 05, 2021 at 11:44:02AM +0100, Tvrtko Ursulin wrote:
> > > > >
> > > > > On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
> > > > > >
> > > > > >
> > > > > > On 9/16/2021 11:03 AM, Matthew Brost wrote:
> > > > > > > The i915 currently has 2k visible priority levels which are currently
> > > > > > > unique. This is changing to statically map these 2k levels into 3
> > > > > > > buckets:
> > > > > > >
> > > > > > > low: < 0
> > > > > > > mid: 0
> > > > > > > high: > 0
> > > > > > >
> > > > > > > Update gem_ctx_shared to understand this. This entails updating
> > > > > > > promotion test to use 3 levels that will map into different buckets and
> > > > > > > also add bit of delay after releasing a cork beforing completing the
> > > > > > > spinners to give time to the i915 schedule to process the fence and
> > > > > > > release and queue the requests.
> > > > > > >
> > > > > > > v2: Add a delay between starting releasing spinner and cork in
> > > > > > > promotion
> > > > > > > v3:
> > > > > > > (Daniele)
> > > > > > > - Always add delay, update commit message
> > > > > > >
> > > > > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > > > > ---
> > > > > > > tests/i915/gem_ctx_shared.c | 5 +++--
> > > > > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > > > > > > index ea1b5dd1b..7f88871b8 100644
> > > > > > > --- a/tests/i915/gem_ctx_shared.c
> > > > > > > +++ b/tests/i915/gem_ctx_shared.c
> > > > > > > @@ -622,6 +622,7 @@ static void unplug_show_queue(int i915, struct
> > > > > > > igt_cork *c, uint64_t ahnd,
> > > > > > > igt_cork_unplug(c); /* batches will now be queued on the engine */
> > > > > > > igt_debugfs_dump(i915, "i915_engine_info");
> > > > > > > + usleep(250000);
> > > > > >
> > > > > > Same as previous patch, with a comment added:
> > > > > >
> > > > > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > > >
> > > > > Come on guys, it is a bit bad and lazy for good taste no? 250ms more test
> > > >
> > > > Yea, this is way too long of a sleep. 25ms seems just fine.
> > >
> > > Until you get 1/1000 failures in CI on some platforms due phase of the moon.
> > > Adding sleeps should be avoided where possible.
> > >
> >
> > Yea, that is always a risk. Looking at this test there is already 1
> > other sleep in the test and in gem_exec_schedule there are 5 other
> > sleeps. I'm not breaking precedent here.
> >
> > > >
> > > > On TGL /w the updated sleep:
> > > > gem_ctx_shared --r *promotion*
> > > > IGT-Version: 1.26-g7e489b053 (x86_64) (Linux: 5.15.0-rc3-guc+ x86_64)
> > > > Starting subtest: Q-promotion
> > > > Starting dynamic subtest: rcs0
> > > > Dynamic subtest rcs0: SUCCESS (0.059s)
> > > > Starting dynamic subtest: bcs0
> > > > Dynamic subtest bcs0: SUCCESS (0.059s)
> > > > Starting dynamic subtest: vcs0
> > > > Dynamic subtest vcs0: SUCCESS (0.060s)
> > > > Starting dynamic subtest: vecs0
> > > > Dynamic subtest vecs0: SUCCESS (0.061s)
> > > > Subtest Q-promotion: SUCCESS (0.239s)
> > > >
> > > > > runtime, multiplied by number of tests and subtests (engines) will add up
> > > > > and over shadows the current test time. For instance current state is:
> > > > >
> > > > > # tests/gem_ctx_shared --r *promotion*
> > > > > IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
> > > > > Starting subtest: Q-promotion
> > > > > Starting dynamic subtest: rcs0
> > > > > Dynamic subtest rcs0: SUCCESS (0.174s)
> > > > > Starting dynamic subtest: bcs0
> > > > > Dynamic subtest bcs0: SUCCESS (0.224s)
> > > > > Starting dynamic subtest: vcs0
> > > > > Dynamic subtest vcs0: SUCCESS (0.153s)
> > > > > Starting dynamic subtest: vecs0
> > > > > Dynamic subtest vecs0: SUCCESS (0.153s)
> > > > > Subtest Q-promotion: SUCCESS (0.708s)
> > > > >
> > > > > This patch instantly makes this 1.7 seconds instead. Add the in/out order
> > > > > subtests, other tests, platforms with more engines, in a world where CI time
> > > > > budget is scarce - can't we do better than this and not settle on sprinkling
> > > > > of usleep all over the place?
> > > > >
> > > > > Like maybe add out fences to the two requests, merge them, and wait on that
> > > > > (since there is no implicit write declared so set domain does not suffice)?
> > > > >
> > > >
> > > > Not sure I follow. Anyways reposting now with a smaller timeout value.
> > > > Unless we hear something we plan on merging this tomorrow.
> > >
> > > The in/out-order test as example. It needs to wait until two dword writes
> > > have completed, right? I am saying pass output fences out from spinners and
> > > wait on them before checking content of the shared buffer.
> > >
> > > I also think sleep after uncorking is also conceptually misleading because
> > > that's not where the problem lies. Problem is set domain does not actually
> > > wait for sdw completion (existing comment even hints so "no write hazard
> > > lies!"). If I am right sporadic fail can in *theory* happen with execlists
> > > as well.
> > >
> >
> > Still not following what you are saying. The sleep just adds period of
> > time for all the uncorked requests to make it into the GuC scheduler,
> > without it is possible for the lower priority request (submitted first)
> > completes before the higher priority request (submitted after) makes it
> > into the GuC. The sleep ensures alls the requests are in the GuC
> > scheduler still stuck behind spinning requests on the hardware, then
> > after the spinners complete the requests are scheduled in order of
> > priority. Yes, this possible with execlists too but I think the timing
> > is different enough it doesn't happen.
> >
> > Again adding a sleep here seems legit to me, the same problem would
> > exist if we used fences as you suggest (e.g. the lower priority request
> > would be submitted first + could complete before the higher priority
> > request is submitted). Let's not over think this one.
>
> I don't understand how sleep _after_ uncork can have an effect you describe.
> Both HI and LO have been submitted and the following operation will just
> check the writes in memory.
>
After the uncork but before releasing the spinning batches, anything
submitted to the GuC is still be blocked behind the spinners. A sleep
makes sure that all contexts submitted after the uncork are processed by
the GuC, sitting in the GuC scheduler, and still blocked behind the
spinners. Once the spinners are released now the GuC submits uncorked
requests in the correct order. Without this sleep, there is a race where
the earlier contexts (lower priority) is in the GuC scheduler but the
later ones (higher priority) are not. In this case the earlier (lower
priority) contexts get submitted and complete before the GuC sees the
higher priority ones causing the test to fail. It only fails like this 1
out of 10ish times without the sleep (i.e. it is racey). I just ran this
1000x times in the loop /w the sleep and didn't see a failure.
Make sense? Can we get this merged and move on?
> Unless the problem really is that the spinner does not start executing
> before the unplug? Does the sleep before unplug work as well?
>
No see above. This doesn't help at all.
Matt
> Regards,
>
> Tvrtko
>
>
> >
> > Matt
> >
> > > Regards,
> > >
> > > Tvrtko
> > >
> > > >
> > > > Matt
> > > >
> > > > > Unless I am missing something it would be strictly correct for execlists
> > > > > backend as well since it now works just because it is fast enough.
> > > > >
> > > > > Regards,
> > > > >
> > > > > Tvrtko
> > > > >
> > > > > >
> > > > > > Daniele
> > > > > >
> > > > > > > for (int n = 0; n < ARRAY_SIZE(spin); n++) {
> > > > > > > ahnd = spin[n]->ahnd;
> > > > > > > @@ -831,10 +832,10 @@ static void promotion(int i915, const
> > > > > > > intel_ctx_cfg_t *cfg, unsigned ring)
> > > > > > > gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
> > > > > > > ctx[HI] = intel_ctx_create(i915, &q_cfg);
> > > > > > > - gem_context_set_priority(i915, ctx[HI]->id, 0);
> > > > > > > + gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
> > > > > > > ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
> > > > > > > - gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
> > > > > > > + gem_context_set_priority(i915, ctx[NOISE]->id, 0);
> > > > > > > result = gem_create(i915, 4096);
> > > > > > > result_offset = get_offset(ahnd, result, 4096, 0);
> > > > > >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-10-08 17:49 ` Matthew Brost
@ 2021-10-11 8:04 ` Tvrtko Ursulin
2021-10-11 17:18 ` Matthew Brost
0 siblings, 1 reply; 24+ messages in thread
From: Tvrtko Ursulin @ 2021-10-11 8:04 UTC (permalink / raw)
To: Matthew Brost; +Cc: Daniele Ceraolo Spurio, igt-dev, john.c.harrison
On 08/10/2021 18:49, Matthew Brost wrote:
> On Wed, Oct 06, 2021 at 07:34:45PM +0100, Tvrtko Ursulin wrote:
>>
>> On 06/10/2021 17:41, Matthew Brost wrote:
>>> On Wed, Oct 06, 2021 at 09:12:41AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 05/10/2021 17:44, Matthew Brost wrote:
>>>>> On Tue, Oct 05, 2021 at 11:44:02AM +0100, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 9/16/2021 11:03 AM, Matthew Brost wrote:
>>>>>>>> The i915 currently has 2k visible priority levels which are currently
>>>>>>>> unique. This is changing to statically map these 2k levels into 3
>>>>>>>> buckets:
>>>>>>>>
>>>>>>>> low: < 0
>>>>>>>> mid: 0
>>>>>>>> high: > 0
>>>>>>>>
>>>>>>>> Update gem_ctx_shared to understand this. This entails updating
>>>>>>>> promotion test to use 3 levels that will map into different buckets and
>>>>>>>> also add bit of delay after releasing a cork beforing completing the
>>>>>>>> spinners to give time to the i915 schedule to process the fence and
>>>>>>>> release and queue the requests.
>>>>>>>>
>>>>>>>> v2: Add a delay between starting releasing spinner and cork in
>>>>>>>> promotion
>>>>>>>> v3:
>>>>>>>> (Daniele)
>>>>>>>> - Always add delay, update commit message
>>>>>>>>
>>>>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>>>>>>> ---
>>>>>>>> tests/i915/gem_ctx_shared.c | 5 +++--
>>>>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
>>>>>>>> index ea1b5dd1b..7f88871b8 100644
>>>>>>>> --- a/tests/i915/gem_ctx_shared.c
>>>>>>>> +++ b/tests/i915/gem_ctx_shared.c
>>>>>>>> @@ -622,6 +622,7 @@ static void unplug_show_queue(int i915, struct
>>>>>>>> igt_cork *c, uint64_t ahnd,
>>>>>>>> igt_cork_unplug(c); /* batches will now be queued on the engine */
>>>>>>>> igt_debugfs_dump(i915, "i915_engine_info");
>>>>>>>> + usleep(250000);
>>>>>>>
>>>>>>> Same as previous patch, with a comment added:
>>>>>>>
>>>>>>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>>>>
>>>>>> Come on guys, it is a bit bad and lazy for good taste no? 250ms more test
>>>>>
>>>>> Yea, this is way too long of a sleep. 25ms seems just fine.
>>>>
>>>> Until you get 1/1000 failures in CI on some platforms due phase of the moon.
>>>> Adding sleeps should be avoided where possible.
>>>>
>>>
>>> Yea, that is always a risk. Looking at this test there is already 1
>>> other sleep in the test and in gem_exec_schedule there are 5 other
>>> sleeps. I'm not breaking precedent here.
>>>
>>>>>
>>>>> On TGL /w the updated sleep:
>>>>> gem_ctx_shared --r *promotion*
>>>>> IGT-Version: 1.26-g7e489b053 (x86_64) (Linux: 5.15.0-rc3-guc+ x86_64)
>>>>> Starting subtest: Q-promotion
>>>>> Starting dynamic subtest: rcs0
>>>>> Dynamic subtest rcs0: SUCCESS (0.059s)
>>>>> Starting dynamic subtest: bcs0
>>>>> Dynamic subtest bcs0: SUCCESS (0.059s)
>>>>> Starting dynamic subtest: vcs0
>>>>> Dynamic subtest vcs0: SUCCESS (0.060s)
>>>>> Starting dynamic subtest: vecs0
>>>>> Dynamic subtest vecs0: SUCCESS (0.061s)
>>>>> Subtest Q-promotion: SUCCESS (0.239s)
>>>>>
>>>>>> runtime, multiplied by number of tests and subtests (engines) will add up
>>>>>> and over shadows the current test time. For instance current state is:
>>>>>>
>>>>>> # tests/gem_ctx_shared --r *promotion*
>>>>>> IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
>>>>>> Starting subtest: Q-promotion
>>>>>> Starting dynamic subtest: rcs0
>>>>>> Dynamic subtest rcs0: SUCCESS (0.174s)
>>>>>> Starting dynamic subtest: bcs0
>>>>>> Dynamic subtest bcs0: SUCCESS (0.224s)
>>>>>> Starting dynamic subtest: vcs0
>>>>>> Dynamic subtest vcs0: SUCCESS (0.153s)
>>>>>> Starting dynamic subtest: vecs0
>>>>>> Dynamic subtest vecs0: SUCCESS (0.153s)
>>>>>> Subtest Q-promotion: SUCCESS (0.708s)
>>>>>>
>>>>>> This patch instantly makes this 1.7 seconds instead. Add the in/out order
>>>>>> subtests, other tests, platforms with more engines, in a world where CI time
>>>>>> budget is scarce - can't we do better than this and not settle on sprinkling
>>>>>> of usleep all over the place?
>>>>>>
>>>>>> Like maybe add out fences to the two requests, merge them, and wait on that
>>>>>> (since there is no implicit write declared so set domain does not suffice)?
>>>>>>
>>>>>
>>>>> Not sure I follow. Anyways reposting now with a smaller timeout value.
>>>>> Unless we hear something we plan on merging this tomorrow.
>>>>
>>>> The in/out-order test as example. It needs to wait until two dword writes
>>>> have completed, right? I am saying pass output fences out from spinners and
>>>> wait on them before checking content of the shared buffer.
>>>>
>>>> I also think sleep after uncorking is also conceptually misleading because
>>>> that's not where the problem lies. Problem is set domain does not actually
>>>> wait for sdw completion (existing comment even hints so "no write hazard
>>>> lies!"). If I am right sporadic fail can in *theory* happen with execlists
>>>> as well.
>>>>
>>>
>>> Still not following what you are saying. The sleep just adds period of
>>> time for all the uncorked requests to make it into the GuC scheduler,
>>> without it is possible for the lower priority request (submitted first)
>>> completes before the higher priority request (submitted after) makes it
>>> into the GuC. The sleep ensures alls the requests are in the GuC
>>> scheduler still stuck behind spinning requests on the hardware, then
>>> after the spinners complete the requests are scheduled in order of
>>> priority. Yes, this possible with execlists too but I think the timing
>>> is different enough it doesn't happen.
>>>
>>> Again adding a sleep here seems legit to me, the same problem would
>>> exist if we used fences as you suggest (e.g. the lower priority request
>>> would be submitted first + could complete before the higher priority
>>> request is submitted). Let's not over think this one.
>>
>> I don't understand how sleep _after_ uncork can have an effect you describe.
>> Both HI and LO have been submitted and the following operation will just
>> check the writes in memory.
>>
>
> After the uncork but before releasing the spinning batches, anything
> submitted to the GuC is still be blocked behind the spinners. A sleep
> makes sure that all contexts submitted after the uncork are processed by
> the GuC, sitting in the GuC scheduler, and still blocked behind the
> spinners. Once the spinners are released now the GuC submits uncorked
> requests in the correct order. Without this sleep, there is a race where
> the earlier contexts (lower priority) is in the GuC scheduler but the
> later ones (higher priority) are not. In this case the earlier (lower
> priority) contexts get submitted and complete before the GuC sees the
> higher priority ones causing the test to fail. It only fails like this 1
> out of 10ish times without the sleep (i.e. it is racey). I just ran this
> 1000x times in the loop /w the sleep and didn't see a failure.
>
> Make sense? Can we get this merged and move on?
I am still skeptical I'm afraid. Let me try to ask you from a different
angle and please feel free to tell me I am missing something crucial.
Is the test testing uapi contract or scheduler implementation details?
If it would be the former, then it would be highlighting the contract is
broken. If it is the latter then why it should be fudged to run with an
incompatible backend?
Personally I can't see that it is uapi being tested. Two unrelated
contexts, no data dependencies, why would there be any guarantees who
runs first? So how about just skip the test with GuC? If I am correct
there may even not be much value to have it with execlists.
Regards,
Tvrtko
>
>> Unless the problem really is that the spinner does not start executing
>> before the unplug? Does the sleep before unplug work as well?
>>
>
> No see above. This doesn't help at all.
>
> Matt
>
>> Regards,
>>
>> Tvrtko
>>
>>
>>>
>>> Matt
>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>>
>>>>> Matt
>>>>>
>>>>>> Unless I am missing something it would be strictly correct for execlists
>>>>>> backend as well since it now works just because it is fast enough.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Tvrtko
>>>>>>
>>>>>>>
>>>>>>> Daniele
>>>>>>>
>>>>>>>> for (int n = 0; n < ARRAY_SIZE(spin); n++) {
>>>>>>>> ahnd = spin[n]->ahnd;
>>>>>>>> @@ -831,10 +832,10 @@ static void promotion(int i915, const
>>>>>>>> intel_ctx_cfg_t *cfg, unsigned ring)
>>>>>>>> gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
>>>>>>>> ctx[HI] = intel_ctx_create(i915, &q_cfg);
>>>>>>>> - gem_context_set_priority(i915, ctx[HI]->id, 0);
>>>>>>>> + gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
>>>>>>>> ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
>>>>>>>> - gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
>>>>>>>> + gem_context_set_priority(i915, ctx[NOISE]->id, 0);
>>>>>>>> result = gem_create(i915, 4096);
>>>>>>>> result_offset = get_offset(ahnd, result, 4096, 0);
>>>>>>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-10-11 8:04 ` Tvrtko Ursulin
@ 2021-10-11 17:18 ` Matthew Brost
2021-10-11 18:50 ` John Harrison
0 siblings, 1 reply; 24+ messages in thread
From: Matthew Brost @ 2021-10-11 17:18 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: Daniele Ceraolo Spurio, igt-dev, john.c.harrison
On Mon, Oct 11, 2021 at 09:04:29AM +0100, Tvrtko Ursulin wrote:
>
> On 08/10/2021 18:49, Matthew Brost wrote:
> > On Wed, Oct 06, 2021 at 07:34:45PM +0100, Tvrtko Ursulin wrote:
> > >
> > > On 06/10/2021 17:41, Matthew Brost wrote:
> > > > On Wed, Oct 06, 2021 at 09:12:41AM +0100, Tvrtko Ursulin wrote:
> > > > >
> > > > > On 05/10/2021 17:44, Matthew Brost wrote:
> > > > > > On Tue, Oct 05, 2021 at 11:44:02AM +0100, Tvrtko Ursulin wrote:
> > > > > > >
> > > > > > > On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > On 9/16/2021 11:03 AM, Matthew Brost wrote:
> > > > > > > > > The i915 currently has 2k visible priority levels which are currently
> > > > > > > > > unique. This is changing to statically map these 2k levels into 3
> > > > > > > > > buckets:
> > > > > > > > >
> > > > > > > > > low: < 0
> > > > > > > > > mid: 0
> > > > > > > > > high: > 0
> > > > > > > > >
> > > > > > > > > Update gem_ctx_shared to understand this. This entails updating
> > > > > > > > > promotion test to use 3 levels that will map into different buckets and
> > > > > > > > > also add bit of delay after releasing a cork beforing completing the
> > > > > > > > > spinners to give time to the i915 schedule to process the fence and
> > > > > > > > > release and queue the requests.
> > > > > > > > >
> > > > > > > > > v2: Add a delay between starting releasing spinner and cork in
> > > > > > > > > promotion
> > > > > > > > > v3:
> > > > > > > > > (Daniele)
> > > > > > > > > - Always add delay, update commit message
> > > > > > > > >
> > > > > > > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > > > > > > ---
> > > > > > > > > tests/i915/gem_ctx_shared.c | 5 +++--
> > > > > > > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
> > > > > > > > > index ea1b5dd1b..7f88871b8 100644
> > > > > > > > > --- a/tests/i915/gem_ctx_shared.c
> > > > > > > > > +++ b/tests/i915/gem_ctx_shared.c
> > > > > > > > > @@ -622,6 +622,7 @@ static void unplug_show_queue(int i915, struct
> > > > > > > > > igt_cork *c, uint64_t ahnd,
> > > > > > > > > igt_cork_unplug(c); /* batches will now be queued on the engine */
> > > > > > > > > igt_debugfs_dump(i915, "i915_engine_info");
> > > > > > > > > + usleep(250000);
> > > > > > > >
> > > > > > > > Same as previous patch, with a comment added:
> > > > > > > >
> > > > > > > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > > > > >
> > > > > > > Come on guys, it is a bit bad and lazy for good taste no? 250ms more test
> > > > > >
> > > > > > Yea, this is way too long of a sleep. 25ms seems just fine.
> > > > >
> > > > > Until you get 1/1000 failures in CI on some platforms due phase of the moon.
> > > > > Adding sleeps should be avoided where possible.
> > > > >
> > > >
> > > > Yea, that is always a risk. Looking at this test there is already 1
> > > > other sleep in the test and in gem_exec_schedule there are 5 other
> > > > sleeps. I'm not breaking precedent here.
> > > >
> > > > > >
> > > > > > On TGL /w the updated sleep:
> > > > > > gem_ctx_shared --r *promotion*
> > > > > > IGT-Version: 1.26-g7e489b053 (x86_64) (Linux: 5.15.0-rc3-guc+ x86_64)
> > > > > > Starting subtest: Q-promotion
> > > > > > Starting dynamic subtest: rcs0
> > > > > > Dynamic subtest rcs0: SUCCESS (0.059s)
> > > > > > Starting dynamic subtest: bcs0
> > > > > > Dynamic subtest bcs0: SUCCESS (0.059s)
> > > > > > Starting dynamic subtest: vcs0
> > > > > > Dynamic subtest vcs0: SUCCESS (0.060s)
> > > > > > Starting dynamic subtest: vecs0
> > > > > > Dynamic subtest vecs0: SUCCESS (0.061s)
> > > > > > Subtest Q-promotion: SUCCESS (0.239s)
> > > > > >
> > > > > > > runtime, multiplied by number of tests and subtests (engines) will add up
> > > > > > > and over shadows the current test time. For instance current state is:
> > > > > > >
> > > > > > > # tests/gem_ctx_shared --r *promotion*
> > > > > > > IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
> > > > > > > Starting subtest: Q-promotion
> > > > > > > Starting dynamic subtest: rcs0
> > > > > > > Dynamic subtest rcs0: SUCCESS (0.174s)
> > > > > > > Starting dynamic subtest: bcs0
> > > > > > > Dynamic subtest bcs0: SUCCESS (0.224s)
> > > > > > > Starting dynamic subtest: vcs0
> > > > > > > Dynamic subtest vcs0: SUCCESS (0.153s)
> > > > > > > Starting dynamic subtest: vecs0
> > > > > > > Dynamic subtest vecs0: SUCCESS (0.153s)
> > > > > > > Subtest Q-promotion: SUCCESS (0.708s)
> > > > > > >
> > > > > > > This patch instantly makes this 1.7 seconds instead. Add the in/out order
> > > > > > > subtests, other tests, platforms with more engines, in a world where CI time
> > > > > > > budget is scarce - can't we do better than this and not settle on sprinkling
> > > > > > > of usleep all over the place?
> > > > > > >
> > > > > > > Like maybe add out fences to the two requests, merge them, and wait on that
> > > > > > > (since there is no implicit write declared so set domain does not suffice)?
> > > > > > >
> > > > > >
> > > > > > Not sure I follow. Anyways reposting now with a smaller timeout value.
> > > > > > Unless we hear something we plan on merging this tomorrow.
> > > > >
> > > > > The in/out-order test as example. It needs to wait until two dword writes
> > > > > have completed, right? I am saying pass output fences out from spinners and
> > > > > wait on them before checking content of the shared buffer.
> > > > >
> > > > > I also think sleep after uncorking is also conceptually misleading because
> > > > > that's not where the problem lies. Problem is set domain does not actually
> > > > > wait for sdw completion (existing comment even hints so "no write hazard
> > > > > lies!"). If I am right sporadic fail can in *theory* happen with execlists
> > > > > as well.
> > > > >
> > > >
> > > > Still not following what you are saying. The sleep just adds period of
> > > > time for all the uncorked requests to make it into the GuC scheduler,
> > > > without it is possible for the lower priority request (submitted first)
> > > > completes before the higher priority request (submitted after) makes it
> > > > into the GuC. The sleep ensures alls the requests are in the GuC
> > > > scheduler still stuck behind spinning requests on the hardware, then
> > > > after the spinners complete the requests are scheduled in order of
> > > > priority. Yes, this possible with execlists too but I think the timing
> > > > is different enough it doesn't happen.
> > > >
> > > > Again adding a sleep here seems legit to me, the same problem would
> > > > exist if we used fences as you suggest (e.g. the lower priority request
> > > > would be submitted first + could complete before the higher priority
> > > > request is submitted). Let's not over think this one.
> > >
> > > I don't understand how sleep _after_ uncork can have an effect you describe.
> > > Both HI and LO have been submitted and the following operation will just
> > > check the writes in memory.
> > >
> >
> > After the uncork but before releasing the spinning batches, anything
> > submitted to the GuC is still be blocked behind the spinners. A sleep
> > makes sure that all contexts submitted after the uncork are processed by
> > the GuC, sitting in the GuC scheduler, and still blocked behind the
> > spinners. Once the spinners are released now the GuC submits uncorked
> > requests in the correct order. Without this sleep, there is a race where
> > the earlier contexts (lower priority) is in the GuC scheduler but the
> > later ones (higher priority) are not. In this case the earlier (lower
> > priority) contexts get submitted and complete before the GuC sees the
> > higher priority ones causing the test to fail. It only fails like this 1
> > out of 10ish times without the sleep (i.e. it is racey). I just ran this
> > 1000x times in the loop /w the sleep and didn't see a failure.
> >
> > Make sense? Can we get this merged and move on?
>
> I am still skeptical I'm afraid. Let me try to ask you from a different
> angle and please feel free to tell me I am missing something crucial.
>
> Is the test testing uapi contract or scheduler implementation details?
>
This is testing that priority inheritance works. To be honest I have no
idea if this is concerned part of the uAPI. This could be one of those
features nobody asked for but someone on the i915 dev team thought it
would be a cool idea so it got implemented. AFAIK no other DRM driver
implents PI, at least PI certainly isn't a feature implemented in the
DRM scheduler.
> If it would be the former, then it would be highlighting the contract is
> broken. If it is the latter then why it should be fudged to run with an
> incompatible backend?
>
Regardless if PI considered part of the uAPI, there is absolutely no way
this breaking any contract. This test is racey, the sleep mitigates
(maybe even seals) the race.
> Personally I can't see that it is uapi being tested. Two unrelated contexts,
> no data dependencies, why would there be any guarantees who runs first? So
> how about just skip the test with GuC? If I am correct there may even not be
> much value to have it with execlists.
>
If PI is indeed a uAPI feature, then yes this test is providing value.
Again I have no idea why we can't merge this and move on. If this test
pops in CI we can revisit just disabling it. If we just drop PI when we
move the DRM scheduler, we can just delete this test.
Matt
> Regards,
>
> Tvrtko
>
> > > Unless the problem really is that the spinner does not start executing
> > > before the unplug? Does the sleep before unplug work as well?
> > >
> >
> > No see above. This doesn't help at all.
> >
> > Matt
> >
> > > Regards,
> > >
> > > Tvrtko
> > >
> > >
> > > >
> > > > Matt
> > > >
> > > > > Regards,
> > > > >
> > > > > Tvrtko
> > > > >
> > > > > >
> > > > > > Matt
> > > > > >
> > > > > > > Unless I am missing something it would be strictly correct for execlists
> > > > > > > backend as well since it now works just because it is fast enough.
> > > > > > >
> > > > > > > Regards,
> > > > > > >
> > > > > > > Tvrtko
> > > > > > >
> > > > > > > >
> > > > > > > > Daniele
> > > > > > > >
> > > > > > > > > for (int n = 0; n < ARRAY_SIZE(spin); n++) {
> > > > > > > > > ahnd = spin[n]->ahnd;
> > > > > > > > > @@ -831,10 +832,10 @@ static void promotion(int i915, const
> > > > > > > > > intel_ctx_cfg_t *cfg, unsigned ring)
> > > > > > > > > gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
> > > > > > > > > ctx[HI] = intel_ctx_create(i915, &q_cfg);
> > > > > > > > > - gem_context_set_priority(i915, ctx[HI]->id, 0);
> > > > > > > > > + gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
> > > > > > > > > ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
> > > > > > > > > - gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
> > > > > > > > > + gem_context_set_priority(i915, ctx[NOISE]->id, 0);
> > > > > > > > > result = gem_create(i915, 4096);
> > > > > > > > > result_offset = get_offset(ahnd, result, 4096, 0);
> > > > > > > >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-10-11 17:18 ` Matthew Brost
@ 2021-10-11 18:50 ` John Harrison
2021-10-12 8:02 ` Tvrtko Ursulin
0 siblings, 1 reply; 24+ messages in thread
From: John Harrison @ 2021-10-11 18:50 UTC (permalink / raw)
To: Matthew Brost, Tvrtko Ursulin; +Cc: Daniele Ceraolo Spurio, igt-dev
On 10/11/2021 10:18, Matthew Brost wrote:
> On Mon, Oct 11, 2021 at 09:04:29AM +0100, Tvrtko Ursulin wrote:
>> On 08/10/2021 18:49, Matthew Brost wrote:
>>> On Wed, Oct 06, 2021 at 07:34:45PM +0100, Tvrtko Ursulin wrote:
>>>> On 06/10/2021 17:41, Matthew Brost wrote:
>>>>> On Wed, Oct 06, 2021 at 09:12:41AM +0100, Tvrtko Ursulin wrote:
>>>>>> On 05/10/2021 17:44, Matthew Brost wrote:
>>>>>>> On Tue, Oct 05, 2021 at 11:44:02AM +0100, Tvrtko Ursulin wrote:
>>>>>>>> On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
>>>>>>>>> On 9/16/2021 11:03 AM, Matthew Brost wrote:
>>>>>>>>>> The i915 currently has 2k visible priority levels which are currently
>>>>>>>>>> unique. This is changing to statically map these 2k levels into 3
>>>>>>>>>> buckets:
>>>>>>>>>>
>>>>>>>>>> low: < 0
>>>>>>>>>> mid: 0
>>>>>>>>>> high: > 0
>>>>>>>>>>
>>>>>>>>>> Update gem_ctx_shared to understand this. This entails updating
>>>>>>>>>> promotion test to use 3 levels that will map into different buckets and
>>>>>>>>>> also add bit of delay after releasing a cork beforing completing the
>>>>>>>>>> spinners to give time to the i915 schedule to process the fence and
>>>>>>>>>> release and queue the requests.
>>>>>>>>>>
>>>>>>>>>> v2: Add a delay between starting releasing spinner and cork in
>>>>>>>>>> promotion
>>>>>>>>>> v3:
>>>>>>>>>> (Daniele)
>>>>>>>>>> - Always add delay, update commit message
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>>>>>>>>> ---
>>>>>>>>>> tests/i915/gem_ctx_shared.c | 5 +++--
>>>>>>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
>>>>>>>>>> index ea1b5dd1b..7f88871b8 100644
>>>>>>>>>> --- a/tests/i915/gem_ctx_shared.c
>>>>>>>>>> +++ b/tests/i915/gem_ctx_shared.c
>>>>>>>>>> @@ -622,6 +622,7 @@ static void unplug_show_queue(int i915, struct
>>>>>>>>>> igt_cork *c, uint64_t ahnd,
>>>>>>>>>> igt_cork_unplug(c); /* batches will now be queued on the engine */
>>>>>>>>>> igt_debugfs_dump(i915, "i915_engine_info");
>>>>>>>>>> + usleep(250000);
>>>>>>>>> Same as previous patch, with a comment added:
>>>>>>>>>
>>>>>>>>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>>>>>> Come on guys, it is a bit bad and lazy for good taste no? 250ms more test
>>>>>>> Yea, this is way too long of a sleep. 25ms seems just fine.
>>>>>> Until you get 1/1000 failures in CI on some platforms due phase of the moon.
>>>>>> Adding sleeps should be avoided where possible.
>>>>>>
>>>>> Yea, that is always a risk. Looking at this test there is already 1
>>>>> other sleep in the test and in gem_exec_schedule there are 5 other
>>>>> sleeps. I'm not breaking precedent here.
>>>>>
>>>>>>> On TGL /w the updated sleep:
>>>>>>> gem_ctx_shared --r *promotion*
>>>>>>> IGT-Version: 1.26-g7e489b053 (x86_64) (Linux: 5.15.0-rc3-guc+ x86_64)
>>>>>>> Starting subtest: Q-promotion
>>>>>>> Starting dynamic subtest: rcs0
>>>>>>> Dynamic subtest rcs0: SUCCESS (0.059s)
>>>>>>> Starting dynamic subtest: bcs0
>>>>>>> Dynamic subtest bcs0: SUCCESS (0.059s)
>>>>>>> Starting dynamic subtest: vcs0
>>>>>>> Dynamic subtest vcs0: SUCCESS (0.060s)
>>>>>>> Starting dynamic subtest: vecs0
>>>>>>> Dynamic subtest vecs0: SUCCESS (0.061s)
>>>>>>> Subtest Q-promotion: SUCCESS (0.239s)
>>>>>>>
>>>>>>>> runtime, multiplied by number of tests and subtests (engines) will add up
>>>>>>>> and over shadows the current test time. For instance current state is:
>>>>>>>>
>>>>>>>> # tests/gem_ctx_shared --r *promotion*
>>>>>>>> IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
>>>>>>>> Starting subtest: Q-promotion
>>>>>>>> Starting dynamic subtest: rcs0
>>>>>>>> Dynamic subtest rcs0: SUCCESS (0.174s)
>>>>>>>> Starting dynamic subtest: bcs0
>>>>>>>> Dynamic subtest bcs0: SUCCESS (0.224s)
>>>>>>>> Starting dynamic subtest: vcs0
>>>>>>>> Dynamic subtest vcs0: SUCCESS (0.153s)
>>>>>>>> Starting dynamic subtest: vecs0
>>>>>>>> Dynamic subtest vecs0: SUCCESS (0.153s)
>>>>>>>> Subtest Q-promotion: SUCCESS (0.708s)
>>>>>>>>
>>>>>>>> This patch instantly makes this 1.7 seconds instead. Add the in/out order
>>>>>>>> subtests, other tests, platforms with more engines, in a world where CI time
>>>>>>>> budget is scarce - can't we do better than this and not settle on sprinkling
>>>>>>>> of usleep all over the place?
>>>>>>>>
>>>>>>>> Like maybe add out fences to the two requests, merge them, and wait on that
>>>>>>>> (since there is no implicit write declared so set domain does not suffice)?
>>>>>>>>
>>>>>>> Not sure I follow. Anyways reposting now with a smaller timeout value.
>>>>>>> Unless we hear something we plan on merging this tomorrow.
>>>>>> The in/out-order test as example. It needs to wait until two dword writes
>>>>>> have completed, right? I am saying pass output fences out from spinners and
>>>>>> wait on them before checking content of the shared buffer.
>>>>>>
>>>>>> I also think sleep after uncorking is also conceptually misleading because
>>>>>> that's not where the problem lies. Problem is set domain does not actually
>>>>>> wait for sdw completion (existing comment even hints so "no write hazard
>>>>>> lies!"). If I am right sporadic fail can in *theory* happen with execlists
>>>>>> as well.
>>>>>>
>>>>> Still not following what you are saying. The sleep just adds period of
>>>>> time for all the uncorked requests to make it into the GuC scheduler,
>>>>> without it is possible for the lower priority request (submitted first)
>>>>> completes before the higher priority request (submitted after) makes it
>>>>> into the GuC. The sleep ensures alls the requests are in the GuC
>>>>> scheduler still stuck behind spinning requests on the hardware, then
>>>>> after the spinners complete the requests are scheduled in order of
>>>>> priority. Yes, this possible with execlists too but I think the timing
>>>>> is different enough it doesn't happen.
>>>>>
>>>>> Again adding a sleep here seems legit to me, the same problem would
>>>>> exist if we used fences as you suggest (e.g. the lower priority request
>>>>> would be submitted first + could complete before the higher priority
>>>>> request is submitted). Let's not over think this one.
>>>> I don't understand how sleep _after_ uncork can have an effect you describe.
>>>> Both HI and LO have been submitted and the following operation will just
>>>> check the writes in memory.
>>>>
>>> After the uncork but before releasing the spinning batches, anything
>>> submitted to the GuC is still be blocked behind the spinners. A sleep
>>> makes sure that all contexts submitted after the uncork are processed by
>>> the GuC, sitting in the GuC scheduler, and still blocked behind the
>>> spinners. Once the spinners are released now the GuC submits uncorked
>>> requests in the correct order. Without this sleep, there is a race where
>>> the earlier contexts (lower priority) is in the GuC scheduler but the
>>> later ones (higher priority) are not. In this case the earlier (lower
>>> priority) contexts get submitted and complete before the GuC sees the
>>> higher priority ones causing the test to fail. It only fails like this 1
>>> out of 10ish times without the sleep (i.e. it is racey). I just ran this
>>> 1000x times in the loop /w the sleep and didn't see a failure.
>>>
>>> Make sense? Can we get this merged and move on?
>> I am still skeptical I'm afraid. Let me try to ask you from a different
>> angle and please feel free to tell me I am missing something crucial.
>>
>> Is the test testing uapi contract or scheduler implementation details?
>>
> This is testing that priority inheritance works. To be honest I have no
> idea if this is concerned part of the uAPI. This could be one of those
> features nobody asked for but someone on the i915 dev team thought it
> would be a cool idea so it got implemented. AFAIK no other DRM driver
> implents PI, at least PI certainly isn't a feature implemented in the
> DRM scheduler.
>
>> If it would be the former, then it would be highlighting the contract is
>> broken. If it is the latter then why it should be fudged to run with an
>> incompatible backend?
>>
> Regardless if PI considered part of the uAPI, there is absolutely no way
> this breaking any contract. This test is racey, the sleep mitigates
> (maybe even seals) the race.
>
>> Personally I can't see that it is uapi being tested. Two unrelated contexts,
>> no data dependencies, why would there be any guarantees who runs first? So
>> how about just skip the test with GuC? If I am correct there may even not be
>> much value to have it with execlists.
>>
> If PI is indeed a uAPI feature, then yes this test is providing value.
> Again I have no idea why we can't merge this and move on. If this test
> pops in CI we can revisit just disabling it. If we just drop PI when we
> move the DRM scheduler, we can just delete this test.
>
> Matt
I believe the point of PI is to prevent PI. That is, inheriting
priorities prevents priority inversion where a high priority request is
blocked waiting for a low priority request to complete. That would
happen quite easily with the hardware compositor in Android some time
back. I have no idea if that is still a real world concern now, but it
certainly was back around 2015 on VLV for certain customers.
I would view this as an artificial test trying to emulate a real world
race condition. As in, genuine applications could hit this but it takes
multiple applications running in parallel with effectively random timing
between them. In order to recreate that race in a synthetic test, we
have to force certain behaviours by doing what could be considered
unrealistic operations. For example, setting up a pointless opening
situations using infinite loop batch buffers and sleeps. Sure, totally
unrealistic, but this is a synthetic test emulating a situation that
would be very difficult to recreate faithfully.
So stop worrying about how realistic the test is. It can't ever be
realistic. Let it do what it needs to do to exercise the problem code
path and call it good.
John.
>
>> Regards,
>>
>> Tvrtko
>>
>>>> Unless the problem really is that the spinner does not start executing
>>>> before the unplug? Does the sleep before unplug work as well?
>>>>
>>> No see above. This doesn't help at all.
>>>
>>> Matt
>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>
>>>>> Matt
>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> Tvrtko
>>>>>>
>>>>>>> Matt
>>>>>>>
>>>>>>>> Unless I am missing something it would be strictly correct for execlists
>>>>>>>> backend as well since it now works just because it is fast enough.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>>
>>>>>>>> Tvrtko
>>>>>>>>
>>>>>>>>> Daniele
>>>>>>>>>
>>>>>>>>>> for (int n = 0; n < ARRAY_SIZE(spin); n++) {
>>>>>>>>>> ahnd = spin[n]->ahnd;
>>>>>>>>>> @@ -831,10 +832,10 @@ static void promotion(int i915, const
>>>>>>>>>> intel_ctx_cfg_t *cfg, unsigned ring)
>>>>>>>>>> gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
>>>>>>>>>> ctx[HI] = intel_ctx_create(i915, &q_cfg);
>>>>>>>>>> - gem_context_set_priority(i915, ctx[HI]->id, 0);
>>>>>>>>>> + gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
>>>>>>>>>> ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
>>>>>>>>>> - gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
>>>>>>>>>> + gem_context_set_priority(i915, ctx[NOISE]->id, 0);
>>>>>>>>>> result = gem_create(i915, 4096);
>>>>>>>>>> result_offset = get_offset(ahnd, result, 4096, 0);
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-10-11 18:50 ` John Harrison
@ 2021-10-12 8:02 ` Tvrtko Ursulin
2021-10-12 17:20 ` Matthew Brost
0 siblings, 1 reply; 24+ messages in thread
From: Tvrtko Ursulin @ 2021-10-12 8:02 UTC (permalink / raw)
To: John Harrison, Matthew Brost; +Cc: Daniele Ceraolo Spurio, igt-dev
On 11/10/2021 19:50, John Harrison wrote:
> On 10/11/2021 10:18, Matthew Brost wrote:
>> On Mon, Oct 11, 2021 at 09:04:29AM +0100, Tvrtko Ursulin wrote:
>>> On 08/10/2021 18:49, Matthew Brost wrote:
>>>> On Wed, Oct 06, 2021 at 07:34:45PM +0100, Tvrtko Ursulin wrote:
>>>>> On 06/10/2021 17:41, Matthew Brost wrote:
>>>>>> On Wed, Oct 06, 2021 at 09:12:41AM +0100, Tvrtko Ursulin wrote:
>>>>>>> On 05/10/2021 17:44, Matthew Brost wrote:
>>>>>>>> On Tue, Oct 05, 2021 at 11:44:02AM +0100, Tvrtko Ursulin wrote:
>>>>>>>>> On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
>>>>>>>>>> On 9/16/2021 11:03 AM, Matthew Brost wrote:
>>>>>>>>>>> The i915 currently has 2k visible priority levels which are
>>>>>>>>>>> currently
>>>>>>>>>>> unique. This is changing to statically map these 2k levels
>>>>>>>>>>> into 3
>>>>>>>>>>> buckets:
>>>>>>>>>>>
>>>>>>>>>>> low: < 0
>>>>>>>>>>> mid: 0
>>>>>>>>>>> high: > 0
>>>>>>>>>>>
>>>>>>>>>>> Update gem_ctx_shared to understand this. This entails updating
>>>>>>>>>>> promotion test to use 3 levels that will map into different
>>>>>>>>>>> buckets and
>>>>>>>>>>> also add bit of delay after releasing a cork beforing
>>>>>>>>>>> completing the
>>>>>>>>>>> spinners to give time to the i915 schedule to process the
>>>>>>>>>>> fence and
>>>>>>>>>>> release and queue the requests.
>>>>>>>>>>>
>>>>>>>>>>> v2: Add a delay between starting releasing spinner and cork in
>>>>>>>>>>> promotion
>>>>>>>>>>> v3:
>>>>>>>>>>> (Daniele)
>>>>>>>>>>> - Always add delay, update commit message
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>>>>>>>>>> ---
>>>>>>>>>>> tests/i915/gem_ctx_shared.c | 5 +++--
>>>>>>>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/tests/i915/gem_ctx_shared.c
>>>>>>>>>>> b/tests/i915/gem_ctx_shared.c
>>>>>>>>>>> index ea1b5dd1b..7f88871b8 100644
>>>>>>>>>>> --- a/tests/i915/gem_ctx_shared.c
>>>>>>>>>>> +++ b/tests/i915/gem_ctx_shared.c
>>>>>>>>>>> @@ -622,6 +622,7 @@ static void unplug_show_queue(int i915,
>>>>>>>>>>> struct
>>>>>>>>>>> igt_cork *c, uint64_t ahnd,
>>>>>>>>>>> igt_cork_unplug(c); /* batches will now be queued
>>>>>>>>>>> on the engine */
>>>>>>>>>>> igt_debugfs_dump(i915, "i915_engine_info");
>>>>>>>>>>> + usleep(250000);
>>>>>>>>>> Same as previous patch, with a comment added:
>>>>>>>>>>
>>>>>>>>>> Reviewed-by: Daniele Ceraolo Spurio
>>>>>>>>>> <daniele.ceraolospurio@intel.com>
>>>>>>>>> Come on guys, it is a bit bad and lazy for good taste no? 250ms
>>>>>>>>> more test
>>>>>>>> Yea, this is way too long of a sleep. 25ms seems just fine.
>>>>>>> Until you get 1/1000 failures in CI on some platforms due phase
>>>>>>> of the moon.
>>>>>>> Adding sleeps should be avoided where possible.
>>>>>>>
>>>>>> Yea, that is always a risk. Looking at this test there is already 1
>>>>>> other sleep in the test and in gem_exec_schedule there are 5 other
>>>>>> sleeps. I'm not breaking precedent here.
>>>>>>
>>>>>>>> On TGL /w the updated sleep:
>>>>>>>> gem_ctx_shared --r *promotion*
>>>>>>>> IGT-Version: 1.26-g7e489b053 (x86_64) (Linux: 5.15.0-rc3-guc+
>>>>>>>> x86_64)
>>>>>>>> Starting subtest: Q-promotion
>>>>>>>> Starting dynamic subtest: rcs0
>>>>>>>> Dynamic subtest rcs0: SUCCESS (0.059s)
>>>>>>>> Starting dynamic subtest: bcs0
>>>>>>>> Dynamic subtest bcs0: SUCCESS (0.059s)
>>>>>>>> Starting dynamic subtest: vcs0
>>>>>>>> Dynamic subtest vcs0: SUCCESS (0.060s)
>>>>>>>> Starting dynamic subtest: vecs0
>>>>>>>> Dynamic subtest vecs0: SUCCESS (0.061s)
>>>>>>>> Subtest Q-promotion: SUCCESS (0.239s)
>>>>>>>>
>>>>>>>>> runtime, multiplied by number of tests and subtests (engines)
>>>>>>>>> will add up
>>>>>>>>> and over shadows the current test time. For instance current
>>>>>>>>> state is:
>>>>>>>>>
>>>>>>>>> # tests/gem_ctx_shared --r *promotion*
>>>>>>>>> IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
>>>>>>>>> Starting subtest: Q-promotion
>>>>>>>>> Starting dynamic subtest: rcs0
>>>>>>>>> Dynamic subtest rcs0: SUCCESS (0.174s)
>>>>>>>>> Starting dynamic subtest: bcs0
>>>>>>>>> Dynamic subtest bcs0: SUCCESS (0.224s)
>>>>>>>>> Starting dynamic subtest: vcs0
>>>>>>>>> Dynamic subtest vcs0: SUCCESS (0.153s)
>>>>>>>>> Starting dynamic subtest: vecs0
>>>>>>>>> Dynamic subtest vecs0: SUCCESS (0.153s)
>>>>>>>>> Subtest Q-promotion: SUCCESS (0.708s)
>>>>>>>>>
>>>>>>>>> This patch instantly makes this 1.7 seconds instead. Add the
>>>>>>>>> in/out order
>>>>>>>>> subtests, other tests, platforms with more engines, in a world
>>>>>>>>> where CI time
>>>>>>>>> budget is scarce - can't we do better than this and not settle
>>>>>>>>> on sprinkling
>>>>>>>>> of usleep all over the place?
>>>>>>>>>
>>>>>>>>> Like maybe add out fences to the two requests, merge them, and
>>>>>>>>> wait on that
>>>>>>>>> (since there is no implicit write declared so set domain does
>>>>>>>>> not suffice)?
>>>>>>>>>
>>>>>>>> Not sure I follow. Anyways reposting now with a smaller timeout
>>>>>>>> value.
>>>>>>>> Unless we hear something we plan on merging this tomorrow.
>>>>>>> The in/out-order test as example. It needs to wait until two
>>>>>>> dword writes
>>>>>>> have completed, right? I am saying pass output fences out from
>>>>>>> spinners and
>>>>>>> wait on them before checking content of the shared buffer.
>>>>>>>
>>>>>>> I also think sleep after uncorking is also conceptually
>>>>>>> misleading because
>>>>>>> that's not where the problem lies. Problem is set domain does not
>>>>>>> actually
>>>>>>> wait for sdw completion (existing comment even hints so "no write
>>>>>>> hazard
>>>>>>> lies!"). If I am right sporadic fail can in *theory* happen with
>>>>>>> execlists
>>>>>>> as well.
>>>>>>>
>>>>>> Still not following what you are saying. The sleep just adds
>>>>>> period of
>>>>>> time for all the uncorked requests to make it into the GuC scheduler,
>>>>>> without it is possible for the lower priority request (submitted
>>>>>> first)
>>>>>> completes before the higher priority request (submitted after)
>>>>>> makes it
>>>>>> into the GuC. The sleep ensures alls the requests are in the GuC
>>>>>> scheduler still stuck behind spinning requests on the hardware, then
>>>>>> after the spinners complete the requests are scheduled in order of
>>>>>> priority. Yes, this possible with execlists too but I think the
>>>>>> timing
>>>>>> is different enough it doesn't happen.
>>>>>>
>>>>>> Again adding a sleep here seems legit to me, the same problem would
>>>>>> exist if we used fences as you suggest (e.g. the lower priority
>>>>>> request
>>>>>> would be submitted first + could complete before the higher priority
>>>>>> request is submitted). Let's not over think this one.
>>>>> I don't understand how sleep _after_ uncork can have an effect you
>>>>> describe.
>>>>> Both HI and LO have been submitted and the following operation will
>>>>> just
>>>>> check the writes in memory.
>>>>>
>>>> After the uncork but before releasing the spinning batches, anything
>>>> submitted to the GuC is still be blocked behind the spinners. A sleep
>>>> makes sure that all contexts submitted after the uncork are
>>>> processed by
>>>> the GuC, sitting in the GuC scheduler, and still blocked behind the
>>>> spinners. Once the spinners are released now the GuC submits uncorked
>>>> requests in the correct order. Without this sleep, there is a race
>>>> where
>>>> the earlier contexts (lower priority) is in the GuC scheduler but the
>>>> later ones (higher priority) are not. In this case the earlier (lower
>>>> priority) contexts get submitted and complete before the GuC sees the
>>>> higher priority ones causing the test to fail. It only fails like
>>>> this 1
>>>> out of 10ish times without the sleep (i.e. it is racey). I just ran
>>>> this
>>>> 1000x times in the loop /w the sleep and didn't see a failure.
>>>>
>>>> Make sense? Can we get this merged and move on?
>>> I am still skeptical I'm afraid. Let me try to ask you from a different
>>> angle and please feel free to tell me I am missing something crucial.
>>>
>>> Is the test testing uapi contract or scheduler implementation details?
>>>
>> This is testing that priority inheritance works. To be honest I have no
>> idea if this is concerned part of the uAPI. This could be one of those
>> features nobody asked for but someone on the i915 dev team thought it
>> would be a cool idea so it got implemented. AFAIK no other DRM driver
>> implents PI, at least PI certainly isn't a feature implemented in the
>> DRM scheduler.
>>
>>> If it would be the former, then it would be highlighting the contract is
>>> broken. If it is the latter then why it should be fudged to run with an
>>> incompatible backend?
>>>
>> Regardless if PI considered part of the uAPI, there is absolutely no way
>> this breaking any contract. This test is racey, the sleep mitigates
>> (maybe even seals) the race.
>>> Personally I can't see that it is uapi being tested. Two unrelated
>>> contexts,
>>> no data dependencies, why would there be any guarantees who runs
>>> first? So
>>> how about just skip the test with GuC? If I am correct there may even
>>> not be
>>> much value to have it with execlists.
>>>
>> If PI is indeed a uAPI feature, then yes this test is providing value.
>> Again I have no idea why we can't merge this and move on. If this test
>> pops in CI we can revisit just disabling it. If we just drop PI when we
>> move the DRM scheduler, we can just delete this test.
>>
>> Matt
> I believe the point of PI is to prevent PI. That is, inheriting
> priorities prevents priority inversion where a high priority request is
> blocked waiting for a low priority request to complete. That would
> happen quite easily with the hardware compositor in Android some time
> back. I have no idea if that is still a real world concern now, but it
> certainly was back around 2015 on VLV for certain customers.
>
> I would view this as an artificial test trying to emulate a real world
> race condition. As in, genuine applications could hit this but it takes
> multiple applications running in parallel with effectively random timing
> between them. In order to recreate that race in a synthetic test, we
> have to force certain behaviours by doing what could be considered
> unrealistic operations. For example, setting up a pointless opening
> situations using infinite loop batch buffers and sleeps. Sure, totally
> unrealistic, but this is a synthetic test emulating a situation that
> would be very difficult to recreate faithfully.
>
> So stop worrying about how realistic the test is. It can't ever be
> realistic. Let it do what it needs to do to exercise the problem code
> path and call it good.
I am talking about gem_exec_shared@reorder here to be clear. Are we all
on the same page?
There all I see are two GPU _readers_, from different contexts, and a
shared buffer object. So I really don't see why would kernel have to
enforce any ordering between the two regardless of the priorities?
Please someone say in plain words I am missing something crucial? If I
am not, then I think test is invalid and solution is not to add sleeps
to it but to remove the test. At least from the GuC execution if someone
wants to argue exercising execlists backend implementation details has
value from this test (it's not even gem_exec_schedule).
Regards,
Tvrtko
>
> John.
>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>> Unless the problem really is that the spinner does not start executing
>>>>> before the unplug? Does the sleep before unplug work as well?
>>>>>
>>>> No see above. This doesn't help at all.
>>>>
>>>> Matt
>>>>
>>>>> Regards,
>>>>>
>>>>> Tvrtko
>>>>>
>>>>>
>>>>>> Matt
>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> Tvrtko
>>>>>>>
>>>>>>>> Matt
>>>>>>>>
>>>>>>>>> Unless I am missing something it would be strictly correct for
>>>>>>>>> execlists
>>>>>>>>> backend as well since it now works just because it is fast enough.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>>
>>>>>>>>> Tvrtko
>>>>>>>>>
>>>>>>>>>> Daniele
>>>>>>>>>>
>>>>>>>>>>> for (int n = 0; n < ARRAY_SIZE(spin); n++) {
>>>>>>>>>>> ahnd = spin[n]->ahnd;
>>>>>>>>>>> @@ -831,10 +832,10 @@ static void promotion(int i915, const
>>>>>>>>>>> intel_ctx_cfg_t *cfg, unsigned ring)
>>>>>>>>>>> gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
>>>>>>>>>>> ctx[HI] = intel_ctx_create(i915, &q_cfg);
>>>>>>>>>>> - gem_context_set_priority(i915, ctx[HI]->id, 0);
>>>>>>>>>>> + gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
>>>>>>>>>>> ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
>>>>>>>>>>> - gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
>>>>>>>>>>> + gem_context_set_priority(i915, ctx[NOISE]->id, 0);
>>>>>>>>>>> result = gem_create(i915, 4096);
>>>>>>>>>>> result_offset = get_offset(ahnd, result, 4096, 0);
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-10-12 8:02 ` Tvrtko Ursulin
@ 2021-10-12 17:20 ` Matthew Brost
2021-10-13 14:13 ` Tvrtko Ursulin
0 siblings, 1 reply; 24+ messages in thread
From: Matthew Brost @ 2021-10-12 17:20 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: John Harrison, Daniele Ceraolo Spurio, igt-dev
On Tue, Oct 12, 2021 at 09:02:17AM +0100, Tvrtko Ursulin wrote:
>
> On 11/10/2021 19:50, John Harrison wrote:
> > On 10/11/2021 10:18, Matthew Brost wrote:
> > > On Mon, Oct 11, 2021 at 09:04:29AM +0100, Tvrtko Ursulin wrote:
> > > > On 08/10/2021 18:49, Matthew Brost wrote:
> > > > > On Wed, Oct 06, 2021 at 07:34:45PM +0100, Tvrtko Ursulin wrote:
> > > > > > On 06/10/2021 17:41, Matthew Brost wrote:
> > > > > > > On Wed, Oct 06, 2021 at 09:12:41AM +0100, Tvrtko Ursulin wrote:
> > > > > > > > On 05/10/2021 17:44, Matthew Brost wrote:
> > > > > > > > > On Tue, Oct 05, 2021 at 11:44:02AM +0100, Tvrtko Ursulin wrote:
> > > > > > > > > > On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
> > > > > > > > > > > On 9/16/2021 11:03 AM, Matthew Brost wrote:
> > > > > > > > > > > > The i915 currently has 2k
> > > > > > > > > > > > visible priority levels which
> > > > > > > > > > > > are currently
> > > > > > > > > > > > unique. This is changing to
> > > > > > > > > > > > statically map these 2k levels
> > > > > > > > > > > > into 3
> > > > > > > > > > > > buckets:
> > > > > > > > > > > >
> > > > > > > > > > > > low: < 0
> > > > > > > > > > > > mid: 0
> > > > > > > > > > > > high: > 0
> > > > > > > > > > > >
> > > > > > > > > > > > Update gem_ctx_shared to understand this. This entails updating
> > > > > > > > > > > > promotion test to use 3 levels
> > > > > > > > > > > > that will map into different
> > > > > > > > > > > > buckets and
> > > > > > > > > > > > also add bit of delay after
> > > > > > > > > > > > releasing a cork beforing
> > > > > > > > > > > > completing the
> > > > > > > > > > > > spinners to give time to the
> > > > > > > > > > > > i915 schedule to process the
> > > > > > > > > > > > fence and
> > > > > > > > > > > > release and queue the requests.
> > > > > > > > > > > >
> > > > > > > > > > > > v2: Add a delay between starting releasing spinner and cork in
> > > > > > > > > > > > promotion
> > > > > > > > > > > > v3:
> > > > > > > > > > > > (Daniele)
> > > > > > > > > > > > - Always add delay, update commit message
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > > tests/i915/gem_ctx_shared.c | 5 +++--
> > > > > > > > > > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git
> > > > > > > > > > > > a/tests/i915/gem_ctx_shared.c
> > > > > > > > > > > > b/tests/i915/gem_ctx_shared.c
> > > > > > > > > > > > index ea1b5dd1b..7f88871b8 100644
> > > > > > > > > > > > --- a/tests/i915/gem_ctx_shared.c
> > > > > > > > > > > > +++ b/tests/i915/gem_ctx_shared.c
> > > > > > > > > > > > @@ -622,6 +622,7 @@ static void
> > > > > > > > > > > > unplug_show_queue(int i915,
> > > > > > > > > > > > struct
> > > > > > > > > > > > igt_cork *c, uint64_t ahnd,
> > > > > > > > > > > > igt_cork_unplug(c); /*
> > > > > > > > > > > > batches will now be queued on
> > > > > > > > > > > > the engine */
> > > > > > > > > > > > igt_debugfs_dump(i915, "i915_engine_info");
> > > > > > > > > > > > + usleep(250000);
> > > > > > > > > > > Same as previous patch, with a comment added:
> > > > > > > > > > >
> > > > > > > > > > > Reviewed-by: Daniele Ceraolo Spurio
> > > > > > > > > > > <daniele.ceraolospurio@intel.com>
> > > > > > > > > > Come on guys, it is a bit bad and lazy
> > > > > > > > > > for good taste no? 250ms more test
> > > > > > > > > Yea, this is way too long of a sleep. 25ms seems just fine.
> > > > > > > > Until you get 1/1000 failures in CI on some
> > > > > > > > platforms due phase of the moon.
> > > > > > > > Adding sleeps should be avoided where possible.
> > > > > > > >
> > > > > > > Yea, that is always a risk. Looking at this test there is already 1
> > > > > > > other sleep in the test and in gem_exec_schedule there are 5 other
> > > > > > > sleeps. I'm not breaking precedent here.
> > > > > > >
> > > > > > > > > On TGL /w the updated sleep:
> > > > > > > > > gem_ctx_shared --r *promotion*
> > > > > > > > > IGT-Version: 1.26-g7e489b053 (x86_64)
> > > > > > > > > (Linux: 5.15.0-rc3-guc+ x86_64)
> > > > > > > > > Starting subtest: Q-promotion
> > > > > > > > > Starting dynamic subtest: rcs0
> > > > > > > > > Dynamic subtest rcs0: SUCCESS (0.059s)
> > > > > > > > > Starting dynamic subtest: bcs0
> > > > > > > > > Dynamic subtest bcs0: SUCCESS (0.059s)
> > > > > > > > > Starting dynamic subtest: vcs0
> > > > > > > > > Dynamic subtest vcs0: SUCCESS (0.060s)
> > > > > > > > > Starting dynamic subtest: vecs0
> > > > > > > > > Dynamic subtest vecs0: SUCCESS (0.061s)
> > > > > > > > > Subtest Q-promotion: SUCCESS (0.239s)
> > > > > > > > >
> > > > > > > > > > runtime, multiplied by number of tests
> > > > > > > > > > and subtests (engines) will add up
> > > > > > > > > > and over shadows the current test time.
> > > > > > > > > > For instance current state is:
> > > > > > > > > >
> > > > > > > > > > # tests/gem_ctx_shared --r *promotion*
> > > > > > > > > > IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
> > > > > > > > > > Starting subtest: Q-promotion
> > > > > > > > > > Starting dynamic subtest: rcs0
> > > > > > > > > > Dynamic subtest rcs0: SUCCESS (0.174s)
> > > > > > > > > > Starting dynamic subtest: bcs0
> > > > > > > > > > Dynamic subtest bcs0: SUCCESS (0.224s)
> > > > > > > > > > Starting dynamic subtest: vcs0
> > > > > > > > > > Dynamic subtest vcs0: SUCCESS (0.153s)
> > > > > > > > > > Starting dynamic subtest: vecs0
> > > > > > > > > > Dynamic subtest vecs0: SUCCESS (0.153s)
> > > > > > > > > > Subtest Q-promotion: SUCCESS (0.708s)
> > > > > > > > > >
> > > > > > > > > > This patch instantly makes this 1.7
> > > > > > > > > > seconds instead. Add the in/out order
> > > > > > > > > > subtests, other tests, platforms with
> > > > > > > > > > more engines, in a world where CI time
> > > > > > > > > > budget is scarce - can't we do better
> > > > > > > > > > than this and not settle on sprinkling
> > > > > > > > > > of usleep all over the place?
> > > > > > > > > >
> > > > > > > > > > Like maybe add out fences to the two
> > > > > > > > > > requests, merge them, and wait on that
> > > > > > > > > > (since there is no implicit write
> > > > > > > > > > declared so set domain does not
> > > > > > > > > > suffice)?
> > > > > > > > > >
> > > > > > > > > Not sure I follow. Anyways reposting now
> > > > > > > > > with a smaller timeout value.
> > > > > > > > > Unless we hear something we plan on merging this tomorrow.
> > > > > > > > The in/out-order test as example. It needs to
> > > > > > > > wait until two dword writes
> > > > > > > > have completed, right? I am saying pass output
> > > > > > > > fences out from spinners and
> > > > > > > > wait on them before checking content of the shared buffer.
> > > > > > > >
> > > > > > > > I also think sleep after uncorking is also
> > > > > > > > conceptually misleading because
> > > > > > > > that's not where the problem lies. Problem is
> > > > > > > > set domain does not actually
> > > > > > > > wait for sdw completion (existing comment even
> > > > > > > > hints so "no write hazard
> > > > > > > > lies!"). If I am right sporadic fail can in
> > > > > > > > *theory* happen with execlists
> > > > > > > > as well.
> > > > > > > >
> > > > > > > Still not following what you are saying. The sleep
> > > > > > > just adds period of
> > > > > > > time for all the uncorked requests to make it into the GuC scheduler,
> > > > > > > without it is possible for the lower priority
> > > > > > > request (submitted first)
> > > > > > > completes before the higher priority request
> > > > > > > (submitted after) makes it
> > > > > > > into the GuC. The sleep ensures alls the requests are in the GuC
> > > > > > > scheduler still stuck behind spinning requests on the hardware, then
> > > > > > > after the spinners complete the requests are scheduled in order of
> > > > > > > priority. Yes, this possible with execlists too but
> > > > > > > I think the timing
> > > > > > > is different enough it doesn't happen.
> > > > > > >
> > > > > > > Again adding a sleep here seems legit to me, the same problem would
> > > > > > > exist if we used fences as you suggest (e.g. the
> > > > > > > lower priority request
> > > > > > > would be submitted first + could complete before the higher priority
> > > > > > > request is submitted). Let's not over think this one.
> > > > > > I don't understand how sleep _after_ uncork can have an
> > > > > > effect you describe.
> > > > > > Both HI and LO have been submitted and the following
> > > > > > operation will just
> > > > > > check the writes in memory.
> > > > > >
> > > > > After the uncork but before releasing the spinning batches, anything
> > > > > submitted to the GuC is still be blocked behind the spinners. A sleep
> > > > > makes sure that all contexts submitted after the uncork are
> > > > > processed by
> > > > > the GuC, sitting in the GuC scheduler, and still blocked behind the
> > > > > spinners. Once the spinners are released now the GuC submits uncorked
> > > > > requests in the correct order. Without this sleep, there is
> > > > > a race where
> > > > > the earlier contexts (lower priority) is in the GuC scheduler but the
> > > > > later ones (higher priority) are not. In this case the earlier (lower
> > > > > priority) contexts get submitted and complete before the GuC sees the
> > > > > higher priority ones causing the test to fail. It only fails
> > > > > like this 1
> > > > > out of 10ish times without the sleep (i.e. it is racey). I
> > > > > just ran this
> > > > > 1000x times in the loop /w the sleep and didn't see a failure.
> > > > >
> > > > > Make sense? Can we get this merged and move on?
> > > > I am still skeptical I'm afraid. Let me try to ask you from a different
> > > > angle and please feel free to tell me I am missing something crucial.
> > > >
> > > > Is the test testing uapi contract or scheduler implementation details?
> > > >
> > > This is testing that priority inheritance works. To be honest I have no
> > > idea if this is concerned part of the uAPI. This could be one of those
> > > features nobody asked for but someone on the i915 dev team thought it
> > > would be a cool idea so it got implemented. AFAIK no other DRM driver
> > > implents PI, at least PI certainly isn't a feature implemented in the
> > > DRM scheduler.
> > >
> > > > If it would be the former, then it would be highlighting the contract is
> > > > broken. If it is the latter then why it should be fudged to run with an
> > > > incompatible backend?
> > > >
> > > Regardless if PI considered part of the uAPI, there is absolutely no way
> > > this breaking any contract. This test is racey, the sleep mitigates
> > > (maybe even seals) the race.
> > > > Personally I can't see that it is uapi being tested. Two
> > > > unrelated contexts,
> > > > no data dependencies, why would there be any guarantees who runs
> > > > first? So
> > > > how about just skip the test with GuC? If I am correct there may
> > > > even not be
> > > > much value to have it with execlists.
> > > >
> > > If PI is indeed a uAPI feature, then yes this test is providing value.
> > > Again I have no idea why we can't merge this and move on. If this test
> > > pops in CI we can revisit just disabling it. If we just drop PI when we
> > > move the DRM scheduler, we can just delete this test.
> > >
> > > Matt
> > I believe the point of PI is to prevent PI. That is, inheriting
> > priorities prevents priority inversion where a high priority request is
> > blocked waiting for a low priority request to complete. That would
> > happen quite easily with the hardware compositor in Android some time
> > back. I have no idea if that is still a real world concern now, but it
> > certainly was back around 2015 on VLV for certain customers.
> >
> > I would view this as an artificial test trying to emulate a real world
> > race condition. As in, genuine applications could hit this but it takes
> > multiple applications running in parallel with effectively random timing
> > between them. In order to recreate that race in a synthetic test, we
> > have to force certain behaviours by doing what could be considered
> > unrealistic operations. For example, setting up a pointless opening
> > situations using infinite loop batch buffers and sleeps. Sure, totally
> > unrealistic, but this is a synthetic test emulating a situation that
> > would be very difficult to recreate faithfully.
> >
> > So stop worrying about how realistic the test is. It can't ever be
> > realistic. Let it do what it needs to do to exercise the problem code
> > path and call it good.
>
> I am talking about gem_exec_shared@reorder here to be clear. Are we all on
> the same page?
>
I don't think we are on the same page. This sleep is required for
gem_exex_shared@promotion. A previous rev of this series only added the
sleep for that test but Daniele suggestion was just add it for all
tests.
> There all I see are two GPU _readers_, from different contexts, and a shared
> buffer object. So I really don't see why would kernel have to enforce any
> ordering between the two regardless of the priorities?
>
> Please someone say in plain words I am missing something crucial? If I am
> not, then I think test is invalid and solution is not to add sleeps to it
> but to remove the test. At least from the GuC execution if someone wants to
> argue exercising execlists backend implementation details has value from
> this test (it's not even gem_exec_schedule).
>
Promotion has value, I haven't looked at reorder in a while so I can't
really comment if that test has any value.
Matt
> Regards,
>
> Tvrtko
>
> >
> > John.
> >
> > > > Regards,
> > > >
> > > > Tvrtko
> > > >
> > > > > > Unless the problem really is that the spinner does not start executing
> > > > > > before the unplug? Does the sleep before unplug work as well?
> > > > > >
> > > > > No see above. This doesn't help at all.
> > > > >
> > > > > Matt
> > > > >
> > > > > > Regards,
> > > > > >
> > > > > > Tvrtko
> > > > > >
> > > > > >
> > > > > > > Matt
> > > > > > >
> > > > > > > > Regards,
> > > > > > > >
> > > > > > > > Tvrtko
> > > > > > > >
> > > > > > > > > Matt
> > > > > > > > >
> > > > > > > > > > Unless I am missing something it would
> > > > > > > > > > be strictly correct for execlists
> > > > > > > > > > backend as well since it now works just because it is fast enough.
> > > > > > > > > >
> > > > > > > > > > Regards,
> > > > > > > > > >
> > > > > > > > > > Tvrtko
> > > > > > > > > >
> > > > > > > > > > > Daniele
> > > > > > > > > > >
> > > > > > > > > > > > for (int n = 0; n < ARRAY_SIZE(spin); n++) {
> > > > > > > > > > > > ahnd = spin[n]->ahnd;
> > > > > > > > > > > > @@ -831,10 +832,10 @@ static void promotion(int i915, const
> > > > > > > > > > > > intel_ctx_cfg_t *cfg, unsigned ring)
> > > > > > > > > > > > gem_context_set_priority(i915, ctx[LO]->id, MIN_PRIO);
> > > > > > > > > > > > ctx[HI] = intel_ctx_create(i915, &q_cfg);
> > > > > > > > > > > > - gem_context_set_priority(i915, ctx[HI]->id, 0);
> > > > > > > > > > > > + gem_context_set_priority(i915, ctx[HI]->id, MAX_PRIO);
> > > > > > > > > > > > ctx[NOISE] = intel_ctx_create(i915, &q_cfg);
> > > > > > > > > > > > - gem_context_set_priority(i915, ctx[NOISE]->id, MIN_PRIO/2);
> > > > > > > > > > > > + gem_context_set_priority(i915, ctx[NOISE]->id, 0);
> > > > > > > > > > > > result = gem_create(i915, 4096);
> > > > > > > > > > > > result_offset = get_offset(ahnd, result, 4096, 0);
> >
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared understand static priority mapping
2021-10-12 17:20 ` Matthew Brost
@ 2021-10-13 14:13 ` Tvrtko Ursulin
0 siblings, 0 replies; 24+ messages in thread
From: Tvrtko Ursulin @ 2021-10-13 14:13 UTC (permalink / raw)
To: Matthew Brost; +Cc: John Harrison, Daniele Ceraolo Spurio, igt-dev
On 12/10/2021 18:20, Matthew Brost wrote:
> On Tue, Oct 12, 2021 at 09:02:17AM +0100, Tvrtko Ursulin wrote:
>>
>> On 11/10/2021 19:50, John Harrison wrote:
>>> On 10/11/2021 10:18, Matthew Brost wrote:
>>>> On Mon, Oct 11, 2021 at 09:04:29AM +0100, Tvrtko Ursulin wrote:
>>>>> On 08/10/2021 18:49, Matthew Brost wrote:
>>>>>> On Wed, Oct 06, 2021 at 07:34:45PM +0100, Tvrtko Ursulin wrote:
>>>>>>> On 06/10/2021 17:41, Matthew Brost wrote:
>>>>>>>> On Wed, Oct 06, 2021 at 09:12:41AM +0100, Tvrtko Ursulin wrote:
>>>>>>>>> On 05/10/2021 17:44, Matthew Brost wrote:
>>>>>>>>>> On Tue, Oct 05, 2021 at 11:44:02AM +0100, Tvrtko Ursulin wrote:
>>>>>>>>>>> On 05/10/2021 00:26, Daniele Ceraolo Spurio wrote:
>>>>>>>>>>>> On 9/16/2021 11:03 AM, Matthew Brost wrote:
>>>>>>>>>>>>> The i915 currently has 2k
>>>>>>>>>>>>> visible priority levels which
>>>>>>>>>>>>> are currently
>>>>>>>>>>>>> unique. This is changing to
>>>>>>>>>>>>> statically map these 2k levels
>>>>>>>>>>>>> into 3
>>>>>>>>>>>>> buckets:
>>>>>>>>>>>>>
>>>>>>>>>>>>> low: < 0
>>>>>>>>>>>>> mid: 0
>>>>>>>>>>>>> high: > 0
>>>>>>>>>>>>>
>>>>>>>>>>>>> Update gem_ctx_shared to understand this. This entails updating
>>>>>>>>>>>>> promotion test to use 3 levels
>>>>>>>>>>>>> that will map into different
>>>>>>>>>>>>> buckets and
>>>>>>>>>>>>> also add bit of delay after
>>>>>>>>>>>>> releasing a cork beforing
>>>>>>>>>>>>> completing the
>>>>>>>>>>>>> spinners to give time to the
>>>>>>>>>>>>> i915 schedule to process the
>>>>>>>>>>>>> fence and
>>>>>>>>>>>>> release and queue the requests.
>>>>>>>>>>>>>
>>>>>>>>>>>>> v2: Add a delay between starting releasing spinner and cork in
>>>>>>>>>>>>> promotion
>>>>>>>>>>>>> v3:
>>>>>>>>>>>>> (Daniele)
>>>>>>>>>>>>> - Always add delay, update commit message
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> tests/i915/gem_ctx_shared.c | 5 +++--
>>>>>>>>>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git
>>>>>>>>>>>>> a/tests/i915/gem_ctx_shared.c
>>>>>>>>>>>>> b/tests/i915/gem_ctx_shared.c
>>>>>>>>>>>>> index ea1b5dd1b..7f88871b8 100644
>>>>>>>>>>>>> --- a/tests/i915/gem_ctx_shared.c
>>>>>>>>>>>>> +++ b/tests/i915/gem_ctx_shared.c
>>>>>>>>>>>>> @@ -622,6 +622,7 @@ static void
>>>>>>>>>>>>> unplug_show_queue(int i915,
>>>>>>>>>>>>> struct
>>>>>>>>>>>>> igt_cork *c, uint64_t ahnd,
>>>>>>>>>>>>> igt_cork_unplug(c); /*
>>>>>>>>>>>>> batches will now be queued on
>>>>>>>>>>>>> the engine */
>>>>>>>>>>>>> igt_debugfs_dump(i915, "i915_engine_info");
>>>>>>>>>>>>> + usleep(250000);
>>>>>>>>>>>> Same as previous patch, with a comment added:
>>>>>>>>>>>>
>>>>>>>>>>>> Reviewed-by: Daniele Ceraolo Spurio
>>>>>>>>>>>> <daniele.ceraolospurio@intel.com>
>>>>>>>>>>> Come on guys, it is a bit bad and lazy
>>>>>>>>>>> for good taste no? 250ms more test
>>>>>>>>>> Yea, this is way too long of a sleep. 25ms seems just fine.
>>>>>>>>> Until you get 1/1000 failures in CI on some
>>>>>>>>> platforms due phase of the moon.
>>>>>>>>> Adding sleeps should be avoided where possible.
>>>>>>>>>
>>>>>>>> Yea, that is always a risk. Looking at this test there is already 1
>>>>>>>> other sleep in the test and in gem_exec_schedule there are 5 other
>>>>>>>> sleeps. I'm not breaking precedent here.
>>>>>>>>
>>>>>>>>>> On TGL /w the updated sleep:
>>>>>>>>>> gem_ctx_shared --r *promotion*
>>>>>>>>>> IGT-Version: 1.26-g7e489b053 (x86_64)
>>>>>>>>>> (Linux: 5.15.0-rc3-guc+ x86_64)
>>>>>>>>>> Starting subtest: Q-promotion
>>>>>>>>>> Starting dynamic subtest: rcs0
>>>>>>>>>> Dynamic subtest rcs0: SUCCESS (0.059s)
>>>>>>>>>> Starting dynamic subtest: bcs0
>>>>>>>>>> Dynamic subtest bcs0: SUCCESS (0.059s)
>>>>>>>>>> Starting dynamic subtest: vcs0
>>>>>>>>>> Dynamic subtest vcs0: SUCCESS (0.060s)
>>>>>>>>>> Starting dynamic subtest: vecs0
>>>>>>>>>> Dynamic subtest vecs0: SUCCESS (0.061s)
>>>>>>>>>> Subtest Q-promotion: SUCCESS (0.239s)
>>>>>>>>>>
>>>>>>>>>>> runtime, multiplied by number of tests
>>>>>>>>>>> and subtests (engines) will add up
>>>>>>>>>>> and over shadows the current test time.
>>>>>>>>>>> For instance current state is:
>>>>>>>>>>>
>>>>>>>>>>> # tests/gem_ctx_shared --r *promotion*
>>>>>>>>>>> IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.15.0-rc4 x86_64)
>>>>>>>>>>> Starting subtest: Q-promotion
>>>>>>>>>>> Starting dynamic subtest: rcs0
>>>>>>>>>>> Dynamic subtest rcs0: SUCCESS (0.174s)
>>>>>>>>>>> Starting dynamic subtest: bcs0
>>>>>>>>>>> Dynamic subtest bcs0: SUCCESS (0.224s)
>>>>>>>>>>> Starting dynamic subtest: vcs0
>>>>>>>>>>> Dynamic subtest vcs0: SUCCESS (0.153s)
>>>>>>>>>>> Starting dynamic subtest: vecs0
>>>>>>>>>>> Dynamic subtest vecs0: SUCCESS (0.153s)
>>>>>>>>>>> Subtest Q-promotion: SUCCESS (0.708s)
>>>>>>>>>>>
>>>>>>>>>>> This patch instantly makes this 1.7
>>>>>>>>>>> seconds instead. Add the in/out order
>>>>>>>>>>> subtests, other tests, platforms with
>>>>>>>>>>> more engines, in a world where CI time
>>>>>>>>>>> budget is scarce - can't we do better
>>>>>>>>>>> than this and not settle on sprinkling
>>>>>>>>>>> of usleep all over the place?
>>>>>>>>>>>
>>>>>>>>>>> Like maybe add out fences to the two
>>>>>>>>>>> requests, merge them, and wait on that
>>>>>>>>>>> (since there is no implicit write
>>>>>>>>>>> declared so set domain does not
>>>>>>>>>>> suffice)?
>>>>>>>>>>>
>>>>>>>>>> Not sure I follow. Anyways reposting now
>>>>>>>>>> with a smaller timeout value.
>>>>>>>>>> Unless we hear something we plan on merging this tomorrow.
>>>>>>>>> The in/out-order test as example. It needs to
>>>>>>>>> wait until two dword writes
>>>>>>>>> have completed, right? I am saying pass output
>>>>>>>>> fences out from spinners and
>>>>>>>>> wait on them before checking content of the shared buffer.
>>>>>>>>>
>>>>>>>>> I also think sleep after uncorking is also
>>>>>>>>> conceptually misleading because
>>>>>>>>> that's not where the problem lies. Problem is
>>>>>>>>> set domain does not actually
>>>>>>>>> wait for sdw completion (existing comment even
>>>>>>>>> hints so "no write hazard
>>>>>>>>> lies!"). If I am right sporadic fail can in
>>>>>>>>> *theory* happen with execlists
>>>>>>>>> as well.
>>>>>>>>>
>>>>>>>> Still not following what you are saying. The sleep
>>>>>>>> just adds period of
>>>>>>>> time for all the uncorked requests to make it into the GuC scheduler,
>>>>>>>> without it is possible for the lower priority
>>>>>>>> request (submitted first)
>>>>>>>> completes before the higher priority request
>>>>>>>> (submitted after) makes it
>>>>>>>> into the GuC. The sleep ensures alls the requests are in the GuC
>>>>>>>> scheduler still stuck behind spinning requests on the hardware, then
>>>>>>>> after the spinners complete the requests are scheduled in order of
>>>>>>>> priority. Yes, this possible with execlists too but
>>>>>>>> I think the timing
>>>>>>>> is different enough it doesn't happen.
>>>>>>>>
>>>>>>>> Again adding a sleep here seems legit to me, the same problem would
>>>>>>>> exist if we used fences as you suggest (e.g. the
>>>>>>>> lower priority request
>>>>>>>> would be submitted first + could complete before the higher priority
>>>>>>>> request is submitted). Let's not over think this one.
>>>>>>> I don't understand how sleep _after_ uncork can have an
>>>>>>> effect you describe.
>>>>>>> Both HI and LO have been submitted and the following
>>>>>>> operation will just
>>>>>>> check the writes in memory.
>>>>>>>
>>>>>> After the uncork but before releasing the spinning batches, anything
>>>>>> submitted to the GuC is still be blocked behind the spinners. A sleep
>>>>>> makes sure that all contexts submitted after the uncork are
>>>>>> processed by
>>>>>> the GuC, sitting in the GuC scheduler, and still blocked behind the
>>>>>> spinners. Once the spinners are released now the GuC submits uncorked
>>>>>> requests in the correct order. Without this sleep, there is
>>>>>> a race where
>>>>>> the earlier contexts (lower priority) is in the GuC scheduler but the
>>>>>> later ones (higher priority) are not. In this case the earlier (lower
>>>>>> priority) contexts get submitted and complete before the GuC sees the
>>>>>> higher priority ones causing the test to fail. It only fails
>>>>>> like this 1
>>>>>> out of 10ish times without the sleep (i.e. it is racey). I
>>>>>> just ran this
>>>>>> 1000x times in the loop /w the sleep and didn't see a failure.
>>>>>>
>>>>>> Make sense? Can we get this merged and move on?
>>>>> I am still skeptical I'm afraid. Let me try to ask you from a different
>>>>> angle and please feel free to tell me I am missing something crucial.
>>>>>
>>>>> Is the test testing uapi contract or scheduler implementation details?
>>>>>
>>>> This is testing that priority inheritance works. To be honest I have no
>>>> idea if this is concerned part of the uAPI. This could be one of those
>>>> features nobody asked for but someone on the i915 dev team thought it
>>>> would be a cool idea so it got implemented. AFAIK no other DRM driver
>>>> implents PI, at least PI certainly isn't a feature implemented in the
>>>> DRM scheduler.
>>>>
>>>>> If it would be the former, then it would be highlighting the contract is
>>>>> broken. If it is the latter then why it should be fudged to run with an
>>>>> incompatible backend?
>>>>>
>>>> Regardless if PI considered part of the uAPI, there is absolutely no way
>>>> this breaking any contract. This test is racey, the sleep mitigates
>>>> (maybe even seals) the race.
>>>>> Personally I can't see that it is uapi being tested. Two
>>>>> unrelated contexts,
>>>>> no data dependencies, why would there be any guarantees who runs
>>>>> first? So
>>>>> how about just skip the test with GuC? If I am correct there may
>>>>> even not be
>>>>> much value to have it with execlists.
>>>>>
>>>> If PI is indeed a uAPI feature, then yes this test is providing value.
>>>> Again I have no idea why we can't merge this and move on. If this test
>>>> pops in CI we can revisit just disabling it. If we just drop PI when we
>>>> move the DRM scheduler, we can just delete this test.
>>>>
>>>> Matt
>>> I believe the point of PI is to prevent PI. That is, inheriting
>>> priorities prevents priority inversion where a high priority request is
>>> blocked waiting for a low priority request to complete. That would
>>> happen quite easily with the hardware compositor in Android some time
>>> back. I have no idea if that is still a real world concern now, but it
>>> certainly was back around 2015 on VLV for certain customers.
>>>
>>> I would view this as an artificial test trying to emulate a real world
>>> race condition. As in, genuine applications could hit this but it takes
>>> multiple applications running in parallel with effectively random timing
>>> between them. In order to recreate that race in a synthetic test, we
>>> have to force certain behaviours by doing what could be considered
>>> unrealistic operations. For example, setting up a pointless opening
>>> situations using infinite loop batch buffers and sleeps. Sure, totally
>>> unrealistic, but this is a synthetic test emulating a situation that
>>> would be very difficult to recreate faithfully.
>>>
>>> So stop worrying about how realistic the test is. It can't ever be
>>> realistic. Let it do what it needs to do to exercise the problem code
>>> path and call it good.
>>
>> I am talking about gem_exec_shared@reorder here to be clear. Are we all on
>> the same page?
>>
>
> I don't think we are on the same page. This sleep is required for
> gem_exex_shared@promotion. A previous rev of this series only added the
> sleep for that test but Daniele suggestion was just add it for all
> tests.
Oh great. Okay have a look at reorder please and see what you think. But
also promotion may have a similar issue.
>> There all I see are two GPU _readers_, from different contexts, and a shared
>> buffer object. So I really don't see why would kernel have to enforce any
>> ordering between the two regardless of the priorities?
>>
>> Please someone say in plain words I am missing something crucial? If I am
>> not, then I think test is invalid and solution is not to add sleeps to it
>> but to remove the test. At least from the GuC execution if someone wants to
>> argue exercising execlists backend implementation details has value from
>> this test (it's not even gem_exec_schedule).
>>
>
> Promotion has value, I haven't looked at reorder in a while so I can't
> really comment if that test has any value.
Which asserts fails in promotion with the guc?
igt_assert_eq_u32(ptr[0], ctx[HI]->id) or,
igt_assert_eq_u32(ptr[0], ctx[NOISE]->id); ?
First one is AFAICS not questionable. It checks the reader which was
submitted second ran after the writer which was submitted first.
Second assert looks questionable to me. It checks that "noise" (lowest
priority) ran last. But "noise" is not in a data dependency chain with
"hi" and "lo" and their shared object. Is someone able to explain why
"noise" wouldn't be allowed to run first?
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2021-10-13 14:14 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-16 18:02 [igt-dev] [PATCH i-g-t 0/3] IGT fixes for priority management + preempt timeout with GuC submission Matthew Brost
2021-09-16 18:02 ` [igt-dev] [PATCH i-g-t 1/3] i915/gem_exec_schedule: Make gem_exec_schedule understand static priority mapping Matthew Brost
2021-10-04 23:24 ` Daniele Ceraolo Spurio
2021-09-16 18:03 ` [igt-dev] [PATCH i-g-t 2/3] i915/gem_ctx_shared: Make gem_ctx_shared " Matthew Brost
2021-10-04 23:26 ` Daniele Ceraolo Spurio
2021-10-05 10:44 ` Tvrtko Ursulin
2021-10-05 16:44 ` Matthew Brost
2021-10-06 8:12 ` Tvrtko Ursulin
2021-10-06 16:41 ` Matthew Brost
2021-10-06 18:34 ` Tvrtko Ursulin
2021-10-08 17:49 ` Matthew Brost
2021-10-11 8:04 ` Tvrtko Ursulin
2021-10-11 17:18 ` Matthew Brost
2021-10-11 18:50 ` John Harrison
2021-10-12 8:02 ` Tvrtko Ursulin
2021-10-12 17:20 ` Matthew Brost
2021-10-13 14:13 ` Tvrtko Ursulin
2021-09-16 18:03 ` [igt-dev] [PATCH i-g-t 3/3] i915/sysfs_preempt_timeout: Update test to work with GuC submission Matthew Brost
2021-09-16 20:49 ` John Harrison
2021-09-17 7:36 ` Tvrtko Ursulin
2021-09-17 16:14 ` Matthew Brost
2021-09-20 9:22 ` Tvrtko Ursulin
2021-09-16 19:33 ` [igt-dev] ✓ Fi.CI.BAT: success for IGT fixes for priority management + preempt timeout " Patchwork
2021-09-16 22:07 ` [igt-dev] ✗ 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.