| Message ID | 20230111214226.907536-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 1/11/2023 1:42 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. > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > --- > drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 93 ++++++++++++++++++++++ > 1 file changed, 93 insertions(+) > > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > index 97ca187e6fde..ff235822743e 100644 > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c > @@ -6,6 +6,7 @@ > #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" > @@ -39,6 +40,98 @@ static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp > return (struct gsccs_teelink_priv *)pxp->gsccs_priv; > } > > +static int gsccs_send_message(struct intel_pxp *pxp, This is unused in this patch, so it would cause a compiler warning unless you add the maybe_unused tag (which needs to be removed when the function gets used). It might also be worth squashing this patch with the next one to not have an unused function as they're both relatively small. > + void *msg_in, size_t msg_in_size, > + void *msg_out, size_t msg_out_size_max, > + size_t *msg_out_len) > +{ > + struct intel_gt *gt = pxp->ctrl_gt; > + struct drm_i915_private *i915 = gt->i915; > + struct gsccs_session_resources *exec = &pxp_to_gsccs_priv(pxp)->arb_exec_res; > + struct intel_gsc_mtl_header *header = exec->pkt_vaddr; > + struct intel_gsc_heci_non_priv_pkt pkt; > + size_t max_msg_size; > + u32 reply_size; > + int ret; > + > + if (!intel_uc_uses_gsc_uc(>->uc)) > + return -ENODEV; This also needs a check that the GSC FW is loaded (could also be performed at a higher level). > + > + 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; > + > + mutex_lock(&exec->cmd_mutex); This seems to perform the same role as pxp->tee_mutex, which is unused when we're in gsccs mode. I'm wondering if there is a way to have only one mutex and use it in both scenarios. Not a blocker. Daniele > + > + if (!exec->pkt_vma || !exec->bb_vma) > + return -ENOENT; > + > + 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); > + > + 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(&pxp->ctrl_gt->uc.gsc, > + exec->ce, &pkt, exec->bb_vaddr, 500); > + 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"); > + 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); > + } > + > + 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(&exec->cmd_mutex); > + return ret; > +} > + > int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, > int arb_session_id) > {
Thanks for the review comment... On Wed, 2023-01-18 at 17:40 -0800, Ceraolo Spurio, Daniele wrote: > > > On 1/11/2023 1:42 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. > > > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> > alan:snip.. > This is unused in this patch, so it would cause a compiler warning > unless you add the maybe_unused tag (which needs to be removed when the > function gets used). It might also be worth squashing this patch with > the next one to not have an unused function as they're both relatively > small. > alan: So if i combine the buffer/vma allocations from earlier to here and squash this together with the create-session, then it will become one very large patch. Also, we know that terminate-session might be coming along soon - which i think needs to go together with create- session (assuming that series gets sufficient rb's... nearly there). That said i will keep this as its own patch (pulling in the buffer/vma allocations and freeings) with the maybe_unused tag. Are you okay with this instead? > > + void *msg_in, size_t msg_in_size, > > + void *msg_out, size_t msg_out_size_max, > > + size_t *msg_out_len) > > +{ > > + struct intel_gt *gt = pxp->ctrl_gt; > > + struct drm_i915_private *i915 = gt->i915; > > + struct gsccs_session_resources *exec = &pxp_to_gsccs_priv(pxp)->arb_exec_res; > > + struct intel_gsc_mtl_header *header = exec->pkt_vaddr; > > + struct intel_gsc_heci_non_priv_pkt pkt; > > + size_t max_msg_size; > > + u32 reply_size; > > + int ret; > > + > > + if (!intel_uc_uses_gsc_uc(>->uc)) > > + return -ENODEV; > > This also needs a check that the GSC FW is loaded (could also be > performed at a higher level). > alan: oh yeah - will add that check. > > + > > + 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; > > + > > + mutex_lock(&exec->cmd_mutex); > > This seems to perform the same role as pxp->tee_mutex, which is unused > when we're in gsccs mode. I'm wondering if there is a way to have only > one mutex and use it in both scenarios. Not a blocker. alan: I'll take a look at. > > Daniele > > > + > > + if (!exec->pkt_vma || !exec->bb_vma) > > + return -ENOENT; > > + > > + 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); > > + > > + 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(&pxp->ctrl_gt->uc.gsc, > > + exec->ce, &pkt, exec->bb_vaddr, 500); > > + 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"); > > + 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); > > + } > > + > > + 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(&exec->cmd_mutex); > > + return ret; > > +} > > + > > int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, > > int arb_session_id) > > { >
On Fri, 2023-01-20 at 23:46 +0000, Teres Alexis, Alan Previn wrote: > Thanks for the review comment... > Replying here with the summary of our offline chat: So we concluded that for the next rev, lets move the buffer/vma allocations from patch #2 into this patch #5-send-message. But for now we shall keep patch #6-create-session as separate because when future terminate-session joins this series, it will join #6. That said, #5 and #6 togther would then appear rather large. However, we both agreed that we can always make the final call to squash all of those together (alloc/free + send-message + create-session + terminate-session) at the end when we are merging if we see fit. That would be easy and painless (as opposed to squashing now and then getting a request to re-split later). NOTE: all numbering above will appear as n-minus-1 on next rev because Patch-#1 is going away (the stubs). thanks again...alan
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c index 97ca187e6fde..ff235822743e 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c @@ -6,6 +6,7 @@ #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" @@ -39,6 +40,98 @@ static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp return (struct gsccs_teelink_priv *)pxp->gsccs_priv; } +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) +{ + struct intel_gt *gt = pxp->ctrl_gt; + struct drm_i915_private *i915 = gt->i915; + struct gsccs_session_resources *exec = &pxp_to_gsccs_priv(pxp)->arb_exec_res; + struct intel_gsc_mtl_header *header = exec->pkt_vaddr; + struct intel_gsc_heci_non_priv_pkt pkt; + size_t max_msg_size; + u32 reply_size; + int ret; + + if (!intel_uc_uses_gsc_uc(>->uc)) + return -ENODEV; + + 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; + + mutex_lock(&exec->cmd_mutex); + + if (!exec->pkt_vma || !exec->bb_vma) + return -ENOENT; + + 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); + + 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(&pxp->ctrl_gt->uc.gsc, + exec->ce, &pkt, exec->bb_vaddr, 500); + 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"); + 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); + } + + 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(&exec->cmd_mutex); + return ret; +} + int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, int arb_session_id) {
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. Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com> --- drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 93 ++++++++++++++++++++++ 1 file changed, 93 insertions(+)