From patchwork Thu Mar 23 23:18:56 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniele Ceraolo Spurio X-Patchwork-Id: 13186167 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id DC32CC74A5B for ; Thu, 23 Mar 2023 23:19:35 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 6B92310E121; Thu, 23 Mar 2023 23:19:35 +0000 (UTC) Received: from mga06.intel.com (mga06b.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 646FC10E121 for ; Thu, 23 Mar 2023 23:19:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1679613573; x=1711149573; h=from:to:subject:date:message-id:mime-version: content-transfer-encoding; bh=M/CZP/XYi88uURwJu3pOSUC/ikXcXcZ0V9dE39VjvBg=; b=Pinj9f4o1XyrPBeQpBW6sKhzoqBowG1uQ+urGe1t8XvJ6gg23B3RdcF9 umvKFh/Jones8NLqAZI/iOgKSnyc12IGPyrBnNQftoLOe5VX820q/JdJS 6mJ0tmY71yCuKTi/noqnpWsdznpqSKh0CQe0ZsDTc7p58LIoiQ8EcSe/g UwpQ++phq4N58UsYQ3lLPSyhCyROC8tyvi0exG5u7Wmlo60E62bAXXlvM tXE8lPDcSGlwDGqcLBntPOWqxoksnuv7tnsV0qrrF3tgFqQrhE/1vktMG xd90/qJHhMclwprvOFQlBzrcI1aI+GmUL4gZM+ok9ISwFVWZD91YU6c9M Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10658"; a="402232264" X-IronPort-AV: E=Sophos;i="5.98,286,1673942400"; d="scan'208";a="402232264" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Mar 2023 16:19:24 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10658"; a="715011509" X-IronPort-AV: E=Sophos;i="5.98,286,1673942400"; d="scan'208";a="715011509" Received: from valcore-skull-1.fm.intel.com ([10.1.27.19]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Mar 2023 16:19:24 -0700 From: Daniele Ceraolo Spurio To: intel-gfx@lists.freedesktop.org Date: Thu, 23 Mar 2023 16:18:56 -0700 Message-Id: <20230323231857.2194435-1-daniele.ceraolospurio@intel.com> X-Mailer: git-send-email 2.37.3 MIME-Version: 1.0 Subject: [Intel-gfx] [CI 1/2] drm/i915: limit double GT reset to pre-MTL X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" Commit 3db9d590557d ("drm/i915/gt: Reset twice") modified the code to always hit the GDRST register twice when doing a reset, with the reported aim to fix invalid post-reset engine state on some platforms (Jasperlake being the only one actually mentioned). This is a problem on MTL, due to the fact that we have to apply a time consuming WA (coming in the next patch) every time we hit the GDRST register in a way that can include the GSC engine. Even post MTL, the expectation is that we'll have some work to do before and after hitting the GDRST if the GSC is involved. Since the issue requiring the double reset seems to be limited to older platforms, instead of trying to handle the double-reset on MTL and future platforms it is just easier to turn it off. The default on MTL is also for GuC to own engine reset, with i915 only covering full-GT reset. Signed-off-by: Daniele Ceraolo Spurio Cc: Chris Wilson Cc: Andi Shyti Cc: Mika Kuoppala Cc: Gwan-gyeong Mun Cc: John Harrison Reviewed-by: Andi Shyti --- drivers/gpu/drm/i915/gt/intel_reset.c | 35 +++++++++++++++------------ 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c index 0bb9094fdacd..2c3463f77e5c 100644 --- a/drivers/gpu/drm/i915/gt/intel_reset.c +++ b/drivers/gpu/drm/i915/gt/intel_reset.c @@ -268,9 +268,27 @@ static int ilk_do_reset(struct intel_gt *gt, intel_engine_mask_t engine_mask, static int gen6_hw_domain_reset(struct intel_gt *gt, u32 hw_domain_mask) { struct intel_uncore *uncore = gt->uncore; - int loops = 2; + int loops; int err; + /* + * On some platforms, e.g. Jasperlake, we see that the engine register + * state is not cleared until shortly after GDRST reports completion, + * causing a failure as we try to immediately resume while the internal + * state is still in flux. If we immediately repeat the reset, the + * second reset appears to serialise with the first, and since it is a + * no-op, the registers should retain their reset value. However, there + * is still a concern that upon leaving the second reset, the internal + * engine state is still in flux and not ready for resuming. + * + * Starting on MTL, there are some prep steps that we need to do when + * resetting some engines that need to be applied every time we write to + * GEN6_GDRST. As those are time consuming (tens of ms), we don't want + * to perform that twice, so, since the Jasperlake issue hasn't been + * observed on MTL, we avoid repeating the reset on newer platforms. + */ + loops = GRAPHICS_VER_FULL(gt->i915) < IP_VER(12, 70) ? 2 : 1; + /* * GEN6_GDRST is not in the gt power well, no need to check * for fifo space for the write or forcewake the chip for @@ -279,20 +297,7 @@ static int gen6_hw_domain_reset(struct intel_gt *gt, u32 hw_domain_mask) do { intel_uncore_write_fw(uncore, GEN6_GDRST, hw_domain_mask); - /* - * Wait for the device to ack the reset requests. - * - * On some platforms, e.g. Jasperlake, we see that the - * engine register state is not cleared until shortly after - * GDRST reports completion, causing a failure as we try - * to immediately resume while the internal state is still - * in flux. If we immediately repeat the reset, the second - * reset appears to serialise with the first, and since - * it is a no-op, the registers should retain their reset - * value. However, there is still a concern that upon - * leaving the second reset, the internal engine state - * is still in flux and not ready for resuming. - */ + /* Wait for the device to ack the reset requests. */ err = __intel_wait_for_register_fw(uncore, GEN6_GDRST, hw_domain_mask, 0, 2000, 0, From patchwork Thu Mar 23 23:18:57 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniele Ceraolo Spurio X-Patchwork-Id: 13186168 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 9E6C7C7619A for ; Thu, 23 Mar 2023 23:19:39 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 27ED610E197; Thu, 23 Mar 2023 23:19:39 +0000 (UTC) Received: from mga06.intel.com (mga06b.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id F1D7010E197 for ; Thu, 23 Mar 2023 23:19:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1679613577; x=1711149577; h=from:to:subject:date:message-id:in-reply-to:references: mime-version:content-transfer-encoding; bh=skut+ituEgFMiAGPkhf08xK7zmjqLbYbPFrzr9i5c48=; b=WG1oUP12UexvEd3pMdAWRPIueKZYQyfuYpa4qCksFGUSBaRWOjn6SrWz 7dJVAoNmnCJNgvdgJpz1bNk+ZM+IYW4vFbZaMRyGb7r6VxeX325JOTxTf JLGKsobc0pYAXlKV37SGEv17VTos4IO4hZA9DCYHDvMDzDVP7yUMYMTZm 88UrZCPjgj/3ySruWSQTBpsq44D0B0cd8bkIS3AimqqytSeXfs6gmi2/m jQHMJBHEOn6zfozb9K1C8Jsumxzr0HOCW3OM9w+fmoETnsH6eO3ZPZ/qH q/L3J/MOrffa3CnH7bMeeCpOWDFQNfnqRRS0Qmy+5VTyY2irjch5oCxhX w==; X-IronPort-AV: E=McAfee;i="6600,9927,10658"; a="402232328" X-IronPort-AV: E=Sophos;i="5.98,286,1673942400"; d="scan'208";a="402232328" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Mar 2023 16:19:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10658"; a="715011521" X-IronPort-AV: E=Sophos;i="5.98,286,1673942400"; d="scan'208";a="715011521" Received: from valcore-skull-1.fm.intel.com ([10.1.27.19]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Mar 2023 16:19:28 -0700 From: Daniele Ceraolo Spurio To: intel-gfx@lists.freedesktop.org Date: Thu, 23 Mar 2023 16:18:57 -0700 Message-Id: <20230323231857.2194435-2-daniele.ceraolospurio@intel.com> X-Mailer: git-send-email 2.37.3 In-Reply-To: <20230323231857.2194435-1-daniele.ceraolospurio@intel.com> References: <20230323231857.2194435-1-daniele.ceraolospurio@intel.com> MIME-Version: 1.0 Subject: [Intel-gfx] [CI 2/2] drm/i915/gsc: implement wa 14015076503 X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" 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. 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. v2: add comment to explain why it is safe to skip the GSC reset (John) Signed-off-by: Daniele Ceraolo Spurio Cc: Matt Roper Cc: John Harrison Cc: Rodrigo Vivi Reviewed-by: John Harrison --- drivers/gpu/drm/i915/gt/intel_reset.c | 84 +++++++++++++++++++++-- drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h | 2 + drivers/gpu/drm/i915/i915_reg.h | 14 +++- 3 files changed, 93 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..797ea8340467 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,74 @@ 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 is the first ALL_ENGINES + * reset 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. + * Skipping the GSC engine is safe because, differently from other + * engines, the GSCCS only role is to forward the commands to the GSC + * FW, so it doesn't have any HW outside of the CS itself and therefore + * it has no state that we don't explicitly re-init on resume or on + * context switch LRC or power context). The HW for the GSC uC is + * managed by the GSC FW so we don't need to care about that. + */ + 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 +782,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 +816,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 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..8dac20b0e7a7 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