[PATCH v2 0/6] drm/i915: Add support for GSC FW loading

[PATCH v2 0/6] drm/i915: Add support for GSC FW loading

[Daniele Ceraolo Spurio]

Starting from MTL, the GSC FW is runtime loaded by the driver, instead
of being stored in flash memory. Loading the GSC FW is required to allow
the media GT to go into its C6 state and for content protection features
(PXP, HDCP).

The loading happens via a submission to the GSC engine. All subsequent
communication with the FW will also happen via the engine, although no
further messages are implemented as part of this series.

This series also adds the GSC engine flag to the MTL platform, with the
engine being runtime disabled if the FW is not selected, which makes the
FW definition (not included in the series) the ultimate enabler for the
whole GSC block.

Note that just loading the FW is not enough for it to be fully
functional. We'll also need to establish and handle a communication
channel between GSC and CSME (a.k.a. SW proxy). This will require a new
mei component to handle the CSME side of things, so it will be pushed as
a separate series.

v2: Use wq to load the GSC, address minor review comments.

Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

Daniele Ceraolo Spurio (5):
  drm/i915/uc: Introduce GSC FW
  drm/i915/gsc: Skip the version check when fetching the GSC FW
  drm/i915/gsc: GSC firmware loading
  drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded
  drm/i915/mtl: MTL has one GSC CS on the media GT

Jonathan Cavitt (1):
  drm/i915/gsc: Disable GSC engine and power well if FW is not selected

 drivers/gpu/drm/i915/Makefile                |  11 +-
 drivers/gpu/drm/i915/gt/intel_engine.h       |   2 +
 drivers/gpu/drm/i915/gt/intel_engine_cs.c    |  18 ++
 drivers/gpu/drm/i915/gt/intel_gpu_commands.h |   7 +
 drivers/gpu/drm/i915/gt/intel_gt.h           |   5 +
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c    | 210 +++++++++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h    |  15 ++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c    | 137 ++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h    |  47 +++++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c        |  23 ++
 drivers/gpu/drm/i915/gt/uc/intel_uc.h        |   3 +
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c     |  78 +++++--
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h     |   8 +-
 drivers/gpu/drm/i915/i915_params.c           |   3 +
 drivers/gpu/drm/i915/i915_params.h           |   1 +
 drivers/gpu/drm/i915/i915_pci.c              |   2 +-
 drivers/gpu/drm/i915/i915_reg.h              |   3 +
 drivers/gpu/drm/i915/intel_uncore.c          |  61 ++++++
 drivers/gpu/drm/i915/intel_uncore.h          |  13 ++
 19 files changed, 626 insertions(+), 21 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h

-- 
2.37.3

[PATCH v2 1/6] drm/i915/uc: Introduce GSC FW

[Daniele Ceraolo Spurio]

On MTL the GSC FW needs to be loaded on the media GT by the graphics
driver. We're going to treat it like a new uc_fw, so add the initial
defs and init/fini functions for it.

Similarly to the other FWs, the GSC FW path can be overriden via
modparam. The modparam can also be used to disable the GSC FW loading by
setting it to an empty string.

Note that the new structure has been called intel_gsc_uc to avoid
confusion with the existing intel_gsc, which instead represents the heci
gsc interfaces.

v2: re-order Makefile list to be properly sorted (Jani, Alan), better
    comment (alan)

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com> #v1
---
 drivers/gpu/drm/i915/Makefile             | 10 ++--
 drivers/gpu/drm/i915/gt/intel_gt.h        |  5 ++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 70 +++++++++++++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 36 ++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c     | 17 ++++++
 drivers/gpu/drm/i915/gt/uc/intel_uc.h     |  3 +
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  | 29 +++++++++-
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  |  7 ++-
 drivers/gpu/drm/i915/i915_params.c        |  3 +
 drivers/gpu/drm/i915/i915_params.h        |  1 +
 10 files changed, 172 insertions(+), 9 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 01974b82d205..69f6e9af1b56 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -188,9 +188,8 @@ i915-y += \
 	  i915_vma_resource.o
 
 # general-purpose microcontroller (GuC) support
-i915-y += gt/uc/intel_uc.o \
-	  gt/uc/intel_uc_debugfs.o \
-	  gt/uc/intel_uc_fw.o \
+i915-y += \
+	  gt/uc/intel_gsc_uc.o \
 	  gt/uc/intel_guc.o \
 	  gt/uc/intel_guc_ads.o \
 	  gt/uc/intel_guc_capture.o \
@@ -205,7 +204,10 @@ i915-y += gt/uc/intel_uc.o \
 	  gt/uc/intel_guc_submission.o \
 	  gt/uc/intel_huc.o \
 	  gt/uc/intel_huc_debugfs.o \
-	  gt/uc/intel_huc_fw.o
+	  gt/uc/intel_huc_fw.o \
+	  gt/uc/intel_uc.o \
+	  gt/uc/intel_uc_debugfs.o \
+	  gt/uc/intel_uc_fw.o
 
 # graphics system controller (GSC) support
 i915-y += gt/intel_gsc.o
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h
index e0365d556248..d2f4fbde5f9f 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt.h
@@ -39,6 +39,11 @@ static inline struct intel_gt *huc_to_gt(struct intel_huc *huc)
 	return container_of(huc, struct intel_gt, uc.huc);
 }
 
+static inline struct intel_gt *gsc_uc_to_gt(struct intel_gsc_uc *gsc_uc)
+{
+	return container_of(gsc_uc, struct intel_gt, uc.gsc);
+}
+
 static inline struct intel_gt *gsc_to_gt(struct intel_gsc *gsc)
 {
 	return container_of(gsc, struct intel_gt, gsc);
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
new file mode 100644
index 000000000000..65cbf1ce9fa1
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include <linux/types.h>
+
+#include "gt/intel_gt.h"
+#include "intel_gsc_uc.h"
+#include "i915_drv.h"
+
+static bool gsc_engine_supported(struct intel_gt *gt)
+{
+	intel_engine_mask_t mask;
+
+	/*
+	 * We reach here from i915_driver_early_probe for the primary GT before
+	 * its engine mask is set, so we use the device info engine mask for it.
+	 * For other GTs we expect the GT-specific mask to be set before we
+	 * call this function.
+	 */
+	GEM_BUG_ON(!gt_is_root(gt) && !gt->info.engine_mask);
+
+	if (gt_is_root(gt))
+		mask = RUNTIME_INFO(gt->i915)->platform_engine_mask;
+	else
+		mask = gt->info.engine_mask;
+
+	return __HAS_ENGINE(mask, GSC0);
+}
+
+void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc)
+{
+	intel_uc_fw_init_early(&gsc->fw, INTEL_UC_FW_TYPE_GSC);
+
+	/* we can arrive here from i915_driver_early_probe for primary
+	 * GT with it being not fully setup hence check device info's
+	 * engine mask
+	 */
+	if (!gsc_engine_supported(gsc_uc_to_gt(gsc))){
+		intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_NOT_SUPPORTED);
+		return;
+	}
+}
+
+int intel_gsc_uc_init(struct intel_gsc_uc *gsc)
+{
+	struct drm_i915_private *i915 = gsc_uc_to_gt(gsc)->i915;
+	int err;
+
+	err = intel_uc_fw_init(&gsc->fw);
+	if (err)
+		goto out;
+
+	intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_LOADABLE);
+
+	return 0;
+
+out:
+	i915_probe_error(i915, "failed with %d\n", err);
+	return err;
+}
+
+void intel_gsc_uc_fini(struct intel_gsc_uc *gsc)
+{
+	if (!intel_uc_fw_is_loadable(&gsc->fw))
+		return;
+
+	intel_uc_fw_fini(&gsc->fw);
+}
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
new file mode 100644
index 000000000000..ea2b1c0713b8
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef _INTEL_GSC_UC_H_
+#define _INTEL_GSC_UC_H_
+
+#include "intel_uc_fw.h"
+
+struct intel_gsc_uc {
+	/* Generic uC firmware management */
+	struct intel_uc_fw fw;
+};
+
+void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc);
+int intel_gsc_uc_init(struct intel_gsc_uc *gsc);
+void intel_gsc_uc_fini(struct intel_gsc_uc *gsc);
+
+static inline bool intel_gsc_uc_is_supported(struct intel_gsc_uc *gsc)
+{
+	return intel_uc_fw_is_supported(&gsc->fw);
+}
+
+static inline bool intel_gsc_uc_is_wanted(struct intel_gsc_uc *gsc)
+{
+	return intel_uc_fw_is_enabled(&gsc->fw);
+}
+
+static inline bool intel_gsc_uc_is_used(struct intel_gsc_uc *gsc)
+{
+	GEM_BUG_ON(__intel_uc_fw_status(&gsc->fw) == INTEL_UC_FIRMWARE_SELECTED);
+	return intel_uc_fw_is_available(&gsc->fw);
+}
+
+#endif
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 4f4b519e12c1..6f74586f87d8 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -7,6 +7,7 @@
 
 #include "gt/intel_gt.h"
 #include "gt/intel_reset.h"
+#include "intel_gsc_uc.h"
 #include "intel_guc.h"
 #include "intel_guc_ads.h"
 #include "intel_guc_submission.h"
@@ -126,6 +127,7 @@ void intel_uc_init_early(struct intel_uc *uc)
 
 	intel_guc_init_early(&uc->guc);
 	intel_huc_init_early(&uc->huc);
+	intel_gsc_uc_init_early(&uc->gsc);
 
 	__confirm_options(uc);
 
@@ -296,15 +298,26 @@ static void __uc_fetch_firmwares(struct intel_uc *uc)
 						  INTEL_UC_FIRMWARE_ERROR);
 		}
 
+		if (intel_uc_wants_gsc_uc(uc)) {
+			drm_dbg(&uc_to_gt(uc)->i915->drm,
+				"Failed to fetch GuC: %d disabling GSC\n", err);
+			intel_uc_fw_change_status(&uc->gsc.fw,
+						  INTEL_UC_FIRMWARE_ERROR);
+		}
+
 		return;
 	}
 
 	if (intel_uc_wants_huc(uc))
 		intel_uc_fw_fetch(&uc->huc.fw);
+
+	if (intel_uc_wants_gsc_uc(uc))
+		intel_uc_fw_fetch(&uc->gsc.fw);
 }
 
 static void __uc_cleanup_firmwares(struct intel_uc *uc)
 {
+	intel_uc_fw_cleanup_fetch(&uc->gsc.fw);
 	intel_uc_fw_cleanup_fetch(&uc->huc.fw);
 	intel_uc_fw_cleanup_fetch(&uc->guc.fw);
 }
@@ -330,11 +343,15 @@ static int __uc_init(struct intel_uc *uc)
 	if (intel_uc_uses_huc(uc))
 		intel_huc_init(huc);
 
+	if (intel_uc_uses_gsc_uc(uc))
+		intel_gsc_uc_init(&uc->gsc);
+
 	return 0;
 }
 
 static void __uc_fini(struct intel_uc *uc)
 {
+	intel_gsc_uc_fini(&uc->gsc);
 	intel_huc_fini(&uc->huc);
 	intel_guc_fini(&uc->guc);
 }
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index a8f38c2c60e2..5d0f1bcc381e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -6,6 +6,7 @@
 #ifndef _INTEL_UC_H_
 #define _INTEL_UC_H_
 
+#include "intel_gsc_uc.h"
 #include "intel_guc.h"
 #include "intel_guc_rc.h"
 #include "intel_guc_submission.h"
@@ -27,6 +28,7 @@ struct intel_uc_ops {
 
 struct intel_uc {
 	struct intel_uc_ops const *ops;
+	struct intel_gsc_uc gsc;
 	struct intel_guc guc;
 	struct intel_huc huc;
 
@@ -87,6 +89,7 @@ uc_state_checkers(huc, huc);
 uc_state_checkers(guc, guc_submission);
 uc_state_checkers(guc, guc_slpc);
 uc_state_checkers(guc, guc_rc);
+uc_state_checkers(gsc, gsc_uc);
 
 #undef uc_state_checkers
 #undef __uc_state_checker
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index 6c83a8b66c9e..1d6249d4b8ef 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -19,11 +19,18 @@
 static inline struct intel_gt *
 ____uc_fw_to_gt(struct intel_uc_fw *uc_fw, enum intel_uc_fw_type type)
 {
-	if (type == INTEL_UC_FW_TYPE_GUC)
+	GEM_BUG_ON(type >= INTEL_UC_FW_NUM_TYPES);
+
+	switch (type) {
+	case INTEL_UC_FW_TYPE_GUC:
 		return container_of(uc_fw, struct intel_gt, uc.guc.fw);
+	case INTEL_UC_FW_TYPE_HUC:
+		return container_of(uc_fw, struct intel_gt, uc.huc.fw);
+	case INTEL_UC_FW_TYPE_GSC:
+		return container_of(uc_fw, struct intel_gt, uc.gsc.fw);
+	}
 
-	GEM_BUG_ON(type != INTEL_UC_FW_TYPE_HUC);
-	return container_of(uc_fw, struct intel_gt, uc.huc.fw);
+	return NULL;
 }
 
 static inline struct intel_gt *__uc_fw_to_gt(struct intel_uc_fw *uc_fw)
@@ -246,6 +253,14 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
 	int i;
 	bool found;
 
+	/*
+	 * GSC FW support is still not fully in place, so we're not defining
+	 * the FW blob yet because we don't want the driver to attempt to load
+	 * it until we're ready for it.
+	 */
+	if (uc_fw->type == INTEL_UC_FW_TYPE_GSC)
+		return;
+
 	/*
 	 * The only difference between the ADL GuC FWs is the HWConfig support.
 	 * ADL-N does not support HWConfig, so we should use the same binary as
@@ -375,6 +390,11 @@ static const char *__override_huc_firmware_path(struct drm_i915_private *i915)
 	return "";
 }
 
+static const char *__override_gsc_firmware_path(struct drm_i915_private *i915)
+{
+	return i915->params.gsc_firmware_path;
+}
+
 static void __uc_fw_user_override(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
 {
 	const char *path = NULL;
@@ -386,6 +406,9 @@ static void __uc_fw_user_override(struct drm_i915_private *i915, struct intel_uc
 	case INTEL_UC_FW_TYPE_HUC:
 		path = __override_huc_firmware_path(i915);
 		break;
+	case INTEL_UC_FW_TYPE_GSC:
+		path = __override_gsc_firmware_path(i915);
+		break;
 	}
 
 	if (unlikely(path)) {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
index 3ab87c54a398..f4310516b3e6 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -61,9 +61,10 @@ enum intel_uc_fw_status {
 
 enum intel_uc_fw_type {
 	INTEL_UC_FW_TYPE_GUC = 0,
-	INTEL_UC_FW_TYPE_HUC
+	INTEL_UC_FW_TYPE_HUC,
+	INTEL_UC_FW_TYPE_GSC,
 };
-#define INTEL_UC_FW_NUM_TYPES 2
+#define INTEL_UC_FW_NUM_TYPES 3
 
 struct intel_uc_fw_ver {
 	u32 major;
@@ -204,6 +205,8 @@ static inline const char *intel_uc_fw_type_repr(enum intel_uc_fw_type type)
 		return "GuC";
 	case INTEL_UC_FW_TYPE_HUC:
 		return "HuC";
+	case INTEL_UC_FW_TYPE_GSC:
+		return "GSC";
 	}
 	return "uC";
 }
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index d1e4d528cb17..61578f2860cd 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -192,6 +192,9 @@ i915_param_named_unsafe(huc_firmware_path, charp, 0400,
 i915_param_named_unsafe(dmc_firmware_path, charp, 0400,
 	"DMC firmware path to use instead of the default one");
 
+i915_param_named_unsafe(gsc_firmware_path, charp, 0400,
+	"GSC firmware path to use instead of the default one");
+
 i915_param_named_unsafe(enable_dp_mst, bool, 0400,
 	"Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)");
 
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 2733cb6cfe09..3f51f90145b6 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -64,6 +64,7 @@ struct drm_printer;
 	param(char *, guc_firmware_path, NULL, 0400) \
 	param(char *, huc_firmware_path, NULL, 0400) \
 	param(char *, dmc_firmware_path, NULL, 0400) \
+	param(char *, gsc_firmware_path, NULL, 0400) \
 	param(bool, memtest, false, 0400) \
 	param(int, mmio_debug, -IS_ENABLED(CONFIG_DRM_I915_DEBUG_MMIO), 0600) \
 	param(int, edp_vswing, 0, 0400) \
-- 
2.37.3

[PATCH v2 2/6] drm/i915/gsc: Skip the version check when fetching the GSC FW

[Daniele Ceraolo Spurio]

The current exectation from the FW side is that the driver will query
the GSC FW version after the FW is loaded, similarly to what the mei
driver does on DG2. However, we're discussing with the FW team if there
is a way to extract the version from the bin file before loading, so we
can keep the code the same as for older FWs.

Since the GSC FW version is not currently required for functionality and
is only needed for debug purposes, we can skip the FW version for now at
fetch time and add it later on when we've agreed on the approach.

v2: rebased on uc_fw version struct changes.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> #v1
---
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 29 +++++++++++++++++++-----
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index 1d6249d4b8ef..78ab60c07a2b 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -655,6 +655,26 @@ static bool guc_check_version_range(struct intel_uc_fw *uc_fw)
 	return true;
 }
 
+static int check_fw_header(struct intel_gt *gt,
+			   const struct firmware *fw,
+			   struct intel_uc_fw *uc_fw)
+{
+	int err = 0;
+
+	/* GSC FW version is queried after the FW is loaded */
+	if (uc_fw->type == INTEL_UC_FW_TYPE_GSC)
+		return 0;
+
+	if (uc_fw->loaded_via_gsc)
+		err = check_gsc_manifest(fw, uc_fw);
+	else
+		err = check_ccs_header(gt, fw, uc_fw);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 /**
  * intel_uc_fw_fetch - fetch uC firmware
  * @uc_fw: uC firmware
@@ -724,17 +744,14 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 	if (err)
 		goto fail;
 
-	if (uc_fw->loaded_via_gsc)
-		err = check_gsc_manifest(fw, uc_fw);
-	else
-		err = check_ccs_header(gt, fw, uc_fw);
+	err = check_fw_header(gt, fw, uc_fw);
 	if (err)
 		goto fail;
 
 	if (uc_fw->type == INTEL_UC_FW_TYPE_GUC && !guc_check_version_range(uc_fw))
 		goto fail;
 
-	if (uc_fw->file_wanted.ver.major) {
+	if (uc_fw->file_wanted.ver.major && uc_fw->file_selected.ver.major) {
 		/* Check the file's major version was as it claimed */
 		if (uc_fw->file_selected.ver.major != uc_fw->file_wanted.ver.major) {
 			drm_notice(&i915->drm, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
@@ -751,7 +768,7 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 		}
 	}
 
-	if (old_ver) {
+	if (old_ver && uc_fw->file_selected.ver.major) {
 		/* Preserve the version that was really wanted */
 		memcpy(&uc_fw->file_wanted, &file_ideal, sizeof(uc_fw->file_wanted));
 
-- 
2.37.3

[PATCH v2 3/6] drm/i915/gsc: GSC firmware loading

[Daniele Ceraolo Spurio]

GSC FW is loaded by submitting a dedicated command via the GSC engine.
The memory area used for loading the FW is then re-purposed as local
memory for the GSC itself, so we use a separate allocation instead of
using the one where we keep the firmware stored for reload.

The GSC is not reset as part of GT reset, so we only need to load it on
first boot and S3/S4 exit.

v2: use REG_* for register fields definitions (Rodrigo), move to WQ
    immediately

Bspec: 63347, 65346
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/Makefile                |   1 +
 drivers/gpu/drm/i915/gt/intel_engine.h       |   2 +
 drivers/gpu/drm/i915/gt/intel_gpu_commands.h |   7 +
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c    | 187 +++++++++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h    |  15 ++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c    |  69 ++++++-
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h    |  11 ++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c        |   6 +
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c     |  20 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h     |   1 +
 10 files changed, 313 insertions(+), 6 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 69f6e9af1b56..dfa211451a1d 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -189,6 +189,7 @@ i915-y += \
 
 # general-purpose microcontroller (GuC) support
 i915-y += \
+	  gt/uc/intel_gsc_fw.o \
 	  gt/uc/intel_gsc_uc.o \
 	  gt/uc/intel_guc.o \
 	  gt/uc/intel_guc_ads.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index cbc8b857d5f7..0e24af5efee9 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -172,6 +172,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
 #define I915_GEM_HWS_MIGRATE		(0x42 * sizeof(u32))
 #define I915_GEM_HWS_PXP		0x60
 #define I915_GEM_HWS_PXP_ADDR		(I915_GEM_HWS_PXP * sizeof(u32))
+#define I915_GEM_HWS_GSC		0x62
+#define I915_GEM_HWS_GSC_ADDR		(I915_GEM_HWS_GSC * sizeof(u32))
 #define I915_GEM_HWS_SCRATCH		0x80
 
 #define I915_HWS_CSB_BUF0_INDEX		0x10
diff --git a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
index f50ea92910d9..49ebda141266 100644
--- a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
+++ b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
@@ -21,6 +21,7 @@
 #define INSTR_CLIENT_SHIFT      29
 #define   INSTR_MI_CLIENT       0x0
 #define   INSTR_BC_CLIENT       0x2
+#define   INSTR_GSC_CLIENT      0x2 /* MTL+ */
 #define   INSTR_RC_CLIENT       0x3
 #define INSTR_SUBCLIENT_SHIFT   27
 #define INSTR_SUBCLIENT_MASK    0x18000000
@@ -432,6 +433,12 @@
 #define COLOR_BLT     ((0x2<<29)|(0x40<<22))
 #define SRC_COPY_BLT  ((0x2<<29)|(0x43<<22))
 
+#define GSC_INSTR(opcode, data, flags) \
+	(__INSTR(INSTR_GSC_CLIENT) | (opcode) << 22 | (data) << 9 | (flags))
+
+#define GSC_FW_LOAD GSC_INSTR(1, 0, 2)
+#define   HECI1_FW_LIMIT_VALID (1<<31)
+
 /*
  * Used to convert any address to canonical form.
  * Starting from gen8, some commands (e.g. STATE_BASE_ADDRESS,
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
new file mode 100644
index 000000000000..f88069ab71ab
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include "gt/intel_engine_pm.h"
+#include "gt/intel_gpu_commands.h"
+#include "gt/intel_gt.h"
+#include "gt/intel_ring.h"
+#include "intel_gsc_fw.h"
+
+#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_INIT_COMPLETE_BIT		REG_BIT(9)
+
+static bool gsc_is_in_reset(struct intel_uncore *uncore)
+{
+	u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
+
+	return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) ==
+	       GSC_FW_CURRENT_STATE_RESET;
+}
+
+bool intel_gsc_uc_fw_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 fw_status & GSC_FW_INIT_COMPLETE_BIT;
+}
+
+static int emit_gsc_fw_load(struct i915_request *rq, struct intel_gsc_uc *gsc)
+{
+	u32 offset = i915_ggtt_offset(gsc->local);
+	u32 *cs;
+
+	cs = intel_ring_begin(rq, 4);
+	if (IS_ERR(cs))
+		return PTR_ERR(cs);
+
+	*cs++ = GSC_FW_LOAD;
+	*cs++ = lower_32_bits(offset);
+	*cs++ = upper_32_bits(offset);
+	*cs++ = (gsc->local->size / SZ_4K) | HECI1_FW_LIMIT_VALID;
+
+	intel_ring_advance(rq, cs);
+
+	return 0;
+}
+
+static int gsc_fw_load(struct intel_gsc_uc *gsc)
+{
+	struct intel_context *ce = gsc->ce;
+	struct i915_request *rq;
+	int err;
+
+	if (!ce)
+		return -ENODEV;
+
+	rq = i915_request_create(ce);
+	if (IS_ERR(rq))
+		return PTR_ERR(rq);
+
+	if (ce->engine->emit_init_breadcrumb) {
+		err = ce->engine->emit_init_breadcrumb(rq);
+		if (err)
+			goto out_rq;
+	}
+
+	err = emit_gsc_fw_load(rq, gsc);
+	if (err)
+		goto out_rq;
+
+	err = ce->engine->emit_flush(rq, 0);
+
+out_rq:
+	i915_request_get(rq);
+
+	if (unlikely(err))
+		i915_request_set_error_once(rq, err);
+
+	i915_request_add(rq);
+
+	if (!err && i915_request_wait(rq, 0, msecs_to_jiffies(500)) < 0)
+		err = -ETIME;
+
+	i915_request_put(rq);
+
+	if (err)
+		drm_err(&gsc_uc_to_gt(gsc)->i915->drm,
+			"Request submission for GSC load failed (%d)\n",
+			err);
+
+	return err;
+}
+
+static int gsc_fw_load_prepare(struct intel_gsc_uc *gsc)
+{
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	struct drm_i915_private *i915 = gt->i915;
+	struct drm_i915_gem_object *obj;
+	void *src, *dst;
+
+	if (!gsc->local)
+		return -ENODEV;
+
+	obj = gsc->local->obj;
+
+	if (obj->base.size < gsc->fw.size)
+		return -ENOSPC;
+
+	dst = i915_gem_object_pin_map_unlocked(obj,
+					       i915_coherent_map_type(i915, obj, true));
+	if (IS_ERR(dst))
+		return PTR_ERR(dst);
+
+	src = i915_gem_object_pin_map_unlocked(gsc->fw.obj,
+					       i915_coherent_map_type(i915, gsc->fw.obj, true));
+	if (IS_ERR(src)) {
+		i915_gem_object_unpin_map(obj);
+		return PTR_ERR(src);
+	}
+
+	memset(dst, 0, obj->base.size);
+	memcpy(dst, src, gsc->fw.size);
+
+	i915_gem_object_unpin_map(gsc->fw.obj);
+	i915_gem_object_unpin_map(obj);
+
+	return 0;
+}
+
+static int gsc_fw_wait(struct intel_gt *gt)
+{
+	return intel_wait_for_register(gt->uncore,
+				       GSC_FW_STATUS_REG,
+				       GSC_FW_INIT_COMPLETE_BIT,
+				       GSC_FW_INIT_COMPLETE_BIT,
+				       500);
+}
+
+int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc)
+{
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	struct intel_uc_fw *gsc_fw = &gsc->fw;
+	int err;
+
+	/* check current fw status */
+	if (intel_gsc_uc_fw_init_done(gsc)) {
+		if (GEM_WARN_ON(!intel_uc_fw_is_loaded(gsc_fw)))
+			intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
+		return -EEXIST;
+	}
+
+	if (!intel_uc_fw_is_loadable(gsc_fw))
+		return -ENOEXEC;
+
+	/* FW blob is ok, so clean the status */
+	intel_uc_fw_sanitize(&gsc->fw);
+
+	if (!gsc_is_in_reset(gt->uncore))
+		return -EIO;
+
+	err = gsc_fw_load_prepare(gsc);
+	if (err)
+		goto fail;
+
+	err = gsc_fw_load(gsc);
+	if (err)
+		goto fail;
+
+	err = gsc_fw_wait(gt);
+	if (err)
+		goto fail;
+
+	/* FW is not fully operational until we enable SW proxy */
+	intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
+
+	drm_info(&gt->i915->drm, "Loaded GSC firmware %s\n",
+		 gsc_fw->file_selected.path);
+
+	return 0;
+
+fail:
+	return intel_uc_fw_mark_load_failed(gsc_fw, err);
+}
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
new file mode 100644
index 000000000000..4b5dbb44afb4
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef _INTEL_GSC_FW_H_
+#define _INTEL_GSC_FW_H_
+
+#include <linux/types.h>
+
+struct intel_gsc_uc;
+
+int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc);
+bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc);
+#endif
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
index 65cbf1ce9fa1..b1a2f15137b3 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -7,8 +7,19 @@
 
 #include "gt/intel_gt.h"
 #include "intel_gsc_uc.h"
+#include "intel_gsc_fw.h"
 #include "i915_drv.h"
 
+void gsc_work(struct work_struct *work)
+{
+	struct intel_gsc_uc *gsc = container_of(work, typeof(*gsc), work);
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	intel_wakeref_t wakeref;
+
+	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
+		intel_gsc_uc_fw_upload(gsc);
+}
+
 static bool gsc_engine_supported(struct intel_gt *gt)
 {
 	intel_engine_mask_t mask;
@@ -32,6 +43,7 @@ static bool gsc_engine_supported(struct intel_gt *gt)
 void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc)
 {
 	intel_uc_fw_init_early(&gsc->fw, INTEL_UC_FW_TYPE_GSC);
+	INIT_WORK(&gsc->work, gsc_work);
 
 	/* we can arrive here from i915_driver_early_probe for primary
 	 * GT with it being not fully setup hence check device info's
@@ -45,17 +57,46 @@ void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc)
 
 int intel_gsc_uc_init(struct intel_gsc_uc *gsc)
 {
-	struct drm_i915_private *i915 = gsc_uc_to_gt(gsc)->i915;
+	static struct lock_class_key gsc_lock;
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	struct drm_i915_private *i915 = gt->i915;
+	struct intel_engine_cs *engine = gt->engine[GSC0];
+	struct intel_context *ce;
+	struct i915_vma *vma;
 	int err;
 
 	err = intel_uc_fw_init(&gsc->fw);
 	if (err)
 		goto out;
 
+	vma = intel_guc_allocate_vma(&gt->uc.guc, SZ_8M);
+	if (IS_ERR(vma)) {
+		err = PTR_ERR(vma);
+		goto out_fw;
+	}
+
+	gsc->local = vma;
+
+	ce = intel_engine_create_pinned_context(engine, engine->gt->vm, SZ_4K,
+						I915_GEM_HWS_GSC_ADDR,
+						&gsc_lock, "gsc_context");
+	if (IS_ERR(ce)) {
+		drm_err(&gt->i915->drm,
+			"failed to create GSC CS ctx for FW communication\n");
+		err =  PTR_ERR(ce);
+		goto out_vma;
+	}
+
+	gsc->ce = ce;
+
 	intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_LOADABLE);
 
 	return 0;
 
+out_vma:
+	i915_vma_unpin_and_release(&gsc->local, 0);
+out_fw:
+	intel_uc_fw_fini(&gsc->fw);
 out:
 	i915_probe_error(i915, "failed with %d\n", err);
 	return err;
@@ -66,5 +107,31 @@ void intel_gsc_uc_fini(struct intel_gsc_uc *gsc)
 	if (!intel_uc_fw_is_loadable(&gsc->fw))
 		return;
 
+	flush_work(&gsc->work);
+
+	if (gsc->ce)
+		intel_engine_destroy_pinned_context(fetch_and_zero(&gsc->ce));
+
+	i915_vma_unpin_and_release(&gsc->local, 0);
+
 	intel_uc_fw_fini(&gsc->fw);
 }
+
+void intel_gsc_uc_suspend(struct intel_gsc_uc *gsc)
+{
+	if (!intel_uc_fw_is_loadable(&gsc->fw))
+		return;
+
+	flush_work(&gsc->work);
+}
+
+void intel_gsc_uc_load_start(struct intel_gsc_uc *gsc)
+{
+	if (!intel_uc_fw_is_loadable(&gsc->fw))
+		return;
+
+	if (intel_gsc_uc_fw_init_done(gsc))
+		return;
+
+	queue_work(system_unbound_wq, &gsc->work);
+}
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
index ea2b1c0713b8..03fd0a8e8db1 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
@@ -8,14 +8,25 @@
 
 #include "intel_uc_fw.h"
 
+struct i915_vma;
+struct intel_context;
+
 struct intel_gsc_uc {
 	/* Generic uC firmware management */
 	struct intel_uc_fw fw;
+
+	/* GSC-specific additions */
+	struct i915_vma *local; /* private memory for GSC usage */
+	struct intel_context *ce; /* for submission to GSC FW via GSC engine */
+
+	struct work_struct work; /* for delayed load */
 };
 
 void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc);
 int intel_gsc_uc_init(struct intel_gsc_uc *gsc);
 void intel_gsc_uc_fini(struct intel_gsc_uc *gsc);
+void intel_gsc_uc_suspend(struct intel_gsc_uc *gsc);
+void intel_gsc_uc_load_start(struct intel_gsc_uc *gsc);
 
 static inline bool intel_gsc_uc_is_supported(struct intel_gsc_uc *gsc)
 {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 6f74586f87d8..9a8a1abf71d7 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -7,6 +7,7 @@
 
 #include "gt/intel_gt.h"
 #include "gt/intel_reset.h"
+#include "intel_gsc_fw.h"
 #include "intel_gsc_uc.h"
 #include "intel_guc.h"
 #include "intel_guc_ads.h"
@@ -548,6 +549,8 @@ static int __uc_init_hw(struct intel_uc *uc)
 		intel_rps_lower_unslice(&uc_to_gt(uc)->rps);
 	}
 
+	intel_gsc_uc_load_start(&uc->gsc);
+
 	drm_info(&i915->drm, "GuC submission %s\n",
 		 str_enabled_disabled(intel_uc_uses_guc_submission(uc)));
 	drm_info(&i915->drm, "GuC SLPC %s\n",
@@ -676,6 +679,9 @@ void intel_uc_suspend(struct intel_uc *uc)
 	intel_wakeref_t wakeref;
 	int err;
 
+	/* flush the GSC worker */
+	intel_gsc_uc_suspend(&uc->gsc);
+
 	if (!intel_guc_is_ready(guc)) {
 		guc->interrupts.enabled = false;
 		return;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index 78ab60c07a2b..d6ff6c584c1e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -930,6 +930,20 @@ static int uc_fw_xfer(struct intel_uc_fw *uc_fw, u32 dst_offset, u32 dma_flags)
 	return ret;
 }
 
+int intel_uc_fw_mark_load_failed(struct intel_uc_fw *uc_fw, int err)
+{
+	struct intel_gt *gt = __uc_fw_to_gt(uc_fw);
+
+	GEM_BUG_ON(!intel_uc_fw_is_loadable(uc_fw));
+
+	i915_probe_error(gt->i915, "Failed to load %s firmware %s (%d)\n",
+			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
+			 err);
+	intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_LOAD_FAIL);
+
+	return err;
+}
+
 /**
  * intel_uc_fw_upload - load uC firmware using custom loader
  * @uc_fw: uC firmware
@@ -966,11 +980,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, u32 dst_offset, u32 dma_flags)
 	return 0;
 
 fail:
-	i915_probe_error(gt->i915, "Failed to load %s firmware %s (%d)\n",
-			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
-			 err);
-	intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_LOAD_FAIL);
-	return err;
+	return intel_uc_fw_mark_load_failed(uc_fw, err);
 }
 
 static inline bool uc_fw_need_rsa_in_memory(struct intel_uc_fw *uc_fw)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
index f4310516b3e6..6ba00e6b3975 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -289,6 +289,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, u32 offset, u32 dma_flags);
 int intel_uc_fw_init(struct intel_uc_fw *uc_fw);
 void intel_uc_fw_fini(struct intel_uc_fw *uc_fw);
 size_t intel_uc_fw_copy_rsa(struct intel_uc_fw *uc_fw, void *dst, u32 max_len);
+int intel_uc_fw_mark_load_failed(struct intel_uc_fw *uc_fw, int err);
 void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p);
 
 #endif
-- 
2.37.3

[PATCH v2 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded

[Daniele Ceraolo Spurio]

If the GSC was loaded, the only way to stop it during the driver unload
flow is to do a driver-FLR.
The driver-initiated FLR is not the same as PCI config space FLR in
that it doesn't reset the SGUnit and doesn't modify the PCI config
space. Thus, it doesn't require a re-enumeration of the PCI BARs.
However, the driver-FLR does cause a memory wipe of graphics memory
on all discrete GPU platforms or a wipe limited to stolen memory
on the integrated GPU platforms.

We perform the FLR as the last action before releasing the MMIO bar, so
that we don't have to care about the consequences of the reset on the
unload flow.

v2: rename FLR function, add comment to explain FLR impact (Rodrigo),
    better explain why GSC needs FLR (Alan)

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 23 +++++++++
 drivers/gpu/drm/i915/i915_reg.h           |  3 ++
 drivers/gpu/drm/i915/intel_uncore.c       | 58 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uncore.h       | 13 +++++
 4 files changed, 97 insertions(+)

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 f88069ab71ab..e73d4440c5e8 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
@@ -166,6 +166,29 @@ int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc)
 	if (err)
 		goto fail;
 
+	/*
+	 * GSC is only killed by an FLR, so we need to trigger one on unload to
+	 * make sure we stop it. This is because we assign a chunk of memory to
+	 * the GSC as part of the FW load , so we need to make sure it stops
+	 * using it when we release it to the system on driver unload. Note that
+	 * this is not a problem of the unload per-se, because the GSC will not
+	 * touch that memory unless there are requests for it coming from the
+	 * driver; therefore, no accesses will happen while i915 is not loaded,
+	 * but if we re-load the driver then the GSC might wake up and try to
+	 * access that old memory location again.
+	 * Given that an FLR is a very disruptive action (see the FLR function
+	 * for details), we want to do it as the last action before releasing
+	 * the access to the MMIO bar, which means we need to do it as part of
+	 * the primary uncore cleanup.
+	 * An alternative approach to the FLR would be to use a memory location
+	 * that survives driver unload, like e.g. stolen memory, and keep the
+	 * GSC loaded across reloads. However, this requires us to make sure we
+	 * preserve that memory location on unload and then determine and
+	 * reserve its offset on each subsequent load, which is not trivial, so
+	 * it is easier to just kill everything and start fresh.
+	 */
+	intel_uncore_set_flr_on_fini(&gt->i915->uncore);
+
 	err = gsc_fw_load(gsc);
 	if (err)
 		goto fail;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0b90fe6a28f7..b95d533652a4 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -118,6 +118,9 @@
 
 #define GU_CNTL				_MMIO(0x101010)
 #define   LMEM_INIT			REG_BIT(7)
+#define   DRIVERFLR			REG_BIT(31)
+#define GU_DEBUG			_MMIO(0x101018)
+#define   DRIVERFLR_STATUS		REG_BIT(31)
 
 #define GEN6_STOLEN_RESERVED		_MMIO(0x1082C0)
 #define GEN6_STOLEN_RESERVED_ADDR_MASK	(0xFFF << 20)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 8006a6c61466..3bfb4af0df78 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -2703,6 +2703,61 @@ void intel_uncore_prune_engine_fw_domains(struct intel_uncore *uncore,
 	}
 }
 
+/*
+ * The driver-initiated FLR is the highest level of reset that we can trigger
+ * from within the driver. It is different from the PCI FLR in that it doesn't
+ * fully reset the SGUnit and doesn't modify the PCI config space and therefore
+ * it doesn't require a re-enumeration of the PCI BARs. However, the
+ * driver-initiated FLR does still cause a reset of both GT and display and a
+ * memory wipe of local and stolen memory, so recovery would require a full HW
+ * re-init and saving/restoring (or re-populating) the wiped memory. Since we
+ * perform the FLR as the very last action before releasing access to the HW
+ * during the driver release flow, we don't attempt recovery at all, because
+ * if/when a new instance of i915 is bound to the device it will do a full
+ * re-init anyway.
+ */
+static void driver_initiated_flr(struct intel_uncore *uncore)
+{
+	struct drm_i915_private *i915 = uncore->i915;
+	const unsigned int flr_timeout_ms = 3000; /* specs recommend a 3s wait */
+	int ret;
+
+	drm_dbg(&i915->drm, "Triggering Driver-FLR\n");
+
+	/*
+	 * Make sure any pending FLR requests have cleared by waiting for the
+	 * FLR trigger bit to go to zero. Also clear GU_DEBUG's DRIVERFLR_STATUS
+	 * to make sure it's not still set from a prior attempt (it's a write to
+	 * clear bit).
+	 * Note that we should never be in a situation where a previous attempt
+	 * is still pending (unless the HW is totally dead), but better to be
+	 * safe in case something unexpected happens
+	 */
+	ret = intel_wait_for_register_fw(uncore, GU_CNTL, DRIVERFLR, 0, flr_timeout_ms);
+	if (ret) {
+		drm_err(&i915->drm,
+			"Failed to wait for Driver-FLR bit to clear! %d\n",
+			ret);
+		return;
+	}
+	intel_uncore_write_fw(uncore, GU_DEBUG, DRIVERFLR_STATUS);
+
+	/* Trigger the actual Driver-FLR */
+	intel_uncore_rmw_fw(uncore, GU_CNTL, 0, DRIVERFLR);
+
+	ret = intel_wait_for_register_fw(uncore, GU_DEBUG,
+					 DRIVERFLR_STATUS, DRIVERFLR_STATUS,
+					 flr_timeout_ms);
+	if (ret) {
+		drm_err(&i915->drm, "wait for Driver-FLR completion failed! %d\n", ret);
+		return;
+	}
+
+	intel_uncore_write_fw(uncore, GU_DEBUG, DRIVERFLR_STATUS);
+
+	return;
+}
+
 /* Called via drm-managed action */
 void intel_uncore_fini_mmio(struct drm_device *dev, void *data)
 {
@@ -2716,6 +2771,9 @@ void intel_uncore_fini_mmio(struct drm_device *dev, void *data)
 		intel_uncore_fw_domains_fini(uncore);
 		iosf_mbi_punit_release();
 	}
+
+	if (intel_uncore_needs_flr_on_fini(uncore))
+		driver_initiated_flr(uncore);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index e9e38490815d..9ea1f4864a3a 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -153,6 +153,7 @@ struct intel_uncore {
 #define UNCORE_HAS_FPGA_DBG_UNCLAIMED	BIT(1)
 #define UNCORE_HAS_DBG_UNCLAIMED	BIT(2)
 #define UNCORE_HAS_FIFO			BIT(3)
+#define UNCORE_NEEDS_FLR_ON_FINI	BIT(4)
 
 	const struct intel_forcewake_range *fw_domains_table;
 	unsigned int fw_domains_table_entries;
@@ -223,6 +224,18 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
 	return uncore->flags & UNCORE_HAS_FIFO;
 }
 
+static inline bool
+intel_uncore_needs_flr_on_fini(const struct intel_uncore *uncore)
+{
+	return uncore->flags & UNCORE_NEEDS_FLR_ON_FINI;
+}
+
+static inline bool
+intel_uncore_set_flr_on_fini(struct intel_uncore *uncore)
+{
+	return uncore->flags |= UNCORE_NEEDS_FLR_ON_FINI;
+}
+
 void intel_uncore_mmio_debug_init_early(struct drm_i915_private *i915);
 void intel_uncore_init_early(struct intel_uncore *uncore,
 			     struct intel_gt *gt);
-- 
2.37.3

[PATCH v2 5/6] drm/i915/gsc: Disable GSC engine and power well if FW is not selected

[Daniele Ceraolo Spurio]

From: Jonathan Cavitt <jonathan.cavitt@intel.com>

The GSC CS is only used for communicating with the GSC FW, so no need to
initialize it if we're not going to use the FW. If we're not using
neither the engine nor the microcontoller, then we can also disable the
power well.

IMPORTANT: lack of GSC FW breaks media C6 due to opposing requirements
between CS setup and forcewake idleness. See in-code comment for detail.

Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: John C Harrison <John.C.Harrison@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_uncore.c       |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index c33e0d72d670..99c4b866addd 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -894,6 +894,24 @@ static intel_engine_mask_t init_engine_mask(struct intel_gt *gt)
 	engine_mask_apply_compute_fuses(gt);
 	engine_mask_apply_copy_fuses(gt);
 
+	/*
+	 * The only use of the GSC CS is to load and communicate with the GSC
+	 * FW, so we have no use for it if we don't have the FW.
+	 *
+	 * IMPORTANT: in cases where we don't have the GSC FW, we have a
+	 * catch-22 situation that breaks media C6 due to 2 requirements:
+	 * 1) once turned on, the GSC power well will not go to sleep unless the
+	 *    GSC FW is loaded.
+	 * 2) to enable idling (which is required for media C6) we need to
+	 *    initialize the IDLE_MSG register for the GSC CS and do at least 1
+	 *    submission, which will wake up the GSC power well.
+	 */
+	if (__HAS_ENGINE(info->engine_mask, GSC0) && !intel_uc_wants_gsc_uc(&gt->uc)) {
+		drm_notice(&gt->i915->drm,
+			   "No GSC FW selected, disabling GSC CS and media C6\n");
+		info->engine_mask &= ~BIT(GSC0);
+	}
+
 	return info->engine_mask;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 3bfb4af0df78..cb45e4a4ace4 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -2701,6 +2701,9 @@ void intel_uncore_prune_engine_fw_domains(struct intel_uncore *uncore,
 		if (fw_domains & BIT(domain_id))
 			fw_domain_fini(uncore, domain_id);
 	}
+
+	if ((fw_domains & BIT(FW_DOMAIN_ID_GSC)) && !HAS_ENGINE(gt, GSC0))
+		fw_domain_fini(uncore, FW_DOMAIN_ID_GSC);
 }
 
 /*
-- 
2.37.3

[PATCH v2 6/6] drm/i915/mtl: MTL has one GSC CS on the media GT

[Daniele Ceraolo Spurio]

Now that we have the GSC FW support code as a user to the GSC CS, we
can add the relevant flag to the engine mask. Note that the engine will
still be disabled until we define the GSC FW binary file.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 414b4bfd514b..3f803c1280c0 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -1125,7 +1125,7 @@ static const struct intel_gt_definition xelpmp_extra_gt[] = {
 		.type = GT_MEDIA,
 		.name = "Standalone Media GT",
 		.gsi_offset = MTL_MEDIA_GSI_BASE,
-		.engine_mask = BIT(VECS0) | BIT(VCS0) | BIT(VCS2),
+		.engine_mask = BIT(VECS0) | BIT(VCS0) | BIT(VCS2) | BIT(GSC0),
 	},
 	{}
 };
-- 
2.37.3

[PATCH] drm/i915/gsc: GSC firmware loading

[Daniele Ceraolo Spurio]

GSC FW is loaded by submitting a dedicated command via the GSC engine.
The memory area used for loading the FW is then re-purposed as local
memory for the GSC itself, so we use a separate allocation instead of
using the one where we keep the firmware stored for reload.

The GSC is not reset as part of GT reset, so we only need to load it on
first boot and S3/S4 exit.

v2: use REG_* for register fields definitions (Rodrigo), move to WQ
    immediately

v3: mark worker function as static

Bspec: 63347, 65346
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/Makefile                |   1 +
 drivers/gpu/drm/i915/gt/intel_engine.h       |   2 +
 drivers/gpu/drm/i915/gt/intel_gpu_commands.h |   7 +
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c    | 187 +++++++++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h    |  15 ++
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c    |  69 ++++++-
 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h    |  11 ++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c        |   6 +
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c     |  20 +-
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h     |   1 +
 10 files changed, 313 insertions(+), 6 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
 create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 69f6e9af1b56..dfa211451a1d 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -189,6 +189,7 @@ i915-y += \
 
 # general-purpose microcontroller (GuC) support
 i915-y += \
+	  gt/uc/intel_gsc_fw.o \
 	  gt/uc/intel_gsc_uc.o \
 	  gt/uc/intel_guc.o \
 	  gt/uc/intel_guc_ads.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index cbc8b857d5f7..0e24af5efee9 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -172,6 +172,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
 #define I915_GEM_HWS_MIGRATE		(0x42 * sizeof(u32))
 #define I915_GEM_HWS_PXP		0x60
 #define I915_GEM_HWS_PXP_ADDR		(I915_GEM_HWS_PXP * sizeof(u32))
+#define I915_GEM_HWS_GSC		0x62
+#define I915_GEM_HWS_GSC_ADDR		(I915_GEM_HWS_GSC * sizeof(u32))
 #define I915_GEM_HWS_SCRATCH		0x80
 
 #define I915_HWS_CSB_BUF0_INDEX		0x10
diff --git a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
index f50ea92910d9..49ebda141266 100644
--- a/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
+++ b/drivers/gpu/drm/i915/gt/intel_gpu_commands.h
@@ -21,6 +21,7 @@
 #define INSTR_CLIENT_SHIFT      29
 #define   INSTR_MI_CLIENT       0x0
 #define   INSTR_BC_CLIENT       0x2
+#define   INSTR_GSC_CLIENT      0x2 /* MTL+ */
 #define   INSTR_RC_CLIENT       0x3
 #define INSTR_SUBCLIENT_SHIFT   27
 #define INSTR_SUBCLIENT_MASK    0x18000000
@@ -432,6 +433,12 @@
 #define COLOR_BLT     ((0x2<<29)|(0x40<<22))
 #define SRC_COPY_BLT  ((0x2<<29)|(0x43<<22))
 
+#define GSC_INSTR(opcode, data, flags) \
+	(__INSTR(INSTR_GSC_CLIENT) | (opcode) << 22 | (data) << 9 | (flags))
+
+#define GSC_FW_LOAD GSC_INSTR(1, 0, 2)
+#define   HECI1_FW_LIMIT_VALID (1<<31)
+
 /*
  * Used to convert any address to canonical form.
  * Starting from gen8, some commands (e.g. STATE_BASE_ADDRESS,
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
new file mode 100644
index 000000000000..f88069ab71ab
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include "gt/intel_engine_pm.h"
+#include "gt/intel_gpu_commands.h"
+#include "gt/intel_gt.h"
+#include "gt/intel_ring.h"
+#include "intel_gsc_fw.h"
+
+#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_INIT_COMPLETE_BIT		REG_BIT(9)
+
+static bool gsc_is_in_reset(struct intel_uncore *uncore)
+{
+	u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
+
+	return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) ==
+	       GSC_FW_CURRENT_STATE_RESET;
+}
+
+bool intel_gsc_uc_fw_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 fw_status & GSC_FW_INIT_COMPLETE_BIT;
+}
+
+static int emit_gsc_fw_load(struct i915_request *rq, struct intel_gsc_uc *gsc)
+{
+	u32 offset = i915_ggtt_offset(gsc->local);
+	u32 *cs;
+
+	cs = intel_ring_begin(rq, 4);
+	if (IS_ERR(cs))
+		return PTR_ERR(cs);
+
+	*cs++ = GSC_FW_LOAD;
+	*cs++ = lower_32_bits(offset);
+	*cs++ = upper_32_bits(offset);
+	*cs++ = (gsc->local->size / SZ_4K) | HECI1_FW_LIMIT_VALID;
+
+	intel_ring_advance(rq, cs);
+
+	return 0;
+}
+
+static int gsc_fw_load(struct intel_gsc_uc *gsc)
+{
+	struct intel_context *ce = gsc->ce;
+	struct i915_request *rq;
+	int err;
+
+	if (!ce)
+		return -ENODEV;
+
+	rq = i915_request_create(ce);
+	if (IS_ERR(rq))
+		return PTR_ERR(rq);
+
+	if (ce->engine->emit_init_breadcrumb) {
+		err = ce->engine->emit_init_breadcrumb(rq);
+		if (err)
+			goto out_rq;
+	}
+
+	err = emit_gsc_fw_load(rq, gsc);
+	if (err)
+		goto out_rq;
+
+	err = ce->engine->emit_flush(rq, 0);
+
+out_rq:
+	i915_request_get(rq);
+
+	if (unlikely(err))
+		i915_request_set_error_once(rq, err);
+
+	i915_request_add(rq);
+
+	if (!err && i915_request_wait(rq, 0, msecs_to_jiffies(500)) < 0)
+		err = -ETIME;
+
+	i915_request_put(rq);
+
+	if (err)
+		drm_err(&gsc_uc_to_gt(gsc)->i915->drm,
+			"Request submission for GSC load failed (%d)\n",
+			err);
+
+	return err;
+}
+
+static int gsc_fw_load_prepare(struct intel_gsc_uc *gsc)
+{
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	struct drm_i915_private *i915 = gt->i915;
+	struct drm_i915_gem_object *obj;
+	void *src, *dst;
+
+	if (!gsc->local)
+		return -ENODEV;
+
+	obj = gsc->local->obj;
+
+	if (obj->base.size < gsc->fw.size)
+		return -ENOSPC;
+
+	dst = i915_gem_object_pin_map_unlocked(obj,
+					       i915_coherent_map_type(i915, obj, true));
+	if (IS_ERR(dst))
+		return PTR_ERR(dst);
+
+	src = i915_gem_object_pin_map_unlocked(gsc->fw.obj,
+					       i915_coherent_map_type(i915, gsc->fw.obj, true));
+	if (IS_ERR(src)) {
+		i915_gem_object_unpin_map(obj);
+		return PTR_ERR(src);
+	}
+
+	memset(dst, 0, obj->base.size);
+	memcpy(dst, src, gsc->fw.size);
+
+	i915_gem_object_unpin_map(gsc->fw.obj);
+	i915_gem_object_unpin_map(obj);
+
+	return 0;
+}
+
+static int gsc_fw_wait(struct intel_gt *gt)
+{
+	return intel_wait_for_register(gt->uncore,
+				       GSC_FW_STATUS_REG,
+				       GSC_FW_INIT_COMPLETE_BIT,
+				       GSC_FW_INIT_COMPLETE_BIT,
+				       500);
+}
+
+int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc)
+{
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	struct intel_uc_fw *gsc_fw = &gsc->fw;
+	int err;
+
+	/* check current fw status */
+	if (intel_gsc_uc_fw_init_done(gsc)) {
+		if (GEM_WARN_ON(!intel_uc_fw_is_loaded(gsc_fw)))
+			intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
+		return -EEXIST;
+	}
+
+	if (!intel_uc_fw_is_loadable(gsc_fw))
+		return -ENOEXEC;
+
+	/* FW blob is ok, so clean the status */
+	intel_uc_fw_sanitize(&gsc->fw);
+
+	if (!gsc_is_in_reset(gt->uncore))
+		return -EIO;
+
+	err = gsc_fw_load_prepare(gsc);
+	if (err)
+		goto fail;
+
+	err = gsc_fw_load(gsc);
+	if (err)
+		goto fail;
+
+	err = gsc_fw_wait(gt);
+	if (err)
+		goto fail;
+
+	/* FW is not fully operational until we enable SW proxy */
+	intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
+
+	drm_info(&gt->i915->drm, "Loaded GSC firmware %s\n",
+		 gsc_fw->file_selected.path);
+
+	return 0;
+
+fail:
+	return intel_uc_fw_mark_load_failed(gsc_fw, err);
+}
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
new file mode 100644
index 000000000000..4b5dbb44afb4
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef _INTEL_GSC_FW_H_
+#define _INTEL_GSC_FW_H_
+
+#include <linux/types.h>
+
+struct intel_gsc_uc;
+
+int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc);
+bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc);
+#endif
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
index 65cbf1ce9fa1..3692ba387834 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -7,8 +7,19 @@
 
 #include "gt/intel_gt.h"
 #include "intel_gsc_uc.h"
+#include "intel_gsc_fw.h"
 #include "i915_drv.h"
 
+static void gsc_work(struct work_struct *work)
+{
+	struct intel_gsc_uc *gsc = container_of(work, typeof(*gsc), work);
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	intel_wakeref_t wakeref;
+
+	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
+		intel_gsc_uc_fw_upload(gsc);
+}
+
 static bool gsc_engine_supported(struct intel_gt *gt)
 {
 	intel_engine_mask_t mask;
@@ -32,6 +43,7 @@ static bool gsc_engine_supported(struct intel_gt *gt)
 void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc)
 {
 	intel_uc_fw_init_early(&gsc->fw, INTEL_UC_FW_TYPE_GSC);
+	INIT_WORK(&gsc->work, gsc_work);
 
 	/* we can arrive here from i915_driver_early_probe for primary
 	 * GT with it being not fully setup hence check device info's
@@ -45,17 +57,46 @@ void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc)
 
 int intel_gsc_uc_init(struct intel_gsc_uc *gsc)
 {
-	struct drm_i915_private *i915 = gsc_uc_to_gt(gsc)->i915;
+	static struct lock_class_key gsc_lock;
+	struct intel_gt *gt = gsc_uc_to_gt(gsc);
+	struct drm_i915_private *i915 = gt->i915;
+	struct intel_engine_cs *engine = gt->engine[GSC0];
+	struct intel_context *ce;
+	struct i915_vma *vma;
 	int err;
 
 	err = intel_uc_fw_init(&gsc->fw);
 	if (err)
 		goto out;
 
+	vma = intel_guc_allocate_vma(&gt->uc.guc, SZ_8M);
+	if (IS_ERR(vma)) {
+		err = PTR_ERR(vma);
+		goto out_fw;
+	}
+
+	gsc->local = vma;
+
+	ce = intel_engine_create_pinned_context(engine, engine->gt->vm, SZ_4K,
+						I915_GEM_HWS_GSC_ADDR,
+						&gsc_lock, "gsc_context");
+	if (IS_ERR(ce)) {
+		drm_err(&gt->i915->drm,
+			"failed to create GSC CS ctx for FW communication\n");
+		err =  PTR_ERR(ce);
+		goto out_vma;
+	}
+
+	gsc->ce = ce;
+
 	intel_uc_fw_change_status(&gsc->fw, INTEL_UC_FIRMWARE_LOADABLE);
 
 	return 0;
 
+out_vma:
+	i915_vma_unpin_and_release(&gsc->local, 0);
+out_fw:
+	intel_uc_fw_fini(&gsc->fw);
 out:
 	i915_probe_error(i915, "failed with %d\n", err);
 	return err;
@@ -66,5 +107,31 @@ void intel_gsc_uc_fini(struct intel_gsc_uc *gsc)
 	if (!intel_uc_fw_is_loadable(&gsc->fw))
 		return;
 
+	flush_work(&gsc->work);
+
+	if (gsc->ce)
+		intel_engine_destroy_pinned_context(fetch_and_zero(&gsc->ce));
+
+	i915_vma_unpin_and_release(&gsc->local, 0);
+
 	intel_uc_fw_fini(&gsc->fw);
 }
+
+void intel_gsc_uc_suspend(struct intel_gsc_uc *gsc)
+{
+	if (!intel_uc_fw_is_loadable(&gsc->fw))
+		return;
+
+	flush_work(&gsc->work);
+}
+
+void intel_gsc_uc_load_start(struct intel_gsc_uc *gsc)
+{
+	if (!intel_uc_fw_is_loadable(&gsc->fw))
+		return;
+
+	if (intel_gsc_uc_fw_init_done(gsc))
+		return;
+
+	queue_work(system_unbound_wq, &gsc->work);
+}
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
index ea2b1c0713b8..03fd0a8e8db1 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
@@ -8,14 +8,25 @@
 
 #include "intel_uc_fw.h"
 
+struct i915_vma;
+struct intel_context;
+
 struct intel_gsc_uc {
 	/* Generic uC firmware management */
 	struct intel_uc_fw fw;
+
+	/* GSC-specific additions */
+	struct i915_vma *local; /* private memory for GSC usage */
+	struct intel_context *ce; /* for submission to GSC FW via GSC engine */
+
+	struct work_struct work; /* for delayed load */
 };
 
 void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc);
 int intel_gsc_uc_init(struct intel_gsc_uc *gsc);
 void intel_gsc_uc_fini(struct intel_gsc_uc *gsc);
+void intel_gsc_uc_suspend(struct intel_gsc_uc *gsc);
+void intel_gsc_uc_load_start(struct intel_gsc_uc *gsc);
 
 static inline bool intel_gsc_uc_is_supported(struct intel_gsc_uc *gsc)
 {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 6f74586f87d8..9a8a1abf71d7 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -7,6 +7,7 @@
 
 #include "gt/intel_gt.h"
 #include "gt/intel_reset.h"
+#include "intel_gsc_fw.h"
 #include "intel_gsc_uc.h"
 #include "intel_guc.h"
 #include "intel_guc_ads.h"
@@ -548,6 +549,8 @@ static int __uc_init_hw(struct intel_uc *uc)
 		intel_rps_lower_unslice(&uc_to_gt(uc)->rps);
 	}
 
+	intel_gsc_uc_load_start(&uc->gsc);
+
 	drm_info(&i915->drm, "GuC submission %s\n",
 		 str_enabled_disabled(intel_uc_uses_guc_submission(uc)));
 	drm_info(&i915->drm, "GuC SLPC %s\n",
@@ -676,6 +679,9 @@ void intel_uc_suspend(struct intel_uc *uc)
 	intel_wakeref_t wakeref;
 	int err;
 
+	/* flush the GSC worker */
+	intel_gsc_uc_suspend(&uc->gsc);
+
 	if (!intel_guc_is_ready(guc)) {
 		guc->interrupts.enabled = false;
 		return;
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index 78ab60c07a2b..d6ff6c584c1e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -930,6 +930,20 @@ static int uc_fw_xfer(struct intel_uc_fw *uc_fw, u32 dst_offset, u32 dma_flags)
 	return ret;
 }
 
+int intel_uc_fw_mark_load_failed(struct intel_uc_fw *uc_fw, int err)
+{
+	struct intel_gt *gt = __uc_fw_to_gt(uc_fw);
+
+	GEM_BUG_ON(!intel_uc_fw_is_loadable(uc_fw));
+
+	i915_probe_error(gt->i915, "Failed to load %s firmware %s (%d)\n",
+			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
+			 err);
+	intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_LOAD_FAIL);
+
+	return err;
+}
+
 /**
  * intel_uc_fw_upload - load uC firmware using custom loader
  * @uc_fw: uC firmware
@@ -966,11 +980,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, u32 dst_offset, u32 dma_flags)
 	return 0;
 
 fail:
-	i915_probe_error(gt->i915, "Failed to load %s firmware %s (%d)\n",
-			 intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
-			 err);
-	intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_LOAD_FAIL);
-	return err;
+	return intel_uc_fw_mark_load_failed(uc_fw, err);
 }
 
 static inline bool uc_fw_need_rsa_in_memory(struct intel_uc_fw *uc_fw)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
index f4310516b3e6..6ba00e6b3975 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -289,6 +289,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, u32 offset, u32 dma_flags);
 int intel_uc_fw_init(struct intel_uc_fw *uc_fw);
 void intel_uc_fw_fini(struct intel_uc_fw *uc_fw);
 size_t intel_uc_fw_copy_rsa(struct intel_uc_fw *uc_fw, void *dst, u32 max_len);
+int intel_uc_fw_mark_load_failed(struct intel_uc_fw *uc_fw, int err);
 void intel_uc_fw_dump(const struct intel_uc_fw *uc_fw, struct drm_printer *p);
 
 #endif
-- 
2.37.3

Re: [Intel-gfx] [PATCH v2 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded

[Rodrigo Vivi]

On Mon, Dec 05, 2022 at 05:19:06PM -0800, Daniele Ceraolo Spurio wrote:
> If the GSC was loaded, the only way to stop it during the driver unload
> flow is to do a driver-FLR.
> The driver-initiated FLR is not the same as PCI config space FLR in
> that it doesn't reset the SGUnit and doesn't modify the PCI config
> space. Thus, it doesn't require a re-enumeration of the PCI BARs.
> However, the driver-FLR does cause a memory wipe of graphics memory
> on all discrete GPU platforms or a wipe limited to stolen memory
> on the integrated GPU platforms.
> 
> We perform the FLR as the last action before releasing the MMIO bar, so
> that we don't have to care about the consequences of the reset on the
> unload flow.
> 
> v2: rename FLR function, add comment to explain FLR impact (Rodrigo),
>     better explain why GSC needs FLR (Alan)
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 23 +++++++++
>  drivers/gpu/drm/i915/i915_reg.h           |  3 ++
>  drivers/gpu/drm/i915/intel_uncore.c       | 58 +++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_uncore.h       | 13 +++++
>  4 files changed, 97 insertions(+)
> 
> 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 f88069ab71ab..e73d4440c5e8 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> @@ -166,6 +166,29 @@ int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc)
>  	if (err)
>  		goto fail;
>  
> +	/*
> +	 * GSC is only killed by an FLR, so we need to trigger one on unload to
> +	 * make sure we stop it. This is because we assign a chunk of memory to
> +	 * the GSC as part of the FW load , so we need to make sure it stops
> +	 * using it when we release it to the system on driver unload. Note that
> +	 * this is not a problem of the unload per-se, because the GSC will not
> +	 * touch that memory unless there are requests for it coming from the
> +	 * driver; therefore, no accesses will happen while i915 is not loaded,
> +	 * but if we re-load the driver then the GSC might wake up and try to
> +	 * access that old memory location again.
> +	 * Given that an FLR is a very disruptive action (see the FLR function
> +	 * for details), we want to do it as the last action before releasing
> +	 * the access to the MMIO bar, which means we need to do it as part of
> +	 * the primary uncore cleanup.
> +	 * An alternative approach to the FLR would be to use a memory location
> +	 * that survives driver unload, like e.g. stolen memory, and keep the
> +	 * GSC loaded across reloads. However, this requires us to make sure we
> +	 * preserve that memory location on unload and then determine and
> +	 * reserve its offset on each subsequent load, which is not trivial, so
> +	 * it is easier to just kill everything and start fresh.
> +	 */
> +	intel_uncore_set_flr_on_fini(&gt->i915->uncore);
> +
>  	err = gsc_fw_load(gsc);
>  	if (err)
>  		goto fail;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0b90fe6a28f7..b95d533652a4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -118,6 +118,9 @@
>  
>  #define GU_CNTL				_MMIO(0x101010)
>  #define   LMEM_INIT			REG_BIT(7)
> +#define   DRIVERFLR			REG_BIT(31)
> +#define GU_DEBUG			_MMIO(0x101018)
> +#define   DRIVERFLR_STATUS		REG_BIT(31)
>  
>  #define GEN6_STOLEN_RESERVED		_MMIO(0x1082C0)
>  #define GEN6_STOLEN_RESERVED_ADDR_MASK	(0xFFF << 20)
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 8006a6c61466..3bfb4af0df78 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -2703,6 +2703,61 @@ void intel_uncore_prune_engine_fw_domains(struct intel_uncore *uncore,
>  	}
>  }
>  
> +/*
> + * The driver-initiated FLR is the highest level of reset that we can trigger
> + * from within the driver. It is different from the PCI FLR in that it doesn't
> + * fully reset the SGUnit and doesn't modify the PCI config space and therefore
> + * it doesn't require a re-enumeration of the PCI BARs. However, the
> + * driver-initiated FLR does still cause a reset of both GT and display and a
> + * memory wipe of local and stolen memory, so recovery would require a full HW
> + * re-init and saving/restoring (or re-populating) the wiped memory. Since we
> + * perform the FLR as the very last action before releasing access to the HW
> + * during the driver release flow, we don't attempt recovery at all, because
> + * if/when a new instance of i915 is bound to the device it will do a full
> + * re-init anyway.
> + */
> +static void driver_initiated_flr(struct intel_uncore *uncore)
> +{
> +	struct drm_i915_private *i915 = uncore->i915;
> +	const unsigned int flr_timeout_ms = 3000; /* specs recommend a 3s wait */
> +	int ret;
> +
> +	drm_dbg(&i915->drm, "Triggering Driver-FLR\n");
> +
> +	/*
> +	 * Make sure any pending FLR requests have cleared by waiting for the
> +	 * FLR trigger bit to go to zero. Also clear GU_DEBUG's DRIVERFLR_STATUS
> +	 * to make sure it's not still set from a prior attempt (it's a write to
> +	 * clear bit).
> +	 * Note that we should never be in a situation where a previous attempt
> +	 * is still pending (unless the HW is totally dead), but better to be
> +	 * safe in case something unexpected happens
> +	 */
> +	ret = intel_wait_for_register_fw(uncore, GU_CNTL, DRIVERFLR, 0, flr_timeout_ms);
> +	if (ret) {
> +		drm_err(&i915->drm,
> +			"Failed to wait for Driver-FLR bit to clear! %d\n",
> +			ret);
> +		return;
> +	}
> +	intel_uncore_write_fw(uncore, GU_DEBUG, DRIVERFLR_STATUS);
> +
> +	/* Trigger the actual Driver-FLR */
> +	intel_uncore_rmw_fw(uncore, GU_CNTL, 0, DRIVERFLR);
> +
> +	ret = intel_wait_for_register_fw(uncore, GU_DEBUG,
> +					 DRIVERFLR_STATUS, DRIVERFLR_STATUS,
> +					 flr_timeout_ms);
> +	if (ret) {
> +		drm_err(&i915->drm, "wait for Driver-FLR completion failed! %d\n", ret);
> +		return;
> +	}
> +
> +	intel_uncore_write_fw(uncore, GU_DEBUG, DRIVERFLR_STATUS);
> +
> +	return;
> +}
> +
>  /* Called via drm-managed action */
>  void intel_uncore_fini_mmio(struct drm_device *dev, void *data)
>  {
> @@ -2716,6 +2771,9 @@ void intel_uncore_fini_mmio(struct drm_device *dev, void *data)
>  		intel_uncore_fw_domains_fini(uncore);
>  		iosf_mbi_punit_release();
>  	}
> +
> +	if (intel_uncore_needs_flr_on_fini(uncore))
> +		driver_initiated_flr(uncore);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index e9e38490815d..9ea1f4864a3a 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -153,6 +153,7 @@ struct intel_uncore {
>  #define UNCORE_HAS_FPGA_DBG_UNCLAIMED	BIT(1)
>  #define UNCORE_HAS_DBG_UNCLAIMED	BIT(2)
>  #define UNCORE_HAS_FIFO			BIT(3)
> +#define UNCORE_NEEDS_FLR_ON_FINI	BIT(4)
>  
>  	const struct intel_forcewake_range *fw_domains_table;
>  	unsigned int fw_domains_table_entries;
> @@ -223,6 +224,18 @@ intel_uncore_has_fifo(const struct intel_uncore *uncore)
>  	return uncore->flags & UNCORE_HAS_FIFO;
>  }
>  
> +static inline bool
> +intel_uncore_needs_flr_on_fini(const struct intel_uncore *uncore)
> +{
> +	return uncore->flags & UNCORE_NEEDS_FLR_ON_FINI;
> +}
> +
> +static inline bool
> +intel_uncore_set_flr_on_fini(struct intel_uncore *uncore)
> +{
> +	return uncore->flags |= UNCORE_NEEDS_FLR_ON_FINI;
> +}
> +
>  void intel_uncore_mmio_debug_init_early(struct drm_i915_private *i915);
>  void intel_uncore_init_early(struct intel_uncore *uncore,
>  			     struct intel_gt *gt);
> -- 
> 2.37.3
> 

Re: [PATCH] drm/i915/gsc: GSC firmware loading

[Teres Alexis, Alan Previn]

Diffed the patches and reviewed the changes -- i.e. the use of the worker for the gsc fw loading cmd submission.
All looks good - just a few nits below.

Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>

On Mon, 2022-12-05 at 21:15 -0800, Ceraolo Spurio, Daniele wrote:
> GSC FW is loaded by submitting a dedicated command via the GSC engine.
> The memory area used for loading the FW is then re-purposed as local
> memory for the GSC itself, so we use a separate allocation instead of
> using the one where we keep the firmware stored for reload.
> 
> 
> 
Alan:[snip]


> +int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc)
> +{
> +	struct intel_gt *gt = gsc_uc_to_gt(gsc);
> +	struct intel_uc_fw *gsc_fw = &gsc->fw;
> +	int err;
> +
> +	/* check current fw status */
> +	if (intel_gsc_uc_fw_init_done(gsc)) {
> +		if (GEM_WARN_ON(!intel_uc_fw_is_loaded(gsc_fw)))
> +			intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
> +		return -EEXIST;
> +	}
> +
Alan: Nit: I see you've put the -EEXIST back again - not sure if we need it. I'm marking this as Nit simply because we
dont consumer the return value from where its being called - but its still weird that we are returning an error in the
case where FW is already there so we skip loading and allow normal operational flow (not error-ing out).

Alan: Nit: not sure if we should explain in the comments how we can already find the gsc-fw pre-loaded (IIRC, it could
be a prior driver unload that didn't do the FLR?).

> +	if (!intel_uc_fw_is_loadable(gsc_fw))
> +		return -ENOEXEC;
> +
> +	/* FW blob is ok, so clean the status */
> +	intel_uc_fw_sanitize(&gsc->fw);
> +
> +	if (!gsc_is_in_reset(gt->uncore))
> +		return -EIO;
> +
> +	err = gsc_fw_load_prepare(gsc);
> +	if (err)
> +		goto fail;
> +
> +	

Alan: [snip]

> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> index 65cbf1ce9fa1..3692ba387834 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> @@ -7,8 +7,19 @@
>  
>  #include "gt/intel_gt.h"
>  #include "intel_gsc_uc.h"
> +#include "intel_gsc_fw.h"

Alan: alphabetical ordering?  (not sure if this is a hard rule, for me its a nit)

>  #include "i915_drv.h"
>  
> +static void gsc_work(struct work_struct *work)
Alan: Nit: would ne nicer to call it gsc_load_worker unless there maybe future plans to expand this worker's scope.
> +{
> +	struct intel_gsc_uc *gsc = container_of(work, typeof(*gsc), work);
> +	struct intel_gt *gt = gsc_uc_to_gt(gsc);
> +	intel_wakeref_t wakeref;
> +
> +	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> +		intel_gsc_uc_fw_upload(gsc);
> +}
> +

Alan:[snip]


>  struct intel_gsc_uc {
>  	/* Generic uC firmware management */
>  	struct intel_uc_fw fw;
> +
> +	/* GSC-specific additions */
> +	struct i915_vma *local; /* private memory for GSC usage */
> +	struct intel_context *ce; /* for submission to GSC FW via GSC engine */
> +
> +	struct work_struct work; /* for delayed load */
Alan: nit: would be nicer to call it "load_worker" unless the future plan is to expand the scope of what else that
worker could be used for.

Alan:[snip]

Re: [PATCH] drm/i915/gsc: GSC firmware loading

[Ceraolo Spurio, Daniele]

On 12/7/2022 2:16 AM, Teres Alexis, Alan Previn wrote:
> Diffed the patches and reviewed the changes -- i.e. the use of the worker for the gsc fw loading cmd submission.
> All looks good - just a few nits below.
>
> Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>
> On Mon, 2022-12-05 at 21:15 -0800, Ceraolo Spurio, Daniele wrote:
>> GSC FW is loaded by submitting a dedicated command via the GSC engine.
>> The memory area used for loading the FW is then re-purposed as local
>> memory for the GSC itself, so we use a separate allocation instead of
>> using the one where we keep the firmware stored for reload.
>>
>>
>>
> Alan:[snip]
>
>
>> +int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc)
>> +{
>> +	struct intel_gt *gt = gsc_uc_to_gt(gsc);
>> +	struct intel_uc_fw *gsc_fw = &gsc->fw;
>> +	int err;
>> +
>> +	/* check current fw status */
>> +	if (intel_gsc_uc_fw_init_done(gsc)) {
>> +		if (GEM_WARN_ON(!intel_uc_fw_is_loaded(gsc_fw)))
>> +			intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
>> +		return -EEXIST;
>> +	}
>> +
> Alan: Nit: I see you've put the -EEXIST back again - not sure if we need it. I'm marking this as Nit simply because we
> dont consumer the return value from where its being called - but its still weird that we are returning an error in the
> case where FW is already there so we skip loading and allow normal operational flow (not error-ing out).
>
> Alan: Nit: not sure if we should explain in the comments how we can already find the gsc-fw pre-loaded (IIRC, it could
> be a prior driver unload that didn't do the FLR?).

It should be impossible to get here with an already loaded FW, because 
we only queue the worker if the FW is not already loaded. However, for 
safety I wanted to add in a check to make sure we're covered in case 
something weird happens. Note that there is a GEM_WARN_ON tied to 
intel_uc_fw_is_loaded(); this is because if that function returns true 
it means the driver knows that the FW is already loaded and therefore 
the error is just that the worker got called one time too many, while if 
that is false it means that we got out of sync with the HW state and 
that's a serious bug we want to flag.

Regarding the -EEXIST, that's in preparation for a follow up patch that 
adds more functions to the worker (SW proxy) that will have to be 
skipped if the GSC is already loaded. As you said it doesn't make a 
difference now so I thought to start directly with it instead of 
returning zero now and switching later.

>
>> +	if (!intel_uc_fw_is_loadable(gsc_fw))
>> +		return -ENOEXEC;
>> +
>> +	/* FW blob is ok, so clean the status */
>> +	intel_uc_fw_sanitize(&gsc->fw);
>> +
>> +	if (!gsc_is_in_reset(gt->uncore))
>> +		return -EIO;
>> +
>> +	err = gsc_fw_load_prepare(gsc);
>> +	if (err)
>> +		goto fail;
>> +
>> +	
> Alan: [snip]
>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
>> index 65cbf1ce9fa1..3692ba387834 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
>> @@ -7,8 +7,19 @@
>>   
>>   #include "gt/intel_gt.h"
>>   #include "intel_gsc_uc.h"
>> +#include "intel_gsc_fw.h"
> Alan: alphabetical ordering?  (not sure if this is a hard rule, for me its a nit)
>
>>   #include "i915_drv.h"
>>   
>> +static void gsc_work(struct work_struct *work)
> Alan: Nit: would ne nicer to call it gsc_load_worker unless there maybe future plans to expand this worker's scope.

There is, this same worker will handle sw proxy as well.

>> +{
>> +	struct intel_gsc_uc *gsc = container_of(work, typeof(*gsc), work);
>> +	struct intel_gt *gt = gsc_uc_to_gt(gsc);
>> +	intel_wakeref_t wakeref;
>> +
>> +	with_intel_runtime_pm(gt->uncore->rpm, wakeref)
>> +		intel_gsc_uc_fw_upload(gsc);
>> +}
>> +
> Alan:[snip]
>
>
>>   struct intel_gsc_uc {
>>   	/* Generic uC firmware management */
>>   	struct intel_uc_fw fw;
>> +
>> +	/* GSC-specific additions */
>> +	struct i915_vma *local; /* private memory for GSC usage */
>> +	struct intel_context *ce; /* for submission to GSC FW via GSC engine */
>> +
>> +	struct work_struct work; /* for delayed load */
> Alan: nit: would be nicer to call it "load_worker" unless the future plan is to expand the scope of what else that
> worker could be used for.

same as above.

Daniele

>
> Alan:[snip]
>
>

Re: [PATCH v2 1/6] drm/i915/uc: Introduce GSC FW

[Teres Alexis, Alan Previn]

On Mon, 2022-12-05 at 17:19 -0800, Ceraolo Spurio, Daniele wrote:
> On MTL the GSC FW needs to be loaded on the media GT by the graphics
> driver. We're going to treat it like a new uc_fw, so add the initial
> defs and init/fini functions for it.
> 
> Similarly to the other FWs, the GSC FW path can be overriden via
> modparam. The modparam can also be used to disable the GSC FW loading by
> setting it to an empty string.
> 
> Note that the new structure has been called intel_gsc_uc to avoid
> confusion with the existing intel_gsc, which instead represents the heci
> gsc interfaces.
> 
> v2: re-order Makefile list to be properly sorted (Jani, Alan), better
>     comment (alan)
> 
Alan:[snip]

As per offline conversation, i re-reviewed the difference from Rev1 to Rev2 - i.e. just the makefile and the comment changes, all looks good, so:
Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>

Re: [PATCH v2 2/6] drm/i915/gsc: Skip the version check when fetching the GSC FW

[Teres Alexis, Alan Previn]

On Mon, 2022-12-05 at 17:19 -0800, Ceraolo Spurio, Daniele wrote:
> The current exectation from the FW side is that the driver will query
> the GSC FW version after the FW is loaded, similarly to what the mei
> driver does on DG2. However, we're discussing with the FW team if there
> is a way to extract the version from the bin file before loading, so we
> can keep the code the same as for older FWs.
> 
> Since the GSC FW version is not currently required for functionality and
> is only needed for debug purposes, we can skip the FW version for now at
> fetch time and add it later on when we've agreed on the approach.
> 
> v2: rebased on uc_fw version struct changes.
> 
Alan:[snip]
Reviewed the changes that came from the rebase (upstream merges) - so minor i missed it at first.
Functionally identical as before so:
Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>