[PATCH v2 4/9] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC

[Teres Alexis, Alan Previn]

On Wed, 2023-01-18 at 17:28 -0800, Ceraolo Spurio, Daniele wrote:
> > 
> > 
> > On 1/11/2023 1:42 PM, Alan Previn wrote:
> > > > Add helper functions into (new) common heci-packet-submission files
> > > > to handle generating the MTL GSC-CS Memory-Header and emitting of
> > > > the Heci-Cmd-Packet instructions that gets submitted to the engine.
> > > > 
> > > > NOTE1: This common functions for heci-packet-submission will be used by
> > > > different i915 callers:
> > > >       1- GSC-SW-Proxy: This is pending upstream publication awaiting
> > > >          a few remaining opens
> > > >       2- MTL-HDCP: An equivalent patch has also been published at:
> > > >          https://patchwork.freedesktop.org/series/111876/. (Patch 1)
> > > >       3- PXP: This series.
> > > > 
> > > > NOTE3: Additional clarity about the heci-cmd-pkt layout and where the
> > > >         common helpers come in:
> > > >       - When an internal subsystem needs to send a command request
> > > >         to the security firmware on MTL onwards, it will send that
> > > >         via the GSC-engine-command-streamer.
> > > >       - However those commands, (lets call them "gsc_specific_fw_api"
> > > >         calls), are not understood by the GSC command streamer hw.
> > > >       - The command streamer DOES understand GSC_HECI_CMD_PKT but
> > > >         requires an additional header before the "gsc_specific_fw_api"
> > > >         is sent by the hw engine to the firmware (with additional
> > > >         metadata).
> > 
> > This is slightly incorrect. The GSC CS only looks at the 
> > GSC_HECI_CMD_PKT instruction. The extra header is also passed on the FW 
> > and it is part of the FW specific API. Basically the first header tells 
> > the FW generic info about the message and what type of command it is, 
> > while the second header is instead feature-specific (PXP, HDCP, proxy, etc).
> > 
Alan: yup, my mistake - will fix this.

Alan: [snip]

> > > > +
> > > > +int
> > > > +intel_gsc_uc_heci_cmd_submit_nonpriv(struct intel_gsc_uc *gsc,
> > > > +                                    struct intel_context *ce,
> > > > +                                    struct intel_gsc_heci_non_priv_pkt *pkt,
> > > > +                                    u32 *cs, int timeout_ms)
> > 
> > "cs" is usually used for when we write directly in the ring. Maybe use 
> > "cmd" instead? not a blocker
> > 
Alan: sure.

> > > > +{
> > > > +       struct intel_engine_cs *eng;
> > > > +       struct i915_request *rq;
> > > > +       int err;
> > > > +
> > > > +       rq = intel_context_create_request(ce);
> > > > +       if (IS_ERR(rq))
> > > > +               return PTR_ERR(rq);
> > > > +
> > > > +       emit_gsc_heci_pkt_nonpriv(cs, pkt);
> > > > +
> > > > +       i915_vma_lock(pkt->bb_vma);
> > > > +       err = i915_vma_move_to_active(pkt->bb_vma, rq, EXEC_OBJECT_WRITE);
> > > > +       i915_vma_unlock(pkt->bb_vma);
> > > > +
> > > > +       if (!err) {
> > > > +               i915_vma_lock(pkt->heci_pkt_vma);
> > > > +               err = i915_vma_move_to_active(pkt->heci_pkt_vma, rq, EXEC_OBJECT_WRITE);
> > > > +               i915_vma_unlock(pkt->heci_pkt_vma);
> > > > +       }
> > > > +
> > > > +       eng = rq->context->engine;
> > > > +       if (!err && eng->emit_init_breadcrumb)
> > > > +               err = eng->emit_init_breadcrumb(rq);
> > > > +
> > > > +       if (!err)
> > > > +               err = eng->emit_bb_start(rq, i915_vma_offset(pkt->bb_vma), PAGE_SIZE, 0);
> > > > +
> > > > +       if (err) {
> > > > +               i915_request_add(rq);
> > 
> > You're missing a i915_request_set_error_once here. I suggest using a 
> > goto and running the same code for the request_add in both the passing 
> > and failure cases, like what we do for the pxp session termination 
> > submission.
Alan: got it - will fix accordingly.
> > 
> > > > +               return err;
> > > > +       }
> > > > +
> > > > +       i915_request_get(rq);
> > > > +
> > > > +       i915_request_add(rq);
> > > > +       if (i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE,
> > > > +                             msecs_to_jiffies(timeout_ms)) < 0) {
> > > > +               i915_request_put(rq);
> > > > +               return -ETIME;
> > > > +       }
> > > > +
> > > > +       i915_request_put(rq);
> > > > +
> > > > +       err = ce->engine->emit_flush(rq, 0);
> > > > +       if (err)
> > > > +               drm_err(&gsc_uc_to_gt(gsc)->i915->drm,
> > > > +                       "Failed emit-flush for gsc-heci-non-priv-pkterr=%d\n", err);
> > > > +
> > > > +       if (unlikely(err))
> > > > +               i915_request_set_error_once(rq, err);
> > 
> > the emit_flush and the set_error must be done before the request_add.
Alan: yeah - my bad will do.
> > 
Alan:[snip]

> > > > +struct intel_gsc_mtl_header {
> > > > +       u32 validity_marker;
> > > > +#define GSC_HECI_VALIDITY_MARKER 0xA578875A
> > > > +
> > > > +       u8 heci_client_id;
> > > > +#define GSC_HECI_MEADDRESS_PXP 17
> > > > +#define GSC_HECI_MEADDRESS_HDCP 18
> > > > +
> > > > +       u8 reserved1;
> > > > +
> > > > +       u16 header_version;
> > > > +#define MTL_GSC_HECI_HEADER_VERSION 1
> > > > +
> > > > +       u64 host_session_handle;
> > > > +#define GSC_HECI_HOST_SESSION_USAGE_MASK REG_GENMASK64(63, 60)
> > > > +#define GSC_HECI_SESSION_PXP_SINGLE BIT_ULL(60)
> > 
> > Are those in the specs anywhere? Otherwise, if they're i915-defined, can 
> > you add an explanation on how they're defined?
> > 
> > Daniele

Alan: HW specs allows software to define this as long as its unique per user-process and usage.
This bitfield is something I discussed offline with Suraj to differentiate PXP from HDCP
This would also come in handy for debuggability as well. Will add comments accordingly
> > 
>