| Message ID | 20230505160415.889525-7-daniele.ceraolospurio@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: GSC FW support for MTL | expand |
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
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 > > >
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. > > > >
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.
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
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