[PATCH 1/3] drm/i915: Refactor confusing __intel_gt_reset()
Nirmoy Das
nirmoy.das at intel.com
Fri Apr 19 08:44:58 UTC 2024
Hi John.
On 4/19/2024 1:27 AM, John Harrison wrote:
> On 4/18/2024 10:10, Nirmoy Das wrote:
>> __intel_gt_reset() is really for resetting engines though
>> the name might suggest something else. So add two helper functions
>> to remove confusions with no functional changes.
> Technically you only added one and just moved the other :). It already
> existed, it just wasn't being used everywhere that it could be!
I did have one more helper functions but I removed it in favor of
intel_gt_reset_engine() but didn't change the commit message :/
Thanks for catching it. I will fix it.
>
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das at intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +-
>> .../drm/i915/gt/intel_execlists_submission.c | 2 +-
>> drivers/gpu/drm/i915/gt/intel_gt.c | 2 +-
>> drivers/gpu/drm/i915/gt/intel_gt_pm.c | 2 +-
>> drivers/gpu/drm/i915/gt/intel_reset.c | 43 ++++++++++++++-----
>> drivers/gpu/drm/i915/gt/intel_reset.h | 3 +-
>> drivers/gpu/drm/i915/gt/selftest_reset.c | 2 +-
>> drivers/gpu/drm/i915/i915_driver.c | 2 +-
>> 8 files changed, 41 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> index 8c44af1c3451..5c8e9ee3b008 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> @@ -678,7 +678,7 @@ void intel_engines_release(struct intel_gt *gt)
>> */
>> GEM_BUG_ON(intel_gt_pm_is_awake(gt));
>> if (!INTEL_INFO(gt->i915)->gpu_reset_clobbers_display)
>> - __intel_gt_reset(gt, ALL_ENGINES);
>> + intel_gt_reset_all_engines(gt);
>> /* Decouple the backend; but keep the layout for late GPU
>> resets */
>> for_each_engine(engine, gt, id) {
>> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> index 355aab5b38ba..21829439e686 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
>> @@ -2898,7 +2898,7 @@ static void enable_error_interrupt(struct
>> intel_engine_cs *engine)
>> drm_err(&engine->i915->drm,
>> "engine '%s' resumed still in error: %08x\n",
>> engine->name, status);
>> - __intel_gt_reset(engine->gt, engine->mask);
>> + intel_gt_reset_engine(engine);
>> }
>> /*
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
>> b/drivers/gpu/drm/i915/gt/intel_gt.c
>> index 580b5141ce1e..626b166e67ef 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>> @@ -832,7 +832,7 @@ void intel_gt_driver_unregister(struct intel_gt *gt)
>> /* Scrub all HW state upon release */
>> with_intel_runtime_pm(gt->uncore->rpm, wakeref)
>> - __intel_gt_reset(gt, ALL_ENGINES);
>> + intel_gt_reset_all_engines(gt);
>> }
>> void intel_gt_driver_release(struct intel_gt *gt)
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>> b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>> index 220ac4f92edf..c08fdb65cc69 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
>> @@ -159,7 +159,7 @@ static bool reset_engines(struct intel_gt *gt)
>> if (INTEL_INFO(gt->i915)->gpu_reset_clobbers_display)
>> return false;
>> - return __intel_gt_reset(gt, ALL_ENGINES) == 0;
>> + return intel_gt_reset_all_engines(gt) == 0;
>> }
>> static void gt_sanitize(struct intel_gt *gt, bool force)
>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c
>> b/drivers/gpu/drm/i915/gt/intel_reset.c
>> index c8e9aa41fdea..b825daace58e 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>> @@ -764,7 +764,7 @@ wa_14015076503_end(struct intel_gt *gt,
>> intel_engine_mask_t engine_mask)
>> HECI_H_GS1_ER_PREP, 0);
>> }
>> -int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t
>> engine_mask)
>> +static int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t
>> engine_mask)
>> {
>> const int retries = engine_mask == ALL_ENGINES ?
>> RESET_MAX_RETRIES : 1;
>> reset_func reset;
>> @@ -795,6 +795,34 @@ int __intel_gt_reset(struct intel_gt *gt,
>> intel_engine_mask_t engine_mask)
>> return ret;
>> }
>> +/**
>> + * intel_gt_reset_all_engines() - Reset all engines in the given gt.
>> + * @gt: the GT to reset all engines for.
>> + *
>> + * This function resets all engines within the given gt.
>> + *
>> + * Returns:
>> + * Zero on success, negative error code on failure.
>> + */
>> +int intel_gt_reset_all_engines(struct intel_gt *gt)
>> +{
>> + return __intel_gt_reset(gt, ALL_ENGINES);
>> +}
>> +
>> +/**
>> + * intel_gt_reset_engine() - Reset a specific engine within a gt.
>> + * @engine: engine to be reset.
>> + *
>> + * This function resets the specified engine within a gt.
>> + *
>> + * Returns:
>> + * Zero on success, negative error code on failure.
>> + */
>> +int intel_gt_reset_engine(struct intel_engine_cs *engine)
>> +{
>> + return __intel_gt_reset(engine->gt, engine->mask);
>> +}
>> +
> You could have just dropped the 'static' from the existing copy of
> this function and added the new version next to it. That would make
> the diff simpler and therefore clearer. Unless you think there is a
> good reason to move the code up here?
Yes, make sense, I will do that.
Thanks,
Nirmoy
>
> John.
>
>> bool intel_has_gpu_reset(const struct intel_gt *gt)
>> {
>> if (!gt->i915->params.reset)
>> @@ -978,7 +1006,7 @@ static void __intel_gt_set_wedged(struct
>> intel_gt *gt)
>> /* Even if the GPU reset fails, it should still stop the
>> engines */
>> if (!INTEL_INFO(gt->i915)->gpu_reset_clobbers_display)
>> - __intel_gt_reset(gt, ALL_ENGINES);
>> + intel_gt_reset_all_engines(gt);
>> for_each_engine(engine, gt, id)
>> engine->submit_request = nop_submit_request;
>> @@ -1089,7 +1117,7 @@ static bool __intel_gt_unset_wedged(struct
>> intel_gt *gt)
>> /* We must reset pending GPU events before restoring our
>> submission */
>> ok = !HAS_EXECLISTS(gt->i915); /* XXX better agnosticism
>> desired */
>> if (!INTEL_INFO(gt->i915)->gpu_reset_clobbers_display)
>> - ok = __intel_gt_reset(gt, ALL_ENGINES) == 0;
>> + ok = intel_gt_reset_all_engines(gt) == 0;
>> if (!ok) {
>> /*
>> * Warn CI about the unrecoverable wedged condition.
>> @@ -1133,10 +1161,10 @@ static int do_reset(struct intel_gt *gt,
>> intel_engine_mask_t stalled_mask)
>> {
>> int err, i;
>> - err = __intel_gt_reset(gt, ALL_ENGINES);
>> + err = intel_gt_reset_all_engines(gt);
>> for (i = 0; err && i < RESET_MAX_RETRIES; i++) {
>> msleep(10 * (i + 1));
>> - err = __intel_gt_reset(gt, ALL_ENGINES);
>> + err = intel_gt_reset_all_engines(gt);
>> }
>> if (err)
>> return err;
>> @@ -1270,11 +1298,6 @@ void intel_gt_reset(struct intel_gt *gt,
>> goto finish;
>> }
>> -static int intel_gt_reset_engine(struct intel_engine_cs *engine)
>> -{
>> - return __intel_gt_reset(engine->gt, engine->mask);
>> -}
>> -
>> int __intel_engine_reset_bh(struct intel_engine_cs *engine, const
>> char *msg)
>> {
>> struct intel_gt *gt = engine->gt;
>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.h
>> b/drivers/gpu/drm/i915/gt/intel_reset.h
>> index f615b30b81c5..c00de353075c 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_reset.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.h
>> @@ -54,7 +54,8 @@ int intel_gt_terminally_wedged(struct intel_gt *gt);
>> void intel_gt_set_wedged_on_init(struct intel_gt *gt);
>> void intel_gt_set_wedged_on_fini(struct intel_gt *gt);
>> -int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t
>> engine_mask);
>> +int intel_gt_reset_engine(struct intel_engine_cs *engine);
>> +int intel_gt_reset_all_engines(struct intel_gt *gt);
>> int intel_reset_guc(struct intel_gt *gt);
>> diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c
>> b/drivers/gpu/drm/i915/gt/selftest_reset.c
>> index f40de408cd3a..2cfc23c58e90 100644
>> --- a/drivers/gpu/drm/i915/gt/selftest_reset.c
>> +++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
>> @@ -281,7 +281,7 @@ static int igt_atomic_reset(void *arg)
>> awake = reset_prepare(gt);
>> p->critical_section_begin();
>> - err = __intel_gt_reset(gt, ALL_ENGINES);
>> + err = intel_gt_reset_all_engines(gt);
>> p->critical_section_end();
>> reset_finish(gt, awake);
>> diff --git a/drivers/gpu/drm/i915/i915_driver.c
>> b/drivers/gpu/drm/i915/i915_driver.c
>> index 4b9233c07a22..622a24305bc2 100644
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -202,7 +202,7 @@ static void sanitize_gpu(struct drm_i915_private
>> *i915)
>> unsigned int i;
>> for_each_gt(gt, i915, i)
>> - __intel_gt_reset(gt, ALL_ENGINES);
>> + intel_gt_reset_all_engines(gt);
>> }
>> }
>
More information about the Intel-gfx
mailing list