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

[Dong, Zhanjun]

> -----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.

> 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.

> 
> 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;