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

[John Harrison <john.c.harrison@intel.com>]

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.

>>
>>> 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 */