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.wajdec...@intel.com>
Cc: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiar...@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kam...@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));
+
+       /* 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,
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to