Re: [Intel-gfx] [PATCH v5 3/7] drm/i915/huc: Load GSC-enabled HuC via DMA xfer if the fuse says so

[John Harrison <john.c.harrison@intel.com>]

On 5/31/2023 16:54, Daniele Ceraolo Spurio wrote:
> In the previous patch we extracted the offset of the legacy-style HuC
> binary located within the GSC-enabled blob, so now we can use that to
> load the HuC via DMA if the fuse is set that way.
> Note that we now need to differentiate between "GSC-enabled binary" and
> "loaded by GSC", so the former case has been renamed to "has GSC headers"
> for clarity, while the latter is now based on the fuse instead of the
> binary format. This way, all the legacy load paths are automatically
> taken (including the auth by GuC) without having to implement further
> code changes.
>
> v2: s/is_meu_binary/has_gsc_headers/, clearer logs (John)
>
> v3: split check for GSC access, better comments (John)
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_huc.c    | 49 +++++++++++++++++------
>   drivers/gpu/drm/i915/gt/uc/intel_huc.h    |  4 +-
>   drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c |  2 +-
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  | 12 +++---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  |  2 +-
>   5 files changed, 47 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index 6d795438b3e4..27c5e41fa84c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -298,31 +298,54 @@ void intel_huc_init_early(struct intel_huc *huc)
>   static int check_huc_loading_mode(struct intel_huc *huc)
>   {
>   	struct intel_gt *gt = huc_to_gt(huc);
> -	bool fw_needs_gsc = intel_huc_is_loaded_by_gsc(huc);
> -	bool hw_uses_gsc = false;
> +	bool gsc_enabled = huc->fw.has_gsc_headers;
>   
>   	/*
>   	 * The fuse for HuC load via GSC is only valid on platforms that have
>   	 * GuC deprivilege.
>   	 */
>   	if (HAS_GUC_DEPRIVILEGE(gt->i915))
> -		hw_uses_gsc = intel_uncore_read(gt->uncore, GUC_SHIM_CONTROL2) &
> -			      GSC_LOADS_HUC;
> +		huc->loaded_via_gsc = intel_uncore_read(gt->uncore, GUC_SHIM_CONTROL2) &
> +				      GSC_LOADS_HUC;
>   
> -	if (fw_needs_gsc != hw_uses_gsc) {
> -		huc_err(huc, "mismatch between FW (%s) and HW (%s) load modes\n",
> -			HUC_LOAD_MODE_STRING(fw_needs_gsc), HUC_LOAD_MODE_STRING(hw_uses_gsc));
> +	if (huc->loaded_via_gsc && !gsc_enabled) {
> +		huc_err(huc, "HW requires a GSC-enabled blob, but we found a legacy one\n");
>   		return -ENOEXEC;
>   	}
>   
> -	/* make sure we can access the GSC via the mei driver if we need it */
> -	if (!(IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_INTEL_MEI_GSC)) &&
> -	    fw_needs_gsc) {
> -		huc_info(huc, "can't load due to missing MEI modules\n");
> -		return -EIO;
> +	/*
> +	 * On newer platforms we have GSC-enabled binaries but we load the HuC
> +	 * via DMA. To do so we need to find the location of the legacy-style
> +	 * binary inside the GSC-enabled one, which we do at fetch time. Make
> +	 * sure that we were able to do so if the fuse says we need to load via
> +	 * DMA and the binary is GSC-enabled.
> +	 */
> +	if (!huc->loaded_via_gsc && gsc_enabled && !huc->fw.dma_start_offset) {
> +		huc_err(huc, "HW in DMA mode, but we have an incompatible GSC-enabled blob\n");
> +		return -ENOEXEC;
> +	}
> +
> +	/*
> +	 * If the HuC is loaded via GSC, we need to be able to access the GSC.
> +	 * On DG2 this is done via the mei components, while on newer platforms
> +	 * it is done via the GSCCS,
> +	 */
> +	if (huc->loaded_via_gsc) {
> +		if (IS_DG2(gt->i915)) {
> +			if (!IS_ENABLED(CONFIG_INTEL_MEI_PXP) ||
> +			    !IS_ENABLED(CONFIG_INTEL_MEI_GSC)) {
> +				huc_info(huc, "can't load due to missing mei modules\n");
> +				return -EIO;
> +			}
> +		} else {
> +			if (!HAS_ENGINE(gt, GSC0)){
Checkpatch is complaining about lack of a space here. Maybe fix on merge 
rather than repost if that is the only issue?

John.