Re: [Intel-xe] [PATCH 3/3] drm/xe: Update GuC/HuC firmware autoselect logic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Apr 03, 2023 at 06:09:10PM +0000, Anusha Srivatsa wrote:


-----Original Message-----
From: De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>
Sent: Wednesday, March 29, 2023 8:46 PM
To: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>
Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx; Harrison, John C
<john.c.harrison@xxxxxxxxx>; Ceraolo Spurio, Daniele
<daniele.ceraolospurio@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Daniel
Vetter <daniel.vetter@xxxxxxxx>; Dave Airlie <airlied@xxxxxxxxxx>
Subject: Re: [PATCH 3/3] drm/xe: Update GuC/HuC firmware autoselect logic

On Tue, Mar 28, 2023 at 04:31:13PM -0700, Anusha Srivatsa wrote:
>
>
>> -----Original Message-----
>> From: De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>
>> Sent: Thursday, March 23, 2023 10:18 PM
>> To: intel-xe@xxxxxxxxxxxxxxxxxxxxx
>> Cc: Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>; Harrison, John C
>> <john.c.harrison@xxxxxxxxx>; Ceraolo Spurio, Daniele
>> <daniele.ceraolospurio@xxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
>> Daniel Vetter <daniel.vetter@xxxxxxxx>; Dave Airlie
>> <airlied@xxxxxxxxxx>; De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>
>> Subject: [PATCH 3/3] drm/xe: Update GuC/HuC firmware autoselect logic
>>
>> Update the logic to autoselect GuC/HuC for the platforms with the
>> following
>> improvements:
>>
>> - Document what is the firmware file that is expected to be
>>   loaded and what is checked from blob headers
>> - When the platform is under force-probe it's desired to enforce
>>   the full-version requirement so the correct firmware is used
>>   before widespread adoption and backward-compatibility
>>
>Extra line ^
>
>>   commitments
>> - Directory from which we expect firmware blobs to be available in
>>   upstream linux-firmware repository depends on the platform: for
>>   the ones supported by i915 it uses the i915/ directory, but the ones
>>   expected to be supported by xe, it's on the xe/ directory. This
>>   means that for platforms in the intersection, the firmware is
>>   loaded from a different directory, but that is not much important
>>   in the firmware repo and it avoids firmware duplication.
>>
>> - Make the table with the firmware definitions clearly state the
>>   versions being expected. Now with macros to select the version it's
>>   possible to choose between full-version/major-version for GuC and
>>   full-version/no-version for HuC. These are similar to the macros used
>>   in i915, but implemented in a slightly different way to avoid
>>   duplicating the macros for each firmware/type and functionality,
>>   besides adding the support for different directories.
>>
>> - There is no check added regarding force-probe since xe should
>>   reuse the same firmware files published for i915 for past
>>   platforms. This can be improved later with additional
>>   kunit checking against a hardcoded list of platforms that
>Extra line here.
>
>>   falls in this category.
>> - As mentioned in the TODO, the major version fallback was not
>>   implemented before as currently each platform only supports one
>>   major. That can be easily added later.
>>
>> - GuC version for MTL and PVC were updated to 70.6.4, using the exact
>>   full version, while the
>>
>> After this the GuC firmware used by PVC changes to pvc_guc_70.5.2.bin
>> since it's using a file not published yet.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/xe/xe_uc_fw.c       | 315 +++++++++++++++++-----------
>>  drivers/gpu/drm/xe/xe_uc_fw.h       |   2 +-
>>  drivers/gpu/drm/xe/xe_uc_fw_types.h |   7 +
>>  3 files changed, 204 insertions(+), 120 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c
>> b/drivers/gpu/drm/xe/xe_uc_fw.c index 174c42873ebb..653bc3584cc5
>> 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>> @@ -17,6 +17,137 @@
>>  #include "xe_mmio.h"
>>  #include "xe_uc_fw.h"
>>
>> +/*
>> + * List of required GuC and HuC binaries per-platform. They must be
>> +ordered
>> + * based on platform, from newer to older.
>> + *
>> + * Versioning follows the guidelines from
>> + * Documentation/driver-api/firmware/firmware-usage-guidelines.rst.
>> +There is a
>> + * distinction for platforms being officially supported by the driver or not.
>> + * Platforms not available publicly or not yet officially supported
>> +by the
>> + * driver (under force-probe), use the mmp_ver(): the firmware
>> +autoselect logic
>> + * will select the firmware from disk with filename that matches the
>> +full
>> + * "mpp version", i.e. major.minor.patch. mmp_ver() should only be
>> +used for
>> + * this case.
>> + *
>> + * For platforms officially supported by the driver, the filename
>> +always only
>> + * ever contains the major version (GuC) or no version at all (HuC).
>> + *
>> + * After loading the file, the driver parses the versions embedded in the blob.
>> + * The major version needs to match a major version supported by the
>> +driver (if
>> + * any). The minor version is also checked and a notice emitted to
>> +the log if
>> + * the version found is smaller than the version wanted. This is
>> +done only for
>> + * informational purposes so users may have a chance to upgrade, but
>> +the driver
>> + * still loads and use the older firmware.
>> + *
>> + * Examples:
>> + *
>> + *	1) Platform officially supported by i915 - using Tigerlake as example.
>> + *	   Driver loads the following firmware blobs from disk:
>> + *
>> + *		- i915/tgl_guc_<major>.bin
>> + *		- i915/tgl_huc.bin
>> + *
>> + *	   <major> number for GuC is checked that it matches the version inside
>> + *	   the blob. <minor> version is checked and if smaller than the expected
>> + *	   an info message is emitted about that.
>> + *
>> + *	1) XE_<FUTUREINTELPLATFORM>, still under require_force_probe.
>> Using
>> + *	   "wipplat" as a short-name. Driver loads the following firmware blobs
>> + *	   from disk:
>> + *
>> + *		- xe/wipplat_guc_<major>.<minor>.<patch>.bin
>> + *		- xe/wipplat_huc_<major>.<minor>.<patch>.bin
>> + *
>> + *	   <major> and <minor> are checked that they match the version inside
>> + *	   the blob. Both of them need to match exactly what the driver is
>> + *	   expecting, otherwise it fails.
>> + *
>> + *	3) Platform officially supported by xe and out of force-probe. Using
>> + *	   "plat" as a short-name. Except for the different directory, the
>> + *	   behavior is the same as (1). Driver loads the following firmware
>> + *	   blobs from disk:
>> + *
>> + *		- xe/plat_guc_<major>.bin
>> + *		- xe/plat_huc.bin
>> + *
>> + *	   <major> number for GuC is checked that it matches the version inside
>> + *	   the blob. <minor> version is checked and if smaller than the expected
>> + *	   an info message is emitted about that.
>> + *
>> + * For the platforms already released with a major version, they
>> +should never be
>> + * removed from the table. Instead new entries with newer versions
>> +may be added
>> + * before them, so they take precedence.
>> + *
>> + * TODO: Currently there's no fallback on major version. That's
>> +because xe
>> + * driver only supports the one major version of each firmware in the table.
>> + * This needs to be fixed when the major version of GuC is updated.
>> + */
>> +
>> +struct uc_fw_entry {
>> +	enum xe_platform platform;
>> +	struct {
>> +		const char *path;
>> +		u16 major;
>> +		u16 minor;
>> +		bool full_ver_required;
>> +	};
>> +};
>> +
>> +struct fw_blobs_by_type {
>> +	const struct uc_fw_entry *entries;
>> +	u32 count;
>> +};
>> +
>> +#define XE_GUC_FIRMWARE_DEFS(fw_def, mmp_ver, major_ver)
>> 		\
>> +	fw_def(METEORLAKE,	mmp_ver(  i915,	guc,	mtl,	70, 6, 4))
>> 	\
>> +	fw_def(PVC,		mmp_ver(  xe,	guc,	pvc,	70, 6, 4))
>> 	\
>> +	fw_def(ALDERLAKE_P,	major_ver(i915,	guc,	adlp,	70, 5))
>> 	\
>> +	fw_def(ALDERLAKE_S,	major_ver(i915,	guc,	tgl,	70, 5))
>> 	\
>> +	fw_def(DG2,		major_ver(i915,	guc,	dg2,	70, 5))
>> 	\
>> +	fw_def(DG1,		major_ver(i915,	guc,	dg1,	70, 5))
>> 	\
>> +	fw_def(TIGERLAKE,	major_ver(i915,	guc,	tgl,	70, 5))
>> +
>> +#define XE_HUC_FIRMWARE_DEFS(fw_def, mmp_ver, no_ver)
>> 		\
>> +	fw_def(ALDERLAKE_S,	no_ver(i915,	huc,	tgl))
>> 	\
>> +	fw_def(DG1,		no_ver(i915,	huc,	dg1))
>> 	\
>> +	fw_def(TIGERLAKE,	no_ver(i915,	huc,	tgl))
>> +
>> +#define MAKE_FW_PATH(dir__, uc__, shortname__, version__)
>> 	\
>> +	__stringify(dir__) "/" __stringify(shortname__) "_"
>> +__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)
>> 	\
>> +	MAKE_FW_PATH(dir_, uc_, shortname_, "_" __stringify(a)) #define
>> +fw_filename_no_ver(dir_, uc_, shortname_)
>> 	\
>> +	MAKE_FW_PATH(dir_, uc_, shortname_, "")
>> +
>> +#define uc_fw_entry_mmp_ver(dir_, uc_, shortname_, a, b, c)
>> 	\
>> +	{ fw_filename_mmp_ver(dir_, uc_, shortname_, a, b, c),
>> 	\
>> +	  a, b, true }
>> +#define uc_fw_entry_major_ver(dir_, uc_, shortname_, a, b)
>> 	\
>> +	{ fw_filename_major_ver(dir_, uc_, shortname_, a, b),
>> 	\
>> +	  a, b }
>Why is b required here?

because it is setting the minor in the corresponding struct uc_fw_entry.
 From the tables above, basically for the rows using major_ver(), it will use up to
the major version in the arguments to decide what is the
*file*  to load. The path for the file is constructed with the macro above, so it
can be used by both MODULE_FIRMWARE and by setting the patch in the
uc_fw_entry.  The same major_ver() is used to fill out the rest of the
uc_fw_entry, where we need the minor too.

See doucumentation above. Copying the relevant part here:

	<major> number for GuC is checked that it matches the version inside
	the blob. <minor> version is checked and if smaller than the expected
	an info message is emitted about that.


Lucas De Marchi

Thanks for the explanation. The auto select logic looks good.
With the extra lines removed from commit message,

Reviewed-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>


thanks, applied

Lucas De Marchi



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux