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

[John Harrison]

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 at 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.

> +
>   /*
>    * 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.

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);