On Tue, Apr 26, 2022 at 05:26:14PM -0700, Daniele Ceraolo Spurio wrote:
> The huc_is_authenticated function return is based on our SW tracking of
> the HuC auth status. However, around suspend/resume and reset this can
> go out of sync with the actual HW state, which is why we use
> huc_check_state() to look at the actual HW state. Instead of having this
> duality, just make huc_is_authenticated() return the HW state and use it
> everywhere we need to know if HuC is running.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_huc.c | 23 ++++++++++++++---------
> drivers/gpu/drm/i915/gt/uc/intel_huc.h | 5 -----
> 2 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index 556829de9c172..773020e69589a 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -80,6 +80,18 @@ void intel_huc_fini(struct intel_huc *huc)
> intel_uc_fw_fini(&huc->fw);
> }
>
> +static bool huc_is_authenticated(struct intel_huc *huc)
> +{
> + struct intel_gt *gt = huc_to_gt(huc);
> + intel_wakeref_t wakeref;
> + u32 status = 0;
> +
> + with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> + status = intel_uncore_read(gt->uncore, huc->status.reg);
> +
> + return (status & huc->status.mask) == huc->status.value;
> +}
> +
> /**
> * intel_huc_auth() - Authenticate HuC uCode
> * @huc: intel_huc structure
> @@ -96,7 +108,7 @@ int intel_huc_auth(struct intel_huc *huc)
> struct intel_guc *guc = >->uc.guc;
> int ret;
>
> - GEM_BUG_ON(intel_huc_is_authenticated(huc));
> + GEM_BUG_ON(huc_is_authenticated(huc));
>
> if (!intel_uc_fw_is_loaded(&huc->fw))
> return -ENOEXEC;
> @@ -150,10 +162,6 @@ int intel_huc_auth(struct intel_huc *huc)
> */
> int intel_huc_check_status(struct intel_huc *huc)
> {
> - struct intel_gt *gt = huc_to_gt(huc);
> - intel_wakeref_t wakeref;
> - u32 status = 0;
> -
> switch (__intel_uc_fw_status(&huc->fw)) {
> case INTEL_UC_FIRMWARE_NOT_SUPPORTED:
> return -ENODEV;
> @@ -167,10 +175,7 @@ int intel_huc_check_status(struct intel_huc *huc)
> break;
> }
>
> - with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> - status = intel_uncore_read(gt->uncore, huc->status.reg);
> -
> - return (status & huc->status.mask) == huc->status.value;
oh, these variable names look so generic, while it looks like the only usage
for them is for mask = loaded and value = loaded...
But anyway it is better this indirection with some generic name than duplicating
the definition depending on platform in here...
so:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> + return huc_is_authenticated(huc);
> }
>
> /**
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> index 73ec670800f2b..77d813840d76c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> @@ -50,11 +50,6 @@ static inline bool intel_huc_is_used(struct intel_huc *huc)
> return intel_uc_fw_is_available(&huc->fw);
> }
>
> -static inline bool intel_huc_is_authenticated(struct intel_huc *huc)
> -{
> - return intel_uc_fw_is_running(&huc->fw);
> -}
> -
> void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p);
>
> #endif
> --
> 2.25.1
>
