| Message ID | 20210819061639.21051-13-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up GuC CI failures, simplify locking, and kernel DOC | expand |
On 8/18/2021 11:16 PM, Matthew Brost wrote: > While debugging an issue with full GT resets I went down a rabbit hole > thinking the scrubbing of lost G2H wasn't working correctly. This proved > to be incorrect as this was working just fine but this chase inspired me > to write a selftest to prove that this works. This simple selftest > injects errors dropping various G2H and then issues a full GT reset > proving that the scrubbing of these G2H doesn't blow up. > > v2: > (Daniel Vetter) > - Use ifdef instead of macros for selftests > v3: > (Checkpatch) > - A space after 'switch' statement > > Signed-off-by: Matthew Brost <matthew.brost@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_context_types.h | 18 +++ > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 25 ++++ > drivers/gpu/drm/i915/gt/uc/selftest_guc.c | 126 ++++++++++++++++++ > .../drm/i915/selftests/i915_live_selftests.h | 1 + > .../i915/selftests/intel_scheduler_helpers.c | 12 ++ > .../i915/selftests/intel_scheduler_helpers.h | 2 + > 6 files changed, 184 insertions(+) > create mode 100644 drivers/gpu/drm/i915/gt/uc/selftest_guc.c > > diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h > index e54351a170e2..3a73f3117873 100644 > --- a/drivers/gpu/drm/i915/gt/intel_context_types.h > +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h > @@ -198,6 +198,24 @@ struct intel_context { > */ > u8 guc_prio; > u32 guc_prio_count[GUC_CLIENT_PRIORITY_NUM]; > + > +#ifdef CONFIG_DRM_I915_SELFTEST > + /** > + * @drop_schedule_enable: Force drop of schedule enable G2H for selftest > + */ > + bool drop_schedule_enable; > + > + /** > + * @drop_schedule_disable: Force drop of schedule disable G2H for > + * selftest > + */ > + bool drop_schedule_disable; > + > + /** > + * @drop_deregister: Force drop of deregister G2H for selftest > + */ > + bool drop_deregister; > +#endif > }; > > #endif /* __INTEL_CONTEXT_TYPES__ */ > 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 e53a4ef7d442..e0e85e4ad512 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c > @@ -2635,6 +2635,13 @@ int intel_guc_deregister_done_process_msg(struct intel_guc *guc, > > trace_intel_context_deregister_done(ce); > > +#ifdef CONFIG_DRM_I915_SELFTEST > + if (unlikely(ce->drop_deregister)) { > + ce->drop_deregister = false; > + return 0; > + } > +#endif > + > if (context_wait_for_deregister_to_register(ce)) { > struct intel_runtime_pm *runtime_pm = > &ce->engine->gt->i915->runtime_pm; > @@ -2689,10 +2696,24 @@ int intel_guc_sched_done_process_msg(struct intel_guc *guc, > trace_intel_context_sched_done(ce); > > if (context_pending_enable(ce)) { > +#ifdef CONFIG_DRM_I915_SELFTEST > + if (unlikely(ce->drop_schedule_enable)) { > + ce->drop_schedule_enable = false; > + return 0; > + } > +#endif > + > clr_context_pending_enable(ce); > } else if (context_pending_disable(ce)) { > bool banned; > > +#ifdef CONFIG_DRM_I915_SELFTEST > + if (unlikely(ce->drop_schedule_disable)) { > + ce->drop_schedule_disable = false; > + return 0; > + } > +#endif > + > /* > * Unpin must be done before __guc_signal_context_fence, > * otherwise a race exists between the requests getting > @@ -3069,3 +3090,7 @@ bool intel_guc_virtual_engine_has_heartbeat(const struct intel_engine_cs *ve) > > return false; > } > + > +#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > +#include "selftest_guc.c" > +#endif > diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c > new file mode 100644 > index 000000000000..264e2f705c17 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c > @@ -0,0 +1,126 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright �� 2021 Intel Corporation > + */ > + > +#include "selftests/intel_scheduler_helpers.h" > + > +static struct i915_request *nop_user_request(struct intel_context *ce, > + struct i915_request *from) > +{ > + struct i915_request *rq; > + int ret; > + > + rq = intel_context_create_request(ce); > + if (IS_ERR(rq)) > + return rq; > + > + if (from) { > + ret = i915_sw_fence_await_dma_fence(&rq->submit, > + &from->fence, 0, > + I915_FENCE_GFP); > + if (ret < 0) { > + i915_request_put(rq); > + return ERR_PTR(ret); > + } > + } > + > + i915_request_get(rq); > + i915_request_add(rq); > + > + return rq; > +} > + > +static int intel_guc_scrub_ctbs(void *arg) > +{ > + struct intel_gt *gt = arg; > + int ret = 0; > + int i; > + struct i915_request *last[3] = {NULL, NULL, NULL}, *rq; > + intel_wakeref_t wakeref; > + struct intel_engine_cs *engine; > + struct intel_context *ce; > + > + wakeref = intel_runtime_pm_get(gt->uncore->rpm); > + engine = intel_selftest_find_any_engine(gt); > + > + /* Submit requests and inject errors forcing G2H to be dropped */ > + for (i = 0; i < 3; ++i) { > + ce = intel_context_create(engine); > + if (IS_ERR(ce)) { > + ret = PTR_ERR(ce); > + pr_err("Failed to create context, %d: %d\n", i, ret); > + goto err; > + } > + > + switch (i) { > + case 0: > + ce->drop_schedule_enable = true; > + break; > + case 1: > + ce->drop_schedule_disable = true; > + break; > + case 2: > + ce->drop_deregister = true; > + break; > + } Would it be worth making the drop bitmask? #ifdef CONFIG_DRM_I915_SELFTEST /** * @drop_g2h: Force drop of selected G2H for selftest */ u32 drop_g2h; #define I915_SELFTEST_DROP_GUC_SCHED_ENABLE BIT(0) .... #endif So in the test instead of a switch you can use: ce->drop_g2h = BIT(i); Not a blocker. > + > + rq = nop_user_request(ce, NULL); > + intel_context_put(ce); > + > + if (IS_ERR(rq)) { > + ret = PTR_ERR(rq); > + pr_err("Failed to create request, %d: %d\n", i, ret); > + goto err; > + } > + > + last[i] = rq; > + } > + > + for (i = 0; i < 3; ++i) { > + ret = i915_request_wait(last[i], 0, HZ); > + if (ret < 0) { > + pr_err("Last request failed to complete: %d\n", ret); > + goto err; > + } > + i915_request_put(last[i]); > + last[i] = NULL; > + } > + > + /* Force all H2G / G2H to be submitted / processed */ > + intel_gt_retire_requests(gt); > + msleep(500); > + > + /* Scrub missing G2H */ > + intel_gt_handle_error(engine->gt, -1, 0, "selftest reset"); > + > + ret = intel_gt_wait_for_idle(gt, HZ); I think here we could use a small comment where we explain that the GT won't go idle if the scrubbing was not done correctly. With that: Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Daniele > + if (ret < 0) { > + pr_err("GT failed to idle: %d\n", ret); > + goto err; > + } > + > +err: > + for (i = 0; i < 3; ++i) > + if (last[i]) > + i915_request_put(last[i]); > + intel_runtime_pm_put(gt->uncore->rpm, wakeref); > + > + return ret; > +} > + > +int intel_guc_live_selftests(struct drm_i915_private *i915) > +{ > + static const struct i915_subtest tests[] = { > + SUBTEST(intel_guc_scrub_ctbs), > + }; > + struct intel_gt *gt = &i915->gt; > + > + if (intel_gt_is_wedged(gt)) > + return 0; > + > + if (!intel_uc_uses_guc_submission(>->uc)) > + return 0; > + > + return intel_gt_live_subtests(tests, gt); > +} > diff --git a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h > index cfa5c4165a4f..3cf6758931f9 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h > +++ b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h > @@ -47,5 +47,6 @@ selftest(execlists, intel_execlists_live_selftests) > selftest(ring_submission, intel_ring_submission_live_selftests) > selftest(perf, i915_perf_live_selftests) > selftest(slpc, intel_slpc_live_selftests) > +selftest(guc, intel_guc_live_selftests) > /* Here be dragons: keep last to run last! */ > selftest(late_gt_pm, intel_gt_pm_late_selftests) > diff --git a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c > index 4b328346b48a..310fb83c527e 100644 > --- a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c > +++ b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c > @@ -14,6 +14,18 @@ > #define REDUCED_PREEMPT 10 > #define WAIT_FOR_RESET_TIME 10000 > > +struct intel_engine_cs *intel_selftest_find_any_engine(struct intel_gt *gt) > +{ > + struct intel_engine_cs *engine; > + enum intel_engine_id id; > + > + for_each_engine(engine, gt, id) > + return engine; > + > + pr_err("No valid engine found!\n"); > + return NULL; > +} > + > int intel_selftest_modify_policy(struct intel_engine_cs *engine, > struct intel_selftest_saved_policy *saved, > u32 modify_type) > diff --git a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h > index 35c098601ac0..ae60bb507f45 100644 > --- a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h > +++ b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h > @@ -10,6 +10,7 @@ > > struct i915_request; > struct intel_engine_cs; > +struct intel_gt; > > struct intel_selftest_saved_policy { > u32 flags; > @@ -23,6 +24,7 @@ enum selftest_scheduler_modify { > SELFTEST_SCHEDULER_MODIFY_FAST_RESET, > }; > > +struct intel_engine_cs *intel_selftest_find_any_engine(struct intel_gt *gt); > int intel_selftest_modify_policy(struct intel_engine_cs *engine, > struct intel_selftest_saved_policy *saved, > enum selftest_scheduler_modify modify_type);
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h index e54351a170e2..3a73f3117873 100644 --- a/drivers/gpu/drm/i915/gt/intel_context_types.h +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h @@ -198,6 +198,24 @@ struct intel_context { */ u8 guc_prio; u32 guc_prio_count[GUC_CLIENT_PRIORITY_NUM]; + +#ifdef CONFIG_DRM_I915_SELFTEST + /** + * @drop_schedule_enable: Force drop of schedule enable G2H for selftest + */ + bool drop_schedule_enable; + + /** + * @drop_schedule_disable: Force drop of schedule disable G2H for + * selftest + */ + bool drop_schedule_disable; + + /** + * @drop_deregister: Force drop of deregister G2H for selftest + */ + bool drop_deregister; +#endif }; #endif /* __INTEL_CONTEXT_TYPES__ */ 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 e53a4ef7d442..e0e85e4ad512 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2635,6 +2635,13 @@ int intel_guc_deregister_done_process_msg(struct intel_guc *guc, trace_intel_context_deregister_done(ce); +#ifdef CONFIG_DRM_I915_SELFTEST + if (unlikely(ce->drop_deregister)) { + ce->drop_deregister = false; + return 0; + } +#endif + if (context_wait_for_deregister_to_register(ce)) { struct intel_runtime_pm *runtime_pm = &ce->engine->gt->i915->runtime_pm; @@ -2689,10 +2696,24 @@ int intel_guc_sched_done_process_msg(struct intel_guc *guc, trace_intel_context_sched_done(ce); if (context_pending_enable(ce)) { +#ifdef CONFIG_DRM_I915_SELFTEST + if (unlikely(ce->drop_schedule_enable)) { + ce->drop_schedule_enable = false; + return 0; + } +#endif + clr_context_pending_enable(ce); } else if (context_pending_disable(ce)) { bool banned; +#ifdef CONFIG_DRM_I915_SELFTEST + if (unlikely(ce->drop_schedule_disable)) { + ce->drop_schedule_disable = false; + return 0; + } +#endif + /* * Unpin must be done before __guc_signal_context_fence, * otherwise a race exists between the requests getting @@ -3069,3 +3090,7 @@ bool intel_guc_virtual_engine_has_heartbeat(const struct intel_engine_cs *ve) return false; } + +#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) +#include "selftest_guc.c" +#endif diff --git a/drivers/gpu/drm/i915/gt/uc/selftest_guc.c b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c new file mode 100644 index 000000000000..264e2f705c17 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/uc/selftest_guc.c @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright �� 2021 Intel Corporation + */ + +#include "selftests/intel_scheduler_helpers.h" + +static struct i915_request *nop_user_request(struct intel_context *ce, + struct i915_request *from) +{ + struct i915_request *rq; + int ret; + + rq = intel_context_create_request(ce); + if (IS_ERR(rq)) + return rq; + + if (from) { + ret = i915_sw_fence_await_dma_fence(&rq->submit, + &from->fence, 0, + I915_FENCE_GFP); + if (ret < 0) { + i915_request_put(rq); + return ERR_PTR(ret); + } + } + + i915_request_get(rq); + i915_request_add(rq); + + return rq; +} + +static int intel_guc_scrub_ctbs(void *arg) +{ + struct intel_gt *gt = arg; + int ret = 0; + int i; + struct i915_request *last[3] = {NULL, NULL, NULL}, *rq; + intel_wakeref_t wakeref; + struct intel_engine_cs *engine; + struct intel_context *ce; + + wakeref = intel_runtime_pm_get(gt->uncore->rpm); + engine = intel_selftest_find_any_engine(gt); + + /* Submit requests and inject errors forcing G2H to be dropped */ + for (i = 0; i < 3; ++i) { + ce = intel_context_create(engine); + if (IS_ERR(ce)) { + ret = PTR_ERR(ce); + pr_err("Failed to create context, %d: %d\n", i, ret); + goto err; + } + + switch (i) { + case 0: + ce->drop_schedule_enable = true; + break; + case 1: + ce->drop_schedule_disable = true; + break; + case 2: + ce->drop_deregister = true; + break; + } + + rq = nop_user_request(ce, NULL); + intel_context_put(ce); + + if (IS_ERR(rq)) { + ret = PTR_ERR(rq); + pr_err("Failed to create request, %d: %d\n", i, ret); + goto err; + } + + last[i] = rq; + } + + for (i = 0; i < 3; ++i) { + ret = i915_request_wait(last[i], 0, HZ); + if (ret < 0) { + pr_err("Last request failed to complete: %d\n", ret); + goto err; + } + i915_request_put(last[i]); + last[i] = NULL; + } + + /* Force all H2G / G2H to be submitted / processed */ + intel_gt_retire_requests(gt); + msleep(500); + + /* Scrub missing G2H */ + intel_gt_handle_error(engine->gt, -1, 0, "selftest reset"); + + ret = intel_gt_wait_for_idle(gt, HZ); + if (ret < 0) { + pr_err("GT failed to idle: %d\n", ret); + goto err; + } + +err: + for (i = 0; i < 3; ++i) + if (last[i]) + i915_request_put(last[i]); + intel_runtime_pm_put(gt->uncore->rpm, wakeref); + + return ret; +} + +int intel_guc_live_selftests(struct drm_i915_private *i915) +{ + static const struct i915_subtest tests[] = { + SUBTEST(intel_guc_scrub_ctbs), + }; + struct intel_gt *gt = &i915->gt; + + if (intel_gt_is_wedged(gt)) + return 0; + + if (!intel_uc_uses_guc_submission(>->uc)) + return 0; + + return intel_gt_live_subtests(tests, gt); +} diff --git a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h index cfa5c4165a4f..3cf6758931f9 100644 --- a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h +++ b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h @@ -47,5 +47,6 @@ selftest(execlists, intel_execlists_live_selftests) selftest(ring_submission, intel_ring_submission_live_selftests) selftest(perf, i915_perf_live_selftests) selftest(slpc, intel_slpc_live_selftests) +selftest(guc, intel_guc_live_selftests) /* Here be dragons: keep last to run last! */ selftest(late_gt_pm, intel_gt_pm_late_selftests) diff --git a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c index 4b328346b48a..310fb83c527e 100644 --- a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c +++ b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.c @@ -14,6 +14,18 @@ #define REDUCED_PREEMPT 10 #define WAIT_FOR_RESET_TIME 10000 +struct intel_engine_cs *intel_selftest_find_any_engine(struct intel_gt *gt) +{ + struct intel_engine_cs *engine; + enum intel_engine_id id; + + for_each_engine(engine, gt, id) + return engine; + + pr_err("No valid engine found!\n"); + return NULL; +} + int intel_selftest_modify_policy(struct intel_engine_cs *engine, struct intel_selftest_saved_policy *saved, u32 modify_type) diff --git a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h index 35c098601ac0..ae60bb507f45 100644 --- a/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h +++ b/drivers/gpu/drm/i915/selftests/intel_scheduler_helpers.h @@ -10,6 +10,7 @@ struct i915_request; struct intel_engine_cs; +struct intel_gt; struct intel_selftest_saved_policy { u32 flags; @@ -23,6 +24,7 @@ enum selftest_scheduler_modify { SELFTEST_SCHEDULER_MODIFY_FAST_RESET, }; +struct intel_engine_cs *intel_selftest_find_any_engine(struct intel_gt *gt); int intel_selftest_modify_policy(struct intel_engine_cs *engine, struct intel_selftest_saved_policy *saved, enum selftest_scheduler_modify modify_type);
While debugging an issue with full GT resets I went down a rabbit hole thinking the scrubbing of lost G2H wasn't working correctly. This proved to be incorrect as this was working just fine but this chase inspired me to write a selftest to prove that this works. This simple selftest injects errors dropping various G2H and then issues a full GT reset proving that the scrubbing of these G2H doesn't blow up. v2: (Daniel Vetter) - Use ifdef instead of macros for selftests v3: (Checkpatch) - A space after 'switch' statement Signed-off-by: Matthew Brost <matthew.brost@intel.com> --- drivers/gpu/drm/i915/gt/intel_context_types.h | 18 +++ .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 25 ++++ drivers/gpu/drm/i915/gt/uc/selftest_guc.c | 126 ++++++++++++++++++ .../drm/i915/selftests/i915_live_selftests.h | 1 + .../i915/selftests/intel_scheduler_helpers.c | 12 ++ .../i915/selftests/intel_scheduler_helpers.h | 2 + 6 files changed, 184 insertions(+) create mode 100644 drivers/gpu/drm/i915/gt/uc/selftest_guc.c