[Intel-xe] [PATCH 4/4] drm/xe: Sort xe_regs.h

Wed Jul 26 14:46:09 UTC 2023

On Tue, Jul 25, 2023 at 03:55:15PM -0700, Matt Roper wrote:
>On Tue, Jul 25, 2023 at 01:28:36PM -0700, Lucas De Marchi wrote:
>> Sort it by register address, but  keeping a few exceptions when a
>> register for a platform changed the address, but has the same
>> functionality as an already existing register.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/regs/xe_regs.h | 56 +++++++++++++++----------------
>>  1 file changed, 28 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_regs.h b/drivers/gpu/drm/xe/regs/xe_regs.h
>> index 3f7052fec6cc..0b3607e0ba1c 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_regs.h
>> @@ -7,9 +7,6 @@
>>
>>  #include "regs/xe_reg_defs.h"
>>
>> -#define GU_CNTL					XE_REG(0x101010)
>> -#define   LMEM_INIT				REG_BIT(7)
>> -
>>  #define RENDER_RING_BASE			0x02000
>>  #define BSD_RING_BASE				0x1c0000
>>  #define BSD2_RING_BASE				0x1c4000
>> @@ -45,36 +42,15 @@
>>  #define FF_THREAD_MODE				XE_REG(0x20a0)
>
>Looks like there are some registers that don't really belong in this
>file.  We'll need to move at least this one to the engine register
>header at some point.  Actually we should probably move the *_RING_BASE
>registers as well.  But that can all happen in a future patch.
>
>>  #define   FF_TESSELATION_DOP_GATE_DISABLE	BIT(19)
>>
>> -#define PVC_RP_STATE_CAP			XE_REG(0x281014)
>> -#define MTL_RP_STATE_CAP			XE_REG(0x138000)
>> -
>> -#define MTL_MEDIAP_STATE_CAP			XE_REG(0x138020)
>> -#define   MTL_RP0_CAP_MASK			REG_GENMASK(8, 0)
>> -#define   MTL_RPN_CAP_MASK			REG_GENMASK(24, 16)
>> -
>> -#define MTL_GT_RPE_FREQUENCY			XE_REG(0x13800c)
>> -#define MTL_MPE_FREQUENCY			XE_REG(0x13802c)
>> -#define   MTL_RPE_MASK				REG_GENMASK(8, 0)
>> -
>> -#define SOFTWARE_FLAGS_SPR33			XE_REG(0x4f084)
>> +#define TIMESTAMP_OVERRIDE					XE_REG(0x44074)
>> +#define   TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_MASK	REG_GENMASK(15, 12)
>> +#define   TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_MASK		REG_GENMASK(9, 0)
>>
>>  #define PCU_IRQ_OFFSET				0x444e0
>>  #define GU_MISC_IRQ_OFFSET			0x444f0
>>  #define   GU_MISC_GSE				REG_BIT(27)
>>
>> -#define GFX_MSTR_IRQ				XE_REG(0x190010)
>> -#define   MASTER_IRQ				REG_BIT(31)
>> -#define   GU_MISC_IRQ				REG_BIT(29)
>> -#define   DISPLAY_IRQ				REG_BIT(16)
>> -#define   GT_DW_IRQ(x)				REG_BIT(x)
>> -
>> -#define DG1_MSTR_TILE_INTR			XE_REG(0x190008)
>> -#define   DG1_MSTR_IRQ				REG_BIT(31)
>> -#define   DG1_MSTR_TILE(t)			REG_BIT(t)
>> -
>> -#define TIMESTAMP_OVERRIDE					XE_REG(0x44074)
>> -#define   TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_MASK	REG_GENMASK(15, 12)
>> -#define   TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_MASK		REG_GENMASK(9, 0)
>> +#define SOFTWARE_FLAGS_SPR33			XE_REG(0x4f084)
>>
>>  #define GGC					XE_REG(0x108040)
>>  #define   GMS_MASK				REG_GENMASK(15, 8)
>> @@ -87,4 +63,28 @@
>>  #define STOLEN_RESERVED				XE_REG(0x1082C0)
>
>We should also make sure we're writing hex in lowercase consistently
>too.  Again, that can be done in a future patch.

yeah, I mistakenly added this is uppercase in the previous patch since I
copied the value from the other register that is also uppercase.

>
>>  #define   WOPCM_SIZE_MASK			REG_GENMASK64(8, 7)
>>
>> +#define GU_CNTL					XE_REG(0x101010)
>> +#define   LMEM_INIT				REG_BIT(7)
>> +
>> +#define MTL_RP_STATE_CAP			XE_REG(0x138000)
>> +#define PVC_RP_STATE_CAP			XE_REG(0x281014)
>
>I think it's better to keep consistent sorting rather than trying to
>keep registers that moved between platforms in the same place.  Too many
>exceptions like this ultimately leads to chaos when people aren't sure
>how far they can stretch the exceptions.

Agreed, but we probably should write down the rules. It would be good to
have some rules that will allow use to eventually convert this to an
autogenerated file. However we have the following situations:

1) Regardless of moving the engine bases,

#define RENDER_RING_BASE		0x02000
#define BSD_RING_BASE			0x1c0000

would we leave them spreadout through the file?

2) When the register keeps the same definition and just changes the
offset for a new platform, what to do with the bitfield definition?

Lucas De Marchi

>
>> +
>> +#define MTL_MEDIAP_STATE_CAP			XE_REG(0x138020)
>> +#define   MTL_RPN_CAP_MASK			REG_GENMASK(24, 16)
>> +#define   MTL_RP0_CAP_MASK			REG_GENMASK(8, 0)
>> +
>> +#define MTL_GT_RPE_FREQUENCY			XE_REG(0x13800c)
>
>It looks like this one should be slightly earlier (0x13800c < 0x138020).
>
>
>Matt
>
>> +#define MTL_MPE_FREQUENCY			XE_REG(0x13802c)
>> +#define   MTL_RPE_MASK				REG_GENMASK(8, 0)
>> +
>> +#define DG1_MSTR_TILE_INTR			XE_REG(0x190008)
>> +#define   DG1_MSTR_IRQ				REG_BIT(31)
>> +#define   DG1_MSTR_TILE(t)			REG_BIT(t)
>> +
>> +#define GFX_MSTR_IRQ				XE_REG(0x190010)
>> +#define   MASTER_IRQ				REG_BIT(31)
>> +#define   GU_MISC_IRQ				REG_BIT(29)
>> +#define   DISPLAY_IRQ				REG_BIT(16)
>> +#define   GT_DW_IRQ(x)				REG_BIT(x)
>> +
>>  #endif
>> --
>> 2.40.1
>>
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation