[Intel-gfx] [PATCH v7 0/8] drm/i915/pxp: Add MTL PXP Support

[Intel-gfx] [PATCH v7 0/8] drm/i915/pxp: Add MTL PXP Support

[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 #3. 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 #5.

Take note that Patch #4 adds the buffer allocation and gsccs-send-
message without actually being called by the arb-creation code
which gets added in Patch #5. Additionally, a seperate series being
reviewed is introducing a change for session teardown (in pxp front-
end layer that will need backend support by both legacy and gsccs).
If we squash all of these together (buffer-alloc, send-message,
arb-creation and, in future, session-termination), the single patch
will be rather large. That said, we are keeping Patch #4 and #5
separate for now, but at merge time, we can squash them together
if maintainer requires it.

Changes from prior revs:
   v1 : - fixed when building with CONFIG_PXP disabled.
        - more alignment with gsc_mtl_header structure from the HDCP
   v2 : - (all following changes as per reviews from Daniele)
        - squashed Patch #1 from v1 into the next one.
        - replaced unnecessary "uses_gsccs" boolean in the pxp
          with "HAS_ENGINE(pxp->ctrl_gt, GSC0)".
        - moved the stashing of gsccs resources from a dynamically
          allocated opaque handle to an explicit sub-struct in
          'struct intel_pxp'.
        - moved the buffer object allocations from Patch #1 of this
          series to Patch #5 (but keep the context allocation in
          Patch #1).
        - used the kernel default ppgtt for the gsccs context.
        - optimized the buffer allocation and deallocation code
          and drop the need to stash the drm_i915_gem_object.
        - use a macro with the right mmio reg base (depending
          on root-tile vs media-tile) along with common relative
          offset to access all KCR registers thus minimizing
          changes to the KCR register access codes.
        - fixed bugs in the heci packet request submission code
          in Patch #3 (of this series)
        - add comments in the mtl-gsc-heci-header regarding the
          host-session-handle.
        - re-use tee-mutex instead of introducing a gsccs specific
          cmd mutex.
        - minor cosmetic improvements in Patch #5.
	- before creating arb session, ensure intel_pxp_start
          first ensures the GSC FW is up and running.
        - use 2 second timeout for the pending-bit scenario when
          sending command to GSC-FW as per specs.
        - simplify intel_pxp_get_irq_gt with addition comments
        - redo Patch #7 to minimize the changes without introducing
          a common  abstraction helper for suspend/resume/init/fini
          codes that have to change the kcr power state.
   v3 : - rebase onto latest drm-tip with the updated firmware
          session invalidation flow
        - on Patch#1: move 'mutex_init(&pxp->tee_mutex)' to a common
          init place in intel_pxp_init since its needed everywhere
          (Daniele)
        - on Patch#1: remove unneccasary "ce->vm = i915_vm_get(vm);"
          (Daniele)
        - on Patch#2: move the introduction of host_session_handle to
          Patch#4 where it starts getting used.
        - on Patch#4: move host-session-handle initialization to the
          allocate-resources during gsccs-init (away from Patch#5)
          and add the required call to PXP-firmware to cleanup the
          host-session-handle in destroy-resources during gsccs-fini
        - on Patch#5: add dispatching of arb session termination
          firmware cmd during session teardown (as per latest
          upstream flows)
   v4 : - Added proper initialization and cleanup of host-session-handle
          that the gsc firmware expects.
        - Fix the stream-session-key invalidation instruction for MTL.
   v5 : - In Patch #4, move the tee_mutex locking to after we check for
          valid vma allocations.
        - In Patch #5, wait for intel_huc_is_authenticated instead of
          intel_uc_fw_is_running(gsc-fw) before starting ARB session.
        - In Patch #5, increase the necessary timeouts at the top-level
          (PXP arb session creation / termination) to accomodate SLA of
          GSC firmware when busy with pending-bit responses.
        - In Patch #5, remove redundant host_session_handle init as
          we need a single handle that is already initialized during
          execution_resource init in Patch #4.
        - In Patch #8, increase the wait timeout for termination to
          align with the same SLA.
   v6 : - (multiple patches) always name variables of type struct
          gsccs_session_resources * as 'exec_res'. (Daniele).
        - In gsccs_allocate_execution_resource, always put and take the
          contexts vm to enforce its the default pxp->ctrl_gt->vm.
          (Daniele)
        - In Patch #3: Rebase with the upstream-merged version of the
          intel_gsc_uc_heci_cmd_submit.* files that was part of the hdcp
          merge (adding only the difference of the non-priv submision).
          Fix the non-priv submission helper to use the ww-aware versions
          of request creation + submission (some re-ordeing and calling
          i915_gem_ww_ctx_init and intel_context_pin_ww). (Alan)
        - In Patch #4: Misc coding styling improvements (Daniele).
          Replace PXP43_MAX_HECI_IN_SIZE and PXP43_MAX_HECI_OUT_SIZE
          with PXP43_MAX_HECI_INOUT_SIZE to simplify the size checking.
          Clear the MTL-GSC-HECI header's validity marker of the output
          packet before submission to avoid stale values (Daniele).
          Fix a bug with the gsccs_allocate_execution_resource error
          condition bailing out when out of mem (Daniele).
        - In Patch #5: Add intel_gsc_uc_fw_proxy_init_done when starting
          arb session (in front-end code) when called on platforms with GSC
          engine (Daniele). Update the fw-response-error reporting to
          match what is being merged upstream for the ADL case (Daniele).
        - Old Patch #6: Remove this patch completely. We don't need to
          use the root-gt's uncore to handle KCR irq / power registers.
          (Daniele).
        - New Patch #6: Update the documentation in i915 UAPI for PXP
          context creation to include additional error meanings that was
          always there in upstream code but just never documented. (Alan).
          Add a GET_PARAM for PXP support so that UMDs won't have to create
          a PXP context to know if it's available since that method would
          suffer a longer delay if called too early in kernel startup
          because of the additional dependencies on devices with GSC engine.
          (Tvrtko, Alan, Rodrigo, Lionel).
        - Patch #7: Move intel_pxp_init_hw into backend code to be
          consistent with legacy mei-pxp based tee transport. (Daniele).
        - Patch #8: With 3 places now being aware of mei-pxp vs gsccs-pxp
          difference in timeouts for round trip delays when communicating
          with pxp firmware (getting fw commands sent and receiving the
          reply), add a helper for this. (Daniele)

Alan Previn (8):
  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 and cleanup
  drm/i915/uapi/pxp: Fix UAPI spec comments and add GET_PARAM for PXP
  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                 |   1 +
 drivers/gpu/drm/i915/gt/intel_gt_irq.c        |   3 +-
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c     |  10 +
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h     |   1 +
 .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c | 102 ++++
 .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h |  26 +-
 drivers/gpu/drm/i915/i915_getparam.c          |   5 +
 drivers/gpu/drm/i915/i915_pci.c               |   1 +
 drivers/gpu/drm/i915/pxp/intel_pxp.c          |  82 +++-
 drivers/gpu/drm/i915/pxp/intel_pxp.h          |   1 +
 .../drm/i915/pxp/intel_pxp_cmd_interface_43.h |  24 +
 drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c  |   6 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c    | 444 ++++++++++++++++++
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h    |  39 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c       |   3 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_regs.h     |  27 ++
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c  |  25 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c      |   2 -
 drivers/gpu/drm/i915/pxp/intel_pxp_types.h    |  20 +
 include/uapi/drm/i915_drm.h                   |  22 +
 20 files changed, 806 insertions(+), 38 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
 create mode 100644 drivers/gpu/drm/i915/pxp/intel_pxp_regs.h


base-commit: fbadfcf137737f02425a35bf3ae17a1492301f21
-- 
2.39.0

[Intel-gfx] [PATCH v7 1/8] drm/i915/pxp: Add GSC-CS back-end resource init and cleanup

[Alan Previn]

For MTL, the PXP back-end transport uses the GSC engine to submit
HECI packets through the HW to the GSC firmware for PXP arb
session management. This submission uses a non-priveleged
batch buffer, a buffer for the command packet and of course
a context targeting the GSC-CS.

Thus for MTL, we need to allocate and free a set of execution
submission resources for the management of the arbitration session.
Lets start with the context creation first since that object and
its usage is very straight-forward. We'll add the buffer allocation
and freeing later when we introduce the gsccs' send-message function.

Do this one time allocation of gsccs specific resources in
a new gsccs source file with intel_pxp_gsccs_init / fini functions
and hook them up from the PXP front-end.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/Makefile              |  1 +
 drivers/gpu/drm/i915/pxp/intel_pxp.c       | 20 +++++--
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 63 ++++++++++++++++++++++
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h | 29 ++++++++++
 drivers/gpu/drm/i915/pxp/intel_pxp_tee.c   |  2 -
 drivers/gpu/drm/i915/pxp/intel_pxp_types.h |  8 +++
 6 files changed, 117 insertions(+), 6 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 97b0d4ae221a..5df177f9afd1 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -338,6 +338,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 9d4c7724e98e..f93aa171aa1e 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 (HAS_ENGINE(pxp->ctrl_gt, GSC0))
+		ret = intel_pxp_gsccs_init(pxp);
+	else
+		ret = intel_pxp_tee_component_init(pxp);
 	if (ret)
 		goto out_context;
 
@@ -165,9 +169,12 @@ static struct intel_gt *find_gt_for_required_protected_content(struct drm_i915_p
 	/*
 	 * For MTL onwards, PXP-controller-GT needs to have a valid GSC engine
 	 * on the media GT. NOTE: if we have a media-tile with a GSC-engine,
-	 * the VDBOX is already present so skip that check
+	 * the VDBOX is already present so skip that check. We also have to
+	 * ensure the GSC and HUC firmware are coming online
 	 */
-	if (i915->media_gt && HAS_ENGINE(i915->media_gt, GSC0))
+	if (i915->media_gt && HAS_ENGINE(i915->media_gt, GSC0) &&
+	    intel_uc_fw_is_loadable(&i915->media_gt->uc.gsc.fw) &&
+	    intel_uc_fw_is_loadable(&i915->media_gt->uc.huc.fw))
 		return i915->media_gt;
 
 	/*
@@ -207,7 +214,9 @@ int intel_pxp_init(struct drm_i915_private *i915)
 	if (!i915->pxp)
 		return -ENOMEM;
 
+	/* init common info used by all feature-mode usages*/
 	i915->pxp->ctrl_gt = gt;
+	mutex_init(&i915->pxp->tee_mutex);
 
 	/*
 	 * If full PXP feature is not available but HuC is loaded by GSC on pre-MTL
@@ -229,7 +238,10 @@ void intel_pxp_fini(struct drm_i915_private *i915)
 
 	i915->pxp->arb_is_valid = false;
 
-	intel_pxp_tee_component_fini(i915->pxp);
+	if (HAS_ENGINE(i915->pxp->ctrl_gt, GSC0))
+		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..bad55719a7ac
--- /dev/null
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
@@ -0,0 +1,63 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright(c) 2023 Intel Corporation.
+ */
+
+#include "gem/i915_gem_internal.h"
+
+#include "gt/intel_context.h"
+
+#include "i915_drv.h"
+#include "intel_pxp_cmd_interface_43.h"
+#include "intel_pxp_gsccs.h"
+#include "intel_pxp_types.h"
+
+static void
+gsccs_destroy_execution_resource(struct intel_pxp *pxp)
+{
+	struct gsccs_session_resources *exec_res = &pxp->gsccs_res;
+
+	if (exec_res->ce)
+		intel_context_put(exec_res->ce);
+
+	memset(exec_res, 0, sizeof(*exec_res));
+}
+
+static int
+gsccs_allocate_execution_resource(struct intel_pxp *pxp)
+{
+	struct intel_gt *gt = pxp->ctrl_gt;
+	struct gsccs_session_resources *exec_res = &pxp->gsccs_res;
+	struct intel_engine_cs *engine = gt->engine[GSC0];
+	struct intel_context *ce;
+
+	/*
+	 * First, ensure the GSC engine is present.
+	 * NOTE: Backend would only be called with the correct gt.
+	 */
+	if (!engine)
+		return -ENODEV;
+
+	/* Finally, create an intel_context to be used during the submission */
+	ce = intel_context_create(engine);
+	if (IS_ERR(ce)) {
+		drm_err(&gt->i915->drm, "Failed creating gsccs backend ctx\n");
+		return PTR_ERR(ce);
+	}
+
+	i915_vm_put(ce->vm);
+	ce->vm = i915_vm_get(pxp->ctrl_gt->vm);
+	exec_res->ce = ce;
+
+	return 0;
+}
+
+void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
+{
+	gsccs_destroy_execution_resource(pxp);
+}
+
+int intel_pxp_gsccs_init(struct intel_pxp *pxp)
+{
+	return gsccs_allocate_execution_resource(pxp);
+}
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..354ea9a8f940
--- /dev/null
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
@@ -0,0 +1,29 @@
+/* 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
+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_tee.c b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
index a2846b1dbbee..1ce07d7e8769 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_tee.c
@@ -284,8 +284,6 @@ int intel_pxp_tee_component_init(struct intel_pxp *pxp)
 	struct intel_gt *gt = pxp->ctrl_gt;
 	struct drm_i915_private *i915 = gt->i915;
 
-	mutex_init(&pxp->tee_mutex);
-
 	ret = alloc_streaming_command(pxp);
 	if (ret)
 		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 007de49e1ea4..1e01036d0455 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
@@ -26,6 +26,14 @@ struct intel_pxp {
 	 */
 	struct intel_gt *ctrl_gt;
 
+	/**
+	 * @gsccs_res: resources for request submission for platforms that have a GSC engine.
+	 */
+	struct gsccs_session_resources {
+		u64 host_session_handle; /* used by firmware to link commands to sessions */
+		struct intel_context *ce; /* context for gsc command submission */
+	} gsccs_res;
+
 	/**
 	 * @pxp_component: i915_pxp_component struct of the bound mei_pxp
 	 * module. Only set and cleared inside component bind/unbind functions,
-- 
2.39.0

[Intel-gfx] [PATCH v7 2/8] drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation

[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>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_irq.c       |  3 +-
 drivers/gpu/drm/i915/pxp/intel_pxp.c         | 32 ++++++++++++--------
 drivers/gpu/drm/i915/pxp/intel_pxp_regs.h    | 27 +++++++++++++++++
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c | 12 +++-----
 drivers/gpu/drm/i915/pxp/intel_pxp_types.h   |  6 ++++
 5 files changed, 58 insertions(+), 22 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 1b25a6039152..c360776a98b5 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 f93aa171aa1e..8949d4be7882 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,22 @@ 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 void kcr_pxp_set_status(const struct intel_pxp *pxp, bool enable)
+{
+	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, KCR_INIT(pxp->kcr_base), val);
+}
 
-static void kcr_pxp_enable(struct intel_gt *gt)
+static void kcr_pxp_enable(const struct intel_pxp *pxp)
 {
-	intel_uncore_write(gt->uncore, KCR_INIT,
-			   _MASKED_BIT_ENABLE(KCR_INIT_ALLOW_DISPLAY_ME_WRITES));
+	kcr_pxp_set_status(pxp, true);
 }
 
-static void kcr_pxp_disable(struct intel_gt *gt)
+static void kcr_pxp_disable(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, false);
 }
 
 static int create_vcs_context(struct intel_pxp *pxp)
@@ -127,6 +129,11 @@ static void pxp_init_full(struct intel_pxp *pxp)
 	init_completion(&pxp->termination);
 	complete_all(&pxp->termination);
 
+	if (pxp->ctrl_gt->type == GT_MEDIA)
+		pxp->kcr_base = MTL_KCR_BASE;
+	else
+		pxp->kcr_base = GEN12_KCR_BASE;
+
 	intel_pxp_session_management_init(pxp);
 
 	ret = create_vcs_context(pxp);
@@ -369,14 +376,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..a9e7e6efa4c7
--- /dev/null
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_regs.h
@@ -0,0 +1,27 @@
+/* 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 subsystem register base address */
+#define GEN12_KCR_BASE 0x32000
+#define MTL_KCR_BASE 0x386000
+
+/* KCR enable/disable control */
+#define KCR_INIT(base) _MMIO((base) + 0xf0)
+
+/* 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 KCR_SIP(base) _MMIO((base) + 0x260)
+
+/* PXP global terminate register for session termination */
+#define KCR_GLOBAL_TERMINATE(base) _MMIO((base) + 0xf8)
+
+#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 7de849cb6c47..7899079e17b0 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -10,14 +10,10 @@
 #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;
@@ -26,7 +22,7 @@ static bool intel_pxp_session_is_in_play(struct intel_pxp *pxp, u32 id)
 
 	/* 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);
+		sip = intel_uncore_read(uncore, KCR_SIP(pxp->kcr_base));
 
 	return sip & BIT(id);
 }
@@ -44,7 +40,7 @@ static int pxp_wait_for_session_state(struct intel_pxp *pxp, u32 id, bool in_pla
 		return in_play ? -ENODEV : 0;
 
 	ret = intel_wait_for_register(uncore,
-				      GEN12_KCR_SIP,
+				      KCR_SIP(pxp->kcr_base),
 				      mask,
 				      in_play ? mask : 0,
 				      100);
@@ -108,7 +104,7 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
 		return ret;
 	}
 
-	intel_uncore_write(gt->uncore, PXP_GLOBAL_TERMINATE, 1);
+	intel_uncore_write(gt->uncore, KCR_GLOBAL_TERMINATE(pxp->kcr_base), 1);
 
 	intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION);
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
index 1e01036d0455..fdd98911968d 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;
 
+	/**
+	 * @kcr_base: base mmio offset for the KCR engine which is different on legacy platforms
+	 * vs newer platforms where the KCR is inside the media-tile.
+	 */
+	u32 kcr_base;
+
 	/**
 	 * @gsccs_res: resources for request submission for platforms that have a GSC engine.
 	 */
-- 
2.39.0

[Intel-gfx] [PATCH v7 3/8] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC

[Alan Previn]

Add helper functions into a new file for heci-packet-submission.
The helpers will handle generating the MTL GSC-CS Memory-Header
and submission of the Heci-Cmd-Packet instructions to the engine.

NOTE1: These common functions for heci-packet-submission will be used
by different i915 callers:
     1- GSC-SW-Proxy: This is pending upstream publication awaiting
        a few remaining opens
     2- MTL-HDCP: An equivalent patch has also been published at:
        https://patchwork.freedesktop.org/series/111876/. (Patch 1)
     3- PXP: This series.

NOTE2: A difference in this patch vs what is appearing is in bullet 2
above is that HDCP (and SW-Proxy) will be using priveleged submission
(GGTT and common gsc-uc-context) while PXP will be using non-priveleged
PPGTT, context and batch buffer. Therefore this patch will only slightly
overlap with the MTL-HDCP patches despite have very similar function
names (emit_foo vs emit_nonpriv_foo). This is because HECI_CMD_PKT
instructions require different flows and hw-specific code when done
via PPGTT based submission (not different from other engines). MTL-HDCP
contains the same intel_gsc_mtl_header_t structures as this but the
helpers there are different. Both add the same new file names.

NOTE3: Additional clarity about the heci-cmd-pkt layout and where the
       common helpers come in:
     - On MTL, when an i915 subsystem needs to send a command request
       to the security firmware, it will send that via the GSC-
       engine-command-streamer.
     - However those commands, (lets call them "gsc_specific_fw_api"
       calls), are not understood by the GSC command streamer hw.
     - The GSC CS only looks at the GSC_HECI_CMD_PKT instruction and
       passes it along to the GSC firmware.
     - The GSC FW on the other hand needs additional metadata to know
       which usage service is being called (PXP, HDCP, proxy, etc) along
       with session specific info. Thus an extra header called GSC-CS
       HECI Memory Header, (C) in below diagram is prepended before
       the FW specific API, (D).
     - Thus, the structural layout of the request submitted would
       need to look like the diagram below (for non-priv PXP).
     - In the diagram, the common helper for HDCP, (GSC-Sw-Proxy) and
       PXP (i.e. new function intel_gsc_uc_heci_cmd_emit_mtl_header)
       will populate blob (C) while additional helpers, different for
       PPGGTT (this patch) vs GGTT (HDCP series) will populate
       blobs (A) and (B) below.
      ___________________________________________________________
 (A)  |  MI_BATCH_BUFFER_START (ppgtt, batchbuff-addr, ...)     |
      |     |                                                   |
      |    _|________________________________________________   |
      | (B)| GSC_HECI_CMD_PKT (pkt-addr-in, pkt-size-in,    |   |
      |    |                   pkt-addr-out, pkt-size-out)  |--------
      |    | MI_BATCH_BUFFER_END                            |   |   |
      |    |________________________________________________|   |   |
      |                                                         |   |
      |_________________________________________________________|   |
                                                                    |
            ---------------------------------------------------------
            |
           \|/
      ______V___________________________________________
      |   _________________________________________    |
      |(C)|                                       |    |
      |   | struct intel_gsc_mtl_header {         |    |
      |   |   validity marker                     |    |
      |   |   heci_clent_id                       |    |
      |   |   ...                                 |    |
      |   |  }                                    |    |
      |   |_______________________________________|    |
      |(D)|                                       |    |
      |   | struct gsc_fw_specific_api_foobar {   |    |
      |   |     ...                               |    |
      |   |     For an example, see               |    |
      |   |     'struct pxp43_create_arb_in' at   |    |
      |   |     intel_pxp_cmd_interface_43.h      |    |
      |   |                                       |    |
      |   | }                                     |    |
      |   |  Struture depends on command type     |    |
      |   | struct gsc_fw_specific_api_foobar {   |    |
      |   |_______________________________________|    |
      |________________________________________________|

That said, this patch provides basic helpers but leaves the
PXP subsystem (i.e. the caller) to handle (D) and everything
else such as input/output size verification or handling the
responses from security firmware (for example, requiring a retry).

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c | 102 ++++++++++++++++++
 .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h |  25 ++++-
 2 files changed, 126 insertions(+), 1 deletion(-)

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
index ea0da06e2f39..12c2a0e1dd1e 100644
--- 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
@@ -3,6 +3,7 @@
  * Copyright © 2023 Intel Corporation
  */
 
+#include "gt/intel_context.h"
 #include "gt/intel_engine_pm.h"
 #include "gt/intel_gpu_commands.h"
 #include "gt/intel_gt.h"
@@ -107,3 +108,104 @@ void intel_gsc_uc_heci_cmd_emit_mtl_header(struct intel_gsc_mtl_header *header,
 	header->header_version = MTL_GSC_HEADER_VERSION;
 	header->message_size = message_size;
 }
+
+static void
+emit_gsc_heci_pkt_nonpriv(u32 *cmd, struct intel_gsc_heci_non_priv_pkt *pkt)
+{
+	*cmd++ = GSC_HECI_CMD_PKT;
+	*cmd++ = lower_32_bits(pkt->addr_in);
+	*cmd++ = upper_32_bits(pkt->addr_in);
+	*cmd++ = pkt->size_in;
+	*cmd++ = lower_32_bits(pkt->addr_out);
+	*cmd++ = upper_32_bits(pkt->addr_out);
+	*cmd++ = pkt->size_out;
+	*cmd++ = 0;
+	*cmd++ = 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 *cmd, int timeout_ms)
+{
+	struct intel_engine_cs *eng;
+	struct i915_gem_ww_ctx ww;
+	struct i915_request *rq;
+	int err, trials = 0;
+
+	i915_gem_ww_ctx_init(&ww, false);
+retry:
+	err = i915_gem_object_lock(pkt->bb_vma->obj, &ww);
+	if (err)
+		goto out_ww;
+	err = i915_gem_object_lock(pkt->heci_pkt_vma->obj, &ww);
+	if (err)
+		goto out_ww;
+	err = intel_context_pin_ww(ce, &ww);
+	if (err)
+		goto out_ww;
+
+	rq = i915_request_create(ce);
+	if (IS_ERR(rq)) {
+		err = PTR_ERR(rq);
+		goto out_unpin_ce;
+	}
+
+	emit_gsc_heci_pkt_nonpriv(cmd, pkt);
+
+	err = i915_vma_move_to_active(pkt->bb_vma, rq, EXEC_OBJECT_WRITE);
+	if (err)
+		goto out_rq;
+	err = i915_vma_move_to_active(pkt->heci_pkt_vma, rq, EXEC_OBJECT_WRITE);
+	if (err)
+		goto out_rq;
+
+	eng = rq->context->engine;
+	if (eng->emit_init_breadcrumb) {
+		err = eng->emit_init_breadcrumb(rq);
+		if (err)
+			goto out_rq;
+	}
+
+	err = eng->emit_bb_start(rq, i915_vma_offset(pkt->bb_vma), PAGE_SIZE, 0);
+	if (err)
+		goto out_rq;
+
+	err = ce->engine->emit_flush(rq, 0);
+	if (err)
+		drm_err(&gsc_uc_to_gt(gsc)->i915->drm,
+			"Failed emit-flush for gsc-heci-non-priv-pkterr=%d\n", err);
+
+out_rq:
+	i915_request_get(rq);
+
+	if (unlikely(err))
+		i915_request_set_error_once(rq, err);
+
+	i915_request_add(rq);
+
+	if (!err) {
+		if (i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE,
+				      msecs_to_jiffies(timeout_ms)) < 0)
+			err = -ETIME;
+	}
+
+	i915_request_put(rq);
+
+out_unpin_ce:
+	intel_context_unpin(ce);
+out_ww:
+	if (err == -EDEADLK) {
+		err = i915_gem_ww_ctx_backoff(&ww);
+		if (!err) {
+			if (++trials < 10)
+				goto retry;
+			else
+				err = EAGAIN;
+		}
+	}
+	i915_gem_ww_ctx_fini(&ww);
+
+	return err;
+}
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
index 3d56ae501991..3addce861854 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
@@ -8,7 +8,10 @@
 
 #include <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
@@ -47,7 +50,7 @@ struct intel_gsc_mtl_header {
 	 * we distinguish the flags using OUTFLAG or INFLAG
 	 */
 	u32 flags;
-#define GSC_OUTFLAG_MSG_PENDING	1
+#define GSC_OUTFLAG_MSG_PENDING 1
 
 	u32 status;
 } __packed;
@@ -58,4 +61,24 @@ int intel_gsc_uc_heci_cmd_submit_packet(struct intel_gsc_uc *gsc,
 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);
+
+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

[Intel-gfx] [PATCH v7 4/8] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages

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

We use non-priveleged batches for submission to GSC engine
which require two buffers for the request:
     - a buffer for the HECI packet that contains PXP FW commands
     - a batch-buffer that contains the engine instruction for
       sending the HECI packet to the GSC firmware.

Thus, add the allocation and freeing of these buffers in gsccs
init and fini.

The GSC-fw may reply to commands with a SUCCESS but with an
additional pending-bit set in the reply packet. This bit
means the GSC-FW is currently busy and the caller needs to
try again with the gsc_message_handle the fw returned. Thus,
add a wrapper to continuously retry send_message while
replaying the gsc_message_handle. Retries need to follow the
arch-spec count and delay until GSC-FW replies with the real
SUCCESS or timeout after that spec'd delay.

The GSC-fw requires a non-zero host_session_handle provided
by the caller to enable gsc_message_handle tracking. Thus,
allocate the host_session_handle at init and destroy it
at fini (the latter requiring an FYI to the gsc-firmware).

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h |   1 +
 .../drm/i915/pxp/intel_pxp_cmd_interface_43.h |   3 +
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c    | 240 +++++++++++++++++-
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h    |   4 +
 drivers/gpu/drm/i915/pxp/intel_pxp_types.h    |   6 +
 5 files changed, 253 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
index 3addce861854..e4d6662339e8 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
@@ -51,6 +51,7 @@ struct intel_gsc_mtl_header {
 	 */
 	u32 flags;
 #define GSC_OUTFLAG_MSG_PENDING 1
+#define GSC_INFLAG_MSG_CLEANUP BIT(1)
 
 	u32 status;
 } __packed;
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..c65ada99e54f 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
@@ -12,6 +12,9 @@
 /* PXP-Cmd-Op definitions */
 #define PXP43_CMDID_START_HUC_AUTH 0x0000003A
 
+/* PXP-Packet sizes for MTL's GSCCS-HECI instruction */
+#define PXP43_MAX_HECI_INOUT_SIZE (SZ_32K)
+
 /* PXP-Input-Packet: HUC-Authentication */
 struct pxp43_start_huc_auth_in {
 	struct pxp_cmd_header header;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
index bad55719a7ac..16e3b73d0653 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
@@ -6,23 +6,232 @@
 #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"
 #include "intel_pxp_gsccs.h"
 #include "intel_pxp_types.h"
 
+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,
+		   u64 *gsc_msg_handle_retry)
+{
+	struct intel_gt *gt = pxp->ctrl_gt;
+	struct drm_i915_private *i915 = gt->i915;
+	struct gsccs_session_resources *exec_res =  &pxp->gsccs_res;
+	struct intel_gsc_mtl_header *header = exec_res->pkt_vaddr;
+	struct intel_gsc_heci_non_priv_pkt pkt;
+	size_t max_msg_size;
+	u32 reply_size;
+	int ret;
+
+	if (!exec_res->ce)
+		return -ENODEV;
+
+	max_msg_size = PXP43_MAX_HECI_INOUT_SIZE - sizeof(*header);
+
+	if (msg_in_size > max_msg_size || msg_out_size_max > max_msg_size)
+		return -ENOSPC;
+
+	if (!exec_res->pkt_vma || !exec_res->bb_vma)
+		return -ENOENT;
+
+	GEM_BUG_ON(exec_res->pkt_vma->size < (2 * PXP43_MAX_HECI_INOUT_SIZE));
+
+	mutex_lock(&pxp->tee_mutex);
+
+	memset(header, 0, sizeof(*header));
+	intel_gsc_uc_heci_cmd_emit_mtl_header(header, HECI_MEADDRESS_PXP,
+					      msg_in_size + sizeof(*header),
+					      exec_res->host_session_handle);
+
+	/* check if this is a host-session-handle cleanup call (empty packet) */
+	if (!msg_in && !msg_out)
+		header->flags |= GSC_INFLAG_MSG_CLEANUP;
+
+	/* copy caller provided gsc message handle if this is polling for a prior msg completion */
+	header->gsc_message_handle = *gsc_msg_handle_retry;
+
+	/* NOTE: zero size packets are used for session-cleanups */
+	if (msg_in && msg_in_size)
+		memcpy(exec_res->pkt_vaddr + sizeof(*header), msg_in, msg_in_size);
+
+	pkt.addr_in = i915_vma_offset(exec_res->pkt_vma);
+	pkt.size_in = header->message_size;
+	pkt.addr_out = pkt.addr_in + PXP43_MAX_HECI_INOUT_SIZE;
+	pkt.size_out = msg_out_size_max + sizeof(*header);
+	pkt.heci_pkt_vma = exec_res->pkt_vma;
+	pkt.bb_vma = exec_res->bb_vma;
+
+	/*
+	 * Before submitting, let's clear-out the validity marker on the reply offset.
+	 * We use offset PXP43_MAX_HECI_INOUT_SIZE for reply location so point header there.
+	 */
+	header = exec_res->pkt_vaddr + PXP43_MAX_HECI_INOUT_SIZE;
+	header->validity_marker = 0;
+
+	ret = intel_gsc_uc_heci_cmd_submit_nonpriv(&gt->uc.gsc,
+						   exec_res->ce, &pkt, exec_res->bb_vaddr,
+						   GSC_REPLY_LATENCY_MS);
+	if (ret) {
+		drm_err(&i915->drm, "failed to send gsc PXP msg (%d)\n", ret);
+		goto unlock;
+	}
+
+	/* 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_OUTFLAG_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;
+	}
+
+	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;
+	}
+
+	if (msg_out)
+		memcpy(msg_out, exec_res->pkt_vaddr + PXP43_MAX_HECI_INOUT_SIZE + sizeof(*header),
+		       reply_size);
+	if (msg_out_len)
+		*msg_out_len = reply_size;
+
+unlock:
+	mutex_unlock(&pxp->tee_mutex);
+	return ret;
+}
+
+static int
+gsccs_send_message_retry_complete(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)
+{
+	u64 gsc_session_retry = 0;
+	int ret, tries = 0;
+
+	/*
+	 * Keep sending request if GSC firmware was busy. Based on fw specs +
+	 * sw overhead (and testing) we expect a worst case pending-bit delay of
+	 * GSC_PENDING_RETRY_MAXCOUNT x GSC_PENDING_RETRY_PAUSE_MS millisecs.
+	 */
+	do {
+		ret = gsccs_send_message(pxp, msg_in, msg_in_size, msg_out, msg_out_size_max,
+					 msg_out_len, &gsc_session_retry);
+		/* Only try again if gsc says so */
+		if (ret != -EAGAIN)
+			break;
+
+		msleep(GSC_PENDING_RETRY_PAUSE_MS);
+	} while (++tries < GSC_PENDING_RETRY_MAXCOUNT);
+
+	return ret;
+}
+
+static void
+gsccs_cleanup_fw_host_session_handle(struct intel_pxp *pxp)
+{
+	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
+	int ret;
+
+	ret = gsccs_send_message_retry_complete(pxp, NULL, 0, NULL, 0, NULL);
+	if (ret)
+		drm_dbg(&i915->drm, "Failed to send gsccs msg host-session-cleanup: ret=[%d]\n",
+			ret);
+}
+
 static void
 gsccs_destroy_execution_resource(struct intel_pxp *pxp)
 {
 	struct gsccs_session_resources *exec_res = &pxp->gsccs_res;
 
+	if (exec_res->host_session_handle)
+		gsccs_cleanup_fw_host_session_handle(pxp);
 	if (exec_res->ce)
 		intel_context_put(exec_res->ce);
+	if (exec_res->bb_vma)
+		i915_vma_unpin_and_release(&exec_res->bb_vma, I915_VMA_RELEASE_MAP);
+	if (exec_res->pkt_vma)
+		i915_vma_unpin_and_release(&exec_res->pkt_vma, I915_VMA_RELEASE_MAP);
 
 	memset(exec_res, 0, sizeof(*exec_res));
 }
 
+static int
+gsccs_create_buffer(struct intel_gt *gt,
+		    const char *bufname, size_t size,
+		    struct i915_vma **vma, void **map)
+{
+	struct drm_i915_private *i915 = gt->i915;
+	struct drm_i915_gem_object *obj;
+	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, gt->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;
+	}
+
+	/* return a virtual 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_put;
+	}
+
+	/* all PXP sessions commands are treated as non-privileged */
+	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_put:
+	i915_gem_object_put(obj);
+out_none:
+	*vma = NULL;
+	*map = NULL;
+
+	return err;
+}
+
 static int
 gsccs_allocate_execution_resource(struct intel_pxp *pxp)
 {
@@ -30,6 +239,7 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp)
 	struct gsccs_session_resources *exec_res = &pxp->gsccs_res;
 	struct intel_engine_cs *engine = gt->engine[GSC0];
 	struct intel_context *ce;
+	int err = 0;
 
 	/*
 	 * First, ensure the GSC engine is present.
@@ -38,18 +248,46 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp)
 	if (!engine)
 		return -ENODEV;
 
+	/*
+	 * 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, "Heci Packet",
+				  2 * PXP43_MAX_HECI_INOUT_SIZE,
+				  &exec_res->pkt_vma, &exec_res->pkt_vaddr);
+	if (err)
+		return err;
+
+	err = gsccs_create_buffer(pxp->ctrl_gt, "Batch Buffer", PAGE_SIZE,
+				  &exec_res->bb_vma, &exec_res->bb_vaddr);
+	if (err)
+		goto free_pkt;
+
 	/* Finally, create an intel_context to be used during the submission */
 	ce = intel_context_create(engine);
 	if (IS_ERR(ce)) {
 		drm_err(&gt->i915->drm, "Failed creating gsccs backend ctx\n");
-		return PTR_ERR(ce);
+		err = PTR_ERR(ce);
+		goto free_batch;
 	}
 
 	i915_vm_put(ce->vm);
 	ce->vm = i915_vm_get(pxp->ctrl_gt->vm);
 	exec_res->ce = ce;
 
+	/* initialize host-session-handle (for all i915-to-gsc-firmware PXP cmds) */
+	get_random_bytes(&exec_res->host_session_handle, sizeof(exec_res->host_session_handle));
+
 	return 0;
+
+free_batch:
+	i915_vma_unpin_and_release(&exec_res->bb_vma, I915_VMA_RELEASE_MAP);
+free_pkt:
+	i915_vma_unpin_and_release(&exec_res->pkt_vma, I915_VMA_RELEASE_MAP);
+	memset(exec_res, 0, sizeof(*exec_res));
+
+	return err;
 }
 
 void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
index 354ea9a8f940..bd1c028bc80f 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
@@ -10,6 +10,10 @@
 
 struct intel_pxp;
 
+#define GSC_REPLY_LATENCY_MS 200
+#define GSC_PENDING_RETRY_MAXCOUNT 40
+#define GSC_PENDING_RETRY_PAUSE_MS 50
+
 #ifdef CONFIG_DRM_I915_PXP
 void intel_pxp_gsccs_fini(struct intel_pxp *pxp);
 int intel_pxp_gsccs_init(struct intel_pxp *pxp);
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
index fdd98911968d..73392fbab7ee 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
@@ -38,6 +38,12 @@ struct intel_pxp {
 	struct gsccs_session_resources {
 		u64 host_session_handle; /* used by firmware to link commands to sessions */
 		struct intel_context *ce; /* context for gsc command submission */
+
+		struct i915_vma *pkt_vma; /* GSC FW cmd packet vma */
+		void *pkt_vaddr;  /* GSC FW cmd packet virt pointer */
+
+		struct i915_vma *bb_vma; /* HECI_PKT batch buffer vma */
+		void *bb_vaddr; /* HECI_PKT batch buffer virt pointer */
 	} gsccs_res;
 
 	/**
-- 
2.39.0

[Intel-gfx] [PATCH v7 5/8] drm/i915/pxp: Add ARB session creation and cleanup

[Alan Previn]

Add MTL's function for ARB session creation using PXP firmware
version 4.3 ABI structure format.

Also add MTL's function for ARB session invalidation but this
reuses PXP firmware version 4.2 ABI structure format.

For both cases, in the back-end gsccs functions for sending messages
to the firmware inspect the GSC-CS-Mem-Header's pending-bit which
means the GSC firmware is busy and we should retry.

Given the last hw requirement, lets also update functions in
front-end layer that wait for session creation or teardown
completion to use new worst case timeout periods.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c     |  10 ++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h     |   1 +
 drivers/gpu/drm/i915/pxp/intel_pxp.c          |  28 +++-
 .../drm/i915/pxp/intel_pxp_cmd_interface_43.h |  21 +++
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c    | 130 ++++++++++++++++++
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h    |   8 +-
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c  |  11 +-
 7 files changed, 202 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
index 1d9fdfb11268..85f720af2f75 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
@@ -13,6 +13,7 @@
 #define GSC_FW_STATUS_REG			_MMIO(0x116C40)
 #define GSC_FW_CURRENT_STATE			REG_GENMASK(3, 0)
 #define   GSC_FW_CURRENT_STATE_RESET		0
+#define   GSC_FW_PROXY_STATE_NORMAL		5
 #define GSC_FW_INIT_COMPLETE_BIT		REG_BIT(9)
 
 static bool gsc_is_in_reset(struct intel_uncore *uncore)
@@ -31,6 +32,15 @@ bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc)
 	return fw_status & GSC_FW_INIT_COMPLETE_BIT;
 }
 
+bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc)
+{
+	struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore;
+	u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
+
+	return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) ==
+	       GSC_FW_PROXY_STATE_NORMAL;
+}
+
 static int emit_gsc_fw_load(struct i915_request *rq, struct intel_gsc_uc *gsc)
 {
 	u32 offset = i915_ggtt_offset(gsc->local);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
index f4c1106bb2a9..fff8928218df 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
@@ -13,5 +13,6 @@ struct intel_uncore;
 
 int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc);
 bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc);
+bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc);
 
 #endif
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 8949d4be7882..550457bbb034 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -291,6 +291,8 @@ static bool pxp_component_bound(struct intel_pxp *pxp)
 
 static int __pxp_global_teardown_final(struct intel_pxp *pxp)
 {
+	int timeout;
+
 	if (!pxp->arb_is_valid)
 		return 0;
 	/*
@@ -300,7 +302,12 @@ static int __pxp_global_teardown_final(struct intel_pxp *pxp)
 	intel_pxp_mark_termination_in_progress(pxp);
 	intel_pxp_terminate(pxp, false);
 
-	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
+	if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
+		timeout = GSCFW_MAX_ROUND_TRIP_LATENCY_MS;
+	else
+		timeout = 250;
+
+	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout)))
 		return -ETIMEDOUT;
 
 	return 0;
@@ -308,6 +315,8 @@ static int __pxp_global_teardown_final(struct intel_pxp *pxp)
 
 static int __pxp_global_teardown_restart(struct intel_pxp *pxp)
 {
+	int timeout;
+
 	if (pxp->arb_is_valid)
 		return 0;
 	/*
@@ -316,7 +325,12 @@ static int __pxp_global_teardown_restart(struct intel_pxp *pxp)
 	 */
 	pxp_queue_termination(pxp);
 
-	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
+	if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
+		timeout = GSCFW_MAX_ROUND_TRIP_LATENCY_MS;
+	else
+		timeout = 250;
+
+	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout)))
 		return -ETIMEDOUT;
 
 	return 0;
@@ -354,8 +368,14 @@ 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 (HAS_ENGINE(pxp->ctrl_gt, GSC0)) {
+		/* Use a larger 1 second timeout considering all the dependencies. */
+		if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 1000))
+			return -ENXIO;
+	} else {
+		if (wait_for(pxp_component_bound(pxp), 250))
+			return -ENXIO;
+	}
 
 	mutex_lock(&pxp->arb_mutex);
 
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 c65ada99e54f..09777719cd84 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_INOUT_SIZE (SZ_32K)
@@ -26,4 +27,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 BIT(0)
+		#define PXP43_INIT_SESSION_APPTYPE BIT(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 16e3b73d0653..4bc276daca16 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
@@ -6,13 +6,46 @@
 #include "gem/i915_gem_internal.h"
 
 #include "gt/intel_context.h"
+#include "gt/uc/intel_gsc_fw.h"
 #include "gt/uc/intel_gsc_uc_heci_cmd_submit.h"
 
 #include "i915_drv.h"
+#include "intel_pxp_cmd_interface_42.h"
 #include "intel_pxp_cmd_interface_43.h"
 #include "intel_pxp_gsccs.h"
 #include "intel_pxp_types.h"
 
+static bool
+is_fw_err_platform_config(u32 type)
+{
+	switch (type) {
+	case PXP_STATUS_ERROR_API_VERSION:
+	case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF:
+	case PXP_STATUS_PLATFCONFIG_KF1_BAD:
+		return true;
+	default:
+		break;
+	}
+	return false;
+}
+
+static const char *
+fw_err_to_string(u32 type)
+{
+	switch (type) {
+	case PXP_STATUS_ERROR_API_VERSION:
+		return "ERR_API_VERSION";
+	case PXP_STATUS_NOT_READY:
+		return "ERR_NOT_READY";
+	case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF:
+	case PXP_STATUS_PLATFCONFIG_KF1_BAD:
+		return "ERR_PLATFORM_CONFIG";
+	default:
+		break;
+	}
+	return NULL;
+}
+
 static int
 gsccs_send_message(struct intel_pxp *pxp,
 		   void *msg_in, size_t msg_in_size,
@@ -152,6 +185,103 @@ gsccs_send_message_retry_complete(struct intel_pxp *pxp,
 	return ret;
 }
 
+bool intel_pxp_gsccs_is_ready_for_sessions(struct intel_pxp *pxp)
+{
+	/*
+	 * GSC-fw loading, HuC-fw loading, HuC-fw authentication and
+	 * GSC-proxy init flow (requiring an mei component driver)
+	 * must all occur first before we can start requesting for PXP
+	 * sessions. Checking for completion on HuC authentication and
+	 * gsc-proxy init flow (the last set of dependencies that
+	 * are out of order) will suffice.
+	 */
+	if (intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc) &&
+	    intel_gsc_uc_fw_proxy_init_done(&pxp->ctrl_gt->uc.gsc))
+		return true;
+
+	return false;
+}
+
+int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
+				   int arb_session_id)
+{
+	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
+	struct pxp43_create_arb_in msg_in = {0};
+	struct pxp43_create_arb_out msg_out = {0};
+	int ret;
+
+	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;
+
+	ret = gsccs_send_message_retry_complete(pxp,
+						&msg_in, sizeof(msg_in),
+						&msg_out, sizeof(msg_out), NULL);
+	if (ret) {
+		drm_err(&i915->drm, "Failed to init session %d, ret=[%d]\n", arb_session_id, ret);
+	} else if (msg_out.header.status != 0) {
+		if (is_fw_err_platform_config(msg_out.header.status)) {
+			drm_info_once(&i915->drm,
+				      "PXP init-session-%d failed due to BIOS/SOC:0x%08x:%s\n",
+				      arb_session_id, msg_out.header.status,
+				      fw_err_to_string(msg_out.header.status));
+		} else {
+			drm_dbg(&i915->drm, "PXP init-session-%d failed 0x%08x:%st:\n",
+				arb_session_id, msg_out.header.status,
+				fw_err_to_string(msg_out.header.status));
+			drm_dbg(&i915->drm, "     cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n",
+				msg_in.header.command_id, msg_in.header.api_version);
+		}
+	}
+
+	return ret;
+}
+
+void intel_pxp_gsccs_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id)
+{
+	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
+	struct pxp42_inv_stream_key_in msg_in = {0};
+	struct pxp42_inv_stream_key_out msg_out = {0};
+	int ret = 0;
+
+	/*
+	 * Stream key invalidation reuses the same version 4.2 input/output
+	 * command format but firmware requires 4.3 API interaction
+	 */
+	msg_in.header.api_version = PXP_APIVER(4, 3);
+	msg_in.header.command_id = PXP42_CMDID_INVALIDATE_STREAM_KEY;
+	msg_in.header.buffer_len = sizeof(msg_in) - sizeof(msg_in.header);
+
+	msg_in.header.stream_id = FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_VALID, 1);
+	msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_APP_TYPE, 0);
+	msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_ID, session_id);
+
+	ret = gsccs_send_message_retry_complete(pxp,
+						&msg_in, sizeof(msg_in),
+						&msg_out, sizeof(msg_out), NULL);
+	if (ret) {
+		drm_err(&i915->drm, "Failed to inv-stream-key-%u, ret=[%d]\n",
+			session_id, ret);
+	} else if (msg_out.header.status != 0) {
+		if (is_fw_err_platform_config(msg_out.header.status)) {
+			drm_info_once(&i915->drm,
+				      "PXP inv-stream-key-%u failed due to BIOS/SOC :0x%08x:%s\n",
+				      session_id, msg_out.header.status,
+				      fw_err_to_string(msg_out.header.status));
+		} else {
+			drm_dbg(&i915->drm, "PXP inv-stream-key-%u failed 0x%08x:%s:\n",
+				session_id, msg_out.header.status,
+				fw_err_to_string(msg_out.header.status));
+			drm_dbg(&i915->drm, "     cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n",
+				msg_in.header.command_id, msg_in.header.api_version);
+		}
+	}
+}
+
 static void
 gsccs_cleanup_fw_host_session_handle(struct intel_pxp *pxp)
 {
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
index bd1c028bc80f..820c2def21ee 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
@@ -10,14 +10,18 @@
 
 struct intel_pxp;
 
-#define GSC_REPLY_LATENCY_MS 200
+#define GSC_REPLY_LATENCY_MS 210
 #define GSC_PENDING_RETRY_MAXCOUNT 40
 #define GSC_PENDING_RETRY_PAUSE_MS 50
+#define GSCFW_MAX_ROUND_TRIP_LATENCY_MS (GSC_PENDING_RETRY_MAXCOUNT * GSC_PENDING_RETRY_PAUSE_MS)
 
 #ifdef CONFIG_DRM_I915_PXP
 void intel_pxp_gsccs_fini(struct intel_pxp *pxp);
 int intel_pxp_gsccs_init(struct intel_pxp *pxp);
 
+int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, int arb_session_id);
+void intel_pxp_gsccs_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
+
 #else
 static inline void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
 {
@@ -30,4 +34,6 @@ static inline int intel_pxp_gsccs_init(struct intel_pxp *pxp)
 
 #endif
 
+bool intel_pxp_gsccs_is_ready_for_sessions(struct intel_pxp *pxp);
+
 #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 7899079e17b0..e4d8242302c5 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"
@@ -62,7 +63,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 (HAS_ENGINE(pxp->ctrl_gt, GSC0))
+		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(&gt->i915->drm, "tee cmd for arb session creation failed\n");
 		return ret;
@@ -106,7 +110,10 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
 
 	intel_uncore_write(gt->uncore, KCR_GLOBAL_TERMINATE(pxp->kcr_base), 1);
 
-	intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION);
+	if (HAS_ENGINE(gt, GSC0))
+		intel_pxp_gsccs_end_arb_fw_session(pxp, ARB_SESSION);
+	else
+		intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION);
 
 	return ret;
 }
-- 
2.39.0

[Intel-gfx] [PATCH v7 6/8] drm/i915/uapi/pxp: Fix UAPI spec comments and add GET_PARAM for PXP

[Alan Previn]

1. UAPI update:
Without actually changing backward compatible behavior, update
i915's drm-uapi comments that describe the possible error values
when creating a context with I915_CONTEXT_PARAM_PROTECTED_CONTENT.
Since the first merge of PXP support on ADL, i915 returns
-ENXIO if a dependency such as firmware or component driver
was yet to be loaded or returns -EIO if the creation attempt
failed when requested by the PXP firmware (specific firmware
error responses are reported in dmesg).

2. GET_PARAM for PXP:
Because of the additional firmware, component-driver and
initialization depedencies required on MTL platform before a
PXP context can be created, UMD calling for PXP creation as a
way to get-caps can take a long time. An actual real world
customer stack has seen this happen in the 4-to-8 second range
after the kernel starts (which sees MESA's init appear in the
middle of this range as the compositor comes up). To avoid
unncessary delays experienced by the UMD for get-caps purposes,
add a GET_PARAM for I915_PARAM_PXP_SUPPORT.

However, some failures can still occur after all the depedencies
are met (such as firmware init flow failure, bios configurations
or SOC fusing not allowing PXP enablement). Those scenarios will
only be known to user space when it attempts creating a PXP context.

With this change, large delays are only met by user-space procsses
that explicitly need to create a PXP context and boot very early.
There is no way to avoid this today.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/i915_getparam.c |  5 +++++
 include/uapi/drm/i915_drm.h          | 22 ++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
index 2238e096c957..9729384f033f 100644
--- a/drivers/gpu/drm/i915/i915_getparam.c
+++ b/drivers/gpu/drm/i915/i915_getparam.c
@@ -5,6 +5,8 @@
 #include "gem/i915_gem_mman.h"
 #include "gt/intel_engine_user.h"
 
+#include "pxp/intel_pxp.h"
+
 #include "i915_cmd_parser.h"
 #include "i915_drv.h"
 #include "i915_getparam.h"
@@ -102,6 +104,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
 		if (value < 0)
 			return value;
 		break;
+	case I915_PARAM_PXP_STATUS:
+		value = intel_pxp_is_enabled(i915->pxp) ? 0 : -ENODEV;
+		break;
 	case I915_PARAM_MMAP_GTT_VERSION:
 		/* Though we've started our numbering from 1, and so class all
 		 * earlier versions as 0, in effect their value is undefined as
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index dba7c5a5b25e..0c1729bd911d 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -771,6 +771,20 @@ typedef struct drm_i915_irq_wait {
  */
 #define I915_PARAM_OA_TIMESTAMP_FREQUENCY 57
 
+/*
+ * Query the status of PXP support in i915.
+ *
+ * The query can fail in the following scenarios with the listed error codes:
+ *  -ENODEV = PXP support is not available on the GPU device or in the kernel
+ *            due to missing component drivers or kernel configs.
+ * If the IOCTL is successful, the returned parameter will be set to one of the
+ * following values:
+ *   0 = PXP support maybe available but underlying SOC fusing, BIOS or firmware
+ *       configuration is unknown and a PXP-context-creation would be required
+ *       for final verification of feature availibility.
+ */
+#define I915_PARAM_PXP_STATUS		 58
+
 /* Must be kept compact -- no holes and well documented */
 
 /**
@@ -2096,6 +2110,14 @@ struct drm_i915_gem_context_param {
  *
  * -ENODEV: feature not available
  * -EPERM: trying to mark a recoverable or not bannable context as protected
+ * -ENXIO: A dependency such as a component driver or firmware is not yet
+ *         loaded and user space may attempt again. Depending on the device
+ *         this error may be reported if the protected context creation is
+ *         attempted very early from kernel start (numbers vary depending on
+ *         system and kernel config):
+ *            - ADL/RPL: up to 3 seconds
+ *            - MTL: up to 8 seconds
+ * -EIO: The firmware did not succeed in creating the protected context.
  */
 #define I915_CONTEXT_PARAM_PROTECTED_CONTENT    0xd
 /* Must be kept compact -- no holes and well documented */
-- 
2.39.0

[Intel-gfx] [PATCH v7 7/8] drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component

[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 a tee component
to start sending GSC-CS firmware messages.

Thus, immediately enable (or disable) KCR HW on PXP's init,
fini and resume.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 15 ++++++++++++++-
 drivers/gpu/drm/i915/pxp/intel_pxp_pm.c    |  3 ++-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
index 4bc276daca16..8dc41de3f6f7 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
@@ -10,6 +10,7 @@
 #include "gt/uc/intel_gsc_uc_heci_cmd_submit.h"
 
 #include "i915_drv.h"
+#include "intel_pxp.h"
 #include "intel_pxp_cmd_interface_42.h"
 #include "intel_pxp_cmd_interface_43.h"
 #include "intel_pxp_gsccs.h"
@@ -422,10 +423,22 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp)
 
 void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
 {
+	intel_wakeref_t wakeref;
+
 	gsccs_destroy_execution_resource(pxp);
+	with_intel_runtime_pm(&pxp->ctrl_gt->i915->runtime_pm, wakeref)
+		intel_pxp_fini_hw(pxp);
 }
 
 int intel_pxp_gsccs_init(struct intel_pxp *pxp)
 {
-	return gsccs_allocate_execution_resource(pxp);
+	int ret;
+	intel_wakeref_t wakeref;
+
+	ret = gsccs_allocate_execution_resource(pxp);
+	if (!ret) {
+		with_intel_runtime_pm(&pxp->ctrl_gt->i915->runtime_pm, wakeref)
+			intel_pxp_init_hw(pxp);
+	}
+	return ret;
 }
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
index 4f836b317424..1a04067f61fc 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_pm.c
@@ -43,8 +43,9 @@ void intel_pxp_resume_complete(struct intel_pxp *pxp)
 	 * The 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.
+	 * NOTE: GSC-CS backend doesn't rely on components.
 	 */
-	if (!pxp->pxp_component)
+	if (!HAS_ENGINE(pxp->ctrl_gt, GSC0) && !pxp->pxp_component)
 		return;
 
 	intel_pxp_init_hw(pxp);
-- 
2.39.0

[Intel-gfx] [PATCH v7 8/8] drm/i915/pxp: Enable PXP with MTL-GSC-CS

[Alan Previn]

Enable PXP with MTL-GSC-CS: add the has_pxp into device info
and increase the debugfs teardown timeouts to align with
new GSC-CS + firmware specs.

Now that we have 3 places that are selecting pxp timeouts
based on tee vs gsccs back-end, let's add a helper.

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/i915_pci.c              |  1 +
 drivers/gpu/drm/i915/pxp/intel_pxp.c         | 18 ++++++++++--------
 drivers/gpu/drm/i915/pxp/intel_pxp.h         |  1 +
 drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c |  6 +++++-
 drivers/gpu/drm/i915/pxp/intel_pxp_session.c |  2 +-
 5 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index cddb6e197972..9bbea83c5a4e 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.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index 550457bbb034..833ca2bdd6fa 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -289,6 +289,14 @@ static bool pxp_component_bound(struct intel_pxp *pxp)
 	return bound;
 }
 
+int intel_pxp_get_backend_timeout_ms(struct intel_pxp *pxp)
+{
+	if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
+		return GSCFW_MAX_ROUND_TRIP_LATENCY_MS;
+	else
+		return 250;
+}
+
 static int __pxp_global_teardown_final(struct intel_pxp *pxp)
 {
 	int timeout;
@@ -302,10 +310,7 @@ static int __pxp_global_teardown_final(struct intel_pxp *pxp)
 	intel_pxp_mark_termination_in_progress(pxp);
 	intel_pxp_terminate(pxp, false);
 
-	if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
-		timeout = GSCFW_MAX_ROUND_TRIP_LATENCY_MS;
-	else
-		timeout = 250;
+	timeout = intel_pxp_get_backend_timeout_ms(pxp);
 
 	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout)))
 		return -ETIMEDOUT;
@@ -325,10 +330,7 @@ static int __pxp_global_teardown_restart(struct intel_pxp *pxp)
 	 */
 	pxp_queue_termination(pxp);
 
-	if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
-		timeout = GSCFW_MAX_ROUND_TRIP_LATENCY_MS;
-	else
-		timeout = 250;
+	timeout = intel_pxp_get_backend_timeout_ms(pxp);
 
 	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout)))
 		return -ETIMEDOUT;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.h b/drivers/gpu/drm/i915/pxp/intel_pxp.h
index 3ded0890cd27..9b11211255f9 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.h
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.h
@@ -26,6 +26,7 @@ void intel_pxp_fini_hw(struct intel_pxp *pxp);
 void intel_pxp_mark_termination_in_progress(struct intel_pxp *pxp);
 void intel_pxp_tee_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
 
+int intel_pxp_get_backend_timeout_ms(struct intel_pxp *pxp);
 int intel_pxp_start(struct intel_pxp *pxp);
 void intel_pxp_end(struct intel_pxp *pxp);
 
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
index 4b8e70caa3ad..e07c5b380789 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c
@@ -14,6 +14,7 @@
 
 #include "intel_pxp.h"
 #include "intel_pxp_debugfs.h"
+#include "intel_pxp_gsccs.h"
 #include "intel_pxp_irq.h"
 #include "intel_pxp_types.h"
 
@@ -45,6 +46,7 @@ static int pxp_terminate_set(void *data, u64 val)
 {
 	struct intel_pxp *pxp = data;
 	struct intel_gt *gt = pxp->ctrl_gt;
+	int timeout_ms;
 
 	if (!intel_pxp_is_active(pxp))
 		return -ENODEV;
@@ -54,8 +56,10 @@ static int pxp_terminate_set(void *data, u64 val)
 	intel_pxp_irq_handler(pxp, GEN12_DISPLAY_PXP_STATE_TERMINATED_INTERRUPT);
 	spin_unlock_irq(gt->irq_lock);
 
+	timeout_ms = intel_pxp_get_backend_timeout_ms(pxp);
+
 	if (!wait_for_completion_timeout(&pxp->termination,
-					 msecs_to_jiffies(100)))
+					 msecs_to_jiffies(timeout_ms)))
 		return -ETIMEDOUT;
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
index e4d8242302c5..0a3e66b0265e 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp_session.c
@@ -44,7 +44,7 @@ static int pxp_wait_for_session_state(struct intel_pxp *pxp, u32 id, bool in_pla
 				      KCR_SIP(pxp->kcr_base),
 				      mask,
 				      in_play ? mask : 0,
-				      100);
+				      250);
 
 	intel_runtime_pm_put(uncore->rpm, wakeref);
 
-- 
2.39.0

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/pxp: Add MTL PXP Support (rev7)

[Patchwork]

== Series Details ==

Series: drm/i915/pxp: Add MTL PXP Support (rev7)
URL   : https://patchwork.freedesktop.org/series/112647/
State : warning

== Summary ==

Error: dim checkpatch failed
495f3b5c4d82 drm/i915/pxp: Add GSC-CS back-end resource init and cleanup
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
-:99: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#99: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 173 lines checked
a30073078612 drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
-:109: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#109: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 153 lines checked
ac85c33a6337 drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC
04a1ebb47e9c drm/i915/pxp: Add GSC-CS backend to send GSC fw messages
-:106: WARNING:AVOID_BUG: Do not crash the kernel unless it is absolutely unavoidable--use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants
#106: FILE: drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c:43:
+	GEM_BUG_ON(exec_res->pkt_vma->size < (2 * PXP43_MAX_HECI_INOUT_SIZE));

total: 0 errors, 1 warnings, 0 checks, 324 lines checked
d3cbd5ec964c drm/i915/pxp: Add ARB session creation and cleanup
c7ac1fff4798 drm/i915/uapi/pxp: Fix UAPI spec comments and add GET_PARAM for PXP
c96e3afaa85b drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component
6fcb7aeab404 drm/i915/pxp: Enable PXP with MTL-GSC-CS

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/pxp: Add MTL PXP Support (rev7)

[Patchwork]

== Series Details ==

Series: drm/i915/pxp: Add MTL PXP Support (rev7)
URL   : https://patchwork.freedesktop.org/series/112647/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/pxp: Add MTL PXP Support (rev7)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4476 bytes --]

== Series Details ==

Series: drm/i915/pxp: Add MTL PXP Support (rev7)
URL   : https://patchwork.freedesktop.org/series/112647/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12981 -> Patchwork_112647v7
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/index.html

Participating hosts (36 -> 35)
------------------------------

  Missing    (1): fi-snb-2520m 

Known issues
------------

  Here are the changes found in Patchwork_112647v7 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@migrate:
    - bat-dg2-11:         [PASS][1] -> [DMESG-WARN][2] ([i915#7699])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/bat-dg2-11/igt@i915_selftest@live@migrate.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/bat-dg2-11/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@requests:
    - bat-rpls-1:         [PASS][3] -> [ABORT][4] ([i915#4983] / [i915#7911])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/bat-rpls-1/igt@i915_selftest@live@requests.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/bat-rpls-1/igt@i915_selftest@live@requests.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         NOTRUN -> [SKIP][5] ([i915#5354]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

  
#### Possible fixes ####

  * igt@i915_pm_rps@basic-api:
    - bat-dg2-11:         [FAIL][6] ([i915#8308]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/bat-dg2-11/igt@i915_pm_rps@basic-api.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/bat-dg2-11/igt@i915_pm_rps@basic-api.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1:
    - bat-dg2-8:          [FAIL][8] ([i915#7932]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html

  
#### Warnings ####

  * igt@i915_selftest@live@slpc:
    - bat-rpls-2:         [DMESG-FAIL][10] ([i915#6367] / [i915#7913]) -> [DMESG-FAIL][11] ([i915#6367] / [i915#6997] / [i915#7913])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/bat-rpls-2/igt@i915_selftest@live@slpc.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/bat-rpls-2/igt@i915_selftest@live@slpc.html

  
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#8308]: https://gitlab.freedesktop.org/drm/intel/issues/8308


Build changes
-------------

  * Linux: CI_DRM_12981 -> Patchwork_112647v7

  CI-20190529: 20190529
  CI_DRM_12981: fbadfcf137737f02425a35bf3ae17a1492301f21 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7243: 402a13477510ab05591839a2bf4586de1158e60c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_112647v7: fbadfcf137737f02425a35bf3ae17a1492301f21 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

1f1688e35648 drm/i915/pxp: Enable PXP with MTL-GSC-CS
eaf06189f88a drm/i915/pxp: On MTL, KCR enabling doesn't wait on tee component
8d8dba86604d drm/i915/uapi/pxp: Fix UAPI spec comments and add GET_PARAM for PXP
de5bd70a9c89 drm/i915/pxp: Add ARB session creation and cleanup
96a4f94f7b8c drm/i915/pxp: Add GSC-CS backend to send GSC fw messages
970c4dc0b51f drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC
357fc1709117 drm/i915/pxp: Add MTL hw-plumbing enabling for KCR operation
9c938ac29edc drm/i915/pxp: Add GSC-CS back-end resource init and cleanup

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/index.html

[-- Attachment #2: Type: text/html, Size: 5386 bytes --]

[Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/pxp: Add MTL PXP Support (rev7)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 15122 bytes --]

== Series Details ==

Series: drm/i915/pxp: Add MTL PXP Support (rev7)
URL   : https://patchwork.freedesktop.org/series/112647/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12981_full -> Patchwork_112647v7_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_112647v7_full:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset@pipe-d:
    - {shard-tglu}:       [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-tglu-9/igt@kms_busy@extended-modeset-hang-newfb-with-reset@pipe-d.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-tglu-5/igt@kms_busy@extended-modeset-hang-newfb-with-reset@pipe-d.html

  
Known issues
------------

  Here are the changes found in Patchwork_112647v7_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-pace-solo@rcs0:
    - shard-apl:          [PASS][3] -> [FAIL][4] ([i915#2842])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-apl6/igt@gem_exec_fair@basic-pace-solo@rcs0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-apl7/igt@gem_exec_fair@basic-pace-solo@rcs0.html

  * igt@i915_pm_rps@reset:
    - shard-snb:          [PASS][5] -> [INCOMPLETE][6] ([i915#7790])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-snb5/igt@i915_pm_rps@reset.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-snb4/igt@i915_pm_rps@reset.html

  * igt@i915_selftest@live@gt_heartbeat:
    - shard-apl:          [PASS][7] -> [DMESG-FAIL][8] ([i915#5334])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-apl6/igt@i915_selftest@live@gt_heartbeat.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-apl6/igt@i915_selftest@live@gt_heartbeat.html

  * igt@kms_ccs@pipe-c-bad-pixel-format-y_tiled_gen12_mc_ccs:
    - shard-apl:          NOTRUN -> [SKIP][9] ([fdo#109271] / [i915#3886])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-apl3/igt@kms_ccs@pipe-c-bad-pixel-format-y_tiled_gen12_mc_ccs.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-apl:          [PASS][10] -> [FAIL][11] ([i915#2346])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-apl3/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-apl6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
    - shard-glk:          [PASS][12] -> [FAIL][13] ([i915#2346])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-glk6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-glk8/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_cursor_legacy@short-flip-before-cursor-atomic-transitions-varying-size:
    - shard-snb:          [PASS][14] -> [SKIP][15] ([fdo#109271])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-snb7/igt@kms_cursor_legacy@short-flip-before-cursor-atomic-transitions-varying-size.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-snb4/igt@kms_cursor_legacy@short-flip-before-cursor-atomic-transitions-varying-size.html

  * igt@kms_flip@2x-nonexisting-fb:
    - shard-apl:          NOTRUN -> [SKIP][16] ([fdo#109271]) +32 similar issues
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-apl3/igt@kms_flip@2x-nonexisting-fb.html

  * igt@kms_flip@2x-plain-flip-ts-check-interruptible@ab-hdmi-a1-hdmi-a2:
    - shard-glk:          [PASS][17] -> [FAIL][18] ([i915#2122])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-glk1/igt@kms_flip@2x-plain-flip-ts-check-interruptible@ab-hdmi-a1-hdmi-a2.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-glk1/igt@kms_flip@2x-plain-flip-ts-check-interruptible@ab-hdmi-a1-hdmi-a2.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@a-hdmi-a1:
    - shard-glk:          [PASS][19] -> [FAIL][20] ([i915#79])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-glk1/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-hdmi-a1.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-glk1/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-hdmi-a1.html

  * igt@kms_flip@flip-vs-suspend-interruptible@c-dp1:
    - shard-apl:          [PASS][21] -> [ABORT][22] ([i915#180])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-apl1/igt@kms_flip@flip-vs-suspend-interruptible@c-dp1.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-apl7/igt@kms_flip@flip-vs-suspend-interruptible@c-dp1.html

  
#### Possible fixes ####

  * igt@drm_fdinfo@virtual-idle:
    - {shard-rkl}:        [FAIL][23] ([i915#7742]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-rkl-2/igt@drm_fdinfo@virtual-idle.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-rkl-2/igt@drm_fdinfo@virtual-idle.html

  * igt@gem_exec_fair@basic-none-solo@rcs0:
    - shard-apl:          [FAIL][25] ([i915#2842]) -> [PASS][26] +1 similar issue
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-apl4/igt@gem_exec_fair@basic-none-solo@rcs0.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-apl1/igt@gem_exec_fair@basic-none-solo@rcs0.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - {shard-rkl}:        [FAIL][27] ([i915#2842]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-rkl-6/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-rkl-7/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@i915_pm_rpm@dpms-mode-unset-lpsp:
    - {shard-rkl}:        [SKIP][29] ([i915#1397]) -> [PASS][30] +1 similar issue
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-rkl-6/igt@i915_pm_rpm@dpms-mode-unset-lpsp.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-rkl-7/igt@i915_pm_rpm@dpms-mode-unset-lpsp.html

  * igt@kms_cursor_crc@cursor-suspend@pipe-c-dp-1:
    - shard-apl:          [ABORT][31] ([i915#180]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-apl1/igt@kms_cursor_crc@cursor-suspend@pipe-c-dp-1.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-apl3/igt@kms_cursor_crc@cursor-suspend@pipe-c-dp-1.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
    - shard-glk:          [FAIL][33] ([i915#2346]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-glk9/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-glk9/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html

  * igt@kms_cursor_legacy@forked-move@pipe-b:
    - {shard-rkl}:        [INCOMPLETE][35] ([i915#8011]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-rkl-7/igt@kms_cursor_legacy@forked-move@pipe-b.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-rkl-3/igt@kms_cursor_legacy@forked-move@pipe-b.html

  * igt@kms_plane_scaling@i915-max-src-size@pipe-a-hdmi-a-2:
    - {shard-rkl}:        [FAIL][37] ([i915#8292]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12981/shard-rkl-4/igt@kms_plane_scaling@i915-max-src-size@pipe-a-hdmi-a-2.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/shard-rkl-6/igt@kms_plane_scaling@i915-max-src-size@pipe-a-hdmi-a-2.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109302]: https://bugs.freedesktop.org/show_bug.cgi?id=109302
  [fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#315]: https://gitlab.freedesktop.org/drm/intel/issues/315
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
  [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3469]: https://gitlab.freedesktop.org/drm/intel/issues/3469
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3742]: https://gitlab.freedesktop.org/drm/intel/issues/3742
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3952]: https://gitlab.freedesktop.org/drm/intel/issues/3952
  [i915#3989]: https://gitlab.freedesktop.org/drm/intel/issues/3989
  [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4281]: https://gitlab.freedesktop.org/drm/intel/issues/4281
  [i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4771]: https://gitlab.freedesktop.org/drm/intel/issues/4771
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4816]: https://gitlab.freedesktop.org/drm/intel/issues/4816
  [i915#4818]: https://gitlab.freedesktop.org/drm/intel/issues/4818
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4859]: https://gitlab.freedesktop.org/drm/intel/issues/4859
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#4880]: https://gitlab.freedesktop.org/drm/intel/issues/4880
  [i915#4881]: https://gitlab.freedesktop.org/drm/intel/issues/4881
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5234]: https://gitlab.freedesktop.org/drm/intel/issues/5234
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6301]: https://gitlab.freedesktop.org/drm/intel/issues/6301
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
  [i915#6953]: https://gitlab.freedesktop.org/drm/intel/issues/6953
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
  [i915#7701]: https://gitlab.freedesktop.org/drm/intel/issues/7701
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7790]: https://gitlab.freedesktop.org/drm/intel/issues/7790
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#7975]: https://gitlab.freedesktop.org/drm/intel/issues/7975
  [i915#8011]: https://gitlab.freedesktop.org/drm/intel/issues/8011
  [i915#8292]: https://gitlab.freedesktop.org/drm/intel/issues/8292
  [i915#8308]: https://gitlab.freedesktop.org/drm/intel/issues/8308


Build changes
-------------

  * Linux: CI_DRM_12981 -> Patchwork_112647v7

  CI-20190529: 20190529
  CI_DRM_12981: fbadfcf137737f02425a35bf3ae17a1492301f21 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7243: 402a13477510ab05591839a2bf4586de1158e60c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_112647v7: fbadfcf137737f02425a35bf3ae17a1492301f21 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_112647v7/index.html

[-- Attachment #2: Type: text/html, Size: 11528 bytes --]

Re: [Intel-gfx] [PATCH v7 3/8] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC

[Ceraolo Spurio, Daniele]

On 4/6/2023 10:44 AM, Alan Previn wrote:
> Add helper functions into a new file for heci-packet-submission.
> The helpers will handle generating the MTL GSC-CS Memory-Header
> and submission of the Heci-Cmd-Packet instructions to the engine.
>
> NOTE1: These common functions for heci-packet-submission will be used
> by different i915 callers:
>       1- GSC-SW-Proxy: This is pending upstream publication awaiting
>          a few remaining opens
>       2- MTL-HDCP: An equivalent patch has also been published at:
>          https://patchwork.freedesktop.org/series/111876/. (Patch 1)
>       3- PXP: This series.
>
> NOTE2: A difference in this patch vs what is appearing is in bullet 2
> above is that HDCP (and SW-Proxy) will be using priveleged submission
> (GGTT and common gsc-uc-context) while PXP will be using non-priveleged
> PPGTT, context and batch buffer. Therefore this patch will only slightly
> overlap with the MTL-HDCP patches despite have very similar function
> names (emit_foo vs emit_nonpriv_foo). This is because HECI_CMD_PKT
> instructions require different flows and hw-specific code when done
> via PPGTT based submission (not different from other engines). MTL-HDCP
> contains the same intel_gsc_mtl_header_t structures as this but the
> helpers there are different. Both add the same new file names.
>
> NOTE3: Additional clarity about the heci-cmd-pkt layout and where the
>         common helpers come in:
>       - On MTL, when an i915 subsystem needs to send a command request
>         to the security firmware, it will send that via the GSC-
>         engine-command-streamer.
>       - However those commands, (lets call them "gsc_specific_fw_api"
>         calls), are not understood by the GSC command streamer hw.
>       - The GSC CS only looks at the GSC_HECI_CMD_PKT instruction and
>         passes it along to the GSC firmware.
>       - The GSC FW on the other hand needs additional metadata to know
>         which usage service is being called (PXP, HDCP, proxy, etc) along
>         with session specific info. Thus an extra header called GSC-CS
>         HECI Memory Header, (C) in below diagram is prepended before
>         the FW specific API, (D).
>       - Thus, the structural layout of the request submitted would
>         need to look like the diagram below (for non-priv PXP).
>       - In the diagram, the common helper for HDCP, (GSC-Sw-Proxy) and
>         PXP (i.e. new function intel_gsc_uc_heci_cmd_emit_mtl_header)
>         will populate blob (C) while additional helpers, different for
>         PPGGTT (this patch) vs GGTT (HDCP series) will populate
>         blobs (A) and (B) below.
>        ___________________________________________________________
>   (A)  |  MI_BATCH_BUFFER_START (ppgtt, batchbuff-addr, ...)     |
>        |     |                                                   |
>        |    _|________________________________________________   |
>        | (B)| GSC_HECI_CMD_PKT (pkt-addr-in, pkt-size-in,    |   |
>        |    |                   pkt-addr-out, pkt-size-out)  |--------
>        |    | MI_BATCH_BUFFER_END                            |   |   |
>        |    |________________________________________________|   |   |
>        |                                                         |   |
>        |_________________________________________________________|   |
>                                                                      |
>              ---------------------------------------------------------
>              |
>             \|/
>        ______V___________________________________________
>        |   _________________________________________    |
>        |(C)|                                       |    |
>        |   | struct intel_gsc_mtl_header {         |    |
>        |   |   validity marker                     |    |
>        |   |   heci_clent_id                       |    |
>        |   |   ...                                 |    |
>        |   |  }                                    |    |
>        |   |_______________________________________|    |
>        |(D)|                                       |    |
>        |   | struct gsc_fw_specific_api_foobar {   |    |
>        |   |     ...                               |    |
>        |   |     For an example, see               |    |
>        |   |     'struct pxp43_create_arb_in' at   |    |
>        |   |     intel_pxp_cmd_interface_43.h      |    |
>        |   |                                       |    |
>        |   | }                                     |    |
>        |   |  Struture depends on command type     |    |
>        |   | struct gsc_fw_specific_api_foobar {   |    |
>        |   |_______________________________________|    |
>        |________________________________________________|
>
> That said, this patch provides basic helpers but leaves the
> PXP subsystem (i.e. the caller) to handle (D) and everything
> else such as input/output size verification or handling the
> responses from security firmware (for example, requiring a retry).
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c | 102 ++++++++++++++++++
>   .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h |  25 ++++-
>   2 files changed, 126 insertions(+), 1 deletion(-)
>
> 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
> index ea0da06e2f39..12c2a0e1dd1e 100644
> --- 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
> @@ -3,6 +3,7 @@
>    * Copyright © 2023 Intel Corporation
>    */
>   
> +#include "gt/intel_context.h"
>   #include "gt/intel_engine_pm.h"
>   #include "gt/intel_gpu_commands.h"
>   #include "gt/intel_gt.h"
> @@ -107,3 +108,104 @@ void intel_gsc_uc_heci_cmd_emit_mtl_header(struct intel_gsc_mtl_header *header,
>   	header->header_version = MTL_GSC_HEADER_VERSION;
>   	header->message_size = message_size;
>   }
> +
> +static void
> +emit_gsc_heci_pkt_nonpriv(u32 *cmd, struct intel_gsc_heci_non_priv_pkt *pkt)
> +{
> +	*cmd++ = GSC_HECI_CMD_PKT;
> +	*cmd++ = lower_32_bits(pkt->addr_in);
> +	*cmd++ = upper_32_bits(pkt->addr_in);
> +	*cmd++ = pkt->size_in;
> +	*cmd++ = lower_32_bits(pkt->addr_out);
> +	*cmd++ = upper_32_bits(pkt->addr_out);
> +	*cmd++ = pkt->size_out;
> +	*cmd++ = 0;
> +	*cmd++ = 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 *cmd, int timeout_ms)
> +{
> +	struct intel_engine_cs *eng;

We always use "engine" for engine_cs variables. IMO it's better to stick 
to that here as well for consistency across the code.

> +	struct i915_gem_ww_ctx ww;
> +	struct i915_request *rq;
> +	int err, trials = 0;
> +

Is the assumption that the caller is holding a wakeref already? 
Otherwise we're going to need and engine_pm_get() here (assuming it 
doesn't interfere with any locking, otherwise it has to be in the caller)

> +	i915_gem_ww_ctx_init(&ww, false);
> +retry:
> +	err = i915_gem_object_lock(pkt->bb_vma->obj, &ww);
> +	if (err)
> +		goto out_ww;
> +	err = i915_gem_object_lock(pkt->heci_pkt_vma->obj, &ww);
> +	if (err)
> +		goto out_ww;
> +	err = intel_context_pin_ww(ce, &ww);
> +	if (err)
> +		goto out_ww;
> +
> +	rq = i915_request_create(ce);
> +	if (IS_ERR(rq)) {
> +		err = PTR_ERR(rq);
> +		goto out_unpin_ce;
> +	}
> +
> +	emit_gsc_heci_pkt_nonpriv(cmd, pkt);
> +
> +	err = i915_vma_move_to_active(pkt->bb_vma, rq, EXEC_OBJECT_WRITE);

nit: I don't think we need EXEC_OBJECT_WRITE for the bb as we're not 
going to write it.

> +	if (err)
> +		goto out_rq;
> +	err = i915_vma_move_to_active(pkt->heci_pkt_vma, rq, EXEC_OBJECT_WRITE);
> +	if (err)
> +		goto out_rq;
> +
> +	eng = rq->context->engine;
> +	if (eng->emit_init_breadcrumb) {
> +		err = eng->emit_init_breadcrumb(rq);
> +		if (err)
> +			goto out_rq;
> +	}
> +
> +	err = eng->emit_bb_start(rq, i915_vma_offset(pkt->bb_vma), PAGE_SIZE, 0);
> +	if (err)
> +		goto out_rq;
> +
> +	err = ce->engine->emit_flush(rq, 0);
> +	if (err)
> +		drm_err(&gsc_uc_to_gt(gsc)->i915->drm,
> +			"Failed emit-flush for gsc-heci-non-priv-pkterr=%d\n", err);
> +
> +out_rq:
> +	i915_request_get(rq);
> +
> +	if (unlikely(err))
> +		i915_request_set_error_once(rq, err);
> +
> +	i915_request_add(rq);
> +
> +	if (!err) {
> +		if (i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE,
> +				      msecs_to_jiffies(timeout_ms)) < 0)
> +			err = -ETIME;
> +	}
> +
> +	i915_request_put(rq);
> +
> +out_unpin_ce:
> +	intel_context_unpin(ce);
> +out_ww:
> +	if (err == -EDEADLK) {
> +		err = i915_gem_ww_ctx_backoff(&ww);
> +		if (!err) {
> +			if (++trials < 10)
> +				goto retry;
> +			else
> +				err = EAGAIN;
> +		}
> +	}
> +	i915_gem_ww_ctx_fini(&ww);
> +
> +	return err;
> +}
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> index 3d56ae501991..3addce861854 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> @@ -8,7 +8,10 @@
>   
>   #include <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
> @@ -47,7 +50,7 @@ struct intel_gsc_mtl_header {
>   	 * we distinguish the flags using OUTFLAG or INFLAG
>   	 */
>   	u32 flags;
> -#define GSC_OUTFLAG_MSG_PENDING	1
> +#define GSC_OUTFLAG_MSG_PENDING 1

Nit: this change on the define is not really needed

Daniele

>   
>   	u32 status;
>   } __packed;
> @@ -58,4 +61,24 @@ int intel_gsc_uc_heci_cmd_submit_packet(struct intel_gsc_uc *gsc,
>   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);
> +
> +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

Re: [Intel-gfx] [PATCH v7 4/8] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages

[Ceraolo Spurio, Daniele]

On 4/6/2023 10:44 AM, 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.
>
> We use non-priveleged batches for submission to GSC engine
> which require two buffers for the request:
>       - a buffer for the HECI packet that contains PXP FW commands
>       - a batch-buffer that contains the engine instruction for
>         sending the HECI packet to the GSC firmware.
>
> Thus, add the allocation and freeing of these buffers in gsccs
> init and fini.
>
> The GSC-fw may reply to commands with a SUCCESS but with an
> additional pending-bit set in the reply packet. This bit
> means the GSC-FW is currently busy and the caller needs to
> try again with the gsc_message_handle the fw returned. Thus,
> add a wrapper to continuously retry send_message while
> replaying the gsc_message_handle. Retries need to follow the
> arch-spec count and delay until GSC-FW replies with the real
> SUCCESS or timeout after that spec'd delay.
>
> The GSC-fw requires a non-zero host_session_handle provided
> by the caller to enable gsc_message_handle tracking. Thus,
> allocate the host_session_handle at init and destroy it
> at fini (the latter requiring an FYI to the gsc-firmware).
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h |   1 +
>   .../drm/i915/pxp/intel_pxp_cmd_interface_43.h |   3 +
>   drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c    | 240 +++++++++++++++++-
>   drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h    |   4 +
>   drivers/gpu/drm/i915/pxp/intel_pxp_types.h    |   6 +
>   5 files changed, 253 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> index 3addce861854..e4d6662339e8 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> @@ -51,6 +51,7 @@ struct intel_gsc_mtl_header {
>   	 */
>   	u32 flags;
>   #define GSC_OUTFLAG_MSG_PENDING 1
> +#define GSC_INFLAG_MSG_CLEANUP BIT(1)

For consistency those should all be BIT() defines

>   
>   	u32 status;
>   } __packed;

<snip>

> @@ -38,18 +248,46 @@ gsccs_allocate_execution_resource(struct intel_pxp *pxp)
>   	if (!engine)
>   		return -ENODEV;
>   
> +	/*
> +	 * 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, "Heci Packet",
> +				  2 * PXP43_MAX_HECI_INOUT_SIZE,
> +				  &exec_res->pkt_vma, &exec_res->pkt_vaddr);
> +	if (err)
> +		return err;
> +
> +	err = gsccs_create_buffer(pxp->ctrl_gt, "Batch Buffer", PAGE_SIZE,
> +				  &exec_res->bb_vma, &exec_res->bb_vaddr);
> +	if (err)
> +		goto free_pkt;
> +
>   	/* Finally, create an intel_context to be used during the submission */
>   	ce = intel_context_create(engine);
>   	if (IS_ERR(ce)) {
>   		drm_err(&gt->i915->drm, "Failed creating gsccs backend ctx\n");
> -		return PTR_ERR(ce);
> +		err = PTR_ERR(ce);
> +		goto free_batch;
>   	}
>   
>   	i915_vm_put(ce->vm);
>   	ce->vm = i915_vm_get(pxp->ctrl_gt->vm);
>   	exec_res->ce = ce;
>   
> +	/* initialize host-session-handle (for all i915-to-gsc-firmware PXP cmds) */
> +	get_random_bytes(&exec_res->host_session_handle, sizeof(exec_res->host_session_handle));

Thinking back at this, maybe a possible solution to avoid randomly 
generated clashing values is to check if any of the existing exec_res 
already uses the generated value. Not a blocker because we only have 1 
exec_res for now, so no chance of clashing.

With the define fixed:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> +
>   	return 0;
> +
> +free_batch:
> +	i915_vma_unpin_and_release(&exec_res->bb_vma, I915_VMA_RELEASE_MAP);
> +free_pkt:
> +	i915_vma_unpin_and_release(&exec_res->pkt_vma, I915_VMA_RELEASE_MAP);
> +	memset(exec_res, 0, sizeof(*exec_res));
> +
> +	return err;
>   }
>   
>   void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> index 354ea9a8f940..bd1c028bc80f 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> @@ -10,6 +10,10 @@
>   
>   struct intel_pxp;
>   
> +#define GSC_REPLY_LATENCY_MS 200
> +#define GSC_PENDING_RETRY_MAXCOUNT 40
> +#define GSC_PENDING_RETRY_PAUSE_MS 50
> +
>   #ifdef CONFIG_DRM_I915_PXP
>   void intel_pxp_gsccs_fini(struct intel_pxp *pxp);
>   int intel_pxp_gsccs_init(struct intel_pxp *pxp);
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> index fdd98911968d..73392fbab7ee 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_types.h
> @@ -38,6 +38,12 @@ struct intel_pxp {
>   	struct gsccs_session_resources {
>   		u64 host_session_handle; /* used by firmware to link commands to sessions */
>   		struct intel_context *ce; /* context for gsc command submission */
> +
> +		struct i915_vma *pkt_vma; /* GSC FW cmd packet vma */
> +		void *pkt_vaddr;  /* GSC FW cmd packet virt pointer */
> +
> +		struct i915_vma *bb_vma; /* HECI_PKT batch buffer vma */
> +		void *bb_vaddr; /* HECI_PKT batch buffer virt pointer */
>   	} gsccs_res;
>   
>   	/**

Re: [Intel-gfx] [PATCH v7 5/8] drm/i915/pxp: Add ARB session creation and cleanup

[Ceraolo Spurio, Daniele]

On 4/6/2023 10:44 AM, Alan Previn wrote:
> Add MTL's function for ARB session creation using PXP firmware
> version 4.3 ABI structure format.
>
> Also add MTL's function for ARB session invalidation but this
> reuses PXP firmware version 4.2 ABI structure format.
>
> For both cases, in the back-end gsccs functions for sending messages
> to the firmware inspect the GSC-CS-Mem-Header's pending-bit which
> means the GSC firmware is busy and we should retry.
>
> Given the last hw requirement, lets also update functions in
> front-end layer that wait for session creation or teardown
> completion to use new worst case timeout periods.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c     |  10 ++
>   drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h     |   1 +
>   drivers/gpu/drm/i915/pxp/intel_pxp.c          |  28 +++-
>   .../drm/i915/pxp/intel_pxp_cmd_interface_43.h |  21 +++
>   drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c    | 130 ++++++++++++++++++
>   drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h    |   8 +-
>   drivers/gpu/drm/i915/pxp/intel_pxp_session.c  |  11 +-
>   7 files changed, 202 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> index 1d9fdfb11268..85f720af2f75 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> @@ -13,6 +13,7 @@
>   #define GSC_FW_STATUS_REG			_MMIO(0x116C40)
>   #define GSC_FW_CURRENT_STATE			REG_GENMASK(3, 0)
>   #define   GSC_FW_CURRENT_STATE_RESET		0
> +#define   GSC_FW_PROXY_STATE_NORMAL		5
>   #define GSC_FW_INIT_COMPLETE_BIT		REG_BIT(9)
>   
>   static bool gsc_is_in_reset(struct intel_uncore *uncore)
> @@ -31,6 +32,15 @@ bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc)
>   	return fw_status & GSC_FW_INIT_COMPLETE_BIT;
>   }
>   
> +bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc)
> +{
> +	struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore;
> +	u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
> +
> +	return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) ==
> +	       GSC_FW_PROXY_STATE_NORMAL;
> +}
> +
>   static int emit_gsc_fw_load(struct i915_request *rq, struct intel_gsc_uc *gsc)
>   {
>   	u32 offset = i915_ggtt_offset(gsc->local);
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> index f4c1106bb2a9..fff8928218df 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> @@ -13,5 +13,6 @@ struct intel_uncore;
>   
>   int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc);
>   bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc);
> +bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc);
>   
>   #endif
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> index 8949d4be7882..550457bbb034 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
> @@ -291,6 +291,8 @@ static bool pxp_component_bound(struct intel_pxp *pxp)
>   
>   static int __pxp_global_teardown_final(struct intel_pxp *pxp)
>   {
> +	int timeout;
> +
>   	if (!pxp->arb_is_valid)
>   		return 0;
>   	/*
> @@ -300,7 +302,12 @@ static int __pxp_global_teardown_final(struct intel_pxp *pxp)
>   	intel_pxp_mark_termination_in_progress(pxp);
>   	intel_pxp_terminate(pxp, false);
>   
> -	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
> +	if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
> +		timeout = GSCFW_MAX_ROUND_TRIP_LATENCY_MS;
> +	else
> +		timeout = 250;
> +
> +	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout)))
>   		return -ETIMEDOUT;
>   
>   	return 0;
> @@ -308,6 +315,8 @@ static int __pxp_global_teardown_final(struct intel_pxp *pxp)
>   
>   static int __pxp_global_teardown_restart(struct intel_pxp *pxp)
>   {
> +	int timeout;
> +
>   	if (pxp->arb_is_valid)
>   		return 0;
>   	/*
> @@ -316,7 +325,12 @@ static int __pxp_global_teardown_restart(struct intel_pxp *pxp)
>   	 */
>   	pxp_queue_termination(pxp);
>   
> -	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(250)))
> +	if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
> +		timeout = GSCFW_MAX_ROUND_TRIP_LATENCY_MS;
> +	else
> +		timeout = 250;
> +
> +	if (!wait_for_completion_timeout(&pxp->termination, msecs_to_jiffies(timeout)))
>   		return -ETIMEDOUT;
>   
>   	return 0;
> @@ -354,8 +368,14 @@ 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 (HAS_ENGINE(pxp->ctrl_gt, GSC0)) {
> +		/* Use a larger 1 second timeout considering all the dependencies. */
> +		if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 1000))
> +			return -ENXIO;

This needs a comment to explain that we expect userspace to retry later 
if we return -ENXIO and therefore the timeout is smaller that what would 
be required to guarantee that the init can complete. It also needs an 
ack from the userspace drivers for this behavior.


> +	} else {
> +		if (wait_for(pxp_component_bound(pxp), 250))
> +			return -ENXIO;
> +	}
>   
>   	mutex_lock(&pxp->arb_mutex);
>   
> 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 c65ada99e54f..09777719cd84 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_INOUT_SIZE (SZ_32K)
> @@ -26,4 +27,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 BIT(0)
> +		#define PXP43_INIT_SESSION_APPTYPE BIT(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 16e3b73d0653..4bc276daca16 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c
> @@ -6,13 +6,46 @@
>   #include "gem/i915_gem_internal.h"
>   
>   #include "gt/intel_context.h"
> +#include "gt/uc/intel_gsc_fw.h"
>   #include "gt/uc/intel_gsc_uc_heci_cmd_submit.h"
>   
>   #include "i915_drv.h"
> +#include "intel_pxp_cmd_interface_42.h"
>   #include "intel_pxp_cmd_interface_43.h"
>   #include "intel_pxp_gsccs.h"
>   #include "intel_pxp_types.h"
>   
> +static bool
> +is_fw_err_platform_config(u32 type)
> +{
> +	switch (type) {
> +	case PXP_STATUS_ERROR_API_VERSION:
> +	case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF:
> +	case PXP_STATUS_PLATFCONFIG_KF1_BAD:
> +		return true;
> +	default:
> +		break;
> +	}
> +	return false;
> +}
> +
> +static const char *
> +fw_err_to_string(u32 type)
> +{
> +	switch (type) {
> +	case PXP_STATUS_ERROR_API_VERSION:
> +		return "ERR_API_VERSION";
> +	case PXP_STATUS_NOT_READY:
> +		return "ERR_NOT_READY";
> +	case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF:
> +	case PXP_STATUS_PLATFCONFIG_KF1_BAD:
> +		return "ERR_PLATFORM_CONFIG";
> +	default:
> +		break;
> +	}
> +	return NULL;
> +}
> +

There is a lot of replication for this error handling, I'm wondering if 
it's worth adding a common function to handle this. Something like:

intel_pxp_header_error(u32 header, const char *op, u32 id)
{
	if (is_fw_err_platform_config(header.status)) {
		drm_info_once(&i915->drm,
			      "PXP %s-%d failed due to BIOS/SOC:0x%08x:%s\n",
			      op, id, header.status,
			      fw_err_to_string(header.status));
	} else {
		drm_dbg(&i915->drm, "PXP %s-%d failed 0x%08x:%st:\n",
			op, id, header.status,
			fw_err_to_string(header.status));
		drm_dbg(&i915->drm, "     cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n",
			header.command_id, header.api_version);
	}
}


Not a blocker.

>   static int
>   gsccs_send_message(struct intel_pxp *pxp,
>   		   void *msg_in, size_t msg_in_size,
> @@ -152,6 +185,103 @@ gsccs_send_message_retry_complete(struct intel_pxp *pxp,
>   	return ret;
>   }
>   
> +bool intel_pxp_gsccs_is_ready_for_sessions(struct intel_pxp *pxp)
> +{
> +	/*
> +	 * GSC-fw loading, HuC-fw loading, HuC-fw authentication and
> +	 * GSC-proxy init flow (requiring an mei component driver)
> +	 * must all occur first before we can start requesting for PXP
> +	 * sessions. Checking for completion on HuC authentication and
> +	 * gsc-proxy init flow (the last set of dependencies that
> +	 * are out of order) will suffice.
> +	 */
> +	if (intel_huc_is_authenticated(&pxp->ctrl_gt->uc.huc) &&
> +	    intel_gsc_uc_fw_proxy_init_done(&pxp->ctrl_gt->uc.gsc))
> +		return true;
> +
> +	return false;
> +}
> +
> +int intel_pxp_gsccs_create_session(struct intel_pxp *pxp,
> +				   int arb_session_id)
> +{
> +	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
> +	struct pxp43_create_arb_in msg_in = {0};
> +	struct pxp43_create_arb_out msg_out = {0};
> +	int ret;
> +
> +	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;
> +
> +	ret = gsccs_send_message_retry_complete(pxp,
> +						&msg_in, sizeof(msg_in),
> +						&msg_out, sizeof(msg_out), NULL);
> +	if (ret) {
> +		drm_err(&i915->drm, "Failed to init session %d, ret=[%d]\n", arb_session_id, ret);
> +	} else if (msg_out.header.status != 0) {
> +		if (is_fw_err_platform_config(msg_out.header.status)) {
> +			drm_info_once(&i915->drm,
> +				      "PXP init-session-%d failed due to BIOS/SOC:0x%08x:%s\n",
> +				      arb_session_id, msg_out.header.status,
> +				      fw_err_to_string(msg_out.header.status));
> +		} else {
> +			drm_dbg(&i915->drm, "PXP init-session-%d failed 0x%08x:%st:\n",
> +				arb_session_id, msg_out.header.status,
> +				fw_err_to_string(msg_out.header.status));
> +			drm_dbg(&i915->drm, "     cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n",
> +				msg_in.header.command_id, msg_in.header.api_version);
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +void intel_pxp_gsccs_end_arb_fw_session(struct intel_pxp *pxp, u32 session_id)
> +{
> +	struct drm_i915_private *i915 = pxp->ctrl_gt->i915;
> +	struct pxp42_inv_stream_key_in msg_in = {0};
> +	struct pxp42_inv_stream_key_out msg_out = {0};
> +	int ret = 0;
> +
> +	/*
> +	 * Stream key invalidation reuses the same version 4.2 input/output
> +	 * command format but firmware requires 4.3 API interaction
> +	 */
> +	msg_in.header.api_version = PXP_APIVER(4, 3);
> +	msg_in.header.command_id = PXP42_CMDID_INVALIDATE_STREAM_KEY;
> +	msg_in.header.buffer_len = sizeof(msg_in) - sizeof(msg_in.header);
> +
> +	msg_in.header.stream_id = FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_VALID, 1);
> +	msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_APP_TYPE, 0);
> +	msg_in.header.stream_id |= FIELD_PREP(PXP_CMDHDR_EXTDATA_SESSION_ID, session_id);
> +
> +	ret = gsccs_send_message_retry_complete(pxp,
> +						&msg_in, sizeof(msg_in),
> +						&msg_out, sizeof(msg_out), NULL);
> +	if (ret) {
> +		drm_err(&i915->drm, "Failed to inv-stream-key-%u, ret=[%d]\n",
> +			session_id, ret);
> +	} else if (msg_out.header.status != 0) {
> +		if (is_fw_err_platform_config(msg_out.header.status)) {
> +			drm_info_once(&i915->drm,
> +				      "PXP inv-stream-key-%u failed due to BIOS/SOC :0x%08x:%s\n",
> +				      session_id, msg_out.header.status,
> +				      fw_err_to_string(msg_out.header.status));
> +		} else {
> +			drm_dbg(&i915->drm, "PXP inv-stream-key-%u failed 0x%08x:%s:\n",
> +				session_id, msg_out.header.status,
> +				fw_err_to_string(msg_out.header.status));
> +			drm_dbg(&i915->drm, "     cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n",
> +				msg_in.header.command_id, msg_in.header.api_version);
> +		}
> +	}
> +}
> +
>   static void
>   gsccs_cleanup_fw_host_session_handle(struct intel_pxp *pxp)
>   {
> diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> index bd1c028bc80f..820c2def21ee 100644
> --- a/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> @@ -10,14 +10,18 @@
>   
>   struct intel_pxp;
>   
> -#define GSC_REPLY_LATENCY_MS 200
> +#define GSC_REPLY_LATENCY_MS 210

why move from 200 to 210? not a problem, I just want to understand why 
this is required.

Daniele

>   #define GSC_PENDING_RETRY_MAXCOUNT 40
>   #define GSC_PENDING_RETRY_PAUSE_MS 50
> +#define GSCFW_MAX_ROUND_TRIP_LATENCY_MS (GSC_PENDING_RETRY_MAXCOUNT * GSC_PENDING_RETRY_PAUSE_MS)
>   
>   #ifdef CONFIG_DRM_I915_PXP
>   void intel_pxp_gsccs_fini(struct intel_pxp *pxp);
>   int intel_pxp_gsccs_init(struct intel_pxp *pxp);
>   
> +int intel_pxp_gsccs_create_session(struct intel_pxp *pxp, int arb_session_id);
> +void intel_pxp_gsccs_end_arb_fw_session(struct intel_pxp *pxp, u32 arb_session_id);
> +
>   #else
>   static inline void intel_pxp_gsccs_fini(struct intel_pxp *pxp)
>   {
> @@ -30,4 +34,6 @@ static inline int intel_pxp_gsccs_init(struct intel_pxp *pxp)
>   
>   #endif
>   
> +bool intel_pxp_gsccs_is_ready_for_sessions(struct intel_pxp *pxp);
> +
>   #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 7899079e17b0..e4d8242302c5 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"
> @@ -62,7 +63,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 (HAS_ENGINE(pxp->ctrl_gt, GSC0))
> +		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(&gt->i915->drm, "tee cmd for arb session creation failed\n");
>   		return ret;
> @@ -106,7 +110,10 @@ static int pxp_terminate_arb_session_and_global(struct intel_pxp *pxp)
>   
>   	intel_uncore_write(gt->uncore, KCR_GLOBAL_TERMINATE(pxp->kcr_base), 1);
>   
> -	intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION);
> +	if (HAS_ENGINE(gt, GSC0))
> +		intel_pxp_gsccs_end_arb_fw_session(pxp, ARB_SESSION);
> +	else
> +		intel_pxp_tee_end_arb_fw_session(pxp, ARB_SESSION);
>   
>   	return ret;
>   }

Re: [Intel-gfx] [PATCH v7 6/8] drm/i915/uapi/pxp: Fix UAPI spec comments and add GET_PARAM for PXP

[Ceraolo Spurio, Daniele]

On 4/6/2023 10:44 AM, Alan Previn wrote:
> 1. UAPI update:
> Without actually changing backward compatible behavior, update
> i915's drm-uapi comments that describe the possible error values
> when creating a context with I915_CONTEXT_PARAM_PROTECTED_CONTENT.
> Since the first merge of PXP support on ADL, i915 returns
> -ENXIO if a dependency such as firmware or component driver
> was yet to be loaded or returns -EIO if the creation attempt
> failed when requested by the PXP firmware (specific firmware
> error responses are reported in dmesg).
>
> 2. GET_PARAM for PXP:
> Because of the additional firmware, component-driver and
> initialization depedencies required on MTL platform before a
> PXP context can be created, UMD calling for PXP creation as a
> way to get-caps can take a long time. An actual real world
> customer stack has seen this happen in the 4-to-8 second range
> after the kernel starts (which sees MESA's init appear in the
> middle of this range as the compositor comes up). To avoid
> unncessary delays experienced by the UMD for get-caps purposes,
> add a GET_PARAM for I915_PARAM_PXP_SUPPORT.
>
> However, some failures can still occur after all the depedencies
> are met (such as firmware init flow failure, bios configurations
> or SOC fusing not allowing PXP enablement). Those scenarios will
> only be known to user space when it attempts creating a PXP context.
>
> With this change, large delays are only met by user-space procsses
> that explicitly need to create a PXP context and boot very early.
> There is no way to avoid this today.
>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_getparam.c |  5 +++++
>   include/uapi/drm/i915_drm.h          | 22 ++++++++++++++++++++++
>   2 files changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c
> index 2238e096c957..9729384f033f 100644
> --- a/drivers/gpu/drm/i915/i915_getparam.c
> +++ b/drivers/gpu/drm/i915/i915_getparam.c
> @@ -5,6 +5,8 @@
>   #include "gem/i915_gem_mman.h"
>   #include "gt/intel_engine_user.h"
>   
> +#include "pxp/intel_pxp.h"
> +
>   #include "i915_cmd_parser.h"
>   #include "i915_drv.h"
>   #include "i915_getparam.h"
> @@ -102,6 +104,9 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
>   		if (value < 0)
>   			return value;
>   		break;
> +	case I915_PARAM_PXP_STATUS:
> +		value = intel_pxp_is_enabled(i915->pxp) ? 0 : -ENODEV;
> +		break;
>   	case I915_PARAM_MMAP_GTT_VERSION:
>   		/* Though we've started our numbering from 1, and so class all
>   		 * earlier versions as 0, in effect their value is undefined as
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index dba7c5a5b25e..0c1729bd911d 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -771,6 +771,20 @@ typedef struct drm_i915_irq_wait {
>    */
>   #define I915_PARAM_OA_TIMESTAMP_FREQUENCY 57
>   
> +/*
> + * Query the status of PXP support in i915.
> + *
> + * The query can fail in the following scenarios with the listed error codes:
> + *  -ENODEV = PXP support is not available on the GPU device or in the kernel
> + *            due to missing component drivers or kernel configs.
> + * If the IOCTL is successful, the returned parameter will be set to one of the
> + * following values:
> + *   0 = PXP support maybe available but underlying SOC fusing, BIOS or firmware
> + *       configuration is unknown and a PXP-context-creation would be required
> + *       for final verification of feature availibility.

Would it be useful to add:

1 = PXP support is available

And start returning that after we've successfully created our first 
session? Not sure if userspace would use this though, since they still 
need to handle the 0 case anyway.
I'm also ok with this patch as-is, as long as you get an ack from the 
userspace drivers for this interface behavior:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> + */
> +#define I915_PARAM_PXP_STATUS		 58
> +
>   /* Must be kept compact -- no holes and well documented */
>   
>   /**
> @@ -2096,6 +2110,14 @@ struct drm_i915_gem_context_param {
>    *
>    * -ENODEV: feature not available
>    * -EPERM: trying to mark a recoverable or not bannable context as protected
> + * -ENXIO: A dependency such as a component driver or firmware is not yet
> + *         loaded and user space may attempt again. Depending on the device
> + *         this error may be reported if the protected context creation is
> + *         attempted very early from kernel start (numbers vary depending on
> + *         system and kernel config):
> + *            - ADL/RPL: up to 3 seconds
> + *            - MTL: up to 8 seconds
> + * -EIO: The firmware did not succeed in creating the protected context.
>    */
>   #define I915_CONTEXT_PARAM_PROTECTED_CONTENT    0xd
>   /* Must be kept compact -- no holes and well documented */

Re: [Intel-gfx] [PATCH v7 6/8] drm/i915/uapi/pxp: Fix UAPI spec comments and add GET_PARAM for PXP

[Teres Alexis, Alan Previn]

Hi Lionel, does this patch work for you?

On Mon, 2023-04-10 at 10:22 -0700, Ceraolo Spurio, Daniele wrote:
> On 4/6/2023 10:44 AM, Alan Previn wrote:
alan:snip

> > +/*
> > + * Query the status of PXP support in i915.
> > + *
> > + * The query can fail in the following scenarios with the listed error codes:
> > + *  -ENODEV = PXP support is not available on the GPU device or in the kernel
> > + *            due to missing component drivers or kernel configs.
> > + * If the IOCTL is successful, the returned parameter will be set to one of the
> > + * following values:
> > + *   0 = PXP support maybe available but underlying SOC fusing, BIOS or firmware
> > + *       configuration is unknown and a PXP-context-creation would be required
> > + *       for final verification of feature availibility.
> 
> Would it be useful to add:
> 
> 1 = PXP support is available
> 
> And start returning that after we've successfully created our first 
> session? Not sure if userspace would use this though, since they still 
> need to handle the 0 case anyway.
> I'm also ok with this patch as-is, as long as you get an ack from the 
> userspace drivers for this interface behavior:
> 
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> Daniele

alan:snip

Re: [Intel-gfx] [PATCH v7 3/8] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC

[Teres Alexis, Alan Previn]

On Mon, 2023-04-10 at 09:10 -0700, Ceraolo Spurio, Daniele wrote:
> 
alan:snip

> > +int
> > +intel_gsc_uc_heci_cmd_submit_nonpriv(struct intel_gsc_uc *gsc,
> > +				     struct intel_context *ce,
> > +				     struct intel_gsc_heci_non_priv_pkt *pkt,
> > +				     u32 *cmd, int timeout_ms)
> > +{
> > +	struct intel_engine_cs *eng;
> 
> We always use "engine" for engine_cs variables. IMO it's better to stick 
> to that here as well for consistency across the code.
alan: will do
> 
> > +	struct i915_gem_ww_ctx ww;
> > +	struct i915_request *rq;
> > +	int err, trials = 0;
> > +
> 
> Is the assumption that the caller is holding a wakeref already? 
> Otherwise we're going to need and engine_pm_get() here (assuming it 
> doesn't interfere with any locking, otherwise it has to be in the caller)
alan: right now the only places this can get called from is via intel_pxp_gsccs_create_session or
intel_pxp_gsccs_end_arb_fw_session. These functions are either being called by intel_pxp_start
or intel_pxp_end. intel_pxp_start calls intel_runtime_pm_get_if_in_use indirectly from the
session-worker and while intel_pxp_end takes an explicit intel_runtime_pm_get (since it is
for suspend/shutdown cleanup and doesn't use the worker). I'm assuming runtime_pm works right?
we have a similar logic across the paths from ADL version where we dont take explicit
engine_pm_get for the termination via VDBOX because its part of the same code paths.

alan:snip

> > +	err = i915_vma_move_to_active(pkt->bb_vma, rq, EXEC_OBJECT_WRITE);
> 
> nit: I don't think we need EXEC_OBJECT_WRITE for the bb as we're not 
> going to write it.
alan: yes - will remove. (had accidentally kept above flag from offline
debugging version of the code that had additional store dwords into bb).

> 
> > +	if (err)
> > +		goto out_rq;
> > +	err = i915_vma_move_to_active(pkt->heci_pkt_vma, rq, EXEC_OBJECT_WRITE);
> > +	if (err)
> > +		goto out_rq;
> > +
> > +	eng = rq->context->engine;
> > +	if (eng->emit_init_breadcrumb) {
> > +		err = eng->emit_init_breadcrumb(rq);
> > +		if (err)
> > +			goto out_rq;
> > +	}
> > +
> > +	err = eng->emit_bb_start(rq, i915_vma_offset(pkt->bb_vma), PAGE_SIZE, 0);
> > +	if (err)
> > +		goto out_rq;
> > +
> > +	err = ce->engine->emit_flush(rq, 0);
> > +	if (err)
> > +		drm_err(&gsc_uc_to_gt(gsc)->i915->drm,
> > +			"Failed emit-flush for gsc-heci-non-priv-pkterr=%d\n", err);
> > +
> > +out_rq:
> > +	i915_request_get(rq);
> > +
> > +	if (unlikely(err))
> > +		i915_request_set_error_once(rq, err);
> > +
> > +	i915_request_add(rq);
> > +
> > +	if (!err) {
> > +		if (i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE,
> > +				      msecs_to_jiffies(timeout_ms)) < 0)
> > +			err = -ETIME;
> > +	}
> > +
> > +	i915_request_put(rq);
> > +
> > +out_unpin_ce:
> > +	intel_context_unpin(ce);
> > +out_ww:
> > +	if (err == -EDEADLK) {
> > +		err = i915_gem_ww_ctx_backoff(&ww);
> > +		if (!err) {
> > +			if (++trials < 10)
> > +				goto retry;
> > +			else
> > +				err = EAGAIN;
> > +		}
> > +	}
> > +	i915_gem_ww_ctx_fini(&ww);
> > +
> > +	return err;
> > +}
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> > index 3d56ae501991..3addce861854 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
> > @@ -8,7 +8,10 @@
> >   
> >   #include <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
> > @@ -47,7 +50,7 @@ struct intel_gsc_mtl_header {
> >   	 * we distinguish the flags using OUTFLAG or INFLAG
> >   	 */
> >   	u32 flags;
> > -#define GSC_OUTFLAG_MSG_PENDING	1
> > +#define GSC_OUTFLAG_MSG_PENDING 1
> 
> Nit: this change on the define is not really needed
sure - will fix.
> 
> Daniele

Re: [Intel-gfx] [PATCH v7 4/8] drm/i915/pxp: Add GSC-CS backend to send GSC fw messages

[Teres Alexis, Alan Previn]

On Mon, 2023-04-10 at 09:28 -0700, Ceraolo Spurio, Daniele wrote:
> 
alan:snip
> >   #define GSC_OUTFLAG_MSG_PENDING 1
> > +#define GSC_INFLAG_MSG_CLEANUP BIT(1)
> 
> For consistency those should all be BIT() defines
alan: will do.
> 
> With the define fixed:
> 
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> Daniele
> 
alan: thanks Daniele.

Re: [Intel-gfx] [PATCH v7 5/8] drm/i915/pxp: Add ARB session creation and cleanup

[Teres Alexis, Alan Previn]

On Mon, 2023-04-10 at 10:14 -0700, Ceraolo Spurio, Daniele wrote:
> 
> 
> 
alan:snip

> > @@ -354,8 +368,14 @@ 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 (HAS_ENGINE(pxp->ctrl_gt, GSC0)) {
> > +		/* Use a larger 1 second timeout considering all the dependencies. */
> > +		if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 1000))
> > +			return -ENXIO;
> 
> This needs a comment to explain that we expect userspace to retry later 
> if we return -ENXIO and therefore the timeout is smaller that what would 
> be required to guarantee that the init can complete. It also needs an 
> ack from the userspace drivers for this behavior.
> 
alan: I agree with a requirement to comment this down. However, i believe its better
to put this int the UAPI header file comment-documentation since it applies to both
current MTL as well as previous ADL cases (this is not new behavior being introduced
in MTL but it is fixing of the spec of existing behavior).
That said, your feedback is already being addressed by patch #6 but i will ammend
patch #6 to add the detail you highlighted WRT ~"timeout is not larger than the completion-guarantee...".

> 
alan:snip
> > +fw_err_to_string(u32 type)
> > +{
> > +	switch (type) {
> > +	case PXP_STATUS_ERROR_API_VERSION:
> > +		return "ERR_API_VERSION";
> > +	case PXP_STATUS_NOT_READY:
> > +		return "ERR_NOT_READY";
> > +	case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF:
> > +	case PXP_STATUS_PLATFCONFIG_KF1_BAD:
> > +		return "ERR_PLATFORM_CONFIG";
> > +	default:
> > +		break;
> > +	}
> > +	return NULL;
> > +}
> > +
> 
> There is a lot of replication for this error handling, I'm wondering if 
> it's worth adding a common function to handle this. Something like:
> 
> intel_pxp_header_error(u32 header, const char *op, u32 id)
> {
> 	if (is_fw_err_platform_config(header.status)) {
> 		drm_info_once(&i915->drm,
> 			      "PXP %s-%d failed due to BIOS/SOC:0x%08x:%s\n",
> 			      op, id, header.status,
> 			      fw_err_to_string(header.status));
> 	} else {
> 		drm_dbg(&i915->drm, "PXP %s-%d failed 0x%08x:%st:\n",
> 			op, id, header.status,
> 			fw_err_to_string(header.status));
> 		drm_dbg(&i915->drm, "     cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n",
> 			header.command_id, header.api_version);
> 	}
> }
> 
> 
> Not a blocker.
alan: Thanks - i will have to address as a stand alone patch since i have to
think about moving this to a comment helper layer (common to both ADL-mei-comp and MTL-gsccs)
while potentially have different set of error codes that can mean different reporting levels
(i.e. notice once coz its a platform limit vs always err out if its runtime related).
Once this series gets merged it will be easier to work on that patch (which would require both
backends to be present in the baseline).
> > 
alan:snip
> > +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
> > @@ -10,14 +10,18 @@
> >   
> >   struct intel_pxp;
> >   
> > -#define GSC_REPLY_LATENCY_MS 200
> > +#define GSC_REPLY_LATENCY_MS 210
> 
> why move from 200 to 210? not a problem, I just want to understand why 
> this is required.
> 
> Daniele
alan: so 200 is based on the fw spec - and from real testing i observed the occasional highs touching ~185ms.
However, the spec gives this number as the expected max time the firmware is going to take to process the request
and post a reply. Therefore it doesn't include the additional overhead for the request creation, emision,
submission via guc and the completion pipeline completion indication. All of these contribute additional layers
of possible delay. Since arb-session creation and invalidation are not common events,
I believe a slightly wider overhead of 10 milisec will not hurt.

Re: [Intel-gfx] [PATCH v7 5/8] drm/i915/pxp: Add ARB session creation and cleanup

[Ceraolo Spurio, Daniele]

On 4/17/2023 11:21 AM, Teres Alexis, Alan Previn wrote:
> On Mon, 2023-04-10 at 10:14 -0700, Ceraolo Spurio, Daniele wrote:
>>
>>
> alan:snip
>
>>> @@ -354,8 +368,14 @@ 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 (HAS_ENGINE(pxp->ctrl_gt, GSC0)) {
>>> +		/* Use a larger 1 second timeout considering all the dependencies. */
>>> +		if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 1000))
>>> +			return -ENXIO;
>> This needs a comment to explain that we expect userspace to retry later
>> if we return -ENXIO and therefore the timeout is smaller that what would
>> be required to guarantee that the init can complete. It also needs an
>> ack from the userspace drivers for this behavior.
>>
> alan: I agree with a requirement to comment this down. However, i believe its better
> to put this int the UAPI header file comment-documentation since it applies to both
> current MTL as well as previous ADL cases (this is not new behavior being introduced
> in MTL but it is fixing of the spec of existing behavior).
> That said, your feedback is already being addressed by patch #6 but i will ammend
> patch #6 to add the detail you highlighted WRT ~"timeout is not larger than the completion-guarantee...".

Can you move that comment update for the context getparam from the next 
patch to this one? that way it's all nicely self-contained in this patch.

> alan:snip
>>> +fw_err_to_string(u32 type)
>>> +{
>>> +	switch (type) {
>>> +	case PXP_STATUS_ERROR_API_VERSION:
>>> +		return "ERR_API_VERSION";
>>> +	case PXP_STATUS_NOT_READY:
>>> +		return "ERR_NOT_READY";
>>> +	case PXP_STATUS_PLATFCONFIG_KF1_NOVERIF:
>>> +	case PXP_STATUS_PLATFCONFIG_KF1_BAD:
>>> +		return "ERR_PLATFORM_CONFIG";
>>> +	default:
>>> +		break;
>>> +	}
>>> +	return NULL;
>>> +}
>>> +
>> There is a lot of replication for this error handling, I'm wondering if
>> it's worth adding a common function to handle this. Something like:
>>
>> intel_pxp_header_error(u32 header, const char *op, u32 id)
>> {
>> 	if (is_fw_err_platform_config(header.status)) {
>> 		drm_info_once(&i915->drm,
>> 			      "PXP %s-%d failed due to BIOS/SOC:0x%08x:%s\n",
>> 			      op, id, header.status,
>> 			      fw_err_to_string(header.status));
>> 	} else {
>> 		drm_dbg(&i915->drm, "PXP %s-%d failed 0x%08x:%st:\n",
>> 			op, id, header.status,
>> 			fw_err_to_string(header.status));
>> 		drm_dbg(&i915->drm, "     cmd-detail: ID=[0x%08x],API-Ver-[0x%08x]\n",
>> 			header.command_id, header.api_version);
>> 	}
>> }
>>
>>
>> Not a blocker.
> alan: Thanks - i will have to address as a stand alone patch since i have to
> think about moving this to a comment helper layer (common to both ADL-mei-comp and MTL-gsccs)
> while potentially have different set of error codes that can mean different reporting levels
> (i.e. notice once coz its a platform limit vs always err out if its runtime related).
> Once this series gets merged it will be easier to work on that patch (which would require both
> backends to be present in the baseline).
> alan:snip
>>> +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.h
>>> @@ -10,14 +10,18 @@
>>>    
>>>    struct intel_pxp;
>>>    
>>> -#define GSC_REPLY_LATENCY_MS 200
>>> +#define GSC_REPLY_LATENCY_MS 210
>> why move from 200 to 210? not a problem, I just want to understand why
>> this is required.
>>
>> Daniele
> alan: so 200 is based on the fw spec - and from real testing i observed the occasional highs touching ~185ms.
> However, the spec gives this number as the expected max time the firmware is going to take to process the request
> and post a reply. Therefore it doesn't include the additional overhead for the request creation, emision,
> submission via guc and the completion pipeline completion indication. All of these contribute additional layers
> of possible delay. Since arb-session creation and invalidation are not common events,
> I believe a slightly wider overhead of 10 milisec will not hurt.

Agreed. Can you add a comment? something like "Max FW response time is 
200ms, to which we add 10ms to account for overhead".
With those 2 nits addressed:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

>

Re: [Intel-gfx] [PATCH v7 5/8] drm/i915/pxp: Add ARB session creation and cleanup

[Teres Alexis, Alan Previn]

On Mon, 2023-04-17 at 12:15 -0700, Ceraolo Spurio, Daniele wrote:
> On 4/17/2023 11:21 AM, Teres Alexis, Alan Previn wrote:
> > On Mon, 2023-04-10 at 10:14 -0700, Ceraolo Spurio, Daniele wrote:
> > > > @@ -354,8 +368,14 @@ 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 (HAS_ENGINE(pxp->ctrl_gt, GSC0)) {
> > > > +		/* Use a larger 1 second timeout considering all the dependencies. */
> > > > +		if (wait_for(intel_pxp_gsccs_is_ready_for_sessions(pxp), 1000))
> > > > +			return -ENXIO;
> > > This needs a comment to explain that we expect userspace to retry later
> > > if we return -ENXIO and therefore the timeout is smaller that what would
> > > be required to guarantee that the init can complete. It also needs an
> > > ack from the userspace drivers for this behavior.
> > > 
> > alan: I agree with a requirement to comment this down. However, i believe its better
> > to put this int the UAPI header file comment-documentation since it applies to both
> > current MTL as well as previous ADL cases (this is not new behavior being introduced
> > in MTL but it is fixing of the spec of existing behavior).
> > That said, your feedback is already being addressed by patch #6 but i will ammend
> > patch #6 to add the detail you highlighted WRT ~"timeout is not larger than the completion-guarantee...".
> 
> Can you move that comment update for the context getparam from the next 
> patch to this one? that way it's all nicely self-contained in this patch.
alan: sounds good - will do
alan:snip

> > > > 
> > > > -#define GSC_REPLY_LATENCY_MS 200
> > > > +#define GSC_REPLY_LATENCY_MS 210
> > > why move from 200 to 210? not a problem, I just want to understand why
> > > this is required.
> > > 
> > > Daniele
> > alan: so 200 is based on the fw spec - and from real testing i observed the occasional highs touching ~185ms.
> > However, the spec gives this number as the expected max time the firmware is going to take to process the request
> > and post a reply. Therefore it doesn't include the additional overhead for the request creation, emision,
> > submission via guc and the completion pipeline completion indication. All of these contribute additional layers
> > of possible delay. Since arb-session creation and invalidation are not common events,
> > I believe a slightly wider overhead of 10 milisec will not hurt.
> 
> Agreed. Can you add a comment? something like "Max FW response time is 
> 200ms, to which we add 10ms to account for overhead".
alan: will do and thanks for the conditional rb. will fix these accordingly.

> With those 2 nits addressed:
> 
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> Daniele

Re: [Intel-gfx] [PATCH v7 6/8] drm/i915/uapi/pxp: Fix UAPI spec comments and add GET_PARAM for PXP

[Lionel Landwerlin]

On 14/04/2023 18:17, Teres Alexis, Alan Previn wrote:
> Hi Lionel, does this patch work for you?


Hi, Sorry for the late answer.

That looks good :


Acked-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>


Thanks,


-Lionel


>
> On Mon, 2023-04-10 at 10:22 -0700, Ceraolo Spurio, Daniele wrote:
>> On 4/6/2023 10:44 AM, Alan Previn wrote:
> alan:snip
>
>>> +/*
>>> + * Query the status of PXP support in i915.
>>> + *
>>> + * The query can fail in the following scenarios with the listed error codes:
>>> + *  -ENODEV = PXP support is not available on the GPU device or in the kernel
>>> + *            due to missing component drivers or kernel configs.
>>> + * If the IOCTL is successful, the returned parameter will be set to one of the
>>> + * following values:
>>> + *   0 = PXP support maybe available but underlying SOC fusing, BIOS or firmware
>>> + *       configuration is unknown and a PXP-context-creation would be required
>>> + *       for final verification of feature availibility.
>> Would it be useful to add:
>>
>> 1 = PXP support is available
>>
>> And start returning that after we've successfully created our first
>> session? Not sure if userspace would use this though, since they still
>> need to handle the 0 case anyway.
>> I'm also ok with this patch as-is, as long as you get an ack from the
>> userspace drivers for this interface behavior:
>>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>
>> Daniele
> alan:snip
>

Re: [Intel-gfx] [PATCH v7 6/8] drm/i915/uapi/pxp: Fix UAPI spec comments and add GET_PARAM for PXP

[Teres Alexis, Alan Previn]

On Tue, 2023-04-18 at 21:06 +0300, Landwerlin, Lionel G wrote:
> On 14/04/2023 18:17, Teres Alexis, Alan Previn wrote:
> > Hi Lionel, does this patch work for you?
> 
> 
> Hi, Sorry for the late answer.
> 
> That looks good :
alan: Great - thanks Lionel! :) I'll help get the MESA side PR out in coming days.

> 
> Acked-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> 
> 
> Thanks,
> 
> 
> -Lionel

alan:snip

Re: [Intel-gfx] [PATCH v7 3/8] drm/i915/pxp: Add MTL helpers to submit Heci-Cmd-Packet to GSC

[Ceraolo Spurio, Daniele]

On 4/17/2023 10:56 AM, Teres Alexis, Alan Previn wrote:
> On Mon, 2023-04-10 at 09:10 -0700, Ceraolo Spurio, Daniele wrote:
> alan:snip
>
>>> +int
>>> +intel_gsc_uc_heci_cmd_submit_nonpriv(struct intel_gsc_uc *gsc,
>>> +				     struct intel_context *ce,
>>> +				     struct intel_gsc_heci_non_priv_pkt *pkt,
>>> +				     u32 *cmd, int timeout_ms)
>>> +{
>>> +	struct intel_engine_cs *eng;
>> We always use "engine" for engine_cs variables. IMO it's better to stick
>> to that here as well for consistency across the code.
> alan: will do
>>> +	struct i915_gem_ww_ctx ww;
>>> +	struct i915_request *rq;
>>> +	int err, trials = 0;
>>> +
>> Is the assumption that the caller is holding a wakeref already?
>> Otherwise we're going to need and engine_pm_get() here (assuming it
>> doesn't interfere with any locking, otherwise it has to be in the caller)
> alan: right now the only places this can get called from is via intel_pxp_gsccs_create_session or
> intel_pxp_gsccs_end_arb_fw_session. These functions are either being called by intel_pxp_start
> or intel_pxp_end. intel_pxp_start calls intel_runtime_pm_get_if_in_use indirectly from the
> session-worker and while intel_pxp_end takes an explicit intel_runtime_pm_get (since it is
> for suspend/shutdown cleanup and doesn't use the worker). I'm assuming runtime_pm works right?
> we have a similar logic across the paths from ADL version where we dont take explicit
> engine_pm_get for the termination via VDBOX because its part of the same code paths.

rpm_get works for the power management side, but not for "activeness" 
tracking, for which we need engine_pm_get. However, I've just realized 
that the context_pin contains an engine_pm_get, so we're covered.

Therefore, with the other minor comments addressed, this is:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

>
> alan:snip
>
>>> +	err = i915_vma_move_to_active(pkt->bb_vma, rq, EXEC_OBJECT_WRITE);
>> nit: I don't think we need EXEC_OBJECT_WRITE for the bb as we're not
>> going to write it.
> alan: yes - will remove. (had accidentally kept above flag from offline
> debugging version of the code that had additional store dwords into bb).
>
>>> +	if (err)
>>> +		goto out_rq;
>>> +	err = i915_vma_move_to_active(pkt->heci_pkt_vma, rq, EXEC_OBJECT_WRITE);
>>> +	if (err)
>>> +		goto out_rq;
>>> +
>>> +	eng = rq->context->engine;
>>> +	if (eng->emit_init_breadcrumb) {
>>> +		err = eng->emit_init_breadcrumb(rq);
>>> +		if (err)
>>> +			goto out_rq;
>>> +	}
>>> +
>>> +	err = eng->emit_bb_start(rq, i915_vma_offset(pkt->bb_vma), PAGE_SIZE, 0);
>>> +	if (err)
>>> +		goto out_rq;
>>> +
>>> +	err = ce->engine->emit_flush(rq, 0);
>>> +	if (err)
>>> +		drm_err(&gsc_uc_to_gt(gsc)->i915->drm,
>>> +			"Failed emit-flush for gsc-heci-non-priv-pkterr=%d\n", err);
>>> +
>>> +out_rq:
>>> +	i915_request_get(rq);
>>> +
>>> +	if (unlikely(err))
>>> +		i915_request_set_error_once(rq, err);
>>> +
>>> +	i915_request_add(rq);
>>> +
>>> +	if (!err) {
>>> +		if (i915_request_wait(rq, I915_WAIT_INTERRUPTIBLE,
>>> +				      msecs_to_jiffies(timeout_ms)) < 0)
>>> +			err = -ETIME;
>>> +	}
>>> +
>>> +	i915_request_put(rq);
>>> +
>>> +out_unpin_ce:
>>> +	intel_context_unpin(ce);
>>> +out_ww:
>>> +	if (err == -EDEADLK) {
>>> +		err = i915_gem_ww_ctx_backoff(&ww);
>>> +		if (!err) {
>>> +			if (++trials < 10)
>>> +				goto retry;
>>> +			else
>>> +				err = EAGAIN;
>>> +		}
>>> +	}
>>> +	i915_gem_ww_ctx_fini(&ww);
>>> +
>>> +	return err;
>>> +}
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
>>> index 3d56ae501991..3addce861854 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h
>>> @@ -8,7 +8,10 @@
>>>    
>>>    #include <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
>>> @@ -47,7 +50,7 @@ struct intel_gsc_mtl_header {
>>>    	 * we distinguish the flags using OUTFLAG or INFLAG
>>>    	 */
>>>    	u32 flags;
>>> -#define GSC_OUTFLAG_MSG_PENDING	1
>>> +#define GSC_OUTFLAG_MSG_PENDING 1
>> Nit: this change on the define is not really needed
> sure - will fix.
>> Daniele