Re: [Intel-gfx] [PATCH 1/4] drm/i915/huc: check HW directly for HuC auth status

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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@intel.com>
> ---
>  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 = &gt->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@intel.com>



> +	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
>