[Intel-xe] [PATCH 2/6] drm/xe: Separate number of registers from MI_LRI opcode

[Lucas De Marchi]

On Thu, Oct 12, 2023 at 04:04:20PM -0700, Matt Roper wrote:
>On Thu, Oct 12, 2023 at 03:55:41PM -0500, Lucas De Marchi wrote:
>> On Wed, Oct 11, 2023 at 04:10:00PM -0700, Matt Roper wrote:
>> > Keeping the number of registers to be loaded as a separate macro from
>> > the instruction opcode will simplify some upcoming LRC parsing code.
>> >
>> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>> > ---
>> > drivers/gpu/drm/xe/regs/xe_gpu_commands.h | 4 +++-
>> > drivers/gpu/drm/xe/xe_gt.c                | 2 +-
>> > drivers/gpu/drm/xe/xe_lrc.c               | 2 +-
>> > drivers/gpu/drm/xe/xe_ring_ops.c          | 2 +-
>> > 4 files changed, 6 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/regs/xe_gpu_commands.h b/drivers/gpu/drm/xe/regs/xe_gpu_commands.h
>> > index 9432a960346b..ad1e5466671b 100644
>> > --- a/drivers/gpu/drm/xe/regs/xe_gpu_commands.h
>> > +++ b/drivers/gpu/drm/xe/regs/xe_gpu_commands.h
>> > @@ -25,9 +25,11 @@
>> > #define MI_BATCH_BUFFER_END	MI_INSTR(0x0a, 0)
>> > #define MI_STORE_DATA_IMM	MI_INSTR(0x20, 0)
>> >
>> > -#define MI_LOAD_REGISTER_IMM(x)	MI_INSTR(0x22, 2*(x)-1)
>> > +#define MI_LOAD_REGISTER_IMM	MI_INSTR(0x22, 0)
>>
>> since we are going to have a churn in the codebase due to this, maybe
>> it's worth going ahead and doing a s/LOAD_REGISTER_IMMEDIATE/LRI/ so all
>> the macros follow the same pattern?
>
>You mean so that the instruction macro matches the field macro names?  I
>thought about that before but was uncertain whether it was worth
>switching the instruction macro names away from the official name given
>in the bspec.  If we do that for LOAD_REGISTER_IMM, we should probably
>do the same for all of the various LOAD and STORE instructions for
>consistency (although I guess MI_STORE_DATA_IMM is the only one we use
>at the moment).  I don't have a strong feeling either way; what do you
>think?

Thinking that you are printing to debugfs with #cmd, then that would be
user visible and deviating from the bspec. Doing the other way around
and always using the long version would make the code harder to read.

So.. meh, let's keep it as is.

Lucas De Marchi

>
>
>Matt
>
>>
>> Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>
>> Lucas De Marchi
>>
>> > #define   MI_LRI_LRM_CS_MMIO		REG_BIT(19)
>> > #define   MI_LRI_MMIO_REMAP_EN		REG_BIT(17)
>> > +#define   MI_LRI_LENGTH			GENMASK(5, 0)
>> > +#define   MI_LRI_NUM_REGS(x)		REG_FIELD_PREP(MI_LRI_LENGTH, 2 * (x) - 1)
>> > #define   MI_LRI_FORCE_POSTED		(1<<12)
>> >
>> > #define MI_FLUSH_DW		MI_INSTR(0x26, 0)
>> > diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>> > index c63e2e4750b1..a42ee3b9b8c7 100644
>> > --- a/drivers/gpu/drm/xe/xe_gt.c
>> > +++ b/drivers/gpu/drm/xe/xe_gt.c
>> > @@ -145,7 +145,7 @@ static int emit_wa_job(struct xe_gt *gt, struct xe_exec_queue *q)
>> > 	if (count) {
>> > 		xe_gt_dbg(gt, "LRC WA %s save-restore batch\n", sr->name);
>> >
>> > -		bb->cs[bb->len++] = MI_LOAD_REGISTER_IMM(count);
>> > +		bb->cs[bb->len++] = MI_LOAD_REGISTER_IMM | MI_LRI_NUM_REGS(count);
>> >
>> > 		xa_for_each(&sr->xa, idx, entry) {
>> > 			struct xe_reg reg = entry->reg;
>> > diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
>> > index 35ae6e531d8a..81463bd5e490 100644
>> > --- a/drivers/gpu/drm/xe/xe_lrc.c
>> > +++ b/drivers/gpu/drm/xe/xe_lrc.c
>> > @@ -111,7 +111,7 @@ static void set_offsets(u32 *regs,
>> > 		flags = *data >> 6;
>> > 		data++;
>> >
>> > -		*regs = MI_LOAD_REGISTER_IMM(count);
>> > +		*regs = MI_LOAD_REGISTER_IMM | MI_LRI_NUM_REGS(count);
>> > 		if (flags & POSTED)
>> > 			*regs |= MI_LRI_FORCE_POSTED;
>> > 		*regs |= MI_LRI_LRM_CS_MMIO;
>> > diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c b/drivers/gpu/drm/xe/xe_ring_ops.c
>> > index b95cc7713ff9..1e36b07d3e01 100644
>> > --- a/drivers/gpu/drm/xe/xe_ring_ops.c
>> > +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
>> > @@ -50,7 +50,7 @@ static u32 preparser_disable(bool state)
>> > static int emit_aux_table_inv(struct xe_gt *gt, struct xe_reg reg,
>> > 			      u32 *dw, int i)
>> > {
>> > -	dw[i++] = MI_LOAD_REGISTER_IMM(1) | MI_LRI_MMIO_REMAP_EN;
>> > +	dw[i++] = MI_LOAD_REGISTER_IMM | MI_LRI_NUM_REGS(1) | MI_LRI_MMIO_REMAP_EN;
>> > 	dw[i++] = reg.addr + gt->mmio.adj_offset;
>> > 	dw[i++] = AUX_INV;
>> > 	dw[i++] = MI_NOOP;
>> > --
>> > 2.41.0
>> >
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation