Hi Andrzej,
On Mon, Jul 17, 2023 at 09:11:26PM +0200, Andrzej Hajda wrote:
> On 17.07.2023 19:30, 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.sh...@linux.intel.com>
> > Cc: <sta...@vger.kernel.org> # 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)
> > +{
> > + if (HAS_FLAT_CCS(engine->i915))
> > + return _MMIO(0);
>
> Why not INVALID_MMIO_REG ? Here and below.
>
> > +
> > + 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;
> > + default:
> > + return _MMIO(0);
> > + }
> > +}
> > +
> > +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;
>
> return i915_mmio_reg_valid(intel_get_aux_inv_reg(engine));
>
> > +}
> > +
> > +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;
> > +}
> > +
>
> I am not sure about prefixes, IMHO gen12_ instead of intel_ is more adequate
> as this is only gen12 feature, works only on gen12, and is called from gen12
> context, up to you. In any case we can squash intel_emit_aux_table_inv and
> gen12_emit_aux_table_inv into one function, am I right?
you and Matt have made exactly the same comments... will fix
them, Thank you!
Andi
