Re: [PATCH 2/2] drm/xe/vf: Custom GuC initialization if VF

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

On 5/20/2024 14:50, Michal Wajdeczko wrote:
> On 20.05.2024 23:30, John Harrison wrote:
>> On 5/20/2024 14:23, Michal Wajdeczko wrote:
>>> The GuC firmware is loaded and initialized by the PF driver. Make
>>> sure VF drivers only perform permitted operations. For submission
>>> initialization, use number of GuC context IDs from self config.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> ---
>>>    drivers/gpu/drm/xe/xe_gt_sriov_vf.c | 17 ++++++++++++
>>>    drivers/gpu/drm/xe/xe_gt_sriov_vf.h |  2 ++
>>>    drivers/gpu/drm/xe/xe_guc.c         | 40 +++++++++++++++++++++++++++++
>>>    3 files changed, 59 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
>>> b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
>>> index 378dde5ad4f9..bf4dfe301a71 100644
>>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
>>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
>>> @@ -408,6 +408,23 @@ int xe_gt_sriov_vf_query_config(struct xe_gt *gt)
>>>        return 0;
>>>    }
>>>    +/**
>>> + * xe_gt_sriov_vf_guc_ids - VF GuC context IDs configuration.
>>> + * @gt: the &xe_gt
>>> + *
>>> + * This function is for VF use only.
>>> + *
>>> + * Return: number of GuC context IDs assigned to VF.
>>> + */
>>> +u16 xe_gt_sriov_vf_guc_ids(struct xe_gt *gt)
>>> +{
>>> +    xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>>> +    xe_gt_assert(gt, gt->sriov.vf.guc_version.major ||
>>> gt->sriov.vf.guc_version.major);
>> I may be going blind but this says 'X || X'? Is the second term supposed
>> to say minor?
> oops, it's me blind (again)
>
>> Is there any need to check past the major version anyway?
>> Xe does not support legacy GuCs from before VF version 1.0.0. So if
>> major is zero, there is a problem.
> this is to make sure that we have completed the GuC handshake (and have
> negotiated major.minor != 0)
Sure, 0.0 is bad but 0.anything is also bad. And anything.0 is valid (if 
major is != 0). So simply asserting major !=0 is sufficient.

John.

>
>> John.
>>
>>> +    xe_gt_assert(gt, gt->sriov.vf.self_config.num_ctxs);
>>> +
>>> +    return gt->sriov.vf.self_config.num_ctxs;
>>> +}
>>> +
>>>    static int relay_action_handshake(struct xe_gt *gt, u32 *major, u32
>>> *minor)
>>>    {
>>>        u32 request[VF2PF_HANDSHAKE_REQUEST_MSG_LEN] = {
>>> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
>>> b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
>>> index 997cb7541036..d6d37b193d17 100644
>>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
>>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.h
>>> @@ -16,6 +16,8 @@ int xe_gt_sriov_vf_query_config(struct xe_gt *gt);
>>>    int xe_gt_sriov_vf_connect(struct xe_gt *gt);
>>>    int xe_gt_sriov_vf_query_runtime(struct xe_gt *gt);
>>>    +u16 xe_gt_sriov_vf_guc_ids(struct xe_gt *gt);
>>> +
>>>    void xe_gt_sriov_vf_print_config(struct xe_gt *gt, struct
>>> drm_printer *p);
>>>    void xe_gt_sriov_vf_print_runtime(struct xe_gt *gt, struct
>>> drm_printer *p);
>>>    void xe_gt_sriov_vf_print_version(struct xe_gt *gt, struct
>>> drm_printer *p);
>>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>>> index 807ad53449e4..cec709a908c3 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>>> @@ -295,6 +295,23 @@ static int xe_guc_realloc_post_hwconfig(struct
>>> xe_guc *guc)
>>>        return 0;
>>>    }
>>>    +static int vf_guc_init(struct xe_guc *guc)
>>> +{
>>> +    int err;
>>> +
>>> +    xe_guc_comm_init_early(guc);
>>> +
>>> +    err = xe_guc_ct_init(&guc->ct);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    err = xe_guc_relay_init(&guc->relay);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>    int xe_guc_init(struct xe_guc *guc)
>>>    {
>>>        struct xe_device *xe = guc_to_xe(guc);
>>> @@ -309,6 +326,13 @@ int xe_guc_init(struct xe_guc *guc)
>>>        if (!xe_uc_fw_is_enabled(&guc->fw))
>>>            return 0;
>>>    +    if (IS_SRIOV_VF(xe)) {
>>> +        ret = vf_guc_init(guc);
>>> +        if (ret)
>>> +            goto out;
>>> +        return 0;
>>> +    }
>>> +
>>>        ret = xe_guc_log_init(&guc->log);
>>>        if (ret)
>>>            goto out;
>>> @@ -342,6 +366,19 @@ int xe_guc_init(struct xe_guc *guc)
>>>        return ret;
>>>    }
>>>    +static int vf_guc_init_post_hwconfig(struct xe_guc *guc)
>>> +{
>>> +    int err;
>>> +
>>> +    err = xe_guc_submit_init(guc,
>>> xe_gt_sriov_vf_guc_ids(guc_to_gt(guc)));
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    /* XXX xe_guc_db_mgr_init not needed for now */
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>    /**
>>>     * xe_guc_init_post_hwconfig - initialize GuC post hwconfig load
>>>     * @guc: The GuC object
>>> @@ -352,6 +389,9 @@ int xe_guc_init_post_hwconfig(struct xe_guc *guc)
>>>    {
>>>        int ret;
>>>    +    if (IS_SRIOV_VF(guc_to_xe(guc)))
>>> +        return vf_guc_init_post_hwconfig(guc);
>>> +
>>>        ret = xe_guc_realloc_post_hwconfig(guc);
>>>        if (ret)
>>>            return ret;