[PATCH] drm/i915: Allow normal clients to always preempt idle priority clients

[PATCH] drm/i915: Allow normal clients to always preempt idle priority clients

[Chris Wilson]

When first enabling preemption, we hesitated from making it a free-for-all
where every higher priority client would force a preempt-to-idle cycle
and take over from all lower priority clients. We hesitated because we
were uncertain just how well preemption would work in practice, whether
the preemption latency itself would detract from the latency gains for
higher priority tasks and whether it would work at all. Since
introducing preemption, we have been enabling it for more common tasks,
even giving normal clients a small preemptive boost when they first
start (to aide fairness and improve interactivity). Now lets take one
step further and give permission for all normal (priority:0) clients to
preempt any idle (priority:<0) task so that users running long compute
jobs do not overly impact other jobs (i.e. their desktop) and the system
remains responsive under such idle loads.

References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
References: b16c765122f9 ("drm/i915: Priority boost for new clients")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: "Bloomfield, Jon" <jon.bloomfield@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 34d0a148e664..983ad1e7914d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -592,7 +592,20 @@ intel_engine_has_preemption(const struct intel_engine_cs *engine)
 
 static inline bool __execlists_need_preempt(int prio, int last)
 {
-	return prio > max(0, last);
+	/*
+	 * Allow preemption of low -> normal -> high, but we do
+	 * not allow low priority tasks to preempt other low priority
+	 * tasks under the impression that latency for low priority
+	 * tasks does not matter (as much as background throughput),
+	 * so kiss.
+	 *
+	 * More naturally we would write
+	 * 	prio >= max(0, last);
+	 * except that we wish to prevent triggering preemption at the same
+	 * priority level: the task that is running should remain running
+	 * to preserve FIFO ordering of dependencies.
+	 */
+	return prio > max(I915_PRIORITY_NORMAL - 1, last);
 }
 
 static inline void
-- 
2.20.1

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

✗ Fi.CI.CHECKPATCH: warning for drm/i915: Allow normal clients to always preempt idle priority clients

[Patchwork]

== Series Details ==

Series: drm/i915: Allow normal clients to always preempt idle priority clients
URL   : https://patchwork.freedesktop.org/series/56072/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
0f047db61683 drm/i915: Allow normal clients to always preempt idle priority clients
-:24: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#24: 
References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")

-:24: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")'
#24: 
References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")

-:25: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit b16c765122f9 ("drm/i915: Priority boost for new clients")'
#25: 
References: b16c765122f9 ("drm/i915: Priority boost for new clients")

-:49: WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#49: FILE: drivers/gpu/drm/i915/intel_ringbuffer.h:603:
+^I * ^Iprio >= max(0, last);$

total: 2 errors, 2 warnings, 0 checks, 21 lines checked

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

✗ Fi.CI.SPARSE: warning for drm/i915: Allow normal clients to always preempt idle priority clients

[Patchwork]

== Series Details ==

Series: drm/i915: Allow normal clients to always preempt idle priority clients
URL   : https://patchwork.freedesktop.org/series/56072/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Allow normal clients to always preempt idle priority clients
-O:drivers/gpu/drm/i915/intel_ringbuffer.h:595:23: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_ringbuffer.h:595:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_ringbuffer.h:608:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_ringbuffer.h:608:23: warning: expression using sizeof(void)

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

✓ Fi.CI.BAT: success for drm/i915: Allow normal clients to always preempt idle priority clients

[Patchwork]

== Series Details ==

Series: drm/i915: Allow normal clients to always preempt idle priority clients
URL   : https://patchwork.freedesktop.org/series/56072/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5522 -> Patchwork_12114
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Suppressed ####

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

  * igt@i915_selftest@live_hangcheck:
    - {fi-icl-y}:         PASS -> INCOMPLETE

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       NOTRUN -> INCOMPLETE [fdo#107718]

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         PASS -> INCOMPLETE [fdo#103927]

  * igt@kms_busy@basic-flip-a:
    - fi-gdg-551:         PASS -> FAIL [fdo#103182] +1

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362] +1

  * igt@pm_rpm@basic-rte:
    - fi-byt-j1900:       PASS -> FAIL [fdo#108800]

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  * igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - fi-kbl-7567u:       {SKIP} [fdo#109271] -> PASS +1

  * igt@prime_vgem@basic-fence-flip:
    - fi-kbl-7500u:       {SKIP} [fdo#109271] -> PASS +33

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#108800]: https://bugs.freedesktop.org/show_bug.cgi?id=108800
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271


Participating hosts (49 -> 45)
------------------------------

  Missing    (4): fi-kbl-soraka fi-ctg-p8600 fi-bsw-cyan fi-ilk-m540 


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

    * Linux: CI_DRM_5522 -> Patchwork_12114

  CI_DRM_5522: 3f287cb6d4ae4689eb7c53e4c25f0fba3df16438 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4802: 4049adf01014af077df2174def4fadf7cecb066e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12114: 0f047db616837cfdcac1e08e596030251cee34ef @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

0f047db61683 drm/i915: Allow normal clients to always preempt idle priority clients

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12114/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Allow normal clients to always preempt idle priority clients

[Patchwork]

== Series Details ==

Series: drm/i915: Allow normal clients to always preempt idle priority clients
URL   : https://patchwork.freedesktop.org/series/56072/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5522_full -> Patchwork_12114_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_softpin@noreloc-s3:
    - shard-snb:          PASS -> DMESG-WARN [fdo#102365]

  * igt@kms_plane@plane-position-covered-pipe-b-planes:
    - shard-apl:          PASS -> FAIL [fdo#103166]

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-none:
    - shard-glk:          PASS -> FAIL [fdo#103166] +2

  
#### Possible fixes ####

  * igt@gem_exec_blt@cold:
    - shard-glk:          DMESG-WARN [fdo#105763] / [fdo#106538] -> PASS

  * igt@kms_plane@plane-position-covered-pipe-a-planes:
    - shard-apl:          FAIL [fdo#103166] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
    - shard-glk:          FAIL [fdo#103166] -> PASS +1

  
#### Warnings ####

  * igt@i915_suspend@shrink:
    - shard-snb:          DMESG-WARN [fdo#109244] -> INCOMPLETE [fdo#105411] / [fdo#106886]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102365]: https://bugs.freedesktop.org/show_bug.cgi?id=102365
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
  [fdo#106886]: https://bugs.freedesktop.org/show_bug.cgi?id=106886
  [fdo#109244]: https://bugs.freedesktop.org/show_bug.cgi?id=109244
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278


Participating hosts (6 -> 4)
------------------------------

  Missing    (2): shard-skl shard-iclb 


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

    * Linux: CI_DRM_5522 -> Patchwork_12114

  CI_DRM_5522: 3f287cb6d4ae4689eb7c53e4c25f0fba3df16438 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4802: 4049adf01014af077df2174def4fadf7cecb066e @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12114: 0f047db616837cfdcac1e08e596030251cee34ef @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12114/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Allow normal clients to always preempt idle priority clients

[Daniele Ceraolo Spurio]

On 01/31/2019 05:42 PM, Chris Wilson wrote:
> When first enabling preemption, we hesitated from making it a free-for-all
> where every higher priority client would force a preempt-to-idle cycle
> and take over from all lower priority clients. We hesitated because we
> were uncertain just how well preemption would work in practice, whether
> the preemption latency itself would detract from the latency gains for
> higher priority tasks and whether it would work at all. Since
> introducing preemption, we have been enabling it for more common tasks,
> even giving normal clients a small preemptive boost when they first
> start (to aide fairness and improve interactivity). Now lets take one
> step further and give permission for all normal (priority:0) clients to
> preempt any idle (priority:<0) task so that users running long compute
> jobs do not overly impact other jobs (i.e. their desktop) and the system
> remains responsive under such idle loads.
> 
> References: f6322eddaff7 ("drm/i915/preemption: Allow preemption between submission ports")
> References: b16c765122f9 ("drm/i915: Priority boost for new clients")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: "Bloomfield, Jon" <jon.bloomfield@intel.com>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> ---
>   drivers/gpu/drm/i915/intel_ringbuffer.h | 15 ++++++++++++++-
>   1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 34d0a148e664..983ad1e7914d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -592,7 +592,20 @@ intel_engine_has_preemption(const struct intel_engine_cs *engine)
>   
>   static inline bool __execlists_need_preempt(int prio, int last)
>   {
> -	return prio > max(0, last);
> +	/*
> +	 * Allow preemption of low -> normal -> high, but we do
> +	 * not allow low priority tasks to preempt other low priority
> +	 * tasks under the impression that latency for low priority
> +	 * tasks does not matter (as much as background throughput),
> +	 * so kiss.
> +	 *
> +	 * More naturally we would write
> +	 * 	prio >= max(0, last);
> +	 * except that we wish to prevent triggering preemption at the same
> +	 * priority level: the task that is running should remain running
> +	 * to preserve FIFO ordering of dependencies.
> +	 */
> +	return prio > max(I915_PRIORITY_NORMAL - 1, last);
>   }
>   
>   static inline void
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx