On 3/15/2024 04:21, Badal Nilawar wrote:
> Enable GuC Wa_14019882105 to block interrupts during C6 flow
> when the memory path has been blocked
>
> Cc: John Harrison <john.harrison at intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar at intel.com>
> ---
> drivers/gpu/drm/xe/abi/guc_klvs_abi.h | 7 +++++
> drivers/gpu/drm/xe/xe_guc_ads.c | 41 +++++++++++++++++++++------
> drivers/gpu/drm/xe/xe_wa_oob.rules | 1 +
> 3 files changed, 40 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
> index 0400bc0fccdc..5dd45e06f0b6 100644
> --- a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
> +++ b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
> @@ -319,4 +319,11 @@ enum {
> #define GUC_KLV_VF_CFG_BEGIN_CONTEXT_ID_KEY 0x8a0b
> #define GUC_KLV_VF_CFG_BEGIN_CONTEXT_ID_LEN 1u
>
> +/*
> + * Workaround keys:
> + */
> +enum xe_guc_klv_ids {
> + GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED = 0x9002,
> +};
> +
> #endif
> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
> index ee48cf01fe22..3d2048747230 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
> @@ -7,6 +7,8 @@
>
> #include <drm/drm_managed.h>
>
> +#include <generated/xe_wa_oob.h>
> +
> #include "regs/xe_engine_regs.h"
> #include "regs/xe_gt_regs.h"
> #include "regs/xe_guc_regs.h"
> @@ -19,6 +21,7 @@
> #include "xe_map.h"
> #include "xe_mmio.h"
> #include "xe_platform_types.h"
> +#include "xe_wa.h"
>
> /* Slack of a few additional entries per engine */
> #define ADS_REGSET_EXTRA_MAX 8
> @@ -281,8 +284,30 @@ static size_t calculate_golden_lrc_size(struct xe_guc_ads *ads)
>
> #define GUC_VER(maj, min, pat) (((maj) << 16) | ((min) << 8) | (pat))
>
> +static u32 guc_waklv_opt_in(struct xe_guc_ads *ads,
> + enum xe_guc_klv_ids klv_id, u32 offset, u32 remain)
If going straight to a generic helper, I would recommend following the
i915 version - move the += size / -= size inside the helper rather than
duplicating for each call.
Also, not sure opt_in is the best name. There could be future KLVs that
take parameters (it did almost happen already but the direction changed
for other reasons) which would still be an opt in. Hence the i915
version is called guc_waklv_enable_simple.
> +{
> + u32 size;
> + u32 klv_entry[] = {
> + /* 16:16 key/length */
> + FIELD_PREP(GUC_KLV_0_KEY, klv_id) |
> + FIELD_PREP(GUC_KLV_0_LEN, 0),
> + /* 0 dwords data */
> + };
> +
> + size = sizeof(klv_entry);
> +
> + if (remain < size)
> + drm_warn(&ads_to_xe(ads)->drm, "Cannot apply guc wa klv_id %d\n", klv_id);
The message says 'cannot apply' and the copy is skipped but the size is
still returned and overflow will still happen.
John.
> + else
> + xe_map_memcpy_to(ads_to_xe(ads), ads_to_map(ads), offset,
> + klv_entry, size);
> + return size;
> +}
> +
> static void guc_waklv_init(struct xe_guc_ads *ads)
> {
> + struct xe_gt *gt = ads_to_gt(ads);
> u32 addr_ggtt, offset, remain, size;
> struct xe_uc_fw *uc_fw = &ads_to_guc(ads)->fw;
> struct xe_uc_fw_version *version = &uc_fw->versions.found[XE_UC_FW_VER_RELEASE];
> @@ -293,15 +318,13 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
> offset = guc_ads_waklv_offset(ads);
> remain = guc_ads_waklv_size(ads);
>
> - /*
> - * Add workarounds here:
> - *
> - * if (want_wa_<name>) {
> - * size = guc_waklv_<name>(guc, offset, remain);
> - * offset += size;
> - * remain -= size;
> - * }
> - */
> + if (XE_WA(gt, 14019882105)) {
> + size = guc_waklv_opt_in(ads,
> + GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
> + offset, remain);
> + offset += size;
> + remain -= size;
> + }
>
> size = guc_ads_waklv_size(ads) - remain;
> if (!size)
> diff --git a/drivers/gpu/drm/xe/xe_wa_oob.rules b/drivers/gpu/drm/xe/xe_wa_oob.rules
> index 48cdba1cbf95..a8d15f004b6c 100644
> --- a/drivers/gpu/drm/xe/xe_wa_oob.rules
> +++ b/drivers/gpu/drm/xe/xe_wa_oob.rules
> @@ -19,3 +19,4 @@
> GRAPHICS_VERSION_RANGE(1270, 1274)
> MEDIA_VERSION(1300)
> PLATFORM(DG2)
> +14019882105 GRAPHICS_VERSION(2004), GRAPHICS_STEP(A0, B0)