* [PATCH v2 0/9] drm/i915/pxp: Add MTL PXP Support
@ 2023-01-11 21:42 Alan Previn
2023-01-11 21:42 ` [PATCH v2 1/9] drm/i915/pxp: Add MTL PXP GSC-CS back-end skeleton Alan Previn
` (8 more replies)
0 siblings, 9 replies; 27+ messages in thread
From: Alan Previn @ 2023-01-11 21:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Juston Li, Daniele Ceraolo Spurio, dri-devel, Alan Previn
This series enables PXP on MTL. On ADL/TGL platforms, we rely on
the mei driver via the i915-mei PXP component interface to establish
a connection to the security firmware via the HECI device interface.
That interface is used to create and teardown the PXP ARB session.
PXP ARB session is created when protected contexts are created.
In this series, the front end behaviors and interfaces (uapi) remain
the same. We add backend support for MTL but with MTL we directly use
the GSC-CS engine on the MTL GPU device to send messages to the PXP
(a.k.a. GSC a.k.a graphics-security) firmware. With MTL, the format
of the message is slightly different with a 2-layer packetization
that is explained in detail in Patch #4. Also, the second layer
which is the actual PXP firmware packet is now rev'd to version 4.3
for MTL that is defined in Patch #6.
Changes from prior revs:
v1 : - fixed when building with CONFIG_PXP disabled.
- more alignment with gsc_mtl_header structure from the HDCP
Alan Previn (9):
drm/i915/pxp: Add MTL PXP GSC-CS back-end skeleton
drm/i915/pxp: Add GSC-CS back-end resource init and cleanup
drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation
drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC
drm/i915/pxp: Add GSC-CS backend to send GSC fw messages
drm/i915/pxp: Add ARB session creation with new PXP API Ver4.3
drm/i915/pxp: MTL-KCR interrupt ctrl's are in GT-0
drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component
drm/i915/pxp: Enable PXP with MTL-GSC-CS
drivers/gpu/drm/i915/Makefile | 2 +
drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 2 +
drivers/gpu/drm/i915/gt/intel_gt_irq.c | 3 +-
.../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c | 107 +++++
.../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h | 69 ++++
drivers/gpu/drm/i915/i915_pci.c | 1 +
drivers/gpu/drm/i915/pxp/intel_pxp.c | 92 ++++-
drivers/gpu/drm/i915/pxp/intel_pxp.h | 4 +-
.../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 27 +-
drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 380 ++++++++++++++++++
drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h | 30 ++
drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 23 +-
drivers/gpu/drm/i915/pxp/intel_pxp_irq.h | 8 +
drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 10 +-
drivers/gpu/drm/i915/pxp/intel_pxp_regs.h | 26 ++
drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 37 +-
drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 13 +-
drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 11 +
19 files changed, 791 insertions(+), 56 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_regs.h
base-commit: bf7f7c53ac622a3f6d6738d062e59dd21ce28bd7
--
2.39.0
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 1/9] drm/i915/pxp: Add MTL PXP GSC-CS back-end skeleton
2023-01-11 21:42 [PATCH v2 0/9] drm/i915/pxp: Add MTL PXP Support Alan Previn
@ 2023-01-11 21:42 ` Alan Previn
2023-01-18 17:51 ` Ceraolo Spurio, Daniele
2023-01-11 21:42 ` [PATCH v2 2/9] drm/i915/pxp: Add GSC-CS back-end resource init and cleanup Alan Previn
` (7 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Alan Previn @ 2023-01-11 21:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Juston Li, Daniele Ceraolo Spurio, dri-devel, Alan Previn
Add MTL PXP GSC-CS back-end stub functions hook them
up from PXP front-end and PXP session management functions.
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/pxp/intel_pxp.c | 19 +++++++++++--
drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 23 +++++++++++++++
drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h | 30 ++++++++++++++++++++
drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 6 +++-
drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 6 ++++
6 files changed, 81 insertions(+), 4 deletions(-)
create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index f47f00b162a4..eae4325310e8 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -330,6 +330,7 @@ i915-y += \
i915-$(CONFIG_DRM_I915_PXP) += \
pxp/intel_pxp_cmd.o \
pxp/intel_pxp_debugfs.o \
+ pxp/intel_pxp_gsccs.o \
pxp/intel_pxp_irq.o \
pxp/intel_pxp_pm.o \
pxp/intel_pxp_session.o
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index cfc9af8b3d21..be52bf92e847 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -12,6 +12,7 @@
#include "i915_drv.h"
#include "intel_pxp.h"
+#include "intel_pxp_gsccs.h"
#include "intel_pxp_irq.h"
#include "intel_pxp_session.h"
#include "intel_pxp_tee.h"
@@ -132,7 +133,10 @@ static void pxp_init_full(struct intel_pxp *pxp)
if (ret)
return;
- ret = intel_pxp_tee_component_init(pxp);
+ if (pxp->uses_gsccs)
+ ret = intel_pxp_gsccs_init(pxp);
+ else
+ ret = intel_pxp_tee_component_init(pxp);
if (ret)
goto out_context;
@@ -157,6 +161,11 @@ static struct intel_gt *find_gt_for_required_teelink(struct drm_i915_private *i9
return NULL;
}
+static bool pxp_has_gsccs(struct drm_i915_private *i915)
+{
+ return (i915->media_gt && HAS_ENGINE(i915->media_gt, GSC0));
+}
+
static struct intel_gt *find_gt_for_required_protected_content(struct drm_i915_private *i915)
{
if (!IS_ENABLED(CONFIG_DRM_I915_PXP) || !INTEL_INFO(i915)->has_pxp)
@@ -167,7 +176,7 @@ static struct intel_gt *find_gt_for_required_protected_content(struct drm_i915_p
* on the media GT. NOTE: if we have a media-tile with a GSC-engine,
* the VDBOX is already present so skip that check
*/
- if (i915->media_gt && HAS_ENGINE(i915->media_gt, GSC0))
+ if (pxp_has_gsccs(i915))
return i915->media_gt;
/*
@@ -208,6 +217,7 @@ int intel_pxp_init(struct drm_i915_private *i915)
return -ENOMEM;
i915->pxp->ctrl_gt = gt;
+ i915->pxp->uses_gsccs = pxp_has_gsccs(i915);
/*
* If full PXP feature is not available but HuC is loaded by GSC on pre-MTL
@@ -229,7 +239,10 @@ void intel_pxp_fini(struct drm_i915_private *i915)
i915->pxp->arb_is_valid = false;
- intel_pxp_tee_component_fini(i915->pxp);
+ if (i915->pxp->uses_gsccs)
+ intel_pxp_gsccs_fini(i915->pxp);
+ else
+ intel_pxp_tee_component_fini(i915->pxp);
destroy_vcs_context(i915->pxp);
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
new file mode 100644
index 000000000000..21400650fc86
--- /dev/null
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright(c) 2023 Intel Corporation.
+ */
+
+#include "i915_drv.h"
+#include "intel_pxp_types.h"
+#include "intel_pxp_gsccs.h"
+
+int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
+ int arb_session_id)
+{
+ return -ENODEV;
+}
+
+void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
+{
+}
+
+int intel_pxp_gsccs_init(struct intel_pxp *pxp)
+{
+ return 0;
+}
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
new file mode 100644
index 000000000000..07f8c9b6f636
--- /dev/null
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright(c) 2022, Intel Corporation. All rights reserved.
+ */
+
+#ifndef __INTEL_PXP_GSCCS_H__
+#define __INTEL_PXP_GSCCS_H__
+
+#include <linux/types.h>
+
+struct intel_pxp;
+
+#ifdef CONFIG_DRM_I915_PXP
+int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
+ int arb_session_id);
+void intel_pxp_gsccs_fini(struct intel_pxp *pxp);
+int intel_pxp_gsccs_init(struct intel_pxp *pxp);
+
+#else
+static inline void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
+{
+}
+
+static inline int intel_pxp_gsccs_init(struct intel_pxp *pxp)
+{
+ return 0;
+}
+#endif
+
+#endif /*__INTEL_PXP_GSCCS_H__ */
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
index ae413580b81a..080aa2209c5b 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -7,6 +7,7 @@
#include "intel_pxp.h"
#include "intel_pxp_cmd.h"
+#include "intel_pxp_gsccs.h"
#include "intel_pxp_session.h"
#include "intel_pxp_tee.h"
#include "intel_pxp_types.h"
@@ -66,7 +67,10 @@ static int pxp_create_arb_session(struct intel_pxp *pxp)
return -EEXIST;
}
- ret = intel_pxp_tee_cmd_create_arb_session(pxp, ARB_SESSION);
+ if (pxp->uses_gsccs)
+ ret = intel_pxp_gsccs_create_session(pxp, ARB_SESSION);
+ else
+ ret = intel_pxp_tee_cmd_create_arb_session(pxp, ARB_SESSION);
if (ret) {
drm_err(>->i915->drm, "tee cmd for arb session creation failed\n");
return ret;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
index 7dc5f08d1583..43aa61c26de5 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
@@ -26,6 +26,12 @@ struct intel_pxp {
*/
struct intel_gt *ctrl_gt;
+ /**
+ * @uses_gsccs: PXP interface for firmware access and pxp-session controls is
+ * via the GSC-CS engine. This is for MTL+ platforms.
+ */
+ bool uses_gsccs;
+
/**
* @pxp_component: i915_pxp_component struct of the bound mei_pxp
* module. Only set and cleared inside component bind/unbind functions,
--
2.39.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 2/9] drm/i915/pxp: Add GSC-CS back-end resource init and cleanup
2023-01-11 21:42 [PATCH v2 0/9] drm/i915/pxp: Add MTL PXP Support Alan Previn
2023-01-11 21:42 ` [PATCH v2 1/9] drm/i915/pxp: Add MTL PXP GSC-CS back-end skeleton Alan Previn
@ 2023-01-11 21:42 ` Alan Previn
2023-01-18 23:55 ` Ceraolo Spurio, Daniele
2023-01-11 21:42 ` [PATCH v2 3/9] drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation Alan Previn
` (6 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Alan Previn @ 2023-01-11 21:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Juston Li, Daniele Ceraolo Spurio, dri-devel, Alan Previn
For MTL, PXP transport back-end uses the GSC engine to submit
HECI packets for PXP arb session management. The command submission
that uses non-priveleged mode requires us to allocate (or free)
a set of execution submission resources (buffer-object, batch-buffer
and context). Thus, do this one time allocation of resources in
GSC-CS init and clean them up in fini.
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
.../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 6 +-
drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 216 +++++++++++++++++-
drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 5 +
3 files changed, 225 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
index ad67e3f49c20..52b9a61bcdd4 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
@@ -10,7 +10,11 @@
#include "intel_pxp_cmd_interface_cmn.h"
/* PXP-Cmd-Op definitions */
-#define PXP43_CMDID_START_HUC_AUTH 0x0000003A
+#define PXP43_CMDID_START_HUC_AUTH 0x0000003A
+
+/* PXP-Packet sizes for MTL's GSCCS-HECI instruction */
+#define PXP43_MAX_HECI_IN_SIZE (SZ_32K)
+#define PXP43_MAX_HECI_OUT_SIZE (SZ_32K)
/* PXP-Input-Packet: HUC-Authentication */
struct pxp43_start_huc_auth_in {
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
index 21400650fc86..97ca187e6fde 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
@@ -3,9 +3,41 @@
* Copyright(c) 2023 Intel Corporation.
*/
+#include "gem/i915_gem_internal.h"
+
+#include "gt/intel_context.h"
+
#include "i915_drv.h"
-#include "intel_pxp_types.h"
+#include "intel_pxp_cmd_interface_43.h"
#include "intel_pxp_gsccs.h"
+#include "intel_pxp_types.h"
+
+struct gsccs_session_resources {
+ struct mutex cmd_mutex; /* Protects submission for arb session */
+ u64 host_session_handle; /* used by firmware to link commands to sessions */
+
+ struct intel_context *ce; /* context for gsc command submission */
+ struct i915_ppgtt *ppgtt; /* ppgtt for gsc command submission */
+
+ struct drm_i915_gem_object *pkt_obj; /* PXP HECI message packet buffer */
+ struct i915_vma *pkt_vma; /* PXP HECI message packet vma */
+ void *pkt_vaddr; /* PXP HECI message packet virt memory pointer */
+
+ /* Buffer info for GSC engine batch buffer: */
+ struct drm_i915_gem_object *bb_obj; /* batch buffer object */
+ struct i915_vma *bb_vma; /* batch buffer vma */
+ void *bb_vaddr; /* batch buffer virtual memory pointer */
+};
+
+struct gsccs_teelink_priv {
+ /** @arb_exec_res: resources for arb-session GSC-CS PXP command submission */
+ struct gsccs_session_resources arb_exec_res;
+};
+
+static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp)
+{
+ return (struct gsccs_teelink_priv *)pxp->gsccs_priv;
+}
int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
int arb_session_id)
@@ -13,11 +45,193 @@ int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
return -ENODEV;
}
+static void
+gsccs_destroy_buffer(struct drm_i915_private *i915, struct i915_vma *vma,
+ struct drm_i915_gem_object *obj)
+{
+ int err;
+
+ i915_vma_unpin(vma);
+ err = i915_vma_unbind(vma);
+ if (err)
+ drm_dbg(&i915->drm, "Unexpected failure when vma-unbinding = %d\n", err);
+
+ i915_gem_object_unpin_map(obj);
+ i915_gem_object_unpin_pages(obj);
+ i915_gem_object_put(obj);
+}
+
+static int
+gsccs_create_buffer(struct drm_i915_private *i915, const char *bufname,
+ size_t size, struct i915_ppgtt *ppgtt,
+ struct drm_i915_gem_object **obj,
+ struct i915_vma **vma, void **map)
+{
+ int err = 0;
+
+ *obj = i915_gem_object_create_internal(i915, size);
+ if (IS_ERR(*obj)) {
+ drm_err(&i915->drm, "Failed to allocate gsccs backend %s.\n", bufname);
+ err = PTR_ERR(*obj);
+ goto out_none;
+ }
+
+ *vma = i915_vma_instance(*obj, &ppgtt->vm, NULL);
+ if (IS_ERR(*vma)) {
+ drm_err(&i915->drm, "Failed to vma-instance gsccs backend %s.\n", bufname);
+ err = PTR_ERR(*vma);
+ goto out_put;
+ }
+
+ err = i915_gem_object_pin_pages_unlocked(*obj);
+ if (err) {
+ drm_err(&i915->drm, "Failed to pin gsccs backend %s.\n", bufname);
+ goto out_put;
+ }
+
+ /* map to the virtual memory pointer */
+ *map = i915_gem_object_pin_map_unlocked(*obj, i915_coherent_map_type(i915, *obj, true));
+ if (IS_ERR(*map)) {
+ drm_err(&i915->drm, "Failed to map gsccs backend %s.\n", bufname);
+ err = PTR_ERR(*map);
+ goto out_unpin;
+ }
+
+ /* all PXP sessions commands are treated as non-priveleged */
+ err = i915_vma_pin(*vma, 0, 0, PIN_USER);
+ if (err) {
+ drm_err(&i915->drm, "Failed to vma-pin gsccs backend %s.\n", bufname);
+ goto out_unmap;
+ }
+
+ return 0;
+
+out_unmap:
+ i915_gem_object_unpin_map(*obj);
+out_unpin:
+ i915_gem_object_unpin_pages(*obj);
+out_put:
+ i915_gem_object_put(*obj);
+out_none:
+ *obj = NULL;
+ *vma = NULL;
+ *map = NULL;
+
+ return err;
+}
+
+static void
+gsccs_destroy_execution_resource(struct intel_pxp *pxp,
+ struct gsccs_session_resources *strm_res)
+{
+ if (strm_res->ce)
+ intel_context_put(strm_res->ce);
+ if (strm_res->bb_obj)
+ gsccs_destroy_buffer(pxp->ctrl_gt->i915, strm_res->bb_vma, strm_res->bb_obj);
+ if (strm_res->pkt_obj)
+ gsccs_destroy_buffer(pxp->ctrl_gt->i915, strm_res->pkt_vma, strm_res->pkt_obj);
+ if (strm_res->ppgtt)
+ i915_vm_put(&strm_res->ppgtt->vm);
+
+ memset(strm_res, 0, sizeof(*strm_res));
+}
+
+static int
+gsccs_allocate_execution_resource(struct intel_pxp *pxp,
+ struct gsccs_session_resources *strm_res)
+{
+ struct intel_gt *gt = pxp->ctrl_gt;
+ struct intel_engine_cs *engine = gt->engine[GSC0];
+ struct i915_ppgtt *ppgtt;
+ struct intel_context *ce;
+ int err = 0;
+
+ /*
+ * First, ensure the GSC engine is present.
+ * NOTE: Backend should would only be called with the correct gt.
+ */
+ if (!engine)
+ return -ENODEV;
+
+ mutex_init(&strm_res->cmd_mutex);
+
+ ppgtt = i915_ppgtt_create(gt, 0);
+ if (IS_ERR(ppgtt))
+ return PTR_ERR(ppgtt);
+
+ strm_res->ppgtt = ppgtt;
+
+ /*
+ * Now, allocate, pin and map two objects, one for the heci message packet
+ * and another for the batch buffer we submit into GSC engine (that includes the packet).
+ * NOTE: GSC-CS backend is currently only supported on MTL, so we allocate shmem.
+ */
+ err = gsccs_create_buffer(pxp->ctrl_gt->i915, "Heci Packet",
+ PXP43_MAX_HECI_IN_SIZE + PXP43_MAX_HECI_OUT_SIZE,
+ strm_res->ppgtt,
+ &strm_res->pkt_obj, &strm_res->pkt_vma,
+ &strm_res->pkt_vaddr);
+ if (err) {
+ gsccs_destroy_execution_resource(pxp, strm_res);
+ return err;
+ }
+
+ err = gsccs_create_buffer(pxp->ctrl_gt->i915, "Batch Buffer",
+ PAGE_SIZE, strm_res->ppgtt,
+ &strm_res->bb_obj, &strm_res->bb_vma,
+ &strm_res->bb_vaddr);
+ if (err) {
+ gsccs_destroy_execution_resource(pxp, strm_res);
+ return err;
+ }
+ /*
+ * TODO: Consider optimization of pre-populating batch buffer
+ * with the send-HECI instruction now at init and reuse through its life.
+ */
+
+ /* Finally, create an intel_context to be used during the submission */
+ ce = intel_context_create(engine);
+ if (IS_ERR(ce)) {
+ drm_err(>->i915->drm, "Failed creating gsccs backend ctx\n");
+ gsccs_destroy_execution_resource(pxp, strm_res);
+ return PTR_ERR(ce);
+ }
+ i915_vm_put(ce->vm);
+ ce->vm = i915_vm_get(&ppgtt->vm);
+
+ strm_res->ce = ce;
+
+ return 0;
+}
+
void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
{
+ struct gsccs_teelink_priv *gsccs = pxp_to_gsccs_priv(pxp);
+
+ if (!gsccs)
+ return;
+
+ gsccs_destroy_execution_resource(pxp, &gsccs->arb_exec_res);
+ kfree(gsccs);
+ pxp->gsccs_priv = NULL;
}
int intel_pxp_gsccs_init(struct intel_pxp *pxp)
{
+ struct gsccs_teelink_priv *gsccs;
+ int ret;
+
+ gsccs = kzalloc(sizeof(*gsccs), GFP_KERNEL);
+ if (!gsccs)
+ return -ENOMEM;
+
+ ret = gsccs_allocate_execution_resource(pxp, &gsccs->arb_exec_res);
+ if (ret) {
+ kfree(gsccs);
+ return ret;
+ }
+
+ pxp->gsccs_priv = gsccs;
+
return 0;
}
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
index 43aa61c26de5..fdcb9a66f691 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
@@ -32,6 +32,11 @@ struct intel_pxp {
*/
bool uses_gsccs;
+ /**
+ * @gsccs_priv: GSC-CS based tee-link private context.
+ */
+ void *gsccs_priv;
+
/**
* @pxp_component: i915_pxp_component struct of the bound mei_pxp
* module. Only set and cleared inside component bind/unbind functions,
--
2.39.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 3/9] drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation
2023-01-11 21:42 [PATCH v2 0/9] drm/i915/pxp: Add MTL PXP Support Alan Previn
2023-01-11 21:42 ` [PATCH v2 1/9] drm/i915/pxp: Add MTL PXP GSC-CS back-end skeleton Alan Previn
2023-01-11 21:42 ` [PATCH v2 2/9] drm/i915/pxp: Add GSC-CS back-end resource init and cleanup Alan Previn
@ 2023-01-11 21:42 ` Alan Previn
2023-01-19 0:09 ` Ceraolo Spurio, Daniele
2023-01-11 21:42 ` [PATCH v2 4/9] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC Alan Previn
` (5 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Alan Previn @ 2023-01-11 21:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Juston Li, Daniele Ceraolo Spurio, dri-devel, Alan Previn
Add MTL hw-plumbing enabling for KCR operation under PXP
which includes:
1. Updating 'pick-gt' to get the media tile for
KCR interrupt handling
2. Adding MTL's KCR registers for PXP operation
(init, status-checking, etc.).
While doing #2, lets create a separate registers header file for PXP
to be consistent with other i915 global subsystems.
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
drivers/gpu/drm/i915/gt/intel_gt_irq.c | 3 +-
drivers/gpu/drm/i915/pxp/intel_pxp.c | 35 ++++++++++++--------
drivers/gpu/drm/i915/pxp/intel_pxp_regs.h | 26 +++++++++++++++
drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 29 +++++++++++-----
4 files changed, 70 insertions(+), 23 deletions(-)
create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_regs.h
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
index 8fac2660e16b..957fa11373fc 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
@@ -100,7 +100,8 @@ static struct intel_gt *pick_gt(struct intel_gt *gt, u8 class, u8 instance)
case VIDEO_ENHANCEMENT_CLASS:
return media_gt;
case OTHER_CLASS:
- if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, GSC0))
+ if ((instance == OTHER_GSC_INSTANCE || instance == OTHER_KCR_INSTANCE) &&
+ HAS_ENGINE(media_gt, GSC0))
return media_gt;
fallthrough;
default:
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index be52bf92e847..809b49f59594 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -14,6 +14,7 @@
#include "intel_pxp.h"
#include "intel_pxp_gsccs.h"
#include "intel_pxp_irq.h"
+#include "intel_pxp_regs.h"
#include "intel_pxp_session.h"
#include "intel_pxp_tee.h"
#include "intel_pxp_types.h"
@@ -61,21 +62,30 @@ bool intel_pxp_is_active(const struct intel_pxp *pxp)
return IS_ENABLED(CONFIG_DRM_I915_PXP) && pxp && pxp->arb_is_valid;
}
-/* KCR register definitions */
-#define KCR_INIT _MMIO(0x320f0)
-/* Setting KCR Init bit is required after system boot */
-#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES REG_BIT(14)
+static i915_reg_t get_kcr_reg(const struct intel_pxp *pxp)
+{
+ if (pxp->gsccs_priv)
+ return MTL_KCR_INIT;
+ return GEN12_KCR_INIT;
+}
-static void kcr_pxp_enable(struct intel_gt *gt)
+static void kcr_pxp_set_status(const struct intel_pxp *pxp, bool enable)
{
- intel_uncore_write(gt->uncore, KCR_INIT,
- _MASKED_BIT_ENABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES));
+ i915_reg_t reg = get_kcr_reg(pxp);
+ u32 val = enable ? _MASKED_BIT_ENABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES) :
+ _MASKED_BIT_DISABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES);
+
+ intel_uncore_write(pxp->ctrl_gt->uncore, reg, val);
}
-static void kcr_pxp_disable(struct intel_gt *gt)
+static void kcr_pxp_enable(const struct intel_pxp *pxp)
{
- intel_uncore_write(gt->uncore, KCR_INIT,
- _MASKED_BIT_DISABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES));
+ kcr_pxp_set_status(pxp, true);
+}
+
+static void kcr_pxp_disable(const struct intel_pxp *pxp)
+{
+ kcr_pxp_set_status(pxp, false);
}
static int create_vcs_context(struct intel_pxp *pxp)
@@ -323,14 +333,13 @@ int intel_pxp_start(struct intel_pxp *pxp)
void intel_pxp_init_hw(struct intel_pxp *pxp)
{
- kcr_pxp_enable(pxp->ctrl_gt);
+ kcr_pxp_enable(pxp);
intel_pxp_irq_enable(pxp);
}
void intel_pxp_fini_hw(struct intel_pxp *pxp)
{
- kcr_pxp_disable(pxp->ctrl_gt);
-
+ kcr_pxp_disable(pxp);
intel_pxp_irq_disable(pxp);
}
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_regs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_regs.h
new file mode 100644
index 000000000000..dd4131903d4e
--- /dev/null
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_regs.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright(c) 2023, Intel Corporation. All rights reserved.
+ */
+
+#ifndef __INTEL_PXP_REGS_H__
+#define __INTEL_PXP_REGS_H__
+
+#include "i915_reg_defs.h"
+
+/* KCR enable/disable control */
+#define GEN12_KCR_INIT _MMIO(0x320f0)
+#define MTL_KCR_INIT _MMIO(0x3860f0)
+
+/* Setting KCR Init bit is required after system boot */
+#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES REG_BIT(14)
+
+/* KCR hwdrm session in play status 0-31 */
+#define GEN12_KCR_SIP _MMIO(0x32260)
+#define MTL_KCR_SIP _MMIO(0x386260)
+
+/* PXP global terminate register for session termination */
+#define GEN12_KCR_GLOBAL_TERMINATE _MMIO(0x320f8)
+#define MTL_KCR_GLOBAL_TERMINATE _MMIO(0x3860f8)
+
+#endif /* __INTEL_PXP_REGS_H__ */
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
index 080aa2209c5b..7bb06e67b155 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -11,23 +11,25 @@
#include "intel_pxp_session.h"
#include "intel_pxp_tee.h"
#include "intel_pxp_types.h"
+#include "intel_pxp_regs.h"
#define ARB_SESSION I915_PROTECTED_CONTENT_DEFAULT_SESSION /* shorter define */
-#define GEN12_KCR_SIP _MMIO(0x32260) /* KCR hwdrm session in play 0-31 */
-
-/* PXP global terminate register for session termination */
-#define PXP_GLOBAL_TERMINATE _MMIO(0x320f8)
-
static bool intel_pxp_session_is_in_play(struct intel_pxp *pxp, u32 id)
{
struct intel_uncore *uncore = pxp->ctrl_gt->uncore;
intel_wakeref_t wakeref;
+ i915_reg_t reg;
u32 sip = 0;
/* if we're suspended the session is considered off */
- with_intel_runtime_pm_if_in_use(uncore->rpm, wakeref)
- sip = intel_uncore_read(uncore, GEN12_KCR_SIP);
+ with_intel_runtime_pm_if_in_use(uncore->rpm, wakeref) {
+ if (pxp->gsccs_priv)
+ reg = MTL_KCR_SIP;
+ else
+ reg = GEN12_KCR_SIP;
+ sip = intel_uncore_read(uncore, reg);
+ }
return sip & BIT(id);
}
@@ -37,6 +39,7 @@ static int pxp_wait_for_session_state(struct intel_pxp *pxp, u32 id, bool in_pla
struct intel_uncore *uncore = pxp->ctrl_gt->uncore;
intel_wakeref_t wakeref;
u32 mask = BIT(id);
+ i915_reg_t reg;
int ret;
/* if we're suspended the session is considered off */
@@ -44,8 +47,13 @@ static int pxp_wait_for_session_state(struct intel_pxp *pxp, u32 id, bool in_pla
if (!wakeref)
return in_play ? -ENODEV : 0;
+ if (pxp->gsccs_priv)
+ reg = MTL_KCR_SIP;
+ else
+ reg = GEN12_KCR_SIP;
+
ret = intel_wait_for_register(uncore,
- GEN12_KCR_SIP,
+ reg,
mask,
in_play ? mask : 0,
100);
@@ -112,7 +120,10 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
return ret;
}
- intel_uncore_write(gt->uncore, PXP_GLOBAL_TERMINATE, 1);
+ if (pxp->gsccs_priv)
+ intel_uncore_write(gt->uncore, MTL_KCR_GLOBAL_TERMINATE, 1);
+ else
+ intel_uncore_write(gt->uncore, GEN12_KCR_GLOBAL_TERMINATE, 1);
return ret;
}
--
2.39.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 4/9] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC
2023-01-11 21:42 [PATCH v2 0/9] drm/i915/pxp: Add MTL PXP Support Alan Previn
` (2 preceding siblings ...)
2023-01-11 21:42 ` [PATCH v2 3/9] drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation Alan Previn
@ 2023-01-11 21:42 ` Alan Previn
2023-01-19 1:28 ` Ceraolo Spurio, Daniele
2023-01-11 21:42 ` [PATCH v2 5/9] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages Alan Previn
` (4 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Alan Previn @ 2023-01-11 21:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Juston Li, Daniele Ceraolo Spurio, dri-devel, Alan Previn
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.
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 very 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:
- 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).
- 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
GGTT (not in this series) vs PPGTT (this patch) 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 everything else from
input/output packet size verification to handling the
responses from security firmware (such as requiring a retry).
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 2 +
.../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c | 107 ++++++++++++++++++
.../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h | 69 +++++++++++
4 files changed, 179 insertions(+)
create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index eae4325310e8..7dc18554da10 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -194,6 +194,7 @@ i915-y += \
i915-y += \
gt/uc/intel_gsc_fw.o \
gt/uc/intel_gsc_uc.o \
+ gt/uc/intel_gsc_uc_heci_cmd_submit.o \
gt/uc/intel_guc.o \
gt/uc/intel_guc_ads.o \
gt/uc/intel_guc_capture.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
index 2af1ae3831df..454179884801 100644
--- a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
+++ b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
@@ -439,6 +439,8 @@
#define GSC_FW_LOAD GSC_INSTR(1, 0, 2)
#define HECI1_FW_LIMIT_VALID (1 << 31)
+#define GSC_HECI_CMD_PKT GSC_INSTR(0, 0, 6)
+
/*
* Used to convert any address to canonical form.
* Starting from gen8, some commands (e.g. STATE_BASE_ADDRESS,
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
new file mode 100644
index 000000000000..01bb7e587b1b
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include <linux/types.h>
+
+#include "i915_drv.h"
+#include "i915_vma.h"
+
+#include "gt/intel_gpu_commands.h"
+#include "gt/intel_context.h"
+#include "gt/intel_gt.h"
+#include "gt/intel_ring.h"
+
+#include "intel_gsc_fw.h"
+#include "intel_gsc_uc.h"
+#include "intel_gsc_uc_heci_cmd_submit.h"
+
+void intel_gsc_uc_heci_cmd_emit_mtl_header(struct intel_gsc_mtl_header *header,
+ u8 heci_client_id, u32 message_size,
+ u64 host_session_id)
+{
+ host_session_id &= ~GSC_HECI_HOST_SESSION_USAGE_MASK;
+ if (heci_client_id == GSC_HECI_MEADDRESS_PXP)
+ host_session_id |= GSC_HECI_SESSION_PXP_SINGLE;
+
+ header->validity_marker = GSC_HECI_VALIDITY_MARKER;
+ header->heci_client_id = heci_client_id;
+ header->host_session_handle = host_session_id;
+ header->header_version = MTL_GSC_HECI_HEADER_VERSION;
+ header->message_size = message_size;
+}
+
+static void
+emit_gsc_heci_pkt_nonpriv(u32 *cs, struct intel_gsc_heci_non_priv_pkt *pkt)
+{
+ *cs++ = GSC_HECI_CMD_PKT;
+ *cs++ = lower_32_bits(pkt->addr_in);
+ *cs++ = upper_32_bits(pkt->addr_in);
+ *cs++ = pkt->size_in;
+ *cs++ = lower_32_bits(pkt->addr_out);
+ *cs++ = upper_32_bits(pkt->addr_out);
+ *cs++ = pkt->size_out;
+ *cs++ = 0;
+ *cs++ = MI_BATCH_BUFFER_END;
+}
+
+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)
+{
+ 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);
+ 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);
+
+ 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
new file mode 100644
index 000000000000..23155bed3fee
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef _INTEL_GSC_UC_HECI_CMD_H_
+#define _INTEL_GSC_UC_HECI_CMD_H_
+
+#include <linux/types.h>
+
+struct i915_vma;
+struct intel_context;
+struct intel_gsc_uc;
+
+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)
+
+ u64 gsc_message_handle;
+
+ u32 message_size; /* lower 20 bits only, upper 12 are reserved */
+
+ /*
+ * Flags mask:
+ * Bit 0: Pending
+ * Bit 1: Session Cleanup;
+ * Bits 2-15: Flags
+ * Bits 16-31: Extension Size
+ */
+ u32 flags;
+#define GSC_HECI_FLAG_MSG_PENDING BIT(0)
+#define GSC_HECI_FLAG_MSG_CLEANUP BIT(1)
+
+ u32 status;
+} __packed;
+
+struct intel_gsc_heci_non_priv_pkt {
+ u64 addr_in;
+ u32 size_in;
+ u64 addr_out;
+ u32 size_out;
+ struct i915_vma *heci_pkt_vma;
+ struct i915_vma *bb_vma;
+};
+
+void
+intel_gsc_uc_heci_cmd_emit_mtl_header(struct intel_gsc_mtl_header *header,
+ u8 heci_client_id, u32 msg_size,
+ u64 host_session_id);
+
+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);
+#endif
--
2.39.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 5/9] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages
2023-01-11 21:42 [PATCH v2 0/9] drm/i915/pxp: Add MTL PXP Support Alan Previn
` (3 preceding siblings ...)
2023-01-11 21:42 ` [PATCH v2 4/9] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC Alan Previn
@ 2023-01-11 21:42 ` Alan Previn
2023-01-19 1:40 ` Ceraolo Spurio, Daniele
2023-01-11 21:42 ` [PATCH v2 6/9] drm/i915/pxp: Add ARB session creation with new PXP API Ver4.3 Alan Previn
` (3 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Alan Previn @ 2023-01-11 21:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Juston Li, Daniele Ceraolo Spurio, dri-devel, Alan Previn
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@intel.com>
---
drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 93 ++++++++++++++++++++++
1 file changed, 93 insertions(+)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
index 97ca187e6fde..ff235822743e 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
@@ -6,6 +6,7 @@
#include "gem/i915_gem_internal.h"
#include "gt/intel_context.h"
+#include "gt/uc/intel_gsc_uc_heci_cmd_submit.h"
#include "i915_drv.h"
#include "intel_pxp_cmd_interface_43.h"
@@ -39,6 +40,98 @@ static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp
return (struct gsccs_teelink_priv *)pxp->gsccs_priv;
}
+static int gsccs_send_message(struct intel_pxp *pxp,
+ 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;
+
+ 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);
+
+ 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)
{
--
2.39.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 6/9] drm/i915/pxp: Add ARB session creation with new PXP API Ver4.3
2023-01-11 21:42 [PATCH v2 0/9] drm/i915/pxp: Add MTL PXP Support Alan Previn
` (4 preceding siblings ...)
2023-01-11 21:42 ` [PATCH v2 5/9] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages Alan Previn
@ 2023-01-11 21:42 ` Alan Previn
2023-01-19 1:50 ` Ceraolo Spurio, Daniele
2023-01-11 21:42 ` [PATCH v2 7/9] drm/i915/pxp: MTL-KCR interrupt ctrl's are in GT-0 Alan Previn
` (2 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Alan Previn @ 2023-01-11 21:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Juston Li, Daniele Ceraolo Spurio, dri-devel, Alan Previn
Add MTL's function for ARB session creation using PXP firmware
version 4.3 ABI structure format.
Before checking the return status, look at the GSC-CS-Mem-Header's
pending-bit which means the GSC firmware is busy and we should
resubmit.
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
.../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 21 +++++++
drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 56 ++++++++++++++++++-
2 files changed, 74 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
index 52b9a61bcdd4..ee78c0817ba1 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
@@ -11,6 +11,7 @@
/* PXP-Cmd-Op definitions */
#define PXP43_CMDID_START_HUC_AUTH 0x0000003A
+#define PXP43_CMDID_INIT_SESSION 0x00000036
/* PXP-Packet sizes for MTL's GSCCS-HECI instruction */
#define PXP43_MAX_HECI_IN_SIZE (SZ_32K)
@@ -27,4 +28,24 @@ struct pxp43_start_huc_auth_out {
struct pxp_cmd_header header;
} __packed;
+/* PXP-Input-Packet: Init PXP session */
+struct pxp43_create_arb_in {
+ struct pxp_cmd_header header;
+ /* header.stream_id fields for vesion 4.3 of Init PXP session: */
+ #define PXP43_INIT_SESSION_VALID GENMASK(0, 0)
+ #define PXP43_INIT_SESSION_APPTYPE GENMASK(1, 1)
+ #define PXP43_INIT_SESSION_APPID GENMASK(17, 2)
+ u32 protection_mode;
+ #define PXP43_INIT_SESSION_PROTECTION_ARB 0x2
+ u32 sub_session_id;
+ u32 init_flags;
+ u32 rsvd[12];
+} __packed;
+
+/* PXP-Input-Packet: Init PXP session */
+struct pxp43_create_arb_out {
+ struct pxp_cmd_header header;
+ u32 rsvd[8];
+} __packed;
+
#endif /* __INTEL_PXP_FW_INTERFACE_43_H__ */
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
index ff235822743e..1b06629ac16e 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
@@ -43,7 +43,8 @@ static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp
static int gsccs_send_message(struct intel_pxp *pxp,
void *msg_in, size_t msg_in_size,
void *msg_out, size_t msg_out_size_max,
- size_t *msg_out_len)
+ size_t *msg_out_len,
+ u64 *gsc_msg_handle_retry)
{
struct intel_gt *gt = pxp->ctrl_gt;
struct drm_i915_private *i915 = gt->i915;
@@ -75,6 +76,9 @@ static int gsccs_send_message(struct intel_pxp *pxp,
msg_in_size + sizeof(*header),
exec->host_session_handle);
+ /* copy caller provided gsc message handle if this is polling for a prior msg completion */
+ header->gsc_message_handle = *gsc_msg_handle_retry;
+
memcpy(exec->pkt_vaddr + sizeof(*header), msg_in, msg_in_size);
pkt.addr_in = i915_vma_offset(exec->pkt_vma);
@@ -91,7 +95,7 @@ static int gsccs_send_message(struct intel_pxp *pxp,
goto unlock;
}
- /* we keep separate location for reply, so get the response header loc first */
+ /* we keep separate location for reply, so go to the response header now */
header = exec->pkt_vaddr + PXP43_MAX_HECI_IN_SIZE;
/* Response validity marker, status and busyness */
@@ -108,6 +112,13 @@ static int gsccs_send_message(struct intel_pxp *pxp,
}
if (header->flags & GSC_HECI_FLAG_MSG_PENDING) {
drm_dbg(&i915->drm, "gsc PXP reply is busy\n");
+ /*
+ * When the GSC firmware replies with pending bit, it means that the requested
+ * operation has begun but the completion is pending and the caller needs
+ * to re-request with the gsc_message_handle that was returned by the firmware.
+ * until the pending bit is turned off.
+ */
+ *gsc_msg_handle_retry = header->gsc_message_handle;
ret = -EAGAIN;
goto unlock;
}
@@ -135,7 +146,46 @@ static int gsccs_send_message(struct intel_pxp *pxp,
int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
int arb_session_id)
{
- return -ENODEV;
+ struct gsccs_session_resources *exec = &pxp_to_gsccs_priv(pxp)->arb_exec_res;
+ struct pxp43_create_arb_in msg_in = {0};
+ struct pxp43_create_arb_out msg_out = {0};
+ u64 gsc_session_retry = 0;
+ int insize, outsize, ret, tries = 0;
+ void *inptr, *outptr;
+
+ /* get a unique host-session-handle (used later in HW cmds) at time of session creation */
+ get_random_bytes(&exec->host_session_handle, sizeof(exec->host_session_handle));
+
+ msg_in.header.api_version = PXP_APIVER(4, 3);
+ msg_in.header.command_id = PXP43_CMDID_INIT_SESSION;
+ msg_in.header.stream_id = (FIELD_PREP(PXP43_INIT_SESSION_APPID, arb_session_id) |
+ FIELD_PREP(PXP43_INIT_SESSION_VALID, 1) |
+ FIELD_PREP(PXP43_INIT_SESSION_APPTYPE, 0));
+ msg_in.header.buffer_len = sizeof(msg_in) - sizeof(msg_in.header);
+ msg_in.protection_mode = PXP43_INIT_SESSION_PROTECTION_ARB;
+
+ inptr = &msg_in;
+ outptr = &msg_out;
+ insize = sizeof(msg_in);
+ outsize = sizeof(msg_out);
+
+ /*
+ * Keep sending request if GSC firmware was busy.
+ * Based on test data, we expects a worst case delay of 250 milisecs.
+ */
+ do {
+ ret = gsccs_send_message(pxp,
+ inptr, insize,
+ outptr, outsize, NULL,
+ &gsc_session_retry);
+ /* Only try again if gsc says so */
+ if (ret != -EAGAIN)
+ break;
+
+ msleep(20);
+ } while (++tries < 12);
+
+ return ret;
}
static void
--
2.39.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 7/9] drm/i915/pxp: MTL-KCR interrupt ctrl's are in GT-0
2023-01-11 21:42 [PATCH v2 0/9] drm/i915/pxp: Add MTL PXP Support Alan Previn
` (5 preceding siblings ...)
2023-01-11 21:42 ` [PATCH v2 6/9] drm/i915/pxp: Add ARB session creation with new PXP API Ver4.3 Alan Previn
@ 2023-01-11 21:42 ` Alan Previn
2023-01-19 2:01 ` Ceraolo Spurio, Daniele
2023-01-11 21:42 ` [PATCH v2 8/9] drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component Alan Previn
2023-01-11 21:42 ` [PATCH v2 9/9] drm/i915/pxp: Enable PXP with MTL-GSC-CS Alan Previn
8 siblings, 1 reply; 27+ messages in thread
From: Alan Previn @ 2023-01-11 21:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Juston Li, Daniele Ceraolo Spurio, dri-devel, Alan Previn
Despite KCR subsystem being in the media-tile (close to the
GSC-CS), the IRQ controls for it are on GT-0 with other global
IRQ controls. Thus, add a helper for KCR hw interrupt
enable/disable functions to get the correct gt structure (for
uncore) for MTL.
In the helper, we get GT-0's handle for uncore when touching
IRQ registers despite the pxp->ctrl_gt being the media-tile.
No difference for legacy of course.
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 23 +++++++++++++++++---
drivers/gpu/drm/i915/pxp/intel_pxp_irq.h | 8 +++++++
3 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
index 4b8e70caa3ad..9f6e300486b4 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
@@ -44,7 +44,7 @@ static int pxp_terminate_get(void *data, u64 *val)
static int pxp_terminate_set(void *data, u64 val)
{
struct intel_pxp *pxp = data;
- struct intel_gt *gt = pxp->ctrl_gt;
+ struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
if (!intel_pxp_is_active(pxp))
return -ENODEV;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
index 91e9622c07d0..2eef0c19e91a 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
@@ -8,6 +8,7 @@
#include "gt/intel_gt_regs.h"
#include "gt/intel_gt_types.h"
+#include "i915_drv.h"
#include "i915_irq.h"
#include "i915_reg.h"
@@ -17,6 +18,22 @@
#include "intel_pxp_types.h"
#include "intel_runtime_pm.h"
+/**
+ * intel_pxp_get_irq_gt - Find the correct GT that owns KCR interrupts
+ * @pxp: pointer to pxp struct
+ *
+ * For platforms with a single GT, we return the pxp->ctrl_gt (as expected)
+ * but for MTL+ that has a media-tile, although the KCR engine is in the
+ * media-tile (i.e. pxp->ctrl_gt), the IRQ controls are on the root tile.
+ */
+struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp)
+{
+ if (pxp->uses_gsccs)
+ return to_gt(pxp->ctrl_gt->i915);
+
+ return pxp->ctrl_gt;
+}
+
/**
* intel_pxp_irq_handler - Handles PXP interrupts.
* @pxp: pointer to pxp struct
@@ -29,7 +46,7 @@ void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
if (GEM_WARN_ON(!intel_pxp_is_enabled(pxp)))
return;
- gt = pxp->ctrl_gt;
+ gt = intel_pxp_get_irq_gt(pxp);
lockdep_assert_held(gt->irq_lock);
@@ -68,7 +85,7 @@ static inline void pxp_irq_reset(struct intel_gt *gt)
void intel_pxp_irq_enable(struct intel_pxp *pxp)
{
- struct intel_gt *gt = pxp->ctrl_gt;
+ struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
spin_lock_irq(gt->irq_lock);
@@ -83,7 +100,7 @@ void intel_pxp_irq_enable(struct intel_pxp *pxp)
void intel_pxp_irq_disable(struct intel_pxp *pxp)
{
- struct intel_gt *gt = pxp->ctrl_gt;
+ struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
/*
* We always need to submit a global termination when we re-enable the
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
index 8c292dc86f68..eea87c9eb62b 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
@@ -9,6 +9,7 @@
#include <linux/types.h>
struct intel_pxp;
+struct intel_gt;
#define GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT BIT(1)
#define GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT BIT(2)
@@ -23,6 +24,8 @@ struct intel_pxp;
void intel_pxp_irq_enable(struct intel_pxp *pxp);
void intel_pxp_irq_disable(struct intel_pxp *pxp);
void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir);
+struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp);
+
#else
static inline void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
{
@@ -35,6 +38,11 @@ static inline void intel_pxp_irq_enable(struct intel_pxp *pxp)
static inline void intel_pxp_irq_disable(struct intel_pxp *pxp)
{
}
+
+static inline struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp)
+{
+ return NULL;
+}
#endif
#endif /* __INTEL_PXP_IRQ_H__ */
--
2.39.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 8/9] drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component
2023-01-11 21:42 [PATCH v2 0/9] drm/i915/pxp: Add MTL PXP Support Alan Previn
` (6 preceding siblings ...)
2023-01-11 21:42 ` [PATCH v2 7/9] drm/i915/pxp: MTL-KCR interrupt ctrl's are in GT-0 Alan Previn
@ 2023-01-11 21:42 ` Alan Previn
2023-01-19 2:17 ` Ceraolo Spurio, Daniele
2023-01-11 21:42 ` [PATCH v2 9/9] drm/i915/pxp: Enable PXP with MTL-GSC-CS Alan Previn
8 siblings, 1 reply; 27+ messages in thread
From: Alan Previn @ 2023-01-11 21:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Juston Li, Daniele Ceraolo Spurio, dri-devel, Alan Previn
On legacy platforms, KCR HW enabling is done at the time the mei
component interface is bound. It's also disabled during unbind.
However, for MTL onwards, we don't depend on the tee-component
operation before we can start sending GSC-CS firmware messages.
Thus, immediately enable KCR HW on PXP's init, fini and resume.
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
drivers/gpu/drm/i915/pxp/intel_pxp.c | 52 ++++++++++++++++++------
drivers/gpu/drm/i915/pxp/intel_pxp.h | 4 +-
drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 10 ++---
drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 13 +-----
4 files changed, 49 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 809b49f59594..90e739345924 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -143,10 +143,12 @@ static void pxp_init_full(struct intel_pxp *pxp)
if (ret)
return;
- if (pxp->uses_gsccs)
+ if (pxp->uses_gsccs) {
ret = intel_pxp_gsccs_init(pxp);
- else
+ intel_pxp_init_hw(pxp, true);
+ } else {
ret = intel_pxp_tee_component_init(pxp);
+ }
if (ret)
goto out_context;
@@ -249,10 +251,12 @@ void intel_pxp_fini(struct drm_i915_private *i915)
i915->pxp->arb_is_valid = false;
- if (i915->pxp->uses_gsccs)
+ if (i915->pxp->uses_gsccs) {
+ intel_pxp_fini_hw(i915->pxp, true);
intel_pxp_gsccs_fini(i915->pxp);
- else
+ } else {
intel_pxp_tee_component_fini(i915->pxp);
+ }
destroy_vcs_context(i915->pxp);
@@ -304,8 +308,9 @@ int intel_pxp_start(struct intel_pxp *pxp)
if (!intel_pxp_is_enabled(pxp))
return -ENODEV;
- if (wait_for(pxp_component_bound(pxp), 250))
- return -ENXIO;
+ if (!pxp->uses_gsccs)
+ if (wait_for(pxp_component_bound(pxp), 250))
+ return -ENXIO;
mutex_lock(&pxp->arb_mutex);
@@ -331,16 +336,39 @@ int intel_pxp_start(struct intel_pxp *pxp)
return ret;
}
-void intel_pxp_init_hw(struct intel_pxp *pxp)
+static void
+intel_pxp_hw_state_change(struct intel_pxp *pxp, bool enable,
+ bool skip_if_runtime_pm_off)
+{
+ intel_wakeref_t wakeref;
+
+ if (skip_if_runtime_pm_off) {
+ /* if we are suspended, the HW will be re-initialized on resume */
+ wakeref = intel_runtime_pm_get_if_in_use(&pxp->ctrl_gt->i915->runtime_pm);
+ if (!wakeref)
+ return;
+ }
+
+ if (enable) {
+ kcr_pxp_enable(pxp);
+ intel_pxp_irq_enable(pxp);
+ } else {
+ kcr_pxp_disable(pxp);
+ intel_pxp_irq_disable(pxp);
+ }
+
+ if (skip_if_runtime_pm_off)
+ intel_runtime_pm_put(&pxp->ctrl_gt->i915->runtime_pm, wakeref);
+}
+
+void intel_pxp_init_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off)
{
- kcr_pxp_enable(pxp);
- intel_pxp_irq_enable(pxp);
+ intel_pxp_hw_state_change(pxp, true, skip_if_runtime_pm_off);
}
-void intel_pxp_fini_hw(struct intel_pxp *pxp)
+void intel_pxp_fini_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off)
{
- kcr_pxp_disable(pxp);
- intel_pxp_irq_disable(pxp);
+ intel_pxp_hw_state_change(pxp, false, skip_if_runtime_pm_off);
}
int intel_pxp_key_check(struct intel_pxp *pxp,
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 04440fada711..6c1fe3f0a20c 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -20,8 +20,8 @@ bool intel_pxp_is_active(const struct intel_pxp *pxp);
int intel_pxp_init(struct drm_i915_private *i915);
void intel_pxp_fini(struct drm_i915_private *i915);
-void intel_pxp_init_hw(struct intel_pxp *pxp);
-void intel_pxp_fini_hw(struct intel_pxp *pxp);
+void intel_pxp_init_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off);
+void intel_pxp_fini_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off);
void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
index 892d39cc61c1..94c1b2fe1eb2 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
@@ -29,7 +29,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp)
return;
with_intel_runtime_pm(&pxp->ctrl_gt->i915->runtime_pm, wakeref) {
- intel_pxp_fini_hw(pxp);
+ intel_pxp_fini_hw(pxp, false);
pxp->hw_state_invalidated = false;
}
}
@@ -40,14 +40,14 @@ void intel_pxp_resume(struct intel_pxp *pxp)
return;
/*
- * The PXP component gets automatically unbound when we go into S3 and
+ * On Pre-MTL, PXP component gets automatically unbound when we go into S3 and
* re-bound after we come out, so in that scenario we can defer the
* hw init to the bind call.
*/
- if (!pxp->pxp_component)
+ if (!pxp->uses_gsccs & !pxp->pxp_component)
return;
- intel_pxp_init_hw(pxp);
+ intel_pxp_init_hw(pxp, false);
}
void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
@@ -57,7 +57,7 @@ void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
pxp->arb_is_valid = false;
- intel_pxp_fini_hw(pxp);
+ intel_pxp_fini_hw(pxp, false);
pxp->hw_state_invalidated = false;
}
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
index d50354bfb993..9b34f2056b19 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
@@ -141,16 +141,9 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
}
}
- /* if we are suspended, the HW will be re-initialized on resume */
- wakeref = intel_runtime_pm_get_if_in_use(&i915->runtime_pm);
- if (!wakeref)
- return 0;
-
/* the component is required to fully start the PXP HW */
if (intel_pxp_is_enabled(pxp))
- intel_pxp_init_hw(pxp);
-
- intel_runtime_pm_put(&i915->runtime_pm, wakeref);
+ intel_pxp_init_hw(pxp, true);
return ret;
}
@@ -160,11 +153,9 @@ static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
{
struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
struct intel_pxp *pxp = i915->pxp;
- intel_wakeref_t wakeref;
if (intel_pxp_is_enabled(pxp))
- with_intel_runtime_pm_if_in_use(&i915->runtime_pm, wakeref)
- intel_pxp_fini_hw(pxp);
+ intel_pxp_fini_hw(pxp, true);
mutex_lock(&pxp->tee_mutex);
pxp->pxp_component = NULL;
--
2.39.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 9/9] drm/i915/pxp: Enable PXP with MTL-GSC-CS
2023-01-11 21:42 [PATCH v2 0/9] drm/i915/pxp: Add MTL PXP Support Alan Previn
` (7 preceding siblings ...)
2023-01-11 21:42 ` [PATCH v2 8/9] drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component Alan Previn
@ 2023-01-11 21:42 ` Alan Previn
8 siblings, 0 replies; 27+ messages in thread
From: Alan Previn @ 2023-01-11 21:42 UTC (permalink / raw)
To: intel-gfx; +Cc: Juston Li, Daniele Ceraolo Spurio, dri-devel, Alan Previn
Enable PXP with MTL-GSC-CS: add the has_pxp into device info
and increase the timeouts for new GSC-CS + firmware specs.
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
drivers/gpu/drm/i915/i915_pci.c | 1 +
drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index bc1af7e8f398..0cac8cce1f7b 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -1148,6 +1148,7 @@ static const struct intel_device_info mtl_info = {
.has_guc_deprivilege = 1,
.has_mslice_steering = 0,
.has_snoop = 1,
+ .has_pxp = 1,
.__runtime.memory_regions = REGION_SMEM | REGION_STOLEN_LMEM,
.__runtime.platform_engine_mask = BIT(RCS0) | BIT(BCS0) | BIT(CCS0),
.require_force_probe = 1,
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
index 7bb06e67b155..e4e60e3b9216 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -56,7 +56,7 @@ static int pxp_wait_for_session_state(struct intel_pxp *pxp, u32 id, bool in_pla
reg,
mask,
in_play ? mask : 0,
- 100);
+ 250);
intel_runtime_pm_put(uncore->rpm, wakeref);
--
2.39.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/9] drm/i915/pxp: Add MTL PXP GSC-CS back-end skeleton
2023-01-11 21:42 ` [PATCH v2 1/9] drm/i915/pxp: Add MTL PXP GSC-CS back-end skeleton Alan Previn
@ 2023-01-18 17:51 ` Ceraolo Spurio, Daniele
2023-01-20 0:20 ` Teres Alexis, Alan Previn
0 siblings, 1 reply; 27+ messages in thread
From: Ceraolo Spurio, Daniele @ 2023-01-18 17:51 UTC (permalink / raw)
To: Alan Previn, intel-gfx; +Cc: Juston Li, dri-devel
On 1/11/2023 1:42 PM, Alan Previn wrote:
> Add MTL PXP GSC-CS back-end stub functions hook them
> up from PXP front-end and PXP session management functions.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/pxp/intel_pxp.c | 19 +++++++++++--
> drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 23 +++++++++++++++
> drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h | 30 ++++++++++++++++++++
> drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 6 +++-
> drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 6 ++++
> 6 files changed, 81 insertions(+), 4 deletions(-)
This patch is relatively small and it doesn't look like we have any
benefits by introducing the stubs early (please correct me if I'm
wrong), so I suggest to just squash it with the first patch that adds
code to those functions.
> create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index f47f00b162a4..eae4325310e8 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -330,6 +330,7 @@ i915-y += \
> i915-$(CONFIG_DRM_I915_PXP) += \
> pxp/intel_pxp_cmd.o \
> pxp/intel_pxp_debugfs.o \
> + pxp/intel_pxp_gsccs.o \
> pxp/intel_pxp_irq.o \
> pxp/intel_pxp_pm.o \
> pxp/intel_pxp_session.o
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index cfc9af8b3d21..be52bf92e847 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -12,6 +12,7 @@
> #include "i915_drv.h"
>
> #include "intel_pxp.h"
> +#include "intel_pxp_gsccs.h"
> #include "intel_pxp_irq.h"
> #include "intel_pxp_session.h"
> #include "intel_pxp_tee.h"
> @@ -132,7 +133,10 @@ static void pxp_init_full(struct intel_pxp *pxp)
> if (ret)
> return;
>
> - ret = intel_pxp_tee_component_init(pxp);
> + if (pxp->uses_gsccs)
> + ret = intel_pxp_gsccs_init(pxp);
> + else
> + ret = intel_pxp_tee_component_init(pxp);
> if (ret)
> goto out_context;
>
> @@ -157,6 +161,11 @@ static struct intel_gt *find_gt_for_required_teelink(struct drm_i915_private *i9
> return NULL;
> }
>
> +static bool pxp_has_gsccs(struct drm_i915_private *i915)
> +{
> + return (i915->media_gt && HAS_ENGINE(i915->media_gt, GSC0));
> +}
> +
> static struct intel_gt *find_gt_for_required_protected_content(struct drm_i915_private *i915)
> {
> if (!IS_ENABLED(CONFIG_DRM_I915_PXP) || !INTEL_INFO(i915)->has_pxp)
> @@ -167,7 +176,7 @@ static struct intel_gt *find_gt_for_required_protected_content(struct drm_i915_p
> * on the media GT. NOTE: if we have a media-tile with a GSC-engine,
> * the VDBOX is already present so skip that check
> */
> - if (i915->media_gt && HAS_ENGINE(i915->media_gt, GSC0))
> + if (pxp_has_gsccs(i915))
> return i915->media_gt;
>
> /*
> @@ -208,6 +217,7 @@ int intel_pxp_init(struct drm_i915_private *i915)
> return -ENOMEM;
>
> i915->pxp->ctrl_gt = gt;
> + i915->pxp->uses_gsccs = pxp_has_gsccs(i915);
At this point, if we're on a platform with the media GT we've already
selected it, so you can just do HAS_ENGINE(pxp->ctrl_gt, GSC0).
Also, do we really need a local variable to store this info? Can't we
just do HAS_ENGINE(...) where we need it?
Daniele
>
> /*
> * If full PXP feature is not available but HuC is loaded by GSC on pre-MTL
> @@ -229,7 +239,10 @@ void intel_pxp_fini(struct drm_i915_private *i915)
>
> i915->pxp->arb_is_valid = false;
>
> - intel_pxp_tee_component_fini(i915->pxp);
> + if (i915->pxp->uses_gsccs)
> + intel_pxp_gsccs_fini(i915->pxp);
> + else
> + intel_pxp_tee_component_fini(i915->pxp);
>
> destroy_vcs_context(i915->pxp);
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> new file mode 100644
> index 000000000000..21400650fc86
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2023 Intel Corporation.
> + */
> +
> +#include "i915_drv.h"
> +#include "intel_pxp_types.h"
> +#include "intel_pxp_gsccs.h"
> +
> +int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
> + int arb_session_id)
> +{
> + return -ENODEV;
> +}
> +
> +void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
> +{
> +}
> +
> +int intel_pxp_gsccs_init(struct intel_pxp *pxp)
> +{
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> new file mode 100644
> index 000000000000..07f8c9b6f636
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2022, Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_PXP_GSCCS_H__
> +#define __INTEL_PXP_GSCCS_H__
> +
> +#include <linux/types.h>
> +
> +struct intel_pxp;
> +
> +#ifdef CONFIG_DRM_I915_PXP
> +int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
> + int arb_session_id);
> +void intel_pxp_gsccs_fini(struct intel_pxp *pxp);
> +int intel_pxp_gsccs_init(struct intel_pxp *pxp);
> +
> +#else
> +static inline void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
> +{
> +}
> +
> +static inline int intel_pxp_gsccs_init(struct intel_pxp *pxp)
> +{
> + return 0;
> +}
> +#endif
> +
> +#endif /*__INTEL_PXP_GSCCS_H__ */
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> index ae413580b81a..080aa2209c5b 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -7,6 +7,7 @@
>
> #include "intel_pxp.h"
> #include "intel_pxp_cmd.h"
> +#include "intel_pxp_gsccs.h"
> #include "intel_pxp_session.h"
> #include "intel_pxp_tee.h"
> #include "intel_pxp_types.h"
> @@ -66,7 +67,10 @@ static int pxp_create_arb_session(struct intel_pxp *pxp)
> return -EEXIST;
> }
>
> - ret = intel_pxp_tee_cmd_create_arb_session(pxp, ARB_SESSION);
> + if (pxp->uses_gsccs)
> + ret = intel_pxp_gsccs_create_session(pxp, ARB_SESSION);
> + else
> + ret = intel_pxp_tee_cmd_create_arb_session(pxp, ARB_SESSION);
> if (ret) {
> drm_err(>->i915->drm, "tee cmd for arb session creation failed\n");
> return ret;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> index 7dc5f08d1583..43aa61c26de5 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> @@ -26,6 +26,12 @@ struct intel_pxp {
> */
> struct intel_gt *ctrl_gt;
>
> + /**
> + * @uses_gsccs: PXP interface for firmware access and pxp-session controls is
> + * via the GSC-CS engine. This is for MTL+ platforms.
> + */
> + bool uses_gsccs;
> +
> /**
> * @pxp_component: i915_pxp_component struct of the bound mei_pxp
> * module. Only set and cleared inside component bind/unbind functions,
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/9] drm/i915/pxp: Add GSC-CS back-end resource init and cleanup
2023-01-11 21:42 ` [PATCH v2 2/9] drm/i915/pxp: Add GSC-CS back-end resource init and cleanup Alan Previn
@ 2023-01-18 23:55 ` Ceraolo Spurio, Daniele
2023-01-20 20:48 ` Teres Alexis, Alan Previn
0 siblings, 1 reply; 27+ messages in thread
From: Ceraolo Spurio, Daniele @ 2023-01-18 23:55 UTC (permalink / raw)
To: Alan Previn, intel-gfx; +Cc: Juston Li, dri-devel
On 1/11/2023 1:42 PM, Alan Previn wrote:
> For MTL, PXP transport back-end uses the GSC engine to submit
> HECI packets for PXP arb session management. The command submission
> that uses non-priveleged mode requires us to allocate (or free)
> a set of execution submission resources (buffer-object, batch-buffer
> and context). Thus, do this one time allocation of resources in
> GSC-CS init and clean them up in fini.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
> .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 6 +-
> drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 216 +++++++++++++++++-
> drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 5 +
> 3 files changed, 225 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> index ad67e3f49c20..52b9a61bcdd4 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> @@ -10,7 +10,11 @@
> #include "intel_pxp_cmd_interface_cmn.h"
>
> /* PXP-Cmd-Op definitions */
> -#define PXP43_CMDID_START_HUC_AUTH 0x0000003A
> +#define PXP43_CMDID_START_HUC_AUTH 0x0000003A
> +
> +/* PXP-Packet sizes for MTL's GSCCS-HECI instruction */
> +#define PXP43_MAX_HECI_IN_SIZE (SZ_32K)
> +#define PXP43_MAX_HECI_OUT_SIZE (SZ_32K)
>
> /* PXP-Input-Packet: HUC-Authentication */
> struct pxp43_start_huc_auth_in {
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> index 21400650fc86..97ca187e6fde 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> @@ -3,9 +3,41 @@
> * Copyright(c) 2023 Intel Corporation.
> */
>
> +#include "gem/i915_gem_internal.h"
> +
> +#include "gt/intel_context.h"
> +
> #include "i915_drv.h"
> -#include "intel_pxp_types.h"
> +#include "intel_pxp_cmd_interface_43.h"
> #include "intel_pxp_gsccs.h"
> +#include "intel_pxp_types.h"
> +
> +struct gsccs_session_resources {
> + struct mutex cmd_mutex; /* Protects submission for arb session */
> + u64 host_session_handle; /* used by firmware to link commands to sessions */
> +
> + struct intel_context *ce; /* context for gsc command submission */
> + struct i915_ppgtt *ppgtt; /* ppgtt for gsc command submission */
The arb session creation is a kernel submission, so can't you just use
the default kernel ppgtt (i.e., gt->vm)?
> +
> + struct drm_i915_gem_object *pkt_obj; /* PXP HECI message packet buffer */
> + struct i915_vma *pkt_vma; /* PXP HECI message packet vma */
> + void *pkt_vaddr; /* PXP HECI message packet virt memory pointer */
> +
> + /* Buffer info for GSC engine batch buffer: */
> + struct drm_i915_gem_object *bb_obj; /* batch buffer object */
> + struct i915_vma *bb_vma; /* batch buffer vma */
> + void *bb_vaddr; /* batch buffer virtual memory pointer */
You aren't actually making use of any of the variables in this struct in
this patch, apart from initialization. Some of those are pretty clear on
what they will be used for (e.g. the context), bu others are a bit more
vague(e.g. the vaddrs). It'll probably be cleaner to reorder things so
the more implementation-specific variables are added when they're used.
I'll try to add more comment in follow-up patches.
> +};
> +
> +struct gsccs_teelink_priv {
> + /** @arb_exec_res: resources for arb-session GSC-CS PXP command submission */
> + struct gsccs_session_resources arb_exec_res;
> +};
> +
> +static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp)
> +{
> + return (struct gsccs_teelink_priv *)pxp->gsccs_priv;
> +}
Why do we need this layer of obfuscation with the void gsccs_priv? it's
not like that can be assigned to anything else but
gsccs_session_resources, so why not just have a pointer to that?
If you want to have different priv based on the backend (mei vs gsccs)
than it should be a more generic priv and be used in both cases.
>
> int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
> int arb_session_id)
> @@ -13,11 +45,193 @@ int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
> return -ENODEV;
> }
>
> +static void
> +gsccs_destroy_buffer(struct drm_i915_private *i915, struct i915_vma *vma,
> + struct drm_i915_gem_object *obj)
> +{
> + int err;
> +
> + i915_vma_unpin(vma);
> + err = i915_vma_unbind(vma);
> + if (err)
> + drm_dbg(&i915->drm, "Unexpected failure when vma-unbinding = %d\n", err);
I don't think you need to explicitly call unbind here, it should be
automatically covered by the object cleanup as long as it is unpinned
> +
> + i915_gem_object_unpin_map(obj);
> + i915_gem_object_unpin_pages(obj);
> + i915_gem_object_put(obj);
If you don't explicitly pin the pages (which you don't need to, see
comment below) the whole cleanup in this function can be done with just:
i915_vma_unpin_and_release(vma, I915_VMA_RELEASE_MAP);
> +}
> +
> +static int
> +gsccs_create_buffer(struct drm_i915_private *i915, const char *bufname,
> + size_t size, struct i915_ppgtt *ppgtt,
personal preference: IMO better to pass a generic "i915_address_space
*vm" to this function.
> + struct drm_i915_gem_object **obj,
> + struct i915_vma **vma, void **map)
> +{
> + int err = 0;
> +
> + *obj = i915_gem_object_create_internal(i915, size);
> + if (IS_ERR(*obj)) {
> + drm_err(&i915->drm, "Failed to allocate gsccs backend %s.\n", bufname);
> + err = PTR_ERR(*obj);
> + goto out_none;
> + }
> +
> + *vma = i915_vma_instance(*obj, &ppgtt->vm, NULL);
> + if (IS_ERR(*vma)) {
> + drm_err(&i915->drm, "Failed to vma-instance gsccs backend %s.\n", bufname);
> + err = PTR_ERR(*vma);
> + goto out_put;
> + }
> +
> + err = i915_gem_object_pin_pages_unlocked(*obj);
> + if (err) {
> + drm_err(&i915->drm, "Failed to pin gsccs backend %s.\n", bufname);
> + goto out_put;
> + }
You're doing a vma_pin below, so no need to explicitly pin the pages
here as the vma_pin will cover it.
Also, do you need the object to be returned by the function? As
mentioned above, the cleanup can be done with just the vma pointer and
from a quick look I don't see the object being used in follow up patches.
> +
> + /* map to the virtual memory pointer */
> + *map = i915_gem_object_pin_map_unlocked(*obj, i915_coherent_map_type(i915, *obj, true));
> + if (IS_ERR(*map)) {
> + drm_err(&i915->drm, "Failed to map gsccs backend %s.\n", bufname);
> + err = PTR_ERR(*map);
> + goto out_unpin;
> + }
> +
> + /* all PXP sessions commands are treated as non-priveleged */
> + err = i915_vma_pin(*vma, 0, 0, PIN_USER);
> + if (err) {
> + drm_err(&i915->drm, "Failed to vma-pin gsccs backend %s.\n", bufname);
> + goto out_unmap;
> + }
> +
> + return 0;
> +
> +out_unmap:
> + i915_gem_object_unpin_map(*obj);
> +out_unpin:
> + i915_gem_object_unpin_pages(*obj);
> +out_put:
> + i915_gem_object_put(*obj);
> +out_none:
> + *obj = NULL;
> + *vma = NULL;
> + *map = NULL;
> +
> + return err;
> +}
> +
> +static void
> +gsccs_destroy_execution_resource(struct intel_pxp *pxp,
> + struct gsccs_session_resources *strm_res)
> +{
> + if (strm_res->ce)
> + intel_context_put(strm_res->ce);
> + if (strm_res->bb_obj)
> + gsccs_destroy_buffer(pxp->ctrl_gt->i915, strm_res->bb_vma, strm_res->bb_obj);
> + if (strm_res->pkt_obj)
> + gsccs_destroy_buffer(pxp->ctrl_gt->i915, strm_res->pkt_vma, strm_res->pkt_obj);
> + if (strm_res->ppgtt)
> + i915_vm_put(&strm_res->ppgtt->vm);
> +
> + memset(strm_res, 0, sizeof(*strm_res));
> +}
> +
> +static int
> +gsccs_allocate_execution_resource(struct intel_pxp *pxp,
> + struct gsccs_session_resources *strm_res)
> +{
> + struct intel_gt *gt = pxp->ctrl_gt;
> + struct intel_engine_cs *engine = gt->engine[GSC0];
> + struct i915_ppgtt *ppgtt;
> + struct intel_context *ce;
> + int err = 0;
> +
> + /*
> + * First, ensure the GSC engine is present.
> + * NOTE: Backend should would only be called with the correct gt.
typo: should would
> + */
> + if (!engine)
> + return -ENODEV;
> +
> + mutex_init(&strm_res->cmd_mutex);
> +
> + ppgtt = i915_ppgtt_create(gt, 0);
> + if (IS_ERR(ppgtt))
> + return PTR_ERR(ppgtt);
> +
> + strm_res->ppgtt = ppgtt;
> +
> + /*
> + * Now, allocate, pin and map two objects, one for the heci message packet
> + * and another for the batch buffer we submit into GSC engine (that includes the packet).
> + * NOTE: GSC-CS backend is currently only supported on MTL, so we allocate shmem.
> + */
> + err = gsccs_create_buffer(pxp->ctrl_gt->i915, "Heci Packet",
> + PXP43_MAX_HECI_IN_SIZE + PXP43_MAX_HECI_OUT_SIZE,
> + strm_res->ppgtt,
> + &strm_res->pkt_obj, &strm_res->pkt_vma,
> + &strm_res->pkt_vaddr);
> + if (err) {
> + gsccs_destroy_execution_resource(pxp, strm_res);
> + return err;
> + }
> +
> + err = gsccs_create_buffer(pxp->ctrl_gt->i915, "Batch Buffer",
> + PAGE_SIZE, strm_res->ppgtt,
> + &strm_res->bb_obj, &strm_res->bb_vma,
> + &strm_res->bb_vaddr);
> + if (err) {
> + gsccs_destroy_execution_resource(pxp, strm_res);
> + return err;
> + }
> + /*
> + * TODO: Consider optimization of pre-populating batch buffer
> + * with the send-HECI instruction now at init and reuse through its life.
> + */
This looks like a nice optimization, and it would also allow us to drop
the bb_vaddr variable from the struct. If the only heci message we're
ever going to send with this object is the session creation, we can
probably also fill the "send" section of the pkt.
> +
> + /* Finally, create an intel_context to be used during the submission */
> + ce = intel_context_create(engine);
> + if (IS_ERR(ce)) {
> + drm_err(>->i915->drm, "Failed creating gsccs backend ctx\n");
> + gsccs_destroy_execution_resource(pxp, strm_res);
we usually prefer onion unwind to calling the full destroy function from
the create one. Not a blocker.
Daniele
> + return PTR_ERR(ce);
> + }
> + i915_vm_put(ce->vm);
> + ce->vm = i915_vm_get(&ppgtt->vm);
> +
> + strm_res->ce = ce;
> +
> + return 0;
> +}
> +
> void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
> {
> + struct gsccs_teelink_priv *gsccs = pxp_to_gsccs_priv(pxp);
> +
> + if (!gsccs)
> + return;
> +
> + gsccs_destroy_execution_resource(pxp, &gsccs->arb_exec_res);
> + kfree(gsccs);
> + pxp->gsccs_priv = NULL;
> }
>
> int intel_pxp_gsccs_init(struct intel_pxp *pxp)
> {
> + struct gsccs_teelink_priv *gsccs;
> + int ret;
> +
> + gsccs = kzalloc(sizeof(*gsccs), GFP_KERNEL);
> + if (!gsccs)
> + return -ENOMEM;
> +
> + ret = gsccs_allocate_execution_resource(pxp, &gsccs->arb_exec_res);
> + if (ret) {
> + kfree(gsccs);
> + return ret;
> + }
> +
> + pxp->gsccs_priv = gsccs;
> +
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> index 43aa61c26de5..fdcb9a66f691 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> @@ -32,6 +32,11 @@ struct intel_pxp {
> */
> bool uses_gsccs;
>
> + /**
> + * @gsccs_priv: GSC-CS based tee-link private context.
> + */
> + void *gsccs_priv;
> +
> /**
> * @pxp_component: i915_pxp_component struct of the bound mei_pxp
> * module. Only set and cleared inside component bind/unbind functions,
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/9] drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation
2023-01-11 21:42 ` [PATCH v2 3/9] drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation Alan Previn
@ 2023-01-19 0:09 ` Ceraolo Spurio, Daniele
2023-01-20 20:49 ` Teres Alexis, Alan Previn
0 siblings, 1 reply; 27+ messages in thread
From: Ceraolo Spurio, Daniele @ 2023-01-19 0:09 UTC (permalink / raw)
To: Alan Previn, intel-gfx; +Cc: Juston Li, dri-devel
On 1/11/2023 1:42 PM, Alan Previn wrote:
> Add MTL hw-plumbing enabling for KCR operation under PXP
> which includes:
>
> 1. Updating 'pick-gt' to get the media tile for
> KCR interrupt handling
> 2. Adding MTL's KCR registers for PXP operation
> (init, status-checking, etc.).
>
> While doing #2, lets create a separate registers header file for PXP
> to be consistent with other i915 global subsystems.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_gt_irq.c | 3 +-
> drivers/gpu/drm/i915/pxp/intel_pxp.c | 35 ++++++++++++--------
> drivers/gpu/drm/i915/pxp/intel_pxp_regs.h | 26 +++++++++++++++
> drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 29 +++++++++++-----
> 4 files changed, 70 insertions(+), 23 deletions(-)
> create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_regs.h
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_irq.c b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> index 8fac2660e16b..957fa11373fc 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_irq.c
> @@ -100,7 +100,8 @@ static struct intel_gt *pick_gt(struct intel_gt *gt, u8 class, u8 instance)
> case VIDEO_ENHANCEMENT_CLASS:
> return media_gt;
> case OTHER_CLASS:
> - if (instance == OTHER_GSC_INSTANCE && HAS_ENGINE(media_gt, GSC0))
> + if ((instance == OTHER_GSC_INSTANCE || instance == OTHER_KCR_INSTANCE) &&
> + HAS_ENGINE(media_gt, GSC0))
> return media_gt;
> fallthrough;
> default:
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index be52bf92e847..809b49f59594 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -14,6 +14,7 @@
> #include "intel_pxp.h"
> #include "intel_pxp_gsccs.h"
> #include "intel_pxp_irq.h"
> +#include "intel_pxp_regs.h"
> #include "intel_pxp_session.h"
> #include "intel_pxp_tee.h"
> #include "intel_pxp_types.h"
> @@ -61,21 +62,30 @@ bool intel_pxp_is_active(const struct intel_pxp *pxp)
> return IS_ENABLED(CONFIG_DRM_I915_PXP) && pxp && pxp->arb_is_valid;
> }
>
> -/* KCR register definitions */
> -#define KCR_INIT _MMIO(0x320f0)
> -/* Setting KCR Init bit is required after system boot */
> -#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES REG_BIT(14)
> +static i915_reg_t get_kcr_reg(const struct intel_pxp *pxp)
> +{
> + if (pxp->gsccs_priv)
IMO here a better check would be:
if (pxp->ctrl_gt->type == GT_MEDIA)
It's equivalent, but IMO from a readability perspective it highlights
the fact that this is a change due to us moving to the media GT model
and has nothing to do with the GSC CS itself.
> + return MTL_KCR_INIT;
> + return GEN12_KCR_INIT;
> +}
>
> -static void kcr_pxp_enable(struct intel_gt *gt)
> +static void kcr_pxp_set_status(const struct intel_pxp *pxp, bool enable)
> {
> - intel_uncore_write(gt->uncore, KCR_INIT,
> - _MASKED_BIT_ENABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES));
> + i915_reg_t reg = get_kcr_reg(pxp);
> + u32 val = enable ? _MASKED_BIT_ENABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES) :
> + _MASKED_BIT_DISABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES);
> +
> + intel_uncore_write(pxp->ctrl_gt->uncore, reg, val);
> }
>
> -static void kcr_pxp_disable(struct intel_gt *gt)
> +static void kcr_pxp_enable(const struct intel_pxp *pxp)
> {
> - intel_uncore_write(gt->uncore, KCR_INIT,
> - _MASKED_BIT_DISABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES));
> + kcr_pxp_set_status(pxp, true);
> +}
> +
> +static void kcr_pxp_disable(const struct intel_pxp *pxp)
> +{
> + kcr_pxp_set_status(pxp, false);
> }
>
> static int create_vcs_context(struct intel_pxp *pxp)
> @@ -323,14 +333,13 @@ int intel_pxp_start(struct intel_pxp *pxp)
>
> void intel_pxp_init_hw(struct intel_pxp *pxp)
> {
> - kcr_pxp_enable(pxp->ctrl_gt);
> + kcr_pxp_enable(pxp);
> intel_pxp_irq_enable(pxp);
> }
>
> void intel_pxp_fini_hw(struct intel_pxp *pxp)
> {
> - kcr_pxp_disable(pxp->ctrl_gt);
> -
> + kcr_pxp_disable(pxp);
> intel_pxp_irq_disable(pxp);
> }
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_regs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_regs.h
> new file mode 100644
> index 000000000000..dd4131903d4e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_regs.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2023, Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_PXP_REGS_H__
> +#define __INTEL_PXP_REGS_H__
> +
> +#include "i915_reg_defs.h"
> +
> +/* KCR enable/disable control */
> +#define GEN12_KCR_INIT _MMIO(0x320f0)
> +#define MTL_KCR_INIT _MMIO(0x3860f0)
> +
> +/* Setting KCR Init bit is required after system boot */
> +#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES REG_BIT(14)
> +
> +/* KCR hwdrm session in play status 0-31 */
> +#define GEN12_KCR_SIP _MMIO(0x32260)
> +#define MTL_KCR_SIP _MMIO(0x386260)
> +
> +/* PXP global terminate register for session termination */
> +#define GEN12_KCR_GLOBAL_TERMINATE _MMIO(0x320f8)
> +#define MTL_KCR_GLOBAL_TERMINATE _MMIO(0x3860f8)
Non blocking suggestion:
it looks like all KCR regs are at the same relative offsets, just from a
different base (which makes, sense, because the HW just got moved to the
media tile as-is). So we could possibly have something like what we do
for the engines:
#define GEN12_KCR_BASE 0x32000
#define MTL_KCR_BASE 0x386000
#define KCR_INIT(base) _MMIO(base + 0xf0)
#define KCR_GLOBAL_TERMINATE(base) _MMIO(base + 0xf8)
#define KCR_SIP(base) _MMIO(base + 0x260)
Then if we store the correct base in the pxp structure we can just pass
it in the define when we need to access a register and remove the if
conditions at each access.
Daniele
> +
> +#endif /* __INTEL_PXP_REGS_H__ */
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> index 080aa2209c5b..7bb06e67b155 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
> @@ -11,23 +11,25 @@
> #include "intel_pxp_session.h"
> #include "intel_pxp_tee.h"
> #include "intel_pxp_types.h"
> +#include "intel_pxp_regs.h"
>
> #define ARB_SESSION I915_PROTECTED_CONTENT_DEFAULT_SESSION /* shorter define */
>
> -#define GEN12_KCR_SIP _MMIO(0x32260) /* KCR hwdrm session in play 0-31 */
> -
> -/* PXP global terminate register for session termination */
> -#define PXP_GLOBAL_TERMINATE _MMIO(0x320f8)
> -
> static bool intel_pxp_session_is_in_play(struct intel_pxp *pxp, u32 id)
> {
> struct intel_uncore *uncore = pxp->ctrl_gt->uncore;
> intel_wakeref_t wakeref;
> + i915_reg_t reg;
> u32 sip = 0;
>
> /* if we're suspended the session is considered off */
> - with_intel_runtime_pm_if_in_use(uncore->rpm, wakeref)
> - sip = intel_uncore_read(uncore, GEN12_KCR_SIP);
> + with_intel_runtime_pm_if_in_use(uncore->rpm, wakeref) {
> + if (pxp->gsccs_priv)
> + reg = MTL_KCR_SIP;
> + else
> + reg = GEN12_KCR_SIP;
> + sip = intel_uncore_read(uncore, reg);
> + }
>
> return sip & BIT(id);
> }
> @@ -37,6 +39,7 @@ static int pxp_wait_for_session_state(struct intel_pxp *pxp, u32 id, bool in_pla
> struct intel_uncore *uncore = pxp->ctrl_gt->uncore;
> intel_wakeref_t wakeref;
> u32 mask = BIT(id);
> + i915_reg_t reg;
> int ret;
>
> /* if we're suspended the session is considered off */
> @@ -44,8 +47,13 @@ static int pxp_wait_for_session_state(struct intel_pxp *pxp, u32 id, bool in_pla
> if (!wakeref)
> return in_play ? -ENODEV : 0;
>
> + if (pxp->gsccs_priv)
> + reg = MTL_KCR_SIP;
> + else
> + reg = GEN12_KCR_SIP;
> +
> ret = intel_wait_for_register(uncore,
> - GEN12_KCR_SIP,
> + reg,
> mask,
> in_play ? mask : 0,
> 100);
> @@ -112,7 +120,10 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
> return ret;
> }
>
> - intel_uncore_write(gt->uncore, PXP_GLOBAL_TERMINATE, 1);
> + if (pxp->gsccs_priv)
> + intel_uncore_write(gt->uncore, MTL_KCR_GLOBAL_TERMINATE, 1);
> + else
> + intel_uncore_write(gt->uncore, GEN12_KCR_GLOBAL_TERMINATE, 1);
>
> return ret;
> }
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/9] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC
2023-01-11 21:42 ` [PATCH v2 4/9] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC Alan Previn
@ 2023-01-19 1:28 ` Ceraolo Spurio, Daniele
2023-01-20 20:58 ` Teres Alexis, Alan Previn
0 siblings, 1 reply; 27+ messages in thread
From: Ceraolo Spurio, Daniele @ 2023-01-19 1:28 UTC (permalink / raw)
To: Alan Previn, intel-gfx; +Cc: Juston Li, dri-devel
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.
>
> 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 very 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:
> - 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).
> - 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
> GGTT (not in this series) vs PPGTT (this patch) 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 everything else from
> input/output packet size verification to handling the
> responses from security firmware (such as requiring a retry).
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 2 +
> .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c | 107 ++++++++++++++++++
> .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h | 69 +++++++++++
> 4 files changed, 179 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
> create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index eae4325310e8..7dc18554da10 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -194,6 +194,7 @@ i915-y += \
> i915-y += \
> gt/uc/intel_gsc_fw.o \
> gt/uc/intel_gsc_uc.o \
> + gt/uc/intel_gsc_uc_heci_cmd_submit.o \
> gt/uc/intel_guc.o \
> gt/uc/intel_guc_ads.o \
> gt/uc/intel_guc_capture.o \
> diff --git a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> index 2af1ae3831df..454179884801 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
> @@ -439,6 +439,8 @@
> #define GSC_FW_LOAD GSC_INSTR(1, 0, 2)
> #define HECI1_FW_LIMIT_VALID (1 << 31)
>
> +#define GSC_HECI_CMD_PKT GSC_INSTR(0, 0, 6)
> +
> /*
> * Used to convert any address to canonical form.
> * Starting from gen8, some commands (e.g. STATE_BASE_ADDRESS,
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
> new file mode 100644
> index 000000000000..01bb7e587b1b
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c
> @@ -0,0 +1,107 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#include <linux/types.h>
> +
> +#include "i915_drv.h"
> +#include "i915_vma.h"
> +
> +#include "gt/intel_gpu_commands.h"
> +#include "gt/intel_context.h"
> +#include "gt/intel_gt.h"
> +#include "gt/intel_ring.h"
> +
> +#include "intel_gsc_fw.h"
> +#include "intel_gsc_uc.h"
> +#include "intel_gsc_uc_heci_cmd_submit.h"
> +
> +void intel_gsc_uc_heci_cmd_emit_mtl_header(struct intel_gsc_mtl_header *header,
> + u8 heci_client_id, u32 message_size,
> + u64 host_session_id)
> +{
> + host_session_id &= ~GSC_HECI_HOST_SESSION_USAGE_MASK;
> + if (heci_client_id == GSC_HECI_MEADDRESS_PXP)
> + host_session_id |= GSC_HECI_SESSION_PXP_SINGLE;
> +
> + header->validity_marker = GSC_HECI_VALIDITY_MARKER;
> + header->heci_client_id = heci_client_id;
> + header->host_session_handle = host_session_id;
> + header->header_version = MTL_GSC_HECI_HEADER_VERSION;
> + header->message_size = message_size;
> +}
> +
> +static void
> +emit_gsc_heci_pkt_nonpriv(u32 *cs, struct intel_gsc_heci_non_priv_pkt *pkt)
> +{
> + *cs++ = GSC_HECI_CMD_PKT;
> + *cs++ = lower_32_bits(pkt->addr_in);
> + *cs++ = upper_32_bits(pkt->addr_in);
> + *cs++ = pkt->size_in;
> + *cs++ = lower_32_bits(pkt->addr_out);
> + *cs++ = upper_32_bits(pkt->addr_out);
> + *cs++ = pkt->size_out;
> + *cs++ = 0;
> + *cs++ = MI_BATCH_BUFFER_END;
> +}
> +
> +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
> +{
> + 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.
> + 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.
> +
> + 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
> new file mode 100644
> index 000000000000..23155bed3fee
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> @@ -0,0 +1,69 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#ifndef _INTEL_GSC_UC_HECI_CMD_H_
> +#define _INTEL_GSC_UC_HECI_CMD_H_
> +
> +#include <linux/types.h>
> +
> +struct i915_vma;
> +struct intel_context;
> +struct intel_gsc_uc;
> +
> +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
> +
> + u64 gsc_message_handle;
> +
> + u32 message_size; /* lower 20 bits only, upper 12 are reserved */
> +
> + /*
> + * Flags mask:
> + * Bit 0: Pending
> + * Bit 1: Session Cleanup;
> + * Bits 2-15: Flags
> + * Bits 16-31: Extension Size
> + */
> + u32 flags;
> +#define GSC_HECI_FLAG_MSG_PENDING BIT(0)
> +#define GSC_HECI_FLAG_MSG_CLEANUP BIT(1)
> +
> + u32 status;
> +} __packed;
> +
> +struct intel_gsc_heci_non_priv_pkt {
> + u64 addr_in;
> + u32 size_in;
> + u64 addr_out;
> + u32 size_out;
> + struct i915_vma *heci_pkt_vma;
> + struct i915_vma *bb_vma;
> +};
> +
> +void
> +intel_gsc_uc_heci_cmd_emit_mtl_header(struct intel_gsc_mtl_header *header,
> + u8 heci_client_id, u32 msg_size,
> + u64 host_session_id);
> +
> +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);
> +#endif
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/9] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages
2023-01-11 21:42 ` [PATCH v2 5/9] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages Alan Previn
@ 2023-01-19 1:40 ` Ceraolo Spurio, Daniele
2023-01-20 23:46 ` Teres Alexis, Alan Previn
0 siblings, 1 reply; 27+ messages in thread
From: Ceraolo Spurio, Daniele @ 2023-01-19 1:40 UTC (permalink / raw)
To: Alan Previn, intel-gfx; +Cc: Juston Li, dri-devel
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@intel.com>
> ---
> drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 93 ++++++++++++++++++++++
> 1 file changed, 93 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> index 97ca187e6fde..ff235822743e 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> @@ -6,6 +6,7 @@
> #include "gem/i915_gem_internal.h"
>
> #include "gt/intel_context.h"
> +#include "gt/uc/intel_gsc_uc_heci_cmd_submit.h"
>
> #include "i915_drv.h"
> #include "intel_pxp_cmd_interface_43.h"
> @@ -39,6 +40,98 @@ static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp
> return (struct gsccs_teelink_priv *)pxp->gsccs_priv;
> }
>
> +static int gsccs_send_message(struct intel_pxp *pxp,
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.
> + 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).
> +
> + 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.
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)
> {
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 6/9] drm/i915/pxp: Add ARB session creation with new PXP API Ver4.3
2023-01-11 21:42 ` [PATCH v2 6/9] drm/i915/pxp: Add ARB session creation with new PXP API Ver4.3 Alan Previn
@ 2023-01-19 1:50 ` Ceraolo Spurio, Daniele
2023-01-25 4:13 ` Teres Alexis, Alan Previn
0 siblings, 1 reply; 27+ messages in thread
From: Ceraolo Spurio, Daniele @ 2023-01-19 1:50 UTC (permalink / raw)
To: Alan Previn, intel-gfx; +Cc: Juston Li, dri-devel
On 1/11/2023 1:42 PM, Alan Previn wrote:
> Add MTL's function for ARB session creation using PXP firmware
> version 4.3 ABI structure format.
>
> Before checking the return status, look at the GSC-CS-Mem-Header's
> pending-bit which means the GSC firmware is busy and we should
> resubmit.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
> .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 21 +++++++
> drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 56 ++++++++++++++++++-
> 2 files changed, 74 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> index 52b9a61bcdd4..ee78c0817ba1 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h
> @@ -11,6 +11,7 @@
>
> /* PXP-Cmd-Op definitions */
> #define PXP43_CMDID_START_HUC_AUTH 0x0000003A
> +#define PXP43_CMDID_INIT_SESSION 0x00000036
>
> /* PXP-Packet sizes for MTL's GSCCS-HECI instruction */
> #define PXP43_MAX_HECI_IN_SIZE (SZ_32K)
> @@ -27,4 +28,24 @@ struct pxp43_start_huc_auth_out {
> struct pxp_cmd_header header;
> } __packed;
>
> +/* PXP-Input-Packet: Init PXP session */
> +struct pxp43_create_arb_in {
> + struct pxp_cmd_header header;
> + /* header.stream_id fields for vesion 4.3 of Init PXP session: */
> + #define PXP43_INIT_SESSION_VALID GENMASK(0, 0)
GENMASK(0, 0) -> BIT(0) ? same for (1, 1)
> + #define PXP43_INIT_SESSION_APPTYPE GENMASK(1, 1)
> + #define PXP43_INIT_SESSION_APPID GENMASK(17, 2)
> + u32 protection_mode;
> + #define PXP43_INIT_SESSION_PROTECTION_ARB 0x2
> + u32 sub_session_id;
> + u32 init_flags;
> + u32 rsvd[12];
> +} __packed;
> +
> +/* PXP-Input-Packet: Init PXP session */
> +struct pxp43_create_arb_out {
> + struct pxp_cmd_header header;
> + u32 rsvd[8];
> +} __packed;
> +
> #endif /* __INTEL_PXP_FW_INTERFACE_43_H__ */
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> index ff235822743e..1b06629ac16e 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> @@ -43,7 +43,8 @@ static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp
> static int gsccs_send_message(struct intel_pxp *pxp,
> void *msg_in, size_t msg_in_size,
> void *msg_out, size_t msg_out_size_max,
> - size_t *msg_out_len)
> + size_t *msg_out_len,
> + u64 *gsc_msg_handle_retry)
> {
> struct intel_gt *gt = pxp->ctrl_gt;
> struct drm_i915_private *i915 = gt->i915;
> @@ -75,6 +76,9 @@ static int gsccs_send_message(struct intel_pxp *pxp,
> msg_in_size + sizeof(*header),
> exec->host_session_handle);
>
> + /* copy caller provided gsc message handle if this is polling for a prior msg completion */
> + header->gsc_message_handle = *gsc_msg_handle_retry;
> +
> memcpy(exec->pkt_vaddr + sizeof(*header), msg_in, msg_in_size);
>
> pkt.addr_in = i915_vma_offset(exec->pkt_vma);
> @@ -91,7 +95,7 @@ static int gsccs_send_message(struct intel_pxp *pxp,
> goto unlock;
> }
>
> - /* we keep separate location for reply, so get the response header loc first */
> + /* we keep separate location for reply, so go to the response header now */
Any reason to update the comment in this patch and not directly in the
original one?
> header = exec->pkt_vaddr + PXP43_MAX_HECI_IN_SIZE;
>
> /* Response validity marker, status and busyness */
> @@ -108,6 +112,13 @@ static int gsccs_send_message(struct intel_pxp *pxp,
> }
> if (header->flags & GSC_HECI_FLAG_MSG_PENDING) {
> drm_dbg(&i915->drm, "gsc PXP reply is busy\n");
> + /*
> + * When the GSC firmware replies with pending bit, it means that the requested
> + * operation has begun but the completion is pending and the caller needs
> + * to re-request with the gsc_message_handle that was returned by the firmware.
> + * until the pending bit is turned off.
> + */
> + *gsc_msg_handle_retry = header->gsc_message_handle;
> ret = -EAGAIN;
> goto unlock;
> }
> @@ -135,7 +146,46 @@ static int gsccs_send_message(struct intel_pxp *pxp,
> int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
> int arb_session_id)
> {
> - return -ENODEV;
> + struct gsccs_session_resources *exec = &pxp_to_gsccs_priv(pxp)->arb_exec_res;
> + struct pxp43_create_arb_in msg_in = {0};
> + struct pxp43_create_arb_out msg_out = {0};
> + u64 gsc_session_retry = 0;
> + int insize, outsize, ret, tries = 0;
> + void *inptr, *outptr;
> +
> + /* get a unique host-session-handle (used later in HW cmds) at time of session creation */
> + get_random_bytes(&exec->host_session_handle, sizeof(exec->host_session_handle));
> +
> + msg_in.header.api_version = PXP_APIVER(4, 3);
> + msg_in.header.command_id = PXP43_CMDID_INIT_SESSION;
> + msg_in.header.stream_id = (FIELD_PREP(PXP43_INIT_SESSION_APPID, arb_session_id) |
> + FIELD_PREP(PXP43_INIT_SESSION_VALID, 1) |
> + FIELD_PREP(PXP43_INIT_SESSION_APPTYPE, 0));
> + msg_in.header.buffer_len = sizeof(msg_in) - sizeof(msg_in.header);
> + msg_in.protection_mode = PXP43_INIT_SESSION_PROTECTION_ARB;
> +
> + inptr = &msg_in;
> + outptr = &msg_out;
> + insize = sizeof(msg_in);
> + outsize = sizeof(msg_out);
Are those local vars required? Can't you just pass the values directly?
it doesn't look like you're saving many characters.
> +
> + /*
> + * Keep sending request if GSC firmware was busy.
> + * Based on test data, we expects a worst case delay of 250 milisecs.
> + */
> + do {
> + ret = gsccs_send_message(pxp,
> + inptr, insize,
> + outptr, outsize, NULL,
> + &gsc_session_retry);
> + /* Only try again if gsc says so */
> + if (ret != -EAGAIN)
> + break;
> +
> + msleep(20);
I seem to remember that the recommended sleep time was 50ms, but can't
find that in the specs now.
> + } while (++tries < 12);
Found a note in the specs that says we should give up retrying after 2
secs, so should probably adjust the retry count accordingly.
Daniele
> +
> + return ret;
> }
>
> static void
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 7/9] drm/i915/pxp: MTL-KCR interrupt ctrl's are in GT-0
2023-01-11 21:42 ` [PATCH v2 7/9] drm/i915/pxp: MTL-KCR interrupt ctrl's are in GT-0 Alan Previn
@ 2023-01-19 2:01 ` Ceraolo Spurio, Daniele
2023-01-23 21:11 ` Teres Alexis, Alan Previn
0 siblings, 1 reply; 27+ messages in thread
From: Ceraolo Spurio, Daniele @ 2023-01-19 2:01 UTC (permalink / raw)
To: Alan Previn, intel-gfx; +Cc: Juston Li, dri-devel
On 1/11/2023 1:42 PM, Alan Previn wrote:
> Despite KCR subsystem being in the media-tile (close to the
> GSC-CS), the IRQ controls for it are on GT-0 with other global
> IRQ controls. Thus, add a helper for KCR hw interrupt
> enable/disable functions to get the correct gt structure (for
> uncore) for MTL.
>
> In the helper, we get GT-0's handle for uncore when touching
> IRQ registers despite the pxp->ctrl_gt being the media-tile.
> No difference for legacy of course.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
> drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +-
> drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 23 +++++++++++++++++---
> drivers/gpu/drm/i915/pxp/intel_pxp_irq.h | 8 +++++++
> 3 files changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> index 4b8e70caa3ad..9f6e300486b4 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> @@ -44,7 +44,7 @@ static int pxp_terminate_get(void *data, u64 *val)
> static int pxp_terminate_set(void *data, u64 val)
> {
> struct intel_pxp *pxp = data;
> - struct intel_gt *gt = pxp->ctrl_gt;
> + struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
This doesn't seem to be required here. The only use of gt in this
function is an assert below and both the root and media gt point to the
same irq_lock, so it doesn't matter which one we're using. Should we
keep it anyway just for safety in case things change in the future? or
just add a comment instead?
>
> if (!intel_pxp_is_active(pxp))
> return -ENODEV;
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> index 91e9622c07d0..2eef0c19e91a 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> @@ -8,6 +8,7 @@
> #include "gt/intel_gt_regs.h"
> #include "gt/intel_gt_types.h"
>
> +#include "i915_drv.h"
> #include "i915_irq.h"
> #include "i915_reg.h"
>
> @@ -17,6 +18,22 @@
> #include "intel_pxp_types.h"
> #include "intel_runtime_pm.h"
>
> +/**
> + * intel_pxp_get_irq_gt - Find the correct GT that owns KCR interrupts
> + * @pxp: pointer to pxp struct
> + *
> + * For platforms with a single GT, we return the pxp->ctrl_gt (as expected)
> + * but for MTL+ that has a media-tile, although the KCR engine is in the
> + * media-tile (i.e. pxp->ctrl_gt), the IRQ controls are on the root tile.
> + */
> +struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp)
> +{
> + if (pxp->uses_gsccs)
> + return to_gt(pxp->ctrl_gt->i915);
> +
> + return pxp->ctrl_gt;
AFAICT here we can skip the if and always return the root gt, because
that's what happens in both cases. If you want to make sure we don't get
issues in the future maybe instead add a:
GEM_BUG_ON(!i915->media_gt && !gt_is_root(pxp->ctrl_gt))
> +}
> +
> /**
> * intel_pxp_irq_handler - Handles PXP interrupts.
> * @pxp: pointer to pxp struct
> @@ -29,7 +46,7 @@ void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> if (GEM_WARN_ON(!intel_pxp_is_enabled(pxp)))
> return;
>
> - gt = pxp->ctrl_gt;
> + gt = intel_pxp_get_irq_gt(pxp);
Here also we only have the assert below
Daniele
>
> lockdep_assert_held(gt->irq_lock);
>
> @@ -68,7 +85,7 @@ static inline void pxp_irq_reset(struct intel_gt *gt)
>
> void intel_pxp_irq_enable(struct intel_pxp *pxp)
> {
> - struct intel_gt *gt = pxp->ctrl_gt;
> + struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
>
> spin_lock_irq(gt->irq_lock);
>
> @@ -83,7 +100,7 @@ void intel_pxp_irq_enable(struct intel_pxp *pxp)
>
> void intel_pxp_irq_disable(struct intel_pxp *pxp)
> {
> - struct intel_gt *gt = pxp->ctrl_gt;
> + struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
>
> /*
> * We always need to submit a global termination when we re-enable the
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
> index 8c292dc86f68..eea87c9eb62b 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
> @@ -9,6 +9,7 @@
> #include <linux/types.h>
>
> struct intel_pxp;
> +struct intel_gt;
>
> #define GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT BIT(1)
> #define GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT BIT(2)
> @@ -23,6 +24,8 @@ struct intel_pxp;
> void intel_pxp_irq_enable(struct intel_pxp *pxp);
> void intel_pxp_irq_disable(struct intel_pxp *pxp);
> void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir);
> +struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp);
> +
> #else
> static inline void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> {
> @@ -35,6 +38,11 @@ static inline void intel_pxp_irq_enable(struct intel_pxp *pxp)
> static inline void intel_pxp_irq_disable(struct intel_pxp *pxp)
> {
> }
> +
> +static inline struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp)
> +{
> + return NULL;
> +}
> #endif
>
> #endif /* __INTEL_PXP_IRQ_H__ */
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 8/9] drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component
2023-01-11 21:42 ` [PATCH v2 8/9] drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component Alan Previn
@ 2023-01-19 2:17 ` Ceraolo Spurio, Daniele
2023-01-24 0:15 ` Teres Alexis, Alan Previn
0 siblings, 1 reply; 27+ messages in thread
From: Ceraolo Spurio, Daniele @ 2023-01-19 2:17 UTC (permalink / raw)
To: Alan Previn, intel-gfx; +Cc: Juston Li, dri-devel
On 1/11/2023 1:42 PM, Alan Previn wrote:
> On legacy platforms, KCR HW enabling is done at the time the mei
> component interface is bound. It's also disabled during unbind.
> However, for MTL onwards, we don't depend on the tee-component
> operation before we can start sending GSC-CS firmware messages.
>
> Thus, immediately enable KCR HW on PXP's init, fini and resume.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
> drivers/gpu/drm/i915/pxp/intel_pxp.c | 52 ++++++++++++++++++------
> drivers/gpu/drm/i915/pxp/intel_pxp.h | 4 +-
> drivers/gpu/drm/i915/pxp/intel_pxp_pm.c | 10 ++---
> drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 13 +-----
> 4 files changed, 49 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index 809b49f59594..90e739345924 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -143,10 +143,12 @@ static void pxp_init_full(struct intel_pxp *pxp)
> if (ret)
> return;
>
> - if (pxp->uses_gsccs)
> + if (pxp->uses_gsccs) {
> ret = intel_pxp_gsccs_init(pxp);
> - else
> + intel_pxp_init_hw(pxp, true);
> + } else {
> ret = intel_pxp_tee_component_init(pxp);
> + }
> if (ret)
> goto out_context;
>
> @@ -249,10 +251,12 @@ void intel_pxp_fini(struct drm_i915_private *i915)
>
> i915->pxp->arb_is_valid = false;
>
> - if (i915->pxp->uses_gsccs)
> + if (i915->pxp->uses_gsccs) {
> + intel_pxp_fini_hw(i915->pxp, true);
> intel_pxp_gsccs_fini(i915->pxp);
> - else
> + } else {
> intel_pxp_tee_component_fini(i915->pxp);
> + }
>
> destroy_vcs_context(i915->pxp);
>
> @@ -304,8 +308,9 @@ int intel_pxp_start(struct intel_pxp *pxp)
> if (!intel_pxp_is_enabled(pxp))
> return -ENODEV;
>
> - if (wait_for(pxp_component_bound(pxp), 250))
> - return -ENXIO;
> + if (!pxp->uses_gsccs)
> + if (wait_for(pxp_component_bound(pxp), 250))
> + return -ENXIO;
>
> mutex_lock(&pxp->arb_mutex);
>
> @@ -331,16 +336,39 @@ int intel_pxp_start(struct intel_pxp *pxp)
> return ret;
> }
>
> -void intel_pxp_init_hw(struct intel_pxp *pxp)
> +static void
> +intel_pxp_hw_state_change(struct intel_pxp *pxp, bool enable,
> + bool skip_if_runtime_pm_off)
"skip_if_runtime_pm_off" is a bit confusing. maybe "needs_rpm" or
something like that would be a better option?
I'm also not super convinced about this common function with a
conditional rpm. wouldn't it be simpler to just add an
with_intel_runtime_pm_if_in_use() before the pxp_init/fini_hw in
pxp_init_full and pxp_fini? Not a blocker.
> +{
> + intel_wakeref_t wakeref;
> +
> + if (skip_if_runtime_pm_off) {
> + /* if we are suspended, the HW will be re-initialized on resume */
> + wakeref = intel_runtime_pm_get_if_in_use(&pxp->ctrl_gt->i915->runtime_pm);
> + if (!wakeref)
> + return;
> + }
> +
> + if (enable) {
> + kcr_pxp_enable(pxp);
> + intel_pxp_irq_enable(pxp);
> + } else {
> + kcr_pxp_disable(pxp);
> + intel_pxp_irq_disable(pxp);
> + }
> +
> + if (skip_if_runtime_pm_off)
> + intel_runtime_pm_put(&pxp->ctrl_gt->i915->runtime_pm, wakeref);
> +}
> +
> +void intel_pxp_init_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off)
> {
> - kcr_pxp_enable(pxp);
> - intel_pxp_irq_enable(pxp);
> + intel_pxp_hw_state_change(pxp, true, skip_if_runtime_pm_off);
> }
>
> -void intel_pxp_fini_hw(struct intel_pxp *pxp)
> +void intel_pxp_fini_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off)
> {
> - kcr_pxp_disable(pxp);
> - intel_pxp_irq_disable(pxp);
> + intel_pxp_hw_state_change(pxp, false, skip_if_runtime_pm_off);
> }
>
> int intel_pxp_key_check(struct intel_pxp *pxp,
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> index 04440fada711..6c1fe3f0a20c 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
> @@ -20,8 +20,8 @@ bool intel_pxp_is_active(const struct intel_pxp *pxp);
> int intel_pxp_init(struct drm_i915_private *i915);
> void intel_pxp_fini(struct drm_i915_private *i915);
>
> -void intel_pxp_init_hw(struct intel_pxp *pxp);
> -void intel_pxp_fini_hw(struct intel_pxp *pxp);
> +void intel_pxp_init_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off);
> +void intel_pxp_fini_hw(struct intel_pxp *pxp, bool skip_if_runtime_pm_off);
>
> void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
>
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> index 892d39cc61c1..94c1b2fe1eb2 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
> @@ -29,7 +29,7 @@ void intel_pxp_suspend(struct intel_pxp *pxp)
> return;
>
> with_intel_runtime_pm(&pxp->ctrl_gt->i915->runtime_pm, wakeref) {
> - intel_pxp_fini_hw(pxp);
> + intel_pxp_fini_hw(pxp, false);
If you go with the runtime pm inside the intel_pxp_fini_hw, then drop it
from here and have:
intel_pxp_fini_hw(pxp, true);
> pxp->hw_state_invalidated = false;
> }
> }
> @@ -40,14 +40,14 @@ void intel_pxp_resume(struct intel_pxp *pxp)
> return;
>
> /*
> - * The PXP component gets automatically unbound when we go into S3 and
> + * On Pre-MTL, PXP component gets automatically unbound when we go into S3 and
> * re-bound after we come out, so in that scenario we can defer the
> * hw init to the bind call.
> */
> - if (!pxp->pxp_component)
> + if (!pxp->uses_gsccs & !pxp->pxp_component)
You have a bitwise & instead of &&.
Also, I'd go instead with:
if (pxp->pxp_component_added && !pxp->pxp_component)
so we focus on the component availability.
Daniele
> return;
>
> - intel_pxp_init_hw(pxp);
> + intel_pxp_init_hw(pxp, false);
> }
>
> void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
> @@ -57,7 +57,7 @@ void intel_pxp_runtime_suspend(struct intel_pxp *pxp)
>
> pxp->arb_is_valid = false;
>
> - intel_pxp_fini_hw(pxp);
> + intel_pxp_fini_hw(pxp, false);
>
> pxp->hw_state_invalidated = false;
> }
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> index d50354bfb993..9b34f2056b19 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
> @@ -141,16 +141,9 @@ static int i915_pxp_tee_component_bind(struct device *i915_kdev,
> }
> }
>
> - /* if we are suspended, the HW will be re-initialized on resume */
> - wakeref = intel_runtime_pm_get_if_in_use(&i915->runtime_pm);
> - if (!wakeref)
> - return 0;
> -
> /* the component is required to fully start the PXP HW */
> if (intel_pxp_is_enabled(pxp))
> - intel_pxp_init_hw(pxp);
> -
> - intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> + intel_pxp_init_hw(pxp, true);
>
> return ret;
> }
> @@ -160,11 +153,9 @@ static void i915_pxp_tee_component_unbind(struct device *i915_kdev,
> {
> struct drm_i915_private *i915 = kdev_to_i915(i915_kdev);
> struct intel_pxp *pxp = i915->pxp;
> - intel_wakeref_t wakeref;
>
> if (intel_pxp_is_enabled(pxp))
> - with_intel_runtime_pm_if_in_use(&i915->runtime_pm, wakeref)
> - intel_pxp_fini_hw(pxp);
> + intel_pxp_fini_hw(pxp, true);
>
> mutex_lock(&pxp->tee_mutex);
> pxp->pxp_component = NULL;
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/9] drm/i915/pxp: Add MTL PXP GSC-CS back-end skeleton
2023-01-18 17:51 ` Ceraolo Spurio, Daniele
@ 2023-01-20 0:20 ` Teres Alexis, Alan Previn
0 siblings, 0 replies; 27+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-20 0:20 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: justonli, dri-devel
Thanks for reviewing Daniele - will fix these on re-rev.
And you're right - we dont need a variable "gsccs" (so HAS_ENGINE should work fine).
On Wed, 2023-01-18 at 09:51 -0800, Ceraolo Spurio, Daniele wrote:
>
>
Alan: [snip]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/9] drm/i915/pxp: Add GSC-CS back-end resource init and cleanup
2023-01-18 23:55 ` Ceraolo Spurio, Daniele
@ 2023-01-20 20:48 ` Teres Alexis, Alan Previn
0 siblings, 0 replies; 27+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-20 20:48 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: justonli, dri-devel
Thanks for reviewing, Daniele. Responses are inline below.
On Wed, 2023-01-18 at 15:55 -0800, Ceraolo Spurio, Daniele wrote:
> >
> >
> > On 1/11/2023 1:42 PM, Alan Previn wrote:
> > > > For MTL, PXP transport back-end uses the GSC engine to submit
> > > > HECI packets for PXP arb session management. The command submission
> > > > that uses non-priveleged mode requires us to allocate (or free)
> > > > a set of execution submission resources (buffer-object, batch-buffer
> > > > and context). Thus, do this one time allocation of resources in
> > > > GSC-CS init and clean them up in fini.
> > > >
> > > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > > > ---
> > > >
Alan: [snip]
> > > > +struct gsccs_session_resources {
> > > > + struct mutex cmd_mutex; /* Protects submission for arb session */
> > > > + u64 host_session_handle; /* used by firmware to link commands to sessions */
> > > > +
> > > > + struct intel_context *ce; /* context for gsc command submission */
> > > > + struct i915_ppgtt *ppgtt; /* ppgtt for gsc command submission */
> >
> > The arb session creation is a kernel submission, so can't you just use
> > the default kernel ppgtt (i.e., gt->vm)?
Alan: yes i can - will remove 'ppgtt'
> >
> > > > +
> > > > + struct drm_i915_gem_object *pkt_obj; /* PXP HECI message packet buffer */
> > > > + struct i915_vma *pkt_vma; /* PXP HECI message packet vma */
> > > > + void *pkt_vaddr; /* PXP HECI message packet virt memory pointer */
> > > > +
> > > > + /* Buffer info for GSC engine batch buffer: */
> > > > + struct drm_i915_gem_object *bb_obj; /* batch buffer object */
> > > > + struct i915_vma *bb_vma; /* batch buffer vma */
> > > > + void *bb_vaddr; /* batch buffer virtual memory pointer */
> >
> > You aren't actually making use of any of the variables in this struct in
> > this patch, apart from initialization. Some of those are pretty clear on
> > what they will be used for (e.g. the context), bu others are a bit more
> > vague(e.g. the vaddrs). It'll probably be cleaner to reorder things so
> > the more implementation-specific variables are added when they're used.
> > I'll try to add more comment in follow-up patches.
> >
Alan: okay - will keep context and introduce the other members of this structure in the patches they
get introduced in (..or not, depending on the subsequent review comments).
> > > > +};
> > > > +
> > > > +struct gsccs_teelink_priv {
> > > > + /** @arb_exec_res: resources for arb-session GSC-CS PXP command submission */
> > > > + struct gsccs_session_resources arb_exec_res;
> > > > +};
> > > > +
> > > > +static inline struct gsccs_teelink_priv *pxp_to_gsccs_priv(struct intel_pxp *pxp)
> > > > +{
> > > > + return (struct gsccs_teelink_priv *)pxp->gsccs_priv;
> > > > +}
> >
> > Why do we need this layer of obfuscation with the void gsccs_priv? it's
> > not like that can be assigned to anything else but
> > gsccs_session_resources, so why not just have a pointer to that?
> > If you want to have different priv based on the backend (mei vs gsccs)
> > than it should be a more generic priv and be used in both cases.
> >
Alan: Agreed. I will just include that into the pxp structure. My original intention was to prepare for
that abstraction of backends (like you mentioned, as per the original RFC) as well as to limit
the build time impact on other pxp files when changing this structure. However given that
the former would be a much bigger effort that ought to begin only after mtl-pxp is fully
completed (as per offline discussions) and merged (for a proper "two-steps-back and absract"),
and this structure really shouldnt change much at all moving forward after this series, i will
re-rev as per your recommendation.
> > > >
> > > > int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
> > > > int arb_session_id)
> > > > @@ -13,11 +45,193 @@ int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
> > > > return -ENODEV;
> > > > }
> > > >
> > > > +static void
> > > > +gsccs_destroy_buffer(struct drm_i915_private *i915, struct i915_vma *vma,
> > > > + struct drm_i915_gem_object *obj)
> > > > +{
> > > > + int err;
> > > > +
> > > > + i915_vma_unpin(vma);
> > > > + err = i915_vma_unbind(vma);
> > > > + if (err)
> > > > + drm_dbg(&i915->drm, "Unexpected failure when vma-unbinding = %d\n", err);
> >
> > I don't think you need to explicitly call unbind here, it should be
> > automatically covered by the object cleanup as long as it is unpinned
Alan: Thanks - I will fix accordingly
> >
> > > > +
> > > > + i915_gem_object_unpin_map(obj);
> > > > + i915_gem_object_unpin_pages(obj);
> > > > + i915_gem_object_put(obj);
> >
> > If you don't explicitly pin the pages (which you don't need to, see
> > comment below) the whole cleanup in this function can be done with just:
> >
> > i915_vma_unpin_and_release(vma, I915_VMA_RELEASE_MAP);
> >
Alan: good catch - i missed that (my code was based off some selftest that
had different goals). Will optimize this accordingly.
> > > > +}
> > > > +
> > > > +static int
> > > > +gsccs_create_buffer(struct drm_i915_private *i915, const char *bufname,
> > > > + size_t size, struct i915_ppgtt *ppgtt,
> >
> > personal preference: IMO better to pass a generic "i915_address_space
> > *vm" to this function.
Alan: sounds good - no prob.
> >
> > > > + struct drm_i915_gem_object **obj,
> > > > + struct i915_vma **vma, void **map)
> > > > +{
> > > > + int err = 0;
> > > > +
> > > > + *obj = i915_gem_object_create_internal(i915, size);
> > > > + if (IS_ERR(*obj)) {
> > > > + drm_err(&i915->drm, "Failed to allocate gsccs backend %s.\n", bufname);
> > > > + err = PTR_ERR(*obj);
> > > > + goto out_none;
> > > > + }
> > > > +
> > > > + *vma = i915_vma_instance(*obj, &ppgtt->vm, NULL);
> > > > + if (IS_ERR(*vma)) {
> > > > + drm_err(&i915->drm, "Failed to vma-instance gsccs backend %s.\n", bufname);
> > > > + err = PTR_ERR(*vma);
> > > > + goto out_put;
> > > > + }
> > > > +
> > > > + err = i915_gem_object_pin_pages_unlocked(*obj);
> > > > + if (err) {
> > > > + drm_err(&i915->drm, "Failed to pin gsccs backend %s.\n", bufname);
> > > > + goto out_put;
> > > > + }
> >
> > You're doing a vma_pin below, so no need to explicitly pin the pages
> > here as the vma_pin will cover it.
> > Also, do you need the object to be returned by the function? As
> > mentioned above, the cleanup can be done with just the vma pointer and
> > from a quick look I don't see the object being used in follow up patches.
> >
Alan: Yup - got it. Yes we don't reference object elsewhere.
> > > > +
> > > > + /* map to the virtual memory pointer */
> > > > + *map = i915_gem_object_pin_map_unlocked(*obj, i915_coherent_map_type(i915, *obj, true));
> > > > + if (IS_ERR(*map)) {
> > > > + drm_err(&i915->drm, "Failed to map gsccs backend %s.\n", bufname);
> > > > + err = PTR_ERR(*map);
> > > > + goto out_unpin;
> > > > + }
> > > >
Alan: [snip]
> > > > +static int
> > > > +gsccs_allocate_execution_resource(struct intel_pxp *pxp,
> > > > + struct gsccs_session_resources *strm_res)
> > > > +{
> > > > + struct intel_gt *gt = pxp->ctrl_gt;
> > > > + struct intel_engine_cs *engine = gt->engine[GSC0];
> > > > + struct i915_ppgtt *ppgtt;
> > > > + struct intel_context *ce;
> > > > + int err = 0;
> > > > +
> > > > + /*
> > > > + * First, ensure the GSC engine is present.
> > > > + * NOTE: Backend should would only be called with the correct gt.
> >
> > typo: should would
Alan: yup
> >
> > > >
Alan: [snip]
> > > > + /*
> > > > + * TODO: Consider optimization of pre-populating batch buffer
> > > > + * with the send-HECI instruction now at init and reuse through its life.
> > > > + */
> >
> > This looks like a nice optimization, and it would also allow us to drop
> > the bb_vaddr variable from the struct. If the only heci message we're
> > ever going to send with this object is the session creation, we can
> > probably also fill the "send" section of the pkt.
> >
Alan: unfortunately, i have to remove that comment because we will be seeing the session-teardown coming up.
As per the other series we are reviewing for ADL, i'll have to implement the same for MTL - but i want that
to get reviewed and get the RB's first before i update this series to include it.
> > > > +
> > > > + /* Finally, create an intel_context to be used during the submission */
> > > > + ce = intel_context_create(engine);
> > > > + if (IS_ERR(ce)) {
> > > > + drm_err(>->i915->drm, "Failed creating gsccs backend ctx\n");
> > > > + gsccs_destroy_execution_resource(pxp, strm_res);
> >
> > we usually prefer onion unwind to calling the full destroy function from
> > the create one. Not a blocker.
> >
> > Daniele
> >
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/9] drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation
2023-01-19 0:09 ` Ceraolo Spurio, Daniele
@ 2023-01-20 20:49 ` Teres Alexis, Alan Previn
0 siblings, 0 replies; 27+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-20 20:49 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: justonli, dri-devel
Thanks for reviewing.
On Wed, 2023-01-18 at 16:09 -0800, Ceraolo Spurio, Daniele wrote:
> >
> >
Alan: [snip]
> > > > -/* KCR register definitions */
> > > > -#define KCR_INIT _MMIO(0x320f0)
> > > > -/* Setting KCR Init bit is required after system boot */
> > > > -#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES REG_BIT(14)
> > > > +static i915_reg_t get_kcr_reg(const struct intel_pxp *pxp)
> > > > +{
> > > > + if (pxp->gsccs_priv)
> >
> > IMO here a better check would be:
> >
> > if (pxp->ctrl_gt->type == GT_MEDIA)
> >
> > It's equivalent, but IMO from a readability perspective it highlights
> > the fact that this is a change due to us moving to the media GT model
> > and has nothing to do with the GSC CS itself.
> >
Alan: Yes agreed - in alignment with to your next comment, this readiblity
improvement shall therefore go into pxp_init (when we initialize value for kcr_base offset)
Alan: [snip]
> > > > +#ifndef __INTEL_PXP_REGS_H__
> > > > +#define __INTEL_PXP_REGS_H__
> > > > +
> > > > +#include "i915_reg_defs.h"
> > > > +
> > > > +/* KCR enable/disable control */
> > > > +#define GEN12_KCR_INIT _MMIO(0x320f0)
> > > > +#define MTL_KCR_INIT _MMIO(0x3860f0)
> > > > +
> > > > +/* Setting KCR Init bit is required after system boot */
> > > > +#define KCR_INIT_ALLOW_DISPLAY_ME_WRITES REG_BIT(14)
> > > > +
> > > > +/* KCR hwdrm session in play status 0-31 */
> > > > +#define GEN12_KCR_SIP _MMIO(0x32260)
> > > > +#define MTL_KCR_SIP _MMIO(0x386260)
> > > > +
> > > > +/* PXP global terminate register for session termination */
> > > > +#define GEN12_KCR_GLOBAL_TERMINATE _MMIO(0x320f8)
> > > > +#define MTL_KCR_GLOBAL_TERMINATE _MMIO(0x3860f8)
> >
> > Non blocking suggestion:
> > it looks like all KCR regs are at the same relative offsets, just from a
> > different base (which makes, sense, because the HW just got moved to the
> > media tile as-is). So we could possibly have something like what we do
> > for the engines:
> >
> > #define GEN12_KCR_BASE 0x32000
> > #define MTL_KCR_BASE 0x386000
> >
> > #define KCR_INIT(base) _MMIO(base + 0xf0)
> > #define KCR_GLOBAL_TERMINATE(base) _MMIO(base + 0xf8)
> > #define KCR_SIP(base) _MMIO(base + 0x260)
> >
> > Then if we store the correct base in the pxp structure we can just pass
> > it in the define when we need to access a register and remove the if
> > conditions at each access.
> >
Alan: sounds good - will do this.
Alan: [snip]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 4/9] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC
2023-01-19 1:28 ` Ceraolo Spurio, Daniele
@ 2023-01-20 20:58 ` Teres Alexis, Alan Previn
0 siblings, 0 replies; 27+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-20 20:58 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: justonli, dri-devel
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
> >
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 5/9] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages
2023-01-19 1:40 ` Ceraolo Spurio, Daniele
@ 2023-01-20 23:46 ` Teres Alexis, Alan Previn
2023-01-21 0:08 ` [Intel-gfx] " Teres Alexis, Alan Previn
0 siblings, 1 reply; 27+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-20 23:46 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: justonli, dri-devel
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@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(>->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)
> > {
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Intel-gfx] [PATCH v2 5/9] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages
2023-01-20 23:46 ` Teres Alexis, Alan Previn
@ 2023-01-21 0:08 ` Teres Alexis, Alan Previn
0 siblings, 0 replies; 27+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-21 0:08 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: dri-devel
On Fri, 2023-01-20 at 23:46 +0000, Teres Alexis, Alan Previn wrote:
> Thanks for the review comment...
>
Replying here with the summary of our offline chat:
So we concluded that for the next rev, lets move the
buffer/vma allocations from patch #2 into this patch
#5-send-message. But for now we shall keep patch
#6-create-session as separate because when future
terminate-session joins this series, it will join
#6. That said, #5 and #6 togther would then appear
rather large. However, we both agreed that we can
always make the final call to squash all of those
together (alloc/free + send-message + create-session +
terminate-session) at the end when we are merging if
we see fit. That would be easy and painless (as opposed to
squashing now and then getting a request to re-split
later). NOTE: all numbering above will appear as n-minus-1
on next rev because Patch-#1 is going away (the stubs).
thanks again...alan
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 7/9] drm/i915/pxp: MTL-KCR interrupt ctrl's are in GT-0
2023-01-19 2:01 ` Ceraolo Spurio, Daniele
@ 2023-01-23 21:11 ` Teres Alexis, Alan Previn
0 siblings, 0 replies; 27+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-23 21:11 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: justonli, dri-devel
On Wed, 2023-01-18 at 18:01 -0800, Ceraolo Spurio, Daniele wrote:
>
>
> On 1/11/2023 1:42 PM, Alan Previn wrote:
> > Despite KCR subsystem being in the media-tile (close to the
> > GSC-CS), the IRQ controls for it are on GT-0 with other global
> > IRQ controls. Thus, add a helper for KCR hw interrupt
> > enable/disable functions to get the correct gt structure (for
> > uncore) for MTL.
> >
> > In the helper, we get GT-0's handle for uncore when touching
> > IRQ registers despite the pxp->ctrl_gt being the media-tile.
> > No difference for legacy of course.
> >
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > ---
> > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 2 +-
> > drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 23 +++++++++++++++++---
> > drivers/gpu/drm/i915/pxp/intel_pxp_irq.h | 8 +++++++
> > 3 files changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > index 4b8e70caa3ad..9f6e300486b4 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
> > @@ -44,7 +44,7 @@ static int pxp_terminate_get(void *data, u64 *val)
> > static int pxp_terminate_set(void *data, u64 val)
> > {
> > struct intel_pxp *pxp = data;
> > - struct intel_gt *gt = pxp->ctrl_gt;
> > + struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
>
> This doesn't seem to be required here. The only use of gt in this
> function is an assert below and both the root and media gt point to the
> same irq_lock, so it doesn't matter which one we're using. Should we
> keep it anyway just for safety in case things change in the future? or
> just add a comment instead?
>
I rather we keep this helper for consistency: everything else in front-end
pxp code is using pxp->ctrl_gt, but if we use pxp->[root_gt] here, it would
just read like a bug without understanding the hw details.
As you have pointed out farther down, the helper could just return root gt
always (since they both point to the same IRQ register). I will go ahead and
follow your recommendation for the helper internals (with the comments
explaining in detail) but have the callers continue to use that helper.
> >
> > if (!intel_pxp_is_active(pxp))
> > return -ENODEV;
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > index 91e9622c07d0..2eef0c19e91a 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.c
> > @@ -8,6 +8,7 @@
> > #include "gt/intel_gt_regs.h"
> > #include "gt/intel_gt_types.h"
> >
> > +#include "i915_drv.h"
> > #include "i915_irq.h"
> > #include "i915_reg.h"
> >
> > @@ -17,6 +18,22 @@
> > #include "intel_pxp_types.h"
> > #include "intel_runtime_pm.h"
> >
> > +/**
> > + * intel_pxp_get_irq_gt - Find the correct GT that owns KCR interrupts
> > + * @pxp: pointer to pxp struct
> > + *
> > + * For platforms with a single GT, we return the pxp->ctrl_gt (as expected)
> > + * but for MTL+ that has a media-tile, although the KCR engine is in the
> > + * media-tile (i.e. pxp->ctrl_gt), the IRQ controls are on the root tile.
> > + */
> > +struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp)
> > +{
> > + if (pxp->uses_gsccs)
> > + return to_gt(pxp->ctrl_gt->i915);
> > +
> > + return pxp->ctrl_gt;
>
> AFAICT here we can skip the if and always return the root gt, because
> that's what happens in both cases. If you want to make sure we don't get
> issues in the future maybe instead add a:
>
> GEM_BUG_ON(!i915->media_gt && !gt_is_root(pxp->ctrl_gt))
will do.
>
> > +}
> > +
> > /**
> > * intel_pxp_irq_handler - Handles PXP interrupts.
> > * @pxp: pointer to pxp struct
> > @@ -29,7 +46,7 @@ void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> > if (GEM_WARN_ON(!intel_pxp_is_enabled(pxp)))
> > return;
> >
> > - gt = pxp->ctrl_gt;
> > + gt = intel_pxp_get_irq_gt(pxp);
>
> Here also we only have the assert below
i will follow the above recommendation.
>
> Daniele
>
> >
> > lockdep_assert_held(gt->irq_lock);
> >
> > @@ -68,7 +85,7 @@ static inline void pxp_irq_reset(struct intel_gt *gt)
> >
> > void intel_pxp_irq_enable(struct intel_pxp *pxp)
> > {
> > - struct intel_gt *gt = pxp->ctrl_gt;
> > + struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
> >
> > spin_lock_irq(gt->irq_lock);
> >
> > @@ -83,7 +100,7 @@ void intel_pxp_irq_enable(struct intel_pxp *pxp)
> >
> > void intel_pxp_irq_disable(struct intel_pxp *pxp)
> > {
> > - struct intel_gt *gt = pxp->ctrl_gt;
> > + struct intel_gt *gt = intel_pxp_get_irq_gt(pxp);
> >
> > /*
> > * We always need to submit a global termination when we re-enable the
> > diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
> > index 8c292dc86f68..eea87c9eb62b 100644
> > --- a/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_irq.h
> > @@ -9,6 +9,7 @@
> > #include <linux/types.h>
> >
> > struct intel_pxp;
> > +struct intel_gt;
> >
> > #define GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT BIT(1)
> > #define GEN12_DISPLAY_APP_TERMINATED_PER_FW_REQ_INTERRUPT BIT(2)
> > @@ -23,6 +24,8 @@ struct intel_pxp;
> > void intel_pxp_irq_enable(struct intel_pxp *pxp);
> > void intel_pxp_irq_disable(struct intel_pxp *pxp);
> > void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir);
> > +struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp);
> > +
> > #else
> > static inline void intel_pxp_irq_handler(struct intel_pxp *pxp, u16 iir)
> > {
> > @@ -35,6 +38,11 @@ static inline void intel_pxp_irq_enable(struct intel_pxp *pxp)
> > static inline void intel_pxp_irq_disable(struct intel_pxp *pxp)
> > {
> > }
> > +
> > +static inline struct intel_gt *intel_pxp_get_irq_gt(struct intel_pxp *pxp)
> > +{
> > + return NULL;
> > +}
> > #endif
> >
> > #endif /* __INTEL_PXP_IRQ_H__ */
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 8/9] drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component
2023-01-19 2:17 ` Ceraolo Spurio, Daniele
@ 2023-01-24 0:15 ` Teres Alexis, Alan Previn
0 siblings, 0 replies; 27+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-24 0:15 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: justonli, dri-devel
Thanks - will go with your suggestion - ditch all the abstraction / consolidation ..
add only the two additional calls with the explicit runtime-takes - that minimizes the changes.
And won't forget to the fix the '&' -> '&&'
...alan
On Wed, 2023-01-18 at 18:17 -0800, Ceraolo Spurio, Daniele wrote:
>
>
> On 1/11/2023 1:42 PM, Alan Previn wrote:
> > On legacy platforms, KCR HW enabling is done at the time the mei
> > component interface is bound. It's also disabled during unbind.
> > However, for MTL onwards, we don't depend on the tee-component
> > operation before we can start sending GSC-CS firmware messages.
> >
> > Thus, immediately enable KCR HW on PXP's init, fini and resume.
alan:snip..
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 6/9] drm/i915/pxp: Add ARB session creation with new PXP API Ver4.3
2023-01-19 1:50 ` Ceraolo Spurio, Daniele
@ 2023-01-25 4:13 ` Teres Alexis, Alan Previn
0 siblings, 0 replies; 27+ messages in thread
From: Teres Alexis, Alan Previn @ 2023-01-25 4:13 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: justonli, dri-devel
Thanks Daniele - agreed on all counts - will fix accordingly - already done and tested internally - will re-rev shortly.
..alan
On Wed, 2023-01-18 at 17:50 -0800, Ceraolo Spurio, Daniele wrote:
>
>
> On 1/11/2023 1:42 PM, Alan Previn wrote:
> > Add MTL's function for ARB session creation using PXP firmware
> > version 4.3 ABI structure format.
> >
> > Before checking the return status, look at the GSC-CS-Mem-Header's
> > pending-bit which means the GSC firmware is busy and we should
> > resubmit.
> >
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > ---
> > .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 21 +++++++
> > drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 56 ++++++++++++++++++-
> > 2 files changed, 74 insertions(+), 3 deletions(-)
alan:snip..
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2023-01-25 4:13 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-11 21:42 [PATCH v2 0/9] drm/i915/pxp: Add MTL PXP Support Alan Previn
2023-01-11 21:42 ` [PATCH v2 1/9] drm/i915/pxp: Add MTL PXP GSC-CS back-end skeleton Alan Previn
2023-01-18 17:51 ` Ceraolo Spurio, Daniele
2023-01-20 0:20 ` Teres Alexis, Alan Previn
2023-01-11 21:42 ` [PATCH v2 2/9] drm/i915/pxp: Add GSC-CS back-end resource init and cleanup Alan Previn
2023-01-18 23:55 ` Ceraolo Spurio, Daniele
2023-01-20 20:48 ` Teres Alexis, Alan Previn
2023-01-11 21:42 ` [PATCH v2 3/9] drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation Alan Previn
2023-01-19 0:09 ` Ceraolo Spurio, Daniele
2023-01-20 20:49 ` Teres Alexis, Alan Previn
2023-01-11 21:42 ` [PATCH v2 4/9] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC Alan Previn
2023-01-19 1:28 ` Ceraolo Spurio, Daniele
2023-01-20 20:58 ` Teres Alexis, Alan Previn
2023-01-11 21:42 ` [PATCH v2 5/9] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages Alan Previn
2023-01-19 1:40 ` Ceraolo Spurio, Daniele
2023-01-20 23:46 ` Teres Alexis, Alan Previn
2023-01-21 0:08 ` [Intel-gfx] " Teres Alexis, Alan Previn
2023-01-11 21:42 ` [PATCH v2 6/9] drm/i915/pxp: Add ARB session creation with new PXP API Ver4.3 Alan Previn
2023-01-19 1:50 ` Ceraolo Spurio, Daniele
2023-01-25 4:13 ` Teres Alexis, Alan Previn
2023-01-11 21:42 ` [PATCH v2 7/9] drm/i915/pxp: MTL-KCR interrupt ctrl's are in GT-0 Alan Previn
2023-01-19 2:01 ` Ceraolo Spurio, Daniele
2023-01-23 21:11 ` Teres Alexis, Alan Previn
2023-01-11 21:42 ` [PATCH v2 8/9] drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component Alan Previn
2023-01-19 2:17 ` Ceraolo Spurio, Daniele
2023-01-24 0:15 ` Teres Alexis, Alan Previn
2023-01-11 21:42 ` [PATCH v2 9/9] drm/i915/pxp: Enable PXP with MTL-GSC-CS Alan Previn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).