[Intel-gfx] [PATCH 0/2] Don't send double context enable/disable requests

[Intel-gfx] [PATCH 0/2] Don't send double context enable/disable requests

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

The driver could sometimes send context enable/disable requests when a
previous request was still pending. This is not allowed. So stop doing
it.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>


John Harrison (2):
  drm/i915/guc: Don't double enable a context
  drm/i915/guc: Don't disable a context whose enable is still pending

 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 33 +++++++++++++++----
 1 file changed, 26 insertions(+), 7 deletions(-)

-- 
2.41.0

[Intel-gfx] [PATCH 1/2] drm/i915/guc: Don't double enable a context

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

If a context is blocked, unblocked and subitted repeatedly in rapid
succession, the driver can end up trying to enable the context while
the previous enable request is still in flight. This can lead to much
confusion in the state tracking.

Prevent that by checking the pending enable flag before trying to
enable a context.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index d37698bd6b91a..d399e4d238c10 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -720,7 +720,7 @@ static int __guc_add_request(struct intel_guc *guc, struct i915_request *rq)
 	if (unlikely(context_blocked(ce) && !intel_context_is_parent(ce)))
 		goto out;
 
-	enabled = context_enabled(ce) || context_blocked(ce);
+	enabled = context_enabled(ce) || context_blocked(ce) || context_pending_enable(ce);
 
 	if (!enabled) {
 		action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_SET;
-- 
2.41.0

[Intel-gfx] [PATCH 2/2] drm/i915/guc: Don't disable a context whose enable is still pending

[John.C.Harrison]

From: John Harrison <John.C.Harrison@Intel.com>

Various processes involve requesting GuC to disable a given context.
However context enable/disable is an asynchronous process in the GuC.
Thus, it is possible the previous enable request is still being
processed when the disable request is triggered. Having both enable
and disable in flight concurrently is illegal - GuC will return an
error and fail the second operation. The KMD side handler for the
completion message also can't cope with having both pending flags set.
So delay the disable request until it is safe to send.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 31 +++++++++++++++----
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index d399e4d238c10..8c34b0a5abf9a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -3150,7 +3150,8 @@ guc_context_revoke(struct intel_context *ce, struct i915_request *rq,
 		guc_cancel_context_requests(ce);
 		intel_engine_signal_breadcrumbs(ce->engine);
 	} else if (!context_pending_disable(ce)) {
-		u16 guc_id;
+		u16 guc_id = ~0;
+		bool pending_enable = context_pending_enable(ce);
 
 		/*
 		 * We add +2 here as the schedule disable complete CTB handler
@@ -3158,7 +3159,11 @@ guc_context_revoke(struct intel_context *ce, struct i915_request *rq,
 		 */
 		atomic_add(2, &ce->pin_count);
 
-		guc_id = prep_context_pending_disable(ce);
+		if (pending_enable)
+			guc_id = ce->guc_id.id;
+		else
+			guc_id = prep_context_pending_disable(ce);
+
 		spin_unlock_irqrestore(&ce->guc_state.lock, flags);
 
 		/*
@@ -3169,7 +3174,15 @@ guc_context_revoke(struct intel_context *ce, struct i915_request *rq,
 		with_intel_runtime_pm(runtime_pm, wakeref) {
 			__guc_context_set_preemption_timeout(guc, guc_id,
 							     preempt_timeout_ms);
-			__guc_context_sched_disable(guc, ce, guc_id);
+			if (!pending_enable)
+				__guc_context_sched_disable(guc, ce, guc_id);
+		}
+
+		if (pending_enable) {
+			/* Can't have both in flight concurrently, so try again later... */
+			mod_delayed_work(system_unbound_wq,
+					 &ce->guc_state.sched_disable_delay_work,
+					 msecs_to_jiffies(1));
 		}
 	} else {
 		if (!context_guc_id_invalid(ce))
@@ -3222,7 +3235,13 @@ static void __delay_sched_disable(struct work_struct *wrk)
 
 	spin_lock_irqsave(&ce->guc_state.lock, flags);
 
-	if (bypass_sched_disable(guc, ce)) {
+	if (context_pending_enable(ce)) {
+		spin_unlock_irqrestore(&ce->guc_state.lock, flags);
+		/* Can't have both in flight concurrently, so try again later... */
+		mod_delayed_work(system_unbound_wq,
+				 &ce->guc_state.sched_disable_delay_work,
+				 msecs_to_jiffies(1));
+	} else if (bypass_sched_disable(guc, ce)) {
 		spin_unlock_irqrestore(&ce->guc_state.lock, flags);
 		intel_context_sched_disable_unpin(ce);
 	} else {
@@ -3257,8 +3276,8 @@ static void guc_context_sched_disable(struct intel_context *ce)
 	if (bypass_sched_disable(guc, ce)) {
 		spin_unlock_irqrestore(&ce->guc_state.lock, flags);
 		intel_context_sched_disable_unpin(ce);
-	} else if (!intel_context_is_closed(ce) && !guc_id_pressure(guc, ce) &&
-		   delay) {
+	} else if ((!intel_context_is_closed(ce) && !guc_id_pressure(guc, ce) &&
+		    delay) || context_pending_enable(ce)) {
 		spin_unlock_irqrestore(&ce->guc_state.lock, flags);
 		mod_delayed_work(system_unbound_wq,
 				 &ce->guc_state.sched_disable_delay_work,
-- 
2.41.0

[Intel-gfx] ✗ Fi.CI.BAT: failure for Don't send double context enable/disable requests

[Patchwork]

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

== Series Details ==

Series: Don't send double context enable/disable requests
URL   : https://patchwork.freedesktop.org/series/126234/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13859 -> Patchwork_126234v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_126234v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_126234v1, please notify your bug team (lgci.bug.filing@intel.com) 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_126234v1/index.html

Participating hosts (33 -> 34)
------------------------------

  Additional (2): bat-dg2-8 bat-dg2-9 
  Missing    (1): fi-kbl-soraka 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_parallel@engines@fds:
    - bat-dg2-8:          NOTRUN -> [TIMEOUT][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-8/igt@gem_exec_parallel@engines@fds.html

  * igt@i915_selftest@live@gem_contexts:
    - bat-adlm-1:         [PASS][2] -> [ABORT][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13859/bat-adlm-1/igt@i915_selftest@live@gem_contexts.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-adlm-1/igt@i915_selftest@live@gem_contexts.html

  * igt@i915_selftest@live@requests:
    - bat-atsm-1:         [PASS][4] -> [ABORT][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13859/bat-atsm-1/igt@i915_selftest@live@requests.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-atsm-1/igt@i915_selftest@live@requests.html

  * igt@i915_selftest@live@workarounds:
    - bat-dg2-11:         [PASS][6] -> [ABORT][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13859/bat-dg2-11/igt@i915_selftest@live@workarounds.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-11/igt@i915_selftest@live@workarounds.html
    - bat-dg2-9:          NOTRUN -> [ABORT][8]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@i915_selftest@live@workarounds.html

  * igt@kms_pipe_crc_basic@read-crc@pipe-d-dp-5:
    - bat-adlp-11:        [PASS][9] -> [FAIL][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13859/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc@pipe-d-dp-5.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc@pipe-d-dp-5.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_flink_basic@bad-flink:
    - bat-dg2-8:          NOTRUN -> [TIMEOUT][11] ([i915#8628]) +2 other tests timeout
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-8/igt@gem_flink_basic@bad-flink.html

  * igt@gem_mmap@basic:
    - bat-dg2-9:          NOTRUN -> [SKIP][12] ([i915#4083])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@gem_mmap@basic.html

  * igt@gem_mmap_gtt@basic:
    - bat-dg2-9:          NOTRUN -> [SKIP][13] ([i915#4077]) +2 other tests skip
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@gem_mmap_gtt@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-dg2-9:          NOTRUN -> [SKIP][14] ([i915#4079]) +1 other test skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@gem_render_tiled_blits@basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-dg2-9:          NOTRUN -> [SKIP][15] ([i915#6621])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@i915_pm_rps@basic-api.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-dg2-9:          NOTRUN -> [SKIP][16] ([i915#5190])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-dg2-9:          NOTRUN -> [SKIP][17] ([i915#4215] / [i915#5190])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_addfb_basic@framebuffer-vs-set-tiling:
    - bat-dg2-9:          NOTRUN -> [SKIP][18] ([i915#4212]) +6 other tests skip
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html

  * igt@kms_addfb_basic@tile-pitch-mismatch:
    - bat-dg2-9:          NOTRUN -> [SKIP][19] ([i915#4212] / [i915#5608])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@kms_addfb_basic@tile-pitch-mismatch.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-dg2-9:          NOTRUN -> [SKIP][20] ([i915#4103] / [i915#4213] / [i915#5608]) +1 other test skip
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-dg2-9:          NOTRUN -> [SKIP][21] ([fdo#109285])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-dg2-9:          NOTRUN -> [SKIP][22] ([i915#5274])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-dp-5:
    - bat-adlp-11:        [PASS][23] -> [ABORT][24] ([i915#8668])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13859/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-dp-5.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-dp-5.html

  * igt@kms_pipe_crc_basic@read-crc@pipe-c-dp-5:
    - bat-adlp-11:        [PASS][25] -> [DMESG-FAIL][26] ([i915#6868])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13859/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc@pipe-c-dp-5.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc@pipe-c-dp-5.html

  * igt@kms_psr@sprite_plane_onoff:
    - bat-dg2-9:          NOTRUN -> [SKIP][27] ([i915#1072]) +3 other tests skip
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@kms_psr@sprite_plane_onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-dg2-9:          NOTRUN -> [SKIP][28] ([i915#3555] / [i915#4098])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-flip:
    - bat-dg2-9:          NOTRUN -> [SKIP][29] ([i915#3708])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-dg2-9:          NOTRUN -> [SKIP][30] ([i915#3708] / [i915#4077]) +1 other test skip
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-write:
    - bat-dg2-9:          NOTRUN -> [SKIP][31] ([i915#3291] / [i915#3708]) +2 other tests skip
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-dg2-9/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1:
    - bat-rplp-1:         [ABORT][32] ([i915#8668]) -> [PASS][33]
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13859/bat-rplp-1/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126234v1/bat-rplp-1/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1.html

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

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5608]: https://gitlab.freedesktop.org/drm/intel/issues/5608
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6868]: https://gitlab.freedesktop.org/drm/intel/issues/6868
  [i915#8628]: https://gitlab.freedesktop.org/drm/intel/issues/8628
  [i915#8668]: https://gitlab.freedesktop.org/drm/intel/issues/8668


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

  * Linux: CI_DRM_13859 -> Patchwork_126234v1

  CI-20190529: 20190529
  CI_DRM_13859: 9155ae0ae05f320d84eaf2c4e81413bf937a5f3c @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7581: 7581
  Patchwork_126234v1: 9155ae0ae05f320d84eaf2c4e81413bf937a5f3c @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

05270c8d0f90 drm/i915/guc: Don't disable a context whose enable is still pending
e6dda6635d45 drm/i915/guc: Don't double enable a context

== Logs ==

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

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

Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Don't double enable a context

[Daniele Ceraolo Spurio]

On 11/9/2023 4:54 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> If a context is blocked, unblocked and subitted repeatedly in rapid
> succession, the driver can end up trying to enable the context while
> the previous enable request is still in flight. This can lead to much
> confusion in the state tracking.
>
> Prevent that by checking the pending enable flag before trying to
> enable a context.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

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

Daniele

> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index d37698bd6b91a..d399e4d238c10 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -720,7 +720,7 @@ static int __guc_add_request(struct intel_guc *guc, struct i915_request *rq)
>   	if (unlikely(context_blocked(ce) && !intel_context_is_parent(ce)))
>   		goto out;
>   
> -	enabled = context_enabled(ce) || context_blocked(ce);
> +	enabled = context_enabled(ce) || context_blocked(ce) || context_pending_enable(ce);
>   
>   	if (!enabled) {
>   		action[len++] = INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_SET;