Re: [PATCH] drm/xe: Add POST_SYNC prefix to post sync instructions

[Matthew Brost <matthew.brost@intel.com>]

On Mon, May 12, 2025 at 06:42:03AM +0000, Bhadane, Dnyaneshwar wrote:
> 
> 
> > -----Original Message-----
> > From: Vodapalli, Ravi Kumar <ravi.kumar.vodapalli@intel.com>
> > Sent: Thursday, April 24, 2025 3:00 PM
> > To: intel-xe@lists.freedesktop.org
> > Cc: Vivekanandan, Balasubramani <balasubramani.vivekanandan@intel.com>;
> > Roper, Matthew D <matthew.d.roper@intel.com>; De Marchi, Lucas
> > <lucas.demarchi@intel.com>; Sousa, Gustavo <gustavo.sousa@intel.com>;
> > Taylor, Clinton A <clinton.a.taylor@intel.com>; Atwood, Matthew S
> > <matthew.s.atwood@intel.com>; Bhadane, Dnyaneshwar
> > <dnyaneshwar.bhadane@intel.com>; Kalvala, Haridhar
> > <haridhar.kalvala@intel.com>; Chauhan, Shekhar
> > <shekhar.chauhan@intel.com>
> > Subject: [PATCH] drm/xe: Add POST_SYNC prefix to post sync instructions
> > 
> > In existing code for PIPE CONTROL post sync operation instructions
> > POST_SYNC prefix is not present in the name, add it so that it will represent it
> > is a post sync operation type.
> Is there a specific reason for this renaming? Is it truly necessary?
> Regardless, the terms PIPE_CONTROL_POST_SYNC_TYPE_QW_WRITE and PIPE_CONTROL_POST_SYNC_OP_TYPE_QW_WRITE would be more descriptive. 
> If renaming is indeed required, it should be applied to PIPE_CONTROL_WRITE_TIMESTAMP. 
> This refers to all the post-sync operation types defined and used in the code within the Xe/i915 context.
> 
> Renaming can clutter the Git history with unnecessary changes, making it harder to track the evolution of the code and understand the reasons behind certain modification.

I don't think we should consider git history too much in when making
choices about renaming. If this is a better name, more inline with bspec
I'd say let's use a better name. If we need to rename more operations
with post-sync prefixes, let's also do that.

Matt 

> 
> Regards,
> Dnyaneshwar
> > 
> > Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@intel.com>
> > ---
> >  drivers/gpu/drm/xe/instructions/xe_gpu_commands.h | 2 +-
> >  drivers/gpu/drm/xe/xe_ring_ops.c                  | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/instructions/xe_gpu_commands.h
> > b/drivers/gpu/drm/xe/instructions/xe_gpu_commands.h
> > index 8cfcd3360896..75efdacc4979 100644
> > --- a/drivers/gpu/drm/xe/instructions/xe_gpu_commands.h
> > +++ b/drivers/gpu/drm/xe/instructions/xe_gpu_commands.h
> > @@ -54,7 +54,7 @@
> >  #define   PIPE_CONTROL_GLOBAL_SNAPSHOT_RESET		(1<<19)
> >  #define	  PIPE_CONTROL_TLB_INVALIDATE
> > 	BIT(18)
> >  #define   PIPE_CONTROL_PSD_SYNC				(1<<17)
> > -#define   PIPE_CONTROL_QW_WRITE				(1<<14)
> > +#define   PIPE_CONTROL_POST_SYNC_QW_WRITE		(1<<14)
> >  #define   PIPE_CONTROL_DEPTH_STALL			(1<<13)
> >  #define   PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH	(1<<12)
> >  #define   PIPE_CONTROL_INSTRUCTION_CACHE_INVALIDATE	(1<<11)
> > diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c
> > b/drivers/gpu/drm/xe/xe_ring_ops.c
> > index a7582b097ae6..da50213b78c5 100644
> > --- a/drivers/gpu/drm/xe/xe_ring_ops.c
> > +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
> > @@ -145,7 +145,7 @@ static int emit_pipe_invalidate(u32 mask_flags, bool
> > invalidate_tlb, u32 *dw,
> >  		PIPE_CONTROL_VF_CACHE_INVALIDATE |
> >  		PIPE_CONTROL_CONST_CACHE_INVALIDATE |
> >  		PIPE_CONTROL_STATE_CACHE_INVALIDATE |
> > -		PIPE_CONTROL_QW_WRITE |
> > +		PIPE_CONTROL_POST_SYNC_QW_WRITE |
> >  		PIPE_CONTROL_STORE_DATA_INDEX;
> > 
> >  	if (invalidate_tlb)
> > @@ -216,7 +216,7 @@ static int emit_pipe_imm_ggtt(u32 addr, u32 value,
> > bool stall_only, u32 *dw,
> >  			      int i)
> >  {
> >  	u32 flags = PIPE_CONTROL_CS_STALL |
> > PIPE_CONTROL_GLOBAL_GTT_IVB |
> > -		    PIPE_CONTROL_QW_WRITE;
> > +		    PIPE_CONTROL_POST_SYNC_QW_WRITE;
> > 
> >  	if (!stall_only)
> >  		flags |= PIPE_CONTROL_FLUSH_ENABLE;
> > --
> > 2.25.1
>