[PATCH] drm/xe/uc: Add support for different firmware files on each GT
John Harrison
john.c.harrison at intel.com
Tue Feb 25 00:33:46 UTC 2025
On 2/24/2025 16:00, Cavitt, Jonathan wrote:
> 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.
> """
Not seeing how that is any different.
>
>> 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?
Yes, usage will follow.
>
>> };
>> };
>>
>> @@ -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
Not seeing how that is improving things. If the types of firmware
balloon out to many variants then sure, a helper would be sensible. But
with just two, the helper is more lines of code than just testing inline.
John.
>
>> +
>> 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