[PATCH v2] drm/xe: Use function to emit PIPE_CONTROL

[Matt Roper]

On Mon, Jan 29, 2024 at 01:15:09PM -0800, José Roberto de Souza wrote:
> This reduces code duplication in xe_ring_ops.
> 
> v2:
> - fix flags of emit_pipe_imm_ggtt()
> - reduce to only one function
> 
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_ring_ops.c | 57 ++++++++++++++------------------
>  1 file changed, 24 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c b/drivers/gpu/drm/xe/xe_ring_ops.c
> index 1e4c06eacd984..be488d55325cc 100644
> --- a/drivers/gpu/drm/xe/xe_ring_ops.c
> +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
> @@ -113,6 +113,19 @@ static int emit_flush_invalidate(u32 flag, u32 *dw, int i)
>  	return i;
>  }
>  
> +static int
> +emit_pipe_control(u32 *dw, int i, u32 bit_group_0, u32 bit_group_1, u32 offset, u32 value)
> +{
> +	dw[i++] = GFX_OP_PIPE_CONTROL(6) | bit_group_0;
> +	dw[i++] = bit_group_1;
> +	dw[i++] = offset;
> +	dw[i++] = 0;
> +	dw[i++] = value;
> +	dw[i++] = 0;
> +
> +	return i;
> +}
> +
>  static int emit_pipe_invalidate(u32 mask_flags, bool invalidate_tlb, u32 *dw,
>  				int i)
>  {
> @@ -131,14 +144,7 @@ static int emit_pipe_invalidate(u32 mask_flags, bool invalidate_tlb, u32 *dw,
>  
>  	flags &= ~mask_flags;
>  
> -	dw[i++] = GFX_OP_PIPE_CONTROL(6);
> -	dw[i++] = flags;
> -	dw[i++] = LRC_PPHWSP_SCRATCH_ADDR;
> -	dw[i++] = 0;
> -	dw[i++] = 0;
> -	dw[i++] = 0;
> -
> -	return i;
> +	return emit_pipe_control(dw, i, 0, flags, LRC_PPHWSP_SCRATCH_ADDR, 0);
>  }
>  
>  static int emit_store_imm_ppgtt_posted(u64 addr, u64 value,
> @@ -174,14 +180,7 @@ static int emit_render_cache_flush(struct xe_sched_job *job, u32 *dw, int i)
>  	else if (job->q->class == XE_ENGINE_CLASS_COMPUTE)
>  		flags &= ~PIPE_CONTROL_3D_ENGINE_FLAGS;
>  
> -	dw[i++] = GFX_OP_PIPE_CONTROL(6) | PIPE_CONTROL0_HDC_PIPELINE_FLUSH;
> -	dw[i++] = flags;
> -	dw[i++] = 0;
> -	dw[i++] = 0;
> -	dw[i++] = 0;
> -	dw[i++] = 0;
> -
> -	return i;
> +	return emit_pipe_control(dw, i, PIPE_CONTROL0_HDC_PIPELINE_FLUSH, flags, 0, 0);
>  }
>  
>  static int emit_pipe_control_to_ring_end(struct xe_hw_engine *hwe, u32 *dw, int i)
> @@ -189,14 +188,9 @@ static int emit_pipe_control_to_ring_end(struct xe_hw_engine *hwe, u32 *dw, int
>  	if (hwe->class != XE_ENGINE_CLASS_RENDER)
>  		return i;
>  
> -	if (XE_WA(hwe->gt, 16020292621)) {
> -		dw[i++] = GFX_OP_PIPE_CONTROL(6);
> -		dw[i++] = PIPE_CONTROL_LRI_POST_SYNC;
> -		dw[i++] = RING_NOPID(hwe->mmio_base).addr;
> -		dw[i++] = 0;
> -		dw[i++] = 0;
> -		dw[i++] = 0;
> -	}
> +	if (XE_WA(hwe->gt, 16020292621))
> +		i = emit_pipe_control(dw, i, 0, PIPE_CONTROL_LRI_POST_SYNC,
> +				      RING_NOPID(hwe->mmio_base).addr, 0);
>  
>  	return i;
>  }
> @@ -204,16 +198,13 @@ static int emit_pipe_control_to_ring_end(struct xe_hw_engine *hwe, u32 *dw, int
>  static int emit_pipe_imm_ggtt(u32 addr, u32 value, bool stall_only, u32 *dw,
>  			      int i)
>  {
> -	dw[i++] = GFX_OP_PIPE_CONTROL(6);
> -	dw[i++] = (stall_only ? PIPE_CONTROL_CS_STALL :
> -		   PIPE_CONTROL_FLUSH_ENABLE | PIPE_CONTROL_CS_STALL) |
> -		PIPE_CONTROL_GLOBAL_GTT_IVB | PIPE_CONTROL_QW_WRITE;
> -	dw[i++] = addr;
> -	dw[i++] = 0;
> -	dw[i++] = value;
> -	dw[i++] = 0; /* We're thrashing one extra dword. */
> +	u32 flags = PIPE_CONTROL_CS_STALL | PIPE_CONTROL_GLOBAL_GTT_IVB |
> +		    PIPE_CONTROL_QW_WRITE;
>  
> -	return i;
> +	if (stall_only)

I think this is supposed to be !stall_only, right?


I was going to suggest still adding assertions to catch "easy" mistakes
in emit_pipe_control() like not passing an address when a post-sync
operation is requested, but then I realized you don't have any kind of
gt/xe pointer in any of these functions, so plumbing that down to the
necessary level would be extra work.  Maybe we can do that in the future
(I think we'll have to plumb those down at some point soon for XE_WA()
matching), but we don't need to worry about it for this patch.

So aside from the stall_only condition inversion,

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


> +		flags |= PIPE_CONTROL_FLUSH_ENABLE;
> +
> +	return emit_pipe_control(dw, i, 0, flags, addr, value);
>  }
>  
>  static u32 get_ppgtt_flag(struct xe_sched_job *job)
> -- 
> 2.43.0
> 

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