[PATCH] drm/xe/uc: Add support for different firmware files on each GT
Cavitt, Jonathan
jonathan.cavitt at intel.com
Tue Feb 25 15:12:49 UTC 2025
-----Original Message-----
From: Harrison, John C <john.c.harrison at intel.com>
Sent: Monday, February 24, 2025 4:34 PM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; Intel-Xe at Lists.FreeDesktop.Org
Subject: Re: [PATCH] drm/xe/uc: Add support for different firmware files on each GT
>
> 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.
I didn't necessarily want to point it out, but it gets rid of repeated uses of the word
"different" from the original.
And that's the reason it's a NIT and not a proper change request.
>
> >
> >> 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.
Just figured it would reduce code nesting.
None of these NITs are blocking, by the way. And my Reviewed-by still stands.
-Jonathan Cavitt
>
> 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