Re: [PATCH] drm/xe/ggtt: use full-range drm_mm with reserved nodes on PF

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, May 28, 2026 at 06:28:47PM -0400, Rodrigo Vivi wrote:
> 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 <daniele.ceraolospurio@intel.com>
> > 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/20260511214122.8468-1-ville.syrjala@linux.intel.com/
> 
> And more specifically the discussion in this patch:
> https://lore.kernel.org/intel-xe/20260511214122.8468-13-ville.syrjala@linux.intel.com/
> 
> > 
> > > 
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Assisted-by: GitHub-Copilot:claude-sonnet-4.6
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  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.

Basically what the display needs is:
1. specify where in ggtt the buffer was originally placed by the GOP,
   this may be partially or fully inside these GuC reserved ranges
2. bind the buffer to some acceptable location (assuming the original
   location wasn't acceptable) without overwriting the PTEs for the
   original location

I suppose this could be achieved even with this "mm doesn't cover the
ends" hack, but step 1 there becomes a bit dodgy because we can't
insert the mm node if it's fully outside the mm. I suppose it could 
still work if you hide it in a function that only validates the real
ggtt offsets, but then ignores the fact that the node can't be
inserted due to being fully inside those reserved ranges. And then
whatever cleans up that original mm node must also ignore the fact
that the node maybe wasn't even allocated. And also
xe_ggtt_initial_clear() will need special code to clear the
reserved ranges.

My original idea was that we'd just include the reserved regions
in the mm, and then the display could just keep the buffer at its
original location, and later the guc code can reserve what is
left over. So we could skip step 2 above completely. But after
a second thought we probably don't want to skip that step because
we might free the display bo later, at which point we might free
up some of the reserved ranges. So I guess we'd still want to keep
step 2. But I think it'd still result in less special cases in the
code. We'd just need the guc code to reserve what it needs, after
the display code has rebound the bo to an acceptable location.

So we'd end up with:
1. insert node for the bo's original ggtt location
2. rebind the display bo to an acceptable ggtt location
3. undo step 1
4. xe_ggtt_initial_clear() (now also clears the reserved ranges
   without any special code)
5. guc steals the reserved ranges explicitly

So only two special cases left really, and all the rest
of the code is blissfully unaware of any of it.

Hmm, although hibernation might still be a slight issue for
xe_ggtt_initial_clear(). As in how would the reserved regions
get cleared during resume from hibernation? I have no idea 
how the current xe ggtt code handles resume at all...

-- 
Ville Syrjälä
Intel