| Message ID | 20230721161514.818895-8-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update AUX invalidation sequence | expand |
On 21.07.2023 18:15, Andi Shyti wrote: > Commit af9e423a8aae ("drm/i915/gt: Ensure memory quiesced before > invalidation") has made sure that the memory is quiesced before > invalidating the AUX CCS table. Do it for all the other engines > and not just RCS. > > 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 | 36 ++++++++++++++++-------- > 1 file changed, 25 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > index 5e19b45a5cabe..646151e1b5deb 100644 > --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c > @@ -331,26 +331,40 @@ 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, *cs; > + u32 cmd_flush = 0; > + u32 cmd = 4; > + u32 *cs; > > - cmd = 4; > - if (mode & EMIT_INVALIDATE) { > + if (mode & EMIT_INVALIDATE) > cmd += 2; > > - if (gen12_needs_ccs_aux_inv(rq->engine) && > - (rq->engine->class == VIDEO_DECODE_CLASS || > - rq->engine->class == VIDEO_ENHANCEMENT_CLASS)) { > - aux_inv = rq->engine->mask & > - ~GENMASK(_BCS(I915_MAX_BCS - 1), BCS0); > - if (aux_inv) > - cmd += 4; > - } > + if (gen12_needs_ccs_aux_inv(rq->engine)) > + aux_inv = rq->engine->mask & > + ~GENMASK(_BCS(I915_MAX_BCS - 1), BCS0); Shouldn't we remove BCS check for MTL? And move it inside gen12_needs_ccs_aux_inv? Btw aux_inv is used as bool, make better is to make it bool. Regards Andrzej > + > + /* > + * On Aux CCS platforms the invalidation of the Aux > + * table requires quiescing memory traffic beforehand > + */ > + if (aux_inv) { > + cmd += 4; /* for the AUX invalidation */ > + cmd += 2; /* for the engine quiescing */ > + > + cmd_flush = MI_FLUSH_DW; > + > + if (rq->engine->class == COPY_ENGINE_CLASS) > + cmd_flush |= MI_FLUSH_DW_CCS; > } > > cs = intel_ring_begin(rq, cmd); > if (IS_ERR(cs)) > return PTR_ERR(cs); > > + if (cmd_flush) { > + *cs++ = cmd_flush; > + *cs++ = 0; > + } > + > if (mode & EMIT_INVALIDATE) > *cs++ = preparser_disable(true); >
Hi Andrzej, > > intel_engine_mask_t aux_inv = 0; > > - u32 cmd, *cs; > > + u32 cmd_flush = 0; > > + u32 cmd = 4; > > + u32 *cs; > > - cmd = 4; > > - if (mode & EMIT_INVALIDATE) { > > + if (mode & EMIT_INVALIDATE) > > cmd += 2; > > - if (gen12_needs_ccs_aux_inv(rq->engine) && > > - (rq->engine->class == VIDEO_DECODE_CLASS || > > - rq->engine->class == VIDEO_ENHANCEMENT_CLASS)) { > > - aux_inv = rq->engine->mask & > > - ~GENMASK(_BCS(I915_MAX_BCS - 1), BCS0); > > - if (aux_inv) > > - cmd += 4; > > - } > > + if (gen12_needs_ccs_aux_inv(rq->engine)) > > + aux_inv = rq->engine->mask & > > + ~GENMASK(_BCS(I915_MAX_BCS - 1), BCS0); > > Shouldn't we remove BCS check for MTL? And move it inside > gen12_needs_ccs_aux_inv? > Btw aux_inv is used as bool, make better is to make it bool. Both the cleanups come in patch 9. I wanted to move it initially before, but per engine check come later in the series. I think would need to re-architecture all the patch structure if I want to remove it :) Are you strong with this change? Andi
On 24.07.2023 11:14, Andi Shyti wrote: > Hi Andrzej, > >>> intel_engine_mask_t aux_inv = 0; >>> - u32 cmd, *cs; >>> + u32 cmd_flush = 0; >>> + u32 cmd = 4; >>> + u32 *cs; >>> - cmd = 4; >>> - if (mode & EMIT_INVALIDATE) { >>> + if (mode & EMIT_INVALIDATE) >>> cmd += 2; >>> - if (gen12_needs_ccs_aux_inv(rq->engine) && >>> - (rq->engine->class == VIDEO_DECODE_CLASS || >>> - rq->engine->class == VIDEO_ENHANCEMENT_CLASS)) { >>> - aux_inv = rq->engine->mask & >>> - ~GENMASK(_BCS(I915_MAX_BCS - 1), BCS0); >>> - if (aux_inv) >>> - cmd += 4; >>> - } >>> + if (gen12_needs_ccs_aux_inv(rq->engine)) >>> + aux_inv = rq->engine->mask & >>> + ~GENMASK(_BCS(I915_MAX_BCS - 1), BCS0); >> Shouldn't we remove BCS check for MTL? And move it inside >> gen12_needs_ccs_aux_inv? >> Btw aux_inv is used as bool, make better is to make it bool. > Both the cleanups come in patch 9. I wanted to move it initially > before, but per engine check come later in the series. > > I think would need to re-architecture all the patch structure if > I want to remove it :) > > Are you strong with this change? Nope, if it finally arrives then OK for me. Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com> Regards Andrzej > > Andi
diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c index 5e19b45a5cabe..646151e1b5deb 100644 --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c @@ -331,26 +331,40 @@ 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, *cs; + u32 cmd_flush = 0; + u32 cmd = 4; + u32 *cs; - cmd = 4; - if (mode & EMIT_INVALIDATE) { + if (mode & EMIT_INVALIDATE) cmd += 2; - if (gen12_needs_ccs_aux_inv(rq->engine) && - (rq->engine->class == VIDEO_DECODE_CLASS || - rq->engine->class == VIDEO_ENHANCEMENT_CLASS)) { - aux_inv = rq->engine->mask & - ~GENMASK(_BCS(I915_MAX_BCS - 1), BCS0); - if (aux_inv) - cmd += 4; - } + if (gen12_needs_ccs_aux_inv(rq->engine)) + 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) { + cmd += 4; /* for the AUX invalidation */ + cmd += 2; /* for the engine quiescing */ + + cmd_flush = MI_FLUSH_DW; + + if (rq->engine->class == COPY_ENGINE_CLASS) + cmd_flush |= MI_FLUSH_DW_CCS; } cs = intel_ring_begin(rq, cmd); if (IS_ERR(cs)) return PTR_ERR(cs); + if (cmd_flush) { + *cs++ = cmd_flush; + *cs++ = 0; + } + if (mode & EMIT_INVALIDATE) *cs++ = preparser_disable(true);
Commit af9e423a8aae ("drm/i915/gt: Ensure memory quiesced before invalidation") has made sure that the memory is quiesced before invalidating the AUX CCS table. Do it for all the other engines and not just RCS. 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 | 36 ++++++++++++++++-------- 1 file changed, 25 insertions(+), 11 deletions(-)