Re: [PATCH v2 06/12] drm/xe/pxp: Add GSC session initialization support

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

On 11/7/2024 14:37, Daniele Ceraolo Spurio wrote:
> On 10/8/2024 11:43 AM, John Harrison wrote:
>> On 8/16/2024 12:00, Daniele Ceraolo Spurio wrote:
>>> A session is initialized (i.e. started) by sending a message to the 
>>> GSC.
>>>
>>> Note that this patch is meant to be squashed with the follow-up patches
>>> that implement the other pieces of the session initialization and queue
>>> setup flow. It is separate for now for ease of review.
>>>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/abi/gsc_pxp_commands_abi.h | 21 ++++++++
>>>   drivers/gpu/drm/xe/xe_pxp_submit.c            | 50 
>>> +++++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_pxp_submit.h            |  1 +
>>>   3 files changed, 72 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/abi/gsc_pxp_commands_abi.h 
>>> b/drivers/gpu/drm/xe/abi/gsc_pxp_commands_abi.h
>>> index 4a59c564a0d0..734feb38f570 100644
>>> --- a/drivers/gpu/drm/xe/abi/gsc_pxp_commands_abi.h
>>> +++ b/drivers/gpu/drm/xe/abi/gsc_pxp_commands_abi.h
>>> @@ -50,6 +50,7 @@ struct pxp_cmd_header {
>>>   } __packed;
>>>     #define PXP43_CMDID_INVALIDATE_STREAM_KEY 0x00000007
>>> +#define PXP43_CMDID_INIT_SESSION 0x00000036
>>>   #define PXP43_CMDID_NEW_HUC_AUTH 0x0000003F /* MTL+ */
>>>     /* PXP-Input-Packet: HUC Auth-only */
>>> @@ -64,6 +65,26 @@ struct pxp43_huc_auth_out {
>>>       struct pxp_cmd_header header;
>>>   } __packed;
>>>   +/* PXP-Input-Packet: Init PXP session */
>>> +struct pxp43_create_arb_in {
>>> +    struct pxp_cmd_header header;
>>> +        /* header.stream_id fields for vesion 4.3 of Init PXP 
>>> session: */
>>> +        #define PXP43_INIT_SESSION_VALID BIT(0)
>>> +        #define PXP43_INIT_SESSION_APPTYPE BIT(1)
>>> +        #define PXP43_INIT_SESSION_APPID GENMASK(17, 2)
>>> +    u32 protection_mode;
>>> +        #define PXP43_INIT_SESSION_PROTECTION_ARB 0x2
>>> +    u32 sub_session_id;
>>> +    u32 init_flags;
>>> +    u32 rsvd[12];
>>> +} __packed;
>>> +
>>> +/* PXP-Input-Packet: Init PXP session */
>>> +struct pxp43_create_arb_out {
>>> +    struct pxp_cmd_header header;
>>> +    u32 rsvd[8];
>>> +} __packed;
>>> +
>>>   /* PXP-Input-Packet: Invalidate Stream Key */
>>>   struct pxp43_inv_stream_key_in {
>>>       struct pxp_cmd_header header;
>>> diff --git a/drivers/gpu/drm/xe/xe_pxp_submit.c 
>>> b/drivers/gpu/drm/xe/xe_pxp_submit.c
>>> index 41684d666376..c9258c861556 100644
>>> --- a/drivers/gpu/drm/xe/xe_pxp_submit.c
>>> +++ b/drivers/gpu/drm/xe/xe_pxp_submit.c
>>> @@ -26,6 +26,8 @@
>>>   #include "instructions/xe_mi_commands.h"
>>>   #include "regs/xe_gt_regs.h"
>>>   +#define ARB_SESSION 0xF /* TODO: move to UAPI */
>> This same define is now in two separate source files? Even if it 
>> can't be moved to the UAPI header yet it should at least be in an 
>> internal header rather than being replicated.
>
> I thought about it, but couldn't find a clean solution. This define 
> would belong in pxp.h, but that's not included from this file and I 
> didn't want to add the include just for a define that is going away a 
> few patches later. I could put it in pxp_types.h or pxp_submit.h, but 
> if it needs to be in the wrong place anyway I thought I might as well 
> just duplicate it.
> Any preference?
Yeah, I saw that it disappeared again later but it still feels wrong to 
be defining the same thing in multiple places. Also, as per comment in 
later patch, it would also be cleaner to use the correct name from the 
start rather than having extra deltas to rename it later.

Not entirely convinced it can't be added to the correct header file in 
advance of the rest of the API arriving. It's not like it's tentative or 
won't be landing in some much later patch series. But failing that, 
pxp.h seems like the right place. Is adding and removing a #include 
really worse than adding and removing a #define?

John.

>
>>
>>> +
>>>   /*
>>>    * The VCS is used for kernel-owned GGTT submissions to issue key 
>>> termination.
>>>    * Terminations are serialized, so we only need a single queue and 
>>> a single
>>> @@ -470,6 +472,54 @@ static int gsccs_send_message(struct 
>>> xe_pxp_gsc_client_resources *gsc_res,
>>>       return ret;
>>>   }
>>>   +/**
>>> + * xe_pxp_submit_session_init - submits a PXP GSC session 
>>> initialization
>>> + * @gsc_res: the pxp client resources
>>> + * @id: the session to initialize
>>> + *
>>> + * Submit a message to the GSC FW to initialize (i.e. start) a PXP 
>>> session.
>>> + *
>>> + * Returns 0 if the submission is successful, an errno value 
>>> otherwise.
>>> + */
>>> +int xe_pxp_submit_session_init(struct xe_pxp_gsc_client_resources 
>>> *gsc_res, u32 id)
>>> +{
>>> +    struct xe_device *xe = gsc_res->vm->xe;
>>> +    struct pxp43_create_arb_in msg_in = {0};
>>> +    struct pxp43_create_arb_out msg_out = {0};
>>> +    int ret;
>>> +
>>> +    msg_in.header.api_version = PXP_APIVER(4, 3);
>>> +    msg_in.header.command_id = PXP43_CMDID_INIT_SESSION;
>>> +    msg_in.header.stream_id = (FIELD_PREP(PXP43_INIT_SESSION_APPID, 
>>> id) |
>>> +                   FIELD_PREP(PXP43_INIT_SESSION_VALID, 1) |
>>> +                   FIELD_PREP(PXP43_INIT_SESSION_APPTYPE, 0));
>>> +    msg_in.header.buffer_len = sizeof(msg_in) - sizeof(msg_in.header);
>>> +
>>> +    if (id == ARB_SESSION)
>>> +        msg_in.protection_mode = PXP43_INIT_SESSION_PROTECTION_ARB;
>>> +
>>> +    ret = gsccs_send_message(gsc_res, &msg_in, sizeof(msg_in),
>>> +                 &msg_out, sizeof(msg_out));
>>> +    if (ret) {
>>> +        drm_err(&xe->drm, "Failed to init session %d, ret=[%d]\n", 
>>> id, ret);
>> %pe for error code
>>
>>> +    } else if (msg_out.header.status != 0) {
>>> +        if (is_fw_err_platform_config(msg_out.header.status)) {
>>> +            drm_info_once(&xe->drm,
>>> +                      "PXP init-session-%d failed due to 
>>> BIOS/SOC:0x%08x:%s\n",
>> Style mis-match - "init session %d" in the first error but then 
>> "init-session-%d" in this one and the one below (I prefer the first 
>> one that actually looks like an operation rather than variable).
>>
>>> +                      id, msg_out.header.status,
>>> + fw_err_to_string(msg_out.header.status));
>>> +        } else {
>>> +            drm_dbg(&xe->drm, "PXP init-session-%d failed 
>>> 0x%08x:%st:\n",
>>> +                id, msg_out.header.status,
>>> +                fw_err_to_string(msg_out.header.status));
>>> +            drm_dbg(&xe->drm, "     cmd-detail: 
>>> ID=[0x%08x],API-Ver-[0x%08x]\n",
>> More mis-matching message styles - 'SOC:%s:%s' vs 'ID=[%x]'. Neither 
>> of which is the normal format of kernel messages.
>
> Those I've copied straight from i915. Will reword.
>
> Daniele
>
>>
>> John.
>>
>>> + msg_in.header.command_id, msg_in.header.api_version);
>>> +        }
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   /**
>>>    * xe_pxp_submit_session_invalidation - submits a PXP GSC 
>>> invalidation
>>>    * @gsc_res: the pxp client resources
>>> diff --git a/drivers/gpu/drm/xe/xe_pxp_submit.h 
>>> b/drivers/gpu/drm/xe/xe_pxp_submit.h
>>> index 48fdc9b09116..c9efda02f4b0 100644
>>> --- a/drivers/gpu/drm/xe/xe_pxp_submit.h
>>> +++ b/drivers/gpu/drm/xe/xe_pxp_submit.h
>>> @@ -14,6 +14,7 @@ struct xe_pxp_gsc_client_resources;
>>>   int xe_pxp_allocate_execution_resources(struct xe_pxp *pxp);
>>>   void xe_pxp_destroy_execution_resources(struct xe_pxp *pxp);
>>>   +int xe_pxp_submit_session_init(struct xe_pxp_gsc_client_resources 
>>> *gsc_res, u32 id);
>>>   int xe_pxp_submit_session_termination(struct xe_pxp *pxp, u32 id);
>>>   int xe_pxp_submit_session_invalidation(struct 
>>> xe_pxp_gsc_client_resources *gsc_res,
>>>                          u32 id);
>