diff mbox series

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

Message ID 20230505160415.889525-5-daniele.ceraolospurio@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: GSC FW support for MTL | expand

Commit Message

Daniele Ceraolo Spurio May 5, 2023, 4:04 p.m. UTC
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>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c     | 91 ++++++++++++++++++-
 .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h |  1 +
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c      | 44 +++++----
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h      |  1 +
 4 files changed, 120 insertions(+), 17 deletions(-)

Comments

Teres Alexis, Alan Previn May 25, 2023, 11:29 p.m. UTC | #1
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
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
index df53c13e99a3..0b6dcd982b14 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
@@ -11,6 +11,7 @@ 
 #include "gt/intel_ring.h"
 #include "intel_gsc_fw.h"
 #include "intel_gsc_meu_headers.h"
+#include "intel_gsc_uc_heci_cmd_submit.h"
 
 #define GSC_FW_STATUS_REG			_MMIO(0x116C40)
 #define GSC_FW_CURRENT_STATE			REG_GENMASK(3, 0)
@@ -270,6 +271,84 @@  static int gsc_fw_wait(struct intel_gt *gt)
 				       500);
 }
 
+struct intel_gsc_mkhi_header {
+	u8  group_id;
+#define MKHI_GROUP_ID_GFX_SRV 0x30
+
+	u8  command;
+#define MKHI_GFX_SRV_GET_HOST_COMPATIBILITY_VERSION (0x42)
+
+	u8  reserved;
+	u8  result;
+} __packed;
+
+struct mtl_gsc_ver_msg_in {
+	struct intel_gsc_mtl_header header;
+	struct intel_gsc_mkhi_header mkhi;
+} __packed;
+
+struct mtl_gsc_ver_msg_out {
+	struct intel_gsc_mtl_header header;
+	struct intel_gsc_mkhi_header mkhi;
+	u16 proj_major;
+	u16 compat_major;
+	u16 compat_minor;
+	u16 reserved[5];
+} __packed;
+
+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);
+	if (err) {
+		gt_err(gt, "failed to allocate vma for GSC version query\n");
+		return err;
+	}
+
+	offset = i915_ggtt_offset(vma);
+	msg_in = vaddr;
+	msg_out = vaddr + SZ_4K;
+
+	intel_gsc_uc_heci_cmd_emit_mtl_header(&msg_in->header,
+					      HECI_MEADDRESS_MKHI,
+					      sizeof(*msg_in), 0);
+	msg_in->mkhi.group_id = MKHI_GROUP_ID_GFX_SRV;
+	msg_in->mkhi.command = MKHI_GFX_SRV_GET_HOST_COMPATIBILITY_VERSION;
+
+	err = intel_gsc_uc_heci_cmd_submit_packet(&gt->uc.gsc,
+						  offset, sizeof(*msg_in),
+						  offset + SZ_4K, SZ_4K);
+	if (err) {
+		gt_err(gt,
+		       "failed to submit GSC request for compatibility version: %d\n",
+		       err);
+		goto out_vma;
+	}
+
+	if (msg_out->header.message_size != sizeof(*msg_out)) {
+		gt_err(gt, "invalid GSC reply length %u [expected %zu], s=0x%x, f=0x%x, r=0x%x\n",
+			msg_out->header.message_size, sizeof(*msg_out),
+			msg_out->header.status, msg_out->header.flags, msg_out->mkhi.result);
+		err = -EPROTO;
+		goto out_vma;
+	}
+
+	gsc->fw.file_selected.ver.major = msg_out->compat_major;
+	gsc->fw.file_selected.ver.minor = msg_out->compat_minor;
+
+out_vma:
+	i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
+	return err;
+}
+
 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",
 		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
@@ -14,6 +14,7 @@  struct intel_gsc_mtl_header {
 #define GSC_HECI_VALIDITY_MARKER 0xA578875A
 
 	u8 heci_client_id;
+#define HECI_MEADDRESS_MKHI 7
 #define HECI_MEADDRESS_PROXY 10
 #define HECI_MEADDRESS_PXP 17
 #define HECI_MEADDRESS_HDCP 18
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
@@ -730,6 +730,31 @@  static int try_firmware_load(struct intel_uc_fw *uc_fw, const struct firmware **
 	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;
+
+	if (!wanted->ver.major || !selected->ver.major)
+		return 0;
+
+	/* Check the file's major version was as it claimed */
+	if (selected->ver.major != wanted->ver.major) {
+		gt_notice(gt, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
+			  intel_uc_fw_type_repr(uc_fw->type), selected->path,
+			  selected->ver.major, selected->ver.minor,
+			  wanted->ver.major, wanted->ver.minor);
+		if (!intel_uc_fw_is_overridden(uc_fw))
+			return -ENOEXEC;
+	} else {
+		if (old_ver && selected->ver.minor < wanted->ver.minor)
+			*old_ver = true;
+	}
+
+	return 0;
+}
+
 /**
  * intel_uc_fw_fetch - fetch uC firmware
  * @uc_fw: uC firmware
@@ -797,22 +822,9 @@  int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 			goto fail;
 	}
 
-	if (uc_fw->file_wanted.ver.major && uc_fw->file_selected.ver.major) {
-		/* Check the file's major version was as it claimed */
-		if (uc_fw->file_selected.ver.major != uc_fw->file_wanted.ver.major) {
-			gt_notice(gt, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
-				  intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
-				  uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor,
-				  uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor);
-			if (!intel_uc_fw_is_overridden(uc_fw)) {
-				err = -ENOEXEC;
-				goto fail;
-			}
-		} else {
-			if (uc_fw->file_selected.ver.minor < uc_fw->file_wanted.ver.minor)
-				old_ver = true;
-		}
-	}
+	err = intel_uc_check_file_version(uc_fw, &old_ver);
+	if (err)
+		goto fail;
 
 	if (old_ver && uc_fw->file_selected.ver.major) {
 		/* Preserve the version that was really wanted */
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
@@ -287,6 +287,7 @@  static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
 
 void intel_uc_fw_version_from_meu_manifest(struct intel_uc_fw_ver *ver,
 					   const void *data);
+int intel_uc_check_file_version(struct intel_uc_fw *uc_fw, bool *old_ver);
 void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
 			    enum intel_uc_fw_type type,
 			    bool needs_ggtt_mapping);