[Intel-xe] [PATCH v3 9/9] drm/xe/guc: Port workarounds to OOB infra
Lucas De Marchi
lucas.demarchi at intel.com
Wed May 17 22:25:07 UTC 2023
On Wed, May 17, 2023 at 02:25:52PM -0700, Matt Roper wrote:
>On Tue, May 16, 2023 at 03:19:50PM -0700, Lucas De Marchi wrote:
>> Let xe_guc.c start using XE_WA() for workarounds. The one missing
>> workaround is left with a "FIXME" since it's not properly implemented
>> yet.
>>
>> Also, some changes were made to a few workarounds:
>>
>> - Wa_22011383443 not implemented due to being an early PVC
>> stepping. Wa_16011759253 that has the same condition is the
>> one covering DG2.
>>
>> - Wa_22011391025 and Wa_14012197797 were split in 2, since they
>> cover different steppings and we do want to report both as
>> implemented. The latter may eventually be removed due to being
>> early DG2 steppings, but the former should remain.
>>
>> - Wa_22012727170 and Wa_22012727685 were split in 2 so both of
>> them can be reported as active, regardless if they (currently)
>> have the same rules.
>>
>> - Wa_16015675438 and Wa_18020744125 were not checking by
>> platform or IP version, hence making them not future-proof.
>> Those workarounds should only be active in PVC and DG2,
>> besides the check for "no render engine".
>>
>> DG2 system used for test:
>> XE_DG2 G12 56b2:0001 dgfx:1 gfx:Xe_HPG (12.55) media:Xe_HPM (12.55) ...
>> Stepping = (G:A1, M:A1, D:C0, B:**)
>>
>> Output is as expected for the current OOB workarounds:
>>
>> $ cat /sys/kernel/debug/dri/0/workarounds
>> ...
>> OOB Workarounds
>> 14014475959
>> 22011391025
>> 14012197797
>> 16015675438
>> 16016323070
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>> drivers/gpu/drm/xe/Makefile | 2 +-
>> drivers/gpu/drm/xe/xe_guc.c | 43 ++++++++----------------------
>> drivers/gpu/drm/xe/xe_wa_oob.rules | 16 +++++++++++
>> 3 files changed, 28 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index fcf04951a309..07af0d022263 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -38,7 +38,7 @@ $(XE_WA_OOB) &: $(obj)/xe_gen_wa_oob $(srctree)/$(src)/xe_wa_oob.rules
>> @mkdir -p $(@D)
>> $(call cmd,wa_oob)
>>
>> -$(obj)/xe_wa.o: $(XE_WA_OOB)
>> +$(obj)/xe_wa.o $(obj)/xe_guc.o: $(XE_WA_OOB)
>>
>> # Please keep these build lists sorted!
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index 1b5d0b6ad180..252a4bfe6d63 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -5,6 +5,7 @@
>>
>> #include "xe_guc.h"
>>
>> +#include "generated/xe_wa_oob.h"
>> #include "regs/xe_gt_regs.h"
>> #include "regs/xe_guc_regs.h"
>> #include "xe_bo.h"
>> @@ -20,6 +21,7 @@
>> #include "xe_mmio.h"
>> #include "xe_platform_types.h"
>> #include "xe_uc_fw.h"
>> +#include "xe_wa.h"
>> #include "xe_wopcm.h"
>>
>> #define MEDIA_GUC_HOST_INTERRUPT XE_REG(0x190304)
>> @@ -136,29 +138,16 @@ static u32 guc_ctl_wa_flags(struct xe_guc *guc)
>> struct xe_gt *gt = guc_to_gt(guc);
>> u32 flags = 0;
>>
>> - /* Wa_22012773006:gen11,gen12 < XeHP */
>> - if (GRAPHICS_VER(xe) >= 11 &&
>> - GRAPHICS_VERx100(xe) < 1250)
>> + if (XE_WA(xe, 22012773006))
>> flags |= GUC_WA_POLLCS;
>>
>> - /* Wa_16011759253 */
>> - /* Wa_22011383443 */
>> - if (IS_SUBPLATFORM_STEP(xe, XE_DG2, XE_SUBPLATFORM_DG2_G10, STEP_A0, STEP_B0))
>> + if (XE_WA(xe, 16011759253))
>> flags |= GUC_WA_GAM_CREDITS;
>>
>> - /* Wa_14014475959 */
>> - if (IS_PLATFORM_STEP(xe, XE_METEORLAKE, STEP_A0, STEP_B0) ||
>> - xe->info.platform == XE_DG2)
>> + if (XE_WA(xe, 14014475959))
>> flags |= GUC_WA_HOLD_CCS_SWITCHOUT;
>
>Hmm, this workaround was already implemented incorrectly, but it does do
>a good job of highlighting a shortcoming that we need to address...see
>comment farther down.
>
>>
>> - /*
>> - * Wa_14012197797
>> - * Wa_22011391025
>> - *
>> - * The same WA bit is used for both and 22011391025 is applicable to
>> - * all DG2.
>> - */
>> - if (xe->info.platform == XE_DG2)
>> + if (XE_WA(xe, 22011391025) || XE_WA(xe, 14012197797))
>> flags |= GUC_WA_DUAL_QUEUE;
>>
>> /*
>> @@ -169,27 +158,17 @@ static u32 guc_ctl_wa_flags(struct xe_guc *guc)
>> if (GRAPHICS_VERx100(xe) < 1270)
>> flags |= GUC_WA_PRE_PARSER;
>>
>> - /* Wa_16011777198 */
>> - if (IS_SUBPLATFORM_STEP(xe, XE_DG2, XE_SUBPLATFORM_DG2_G10, STEP_A0, STEP_C0) ||
>> - IS_SUBPLATFORM_STEP(xe, XE_DG2, XE_SUBPLATFORM_DG2_G11, STEP_A0,
>> - STEP_B0))
>> + if (XE_WA(xe, 16011777198))
>> flags |= GUC_WA_RCS_RESET_BEFORE_RC6;
>>
>> - /*
>> - * Wa_22012727170
>> - * Wa_22012727685
>> - */
>> - if (IS_SUBPLATFORM_STEP(xe, XE_DG2, XE_SUBPLATFORM_DG2_G10, STEP_A0, STEP_C0) ||
>> - IS_SUBPLATFORM_STEP(xe, XE_DG2, XE_SUBPLATFORM_DG2_G11, STEP_A0,
>> - STEP_FOREVER))
>> + if (XE_WA(xe, 22012727170) || XE_WA(xe, 22012727685))
>> flags |= GUC_WA_CONTEXT_ISOLATION;
>>
>> - /* Wa_16015675438, Wa_18020744125 */
>> - if (!xe_hw_engine_mask_per_class(gt, XE_ENGINE_CLASS_RENDER))
>> + if ((XE_WA(xe, 16015675438) || XE_WA(xe, 16016323070)) &&
>> + !xe_hw_engine_mask_per_class(gt, XE_ENGINE_CLASS_RENDER))
>> flags |= GUC_WA_RCS_REGS_IN_CCS_REGS_LIST;
>>
>> - /* Wa_1509372804 */
>> - if (IS_PLATFORM_STEP(xe, XE_PVC, STEP_B0, STEP_C0))
>> + if (XE_WA(xe, 1509372804))
>> flags |= GUC_WA_RENDER_RST_RC6_EXIT;
>>
>> return flags;
>> diff --git a/drivers/gpu/drm/xe/xe_wa_oob.rules b/drivers/gpu/drm/xe/xe_wa_oob.rules
>> index e69de29bb2d1..060ff25496a7 100644
>> --- a/drivers/gpu/drm/xe/xe_wa_oob.rules
>> +++ b/drivers/gpu/drm/xe/xe_wa_oob.rules
>> @@ -0,0 +1,16 @@
>> +22012773006 GRAPHICS_VERSION_RANGE(1100, 1250)
>> +16011759253 SUBPLATFORM(DG2, G10), STEP(A0, B0)
>> +14014475959 PLATFORM(METEORLAKE), STEP(A0, B0)
>
>In general we can't match on platform anymore for disaggregated IP
>platforms; we need to match solely on IP+version now. For example, this
>workaround applies to 12.70, but does not apply to 12.71 and also does
>not apply to the media GT for media version 13. The old way of doing
>things with subplatforms also doesn't work well anymore since we have to
>be ready for this 12.70 graphics chiplet to potentially be re-used in
>some future non-MTL platform.
>
>A first step of doing
>
> 14014475959 GRAPHICS_VERSION(1270), STEP(A0, B0)
>
>isn't quite enough since it will still result in this workaround being
>applied every time guc_ctl_wa_flags() gets called on a platform with
>graphics version 12.70. In reality, we only want this workaround to be
>applied on the _primary_ GT if the graphics version is 12.70. When we
>loop around and run the function a second time on the media GT, it
>should not be applied since media version 13 does not need this
>workaround. Other workarounds might apply to both graphics and media
>GTs, but only specific steppings of each, so we need to specify which IP
>to check the stepping for. At the moment XE_RTP_MATCH_STEP can only
>cope with graphics steppings in rule_matches().
>
>So maybe we want something like:
>
> 12345678900 GRAPHICS_VER_STEP(1270, B0, B3)
> MEDIA_VER_STEP(1300, C1, F4)
MEDIA_VER_STEP() has a implicit && I'd like to avoid with something like
this:
12345678900 GRAPHICS_VER(1270), STEP(B0, B3)
MEDIA_VER(1300), MEDIA_STEP(C1, F4)
[ MEDIA_STEP() to be added and probably STEP() renamed to
GRAPHICS_STEP ]
This is the same as what you wrote, but without exploding on the number
of matches. For instance I'd expect the next request to be the STEP +
version range.
My idea was to leave this on the device-level and anything that is to be
checked on the gt / engine level or in another way, is checked from
outside. See the handling for the other WA above:
if ((XE_WA(xe, 16015675438) || XE_WA(xe, 16016323070)) &&
!xe_hw_engine_mask_per_class(gt, XE_ENGINE_CLASS_RENDER))
Lucas De Marchi
>
>that ties the stepping to the specific IP and version. And then we'd
>probably also want XE_WA to take an xe_gt as a parameter so that it can
>make sure that gt->type is appropriate as well.
>
>
>Matt
>
>> + PLATFORM(DG2)
>> +22011391025 PLATFORM(DG2)
>> +14012197797 PLATFORM(DG2), STEP(A0, B0)
>> +16011777198 SUBPLATFORM(DG2, G10), STEP(A0, C0)
>> + SUBPLATFORM(DG2, G11), STEP(A0, B0)
>> +22012727170 SUBPLATFORM(DG2, G10), STEP(A0, C0)
>> + SUBPLATFORM(DG2, G11), STEP(A0, FOREVER)
>> +22012727685 SUBPLATFORM(DG2, G10), STEP(A0, C0)
>> + SUBPLATFORM(DG2, G11), STEP(A0, FOREVER)
>> +16015675438 PLATFORM(PVC)
>> + PLATFORM(DG2)
>> +16016323070 PLATFORM(PVC)
>> +1509372804 PLATFORM(PVC), STEP(B0, C0)
>> --
>> 2.40.1
>>
>
>--
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation
More information about the Intel-xe
mailing list