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.
