Re: [PATCH v6 2/4] drm/xe/ggtt: Split GGTT into usable and shareable pools

["Piotr Piórkowski" <piotr.piorkowski@intel.com>]

Lis, Tomasz <tomasz.lis@intel.com> wrote on śro [2026-paź-21 03:01:22 +0200]:
> 
> On 2026-10-20 11:55, Piórkowski, Piotr wrote:
> > From: Piotr Piórkowski <piotr.piorkowski@intel.com>
> > 
> > Driver-owned GGTT allocations and VF provisioning currently use a single
> > GGTT pool. Split it into a usable pool for driver-owned allocations and a
> > shareable pool for VF provisioning.
> > 
> > The pools are separate logical ranges and may overlap, but allocations are
> > limited to the configured size of their corresponding pool. Allocate from
> > the bottom of the usable pool and from the top of the shareable pool when a
> > shareable pool is present.
> > 
> > Add separate insertion APIs for the usable and shareable pools.
> > 
> > v2:
> >   - Rename ggtt->usable_size back to ggtt->size.
> >   - Remove the ggtt_insert_node_in_range() helper.
> > v3:
> >   - Introduce the full_size struct field instead of the ggtt_accessible_size
> >     function.
> > v4:
> >   - Set the shareable pool size only for SR-IOV PF,
> >   - Return -ENOSPC instead of asserting when a large BO no longer fits after
> >     end is clamped to ggtt->size.
> > v5:
> >   - Move range initializer rename and full_size parameter here.
> >   - Use shareable_size and compute the shareable start on demand.
> >   - Constrain regular allocations to the usable pool.
> > 
> > Assisted-by: Claude:claude-5-sonnet
> > Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <dev@lankhorst.se>
> > ---
> >   drivers/gpu/drm/xe/xe_ggtt.c | 166 +++++++++++++++++++++++++++++------
> >   drivers/gpu/drm/xe/xe_ggtt.h |   4 +
> >   2 files changed, 143 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> > index abcc929c08535..8c8354d47782d 100644
> > --- a/drivers/gpu/drm/xe/xe_ggtt.c
> > +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> > @@ -145,8 +145,19 @@ struct xe_ggtt {
> >   	unsigned int access_count;
> >   	/** @wq: Dedicated unordered work queue to process node removals */
> >   	struct workqueue_struct *wq;
> > +#ifdef CONFIG_PCI_IOV
> > +	/** @shareable_size: Size of the GGTT range reserved for sharing with VFs */
> > +	u64 shareable_size;
> > +#endif
> >   };
> > +#ifdef CONFIG_PCI_IOV
> > +static inline u64 ggtt_shareable_start(struct xe_ggtt *ggtt)
> 
> This name is unfortunate. A function with `start` in name actually ignores
> the `ggtt->start`
> 
> and returns a shift/offset within range of nodes assigned to this `xe_ggtt`,
> rather than GVA.
> 
> Can we use something more matching rather than `start`, or add a word which
> would
> 
> emphasize that this returns a relative value rather than not Global Virtual
> Address?
>

Agreed, I'll rename it to ggtt_shareable_offset().


> > +{
> > +	return ggtt->full_size - ggtt->shareable_size;
> > +}
> > +#endif
> > +
> >   static u64 xelp_ggtt_pte_flags(struct xe_bo *bo, u16 pat_index)
> >   {
> >   	u64 pte = XE_PAGE_PRESENT;
> > @@ -358,17 +369,30 @@ static const struct xe_ggtt_pt_ops xelpg_pt_wa_ops = {
> >   	.ggtt_get_pte = xe_ggtt_get_pte,
> >   };
> > -static void __xe_ggtt_init_early(struct xe_ggtt *ggtt, u64 start, u64 size)
> > +static void ggtt_init_ranges(struct xe_ggtt *ggtt, u64 start, u64 full_size,
> > +			     u64 usable_size, u64 shareable_size)
> >   {
> > +	struct xe_gt *gt = ggtt->tile->primary_gt;
> > +
> > +	xe_gt_assert(gt, full_size);
> > +	xe_gt_assert(gt, usable_size);
> > +	xe_gt_assert(gt, usable_size <= full_size);
> > +	xe_gt_assert(gt, shareable_size <= full_size);
> 
> Does that mean usable area and shareable area can overlap?
> 
> If they cannot overlap, then we should assert for sum of the sizes.

Yes, they can overlap. On PF both pools cover the whole range, and drm_mm keeps
the allocations apart (driver allocates from the bottom, VFs from the top).
So an assert on the sum would fail on PF. I'll add a comment to make it clear.

> 
> > +
> >   	ggtt->start = start;
> > -	ggtt->full_size = size;
> > -	ggtt->size = size;
> > -	drm_mm_init(&ggtt->mm, 0, size);
> > +	ggtt->full_size = full_size;
> > +	ggtt->size = usable_size;
> > +
> > +#ifdef CONFIG_PCI_IOV
> > +	if (shareable_size)
> > +		ggtt->shareable_size = shareable_size;
> > +#endif
> > +	drm_mm_init(&ggtt->mm, 0, ggtt->full_size);
> >   }
> >   int xe_ggtt_init_kunit(struct xe_ggtt *ggtt, u32 start, u32 size)
> >   {
> > -	__xe_ggtt_init_early(ggtt, start, size);
> > +	ggtt_init_ranges(ggtt, start, size, size, 0);
> >   	return 0;
> >   }
> >   EXPORT_SYMBOL_IF_KUNIT(xe_ggtt_init_kunit);
> > @@ -445,7 +469,8 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt)
> >   	if (!ggtt->wq)
> >   		return -ENOMEM;
> > -	__xe_ggtt_init_early(ggtt, ggtt_start, ggtt_size);
> > +	ggtt_init_ranges(ggtt, ggtt_start, ggtt_size, ggtt_size,
> > +			 IS_SRIOV_PF(xe) ? ggtt_size : 0);
> >   	err = drmm_add_action_or_reset(&xe->drm, ggtt_fini_early, ggtt);
> >   	if (err)
> > @@ -639,6 +664,11 @@ static void xe_ggtt_invalidate_engine(struct xe_ggtt *ggtt)
> >   	ggtt_invalidate_gt_engine_tlb(ggtt->tile->media_gt);
> >   }
> > +static inline u64 xe_ggtt_bottom(struct xe_ggtt *ggtt)
> > +{
> > +	return xe_wopcm_size(tile_to_xe(ggtt->tile));
> > +}
> > +
> >   /**
> >    * xe_ggtt_shift_nodes() - Shift GGTT nodes to adjust for a change in usable address range.
> >    * @ggtt: the &xe_ggtt struct instance
> > @@ -650,24 +680,63 @@ static void xe_ggtt_invalidate_engine(struct xe_ggtt *ggtt)
> >    * This function may be called multiple times during recovery, but if
> >    * @new_start is unchanged from the current base, it's a noop.
> >    *
> > - * @new_start should be a value between xe_wopcm_size() and #GUC_GGTT_TOP.
> > + * @new_start should be a value between the GGTT bottom and #GUC_GGTT_TOP.
> >    */
> >   void xe_ggtt_shift_nodes(struct xe_ggtt *ggtt, u64 new_start)
> >   {
> >   	guard(mutex)(&ggtt->lock);
> > -	xe_tile_assert(ggtt->tile, new_start >= xe_wopcm_size(tile_to_xe(ggtt->tile)));
> > +	xe_tile_assert(ggtt->tile, new_start >= xe_ggtt_bottom(ggtt));
> >   	xe_tile_assert(ggtt->tile, new_start + ggtt->size <= GUC_GGTT_TOP);
> > +#ifdef CONFIG_PCI_IOV
> > +	xe_tile_assert(ggtt->tile, ggtt->shareable_size == 0);
> > +#endif
> >   	/* pairs with READ_ONCE in xe_ggtt_node_addr() */
> >   	WRITE_ONCE(ggtt->start, new_start);
> >   }
> > -static int xe_ggtt_insert_node_locked(struct xe_ggtt_node *node,
> > -				      u32 size, u32 align, u32 mm_flags)
> > +static int ggtt_insert_node_in_range_locked(struct xe_ggtt_node *node, u32 size,
> > +					    u32 align, u64 range_start,
> > +					    u64 range_size, u32 mm_flags)
> > +{
> > +	struct xe_ggtt *ggtt = node->ggtt;
> > +	u64 range_end = range_start + range_size;
> > +
> > +	lockdep_assert_held(&ggtt->lock);
> > +
> > +	if (!range_size || range_end <= range_start)
> > +		return -EINVAL;
> > +
> > +	if (range_end > ggtt->full_size)
> > +		return -ERANGE;
> > +
> > +	if (size > range_size)
> > +		return -ENOSPC;
> > +
> > +	return drm_mm_insert_node_in_range(&ggtt->mm, &node->base, size, align, 0,
> > +					  range_start, range_end, mm_flags);
> > +}
> > +
> > +/*
> > + * When the shareable range is present, allocations start from the bottom
> > + * to leave space at the top; otherwise they start from the top.
> > + */
> > +static u32 ggtt_usable_insert_flags(struct xe_ggtt *ggtt)
> >   {
> > -	return drm_mm_insert_node_generic(&node->ggtt->mm, &node->base, size, align, 0,
> > -					  mm_flags);
> > +#ifdef CONFIG_PCI_IOV
> > +	if (ggtt->shareable_size > 0)
> > +		return DRM_MM_INSERT_LOW;
> > +#endif
> > +	return DRM_MM_INSERT_HIGH;
> > +}
> > +
> > +static int xe_ggtt_insert_node_locked(struct xe_ggtt_node *node, u32 size, u32 align)
> > +{
> > +	struct xe_ggtt *ggtt = node->ggtt;
> > +
> > +	return ggtt_insert_node_in_range_locked(node, size, align, 0,
> > +						ggtt->size, ggtt_usable_insert_flags(ggtt));
> >   }
> >   static struct xe_ggtt_node *ggtt_node_init(struct xe_ggtt *ggtt)
> > @@ -684,12 +753,12 @@ static struct xe_ggtt_node *ggtt_node_init(struct xe_ggtt *ggtt)
> >   }
> >   /**
> > - * xe_ggtt_insert_node - Insert a &xe_ggtt_node into the GGTT
> > + * xe_ggtt_insert_node() - Insert a &xe_ggtt_node into the GGTT
> >    * @ggtt: the &xe_ggtt into which the node should be inserted.
> >    * @size: size of the node
> >    * @align: alignment constrain of the node
> >    *
> > - * Return: &xe_ggtt_node on success or a ERR_PTR on failure.
> > + * Return: &xe_ggtt_node on success or an ERR_PTR on failure.
> >    */
> >   struct xe_ggtt_node *xe_ggtt_insert_node(struct xe_ggtt *ggtt, u32 size, u32 align)
> >   {
> > @@ -701,8 +770,45 @@ struct xe_ggtt_node *xe_ggtt_insert_node(struct xe_ggtt *ggtt, u32 size, u32 ali
> >   		return node;
> >   	guard(mutex)(&ggtt->lock);
> > -	ret = xe_ggtt_insert_node_locked(node, size, align,
> > -					 DRM_MM_INSERT_HIGH);
> > +
> > +	ret = xe_ggtt_insert_node_locked(node, size, align);
> > +	if (ret) {
> > +		ggtt_node_fini(node);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	return node;
> > +}
> > +
> > +#ifdef CONFIG_PCI_IOV
> > +/**
> > + * xe_ggtt_insert_node_shareable() - Insert a new node in the shareable GGTT range
> > + * @ggtt: the &xe_ggtt into which the node should be inserted.
> > + * @size: size of the node
> > + * @align: alignment constrain of the node
> > + *
> > + * Allocations always start from the top (DRM_MM_INSERT_HIGH).
> > + *
> > + * Return: &xe_ggtt_node on success or an ERR_PTR on failure.
> > + */
> > +struct xe_ggtt_node *xe_ggtt_insert_node_shareable(struct xe_ggtt *ggtt, u32 size, u32 align)
> > +{
> > +	struct xe_ggtt_node *node;
> > +	int ret;
> > +
> > +	if (!ggtt->shareable_size)
> > +		return ERR_PTR(-ENOSPC);
> > +
> > +	node = ggtt_node_init(ggtt);
> > +	if (IS_ERR(node))
> > +		return node;
> > +
> > +	guard(mutex)(&ggtt->lock);
> > +
> > +	ret = ggtt_insert_node_in_range_locked(node, size, align,
> > +					       ggtt_shareable_start(ggtt),
> > +					       ggtt->shareable_size,
> > +					       DRM_MM_INSERT_HIGH);
> >   	if (ret) {
> >   		ggtt_node_fini(node);
> >   		return ERR_PTR(ret);
> > @@ -710,6 +816,7 @@ struct xe_ggtt_node *xe_ggtt_insert_node(struct xe_ggtt *ggtt, u32 size, u32 ali
> >   	return node;
> >   }
> > +#endif
> >   /**
> >    * xe_ggtt_node_pt_size() - Get the size of page table entries needed to map a GGTT node.
> > @@ -793,6 +900,7 @@ void xe_ggtt_map_bo_unlocked(struct xe_ggtt *ggtt, struct xe_bo *bo)
> >    *
> >    * This function allows inserting a GGTT node with a custom transformation function.
> >    * This is useful for display to allow inserting rotated framebuffers to GGTT.
> > + * Allocates from the usable range only.
> >    *
> >    * Return: A pointer to %xe_ggtt_node struct on success. An ERR_PTR otherwise.
> >    */
> > @@ -813,7 +921,7 @@ struct xe_ggtt_node *xe_ggtt_insert_node_transform(struct xe_ggtt *ggtt,
> >   		goto err;
> >   	}
> > -	ret = xe_ggtt_insert_node_locked(node, size, align, 0);
> > +	ret = xe_ggtt_insert_node_locked(node, size, align);
> >   	if (ret)
> >   		goto err_unlock;
> > @@ -835,6 +943,7 @@ struct xe_ggtt_node *xe_ggtt_insert_node_transform(struct xe_ggtt *ggtt,
> >   static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
> >   				  u64 start, u64 end, struct drm_exec *exec)
> >   {
> > +	struct xe_device *xe = tile_to_xe(ggtt->tile);
> >   	u64 alignment = bo->min_align > 0 ? bo->min_align : XE_PAGE_SIZE;
> >   	u8 tile_id = ggtt->tile->id;
> >   	int err;
> > @@ -852,16 +961,16 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
> >   	if (err)
> >   		return err;
> > -	xe_pm_runtime_get_noresume(tile_to_xe(ggtt->tile));
> > +	guard(xe_pm_runtime_noresume)(xe);
> >   	bo->ggtt_node[tile_id] = ggtt_node_init(ggtt);
> >   	if (IS_ERR(bo->ggtt_node[tile_id])) {
> >   		err = PTR_ERR(bo->ggtt_node[tile_id]);
> >   		bo->ggtt_node[tile_id] = NULL;
> > -		goto out;
> > +		return err;
> >   	}
> > -	mutex_lock(&ggtt->lock);
> > +	guard(mutex)(&ggtt->lock);
> >   	/*
> >   	 * When inheriting the initial framebuffer, the framebuffer is
> >   	 * physically located at VRAM address 0, and usually at GGTT address 0 too.
> > @@ -880,28 +989,31 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
> >   	else
> >   		end = 0;
> > -	xe_tile_assert(ggtt->tile, end >= start + xe_bo_size(bo));
> > +	end = min(end, ggtt->size);
> > +
> > +	if (end < start + xe_bo_size(bo)) {
> > +		ggtt_node_fini(bo->ggtt_node[tile_id]);
> > +		bo->ggtt_node[tile_id] = NULL;
> > +		return -ENOSPC;
> > +	}
> >   	err = drm_mm_insert_node_in_range(&ggtt->mm, &bo->ggtt_node[tile_id]->base,
> > -					  xe_bo_size(bo), alignment, 0, start, end, 0);
> > +					  xe_bo_size(bo), alignment, 0, start, end,
> > +					  ggtt_usable_insert_flags(ggtt));
> >   	if (err) {
> >   		ggtt_node_fini(bo->ggtt_node[tile_id]);
> >   		bo->ggtt_node[tile_id] = NULL;
> >   	} else {
> >   		u16 cache_mode = bo->flags & XE_BO_FLAG_NEEDS_UC ? XE_CACHE_NONE : XE_CACHE_WB;
> > -		u16 pat_index = xe_cache_pat_idx(tile_to_xe(ggtt->tile), cache_mode);
> > +		u16 pat_index = xe_cache_pat_idx(xe, cache_mode);
> >   		u64 pte = ggtt->pt_ops->pte_encode_flags(bo, pat_index);
> >   		xe_ggtt_map_bo(ggtt, bo->ggtt_node[tile_id], bo, pte);
> >   	}
> > -	mutex_unlock(&ggtt->lock);
> 
> We're switching to keeping GGTT mutex throughout GGTT invalidation. This
> creates dependency between
> 
> this mutex and invalidation mutex, and also keeps the lock during GuC
> communication, which greatly
> 
> increases the time window in which it's locked (the previous operations are
> insignificant time-wise in
> 
> comparison to GuC comms). Do we have a reason for that, or is switching to
> the guard just a style fix?
>

No real reason, it was just a style change. I'll go back to the explicit unlock before
the invalidation

> -Tomasz
> 

Thanks for review!
Piotr

> >   	if (!err && bo->flags & XE_BO_FLAG_GGTT_INVALIDATE)
> >   		xe_ggtt_invalidate(ggtt);
> > -out:
> > -	xe_pm_runtime_put(tile_to_xe(ggtt->tile));
> > -
> >   	return err;
> >   }
> > diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
> > index c864cc975a695..59b2f02870195 100644
> > --- a/drivers/gpu/drm/xe/xe_ggtt.h
> > +++ b/drivers/gpu/drm/xe/xe_ggtt.h
> > @@ -24,6 +24,10 @@ u64 xe_ggtt_size(struct xe_ggtt *ggtt);
> >   struct xe_ggtt_node *
> >   xe_ggtt_insert_node(struct xe_ggtt *ggtt, u32 size, u32 align);
> > +#ifdef CONFIG_PCI_IOV
> > +struct xe_ggtt_node *
> > +xe_ggtt_insert_node_shareable(struct xe_ggtt *ggtt, u32 size, u32 align);
> > +#endif
> >   struct xe_ggtt_node *
> >   xe_ggtt_insert_node_transform(struct xe_ggtt *ggtt,
> >   			      struct xe_bo *bo, u64 pte,

--