| Message ID | 20200203232838.14822-7-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Commit early to GuC | expand |
On Tue, 04 Feb 2020 00:28:34 +0100, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote: > To be able to setup GuC submission functions during engine init we need > to commit to using GuC as soon as possible. > Currently, the only thing that can stop us from using the > microcontrollers once we've fetched the blobs is a fundamental > error (e.g. OOM); given that if we hit such an error we can't really > fall-back to anything, we can "officialize" the FW fetching completion > as the moment at which we're committing to using GuC. > > To better differentiate this case, the uses_guc check, which indicates > that GuC is supported and was selected in modparam, is renamed to > wants_guc and a new uses_guc is introduced to represent the case were > we're committed to using the GuC. Note that uses_guc does still not imply > that the blob is actually loaded on the HW (is_running is the check for > that). Also, since we need to have attempted the fetch for the result > of uses_guc to be meaningful, we need to make sure we've moved away > from INTEL_UC_FIRMWARE_SELECTED. > > All the GuC changes have been mirrored on the HuC for coherency. > > v2: split fetch return changes and new macros to their own patches, > support HuC only if GuC is wanted, improve "used" state > description (Michal) > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: John Harrison <John.C.Harrison@Intel.com> > Cc: Matthew Brost <matthew.brost@intel.com> > Reviewed-by: Fernando Pacheco <fernando.pacheco@intel.com> #v1 > --- > drivers/gpu/drm/i915/gt/uc/intel_guc.h | 8 +++++++- > drivers/gpu/drm/i915/gt/uc/intel_huc.h | 8 +++++++- > drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 2 +- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 23 ++++++++++++---------- > drivers/gpu/drm/i915/gt/uc/intel_uc.h | 24 ++++++++++++++++++++++- > 5 files changed, 51 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > index 7ca9e5159f05..f6b33745ae0b 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > @@ -143,11 +143,17 @@ static inline bool intel_guc_is_supported(struct > intel_guc *guc) > return intel_uc_fw_is_supported(&guc->fw); > } > -static inline bool intel_guc_is_enabled(struct intel_guc *guc) > +static inline bool intel_guc_is_wanted(struct intel_guc *guc) > { > return intel_uc_fw_is_enabled(&guc->fw); > } > +static inline bool intel_guc_is_used(struct intel_guc *guc) > +{ > + GEM_BUG_ON(__intel_uc_fw_status(&guc->fw) == > INTEL_UC_FIRMWARE_SELECTED); > + return intel_uc_fw_is_available(&guc->fw); > +} > + > static inline bool intel_guc_is_fw_running(struct intel_guc *guc) > { > return intel_uc_fw_is_running(&guc->fw); > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h > b/drivers/gpu/drm/i915/gt/uc/intel_huc.h > index 644c059fe01d..a40b9cfc6c22 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h > @@ -41,11 +41,17 @@ static inline bool intel_huc_is_supported(struct > intel_huc *huc) > return intel_uc_fw_is_supported(&huc->fw); > } > -static inline bool intel_huc_is_enabled(struct intel_huc *huc) > +static inline bool intel_huc_is_wanted(struct intel_huc *huc) > { > return intel_uc_fw_is_enabled(&huc->fw); > } > +static inline bool intel_huc_is_used(struct intel_huc *huc) > +{ > + GEM_BUG_ON(__intel_uc_fw_status(&huc->fw) == > INTEL_UC_FIRMWARE_SELECTED); > + return intel_uc_fw_is_available(&huc->fw); > +} > + > static inline bool intel_huc_is_authenticated(struct intel_huc *huc) > { > return intel_uc_fw_is_running(&huc->fw); > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > index eee193bf2cc4..9cdf4cbe691c 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > @@ -20,7 +20,7 @@ void intel_huc_fw_init_early(struct intel_huc *huc) > struct drm_i915_private *i915 = gt->i915; > intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC, > - intel_uc_uses_guc(uc), > + intel_uc_wants_guc(uc), > INTEL_INFO(i915)->platform, INTEL_REVID(i915)); > } > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index affc4d6f9ead..654d7c0c757a 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -48,17 +48,17 @@ static void __confirm_options(struct intel_uc *uc) > DRM_DEV_DEBUG_DRIVER(i915->drm.dev, > "enable_guc=%d (guc:%s submission:%s huc:%s)\n", > i915_modparams.enable_guc, > - yesno(intel_uc_uses_guc(uc)), > + yesno(intel_uc_wants_guc(uc)), > yesno(intel_uc_uses_guc_submission(uc)), > - yesno(intel_uc_uses_huc(uc))); > + yesno(intel_uc_wants_huc(uc))); > if (i915_modparams.enable_guc == -1) > return; > if (i915_modparams.enable_guc == 0) { > - GEM_BUG_ON(intel_uc_uses_guc(uc)); > + GEM_BUG_ON(intel_uc_wants_guc(uc)); > GEM_BUG_ON(intel_uc_uses_guc_submission(uc)); > - GEM_BUG_ON(intel_uc_uses_huc(uc)); > + GEM_BUG_ON(intel_uc_wants_huc(uc)); > return; > } > @@ -93,7 +93,7 @@ void intel_uc_init_early(struct intel_uc *uc) > __confirm_options(uc); > - if (intel_uc_uses_guc(uc)) > + if (intel_uc_wants_guc(uc)) > uc->ops = &uc_ops_on; > else > uc->ops = &uc_ops_off; > @@ -257,13 +257,13 @@ static void __uc_fetch_firmwares(struct intel_uc > *uc) > { > int err; > - GEM_BUG_ON(!intel_uc_uses_guc(uc)); > + GEM_BUG_ON(!intel_uc_wants_guc(uc)); > err = intel_uc_fw_fetch(&uc->guc.fw); > if (err) > return; > - if (intel_uc_uses_huc(uc)) > + if (intel_uc_wants_huc(uc)) > intel_uc_fw_fetch(&uc->huc.fw); > } > @@ -279,7 +279,10 @@ static void __uc_init(struct intel_uc *uc) > struct intel_huc *huc = &uc->huc; > int ret; > - GEM_BUG_ON(!intel_uc_uses_guc(uc)); > + GEM_BUG_ON(!intel_uc_wants_guc(uc)); > + > + if (!intel_uc_uses_guc(uc)) > + return; > /* XXX: GuC submission is unavailable for now */ > GEM_BUG_ON(intel_uc_supports_guc_submission(uc)); > @@ -322,7 +325,7 @@ static int uc_init_wopcm(struct intel_uc *uc) > struct intel_uncore *uncore = gt->uncore; > u32 base = intel_wopcm_guc_base(>->i915->wopcm); > u32 size = intel_wopcm_guc_size(>->i915->wopcm); > - u32 huc_agent = intel_uc_uses_huc(uc) ? HUC_LOADING_AGENT_GUC : 0; > + u32 huc_agent = intel_uc_wants_huc(uc) ? HUC_LOADING_AGENT_GUC : 0; > u32 mask; > int err; > @@ -402,7 +405,7 @@ static int __uc_init_hw(struct intel_uc *uc) > int ret, attempts; > GEM_BUG_ON(!intel_uc_supports_guc(uc)); > - GEM_BUG_ON(!intel_uc_uses_guc(uc)); > + GEM_BUG_ON(!intel_uc_wants_guc(uc)); > if (!intel_uc_fw_is_available(&guc->fw)) { > ret = __uc_check_hw(uc) || > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h > b/drivers/gpu/drm/i915/gt/uc/intel_uc.h > index 2ce993ceb60a..a41aaf353f88 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h > @@ -40,6 +40,27 @@ void intel_uc_runtime_suspend(struct intel_uc *uc); > int intel_uc_resume(struct intel_uc *uc); > int intel_uc_runtime_resume(struct intel_uc *uc); > +/* > + * We need to know as early as possible if we're going to use GuC or > not to > + * take the correct setup paths. Additionally, once we've started > loading the > + * GuC, it is unsafe to keep executing without it because some parts of > the HW, > + * a subset of which is not cleaned on GT reset, will start expecting > the GuC FW > + * to be running. > + * To solve both these requirements, we commit to using the > microcontrollers if > + * the relevant modparam is set and the blobs are found on the system. > At this > + * stage, the only thing that can stop us from attempting to load the > blobs on > + * the HW and use them is a fundamental issue (e.g. no memory for our > + * structures); if we hit such a problem during driver load we're > broken even > + * without GuC, so there is no point in trying to fall back. > + * > + * Given the above, we can be in one of 4 states, with the last one > implying > + * we're committed to using the microcontroller: nit: maybe we should document below state names in capitals as: NOT_SUPPORTED, WANTED, IN_USE, ... > + * - Not supported: not available in HW and/or firmware not defined. > + * - Supported: available in HW and firmware defined. > + * - Wanted: supported + enabled in modparam. hmm, we are checking modparam during fw path selection, thus for me there is no difference between SUPPORTED and WANTED, what I missed? maybe we only need 3 states: NOT_SUPPORTED, WANTED, IN_USE. > + * - In use: wanted + firmware found on the system and successfully > fetched. In what state we will be after unsuccessful fetch ? still WANTED ? > + */ > + > #define __uc_state_checker(x, state, required) \ > static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \ > { \ > @@ -48,7 +69,8 @@ static inline bool intel_uc_##state##_##x(struct > intel_uc *uc) \ > #define uc_state_checkers(x) \ > __uc_state_checker(x, supports, supported) \ > -__uc_state_checker(x, uses, enabled) > +__uc_state_checker(x, wants, wanted) \ > +__uc_state_checker(x, uses, used) > uc_state_checkers(guc); > uc_state_checkers(huc);
On 2/4/20 9:54 AM, Michal Wajdeczko wrote: > On Tue, 04 Feb 2020 00:28:34 +0100, Daniele Ceraolo Spurio > <daniele.ceraolospurio@intel.com> wrote: > >> To be able to setup GuC submission functions during engine init we need >> to commit to using GuC as soon as possible. >> Currently, the only thing that can stop us from using the >> microcontrollers once we've fetched the blobs is a fundamental >> error (e.g. OOM); given that if we hit such an error we can't really >> fall-back to anything, we can "officialize" the FW fetching completion >> as the moment at which we're committing to using GuC. >> >> To better differentiate this case, the uses_guc check, which indicates >> that GuC is supported and was selected in modparam, is renamed to >> wants_guc and a new uses_guc is introduced to represent the case were >> we're committed to using the GuC. Note that uses_guc does still not imply >> that the blob is actually loaded on the HW (is_running is the check for >> that). Also, since we need to have attempted the fetch for the result >> of uses_guc to be meaningful, we need to make sure we've moved away >> from INTEL_UC_FIRMWARE_SELECTED. >> >> All the GuC changes have been mirrored on the HuC for coherency. >> >> v2: split fetch return changes and new macros to their own patches, >> support HuC only if GuC is wanted, improve "used" state >> description (Michal) >> >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: John Harrison <John.C.Harrison@Intel.com> >> Cc: Matthew Brost <matthew.brost@intel.com> >> Reviewed-by: Fernando Pacheco <fernando.pacheco@intel.com> #v1 >> --- >> drivers/gpu/drm/i915/gt/uc/intel_guc.h | 8 +++++++- >> drivers/gpu/drm/i915/gt/uc/intel_huc.h | 8 +++++++- >> drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 2 +- >> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 23 ++++++++++++---------- >> drivers/gpu/drm/i915/gt/uc/intel_uc.h | 24 ++++++++++++++++++++++- >> 5 files changed, 51 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h >> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h >> index 7ca9e5159f05..f6b33745ae0b 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h >> @@ -143,11 +143,17 @@ static inline bool intel_guc_is_supported(struct >> intel_guc *guc) >> return intel_uc_fw_is_supported(&guc->fw); >> } >> -static inline bool intel_guc_is_enabled(struct intel_guc *guc) >> +static inline bool intel_guc_is_wanted(struct intel_guc *guc) >> { >> return intel_uc_fw_is_enabled(&guc->fw); >> } >> +static inline bool intel_guc_is_used(struct intel_guc *guc) >> +{ >> + GEM_BUG_ON(__intel_uc_fw_status(&guc->fw) == >> INTEL_UC_FIRMWARE_SELECTED); >> + return intel_uc_fw_is_available(&guc->fw); >> +} >> + >> static inline bool intel_guc_is_fw_running(struct intel_guc *guc) >> { >> return intel_uc_fw_is_running(&guc->fw); >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h >> b/drivers/gpu/drm/i915/gt/uc/intel_huc.h >> index 644c059fe01d..a40b9cfc6c22 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h >> @@ -41,11 +41,17 @@ static inline bool intel_huc_is_supported(struct >> intel_huc *huc) >> return intel_uc_fw_is_supported(&huc->fw); >> } >> -static inline bool intel_huc_is_enabled(struct intel_huc *huc) >> +static inline bool intel_huc_is_wanted(struct intel_huc *huc) >> { >> return intel_uc_fw_is_enabled(&huc->fw); >> } >> +static inline bool intel_huc_is_used(struct intel_huc *huc) >> +{ >> + GEM_BUG_ON(__intel_uc_fw_status(&huc->fw) == >> INTEL_UC_FIRMWARE_SELECTED); >> + return intel_uc_fw_is_available(&huc->fw); >> +} >> + >> static inline bool intel_huc_is_authenticated(struct intel_huc *huc) >> { >> return intel_uc_fw_is_running(&huc->fw); >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c >> b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c >> index eee193bf2cc4..9cdf4cbe691c 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c >> @@ -20,7 +20,7 @@ void intel_huc_fw_init_early(struct intel_huc *huc) >> struct drm_i915_private *i915 = gt->i915; >> intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC, >> - intel_uc_uses_guc(uc), >> + intel_uc_wants_guc(uc), >> INTEL_INFO(i915)->platform, INTEL_REVID(i915)); >> } >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> index affc4d6f9ead..654d7c0c757a 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> @@ -48,17 +48,17 @@ static void __confirm_options(struct intel_uc *uc) >> DRM_DEV_DEBUG_DRIVER(i915->drm.dev, >> "enable_guc=%d (guc:%s submission:%s huc:%s)\n", >> i915_modparams.enable_guc, >> - yesno(intel_uc_uses_guc(uc)), >> + yesno(intel_uc_wants_guc(uc)), >> yesno(intel_uc_uses_guc_submission(uc)), >> - yesno(intel_uc_uses_huc(uc))); >> + yesno(intel_uc_wants_huc(uc))); >> if (i915_modparams.enable_guc == -1) >> return; >> if (i915_modparams.enable_guc == 0) { >> - GEM_BUG_ON(intel_uc_uses_guc(uc)); >> + GEM_BUG_ON(intel_uc_wants_guc(uc)); >> GEM_BUG_ON(intel_uc_uses_guc_submission(uc)); >> - GEM_BUG_ON(intel_uc_uses_huc(uc)); >> + GEM_BUG_ON(intel_uc_wants_huc(uc)); >> return; >> } >> @@ -93,7 +93,7 @@ void intel_uc_init_early(struct intel_uc *uc) >> __confirm_options(uc); >> - if (intel_uc_uses_guc(uc)) >> + if (intel_uc_wants_guc(uc)) >> uc->ops = &uc_ops_on; >> else >> uc->ops = &uc_ops_off; >> @@ -257,13 +257,13 @@ static void __uc_fetch_firmwares(struct intel_uc >> *uc) >> { >> int err; >> - GEM_BUG_ON(!intel_uc_uses_guc(uc)); >> + GEM_BUG_ON(!intel_uc_wants_guc(uc)); >> err = intel_uc_fw_fetch(&uc->guc.fw); >> if (err) >> return; >> - if (intel_uc_uses_huc(uc)) >> + if (intel_uc_wants_huc(uc)) >> intel_uc_fw_fetch(&uc->huc.fw); >> } >> @@ -279,7 +279,10 @@ static void __uc_init(struct intel_uc *uc) >> struct intel_huc *huc = &uc->huc; >> int ret; >> - GEM_BUG_ON(!intel_uc_uses_guc(uc)); >> + GEM_BUG_ON(!intel_uc_wants_guc(uc)); >> + >> + if (!intel_uc_uses_guc(uc)) >> + return; >> /* XXX: GuC submission is unavailable for now */ >> GEM_BUG_ON(intel_uc_supports_guc_submission(uc)); >> @@ -322,7 +325,7 @@ static int uc_init_wopcm(struct intel_uc *uc) >> struct intel_uncore *uncore = gt->uncore; >> u32 base = intel_wopcm_guc_base(>->i915->wopcm); >> u32 size = intel_wopcm_guc_size(>->i915->wopcm); >> - u32 huc_agent = intel_uc_uses_huc(uc) ? HUC_LOADING_AGENT_GUC : 0; >> + u32 huc_agent = intel_uc_wants_huc(uc) ? HUC_LOADING_AGENT_GUC : 0; this should've been uses_huc, since this is post fetch. >> u32 mask; >> int err; >> @@ -402,7 +405,7 @@ static int __uc_init_hw(struct intel_uc *uc) >> int ret, attempts; >> GEM_BUG_ON(!intel_uc_supports_guc(uc)); >> - GEM_BUG_ON(!intel_uc_uses_guc(uc)); >> + GEM_BUG_ON(!intel_uc_wants_guc(uc)); >> if (!intel_uc_fw_is_available(&guc->fw)) { >> ret = __uc_check_hw(uc) || >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h >> index 2ce993ceb60a..a41aaf353f88 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h >> @@ -40,6 +40,27 @@ void intel_uc_runtime_suspend(struct intel_uc *uc); >> int intel_uc_resume(struct intel_uc *uc); >> int intel_uc_runtime_resume(struct intel_uc *uc); >> +/* >> + * We need to know as early as possible if we're going to use GuC or >> not to >> + * take the correct setup paths. Additionally, once we've started >> loading the >> + * GuC, it is unsafe to keep executing without it because some parts >> of the HW, >> + * a subset of which is not cleaned on GT reset, will start expecting >> the GuC FW >> + * to be running. >> + * To solve both these requirements, we commit to using the >> microcontrollers if >> + * the relevant modparam is set and the blobs are found on the >> system. At this >> + * stage, the only thing that can stop us from attempting to load the >> blobs on >> + * the HW and use them is a fundamental issue (e.g. no memory for our >> + * structures); if we hit such a problem during driver load we're >> broken even >> + * without GuC, so there is no point in trying to fall back. >> + * >> + * Given the above, we can be in one of 4 states, with the last one >> implying >> + * we're committed to using the microcontroller: > > nit: maybe we should document below state names in capitals as: > NOT_SUPPORTED, WANTED, IN_USE, ... > >> + * - Not supported: not available in HW and/or firmware not defined. >> + * - Supported: available in HW and firmware defined. >> + * - Wanted: supported + enabled in modparam. > > hmm, we are checking modparam during fw path selection, thus for me > there is no difference between SUPPORTED and WANTED, what I missed? > > maybe we only need 3 states: NOT_SUPPORTED, WANTED, IN_USE. > enable_guc=0 maps to SUPPORTED on platforms that have the GuC, i.e. the HW has it but we haven't turned it on. NOT_SUPPORTED is for older platforms that don't have the microcontroller, while WANTED is for enable_guc != 0 on platforms that do support the GuC. Supported vs wanted is mainly used for error logging. Note than NOT_SUPPORTED is not a separate state in the code, but it is quite literally !(SUPPORTED), so the actual states are indeed 3. >> + * - In use: wanted + firmware found on the system and successfully >> fetched. > > In what state we will be after unsuccessful fetch ? still WANTED ? > yes, on purpose. This reflects the current behavior and I believe it'll be useful in case we need to conditionally unwind anything that has been done before the fetch. Daniele >> + */ >> + >> #define __uc_state_checker(x, state, required) \ >> static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \ >> { \ >> @@ -48,7 +69,8 @@ static inline bool intel_uc_##state##_##x(struct >> intel_uc *uc) \ >> #define uc_state_checkers(x) \ >> __uc_state_checker(x, supports, supported) \ >> -__uc_state_checker(x, uses, enabled) >> +__uc_state_checker(x, wants, wanted) \ >> +__uc_state_checker(x, uses, used) >> uc_state_checkers(guc); >> uc_state_checkers(huc);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h index 7ca9e5159f05..f6b33745ae0b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h @@ -143,11 +143,17 @@ static inline bool intel_guc_is_supported(struct intel_guc *guc) return intel_uc_fw_is_supported(&guc->fw); } -static inline bool intel_guc_is_enabled(struct intel_guc *guc) +static inline bool intel_guc_is_wanted(struct intel_guc *guc) { return intel_uc_fw_is_enabled(&guc->fw); } +static inline bool intel_guc_is_used(struct intel_guc *guc) +{ + GEM_BUG_ON(__intel_uc_fw_status(&guc->fw) == INTEL_UC_FIRMWARE_SELECTED); + return intel_uc_fw_is_available(&guc->fw); +} + static inline bool intel_guc_is_fw_running(struct intel_guc *guc) { return intel_uc_fw_is_running(&guc->fw); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h index 644c059fe01d..a40b9cfc6c22 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h @@ -41,11 +41,17 @@ static inline bool intel_huc_is_supported(struct intel_huc *huc) return intel_uc_fw_is_supported(&huc->fw); } -static inline bool intel_huc_is_enabled(struct intel_huc *huc) +static inline bool intel_huc_is_wanted(struct intel_huc *huc) { return intel_uc_fw_is_enabled(&huc->fw); } +static inline bool intel_huc_is_used(struct intel_huc *huc) +{ + GEM_BUG_ON(__intel_uc_fw_status(&huc->fw) == INTEL_UC_FIRMWARE_SELECTED); + return intel_uc_fw_is_available(&huc->fw); +} + static inline bool intel_huc_is_authenticated(struct intel_huc *huc) { return intel_uc_fw_is_running(&huc->fw); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c index eee193bf2cc4..9cdf4cbe691c 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c @@ -20,7 +20,7 @@ void intel_huc_fw_init_early(struct intel_huc *huc) struct drm_i915_private *i915 = gt->i915; intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC, - intel_uc_uses_guc(uc), + intel_uc_wants_guc(uc), INTEL_INFO(i915)->platform, INTEL_REVID(i915)); } diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index affc4d6f9ead..654d7c0c757a 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -48,17 +48,17 @@ static void __confirm_options(struct intel_uc *uc) DRM_DEV_DEBUG_DRIVER(i915->drm.dev, "enable_guc=%d (guc:%s submission:%s huc:%s)\n", i915_modparams.enable_guc, - yesno(intel_uc_uses_guc(uc)), + yesno(intel_uc_wants_guc(uc)), yesno(intel_uc_uses_guc_submission(uc)), - yesno(intel_uc_uses_huc(uc))); + yesno(intel_uc_wants_huc(uc))); if (i915_modparams.enable_guc == -1) return; if (i915_modparams.enable_guc == 0) { - GEM_BUG_ON(intel_uc_uses_guc(uc)); + GEM_BUG_ON(intel_uc_wants_guc(uc)); GEM_BUG_ON(intel_uc_uses_guc_submission(uc)); - GEM_BUG_ON(intel_uc_uses_huc(uc)); + GEM_BUG_ON(intel_uc_wants_huc(uc)); return; } @@ -93,7 +93,7 @@ void intel_uc_init_early(struct intel_uc *uc) __confirm_options(uc); - if (intel_uc_uses_guc(uc)) + if (intel_uc_wants_guc(uc)) uc->ops = &uc_ops_on; else uc->ops = &uc_ops_off; @@ -257,13 +257,13 @@ static void __uc_fetch_firmwares(struct intel_uc *uc) { int err; - GEM_BUG_ON(!intel_uc_uses_guc(uc)); + GEM_BUG_ON(!intel_uc_wants_guc(uc)); err = intel_uc_fw_fetch(&uc->guc.fw); if (err) return; - if (intel_uc_uses_huc(uc)) + if (intel_uc_wants_huc(uc)) intel_uc_fw_fetch(&uc->huc.fw); } @@ -279,7 +279,10 @@ static void __uc_init(struct intel_uc *uc) struct intel_huc *huc = &uc->huc; int ret; - GEM_BUG_ON(!intel_uc_uses_guc(uc)); + GEM_BUG_ON(!intel_uc_wants_guc(uc)); + + if (!intel_uc_uses_guc(uc)) + return; /* XXX: GuC submission is unavailable for now */ GEM_BUG_ON(intel_uc_supports_guc_submission(uc)); @@ -322,7 +325,7 @@ static int uc_init_wopcm(struct intel_uc *uc) struct intel_uncore *uncore = gt->uncore; u32 base = intel_wopcm_guc_base(>->i915->wopcm); u32 size = intel_wopcm_guc_size(>->i915->wopcm); - u32 huc_agent = intel_uc_uses_huc(uc) ? HUC_LOADING_AGENT_GUC : 0; + u32 huc_agent = intel_uc_wants_huc(uc) ? HUC_LOADING_AGENT_GUC : 0; u32 mask; int err; @@ -402,7 +405,7 @@ static int __uc_init_hw(struct intel_uc *uc) int ret, attempts; GEM_BUG_ON(!intel_uc_supports_guc(uc)); - GEM_BUG_ON(!intel_uc_uses_guc(uc)); + GEM_BUG_ON(!intel_uc_wants_guc(uc)); if (!intel_uc_fw_is_available(&guc->fw)) { ret = __uc_check_hw(uc) || diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h index 2ce993ceb60a..a41aaf353f88 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h @@ -40,6 +40,27 @@ void intel_uc_runtime_suspend(struct intel_uc *uc); int intel_uc_resume(struct intel_uc *uc); int intel_uc_runtime_resume(struct intel_uc *uc); +/* + * We need to know as early as possible if we're going to use GuC or not to + * take the correct setup paths. Additionally, once we've started loading the + * GuC, it is unsafe to keep executing without it because some parts of the HW, + * a subset of which is not cleaned on GT reset, will start expecting the GuC FW + * to be running. + * To solve both these requirements, we commit to using the microcontrollers if + * the relevant modparam is set and the blobs are found on the system. At this + * stage, the only thing that can stop us from attempting to load the blobs on + * the HW and use them is a fundamental issue (e.g. no memory for our + * structures); if we hit such a problem during driver load we're broken even + * without GuC, so there is no point in trying to fall back. + * + * Given the above, we can be in one of 4 states, with the last one implying + * we're committed to using the microcontroller: + * - Not supported: not available in HW and/or firmware not defined. + * - Supported: available in HW and firmware defined. + * - Wanted: supported + enabled in modparam. + * - In use: wanted + firmware found on the system and successfully fetched. + */ + #define __uc_state_checker(x, state, required) \ static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \ { \ @@ -48,7 +69,8 @@ static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \ #define uc_state_checkers(x) \ __uc_state_checker(x, supports, supported) \ -__uc_state_checker(x, uses, enabled) +__uc_state_checker(x, wants, wanted) \ +__uc_state_checker(x, uses, used) uc_state_checkers(guc); uc_state_checkers(huc);
To be able to setup GuC submission functions during engine init we need to commit to using GuC as soon as possible. Currently, the only thing that can stop us from using the microcontrollers once we've fetched the blobs is a fundamental error (e.g. OOM); given that if we hit such an error we can't really fall-back to anything, we can "officialize" the FW fetching completion as the moment at which we're committing to using GuC. To better differentiate this case, the uses_guc check, which indicates that GuC is supported and was selected in modparam, is renamed to wants_guc and a new uses_guc is introduced to represent the case were we're committed to using the GuC. Note that uses_guc does still not imply that the blob is actually loaded on the HW (is_running is the check for that). Also, since we need to have attempted the fetch for the result of uses_guc to be meaningful, we need to make sure we've moved away from INTEL_UC_FIRMWARE_SELECTED. All the GuC changes have been mirrored on the HuC for coherency. v2: split fetch return changes and new macros to their own patches, support HuC only if GuC is wanted, improve "used" state description (Michal) Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: John Harrison <John.C.Harrison@Intel.com> Cc: Matthew Brost <matthew.brost@intel.com> Reviewed-by: Fernando Pacheco <fernando.pacheco@intel.com> #v1 --- drivers/gpu/drm/i915/gt/uc/intel_guc.h | 8 +++++++- drivers/gpu/drm/i915/gt/uc/intel_huc.h | 8 +++++++- drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 2 +- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 23 ++++++++++++---------- drivers/gpu/drm/i915/gt/uc/intel_uc.h | 24 ++++++++++++++++++++++- 5 files changed, 51 insertions(+), 14 deletions(-)