Re: [Intel-gfx] [PATCH] drm/i915/huc: check HuC and GuC version compatibility on MTL

[Andrzej Hajda <andrzej.hajda@xxxxxxxxx>]

On 11.07.2023 22:31, Daniele Ceraolo Spurio wrote:
> Due to a change in the auth flow on MTL, GuC 70.7.0 and newer will only
> be able to authenticate HuC 8.5.1 and newer. The plan is to update the 2
> binaries sinchronously in linux-firmware so that the fw repo always has
> a matching pair that works; still, it's better to check in the kernel so
> we can print an error message and abort HuC loading if the binaries are
> out of sync instead of failing the authentication.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: John Harrison <John.C.Harrison@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 42 ++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index 08e16017584b..f0cc5bb47fa0 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -803,11 +803,53 @@ static int try_firmware_load(struct intel_uc_fw *uc_fw, const struct firmware **
>   	return 0;
>   }
>   
> +static int check_mtl_huc_guc_compatibility(struct intel_gt *gt,
> +					   struct intel_uc_fw_file *huc_selected)
> +{
> +	struct intel_uc_fw_file *guc_selected = &gt->uc.guc.fw.file_selected;
> +	struct intel_uc_fw_ver *huc_ver = &huc_selected->ver;
> +	struct intel_uc_fw_ver *guc_ver = &guc_selected->ver;
> +	bool new_huc;
> +	bool new_guc;
> +
> +	/* we can only do this check after having fetched both GuC and HuC */
> +	GEM_BUG_ON(!huc_selected->path || !guc_selected->path);
> +
> +	/*
> +	 * Due to changes in the authentication flow for MTL, HuC 8.5.1 or newer
> +	 * requires GuC 70.7.0 or newer. Older HuC binaries will instead require
> +	 * GuC < 70.7.0.
> +	 */
> +	new_huc = huc_ver->major > 8 ||
> +		  (huc_ver->major == 8 && huc_ver->minor > 5) ||
> +		  (huc_ver->major == 8 && huc_ver->minor == 5 && huc_ver->patch >= 1);
> +
> +	new_guc = guc_ver->major > 70 ||
> +		  (guc_ver->major == 70 && guc_ver->minor >= 7);

Wouldn't be more readable to define sth like UC_VER_FULL(v)
then use UC_VER_FULL(huc_ver) >= IP_VER_FULL(8, 5, 1).
I am not sure if it is worth for two checks.


> +
> +	if (new_huc != new_guc) {
> +		UNEXPECTED(gt, "HuC %u.%u.%u is incompatible with GuC %u.%u.%u\n",
> +			   huc_ver->major, huc_ver->minor, huc_ver->patch,
> +			   guc_ver->major, guc_ver->minor, guc_ver->patch);
> +		gt_info(gt, "MTL GuC 70.7.0+ and HuC 8.5.1+ don't work with older releases\n");
> +		return -ENOEXEC;
> +	}
> +
> +	return 0;
> +}
> +
>   int intel_uc_check_file_version(struct intel_uc_fw *uc_fw, bool *old_ver)
>   {
>   	struct intel_gt *gt = __uc_fw_to_gt(uc_fw);
>   	struct intel_uc_fw_file *wanted = &uc_fw->file_wanted;
>   	struct intel_uc_fw_file *selected = &uc_fw->file_selected;
> +	int ret;
> +
> +	if (IS_METEORLAKE(gt->i915) && uc_fw->type == INTEL_UC_FW_TYPE_HUC) {

Moving this check inside check function would make it more generic, up 
to you.

Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>

Regards
Andrzej


> +		ret = check_mtl_huc_guc_compatibility(gt, selected);
> +		if (ret)
> +			return ret;
> +	}
>   
>   	if (!wanted->ver.major || !selected->ver.major)
>   		return 0;