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@xxxxxxxxx>
>
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@xxxxxxxxx>
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(>->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
