Re: [Intel-gfx] [PATCH v7 3/8] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC
On 4/17/2023 10:56 AM, Teres Alexis, Alan Previn wrote:
On Mon, 2023-04-10 at 09:10 -0700, Ceraolo Spurio, Daniele wrote:
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 *cmd, int timeout_ms)
+{
+ struct intel_engine_cs *eng;
We always use "engine" for engine_cs variables. IMO it's better to stick
to that here as well for consistency across the code.
alan: will do
+ struct i915_gem_ww_ctx ww;
+ struct i915_request *rq;
+ int err, trials = 0;
+
Is the assumption that the caller is holding a wakeref already?
Otherwise we're going to need and engine_pm_get() here (assuming it
doesn't interfere with any locking, otherwise it has to be in the caller)
alan: right now the only places this can get called from is via
intel_pxp_gsccs_create_session or
intel_pxp_gsccs_end_arb_fw_session. These functions are either being called by
intel_pxp_start
or intel_pxp_end. intel_pxp_start calls intel_runtime_pm_get_if_in_use
indirectly from the
session-worker and while intel_pxp_end takes an explicit intel_runtime_pm_get
(since it is
for suspend/shutdown cleanup and doesn't use the worker). I'm assuming
runtime_pm works right?
we have a similar logic across the paths from ADL version where we dont take
explicit
engine_pm_get for the termination via VDBOX because its part of the same code
paths.
rpm_get works for the power management side, but not for "activeness"
tracking, for which we need engine_pm_get. However, I've just realized
that the context_pin contains an engine_pm_get, so we're covered.
Therefore, with the other minor comments addressed, this is:
Reviewed-by: Daniele Ceraolo Spurio
Daniele
alan:snip
+ err = i915_vma_move_to_active(pkt->bb_vma, rq, EXEC_OBJECT_WRITE);
nit: I don't think we need EXEC_OBJECT_WRITE for the bb as we're not
going to write it.
alan: yes - will remove. (had accidentally kept above flag from offline
debugging version of the code that had additional store dwords into bb).
+ if (err)
+ goto out_rq;
+ err = i915_vma_move_to_active(pkt->heci_pkt_vma, rq, EXEC_OBJECT_WRITE);
+ if (err)
+ goto out_rq;
+
+ eng = rq->context->engine;
+ if (eng->emit_init_breadcrumb) {
+ err = eng->emit_init_breadcrumb(rq);
+ if (err)
+ goto out_rq;
+ }
+
+ err = eng->emit_bb_start(rq, i915_vma_offset(pkt->bb_vma), PAGE_SIZE,
0);
+ if (err)
+ goto out_rq;
+
+ err = ce->engine->emit_flush(rq, 0);
+ if (err)
+ drm_err(_uc_to_gt(gsc)->i915->drm,
+ "Failed emit-flush for gsc-heci-non-priv-pkterr=%d\n",
err);
+
+out_rq:
+ i915_request_get(rq);
+
+ if (unlikely(err))
+ i915_request_set_error_once(rq, err);
+
+ i915_request_add(rq);
+
+ if (!err) {
+ if (i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE,
+ msecs_to_jiffies(timeout_ms)) < 0)
+ err = -ETIME;
+ }
+
+ i915_request_put(rq);
+
+out_unpin_ce:
+ intel_context_unpin(ce);
+out_ww:
+ if (err == -EDEADLK) {
+ err = i915_gem_ww_ctx_backoff();
+ if (!err) {
+ if (++trials < 10)
+ goto retry;
+ else
+ err = EAGAIN;
+ }
+ }
+ i915_gem_ww_ctx_fini();
+
+ return err;
+}
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
index 3d56ae501991..3addce861854 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
@@ -8,7 +8,10 @@
#include
+struct i915_vma;
+struct intel_context;
struct intel_gsc_uc;
+
struct intel_gsc_mtl_header {
u32 validity_marker;
#define GSC_HECI_VALIDITY_MARKER 0xA578875A
@@ -47,7 +50,7 @@ struct intel_gsc_mtl_header {
* we distinguish the flags using OUTFLAG or INFLAG
*/
u32 flags;
-#define GSC_OUTFLAG_MSG_PENDING1
+#define GSC_OUTFLAG_MSG_PENDING 1
Nit: this change on the define is not really needed
sure - will fix.
Daniele
Re: [Intel-gfx] [PATCH v7 3/8] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC
On Mon, 2023-04-10 at 09:10 -0700, Ceraolo Spurio, Daniele wrote:
>
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 *cmd, int timeout_ms)
> > +{
> > + struct intel_engine_cs *eng;
>
> We always use "engine" for engine_cs variables. IMO it's better to stick
> to that here as well for consistency across the code.
alan: will do
>
> > + struct i915_gem_ww_ctx ww;
> > + struct i915_request *rq;
> > + int err, trials = 0;
> > +
>
> Is the assumption that the caller is holding a wakeref already?
> Otherwise we're going to need and engine_pm_get() here (assuming it
> doesn't interfere with any locking, otherwise it has to be in the caller)
alan: right now the only places this can get called from is via
intel_pxp_gsccs_create_session or
intel_pxp_gsccs_end_arb_fw_session. These functions are either being called by
intel_pxp_start
or intel_pxp_end. intel_pxp_start calls intel_runtime_pm_get_if_in_use
indirectly from the
session-worker and while intel_pxp_end takes an explicit intel_runtime_pm_get
(since it is
for suspend/shutdown cleanup and doesn't use the worker). I'm assuming
runtime_pm works right?
we have a similar logic across the paths from ADL version where we dont take
explicit
engine_pm_get for the termination via VDBOX because its part of the same code
paths.
alan:snip
> > + err = i915_vma_move_to_active(pkt->bb_vma, rq, EXEC_OBJECT_WRITE);
>
> nit: I don't think we need EXEC_OBJECT_WRITE for the bb as we're not
> going to write it.
alan: yes - will remove. (had accidentally kept above flag from offline
debugging version of the code that had additional store dwords into bb).
>
> > + if (err)
> > + goto out_rq;
> > + err = i915_vma_move_to_active(pkt->heci_pkt_vma, rq, EXEC_OBJECT_WRITE);
> > + if (err)
> > + goto out_rq;
> > +
> > + eng = rq->context->engine;
> > + if (eng->emit_init_breadcrumb) {
> > + err = eng->emit_init_breadcrumb(rq);
> > + if (err)
> > + goto out_rq;
> > + }
> > +
> > + err = eng->emit_bb_start(rq, i915_vma_offset(pkt->bb_vma), PAGE_SIZE,
> > 0);
> > + if (err)
> > + goto out_rq;
> > +
> > + err = ce->engine->emit_flush(rq, 0);
> > + if (err)
> > + drm_err(_uc_to_gt(gsc)->i915->drm,
> > + "Failed emit-flush for gsc-heci-non-priv-pkterr=%d\n",
> > err);
> > +
> > +out_rq:
> > + i915_request_get(rq);
> > +
> > + if (unlikely(err))
> > + i915_request_set_error_once(rq, err);
> > +
> > + i915_request_add(rq);
> > +
> > + if (!err) {
> > + if (i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE,
> > + msecs_to_jiffies(timeout_ms)) < 0)
> > + err = -ETIME;
> > + }
> > +
> > + i915_request_put(rq);
> > +
> > +out_unpin_ce:
> > + intel_context_unpin(ce);
> > +out_ww:
> > + if (err == -EDEADLK) {
> > + err = i915_gem_ww_ctx_backoff();
> > + if (!err) {
> > + if (++trials < 10)
> > + goto retry;
> > + else
> > + err = EAGAIN;
> > + }
> > + }
> > + i915_gem_ww_ctx_fini();
> > +
> > + return err;
> > +}
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> > b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> > index 3d56ae501991..3addce861854 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> > @@ -8,7 +8,10 @@
> >
> > #include
> >
> > +struct i915_vma;
> > +struct intel_context;
> > struct intel_gsc_uc;
> > +
> > struct intel_gsc_mtl_header {
> > u32 validity_marker;
> > #define GSC_HECI_VALIDITY_MARKER 0xA578875A
> > @@ -47,7 +50,7 @@ struct intel_gsc_mtl_header {
> > * we distinguish the flags using OUTFLAG or INFLAG
> > */
> > u32 flags;
> > -#define GSC_OUTFLAG_MSG_PENDING1
> > +#define GSC_OUTFLAG_MSG_PENDING 1
>
> Nit: this change on the define is not really needed
sure - will fix.
>
> Daniele
Re: [Intel-gfx] [PATCH v7 3/8] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC
On 4/6/2023 10:44 AM, Alan Previn wrote:
Add helper functions into a new file for heci-packet-submission.
The helpers will handle generating the MTL GSC-CS Memory-Header
and submission of the Heci-Cmd-Packet instructions to the engine.
NOTE1: These 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.
NOTE2: A difference in this patch vs what is appearing is in bullet 2
above is that HDCP (and SW-Proxy) will be using priveleged submission
(GGTT and common gsc-uc-context) while PXP will be using non-priveleged
PPGTT, context and batch buffer. Therefore this patch will only slightly
overlap with the MTL-HDCP patches despite have very similar function
names (emit_foo vs emit_nonpriv_foo). This is because HECI_CMD_PKT
instructions require different flows and hw-specific code when done
via PPGTT based submission (not different from other engines). MTL-HDCP
contains the same intel_gsc_mtl_header_t structures as this but the
helpers there are different. Both add the same new file names.
NOTE3: Additional clarity about the heci-cmd-pkt layout and where the
common helpers come in:
- On MTL, when an i915 subsystem needs to send a command request
to the security firmware, 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 GSC CS only looks at the GSC_HECI_CMD_PKT instruction and
passes it along to the GSC firmware.
- The GSC FW on the other hand needs additional metadata to know
which usage service is being called (PXP, HDCP, proxy, etc) along
with session specific info. Thus an extra header called GSC-CS
HECI Memory Header, (C) in below diagram is prepended before
the FW specific API, (D).
- Thus, the structural layout of the request submitted would
need to look like the diagram below (for non-priv PXP).
- In the diagram, the common helper for HDCP, (GSC-Sw-Proxy) and
PXP (i.e. new function intel_gsc_uc_heci_cmd_emit_mtl_header)
will populate blob (C) while additional helpers, different for
PPGGTT (this patch) vs GGTT (HDCP series) will populate
blobs (A) and (B) below.
___
(A) | MI_BATCH_BUFFER_START (ppgtt, batchbuff-addr, ...) |
| | |
|_| |
| (B)| GSC_HECI_CMD_PKT (pkt-addr-in, pkt-size-in,| |
|| pkt-addr-out, pkt-size-out) |
|| MI_BATCH_BUFFER_END| | |
||| | |
| | |
|_| |
|
-
|
\|/
__V___
| _|
|(C)| ||
| | struct intel_gsc_mtl_header { ||
| | validity marker ||
| | heci_clent_id ||
| | ... ||
| | }||
| |___||
|(D)| ||
| | struct gsc_fw_specific_api_foobar { ||
| | ... ||
| | For an example, see ||
| | 'struct pxp43_create_arb_in' at ||
| | intel_pxp_cmd_interface_43.h ||
| | ||
| | } ||
| | Struture depends on command type ||
| | struct gsc_fw_specific_api_foobar { ||
| |___||
||
That said, this patch provides basic helpers but leaves the
PXP subsystem (i.e. the caller) to handle (D) and everything
else such as input/output size verification or handling the
responses from security firmware (for example, requiring a retry).
Signed-off-by: Alan Previn
---
.../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c | 102
[Intel-gfx] [PATCH v7 3/8] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC
Add helper functions into a new file for heci-packet-submission.
The helpers will handle generating the MTL GSC-CS Memory-Header
and submission of the Heci-Cmd-Packet instructions to the engine.
NOTE1: These 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.
NOTE2: A difference in this patch vs what is appearing is in bullet 2
above is that HDCP (and SW-Proxy) will be using priveleged submission
(GGTT and common gsc-uc-context) while PXP will be using non-priveleged
PPGTT, context and batch buffer. Therefore this patch will only slightly
overlap with the MTL-HDCP patches despite have very similar function
names (emit_foo vs emit_nonpriv_foo). This is because HECI_CMD_PKT
instructions require different flows and hw-specific code when done
via PPGTT based submission (not different from other engines). MTL-HDCP
contains the same intel_gsc_mtl_header_t structures as this but the
helpers there are different. Both add the same new file names.
NOTE3: Additional clarity about the heci-cmd-pkt layout and where the
common helpers come in:
- On MTL, when an i915 subsystem needs to send a command request
to the security firmware, 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 GSC CS only looks at the GSC_HECI_CMD_PKT instruction and
passes it along to the GSC firmware.
- The GSC FW on the other hand needs additional metadata to know
which usage service is being called (PXP, HDCP, proxy, etc) along
with session specific info. Thus an extra header called GSC-CS
HECI Memory Header, (C) in below diagram is prepended before
the FW specific API, (D).
- Thus, the structural layout of the request submitted would
need to look like the diagram below (for non-priv PXP).
- In the diagram, the common helper for HDCP, (GSC-Sw-Proxy) and
PXP (i.e. new function intel_gsc_uc_heci_cmd_emit_mtl_header)
will populate blob (C) while additional helpers, different for
PPGGTT (this patch) vs GGTT (HDCP series) will populate
blobs (A) and (B) below.
___
(A) | MI_BATCH_BUFFER_START (ppgtt, batchbuff-addr, ...) |
| | |
|_| |
| (B)| GSC_HECI_CMD_PKT (pkt-addr-in, pkt-size-in,| |
|| pkt-addr-out, pkt-size-out) |
|| MI_BATCH_BUFFER_END| | |
||| | |
| | |
|_| |
|
-
|
\|/
__V___
| _|
|(C)| ||
| | struct intel_gsc_mtl_header { ||
| | validity marker ||
| | heci_clent_id ||
| | ... ||
| | }||
| |___||
|(D)| ||
| | struct gsc_fw_specific_api_foobar { ||
| | ... ||
| | For an example, see ||
| | 'struct pxp43_create_arb_in' at ||
| | intel_pxp_cmd_interface_43.h ||
| | ||
| | } ||
| | Struture depends on command type ||
| | struct gsc_fw_specific_api_foobar { ||
| |___||
||
That said, this patch provides basic helpers but leaves the
PXP subsystem (i.e. the caller) to handle (D) and everything
else such as input/output size verification or handling the
responses from security firmware (for example, requiring a retry).
Signed-off-by: Alan Previn
---
.../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c | 102 ++
.../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h | 25 -
2 files changed, 126
