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

["Laguna, Lukasz" <lukasz.laguna@intel.com>]

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.

Lukasz

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