| Message ID | 20230228022150.1657843-5-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 GSC engine based method for sending PXP firmware packets > to the GSC firmware for MTL (and future) products. > > Use the newly added helpers to populate the GSC-CS memory > header and send the message packet to the FW by dispatching > the GSC_HECI_CMD_PKT instruction on the GSC engine. > > We use non-priveleged batches for submission to GSC engine > which require two buffers for the request: > - a buffer for the HECI packet that contains PXP FW commands > - a batch-buffer that contains the engine instruction for > sending the HECI packet to the GSC firmware. > > Thus, add the allocation and freeing of these buffers in gsccs > init and fini. > > The GSC-fw may reply to commands with a SUCCESS but with an > additional pending-bit set in the reply packet. This bit > means the GSC-FW is currently busy and the caller needs to > try again with the gsc_message_handle the fw gave. The > GSC-fw requires a non-zero host_session_handle provided > by the caller to enable gsc_message_handle tracking. > > Thus, allocate the host_session_handle at init and destroy it > at fini (the latter requiring an FYI to the gsc-firmware). > Also ensure the send-message function allows replay of the > gsc_message_handle. > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > --- > .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 4 + > drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 239 +++++++++++++++++- > drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h | 4 + > drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 6 + > 4 files changed, 251 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h > index ad67e3f49c20..b2523d6918c7 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 > @@ -12,6 +12,10 @@ > /* PXP-Cmd-Op definitions */ > #define PXP43_CMDID_START_HUC_AUTH 0x0000003A > > +/* PXP-Packet sizes for MTL's GSCCS-HECI instruction */ > +#define PXP43_MAX_HECI_IN_SIZE (SZ_32K) > +#define PXP43_MAX_HECI_OUT_SIZE (SZ_32K) > + > /* PXP-Input-Packet: HUC-Authentication */ > struct pxp43_start_huc_auth_in { > struct pxp_cmd_header header; > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > index 13693e78b57e..30aa660a975f 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > @@ -6,19 +6,226 @@ > #include "gem/i915_gem_internal.h" > > #include "gt/intel_context.h" > +#include "gt/uc/intel_gsc_uc_heci_cmd_submit.h" > > #include "i915_drv.h" > #include "intel_pxp_cmd_interface_43.h" > #include "intel_pxp_gsccs.h" > #include "intel_pxp_types.h" > > +static int > +gsccs_send_message(struct intel_pxp *pxp, > + void *msg_in, size_t msg_in_size, > + void *msg_out, size_t msg_out_size_max, > + size_t *msg_out_len, > + u64 *gsc_msg_handle_retry) > +{ > + struct intel_gt *gt = pxp->ctrl_gt; > + struct drm_i915_private *i915 = gt->i915; > + struct gsccs_session_resources *exec = &pxp->gsccs_res; in the alloc/free functions you called this object *strm_res; IMO better to use a consistent naming so it is clear they're the same object > + struct intel_gsc_mtl_header *header = exec->pkt_vaddr; > + struct intel_gsc_heci_non_priv_pkt pkt; > + bool null_pkt = !msg_in && !msg_out; > + size_t max_msg_size; > + u32 reply_size; > + int ret; > + > + if (!exec->ce) > + return -ENODEV; > + > + max_msg_size = PXP43_MAX_HECI_IN_SIZE - sizeof(*header); Using the same max_msg_size for both in and out only works if PXP43_MAX_HECI_IN_SIZE == PXP43_MAX_HECI_OUT_SIZE. This is true now, but I'd add a: BUILD_BUG_ON(PXP43_MAX_HECI_IN_SIZE != PXP43_MAX_HECI_OUT_SIZE); just to be safe. Potentially also a: GEM_BUG_ON(exec->pkt_vma->size < (PXP43_MAX_HECI_IN_SIZE + PXP43_MAX_HECI_OUT_SIZE)); After checking that exec->pkt_vma exists. > + > + if (msg_in_size > max_msg_size || msg_out_size_max > max_msg_size) > + return -ENOSPC; > + > + if (!exec->pkt_vma || !exec->bb_vma) > + return -ENOENT; > + > + mutex_lock(&pxp->tee_mutex); > + > + memset(header, 0, sizeof(*header)); > + intel_gsc_uc_heci_cmd_emit_mtl_header(header, GSC_HECI_MEADDRESS_PXP, > + msg_in_size + sizeof(*header), > + exec->host_session_handle); > + > + /* check if this is a host-session-handle cleanup call */ > + if (null_pkt) nit: can't you just use if (!msg_in && !msg_out) instead of a local var? not a blocker. > + header->flags |= GSC_HECI_FLAG_MSG_CLEANUP; > + > + /* copy caller provided gsc message handle if this is polling for a prior msg completion */ > + header->gsc_message_handle = *gsc_msg_handle_retry; > + > + /* NOTE: zero size packets are used for session-cleanups */ > + if (msg_in && msg_in_size) > + memcpy(exec->pkt_vaddr + sizeof(*header), msg_in, msg_in_size); > + > + pkt.addr_in = i915_vma_offset(exec->pkt_vma); > + pkt.size_in = header->message_size; > + pkt.addr_out = pkt.addr_in + PXP43_MAX_HECI_IN_SIZE; > + pkt.size_out = msg_out_size_max + sizeof(*header); > + pkt.heci_pkt_vma = exec->pkt_vma; > + pkt.bb_vma = exec->bb_vma; > + > + ret = intel_gsc_uc_heci_cmd_submit_nonpriv(>->uc.gsc, > + exec->ce, &pkt, exec->bb_vaddr, > + GSC_REPLY_LATENCY_MS); > + if (ret) { > + drm_err(&i915->drm, "failed to send gsc PXP msg (%d)\n", ret); > + goto unlock; > + } > + > + /* we keep separate location for reply, so get the response header loc first */ > + header = exec->pkt_vaddr + PXP43_MAX_HECI_IN_SIZE; > + > + /* Response validity marker, status and busyness */ > + if (header->validity_marker != GSC_HECI_VALIDITY_MARKER) { AFAICS you're not clearing the reply header when you re-send the same packets after the pending bit, so this marker might be stale data. Same for the other fields below. > + drm_err(&i915->drm, "gsc PXP reply with invalid validity marker\n"); > + ret = -EINVAL; > + goto unlock; > + } > + if (header->status != 0) { > + drm_dbg(&i915->drm, "gsc PXP reply status has error = 0x%08x\n", > + header->status); > + ret = -EINVAL; > + goto unlock; > + } > + if (header->flags & GSC_HECI_FLAG_MSG_PENDING) { > + drm_dbg(&i915->drm, "gsc PXP reply is busy\n"); > + /* > + * When the GSC firmware replies with pending bit, it means that the requested > + * operation has begun but the completion is pending and the caller needs > + * to re-request with the gsc_message_handle that was returned by the firmware. > + * until the pending bit is turned off. > + */ > + *gsc_msg_handle_retry = header->gsc_message_handle; Non blocking question: would it be worth it to copy the value to the header_in directly, instead of returning the value to the caller and copying it on resubmit? Just a thought, I see pro and cons with both approaches. > + ret = -EAGAIN; > + goto unlock; > + } > + > + reply_size = header->message_size - sizeof(*header); > + if (reply_size > msg_out_size_max) { > + drm_warn(&i915->drm, "caller with insufficient PXP reply size %u (%ld)\n", > + reply_size, msg_out_size_max); > + reply_size = msg_out_size_max; > + } else if (reply_size != msg_out_size_max) { > + drm_dbg(&i915->drm, "caller unexpected PXP reply size %u (%ld)\n", > + reply_size, msg_out_size_max); Are we expecting all caller to always pass the exact size? Not a complain, but if that's the case then maybe rename msg_out_size_max to msg_out_expected_size, so it's clearer. size_max sounds like any size smaller than it is allowed. My personal preference would be to leave this as a size_max and have the caller decide if the actual returned size matches the expectations (via msg_out_len) > + } > + > + if (msg_out) > + memcpy(msg_out, exec->pkt_vaddr + PXP43_MAX_HECI_IN_SIZE + sizeof(*header), > + reply_size); > + if (msg_out_len) > + *msg_out_len = reply_size; > + > +unlock: > + mutex_unlock(&pxp->tee_mutex); > + return ret; > +} > + > +static int > +gsccs_send_message_retry_complete(struct intel_pxp *pxp, > + void *msg_in, size_t msg_in_size, > + void *msg_out, size_t msg_out_size_max, > + size_t *msg_out_len) > +{ > + u64 gsc_session_retry = 0; > + int ret, tries = 0; > + > + /* > + * Keep sending request if GSC firmware was busy. Based on fw specs + > + * sw overhead (and testing) we expect a worst case pending-bit delay of > + * GSC_PENDING_RETRY_MAXCOUNT x GSC_PENDING_RETRY_PAUSE_MS millisecs. > + */ > + do { > + ret = gsccs_send_message(pxp, msg_in, msg_in_size, msg_out, msg_out_size_max, > + msg_out_len, &gsc_session_retry); > + /* Only try again if gsc says so */ > + if (ret != -EAGAIN) > + break; > + > + msleep(GSC_PENDING_RETRY_PAUSE_MS); > + } while (++tries < GSC_PENDING_RETRY_MAXCOUNT); > + > + return ret; > +} > + > +static int > +gsccs_create_buffer(struct intel_gt *gt, > + const char *bufname, size_t size, > + struct i915_vma **vma, void **map) > +{ > + struct drm_i915_private *i915 = gt->i915; > + struct drm_i915_gem_object *obj; > + int err = 0; > + > + obj = i915_gem_object_create_internal(i915, size); > + if (IS_ERR(obj)) { > + drm_err(&i915->drm, "Failed to allocate gsccs backend %s.\n", bufname); > + err = PTR_ERR(obj); > + goto out_none; > + } > + > + *vma = i915_vma_instance(obj, gt->vm, NULL); > + if (IS_ERR(*vma)) { > + drm_err(&i915->drm, "Failed to vma-instance gsccs backend %s.\n", bufname); > + err = PTR_ERR(*vma); > + goto out_put; > + } > + > + /* return a virtual pointer */ > + *map = i915_gem_object_pin_map_unlocked(obj, i915_coherent_map_type(i915, obj, true)); > + if (IS_ERR(*map)) { > + drm_err(&i915->drm, "Failed to map gsccs backend %s.\n", bufname); > + err = PTR_ERR(*map); > + goto out_put; > + } > + > + /* all PXP sessions commands are treated as non-priveleged */ typo priveleged > + err = i915_vma_pin(*vma, 0, 0, PIN_USER); > + if (err) { > + drm_err(&i915->drm, "Failed to vma-pin gsccs backend %s.\n", bufname); > + goto out_unmap; > + } > + > + return 0; > + > +out_unmap: > + i915_gem_object_unpin_map(obj); > +out_put: > + i915_gem_object_put(obj); > +out_none: > + *vma = NULL; > + *map = NULL; > + > + return err; > +} > + nit: maybe move gsccs_create_buffer after the cleanup/destruction functions? so we can group all the creation functions close together. > +static void > +gsccs_cleanup_fw_host_session_handle(struct intel_pxp *pxp) > +{ > + struct drm_i915_private *i915 = pxp->ctrl_gt->i915; > + int ret; > + > + ret = gsccs_send_message_retry_complete(pxp, NULL, 0, NULL, 0, NULL); > + if (ret) > + drm_dbg(&i915->drm, "Failed to send gsccs msg host-session-cleanup: ret=[%d]\n", > + ret); > +} > + > static void > gsccs_destroy_execution_resource(struct intel_pxp *pxp) > { > struct gsccs_session_resources *strm_res = &pxp->gsccs_res; > > + if (strm_res->host_session_handle) > + gsccs_cleanup_fw_host_session_handle(pxp); > if (strm_res->ce) > intel_context_put(strm_res->ce); > + if (strm_res->bb_vma) > + i915_vma_unpin_and_release(&strm_res->bb_vma, I915_VMA_RELEASE_MAP); > + if (strm_res->pkt_vma) > + i915_vma_unpin_and_release(&strm_res->pkt_vma, I915_VMA_RELEASE_MAP); > > memset(strm_res, 0, sizeof(*strm_res)); > } > @@ -30,6 +237,7 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp) > struct gsccs_session_resources *strm_res = &pxp->gsccs_res; > struct intel_engine_cs *engine = gt->engine[GSC0]; > struct intel_context *ce; > + int err = 0; > > /* > * First, ensure the GSC engine is present. > @@ -38,16 +246,43 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp) > if (!engine) > return -ENODEV; > > + /* > + * Now, allocate, pin and map two objects, one for the heci message packet > + * and another for the batch buffer we submit into GSC engine (that includes the packet). > + * NOTE: GSC-CS backend is currently only supported on MTL, so we allocate shmem. > + */ > + err = gsccs_create_buffer(pxp->ctrl_gt, "Heci Packet", > + PXP43_MAX_HECI_IN_SIZE + PXP43_MAX_HECI_OUT_SIZE, > + &strm_res->pkt_vma, &strm_res->pkt_vaddr); > + if (err) > + return err; > + > + err = gsccs_create_buffer(pxp->ctrl_gt, "Batch Buffer", PAGE_SIZE, > + &strm_res->bb_vma, &strm_res->bb_vaddr); > + if (err) > + goto free_pkt; > + > /* Finally, create an intel_context to be used during the submission */ > ce = intel_context_create(engine); > if (IS_ERR(ce)) { > drm_err(>->i915->drm, "Failed creating gsccs backend ctx\n"); > - return PTR_ERR(ce); > + err = PTR_ERR(ce); > + goto free_batch; > } > - > strm_res->ce = ce; > > + /* initialize host-session-handle (for all i915-to-gsc-firmware PXP cmds) */ > + get_random_bytes(&strm_res->host_session_handle, sizeof(strm_res->host_session_handle)); This does not guarantee that each host session handle is unique (although getting the same u64 twice is going to be extremely extremely unlikely). Not sure if it is a problem. > + > return 0; > + > +free_pkt: > + i915_vma_unpin_and_release(&strm_res->pkt_vma, I915_VMA_RELEASE_MAP); > +free_batch: > + i915_vma_unpin_and_release(&strm_res->bb_vma, I915_VMA_RELEASE_MAP); those gotos are the wrong way around, the pkt is allocated first and therefore it should be freed second Daniele > + memset(strm_res, 0, sizeof(*strm_res)); > + > + return err; > } > > void intel_pxp_gsccs_fini(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 354ea9a8f940..bd1c028bc80f 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h > @@ -10,6 +10,10 @@ > > struct intel_pxp; > > +#define GSC_REPLY_LATENCY_MS 200 > +#define GSC_PENDING_RETRY_MAXCOUNT 40 > +#define GSC_PENDING_RETRY_PAUSE_MS 50 > + > #ifdef CONFIG_DRM_I915_PXP > void intel_pxp_gsccs_fini(struct intel_pxp *pxp); > int intel_pxp_gsccs_init(struct intel_pxp *pxp); > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > index fdd98911968d..73392fbab7ee 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h > @@ -38,6 +38,12 @@ struct intel_pxp { > struct gsccs_session_resources { > u64 host_session_handle; /* used by firmware to link commands to sessions */ > struct intel_context *ce; /* context for gsc command submission */ > + > + struct i915_vma *pkt_vma; /* GSC FW cmd packet vma */ > + void *pkt_vaddr; /* GSC FW cmd packet virt pointer */ > + > + struct i915_vma *bb_vma; /* HECI_PKT batch buffer vma */ > + void *bb_vaddr; /* HECI_PKT batch buffer virt pointer */ > } gsccs_res; > > /**
Hi Daniele - thanks for reviewing this - i will fix all of code in accordance to the review comments you provided with some exceptions / alternatives: On Fri, 2023-03-03 at 17:07 -0800, Ceraolo Spurio, Daniele wrote: > > On 2/27/2023 6:21 PM, Alan Previn wrote: > > Add GSC engine based method for sending PXP firmware packets > > to the GSC firmware for MTL (and future) products. > > > > Use the newly added helpers to populate the GSC-CS memory > > header and send the message packet to the FW by dispatching > > the GSC_HECI_CMD_PKT instruction on the GSC engine. > > +gsccs_send_message(struct intel_pxp *pxp, > > + void *msg_in, size_t msg_in_size, > > + void *msg_out, size_t msg_out_size_max, > > + size_t *msg_out_len, > > + u64 *gsc_msg_handle_retry) > > +{ > > + struct intel_gt *gt = pxp->ctrl_gt; > > + struct drm_i915_private *i915 = gt->i915; > > + struct gsccs_session_resources *exec = &pxp->gsccs_res; > > in the alloc/free functions you called this object *strm_res; IMO better > to use a consistent naming so it is clear they're the same object > alan: agred - actually i think i will go with "exec_res" across the board instead. alan:snip > > + max_msg_size = PXP43_MAX_HECI_IN_SIZE - sizeof(*header); > > Using the same max_msg_size for both in and out only works if > PXP43_MAX_HECI_IN_SIZE == PXP43_MAX_HECI_OUT_SIZE. This is true now, but > I'd add a: > BUILD_BUG_ON(PXP43_MAX_HECI_IN_SIZE != PXP43_MAX_HECI_OUT_SIZE); > just to be safe. Potentially also a: > GEM_BUG_ON(exec->pkt_vma->size < (PXP43_MAX_HECI_IN_SIZE + > PXP43_MAX_HECI_OUT_SIZE)); > After checking that exec->pkt_vma exists. > alan: actually an even simpler alternative would be to just use #define PXP43_MAX_HECI_INOUT_SIZE - a single definition that will make both the code and logic easier - it is after reflecting the HW spec too where the total sizes would be 2xPXP43_MAX_HECI_INOUT_SIZE alan:snip > > > nit: can't you just use if (!msg_in && !msg_out) instead of a local var? > not a blocker. alan:sure alan:snip > > + /* Response validity marker, status and busyness */ > > + if (header->validity_marker != GSC_HECI_VALIDITY_MARKER) { > > AFAICS you're not clearing the reply header when you re-send the same > packets after the pending bit, so this marker might be stale data. Same > for the other fields below. Good catch - I can clear it before i do the submission - and i assume you agree that clearing the validity marker alone (in the reply offset) is sufficient here to strenghten this check. alan:snip > > > > + if (header->flags & GSC_HECI_FLAG_MSG_PENDING) { > > + drm_dbg(&i915->drm, "gsc PXP reply is busy\n"); > > + /* > > + * When the GSC firmware replies with pending bit, it means that the requested > > + * operation has begun but the completion is pending and the caller needs > > + * to re-request with the gsc_message_handle that was returned by the firmware. > > + * until the pending bit is turned off. > > + */ > > + *gsc_msg_handle_retry = header->gsc_message_handle; > > Non blocking question: would it be worth it to copy the value to the > header_in directly, instead of returning the value to the caller and > copying it on resubmit? Just a thought, I see pro and cons with both > approaches. > alan: Hmm - good idea - okay - let me think about this one... although i prefer the control be on the caller's side. alan:snip > > + } else if (reply_size != msg_out_size_max) { > > + drm_dbg(&i915->drm, "caller unexpected PXP reply size %u (%ld)\n", > > + reply_size, msg_out_size_max); > Are we expecting all caller to always pass the exact size? Not a > complain, but if that's the case then maybe rename msg_out_size_max to > msg_out_expected_size, so it's clearer. size_max sounds like any size > smaller than it is allowed. My personal preference would be to leave > this as a size_max and have the caller decide if the actual returned > size matches the expectations (via msg_out_len) > No, i am allowing the user to provide buffers bigger than what the reply In which case you are right- i can let the caller do the size checking. and remove that dbg msg. alan:snip > > + /* all PXP sessions commands are treated as non-priveleged */ > typo priveleged will fix. > nit: maybe move gsccs_create_buffer after the cleanup/destruction > functions? so we can group all the creation functions close together. > i was also think of moving functions around to group them but i want to keep the init/fini right at the bottomg. > > +static void > > +gsccs_cleanup_fw_host_session_handle(struct intel_pxp *pxp) > > +{ > > + struct drm_i915_private *i915 = pxp->ctrl_gt->i915; > > + int ret; > > + > > + ret = gsccs_send_message_retry_complete(pxp, NULL, 0, NULL, 0, NULL); > > + if (ret) > > + drm_dbg(&i915->drm, "Failed to send gsccs msg host-session-cleanup: ret=[%d]\n", > > + ret); > > +} > > + > > static void > > gsccs_destroy_execution_resource(struct intel_pxp *pxp) > > { > > struct gsccs_session_resources *strm_res = &pxp->gsccs_res; > > > > + if (strm_res->host_session_handle) > > + gsccs_cleanup_fw_host_session_handle(pxp); > > if (strm_res->ce) > > intel_context_put(strm_res->ce); > > + if (strm_res->bb_vma) > > + i915_vma_unpin_and_release(&strm_res->bb_vma, I915_VMA_RELEASE_MAP); > > + if (strm_res->pkt_vma) > > + i915_vma_unpin_and_release(&strm_res->pkt_vma, I915_VMA_RELEASE_MAP); > > > > memset(strm_res, 0, sizeof(*strm_res)); > > } > > @@ -30,6 +237,7 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp) > > struct gsccs_session_resources *strm_res = &pxp->gsccs_res; > > struct intel_engine_cs *engine = gt->engine[GSC0]; > > struct intel_context *ce; > > + int err = 0; > > > > /* > > * First, ensure the GSC engine is present. > > @@ -38,16 +246,43 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp) > > if (!engine) > > return -ENODEV; > > > > + /* > > + * Now, allocate, pin and map two objects, one for the heci message packet > > + * and another for the batch buffer we submit into GSC engine (that includes the packet). > > + * NOTE: GSC-CS backend is currently only supported on MTL, so we allocate shmem. > > + */ > > + err = gsccs_create_buffer(pxp->ctrl_gt, "Heci Packet", > > + PXP43_MAX_HECI_IN_SIZE + PXP43_MAX_HECI_OUT_SIZE, > > + &strm_res->pkt_vma, &strm_res->pkt_vaddr); > > + if (err) > > + return err; > > + > > + err = gsccs_create_buffer(pxp->ctrl_gt, "Batch Buffer", PAGE_SIZE, > > + &strm_res->bb_vma, &strm_res->bb_vaddr); > > + if (err) > > + goto free_pkt; > > + > > /* Finally, create an intel_context to be used during the submission */ > > ce = intel_context_create(engine); > > if (IS_ERR(ce)) { > > drm_err(>->i915->drm, "Failed creating gsccs backend ctx\n"); > > - return PTR_ERR(ce); > > + err = PTR_ERR(ce); > > + goto free_batch; > > } > > - > > strm_res->ce = ce; > > > > + /* initialize host-session-handle (for all i915-to-gsc-firmware PXP cmds) */ > > + get_random_bytes(&strm_res->host_session_handle, sizeof(strm_res->host_session_handle)); > > This does not guarantee that each host session handle is unique > (although getting the same u64 twice is going to be extremely extremely > unlikely). Not sure if it is a problem. > yes, you are correct.. i am hoping this is sufficioent. > > + > > return 0; > > + > > +free_pkt: > > + i915_vma_unpin_and_release(&strm_res->pkt_vma, I915_VMA_RELEASE_MAP); > > +free_batch: > > + i915_vma_unpin_and_release(&strm_res->bb_vma, I915_VMA_RELEASE_MAP); > > those gotos are the wrong way around, the pkt is allocated first and > therefore it should be freed second alan: yeah - my mistake - will fix it - thanks.
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 ad67e3f49c20..b2523d6918c7 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 @@ -12,6 +12,10 @@ /* PXP-Cmd-Op definitions */ #define PXP43_CMDID_START_HUC_AUTH 0x0000003A +/* PXP-Packet sizes for MTL's GSCCS-HECI instruction */ +#define PXP43_MAX_HECI_IN_SIZE (SZ_32K) +#define PXP43_MAX_HECI_OUT_SIZE (SZ_32K) + /* PXP-Input-Packet: HUC-Authentication */ struct pxp43_start_huc_auth_in { struct pxp_cmd_header header; diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c index 13693e78b57e..30aa660a975f 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c @@ -6,19 +6,226 @@ #include "gem/i915_gem_internal.h" #include "gt/intel_context.h" +#include "gt/uc/intel_gsc_uc_heci_cmd_submit.h" #include "i915_drv.h" #include "intel_pxp_cmd_interface_43.h" #include "intel_pxp_gsccs.h" #include "intel_pxp_types.h" +static int +gsccs_send_message(struct intel_pxp *pxp, + void *msg_in, size_t msg_in_size, + void *msg_out, size_t msg_out_size_max, + size_t *msg_out_len, + u64 *gsc_msg_handle_retry) +{ + struct intel_gt *gt = pxp->ctrl_gt; + struct drm_i915_private *i915 = gt->i915; + struct gsccs_session_resources *exec = &pxp->gsccs_res; + struct intel_gsc_mtl_header *header = exec->pkt_vaddr; + struct intel_gsc_heci_non_priv_pkt pkt; + bool null_pkt = !msg_in && !msg_out; + size_t max_msg_size; + u32 reply_size; + int ret; + + if (!exec->ce) + return -ENODEV; + + max_msg_size = PXP43_MAX_HECI_IN_SIZE - sizeof(*header); + + if (msg_in_size > max_msg_size || msg_out_size_max > max_msg_size) + return -ENOSPC; + + if (!exec->pkt_vma || !exec->bb_vma) + return -ENOENT; + + mutex_lock(&pxp->tee_mutex); + + memset(header, 0, sizeof(*header)); + intel_gsc_uc_heci_cmd_emit_mtl_header(header, GSC_HECI_MEADDRESS_PXP, + msg_in_size + sizeof(*header), + exec->host_session_handle); + + /* check if this is a host-session-handle cleanup call */ + if (null_pkt) + header->flags |= GSC_HECI_FLAG_MSG_CLEANUP; + + /* copy caller provided gsc message handle if this is polling for a prior msg completion */ + header->gsc_message_handle = *gsc_msg_handle_retry; + + /* NOTE: zero size packets are used for session-cleanups */ + if (msg_in && msg_in_size) + memcpy(exec->pkt_vaddr + sizeof(*header), msg_in, msg_in_size); + + pkt.addr_in = i915_vma_offset(exec->pkt_vma); + pkt.size_in = header->message_size; + pkt.addr_out = pkt.addr_in + PXP43_MAX_HECI_IN_SIZE; + pkt.size_out = msg_out_size_max + sizeof(*header); + pkt.heci_pkt_vma = exec->pkt_vma; + pkt.bb_vma = exec->bb_vma; + + ret = intel_gsc_uc_heci_cmd_submit_nonpriv(>->uc.gsc, + exec->ce, &pkt, exec->bb_vaddr, + GSC_REPLY_LATENCY_MS); + if (ret) { + drm_err(&i915->drm, "failed to send gsc PXP msg (%d)\n", ret); + goto unlock; + } + + /* we keep separate location for reply, so get the response header loc first */ + header = exec->pkt_vaddr + PXP43_MAX_HECI_IN_SIZE; + + /* Response validity marker, status and busyness */ + if (header->validity_marker != GSC_HECI_VALIDITY_MARKER) { + drm_err(&i915->drm, "gsc PXP reply with invalid validity marker\n"); + ret = -EINVAL; + goto unlock; + } + if (header->status != 0) { + drm_dbg(&i915->drm, "gsc PXP reply status has error = 0x%08x\n", + header->status); + ret = -EINVAL; + goto unlock; + } + if (header->flags & GSC_HECI_FLAG_MSG_PENDING) { + drm_dbg(&i915->drm, "gsc PXP reply is busy\n"); + /* + * When the GSC firmware replies with pending bit, it means that the requested + * operation has begun but the completion is pending and the caller needs + * to re-request with the gsc_message_handle that was returned by the firmware. + * until the pending bit is turned off. + */ + *gsc_msg_handle_retry = header->gsc_message_handle; + ret = -EAGAIN; + goto unlock; + } + + reply_size = header->message_size - sizeof(*header); + if (reply_size > msg_out_size_max) { + drm_warn(&i915->drm, "caller with insufficient PXP reply size %u (%ld)\n", + reply_size, msg_out_size_max); + reply_size = msg_out_size_max; + } else if (reply_size != msg_out_size_max) { + drm_dbg(&i915->drm, "caller unexpected PXP reply size %u (%ld)\n", + reply_size, msg_out_size_max); + } + + if (msg_out) + memcpy(msg_out, exec->pkt_vaddr + PXP43_MAX_HECI_IN_SIZE + sizeof(*header), + reply_size); + if (msg_out_len) + *msg_out_len = reply_size; + +unlock: + mutex_unlock(&pxp->tee_mutex); + return ret; +} + +static int +gsccs_send_message_retry_complete(struct intel_pxp *pxp, + void *msg_in, size_t msg_in_size, + void *msg_out, size_t msg_out_size_max, + size_t *msg_out_len) +{ + u64 gsc_session_retry = 0; + int ret, tries = 0; + + /* + * Keep sending request if GSC firmware was busy. Based on fw specs + + * sw overhead (and testing) we expect a worst case pending-bit delay of + * GSC_PENDING_RETRY_MAXCOUNT x GSC_PENDING_RETRY_PAUSE_MS millisecs. + */ + do { + ret = gsccs_send_message(pxp, msg_in, msg_in_size, msg_out, msg_out_size_max, + msg_out_len, &gsc_session_retry); + /* Only try again if gsc says so */ + if (ret != -EAGAIN) + break; + + msleep(GSC_PENDING_RETRY_PAUSE_MS); + } while (++tries < GSC_PENDING_RETRY_MAXCOUNT); + + return ret; +} + +static int +gsccs_create_buffer(struct intel_gt *gt, + const char *bufname, size_t size, + struct i915_vma **vma, void **map) +{ + struct drm_i915_private *i915 = gt->i915; + struct drm_i915_gem_object *obj; + int err = 0; + + obj = i915_gem_object_create_internal(i915, size); + if (IS_ERR(obj)) { + drm_err(&i915->drm, "Failed to allocate gsccs backend %s.\n", bufname); + err = PTR_ERR(obj); + goto out_none; + } + + *vma = i915_vma_instance(obj, gt->vm, NULL); + if (IS_ERR(*vma)) { + drm_err(&i915->drm, "Failed to vma-instance gsccs backend %s.\n", bufname); + err = PTR_ERR(*vma); + goto out_put; + } + + /* return a virtual pointer */ + *map = i915_gem_object_pin_map_unlocked(obj, i915_coherent_map_type(i915, obj, true)); + if (IS_ERR(*map)) { + drm_err(&i915->drm, "Failed to map gsccs backend %s.\n", bufname); + err = PTR_ERR(*map); + goto out_put; + } + + /* all PXP sessions commands are treated as non-priveleged */ + err = i915_vma_pin(*vma, 0, 0, PIN_USER); + if (err) { + drm_err(&i915->drm, "Failed to vma-pin gsccs backend %s.\n", bufname); + goto out_unmap; + } + + return 0; + +out_unmap: + i915_gem_object_unpin_map(obj); +out_put: + i915_gem_object_put(obj); +out_none: + *vma = NULL; + *map = NULL; + + return err; +} + +static void +gsccs_cleanup_fw_host_session_handle(struct intel_pxp *pxp) +{ + struct drm_i915_private *i915 = pxp->ctrl_gt->i915; + int ret; + + ret = gsccs_send_message_retry_complete(pxp, NULL, 0, NULL, 0, NULL); + if (ret) + drm_dbg(&i915->drm, "Failed to send gsccs msg host-session-cleanup: ret=[%d]\n", + ret); +} + static void gsccs_destroy_execution_resource(struct intel_pxp *pxp) { struct gsccs_session_resources *strm_res = &pxp->gsccs_res; + if (strm_res->host_session_handle) + gsccs_cleanup_fw_host_session_handle(pxp); if (strm_res->ce) intel_context_put(strm_res->ce); + if (strm_res->bb_vma) + i915_vma_unpin_and_release(&strm_res->bb_vma, I915_VMA_RELEASE_MAP); + if (strm_res->pkt_vma) + i915_vma_unpin_and_release(&strm_res->pkt_vma, I915_VMA_RELEASE_MAP); memset(strm_res, 0, sizeof(*strm_res)); } @@ -30,6 +237,7 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp) struct gsccs_session_resources *strm_res = &pxp->gsccs_res; struct intel_engine_cs *engine = gt->engine[GSC0]; struct intel_context *ce; + int err = 0; /* * First, ensure the GSC engine is present. @@ -38,16 +246,43 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp) if (!engine) return -ENODEV; + /* + * Now, allocate, pin and map two objects, one for the heci message packet + * and another for the batch buffer we submit into GSC engine (that includes the packet). + * NOTE: GSC-CS backend is currently only supported on MTL, so we allocate shmem. + */ + err = gsccs_create_buffer(pxp->ctrl_gt, "Heci Packet", + PXP43_MAX_HECI_IN_SIZE + PXP43_MAX_HECI_OUT_SIZE, + &strm_res->pkt_vma, &strm_res->pkt_vaddr); + if (err) + return err; + + err = gsccs_create_buffer(pxp->ctrl_gt, "Batch Buffer", PAGE_SIZE, + &strm_res->bb_vma, &strm_res->bb_vaddr); + if (err) + goto free_pkt; + /* Finally, create an intel_context to be used during the submission */ ce = intel_context_create(engine); if (IS_ERR(ce)) { drm_err(>->i915->drm, "Failed creating gsccs backend ctx\n"); - return PTR_ERR(ce); + err = PTR_ERR(ce); + goto free_batch; } - strm_res->ce = ce; + /* initialize host-session-handle (for all i915-to-gsc-firmware PXP cmds) */ + get_random_bytes(&strm_res->host_session_handle, sizeof(strm_res->host_session_handle)); + return 0; + +free_pkt: + i915_vma_unpin_and_release(&strm_res->pkt_vma, I915_VMA_RELEASE_MAP); +free_batch: + i915_vma_unpin_and_release(&strm_res->bb_vma, I915_VMA_RELEASE_MAP); + memset(strm_res, 0, sizeof(*strm_res)); + + return err; } void intel_pxp_gsccs_fini(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 354ea9a8f940..bd1c028bc80f 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h @@ -10,6 +10,10 @@ struct intel_pxp; +#define GSC_REPLY_LATENCY_MS 200 +#define GSC_PENDING_RETRY_MAXCOUNT 40 +#define GSC_PENDING_RETRY_PAUSE_MS 50 + #ifdef CONFIG_DRM_I915_PXP void intel_pxp_gsccs_fini(struct intel_pxp *pxp); int intel_pxp_gsccs_init(struct intel_pxp *pxp); diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h index fdd98911968d..73392fbab7ee 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h @@ -38,6 +38,12 @@ struct intel_pxp { struct gsccs_session_resources { u64 host_session_handle; /* used by firmware to link commands to sessions */ struct intel_context *ce; /* context for gsc command submission */ + + struct i915_vma *pkt_vma; /* GSC FW cmd packet vma */ + void *pkt_vaddr; /* GSC FW cmd packet virt pointer */ + + struct i915_vma *bb_vma; /* HECI_PKT batch buffer vma */ + void *bb_vaddr; /* HECI_PKT batch buffer virt pointer */ } gsccs_res; /**
Add GSC engine based method for sending PXP firmware packets to the GSC firmware for MTL (and future) products. Use the newly added helpers to populate the GSC-CS memory header and send the message packet to the FW by dispatching the GSC_HECI_CMD_PKT instruction on the GSC engine. We use non-priveleged batches for submission to GSC engine which require two buffers for the request: - a buffer for the HECI packet that contains PXP FW commands - a batch-buffer that contains the engine instruction for sending the HECI packet to the GSC firmware. Thus, add the allocation and freeing of these buffers in gsccs init and fini. The GSC-fw may reply to commands with a SUCCESS but with an additional pending-bit set in the reply packet. This bit means the GSC-FW is currently busy and the caller needs to try again with the gsc_message_handle the fw gave. The GSC-fw requires a non-zero host_session_handle provided by the caller to enable gsc_message_handle tracking. Thus, allocate the host_session_handle at init and destroy it at fini (the latter requiring an FYI to the gsc-firmware). Also ensure the send-message function allows replay of the gsc_message_handle. Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> --- .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 4 + drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 239 +++++++++++++++++- drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h | 4 + drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 6 + 4 files changed, 251 insertions(+), 2 deletions(-)