Re: [Intel-gfx] [PATCH 4/5] drm/i915/uc: Split firmware table validation to a separate function

[Ceraolo Spurio, Daniele]

On 4/14/2023 5:57 PM, john.c.harri...@intel.com wrote:
From: John Harrison <john.c.harri...@intel.com>

The validation of the firmware table was being done inside the code
for scanning the table for the next available firmware blob. Which is
unnecessary. Potentially, it should be a selftest. But either way, the
first step is pulling it out into a separate function that can be
called just once rather than once per blob attempt per blob type.

Signed-off-by: John Harrison <john.c.harri...@intel.com>
---
  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 164 ++++++++++++++---------
  1 file changed, 99 insertions(+), 65 deletions(-)

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 6bb45d6b8da5f..c589782467265 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -233,20 +233,22 @@ struct fw_blobs_by_type {
        u32 count;
  };
  
+static const struct uc_fw_platform_requirement blobs_guc[] = {
+       INTEL_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, GUC_FW_BLOB_MMP)
+};
+
+static const struct uc_fw_platform_requirement blobs_huc[] = {
+       INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, HUC_FW_BLOB_MMP, 
HUC_FW_BLOB_GSC)
+};
+
+static const struct fw_blobs_by_type blobs_all[INTEL_UC_FW_NUM_TYPES] = {
+       [INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
+       [INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) },
+};
+
  static void
  __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
  {
-       static const struct uc_fw_platform_requirement blobs_guc[] = {
-               INTEL_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, 
GUC_FW_BLOB_MMP)
-       };
-       static const struct uc_fw_platform_requirement blobs_huc[] = {
-               INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, 
HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC)
-       };
-       static const struct fw_blobs_by_type blobs_all[INTEL_UC_FW_NUM_TYPES] = 
{
-               [INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
-               [INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) },
-       };
-       static bool verified[INTEL_UC_FW_NUM_TYPES];
        const struct uc_fw_platform_requirement *fw_blobs;
        enum intel_platform p = INTEL_INFO(i915)->platform;
        u32 fw_count;
@@ -286,6 +288,11 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct 
intel_uc_fw *uc_fw)
                        continue;
  
  		if (uc_fw->file_selected.path) {
+                       /*
+                        * Continuing an earlier search after a found blob 
failed to load.
+                        * Once the previously chosen path has been found, 
clear it out
+                        * and let the search continue from there.
+                        */
                        if (uc_fw->file_selected.path == blob->path)
                                uc_fw->file_selected.path = NULL;
  
@@ -306,78 +313,103 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
                /* Failed to find a match for the last attempt?! */
                uc_fw->file_selected.path = NULL;
        }
+}
  
-	/* make sure the list is ordered as expected */
-       if (IS_ENABLED(CONFIG_DRM_I915_SELFTEST) && !verified[uc_fw->type]) {
-               verified[uc_fw->type] = true;
+static void validate_fw_table_type(struct drm_i915_private *i915, enum 
intel_uc_fw_type type)
+{
+       const struct uc_fw_platform_requirement *fw_blobs;
+       u32 fw_count;
+       int i;
  
-		for (i = 1; i < fw_count; i++) {
-                       /* Next platform is good: */
-                       if (fw_blobs[i].p < fw_blobs[i - 1].p)
-                               continue;
+       if (type >= ARRAY_SIZE(blobs_all)) {
+               drm_err(&i915->drm, "No blob array for %s\n", 
intel_uc_fw_type_repr(type));
+               return;
+       }
  
-			/* Next platform revision is good: */
-                       if (fw_blobs[i].p == fw_blobs[i - 1].p &&
-                           fw_blobs[i].rev < fw_blobs[i - 1].rev)
-                               continue;
+       fw_blobs = blobs_all[type].blobs;
+       fw_count = blobs_all[type].count;
  
-			/* Platform/revision must be in order: */
-                       if (fw_blobs[i].p != fw_blobs[i - 1].p ||
-                           fw_blobs[i].rev != fw_blobs[i - 1].rev)
-                               goto bad;
+       if (!fw_count)
+               return;
  
-			/* Next major version is good: */
-                       if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
-                               continue;
+       /* make sure the list is ordered as expected */
+       for (i = 1; i < fw_count; i++) {
+               /* Next platform is good: */
+               if (fw_blobs[i].p < fw_blobs[i - 1].p)
+                       continue;
  
-			/* New must be before legacy: */
-                       if (!fw_blobs[i].blob.legacy && fw_blobs[i - 
1].blob.legacy)
-                               goto bad;
+               /* Next platform revision is good: */
+               if (fw_blobs[i].p == fw_blobs[i - 1].p &&
+                   fw_blobs[i].rev < fw_blobs[i - 1].rev)
+                       continue;
  
-			/* New to legacy also means 0.0 to X.Y (HuC), or X.0 to X.Y (GuC) */
-                       if (fw_blobs[i].blob.legacy && !fw_blobs[i - 
1].blob.legacy) {
-                               if (!fw_blobs[i - 1].blob.major)
-                                       continue;
+               /* Platform/revision must be in order: */
+               if (fw_blobs[i].p != fw_blobs[i - 1].p ||
+                   fw_blobs[i].rev != fw_blobs[i - 1].rev)
+                       goto bad;
  
-				if (fw_blobs[i].blob.major == fw_blobs[i - 1].blob.major)
-                                       continue;
-                       }
+               /* Next major version is good: */
+               if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
+                       continue;
  
-			/* Major versions must be in order: */
-                       if (fw_blobs[i].blob.major != fw_blobs[i - 
1].blob.major)
-                               goto bad;
+               /* New must be before legacy: */
+               if (!fw_blobs[i].blob.legacy && fw_blobs[i - 1].blob.legacy)
+                       goto bad;
  
-			/* Next minor version is good: */
-                       if (fw_blobs[i].blob.minor < fw_blobs[i - 1].blob.minor)
+               /* New to legacy also means 0.0 to X.Y (HuC), or X.0 to X.Y 
(GuC) */
+               if (fw_blobs[i].blob.legacy && !fw_blobs[i - 1].blob.legacy) {
+                       if (!fw_blobs[i - 1].blob.major)
                                continue;
  
-			/* Minor versions must be in order: */
-                       if (fw_blobs[i].blob.minor != fw_blobs[i - 
1].blob.minor)
-                               goto bad;
-
-                       /* Patch versions must be in order: */
-                       if (fw_blobs[i].blob.patch <= fw_blobs[i - 
1].blob.patch)
+                       if (fw_blobs[i].blob.major == fw_blobs[i - 
1].blob.major)
                                continue;
+               }
+
+               /* Major versions must be in order: */
+               if (fw_blobs[i].blob.major != fw_blobs[i - 1].blob.major)
+                       goto bad;
+
+               /* Next minor version is good: */
+               if (fw_blobs[i].blob.minor < fw_blobs[i - 1].blob.minor)
+                       continue;
+
+               /* Minor versions must be in order: */
+               if (fw_blobs[i].blob.minor != fw_blobs[i - 1].blob.minor)
+                       goto bad;
+
+               /* Patch versions must be in order: */
+               if (fw_blobs[i].blob.patch <= fw_blobs[i - 1].blob.patch)
+                       continue;
  
  bad:
-                       drm_err(&i915->drm, "Invalid %s blob order: %s r%u 
%s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
-                               intel_uc_fw_type_repr(uc_fw->type),
-                               intel_platform_name(fw_blobs[i - 1].p), 
fw_blobs[i - 1].rev,
-                               fw_blobs[i - 1].blob.legacy ? "L" : "v",
-                               fw_blobs[i - 1].blob.major,
-                               fw_blobs[i - 1].blob.minor,
-                               fw_blobs[i - 1].blob.patch,
-                               intel_platform_name(fw_blobs[i].p), 
fw_blobs[i].rev,
-                               fw_blobs[i].blob.legacy ? "L" : "v",
-                               fw_blobs[i].blob.major,
-                               fw_blobs[i].blob.minor,
-                               fw_blobs[i].blob.patch);
-
-                       uc_fw->file_selected.path = NULL;
-               }
+               drm_err(&i915->drm, "Invalid %s blob order: %s r%u %s%d.%d.%d comes 
before %s r%u %s%d.%d.%d\n",
+                       intel_uc_fw_type_repr(type),
+                       intel_platform_name(fw_blobs[i - 1].p), fw_blobs[i - 
1].rev,
+                       fw_blobs[i - 1].blob.legacy ? "L" : "v",
+                       fw_blobs[i - 1].blob.major,
+                       fw_blobs[i - 1].blob.minor,
+                       fw_blobs[i - 1].blob.patch,
+                       intel_platform_name(fw_blobs[i].p), fw_blobs[i].rev,
+                       fw_blobs[i].blob.legacy ? "L" : "v",
+                       fw_blobs[i].blob.major,
+                       fw_blobs[i].blob.minor,
+                       fw_blobs[i].blob.patch);

Confirming that this big diff was just an indent change was painful :/

        }
  }
  
+static void validate_fw_table(struct drm_i915_private *i915)
+{
+       enum intel_uc_fw_type type;
+       static bool done;
+
+       if (!IS_ENABLED(CONFIG_DRM_I915_SELFTEST) || done)
+               return;
+       done = true;
+
+       for (type = 0; type < INTEL_UC_FW_NUM_TYPES; type++)
+               validate_fw_table_type(i915, type);
+}
+
  static const char *__override_guc_firmware_path(struct drm_i915_private *i915)
  {
        if (i915->params.enable_guc & ENABLE_GUC_MASK)
@@ -432,6 +464,8 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
  {
        struct drm_i915_private *i915 = ____uc_fw_to_gt(uc_fw, type)->i915;
  
+	validate_fw_table(i915);

Personal preference: IMO since we're calling intel_uc_fw_init_early per 
FW type it would've been cleaner to restrict validate_fw_table() to a 
single blob type. This would have the negative side effect of having to 
track the "done" status per FW type, so I can see it's not a clean 
improvement. Not a blocker.

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospu...@intel.com>

Daniele

+
        /*
         * we use FIRMWARE_UNINITIALIZED to detect checks against uc_fw->status
         * before we're looked at the HW caps to see if we have uc support