[PATCH 0/1] Inherit GPU scheduling priority from process nice

[PATCH 0/1] Inherit GPU scheduling priority from process nice

[Tvrtko Ursulin]

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

Current processing landscape seems to be more and more composed of pipelines
where computations are done on multiple hardware devices. Furthermore some of
the non-CPU devices, like in this case many GPUs supported by the i915 driver,
actually support priority based scheduling which is currently rather
inaccessible to the user (in terms of being able to control it from the
outside).

From these two statements a question arises on how to allow for a simple,
effective and consolidated user experience. In other words why user would not be
able to do something like:

 $ nice ffmmpeg ...transcode my videos...
 $ my-favourite-game

And have the nice hint apply to GPU parts of the transcode pipeline as well?

This would in fact follow the approach taken by kernel's block scheduler where
ionice is by default inherited from process nice.

This series implements the same idea by inheriting context creator and batch
buffer submitted nice value as context nice. To avoid influencing GPU scheduling
aware clients this is done only one for contexts where userspace hasn't
explicitly specified a non-default scheduling priority

The approach is completely compatible with GuC and drm/scheduler since all
support at least low/normal/high priority levels with just the granularity of
available control differing. In other words with GuC scheduling there is no
difference between nice 5 and 10, both would map to low priority, but the
default case of positive or negative nice, versus nice 0, is still correctly
propagated to the firmware scheduler.

With the series applied I simulated the scenario of a background GPU task
running simultaneously with an interactive client, varying the former's nice
value.

Simulating a non-interactive GPU background task was:
  vblank_mode=0 nice -n <N> glxgears -geometry 1600x800

Interactive client was simulated with:
  gem_wsim -w ~/test.wsim -r 300 -v # (This one is self-capped at ~60fps.)

These were the results on DG1, first with execlists (default):

   Background nice  |   Interactive FPS
 -------------------+--------------------
      <not running> |         59
                  0 |         35
                 10 |         42

As we can see running the background load with nice 10 can somewhat help the
performance of the interactive/foreground task. (Although to be noted is that
without the fair scheduler completed there are possible starvation issues
depending on the workload which cannot be fixed by this patch.)

Now results with GuC (although it is not default on DG1):

   Background nice  |   Interactive FPS
 -------------------+--------------------
      <not running> |         58
                  0 |         26
                 10 |         25

Unfortunately GuC is not showing any change (25 vs 26 is rounding/run error).
But reverse mesurement with background client at nice 0 and foreground at nice
-10 does give 40 FPS proving the priority adjustment does work. (Same reverse
test gives 46 FPS with execlists). What is happening with GuC here is something
to be looked at since it seems normal-vs-low GuC priority time slices
differently than normal-vs-high. Normal does not seem to be preferred over low,
in this test at least.

v2:
 * Moved notifier outside task_rq_lock.
 * Some improvements and restructuring on the i915 side of the series.

v3:
 * Dropped task nice notifier - inheriting nice on request submit time is good
   enough.

v4:
 * Realisation came that this can be heavily simplified and only one simple
   patch is enough to achieve the desired behaviour.
 * Fixed the priority adjustment location to actually worked after rebase!
 * Re-done the benchmarking.

Tvrtko Ursulin (1):
  drm/i915: Inherit submitter nice when scheduling requests

 drivers/gpu/drm/i915/i915_request.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

-- 
2.32.0

[Intel-gfx] [PATCH 0/1] Inherit GPU scheduling priority from process nice

[Tvrtko Ursulin]

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

Current processing landscape seems to be more and more composed of pipelines
where computations are done on multiple hardware devices. Furthermore some of
the non-CPU devices, like in this case many GPUs supported by the i915 driver,
actually support priority based scheduling which is currently rather
inaccessible to the user (in terms of being able to control it from the
outside).

From these two statements a question arises on how to allow for a simple,
effective and consolidated user experience. In other words why user would not be
able to do something like:

 $ nice ffmmpeg ...transcode my videos...
 $ my-favourite-game

And have the nice hint apply to GPU parts of the transcode pipeline as well?

This would in fact follow the approach taken by kernel's block scheduler where
ionice is by default inherited from process nice.

This series implements the same idea by inheriting context creator and batch
buffer submitted nice value as context nice. To avoid influencing GPU scheduling
aware clients this is done only one for contexts where userspace hasn't
explicitly specified a non-default scheduling priority

The approach is completely compatible with GuC and drm/scheduler since all
support at least low/normal/high priority levels with just the granularity of
available control differing. In other words with GuC scheduling there is no
difference between nice 5 and 10, both would map to low priority, but the
default case of positive or negative nice, versus nice 0, is still correctly
propagated to the firmware scheduler.

With the series applied I simulated the scenario of a background GPU task
running simultaneously with an interactive client, varying the former's nice
value.

Simulating a non-interactive GPU background task was:
  vblank_mode=0 nice -n <N> glxgears -geometry 1600x800

Interactive client was simulated with:
  gem_wsim -w ~/test.wsim -r 300 -v # (This one is self-capped at ~60fps.)

These were the results on DG1, first with execlists (default):

   Background nice  |   Interactive FPS
 -------------------+--------------------
      <not running> |         59
                  0 |         35
                 10 |         42

As we can see running the background load with nice 10 can somewhat help the
performance of the interactive/foreground task. (Although to be noted is that
without the fair scheduler completed there are possible starvation issues
depending on the workload which cannot be fixed by this patch.)

Now results with GuC (although it is not default on DG1):

   Background nice  |   Interactive FPS
 -------------------+--------------------
      <not running> |         58
                  0 |         26
                 10 |         25

Unfortunately GuC is not showing any change (25 vs 26 is rounding/run error).
But reverse mesurement with background client at nice 0 and foreground at nice
-10 does give 40 FPS proving the priority adjustment does work. (Same reverse
test gives 46 FPS with execlists). What is happening with GuC here is something
to be looked at since it seems normal-vs-low GuC priority time slices
differently than normal-vs-high. Normal does not seem to be preferred over low,
in this test at least.

v2:
 * Moved notifier outside task_rq_lock.
 * Some improvements and restructuring on the i915 side of the series.

v3:
 * Dropped task nice notifier - inheriting nice on request submit time is good
   enough.

v4:
 * Realisation came that this can be heavily simplified and only one simple
   patch is enough to achieve the desired behaviour.
 * Fixed the priority adjustment location to actually worked after rebase!
 * Re-done the benchmarking.

Tvrtko Ursulin (1):
  drm/i915: Inherit submitter nice when scheduling requests

 drivers/gpu/drm/i915/i915_request.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

-- 
2.32.0

[PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Tvrtko Ursulin]

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

Inherit submitter nice at point of request submission to account for long
running processes getting either externally or self re-niced.

This accounts for the current processing landscape where computational
pipelines are composed of CPU and GPU parts working in tandem.

Nice value will only apply to requests which originate from user contexts
and have default context priority. This is to avoid disturbing any
application made choices of low and high (batch processing and latency
sensitive compositing). In this case nice value adjusts the effective
priority in the narrow band of -19 to +20 around
I915_CONTEXT_DEFAULT_PRIORITY.

This means that userspace using the context priority uapi directly has a
wider range of possible adjustments (in practice that only applies to
execlists platforms - with GuC there are only three priority buckets), but
in all cases nice adjustment has the expected effect: positive nice
lowering the scheduling priority and negative nice raising it.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 582770360ad1..e5cfa073d8f0 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1811,8 +1811,17 @@ void i915_request_add(struct i915_request *rq)
 	/* XXX placeholder for selftests */
 	rcu_read_lock();
 	ctx = rcu_dereference(rq->context->gem_context);
-	if (ctx)
+	if (ctx) {
 		attr = ctx->sched;
+		/*
+		 * Inherit process nice when scheduling user contexts but only
+		 * if context has the default priority to avoid touching
+		 * contexts where GEM uapi has been used to explicitly lower
+		 * or elevate it.
+		 */
+		if (attr.priority == I915_CONTEXT_DEFAULT_PRIORITY)
+			attr.priority = -task_nice(current);
+	}
 	rcu_read_unlock();
 
 	__i915_request_queue(rq, &attr);
-- 
2.32.0

[Intel-gfx] [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Tvrtko Ursulin]

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

Inherit submitter nice at point of request submission to account for long
running processes getting either externally or self re-niced.

This accounts for the current processing landscape where computational
pipelines are composed of CPU and GPU parts working in tandem.

Nice value will only apply to requests which originate from user contexts
and have default context priority. This is to avoid disturbing any
application made choices of low and high (batch processing and latency
sensitive compositing). In this case nice value adjusts the effective
priority in the narrow band of -19 to +20 around
I915_CONTEXT_DEFAULT_PRIORITY.

This means that userspace using the context priority uapi directly has a
wider range of possible adjustments (in practice that only applies to
execlists platforms - with GuC there are only three priority buckets), but
in all cases nice adjustment has the expected effect: positive nice
lowering the scheduling priority and negative nice raising it.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_request.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
index 582770360ad1..e5cfa073d8f0 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1811,8 +1811,17 @@ void i915_request_add(struct i915_request *rq)
 	/* XXX placeholder for selftests */
 	rcu_read_lock();
 	ctx = rcu_dereference(rq->context->gem_context);
-	if (ctx)
+	if (ctx) {
 		attr = ctx->sched;
+		/*
+		 * Inherit process nice when scheduling user contexts but only
+		 * if context has the default priority to avoid touching
+		 * contexts where GEM uapi has been used to explicitly lower
+		 * or elevate it.
+		 */
+		if (attr.priority == I915_CONTEXT_DEFAULT_PRIORITY)
+			attr.priority = -task_nice(current);
+	}
 	rcu_read_unlock();
 
 	__i915_request_queue(rq, &attr);
-- 
2.32.0

[Intel-gfx] ✗ Fi.CI.BAT: failure for Inherit GPU scheduling priority from process nice (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 12059 bytes --]

== Series Details ==

Series: Inherit GPU scheduling priority from process nice (rev2)
URL   : https://patchwork.freedesktop.org/series/102203/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11472 -> Patchwork_22816
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_22816 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_22816, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/index.html

Participating hosts (47 -> 49)
------------------------------

  Additional (5): bat-adlm-1 bat-adlp-4 bat-hsw-1 fi-pnv-d510 bat-rpls-2 
  Missing    (3): fi-bsw-cyan shard-tglu fi-bdw-samus 

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

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

### CI changes ###

#### Possible regressions ####

  * boot:
    - fi-pnv-d510:        NOTRUN -> [FAIL][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-pnv-d510/boot.html

  

### IGT changes ###

#### Possible regressions ####

  * igt@gem_lmem_swapping@basic:
    - fi-cfl-8109u:       NOTRUN -> [FAIL][2]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-cfl-8109u/igt@gem_lmem_swapping@basic.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - fi-hsw-4770:        NOTRUN -> [FAIL][3]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-hsw-4770/igt@gem_lmem_swapping@parallel-random-engines.html

  
#### Warnings ####

  * igt@gem_lmem_swapping@basic:
    - fi-skl-guc:         [FAIL][4] ([i915#4547]) -> [FAIL][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-skl-guc/igt@gem_lmem_swapping@basic.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-skl-guc/igt@gem_lmem_swapping@basic.html

  * igt@runner@aborted:
    - fi-rkl-guc:         [FAIL][6] ([i915#4312]) -> [FAIL][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-rkl-guc/igt@runner@aborted.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-rkl-guc/igt@runner@aborted.html
    - fi-adl-ddr5:        [FAIL][8] ([i915#4312]) -> [FAIL][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-adl-ddr5/igt@runner@aborted.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-adl-ddr5/igt@runner@aborted.html
    - fi-kbl-7567u:       [FAIL][10] ([i915#4312]) -> [FAIL][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-kbl-7567u/igt@runner@aborted.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-kbl-7567u/igt@runner@aborted.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@gem_busy@busy:
    - {bat-adlp-6}:       NOTRUN -> [SKIP][12] +146 similar issues
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/bat-adlp-6/igt@gem_busy@busy.html

  * igt@gem_lmem_swapping@basic:
    - {bat-rpls-2}:       NOTRUN -> [SKIP][13] +146 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/bat-rpls-2/igt@gem_lmem_swapping@basic.html
    - {bat-dg2-9}:        NOTRUN -> [SKIP][14]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/bat-dg2-9/igt@gem_lmem_swapping@basic.html

  * igt@kms_cursor_legacy@basic-flip-after-cursor-atomic:
    - {bat-jsl-2}:        NOTRUN -> [SKIP][15] +146 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/bat-jsl-2/igt@kms_cursor_legacy@basic-flip-after-cursor-atomic.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-a:
    - {bat-adlm-1}:       NOTRUN -> [SKIP][16] +77 similar issues
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/bat-adlm-1/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-a.html

  * igt@runner@aborted:
    - {bat-adls-5}:       [FAIL][17] ([i915#4312]) -> [FAIL][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/bat-adls-5/igt@runner@aborted.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/bat-adls-5/igt@runner@aborted.html
    - {bat-hsw-1}:        NOTRUN -> [FAIL][19]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/bat-hsw-1/igt@runner@aborted.html
    - {bat-jsl-1}:        [FAIL][20] ([i915#4312]) -> [FAIL][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/bat-jsl-1/igt@runner@aborted.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/bat-jsl-1/igt@runner@aborted.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@core_auth@basic-auth:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][22] ([fdo#109271]) +1 similar issue
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-kbl-8809g/igt@core_auth@basic-auth.html

  * igt@fbdev@eof:
    - fi-kbl-8809g:       NOTRUN -> [INCOMPLETE][23] ([i915#5557])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-kbl-8809g/igt@fbdev@eof.html

  * igt@gem_close_race@basic-process:
    - fi-ivb-3770:        NOTRUN -> [SKIP][24] ([fdo#109271]) +146 similar issues
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-ivb-3770/igt@gem_close_race@basic-process.html

  * igt@gem_exec_fence@basic-await:
    - fi-bsw-nick:        NOTRUN -> [SKIP][25] ([fdo#109271]) +151 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-bsw-nick/igt@gem_exec_fence@basic-await.html

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][26] ([fdo#109271]) +146 similar issues
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-kbl-soraka/igt@kms_flip@basic-flip-vs-dpms.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-b:
    - fi-cfl-8109u:       NOTRUN -> [SKIP][27] ([fdo#109271]) +145 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-b.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c:
    - fi-cfl-8109u:       NOTRUN -> [SKIP][28] ([fdo#109271] / [i915#5341])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-cfl-8109u/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html
    - fi-bsw-nick:        NOTRUN -> [SKIP][29] ([fdo#109271] / [i915#5341])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-bsw-nick/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html
    - fi-kbl-soraka:      NOTRUN -> [SKIP][30] ([fdo#109271] / [i915#5341])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-kbl-soraka/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html
    - fi-ivb-3770:        NOTRUN -> [SKIP][31] ([fdo#109271] / [i915#5341])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-ivb-3770/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-c.html

  * igt@runner@aborted:
    - bat-adlp-4:         NOTRUN -> [FAIL][32] ([i915#5457])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/bat-adlp-4/igt@runner@aborted.html

  
#### Warnings ####

  * igt@gem_lmem_swapping@basic:
    - fi-hsw-4770:        [FAIL][33] -> [SKIP][34] ([fdo#109271])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-hsw-4770/igt@gem_lmem_swapping@basic.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-hsw-4770/igt@gem_lmem_swapping@basic.html

  * igt@runner@aborted:
    - fi-cfl-8109u:       [FAIL][35] -> [FAIL][36] ([i915#4312])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-cfl-8109u/igt@runner@aborted.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-cfl-8109u/igt@runner@aborted.html
    - fi-bsw-nick:        [FAIL][37] ([i915#3690]) -> [FAIL][38] ([i915#3690] / [i915#4312])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-bsw-nick/igt@runner@aborted.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-bsw-nick/igt@runner@aborted.html
    - fi-kbl-8809g:       [FAIL][39] -> [FAIL][40] ([i915#2722])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-kbl-8809g/igt@runner@aborted.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-kbl-8809g/igt@runner@aborted.html
    - fi-kbl-soraka:      [FAIL][41] -> [FAIL][42] ([i915#4312])
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-kbl-soraka/igt@runner@aborted.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-kbl-soraka/igt@runner@aborted.html
    - fi-hsw-4770:        [FAIL][43] ([i915#4312]) -> [FAIL][44] ([i915#4312] / [i915#5594])
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-hsw-4770/igt@runner@aborted.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-hsw-4770/igt@runner@aborted.html
    - fi-ivb-3770:        [FAIL][45] -> [FAIL][46] ([i915#4312])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-ivb-3770/igt@runner@aborted.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-ivb-3770/igt@runner@aborted.html
    - fi-tgl-1115g4:      [FAIL][47] ([i915#4312] / [i915#5257]) -> [FAIL][48] ([i915#3690])
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-tgl-1115g4/igt@runner@aborted.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-tgl-1115g4/igt@runner@aborted.html
    - fi-skl-guc:         [FAIL][49] ([i915#4312] / [i915#5257]) -> [FAIL][50] ([i915#4312])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-skl-guc/igt@runner@aborted.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-skl-guc/igt@runner@aborted.html
    - fi-bsw-n3050:       [FAIL][51] ([i915#4312]) -> [FAIL][52] ([i915#3690] / [i915#4312])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11472/fi-bsw-n3050/igt@runner@aborted.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/fi-bsw-n3050/igt@runner@aborted.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
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2722]: https://gitlab.freedesktop.org/drm/intel/issues/2722
  [i915#3690]: https://gitlab.freedesktop.org/drm/intel/issues/3690
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4547]: https://gitlab.freedesktop.org/drm/intel/issues/4547
  [i915#5171]: https://gitlab.freedesktop.org/drm/intel/issues/5171
  [i915#5174]: https://gitlab.freedesktop.org/drm/intel/issues/5174
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5257]: https://gitlab.freedesktop.org/drm/intel/issues/5257
  [i915#5341]: https://gitlab.freedesktop.org/drm/intel/issues/5341
  [i915#5457]: https://gitlab.freedesktop.org/drm/intel/issues/5457
  [i915#5557]: https://gitlab.freedesktop.org/drm/intel/issues/5557
  [i915#5594]: https://gitlab.freedesktop.org/drm/intel/issues/5594


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

  * Linux: CI_DRM_11472 -> Patchwork_22816

  CI-20190529: 20190529
  CI_DRM_11472: 85882df13168c5f46b41401b96975de857e3ccac @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6415: c3b690bd5f7fb1fb7ed786ab0f3b815930a6a55f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_22816: e6a3a5daa01ec40fddaa59080c65480be07e0856 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

e6a3a5daa01e drm/i915: Inherit submitter nice when scheduling requests

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_22816/index.html

[-- Attachment #2: Type: text/html, Size: 15245 bytes --]

Re: [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Daniel Vetter]

On Thu, Apr 07, 2022 at 04:16:27PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Inherit submitter nice at point of request submission to account for long
> running processes getting either externally or self re-niced.
> 
> This accounts for the current processing landscape where computational
> pipelines are composed of CPU and GPU parts working in tandem.
> 
> Nice value will only apply to requests which originate from user contexts
> and have default context priority. This is to avoid disturbing any
> application made choices of low and high (batch processing and latency
> sensitive compositing). In this case nice value adjusts the effective
> priority in the narrow band of -19 to +20 around
> I915_CONTEXT_DEFAULT_PRIORITY.
> 
> This means that userspace using the context priority uapi directly has a
> wider range of possible adjustments (in practice that only applies to
> execlists platforms - with GuC there are only three priority buckets), but
> in all cases nice adjustment has the expected effect: positive nice
> lowering the scheduling priority and negative nice raising it.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I don't think adding any more fancy features to i915-scheduler makes
sense, at least not before we've cut over to drm/sched.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_request.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 582770360ad1..e5cfa073d8f0 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1811,8 +1811,17 @@ void i915_request_add(struct i915_request *rq)
>  	/* XXX placeholder for selftests */
>  	rcu_read_lock();
>  	ctx = rcu_dereference(rq->context->gem_context);
> -	if (ctx)
> +	if (ctx) {
>  		attr = ctx->sched;
> +		/*
> +		 * Inherit process nice when scheduling user contexts but only
> +		 * if context has the default priority to avoid touching
> +		 * contexts where GEM uapi has been used to explicitly lower
> +		 * or elevate it.
> +		 */
> +		if (attr.priority == I915_CONTEXT_DEFAULT_PRIORITY)
> +			attr.priority = -task_nice(current);
> +	}
>  	rcu_read_unlock();
>  
>  	__i915_request_queue(rq, &attr);
> -- 
> 2.32.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Daniel Vetter]

On Thu, Apr 07, 2022 at 04:16:27PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Inherit submitter nice at point of request submission to account for long
> running processes getting either externally or self re-niced.
> 
> This accounts for the current processing landscape where computational
> pipelines are composed of CPU and GPU parts working in tandem.
> 
> Nice value will only apply to requests which originate from user contexts
> and have default context priority. This is to avoid disturbing any
> application made choices of low and high (batch processing and latency
> sensitive compositing). In this case nice value adjusts the effective
> priority in the narrow band of -19 to +20 around
> I915_CONTEXT_DEFAULT_PRIORITY.
> 
> This means that userspace using the context priority uapi directly has a
> wider range of possible adjustments (in practice that only applies to
> execlists platforms - with GuC there are only three priority buckets), but
> in all cases nice adjustment has the expected effect: positive nice
> lowering the scheduling priority and negative nice raising it.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I don't think adding any more fancy features to i915-scheduler makes
sense, at least not before we've cut over to drm/sched.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_request.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 582770360ad1..e5cfa073d8f0 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -1811,8 +1811,17 @@ void i915_request_add(struct i915_request *rq)
>  	/* XXX placeholder for selftests */
>  	rcu_read_lock();
>  	ctx = rcu_dereference(rq->context->gem_context);
> -	if (ctx)
> +	if (ctx) {
>  		attr = ctx->sched;
> +		/*
> +		 * Inherit process nice when scheduling user contexts but only
> +		 * if context has the default priority to avoid touching
> +		 * contexts where GEM uapi has been used to explicitly lower
> +		 * or elevate it.
> +		 */
> +		if (attr.priority == I915_CONTEXT_DEFAULT_PRIORITY)
> +			attr.priority = -task_nice(current);
> +	}
>  	rcu_read_unlock();
>  
>  	__i915_request_queue(rq, &attr);
> -- 
> 2.32.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Tvrtko Ursulin]

On 08/04/2022 08:58, Daniel Vetter wrote:
> On Thu, Apr 07, 2022 at 04:16:27PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Inherit submitter nice at point of request submission to account for long
>> running processes getting either externally or self re-niced.
>>
>> This accounts for the current processing landscape where computational
>> pipelines are composed of CPU and GPU parts working in tandem.
>>
>> Nice value will only apply to requests which originate from user contexts
>> and have default context priority. This is to avoid disturbing any
>> application made choices of low and high (batch processing and latency
>> sensitive compositing). In this case nice value adjusts the effective
>> priority in the narrow band of -19 to +20 around
>> I915_CONTEXT_DEFAULT_PRIORITY.
>>
>> This means that userspace using the context priority uapi directly has a
>> wider range of possible adjustments (in practice that only applies to
>> execlists platforms - with GuC there are only three priority buckets), but
>> in all cases nice adjustment has the expected effect: positive nice
>> lowering the scheduling priority and negative nice raising it.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> I don't think adding any more fancy features to i915-scheduler makes
> sense, at least not before we've cut over to drm/sched.

Why do you think so?

Drm/sched has at least low/normal/high priority and surely we will keep 
the i915 context priority ABI.

Then this patch is not touching the i915 scheduler at all, neither it is 
fancy.

The cover letter explains how it implements the same approach as the IO 
scheduler. And it explains the reasoning and benefits. Provides an user 
experience benefit today, which can easily be preserved.

Regards,

Tvrtko

> -Daniel
> 
>> ---
>>   drivers/gpu/drm/i915/i915_request.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>> index 582770360ad1..e5cfa073d8f0 100644
>> --- a/drivers/gpu/drm/i915/i915_request.c
>> +++ b/drivers/gpu/drm/i915/i915_request.c
>> @@ -1811,8 +1811,17 @@ void i915_request_add(struct i915_request *rq)
>>   	/* XXX placeholder for selftests */
>>   	rcu_read_lock();
>>   	ctx = rcu_dereference(rq->context->gem_context);
>> -	if (ctx)
>> +	if (ctx) {
>>   		attr = ctx->sched;
>> +		/*
>> +		 * Inherit process nice when scheduling user contexts but only
>> +		 * if context has the default priority to avoid touching
>> +		 * contexts where GEM uapi has been used to explicitly lower
>> +		 * or elevate it.
>> +		 */
>> +		if (attr.priority == I915_CONTEXT_DEFAULT_PRIORITY)
>> +			attr.priority = -task_nice(current);
>> +	}
>>   	rcu_read_unlock();
>>   
>>   	__i915_request_queue(rq, &attr);
>> -- 
>> 2.32.0
>>
> 

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Tvrtko Ursulin]

On 08/04/2022 08:58, Daniel Vetter wrote:
> On Thu, Apr 07, 2022 at 04:16:27PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Inherit submitter nice at point of request submission to account for long
>> running processes getting either externally or self re-niced.
>>
>> This accounts for the current processing landscape where computational
>> pipelines are composed of CPU and GPU parts working in tandem.
>>
>> Nice value will only apply to requests which originate from user contexts
>> and have default context priority. This is to avoid disturbing any
>> application made choices of low and high (batch processing and latency
>> sensitive compositing). In this case nice value adjusts the effective
>> priority in the narrow band of -19 to +20 around
>> I915_CONTEXT_DEFAULT_PRIORITY.
>>
>> This means that userspace using the context priority uapi directly has a
>> wider range of possible adjustments (in practice that only applies to
>> execlists platforms - with GuC there are only three priority buckets), but
>> in all cases nice adjustment has the expected effect: positive nice
>> lowering the scheduling priority and negative nice raising it.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> I don't think adding any more fancy features to i915-scheduler makes
> sense, at least not before we've cut over to drm/sched.

Why do you think so?

Drm/sched has at least low/normal/high priority and surely we will keep 
the i915 context priority ABI.

Then this patch is not touching the i915 scheduler at all, neither it is 
fancy.

The cover letter explains how it implements the same approach as the IO 
scheduler. And it explains the reasoning and benefits. Provides an user 
experience benefit today, which can easily be preserved.

Regards,

Tvrtko

> -Daniel
> 
>> ---
>>   drivers/gpu/drm/i915/i915_request.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
>> index 582770360ad1..e5cfa073d8f0 100644
>> --- a/drivers/gpu/drm/i915/i915_request.c
>> +++ b/drivers/gpu/drm/i915/i915_request.c
>> @@ -1811,8 +1811,17 @@ void i915_request_add(struct i915_request *rq)
>>   	/* XXX placeholder for selftests */
>>   	rcu_read_lock();
>>   	ctx = rcu_dereference(rq->context->gem_context);
>> -	if (ctx)
>> +	if (ctx) {
>>   		attr = ctx->sched;
>> +		/*
>> +		 * Inherit process nice when scheduling user contexts but only
>> +		 * if context has the default priority to avoid touching
>> +		 * contexts where GEM uapi has been used to explicitly lower
>> +		 * or elevate it.
>> +		 */
>> +		if (attr.priority == I915_CONTEXT_DEFAULT_PRIORITY)
>> +			attr.priority = -task_nice(current);
>> +	}
>>   	rcu_read_unlock();
>>   
>>   	__i915_request_queue(rq, &attr);
>> -- 
>> 2.32.0
>>
> 

Re: [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Dave Airlie]

On Fri, 8 Apr 2022 at 18:25, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 08/04/2022 08:58, Daniel Vetter wrote:
> > On Thu, Apr 07, 2022 at 04:16:27PM +0100, Tvrtko Ursulin wrote:
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> Inherit submitter nice at point of request submission to account for long
> >> running processes getting either externally or self re-niced.
> >>
> >> This accounts for the current processing landscape where computational
> >> pipelines are composed of CPU and GPU parts working in tandem.
> >>
> >> Nice value will only apply to requests which originate from user contexts
> >> and have default context priority. This is to avoid disturbing any
> >> application made choices of low and high (batch processing and latency
> >> sensitive compositing). In this case nice value adjusts the effective
> >> priority in the narrow band of -19 to +20 around
> >> I915_CONTEXT_DEFAULT_PRIORITY.
> >>
> >> This means that userspace using the context priority uapi directly has a
> >> wider range of possible adjustments (in practice that only applies to
> >> execlists platforms - with GuC there are only three priority buckets), but
> >> in all cases nice adjustment has the expected effect: positive nice
> >> lowering the scheduling priority and negative nice raising it.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> > I don't think adding any more fancy features to i915-scheduler makes
> > sense, at least not before we've cut over to drm/sched.
>
> Why do you think so?
>
> Drm/sched has at least low/normal/high priority and surely we will keep
> the i915 context priority ABI.
>
> Then this patch is not touching the i915 scheduler at all, neither it is
> fancy.
>
> The cover letter explains how it implements the same approach as the IO
> scheduler. And it explains the reasoning and benefits. Provides an user
> experience benefit today, which can easily be preserved.

won't this cause uAPI divergence between execlists and GuC, like if
something nices to -15 or -18 with execlists and the same with GuC it
won't get the same sort of result will it?

Dave.

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Dave Airlie]

On Fri, 8 Apr 2022 at 18:25, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 08/04/2022 08:58, Daniel Vetter wrote:
> > On Thu, Apr 07, 2022 at 04:16:27PM +0100, Tvrtko Ursulin wrote:
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> Inherit submitter nice at point of request submission to account for long
> >> running processes getting either externally or self re-niced.
> >>
> >> This accounts for the current processing landscape where computational
> >> pipelines are composed of CPU and GPU parts working in tandem.
> >>
> >> Nice value will only apply to requests which originate from user contexts
> >> and have default context priority. This is to avoid disturbing any
> >> application made choices of low and high (batch processing and latency
> >> sensitive compositing). In this case nice value adjusts the effective
> >> priority in the narrow band of -19 to +20 around
> >> I915_CONTEXT_DEFAULT_PRIORITY.
> >>
> >> This means that userspace using the context priority uapi directly has a
> >> wider range of possible adjustments (in practice that only applies to
> >> execlists platforms - with GuC there are only three priority buckets), but
> >> in all cases nice adjustment has the expected effect: positive nice
> >> lowering the scheduling priority and negative nice raising it.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> > I don't think adding any more fancy features to i915-scheduler makes
> > sense, at least not before we've cut over to drm/sched.
>
> Why do you think so?
>
> Drm/sched has at least low/normal/high priority and surely we will keep
> the i915 context priority ABI.
>
> Then this patch is not touching the i915 scheduler at all, neither it is
> fancy.
>
> The cover letter explains how it implements the same approach as the IO
> scheduler. And it explains the reasoning and benefits. Provides an user
> experience benefit today, which can easily be preserved.

won't this cause uAPI divergence between execlists and GuC, like if
something nices to -15 or -18 with execlists and the same with GuC it
won't get the same sort of result will it?

Dave.

Re: [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Tvrtko Ursulin]

On 08/04/2022 10:50, Dave Airlie wrote:
> On Fri, 8 Apr 2022 at 18:25, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 08/04/2022 08:58, Daniel Vetter wrote:
>>> On Thu, Apr 07, 2022 at 04:16:27PM +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Inherit submitter nice at point of request submission to account for long
>>>> running processes getting either externally or self re-niced.
>>>>
>>>> This accounts for the current processing landscape where computational
>>>> pipelines are composed of CPU and GPU parts working in tandem.
>>>>
>>>> Nice value will only apply to requests which originate from user contexts
>>>> and have default context priority. This is to avoid disturbing any
>>>> application made choices of low and high (batch processing and latency
>>>> sensitive compositing). In this case nice value adjusts the effective
>>>> priority in the narrow band of -19 to +20 around
>>>> I915_CONTEXT_DEFAULT_PRIORITY.
>>>>
>>>> This means that userspace using the context priority uapi directly has a
>>>> wider range of possible adjustments (in practice that only applies to
>>>> execlists platforms - with GuC there are only three priority buckets), but
>>>> in all cases nice adjustment has the expected effect: positive nice
>>>> lowering the scheduling priority and negative nice raising it.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> I don't think adding any more fancy features to i915-scheduler makes
>>> sense, at least not before we've cut over to drm/sched.
>>
>> Why do you think so?
>>
>> Drm/sched has at least low/normal/high priority and surely we will keep
>> the i915 context priority ABI.
>>
>> Then this patch is not touching the i915 scheduler at all, neither it is
>> fancy.
>>
>> The cover letter explains how it implements the same approach as the IO
>> scheduler. And it explains the reasoning and benefits. Provides an user
>> experience benefit today, which can easily be preserved.
> 
> won't this cause uAPI divergence between execlists and GuC, like if
> something nices to -15 or -18 with execlists and the same with GuC it
> won't get the same sort of result will it?

Not sure what you consider new ABI divergence but the general problem 
space of execlists vs GuC priority handling is not related to this patch.

Existing GEM context ABI has -1023 - +1023 for user priorities while GuC 
maps that to low/normal/high only. I915_CONTEXT_DEFAULT_PRIORITY is zero 
which maps to GuC normal. Negatives map to GuC low and positives to 
high. Drm/sched is I understand similar or the same.

So any userspace using the existing uapi can already observe differences 
between GuC and execlists. With your example of -15 vs -18 I mean.

I don't think anyone considered that a problem because execution order 
based on priority is not a hard guarantee. Neither is proportionality of 
timeslicing. Otherwise GuC would already be breaking the ABI.

With this patch it simply allows external control - whereas before only 
applications could change their priorities, now users can influence the 
priority of the ones which did not bother to set a non-default priority.

In the case of GuC if user says "nice 10 churn-my-dataset-on-gpu && 
run-my-game", former part get low prio, latter gets normal. I don't see 
any issues there. Same as if the "churn-my-dataset-on-gpu" command 
implemented a command line switch which passed context priority to i915 
via the existing GEM context param ioctl.

I've described the exact experiments in both modes in the cover letter 
which shows it works. (Ignoring the GuC scheduling quirk where 
apparently low-vs-normal timeslices worse than normal-vs-high).

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Tvrtko Ursulin]

On 08/04/2022 10:50, Dave Airlie wrote:
> On Fri, 8 Apr 2022 at 18:25, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 08/04/2022 08:58, Daniel Vetter wrote:
>>> On Thu, Apr 07, 2022 at 04:16:27PM +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> Inherit submitter nice at point of request submission to account for long
>>>> running processes getting either externally or self re-niced.
>>>>
>>>> This accounts for the current processing landscape where computational
>>>> pipelines are composed of CPU and GPU parts working in tandem.
>>>>
>>>> Nice value will only apply to requests which originate from user contexts
>>>> and have default context priority. This is to avoid disturbing any
>>>> application made choices of low and high (batch processing and latency
>>>> sensitive compositing). In this case nice value adjusts the effective
>>>> priority in the narrow band of -19 to +20 around
>>>> I915_CONTEXT_DEFAULT_PRIORITY.
>>>>
>>>> This means that userspace using the context priority uapi directly has a
>>>> wider range of possible adjustments (in practice that only applies to
>>>> execlists platforms - with GuC there are only three priority buckets), but
>>>> in all cases nice adjustment has the expected effect: positive nice
>>>> lowering the scheduling priority and negative nice raising it.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> I don't think adding any more fancy features to i915-scheduler makes
>>> sense, at least not before we've cut over to drm/sched.
>>
>> Why do you think so?
>>
>> Drm/sched has at least low/normal/high priority and surely we will keep
>> the i915 context priority ABI.
>>
>> Then this patch is not touching the i915 scheduler at all, neither it is
>> fancy.
>>
>> The cover letter explains how it implements the same approach as the IO
>> scheduler. And it explains the reasoning and benefits. Provides an user
>> experience benefit today, which can easily be preserved.
> 
> won't this cause uAPI divergence between execlists and GuC, like if
> something nices to -15 or -18 with execlists and the same with GuC it
> won't get the same sort of result will it?

Not sure what you consider new ABI divergence but the general problem 
space of execlists vs GuC priority handling is not related to this patch.

Existing GEM context ABI has -1023 - +1023 for user priorities while GuC 
maps that to low/normal/high only. I915_CONTEXT_DEFAULT_PRIORITY is zero 
which maps to GuC normal. Negatives map to GuC low and positives to 
high. Drm/sched is I understand similar or the same.

So any userspace using the existing uapi can already observe differences 
between GuC and execlists. With your example of -15 vs -18 I mean.

I don't think anyone considered that a problem because execution order 
based on priority is not a hard guarantee. Neither is proportionality of 
timeslicing. Otherwise GuC would already be breaking the ABI.

With this patch it simply allows external control - whereas before only 
applications could change their priorities, now users can influence the 
priority of the ones which did not bother to set a non-default priority.

In the case of GuC if user says "nice 10 churn-my-dataset-on-gpu && 
run-my-game", former part get low prio, latter gets normal. I don't see 
any issues there. Same as if the "churn-my-dataset-on-gpu" command 
implemented a command line switch which passed context priority to i915 
via the existing GEM context param ioctl.

I've described the exact experiments in both modes in the cover letter 
which shows it works. (Ignoring the GuC scheduling quirk where 
apparently low-vs-normal timeslices worse than normal-vs-high).

Regards,

Tvrtko

Re: [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Daniel Vetter]

On Fri, 8 Apr 2022 at 12:29, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 08/04/2022 10:50, Dave Airlie wrote:
> > On Fri, 8 Apr 2022 at 18:25, Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com> wrote:
> >>
> >>
> >> On 08/04/2022 08:58, Daniel Vetter wrote:
> >>> On Thu, Apr 07, 2022 at 04:16:27PM +0100, Tvrtko Ursulin wrote:
> >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>
> >>>> Inherit submitter nice at point of request submission to account for long
> >>>> running processes getting either externally or self re-niced.
> >>>>
> >>>> This accounts for the current processing landscape where computational
> >>>> pipelines are composed of CPU and GPU parts working in tandem.
> >>>>
> >>>> Nice value will only apply to requests which originate from user contexts
> >>>> and have default context priority. This is to avoid disturbing any
> >>>> application made choices of low and high (batch processing and latency
> >>>> sensitive compositing). In this case nice value adjusts the effective
> >>>> priority in the narrow band of -19 to +20 around
> >>>> I915_CONTEXT_DEFAULT_PRIORITY.
> >>>>
> >>>> This means that userspace using the context priority uapi directly has a
> >>>> wider range of possible adjustments (in practice that only applies to
> >>>> execlists platforms - with GuC there are only three priority buckets), but
> >>>> in all cases nice adjustment has the expected effect: positive nice
> >>>> lowering the scheduling priority and negative nice raising it.
> >>>>
> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>
> >>> I don't think adding any more fancy features to i915-scheduler makes
> >>> sense, at least not before we've cut over to drm/sched.
> >>
> >> Why do you think so?
> >>
> >> Drm/sched has at least low/normal/high priority and surely we will keep
> >> the i915 context priority ABI.
> >>
> >> Then this patch is not touching the i915 scheduler at all, neither it is
> >> fancy.
> >>
> >> The cover letter explains how it implements the same approach as the IO
> >> scheduler. And it explains the reasoning and benefits. Provides an user
> >> experience benefit today, which can easily be preserved.
> >
> > won't this cause uAPI divergence between execlists and GuC, like if
> > something nices to -15 or -18 with execlists and the same with GuC it
> > won't get the same sort of result will it?
>
> Not sure what you consider new ABI divergence but the general problem
> space of execlists vs GuC priority handling is not related to this patch.

It 100% is.

Mesa only uses 3 priority levels, which means the 1k execlist levels
(or whatever it was) nonsense has not left the barn and we can get it
back in.

This here bakes it in forever as implicit uapi.

> Existing GEM context ABI has -1023 - +1023 for user priorities while GuC
> maps that to low/normal/high only. I915_CONTEXT_DEFAULT_PRIORITY is zero
> which maps to GuC normal. Negatives map to GuC low and positives to
> high. Drm/sched is I understand similar or the same.
>
> So any userspace using the existing uapi can already observe differences
> between GuC and execlists. With your example of -15 vs -18 I mean.
>
> I don't think anyone considered that a problem because execution order
> based on priority is not a hard guarantee. Neither is proportionality of
> timeslicing. Otherwise GuC would already be breaking the ABI.
>
> With this patch it simply allows external control - whereas before only
> applications could change their priorities, now users can influence the
> priority of the ones which did not bother to set a non-default priority.
>
> In the case of GuC if user says "nice 10 churn-my-dataset-on-gpu &&
> run-my-game", former part get low prio, latter gets normal. I don't see
> any issues there. Same as if the "churn-my-dataset-on-gpu" command
> implemented a command line switch which passed context priority to i915
> via the existing GEM context param ioctl.
>
> I've described the exact experiments in both modes in the cover letter
> which shows it works. (Ignoring the GuC scheduling quirk where
> apparently low-vs-normal timeslices worse than normal-vs-high).

Guc is not breaking anything because the _real_ uapi only has 3 levels
(plus one for kernel stuff on top).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Daniel Vetter]

On Fri, 8 Apr 2022 at 12:29, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 08/04/2022 10:50, Dave Airlie wrote:
> > On Fri, 8 Apr 2022 at 18:25, Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com> wrote:
> >>
> >>
> >> On 08/04/2022 08:58, Daniel Vetter wrote:
> >>> On Thu, Apr 07, 2022 at 04:16:27PM +0100, Tvrtko Ursulin wrote:
> >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>
> >>>> Inherit submitter nice at point of request submission to account for long
> >>>> running processes getting either externally or self re-niced.
> >>>>
> >>>> This accounts for the current processing landscape where computational
> >>>> pipelines are composed of CPU and GPU parts working in tandem.
> >>>>
> >>>> Nice value will only apply to requests which originate from user contexts
> >>>> and have default context priority. This is to avoid disturbing any
> >>>> application made choices of low and high (batch processing and latency
> >>>> sensitive compositing). In this case nice value adjusts the effective
> >>>> priority in the narrow band of -19 to +20 around
> >>>> I915_CONTEXT_DEFAULT_PRIORITY.
> >>>>
> >>>> This means that userspace using the context priority uapi directly has a
> >>>> wider range of possible adjustments (in practice that only applies to
> >>>> execlists platforms - with GuC there are only three priority buckets), but
> >>>> in all cases nice adjustment has the expected effect: positive nice
> >>>> lowering the scheduling priority and negative nice raising it.
> >>>>
> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>
> >>> I don't think adding any more fancy features to i915-scheduler makes
> >>> sense, at least not before we've cut over to drm/sched.
> >>
> >> Why do you think so?
> >>
> >> Drm/sched has at least low/normal/high priority and surely we will keep
> >> the i915 context priority ABI.
> >>
> >> Then this patch is not touching the i915 scheduler at all, neither it is
> >> fancy.
> >>
> >> The cover letter explains how it implements the same approach as the IO
> >> scheduler. And it explains the reasoning and benefits. Provides an user
> >> experience benefit today, which can easily be preserved.
> >
> > won't this cause uAPI divergence between execlists and GuC, like if
> > something nices to -15 or -18 with execlists and the same with GuC it
> > won't get the same sort of result will it?
>
> Not sure what you consider new ABI divergence but the general problem
> space of execlists vs GuC priority handling is not related to this patch.

It 100% is.

Mesa only uses 3 priority levels, which means the 1k execlist levels
(or whatever it was) nonsense has not left the barn and we can get it
back in.

This here bakes it in forever as implicit uapi.

> Existing GEM context ABI has -1023 - +1023 for user priorities while GuC
> maps that to low/normal/high only. I915_CONTEXT_DEFAULT_PRIORITY is zero
> which maps to GuC normal. Negatives map to GuC low and positives to
> high. Drm/sched is I understand similar or the same.
>
> So any userspace using the existing uapi can already observe differences
> between GuC and execlists. With your example of -15 vs -18 I mean.
>
> I don't think anyone considered that a problem because execution order
> based on priority is not a hard guarantee. Neither is proportionality of
> timeslicing. Otherwise GuC would already be breaking the ABI.
>
> With this patch it simply allows external control - whereas before only
> applications could change their priorities, now users can influence the
> priority of the ones which did not bother to set a non-default priority.
>
> In the case of GuC if user says "nice 10 churn-my-dataset-on-gpu &&
> run-my-game", former part get low prio, latter gets normal. I don't see
> any issues there. Same as if the "churn-my-dataset-on-gpu" command
> implemented a command line switch which passed context priority to i915
> via the existing GEM context param ioctl.
>
> I've described the exact experiments in both modes in the cover letter
> which shows it works. (Ignoring the GuC scheduling quirk where
> apparently low-vs-normal timeslices worse than normal-vs-high).

Guc is not breaking anything because the _real_ uapi only has 3 levels
(plus one for kernel stuff on top).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Tvrtko Ursulin]

On 08/04/2022 16:10, Daniel Vetter wrote:
> On Fri, 8 Apr 2022 at 12:29, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 08/04/2022 10:50, Dave Airlie wrote:
>>> On Fri, 8 Apr 2022 at 18:25, Tvrtko Ursulin
>>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>>
>>>>
>>>> On 08/04/2022 08:58, Daniel Vetter wrote:
>>>>> On Thu, Apr 07, 2022 at 04:16:27PM +0100, Tvrtko Ursulin wrote:
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>
>>>>>> Inherit submitter nice at point of request submission to account for long
>>>>>> running processes getting either externally or self re-niced.
>>>>>>
>>>>>> This accounts for the current processing landscape where computational
>>>>>> pipelines are composed of CPU and GPU parts working in tandem.
>>>>>>
>>>>>> Nice value will only apply to requests which originate from user contexts
>>>>>> and have default context priority. This is to avoid disturbing any
>>>>>> application made choices of low and high (batch processing and latency
>>>>>> sensitive compositing). In this case nice value adjusts the effective
>>>>>> priority in the narrow band of -19 to +20 around
>>>>>> I915_CONTEXT_DEFAULT_PRIORITY.
>>>>>>
>>>>>> This means that userspace using the context priority uapi directly has a
>>>>>> wider range of possible adjustments (in practice that only applies to
>>>>>> execlists platforms - with GuC there are only three priority buckets), but
>>>>>> in all cases nice adjustment has the expected effect: positive nice
>>>>>> lowering the scheduling priority and negative nice raising it.
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> I don't think adding any more fancy features to i915-scheduler makes
>>>>> sense, at least not before we've cut over to drm/sched.
>>>>
>>>> Why do you think so?
>>>>
>>>> Drm/sched has at least low/normal/high priority and surely we will keep
>>>> the i915 context priority ABI.
>>>>
>>>> Then this patch is not touching the i915 scheduler at all, neither it is
>>>> fancy.
>>>>
>>>> The cover letter explains how it implements the same approach as the IO
>>>> scheduler. And it explains the reasoning and benefits. Provides an user
>>>> experience benefit today, which can easily be preserved.
>>>
>>> won't this cause uAPI divergence between execlists and GuC, like if
>>> something nices to -15 or -18 with execlists and the same with GuC it
>>> won't get the same sort of result will it?
>>
>> Not sure what you consider new ABI divergence but the general problem
>> space of execlists vs GuC priority handling is not related to this patch.
> 
> It 100% is.
> 
> Mesa only uses 3 priority levels, which means the 1k execlist levels
> (or whatever it was) nonsense has not left the barn and we can get it
> back in.
> 
> This here bakes it in forever as implicit uapi.

Could you please explain what exactly you see baking into uapi? The fact 
user gets the ability to control GPU time distribution? The granularity 
of it by observing say difference between nice 5 and nice 6? Something else?

I maintain the uapi did not in any case provide any statements on the 
latter, so I still don't see a problem there.

Regards,

Tvrtko

> 
>> Existing GEM context ABI has -1023 - +1023 for user priorities while GuC
>> maps that to low/normal/high only. I915_CONTEXT_DEFAULT_PRIORITY is zero
>> which maps to GuC normal. Negatives map to GuC low and positives to
>> high. Drm/sched is I understand similar or the same.
>>
>> So any userspace using the existing uapi can already observe differences
>> between GuC and execlists. With your example of -15 vs -18 I mean.
>>
>> I don't think anyone considered that a problem because execution order
>> based on priority is not a hard guarantee. Neither is proportionality of
>> timeslicing. Otherwise GuC would already be breaking the ABI.
>>
>> With this patch it simply allows external control - whereas before only
>> applications could change their priorities, now users can influence the
>> priority of the ones which did not bother to set a non-default priority.
>>
>> In the case of GuC if user says "nice 10 churn-my-dataset-on-gpu &&
>> run-my-game", former part get low prio, latter gets normal. I don't see
>> any issues there. Same as if the "churn-my-dataset-on-gpu" command
>> implemented a command line switch which passed context priority to i915
>> via the existing GEM context param ioctl.
>>
>> I've described the exact experiments in both modes in the cover letter
>> which shows it works. (Ignoring the GuC scheduling quirk where
>> apparently low-vs-normal timeslices worse than normal-vs-high).
> 
> Guc is not breaking anything because the _real_ uapi only has 3 levels
> (plus one for kernel stuff on top).
> -Daniel

Re: [Intel-gfx] [PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Tvrtko Ursulin]

On 08/04/2022 16:10, Daniel Vetter wrote:
> On Fri, 8 Apr 2022 at 12:29, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 08/04/2022 10:50, Dave Airlie wrote:
>>> On Fri, 8 Apr 2022 at 18:25, Tvrtko Ursulin
>>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>>
>>>>
>>>> On 08/04/2022 08:58, Daniel Vetter wrote:
>>>>> On Thu, Apr 07, 2022 at 04:16:27PM +0100, Tvrtko Ursulin wrote:
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>
>>>>>> Inherit submitter nice at point of request submission to account for long
>>>>>> running processes getting either externally or self re-niced.
>>>>>>
>>>>>> This accounts for the current processing landscape where computational
>>>>>> pipelines are composed of CPU and GPU parts working in tandem.
>>>>>>
>>>>>> Nice value will only apply to requests which originate from user contexts
>>>>>> and have default context priority. This is to avoid disturbing any
>>>>>> application made choices of low and high (batch processing and latency
>>>>>> sensitive compositing). In this case nice value adjusts the effective
>>>>>> priority in the narrow band of -19 to +20 around
>>>>>> I915_CONTEXT_DEFAULT_PRIORITY.
>>>>>>
>>>>>> This means that userspace using the context priority uapi directly has a
>>>>>> wider range of possible adjustments (in practice that only applies to
>>>>>> execlists platforms - with GuC there are only three priority buckets), but
>>>>>> in all cases nice adjustment has the expected effect: positive nice
>>>>>> lowering the scheduling priority and negative nice raising it.
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>
>>>>> I don't think adding any more fancy features to i915-scheduler makes
>>>>> sense, at least not before we've cut over to drm/sched.
>>>>
>>>> Why do you think so?
>>>>
>>>> Drm/sched has at least low/normal/high priority and surely we will keep
>>>> the i915 context priority ABI.
>>>>
>>>> Then this patch is not touching the i915 scheduler at all, neither it is
>>>> fancy.
>>>>
>>>> The cover letter explains how it implements the same approach as the IO
>>>> scheduler. And it explains the reasoning and benefits. Provides an user
>>>> experience benefit today, which can easily be preserved.
>>>
>>> won't this cause uAPI divergence between execlists and GuC, like if
>>> something nices to -15 or -18 with execlists and the same with GuC it
>>> won't get the same sort of result will it?
>>
>> Not sure what you consider new ABI divergence but the general problem
>> space of execlists vs GuC priority handling is not related to this patch.
> 
> It 100% is.
> 
> Mesa only uses 3 priority levels, which means the 1k execlist levels
> (or whatever it was) nonsense has not left the barn and we can get it
> back in.
> 
> This here bakes it in forever as implicit uapi.

Could you please explain what exactly you see baking into uapi? The fact 
user gets the ability to control GPU time distribution? The granularity 
of it by observing say difference between nice 5 and nice 6? Something else?

I maintain the uapi did not in any case provide any statements on the 
latter, so I still don't see a problem there.

Regards,

Tvrtko

> 
>> Existing GEM context ABI has -1023 - +1023 for user priorities while GuC
>> maps that to low/normal/high only. I915_CONTEXT_DEFAULT_PRIORITY is zero
>> which maps to GuC normal. Negatives map to GuC low and positives to
>> high. Drm/sched is I understand similar or the same.
>>
>> So any userspace using the existing uapi can already observe differences
>> between GuC and execlists. With your example of -15 vs -18 I mean.
>>
>> I don't think anyone considered that a problem because execution order
>> based on priority is not a hard guarantee. Neither is proportionality of
>> timeslicing. Otherwise GuC would already be breaking the ABI.
>>
>> With this patch it simply allows external control - whereas before only
>> applications could change their priorities, now users can influence the
>> priority of the ones which did not bother to set a non-default priority.
>>
>> In the case of GuC if user says "nice 10 churn-my-dataset-on-gpu &&
>> run-my-game", former part get low prio, latter gets normal. I don't see
>> any issues there. Same as if the "churn-my-dataset-on-gpu" command
>> implemented a command line switch which passed context priority to i915
>> via the existing GEM context param ioctl.
>>
>> I've described the exact experiments in both modes in the cover letter
>> which shows it works. (Ignoring the GuC scheduling quirk where
>> apparently low-vs-normal timeslices worse than normal-vs-high).
> 
> Guc is not breaking anything because the _real_ uapi only has 3 levels
> (plus one for kernel stuff on top).
> -Daniel

[PATCH 1/1] drm/i915: Inherit submitter nice when scheduling requests

[Tvrtko Ursulin]

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

Inherit submitter nice at point of request submission to account for long
running processes getting either externally or self re-niced.

This accounts for the current processing landscape where computational
pipelines are composed of CPU and GPU parts working in tandem.

Nice value will only apply to requests which originate from user contexts
and have default context priority. This is to avoid disturbing any
application made choices of low and high (batch processing and latency
sensitive compositing). In this case nice value adjusts the effective
priority in the narrow band of -19 to +20 around
I915_CONTEXT_DEFAULT_PRIORITY.

This means that userspace using the context priority uapi directly has a
wider range of possible adjustments (in practice that only applies to
execlists platforms - with GuC there are only three priority buckets), but
in all cases nice adjustment has the expected effect: positive nice
lowering the scheduling priority and negative nice raising it.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 50cbc8b4885b..2d5e71029d7c 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -3043,6 +3043,14 @@ static int eb_request_add(struct i915_execbuffer *eb, struct i915_request *rq,
 	/* Check that the context wasn't destroyed before submission */
 	if (likely(!intel_context_is_closed(eb->context))) {
 		attr = eb->gem_context->sched;
+		/*
+		 * Inherit process nice when scheduling user contexts but only
+		 * if context has the default priority to avoid touching
+		 * contexts where GEM uapi has been used to explicitly lower
+		 * or elevate it.
+		 */
+		if (attr.priority == I915_CONTEXT_DEFAULT_PRIORITY)
+			attr.priority = -task_nice(current);
 	} else {
 		/* Serialise with context_close via the add_to_timeline */
 		i915_request_set_error_once(rq, -ENOENT);
-- 
2.32.0