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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hello,

On 6/3/26 15:44, Rodrigo Vivi wrote:
> On Wed, Jun 03, 2026 at 04:19:41PM +0300, Ville Syrjälä wrote:
>> On Fri, May 29, 2026 at 05:03:55PM -0400, Rodrigo Vivi wrote:
>>> 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.
>>
>> Looks like currently xe_ggtt_initial_clear() is never even called
>> during resume from hibernation, so in that case parts of the GGTT
>> will be left with whatever garbage the GOP put there. So that's
>> one thing that needs fixing.
>>
>>> 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...
>>
>> If we had a full range mm I suppose we'd need a bit of special code
>> to remove the reserved nodes before xe_ggtt_initial_clear() gets
>> called, at least for the resume from hibernation case.
> 
> it looks like Maarten suggestion fix the clear portion.
> But for the steps 1 and 3 above we would need a special reservation
> with a node area only within our managed area?!
> 
> something like (for step 1):
> 
> mm_start = max(start, ggtt->start);
> mm_end = min(start + size, ggtt->start + ggtt->size);
> 
> node->base.start = mm_start - ggtt->start;
> node->base.size = mm_end - mm_start;
> 
> drm_mm_reserve_node(&ggtt->mm, &node->base);

I completely missed this, but that function exists.

Extend xe_ggtt_insert_node to xe_ggtt_insert_node_at, and xe_ggtt_node_remove.
The display code needs to truncate start/end/size as needed, because
size is passed as argument.

The proposal I made in the original series that spawned this
discussion, had the same idea. You can argue the clipping should be done by
xe_ggtt, in which case I'm open for a xe_ggtt_node_reserve_region(begin, end),
with clipping done by xe_ggtt. But I believe xe_ggtt_insert_node_at is more
generic, and can be used in more places.

This also removes the workaround we added that required adding 'xe_ggtt_insert_bo_at'

> 
> and another special function to delete this special node
> if needed to be created?
> 
> Or what do you have on mind for the full area? I believe the full
> area is this patch, but with some additions anyway since we need
> to handle this buffer plus ensure it gets reserved when we don't
> have it...
> 
> I'd like to avoid complications like the phys offset addition or
> the full range if possible.
> 
> Could you please incorporate something simple in a v2 of your series?
> 
> Thanks,
> Rodrigo.
> 
>>
>> -- 
>> Ville Syrjälä
>> Intel

Kind regards,
~Maarten Lankhorst