On Fri, May 29, 2026 at 12:11:22AM +0200, Michal Wajdeczko wrote: > > > On 5/27/2026 4:45 PM, Rodrigo Vivi wrote: > > The PF GGTT allocator was initialised over a relative [0, usable_size) > > range, with ggtt->start added on every address conversion to get the > > actual hardware address. Two consequences of that model were considered > > "horrible hacks": > > > > - ggtt->start (the WOPCM offset) had to be carried around and added > > to every drm_mm result. > > hmm, but this an internal detail of the xe_ggtt implementation, so why > would someone else complain about it? > > > - The GUC_GGTT_TOP ceiling silently truncated the GGTT range instead > > hmm, for the record, this GGTT cap on the top was added back in 2023 > > commit ab10e976fbda8349163ceee2ce99b2bfc97031b8 > Author: Daniele Ceraolo Spurio <[email protected]> > Date: Wed Jun 14 10:47:54 2023 -0700 > > drm/xe: limit GGTT size to GUC_GGTT_TOP > > + * The GuC address space is limited on both ends of the GGTT, because > + * the GuC shim HW redirects accesses to those addresses to other HW > + * areas instead of going through the GGTT. On the bottom end, the GuC > + * can't access offsets below the WOPCM size, while on the top side > the > + * limit is fixed at GUC_GGTT_TOP. To keep things simple, instead of > + * checking each object to see if they are accessed by GuC or not, we > + * just exclude those areas from the allocator. Additionally, to > + * simplify the driver load, we use the maximum WOPCM size in this > logic > > > of being made explicit, leaving PTEs in [GUC_GGTT_TOP, total_size) > > untouched during the initial clear. > > and that likely will not be changed by this patch as after allocating 'two > permanent zones', the drm_mm_for_each_hole will not iterate over them
right... > > > > > Fix this for the PF case by initialising drm_mm over the full hardware > > GGTT range [0, total_size) and permanently reserving the two forbidden > > zones: > > > > - [0, wopcm) — inaccessible below WOPCM > > - [GUC_GGTT_TOP, total_size) — inaccessible above GUC_GGTT_TOP > > that looks odds: why pretend to claim manageability of full [0, 4GB) > of the GGTT and then immediately permanently reserve two end zones to > end up with real [wopcm, GUC_TOP) which is what we already have? yes... > > > > > A new mm_offset field (zero for PF) carries the base offset used in > > address conversions, unifying the existing VF relative model (where > > mm_offset == vf_base) with the new PF absolute model. > > but public xe_ggtt API already uses absolute addressing in PF and VF I know... > > > The public > > xe_ggtt_start() / xe_ggtt_size() API continues to return the usable > > [wopcm, GUC_GGTT_TOP) boundaries, so callers such as the SR-IOV PF > > config code are unaffected. > > > > xe_ggtt_shift_nodes() now updates both ggtt->start and ggtt->mm_offset > > so the VF recovery path remains a single O(1) WRITE_ONCE pair. > > maybe it's just me - but I can't figure out the real rationale for this > patch - what did I miss? This series: https://lore.kernel.org/intel-xe/[email protected]/ And more specifically the discussion in this patch: https://lore.kernel.org/intel-xe/[email protected]/ > > > > > Suggested-by: Ville Syrjälä <[email protected]> > > Cc: Michal Wajdeczko <[email protected]> > > Cc: Maarten Lankhorst <[email protected]> > > Assisted-by: GitHub-Copilot:claude-sonnet-4.6 > > Signed-off-by: Rodrigo Vivi <[email protected]> > > --- > > drivers/gpu/drm/xe/xe_ggtt.c | 123 ++++++++++++++++++++++++++++------- > > 1 file changed, 101 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c > > index a351c578b170..00a6cd2b8a51 100644 > > --- a/drivers/gpu/drm/xe/xe_ggtt.c > > +++ b/drivers/gpu/drm/xe/xe_ggtt.c > > @@ -137,6 +137,17 @@ struct xe_ggtt { > > const struct xe_ggtt_pt_ops *pt_ops; > > /** @mm: The memory manager used to manage individual GGTT allocations > > */ > > struct drm_mm mm; > > + /** > > + * @mm_offset: base offset added to drm_mm node addresses to obtain > > hardware > > + * GGTT addresses. For PF this is 0 (drm_mm uses absolute hardware > > addresses). > > + * For VF this equals @start (drm_mm uses relative addresses from VF > > base). > > + * Updated atomically by xe_ggtt_shift_nodes() during VF recovery. > > + */ > > + u64 mm_offset; > > + /** @reserved_bottom: permanently reserved [0, WOPCM) drm_mm node for > > PF */ > > + struct drm_mm_node reserved_bottom; > > + /** @reserved_top: permanently reserved [GUC_GGTT_TOP, total) drm_mm > > node for PF */ > > + struct drm_mm_node reserved_top; > > maybe all we need is to separate concepts of: > > * raw GGTT - fixed range [0, 4GB) > > from > > * allocable GGTT - configurable sub-range [start, end) > * [wopcm, GUC_TOP) on PF > * [base, base+size) on VF > > and then we can continue to use drm_mm.init(0, end-start) to manage > that [start, end) range in a common way on both PF and VF? we need to be able to use a ggtt buffer that comes out of this range, so I'm afraid it doesn't solve all the cases. > > > > /** @access_count: counts GGTT writes */ > > unsigned int access_count; > > /** @wq: Dedicated unordered work queue to process node removals */ > > @@ -318,6 +329,10 @@ static void ggtt_fini_early(struct drm_device *drm, > > void *arg) > > { > > struct xe_ggtt *ggtt = arg; > > > > + if (drm_mm_node_allocated(&ggtt->reserved_top)) > > + drm_mm_remove_node(&ggtt->reserved_top); > > + if (drm_mm_node_allocated(&ggtt->reserved_bottom)) > > + drm_mm_remove_node(&ggtt->reserved_bottom); > > destroy_workqueue(ggtt->wq); > > drm_mm_takedown(&ggtt->mm); > > } > > @@ -354,16 +369,66 @@ 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) > > +/* > > + * __xe_ggtt_init_early - Generic drm_mm initialisation for both PF and VF. > > + * > > + * @start and @size define the usable GGTT range exposed through the > > public API. > > + * @mm_offset is added to every drm_mm node address to obtain the hardware > > + * address (0 for PF where the drm_mm spans real addresses; @start for VF). > > + * @mm_size is the total span passed to drm_mm_init() (equals @size for VF; > > + * equals the full hardware GGTT size for PF). > > + */ > > +static void __xe_ggtt_init_early(struct xe_ggtt *ggtt, u64 start, u64 size, > > + u64 mm_offset, u64 mm_size) > > { > > ggtt->start = start; > > ggtt->size = size; > > - drm_mm_init(&ggtt->mm, 0, size); > > + ggtt->mm_offset = mm_offset; > > + drm_mm_init(&ggtt->mm, 0, mm_size); > > +} > > + > > +/* > > + * __xe_ggtt_reserve_pf_nodes - Permanently reserve the forbidden GGTT > > zones. > > + * > > + * Must be called after __xe_ggtt_init_early() for the PF case. Reserves > > + * [0, @wopcm) and [@guc_top, @total_size) so the allocator never hands > > them > > + * out. On failure the drm_mm is torn down and the error is returned. > > + */ > > +static int __xe_ggtt_reserve_pf_nodes(struct xe_ggtt *ggtt, u64 wopcm, > > + u64 guc_top, u64 total_size) > > +{ > > + int err; > > + > > + /* Reserve [0, wopcm) — GuC cannot access below WOPCM */ > > + if (wopcm > 0) { > > + ggtt->reserved_bottom.start = 0; > > + ggtt->reserved_bottom.size = wopcm; > > + err = drm_mm_reserve_node(&ggtt->mm, &ggtt->reserved_bottom); > > + if (WARN_ON(err)) > > + goto err_takedown; > > + } > > + > > + /* Reserve [guc_top, total_size) — GuC cannot access above GUC_GGTT_TOP > > */ > > + if (guc_top < total_size) { > > + ggtt->reserved_top.start = guc_top; > > + ggtt->reserved_top.size = total_size - guc_top; > > + err = drm_mm_reserve_node(&ggtt->mm, &ggtt->reserved_top); > > + if (WARN_ON(err)) > > + goto err_remove_bottom; > > + } > > + > > + return 0; > > + > > +err_remove_bottom: > > + drm_mm_remove_node(&ggtt->reserved_bottom); > > +err_takedown: > > + drm_mm_takedown(&ggtt->mm); > > + return err; > > } > > > > int xe_ggtt_init_kunit(struct xe_ggtt *ggtt, u32 start, u32 size) > > { > > - __xe_ggtt_init_early(ggtt, start, size); > > + __xe_ggtt_init_early(ggtt, start, size, start, size); > > return 0; > > } > > EXPORT_SYMBOL_IF_KUNIT(xe_ggtt_init_kunit); > > @@ -405,8 +470,13 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt) > > xe_tile_err(ggtt->tile, "Hardware reported no > > preallocated GSM\n"); > > return -ENOMEM; > > } > > + /* > > + * For PF, ggtt_size holds the full hardware GGTT size. The > > + * WOPCM and GUC_GGTT_TOP limits are enforced via permanently > > + * reserved drm_mm nodes rather than by capping the range. > > + */ > > ggtt_start = wopcm; > > - ggtt_size = (gsm_size / 8) * (u64)XE_PAGE_SIZE - ggtt_start; > > + ggtt_size = (gsm_size / 8) * (u64)XE_PAGE_SIZE; > > } else { > > ggtt_start = xe_tile_sriov_vf_ggtt_base(ggtt->tile); > > ggtt_size = xe_tile_sriov_vf_ggtt(ggtt->tile); > > @@ -423,9 +493,6 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt) > > if (IS_DGFX(xe) && xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K) > > ggtt->flags |= XE_GGTT_FLAGS_64K; > > > > - 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 && XE_GT_WA(ggtt->tile->media_gt, > > 22019338487)) || > > @@ -438,7 +505,19 @@ int xe_ggtt_init_early(struct xe_ggtt *ggtt) > > if (!ggtt->wq) > > return -ENOMEM; > > > > - __xe_ggtt_init_early(ggtt, ggtt_start, ggtt_size); > > + if (!IS_SRIOV_VF(xe)) { > > + __xe_ggtt_init_early(ggtt, ggtt_start, GUC_GGTT_TOP - > > ggtt_start, > > + 0, ggtt_size); > > + err = __xe_ggtt_reserve_pf_nodes(ggtt, ggtt_start, GUC_GGTT_TOP, > > + ggtt_size); > > + if (err) { > > + destroy_workqueue(ggtt->wq); > > + return err; > > + } > > + } else { > > + __xe_ggtt_init_early(ggtt, ggtt_start, ggtt_size, > > + ggtt_start, ggtt_size); > > + } > > > > err = drmm_add_action_or_reset(&xe->drm, ggtt_fini_early, ggtt); > > if (err) > > @@ -459,7 +538,7 @@ static void xe_ggtt_initial_clear(struct xe_ggtt *ggtt) > > /* Display may have allocated inside ggtt, so be careful with clearing > > here */ > > mutex_lock(&ggtt->lock); > > drm_mm_for_each_hole(hole, &ggtt->mm, start, end) > > - xe_ggtt_clear(ggtt, ggtt->start + start, end - start); > > + xe_ggtt_clear(ggtt, ggtt->mm_offset + start, end - start); > > > > xe_ggtt_invalidate(ggtt); > > mutex_unlock(&ggtt->lock); > > @@ -613,6 +692,7 @@ void xe_ggtt_shift_nodes(struct xe_ggtt *ggtt, u64 > > new_start) > > > > /* pairs with READ_ONCE in xe_ggtt_node_addr() */ > > WRITE_ONCE(ggtt->start, new_start); > > + WRITE_ONCE(ggtt->mm_offset, new_start); > > } > > > > static int xe_ggtt_insert_node_locked(struct xe_ggtt_node *node, > > @@ -815,20 +895,19 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt > > *ggtt, struct xe_bo *bo, > > > > mutex_lock(&ggtt->lock); > > /* > > - * When inheriting the initial framebuffer, the framebuffer is > > - * physically located at VRAM address 0, and usually at GGTT address 0 > > too. > > - * > > - * The display code will ask for a GGTT allocation between end of BO and > > - * remainder of GGTT, unaware that the start is reserved by WOPCM. > > + * Convert caller-supplied hardware GGTT addresses to drm_mm-relative > > + * coordinates. For PF mm_offset is 0 so this is a no-op; callers pass > > + * real addresses and the reserved_bottom/top nodes prevent allocations > > + * in the forbidden regions. For VF, mm_offset equals the VF base so > > + * we convert to relative drm_mm space. > > */ > > - if (start >= ggtt->start) > > - start -= ggtt->start; > > + if (start >= ggtt->mm_offset) > > + start -= ggtt->mm_offset; > > else > > start = 0; > > > > - /* Should never happen, but since we handle start, fail graciously for > > end */ > > - if (end >= ggtt->start) > > - end -= ggtt->start; > > + if (end >= ggtt->mm_offset) > > + end -= ggtt->mm_offset; > > else > > end = 0; > > > > @@ -923,7 +1002,7 @@ u64 xe_ggtt_largest_hole(struct xe_ggtt *ggtt, u64 > > alignment, u64 *spare) > > > > mutex_lock(&ggtt->lock); > > drm_mm_for_each_hole(entry, mm, hole_start, hole_end) { > > - hole_start = max(hole_start, ggtt->start); > > + hole_start = max(hole_start, ggtt->mm_offset); > > hole_start = ALIGN(hole_start, alignment); > > hole_end = ALIGN_DOWN(hole_end, alignment); > > if (hole_start >= hole_end) > > @@ -1098,7 +1177,7 @@ u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 > > alignment, struct drm_printer > > > > mutex_lock(&ggtt->lock); > > drm_mm_for_each_hole(entry, mm, hole_start, hole_end) { > > - hole_start = max(hole_start, ggtt->start); > > + hole_start = max(hole_start, ggtt->mm_offset); > > hole_start = ALIGN(hole_start, alignment); > > hole_end = ALIGN_DOWN(hole_end, alignment); > > if (hole_start >= hole_end) > > @@ -1152,7 +1231,7 @@ u64 xe_ggtt_read_pte(struct xe_ggtt *ggtt, u64 offset) > > u64 xe_ggtt_node_addr(const struct xe_ggtt_node *node) > > { > > /* pairs with WRITE_ONCE in xe_ggtt_shift_nodes() */ > > - return node->base.start + READ_ONCE(node->ggtt->start); > > + return node->base.start + READ_ONCE(node->ggtt->mm_offset); > > } > > > > /** >
