[PATCH 2/2] drm/i915/guc: Fix missing return code checks in submission init
John Harrison
john.c.harrison at intel.com
Fri Feb 17 20:40:52 UTC 2023
On 1/24/2023 17:01, Ceraolo Spurio, Daniele wrote:
> 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?
Yeah, copy/paste thing.
>
>> +
>> + 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.
As per comment on previous patch, I'm thinking I'll rename
guc_park_engine_stats() to guc_cancel_busyness_worker() (and add a
matching enable). And I agree on the naming here. But adding yet another
wrapper would mean having this:
guc_fini_engine_stats() {
guc_cancel_busyness_worker() {
cancel_delayed_work_sync();
}
}
Which seems excessive. A wrapper around a wrapper around a one line
piece of code. I guess the compiler will optimise it all out and it
leaves room for future expansion if other things need to happen in the
middle layers in the future.
John.
>
> 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