[PATCH v4 4/7] drm/i915/uc: Use correct error code for GuC initialization failure
Anusha Srivatsa
anusha.srivatsa at intel.com
Fri Mar 23 21:30:20 UTC 2018
On Wed, Mar 21, 2018 at 05:32:59PM +0000, 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)
>
> v4: rebase
>
> 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>
>
> Conflicts:
> drivers/gpu/drm/i915/intel_uc.c
> ---
> drivers/gpu/drm/i915/i915_gem.c | 17 ++++++++++-------
> drivers/gpu/drm/i915/intel_guc.h | 5 +++++
> drivers/gpu/drm/i915/intel_uc.c | 17 +++++++++++++----
> drivers/gpu/drm/i915/intel_uc_fw.h | 5 +++++
> 4 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7fad180..9b13b3e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5361,8 +5361,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)
> @@ -5409,7 +5411,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);
> @@ -5423,15 +5426,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 9a95d15..669d42c 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -155,4 +155,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 d1feb85..b7291ea 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -427,15 +427,24 @@ 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));
Little confused here... do we need this check here?
Anusha
> + /* We want to disable GPU submission but keep KMS alive */
> + return -EIO;
> }
>
> void intel_uc_fini_hw(struct drm_i915_private *i915)
> {
> + struct intel_guc *guc = &i915->guc;
> +
> + if (!intel_guc_is_loaded(guc))
> + return;
> +
> intel_uc_sanitize(i915);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
> index dc33b12..77ad2aa 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;
> +}
> +
> /**
> * intel_uc_fw_get_upload_size() - Get size of firmware needed to be uploaded.
> * @uc_fw: uC firmware.
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx-trybot mailing list
> Intel-gfx-trybot at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx-trybot
--
Anusha Srivatsa
More information about the Intel-gfx-trybot
mailing list