| Message ID | 20230111214226.907536-7-alan.previn.teres.alexis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pxp: Add MTL PXP Support | expand |
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@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
Thanks Daniele - agreed on all counts - will fix accordingly - already done and tested internally - will re-rev shortly. ..alan On Wed, 2023-01-18 at 17:50 -0800, Ceraolo Spurio, Daniele wrote: > > > 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@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(-) alan:snip..
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) + #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 */ 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); + + /* + * 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); + } while (++tries < 12); + + return ret; } static void
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@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(-)