| Message ID | 20230406174419.471256-6-alan.previn.teres.alexis@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/pxp: Add MTL PXP Support | expand |
On 4/6/2023 10:44 AM, Alan Previn wrote: > Add MTL's function for ARB session creation using PXP firmware > version 4.3 ABI structure format. > > Also add MTL's function for ARB session invalidation but this > reuses PXP firmware version 4.2 ABI structure format. > > For both cases, in the back-end gsccs functions for sending messages > to the firmware inspect the GSC-CS-Mem-Header's pending-bit which > means the GSC firmware is busy and we should retry. > > Given the last hw requirement, lets also update functions in > front-end layer that wait for session creation or teardown > completion to use new worst case timeout periods. > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 10 ++ > drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 1 + > drivers/gpu/drm/i915/pxp/intel_pxp.c | 28 +++- > .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 21 +++ > drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 130 ++++++++++++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h | 8 +- > drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 11 +- > 7 files changed, 202 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c > index 1d9fdfb11268..85f720af2f75 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c > @@ -13,6 +13,7 @@ > #define GSC_FW_STATUS_REG _MMIO(0x116C40) > #define GSC_FW_CURRENT_STATE REG_GENMASK(3, 0) > #define GSC_FW_CURRENT_STATE_RESET 0 > +#define GSC_FW_PROXY_STATE_NORMAL 5 > #define GSC_FW_INIT_COMPLETE_BIT REG_BIT(9) > > static bool gsc_is_in_reset(struct intel_uncore *uncore) > @@ -31,6 +32,15 @@ bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc) > return fw_status & GSC_FW_INIT_COMPLETE_BIT; > } > > +bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc) > +{ > + struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore; > + u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG); > + > + return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) == > + GSC_FW_PROXY_STATE_NORMAL; > +} > + > static int emit_gsc_fw_load(struct i915_request *rq, struct intel_gsc_uc *gsc) > { > u32 offset = i915_ggtt_offset(gsc->local); > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h > index f4c1106bb2a9..fff8928218df 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h > @@ -13,5 +13,6 @@ struct intel_uncore; > > int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc); > bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc); > +bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc); > > #endif > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c > index 8949d4be7882..550457bbb034 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > @@ -291,6 +291,8 @@ static bool pxp_component_bound(struct intel_pxp *pxp) > > static int __pxp_global_teardown_final(struct intel_pxp *pxp) > { > + int timeout; > + > if (!pxp->arb_is_valid) > return 0; > /* > @@ -300,7 +302,12 @@ static int __pxp_global_teardown_final(struct intel_pxp *pxp) > intel_pxp_mark_termination_in_progress(pxp); > intel_pxp_terminate(pxp, false); > > - if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250))) > + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) > + timeout = GSCFW_MAX_ROUND_TRIP_LATENCY_MS; > + else > + timeout = 250; > + > + if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout))) > return -ETIMEDOUT; > > return 0; > @@ -308,6 +315,8 @@ static int __pxp_global_teardown_final(struct intel_pxp *pxp) > > static int __pxp_global_teardown_restart(struct intel_pxp *pxp) > { > + int timeout; > + > if (pxp->arb_is_valid) > return 0; > /* > @@ -316,7 +325,12 @@ static int __pxp_global_teardown_restart(struct intel_pxp *pxp) > */ > pxp_queue_termination(pxp); > > - if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250))) > + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) > + timeout = GSCFW_MAX_ROUND_TRIP_LATENCY_MS; > + else > + timeout = 250; > + > + if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout))) > return -ETIMEDOUT; > > return 0; > @@ -354,8 +368,14 @@ int intel_pxp_start(struct intel_pxp *pxp) > if (!intel_pxp_is_enabled(pxp)) > return -ENODEV; > > - if (wait_for(pxp_component_bound(pxp), 250)) > - return -ENXIO; > + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) { > + /* Use a larger 1 second timeout considering all the dependencies. */ > + if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 1000)) > + return -ENXIO; This needs a comment to explain that we expect userspace to retry later if we return -ENXIO and therefore the timeout is smaller that what would be required to guarantee that the init can complete. It also needs an ack from the userspace drivers for this behavior. > + } else { > + if (wait_for(pxp_component_bound(pxp), 250)) > + return -ENXIO; > + } > > mutex_lock(&pxp->arb_mutex); > > 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 c65ada99e54f..09777719cd84 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_INOUT_SIZE (SZ_32K) > @@ -26,4 +27,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 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; > + > #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 16e3b73d0653..4bc276daca16 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > @@ -6,13 +6,46 @@ > #include "gem/i915_gem_internal.h" > > #include "gt/intel_context.h" > +#include "gt/uc/intel_gsc_fw.h" > #include "gt/uc/intel_gsc_uc_heci_cmd_submit.h" > > #include "i915_drv.h" > +#include "intel_pxp_cmd_interface_42.h" > #include "intel_pxp_cmd_interface_43.h" > #include "intel_pxp_gsccs.h" > #include "intel_pxp_types.h" > > +static bool > +is_fw_err_platform_config(u32 type) > +{ > + switch (type) { > + case PXP_STATUS_ERROR_API_VERSION: > + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: > + case PXP_STATUS_PLATFCONFIG_KF1_BAD: > + return true; > + default: > + break; > + } > + return false; > +} > + > +static const char * > +fw_err_to_string(u32 type) > +{ > + switch (type) { > + case PXP_STATUS_ERROR_API_VERSION: > + return "ERR_API_VERSION"; > + case PXP_STATUS_NOT_READY: > + return "ERR_NOT_READY"; > + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: > + case PXP_STATUS_PLATFCONFIG_KF1_BAD: > + return "ERR_PLATFORM_CONFIG"; > + default: > + break; > + } > + return NULL; > +} > + There is a lot of replication for this error handling, I'm wondering if it's worth adding a common function to handle this. Something like: intel_pxp_header_error(u32 header, const char *op, u32 id) { if (is_fw_err_platform_config(header.status)) { drm_info_once(&i915->drm, "PXP %s-%d failed due to BIOS/SOC:0x%08x:%s\n", op, id, header.status, fw_err_to_string(header.status)); } else { drm_dbg(&i915->drm, "PXP %s-%d failed 0x%08x:%st:\n", op, id, header.status, fw_err_to_string(header.status)); drm_dbg(&i915->drm, " cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n", header.command_id, header.api_version); } } Not a blocker. > static int > gsccs_send_message(struct intel_pxp *pxp, > void *msg_in, size_t msg_in_size, > @@ -152,6 +185,103 @@ gsccs_send_message_retry_complete(struct intel_pxp *pxp, > return ret; > } > > +bool intel_pxp_gsccs_is_ready_for_sessions(struct intel_pxp *pxp) > +{ > + /* > + * GSC-fw loading, HuC-fw loading, HuC-fw authentication and > + * GSC-proxy init flow (requiring an mei component driver) > + * must all occur first before we can start requesting for PXP > + * sessions. Checking for completion on HuC authentication and > + * gsc-proxy init flow (the last set of dependencies that > + * are out of order) will suffice. > + */ > + if (intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc) && > + intel_gsc_uc_fw_proxy_init_done(&pxp->ctrl_gt->uc.gsc)) > + return true; > + > + return false; > +} > + > +int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, > + int arb_session_id) > +{ > + struct drm_i915_private *i915 = pxp->ctrl_gt->i915; > + 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, 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; > + > + ret = gsccs_send_message_retry_complete(pxp, > + &msg_in, sizeof(msg_in), > + &msg_out, sizeof(msg_out), NULL); > + if (ret) { > + drm_err(&i915->drm, "Failed to init session %d, ret=[%d]\n", arb_session_id, ret); > + } else if (msg_out.header.status != 0) { > + if (is_fw_err_platform_config(msg_out.header.status)) { > + drm_info_once(&i915->drm, > + "PXP init-session-%d failed due to BIOS/SOC:0x%08x:%s\n", > + arb_session_id, msg_out.header.status, > + fw_err_to_string(msg_out.header.status)); > + } else { > + drm_dbg(&i915->drm, "PXP init-session-%d failed 0x%08x:%st:\n", > + arb_session_id, msg_out.header.status, > + fw_err_to_string(msg_out.header.status)); > + drm_dbg(&i915->drm, " cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n", > + msg_in.header.command_id, msg_in.header.api_version); > + } > + } > + > + return ret; > +} > + > +void intel_pxp_gsccs_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id) > +{ > + struct drm_i915_private *i915 = pxp->ctrl_gt->i915; > + struct pxp42_inv_stream_key_in msg_in = {0}; > + struct pxp42_inv_stream_key_out msg_out = {0}; > + int ret = 0; > + > + /* > + * Stream key invalidation reuses the same version 4.2 input/output > + * command format but firmware requires 4.3 API interaction > + */ > + msg_in.header.api_version = PXP_APIVER(4, 3); > + msg_in.header.command_id = PXP42_CMDID_INVALIDATE_STREAM_KEY; > + msg_in.header.buffer_len = sizeof(msg_in) - sizeof(msg_in.header); > + > + msg_in.header.stream_id = FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_VALID, 1); > + msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_APP_TYPE, 0); > + msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_ID, session_id); > + > + ret = gsccs_send_message_retry_complete(pxp, > + &msg_in, sizeof(msg_in), > + &msg_out, sizeof(msg_out), NULL); > + if (ret) { > + drm_err(&i915->drm, "Failed to inv-stream-key-%u, ret=[%d]\n", > + session_id, ret); > + } else if (msg_out.header.status != 0) { > + if (is_fw_err_platform_config(msg_out.header.status)) { > + drm_info_once(&i915->drm, > + "PXP inv-stream-key-%u failed due to BIOS/SOC :0x%08x:%s\n", > + session_id, msg_out.header.status, > + fw_err_to_string(msg_out.header.status)); > + } else { > + drm_dbg(&i915->drm, "PXP inv-stream-key-%u failed 0x%08x:%s:\n", > + session_id, msg_out.header.status, > + fw_err_to_string(msg_out.header.status)); > + drm_dbg(&i915->drm, " cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n", > + msg_in.header.command_id, msg_in.header.api_version); > + } > + } > +} > + > static void > gsccs_cleanup_fw_host_session_handle(struct intel_pxp *pxp) > { > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h > index bd1c028bc80f..820c2def21ee 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h > @@ -10,14 +10,18 @@ > > struct intel_pxp; > > -#define GSC_REPLY_LATENCY_MS 200 > +#define GSC_REPLY_LATENCY_MS 210 why move from 200 to 210? not a problem, I just want to understand why this is required. Daniele > #define GSC_PENDING_RETRY_MAXCOUNT 40 > #define GSC_PENDING_RETRY_PAUSE_MS 50 > +#define GSCFW_MAX_ROUND_TRIP_LATENCY_MS (GSC_PENDING_RETRY_MAXCOUNT * GSC_PENDING_RETRY_PAUSE_MS) > > #ifdef CONFIG_DRM_I915_PXP > void intel_pxp_gsccs_fini(struct intel_pxp *pxp); > int intel_pxp_gsccs_init(struct intel_pxp *pxp); > > +int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, int arb_session_id); > +void intel_pxp_gsccs_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id); > + > #else > static inline void intel_pxp_gsccs_fini(struct intel_pxp *pxp) > { > @@ -30,4 +34,6 @@ static inline int intel_pxp_gsccs_init(struct intel_pxp *pxp) > > #endif > > +bool intel_pxp_gsccs_is_ready_for_sessions(struct intel_pxp *pxp); > + > #endif /*__INTEL_PXP_GSCCS_H__ */ > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > index 7899079e17b0..e4d8242302c5 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > @@ -7,6 +7,7 @@ > > #include "intel_pxp.h" > #include "intel_pxp_cmd.h" > +#include "intel_pxp_gsccs.h" > #include "intel_pxp_session.h" > #include "intel_pxp_tee.h" > #include "intel_pxp_types.h" > @@ -62,7 +63,10 @@ static int pxp_create_arb_session(struct intel_pxp *pxp) > return -EEXIST; > } > > - ret = intel_pxp_tee_cmd_create_arb_session(pxp, ARB_SESSION); > + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) > + ret = intel_pxp_gsccs_create_session(pxp, ARB_SESSION); > + else > + ret = intel_pxp_tee_cmd_create_arb_session(pxp, ARB_SESSION); > if (ret) { > drm_err(>->i915->drm, "tee cmd for arb session creation failed\n"); > return ret; > @@ -106,7 +110,10 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp) > > intel_uncore_write(gt->uncore, KCR_GLOBAL_TERMINATE(pxp->kcr_base), 1); > > - intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION); > + if (HAS_ENGINE(gt, GSC0)) > + intel_pxp_gsccs_end_arb_fw_session(pxp, ARB_SESSION); > + else > + intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION); > > return ret; > }
On Mon, 2023-04-10 at 10:14 -0700, Ceraolo Spurio, Daniele wrote: > > > alan:snip > > @@ -354,8 +368,14 @@ int intel_pxp_start(struct intel_pxp *pxp) > > if (!intel_pxp_is_enabled(pxp)) > > return -ENODEV; > > > > - if (wait_for(pxp_component_bound(pxp), 250)) > > - return -ENXIO; > > + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) { > > + /* Use a larger 1 second timeout considering all the dependencies. */ > > + if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 1000)) > > + return -ENXIO; > > This needs a comment to explain that we expect userspace to retry later > if we return -ENXIO and therefore the timeout is smaller that what would > be required to guarantee that the init can complete. It also needs an > ack from the userspace drivers for this behavior. > alan: I agree with a requirement to comment this down. However, i believe its better to put this int the UAPI header file comment-documentation since it applies to both current MTL as well as previous ADL cases (this is not new behavior being introduced in MTL but it is fixing of the spec of existing behavior). That said, your feedback is already being addressed by patch #6 but i will ammend patch #6 to add the detail you highlighted WRT ~"timeout is not larger than the completion-guarantee...". > alan:snip > > +fw_err_to_string(u32 type) > > +{ > > + switch (type) { > > + case PXP_STATUS_ERROR_API_VERSION: > > + return "ERR_API_VERSION"; > > + case PXP_STATUS_NOT_READY: > > + return "ERR_NOT_READY"; > > + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: > > + case PXP_STATUS_PLATFCONFIG_KF1_BAD: > > + return "ERR_PLATFORM_CONFIG"; > > + default: > > + break; > > + } > > + return NULL; > > +} > > + > > There is a lot of replication for this error handling, I'm wondering if > it's worth adding a common function to handle this. Something like: > > intel_pxp_header_error(u32 header, const char *op, u32 id) > { > if (is_fw_err_platform_config(header.status)) { > drm_info_once(&i915->drm, > "PXP %s-%d failed due to BIOS/SOC:0x%08x:%s\n", > op, id, header.status, > fw_err_to_string(header.status)); > } else { > drm_dbg(&i915->drm, "PXP %s-%d failed 0x%08x:%st:\n", > op, id, header.status, > fw_err_to_string(header.status)); > drm_dbg(&i915->drm, " cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n", > header.command_id, header.api_version); > } > } > > > Not a blocker. alan: Thanks - i will have to address as a stand alone patch since i have to think about moving this to a comment helper layer (common to both ADL-mei-comp and MTL-gsccs) while potentially have different set of error codes that can mean different reporting levels (i.e. notice once coz its a platform limit vs always err out if its runtime related). Once this series gets merged it will be easier to work on that patch (which would require both backends to be present in the baseline). > > alan:snip > > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h > > @@ -10,14 +10,18 @@ > > > > struct intel_pxp; > > > > -#define GSC_REPLY_LATENCY_MS 200 > > +#define GSC_REPLY_LATENCY_MS 210 > > why move from 200 to 210? not a problem, I just want to understand why > this is required. > > Daniele alan: so 200 is based on the fw spec - and from real testing i observed the occasional highs touching ~185ms. However, the spec gives this number as the expected max time the firmware is going to take to process the request and post a reply. Therefore it doesn't include the additional overhead for the request creation, emision, submission via guc and the completion pipeline completion indication. All of these contribute additional layers of possible delay. Since arb-session creation and invalidation are not common events, I believe a slightly wider overhead of 10 milisec will not hurt.
On 4/17/2023 11:21 AM, Teres Alexis, Alan Previn wrote: > On Mon, 2023-04-10 at 10:14 -0700, Ceraolo Spurio, Daniele wrote: >> >> > alan:snip > >>> @@ -354,8 +368,14 @@ int intel_pxp_start(struct intel_pxp *pxp) >>> if (!intel_pxp_is_enabled(pxp)) >>> return -ENODEV; >>> >>> - if (wait_for(pxp_component_bound(pxp), 250)) >>> - return -ENXIO; >>> + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) { >>> + /* Use a larger 1 second timeout considering all the dependencies. */ >>> + if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 1000)) >>> + return -ENXIO; >> This needs a comment to explain that we expect userspace to retry later >> if we return -ENXIO and therefore the timeout is smaller that what would >> be required to guarantee that the init can complete. It also needs an >> ack from the userspace drivers for this behavior. >> > alan: I agree with a requirement to comment this down. However, i believe its better > to put this int the UAPI header file comment-documentation since it applies to both > current MTL as well as previous ADL cases (this is not new behavior being introduced > in MTL but it is fixing of the spec of existing behavior). > That said, your feedback is already being addressed by patch #6 but i will ammend > patch #6 to add the detail you highlighted WRT ~"timeout is not larger than the completion-guarantee...". Can you move that comment update for the context getparam from the next patch to this one? that way it's all nicely self-contained in this patch. > alan:snip >>> +fw_err_to_string(u32 type) >>> +{ >>> + switch (type) { >>> + case PXP_STATUS_ERROR_API_VERSION: >>> + return "ERR_API_VERSION"; >>> + case PXP_STATUS_NOT_READY: >>> + return "ERR_NOT_READY"; >>> + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: >>> + case PXP_STATUS_PLATFCONFIG_KF1_BAD: >>> + return "ERR_PLATFORM_CONFIG"; >>> + default: >>> + break; >>> + } >>> + return NULL; >>> +} >>> + >> There is a lot of replication for this error handling, I'm wondering if >> it's worth adding a common function to handle this. Something like: >> >> intel_pxp_header_error(u32 header, const char *op, u32 id) >> { >> if (is_fw_err_platform_config(header.status)) { >> drm_info_once(&i915->drm, >> "PXP %s-%d failed due to BIOS/SOC:0x%08x:%s\n", >> op, id, header.status, >> fw_err_to_string(header.status)); >> } else { >> drm_dbg(&i915->drm, "PXP %s-%d failed 0x%08x:%st:\n", >> op, id, header.status, >> fw_err_to_string(header.status)); >> drm_dbg(&i915->drm, " cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n", >> header.command_id, header.api_version); >> } >> } >> >> >> Not a blocker. > alan: Thanks - i will have to address as a stand alone patch since i have to > think about moving this to a comment helper layer (common to both ADL-mei-comp and MTL-gsccs) > while potentially have different set of error codes that can mean different reporting levels > (i.e. notice once coz its a platform limit vs always err out if its runtime related). > Once this series gets merged it will be easier to work on that patch (which would require both > backends to be present in the baseline). > alan:snip >>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h >>> @@ -10,14 +10,18 @@ >>> >>> struct intel_pxp; >>> >>> -#define GSC_REPLY_LATENCY_MS 200 >>> +#define GSC_REPLY_LATENCY_MS 210 >> why move from 200 to 210? not a problem, I just want to understand why >> this is required. >> >> Daniele > alan: so 200 is based on the fw spec - and from real testing i observed the occasional highs touching ~185ms. > However, the spec gives this number as the expected max time the firmware is going to take to process the request > and post a reply. Therefore it doesn't include the additional overhead for the request creation, emision, > submission via guc and the completion pipeline completion indication. All of these contribute additional layers > of possible delay. Since arb-session creation and invalidation are not common events, > I believe a slightly wider overhead of 10 milisec will not hurt. Agreed. Can you add a comment? something like "Max FW response time is 200ms, to which we add 10ms to account for overhead". With those 2 nits addressed: Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Daniele >
On Mon, 2023-04-17 at 12:15 -0700, Ceraolo Spurio, Daniele wrote: > On 4/17/2023 11:21 AM, Teres Alexis, Alan Previn wrote: > > On Mon, 2023-04-10 at 10:14 -0700, Ceraolo Spurio, Daniele wrote: > > > > @@ -354,8 +368,14 @@ int intel_pxp_start(struct intel_pxp *pxp) > > > > if (!intel_pxp_is_enabled(pxp)) > > > > return -ENODEV; > > > > > > > > - if (wait_for(pxp_component_bound(pxp), 250)) > > > > - return -ENXIO; > > > > + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) { > > > > + /* Use a larger 1 second timeout considering all the dependencies. */ > > > > + if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 1000)) > > > > + return -ENXIO; > > > This needs a comment to explain that we expect userspace to retry later > > > if we return -ENXIO and therefore the timeout is smaller that what would > > > be required to guarantee that the init can complete. It also needs an > > > ack from the userspace drivers for this behavior. > > > > > alan: I agree with a requirement to comment this down. However, i believe its better > > to put this int the UAPI header file comment-documentation since it applies to both > > current MTL as well as previous ADL cases (this is not new behavior being introduced > > in MTL but it is fixing of the spec of existing behavior). > > That said, your feedback is already being addressed by patch #6 but i will ammend > > patch #6 to add the detail you highlighted WRT ~"timeout is not larger than the completion-guarantee...". > > Can you move that comment update for the context getparam from the next > patch to this one? that way it's all nicely self-contained in this patch. alan: sounds good - will do alan:snip > > > > > > > > -#define GSC_REPLY_LATENCY_MS 200 > > > > +#define GSC_REPLY_LATENCY_MS 210 > > > why move from 200 to 210? not a problem, I just want to understand why > > > this is required. > > > > > > Daniele > > alan: so 200 is based on the fw spec - and from real testing i observed the occasional highs touching ~185ms. > > However, the spec gives this number as the expected max time the firmware is going to take to process the request > > and post a reply. Therefore it doesn't include the additional overhead for the request creation, emision, > > submission via guc and the completion pipeline completion indication. All of these contribute additional layers > > of possible delay. Since arb-session creation and invalidation are not common events, > > I believe a slightly wider overhead of 10 milisec will not hurt. > > Agreed. Can you add a comment? something like "Max FW response time is > 200ms, to which we add 10ms to account for overhead". alan: will do and thanks for the conditional rb. will fix these accordingly. > With those 2 nits addressed: > > Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > > Daniele
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c index 1d9fdfb11268..85f720af2f75 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c @@ -13,6 +13,7 @@ #define GSC_FW_STATUS_REG _MMIO(0x116C40) #define GSC_FW_CURRENT_STATE REG_GENMASK(3, 0) #define GSC_FW_CURRENT_STATE_RESET 0 +#define GSC_FW_PROXY_STATE_NORMAL 5 #define GSC_FW_INIT_COMPLETE_BIT REG_BIT(9) static bool gsc_is_in_reset(struct intel_uncore *uncore) @@ -31,6 +32,15 @@ bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc) return fw_status & GSC_FW_INIT_COMPLETE_BIT; } +bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc) +{ + struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore; + u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG); + + return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) == + GSC_FW_PROXY_STATE_NORMAL; +} + static int emit_gsc_fw_load(struct i915_request *rq, struct intel_gsc_uc *gsc) { u32 offset = i915_ggtt_offset(gsc->local); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h index f4c1106bb2a9..fff8928218df 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h @@ -13,5 +13,6 @@ struct intel_uncore; int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc); bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc); +bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc); #endif diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c index 8949d4be7882..550457bbb034 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c @@ -291,6 +291,8 @@ static bool pxp_component_bound(struct intel_pxp *pxp) static int __pxp_global_teardown_final(struct intel_pxp *pxp) { + int timeout; + if (!pxp->arb_is_valid) return 0; /* @@ -300,7 +302,12 @@ static int __pxp_global_teardown_final(struct intel_pxp *pxp) intel_pxp_mark_termination_in_progress(pxp); intel_pxp_terminate(pxp, false); - if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250))) + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) + timeout = GSCFW_MAX_ROUND_TRIP_LATENCY_MS; + else + timeout = 250; + + if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout))) return -ETIMEDOUT; return 0; @@ -308,6 +315,8 @@ static int __pxp_global_teardown_final(struct intel_pxp *pxp) static int __pxp_global_teardown_restart(struct intel_pxp *pxp) { + int timeout; + if (pxp->arb_is_valid) return 0; /* @@ -316,7 +325,12 @@ static int __pxp_global_teardown_restart(struct intel_pxp *pxp) */ pxp_queue_termination(pxp); - if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250))) + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) + timeout = GSCFW_MAX_ROUND_TRIP_LATENCY_MS; + else + timeout = 250; + + if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout))) return -ETIMEDOUT; return 0; @@ -354,8 +368,14 @@ int intel_pxp_start(struct intel_pxp *pxp) if (!intel_pxp_is_enabled(pxp)) return -ENODEV; - if (wait_for(pxp_component_bound(pxp), 250)) - return -ENXIO; + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) { + /* Use a larger 1 second timeout considering all the dependencies. */ + if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 1000)) + return -ENXIO; + } else { + if (wait_for(pxp_component_bound(pxp), 250)) + return -ENXIO; + } mutex_lock(&pxp->arb_mutex); 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 c65ada99e54f..09777719cd84 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_INOUT_SIZE (SZ_32K) @@ -26,4 +27,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 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; + #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 16e3b73d0653..4bc276daca16 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c @@ -6,13 +6,46 @@ #include "gem/i915_gem_internal.h" #include "gt/intel_context.h" +#include "gt/uc/intel_gsc_fw.h" #include "gt/uc/intel_gsc_uc_heci_cmd_submit.h" #include "i915_drv.h" +#include "intel_pxp_cmd_interface_42.h" #include "intel_pxp_cmd_interface_43.h" #include "intel_pxp_gsccs.h" #include "intel_pxp_types.h" +static bool +is_fw_err_platform_config(u32 type) +{ + switch (type) { + case PXP_STATUS_ERROR_API_VERSION: + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: + case PXP_STATUS_PLATFCONFIG_KF1_BAD: + return true; + default: + break; + } + return false; +} + +static const char * +fw_err_to_string(u32 type) +{ + switch (type) { + case PXP_STATUS_ERROR_API_VERSION: + return "ERR_API_VERSION"; + case PXP_STATUS_NOT_READY: + return "ERR_NOT_READY"; + case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF: + case PXP_STATUS_PLATFCONFIG_KF1_BAD: + return "ERR_PLATFORM_CONFIG"; + default: + break; + } + return NULL; +} + static int gsccs_send_message(struct intel_pxp *pxp, void *msg_in, size_t msg_in_size, @@ -152,6 +185,103 @@ gsccs_send_message_retry_complete(struct intel_pxp *pxp, return ret; } +bool intel_pxp_gsccs_is_ready_for_sessions(struct intel_pxp *pxp) +{ + /* + * GSC-fw loading, HuC-fw loading, HuC-fw authentication and + * GSC-proxy init flow (requiring an mei component driver) + * must all occur first before we can start requesting for PXP + * sessions. Checking for completion on HuC authentication and + * gsc-proxy init flow (the last set of dependencies that + * are out of order) will suffice. + */ + if (intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc) && + intel_gsc_uc_fw_proxy_init_done(&pxp->ctrl_gt->uc.gsc)) + return true; + + return false; +} + +int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, + int arb_session_id) +{ + struct drm_i915_private *i915 = pxp->ctrl_gt->i915; + 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, 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; + + ret = gsccs_send_message_retry_complete(pxp, + &msg_in, sizeof(msg_in), + &msg_out, sizeof(msg_out), NULL); + if (ret) { + drm_err(&i915->drm, "Failed to init session %d, ret=[%d]\n", arb_session_id, ret); + } else if (msg_out.header.status != 0) { + if (is_fw_err_platform_config(msg_out.header.status)) { + drm_info_once(&i915->drm, + "PXP init-session-%d failed due to BIOS/SOC:0x%08x:%s\n", + arb_session_id, msg_out.header.status, + fw_err_to_string(msg_out.header.status)); + } else { + drm_dbg(&i915->drm, "PXP init-session-%d failed 0x%08x:%st:\n", + arb_session_id, msg_out.header.status, + fw_err_to_string(msg_out.header.status)); + drm_dbg(&i915->drm, " cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n", + msg_in.header.command_id, msg_in.header.api_version); + } + } + + return ret; +} + +void intel_pxp_gsccs_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id) +{ + struct drm_i915_private *i915 = pxp->ctrl_gt->i915; + struct pxp42_inv_stream_key_in msg_in = {0}; + struct pxp42_inv_stream_key_out msg_out = {0}; + int ret = 0; + + /* + * Stream key invalidation reuses the same version 4.2 input/output + * command format but firmware requires 4.3 API interaction + */ + msg_in.header.api_version = PXP_APIVER(4, 3); + msg_in.header.command_id = PXP42_CMDID_INVALIDATE_STREAM_KEY; + msg_in.header.buffer_len = sizeof(msg_in) - sizeof(msg_in.header); + + msg_in.header.stream_id = FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_VALID, 1); + msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_APP_TYPE, 0); + msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_ID, session_id); + + ret = gsccs_send_message_retry_complete(pxp, + &msg_in, sizeof(msg_in), + &msg_out, sizeof(msg_out), NULL); + if (ret) { + drm_err(&i915->drm, "Failed to inv-stream-key-%u, ret=[%d]\n", + session_id, ret); + } else if (msg_out.header.status != 0) { + if (is_fw_err_platform_config(msg_out.header.status)) { + drm_info_once(&i915->drm, + "PXP inv-stream-key-%u failed due to BIOS/SOC :0x%08x:%s\n", + session_id, msg_out.header.status, + fw_err_to_string(msg_out.header.status)); + } else { + drm_dbg(&i915->drm, "PXP inv-stream-key-%u failed 0x%08x:%s:\n", + session_id, msg_out.header.status, + fw_err_to_string(msg_out.header.status)); + drm_dbg(&i915->drm, " cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n", + msg_in.header.command_id, msg_in.header.api_version); + } + } +} + static void gsccs_cleanup_fw_host_session_handle(struct intel_pxp *pxp) { diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h index bd1c028bc80f..820c2def21ee 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h @@ -10,14 +10,18 @@ struct intel_pxp; -#define GSC_REPLY_LATENCY_MS 200 +#define GSC_REPLY_LATENCY_MS 210 #define GSC_PENDING_RETRY_MAXCOUNT 40 #define GSC_PENDING_RETRY_PAUSE_MS 50 +#define GSCFW_MAX_ROUND_TRIP_LATENCY_MS (GSC_PENDING_RETRY_MAXCOUNT * GSC_PENDING_RETRY_PAUSE_MS) #ifdef CONFIG_DRM_I915_PXP void intel_pxp_gsccs_fini(struct intel_pxp *pxp); int intel_pxp_gsccs_init(struct intel_pxp *pxp); +int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, int arb_session_id); +void intel_pxp_gsccs_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id); + #else static inline void intel_pxp_gsccs_fini(struct intel_pxp *pxp) { @@ -30,4 +34,6 @@ static inline int intel_pxp_gsccs_init(struct intel_pxp *pxp) #endif +bool intel_pxp_gsccs_is_ready_for_sessions(struct intel_pxp *pxp); + #endif /*__INTEL_PXP_GSCCS_H__ */ diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c index 7899079e17b0..e4d8242302c5 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c @@ -7,6 +7,7 @@ #include "intel_pxp.h" #include "intel_pxp_cmd.h" +#include "intel_pxp_gsccs.h" #include "intel_pxp_session.h" #include "intel_pxp_tee.h" #include "intel_pxp_types.h" @@ -62,7 +63,10 @@ static int pxp_create_arb_session(struct intel_pxp *pxp) return -EEXIST; } - ret = intel_pxp_tee_cmd_create_arb_session(pxp, ARB_SESSION); + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) + ret = intel_pxp_gsccs_create_session(pxp, ARB_SESSION); + else + ret = intel_pxp_tee_cmd_create_arb_session(pxp, ARB_SESSION); if (ret) { drm_err(>->i915->drm, "tee cmd for arb session creation failed\n"); return ret; @@ -106,7 +110,10 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp) intel_uncore_write(gt->uncore, KCR_GLOBAL_TERMINATE(pxp->kcr_base), 1); - intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION); + if (HAS_ENGINE(gt, GSC0)) + intel_pxp_gsccs_end_arb_fw_session(pxp, ARB_SESSION); + else + intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION); return ret; }
Add MTL's function for ARB session creation using PXP firmware version 4.3 ABI structure format. Also add MTL's function for ARB session invalidation but this reuses PXP firmware version 4.2 ABI structure format. For both cases, in the back-end gsccs functions for sending messages to the firmware inspect the GSC-CS-Mem-Header's pending-bit which means the GSC firmware is busy and we should retry. Given the last hw requirement, lets also update functions in front-end layer that wait for session creation or teardown completion to use new worst case timeout periods. Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 10 ++ drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 1 + drivers/gpu/drm/i915/pxp/intel_pxp.c | 28 +++- .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 21 +++ drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 130 ++++++++++++++++++ drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h | 8 +- drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 11 +- 7 files changed, 202 insertions(+), 7 deletions(-)