Skip to contentSkip to conversation

gem_wsim: Fix preempt period assert

PATCH In review Tvrtko Ursulin 3 participants 3 messages Apr 27, 2020

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 gem_wsim: Fix preempt period assert

From: Tvrtko Ursulin <[email protected]>

Recently added assert in a common helper used for calculating batch duration and preemption period is harmful when preemption is disabled on a context. Split out into low level and high level helper and use the former for preemption period queries.

---
 benchmarks/gem_wsim.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
benchmarks/gem_wsim.c +12 -3
index 81f47b86d619..ad4edb936920 100644
@@ -1151,7 +1151,7 @@ __get_ctx(struct workload *wrk, const struct w_step *w)
 }
 
 static unsigned long
-get_bb_sz(const struct w_step *w, unsigned int duration)
+__get_bb_sz(const struct w_step *w, unsigned int duration)
 {
 	enum intel_engine_id engine = w->engine;
 	struct ctx *ctx = __get_ctx(w->wrk, w);
@@ -1165,6 +1165,15 @@ get_bb_sz(const struct w_step *w, unsigned int duration)
 	d = ALIGN(duration * engine_calib_map[engine] * sizeof(uint32_t) /
 		  nop_calibration_us,
 		  sizeof(uint32_t));
+
+	return d;
+}
+
+static unsigned long
+get_bb_sz(const struct w_step *w, unsigned int duration)
+{
+	unsigned long d = __get_bb_sz(w, duration);
+
 	igt_assert(d);
 
 	return d;
@@ -1174,7 +1183,7 @@ static void
 init_bb(struct w_step *w, unsigned int flags)
 {
 	const unsigned int arb_period =
-			get_bb_sz(w, w->preempt_us) / sizeof(uint32_t);
+			__get_bb_sz(w, w->preempt_us) / sizeof(uint32_t);
 	const unsigned int mmap_len = ALIGN(w->bb_sz, 4096);
 	unsigned int i;
 	uint32_t *ptr;
@@ -1395,7 +1404,7 @@ alloc_step_batch(struct workload *wrk, struct w_step *w, unsigned int flags)
 
 	if (w->unbound_duration)
 		/* nops + MI_ARB_CHK + MI_BATCH_BUFFER_START */
-		w->bb_sz = max(PAGE_SIZE, get_bb_sz(w, w->preempt_us)) +
+		w->bb_sz = max(PAGE_SIZE, __get_bb_sz(w, w->preempt_us)) +
 			   (1 + 3) * sizeof(uint32_t);
 	else
 		w->bb_sz = get_bb_sz(w, w->duration.max);
Signature · 6 lines
2.20.1

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev
Reviewed-by on v1 1/1
#1 lore raw

Quoting Tvrtko Ursulin (2020-04-27 10:00:14)

Quoted 26 lines

From: Tvrtko Ursulin <[email protected]>

Recently added assert in a common helper used for calculating batch duration and preemption period is harmful when preemption is disabled on a context. Split out into low level and high level helper and use the former for preemption period queries.

---
 benchmarks/gem_wsim.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
benchmarks/gem_wsim.c +1 -1
index 81f47b86d619..ad4edb936920 100644
@@ -1151,7 +1151,7 @@ __get_ctx(struct workload *wrk, const struct w_step *w)
 }
 
 static unsigned long
-get_bb_sz(const struct w_step *w, unsigned int duration)
+__get_bb_sz(const struct w_step *w, unsigned int duration)
 {
        enum intel_engine_id engine = w->engine;
        struct ctx *ctx = __get_ctx(w->wrk, w);

benchmarks/gem_wsim.c get_bb_sz(const struct w_step *w, unsigned int duration) · line 1167

Quoted 3 lines
benchmarks/gem_wsim.c +0 -0
@@ -1165,6 +1165,15 @@ get_bb_sz(const struct w_step *w, unsigned int duration)
        d = ALIGN(duration * engine_calib_map[engine] * sizeof(uint32_t) /
                  nop_calibration_us,
                  sizeof(uint32_t));

Preempt disabled == w->preempt_us = 0 => duration = 0.

Ok, that follows that d is expected to be 0. And the caller expands to at least a page.

Not really convinced the assert is worth it, but

-Chris
_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

#2 lore raw

[igt-dev] ✗ Fi.CI.BAT: failure for gem_wsim: Fix preempt period assert

In reply to Tvrtko Ursulin
== Series Details ==

== Series Details ==

Series: gem_wsim: Fix preempt period assert
URL : https://patchwork.freedesktop.org/series/76544/
State : failure

== Summary ==

CI Bug Log - changes from IGT_5612 -> IGTPW_4509
====================================================

Summary
-------

**FAILURE**

Serious unknown changes coming with IGTPW_4509 absolutely need to be verified manually.

If you think the reported changes have nothing to do with the changes introduced in IGTPW_4509, 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_4509/index.html

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

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

### IGT changes ###

#### Possible regressions ####

* igt@i915_selftest@live@execlists:
- fi-icl-y: NOTRUN -> [INCOMPLETE][1]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4509/fi-icl-y/igt@i915_selftest@[email protected]

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

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

### IGT changes ###

#### Issues hit ####

* igt@i915_selftest@live@gt_pm:
- fi-skl-6700k2: [PASS][2] -> [DMESG-FAIL][3] ([i915#1791])
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5612/fi-skl-6700k2/igt@i915_selftest@live@gt_pm.html
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4509/fi-skl-6700k2/igt@i915_selftest@live@gt_pm.html
- fi-kbl-x1275: [PASS][4] -> [DMESG-FAIL][5] ([i915#1791])
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5612/fi-kbl-x1275/igt@i915_selftest@live@gt_pm.html
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4509/fi-kbl-x1275/igt@i915_selftest@live@gt_pm.html
- fi-kbl-r: [PASS][6] -> [DMESG-FAIL][7] ([i915#1791])
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5612/fi-kbl-r/igt@i915_selftest@live@gt_pm.html
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4509/fi-kbl-r/igt@i915_selftest@live@gt_pm.html

#### Possible fixes ####

* igt@i915_selftest@live@gt_pm:
- fi-bdw-5557u: [DMESG-FAIL][8] ([i915#1791]) -> [PASS][9]
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5612/fi-bdw-5557u/igt@i915_selftest@live@gt_pm.html
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4509/fi-bdw-5557u/igt@i915_selftest@live@gt_pm.html

[i915#1791]: https://gitlab.freedesktop.org/drm/intel/issues/1791

Participating hosts (45 -> 43)
------------------------------

Additional (3): fi-icl-y fi-kbl-7560u fi-bxt-dsi
Missing (5): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-kbl-7500u fi-bdw-samus

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

* CI: CI-20190529 -> None
* IGT: IGT_5612 -> IGTPW_4509

CI-20190529: 20190529
CI_DRM_8370: 1f3ffd7683d5457e14a1f879a8714a74b7b7faeb @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_4509: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4509/index.html
IGT_5612: c8dc1fd926a550308b971ca7d83fe0a927a38152 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4509/index.html
_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev