[Intel-gfx] [PATCH v5 5/8] drm/i915/uc: Use correct error code for GuC initialization failure

[Sagar Arun Kamble]

On 3/28/2018 2:05 AM, 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>
<snip>
>   void intel_uc_fini_hw(struct drm_i915_private *i915)
>   {
> +	struct intel_guc *guc = &i915->guc;
> +
> +	if (!intel_guc_is_loaded(guc))
> +		return;
I feel above guc_is_loaded check is more applicable in uc_sanitize. So 
callers won't have to bother if GuC is loaded or not.
w/ or w/o that change patch looks good though.
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> +
>   	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.

-- 
Thanks,
Sagar