[Intel-xe] [PATCH 2/2] drm/xe/huc: Macro support for loading unversiond HuC

[Lucas De Marchi]

On Tue, Mar 21, 2023 at 02:48:58PM -0700, Matt Roper wrote:
>On Tue, Mar 21, 2023 at 02:03:49PM -0700, Lucas De Marchi wrote:
>> On Tue, Mar 21, 2023 at 08:50:59AM -0700, Matt Roper wrote:
>> > On Mon, Mar 20, 2023 at 04:52:54PM -0700, Anusha Srivatsa wrote:
>> > > Follow the new direction of firmware and add macro
>> > > support for loading unversioned HuC.
>> > >
>> > > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa at intel.com>
>> > > ---
>> > >  drivers/gpu/drm/xe/xe_uc_fw.c | 16 +++++++++++++---
>> > >  1 file changed, 13 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
>> > > index e2e5f7fbe9db..3c522d970228 100644
>> > > --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>> > > +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>> > > @@ -50,7 +50,7 @@ static struct xe_device *uc_fw_to_xe(struct xe_uc_fw *uc_fw)
>> > >  	fw_def(DG1,          0, guc_def(dg1,  70, 5, 2)) \
>> > >  	fw_def(TIGERLAKE,    0, guc_def(tgl,  70, 5, 2))
>> > >
>> > > -#define XE_HUC_FIRMWARE_DEFS(fw_def, huc_def) \
>> > > +#define XE_HUC_FIRMWARE_DEFS(fw_def, huc_raw, huc_def) \
>> >
>> > Why do we still need the old versioned filename?  Can't we get rid of it
>> > completely?  There's no backwards compatibility necessary on Xe yet so
>> > this is our chance to eliminate the version numbers on HuC completely.
>> > It's not clear to me why we're still using huc_def instead of huc_raw
>> > below either.
>>
>> note the version is saved to uc_fw->major_ver_wanted and
>> uc_fw->minor_ver_wanted.
>
>I thought we didn't want to use those for HuC, only for GuC?  Since the
>kernel doesn't interact with the firmware at all, there's no need for us
>to enforce any kind of minimum version check as far as I can see.  We
>don't even care about the major version number, which is why it isn't
>included in the filename we load, right?

GuC firmware uses a major number because
when/if it changes to a new major, it is expected that the driver
will retain compatibility with the previous version. So we could give
preference to e.g. pvc_guc_71.bin, but we should still fallback
to pvc_guc_70.bin. That is assuming there is a need to break the
compatibility. That should be avoided.

The major/minor is encoded inside the firmware and is still something
we check against for sanity checks.

For HuC, there is no expectation we will ever have a major update after
the first one is released. The version check is there mostly for sanity
check only that we are not trying to load something wrong, like trying
to load a firmware from before we removed the force_probe. Or from a
different platform[1]. From the user point of view, there is no easy way to
see what is wrong if the filename doesn't have the version and the
module doesn't check the version.

This follows https://docs.kernel.org/driver-api/firmware/firmware-usage-guidelines.html
to the letter as far as I can see.

[1] talking about different platform, the >= check is because it should
be ok if a platform N requires the same firmware of platform N-1. What
should not be ok is for platform N-1 to require the firmware from
platform N. In that case we'd probably break the rules from the
guideline above and it's never something we are expecting to do.


Lucas De Marchi