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@xxxxxxxxx>
>
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)
> > {
>
