On Thu, Feb 23, 2023 at 10:08:51AM +0100, Andrzej Hajda wrote:
> On 17.02.2023 19:54, Matt Roper wrote:
> > MTL's primary GT can continue to use the same engine TLB invalidation
> > programming as past Xe_HP-based platforms. However the media GT needs
> > some special handling:
> > * Invalidation registers on the media GT are singleton registers
> > (unlike the primary GT where they are still MCR).
> > * Since the GSC is now exposed as an engine, there's a new register to
> > use for TLB invalidation. The offset is identical to the compute
> > engine offset, but this is expected --- compute engines only exist on
> > the primary GT while the GSC only exists on the media GT.
> > * Although there's only a single GSC engine instance, it inexplicably
> > uses bit 1 to request invalidations rather than bit 0.
> >
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 52 ++++++++++++++++-------
> > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 +
> > 2 files changed, 38 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > index f3a91e7f85f7..af8e158fbd84 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> > @@ -1166,6 +1166,11 @@ static int intel_engine_init_tlb_invalidation(struct intel_engine_cs *engine)
> > [COPY_ENGINE_CLASS].mcr_reg = XEHP_BLT_TLB_INV_CR,
> > [COMPUTE_CLASS].mcr_reg = XEHP_COMPCTX_TLB_INV_CR,
> > };
> > + static const union intel_engine_tlb_inv_reg xelpmp_regs[] = {
> > + [VIDEO_DECODE_CLASS].reg = GEN12_VD_TLB_INV_CR,
> > + [VIDEO_ENHANCEMENT_CLASS].reg = GEN12_VE_TLB_INV_CR,
> > + [OTHER_CLASS].reg = XELPMP_GSC_TLB_INV_CR,
> > + };
> > struct drm_i915_private *i915 = engine->i915;
> > const unsigned int instance = engine->instance;
> > const unsigned int class = engine->class;
> > @@ -1185,19 +1190,28 @@ static int intel_engine_init_tlb_invalidation(struct intel_engine_cs *engine)
> > * 12.00 -> 12.50 transition multi cast handling is required too.
> > */
> > - if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
> > - GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
> > - regs = xehp_regs;
> > - num = ARRAY_SIZE(xehp_regs);
> > - } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
> > - GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
> > - regs = gen12_regs;
> > - num = ARRAY_SIZE(gen12_regs);
> > - } else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {
> > - regs = gen8_regs;
> > - num = ARRAY_SIZE(gen8_regs);
> > - } else if (GRAPHICS_VER(i915) < 8) {
> > - return 0;
> > + if (engine->gt->type == GT_MEDIA) {
> > + if (MEDIA_VER_FULL(i915) == IP_VER(13, 0)) {
> > + regs = xelpmp_regs;
> > + num = ARRAY_SIZE(xelpmp_regs);
> > + }
>
> As I understand currently only GEN13 of media can have GT_MEDIA, so fallback
> to gt_WARN_ONCE below is expected behavior.
"Gen" terminology isn't used anymore, but yes, standalone media is a new
feature starting from media version 13.
>
> > + } else {
> > + if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 71) ||
>
> 12.71 is not yet present in i915_pci.c, maybe they should be added together,
> up to you.
No, i915_pci.c isn't the source of IP versions anymore. Starting with
MTL (and continuing with future platforms), the graphics, media, and
display IP versions are read out directly from the hardware itself (the
GMD_ID registers); they no longer get derived from PCI devid matching.
The vestigial 12.70 value in i915_pci.c shouldn't get used anywhere
except as a very basic sanity check that the GMD_ID registers are
correctly reporting a high enough version.
Matt
>
> > + GRAPHICS_VER_FULL(i915) == IP_VER(12, 70) > + GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
> > + GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
> > + regs = xehp_regs;
> > + num = ARRAY_SIZE(xehp_regs);
> > + } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
> > + GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
> > + regs = gen12_regs;
> > + num = ARRAY_SIZE(gen12_regs);
> > + } else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {
> > + regs = gen8_regs;
> > + num = ARRAY_SIZE(gen8_regs);
> > + } else if (GRAPHICS_VER(i915) < 8) {
> > + return 0;
> > + }
> > }
> > if (gt_WARN_ONCE(engine->gt, !num,
> > @@ -1212,7 +1226,14 @@ static int intel_engine_init_tlb_invalidation(struct intel_engine_cs *engine)
> > reg = regs[class];
> > - if (regs == gen8_regs && class == VIDEO_DECODE_CLASS && instance == 1) {
> > + if (class == OTHER_CLASS) {
>
> Maybe it would be safer:
> if (regs == xelpmp_regs && class == OTHER_CLASS)
>
>
> > + /*
> > + * There's only a single GSC instance, but it uses register bit
> > + * 1 instead of either 0 or OTHER_GSC_INSTANCE.
> > + */
> > + GEM_WARN_ON(instance != OTHER_GSC_INSTANCE);
> > + val = 1;
> > + } else if (regs == gen8_regs && class == VIDEO_DECODE_CLASS && instance == 1) {
> > reg.reg = GEN8_M2TCR;
> > val = 0;
> > } else {
> > @@ -1228,7 +1249,8 @@ static int intel_engine_init_tlb_invalidation(struct intel_engine_cs *engine)
> > if (GRAPHICS_VER(i915) >= 12 &&
> > (engine->class == VIDEO_DECODE_CLASS ||
> > engine->class == VIDEO_ENHANCEMENT_CLASS ||
> > - engine->class == COMPUTE_CLASS))
> > + engine->class == COMPUTE_CLASS ||
> > + engine->class == OTHER_CLASS))
> This is little surprise, I would expect non-masked reg for single instance,
> but it follows bspec, so OK.
>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
>
> Regards
> Andrzej
>
> > engine->tlb_inv.request = _MASKED_BIT_ENABLE(val);
> > else
> > engine->tlb_inv.request = val;
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > index 416976d396ba..423e3e9c564b 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > @@ -1090,6 +1090,7 @@
> > #define XEHP_BLT_TLB_INV_CR MCR_REG(0xcee4)
> > #define GEN12_COMPCTX_TLB_INV_CR _MMIO(0xcf04)
> > #define XEHP_COMPCTX_TLB_INV_CR MCR_REG(0xcf04)
> > +#define XELPMP_GSC_TLB_INV_CR _MMIO(0xcf04) /* media GT only */
> > #define XEHP_MERT_MOD_CTRL MCR_REG(0xcf28)
> > #define RENDER_MOD_CTRL MCR_REG(0xcf2c)
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
