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

[Ceraolo Spurio, Daniele]

On 3/21/2023 3:48 PM, Srivatsa, Anusha wrote:
> +Daniele
>
>> -----Original Message-----
>> From: Srivatsa, Anusha
>> Sent: Tuesday, March 21, 2023 3:24 PM
>> To: Roper, Matthew D <matthew.d.roper at intel.com>; De Marchi, Lucas
>> <lucas.demarchi at intel.com>
>> Cc: intel-xe at lists.freedesktop.org
>> Subject: RE: [Intel-xe] [PATCH 2/2] drm/xe/huc: Macro support for loading
>> unversiond HuC
>>
>>
>>
>>> -----Original Message-----
>>> From: Roper, Matthew D <matthew.d.roper at intel.com>
>>> Sent: Tuesday, March 21, 2023 2:49 PM
>>> To: De Marchi, Lucas <lucas.demarchi at intel.com>
>>> Cc: Srivatsa, Anusha <anusha.srivatsa at intel.com>; intel-
>>> xe at lists.freedesktop.org
>>> Subject: Re: [Intel-xe] [PATCH 2/2] drm/xe/huc: Macro support for
>>> loading unversiond HuC
>>>
>>> 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?
>>>
>> Yeah if we go down the route of wanting to load unversioned huC. Then we
>> should also override the version check for HUC like we do in i915(this change is
>> part of v2 of this patch that I was just about to send).
>>
>> Else we see:
>> [  292.632408] xe 0000:00:02.0: [drm] HuC firmware i915/tgl_huc.bin:
>> unexpected version: 7.9 != 0.0 [  292.632415] xe 0000:00:02.0: [drm] HuC
>> firmware i915/tgl_huc.bin: fetch failed with error -8
>>
>> Because it get 0 and 0 fro mthe table but it finds the 7.9 version which is actually
>> right but we should not have the check at all for HuC.
>>
>> Anusha
>>
>>> Matt
>>>
>>>> Later when we are parsing the blob we read the version that is
>>>> encoded inside the blob (rather than on the filename).  See xe_uc_fw_init().
>>>> I think the version check is still makes sense:
>>>> major should never change and minor has to be equal on greater than
>>>> the one we first started supporting.
>>>>
>>>> The version in the filename needs to be removed, however the version
>>>> in the table I believe can stay... it should basically check the
>>>> minimum version required. We could rename
>>>> s/minor_ver_wanted/minor_ver_minimum/
>>>> to clarify.  And comments on top of the macro definition to document.
>
> @Ceraolo Spurio, Daniele Any thoughts? Should the driver not check huC version at all or just not have it in the file name and still do version checks for sanity purposes?

There is no commitment for HuC to maintain a stable major version. 
Differently to what happens with GuC, we have no interface with HuC and 
therefore no agreement on what the version number represents from a 
feature/interface POV. The version is something that is handled 
exclusively between the HuC and the media driver.

This means that the only check we can do on the HuC version is that the 
version is >= to the one we started with, but IMO that's not worth it so 
my suggestion would be to just skip the version checks entirely for HuC 
after release. It might still be worth keeping the ability to do exact 
matches for early drops (i.e. before force_probe removal) like we have 
on the i915 side, but I think that is a separate topic.

Daniele

>
> Anusha
>>>> Lucas De Marchi
>>> --
>>> Matt Roper
>>> Graphics Software Engineer
>>> Linux GPU Platform Enablement
>>> Intel Corporation