[Intel-xe] [PATCH] drm/xe: Add patch version on guc firmware init

[Ceraolo Spurio, Daniele]

On 8/16/2023 5:48 PM, Dong, Zhanjun wrote:
>
>> -----Original Message-----
>> From: Ceraolo Spurio, Daniele <daniele.ceraolospurio at intel.com>
>> Sent: August 16, 2023 4:47 PM
>> To: Dong, Zhanjun <zhanjun.dong at intel.com>; intel-xe at lists.freedesktop.org
>> Subject: Re: [Intel-xe] [PATCH] drm/xe: Add patch version on guc firmware init
>>
>>
>>
>> On 8/15/2023 4:56 PM, Zhanjun Dong wrote:
>>> Add patch version info on GuC firmware init. This is required info for
>>> GuC log decoder.
>>>
>>> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
>>> ---
>>>    drivers/gpu/drm/xe/xe_uc_fw.c       | 7 +++++--
>>>    drivers/gpu/drm/xe/xe_uc_fw_types.h | 2 ++
>>>    2 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c
>> b/drivers/gpu/drm/xe/xe_uc_fw.c
>>> index 6c95a3e4c3f2..408b816f9e40 100644
>>> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>>> @@ -403,11 +403,14 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
>>>    					   css->sw_version);
>>>    	uc_fw->minor_ver_found = FIELD_GET(CSS_SW_VERSION_UC_MINOR,
>>>    					   css->sw_version);
>>> +	uc_fw->patch_ver_found = FIELD_GET(CSS_SW_VERSION_UC_PATCH,
>>> +					   css->sw_version);
>>>
>>> -	drm_info(&xe->drm, "Using %s firmware (%u.%u) from %s\n",
>>> +	drm_info(&xe->drm, "Using %s firmware (%u.%u) from %s
>> version %u.%u.%u\n",
>>>    		 xe_uc_fw_type_repr(uc_fw->type),
>>>    		 uc_fw->major_ver_found, uc_fw->minor_ver_found,
>>> -		 uc_fw->path);
>>> +		 uc_fw->path,
>>> +		 uc_fw->major_ver_found, uc_fw->minor_ver_found, uc_fw-
>>> patch_ver_found);
>> Why log major an minor twice? can't you just add the patch number where
>> major and minor are already logged?
> Yes, it log twice. I'm not sure if there are any test case is looking for the existing format of:
> Using %s firmware (%u.%u)
> While the decode is looking for the format of:
> version %u.%u.%u
> So log it twice will make both side happy.
>
> Of course, if no test case or test script is looking for this format, we could simply switch to typical i915 FW version print format to avoid output twice.

If there are any scripts they can be updated. dmesg output is not API so 
we shouldn't try to keep it backward compatible. Just log it once with 
the 3 numbers

>
>> Also, do we need to update xe_uc_fw_print to include the patch number as
>> well?
> For this line printed in xe_uc_fw_print:
> drm_printf(p, "\tversion: wanted %u.%u, found %u.%u\n",
> 		   uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted,
> 		   uc_fw->major_ver_found, uc_fw->minor_ver_found);
> As long as we have the FW bin filename is xx.yy and the above format follows bin filename format, I think this is fine.

This is going to end up in the debugfs info. If we don't have dmesg from 
boot then having the patch info available in debugfs when capturing the 
GuC logs would be useful IMO.

Daniele

>
>> Daniele
>>
>>>    	err = uc_fw_check_version_requirements(uc_fw);
>>>    	if (err)
>>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw_types.h
>> b/drivers/gpu/drm/xe/xe_uc_fw_types.h
>>> index 837f49a2347e..444bff83cdbe 100644
>>> --- a/drivers/gpu/drm/xe/xe_uc_fw_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_uc_fw_types.h
>>> @@ -106,6 +106,8 @@ struct xe_uc_fw {
>>>    	u16 major_ver_found;
>>>    	/** @minor_ver_found: major version found in firmware blob */
>>>    	u16 minor_ver_found;
>>> +	/** @patch_ver_found: patch version found in firmware blob */
>>> +	u16 patch_ver_found;
>>>
>>>    	/** @rsa_size: RSA size */
>>>    	u32 rsa_size;