Re: [PATCH] drm/xe/uc: Add support for different firmware files on each GT

[John Harrison <john.c.harrison@intel.com>]

On 2/24/2025 16:00, Cavitt, Jonathan wrote:
> Some minor nits below, but otherwise:
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
>
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of John.C.Harrison@Intel.com
> Sent: Monday, February 24, 2025 2:21 PM
> To: Intel-Xe@Lists.FreeDesktop.Org
> Cc: Harrison, John C <john.c.harrison@intel.com>
> Subject: [PATCH] drm/xe/uc: Add support for different firmware files on each GT
>> From: John Harrison <John.C.Harrison@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@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
>>
>>