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@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tony Ye <tony.ye@xxxxxxxxx>
> ---
> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
