On 9/16/2022 02:10, Tvrtko Ursulin wrote:
On 15/09/2022 21:03, John Harrison wrote:
On 9/15/2022 01:59, Tvrtko Ursulin wrote:
Hi,
On 15/09/2022 00:46, John.C.Harrison@xxxxxxxxx wrote:
From: John Harrison <John.C.Harrison@xxxxxxxxx>
Going forwards, the intention is for GuC firmware files to be named
for their major version only and HuC firmware files to have no version
number in the name at all. This patch adds those entries for all
platforms that are officially GuC/HuC enabled.
Also, update the expected GuC version numbers to the latest firmware
release for those platforms.
Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
---
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index 1169e2a09da24..b91ad4aede1f7 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -72,12 +72,14 @@ void intel_uc_fw_change_status(struct
intel_uc_fw *uc_fw,
* security fixes, etc. to be enabled.
*/
#define INTEL_GUC_FIRMWARE_DEFS(fw_def, guc_maj, guc_mmp) \
- fw_def(DG2, 0, guc_mmp(dg2, 70, 4, 1)) \
+ fw_def(DG2, 0, guc_maj(dg2, 70, 5)) \
Just glancing over out of curiosity. Part which confused me is that
if only major is supposed to be used then what is the '5' in
guc_maj(dg2, 70, 5) ?
See the earlier patch that added support for version reduced
filenames. The minor number is still specified because want to be
able to warn the user if their firmware is out of date and causing
them to miss features, security fixes, etc. The driver will still
load any old firmware with the right name and work with it, but
user's need to know that there are updates available.
Got it. Release is deemed not important enough to warn about? no
actually, it's different, I guess we never expect to bump only the
release with a source level change - so in practice kernel could not
warn that there is a newer release version since it would never know.
In other words those ones would only be hitting linux-firmware, while
minor changes would be kernel patches as well.
By "release" you mean patch level? As in <MAJOR.MINOR.PATCH>. Yeah, I'm
in two minds about being explicit on the patch level. It is possible
that a particular feature might be added in 70.6.0 but then we find a
bug, so it's only usable in 70.6.1. In which case, we want the KMD to
notify users that an update is required. But in that case, we would also
want to patch the kernel to disable that feature unless 70.6.1 or later
is actually found.
Or, there is just a straight up bug that affects everything and is fixed
in 70.5.2. Again, we would want to notify that an update is available.
But we can't exactly patch the kernel to not load i915 if 70.5.2 is not
available.
Having said that, updating the 'requested' version is a KMD change
regardless of whether it is a bump of the major, minor or patch level
version number. In which case, we have the opportunity to add in patch
level reporting at that point if required.
But the counter argument is that, as you say, a firmware bug fix does
not generally require an actual KMD code change. The i915 patch would
purely be to bump the version number. So is it worth the churn? For
major/minor changes there are corresponding i915 changes - either a
re-write of existing code (major) or a new feature being added (minor).
So bumping the number is 'free' given that a patch is required regardless.
And is the code churn to provide a warning really useful? Would an end
user ever notice the line in dmesg? Isn't it up to the distros to
provide important bug fixes via package updates when necessary?
So yeah, not sure. Unnecessary code churn versus warning about
potentially important bug fixes. Pick your evil.
I also couldn't find guc_maj with grep so I guess it's some sort of
a magic concatenation macro or what?
'guc_maj' is a macro parameter as per the definition of the macro
three lines above. According to where INTEL_GUC_FIRMWARE_DEFS is
used, it becomes either a mechanism for creating just a
'MODULE_FIRMWARE' definition for the firmware file or a table entry
giving all the version information as well as the filename.
Doh thanks, macro magic was apparently impenetrable to me yesterday.
Yeah, this particular set of macro magic is impressively complex. It
took me quite a while to properly understand exactly what is happening
and how when I did the actual versionless filename updates in the first
place!
John.
Regards,
Tvrtko
