* [PATCH 1/2] drm/i915: Mark semaphores as complete on unsubmit out if payload was started
@ 2019-05-14 7:44 Chris Wilson
2019-05-14 7:44 ` [PATCH 2/2] drm/i915: Truly bump ready tasks ahead of busywaits Chris Wilson
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Chris Wilson @ 2019-05-14 7:44 UTC (permalink / raw)
To: intel-gfx
Avoid charging us for the presumed busywait if the request was preempted
after successfully using semaphores to reduce inter-engine latency.
References: ca6e56f654e7 ("drm/i915: Disable semaphore busywaits on saturated systems")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
drivers/gpu/drm/i915/i915_request.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index bed213148cbb..4e6a7f37fffc 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -509,6 +509,10 @@ void __i915_request_unsubmit(struct i915_request *request)
/* Transfer back from the global per-engine timeline to per-context */
move_to_timeline(request, request->timeline);
+ /* We've already spun, don't charge on resubmitting. */
+ if (request->sched.semaphores && i915_request_started(request))
+ request->sched.semaphores = 0;
+
/*
* We don't need to wake_up any waiters on request->execute, they
* will get woken by any other event or us re-adding this request
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/i915: Truly bump ready tasks ahead of busywaits
2019-05-14 7:44 [PATCH 1/2] drm/i915: Mark semaphores as complete on unsubmit out if payload was started Chris Wilson
@ 2019-05-14 7:44 ` Chris Wilson
2019-05-14 8:04 ` [PATCH] " Chris Wilson
2019-05-14 8:10 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Mark semaphores as complete on unsubmit out if payload was started (rev2) Patchwork
2019-05-14 8:30 ` ✓ Fi.CI.BAT: success " Patchwork
2 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2019-05-14 7:44 UTC (permalink / raw)
To: intel-gfx; +Cc: Dmitry Ermilov
In commit b7404c7ecb38 ("drm/i915: Bump ready tasks ahead of
busywaits"), I tried cutting a corner in order to not install a signal
for each of our dependencies, and only listened to requests on which we
were intending to busywait. The compromise that was made was that
instead of then being able to promite the request with a full
NOSEMAPHORE like its non-busywaiting brethren, as we had not ensured we
had cleared the semaphore chain, we settled for only using the NEWCLIENT
boost. With an over saturated system with multiple NEWCLIENTS in flight
at any time, this was found to be an inadequate promotion and left us
with a much poorer scheduling order than prior to using semaphores.
The outcome of this patch, is that all requests have NOSEMAPHORE
priority when they have no dependencies and are ready to run and not
busywait, restoring the pre-semaphore ordering on saturated systems.
We can demonstrate the effect of poor scheduling order by oversaturating
the system using gem_wsim on a system with multiple vcs engines
(i.e running the same workloads across more clients than required for
peak throughput, e.g. media_load_balance_17i7.wsim -c4 -b context):
x v5.1 (normalized)
+ tip
* fix
+------------------------------------------------------------------------+
| * |
| ** |
| ** |
| ** |
| ** |
| ** |
| ** |
| ** |
| ** |
| ** |
| ** |
| ** |
| ** |
| ** |
| ** |
| ** |
| + **xx |
| + **xx |
| + ***xx |
| + ***xx |
| + ***xx |
| + ***xx |
| + ****x |
| +++ ****x |
| +++ ****x |
| +++ ****x |
| ++++ ****x |
| ++++ ****x |
| ++++ ****xx |
| ++++ ****xx |
| ++++ ****xx |
| ++++ ****xx |
| ++++ ****xx |
| ++++ + ****xx |
| ++++ + ****xx |
| ++++++ + ****xx |
| ++++++++++ ****xxx|
| ++++++++++ + ******xx|
| + ++++++++++++ ++ +++++ + ++ *******xx|
| |A| |
||__________M_______A___________________| |
| |A| |
+------------------------------------------------------------------------+
N Min Max Median Avg Stddev
x 120 0.99456 1.00628 1 1.0001545 0.0024387139
+ 120 0.873021 1.00037 0.884144 0.90148752 0.039190862
Difference at 99.5% confidence
-0.098667 +/- 0.0110762
-9.86517% +/- 1.10745%
(Student's t, pooled s = 0.0277657)
* 120 0.990207 1.00165 0.997047 0.99699748 0.0021024
Difference at 99.5% confidence
-0.003157 +/- 0.000908245
-0.315651% +/- 0.0908105%
(Student's t, pooled s = 0.00227678)
Fixes: b7404c7ecb38 ("drm/i915: Bump ready tasks ahead of busywaits")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Cc: Dmitry Ermilov <dmitry.ermilov@intel.com>
---
drivers/gpu/drm/i915/i915_request.c | 31 +++++++++++------------------
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 4e6a7f37fffc..bcb3b54cb806 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -573,18 +573,7 @@ semaphore_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state)
switch (state) {
case FENCE_COMPLETE:
- /*
- * We only check a small portion of our dependencies
- * and so cannot guarantee that there remains no
- * semaphore chain across all. Instead of opting
- * for the full NOSEMAPHORE boost, we go for the
- * smaller (but still preempting) boost of
- * NEWCLIENT. This will be enough to boost over
- * a busywaiting request (as that cannot be
- * NEWCLIENT) without accidentally boosting
- * a busywait over real work elsewhere.
- */
- i915_schedule_bump_priority(request, I915_PRIORITY_NEWCLIENT);
+ i915_schedule_bump_priority(request, I915_PRIORITY_NOSEMAPHORE);
break;
case FENCE_FREE:
@@ -850,12 +839,6 @@ emit_semaphore_wait(struct i915_request *to,
if (err < 0)
return err;
- err = i915_sw_fence_await_dma_fence(&to->semaphore,
- &from->fence, 0,
- I915_FENCE_GFP);
- if (err < 0)
- return err;
-
/* Only submit our spinner after the signaler is running! */
err = i915_request_await_execution(to, from, gfp);
if (err)
@@ -921,8 +904,18 @@ i915_request_await_request(struct i915_request *to, struct i915_request *from)
&from->fence, 0,
I915_FENCE_GFP);
}
+ if (ret < 0)
+ return ret;
+
+ if (to->sched.flags & I915_SCHED_HAS_SEMAPHORE_CHAIN) {
+ ret = i915_sw_fence_await_dma_fence(&to->semaphore,
+ &from->fence, 0,
+ I915_FENCE_GFP);
+ if (ret < 0)
+ return ret;
+ }
- return ret < 0 ? ret : 0;
+ return 0;
}
int
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] drm/i915: Truly bump ready tasks ahead of busywaits
2019-05-14 7:44 ` [PATCH 2/2] drm/i915: Truly bump ready tasks ahead of busywaits Chris Wilson
@ 2019-05-14 8:04 ` Chris Wilson
2019-05-14 15:08 ` Chris Wilson
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2019-05-14 8:04 UTC (permalink / raw)
To: intel-gfx; +Cc: Dmitry Ermilov
In commit b7404c7ecb38 ("drm/i915: Bump ready tasks ahead of
busywaits"), I tried cutting a corner in order to not install a signal
for each of our dependencies, and only listened to requests on which we
were intending to busywait. The compromise that was made was that
instead of then being able to promite the request with a full
NOSEMAPHORE like its non-busywaiting brethren, as we had not ensured we
had cleared the semaphore chain, we settled for only using the NEWCLIENT
boost. With an over saturated system with multiple NEWCLIENTS in flight
at any time, this was found to be an inadequate promotion and left us
with a much poorer scheduling order than prior to using semaphores.
The outcome of this patch, is that all requests have NOSEMAPHORE
priority when they have no dependencies and are ready to run and not
busywait, restoring the pre-semaphore ordering on saturated systems.
We can demonstrate the effect of poor scheduling order by oversaturating
the system using gem_wsim on a system with multiple vcs engines
(i.e running the same workloads across more clients than required for
peak throughput, e.g. media_load_balance_17i7.wsim -c4 -b context):
x v5.1 (normalized)
+ tip
* fix
+------------------------------------------------------------------------+
| x |
| x |
| x |
| x |
| %x |
| %%x |
| %%x |
| %%x |
| %%x |
| %%x |
| %%x |
| %%x |
| %%x |
| %%x |
| %%x |
| %#x |
| %#x |
| %#x |
| %#x |
| %#x |
| + %#xx |
| + %#xx |
| + %%#xx |
| + %%#xx |
| + %%#xx |
| + %%#xx |
| + %%##x |
| +++ %%##x |
| +++ %%##x |
| +++ %%##x |
| ++++ %%##x |
| ++++ %%##x |
| ++++ %%##xx |
| ++++ %###xx |
| ++++ %###xx |
| ++++ %###xx |
| ++++ %###xx |
| ++++ + %#O#xx |
| ++++ + %#O#xx |
| ++++++ + %#O#xx |
| ++++++++++ %OOOxxx|
| ++++++++++ + %#OOO#xx|
| + ++++++++++++ ++ +++++ + ++ @@OOOO#xx|
| |A_| |
||__________M_______A____________________| |
| |A_| |
+------------------------------------------------------------------------+
N Min Max Median Avg Stddev
x 120 0.99456 1.00628 0.999985 1.0001545 0.0024387139
+ 120 0.873021 1.00037 0.884134 0.90148752 0.039190862
Difference at 99.5% confidence
-0.098667 +/- 0.0110762
-9.86517% +/- 1.10745%
(Student's t, pooled s = 0.0277657)
% 120 0.990207 1.00165 0.9970265 0.99699748 0.0021024
Difference at 99.5% confidence
-0.003157 +/- 0.000908245
-0.315651% +/- 0.0908105%
(Student's t, pooled s = 0.00227678)
Fixes: b7404c7ecb38 ("drm/i915: Bump ready tasks ahead of busywaits")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Cc: Dmitry Ermilov <dmitry.ermilov@intel.com>
---
drivers/gpu/drm/i915/i915_request.c | 31 +++++++++++------------------
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 4e6a7f37fffc..bcb3b54cb806 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -573,18 +573,7 @@ semaphore_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state)
switch (state) {
case FENCE_COMPLETE:
- /*
- * We only check a small portion of our dependencies
- * and so cannot guarantee that there remains no
- * semaphore chain across all. Instead of opting
- * for the full NOSEMAPHORE boost, we go for the
- * smaller (but still preempting) boost of
- * NEWCLIENT. This will be enough to boost over
- * a busywaiting request (as that cannot be
- * NEWCLIENT) without accidentally boosting
- * a busywait over real work elsewhere.
- */
- i915_schedule_bump_priority(request, I915_PRIORITY_NEWCLIENT);
+ i915_schedule_bump_priority(request, I915_PRIORITY_NOSEMAPHORE);
break;
case FENCE_FREE:
@@ -850,12 +839,6 @@ emit_semaphore_wait(struct i915_request *to,
if (err < 0)
return err;
- err = i915_sw_fence_await_dma_fence(&to->semaphore,
- &from->fence, 0,
- I915_FENCE_GFP);
- if (err < 0)
- return err;
-
/* Only submit our spinner after the signaler is running! */
err = i915_request_await_execution(to, from, gfp);
if (err)
@@ -921,8 +904,18 @@ i915_request_await_request(struct i915_request *to, struct i915_request *from)
&from->fence, 0,
I915_FENCE_GFP);
}
+ if (ret < 0)
+ return ret;
+
+ if (to->sched.flags & I915_SCHED_HAS_SEMAPHORE_CHAIN) {
+ ret = i915_sw_fence_await_dma_fence(&to->semaphore,
+ &from->fence, 0,
+ I915_FENCE_GFP);
+ if (ret < 0)
+ return ret;
+ }
- return ret < 0 ? ret : 0;
+ return 0;
}
int
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Mark semaphores as complete on unsubmit out if payload was started (rev2)
2019-05-14 7:44 [PATCH 1/2] drm/i915: Mark semaphores as complete on unsubmit out if payload was started Chris Wilson
2019-05-14 7:44 ` [PATCH 2/2] drm/i915: Truly bump ready tasks ahead of busywaits Chris Wilson
@ 2019-05-14 8:10 ` Patchwork
2019-05-14 8:30 ` ✓ Fi.CI.BAT: success " Patchwork
2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2019-05-14 8:10 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915: Mark semaphores as complete on unsubmit out if payload was started (rev2)
URL : https://patchwork.freedesktop.org/series/60614/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
4fbdde9dc1c3 drm/i915: Mark semaphores as complete on unsubmit out if payload was started
-:10: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#10:
References: ca6e56f654e7 ("drm/i915: Disable semaphore busywaits on saturated systems")
-:10: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit ca6e56f654e7 ("drm/i915: Disable semaphore busywaits on saturated systems")'
#10:
References: ca6e56f654e7 ("drm/i915: Disable semaphore busywaits on saturated systems")
total: 1 errors, 1 warnings, 0 checks, 10 lines checked
1009566f9389 drm/i915: Truly bump ready tasks ahead of busywaits
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Mark semaphores as complete on unsubmit out if payload was started (rev2)
2019-05-14 7:44 [PATCH 1/2] drm/i915: Mark semaphores as complete on unsubmit out if payload was started Chris Wilson
2019-05-14 7:44 ` [PATCH 2/2] drm/i915: Truly bump ready tasks ahead of busywaits Chris Wilson
2019-05-14 8:10 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Mark semaphores as complete on unsubmit out if payload was started (rev2) Patchwork
@ 2019-05-14 8:30 ` Patchwork
2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2019-05-14 8:30 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: series starting with [1/2] drm/i915: Mark semaphores as complete on unsubmit out if payload was started (rev2)
URL : https://patchwork.freedesktop.org/series/60614/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_6078 -> Patchwork_13013
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13013/
Known issues
------------
Here are the changes found in Patchwork_13013 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_suspend@basic-s3:
- fi-blb-e6850: [PASS][1] -> [INCOMPLETE][2] ([fdo#107718])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6078/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13013/fi-blb-e6850/igt@gem_exec_suspend@basic-s3.html
#### Possible fixes ####
* igt@i915_selftest@live_hangcheck:
- fi-skl-iommu: [INCOMPLETE][3] ([fdo#108602] / [fdo#108744]) -> [PASS][4]
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6078/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13013/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
[fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
Participating hosts (54 -> 45)
------------------------------
Missing (9): fi-kbl-soraka fi-cml-u2 fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus
Build changes
-------------
* Linux: CI_DRM_6078 -> Patchwork_13013
CI_DRM_6078: c8c778558fd52abd3303d8ea324df788062adc97 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4984: 66c887d2f7a92a4a97acd9611d5342afc5d4f815 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_13013: 1009566f93896221a1dda12d8935708230bea157 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
1009566f9389 drm/i915: Truly bump ready tasks ahead of busywaits
4fbdde9dc1c3 drm/i915: Mark semaphores as complete on unsubmit out if payload was started
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13013/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Truly bump ready tasks ahead of busywaits
2019-05-14 8:04 ` [PATCH] " Chris Wilson
@ 2019-05-14 15:08 ` Chris Wilson
0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2019-05-14 15:08 UTC (permalink / raw)
To: intel-gfx; +Cc: Dmitry Ermilov
Quoting Chris Wilson (2019-05-14 09:04:39)
> In commit b7404c7ecb38 ("drm/i915: Bump ready tasks ahead of
> busywaits"), I tried cutting a corner in order to not install a signal
> for each of our dependencies, and only listened to requests on which we
> were intending to busywait. The compromise that was made was that
> instead of then being able to promite the request with a full
> NOSEMAPHORE like its non-busywaiting brethren, as we had not ensured we
> had cleared the semaphore chain, we settled for only using the NEWCLIENT
> boost. With an over saturated system with multiple NEWCLIENTS in flight
> at any time, this was found to be an inadequate promotion and left us
> with a much poorer scheduling order than prior to using semaphores.
>
> The outcome of this patch, is that all requests have NOSEMAPHORE
> priority when they have no dependencies and are ready to run and not
> busywait, restoring the pre-semaphore ordering on saturated systems.
[snip]
> Fixes: b7404c7ecb38 ("drm/i915: Bump ready tasks ahead of busywaits")
Confirmed on the skl-xeon box this fixes that particular regression.
Alas that is not the last regression there. There is also an impact from
NEWCLIENT.
Imagine a client that does a work packet of (rcsA, rcsB, vcs), and now
imagine that there are 30 clients subscribed to the system. With
NEWCLIENT promotion we end up with
rcsAx30, (rcsB, vcs)x30
that is we do not start executing any vcs packets until all 30 clients
complete their first request -- whereas previously we would interleave
the vcs packed from client 1 with the first request in client 2. This
greatly reduces the concurrency between clients.
A temporary bandaid would be to revert
commit 1413b2bc0717036a5a653eef20cc3ae4cc66501a
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Mon Feb 4 15:01:01 2019 +0000
drm/i915: Trim NEWCLIENT boosting
There is an interesting problem underneath to try and minimise queuing
times across the multiple systems, as again we are fortunate that the
previous FIFO happens to be close to ideal ordering.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-14 15:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-14 7:44 [PATCH 1/2] drm/i915: Mark semaphores as complete on unsubmit out if payload was started Chris Wilson
2019-05-14 7:44 ` [PATCH 2/2] drm/i915: Truly bump ready tasks ahead of busywaits Chris Wilson
2019-05-14 8:04 ` [PATCH] " Chris Wilson
2019-05-14 15:08 ` Chris Wilson
2019-05-14 8:10 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/i915: Mark semaphores as complete on unsubmit out if payload was started (rev2) Patchwork
2019-05-14 8:30 ` ✓ Fi.CI.BAT: success " 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.