Re: [Intel-gfx] [PATCH] drm/i915/gsc: GSC firmware loading
On 12/7/2022 2:16 AM, Teres Alexis, Alan Previn wrote:
Diffed the patches and reviewed the changes -- i.e. the use of the worker for
the gsc fw loading cmd submission.
All looks good - just a few nits below.
Reviewed-by: Alan Previn
On Mon, 2022-12-05 at 21:15 -0800, Ceraolo Spurio, Daniele wrote:
GSC FW is loaded by submitting a dedicated command via the GSC engine.
The memory area used for loading the FW is then re-purposed as local
memory for the GSC itself, so we use a separate allocation instead of
using the one where we keep the firmware stored for reload.
Alan:[snip]
+int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc)
+{
+ struct intel_gt *gt = gsc_uc_to_gt(gsc);
+ struct intel_uc_fw *gsc_fw = &gsc->fw;
+ int err;
+
+ /* check current fw status */
+ if (intel_gsc_uc_fw_init_done(gsc)) {
+ if (GEM_WARN_ON(!intel_uc_fw_is_loaded(gsc_fw)))
+ intel_uc_fw_change_status(gsc_fw,
INTEL_UC_FIRMWARE_TRANSFERRED);
+ return -EEXIST;
+ }
+
Alan: Nit: I see you've put the -EEXIST back again - not sure if we need it.
I'm marking this as Nit simply because we
dont consumer the return value from where its being called - but its still
weird that we are returning an error in the
case where FW is already there so we skip loading and allow normal operational
flow (not error-ing out).
Alan: Nit: not sure if we should explain in the comments how we can already
find the gsc-fw pre-loaded (IIRC, it could
be a prior driver unload that didn't do the FLR?).
It should be impossible to get here with an already loaded FW, because
we only queue the worker if the FW is not already loaded. However, for
safety I wanted to add in a check to make sure we're covered in case
something weird happens. Note that there is a GEM_WARN_ON tied to
intel_uc_fw_is_loaded(); this is because if that function returns true
it means the driver knows that the FW is already loaded and therefore
the error is just that the worker got called one time too many, while if
that is false it means that we got out of sync with the HW state and
that's a serious bug we want to flag.
Regarding the -EEXIST, that's in preparation for a follow up patch that
adds more functions to the worker (SW proxy) that will have to be
skipped if the GSC is already loaded. As you said it doesn't make a
difference now so I thought to start directly with it instead of
returning zero now and switching later.
+ if (!intel_uc_fw_is_loadable(gsc_fw))
+ return -ENOEXEC;
+
+ /* FW blob is ok, so clean the status */
+ intel_uc_fw_sanitize(&gsc->fw);
+
+ if (!gsc_is_in_reset(gt->uncore))
+ return -EIO;
+
+ err = gsc_fw_load_prepare(gsc);
+ if (err)
+ goto fail;
+
+
Alan: [snip]
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
index 65cbf1ce9fa1..3692ba387834 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -7,8 +7,19 @@
#include "gt/intel_gt.h"
#include "intel_gsc_uc.h"
+#include "intel_gsc_fw.h"
Alan: alphabetical ordering? (not sure if this is a hard rule, for me its a
nit)
#include "i915_drv.h"
+static void gsc_work(struct work_struct *work)
Alan: Nit: would ne nicer to call it gsc_load_worker unless there maybe future
plans to expand this worker's scope.
There is, this same worker will handle sw proxy as well.
+{
+ struct intel_gsc_uc *gsc = container_of(work, typeof(*gsc), work);
+ struct intel_gt *gt = gsc_uc_to_gt(gsc);
+ intel_wakeref_t wakeref;
+
+ with_intel_runtime_pm(gt->uncore->rpm, wakeref)
+ intel_gsc_uc_fw_upload(gsc);
+}
+
Alan:[snip]
struct intel_gsc_uc {
/* Generic uC firmware management */
struct intel_uc_fw fw;
+
+ /* GSC-specific additions */
+ struct i915_vma *local; /* private memory for GSC usage */
+ struct intel_context *ce; /* for submission to GSC FW via GSC engine */
+
+ struct work_struct work; /* for delayed load */
Alan: nit: would be nicer to call it "load_worker" unless the future plan is to
expand the scope of what else that
worker could be used for.
same as above.
Daniele
Alan:[snip]
Re: [Intel-gfx] [PATCH] drm/i915/gsc: GSC firmware loading
Diffed the patches and reviewed the changes -- i.e. the use of the worker for
the gsc fw loading cmd submission.
All looks good - just a few nits below.
Reviewed-by: Alan Previn
On Mon, 2022-12-05 at 21:15 -0800, Ceraolo Spurio, Daniele wrote:
> GSC FW is loaded by submitting a dedicated command via the GSC engine.
> The memory area used for loading the FW is then re-purposed as local
> memory for the GSC itself, so we use a separate allocation instead of
> using the one where we keep the firmware stored for reload.
>
>
>
Alan:[snip]
> +int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc)
> +{
> + struct intel_gt *gt = gsc_uc_to_gt(gsc);
> + struct intel_uc_fw *gsc_fw = &gsc->fw;
> + int err;
> +
> + /* check current fw status */
> + if (intel_gsc_uc_fw_init_done(gsc)) {
> + if (GEM_WARN_ON(!intel_uc_fw_is_loaded(gsc_fw)))
> + intel_uc_fw_change_status(gsc_fw,
> INTEL_UC_FIRMWARE_TRANSFERRED);
> + return -EEXIST;
> + }
> +
Alan: Nit: I see you've put the -EEXIST back again - not sure if we need it.
I'm marking this as Nit simply because we
dont consumer the return value from where its being called - but its still
weird that we are returning an error in the
case where FW is already there so we skip loading and allow normal operational
flow (not error-ing out).
Alan: Nit: not sure if we should explain in the comments how we can already
find the gsc-fw pre-loaded (IIRC, it could
be a prior driver unload that didn't do the FLR?).
> + if (!intel_uc_fw_is_loadable(gsc_fw))
> + return -ENOEXEC;
> +
> + /* FW blob is ok, so clean the status */
> + intel_uc_fw_sanitize(&gsc->fw);
> +
> + if (!gsc_is_in_reset(gt->uncore))
> + return -EIO;
> +
> + err = gsc_fw_load_prepare(gsc);
> + if (err)
> + goto fail;
> +
> +
Alan: [snip]
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> index 65cbf1ce9fa1..3692ba387834 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
> @@ -7,8 +7,19 @@
>
> #include "gt/intel_gt.h"
> #include "intel_gsc_uc.h"
> +#include "intel_gsc_fw.h"
Alan: alphabetical ordering? (not sure if this is a hard rule, for me its a
nit)
> #include "i915_drv.h"
>
> +static void gsc_work(struct work_struct *work)
Alan: Nit: would ne nicer to call it gsc_load_worker unless there maybe future
plans to expand this worker's scope.
> +{
> + struct intel_gsc_uc *gsc = container_of(work, typeof(*gsc), work);
> + struct intel_gt *gt = gsc_uc_to_gt(gsc);
> + intel_wakeref_t wakeref;
> +
> + with_intel_runtime_pm(gt->uncore->rpm, wakeref)
> + intel_gsc_uc_fw_upload(gsc);
> +}
> +
Alan:[snip]
> struct intel_gsc_uc {
> /* Generic uC firmware management */
> struct intel_uc_fw fw;
> +
> + /* GSC-specific additions */
> + struct i915_vma *local; /* private memory for GSC usage */
> + struct intel_context *ce; /* for submission to GSC FW via GSC engine */
> +
> + struct work_struct work; /* for delayed load */
Alan: nit: would be nicer to call it "load_worker" unless the future plan is to
expand the scope of what else that
worker could be used for.
Alan:[snip]
[Intel-gfx] [PATCH] drm/i915/gsc: GSC firmware loading
GSC FW is loaded by submitting a dedicated command via the GSC engine.
The memory area used for loading the FW is then re-purposed as local
memory for the GSC itself, so we use a separate allocation instead of
using the one where we keep the firmware stored for reload.
The GSC is not reset as part of GT reset, so we only need to load it on
first boot and S3/S4 exit.
v2: use REG_* for register fields definitions (Rodrigo), move to WQ
immediately
v3: mark worker function as static
Bspec: 63347, 65346
Signed-off-by: Daniele Ceraolo Spurio
Cc: Alan Previn
Cc: John Harrison
Cc: Rodrigo Vivi
---
drivers/gpu/drm/i915/Makefile| 1 +
drivers/gpu/drm/i915/gt/intel_engine.h | 2 +
drivers/gpu/drm/i915/gt/intel_gpu_commands.h | 7 +
drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c| 187 +++
drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h| 15 ++
drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c| 69 ++-
drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h| 11 ++
drivers/gpu/drm/i915/gt/uc/intel_uc.c| 6 +
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 20 +-
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 1 +
10 files changed, 313 insertions(+), 6 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 69f6e9af1b56..dfa211451a1d 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -189,6 +189,7 @@ i915-y += \
# general-purpose microcontroller (GuC) support
i915-y += \
+ gt/uc/intel_gsc_fw.o \
gt/uc/intel_gsc_uc.o \
gt/uc/intel_guc.o \
gt/uc/intel_guc_ads.o \
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
b/drivers/gpu/drm/i915/gt/intel_engine.h
index cbc8b857d5f7..0e24af5efee9 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -172,6 +172,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int
reg, u32 value)
#define I915_GEM_HWS_MIGRATE (0x42 * sizeof(u32))
#define I915_GEM_HWS_PXP 0x60
#define I915_GEM_HWS_PXP_ADDR (I915_GEM_HWS_PXP * sizeof(u32))
+#define I915_GEM_HWS_GSC 0x62
+#define I915_GEM_HWS_GSC_ADDR (I915_GEM_HWS_GSC * sizeof(u32))
#define I915_GEM_HWS_SCRATCH 0x80
#define I915_HWS_CSB_BUF0_INDEX0x10
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_MASK0x1800
@@ -432,6 +433,12 @@
#define COLOR_BLT ((0x2<<29)|(0x40<<22))
#define SRC_COPY_BLT ((0x2<<29)|(0x43<<22))
+#define GSC_INSTR(opcode, data, flags) \
+ (__INSTR(INSTR_GSC_CLIENT) | (opcode) << 22 | (data) << 9 | (flags))
+
+#define GSC_FW_LOAD GSC_INSTR(1, 0, 2)
+#define HECI1_FW_LIMIT_VALID (1<<31)
+
/*
* Used to convert any address to canonical form.
* Starting from gen8, some commands (e.g. STATE_BASE_ADDRESS,
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
new file mode 100644
index ..f88069ab71ab
--- /dev/null
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include "gt/intel_engine_pm.h"
+#include "gt/intel_gpu_commands.h"
+#include "gt/intel_gt.h"
+#include "gt/intel_ring.h"
+#include "intel_gsc_fw.h"
+
+#define GSC_FW_STATUS_REG _MMIO(0x116C40)
+#define GSC_FW_CURRENT_STATE REG_GENMASK(3, 0)
+#define GSC_FW_CURRENT_STATE_RESET 0
+#define GSC_FW_INIT_COMPLETE_BIT REG_BIT(9)
+
+static bool gsc_is_in_reset(struct intel_uncore *uncore)
+{
+ u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
+
+ return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) ==
+ GSC_FW_CURRENT_STATE_RESET;
+}
+
+bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc)
+{
+ struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore;
+ u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG);
+
+ return fw_status & GSC_FW_INIT_COMPLETE_BIT;
+}
+
+static int emit_gsc_fw_load(struct i915_request *rq, struct intel_gsc_uc *gsc)
+{
+ u32 offset = i915_ggtt_offset(gsc->local);
+ u32 *cs;
+
+ cs = intel_ring_begin(rq, 4);
+ if (IS_ERR(cs))
+ return PTR_ERR(cs);
+
+ *cs++ = GSC_FW_LOAD;
+ *cs++ = lower_32_bits(offset);
+
