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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

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

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

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

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