* [Intel-gfx] [PATCH 0/6] drm/i915: Add support for GSC FW loading
@ 2022-11-21 23:16 Daniele Ceraolo Spurio
2022-11-21 23:16 ` [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW Daniele Ceraolo Spurio
` (8 more replies)
0 siblings, 9 replies; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2022-11-21 23:16 UTC (permalink / raw)
To: intel-gfx; +Cc: Alan Previn, dri-devel, Rodrigo Vivi
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. The loading is
currently done serially as part of driver init/resume, but the plan is
to move it to a worker thread in the future to allow non-CP submission
to go through while we wait for GSC FW load to complete. This will be
done once all the pieces required for GSC to fully function are in place
(see further below).
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.
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 | 4 +-
drivers/gpu/drm/i915/gem/i915_gem_pm.c | 14 +-
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.c | 11 ++
drivers/gpu/drm/i915/gt/intel_gt.h | 5 +
drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 195 +++++++++++++++++++
drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 13 ++
drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 103 ++++++++++
drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 43 ++++
drivers/gpu/drm/i915/gt/uc/intel_uc.c | 32 +++
drivers/gpu/drm/i915/gt/uc/intel_uc.h | 5 +
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 74 +++++--
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 | 48 +++++
drivers/gpu/drm/i915/intel_uncore.h | 13 ++
21 files changed, 585 insertions(+), 19 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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW
2022-11-21 23:16 [Intel-gfx] [PATCH 0/6] drm/i915: Add support for GSC FW loading Daniele Ceraolo Spurio
@ 2022-11-21 23:16 ` Daniele Ceraolo Spurio
2022-11-22 9:03 ` Jani Nikula
2022-11-29 23:48 ` Teres Alexis, Alan Previn
2022-11-21 23:16 ` [Intel-gfx] [PATCH 2/6] drm/i915/gsc: Skip the version check when fetching the " Daniele Ceraolo Spurio
` (7 subsequent siblings)
8 siblings, 2 replies; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2022-11-21 23:16 UTC (permalink / raw)
To: intel-gfx; +Cc: Alan Previn, dri-devel
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.
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>
---
drivers/gpu/drm/i915/Makefile | 3 +-
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 | 25 +++++++-
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, 164 insertions(+), 6 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..92d37cf71e16 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -205,7 +205,8 @@ 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_gsc_uc.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 1d28286e6f06..622935770aa1 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 0c80ba51a4bd..5b2e4503aee7 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,10 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
int i;
bool found;
+ /* FW is not defined until all the support is in place */
+ 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
@@ -374,6 +385,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;
@@ -385,6 +401,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 bc898ba5355d..5d2a8965a592 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
/*
* The firmware build process will generate a version header file with major and
@@ -205,6 +206,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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Intel-gfx] [PATCH 2/6] drm/i915/gsc: Skip the version check when fetching the GSC FW
2022-11-21 23:16 [Intel-gfx] [PATCH 0/6] drm/i915: Add support for GSC FW loading Daniele Ceraolo Spurio
2022-11-21 23:16 ` [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW Daniele Ceraolo Spurio
@ 2022-11-21 23:16 ` Daniele Ceraolo Spurio
2022-11-22 18:53 ` Rodrigo Vivi
2022-11-21 23:16 ` [Intel-gfx] [PATCH 3/6] drm/i915/gsc: GSC firmware loading Daniele Ceraolo Spurio
` (6 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2022-11-21 23:16 UTC (permalink / raw)
To: intel-gfx; +Cc: Alan Previn, dri-devel
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.
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>
---
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 5b2e4503aee7..3df52fd59edc 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -564,6 +564,26 @@ static int check_ccs_header(struct intel_gt *gt,
return 0;
}
+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
@@ -633,14 +653,11 @@ 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->file_wanted.major_ver) {
+ if (uc_fw->file_wanted.major_ver && uc_fw->file_selected.major_ver) {
/* Check the file's major version was as it claimed */
if (uc_fw->file_selected.major_ver != uc_fw->file_wanted.major_ver) {
drm_notice(&i915->drm, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
@@ -657,7 +674,7 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
}
}
- if (old_ver) {
+ if (old_ver && uc_fw->file_selected.major_ver) {
/* Preserve the version that was really wanted */
memcpy(&uc_fw->file_wanted, &file_ideal, sizeof(uc_fw->file_wanted));
--
2.37.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Intel-gfx] [PATCH 3/6] drm/i915/gsc: GSC firmware loading
2022-11-21 23:16 [Intel-gfx] [PATCH 0/6] drm/i915: Add support for GSC FW loading Daniele Ceraolo Spurio
2022-11-21 23:16 ` [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW Daniele Ceraolo Spurio
2022-11-21 23:16 ` [Intel-gfx] [PATCH 2/6] drm/i915/gsc: Skip the version check when fetching the " Daniele Ceraolo Spurio
@ 2022-11-21 23:16 ` Daniele Ceraolo Spurio
2022-11-22 19:01 ` Rodrigo Vivi
2022-12-01 22:00 ` Teres Alexis, Alan Previn
2022-11-21 23:16 ` [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded Daniele Ceraolo Spurio
` (5 subsequent siblings)
8 siblings, 2 replies; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2022-11-21 23:16 UTC (permalink / raw)
To: intel-gfx; +Cc: Alan Previn, dri-devel
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.
Note that the GSC load takes a lot of time (up to a few hundred ms).
This patch loads it serially as part of driver init/resume, but, given
that GSC is only required for PM and content-protection features
(media C6, PXP, HDCP), we could move the load to a worker thread to unblock
non-CP userspace submissions earlier. This will be done as a follow up
step, because there are extra init steps required to actually make use of
the GSC (including a mei component) and it will be cleaner (and easier to
review) if we implement the async load once all the pieces we need for GSC
to work are in place. A TODO has been added to the code to mark this
intention.
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>
---
drivers/gpu/drm/i915/Makefile | 1 +
drivers/gpu/drm/i915/gem/i915_gem_pm.c | 14 +-
drivers/gpu/drm/i915/gt/intel_engine.h | 2 +
drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 7 +
drivers/gpu/drm/i915/gt/intel_gt.c | 11 ++
drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 186 +++++++++++++++++++
drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 13 ++
drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 35 +++-
drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 7 +
drivers/gpu/drm/i915/gt/uc/intel_uc.c | 15 ++
drivers/gpu/drm/i915/gt/uc/intel_uc.h | 2 +
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 20 +-
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 1 +
13 files changed, 307 insertions(+), 7 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 92d37cf71e16..1d45a6f451fa 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -206,6 +206,7 @@ i915-y += gt/uc/intel_uc.o \
gt/uc/intel_huc.o \
gt/uc/intel_huc_debugfs.o \
gt/uc/intel_huc_fw.o \
+ gt/uc/intel_gsc_fw.o \
gt/uc/intel_gsc_uc.o
# graphics system controller (GSC) support
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
index 0d812f4d787d..f77eb4009aba 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
@@ -232,10 +232,22 @@ void i915_gem_resume(struct drm_i915_private *i915)
* guarantee that the context image is complete. So let's just reset
* it and start again.
*/
- for_each_gt(gt, i915, i)
+ for_each_gt(gt, i915, i) {
if (intel_gt_resume(gt))
goto err_wedged;
+ /*
+ * TODO: this is a long operation (up to ~200ms) and we don't
+ * need to complete it before driver load/resume is done, so it
+ * should be handled in a separate thread to unlock userspace
+ * submission. However, there are a couple of other pieces that
+ * are required for full GSC support that will complicate things
+ * a bit, and it is easier to move everything to a worker at the
+ * same time, so keep it here for now.
+ */
+ intel_uc_init_hw_late(>->uc);
+ }
+
ret = lmem_restore(i915, I915_TTM_BACKUP_ALLOW_GPU);
GEM_WARN_ON(ret);
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/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
index b5ad9caa5537..a12e74a1fe5d 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -750,6 +750,17 @@ int intel_gt_init(struct intel_gt *gt)
intel_pxp_init(>->pxp);
+ /*
+ * TODO: this is a long operation (up to ~200ms) and we don't need
+ * to complete it before driver load/resume is done, so it should
+ * be handled in a separate thread to unlock userspace submission.
+ * However, there are a couple of other pieces that are required
+ * for full GSC support that will complicate things a bit, and it
+ * is easier to move everything to a worker at the same time, so
+ * keep it here for now.
+ */
+ intel_uc_init_hw_late(>->uc);
+
goto out_fw;
err_gt:
__intel_gt_disable(gt);
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..510fb47193ec
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
@@ -0,0 +1,186 @@
+// 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 GENMASK(3, 0)
+#define GSC_FW_CURRENT_STATE_RESET 0
+#define GSC_FW_INIT_COMPLETE_BIT 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;
+}
+
+static bool gsc_init_done(struct intel_uncore *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_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 (gsc_init_done(gt->uncore)) {
+ if (GEM_WARN_ON(!intel_uc_fw_is_loaded(gsc_fw)))
+ intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
+ return 0;
+ }
+
+ 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(>->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..d714e5e69a6d
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#ifndef _INTEL_GSC_FW_H_
+#define _INTEL_GSC_FW_H_
+
+struct intel_gsc_uc;
+
+int intel_gsc_fw_upload(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..dd985a0f0613 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -45,17 +45,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(>->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(>->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 +95,9 @@ void intel_gsc_uc_fini(struct intel_gsc_uc *gsc)
if (!intel_uc_fw_is_loadable(&gsc->fw))
return;
+ 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);
}
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..6fdb54243372 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
@@ -8,9 +8,16 @@
#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 */
};
void intel_gsc_uc_init_early(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 622935770aa1..b2c2512a3391 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"
@@ -593,6 +594,19 @@ static void __uc_fini_hw(struct intel_uc *uc)
__uc_sanitize(uc);
}
+/*
+ * Firmwares loaded via the GSC engine require the submission back-end to have
+ * been initialized, so can only be loaded late in the probe/resume process.
+ * TODO move to worker
+ */
+static int __uc_init_hw_late(struct intel_uc *uc)
+{
+ if (!intel_uc_uses_gsc_uc(uc))
+ return 0;
+
+ return intel_gsc_fw_upload(&uc->gsc);
+}
+
/**
* intel_uc_reset_prepare - Prepare for reset
* @uc: the intel_uc structure
@@ -751,5 +765,6 @@ static const struct intel_uc_ops uc_ops_on = {
.fini = __uc_fini,
.init_hw = __uc_init_hw,
+ .init_hw_late = __uc_init_hw_late,
.fini_hw = __uc_fini_hw,
};
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index 5d0f1bcc381e..39993ba6c24e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -23,6 +23,7 @@ struct intel_uc_ops {
int (*init)(struct intel_uc *uc);
void (*fini)(struct intel_uc *uc);
int (*init_hw)(struct intel_uc *uc);
+ int (*init_hw_late)(struct intel_uc *uc);
void (*fini_hw)(struct intel_uc *uc);
};
@@ -112,6 +113,7 @@ intel_uc_ops_function(cleanup_firmwares, fini_fw, void, );
intel_uc_ops_function(init, init, int, 0);
intel_uc_ops_function(fini, fini, void, );
intel_uc_ops_function(init_hw, init_hw, int, 0);
+intel_uc_ops_function(init_hw_late, init_hw_late, int, 0);
intel_uc_ops_function(fini_hw, fini_hw, void, );
#undef intel_uc_ops_function
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 3df52fd59edc..dcec115209de 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -836,6 +836,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
@@ -872,11 +886,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 5d2a8965a592..05760fb85fff 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
@@ -290,6 +290,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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded
2022-11-21 23:16 [Intel-gfx] [PATCH 0/6] drm/i915: Add support for GSC FW loading Daniele Ceraolo Spurio
` (2 preceding siblings ...)
2022-11-21 23:16 ` [Intel-gfx] [PATCH 3/6] drm/i915/gsc: GSC firmware loading Daniele Ceraolo Spurio
@ 2022-11-21 23:16 ` Daniele Ceraolo Spurio
2022-11-22 0:17 ` Ceraolo Spurio, Daniele
` (2 more replies)
2022-11-21 23:16 ` [Intel-gfx] [PATCH 5/6] drm/i915/gsc: Disable GSC engine and power well if FW is not selected Daniele Ceraolo Spurio
` (4 subsequent siblings)
8 siblings, 3 replies; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2022-11-21 23:16 UTC (permalink / raw)
To: intel-gfx; +Cc: Alan Previn, dri-devel
If the GSC was loaded, the only way to stop it during the driver unload
flow is to do a driver-FLR.
The driver-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.
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 9 +++++
drivers/gpu/drm/i915/i915_reg.h | 3 ++
drivers/gpu/drm/i915/intel_uncore.c | 45 +++++++++++++++++++++++
drivers/gpu/drm/i915/intel_uncore.h | 13 +++++++
4 files changed, 70 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 510fb47193ec..5dad3c19c445 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
@@ -173,6 +173,15 @@ int intel_gsc_fw_upload(struct intel_gsc_uc *gsc)
if (err)
goto fail;
+ /*
+ * Once the GSC FW is loaded, the only way to kill it on driver unload
+ * is to do a driver FLR. Given this is a very disruptive action, 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.
+ */
+ intel_uncore_set_flr_on_fini(>->i915->uncore);
+
/* FW is not fully operational until we enable SW proxy */
intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8e1892d14774..60e55245200b 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..c1befa33ff59 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -2703,6 +2703,48 @@ void intel_uncore_prune_engine_fw_domains(struct intel_uncore *uncore,
}
}
+static void driver_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 +2758,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_flr(uncore);
}
/**
diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 5449146a0624..a9fa0b11e7e4 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(3)
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Intel-gfx] [PATCH 5/6] drm/i915/gsc: Disable GSC engine and power well if FW is not selected
2022-11-21 23:16 [Intel-gfx] [PATCH 0/6] drm/i915: Add support for GSC FW loading Daniele Ceraolo Spurio
` (3 preceding siblings ...)
2022-11-21 23:16 ` [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded Daniele Ceraolo Spurio
@ 2022-11-21 23:16 ` Daniele Ceraolo Spurio
2022-11-22 20:52 ` Rodrigo Vivi
2022-11-21 23:16 ` [Intel-gfx] [PATCH 6/6] drm/i915/mtl: MTL has one GSC CS on the media GT Daniele Ceraolo Spurio
` (3 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2022-11-21 23:16 UTC (permalink / raw)
To: intel-gfx; +Cc: Jonathan Cavitt, dri-devel, Rodrigo Vivi
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>
---
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(>->uc)) {
+ drm_notice(>->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 c1befa33ff59..e63d957b59eb 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);
}
static void driver_flr(struct intel_uncore *uncore)
--
2.37.3
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Intel-gfx] [PATCH 6/6] drm/i915/mtl: MTL has one GSC CS on the media GT
2022-11-21 23:16 [Intel-gfx] [PATCH 0/6] drm/i915: Add support for GSC FW loading Daniele Ceraolo Spurio
` (4 preceding siblings ...)
2022-11-21 23:16 ` [Intel-gfx] [PATCH 5/6] drm/i915/gsc: Disable GSC engine and power well if FW is not selected Daniele Ceraolo Spurio
@ 2022-11-21 23:16 ` Daniele Ceraolo Spurio
2022-11-22 20:52 ` Rodrigo Vivi
2022-11-21 23:42 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add support for GSC FW loading Patchwork
` (2 subsequent siblings)
8 siblings, 1 reply; 30+ messages in thread
From: Daniele Ceraolo Spurio @ 2022-11-21 23:16 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel, Rodrigo Vivi
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>
---
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 6da9784fe4a2..46acbf390195 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -1124,7 +1124,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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add support for GSC FW loading
2022-11-21 23:16 [Intel-gfx] [PATCH 0/6] drm/i915: Add support for GSC FW loading Daniele Ceraolo Spurio
` (5 preceding siblings ...)
2022-11-21 23:16 ` [Intel-gfx] [PATCH 6/6] drm/i915/mtl: MTL has one GSC CS on the media GT Daniele Ceraolo Spurio
@ 2022-11-21 23:42 ` Patchwork
2022-11-21 23:42 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2022-11-22 0:08 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
8 siblings, 0 replies; 30+ messages in thread
From: Patchwork @ 2022-11-21 23:42 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Add support for GSC FW loading
URL : https://patchwork.freedesktop.org/series/111170/
State : warning
== Summary ==
Error: dim checkpatch failed
450d4acc03ed drm/i915/uc: Introduce GSC FW
Traceback (most recent call last):
File "scripts/spdxcheck.py", line 11, in <module>
import git
ModuleNotFoundError: No module named 'git'
Traceback (most recent call last):
File "scripts/spdxcheck.py", line 11, in <module>
import git
ModuleNotFoundError: No module named 'git'
-:10: WARNING:TYPO_SPELLING: 'overriden' may be misspelled - perhaps 'overridden'?
#10:
Similarly to the other FWs, the GSC FW path can be overriden via
^^^^^^^^^
-:53: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#53:
new file mode 100644
-:79: 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
#79: FILE: drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c:22:
+ GEM_BUG_ON(!gt_is_root(gt) && !gt->info.engine_mask);
-:97: ERROR:SPACING: space required before the open brace '{'
#97: FILE: drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c:40:
+ if (!gsc_engine_supported(gsc_uc_to_gt(gsc))){
-:165: 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
#165: FILE: drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h:32:
+ GEM_BUG_ON(__intel_uc_fw_status(&gsc->fw) == INTEL_UC_FIRMWARE_SELECTED);
-:270: 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
#270: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:22:
+ GEM_BUG_ON(type >= INTEL_UC_FW_NUM_TYPES);
-:333: CHECK:LINE_SPACING: Please use a blank line after function/struct/union/enum declarations
#333: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h:67:
};
+#define INTEL_UC_FW_NUM_TYPES 3
-:355: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#355: FILE: drivers/gpu/drm/i915/i915_params.c:196:
+i915_param_named_unsafe(gsc_firmware_path, charp, 0400,
+ "GSC firmware path to use instead of the default one");
total: 1 errors, 5 warnings, 2 checks, 289 lines checked
4a0e0515da43 drm/i915/gsc: Skip the version check when fetching the GSC FW
33d39c597ad6 drm/i915/gsc: GSC firmware loading
Traceback (most recent call last):
File "scripts/spdxcheck.py", line 11, in <module>
import git
ModuleNotFoundError: No module named 'git'
Traceback (most recent call last):
File "scripts/spdxcheck.py", line 11, in <module>
import git
ModuleNotFoundError: No module named 'git'
-:103: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#103: FILE: drivers/gpu/drm/i915/gt/intel_gpu_commands.h:440:
+#define HECI1_FW_LIMIT_VALID (1<<31)
^
-:131: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#131:
new file mode 100644
-:495: 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
#495: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:843:
+ GEM_BUG_ON(!intel_uc_fw_is_loadable(uc_fw));
total: 0 errors, 2 warnings, 1 checks, 430 lines checked
4ec90ede72df drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded
-:104: WARNING:RETURN_VOID: void function return statements are not generally useful
#104: FILE: drivers/gpu/drm/i915/intel_uncore.c:2746:
+ return;
+}
total: 0 errors, 1 warnings, 0 checks, 106 lines checked
79ac96c18521 drm/i915/gsc: Disable GSC engine and power well if FW is not selected
69e22032d6be drm/i915/mtl: MTL has one GSC CS on the media GT
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: Add support for GSC FW loading
2022-11-21 23:16 [Intel-gfx] [PATCH 0/6] drm/i915: Add support for GSC FW loading Daniele Ceraolo Spurio
` (6 preceding siblings ...)
2022-11-21 23:42 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add support for GSC FW loading Patchwork
@ 2022-11-21 23:42 ` Patchwork
2022-11-22 0:08 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
8 siblings, 0 replies; 30+ messages in thread
From: Patchwork @ 2022-11-21 23:42 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Add support for GSC FW loading
URL : https://patchwork.freedesktop.org/series/111170/
State : warning
== Summary ==
Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Add support for GSC FW loading
2022-11-21 23:16 [Intel-gfx] [PATCH 0/6] drm/i915: Add support for GSC FW loading Daniele Ceraolo Spurio
` (7 preceding siblings ...)
2022-11-21 23:42 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
@ 2022-11-22 0:08 ` Patchwork
8 siblings, 0 replies; 30+ messages in thread
From: Patchwork @ 2022-11-22 0:08 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 6592 bytes --]
== Series Details ==
Series: drm/i915: Add support for GSC FW loading
URL : https://patchwork.freedesktop.org/series/111170/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_12409 -> Patchwork_111170v1
====================================================
Summary
-------
**FAILURE**
Serious unknown changes coming with Patchwork_111170v1 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_111170v1, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111170v1/index.html
Participating hosts (35 -> 35)
------------------------------
Additional (2): bat-rpls-2 bat-dg2-11
Missing (2): fi-ilk-m540 fi-rkl-11600
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_111170v1:
### IGT changes ###
#### Possible regressions ####
* igt@i915_selftest@live@gt_engines:
- fi-ivb-3770: [PASS][1] -> [DMESG-WARN][2] +39 similar issues
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12409/fi-ivb-3770/igt@i915_selftest@live@gt_engines.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111170v1/fi-ivb-3770/igt@i915_selftest@live@gt_engines.html
* igt@i915_selftest@live@perf:
- fi-hsw-4770: [PASS][3] -> [DMESG-WARN][4] +40 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12409/fi-hsw-4770/igt@i915_selftest@live@perf.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111170v1/fi-hsw-4770/igt@i915_selftest@live@perf.html
* igt@i915_selftest@live@uncore:
- fi-snb-2520m: [PASS][5] -> [DMESG-WARN][6] +39 similar issues
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12409/fi-snb-2520m/igt@i915_selftest@live@uncore.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111170v1/fi-snb-2520m/igt@i915_selftest@live@uncore.html
Known issues
------------
Here are the changes found in Patchwork_111170v1 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_render_tiled_blits@basic:
- fi-apl-guc: [PASS][7] -> [INCOMPLETE][8] ([i915#7056])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12409/fi-apl-guc/igt@gem_render_tiled_blits@basic.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111170v1/fi-apl-guc/igt@gem_render_tiled_blits@basic.html
#### Possible fixes ####
* igt@gem_exec_suspend@basic-s3@smem:
- {bat-rplp-1}: [DMESG-WARN][9] ([i915#2867]) -> [PASS][10]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12409/bat-rplp-1/igt@gem_exec_suspend@basic-s3@smem.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111170v1/bat-rplp-1/igt@gem_exec_suspend@basic-s3@smem.html
* igt@i915_selftest@live@hangcheck:
- {bat-dg2-9}: [INCOMPLETE][11] ([i915#7349]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12409/bat-dg2-9/igt@i915_selftest@live@hangcheck.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111170v1/bat-dg2-9/igt@i915_selftest@live@hangcheck.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
[fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
[i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
[i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
[i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
[i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[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#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
[i915#4258]: https://gitlab.freedesktop.org/drm/intel/issues/4258
[i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
[i915#4418]: https://gitlab.freedesktop.org/drm/intel/issues/4418
[i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
[i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
[i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
[i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
[i915#6257]: https://gitlab.freedesktop.org/drm/intel/issues/6257
[i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
[i915#7056]: https://gitlab.freedesktop.org/drm/intel/issues/7056
[i915#7346]: https://gitlab.freedesktop.org/drm/intel/issues/7346
[i915#7348]: https://gitlab.freedesktop.org/drm/intel/issues/7348
[i915#7349]: https://gitlab.freedesktop.org/drm/intel/issues/7349
[i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
[i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
Build changes
-------------
* Linux: CI_DRM_12409 -> Patchwork_111170v1
CI-20190529: 20190529
CI_DRM_12409: ab70218208c05b2a68a1143438a56c3d494eb529 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7069: 40a2de5cc6a6b43af7da7905bfe1ede9d9a3200c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_111170v1: ab70218208c05b2a68a1143438a56c3d494eb529 @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
8476feaf19d5 drm/i915/mtl: MTL has one GSC CS on the media GT
faba11959cd4 drm/i915/gsc: Disable GSC engine and power well if FW is not selected
131e5e37fc13 drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded
9d9b1090faaf drm/i915/gsc: GSC firmware loading
0068cf3d27fb drm/i915/gsc: Skip the version check when fetching the GSC FW
9fdbfecae323 drm/i915/uc: Introduce GSC FW
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111170v1/index.html
[-- Attachment #2: Type: text/html, Size: 5365 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded
2022-11-21 23:16 ` [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded Daniele Ceraolo Spurio
@ 2022-11-22 0:17 ` Ceraolo Spurio, Daniele
2022-11-22 20:46 ` Rodrigo Vivi
2022-12-01 22:40 ` Teres Alexis, Alan Previn
2 siblings, 0 replies; 30+ messages in thread
From: Ceraolo Spurio, Daniele @ 2022-11-22 0:17 UTC (permalink / raw)
To: intel-gfx; +Cc: Alan Previn, dri-devel
On 11/21/2022 3:16 PM, 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-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.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 9 +++++
> drivers/gpu/drm/i915/i915_reg.h | 3 ++
> drivers/gpu/drm/i915/intel_uncore.c | 45 +++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_uncore.h | 13 +++++++
> 4 files changed, 70 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 510fb47193ec..5dad3c19c445 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> @@ -173,6 +173,15 @@ int intel_gsc_fw_upload(struct intel_gsc_uc *gsc)
> if (err)
> goto fail;
>
> + /*
> + * Once the GSC FW is loaded, the only way to kill it on driver unload
> + * is to do a driver FLR. Given this is a very disruptive action, 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.
> + */
> + intel_uncore_set_flr_on_fini(>->i915->uncore);
> +
> /* FW is not fully operational until we enable SW proxy */
> intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8e1892d14774..60e55245200b 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..c1befa33ff59 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -2703,6 +2703,48 @@ void intel_uncore_prune_engine_fw_domains(struct intel_uncore *uncore,
> }
> }
>
> +static void driver_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 +2758,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_flr(uncore);
> }
>
> /**
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 5449146a0624..a9fa0b11e7e4 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(3)
Dumb mistake, this should be 4 (and that's why the series is failing on
older gens that don't support the driver FLR). Will wait for comments
before re-spinning.
Daniele
>
> 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);
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW
2022-11-21 23:16 ` [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW Daniele Ceraolo Spurio
@ 2022-11-22 9:03 ` Jani Nikula
2022-11-22 19:42 ` Ceraolo Spurio, Daniele
2022-11-29 23:48 ` Teres Alexis, Alan Previn
1 sibling, 1 reply; 30+ messages in thread
From: Jani Nikula @ 2022-11-22 9:03 UTC (permalink / raw)
To: Daniele Ceraolo Spurio, intel-gfx; +Cc: dri-devel, Alan Previn
On Mon, 21 Nov 2022, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> 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.
>
> 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>
> ---
> drivers/gpu/drm/i915/Makefile | 3 +-
> 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 | 25 +++++++-
> 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, 164 insertions(+), 6 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..92d37cf71e16 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -205,7 +205,8 @@ 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_gsc_uc.o
Comment near the top of the file:
# Please keep these build lists sorted!
>
> # 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 1d28286e6f06..622935770aa1 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"
And thus intel_gsc_uc.h becomes another file that causes the entire
driver to be rebuilt when modified.
*sad trombone*
> #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 0c80ba51a4bd..5b2e4503aee7 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,10 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
> int i;
> bool found;
>
> + /* FW is not defined until all the support is in place */
> + 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
> @@ -374,6 +385,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;
> @@ -385,6 +401,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 bc898ba5355d..5d2a8965a592 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
>
> /*
> * The firmware build process will generate a version header file with major and
> @@ -205,6 +206,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) \
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 2/6] drm/i915/gsc: Skip the version check when fetching the GSC FW
2022-11-21 23:16 ` [Intel-gfx] [PATCH 2/6] drm/i915/gsc: Skip the version check when fetching the " Daniele Ceraolo Spurio
@ 2022-11-22 18:53 ` Rodrigo Vivi
0 siblings, 0 replies; 30+ messages in thread
From: Rodrigo Vivi @ 2022-11-22 18:53 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx, dri-devel, Alan Previn
On Mon, Nov 21, 2022 at 03:16:13PM -0800, Daniele Ceraolo Spurio 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.
>
> 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>
> ---
> 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 5b2e4503aee7..3df52fd59edc 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -564,6 +564,26 @@ static int check_ccs_header(struct intel_gt *gt,
> return 0;
> }
>
> +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
> @@ -633,14 +653,11 @@ 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->file_wanted.major_ver) {
> + if (uc_fw->file_wanted.major_ver && uc_fw->file_selected.major_ver) {
> /* Check the file's major version was as it claimed */
> if (uc_fw->file_selected.major_ver != uc_fw->file_wanted.major_ver) {
> drm_notice(&i915->drm, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
> @@ -657,7 +674,7 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
> }
> }
>
> - if (old_ver) {
> + if (old_ver && uc_fw->file_selected.major_ver) {
> /* Preserve the version that was really wanted */
> memcpy(&uc_fw->file_wanted, &file_ideal, sizeof(uc_fw->file_wanted));
>
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 3/6] drm/i915/gsc: GSC firmware loading
2022-11-21 23:16 ` [Intel-gfx] [PATCH 3/6] drm/i915/gsc: GSC firmware loading Daniele Ceraolo Spurio
@ 2022-11-22 19:01 ` Rodrigo Vivi
2022-11-22 19:39 ` Ceraolo Spurio, Daniele
2022-12-01 22:00 ` Teres Alexis, Alan Previn
1 sibling, 1 reply; 30+ messages in thread
From: Rodrigo Vivi @ 2022-11-22 19:01 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx, dri-devel, Alan Previn
On Mon, Nov 21, 2022 at 03:16:14PM -0800, Daniele Ceraolo Spurio 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.
>
> The GSC is not reset as part of GT reset, so we only need to load it on
> first boot and S3/S4 exit.
>
> Note that the GSC load takes a lot of time (up to a few hundred ms).
> This patch loads it serially as part of driver init/resume, but, given
> that GSC is only required for PM and content-protection features
> (media C6, PXP, HDCP), we could move the load to a worker thread to unblock
> non-CP userspace submissions earlier. This will be done as a follow up
> step, because there are extra init steps required to actually make use of
> the GSC (including a mei component) and it will be cleaner (and easier to
> review) if we implement the async load once all the pieces we need for GSC
> to work are in place. A TODO has been added to the code to mark this
> intention.
>
> 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>
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> drivers/gpu/drm/i915/gem/i915_gem_pm.c | 14 +-
> drivers/gpu/drm/i915/gt/intel_engine.h | 2 +
> drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 7 +
> drivers/gpu/drm/i915/gt/intel_gt.c | 11 ++
> drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 186 +++++++++++++++++++
> drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 13 ++
> drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 35 +++-
> drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 7 +
> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 15 ++
> drivers/gpu/drm/i915/gt/uc/intel_uc.h | 2 +
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 20 +-
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 1 +
> 13 files changed, 307 insertions(+), 7 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 92d37cf71e16..1d45a6f451fa 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -206,6 +206,7 @@ i915-y += gt/uc/intel_uc.o \
> gt/uc/intel_huc.o \
> gt/uc/intel_huc_debugfs.o \
> gt/uc/intel_huc_fw.o \
> + gt/uc/intel_gsc_fw.o \
> gt/uc/intel_gsc_uc.o
>
> # graphics system controller (GSC) support
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> index 0d812f4d787d..f77eb4009aba 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> @@ -232,10 +232,22 @@ void i915_gem_resume(struct drm_i915_private *i915)
> * guarantee that the context image is complete. So let's just reset
> * it and start again.
> */
> - for_each_gt(gt, i915, i)
> + for_each_gt(gt, i915, i) {
> if (intel_gt_resume(gt))
> goto err_wedged;
>
> + /*
> + * TODO: this is a long operation (up to ~200ms) and we don't
> + * need to complete it before driver load/resume is done, so it
> + * should be handled in a separate thread to unlock userspace
> + * submission. However, there are a couple of other pieces that
> + * are required for full GSC support that will complicate things
> + * a bit, and it is easier to move everything to a worker at the
> + * same time, so keep it here for now.
> + */
> + intel_uc_init_hw_late(>->uc);
> + }
> +
> ret = lmem_restore(i915, I915_TTM_BACKUP_ALLOW_GPU);
> GEM_WARN_ON(ret);
>
> 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/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index b5ad9caa5537..a12e74a1fe5d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -750,6 +750,17 @@ int intel_gt_init(struct intel_gt *gt)
>
> intel_pxp_init(>->pxp);
>
> + /*
> + * TODO: this is a long operation (up to ~200ms) and we don't need
> + * to complete it before driver load/resume is done, so it should
> + * be handled in a separate thread to unlock userspace submission.
shouldn't we already add an workqueue for this?
> + * However, there are a couple of other pieces that are required
> + * for full GSC support that will complicate things a bit, and it
> + * is easier to move everything to a worker at the same time, so
> + * keep it here for now.
> + */
> + intel_uc_init_hw_late(>->uc);
> +
> goto out_fw;
> err_gt:
> __intel_gt_disable(gt);
> 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..510fb47193ec
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> @@ -0,0 +1,186 @@
> +// 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 GENMASK(3, 0)
> +#define GSC_FW_CURRENT_STATE_RESET 0
> +#define GSC_FW_INIT_COMPLETE_BIT BIT(9)
let's prefer the REG_GENMASK AND REG_BIT as documented in i915_reg.h
> +
> +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;
> +}
> +
> +static bool gsc_init_done(struct intel_uncore *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_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 (gsc_init_done(gt->uncore)) {
> + if (GEM_WARN_ON(!intel_uc_fw_is_loaded(gsc_fw)))
> + intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
> + return 0;
> + }
> +
> + 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(>->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..d714e5e69a6d
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#ifndef _INTEL_GSC_FW_H_
> +#define _INTEL_GSC_FW_H_
> +
> +struct intel_gsc_uc;
> +
> +int intel_gsc_fw_upload(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..dd985a0f0613 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> @@ -45,17 +45,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(>->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(>->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 +95,9 @@ void intel_gsc_uc_fini(struct intel_gsc_uc *gsc)
> if (!intel_uc_fw_is_loadable(&gsc->fw))
> return;
>
> + 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);
> }
> 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..6fdb54243372 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> @@ -8,9 +8,16 @@
>
> #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 */
> };
>
> void intel_gsc_uc_init_early(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 622935770aa1..b2c2512a3391 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"
> @@ -593,6 +594,19 @@ static void __uc_fini_hw(struct intel_uc *uc)
> __uc_sanitize(uc);
> }
>
> +/*
> + * Firmwares loaded via the GSC engine require the submission back-end to have
> + * been initialized, so can only be loaded late in the probe/resume process.
> + * TODO move to worker
> + */
> +static int __uc_init_hw_late(struct intel_uc *uc)
> +{
> + if (!intel_uc_uses_gsc_uc(uc))
> + return 0;
> +
> + return intel_gsc_fw_upload(&uc->gsc);
> +}
> +
> /**
> * intel_uc_reset_prepare - Prepare for reset
> * @uc: the intel_uc structure
> @@ -751,5 +765,6 @@ static const struct intel_uc_ops uc_ops_on = {
> .fini = __uc_fini,
>
> .init_hw = __uc_init_hw,
> + .init_hw_late = __uc_init_hw_late,
> .fini_hw = __uc_fini_hw,
> };
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> index 5d0f1bcc381e..39993ba6c24e 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> @@ -23,6 +23,7 @@ struct intel_uc_ops {
> int (*init)(struct intel_uc *uc);
> void (*fini)(struct intel_uc *uc);
> int (*init_hw)(struct intel_uc *uc);
> + int (*init_hw_late)(struct intel_uc *uc);
> void (*fini_hw)(struct intel_uc *uc);
> };
>
> @@ -112,6 +113,7 @@ intel_uc_ops_function(cleanup_firmwares, fini_fw, void, );
> intel_uc_ops_function(init, init, int, 0);
> intel_uc_ops_function(fini, fini, void, );
> intel_uc_ops_function(init_hw, init_hw, int, 0);
> +intel_uc_ops_function(init_hw_late, init_hw_late, int, 0);
> intel_uc_ops_function(fini_hw, fini_hw, void, );
> #undef intel_uc_ops_function
>
> 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 3df52fd59edc..dcec115209de 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -836,6 +836,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
> @@ -872,11 +886,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 5d2a8965a592..05760fb85fff 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> @@ -290,6 +290,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
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 3/6] drm/i915/gsc: GSC firmware loading
2022-11-22 19:01 ` Rodrigo Vivi
@ 2022-11-22 19:39 ` Ceraolo Spurio, Daniele
2022-11-22 20:58 ` Rodrigo Vivi
0 siblings, 1 reply; 30+ messages in thread
From: Ceraolo Spurio, Daniele @ 2022-11-22 19:39 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx, dri-devel, Alan Previn
On 11/22/2022 11:01 AM, Rodrigo Vivi wrote:
> On Mon, Nov 21, 2022 at 03:16:14PM -0800, Daniele Ceraolo Spurio 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.
>>
>> The GSC is not reset as part of GT reset, so we only need to load it on
>> first boot and S3/S4 exit.
>>
>> Note that the GSC load takes a lot of time (up to a few hundred ms).
>> This patch loads it serially as part of driver init/resume, but, given
>> that GSC is only required for PM and content-protection features
>> (media C6, PXP, HDCP), we could move the load to a worker thread to unblock
>> non-CP userspace submissions earlier. This will be done as a follow up
>> step, because there are extra init steps required to actually make use of
>> the GSC (including a mei component) and it will be cleaner (and easier to
>> review) if we implement the async load once all the pieces we need for GSC
>> to work are in place. A TODO has been added to the code to mark this
>> intention.
>>
>> 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>
>> ---
>> drivers/gpu/drm/i915/Makefile | 1 +
>> drivers/gpu/drm/i915/gem/i915_gem_pm.c | 14 +-
>> drivers/gpu/drm/i915/gt/intel_engine.h | 2 +
>> drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 7 +
>> drivers/gpu/drm/i915/gt/intel_gt.c | 11 ++
>> drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 186 +++++++++++++++++++
>> drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 13 ++
>> drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 35 +++-
>> drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 7 +
>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 15 ++
>> drivers/gpu/drm/i915/gt/uc/intel_uc.h | 2 +
>> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 20 +-
>> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 1 +
>> 13 files changed, 307 insertions(+), 7 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 92d37cf71e16..1d45a6f451fa 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -206,6 +206,7 @@ i915-y += gt/uc/intel_uc.o \
>> gt/uc/intel_huc.o \
>> gt/uc/intel_huc_debugfs.o \
>> gt/uc/intel_huc_fw.o \
>> + gt/uc/intel_gsc_fw.o \
>> gt/uc/intel_gsc_uc.o
>>
>> # graphics system controller (GSC) support
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
>> index 0d812f4d787d..f77eb4009aba 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
>> @@ -232,10 +232,22 @@ void i915_gem_resume(struct drm_i915_private *i915)
>> * guarantee that the context image is complete. So let's just reset
>> * it and start again.
>> */
>> - for_each_gt(gt, i915, i)
>> + for_each_gt(gt, i915, i) {
>> if (intel_gt_resume(gt))
>> goto err_wedged;
>>
>> + /*
>> + * TODO: this is a long operation (up to ~200ms) and we don't
>> + * need to complete it before driver load/resume is done, so it
>> + * should be handled in a separate thread to unlock userspace
>> + * submission. However, there are a couple of other pieces that
>> + * are required for full GSC support that will complicate things
>> + * a bit, and it is easier to move everything to a worker at the
>> + * same time, so keep it here for now.
>> + */
>> + intel_uc_init_hw_late(>->uc);
>> + }
>> +
>> ret = lmem_restore(i915, I915_TTM_BACKUP_ALLOW_GPU);
>> GEM_WARN_ON(ret);
>>
>> 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/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
>> index b5ad9caa5537..a12e74a1fe5d 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
>> @@ -750,6 +750,17 @@ int intel_gt_init(struct intel_gt *gt)
>>
>> intel_pxp_init(>->pxp);
>>
>> + /*
>> + * TODO: this is a long operation (up to ~200ms) and we don't need
>> + * to complete it before driver load/resume is done, so it should
>> + * be handled in a separate thread to unlock userspace submission.
> shouldn't we already add an workqueue for this?
My main reason to not add it immediately was to avoid code trashing,
considering that GSC is still off by default and the workqueue
management would have to be reworked when the SW proxy comes along,
which will have to happen before we enable GSC. SW proxy requires a
workqueue to handle messages between GSC and CSME (the flow is triggered
by an interrupt, but it takes too long to process within the interrupt
handler) and my plan was to therefore have a single combined WQ to
handle all GSC-related operations, with HuC authentication also being
added later on. SW proxy also adds a new mei component (similar to the
PXP one) which is required for full GSC functionality, so I was also
planning to move all the GSC init to the component bind function.
If you still think it is better to add a WQ immediately and replace it
later with the full solution, I'll do that.
Daniele
>
>> + * However, there are a couple of other pieces that are required
>> + * for full GSC support that will complicate things a bit, and it
>> + * is easier to move everything to a worker at the same time, so
>> + * keep it here for now.
>> + */
>> + intel_uc_init_hw_late(>->uc);
>> +
>> goto out_fw;
>> err_gt:
>> __intel_gt_disable(gt);
>> 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..510fb47193ec
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
>> @@ -0,0 +1,186 @@
>> +// 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 GENMASK(3, 0)
>> +#define GSC_FW_CURRENT_STATE_RESET 0
>> +#define GSC_FW_INIT_COMPLETE_BIT BIT(9)
> let's prefer the REG_GENMASK AND REG_BIT as documented in i915_reg.h
ok
>
>> +
>> +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;
>> +}
>> +
>> +static bool gsc_init_done(struct intel_uncore *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_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 (gsc_init_done(gt->uncore)) {
>> + if (GEM_WARN_ON(!intel_uc_fw_is_loaded(gsc_fw)))
>> + intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
>> + return 0;
>> + }
>> +
>> + 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(>->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..d714e5e69a6d
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2022 Intel Corporation
>> + */
>> +
>> +#ifndef _INTEL_GSC_FW_H_
>> +#define _INTEL_GSC_FW_H_
>> +
>> +struct intel_gsc_uc;
>> +
>> +int intel_gsc_fw_upload(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..dd985a0f0613 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
>> @@ -45,17 +45,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(>->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(>->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 +95,9 @@ void intel_gsc_uc_fini(struct intel_gsc_uc *gsc)
>> if (!intel_uc_fw_is_loadable(&gsc->fw))
>> return;
>>
>> + 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);
>> }
>> 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..6fdb54243372 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
>> @@ -8,9 +8,16 @@
>>
>> #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 */
>> };
>>
>> void intel_gsc_uc_init_early(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 622935770aa1..b2c2512a3391 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"
>> @@ -593,6 +594,19 @@ static void __uc_fini_hw(struct intel_uc *uc)
>> __uc_sanitize(uc);
>> }
>>
>> +/*
>> + * Firmwares loaded via the GSC engine require the submission back-end to have
>> + * been initialized, so can only be loaded late in the probe/resume process.
>> + * TODO move to worker
>> + */
>> +static int __uc_init_hw_late(struct intel_uc *uc)
>> +{
>> + if (!intel_uc_uses_gsc_uc(uc))
>> + return 0;
>> +
>> + return intel_gsc_fw_upload(&uc->gsc);
>> +}
>> +
>> /**
>> * intel_uc_reset_prepare - Prepare for reset
>> * @uc: the intel_uc structure
>> @@ -751,5 +765,6 @@ static const struct intel_uc_ops uc_ops_on = {
>> .fini = __uc_fini,
>>
>> .init_hw = __uc_init_hw,
>> + .init_hw_late = __uc_init_hw_late,
>> .fini_hw = __uc_fini_hw,
>> };
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> index 5d0f1bcc381e..39993ba6c24e 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> @@ -23,6 +23,7 @@ struct intel_uc_ops {
>> int (*init)(struct intel_uc *uc);
>> void (*fini)(struct intel_uc *uc);
>> int (*init_hw)(struct intel_uc *uc);
>> + int (*init_hw_late)(struct intel_uc *uc);
>> void (*fini_hw)(struct intel_uc *uc);
>> };
>>
>> @@ -112,6 +113,7 @@ intel_uc_ops_function(cleanup_firmwares, fini_fw, void, );
>> intel_uc_ops_function(init, init, int, 0);
>> intel_uc_ops_function(fini, fini, void, );
>> intel_uc_ops_function(init_hw, init_hw, int, 0);
>> +intel_uc_ops_function(init_hw_late, init_hw_late, int, 0);
>> intel_uc_ops_function(fini_hw, fini_hw, void, );
>> #undef intel_uc_ops_function
>>
>> 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 3df52fd59edc..dcec115209de 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> @@ -836,6 +836,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
>> @@ -872,11 +886,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 5d2a8965a592..05760fb85fff 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> @@ -290,6 +290,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
>>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW
2022-11-22 9:03 ` Jani Nikula
@ 2022-11-22 19:42 ` Ceraolo Spurio, Daniele
2022-11-22 20:11 ` Jani Nikula
0 siblings, 1 reply; 30+ messages in thread
From: Ceraolo Spurio, Daniele @ 2022-11-22 19:42 UTC (permalink / raw)
To: Jani Nikula, intel-gfx; +Cc: dri-devel, Alan Previn
On 11/22/2022 1:03 AM, Jani Nikula wrote:
> On Mon, 21 Nov 2022, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> 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.
>>
>> 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>
>> ---
>> drivers/gpu/drm/i915/Makefile | 3 +-
>> 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 | 25 +++++++-
>> 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, 164 insertions(+), 6 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..92d37cf71e16 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -205,7 +205,8 @@ 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_gsc_uc.o
> Comment near the top of the file:
>
> # Please keep these build lists sorted!
My bad, dumb mistake.
>
>>
>> # 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 1d28286e6f06..622935770aa1 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"
> And thus intel_gsc_uc.h becomes another file that causes the entire
> driver to be rebuilt when modified.
>
> *sad trombone*
I just followed the same pattern as what is done for GuC and HuC files.
What's the recommendation here? Should I split out gsc_uc_types.h from
gsc_uc.h ?
Daniele
>> #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 0c80ba51a4bd..5b2e4503aee7 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,10 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>> int i;
>> bool found;
>>
>> + /* FW is not defined until all the support is in place */
>> + 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
>> @@ -374,6 +385,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;
>> @@ -385,6 +401,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 bc898ba5355d..5d2a8965a592 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
>>
>> /*
>> * The firmware build process will generate a version header file with major and
>> @@ -205,6 +206,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) \
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW
2022-11-22 19:42 ` Ceraolo Spurio, Daniele
@ 2022-11-22 20:11 ` Jani Nikula
0 siblings, 0 replies; 30+ messages in thread
From: Jani Nikula @ 2022-11-22 20:11 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: dri-devel, Alan Previn
On Tue, 22 Nov 2022, "Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com> wrote:
> On 11/22/2022 1:03 AM, Jani Nikula wrote:
>> On Mon, 21 Nov 2022, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote:
>>> 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"
>> And thus intel_gsc_uc.h becomes another file that causes the entire
>> driver to be rebuilt when modified.
>>
>> *sad trombone*
>
> I just followed the same pattern as what is done for GuC and HuC files.
> What's the recommendation here? Should I split out gsc_uc_types.h from
> gsc_uc.h ?
Sorry for not being clear, I'm not insisting you do anything at this
time.
But it is something that needs to be refactored eventually.
As an anecdotal data point: I just scripted all the #include
dependencies across all the files in the driver into a digraph and had
graphviz turn it into svg, and on my 80 cm wide 4k screen zoomed out as
far as Firefox can, it's still 15 screenfuls side by side. ;D
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded
2022-11-21 23:16 ` [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded Daniele Ceraolo Spurio
2022-11-22 0:17 ` Ceraolo Spurio, Daniele
@ 2022-11-22 20:46 ` Rodrigo Vivi
2022-11-22 22:50 ` Ceraolo Spurio, Daniele
2022-12-01 22:40 ` Teres Alexis, Alan Previn
2 siblings, 1 reply; 30+ messages in thread
From: Rodrigo Vivi @ 2022-11-22 20:46 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx, dri-devel, Alan Previn
On Mon, Nov 21, 2022 at 03:16:15PM -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-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.
Nothing major or blocking, but a few thoughts:
1. Should we document this in the code, at least in a comment in the
flr function?
2. Should we call this driver_initiated_flr, aiming to reduce even more
the ambiguity of it?
>
> 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.
3. should we try to implement this already in the gt_reset case as the
last resrouce before wedging the gt? So we can already test this flow
in the current platforms?
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 9 +++++
> drivers/gpu/drm/i915/i915_reg.h | 3 ++
> drivers/gpu/drm/i915/intel_uncore.c | 45 +++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_uncore.h | 13 +++++++
> 4 files changed, 70 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 510fb47193ec..5dad3c19c445 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> @@ -173,6 +173,15 @@ int intel_gsc_fw_upload(struct intel_gsc_uc *gsc)
> if (err)
> goto fail;
>
> + /*
> + * Once the GSC FW is loaded, the only way to kill it on driver unload
> + * is to do a driver FLR. Given this is a very disruptive action, 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.
> + */
> + intel_uncore_set_flr_on_fini(>->i915->uncore);
> +
> /* FW is not fully operational until we enable SW proxy */
> intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8e1892d14774..60e55245200b 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..c1befa33ff59 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -2703,6 +2703,48 @@ void intel_uncore_prune_engine_fw_domains(struct intel_uncore *uncore,
> }
> }
>
> +static void driver_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 +2758,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_flr(uncore);
> }
>
> /**
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index 5449146a0624..a9fa0b11e7e4 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(3)
>
> 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
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 5/6] drm/i915/gsc: Disable GSC engine and power well if FW is not selected
2022-11-21 23:16 ` [Intel-gfx] [PATCH 5/6] drm/i915/gsc: Disable GSC engine and power well if FW is not selected Daniele Ceraolo Spurio
@ 2022-11-22 20:52 ` Rodrigo Vivi
2022-11-22 22:58 ` Ceraolo Spurio, Daniele
0 siblings, 1 reply; 30+ messages in thread
From: Rodrigo Vivi @ 2022-11-22 20:52 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx, Jonathan Cavitt, dri-devel
On Mon, Nov 21, 2022 at 03:16:16PM -0800, Daniele Ceraolo Spurio wrote:
> 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>
> ---
> 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(>->uc)) {
> + drm_notice(>->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 c1befa33ff59..e63d957b59eb 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);
On a quick glace I was asking "why do you need this since it doesn't have the gsc0?
Then I remember that fw_domain got initialized and it will be skipped, right?
Then I though about at least have a comment here, but finally I got myself wondering
why we don't do this already in the if above, while we are cleaning the engine mask?
> }
>
> static void driver_flr(struct intel_uncore *uncore)
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 6/6] drm/i915/mtl: MTL has one GSC CS on the media GT
2022-11-21 23:16 ` [Intel-gfx] [PATCH 6/6] drm/i915/mtl: MTL has one GSC CS on the media GT Daniele Ceraolo Spurio
@ 2022-11-22 20:52 ` Rodrigo Vivi
0 siblings, 0 replies; 30+ messages in thread
From: Rodrigo Vivi @ 2022-11-22 20:52 UTC (permalink / raw)
To: Daniele Ceraolo Spurio; +Cc: intel-gfx, dri-devel
On Mon, Nov 21, 2022 at 03:16:17PM -0800, Daniele Ceraolo Spurio wrote:
> 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 6da9784fe4a2..46acbf390195 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -1124,7 +1124,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
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 3/6] drm/i915/gsc: GSC firmware loading
2022-11-22 19:39 ` Ceraolo Spurio, Daniele
@ 2022-11-22 20:58 ` Rodrigo Vivi
0 siblings, 0 replies; 30+ messages in thread
From: Rodrigo Vivi @ 2022-11-22 20:58 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele; +Cc: intel-gfx, Alan Previn, dri-devel
On Tue, Nov 22, 2022 at 11:39:31AM -0800, Ceraolo Spurio, Daniele wrote:
>
>
> On 11/22/2022 11:01 AM, Rodrigo Vivi wrote:
> > On Mon, Nov 21, 2022 at 03:16:14PM -0800, Daniele Ceraolo Spurio 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.
> > >
> > > The GSC is not reset as part of GT reset, so we only need to load it on
> > > first boot and S3/S4 exit.
> > >
> > > Note that the GSC load takes a lot of time (up to a few hundred ms).
> > > This patch loads it serially as part of driver init/resume, but, given
> > > that GSC is only required for PM and content-protection features
> > > (media C6, PXP, HDCP), we could move the load to a worker thread to unblock
> > > non-CP userspace submissions earlier. This will be done as a follow up
> > > step, because there are extra init steps required to actually make use of
> > > the GSC (including a mei component) and it will be cleaner (and easier to
> > > review) if we implement the async load once all the pieces we need for GSC
> > > to work are in place. A TODO has been added to the code to mark this
> > > intention.
> > >
> > > 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>
> > > ---
> > > drivers/gpu/drm/i915/Makefile | 1 +
> > > drivers/gpu/drm/i915/gem/i915_gem_pm.c | 14 +-
> > > drivers/gpu/drm/i915/gt/intel_engine.h | 2 +
> > > drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 7 +
> > > drivers/gpu/drm/i915/gt/intel_gt.c | 11 ++
> > > drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 186 +++++++++++++++++++
> > > drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 13 ++
> > > drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 35 +++-
> > > drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 7 +
> > > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 15 ++
> > > drivers/gpu/drm/i915/gt/uc/intel_uc.h | 2 +
> > > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 20 +-
> > > drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 1 +
> > > 13 files changed, 307 insertions(+), 7 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 92d37cf71e16..1d45a6f451fa 100644
> > > --- a/drivers/gpu/drm/i915/Makefile
> > > +++ b/drivers/gpu/drm/i915/Makefile
> > > @@ -206,6 +206,7 @@ i915-y += gt/uc/intel_uc.o \
> > > gt/uc/intel_huc.o \
> > > gt/uc/intel_huc_debugfs.o \
> > > gt/uc/intel_huc_fw.o \
> > > + gt/uc/intel_gsc_fw.o \
> > > gt/uc/intel_gsc_uc.o
> > > # graphics system controller (GSC) support
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pm.c b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> > > index 0d812f4d787d..f77eb4009aba 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pm.c
> > > @@ -232,10 +232,22 @@ void i915_gem_resume(struct drm_i915_private *i915)
> > > * guarantee that the context image is complete. So let's just reset
> > > * it and start again.
> > > */
> > > - for_each_gt(gt, i915, i)
> > > + for_each_gt(gt, i915, i) {
> > > if (intel_gt_resume(gt))
> > > goto err_wedged;
> > > + /*
> > > + * TODO: this is a long operation (up to ~200ms) and we don't
> > > + * need to complete it before driver load/resume is done, so it
> > > + * should be handled in a separate thread to unlock userspace
> > > + * submission. However, there are a couple of other pieces that
> > > + * are required for full GSC support that will complicate things
> > > + * a bit, and it is easier to move everything to a worker at the
> > > + * same time, so keep it here for now.
> > > + */
> > > + intel_uc_init_hw_late(>->uc);
> > > + }
> > > +
> > > ret = lmem_restore(i915, I915_TTM_BACKUP_ALLOW_GPU);
> > > GEM_WARN_ON(ret);
> > > 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/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> > > index b5ad9caa5537..a12e74a1fe5d 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > > @@ -750,6 +750,17 @@ int intel_gt_init(struct intel_gt *gt)
> > > intel_pxp_init(>->pxp);
> > > + /*
> > > + * TODO: this is a long operation (up to ~200ms) and we don't need
> > > + * to complete it before driver load/resume is done, so it should
> > > + * be handled in a separate thread to unlock userspace submission.
> > shouldn't we already add an workqueue for this?
>
> My main reason to not add it immediately was to avoid code trashing,
> considering that GSC is still off by default and the workqueue management
> would have to be reworked when the SW proxy comes along, which will have to
> happen before we enable GSC. SW proxy requires a workqueue to handle
> messages between GSC and CSME (the flow is triggered by an interrupt, but it
> takes too long to process within the interrupt handler) and my plan was to
> therefore have a single combined WQ to handle all GSC-related operations,
> with HuC authentication also being added later on. SW proxy also adds a new
> mei component (similar to the PXP one) which is required for full GSC
> functionality, so I was also planning to move all the GSC init to the
> component bind function.
>
> If you still think it is better to add a WQ immediately and replace it later
> with the full solution, I'll do that.
No no, no need to do this now as long as we are committed to do this in a wq
before enabling the gsc fw by default. I have not checked the bits and cmd
addresses closely, but overall the patch looks good. (besides the REG_ stuff)
>
> Daniele
>
> >
> > > + * However, there are a couple of other pieces that are required
> > > + * for full GSC support that will complicate things a bit, and it
> > > + * is easier to move everything to a worker at the same time, so
> > > + * keep it here for now.
> > > + */
> > > + intel_uc_init_hw_late(>->uc);
> > > +
> > > goto out_fw;
> > > err_gt:
> > > __intel_gt_disable(gt);
> > > 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..510fb47193ec
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> > > @@ -0,0 +1,186 @@
> > > +// 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 GENMASK(3, 0)
> > > +#define GSC_FW_CURRENT_STATE_RESET 0
> > > +#define GSC_FW_INIT_COMPLETE_BIT BIT(9)
> > let's prefer the REG_GENMASK AND REG_BIT as documented in i915_reg.h
>
> ok
>
> >
> > > +
> > > +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;
> > > +}
> > > +
> > > +static bool gsc_init_done(struct intel_uncore *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_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 (gsc_init_done(gt->uncore)) {
> > > + if (GEM_WARN_ON(!intel_uc_fw_is_loaded(gsc_fw)))
> > > + intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
> > > + return 0;
> > > + }
> > > +
> > > + 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(>->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..d714e5e69a6d
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> > > @@ -0,0 +1,13 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright © 2022 Intel Corporation
> > > + */
> > > +
> > > +#ifndef _INTEL_GSC_FW_H_
> > > +#define _INTEL_GSC_FW_H_
> > > +
> > > +struct intel_gsc_uc;
> > > +
> > > +int intel_gsc_fw_upload(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..dd985a0f0613 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> > > @@ -45,17 +45,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(>->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(>->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 +95,9 @@ void intel_gsc_uc_fini(struct intel_gsc_uc *gsc)
> > > if (!intel_uc_fw_is_loadable(&gsc->fw))
> > > return;
> > > + 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);
> > > }
> > > 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..6fdb54243372 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
> > > @@ -8,9 +8,16 @@
> > > #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 */
> > > };
> > > void intel_gsc_uc_init_early(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 622935770aa1..b2c2512a3391 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"
> > > @@ -593,6 +594,19 @@ static void __uc_fini_hw(struct intel_uc *uc)
> > > __uc_sanitize(uc);
> > > }
> > > +/*
> > > + * Firmwares loaded via the GSC engine require the submission back-end to have
> > > + * been initialized, so can only be loaded late in the probe/resume process.
> > > + * TODO move to worker
> > > + */
> > > +static int __uc_init_hw_late(struct intel_uc *uc)
> > > +{
> > > + if (!intel_uc_uses_gsc_uc(uc))
> > > + return 0;
> > > +
> > > + return intel_gsc_fw_upload(&uc->gsc);
> > > +}
> > > +
> > > /**
> > > * intel_uc_reset_prepare - Prepare for reset
> > > * @uc: the intel_uc structure
> > > @@ -751,5 +765,6 @@ static const struct intel_uc_ops uc_ops_on = {
> > > .fini = __uc_fini,
> > > .init_hw = __uc_init_hw,
> > > + .init_hw_late = __uc_init_hw_late,
> > > .fini_hw = __uc_fini_hw,
> > > };
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> > > index 5d0f1bcc381e..39993ba6c24e 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> > > @@ -23,6 +23,7 @@ struct intel_uc_ops {
> > > int (*init)(struct intel_uc *uc);
> > > void (*fini)(struct intel_uc *uc);
> > > int (*init_hw)(struct intel_uc *uc);
> > > + int (*init_hw_late)(struct intel_uc *uc);
> > > void (*fini_hw)(struct intel_uc *uc);
> > > };
> > > @@ -112,6 +113,7 @@ intel_uc_ops_function(cleanup_firmwares, fini_fw, void, );
> > > intel_uc_ops_function(init, init, int, 0);
> > > intel_uc_ops_function(fini, fini, void, );
> > > intel_uc_ops_function(init_hw, init_hw, int, 0);
> > > +intel_uc_ops_function(init_hw_late, init_hw_late, int, 0);
> > > intel_uc_ops_function(fini_hw, fini_hw, void, );
> > > #undef intel_uc_ops_function
> > > 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 3df52fd59edc..dcec115209de 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > > @@ -836,6 +836,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
> > > @@ -872,11 +886,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 5d2a8965a592..05760fb85fff 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> > > @@ -290,6 +290,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
> > >
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded
2022-11-22 20:46 ` Rodrigo Vivi
@ 2022-11-22 22:50 ` Ceraolo Spurio, Daniele
2022-11-23 18:32 ` Rodrigo Vivi
0 siblings, 1 reply; 30+ messages in thread
From: Ceraolo Spurio, Daniele @ 2022-11-22 22:50 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx, dri-devel, Alan Previn
On 11/22/2022 12:46 PM, Rodrigo Vivi wrote:
> On Mon, Nov 21, 2022 at 03:16:15PM -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-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.
> Nothing major or blocking, but a few thoughts:
>
> 1. Should we document this in the code, at least in a comment in the
> flr function?
Sure, I'll add it in
> 2. Should we call this driver_initiated_flr, aiming to reduce even more
> the ambiguity of it?
ok
>
>> 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.
> 3. should we try to implement this already in the gt_reset case as the
> last resrouce before wedging the gt? So we can already test this flow
> in the current platforms?
This would be nice to have, but very complicated to implement. The fact
that FLR kills everything on the system, including resetting display and
wiping LMEM, means that we would need a new recovery path to
re-initialize all components. There are also potential questions on how
to handle LMEM: do we try to migrate it to SMEM before triggering the
FLR (potentially via CPU memcpy if the GT is dead), or do we just let it
get wiped?
The reason why I wanted the FLR to be the very last thing before
releasing MMIO access was exactly to not have to care about the recovery
path ;)
Daniele
>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 9 +++++
>> drivers/gpu/drm/i915/i915_reg.h | 3 ++
>> drivers/gpu/drm/i915/intel_uncore.c | 45 +++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_uncore.h | 13 +++++++
>> 4 files changed, 70 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 510fb47193ec..5dad3c19c445 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
>> @@ -173,6 +173,15 @@ int intel_gsc_fw_upload(struct intel_gsc_uc *gsc)
>> if (err)
>> goto fail;
>>
>> + /*
>> + * Once the GSC FW is loaded, the only way to kill it on driver unload
>> + * is to do a driver FLR. Given this is a very disruptive action, 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.
>> + */
>> + intel_uncore_set_flr_on_fini(>->i915->uncore);
>> +
>> /* FW is not fully operational until we enable SW proxy */
>> intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 8e1892d14774..60e55245200b 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..c1befa33ff59 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -2703,6 +2703,48 @@ void intel_uncore_prune_engine_fw_domains(struct intel_uncore *uncore,
>> }
>> }
>>
>> +static void driver_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 +2758,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_flr(uncore);
>> }
>>
>> /**
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
>> index 5449146a0624..a9fa0b11e7e4 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(3)
>>
>> 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
>>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 5/6] drm/i915/gsc: Disable GSC engine and power well if FW is not selected
2022-11-22 20:52 ` Rodrigo Vivi
@ 2022-11-22 22:58 ` Ceraolo Spurio, Daniele
2022-11-23 18:34 ` Rodrigo Vivi
0 siblings, 1 reply; 30+ messages in thread
From: Ceraolo Spurio, Daniele @ 2022-11-22 22:58 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-gfx, Jonathan Cavitt, dri-devel
On 11/22/2022 12:52 PM, Rodrigo Vivi wrote:
> On Mon, Nov 21, 2022 at 03:16:16PM -0800, Daniele Ceraolo Spurio wrote:
>> 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>
>> ---
>> 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(>->uc)) {
>> + drm_notice(>->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 c1befa33ff59..e63d957b59eb 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);
> On a quick glace I was asking "why do you need this since it doesn't have the gsc0?
> Then I remember that fw_domain got initialized and it will be skipped, right?
> Then I though about at least have a comment here, but finally I got myself wondering
> why we don't do this already in the if above, while we are cleaning the engine mask?
I've followed the existing code flows that we have in place for fused
off VCS/VECS. Basically the existing code goes like this:
1) All FW domains for the platform are initialized
2) We read the fuses and adjust the engine mask accordingly
3) We go back and prune the FW domains that are not applicable anymore
due to the updated mask.
The idea is to have a single gt-level function doing all the mask
adjusting and an uncore-level one doing all the domain pruning. I'm not
against changing this approach, but in that case we should update the
behavior for VCS/VECS as well (which might be complicated, because
VCS/VECS engines share FW domains, so the pruning logic is ugly).
Daniele
>
>> }
>>
>> static void driver_flr(struct intel_uncore *uncore)
>> --
>> 2.37.3
>>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded
2022-11-22 22:50 ` Ceraolo Spurio, Daniele
@ 2022-11-23 18:32 ` Rodrigo Vivi
0 siblings, 0 replies; 30+ messages in thread
From: Rodrigo Vivi @ 2022-11-23 18:32 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele; +Cc: intel-gfx, Alan Previn, dri-devel
On Tue, Nov 22, 2022 at 02:50:17PM -0800, Ceraolo Spurio, Daniele wrote:
>
>
> On 11/22/2022 12:46 PM, Rodrigo Vivi wrote:
> > On Mon, Nov 21, 2022 at 03:16:15PM -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-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.
> > Nothing major or blocking, but a few thoughts:
> >
> > 1. Should we document this in the code, at least in a comment in the
> > flr function?
>
> Sure, I'll add it in
>
> > 2. Should we call this driver_initiated_flr, aiming to reduce even more
> > the ambiguity of it?
>
> ok
>
> >
> > > 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.
> > 3. should we try to implement this already in the gt_reset case as the
> > last resrouce before wedging the gt? So we can already test this flow
> > in the current platforms?
>
> This would be nice to have, but very complicated to implement. The fact that
> FLR kills everything on the system, including resetting display and wiping
> LMEM, means that we would need a new recovery path to re-initialize all
> components. There are also potential questions on how to handle LMEM: do we
> try to migrate it to SMEM before triggering the FLR (potentially via CPU
> memcpy if the GT is dead), or do we just let it get wiped?
>
> The reason why I wanted the FLR to be the very last thing before releasing
> MMIO access was exactly to not have to care about the recovery path ;)
it makes sense indeed.
>
> Daniele
>
> >
> > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > > ---
> > > drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 9 +++++
> > > drivers/gpu/drm/i915/i915_reg.h | 3 ++
> > > drivers/gpu/drm/i915/intel_uncore.c | 45 +++++++++++++++++++++++
> > > drivers/gpu/drm/i915/intel_uncore.h | 13 +++++++
> > > 4 files changed, 70 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 510fb47193ec..5dad3c19c445 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> > > @@ -173,6 +173,15 @@ int intel_gsc_fw_upload(struct intel_gsc_uc *gsc)
> > > if (err)
> > > goto fail;
> > > + /*
> > > + * Once the GSC FW is loaded, the only way to kill it on driver unload
> > > + * is to do a driver FLR. Given this is a very disruptive action, 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.
> > > + */
> > > + intel_uncore_set_flr_on_fini(>->i915->uncore);
> > > +
> > > /* FW is not fully operational until we enable SW proxy */
> > > intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED);
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 8e1892d14774..60e55245200b 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..c1befa33ff59 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -2703,6 +2703,48 @@ void intel_uncore_prune_engine_fw_domains(struct intel_uncore *uncore,
> > > }
> > > }
> > > +static void driver_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 +2758,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_flr(uncore);
> > > }
> > > /**
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> > > index 5449146a0624..a9fa0b11e7e4 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(3)
> > > 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
> > >
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 5/6] drm/i915/gsc: Disable GSC engine and power well if FW is not selected
2022-11-22 22:58 ` Ceraolo Spurio, Daniele
@ 2022-11-23 18:34 ` Rodrigo Vivi
0 siblings, 0 replies; 30+ messages in thread
From: Rodrigo Vivi @ 2022-11-23 18:34 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele; +Cc: intel-gfx, Jonathan Cavitt, dri-devel
On Tue, Nov 22, 2022 at 02:58:37PM -0800, Ceraolo Spurio, Daniele wrote:
>
>
> On 11/22/2022 12:52 PM, Rodrigo Vivi wrote:
> > On Mon, Nov 21, 2022 at 03:16:16PM -0800, Daniele Ceraolo Spurio wrote:
> > > 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>
> > > ---
> > > 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(>->uc)) {
> > > + drm_notice(>->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 c1befa33ff59..e63d957b59eb 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);
> > On a quick glace I was asking "why do you need this since it doesn't have the gsc0?
> > Then I remember that fw_domain got initialized and it will be skipped, right?
> > Then I though about at least have a comment here, but finally I got myself wondering
> > why we don't do this already in the if above, while we are cleaning the engine mask?
>
> I've followed the existing code flows that we have in place for fused off
> VCS/VECS. Basically the existing code goes like this:
>
> 1) All FW domains for the platform are initialized
> 2) We read the fuses and adjust the engine mask accordingly
> 3) We go back and prune the FW domains that are not applicable anymore due
> to the updated mask.
>
> The idea is to have a single gt-level function doing all the mask adjusting
> and an uncore-level one doing all the domain pruning. I'm not against
> changing this approach, but in that case we should update the behavior for
> VCS/VECS as well (which might be complicated, because VCS/VECS engines share
> FW domains, so the pruning logic is ugly).
okay, then let's move with this...
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> Daniele
>
> >
> > > }
> > > static void driver_flr(struct intel_uncore *uncore)
> > > --
> > > 2.37.3
> > >
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW
2022-11-21 23:16 ` [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW Daniele Ceraolo Spurio
2022-11-22 9:03 ` Jani Nikula
@ 2022-11-29 23:48 ` Teres Alexis, Alan Previn
2022-11-30 17:08 ` Ceraolo Spurio, Daniele
1 sibling, 1 reply; 30+ messages in thread
From: Teres Alexis, Alan Previn @ 2022-11-29 23:48 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: dri-devel
Besides the nit below, just would like to echo the same thing Nikula said about not including the type definition in the
main uc header (which i know can be a bit more work especially if we go with allocation of the structure at init time
and using a ptr in the uc structure).
That said,
Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
On Mon, 2022-11-21 at 15:16 -0800, Ceraolo Spurio, Daniele wrote:
> On MTL the GSC FW needs to be loaded on the media GT by the graphics
>
>
> @@ -246,6 +253,10 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
> int i;
> bool found;
>
> + /* FW is not defined until all the support is in place */
Nit: perhaps a little bit more explanation would be better here for folks reading thru the codes
> + if (uc_fw->type == INTEL_UC_FW_TYPE_GSC)
> + return;
> +
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW
2022-11-29 23:48 ` Teres Alexis, Alan Previn
@ 2022-11-30 17:08 ` Ceraolo Spurio, Daniele
0 siblings, 0 replies; 30+ messages in thread
From: Ceraolo Spurio, Daniele @ 2022-11-30 17:08 UTC (permalink / raw)
To: Teres Alexis, Alan Previn, intel-gfx; +Cc: dri-devel
On 11/29/2022 3:48 PM, Teres Alexis, Alan Previn wrote:
> Besides the nit below, just would like to echo the same thing Nikula said about not including the type definition in the
> main uc header (which i know can be a bit more work especially if we go with allocation of the structure at init time
> and using a ptr in the uc structure).
I had a stab at this and it is quite complicated due to the
inter-dependencies between the headers. Even if I split out the type to
its own header, I'd have to include it in the current one for the status
checker functions to work, so there would be no gain. The whole
infrastructure needs to be reworked so that the headers can be fully
decoupled. I have a few ideas on how to go at that and I'll try to send
out a few patches on the side to get that effort going.
> That said,
> Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>
> On Mon, 2022-11-21 at 15:16 -0800, Ceraolo Spurio, Daniele wrote:
>> On MTL the GSC FW needs to be loaded on the media GT by the graphics
>>
>>
>> @@ -246,6 +253,10 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
>> int i;
>> bool found;
>>
>> + /* FW is not defined until all the support is in place */
> Nit: perhaps a little bit more explanation would be better here for folks reading thru the codes
Not sure what extra explanation I can provide here. The reason we're not
defining the blob is because the support code is not fully there, there
is no need to go into details of what's actually missing as that will
evolve with time. I can however rephrase this if you think the current
sentence is not clear, would something like this work:
"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".
Daniele
>> + if (uc_fw->type == INTEL_UC_FW_TYPE_GSC)
>> + return;
>> +
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 3/6] drm/i915/gsc: GSC firmware loading
2022-11-21 23:16 ` [Intel-gfx] [PATCH 3/6] drm/i915/gsc: GSC firmware loading Daniele Ceraolo Spurio
2022-11-22 19:01 ` Rodrigo Vivi
@ 2022-12-01 22:00 ` Teres Alexis, Alan Previn
1 sibling, 0 replies; 30+ messages in thread
From: Teres Alexis, Alan Previn @ 2022-12-01 22:00 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: dri-devel
I only have one minor nits below. Rodrigo already captured other minor issues. Functionally, all LGTM so
Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
On Mon, 2022-11-21 at 15:16 -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.
>
> @@ -206,6 +206,7 @@ i915-y += gt/uc/intel_uc.o \
> gt/uc/intel_huc.o \
> gt/uc/intel_huc_debugfs.o \
> gt/uc/intel_huc_fw.o \
> + gt/uc/intel_gsc_fw.o \
> gt/uc/intel_gsc_uc.o
>
Nit: Perhaps while here, you fix above to be alphabetical - i think thats the practice?
>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded
2022-11-21 23:16 ` [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded Daniele Ceraolo Spurio
2022-11-22 0:17 ` Ceraolo Spurio, Daniele
2022-11-22 20:46 ` Rodrigo Vivi
@ 2022-12-01 22:40 ` Teres Alexis, Alan Previn
2022-12-01 22:52 ` Ceraolo Spurio, Daniele
2 siblings, 1 reply; 30+ messages in thread
From: Teres Alexis, Alan Previn @ 2022-12-01 22:40 UTC (permalink / raw)
To: Ceraolo Spurio, Daniele, intel-gfx; +Cc: dri-devel
Few nits - most of which are repeats from existing review comments.
I did have 1 feedback. Functionally, code logic is correct.
To speed things up, I'll provide a conditional R-b if you address the feedback below + fix the the BIT3->to-BIT4 uncore-
flags fix. Others are nits in my book:
(conditional) Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
On Mon, 2022-11-21 at 15:16 -0800, Ceraolo Spurio, Daniele 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-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.
Alan: [snip]
> > + /*
> + * Once the GSC FW is loaded, the only way to kill it on driver unload
> + * is to do a driver FLR. Given this is a very disruptive action, 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.
> + */
> + intel_uncore_set_flr_on_fini(>->i915->uncore);
Alan: Nit: Perhaps define what disruptive (i.e. the whole memory wiping impact) - aligns with what Rodrigo commented i
think?
Alan: Nit: Might be important for developers debugging issues to state (in comments) that the security FW has been
provided a dynamically allocated memory which is why it MUST be killed on unload (unlike prior Gen SOCs).
Alan: Feedback: I think intel_uncore_set_flr_on_fini should called before gsc_fw_load() (or after but still called if
loading failed with and error indicating the instruction was already sent such as the timeout error, before the bail).
This would be better to ensure a clean slate is set upon unload even if gsc firmware was attempted to get loaded.
Alan: [snip]
> + /*
> + * 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);
> +
Alan: Nit: with the current definition of MTL registers, nothing is wrong with above code but for the sake of code-
intent-readability, perhaps better to use intel_uncore_rmw_fw on above too.
Alan: [snip]
> @@ -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(3)
>
Alan: Fix: yeah - this needs to be 4 - u already caught that.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded
2022-12-01 22:40 ` Teres Alexis, Alan Previn
@ 2022-12-01 22:52 ` Ceraolo Spurio, Daniele
0 siblings, 0 replies; 30+ messages in thread
From: Ceraolo Spurio, Daniele @ 2022-12-01 22:52 UTC (permalink / raw)
To: Teres Alexis, Alan Previn, intel-gfx; +Cc: dri-devel
On 12/1/2022 2:40 PM, Teres Alexis, Alan Previn wrote:
> Few nits - most of which are repeats from existing review comments.
> I did have 1 feedback. Functionally, code logic is correct.
>
> To speed things up, I'll provide a conditional R-b if you address the feedback below + fix the the BIT3->to-BIT4 uncore-
> flags fix. Others are nits in my book:
> (conditional) Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>
>
> On Mon, 2022-11-21 at 15:16 -0800, Ceraolo Spurio, Daniele 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-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.
>
> Alan: [snip]
>
>
>>> + /*
>> + * Once the GSC FW is loaded, the only way to kill it on driver unload
>> + * is to do a driver FLR. Given this is a very disruptive action, 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.
>> + */
>> + intel_uncore_set_flr_on_fini(>->i915->uncore);
> Alan: Nit: Perhaps define what disruptive (i.e. the whole memory wiping impact) - aligns with what Rodrigo commented i
> think?
I'll add it in the FLR function and refer to that one
>
> Alan: Nit: Might be important for developers debugging issues to state (in comments) that the security FW has been
> provided a dynamically allocated memory which is why it MUST be killed on unload (unlike prior Gen SOCs).
>
> Alan: Feedback: I think intel_uncore_set_flr_on_fini should called before gsc_fw_load() (or after but still called if
> loading failed with and error indicating the instruction was already sent such as the timeout error, before the bail).
> This would be better to ensure a clean slate is set upon unload even if gsc firmware was attempted to get loaded.
Ok, I'll move it to before.
>
> Alan: [snip]
>
>
>> + /*
>> + * 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);
>> +
> Alan: Nit: with the current definition of MTL registers, nothing is wrong with above code but for the sake of code-
> intent-readability, perhaps better to use intel_uncore_rmw_fw on above too.
This can't be a rmw, this register has a bunch of bits that are write to
clear/take action, so we must write only the FLR bit.
Daniele
>
> Alan: [snip]
>
>> @@ -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(3)
>>
> Alan: Fix: yeah - this needs to be 4 - u already caught that.
>
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2022-12-01 22:53 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-21 23:16 [Intel-gfx] [PATCH 0/6] drm/i915: Add support for GSC FW loading Daniele Ceraolo Spurio
2022-11-21 23:16 ` [Intel-gfx] [PATCH 1/6] drm/i915/uc: Introduce GSC FW Daniele Ceraolo Spurio
2022-11-22 9:03 ` Jani Nikula
2022-11-22 19:42 ` Ceraolo Spurio, Daniele
2022-11-22 20:11 ` Jani Nikula
2022-11-29 23:48 ` Teres Alexis, Alan Previn
2022-11-30 17:08 ` Ceraolo Spurio, Daniele
2022-11-21 23:16 ` [Intel-gfx] [PATCH 2/6] drm/i915/gsc: Skip the version check when fetching the " Daniele Ceraolo Spurio
2022-11-22 18:53 ` Rodrigo Vivi
2022-11-21 23:16 ` [Intel-gfx] [PATCH 3/6] drm/i915/gsc: GSC firmware loading Daniele Ceraolo Spurio
2022-11-22 19:01 ` Rodrigo Vivi
2022-11-22 19:39 ` Ceraolo Spurio, Daniele
2022-11-22 20:58 ` Rodrigo Vivi
2022-12-01 22:00 ` Teres Alexis, Alan Previn
2022-11-21 23:16 ` [Intel-gfx] [PATCH 4/6] drm/i915/gsc: Do a driver-FLR on unload if GSC was loaded Daniele Ceraolo Spurio
2022-11-22 0:17 ` Ceraolo Spurio, Daniele
2022-11-22 20:46 ` Rodrigo Vivi
2022-11-22 22:50 ` Ceraolo Spurio, Daniele
2022-11-23 18:32 ` Rodrigo Vivi
2022-12-01 22:40 ` Teres Alexis, Alan Previn
2022-12-01 22:52 ` Ceraolo Spurio, Daniele
2022-11-21 23:16 ` [Intel-gfx] [PATCH 5/6] drm/i915/gsc: Disable GSC engine and power well if FW is not selected Daniele Ceraolo Spurio
2022-11-22 20:52 ` Rodrigo Vivi
2022-11-22 22:58 ` Ceraolo Spurio, Daniele
2022-11-23 18:34 ` Rodrigo Vivi
2022-11-21 23:16 ` [Intel-gfx] [PATCH 6/6] drm/i915/mtl: MTL has one GSC CS on the media GT Daniele Ceraolo Spurio
2022-11-22 20:52 ` Rodrigo Vivi
2022-11-21 23:42 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add support for GSC FW loading Patchwork
2022-11-21 23:42 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2022-11-22 0:08 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).