From: "Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com> To: "Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>, "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org> Cc: "Harrison, John C" <john.c.harrison@intel.com>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs Date: Wed, 31 May 2023 17:25:57 -0700 [thread overview] Message-ID: <f370372e-42b9-17af-c553-475814c25807@intel.com> (raw) In-Reply-To: <63637814b7d02639de39287ec92eace9f3aff46a.camel@intel.com> 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 > > >
WARNING: multiple messages have this Message-ID (diff)
From: "Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com> To: "Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>, "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org> Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org> Subject: Re: [Intel-gfx] [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs Date: Wed, 31 May 2023 17:25:57 -0700 [thread overview] Message-ID: <f370372e-42b9-17af-c553-475814c25807@intel.com> (raw) In-Reply-To: <63637814b7d02639de39287ec92eace9f3aff46a.camel@intel.com> 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 > > >
next prev parent reply other threads:[~2023-06-01 0:26 UTC|newest] Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-05-05 16:04 [PATCH 0/6] drm/i915: GSC FW support for MTL Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] " Daniele Ceraolo Spurio 2023-05-05 16:04 ` [PATCH 1/6] DO NOT REVIEW: drm/i915: HuC loading and authentication " Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] " Daniele Ceraolo Spurio 2023-05-05 16:04 ` [PATCH 2/6] drm/i915/uc/gsc: fixes and updates for GSC memory allocation Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] " Daniele Ceraolo Spurio 2023-05-23 0:13 ` Teres Alexis, Alan Previn 2023-05-23 0:13 ` [Intel-gfx] " Teres Alexis, Alan Previn 2023-05-23 15:21 ` Ceraolo Spurio, Daniele 2023-05-23 15:21 ` [Intel-gfx] " Ceraolo Spurio, Daniele 2023-06-06 0:00 ` Teres Alexis, Alan Previn 2023-06-06 0:00 ` [Intel-gfx] " Teres Alexis, Alan Previn 2023-05-05 16:04 ` [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] " Daniele Ceraolo Spurio 2023-05-25 5:14 ` Teres Alexis, Alan Previn 2023-05-25 5:14 ` [Intel-gfx] " Teres Alexis, Alan Previn 2023-05-25 16:56 ` Ceraolo Spurio, Daniele 2023-05-25 16:56 ` [Intel-gfx] " Ceraolo Spurio, Daniele 2023-05-25 22:03 ` Teres Alexis, Alan Previn 2023-05-25 22:03 ` [Intel-gfx] " Teres Alexis, Alan Previn 2023-05-27 1:27 ` Ceraolo Spurio, Daniele 2023-05-27 1:27 ` [Intel-gfx] " Ceraolo Spurio, Daniele 2023-06-05 23:51 ` Teres Alexis, Alan Previn 2023-06-05 23:51 ` [Intel-gfx] " Teres Alexis, Alan Previn 2023-05-05 16:04 ` [PATCH 4/6] drm/i915/uc/gsc: query the GSC FW for its compatibility version Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] " Daniele Ceraolo Spurio 2023-05-25 23:29 ` Teres Alexis, Alan Previn 2023-05-25 23:29 ` [Intel-gfx] " Teres Alexis, Alan Previn 2023-05-05 16:04 ` [PATCH 5/6] drm/i915/uc/gsc: define gsc fw Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] " Daniele Ceraolo Spurio 2023-05-25 23:48 ` Teres Alexis, Alan Previn 2023-05-25 23:48 ` [Intel-gfx] " Teres Alexis, Alan Previn 2023-06-01 0:32 ` Ceraolo Spurio, Daniele 2023-06-01 0:32 ` [Intel-gfx] " Ceraolo Spurio, Daniele 2023-05-05 16:04 ` [PATCH 6/6] drm/i915/uc/gsc: Add a gsc_info debugfs Daniele Ceraolo Spurio 2023-05-05 16:04 ` [Intel-gfx] " Daniele Ceraolo Spurio 2023-05-26 22:57 ` Teres Alexis, Alan Previn 2023-05-26 22:57 ` [Intel-gfx] " Teres Alexis, Alan Previn 2023-06-01 0:25 ` Ceraolo Spurio, Daniele [this message] 2023-06-01 0:25 ` Ceraolo Spurio, Daniele 2023-06-05 23:46 ` Teres Alexis, Alan Previn 2023-06-05 23:46 ` [Intel-gfx] " Teres Alexis, Alan Previn 2023-06-05 23:53 ` Ceraolo Spurio, Daniele 2023-06-05 23:53 ` [Intel-gfx] " Ceraolo Spurio, Daniele 2023-05-05 21:02 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915: GSC FW support for MTL Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=f370372e-42b9-17af-c553-475814c25807@intel.com \ --to=daniele.ceraolospurio@intel.com \ --cc=alan.previn.teres.alexis@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=john.c.harrison@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.