[Intel-xe] [PATCH v3 9/9] drm/xe/guc: Port workarounds to OOB infra
Matt Roper
matthew.d.roper at intel.com
Wed May 17 22:43:03 UTC 2023
On Wed, May 17, 2023 at 03:25:07PM -0700, Lucas De Marchi wrote:
> 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))
How would we write the condition if a workaround can potentially apply
to both GTs, but only to specific steppings of each? E.g., in the
"12345678900" I gave above, trying to write the code like this:
void init(struct xe_gt *gt) {
if (XE_WA(xe, 12345678900))
do_something(gt);
}
won't work since the XE_WA() will evaluate the same way both times we
call the function (primary GT and media GT). If we happen to be running
on a platform that falls within the range for both graphics and media,
then that's okay. But if it turns out the platform's graphics is
in-range and media is out-of-range, or vice-versa, then we're applying
the workaround somewhere we shouldn't be.
Effectively XE_WA() is only telling us whether the workaround will be
active _somewhere_ on the platform. It's not currently capable of
telling us whether it should be active on the part of the platform we're
operating on right now (i.e., it doesn't take xe_gt as parameter rather
than xe_device).
Matt
>
>
> 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
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
More information about the Intel-xe
mailing list