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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, May 29, 2026 at 01:50:34PM +0300, Ville Syrjälä wrote:
> 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.

right, so basically we could keep the xe_ggtt as is and provide
2 hooks:

1. one to reserve the portion of the BIOS FB that goes
inside our managed ggtt area
2. a special clear for this area

And in between you do the rebind with existing infrastructure
to an empty region?! Is this what you are thinking now?

> 
> 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...

The resume should only restore the pinned bo's one by one, nothing
special. So I guess if we keep the original code we are okay,
but if we start to managing the full range with the reserved areas
we might have some difficulties here on the way...

> 
> -- 
> Ville Syrjälä
> Intel