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