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