[Intel-gfx] [PATCH i-g-t 0/7] gem_wsim fixes and new features

[Intel-gfx] [PATCH i-g-t 0/7] gem_wsim fixes and new features

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A bag of new features and a couple fixes which came to be as I was testing
the dynamic SSEU story.

Tvrtko Ursulin (7):
  gem_wsim: Check sleep times
  gem_wsim: Make workload commands case sensitive
  gem_wsim: Context priority support
  gem_wsim: Stop keeping batches mapped
  gem_wsim: Make batches preemptable by default
  gem_wsim: Per context preemption point control
  gem_wsim: Fix BCS usage under VCS2 remap warning

 benchmarks/gem_wsim.c  | 219 +++++++++++++++++++++++++++++++++++++----
 benchmarks/wsim/README |  34 +++++++
 2 files changed, 234 insertions(+), 19 deletions(-)

-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[igt-dev] [PATCH i-g-t 1/7] gem_wsim: Check sleep times

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Notice in more places if we are running behind.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 benchmarks/gem_wsim.c | 52 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index 80f180829241..ead91b0f009b 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -1718,6 +1718,21 @@ static bool sync_deps(struct workload *wrk, struct w_step *w)
 	return synced;
 }
 
+static unsigned int measured_usleep(unsigned int usec)
+{
+	struct timespec ts = { };
+	unsigned int slept;
+
+	slept = igt_nsec_elapsed(&ts);
+	igt_assert(slept == 0);
+	do {
+		usleep(usec - slept);
+		slept = igt_nsec_elapsed(&ts) / 1000;
+	} while (slept < usec);
+
+	return igt_nsec_elapsed(&ts);
+}
+
 static void *run_workload(void *data)
 {
 	struct workload *wrk = (struct workload *)data;
@@ -1739,7 +1754,7 @@ static void *run_workload(void *data)
 	     count++) {
 		unsigned int cur_seqno = wrk->sync_seqno;
 
-		clock_gettime(CLOCK_MONOTONIC, &wrk->repeat_start);
+		igt_gettime(&wrk->repeat_start);
 
 		for (i = 0, w = wrk->steps; wrk->run && (i < wrk->nr_steps);
 		     i++, w++) {
@@ -1751,13 +1766,14 @@ static void *run_workload(void *data)
 			} else if (w->type == PERIOD) {
 				struct timespec now;
 
-				clock_gettime(CLOCK_MONOTONIC, &now);
+				igt_gettime(&now);
 				do_sleep = w->period -
 					   elapsed_us(&wrk->repeat_start, &now);
 				if (do_sleep < 0) {
-					if (verbose > 1)
-						printf("%u: Dropped period @ %u/%u (%dus late)!\n",
-						       wrk->id, count, i, do_sleep);
+					if (verbose > 1 &&
+					    -do_sleep > 10 * w->period / 100)
+						printf("%u: Missed period @ %u/%u (%dus late)!\n",
+						       wrk->id, count, i, -do_sleep);
 					continue;
 				}
 			} else if (w->type == SYNC) {
@@ -1793,7 +1809,31 @@ static void *run_workload(void *data)
 			}
 
 			if (do_sleep || w->type == PERIOD) {
-				usleep(do_sleep);
+				long slept = measured_usleep(do_sleep) / 1000;
+
+				if ( w->type == PERIOD) {
+					struct timespec now;
+
+					igt_gettime(&now);
+
+					slept = elapsed_us(&wrk->repeat_start,
+							   &now);
+
+					if (verbose > 1 &&
+					    slept > (110 * w->period / 100))
+						printf("%u: Overslept period by %ldus @ %u/%u! (%uus)\n",
+						       wrk->id,
+						       -slept, count,
+						       i, w->period);
+				} else {
+					if (verbose > 1 &&
+					    slept > (110 * do_sleep / 100))
+						printf("%u: Overslept by %ldus @ %u/%u! (%uus)\n",
+						       wrk->id,
+						       slept - do_sleep, count,
+						       i, do_sleep);
+				}
+
 				continue;
 			}
 
-- 
2.17.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 2/7] gem_wsim: Make workload commands case sensitive

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Need namespace for new commands and I never documented they are case
insensitive so it is fine.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 benchmarks/gem_wsim.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index ead91b0f009b..e5eb1ef70a35 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -310,7 +310,7 @@ parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
 		if ((field = strtok_r(fstart, ".", &fctx)) != NULL) {
 			fstart = NULL;
 
-			if (!strcasecmp(field, "d")) {
+			if (!strcmp(field, "d")) {
 				if ((field = strtok_r(fstart, ".", &fctx)) !=
 				    NULL) {
 					tmp = atoi(field);
@@ -326,7 +326,7 @@ parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
 					step.delay = tmp;
 					goto add_step;
 				}
-			} else if (!strcasecmp(field, "p")) {
+			} else if (!strcmp(field, "p")) {
 				if ((field = strtok_r(fstart, ".", &fctx)) !=
 				    NULL) {
 					tmp = atoi(field);
@@ -342,7 +342,7 @@ parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
 					step.period = tmp;
 					goto add_step;
 				}
-			} else if (!strcasecmp(field, "s")) {
+			} else if (!strcmp(field, "s")) {
 				if ((field = strtok_r(fstart, ".", &fctx)) !=
 				    NULL) {
 					tmp = atoi(field);
@@ -359,7 +359,7 @@ parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
 					step.target = tmp;
 					goto add_step;
 				}
-			} else if (!strcasecmp(field, "t")) {
+			} else if (!strcmp(field, "t")) {
 				if ((field = strtok_r(fstart, ".", &fctx)) !=
 				    NULL) {
 					tmp = atoi(field);
@@ -375,7 +375,7 @@ parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
 					step.throttle = tmp;
 					goto add_step;
 				}
-			} else if (!strcasecmp(field, "q")) {
+			} else if (!strcmp(field, "q")) {
 				if ((field = strtok_r(fstart, ".", &fctx)) !=
 				    NULL) {
 					tmp = atoi(field);
@@ -391,7 +391,7 @@ parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
 					step.throttle = tmp;
 					goto add_step;
 				}
-			} else if (!strcasecmp(field, "a")) {
+			} else if (!strcmp(field, "a")) {
 				if ((field = strtok_r(fstart, ".", &fctx)) !=
 				    NULL) {
 					tmp = atoi(field);
@@ -407,7 +407,7 @@ parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
 					step.target = tmp;
 					goto add_step;
 				}
-			} else if (!strcasecmp(field, "f")) {
+			} else if (!strcmp(field, "f")) {
 				step.type = SW_FENCE;
 				goto add_step;
 			}
-- 
2.17.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 3/7] gem_wsim: Context priority support

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A new workload command ('P') is added which enables per context dynamic
priority control.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 benchmarks/gem_wsim.c  | 48 +++++++++++++++++++++++++++++++++++++++++-
 benchmarks/wsim/README | 18 ++++++++++++++++
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index e5eb1ef70a35..34758419254a 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -82,7 +82,8 @@ enum w_type
 	THROTTLE,
 	QD_THROTTLE,
 	SW_FENCE,
-	SW_FENCE_SIGNAL
+	SW_FENCE_SIGNAL,
+	CTX_PRIORITY
 };
 
 struct deps
@@ -114,6 +115,7 @@ struct w_step
 		int target;
 		int throttle;
 		int fence_signal;
+		int priority;
 	};
 
 	/* Implementation details */
@@ -162,6 +164,7 @@ struct workload
 	unsigned int nr_ctxs;
 	struct {
 		uint32_t id;
+		int priority;
 		unsigned int static_vcs;
 	} *ctx_list;
 
@@ -342,6 +345,36 @@ parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
 					step.period = tmp;
 					goto add_step;
 				}
+			} else if (!strcmp(field, "P")) {
+				unsigned int nr = 0;
+				while ((field = strtok_r(fstart, ".", &fctx)) !=
+				    NULL) {
+					tmp = atoi(field);
+					if (tmp <= 0 && nr == 0) {
+						if (verbose)
+							fprintf(stderr,
+								"Invalid context at step %u!\n",
+								nr_steps);
+						return NULL;
+					}
+
+					if (nr == 0) {
+						step.context = tmp;
+					} else if (nr == 1) {
+						step.priority = tmp;
+					} else {
+						if (verbose)
+							fprintf(stderr,
+								"Invalid priority format at step %u!\n",
+								nr_steps);
+						return NULL;
+					}
+
+					nr++;
+				}
+
+				step.type = CTX_PRIORITY;
+				goto add_step;
 			} else if (!strcmp(field, "s")) {
 				if ((field = strtok_r(fstart, ".", &fctx)) !=
 				    NULL) {
@@ -1806,6 +1839,19 @@ static void *run_workload(void *data)
 				inc = cur_seqno - wrk->sync_seqno;
 				sw_sync_timeline_inc(wrk->sync_timeline, inc);
 				continue;
+			} else if (w->type == CTX_PRIORITY) {
+				if (w->priority != wrk->ctx_list[w->context].priority) {
+					struct drm_i915_gem_context_param param = {
+						.ctx_id = wrk->ctx_list[w->context].id,
+						.param = I915_CONTEXT_PARAM_PRIORITY,
+						.value = w->priority,
+					};
+
+					gem_context_set_param(fd, &param);
+					wrk->ctx_list[w->context].priority =
+								    w->priority;
+				}
+				continue;
 			}
 
 			if (do_sleep || w->type == PERIOD) {
diff --git a/benchmarks/wsim/README b/benchmarks/wsim/README
index 01766d37564f..fd9bff3401af 100644
--- a/benchmarks/wsim/README
+++ b/benchmarks/wsim/README
@@ -3,6 +3,7 @@ Workload descriptor format
 
 ctx.engine.duration_us.dependency.wait,...
 <uint>.<str>.<uint>[-<uint>].<int <= 0>[/<int <= 0>][...].<0|1>,...
+P.<uint>.<int>
 d|p|s|t|q|a.<int>,...
 f
 
@@ -22,6 +23,7 @@ Additional workload steps are also supported:
  'q' - Throttle to n max queue depth.
  'f' - Create a sync fence.
  'a' - Advance the previously created sync fence.
+ 'P' - Context priority.
 
 Engine ids: RCS, BCS, VCS, VCS1, VCS2, VECS
 
@@ -110,3 +112,19 @@ created at the second step. They are submitted ahead of time while still not
 runnable. When the second RCS batch completes the standalone fence is signaled
 which allows the two VCS batches to be executed. Finally we wait until the both
 VCS batches have completed before starting the (optional) next iteration.
+
+Context priority
+----------------
+
+  P.1.-1
+  1.RCS.1000.0.0
+  P.2.1
+  2.BCS.1000.-2.0
+
+Context 1 is marked as low priority (-1) and then a batch buffer is submitted
+against it. Context 2 is marked as high priority (1) and then a batch buffer
+is submitted against it which depends on the batch from context 1.
+
+Context priority command is executed at workload runtime and is valid until
+overriden by another (optional) same context priority change. Actual driver
+ioctls are executed only if the priority level has changed for the context.
-- 
2.17.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 4/7] gem_wsim: Stop keeping batches mapped

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

The reason, which I assume was there at some point, to keep the batches
persistently memory mapped does not appear to be there. So unmap them
after creation and remove the unused structure members.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 benchmarks/gem_wsim.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index 34758419254a..24f518a7770f 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -128,7 +128,6 @@ struct w_step
 	struct drm_i915_gem_relocation_entry reloc[4];
 	unsigned long bb_sz;
 	uint32_t bb_handle;
-	uint32_t *mapped_batch;
 	uint32_t *seqno_value;
 	uint32_t *seqno_address;
 	uint32_t *rt0_value;
@@ -136,7 +135,6 @@ struct w_step
 	uint32_t *rt1_address;
 	uint32_t *latch_value;
 	uint32_t *latch_address;
-	unsigned int mapped_len;
 };
 
 DECLARE_EWMA(uint64_t, rt, 4, 2)
@@ -755,8 +753,7 @@ terminate_bb(struct w_step *w, unsigned int flags)
 
 	*cs = bbe;
 
-	w->mapped_batch = ptr;
-	w->mapped_len = mmap_len;
+	munmap(ptr, mmap_len);
 }
 
 static const unsigned int eb_engine_map[NUM_ENGINES] = {
-- 
2.17.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 5/7] gem_wsim: Make batches preemptable by default

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

MI_NOOP cannot be preempted which means up to now gem_wsim workloads were
preemptable on batch buffer granularity only.

Add MI_ARB_CHK every 100us so the new default is mid-batch preemption.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 benchmarks/gem_wsim.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index 24f518a7770f..f60c086a3f1e 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -683,6 +683,25 @@ static unsigned long get_bb_sz(unsigned int duration)
 		     nop_calibration_us, sizeof(uint32_t));
 }
 
+static void
+init_bb(struct w_step *w, unsigned int flags)
+{
+	/* Preemption point every 100us. */
+	const unsigned int arb_period = get_bb_sz(100) / sizeof(uint32_t);
+	unsigned int i;
+	uint32_t *ptr;
+
+	gem_set_domain(fd, w->bb_handle,
+		       I915_GEM_DOMAIN_WC, I915_GEM_DOMAIN_WC);
+
+	ptr = gem_mmap__wc(fd, w->bb_handle, 0, w->bb_sz, PROT_WRITE);
+
+	for (i = arb_period; i < w->bb_sz / sizeof(uint32_t); i += arb_period)
+		ptr[i] = 0x5 << 23; /* MI_ARB_CHK */
+
+	munmap(ptr, w->bb_sz);
+}
+
 static void
 terminate_bb(struct w_step *w, unsigned int flags)
 {
@@ -836,6 +855,7 @@ alloc_step_batch(struct workload *wrk, struct w_step *w, unsigned int flags)
 
 	w->bb_sz = get_bb_sz(w->duration.max);
 	w->bb_handle = w->obj[j].handle = gem_create(fd, w->bb_sz);
+	init_bb(w, flags);
 	terminate_bb(w, flags);
 
 	if (flags & SEQNO) {
-- 
2.17.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 6/7] gem_wsim: Per context preemption point control

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Allow workloads to specify frequency of preemption points per context.

New workload command ('X') is added to allow this.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 benchmarks/gem_wsim.c  | 84 ++++++++++++++++++++++++++++++++++++++++--
 benchmarks/wsim/README | 18 ++++++++-
 2 files changed, 98 insertions(+), 4 deletions(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index f60c086a3f1e..fd268b3ab456 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -83,7 +83,8 @@ enum w_type
 	QD_THROTTLE,
 	SW_FENCE,
 	SW_FENCE_SIGNAL,
-	CTX_PRIORITY
+	CTX_PRIORITY,
+	PREEMPTION
 };
 
 struct deps
@@ -122,6 +123,7 @@ struct w_step
 	unsigned int idx;
 	struct igt_list rq_link;
 	unsigned int request;
+	unsigned int preempt_us;
 
 	struct drm_i915_gem_execbuffer2 eb;
 	struct drm_i915_gem_exec_object2 *obj;
@@ -441,6 +443,42 @@ parse_workload(struct w_arg *arg, unsigned int flags, struct workload *app_w)
 			} else if (!strcmp(field, "f")) {
 				step.type = SW_FENCE;
 				goto add_step;
+			} else if (!strcmp(field, "X")) {
+				unsigned int nr = 0;
+				while ((field = strtok_r(fstart, ".", &fctx)) !=
+				    NULL) {
+					tmp = atoi(field);
+					if (tmp <= 0 && nr == 0) {
+						if (verbose)
+							fprintf(stderr,
+								"Invalid context at step %u!\n",
+								nr_steps);
+						return NULL;
+					} else if (tmp < 0 && nr == 1) {
+						if (verbose)
+							fprintf(stderr,
+								"Invalid preemption period at step %u!\n",
+								nr_steps);
+						return NULL;
+					}
+
+					if (nr == 0) {
+						step.context = tmp;
+					} else if (nr == 1) {
+						step.period = tmp;
+					} else {
+						if (verbose)
+							fprintf(stderr,
+								"Invalid preemption format at step %u!\n",
+								nr_steps);
+						return NULL;
+					}
+
+					nr++;
+				}
+
+				step.type = PREEMPTION;
+				goto add_step;
 			}
 
 			tmp = atoi(field);
@@ -686,11 +724,14 @@ static unsigned long get_bb_sz(unsigned int duration)
 static void
 init_bb(struct w_step *w, unsigned int flags)
 {
-	/* Preemption point every 100us. */
-	const unsigned int arb_period = get_bb_sz(100) / sizeof(uint32_t);
+	const unsigned int arb_period =
+			get_bb_sz(w->preempt_us) / sizeof(uint32_t);
 	unsigned int i;
 	uint32_t *ptr;
 
+	if (!arb_period)
+		return;
+
 	gem_set_domain(fd, w->bb_handle,
 		       I915_GEM_DOMAIN_WC, I915_GEM_DOMAIN_WC);
 
@@ -965,6 +1006,41 @@ prepare_workload(unsigned int id, struct workload *wrk, unsigned int flags)
 		}
 	}
 
+	/* Record default preemption. */
+	for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) {
+		if (w->type == BATCH)
+			w->preempt_us = 100;
+	}
+
+	/*
+	 * Scan for contexts with modified preemption config and record their
+	 * preemption period for the following steps belonging to the same
+	 * context.
+	 */
+	for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) {
+		struct w_step *w2;
+		int j;
+
+		if (w->type != PREEMPTION)
+			continue;
+
+		for (j = i + 1; j < wrk->nr_steps; j++) {
+			w2 = &wrk->steps[j];
+
+			if (w2->context != w->context)
+				continue;
+			else if (w2->type == PREEMPTION)
+				break;
+			else if (w2->type != BATCH)
+				continue;
+
+			w2->preempt_us = w->period;
+		}
+	}
+
+	/*
+	 * Allocate batch buffers.
+	 */
 	for (i = 0, w = wrk->steps; i < wrk->nr_steps; i++, w++) {
 		unsigned int _flags = flags;
 		enum intel_engine_id engine = w->engine;
@@ -1869,6 +1945,8 @@ static void *run_workload(void *data)
 								    w->priority;
 				}
 				continue;
+			} else if (w->type == PREEMPTION) {
+				continue;
 			}
 
 			if (do_sleep || w->type == PERIOD) {
diff --git a/benchmarks/wsim/README b/benchmarks/wsim/README
index fd9bff3401af..205cd6c93afb 100644
--- a/benchmarks/wsim/README
+++ b/benchmarks/wsim/README
@@ -3,7 +3,7 @@ Workload descriptor format
 
 ctx.engine.duration_us.dependency.wait,...
 <uint>.<str>.<uint>[-<uint>].<int <= 0>[/<int <= 0>][...].<0|1>,...
-P.<uint>.<int>
+P|X.<uint>.<int>
 d|p|s|t|q|a.<int>,...
 f
 
@@ -24,6 +24,7 @@ Additional workload steps are also supported:
  'f' - Create a sync fence.
  'a' - Advance the previously created sync fence.
  'P' - Context priority.
+ 'X' - Context preemption control.
 
 Engine ids: RCS, BCS, VCS, VCS1, VCS2, VECS
 
@@ -128,3 +129,18 @@ is submitted against it which depends on the batch from context 1.
 Context priority command is executed at workload runtime and is valid until
 overriden by another (optional) same context priority change. Actual driver
 ioctls are executed only if the priority level has changed for the context.
+
+Context preemption control
+--------------------------
+
+  X.1.0
+  1.RCS.1000.0.0
+  X.1.500
+  1.RCS.1000.0.0
+
+Context 1 is marked as non-preemptable batches and a batch is sent against 1.
+The same context is then marked to have batches which can be preempted every
+500us and another batch is submitted.
+
+Same as with context priority, context preemption commands are valid until
+optionally overriden by another preemption control change on the same context.
-- 
2.17.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t 7/7] gem_wsim: Fix BCS usage under VCS2 remap warning

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Need to check we actually are in VCS2 remapping mode!

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 benchmarks/gem_wsim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index fd268b3ab456..850c8972f6eb 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -661,7 +661,7 @@ add_step:
 		}
 	}
 
-	if (bcs_used && verbose)
+	if (bcs_used && (flags & VCS2REMAP) && verbose)
 		printf("BCS usage in workload with VCS2 remapping enabled!\n");
 
 	return wrk;
-- 
2.17.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/7] gem_wsim: Check sleep times

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-08-14 16:05:13)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Notice in more places if we are running behind.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  benchmarks/gem_wsim.c | 52 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> index 80f180829241..ead91b0f009b 100644
> --- a/benchmarks/gem_wsim.c
> +++ b/benchmarks/gem_wsim.c
> @@ -1718,6 +1718,21 @@ static bool sync_deps(struct workload *wrk, struct w_step *w)
>         return synced;
>  }
>  
> +static unsigned int measured_usleep(unsigned int usec)
> +{
> +       struct timespec ts = { };
> +       unsigned int slept;
> +
> +       slept = igt_nsec_elapsed(&ts);
> +       igt_assert(slept == 0);
> +       do {
> +               usleep(usec - slept);
> +               slept = igt_nsec_elapsed(&ts) / 1000;
> +       } while (slept < usec);
> +
> +       return igt_nsec_elapsed(&ts);
> +}
> +
>  static void *run_workload(void *data)
>  {
>         struct workload *wrk = (struct workload *)data;
> @@ -1739,7 +1754,7 @@ static void *run_workload(void *data)
>              count++) {
>                 unsigned int cur_seqno = wrk->sync_seqno;
>  
> -               clock_gettime(CLOCK_MONOTONIC, &wrk->repeat_start);
> +               igt_gettime(&wrk->repeat_start);

Were you already linking against libigt?
This code is not set up to hit an igt_assert()...
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 4/7] gem_wsim: Stop keeping batches mapped

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-08-14 16:05:16)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> The reason, which I assume was there at some point, to keep the batches
> persistently memory mapped does not appear to be there. So unmap them
> after creation and remove the unused structure members.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  benchmarks/gem_wsim.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> index 34758419254a..24f518a7770f 100644
> --- a/benchmarks/gem_wsim.c
> +++ b/benchmarks/gem_wsim.c
> @@ -128,7 +128,6 @@ struct w_step
>         struct drm_i915_gem_relocation_entry reloc[4];
>         unsigned long bb_sz;
>         uint32_t bb_handle;
> -       uint32_t *mapped_batch;
>         uint32_t *seqno_value;
>         uint32_t *seqno_address;
>         uint32_t *rt0_value;
> @@ -136,7 +135,6 @@ struct w_step
>         uint32_t *rt1_address;
>         uint32_t *latch_value;
>         uint32_t *latch_address;
> -       unsigned int mapped_len;
>  };
>  
>  DECLARE_EWMA(uint64_t, rt, 4, 2)
> @@ -755,8 +753,7 @@ terminate_bb(struct w_step *w, unsigned int flags)
>  
>         *cs = bbe;
>  
> -       w->mapped_batch = ptr;
> -       w->mapped_len = mmap_len;
> +       munmap(ptr, mmap_len);

The mapped_batch/ptr contains seqno_value/seqno_address etc which is
used after terminate_bb().
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 4/7] gem_wsim: Stop keeping batches mapped

[Tvrtko Ursulin]

On 14/08/2018 16:13, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-08-14 16:05:16)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> The reason, which I assume was there at some point, to keep the batches
>> persistently memory mapped does not appear to be there. So unmap them
>> after creation and remove the unused structure members.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   benchmarks/gem_wsim.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
>> index 34758419254a..24f518a7770f 100644
>> --- a/benchmarks/gem_wsim.c
>> +++ b/benchmarks/gem_wsim.c
>> @@ -128,7 +128,6 @@ struct w_step
>>          struct drm_i915_gem_relocation_entry reloc[4];
>>          unsigned long bb_sz;
>>          uint32_t bb_handle;
>> -       uint32_t *mapped_batch;
>>          uint32_t *seqno_value;
>>          uint32_t *seqno_address;
>>          uint32_t *rt0_value;
>> @@ -136,7 +135,6 @@ struct w_step
>>          uint32_t *rt1_address;
>>          uint32_t *latch_value;
>>          uint32_t *latch_address;
>> -       unsigned int mapped_len;
>>   };
>>   
>>   DECLARE_EWMA(uint64_t, rt, 4, 2)
>> @@ -755,8 +753,7 @@ terminate_bb(struct w_step *w, unsigned int flags)
>>   
>>          *cs = bbe;
>>   
>> -       w->mapped_batch = ptr;
>> -       w->mapped_len = mmap_len;
>> +       munmap(ptr, mmap_len);
> 
> The mapped_batch/ptr contains seqno_value/seqno_address etc which is
> used after terminate_bb().

Oh dear, I was blind.. Thanks for being sharp as always.

Regards,

Tvrtko

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/7] gem_wsim: Check sleep times

[Tvrtko Ursulin]

On 14/08/2018 16:09, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-08-14 16:05:13)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Notice in more places if we are running behind.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   benchmarks/gem_wsim.c | 52 ++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 46 insertions(+), 6 deletions(-)
>>
>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
>> index 80f180829241..ead91b0f009b 100644
>> --- a/benchmarks/gem_wsim.c
>> +++ b/benchmarks/gem_wsim.c
>> @@ -1718,6 +1718,21 @@ static bool sync_deps(struct workload *wrk, struct w_step *w)
>>          return synced;
>>   }
>>   
>> +static unsigned int measured_usleep(unsigned int usec)
>> +{
>> +       struct timespec ts = { };
>> +       unsigned int slept;
>> +
>> +       slept = igt_nsec_elapsed(&ts);
>> +       igt_assert(slept == 0);
>> +       do {
>> +               usleep(usec - slept);
>> +               slept = igt_nsec_elapsed(&ts) / 1000;
>> +       } while (slept < usec);
>> +
>> +       return igt_nsec_elapsed(&ts);
>> +}
>> +
>>   static void *run_workload(void *data)
>>   {
>>          struct workload *wrk = (struct workload *)data;
>> @@ -1739,7 +1754,7 @@ static void *run_workload(void *data)
>>               count++) {
>>                  unsigned int cur_seqno = wrk->sync_seqno;
>>   
>> -               clock_gettime(CLOCK_MONOTONIC, &wrk->repeat_start);
>> +               igt_gettime(&wrk->repeat_start);
> 
> Were you already linking against libigt?
> This code is not set up to hit an igt_assert()...

This code meaning gem_wsim? There's a lot of asserts in it already, and 
when they trigger traceback looks fine. What do you think is missing?

Regards,

Tvrtko




_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/7] gem_wsim: Check sleep times

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-08-14 16:21:08)
> 
> On 14/08/2018 16:09, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-08-14 16:05:13)
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> Notice in more places if we are running behind.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> ---
> >>   benchmarks/gem_wsim.c | 52 ++++++++++++++++++++++++++++++++++++++-----
> >>   1 file changed, 46 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
> >> index 80f180829241..ead91b0f009b 100644
> >> --- a/benchmarks/gem_wsim.c
> >> +++ b/benchmarks/gem_wsim.c
> >> @@ -1718,6 +1718,21 @@ static bool sync_deps(struct workload *wrk, struct w_step *w)
> >>          return synced;
> >>   }
> >>   
> >> +static unsigned int measured_usleep(unsigned int usec)
> >> +{
> >> +       struct timespec ts = { };
> >> +       unsigned int slept;
> >> +
> >> +       slept = igt_nsec_elapsed(&ts);
> >> +       igt_assert(slept == 0);
> >> +       do {
> >> +               usleep(usec - slept);
> >> +               slept = igt_nsec_elapsed(&ts) / 1000;
> >> +       } while (slept < usec);
> >> +
> >> +       return igt_nsec_elapsed(&ts);
> >> +}
> >> +
> >>   static void *run_workload(void *data)
> >>   {
> >>          struct workload *wrk = (struct workload *)data;
> >> @@ -1739,7 +1754,7 @@ static void *run_workload(void *data)
> >>               count++) {
> >>                  unsigned int cur_seqno = wrk->sync_seqno;
> >>   
> >> -               clock_gettime(CLOCK_MONOTONIC, &wrk->repeat_start);
> >> +               igt_gettime(&wrk->repeat_start);
> > 
> > Were you already linking against libigt?
> > This code is not set up to hit an igt_assert()...
> 
> This code meaning gem_wsim? There's a lot of asserts in it already, and 
> when they trigger traceback looks fine. What do you think is missing?

We have different values of fine ;) Looks nothing but a mess for me.
I don't think igt itself has much value outside of being the test
runner, we've baked too much knowledge of it being a test harness into
the code (and in many cases work to reduce those assumptions). I'd
rather libigtcore be distinct and not bring in the interlinked
igt_subtest+igt_assert.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for gem_wsim fixes and new features

[Patchwork]

== Series Details ==

Series: gem_wsim fixes and new features
URL   : https://patchwork.freedesktop.org/series/48196/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4666 -> IGTPW_1711 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48196/revisions/1/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_hangcheck:
      fi-skl-guc:         PASS -> DMESG-FAIL (fdo#107174)
      fi-kbl-7567u:       PASS -> DMESG-FAIL (fdo#106560, fdo#106947)

    igt@kms_chamelium@dp-edid-read:
      fi-kbl-7500u:       PASS -> FAIL (fdo#103841)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_hangcheck:
      fi-kbl-7560u:       DMESG-FAIL (fdo#106560, fdo#106947) -> PASS

    igt@drv_selftest@live_workarounds:
      fi-skl-6700hq:      DMESG-FAIL (fdo#107292) -> PASS
      fi-cnl-psr:         DMESG-FAIL (fdo#107292) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#106560 https://bugs.freedesktop.org/show_bug.cgi?id=106560
  fdo#106947 https://bugs.freedesktop.org/show_bug.cgi?id=106947
  fdo#107174 https://bugs.freedesktop.org/show_bug.cgi?id=107174
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292


== Participating hosts (53 -> 49) ==

  Additional (1): fi-glk-j4005 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * IGT: IGT_4594 -> IGTPW_1711

  CI_DRM_4666: 26f5eeef80e4332958ea855e90a4d015a9481e3f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1711: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1711/
  IGT_4594: b0263e5d0563a81a42cf66e7d3b84662d3222862 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1711/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/7] gem_wsim: Check sleep times

[Tvrtko Ursulin]

On 14/08/2018 16:27, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-08-14 16:21:08)
>>
>> On 14/08/2018 16:09, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2018-08-14 16:05:13)
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Notice in more places if we are running behind.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> ---
>>>>    benchmarks/gem_wsim.c | 52 ++++++++++++++++++++++++++++++++++++++-----
>>>>    1 file changed, 46 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
>>>> index 80f180829241..ead91b0f009b 100644
>>>> --- a/benchmarks/gem_wsim.c
>>>> +++ b/benchmarks/gem_wsim.c
>>>> @@ -1718,6 +1718,21 @@ static bool sync_deps(struct workload *wrk, struct w_step *w)
>>>>           return synced;
>>>>    }
>>>>    
>>>> +static unsigned int measured_usleep(unsigned int usec)
>>>> +{
>>>> +       struct timespec ts = { };
>>>> +       unsigned int slept;
>>>> +
>>>> +       slept = igt_nsec_elapsed(&ts);
>>>> +       igt_assert(slept == 0);
>>>> +       do {
>>>> +               usleep(usec - slept);
>>>> +               slept = igt_nsec_elapsed(&ts) / 1000;
>>>> +       } while (slept < usec);
>>>> +
>>>> +       return igt_nsec_elapsed(&ts);
>>>> +}
>>>> +
>>>>    static void *run_workload(void *data)
>>>>    {
>>>>           struct workload *wrk = (struct workload *)data;
>>>> @@ -1739,7 +1754,7 @@ static void *run_workload(void *data)
>>>>                count++) {
>>>>                   unsigned int cur_seqno = wrk->sync_seqno;
>>>>    
>>>> -               clock_gettime(CLOCK_MONOTONIC, &wrk->repeat_start);
>>>> +               igt_gettime(&wrk->repeat_start);
>>>
>>> Were you already linking against libigt?
>>> This code is not set up to hit an igt_assert()...
>>
>> This code meaning gem_wsim? There's a lot of asserts in it already, and
>> when they trigger traceback looks fine. What do you think is missing?
> 
> We have different values of fine ;) Looks nothing but a mess for me.
> I don't think igt itself has much value outside of being the test
> runner, we've baked too much knowledge of it being a test harness into
> the code (and in many cases work to reduce those assumptions). I'd
> rather libigtcore be distinct and not bring in the interlinked
> igt_subtest+igt_assert.

To not use it is to reimplement existing handy helpers - not ideal.

To split the test bits from useful helpers sounds like quite a task.

To wean gem_wsim off most igt_ stuff needs decision to be made on the 
former - what is core api, and what is test related.

To me problems outweigh the benefit. :( If gem_wsim was under tools, 
like intel_gpu_top, then yeah, it would be justifiable. Since it is in 
benchmarks I don't think we should bother.

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 1/7] gem_wsim: Check sleep times

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-08-14 19:27:08)
> To not use it is to reimplement existing handy helpers - not ideal.

The problem was that they were overly handy so I tried to avoid them in
benchmarks/ after being burnt too often.
 
> To split the test bits from useful helpers sounds like quite a task.

Nah, it's a drop in the ocean compared to writing the benchmarking
framework / integrating into an existing one like ezbench.
 
> To wean gem_wsim off most igt_ stuff needs decision to be made on the 
> former - what is core api, and what is test related.
> 
> To me problems outweigh the benefit. :( If gem_wsim was under tools, 
> like intel_gpu_top, then yeah, it would be justifiable. Since it is in 
> benchmarks I don't think we should bother.

I dream one day we will get microbenchmarks as part of CI. Not only
would that require a radical overhaul of the benchmarks/ but a
substantial investment of hw & maintenance. In my dream, I also have a
vision of like-for-like comparisons against other drivers.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for gem_wsim fixes and new features

[Patchwork]

== Series Details ==

Series: gem_wsim fixes and new features
URL   : https://patchwork.freedesktop.org/series/48196/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4594_full -> IGTPW_1711_full =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/48196/revisions/1/mbox/

== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-snb:          PASS -> INCOMPLETE (fdo#105411, fdo#106886)

    igt@drv_suspend@sysfs-reader:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665, fdo#107342)

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          PASS -> INCOMPLETE (fdo#106023, fdo#103665)

    igt@kms_setmode@basic:
      shard-apl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@gem_workarounds@suspend-resume:
      shard-glk:          FAIL (fdo#103375) -> PASS

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_draw_crc@draw-method-xrgb8888-blt-ytiled:
      shard-glk:          FAIL -> PASS

    igt@prime_vgem@wait-vebox:
      shard-snb:          INCOMPLETE (fdo#105411) -> SKIP

    
  fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#107342 https://bugs.freedesktop.org/show_bug.cgi?id=107342
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * IGT: IGT_4594 -> IGTPW_1711
    * Linux: CI_DRM_4664 -> CI_DRM_4666

  CI_DRM_4664: 19e458884fe1d8d10e453529933199250cc8821f @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4666: 26f5eeef80e4332958ea855e90a4d015a9481e3f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1711: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1711/
  IGT_4594: b0263e5d0563a81a42cf66e7d3b84662d3222862 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1711/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev