From: John Harrison <john.c.harrison@intel.com>
To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>,
<intel-gfx@lists.freedesktop.org>
Cc: Matt Roper <matthew.d.roper@intel.com>,
dri-devel@lists.freedesktop.org,
Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915/gsc: implement wa 14015076503
Date: Wed, 22 Mar 2023 12:44:00 -0700 [thread overview]
Message-ID: <6763dca1-5061-f508-0846-8ef51a7acdc8@intel.com> (raw)
In-Reply-To: <20230320211039.1513368-2-daniele.ceraolospurio@intel.com>
On 3/20/2023 14:10, Daniele Ceraolo Spurio wrote:
> The WA states that we need to alert the GSC FW before doing a GSC engine
> reset and then wait for 200ms. The GuC owns engine reset, so on the i915
> side we only need to apply this for full GT reset.
>
> Given that we do full GT resets in the resume paths to cleanup the HW
> state and that a long wait in those scenarios would not be acceptable,
> a faster path has been introduced where, if the GSC is idle, we try first
> to individually reset the GuC and all engines except the GSC and only fall
> back to full reset if that fails.
I'm confused. If the code path is a resume then surely all engines are
idle. But, there is presumably a reason why we do a full GT reset on the
resume - because the hardware state is not guaranteed to be sensible at
that point. So how is it safe to skip the GSC reset?
>
> Note: according to the WA specs, if the GSC is idle it should be possible
> to only wait for the uC wakeup time (~15ms) instead of the whole 200ms.
> However, the GSC FW team have mentioned that the wakeup time can change
> based on other things going on in the HW and pcode, so a good security
> margin would be required. Given that when the GSC is idle we already
> skip the wait & reset entirely and that this reduced wait would still
> likely be too long to use in resume paths, it's not worth adding support
> for this reduced wait.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: John Harrison <john.c.harrison@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_reset.c | 77 +++++++++++++++++++++--
> drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 2 +
> drivers/gpu/drm/i915/i915_reg.h | 14 ++++-
> 3 files changed, 86 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 2c3463f77e5c..5f75f59122cf 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -14,6 +14,8 @@
>
> #include "gt/intel_gt_regs.h"
>
> +#include "gt/uc/intel_gsc_fw.h"
> +
> #include "i915_drv.h"
> #include "i915_file_private.h"
> #include "i915_gpu_error.h"
> @@ -695,6 +697,67 @@ static reset_func intel_get_gpu_reset(const struct intel_gt *gt)
> return NULL;
> }
>
> +static int __reset_guc(struct intel_gt *gt)
> +{
> + u32 guc_domain =
> + GRAPHICS_VER(gt->i915) >= 11 ? GEN11_GRDOM_GUC : GEN9_GRDOM_GUC;
> +
> + return gen6_hw_domain_reset(gt, guc_domain);
> +}
> +
> +static bool needs_wa_14015076503(struct intel_gt *gt, intel_engine_mask_t engine_mask)
> +{
> + if (!IS_METEORLAKE(gt->i915) || !HAS_ENGINE(gt, GSC0))
> + return false;
> +
> + if (!__HAS_ENGINE(engine_mask, GSC0))
> + return false;
> +
> + return intel_gsc_uc_fw_init_done(>->uc.gsc);
> +}
> +
> +static intel_engine_mask_t
> +wa_14015076503_start(struct intel_gt *gt, intel_engine_mask_t engine_mask, bool first)
> +{
> + if (!needs_wa_14015076503(gt, engine_mask))
> + return engine_mask;
> +
> + /*
> + * wa_14015076503: if the GSC FW is loaded, we need to alert it that
> + * we're going to do a GSC engine reset and then wait for 200ms for the
> + * FW to get ready for it. However, if this the first ALL_ENGINES reset
if this is
John.
> + * attempt and the GSC is not busy, we can try to instead reset the GuC
> + * and all the other engines individually to avoid the 200ms wait.
> + */
> + if (engine_mask == ALL_ENGINES && first && intel_engine_is_idle(gt->engine[GSC0])) {
> + __reset_guc(gt);
> + engine_mask = gt->info.engine_mask & ~BIT(GSC0);
> + } else {
> + intel_uncore_rmw(gt->uncore,
> + HECI_H_GS1(MTL_GSC_HECI2_BASE),
> + 0, HECI_H_GS1_ER_PREP);
> +
> + /* make sure the reset bit is clear when writing the CSR reg */
> + intel_uncore_rmw(gt->uncore,
> + HECI_H_CSR(MTL_GSC_HECI2_BASE),
> + HECI_H_CSR_RST, HECI_H_CSR_IG);
> + msleep(200);
> + }
> +
> + return engine_mask;
> +}
> +
> +static void
> +wa_14015076503_end(struct intel_gt *gt, intel_engine_mask_t engine_mask)
> +{
> + if (!needs_wa_14015076503(gt, engine_mask))
> + return;
> +
> + intel_uncore_rmw(gt->uncore,
> + HECI_H_GS1(MTL_GSC_HECI2_BASE),
> + HECI_H_GS1_ER_PREP, 0);
> +}
> +
> int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask)
> {
> const int retries = engine_mask == ALL_ENGINES ? RESET_MAX_RETRIES : 1;
> @@ -712,10 +775,16 @@ int __intel_gt_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask)
> */
> intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_ALL);
> for (retry = 0; ret == -ETIMEDOUT && retry < retries; retry++) {
> - GT_TRACE(gt, "engine_mask=%x\n", engine_mask);
> + intel_engine_mask_t reset_mask;
> +
> + reset_mask = wa_14015076503_start(gt, engine_mask, !retry);
> +
> + GT_TRACE(gt, "engine_mask=%x\n", reset_mask);
> preempt_disable();
> - ret = reset(gt, engine_mask, retry);
> + ret = reset(gt, reset_mask, retry);
> preempt_enable();
> +
> + wa_14015076503_end(gt, reset_mask);
> }
> intel_uncore_forcewake_put(gt->uncore, FORCEWAKE_ALL);
>
> @@ -740,14 +809,12 @@ bool intel_has_reset_engine(const struct intel_gt *gt)
>
> int intel_reset_guc(struct intel_gt *gt)
> {
> - u32 guc_domain =
> - GRAPHICS_VER(gt->i915) >= 11 ? GEN11_GRDOM_GUC : GEN9_GRDOM_GUC;
> int ret;
>
> GEM_BUG_ON(!HAS_GT_UC(gt->i915));
>
> intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_ALL);
> - ret = gen6_hw_domain_reset(gt, guc_domain);
> + ret = __reset_guc(gt);
> intel_uncore_forcewake_put(gt->uncore, FORCEWAKE_ALL);
>
> return ret;
> 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 4b5dbb44afb4..f4c1106bb2a9 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> @@ -9,7 +9,9 @@
> #include <linux/types.h>
>
> struct intel_gsc_uc;
> +struct intel_uncore;
>
> int intel_gsc_uc_fw_upload(struct intel_gsc_uc *gsc);
> bool intel_gsc_uc_fw_init_done(struct intel_gsc_uc *gsc);
> +
> #endif
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index d22ffd7a32dc..80e33ede7fac 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -928,8 +928,18 @@
> #define DG1_GSC_HECI2_BASE 0x00259000
> #define DG2_GSC_HECI1_BASE 0x00373000
> #define DG2_GSC_HECI2_BASE 0x00374000
> -
> -
> +#define MTL_GSC_HECI1_BASE 0x00116000
> +#define MTL_GSC_HECI2_BASE 0x00117000
> +
> +#define HECI_H_CSR(base) _MMIO(base + 0x4)
> +#define HECI_H_CSR_IE REG_BIT(0)
> +#define HECI_H_CSR_IS REG_BIT(1)
> +#define HECI_H_CSR_IG REG_BIT(2)
> +#define HECI_H_CSR_RDY REG_BIT(3)
> +#define HECI_H_CSR_RST REG_BIT(4)
> +
> +#define HECI_H_GS1(base) _MMIO(base + 0xc4c)
> +#define HECI_H_GS1_ER_PREP REG_BIT(0)
>
> #define HSW_GTT_CACHE_EN _MMIO(0x4024)
> #define GTT_CACHE_EN_ALL 0xF0007FFF
next prev parent reply other threads:[~2023-03-22 19:44 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-20 21:10 [Intel-gfx] [PATCH 1/2] drm/i915: limit double GT reset to pre-MTL Daniele Ceraolo Spurio
2023-03-20 21:10 ` [Intel-gfx] [PATCH 2/2] drm/i915/gsc: implement wa 14015076503 Daniele Ceraolo Spurio
2023-03-22 19:44 ` John Harrison [this message]
2023-03-22 20:59 ` Ceraolo Spurio, Daniele
2023-03-22 21:09 ` John Harrison
2023-03-20 21:41 ` [Intel-gfx] [PATCH 1/2] drm/i915: limit double GT reset to pre-MTL Andi Shyti
2023-03-21 13:33 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] " Patchwork
2023-03-21 13:33 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2023-03-21 13:33 ` [Intel-gfx] ✗ Fi.CI.DOCS: " Patchwork
2023-03-21 13:40 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2023-03-21 18:29 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2023-03-22 19:44 ` [Intel-gfx] [PATCH 1/2] " John Harrison
2023-03-22 21:03 ` Ceraolo Spurio, Daniele
2023-03-23 0:16 ` Andi Shyti
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=6763dca1-5061-f508-0846-8ef51a7acdc8@intel.com \
--to=john.c.harrison@intel.com \
--cc=daniele.ceraolospurio@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=matthew.d.roper@intel.com \
--cc=rodrigo.vivi@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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).