[PATCH 2/2] drm/i915/guc: Fix missing return code checks in submission init
Ceraolo Spurio, Daniele
daniele.ceraolospurio at intel.com
Wed Jan 25 01:01:59 UTC 2023
On 1/11/2023 5:54 PM, John.C.Harrison at Intel.com wrote:
> From: John Harrison <John.C.Harrison at Intel.com>
>
> The CI results for the 'fast request' patch set (enables error return
> codes for fire-and-forget H2G messages) hit an issue with the KMD
> sending context submission requests on an invalid context. That was
> caused by a fault injection probe failing the context creation of a
> kernel context. However, there was no return code checking on any of
> the kernel context registration paths. So the driver kept going and
> tried to use the kernel context for the record defaults process.
>
> This would not cause any actual problems. The invalid requests would
> be rejected by GuC and ultimately the start up sequence would
> correctly wedge due to the context creation failure. But fixing the
> issue correctly rather ignoring it means we won't get CI complaining
> when the fast request patch lands and enables the extra error checking.
>
> So fix it by checking for errors and aborting as appropriate when
> creating kernel contexts. While at it, clean up some other submission
> init related failure cleanup paths. Also, rename guc_init_lrc_mapping
> to guc_init_submission as the former name hasn't been valid in a long
> time.
>
> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> ---
> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 91 ++++++++++++++-----
> .../gpu/drm/i915/gt/uc/intel_guc_submission.h | 2 +-
> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 7 +-
> 3 files changed, 75 insertions(+), 25 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 982364777d0c6..dd856fd92945b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1431,7 +1431,7 @@ static int guc_action_enable_usage_stats(struct intel_guc *guc)
> return intel_guc_send(guc, action, ARRAY_SIZE(action));
> }
>
> -static void guc_init_engine_stats(struct intel_guc *guc)
> +static int guc_init_engine_stats(struct intel_guc *guc)
> {
> struct intel_gt *gt = guc_to_gt(guc);
> intel_wakeref_t wakeref;
> @@ -1447,6 +1447,8 @@ static void guc_init_engine_stats(struct intel_guc *guc)
> cancel_delayed_work_sync(&guc->timestamp.work);
> drm_err(>->i915->drm, "Failed to enable usage stats: %d!\n", ret);
> }
> +
> + return ret;
> }
>
> static void guc_park_engine_stats(struct intel_guc *guc)
> @@ -4108,9 +4110,11 @@ static void guc_set_default_submission(struct intel_engine_cs *engine)
> engine->submit_request = guc_submit_request;
> }
>
> -static inline void guc_kernel_context_pin(struct intel_guc *guc,
> - struct intel_context *ce)
> +static inline int guc_kernel_context_pin(struct intel_guc *guc,
> + struct intel_context *ce)
> {
> + int ret;
> +
> /*
> * Note: we purposefully do not check the returns below because
> * the registration can only fail if a reset is just starting.
> @@ -4118,16 +4122,24 @@ static inline void guc_kernel_context_pin(struct intel_guc *guc,
> * isn't happening and even it did this code would be run again.
> */
>
> - if (context_guc_id_invalid(ce))
> - pin_guc_id(guc, ce);
> + if (context_guc_id_invalid(ce)) {
> + int ret = pin_guc_id(guc, ce);
Why do you need a local ret variable inside this if statement, when you
already have a function-level one? or is it just a cut & paste error?
> +
> + if (ret < 0)
> + return ret;
> + }
>
> if (!test_bit(CONTEXT_GUC_INIT, &ce->flags))
> guc_context_init(ce);
>
> - try_context_registration(ce, true);
> + ret = try_context_registration(ce, true);
> + if (ret)
> + unpin_guc_id(guc, ce);
> +
> + return ret;
> }
>
> -static inline void guc_init_lrc_mapping(struct intel_guc *guc)
> +static inline int guc_init_submission(struct intel_guc *guc)
> {
> struct intel_gt *gt = guc_to_gt(guc);
> struct intel_engine_cs *engine;
> @@ -4154,9 +4166,17 @@ static inline void guc_init_lrc_mapping(struct intel_guc *guc)
> struct intel_context *ce;
>
> list_for_each_entry(ce, &engine->pinned_contexts_list,
> - pinned_contexts_link)
> - guc_kernel_context_pin(guc, ce);
> + pinned_contexts_link) {
> + int ret = guc_kernel_context_pin(guc, ce);
> +
> + if (ret) {
> + /* No point in trying to clean up as i915 will wedge on failure */
> + return ret;
> + }
> + }
> }
> +
> + return 0;
> }
>
> static void guc_release(struct intel_engine_cs *engine)
> @@ -4400,30 +4420,57 @@ static int guc_init_global_schedule_policy(struct intel_guc *guc)
> return ret;
> }
>
> -void intel_guc_submission_enable(struct intel_guc *guc)
> +static void guc_route_semaphores(struct intel_guc *guc, bool to_guc)
> {
> struct intel_gt *gt = guc_to_gt(guc);
> + u32 val;
>
> - /* Enable and route to GuC */
> - if (GRAPHICS_VER(gt->i915) >= 12)
> - intel_uncore_write(gt->uncore, GEN12_GUC_SEM_INTR_ENABLES,
> - GUC_SEM_INTR_ROUTE_TO_GUC |
> - GUC_SEM_INTR_ENABLE_ALL);
> + if (GRAPHICS_VER(gt->i915) < 12)
> + return;
>
> - guc_init_lrc_mapping(guc);
> - guc_init_engine_stats(guc);
> - guc_init_global_schedule_policy(guc);
> + if (to_guc)
> + val = GUC_SEM_INTR_ROUTE_TO_GUC | GUC_SEM_INTR_ENABLE_ALL;
> + else
> + val = 0;
> +
> + intel_uncore_write(gt->uncore, GEN12_GUC_SEM_INTR_ENABLES, val);
> +}
> +
> +int intel_guc_submission_enable(struct intel_guc *guc)
> +{
> + int ret;
> +
> + /* Semaphore interrupt enable and route to GuC */
> + guc_route_semaphores(guc, true);
> +
> + ret = guc_init_submission(guc);
> + if (ret)
> + goto fail_sem;
> +
> + ret = guc_init_engine_stats(guc);
> + if (ret)
> + goto fail_sem;
> +
> + ret = guc_init_global_schedule_policy(guc);
> + if (ret)
> + goto fail_stats;
> +
> + return 0;
> +
> +fail_stats:
> + guc_park_engine_stats(guc);
personal preference: I'd prefer an extra guc_fini_engine_stats wrapper
just so that we're balanced with the naming, but not a blocker.
Daniele
> +fail_sem:
> + guc_route_semaphores(guc, false);
> + return ret;
> }
>
> /* Note: By the time we're here, GuC may have already been reset */
> void intel_guc_submission_disable(struct intel_guc *guc)
> {
> - struct intel_gt *gt = guc_to_gt(guc);
> guc_park_engine_stats(guc);
>
> - /* Disable and route to host */
> - if (GRAPHICS_VER(gt->i915) >= 12)
> - intel_uncore_write(gt->uncore, GEN12_GUC_SEM_INTR_ENABLES, 0x0);
> + /* Semaphore interrupt disable and route to host */
> + guc_route_semaphores(guc, false);
> }
>
> static bool __guc_submission_supported(struct intel_guc *guc)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> index 5a95a9f0a8e31..c57b29cdb1a64 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h
> @@ -15,7 +15,7 @@ struct intel_engine_cs;
>
> void intel_guc_submission_init_early(struct intel_guc *guc);
> int intel_guc_submission_init(struct intel_guc *guc);
> -void intel_guc_submission_enable(struct intel_guc *guc);
> +int intel_guc_submission_enable(struct intel_guc *guc);
> void intel_guc_submission_disable(struct intel_guc *guc);
> void intel_guc_submission_fini(struct intel_guc *guc);
> int intel_guc_preempt_work_create(struct intel_guc *guc);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 9a8a1abf71d7f..8e338b3321a93 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -537,8 +537,11 @@ static int __uc_init_hw(struct intel_uc *uc)
> else
> intel_huc_auth(huc);
>
> - if (intel_uc_uses_guc_submission(uc))
> - intel_guc_submission_enable(guc);
> + if (intel_uc_uses_guc_submission(uc)) {
> + ret = intel_guc_submission_enable(guc);
> + if (ret)
> + goto err_log_capture;
> + }
>
> if (intel_uc_uses_guc_slpc(uc)) {
> ret = intel_guc_slpc_enable(&guc->slpc);
More information about the dri-devel
mailing list