Re: [PATCH 1/3] drm/i915: Refactor confusing __intel_gt_reset()

[Nirmoy Das <nirmoy.das@xxxxxxxxx>]

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@xxxxxxxxx>
> > ---
> >   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);
> >       }
> >   }