Message ID | 20180313135408.39556-1-michal.wajdeczko@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 3/13/2018 7:24 PM, Michal Wajdeczko wrote: > Since commit 6ca9a2beb54a ("drm/i915: Unwind i915_gem_init() failure") > we believed that we correctly handle all errors encountered during > GuC initialization, including special one that indicates request to > run driver with disabled GPU submission (-EIO). > > Unfortunately since commit 121981fafe69 ("drm/i915/guc: Combine > enable_guc_loading|submission modparams") we stopped using that > error code to avoid unwanted fallback to execlist submission mode. > > In result any GuC initialization failure was treated as non-recoverable > error leading to driver load abort, so we could not even read related > GuC error log to investigate cause of the problem. > > Fix that by always returning -EIO on uC hardware related failure. > > v2: don't allow -EIO from uc_init > don't call uc_fini[_misc] on -EIO > mark guc fw as failed on hw init failure > prepare uc_fini_hw to run after earlier -EIO > > v3: update comments (Sagar) > use sanitize functions on failure in init_hw (Michal) > and also sanitize guc/huc fw in fini_hw (Michal) > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Winiarski <michal.winiarski@intel.com> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 17 ++++++++++------- > drivers/gpu/drm/i915/intel_guc.h | 5 +++++ > drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++---- > drivers/gpu/drm/i915/intel_uc_fw.h | 5 +++++ > 4 files changed, 34 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 05b0724..8eed87d 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -5338,8 +5338,10 @@ int i915_gem_init(struct drm_i915_private *dev_priv) > intel_init_gt_powersave(dev_priv); > > ret = intel_uc_init(dev_priv); > - if (ret) > + if (ret) { > + GEM_BUG_ON(ret == -EIO); > goto err_pm; > + } > > ret = i915_gem_init_hw(dev_priv); > if (ret) > @@ -5386,7 +5388,8 @@ int i915_gem_init(struct drm_i915_private *dev_priv) > i915_gem_contexts_lost(dev_priv); > intel_uc_fini_hw(dev_priv); > err_uc_init: > - intel_uc_fini(dev_priv); > + if (ret != -EIO) > + intel_uc_fini(dev_priv); > err_pm: > if (ret != -EIO) { > intel_cleanup_gt_powersave(dev_priv); > @@ -5400,15 +5403,15 @@ int i915_gem_init(struct drm_i915_private *dev_priv) > intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); > mutex_unlock(&dev_priv->drm.struct_mutex); > > - intel_uc_fini_misc(dev_priv); > - > - if (ret != -EIO) > + if (ret != -EIO) { > + intel_uc_fini_misc(dev_priv); > i915_gem_cleanup_userptr(dev_priv); > + } > > if (ret == -EIO) { > /* > - * Allow engine initialisation to fail by marking the GPU as > - * wedged. But we only want to do this where the GPU is angry, > + * Allow engines or uC initialization to fail by marking the GPU > + * as wedged. But we only want to do this when the GPU is angry, > * for all other failure, such as an allocation failure, bail. > */ > if (!i915_terminally_wedged(&dev_priv->gpu_error)) { > diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h > index d878160..faa9e01 100644 > --- a/drivers/gpu/drm/i915/intel_guc.h > +++ b/drivers/gpu/drm/i915/intel_guc.h > @@ -139,4 +139,9 @@ static inline int intel_guc_sanitize(struct intel_guc *guc) > return 0; > } > > +static inline bool intel_guc_is_loaded(struct intel_guc *guc) > +{ > + return intel_uc_fw_is_loaded(&guc->fw); > +} > + > #endif > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 9d5ffd7..f89acf4 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -447,15 +447,20 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) > * Note that there is no fallback as either user explicitly asked for > * the GuC or driver default option was to run with the GuC enabled. > */ > - if (GEM_WARN_ON(ret == -EIO)) > - ret = -EINVAL; > - > dev_err(dev_priv->drm.dev, "GuC initialization failed %d\n", ret); > - return ret; > + > + /* Sanitize GuC/HuC to avoid clean-up on wedged */ > + intel_huc_sanitize(huc); > + intel_guc_sanitize(guc); > + GEM_BUG_ON(intel_guc_is_loaded(guc)); > + How about also resetting the hw using __intel_uc_reset_hw(). > + /* We want to disable GPU submission but keep KMS alive */ > + return -EIO; > } > > void intel_uc_fini_hw(struct drm_i915_private *dev_priv) > { > + struct intel_huc *huc = &dev_priv->huc; > struct intel_guc *guc = &dev_priv->guc; > > if (!USES_GUC(dev_priv)) > @@ -463,10 +468,15 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) > > GEM_BUG_ON(!HAS_GUC(dev_priv)); > > + if (!intel_guc_is_loaded(guc)) > + return; > + And it would be good to consolidate uc_fini_hw into uc_sanitize where uc_sanitize will look like: void intel_uc_sanitize(struct drm_i915_private *i915) { struct intel_guc *guc = &i915->guc; struct intel_huc *huc = &i915->huc; if (USES_GUC(i915)) return; GEM_BUG_ON(!HAS_GUC(i915)); if (!intel_guc_is_loaded(guc)) return; if (USES_GUC_SUBMISSION(i915)) intel_guc_submission_disable(guc); guc_disable_communication(guc); if (USES_GUC_SUBMISSION(i915)) gen9_disable_guc_interrupts(i915); intel_huc_sanitize(huc); intel_guc_sanitize(guc); __intel_uc_reset_hw(i915); } Then we can remove the call to intel_uc_fini_hw from i915_gem_fini. This will also allow us to release the doorbells properly (calling destroy_doorbell) as part of uc_sanitize. > if (USES_GUC_SUBMISSION(dev_priv)) > intel_guc_submission_disable(guc); > > guc_disable_communication(guc); > + intel_huc_sanitize(huc); > + intel_guc_sanitize(guc); > > if (USES_GUC_SUBMISSION(dev_priv)) > gen9_disable_guc_interrupts(dev_priv); > diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h > index 2601521..ff9ce9a 100644 > --- a/drivers/gpu/drm/i915/intel_uc_fw.h > +++ b/drivers/gpu/drm/i915/intel_uc_fw.h > @@ -121,6 +121,11 @@ static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) > uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING; > } > > +static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw) > +{ > + return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS; > +} > + > void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, > struct intel_uc_fw *uc_fw); > int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 05b0724..8eed87d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -5338,8 +5338,10 @@ int i915_gem_init(struct drm_i915_private *dev_priv) intel_init_gt_powersave(dev_priv); ret = intel_uc_init(dev_priv); - if (ret) + if (ret) { + GEM_BUG_ON(ret == -EIO); goto err_pm; + } ret = i915_gem_init_hw(dev_priv); if (ret) @@ -5386,7 +5388,8 @@ int i915_gem_init(struct drm_i915_private *dev_priv) i915_gem_contexts_lost(dev_priv); intel_uc_fini_hw(dev_priv); err_uc_init: - intel_uc_fini(dev_priv); + if (ret != -EIO) + intel_uc_fini(dev_priv); err_pm: if (ret != -EIO) { intel_cleanup_gt_powersave(dev_priv); @@ -5400,15 +5403,15 @@ int i915_gem_init(struct drm_i915_private *dev_priv) intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); mutex_unlock(&dev_priv->drm.struct_mutex); - intel_uc_fini_misc(dev_priv); - - if (ret != -EIO) + if (ret != -EIO) { + intel_uc_fini_misc(dev_priv); i915_gem_cleanup_userptr(dev_priv); + } if (ret == -EIO) { /* - * Allow engine initialisation to fail by marking the GPU as - * wedged. But we only want to do this where the GPU is angry, + * Allow engines or uC initialization to fail by marking the GPU + * as wedged. But we only want to do this when the GPU is angry, * for all other failure, such as an allocation failure, bail. */ if (!i915_terminally_wedged(&dev_priv->gpu_error)) { diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h index d878160..faa9e01 100644 --- a/drivers/gpu/drm/i915/intel_guc.h +++ b/drivers/gpu/drm/i915/intel_guc.h @@ -139,4 +139,9 @@ static inline int intel_guc_sanitize(struct intel_guc *guc) return 0; } +static inline bool intel_guc_is_loaded(struct intel_guc *guc) +{ + return intel_uc_fw_is_loaded(&guc->fw); +} + #endif diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 9d5ffd7..f89acf4 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -447,15 +447,20 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv) * Note that there is no fallback as either user explicitly asked for * the GuC or driver default option was to run with the GuC enabled. */ - if (GEM_WARN_ON(ret == -EIO)) - ret = -EINVAL; - dev_err(dev_priv->drm.dev, "GuC initialization failed %d\n", ret); - return ret; + + /* Sanitize GuC/HuC to avoid clean-up on wedged */ + intel_huc_sanitize(huc); + intel_guc_sanitize(guc); + GEM_BUG_ON(intel_guc_is_loaded(guc)); + + /* We want to disable GPU submission but keep KMS alive */ + return -EIO; } void intel_uc_fini_hw(struct drm_i915_private *dev_priv) { + struct intel_huc *huc = &dev_priv->huc; struct intel_guc *guc = &dev_priv->guc; if (!USES_GUC(dev_priv)) @@ -463,10 +468,15 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv) GEM_BUG_ON(!HAS_GUC(dev_priv)); + if (!intel_guc_is_loaded(guc)) + return; + if (USES_GUC_SUBMISSION(dev_priv)) intel_guc_submission_disable(guc); guc_disable_communication(guc); + intel_huc_sanitize(huc); + intel_guc_sanitize(guc); if (USES_GUC_SUBMISSION(dev_priv)) gen9_disable_guc_interrupts(dev_priv); diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h index 2601521..ff9ce9a 100644 --- a/drivers/gpu/drm/i915/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/intel_uc_fw.h @@ -121,6 +121,11 @@ static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw) uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING; } +static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw) +{ + return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS; +} + void intel_uc_fw_fetch(struct drm_i915_private *dev_priv, struct intel_uc_fw *uc_fw); int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
Since commit 6ca9a2beb54a ("drm/i915: Unwind i915_gem_init() failure") we believed that we correctly handle all errors encountered during GuC initialization, including special one that indicates request to run driver with disabled GPU submission (-EIO). Unfortunately since commit 121981fafe69 ("drm/i915/guc: Combine enable_guc_loading|submission modparams") we stopped using that error code to avoid unwanted fallback to execlist submission mode. In result any GuC initialization failure was treated as non-recoverable error leading to driver load abort, so we could not even read related GuC error log to investigate cause of the problem. Fix that by always returning -EIO on uC hardware related failure. v2: don't allow -EIO from uc_init don't call uc_fini[_misc] on -EIO mark guc fw as failed on hw init failure prepare uc_fini_hw to run after earlier -EIO v3: update comments (Sagar) use sanitize functions on failure in init_hw (Michal) and also sanitize guc/huc fw in fini_hw (Michal) Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michal Winiarski <michal.winiarski@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 17 ++++++++++------- drivers/gpu/drm/i915/intel_guc.h | 5 +++++ drivers/gpu/drm/i915/intel_uc.c | 18 ++++++++++++++---- drivers/gpu/drm/i915/intel_uc_fw.h | 5 +++++ 4 files changed, 34 insertions(+), 11 deletions(-)