[PATCH v4 4/7] drm/i915/uc: Use correct error code for GuC initialization failure
Michal Wajdeczko
michal.wajdeczko at intel.com
Sat Mar 24 07:09:24 UTC 2018
On Fri, 23 Mar 2018 22:43:20 +0100, Anusha Srivatsa
<anusha.srivatsa at intel.com> wrote:
> 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);
>> +}
>
> Why not use intel_uc_fw_is_loaded() instead? In that case both huc and
> guc
> load can be using intel_uc_fw_is_loaded....
Are you asking why uc is not using uc_fw_is_loaded ?
I wanted to avoid reading guc internals out of guc.h|c files
These calls are nicely inlined by compiler, so no cost at all
/m
ps. reminder - this is try-bot thread
>
> Anusha
>> #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));
>> +
>> + /* 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
More information about the Intel-gfx-trybot
mailing list