Re: [PATCH v11 02/13] drm/xe/xelp: Quiesce memory traffic before invalidating auxccs

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Sep 01, 2025 at 11:37:33AM +0100, Tvrtko Ursulin wrote:
> 
> On 27/08/2025 15:39, Rodrigo Vivi wrote:
> > On Thu, Aug 21, 2025 at 03:14:44PM +0100, Tvrtko Ursulin wrote:
> > > According to i915 before invalidating auxccs we must quiesce the memory
> > > traffic by an extra flush.
> > 
> > Perhaps we should be more specific about what commits exactly are mentioning this?
> > 
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_ring_ops.c       | 7 ++++++-
> > >   drivers/gpu/drm/xe/xe_ring_ops_types.h | 2 +-
> > >   2 files changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c b/drivers/gpu/drm/xe/xe_ring_ops.c
> > > index 761740d7769f..f56506ebbeca 100644
> > > --- a/drivers/gpu/drm/xe/xe_ring_ops.c
> > > +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
> > > @@ -358,10 +358,15 @@ static void __emit_job_gen12_render_compute(struct xe_sched_job *job,
> > >   	struct xe_gt *gt = job->q->gt;
> > >   	struct xe_device *xe = gt_to_xe(gt);
> > >   	bool lacks_render = !(gt->info.engine_mask & XE_HW_ENGINE_RCS_MASK);
> > > +	const bool aux_ccs = has_aux_ccs(xe);
> > >   	u32 mask_flags = 0;
> > >   	i = emit_copy_timestamp(lrc, dw, i);
> > > +	/* hsdes: 1809175790 */
> > 
> > in i915 I just see a mention to this HSD which is the
> > 972282c4cf24 ("drm/i915/gen12: Add aux table invalidate for all engines")
> > 
> > but that is not about emitting the flush, but about fixing the aux
> > table invalidation itself. And that is in sync what written on this
> > HSD, but I don't believe this patch is about that.
> > 
> > So, although this patch itself kind of makes sense, I believe it
> > deserves a better message.
> 
> My logic was this:
> 
> d0d829e56674c (Daniele Ceraolo Spurio 2020-12-09 23:36:18 +0000 244) int
> gen12_emit_flush_rcs(struct i915_request *rq, u32 mode)
> d0d829e56674c (Daniele Ceraolo Spurio 2020-12-09 23:36:18 +0000 245) {
> 803efd297e315 (Daniele Ceraolo Spurio 2022-03-01 15:15:40 -0800 246) struct
> intel_engine_cs *engine = rq->engine;
> 803efd297e315 (Daniele Ceraolo Spurio 2022-03-01 15:15:40 -0800 247)
> ad8ebf12217e4 (Jonathan Cavitt        2023-07-25 02:19:46 +0200 248)    /*
> ad8ebf12217e4 (Jonathan Cavitt        2023-07-25 02:19:46 +0200 249) * On
> Aux CCS platforms the invalidation of the Aux
> ad8ebf12217e4 (Jonathan Cavitt        2023-07-25 02:19:46 +0200 250) * table
> requires quiescing memory traffic beforehand
> ad8ebf12217e4 (Jonathan Cavitt        2023-07-25 02:19:46 +0200 251)     */
> ad8ebf12217e4 (Jonathan Cavitt        2023-07-25 02:19:46 +0200 252) if
> (mode & EMIT_FLUSH || gen12_needs_ccs_aux_inv(engine)) {
> 
> So commit ad8ebf12217e4 added the flush before invalidate:
> 
> commit ad8ebf12217e451cd19804b1c3e97ad56491c74a
> Author: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Date:   Tue Jul 25 02:19:46 2023 +0200
> 
>     drm/i915/gt: Ensure memory quiesced before invalidation
> 
>     All memory traffic must be quiesced before requesting
>     an aux invalidation on platforms that use Aux CCS.
> 
>     Fixes: 972282c4cf24 ("drm/i915/gen12: Add aux table invalidate for all
> engines")
>     Requires: a2a4aa0eef3b ("drm/i915: Add the gen12_needs_ccs_aux_inv
> helper")
>     Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
>     Signed-off-by: Andi Shyti <andi.shyti@linux.intel.com>
>     Cc: <stable@vger.kernel.org> # v5.8+
>     Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
>     Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
>     Link: https://patchwork.freedesktop.org/patch/msgid/20230725001950.1014671-4-andi.shyti@linux.intel.com
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> index 46744f966077..58b448708e75 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> @@ -214,7 +214,11 @@ int gen12_emit_flush_rcs(struct i915_request *rq, u32
> mode)
>  {
>         struct intel_engine_cs *engine = rq->engine;
> 
> -       if (mode & EMIT_FLUSH) {
> +       /*
> +        * On Aux CCS platforms the invalidation of the Aux
> +        * table requires quiescing memory traffic beforehand
> +        */
> +       if (mode & EMIT_FLUSH || gen12_needs_ccs_aux_inv(engine)) {
> 
> And ad8ebf12217e4 referenced 972282c4cf24 as the commit it was fixing, while
> 972282c4cf24 is the one which references hsdes#1809175790. So I decided to
> tag this patch with it as a transitive property.
> 
> If I remove the HSD reference, replace it with a comment as the one in i915,
> and put the above commit chain reasoning to the commit message will that be
> good enough?

Yeap, I believe it makes more sense.

> 
> Regards,
> 
> Tvrtko
> 
> > > +	if (aux_ccs)
> > > +		i = emit_render_cache_flush(job, dw, i);
> > > +
> > >   	dw[i++] = preparser_disable(true);
> > >   	if (lacks_render)
> > >   		mask_flags = PIPE_CONTROL_3D_ARCH_FLAGS;
> > > @@ -372,7 +377,7 @@ static void __emit_job_gen12_render_compute(struct xe_sched_job *job,
> > >   	i = emit_pipe_invalidate(mask_flags, job->ring_ops_flush_tlb, dw, i);
> > >   	/* hsdes: 1809175790 */
> > > -	if (has_aux_ccs(xe))
> > > +	if (aux_ccs)
> > >   		i = emit_aux_table_inv(gt, CCS_AUX_INV, dw, i);
> > >   	dw[i++] = preparser_disable(false);
> > > diff --git a/drivers/gpu/drm/xe/xe_ring_ops_types.h b/drivers/gpu/drm/xe/xe_ring_ops_types.h
> > > index d7e3e150a9a5..477dc7defd72 100644
> > > --- a/drivers/gpu/drm/xe/xe_ring_ops_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_ring_ops_types.h
> > > @@ -8,7 +8,7 @@
> > >   struct xe_sched_job;
> > > -#define MAX_JOB_SIZE_DW 58
> > > +#define MAX_JOB_SIZE_DW 70
> > >   #define MAX_JOB_SIZE_BYTES (MAX_JOB_SIZE_DW * 4)
> > >   /**
> > > -- 
> > > 2.48.0
> > > 
>