| Message ID | 20240415164441.5684-1-nirmoy.das@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] drm/i915: Don't reset GuC before engine reset on full GT reset | expand |
On 4/15/2024 09:44, Nirmoy Das wrote: > Currently intel_gt_reset() happens as follows: > > reset_prepare() ---> Sends GDRST to GuC, GuC is in GS_MIA_IN_RESET > do_reset() > __intel_gt_reset() > *_engine_reset_prepare() -->RESET_CTL expects running > GuC > *_reset_engines() > intel_gt_init_hw() --> GuC FW loading happens, GuC comes out of > GS_MIA_IN_RESET. > > Fix the above flow so that GuC reset happens after all the > engines reset is done. > > Cc: John Harrison <John.C.Harrison@Intel.com> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_reset.c | 9 ++++-- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 42 +++++++++++++++++++++------ > drivers/gpu/drm/i915/gt/uc/intel_uc.h | 1 + > 3 files changed, 41 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c > index c8e9aa41fdea..9ebd68ce0c22 100644 > --- a/drivers/gpu/drm/i915/gt/intel_reset.c > +++ b/drivers/gpu/drm/i915/gt/intel_reset.c > @@ -879,8 +879,11 @@ static intel_engine_mask_t reset_prepare(struct intel_gt *gt) > intel_engine_mask_t awake = 0; > enum intel_engine_id id; > > - /* For GuC mode, ensure submission is disabled before stopping ring */ > - intel_uc_reset_prepare(>->uc); > + /* > + * For GuC mode, ensure submission is disabled before stopping ring. > + * Don't reset the GuC a engine reset requires GuC to be running. These two lines appear to be mutually exclusive unless there is a test for GuC submission being enabled, which I am not seeing. Note that "ensure submission is disabled" means "reset the GuC". > + */ > + intel_uc_reset_prepare_without_guc_reset(>->uc); > > for_each_engine(engine, gt, id) { > if (intel_engine_pm_get_if_awake(engine)) > @@ -1227,6 +1230,8 @@ void intel_gt_reset(struct intel_gt *gt, > > intel_overlay_reset(gt->i915); > > + /* Now that all engines are clean, Reset the GuC */ > + intel_uc_reset_prepare(>->uc); > /* > * Next we need to restore the context, but we don't use those > * yet either... > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index 7a63abf8f644..5feee4db2ccc 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -345,7 +345,7 @@ static void __uc_fini(struct intel_uc *uc) > intel_guc_fini(&uc->guc); > } > > -static int __uc_sanitize(struct intel_uc *uc) > +static void __uc_sanitize_without_guc_reset(struct intel_uc *uc) > { > struct intel_guc *guc = &uc->guc; > struct intel_huc *huc = &uc->huc; > @@ -354,7 +354,11 @@ static int __uc_sanitize(struct intel_uc *uc) > > intel_huc_sanitize(huc); > intel_guc_sanitize(guc); > +} This seems like an extremely bad idea. You are wiping out all the GuC communication structures on the host side while the GuC itself is still executing and using those same structures. Is the failure when doing individual engine resets or when doing a full GT reset? If the former, I think a better approach would be to just not reset GuC at all (or indeed any UC) if not using GuC submission. Although, looking at the code, I'm not seeing an engine only reset path that does nuke the UC layers? If it is the latter, then how/why are individual engine resets happening in the middle of a full GT reset? Don't we just splat everything all at once? Either way, it would be safer to split at the GT reset code layer rather than inside the UC layer. That is, when not using GuC submission, do the entire prepare/reset/init sequence of the UC layers as one 'atomic' operation either before the GT/engine reset or after it (or potentially both before and after?). John. > > +static int __uc_sanitize(struct intel_uc *uc) > +{ > + __uc_sanitize_without_guc_reset(uc); > return __intel_uc_reset_hw(uc); > } > > @@ -593,13 +597,7 @@ static void __uc_fini_hw(struct intel_uc *uc) > __uc_sanitize(uc); > } > > -/** > - * intel_uc_reset_prepare - Prepare for reset > - * @uc: the intel_uc structure > - * > - * Preparing for full gpu reset. > - */ > -void intel_uc_reset_prepare(struct intel_uc *uc) > +static void __intel_uc_reset_prepare(struct intel_uc *uc, bool reset_guc) > { > struct intel_guc *guc = &uc->guc; > > @@ -617,9 +615,35 @@ void intel_uc_reset_prepare(struct intel_uc *uc) > intel_guc_submission_reset_prepare(guc); > > sanitize: > - __uc_sanitize(uc); > + if (reset_guc) > + __uc_sanitize(uc); > + else > + __uc_sanitize_without_guc_reset(uc); > } > > +/** > + * intel_uc_reset_prepare - Prepare for reset > + * @uc: the intel_uc structure > + * > + * Preparing for full gpu reset. > + */ > +void intel_uc_reset_prepare(struct intel_uc *uc) > +{ > + __intel_uc_reset_prepare(uc, true); > +} > +/** > + * intel_uc_reset_prepare_without_guc_reset - Prepare for reset but don't reset > + * the GuC > + * @uc: the intel_uc structure > + * > + * Preparing for full gpu reset. > + */ > +void intel_uc_reset_prepare_without_guc_reset(struct intel_uc *uc) > +{ > + __intel_uc_reset_prepare(uc, false); > +} > + > + > void intel_uc_reset(struct intel_uc *uc, intel_engine_mask_t stalled) > { > struct intel_guc *guc = &uc->guc; > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h > index 014bb7d83689..9d6191ece498 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h > @@ -46,6 +46,7 @@ void intel_uc_driver_late_release(struct intel_uc *uc); > void intel_uc_driver_remove(struct intel_uc *uc); > void intel_uc_init_mmio(struct intel_uc *uc); > void intel_uc_reset_prepare(struct intel_uc *uc); > +void intel_uc_reset_prepare_without_guc_reset(struct intel_uc *uc); > void intel_uc_reset(struct intel_uc *uc, intel_engine_mask_t stalled); > void intel_uc_reset_finish(struct intel_uc *uc); > void intel_uc_cancel_requests(struct intel_uc *uc);
Hi John, On 4/17/2024 2:37 AM, John Harrison wrote: > On 4/15/2024 09:44, Nirmoy Das wrote: >> Currently intel_gt_reset() happens as follows: >> >> reset_prepare() ---> Sends GDRST to GuC, GuC is in GS_MIA_IN_RESET >> do_reset() >> __intel_gt_reset() >> *_engine_reset_prepare() -->RESET_CTL expects running >> GuC >> *_reset_engines() >> intel_gt_init_hw() --> GuC FW loading happens, GuC comes out of >> GS_MIA_IN_RESET. >> >> Fix the above flow so that GuC reset happens after all the >> engines reset is done. >> >> Cc: John Harrison <John.C.Harrison@Intel.com> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >> --- >> drivers/gpu/drm/i915/gt/intel_reset.c | 9 ++++-- >> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 42 +++++++++++++++++++++------ >> drivers/gpu/drm/i915/gt/uc/intel_uc.h | 1 + >> 3 files changed, 41 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c >> b/drivers/gpu/drm/i915/gt/intel_reset.c >> index c8e9aa41fdea..9ebd68ce0c22 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_reset.c >> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c >> @@ -879,8 +879,11 @@ static intel_engine_mask_t reset_prepare(struct >> intel_gt *gt) >> intel_engine_mask_t awake = 0; >> enum intel_engine_id id; >> - /* For GuC mode, ensure submission is disabled before stopping >> ring */ >> - intel_uc_reset_prepare(>->uc); >> + /* >> + * For GuC mode, ensure submission is disabled before stopping >> ring. >> + * Don't reset the GuC a engine reset requires GuC to be running. > These two lines appear to be mutually exclusive unless there is a test > for GuC submission being enabled, which I am not seeing. Note that > "ensure submission is disabled" means "reset the GuC". > >> + */ >> + intel_uc_reset_prepare_without_guc_reset(>->uc); >> for_each_engine(engine, gt, id) { >> if (intel_engine_pm_get_if_awake(engine)) >> @@ -1227,6 +1230,8 @@ void intel_gt_reset(struct intel_gt *gt, >> intel_overlay_reset(gt->i915); >> + /* Now that all engines are clean, Reset the GuC */ >> + intel_uc_reset_prepare(>->uc); >> /* >> * Next we need to restore the context, but we don't use those >> * yet either... >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> index 7a63abf8f644..5feee4db2ccc 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> @@ -345,7 +345,7 @@ static void __uc_fini(struct intel_uc *uc) >> intel_guc_fini(&uc->guc); >> } >> -static int __uc_sanitize(struct intel_uc *uc) >> +static void __uc_sanitize_without_guc_reset(struct intel_uc *uc) >> { >> struct intel_guc *guc = &uc->guc; >> struct intel_huc *huc = &uc->huc; >> @@ -354,7 +354,11 @@ static int __uc_sanitize(struct intel_uc *uc) >> intel_huc_sanitize(huc); >> intel_guc_sanitize(guc); >> +} > This seems like an extremely bad idea. You are wiping out all the GuC > communication structures on the host side while the GuC itself is > still executing and using those same structures. > > Is the failure when doing individual engine resets or when doing a > full GT reset? The failed test is doing "intel_gt_reset(gt, ALL_ENGINES, NULL)" so a full GT reset. > > If the former, I think a better approach would be to just not reset > GuC at all (or indeed any UC) if not using GuC submission. Although, > looking at the code, I'm not seeing an engine only reset path that > does nuke the UC layers? Yes, intel_engine_reset() doesn't touch UC layer. > > If it is the latter, This is the case here. > then how/why are individual engine resets happening in the middle of a > full GT reset? Don't we just splat everything all at once? It seems we use __intel_gt_reset(engine->gt, engine_mask) to reset all or some engines. > Either way, it would be safer to split at the GT reset code layer > rather than inside the UC layer. That is, when not using GuC > submission, do the entire prepare/reset/init sequence of the UC layers > as one 'atomic' operation either before the GT/engine reset or after > it (or potentially both before and after?). I think this should work. Let me try it out Thanks, Nirmoy > > John. > > >> +static int __uc_sanitize(struct intel_uc *uc) >> +{ >> + __uc_sanitize_without_guc_reset(uc); >> return __intel_uc_reset_hw(uc); >> } >> @@ -593,13 +597,7 @@ static void __uc_fini_hw(struct intel_uc *uc) >> __uc_sanitize(uc); >> } >> -/** >> - * intel_uc_reset_prepare - Prepare for reset >> - * @uc: the intel_uc structure >> - * >> - * Preparing for full gpu reset. >> - */ >> -void intel_uc_reset_prepare(struct intel_uc *uc) >> +static void __intel_uc_reset_prepare(struct intel_uc *uc, bool >> reset_guc) >> { >> struct intel_guc *guc = &uc->guc; >> @@ -617,9 +615,35 @@ void intel_uc_reset_prepare(struct intel_uc *uc) >> intel_guc_submission_reset_prepare(guc); >> sanitize: >> - __uc_sanitize(uc); >> + if (reset_guc) >> + __uc_sanitize(uc); >> + else >> + __uc_sanitize_without_guc_reset(uc); >> } >> +/** >> + * intel_uc_reset_prepare - Prepare for reset >> + * @uc: the intel_uc structure >> + * >> + * Preparing for full gpu reset. >> + */ >> +void intel_uc_reset_prepare(struct intel_uc *uc) >> +{ >> + __intel_uc_reset_prepare(uc, true); >> +} >> +/** >> + * intel_uc_reset_prepare_without_guc_reset - Prepare for reset but >> don't reset >> + * the GuC >> + * @uc: the intel_uc structure >> + * >> + * Preparing for full gpu reset. >> + */ >> +void intel_uc_reset_prepare_without_guc_reset(struct intel_uc *uc) >> +{ >> + __intel_uc_reset_prepare(uc, false); >> +} >> + >> + >> void intel_uc_reset(struct intel_uc *uc, intel_engine_mask_t stalled) >> { >> struct intel_guc *guc = &uc->guc; >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h >> index 014bb7d83689..9d6191ece498 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h >> @@ -46,6 +46,7 @@ void intel_uc_driver_late_release(struct intel_uc >> *uc); >> void intel_uc_driver_remove(struct intel_uc *uc); >> void intel_uc_init_mmio(struct intel_uc *uc); >> void intel_uc_reset_prepare(struct intel_uc *uc); >> +void intel_uc_reset_prepare_without_guc_reset(struct intel_uc *uc); >> void intel_uc_reset(struct intel_uc *uc, intel_engine_mask_t stalled); >> void intel_uc_reset_finish(struct intel_uc *uc); >> void intel_uc_cancel_requests(struct intel_uc *uc); >
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c index c8e9aa41fdea..9ebd68ce0c22 100644 --- a/drivers/gpu/drm/i915/gt/intel_reset.c +++ b/drivers/gpu/drm/i915/gt/intel_reset.c @@ -879,8 +879,11 @@ static intel_engine_mask_t reset_prepare(struct intel_gt *gt) intel_engine_mask_t awake = 0; enum intel_engine_id id; - /* For GuC mode, ensure submission is disabled before stopping ring */ - intel_uc_reset_prepare(>->uc); + /* + * For GuC mode, ensure submission is disabled before stopping ring. + * Don't reset the GuC a engine reset requires GuC to be running. + */ + intel_uc_reset_prepare_without_guc_reset(>->uc); for_each_engine(engine, gt, id) { if (intel_engine_pm_get_if_awake(engine)) @@ -1227,6 +1230,8 @@ void intel_gt_reset(struct intel_gt *gt, intel_overlay_reset(gt->i915); + /* Now that all engines are clean, Reset the GuC */ + intel_uc_reset_prepare(>->uc); /* * Next we need to restore the context, but we don't use those * yet either... diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 7a63abf8f644..5feee4db2ccc 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -345,7 +345,7 @@ static void __uc_fini(struct intel_uc *uc) intel_guc_fini(&uc->guc); } -static int __uc_sanitize(struct intel_uc *uc) +static void __uc_sanitize_without_guc_reset(struct intel_uc *uc) { struct intel_guc *guc = &uc->guc; struct intel_huc *huc = &uc->huc; @@ -354,7 +354,11 @@ static int __uc_sanitize(struct intel_uc *uc) intel_huc_sanitize(huc); intel_guc_sanitize(guc); +} +static int __uc_sanitize(struct intel_uc *uc) +{ + __uc_sanitize_without_guc_reset(uc); return __intel_uc_reset_hw(uc); } @@ -593,13 +597,7 @@ static void __uc_fini_hw(struct intel_uc *uc) __uc_sanitize(uc); } -/** - * intel_uc_reset_prepare - Prepare for reset - * @uc: the intel_uc structure - * - * Preparing for full gpu reset. - */ -void intel_uc_reset_prepare(struct intel_uc *uc) +static void __intel_uc_reset_prepare(struct intel_uc *uc, bool reset_guc) { struct intel_guc *guc = &uc->guc; @@ -617,9 +615,35 @@ void intel_uc_reset_prepare(struct intel_uc *uc) intel_guc_submission_reset_prepare(guc); sanitize: - __uc_sanitize(uc); + if (reset_guc) + __uc_sanitize(uc); + else + __uc_sanitize_without_guc_reset(uc); } +/** + * intel_uc_reset_prepare - Prepare for reset + * @uc: the intel_uc structure + * + * Preparing for full gpu reset. + */ +void intel_uc_reset_prepare(struct intel_uc *uc) +{ + __intel_uc_reset_prepare(uc, true); +} +/** + * intel_uc_reset_prepare_without_guc_reset - Prepare for reset but don't reset + * the GuC + * @uc: the intel_uc structure + * + * Preparing for full gpu reset. + */ +void intel_uc_reset_prepare_without_guc_reset(struct intel_uc *uc) +{ + __intel_uc_reset_prepare(uc, false); +} + + void intel_uc_reset(struct intel_uc *uc, intel_engine_mask_t stalled) { struct intel_guc *guc = &uc->guc; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h index 014bb7d83689..9d6191ece498 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h @@ -46,6 +46,7 @@ void intel_uc_driver_late_release(struct intel_uc *uc); void intel_uc_driver_remove(struct intel_uc *uc); void intel_uc_init_mmio(struct intel_uc *uc); void intel_uc_reset_prepare(struct intel_uc *uc); +void intel_uc_reset_prepare_without_guc_reset(struct intel_uc *uc); void intel_uc_reset(struct intel_uc *uc, intel_engine_mask_t stalled); void intel_uc_reset_finish(struct intel_uc *uc); void intel_uc_cancel_requests(struct intel_uc *uc);
Currently intel_gt_reset() happens as follows: reset_prepare() ---> Sends GDRST to GuC, GuC is in GS_MIA_IN_RESET do_reset() __intel_gt_reset() *_engine_reset_prepare() -->RESET_CTL expects running GuC *_reset_engines() intel_gt_init_hw() --> GuC FW loading happens, GuC comes out of GS_MIA_IN_RESET. Fix the above flow so that GuC reset happens after all the engines reset is done. Cc: John Harrison <John.C.Harrison@Intel.com> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> --- drivers/gpu/drm/i915/gt/intel_reset.c | 9 ++++-- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 42 +++++++++++++++++++++------ drivers/gpu/drm/i915/gt/uc/intel_uc.h | 1 + 3 files changed, 41 insertions(+), 11 deletions(-)