[Intel-xe] [PATCH 4/4] drm/xe: Sort xe_regs.h
Matt Roper
matthew.d.roper at intel.com
Wed Jul 26 17:36:36 UTC 2023
On Wed, Jul 26, 2023 at 11:46:09AM -0300, Lucas De Marchi wrote:
> 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?
I think these should probably move to xe_engine_regs.h, at the very top.
They're not registers themselves, but rather the expected parameters
that will be passed to every register in that 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?
I don't have a strong feeling about this one, but I mostly just leave
the old definitions where they are and add the new ones under the new
register.
Matt
>
> 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
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
More information about the Intel-xe
mailing list