diff mbox series

[4/7] drm/i915/uc: Abort early on uc_init failure

Message ID 20200115013143.34961-5-daniele.ceraolospurio@intel.com (mailing list archive)
State New, archived
Headers show
Series Commit early to GuC | expand

Commit Message

Daniele Ceraolo Spurio Jan. 15, 2020, 1:31 a.m. UTC 
Now that we can differentiate wants vs uses GuC/HuC, intel_uc_init is
restricted to running only if we have successfully fetched the required
blob(s) and are committed to using the microcontroller(s).
The only remaining thing that can go wrong in uc_init is the allocation
of GuC/HuC related objects; if we get such a failure better to bail out
immediately instead of wedging later, like we do for e.g.
intel_engines_init, since without objects we can't use the HW, including
not being able to attempt the firmware load.

While at it, remove the unneeded fw_cleanup call (this is handled
outside of gt_init) and add a probe failure injection point for testing.
Also, update the logs for <g/h>uc_init failures to probe_failure() since
they will cause the driver load to fail.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt.c     |  4 +++-
 drivers/gpu/drm/i915/gt/uc/intel_guc.c |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_huc.c |  2 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc.c  | 24 +++++++++++++++++-------
 drivers/gpu/drm/i915/gt/uc/intel_uc.h  |  4 ++--
 5 files changed, 24 insertions(+), 12 deletions(-)

Comments

Fernando Pacheco Jan. 27, 2020, 11:57 p.m. UTC | #1 
On 1/14/20 5:31 PM, Daniele Ceraolo Spurio wrote:
> Now that we can differentiate wants vs uses GuC/HuC, intel_uc_init is
> restricted to running only if we have successfully fetched the required
> blob(s) and are committed to using the microcontroller(s).
> The only remaining thing that can go wrong in uc_init is the allocation
> of GuC/HuC related objects; if we get such a failure better to bail out
> immediately instead of wedging later, like we do for e.g.
> intel_engines_init, since without objects we can't use the HW, including
> not being able to attempt the firmware load.
>
> While at it, remove the unneeded fw_cleanup call (this is handled
> outside of gt_init) and add a probe failure injection point for testing.
> Also, update the logs for <g/h>uc_init failures to probe_failure() since
> they will cause the driver load to fail.

Reviewed-by: Fernando Pacheco <fernando.pacheco@intel.com>

>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c     |  4 +++-
>  drivers/gpu/drm/i915/gt/uc/intel_guc.c |  2 +-
>  drivers/gpu/drm/i915/gt/uc/intel_huc.c |  2 +-
>  drivers/gpu/drm/i915/gt/uc/intel_uc.c  | 24 +++++++++++++++++-------
>  drivers/gpu/drm/i915/gt/uc/intel_uc.h  |  4 ++--
>  5 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index da2b6e2ae692..85f21f18c785 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -584,7 +584,9 @@ int intel_gt_init(struct intel_gt *gt)
>  	if (err)
>  		goto err_engines;
>  
> -	intel_uc_init(&gt->uc);
> +	err = intel_uc_init(&gt->uc);
> +	if (err)
> +		goto err_engines;
>  
>  	err = intel_gt_resume(gt);
>  	if (err)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> index 5d00a3b2d914..c46f5ae77348 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
> @@ -376,7 +376,7 @@ int intel_guc_init(struct intel_guc *guc)
>  	intel_uc_fw_fini(&guc->fw);
>  err_fetch:
>  	intel_uc_fw_cleanup_fetch(&guc->fw);
> -	DRM_DEV_DEBUG_DRIVER(gt->i915->drm.dev, "failed with %d\n", ret);
> +	i915_probe_error(gt->i915, "failed with %d\n", ret);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index 32a069841c14..5f448d0e360b 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -127,7 +127,7 @@ int intel_huc_init(struct intel_huc *huc)
>  	intel_uc_fw_fini(&huc->fw);
>  out:
>  	intel_uc_fw_cleanup_fetch(&huc->fw);
> -	DRM_DEV_DEBUG_DRIVER(i915->drm.dev, "failed with %d\n", err);
> +	i915_probe_error(i915, "failed with %d\n", err);
>  	return err;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 8843d4f16a7f..d57b731952ef 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -273,7 +273,7 @@ static void __uc_cleanup_firmwares(struct intel_uc *uc)
>  	intel_uc_fw_cleanup_fetch(&uc->guc.fw);
>  }
>  
> -static void __uc_init(struct intel_uc *uc)
> +static int __uc_init(struct intel_uc *uc)
>  {
>  	struct intel_guc *guc = &uc->guc;
>  	struct intel_huc *huc = &uc->huc;
> @@ -282,19 +282,29 @@ static void __uc_init(struct intel_uc *uc)
>  	GEM_BUG_ON(!intel_uc_wants_guc(uc));
>  
>  	if (!intel_uc_uses_guc(uc))
> -		return;
> +		return 0;
> +
> +	if (i915_inject_probe_failure(uc_to_gt(uc)->i915))
> +		return -ENOMEM;
>  
>  	/* XXX: GuC submission is unavailable for now */
>  	GEM_BUG_ON(intel_uc_supports_guc_submission(uc));
>  
>  	ret = intel_guc_init(guc);
> -	if (ret) {
> -		intel_uc_fw_cleanup_fetch(&huc->fw);
> -		return;
> +	if (ret)
> +		return ret;
> +
> +	if (intel_uc_uses_huc(uc)) {
> +		ret = intel_huc_init(huc);
> +		if (ret)
> +			goto out_guc;
>  	}
>  
> -	if (intel_uc_uses_huc(uc))
> -		intel_huc_init(huc);
> +	return 0;
> +
> +out_guc:
> +	intel_guc_fini(guc);
> +	return ret;
>  }
>  
>  static void __uc_fini(struct intel_uc *uc)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> index f2f7351ff22a..2d9f17196761 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> @@ -16,7 +16,7 @@ struct intel_uc_ops {
>  	int (*sanitize)(struct intel_uc *uc);
>  	void (*init_fw)(struct intel_uc *uc);
>  	void (*fini_fw)(struct intel_uc *uc);
> -	void (*init)(struct intel_uc *uc);
> +	int (*init)(struct intel_uc *uc);
>  	void (*fini)(struct intel_uc *uc);
>  	int (*init_hw)(struct intel_uc *uc);
>  	void (*fini_hw)(struct intel_uc *uc);
> @@ -98,7 +98,7 @@ static inline _TYPE intel_uc_##_NAME(struct intel_uc *uc) \
>  intel_uc_ops_function(sanitize, sanitize, int, 0);
>  intel_uc_ops_function(fetch_firmwares, init_fw, void, );
>  intel_uc_ops_function(cleanup_firmwares, fini_fw, void, );
> -intel_uc_ops_function(init, init, void, );
> +intel_uc_ops_function(init, init, int, 0);
>  intel_uc_ops_function(fini, fini, void, );
>  intel_uc_ops_function(init_hw, init_hw, int, 0);
>  intel_uc_ops_function(fini_hw, fini_hw, void, );
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index da2b6e2ae692..85f21f18c785 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -584,7 +584,9 @@  int intel_gt_init(struct intel_gt *gt)
 	if (err)
 		goto err_engines;
 
-	intel_uc_init(&gt->uc);
+	err = intel_uc_init(&gt->uc);
+	if (err)
+		goto err_engines;
 
 	err = intel_gt_resume(gt);
 	if (err)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
index 5d00a3b2d914..c46f5ae77348 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
@@ -376,7 +376,7 @@  int intel_guc_init(struct intel_guc *guc)
 	intel_uc_fw_fini(&guc->fw);
 err_fetch:
 	intel_uc_fw_cleanup_fetch(&guc->fw);
-	DRM_DEV_DEBUG_DRIVER(gt->i915->drm.dev, "failed with %d\n", ret);
+	i915_probe_error(gt->i915, "failed with %d\n", ret);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
index 32a069841c14..5f448d0e360b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
@@ -127,7 +127,7 @@  int intel_huc_init(struct intel_huc *huc)
 	intel_uc_fw_fini(&huc->fw);
 out:
 	intel_uc_fw_cleanup_fetch(&huc->fw);
-	DRM_DEV_DEBUG_DRIVER(i915->drm.dev, "failed with %d\n", err);
+	i915_probe_error(i915, "failed with %d\n", err);
 	return err;
 }
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 8843d4f16a7f..d57b731952ef 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -273,7 +273,7 @@  static void __uc_cleanup_firmwares(struct intel_uc *uc)
 	intel_uc_fw_cleanup_fetch(&uc->guc.fw);
 }
 
-static void __uc_init(struct intel_uc *uc)
+static int __uc_init(struct intel_uc *uc)
 {
 	struct intel_guc *guc = &uc->guc;
 	struct intel_huc *huc = &uc->huc;
@@ -282,19 +282,29 @@  static void __uc_init(struct intel_uc *uc)
 	GEM_BUG_ON(!intel_uc_wants_guc(uc));
 
 	if (!intel_uc_uses_guc(uc))
-		return;
+		return 0;
+
+	if (i915_inject_probe_failure(uc_to_gt(uc)->i915))
+		return -ENOMEM;
 
 	/* XXX: GuC submission is unavailable for now */
 	GEM_BUG_ON(intel_uc_supports_guc_submission(uc));
 
 	ret = intel_guc_init(guc);
-	if (ret) {
-		intel_uc_fw_cleanup_fetch(&huc->fw);
-		return;
+	if (ret)
+		return ret;
+
+	if (intel_uc_uses_huc(uc)) {
+		ret = intel_huc_init(huc);
+		if (ret)
+			goto out_guc;
 	}
 
-	if (intel_uc_uses_huc(uc))
-		intel_huc_init(huc);
+	return 0;
+
+out_guc:
+	intel_guc_fini(guc);
+	return ret;
 }
 
 static void __uc_fini(struct intel_uc *uc)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index f2f7351ff22a..2d9f17196761 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -16,7 +16,7 @@  struct intel_uc_ops {
 	int (*sanitize)(struct intel_uc *uc);
 	void (*init_fw)(struct intel_uc *uc);
 	void (*fini_fw)(struct intel_uc *uc);
-	void (*init)(struct intel_uc *uc);
+	int (*init)(struct intel_uc *uc);
 	void (*fini)(struct intel_uc *uc);
 	int (*init_hw)(struct intel_uc *uc);
 	void (*fini_hw)(struct intel_uc *uc);
@@ -98,7 +98,7 @@  static inline _TYPE intel_uc_##_NAME(struct intel_uc *uc) \
 intel_uc_ops_function(sanitize, sanitize, int, 0);
 intel_uc_ops_function(fetch_firmwares, init_fw, void, );
 intel_uc_ops_function(cleanup_firmwares, fini_fw, void, );
-intel_uc_ops_function(init, init, void, );
+intel_uc_ops_function(init, init, int, 0);
 intel_uc_ops_function(fini, fini, void, );
 intel_uc_ops_function(init_hw, init_hw, int, 0);
 intel_uc_ops_function(fini_hw, fini_hw, void, );