Hi Matt,
On Mon, Jul 17, 2023 at 01:27:09PM -0700, Matt Roper wrote:
> On Mon, Jul 17, 2023 at 07:30:59PM +0200, 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@xxxxxxxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # v5.8+
> > ---
> > drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 63 +++++++++++++++---------
> > drivers/gpu/drm/i915/gt/gen8_engine_cs.h | 3 +-
> > drivers/gpu/drm/i915/gt/intel_lrc.c | 17 +------
> > 3 files changed, 44 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > index fbc70f3b7f2fd..6d21a1ac06e73 100644
> > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> > @@ -165,7 +165,8 @@ 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 u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs,
> > + const i915_reg_t inv_reg)
> > {
> > u32 gsi_offset = gt->uncore->gsi_offset;
> >
> > @@ -187,6 +188,40 @@ u32 *gen12_emit_aux_table_inv(struct intel_gt *gt, u32 *cs, const i915_reg_t inv
> > return cs;
> > }
> >
> > +static i915_reg_t intel_get_aux_inv_reg(struct intel_engine_cs *engine)
>
> Generally we try to avoid putting "intel_" and "i915_" prefixes on
> static functions.
>
> > +{
> > + if (HAS_FLAT_CCS(engine->i915))
> > + return _MMIO(0);
> > +
> > + switch (engine->id) {
> > + case RCS0:
> > + return GEN12_CCS_AUX_INV;
> > + case VCS0:
> > + return GEN12_VD0_AUX_INV;
> > + case VCS2:
> > + return GEN12_VD2_AUX_INV;
> > + case VECS0:
> > + return GEN12_VE0_AUX_INV;
>
> We need CCS0 here (0x42c0). And on graphics versions 12.70 and beyond
> we also need BCS0 too (0x4248) since the blitter gained the ability to
> interpret CCS compression.
Thanks, will add.
> > + default:
> > + return _MMIO(0);
>
> It might be cleaner to use INVALID_MMIO_REG here (and then check for
> i915_mmio_reg_valid() below).
>
> > + }
> > +}
> > +
> > +static bool intel_engine_has_aux_inv(struct intel_engine_cs *engine)
> > +{
> > + i915_reg_t reg = intel_get_aux_inv_reg(engine);
> > +
> > + return !!reg.reg;
> > +}
> > +
> > +u32 *intel_emit_aux_table_inv(struct intel_engine_cs *engine, u32 *cs)
> > +{
> > + i915_reg_t reg = intel_get_aux_inv_reg(engine);
> > + struct intel_gt *gt = engine->gt;
> > +
> > + return reg.reg ? gen12_emit_aux_table_inv(gt, cs, reg) : cs;
> > +}
>
> Rather than adding this new wrapper function, can we just do the
> register lookup at the top of gen12_emit_aux_table_inv() (and bail out
> of that function early if there isn't a valid register)?
>
> Keeping the non-static function as the one with "gen12" in the name also
> helps reduce confusion about whether this is something that older
> platforms should have been calling as well.
You and Andrzej have made the same comments, will fix them.
BTW, this set of functions are doing more or less a similar thing
to what you have done here[*]. I will add the PVC flag.
Thank you!
Andi
[*] https://patchwork.freedesktop.org/patch/539304/?series=118334&rev=1
