Re: [PATCH 1/3] drm/xe/mm: add XE DRM MM manager with shadow support

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Mar 27, 2026 at 11:54:15AM +0100, Michal Wajdeczko wrote:
> 
> 
> On 3/26/2026 8:57 PM, Thomas Hellström wrote:
> > On Fri, 2026-03-20 at 12:12 +0000, Satyanarayana K V P wrote:
> >> Add a xe_drm_mm manager to allocate sub-ranges from a BO-backed pool
> >> using drm_mm.
> > 
> > Just a comment on the naming. xe_drm_mm sounds like this is yet another
> > specialized range manager implementation.
> 
> well, in fact it looks like *very* specialized MM, not much reusable elsewhere
> 
> > 
> > Could we invent a better name? 
> > 
> > xe_mm_suballoc? Something even better?
> 
> xe_shadow_pool ?
> 
> or if we split this MM into "plain pool" and "shadow pool":
> 
> xe_pool		--> like xe_sa but works without fences (can be reused in xe_guc_buf)
> xe_shadow_pool	--> built on top of plain, with shadow logic
> 
> 
> more comments below
> 
> > 
> > 
> > Thanks,
> > Thomas
> > 
> > 
> > 
> >>
> >> Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
> >> Cc: Matthew Brost <matthew.brost@intel.com>
> >> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> >> Cc: Maarten Lankhorst <dev@lankhorst.se>
> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/Makefile          |   1 +
> >>  drivers/gpu/drm/xe/xe_drm_mm.c       | 200
> >> +++++++++++++++++++++++++++
> >>  drivers/gpu/drm/xe/xe_drm_mm.h       |  55 ++++++++
> >>  drivers/gpu/drm/xe/xe_drm_mm_types.h |  42 ++++++
> >>  4 files changed, 298 insertions(+)
> >>  create mode 100644 drivers/gpu/drm/xe/xe_drm_mm.c
> >>  create mode 100644 drivers/gpu/drm/xe/xe_drm_mm.h
> >>  create mode 100644 drivers/gpu/drm/xe/xe_drm_mm_types.h
> >>
> >> diff --git a/drivers/gpu/drm/xe/Makefile
> >> b/drivers/gpu/drm/xe/Makefile
> >> index dab979287a96..6ab4e2392df1 100644
> >> --- a/drivers/gpu/drm/xe/Makefile
> >> +++ b/drivers/gpu/drm/xe/Makefile
> >> @@ -41,6 +41,7 @@ xe-y += xe_bb.o \
> >>  	xe_device_sysfs.o \
> >>  	xe_dma_buf.o \
> >>  	xe_drm_client.o \
> >> +	xe_drm_mm.o \
> >>  	xe_drm_ras.o \
> >>  	xe_eu_stall.o \
> >>  	xe_exec.o \
> >> diff --git a/drivers/gpu/drm/xe/xe_drm_mm.c
> >> b/drivers/gpu/drm/xe/xe_drm_mm.c
> >> new file mode 100644
> >> index 000000000000..c5b1766fa75a
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/xe/xe_drm_mm.c
> >> @@ -0,0 +1,200 @@
> >> +// SPDX-License-Identifier: MIT
> >> +/*
> >> + * Copyright © 2026 Intel Corporation
> >> + */
> >> +
> >> +#include <drm/drm_managed.h>
> 
> nit: <linux> headers go first
> 
> >> +#include <linux/kernel.h>
> >> +
> >> +#include "xe_device_types.h"
> >> +#include "xe_drm_mm_types.h"
> >> +#include "xe_drm_mm.h"
> >> +#include "xe_map.h"
> >> +
> >> +static void xe_drm_mm_manager_fini(struct drm_device *drm, void
> >> *arg)
> >> +{
> >> +	struct xe_drm_mm_manager *drm_mm_manager = arg;
> >> +	struct xe_bo *bo = drm_mm_manager->bo;
> >> +
> >> +	if (!bo) {
> 
> not needed, we shouldn't be here if we failed to allocate a bo
> 

Agree this likely isn't needed.

> >> +		drm_err(drm, "no bo for drm mm manager\n");
> 
> btw, our MM seems to be 'tile' oriented, so we should use xe_tile_err() 
> 

I'd rather leave this as an independent component which we can reuse
anywhere in the future.

> >> +		return;
> >> +	}
> >> +
> >> +	drm_mm_takedown(&drm_mm_manager->base);
> >> +
> >> +	if (drm_mm_manager->is_iomem)
> >> +		kvfree(drm_mm_manager->cpu_addr);
> >> +
> >> +	drm_mm_manager->bo = NULL;
> >> +	drm_mm_manager->shadow = NULL;
> >> +}
> >> +
> >> +/**
> >> + * xe_drm_mm_manager_init() - Create and initialize the DRM MM
> >> manager.
> >> + * @tile: the &xe_tile where allocate.
> >> + * @size: number of bytes to allocate
> >> + * @guard: number of bytes to exclude from allocation for guard
> >> region
> 
> do we really need this guard ? it was already questionable on the xe_sa
> 

Isn’t this what you were suggesting in patch #3 with a tail region? It
looks like this is implementing your suggestion to avoid having each BB
allocate an extra DW — we’d just set this to 1 for the current use case.
It’s simpler than my suggestion to allocate a drm_mm node to reserve the
“tail” / “guard..

> >> + * @flags: additional flags for configuring the DRM MM manager.
> >> + *
> >> + * Initializes a DRM MM manager for managing memory allocations on a
> >> specific
> >> + * XE tile. The function allocates a buffer object to back the
> >> memory region
> >> + * managed by the DRM MM manager.
> >> + *
> >> + * Return: a pointer to the &xe_drm_mm_manager, or an error pointer
> >> on failure.
> >> + */
> >> +struct xe_drm_mm_manager *xe_drm_mm_manager_init(struct xe_tile
> >> *tile, u32 size,
> >> +						 u32 guard, u32
> >> flags)
> >> +{
> >> +	struct xe_device *xe = tile_to_xe(tile);
> >> +	struct xe_drm_mm_manager *drm_mm_manager;
> >> +	u64 managed_size;
> >> +	struct xe_bo *bo;
> >> +	int ret;
> >> +
> >> +	xe_tile_assert(tile, size > guard);
> >> +	managed_size = size - guard;
> >> +
> >> +	drm_mm_manager = drmm_kzalloc(&xe->drm,
> >> sizeof(*drm_mm_manager), GFP_KERNEL);
> >> +	if (!drm_mm_manager)
> >> +		return ERR_PTR(-ENOMEM);
> 
> can't we make this manager a member of the tile and then use
> container_of to get parent tile pointer?
> 
> I guess we will have exactly one this MM per tile, no?
> 

Again see above - I strong prefer this is an independent component
(malloc'd object) rather than an embedded object for future reuse.

> >> +
> >> +	bo = xe_managed_bo_create_pin_map(xe, tile, size,
> >> +					 
> >> XE_BO_FLAG_VRAM_IF_DGFX(tile) |
> >> +					  XE_BO_FLAG_GGTT |
> >> +					  XE_BO_FLAG_GGTT_INVALIDATE
> >> |
> >> +					 
> >> XE_BO_FLAG_PINNED_NORESTORE);
> >> +	if (IS_ERR(bo)) {
> >> +		drm_err(&xe->drm, "Failed to prepare %uKiB BO for
> 
> xe_tile_err(tile, ...
> 
> but maybe nicer solution would be to add such error to the
> xe_managed_bo_create_pin_map() to avoid duplicating this diag
> messages in all callers
> 
> >> DRM MM manager (%pe)\n",
> >> +			size / SZ_1K, bo);
> >> +		return ERR_CAST(bo);
> >> +	}
> >> +	drm_mm_manager->bo = bo;
> >> +	drm_mm_manager->is_iomem = bo->vmap.is_iomem;
> 
> do we need to cache this?
> 
> >> +
> >> +	if (bo->vmap.is_iomem) {
> >> +		drm_mm_manager->cpu_addr = kvzalloc(managed_size,
> >> GFP_KERNEL);
> >> +		if (!drm_mm_manager->cpu_addr)
> >> +			return ERR_PTR(-ENOMEM);
> >> +	} else {
> >> +		drm_mm_manager->cpu_addr = bo->vmap.vaddr;
> >> +		memset(drm_mm_manager->cpu_addr, 0, bo-
> >>> ttm.base.size);
> 
> btw, maybe we should consider adding XE_BO_FLAG_ZERO and let
> the xe_create_bo do initial clear for us?
> 

The memset isn't needed per my comments here [1].

[1] https://patchwork.freedesktop.org/patch/713119/?series=163588&rev=1#comment_1315262

> @Matt @Thomas ?
> 
> >> +	}
> >> +
> >> +	if (flags & XE_DRM_MM_BO_MANAGER_FLAG_SHADOW) {
> 
> hmm, so this is not a main feature of this MM?
> then maybe we should have two components:
> 
> 	* xe_pool (plain MM, like xe_sa but without fences)
> 	* xe_shadow (adds shadow BO on top of plain MM)
> 
> >> +		struct xe_bo *shadow;
> >> +
> >> +		ret = drmm_mutex_init(&xe->drm, &drm_mm_manager-
> >>> swap_guard);
> >> +		if (ret)
> >> +			return ERR_PTR(ret);
> >> +		if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {
> >> +			fs_reclaim_acquire(GFP_KERNEL);
> >> +			might_lock(&drm_mm_manager->swap_guard);
> >> +			fs_reclaim_release(GFP_KERNEL);
> >> +		}
> >> +
> >> +		shadow = xe_managed_bo_create_pin_map(xe, tile,
> >> size,
> >> +						     
> >> XE_BO_FLAG_VRAM_IF_DGFX(tile) |
> >> +						     
> >> XE_BO_FLAG_GGTT |
> >> +						     
> >> XE_BO_FLAG_GGTT_INVALIDATE |
> >> +						     
> >> XE_BO_FLAG_PINNED_NORESTORE);
> >> +		if (IS_ERR(shadow)) {
> >> +			drm_err(&xe->drm,
> >> +				"Failed to prepare %uKiB shadow BO
> >> for DRM MM manager (%pe)\n",
> >> +				size / SZ_1K, shadow);
> >> +			return ERR_CAST(shadow);
> >> +		}
> >> +		drm_mm_manager->shadow = shadow;
> >> +	}
> >> +
> >> +	drm_mm_init(&drm_mm_manager->base, 0, managed_size);
> >> +	ret = drmm_add_action_or_reset(&xe->drm,
> >> xe_drm_mm_manager_fini, drm_mm_manager);
> >> +	if (ret)
> >> +		return ERR_PTR(ret);
> >> +
> >> +	return drm_mm_manager;
> >> +}
> >> +
> >> +/**
> >> + * xe_drm_mm_bo_swap_shadow() - Swap the primary BO with the shadow
> >> BO.
> 
> do we need _bo_ in the function name here?
> 
> >> + * @drm_mm_manager: the DRM MM manager containing the primary and
> >> shadow BOs.
> >> + *
> >> + * Swaps the primary buffer object with the shadow buffer object in
> >> the DRM MM
> >> + * manager.
> 
> say a word about required swap_guard mutex
> 
> and/or add the _locked suffix to the function name
> 
> >> + *
> >> + * Return: None.
> >> + */
> >> +void xe_drm_mm_bo_swap_shadow(struct xe_drm_mm_manager
> >> *drm_mm_manager)
> >> +{
> >> +	struct xe_device *xe = tile_to_xe(drm_mm_manager->bo->tile);
> >> +
> >> +	xe_assert(xe, drm_mm_manager->shadow);
> 
> use xe_tile_assert
> 
> >> +	lockdep_assert_held(&drm_mm_manager->swap_guard);
> >> +
> >> +	swap(drm_mm_manager->bo, drm_mm_manager->shadow);
> >> +	if (!drm_mm_manager->bo->vmap.is_iomem)
> >> +		drm_mm_manager->cpu_addr = drm_mm_manager->bo-
> >>> vmap.vaddr;
> >> +}
> >> +
> >> +/**
> >> + * xe_drm_mm_sync_shadow() - Synchronize the shadow BO with the
> >> primary BO.
> >> + * @drm_mm_manager: the DRM MM manager containing the primary and
> >> shadow BOs.
> >> + * @node: the DRM MM node representing the region to synchronize.
> >> + *
> >> + * Copies the contents of the specified region from the primary
> >> buffer object to
> >> + * the shadow buffer object in the DRM MM manager.
> >> + *
> >> + * Return: None.
> >> + */
> >> +void xe_drm_mm_sync_shadow(struct xe_drm_mm_manager *drm_mm_manager,
> >> +			   struct drm_mm_node *node)
> >> +{
> >> +	struct xe_device *xe = tile_to_xe(drm_mm_manager->bo->tile);
> >> +
> >> +	xe_assert(xe, drm_mm_manager->shadow);
> >> +	lockdep_assert_held(&drm_mm_manager->swap_guard);
> >> +
> >> +	xe_map_memcpy_to(xe, &drm_mm_manager->shadow->vmap,
> >> +			 node->start,
> >> +			 drm_mm_manager->cpu_addr + node->start,
> >> +			 node->size);
> 
> maybe I'm missing something, but if primary BO.is_iomem==true then
> who/when updates the actual primary BO memory? or is it unused by
> design and only shadow has the data ...
> 
> maybe some DOC section with theory-of-operation will help here?
> 

Yes, I think this an issue, this won't work as...

I think we need a xe_drm_mm_flush call...

e.g.

xe_drm_mm_flush();
xe_sriov_vf_ccs_rw_update_bb_addr();
xe_sriov_vf_ccs_rw_update_bb_addr(); 

> >> +}
> >> +
> >> +/**
> >> + * xe_drm_mm_insert_node() - Insert a node into the DRM MM manager.
> >> + * @drm_mm_manager: the DRM MM manager to insert the node into.
> >> + * @node: the DRM MM node to insert.
> 
> in recent changes to xe_ggtt we finally hidden the implementation details
> of the MM used by the xe_ggtt
> 

We should probably privatize struct xe_drm_mm_manager as well. For
example, we could move the struct xe_drm_mm_manager definition into
xe_drm_mm.c rather than xe_drm_mm_types.h, given my suggestion to make
this a component rather than an embedded struct.

> why here we start again exposing impl detail as part of the API?
> if we can't allocate xe_drm_mm_node here, maybe at least take it
> as a parameter and update in place
> 
> >> + * @size: the size of the node to insert.
> >> + *
> >> + * Inserts a node into the DRM MM manager and clears the
> >> corresponding memory region
> >> + * in both the primary and shadow buffer objects.
> >> + *
> >> + * Return: 0 on success, or a negative error code on failure.
> >> + */
> >> +int xe_drm_mm_insert_node(struct xe_drm_mm_manager *drm_mm_manager,
> >> +			  struct drm_mm_node *node, u32 size)
> >> +{
> >> +	struct drm_mm *mm = &drm_mm_manager->base;
> >> +	int ret;
> >> +
> >> +	ret = drm_mm_insert_node(mm, node, size);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	memset((void *)drm_mm_manager->bo->vmap.vaddr + node->start,
> >> 0, node->size);
> 
> iosys_map_memset(bo->vmap, start, 0, size) ?
> 
> >> +	if (drm_mm_manager->shadow)
> >> +		memset((void *)drm_mm_manager->shadow->vmap.vaddr +
> >> node->start, 0,
> >> +		       node->size);
> 
> what about clearing the drm_mm_manager->cpu_addr ?
> 

None of the memset calls are needed [1].

[1] https://patchwork.freedesktop.org/patch/713119/?series=163588&rev=1#comment_1315262

> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + * xe_drm_mm_remove_node() - Remove a node from the DRM MM manager.
> >> + * @node: the DRM MM node to remove.
> >> + *
> >> + * Return: None.
> >> + */
> >> +void xe_drm_mm_remove_node(struct drm_mm_node *node)
> >> +{
> >> +	return drm_mm_remove_node(node);
> >> +}
> >> diff --git a/drivers/gpu/drm/xe/xe_drm_mm.h
> >> b/drivers/gpu/drm/xe/xe_drm_mm.h
> >> new file mode 100644
> >> index 000000000000..aeb7cab92d0b
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/xe/xe_drm_mm.h
> >> @@ -0,0 +1,55 @@
> >> +/* SPDX-License-Identifier: MIT */
> >> +/*
> >> + * Copyright © 2026 Intel Corporation
> >> + */
> >> +#ifndef _XE_DRM_MM_H_
> >> +#define _XE_DRM_MM_H_
> >> +
> >> +#include <linux/sizes.h>
> >> +#include <linux/types.h>
> >> +
> >> +#include "xe_bo.h"
> >> +#include "xe_drm_mm_types.h"
> >> +
> >> +struct dma_fence;
> 
> do we need this?
>

Nope.

Matt

> >> +struct xe_tile;
> >> +
> >> +#define XE_DRM_MM_BO_MANAGER_FLAG_SHADOW    BIT(0)
> >> +
> >> +struct xe_drm_mm_manager *xe_drm_mm_manager_init(struct xe_tile
> >> *tile, u32 size,
> >> +						 u32 guard, u32
> >> flags);
> >> +void xe_drm_mm_bo_swap_shadow(struct xe_drm_mm_manager
> >> *drm_mm_manager);
> >> +void xe_drm_mm_sync_shadow(struct xe_drm_mm_manager *drm_mm_manager,
> >> +			   struct drm_mm_node *node);
> >> +int xe_drm_mm_insert_node(struct xe_drm_mm_manager *drm_mm_manager,
> >> +			  struct drm_mm_node *node, u32 size);
> >> +void xe_drm_mm_remove_node(struct drm_mm_node *node);
> >> +
> >> +/**
> >> + * xe_drm_mm_manager_gpu_addr() - Retrieve GPU address of a back
> >> storage BO
> >> + * within a memory manager.
> >> + * @drm_mm_manager: The DRM MM memory manager.
> >> + *
> >> + * Returns: GGTT address of the back storage BO
> >> + */
> >> +static inline u64 xe_drm_mm_manager_gpu_addr(struct
> >> xe_drm_mm_manager
> >> +					     *drm_mm_manager)
> >> +{
> >> +	return xe_bo_ggtt_addr(drm_mm_manager->bo);
> >> +}
> >> +
> >> +/**
> >> + * xe_drm_mm_bo_swap_guard() - Retrieve the mutex used to guard swap
> >> operations
> 
> hmm, do we need the _bo_ here?
> 
> >> + * on a memory manager.
> >> + * @drm_mm_manager: The DRM MM memory manager.
> >> + *
> >> + * Returns: Swap guard mutex.
> >> + */
> >> +static inline struct mutex *xe_drm_mm_bo_swap_guard(struct
> >> xe_drm_mm_manager
> >> +						    *drm_mm_manager)
> >> +{
> >> +	return &drm_mm_manager->swap_guard;
> >> +}
> >> +
> >> +#endif
> >> +
> >> diff --git a/drivers/gpu/drm/xe/xe_drm_mm_types.h
> >> b/drivers/gpu/drm/xe/xe_drm_mm_types.h
> >> new file mode 100644
> >> index 000000000000..69e0937dd8de
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/xe/xe_drm_mm_types.h
> >> @@ -0,0 +1,42 @@
> >> +/* SPDX-License-Identifier: MIT */
> >> +/*
> >> + * Copyright © 2026 Intel Corporation
> >> + */
> >> +
> >> +#ifndef _XE_DRM_MM_TYPES_H_
> >> +#define _XE_DRM_MM_TYPES_H_
> >> +
> >> +#include <drm/drm_mm.h>
> >> +
> >> +struct xe_bo;
> >> +
> 
> without kernel-doc for the struct itself, below kernel-docs for the
> members are currently not recognized by the tool
> 
> >> +struct xe_drm_mm_manager {
> >> +	/** @base: Range allocator over [0, @size) in bytes */
> >> +	struct drm_mm base;
> >> +	/** @bo: Active pool BO (GGTT-pinned, CPU-mapped). */
> >> +	struct xe_bo *bo;
> >> +	/** @shadow: Shadow BO for atomic command updates. */
> >> +	struct xe_bo *shadow;
> >> +	/** @swap_guard: Timeline guard updating @bo and @shadow */
> >> +	struct mutex swap_guard;
> >> +	/** @cpu_addr: CPU virtual address of the active BO. */
> >> +	void *cpu_addr;
> >> +	/** @size: Total size of the managed address space. */
> >> +	u64 size;
> >> +	/** @is_iomem: Whether the managed address space is I/O
> >> memory. */
> >> +	bool is_iomem;
> >> +};
> >> +
> 
> ditto
> 
> >> +struct xe_drm_mm_bb {
> >> +	/** @node: Range node for this batch buffer. */
> >> +	struct drm_mm_node node;
> >> +	/** @manager: Manager this batch buffer belongs to. */
> >> +	struct xe_drm_mm_manager *manager;
> >> +	/** @cs: Command stream for this batch buffer. */
> >> +	u32 *cs;
> >> +	/** @len: Length of the CS in dwords. */
> >> +	u32 len;
> >> +};
> 
> but we are not using this struct yet in this patch, correct?
> 
> >> +
> >> +#endif
> >> +
>