[Intel-gfx] [PATCH v2 5/9] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages

[Teres Alexis, Alan Previn]

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 at 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(&gt->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)
> >   {
>