[Intel-gfx] [PATCH] drm/i915/guc: Use correct error code for GuC initialization failure

[Chris Wilson]

From: Michal Wajdeczko <michal.wajdeczko at intel.com>

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

Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
Cc: Chris Wilson <chris at chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski at intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c    | 15 ++++++++-------
 drivers/gpu/drm/i915/intel_guc.h   |  5 +++++
 drivers/gpu/drm/i915/intel_uc.c    | 13 +++++++++----
 drivers/gpu/drm/i915/intel_uc_fw.h |  9 +++++++--
 4 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a4031fab57b0..139dfde0459e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5523,8 +5523,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)
@@ -5577,7 +5579,8 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	mutex_lock(&dev_priv->drm.struct_mutex);
 	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);
@@ -5592,11 +5595,6 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 
 err_uc_misc:
-	intel_uc_fini_misc(dev_priv);
-
-	if (ret != -EIO)
-		i915_gem_cleanup_userptr(dev_priv);
-
 	if (ret == -EIO) {
 		/*
 		 * Allow engine initialisation to fail by marking the GPU as
@@ -5609,6 +5607,9 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 			i915_gem_set_wedged(dev_priv);
 		}
 		ret = 0;
+	} else {
+		intel_uc_fini_misc(dev_priv);
+		i915_gem_cleanup_userptr(dev_priv);
 	}
 
 	i915_gem_drain_freed_objects(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 4121928a495e..31a620433d81 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -121,6 +121,11 @@ static inline void intel_guc_to_host_event_handler(struct intel_guc *guc)
 	guc->handler(guc);
 }
 
+static inline bool intel_guc_is_loaded(const struct intel_guc *guc)
+{
+	return intel_uc_fw_is_loaded(&guc->fw);
+}
+
 /* GuC addresses above GUC_GGTT_TOP also don't map through the GTT */
 #define GUC_GGTT_TOP	0xFEE00000
 
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 7c95697e1a35..161d66edb174 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -424,11 +424,13 @@ int intel_uc_init_hw(struct drm_i915_private *i915)
 	 * 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(i915->drm.dev, "GuC initialization failed %d\n", ret);
-	return ret;
+
+	/* Mark GuC firmware as failed to avoid redundant clean-up */
+	guc->fw.load_status = INTEL_UC_FIRMWARE_FAIL;
+
+	/* We want to disable GPU submission but keep KMS alive */
+	return -EIO;
 }
 
 void intel_uc_fini_hw(struct drm_i915_private *i915)
@@ -440,6 +442,9 @@ void intel_uc_fini_hw(struct drm_i915_private *i915)
 
 	GEM_BUG_ON(!HAS_GUC(i915));
 
+	if (!intel_guc_is_loaded(guc))
+		return;
+
 	if (USES_GUC_SUBMISSION(i915))
 		intel_guc_submission_disable(guc);
 
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
index 87910aa83267..71a0f9800f4d 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -115,9 +115,14 @@ static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw)
 	return uc_fw->path != NULL;
 }
 
+static inline bool intel_uc_fw_is_loaded(const struct intel_uc_fw *uc_fw)
+{
+	return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS;
+}
+
 static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
 {
-	if (uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
+	if (intel_uc_fw_is_loaded(uc_fw))
 		uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
 }
 
@@ -131,7 +136,7 @@ static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
  */
 static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
 {
-	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (!intel_uc_fw_is_loaded(uc_fw))
 		return 0;
 
 	return uc_fw->header_size + uc_fw->ucode_size;
-- 
2.18.0