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

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx>]

On 4/26/2022 5:26 PM, 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 = &gt->uc.guc;
>   	int ret;
>   
> -	GEM_BUG_ON(intel_huc_is_authenticated(huc));
> +	GEM_BUG_ON(huc_is_authenticated(huc));

It looks like even on gen9 HuC is surviving the reset, so this BUG_ON is 
now being triggered. I'm going to just change this to a BUG_ON on 
intel_uc_fw_is_running() for now, which would be equivalent to what we 
have right now, and in the meantime I'll follow up with the HuC team to 
see if we can just skip this (and the huc_fw_upload) if HuC shows up as 
already authenticated.

Daniele

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