| Message ID | 20190813162628.18531-1-fernando.pacheco@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/uc: Fini hw even if GuC is not running | expand |
On 8/13/19 9:26 AM, Fernando Pacheco wrote: > We should not be skipping uc_fini_hw on finding GuC > is no longer running. There is plenty of hw and internal > state that can be cleaned up without having to communicate > with GuC. > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110943 > Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index 0dc2b0cf4604..c698cddc14dc 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -521,7 +521,7 @@ void intel_uc_fini_hw(struct intel_uc *uc) > { > struct intel_guc *guc = &uc->guc; > > - if (!intel_guc_is_running(guc)) > + if (!intel_uc_supports_guc(uc)) Both calls below handle the case where GuC is already not running so we're safe, but now that we wedge when we hit a guc error we can also do something like: -EIO load error -> uc_fini_hw() -> wedge and then unload -> uc_fini_hw() it should be all be handled safely (the fault injection test is passing where we've run it), but we end up with "GuC communication disabled" multiple times in the logs: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13999/fi-skl-guc/igt@i915_module_load@reload-with-fault-injection.html <6> [237.818905] [drm] GuC communication enabled .... <6> [237.822739] i915 0000:00:02.0: [drm:__i915_inject_load_error [i915]] Injecting failure -5 at checkpoint 44 [i915_gem_init:1503] <6> [237.840808] [drm] GuC communication disabled ... <6> [238.160935] [drm] GuC communication disabled Maybe return early from guc_disable_communication if the communication was already disabled? Daniele > return; > > if (intel_uc_supports_guc_submission(uc)) >
On Tue, 13 Aug 2019 18:26:28 +0200, Fernando Pacheco <fernando.pacheco@intel.com> wrote: > We should not be skipping uc_fini_hw on finding GuC > is no longer running. There is plenty of hw and internal > state that can be cleaned up without having to communicate > with GuC. > > Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index 0dc2b0cf4604..c698cddc14dc 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -521,7 +521,7 @@ void intel_uc_fini_hw(struct intel_uc *uc) > { > struct intel_guc *guc = &uc->guc; > - if (!intel_guc_is_running(guc)) > + if (!intel_uc_supports_guc(uc)) there is a huge difference between is_running vs supports_guc and choosing supports_guc is optimist approach as we can fail to fetch guc fw and abort early, so maybe if (!intel_uc_fw_is_available(&guc->fw)) would be closer to reality (assuming we don't fail on wopcm (hmm, maybe we should force fw state to FAIL in such case?) > return; > if (intel_uc_supports_guc_submission(uc))
On 8/13/19 1:16 PM, Daniele Ceraolo Spurio wrote: > > > On 8/13/19 9:26 AM, Fernando Pacheco wrote: >> We should not be skipping uc_fini_hw on finding GuC >> is no longer running. There is plenty of hw and internal >> state that can be cleaned up without having to communicate >> with GuC. >> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110943 > >> Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> index 0dc2b0cf4604..c698cddc14dc 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> @@ -521,7 +521,7 @@ void intel_uc_fini_hw(struct intel_uc *uc) >> { >> struct intel_guc *guc = &uc->guc; >> - if (!intel_guc_is_running(guc)) >> + if (!intel_uc_supports_guc(uc)) > > Both calls below handle the case where GuC is already not running so we're safe, but now that we wedge when we hit a guc error we can also do something like: > > -EIO load error -> uc_fini_hw() -> wedge > and then > unload -> uc_fini_hw() > > it should be all be handled safely (the fault injection test is passing where we've run it), but we end up with "GuC communication disabled" multiple times in the logs: > > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13999/fi-skl-guc/igt@i915_module_load@reload-with-fault-injection.html > > <6> [237.818905] [drm] GuC communication enabled > .... > <6> [237.822739] i915 0000:00:02.0: [drm:__i915_inject_load_error [i915]] Injecting failure -5 at checkpoint 44 [i915_gem_init:1503] > <6> [237.840808] [drm] GuC communication disabled > ... > <6> [238.160935] [drm] GuC communication disabled > > Maybe return early from guc_disable_communication if the communication was already disabled? As discussed offline, an early return might also require changes to the stop communication phase. I'll work on this separately as for now the extra disable only results in the extra logging. Thanks, Fernando > > > Daniele > >> return; >> if (intel_uc_supports_guc_submission(uc)) >>
On 8/13/19 1:18 PM, Michal Wajdeczko wrote: > On Tue, 13 Aug 2019 18:26:28 +0200, Fernando Pacheco <fernando.pacheco@intel.com> wrote: > >> We should not be skipping uc_fini_hw on finding GuC >> is no longer running. There is plenty of hw and internal >> state that can be cleaned up without having to communicate >> with GuC. >> >> Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com> >> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> --- >> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> index 0dc2b0cf4604..c698cddc14dc 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> @@ -521,7 +521,7 @@ void intel_uc_fini_hw(struct intel_uc *uc) >> { >> struct intel_guc *guc = &uc->guc; >> - if (!intel_guc_is_running(guc)) >> + if (!intel_uc_supports_guc(uc)) > > there is a huge difference between is_running vs supports_guc > and choosing supports_guc is optimist approach as we can fail > to fetch guc fw and abort early, so maybe > > if (!intel_uc_fw_is_available(&guc->fw)) This is the better check. Thanks! > > would be closer to reality (assuming we don't fail on wopcm > (hmm, maybe we should force fw state to FAIL in such case?) That would make sense to me. The fail on wopcm directly affects the state of the fw because we abort the upload as a result. Thanks, Fernando > > >> return; >> if (intel_uc_supports_guc_submission(uc))
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 0dc2b0cf4604..c698cddc14dc 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -521,7 +521,7 @@ void intel_uc_fini_hw(struct intel_uc *uc) { struct intel_guc *guc = &uc->guc; - if (!intel_guc_is_running(guc)) + if (!intel_uc_supports_guc(uc)) return; if (intel_uc_supports_guc_submission(uc))
We should not be skipping uc_fini_hw on finding GuC is no longer running. There is plenty of hw and internal state that can be cleaned up without having to communicate with GuC. Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)