| Message ID | 20230720210737.761400-10-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update AUX invalidation sequence | expand |
Hi Andi, On Thursday, 20 July 2023 23:07:37 CEST Andi Shyti wrote: > Perform some refactoring with the purpose of keeping in one > single place all the operations around the aux table > invalidation. > > With this refactoring add more engines where the invalidation > should be performed. > > Fixes: 972282c4cf24 ("drm/i915/gen12: Add aux table invalidate for all engines") > Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> > Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: <stable@vger.kernel.org> # v5.8+ > --- > drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 58 +++++++++++++++--------- > drivers/gpu/drm/i915/gt/gen8_engine_cs.h | 3 +- > drivers/gpu/drm/i915/gt/intel_lrc.c | 17 +------ > 3 files changed, 41 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > index 3ded597f002a2..30fb4e0af6134 100644 > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > @@ -165,9 +165,36 @@ static u32 preparser_disable(bool state) > return MI_ARB_CHECK | 1 << 8 | state; > } > > -u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv_reg) > +static i915_reg_t gen12_get_aux_inv_reg(struct intel_engine_cs *engine) > { > - u32 gsi_offset = gt->uncore->gsi_offset; > + if (!HAS_AUX_CCS(engine->i915)) > + return INVALID_MMIO_REG; > + > + switch (engine->id) { > + case RCS0: > + return GEN12_CCS_AUX_INV; > + case BCS0: > + return GEN12_BCS0_AUX_INV; > + case VCS0: > + return GEN12_VD0_AUX_INV; > + case VCS2: > + return GEN12_VD2_AUX_INV; > + case VECS0: > + return GEN12_VE0_AUX_INV; > + case CCS0: > + return GEN12_CCS0_AUX_INV; > + default: > + return INVALID_MMIO_REG; > + } > +} > + > +u32 *gen12_emit_aux_table_inv(struct intel_engine_cs *engine, u32 *cs) > +{ > + i915_reg_t inv_reg = gen12_get_aux_inv_reg(engine); > + u32 gsi_offset = engine->gt->uncore->gsi_offset; > + > + if (i915_mmio_reg_valid(inv_reg)) > + return cs; Is that correct? Now the original body of gen12_emit_aux_table_inv() will be executed only if either (!HAS_AUX_CCS(engine->i915) or the engine is not one of (RCS0, BCS0, VCS0, VCS2 or CCS0), ... > > *cs++ = MI_LOAD_REGISTER_IMM(1) | MI_LRI_MMIO_REMAP_EN; > *cs++ = i915_mmio_reg_offset(inv_reg) + gsi_offset; > @@ -201,6 +228,11 @@ static u32 *intel_emit_pipe_control_cs(struct i915_request *rq, u32 bit_group_0, > return cs; > } > > +static bool gen12_engine_has_aux_inv(struct intel_engine_cs *engine) > +{ > + return i915_mmio_reg_valid(gen12_get_aux_inv_reg(engine)); > +} > + > static int mtl_dummy_pipe_control(struct i915_request *rq) > { > /* Wa_14016712196 */ > @@ -307,11 +339,7 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) > > cs = gen8_emit_pipe_control(cs, flags, LRC_PPHWSP_SCRATCH_ADDR); > > - if (!HAS_FLAT_CCS(rq->engine->i915)) { ... while before it was executed only if (!HAS_FLAT_CCS(rq->engine->i915)), which, according to commit description of PATCH 2/9, rather had the opposite meaning. Am I missing something? Thanks, Janusz > - /* hsdes: 1809175790 */ > - cs = gen12_emit_aux_table_inv(rq->engine->gt, cs, > - GEN12_CCS_AUX_INV); > - } > + cs = gen12_emit_aux_table_inv(engine, cs); > > *cs++ = preparser_disable(false); > intel_ring_advance(rq, cs); > @@ -322,7 +350,6 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) > > int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) > { > - intel_engine_mask_t aux_inv = 0; > u32 cmd_flush = 0; > u32 cmd = 4; > u32 *cs; > @@ -330,15 +357,11 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) > if (mode & EMIT_INVALIDATE) > cmd += 2; > > - if (HAS_AUX_CCS(rq->engine->i915)) > - aux_inv = rq->engine->mask & > - ~GENMASK(_BCS(I915_MAX_BCS - 1), BCS0); > - > /* > * On Aux CCS platforms the invalidation of the Aux > * table requires quiescing memory traffic beforehand > */ > - if (aux_inv) { > + if (gen12_engine_has_aux_inv(rq->engine)) { > cmd += 8; /* for the AUX invalidation */ > cmd += 2; /* for the engine quiescing */ > > @@ -381,14 +404,7 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) > *cs++ = 0; /* upper addr */ > *cs++ = 0; /* value */ > > - if (aux_inv) { /* hsdes: 1809175790 */ > - if (rq->engine->class == VIDEO_DECODE_CLASS) > - cs = gen12_emit_aux_table_inv(rq->engine->gt, > - cs, GEN12_VD0_AUX_INV); > - else > - cs = gen12_emit_aux_table_inv(rq->engine->gt, > - cs, GEN12_VE0_AUX_INV); > - } > + cs = gen12_emit_aux_table_inv(rq->engine, cs); > > if (mode & EMIT_INVALIDATE) > *cs++ = preparser_disable(false); > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.h b/drivers/gpu/drm/i915/gt/gen8_engine_cs.h > index a44eda096557c..867ba697aceb8 100644 > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.h > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.h > @@ -13,6 +13,7 @@ > #include "intel_gt_regs.h" > #include "intel_gpu_commands.h" > > +struct intel_engine_cs; > struct intel_gt; > struct i915_request; > > @@ -46,7 +47,7 @@ u32 *gen8_emit_fini_breadcrumb_rcs(struct i915_request *rq, u32 *cs); > u32 *gen11_emit_fini_breadcrumb_rcs(struct i915_request *rq, u32 *cs); > u32 *gen12_emit_fini_breadcrumb_rcs(struct i915_request *rq, u32 *cs); > > -u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv_reg); > +u32 *gen12_emit_aux_table_inv(struct intel_engine_cs *engine, u32 *cs); > > static inline u32 * > __gen8_emit_pipe_control(u32 *batch, u32 bit_group_0, > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > index 235f3fab60a98..119deb9f938c7 100644 > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > @@ -1371,10 +1371,7 @@ gen12_emit_indirect_ctx_rcs(const struct intel_context *ce, u32 *cs) > IS_DG2_G11(ce->engine->i915)) > cs = gen8_emit_pipe_control(cs, PIPE_CONTROL_INSTRUCTION_CACHE_INVALIDATE, 0); > > - /* hsdes: 1809175790 */ > - if (!HAS_FLAT_CCS(ce->engine->i915)) > - cs = gen12_emit_aux_table_inv(ce->engine->gt, > - cs, GEN12_CCS_AUX_INV); > + cs = gen12_emit_aux_table_inv(ce->engine, cs); > > /* Wa_16014892111 */ > if (IS_MTL_GRAPHICS_STEP(ce->engine->i915, M, STEP_A0, STEP_B0) || > @@ -1399,17 +1396,7 @@ gen12_emit_indirect_ctx_xcs(const struct intel_context *ce, u32 *cs) > PIPE_CONTROL_INSTRUCTION_CACHE_INVALIDATE, > 0); > > - /* hsdes: 1809175790 */ > - if (!HAS_FLAT_CCS(ce->engine->i915)) { > - if (ce->engine->class == VIDEO_DECODE_CLASS) > - cs = gen12_emit_aux_table_inv(ce->engine->gt, > - cs, GEN12_VD0_AUX_INV); > - else if (ce->engine->class == VIDEO_ENHANCEMENT_CLASS) > - cs = gen12_emit_aux_table_inv(ce->engine->gt, > - cs, GEN12_VE0_AUX_INV); > - } > - > - return cs; > + return gen12_emit_aux_table_inv(ce->engine, cs); > } > > static void > --------------------------------------------------------------------- Intel Technology Poland sp. z o.o. ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN. Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych. Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
Hi Janusz, > > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > > index 3ded597f002a2..30fb4e0af6134 100644 > > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > > @@ -165,9 +165,36 @@ static u32 preparser_disable(bool state) > > return MI_ARB_CHECK | 1 << 8 | state; > > } > > > > -u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv_reg) > > +static i915_reg_t gen12_get_aux_inv_reg(struct intel_engine_cs *engine) > > { > > - u32 gsi_offset = gt->uncore->gsi_offset; > > + if (!HAS_AUX_CCS(engine->i915)) > > + return INVALID_MMIO_REG; > > + > > + switch (engine->id) { > > + case RCS0: > > + return GEN12_CCS_AUX_INV; > > + case BCS0: > > + return GEN12_BCS0_AUX_INV; > > + case VCS0: > > + return GEN12_VD0_AUX_INV; > > + case VCS2: > > + return GEN12_VD2_AUX_INV; > > + case VECS0: > > + return GEN12_VE0_AUX_INV; > > + case CCS0: > > + return GEN12_CCS0_AUX_INV; > > + default: > > + return INVALID_MMIO_REG; > > + } > > +} > > + > > +u32 *gen12_emit_aux_table_inv(struct intel_engine_cs *engine, u32 *cs) > > +{ > > + i915_reg_t inv_reg = gen12_get_aux_inv_reg(engine); > > + u32 gsi_offset = engine->gt->uncore->gsi_offset; > > + > > + if (i915_mmio_reg_valid(inv_reg)) > > + return cs; > > Is that correct? Now the original body of gen12_emit_aux_table_inv() will be > executed only if either (!HAS_AUX_CCS(engine->i915) or the engine is not one > of (RCS0, BCS0, VCS0, VCS2 or CCS0), ... > > > > > *cs++ = MI_LOAD_REGISTER_IMM(1) | MI_LRI_MMIO_REMAP_EN; > > *cs++ = i915_mmio_reg_offset(inv_reg) + gsi_offset; > > @@ -201,6 +228,11 @@ static u32 *intel_emit_pipe_control_cs(struct i915_request *rq, u32 bit_group_0, > > return cs; > > } > > > > +static bool gen12_engine_has_aux_inv(struct intel_engine_cs *engine) > > +{ > > + return i915_mmio_reg_valid(gen12_get_aux_inv_reg(engine)); > > +} > > + > > static int mtl_dummy_pipe_control(struct i915_request *rq) > > { > > /* Wa_14016712196 */ > > @@ -307,11 +339,7 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) > > > > cs = gen8_emit_pipe_control(cs, flags, LRC_PPHWSP_SCRATCH_ADDR); > > > > - if (!HAS_FLAT_CCS(rq->engine->i915)) { > > ... while before it was executed only if (!HAS_FLAT_CCS(rq->engine->i915)), > which, according to commit description of PATCH 2/9, rather had the opposite > meaning. Am I missing something? flat_ccs and aux_ccs are not mutually exclusive, so far the can both miss like in PVC. So that the !HAS_FLAT_CCS() is an approximation and that's why we need a better evaluation. Aux invalidation is needed only on platforms from TGL and beyond excluding PVC. The above engines are the only engines where AUX invalidation happens, but there are no cases when we reach the default condition, as the emit_flush_rcs is already called within that set of engines. The default is there just for completeness. Does this answer? Thanks, Andi
diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c index 3ded597f002a2..30fb4e0af6134 100644 --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c @@ -165,9 +165,36 @@ static u32 preparser_disable(bool state) return MI_ARB_CHECK | 1 << 8 | state; } -u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv_reg) +static i915_reg_t gen12_get_aux_inv_reg(struct intel_engine_cs *engine) { - u32 gsi_offset = gt->uncore->gsi_offset; + if (!HAS_AUX_CCS(engine->i915)) + return INVALID_MMIO_REG; + + switch (engine->id) { + case RCS0: + return GEN12_CCS_AUX_INV; + case BCS0: + return GEN12_BCS0_AUX_INV; + case VCS0: + return GEN12_VD0_AUX_INV; + case VCS2: + return GEN12_VD2_AUX_INV; + case VECS0: + return GEN12_VE0_AUX_INV; + case CCS0: + return GEN12_CCS0_AUX_INV; + default: + return INVALID_MMIO_REG; + } +} + +u32 *gen12_emit_aux_table_inv(struct intel_engine_cs *engine, u32 *cs) +{ + i915_reg_t inv_reg = gen12_get_aux_inv_reg(engine); + u32 gsi_offset = engine->gt->uncore->gsi_offset; + + if (i915_mmio_reg_valid(inv_reg)) + return cs; *cs++ = MI_LOAD_REGISTER_IMM(1) | MI_LRI_MMIO_REMAP_EN; *cs++ = i915_mmio_reg_offset(inv_reg) + gsi_offset; @@ -201,6 +228,11 @@ static u32 *intel_emit_pipe_control_cs(struct i915_request *rq, u32 bit_group_0, return cs; } +static bool gen12_engine_has_aux_inv(struct intel_engine_cs *engine) +{ + return i915_mmio_reg_valid(gen12_get_aux_inv_reg(engine)); +} + static int mtl_dummy_pipe_control(struct i915_request *rq) { /* Wa_14016712196 */ @@ -307,11 +339,7 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) cs = gen8_emit_pipe_control(cs, flags, LRC_PPHWSP_SCRATCH_ADDR); - if (!HAS_FLAT_CCS(rq->engine->i915)) { - /* hsdes: 1809175790 */ - cs = gen12_emit_aux_table_inv(rq->engine->gt, cs, - GEN12_CCS_AUX_INV); - } + cs = gen12_emit_aux_table_inv(engine, cs); *cs++ = preparser_disable(false); intel_ring_advance(rq, cs); @@ -322,7 +350,6 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32 mode) int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) { - intel_engine_mask_t aux_inv = 0; u32 cmd_flush = 0; u32 cmd = 4; u32 *cs; @@ -330,15 +357,11 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) if (mode & EMIT_INVALIDATE) cmd += 2; - if (HAS_AUX_CCS(rq->engine->i915)) - aux_inv = rq->engine->mask & - ~GENMASK(_BCS(I915_MAX_BCS - 1), BCS0); - /* * On Aux CCS platforms the invalidation of the Aux * table requires quiescing memory traffic beforehand */ - if (aux_inv) { + if (gen12_engine_has_aux_inv(rq->engine)) { cmd += 8; /* for the AUX invalidation */ cmd += 2; /* for the engine quiescing */ @@ -381,14 +404,7 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32 mode) *cs++ = 0; /* upper addr */ *cs++ = 0; /* value */ - if (aux_inv) { /* hsdes: 1809175790 */ - if (rq->engine->class == VIDEO_DECODE_CLASS) - cs = gen12_emit_aux_table_inv(rq->engine->gt, - cs, GEN12_VD0_AUX_INV); - else - cs = gen12_emit_aux_table_inv(rq->engine->gt, - cs, GEN12_VE0_AUX_INV); - } + cs = gen12_emit_aux_table_inv(rq->engine, cs); if (mode & EMIT_INVALIDATE) *cs++ = preparser_disable(false); diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.h b/drivers/gpu/drm/i915/gt/gen8_engine_cs.h index a44eda096557c..867ba697aceb8 100644 --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.h +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.h @@ -13,6 +13,7 @@ #include "intel_gt_regs.h" #include "intel_gpu_commands.h" +struct intel_engine_cs; struct intel_gt; struct i915_request; @@ -46,7 +47,7 @@ u32 *gen8_emit_fini_breadcrumb_rcs(struct i915_request *rq, u32 *cs); u32 *gen11_emit_fini_breadcrumb_rcs(struct i915_request *rq, u32 *cs); u32 *gen12_emit_fini_breadcrumb_rcs(struct i915_request *rq, u32 *cs); -u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv_reg); +u32 *gen12_emit_aux_table_inv(struct intel_engine_cs *engine, u32 *cs); static inline u32 * __gen8_emit_pipe_control(u32 *batch, u32 bit_group_0, diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index 235f3fab60a98..119deb9f938c7 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -1371,10 +1371,7 @@ gen12_emit_indirect_ctx_rcs(const struct intel_context *ce, u32 *cs) IS_DG2_G11(ce->engine->i915)) cs = gen8_emit_pipe_control(cs, PIPE_CONTROL_INSTRUCTION_CACHE_INVALIDATE, 0); - /* hsdes: 1809175790 */ - if (!HAS_FLAT_CCS(ce->engine->i915)) - cs = gen12_emit_aux_table_inv(ce->engine->gt, - cs, GEN12_CCS_AUX_INV); + cs = gen12_emit_aux_table_inv(ce->engine, cs); /* Wa_16014892111 */ if (IS_MTL_GRAPHICS_STEP(ce->engine->i915, M, STEP_A0, STEP_B0) || @@ -1399,17 +1396,7 @@ gen12_emit_indirect_ctx_xcs(const struct intel_context *ce, u32 *cs) PIPE_CONTROL_INSTRUCTION_CACHE_INVALIDATE, 0); - /* hsdes: 1809175790 */ - if (!HAS_FLAT_CCS(ce->engine->i915)) { - if (ce->engine->class == VIDEO_DECODE_CLASS) - cs = gen12_emit_aux_table_inv(ce->engine->gt, - cs, GEN12_VD0_AUX_INV); - else if (ce->engine->class == VIDEO_ENHANCEMENT_CLASS) - cs = gen12_emit_aux_table_inv(ce->engine->gt, - cs, GEN12_VE0_AUX_INV); - } - - return cs; + return gen12_emit_aux_table_inv(ce->engine, cs); } static void
Perform some refactoring with the purpose of keeping in one single place all the operations around the aux table invalidation. With this refactoring add more engines where the invalidation should be performed. Fixes: 972282c4cf24 ("drm/i915/gen12: Add aux table invalidate for all engines") Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: <stable@vger.kernel.org> # v5.8+ --- drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 58 +++++++++++++++--------- drivers/gpu/drm/i915/gt/gen8_engine_cs.h | 3 +- drivers/gpu/drm/i915/gt/intel_lrc.c | 17 +------ 3 files changed, 41 insertions(+), 37 deletions(-)