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 12:06:21PM +0100, Thomas Hellström wrote:
> On Fri, 2026-03-27 at 11:54 +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
> 
> True, what I meant was it's not *implementing* a new range manager,
> like drm_mm, but rather using an existing.
> 
> > 
> > > 
> > > Could we invent a better name? 
> > > 
> > > xe_mm_suballoc? Something even better?
> > 
> > xe_shadow_pool ?
> 
> I noticed a new patch was sent out renaming to xe_mm_suballoc but 
> xe_shadow_pool sounds much better.

+1, tend to not look at naming nits when reviewing. xe_shadow_pool is a
better name.

Matt

> 
> Thanks,
> Thomas
> 
> 
> 
> > 
> > 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
> > 
> > > > +		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() 
> > 
> > > > +		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
> > 
> > > > + * @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?
> > 
> > > > +
> > > > +	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?
> > 
> > @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?
> > 
> > > > +}
> > > > +
> > > > +/**
> > > > + * 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
> > 
> > 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 ?
> > 
> > > > +	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?
> > 
> > > > +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
> > > > +