Re: [PATCH v3 2/3] drm/xe/mm: Add batch buffer allocation functions for xe_mem_pool manager

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Wed, 2026-04-01 at 16:15 +0000, Satyanarayana K V P wrote:
> New APIs xe_pool_bb_alloc(), xe_pool_bb_insert() and
> xe_pool_bb_free() are created to manage allocations from the
> xe_mem_pool manager.
> 
> 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>
> 
> ---
> V2 -> V3:
> - Renamed xe_mm_suballoc to xe_mem_pool.
> 
> V1 -> V2:
> - Renamed xe_drm_mm to xe_mm_suballoc (Thomas)
> - Removed memset from xe_drm_mm_bb_insert() (Matt).
> ---

Claude's kreview:

> diff --git a/drivers/gpu/drm/xe/xe_bb.c b/drivers/gpu/drm/xe/xe_bb.c

[ ... ]

> +/**
> + * xe_pool_bb_alloc() - Allocate a new batch buffer structure for
drm_mm
> + *
> + * Allocates a new xe_pool_bb structure for use with xe_pool memory
> + * management.
> + *
> + * Returns: Batch buffer structure or an ERR_PTR(-ENOMEM).
> + */
> +struct xe_mem_pool_bb *xe_pool_bb_alloc(void)

The kerneldoc says "Allocates a new xe_pool_bb structure" but the
returned type is struct xe_mem_pool_bb. The type xe_pool_bb does not
exist. Should the description read "xe_mem_pool_bb structure"?

> +/**
> + * xe_pool_bb_insert() - Initialize a batch buffer and insert a hole
> + * @bb: Batch buffer structure to initialize
> + * @bb_pool: drm_mm manager to allocate from
> + * @dwords: Number of dwords to be allocated
> + *
> + * Initializes the batch buffer by allocating memory from the
specified
> + * drm_mm manager.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int xe_pool_bb_insert(struct xe_mem_pool_bb *bb,
> +		      struct xe_mem_pool_manager *bb_pool, u32 dwords)

The one-line description says "insert a hole", but in drm_mm
terminology a hole is available free space. This function calls
xe_mem_pool_insert_node() which calls drm_mm_insert_node() to
allocate a node from within a hole. Should the description say
"insert a node" instead of "insert a hole"?

Also, xe_mem_pool_insert_node() and xe_mem_pool_remove_node() have
no internal locking. The current caller in xe_migrate.c wraps both
xe_pool_bb_insert() and xe_pool_bb_free() inside:

    scoped_guard(mutex, xe_mem_pool_bo_swap_guard(bb_pool)) {

but neither function documents this requirement. Would it make sense
to add a locking precondition to the kerneldoc of both functions,
similar to the _locked suffix convention used elsewhere in this file?

> +/**
> + * xe_pool_bb_free() - Free a batch buffer allocated with drm_mm
> + * @bb: Batch buffer structure to free
> + */
> +void xe_pool_bb_free(struct xe_mem_pool_bb *bb)

Same locking question as xe_pool_bb_insert() above --
drm_mm_remove_node()
requires external locking and the caller holds swap_guard, but this
is not documented here.

Also, this isn't a bug, but the new functions are named xe_pool_bb_*
while the type they operate on is struct xe_mem_pool_bb and the
manager type is struct xe_mem_pool_manager. The existing infrastructure
uses the xe_mem_pool_ prefix consistently. Would
xe_mem_pool_bb_alloc(),
xe_mem_pool_bb_insert(), and xe_mem_pool_bb_free() be more consistent
with the rest of the subsystem?

/Thomas