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

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

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?

>
>> +
>>   /*
>>    * 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);