[Intel-gfx] [PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-19 22:50:43)
> If we never attempted to load HuC firmware, or we already wedged
> or reset GuC/HuC, then there is no reason to wake up the device
> to check one bit in the register that will be for sure cleared.
> 
> v2: check if HuC was enabled; subtle change in ABI
>     reuse hus_is_load helper
> 
> Suggested-by: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tony Ye <tony.ye at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_huc.c | 20 ++++++++++++--------
>  drivers/gpu/drm/i915/intel_huc.h |  5 +++++
>  2 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 1ff1fb015e58..bfdebec1cfc8 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -113,7 +113,7 @@ int intel_huc_auth(struct intel_huc *huc)
>         u32 status;
>         int ret;
>  
> -       if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> +       if (!intel_huc_is_loaded(huc))
>                 return -ENOEXEC;
>  
>         ret = intel_guc_auth_huc(guc,
> @@ -150,21 +150,25 @@ int intel_huc_auth(struct intel_huc *huc)
>   * This function reads status register to verify if HuC
>   * firmware was successfully loaded.
>   *
> - * Returns: 1 if HuC firmware is loaded and verified,
> - * 0 if HuC firmware is not loaded and -ENODEV if HuC
> - * is not present on this platform.
> + * Returns: 1 if HuC firmware is loaded and verified, 0 if HuC firmware is not
> + * enabled, -ENOPKG if HuC firmware is not loaded and -ENODEV if HuC is not
> + * present on this platform.
>   */
>  int intel_huc_check_status(struct intel_huc *huc)
>  {
>         struct drm_i915_private *dev_priv = huc_to_i915(huc);
>         intel_wakeref_t wakeref;
> -       bool status = false;
> +       bool verified = false;
>  
>         if (!HAS_HUC(dev_priv))
>                 return -ENODEV;
>  
> -       with_intel_runtime_pm(dev_priv, wakeref)
> -               status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
> +       if (!USES_HUC(dev_priv))
> +               return 0;

Hmm, EOPNOTSUPP here for the user disabled case?

Then

if (!intel_huc_is_loaded(huc))
	return -ENOPKG;

bool verified = false;
with_intel_runtime_pm(dev_priv, wakeref)
	verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
return verified.

That keeps it a bit flatter with the special casing separate and the 0/1
result as given by the register status. Then if negative, we know one of
the preconditions for using HuC is not available, and if 0 something went
wrong in mmio setup.

Does that make sense?

> -       return status;
> +       if (intel_huc_is_loaded(huc))
> +               with_intel_runtime_pm(dev_priv, wakeref)
> +                       verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
> +
> +       return verified ? 1 : -ENOPKG;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
> index a0c21ae02a99..cde3d303718d 100644
> --- a/drivers/gpu/drm/i915/intel_huc.h
> +++ b/drivers/gpu/drm/i915/intel_huc.h
> @@ -44,6 +44,11 @@ void intel_huc_fini(struct intel_huc *huc);
>  int intel_huc_auth(struct intel_huc *huc);
>  int intel_huc_check_status(struct intel_huc *huc);
>  
> +static inline bool intel_huc_is_loaded(struct intel_huc *huc)
> +{
> +       return intel_uc_fw_is_loaded(&huc->fw);
> +}
> +
>  static inline void intel_huc_fini_misc(struct intel_huc *huc)
>  {
>         intel_uc_fw_cleanup_fetch(&huc->fw);
> -- 
> 2.19.2
>