[PATCH v2 6/9] drm/i915/pxp: Add ARB session creation with new PXP API Ver4.3
Ceraolo Spurio, Daniele
daniele.ceraolospurio at intel.com
Thu Jan 19 01:50:14 UTC 2023
On 1/11/2023 1:42 PM, Alan Previn wrote:
> Add MTL's function for ARB session creation using PXP firmware
> version 4.3 ABI structure format.
>
> Before checking the return status, look at the GSC-CS-Mem-Header's
> pending-bit which means the GSC firmware is busy and we should
> resubmit.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis at intel.com>
> ---
> .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 21 +++++++
> drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 56 ++++++++++++++++++-
> 2 files changed, 74 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> index 52b9a61bcdd4..ee78c0817ba1 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> @@ -11,6 +11,7 @@
>
> /* PXP-Cmd-Op definitions */
> #define PXP43_CMDID_START_HUC_AUTH 0x0000003A
> +#define PXP43_CMDID_INIT_SESSION 0x00000036
>
> /* PXP-Packet sizes for MTL's GSCCS-HECI instruction */
> #define PXP43_MAX_HECI_IN_SIZE (SZ_32K)
> @@ -27,4 +28,24 @@ struct pxp43_start_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 GENMASK(0, 0)
GENMASK(0, 0) -> BIT(0) ? same for (1, 1)
> + #define PXP43_INIT_SESSION_APPTYPE GENMASK(1, 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;
> +
> #endif /* __INTEL_PXP_FW_INTERFACE_43_H__ */
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> index ff235822743e..1b06629ac16e 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> @@ -43,7 +43,8 @@ static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp
> static int gsccs_send_message(struct intel_pxp *pxp,
> void *msg_in, size_t msg_in_size,
> void *msg_out, size_t msg_out_size_max,
> - size_t *msg_out_len)
> + size_t *msg_out_len,
> + u64 *gsc_msg_handle_retry)
> {
> struct intel_gt *gt = pxp->ctrl_gt;
> struct drm_i915_private *i915 = gt->i915;
> @@ -75,6 +76,9 @@ static int gsccs_send_message(struct intel_pxp *pxp,
> msg_in_size + sizeof(*header),
> exec->host_session_handle);
>
> + /* copy caller provided gsc message handle if this is polling for a prior msg completion */
> + header->gsc_message_handle = *gsc_msg_handle_retry;
> +
> memcpy(exec->pkt_vaddr + sizeof(*header), msg_in, msg_in_size);
>
> pkt.addr_in = i915_vma_offset(exec->pkt_vma);
> @@ -91,7 +95,7 @@ static int gsccs_send_message(struct intel_pxp *pxp,
> goto unlock;
> }
>
> - /* we keep separate location for reply, so get the response header loc first */
> + /* we keep separate location for reply, so go to the response header now */
Any reason to update the comment in this patch and not directly in the
original one?
> header = exec->pkt_vaddr + PXP43_MAX_HECI_IN_SIZE;
>
> /* Response validity marker, status and busyness */
> @@ -108,6 +112,13 @@ static int gsccs_send_message(struct intel_pxp *pxp,
> }
> if (header->flags & GSC_HECI_FLAG_MSG_PENDING) {
> drm_dbg(&i915->drm, "gsc PXP reply is busy\n");
> + /*
> + * When the GSC firmware replies with pending bit, it means that the requested
> + * operation has begun but the completion is pending and the caller needs
> + * to re-request with the gsc_message_handle that was returned by the firmware.
> + * until the pending bit is turned off.
> + */
> + *gsc_msg_handle_retry = header->gsc_message_handle;
> ret = -EAGAIN;
> goto unlock;
> }
> @@ -135,7 +146,46 @@ static int gsccs_send_message(struct intel_pxp *pxp,
> int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
> int arb_session_id)
> {
> - return -ENODEV;
> + struct gsccs_session_resources *exec = &pxp_to_gsccs_priv(pxp)->arb_exec_res;
> + struct pxp43_create_arb_in msg_in = {0};
> + struct pxp43_create_arb_out msg_out = {0};
> + u64 gsc_session_retry = 0;
> + int insize, outsize, ret, tries = 0;
> + void *inptr, *outptr;
> +
> + /* get a unique host-session-handle (used later in HW cmds) at time of session creation */
> + get_random_bytes(&exec->host_session_handle, sizeof(exec->host_session_handle));
> +
> + 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, arb_session_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);
> + msg_in.protection_mode = PXP43_INIT_SESSION_PROTECTION_ARB;
> +
> + inptr = &msg_in;
> + outptr = &msg_out;
> + insize = sizeof(msg_in);
> + outsize = sizeof(msg_out);
Are those local vars required? Can't you just pass the values directly?
it doesn't look like you're saving many characters.
> +
> + /*
> + * Keep sending request if GSC firmware was busy.
> + * Based on test data, we expects a worst case delay of 250 milisecs.
> + */
> + do {
> + ret = gsccs_send_message(pxp,
> + inptr, insize,
> + outptr, outsize, NULL,
> + &gsc_session_retry);
> + /* Only try again if gsc says so */
> + if (ret != -EAGAIN)
> + break;
> +
> + msleep(20);
I seem to remember that the recommended sleep time was 50ms, but can't
find that in the specs now.
> + } while (++tries < 12);
Found a note in the specs that says we should give up retrying after 2
secs, so should probably adjust the retry count accordingly.
Daniele
> +
> + return ret;
> }
>
> static void
More information about the dri-devel
mailing list