[PATCH v9 3/4] drm/xe: Move struct xe_vram_region to a dedicated header
Matthew Brost
matthew.brost at intel.com
Tue Jul 8 07:42:32 UTC 2025
On Tue, Jul 08, 2025 at 09:03:46AM +0200, Piotr Piórkowski wrote:
> Matthew Brost <matthew.brost at intel.com> wrote on pon [2025-lip-07 09:38:15 -0700]:
> > On Mon, Jul 07, 2025 at 04:56:08PM +0200, Piórkowski, Piotr wrote:
> > > From: Piotr Piórkowski <piotr.piorkowski at intel.com>
> > >
> > > Let's move the xe_vram_region structure to a new header dedicated to VRAM
> > > to improve modularity and avoid unnecessary dependencies when only
> > > VRAM-related structures are needed.
> > >
> > > v2: Fix build if CONFIG_DRM_XE_DEVMEM_MIRROR is enabled
> > > v3: Fix build if CONFIG_DRM_XE_DISPLAY is enabled
> > >
> > > Signed-off-by: Piotr Piórkowski <piotr.piorkowski at intel.com>
> > > Suggested-by: Jani Nikula <jani.nikula at intel.com>
> > > Reviewed-by: Satyanarayana K V P <satyanarayana.k.v.p at intel.com>
> > > ---
> > > drivers/gpu/drm/xe/display/xe_fb_pin.c | 1 +
> > > drivers/gpu/drm/xe/display/xe_plane_initial.c | 1 +
> > > drivers/gpu/drm/xe/xe_bo.c | 1 +
> > > drivers/gpu/drm/xe/xe_bo_types.h | 1 +
> > > drivers/gpu/drm/xe/xe_device.c | 1 +
> > > drivers/gpu/drm/xe/xe_device_types.h | 60 +--------------
> > > drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c | 1 +
> > > drivers/gpu/drm/xe/xe_svm.c | 3 +-
> > > drivers/gpu/drm/xe/xe_tile.c | 1 +
> > > drivers/gpu/drm/xe/xe_tile.h | 11 ---
> > > drivers/gpu/drm/xe/xe_ttm_vram_mgr.c | 1 +
> > > drivers/gpu/drm/xe/xe_vram.c | 1 +
> > > drivers/gpu/drm/xe/xe_vram_types.h | 74 +++++++++++++++++++
> > > 13 files changed, 86 insertions(+), 71 deletions(-)
> > > create mode 100644 drivers/gpu/drm/xe/xe_vram_types.h
> > >
> > > diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > index 2187b2de64e1..f2cfba674899 100644
> > > --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > @@ -16,6 +16,7 @@
> > > #include "xe_device.h"
> > > #include "xe_ggtt.h"
> > > #include "xe_pm.h"
> > > +#include "xe_vram_types.h"
> > >
> > > static void
> > > write_dpt_rotated(struct xe_bo *bo, struct iosys_map *map, u32 *dpt_ofs, u32 bo_ofs,
> > > diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > index 873f4e18cb2a..11591c7cdc70 100644
> > > --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> > > @@ -21,6 +21,7 @@
> > > #include "intel_plane.h"
> > > #include "intel_plane_initial.h"
> > > #include "xe_bo.h"
> > > +#include "xe_vram_types.h"
> > > #include "xe_wa.h"
> > >
> > > #include <generated/xe_wa_oob.h>
> > > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > > index 7f8470b22dc9..a35a9e0dbfae 100644
> > > --- a/drivers/gpu/drm/xe/xe_bo.c
> > > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > > @@ -34,6 +34,7 @@
> > > #include "xe_trace_bo.h"
> > > #include "xe_ttm_stolen_mgr.h"
> > > #include "xe_vm.h"
> > > +#include "xe_vram_types.h"
> > >
> > > const char *const xe_mem_type_to_name[TTM_NUM_MEM_TYPES] = {
> > > [XE_PL_SYSTEM] = "system",
> > > diff --git a/drivers/gpu/drm/xe/xe_bo_types.h b/drivers/gpu/drm/xe/xe_bo_types.h
> > > index ff560d82496f..57d34698139e 100644
> > > --- a/drivers/gpu/drm/xe/xe_bo_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_bo_types.h
> > > @@ -9,6 +9,7 @@
> > > #include <linux/iosys-map.h>
> > >
> > > #include <drm/drm_gpusvm.h>
> > > +#include <drm/drm_pagemap.h>
> > > #include <drm/ttm/ttm_bo.h>
> > > #include <drm/ttm/ttm_device.h>
> > > #include <drm/ttm/ttm_placement.h>
> > > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > > index 19019b032c78..4bd0619574ff 100644
> > > --- a/drivers/gpu/drm/xe/xe_device.c
> > > +++ b/drivers/gpu/drm/xe/xe_device.c
> > > @@ -63,6 +63,7 @@
> > > #include "xe_ttm_sys_mgr.h"
> > > #include "xe_vm.h"
> > > #include "xe_vram.h"
> > > +#include "xe_vram_types.h"
> > > #include "xe_vsec.h"
> > > #include "xe_wait_user_fence.h"
> > > #include "xe_wa.h"
> > > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > > index ece4bfcde477..55b068183f8c 100644
> > > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > > @@ -10,7 +10,6 @@
> > >
> > > #include <drm/drm_device.h>
> > > #include <drm/drm_file.h>
> > > -#include <drm/drm_pagemap.h>
> > > #include <drm/ttm/ttm_device.h>
> > >
> > > #include "xe_devcoredump_types.h"
> > > @@ -24,7 +23,6 @@
> > > #include "xe_sriov_types.h"
> > > #include "xe_step_types.h"
> > > #include "xe_survivability_mode_types.h"
> > > -#include "xe_ttm_vram_mgr_types.h"
> > >
> > > #if IS_ENABLED(CONFIG_DRM_XE_DEBUG)
> > > #define TEST_VM_OPS_ERROR
> > > @@ -36,6 +34,7 @@ struct intel_dg_nvm_dev;
> > > struct xe_ggtt;
> > > struct xe_pat_ops;
> > > struct xe_pxp;
> > > +struct xe_vram;
> > >
> > > #define XE_BO_INVALID_OFFSET LONG_MAX
> > >
> > > @@ -68,63 +67,6 @@ struct xe_pxp;
> > > const struct xe_tile * : (const struct xe_device *)((tile__)->xe), \
> > > struct xe_tile * : (tile__)->xe)
> > >
> > > -/**
> > > - * struct xe_vram_region - memory region structure
> > > - * This is used to describe a memory region in xe
> > > - * device, such as HBM memory or CXL extension memory.
> > > - */
> > > -struct xe_vram_region {
> > > - /** @tile: Back pointer to tile */
> > > - struct xe_tile *tile;
> > > - /** @io_start: IO start address of this VRAM instance */
> > > - resource_size_t io_start;
> > > - /**
> > > - * @io_size: IO size of this VRAM instance
> > > - *
> > > - * This represents how much of this VRAM we can access
> > > - * via the CPU through the VRAM BAR. This can be smaller
> > > - * than @usable_size, in which case only part of VRAM is CPU
> > > - * accessible (typically the first 256M). This
> > > - * configuration is known as small-bar.
> > > - */
> > > - resource_size_t io_size;
> > > - /** @dpa_base: This memory regions's DPA (device physical address) base */
> > > - resource_size_t dpa_base;
> > > - /**
> > > - * @usable_size: usable size of VRAM
> > > - *
> > > - * Usable size of VRAM excluding reserved portions
> > > - * (e.g stolen mem)
> > > - */
> > > - resource_size_t usable_size;
> > > - /**
> > > - * @actual_physical_size: Actual VRAM size
> > > - *
> > > - * Actual VRAM size including reserved portions
> > > - * (e.g stolen mem)
> > > - */
> > > - resource_size_t actual_physical_size;
> > > - /** @mapping: pointer to VRAM mappable space */
> > > - void __iomem *mapping;
> > > - /** @ttm: VRAM TTM manager */
> > > - struct xe_ttm_vram_mgr ttm;
> > > -#if IS_ENABLED(CONFIG_DRM_XE_PAGEMAP)
> > > - /** @pagemap: Used to remap device memory as ZONE_DEVICE */
> > > - struct dev_pagemap pagemap;
> > > - /**
> > > - * @dpagemap: The struct drm_pagemap of the ZONE_DEVICE memory
> > > - * pages of this tile.
> > > - */
> > > - struct drm_pagemap dpagemap;
> > > - /**
> > > - * @hpa_base: base host physical address
> > > - *
> > > - * This is generated when remap device memory as ZONE_DEVICE
> > > - */
> > > - resource_size_t hpa_base;
> > > -#endif
> > > -};
> > > -
> > > /**
> > > * struct xe_mmio - register mmio structure
> > > *
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> > > index e69a05126dae..275ab5f03291 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> > > +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> > > @@ -33,6 +33,7 @@
> > > #include "xe_migrate.h"
> > > #include "xe_sriov.h"
> > > #include "xe_ttm_vram_mgr.h"
> > > +#include "xe_vram_types.h"
> > > #include "xe_wopcm.h"
> > >
> > > #define make_u64_from_u32(hi, lo) ((u64)((u64)(u32)(hi) << 32 | (u32)(lo)))
> > > diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> > > index e6871734ffa9..ee015e3f5d9e 100644
> > > --- a/drivers/gpu/drm/xe/xe_svm.c
> > > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > > @@ -17,6 +17,7 @@
> > > #include "xe_ttm_vram_mgr.h"
> > > #include "xe_vm.h"
> > > #include "xe_vm_types.h"
> > > +#include "xe_vram_types.h"
> > >
> > > static bool xe_svm_range_in_vram(struct xe_svm_range *range)
> > > {
> > > @@ -1006,7 +1007,7 @@ int xe_svm_alloc_vram(struct xe_tile *tile, struct xe_svm_range *range,
> > > xe_assert(tile_to_xe(tile), range->base.flags.migrate_devmem);
> > > range_debug(range, "ALLOCATE VRAM");
> > >
> > > - dpagemap = xe_tile_local_pagemap(tile);
> > > + dpagemap = &tile->mem.vram->dpagemap;
> >
> > I'd leave the helper here and move 'tile->mem.vram->dpagemap' to the
> > helper.
>
> I removed this helper with full premeditation and after consulting with the author of the code.
> I decided that since we only use it in this one place and we can avoid adding include
> xe_vram_types.h to xe_tile.h, it should be done.
>
Ok. I prefer layering, as the compiler figures it all out anyway and it
makes for more maintainable code. See [1] if you don't believe me—I
removed one member in that patch.
[1] https://patchwork.freedesktop.org/patch/660681/?series=150710&rev=4
> >
> > > return drm_pagemap_populate_mm(dpagemap, xe_svm_range_start(range),
> > > xe_svm_range_end(range),
> > > range->base.gpusvm->mm,
> > > diff --git a/drivers/gpu/drm/xe/xe_tile.c b/drivers/gpu/drm/xe/xe_tile.c
> > > index 858ce0183aaa..bd2ff91a7d1c 100644
> > > --- a/drivers/gpu/drm/xe/xe_tile.c
> > > +++ b/drivers/gpu/drm/xe/xe_tile.c
> > > @@ -20,6 +20,7 @@
> > > #include "xe_ttm_vram_mgr.h"
> > > #include "xe_wa.h"
> > > #include "xe_vram.h"
> > > +#include "xe_vram_types.h"
> > >
> > > /**
> > > * DOC: Multi-tile Design
> > > diff --git a/drivers/gpu/drm/xe/xe_tile.h b/drivers/gpu/drm/xe/xe_tile.h
> > > index fb0473fa9098..8dd87ccb2aca 100644
> > > --- a/drivers/gpu/drm/xe/xe_tile.h
> > > +++ b/drivers/gpu/drm/xe/xe_tile.h
> > > @@ -18,15 +18,4 @@ int xe_tile_alloc_vram(struct xe_tile *tile);
> > >
> > > void xe_tile_migrate_wait(struct xe_tile *tile);
> > >
> > > -#if IS_ENABLED(CONFIG_DRM_XE_PAGEMAP)
> > > -static inline struct drm_pagemap *xe_tile_local_pagemap(struct xe_tile *tile)
> > > -{
> > > - return &tile->mem.vram->dpagemap;
> > > -}
> > > -#else
> > > -static inline struct drm_pagemap *xe_tile_local_pagemap(struct xe_tile *tile)
> > > -{
> > > - return NULL;
> > > -}
> > > -#endif
> > > #endif
> > > diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> > > index 3de2df47959b..8f9b8a1d2c05 100644
> > > --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> > > +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> > > @@ -15,6 +15,7 @@
> > > #include "xe_gt.h"
> > > #include "xe_res_cursor.h"
> > > #include "xe_ttm_vram_mgr.h"
> > > +#include "xe_vram_types.h"
> > >
> > > static inline struct drm_buddy_block *
> > > xe_ttm_vram_mgr_first_block(struct list_head *list)
> > > diff --git a/drivers/gpu/drm/xe/xe_vram.c b/drivers/gpu/drm/xe/xe_vram.c
> > > index 71f463f2c3be..35fd7f5d7cff 100644
> > > --- a/drivers/gpu/drm/xe/xe_vram.c
> > > +++ b/drivers/gpu/drm/xe/xe_vram.c
> > > @@ -20,6 +20,7 @@
> > > #include "xe_module.h"
> > > #include "xe_sriov.h"
> > > #include "xe_vram.h"
> > > +#include "xe_vram_types.h"
> > >
> > > #define BAR_SIZE_SHIFT 20
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_vram_types.h b/drivers/gpu/drm/xe/xe_vram_types.h
> > > new file mode 100644
> > > index 000000000000..a01838236036
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/xe/xe_vram_types.h
> > > @@ -0,0 +1,74 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright © 2025 Intel Corporation
> > > + */
> > > +
> > > +#ifndef _XE_VRAM_TYPES_H_
> > > +#define _XE_VRAM_TYPES_H_
> > > +
> > > +#if IS_ENABLED(CONFIG_DRM_XE_PAGEMAP)
> > > +#include <drm/drm_pagemap.h>
> > > +#endif
> > > +
> > > +#include "xe_ttm_vram_mgr_types.h"
> > > +
> > > +struct xe_tile;
> > > +
> > > +/**
> > > + * struct xe_vram_region - memory region structure
> > > + * This is used to describe a memory region in xe
> > > + * device, such as HBM memory or CXL extension memory.
> > > + */
> > > +struct xe_vram_region {
> > > + /** @tile: Back pointer to tile */
> > > + struct xe_tile *tile;
> >
> > If the goal is share one xe_vram_region among several tiles, doesn't
> > having a back pointer to some extent break this? It would be better to
> > only have a 1 way relationship here: tile -> VRAM but VRAM cannot lookup
> > the tile. I haven't look through the entire series, but in what cases is
> > this backpointer needed?
> >
> > Matt
>
> I have mixed feelings about this back pointer.
> But let me start from the beginning:
> My goal is to make vram_region independent of structures such as xe_device or xe_tile.
> In the future, I would like this object to be dynamically assigned to these objects,
> creating a many-to-many relationship where one tile can have several vram regions
> and one vram region can be shared by many tiles. From a logical point of view,
> this back pointer actually breaks the one vram region to many tiles relationship.
> Currently, from what I can see, this back pointer is only used in the SVM code:
>
> drivers/gpu/drm/xe/xe_svm.c: In function ‘xe_vram_region_page_to_dpa’:
> drivers/gpu/drm/xe/xe_svm.c:314:34: error: ‘struct xe_vram_region’ has no member named ‘tile’
> 314 | struct xe_tile *tile = vr->tile;
> | ^~
> drivers/gpu/drm/xe/xe_svm.c: In function ‘xe_svm_copy’:
> drivers/gpu/drm/xe/xe_svm.c:369:34: error: ‘struct xe_vram_region’ has no member named ‘tile’
> 369 | tile = vr->tile;
> | ^~
> drivers/gpu/drm/xe/xe_svm.c: In function ‘xe_svm_populate_devmem_pfn’:
> drivers/gpu/drm/xe/xe_svm.c:521:42: error: ‘struct xe_vram_region’ has no member named ‘tile’
> 521 | struct xe_tile *tile = vr->tile;
> | ^~
> drivers/gpu/drm/xe/xe_svm.c: In function ‘xe_drm_pagemap_populate_mm’:
> drivers/gpu/drm/xe/xe_svm.c:688:34: error: ‘struct xe_vram_region’ has no member named ‘tile’
> 688 | struct xe_tile *tile = vr->tile;
> | ^~
>
> - I think this is your code, so you should know best if we can actually drop of it there.
> From what I can see, removing of this back pointer will also cause a problem with getting
> the xe pointer, which can sometimes be useful.
If the Xe pointer is useful, store it in VRAM.
I see three cases in xe_svm.c:
1. xe_vram_region_page_to_dpa: used for asserts. I think we can convert
these to Xe device–based asserts.
2. xe_svm_copy: used during migration. This is likely a case where we do
need some kind of backpointer. I'd say store xe_migrate in the VRAM,
storing xe_migrate of the first tile which contains the VRAM.
3. xe_svm_populate_devmem_pfn: this goes from VR → tile → buddy. While this
relationship is needed, the implementation is fairly roundabout. I think
we can go directly from VR → buddy without using the tile backpointer.
Matt
>
> Let me know what you think, or if you have any ideas. I'm open to getting rid of it.>
>
> Thanks,
> Piotr
>
> > > + /** @io_start: IO start address of this VRAM instance */
> > > + resource_size_t io_start;
> > > + /**
> > > + * @io_size: IO size of this VRAM instance
> > > + *
> > > + * This represents how much of this VRAM we can access
> > > + * via the CPU through the VRAM BAR. This can be smaller
> > > + * than @usable_size, in which case only part of VRAM is CPU
> > > + * accessible (typically the first 256M). This
> > > + * configuration is known as small-bar.
> > > + */
> > > + resource_size_t io_size;
> > > + /** @dpa_base: This memory regions's DPA (device physical address) base */
> > > + resource_size_t dpa_base;
> > > + /**
> > > + * @usable_size: usable size of VRAM
> > > + *
> > > + * Usable size of VRAM excluding reserved portions
> > > + * (e.g stolen mem)
> > > + */
> > > + resource_size_t usable_size;
> > > + /**
> > > + * @actual_physical_size: Actual VRAM size
> > > + *
> > > + * Actual VRAM size including reserved portions
> > > + * (e.g stolen mem)
> > > + */
> > > + resource_size_t actual_physical_size;
> > > + /** @mapping: pointer to VRAM mappable space */
> > > + void __iomem *mapping;
> > > + /** @ttm: VRAM TTM manager */
> > > + struct xe_ttm_vram_mgr ttm;
> > > +#if IS_ENABLED(CONFIG_DRM_XE_PAGEMAP)
> > > + /** @pagemap: Used to remap device memory as ZONE_DEVICE */
> > > + struct dev_pagemap pagemap;
> > > + /**
> > > + * @dpagemap: The struct drm_pagemap of the ZONE_DEVICE memory
> > > + * pages of this tile.
> > > + */
> > > + struct drm_pagemap dpagemap;
> > > + /**
> > > + * @hpa_base: base host physical address
> > > + *
> > > + * This is generated when remap device memory as ZONE_DEVICE
> > > + */
> > > + resource_size_t hpa_base;
> > > +#endif
> > > +};
> > > +
> > > +#endif
> > > --
> > > 2.34.1
> > >
>
> --
More information about the Intel-xe
mailing list