Re: [PATCH v5 3/6] drm/xe: Start using ggtt->start in preparation of balloon removal

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 10/10/2025 2:06 PM, Maarten Lankhorst wrote:
> Instead of having ggtt->size point to the end of ggtt, have ggtt->size
> be the actual size of the GGTT, and introduce ggtt->start to point to
> the beginning of GGTT.
> 
> This will allow a massive cleanup of GGTT in case of SRIOV-VF.
> 
> Reviewed-by: Stuart Summers <stuart.summers@intel.com>
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/xe/xe_ggtt.c               | 70 ++++++++++++----------
>  drivers/gpu/drm/xe/xe_ggtt.h               |  2 +
>  drivers/gpu/drm/xe/xe_ggtt_types.h         |  4 +-
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c |  4 +-
>  4 files changed, 47 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 33b09737ccba8..1fcb128e661b6 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -124,10 +124,20 @@ static void ggtt_update_access_counter(struct xe_ggtt *ggtt)
>  	}
>  }
>  

don't forget to add kernel-doc for all new public functions

> +u64 xe_ggtt_start(struct xe_ggtt *ggtt)
> +{
> +	return ggtt->start;
> +}
> +
> +u64 xe_ggtt_size(struct xe_ggtt *ggtt)
> +{
> +	return ggtt->size;
> +}
> +
>  static void xe_ggtt_set_pte(struct xe_ggtt *ggtt, u64 addr, u64 pte)
>  {
>  	xe_tile_assert(ggtt->tile, !(addr & XE_PTE_MASK));
> -	xe_tile_assert(ggtt->tile, addr < ggtt->size);
> +	xe_tile_assert(ggtt->tile, addr < ggtt->start + ggtt->size);

shouldn't we also check for

	xe_tile_assert(ggtt->tile, addr >= ggtt->start);

>  
>  	writeq(pte, &ggtt->gsm[addr >> XE_PTE_SHIFT]);
>  }
> @@ -233,16 +243,16 @@ static const struct xe_ggtt_pt_ops xelpg_pt_wa_ops = {
>  	.ggtt_set_pte = xe_ggtt_set_pte_and_flush,
>  };
>  
> -static void __xe_ggtt_init_early(struct xe_ggtt *ggtt, u32 reserved)
> +static void __xe_ggtt_init_early(struct xe_ggtt *ggtt, u64 start, u64 size)
>  {
> -	drm_mm_init(&ggtt->mm, reserved,
> -		    ggtt->size - reserved);
> +	ggtt->start = start;
> +	ggtt->size = size;
> +	drm_mm_init(&ggtt->mm, start, size);

since drm_mm is internal detail of the ggtt then maybe we should aim to just make this as:

	drm_mm_init(&ggtt->mm, 0, size);

and add "start" only in relevant ggtt_node functions (as ggtt_node is/will be also private) 

then we even will not require patch 2/6 since this "start" will be fully own by our ggtt

>  }
>  
> -int xe_ggtt_init_kunit(struct xe_ggtt *ggtt, u32 reserved, u32 size)
> +int xe_ggtt_init_kunit(struct xe_ggtt *ggtt, u32 start, u32 size)
>  {
> -	ggtt->size = size;
> -	__xe_ggtt_init_early(ggtt, reserved);
> +	__xe_ggtt_init_early(ggtt, start, size);
>  	return 0;
>  }
>  EXPORT_SYMBOL_IF_KUNIT(xe_ggtt_init_kunit);
> @@ -263,26 +273,32 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt)
>  	struct xe_device *xe = tile_to_xe(ggtt->tile);
>  	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>  	unsigned int gsm_size;
> +	u64 ggtt_start, wopcm = xe_wopcm_size(xe), ggtt_size;

maybe it's just me, but mixing plain var declarations with some assignments is not making the code any easier
what about:

	u64 wopcm = xe_wopcm_size(xe);
	u64 ggtt_start, ggtt_size;	

>  	int err;
>  
> -	if (IS_SRIOV_VF(xe) || GRAPHICS_VERx100(xe) >= 1250)
> -		gsm_size = SZ_8M; /* GGTT is expected to be 4GiB */
> -	else
> -		gsm_size = probe_gsm_size(pdev);
> -
> -	if (gsm_size == 0) {
> -		xe_tile_err(ggtt->tile, "Hardware reported no preallocated GSM\n");
> -		return -ENOMEM;
> +	if (!IS_SRIOV_VF(xe)) {
> +		if (GRAPHICS_VERx100(xe) >= 1250)
> +			gsm_size = SZ_8M; /* GGTT is expected to be 4GiB */
> +		else
> +			gsm_size = probe_gsm_size(pdev);
> +		if (gsm_size == 0) {
> +			xe_tile_err(ggtt->tile, "Hardware reported no preallocated GSM\n");
> +			return -ENOMEM;
> +		}
> +		ggtt_start = wopcm;
> +		ggtt_size = (gsm_size / 8) * (u64) XE_PAGE_SIZE - ggtt_start;
> +	} else {
> +		/* GGTT is expected to be 4GiB */
> +		ggtt_start = wopcm;
> +		ggtt_size = SZ_4G - ggtt_start;
>  	}
>  
>  	ggtt->gsm = ggtt->tile->mmio.regs + SZ_8M;
> -	ggtt->size = (gsm_size / 8) * (u64) XE_PAGE_SIZE;
> -
>  	if (IS_DGFX(xe) && xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)
>  		ggtt->flags |= XE_GGTT_FLAGS_64K;
>  
> -	if (ggtt->size > GUC_GGTT_TOP)
> -		ggtt->size = GUC_GGTT_TOP;
> +	if (ggtt_size + ggtt_start > GUC_GGTT_TOP)
> +		ggtt_size = GUC_GGTT_TOP - ggtt_start;
>  
>  	if (GRAPHICS_VERx100(xe) >= 1270)
>  		ggtt->pt_ops = (ggtt->tile->media_gt &&
> @@ -293,7 +309,7 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt)
>  		ggtt->pt_ops = &xelp_pt_ops;
>  
>  	ggtt->wq = alloc_workqueue("xe-ggtt-wq", 0, WQ_MEM_RECLAIM);
> -	__xe_ggtt_init_early(ggtt, xe_wopcm_size(xe));
> +	__xe_ggtt_init_early(ggtt, ggtt_start, ggtt_size);
>  
>  	err = drmm_add_action_or_reset(&xe->drm, ggtt_fini_early, ggtt);
>  	if (err)
> @@ -527,11 +543,9 @@ void xe_ggtt_node_remove_balloon_locked(struct xe_ggtt_node *node)
>  static void xe_ggtt_assert_fit(struct xe_ggtt *ggtt, u64 start, u64 size)
>  {
>  	struct xe_tile *tile = ggtt->tile;
> -	struct xe_device *xe = tile_to_xe(tile);
> -	u64 __maybe_unused wopcm = xe_wopcm_size(xe);
>  
> -	xe_tile_assert(tile, start >= wopcm);
> -	xe_tile_assert(tile, start + size < ggtt->size - wopcm);
> +	xe_tile_assert(tile, start >= ggtt->start);
> +	xe_tile_assert(tile, start + size <= ggtt->start + ggtt->size);
>  }
>  
>  /**
> @@ -840,14 +854,12 @@ u64 xe_ggtt_largest_hole(struct xe_ggtt *ggtt, u64 alignment, u64 *spare)
>  {
>  	const struct drm_mm *mm = &ggtt->mm;
>  	const struct drm_mm_node *entry;
> -	u64 hole_min_start = xe_wopcm_size(tile_to_xe(ggtt->tile));
>  	u64 hole_start, hole_end, hole_size;
>  	u64 max_hole = 0;
>  
>  	mutex_lock(&ggtt->lock);
> -
>  	drm_mm_for_each_hole(entry, mm, hole_start, hole_end) {
> -		hole_start = max(hole_start, hole_min_start);
> +		hole_start = max(hole_start, ggtt->start);
>  		hole_start = ALIGN(hole_start, alignment);
>  		hole_end = ALIGN_DOWN(hole_end, alignment);
>  		if (hole_start >= hole_end)
> @@ -940,15 +952,13 @@ u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 alignment, struct drm_printer
>  {
>  	const struct drm_mm *mm = &ggtt->mm;
>  	const struct drm_mm_node *entry;
> -	u64 hole_min_start = xe_wopcm_size(tile_to_xe(ggtt->tile));
>  	u64 hole_start, hole_end, hole_size;
>  	u64 total = 0;
>  	char buf[10];
>  
>  	mutex_lock(&ggtt->lock);
> -
>  	drm_mm_for_each_hole(entry, mm, hole_start, hole_end) {
> -		hole_start = max(hole_start, hole_min_start);
> +		hole_start = max(hole_start, ggtt->start);
>  		hole_start = ALIGN(hole_start, alignment);
>  		hole_end = ALIGN_DOWN(hole_end, alignment);
>  		if (hole_start >= hole_end)
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
> index 75fc7a1efea76..6482bddb2ef36 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt.h
> @@ -23,6 +23,8 @@ int xe_ggtt_node_insert_balloon_locked(struct xe_ggtt_node *node,
>  				       u64 start, u64 size);
>  void xe_ggtt_node_remove_balloon_locked(struct xe_ggtt_node *node);
>  void xe_ggtt_shift_nodes_locked(struct xe_ggtt *ggtt, s64 shift);
> +u64 xe_ggtt_start(struct xe_ggtt *ggtt);
> +u64 xe_ggtt_size(struct xe_ggtt *ggtt);
>  
>  int xe_ggtt_node_insert(struct xe_ggtt_node *node, u32 size, u32 align);
>  int xe_ggtt_node_insert_locked(struct xe_ggtt_node *node,
> diff --git a/drivers/gpu/drm/xe/xe_ggtt_types.h b/drivers/gpu/drm/xe/xe_ggtt_types.h
> index c5e999d58ff2a..a27919302d6b2 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt_types.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt_types.h
> @@ -22,7 +22,9 @@ struct xe_gt;
>  struct xe_ggtt {
>  	/** @tile: Back pointer to tile where this GGTT belongs */
>  	struct xe_tile *tile;
> -	/** @size: Total size of this GGTT */
> +	/** @start: Start offset of GGTT */
> +	u64 start;
> +	/** @size: Total usable size of this GGTT */
>  	u64 size;
>  
>  #define XE_GGTT_FLAGS_64K BIT(0)
> 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 b2e5c52978e6a..2289756761636 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> @@ -343,8 +343,8 @@ static int pf_push_full_vf_config(struct xe_gt *gt, unsigned int vfid)
>  	xe_gt_assert(gt, num_dwords <= max_cfg_dwords);
>  
>  	if (vfid == PFID) {
> -		u64 ggtt_start = xe_wopcm_size(gt_to_xe(gt));
> -		u64 ggtt_size = gt_to_tile(gt)->mem.ggtt->size - ggtt_start;
> +		u64 ggtt_start = xe_ggtt_start(gt_to_tile(gt)->mem.ggtt);
> +		u64 ggtt_size = xe_ggtt_size(gt_to_tile(gt)->mem.ggtt);
>  
>  		/* plain PF config data will never include a real GGTT region */
>  		xe_gt_assert(gt, !encode_config_ggtt(cfg + num_dwords, config, true));