[PATCH] drm/xe: Remove unused "mmio_ext" code
Matt Roper
matthew.d.roper at intel.com
Tue Jan 21 16:39:42 UTC 2025
On Wed, Jan 15, 2025 at 11:46:10AM +0200, Levi, Ilia wrote:
> On 07/01/2025 1:43, Matt Roper wrote:
> > The "mmio_ext" and 'REG_EXT" code is currently unused on any existing
> > platform. Going forward, this also isn't the design we want to use for
> > any future platforms/features either, so we should just go ahead and
> > remove the dead code to avoid confusion.
> >
> > mmio_ext was originally added in an attempt to hack around the early
> > (mis)design of the Xe driver, which used xe_gt as the target for all
> > register MMIO access, even those completely unrelated to the GT subunit
> > of the hardware. With the introduction of commit 34953ee349dd ("drm/xe:
> > Create dedicated xe_mmio structure") and its follow-up patches, that
> > misdesign has been corrected and access to register MMIO regions
> > specific to hardware units is now done through xe_mmio structures which
> > encapsulate an iomap, region size, and some other metadata.
> >
> > Although all of the registers used by the driver today happen to fall
> > within one specific PCI BAR region, and thus re-use a single device-wide
> > iomap, there's no requirement that this stay true for future platforms
> > or features. I.e., if a future platform adds a new 'foo' hardware unit
> > that exists at a different area in the BAR, or even in a completely
> > different BAR, then that would be handled by doing a separate iomap of
> > that unit's register region and wrapping it in its own 'struct xe_mmio
> > foo_regs' structure. The pointer to the new 'foo_regs' could be placed
> > within the xe_device, xe_tile, xe_gt, etc., according to where the new
> > hardware unit falls within the current hardware hierarchy.
> So this approach suggests that each instance of struct xe_mmio should be
> initialized with its own iomap() call? If so, I think there's more work
No, using a separate iomap is an option rather than a requirement; it
really comes down to what makes the most sense. For example we already
have gt->mmio which shares an iomap with tile->mmio since the prior is a
subset of the latter, and they share the same natural offsets in the
bspec (i.e., the register offsets are from the beginning of GTTMMADR).
If you have a new hardware unit that is truly self-contained and has its
own register offset scheme (i.e., offsets are expressed as values from
the beginning of some other base rather than the GTTMMADR) then it makes
a lot more sense to use a separate iomap and use the natural offsets for
that hardware unit.
> to be done here, something like:
> 1. get rid of xe->mmio
> 2. refactor mmio_multi_tile_setup (each tile has its own xe_mmio)
Yeah, at the moment each of our tiles is sharing an iomap for historical
reasons. It probably does make sense to split that up now (since each
tile has its own unique offsets) and there's no specific reason to keep
the old unified iomap around. I'll send some patches for that in a bit.
>
> - Ilia
> >
> > This effectively reverts the following commits, although parts of these
> > commits had already vanished or changed with the earlier xe_mmio
> > refactor work:
> >
> > - commit 399a13323f0d ("drm/xe: add 28-bit address support in struct
> > xe_reg")
> > - commit fdef72e02e20 ("drm/xe: add a flag to bypass multi-tile config
> > from MTCFG reg")
> > - commit 866b2b176434 ("drm/xe: add MMIO extension support flags")
> > - commit ef29b390c734 ("drm/xe: map MMIO BAR according to the num of
> > tiles in device desc")
> > - commit a4e2f3a299ea ("drm/xe: refactor xe_mmio_probe_tiles to support
> > MMIO extension")
> >
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: Koby Elbaz <kelbaz at habana.ai>
> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > ---
> > drivers/gpu/drm/xe/regs/xe_reg_defs.h | 16 +----------
> > drivers/gpu/drm/xe/xe_device_types.h | 11 --------
> > drivers/gpu/drm/xe/xe_mmio.c | 39 ---------------------------
> > drivers/gpu/drm/xe/xe_pci.c | 3 ---
> > drivers/gpu/drm/xe/xe_pci_types.h | 2 --
> > 5 files changed, 1 insertion(+), 70 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/regs/xe_reg_defs.h b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
> > index 51fd40ffafcb..0234d1e84139 100644
> > --- a/drivers/gpu/drm/xe/regs/xe_reg_defs.h
> > +++ b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
> > @@ -21,7 +21,7 @@ struct xe_reg {
> > union {
> > struct {
> > /** @addr: address */
> > - u32 addr:28;
> > + u32 addr:22;
> This will limit the xe_mmio read/write operations up to 8MiB offset across
> all xe_mmio instances. It is enough for now, but we might want to leave it
> as is for future platforms. Also, this means that setting regs_size in
We can always raise the size again if/when an actual user shows up.
That way it will be more obvious what's going on and we can review the
changes together. But adding an assertion sounds like a good idea.
Matt
> xe_mmio struct to anything bigger is meaningless and probably a bug.
> Maybe it's worth to add an initializer to xe_mmio struct that asserts this?
>
> - Ilia
> > /**
> > * @masked: register is "masked", with upper 16bits used
> > * to identify the bits that are updated on the lower
> > @@ -41,10 +41,6 @@ struct xe_reg {
> > * @vf: register is accessible from the Virtual Function.
> > */
> > u32 vf:1;
> > - /**
> > - * @ext: access MMIO extension space for current register.
> > - */
> > - u32 ext:1;
> > };
> > /** @raw: Raw value with both address and options */
> > u32 raw;
> > @@ -111,16 +107,6 @@ struct xe_reg_mcr {
> > */
> > #define XE_REG(r_, ...) ((const struct xe_reg)XE_REG_INITIALIZER(r_, ##__VA_ARGS__))
> >
> > -/**
> > - * XE_REG_EXT - Create a struct xe_reg from extension offset and additional
> > - * flags
> > - * @r_: Register extension offset
> > - * @...: Additional options like access mode. See struct xe_reg for available
> > - * options.
> > - */
> > -#define XE_REG_EXT(r_, ...) \
> > - ((const struct xe_reg)XE_REG_INITIALIZER(r_, ##__VA_ARGS__, .ext = 1))
> > -
> > /**
> > * XE_REG_MCR - Create a struct xe_reg_mcr from offset and additional flags
> > * @r_: Register offset
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > index 8a7b15972413..16ebb2859877 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -186,13 +186,6 @@ struct xe_tile {
> > */
> > struct xe_mmio mmio;
> >
> > - /**
> > - * @mmio_ext: MMIO-extension info for a tile.
> > - *
> > - * Each tile has its own additional 256MB (28-bit) MMIO-extension space.
> > - */
> > - struct xe_mmio mmio_ext;
> > -
> > /** @mem: memory management info for tile */
> > struct {
> > /**
> > @@ -263,8 +256,6 @@ struct xe_device {
> > const char *graphics_name;
> > /** @info.media_name: media IP name */
> > const char *media_name;
> > - /** @info.tile_mmio_ext_size: size of MMIO extension space, per-tile */
> > - u32 tile_mmio_ext_size;
> > /** @info.graphics_verx100: graphics IP version */
> > u32 graphics_verx100;
> > /** @info.media_verx100: media IP version */
> > @@ -314,8 +305,6 @@ struct xe_device {
> > u8 has_heci_gscfi:1;
> > /** @info.has_llc: Device has a shared CPU+GPU last level cache */
> > u8 has_llc:1;
> > - /** @info.has_mmio_ext: Device has extra MMIO address range */
> > - u8 has_mmio_ext:1;
> > /** @info.has_range_tlb_invalidation: Has range based TLB invalidations */
> > u8 has_range_tlb_invalidation:1;
> > /** @info.has_sriov: Supports SR-IOV */
> > diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> > index a48f239cad1c..d321a21aacf0 100644
> > --- a/drivers/gpu/drm/xe/xe_mmio.c
> > +++ b/drivers/gpu/drm/xe/xe_mmio.c
> > @@ -103,50 +103,11 @@ static void mmio_multi_tile_setup(struct xe_device *xe, size_t tile_mmio_size)
> > }
> > }
> >
> > -/*
> > - * On top of all the multi-tile MMIO space there can be a platform-dependent
> > - * extension for each tile, resulting in a layout like below:
> > - *
> > - * .----------------------. <- ext_base + tile_count * tile_mmio_ext_size
> > - * | .... |
> > - * |----------------------| <- ext_base + 2 * tile_mmio_ext_size
> > - * | tile1->mmio_ext.regs |
> > - * |----------------------| <- ext_base + 1 * tile_mmio_ext_size
> > - * | tile0->mmio_ext.regs |
> > - * |======================| <- ext_base = tile_count * tile_mmio_size
> > - * | |
> > - * | mmio.regs |
> > - * | |
> > - * '----------------------' <- 0MB
> > - *
> > - * Set up the tile[]->mmio_ext pointers/sizes.
> > - */
> > -static void mmio_extension_setup(struct xe_device *xe, size_t tile_mmio_size,
> > - size_t tile_mmio_ext_size)
> > -{
> > - struct xe_tile *tile;
> > - void __iomem *regs;
> > - u8 id;
> > -
> > - if (!xe->info.has_mmio_ext)
> > - return;
> > -
> > - regs = xe->mmio.regs + tile_mmio_size * xe->info.tile_count;
> > - for_each_tile(tile, xe, id) {
> > - tile->mmio_ext.regs_size = tile_mmio_ext_size;
> > - tile->mmio_ext.regs = regs;
> > - tile->mmio_ext.tile = tile;
> > - regs += tile_mmio_ext_size;
> > - }
> > -}
> > -
> > int xe_mmio_probe_tiles(struct xe_device *xe)
> > {
> > size_t tile_mmio_size = SZ_16M;
> > - size_t tile_mmio_ext_size = xe->info.tile_mmio_ext_size;
> >
> > mmio_multi_tile_setup(xe, tile_mmio_size);
> > - mmio_extension_setup(xe, tile_mmio_size, tile_mmio_ext_size);
> >
> > return devm_add_action_or_reset(xe->drm.dev, tiles_fini, xe);
> > }
> > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > index 7d146e3e8e21..90fcfafd362f 100644
> > --- a/drivers/gpu/drm/xe/xe_pci.c
> > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > @@ -61,7 +61,6 @@ struct xe_device_desc {
> > u8 has_heci_gscfi:1;
> > u8 has_heci_cscfi:1;
> > u8 has_llc:1;
> > - u8 has_mmio_ext:1;
> > u8 has_sriov:1;
> > u8 skip_guc_pc:1;
> > u8 skip_mtcfg:1;
> > @@ -622,7 +621,6 @@ static int xe_info_init_early(struct xe_device *xe,
> > xe->info.has_heci_gscfi = desc->has_heci_gscfi;
> > xe->info.has_heci_cscfi = desc->has_heci_cscfi;
> > xe->info.has_llc = desc->has_llc;
> > - xe->info.has_mmio_ext = desc->has_mmio_ext;
> > xe->info.has_sriov = desc->has_sriov;
> > xe->info.skip_guc_pc = desc->skip_guc_pc;
> > xe->info.skip_mtcfg = desc->skip_mtcfg;
> > @@ -682,7 +680,6 @@ static int xe_info_init(struct xe_device *xe,
> >
> > xe->info.graphics_name = graphics_desc->name;
> > xe->info.media_name = media_desc ? media_desc->name : "none";
> > - xe->info.tile_mmio_ext_size = graphics_desc->tile_mmio_ext_size;
> >
> > xe->info.dma_mask_size = graphics_desc->dma_mask_size;
> > xe->info.vram_flags = graphics_desc->vram_flags;
> > diff --git a/drivers/gpu/drm/xe/xe_pci_types.h b/drivers/gpu/drm/xe/xe_pci_types.h
> > index 79b0f80376a4..873efec5cdee 100644
> > --- a/drivers/gpu/drm/xe/xe_pci_types.h
> > +++ b/drivers/gpu/drm/xe/xe_pci_types.h
> > @@ -20,8 +20,6 @@ struct xe_graphics_desc {
> >
> > u64 hw_engine_mask; /* hardware engines provided by graphics IP */
> >
> > - u32 tile_mmio_ext_size; /* size of MMIO extension space, per-tile */
> > -
> > u8 max_remote_tiles:2;
> >
> > u8 has_asid:1;
>
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
More information about the Intel-xe
mailing list