[PATCH] drm/xe/uc: Add support for different firmware files on each GT
Cavitt, Jonathan
jonathan.cavitt at intel.com
Tue Feb 25 00:00:15 UTC 2025
Some minor nits below, but otherwise:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-----Original Message-----
From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of John.C.Harrison at Intel.com
Sent: Monday, February 24, 2025 2:21 PM
To: Intel-Xe at Lists.FreeDesktop.Org
Cc: Harrison, John C <john.c.harrison at intel.com>
Subject: [PATCH] drm/xe/uc: Add support for different firmware files on each GT
>
> From: John Harrison <John.C.Harrison at Intel.com>
>
> The different GTs on a device can be very different. So add support
> for being able to load a different GuC firmware image that is tailored
> to the specific GT type.
NIT:
Commit message is good, and we can leave it as-is. Though perhaps consider this
rewording:
"""
The various GTs on a device may contain differences that need to be accounted
for by firmware. So, add support for being able to load a unique GuC firmware
image per GT type, tailored to its usage.
"""
>
> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> ---
> drivers/gpu/drm/xe/xe_uc_fw.c | 73 +++++++++++++++++++++--------------
> 1 file changed, 43 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
> index 18e06ee9e23f..eb77528628f2 100644
> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
> @@ -98,6 +98,7 @@ struct uc_fw_entry {
> u16 minor;
> u16 patch;
> bool full_ver_required;
> + const char *gt_type;
NIT:
It seems like this value is currently only ever being set to an empty string. I
take it this may not be the case in the future?
> };
> };
>
> @@ -107,21 +108,21 @@ struct fw_blobs_by_type {
> };
>
> #define XE_GUC_FIRMWARE_DEFS(fw_def, mmp_ver, major_ver) \
> - fw_def(PANTHERLAKE, mmp_ver(xe, guc, ptl, 70, 38, 1)) \
> - fw_def(BATTLEMAGE, major_ver(xe, guc, bmg, 70, 29, 2)) \
> - fw_def(LUNARLAKE, major_ver(xe, guc, lnl, 70, 29, 2)) \
> - fw_def(METEORLAKE, major_ver(i915, guc, mtl, 70, 29, 2)) \
> - fw_def(PVC, mmp_ver(xe, guc, pvc, 70, 29, 2)) \
> - fw_def(DG2, major_ver(i915, guc, dg2, 70, 29, 2)) \
> - fw_def(DG1, major_ver(i915, guc, dg1, 70, 29, 2)) \
> - fw_def(ALDERLAKE_N, major_ver(i915, guc, tgl, 70, 29, 2)) \
> - fw_def(ALDERLAKE_P, major_ver(i915, guc, adlp, 70, 29, 2)) \
> - fw_def(ALDERLAKE_S, major_ver(i915, guc, tgl, 70, 29, 2)) \
> - fw_def(ROCKETLAKE, major_ver(i915, guc, tgl, 70, 29, 2)) \
> - fw_def(TIGERLAKE, major_ver(i915, guc, tgl, 70, 29, 2))
> + fw_def(PANTHERLAKE, mmp_ver(xe, guc, ptl, 70, 38, 1, "")) \
> + fw_def(BATTLEMAGE, major_ver(xe, guc, bmg, 70, 29, 2, "")) \
> + fw_def(LUNARLAKE, major_ver(xe, guc, lnl, 70, 29, 2, "")) \
> + fw_def(METEORLAKE, major_ver(i915, guc, mtl, 70, 29, 2, "")) \
> + fw_def(PVC, mmp_ver(xe, guc, pvc, 70, 29, 2, "")) \
> + fw_def(DG2, major_ver(i915, guc, dg2, 70, 29, 2, "")) \
> + fw_def(DG1, major_ver(i915, guc, dg1, 70, 29, 2, "")) \
> + fw_def(ALDERLAKE_N, major_ver(i915, guc, tgl, 70, 29, 2, "")) \
> + fw_def(ALDERLAKE_P, major_ver(i915, guc, adlp, 70, 29, 2, "")) \
> + fw_def(ALDERLAKE_S, major_ver(i915, guc, tgl, 70, 29, 2, "")) \
> + fw_def(ROCKETLAKE, major_ver(i915, guc, tgl, 70, 29, 2, "")) \
> + fw_def(TIGERLAKE, major_ver(i915, guc, tgl, 70, 29, 2, ""))
>
> #define XE_HUC_FIRMWARE_DEFS(fw_def, mmp_ver, no_ver) \
> - fw_def(PANTHERLAKE, mmp_ver(xe, huc, ptl, 10, 2, 1)) \
> + fw_def(PANTHERLAKE, mmp_ver(xe, huc, ptl, 10, 2, 1, "")) \
> fw_def(BATTLEMAGE, no_ver(xe, huc, bmg)) \
> fw_def(LUNARLAKE, no_ver(xe, huc, lnl)) \
> fw_def(METEORLAKE, no_ver(i915, huc_gsc, mtl)) \
> @@ -136,30 +137,30 @@ struct fw_blobs_by_type {
> fw_def(LUNARLAKE, major_ver(xe, gsc, lnl, 104, 1, 0)) \
> fw_def(METEORLAKE, major_ver(i915, gsc, mtl, 102, 1, 0))
>
> -#define MAKE_FW_PATH(dir__, uc__, shortname__, version__) \
> - __stringify(dir__) "/" __stringify(shortname__) "_" __stringify(uc__) version__ ".bin"
> +#define MAKE_FW_PATH(dir__, uc__, shortname__, type__, version__) \
> + __stringify(dir__) "/" __stringify(shortname__) type__ "_" __stringify(uc__) version__ ".bin"
>
> -#define fw_filename_mmp_ver(dir_, uc_, shortname_, a, b, c) \
> - MAKE_FW_PATH(dir_, uc_, shortname_, "_" __stringify(a ## . ## b ## . ## c))
> -#define fw_filename_major_ver(dir_, uc_, shortname_, a, b, c) \
> - MAKE_FW_PATH(dir_, uc_, shortname_, "_" __stringify(a))
> +#define fw_filename_mmp_ver(dir_, uc_, shortname_, a, b, c, t) \
> + MAKE_FW_PATH(dir_, uc_, shortname_, t, "_" __stringify(a ## . ## b ## . ## c))
> +#define fw_filename_major_ver(dir_, uc_, shortname_, a, b, c, t) \
> + MAKE_FW_PATH(dir_, uc_, shortname_, t, "_" __stringify(a))
> #define fw_filename_no_ver(dir_, uc_, shortname_) \
> - MAKE_FW_PATH(dir_, uc_, shortname_, "")
> + MAKE_FW_PATH(dir_, uc_, shortname_, "", "")
> #define fw_filename_gsc(dir_, uc_, shortname_, a, b, c) \
> - MAKE_FW_PATH(dir_, uc_, shortname_, "_" __stringify(b))
> -
> -#define uc_fw_entry_mmp_ver(dir_, uc_, shortname_, a, b, c) \
> - { fw_filename_mmp_ver(dir_, uc_, shortname_, a, b, c), \
> - a, b, c, true }
> -#define uc_fw_entry_major_ver(dir_, uc_, shortname_, a, b, c) \
> - { fw_filename_major_ver(dir_, uc_, shortname_, a, b, c), \
> - a, b, c }
> + MAKE_FW_PATH(dir_, uc_, shortname_, "", "_" __stringify(b))
> +
> +#define uc_fw_entry_mmp_ver(dir_, uc_, shortname_, a, b, c, t) \
> + { fw_filename_mmp_ver(dir_, uc_, shortname_, a, b, c, t), \
> + a, b, c, true, t }
> +#define uc_fw_entry_major_ver(dir_, uc_, shortname_, a, b, c, t) \
> + { fw_filename_major_ver(dir_, uc_, shortname_, a, b, c, t), \
> + a, b, c, false, t }
> #define uc_fw_entry_no_ver(dir_, uc_, shortname_) \
> { fw_filename_no_ver(dir_, uc_, shortname_), \
> - 0, 0 }
> + 0, 0, 0, false, "" }
> #define uc_fw_entry_gsc(dir_, uc_, shortname_, a, b, c) \
> { fw_filename_gsc(dir_, uc_, shortname_, a, b, c), \
> - a, b, c }
> + a, b, c, false, "" }
>
> /* All blobs need to be declared via MODULE_FIRMWARE() */
> #define XE_UC_MODULE_FIRMWARE(platform__, fw_filename) \
> @@ -227,6 +228,7 @@ uc_fw_auto_select(struct xe_device *xe, struct xe_uc_fw *uc_fw)
> };
> static const struct uc_fw_entry *entries;
> enum xe_platform p = xe->info.platform;
> + struct xe_gt *gt = uc_fw_to_gt(uc_fw);
> u32 count;
> int i;
>
> @@ -234,8 +236,19 @@ uc_fw_auto_select(struct xe_device *xe, struct xe_uc_fw *uc_fw)
> entries = blobs_all[uc_fw->type].entries;
> count = blobs_all[uc_fw->type].count;
>
> + xe_gt_assert(gt, (gt->info.type == XE_GT_TYPE_MAIN) || (gt->info.type == XE_GT_TYPE_MEDIA));
> +
> for (i = 0; i < count && p <= entries[i].platform; i++) {
> if (p == entries[i].platform) {
> + if (entries[i].gt_type[0]) {
> + if (gt->info.type == XE_GT_TYPE_MAIN &&
> + (entries[i].gt_type[0] != 'g'))
> + continue;
> + if (gt->info.type == XE_GT_TYPE_MEDIA &&
> + (entries[i].gt_type[0] != 'm'))
> + continue;
> + }
NIT:
There may be value in breaking this block of if-statements into a helper function:
"""
static bool check_skip_auto_fw(struct xe_gt *gt, const char *gt_type)
{
char tag;
// ASIDE: Probably unnecessary check below.
if (!gt_type)
return false;
tag = gt_type[0];
if (!tag)
return false;
else if (gt->info.type == XE_GT_TYPE_MAIN && tag != 'g')
return true;
else if (gt->info.type == XE_GT_TYPE_MEDIA && tag != 'm')
return true;
else
return false;
}
...
for (i = 0; i < count && p <= entries[i].platform; i++) {
if (p == entries[i].platform &&
!check_skip_auto_fw(gt, entries[i].gt_type)) {
...
"""
Just something to consider.
-Jonathan Cavitt
> +
> uc_fw->path = entries[i].path;
> uc_fw->versions.wanted.major = entries[i].major;
> uc_fw->versions.wanted.minor = entries[i].minor;
> --
> 2.47.0
>
>
More information about the Intel-xe
mailing list