Re: [RFC PATCH 2/5] drm/xe: Add MI_COPY_MEM_MEM GPU instruction definitions

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

On Fri, Jun 07, 2024 at 01:04:27PM +0200, Michal Wajdeczko wrote:
> 
> 
> On 07.06.2024 08:52, Matthew Brost wrote:
> > MI_COPY_MEM_MEM GPU instructions are used to copy ctx timestamp from a
> > LRC registers to another location at the beginning of every jobs
> > execution. Add MI_COPY_MEM_MEM GPU instruction definitions.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/instructions/xe_mi_commands.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/instructions/xe_mi_commands.h b/drivers/gpu/drm/xe/instructions/xe_mi_commands.h
> > index c74ceb550dce..f3deabb18ce4 100644
> > --- a/drivers/gpu/drm/xe/instructions/xe_mi_commands.h
> > +++ b/drivers/gpu/drm/xe/instructions/xe_mi_commands.h
> > @@ -56,6 +56,10 @@
> >  #define   MI_FLUSH_IMM_QW		REG_FIELD_PREP(MI_FLUSH_DW_LEN_DW, 5 - 2)
> >  #define   MI_FLUSH_DW_USE_GTT		REG_BIT(2)
> >  
> > +#define MI_COPY_MEM_MEM			(__MI_INSTR(0x2e) | 3)
> 
> all instruction definitions shall be in the opcode order, so move this
> one with 0x2e below 0x29
> 

Thanks - Will fix.

> and maybe instead of plain "3" better to use magic XE_INSTR_NUM_DW(5) ?
> 

I figured there was macro for this but wasn't immediately clear what to
use. TBH I don't love using a macro like this as it is quite clear in
bspec bits 7:0 the only valid value for this instruction is 3. Now in
our headers we have level of direction... I guess using the macro makes
more sense in variable length instructions but this is a fixed
instruction. Anyways can change if using the macro is the consensus.

> btw, IMO this latter macro would be better if defined as:
> 
> #define XE_INSTR_DW_LEN(dw)	REG_FIELD_PREP(GENMASK(7, 0), (dw))
> #define XE_INSTR_SIZE(size)	XE_INSTR_DW_LEN((size) - 2)
> 
> so for fixed instr length we could use XE_INSTR_DW_LEN(3) as in bspec
> 
> > +#define	  MI_COPY_MEM_MEM_SRC_GGTT	REG_BIT(22)
> > +#define	  MI_COPY_MEM_MEM_DST_GGTT	REG_BIT(21)
> > +
> 
> there should be just extra 2 spaces, not tab + spc + spc

Noticed that in PW link that alignment looked goofy. Will fix.

Matt

> 
> >  #define MI_LOAD_REGISTER_MEM		(__MI_INSTR(0x29) | XE_INSTR_NUM_DW(4))
> >  #define   MI_LRM_USE_GGTT		REG_BIT(22)
> >