Re: [Intel-xe] [PATCH v3 27/30] drm/xe: Extend uAPI to query HuC micro-controler firmware version

[Francois Dugast <francois.dugast@intel.com>]

On Tue, Oct 03, 2023 at 05:48:07PM -0700, John Harrison wrote:
> On 9/27/2023 10:22, Souza, Jose wrote:
> > + John Harrison
> > 
> > On Wed, 2023-09-27 at 13:04 -0400, Rodrigo Vivi wrote:
> > > On Tue, Sep 26, 2023 at 04:46:36PM +0000, Souza, Jose wrote:
> > > > On Tue, 2023-09-26 at 12:55 +0000, Francois Dugast wrote:
> > > > > The infrastructure to query GuC firmware version is already in place. It
> > > > > is extended with a new micro-controller type to query the HuC firmware
> > > > > version. It can be used from user space to know if HuC is running.
> > > > > 
> > > > > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > > > > ---
> > > > >   drivers/gpu/drm/xe/xe_query.c | 9 +++++++++
> > > > >   include/uapi/drm/xe_drm.h     | 1 +
> > > > >   2 files changed, 10 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > > > > index 7a0ffd9a654a..c250ca534bb9 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_query.c
> > > > > +++ b/drivers/gpu/drm/xe/xe_query.c
> > > > > @@ -530,6 +530,15 @@ query_uc_fw_version(struct xe_device *xe, struct drm_xe_device_query *query)
> > > > >   		resp.branch_ver = 0;
> > > > >   		break;
> > > > >   	}
> > > > > +	case XE_QUERY_UC_TYPE_HUC: {
> > > > > +		struct xe_huc *huc = &xe->tiles[0].primary_gt->uc.huc;
> > > > > +
> > > > > +		resp.major_ver = huc->fw.major_ver_found;
> > > > > +		resp.minor_ver = huc->fw.minor_ver_found;
> > > > > +		resp.patch_ver = huc->fw.patch_ver_found;
> > > > Have you confirmed that HuC will not have something like submission version like GuC have?
> > > Nah... GuC is the only complicated fw in our set of fw...
> HuC has no split interface. It is only ever accessed by the UMD from a batch
> buffer. The KMD has no dealings with the HuC beyond providing the firmware
> image to whatever entity does the loading (GuC or GSC according to
> platform). So no need to multiple interface versions.

Many thanks John for the explanations here and below.

Jose: with this, back to the original submission, it seems returning
just the firmware version for HuC is correct, right?

Francois

> 
> > > 
> > > > At least in GuC, when running in SRIOV mode the VFs will not have access to the actual GuC version, that is why it have submission version.
> > > > 
> > > > Not sure if providing a complete different firmware version from one kernel version to other would be considered a uAPI break...
> > > hmmm... but now what I'm asking myself is if we shouldn't move the guc one to
> > > have the current loaded firmware and create a special category for the
> > > submission version:
> > > 
> > > XE_QUERY_UC_TYPE_GUC
> > > XE_QUERY_UC_TYPE_GUC_SUBMISSION
> > > XE_QUERY_UC_TYPE_HUC
> > I don't think any UMD would fetch the actual GUC FW version and risk fail when running under SRIOV VF.
> > If needed we can map a submission version to a actual version...
> > 
> > > But to be really really honest, there's something really fishy on this
> > > submission version. Why the VF cannot read the running firmware and
> > > get the submission version from there?
> > Got this information from John, he can explain it better.
> Because the VF does not need to know the master version number.
> 
> Especially when you get in to VF migration and such. The VF could start
> executing with one back end GuC version but then be migrated to a system
> with a completely different back end GuC version. As long as the submission
> API is compatible then the VF doesn't care. However, the PF that is managing
> the GuC very definitely needs to know how to manage that specific GuC
> version. Even ignoring live migration, an end customer may have a specific
> OS image that they have validated and tested and want to run on some cloud
> server system. The cloud provider may need to update the GuC version to take
> security fixes. But the customer's image should not have to change as a
> result. The GuC update must be backwards compatible at the VF level even if
> it is backwards breaking at the PF level.
> 
> In short, the GuC presents two completely separate APIs. One for management
> that is only visible to the PF and one for clients/submission that is
> visible to the VF (and directly to the UMDs if we ever support direct
> submission, plus indirectly to the UMDs via bugs!). On native, the KMD sees
> everything so technically only one version is required for native. But for
> SRIOV, the two interfaces are totally separate. A VF KMD does not have
> access to the management interfaces and does not care what master version
> the GuC is. It only cares that the client interface matches what it knows
> about. Likewise a UMD. Therefore, we need two completely separate interface
> version numbers. And we need to be very careful that nothing uses the master
> version when it should be using the submission version. Otherwise, stuff
> will break when it starts to run in a VF.
> 
> Whether it is useful to return the master version via this query interface
> is debatable. There should be no functional requirement for it. Any UMD code
> should only care about the client interface/behaviour and so should only
> need the submission interface version number. Potentially we might want to
> report the master version to the end user via some control panel information
> tool thing. But that should be the only purpose for it.
> 
> John.
> 
> 
> > > > > +		resp.branch_ver = 0;
> > > > > +		break;
> > > > > +	}
> > > > >   	default:
> > > > >   		return -EINVAL;
> > > > >   	}
> > > > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > > > index 84091860c7d2..fe7e83a5bd3e 100644
> > > > > --- a/include/uapi/drm/xe_drm.h
> > > > > +++ b/include/uapi/drm/xe_drm.h
> > > > > @@ -478,6 +478,7 @@ struct drm_xe_query_topology_mask {
> > > > >   struct drm_xe_query_uc_fw_version {
> > > > >   	/** @uc: The micro-controller type to query firmware version */
> > > > >   #define XE_QUERY_UC_TYPE_GUC 0
> > > > > +#define XE_QUERY_UC_TYPE_HUC 1
> > > > >   	__u16 uc_type;
> > > > >   	/** @pad: MBZ */
>