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 8:29 AM, Daniele Ceraolo Spurio wrote:
>
>
> 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).

I ended up rabbit-holing on this and producing a couple of patches to 
remove gt->sriov.vf.guc_version and use the compatibility version 
directly: https://patchwork.freedesktop.org/series/145667/
Let me know what you think.

Daniele

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