[Intel-xe] [PATCH 1/3] drm/xe/huc: Macro support for loading unversiond HuC

[Lucas De Marchi]

On Wed, Mar 22, 2023 at 04:28:10PM -0700, Anusha Srivatsa wrote:
>Follow the new direction of firmware and add macro
>support for loading unversioned HuC. Keep HuC
>versioned loading support as well for platforms
>that fall under force_probe support
>
>Add check to ensure driver does not do any version check
>for HuC if going through unversioned load.
>
>v2: unversioned firmware to be the default for platforms
>not under force_probe. Maintain versioned firmware macro support
>for platforms under force-probe protection.
>
>Cc: Matt Roper <matthew.d.roper at intel.com>
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
>---
> drivers/gpu/drm/xe/xe_uc_fw.c | 43 +++++++++++++++++++++++------------
> 1 file changed, 28 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
>index 00408f1b73e1..4ade5b1b6d31 100644
>--- a/drivers/gpu/drm/xe/xe_uc_fw.c
>+++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>@@ -50,15 +50,19 @@ static struct xe_device *uc_fw_to_xe(struct xe_uc_fw *uc_fw)
> 	fw_def(DG1,          0, guc_def(dg1,  70, 5, 2)) \
> 	fw_def(TIGERLAKE,    0, guc_def(tgl,  70, 5, 2))
>
>-#define XE_HUC_FIRMWARE_DEFS(fw_def, huc_def) \
>-	fw_def(DG1,          0, huc_def(dg1,  7, 9, 3)) \
>-	fw_def(TIGERLAKE,    0, huc_def(tgl,  7, 9, 3))
>+#define XE_HUC_FIRMWARE_DEFS(fw_def, huc_def, huc_ver) \
>+	fw_def(DG1,          0, huc_def(dg1)) \
>+	fw_def(TIGERLAKE,    0, huc_def(tgl))
>
> #define __MAKE_UC_FW_PATH_MAJOR(prefix_, name_, major_) \
> 	"i915/" \
> 	__stringify(prefix_) "_" name_ "_" \
> 	__stringify(major_) ".bin"
>
>+#define __MAKE_HUC_FW_PATH(prefix_, name_) \

this name

>+        "i915/" \
>+        __stringify(prefix_) "_" name_ ".bin"
>+
> #define __MAKE_UC_FW_PATH(prefix_, name_, major_, minor_, patch_) \

clashes with this name. I suppose you mentioned to have

__MAKE_UC_FW_PATH_VERSIONED() vs __MAKE_UC_FW_PATH()

>         "i915/" \
>        __stringify(prefix_) name_ \
>@@ -69,15 +73,19 @@ static struct xe_device *uc_fw_to_xe(struct xe_uc_fw *uc_fw)
> #define MAKE_GUC_FW_PATH(prefix_, major_, minor_, patch_) \
> 	__MAKE_UC_FW_PATH_MAJOR(prefix_, "guc", major_)
>
>-#define MAKE_HUC_FW_PATH(prefix_, major_, minor_, bld_num_) \
>-	__MAKE_UC_FW_PATH(prefix_, "_huc_", major_, minor_, bld_num_)
>+#define MAKE_HUC_FW_PATH(prefix_) \
>+	__MAKE_HUC_FW_PATH(prefix_, "huc")
>+
>+#define MAKE_HUC_FW_VERSION_PATH(prefix_, major_, minor_, patch_) \
>+	__MAKE_UC_FW_PATH(prefix_, "huc", major_, minor_, patch_)
>+
>
> /* All blobs need to be declared via MODULE_FIRMWARE() */
> #define XE_UC_MODULE_FW(platform_, revid_, uc_) \
> 	MODULE_FIRMWARE(uc_);
>
> XE_GUC_FIRMWARE_DEFS(XE_UC_MODULE_FW, MAKE_GUC_FW_PATH)
>-XE_HUC_FIRMWARE_DEFS(XE_UC_MODULE_FW, MAKE_HUC_FW_PATH)
>+XE_HUC_FIRMWARE_DEFS(XE_UC_MODULE_FW, MAKE_HUC_FW_PATH, MAKE_HUC_FW_VERSION_PATH)
>
> /* The below structs and macros are used to iterate across the list of blobs */
> struct __packed uc_fw_blob {
>@@ -93,9 +101,12 @@ struct __packed uc_fw_blob {
> 	UC_FW_BLOB(major_, minor_, \
> 		   MAKE_GUC_FW_PATH(prefix_, major_, minor_, patch_))
>
>-#define HUC_FW_BLOB(prefix_, major_, minor_, bld_num_) \
>+#define HUC_FW_BLOB(prefix_) \
>+	UC_FW_BLOB(0, 0, MAKE_HUC_FW_PATH(prefix_))
>+
>+#define HUC_FW_VERSION_BLOB(prefix_, major_, minor_, bld_num_) \
> 	UC_FW_BLOB(major_, minor_, \
>-		   MAKE_HUC_FW_PATH(prefix_, major_, minor_, bld_num_))
>+		   MAKE_HUC_FW_VERSION_PATH(prefix_, major_, minor_, bld_num_))

I think this is getting super complex and hard to follow and it still
doesn't fix the guc situation :(

>
> struct __packed uc_fw_platform_requirement {
> 	enum xe_platform p;
>@@ -122,7 +133,7 @@ uc_fw_auto_select(struct xe_device *xe, struct xe_uc_fw *uc_fw)
> 		XE_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB)
> 	};
> 	static const struct uc_fw_platform_requirement blobs_huc[] = {
>-		XE_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB)
>+		XE_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, HUC_FW_VERSION_BLOB)
> 	};
> 	static const struct fw_blobs_by_type blobs_all[XE_UC_FW_NUM_TYPES] = {
> 		[XE_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
>@@ -299,15 +310,17 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
> 	uc_fw->minor_ver_found = FIELD_GET(CSS_SW_VERSION_UC_MINOR,
> 					   css->sw_version);
>
>-	if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
>-	    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
>-		drm_notice(&xe->drm, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
>-			   xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
>-			   uc_fw->major_ver_found, uc_fw->minor_ver_found,
>-			   uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
>+	if (uc_fw->major_ver_wanted) {
>+		if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
>+		    uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
>+			drm_notice(&xe->drm, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
>+				   xe_uc_fw_type_repr(uc_fw->type), uc_fw->path,
>+				   uc_fw->major_ver_found, uc_fw->minor_ver_found,
>+				   uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> 		if (!xe_uc_fw_is_overridden(uc_fw)) {
> 			err = -ENOEXEC;
> 			goto fail;
>+			}

missing indentation in the whole block.

I think this may work for HuC, but I think we will need to rewrite parts
of what this is doing here as it's getting very complex.

Lucas De Marchi

> 		}
> 	}
>
>-- 
>2.25.1
>