Re: [PATCH 4/6] drm/i915/uc/gsc: query the GSC FW for its compatibility version

From: "Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>
To: "Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Cc: "Harrison, John C" <john.c.harrison@intel.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 4/6] drm/i915/uc/gsc: query the GSC FW for its compatibility version
Date: Thu, 25 May 2023 23:29:39 +0000	[thread overview]
Message-ID: <f34f960c06971bc784fdfb1cf2f9e067bddacf47.camel@intel.com> (raw)
In-Reply-To: <20230505160415.889525-5-daniele.ceraolospurio@intel.com>

On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote:
> The compatibility version is queried via an MKHI command. Right now, the
> only existing interface is 1.0
> This is basically the interface version for the GSC FW, so the plan is
> to use it as the main tracked version, including for the binary naming
> in the fetch code.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 

alan: just a couple of minor things nits below.
One ask though, in line with the clarification we had over the offline coversation,
I am wondering if we can document the fact that the file_selected.ver remains as major-minor::zero-zero
for the case of gsc until after the firmware is loaded and we query via this function (which happens
later at gt-late-init). However, that comment might not belong here - perhaps it belongs in the prior
patch together with the other comment i requested for (asking for additional explainations about the
different types of versions for gsc).

That said, for this patch, LGTM:
Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>

alan:snip
> +static int gsc_fw_query_compatibility_version(struct intel_gsc_uc *gsc)
> +{
> +	struct intel_gt *gt = gsc_uc_to_gt(gsc);
> +	struct mtl_gsc_ver_msg_in *msg_in;
> +	struct mtl_gsc_ver_msg_out *msg_out;
> +	struct i915_vma *vma;
> +	u64 offset;
> +	void *vaddr;
> +	int err;
> +
> +	err = intel_guc_allocate_and_map_vma(&gt->uc.guc, PAGE_SIZE * 2,
> +					     &vma, &vaddr);
alan: nit: im assuming this code will be used for future discrete cards,.. if so,
perhaps we should also be using "SZ_4K * 2" above since different host-cpu-arch
could have different PAGE sizes - this way we'll be consistent with exact size allocations.
also its more consistent in this function - maybe a #define GSC_UC_GET_ABI_VER_PKT_SIZE SZ_4K
at top of function is nice. either way, i consider this a nit.

> +	if (err) {
> +		gt_err(gt, "failed to allocate vma for GSC version query\n");
> +		return err;
> +	}

alan:snip

> +
>  int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc)
>  {
>  	struct intel_gt *gt = gsc_uc_to_gt(gsc);
> @@ -327,11 +406,21 @@ int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc)
>  	if (err)
>  		goto fail;
>  
> +	err = gsc_fw_query_compatibility_version(gsc);
> +	if (err)
> +		goto fail;
> +
> +	/* we only support compatibility version 1.0 at the moment */
> +	err = intel_uc_check_file_version(gsc_fw, NULL);
> +	if (err)
> +		goto fail;
> +
>  	/* FW is not fully operational until we enable SW proxy */
>  	intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
>  
> -	gt_info(gt, "Loaded GSC firmware %s (r%u.%u.%u.%u, svn%u)\n",
> +	gt_info(gt, "Loaded GSC firmware %s (cv%u.%u, r%u.%u.%u.%u, svn %u)\n",
alan:nit "abi" instead of "cv"?
>  		gsc_fw->file_selected.path,
> +		gsc_fw->file_selected.ver.major, gsc_fw->file_selected.ver.minor,
>  		gsc->release.major, gsc->release.minor,
>  		gsc->release.patch, gsc->release.build,
>  		gsc->security_version);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> index 8f199d5f963e..fb1453ed4ecf 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
alan:snip

> 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 cd8fc194f7fa..36ee96c02d74 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
alan:snip

> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> index 279244744d43..4406e7b48b27 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
alan:snip