[Intel-xe] [PATCH 05/26] drm/xe: Move register MMIO into xe_tile
Lucas De Marchi
lucas.demarchi at intel.com
Sat May 13 05:53:23 UTC 2023
On Wed, May 10, 2023 at 08:47:01PM -0700, Matt Roper wrote:
>Each tile has its own register region in the BAR, containing instances
>of all registers for the platform. In contrast, the multiple GTs within
>a tile share the same MMIO space; there's just a small subset of
>registers (the GSI registers) which have multiple copies at different
>offsets (0x0 for primary GT, 0x380000 for media GT). Move the register
>MMIO region size/pointers to the tile structure, leaving just the GSI
>offset information in the GT structure.
Side node:
I was not very confortable with the sentiment "the abstraction is
currently completely broken as gt is used to access registers that are
not per gt". I was not the one doing the previous abstraction, but I
understand it as a valid one too: the gt, just like uncore in i915, is
the leaf node that knows, depending on the register, what is the offset
to be applied, which essentially boils down to
tile_base + addr
tile_base + gsi_base + addr
gt->regs previously pointed to the right tile_base depending on what
they were ("normal" vs media-type). Here we are creating the tile
abstraction so we differentiate the a complete tile vs gt. It seems
cleaner and I like the direction.
>
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>---
> drivers/gpu/drm/xe/display/ext/i915_irq.c | 2 +-
> drivers/gpu/drm/xe/xe_device_types.h | 16 ++++++++++++++
> drivers/gpu/drm/xe/xe_ggtt.c | 3 ++-
> drivers/gpu/drm/xe/xe_gt_types.h | 9 +++-----
> drivers/gpu/drm/xe/xe_mmio.c | 26 ++++++++++++-----------
> drivers/gpu/drm/xe/xe_mmio.h | 21 +++++++++++++-----
> 6 files changed, 52 insertions(+), 25 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/display/ext/i915_irq.c b/drivers/gpu/drm/xe/display/ext/i915_irq.c
>index afde97b6faa6..a9cbd7b59360 100644
>--- a/drivers/gpu/drm/xe/display/ext/i915_irq.c
>+++ b/drivers/gpu/drm/xe/display/ext/i915_irq.c
>@@ -920,7 +920,7 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
>
> void gen11_display_irq_handler(struct drm_i915_private *i915)
> {
>- void __iomem * const regs = to_gt(i915)->mmio.regs;
>+ void __iomem * const regs = xe_device_get_root_tile(i915)->mmio.regs;
> const u32 disp_ctl = raw_reg_read(regs, GEN11_DISPLAY_INT_CTL);
>
> /*
>diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>index 5dcf1695925f..2481b2045284 100644
>--- a/drivers/gpu/drm/xe/xe_device_types.h
>+++ b/drivers/gpu/drm/xe/xe_device_types.h
>@@ -80,6 +80,22 @@ struct xe_tile {
> struct xe_gt primary_gt;
>
> /* TODO: Add media GT here */
>+
>+ /**
>+ * @mmio: MMIO info for a tile.
>+ *
>+ * Each tile has its own 16MB space in BAR0, laid out as:
>+ * * 0-4MB: registers
>+ * * 4MB-8MB: reserved
>+ * * 8MB-16MB: global GTT
>+ */
>+ struct {
>+ /** @size: size of tile's MMIO space */
>+ size_t size;
>+
>+ /** @regs: pointer to tile's MMIO space (starting with registers) */
>+ void *regs;
>+ } mmio;
> };
>
> /**
>diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
>index 546240261e0a..200976da3dc1 100644
>--- a/drivers/gpu/drm/xe/xe_ggtt.c
>+++ b/drivers/gpu/drm/xe/xe_ggtt.c
>@@ -93,6 +93,7 @@ static void ggtt_fini_noalloc(struct drm_device *drm, void *arg)
> int xe_ggtt_init_noalloc(struct xe_gt *gt, struct xe_ggtt *ggtt)
> {
> struct xe_device *xe = gt_to_xe(gt);
>+ struct xe_tile *tile = gt_to_tile(gt);
> struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> unsigned int gsm_size;
>
>@@ -106,7 +107,7 @@ int xe_ggtt_init_noalloc(struct xe_gt *gt, struct xe_ggtt *ggtt)
> return -ENOMEM;
> }
>
>- ggtt->gsm = gt->mmio.regs + SZ_8M;
>+ ggtt->gsm = tile->mmio.regs + SZ_8M;
> ggtt->size = (gsm_size / 8) * (u64) XE_PAGE_SIZE;
>
> if (IS_DGFX(xe) && xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)
>diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
>index c4376d50786b..03dd625b2781 100644
>--- a/drivers/gpu/drm/xe/xe_gt_types.h
>+++ b/drivers/gpu/drm/xe/xe_gt_types.h
>@@ -124,14 +124,11 @@ struct xe_gt {
> } info;
>
> /**
>- * @mmio: mmio info for GT, can be subset of the global device mmio
>- * space
>+ * @mmio: mmio info for GT. All GTs within a tile share the same
>+ * register space, but have their own copy of GSI registers at a
>+ * specific offset, as well as their own forcewake handling.
> */
> struct {
>- /** @size: size of MMIO space on GT */
>- size_t size;
>- /** @regs: pointer to MMIO space on GT */
>- void *regs;
> /** @fw: force wake for GT */
> struct xe_force_wake fw;
> /**
>diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>index 254b4a63d901..54fa1212fcd9 100644
>--- a/drivers/gpu/drm/xe/xe_mmio.c
>+++ b/drivers/gpu/drm/xe/xe_mmio.c
>@@ -307,6 +307,7 @@ static void xe_mmio_probe_tiles(struct xe_device *xe)
>
> if (xe->info.tile_count > 1) {
> const int mmio_bar = 0;
>+ struct xe_tile *tile;
> size_t size;
> void *regs;
>
>@@ -320,11 +321,11 @@ static void xe_mmio_probe_tiles(struct xe_device *xe)
> size = xe->mmio.size / adj_tile_count;
> regs = xe->mmio.regs;
>
>- for_each_gt(gt, xe, id) {
>- if (id && !xe_gt_is_media_type(gt))
>- regs += size;
>- gt->mmio.size = size;
>- gt->mmio.regs = regs;
>+ for_each_tile(tile, xe, id) {
>+ tile->mmio.size = size;
>+ tile->mmio.regs = regs;
it looks like the adj_tile_count meaning is now lost and w could have a
single tile_count variable and info printed?
also, I'm not sure there is much value to do
hardcoded -------->>----.
xe->mmio.size = SZ_16M * tile_count
and then do
size = xe->mmio.size / tile_count
size is always SZ_16M, as documented in the mmio struct.
AFAICS, this whole thing could now be simplified to:
mtcfg = xe_mmio_read64(gt, XEHP_MTCFG_ADDR);
xe->info.tile_count = REG_FIELD_GET(TILE_COUNT, mtcfg) + 1;
/* re-map IO to cover all tiles */
if (xe->info.tile_count > 1) {
pci_iounmap(to_pci_dev(xe->drm.dev), xe->mmio.regs);
xe->mmio.size = SZ_16M * xe->info.tile_count;
xe->mmio.regs = pci_iomap(to_pci_dev(xe->drm.dev),
mmio_bar, xe->mmio.size);
}
for_each_tile(tile, xe, id) {
tile->mmio.size = SZ_16M;
tile->mmio.regs = xe->mmio.regs + SZ_16M * id;
}
anyway, this is a small simplification. Patch looks correct to me.
Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>
Lucas De Marchi
>+
>+ regs += size;
> }
> }
> }
>@@ -340,15 +341,16 @@ static void mmio_fini(struct drm_device *drm, void *arg)
>
> int xe_mmio_init(struct xe_device *xe)
> {
>+ struct xe_tile *root_tile = xe_device_get_root_tile(xe);
> struct xe_gt *gt = xe_device_get_gt(xe, 0);
> const int mmio_bar = 0;
> int err;
>
> /*
>- * Map the entire BAR, which includes registers (0-4MB), reserved space
>- * (4MB-8MB), and GGTT (8MB-16MB). Other parts of the driver (GTs,
>- * GGTTs) will derive the pointers they need from the mapping in the
>- * device structure.
>+ * Map the first 16MB of th BAR, which includes the registers (0-4MB),
>+ * reserved space (4MB-8MB), and GGTT (8MB-16MB) for a single tile.
>+ * This will get remapped later if we determine that we're running
>+ * on a multi-tile system.
> */
> xe->mmio.size = SZ_16M;
> xe->mmio.regs = pci_iomap(to_pci_dev(xe->drm.dev), mmio_bar,
>@@ -362,9 +364,9 @@ int xe_mmio_init(struct xe_device *xe)
> if (err)
> return err;
>
>- /* 1 GT for now, 1 to 1 mapping, may change on multi-GT devices */
>- gt->mmio.size = xe->mmio.size;
>- gt->mmio.regs = xe->mmio.regs;
>+ /* Setup first tile; other tiles (if present) will be setup later. */
>+ root_tile->mmio.size = xe->mmio.size;
>+ root_tile->mmio.regs = xe->mmio.regs;
>
> /*
> * The boot firmware initializes local memory and assesses its health.
>diff --git a/drivers/gpu/drm/xe/xe_mmio.h b/drivers/gpu/drm/xe/xe_mmio.h
>index 1407f1189b0d..acf0b18f3111 100644
>--- a/drivers/gpu/drm/xe/xe_mmio.h
>+++ b/drivers/gpu/drm/xe/xe_mmio.h
>@@ -10,6 +10,7 @@
> #include <linux/io-64-nonatomic-lo-hi.h>
>
> #include "regs/xe_reg_defs.h"
>+#include "xe_device_types.h"
> #include "xe_gt_types.h"
>
> struct drm_device;
>@@ -20,27 +21,33 @@ int xe_mmio_init(struct xe_device *xe);
>
> static inline u8 xe_mmio_read8(struct xe_gt *gt, struct xe_reg reg)
> {
>+ struct xe_tile *tile = gt_to_tile(gt);
>+
> if (reg.addr < gt->mmio.adj_limit)
> reg.addr += gt->mmio.adj_offset;
>
>- return readb(gt->mmio.regs + reg.addr);
>+ return readb(tile->mmio.regs + reg.addr);
> }
>
> static inline void xe_mmio_write32(struct xe_gt *gt,
> struct xe_reg reg, u32 val)
> {
>+ struct xe_tile *tile = gt_to_tile(gt);
>+
> if (reg.addr < gt->mmio.adj_limit)
> reg.addr += gt->mmio.adj_offset;
>
>- writel(val, gt->mmio.regs + reg.addr);
>+ writel(val, tile->mmio.regs + reg.addr);
> }
>
> static inline u32 xe_mmio_read32(struct xe_gt *gt, struct xe_reg reg)
> {
>+ struct xe_tile *tile = gt_to_tile(gt);
>+
> if (reg.addr < gt->mmio.adj_limit)
> reg.addr += gt->mmio.adj_offset;
>
>- return readl(gt->mmio.regs + reg.addr);
>+ return readl(tile->mmio.regs + reg.addr);
> }
>
> static inline u32 xe_mmio_rmw32(struct xe_gt *gt, struct xe_reg reg, u32 clr,
>@@ -58,18 +65,22 @@ static inline u32 xe_mmio_rmw32(struct xe_gt *gt, struct xe_reg reg, u32 clr,
> static inline void xe_mmio_write64(struct xe_gt *gt,
> struct xe_reg reg, u64 val)
> {
>+ struct xe_tile *tile = gt_to_tile(gt);
>+
> if (reg.addr < gt->mmio.adj_limit)
> reg.addr += gt->mmio.adj_offset;
>
>- writeq(val, gt->mmio.regs + reg.addr);
>+ writeq(val, tile->mmio.regs + reg.addr);
> }
>
> static inline u64 xe_mmio_read64(struct xe_gt *gt, struct xe_reg reg)
> {
>+ struct xe_tile *tile = gt_to_tile(gt);
>+
> if (reg.addr < gt->mmio.adj_limit)
> reg.addr += gt->mmio.adj_offset;
>
>- return readq(gt->mmio.regs + reg.addr);
>+ return readq(tile->mmio.regs + reg.addr);
> }
>
> static inline int xe_mmio_write32_and_verify(struct xe_gt *gt,
>--
>2.40.0
>
More information about the Intel-xe
mailing list