Re: [PATCH v3 1/4] drm/i915/uc: Use correct error code for GuC initialization failure

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

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,

-- 
Thanks,
Sagar

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