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
> Cc: Jonathan Cavitt
> Cc: Matt Roper
> Cc: # 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
> -
