[PATCH v9 3/4] drm/xe: Move struct xe_vram_region to a dedicated header
Piotr Piórkowski
piotr.piorkowski at intel.com
Tue Jul 8 07:03:46 UTC 2025
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.
>
> > 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.
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