* [Intel-gfx] [PATCH 0/6] drm/i915: GSC FW support for MTL
@ 2023-05-05 16:04 Daniele Ceraolo Spurio
2023-05-05 16:04 ` [Intel-gfx] [PATCH 1/6] DO NOT REVIEW: drm/i915: HuC loading and authentication " Daniele Ceraolo Spurio
` (6 more replies)
0 siblings, 7 replies; 23+ messages in thread
From: Daniele Ceraolo Spurio @ 2023-05-05 16:04 UTC (permalink / raw)
To: intel-gfx; +Cc: Alan Previn, dri-devel
Last chunk of the required support for the GSC FW. This includes some
fixes to the GSC memory allocation, FW idefinition and version
management, plus a new debugfs for debug information.
Adding the FW definition will enable all the features that are dependent
on the GSC being loaded (Media C6, HuC loading, SW proxy, PXP, HDCP).
The HuC series [1] has been included, squashed in a single patch,
as one of the patches in this series depends on it, but it should be
reviewd in its own thread.
[1] https://patchwork.freedesktop.org/series/117080/
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Suraj Kandpal <suraj.kandpal@intel.com>
Daniele Ceraolo Spurio (6):
DO NOT REVIEW: drm/i915: HuC loading and authentication for MTL
drm/i915/uc/gsc: fixes and updates for GSC memory allocation
drm/i915/uc/gsc: extract release and security versions from the gsc
binary
drm/i915/uc/gsc: query the GSC FW for its compatibility version
drm/i915/uc/gsc: define gsc fw
drm/i915/uc/gsc: Add a gsc_info debugfs
drivers/gpu/drm/i915/Makefile | 3 +-
drivers/gpu/drm/i915/gt/intel_ggtt.c | 3 +
drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 277 +++++++++++++++---
drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 2 +
.../drm/i915/gt/uc/intel_gsc_meu_headers.h | 143 +++++++++
drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 154 +++++++++-
drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 6 +
.../gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c | 38 +++
.../gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h | 14 +
.../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c | 2 +-
.../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h | 1 +
drivers/gpu/drm/i915/gt/uc/intel_guc.c | 2 +-
drivers/gpu/drm/i915/gt/uc/intel_huc.c | 182 ++++++++----
drivers/gpu/drm/i915/gt/uc/intel_huc.h | 26 +-
drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 224 +++++++++++++-
drivers/gpu/drm/i915/gt/uc/intel_huc_fw.h | 6 +-
drivers/gpu/drm/i915/gt/uc/intel_huc_print.h | 21 ++
drivers/gpu/drm/i915/gt/uc/intel_uc.c | 10 +-
drivers/gpu/drm/i915/gt/uc/intel_uc.h | 2 +
drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c | 2 +
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 218 +++++++++-----
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 13 +-
drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h | 6 -
drivers/gpu/drm/i915/i915_getparam.c | 6 +-
drivers/gpu/drm/i915/i915_reg.h | 25 ++
.../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 14 +-
drivers/gpu/drm/i915/pxp/intel_pxp_huc.c | 2 +-
include/uapi/drm/i915_drm.h | 3 +-
28 files changed, 1191 insertions(+), 214 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h
create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c
create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h
create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_huc_print.h
--
2.40.0
^ permalink raw reply [flat|nested] 23+ messages in thread* [Intel-gfx] [PATCH 1/6] DO NOT REVIEW: drm/i915: HuC loading and authentication for MTL 2023-05-05 16:04 [Intel-gfx] [PATCH 0/6] drm/i915: GSC FW support for MTL Daniele Ceraolo Spurio @ 2023-05-05 16:04 ` Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] [PATCH 2/6] drm/i915/uc/gsc: fixes and updates for GSC memory allocation Daniele Ceraolo Spurio ` (5 subsequent siblings) 6 siblings, 0 replies; 23+ messages in thread From: Daniele Ceraolo Spurio @ 2023-05-05 16:04 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel This is a of the HuC support for MTL series [1]. It's been included because one of the new patches in this series depend on it, but it should be reviewd separately in its own thread. [1] https://patchwork.freedesktop.org/series/117080/ Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> --- drivers/gpu/drm/i915/gt/intel_ggtt.c | 3 + .../drm/i915/gt/uc/intel_gsc_meu_headers.h | 74 ++++++ drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 31 ++- .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c | 2 +- drivers/gpu/drm/i915/gt/uc/intel_guc.c | 2 +- drivers/gpu/drm/i915/gt/uc/intel_huc.c | 182 ++++++++++---- drivers/gpu/drm/i915/gt/uc/intel_huc.h | 26 +- drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 233 +++++++++++++++++- drivers/gpu/drm/i915/gt/uc/intel_huc_fw.h | 6 +- drivers/gpu/drm/i915/gt/uc/intel_huc_print.h | 21 ++ drivers/gpu/drm/i915/gt/uc/intel_uc.c | 10 +- drivers/gpu/drm/i915/gt/uc/intel_uc.h | 2 + drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 120 +++++---- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 9 +- drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h | 6 - drivers/gpu/drm/i915/i915_getparam.c | 6 +- drivers/gpu/drm/i915/i915_reg.h | 3 + .../drm/i915/pxp/intel_pxp_cmd_interface_43.h | 14 +- drivers/gpu/drm/i915/pxp/intel_pxp_huc.c | 2 +- include/uapi/drm/i915_drm.h | 3 +- 20 files changed, 621 insertions(+), 134 deletions(-) create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_huc_print.h diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index 20915edc8bd9..ab71ed11de79 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c @@ -1322,6 +1322,9 @@ void i915_ggtt_resume(struct i915_ggtt *ggtt) ggtt->vm.scratch_range(&ggtt->vm, ggtt->error_capture.start, ggtt->error_capture.size); + list_for_each_entry(gt, &ggtt->gt_list, ggtt_link) + intel_uc_resume_mappings(>->uc); + ggtt->invalidate(ggtt); if (flush) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h new file mode 100644 index 000000000000..d55a66202576 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h @@ -0,0 +1,74 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2023 Intel Corporation + */ + +#ifndef _INTEL_GSC_MEU_H_ +#define _INTEL_GSC_MEU_H_ + +#include <linux/types.h> + +/* Code partition directory (CPD) structures */ +struct intel_gsc_cpd_header_v2 { + u32 header_marker; +#define INTEL_GSC_CPD_HEADER_MARKER 0x44504324 + + u32 num_of_entries; + u8 header_version; + u8 entry_version; + u8 header_length; /* in bytes */ + u8 flags; + u32 partition_name; + u32 crc32; +} __packed; + +struct intel_gsc_cpd_entry { + u8 name[12]; + + /* + * Bits 0-24: offset from the beginning of the code partition + * Bit 25: huffman compressed + * Bits 26-31: reserved + */ + u32 offset; +#define INTEL_GSC_CPD_ENTRY_OFFSET_MASK GENMASK(24, 0) +#define INTEL_GSC_CPD_ENTRY_HUFFMAN_COMP BIT(25) + + /* + * Module/Item length, in bytes. For Huffman-compressed modules, this + * refers to the uncompressed size. For software-compressed modules, + * this refers to the compressed size. + */ + u32 length; + + u8 reserved[4]; +} __packed; + +struct intel_gsc_meu_version { + u16 major; + u16 minor; + u16 hotfix; + u16 build; +} __packed; + +struct intel_gsc_manifest_header { + u32 header_type; /* 0x4 for manifest type */ + u32 header_length; /* in dwords */ + u32 header_version; + u32 flags; + u32 vendor; + u32 date; + u32 size; /* In dwords, size of entire manifest (header + extensions) */ + u32 header_id; + u32 internal_data; + struct intel_gsc_meu_version fw_version; + u32 security_version; + struct intel_gsc_meu_version meu_kit_version; + u32 meu_manifest_version; + u8 general_data[4]; + u8 reserved3[56]; + u32 modulus_size; /* in dwords */ + u32 exponent_size; /* in dwords */ +} __packed; + +#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 fb0984f875f9..bbf21b93fc8d 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c @@ -29,13 +29,29 @@ static void gsc_work(struct work_struct *work) if (actions & GSC_ACTION_FW_LOAD) { ret = intel_gsc_uc_fw_upload(gsc); - if (ret == -EEXIST) /* skip proxy if not a new load */ - actions &= ~GSC_ACTION_FW_LOAD; - else if (ret) + if (!ret) + /* setup proxy on a new load */ + actions |= GSC_ACTION_SW_PROXY; + else if (ret != -EEXIST) goto out_put; + + /* + * The HuC auth can be done both before or after the proxy init; + * if done after, a proxy request will be issued and must be + * serviced before the authentication can complete. + * Since this worker also handles proxy requests, we can't + * perform an action that requires the proxy from within it and + * then stall waiting for it, because we'd be blocking the + * service path. Therefore, it is easier for us to load HuC + * first and do proxy later. The GSC will ack the HuC auth and + * then send the HuC proxy request as part of the proxy init + * flow. + */ + if (intel_uc_uses_huc(>->uc)) + intel_huc_auth(>->uc.huc, INTEL_HUC_AUTH_BY_GSC); } - if (actions & (GSC_ACTION_FW_LOAD | GSC_ACTION_SW_PROXY)) { + if (actions & GSC_ACTION_SW_PROXY) { if (!intel_gsc_uc_fw_init_done(gsc)) { gt_err(gt, "Proxy request received with GSC not loaded!\n"); goto out_put; @@ -90,7 +106,12 @@ void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc) { struct intel_gt *gt = gsc_uc_to_gt(gsc); - intel_uc_fw_init_early(&gsc->fw, INTEL_UC_FW_TYPE_GSC); + /* + * GSC FW needs to be copied to a dedicated memory allocations for + * loading (see gsc->local), so we don't need to GGTT map the FW image + * itself into GGTT. + */ + intel_uc_fw_init_early(&gsc->fw, INTEL_UC_FW_TYPE_GSC, false); INIT_WORK(&gsc->work, gsc_work); /* we can arrive here from i915_driver_early_probe for primary diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c index ea0da06e2f39..edcfa990239d 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.c @@ -98,7 +98,7 @@ void intel_gsc_uc_heci_cmd_emit_mtl_header(struct intel_gsc_mtl_header *header, u64 host_session_id) { host_session_id &= ~HOST_SESSION_MASK; - if (heci_client_id == HECI_MEADDRESS_PXP) + if (host_session_id && heci_client_id == HECI_MEADDRESS_PXP) host_session_id |= HOST_SESSION_PXP_SINGLE; header->validity_marker = GSC_HECI_VALIDITY_MARKER; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc.c index c9f20385f6a0..2eb891b270ae 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c @@ -164,7 +164,7 @@ void intel_guc_init_early(struct intel_guc *guc) struct intel_gt *gt = guc_to_gt(guc); struct drm_i915_private *i915 = gt->i915; - intel_uc_fw_init_early(&guc->fw, INTEL_UC_FW_TYPE_GUC); + intel_uc_fw_init_early(&guc->fw, INTEL_UC_FW_TYPE_GUC, true); intel_guc_ct_init_early(&guc->ct); intel_guc_log_init_early(&guc->log); intel_guc_submission_init_early(guc); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index aefdaa62da99..54411ac33f35 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -6,23 +6,15 @@ #include <linux/types.h> #include "gt/intel_gt.h" -#include "gt/intel_gt_print.h" #include "intel_guc_reg.h" #include "intel_huc.h" +#include "intel_huc_print.h" #include "i915_drv.h" +#include "i915_reg.h" #include <linux/device/bus.h> #include <linux/mei_aux.h> -#define huc_printk(_huc, _level, _fmt, ...) \ - gt_##_level(huc_to_gt(_huc), "HuC: " _fmt, ##__VA_ARGS__) -#define huc_err(_huc, _fmt, ...) huc_printk((_huc), err, _fmt, ##__VA_ARGS__) -#define huc_warn(_huc, _fmt, ...) huc_printk((_huc), warn, _fmt, ##__VA_ARGS__) -#define huc_notice(_huc, _fmt, ...) huc_printk((_huc), notice, _fmt, ##__VA_ARGS__) -#define huc_info(_huc, _fmt, ...) huc_printk((_huc), info, _fmt, ##__VA_ARGS__) -#define huc_dbg(_huc, _fmt, ...) huc_printk((_huc), dbg, _fmt, ##__VA_ARGS__) -#define huc_probe_error(_huc, _fmt, ...) huc_printk((_huc), probe_error, _fmt, ##__VA_ARGS__) - /** * DOC: HuC * @@ -115,7 +107,7 @@ static enum hrtimer_restart huc_delayed_load_timer_callback(struct hrtimer *hrti { struct intel_huc *huc = container_of(hrtimer, struct intel_huc, delayed_load.timer); - if (!intel_huc_is_authenticated(huc)) { + if (!intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GSC)) { if (huc->delayed_load.status == INTEL_HUC_WAITING_ON_GSC) huc_notice(huc, "timed out waiting for MEI GSC\n"); else if (huc->delayed_load.status == INTEL_HUC_WAITING_ON_PXP) @@ -133,7 +125,7 @@ static void huc_delayed_load_start(struct intel_huc *huc) { ktime_t delay; - GEM_BUG_ON(intel_huc_is_authenticated(huc)); + GEM_BUG_ON(intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GSC)); /* * On resume we don't have to wait for MEI-GSC to be re-probed, but we @@ -276,7 +268,7 @@ void intel_huc_init_early(struct intel_huc *huc) struct drm_i915_private *i915 = huc_to_gt(huc)->i915; struct intel_gt *gt = huc_to_gt(huc); - intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC); + intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC, true); /* * we always init the fence as already completed, even if HuC is not @@ -293,13 +285,23 @@ void intel_huc_init_early(struct intel_huc *huc) } if (GRAPHICS_VER(i915) >= 11) { - huc->status.reg = GEN11_HUC_KERNEL_LOAD_INFO; - huc->status.mask = HUC_LOAD_SUCCESSFUL; - huc->status.value = HUC_LOAD_SUCCESSFUL; + huc->status[INTEL_HUC_AUTH_BY_GUC].reg = GEN11_HUC_KERNEL_LOAD_INFO; + huc->status[INTEL_HUC_AUTH_BY_GUC].mask = HUC_LOAD_SUCCESSFUL; + huc->status[INTEL_HUC_AUTH_BY_GUC].value = HUC_LOAD_SUCCESSFUL; } else { - huc->status.reg = HUC_STATUS2; - huc->status.mask = HUC_FW_VERIFIED; - huc->status.value = HUC_FW_VERIFIED; + huc->status[INTEL_HUC_AUTH_BY_GUC].reg = HUC_STATUS2; + huc->status[INTEL_HUC_AUTH_BY_GUC].mask = HUC_FW_VERIFIED; + huc->status[INTEL_HUC_AUTH_BY_GUC].value = HUC_FW_VERIFIED; + } + + if (IS_DG2(i915)) { + huc->status[INTEL_HUC_AUTH_BY_GSC].reg = GEN11_HUC_KERNEL_LOAD_INFO; + huc->status[INTEL_HUC_AUTH_BY_GSC].mask = HUC_LOAD_SUCCESSFUL; + huc->status[INTEL_HUC_AUTH_BY_GSC].value = HUC_LOAD_SUCCESSFUL; + } else { + huc->status[INTEL_HUC_AUTH_BY_GSC].reg = HECI_FWSTS5(MTL_GSC_HECI1_BASE); + huc->status[INTEL_HUC_AUTH_BY_GSC].mask = HECI_FWSTS5_HUC_AUTH_DONE; + huc->status[INTEL_HUC_AUTH_BY_GSC].value = HECI_FWSTS5_HUC_AUTH_DONE; } } @@ -307,51 +309,72 @@ void intel_huc_init_early(struct intel_huc *huc) static int check_huc_loading_mode(struct intel_huc *huc) { struct intel_gt *gt = huc_to_gt(huc); - bool fw_needs_gsc = intel_huc_is_loaded_by_gsc(huc); - bool hw_uses_gsc = false; + bool fw_is_meu = huc->fw.is_meu_binary; /* * The fuse for HuC load via GSC is only valid on platforms that have * GuC deprivilege. */ if (HAS_GUC_DEPRIVILEGE(gt->i915)) - hw_uses_gsc = intel_uncore_read(gt->uncore, GUC_SHIM_CONTROL2) & - GSC_LOADS_HUC; + huc->loaded_via_gsc = intel_uncore_read(gt->uncore, GUC_SHIM_CONTROL2) & + GSC_LOADS_HUC; + + if (huc->loaded_via_gsc && !fw_is_meu) { + huc_err(huc, "HW requires a MEU blob, but we found a legacy one\n"); + return -ENOEXEC; + } - if (fw_needs_gsc != hw_uses_gsc) { - huc_err(huc, "mismatch between FW (%s) and HW (%s) load modes\n", - HUC_LOAD_MODE_STRING(fw_needs_gsc), HUC_LOAD_MODE_STRING(hw_uses_gsc)); + /* + * Newer meu blobs contain the old FW structure inside. If we found + * that, we can use it to load the legacy way. + */ + if (!huc->loaded_via_gsc && fw_is_meu && !huc->fw.dma_start_offset) { + huc_err(huc," HW in legacy mode, but we have an incompatible meu blob\n"); return -ENOEXEC; } - /* make sure we can access the GSC via the mei driver if we need it */ + /* make sure we can access the GSC if we need it */ if (!(IS_ENABLED(CONFIG_INTEL_MEI_PXP) && IS_ENABLED(CONFIG_INTEL_MEI_GSC)) && - fw_needs_gsc) { + !HAS_ENGINE(gt, GSC0) && huc->loaded_via_gsc) { huc_info(huc, "can't load due to missing MEI modules\n"); return -EIO; } - huc_dbg(huc, "loaded by GSC = %s\n", str_yes_no(fw_needs_gsc)); + huc_dbg(huc, "loaded by GSC = %s\n", str_yes_no(huc->loaded_via_gsc)); return 0; } int intel_huc_init(struct intel_huc *huc) { + struct intel_gt *gt = huc_to_gt(huc); int err; err = check_huc_loading_mode(huc); if (err) goto out; + if (HAS_ENGINE(gt, GSC0)) { + struct i915_vma *vma = intel_guc_allocate_vma(>->uc.guc, SZ_8K); + if (IS_ERR(vma)) { + huc_info(huc, "Failed to allocate heci pkt\n"); + goto out; + } + + huc->heci_pkt = vma; + } + err = intel_uc_fw_init(&huc->fw); if (err) - goto out; + goto out_pkt; intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_LOADABLE); return 0; +out_pkt: + if (huc->heci_pkt) + i915_vma_unpin_and_release(&huc->heci_pkt, 0); out: intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_INIT_FAIL); huc_info(huc, "initialization failed %pe\n", ERR_PTR(err)); @@ -366,6 +389,9 @@ void intel_huc_fini(struct intel_huc *huc) */ delayed_huc_load_fini(huc); + if (huc->heci_pkt) + i915_vma_unpin_and_release(&huc->heci_pkt, 0); + if (intel_uc_fw_is_loadable(&huc->fw)) intel_uc_fw_fini(&huc->fw); } @@ -383,34 +409,46 @@ void intel_huc_suspend(struct intel_huc *huc) delayed_huc_load_complete(huc); } -int intel_huc_wait_for_auth_complete(struct intel_huc *huc) +static const char *auth_mode_string(struct intel_huc *huc, + enum intel_huc_authentication_type type) +{ + bool partial = !huc->loaded_via_gsc && huc->fw.is_meu_binary && + type == INTEL_HUC_AUTH_BY_GUC; + + return partial ? "clear media" : "all workloads"; +} + +int intel_huc_wait_for_auth_complete(struct intel_huc *huc, + enum intel_huc_authentication_type type) { struct intel_gt *gt = huc_to_gt(huc); int ret; ret = __intel_wait_for_register(gt->uncore, - huc->status.reg, - huc->status.mask, - huc->status.value, + huc->status[type].reg, + huc->status[type].mask, + huc->status[type].value, 2, 50, NULL); /* mark the load process as complete even if the wait failed */ delayed_huc_load_complete(huc); if (ret) { - huc_err(huc, "firmware not verified %pe\n", ERR_PTR(ret)); + huc_err(huc, "firmware not verified for %s: %pe\n", + auth_mode_string(huc, type), ERR_PTR(ret)); intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_LOAD_FAIL); return ret; } intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_RUNNING); - huc_info(huc, "authenticated!\n"); + huc_info(huc, "authenticated for %s!\n", auth_mode_string(huc, type)); return 0; } /** * intel_huc_auth() - Authenticate HuC uCode * @huc: intel_huc structure + * @type: authentication type (via GuC or via GSC) * * Called after HuC and GuC firmware loading during intel_uc_init_hw(). * @@ -418,7 +456,7 @@ int intel_huc_wait_for_auth_complete(struct intel_huc *huc) * passing the offset of the RSA signature to intel_guc_auth_huc(). It then * waits for up to 50ms for firmware verification ACK. */ -int intel_huc_auth(struct intel_huc *huc) +int intel_huc_auth(struct intel_huc *huc, enum intel_huc_authentication_type type) { struct intel_gt *gt = huc_to_gt(huc); struct intel_guc *guc = >->uc.guc; @@ -427,44 +465,67 @@ int intel_huc_auth(struct intel_huc *huc) if (!intel_uc_fw_is_loaded(&huc->fw)) return -ENOEXEC; - /* GSC will do the auth */ + /* GSC will do the auth with the load */ if (intel_huc_is_loaded_by_gsc(huc)) return -ENODEV; + if (intel_huc_is_authenticated(huc, type)) + return -EEXIST; + ret = i915_inject_probe_error(gt->i915, -ENXIO); if (ret) goto fail; - GEM_BUG_ON(intel_uc_fw_is_running(&huc->fw)); - - ret = intel_guc_auth_huc(guc, intel_guc_ggtt_offset(guc, huc->fw.rsa_data)); - if (ret) { - huc_err(huc, "authentication by GuC failed %pe\n", ERR_PTR(ret)); - goto fail; + switch(type) { + case INTEL_HUC_AUTH_BY_GUC: + ret = intel_guc_auth_huc(guc, intel_guc_ggtt_offset(guc, huc->fw.rsa_data)); + break; + case INTEL_HUC_AUTH_BY_GSC: + ret = intel_huc_fw_auth_via_gsccs(huc); + break; + default: + MISSING_CASE(type); + ret = -EINVAL; } + if (ret) + goto fail; /* Check authentication status, it should be done by now */ - ret = intel_huc_wait_for_auth_complete(huc); + ret = intel_huc_wait_for_auth_complete(huc, type); if (ret) goto fail; return 0; fail: - huc_probe_error(huc, "authentication failed %pe\n", ERR_PTR(ret)); + huc_probe_error(huc, "%s authentication failed %pe\n", + auth_mode_string(huc, type), ERR_PTR(ret)); return ret; } -bool intel_huc_is_authenticated(struct intel_huc *huc) +bool intel_huc_is_authenticated(struct intel_huc *huc, + enum intel_huc_authentication_type type) { struct intel_gt *gt = huc_to_gt(huc); intel_wakeref_t wakeref; u32 status = 0; with_intel_runtime_pm(gt->uncore->rpm, wakeref) - status = intel_uncore_read(gt->uncore, huc->status.reg); + status = intel_uncore_read(gt->uncore, huc->status[type].reg); + + return (status & huc->status[type].mask) == huc->status[type].value; +} - return (status & huc->status.mask) == huc->status.value; +static bool huc_is_fully_authenticated(struct intel_huc *huc) +{ + struct intel_uc_fw *huc_fw = &huc->fw; + + if (!huc_fw->is_meu_binary) + return intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GUC); + else if (intel_huc_is_loaded_by_gsc(huc) || HAS_ENGINE(huc_to_gt(huc), GSC0)) + return intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GSC); + else + return false; } /** @@ -479,7 +540,9 @@ bool intel_huc_is_authenticated(struct intel_huc *huc) */ int intel_huc_check_status(struct intel_huc *huc) { - switch (__intel_uc_fw_status(&huc->fw)) { + struct intel_uc_fw *huc_fw = &huc->fw; + + switch (__intel_uc_fw_status(huc_fw)) { case INTEL_UC_FIRMWARE_NOT_SUPPORTED: return -ENODEV; case INTEL_UC_FIRMWARE_DISABLED: @@ -496,7 +559,17 @@ int intel_huc_check_status(struct intel_huc *huc) break; } - return intel_huc_is_authenticated(huc); + /* + * meu binaries loaded by GuC are first partially authenticated by GuC + * and then fully authenticated by GSC + */ + if (huc_is_fully_authenticated(huc)) + return 1; /* full auth */ + else if (huc_fw->is_meu_binary && !huc->loaded_via_gsc && + intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GUC)) + return 2; /* clear media only */ + else + return 0; } static bool huc_has_delayed_load(struct intel_huc *huc) @@ -510,7 +583,10 @@ void intel_huc_update_auth_status(struct intel_huc *huc) if (!intel_uc_fw_is_loadable(&huc->fw)) return; - if (intel_huc_is_authenticated(huc)) + if (!huc->fw.is_meu_binary) + return; + + if (huc_is_fully_authenticated(huc)) intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_RUNNING); else if (huc_has_delayed_load(huc)) @@ -543,5 +619,5 @@ void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p) with_intel_runtime_pm(gt->uncore->rpm, wakeref) drm_printf(p, "HuC status: 0x%08x\n", - intel_uncore_read(gt->uncore, huc->status.reg)); + intel_uncore_read(gt->uncore, huc->status[INTEL_HUC_AUTH_BY_GUC].reg)); } diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h b/drivers/gpu/drm/i915/gt/uc/intel_huc.h index db555b3c1f56..319d38c4ab51 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h @@ -15,6 +15,7 @@ #include <linux/hrtimer.h> struct bus_type; +struct i915_vma; enum intel_huc_delayed_load_status { INTEL_HUC_WAITING_ON_GSC = 0, @@ -22,6 +23,12 @@ enum intel_huc_delayed_load_status { INTEL_HUC_DELAYED_LOAD_ERROR, }; +enum intel_huc_authentication_type { + INTEL_HUC_AUTH_BY_GUC = 0, + INTEL_HUC_AUTH_BY_GSC, + INTEL_HUC_AUTH_MAX_MODES +}; + struct intel_huc { /* Generic uC firmware management */ struct intel_uc_fw fw; @@ -31,7 +38,7 @@ struct intel_huc { i915_reg_t reg; u32 mask; u32 value; - } status; + } status[INTEL_HUC_AUTH_MAX_MODES]; struct { struct i915_sw_fence fence; @@ -39,6 +46,11 @@ struct intel_huc { struct notifier_block nb; enum intel_huc_delayed_load_status status; } delayed_load; + + /* for load via GSCCS */ + struct i915_vma *heci_pkt; + + bool loaded_via_gsc; }; int intel_huc_sanitize(struct intel_huc *huc); @@ -46,11 +58,13 @@ void intel_huc_init_early(struct intel_huc *huc); int intel_huc_init(struct intel_huc *huc); void intel_huc_fini(struct intel_huc *huc); void intel_huc_suspend(struct intel_huc *huc); -int intel_huc_auth(struct intel_huc *huc); -int intel_huc_wait_for_auth_complete(struct intel_huc *huc); +int intel_huc_auth(struct intel_huc *huc, enum intel_huc_authentication_type type); +int intel_huc_wait_for_auth_complete(struct intel_huc *huc, + enum intel_huc_authentication_type type); +bool intel_huc_is_authenticated(struct intel_huc *huc, + enum intel_huc_authentication_type type); int intel_huc_check_status(struct intel_huc *huc); void intel_huc_update_auth_status(struct intel_huc *huc); -bool intel_huc_is_authenticated(struct intel_huc *huc); void intel_huc_register_gsc_notifier(struct intel_huc *huc, struct bus_type *bus); void intel_huc_unregister_gsc_notifier(struct intel_huc *huc, struct bus_type *bus); @@ -73,13 +87,13 @@ static inline bool intel_huc_is_used(struct intel_huc *huc) static inline bool intel_huc_is_loaded_by_gsc(const struct intel_huc *huc) { - return huc->fw.loaded_via_gsc; + return huc->loaded_via_gsc; } static inline bool intel_huc_wait_required(struct intel_huc *huc) { return intel_huc_is_used(huc) && intel_huc_is_loaded_by_gsc(huc) && - !intel_huc_is_authenticated(huc); + !intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GSC); } void intel_huc_load_status(struct intel_huc *huc, struct drm_printer *p); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c index 534b0aa43316..0a0bd5504057 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c @@ -5,10 +5,239 @@ #include "gt/intel_gsc.h" #include "gt/intel_gt.h" +#include "intel_gsc_meu_headers.h" +#include "intel_gsc_uc_heci_cmd_submit.h" #include "intel_huc.h" #include "intel_huc_fw.h" +#include "intel_huc_print.h" #include "i915_drv.h" #include "pxp/intel_pxp_huc.h" +#include "pxp/intel_pxp_cmd_interface_43.h" + +struct mtl_huc_auth_msg_in { + struct intel_gsc_mtl_header header; + struct pxp43_new_huc_auth_in huc_in; +} __packed; + +struct mtl_huc_auth_msg_out { + struct intel_gsc_mtl_header header; + struct pxp43_huc_auth_out huc_out; +} __packed; + +int intel_huc_fw_auth_via_gsccs(struct intel_huc *huc) +{ + struct intel_gt *gt = huc_to_gt(huc); + struct drm_i915_private *i915 = gt->i915; + struct drm_i915_gem_object *obj; + struct mtl_huc_auth_msg_in *msg_in; + struct mtl_huc_auth_msg_out *msg_out; + void *pkt_vaddr; + u64 pkt_offset; + int retry = 5; + int err = 0; + + if (!huc->heci_pkt) + return -ENODEV; + + obj = huc->heci_pkt->obj; + pkt_offset = i915_ggtt_offset(huc->heci_pkt); + + pkt_vaddr = i915_gem_object_pin_map_unlocked(obj, + i915_coherent_map_type(i915, obj, true)); + if (IS_ERR(pkt_vaddr)) + return PTR_ERR(pkt_vaddr); + + msg_in = pkt_vaddr; + msg_out = pkt_vaddr + SZ_4K; + + intel_gsc_uc_heci_cmd_emit_mtl_header(&msg_in->header, + HECI_MEADDRESS_PXP, + sizeof(*msg_in), 0); + + msg_in->huc_in.header.api_version = PXP_APIVER(4, 3); + msg_in->huc_in.header.command_id = PXP43_CMDID_NEW_HUC_AUTH; + msg_in->huc_in.header.status = 0; + msg_in->huc_in.header.buffer_len = sizeof(msg_in->huc_in) - + sizeof(msg_in->huc_in.header); + msg_in->huc_in.huc_base_address = huc->fw.dummy.start; + msg_in->huc_in.huc_size = huc->fw.obj->base.size; + + do { + err = intel_gsc_uc_heci_cmd_submit_packet(>->uc.gsc, + pkt_offset, sizeof(*msg_in), + pkt_offset + SZ_4K, SZ_4K); + if (err) { + huc_err(huc, "failed to submit GSC request to auth: %d\n", err); + goto out_unpin; + } + + if (msg_out->header.flags & GSC_OUTFLAG_MSG_PENDING) { + msg_in->header.gsc_message_handle = msg_out->header.gsc_message_handle; + err = -EBUSY; + msleep(50); + } + } while (--retry && err == -EBUSY); + + if (err) + goto out_unpin; + + if (msg_out->header.message_size != sizeof(*msg_out)) { + huc_err(huc, "invalid GSC reply length %u [expected %zu]\n", + msg_out->header.message_size, sizeof(*msg_out)); + err = -EPROTO; + goto out_unpin; + } + + /* + * The GSC will return PXP_STATUS_OP_NOT_PERMITTED if the HuC is already + * loaded. If the same error is ever returned with HuC not loaded we'll + * still catch it when we check the authentication bit later. + */ + if (msg_out->huc_out.header.status != PXP_STATUS_SUCCESS && + msg_out->huc_out.header.status != PXP_STATUS_OP_NOT_PERMITTED) { + huc_err(huc, "auth failed with GSC error = 0x%x\n", + msg_out->huc_out.header.status); + err = -EIO; + goto out_unpin; + } + +out_unpin: + i915_gem_object_unpin_map(obj); + return err; +} + +static void get_version_from_meu_manifest(struct intel_uc_fw_ver *ver, const void *data) +{ + const struct intel_gsc_manifest_header *manifest = data; + + ver->major = manifest->fw_version.major; + ver->minor = manifest->fw_version.minor; + ver->patch = manifest->fw_version.hotfix; +} + +static bool css_valid(const void *data, size_t size) +{ + const struct uc_css_header *css = data; + + if (unlikely(size < sizeof(struct uc_css_header))) + return false; + + if (css->module_type != 0x6) + return false; + + if (css->module_vendor != PCI_VENDOR_ID_INTEL) + return false; + + return true; +} + +static inline u32 entry_offset(const struct intel_gsc_cpd_entry *entry) +{ + return entry->offset & INTEL_GSC_CPD_ENTRY_OFFSET_MASK; +} + +int intel_huc_fw_get_binary_info(struct intel_uc_fw *huc_fw, const void *data, size_t size) +{ + struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw); + const struct intel_gsc_cpd_header_v2 *header = data; + const struct intel_gsc_cpd_entry *entry; + size_t min_size = sizeof(*header); + int i; + + if (!huc_fw->is_meu_binary) { + huc_err(huc, "Invalid FW type MEU parsing!\n"); + return -EINVAL; + } + + if (size < sizeof(*header)) { + huc_err(huc, "MEU FW too small! %zu < %zu\n", size, min_size); + return -ENODATA; + } + + /* + * The meu HuC binary starts with a directory header, followed by a + * series of entries. Each entry is identified by a name and points to + * a specific section of the binary containing the relevant data. + * The entries we're interested in are + * - "HUCP.man": points to the GSC manifest header for the HuC, which + * contains the version info. + * - "huc_fw": points to the legacy-style binary that can be used for + * load via the DMA. This entry only contains a valid CSS + * on binaries for platforms that support 2-step HuC load + * via dma and auth via GSC (like MTL). + * + * -------------------------------------------------- + * [ intel_gsc_cpd_header_v2 ] + * -------------------------------------------------- + * [ intel_gsc_cpd_entry[] ] + * [ entry1 ] + * [ ... ] + * [ entryX ] + * [ "HUCP.man" ] + * [ ... ] + * [ offset >----------------------------]------o + * [ ... ] | + * [ entryY ] | + * [ "huc_fw" ] | + * [ ... ] | + * [ offset >----------------------------]----------o + * -------------------------------------------------- | | + * | | + * -------------------------------------------------- | | + * [ intel_gsc_manifest_header ]<-----o | + * [ ... ] | + * [ intel_gsc_meu_version fw_version ] | + * [ ... ] | + * -------------------------------------------------- | + * | + * -------------------------------------------------- | + * [ data[] ]<---------o + * [ ... ] + * [ ... ] + * -------------------------------------------------- + */ + + if (header->header_marker != INTEL_GSC_CPD_HEADER_MARKER) { + huc_err(huc, "invalid marker for meu CPD header: 0x%08x!\n", + header->header_marker); + return -EINVAL; + } + + /* we only have binaries with header v2 and entry v1 for now */ + if (header->header_version != 2 || header->entry_version != 1) { + huc_err(huc, "invalid meu CPD header/entry version %u:%u!\n", + header->header_version, header->entry_version); + return -EINVAL; + } + + if (header->header_length < sizeof(struct intel_gsc_cpd_header_v2)) { + huc_err(huc, "invalid meu CPD header length %u!\n", + header->header_length); + return -EINVAL; + } + + min_size = header->header_length + sizeof(*entry) * header->num_of_entries; + if (size < min_size) { + huc_err(huc, "MEU FW too small! %zu < %zu\n", size, min_size); + return -ENODATA; + } + + entry = data + header->header_length; + + for (i = 0; i < header->num_of_entries; i++, entry++) { + if (strcmp(entry->name, "HUCP.man") == 0) + get_version_from_meu_manifest(&huc_fw->file_selected.ver, + data + entry_offset(entry)); + + if (strcmp(entry->name, "huc_fw") == 0) { + u32 offset = entry_offset(entry); + if (offset < size && css_valid(data + offset, size - offset)) + huc_fw->dma_start_offset = offset; + } + } + + return 0; +} int intel_huc_fw_load_and_auth_via_gsc(struct intel_huc *huc) { @@ -25,7 +254,7 @@ int intel_huc_fw_load_and_auth_via_gsc(struct intel_huc *huc) * component gets re-bound and this function called again. If so, just * mark the HuC as loaded. */ - if (intel_huc_is_authenticated(huc)) { + if (intel_huc_is_authenticated(huc, INTEL_HUC_AUTH_BY_GSC)) { intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_RUNNING); return 0; } @@ -38,7 +267,7 @@ int intel_huc_fw_load_and_auth_via_gsc(struct intel_huc *huc) intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_TRANSFERRED); - return intel_huc_wait_for_auth_complete(huc); + return intel_huc_wait_for_auth_complete(huc, INTEL_HUC_AUTH_BY_GSC); } /** diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.h index db42e238b45f..307ab45e6b09 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.h @@ -7,8 +7,12 @@ #define _INTEL_HUC_FW_H_ struct intel_huc; +struct intel_uc_fw; + +#include <linux/types.h> int intel_huc_fw_load_and_auth_via_gsc(struct intel_huc *huc); +int intel_huc_fw_auth_via_gsccs(struct intel_huc *huc); int intel_huc_fw_upload(struct intel_huc *huc); - +int intel_huc_fw_get_binary_info(struct intel_uc_fw *huc_fw, const void *data, size_t size); #endif diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_print.h b/drivers/gpu/drm/i915/gt/uc/intel_huc_print.h new file mode 100644 index 000000000000..915d310ee1df --- /dev/null +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_print.h @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2023 Intel Corporation + */ + +#ifndef __INTEL_HUC_PRINT__ +#define __INTEL_HUC_PRINT__ + +#include "gt/intel_gt.h" +#include "gt/intel_gt_print.h" + +#define huc_printk(_huc, _level, _fmt, ...) \ + gt_##_level(huc_to_gt(_huc), "HuC: " _fmt, ##__VA_ARGS__) +#define huc_err(_huc, _fmt, ...) huc_printk((_huc), err, _fmt, ##__VA_ARGS__) +#define huc_warn(_huc, _fmt, ...) huc_printk((_huc), warn, _fmt, ##__VA_ARGS__) +#define huc_notice(_huc, _fmt, ...) huc_printk((_huc), notice, _fmt, ##__VA_ARGS__) +#define huc_info(_huc, _fmt, ...) huc_printk((_huc), info, _fmt, ##__VA_ARGS__) +#define huc_dbg(_huc, _fmt, ...) huc_printk((_huc), dbg, _fmt, ##__VA_ARGS__) +#define huc_probe_error(_huc, _fmt, ...) huc_printk((_huc), probe_error, _fmt, ##__VA_ARGS__) + +#endif /* __INTEL_HUC_PRINT__ */ diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 996168312340..05ad56274007 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -535,7 +535,7 @@ static int __uc_init_hw(struct intel_uc *uc) if (intel_huc_is_loaded_by_gsc(huc)) intel_huc_update_auth_status(huc); else - intel_huc_auth(huc); + intel_huc_auth(huc, INTEL_HUC_AUTH_BY_GUC); if (intel_uc_uses_guc_submission(uc)) { ret = intel_guc_submission_enable(guc); @@ -697,6 +697,12 @@ void intel_uc_suspend(struct intel_uc *uc) } } +static void __uc_resume_mappings(struct intel_uc *uc) +{ + intel_uc_fw_resume_mapping(&uc->guc.fw); + intel_uc_fw_resume_mapping(&uc->huc.fw); +} + static int __uc_resume(struct intel_uc *uc, bool enable_communication) { struct intel_guc *guc = &uc->guc; @@ -764,4 +770,6 @@ static const struct intel_uc_ops uc_ops_on = { .init_hw = __uc_init_hw, .fini_hw = __uc_fini_hw, + + .resume_mappings = __uc_resume_mappings, }; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h index 5d0f1bcc381e..c2783e6e752b 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h @@ -24,6 +24,7 @@ struct intel_uc_ops { void (*fini)(struct intel_uc *uc); int (*init_hw)(struct intel_uc *uc); void (*fini_hw)(struct intel_uc *uc); + void (*resume_mappings)(struct intel_uc *uc); }; struct intel_uc { @@ -113,6 +114,7 @@ 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(fini_hw, fini_hw, void, ); +intel_uc_ops_function(resume_mappings, resume_mappings, void, ); #undef intel_uc_ops_function #endif 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 6b71b9febd74..796f54a62eef 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -102,6 +102,7 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw, fw_def(SKYLAKE, 0, guc_mmp(skl, 70, 1, 1)) #define INTEL_HUC_FIRMWARE_DEFS(fw_def, huc_raw, huc_mmp, huc_gsc) \ + fw_def(METEORLAKE, 0, huc_gsc(mtl)) \ fw_def(DG2, 0, huc_gsc(dg2)) \ fw_def(ALDERLAKE_P, 0, huc_raw(tgl)) \ fw_def(ALDERLAKE_P, 0, huc_mmp(tgl, 7, 9, 3)) \ @@ -180,7 +181,7 @@ struct __packed uc_fw_blob { u8 major; u8 minor; u8 patch; - bool loaded_via_gsc; + bool is_meu_binary; }; #define UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \ @@ -189,9 +190,9 @@ struct __packed uc_fw_blob { .patch = patch_, \ .path = path_, -#define UC_FW_BLOB_NEW(major_, minor_, patch_, gsc_, path_) \ +#define UC_FW_BLOB_NEW(major_, minor_, patch_, meu_, path_) \ { UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \ - .legacy = false, .loaded_via_gsc = gsc_ } + .legacy = false, .is_meu_binary = meu_ } #define UC_FW_BLOB_OLD(major_, minor_, patch_, path_) \ { UC_FW_BLOB_BASE(major_, minor_, patch_, path_) \ @@ -296,7 +297,7 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) uc_fw->file_wanted.path = blob->path; uc_fw->file_wanted.ver.major = blob->major; uc_fw->file_wanted.ver.minor = blob->minor; - uc_fw->loaded_via_gsc = blob->loaded_via_gsc; + uc_fw->is_meu_binary = blob->is_meu_binary; found = true; break; } @@ -422,12 +423,14 @@ static void __uc_fw_user_override(struct drm_i915_private *i915, struct intel_uc * intel_uc_fw_init_early - initialize the uC object and select the firmware * @uc_fw: uC firmware * @type: type of uC + * @needs_ggtt_mapping: whether the FW needs to be GGTT mapped for loading * * Initialize the state of our uC object and relevant tracking and select the * firmware to fetch and load. */ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw, - enum intel_uc_fw_type type) + enum intel_uc_fw_type type, + bool needs_ggtt_mapping) { struct drm_i915_private *i915 = ____uc_fw_to_gt(uc_fw, type)->i915; @@ -440,6 +443,7 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw, GEM_BUG_ON(uc_fw->file_selected.path); uc_fw->type = type; + uc_fw->needs_ggtt_mapping = needs_ggtt_mapping; if (HAS_GT_UC(i915)) { __uc_fw_auto_select(i915, uc_fw); @@ -489,33 +493,6 @@ static void __force_fw_fetch_failures(struct intel_uc_fw *uc_fw, int e) } } -static int check_gsc_manifest(struct intel_gt *gt, - const struct firmware *fw, - struct intel_uc_fw *uc_fw) -{ - u32 *dw = (u32 *)fw->data; - u32 version_hi, version_lo; - size_t min_size; - - /* Check the size of the blob before examining buffer contents */ - min_size = sizeof(u32) * (HUC_GSC_VERSION_LO_DW + 1); - if (unlikely(fw->size < min_size)) { - gt_warn(gt, "%s firmware %s: invalid size: %zu < %zu\n", - intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path, - fw->size, min_size); - return -ENODATA; - } - - version_hi = dw[HUC_GSC_VERSION_HI_DW]; - version_lo = dw[HUC_GSC_VERSION_LO_DW]; - - uc_fw->file_selected.ver.major = FIELD_GET(HUC_GSC_MAJOR_VER_HI_MASK, version_hi); - uc_fw->file_selected.ver.minor = FIELD_GET(HUC_GSC_MINOR_VER_HI_MASK, version_hi); - uc_fw->file_selected.ver.patch = FIELD_GET(HUC_GSC_PATCH_VER_LO_MASK, version_lo); - - return 0; -} - static void uc_unpack_css_version(struct intel_uc_fw_ver *ver, u32 css_value) { /* Get version numbers from the CSS header */ @@ -572,22 +549,22 @@ static void guc_read_css_info(struct intel_uc_fw *uc_fw, struct uc_css_header *c uc_fw->private_data_size = css->private_data_size; } -static int check_ccs_header(struct intel_gt *gt, - const struct firmware *fw, - struct intel_uc_fw *uc_fw) +static int __check_ccs_header(struct intel_gt *gt, + const void *fw_data, size_t fw_size, + struct intel_uc_fw *uc_fw) { struct uc_css_header *css; size_t size; /* Check the size of the blob before examining buffer contents */ - if (unlikely(fw->size < sizeof(struct uc_css_header))) { + if (unlikely(fw_size < sizeof(struct uc_css_header))) { gt_warn(gt, "%s firmware %s: invalid size: %zu < %zu\n", intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path, - fw->size, sizeof(struct uc_css_header)); + fw_size, sizeof(struct uc_css_header)); return -ENODATA; } - css = (struct uc_css_header *)fw->data; + css = (struct uc_css_header *)fw_data; /* Check integrity of size values inside CSS header */ size = (css->header_size_dw - css->key_size_dw - css->modulus_size_dw - @@ -595,7 +572,7 @@ static int check_ccs_header(struct intel_gt *gt, if (unlikely(size != sizeof(struct uc_css_header))) { gt_warn(gt, "%s firmware %s: unexpected header size: %zu != %zu\n", intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path, - fw->size, sizeof(struct uc_css_header)); + fw_size, sizeof(struct uc_css_header)); return -EPROTO; } @@ -607,10 +584,10 @@ static int check_ccs_header(struct intel_gt *gt, /* At least, it should have header, uCode and RSA. Size of all three. */ size = sizeof(struct uc_css_header) + uc_fw->ucode_size + uc_fw->rsa_size; - if (unlikely(fw->size < size)) { + if (unlikely(fw_size < size)) { gt_warn(gt, "%s firmware %s: invalid size: %zu < %zu\n", intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path, - fw->size, size); + fw_size, size); return -ENOEXEC; } @@ -631,6 +608,32 @@ static int check_ccs_header(struct intel_gt *gt, return 0; } +static int check_gsc_manifest(struct intel_gt *gt, + const struct firmware *fw, + struct intel_uc_fw *uc_fw) +{ + if (uc_fw->type != INTEL_UC_FW_TYPE_HUC) { + gt_err(gt, "trying to MEU-parse a non-HuC binary"); + return -EINVAL; + } + + intel_huc_fw_get_binary_info(uc_fw, fw->data, fw->size); + + if (uc_fw->dma_start_offset) { + u32 delta = uc_fw->dma_start_offset; + __check_ccs_header(gt, fw->data + delta, fw->size - delta, uc_fw); + } + + return 0; +} + +static int check_ccs_header(struct intel_gt *gt, + const struct firmware *fw, + struct intel_uc_fw *uc_fw) +{ + return __check_ccs_header(gt, fw->data, fw->size, uc_fw); +} + static bool is_ver_8bit(struct intel_uc_fw_ver *ver) { return ver->major < 0xFF && ver->minor < 0xFF && ver->patch < 0xFF; @@ -678,7 +681,7 @@ static int check_fw_header(struct intel_gt *gt, if (uc_fw->type == INTEL_UC_FW_TYPE_GSC) return 0; - if (uc_fw->loaded_via_gsc) + if (uc_fw->is_meu_binary) err = check_gsc_manifest(gt, fw, uc_fw); else err = check_ccs_header(gt, fw, uc_fw); @@ -699,7 +702,7 @@ static int try_firmware_load(struct intel_uc_fw *uc_fw, const struct firmware ** if (err) return err; - if ((*fw)->size > INTEL_UC_RSVD_GGTT_PER_FW) { + if (uc_fw->needs_ggtt_mapping && (*fw)->size > INTEL_UC_RSVD_GGTT_PER_FW) { gt_err(gt, "%s firmware %s: size (%zuKB) exceeds max supported size (%uKB)\n", intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path, (*fw)->size / SZ_1K, INTEL_UC_RSVD_GGTT_PER_FW / SZ_1K); @@ -880,6 +883,9 @@ static void uc_fw_bind_ggtt(struct intel_uc_fw *uc_fw) struct i915_vma_resource *dummy = &uc_fw->dummy; u32 pte_flags = 0; + if (!uc_fw->needs_ggtt_mapping) + return; + dummy->start = uc_fw_ggtt_offset(uc_fw); dummy->node_size = obj->base.size; dummy->bi.pages = obj->mm.pages; @@ -901,11 +907,13 @@ static void uc_fw_bind_ggtt(struct intel_uc_fw *uc_fw) static void uc_fw_unbind_ggtt(struct intel_uc_fw *uc_fw) { - struct drm_i915_gem_object *obj = uc_fw->obj; struct i915_ggtt *ggtt = __uc_fw_to_gt(uc_fw)->ggtt; - u64 start = uc_fw_ggtt_offset(uc_fw); + struct i915_vma_resource *dummy = &uc_fw->dummy; + + if (!dummy->node_size) + return; - ggtt->vm.clear_range(&ggtt->vm, start, obj->base.size); + ggtt->vm.clear_range(&ggtt->vm, dummy->start, dummy->node_size); } static int uc_fw_xfer(struct intel_uc_fw *uc_fw, u32 dst_offset, u32 dma_flags) @@ -922,7 +930,7 @@ static int uc_fw_xfer(struct intel_uc_fw *uc_fw, u32 dst_offset, u32 dma_flags) intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL); /* Set the source address for the uCode */ - offset = uc_fw_ggtt_offset(uc_fw); + offset = uc_fw->dummy.start + uc_fw->dma_start_offset; GEM_BUG_ON(upper_32_bits(offset) & 0xFFFF0000); intel_uncore_write_fw(uncore, DMA_ADDR_0_LOW, lower_32_bits(offset)); intel_uncore_write_fw(uncore, DMA_ADDR_0_HIGH, upper_32_bits(offset)); @@ -996,9 +1004,7 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw, u32 dst_offset, u32 dma_flags) return -ENOEXEC; /* Call custom loader */ - uc_fw_bind_ggtt(uc_fw); err = uc_fw_xfer(uc_fw, dst_offset, dma_flags); - uc_fw_unbind_ggtt(uc_fw); if (err) goto fail; @@ -1102,6 +1108,8 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw) goto out_unpin; } + uc_fw_bind_ggtt(uc_fw); + return 0; out_unpin: @@ -1112,6 +1120,7 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw) void intel_uc_fw_fini(struct intel_uc_fw *uc_fw) { + uc_fw_unbind_ggtt(uc_fw); uc_fw_rsa_data_destroy(uc_fw); if (i915_gem_object_has_pinned_pages(uc_fw->obj)) @@ -1120,6 +1129,17 @@ void intel_uc_fw_fini(struct intel_uc_fw *uc_fw) intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_AVAILABLE); } +void intel_uc_fw_resume_mapping(struct intel_uc_fw *uc_fw) +{ + if (!intel_uc_fw_is_available(uc_fw)) + return; + + if (!i915_gem_object_has_pinned_pages(uc_fw->obj)) + return; + + uc_fw_bind_ggtt(uc_fw); +} + /** * intel_uc_fw_cleanup_fetch - cleanup uC firmware * @uc_fw: uC firmware @@ -1149,7 +1169,7 @@ size_t intel_uc_fw_copy_rsa(struct intel_uc_fw *uc_fw, void *dst, u32 max_len) { struct intel_memory_region *mr = uc_fw->obj->mm.region; u32 size = min_t(u32, uc_fw->rsa_size, max_len); - u32 offset = sizeof(struct uc_css_header) + uc_fw->ucode_size; + u32 offset = uc_fw->dma_start_offset + sizeof(struct uc_css_header) + uc_fw->ucode_size; struct sgt_iter iter; size_t count = 0; int idx; 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 6ba00e6b3975..8f2306627332 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h @@ -105,6 +105,7 @@ struct intel_uc_fw { * threaded as it done during driver load (inherently single threaded) * or during a GT reset (mutex guarantees single threaded). */ + bool needs_ggtt_mapping; struct i915_vma_resource dummy; struct i915_vma *rsa_data; @@ -112,7 +113,9 @@ struct intel_uc_fw { u32 ucode_size; u32 private_data_size; - bool loaded_via_gsc; + u32 dma_start_offset; + + bool is_meu_binary; }; /* @@ -282,12 +285,14 @@ static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw) } void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw, - enum intel_uc_fw_type type); + enum intel_uc_fw_type type, + bool needs_ggtt_mapping); int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw); void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw); 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); +void intel_uc_fw_resume_mapping(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); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h index 646fa8aa6cf1..7fe405126249 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw_abi.h @@ -84,10 +84,4 @@ struct uc_css_header { } __packed; static_assert(sizeof(struct uc_css_header) == 128); -#define HUC_GSC_VERSION_HI_DW 44 -#define HUC_GSC_MAJOR_VER_HI_MASK (0xFF << 0) -#define HUC_GSC_MINOR_VER_HI_MASK (0xFF << 16) -#define HUC_GSC_VERSION_LO_DW 45 -#define HUC_GSC_PATCH_VER_LO_MASK (0xFF << 0) - #endif /* _INTEL_UC_FW_ABI_H */ diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i915/i915_getparam.c index 2238e096c957..7aa47550e4f2 100644 --- a/drivers/gpu/drm/i915/i915_getparam.c +++ b/drivers/gpu/drm/i915/i915_getparam.c @@ -98,7 +98,11 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data, value = sseu->min_eu_in_pool; break; case I915_PARAM_HUC_STATUS: - value = intel_huc_check_status(&to_gt(i915)->uc.huc); + /* On platform with a media GT, the HuC is on that GT */ + if (i915->media_gt) + value = intel_huc_check_status(&i915->media_gt->uc.huc); + else + value = intel_huc_check_status(&to_gt(i915)->uc.huc); if (value < 0) return value; break; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index f82b2b245b0a..94baadce29f6 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -941,6 +941,9 @@ #define HECI_H_GS1(base) _MMIO((base) + 0xc4c) #define HECI_H_GS1_ER_PREP REG_BIT(0) +#define HECI_FWSTS5(base) _MMIO(base + 0xc68) +#define HECI_FWSTS5_HUC_AUTH_DONE (1 << 19) + #define HSW_GTT_CACHE_EN _MMIO(0x4024) #define GTT_CACHE_EN_ALL 0xF0007FFF #define GEN7_WR_WATERMARK _MMIO(0x4028) diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h index ad67e3f49c20..ac7a3829dda6 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_cmd_interface_43.h @@ -11,15 +11,23 @@ /* PXP-Cmd-Op definitions */ #define PXP43_CMDID_START_HUC_AUTH 0x0000003A +#define PXP43_CMDID_NEW_HUC_AUTH 0x0000003F /* MTL+ */ -/* PXP-Input-Packet: HUC-Authentication */ +/* PXP-Input-Packet: HUC Load and Authentication */ struct pxp43_start_huc_auth_in { struct pxp_cmd_header header; __le64 huc_base_address; } __packed; -/* PXP-Output-Packet: HUC-Authentication */ -struct pxp43_start_huc_auth_out { +/* PXP-Input-Packet: HUC Auth-only */ +struct pxp43_new_huc_auth_in { + struct pxp_cmd_header header; + u64 huc_base_address; + u32 huc_size; +} __packed; + +/* PXP-Output-Packet: HUC Load and Authentication or Auth-only */ +struct pxp43_huc_auth_out { struct pxp_cmd_header header; } __packed; diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp_huc.c b/drivers/gpu/drm/i915/pxp/intel_pxp_huc.c index 23431c36b60b..5eedce916942 100644 --- a/drivers/gpu/drm/i915/pxp/intel_pxp_huc.c +++ b/drivers/gpu/drm/i915/pxp/intel_pxp_huc.c @@ -19,7 +19,7 @@ int intel_pxp_huc_load_and_auth(struct intel_pxp *pxp) struct intel_gt *gt; struct intel_huc *huc; struct pxp43_start_huc_auth_in huc_in = {0}; - struct pxp43_start_huc_auth_out huc_out = {0}; + struct pxp43_huc_auth_out huc_out = {0}; dma_addr_t huc_phys_addr; u8 client_id = 0; u8 fence_id = 0; diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index dba7c5a5b25e..43b8de42a94e 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -659,7 +659,8 @@ typedef struct drm_i915_irq_wait { * If the IOCTL is successful, the returned parameter will be set to one of the * following values: * * 0 if HuC firmware load is not complete, - * * 1 if HuC firmware is authenticated and running. + * * 1 if HuC firmware is loaded and fully authenticated, + * * 2 if HuC firmware is loaded and authenticated for clear media only */ #define I915_PARAM_HUC_STATUS 42 -- 2.40.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [Intel-gfx] [PATCH 2/6] drm/i915/uc/gsc: fixes and updates for GSC memory allocation 2023-05-05 16:04 [Intel-gfx] [PATCH 0/6] drm/i915: GSC FW support for MTL Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] [PATCH 1/6] DO NOT REVIEW: drm/i915: HuC loading and authentication " Daniele Ceraolo Spurio @ 2023-05-05 16:04 ` Daniele Ceraolo Spurio 2023-05-23 0:13 ` Teres Alexis, Alan Previn 2023-05-05 16:04 ` [Intel-gfx] [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary Daniele Ceraolo Spurio ` (4 subsequent siblings) 6 siblings, 1 reply; 23+ messages in thread From: Daniele Ceraolo Spurio @ 2023-05-05 16:04 UTC (permalink / raw) To: intel-gfx; +Cc: Alan Previn, dri-devel A few fixes/updates are required around the GSC memory allocation and it is easier to do them all at the same time. The changes are as follows: 1 - Switch the memory allocation to stolen memory. We need to avoid accesses from GSC FW to normal memory after the suspend function has completed and to do so we can either switch to using stolen or make sure the GSC is gone to sleep before the end of the suspend function. Given that the GSC waits for a bit before going idle even if there are no pending operations, it is easier and quicker to just use stolen memory. 2 - Reduce the GSC allocation size to 4MBs, which is the POR requirement. The 8MBs were needed only for early FW and I had misunderstood that as being the expected POR size when I sent the original patch. 3 - Perma-map the GSC allocation. This isn't required immediately, but it will be needed later to be able to quickly extract the GSC logs, which are inside the allocation. Since the mapping code needs to be rewritten due to switching to stolen, it makes sense to do the switch immediately to avoid having to change it again later. Note that the explicit setting of CACHE_NONE for Wa_22016122933 has been dropped because that's the default setting for stolen memory on !LLC platforms. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Alan Previn <alan.previn.teres.alexis@intel.com> Cc: John Harrison <John.C.Harrison@Intel.com> Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 29 ++------- drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 77 ++++++++++++++++++++--- drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 1 + 3 files changed, 75 insertions(+), 32 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c index f46eb17a7a98..4814901fac9e 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c @@ -3,6 +3,7 @@ * Copyright © 2022 Intel Corporation */ +#include "gem/i915_gem_lmem.h" #include "gt/intel_engine_pm.h" #include "gt/intel_gpu_commands.h" #include "gt/intel_gt.h" @@ -109,38 +110,21 @@ 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; + void *src; if (!gsc->local) return -ENODEV; - obj = gsc->local->obj; - - if (obj->base.size < gsc->fw.size) + if (gsc->local->size < gsc->fw.size) return -ENOSPC; - /* - * Wa_22016122933: For MTL the shared memory needs to be mapped - * as WC on CPU side and UC (PAT index 2) on GPU side - */ - if (IS_METEORLAKE(i915)) - i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE); - - 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); + if (IS_ERR(src)) return PTR_ERR(src); - } - memset(dst, 0, obj->base.size); - memcpy(dst, src, gsc->fw.size); + memset_io(gsc->local_vaddr, 0, gsc->local->size); + memcpy_toio(gsc->local_vaddr, src, gsc->fw.size); /* * Wa_22016122933: Making sure the data in dst is @@ -149,7 +133,6 @@ static int gsc_fw_load_prepare(struct intel_gsc_uc *gsc) intel_guc_write_barrier(>->uc.guc); i915_gem_object_unpin_map(gsc->fw.obj); - i915_gem_object_unpin_map(obj); return 0; } 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 bbf21b93fc8d..2ae693b01b49 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c @@ -130,26 +130,85 @@ void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc) } } +static int gsc_allocate_and_map_vma(struct intel_gsc_uc *gsc, u32 size) +{ + struct intel_gt *gt = gsc_uc_to_gt(gsc); + struct drm_i915_gem_object *obj; + struct i915_vma *vma; + void __iomem *vaddr; + int ret = 0; + + /* + * The GSC FW doesn't immediately suspend after becoming idle, so there + * is a chance that it could still be awake after we successfully + * return from the pci suspend function, even if there are no pending + * operations. + * The FW might therefore try to access memory for its suspend operation + * after the kernel has completed the HW suspend flow; this can cause + * issues if the FW is mapped in normal RAM memory, as some of the + * involved HW units might've already lost power. + * The driver must therefore avoid this situation and the recommended + * way to do so is to use stolen memory for the GSC memory allocation, + * because stolen memory takes a different path in HW and it is + * guaranteed to always work as long as the GPU itself is awake (which + * it must be if the GSC is awake). + */ + obj = i915_gem_object_create_stolen(gt->i915, size); + if (IS_ERR(obj)) + return PTR_ERR(obj); + + vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0); + if (IS_ERR(vma)) { + ret = PTR_ERR(vma); + goto err; + } + + vaddr = i915_vma_pin_iomap(vma); + i915_vma_unpin(vma); + if (IS_ERR(vaddr)) { + ret = PTR_ERR(vaddr); + goto err; + } + + i915_vma_make_unshrinkable(vma); + + gsc->local = vma; + gsc->local_vaddr = vaddr; + + return 0; + +err: + i915_gem_object_put(obj); + return ret; +} + +static void gsc_unmap_and_free_vma(struct intel_gsc_uc *gsc) +{ + struct i915_vma *vma = fetch_and_zero(&gsc->local); + + if (!vma) + return; + + gsc->local_vaddr = NULL; + i915_vma_unpin_iomap(vma); + i915_gem_object_put(vma->obj); +} + int intel_gsc_uc_init(struct intel_gsc_uc *gsc) { static struct lock_class_key gsc_lock; struct intel_gt *gt = gsc_uc_to_gt(gsc); 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); + err = gsc_allocate_and_map_vma(gsc, SZ_4M); + if (err) goto out_fw; - } - - gsc->local = vma; ce = intel_engine_create_pinned_context(engine, engine->gt->vm, SZ_4K, I915_GEM_HWS_GSC_ADDR, @@ -170,7 +229,7 @@ int intel_gsc_uc_init(struct intel_gsc_uc *gsc) return 0; out_vma: - i915_vma_unpin_and_release(&gsc->local, 0); + gsc_unmap_and_free_vma(gsc); out_fw: intel_uc_fw_fini(&gsc->fw); out: @@ -194,7 +253,7 @@ void intel_gsc_uc_fini(struct intel_gsc_uc *gsc) if (gsc->ce) intel_engine_destroy_pinned_context(fetch_and_zero(&gsc->ce)); - i915_vma_unpin_and_release(&gsc->local, 0); + gsc_unmap_and_free_vma(gsc); 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 a2a0813b8a76..c01286dddbdb 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h @@ -18,6 +18,7 @@ struct intel_gsc_uc { /* GSC-specific additions */ struct i915_vma *local; /* private memory for GSC usage */ + void __iomem *local_vaddr; /* pointer to access the private memory */ struct intel_context *ce; /* for submission to GSC FW via GSC engine */ /* for delayed load and proxy handling */ -- 2.40.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 2/6] drm/i915/uc/gsc: fixes and updates for GSC memory allocation 2023-05-05 16:04 ` [Intel-gfx] [PATCH 2/6] drm/i915/uc/gsc: fixes and updates for GSC memory allocation Daniele Ceraolo Spurio @ 2023-05-23 0:13 ` Teres Alexis, Alan Previn 2023-05-23 15:21 ` Ceraolo Spurio, Daniele 0 siblings, 1 reply; 23+ messages in thread From: Teres Alexis, Alan Previn @ 2023-05-23 0:13 UTC (permalink / raw) To: Ceraolo Spurio, Daniele, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: > A few fixes/updates are required around the GSC memory allocation and it > is easier to do them all at the same time. The changes are as follows: alan:snip > @@ -109,38 +110,21 @@ 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; > + void *src; alan:snip > > - memset(dst, 0, obj->base.size); > - memcpy(dst, src, gsc->fw.size); > + memset_io(gsc->local_vaddr, 0, gsc->local->size); > + memcpy_toio(gsc->local_vaddr, src, gsc->fw.size); alan: i wonder if it there is benefit to do the memcpy_toio first and then do the memset_io but only for the balance of area from offset 'gsc->fw.size' for (gsc->local->size - gsc->fw.size) bytes. alan:snip > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > @@ -130,26 +130,85 @@ void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc) > } > } > > +static int gsc_allocate_and_map_vma(struct intel_gsc_uc *gsc, u32 size) alan:snip > + obj = i915_gem_object_create_stolen(gt->i915, size); > + if (IS_ERR(obj)) > + return PTR_ERR(obj); > + > + vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0); alan: should we be passing in the PIN_MAPPABLE flag into the last param? alan:snip > 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 a2a0813b8a76..c01286dddbdb 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h > @@ -18,6 +18,7 @@ struct intel_gsc_uc { > > /* GSC-specific additions */ > struct i915_vma *local; /* private memory for GSC usage */ > + void __iomem *local_vaddr; /* pointer to access the private memory */ alan:nit: relooking at the these variable names that originate from last year's patch you worked on introducing gsc_uc, i am wondering now if we should rename "local" to "privmem" and local_vaddr becomes privmem_vaddr. (no significant reason other than improving readibility of the code) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 2/6] drm/i915/uc/gsc: fixes and updates for GSC memory allocation 2023-05-23 0:13 ` Teres Alexis, Alan Previn @ 2023-05-23 15:21 ` Ceraolo Spurio, Daniele 2023-06-06 0:00 ` Teres Alexis, Alan Previn 0 siblings, 1 reply; 23+ messages in thread From: Ceraolo Spurio, Daniele @ 2023-05-23 15:21 UTC (permalink / raw) To: Teres Alexis, Alan Previn, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On 5/22/2023 5:13 PM, Teres Alexis, Alan Previn wrote: > On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: >> A few fixes/updates are required around the GSC memory allocation and it >> is easier to do them all at the same time. The changes are as follows: > > alan:snip > >> @@ -109,38 +110,21 @@ 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; >> + void *src; > > alan:snip > >> >> - memset(dst, 0, obj->base.size); >> - memcpy(dst, src, gsc->fw.size); >> + memset_io(gsc->local_vaddr, 0, gsc->local->size); >> + memcpy_toio(gsc->local_vaddr, src, gsc->fw.size); > alan: i wonder if it there is benefit to do the memcpy_toio first > and then do the memset_io but only for the balance of area from > offset 'gsc->fw.size' for (gsc->local->size - gsc->fw.size) bytes. I usually always memset first to start from a clean slate, but you're right I could flip this. > > alan:snip > >> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c >> @@ -130,26 +130,85 @@ void intel_gsc_uc_init_early(struct intel_gsc_uc *gsc) >> } >> } >> >> +static int gsc_allocate_and_map_vma(struct intel_gsc_uc *gsc, u32 size) > alan:snip > >> + obj = i915_gem_object_create_stolen(gt->i915, size); >> + if (IS_ERR(obj)) >> + return PTR_ERR(obj); >> + >> + vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0); > alan: should we be passing in the PIN_MAPPABLE flag into the last param? No, PIN_MAPPABLE is only for legacy platform that used the aperture BAR for stolen mem access via GGTT. MTL doesn't have it and stolen is directly accessible via the LMEM BAR (which is actually the same BAR 2, but now behaves differently). > > > alan:snip > >> 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 a2a0813b8a76..c01286dddbdb 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h >> @@ -18,6 +18,7 @@ struct intel_gsc_uc { >> >> /* GSC-specific additions */ >> struct i915_vma *local; /* private memory for GSC usage */ >> + void __iomem *local_vaddr; /* pointer to access the private memory */ > alan:nit: relooking at the these variable names that originate from > last year's patch you worked on introducing gsc_uc, i am wondering now > if we should rename "local" to "privmem" and local_vaddr becomes privmem_vaddr. > (no significant reason other than improving readibility of the code) IIRC I used local because one of the GSC docs referred to it that way. I don't mind the renaming, but I don't think it should be done as part of this patch. Daniele > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 2/6] drm/i915/uc/gsc: fixes and updates for GSC memory allocation 2023-05-23 15:21 ` Ceraolo Spurio, Daniele @ 2023-06-06 0:00 ` Teres Alexis, Alan Previn 0 siblings, 0 replies; 23+ messages in thread From: Teres Alexis, Alan Previn @ 2023-06-06 0:00 UTC (permalink / raw) To: Ceraolo Spurio, Daniele, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On Tue, 2023-05-23 at 08:21 -0700, Ceraolo Spurio, Daniele wrote: > > > > > > +static int gsc_allocate_and_map_vma(struct intel_gsc_uc *gsc, u32 size) > > alan:snip > > > + obj = i915_gem_object_create_stolen(gt->i915, s0ize); > > > + if (IS_ERR(obj)) > > > + return PTR_ERR(obj); > > > + > > > + vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0); > > alan: should we be passing in the PIN_MAPPABLE flag into the last param? > > No, PIN_MAPPABLE is only for legacy platform that used the aperture BAR > for stolen mem access via GGTT. MTL doesn't have it and stolen is > directly accessible via the LMEM BAR (which is actually the same BAR 2, > but now behaves differently). alan: thanks - sounds good - i forgot about those differentiations > > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h > > > @@ -18,6 +18,7 @@ struct intel_gsc_uc { > > > > > > /* GSC-specific additions */ > > > struct i915_vma *local; /* private memory for GSC usage */ > > > + void __iomem *local_vaddr; /* pointer to access the private memory */ > > alan:nit: relooking at the these variable names that originate from > > last year's patch you worked on introducing gsc_uc, i am wondering now > > if we should rename "local" to "privmem" and local_vaddr becomes privmem_vaddr. > > (no significant reason other than improving readibility of the code) > > IIRC I used local because one of the GSC docs referred to it that way. I > don't mind the renaming, but I don't think it should be done as part of > this patch. alan: sure - sounds good. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Intel-gfx] [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary 2023-05-05 16:04 [Intel-gfx] [PATCH 0/6] drm/i915: GSC FW support for MTL Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] [PATCH 1/6] DO NOT REVIEW: drm/i915: HuC loading and authentication " Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] [PATCH 2/6] drm/i915/uc/gsc: fixes and updates for GSC memory allocation Daniele Ceraolo Spurio @ 2023-05-05 16:04 ` Daniele Ceraolo Spurio 2023-05-25 5:14 ` Teres Alexis, Alan Previn 2023-05-05 16:04 ` [Intel-gfx] [PATCH 4/6] drm/i915/uc/gsc: query the GSC FW for its compatibility version Daniele Ceraolo Spurio ` (3 subsequent siblings) 6 siblings, 1 reply; 23+ messages in thread From: Daniele Ceraolo Spurio @ 2023-05-05 16:04 UTC (permalink / raw) To: intel-gfx; +Cc: Alan Previn, dri-devel The release and security versions of the GSC binary are not used at runtime to decide interface compatibility (there is a separate version for that), but they're still useful for debug, so it is still worth extracting them and printing them out in dmesg. To get to these version, we need to navigate through various headers in the binary. See in-code comment for details. Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Alan Previn <alan.previn.teres.alexis@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 130 +++++++++++++++++- drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 2 + .../drm/i915/gt/uc/intel_gsc_meu_headers.h | 83 ++++++++++- drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 3 + drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 13 +- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 30 ++-- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 3 + 7 files changed, 237 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c index 4814901fac9e..df53c13e99a3 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c @@ -10,6 +10,7 @@ #include "gt/intel_gt_print.h" #include "gt/intel_ring.h" #include "intel_gsc_fw.h" +#include "intel_gsc_meu_headers.h" #define GSC_FW_STATUS_REG _MMIO(0x116C40) #define GSC_FW_CURRENT_STATE REG_GENMASK(3, 0) @@ -42,6 +43,129 @@ bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc) return fw_status & GSC_FW_INIT_COMPLETE_BIT; } +static inline u32 cpd_entry_offset(const struct intel_gsc_cpd_entry *entry) +{ + return entry->offset & INTEL_GSC_CPD_ENTRY_OFFSET_MASK; +} + +int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, size_t size) +{ + struct intel_gsc_uc *gsc = container_of(gsc_fw, struct intel_gsc_uc, fw); + struct intel_gt *gt = gsc_uc_to_gt(gsc); + const struct intel_gsc_layout_pointers *layout = data; + const struct intel_gsc_bpdt_header *bpdt_header = NULL; + const struct intel_gsc_bpdt_entry *bpdt_entry = NULL; + const struct intel_gsc_cpd_header_v2 *cpd_header = NULL; + const struct intel_gsc_cpd_entry *cpd_entry = NULL; + const struct intel_gsc_manifest_header *manifest; + size_t min_size = sizeof(*layout); + int i; + + if (size < min_size) { + gt_err(gt, "GSC FW too small! %zu < %zu\n", size, min_size); + return -ENODATA; + } + + /* + * The GSC binary starts with the pointer layout, which contains the + * locations of the various partitions of the binary. The one we're + * interested in to get the version is the boot1 partition, where we can + * find a BPDT header followed by entries, one of which points to the + * RBE sub-section of the partition. From here, we can parse the CPD + * header and the following entries to find the manifest location + * (entry identified by the "RBEP.man" name), from which we can finally + * extract the version. + * + * -------------------------------------------------- + * [ intel_gsc_layout_pointers ] + * [ ... ] + * [ boot1_offset >---------------------------]------o + * [ ... ] | + * -------------------------------------------------- | + * | + * -------------------------------------------------- | + * [ intel_gsc_bpdt_header ]<-----o + * -------------------------------------------------- + * [ intel_gsc_bpdt_entry[] ] + * [ entry1 ] + * [ ... ] + * [ entryX ] + * [ type == GSC_RBE ] + * [ offset >-----------------------------]------o + * [ ... ] | + * -------------------------------------------------- | + * | + * -------------------------------------------------- | + * [ intel_gsc_cpd_header_v2 ]<-----o + * -------------------------------------------------- + * [ intel_gsc_cpd_entry[] ] + * [ entry1 ] + * [ ... ] + * [ entryX ] + * [ "RBEP.man" ] + * [ ... ] + * [ offset >----------------------------]------o + * [ ... ] | + * -------------------------------------------------- | + * | + * -------------------------------------------------- | + * [ intel_gsc_manifest_header ]<-----o + * [ ... ] + * [ intel_gsc_meu_version fw_version ] + * [ ... ] + * -------------------------------------------------- + */ + + min_size = layout->boot1_offset + layout->boot1_offset > size; + if (size < min_size) { + gt_err(gt, "GSC FW too small for boot section! %zu < %zu\n", + size, min_size); + return -ENODATA; + } + + bpdt_header = data + layout->boot1_offset; + if (bpdt_header->signature != INTEL_GSC_BPDT_HEADER_SIGNATURE) { + gt_err(gt, "invalid signature for meu BPDT header: 0x%08x!\n", + bpdt_header->signature); + return -EINVAL; + } + + bpdt_entry = (void *)bpdt_header + sizeof(*bpdt_header); + for (i = 0; i < bpdt_header->descriptor_count; i++, bpdt_entry++) { + if ((bpdt_entry->type & INTEL_GSC_BPDT_ENTRY_TYPE_MASK) != + INTEL_GSC_BPDT_ENTRY_TYPE_GSC_RBE) + continue; + + cpd_header = (void *)bpdt_header + bpdt_entry->sub_partition_offset; + break; + } + + if (!cpd_header) { + gt_err(gt, "couldn't find CPD header in GSC binary!\n"); + return -ENODATA; + } + + if (cpd_header->header_marker != INTEL_GSC_CPD_HEADER_MARKER) { + gt_err(gt, "invalid marker for meu CPD header in GSC bin: 0x%08x!\n", + cpd_header->header_marker); + return -EINVAL; + } + + cpd_entry = (void *)cpd_header + cpd_header->header_length; + for (i = 0; i < cpd_header->num_of_entries; i++, cpd_entry++) { + if (strcmp(cpd_entry->name, "RBEP.man") == 0) { + manifest = (void *)cpd_header + cpd_entry_offset(cpd_entry); + intel_uc_fw_version_from_meu_manifest(&gsc->release, + manifest); + gsc->security_version = manifest->security_version; + break; + } + } + + return 0; +} + + static int emit_gsc_fw_load(struct i915_request *rq, struct intel_gsc_uc *gsc) { u32 offset = i915_ggtt_offset(gsc->local); @@ -206,7 +330,11 @@ int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc) /* FW is not fully operational until we enable SW proxy */ intel_uc_fw_change_status(gsc_fw, INTEL_UC_FIRMWARE_TRANSFERRED); - gt_info(gt, "Loaded GSC firmware %s\n", gsc_fw->file_selected.path); + gt_info(gt, "Loaded GSC firmware %s (r%u.%u.%u.%u, svn%u)\n", + gsc_fw->file_selected.path, + gsc->release.major, gsc->release.minor, + gsc->release.patch, gsc->release.build, + gsc->security_version); return 0; diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h index fff8928218df..8d7b9e4f1ffc 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h @@ -9,8 +9,10 @@ #include <linux/types.h> struct intel_gsc_uc; +struct intel_uc_fw; struct intel_uncore; +int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, size_t size); int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc); bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc); bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h index d55a66202576..8bce2b8aed84 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h @@ -8,6 +8,82 @@ #include <linux/types.h> +struct intel_gsc_meu_version { + u16 major; + u16 minor; + u16 hotfix; + u16 build; +} __packed; + +struct intel_gsc_layout_pointers { + u8 rom_bypass_vector[16]; + + /* size of pointers layout not including ROM bypass vector */ + u16 size; + + /* + * bit0: Backup copy of layout pointers exist + * bits1-15: reserved + */ + u8 flags; + + u8 reserved; + + u32 crc32; + + u32 datap_offset; + u32 datap_size; + + u32 boot1_offset; + u32 boot1_size; + + u32 boot2_offset; + u32 boot2_size; + + u32 boot3_offset; + u32 boot3_size; + + u32 boot4_offset; + u32 boot4_size; + + u32 boot5_offset; + u32 boot5_size; + + u32 temp_pages_offset; + u32 temp_pages_size; +} __packed; + +struct intel_gsc_bpdt_header { + u32 signature; +#define INTEL_GSC_BPDT_HEADER_SIGNATURE 0x000055AA + + u16 descriptor_count; /* bum of entries after the header */ + + u8 version; + u8 configuration; + + u32 crc32; + + u32 build_version; + struct intel_gsc_meu_version tool_version; +} __packed; + + +struct intel_gsc_bpdt_entry { + /* + * Bits 0-15: BPDT entry type + * Bits 16-17: reserved + * Bit 18: code sub-partition + * Bits 19-31: reserved + */ + u32 type; +#define INTEL_GSC_BPDT_ENTRY_TYPE_MASK GENMASK(15,0) +#define INTEL_GSC_BPDT_ENTRY_TYPE_GSC_RBE 0x1 + + u32 sub_partition_offset; /* from the base of the BPDT header */ + u32 sub_partition_size; +} __packed; + /* Code partition directory (CPD) structures */ struct intel_gsc_cpd_header_v2 { u32 header_marker; @@ -44,13 +120,6 @@ struct intel_gsc_cpd_entry { u8 reserved[4]; } __packed; -struct intel_gsc_meu_version { - u16 major; - u16 minor; - u16 hotfix; - u16 build; -} __packed; - struct intel_gsc_manifest_header { u32 header_type; /* 0x4 for manifest type */ u32 header_length; /* in dwords */ 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 c01286dddbdb..d778a01f8c05 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h @@ -17,6 +17,9 @@ struct intel_gsc_uc { struct intel_uc_fw fw; /* GSC-specific additions */ + struct intel_uc_fw_ver release; + u32 security_version; + struct i915_vma *local; /* private memory for GSC usage */ void __iomem *local_vaddr; /* pointer to access the private memory */ struct intel_context *ce; /* for submission to GSC FW via GSC engine */ diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c index 0a0bd5504057..0c01d48b1dd9 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c @@ -106,15 +106,6 @@ int intel_huc_fw_auth_via_gsccs(struct intel_huc *huc) return err; } -static void get_version_from_meu_manifest(struct intel_uc_fw_ver *ver, const void *data) -{ - const struct intel_gsc_manifest_header *manifest = data; - - ver->major = manifest->fw_version.major; - ver->minor = manifest->fw_version.minor; - ver->patch = manifest->fw_version.hotfix; -} - static bool css_valid(const void *data, size_t size) { const struct uc_css_header *css = data; @@ -226,8 +217,8 @@ int intel_huc_fw_get_binary_info(struct intel_uc_fw *huc_fw, const void *data, s for (i = 0; i < header->num_of_entries; i++, entry++) { if (strcmp(entry->name, "HUCP.man") == 0) - get_version_from_meu_manifest(&huc_fw->file_selected.ver, - data + entry_offset(entry)); + intel_uc_fw_version_from_meu_manifest(&huc_fw->file_selected.ver, + data + entry_offset(entry)); if (strcmp(entry->name, "huc_fw") == 0) { u32 offset = entry_offset(entry); 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 796f54a62eef..cd8fc194f7fa 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -12,6 +12,8 @@ #include "gem/i915_gem_lmem.h" #include "gt/intel_gt_print.h" +#include "intel_gsc_fw.h" +#include "intel_gsc_meu_headers.h" #include "intel_uc_fw.h" #include "intel_uc_fw_abi.h" #include "i915_drv.h" @@ -419,6 +421,17 @@ static void __uc_fw_user_override(struct drm_i915_private *i915, struct intel_uc } } +void intel_uc_fw_version_from_meu_manifest(struct intel_uc_fw_ver *ver, + const void *data) +{ + const struct intel_gsc_manifest_header *manifest = data; + + ver->major = manifest->fw_version.major; + ver->minor = manifest->fw_version.minor; + ver->patch = manifest->fw_version.hotfix; + ver->build = manifest->fw_version.build; +} + /** * intel_uc_fw_init_early - initialize the uC object and select the firmware * @uc_fw: uC firmware @@ -612,13 +625,18 @@ static int check_gsc_manifest(struct intel_gt *gt, const struct firmware *fw, struct intel_uc_fw *uc_fw) { - if (uc_fw->type != INTEL_UC_FW_TYPE_HUC) { - gt_err(gt, "trying to MEU-parse a non-HuC binary"); + switch (uc_fw->type) { + case INTEL_UC_FW_TYPE_HUC: + intel_huc_fw_get_binary_info(uc_fw, fw->data, fw->size); + break; + case INTEL_UC_FW_TYPE_GSC: + intel_gsc_fw_get_binary_info(uc_fw, fw->data, fw->size); + break; + default: + MISSING_CASE(uc_fw->type); return -EINVAL; } - intel_huc_fw_get_binary_info(uc_fw, fw->data, fw->size); - if (uc_fw->dma_start_offset) { u32 delta = uc_fw->dma_start_offset; __check_ccs_header(gt, fw->data + delta, fw->size - delta, uc_fw); @@ -677,10 +695,6 @@ static int check_fw_header(struct intel_gt *gt, { 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->is_meu_binary) err = check_gsc_manifest(gt, fw, uc_fw); else 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 8f2306627332..279244744d43 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h @@ -70,6 +70,7 @@ struct intel_uc_fw_ver { u32 major; u32 minor; u32 patch; + u32 build; }; /* @@ -284,6 +285,8 @@ static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw) return __intel_uc_fw_get_upload_size(uc_fw); } +void intel_uc_fw_version_from_meu_manifest(struct intel_uc_fw_ver *ver, + const void *data); void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw, enum intel_uc_fw_type type, bool needs_ggtt_mapping); -- 2.40.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary 2023-05-05 16:04 ` [Intel-gfx] [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary Daniele Ceraolo Spurio @ 2023-05-25 5:14 ` Teres Alexis, Alan Previn 2023-05-25 16:56 ` Ceraolo Spurio, Daniele 2023-05-27 1:27 ` Ceraolo Spurio, Daniele 0 siblings, 2 replies; 23+ messages in thread From: Teres Alexis, Alan Previn @ 2023-05-25 5:14 UTC (permalink / raw) To: Ceraolo Spurio, Daniele, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: alan: snip > +int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, size_t size) > +{ alan:snip > + /* > + * The GSC binary starts with the pointer layout, which contains the > + * locations of the various partitions of the binary. The one we're > + * interested in to get the version is the boot1 partition, where we can > + * find a BPDT header followed by entries, one of which points to the > + * RBE sub-section of the partition. From here, we can parse the CPD alan: nit: could we add the meaning of 'RBE', probably not here but in the header file where GSC_RBE is defined? > + * header and the following entries to find the manifest location > + * (entry identified by the "RBEP.man" name), from which we can finally > + * extract the version. > + * > + * -------------------------------------------------- > + * [ intel_gsc_layout_pointers ] > + * [ ... ] > + * [ boot1_offset >---------------------------]------o > + * [ ... ] | > + * -------------------------------------------------- | > + * | > + * -------------------------------------------------- | > + * [ intel_gsc_bpdt_header ]<-----o > + * -------------------------------------------------- alan: special thanks for the diagram - love these! :) alan: snip > + min_size = layout->boot1_offset + layout->boot1_offset > size; alan: latter is a binary so + 1? or is this a typo and supposed to be: min_size = layout->boot1_offset; actually since we are accessing a bpdt_header hanging off that offset, it should rather be: min_size = layout->boot1_offset + sizeof(*bpdt_header); > + if (size < min_size) { > + gt_err(gt, "GSC FW too small for boot section! %zu < %zu\n", > + size, min_size); > + return -ENODATA; > + } > + > + bpdt_header = data + layout->boot1_offset; > + if (bpdt_header->signature != INTEL_GSC_BPDT_HEADER_SIGNATURE) { > + gt_err(gt, "invalid signature for meu BPDT header: 0x%08x!\n", > + bpdt_header->signature); > + return -EINVAL; > + } > + alan: IIUC, to be strict about the size-crawl-checking, we should check minsize again - this time with the additional "bpdt_header->descriptor_count * sizeof(*bpdt_entry)". (hope i got that right?) - adding that check before parsing through the (bpdt_entry++)'s ->type > + bpdt_entry = (void *)bpdt_header + sizeof(*bpdt_header); > + for (i = 0; i < bpdt_header->descriptor_count; i++, bpdt_entry++) { > + if ((bpdt_entry->type & INTEL_GSC_BPDT_ENTRY_TYPE_MASK) != > + INTEL_GSC_BPDT_ENTRY_TYPE_GSC_RBE) > + continue; > + > + cpd_header = (void *)bpdt_header + bpdt_entry->sub_partition_offset; > + break; > + } > + > + if (!cpd_header) { > + gt_err(gt, "couldn't find CPD header in GSC binary!\n"); > + return -ENODATA; > + } alan: same as above, so for size-crawl-checking, we should check minsize again with the addition of cpd_header, no? > + > + if (cpd_header->header_marker != INTEL_GSC_CPD_HEADER_MARKER) { > + gt_err(gt, "invalid marker for meu CPD header in GSC bin: 0x%08x!\n", > + cpd_header->header_marker); > + return -EINVAL; > + } alan: and again here, the size crawl checking with cpd_header->num_of_entries * *cpd_entry > + cpd_entry = (void *)cpd_header + cpd_header->header_length; > + for (i = 0; i < cpd_header->num_of_entries; i++, cpd_entry++) { > + if (strcmp(cpd_entry->name, "RBEP.man") == 0) { > + manifest = (void *)cpd_header + cpd_entry_offset(cpd_entry); > + intel_uc_fw_version_from_meu_manifest(&gsc->release, > + manifest); > + gsc->security_version = manifest->security_version; > + break; > + } > + } > + > + return 0; alan:snip > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h > index fff8928218df..8d7b9e4f1ffc 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h alan:snip > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h > index d55a66202576..8bce2b8aed84 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h alan:snip > +struct intel_gsc_layout_pointers { > + u8 rom_bypass_vector[16]; alan:snip... > + u32 temp_pages_offset; > + u32 temp_pages_size; > +} __packed; alan: structure layout seems unnecessarily repetitive... why not -> struct partition_info { u32 offset; u32 size; }; struct intel_gsc_layout_pointers { u8 rom_bypass_vector[16]; ... struct partition_info datap; struct partition_info bootregion[5]; struct partition_info trace; }__packed; > + alan: we should put the 'bpdt' acronym meaning and if its an intel specific name, then a bit of additional comment explaining what it means. > +struct intel_gsc_bpdt_header { > + u32 signature; > +#define INTEL_GSC_BPDT_HEADER_SIGNATURE 0x000055AA > + > + u16 descriptor_count; /* bum of entries after the header */ alan:s/bum/num > + > + u8 version; > + u8 configuration; > + > + u32 crc32; > + > + u32 build_version; > + struct intel_gsc_meu_version tool_version; alan: nit: "struct intel_gsc_meu_version meu_version" is better no? > +} __packed; > + > + > +struct intel_gsc_bpdt_entry { > + /* > + * Bits 0-15: BPDT entry type > + * Bits 16-17: reserved > + * Bit 18: code sub-partition > + * Bits 19-31: reserved > + */ alan: nit: i think its neater to just put above comments next to the #defines on the lines following 'type' and create a genmask for code-sub-partition (if we use it, else skip it?) - the benefit being a little less clutter > + u32 type; > +#define INTEL_GSC_BPDT_ENTRY_TYPE_MASK GENMASK(15,0) > +#define INTEL_GSC_BPDT_ENTRY_TYPE_GSC_RBE 0x1 > + > + u32 sub_partition_offset; /* from the base of the BPDT header */ > + u32 sub_partition_size; > +} __packed; > + alan:snip > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h > @@ -17,6 +17,9 @@ struct intel_gsc_uc { > struct intel_uc_fw fw; > > /* GSC-specific additions */ > + struct intel_uc_fw_ver release; > + u32 security_version; alan: for consistency and less redundancy, can't we add "security_version" into 'struct intel_uc_fw_ver' (which is zero for firmware that doesn't have it). That way, intel_gsc_uc can re-use intel_uc_fw.file_selected just like huc? alan:snip > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > index 0a0bd5504057..0c01d48b1dd9 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c alan:snip > 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 796f54a62eef..cd8fc194f7fa 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c alan:snip > 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 8f2306627332..279244744d43 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h alan:snip ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary 2023-05-25 5:14 ` Teres Alexis, Alan Previn @ 2023-05-25 16:56 ` Ceraolo Spurio, Daniele 2023-05-25 22:03 ` Teres Alexis, Alan Previn 2023-05-27 1:27 ` Ceraolo Spurio, Daniele 1 sibling, 1 reply; 23+ messages in thread From: Ceraolo Spurio, Daniele @ 2023-05-25 16:56 UTC (permalink / raw) To: Teres Alexis, Alan Previn, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On 5/24/2023 10:14 PM, Teres Alexis, Alan Previn wrote: > On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: > > > alan: snip > > >> +int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, size_t size) >> +{ > alan:snip >> + /* >> + * The GSC binary starts with the pointer layout, which contains the >> + * locations of the various partitions of the binary. The one we're >> + * interested in to get the version is the boot1 partition, where we can >> + * find a BPDT header followed by entries, one of which points to the >> + * RBE sub-section of the partition. From here, we can parse the CPD > alan: nit: could we add the meaning of 'RBE', probably not here but in the header file where GSC_RBE is defined? I have no idea what RBE means, the specs just says to look for the section named that way. I didn't dig because we don't really care about its name, just that the CPD header is in there. >> + * header and the following entries to find the manifest location >> + * (entry identified by the "RBEP.man" name), from which we can finally >> + * extract the version. >> + * >> + * -------------------------------------------------- >> + * [ intel_gsc_layout_pointers ] >> + * [ ... ] >> + * [ boot1_offset >---------------------------]------o >> + * [ ... ] | >> + * -------------------------------------------------- | >> + * | >> + * -------------------------------------------------- | >> + * [ intel_gsc_bpdt_header ]<-----o >> + * -------------------------------------------------- > alan: special thanks for the diagram - love these! :) > alan: snip > >> + min_size = layout->boot1_offset + layout->boot1_offset > size; > alan: latter is a binary so + 1? or is this a typo and supposed to be: > min_size = layout->boot1_offset; it's a cut & paste typo, it should be min_size = layout->boot1_offset + layout->boot1_size; thanks for spotting it. > actually since we are accessing a bpdt_header hanging off that offset, it should rather be: > min_size = layout->boot1_offset + sizeof(*bpdt_header); I can add a check to make sure that boot1_size >= sizeof(*bpdt_header) >> + if (size < min_size) { >> + gt_err(gt, "GSC FW too small for boot section! %zu < %zu\n", >> + size, min_size); >> + return -ENODATA; >> + } >> + >> + bpdt_header = data + layout->boot1_offset; >> + if (bpdt_header->signature != INTEL_GSC_BPDT_HEADER_SIGNATURE) { >> + gt_err(gt, "invalid signature for meu BPDT header: 0x%08x!\n", >> + bpdt_header->signature); >> + return -EINVAL; >> + } >> + > alan: IIUC, to be strict about the size-crawl-checking, we should check minsize > again - this time with the additional "bpdt_header->descriptor_count * sizeof(*bpdt_entry)". > (hope i got that right?) - adding that check before parsing through the (bpdt_entry++)'s ->type will do (same for the comments below) >> + bpdt_entry = (void *)bpdt_header + sizeof(*bpdt_header); >> + for (i = 0; i < bpdt_header->descriptor_count; i++, bpdt_entry++) { >> + if ((bpdt_entry->type & INTEL_GSC_BPDT_ENTRY_TYPE_MASK) != >> + INTEL_GSC_BPDT_ENTRY_TYPE_GSC_RBE) >> + continue; >> + >> + cpd_header = (void *)bpdt_header + bpdt_entry->sub_partition_offset; >> + break; >> + } >> + >> + if (!cpd_header) { >> + gt_err(gt, "couldn't find CPD header in GSC binary!\n"); >> + return -ENODATA; >> + } > alan: same as above, so for size-crawl-checking, we should check minsize again with the addition of cpd_header, no? >> + >> + if (cpd_header->header_marker != INTEL_GSC_CPD_HEADER_MARKER) { >> + gt_err(gt, "invalid marker for meu CPD header in GSC bin: 0x%08x!\n", >> + cpd_header->header_marker); >> + return -EINVAL; >> + } > alan: and again here, the size crawl checking with cpd_header->num_of_entries * *cpd_entry >> + cpd_entry = (void *)cpd_header + cpd_header->header_length; >> + for (i = 0; i < cpd_header->num_of_entries; i++, cpd_entry++) { >> + if (strcmp(cpd_entry->name, "RBEP.man") == 0) { >> + manifest = (void *)cpd_header + cpd_entry_offset(cpd_entry); >> + intel_uc_fw_version_from_meu_manifest(&gsc->release, >> + manifest); >> + gsc->security_version = manifest->security_version; >> + break; >> + } >> + } >> + >> + return 0; > alan:snip > >> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h >> index fff8928218df..8d7b9e4f1ffc 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h > alan:snip > > >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h >> index d55a66202576..8bce2b8aed84 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h > alan:snip > > > >> +struct intel_gsc_layout_pointers { >> + u8 rom_bypass_vector[16]; > alan:snip... >> + u32 temp_pages_offset; >> + u32 temp_pages_size; >> +} __packed; > alan: structure layout seems unnecessarily repetitive... why not -> > struct partition_info { > u32 offset; > u32 size; > }; > struct intel_gsc_layout_pointers { > u8 rom_bypass_vector[16]; > ... > struct partition_info datap; > struct partition_info bootregion[5]; > struct partition_info trace; > }__packed; > > >> + > alan: we should put the 'bpdt' acronym meaning and if its an intel specific > name, then a bit of additional comment explaining what it means. BPD stands for Boot Partition Directory, not sure what the T is for (Type maybe?). I'll add a comment that these are the structures for the GSC boot partition, it should make things clear enough. >> +struct intel_gsc_bpdt_header { >> + u32 signature; >> +#define INTEL_GSC_BPDT_HEADER_SIGNATURE 0x000055AA >> + >> + u16 descriptor_count; /* bum of entries after the header */ > alan:s/bum/num >> + >> + u8 version; >> + u8 configuration; >> + >> + u32 crc32; >> + >> + u32 build_version; >> + struct intel_gsc_meu_version tool_version; > alan: nit: "struct intel_gsc_meu_version meu_version" is better no? no. struct intel_gsc_meu_version is the basic struct used for versions encoded in the meu headers and it is used for multiple entries in the various headers. This particular one is the tool_version, hence the name. >> +} __packed; >> + >> + >> +struct intel_gsc_bpdt_entry { >> + /* >> + * Bits 0-15: BPDT entry type >> + * Bits 16-17: reserved >> + * Bit 18: code sub-partition >> + * Bits 19-31: reserved >> + */ > alan: nit: i think its neater to just put above comments next to the #defines on the lines following 'type' and > create a genmask for code-sub-partition (if we use it, else skip it?) - the benefit being a little less clutter I think the lines would get too long if I put the comments on the same lines as the defines. We only use the lower 16 bits, hence why I only defined those, but I wanted to have the whole thing documented for completeness. > >> + u32 type; >> +#define INTEL_GSC_BPDT_ENTRY_TYPE_MASK GENMASK(15,0) >> +#define INTEL_GSC_BPDT_ENTRY_TYPE_GSC_RBE 0x1 >> + >> + u32 sub_partition_offset; /* from the base of the BPDT header */ >> + u32 sub_partition_size; >> +} __packed; >> + > alan:snip > > >> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h >> @@ -17,6 +17,9 @@ struct intel_gsc_uc { >> struct intel_uc_fw fw; >> >> /* GSC-specific additions */ >> + struct intel_uc_fw_ver release; >> + u32 security_version; > alan: for consistency and less redundancy, can't we add "security_version" > into 'struct intel_uc_fw_ver' (which is zero for firmware that doesn't > have it). That way, intel_gsc_uc can re-use intel_uc_fw.file_selected > just like huc? I'm not sure what you mean by re-using intel_uc_fw.file_selected. Is that for the call from intel_uc_fw_version_from_meu_manifest? I'm purposely not doing that. Note that the GSC has 3 versions: Release version (incremented with each build and encoded in the header) Security version (also encoded in the header) Compatibility version (queried via message to the GSC) The one we care about for communicating with the GSC is the last one, so that's the one I stored in intel_uc_fw.file_selected (in the next patch). The other 2 versions are not strictly required to use the GSC and we only fetch them for debug purposes, so if something goes wrong we know exactly what we've loaded. Daniele > > alan:snip > > > >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c >> index 0a0bd5504057..0c01d48b1dd9 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > alan:snip > > >> 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 796f54a62eef..cd8fc194f7fa 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > alan:snip > >> 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 8f2306627332..279244744d43 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h > alan:snip ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary 2023-05-25 16:56 ` Ceraolo Spurio, Daniele @ 2023-05-25 22:03 ` Teres Alexis, Alan Previn 0 siblings, 0 replies; 23+ messages in thread From: Teres Alexis, Alan Previn @ 2023-05-25 22:03 UTC (permalink / raw) To: Ceraolo Spurio, Daniele, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On Thu, 2023-05-25 at 09:56 -0700, Ceraolo Spurio, Daniele wrote: > On 5/24/2023 10:14 PM, Teres Alexis, Alan Previn wrote: > > On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: alan:snip > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h > > > @@ -17,6 +17,9 @@ struct intel_gsc_uc { > > > struct intel_uc_fw fw; > > > > > > /* GSC-specific additions */ > > > + struct intel_uc_fw_ver release; > > > + u32 security_version; > > alan: for consistency and less redundancy, can't we add "security_version" > > into 'struct intel_uc_fw_ver' (which is zero for firmware that doesn't > > have it). That way, intel_gsc_uc can re-use intel_uc_fw.file_selected > > just like huc? > > I'm not sure what you mean by re-using intel_uc_fw.file_selected. Is > that for the call from intel_uc_fw_version_from_meu_manifest? I'm > purposely not doing that. Note that the GSC has 3 versions: > > Release version (incremented with each build and encoded in the header) > Security version (also encoded in the header) > Compatibility version (queried via message to the GSC) > > The one we care about for communicating with the GSC is the last one, so > that's the one I stored in intel_uc_fw.file_selected (in the next > patch). The other 2 versions are not strictly required to use the GSC > and we only fetch them for debug purposes, so if something goes wrong we > know exactly what we've loaded. > > Daniele alan: okay thanks - seeing that now in the next patch... (and i also forgot that the GSC release version doesnt reflect interface versioning in anyway like GuC does). In that case, above additional versions are fine. Would definitely love to see additional comments under "GSC-specific-additions" that explain those 3 versioning items and what we care about as how you have explained here. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary 2023-05-25 5:14 ` Teres Alexis, Alan Previn 2023-05-25 16:56 ` Ceraolo Spurio, Daniele @ 2023-05-27 1:27 ` Ceraolo Spurio, Daniele 2023-06-05 23:51 ` Teres Alexis, Alan Previn 1 sibling, 1 reply; 23+ messages in thread From: Ceraolo Spurio, Daniele @ 2023-05-27 1:27 UTC (permalink / raw) To: Teres Alexis, Alan Previn, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org <snip> >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h >> index d55a66202576..8bce2b8aed84 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h > alan:snip > > > >> +struct intel_gsc_layout_pointers { >> + u8 rom_bypass_vector[16]; > alan:snip... >> + u32 temp_pages_offset; >> + u32 temp_pages_size; >> +} __packed; > alan: structure layout seems unnecessarily repetitive... why not -> > struct partition_info { > u32 offset; > u32 size; > }; > struct intel_gsc_layout_pointers { > u8 rom_bypass_vector[16]; > ... > struct partition_info datap; > struct partition_info bootregion[5]; > struct partition_info trace; > }__packed; > I've just realized that I didn't reply to this comment. The specs have the structure defined that way, so I tried to keep a 1:1 match like we usually do. I think switching to a partition_info structure is ok, but I'll avoid the array because the GSC partition are 1-based, which could cause confusion (i.e. partition boot1 would be bootregion[0]). Daniele ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary 2023-05-27 1:27 ` Ceraolo Spurio, Daniele @ 2023-06-05 23:51 ` Teres Alexis, Alan Previn 0 siblings, 0 replies; 23+ messages in thread From: Teres Alexis, Alan Previn @ 2023-06-05 23:51 UTC (permalink / raw) To: Ceraolo Spurio, Daniele, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On Fri, 2023-05-26 at 18:27 -0700, Ceraolo Spurio, Daniele wrote: > <snip> > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h > > > index d55a66202576..8bce2b8aed84 100644 > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h alan:snip > > alan: structure layout seems unnecessarily repetitive... why not -> > > struct partition_info { > > u32 offset; > > u32 size; > > }; > > struct intel_gsc_layout_pointers { > > u8 rom_bypass_vector[16]; > > ... > > struct partition_info datap; > > struct partition_info bootregion[5]; > > struct partition_info trace; > > }__packed; > > > > I've just realized that I didn't reply to this comment. The specs have > the structure defined that way, so I tried to keep a 1:1 match like we > usually do. I think switching to a partition_info structure is ok, but > I'll avoid the array because the GSC partition are 1-based, which could > cause confusion (i.e. partition boot1 would be bootregion[0]). alan: sure - that's fine - let's stick to align with the spec definitions ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Intel-gfx] [PATCH 4/6] drm/i915/uc/gsc: query the GSC FW for its compatibility version 2023-05-05 16:04 [Intel-gfx] [PATCH 0/6] drm/i915: GSC FW support for MTL Daniele Ceraolo Spurio ` (2 preceding siblings ...) 2023-05-05 16:04 ` [Intel-gfx] [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary Daniele Ceraolo Spurio @ 2023-05-05 16:04 ` Daniele Ceraolo Spurio 2023-05-25 23:29 ` Teres Alexis, Alan Previn 2023-05-05 16:04 ` [Intel-gfx] [PATCH 5/6] drm/i915/uc/gsc: define gsc fw Daniele Ceraolo Spurio ` (2 subsequent siblings) 6 siblings, 1 reply; 23+ messages in thread From: Daniele Ceraolo Spurio @ 2023-05-05 16:04 UTC (permalink / raw) To: intel-gfx; +Cc: Alan Previn, dri-devel The compatibility version is queried via an MKHI command. Right now, the only existing interface is 1.0 This is basically the interface version for the GSC FW, so the plan is to use it as the main tracked version, including for the binary naming in the fetch code. 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_gsc_fw.c | 91 ++++++++++++++++++- .../i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h | 1 + drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 44 +++++---- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 1 + 4 files changed, 120 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c index df53c13e99a3..0b6dcd982b14 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c @@ -11,6 +11,7 @@ #include "gt/intel_ring.h" #include "intel_gsc_fw.h" #include "intel_gsc_meu_headers.h" +#include "intel_gsc_uc_heci_cmd_submit.h" #define GSC_FW_STATUS_REG _MMIO(0x116C40) #define GSC_FW_CURRENT_STATE REG_GENMASK(3, 0) @@ -270,6 +271,84 @@ static int gsc_fw_wait(struct intel_gt *gt) 500); } +struct intel_gsc_mkhi_header { + u8 group_id; +#define MKHI_GROUP_ID_GFX_SRV 0x30 + + u8 command; +#define MKHI_GFX_SRV_GET_HOST_COMPATIBILITY_VERSION (0x42) + + u8 reserved; + u8 result; +} __packed; + +struct mtl_gsc_ver_msg_in { + struct intel_gsc_mtl_header header; + struct intel_gsc_mkhi_header mkhi; +} __packed; + +struct mtl_gsc_ver_msg_out { + struct intel_gsc_mtl_header header; + struct intel_gsc_mkhi_header mkhi; + u16 proj_major; + u16 compat_major; + u16 compat_minor; + u16 reserved[5]; +} __packed; + +static int gsc_fw_query_compatibility_version(struct intel_gsc_uc *gsc) +{ + struct intel_gt *gt = gsc_uc_to_gt(gsc); + struct mtl_gsc_ver_msg_in *msg_in; + struct mtl_gsc_ver_msg_out *msg_out; + struct i915_vma *vma; + u64 offset; + void *vaddr; + int err; + + err = intel_guc_allocate_and_map_vma(>->uc.guc, PAGE_SIZE * 2, + &vma, &vaddr); + if (err) { + gt_err(gt, "failed to allocate vma for GSC version query\n"); + return err; + } + + offset = i915_ggtt_offset(vma); + msg_in = vaddr; + msg_out = vaddr + SZ_4K; + + intel_gsc_uc_heci_cmd_emit_mtl_header(&msg_in->header, + HECI_MEADDRESS_MKHI, + sizeof(*msg_in), 0); + msg_in->mkhi.group_id = MKHI_GROUP_ID_GFX_SRV; + msg_in->mkhi.command = MKHI_GFX_SRV_GET_HOST_COMPATIBILITY_VERSION; + + err = intel_gsc_uc_heci_cmd_submit_packet(>->uc.gsc, + offset, sizeof(*msg_in), + offset + SZ_4K, SZ_4K); + if (err) { + gt_err(gt, + "failed to submit GSC request for compatibility version: %d\n", + err); + goto out_vma; + } + + if (msg_out->header.message_size != sizeof(*msg_out)) { + gt_err(gt, "invalid GSC reply length %u [expected %zu], s=0x%x, f=0x%x, r=0x%x\n", + msg_out->header.message_size, sizeof(*msg_out), + msg_out->header.status, msg_out->header.flags, msg_out->mkhi.result); + err = -EPROTO; + goto out_vma; + } + + gsc->fw.file_selected.ver.major = msg_out->compat_major; + gsc->fw.file_selected.ver.minor = msg_out->compat_minor; + +out_vma: + i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP); + return err; +} + int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc) { struct intel_gt *gt = gsc_uc_to_gt(gsc); @@ -327,11 +406,21 @@ int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc) if (err) goto fail; + err = gsc_fw_query_compatibility_version(gsc); + if (err) + goto fail; + + /* we only support compatibility version 1.0 at the moment */ + err = intel_uc_check_file_version(gsc_fw, NULL); + 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); - gt_info(gt, "Loaded GSC firmware %s (r%u.%u.%u.%u, svn%u)\n", + gt_info(gt, "Loaded GSC firmware %s (cv%u.%u, r%u.%u.%u.%u, svn %u)\n", gsc_fw->file_selected.path, + gsc_fw->file_selected.ver.major, gsc_fw->file_selected.ver.minor, gsc->release.major, gsc->release.minor, gsc->release.patch, gsc->release.build, gsc->security_version); diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h index 8f199d5f963e..fb1453ed4ecf 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h @@ -14,6 +14,7 @@ struct intel_gsc_mtl_header { #define GSC_HECI_VALIDITY_MARKER 0xA578875A u8 heci_client_id; +#define HECI_MEADDRESS_MKHI 7 #define HECI_MEADDRESS_PROXY 10 #define HECI_MEADDRESS_PXP 17 #define HECI_MEADDRESS_HDCP 18 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 cd8fc194f7fa..36ee96c02d74 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -730,6 +730,31 @@ static int try_firmware_load(struct intel_uc_fw *uc_fw, const struct firmware ** return 0; } +int intel_uc_check_file_version(struct intel_uc_fw *uc_fw, bool *old_ver) +{ + struct intel_gt *gt = __uc_fw_to_gt(uc_fw); + struct intel_uc_fw_file *wanted = &uc_fw->file_wanted; + struct intel_uc_fw_file *selected = &uc_fw->file_selected; + + if (!wanted->ver.major || !selected->ver.major) + return 0; + + /* Check the file's major version was as it claimed */ + if (selected->ver.major != wanted->ver.major) { + gt_notice(gt, "%s firmware %s: unexpected version: %u.%u != %u.%u\n", + intel_uc_fw_type_repr(uc_fw->type), selected->path, + selected->ver.major, selected->ver.minor, + wanted->ver.major, wanted->ver.minor); + if (!intel_uc_fw_is_overridden(uc_fw)) + return -ENOEXEC; + } else { + if (old_ver && selected->ver.minor < wanted->ver.minor) + *old_ver = true; + } + + return 0; +} + /** * intel_uc_fw_fetch - fetch uC firmware * @uc_fw: uC firmware @@ -797,22 +822,9 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw) goto fail; } - if (uc_fw->file_wanted.ver.major && uc_fw->file_selected.ver.major) { - /* Check the file's major version was as it claimed */ - if (uc_fw->file_selected.ver.major != uc_fw->file_wanted.ver.major) { - gt_notice(gt, "%s firmware %s: unexpected version: %u.%u != %u.%u\n", - intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path, - uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor, - uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor); - if (!intel_uc_fw_is_overridden(uc_fw)) { - err = -ENOEXEC; - goto fail; - } - } else { - if (uc_fw->file_selected.ver.minor < uc_fw->file_wanted.ver.minor) - old_ver = true; - } - } + err = intel_uc_check_file_version(uc_fw, &old_ver); + if (err) + goto fail; if (old_ver && uc_fw->file_selected.ver.major) { /* Preserve the version that was really wanted */ 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 279244744d43..4406e7b48b27 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h @@ -287,6 +287,7 @@ static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw) void intel_uc_fw_version_from_meu_manifest(struct intel_uc_fw_ver *ver, const void *data); +int intel_uc_check_file_version(struct intel_uc_fw *uc_fw, bool *old_ver); void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw, enum intel_uc_fw_type type, bool needs_ggtt_mapping); -- 2.40.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 4/6] drm/i915/uc/gsc: query the GSC FW for its compatibility version 2023-05-05 16:04 ` [Intel-gfx] [PATCH 4/6] drm/i915/uc/gsc: query the GSC FW for its compatibility version Daniele Ceraolo Spurio @ 2023-05-25 23:29 ` Teres Alexis, Alan Previn 0 siblings, 0 replies; 23+ messages in thread From: Teres Alexis, Alan Previn @ 2023-05-25 23:29 UTC (permalink / raw) To: Ceraolo Spurio, Daniele, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: > The compatibility version is queried via an MKHI command. Right now, the > only existing interface is 1.0 > This is basically the interface version for the GSC FW, so the plan is > to use it as the main tracked version, including for the binary naming > in the fetch code. > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> > alan: just a couple of minor things nits below. One ask though, in line with the clarification we had over the offline coversation, I am wondering if we can document the fact that the file_selected.ver remains as major-minor::zero-zero for the case of gsc until after the firmware is loaded and we query via this function (which happens later at gt-late-init). However, that comment might not belong here - perhaps it belongs in the prior patch together with the other comment i requested for (asking for additional explainations about the different types of versions for gsc). That said, for this patch, LGTM: Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com> alan:snip > +static int gsc_fw_query_compatibility_version(struct intel_gsc_uc *gsc) > +{ > + struct intel_gt *gt = gsc_uc_to_gt(gsc); > + struct mtl_gsc_ver_msg_in *msg_in; > + struct mtl_gsc_ver_msg_out *msg_out; > + struct i915_vma *vma; > + u64 offset; > + void *vaddr; > + int err; > + > + err = intel_guc_allocate_and_map_vma(>->uc.guc, PAGE_SIZE * 2, > + &vma, &vaddr); alan: nit: im assuming this code will be used for future discrete cards,.. if so, perhaps we should also be using "SZ_4K * 2" above since different host-cpu-arch could have different PAGE sizes - this way we'll be consistent with exact size allocations. also its more consistent in this function - maybe a #define GSC_UC_GET_ABI_VER_PKT_SIZE SZ_4K at top of function is nice. either way, i consider this a nit. > + if (err) { > + gt_err(gt, "failed to allocate vma for GSC version query\n"); > + return err; > + } alan:snip > + > int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc) > { > struct intel_gt *gt = gsc_uc_to_gt(gsc); > @@ -327,11 +406,21 @@ int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc) > if (err) > goto fail; > > + err = gsc_fw_query_compatibility_version(gsc); > + if (err) > + goto fail; > + > + /* we only support compatibility version 1.0 at the moment */ > + err = intel_uc_check_file_version(gsc_fw, NULL); > + 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); > > - gt_info(gt, "Loaded GSC firmware %s (r%u.%u.%u.%u, svn%u)\n", > + gt_info(gt, "Loaded GSC firmware %s (cv%u.%u, r%u.%u.%u.%u, svn %u)\n", alan:nit "abi" instead of "cv"? > gsc_fw->file_selected.path, > + gsc_fw->file_selected.ver.major, gsc_fw->file_selected.ver.minor, > gsc->release.major, gsc->release.minor, > gsc->release.patch, gsc->release.build, > gsc->security_version); > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h > index 8f199d5f963e..fb1453ed4ecf 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_heci_cmd_submit.h alan:snip > 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 cd8fc194f7fa..36ee96c02d74 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c alan:snip > 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 279244744d43..4406e7b48b27 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h alan:snip ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Intel-gfx] [PATCH 5/6] drm/i915/uc/gsc: define gsc fw 2023-05-05 16:04 [Intel-gfx] [PATCH 0/6] drm/i915: GSC FW support for MTL Daniele Ceraolo Spurio ` (3 preceding siblings ...) 2023-05-05 16:04 ` [Intel-gfx] [PATCH 4/6] drm/i915/uc/gsc: query the GSC FW for its compatibility version Daniele Ceraolo Spurio @ 2023-05-05 16:04 ` Daniele Ceraolo Spurio 2023-05-25 23:48 ` Teres Alexis, Alan Previn 2023-05-05 16:04 ` [Intel-gfx] [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs Daniele Ceraolo Spurio 2023-05-05 21:02 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915: GSC FW support for MTL Patchwork 6 siblings, 1 reply; 23+ messages in thread From: Daniele Ceraolo Spurio @ 2023-05-05 16:04 UTC (permalink / raw) To: intel-gfx; +Cc: Alan Previn, dri-devel Add FW definition and the matching override modparam. The GSC FW has both a release version, based on platform and a rolling counter, and a compatibility version, which is the one tracking interface changes. Since what we care about is the interface, we use the compatibility version in the buinary names. Same as with the GuC, a major version bump indicate a backward-incompatible change, while a minor version bump indicates a backward-compatible one, so we use only the former in the file name. 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 | 32 ++++++++++++++++++------ 1 file changed, 24 insertions(+), 8 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 36ee96c02d74..531cd172151d 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c @@ -124,6 +124,18 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw, fw_def(BROXTON, 0, huc_mmp(bxt, 2, 0, 0)) \ fw_def(SKYLAKE, 0, huc_mmp(skl, 2, 0, 0)) +/* + * The GSC FW has both a release version, based on platform and a rolling + * counter, and a compatibility version, which is the one tracking + * interface changes. Since what we care about is the interface, we use + * the compatibility version in the buinary names. + * Same as with the GuC, a major version bump indicate a + * backward-incompatible change, while a minor version bump indicates a + * backward-compatible one, so we use only the former in the file name. + */ +#define INTEL_GSC_FIRMWARE_DEFS(fw_def, gsc_def) \ + fw_def(METEORLAKE, 0, gsc_def(mtl, 1, 0)) + /* * Set of macros for producing a list of filenames from the above table. */ @@ -159,6 +171,9 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw, #define MAKE_HUC_FW_PATH_MMP(prefix_, major_, minor_, patch_) \ __MAKE_UC_FW_PATH_MMP(prefix_, "huc", major_, minor_, patch_) +#define MAKE_GSC_FW_PATH(prefix_, major_, minor_) \ + __MAKE_UC_FW_PATH_MAJOR(prefix_, "gsc", major_) + /* * All blobs need to be declared via MODULE_FIRMWARE(). * This first expansion of the table macros is solely to provide @@ -169,6 +184,7 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw, INTEL_GUC_FIRMWARE_DEFS(INTEL_UC_MODULE_FW, MAKE_GUC_FW_PATH_MAJOR, MAKE_GUC_FW_PATH_MMP) INTEL_HUC_FIRMWARE_DEFS(INTEL_UC_MODULE_FW, MAKE_HUC_FW_PATH_BLANK, MAKE_HUC_FW_PATH_MMP, MAKE_HUC_FW_PATH_GSC) +INTEL_GSC_FIRMWARE_DEFS(INTEL_UC_MODULE_FW, MAKE_GSC_FW_PATH) /* * The next expansion of the table macros (in __uc_fw_auto_select below) provides @@ -218,6 +234,10 @@ struct __packed uc_fw_blob { #define HUC_FW_BLOB_GSC(prefix_) \ UC_FW_BLOB_NEW(0, 0, 0, true, MAKE_HUC_FW_PATH_GSC(prefix_)) +#define GSC_FW_BLOB(prefix_, major_, minor_) \ + UC_FW_BLOB_NEW(major_, minor_, 0, true, \ + MAKE_GSC_FW_PATH(prefix_, major_, minor_)) + struct __packed uc_fw_platform_requirement { enum intel_platform p; u8 rev; /* first platform rev using this FW */ @@ -245,9 +265,13 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) static const struct uc_fw_platform_requirement blobs_huc[] = { INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC) }; + static const struct uc_fw_platform_requirement blobs_gsc[] = { + INTEL_GSC_FIRMWARE_DEFS(MAKE_FW_LIST, GSC_FW_BLOB) + }; static const struct fw_blobs_by_type blobs_all[INTEL_UC_FW_NUM_TYPES] = { [INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) }, [INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) }, + [INTEL_UC_FW_TYPE_GSC] = { blobs_gsc, ARRAY_SIZE(blobs_gsc) }, }; static bool verified[INTEL_UC_FW_NUM_TYPES]; const struct uc_fw_platform_requirement *fw_blobs; @@ -257,14 +281,6 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) int i; bool found; - /* - * GSC FW support is still not fully in place, so we're not defining - * the FW blob yet because we don't want the driver to attempt to load - * it until we're ready for it. - */ - if (uc_fw->type == INTEL_UC_FW_TYPE_GSC) - return; - /* * The only difference between the ADL GuC FWs is the HWConfig support. * ADL-N does not support HWConfig, so we should use the same binary as -- 2.40.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 5/6] drm/i915/uc/gsc: define gsc fw 2023-05-05 16:04 ` [Intel-gfx] [PATCH 5/6] drm/i915/uc/gsc: define gsc fw Daniele Ceraolo Spurio @ 2023-05-25 23:48 ` Teres Alexis, Alan Previn 2023-06-01 0:32 ` Ceraolo Spurio, Daniele 0 siblings, 1 reply; 23+ messages in thread From: Teres Alexis, Alan Previn @ 2023-05-25 23:48 UTC (permalink / raw) To: Ceraolo Spurio, Daniele, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org Considering the only request i have below is touching up of existing comments (as far as this patch is concerned), and since the rest of the code looks good, here is my R-b - but i hope you can anwser my newbie question at the bottom: Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com> On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: > Add FW definition and the matching override modparam. > > The GSC FW has both a release version, based on platform and a rolling > counter, and a compatibility version, which is the one tracking > interface changes. Since what we care about is the interface, we use > the compatibility version in the buinary names. alan :s/buinary/binary > > Same as with the GuC, a major version bump indicate a > backward-incompatible change, while a minor version bump indicates a > backward-compatible one, so we use only the former in the file name. > > 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 | 32 ++++++++++++++++++------ > 1 file changed, 24 insertions(+), 8 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 36ee96c02d74..531cd172151d 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c > @@ -124,6 +124,18 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw, > fw_def(BROXTON, 0, huc_mmp(bxt, 2, 0, 0)) \ > fw_def(SKYLAKE, 0, huc_mmp(skl, 2, 0, 0)) > > +/* > + * The GSC FW has both a release version, based on platform and a rolling > + * counter, and a compatibility version, which is the one tracking > + * interface changes. Since what we care about is the interface, we use > + * the compatibility version in the buinary names. alan:s/buinary/binary also, since we will (i hope) be adding several comments (alongside the new version objects under intel_gsc_uc structure) in the patch #3 about what their differences are and which one we care about and when they get populated, perhaps we can minimize the information here and redirect to that other comment... OR ... we can minimize the comments in patch #3 and redirect here (will be good to have a single location with detailed explaination in the comments and a redirect-ptr from the other location since a reader would most likely stumble onto those questions from either of these locations). > + * Same as with the GuC, a major version bump indicate a > + * backward-incompatible change, while a minor version bump indicates a > + * backward-compatible one, so we use only the former in the file name. > + */ > +#define INTEL_GSC_FIRMWARE_DEFS(fw_def, gsc_def) \ > + fw_def(METEORLAKE, 0, gsc_def(mtl, 1, 0)) > + > /* > > alan:snip > @@ -257,14 +281,6 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) > int i; > bool found; > > - /* > - * GSC FW support is still not fully in place, so we're not defining > - * the FW blob yet because we don't want the driver to attempt to load > - * it until we're ready for it. > - */ > - if (uc_fw->type == INTEL_UC_FW_TYPE_GSC) > - return; > - alan: more of a newbie question from myself: considering this is a new firmware we are adding, is there some kind of requirement to provide a link to the patch targetting the linux firmware repo that is a dependency of this series? or perhaps we should mention in the series that merge will only happen after that patch gets merged (with a final rev that includes the patch on the fw-repo side?). Just trying to understand the process. > /* > * 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 5/6] drm/i915/uc/gsc: define gsc fw 2023-05-25 23:48 ` Teres Alexis, Alan Previn @ 2023-06-01 0:32 ` Ceraolo Spurio, Daniele 0 siblings, 0 replies; 23+ messages in thread From: Ceraolo Spurio, Daniele @ 2023-06-01 0:32 UTC (permalink / raw) To: Teres Alexis, Alan Previn, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On 5/25/2023 4:48 PM, Teres Alexis, Alan Previn wrote: > Considering the only request i have below is touching up of existing comments (as > far as this patch is concerned), and since the rest of the code looks good, here is > my R-b - but i hope you can anwser my newbie question at the bottom: > > Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com> > > On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: >> Add FW definition and the matching override modparam. >> >> The GSC FW has both a release version, based on platform and a rolling >> counter, and a compatibility version, which is the one tracking >> interface changes. Since what we care about is the interface, we use >> the compatibility version in the buinary names. > alan :s/buinary/binary > >> Same as with the GuC, a major version bump indicate a >> backward-incompatible change, while a minor version bump indicates a >> backward-compatible one, so we use only the former in the file name. >> >> 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 | 32 ++++++++++++++++++------ >> 1 file changed, 24 insertions(+), 8 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 36ee96c02d74..531cd172151d 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c >> @@ -124,6 +124,18 @@ void intel_uc_fw_change_status(struct intel_uc_fw *uc_fw, >> fw_def(BROXTON, 0, huc_mmp(bxt, 2, 0, 0)) \ >> fw_def(SKYLAKE, 0, huc_mmp(skl, 2, 0, 0)) >> >> +/* >> + * The GSC FW has both a release version, based on platform and a rolling >> + * counter, and a compatibility version, which is the one tracking >> + * interface changes. Since what we care about is the interface, we use >> + * the compatibility version in the buinary names. > alan:s/buinary/binary > also, since we will (i hope) be adding several comments (alongside the new > version objects under intel_gsc_uc structure) in the patch #3 about what > their differences are and which one we care about and when they get populated, > perhaps we can minimize the information here and redirect to that other > comment... OR ... we can minimize the comments in patch #3 and redirect here > (will be good to have a single location with detailed explaination in the > comments and a redirect-ptr from the other location since a reader would > most likely stumble onto those questions from either of these locations). I'll redirect, that makes more sense > >> + * Same as with the GuC, a major version bump indicate a >> + * backward-incompatible change, while a minor version bump indicates a >> + * backward-compatible one, so we use only the former in the file name. >> + */ >> +#define INTEL_GSC_FIRMWARE_DEFS(fw_def, gsc_def) \ >> + fw_def(METEORLAKE, 0, gsc_def(mtl, 1, 0)) >> + >> /* >> >> > alan:snip > >> @@ -257,14 +281,6 @@ __uc_fw_auto_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw) >> int i; >> bool found; >> >> - /* >> - * GSC FW support is still not fully in place, so we're not defining >> - * the FW blob yet because we don't want the driver to attempt to load >> - * it until we're ready for it. >> - */ >> - if (uc_fw->type == INTEL_UC_FW_TYPE_GSC) >> - return; >> - > alan: more of a newbie question from myself: considering this is a new firmware > we are adding, is there some kind of requirement to provide a link to the patch > targetting the linux firmware repo that is a dependency of this series? > or perhaps we should mention in the series that merge will only happen after > that patch gets merged (with a final rev that includes the patch on > the fw-repo side?). Just trying to understand the process. We usually merge the kernel patch first and do the pull-request to linux-firmware immediately after. We did have cases where we change firmware version between the first rev and the one that gets merged, so we only go to linux-firmware once we're sure it's not going to change anymore. Case in point, the GSC team has asked me to hold off in sending the current blob to linux-firmware because they have some pending fixes they want to include in the first "official" release. Daniele > > >> /* >> * 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Intel-gfx] [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs 2023-05-05 16:04 [Intel-gfx] [PATCH 0/6] drm/i915: GSC FW support for MTL Daniele Ceraolo Spurio ` (4 preceding siblings ...) 2023-05-05 16:04 ` [Intel-gfx] [PATCH 5/6] drm/i915/uc/gsc: define gsc fw Daniele Ceraolo Spurio @ 2023-05-05 16:04 ` Daniele Ceraolo Spurio 2023-05-26 22:57 ` Teres Alexis, Alan Previn 2023-05-05 21:02 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915: GSC FW support for MTL Patchwork 6 siblings, 1 reply; 23+ messages in thread From: Daniele Ceraolo Spurio @ 2023-05-05 16:04 UTC (permalink / raw) To: intel-gfx; +Cc: Alan Previn, dri-devel Add a new debugfs to dump information about the GSC. This includes: - the FW path and SW tracking status; - the release, security and compatibility versions; - the HECI1 status registers. Note that those are the same registers that the mei driver dumps in their own status sysfs on DG2 (where mei owns the GSC). To make it simpler to loop through the status register, the code has been update to use a PICK macro and the existing code using the regs had been adapted to match. 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/uc/intel_gsc_fw.c | 29 +++++------- drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 46 ++++++++++++++++++- drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h | 2 + .../gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c | 38 +++++++++++++++ .../gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h | 14 ++++++ drivers/gpu/drm/i915/gt/uc/intel_huc.c | 6 +-- drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c | 2 + drivers/gpu/drm/i915/i915_reg.h | 26 ++++++++++- 9 files changed, 142 insertions(+), 24 deletions(-) create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c create mode 100644 drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index d97d45ae1a0d..fd6a1ae542bd 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -196,7 +196,8 @@ i915-y += \ gt/uc/intel_gsc_fw.o \ gt/uc/intel_gsc_proxy.o \ gt/uc/intel_gsc_uc.o \ - gt/uc/intel_gsc_uc_heci_cmd_submit.o\ + gt/uc/intel_gsc_uc_debugfs.o \ + gt/uc/intel_gsc_uc_heci_cmd_submit.o \ gt/uc/intel_guc.o \ gt/uc/intel_guc_ads.o \ gt/uc/intel_guc_capture.o \ diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c index 0b6dcd982b14..3014e982aab2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c @@ -12,36 +12,31 @@ #include "intel_gsc_fw.h" #include "intel_gsc_meu_headers.h" #include "intel_gsc_uc_heci_cmd_submit.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_PROXY_STATE_NORMAL 5 -#define GSC_FW_INIT_COMPLETE_BIT REG_BIT(9) +#include "i915_reg.h" static bool gsc_is_in_reset(struct intel_uncore *uncore) { - u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG); + u32 fw_status = intel_uncore_read(uncore, HECI_FWSTS(MTL_GSC_HECI1_BASE, 1)); - return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) == - GSC_FW_CURRENT_STATE_RESET; + return REG_FIELD_GET(HECI1_FWSTS1_CURRENT_STATE, fw_status) == + HECI1_FWSTS1_CURRENT_STATE_RESET; } bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc) { struct intel_uncore *uncore = gsc_uc_to_gt(gsc)->uncore; - u32 fw_status = intel_uncore_read(uncore, GSC_FW_STATUS_REG); + u32 fw_status = intel_uncore_read(uncore, HECI_FWSTS(MTL_GSC_HECI1_BASE, 1)); - return REG_FIELD_GET(GSC_FW_CURRENT_STATE, fw_status) == - GSC_FW_PROXY_STATE_NORMAL; + return REG_FIELD_GET(HECI1_FWSTS1_CURRENT_STATE, fw_status) == + HECI1_FWSTS1_PROXY_STATE_NORMAL; } 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); + u32 fw_status = intel_uncore_read(uncore, HECI_FWSTS(MTL_GSC_HECI1_BASE, 1)); - return fw_status & GSC_FW_INIT_COMPLETE_BIT; + return fw_status & HECI1_FWSTS1_INIT_COMPLETE; } static inline u32 cpd_entry_offset(const struct intel_gsc_cpd_entry *entry) @@ -265,9 +260,9 @@ static int gsc_fw_load_prepare(struct intel_gsc_uc *gsc) 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, + HECI_FWSTS(MTL_GSC_HECI1_BASE, 1), + HECI1_FWSTS1_INIT_COMPLETE, + HECI1_FWSTS1_INIT_COMPLETE, 500); } 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 2ae693b01b49..5475e95d61c6 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c @@ -9,8 +9,9 @@ #include "gt/intel_gt_print.h" #include "intel_gsc_uc.h" #include "intel_gsc_fw.h" -#include "i915_drv.h" #include "intel_gsc_proxy.h" +#include "i915_drv.h" +#include "i915_reg.h" static void gsc_work(struct work_struct *work) { @@ -301,3 +302,46 @@ void intel_gsc_uc_load_start(struct intel_gsc_uc *gsc) queue_work(gsc->wq, &gsc->work); } + +void intel_gsc_uc_load_status(struct intel_gsc_uc *gsc, struct drm_printer *p) +{ + struct intel_gt *gt = gsc_uc_to_gt(gsc); + struct intel_uncore *uncore = gt->uncore; + intel_wakeref_t wakeref; + + if (!intel_gsc_uc_is_supported(gsc)) { + drm_printf(p, "GSC not supported\n"); + return; + } + + if (!intel_gsc_uc_is_wanted(gsc)) { + drm_printf(p, "GSC disabled\n"); + return; + } + + drm_printf(p, "GSC firmware: %s\n", gsc->fw.file_selected.path); + if (gsc->fw.file_selected.path != gsc->fw.file_wanted.path) + drm_printf(p, "GSC firmware wanted: %s\n", gsc->fw.file_wanted.path); + drm_printf(p, "\tstatus: %s\n", intel_uc_fw_status_repr(gsc->fw.status)); + + + drm_printf(p, "Release: %u.%u.%u.%u\n", + gsc->release.major, gsc->release.minor, + gsc->release.patch, gsc->release.build); + + drm_printf(p, "Compatibility Version: %u.%u [min expected %u.%u]\n", + gsc->fw.file_selected.ver.major, gsc->fw.file_selected.ver.minor, + gsc->fw.file_wanted.ver.major, gsc->fw.file_wanted.ver.minor); + + drm_printf(p, "SVN: %u\n", gsc->security_version); + + with_intel_runtime_pm(uncore->rpm, wakeref) { + u32 i; + + for (i = 1; i <= 6; i++) { + u32 status = intel_uncore_read(uncore, + HECI_FWSTS(MTL_GSC_HECI1_BASE, i)); + drm_printf(p, "HECI1 FWSTST%u = 0x%08x\n", i, status); + } + } +} 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 d778a01f8c05..f3d30e48e5e2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h @@ -8,6 +8,7 @@ #include "intel_uc_fw.h" +struct drm_printer; struct i915_vma; struct intel_context; struct i915_gsc_proxy_component; @@ -48,6 +49,7 @@ void intel_gsc_uc_suspend(struct intel_gsc_uc *gsc); void intel_gsc_uc_resume(struct intel_gsc_uc *gsc); void intel_gsc_uc_flush_work(struct intel_gsc_uc *gsc); void intel_gsc_uc_load_start(struct intel_gsc_uc *gsc); +void intel_gsc_uc_load_status(struct intel_gsc_uc *gsc, struct drm_printer *p); static inline bool intel_gsc_uc_is_supported(struct intel_gsc_uc *gsc) { diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c new file mode 100644 index 000000000000..da9f96b72291 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c @@ -0,0 +1,38 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2020 Intel Corporation + */ + +#include <drm/drm_print.h> + +#include "gt/intel_gt.h" +#include "gt/intel_gt_debugfs.h" +#include "gt/intel_gt_print.h" +#include "intel_gsc_uc.h" +#include "i915_drv.h" + +static int gsc_info_show(struct seq_file *m, void *data) +{ + struct drm_printer p = drm_seq_file_printer(m); + struct intel_gsc_uc *gsc = m->private; + + if (!intel_gsc_uc_is_supported(gsc)) + return -ENODEV; + + intel_gsc_uc_load_status(gsc, &p); + + return 0; +} +DEFINE_INTEL_GT_DEBUGFS_ATTRIBUTE(gsc_info); + +void intel_gsc_uc_debugfs_register(struct intel_gsc_uc *gsc_uc, struct dentry *root) +{ + static const struct intel_gt_debugfs_file files[] = { + { "gsc_info", &gsc_info_fops, NULL }, + }; + + if (!intel_gsc_uc_is_supported(gsc_uc)) + return; + + intel_gt_debugfs_register_files(root, files, ARRAY_SIZE(files), gsc_uc); +} diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h new file mode 100644 index 000000000000..c405e5574253 --- /dev/null +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2020 Intel Corporation + */ + +#ifndef DEBUGFS_GSC_UC_H +#define DEBUGFS_GSC_UC_H + +struct intel_gsc_uc; +struct dentry; + +void intel_gsc_uc_debugfs_register(struct intel_gsc_uc *gsc, struct dentry *root); + +#endif /* DEBUGFS_GSC_UC_H */ diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c index 54411ac33f35..04b48433954a 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c @@ -299,9 +299,9 @@ void intel_huc_init_early(struct intel_huc *huc) huc->status[INTEL_HUC_AUTH_BY_GSC].mask = HUC_LOAD_SUCCESSFUL; huc->status[INTEL_HUC_AUTH_BY_GSC].value = HUC_LOAD_SUCCESSFUL; } else { - huc->status[INTEL_HUC_AUTH_BY_GSC].reg = HECI_FWSTS5(MTL_GSC_HECI1_BASE); - huc->status[INTEL_HUC_AUTH_BY_GSC].mask = HECI_FWSTS5_HUC_AUTH_DONE; - huc->status[INTEL_HUC_AUTH_BY_GSC].value = HECI_FWSTS5_HUC_AUTH_DONE; + huc->status[INTEL_HUC_AUTH_BY_GSC].reg = HECI_FWSTS(MTL_GSC_HECI1_BASE, 5); + huc->status[INTEL_HUC_AUTH_BY_GSC].mask = HECI1_FWSTS5_HUC_AUTH_DONE; + huc->status[INTEL_HUC_AUTH_BY_GSC].value = HECI1_FWSTS5_HUC_AUTH_DONE; } } diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c index 2f93cc4e408a..6d541c866edb 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c @@ -10,6 +10,7 @@ #include "gt/intel_gt_debugfs.h" #include "intel_guc_debugfs.h" +#include "intel_gsc_uc_debugfs.h" #include "intel_huc_debugfs.h" #include "intel_uc.h" #include "intel_uc_debugfs.h" @@ -58,6 +59,7 @@ void intel_uc_debugfs_register(struct intel_uc *uc, struct dentry *gt_root) intel_gt_debugfs_register_files(root, files, ARRAY_SIZE(files), uc); + intel_gsc_uc_debugfs_register(&uc->gsc, root); intel_guc_debugfs_register(&uc->guc, root); intel_huc_debugfs_register(&uc->huc, root); } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 94baadce29f6..cf34b777a63c 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -941,8 +941,30 @@ #define HECI_H_GS1(base) _MMIO((base) + 0xc4c) #define HECI_H_GS1_ER_PREP REG_BIT(0) -#define HECI_FWSTS5(base) _MMIO(base + 0xc68) -#define HECI_FWSTS5_HUC_AUTH_DONE (1 << 19) +/* + * The FWSTS register values are FW defined and can be different between + * HECI1 and HECI2 + */ +#define HECI_FWSTS1 0xc40 +#define HECI1_FWSTS1_CURRENT_STATE REG_GENMASK(3, 0) +#define HECI1_FWSTS1_CURRENT_STATE_RESET 0 +#define HECI1_FWSTS1_PROXY_STATE_NORMAL 5 +#define HECI1_FWSTS1_INIT_COMPLETE REG_BIT(9) +#define HECI_FWSTS2 0xc48 +#define HECI_FWSTS3 0xc60 +#define HECI_FWSTS4 0xc64 +#define HECI_FWSTS5 0xc68 +#define HECI1_FWSTS5_HUC_AUTH_DONE (1 << 19) +#define HECI_FWSTS6 0xc6c + +/* the FWSTS regs are 1-based, so we use -base for index 0 to get an invalid reg */ +#define HECI_FWSTS(base, x) _MMIO((base) + _PICK(x, -(base), \ + HECI_FWSTS1, \ + HECI_FWSTS2, \ + HECI_FWSTS3, \ + HECI_FWSTS4, \ + HECI_FWSTS5, \ + HECI_FWSTS6)) #define HSW_GTT_CACHE_EN _MMIO(0x4024) #define GTT_CACHE_EN_ALL 0xF0007FFF -- 2.40.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs 2023-05-05 16:04 ` [Intel-gfx] [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs Daniele Ceraolo Spurio @ 2023-05-26 22:57 ` Teres Alexis, Alan Previn 2023-06-01 0:25 ` Ceraolo Spurio, Daniele 0 siblings, 1 reply; 23+ messages in thread From: Teres Alexis, Alan Previn @ 2023-05-26 22:57 UTC (permalink / raw) To: Ceraolo Spurio, Daniele, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: > Add a new debugfs to dump information about the GSC. This includes: alan:snip Actually everything looks good except for a couple of questions + asks - hope we can close on this patch in next rev. > > - the FW path and SW tracking status; > - the release, security and compatibility versions; > - the HECI1 status registers. > > 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 0b6dcd982b14..3014e982aab2 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c > @@ -12,36 +12,31 @@ > #include "intel_gsc_fw.h" > #include "intel_gsc_meu_headers.h" > #include "intel_gsc_uc_heci_cmd_submit.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_PROXY_STATE_NORMAL 5 > -#define GSC_FW_INIT_COMPLETE_BIT REG_BIT(9) > +#include "i915_reg.h" > alan:snip alan: btw, just to be consistent with other top-level "intel_foo_is..." checking functions, why don't we take the runtime wakeref inside the following functions and make it easier for any callers? (just like what we do for "intel_huc_is_authenticated"): static bool gsc_is_in_reset(struct intel_uncore *uncore) bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc) bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *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 > index 2ae693b01b49..5475e95d61c6 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c > @@ -9,8 +9,9 @@ > #include "gt/intel_gt_print.h" > #include "intel_gsc_uc.h" alan: nit: not this patch's fault, alphabetically, intel_gsc_uc.h is after intel_gsc_proxy.h > #include "intel_gsc_fw.h" > -#include "i915_drv.h" > #include "intel_gsc_proxy.h" > +#include "i915_drv.h" > +#include "i915_reg.h" > > static void gsc_work(struct work_struct *work) > { > @@ -301,3 +302,46 @@ void intel_gsc_uc_load_start(struct intel_gsc_uc *gsc) > queue_work(gsc->wq, &gsc->work); > } > + alan: btw, why are we putting intel_gsc_uc_load_status in intel_gsc_uc.c if the only caller is gsc_uc's debugfs? why not just make it a static in there? unless u plan to call it from "err_print_uc" - then can we add that in next rev? > +void intel_gsc_uc_load_status(struct intel_gsc_uc *gsc, struct drm_printer *p) > +{ > + struct intel_gt *gt = gsc_uc_to_gt(gsc); > + struct intel_uncore *uncore = gt->uncore; > + intel_wakeref_t wakeref; > + > + if (!intel_gsc_uc_is_supported(gsc)) { alan: this was already checked in caller so we'll never get here. i think we should remove the check in the caller, let below msg appear. > + drm_printf(p, "GSC not supported\n"); > + return; > + } alan:snip > + drm_printf(p, "HECI1 FWSTST%u = 0x%08x\n", i, status); alan:nit: do you we could add those additional shim regs? (seemed useful in recent offline debugs). alan:snip > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c > new file mode 100644 > index 000000000000..da9f96b72291 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c > @@ -0,0 +1,38 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2020 Intel Corporation alan:2023? alan:snip > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h > new file mode 100644 > index 000000000000..c405e5574253 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h > @@ -0,0 +1,14 @@ > +/* SPDX-License-Identifier: MIT */ > +/* > + * Copyright © 2020 Intel Corporation alan:2023? alan:snip > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c alan:snip > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c > index 2f93cc4e408a..6d541c866edb 100644 alan:snip ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs 2023-05-26 22:57 ` Teres Alexis, Alan Previn @ 2023-06-01 0:25 ` Ceraolo Spurio, Daniele 2023-06-05 23:46 ` Teres Alexis, Alan Previn 0 siblings, 1 reply; 23+ messages in thread From: Ceraolo Spurio, Daniele @ 2023-06-01 0:25 UTC (permalink / raw) To: Teres Alexis, Alan Previn, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On 5/26/2023 3:57 PM, Teres Alexis, Alan Previn wrote: > On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: >> Add a new debugfs to dump information about the GSC. This includes: > alan:snip > Actually everything looks good except for a couple of questions + asks - hope we can close on this patch in next rev. > >> - the FW path and SW tracking status; >> - the release, security and compatibility versions; >> - the HECI1 status registers. >> >> 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 0b6dcd982b14..3014e982aab2 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c >> @@ -12,36 +12,31 @@ >> #include "intel_gsc_fw.h" >> #include "intel_gsc_meu_headers.h" >> #include "intel_gsc_uc_heci_cmd_submit.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_PROXY_STATE_NORMAL 5 >> -#define GSC_FW_INIT_COMPLETE_BIT REG_BIT(9) >> +#include "i915_reg.h" >> > alan:snip > > alan: btw, just to be consistent with other top-level "intel_foo_is..." checking functions, > why don't we take the runtime wakeref inside the following functions and make it easier for any callers? > (just like what we do for "intel_huc_is_authenticated"): > static bool gsc_is_in_reset(struct intel_uncore *uncore) > bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc) > bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc) The idea was that we shouldn't check the FW status if we're not planning to do something with it, in which case we should already have a wakeref. HuC is a special case because userspace can query that when the HW is idle. This said, I have nothing against adding an extra wakeref , but I don't think it should be in this patch. > > >> 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 2ae693b01b49..5475e95d61c6 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c >> @@ -9,8 +9,9 @@ >> #include "gt/intel_gt_print.h" >> #include "intel_gsc_uc.h" > alan: nit: not this patch's fault, alphabetically, intel_gsc_uc.h is after intel_gsc_proxy.h will fix >> #include "intel_gsc_fw.h" >> -#include "i915_drv.h" >> #include "intel_gsc_proxy.h" >> +#include "i915_drv.h" >> +#include "i915_reg.h" >> >> static void gsc_work(struct work_struct *work) >> { >> @@ -301,3 +302,46 @@ void intel_gsc_uc_load_start(struct intel_gsc_uc *gsc) >> queue_work(gsc->wq, &gsc->work); >> } >> + > alan: btw, why are we putting intel_gsc_uc_load_status in intel_gsc_uc.c if the only caller is gsc_uc's debugfs? > why not just make it a static in there? unless u plan to call it from "err_print_uc" - then can we add that in next rev? I do indeed plan to follow up and add this to the error state, but I'll do that as a separate patch as I also want to add the GSC logs to the error state at the same time. > >> +void intel_gsc_uc_load_status(struct intel_gsc_uc *gsc, struct drm_printer *p) >> +{ >> + struct intel_gt *gt = gsc_uc_to_gt(gsc); >> + struct intel_uncore *uncore = gt->uncore; >> + intel_wakeref_t wakeref; >> + >> + if (!intel_gsc_uc_is_supported(gsc)) { > alan: this was already checked in caller so we'll never get here. i think we should remove the check in the caller, let below msg appear. I did the same as what we do for GuC and HuC. I'd prefer to be consistent in behavior with those. >> + drm_printf(p, "GSC not supported\n"); >> + return; >> + } > alan:snip > > > >> + drm_printf(p, "HECI1 FWSTST%u = 0x%08x\n", i, status); > alan:nit: do you we could add those additional shim regs? (seemed useful in recent offline debugs). Agreed that it would be useful; I'll try to get a complete list from arch and/or the GSC FW team. Are you ok if we go ahead with this in the meantime? > alan:snip > >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c >> new file mode 100644 >> index 000000000000..da9f96b72291 >> --- /dev/null >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.c >> @@ -0,0 +1,38 @@ >> +// SPDX-License-Identifier: MIT >> +/* >> + * Copyright © 2020 Intel Corporation > alan:2023? D'oh! Daniele > > alan:snip > >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h >> new file mode 100644 >> index 000000000000..c405e5574253 >> --- /dev/null >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc_debugfs.h >> @@ -0,0 +1,14 @@ >> +/* SPDX-License-Identifier: MIT */ >> +/* >> + * Copyright © 2020 Intel Corporation > alan:2023? > alan:snip > >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c > alan:snip > > >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c >> index 2f93cc4e408a..6d541c866edb 100644 > alan:snip > > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs 2023-06-01 0:25 ` Ceraolo Spurio, Daniele @ 2023-06-05 23:46 ` Teres Alexis, Alan Previn 2023-06-05 23:53 ` Ceraolo Spurio, Daniele 0 siblings, 1 reply; 23+ messages in thread From: Teres Alexis, Alan Previn @ 2023-06-05 23:46 UTC (permalink / raw) To: Ceraolo Spurio, Daniele, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On Wed, 2023-05-31 at 17:25 -0700, Ceraolo Spurio, Daniele wrote: > > On 5/26/2023 3:57 PM, Teres Alexis, Alan Previn wrote: > > On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: > > > Add a new debugfs to dump information about the GSC. This includes: > > alan:snip > > Actually everything looks good except for a couple of questions + asks - hope we can close on this patch in next rev. > > > > > - the FW path and SW tracking status; > > > - the release, security and compatibility versions; > > > - the HECI1 status registers. > > > > > > 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 0b6dcd982b14..3014e982aab2 100644 > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c > > > @@ -12,36 +12,31 @@ > > > #include "intel_gsc_fw.h" > > > #include "intel_gsc_meu_headers.h" > > > #include "intel_gsc_uc_heci_cmd_submit.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_PROXY_STATE_NORMAL 5 > > > -#define GSC_FW_INIT_COMPLETE_BIT REG_BIT(9) > > > +#include "i915_reg.h" > > > > > alan:snip > > > > alan: btw, just to be consistent with other top-level "intel_foo_is..." checking functions, > > why don't we take the runtime wakeref inside the following functions and make it easier for any callers? > > (just like what we do for "intel_huc_is_authenticated"): > > static bool gsc_is_in_reset(struct intel_uncore *uncore) > > bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc) > > bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc) > > The idea was that we shouldn't check the FW status if we're not planning > to do something with it, in which case we should already have a wakeref. > HuC is a special case because userspace can query that when the HW is > idle. This said, I have nothing against adding an extra wakeref , but I > don't think it should be in this patch. alan: i believe intel_pxp_gsccs_is_ready_for_sessions is being used in a similar way where one of the uses it to check huc-status and gsc-proxy status without actually doing any operation. If u still wanna keep it differently then I'll have to update the PXP code. Or perhaps you could help me fix that on the PXP side? alna:snip > > > > > +void intel_gsc_uc_load_status(struct intel_gsc_uc *gsc, struct drm_printer *p) > > > +{ > > > + struct intel_gt *gt = gsc_uc_to_gt(gsc); > > > + struct intel_uncore *uncore = gt->uncore; > > > + intel_wakeref_t wakeref; > > > + > > > + if (!intel_gsc_uc_is_supported(gsc)) { > > alan: this was already checked in caller so we'll never get here. i think we should remove the check in the caller, let below msg appear. > > I did the same as what we do for GuC and HuC. I'd prefer to be > consistent in behavior with those. alan: okay - sounds good > > > > + drm_printf(p, "GSC not supported\n"); > > > + return; > > > + } > > alan:snip > > > + drm_printf(p, "HECI1 FWSTST%u = 0x%08x\n", i, status); > > alan:nit: do you we could add those additional shim regs? (seemed useful in recent offline debugs). > > Agreed that it would be useful; I'll try to get a complete list from > arch and/or the GSC FW team. Are you ok if we go ahead with this in the > meantime? alan: yes sure. > > > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs 2023-06-05 23:46 ` Teres Alexis, Alan Previn @ 2023-06-05 23:53 ` Ceraolo Spurio, Daniele 0 siblings, 0 replies; 23+ messages in thread From: Ceraolo Spurio, Daniele @ 2023-06-05 23:53 UTC (permalink / raw) To: Teres Alexis, Alan Previn, intel-gfx@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org On 6/5/2023 4:46 PM, Teres Alexis, Alan Previn wrote: > On Wed, 2023-05-31 at 17:25 -0700, Ceraolo Spurio, Daniele wrote: >> On 5/26/2023 3:57 PM, Teres Alexis, Alan Previn wrote: >>> On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote: >>>> Add a new debugfs to dump information about the GSC. This includes: >>> alan:snip >>> Actually everything looks good except for a couple of questions + asks - hope we can close on this patch in next rev. >>> >>>> - the FW path and SW tracking status; >>>> - the release, security and compatibility versions; >>>> - the HECI1 status registers. >>>> >>>> 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 0b6dcd982b14..3014e982aab2 100644 >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c >>>> @@ -12,36 +12,31 @@ >>>> #include "intel_gsc_fw.h" >>>> #include "intel_gsc_meu_headers.h" >>>> #include "intel_gsc_uc_heci_cmd_submit.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_PROXY_STATE_NORMAL 5 >>>> -#define GSC_FW_INIT_COMPLETE_BIT REG_BIT(9) >>>> +#include "i915_reg.h" >>>> >>> alan:snip >>> >>> alan: btw, just to be consistent with other top-level "intel_foo_is..." checking functions, >>> why don't we take the runtime wakeref inside the following functions and make it easier for any callers? >>> (just like what we do for "intel_huc_is_authenticated"): >>> static bool gsc_is_in_reset(struct intel_uncore *uncore) >>> bool intel_gsc_uc_fw_proxy_init_done(struct intel_gsc_uc *gsc) >>> bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc) >> The idea was that we shouldn't check the FW status if we're not planning >> to do something with it, in which case we should already have a wakeref. >> HuC is a special case because userspace can query that when the HW is >> idle. This said, I have nothing against adding an extra wakeref , but I >> don't think it should be in this patch. > alan: i believe intel_pxp_gsccs_is_ready_for_sessions is being used in a > similar way where one of the uses it to check huc-status and gsc-proxy > status without actually doing any operation. If u still wanna keep it > differently then I'll have to update the PXP code. Or perhaps you could > help me fix that on the PXP side? Sure, but let's take this to a separate patch. This patch is not adding that code nor any calls to it (just updating the defines), so it isn't the right place to add that fix. Daniele > > alna:snip >>>> +void intel_gsc_uc_load_status(struct intel_gsc_uc *gsc, struct drm_printer *p) >>>> +{ >>>> + struct intel_gt *gt = gsc_uc_to_gt(gsc); >>>> + struct intel_uncore *uncore = gt->uncore; >>>> + intel_wakeref_t wakeref; >>>> + >>>> + if (!intel_gsc_uc_is_supported(gsc)) { >>> alan: this was already checked in caller so we'll never get here. i think we should remove the check in the caller, let below msg appear. >> I did the same as what we do for GuC and HuC. I'd prefer to be >> consistent in behavior with those. > alan: okay - sounds good >>>> + drm_printf(p, "GSC not supported\n"); >>>> + return; >>>> + } >>> alan:snip >>>> + drm_printf(p, "HECI1 FWSTST%u = 0x%08x\n", i, status); >>> alan:nit: do you we could add those additional shim regs? (seemed useful in recent offline debugs). >> Agreed that it would be useful; I'll try to get a complete list from >> arch and/or the GSC FW team. Are you ok if we go ahead with this in the >> meantime? > alan: yes sure. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915: GSC FW support for MTL 2023-05-05 16:04 [Intel-gfx] [PATCH 0/6] drm/i915: GSC FW support for MTL Daniele Ceraolo Spurio ` (5 preceding siblings ...) 2023-05-05 16:04 ` [Intel-gfx] [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs Daniele Ceraolo Spurio @ 2023-05-05 21:02 ` Patchwork 6 siblings, 0 replies; 23+ messages in thread From: Patchwork @ 2023-05-05 21:02 UTC (permalink / raw) To: Ceraolo Spurio, Daniele; +Cc: intel-gfx == Series Details == Series: drm/i915: GSC FW support for MTL URL : https://patchwork.freedesktop.org/series/117396/ State : failure == Summary == Error: patch https://patchwork.freedesktop.org/api/1.0/series/117396/revisions/1/mbox/ not applied Applying: DO NOT REVIEW: drm/i915: HuC loading and authentication for MTL Using index info to reconstruct a base tree... M drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c Falling back to patching base and 3-way merge... Auto-merging drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c error: Failed to merge in the changes. hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 DO NOT REVIEW: drm/i915: HuC loading and authentication for MTL When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Build failed, no error log produced ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2023-06-06 0:00 UTC | newest] Thread overview: 23+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-05 16:04 [Intel-gfx] [PATCH 0/6] drm/i915: GSC FW support for MTL Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] [PATCH 1/6] DO NOT REVIEW: drm/i915: HuC loading and authentication " Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] [PATCH 2/6] drm/i915/uc/gsc: fixes and updates for GSC memory allocation Daniele Ceraolo Spurio 2023-05-23 0:13 ` Teres Alexis, Alan Previn 2023-05-23 15:21 ` Ceraolo Spurio, Daniele 2023-06-06 0:00 ` Teres Alexis, Alan Previn 2023-05-05 16:04 ` [Intel-gfx] [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary Daniele Ceraolo Spurio 2023-05-25 5:14 ` Teres Alexis, Alan Previn 2023-05-25 16:56 ` Ceraolo Spurio, Daniele 2023-05-25 22:03 ` Teres Alexis, Alan Previn 2023-05-27 1:27 ` Ceraolo Spurio, Daniele 2023-06-05 23:51 ` Teres Alexis, Alan Previn 2023-05-05 16:04 ` [Intel-gfx] [PATCH 4/6] drm/i915/uc/gsc: query the GSC FW for its compatibility version Daniele Ceraolo Spurio 2023-05-25 23:29 ` Teres Alexis, Alan Previn 2023-05-05 16:04 ` [Intel-gfx] [PATCH 5/6] drm/i915/uc/gsc: define gsc fw Daniele Ceraolo Spurio 2023-05-25 23:48 ` Teres Alexis, Alan Previn 2023-06-01 0:32 ` Ceraolo Spurio, Daniele 2023-05-05 16:04 ` [Intel-gfx] [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs Daniele Ceraolo Spurio 2023-05-26 22:57 ` Teres Alexis, Alan Previn 2023-06-01 0:25 ` Ceraolo Spurio, Daniele 2023-06-05 23:46 ` Teres Alexis, Alan Previn 2023-06-05 23:53 ` Ceraolo Spurio, Daniele 2023-05-05 21:02 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915: GSC FW support for MTL Patchwork
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox