Re: [PATCH v2 2/8] drm/i915/uc: Unify uC FW selection

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Daniele Ceraolo Spurio (2019-07-24 03:21:47)
> Instead of having 2 identical functions for GuC and HuC firmware
> selection, we can unify the selection logic and just use different lists
> based on FW type.
> 
> Note that the revid is not relevant for current blobs, but the upcoming
> CML will be identified as CFL rev 5, so by considering the revid we're
> ready for that.
> 
> v2: rework blob list defs (Michal), add order check (Chris), fuse GuC
>     and HuC lists into one.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> index ff6f7b157ecb..fa2151fa3a13 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> @@ -23,91 +23,6 @@
>   * Note that HuC firmware loading must be done before GuC loading.
>   */
>  
> -#define SKL_HUC_FW_MAJOR 01
> -#define SKL_HUC_FW_MINOR 07
> -#define SKL_BLD_NUM 1398
> + * List of required GuC and HuC binaries per-platform.
> + * Must be ordered based on platform + revid, from newer to older.
> + */
> +#define INTEL_UC_FIRMWARE_DEFS(fw_def, guc_def, huc_def) \
> +       fw_def(ICELAKE,    0, guc_def(icl, 33, 0, 0), huc_def(icl,  8,  4, 3238)) \
✓

> +       fw_def(COFFEELAKE, 0, guc_def(kbl, 33, 0, 0), huc_def(kbl, 02, 00, 1810)) \
✓
Oh we still don't have the new version. Hmm, I think you have planned
for it well enough.

> +       fw_def(GEMINILAKE, 0, guc_def(glk, 33, 0, 0), huc_def(glk, 03, 01, 2893)) \
✓

> +       fw_def(KABYLAKE,   0, guc_def(kbl, 33, 0, 0), huc_def(kbl, 02, 00, 1810)) \
✓

> +       fw_def(BROXTON,    0, guc_def(bxt, 33, 0, 0), huc_def(bxt, 01,  8, 2893)) \
✓

> +       fw_def(SKYLAKE,    0, guc_def(skl, 33, 0, 0), huc_def(skl, 01, 07, 1398))
✓

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

I still can't believe that encoding version strings into paths is the
best solution but whatever.

> +#define MAKE_GUC_FW_PATH(prefix_, major_, minor_, patch_) \
> +       __MAKE_UC_FW_PATH(prefix_, "_guc_", ".", major_, minor_, patch_)
> +
> +#define MAKE_HUC_FW_PATH(prefix_, major_, minor_, bld_num_) \
> +       __MAKE_UC_FW_PATH(prefix_, "_huc_ver", "_", major_, minor_, bld_num_)
> +
> +/* All blobs need to be declared via MODULE_FIRMWARE() */
> +#define INTEL_UC_MODULE_FW(platform_, revid_, guc_, huc_) \
> +       MODULE_FIRMWARE(guc_); \
> +       MODULE_FIRMWARE(huc_);
> +
> +INTEL_UC_FIRMWARE_DEFS(INTEL_UC_MODULE_FW, MAKE_GUC_FW_PATH, MAKE_HUC_FW_PATH)

Ok. That was fun.

> +
> +/*
> + * The below defs and macros are used to iterate across the list of blobs. See
> + * __uc_fw_select() below for details.
> + */
> +struct __packed intel_uc_fw_blob {
> +       u8 major;
> +       u8 minor;
> +       const char *path;
> +};
> +
> +#define UC_FW_BLOB(major_, minor_, path_) \
> +       { .major = major_, .minor = minor_, .path = path_ }
> +
> +#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
> +       UC_FW_BLOB(major_, minor_, \
> +                  MAKE_GUC_FW_PATH(prefix_, major_, minor_, patch_))
> +
> +#define HUC_FW_BLOB(prefix_, major_, minor_, bld_num_) \
> +       UC_FW_BLOB(major_, minor_, \
> +                  MAKE_HUC_FW_PATH(prefix_, major_, minor_, bld_num_))
> +
> +#define MAKE_FW_LIST(platform_, revid_, guc_, huc_) \
> +{ \
> +       .p = INTEL_##platform_, \
> +       .first_rev = revid_, \
> +       .blobs[INTEL_UC_FW_TYPE_GUC] = guc_, \
> +       .blobs[INTEL_UC_FW_TYPE_HUC] = huc_, \
> +},
> +
> +static void
> +__uc_fw_select(struct intel_uc_fw *uc_fw, enum intel_platform p, u8 rev)
> +{
> +       static const struct __packed {
> +               enum intel_platform p;
> +               u8 first_rev;
> +               const struct intel_uc_fw_blob blobs[INTEL_UC_NUM_TYPES];
> +       } fw_blobs[] = {
> +               INTEL_UC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, HUC_FW_BLOB)
> +       };
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(fw_blobs) && p <= fw_blobs[i].p; i++) {
> +               if (p == fw_blobs[i].p && rev >= fw_blobs[i].first_rev) {
> +                       const struct intel_uc_fw_blob *blob =
> +                                       &fw_blobs[i].blobs[uc_fw->type];
> +                       uc_fw->path = blob->path;
> +                       uc_fw->major_ver_wanted = blob->major;
> +                       uc_fw->minor_ver_wanted = blob->minor;
> +                       break;
> +               }
> +       }
> +
> +       /* make sure the list is ordered as expected */
> +       if (IS_ENABLED(CONFIG_DRM_I915_SELFTEST)) {
> +               for (i = 1; i < ARRAY_SIZE(fw_blobs); i++) {
> +                       if (fw_blobs[i].p < fw_blobs[i - 1].p)
> +                               continue;
> +
> +                       if (fw_blobs[i].p == fw_blobs[i - 1].p &&
> +                           fw_blobs[i].first_rev < fw_blobs[i - 1].first_rev)
> +                               continue;
> +
> +                       pr_err("invalid FW blob order: %s r%u comes before %s r%u\n",
> +                              intel_platform_name(fw_blobs[i - 1].p),
> +                              fw_blobs[i - 1].first_rev,
> +                              intel_platform_name(fw_blobs[i].p),
> +                              fw_blobs[i].first_rev);
> +
> +                       uc_fw->path = NULL;
> +                       uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> +                       return;
> +               }
> +       }
> +}

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx