| Message ID | 20230228022150.1657843-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 2/27/2023 6:21 PM, 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. > > 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> > --- > drivers/gpu/drm/i915/pxp/intel_pxp.c | 34 ++++++++-- > .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 21 +++++++ > drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 62 +++++++++++++++++++ > drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h | 4 ++ > drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 11 +++- > 5 files changed, 126 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c > index aecc65b5da70..61041277be24 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c > @@ -290,6 +290,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; > /* > @@ -299,7 +301,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 = GSC_PENDING_RETRY_LATENCY_MS; > + else > + timeout = 250; > + > + if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout))) > return -ETIMEDOUT; > > return 0; > @@ -307,6 +314,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; > /* > @@ -315,7 +324,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 = GSC_PENDING_RETRY_LATENCY_MS; > + else > + timeout = 250; > + > + if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout))) > return -ETIMEDOUT; > > return 0; > @@ -353,8 +367,20 @@ 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)) { > + /* > + * GSC-fw loading, GSC-proxy init (requiring an mei component driver) and > + * HuC-fw loading must all occur first before we start requesting for PXP > + * sessions. Checking HuC authentication (the last dependency) will suffice. > + * Let's use a much larger 8 second timeout considering all the types of > + * dependencies prior to that. > + */ > + if (wait_for(intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc), 8000)) This big timeout needs an ack from userspace drivers, as intel_pxp_start is called during context creation and the current way to query if the feature is supported is to create a protected context. Unfortunately, we do need to wait to confirm that PXP is available (although in most cases it shouldn't take even close to 8 secs), because until everything is setup we're not sure if things will work as expected. I see 2 potential mitigations in case the timeout doesn't work as-is: 1) we return -EAGAIN (or another dedicated error code) to userspace if the prerequisite steps aren't done yet. This would indicate that the feature is there, but that we haven't completed the setup yet. The caller can then decide if they want to retry immediately or later. Pro: more flexibility for userspace; Cons: new interface return code. 2) we add a getparam to say if PXP is supported in HW and the support is compiled in i915. Userspace can query this as a way to check the feature support and only create the context if they actually need it for PXP operations. Pro: simpler kernel implementation; Cons: new getparam, plus even if the getparam returns true the pxp_start could later fail, so userspace needs to handle that case. > + 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 b2523d6918c7..9089e02a8c2d 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 I don't know where this interface is defined, so for now I'm going to assume this and the below structs are correct. I'll sync with you on the side to get to the specs. > > /* 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 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 30aa660a975f..6f5c5dad8c16 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > @@ -9,6 +9,7 @@ > #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" > @@ -150,6 +151,67 @@ gsccs_send_message_retry_complete(struct intel_pxp *pxp, > return ret; > } > > +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_warn(&i915->drm, "Failed to send gsccs msg for creating-session-%d: ret=[%d]\n", > + arb_session_id, ret); > + else if (msg_out.header.status != 0x0) > + drm_warn(&i915->drm, "PXP firmware failed on creating-session-%d: status=0x%08x\n", > + arb_session_id, msg_out.header.status); Should this follow the same log style as https://patchwork.freedesktop.org/patch/521431/? same for the function below. > + > + 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; > + > + memset(&msg_in, 0, sizeof(msg_in)); > + memset(&msg_out, 0, sizeof(msg_out)); You're already initializing the structs to zero with "= {0}" Daniele > + /* > + * 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_warn(&i915->drm, "Failed to send gsccs msg for inv-stream-session-%d: ret=[%d]\n", > + session_id, ret); > + else if (msg_out.header.status != 0x0) > + drm_warn(&i915->drm, "PXP firmware failed on inv-stream-session-%d: status=0x%08x\n", > + session_id, msg_out.header.status); > +} > + > static int > gsccs_create_buffer(struct intel_gt *gt, > const char *bufname, size_t size, > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h > index bd1c028bc80f..4d393f1248fe 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h > @@ -13,11 +13,15 @@ struct intel_pxp; > #define GSC_REPLY_LATENCY_MS 200 > #define GSC_PENDING_RETRY_MAXCOUNT 40 > #define GSC_PENDING_RETRY_PAUSE_MS 50 > +#define GSC_PENDING_RETRY_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) > { > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c > index b8fa6df7141b..4ddf2ee60222 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 Fri, 2023-03-03 at 17:34 -0800, Ceraolo Spurio, Daniele wrote: > > On 2/27/2023 6:21 PM, Alan Previn wrote: > > Add MTL's function for ARB session creation using PXP firmware > > version 4.3 ABI structure format. > alan:snip > > + ret = gsccs_send_message_retry_complete(pxp, > > + &msg_in, sizeof(msg_in), > > + &msg_out, sizeof(msg_out), NULL); > > + if (ret) > > + drm_warn(&i915->drm, "Failed to send gsccs msg for creating-session-%d: ret=[%d]\n", > > + arb_session_id, ret); > > + else if (msg_out.header.status != 0x0) > > + drm_warn(&i915->drm, "PXP firmware failed on creating-session-%d: status=0x%08x\n", > > + arb_session_id, msg_out.header.status); > > Should this follow the same log style as > https://patchwork.freedesktop.org/patch/521431/? same for the function > below. alan: yeah - i was planning to update once the other got an Rb which is just did. In the coming rev7, I'll probably duplicate some of the fw-err-to-string function for both GSCCS and TEE in the event new or existing error conditions are differently determined as platform persisting issue or runtime issues. alan:snip > > +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; > > + > > + memset(&msg_in, 0, sizeof(msg_in)); > > + memset(&msg_out, 0, sizeof(msg_out)); > > You're already initializing the structs to zero with "= {0}" > alan: oops - copy+paste error - will fix.
alan:snip > @@ -353,8 +367,20 @@ int intel_pxp_start(struct intel_pxp *pxp) alan:snip > > + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) { > > + /* > > + * GSC-fw loading, GSC-proxy init (requiring an mei component driver) and > > + * HuC-fw loading must all occur first before we start requesting for PXP > > + * sessions. Checking HuC authentication (the last dependency) will suffice. > > + * Let's use a much larger 8 second timeout considering all the types of > > + * dependencies prior to that. > > + */ > > + if (wait_for(intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc), 8000)) > > This big timeout needs an ack from userspace drivers, as intel_pxp_start > is called during context creation and the current way to query if the > feature is supported is to create a protected context. Unfortunately, we > do need to wait to confirm that PXP is available (although in most cases > it shouldn't take even close to 8 secs), because until everything is > setup we're not sure if things will work as expected. I see 2 potential > mitigations in case the timeout doesn't work as-is: > > 1) we return -EAGAIN (or another dedicated error code) to userspace if > the prerequisite steps aren't done yet. This would indicate that the > feature is there, but that we haven't completed the setup yet. The > caller can then decide if they want to retry immediately or later. Pro: > more flexibility for userspace; Cons: new interface return code. > > 2) we add a getparam to say if PXP is supported in HW and the support is > compiled in i915. Userspace can query this as a way to check the feature > support and only create the context if they actually need it for PXP > operations. Pro: simpler kernel implementation; Cons: new getparam, plus > even if the getparam returns true the pxp_start could later fail, so > userspace needs to handle that case. > alan: I've cc'd Rodrigo, Joonas and Lionel. Folks - what are your thoughts on above issue? Recap: On MTL, only when creating a GEM Protected (PXP) context for the very first time after a driver load, it will be dependent on (1) loading the GSC firmware, (2) GuC loading the HuC firmware and (3) GSC authenticating the HuC fw. But step 3 also depends on additional GSC-proxy-init steps that depend on a new mei-gsc-proxy component driver. I'd used the 8 second number based on offline conversations with Daniele but that is a worse-case. Alternatively, should we change UAPI instead to return -EAGAIN as per Daniele's proposal? I believe we've had the get-param conversation offline recently and the direction was to stick with attempting to create the context as it is normal in 3D UMD when it comes to testing capabilities for other features too. Thoughts?
On Sat, Mar 25, 2023 at 02:19:21AM -0400, Teres Alexis, Alan Previn wrote: > alan:snip > > > > @@ -353,8 +367,20 @@ int intel_pxp_start(struct intel_pxp *pxp) > alan:snip > > > + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) { > > > + /* > > > + * GSC-fw loading, GSC-proxy init (requiring an mei component driver) and > > > + * HuC-fw loading must all occur first before we start requesting for PXP > > > + * sessions. Checking HuC authentication (the last dependency) will suffice. > > > + * Let's use a much larger 8 second timeout considering all the types of > > > + * dependencies prior to that. > > > + */ > > > + if (wait_for(intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc), 8000)) > > > > This big timeout needs an ack from userspace drivers, as intel_pxp_start > > is called during context creation and the current way to query if the > > feature is supported is to create a protected context. Unfortunately, we > > do need to wait to confirm that PXP is available (although in most cases > > it shouldn't take even close to 8 secs), because until everything is > > setup we're not sure if things will work as expected. I see 2 potential > > mitigations in case the timeout doesn't work as-is: > > > > 1) we return -EAGAIN (or another dedicated error code) to userspace if > > the prerequisite steps aren't done yet. This would indicate that the > > feature is there, but that we haven't completed the setup yet. The > > caller can then decide if they want to retry immediately or later. Pro: > > more flexibility for userspace; Cons: new interface return code. > > > > 2) we add a getparam to say if PXP is supported in HW and the support is > > compiled in i915. Userspace can query this as a way to check the feature > > support and only create the context if they actually need it for PXP > > operations. Pro: simpler kernel implementation; Cons: new getparam, plus > > even if the getparam returns true the pxp_start could later fail, so > > userspace needs to handle that case. > > > > alan: I've cc'd Rodrigo, Joonas and Lionel. Folks - what are your thoughts on above issue? > Recap: On MTL, only when creating a GEM Protected (PXP) context for the very first time after > a driver load, it will be dependent on (1) loading the GSC firmware, (2) GuC loading the HuC > firmware and (3) GSC authenticating the HuC fw. But step 3 also depends on additional > GSC-proxy-init steps that depend on a new mei-gsc-proxy component driver. I'd used the > 8 second number based on offline conversations with Daniele but that is a worse-case. > Alternatively, should we change UAPI instead to return -EAGAIN as per Daniele's proposal? > I believe we've had the get-param conversation offline recently and the direction was to > stick with attempting to create the context as it is normal in 3D UMD when it comes to > testing capabilities for other features too. > > Thoughts? I like the option 1 more. This extra return handling won't break compatibility.
On 26/03/2023 14:18, Rodrigo Vivi wrote: > On Sat, Mar 25, 2023 at 02:19:21AM -0400, Teres Alexis, Alan Previn wrote: >> alan:snip >> >> @@ -353,8 +367,20 @@ int intel_pxp_start(struct intel_pxp *pxp) >> alan:snip >>>> + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) { >>>> + /* >>>> + * GSC-fw loading, GSC-proxy init (requiring an mei component driver) and >>>> + * HuC-fw loading must all occur first before we start requesting for PXP >>>> + * sessions. Checking HuC authentication (the last dependency) will suffice. >>>> + * Let's use a much larger 8 second timeout considering all the types of >>>> + * dependencies prior to that. >>>> + */ >>>> + if (wait_for(intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc), 8000)) >>> This big timeout needs an ack from userspace drivers, as intel_pxp_start >>> is called during context creation and the current way to query if the >>> feature is supported is to create a protected context. Unfortunately, we >>> do need to wait to confirm that PXP is available (although in most cases >>> it shouldn't take even close to 8 secs), because until everything is >>> setup we're not sure if things will work as expected. I see 2 potential >>> mitigations in case the timeout doesn't work as-is: >>> >>> 1) we return -EAGAIN (or another dedicated error code) to userspace if >>> the prerequisite steps aren't done yet. This would indicate that the >>> feature is there, but that we haven't completed the setup yet. The >>> caller can then decide if they want to retry immediately or later. Pro: >>> more flexibility for userspace; Cons: new interface return code. >>> >>> 2) we add a getparam to say if PXP is supported in HW and the support is >>> compiled in i915. Userspace can query this as a way to check the feature >>> support and only create the context if they actually need it for PXP >>> operations. Pro: simpler kernel implementation; Cons: new getparam, plus >>> even if the getparam returns true the pxp_start could later fail, so >>> userspace needs to handle that case. >>> >> alan: I've cc'd Rodrigo, Joonas and Lionel. Folks - what are your thoughts on above issue? >> Recap: On MTL, only when creating a GEM Protected (PXP) context for the very first time after >> a driver load, it will be dependent on (1) loading the GSC firmware, (2) GuC loading the HuC >> firmware and (3) GSC authenticating the HuC fw. But step 3 also depends on additional >> GSC-proxy-init steps that depend on a new mei-gsc-proxy component driver. I'd used the >> 8 second number based on offline conversations with Daniele but that is a worse-case. >> Alternatively, should we change UAPI instead to return -EAGAIN as per Daniele's proposal? >> I believe we've had the get-param conversation offline recently and the direction was to >> stick with attempting to create the context as it is normal in 3D UMD when it comes to >> testing capabilities for other features too. >> >> Thoughts? > I like the option 1 more. This extra return handling won't break compatibility. I like option 2 better because we have to report support as fast as we can when enumerating devices on the system for example. If I understand correctly, with the get param, most apps won't ever be blocking on any PXP stuff if they don't use it. Only the ones that require protected support might block. -Lionel
On 27/03/2023 08:07, Lionel Landwerlin wrote: > On 26/03/2023 14:18, Rodrigo Vivi wrote: >> On Sat, Mar 25, 2023 at 02:19:21AM -0400, Teres Alexis, Alan Previn >> wrote: >>> alan:snip >>> >>> @@ -353,8 +367,20 @@ int intel_pxp_start(struct intel_pxp *pxp) >>> alan:snip >>>>> + if (HAS_ENGINE(pxp->ctrl_gt, GSC0)) { >>>>> + /* >>>>> + * GSC-fw loading, GSC-proxy init (requiring an mei >>>>> component driver) and >>>>> + * HuC-fw loading must all occur first before we start >>>>> requesting for PXP >>>>> + * sessions. Checking HuC authentication (the last >>>>> dependency) will suffice. >>>>> + * Let's use a much larger 8 second timeout considering >>>>> all the types of >>>>> + * dependencies prior to that. >>>>> + */ >>>>> + if >>>>> (wait_for(intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc), 8000)) >>>> This big timeout needs an ack from userspace drivers, as >>>> intel_pxp_start >>>> is called during context creation and the current way to query if the >>>> feature is supported is to create a protected context. >>>> Unfortunately, we >>>> do need to wait to confirm that PXP is available (although in most >>>> cases >>>> it shouldn't take even close to 8 secs), because until everything is >>>> setup we're not sure if things will work as expected. I see 2 potential >>>> mitigations in case the timeout doesn't work as-is: >>>> >>>> 1) we return -EAGAIN (or another dedicated error code) to userspace if >>>> the prerequisite steps aren't done yet. This would indicate that the >>>> feature is there, but that we haven't completed the setup yet. The >>>> caller can then decide if they want to retry immediately or later. Pro: >>>> more flexibility for userspace; Cons: new interface return code. >>>> >>>> 2) we add a getparam to say if PXP is supported in HW and the >>>> support is >>>> compiled in i915. Userspace can query this as a way to check the >>>> feature >>>> support and only create the context if they actually need it for PXP >>>> operations. Pro: simpler kernel implementation; Cons: new getparam, >>>> plus >>>> even if the getparam returns true the pxp_start could later fail, so >>>> userspace needs to handle that case. These two: e6177ec586d1 ("drm/i915/huc: stall media submission until HuC is loaded") b76c14c8fb2a ("drm/i915/huc: better define HuC status getparam possible return values.") They do not help here? It is not possible to use or extend the refined I915_PARAM_HUC_STATUS return values combined with huc load fence for this all to keep working? Regards, Tvrtko >>> alan: I've cc'd Rodrigo, Joonas and Lionel. Folks - what are your >>> thoughts on above issue? >>> Recap: On MTL, only when creating a GEM Protected (PXP) context for >>> the very first time after >>> a driver load, it will be dependent on (1) loading the GSC firmware, >>> (2) GuC loading the HuC >>> firmware and (3) GSC authenticating the HuC fw. But step 3 also >>> depends on additional >>> GSC-proxy-init steps that depend on a new mei-gsc-proxy component >>> driver. I'd used the >>> 8 second number based on offline conversations with Daniele but that >>> is a worse-case. >>> Alternatively, should we change UAPI instead to return -EAGAIN as per >>> Daniele's proposal? >>> I believe we've had the get-param conversation offline recently and >>> the direction was to >>> stick with attempting to create the context as it is normal in 3D UMD >>> when it comes to >>> testing capabilities for other features too. >>> >>> Thoughts? >> I like the option 1 more. This extra return handling won't break >> compatibility. > > > I like option 2 better because we have to report support as fast as we > can when enumerating devices on the system for example. > > If I understand correctly, with the get param, most apps won't ever be > blocking on any PXP stuff if they don't use it. > > Only the ones that require protected support might block. > > > -Lionel > > >
On Mon, 2023-03-27 at 17:15 +0100, Tvrtko Ursulin wrote: > These two: > e6177ec586d1 ("drm/i915/huc: stall media submission until HuC is loaded") > b76c14c8fb2a ("drm/i915/huc: better define HuC status getparam possible return values.") > They do not help here? It is not possible to use or extend the refined I915_PARAM_HUC_STATUS return values combined with huc load fence for this all to keep working? Checking is-huc-loaded won't reflect is-pxp-available (in case fw/fusing isn't allowing it). But the connection between them is hw-internal so i915 asking PXP-fw to attempt a PXP session depends on HuC (and the 3 other things i mentioned). However, Tvrtko's point on using fences-or-equivalent is the same thing John Harrison brought up offline with Daniele as the proper kernel way to do this type of dependency checking. However, any form of dependency-checking won't improve UMD's experience. We still need to decide if i915-PXP should wait-in-kernel or return some-new-spec-error. A useful data point: we are debugging a related issue on actual customer stack. The compositor using mesa is hitting this code path very early.. even before gsc-proxy component is loaded and we are trying to figure out why delaying inside intel_pxp_start is not helping (more delays causes the gsc-proxy component to also get delayed) but that is a different conversation. I'm only mentioning this because we have a strict requirement to get the desktop login window up within 1-2 seconds of bootloader->kernel handoff. That said, if use -EAGAIN, I'm not sure if that would work as it would delay the compositor startup beyond the typical end user experience unless MESA has a timeout to give up on a cap-testing when seeing repeated -EAGAIN (doubt mesa folks like this?). Perhaps we could just immediately return with a different error (instead of current PXP-UAPI spec of -EINVAL or -ENODEV)... perhaps use -ENXIO which apparently is already part of the original pxp code but was never mentioned in UAPI - but we return this immediately and document it in UAPI as "hw/fw insfratructure is not yet ready to create pxp arb session, user space can retry but may need delays of up to x-seconds on ADl/TGL or y-seconds on MTL, before getting a SUCCESS or one of the other errors).
On Tue, Mar 28, 2023 at 05:01:36PM +0000, Teres Alexis, Alan Previn wrote: > On Mon, 2023-03-27 at 17:15 +0100, Tvrtko Ursulin wrote: > > These two: > > e6177ec586d1 ("drm/i915/huc: stall media submission until HuC is loaded") > > b76c14c8fb2a ("drm/i915/huc: better define HuC status getparam possible return values.") > > They do not help here? It is not possible to use or extend the refined I915_PARAM_HUC_STATUS return values combined with huc load fence for this all to keep working? > Checking is-huc-loaded won't reflect is-pxp-available (in case fw/fusing isn't allowing it). But the connection between them is hw-internal so i915 asking PXP-fw to attempt a PXP > session depends on HuC (and the 3 other things i mentioned). However, Tvrtko's point on using fences-or-equivalent is the same thing John Harrison brought up offline with Daniele > as the proper kernel way to do this type of dependency checking. However, any form of dependency-checking won't improve UMD's experience. We still need to decide if i915-PXP should > wait-in-kernel or return some-new-spec-error. A useful data point: we are debugging a related issue on actual customer stack. The compositor using mesa is hitting this code path > very early.. even before gsc-proxy component is loaded and we are trying to figure out why delaying inside intel_pxp_start is not helping (more delays causes the gsc-proxy > component to also get delayed) but that is a different conversation. I'm only mentioning this because we have a strict requirement to get the desktop login window up within 1-2 > seconds of bootloader->kernel handoff. That said, if use -EAGAIN, I'm not sure if that would work as it would delay the compositor startup beyond the typical end user experience > unless MESA has a timeout to give up on a cap-testing when seeing repeated -EAGAIN (doubt mesa folks like this?). Perhaps we could just immediately return with a different error > (instead of current PXP-UAPI spec of -EINVAL or -ENODEV)... perhaps use -ENXIO which apparently is already part of the original pxp code but was never mentioned in UAPI - but we > return this immediately and document it in UAPI as "hw/fw insfratructure is not yet ready to create pxp arb session, user space can retry but may need delays of up to x-seconds on > ADl/TGL or y-seconds on MTL, before getting a SUCCESS or one of the other errors). fair enough. It looks like we need a new get_param! :)
On 28/03/2023 18:52, Rodrigo Vivi wrote: > On Tue, Mar 28, 2023 at 05:01:36PM +0000, Teres Alexis, Alan Previn wrote: >> On Mon, 2023-03-27 at 17:15 +0100, Tvrtko Ursulin wrote: >>> These two: >>> e6177ec586d1 ("drm/i915/huc: stall media submission until HuC is loaded") >>> b76c14c8fb2a ("drm/i915/huc: better define HuC status getparam possible return values.") >>> They do not help here? It is not possible to use or extend the refined I915_PARAM_HUC_STATUS return values combined with huc load fence for this all to keep working? >> Checking is-huc-loaded won't reflect is-pxp-available (in case fw/fusing isn't allowing it). But the connection between them is hw-internal so i915 asking PXP-fw to attempt a PXP >> session depends on HuC (and the 3 other things i mentioned). However, Tvrtko's point on using fences-or-equivalent is the same thing John Harrison brought up offline with Daniele >> as the proper kernel way to do this type of dependency checking. However, any form of dependency-checking won't improve UMD's experience. We still need to decide if i915-PXP should >> wait-in-kernel or return some-new-spec-error. A useful data point: we are debugging a related issue on actual customer stack. The compositor using mesa is hitting this code path >> very early.. even before gsc-proxy component is loaded and we are trying to figure out why delaying inside intel_pxp_start is not helping (more delays causes the gsc-proxy >> component to also get delayed) but that is a different conversation. I'm only mentioning this because we have a strict requirement to get the desktop login window up within 1-2 >> seconds of bootloader->kernel handoff. That said, if use -EAGAIN, I'm not sure if that would work as it would delay the compositor startup beyond the typical end user experience >> unless MESA has a timeout to give up on a cap-testing when seeing repeated -EAGAIN (doubt mesa folks like this?). Perhaps we could just immediately return with a different error >> (instead of current PXP-UAPI spec of -EINVAL or -ENODEV)... perhaps use -ENXIO which apparently is already part of the original pxp code but was never mentioned in UAPI - but we >> return this immediately and document it in UAPI as "hw/fw insfratructure is not yet ready to create pxp arb session, user space can retry but may need delays of up to x-seconds on >> ADl/TGL or y-seconds on MTL, before getting a SUCCESS or one of the other errors). > > fair enough. It looks like we need a new get_param! :) To check I understood this - new parameter would be like "is pxp support present"? And then later it can still fail to initialize due some parameters not easily detectable during boot/probe? How will the context create path look like on those platforms: 1. Block, then potentially error out if the full initialization failed. 2. Error out "in progress" while initializing, error out "something else" if initialization failed. ? Regards, Tvrtko
On Wed, 2023-03-29 at 08:43 +0100, Tvrtko Ursulin wrote: > On 28/03/2023 18:52, Rodrigo Vivi wrote: > > On Tue, Mar 28, 2023 at 05:01:36PM +0000, Teres Alexis, Alan Previn wrote: > > > On Mon, 2023-03-27 at 17:15 +0100, Tvrtko Ursulin wrote: > > > alan:snip > How will the context create path look like on those platforms: > > 1. Block, then potentially error out if the full initialization failed. > 2. Error out "in progress" while initializing, error out "something > else" if initialization failed. > alan:i was thinking of taking a page from huc-authentication's get-param where we could return different values based on startup progress - in all cases we never block: 1. we dont support it in hw/kernel (i.e. not pxp in device-info or not enough CONFIG_foo - reusing intel_pxp_is_supported?) 2. we support it in kernel but internal dependencies are still in progress (i.e. we have not yet completed huc-loading/huc-authen/proxy-init - UAPI spec should include how many max seconds delay per platform) 3. we support it in kernel but internal dependencies failed (i.e. we know huc-load/authent. failed ... or we know proxy-init failed). 4. we support it in kernel but platform has no support (at this stage we actually attempt to create a PXP context or create the arb-session from inside i915-get-param but we ended up a PXP fw error indicating select list of failures such as fusing / BIOS-config / wrong-version. 0. we support it completely i.e. step 4's attempt to create active PXP session succeeded I want to differentiate 3 and 4 (as opposed to return x-means-ENODEV) because i have am sure it will save debug time when facing customer issues. Ofc we will have to optimize the checking sequences above but at #4, we would be creating a session which might take up to ~200 milisecs for the round trip response from fw. We could store a flag in i915-pxp-internal-struct to indicate if we ever did succeed a pxp creation after a fresh boot ... intel_pxp_is_ready_for_active()? ... true only if we ever did allocate a slot successfully at least once since boot. This also ensure mesa init will return almost immediately except at the first time when hitting #4.
On 30/03/2023 01:10, Teres Alexis, Alan Previn wrote: > > On Wed, 2023-03-29 at 08:43 +0100, Tvrtko Ursulin wrote: >> On 28/03/2023 18:52, Rodrigo Vivi wrote: >>> On Tue, Mar 28, 2023 at 05:01:36PM +0000, Teres Alexis, Alan Previn wrote: >>>> On Mon, 2023-03-27 at 17:15 +0100, Tvrtko Ursulin wrote: >>>> > alan:snip >> How will the context create path look like on those platforms: >> >> 1. Block, then potentially error out if the full initialization failed. >> 2. Error out "in progress" while initializing, error out "something >> else" if initialization failed. >> > alan:i was thinking of taking a page from huc-authentication's get-param where we could return different values based on startup progress - in all cases we never block: If context create never blocks then the only added value of new getparam is just granularity of reported statuses, versus potential overload of errnos from context create? > 1. we dont support it in hw/kernel (i.e. not pxp in device-info or not enough CONFIG_foo - reusing intel_pxp_is_supported?) > 2. we support it in kernel but internal dependencies are still in progress (i.e. we have not yet completed huc-loading/huc-authen/proxy-init - UAPI spec should include how many > max seconds delay per platform) > 3. we support it in kernel but internal dependencies failed (i.e. we know huc-load/authent. failed ... or we know proxy-init failed). > 4. we support it in kernel but platform has no support (at this stage we actually attempt to create a PXP context or create the arb-session from inside i915-get-param but we ended > up a PXP fw error indicating select list of failures such as fusing / BIOS-config / wrong-version. > 0. we support it completely i.e. step 4's attempt to create active PXP session succeeded > > I want to differentiate 3 and 4 (as opposed to return x-means-ENODEV) because i have am sure it will save debug time when facing customer issues. > Ofc we will have to optimize the checking sequences above but at #4, we would be creating a session which might take up to ~200 milisecs for the round trip response from fw. Not sure I get this. If getparam is trying to set this up, which is possibly questionable in itself, where it needs to block for 200ms (max?), and nothing else blocks, then where is the missing max delay mentioned as a starting point for the discussion? Is it expected to elapse while userspace is repeatedly calling getparam and it is getting some "in progress" value? > We could store a flag in i915-pxp-internal-struct to indicate if we ever did succeed a pxp creation after a fresh boot ... intel_pxp_is_ready_for_active()? > ... true only if we ever did allocate a slot successfully at least once since boot. > This also ensure mesa init will return almost immediately except at the first time when hitting #4. Even 200ms is possibly not good enough since boot time targets (to UI AFAIR) are pretty tight. Don't know... Maybe I'd need a timeline diagram showing the involved components to understand this properly. But intuitively I thought that what Mesa wants is a no-cost getparam which would somewhat reliably tell it if the feature is supposed to be there and context create at a later stage, with the protected flag set, is supposed to work. AFAIU it can still fail at that point or probably block until the required setup is done. Regards, Tvrtko
On Thu, 2023-03-30 at 13:25 +0100, Tvrtko Ursulin wrote: > On 30/03/2023 01:10, Teres Alexis, Alan Previn wrote: > > On Wed, 2023-03-29 at 08:43 +0100, Tvrtko Ursulin wrote: > > > On 28/03/2023 18:52, Rodrigo Vivi wrote: > > > > On Tue, Mar 28, 2023 at 05:01:36PM +0000, Teres Alexis, Alan Previn wrote: > > > > > On Mon, 2023-03-27 at 17:15 +0100, Tvrtko Ursulin wrote: alan:snip (excuse my snips - my evolution keeps inserting CRs - still looking for solution) > But intuitively I thought that what Mesa wants is a no-cost getparam > which would somewhat reliably tell it if the feature is supposed to be > there and context create at a later stage, with the protected flag set, > is supposed to work. AFAIU it can still fail at that point or probably > block until the required setup is done. Yes - that's right - i had another round of discussions with Daniele about a cleaner approach - below.. alan:snip > Even 200ms is possibly not good enough since boot time targets (to UI > AFAIR) are pretty tight. Don't know... Maybe I'd need a timeline diagram > showing the involved components to understand this properly. Absolutely, my experiences in i915 on embedded products even had PORs of <1000milisec to first-fully-renderered-display from cold-boot so yes, we need to work with this requirement in mind and do testing on real customer stack. I spoke to Daniele and we have another idea - but would also impact mesa, for the better: 1. Introduce get-param (is_PXP_avail) - will return a simple yes or no - yes means : i915-device-info supports it, kernel configs supports it and required-firmwares were found (not necessarily loaded/init yet). (NOTE: this would be made to hook up to pxp helpers such as intel_pxp_is_supported) 2. Gem-pxp-context-creation continues blocking like today with minor tweak: (same)- success = all dependencies are in place, all firmware init completed, pxp arb session successfully completed. (same)- non-success -ENODEV = if any dependency wasnt available or fw failed to create arb-session due to fw-init-failure/BIOS/platform config. (tweak)- non-success -ENXIO (or some other -E'FOO') if component-driver-init or firmware-init is still pending after brief timeout. - on timeout - TBD - need testing/debug on real world stack. - UAPI spec needs update but pxp implementation currently uses -ENXIO for similiar reason inheritted first merge. Thus, with this: Get param would always be immediate. Pxp-context-creation would only block when all dependencies are in place and we attempt to create the pxp arb session. (firmware can take up to 200-milisecs, according to MTL spec, so I'd say ~210 given other overheads between i915 and fw and back). We would need to change MESA-get-caps to use get-param (and not pxp-context-creation) as it would always return immediately with kernel side support. And if application explicitly requires PXP support, then it needs to call pxp-context-creation that may block or require retry. WRT to fast-boot-to-first-frame, I am hoping real customer stack doesn't require PXP on the compositor and first mesa instance works fine without PXP caps. And when customer apps that needs PXP starts, it would create pxp context which would block but the app would not have a choice. ..alan
On 30/03/2023 20:44, Teres Alexis, Alan Previn wrote: > On Thu, 2023-03-30 at 13:25 +0100, Tvrtko Ursulin wrote: >> On 30/03/2023 01:10, Teres Alexis, Alan Previn wrote: >>> On Wed, 2023-03-29 at 08:43 +0100, Tvrtko Ursulin wrote: >>>> On 28/03/2023 18:52, Rodrigo Vivi wrote: >>>>> On Tue, Mar 28, 2023 at 05:01:36PM +0000, Teres Alexis, Alan Previn wrote: >>>>>> On Mon, 2023-03-27 at 17:15 +0100, Tvrtko Ursulin wrote: > alan:snip (excuse my snips - my evolution keeps inserting CRs - still looking for solution) >> But intuitively I thought that what Mesa wants is a no-cost getparam >> which would somewhat reliably tell it if the feature is supposed to be >> there and context create at a later stage, with the protected flag set, >> is supposed to work. AFAIU it can still fail at that point or probably >> block until the required setup is done. > Yes - that's right - i had another round of discussions with Daniele about a cleaner approach - below.. > alan:snip >> Even 200ms is possibly not good enough since boot time targets (to UI >> AFAIR) are pretty tight. Don't know... Maybe I'd need a timeline diagram >> showing the involved components to understand this properly. > Absolutely, my experiences in i915 on embedded products even had PORs of <1000milisec to first-fully-renderered-display from cold-boot so yes, we need to work with this requirement > in mind and do testing on real customer stack. > > I spoke to Daniele and we have another idea - but would also impact mesa, for the better: > > 1. Introduce get-param (is_PXP_avail) > - will return a simple yes or no > - yes means : i915-device-info supports it, kernel configs supports it and required-firmwares were found (not necessarily loaded/init yet). > (NOTE: this would be made to hook up to pxp helpers such as intel_pxp_is_supported) > 2. Gem-pxp-context-creation continues blocking like today with minor tweak: > (same)- success = all dependencies are in place, all firmware init completed, pxp arb session successfully completed. > (same)- non-success -ENODEV = if any dependency wasnt available or fw failed to create arb-session due to fw-init-failure/BIOS/platform config. > (tweak)- non-success -ENXIO (or some other -E'FOO') if component-driver-init or firmware-init is still pending after brief timeout. > - on timeout - TBD - need testing/debug on real world stack. > - UAPI spec needs update but pxp implementation currently uses -ENXIO for similiar reason inheritted first merge. > > Thus, with this: Get param would always be immediate. Pxp-context-creation would only block when all dependencies are in place and we attempt to create the pxp arb session. > (firmware can take up to 200-milisecs, according to MTL spec, so I'd say ~210 given other overheads between i915 and fw and back). > We would need to change MESA-get-caps to use get-param (and not pxp-context-creation) as it would always return immediately with kernel side support. > And if application explicitly requires PXP support, then it needs to call pxp-context-creation that may block or require retry. The above sounds good to me. I am only not 100% clear on the ENODEV option from context create, does it include even things which can be detected without any timeouts at probe time, or just failures which take time to learn about. > WRT to fast-boot-to-first-frame, I am hoping real customer stack doesn't require PXP on the compositor and first mesa instance works fine without PXP caps. > And when customer apps that needs PXP starts, it would create pxp context which would block but the app would not have a choice. Yeah that sounds like an unlikely use case and one that we cannot improve on the kernel or uapi side. (I can imagine resuming directly into a full screen video playback post suspend, but a cold boot into it is a stretch.) Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c index aecc65b5da70..61041277be24 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c @@ -290,6 +290,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; /* @@ -299,7 +301,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 = GSC_PENDING_RETRY_LATENCY_MS; + else + timeout = 250; + + if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout))) return -ETIMEDOUT; return 0; @@ -307,6 +314,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; /* @@ -315,7 +324,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 = GSC_PENDING_RETRY_LATENCY_MS; + else + timeout = 250; + + if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout))) return -ETIMEDOUT; return 0; @@ -353,8 +367,20 @@ 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)) { + /* + * GSC-fw loading, GSC-proxy init (requiring an mei component driver) and + * HuC-fw loading must all occur first before we start requesting for PXP + * sessions. Checking HuC authentication (the last dependency) will suffice. + * Let's use a much larger 8 second timeout considering all the types of + * dependencies prior to that. + */ + if (wait_for(intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc), 8000)) + 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 b2523d6918c7..9089e02a8c2d 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 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 30aa660a975f..6f5c5dad8c16 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c @@ -9,6 +9,7 @@ #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" @@ -150,6 +151,67 @@ gsccs_send_message_retry_complete(struct intel_pxp *pxp, return ret; } +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_warn(&i915->drm, "Failed to send gsccs msg for creating-session-%d: ret=[%d]\n", + arb_session_id, ret); + else if (msg_out.header.status != 0x0) + drm_warn(&i915->drm, "PXP firmware failed on creating-session-%d: status=0x%08x\n", + arb_session_id, msg_out.header.status); + + 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; + + memset(&msg_in, 0, sizeof(msg_in)); + memset(&msg_out, 0, sizeof(msg_out)); + /* + * 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_warn(&i915->drm, "Failed to send gsccs msg for inv-stream-session-%d: ret=[%d]\n", + session_id, ret); + else if (msg_out.header.status != 0x0) + drm_warn(&i915->drm, "PXP firmware failed on inv-stream-session-%d: status=0x%08x\n", + session_id, msg_out.header.status); +} + static int gsccs_create_buffer(struct intel_gt *gt, const char *bufname, size_t size, diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h index bd1c028bc80f..4d393f1248fe 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h @@ -13,11 +13,15 @@ struct intel_pxp; #define GSC_REPLY_LATENCY_MS 200 #define GSC_PENDING_RETRY_MAXCOUNT 40 #define GSC_PENDING_RETRY_PAUSE_MS 50 +#define GSC_PENDING_RETRY_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) { diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c index b8fa6df7141b..4ddf2ee60222 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. 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> --- drivers/gpu/drm/i915/pxp/intel_pxp.c | 34 ++++++++-- .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 21 +++++++ drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 62 +++++++++++++++++++ drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h | 4 ++ drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 11 +++- 5 files changed, 126 insertions(+), 6 deletions(-)