Re: [PATCH 02/12] drm/xe: Fix ring flush invalidation

[Matt Roper <matthew.d.roper@intel.com>]

On Fri, Feb 21, 2025 at 10:17:21AM +0000, Tvrtko Ursulin wrote:
> Emit_flush_invalidate() is incorrectly marking the write to LRC_PPHWSP as
> a GGTT write and also writing an atypical ~0 dword as the payload. Fix it.

Technically the write is a GGTT write (the PPHWSP is allocated as part
of the LRC, and the LRC is mapped to the GGTT).  But the important thing
here is that bit 21 (MI_FLUSH_DW_STORE_INDEX) redefines the meaning of
the PPGTT vs GGTT bit to instead mean "index into pphwsp" vs "index into
ghwsp."  And based on the name of the offset we use
(LRC_PPHWSP_FLUSH_INVAL_SCRATCH_ADDR) it sounds like the pphwsp was
indeed intended here, so dropping the 'use GTT' flag is in line with
that.

I don't think the actual destination (ghwsp vs pphwsp) or the value (~0
or 0) actually has any functional impact since there doesn't seem to be
anything watching and waiting for those writes to show up in memory.  We
just have to use the OP_STOREDW flag because the TLB invalidate flag
mandates that it also be present.

But your patch makes things more consistent, so

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> 
> While at it drop the unused flags argument.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> ---
>  drivers/gpu/drm/xe/xe_ring_ops.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c b/drivers/gpu/drm/xe/xe_ring_ops.c
> index 0c230ee53bba..a2d1fb8f0adf 100644
> --- a/drivers/gpu/drm/xe/xe_ring_ops.c
> +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
> @@ -111,16 +111,13 @@ static int emit_bb_start(u64 batch_addr, u32 ppgtt_flag, u32 *dw, int i)
>  	return i;
>  }
>  
> -static int emit_flush_invalidate(u32 flag, u32 *dw, int i)
> +static int emit_flush_invalidate(u32 *dw, int i)
>  {
> -	dw[i] = MI_FLUSH_DW;
> -	dw[i] |= flag;
> -	dw[i++] |= MI_INVALIDATE_TLB | MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_IMM_DW |
> -		MI_FLUSH_DW_STORE_INDEX;
> -
> -	dw[i++] = LRC_PPHWSP_FLUSH_INVAL_SCRATCH_ADDR | MI_FLUSH_DW_USE_GTT;
> +	dw[i++] = MI_FLUSH_DW | MI_INVALIDATE_TLB | MI_FLUSH_DW_OP_STOREDW |
> +		  MI_FLUSH_IMM_DW | MI_FLUSH_DW_STORE_INDEX;
> +	dw[i++] = LRC_PPHWSP_FLUSH_INVAL_SCRATCH_ADDR;
> +	dw[i++] = 0;
>  	dw[i++] = 0;
> -	dw[i++] = ~0U;
>  
>  	return i;
>  }
> @@ -409,7 +406,7 @@ static void emit_migration_job_gen12(struct xe_sched_job *job,
>  	if (!IS_SRIOV_VF(gt_to_xe(job->q->gt))) {
>  		/* XXX: Do we need this? Leaving for now. */
>  		dw[i++] = preparser_disable(true);
> -		i = emit_flush_invalidate(0, dw, i);
> +		i = emit_flush_invalidate(dw, i);
>  		dw[i++] = preparser_disable(false);
>  	}
>  
> -- 
> 2.48.0
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation