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 > > >
