Re: [PATCH 3/3] drm/xe/vf: Set submission version in xe_uc_fw struct

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 2/28/2025 1:25 AM, Laguna, Lukasz wrote:
>
> On 2/27/2025 20:43, Daniele Ceraolo Spurio wrote:
>>
>>
>> On 2/27/2025 3:22 AM, Laguna, Lukasz wrote:
>>>
>>> On 2/25/2025 23:30, Daniele Ceraolo Spurio wrote:
>>>>
>>>>
>>>> On 2/25/2025 5:23 AM, Lukasz Laguna wrote:
>>>>> The VF driver has already negotiated the ABI version with GuC. What
>>>>> remains is to populate the generic xe_uc_fw struct with the version
>>>>> before initializing submission.
>>>>>
>>>>> Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/xe/xe_guc.c | 9 ++++++++-
>>>>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_guc.c 
>>>>> b/drivers/gpu/drm/xe/xe_guc.c
>>>>> index bc1ff0a4e1e7..7b38447d902c 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_guc.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>>>>> @@ -703,9 +703,16 @@ int xe_guc_init(struct xe_guc *guc)
>>>>>     static int vf_guc_init_post_hwconfig(struct xe_guc *guc)
>>>>>   {
>>>>> +    struct xe_gt *gt = guc_to_gt(guc);
>>>>> +    struct xe_uc_fw_version ver;
>>>>>       int err;
>>>>>   -    err = xe_guc_submit_init(guc, 
>>>>> xe_gt_sriov_vf_guc_ids(guc_to_gt(guc)));
>>>>> +    err = xe_gt_sriov_vf_get_guc_ver(gt, &ver);
>>>>> +    xe_gt_assert(gt, !err);
>>>>> +
>>>>> +    xe_uc_fw_set_compatibility_ver(&guc->fw, &ver);
>>>>
>>>> Why not just pass &gt->sriov.vf.guc_version here, instead of 
>>>> copying it into another variable first?
>>>
>>> By using xe_gt_sriov_vf_get_guc_ver(), we additionally verify 
>>> whether the handshake completed successfully (major > 0). Only in 
>>> that case we set the version in xe_uc_fw.
>>
>> But it's impossible to get here with a wrong major value, because 
>> xe_gt_sriov_vf_bootstrap() would fail. If you really want to have an 
>> emergency check then IMO it'd be better to just have:
>>
>> xe_assert(xe, gt->sriov.vf.guc_version.major);
>>
>> which you can have in this function without the need for an helper 
>> (the same assert exists in other SRIOV functions like 
>> xe_gt_sriov_vf_lmem).
>>
>
> Next thing is that gt->sriov.vf.guc_version is a different struct 
> (struct xe_gt_sriov_vf_guc_version). xe_gt_sriov_vf_get_guc_ver() sets 
> version in generic xe_uc_fw_version structure format.

Ok I missed that. It makes no sense though for them to be different 
types, because they're meant to hold the exact same information. 
Thinking more on it, why does vf.guc_version even exist as a separate 
variable to begin with? Can't you just store the VF info directly into 
the compatibility struct when you handshake? And if the xe_uc_fw_version 
is missing something (like the branch variable), that needs to be fixed, 
not worked around, because if we ever get a release with a non-zero 
branch value all the runtime checks will break (because they're done on 
the compatibility struct, see the GUC_SUBMIT_VER() macro).

Daniele

>
> Lukasz
>
>> Daniele
>>
>>>
>>> Lukasz
>>>
>>>>
>>>> Daniele
>>>>
>>>>> +
>>>>> +    err = xe_guc_submit_init(guc, xe_gt_sriov_vf_guc_ids(gt));
>>>>>       if (err)
>>>>>           return err;
>>>>
>>