[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