[PATCH 2/2] drm/i915/gsc: implement wa 14015076503
John Harrison
john.c.harrison at intel.com
Wed Mar 22 21:09:16 UTC 2023
On 3/22/2023 13:59, Ceraolo Spurio, Daniele wrote:
> On 3/22/2023 12:44 PM, John Harrison wrote:
>> 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?
>
> 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. I
> can add this to the commit message if it makes things clearer
Okay. With a suitable extra comment to explain the above and the comment
fix below:
Reviewed-by: John Harrison <John.C.Harrison at Intel.com>
>
> Daniele
>
>>
>>>
>>> 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 at intel.com>
>>> Cc: Matt Roper <matthew.d.roper at intel.com>
>>> Cc: John Harrison <john.c.harrison at intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi at 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
>>
>
More information about the dri-devel
mailing list